diff --git a/scripts/pr-gate.d.mts b/scripts/pr-gate.d.mts index 43afa1b95..836cc1dd1 100644 --- a/scripts/pr-gate.d.mts +++ b/scripts/pr-gate.d.mts @@ -81,6 +81,7 @@ export declare function renderComment(input: { downgrades?: string[]; policyMisses?: RedFlag[]; policyExempt?: string | null; + labelsCleared?: boolean; state?: { hash: string; lane: string }; }): string; diff --git a/scripts/pr-gate.mjs b/scripts/pr-gate.mjs index ac350c1fd..39023b862 100644 --- a/scripts/pr-gate.mjs +++ b/scripts/pr-gate.mjs @@ -23,10 +23,11 @@ * time on their diff. Its checks are mechanical FLOORS — cheap filters against * zero-effort submissions. * - * IT IS NOT an authorization boundary. Nothing here decides what merges. - * close-lane exits red, which is a strong signal, not a hard block. Every - * mechanical floor below (a screenshot embed, 40 words of prose, a title - * shape) can be satisfied by a determined author who wants to satisfy it — + * IT IS NOT an authorization boundary. Nothing here decides what merges, and + * nothing here closes, reopens or blocks anything. close-lane exits red, which + * is a strong signal, not a hard block. Every mechanical floor below (a + * screenshot embed, a short paragraph of prose, a title shape) can be + * satisfied by a determined author who wants to satisfy it — * that is expected and it is fine, because clearing the floor buys a human * read, not a merge. The human reviewer is the decision-maker. * @@ -41,10 +42,13 @@ * marker is ever adopted for the sticky update. A contributor pre-posting * the marker gets a fresh bot comment instead of a hijacked one. * - EVERY string that is not a literal in THIS file is sanitized before it - * reaches Markdown (no HTML comments, no live @mentions, no block markers, - * no newlines, length- and count-capped). That includes the mechanical - * red-flag details: two of them interpolate PR filenames, and a filename may - * legally contain a newline, so they are attacker-controlled too. + * reaches Markdown (no HTML comments, no renderable HTML, no live @mentions, + * no live image embeds or links, no block markers, no newlines, length- and + * count-capped). Markdown counts as much as HTML here: `![APPROVED](…)` and + * `[click to approve](…)` forge a green verdict with no angle brackets at + * all. That includes the mechanical red-flag details: two of them + * interpolate PR filenames, and a filename may legally contain a newline, so + * they are attacker-controlled too. * - parseState only reads the state block the bot itself wrote (line 2 of a * marker-leading comment). A block appearing anywhere else in the body is * somebody else's text and is ignored, so hostile content cannot forge a @@ -201,6 +205,20 @@ export const MAX_ITEMS = 8; /** & first, or the escaping escapes its own output. */ const escapeHtml = (s) => s.replace(/&/g, '&').replace(//g, '>'); +/** + * Markdown forges a widget with no angle brackets at all, so escaping HTML is + * only half the job. In a CLOSE-LANE comment, + * `![MERGE LANE — APPROVED](https://evil.example/green.png)` renders a live + * image that looks like a green verdict, and `[click to approve](…)` renders a + * live link to anywhere. Both survive escapeHtml untouched. + * + * Backslash-escaping `[` and `]` is the whole fix: Markdown renders `\[` as a + * literal `[`, so benign text ("check line \[40\]") looks identical while + * inline links, image embeds AND reference links (`[text][ref]`, which need the + * same two characters) all render as inert text. + */ +const escapeMarkdownLinks = (s) => s.replace(/[[\]]/g, '\\$&'); + export function sanitizeModelText(value, max = MAX_STRING) { let t = typeof value === 'string' ? value : String(value ?? ''); t = t @@ -214,10 +232,12 @@ export function sanitizeModelText(value, max = MAX_STRING) { .replace(/@(?=[A-Za-z0-9])/g, '@\u200b') // zero-width break: the mention is inert .trim(); // Truncating AFTER escaping can cut an entity in half (`&l`), which renders - // as those literal characters. It can never re-create a `<`, so it cannot - // re-open a tag. - t = escapeHtml(t); - if (t.length > max) t = `${t.slice(0, max)}…[truncated]`; + // as those literal characters. It can never re-create a `<` or an unescaped + // `[`, so it cannot re-open a tag or a link. A cut landing between a + // backslash and its bracket leaves a dangling `\`, which is only cosmetic — + // drop it so the truncation marker reads cleanly. + t = escapeMarkdownLinks(escapeHtml(t)); + if (t.length > max) t = `${t.slice(0, max).replace(/\\$/, '')}…[truncated]`; return t; } @@ -323,18 +343,85 @@ export function hasScreenshot(body) { return SCREENSHOT_RES.some((match) => match(text)); } -export const INTENT_MIN_WORDS = 40; +/** + * A FLOOR against an empty or boilerplate-only description — NOT a quality bar + * and NOT a length requirement CONTRIBUTING.md makes (it documents no word + * count at all; it asks for "a paragraph you wrote yourself", rough grammar + * preferred). 20 words is roughly one honest sentence about what went wrong, + * which is the least that can distinguish a real report from "fixes bug" or an + * untouched template. + * + * It was 40, and 40 red-Xed real contributors: a specific first-person bug + * report (34 words), a short non-native-English paragraph (38), and a body + * that is mostly a stack trace plus a real explanation (28) all failed. Every + * one of those is pinned as PASSING in test/pr-gate-workflow.test.ts now. Do + * not raise this without re-measuring against those fixtures — a check that is + * red on every terse-but-genuine contribution is a check somebody disables + * inside a week, and it costs real people on the way there. + */ +export const INTENT_MIN_WORDS = 20; -// Everything a contributor can paste WITHOUT writing a word themselves: code, -// quoted logs, checklists, headings, the template's HTML hints, and the -// template's own bold prompts (a whole line of `**...**` is a heading in -// disguise). What survives is the author's own prose. +// A list marker at the start of a line. Read twice below: to know we are inside +// a list (where an indented line is the author continuing their own sentence, +// not pasted output) and to strip the marker while KEEPING the words after it. +const LIST_MARKER_RE = /^[ \t]*([-*+]|\d+[.)])[ \t]+/; + +/** + * Indented code blocks (CommonMark 4.4) are pasted output, not prose — the + * fenced form is already gone via stripCodeFences, and this is the same content + * in the other spelling. + * + * Two guards keep it from eating the author's own words, which is the error + * that matters: an indented line only opens a block after a BLANK line (a code + * block cannot interrupt a paragraph), and never inside a list, where + * indentation means "continuation of the item I am writing" and stripping it + * would re-create the false positive this whole area exists to avoid. + */ +function stripIndentedCode(text) { + const out = []; + let inList = false; + let inCode = false; + let prevBlank = true; + for (const line of text.split('\n')) { + const blank = line.trim() === ''; + const indented = /^(?: {4}|\t)/.test(line); + if (LIST_MARKER_RE.test(line)) inList = true; + else if (!blank && !indented) inList = false; + if (inCode) { + if (blank || indented) continue; // a blank line inside the block is still the block + inCode = false; + } else if (!inList && indented && prevBlank) { + inCode = true; + continue; + } + out.push(line); + prevBlank = blank; + } + return out.join('\n'); +} + +/** + * Counts the words the author actually wrote. + * + * REMOVED — what a contributor can paste without writing anything: fenced and + * indented code, HTML comments (the PR template's hints), headings, raw HTML, + * bare URLs, inline code, link/image syntax, and the template's own bold + * prompts (a whole line of `**...**` is a heading in disguise). That last one + * is what keeps an untouched .github/pull_request_template.md at zero, pinned + * against the real file on disk. + * + * KEPT — the words inside list items and blockquotes. Only the MARKER goes. + * Plenty of people write their own story as four bullets or quote-indent it, + * and deleting those lines scored such a body 0 and closed it: the single worst + * false positive this gate had. + */ export function intentWordCount(body) { - const prose = visibleText(body) // fences + HTML comments (the PR template's hints) + const prose = stripIndentedCode(visibleText(body)) // + fences and HTML comments + .replace(/^[ \t]{0,3}(?:>[ \t]?)+/gm, ' ') // blockquote MARKER only — the words are the author's + .replace(new RegExp(LIST_MARKER_RE.source, 'gm'), ' ') // list MARKER only — ditto + // After the markers, so `- **What changed**` still reads as a template prompt. .replace(/^[ \t]{0,3}#{1,6}[ \t].*$/gm, ' ') // headings .replace(/^[ \t]*\*\*[^\n]*\*\*[ \t]*$/gm, ' ') // bold-only line = template prompt - .replace(/^[ \t]{0,3}>.*$/gm, ' ') // blockquotes - .replace(/^[ \t]*([-*+]|\d+[.)])[ \t].*$/gm, ' ') // list items .replace(/!?\[[^\]]*\]\([^)]*\)/g, ' ') // links + image embeds .replace(/<[^>]+>/g, ' ') // raw HTML tags .replace(/https?:\/\/\S+/g, ' ') // bare URLs @@ -352,7 +439,7 @@ export const hasIntentParagraph = (body) => intentWordCount(body) >= INTENT_MIN_ export const POLICY_FLAG_IDS = ['missing_intent', 'missing_screenshot']; const POLICY_DETAILS = { - missing_intent: `no human-written intent paragraph in the PR description (under ${INTENT_MIN_WORDS} words of prose once code, quotes, lists and the template boilerplate are removed) — required by CONTRIBUTING.md (#3745)`, + missing_intent: `no human-written intent paragraph in the PR description (under ${INTENT_MIN_WORDS} words of prose once code, headings, links and the template's own boilerplate are removed — bullets and quoted lines DO count) — required by CONTRIBUTING.md (#3745)`, missing_screenshot: 'no screenshot of gbrain in use in the PR description — required by CONTRIBUTING.md (#3745)', }; @@ -806,7 +893,16 @@ const LANE_HEADINGS = { const LANE_MARKS = { 'merge-lane': '✅', 'close-lane': '❌', 'needs-maintainer': '⚠️' }; const POLICY_HEADING = 'CLOSE LANE — the PR description is missing something required'; -/** Leads the comment on a #3745 miss: what is missing, how to fix it, how to reopen. */ +/** + * Leads the comment on a #3745 miss: what is missing, and what actually happens + * next. + * + * Say only what this gate DOES. It posts this comment, sets one `gate:*` label + * and exits red — it never closes a PR, so telling an author to "reopen" an + * open PR is both wrong and alarming. Editing the description really does + * re-run the check: `edited` is in the workflow's trigger list, and the rerun + * rewrites this same sticky comment. + */ function policyBlock(policyMisses) { const ids = POLICY_FLAG_IDS.filter((id) => policyMisses.some((f) => f.id === id)); return [ @@ -814,7 +910,7 @@ function policyBlock(policyMisses) { '', ...ids.map((id) => `- ${POLICY_ASKS[id]}`), '', - `Edit the description to add that, then reopen. This is not a judgment on the code — the policy is in [CONTRIBUTING.md](${CONTRIBUTING_URL}).`, + `Edit the description and this check re-runs on its own, updating this comment. Your PR stays open — nothing here closes it, and a maintainer makes the actual call. This is not a judgment on the code. The policy is in [CONTRIBUTING.md](${CONTRIBUTING_URL}).`, ]; } @@ -827,6 +923,7 @@ export function renderComment({ downgrades = [], policyMisses = [], policyExempt = null, + labelsCleared = true, state, }) { const lines = [MARKER]; @@ -834,8 +931,15 @@ export function renderComment({ lines.push(''); if (neutralReason) { lines.push('## PR Gate — NEUTRAL (skipped)', '', `**Reason:** ${sanitizeModelText(neutralReason)}`, ''); + // Don't claim the labels were cleared when the clearing call failed — a + // NEUTRAL run keeps going through a label blip (see runGate), so this + // sentence is the one place that could quietly become untrue. lines.push( - `The **usefulness verdict did not run**, so there is no lane and any previous \`gate:*\` label was cleared. This is a loud skip, not a pass. The mechanical checks below need no model: they ran, and the CONTRIBUTING.md intent-paragraph + screenshot requirement ${ + `The **usefulness verdict did not run**, so there is no lane and ${ + labelsCleared + ? 'any previous `gate:*` label was cleared' + : 'the `gate:*` labels could NOT be updated (that API call failed) — any label still showing is stale' + }. This is a loud skip, not a pass. The mechanical checks below need no model: they ran, and the CONTRIBUTING.md intent-paragraph + screenshot requirement ${ policyExempt ? 'was skipped for this author' : 'passed' } — a miss there is close-lane whether or not the model is reachable.`, '', @@ -910,13 +1014,26 @@ export async function runGate(dir, env = process.env, fetchImpl = fetch) { const neutral = async (reason) => { console.log(`::warning::PR gate NEUTRAL-skip: ${reason}`); - await setLaneLabel(gh, repo, prNumber, null); // no stale verdict survives a skip + // A NEUTRAL run must never be a red X — that is the promise in the + // workflow header ("never a red X for a missing secret"), and a missing + // key plus one failed label DELETE was breaking it: the throw escaped to + // the crash handler, exit 2, and the explanatory comment never posted. A + // NEUTRAL has no verdict to record, so label reconciliation is cosmetic + // here. Log it, say so in the comment, exit 0. (In the VERDICT path below + // a label failure stays fatal on purpose — see the ordering note there.) + let labelsCleared = true; + try { + await setLaneLabel(gh, repo, prNumber, null); // no stale verdict survives a skip + } catch (err) { + labelsCleared = false; + console.log(`::warning::PR gate could not clear gate:* labels on a NEUTRAL run: ${String(err?.message ?? err)}`); + } await upsertStickyComment( gh, repo, prNumber, existing, - renderComment({ titleCheck, flags, policyExempt, neutralReason: reason }), + renderComment({ titleCheck, flags, policyExempt, labelsCleared, neutralReason: reason }), ); return 0; }; @@ -938,7 +1055,7 @@ export async function runGate(dir, env = process.env, fetchImpl = fetch) { lane: 'close-lane', confidence: 1, reasons: [ - 'CONTRIBUTING.md requires a human-written intent paragraph and a screenshot of gbrain in use on every PR; this description is missing at least one of them. Reopen once added.', + 'CONTRIBUTING.md requires a human-written intent paragraph and a screenshot of gbrain in use on every PR; this description is missing at least one of them.', ], reviewer_checklist: [], }; diff --git a/test/pr-gate-workflow.test.ts b/test/pr-gate-workflow.test.ts index a08bf6c8c..79e881206 100644 --- a/test/pr-gate-workflow.test.ts +++ b/test/pr-gate-workflow.test.ts @@ -6,8 +6,13 @@ * actions, trigger shape, 120KB diff cap, persist-credentials:false). * - scripts/pr-gate.mjs rubric carries the load-bearing phrases. * - Unit coverage for the exported title rule, red-flag detector, model-output - * sanitizer, and deterministic lane downgrades (importing the script must - * not execute main — side-effect guard). + * sanitizer (HTML widgets AND Markdown image/link embeds), and deterministic + * lane downgrades (importing the script must not execute main — side-effect + * guard). + * - The false-positive floor: four verbatim real-human descriptions the gate + * used to red-X (bullet-point prose, non-native English, a terse bug report, + * a body that is mostly a stack trace) are pinned as PASSING forever, with + * the zero-effort bodies that must still fail beside them. * - Mocked end-to-end runs of runGate() against a stubbed fetch: close-lane * exit code, marker-hijack, sanitization, truncation, refusal routing, * NEUTRAL label clearing, label swap, and the input-hash spend guard. @@ -750,7 +755,7 @@ describe('intent paragraph detector (#3745, mechanical)', () => { expect(hasIntentParagraph(`${PR_TEMPLATE}\nrenames the flag and updates the docs`)).toBe(false); }); - test('40+ words of the author own prose counts', () => { + test('the author own prose counts, from both sides of the floor', () => { expect(intentWordCount(HUMAN_INTENT)).toBeGreaterThanOrEqual(INTENT_MIN_WORDS); expect(hasIntentParagraph(HUMAN_INTENT)).toBe(true); expect(hasIntentParagraph(COMPLIANT_BODY)).toBe(true); @@ -759,16 +764,42 @@ describe('intent paragraph detector (#3745, mechanical)', () => { expect(hasIntentParagraph(padding(INTENT_MIN_WORDS - 1).join(' '))).toBe(false); }); - test('pasted code, logs, checklists and headings are not prose', () => { + // The floor is a floor against an EMPTY description, not a quality bar, so it + // stays low on purpose. What it must still reject is the zero-effort forms. + test('the floor is low but not zero — boilerplate-only bodies still miss it', () => { + expect(hasIntentParagraph('fixes bug')).toBe(false); + expect(hasIntentParagraph('lorem ipsum dolor sit amet consectetur adipiscing elit sed do')).toBe(false); + expect(intentWordCount(PR_TEMPLATE)).toBe(0); + expect(INTENT_MIN_WORDS).toBeLessThanOrEqual(20); // raising it is what red-Xed real contributors + }); + + test('pasted code, headings and link walls are not prose', () => { const many = padding(80).join(' '); expect(hasIntentParagraph('```\n' + many + '\n```')).toBe(false); - expect(hasIntentParagraph(padding(80).map((w) => `> ${w}`).join('\n'))).toBe(false); - expect(hasIntentParagraph(padding(80).map((w) => `- ${w}`).join('\n'))).toBe(false); - expect(hasIntentParagraph(padding(80).map((w, i) => `${i + 1}. ${w}`).join('\n'))).toBe(false); expect(hasIntentParagraph(`## ${many}`)).toBe(false); expect(hasIntentParagraph(`**${many}**`)).toBe(false); // A wall of links/screenshots is not a paragraph either. expect(hasIntentParagraph(padding(80).map((w) => `![${w}](https://example.com/${w}.png)`).join(' '))).toBe(false); + // Indented code is the other spelling of a fence: still pasted output. + expect(hasIntentParagraph(`log:\n\n${padding(80).map((w) => ` ${w}`).join('\n')}`)).toBe(false); + }); + + // THE false positive this detector had: deleting whole list/quote LINES + // scored an author's own four-bullet story at 0 and closed their PR. Only + // the MARKER is boilerplate; the words after it are theirs. + test('prose written as bullets or a blockquote is still prose', () => { + expect(hasIntentParagraph(padding(30).map((w) => `- ${w}`).join('\n'))).toBe(true); + expect(hasIntentParagraph(padding(30).map((w) => `* ${w}`).join('\n'))).toBe(true); + expect(hasIntentParagraph(padding(30).map((w, i) => `${i + 1}. ${w}`).join('\n'))).toBe(true); + expect(hasIntentParagraph(padding(30).map((w) => `> ${w}`).join('\n'))).toBe(true); + expect(hasIntentParagraph(padding(30).map((w) => `>> ${w}`).join('\n'))).toBe(true); + // The marker itself contributes nothing — 19 bulleted words is still 19. + expect(intentWordCount(padding(19).map((w) => `- ${w}`).join('\n'))).toBe(19); + // An indented line under a bullet is the author continuing their sentence, + // NOT an indented code block. Stripping it would re-create the bug. + expect(intentWordCount('- one two three\n four five six')).toBe(6); + // A bulleted template prompt is still a template prompt, though. + expect(intentWordCount('- **What changed**\n- **How it was tested**')).toBe(0); }); test('non-English prose counts — the policy asks for rough words, not English', () => { @@ -786,6 +817,80 @@ describe('intent paragraph detector (#3745, mechanical)', () => { }); }); +/** + * THE regression that matters most. Four descriptions in the shape real people + * actually write, every one of which the gate red-Xed on a 40-word floor that + * also deleted list and quote lines before counting: + * + * body before → after + * own prose written as four bullets 0 → 55 + * short non-native-English paragraph 38 → 38 + * specific first-person bug report 34 → 34 + * mostly a stack trace + a real reason 28 → 27 + * + * These are FIXTURES, not examples: keep them verbatim. A change to the floor, + * the tokenizer or the strip list that puts any of them back in close-lane is + * the gate rejecting a genuine contributor, which costs more than every forgery + * risk the earlier rounds chased. If one of these ever fails, the fix is the + * detector, not the fixture. + */ +describe('real-human descriptions must never land in close-lane (#3745 false positives)', () => { + const HUMAN_BODIES: Record = { + 'own prose written as a list': [ + '- I hit this every single morning when my cron fires at 6am', + '- the sync dies and I only notice hours later when my agent has no context', + '- took me two days to trace it to the lock file not being released', + '- this patch is what I have been running locally since Tuesday and it holds', + ].join('\n'), + + 'short non-native English': [ + 'Sorry my english not good. I use gbrain for my notes in vietnamese and the names', + 'always break when i search. This fix make the tokenizer read my language correct.', + 'I test on my own brain 3000 notes.', + ].join(' '), + + 'specific first-person bug report': [ + 'My nightly cycle silently stopped extracting atoms three weeks ago and I only found', + 'out when a query came back empty. The cap was being applied to a local model that', + 'has no price.', + ].join(' '), + + 'mostly a stack trace plus a real explanation': [ + 'This crashes every time I run sync on a fresh clone:', + '', + '```', + 'Error: ENOENT', + ' at foo', + '```', + '', + 'I spent an afternoon on it. The path join assumes posix separators and I am on Windows.', + ].join('\n'), + }; + + for (const [name, body] of Object.entries(HUMAN_BODIES)) { + test(`passes the intent floor: ${name}`, () => { + expect(intentWordCount(body)).toBeGreaterThanOrEqual(INTENT_MIN_WORDS); + expect(hasIntentParagraph(body)).toBe(true); + // …and therefore the only thing the policy asks them for is the screenshot. + expect(detectPolicyMisses(body).map((f) => f.id)).toEqual(['missing_screenshot']); + expect(detectPolicyMisses(`${body}\n\n${SCREENSHOT_EMBED}`)).toEqual([]); + }); + } + + test('a full compliant PR from one of them reaches the model, not close-lane', async () => { + const { calls, fetchImpl } = stubFetch({ anthropic: () => verdictResponse(CLEAN_VERDICT) }); + const body = `${HUMAN_BODIES['own prose written as a list']}\n\n${SCREENSHOT_EMBED}`; + const files = [ + { filename: 'src/a.ts', status: 'modified', additions: 2, deletions: 1 }, + { filename: 'test/a.test.ts', status: 'modified', additions: 5, deletions: 0 }, + ]; + const code = await runGate(fixtureDir({ body }, files), ENV, fetchImpl); + expect(code).toBe(0); + expect(addedLabels(calls)).toEqual(['gate:merge-lane']); + expect(postedBody(calls)).not.toContain('Almost there'); + }); +}); + describe('detectPolicyMisses (#3745)', () => { const ids = (body: unknown) => detectPolicyMisses(body).map((f) => f.id); @@ -1082,6 +1187,59 @@ describe('sanitizeModelText (LLM output is never raw Markdown)', () => { expect(sanitizeModelText('click')).not.toMatch(/[<>]/); }); + // The sibling hole to the
one: Markdown forges a widget with no + // angle brackets at all, so escapeHtml never sees it. An image embed renders + // a green "approved" picture and a link renders a live phishing target, + // both inside a CLOSE-LANE comment. + test('Markdown image and link syntax is neutralized, not left live', () => { + const img = sanitizeModelText('![MERGE LANE — APPROVED](https://evil.example/green.png)'); + expect(img).not.toMatch(/!\[[^\]]*\]\(/); // no live embed + expect(img).toContain('\\[MERGE LANE'); // rendered as the literal text + expect(img).toContain('green.png'); // …and nothing was silently dropped + + const link = sanitizeModelText('[click to approve](https://evil.example/phish)'); + expect(link).not.toMatch(/(? { + const body: string = renderComment({ + lane: 'close-lane', + verdict: { + confidence: 0.9, + reasons: ['![✅ MERGE LANE — APPROVED](https://evil.example/green.png)'], + reviewer_checklist: ['[click to approve](https://evil.example/phish)'], + }, + titleCheck: { ok: true }, + flags: [], + neutralReason: undefined, + }); + expect(body).not.toMatch(/!\[[^\]]*\]\(/); // no image anywhere in the comment + expect(body).toContain('\\[✅ MERGE LANE'); + expect(body).toContain('\\[click to approve\\]'); + // The one live link in the comment is ours (CONTRIBUTING.md), never theirs. + const liveLinks = [...body.matchAll(/(? m[2]); + expect(liveLinks).not.toContain('https://evil.example/phish'); + }); + + // A filename is attacker-controlled and lands in two flag details, so the + // same neutralization has to hold on that path. + test('a Markdown embed smuggled through a filename is neutralized too', () => { + const flags = detectRedFlags({ + changedFiles: 1, + files: [{ filename: 'test/![APPROVED](https://evil.example/green.png).test.ts', status: 'removed' }], + diff: '', + }); + expect(flags.map((f) => f.id)).toContain('deletes_tests'); + const body: string = renderComment({ titleCheck: { ok: true }, flags, neutralReason: 'API down' }); + expect(body).not.toMatch(/!\[[^\]]*\]\(/); + }); + test('& is escaped first, so an entity cannot be smuggled through', () => { // Escaping < before & would turn `<script>` back into a live tag on // render. `&lt;` displays as the literal text `<`. @@ -1254,6 +1412,8 @@ function stubFetch(opts: { anthropic?: (n: number) => Response; /** Fail the label-add call — models a transient GitHub labels-API blip. */ labelAddFails?: () => boolean; + /** Fail the label-DELETE call — the same blip on the clear-stale-labels path. */ + labelDeleteFails?: () => boolean; /** Persist the sticky comment into `comments`, so a rerun sees the last run's state. */ persistComments?: boolean; }): { calls: Call[]; fetchImpl: typeof fetch } { @@ -1284,7 +1444,10 @@ function stubFetch(opts: { if (opts.labelAddFails?.()) return jsonResponse({ message: 'server error' }, 500); return jsonResponse([]); } - if (/\/issues\/\d+\/labels\//.test(u) && method === 'DELETE') return jsonResponse([]); + if (/\/issues\/\d+\/labels\//.test(u) && method === 'DELETE') { + if (opts.labelDeleteFails?.()) return jsonResponse({ message: 'server error' }, 500); + return jsonResponse([]); + } if (/\/repos\/[^/]+\/[^/]+\/labels$/.test(u) && method === 'POST') return jsonResponse({}, 201); return jsonResponse({ message: `unrouted ${method} ${u}` }, 404); }) as unknown as typeof fetch; @@ -1459,6 +1622,39 @@ describe('runGate end-to-end (mocked fetch)', () => { ]); }); + // "never a red X for a missing secret" (workflow header) was false the moment + // the labels API also blipped: setLaneLabel threw, the throw escaped to the + // crash handler, and the run exited 2 with no comment at all — a red X and no + // explanation, on a PR that did nothing wrong. + test('a NEUTRAL run survives a label-API failure — comment posts, exit 0', async () => { + const { calls, fetchImpl } = stubFetch({ labelDeleteFails: () => true }); + const code = await runGate(fixtureDir(), { ...ENV, ANTHROPIC_API_KEY: undefined }, fetchImpl); + expect(code).toBe(0); // NOT 2 + const body: string = postedBody(calls); + expect(body).toContain('NEUTRAL'); + // …and the comment does not claim a clearing that did not happen. + expect(body).not.toContain('any previous `gate:*` label was cleared'); + expect(body).toContain('could NOT be updated'); + }); + + test('a NEUTRAL run that clears labels cleanly still says so', async () => { + const { calls, fetchImpl } = stubFetch({}); + expect(await runGate(fixtureDir(), { ...ENV, ANTHROPIC_API_KEY: undefined }, fetchImpl)).toBe(0); + expect(postedBody(calls)).toContain('any previous `gate:*` label was cleared'); + }); + + // The VERDICT path keeps the opposite behaviour on purpose: a label failure + // there must throw BEFORE the sticky comment persists the spend-guard state, + // or the rerun short-circuits and the label stays wrong forever. + test('a label failure on the verdict path is still fatal', async () => { + const { calls, fetchImpl } = stubFetch({ + anthropic: () => verdictResponse(CLEAN_VERDICT), + labelAddFails: () => true, + }); + await expect(runGate(fixtureDir(), ENV, fetchImpl)).rejects.toThrow(/label add failed/); + expect(calls.some((c) => c.method === 'POST' && /\/issues\/\d+\/comments$/.test(c.url))).toBe(false); + }); + test('an unreachable API is a NEUTRAL skip (exit 0), not a verdict', async () => { const { calls, fetchImpl } = stubFetch({ anthropic: () => jsonResponse({ error: 'boom' }, 500) }); const code = await runGate(fixtureDir(), ENV, fetchImpl); @@ -1593,7 +1789,15 @@ describe('runGate — CONTRIBUTING.md #3745 policy (mocked fetch)', () => { expect(body).toContain('Almost there'); expect(body).toContain('A screenshot of gbrain in use'); expect(body).not.toContain('A paragraph you wrote yourself'); // that half is fine - expect(body).toContain('then reopen'); + // The comment may only promise what the gate DOES. It has no close call in + // it (grep the script), so telling an author to reopen an open PR is a lie + // that reads as a threat to a first-time contributor. + expect(body).not.toMatch(/reopen/i); + expect(SCRIPT).not.toMatch(/state:\s*['"]closed['"]/); // …and still no close call + expect(body).toContain('this check re-runs on its own'); + expect(body).toContain('Your PR stays open'); + expect(body).toContain('nothing here closes it'); + expect(body).toContain('a maintainer makes the actual call'); expect(body).toContain('not a judgment on the code'); expect(body).toContain('CONTRIBUTING.md'); expect(body).toContain(CONTRIBUTING_URL); // the deep link, anchor included @@ -1618,7 +1822,8 @@ describe('runGate — CONTRIBUTING.md #3745 policy (mocked fetch)', () => { const body: string = postedBody(calls); expect(body).toContain('A paragraph you wrote yourself'); expect(body).not.toContain('A screenshot of gbrain in use'); // that half is fine - expect(body).toContain('then reopen'); + expect(body).not.toMatch(/reopen/i); + expect(body).toContain('this check re-runs on its own'); }); test('an empty description names both halves', async () => {