fix(ci): let check-issue-queue accept honest Refs #N partial work - #4161
Conversation
) closingIssuesReferences is populated only by a closing keyword, so a PR that is deliberately a slice of a larger issue could not link it without also claiming (and auto-closing) work it did not finish. Five PRs hit this in one evening, all correctly writing `Refs #N` instead of lying with `Closes #N`, and all needing a manual `unqueued` waiver whose meaning is the opposite of the truth. Add a second, narrower accepted shape in scripts/lib/issue-refs.mjs: when closingIssuesReferences closes nothing, the PR body may still name an issue with a non-closing keyword (Refs/References/Part of/Towards). That issue must independently be OPEN and carry `ready` from an authorised applier -- the same check a closing link already goes through -- so a PR referencing a not-ready or closed issue still fails exactly as before. The self-applied- label rule and the escape-label mechanism are unchanged. Closes #4147
|
Warning Review limit reached
This review includes 5 billable files and costs up to $1.25. Or wait 59 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Comment |
Claude review - no findings for
|
extractRefIssueNumbers matched Refs #N anywhere in a PR body, including
inside a fenced code block, an inline code span, a blockquote, or as an
ordinary verb mid-sentence ("this function refs #12 in a loop"). Since
this module can only widen a PARTIAL_WORK fail into a pass, a match with
no authorial intent behind it is a live bypass: an unrelated PR could
pass the issue-queue gate by quoting a ready issue number in a code
sample or a quoted reply.
extractRefIssueNumbers now strips fenced code blocks, inline code spans,
and blockquote lines before scanning, and requires the keyword to start
its line (optionally after a list marker: -, *, +, 1., 1)). Refs #N,
References #N, Part of #N, Towards #N, and refs: #N all keep working in
their ordinary positions; two pre-existing unit test fixtures that ran
multiple keywords together on one line were split one-per-line to match
how these are actually written.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Claude review - no findings for
|
…SSUE fail 910d38f's tightening of extractRefIssueNumbers is correct and stays as-is: a Refs #N wrapped in backticks mid-sentence, or quoted inside a fence, no longer grants a PARTIAL_WORK pass. But that tightening left the failure message silent about why a plausible-looking reference didn't count -- the exact phrasing "...`Refs #N`, and requesting the `unqueued` label..." showed up in two PR bodies today, and would now fail with no clue what to fix. Adds findNearMissRefIssueNumbers (lib/issue-refs.mjs): the permissive keyword+#N scan the strict matcher replaced, run only outside fenced code (the confirmed exploit shape stays silent, on purpose) and excluding numbers the strict matcher already accepted. Its only consumer is nearMissRefsNote, which appends a REMEDY line to the NO_LINKED_ISSUE failure output naming the issue and the accepted line-start form. Neither function is read by evaluate()'s ok/verdict decision -- verified by mutation: disabling the hint call, and removing the fence exclusion, each turn exactly one targeted test red and nothing else. Kept scripts/check-issue-queue.mjs at its allowlisted 957 lines and scripts/lib/issue-refs.mjs at the 400-line module-size limit by combining the two new call sites onto existing lines rather than adding new ones.
Claude review - no findings for
|
|
Ran 2 of 4 clear once #4161 merges:
2 stay red — need a manual
Also checked #4167, which the same Refs-#4153 shape would clear (verified: #4151 and #4152 still need |
Claude review - no findings for
|
Summary
closingIssuesReferencesis populated only by a closing keyword, so a PR that is deliberately a slice of a larger issue could not honestly link it without also claiming (and auto-closing) work it did not finish. Five PRs hit exactly this in one evening, all correctly writingRefs #Ninstead of lying withCloses #N, and all needing a manualunqueuedwaiver whose meaning ("did not need to wait for the queue") is the opposite of the truth.scripts/lib/issue-refs.mjs, consulted only whenclosingIssuesReferencescloses nothing: the PR body may name an issue with a non-closing keyword (Refs,References,Part of,Towards, case-insensitive). That issue must independently be OPEN and carryreadyfrom an authorised applier — the exact same check a closing link already goes through — so:readystill fails (NO_LINKED_ISSUE)readyissue that is CLOSED still fails (a closed issue is not a queue entry)readylabel on the referenced issue still fails (SELF_APPLIED_LABEL, same authority check)unqueuedstill works unchangedCloses #Non a ready issue is unaffected — it passes before this new code path is ever reachedclosingIssuesReferencesstays the primary, preferred signal; nothing here weakens it or adds a self-service bypass.Refs #Nanywhere in the text, including inside a fenced code block, an inline code span, or a blockquote, and as an ordinary verb mid-sentence ("this function refs Compare codebase with IFC libraries #12 in a loop") — none of which name a queue entry on purpose. Since this module can only widen a fail into a pass, an intentless match was a live bypass: any PR could pass the gate by quoting areadyissue number in a code sample or a quoted reply.extractRefIssueNumbersnow strips fenced code blocks, inline code spans, and blockquote lines before scanning, and requires the keyword to start its line (optionally after a list marker:-,*,+,1.,1)) — which is how a realRefs #Nis actually written, and which also rejects the mid-sentence-verb case without a separate rule for it.Refs #N,References #N,Part of #N,Towards #N, andrefs: #Nall keep working in their ordinary positions.Implementation notes
PARTIAL_WORK(pass), printed with the same detail asREADY_ISSUE.gh api graphqlround trip for the referenced issue(s), but only when the first read already closed nothing and the body names a candidate — an ordinary PR that closes its issue pays no extra cost and cannot be failed by this path.scripts/lib/issue-refs.mjs(keptcheck-issue-queue.mjsat its existing 957-line module-size budget without raising it — the file's own header carried a genuine pre-existing formatting defect, aNO_LABELS/NO_TIMELINE/NO_CLOSING_ISSUESparagraph spliced mid-sentence into an unrelated story, which is also fixed here as a byproduct of reflowing the prose to make room).Closeswhile admitting partial coverage); not implemented here.Test plan
node --test scripts/check-issue-queue.test.mjs scripts/lib/issue-refs.test.mjs— 95/95 pass, including end-to-end cases and unit tests covering the regex, the query builder, the fail-closed live-fetch wrapper, the verdict logic, and the intent-scoping pre-pass.test()cases incheck-issue-queue.test.mjsgo RED (one of them loops over 4 keyword variants), everything else stays green.state === 'OPEN'filter — the CLOSED-issue test (both the integration test and itslib/issue-refs.test.mjsunit twin) goes RED; reverted.node scripts/check-module-size.mjs— OK, 0 new over budget.node scripts/check-test-wiring.mjs— OK.Closes #4147