diff --git a/scripts/pr-gate.d.mts b/scripts/pr-gate.d.mts index 0d56f71c1..43afa1b95 100644 --- a/scripts/pr-gate.d.mts +++ b/scripts/pr-gate.d.mts @@ -36,6 +36,8 @@ export declare const POLICY_SCAN_MAX: number; export declare const POLICY_EXEMPT_ASSOCIATIONS: string[]; export declare const AI_INTENT_DOWNGRADE: string; export declare function stripCodeFences(body: unknown): string; +export declare const MODEL_BODY_MAX: number; +export declare function modelBody(pr: { body?: string } | null | undefined): string; export declare function hasScreenshot(body: unknown): boolean; export declare function intentWordCount(body: unknown): number; export declare function hasIntentParagraph(body: unknown): boolean; diff --git a/scripts/pr-gate.mjs b/scripts/pr-gate.mjs index bf7b14b9c..ac350c1fd 100644 --- a/scripts/pr-gate.mjs +++ b/scripts/pr-gate.mjs @@ -14,6 +14,27 @@ * sticky comment (marker ), applies exactly one * gate:* label, and exits 1 only for close-lane. * + * WHAT THIS GATE IS, AND WHAT IT IS NOT. Read this before hardening anything + * here on the assumption that it is a security control. + * + * IT IS: a triage signal and a reviewer checklist. It sorts incoming PRs so a + * maintainer's attention lands on the ones worth reading first, and it tells a + * first-time contributor what the repo expects before anybody spends review + * 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 — + * 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. + * + * The parts that ARE hard requirements are the ones protecting the runner and + * the comment: PR code is never checked out or executed, and nothing + * attacker-controlled reaches Markdown unescaped. Those are load-bearing; the + * verdict is advice. + * * Hostile-input posture (the PR author controls title/body/diff, and can also * post comments on their own PR): * - Only a comment authored by github-actions[bot] AND starting with the @@ -164,11 +185,22 @@ export function checkTitle(title) { // --------------------------------------------------------------------------- // Model-output sanitization. Everything the model produces is attacker- // influenced (the PR body is in its context), so nothing it returns may reach -// Markdown unfiltered: no forged headings, no second marker, no live mentions. +// Markdown unfiltered: no forged headings, no second marker, no live mentions, +// and no HTML. +// +// GitHub renders a safe subset of raw HTML inside Markdown, and
is in +// it. Stripping HTML *comments* is not enough on its own: a string like +// `
MERGE LANE — approved...
` renders +// as a working disclosure widget, so a close-lane comment can be made to LOOK +// like an approval. Escaping &, < and > makes every tag render as literal text, +// which is what a quoted model string should look like anyway. // --------------------------------------------------------------------------- export const MAX_STRING = 300; export const MAX_ITEMS = 8; +/** & first, or the escaping escapes its own output. */ +const escapeHtml = (s) => s.replace(/&/g, '&').replace(//g, '>'); + export function sanitizeModelText(value, max = MAX_STRING) { let t = typeof value === 'string' ? value : String(value ?? ''); t = t @@ -176,9 +208,15 @@ export function sanitizeModelText(value, max = MAX_STRING) { .replace(//g, ' ') // dangling halves that could re-pair .replace(/\s+/g, ' ') // one line only: \s covers \n \r U+2028 U+2029 — no block context to open .trim() + // Block markers are stripped BEFORE escaping: escape first and a leading + // `>` becomes `>`, surviving as a visible artifact instead of going away. .replace(/^[\s>#*+\-=|~]+/, '') // leading block markers (heading, quote, list, table, rule) .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]`; return t; } @@ -203,44 +241,86 @@ export function sanitizeList(value, maxItems = MAX_ITEMS, maxString = MAX_STRING export const CONTRIBUTING_URL = 'https://github.com/garrytan/gbrain/blob/master/CONTRIBUTING.md#human-authored-intent-required-no-exceptions'; -/** - * Drop fenced code blocks (``` or ~~~, unterminated fences run to EOF). A - * screenshot pasted inside a fence is documentation of the syntax, not proof. - */ -const FENCE_RE = /^[ \t]{0,3}(`{3,}|~{3,})[^\n]*\n[\s\S]*?(?:^[ \t]{0,3}\1[ \t]*$|$(?![\s\S]))/gm; - /** * The policy scan runs over the FIRST 16KB of the description only. * - * FENCE_RE backtracks superlinearly on a body that is mostly backticks: 65KB of - * them (GitHub's max body length) measured ~8s across the two policy scans, and - * the PR body is attacker-supplied on a `pull_request_target` runner. The cap - * brings the same input to ~0.4s. - * * Tradeoff, stated plainly: a legitimate description whose intent paragraph AND * screenshot both sit past 16KB of preamble would be judged on the truncated * text and could be closed for a paragraph it does contain. In practice both * appear near the top — .github/pull_request_template.md puts them in the first * two sections, and 16KB is ~2,500 words of prose before the screenshot. The * model payload already caps the same body at 6KB, so the cap here is the looser - * of the two. Raise it if a real PR ever trips it; do not remove it. + * of the two. Raise it if a real PR ever trips it; do not remove it: the body is + * attacker-supplied on a `pull_request_target` runner, and this is the bound on + * every scan below. */ export const POLICY_SCAN_MAX = 16384; -export const stripCodeFences = (body) => - String(body ?? '') - .slice(0, POLICY_SCAN_MAX) - .replace(FENCE_RE, '\n'); + +const FENCE_OPEN_RE = /^[ \t]{0,3}(`{3,}|~{3,})([^\n]*)$/; + +/** + * Drop fenced code blocks (``` or ~~~, unterminated fences run to EOF). A + * screenshot pasted inside a fence is documentation of the syntax, not proof. + * + * Line scanner, not one regex, because the CommonMark closing rule needs a + * length COMPARISON and a backreference can only express equality. A closing + * fence must use the same character and be AT LEAST as long as the opening one, + * so ```` closes ``` — under the old `\1` backreference it did not, the engine + * read it as a new opening fence, and everything after it was stripped to EOF. + * A compliant PR that documented fence syntax then failed the intent check and + * was closed. (The scanner is also linear, which retires the superlinear- + * backtracking hazard the 16KB cap was sized against.) + */ +export const stripCodeFences = (body) => { + const out = []; + let fence = null; // { char, len } while inside a block + for (const line of String(body ?? '').slice(0, POLICY_SCAN_MAX).split('\n')) { + const m = FENCE_OPEN_RE.exec(line); + if (fence) { + // Same character, at least as long, and no info string after it. + if (m && m[1][0] === fence.char && m[1].length >= fence.len && m[2].trim() === '') fence = null; + continue; // fenced content and the fences themselves are not prose + } + if (m) { + fence = { char: m[1][0], len: m[1].length }; + continue; + } + out.push(line); + } + return out.join('\n'); +}; + +const HTML_COMMENT_RE = //g; + +/** Fences and HTML comments both hide text that renders as nothing. */ +const visibleText = (body) => stripCodeFences(body).replace(HTML_COMMENT_RE, ' '); + +// A URL that could actually resolve to an image: absolute, root-relative, or +// something carrying an image extension. `x` is not one. +const IMAGE_URL_RE = /^(?:https?:\/\/\S|\/\S|\S+\.(?:png|jpe?g|gif|webp|svg|avif|bmp|heic)\b)/i; const SCREENSHOT_RES = [ - /!\[[^\]]*\]\(\s*\S/, // markdown image embed - /]*>/i, // raw HTML img tag - /https:\/\/user-images\.githubusercontent\.com\/\S/i, // legacy paste URL - /https:\/\/github\.com\/user-attachments\/assets\/\S/i, // current paste URL + // Markdown embed — the URL must look like a URL, not like a placeholder. + (t) => [...t.matchAll(/!\[[^\]]*\]\(\s*([^)\s]+)/g)].some((m) => IMAGE_URL_RE.test(m[1])), + // Raw HTML img — must carry a src= with a non-empty value. + (t) => /]*\bsrc\s*=\s*(?:"[^"]+"|'[^']+'|[^\s>"'][^\s>]*)/i.test(t), + (t) => /https:\/\/user-images\.githubusercontent\.com\/\S/i.test(t), // legacy paste URL + (t) => /https:\/\/github\.com\/user-attachments\/assets\/\S/i.test(t), // current paste URL ]; +/** + * A FLOOR, not proof. This checks that something image-shaped is actually + * embedded — it cannot check that the image shows gbrain, or that the author + * took it. Anyone who wants to clear it can paste any image at all, and that is + * fine: the check exists to filter zero-effort submissions (an empty body, a + * "screenshot attached" claim with nothing attached, the syntax pasted inside a + * code fence). A human reviewer makes the real call. Do not add cleverness here + * expecting it to hold against someone trying — see the IS/IS NOT block at the + * top of this file. + */ export function hasScreenshot(body) { - const text = stripCodeFences(body); - return SCREENSHOT_RES.some((re) => re.test(text)); + const text = visibleText(body); + return SCREENSHOT_RES.some((match) => match(text)); } export const INTENT_MIN_WORDS = 40; @@ -250,8 +330,7 @@ export const INTENT_MIN_WORDS = 40; // template's own bold prompts (a whole line of `**...**` is a heading in // disguise). What survives is the author's own prose. export function intentWordCount(body) { - const prose = stripCodeFences(body) - .replace(//g, ' ') // HTML comments (the PR template's hints) + const prose = visibleText(body) // fences + HTML comments (the PR template's hints) .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 @@ -571,8 +650,8 @@ function buildPayload({ pr, files, diff, titleCheck, flags }) { '--- UNTRUSTED PR TITLE ---', pr.title ?? '', '', - '--- UNTRUSTED PR BODY (capped at 6KB) ---', - (pr.body ?? '(empty)').slice(0, 6000), + `--- UNTRUSTED PR BODY (capped at ${MODEL_BODY_MAX / 1000}KB) ---`, + modelBody(pr), '', '--- CHANGED FILES (first 100) ---', fileList, @@ -673,9 +752,22 @@ async function setLaneLabel(gh, repo, prNumber, lane) { // The exemption is part of the input tuple: a draft PR marked ready-for-review // changes neither title, body nor head sha, so without it the spend guard would // keep serving the verdict computed while the policy check was waived. +// +// The tuple hashes what the run actually CONSUMES, not the raw body: the model +// only ever sees the first MODEL_BODY_MAX bytes, so hashing the whole body made +// a one-byte edit past that offset mint a new hash and buy a fresh paid call +// with byte-identical model input. The mechanical policy verdict IS computed +// from the full (16KB-capped) body, so its outcome is hashed alongside the +// truncated text — otherwise adding the missing screenshot past 6KB would leave +// the hash unchanged and the cached close-lane would be served forever. +export const MODEL_BODY_MAX = 6000; +export const modelBody = (pr) => (pr?.body ?? '(empty)').slice(0, MODEL_BODY_MAX); + export function hashInputs(pr) { + const exemption = policyExemption(pr) ?? ''; + const policy = exemption ? [] : detectPolicyMisses(pr?.body).map((f) => f.id); return createHash('sha256') - .update(JSON.stringify([pr.title ?? '', pr.body ?? '', pr.head?.sha ?? '', policyExemption(pr) ?? ''])) + .update(JSON.stringify([pr.title ?? '', modelBody(pr), pr.head?.sha ?? '', exemption, policy])) .digest('hex') .slice(0, 16); } @@ -787,6 +879,8 @@ export function renderComment({ lines.push( '', 'Strict usefulness gate (#3698). merge-lane / needs-maintainer exit green; close-lane exits red (strong signal, not a hard block — maintainers decide). PR code is never checked out or executed: verdict is from API metadata + a 120KB-capped diff only.', + '', + 'This is a triage signal and a reviewer checklist, not an authorization boundary. The mechanical checks are floors a determined author can clear; a human reviewer makes the real call.', ); return lines.join('\n'); } @@ -816,6 +910,7 @@ 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 await upsertStickyComment( gh, repo, @@ -823,7 +918,6 @@ export async function runGate(dir, env = process.env, fetchImpl = fetch) { existing, renderComment({ titleCheck, flags, policyExempt, neutralReason: reason }), ); - await setLaneLabel(gh, repo, prNumber, null); // no stale verdict survives a skip return 0; }; @@ -900,8 +994,15 @@ export async function runGate(dir, env = process.env, fetchImpl = fetch) { policyExempt, state: { hash: inputHash, lane }, }); - await upsertStickyComment(gh, repo, prNumber, existing, body); + // ORDER IS LOAD-BEARING: labels FIRST, then the comment carrying the cached + // state. The comment is what makes a rerun short-circuit on the spend guard, + // so persisting it before the labels are reconciled turns a transient label + // API failure into a permanent one — the rerun sees "same hash, lane already + // decided", returns success, and never repairs the stale/missing/duplicate + // label. Written in this order, a failed label call throws with no state + // persisted, and the next run redoes the whole thing. await setLaneLabel(gh, repo, prNumber, lane); + await upsertStickyComment(gh, repo, prNumber, existing, body); console.log( `PR gate verdict: ${lane} (confidence ${verdict.confidence}${degraded ? ', degraded' : ''}${ diff --git a/test/pr-gate-workflow.test.ts b/test/pr-gate-workflow.test.ts index 13e545317..a08bf6c8c 100644 --- a/test/pr-gate-workflow.test.ts +++ b/test/pr-gate-workflow.test.ts @@ -31,6 +31,9 @@ import { detectPolicyMisses, hasScreenshot, hasIntentParagraph, + stripCodeFences, + modelBody, + MODEL_BODY_MAX, intentWordCount, sanitizeModelText, sanitizeList, @@ -75,16 +78,17 @@ const CONTRIBUTING = readFileSync(CONTRIBUTING_PATH, 'utf8'); const PR_TEMPLATE = readFileSync(PR_TEMPLATE_PATH, 'utf8'); /** - * Collect every line that belongs to a `run:` script, in all four YAML scalar - * spellings: `run: cmd`, `run: |`, `run: >` and their `|-`/`>-`/`|+`/`>+` - * chomping variants. A folded block hides interpolation from a `|`-only - * scanner, which is exactly how an env-binding rule rots. + * Collect every line that belongs to a `run:` script, in EVERY YAML block + * scalar spelling: `run: cmd`, `run: |`, `run: >`, the `-`/`+` chomping + * indicators, and the numeric indentation indicator in either order (`|2-` + * and `|-2` are both legal headers). A spelling the scanner cannot see hides + * interpolation from the env-binding rule, which is exactly how that rule rots. */ function runBlockLines(yaml: string): string[] { const lines = yaml.split('\n'); const out: string[] = []; for (let i = 0; i < lines.length; i++) { - const block = lines[i].match(/^(\s*)(?:-\s+)?run:\s*[|>][-+]?\d*\s*$/); + const block = lines[i].match(/^(\s*)(?:-\s+)?run:\s*[|>][0-9]*[-+]?[0-9]*\s*$/); if (block) { const baseIndent = block[1].length; for (let j = i + 1; j < lines.length; j++) { @@ -159,6 +163,24 @@ describe('pr-gate workflow security pins', () => { expect(runBlockLines(single).join('\n')).toContain('${{'); }); + test('the run: scanner sees indentation indicators in both legal orders', () => { + // `|2-` / `>2-` are valid block headers (YAML allows the indentation and + // chomping indicators in either order). A scanner that only knew `|-2` + // would read `run: >2-` as an ordinary value, skip the whole block, and + // report a clean workflow while attacker-controlled text was being + // interpolated straight into the shell. + for (const header of ['>2-', '|2-', '>2', '|2', '>-2', '|+2', '|', '>']) { + const yaml = [ + 'jobs:', + ' x:', + ' steps:', + ` - run: ${header}`, + ' echo ${{ github.event.pull_request.title }}', + ].join('\n'); + expect(runBlockLines(yaml).join('\n')).toContain('${{'); + } + }); + test('all actions are SHA-pinned', () => { const uses = [...WORKFLOW.matchAll(/uses:\s*(\S+)/g)].map((m) => m[1]); expect(uses.length).toBeGreaterThan(0); @@ -604,12 +626,90 @@ describe('mechanical flag details are attacker-controlled (filename injection)', }); }); +// A closing fence must be the same character and AT LEAST as long as the +// opening one (CommonMark 4.5). Getting that backwards is not a security hole, +// it is a false positive that CLOSES compliant PRs: a body documenting fence +// syntax had everything after the longer fence stripped to EOF, so its intent +// paragraph vanished and the gate closed it for a paragraph it did contain. +describe('stripCodeFences (CommonMark fence matching)', () => { + const prose = 'real human intent paragraph about my problem '.repeat(15); + + test('a matching 3-backtick fence closes', () => { + expect(stripCodeFences('```\nhidden\n```\nvisible')).toContain('visible'); + expect(stripCodeFences('```\nhidden\n```\nvisible')).not.toContain('hidden'); + }); + + test('a LONGER closing fence closes the block (the false positive)', () => { + // The bug: ```` was read as a new opening fence, so `prose` was stripped to + // EOF and a legitimate description failed the intent check. + const body = `\`\`\`js\ncode\n\`\`\`\`\n${prose}`; + expect(stripCodeFences(body)).toContain('real human intent paragraph'); + expect(stripCodeFences(body)).not.toContain('code'); + expect(hasIntentParagraph(body)).toBe(true); + }); + + test('a SHORTER closing fence does not close — the block runs to EOF', () => { + const body = `\`\`\`\`\ncode\n\`\`\`\n${prose}`; + expect(stripCodeFences(body)).not.toContain('real human intent paragraph'); + expect(hasIntentParagraph(body)).toBe(false); + }); + + test('tilde fences behave the same and do not cross-close backticks', () => { + expect(stripCodeFences('~~~\nhidden\n~~~\nvisible')).toContain('visible'); + expect(stripCodeFences('~~~~\nhidden\n~~~\nstill hidden')).not.toContain('still hidden'); + // A ``` line inside a ~~~ block is content, not a closer. + expect(stripCodeFences('~~~\n```\nhidden\n~~~\nvisible')).toContain('visible'); + expect(stripCodeFences('~~~\n```\nhidden\n~~~\nvisible')).not.toContain('hidden'); + }); + + test('an unterminated fence swallows the rest of the body', () => { + expect(stripCodeFences(`\`\`\`\n${prose}`)).not.toContain('real human intent paragraph'); + expect(hasIntentParagraph(`\`\`\`\n${prose}`)).toBe(false); + }); + + test('a closing fence may not carry an info string', () => { + // ```` ```js ```` opens; a second ` ```js ` line is content, not a closer. + expect(stripCodeFences('```js\nhidden\n```js\nstill hidden')).not.toContain('still hidden'); + }); +}); + describe('hasScreenshot (#3745, mechanical)', () => { test('accepts all four embed forms GitHub produces', () => { expect(hasScreenshot('here it is:\n\n![my terminal](https://example.com/shot.png)')).toBe(true); expect(hasScreenshot('https://user-images.githubusercontent.com/1234/98765-abcdef.png')).toBe(true); expect(hasScreenshot('https://github.com/user-attachments/assets/0a1b2c3d-4e5f-6789')).toBe(true); expect(hasScreenshot('run')).toBe(true); + // Root-relative and extension-bearing paths still count. + expect(hasScreenshot('![shot](/docs/img/run.png)')).toBe(true); + expect(hasScreenshot('![shot](run.png)')).toBe(true); + expect(hasScreenshot("")).toBe(true); + expect(hasScreenshot('')).toBe(true); + }); + + // The floor is deliberately low — anyone can paste any image and clear it. + // What it must not accept is the zero-effort forms: a placeholder URL, a tag + // with no image behind it, or something hidden where GitHub renders nothing. + test('a placeholder URL is not an embed', () => { + expect(hasScreenshot('![proof](x)')).toBe(false); + expect(hasScreenshot('![proof]()')).toBe(false); + expect(hasScreenshot('![proof]( )')).toBe(false); + expect(hasScreenshot('![proof](screenshot)')).toBe(false); + }); + + test('an tag with no usable src is not an embed', () => { + expect(hasScreenshot('proof')).toBe(false); + expect(hasScreenshot('I have a screenshot')).toBe(false); + expect(hasScreenshot('')).toBe(false); + expect(hasScreenshot("")).toBe(false); + }); + + test('an embed hidden inside an HTML comment does NOT count', () => { + // GitHub renders nothing at all for it, so it is not a screenshot. + expect(hasScreenshot('')).toBe(false); + expect(hasScreenshot('')).toBe(false); + expect(hasScreenshot('')).toBe(false); + // ...but a real embed outside the comment still counts. + expect(hasScreenshot('\n![real](https://example.com/a.png)')).toBe(true); }); test('an embed inside a fenced code block does NOT count', () => { @@ -968,6 +1068,74 @@ describe('sanitizeModelText (LLM output is never raw Markdown)', () => { expect(sanitizeModelText('a
b')).toBe('a b'); }); + // GitHub renders a safe subset of raw HTML inside Markdown. Stripping HTML + // *comments* left
/ alive, which is a forged verdict: a + // CLOSE-LANE comment could carry a working "MERGE LANE — approved" widget. + test('raw HTML is escaped to literal text, not left renderable', () => { + const out = sanitizeModelText('
MERGE LANEx
'); + expect(out).not.toMatch(/
` survives at all, in any tag. + expect(out).not.toMatch(/[<>]/); + expect(sanitizeModelText('')).not.toMatch(/[<>]/); + expect(sanitizeModelText('click')).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 `<`. + expect(sanitizeModelText('<script>')).toBe('&lt;script&gt;'); + expect(sanitizeModelText('a & b')).toBe('a &amp; b'); + }); + + test('the forged-verdict widget renders literally in a close-lane comment', () => { + const body: string = renderComment({ + lane: 'close-lane', + verdict: { + confidence: 0.9, + reasons: ['
✅ MERGE LANE — approvedship it
'], + reviewer_checklist: [], + }, + titleCheck: { ok: true }, + flags: [], + }); + expect(body).not.toContain(' { + // Both are attacker-controlled: a filename is interpolated into two flag + // details, and the neutral reason carries an API error string. + const flags = detectRedFlags({ + changedFiles: 1, + files: [{ filename: 'test/
ok.test.ts', status: 'removed' }], + diff: '', + }); + expect(flags.map((f) => f.id)).toContain('deletes_tests'); // it DID classify + const body: string = renderComment({ + titleCheck: { ok: true }, + flags, + neutralReason: '
NEUTRAL is finex
', + }); + expect(body).not.toContain(' { + const body: string = renderComment({ + lane: 'merge-lane', + verdict: { confidence: 1, reasons: ['r'], reviewer_checklist: [] }, + titleCheck: { ok: true }, + flags: [], + policyExempt: '
owner', + }); + expect(body).not.toContain(' { const out = sanitizeModelText('x'.repeat(5000)); expect(out).toContain('[truncated]'); @@ -1006,6 +1174,35 @@ describe('isOwnComment / hashInputs / parseState', () => { expect(hashInputs(pr)).not.toBe(hashInputs({ ...pr, head: { sha: 'def' } })); }); + // The model only ever sees modelBody(pr). Hashing the whole body meant a + // one-byte edit past the cap minted a new hash and bought a fresh paid call + // with byte-identical model input — the exact amplification the guard exists + // to stop. + test('the hash covers what the model consumes, not the whole body', () => { + expect(modelBody({ body: 'x'.repeat(MODEL_BODY_MAX + 500) })).toHaveLength(MODEL_BODY_MAX); + const head = `${HUMAN_INTENT}\n\n${SCREENSHOT_EMBED}\n${'padding words here. '.repeat(400)}`; + expect(head.length).toBeGreaterThan(MODEL_BODY_MAX); + const pr = (tail: string) => ({ title: 't', body: head + tail, head: { sha: 'abc' } }); + // Same first 6KB, same policy verdict → same inputs → no new call. + expect(hashInputs(pr('a'))).toBe(hashInputs(pr('b'))); + expect(hashInputs(pr(''))).toBe(hashInputs(pr('completely different trailing prose'))); + // An edit INSIDE the window still mints a new hash. + const edited = { title: 't', body: `edited ${head}`, head: { sha: 'abc' } }; + expect(hashInputs(edited)).not.toBe(hashInputs(pr(''))); + }); + + test('a policy fix past the model cap still invalidates the cached verdict', () => { + // The mechanical policy scan reads 16KB, so its outcome is hashed too. + // Without that, adding the missing screenshot at 8KB would leave the hash + // unchanged and the cached close-lane would be served forever. + const filler = 'padding words here. '.repeat(400); // > MODEL_BODY_MAX + const before = { title: 't', body: `${HUMAN_INTENT}\n\n${filler}`, head: { sha: 'abc' } }; + const after = { title: 't', body: `${HUMAN_INTENT}\n\n${filler}\n\n${SCREENSHOT_EMBED}`, head: { sha: 'abc' } }; + expect(detectPolicyMisses(before.body).map((f) => f.id)).toEqual(['missing_screenshot']); + expect(detectPolicyMisses(after.body)).toEqual([]); + expect(hashInputs(before)).not.toBe(hashInputs(after)); + }); + test('state round-trips through the rendered comment', () => { const body = renderComment({ lane: 'close-lane', @@ -1053,8 +1250,12 @@ function jsonResponse(payload: unknown, status = 200): Response { } function stubFetch(opts: { - comments?: unknown[]; + comments?: any[]; anthropic?: (n: number) => Response; + /** Fail the label-add call — models a transient GitHub labels-API blip. */ + labelAddFails?: () => boolean; + /** Persist the sticky comment into `comments`, so a rerun sees the last run's state. */ + persistComments?: boolean; }): { calls: Call[]; fetchImpl: typeof fetch } { const calls: Call[] = []; let anthropicCount = 0; @@ -1069,9 +1270,20 @@ function stubFetch(opts: { return opts.anthropic(anthropicCount++); } if (/\/issues\/\d+\/comments\?/.test(u)) return jsonResponse(opts.comments ?? []); - if (/\/issues\/comments\/\d+$/.test(u) && method === 'PATCH') return jsonResponse({ id: 99 }); - if (/\/issues\/\d+\/comments$/.test(u) && method === 'POST') return jsonResponse({ id: 100 }, 201); - if (/\/issues\/\d+\/labels$/.test(u) && method === 'POST') return jsonResponse([]); + if (/\/issues\/comments\/\d+$/.test(u) && method === 'PATCH') { + if (opts.persistComments && opts.comments?.[0]) opts.comments[0].body = body.body; + return jsonResponse({ id: 99 }); + } + if (/\/issues\/\d+\/comments$/.test(u) && method === 'POST') { + if (opts.persistComments) { + opts.comments!.push({ id: 100, user: { type: 'Bot', login: 'github-actions[bot]' }, body: body.body }); + } + return jsonResponse({ id: 100 }, 201); + } + if (/\/issues\/\d+\/labels$/.test(u) && method === 'POST') { + if (opts.labelAddFails?.()) return jsonResponse({ message: 'server error' }, 500); + return jsonResponse([]); + } if (/\/issues\/\d+\/labels\//.test(u) && method === 'DELETE') return jsonResponse([]); if (/\/repos\/[^/]+\/[^/]+\/labels$/.test(u) && method === 'POST') return jsonResponse({}, 201); return jsonResponse({ message: `unrouted ${method} ${u}` }, 404); @@ -1276,6 +1488,57 @@ describe('runGate end-to-end (mocked fetch)', () => { expect(calls.some((c) => c.method === 'PATCH' || c.method === 'POST')).toBe(false); // nothing rewritten }); + // "Exactly one gate:* label" is only true if a failed label call can be + // repaired. The sticky comment carries the cached state that makes a rerun + // short-circuit, so writing it BEFORE the labels are reconciled turns one + // transient 500 into a permanently wrong label set. + test('a failed label call is repaired by an identical rerun', async () => { + const comments: any[] = []; + let failLabels = true; + const { calls, fetchImpl } = stubFetch({ + comments, + persistComments: true, + labelAddFails: () => failLabels, + anthropic: () => verdictResponse(CLEAN_VERDICT), + }); + const dir = fixtureDir({}, [ + { filename: 'src/a.ts', status: 'modified', additions: 2, deletions: 1 }, + { filename: 'test/a.test.ts', status: 'modified', additions: 5, deletions: 0 }, + ]); + + // Run 1: the label API blips. The run fails loudly... + await expect(runGate(dir, ENV, fetchImpl)).rejects.toThrow(/label add failed/); + // The label add was ATTEMPTED (it is the first write)... + expect(addedLabels(calls)).toEqual(['gate:merge-lane']); + // ...and because it failed first, NO cached state was persisted, so the + // rerun cannot short-circuit on it. + expect(comments).toHaveLength(0); + + // Run 2: byte-identical inputs, labels API healthy again. + failLabels = false; + calls.length = 0; + const code = await runGate(dir, ENV, fetchImpl); + expect(code).toBe(0); + expect(addedLabels(calls)).toEqual(['gate:merge-lane']); + expect(deletedLabels(calls).sort()).toEqual(['gate:close-lane', 'gate:needs-maintainer']); + expect(parseState(postedBody(calls))).toMatchObject({ lane: 'merge-lane' }); + }); + + test('labels are reconciled before the state block is persisted', async () => { + // The ordering itself, pinned directly: whatever else changes, the label + // write must not come after the comment that lets a rerun short-circuit. + const { calls, fetchImpl } = stubFetch({ anthropic: () => verdictResponse(CLEAN_VERDICT) }); + await runGate(fixtureDir({}, [ + { filename: 'src/a.ts', status: 'modified', additions: 2, deletions: 1 }, + { filename: 'test/a.test.ts', status: 'modified', additions: 5, deletions: 0 }, + ]), ENV, fetchImpl); + const labelAt = calls.findIndex((c) => c.method === 'POST' && /\/issues\/\d+\/labels$/.test(c.url)); + const commentAt = calls.findIndex((c) => /\/issues\/\d+\/comments$/.test(c.url) && c.method === 'POST'); + expect(labelAt).toBeGreaterThanOrEqual(0); + expect(commentAt).toBeGreaterThanOrEqual(0); + expect(labelAt).toBeLessThan(commentAt); + }); + test('spend guard does not fire when the head sha moved', async () => { const pr = { title: 'fix(core): a real fix', body: COMPLIANT_BODY, head: { sha: 'cafebabe' } }; const prior = {