/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. 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) — saving to `recordings/<slug>/after-<facet>.<ext>` with a `caption` for what the clip shows. 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. "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, ask the author to split them or track them separately; stop with `needs_more_info` if the intended scope is unclear.

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. Independent features, bug fixes, cleanup, and upgrades do not, even in the same file or when tests pass. Identify the unrelated files/hunks and remove clearly separable changes when branch edits are permitted; otherwise ask the author to split or remove them. Do not guess when changes are entangled. Carry only in-scope work into any fork takeover.

Address Polly's scope findings through the ordinary review process in Step 4.3 before approving this existing PR. Keep your own edits within the same scope. Request clarification when its relationship to the reported bug is uncertain; do not approve until clarified. Record unresolved scope concerns in the review and `fix_summary`. 5. **Report on the existing PR.** Post your fail→pass (or fail→still-fails) result and any diff concerns now as a `gh pr comment` / `gh pr review --comment`, and record its `pr_url` in your output. The `outcome` reflects what you found (`fixed` when the PR resolves every live facet, the shared impact assessment has no unresolved required checks, th