fix: link a PR to its issue on any #N reference, not just closing keywords (#652) - #679
Conversation
…words (#652) The digest linked an open PR to its issue only when the PR body used a closing keyword (fixes/closes/resolves/refs #N). The no-auto-close rule forbids those on intermediate PRs -- they say "Part of #N", "tracking #N", or a bare "#N" -- so such a PR was invisible: issue #409 reported In Progress while its approved PR (#490) sat open. Linkage now matches any #N reference, with documented cross-ref phrases (Blocked by, Depends on, Unblocks, Related to, Relationship to, See also, See, Not part of) stripped first so they never link. The merged-PR scan is deliberately narrower (work verbs: fixes/closes/ resolves/refs/part of/tracking/tracks): a merged intermediate PR keeps its issue In Verification (a narrowed scan had flipped #643/#542/#571/#592/#666 back to Backlog/Ready for Dev on the live board), while a merged PR that merely names another issue in prose does not. mergeable is re-queried until it leaves UNKNOWN (budget 6) and emitted null if it never resolves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-digest-link-any-ref
|
@claude-bot review |
There was a problem hiding this comment.
Verdict: APPROVE
Matches linked issue's root cause (#652): Yes, exactly. #652 diagnosed two independent defects and prescribed both fixes verbatim:
pr_matches_issuematched only(fixes|closes|resolves) #N, which the no-auto-close convention forbids on intermediate PRs — so a "Part of #409" PR resolved to no issue and #409's column fell through to the live-worktree branch, reporting In Progress for an approved, mergeable PR.mergeable: UNKNOWN(GitHub's lazily-computed first-query value) was passed through as if final.
scripts/backlog-digest.sh's new linkage_body/pr_matches_issue (any #N, with documented cross-ref phrases like Blocked by/Related to/Unblocks stripped first) and the bounded MERGE_RETRY_LIMIT/MERGE_RETRY_SLEEP re-query loop implement exactly what #652's "Proposed fix" section asked for. The widened merged-PR regex (fixes|closes|resolves|refs|part of|tracking|tracks, still deliberately excluding bare #N) fixes a real regression the PR's own root-cause section documents was introduced mid-fix (narrowing it flipped #643/#542/#571/#592/#666 off In Verification on the live board) — same underlying defect class as #652, not unrelated scope.
Would the tests catch a regression? Yes, verified independently, not just taken on the PR's word. I checked out the branch, ran the full test_backlog_digest.py suite (68 passed), then mutated pr_matches_issue back to the pre-fix closing-keyword-only regex and re-ran: test_a_bare_number_reference_links_a_pr_to_its_issue, test_a_blocked_by_side_reference_still_links_the_prs_own_issue, and one other failed as expected (3 failed / 65 passed), then restored to green. The tests assert board-visible outcomes (item[\"column\"], item[\"prs\"], orphan lists) rather than jq internals, consistent with docs/agents/testing.md. I also hand-traced the linkage_body regex against every parametrized non-work-reference case (Related to, Unrelated to, Unblocks/Unblocking, See/See also, Not part of) and the boundary regex (#2409 vs issue 409, #4095 vs issue 409) — all correct, including the accidental-but-correct substring behavior on Unrelated to #900.
Minimal / no scope creep: Yes. Three files touched (CHANGELOG.md, the script, its test file), all load-bearing for #652. The new merged_prs (plural) field is the one piece not explicitly asked for in #652, but it falls directly out of widening the merged-PR regex to work-verb spellings (now multiple merged PRs can legitimately match one issue) and is cheap, tested, additive visibility data — not a separate feature.
Rule violations: None found. No Python/API-layer/architecture rules apply (pure bash/jq + test file). No secrets, no injection vectors, no new classes. mergeable's null-on-still-UNKNOWN change doesn't affect backlog-rhythm.sh, which queries mergeable independently via its own gh pr view call and never reads the digest's prs[].mergeable field — confirmed by grep.
Minor observation (non-blocking): The PR description doesn't include an explicit "local fix / structural fix / escalated" scope-assessment line per rules.md Debugging Protocol step 9. I judged this a local fix — confined to one script's existing linkage/merge-scan responsibilities, no new cross-module ownership — so I'm not treating the missing statement as a blocker, but flagging it since the checklist calls for it explicitly.
CI: all checks green (Fast tests, E2E, Docker build, Frontend, Code quality, Merge gate; Algorithm tests skipped as expected for this diff).
Summary
#Nbody reference, not just closing keywords (fixes/closes/resolves/refs #N). A no-auto-close PR that says "Part of #N", "tracking #N", or a bare#Nis visible on the board instead of invisible.Blocked by #N,Depends on,Unblocks/Unblocking,Related to,Relationship to,See/See also,Not part of) are phrase-stripped before matching, so a PR that merely names another issue never links to it.fixes/closes/resolves/refs/part of/tracking/tracks): a merged intermediate PR keeps its issue In Verification (never re-dispatchable), while a merged PR that names another issue in prose does not flip it. A newmerged_prsfield exposes every merged work-referencing PR.mergeableis re-queried until it leaves UNKNOWN (budget 6, covering the Consolidate PredictionSnapshotStore into the unified per-day DailyView format #490 six-pass measurement) and emittednullwhen still UNKNOWN.Root cause
pr_matches_issueinscripts/backlog-digest.shmatched(fixes|closes|resolves|refs) #Nonly. The no-auto-close rule forbids closing verbs on intermediate PRs — they carryPart of #N/tracking #N/ a bare#N— so those PRs resolved to no issue and the issue's column fell through to the live worktree: issue #409 reported In Progress while its approved PR #490 sat open. The merged-PR scan had the same closing-keyword-only blind spot in reverse: narrowing it (during this fix's first pass) flipped #643/#542/#571/#592/#666 from In Verification to Backlog / Ready for Dev on the live board, because their merged PRs (#675/#591/#584/#619/#672) usedRefs #N-style references.Fix
pr_matches_issuenow tests(^|[^0-9])#N\bon alinkage_bodythat first strips the documented cross-ref phrases, soPart of #N/tracking #N/Refs #N/ bare#Nall link while cross-refs never do. Stripping the phrase (not the line) keeps combined references like- Blocked by #100 -- part of #409working.#N, so prose ("until Quarterly schedule-update tick: prevent misfire under long hardware-write retries #456 and Quarterly schedule-update tick: backfill/retry sweep for a missed period #457 are also resolved") cannot flip an unrelated issue to In Verification.merged_pr_forgates the column; the newmerged_prsfield lists every match, sorted.mergeable: bounded re-query (6 passes) around GitHub's lazy computation;prs_foremitsnullwhen it never settles.Test plan
./scripts/quality-check.shpasses locally (Errors: 0).venv/bin/pytest -m slowpasses locally (554 passed, 8 skipped)merged_prsdata), preserves baseline open-PR linkage (backlog-digest matches sessions by cwd, so a dispatched implement-issue session is invisible — rhythm re-proposes resume every tick #647→677, Accept an external consumption forecast as a time-series entity #428→437, Addexternal_solar_modebattery setting for AC-coupled PV systems #162→167), leaks zero UNKNOWN/nullmergeables, and keeps the same threepr_no_issueorphans (PRs fix: stop the fleet prune from destroying the worktrees it cannot remove #653/feat: enforce review-loop legality and derive fleet state from GitHub #638/fix: gate grid_first on export materiality instead of intent classification #354 — all reference only merged/closed numbers).docs/agents/bess-knowledge.mdanddocs/SOFTWARE_DESIGN.mdeach contain 0 matches for "backlog" — neither describes the digest mechanism, so no doc update is owed.Evidence the test discriminates
The RED tests were written first and observed failing on the pre-fix script; the mutations below confirm they still discriminate on the final tree.
fixes|closes|resolves|refs)test_a_merged_intermediate_pr_keeps_the_issue_in_verificationFAILED (a merged "Part of fix: serve sub-lattice net load from the battery instead of idling (#466) #517" PR no longer kept fix: serve sub-lattice net load from the battery instead of idling (#466) #517 In Verification); 63 tests total#Ntest_a_merged_cross_ref_does_not_move_an_issue_to_in_verificationFAILED ("Related to Quarterly schedule-update tick can be silently skipped when hardware retries run long, permanently dropping a period's actuals #403… until Quarterly schedule-update tick: prevent misfire under long hardware-write retries #456/Quarterly schedule-update tick: backfill/retry sweep for a missed period #457" flipped Quarterly schedule-update tick can be silently skipped when hardware retries run long, permanently dropping a period's actuals #403 to In Verification)linkage_bodyphrase-strip neutralizedtest_a_blocked_by_reference_does_not_link_a_pr_to_its_issueFAILED (and the 8-casetest_a_non_work_reference…parametrization)relationship to,see,not part of, andunblockingstrip alternativestest_a_non_work_reference_does_not_link_a_pr_to_its_issue[<phrase>]case FAILED each timeOutcome-level coverage
_gh_shim-driven digest runs above; there is no optimizer/execution model involved.Refs #652