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