Skip to content

Commit fd63ca7

Browse files
BIMvoicelouistrue
andauthored
fix(ci): let check-issue-queue accept honest Refs #N partial work (#4161)
* fix(ci): let check-issue-queue accept honest Refs #N partial work (#4147) 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 * fix(ci): require intent before extractRefIssueNumbers counts a Refs #N 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. * fix(ci): hint at the accepted Refs #N form on a near-miss NO_LINKED_ISSUE 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. --------- Co-authored-by: Louis Trümpler <78563314+louistrue@users.noreply.github.com>
1 parent 1b0d1e7 commit fd63ca7

5 files changed

Lines changed: 1183 additions & 82 deletions

File tree

.github/workflows/issue-queue.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ jobs:
135135
# that decides whether work was requested should not be able to fail
136136
# because a registry was slow.
137137
- name: Unit-test the gate itself
138-
run: node --test scripts/check-issue-queue.test.mjs
138+
run: node --test scripts/check-issue-queue.test.mjs scripts/lib/issue-refs.test.mjs
139139

140140
- name: Check this PR closes a queued issue
141141
env:

scripts/check-issue-queue.mjs

Lines changed: 80 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,15 @@
66
* Steering gate: a PR from outside the maintainer must be work the maintainer
77
* ASKED FOR, or must say out loud that it is not.
88
*
9-
* THE MEASURED PROBLEM, because this gate is a policy and a policy without
10-
* evidence is a preference. This repository takes roughly half its commits from
11-
* one AI-driven external contributor. Of his 743 commits, 608 -- 82% -- carry
12-
* NO linked issue. That is not carelessness: the backlog is empty (19 open, 698
13-
* all-time), so an agent told to be useful has nothing to be useful ABOUT, and
14-
* manufactures its own work queue by sweeping the tree for defects. The
15-
* correlation runs the other way too, and it is the important half: when a
16-
* filed issue directed him he built FEATURES; undirected, he swept, endlessly,
17-
* at 27.7 PRs/day. The bottleneck is not throughput and never was. It is that
18-
* setting direction currently costs one review per PR, and there is no channel
19-
* that costs less.
9+
* THE MEASURED PROBLEM, because this gate is a policy and a policy without evidence is a
10+
* preference. This repository takes roughly half its commits from one AI-driven external
11+
* contributor. Of his 743 commits, 608 -- 82% -- carry NO linked issue. That is not
12+
* carelessness: the backlog is empty (19 open, 698 all-time), so an agent told to be useful
13+
* has nothing to be useful ABOUT, and manufactures its own work queue by sweeping the tree
14+
* for defects. The correlation runs the other way too, and it is the important half: when
15+
* a filed issue directed him he built FEATURES; undirected, he swept, endlessly, at 27.7
16+
* PRs/day. The bottleneck is not throughput and never was. It is that setting direction
17+
* currently costs one review per PR, and there is no channel that costs less.
2018
*
2119
* So this gate makes the cheap channel the only one: a label on an issue.
2220
*
@@ -29,40 +27,31 @@
2927
* BODY, AND THIS REPO HAS ALREADY PAID FOR THAT LESSON.
3028
*
3129
* #2978: the PR body said `Closes ... #2934` on line 1 and, on line 37, "this
32-
* NO_LABELS / NO_TIMELINE / NO_CLOSING_ISSUES A read came back without the
33-
* field it must have. All three are REACHABLE and all three
34-
* are gate bugs rather than contributor errors, so they carry
35-
* the same remedy as the truncation reasons: file it against
36-
* this gate and re-run the job. Named here because a refusal
37-
* with no next action teaches people to ignore refusals.
3830
* PR does not close #2934 on its own". Merging would have closed an issue
3931
* that stays open. Changing line 1 to `Addresses` was NOT enough --
4032
* `closingIssuesReferences` still returned 2934, because GitHub's keyword
4133
* scanner matched `close #2934` INSIDE THE SENTENCE DENYING IT. The scanner
4234
* has no notion of negation. Only rewording the disclaimer to "#2934 stays
4335
* open after this PR" cleared the link.
4436
*
45-
* The direction of that failure is what matters here. A body regex and the
46-
* real link DISAGREE, in both directions: the body can name an issue that is
47-
* not linked (a disclaimer, a "see also", a changelog quote), and the link
48-
* can name an issue the body does not (a closing keyword in a BRANCH COMMIT
49-
* MESSAGE, or a maintainer's manual sidebar link, neither of which appears in
50-
* the body at all). A gate built on a body regex would therefore both pass
51-
* work nobody queued and fail work the maintainer linked by hand.
52-
* `closingIssuesReferences` is the field GitHub itself acts on at merge time,
53-
* so it is the only field whose answer is the same answer.
54-
*
55-
* `userLinkedOnly: true` -- which would restrict the read to manual sidebar
56-
* links -- is deliberately NOT set. A manual link is if anything the STRONGER
57-
* steering signal, since only someone with write access can make one, and
58-
* excluding body- and commit-derived links would fail the ordinary "Closes
59-
* #N" PR this gate is trying to encourage.
60-
*
61-
* AND `gh pr list --search "<n>"` IS NOT AN ALTERNATIVE. AGENTS.md says so
62-
* under "Claiming work", in the same words: it "is a TEXT search: it matches
63-
* comment bodies, so it both misses linked PRs that never mention the number
64-
* and returns unrelated ones that happen to contain it." Nothing below uses
65-
* it.
37+
* The direction of that failure is what matters here. A body regex and the real link
38+
* DISAGREE, in both directions: the body can name an issue that is not linked (a disclaimer,
39+
* a "see also", a changelog quote), and the link can name an issue the body does not
40+
* (a closing keyword in a BRANCH COMMIT MESSAGE, or a maintainer's manual sidebar link,
41+
* neither of which appears in the body at all). A gate built on a body regex would therefore
42+
* both pass work nobody queued and fail work the maintainer linked by hand.
43+
* `closingIssuesReferences` is the field GitHub itself acts on at merge time, so it is the
44+
* only field whose answer is the same answer.
45+
*
46+
* `userLinkedOnly: true` -- which would restrict the read to manual sidebar links -- is
47+
* deliberately NOT set. A manual link is if anything the STRONGER steering signal, since
48+
* only someone with write access can make one, and excluding body- and commit-derived
49+
* links would fail the ordinary "Closes #N" PR this gate is trying to encourage.
50+
*
51+
* AND `gh pr list --search "<n>"` IS NOT AN ALTERNATIVE. AGENTS.md says so under "Claiming
52+
* work", in the same words: it "is a TEXT search: it matches comment bodies, so it
53+
* both misses linked PRs that never mention the number and returns unrelated ones
54+
* that happen to contain it." Nothing below uses it.
6655
*
6756
* ---------------------------------------------------------------------------
6857
* PART 2 -- WHO APPLIED THE LABEL IS CHECKED, BECAUSE IT IS CHECKABLE.
@@ -94,12 +83,11 @@
9483
* dependabot and the changeset release PR close no issue and never will, and
9584
* a gate that reddens every dependency bump is a gate that gets turned off.
9685
*
97-
* THE LOGIN IS NOT ONE STRING, AND THIS BIT ALREADY. On PR #3333, `gh pr
98-
* list --json author` says `app/dependabot`, GraphQL's `author { login }`
99-
* says `dependabot`, and REST says `dependabot[bot]`. Three spellings, one
100-
* actor. `normaliseLogin` folds case, strips a leading `app/` and a trailing
101-
* `[bot]`, and the config lists all three anyway so that the file can be
102-
* audited by reading it rather than by trusting this paragraph.
86+
* THE LOGIN IS NOT ONE STRING, AND THIS BIT ALREADY. On PR #3333, `gh pr list --json
87+
* author` says `app/dependabot`, GraphQL's `author { login }` says `dependabot`, and
88+
* REST says `dependabot[bot]`. Three spellings, one actor. `normaliseLogin` folds case,
89+
* strips a leading `app/` and a trailing `[bot]`, and the config lists all three anyway
90+
* so that the file can be audited by reading it rather than by trusting this paragraph.
10391
*
10492
* ---------------------------------------------------------------------------
10593
* THE TEETH, by failure class, each with its own remedy. Every one of these
@@ -132,6 +120,12 @@
132120
* be filed. It is a failure and not a pass because a partial read that
133121
* reports success is the defect class this repo keeps rediscovering.
134122
*
123+
* NO_LABELS / NO_TIMELINE / NO_CLOSING_ISSUES -- a read came back without the field
124+
* it must have. All three are REACHABLE and all three are gate bugs rather than
125+
* contributor errors, so they carry the same remedy as the truncation reasons
126+
* above: file it against this gate and re-run the job. Named here because a
127+
* refusal with no next action teaches people to ignore refusals.
128+
*
135129
* GH_UNAVAILABLE / GH_ERROR / GH_BAD_JSON / GRAPHQL_ERRORS / NO_PULL_REQUEST
136130
* / NO_AUTHOR / BAD_CONFIG / NO_CONFIG / BAD_ARGS / NO_REPO -- something
137131
* between here and GitHub did not answer.
@@ -142,41 +136,37 @@
142136
* STATED HOLES. Not caveats -- the things this gate is known not to do, written
143137
* down so nobody has to discover them by trusting it.
144138
*
145-
* 1. IT CANNOT TELL AN URGENT DRIVE-BY FIX FROM UNWANTED WORK. Main is red, a
146-
* release is half-published, a crash lands in production: none of that is
147-
* visible in `closingIssuesReferences`, and this gate will fail all three
148-
* exactly as hard as it fails a cosmetic sweep. THAT IS WHAT `escapeLabel`
149-
* IS FOR, and the escape hatch is not an admission of weakness -- a gate
150-
* with no escape gets disabled the first time it is wrong, and a disabled
151-
* gate steers nothing. The cost is that the escape is a human decision
152-
* taken per PR, which is the very cost this gate exists to reduce. It is
153-
* a smaller cost than reviewing 27.7 PRs a day, not zero.
154-
*
155-
* 2. THE ESCAPE LABEL IS ONLY AS STRONG AS `requireLabelAuthority`. With it
156-
* ON (the shipped default) a contributor cannot self-escape: the actor is
157-
* read from the timeline and a non-authority is SELF_APPLIED_LABEL. With
158-
* it OFF the gate is advisory, because anyone with write access can apply
159-
* the label to their own PR. The knob is in the config so that turning it
160-
* off is a reviewable act rather than a discovery.
161-
*
162-
* AND IT IS STILL DEFEATABLE BY A COLLABORATOR, one level up: someone who
163-
* can apply labels can also add themselves to `labelAuthorities`, in this
164-
* file, in a PR. What stops that is not this gate -- it is that the edit
165-
* is a visible line in a diff, and that `.github/workflows/issue-queue.yml`
166-
* has no `paths:` filter, so the PR making the edit is a PR this gate runs
167-
* on. A gate cannot outrank the people who can edit it. It can refuse to
168-
* let them do it quietly.
139+
* 1. IT CANNOT TELL AN URGENT DRIVE-BY FIX FROM UNWANTED WORK. Main is red, a release is
140+
* half-published, a crash lands in production: none of that is visible in
141+
* `closingIssuesReferences`, and this gate will fail all three exactly as hard as it fails
142+
* a cosmetic sweep. THAT IS WHAT `escapeLabel` IS FOR, and the escape hatch is not an
143+
* admission of weakness -- a gate with no escape gets disabled the first time it is wrong,
144+
* and a disabled gate steers nothing. The cost is that the escape is a human decision taken
145+
* per PR, which is the very cost this gate exists to reduce. It is a smaller cost than
146+
* reviewing 27.7 PRs a day, not zero.
147+
*
148+
* 2. THE ESCAPE LABEL IS ONLY AS STRONG AS `requireLabelAuthority`. With it ON (the shipped
149+
* default) a contributor cannot self-escape: the actor is read from the timeline and a
150+
* non-authority is SELF_APPLIED_LABEL. With it OFF the gate is advisory, because anyone with
151+
* write access can apply the label to their own PR. The knob is in the config so that turning
152+
* it off is a reviewable act rather than a discovery.
153+
*
154+
* AND IT IS STILL DEFEATABLE BY A COLLABORATOR, one level up: someone who can apply labels
155+
* can also add themselves to `labelAuthorities`, in this file, in a PR. What stops that
156+
* is not this gate -- it is that the edit is a visible line in a diff, and that
157+
* `.github/workflows/issue-queue.yml` has no `paths:` filter, so the PR making the edit
158+
* is a PR this gate runs on. A gate cannot outrank the people who can edit it. It can
159+
* refuse to let them do it quietly.
169160
*
170161
* 3. IT SAYS NOTHING ABOUT WHETHER THE WORK IS ANY GOOD. A `ready` issue
171162
* closed by a bad patch passes. This is a routing check, not a review.
172163
*
173164
* 4. IT CANNOT SEE THE COMMIT-MESSAGE HALF OF THE LINK UNTIL IT EXISTS.
174-
* `closingIssuesReferences` is computed by GitHub from the body AND the
175-
* branch's commit messages, so it is correct the moment either exists --
176-
* but on a PR opened before the linking commit is pushed, the answer is
177-
* legitimately empty and the gate legitimately fails. Re-running after the
178-
* push is the remedy, and `synchronize` in the workflow's trigger list
179-
* means the re-run is automatic.
165+
* `closingIssuesReferences` is computed by GitHub from the body AND the branch's commit
166+
* messages, so it is correct the moment either exists -- but on a PR opened before the
167+
* linking commit is pushed, the answer is legitimately empty and the gate legitimately fails.
168+
* Re-running after the push is the remedy, and `synchronize` in the workflow's trigger
169+
* list means the re-run is automatic.
180170
*
181171
* 5. AN ISSUE CAN BE LABELLED `ready` AND THEN UNLABELLED. The gate reads the
182172
* CURRENT label set, so an issue whose label was removed stops passing,
@@ -191,12 +181,11 @@
191181
* not a wider guess.
192182
*
193183
* ---------------------------------------------------------------------------
194-
* WIRED BY `.github/workflows/issue-queue.yml`, which carries no `paths:`
195-
* filter for the reason that workflow's own header sets out at length, copied
196-
* from `.github/workflows/pr-review-signal.yml`: a gate whose input can be
197-
* filtered out of its own trigger is the defect it is trying to catch. Its
198-
* regression harness is `scripts/check-issue-queue.test.mjs`, run in the same
199-
* job, before the gate.
184+
* WIRED BY `.github/workflows/issue-queue.yml`, which carries no `paths:` filter for
185+
* the reason that workflow's own header sets out at length, copied from
186+
* `.github/workflows/pr-review-signal.yml`: a gate whose input can be filtered out
187+
* of its own trigger is the defect it is trying to catch. Its regression harness is
188+
* `scripts/check-issue-queue.test.mjs`, run in the same job, before the gate.
200189
*
201190
* Usage:
202191
* node scripts/check-issue-queue.mjs --pr 3540 --repo LTplus-AG/ifc-lite
@@ -210,6 +199,7 @@ import { dirname, join } from 'node:path';
210199
import { fileURLToPath } from 'node:url';
211200
import { isMainEntry } from './lib/is-main-entry.mjs';
212201
import { existsOrThrow } from './lib/exists-or-throw.mjs';
202+
import { buildRefIssues, fetchRefIssuesIfNeeded, findNearMissRefIssueNumbers, partialWorkVerdict, unqueuedRefsNote, nearMissRefsNote } from './lib/issue-refs.mjs';
213203

214204
const SCRIPTS_DIR = dirname(fileURLToPath(import.meta.url));
215205
const DEFAULT_CONFIG = join(SCRIPTS_DIR, 'issue-queue.config.json');
@@ -410,6 +400,7 @@ query($owner:String!, $name:String!, $number:Int!) {
410400
pullRequest(number:$number) {
411401
number
412402
title
403+
body
413404
author { login }
414405
labels(first:100) { pageInfo { hasNextPage } nodes { name } }
415406
timelineItems(last:100, itemTypes:[LABELED_EVENT]) {
@@ -649,6 +640,7 @@ export function normalisePullRequest(payload) {
649640
labels: labelSet(issue?.labels, `Issue #${issue?.number}`),
650641
labelHistory: timelineOf(issue?.timelineItems, `Issue #${issue?.number}`),
651642
})),
643+
refIssues: buildRefIssues(pr.body, issuesConn.nodes, payload?.refIssues, labelSet, timelineOf), nearMissRefs: findNearMissRefIssueNumbers(pr.body), // #4147, cosmetic near-miss hint
652644
};
653645
}
654646

@@ -757,6 +749,10 @@ export function evaluate({ pr, cfg }) {
757749
: null;
758750

759751
if (pr.issues.length === 0) {
752+
// #4147: honest partial work -- a `ready`, OPEN issue named with a non-closing keyword. Can
753+
// only WIDEN this failure into a pass, never narrow a pass into a fail; see lib/issue-refs.mjs.
754+
const partial = partialWorkVerdict({ refIssues: pr.refIssues, readyLabel: cfg.readyLabel, adjudicateLabel: (h, l) => adjudicateLabel(h, l, cfg), escapeProblem, escapeReason: escape.reason });
755+
if (partial) return partial;
760756
lines.push(
761757
// Conditional on escapeProblem: the PR may well CARRY the escape label and
762758
// have it rejected below. Saying "carries no `unqueued` label" while the
@@ -782,6 +778,7 @@ export function evaluate({ pr, cfg }) {
782778
' If you pushed the linking commit after opening this PR, re-run: the link appears when ' +
783779
'the commit does.',
784780
);
781+
lines.push(...unqueuedRefsNote(pr.refIssues, cfg.readyLabel), ...nearMissRefsNote(pr.nearMissRefs)); // #4147
785782
if (escapeProblem) lines.push('', ...escapeProblem);
786783
// The PRIMARY failure is the verdict; the escape problem is carried in
787784
// `lines`. Returning escape.reason here made the field disagree with the
@@ -906,6 +903,9 @@ function main() {
906903
);
907904
}
908905
payload = fetchPayload({ repo, pr: args.pr });
906+
// #4147: a second round trip, taken only when the first could not already pass.
907+
const fail = (reason, message) => { throw new IssueQueueError(reason, message); };
908+
payload.refIssues = fetchRefIssuesIfNeeded({ payload, repo, spawn: spawnSync, fail });
909909
}
910910
if (args.dump) writeFileSync(args.dump, JSON.stringify(payload, null, 2));
911911

0 commit comments

Comments
 (0)