fix(e2e): scope screenshot posting to named journey (bookshelf-2hoy) #795

Merged

zombor merged 1 commit from bd-bookshelf-2hoy into main 2026-06-28 03:22:57 +00:00

Conversation 2 Commits 1 Files changed 30 +183 -29

zombor commented 2026-06-28 02:46:49 +00:00

Owner

Copy link

Summary

• Root cause: each of the 26 screenshot helper functions in e2e/browser/ gated on FORGEJO_TOKEN + PULL_REQUEST_NUMBER only. Running the full suite with PULL_REQUEST_NUMBER set caused every journey to post to that PR — PR #776 got 38 stray screenshots from unrelated beads.
• Fix: new SCREENSHOT_JOURNEY env var required. e2e/screenshotscope.ShouldPost() requires token + prNum + targetJourney all set AND matching the journey's slug. When SCREENSHOT_JOURNEY is unset, nothing posts — the full suite is safe to run with PULL_REQUEST_NUMBER set.
• Mechanism: all 26 helpers now call shouldPostJourneyScreenshot("slug") via a shared wrapper in screenshot_scope_test.go.
• Testable: e2e/screenshotscope package (no e2e build tag) has 100% unit test coverage and is in UNIT_PKGS for make test + make coverage.

Capture pattern (documented in scope.go)

FORGEJO_TOKEN=<tok> \
PULL_REQUEST_NUMBER=<pr> \
SCREENSHOT_JOURNEY=<slug> \
  go test -tags e2e ./e2e/browser/ -run "Journey: Your Journey Name"

Never run the full suite with only PULL_REQUEST_NUMBER set.

Compatibility with bookshelf-qy9b

bookshelf-qy9b wires PULL_REQUEST_NUMBER into the CI browser job. This fix is additive: CI will also need to set SCREENSHOT_JOURNEY=<slug> alongside PULL_REQUEST_NUMBER for the specific journey being captured. The two env vars are independent — setting both allows the correct single screenshot to post.

Test plan

• make test passes (includes e2e/screenshotscope unit tests)
• make coverage passes (100% on e2e/screenshotscope, gate is clean)
• go test -run='^$' -tags e2e ./e2e/browser/ compiles without error
• go vet -tags e2e ./e2e/browser/ clean
• Full browser suite with PULL_REQUEST_NUMBER set but no SCREENSHOT_JOURNEY → no screenshots posted (verified by logic: ShouldPost("any-slug", tok, pr, "") == false)
• With SCREENSHOT_JOURNEY=create-shelf, only uploadCreateShelfScreenshotToPR posts

Closes bead bookshelf-2hoy on merge.

## Summary - **Root cause:** each of the 26 screenshot helper functions in `e2e/browser/` gated on `FORGEJO_TOKEN + PULL_REQUEST_NUMBER` only. Running the full suite with `PULL_REQUEST_NUMBER` set caused every journey to post to that PR — PR #776 got 38 stray screenshots from unrelated beads. - **Fix:** new `SCREENSHOT_JOURNEY` env var required. `e2e/screenshotscope.ShouldPost()` requires token + prNum + targetJourney all set AND matching the journey's slug. When `SCREENSHOT_JOURNEY` is unset, nothing posts — the full suite is safe to run with `PULL_REQUEST_NUMBER` set. - **Mechanism:** all 26 helpers now call `shouldPostJourneyScreenshot("slug")` via a shared wrapper in `screenshot_scope_test.go`. - **Testable:** `e2e/screenshotscope` package (no e2e build tag) has 100% unit test coverage and is in `UNIT_PKGS` for `make test` + `make coverage`. ## Capture pattern (documented in scope.go) ```sh FORGEJO_TOKEN=<tok> \ PULL_REQUEST_NUMBER=<pr> \ SCREENSHOT_JOURNEY=<slug> \ go test -tags e2e ./e2e/browser/ -run "Journey: Your Journey Name" ``` Never run the full suite with only `PULL_REQUEST_NUMBER` set. ## Compatibility with bookshelf-qy9b bookshelf-qy9b wires `PULL_REQUEST_NUMBER` into the CI browser job. This fix is additive: CI will also need to set `SCREENSHOT_JOURNEY=<slug>` alongside `PULL_REQUEST_NUMBER` for the specific journey being captured. The two env vars are independent — setting both allows the correct single screenshot to post. ## Test plan - [x] `make test` passes (includes `e2e/screenshotscope` unit tests) - [x] `make coverage` passes (100% on `e2e/screenshotscope`, gate is clean) - [x] `go test -run='^$' -tags e2e ./e2e/browser/` compiles without error - [x] `go vet -tags e2e ./e2e/browser/` clean - [x] Full browser suite with `PULL_REQUEST_NUMBER` set but no `SCREENSHOT_JOURNEY` → no screenshots posted (verified by logic: `ShouldPost("any-slug", tok, pr, "") == false`) - [x] With `SCREENSHOT_JOURNEY=create-shelf`, only `uploadCreateShelfScreenshotToPR` posts Closes bead bookshelf-2hoy on merge.

zombor added 1 commit 2026-06-28 02:46:51 +00:00

fix(e2e): scope screenshot posting to named journey (bookshelf-2hoy) ... 16d295fbf3

Running the full e2e/browser suite with PULL_REQUEST_NUMBER set caused
every journey's screenshot helper to post to that PR — PR #776 received
38 stray screenshots from unrelated beads.

Root cause: each helper gated on FORGEJO_TOKEN + PULL_REQUEST_NUMBER
only. With no journey filter, all 26 helpers fired for any PR.

Fix: add SCREENSHOT_JOURNEY env var. ShouldPost() in the new
e2e/screenshotscope package requires all three vars to match: token,
prNum, AND targetJourney == journeySlug. When SCREENSHOT_JOURNEY is
unset, nothing posts — running the full suite with PULL_REQUEST_NUMBER
set alone is now safe.

Each journey's upload*ToPR / post*Screenshot helper now calls
shouldPostJourneyScreenshot("slug") instead of the old two-var check.
The shared wrapper (screenshot_scope_test.go) reads all three env vars
and delegates to screenshotscope.ShouldPost.

The screenshotscope package has 100% unit test coverage and is included
in make test + make coverage via UNIT_PKGS.

Capture pattern (documented in scope.go + screenshot_scope_test.go):
  FORGEJO_TOKEN=<tok> PULL_REQUEST_NUMBER=<pr> SCREENSHOT_JOURNEY=<slug> \
    go test -tags e2e ./e2e/browser/ -run "Journey: Your Journey Name"

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

zombor commented 2026-06-28 02:54:32 +00:00

Author

Owner

Copy link

Security Review — PR #795 (bookshelf-2hoy)

Branch: bd-bookshelf-2hoy · HEAD: 16d295fb

Reviewed per .claude/rules/review-standard.md. Diff scope: new e2e/screenshotscope/ package, e2e/browser/screenshot_scope_test.go, 26 test-file one-liners replacing the guard, Makefile + scripts/check-coverage.sh package-list additions.

---

1. TOKEN HANDLING

FORGEJO_TOKEN is read from os.Getenv("FORGEJO_TOKEN") in two places after this change: in the existing caller function (unchanged) and inside screenshotscope.ShouldPost (new). In ShouldPost the token is only checked for emptiness (token == ""); it is never logged, never written to a file, never embedded in a string that reaches test output, and not committed anywhere. The actual POST to the Forgejo API still uses the token local variable in the caller, unchanged. The double-read is harmless.

CONFIRMED: token is not logged, not written to a file, not embedded in a fixture, not exposed in test output.

---

2. NO SECRET COMMITTED

Scanned all new/modified files (e2e/screenshotscope/scope.go, scope_test.go, suite_test.go, e2e/browser/screenshot_scope_test.go, all 26 updated test helpers, Makefile, scripts/check-coverage.sh). The only new constant is EnvScreenshotJourney = "SCREENSHOT_JOURNEY" — the env var name, not a value. No token, credential, or secret value is hardcoded anywhere in the diff.

CONFIRMED: no secret committed.

---

3. NO NEW NETWORK SURFACE

The change only tightens the gating: ShouldPost requires three conditions (token + prNum + targetJourney all non-empty, and targetJourney == slug) vs. the old two (token + prNum). The actual Forgejo API POST call is unchanged and only reached when the guard passes. No new HTTP endpoint is opened; no new outbound destination is introduced.

CONFIRMED: strictly narrower network surface than before.

---

4. MAKEFILE + CHECK-COVERAGE.SH

Both changes append ./e2e/screenshotscope/... to the UNIT_PKGS string literal. This string is passed as plain positional arguments to go test — no eval, no shell variable expansion of untrusted input, no command substitution. The ./e2e/screenshotscope/... glob is resolved by the Go toolchain, not by the shell. No injection vector.

CONFIRMED: no shell-injection, no eval of untrusted input.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #795 (bookshelf-2hoy) Branch: `bd-bookshelf-2hoy` · HEAD: `16d295fb` Reviewed per `.claude/rules/review-standard.md`. Diff scope: new `e2e/screenshotscope/` package, `e2e/browser/screenshot_scope_test.go`, 26 test-file one-liners replacing the guard, `Makefile` + `scripts/check-coverage.sh` package-list additions. --- ### 1. TOKEN HANDLING `FORGEJO_TOKEN` is read from `os.Getenv("FORGEJO_TOKEN")` in two places after this change: in the existing caller function (unchanged) and inside `screenshotscope.ShouldPost` (new). In `ShouldPost` the token is only checked for emptiness (`token == ""`); it is never logged, never written to a file, never embedded in a string that reaches test output, and not committed anywhere. The actual POST to the Forgejo API still uses the `token` local variable in the caller, unchanged. The double-read is harmless. **CONFIRMED: token is not logged, not written to a file, not embedded in a fixture, not exposed in test output.** --- ### 2. NO SECRET COMMITTED Scanned all new/modified files (`e2e/screenshotscope/scope.go`, `scope_test.go`, `suite_test.go`, `e2e/browser/screenshot_scope_test.go`, all 26 updated test helpers, `Makefile`, `scripts/check-coverage.sh`). The only new constant is `EnvScreenshotJourney = "SCREENSHOT_JOURNEY"` — the env var *name*, not a value. No token, credential, or secret value is hardcoded anywhere in the diff. **CONFIRMED: no secret committed.** --- ### 3. NO NEW NETWORK SURFACE The change only tightens the gating: `ShouldPost` requires three conditions (token + prNum + targetJourney all non-empty, and targetJourney == slug) vs. the old two (token + prNum). The actual Forgejo API POST call is unchanged and only reached when the guard passes. No new HTTP endpoint is opened; no new outbound destination is introduced. **CONFIRMED: strictly narrower network surface than before.** --- ### 4. MAKEFILE + CHECK-COVERAGE.SH Both changes append `./e2e/screenshotscope/...` to the `UNIT_PKGS` string literal. This string is passed as plain positional arguments to `go test` — no `eval`, no shell variable expansion of untrusted input, no command substitution. The `./e2e/screenshotscope/...` glob is resolved by the Go toolchain, not by the shell. No injection vector. **CONFIRMED: no shell-injection, no eval of untrusted input.** --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-28 02:56:36 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Phase 0: DEMO Verification

No explicit DEMO: block exists in the bead description or comments. The user dispatcher noted "CI green" and instructed not to re-run tests; CI serves as behavioral verification for this infrastructure fix. Flagged below as MINOR.

Phase 1: Spec Compliance — PASS

All requirements from bookshelf-2hoy met:

• New e2e/screenshotscope package with ShouldPost(journeySlug, token, prNum, targetJourney) — present at e2e/screenshotscope/scope.go.
• SCREENSHOT_JOURNEY unset → nothing posts from any journey — confirmed by targetJourney == "" short-circuit in ShouldPost.
• All 26 screenshot-guard call sites converted — verified by reading every file on origin/bd-bookshelf-2hoy (26 shouldPostJourneyScreenshot calls, 0 remaining token == "" || prNum == "" patterns).
• Capture pattern documented in scope.go package doc and screenshot_scope_test.go header comment.
• Token not logged or leaked — ShouldPost only inspects it for emptiness; posting code remains in the individual helpers.

Phase 2: Code Quality — PASS

Core logic (e2e/screenshotscope/scope.go:50-55)
ShouldPost returns true only when all three of token, prNum, targetJourney are non-empty AND targetJourney == journeySlug (exact string match). No prefix/partial match gap. When SCREENSHOT_JOURNEY is unset the third condition short-circuits immediately — the full suite is safe to run with PULL_REQUEST_NUMBER set alone.

All 26 guards
Every call site uses shouldPostJourneyScreenshot("<slug>") through the shared wrapper in e2e/browser/screenshot_scope_test.go. Slugs are unique per journey. The two reader-fit-mode guards in journey_reader_peripherals_test.go:47 and :99 are intentional — both helpers belong to the same journey, so sharing the slug is correct.

Build-tag boundary

• e2e/screenshotscope/scope.go — no //go:build tag (correct: pure decision function, no Chromium/Docker dep, unit-testable).
• e2e/screenshotscope/scope_test.go / suite_test.go — no //go:build tag (correct: plain unit tests).
• e2e/browser/screenshot_scope_test.go — //go:build e2e (correct: lives in the e2e browser package).

**Coverage gate (Makefile line 70, scripts/check-coverage.sh line 50)** ./e2e/screenshotscope/...is an INCLUSION intoUNIT_PKGS— the new package is now coverage-gated at 100%. This is not an exclusion. BothMakefileandscripts/check-coverage.shupdated in sync. Perno-coverage-exclusions`, this is correct.

Branch coverage of ShouldPost
All branches exercised by scope_test.go:

• token == "" → false: Entry("no token") + Entry("all empty")
• prNum == "" (token set) → false: Entry("no prNum")
• targetJourney == "" (token + prNum set) → false: Entry("no targetJourney")
• All three set, targetJourney != journeySlug → false: four entries (different, prefix, suffix, case)
• All three set, match → true: three entries

Secret/token safety
Token is received as a parameter and passed only to a boolean comparison (token == ""). No logging, no serialisation.

---

[MINOR] e2e/screenshotscope — no DEMO block in bead
The bead has no DEMO: block showing a command + expected output. For future bead authors: add a DEMO: go test -run TestScreenshotScope ./e2e/screenshotscope/ with expected pass output so reviewers can reproduce independently. Does not block this PR.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## CODE REVIEW: APPROVED ### Phase 0: DEMO Verification No explicit `DEMO:` block exists in the bead description or comments. The user dispatcher noted "CI green" and instructed not to re-run tests; CI serves as behavioral verification for this infrastructure fix. Flagged below as MINOR. ### Phase 1: Spec Compliance — PASS All requirements from bookshelf-2hoy met: - New `e2e/screenshotscope` package with `ShouldPost(journeySlug, token, prNum, targetJourney)` — present at `e2e/screenshotscope/scope.go`. - SCREENSHOT_JOURNEY unset → nothing posts from any journey — confirmed by `targetJourney == ""` short-circuit in `ShouldPost`. - All 26 screenshot-guard call sites converted — verified by reading every file on `origin/bd-bookshelf-2hoy` (26 `shouldPostJourneyScreenshot` calls, 0 remaining `token == "" || prNum == ""` patterns). - Capture pattern documented in `scope.go` package doc and `screenshot_scope_test.go` header comment. - Token not logged or leaked — `ShouldPost` only inspects it for emptiness; posting code remains in the individual helpers. ### Phase 2: Code Quality — PASS **Core logic (`e2e/screenshotscope/scope.go:50-55`)** `ShouldPost` returns true only when all three of `token`, `prNum`, `targetJourney` are non-empty AND `targetJourney == journeySlug` (exact string match). No prefix/partial match gap. When `SCREENSHOT_JOURNEY` is unset the third condition short-circuits immediately — the full suite is safe to run with `PULL_REQUEST_NUMBER` set alone. **All 26 guards** Every call site uses `shouldPostJourneyScreenshot("<slug>")` through the shared wrapper in `e2e/browser/screenshot_scope_test.go`. Slugs are unique per journey. The two `reader-fit-mode` guards in `journey_reader_peripherals_test.go:47` and `:99` are intentional — both helpers belong to the same journey, so sharing the slug is correct. **Build-tag boundary** - `e2e/screenshotscope/scope.go` — no `//go:build` tag (correct: pure decision function, no Chromium/Docker dep, unit-testable). - `e2e/screenshotscope/scope_test.go` / `suite_test.go` — no `//go:build` tag (correct: plain unit tests). - `e2e/browser/screenshot_scope_test.go` — `//go:build e2e` (correct: lives in the e2e browser package). **Coverage gate (`Makefile` line 70, `scripts/check-coverage.sh` line 50`)** `./e2e/screenshotscope/...` is an INCLUSION into `UNIT_PKGS` — the new package is now coverage-gated at 100%. This is not an exclusion. Both `Makefile` and `scripts/check-coverage.sh` updated in sync. Per `no-coverage-exclusions`, this is correct. **Branch coverage of `ShouldPost`** All branches exercised by `scope_test.go`: - `token == ""` → false: Entry("no token") + Entry("all empty") - `prNum == ""` (token set) → false: Entry("no prNum") - `targetJourney == ""` (token + prNum set) → false: Entry("no targetJourney") - All three set, `targetJourney != journeySlug` → false: four entries (different, prefix, suffix, case) - All three set, match → true: three entries **Secret/token safety** Token is received as a parameter and passed only to a boolean comparison (`token == ""`). No logging, no serialisation. --- [MINOR] e2e/screenshotscope — no DEMO block in bead The bead has no `DEMO:` block showing a command + expected output. For future bead authors: add a `DEMO: go test -run TestScreenshotScope ./e2e/screenshotscope/` with expected pass output so reviewers can reproduce independently. Does not block this PR. **REVIEW VERDICT: 0 blocker, 0 major, 1 minor**

zombor force-pushed bd-bookshelf-2hoy from 16d295fbf3 to b0d4855f33 2026-06-28 03:17:05 +00:00 Compare

zombor merged commit 9ca24f1d40 into main 2026-06-28 03:22:57 +00:00

zombor referenced this pull request from a commit 2026-06-28 03:23:02 +00:00

Merge pull request 'fix(e2e): scope screenshot posting to named journey (bookshelf-2hoy)' (#795) from bd-bookshelf-2hoy into main

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!795

No description provided.