open/config-audit
1
0
Fork 0
Code Issues Releases 1 Activity

fix(scanners): the command layer's argv is now checked against the CLI that receives it

Browse source 
A command template is a caller with no compiler behind it. It names a scanner and
an argv; nothing checked that the scanner still accepts them. M-BUG-45 measured
what that costs: `--stale-after` arrived malformed, was ignored, and the command
reported "all 14 entries re-verified within the last 90 days" about a threshold
the user had just overridden.

The new guard builds the argv from each template's OWN text (#63 — a hand-typed
call is a path no user takes), reading all three forms a flag appears in,
including the comment-only `GLOBAL_FLAG=""  # --global`; that third form is the
one that dies unobserved, since the default path leaves the variable empty.
Measured: 38 invocations, 54 (CLI, flag) pairs, 15 CLIs, 0 dead scanner paths.

Two premises in the plan text were falsified by measuring:

  - "the flag exists in the CLI's BOOL_FLAGS/VALUE_FLAGS" — only 3 of 34 scanner
    files declare such a surface. The contract is checked on BEHAVIOUR instead:
    run the CLI, ask whether it calls the flag unknown.
  - `--full-machine` was predicted dead on `posture`. It is live. The fasit was
    wrong, not the code.

What the measurement found instead: `campaign-export-cli` was the only one of the
fifteen without the shared `requireValidArgs` gate. Its hand-rolled chain guards
every value branch with `argv[i + 1] !== undefined`, so a trailing `--repo` fell
past all of them to the `startsWith('--')` catch-all and was reported as an
unknown flag — about the flag the CLI itself requires. Classification of "value
flag, no value" across all fifteen: 14 correct, 1 wrong. It now uses ARG_SPEC +
requireValidArgs like the other twelve; valid argv reaches the existing loop
byte-for-byte unchanged. Special-casing it in the test would have rebuilt, in
test code, the prose exception Q1 deleted.

And what the guard itself got wrong, which is worse than what it was looking for:
probing a flag means RUNNING the CLI, and some flags are writers. Its first run
let `drift-cli --save` default its target to the working directory and overwrite
the operator's real ~/.config-audit/baselines/default.json — an ungated write
outside the repo, produced by the guard whose whole subject is ungated writes
outside the repo. Every probe now runs under hermeticEnv() with its own empty
cwd, and the cwd is asserted empty afterwards. Isolation that is only a
convention is not isolation. Side effect: 65s -> 13s, because a hermetic HOME
stops every probe from enumerating ~/.claude.

All six arms seen RED against their own defect, twice — including the ORIGINAL
class (remove --approve-scope from fix-cli) and the plan's own verification
(delete the write-scope-cli line from a template). The non-emptiness arm is
derived from the tree, not pinned to a count that would only be a drift point.

Suite 1707 -> 1724, frozen v5.0.0 + default-output snapshots 0 changed files.

Not fixed here, found while verifying and pre-existing at 749b710: the suite was
NOT green on HEAD. output-file-robustness fails on drift-cli, root cause
diff-engine.mjs:194 — `m.from.severity` where `m.from` is undefined in the moved
section of the drift report. It crashes after the scan, in formatting, so the
CLI exits 3 with no output file. Its own chunk, not this one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pj6UoTi6iPsAB2B2j6EZ1k
This commit is contained in:
Kjell Tore Guttormsen 2026-08-12 21:58:38 +02:00
commit 30c78aeda0
4 changed files with 400 additions and 1 deletions
Download patch file Download diff file Expand all files Collapse all files

208
tests/commands/command-cli-contract.test.mjs Normal file
View file

@ -0,0 +1,208 @@
/**
* Q2 — the command layer's contract with the CLI layer.
*
* The templates tell an agent to run a CLI with a particular argv. Until now
* nothing checked that the CLI on the other end still *accepts* that argv. The
* measured cost of that gap is M-BUG-45: `--stale-after` reached
* `knowledge-refresh-cli` malformed, the CLI ignored it, and the command
* reported "✓ all 14 entries re-verified within the last 90 days" about a
* threshold the user had just overridden. A flag that quietly stops existing
* produces the same sentence.
*
* Three properties are load-bearing here, each learned from a guard that was
* green on its own defect:
*
* 1. **The argv is built from the template's own text**, never hand-typed
* ([[dogfood-the-command-not-the-cli]], #63 — a hand-written call is a path
* no user takes). `tests/helpers/command-invocations.mjs` reads all three
* forms a flag appears in, including the comment-only form
* (`GLOBAL_FLAG="" # --global`), which is the one that can die unobserved
* because the default path leaves the variable empty.
*
* 2. **The probe proves itself per CLI before it is trusted.** A CLI that
* exits on a required-arg check before reaching flag parsing would report
* nothing about any flag, and every pair for it would pass vacuously. So each
* CLI must first be seen rejecting a flag that certainly does not exist. An
* empty sweep certifies nothing (#63, #64). Measured when this was written:
* 15 of 15 report the unknown flag first, so no prefix argv is needed — a
* fact worth re-deriving rather than assuming, which is why it is asserted.
*
* 3. **"Unknown" is distinguished from "needs a value" by the CLI's own
* words**, so a value-taking flag passed last is not mistaken for a dead one.
* That distinction is only sound because every CLI classifies the two
* correctly — measured 14/15 when this was written; the fifteenth
* (`campaign-export-cli`, the last hand-rolled parser) called its own
* required `--repo` an unknown flag and was moved onto the shared
* `requireValidArgs` gate in the same chunk. A guard that special-cased it
* instead would have rebuilt, in test code, the prose exception Q1 deleted.
*
* Not asserted here: that a template calling a gated writer also calls
* `write-scope-cli`. That arm belongs to write-scope-gate-shape.test.mjs, and
* deriving it from the writer set would be false-red — `discover` and
* `config-audit` invoke `scan-orchestrator` without ever reaching its
* `--save-baseline` write.
*/
import { test } from 'node:test';
import { strict as assert } from 'node:assert';
import { spawn } from 'node:child_process';
import { access, mkdtemp, readdir, rm } from 'node:fs/promises';
import { resolve, dirname, join } from 'node:path';
import { tmpdir } from 'node:os';
import { fileURLToPath } from 'node:url';
import { allInvocations, commandTemplates, extractInvocations } from '../helpers/command-invocations.mjs';
import { hermeticEnv } from '../helpers/hermetic-home.mjs';
const __dirname = dirname(fileURLToPath(import.meta.url));
const SCANNERS_DIR = resolve(__dirname, '..', '..', 'scanners');
/** A flag no CLI can plausibly define — the probe's own control. */
const IMPOSSIBLE_FLAG = '--zzz-not-a-real-flag';
/**
* Probing a flag means RUNNING the CLI, and some of these flags are writers.
* Measured the first time this file ran, against the real environment:
* `drift-cli --save` takes its target from the working directory and its name
* from a default, so the probe scanned the temp dir and overwrote the
* operator's `~/.config-audit/baselines/default.json` — an ungated write
* outside the repo, produced by the guard whose whole subject is ungated writes
* outside the repo. `fix-cli --apply` is the same shape one step worse.
*
* So every probe runs with HOME redirected into an empty temp dir AND its own
* empty working directory, and the working directory is asserted to have stayed
* empty. Isolation that is only a convention is not isolation.
*/
async function run(cli, argv) {
const sandbox = await mkdtemp(join(tmpdir(), 'ca-contract-'));
try {
const { code, stderr } = await new Promise((res) => {
const child = spawn(process.execPath, [resolve(SCANNERS_DIR, cli), ...argv], {
cwd: sandbox,
env: hermeticEnv(),
});
let err = '';
child.stderr.on('data', (d) => { err += d; });
child.stdout.on('data', () => {});
child.on('close', (c) => res({ code: c, stderr: err }));
});
return { code, stderr, wrote: await readdir(sandbox) };
} finally {
await rm(sandbox, { recursive: true, force: true });
}
}
/**
* Did the CLI call THIS flag unknown?
*
* Both wordings in the tree name the flag on the same line — `unknown flag
* "--x"` (shared `cli-args`) and `Unknown option: --x` (the BOOL_FLAGS
* parsers). "needs a value" / "requires a value" are deliberately NOT matched:
* a value-taking flag passed last is well-formed as far as this contract goes.
*/
function reportedUnknown(stderr, flag) {
const escaped = flag.replace(/[.*+?^${}()|[\]\\-]/g, '\\$&');
return new RegExp(`unknown (flag|option)[^\\n]*${escaped}`, 'i').test(stderr);
}
const invocations = await allInvocations();
const clis = [...new Set(invocations.map((i) => i.cli))].sort();
/** `Map<cli, Map<flag, site[]>>` — every flag a template hands each CLI. */
const surface = new Map();
for (const inv of invocations) {
if (!surface.has(inv.cli)) surface.set(inv.cli, new Map());
const flags = surface.get(inv.cli);
for (const flag of inv.flags) {
if (!flags.has(flag)) flags.set(flag, []);
flags.get(flag).push(`${inv.file}:${inv.line} (${inv.source.get(flag)})`);
}
}
/**
* The extractor is the floor everything else stands on. A regex that stops
* matching would make every assertion below vacuously green (#63, #64), and the
* failure would look exactly like success. Coverage is derived from the tree
* rather than pinned to a literal count, which would only be a drift point
* (#60, #61).
*/
test('the extractor finds the invocations that are actually in the templates', async () => {
assert.ok(invocations.length > 0, 'No CLI invocation was extracted from any command template.');
assert.ok(clis.length > 0, 'Invocations were found but named no CLI.');
const missed = [];
for (const { file, content } of await commandTemplates()) {
const namesACli = /node\s+\$\{CLAUDE_PLUGIN_ROOT\}\/scanners\//.test(content);
if (namesACli && extractInvocations(content).length === 0) missed.push(file);
}
assert.deepEqual(
missed,
[],
'A template writes an anchored `node ${CLAUDE_PLUGIN_ROOT}/scanners/…` call that the\n' +
'extractor did not see. Every assertion in this file is silent about whatever it cannot\n' +
'parse, so a shrinking sweep reads as a passing one.',
);
});
test('every scanner a template names exists on disk', async () => {
const dead = [];
for (const cli of clis) {
try {
await access(resolve(SCANNERS_DIR, cli));
} catch {
const sites = invocations.filter((i) => i.cli === cli).map((i) => `${i.file}:${i.line}`);
dead.push(`${cli} <- ${sites.join(', ')}`);
}
}
assert.deepEqual(
dead,
[],
'A command template tells the agent to run a scanner that is not there. The user gets a\n' +
"node stack trace the ux-rules promise they never see, because the command's own\n" +
'`2>/dev/null` hides the one line that would explain it.',
);
});
for (const cli of clis) {
const flags = surface.get(cli);
test(`${cli} accepts every flag the command templates hand it`, async () => {
// Property 2 — the probe proves itself against this CLI before its silence
// is allowed to mean anything.
const control = await run(cli, [IMPOSSIBLE_FLAG]);
assert.ok(
reportedUnknown(control.stderr, IMPOSSIBLE_FLAG),
`${cli} did not report ${IMPOSSIBLE_FLAG} as unknown (exit ${control.code}), so this test\n` +
'cannot tell an accepted flag from an unreachable parser — every flag below would pass\n' +
'for the wrong reason. Give this CLI the argv it needs to reach its flag parsing, or\n' +
'fix the CLI. stderr was: ' + JSON.stringify(control.stderr),
);
const dead = [];
const spilled = [];
for (const [flag, sites] of [...flags].sort()) {
const { stderr, wrote } = await run(cli, [flag]);
if (reportedUnknown(stderr, flag)) dead.push(`${flag} <- ${sites.join(', ')}`);
if (wrote.length) spilled.push(`${flag} left ${JSON.stringify(wrote)}`);
}
assert.deepEqual(
spilled,
[],
`${cli} wrote into the working directory while being probed with a single flag and no\n` +
'`--output-file`. Either the CLI writes somewhere it should not, or this probe is no\n' +
'longer contained — and an uncontained probe of a writer is how this test overwrote a\n' +
'real baseline the first time it ran.',
);
assert.deepEqual(
dead,
[],
`${cli} rejects a flag a command template still passes it. The template keeps working\n` +
'right up to the moment a user turns that knob on, and then the CLI exits 3 behind the\n' +
"command's `2>/dev/null` — or, before the unknown-flag guard existed, answered a\n" +
'question nobody asked (M-BUG-45). Flags:\n ' + dead.join('\n '),
);
});
}

146
tests/helpers/command-invocations.mjs Normal file
View file

@ -0,0 +1,146 @@
/**
* Extract the CLI invocations a command template tells the agent to run —
* from the template's OWN text, never hand-written.
*
* [[dogfood-the-command-not-the-cli]] (#63): a hand-typed argv is a path no user
* takes. The flag surface a template actually exercises is only knowable by
* reading the template, and it lives in THREE forms, all of them "the template's
* own text":
*
* 1. literal in the call `--apply`, `--json`
* 2. an `if` assignment `… then RAW_FLAG="--raw"; fi`
* 3. ONLY in a trailing comment `GLOBAL_FLAG="" # --global`
*
* Form 3 is the class that can die silently. The variable is empty on the
* default path, so a template run without the user flag proves nothing about
* whether the CLI still accepts it — exactly the shape of M-BUG-45, where
* `--stale-after` reached the CLI malformed and the command reported success.
* An extractor that reads only forms 1 and 2 is green on its own defect.
*/
import { readFile, readdir } from 'node:fs/promises';
import { resolve, dirname } from 'node:path';
import { fileURLToPath } from 'node:url';
const __dirname = dirname(fileURLToPath(import.meta.url));
export const COMMANDS_DIR = resolve(__dirname, '..', '..', 'commands');
/** `node ${CLAUDE_PLUGIN_ROOT}/scanners/<name>.mjs` — the only anchored form. */
const INVOCATION = /node\s+\$\{CLAUDE_PLUGIN_ROOT\}\/scanners\/([A-Za-z0-9._-]+\.mjs)/;
/** A bare `--flag` token. Stops at `=` so `--flag=value` still yields `--flag`. */
const FLAG_TOKEN = /(^|[\s"'])(--[a-z][a-z0-9-]*)(?=[\s"'=]|$)/g;
/** `$VAR` or `"$VAR"` or `${VAR}` in an argv position. */
const VAR_TOKEN = /\$\{?([A-Z_][A-Z0-9_]*)\}?/g;
/**
* Join shell line-continuations so a multi-line invocation is one logical line.
* Returns `[{ text, line }]` where `line` is the 1-based line the call STARTS on.
*/
function logicalLines(content) {
const raw = content.split('\n');
const out = [];
let buffer = null;
raw.forEach((text, i) => {
const continues = /\\\s*$/.test(text);
const body = text.replace(/\\\s*$/, '');
if (buffer) {
buffer.text += ' ' + body.trim();
if (!continues) { out.push(buffer); buffer = null; }
return;
}
if (continues) { buffer = { text: body, line: i + 1 }; return; }
out.push({ text, line: i + 1 });
});
if (buffer) out.push(buffer);
return out;
}
/**
* Every flag a shell variable can carry, resolved from the template itself.
* Returns `Map<varName, string[]>`.
*
* Both `VAR="--flag"` (any position, so the `if … then VAR="--raw"; fi` idiom is
* covered) and the comment form are read. The comment form is read from the
* WHOLE trailing comment, so `# e.g. SCOPE_FLAG="--full-machine" or
* SCOPE_FLAG="--global"` yields both candidates: a template that documents two
* possible values promises the CLI accepts both.
*/
export function resolveFlagVariables(content) {
const vars = new Map();
const add = (name, flag) => {
if (!vars.has(name)) vars.set(name, []);
if (!vars.get(name).includes(flag)) vars.get(name).push(flag);
};
for (const { text } of logicalLines(content)) {
// Form 2 — assignment to a literal flag, anywhere on the line.
for (const m of text.matchAll(/\b([A-Z_][A-Z0-9_]*)=["'](--[a-z][a-z0-9-]*)["']/g)) {
add(m[1], m[2]);
}
// Form 3 — an empty assignment whose real value lives in the comment.
const commented = text.match(/\b([A-Z_][A-Z0-9_]*)=["']{2}\s*#(.*)$/);
if (commented) {
for (const f of commented[2].matchAll(/(--[a-z][a-z0-9-]*)/g)) add(commented[1], f[1]);
}
}
return vars;
}
/**
* All invocations in one template.
* `[{ cli, flags: string[], line, source: Map<flag, 'literal'|'$VAR'> }]`
*/
export function extractInvocations(content) {
const vars = resolveFlagVariables(content);
const found = [];
for (const { text, line } of logicalLines(content)) {
const call = text.match(INVOCATION);
if (!call) continue;
// Only the part AFTER the script path is argv.
const argv = text.slice(text.indexOf(call[0]) + call[0].length);
const flags = [];
const source = new Map();
const push = (flag, from) => {
if (!flags.includes(flag)) { flags.push(flag); source.set(flag, from); }
};
for (const m of argv.matchAll(FLAG_TOKEN)) push(m[2], 'literal');
for (const m of argv.matchAll(VAR_TOKEN)) {
for (const flag of vars.get(m[1]) ?? []) push(flag, `$${m[1]}`);
}
found.push({ cli: call[1], flags, line, source });
}
return found;
}
/** Every command template, as `[{ file, content }]`, sorted. */
export async function commandTemplates() {
const entries = (await readdir(COMMANDS_DIR)).filter((e) => e.endsWith('.md')).sort();
return Promise.all(
entries.map(async (file) => ({
file,
content: await readFile(resolve(COMMANDS_DIR, file), 'utf-8'),
})),
);
}
/**
* The whole command layer's contract with the CLI layer:
* `[{ file, line, cli, flags, source }]`, one entry per invocation.
*/
export async function allInvocations() {
const out = [];
for (const { file, content } of await commandTemplates()) {
for (const inv of extractInvocations(content)) out.push({ file, ...inv });
}
return out;
}