diff --git a/scripts/pr-gate.d.mts b/scripts/pr-gate.d.mts index 836cc1dd1..b5bad976f 100644 --- a/scripts/pr-gate.d.mts +++ b/scripts/pr-gate.d.mts @@ -69,6 +69,8 @@ export declare function isOwnComment(comment: GhComment | null | undefined): boo export declare function hashInputs( pr: PrIdentity & { title?: string; body?: string; head?: { sha?: string } }, + /** The assembled model payload (changed files + diff). runGate always passes it. */ + payload?: string, ): string; export declare function parseState(body: unknown): { hash: string; lane?: string } | null; diff --git a/scripts/pr-gate.mjs b/scripts/pr-gate.mjs index 39023b862..c92576d5f 100644 --- a/scripts/pr-gate.mjs +++ b/scripts/pr-gate.mjs @@ -43,12 +43,16 @@ * 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 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 + * no image embeds, no LABELLED 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. + * Deliberate stopping point: a BARE url left in a sanitized string still + * autolinks under GFM. That is a self-labelled link — the reader sees exactly + * where it goes — which is why the escaping targets the MASKING characters + * (`[`/`]`) rather than mangling every URL a model legitimately cites. * - 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 @@ -278,6 +282,16 @@ export const POLICY_SCAN_MAX = 16384; const FENCE_OPEN_RE = /^[ \t]{0,3}(`{3,}|~{3,})([^\n]*)$/; +/** + * CommonMark 4.5: a BACKTICK fence's info string may not contain a backtick, + * because ``` `foo` ``` on its own line has to stay an ordinary paragraph with + * inline code in it. A TILDE fence's info string may contain anything. + * + * Only ever asked at the OPENING site. A closing fence may carry no info string + * at all, so the rule is already subsumed there. + */ +const opensFence = (m) => m[1][0] === '~' || !m[2].includes('`'); + /** * Drop fenced code blocks (``` or ~~~, unterminated fences run to EOF). A * screenshot pasted inside a fence is documentation of the syntax, not proof. @@ -290,6 +304,11 @@ const FENCE_OPEN_RE = /^[ \t]{0,3}(`{3,}|~{3,})([^\n]*)$/; * 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.) + * + * Opening too eagerly is the same false-positive class and the same cost: every + * line to EOF disappears, the intent paragraph with it, and a compliant + * contributor gets a red X. Both rules below therefore err toward NOT opening a + * block that CommonMark would not open. */ export const stripCodeFences = (body) => { const out = []; @@ -301,7 +320,7 @@ export const stripCodeFences = (body) => { 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) { + if (m && opensFence(m)) { fence = { char: m[1][0], len: m[1].length }; continue; } @@ -829,8 +848,9 @@ async function setLaneLabel(gh, repo, prNumber, lane) { // --------------------------------------------------------------------------- // Spend guard: `edited` + `synchronize` amplify a single PR into many runs. -// The verdict only depends on title + body + head sha, so if those are -// unchanged since the last sticky comment there is nothing new to classify. +// The verdict is a function of the model payload (and of the mechanical policy +// outcome), so if that payload is byte-identical to the one behind the last +// sticky comment there is nothing new to classify. // --------------------------------------------------------------------------- // JSON.stringify is the separator: it quotes and escapes each field, so no // title or body can forge a boundary, and the tuple order is fixed by the @@ -847,14 +867,31 @@ async function setLaneLabel(gh, repo, prNumber, lane) { // 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. +// +// "What the run consumes" is the WHOLE model payload, not just the body. The +// changed-file list and the diff are in it too, and the workflow degrades the +// diff to a one-line marker when the API 406s on a huge one. Hashing only the +// body made that degradation permanent: run 1 fetched no diff and cached a +// diff-blind verdict, run 2 had the real diff, matched the hash, and served the +// diff-blind verdict forever. So the assembled payload is folded in as a +// fixed-width digest — inside the tuple, where JSON.stringify's quoting still +// makes a forged boundary impossible. export const MODEL_BODY_MAX = 6000; export const modelBody = (pr) => (pr?.body ?? '(empty)').slice(0, MODEL_BODY_MAX); -export function hashInputs(pr) { +/** + * @param payload the exact string buildPayload() hands the model. Omitted only + * by unit tests comparing two prs against each other; runGate always passes + * it, pinned by the diff-unavailable→available test. + */ +export function hashInputs(pr, payload = '') { const exemption = policyExemption(pr) ?? ''; const policy = exemption ? [] : detectPolicyMisses(pr?.body).map((f) => f.id); + const payloadDigest = createHash('sha256').update(String(payload ?? '')).digest('hex'); return createHash('sha256') - .update(JSON.stringify([pr.title ?? '', modelBody(pr), pr.head?.sha ?? '', exemption, policy])) + .update( + JSON.stringify([pr.title ?? '', modelBody(pr), pr.head?.sha ?? '', exemption, policy, payloadDigest]), + ) .digest('hex') .slice(0, 16); } @@ -1038,7 +1075,12 @@ export async function runGate(dir, env = process.env, fetchImpl = fetch) { return 0; }; - const inputHash = hashInputs(pr); + // Built once, unconditionally, and hashed: the spend guard must key on the + // bytes the model actually sees. Building it on the policy-miss path too + // (where no model call happens) keeps ONE hash convention across both paths — + // two conventions is how a cached verdict gets served to the wrong inputs. + const payload = buildPayload({ pr, files, diff, titleCheck, flags }); + const inputHash = hashInputs(pr, payload); let verdict; let degraded = null; if (policyMisses.length > 0) { @@ -1069,13 +1111,13 @@ export async function runGate(dir, env = process.env, fetchImpl = fetch) { const prev = parseState(existing?.body); if (prev && prev.hash === inputHash && LANES.includes(prev.lane)) { console.log( - `PR gate: title+body+head_sha unchanged (${inputHash}) since the last verdict — skipping the LLM call, keeping ${prev.lane}.`, + `PR gate: model payload unchanged (${inputHash}) since the last verdict — skipping the LLM call, keeping ${prev.lane}.`, ); return prev.lane === 'close-lane' ? 1 : 0; } try { - verdict = await callAnthropic(apiKey, buildPayload({ pr, files, diff, titleCheck, flags }), fetchImpl); + verdict = await callAnthropic(apiKey, payload, fetchImpl); } catch (err) { const detail = String(err?.message ?? err).slice(0, 200); if (err?.kind !== 'refusal' && err?.kind !== 'schema') { diff --git a/test/pr-gate-workflow.test.ts b/test/pr-gate-workflow.test.ts index 79e881206..2b804a1e0 100644 --- a/test/pr-gate-workflow.test.ts +++ b/test/pr-gate-workflow.test.ts @@ -13,9 +13,15 @@ * 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. + * - CommonMark fence matching in BOTH directions: a closing fence longer than + * its opener closes, and a backtick fence whose info string contains a + * backtick never opens (4.5). Opening a block CommonMark would not open + * strips the author's prose to EOF — the same red X as closing one late. * - 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. + * NEUTRAL label clearing, label swap, and the spend guard — which keys on the + * whole model payload, so a verdict reached while the diff was unavailable is + * not served back once the real diff arrives. * - The CONTRIBUTING.md #3745 policy: the mechanical screenshot + intent * detectors (all four embed forms, the in-code-fence negative, the real * .github/pull_request_template.md, non-English prose), the forced @@ -27,6 +33,7 @@ * and through a 500, while a compliant PR keeps the loud NEUTRAL skip. */ import { describe, test, expect } from 'bun:test'; +import { safeLoad as yamlLoad } from 'js-yaml'; import { readFileSync, existsSync, mkdtempSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -85,16 +92,24 @@ const PR_TEMPLATE = readFileSync(PR_TEMPLATE_PATH, 'utf8'); /** * 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. + * indicators, the numeric indentation indicator in either order (`|2-` and + * `|-2` are both legal headers), and a trailing comment after the header + * (`run: | # shell block` is legal YAML — js-yaml parses it as a block, pinned + * below). A spelling the scanner cannot see hides interpolation from the + * env-binding rule, which is exactly how that rule rots: the comment spelling + * used to fall through to the single-line branch, which captured the HEADER + * (`| # shell block`) as if it were the whole command and never looked at the + * block body at all — a clean report over an interpolating workflow. */ 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*[|>][0-9]*[-+]?[0-9]*\s*$/); + const block = lines[i].match(/^(\s*)(?:-\s+)?run:\s*[|>][0-9]*[-+]?[0-9]*([ \t]+#.*)?\s*$/); if (block) { + // A `${{ }}` in a YAML comment is inert (it is not part of the scalar), + // but scan it anyway rather than leave the scanner a hiding place. + if (block[2]) out.push(block[2]); const baseIndent = block[1].length; for (let j = i + 1; j < lines.length; j++) { if (lines[j].trim() === '') continue; @@ -186,6 +201,38 @@ describe('pr-gate workflow security pins', () => { } }); + test('the run: scanner sees a block header carrying a trailing comment', () => { + // Guards the guard against REALITY, not against the scanner's own opinion. + // `run: | # shell block` is a legal block header, and the interpolation on + // the next line really does end up in the script — so a purely cosmetic + // formatting edit must not be able to blind the env-binding rule above. + const yaml = [ + 'jobs:', + ' x:', + ' steps:', + ' - run: | # shell block', + ' echo ${{ github.event.pull_request.title }}', + ].join('\n'); + const parsed = yamlLoad(yaml) as { jobs: { x: { steps: { run: string }[] } } }; + expect(parsed.jobs.x.steps[0].run).toContain('${{'); // YAML really puts it in the script… + expect(runBlockLines(yaml).join('\n')).toContain('${{'); // …and the scanner really sees it. + // The comment composes with every chomping/indentation spelling. + for (const header of ['|', '>', '|-', '>2-', '|+2']) { + const y = [ + 'jobs:', + ' x:', + ' steps:', + ` - run: ${header} # note`, + ' echo ${{ github.head_ref }}', + ].join('\n'); + expect(runBlockLines(y).join('\n')).toContain('${{'); + } + // A `${{ }}` inside the header comment is inert YAML, but it is scanned + // anyway — the scanner is not left a hiding place. + const inComment = ['jobs:', ' x:', ' steps:', ' - run: | # ${{ github.head_ref }}', ' echo hi'].join('\n'); + expect(runBlockLines(inComment).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); @@ -676,6 +723,35 @@ describe('stripCodeFences (CommonMark fence matching)', () => { // ```` ```js ```` opens; a second ` ```js ` line is content, not a closer. expect(stripCodeFences('```js\nhidden\n```js\nstill hidden')).not.toContain('still hidden'); }); + + test('a backtick fence info string may not contain a backtick (CommonMark 4.5)', () => { + // The other half of the false-positive class above. Opening a block that + // CommonMark never opens costs exactly what closing one late costs: every + // line to EOF disappears, the intent paragraph with it, red X on a body + // GitHub renders perfectly. + const backtickInfo = `\`\`\`foo\`bar\n${prose}`; + expect(stripCodeFences(backtickInfo)).toContain('real human intent paragraph'); + expect(hasIntentParagraph(backtickInfo)).toBe(true); + + // A TILDE fence has no such restriction — this one really does open. + const tildeInfo = `~~~foo\`bar\n${prose}`; + expect(stripCodeFences(tildeInfo)).not.toContain('real human intent paragraph'); + expect(hasIntentParagraph(tildeInfo)).toBe(false); + + // And a backtick-free info string still opens a backtick fence, as always. + expect(stripCodeFences(`\`\`\`js\n${prose}`)).not.toContain('real human intent paragraph'); + expect(stripCodeFences('```js\nhidden\n```\nvisible')).toContain('visible'); + expect(stripCodeFences('```js\nhidden\n```\nvisible')).not.toContain('hidden'); + }); + + test('the reported false positive, end to end: prose + screenshot after such a line', () => { + // Verbatim shape of the repro: a line whose info string carries a backtick, + // then real prose, then a real embed. Both #3745 halves are present in the + // rendered description, so the gate must report neither as missing. + const body = `\`\`\`foo\`bar\n${prose}\n${SCREENSHOT_EMBED}`; + expect(intentWordCount(body)).toBeGreaterThanOrEqual(INTENT_MIN_WORDS); + expect(detectPolicyMisses(body)).toEqual([]); + }); }); describe('hasScreenshot (#3745, mechanical)', () => { @@ -1359,6 +1435,27 @@ describe('isOwnComment / hashInputs / parseState', () => { expect(detectPolicyMisses(before.body).map((f) => f.id)).toEqual(['missing_screenshot']); expect(detectPolicyMisses(after.body)).toEqual([]); expect(hashInputs(before)).not.toBe(hashInputs(after)); + // Round 5's property, re-pinned with the payload term present: the policy + // outcome must still invalidate even when the model payload is identical. + expect(modelBody(before)).toBe(modelBody(after)); // same 6KB window + expect(hashInputs(before, 'identical payload')).not.toBe(hashInputs(after, 'identical payload')); + }); + + // The model reads the changed-file list and the diff too, and the workflow + // degrades the diff to a marker line when the API 406s on a huge one. Hashing + // only the PR fields froze that: a run that classified with no diff cached its + // verdict, and the next run — real diff in hand — matched the hash and served + // the diff-blind verdict forever. + test('the hash covers the model payload, not just the PR fields', () => { + const pr = { title: 't', body: COMPLIANT_BODY, head: { sha: 'abc' } }; + const noDiff = '--- UNTRUSTED DIFF ---\n[diff unavailable from the GitHub API]'; + const realDiff = '--- UNTRUSTED DIFF ---\ndiff --git a/src/a.ts b/src/a.ts\n+real'; + expect(hashInputs(pr, noDiff)).toBe(hashInputs(pr, noDiff)); + expect(hashInputs(pr, noDiff)).not.toBe(hashInputs(pr, realDiff)); + // A changed FILE LIST with the same diff is a different payload too. + expect(hashInputs(pr, `added src/b.ts\n${realDiff}`)).not.toBe(hashInputs(pr, realDiff)); + // …and the payload cannot silently drop out: omitting it is its own input. + expect(hashInputs(pr, noDiff)).not.toBe(hashInputs(pr)); }); test('state round-trips through the rendered comment', () => { @@ -1664,26 +1761,63 @@ describe('runGate end-to-end (mocked fetch)', () => { expect(calls.filter((c) => c.url.startsWith('https://api.anthropic.com'))).toHaveLength(3); }, 30_000); - test('spend guard: unchanged title+body+head_sha skips the LLM and keeps the verdict', async () => { - const pr = { title: 'fix(core): a real fix', body: COMPLIANT_BODY, head: { sha: 'cafebabe' } }; - const prior = { - id: 55, - user: { type: 'Bot', login: 'github-actions[bot]' }, - body: renderComment({ - lane: 'close-lane', - verdict: { confidence: 0.9, reasons: ['drive-by refactor'], reviewer_checklist: ['c'] }, - titleCheck: { ok: true }, - flags: [], - state: { hash: hashInputs(pr), lane: 'close-lane' }, - }), - }; - const { calls, fetchImpl } = stubFetch({ comments: [prior] }); // no anthropic handler: any call throws - const code = await runGate(fixtureDir(pr), ENV, fetchImpl); + test('spend guard: an unchanged PR skips the LLM and keeps the verdict', async () => { + // Round-tripped through the gate's OWN state block rather than a hash + // recomputed here: hand-building the expected hash would re-implement + // runGate's payload assembly in the test and pin the test's idea of the + // inputs instead of the gate's. + const dir = fixtureDir({ title: 'fix(core): a real fix', body: COMPLIANT_BODY, head: { sha: 'cafebabe' } }); + const comments: any[] = []; + const first = stubFetch({ + comments, + persistComments: true, + anthropic: () => verdictResponse({ ...CLEAN_VERDICT, lane: 'close-lane', reasons: ['drive-by refactor'] }), + }); + expect(await runGate(dir, ENV, first.fetchImpl)).toBe(1); + expect(comments).toHaveLength(1); + + // Same dir, same everything. No anthropic handler: any call throws. + const { calls, fetchImpl } = stubFetch({ comments }); + const code = await runGate(dir, ENV, fetchImpl); expect(code).toBe(1); // the stored close-lane verdict still holds expect(calls.some((c) => c.url.startsWith('https://api.anthropic.com'))).toBe(false); expect(calls.some((c) => c.method === 'PATCH' || c.method === 'POST')).toBe(false); // nothing rewritten }); + test('spend guard does not serve a diff-blind verdict once the diff is available', async () => { + // The workflow degrades to `[diff unavailable …]` when the GitHub API 406s + // on a huge diff. Run 1 therefore classifies with NO diff. Run 2 has the + // real one: same title, same body, same head sha — only the payload moved, + // and that alone has to buy a second verdict. Otherwise the diff-blind + // verdict is the permanent one. + const comments: any[] = []; + const { calls, fetchImpl } = stubFetch({ + comments, + persistComments: true, + anthropic: () => verdictResponse(CLEAN_VERDICT), + }); + const files = [ + { filename: 'src/a.ts', status: 'modified', additions: 2, deletions: 1 }, + { filename: 'test/a.test.ts', status: 'modified', additions: 5, deletions: 0 }, + ]; + const REAL_DIFF = 'diff --git a/src/a.ts b/src/a.ts\n@@ -1 +1 @@\n-old\n+new\n'; + const anthropicCalls = () => calls.filter((c) => c.url.startsWith('https://api.anthropic.com')).length; + + const unavailable = '[diff unavailable from the GitHub API — too large or unfetchable]\n'; + await runGate(fixtureDir({}, files, unavailable), ENV, fetchImpl); + expect(anthropicCalls()).toBe(1); + expect(comments).toHaveLength(1); // the diff-blind verdict is cached + + await runGate(fixtureDir({}, files, REAL_DIFF), ENV, fetchImpl); + expect(anthropicCalls()).toBe(2); // …and is NOT what run 2 gets served + + // Control: a third run on the SAME payload still short-circuits. The guard + // was fixed, not switched off. + calls.length = 0; + await runGate(fixtureDir({}, files, REAL_DIFF), ENV, fetchImpl); + expect(anthropicCalls()).toBe(0); + }); + // "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