diff --git a/.github/scripts/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index 6448fe89..f3618d0b 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -1160,3 +1160,245 @@ test('P-479 (2026-11-03): a conclusion-shaped comment without a lift phrase does assert.match(r.reasons.join('; '), /asks not to merge/) assert.match(r.reasons.join('; '), /6596354014/, 'the refusal still names the stop that stands') }) + +// #2287 (2026-11-03): the predicate that clears a stop was wrong in both directions on `19e206ecb`. +// A. a lift that is negated — "not asking you to lift the hold — you may merge this tip only after the +// second reader re-runs" — carried a quotation of the PASS, a lift phrase and no stop phrase, so it +// cleared the hold it refuses to lift. +// B. the same sentence under a condition — "You may merge this tip once the second reader re-runs" — did +// the same. +// C1. the answer written on an opening line — "Hold cleared — you may merge this tip now" + its quotation — +// was read as a *hold*, because `HOLD_LINE` matched the word it begins with; the same sentence mid-line +// cleared. +// F. that sentence without the quotation never became a conclusion, so it entered the stop scan as an +// ordinary comment and made itself the stop, naming its own id. +// The three real holds at the end are the counterweight: `Hold.`, `Hold — …`, `Hold on` and `Hold off` are +// still requests to stop. +test('#2287 A: a negated lift that quotes the PASS does not clear the hold', () => { + const negated = note({ + id: 6597499991, + body: [ + 'The PASS for this head has landed, but I am not asking you to lift the hold — you may merge this tip only after the second reader re-runs.', + '', + '`Verdict: PASS at ' + HEAD + '` names exactly this head.', + ].join('\n'), + at: '2026-11-03T07:20:00Z', + }) + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), negated] }) + assert.equal(r.ok, false, 'a negated lift is not an answer: ' + r.reasons.join('; ')) + assert.match(r.reasons.join('; '), /6596354014/) +}) + +test('#2287 B: a lift under a condition that quotes the PASS does not clear the hold', () => { + const conditional = note({ + id: 6597499992, + body: [ + 'You may merge this tip once the second reader re-runs.', + '', + '`Verdict: PASS at ' + HEAD + '` names exactly this head.', + ].join('\n'), + at: '2026-11-03T07:25:00Z', + }) + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), conditional] }) + assert.equal(r.ok, false, 'a conditional lift is not an answer: ' + r.reasons.join('; ')) + assert.match(r.reasons.join('; '), /6596354014/) +}) + +test('#2287 C1: "Hold cleared" on an opening line clears the hold instead of being one', () => { + const cleared = note({ + id: 6597499993, + body: [ + 'Hold cleared — you may merge this tip now.', + '', + '`Verdict: PASS at ' + HEAD + '` names exactly this head.', + ].join('\n'), + at: '2026-11-03T07:30:00Z', + }) + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), cleared] }) + assert.equal(r.ok, true, r.reasons.join('; ')) + assert.equal(r.stopRequest, null, 'the opening-line answer is the answer, not a hold of its own') +}) + +test('#2287 C2: "Hold is lifted" on an opening line clears the hold instead of being one', () => { + const lifted = note({ + id: 6597499996, + body: [ + 'Hold is lifted — you may merge this tip now.', + '', + '`Verdict: PASS at ' + HEAD + '` names exactly this head.', + ].join('\n'), + at: '2026-11-03T07:32:00Z', + }) + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), lifted] }) + assert.equal(r.ok, true, r.reasons.join('; ')) + assert.equal(r.stopRequest, null, 'the opening-line answer is the answer, not a hold of its own') +}) + +test('#2287 F: the opening-line answer without the quotation still clears and is not its own stop', () => { + const cleared = note({ id: 6597499994, body: 'Hold cleared — you may merge this tip now.', at: '2026-11-03T07:35:00Z' }) + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), cleared] }) + assert.equal(r.ok, true, r.reasons.join('; ')) + assert.equal(r.stopRequest, null, 'the lift must not be read as the stop it answers') +}) + +// The other direction of #2287: an ordinary yes must still clear a hold. Judging negation and condition +// words across the whole body refused seven of these eight on `7bd71df5`, which is the #2246 wedge the other +// way round — a person wrote yes and the guard kept saying the hold stands. The first three are their own +// cases so the red can name each shape; the rest ride in one loop. +const ordinaryYes = (body, at) => run({ + comments: [ + comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), + note({ id: 6596354014, body: 'Hold — the second reader is still unguarded.', at: '2026-11-03T05:38:25Z' }), + note({ id: 6597499998, body, at }), + ], +}) + +test('#2287 c1: "No objections — you may merge this tip now" clears the hold', () => { + const r = ordinaryYes('No objections — you may merge this tip now.', '2026-11-03T06:15:01Z') + assert.equal(r.ok, true, r.reasons.join('; ')) + assert.equal(r.stopRequest, null) +}) + +test('#2287 c2: a yes followed by "nothing else waits on it" clears the hold', () => { + const r = ordinaryYes('You may merge this tip now — nothing else waits on it.', '2026-11-03T06:15:02Z') + assert.equal(r.ok, true, r.reasons.join('; ')) + assert.equal(r.stopRequest, null) +}) + +test('#2287 c3: a yes after a colon ("Just re-read the diff: …") clears the hold', () => { + const r = ordinaryYes('Just re-read the diff: you may merge this tip now.', '2026-11-03T06:15:03Z') + assert.equal(r.ok, true, r.reasons.join('; ')) + assert.equal(r.stopRequest, null) +}) + +test('#2287 c4-c8: the other ordinary yeses clear the hold', () => { + for (const [i, body, at] of [ + [4, 'No further concerns, you may merge this tip now.', '2026-11-03T06:15:04Z'], + [5, 'You may merge this tip now; I am back after lunch if anything breaks.', '2026-11-03T06:15:05Z'], + [6, 'This needs nothing more from me — go ahead.', '2026-11-03T06:15:06Z'], + [7, 'You may merge this tip now.', '2026-11-03T06:15:07Z'], + [8, 'Once more: you may merge this tip now.', '2026-11-03T06:15:08Z'], + ]) { + const r = ordinaryYes(body, at) + assert.equal(r.ok, true, `c${i} is an ordinary yes: ${r.reasons.join('; ')}`) + assert.equal(r.stopRequest, null, `c${i} clears the hold`) + } +}) + +// P-485 on #2288: the clause scan knew `only/after/once/…` but not the rest of the ways a yes is made +// conditional — "You may merge this tip pending the second reader" cleared the hold with and without its +// quotation. These are the same shape as the case above, one word each, so a hole in the vocabulary fails +// by name. +test('#2287 k1: a yes held pending the second reader keeps the hold (no quotation)', () => { + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), note({ id: 6597499990, body: 'You may merge this tip pending the second reader.', at: '2026-11-03T06:15:10Z' })] }) + assert.equal(r.ok, false, r.reasons.join('; ')) + assert.equal(r.stopRequest?.id, 6596354014) +}) + +test('#2287 k9: a yes held pending the second reader keeps the hold (with its quotation)', () => { + const conditional = note({ + id: 6597499990, + body: ['You may merge this tip pending the second reader.', '', '`Verdict: PASS at ' + HEAD + '` names exactly this head.'].join('\n'), + at: '2026-11-03T06:15:10Z', + }) + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), conditional] }) + assert.equal(r.ok, false, r.reasons.join('; ')) + assert.equal(r.stopRequest?.id, 6596354014) +}) + +test('#2287 k2-k8: the other ways a yes is made conditional keep the hold', () => { + for (const [i, body, at] of [ + [2, 'You may merge this tip provided the second reader lands.', '2026-11-03T06:15:11Z'], + [3, 'You may merge this tip as long as the second reader lands.', '2026-11-03T06:15:12Z'], + [4, 'You may merge this tip as soon as the second reader lands.', '2026-11-03T06:15:13Z'], + [5, 'You may merge this tip subject to the second reader.', '2026-11-03T06:15:14Z'], + [6, 'You may merge this tip assuming the second reader lands.', '2026-11-03T06:15:15Z'], + [7, 'You may merge this tip in case the second reader lands.', '2026-11-03T06:15:16Z'], + [8, 'You may merge this tip depending on the second reader.', '2026-11-03T06:15:17Z'], + ]) { + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), note({ id: 6597499990, body, at })] }) + assert.equal(r.ok, false, `k${i} is conditional: ${r.reasons.join('; ')}`) + assert.equal(r.stopRequest?.id, 6596354014, `k${i} keeps the original hold`) + } +}) + +// #2288 review, 2026-11-03: a clause is judged on its own, so a condition one clause over — after a dash, +// a colon, a semicolon, a full stop or a line break — was never read, and the yes cleared the hold it +// withholds. Each body is a plain yes with the condition moved out of its clause. +test('#2287 k10-k15: a condition in the clause after the lift keeps the hold', () => { + for (const [i, body, at] of [ + [10, 'You may merge this tip — only after the second reader re-runs.', '2026-11-03T06:15:20Z'], + [11, 'You may merge this tip: pending the second reader.', '2026-11-03T06:15:21Z'], + [12, 'You may merge this tip; not until the second reader re-runs.', '2026-11-03T06:15:22Z'], + [13, 'You may merge this tip. Only after the second reader re-runs.', '2026-11-03T06:15:23Z'], + [14, 'You may merge — if the second reader agrees.', '2026-11-03T06:15:24Z'], + [15, 'You may merge this tip\nonly after the second reader re-runs.', '2026-11-03T06:15:26Z'], + ]) { + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), note({ id: 6597499990, body, at })] }) + assert.equal(r.ok, false, `k${i} withholds the answer: ${r.reasons.join('; ')}`) + assert.equal(r.stopRequest?.id, 6596354014, `k${i} keeps the original hold`) + } +}) + +// FlyBot Test on #2288, 2026-11-03: the clause-after vocabulary still missed seven ways a yes is made +// conditional, and a bare negation inside that clause ("has not re-run it") was read only at its start. +// Each is its own test so a hole in the vocabulary fails by name. +for (const [id, body, at] of [ + [16, "You may merge this tip — upon the second reader's approval.", '2026-11-03T08:20:00Z'], + [17, 'You may merge this tip — whenever the second reader agrees.', '2026-11-03T08:20:01Z'], + [18, 'You may merge this tip — should the second reader agree.', '2026-11-03T08:20:02Z'], + [19, 'You may merge this tip — contingent on the second reader.', '2026-11-03T08:20:03Z'], + [20, "You may merge this tip — following the second reader's re-run.", '2026-11-03T08:20:04Z'], + [21, 'You may merge this tip — on condition that the second reader agrees.', '2026-11-03T08:20:05Z'], + [22, 'You may merge this tip — the second reader has not re-run it.', '2026-11-03T08:20:06Z'], +]) { + test(`#2287 k${id}: a yes conditional in the clause after the lift keeps the hold`, () => { + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), note({ id: 6597499990, body, at })] }) + assert.equal(r.ok, false, `k${id} withholds the answer: ${r.reasons.join('; ')}`) + assert.equal(r.stopRequest?.id, 6596354014, `k${id} keeps the original hold`) + }) +} + +test('#2287: a conditional lift with no hold wording of its own keeps the hold', () => { + const conditional = note({ + id: 6597499999, + body: ['You may merge this tip only after the second reader lands.', '', '`Verdict: PASS at ' + HEAD + '` names exactly this head.'].join('\n'), + at: '2026-11-03T07:31:00Z', + }) + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), conditional] }) + assert.equal(r.ok, false, r.reasons.join('; ')) + assert.equal(r.stopRequest?.id, 6596354014) +}) + +// The two shapes that pull the two halves of the fix together: an opening-line answer that is *also* +// conditional. Read as a hold it becomes its own stop, so the refusal would name the wrong id; cleared as an +// unconditional lift it drops the hold it withholds. Only both halves together keep the original stop standing. +test('#2287 C3: an opening-line "Hold cleared" under a condition keeps the hold it answers', () => { + for (const [body, at] of [ + ['Hold cleared — you may merge this tip only after the second reader lands.', '2026-11-03T07:33:00Z'], + ['Hold cleared. You may merge this tip once the second reader lands.', '2026-11-03T07:34:00Z'], + ]) { + const conditional = note({ + id: 6597499997, + body: [body, '', '`Verdict: PASS at ' + HEAD + '` names exactly this head.'].join('\n'), + at, + }) + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6596354014, at: '2026-11-03T05:38:25Z' }), conditional] }) + assert.equal(r.ok, false, body + ' withholds the answer: ' + r.reasons.join('; ')) + assert.equal(r.stopRequest?.id, 6596354014, 'the stop that stands is the original hold, not this answer') + } +}) + +test('#2287: a real hold is still a stop, on an opening line and with the word it begins with', () => { + for (const [body, at] of [ + ['Hold.', '2026-11-03T07:36:00Z'], + ['Hold — the second reader is still unguarded.', '2026-11-03T07:37:00Z'], + ['Hold on — the second reader is still unguarded.', '2026-11-03T07:38:00Z'], + ['Hold off — the second reader is still unguarded.', '2026-11-03T07:39:00Z'], + ]) { + const r = run({ comments: [comment(report(), '2026-11-03T05:00:00Z', { id: 6596712952 }), note({ id: 6597499995, body, at })] }) + assert.equal(r.ok, false, body + ' is still a request to stop: ' + r.reasons.join('; ')) + assert.match(r.reasons.join('; '), /asks not to merge/) + assert.match(r.reasons.join('; '), /6597499995/) + } +}) diff --git a/.github/scripts/enqueue-rules.mjs b/.github/scripts/enqueue-rules.mjs index 79890a91..59504790 100644 --- a/.github/scripts/enqueue-rules.mjs +++ b/.github/scripts/enqueue-rules.mjs @@ -165,8 +165,11 @@ const STOP_REQUEST = new RegExp( ) // `hold` as its own opening line — "Hold." / "Hold — the second reader is still unguarded" — is how the // request is usually shortened. Anywhere else the word is the label this repository already has, and -// reading the label's name as a request would freeze every pull request that discusses one. -const HOLD_LINE = /^[\s>*+-]*(?:hold|hold on|hold off)\b/im +// reading the label's name as a request would freeze every pull request that discusses one. A line that +// says the hold is over is the answer, not the request, so the word is not followed by its own clearing: +// "Hold cleared — you may merge this tip now" was read as a hold while the same sentence mid-line cleared +// (#2287 C1), and without its quotation it made itself the stop, naming its own id (#2287 F). +const HOLD_LINE = /^[\s>*+-]*(?:hold|hold on|hold off)\b(?!\s*(?:(?:is|was|has been|had been|been|gets|got)\s+)?(?:clear|lift|answer|resol|gone|done)\w*)/im // The answer: read only after a stop in the same window, so this vocabulary cannot unblock anything alone. const LIFT_REQUEST = new RegExp( [ @@ -182,13 +185,62 @@ const withoutQuotes = (text) => String(text ?? '') .replace(/"[^"\n]*"/g, ' ') .replace(/“[^”\n]*”/g, ' ') +// A lift clears a stop only when it is an unconditional yes. `LIFT_REQUEST` alone is not one: "I am not +// asking you to lift the hold — you may merge this tip only after the second reader re-runs" and "You may +// merge this tip once the second reader re-runs" both hit it and both *withhold* the answer (FlyBot Test on +// #2282, 2026-11-03: the tip cleared a hold the base refused). Negation and condition words are not in +// `STOP_REQUEST`, and `HOLD_LINE` reads an opening line only, so they are read here. +// +// The scan is per clause, not per body: a yes reads "No objections — you may merge this tip now" or "you may +// merge this tip now; I am back after lunch", and a whole-body scan refused seven of eight ordinary answers +// on `7bd71df5` (FlyBot Test, #2287). A clause that carries a lift phrase must be free of them, so the +// withholding shapes still refuse: the condition sits in the same clause as the lift ("you may merge this +// tip only after …"), and a negated lift takes the lift phrase with it ("not asking you to lift the hold"). +// Every clause that carries a lift is read, not just the first, because "Hold cleared — you may merge this +// tip only after …" carries one on each side of the dash and is still an answer withheld. +const CONDITIONAL = new RegExp( + [ + String.raw`\b(?:not|never|cannot)\b`, + String.raw`n['’]t\b`, + String.raw`\b(?:only|after|once|until|unless|if|when|before|pending)\b`, + String.raw`\b(?:provided|providing|assuming)\b`, + String.raw`\b(?:as (?:long|soon) as|subject to|in case|depending on)\b`, + String.raw`(?:不要|暂不|先别|尚未|还没|之后|以后|若|如果|只有|还要)`, + ].join('|'), + 'i', +) +// `.` `;` `:` `—` `!` `?` and a line break end a clause; the dash is one here so a sentence's two halves +// are judged separately, which is what lets "No objections — you may merge" through. +const CLAUSE = /[.;:!?—–\n]/ +// A condition can also sit in the clause *after* the lift: "You may merge this tip — only after the second +// reader re-runs" cleared the hold that the same words inside the lift's own clause refused, and a dash +// after the yes is ordinary prose (#2288 review, 2026-11-03). A clause that follows a lift withholds the +// answer when it carries a phrase that never sits in a plain yes, or when it opens with a condition word. +// Ordinary trailing talk still clears: "you may merge this tip now; I am back after lunch if anything +// breaks" opens with "I", which is why the broad vocabulary above is not used here. +// FlyBot Test on #2288 (2026-11-03) found seven more of those phrases clearing the hold — "upon the second +// reader's approval", "whenever …", "should …", "contingent on …", "following …", "on condition that …" — +// plus a bare negation anywhere in the clause ("the second reader has not re-run it"), which the +// opening-word rule read only at its start. `no objections` and `nothing else waits on it` stay clear: a +// word boundary keeps `not` inside them from matching. +const WITHHELD = /\b(?:only (?:after|once|when)|not until|not unless|pending|subject to|provided|as (?:long|soon) as|assuming|in case|depending on|upon|whenever|should|contingent (?:on|upon)|dependent (?:on|upon)|prior to|following|in the event|on condition that|not|never)\b|n['’]t\b/i +const WITHHELD_LEADING = /^[\s>*+\-–—(\[]*(?:only|after|once|until|unless|if|when|before|pending|not|never|cannot|n['’]t|provided|providing|assuming)\b/i +const unconditionalLift = (text) => { + if (!LIFT_REQUEST.test(text) || STOP_REQUEST.test(text) || HOLD_LINE.test(text)) return false + const clauses = text.split(CLAUSE) + if (clauses.some((clause) => LIFT_REQUEST.test(clause) && CONDITIONAL.test(clause))) return false + let afterLift = false + for (const clause of clauses) { + if (afterLift && (WITHHELD.test(clause) || WITHHELD_LEADING.test(clause))) return false + if (LIFT_REQUEST.test(clause)) afterLift = true + } + return true +} + // A body that parses as a conclusion but asks only to lift a stop — no ask to stop of its own. This is the // lift that quotes the PASS it answers (#2246 on 2026-11-03); "record only — do not merge" in a PASS is the // marker `recordOnlyAt` reads, so it is not one of these. -const responseOnly = (body) => { - const text = withoutQuotes(body) - return LIFT_REQUEST.test(text) && !STOP_REQUEST.test(text) && !HOLD_LINE.test(text) -} +const responseOnly = (body) => unconditionalLift(withoutQuotes(body)) // The stop request still standing, or null. `since` is the timestamp of the conclusion the decision used, in // ms; null reads the whole list (no anchor, or one that cannot be read — a guard that cannot find the PASS it @@ -217,7 +269,7 @@ export function stopRequestAt(reports, { author, since } = {}) { stop = { id: r.id, login: r.login, at: r.at ?? null, phrase: phrase.trim() } continue } - if (stop && LIFT_REQUEST.test(text)) stop = null + if (stop && unconditionalLift(text)) stop = null } return stop }