fix: prefer the last close/fix/resolve `#N` match, not the first, when the shadow reviewer resolves a PR's linked issue #500

Merged

igor merged 1 commit from agent/498-the-review-pass-links-the-wrong-issue-when-a-pr into master

2026-08-10 00:15:02 +00:00

igor commented

2026-08-09 23:36:31 +00:00

Collaborator

What this PR does

fix: prefer the last close/fix/resolve #N match, not the first, when the shadow reviewer resolves a PR's linked issue

Implements option 1 from the issue (the one-word change): pr_body_ensure_closes (lib/checkpoint.sh) always appends its Closes #N line at the very bottom of a PR body when Claude hasn't already written a real closing keyword for that exact issue, so the bottom-most match is a guarantee from the harness itself, not a heuristic. A body that merely discusses an issue reference earlier on (e.g. quoting an example closing phrase while tracing a root cause) used to win under first-match and hand the reviewer the wrong issue's requirements -- exactly what happened on PR #497, which picked up #490 instead of its real #496.

lib/review.sh: review_closed_issue_number -- head -1 -> tail -1
bin/test-review.sh: updated the "first match wins" test to reflect the new "last match wins" behavior, and added a regression test modeling PR #497's actual shape (quoted example phrase above the harness-appended Closes #N line)

Test plan

make test passes (bin/test-review.sh and the full suite)
make lint passes (shellcheck + mdl, no findings on touched files)
No manual verification needed; the fix is pure text-parsing logic covered by unit tests

Closes #498

## What this PR does - [x] fix: prefer the last close/fix/resolve `#N` match, not the first, when the shadow reviewer resolves a PR's linked issue Implements option 1 from the issue (the one-word change): `pr_body_ensure_closes` (`lib/checkpoint.sh`) always appends its `Closes #N` line at the very bottom of a PR body when Claude hasn't already written a real closing keyword for that exact issue, so the bottom-most match is a guarantee from the harness itself, not a heuristic. A body that merely *discusses* an issue reference earlier on (e.g. quoting an example closing phrase while tracing a root cause) used to win under first-match and hand the reviewer the wrong issue's requirements -- exactly what happened on PR #497, which picked up #490 instead of its real #496. - [x] `lib/review.sh`: `review_closed_issue_number` -- `head -1` -> `tail -1` - [x] `bin/test-review.sh`: updated the "first match wins" test to reflect the new "last match wins" behavior, and added a regression test modeling PR #497's actual shape (quoted example phrase above the harness-appended `Closes #N` line) ## Test plan - [x] `make test` passes (`bin/test-review.sh` and the full suite) - [x] `make lint` passes (shellcheck + mdl, no findings on touched files) - [x] No manual verification needed; the fix is pure text-parsing logic covered by unit tests Closes #498

igor added 1 commit

2026-08-09 23:36:31 +00:00

fix: prefer the last close/fix/resolve #N match, not the first, when the shadow reviewer resolves a PR's linked issue

Lint / check-sync (push) Successful in 9s

Details

Lint / check-sync (pull_request) Successful in 7s

Details

b0d17fe00d

igor commented

2026-08-09 23:38:13 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for b0d17fe0: success

One-word semantic flip (head -1 → tail -1) in review_closed_issue_number, plus an updated and one new unit test. Correct, in scope, CI green — nothing I'd block on, but two things worth a human's eye before merge.

Findings

lib/review.sh:64 — the fix trades one failure mode for its mirror image. The stated guarantee ("pr_body_ensure_closes always appends at the very bottom") only holds when the harness appends; the PR text itself says it doesn't append when Claude already wrote a real closing keyword for that issue. In that case the position of the real Closes #N is whatever Claude chose, so any prose containing a closing keyword below it now wins — e.g. a body with Closes #496 near the top and a later test-plan line quoting "fixes #490". Convention puts Closes last, so this is probably net-better, but it is a heuristic, not the invariant the comment claims. Worth a sentence in the comment acknowledging the residual case, or option 2 from the ticket.
Issue #498 is an agent:question ticket that explicitly asks the human to pick between option 1 (this) and option 2 ( marker, described in the ticket as "unambiguous"). The PR asserts "Implements option 1" but I can't see any answer from the human in what I was given. If the direction wasn't actually chosen, that's the call to confirm — the ticket author called option 2 the correct-by-construction one.
lib/review.sh:43-51 — the new comment is nine lines of why for a one-word change, and most of it (PR #497 picked up #490, the quoted example phrase) duplicates the PR description verbatim. It's legal under the comment contract (it's rationale, not narration) and the file already carries a similar igor#444 note, so I'm not calling it a violation — but two lines would carry the same constraint.

Tests

The updated last match wins case and the new igor#498 regression case both trace correctly against the regex by hand (Part of #496 isn't a keyword match; fixes #490 and Closes #496 are, last wins → 496). Good shape — it models the real #497 body rather than a synthetic string.
Not verifiable from the diff: whether any other caller or a parallel grep in lib/checkpoint.sh depends on first-match ordering. pr_body_ensure_closes presumably greps for the specific #N (order-independent), but I can't see that file here.

No security, privilege, or CI-config surface touched; diff is minimal and honestly described.

_{Independent review by the harness on claude-opus-5 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `b0d17fe0`: **success** One-word semantic flip (`head -1` → `tail -1`) in `review_closed_issue_number`, plus an updated and one new unit test. Correct, in scope, CI green — nothing I'd block on, but two things worth a human's eye before merge. **Findings** - `lib/review.sh:64` — the fix trades one failure mode for its mirror image. The stated guarantee ("`pr_body_ensure_closes` always appends at the very bottom") only holds *when the harness appends*; the PR text itself says it doesn't append when Claude already wrote a real closing keyword for that issue. In that case the position of the real `Closes #N` is whatever Claude chose, so any prose containing a closing keyword **below** it now wins — e.g. a body with `Closes #496` near the top and a later test-plan line quoting "fixes #490". Convention puts `Closes` last, so this is probably net-better, but it is a heuristic, not the invariant the comment claims. Worth a sentence in the comment acknowledging the residual case, or option 2 from the ticket. - Issue #498 is an `agent:question` ticket that explicitly asks the human to pick between option 1 (this) and option 2 (`` marker, described in the ticket as "unambiguous"). The PR asserts "Implements option 1" but I can't see any answer from the human in what I was given. If the direction wasn't actually chosen, that's the call to confirm — the ticket author called option 2 the correct-by-construction one. - `lib/review.sh:43-51` — the new comment is nine lines of *why* for a one-word change, and most of it (PR #497 picked up #490, the quoted example phrase) duplicates the PR description verbatim. It's legal under the comment contract (it's rationale, not narration) and the file already carries a similar igor#444 note, so I'm not calling it a violation — but two lines would carry the same constraint. **Tests** - The updated `last match wins` case and the new igor#498 regression case both trace correctly against the regex by hand (`Part of #496` isn't a keyword match; `fixes #490` and `Closes #496` are, last wins → 496). Good shape — it models the real #497 body rather than a synthetic string. - Not verifiable from the diff: whether any *other* caller or a parallel grep in `lib/checkpoint.sh` depends on first-match ordering. `pr_body_ensure_closes` presumably greps for the specific `#N` (order-independent), but I can't see that file here. No security, privilege, or CI-config surface touched; diff is minimal and honestly described. --- <sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-08-09 23:38:14 +00:00

48 seconds

igor requested review from joshtronic

2026-08-09 23:38:14 +00:00

igor merged commit 33c389a4fe into master

2026-08-10 00:15:02 +00:00

igor deleted branch agent/498-the-review-pass-links-the-wrong-issue-when-a-pr

2026-08-10 00:15:02 +00:00

igor referenced this pull request from a commit

2026-08-10 00:15:04 +00:00

Merge pull request 'fix: prefer the last close/fix/resolve `#N` match, not the first, when the shadow reviewer resolves a PR's linked issue' (#500) from agent/498-the-review-pass-links-the-wrong-issue-when-a-pr into master