test(quality): fail loudly when a source-scanning guard is negative-only (#8619)

* test(quality): fail loudly when a source-scanning guard is negative-only

A negative guard — assert.doesNotMatch(src, /x/) or src.includes(x) === false —
passes against an empty string. Once the code it guards is extracted into another
file the parent no longer contains the string, so the assertion keeps passing while
protecting nothing. The regression coverage is deleted with no test turning red,
which is exactly the failure mode the god-file decomposition campaign (#8617) is
about to trigger 90-odd times.

Adds tests/unit/source-scanner-guards.test.ts: a hard gate (no baseline, no
allowlist) requiring every test variable bound to project source to carry at least
one positive anchor. Classification runs on logical statements with strings, regexes
and comments blanked out, so a guard wrapped across lines cannot slip past — that
folding is what exposed 3 of the 7 violations.

Fixes all 7 violations across 6 files with one stable top-level export anchor each.
Two were security scope guards held only by multi-line negative assertions: the SSRF
guards on /api/sync/initialize (#323) and the proxy-bypass guards on chatHelpers.ts
and chatCore.ts (#3226) — the latter anchored on handleChatCore precisely because
that file is a decomposition target.

Adds tests/_helpers/readSrc.ts, a repo-root-relative reader that throws on a missing
or empty file instead of returning "".

Refs #8617

* docs(changelog): number the fragment for #8619

* chore(skills): sync cli-backup-sync SKILL.md with catalog

Same tip fix as #8657 so Merge integrity is green without waiting for
that PR to land. Regenerated via generate-agent-skills --apply.
This commit is contained in:
MumuTW
2026-07-28 10:57:30 +08:00
committed by GitHub
parent 67d13dc7a8
commit b4776a2d84
9 changed files with 503 additions and 8 deletions

View File

@@ -0,0 +1 @@
- **Source-scanner negative guards** (`tests/unit/source-scanner-guards.test.ts`): hard gate (no baseline, no allowlist) that fails any test variable holding project source read as text whose usages are _all_ negative guards (`assert.doesNotMatch` / `.includes(…) === false`). Those assertions pass against an empty string, so extracting the guarded code into a new file keeps them green while they protect nothing — silently deleting regression coverage during the file-splitting campaign. Classification runs on logical statements with strings, regexes, and comments blanked out, so a guard wrapped across lines cannot slip past. Fixes the 7 existing violations across 6 files with one positive anchor each; two are security scope guards that were held only by multi-line negative assertions — the SSRF guards on `/api/sync/initialize` (`providers-autosync-ssrf-323`) and the proxy-bypass guards on `chatHelpers.ts` / `chatCore.ts` (`proxy-bypass-scope-guard-3226`), the latter pinned to `handleChatCore` precisely because that file is a decomposition target. Adds `tests/_helpers/readSrc.ts`, a repo-root-relative reader that throws on a missing or empty file instead of returning `""`. Regression guards: two synthetic multi-line cases in the gate's own suite. First step of the god-file decomposition campaign tracked in #8617.

57
tests/_helpers/readSrc.ts Normal file
View File

@@ -0,0 +1,57 @@
// Shared reader for tests that assert against project source read as text.
//
// Why this throws instead of returning "" on a missing/empty file:
// static source scanners frequently carry NEGATIVE guards, e.g.
// assert.equal(src.includes(">Active Endpoints<"), false)
// A negative guard passes trivially against an empty string. So if the source is
// moved, renamed, or split into a new component file, a silently-empty read keeps
// the assertion green while it guards nothing at all — the regression coverage is
// deleted without a single test turning red. Failing loudly at read time makes
// that class of silent coverage loss impossible.
//
// The companion hard gate is tests/unit/source-scanner-guards.test.ts, which
// requires every source-bound variable to carry at least one positive anchor.
import { readFileSync } from "node:fs";
import { dirname, isAbsolute, join, resolve } from "node:path";
import { fileURLToPath } from "node:url";
/**
* Repo root, derived from this file's location (tests/_helpers/) — never from cwd, so
* the reader behaves identically however the test runner was invoked.
*/
const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", "..");
/**
* Read a project source file as UTF-8 text, relative to the repository root.
*
* @param relPath Repo-relative path, e.g. "open-sse/executors/claude-web.ts".
* @throws If the file does not exist or its content is empty/whitespace-only.
*/
export function readSrc(relPath: string): string {
const absPath = isAbsolute(relPath) ? relPath : join(REPO_ROOT, relPath);
let content: string;
try {
content = readFileSync(absPath, "utf8");
} catch (err) {
const reason = err instanceof Error ? err.message : String(err);
throw new Error(
`readSrc("${relPath}"): cannot read source file at ${absPath}. ` +
`The file was probably moved, renamed, or split — update the test to point at ` +
`its new location instead of letting the assertions guard nothing. (${reason})`
);
}
if (!content.trim()) {
throw new Error(
`readSrc("${relPath}"): source file at ${absPath} is empty or whitespace-only. ` +
`Negative guards (assert.doesNotMatch / .includes(...) === false) pass trivially ` +
`against empty content, so an empty read would silently disable this test.`
);
}
return content;
}
/** The repository root this helper resolves against. Exposed for tests that need it. */
export { REPO_ROOT };

View File

@@ -402,6 +402,9 @@ describe("Claude Web executor transport orchestration", () => {
"utf8"
);
// Anchor: proves the read still resolved to the real executor module, so the
// negative guard below cannot pass against a moved/emptied file.
assert.match(executorSource, /export class ClaudeWebExecutor extends BaseExecutor/);
assert.doesNotMatch(executorSource, /claudeTurnstileSolver|getCfClearanceToken|tryBackedChat/);
assert.doesNotMatch(indexSource, /ClaudeWebWithAutoRefresh/);
assert.match(indexSource, /"claude-web": new ClaudeWebExecutor\(\)/);

View File

@@ -183,6 +183,9 @@ test("production audit regressions stay localized and provider icons stay bounde
}
const endpoint = readSource("src/app/(dashboard)/dashboard/endpoint/EndpointPageClient.tsx");
// Anchor: the page component itself, so the raw-copy guards below cannot pass
// against a file that was moved, renamed, or split apart.
assert.match(endpoint, /export default function APIPageClient\(/);
for (const rawText of [
'label: "Context Sources"',
">Active Endpoints<",

View File

@@ -33,6 +33,9 @@ test("profile renders mapped icons and localized badge criteria", () => {
});
test("token page no longer contains known raw English controls", () => {
// Anchor: the page component itself, so the raw-copy guards below cannot pass
// against a file that was moved, renamed, or split apart.
assert.match(tokensSource, /export default function TokensPage\(/);
for (const rawText of [
"Send Tokens",
"Create Invite",

View File

@@ -56,6 +56,9 @@ test("POST /api/providers auto-sync uses the trusted internal origin (not reques
});
test("POST /api/sync/initialize never forwards the client Origin to model sync", () => {
// Anchor: the route handler itself, so the SSRF guards below cannot pass against a
// file that was moved, renamed, or split apart.
assert.match(syncInitializeRouteSrc, /export async function POST\(/);
assert.doesNotMatch(
syncInitializeRouteSrc,
/request\.headers\.get\(["']origin["']\)/,

View File

@@ -39,12 +39,18 @@ test("bypassProxyPatch is absent from the chat hot path (scope guard — #3226)"
// If either of these assertions starts failing, a code change has silently
// extended the NVIDIA-only exception to the chat/usage egress path.
const chatHelpers = readFileSync("src/sse/handlers/chatHelpers.ts", "utf8");
// Anchor: without it, extracting the egress code into another module would make the
// negative guard below pass against a file that no longer contains the hot path.
assert.match(chatHelpers, /export async function executeChatWithBreaker\(/);
assert.ok(
!chatHelpers.includes("bypassProxyPatch"),
"chatHelpers.ts must not bypass the proxy patch — only NVIDIA validation may do this (#3226)"
);
const chatCore = readFileSync("open-sse/handlers/chatCore.ts", "utf8");
// Anchor: chatCore.ts is actively being split, so pin the entry point the route
// imports — the negative guard is worthless if this read silently misses the file.
assert.match(chatCore, /export async function handleChatCore\(/);
assert.ok(
!chatCore.includes("bypassProxyPatch"),
"chatCore.ts must not bypass the proxy patch — only NVIDIA validation may do this (#3226)"

View File

@@ -1,14 +1,7 @@
import test from "node:test";
import assert from "node:assert/strict";
import { readFileSync } from "node:fs";
import { dirname, join } from "node:path";
import { fileURLToPath } from "node:url";
const ROOT = join(dirname(fileURLToPath(import.meta.url)), "..", "..");
function readSrc(path: string): string {
return readFileSync(join(ROOT, path), "utf8");
}
import { readSrc } from "../_helpers/readSrc";
function assertInOrder(source: string, labels: string[]) {
let lastIndex = -1;
@@ -34,6 +27,9 @@ test("Usage Token Buffer lives in AI settings instead of General storage", () =>
);
assert.match(aiPage, /UsageTokenBufferTab/);
// Anchor: the storage tab component itself, so the negative guard below cannot
// pass against a file that was moved, renamed, or split apart.
assert.match(generalStorage, /export default function SystemStorageTab\(/);
assert.doesNotMatch(generalStorage, /storageUsageTokenBuffer/);
});

View File

@@ -0,0 +1,423 @@
// Hard gate: every test variable holding project source read as text must carry at
// least one POSITIVE anchor.
//
// The dangerous shape is the negative-only guard:
// const endpoint = readSource("src/.../EndpointPageClient.tsx");
// assert.equal(endpoint.includes(">Active Endpoints<"), false);
// When that markup is later extracted into a new component file, the parent no longer
// contains the string — so the assertion still passes while guarding nothing. The
// regression coverage is deleted silently, with no test turning red.
//
// One positive assertion against the same variable (assert.match(endpoint, /…/) on a
// stable top-level declaration) proves the read still resolved to the right, non-empty
// file — which makes the negative guards meaningful again.
//
// There is deliberately NO baseline and NO allowlist: the violation set is small and
// every instance is a real hole. Fix it by adding an anchor, not by suppressing it.
//
// Classification works on LOGICAL statements, not physical lines, because negative
// guards are routinely written across several lines:
// assert.doesNotMatch(
// source,
// /^const\s+require\s*=\s*createRequire\s*\(/m
// );
// A line-based scan sees a bare `source,` line, matches no negative pattern, and files
// it as a positive anchor — which would let anyone bypass this gate by reformatting.
//
// KNOWN LIMITATION (deliberate): variables are keyed by name per FILE, not per scope.
// A file that declares `const source` in two separate test bodies pools both sets of
// usages, so an anchor on one can mask a negative-only guard on the other
// (tests/unit/8395-plugin-hooks-fire.test.ts is the current example). This errs toward
// UNDER-reporting — it never invents a violation — and closing it needs real
// scope-aware parsing, which is not worth the complexity for the size of this problem.
import assert from "node:assert/strict";
import { readdirSync, readFileSync } from "node:fs";
import { dirname, join, relative, resolve, sep } from "node:path";
import test from "node:test";
import { fileURLToPath } from "node:url";
const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", "..");
const TESTS_ROOT = join(REPO_ROOT, "tests");
const TEST_FILE_RE = /\.(test|spec)\.(ts|tsx|mjs)$/;
const SKIP_DIRS = new Set(["node_modules", "fixtures", "__snapshots__", "coverage"]);
/** Functions whose return value is project source read as text. */
const READER_NAMES = ["readFileSync", "readProjectFile", "readSource", "readSrc"];
/** Safety valve: no single statement in this repo legitimately spans more lines. */
const MAX_UNIT_LINES = 40;
/**
* `const <name> = <reader>(<args>);` — captures the variable and the call arguments so
* we can tell "read a project file" apart from "read a fixture / temp file".
*/
const ASSIGNMENT_RE = new RegExp(
String.raw`(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:await\s+)?(?:${READER_NAMES.join(
"|"
)})\s*\(([^;]*?)\)\s*;`,
"gs"
);
/** A quoted path pointing into one of the project's production source trees. */
const PROJECT_PATH_RE =
/["'`][^"'`]*(?:src|open-sse|electron|bin)\/[^"'`]*\.(?:ts|tsx|mjs|js)["'`]/;
/** Relative escapes such as `new URL("../../open-sse/executors/claude-web.ts", …)`. */
const RELATIVE_PATH_RE = /\.\.\//;
interface SourceVar {
name: string;
/** 1-based line range covered by the assignment statement itself. */
declStartLine: number;
declEndLine: number;
negativeLines: number[];
positiveLines: number[];
}
interface LogicalUnit {
/** 1-based line where the statement starts — what the failure message reports. */
startLine: number;
endLine: number;
/** Joined statement text, with strings, regexes, and comments blanked out. */
code: string;
}
interface Violation {
file: string;
varName: string;
negativeLines: number[];
}
function walkTestFiles(dir: string, acc: string[] = []): string[] {
for (const entry of readdirSync(dir, { withFileTypes: true })) {
const full = join(dir, entry.name);
if (entry.isDirectory()) {
if (!SKIP_DIRS.has(entry.name)) walkTestFiles(full, acc);
} else if (TEST_FILE_RE.test(entry.name)) {
acc.push(full);
}
}
return acc;
}
/**
* Assertions that only prove something is ABSENT. They pass against an empty string,
* so on their own they prove nothing about the source having been read correctly.
* `\s*` spans newlines, so these also match the multi-line form once lines are joined.
*/
function negativeGuardPatterns(name: string): RegExp[] {
const v = escapeForRegExp(name);
return [
new RegExp(String.raw`assert\.doesNotMatch\s*\(\s*${v}\b`),
new RegExp(String.raw`assert\.equal\s*\(\s*${v}\.includes\s*\([^)]*\)\s*,\s*false`),
new RegExp(String.raw`assert\.strictEqual\s*\(\s*${v}\.includes\s*\([^)]*\)\s*,\s*false`),
new RegExp(String.raw`assert\.ok\s*\(\s*!\s*${v}\.includes`),
new RegExp(String.raw`assert\.equal\s*\(\s*${v}\.indexOf\s*\([^)]*\)\s*,\s*-1`),
];
}
function escapeForRegExp(literal: string): string {
return literal.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
}
function lineNumberAt(source: string, index: number): number {
let line = 1;
for (let i = 0; i < index; i++) if (source.charCodeAt(i) === 10) line++;
return line;
}
interface ScrubState {
inBlockComment: boolean;
inTemplate: boolean;
}
/** True when a `/` at this position opens a regex literal rather than dividing. */
function startsRegex(codeSoFar: string): boolean {
const prev = codeSoFar.replace(/\s+$/, "").slice(-1);
return prev === "" || "(,=:[!&|?{};+-*%~^".includes(prev);
}
/** Consume a quoted string starting at `i`; returns the index just past the closing quote. */
function skipQuoted(line: string, start: number, quote: string): number {
let i = start + 1;
while (i < line.length) {
if (line[i] === "\\") i += 2;
else if (line[i] === quote) return i + 1;
else i++;
}
return i;
}
/** Consume a regex literal starting at `i`; returns the index just past the closing slash. */
function skipRegex(line: string, start: number): number {
let i = start + 1;
let inClass = false;
while (i < line.length) {
if (line[i] === "\\") i += 2;
else if (line[i] === "/" && !inClass) return i + 1;
else {
if (line[i] === "[") inClass = true;
else if (line[i] === "]") inClass = false;
i++;
}
}
return i;
}
/**
* Blank out string literals, template literals, regex literals, and comments so bracket
* counting sees only real code brackets: `/notify\.error\(/` must not read as an
* unclosed parenthesis, and a `//` comment must not swallow the rest of a statement.
* Blanking (rather than deleting) also stops a variable name mentioned inside a comment
* or a string from counting as a usage.
*/
function scrubLine(line: string, state: ScrubState): string {
let out = "";
let i = 0;
const blank = (from: number, to: number) => " ".repeat(Math.min(to, line.length) - from);
while (i < line.length) {
const ch = line[i];
if (state.inBlockComment) {
if (ch === "*" && line[i + 1] === "/") {
state.inBlockComment = false;
out += " ";
i += 2;
} else {
out += " ";
i++;
}
continue;
}
if (state.inTemplate) {
if (ch === "\\") {
out += " ";
i += 2;
} else {
if (ch === "`") state.inTemplate = false;
out += " ";
i++;
}
continue;
}
if (ch === "/" && line[i + 1] === "/") return out + blank(i, line.length);
if (ch === "/" && line[i + 1] === "*") {
state.inBlockComment = true;
out += " ";
i += 2;
continue;
}
if (ch === '"' || ch === "'") {
const end = skipQuoted(line, i, ch);
out += blank(i, end);
i = end;
continue;
}
if (ch === "`") {
state.inTemplate = true;
out += " ";
i++;
continue;
}
if (ch === "/" && startsRegex(out)) {
const end = skipRegex(line, i);
out += blank(i, end);
i = end;
continue;
}
out += ch;
i++;
}
return out;
}
function bracketDelta(code: string, open: string, close: string): number {
let depth = 0;
for (const ch of code) {
if (ch === open) depth++;
else if (ch === close) depth--;
}
return depth;
}
/**
* Fold physical lines into logical statements: keep joining while parentheses are still
* open. A line that opens a block (`test("…", () => {`) ends the unit regardless, so a
* whole test body never collapses into one blob — that would let a single negative guard
* mislabel every other assertion around it.
*/
function toLogicalUnits(lines: string[]): LogicalUnit[] {
const state: ScrubState = { inBlockComment: false, inTemplate: false };
const scrubbed = lines.map((line) => scrubLine(line, state));
const units: LogicalUnit[] = [];
let index = 0;
while (index < lines.length) {
let parenDepth = 0;
let braceDepth = 0;
let end = index;
for (; end < lines.length && end - index < MAX_UNIT_LINES; end++) {
parenDepth += bracketDelta(scrubbed[end], "(", ")");
braceDepth += bracketDelta(scrubbed[end], "{", "}");
if (parenDepth <= 0 || braceDepth > 0) break;
}
if (end >= lines.length) end = lines.length - 1;
units.push({
startLine: index + 1,
endLine: end + 1,
code: scrubbed.slice(index, end + 1).join("\n"),
});
index = end + 1;
}
return units;
}
/** Collect every variable in `source` that is bound to a project source file read. */
function collectSourceVars(source: string): SourceVar[] {
const vars: SourceVar[] = [];
ASSIGNMENT_RE.lastIndex = 0;
let match: RegExpExecArray | null;
while ((match = ASSIGNMENT_RE.exec(source)) !== null) {
const [statement, name, args] = match;
if (!PROJECT_PATH_RE.test(args) && !RELATIVE_PATH_RE.test(args)) continue;
const declStartLine = lineNumberAt(source, match.index);
vars.push({
name,
declStartLine,
declEndLine: declStartLine + statement.split("\n").length - 1,
negativeLines: [],
positiveLines: [],
});
}
return vars;
}
/**
* Classify each logical statement that mentions the variable, outside its declaration.
* Anything that is not a recognised negative guard counts as a positive anchor —
* including custom helper calls such as `assertInOrder(source, [...])`, which fail on
* empty input.
*/
function classifyUsages(units: LogicalUnit[], sourceVar: SourceVar): void {
const mention = new RegExp(String.raw`\b${escapeForRegExp(sourceVar.name)}\b`);
const negatives = negativeGuardPatterns(sourceVar.name);
for (const unit of units) {
if (unit.startLine >= sourceVar.declStartLine && unit.startLine <= sourceVar.declEndLine) {
continue;
}
if (!mention.test(unit.code)) continue;
if (negatives.some((pattern) => pattern.test(unit.code))) {
sourceVar.negativeLines.push(unit.startLine);
} else {
sourceVar.positiveLines.push(unit.startLine);
}
}
}
/** Analyze one file's text. Exported so the meta-test can feed the classifier a snippet. */
export function findViolationsInSource(fileLabel: string, source: string): Violation[] {
const sourceVars = collectSourceVars(source);
if (sourceVars.length === 0) return [];
const units = toLogicalUnits(source.split("\n"));
const violations: Violation[] = [];
for (const sourceVar of sourceVars) {
classifyUsages(units, sourceVar);
if (sourceVar.negativeLines.length > 0 && sourceVar.positiveLines.length === 0) {
violations.push({
file: fileLabel,
varName: sourceVar.name,
negativeLines: sourceVar.negativeLines,
});
}
}
return violations;
}
function findViolations(): Violation[] {
const violations: Violation[] = [];
for (const file of walkTestFiles(TESTS_ROOT).sort()) {
const label = relative(REPO_ROOT, file).split(sep).join("/");
violations.push(...findViolationsInSource(label, readFileSync(file, "utf8")));
}
return violations;
}
function describeViolations(violations: Violation[]): string {
const details = violations
.map(
(v) =>
` ${v.file}\n` +
` variable "${v.varName}" is guarded only by negative assertions ` +
`(line${v.negativeLines.length === 1 ? "" : "s"} ${v.negativeLines.join(", ")})\n` +
` fix: add one positive anchor, e.g. ` +
`assert.match(${v.varName}, /export default function SomeComponent/);`
)
.join("\n\n");
return (
`${violations.length} source-bound test variable(s) carry ONLY negative guards:\n\n` +
`${details}\n\n` +
`A negative guard (assert.doesNotMatch / .includes(...) === false) passes against an ` +
`empty string, so once the guarded code is extracted into another file the assertion ` +
`keeps passing while protecting nothing. Add at least one positive assertion against ` +
`the same variable — anchored on a stable top-level declaration of the file it reads — ` +
`so the test fails loudly if the source moves.`
);
}
test("test files scanning project source are not guarded by negative assertions alone", () => {
const violations = findViolations();
assert.deepEqual(violations, [], violations.length > 0 ? describeViolations(violations) : "");
});
test("the scanner actually finds the source-scanning tests it is meant to police", () => {
// Guards the guard: if the walk or the assignment regex silently stops matching,
// findViolations() would return [] and this gate would pass while doing nothing.
const scanned = walkTestFiles(TESTS_ROOT).filter(
(file) => collectSourceVars(readFileSync(file, "utf8")).length > 0
);
assert.ok(
scanned.length >= 20,
`expected the scanner to detect many source-reading test files, found ${scanned.length}` +
`the walk or the assignment pattern has probably stopped matching`
);
});
test("a negative guard split across lines is still detected as a negative guard", () => {
// Regression guard for the line-based blind spot: a bare `source,` line matches no
// negative pattern, so a per-line scan filed it as a positive anchor and the variable
// looked anchored. Reformatting must not become a way past this gate.
const snippet = [
'const source = readFileSync("src/app/example/page.tsx", "utf8");',
"",
"test('multi-line negative guard', () => {",
" assert.doesNotMatch(",
" source,",
" /notify\\.error\\(\\s*data\\.error\\b/,",
' "must not surface the raw error"',
" );",
"});",
].join("\n");
assert.deepEqual(findViolationsInSource("synthetic.test.ts", snippet), [
{ file: "synthetic.test.ts", varName: "source", negativeLines: [4] },
]);
});
test("a positive anchor split across lines still counts as an anchor", () => {
// The mirror case: folding must not turn a multi-line positive assertion into a
// violation. Guards against over-reporting now that units span lines.
const snippet = [
'const source = readFileSync("src/app/example/page.tsx", "utf8");',
"",
"test('multi-line anchor plus negative guard', () => {",
" assert.match(",
" source,",
" /export default function ExamplePage\\(/",
" );",
" assert.doesNotMatch(source, /notify\\.error/);",
"});",
].join("\n");
assert.deepEqual(findViolationsInSource("synthetic.test.ts", snippet), []);
});