pr-review heuristic mispairs after issue transfer (branch encodes pre-transfer issue number)

[ellen-goc]

Symptom

askcc pr-review repeatedly mispaired weyucou/issue-tracker#5 (YAML event schema, transferred from weyucou/task-management#25) with weyucou/issue-tracker#6 (.gitignore, closes weyucou/task-management#52). Each daily cron tick posted a fresh review comment on PR#6 from 2026-05-01 to 2026-05-06 — 7 redundant misposted comments before the issue was relabeled.

Root cause

Two distinct defects in _find_linked_pr_number (askcc/functions.py:97):

1. Substring match on #N: the body fallback uses f"#{issue_number}" in body. For issue_number=5, the substring #5 matches #52, #53, … This is how PR#6 (body referencing task-management#52) was incorrectly paired with issue#5.
2. No transfer-history awareness: after weyucou/task-management#25 was transferred to weyucou/issue-tracker#5, the real fix-PR (PR#2, branch chore/25-yaml-event-schema-v1, body Closes weyucou/task-management#25) referenced only the pre-transfer coordinates. The heuristic never consults the GitHub Timeline transferred_event, so it cannot recover the link.

The wrong PR then passed substring matching and was returned as the linked PR — the rest of pr-review had no way to detect the mismatch and posted a review on it.

Compounding issue

No per-PR dedup on the resulting review comment — every cron tick re-posted the same mismatch warning to PR#6. The 24-hour comment dedup gate evaluates the issue, not the misposted PR. (Caller/cron side — see notes below.)

Current state

• askcc/functions.py:97 — _find_linked_pr_number(gh, owner, repo, issue_number):
  • Branch match: re.compile(rf"^[^/]+/{issue_number}-") — correctly anchored, no bug here.
  • Body fallback: if f"#{issue_number}" in body or f"/issues/{issue_number}" in body — substring bug.
• askcc/functions.py:131 — fetch_pr_content raises ValueError("No linked pull request found …") when _find_linked_pr_number returns None; the CLI propagates this and pr-review exits without posting. So fixing the matcher to return None for the ambiguous case is sufficient — no extra "abort" plumbing required.
• askcc/skills/handle-github-issue/SKILL.md:353-368 — mirrors the same heuristic in prose for the standalone-skill install. Must be kept in sync.
• tests/test_askcc.py:1623 — TestFindLinkedPrNumber covers the happy paths but has no negative test for substring collision or transfer-history matching.

Implementation Plan

Step 1 — Word-boundary body matching (fixes the immediate mispair)

In askcc/functions.py:_find_linked_pr_number, replace the substring fallback with regex matches that enforce a digit boundary:

• re.search(rf"(?<!\d)#{issue_number}(?!\d)", body)
• re.search(rf"/issues/{issue_number}(?!\d)", body)

This alone prevents #5 from matching #52 and is the minimum fix to stop the mispair recurrence.

Step 2 — Prefer explicit close keywords over bare #N

Tighten the body matcher to require GitHub's auto-link keywords for the primary positive match (case-insensitive):

CLOSE_KEYWORD_RE = re.compile(
    rf"\b(?:closes|closed|close|fixes|fixed|fix|resolves|resolved|resolve)\s+"
    rf"(?:(?P<qualified>[\w.-]+/[\w.-]+)#)?(?P<num>\d+)\b",
    re.IGNORECASE,
)

Walk all matches; for each, if qualified is set check it against (owner, repo) or any predecessor (Step 3), and check num against issue_number (or predecessor prior_N). Keep the bare #N//issues/N matchers as a secondary fallback so non-conventional PR bodies are not regressed.

Step 3 — Transfer-history aware lookup

Add _resolve_transferred_predecessors(gh, owner, repo, issue_number) -> list[tuple[str, str, int]]:

• Call gh api --paginate repos/{owner}/{repo}/issues/{N}/timeline.
• For each event where event == "transferred", extract from source.issue:
  • repository.owner.login → prior_owner
  • repository.name → prior_repo
  • number → prior_number
• Append (prior_owner, prior_repo, prior_number) to the list. (GitHub does not currently return multi-hop chains via this endpoint — predecessors point only to the immediately prior issue. The function returns a list to keep the API stable if that changes.)
• Wrap all subprocess/JSON errors and return [] — never raise. Log a warning so misconfigured permissions surface in cron logs.

Plumb the predecessor list into _find_linked_pr_number:

• Branch convention: also match ^[^/]+/{prior_number}- (PRs live in the current repo after transfer; the branch number stays as it was at creation).
• Body keywords (Step 2): also accept Closes {prior_owner}/{prior_repo}#{prior_number} (and Fixes/Resolves variants).

Step 4 — No-positive-match → return None

After Steps 1–3, drop the unconditional fallback "any PR body containing #N". A PR may only be returned when:

• Its branch matches the issue number or a predecessor's number, or
• Its body explicitly closes the issue (or a predecessor) via the keyword regex, or
• Its body contains /issues/N with a digit boundary referencing the issue or a predecessor.

If none of those positive matches hold, return None. fetch_pr_content already raises ValueError on None, which makes the cron run a no-op. This is mitigation #1 from the issue: "Don't run pr-review when the chosen PR's Closes reference doesn't include the input issue."

Step 5 — Sync the standalone skill

Update askcc/skills/handle-github-issue/SKILL.md:353-368 to describe the new heuristic in prose:

• Word-boundary matching (so #5 does not match #52).
• Prefer explicit Closes/Fixes/Resolves references over bare #N.
• Before falling back, query gh api repos/<owner>/<repo>/issues/<N>/timeline for transferred events and accept matches against each predecessor's owner/repo#N and branch number.
• If no positive match is found after all checks, abort the action without posting.

Step 6 — Tests

In tests/test_askcc.py, extend TestFindLinkedPrNumber and add TestResolveTransferredPredecessors:

• Regression for the original bug: PR body "Closes weyucou/task-management#52" with issue_number=5 returns None (was returning the PR's number).
• Substring-collision: PR body "See #51, #500" with issue_number=5 returns None.
• Word-boundary positive: PR body "Fixes #5" with issue_number=5 returns the PR's number.
• Transfer-aware branch: predecessor list [("weyucou", "task-management", 25)] + PR branch chore/25-yaml-event-schema-v1 + issue_number=5 returns the PR's number.
• Transfer-aware body: PR body "Closes weyucou/task-management#25" + same predecessor + issue_number=5 returns the PR's number.
• _resolve_transferred_predecessors: parses a synthetic timeline payload with one transferred event correctly; returns [] for a clean timeline; returns [] (with a logged warning) on gh failure or bad JSON.

Step 7 — Caller-side dedup (out of scope, documented)

Mitigation #3 ((action, repo, target_number) dedup) is enforced by the cron caller, not askcc. Note this in the summary comment so the cron operator can coordinate the change.

Affected Files

• askcc/functions.py — rewrite _find_linked_pr_number (~L97); add _resolve_transferred_predecessors; introduce the CLOSE_KEYWORD_RE constant.
• askcc/skills/handle-github-issue/SKILL.md — update PR-lookup prose (~L353-368).
• tests/test_askcc.py — extend TestFindLinkedPrNumber (~L1623); add TestResolveTransferredPredecessors.

askcc/definitions.py:REVIEWPR_AGENT_PROMPT does not need editing: the Python heuristic runs before the agent in the askcc package flow, so the agent prompt is unaffected. The skill prose change at Step 5 is the only documentation surface.

Acceptance Criteria

• _find_linked_pr_number returns None for issue_number=5 against a PR whose only body reference is Closes weyucou/task-management#52 (regression of the reported root cause).
• _find_linked_pr_number returns the PR number for issue_number=5 against a PR with branch chore/25-yaml-event-schema-v1 when _resolve_transferred_predecessors reports [("weyucou", "task-management", 25)].
• _find_linked_pr_number returns the PR number for issue_number=5 against a PR body Closes weyucou/task-management#25 under the same predecessor set.
• _find_linked_pr_number returns None when no PR body uses an explicit close keyword referencing the issue or any predecessor and no branch matches.
• _resolve_transferred_predecessors parses one synthetic transferred event payload, returns [] on a clean timeline, and returns [] on gh / JSON errors without raising.
• askcc/skills/handle-github-issue/SKILL.md PR-lookup section describes word-boundary matching, close-keyword preference, and the timeline transferred_event lookup.
• tests/test_askcc.py adds the six tests listed in Step 6; full suite passes.
• uv run poe check and uv run poe test both pass.

Dependencies

• GitHub Timeline API: gh api repos/{owner}/{repo}/issues/{N}/timeline returns transferred events containing source.issue.repository.{owner.login,name} and source.issue.number. No preview-header opt-in is required on the current REST API. Verified payload shape via GitHub REST docs.
• No new Python dependencies — uses stdlib re and the already-imported subprocess / gh.

Risks / Open Questions

• Timeline API cost: one extra paginated gh api call per pr-review run. Per-cron impact is small (one issue at a time), but flag it for any future batched caller.
• Multi-hop transfers: GitHub Timeline currently only exposes the immediately prior issue. If A → B → C and we are at C, only B is recoverable. Returning a list keeps the API stable; chase deeper only if a real case appears.
• Per-target dedup (mitigation Add release workflow and bump to v0.1.1 #3): belongs to the cron caller. Will note in the summary comment for the operator. Not implemented here.

[monkut]

Plan summary

Updated the issue body with ## Implementation Plan, ## Acceptance Criteria, and ## Dependencies sections, and self-assigned.

What's changing

• askcc/functions.py:_find_linked_pr_number — replace substring in checks with word-boundary regex, prefer explicit Closes/Fixes/Resolves keywords, add transfer-history-aware matching via the GitHub Timeline transferred_event, and return None instead of falling through when there's no positive match.
• New helper _resolve_transferred_predecessors to look up prior (owner, repo, number) coordinates of a transferred issue.
• askcc/skills/handle-github-issue/SKILL.md — sync the standalone-skill PR-lookup prose with the same rules.
• tests/test_askcc.py — six new tests, including a regression test that asserts #5 no longer matches a body referencing #52 (the original bug).

Root cause in one line

The body fallback used Python in for #{N}, so "#5" in "task-management#52" returned True and the heuristic gleefully paired issue#5 with the wrong PR — followed by 7 daily cron re-posts because no positive match was ever required.

Risks / open questions

• One extra paginated gh api …/timeline call per pr-review run. Acceptable for cron cadence.
• GitHub Timeline exposes only the immediately prior transfer hop. If a multi-hop chain ever appears, the helper returns a list so we can extend later.
• Mitigation Add release workflow and bump to v0.1.1 #3 (per-target dedup keyed by (action, repo, target_number)) is caller-side (cron) and out of scope for this issue. Flagged for the operator — askcc itself becomes a no-op when no PR is matched, which removes the source of the repeat posts; per-target dedup remains a defense-in-depth layer for the cron caller.

:tokens-used: model: N/A, input: 35, output: 15361

[monkut]

PR #109 implements this issue.

What shipped

• _find_linked_pr_number now requires a positive match. The substring #{N} body check is replaced with a word-boundary CLOSE_KEYWORD_RE that recognises Closes/Fixes/Resolves (and inflections) plus #N or owner/repo#N. Bare /issues/N retains a digit-boundary fallback.
• New _resolve_transferred_predecessors(gh, owner, repo, issue_number) walks gh api repos/{owner}/{repo}/issues/{N}/timeline, extracts transferred events, and returns [(prior_owner, prior_repo, prior_number), …]. Errors are logged and yield [], never raised.
• Predecessors plumb through both branch matching (^[^/]+/{N}-) and body matching, so a PR opened against the pre-transfer issue still pairs after the transfer.
• The unconditional substring fallback is dropped. When nothing positively matches, _find_linked_pr_number returns None and fetch_pr_content raises ValueError("No linked pull request found …") — the cron caller becomes a no-op for the ambiguous case.
• askcc/skills/handle-github-issue/SKILL.md is synced with the same heuristic in prose so the standalone-skill install matches the Python flow.

Tests

Six new tests in tests/test_askcc.py:

• Regression: body Closes weyucou/task-management#52 does not pair with issue_number=5.
• Substring rejection: See #51, #500 does not pair with issue_number=5.
• Positive path: Fixes #5 pairs with issue_number=5.
• Transfer-aware branch + body matches under predecessor (weyucou, task-management, 25) for current issue_number=5.
• _resolve_transferred_predecessors parsing, clean timeline, and error paths.

uv run poe check and uv run poe test (265 tests) both pass.

Out of scope — flag for the cron operator

Mitigation #3 (per-target dedup keyed by (action, repo, target_number)) is enforced by the cron caller, not askcc. This PR removes the source of the repeat posts by making askcc a no-op on ambiguous matches; per-target dedup remains a defense-in-depth layer for the cron caller.

:tokens-used: model: N/A, input: 43, output: 32607