resolve-review-pr

Step 2A — Review the existing fix PR

You are reviewing someone else's candidate fix, not writing your own. The reproduction test is evidence only after independent validation. Complete the shared repro audit before this path; the recovered verdict is not an endorsement of the test. A passing repro alone does not prove the PR fixes the bug.

1. Check out the PR head into your worktree (gh pr checkout <number>), then ensure the repro test at test_path is present on top of it (it is your artifact, not theirs — re-apply it if the checkout doesn't carry it). If a test you keep — the repro test, or one the PR adds — names a ticket/issue in its filename or code, rename it and strip the reference per the "name by the problem, never the ticket" rule in 2B.4.
2. Run the same audited repro test against the PR. Compare it with the behavioral failure on the recorded unfixed base:
   • Passes → evidence that the tested behavior is corrected, subject to the journey and diff review below. For a compound bug, run every reproduced facet; all live facets must pass for the PR to fully resolve it.
   • Fails behaviorally → the PR does not fix that reproduced behavior; capture the exact failure. Setup/import failures or invalid test assumptions are verification blockers, not proof that the PR is wrong. Resolve or disclose them without approving the PR or inventing a product change.
3. Record the journey against the PR head — always. You drive the recorder off the reproduction test (the e2e_ui test for web/terminal facets, a VHS tape for cli facets) run against the PR head, and add an after-kind entry to your handoff recordings. This is not gated on the repro handoff carrying footage — you have the test and the journey, which is all the recorder needs, so produce the after-clip whether or not any before-clip was recovered. Burn timed step-by-step captions into each new clip and inspect the render using dev/caption_recording.py; the attachment description alone is not an on-screen caption. Use the same lanes as 2B.5 — see dev/recording-lanes.md (build the SPA first, record via OMNIGENT_E2E_RECORD_DIR, per-surface web / mobile / terminal / cli / desktop mechanics). Save the raw capture to recordings/<slug>/raw/after-<facet>.<ext>, render to recordings/<slug>/after-<facet>.mp4, and declare only the inspected final path with a caption for what the clip shows. If captioning fails, use the shared guide's explicit fallback path instead. The test result determines the verdict separately; the footage must show the product journey and its visible outcome, never the test runner. When the handoff does carry a before clip, carry it through and produce the after; when it carries none, still produce the after and note the missing before. Only omit the after clip when it is genuinely unobtainable (recorder tooling missing, or the fixture can't come online after the SPA build and the leaked runner env is stripped) — say so explicitly in your review comment and in evidence, naming the blocker. An online: false seen while OMNIGENT_RUNNER_ID is still set is your own un-stripped env, not a blocker: re-run with the env -u prefix from dev/recording-lanes.md first. A missing upstream before-clip is never that blocker. Never drop it silently.
4. Review the diff for quality, not just green. Decide whether this is the best practical approach for the repository, not merely an approach that makes the reproduction pass. Identify the plausible alternatives suggested by the surrounding architecture and compare them briefly: does this PR fix the root cause at the correct layer, follow the established abstraction, minimize special cases and long-term maintenance cost, and preserve security, compatibility, and performance? Does it miss facets or obvious adjacent edge cases, or introduce a regression in the surrounding code? Complete the shared impact assessment and run its checks for the whole PR. Record why the selected approach is preferable in the review. Apply resolve-investigate to the reported configuration, competing causes, and historical design rationale; explicitly identify policy changes even when the repro test passes. "Best" means the strongest maintainable fit for this codebase and bug, not a license to replace a sound, idiomatic contribution with a theoretically purer rewrite or a personal style preference.