fix(e2e/browser): gate 2 ungated screenshot helpers; add screenshot-policy-check guard (bookshelf-6krh) #910

Merged

zombor merged 1 commit from bd-bookshelf-6krh into main 2026-07-03 20:52:46 +00:00

Conversation 1 Commits 1 Files changed 5 +123 -185

zombor commented 2026-07-03 19:11:02 +00:00

Owner

Copy link

Summary

• Two e2e/browser journey upload helpers (uploadWorkflowDetailScreenshotToPR + uploadRecomputeMatchScoreScreenshotToPR) bypassed shouldPostJourneyScreenshot, posting screenshots on every CI run regardless of PR title markers. This caused unrelated screenshots to appear on backend PRs (#897, #898, #908, #909).
• Both helpers are now thin wrappers that delegate to uploadJourneyScreenshotToPR (which calls shouldPostJourneyScreenshot), preserving the exact caption text and slugs (wf-detail-older-execution, recompute-match-score-kebab-open).
• New scripts/screenshot_policy_check/main.go guard scans e2e/browser/*.go and fails if any file does a Forgejo issues POST without the gate. Wired into make lint + CI lint job.

Policy-check before/after

Before fix (original files): go run ./scripts/screenshot_policy_check → exits 1 with:

screenshot-policy-check FAILED: Forgejo issue POST calls found without shouldPostJourneyScreenshot gate.
  e2e/browser/journey_workflow_detail_test.go
  e2e/browser/journey_recompute_match_score_test.go

After fix (this PR): → exits 0 with:

screenshot-policy-check: OK — all Forgejo issue POST calls route through shouldPostJourneyScreenshot.

Test plan

• go build -tags e2e ./e2e/browser/... — clean
• make test — all unit tests pass
• make screenshot-policy-check — passes on fixed code
• make e2e-policy-check + make test-policy-check — both still pass
• Policy check confirmed FAILS on a synthetic ungated file, PASSES on fixed files
• Existing [shot:wf-detail-older-execution] and [shot:recompute-match-score-kebab-open] PR-title markers still trigger the respective posts (behavior preserved via shared helper)

Closes bead bookshelf-6krh on merge.

## Summary - Two `e2e/browser` journey upload helpers (`uploadWorkflowDetailScreenshotToPR` + `uploadRecomputeMatchScoreScreenshotToPR`) bypassed `shouldPostJourneyScreenshot`, posting screenshots on every CI run regardless of PR title markers. This caused unrelated screenshots to appear on backend PRs (#897, #898, #908, #909). - Both helpers are now thin wrappers that delegate to `uploadJourneyScreenshotToPR` (which calls `shouldPostJourneyScreenshot`), preserving the exact caption text and slugs (`wf-detail-older-execution`, `recompute-match-score-kebab-open`). - New `scripts/screenshot_policy_check/main.go` guard scans `e2e/browser/*.go` and fails if any file does a Forgejo issues POST without the gate. Wired into `make lint` + CI lint job. ## Policy-check before/after **Before fix (original files):** `go run ./scripts/screenshot_policy_check` → exits 1 with: ``` screenshot-policy-check FAILED: Forgejo issue POST calls found without shouldPostJourneyScreenshot gate. e2e/browser/journey_workflow_detail_test.go e2e/browser/journey_recompute_match_score_test.go ``` **After fix (this PR):** → exits 0 with: ``` screenshot-policy-check: OK — all Forgejo issue POST calls route through shouldPostJourneyScreenshot. ``` ## Test plan - [x] `go build -tags e2e ./e2e/browser/...` — clean - [x] `make test` — all unit tests pass - [x] `make screenshot-policy-check` — passes on fixed code - [x] `make e2e-policy-check` + `make test-policy-check` — both still pass - [x] Policy check confirmed FAILS on a synthetic ungated file, PASSES on fixed files - [x] Existing `[shot:wf-detail-older-execution]` and `[shot:recompute-match-score-kebab-open]` PR-title markers still trigger the respective posts (behavior preserved via shared helper) Closes bead bookshelf-6krh on merge.

zombor added 1 commit 2026-07-03 19:11:03 +00:00

fix(e2e/browser): gate workflow-detail + recompute-score screenshot posts; add anti-regression check (bookshelf-6krh) ... 08670af06b

Both journey upload helpers were bypassing shouldPostJourneyScreenshot,
causing screenshots to fire on every PR including backend-only ones.

- journey_workflow_detail_test.go: replace 80-line bespoke upload function
  with a thin wrapper that delegates to uploadJourneyScreenshotToPR
  (slug: wf-detail-older-execution); preserves exact caption text.
- journey_recompute_match_score_test.go: same — delegates to shared helper
  (slug: recompute-match-score-kebab-open); removes now-unused imports.
- scripts/screenshot_policy_check/main.go: new guard that scans
  e2e/browser/*.go and fails if any file POSTs to a Forgejo issues
  endpoint without routing through shouldPostJourneyScreenshot. Verified:
  FAILS on original files, PASSES on fixed files.
- Makefile: add screenshot-policy-check target; add to lint.
- .forgejo/workflows/ci.yml: add "screenshot policy check" step to lint job.

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

zombor commented 2026-07-03 19:22:34 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED — 2 minor findings, no blockers or majors.

Phase 0: DEMO Verification

The bead has no explicit runnable DEMO block; the VERIFY section requires the full e2e/browser suite. Per project rules, CI is the source of behavioral truth — CI state is success (SHA 08670af06b). Policy-check script was run directly in the worktree and passed. Behavioral verification by CI is accepted per review-standard.

Phase 1: Spec Compliance — PASS

All bead requirements met:

• Both offending journeys converted to delegate to uploadJourneyScreenshotToPR — verified in diff
• Slugs preserved: "wf-detail-older-execution" and "recompute-match-score-kebab-open" — exact strings confirmed in diff
• Caption/comment text preserved — identical fmt.Sprintf bodies carried over verbatim
• Auto-post via [shot:<slug>] PR-title path still works — shouldPostJourneyScreenshot gates on SCREENSHOT_JOURNEY matching the slug; unaffected
• Guard script scripts/screenshot_policy_check/main.go added with no build tag
• Wired into make lint (Makefile line ~117) and CI lint job (.forgejo/workflows/ci.yml step)

Claim verification — "FAILS on original files, PASSES after fix":

Manually verified both original files had /issues/ (×2 each) + MethodPost (×2 each) and zero shouldPostJourneyScreenshot — hasUngatedPost() would return true for both. After fix both patterns are removed from the journey files (delegate to helper). Check passes. The claim is credible and verified.

New-file regression coverage verified: Ran the check against a simulated new journey file with inline POST — correctly flagged as a violation (exit 1, named the file).

Phase 2: Code Quality

---

[MINOR] scripts/screenshot_policy_check/main.go:35 — main() exceeds 30-line function limit
main() spans lines 35–80 (45 executable lines), exceeding the project's 30-line limit (implementation-standard.md). For a utility script this is low-stakes, but the scan loop and error-print block could be extracted into helpers (e.g. scanViolations(dir) []string, printViolations([]string)) to bring each function under the limit.

---

[MINOR] scripts/screenshot_policy_check/main.go:85 — hasUngatedPost is file-level, not function-level; ungated helper in browser_helpers_test.go evades the check
The guard checks whether /issues/ + MethodPost appear without shouldPostJourneyScreenshot anywhere in the same file. Because browser_helpers_test.go already contains shouldPostJourneyScreenshot (the gate call at line 1 of uploadJourneyScreenshotToPR), adding a second raw-POST helper to that same file would evade the check — verified by simulation. This means the guard catches the primary regression vector (copying old per-journey inline code into a new file) but not a new ungated helper added to the shared helpers file. Suggest: document this known gap in the hasUngatedPost doc comment so future maintainers know the guard does not provide complete coverage for browser_helpers_test.go itself.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

CODE REVIEW: APPROVED — 2 minor findings, no blockers or majors. ## Phase 0: DEMO Verification The bead has no explicit runnable DEMO block; the VERIFY section requires the full e2e/browser suite. Per project rules, CI is the source of behavioral truth — CI state is **success** (SHA 08670af06b46392695af5a13a01dcf8cf00b1e7f). Policy-check script was run directly in the worktree and passed. Behavioral verification by CI is accepted per review-standard. ## Phase 1: Spec Compliance — PASS All bead requirements met: - Both offending journeys converted to delegate to `uploadJourneyScreenshotToPR` — verified in diff - Slugs preserved: `"wf-detail-older-execution"` and `"recompute-match-score-kebab-open"` — exact strings confirmed in diff - Caption/comment text preserved — identical `fmt.Sprintf` bodies carried over verbatim - Auto-post via `[shot:<slug>]` PR-title path still works — `shouldPostJourneyScreenshot` gates on SCREENSHOT_JOURNEY matching the slug; unaffected - Guard script `scripts/screenshot_policy_check/main.go` added with no build tag - Wired into `make lint` (Makefile line ~117) and CI lint job (`.forgejo/workflows/ci.yml` step) **Claim verification — "FAILS on original files, PASSES after fix":** Manually verified both original files had `/issues/` (×2 each) + `MethodPost` (×2 each) and zero `shouldPostJourneyScreenshot` — `hasUngatedPost()` would return true for both. After fix both patterns are removed from the journey files (delegate to helper). Check passes. The claim is credible and verified. **New-file regression coverage verified:** Ran the check against a simulated new journey file with inline POST — correctly flagged as a violation (exit 1, named the file). ## Phase 2: Code Quality --- [MINOR] scripts/screenshot_policy_check/main.go:35 — main() exceeds 30-line function limit main() spans lines 35–80 (45 executable lines), exceeding the project's 30-line limit (implementation-standard.md). For a utility script this is low-stakes, but the scan loop and error-print block could be extracted into helpers (e.g. `scanViolations(dir) []string`, `printViolations([]string)`) to bring each function under the limit. --- [MINOR] scripts/screenshot_policy_check/main.go:85 — hasUngatedPost is file-level, not function-level; ungated helper in browser_helpers_test.go evades the check The guard checks whether `/issues/` + `MethodPost` appear without `shouldPostJourneyScreenshot` anywhere in the same file. Because `browser_helpers_test.go` already contains `shouldPostJourneyScreenshot` (the gate call at line 1 of `uploadJourneyScreenshotToPR`), adding a second raw-POST helper to that same file would evade the check — verified by simulation. This means the guard catches the primary regression vector (copying old per-journey inline code into a new file) but not a new ungated helper added to the shared helpers file. Suggest: document this known gap in the `hasUngatedPost` doc comment so future maintainers know the guard does not provide complete coverage for `browser_helpers_test.go` itself. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor force-pushed bd-bookshelf-6krh from 08670af06b to 52f1a5f2d4 2026-07-03 20:32:32 +00:00 Compare

zombor merged commit d65e0797e0 into main 2026-07-03 20:52:46 +00:00

zombor referenced this pull request from a commit 2026-07-03 20:52:47 +00:00

Merge pull request 'fix(e2e/browser): gate 2 ungated screenshot helpers; add screenshot-policy-check guard (bookshelf-6krh)' (#910) from bd-bookshelf-6krh 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!910

No description provided.