fix(commands): stop writing files no later step can read, and payloads nobody asked for
Dogfooding the four read commands (posture, tokens, manifest, whats-active) surfaced four defect classes, all in the seam between what a command template promises and what the scanner behind it actually does. M-BUG-40, fifth arm: posture wrote four temp files it could never read back. #49 closed the $$/cross-block class in four commands, but posture survived it — and so did the guard written to prevent exactly this. The guard compared each $$ path to the block that created it, so a path written once and then read via prose had no second occurrence to flag. Measured live: written from PID 21614, read attempted from PID 23772. The invariant is now blanket (no $$ in any temp path), which also caught fix.md and feature-gap.md. M-BUG-43: 6 of 7 scanners write their payload to stdout when --raw/--json is set even when --output-file was given, and the templates redirected only stderr. Measured: posture 255 182 B, whats-active 35 922 B, drift 28 316 B, manifest 23 825 B, tokens 8 768 B. fix and feature-gap never read the file they wrote, so both recovered one letter grade from a quarter-megabyte dump. tokens swallowed --json and --with-telemetry-recipe: documented, never threaded, so --json returned the humanized payload where the docs promise byte-stable v5.0.0 output. M-BUG-42: manifest's render contract asked for {load}; the payload carries loadPattern, so the Load column rendered blank for all 96 rows. Four new tests (1449 -> 1453), each verified red before the fix. The render-contract test checks {field} names against a live payload from a fixture, since a hardcoded key list would drift. Frozen v5.0.0 snapshots untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGCk9o27eWo9uXLjkZTXEq
This commit is contained in:
parent
09f817977c
commit
acd1cf1248
13 changed files with 336 additions and 21 deletions
222
tests/commands/command-output-discipline.test.mjs
Normal file
222
tests/commands/command-output-discipline.test.mjs
Normal file
|
|
@ -0,0 +1,222 @@
|
|||
/**
|
||||
* Session #50 — command-template output-discipline tests.
|
||||
*
|
||||
* Dogfooding the four read commands (`posture`, `tokens`, `manifest`,
|
||||
* `whats-active`) surfaced three defect classes that all live in the same
|
||||
* seam: what a command template PROMISES versus what the scanner behind it
|
||||
* actually does.
|
||||
*
|
||||
* 1. M-BUG-43 — a scanner invoked with `--raw`/`--json` writes the payload
|
||||
* to stdout *even when `--output-file` is set*
|
||||
* (`token-hotspots-cli.mjs:137`, `manifest.mjs:293`,
|
||||
* `whats-active.mjs:60`, `posture.mjs:101`, `drift-cli.mjs`,
|
||||
* `plugin-health-scanner.mjs`). The command templates redirect only
|
||||
* stderr, so the payload lands in the transcript. Measured on this repo:
|
||||
* posture 255 182 B, whats-active 35 922 B, drift 28 316 B,
|
||||
* manifest 23 825 B, tokens 8 768 B. `.claude/rules/ux-rules.md` rule 1
|
||||
* says NEVER show raw JSON — and a plugin that exists to cut token cost
|
||||
* must not dump a quarter-megabyte to report one grade.
|
||||
*
|
||||
* 2. Flags documented in a command's prose that never reach any shell
|
||||
* (`tokens.md`: `--json`, `--with-telemetry-recipe`). The scanner
|
||||
* supports them; the template silently swallows them. Same "green is
|
||||
* worse than an error" shape as the arg-sink class.
|
||||
*
|
||||
* 3. Render contracts that name a field the scanner never emits
|
||||
* (`manifest.md` asked for `{load}`; the payload carries `loadPattern`),
|
||||
* so the column the command's own prose calls the whole point renders
|
||||
* blank for every row.
|
||||
*
|
||||
* Test 3 runs the real scanners against a fixture rather than asserting
|
||||
* against a hardcoded key list — a hardcoded list drifts, a live payload
|
||||
* cannot.
|
||||
*/
|
||||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { readFile, readdir, mkdtemp, rm } from 'node:fs/promises';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { execFile } from 'node:child_process';
|
||||
import { promisify } from 'node:util';
|
||||
import { resolve, dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
|
||||
const execFileAsync = promisify(execFile);
|
||||
const __dirname = dirname(fileURLToPath(import.meta.url));
|
||||
const ROOT = resolve(__dirname, '..', '..');
|
||||
const COMMANDS_DIR = join(ROOT, 'commands');
|
||||
const FIXTURE = join(ROOT, 'tests', 'fixtures', 'marketplace-medium');
|
||||
|
||||
async function commandFiles() {
|
||||
const entries = await readdir(COMMANDS_DIR);
|
||||
return entries.filter((e) => e.endsWith('.md')).sort();
|
||||
}
|
||||
|
||||
/** Strip `#` comments so the tests never match their own explanatory prose. */
|
||||
function stripComment(line) {
|
||||
const h = line.indexOf('#');
|
||||
return h === -1 ? line : line.slice(0, h);
|
||||
}
|
||||
|
||||
/** Yield [lineNumber, line] for lines inside ```bash fences only. */
|
||||
function bashLines(content) {
|
||||
const out = [];
|
||||
let open = null;
|
||||
content.split('\n').forEach((line, i) => {
|
||||
const m = line.match(/^\s*```(\w*)/);
|
||||
if (m) {
|
||||
open = open === null ? m[1] : null;
|
||||
return;
|
||||
}
|
||||
if (open === 'bash') out.push([i + 1, line]);
|
||||
});
|
||||
return out;
|
||||
}
|
||||
|
||||
/** Yield lines inside ```markdown fences — the render contracts. */
|
||||
function markdownFenceLines(content) {
|
||||
const out = [];
|
||||
let open = null;
|
||||
content.split('\n').forEach((line) => {
|
||||
const m = line.match(/^\s*```(\w*)/);
|
||||
if (m) {
|
||||
open = open === null ? m[1] : null;
|
||||
return;
|
||||
}
|
||||
if (open === 'markdown') out.push(line);
|
||||
});
|
||||
return out;
|
||||
}
|
||||
|
||||
test('Output discipline: a scanner asked for --output-file never also prints to stdout', async () => {
|
||||
// The rule is blanket and needs no per-scanner allowlist: if the command
|
||||
// asked for the payload in a FILE, the same invocation must not let it reach
|
||||
// the transcript. Invocations with no --output-file are out of scope — there
|
||||
// the payload has nowhere else to go (e.g. `drift --save`, 112 B).
|
||||
const violations = [];
|
||||
for (const name of await commandFiles()) {
|
||||
const content = await readFile(join(COMMANDS_DIR, name), 'utf-8');
|
||||
for (const [lineNo, raw] of bashLines(content)) {
|
||||
const line = stripComment(raw);
|
||||
if (!/node \$\{CLAUDE_PLUGIN_ROOT\}\/scanners\//.test(line)) continue;
|
||||
if (!line.includes('--output-file')) continue;
|
||||
const rawish = /--json\b|--raw\b|\$RAW_FLAG/.test(line);
|
||||
if (!rawish) continue;
|
||||
// Mask `2>` so a stderr redirect is never mistaken for a stdout one.
|
||||
const masked = line.replace(/2>/g, '2@');
|
||||
if (/(^|\s)>\s*\S+/.test(masked)) continue;
|
||||
violations.push(
|
||||
`${name}:${lineNo} runs a scanner in raw/json mode with --output-file but never redirects stdout — the payload lands in the transcript`,
|
||||
);
|
||||
}
|
||||
}
|
||||
assert.deepEqual(violations, [], `Unredirected scanner payloads:\n${violations.join('\n')}`);
|
||||
});
|
||||
|
||||
test('Flag threading: every flag documented in prose reaches a shell', async () => {
|
||||
// A flag the user is told to pass must arrive somewhere. Two legitimate
|
||||
// destinations exist: a bash fence (threaded to the scanner) or a documented
|
||||
// control-flow branch handled by the model. The allowlist below carries only
|
||||
// the second kind, each verified by reading the code:
|
||||
// posture --drift / --plugin-health : step 5 runs *different* scanners
|
||||
// fix --dry-run : dry-run is fix-cli's DEFAULT and the
|
||||
// flag is an explicit no-op alias
|
||||
// (`scanners/fix-cli.mjs:18-21`)
|
||||
// knowledge-refresh --no-candidates : skips a web-poll step; never a CLI flag
|
||||
// manifest/whats-active --json : served by `cat` of the payload, whose
|
||||
// content is byte-identical to --raw
|
||||
// (both scanners document --raw as a
|
||||
// no-op; verified by diffing payloads)
|
||||
const PROSE_HANDLED = new Set([
|
||||
'posture.md:--drift',
|
||||
'posture.md:--plugin-health',
|
||||
'fix.md:--dry-run',
|
||||
'knowledge-refresh.md:--no-candidates',
|
||||
'manifest.md:--json',
|
||||
'whats-active.md:--json',
|
||||
]);
|
||||
|
||||
const violations = [];
|
||||
for (const name of await commandFiles()) {
|
||||
const content = await readFile(join(COMMANDS_DIR, name), 'utf-8');
|
||||
const bash = bashLines(content).map(([, l]) => l).join('\n');
|
||||
const documented = new Set();
|
||||
for (const line of content.split('\n')) {
|
||||
const m = line.match(/^\s*[-*]\s+`(--[a-z][a-z0-9-]*)`/);
|
||||
if (m) documented.add(m[1]);
|
||||
}
|
||||
for (const flag of documented) {
|
||||
if (bash.includes(flag)) continue;
|
||||
if (PROSE_HANDLED.has(`${name}:${flag}`)) continue;
|
||||
violations.push(`${name} documents \`${flag}\` but no bash fence ever passes it`);
|
||||
}
|
||||
}
|
||||
assert.deepEqual(violations, [], `Flags dropped between docs and shell:\n${violations.join('\n')}`);
|
||||
});
|
||||
|
||||
test('Render contract: every {field} in a render fence exists in the real payload', async () => {
|
||||
// Derived values the model computes rather than reads. Kept deliberately
|
||||
// short — every entry here is a field the render fence does NOT get from the
|
||||
// scanner, so a long list would hollow out the test.
|
||||
const DERIVED = new Set(['rank']);
|
||||
const CASES = [
|
||||
{ command: 'manifest.md', scanner: 'manifest.mjs', args: [] },
|
||||
{ command: 'tokens.md', scanner: 'token-hotspots-cli.mjs', args: [] },
|
||||
{
|
||||
command: 'whats-active.md',
|
||||
scanner: 'whats-active.mjs',
|
||||
args: ['--verbose', '--suggest-disables'],
|
||||
},
|
||||
];
|
||||
|
||||
const dir = await mkdtemp(join(tmpdir(), 'ca-render-'));
|
||||
try {
|
||||
const violations = [];
|
||||
for (const { command, scanner, args } of CASES) {
|
||||
const outFile = join(dir, `${scanner}.json`);
|
||||
await execFileAsync('node', [
|
||||
join(ROOT, 'scanners', scanner), FIXTURE, '--output-file', outFile, ...args,
|
||||
]);
|
||||
const payload = JSON.parse(await readFile(outFile, 'utf-8'));
|
||||
|
||||
const keys = new Set();
|
||||
const walk = (node) => {
|
||||
if (Array.isArray(node)) node.slice(0, 5).forEach(walk);
|
||||
else if (node && typeof node === 'object') {
|
||||
for (const k of Object.keys(node)) {
|
||||
keys.add(k);
|
||||
walk(node[k]);
|
||||
}
|
||||
}
|
||||
};
|
||||
walk(payload);
|
||||
|
||||
const content = await readFile(join(COMMANDS_DIR, command), 'utf-8');
|
||||
const refs = new Set();
|
||||
for (const line of markdownFenceLines(content)) {
|
||||
const re = /\{([a-zA-Z][a-zA-Z0-9_.]*)\}/g;
|
||||
let m;
|
||||
while ((m = re.exec(line)) !== null) refs.add(m[1]);
|
||||
}
|
||||
for (const ref of refs) {
|
||||
if (DERIVED.has(ref)) continue;
|
||||
if (ref.endsWith('.length')) {
|
||||
// `{foo.length}` is a count of a collection — assert the collection
|
||||
// itself exists, since that is the part the scanner owns.
|
||||
const base = ref.slice(0, -'.length'.length).split('.').pop();
|
||||
if (!keys.has(base)) {
|
||||
violations.push(`${command}: {${ref}} — no \`${base}\` collection in the payload`);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
const leaf = ref.split('.').pop();
|
||||
if (!keys.has(leaf) && !keys.has(ref)) {
|
||||
violations.push(`${command}: {${ref}} is never emitted by ${scanner}`);
|
||||
}
|
||||
}
|
||||
}
|
||||
assert.deepEqual(violations, [], `Render fields the scanner never emits:\n${violations.join('\n')}`);
|
||||
} finally {
|
||||
await rm(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
|
@ -151,6 +151,35 @@ test('Shell state: no $$ temp path is referenced outside the block that created
|
|||
assert.deepEqual(violations, [], `Unresolvable $$ temp paths:\n${violations.join('\n')}`);
|
||||
});
|
||||
|
||||
test('Shell state: no $$ appears in any temp path at all', async () => {
|
||||
// Session #50 closed a blind spot in the test above: it only flags a `$$`
|
||||
// path that is *referenced twice*, because it compares each occurrence to
|
||||
// the block that created it. A path written once and then read via prose
|
||||
// ("Read the JSON output file using the Read tool") has no second
|
||||
// occurrence — so posture.md sat green through #49 while being unreadable
|
||||
// by construction: the PID is never printed, so no later step can name the
|
||||
// file. Measured live: written by PID 21614, read attempted from PID 23772.
|
||||
//
|
||||
// The invariant is therefore blanket, not relational: a command template
|
||||
// must not put `$$` in a temp path at all. The hardened pattern from
|
||||
// drift.md — one fixed literal path, repeated literally — is the only
|
||||
// shape that survives the fence boundary.
|
||||
const violations = [];
|
||||
for (const name of await commandFiles()) {
|
||||
const content = await readFile(resolve(COMMANDS_DIR, name), 'utf-8');
|
||||
content.split('\n').forEach((raw, i) => {
|
||||
const re = /(\/tmp\/[A-Za-z0-9._-]*\$\$[A-Za-z0-9._-]*)/g;
|
||||
let m;
|
||||
while ((m = re.exec(stripComment(raw))) !== null) {
|
||||
violations.push(
|
||||
`${name}:${i + 1} ${m[1]} — $$ differs per Bash call; no later step can name this file`,
|
||||
);
|
||||
}
|
||||
});
|
||||
}
|
||||
assert.deepEqual(violations, [], `Unresolvable $$ temp paths:\n${violations.join('\n')}`);
|
||||
});
|
||||
|
||||
test('Read tool: never asked to expand a glob', async () => {
|
||||
const violations = [];
|
||||
for (const name of await commandFiles()) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue