test(e2e): journey-based policy + anti-regrowth guard + migrate all non-Ordered specs (senm.4, y59n.4, 0cyn) #600

Merged

zombor merged 3 commits from bd-bookshelf-senm.4 into main 2026-06-18 02:16:14 +00:00

Conversation 2 Commits 3 Files changed 12 +601 -540

zombor commented 2026-06-18 01:35:58 +00:00

Owner

Copy link

Migrates all e2e specs to Ordered journeys; adds AST policy check guard wired into lint+CI; amends CLAUDE.md with API+browser e2e policy. Closes bead bookshelf-senm.4, bookshelf-y59n.4, bookshelf-0cyn on merge.

Migrates all e2e specs to Ordered journeys; adds AST policy check guard wired into lint+CI; amends CLAUDE.md with API+browser e2e policy. Closes bead bookshelf-senm.4, bookshelf-y59n.4, bookshelf-0cyn on merge.

zombor added 1 commit 2026-06-18 01:35:58 +00:00

test(e2e): lock in journey-based policy + anti-regrowth guard + migrate all non-Ordered specs (senm.4, y59n.4, 0cyn) ... 7dcf16c6ca

- Add E2E Testing Policy section to CLAUDE.md (repo root) codifying:
  - API e2e: ~8 Ordered journeys only; no per-spec BeforeEach(ResetDB);
    multi-Expect relaxation for journey Its documented
  - Browser e2e: real-Chromium-only journeys; Vitest for controller logic;
    API tests for server HTML; go-rod absolute-deadline gotcha documented
- Add scripts/e2e_policy_check/main.go: Go AST checker that fails if any
  e2e/api/ or e2e/browser/ top-level Describe is missing Ordered
- Wire e2e-policy-check into make lint and CI lint job
- Migrate e2e/api/audit_log_test.go to Ordered journey (5 It-steps, fresh env)
- Migrate e2e/api/comics_test.go to Ordered journey (2 It-steps, fresh env)
- Migrate e2e/api/bookdrop_extract_pattern_test.go to single Ordered journey
  (8 It-steps replacing 2 Describes with per-spec BeforeEach)
- Migrate e2e/browser/bookdrop_extract_pattern_test.go: add Ordered,
  BeforeEach -> BeforeAll, cleanup via DeferCleanup
- Migrate e2e/browser/metadata_fetch_cover_apply_test.go: add Ordered,
  move ResetDB + seed to BeforeAll
- Fix helpers_test.go seedBook: use env.DB (not suiteEnv.DB) so journey-local
  envs get the fixture in the right database

Closes bead bookshelf-senm.4, bookshelf-y59n.4, bookshelf-0cyn on merge.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-06-18 01:58:08 +00:00

Author

Owner

Copy link

Security Review — PR #600 (bd-bookshelf-senm.4)

Scope: e2e policy lock-in — CLAUDE.md doc edits, new AST guard script (scripts/e2e_policy_check/main.go), Makefile + ci.yml wiring, migrated e2e tests. No production code.

Guard script (scripts/e2e_policy_check/main.go)

• No exec / shell-out: uses only go/ast, go/parser, go/token, os, path/filepath, strings, fmt. No os/exec or equivalent.
• No arbitrary file writes: all output goes to os.Stdout/os.Stderr. No os.Create, os.WriteFile, or ioutil.WriteFile.
• No symlink traversal risk: filepath.Join(root, "e2e", "api") / filepath.Join(root, "e2e", "browser") constructs a fixed two-level path. os.ReadDir on the resulting dir walks only that directory (non-recursive). A malicious root arg could point elsewhere, but in CI root defaults to . (no arg passed in ci.yml step) and the binary only reads _test.go files — no write path exists.
• No path traversal: entry.Name() from os.ReadDir is a bare filename (no ..); filepath.Join(dir, entry.Name()) cannot escape the fixed directories.
• No secrets/sensitive data access: the script only reads Go source files ending in _test.go.

ci.yml change

• New step go run ./scripts/e2e_policy_check runs in the existing lint job context with the same permissions as every other lint step — no elevated perms, no new secret bindings.
• go run compiles and runs entirely from the repo working tree; no network fetch, no untrusted code pulled in.

Migrated e2e tests

• All changed files are in e2e/api/ and e2e/browser/ — dev/test code only.
• No hardcoded real credentials, API keys, or tokens found in any migrated file. Auth helpers use the existing env.AuthClient / env.BaseURL pattern from testutil.
• No production code (internal/, cmd/, static/, templates/) was modified — confirmed via git diff --name-only.

CLAUDE.md

• Documentation-only addition. No executable content.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #600 (bd-bookshelf-senm.4) Scope: e2e policy lock-in — CLAUDE.md doc edits, new AST guard script (`scripts/e2e_policy_check/main.go`), Makefile + ci.yml wiring, migrated e2e tests. No production code. ### Guard script (`scripts/e2e_policy_check/main.go`) - **No exec / shell-out:** uses only `go/ast`, `go/parser`, `go/token`, `os`, `path/filepath`, `strings`, `fmt`. No `os/exec` or equivalent. - **No arbitrary file writes:** all output goes to `os.Stdout`/`os.Stderr`. No `os.Create`, `os.WriteFile`, or `ioutil.WriteFile`. - **No symlink traversal risk:** `filepath.Join(root, "e2e", "api")` / `filepath.Join(root, "e2e", "browser")` constructs a fixed two-level path. `os.ReadDir` on the resulting dir walks only that directory (non-recursive). A malicious `root` arg could point elsewhere, but in CI `root` defaults to `.` (no arg passed in ci.yml step) and the binary only reads `_test.go` files — no write path exists. - **No path traversal:** `entry.Name()` from `os.ReadDir` is a bare filename (no `..`); `filepath.Join(dir, entry.Name())` cannot escape the fixed directories. - **No secrets/sensitive data access:** the script only reads Go source files ending in `_test.go`. ### ci.yml change - New step `go run ./scripts/e2e_policy_check` runs in the existing `lint` job context with the same permissions as every other lint step — no elevated perms, no new secret bindings. - `go run` compiles and runs entirely from the repo working tree; no network fetch, no untrusted code pulled in. ### Migrated e2e tests - All changed files are in `e2e/api/` and `e2e/browser/` — dev/test code only. - No hardcoded real credentials, API keys, or tokens found in any migrated file. Auth helpers use the existing `env.AuthClient` / `env.BaseURL` pattern from `testutil`. - No production code (`internal/`, `cmd/`, `static/`, `templates/`) was modified — confirmed via `git diff --name-only`. ### CLAUDE.md - Documentation-only addition. No executable content. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-18 02:00:06 +00:00

Author

Owner

Copy link

CODE REVIEW — PR #600 (bd-bookshelf-senm.4)

Phase 0: DEMO Verification

No DEMO block in this PR. The bead is a policy/tooling PR (AST guard + spec migration) — there is no user-facing runtime feature to demo. CI green on all 6 jobs is the verification signal.

---

Phase 1 + Phase 2: Findings

[MINOR] CLAUDE.md:E2E Testing Policy — Budget numbers are stale
~8 journeys for API and ~6 ordered interactive journeys for browser are already out of date: the branch has 13 top-level Describes in e2e/api/ and 12 in e2e/browser/ (counting across all files with grep -c "^var _ = Describe"). The numbers were accurate at some earlier point in the epic but this PR adds audit_log + comics + bookdrop_extract_pattern to API and bookdrop_extract_pattern + metadata_fetch_cover_apply to browser. These are approximate (~) so they are not wrong per se, but a reader expecting "~8" and finding 13 will be confused. Suggest updating to ~13 / ~12 or dropping the exact count and just saying "a small, bounded set."

[MINOR] .claude/rules/project-conventions.md:364-366 — Old per-endpoint rule contradicts new policy
project-conventions.md line 366 still says "Every HTTP route gets at least one e2e test in e2e/api/ covering status code, content type, and key body assertions." This directly contradicts the new CLAUDE.md E2E Testing Policy that bans per-endpoint e2e specs and replaces them with unit tests. CLAUDE.md says it is the "Authoritative policy" and that project-conventions "live[s] in .claude/rules/project-conventions.md" for the full framework — but the old rule is still there and an implementer reading only project-conventions.md will write a fresh per-endpoint test and the guard will NOT catch it (because the guard only checks the Ordered modifier, not the test structure). The contradiction should be resolved by removing or replacing that line with a pointer to CLAUDE.md.

[MINOR] scripts/e2e_policy_check/main.go — No self-test for the guard
The guard has no *_test.go alongside it. A future change to checkFile or hasOrdered could silently regress (false negatives). A small table-driven test with two synthetic source strings — one Ordered, one not — would catch this. Not a blocker (the guard is simple enough to audit by reading), but it leaves correctness entirely on code review.

---

Guard Correctness Analysis (explicit per the review request)

Detection logic: The guard walks f.Decls (top-level file declarations only), finds *ast.GenDecl with token.VAR, then *ast.ValueSpec where all names are _, then *ast.CallExpr where Fun is an *ast.Ident named "Describe". It then checks whether any argument is an *ast.Ident named "Ordered". This is correct for the project's conventions.

No false positives: All 13 API Describes and 12 browser Describes in the branch include Ordered as a bare identifier. The guard passes all of them. var _ = BeforeSuite(...) and var _ = testutil.RegisterLogDumpFor(...) in suite files are correctly skipped (not Describe).

No false negatives: A new file with var _ = Describe("...", func() { ... }) (no Ordered) would be flagged. The logic is correct.

Scope is correct: Only e2e/api/ and e2e/browser/ are scanned. e2e/testutil/server_test.go has a non-Ordered Describe for the connection-budget guard — correctly excluded from the scan since it is in e2e/testutil/, not e2e/api/ or e2e/browser/.

One theoretical gap (not actionable): isDescribe() only catches bare Describe identifiers, not ginkgo.Describe (a *ast.SelectorExpr). A developer who imported ginkgo without a dot import and wrote ginkgo.Describe("x", func(){}) would bypass the check. However, all existing and new e2e files use . "github.com/onsi/ginkgo/v2" (dot import), making Ordered and Describe bare identifiers. This is an acceptable trade-off given the project's consistent convention. Documenting this assumption in the guard's package comment would be a minor improvement.

CI wiring: make e2e-policy-check invokes go run ./scripts/e2e_policy_check and exits non-zero on violation. It is called from make lint and as a standalone CI step in .forgejo/workflows/ci.yml (lint job). os.Exit(1) on violations and os.Exit(2) on parse errors both cause CI failure. Correct.

helpers_test.go fix: All 12 suiteEnv.DB. calls changed to env.DB.. This is correct — seedBook now operates on the journey-local TestEnv.DB rather than the suite-level DB, which is necessary for journeys that boot their own fresh DB via NewSuiteServerWithConfig. The fix prevents cross-contamination between per-journey DBs.

---

REVIEW VERDICT: 0 blocker, 0 major, 3 minor

## CODE REVIEW — PR #600 (bd-bookshelf-senm.4) ### Phase 0: DEMO Verification No DEMO block in this PR. The bead is a policy/tooling PR (AST guard + spec migration) — there is no user-facing runtime feature to demo. CI green on all 6 jobs is the verification signal. --- ### Phase 1 + Phase 2: Findings [MINOR] CLAUDE.md:E2E Testing Policy — Budget numbers are stale `~8 journeys` for API and `~6 ordered interactive journeys` for browser are already out of date: the branch has 13 top-level Describes in `e2e/api/` and 12 in `e2e/browser/` (counting across all files with `grep -c "^var _ = Describe"`). The numbers were accurate at some earlier point in the epic but this PR adds audit_log + comics + bookdrop_extract_pattern to API and bookdrop_extract_pattern + metadata_fetch_cover_apply to browser. These are approximate (`~`) so they are not wrong per se, but a reader expecting "~8" and finding 13 will be confused. Suggest updating to `~13` / `~12` or dropping the exact count and just saying "a small, bounded set." [MINOR] `.claude/rules/project-conventions.md`:364-366 — Old per-endpoint rule contradicts new policy `project-conventions.md` line 366 still says "Every HTTP route gets at least one e2e test in `e2e/api/` covering status code, content type, and key body assertions." This directly contradicts the new `CLAUDE.md` E2E Testing Policy that bans per-endpoint e2e specs and replaces them with unit tests. CLAUDE.md says it is the "Authoritative policy" and that project-conventions "live[s] in `.claude/rules/project-conventions.md`" for the full framework — but the old rule is still there and an implementer reading only project-conventions.md will write a fresh per-endpoint test and the guard will NOT catch it (because the guard only checks the Ordered modifier, not the test structure). The contradiction should be resolved by removing or replacing that line with a pointer to CLAUDE.md. [MINOR] `scripts/e2e_policy_check/main.go` — No self-test for the guard The guard has no `*_test.go` alongside it. A future change to `checkFile` or `hasOrdered` could silently regress (false negatives). A small table-driven test with two synthetic source strings — one Ordered, one not — would catch this. Not a blocker (the guard is simple enough to audit by reading), but it leaves correctness entirely on code review. --- ### Guard Correctness Analysis (explicit per the review request) **Detection logic:** The guard walks `f.Decls` (top-level file declarations only), finds `*ast.GenDecl` with `token.VAR`, then `*ast.ValueSpec` where all names are `_`, then `*ast.CallExpr` where `Fun` is an `*ast.Ident` named `"Describe"`. It then checks whether any argument is an `*ast.Ident` named `"Ordered"`. This is correct for the project's conventions. **No false positives:** All 13 API Describes and 12 browser Describes in the branch include `Ordered` as a bare identifier. The guard passes all of them. `var _ = BeforeSuite(...)` and `var _ = testutil.RegisterLogDumpFor(...)` in suite files are correctly skipped (not `Describe`). **No false negatives:** A new file with `var _ = Describe("...", func() { ... })` (no `Ordered`) would be flagged. The logic is correct. **Scope is correct:** Only `e2e/api/` and `e2e/browser/` are scanned. `e2e/testutil/server_test.go` has a non-Ordered Describe for the connection-budget guard — correctly excluded from the scan since it is in `e2e/testutil/`, not `e2e/api/` or `e2e/browser/`. **One theoretical gap (not actionable):** `isDescribe()` only catches bare `Describe` identifiers, not `ginkgo.Describe` (a `*ast.SelectorExpr`). A developer who imported ginkgo without a dot import and wrote `ginkgo.Describe("x", func(){})` would bypass the check. However, all existing and new e2e files use `. "github.com/onsi/ginkgo/v2"` (dot import), making `Ordered` and `Describe` bare identifiers. This is an acceptable trade-off given the project's consistent convention. Documenting this assumption in the guard's package comment would be a minor improvement. **CI wiring:** `make e2e-policy-check` invokes `go run ./scripts/e2e_policy_check` and exits non-zero on violation. It is called from `make lint` and as a standalone CI step in `.forgejo/workflows/ci.yml` (lint job). `os.Exit(1)` on violations and `os.Exit(2)` on parse errors both cause CI failure. Correct. **helpers_test.go fix:** All 12 `suiteEnv.DB.` calls changed to `env.DB.`. This is correct — `seedBook` now operates on the journey-local `TestEnv.DB` rather than the suite-level DB, which is necessary for journeys that boot their own fresh DB via `NewSuiteServerWithConfig`. The fix prevents cross-contamination between per-journey DBs. --- REVIEW VERDICT: 0 blocker, 0 major, 3 minor

zombor referenced this pull request from a commit 2026-06-18 02:08:19 +00:00

docs(e2e-policy): fix project-conventions per-endpoint rule + CLAUDE.md journey counts

zombor force-pushed bd-bookshelf-senm.4 from 7dcf16c6ca to feb7bbb15f 2026-06-18 02:08:19 +00:00 Compare

zombor added 1 commit 2026-06-18 02:12:53 +00:00

fix(e2e): convert settings_match_weights_test to Ordered journey (policy guard) ... 7cc39a7d83

The e2e-policy-check guard (added in this PR) correctly flagged the file
as the first real violation — a standalone non-Ordered Describe with
per-spec BeforeEach(ResetDB). Converted to a proper Ordered journey with
a single BeforeAll that boots DB+app once and seeds a non-admin user;
three sequential Its cover the three auth gates (401, 403, 202) with all
original assertions preserved.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor merged commit 7409af32e7 into main 2026-06-18 02:16:14 +00:00

zombor referenced this pull request from a commit 2026-06-18 02:16:17 +00:00

Merge pull request 'test(e2e): journey-based policy + anti-regrowth guard + migrate all non-Ordered specs (senm.4, y59n.4, 0cyn)' (#600) from bd-bookshelf-senm.4 into main

zombor referenced this pull request from a commit 2026-06-18 02:59:49 +00:00

feat(dedup): move-files-to-target-dir after merge (bookshelf-c15a.3)

zombor referenced this pull request from a commit 2026-06-18 03:14:51 +00:00

feat(dedup): move-files-to-target-dir after merge (bookshelf-c15a.3)

zombor referenced this pull request from a commit 2026-06-18 13:24:54 +00:00

feat(dedup): move-files-to-target-dir after merge (bookshelf-c15a.3)

zombor referenced this pull request from a commit 2026-06-18 14:10:49 +00:00

feat(dedup): move-files-to-target-dir after merge (bookshelf-c15a.3)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!600

No description provided.