You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
While validating #4564 (the `review.visual.production_url` fix) against a real metagraphed test PR (JSONbored/metagraphed#4713, closed without merging), found a separate, confirmed-live bug in the #4535/#4540 screenshot-evidence-completeness matrix mode.
Bot-capture ran successfully and produced a real desktop before/after pair (two distinct R2-hosted PNGs, confirmed via direct URL inspection — not placeholders).
`markPullRequestVisualCaptureSatisfied` correctly persisted `visual_capture_satisfied_sha = head_sha` — confirmed via direct `psql` query against the self-host Postgres instance on edge-nl-01:
```
number | head_sha | visual_capture_satisfied_sha
4713 | 3498783e04c7224a6634c242bdc29380d98c05b4 | 3498783e04c7224a6634c242bdc29380d98c05b4
```
Despite that, the panel comment's screenshot-evidence-completeness blocker fired anyway: "no before/after screenshot table (3 viewports × 2 themes) was provided, which screenshot-evidence-completeness treats as a blocker regardless of how trivial the visual diff looks."
Triggered an explicit re-run via the panel's `gittensory-rerun-review` checkbox (after the DB write above already existed) — the blocker fired again, identically.
Expected behavior
Per `src/review/screenshot-table-gate.ts`'s own doc comment: "`botCaptureSatisfied` ⇒ no violation regardless of mode (an automated capture is equivalent to a hand-authored table...)" — line 333: `if (input.botCaptureSatisfied === true) return NO_VIOLATION;`. This should have short-circuited the matrix-mode check entirely on the re-run, once `visual_capture_satisfied_sha` matched `head_sha` in the DB.
Suspected root cause (unconfirmed — needs investigation, not yet fixed)
`src/queue/processors.ts:2946` computes `botCaptureSatisfied` from an in-memory `pr` object (`pr.visualCaptureSatisfiedSha === pr.headSha`). The capture-and-mark step (`markPullRequestVisualCaptureSatisfied`, ~line 10834) writes to the DB but may not update that SAME in-memory `pr` object within the pass, and/or the "re-run" trigger path may not re-fetch `pr` fresh from the DB before evaluating the gate — meaning the intended bypass never actually reaches a call site with the correct value, on either the original pass or a re-run.
Impact
For any repo with matrix mode enabled (currently only metagraphed), bot-capture provides zero benefit toward satisfying the screenshot-evidence-completeness gate — contributors get blocked regardless of a successful automated capture, undermining the entire point of #3607's automated-capture epic (auto-evidence "so contributors touching apps/ui/** don't have to submit their own screenshots" — they currently still do, or get closed).
Suggested next step
Trace the exact `pr` object lifecycle between the capture step and the gate-evaluation call site across both the original webhook pass and the re-run trigger path; confirm whether either ever re-reads `visual_capture_satisfied_sha` fresh from the DB before evaluating `evaluateScreenshotTableGate`.
Context
While validating #4564 (the `review.visual.production_url` fix) against a real metagraphed test PR (JSONbored/metagraphed#4713, closed without merging), found a separate, confirmed-live bug in the #4535/#4540 screenshot-evidence-completeness matrix mode.
What happened
```
number | head_sha | visual_capture_satisfied_sha
4713 | 3498783e04c7224a6634c242bdc29380d98c05b4 | 3498783e04c7224a6634c242bdc29380d98c05b4
```
Expected behavior
Per `src/review/screenshot-table-gate.ts`'s own doc comment: "`botCaptureSatisfied` ⇒ no violation regardless of mode (an automated capture is equivalent to a hand-authored table...)" — line 333: `if (input.botCaptureSatisfied === true) return NO_VIOLATION;`. This should have short-circuited the matrix-mode check entirely on the re-run, once `visual_capture_satisfied_sha` matched `head_sha` in the DB.
Suspected root cause (unconfirmed — needs investigation, not yet fixed)
`src/queue/processors.ts:2946` computes `botCaptureSatisfied` from an in-memory `pr` object (`pr.visualCaptureSatisfiedSha === pr.headSha`). The capture-and-mark step (`markPullRequestVisualCaptureSatisfied`, ~line 10834) writes to the DB but may not update that SAME in-memory `pr` object within the pass, and/or the "re-run" trigger path may not re-fetch `pr` fresh from the DB before evaluating the gate — meaning the intended bypass never actually reaches a call site with the correct value, on either the original pass or a re-run.
Impact
For any repo with matrix mode enabled (currently only metagraphed), bot-capture provides zero benefit toward satisfying the screenshot-evidence-completeness gate — contributors get blocked regardless of a successful automated capture, undermining the entire point of #3607's automated-capture epic (auto-evidence "so contributors touching apps/ui/** don't have to submit their own screenshots" — they currently still do, or get closed).
Suggested next step
Trace the exact `pr` object lifecycle between the capture step and the gate-evaluation call site across both the original webhook pass and the re-run trigger path; confirm whether either ever re-reads `visual_capture_satisfied_sha` fresh from the DB before evaluating `evaluateScreenshotTableGate`.