admin/gbrain
1
0
Fork 0
mirror of https://github.com/garrytan/gbrain.git synced 2026-08-14 08:53:22 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(ci): gate — sanitize mechanical flag details, anchor path regexes, exempt maintainer/bot/draft from the policy check (blind review round 2)

Browse Source 
Blind review round 2 rejected the branch. Five findings, all fixed.

BLOCKING 1 — the verdict was forgeable through a PR filename.
renderComment wrote mechanical red-flag details raw, and two of them
interpolate filenames (adds_recipe, deletes_tests). git allows a newline
inside a filename and JS `[^/]` matches one, so RECIPE_RE's anchors were
decorative: a PR adding `src/core/ai/recipes/x\n## PR Gate — ...\ncc
@octocat\n<!-- ...state... -->\nz.ts` put that text verbatim into the bot's
comment — forged heading, live third-party mention, and on the NEUTRAL
render (which writes no state block of its own) parseState() returned the
ATTACKER's block, so the next run hit the spend guard and silently skipped
the verdict with no label and exit 0. Three layers:
  (a) flag details go through sanitizeList, exactly like the model's
      strings. Audited every other interpolation into the comment; the rest
      are literals in the file, Number()-coerced, or already sanitized.
  (b) every path regex spells its segment class [^/\n], not [^/] —
      RECIPE_RE, SOURCE_EXT_RE and the test-path check.
  (c) parseState reads the state block only off line 2 of a marker-leading
      comment (where renderComment writes it) and STATE_RE is whole-line
      anchored. A block anywhere else is somebody else's text.

BLOCKING 2 — no exemption, so every release PR was close-lane.
Measured: 40 of the last 40 merged PRs would be close-lane on
missing_screenshot, including every /ship release PR. A check that is red
on every release gets switched off within a week, and then it filters
nothing. The #3745 policy exists to filter INCOMING OUTSIDE CONTRIBUTIONS;
release automation cannot take a screenshot of itself. It is now waived for
OWNER/MEMBER/COLLABORATOR, bot authors and drafts — the usefulness verdict,
the title rule and every mechanical red flag still run, and the sticky
comment says the check was skipped. author_association / draft / user.type
are read from the pr.json the workflow already fetches: no new API call, one
source of truth. `draft` is the one author-settable input, so
ready_for_review joins the trigger list and the exemption is folded into the
spend-guard hash — the draft-era verdict cannot be reused after the flip.
Stated as a deliberate decision in both the workflow header and the script.

3 — DOWNGRADE_FLAG_IDS omitted deletes_tests, adds_symlink and
adds_node_modules, so a PR deleting test/e2e/engine-parity.test.ts kept
merge-lane and a green check on the strength of its prose. All three added.
The old test used deletes_tests as its example of a NON-downgrading flag;
rewritten to pin the stronger invariant instead — every id detectRedFlags
can emit is a downgrade trigger (derived from the detector, so a new flag
fails until it is classified on purpose), and the set is still an allowlist
(an unrecognized id changes nothing).

4 — FENCE_RE backtracks superlinearly on a hostile body: 65KB of backticks
(GitHub's max body length) measured 8.2s across the two policy scans on a
pull_request_target runner. stripCodeFences now caps the scan at 16KB —
same input, 0.40s. Tradeoff documented at the constant: the intent
paragraph and the screenshot both sit near the top in practice (the PR
template puts them in the first two sections, and the model payload already
caps the same body at 6KB), so a real contributor is not judged on a
truncated tail.

Verified: test/pr-gate-workflow.test.ts 126 pass / 0 fail (was 95),
typecheck clean, actionlint clean, check-privacy /
check-no-tracked-symlinks / check-progress-to-stdout /
check-bun-test-timeout / check-key-files-current-state all exit 0. Each new
pin was mutation-tested against the pre-fix behavior: all six mutants fail
the suite.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
2026-08-04 10:46:09 +08:00
committed by Sina Matian
co-authored by Claude Fable 5
parent 4a00c31b12
commit 88731d8cf3
4 changed files with 486 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
scripts/pr-gate.d.mts
+14 -5
View File
@@ -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;
scripts/pr-gate.mjs
+134 -15
View File
@@ -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 = '<!-- gbrain-pr-gate -->';
const STATE_PREFIX = '<!-- gbrain-pr-gate-state ';
const STATE_RE = /<!-- gbrain-pr-gate-state (\{[^\n]*?\}) -->/;
// Whole-line anchored: the block is only ever read off line 2 (see parseState).
const STATE_RE = /^<!-- gbrain-pr-gate-state (\{[^\n]*?\}) -->$/;
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<anything>\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(
`<sub>Policy check skipped: ${sanitizeModelText(policyExempt)} — the CONTRIBUTING.md (#3745) intent-paragraph + screenshot requirement is for incoming outside contributions. Everything else below still ran.</sub>`,
'',
);
}
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(
'',
'<sub>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.</sub>',
@@ -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;
}