Skip to content

🐛 Fix pr-review mispair after issue transfer (closes #105)#109

Merged
monkut merged 1 commit into
mainfrom
fix/105-pr-review-mispair-after-transfer
May 16, 2026
Merged

🐛 Fix pr-review mispair after issue transfer (closes #105)#109
monkut merged 1 commit into
mainfrom
fix/105-pr-review-mispair-after-transfer

Conversation

@monkut

@monkut monkut commented May 16, 2026

Copy link
Copy Markdown
Owner

Summary

  • _find_linked_pr_number used a Python in substring check on the PR body. f"#{5}" in "Closes weyucou/task-management#52" is True, so issue Add explore and diagnose commands #5 was paired with the wrong PR — daily cron then re-posted the same misplaced review 7 times.
  • It also had no awareness of the GitHub Timeline transferred_event, so the real fix-PR (branch chore/25-yaml-event-schema-v1, body Closes weyucou/task-management#25) referenced only the pre-transfer coordinates and was never recoverable.

Changes

  • Rewrite the body matcher to use CLOSE_KEYWORD_RE — a word-boundary regex that requires an explicit Closes/Fixes/Resolves (and inflections) keyword followed by #N or owner/repo#N.
  • Add _resolve_transferred_predecessors (askcc/functions.py) — queries gh api repos/{owner}/{repo}/issues/{N}/timeline, extracts transferred events, and returns [(prior_owner, prior_repo, prior_number), …]. All errors are logged and yield [], never raised.
  • Plumb predecessors into branch + body matching so a PR opened against the pre-transfer issue still pairs correctly. The current-repo /issues/N fallback retains a digit boundary.
  • Drop the unconditional substring fallback. A PR is returned only on a positive match — fetch_pr_content already treats None as "no link" and aborts the run (so cron becomes a no-op when matching is ambiguous).
  • Sync the standalone askcc/skills/handle-github-issue/SKILL.md PR-lookup prose with the new rules.

Tests

Six new tests in tests/test_askcc.py:

  • Regression: body Closes weyucou/task-management#52 does not pair with issue_number=5.
  • Substring collision rejection on bare #51 / #500 against issue_number=5.
  • Word-boundary positive path: Fixes #5 pairs with issue_number=5.
  • Transfer-aware branch: predecessor (weyucou, task-management, 25) + branch chore/25-yaml-event-schema-v1 pairs with current issue_number=5.
  • Transfer-aware body: predecessor + body Closes weyucou/task-management#25 pairs with current issue_number=5.
  • _resolve_transferred_predecessors parses one synthetic event, returns [] on a clean timeline, returns [] (with a logged warning) on gh and bad-JSON errors.

Plus _make_pr_responses in TestFetchPrContent now includes the additional timeline response.

Out of scope

Mitigation #3 — per-PR dedup on the review-comment side ((action, repo, target_number) key) — is enforced by the cron caller, not askcc. Flagged in the issue summary so the cron operator can coordinate that defense-in-depth layer. With this fix, askcc itself becomes a no-op when no PR is positively matched, which removes the source of the repeat posts.

Test plan

  • uv run poe check passes.
  • uv run poe test passes — 265 tests, including the 6 new ones.
  • Manual smoke once merged: re-run askcc pr-review against weyucou/issue-tracker#5 and confirm no comment is posted to PR#6.

Two defects in `_find_linked_pr_number` caused `askcc pr-review` to repeatedly
post on the wrong PR after an issue was transferred between repositories:

- The body fallback used Python `in` for `#{N}`, so `#5` matched `#52`.
- No awareness of the GitHub Timeline `transferred_event`, so the real
  fix-PR (referencing the pre-transfer coordinates) was never recovered.

Changes:
- Replace substring body checks with `CLOSE_KEYWORD_RE` — a word-boundary
  regex that requires an explicit close keyword
  (Closes/Fixes/Resolves and inflections) followed by `#N` or
  `owner/repo#N`.
- Add `_resolve_transferred_predecessors` — queries the issue timeline
  and returns prior `(owner, repo, number)` coordinates so PRs filed
  against the pre-transfer issue can still be paired after transfer.
  Errors are logged and yield an empty list, never raised.
- Drop the unconditional substring fallback. A PR is returned only on
  a positive match (branch convention, close keyword, or `/issues/N`
  with a digit boundary). `fetch_pr_content` already treats `None` as
  "no link" and aborts the run.
- Sync `handle-github-issue/SKILL.md` PR-lookup prose with the new
  rules (word boundaries, close keyword preference, timeline lookup).
- Six new tests covering the regression, substring collisions, the
  word-boundary positive path, transfer-aware branch + body matches,
  and the timeline parser's happy/empty/error paths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant