diff --git a/.github/workflows/pr-gate.yml b/.github/workflows/pr-gate.yml index 8810cd75f..399d6da0b 100644 --- a/.github/workflows/pr-gate.yml +++ b/.github/workflows/pr-gate.yml @@ -23,11 +23,30 @@ name: PR Gate # green, never a red X for a missing secret, never a stale verdict. A model # REFUSAL is not a skip: it routes to needs-maintainer so refusing is not a # way to dodge the gate. +# +# #3745 EXEMPTION (deliberate — mirrors policyExemption() in +# scripts/pr-gate.mjs): the intent-paragraph + screenshot requirement filters +# INCOMING OUTSIDE CONTRIBUTIONS. It is waived for repo owners / members / +# collaborators, bot authors, and drafts. +# Release automation cannot take a screenshot of itself, and without the +# exemption every /ship release PR lands in close-lane +# (measured: 40 of the last 40 merged PRs) — a check that is red +# on every release gets switched off within a week, and then it filters +# nothing. Exempt PRs still get the FULL usefulness verdict, the title rule and +# every mechanical red flag; only the description requirement is skipped, and +# the sticky comment says so on its own line. +# author_association / draft / user.type are read from the pr.json fetched +# below — GitHub-computed, not author-settable (except `draft`), and already +# on disk, so nothing new is fetched and there is one source of truth. +# `ready_for_review` is in the trigger list precisely because `draft` IS +# author-settable: leaving draft re-runs the gate with the exemption gone, and +# the exemption is folded into the spend-guard hash so the draft-era verdict +# cannot be reused. # Pinned by test/pr-gate-workflow.test.ts. on: pull_request_target: - types: [opened, edited, synchronize, reopened] + types: [opened, edited, synchronize, reopened, ready_for_review] branches: [master] # issues:write is the ONLY write grant. Everything the script calls is the diff --git a/scripts/pr-gate.d.mts b/scripts/pr-gate.d.mts index fd048b60c..0d56f71c1 100644 --- a/scripts/pr-gate.d.mts +++ b/scripts/pr-gate.d.mts @@ -32,6 +32,8 @@ export declare const DOWNGRADE_FLAG_IDS: string[]; export declare const CONTRIBUTING_URL: string; export declare const INTENT_MIN_WORDS: number; export declare const POLICY_FLAG_IDS: string[]; +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 function hasScreenshot(body: unknown): boolean; @@ -48,6 +50,14 @@ export declare function applyMechanicalDowngrades( intentAuthenticity?: string, ): { lane: string; downgrades: string[] }; +/** The pr.json fields the #3745 exemption reads (all GitHub-computed). */ +export interface PrIdentity { + author_association?: string; + draft?: boolean; + user?: { type?: string; login?: string }; +} +export declare function policyExemption(pr: PrIdentity | null | undefined): string | null; + export interface GhComment { id?: number; body?: unknown; @@ -55,11 +65,9 @@ export interface GhComment { } export declare function isOwnComment(comment: GhComment | null | undefined): boolean; -export declare function hashInputs(pr: { - title?: string; - body?: string; - head?: { sha?: string }; -}): string; +export declare function hashInputs( + pr: PrIdentity & { title?: string; body?: string; head?: { sha?: string } }, +): string; export declare function parseState(body: unknown): { hash: string; lane?: string } | null; export declare function renderComment(input: { @@ -70,6 +78,7 @@ export declare function renderComment(input: { neutralReason?: string; downgrades?: string[]; policyMisses?: RedFlag[]; + policyExempt?: string | null; state?: { hash: string; lane: string }; }): string; diff --git a/scripts/pr-gate.mjs b/scripts/pr-gate.mjs index c81b9caf2..bf7b14b9c 100644 --- a/scripts/pr-gate.mjs +++ b/scripts/pr-gate.mjs @@ -19,9 +19,15 @@ * - Only a comment authored by github-actions[bot] AND starting with the * 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 model-produced string is sanitized before it reaches Markdown - * (no HTML comments, no live @mentions, no block markers, no newlines, - * length- and count-capped). + * - 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. + * - 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 + * cached verdict for the spend guard to reuse. * - The lane is NOT purely model-decided: mechanical signals downgrade a * merge-lane recommendation to needs-maintainer, so a persuasive PR body * cannot talk itself into the fast lane. @@ -48,7 +54,8 @@ import { pathToFileURL } from 'node:url'; const MARKER = ''; const STATE_PREFIX = '/; +// Whole-line anchored: the block is only ever read off line 2 (see parseState). +const STATE_RE = /^$/; const BOT_LOGIN = 'github-actions[bot]'; const MODEL = 'claude-sonnet-5'; const LANES = ['merge-lane', 'close-lane', 'needs-maintainer']; @@ -201,7 +208,28 @@ export const CONTRIBUTING_URL = * 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; -export const stripCodeFences = (body) => String(body ?? '').replace(FENCE_RE, '\n'); + +/** + * 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. + */ +export const POLICY_SCAN_MAX = 16384; +export const stripCodeFences = (body) => + String(body ?? '') + .slice(0, POLICY_SCAN_MAX) + .replace(FENCE_RE, '\n'); const SCREENSHOT_RES = [ /!\[[^\]]*\]\(\s*\S/, // markdown image embed @@ -265,15 +293,60 @@ export function detectPolicyMisses(body) { return misses; } +/** + * #3745 EXEMPTION — who the policy is for. A deliberate decision, not an + * oversight. + * + * The intent paragraph + screenshot exist to filter INCOMING OUTSIDE + * CONTRIBUTIONS: they ask a stranger to show a real situation before a + * maintainer spends review time on their diff. They were never aimed at the + * repo's own traffic. Release automation cannot take a screenshot of itself, + * and /ship writes the description from the CHANGELOG rather than from a + * first-person story — so with no exemption EVERY release PR lands in + * close-lane. Measured on the last 40 merged PRs: 40 of 40 would be + * close-lane on missing_screenshot. A check that is red on every release is a + * check somebody disables inside a week, and then it protects nobody. + * + * Exempt: repo owners / members / collaborators, bot authors, and drafts (a + * draft is explicitly work in progress; its description is expected to be + * unfinished, and `ready_for_review` re-runs the gate with the exemption gone + * — the exemption is folded into hashInputs so the spend guard cannot serve + * the draft-era verdict afterwards). + * + * Waives the intent/screenshot requirement ONLY. An exempt PR still gets the + * full usefulness verdict, the title rule, and every mechanical red flag — + * including the downgrades that keep a maintainer's own merge-lane honest. + * + * author_association and user.type are computed by GitHub, not settable by the + * author. `draft` IS author-settable, which is why the ready_for_review + * trigger and the hash both exist. + */ +export const POLICY_EXEMPT_ASSOCIATIONS = ['OWNER', 'MEMBER', 'COLLABORATOR']; + +export function policyExemption(pr) { + const assoc = String(pr?.author_association ?? '').toUpperCase(); + if (POLICY_EXEMPT_ASSOCIATIONS.includes(assoc)) return `maintainer (${assoc.toLowerCase()})`; + if (pr?.user?.type === 'Bot') return 'bot author'; + if (pr?.draft === true) return 'draft PR'; + return null; +} + // --------------------------------------------------------------------------- // Mechanical red flags (no LLM). // --------------------------------------------------------------------------- -const SOURCE_EXT_RE = /\.(ts|tsx|js|jsx|mjs|cjs|sql|py|sh)$/; -const RECIPE_RE = /^src\/core\/ai\/recipes\/[^/]+\.(ts|mts|js|mjs)$/; +// Every path regex spells its "one path segment" class as [^/\n], never [^/]. +// git allows a newline inside a filename, and JS `.`/`[^/]` both match one, so +// `[^/]+` lets `recipes/x\n\nz.ts` satisfy an anchored pattern — the +// pattern looks single-line but is not. The detail strings built from these +// matches are rendered into a public comment, so a smuggled newline is a +// smuggled Markdown line. (Rendering is sanitized too; this is the second +// layer, and it also keeps the CLASSIFICATION honest.) +const SOURCE_EXT_RE = /(^|\/)[^/\n]*\.(ts|tsx|js|jsx|mjs|cjs|sql|py|sh)$/; +const RECIPE_RE = /^src\/core\/ai\/recipes\/[^/\n]+\.(ts|mts|js|mjs)$/; export const NET_SOURCE_LINE_LIMIT = 400; function isTestFile(path) { - return /(^|\/)test\//.test(path) || /\.(test|spec)\.(ts|tsx|js|mjs|cjs)$/.test(path); + return /(^|\/)test\//.test(path) || /(^|\/)[^/\n]*\.(test|spec)\.(ts|tsx|js|mjs|cjs)$/.test(path); } function addedDependency(files) { @@ -373,6 +446,11 @@ export function detectRedFlags({ changedFiles, files, diff }) { // signals decide. A merge-lane recommendation carrying any of them becomes // needs-maintainer no matter how convincing the PR body was. // --------------------------------------------------------------------------- +// Currently every id detectRedFlags can emit — pinned by a test, so a NEW red +// flag has to be listed here (or deliberately excluded) rather than defaulting +// to "advisory". `deletes_tests`, `adds_symlink` and `adds_node_modules` were +// the omissions: a PR deleting test/e2e/engine-parity.test.ts kept merge-lane +// and a green check as long as the body read well. export const DOWNGRADE_FLAG_IDS = [ 'modifies_workflows', 'adds_dependency', @@ -381,6 +459,9 @@ export const DOWNGRADE_FLAG_IDS = [ 'too_many_files', 'large_source_addition', 'no_test_for_src_change', + 'deletes_tests', + 'adds_symlink', + 'adds_node_modules', ]; /** @@ -589,15 +670,29 @@ async function setLaneLabel(gh, repo, prNumber, lane) { // title or body can forge a boundary, and the tuple order is fixed by the // literal. Literal NUL bytes did the same job but made the whole file "binary" // to grep, which silently defeats any grep-based CI guard over it. +// 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. export function hashInputs(pr) { return createHash('sha256') - .update(JSON.stringify([pr.title ?? '', pr.body ?? '', pr.head?.sha ?? ''])) + .update(JSON.stringify([pr.title ?? '', pr.body ?? '', pr.head?.sha ?? '', policyExemption(pr) ?? ''])) .digest('hex') .slice(0, 16); } +/** + * Read the state block the BOT wrote, and only that one. renderComment emits it + * on line 2, immediately after the marker, so that is the only place we look. A + * global search would also match a block sitting in attacker-controlled text + * further down the comment (a PR filename can contain newlines), which is a + * forged verdict handed straight to the spend guard: the next run would see + * "unchanged inputs, lane already decided" and skip the real verdict. A render + * with no state of its own therefore yields null even when hostile text is + * present. + */ export function parseState(body) { - const m = typeof body === 'string' ? body.match(STATE_RE) : null; + if (typeof body !== 'string' || !body.startsWith(MARKER)) return null; + const m = STATE_RE.exec(body.split('\n')[1] ?? ''); if (!m) return null; try { const state = JSON.parse(m[1]); @@ -639,6 +734,7 @@ export function renderComment({ neutralReason, downgrades = [], policyMisses = [], + policyExempt = null, state, }) { const lines = [MARKER]; @@ -647,7 +743,9 @@ export function renderComment({ if (neutralReason) { lines.push('## PR Gate — NEUTRAL (skipped)', '', `**Reason:** ${sanitizeModelText(neutralReason)}`, ''); 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 passed — a miss there is close-lane whether or not the model is reachable.', + `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 ${ + policyExempt ? 'was skipped for this author' : 'passed' + } — a miss there is close-lane whether or not the model is reachable.`, '', ); } else { @@ -671,12 +769,21 @@ export function renderComment({ // Policy misses already have two sections of their own; a third copy here // just reads as the machine repeating itself at a first-time contributor. const redFlags = flags.filter((f) => !POLICY_FLAG_IDS.includes(f.id)); + if (policyExempt) { + lines.push( + `Policy check skipped: ${sanitizeModelText(policyExempt)} — the CONTRIBUTING.md (#3745) intent-paragraph + screenshot requirement is for incoming outside contributions. Everything else below still ran.`, + '', + ); + } lines.push( `**Title (version-first rule):** ${titleCheck.ok ? '✅ ok' : `❌ ${titleCheck.reason}`}`, '', `**Mechanical red flags:** ${redFlags.length ? '' : 'none'}`, ); - for (const f of redFlags) lines.push(`- ${f.detail}`); + // Sanitized exactly like the model's strings: adds_recipe and deletes_tests + // interpolate PR filenames, and a filename can carry a newline, an @mention + // or an HTML comment straight into this comment. + for (const d of sanitizeList(redFlags.map((f) => f.detail))) lines.push(`- ${d}`); 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.', @@ -698,13 +805,24 @@ export async function runGate(dir, env = process.env, fetchImpl = fetch) { const gh = ghClient(env, fetchImpl); const titleCheck = checkTitle(pr.title ?? ''); - const policyMisses = detectPolicyMisses(pr.body); + // See policyExemption: #3745 filters incoming outside contributions, so a + // maintainer, a bot or a draft is judged on everything EXCEPT the intent + // paragraph + screenshot. author_association / draft / user.type all come + // from the pr.json the workflow already fetched — no extra API call. + const policyExempt = policyExemption(pr); + const policyMisses = policyExempt ? [] : detectPolicyMisses(pr.body); const flags = [...detectRedFlags({ changedFiles: pr.changed_files ?? files.length, files, diff }), ...policyMisses]; const existing = await findOwnComment(gh, repo, prNumber); const neutral = async (reason) => { console.log(`::warning::PR gate NEUTRAL-skip: ${reason}`); - await upsertStickyComment(gh, repo, prNumber, existing, renderComment({ titleCheck, flags, neutralReason: reason })); + await upsertStickyComment( + gh, + repo, + prNumber, + existing, + renderComment({ titleCheck, flags, policyExempt, neutralReason: reason }), + ); await setLaneLabel(gh, repo, prNumber, null); // no stale verdict survives a skip return 0; }; @@ -779,6 +897,7 @@ export async function runGate(dir, env = process.env, fetchImpl = fetch) { flags, downgrades, policyMisses, + policyExempt, state: { hash: inputHash, lane }, }); await upsertStickyComment(gh, repo, prNumber, existing, body); @@ -787,7 +906,7 @@ export async function runGate(dir, env = process.env, fetchImpl = fetch) { console.log( `PR gate verdict: ${lane} (confidence ${verdict.confidence}${degraded ? ', degraded' : ''}${ downgrades.length ? `, ${downgrades.length} mechanical downgrade(s)` : '' - })`, + }${policyExempt ? `, #3745 policy check skipped: ${policyExempt}` : ''})`, ); return lane === 'close-lane' ? 1 : 0; } diff --git a/test/pr-gate-workflow.test.ts b/test/pr-gate-workflow.test.ts index 3a90247bc..13e545317 100644 --- a/test/pr-gate-workflow.test.ts +++ b/test/pr-gate-workflow.test.ts @@ -35,15 +35,18 @@ import { sanitizeModelText, sanitizeList, applyMechanicalDowngrades, + policyExemption, isOwnComment, hashInputs, parseState, renderComment, runGate, CONTRIBUTING_URL, + DOWNGRADE_FLAG_IDS, INTENT_MIN_WORDS, MAX_ITEMS, MAX_STRING, + POLICY_SCAN_MAX, } from '../scripts/pr-gate.mjs'; const WORKFLOW_PATH = join(import.meta.dir, '..', '.github', 'workflows', 'pr-gate.yml'); @@ -164,9 +167,11 @@ describe('pr-gate workflow security pins', () => { } }); - test('triggers on pull_request_target (opened/edited/synchronize/reopened) against master', () => { + test('triggers on pull_request_target against master, ready_for_review included', () => { expect(WORKFLOW).toContain('pull_request_target:'); - expect(WORKFLOW).toMatch(/types:\s*\[opened, edited, synchronize, reopened\]/); + // ready_for_review is load-bearing: drafts are exempt from the #3745 + // policy check, so leaving draft has to re-run the gate without it. + expect(WORKFLOW).toMatch(/types:\s*\[opened, edited, synchronize, reopened, ready_for_review\]/); expect(WORKFLOW).toMatch(/branches:\s*\[master\]/); // Not the unsafe habit of also running plain pull_request with secrets. expect(WORKFLOW).not.toMatch(/^\s*pull_request:\s*$/m); @@ -186,6 +191,19 @@ describe('pr-gate workflow security pins', () => { test('workflow invokes the gate script from the base checkout', () => { expect(WORKFLOW).toContain('node scripts/pr-gate.mjs'); }); + + test('the #3745 exemption is documented as a decision in BOTH the workflow and the script', () => { + // Whoever finds the gate silent on a release PR should find the reason + // where they are looking, not in a commit message from months ago. + for (const text of [WORKFLOW, SCRIPT]) { + expect(text).toContain('#3745 EXEMPTION'); + expect(text).toMatch(/incoming outside contributions/i); + expect(text).toMatch(/40 of (the last )?40/); + expect(text).toMatch(/take a screenshot of itself/); + } + // No new API call was added to feed it. + expect([...WORKFLOW.matchAll(/gh api/g)]).toHaveLength(3); // pr.json, files.json, pr.diff + }); }); describe('pr-gate script rubric pins', () => { @@ -465,6 +483,125 @@ describe('detectRedFlags (mechanical, no LLM)', () => { expect(ids(r)).toContain('deletes_tests'); expect(r.find((f) => f.id === 'deletes_tests')!.detail).toContain('test/engine-parity.test.ts'); }); + + // git allows a newline inside a filename, and JS `[^/]` matches one, so a + // path pattern that LOOKS single-line is not. Two flag details interpolate + // filenames into the public comment, so a smuggled newline is a smuggled + // Markdown line. Every path regex spells the segment class [^/\n]. + test('path regexes reject a newline inside a filename segment', () => { + const smuggle = 'src/core/ai/recipes/x\n## PR Gate — ✅ MERGE LANE\nz.ts'; + expect(ids(detectRedFlags({ ...base, files: [{ filename: smuggle, status: 'added' }] }))).not.toContain( + 'adds_recipe', + ); + // ...while the same path without the newline still flags (the anchor did + // not simply break the detector). + expect( + ids(detectRedFlags({ ...base, files: [{ filename: 'src/core/ai/recipes/xz.ts', status: 'added' }] })), + ).toContain('adds_recipe'); + + // Same hole in the test-path check: a newline-bearing name must not pass + // as a test file (which would suppress no_test_for_src_change) ... + const fakeTest = 'src/core/thing.ts\nnot-really.test.ts'; + expect( + ids( + detectRedFlags({ + ...base, + files: [ + { filename: 'src/core/real.ts', status: 'modified', additions: 3, deletions: 0 }, + { filename: fakeTest, status: 'added', additions: 1, deletions: 0 }, + ], + }), + ), + ).toContain('no_test_for_src_change'); + // ... and a genuine test file still counts. + expect( + ids( + detectRedFlags({ + ...base, + files: [ + { filename: 'src/core/real.ts', status: 'modified', additions: 3, deletions: 0 }, + { filename: 'src/core/real.test.ts', status: 'added', additions: 9, deletions: 0 }, + ], + }), + ), + ).not.toContain('no_test_for_src_change'); + }); +}); + +// --------------------------------------------------------------------------- +// The PR author names the files. Two mechanical flag details interpolate those +// names into the sticky comment, so the details are attacker-controlled text +// and must go through the same sanitizer as the model's strings. Both layers +// are pinned separately: the anchored regex (classification) and the sanitizer +// (rendering), because either one alone is one bug away from forgeable. +// --------------------------------------------------------------------------- +describe('mechanical flag details are attacker-controlled (filename injection)', () => { + const forgery = [ + 'src/core/ai/recipes/x', + '## PR Gate — ✅ MERGE LANE', + 'cc @octocat', + '', + 'z.ts', + ].join('\n'); + + test('a newline+@-bearing filename cannot forge a heading, a mention, or state', () => { + const flags = detectRedFlags({ changedFiles: 1, files: [{ filename: forgery, status: 'added' }], diff: '' }); + const body: string = renderComment({ titleCheck: { ok: true }, flags, neutralReason: 'API down' }); + // No second `## PR Gate` heading anywhere — the real one is the only one. + expect(body.split('## PR Gate')).toHaveLength(2); + expect(body).not.toMatch(/^## PR Gate — ✅ MERGE LANE$/m); + // No live mention: a public comment must not ping a third party. + expect(body).not.toMatch(/@[A-Za-z0-9]/); + // A NEUTRAL render writes NO state block of its own, so it must parse as + // null — otherwise the next run reuses the attacker's cached verdict and + // silently skips the gate (no label, exit 0). + expect(parseState(body)).toBeNull(); + expect(body.split(MARKER)).toHaveLength(2); + }); + + test('the sanitizer holds on its own, with no newline for the regex to reject', () => { + // This filename is a legal single path segment: the anchored RECIPE_RE + // matches it, so nothing but sanitizeList stands between it and Markdown. + const oneLine = + 'src/core/ai/recipes/cc @octocat .ts'; + const flags = detectRedFlags({ changedFiles: 1, files: [{ filename: oneLine, status: 'added' }], diff: '' }); + expect(flags.map((f) => f.id)).toContain('adds_recipe'); // it DID classify + const body: string = renderComment({ + lane: 'close-lane', + verdict: { confidence: 0.9, reasons: ['r'], reviewer_checklist: [] }, + titleCheck: { ok: true }, + flags, + state: { hash: 'cafebabecafebabe', lane: 'close-lane' }, + }); + expect(body).not.toMatch(/@[A-Za-z0-9]/); + expect(body).not.toContain('.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(/@[A-Za-z0-9]/); + expect(body).not.toContain(''); + }); + + test('parseState only reads line 2 of a comment the bot wrote', () => { + const state = ''; + // Right shape, wrong place: anywhere but line 2 is somebody else's text. + expect(parseState(`${MARKER}\n\nsome verdict\n${state}\n`)).toBeNull(); + expect(parseState(`${state}\n${MARKER}`)).toBeNull(); // no leading marker + expect(parseState(`${MARKER}\nprefix ${state}`)).toBeNull(); // not the whole line + // Line 2 of a marker-leading comment is ours. + expect(parseState(`${MARKER}\n${state}\n\nverdict`)).toEqual({ + hash: 'deadbeefdeadbeef', + lane: 'merge-lane', + }); + }); }); describe('hasScreenshot (#3745, mechanical)', () => { @@ -568,6 +705,73 @@ describe('detectPolicyMisses (#3745)', () => { expect(f.detail).toContain('#3745'); } }); + + // The body is attacker-supplied on a pull_request_target runner and the + // fence regex backtracks superlinearly on a wall of backticks: 65KB (GitHub's + // max body length) cost ~8s across the two policy scans before the cap. + test('a hostile all-backticks body is bounded, not superlinear', () => { + const t0 = performance.now(); + detectPolicyMisses('`'.repeat(65536)); + const ms = performance.now() - t0; + // ~0.4s locally, ~8s uncapped. 3s leaves room for a slow CI runner while + // still failing loudly if the cap is ever removed. + expect(ms).toBeLessThan(3000); + }); + + test('the cap cannot false-negative a legitimate long description', () => { + // The intent paragraph and the screenshot both sit near the top in + // practice, so a real body stays compliant however long its tail is. + const longTail = `${COMPLIANT_BODY}\n${'more detail about the change. '.repeat(2000)}`; + expect(longTail.length).toBeGreaterThan(POLICY_SCAN_MAX); + expect(detectPolicyMisses(longTail)).toEqual([]); + // The documented tradeoff, pinned so it is a decision and not a surprise: + // a body that hides BOTH past the cap is judged on the truncated text. + const buried = `${'x '.repeat(POLICY_SCAN_MAX)}\n\n${COMPLIANT_BODY}`; + expect(detectPolicyMisses(buried).map((f) => f.id)).toEqual(['missing_screenshot']); + }); +}); + +// --------------------------------------------------------------------------- +// The #3745 exemption. Without it the check is red on every release PR +// (measured: 40 of the last 40 merged PRs would be close-lane on +// missing_screenshot), and a check that is always red gets switched off. +// --------------------------------------------------------------------------- +describe('policyExemption (#3745 is for incoming outside contributions)', () => { + test.each(['OWNER', 'MEMBER', 'COLLABORATOR'])('%s is exempt', (assoc) => { + expect(policyExemption({ author_association: assoc })).toContain('maintainer'); + }); + + test('bot authors and drafts are exempt', () => { + expect(policyExemption({ user: { type: 'Bot', login: 'github-actions[bot]' } })).toBe('bot author'); + expect(policyExemption({ draft: true })).toBe('draft PR'); + }); + + test.each(['CONTRIBUTOR', 'FIRST_TIME_CONTRIBUTOR', 'FIRST_TIMER', 'NONE', 'MANNEQUIN', ''])( + 'an outside contributor (%s) is NOT exempt', + (assoc) => { + expect(policyExemption({ author_association: assoc, user: { type: 'User' }, draft: false })).toBeNull(); + }, + ); + + test('nothing about the PR being absent grants an exemption', () => { + expect(policyExemption({})).toBeNull(); + expect(policyExemption(null)).toBeNull(); + // pr.json is JSON.parse'd off disk, so the values are whatever the file + // says. `draft` is matched === true, not truthily. + expect(policyExemption(JSON.parse('{"draft":"true"}'))).toBeNull(); + expect(policyExemption({ user: { type: 'User', login: 'bot' } })).toBeNull(); // login is not type + }); + + test('the exemption is part of the spend-guard hash', () => { + // Marking a draft ready-for-review changes neither title, body nor head + // sha. Without the exemption in the hash the gate would keep serving the + // verdict it computed while the policy check was waived. + const pr = { title: 't', body: 'b', head: { sha: 'abc' } }; + expect(hashInputs({ ...pr, draft: true })).not.toBe(hashInputs({ ...pr, draft: false })); + expect(hashInputs({ ...pr, author_association: 'OWNER' })).not.toBe( + hashInputs({ ...pr, author_association: 'CONTRIBUTOR' }), + ); + }); }); describe('CONTRIBUTING.md deep link (#3745)', () => { @@ -614,14 +818,60 @@ describe('applyMechanicalDowngrades (lane is not purely model-decided)', () => { 'too_many_files', 'large_source_addition', 'no_test_for_src_change', + 'deletes_tests', + 'adds_symlink', + 'adds_node_modules', ])('merge-lane + %s downgrades to needs-maintainer', (id) => { const r = applyMechanicalDowngrades('merge-lane', [flag(id)]); expect(r.lane).toBe('needs-maintainer'); expect(r.downgrades).toEqual([`detail for ${id}`]); }); - test('merge-lane with only non-downgrade flags stays merge-lane', () => { - expect(applyMechanicalDowngrades('merge-lane', [flag('deletes_tests')]).lane).toBe('merge-lane'); + // The stronger invariant, and the one that was broken: deletes_tests, + // adds_symlink and adds_node_modules were detected but not in the downgrade + // set, so a PR deleting test/e2e/engine-parity.test.ts kept merge-lane and a + // green check on the strength of its prose. Derived from the detector rather + // than a hand-copied list, so a NEW red flag fails here until it is + // classified on purpose. + test('every id detectRedFlags can emit is a downgrade trigger', () => { + const everything = detectRedFlags({ + changedFiles: 99, + files: [ + { filename: 'node_modules/left-pad/index.js', status: 'added' }, + { filename: '.github/workflows/x.yml', status: 'modified' }, + { filename: 'package.json', status: 'modified', patch: '@@\n+ "left-pad": "^1.3.0",' }, + { filename: 'src/core/ai/recipes/acme-example.ts', status: 'added' }, + { filename: 'src/core/config.ts', status: 'modified', patch: "@@\n+ 'acme_example_key'," }, + { filename: 'src/core/big.ts', status: 'added', additions: 900, deletions: 0 }, + { filename: 'test/gone.test.ts', status: 'removed' }, + ], + diff: 'new file mode 120000\n', + }); + const emitted = everything.map((f) => f.id); + // The fixture really does trip every branch — otherwise this pins nothing. + expect(emitted.sort()).toEqual( + [ + 'adds_config_keys', + 'adds_dependency', + 'adds_node_modules', + 'adds_recipe', + 'adds_symlink', + 'deletes_tests', + 'large_source_addition', + 'modifies_workflows', + 'too_many_files', + ].sort(), + ); + for (const id of emitted) expect(DOWNGRADE_FLAG_IDS).toContain(id); + // no_test_for_src_change is the one branch the fixture above cannot reach + // at the same time (it needs src/ WITHOUT a test file). + expect(DOWNGRADE_FLAG_IDS).toContain('no_test_for_src_change'); + }); + + test('the downgrade set is an allowlist — an unrecognized flag id changes nothing', () => { + // Not "any flag downgrades": a future advisory-only flag must be added to + // DOWNGRADE_FLAG_IDS deliberately, not inherit the behavior. + expect(applyMechanicalDowngrades('merge-lane', [flag('some_future_advisory_flag')]).lane).toBe('merge-lane'); expect(applyMechanicalDowngrades('merge-lane', []).lane).toBe('merge-lane'); }); @@ -1198,6 +1448,70 @@ describe('runGate — CONTRIBUTING.md #3745 policy (mocked fetch)', () => { expect(body).not.toContain('Almost there'); // nothing to fix in the description }); + // A maintainer's release PR has no first-person paragraph and cannot + // screenshot itself. Every one of them being close-lane is how this check + // gets disabled, so the exemption is load-bearing for the check surviving. + const RELEASE_PR_BODY = '## What changed\n\n- v0.42.70.0 fix: three things\n'; + + test.each([ + ['a maintainer', { author_association: 'OWNER' }], + ['an org member', { author_association: 'MEMBER' }], + ['a collaborator', { author_association: 'COLLABORATOR' }], + ['a bot', { user: { type: 'Bot', login: 'github-actions[bot]' } }], + ['a draft', { draft: true }], + ])('%s release PR with no intent paragraph or screenshot is judged normally, not closed', async (_who, who) => { + const { calls, fetchImpl } = stubFetch({ anthropic: () => verdictResponse(CLEAN_VERDICT) }); + const code = await runGate( + fixtureDir({ title: 'v0.42.70.0 fix: three things', body: RELEASE_PR_BODY, ...who }, SRC_AND_TEST), + ENV, + fetchImpl, + ); + expect(code).toBe(0); + expect(addedLabels(calls)).toEqual(['gate:merge-lane']); + const body: string = postedBody(calls); + expect(body).not.toContain('Almost there'); // not the fix-your-description comment + expect(body).toContain('Policy check skipped'); // ...and it says so out loud + expect(body).toContain('MERGE LANE'); + }); + + test('the waiver is the description requirement ONLY — mechanical checks still bite', async () => { + const { calls, fetchImpl } = stubFetch({ anthropic: () => verdictResponse(CLEAN_VERDICT) }); + const code = await runGate( + // Maintainer, no screenshot, but a src change with no test: the #3665 + // downgrade applies to the maintainer exactly as to anyone else. + fixtureDir({ title: 'Update README.md', body: RELEASE_PR_BODY, author_association: 'OWNER' }, [ + { filename: 'src/core/thing.ts', status: 'modified', additions: 12, deletions: 0 }, + ]), + ENV, + fetchImpl, + ); + expect(code).toBe(0); + expect(addedLabels(calls)).toEqual(['gate:needs-maintainer']); + const body: string = postedBody(calls); + expect(body).toContain('Mechanical downgrades applied'); + expect(body).toContain('#3665'); + expect(body).toContain('neither version-first'); // the title rule still ran + expect(body).toContain('Policy check skipped'); + }); + + test('an outside contributor with the same description is still closed', async () => { + // The control for every exemption case above: same body, no exemption. + const { calls, fetchImpl } = stubFetch({}); + const code = await runGate( + fixtureDir( + { title: 'v0.42.70.0 fix: three things', body: RELEASE_PR_BODY, author_association: 'CONTRIBUTOR' }, + SRC_AND_TEST, + ), + ENV, + fetchImpl, + ); + expect(code).toBe(1); + expect(addedLabels(calls)).toEqual(['gate:close-lane']); + const body: string = postedBody(calls); + expect(body).toContain('Almost there'); + expect(body).not.toContain('Policy check skipped'); + }); + test('a human / unclear intent verdict leaves the lane alone', async () => { for (const intent of ['human', 'unclear']) { const { calls, fetchImpl } = stubFetch({