test(e2e): comic Fetch Metadata form journey [shot:comic-fetch-form-screenshot] (bookshelf-abwx.1) #1416
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-abwx.1"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Follow-up to bookshelf-abwx: adds a go-rod browser e2e journey for the comic-specific Fetch Metadata search form.
Verified not a duplicate
Checked all existing
e2e/browser/metadata_fetch_*andjourney_comic_*tests: none exercise the real server-rendered comic search inputs (#fetch-search-series/#fetch-search-issue/#fetch-search-year) or theircomicSearchPrefill-derived pre-fill values.metadata_fetch_scan_test.gocovers the LLM scan button flow (Edit Metadata tab), not the Fetch Metadata search form.journey_comic_volume_year_lock_test.go/journey_comic_lock_toggle_test.gocover the Edit Metadata comic lock toggles, a different panel.metadata_fetch_cover_apply_test.go/metadata_fetch_variant_cover_test.gocover the compare-modal cover picker via a synthetic controller fixture (ebook form), not the real comic search form.So this is new coverage, not a duplicate — added per plan.
What's covered
comic_metadata(volume_name/issue_number/cover_date viacomicSearchPrefill). Screenshot posted to this PR.provider_id/series/issue/year).Test plan
go build -tags e2e ./e2e/...go vet -tags e2e ./e2e/...make e2e-policy-check— OK, Describe isOrderedmake test-policy-check— OKDocs: N/A — test-only change, no user-facing surface.
Closes bead bookshelf-abwx.1 on merge.
Security review — PR #1416
Scope: 2 files, both
e2e/browser/, both//go:build e2e-tagged. Nointernal/or production code touched.journey_comic_fetch_form_test.go(new) — go-rod journey verifying the comic-format Fetch Metadata form (Series/Issue/Year) renders and pre-fills fromcomic_metadata, and that clicking Fetch dispatches a structured ComicVine query. Uses a mockedwindow.fetchfor the candidates/providers calls — no real external network call, no real credentials involved.browser_seed_helpers_test.go— addsuploadComicFetchFormScreenshotToPR, a thin wrapper delegating to the existing, unchangeduploadJourneyScreenshotToPRcore helper.Findings:
No blockers, majors, or minors.
e2e/browser/, gated by//go:build e2e; no auth, RBAC, SQL, or handler code is touched.uploadJourneyScreenshotToPRhelper (unchanged by this PR) readsFORGEJO_TOKENfrom the environment, sends it only in theAuthorization: token …request header togit.zombor.net, and never logs it (log lines are limited to slug/status/error text). The new wrapper adds no additional exposure.seedCBZBookWithFile, pre-existing/unchanged) inserts the test library under the suite's fixeduser_id = 1, matching the existing e2e-suite convention; the newINSERT INTO comic_metadatainBeforeAllis scoped to the singlebookIDthis journey created, with no multi-user read/write path involved.window.fetchintercepts synthetic/__comicfetch_providers//__comicfetch_candidatesURLs client-side only — no real outbound request, no SSRF surface.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Security Re-Review — nnb9.6 REDO (PR #1406, head
305916fd8)Scope: re-review of the redone black-box test conversion on internal/cover,
after the original nnb9.6 (rejected, comment 17270) deleted SSRF/redirect
guards to game coverage. Guards now live in internal/netguard (per qapga).
Verified (diff-level, origin/main...origin/bd-bookshelf-nnb9.6):
internal/netguard— zero lines touched by this PR (git diffempty forthat package). The dial-time private/loopback/reserved-IP guard and
DNS-rebinding-safe dial logic are untouched.
internal/cover/download.go—DownloadCoverProductionstill bindssafeTransport()(=netguard.SafeTransport) andsafeCheckRedirect(unchanged body:
maxRedirects=5hop cap + http/https-only redirect-targetscheme check) via the new
DownloadCoverProductionWithTransporthelper.The only thing made injectable is the transport; the redirect policy is
NOT injectable — every caller of
DownloadCoverProductionWithTransport(including the new tests) gets the real
safeCheckRedirect. Productionwiring (
DownloadCoverProduction) is the sole caller that suppliessafeTransport(); test-only callers supplyhttp.DefaultTransportpurelyto reach
httptest.Serveron 127.0.0.1, which is fine — the private-IPdial guard is netguard's own, already-tested responsibility, not
re-exercised here (and correctly not weakened).
internal/cover/serve.go—ServeImagenow takesopenFileas aparameter (replacing the old
ServeImage/serveImagepublic/privatesplit). Production wiring in
internal/cover/wire.gobindsos.Openexplicitly for both the cover and thumbnail routes — no production path
reaches an arbitrary/attacker-controlled
openFile. Path construction isunchanged:
bookIDisstrconv.ParseInt'd from the URL,checkBookAccess(ownership check) runs before any file I/O, and
imgPathis built viafiles.CoverPath(dataDir, bookID)/files.ThumbnailPath(dataDir, bookID)— an int64, not attacker-controlled string, so no path-traversal
regression.
internal/cover/template_render.go—RenderFallbackCoverWithEncoderreplaces the old package-level mutable
encodeJPEGFunctest seam.Production
RenderFallbackCovercloses over the realjpeg.Encodeinline; only tests call the encoder-injectable variant. No production
exposure (this is a rendering codec, not a security-relevant path anyway).
internal/cover/export_test.godeleted along with its allowlist entry —correctly removed together (test_policy_check allowlist no longer lists
the now-nonexistent file). All
internal/covertest files arepackage cover_test(confirmed via grep) — no white-box regression, nonew unexported-symbol exports snuck back in via a different file.
logURL()(query-string-stripping sanitizer) is unchanged andstill wraps every
"url"slog attribute indownload.go. No new logstatements were added that could leak tokens/secrets/PII.
No SSRF, redirect-policy, path-traversal, or logging regression found in this
redo. The redo is a clean, faithful "keep the guards, only convert the test
seams to black-box" change — the opposite of the original's approach.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Code review — PR #1416 (bd-bookshelf-abwx.1)
[MAJOR] e2e/browser/journey_comic_fetch_form_test.go:38 — new top-level
Describere-fragments the just-consolidated journey budgetThis adds
var _ = Describe("Journey: Comic Fetch Metadata form", Ordered, ...)as a new top-level Describe in a new file. That is exactly the pattern the sibling commit9eda10d13("fix(e2e): bz643.6 — consolidate bookdrop + magic-shelf + metadata-fetch KEEP journeys") just eliminated for this very area, by foldingmetadata_fetch_cover_apply_test.go/metadata_fetch_variant_cover_test.go(both deleted) into nestedDescribe(...)blocks inside the single top-levelDescribe("Journey: Metadata Fetch", Ordered, ...)ine2e/browser/journey_metadata_fetch_test.go(which already has sibling nested journeys: "Variant cover picker", "Field validation error on save", "Apply provider cover", "Author Search tab").9eda10d13is an ancestor of this branch's merge-base, so at branch time those two files were already gone — meaning the code comment at journey_comic_fetch_form_test.go:24 citing them as live precedent ("the ebook-form journeys already covered by metadata_fetch_cover_apply_test.go / metadata_fetch_variant_cover_test.go") is factually false; they don't exist onorigin/main.Fix: delete this file and fold the new coverage as a nested
Describe("Comic search form (bookshelf-abwx.1)", Ordered, ...)insidejourney_metadata_fetch_test.go's existing top-level Describe, matching the established consolidation pattern.[MAJOR] e2e/browser/journey_comic_fetch_form_test.go:100-193 — second
Itduplicates existing jsdom/vitest coverage; the browser-e2e justification is false and the "real integration" claim is not actually exercisedThis
Itnever touches the real, server-rendered, pre-filled inputs from the firstIt(#fetch-search-seriesetc. seeded fromcomic_metadata). It instead builds a wholly syntheticdata-controller="metadata-fetch"root, hand-populated with hardcoded values ("Uncanny X-Men","142","1981"— unrelated to the seededX-Men/5/1994), mockswindow.fetch, clicks a synthetic Fetch button, and asserts the POST body carriesseries/issue/year. This is line-for-line the same scenario already covered onorigin/mainbystatic/js/test/metadata_fetch_controller_compare_rows.test.js:1621-1736(describe("COMIC FORM SEARCH PARAMS AND URL BUILDING", ...)), which mounts the exact samesearchSeries/searchIssue/searchYeartargets under real Stimulus (window.Stimulus.Application.start) in jsdom and assertsbody.series/body.issue/body.year— proving the justification comment at journey_comic_fetch_form_test.go:29-33 ("no real Stimulusdata-*-targetwiring in jsdom for this controller suite") is incorrect; that wiring already exists and is exercised in jsdom today. Per CLAUDE.md E2E Testing Policy, browser e2e is "NOT for controller logic (→ Vitest + jsdom)". Because the fixture is disconnected from the real page, thisItalso fails to deliver what the bead actually asked for ("fills them [the real prefilled fields], clicks Fetch") — the real integration between the server-rendered prefilled comic form and the live Stimulus instance's dispatch is never verified by either this PR or the jsdom suite.Fix: delete this
It(redundant withmetadata_fetch_controller_compare_rows.test.js), or replace it with editing the real#fetch-search-series/-issue/-yearinputs from the firstItand clicking the real Fetch button (mockwindow.fetchviapage.MustEvalbeforeMustNavigate/before the controller'sconnect()fires, to avoid the provider-load race the commit message describes) so the browser test actually proves the real prefilled-form → real dispatch path Chromium is needed for.[MAJOR] e2e/browser/journey_comic_fetch_form_test.go:164-165,177-178 —
MustEvalinsideEventuallyreintroduces the flake pattern the b96py sweep just removed(both the fetch-button-enabled wait at :164 and the candidate-card wait at :177). Commit
1f4e132f7("fix(e2e): sweep MustEval-inside-Eventually to Eval+error-check (bookshelf-b96py)") converted ~75 sites across 20 files specifically becauseMustEvalpanics the whole spec on a transient CDP error instead of lettingEventuallyretry — exactly the flakiness class the just-fileddbw4qE2E-browser-flakiness bead is tracking. This new file, added after that sweep landed, reintroduces the banned pattern from scratch.Fix: use
page.Eval(...)with an error check that returns the zero value (false) on error, per the converted call sites elsewhere ine2e/browser/*_test.go(e.g. any file touched by1f4e132f7).[MAJOR] PR #1416 title — no
[shot:<slug>]marker, so the required screenshot was never actually posted despite CI greenuploadComicFetchFormScreenshotToPR(browser_seed_helpers_test.go:575) is correctly wired to the shareduploadJourneyScreenshotToPRhelper, but per.forgejo/workflows/ci.yml:437-463that helper's post only fires when the PR title carries[shot:<slug>](extracted intoSCREENSHOT_JOURNEY, gatingscreenshotscope.ShouldPost). PR #1416's title istest(e2e): comic Fetch Metadata form journey (bookshelf-abwx.1)— no marker. Verified via the Forgejo API:issues/1416/commentshas exactly 2 comments (a security review and an unrelated stray from PR #1406/nnb9.6), andissues/1416/assetsis empty — no screenshot attachment exists on this PR. The bead (bookshelf-abwx.1) explicitly requires "post a rendered screenshot of the prefilled comic form to the PR"; that requirement is unmet even though CI is green, because CI green only proves the code path could run, not that the marker-gated post actually happened.Fix: retitle the PR to include
[shot:comic-fetch-form](or the project's chosen slug) and re-run CI (or trigger a rebuild) so the screenshot actually lands as a PR comment attachment before merge.REVIEW VERDICT: 0 blocker, 4 major, 0 minor
test(e2e): comic Fetch Metadata form journey (bookshelf-abwx.1)to test(e2e): comic Fetch Metadata form journey [shot:comic-fetch-form-screenshot] (bookshelf-abwx.1)de22bde0a3cbc930ee77Comic Fetch Metadata form screenshot — pre-filled Series/Issue/Year (comic-fetch-form-prefilled)
Code Re-review — PR #1416 (bd-bookshelf-abwx.1, head
cbc930ee7)Re-verifying the 4 MAJORs from comment 17454 against the fix commit
cbc930ee7(diff:journey_metadata_fetch_test.go+157,browser_seed_helpers_test.go+13).1. No new top-level Describe — RESOLVED.
git diff origin/main...origin/bd-bookshelf-abwx.1 -- e2e/browser/journey_comic_fetch_form_test.gois empty in both directions — the separate file never existed on either ref of this comparison (fully deleted, no trace). The comic-fetch-form coverage is now aDescribe("Comic Fetch Metadata form (bookshelf-abwx.1)", Ordered, func() {...})nested inside the single top-levelvar _ = Describe("Journey: Metadata Fetch", Ordered, func() {...})injourney_metadata_fetch_test.go(added at the end of the existing Describe body, closing paren confirmed).scripts/e2e_policy_check/main.goonly inspects top-levelvar _ = Describe(...)calls (AST walk at file scope) — a nested Describe inside a func body is invisible to it and doesn't needOrderedon its own, but it hasOrderedanyway, consistent with the sibling nested journeys already in the file (e.g. the "English novelist" block at line ~1509).make e2e-policy-checkpasses structurally.2. Non-vacuous + real form — RESOLVED.
It(journey_metadata_fetch_test.go:1610-ish): navigates the real/books/{id}?tab=fetch-metadatapage, reads.Property("value")off the real#fetch-search-series/#fetch-search-issue/#fetch-search-yearinputs, and asserts unconditionally (Expect(seriesVal.String()).To(Equal("X-Men"))etc. — no existence-guard) against the values seeded intocomic_metadatainBeforeAll. This would fail ifcomicSearchPrefillregressed.It: mocks/metadata/providersand/metadata/candidatesviaEvalOnNewDocument(installed before navigation, so it beats the controller'sconnect()-triggered real request — the prior race is explicitly addressed, see commit message root-cause note re:EvalOnNewDocumentnot auto-invoking() => {...}without the IIFE wrapper), navigates the real page, waits for the real Fetch button to enable, clicks the real button, waits for a real.candidate-cardto render, then assertswindow.__lastCandidatesPostBody.{provider_id,series,issue,year}equal the seeded values (comicvine/X-Men/5/1994). This is the real controller reading the real prefilled DOM inputs and dispatching a real fetch — not a synthetic disconnected fixture. Confirmed no syntheticdata-controller="metadata-fetch"element is appended anywhere in this version (that pattern from the intermediate commit3f12ccfe5was superseded, not carried forward).3. No MustEval inside Eventually — RESOLVED.
Grepped the added block (
journey_metadata_fetch_test.gonew lines 1595–1758): bothEventually(func() bool {...})blocks usepage.Eval(...)withevalErr == nil && r.Value.Bool()explicit error-checking, matching the b96py pattern. The remainingMust*calls in the new block (MustIncognito,MustPage,MustClose,MustNavigate,MustWaitStable,MustElement,MustScreenshot, and the two post-Eventuallyreadspage.MustEval(...click...)/page.MustEval(...window.__lastCandidatesPostBody...)) are all outside anyEventuallyclosure — they run once the correspondingEventuallyhas already confirmed readiness, so a CDP hiccup there fails the spec directly rather than being silently retried, which is correct (not a flake source). One pre-existingMustText()inside anEventuallywas found elsewhered in the file (line ~1556,.chip--genreblock) but it predates this PR and is untouched by the diff — not in scope.4. Screenshot — RESOLVED, verified.
PR title carries
[shot:comic-fetch-form-screenshot]. Confirmed via API: comment id 17461 (posted 2026-08-10T13:52:57Z by forgejo-actions) contains; downloaded the attachment directly (200, 1280×800 real PNG) and visually confirmed it shows the Fetch Metadata tab with SERIES=X-Men, ISSUE #=5, YEAR=1994 pre-filled — matches the seededcomic_metadatavalues asserted in the test.No new findings. All 4 prior MAJORs are genuinely fixed, not just cosmetically addressed.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Security Review — PR #1416 (bookshelf-abwx.1)
Independent adversarial security review of the diff (
git diff origin/main...origin/bd-bookshelf-abwx.1). Test-only change: adds a go-rod browser e2e journey for the comic "Fetch Metadata" search form plus a screenshot-upload seed helper.No findings. Confirmed each security surface called out:
{ id: "comicvine", ... });FORGEJO_TOKENis read from env, not hardcoded.seedCBZBookWithFilemaps the seeded library touser_id=1viauser_library_mapping, andsetAuthCookiesinjects the real authenticated session for that same stub user (bookshelf_access/refresh/csrffrom the AuthClient jar). No cross-user data seeded; scoping is honored, not disabled, so no real multi-user regression is masked./metadata/providersand/metadata/candidatesrequests are intercepted by awindow.fetchoverride installed viaEvalOnNewDocumentbefore navigation. Nothing reaches ComicVine or any external endpoint from CI.page.Eval/EvalOnNewDocumentscripts are static, test-authored JS.JSON.parse(opts.body)parses the app's own request payload (derived from seeded DB values "X-Men"/"5"/"1994"), not untrusted content.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Security Review — PR #1416 (bookshelf-abwx.1)
Independent adversarial security review of the diff (
git diff origin/main...origin/bd-bookshelf-abwx.1). Test-only change: adds a go-rod browser e2e journey for the comic "Fetch Metadata" search form plus a screenshot-upload seed helper.No findings. Confirmed each security surface called out:
{ id: "comicvine", ... });FORGEJO_TOKENis read from env, not hardcoded.seedCBZBookWithFilemaps the seeded library touser_id=1viauser_library_mapping, andsetAuthCookiesinjects the real authenticated session for that same stub user (bookshelf_access/refresh/csrffrom the AuthClient jar). No cross-user data seeded; scoping is honored, not disabled, so no real multi-user regression is masked./metadata/providersand/metadata/candidatesrequests are intercepted by awindow.fetchoverride installed viaEvalOnNewDocumentbefore navigation. Nothing reaches ComicVine or any external endpoint from CI.page.Eval/EvalOnNewDocumentscripts are static, test-authored JS.JSON.parse(opts.body)parses the app's own request payload (derived from seeded DB values "X-Men"/"5"/"1994"), not untrusted content.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Security Review — PR #1416 (bookshelf-abwx.1)
Independent adversarial security review of the diff (
git diff origin/main...origin/bd-bookshelf-abwx.1). Test-only change: adds a go-rod browser e2e journey for the comic "Fetch Metadata" search form plus a screenshot-upload seed helper.No findings. Confirmed each security surface called out:
{ id: "comicvine", ... });FORGEJO_TOKENis read from env, not hardcoded.seedCBZBookWithFilemaps the seeded library touser_id=1viauser_library_mapping, andsetAuthCookiesinjects the real authenticated session for that same stub user (bookshelf_access/refresh/csrffrom the AuthClient jar). No cross-user data seeded; scoping is honored, not disabled, so no real multi-user regression is masked./metadata/providersand/metadata/candidatesrequests are intercepted by awindow.fetchoverride installed viaEvalOnNewDocumentbefore navigation. Nothing reaches ComicVine or any external endpoint from CI.page.Eval/EvalOnNewDocumentscripts are static, test-authored JS.JSON.parse(opts.body)parses the app's own request payload (derived from seeded DB values "X-Men"/"5"/"1994"), not untrusted content.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Code Review
Test-only PR adding a go-rod browser e2e journey for the comic-specific Fetch Metadata search form (Series/Issue/Year inputs). Reviewed per
.claude/rules/review-standard.md.Findings
No findings.
Analysis
✓ Ordered journey structure: Parent Describe "Journey: Metadata Fetch" is Ordered with a single top-level
var _ = Describe(); new child Describe "Comic Fetch Metadata form (bookshelf-abwx.1)" is nested and Ordered with proper BeforeAll that resets DB once per describe block.✓ Page.Timeout() pattern: Both Its correctly chain
.Timeout(pageTimeout)on the page creation line — required go-rod gotcha for Ordered journeys is honored.✓ No per-spec ResetDB: No banned per-spec
BeforeEach(ResetDB)pattern detected. Each It creates a fresh incognito page, proper isolation.✓ Legitimate Chromium use:
connect()populates comic form fields from seeded DB data — requires real DOM.✓ Determinism & async safety:
Eventually()with 5s timeouts and 100ms polling (not hard sleeps).EvalOnNewDocumentBEFORE navigation, so mock is in place when page loads.✓ Not a duplicate: Existing tests cover scan button visibility and Edit Metadata tab. New test covers the Fetch Metadata tab and comic-specific search form pre-fill — distinct feature, no overlap.
✓ Helper function pattern:
uploadComicFetchFormScreenshotToPR()mirrors existing screenshot helpers (uploadMetadataScanScreenshotToPR, etc.). Consistent, no issues.✓ Black-box test:
package browser_test, notpackage browser. Proper separation.✓ E2E policy compliance: All top-level
var _ = Describe()statements areOrdered. New test adds a child Describe within an existing Ordered parent — does not create a new top-level unordered Describe. Will passmake e2e-policy-check.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Code Review
Test-only PR adding a go-rod browser e2e journey for the comic-specific Fetch Metadata search form (Series/Issue/Year inputs). Reviewed per
.claude/rules/review-standard.md.Findings
No findings.
Analysis
✓ Ordered journey structure: Parent Describe "Journey: Metadata Fetch" is Ordered with a single top-level var _ = Describe(); new child Describe "Comic Fetch Metadata form (bookshelf-abwx.1)" is nested and Ordered with proper BeforeAll that resets DB once per describe block.
✓ Page.Timeout() pattern: Both Its correctly chain .Timeout(pageTimeout) on the page creation line — required go-rod gotcha for Ordered journeys is honored.
✓ No per-spec ResetDB: No banned per-spec BeforeEach(ResetDB) pattern detected. Each It creates a fresh incognito page, proper isolation.
✓ Legitimate Chromium use: First It verifies Stimulus controller connect() populates comic form fields from seeded DB data — requires real DOM. Second It intercepts fetch POSTs via CDP to verify the button dispatches the right structured query with seeded values — requires real network interception. Both satisfy Chromium criteria from CLAUDE.md E2E Testing Policy.
✓ Determinism & async safety: Uses Eventually() with 5s timeouts and 100ms polling (not hard sleeps). Mock fetch set up via EvalOnNewDocument BEFORE navigation, so mock is in place when page loads. Sequential flow: POST → capture body → poll GET → render card → assert captured body. No race condition risk. Seeded DB data, mocked HTTP, fresh page per It — no external dependencies or non-determinism.
✓ Not a duplicate: Existing tests cover scan button visibility and Edit Metadata tab. New test covers Fetch Metadata tab and comic-specific search form pre-fill — distinct feature, no overlap.
✓ Helper function pattern: uploadComicFetchFormScreenshotToPR() mirrors existing screenshot helpers. Consistent, no issues.
✓ Black-box test: package browser_test, not package browser. Proper separation.
✓ E2E policy compliance: All top-level var _ = Describe() are Ordered. New test adds a child Describe within an existing Ordered parent — will pass make e2e-policy-check.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor