/resolve-review-pr

name: resolve-review-pr
description: Review an existing fix PR using the audited repro and full diff, preserving branch and approval rules.

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`](../../../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.

**Check the full PR for scope**, including changes made before you arrived. Establish one concrete reported failure or requested outcome and its acceptance criteria from `bug_url` and the PR's linked issue. Different layers or root causes can contribute to that outcome. If the issue bundles independent problems, identify separable follow-ups in the review. Propose the best-supported scope and keep working on clear requirements. Only a concrete missing input or authorization conflict blocks that work; unresolved design choices remain explicit for PR review under `resolve-investigate`.

For each change, ask whether removing it would leave the intended fix incomplete, incorrect, unsafe, or inadequately tested or documented. Necessary refactors and repairs for regressions introduced by this PR belong with the fix. Apply **Keep the fix focused and complete** from the main instructions, including its allowance for small incidental correctness or robustness improvements. Other independent features, bug fixes, cleanup, and upgrades do not belong, even in the same file or when tests pass. Identify the unrelated files/hunks and remove clearly separable c