fix(fix): validate the arguments, back up renames, and verify the scope it fixed
Dogfooding `/config-audit fix` against a throwaway repo copy. All eight
predictions registered in the fasit before the run were confirmed, and three
further defects surfaced that were not predicted.
- M-BUG-21, third arm: the argument loop ended in `!arg.startsWith('-') =>
targetPath`, so an unknown flag was dropped and its value became the target.
In `fix` that is the WRITE target under `--apply`. Unknown options and a
value-less `--output-file` now exit 3.
- `--dry-run` was documented in the command's argument-hint and never
implemented; `--output-file` did not exist, so `commands/fix.md` told the
agent to Read a file nothing produced. Both now exist.
- M-BUG-31: `file-rename` was excluded from the backup set, so a renamed rule
file had no backup entry while the command promised one and returned a
backupId that could not restore it.
- M-BUG-32: `verifyFixes` hardcoded `includeGlobal: false`, so after a
`--global` run every untouched user-scope finding was reported as verified.
Reproduced against an unmodified ~/.claude/CLAUDE.md.
- M-BUG-29: a rename was applied before other fixes on the same file, which
then failed with ENOENT while the run still exited 0. Renames sort last.
- M-BUG-30: `severityOrder[s] || 4` maps critical (0) to 4, so critical fixes
sorted last. The old test used the same falsy fallback and agreed with the
bug. Now `?? 4`.
- A failed fix exits 2 instead of 0, matching the other scanners' convention.
Frozen tests/snapshots/v5.0.0/ untouched; --json/--raw stdout byte-identical.
Suite 1420/0 (+10).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJ3MCDCnyw7wZSPnUXVhYS
This commit is contained in:
parent
1182f85767
commit
05f1e954d0
7 changed files with 334 additions and 36 deletions
|
|
@ -3,9 +3,9 @@ import assert from 'node:assert/strict';
|
|||
import { resolve, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { cp, rm, readFile, stat } from 'node:fs/promises';
|
||||
import { mkdirSync, existsSync, readdirSync } from 'node:fs';
|
||||
import { mkdirSync, existsSync, readdirSync, readFileSync, writeFileSync, rmSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { execFileSync } from 'node:child_process';
|
||||
import { execFileSync, spawnSync } from 'node:child_process';
|
||||
import { hermeticEnv, HERMETIC_HOME } from '../helpers/hermetic-home.mjs';
|
||||
|
||||
const __dirname = fileURLToPath(new URL('.', import.meta.url));
|
||||
|
|
@ -111,3 +111,111 @@ describe('fix-cli --apply', () => {
|
|||
assert.ok(output.verified.length > 0, 'Should have verified fixes');
|
||||
});
|
||||
});
|
||||
|
||||
// --- v5.14 hardening (økt #45, chunk `fix`) --------------------------------
|
||||
// Every case below was measured against the pre-fix CLI and recorded in
|
||||
// docs/fix-fasit.local.md (F1-F8 + M-BUG-29) before a line of it was fixed.
|
||||
|
||||
/** Run the CLI and return { status, stdout, stderr } without throwing on non-zero. */
|
||||
function runCli(args, opts = {}) {
|
||||
const res = spawnSync('node', [FIX_CLI, ...args], {
|
||||
encoding: 'utf-8',
|
||||
timeout: 30000,
|
||||
env: hermeticEnv(),
|
||||
...opts,
|
||||
});
|
||||
return { status: res.status, stdout: res.stdout || '', stderr: res.stderr || '' };
|
||||
}
|
||||
|
||||
describe('fix-cli argument validation (F1/F3)', () => {
|
||||
it('rejects an unknown flag instead of swallowing its value as the target', () => {
|
||||
const outFile = join(tmpdir(), `ca-fix-argtest-${Date.now()}.json`);
|
||||
const { status, stderr } = runCli([FIXABLE, '--bogus', outFile, '--json']);
|
||||
assert.strictEqual(status, 3, 'Unknown flag must fail loud with exit 3');
|
||||
assert.match(stderr, /--bogus/, 'Error must name the offending flag');
|
||||
assert.ok(!existsSync(outFile), 'No file may be written for a rejected run');
|
||||
});
|
||||
|
||||
it('rejects --output-file with no value', () => {
|
||||
const { status, stderr } = runCli([FIXABLE, '--output-file']);
|
||||
assert.strictEqual(status, 3, 'Missing flag value must fail loud');
|
||||
assert.match(stderr, /--output-file/, 'Error must name the flag missing its value');
|
||||
});
|
||||
|
||||
it('accepts --dry-run, the flag commands/fix.md documents', () => {
|
||||
const { status, stdout } = runCli([FIXABLE, '--dry-run', '--json']);
|
||||
assert.strictEqual(status, 0, '--dry-run must be a supported flag');
|
||||
const output = JSON.parse(stdout);
|
||||
assert.strictEqual(output.backupId, null, '--dry-run must not create a backup');
|
||||
});
|
||||
});
|
||||
|
||||
describe('fix-cli --output-file (F2)', () => {
|
||||
it('writes the JSON payload to the file and leaves stdout byte-stable', () => {
|
||||
const outFile = join(tmpdir(), `ca-fix-out-${Date.now()}.json`);
|
||||
const piped = runCli([FIXABLE, '--json']);
|
||||
const written = runCli([FIXABLE, '--json', '--output-file', outFile]);
|
||||
assert.strictEqual(written.status, 0);
|
||||
assert.ok(existsSync(outFile), '--output-file must produce a file');
|
||||
const fromFile = JSON.parse(readFileSync(outFile, 'utf-8'));
|
||||
assert.ok(Array.isArray(fromFile.planned), 'File must carry the fix payload');
|
||||
assert.strictEqual(
|
||||
written.stdout,
|
||||
piped.stdout,
|
||||
'--json stdout is frozen v5.0.0 — --output-file may not change it',
|
||||
);
|
||||
rmSync(outFile, { force: true });
|
||||
});
|
||||
|
||||
it('writes the payload in default mode too, where stdout carries nothing', () => {
|
||||
const outFile = join(tmpdir(), `ca-fix-out-default-${Date.now()}.json`);
|
||||
const { status } = runCli([FIXABLE, '--output-file', outFile]);
|
||||
assert.strictEqual(status, 0);
|
||||
const fromFile = JSON.parse(readFileSync(outFile, 'utf-8'));
|
||||
assert.ok(Array.isArray(fromFile.planned), 'Default mode must also write the payload');
|
||||
rmSync(outFile, { force: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe('fix-cli exit codes (F8)', () => {
|
||||
it('exits 2 when a planned fix failed', async () => {
|
||||
// Two fixes on one file: the rename and the frontmatter edit. Before the
|
||||
// ordering fix this pair guaranteed one ENOENT failure (M-BUG-29).
|
||||
const dir = await createTmpCopy();
|
||||
const rulesDir = join(dir, '.claude', 'rules');
|
||||
mkdirSync(rulesDir, { recursive: true });
|
||||
writeFileSync(join(rulesDir, 'both.txt'), '---\nglobs: "**/*.ts"\n---\n\nBody.\n');
|
||||
const { status, stdout } = runCli([dir, '--apply', '--json']);
|
||||
const output = JSON.parse(stdout);
|
||||
if (output.failed.length > 0) {
|
||||
assert.strictEqual(status, 2, 'A failed fix must not be reported as exit 0');
|
||||
} else {
|
||||
assert.strictEqual(status, 0, 'A clean run stays exit 0');
|
||||
}
|
||||
await rm(dir, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe('fix-cli backup completeness (F4)', () => {
|
||||
it('backs up a file whose only fix is a rename', async () => {
|
||||
const dir = await createTmpCopy();
|
||||
const rulesDir = join(dir, '.claude', 'rules');
|
||||
mkdirSync(rulesDir, { recursive: true });
|
||||
// Valid `paths:` frontmatter — the extension is the only defect, so
|
||||
// file-rename is the only planned fix for this file.
|
||||
writeFileSync(join(rulesDir, 'renameonly.txt'), '---\npaths: ["**/*.ts"]\n---\n\nBody.\n');
|
||||
|
||||
const { stdout } = runCli([dir, '--apply', '--json']);
|
||||
const output = JSON.parse(stdout);
|
||||
const renames = output.applied.filter((a) => /renameonly\.txt$/.test(a.file));
|
||||
assert.strictEqual(renames.length, 1, 'The rename must have been applied');
|
||||
|
||||
const backupDir = join(HERMETIC_HOME, '.claude', 'config-audit', 'backups', output.backupId);
|
||||
const backedUp = readdirSync(join(backupDir, 'files'));
|
||||
assert.ok(
|
||||
backedUp.some((f) => /renameonly\.txt$/.test(f)),
|
||||
'commands/fix.md promises a mandatory backup — a renamed file must be recoverable',
|
||||
);
|
||||
await rm(dir, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ import { describe, it, beforeEach, afterEach } from 'node:test';
|
|||
import assert from 'node:assert/strict';
|
||||
import { resolve, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { cp, rm, readFile, stat } from 'node:fs/promises';
|
||||
import { cp, rm, readFile, writeFile, stat } from 'node:fs/promises';
|
||||
import { mkdirSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { resetCounter } from '../../scanners/lib/output.mjs';
|
||||
|
|
@ -45,14 +45,39 @@ describe('planFixes', () => {
|
|||
assert.ok(result.fixes.length > 0, 'Should have at least one fix');
|
||||
});
|
||||
|
||||
it('sorts fixes by severity (critical first)', () => {
|
||||
it('sorts fixes by severity (critical first), renames last', () => {
|
||||
const result = planFixes(envelope);
|
||||
// `?? 4`, not `|| 4`: critical weighs 0, and `0 || 4` is 4. The old
|
||||
// assertion used the same falsy fallback as the implementation, so it
|
||||
// agreed with the bug instead of catching it (M-BUG-30).
|
||||
const severityOrder = { critical: 0, high: 1, medium: 2, low: 3, info: 4 };
|
||||
for (let i = 1; i < result.fixes.length; i++) {
|
||||
const prev = severityOrder[result.fixes[i - 1].severity] || 4;
|
||||
const curr = severityOrder[result.fixes[i].severity] || 4;
|
||||
// A file-rename is ordered last on purpose (M-BUG-29) — severity ranks
|
||||
// within the non-rename group.
|
||||
const ranked = result.fixes.filter((f) => f.type !== FIX_TYPES.FILE_RENAME);
|
||||
for (let i = 1; i < ranked.length; i++) {
|
||||
const prev = severityOrder[ranked[i - 1].severity] ?? 4;
|
||||
const curr = severityOrder[ranked[i].severity] ?? 4;
|
||||
assert.ok(prev <= curr, `Fix ${i} should not have higher severity than fix ${i - 1}`);
|
||||
}
|
||||
const renames = result.fixes.filter((f) => f.type === FIX_TYPES.FILE_RENAME);
|
||||
if (renames.length > 0) {
|
||||
assert.strictEqual(
|
||||
result.fixes.at(-1).type,
|
||||
FIX_TYPES.FILE_RENAME,
|
||||
'Renames must sort after every other fix',
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
it('puts a critical fix first (M-BUG-30)', () => {
|
||||
const result = planFixes(envelope);
|
||||
const criticals = result.fixes.filter((f) => f.severity === 'critical');
|
||||
if (criticals.length === 0) return;
|
||||
assert.strictEqual(
|
||||
result.fixes[0].severity,
|
||||
'critical',
|
||||
'critical weighs 0 — a `|| 4` fallback sorted it last, the opposite of the contract',
|
||||
);
|
||||
});
|
||||
|
||||
it('includes manual findings with recommendations', () => {
|
||||
|
|
@ -303,3 +328,75 @@ describe('verifyFixes', () => {
|
|||
assert.ok(verification.verified.length > 0, 'Should verify at least one fix');
|
||||
});
|
||||
});
|
||||
|
||||
// --- v5.14 hardening (økt #45, chunk `fix`) --------------------------------
|
||||
|
||||
describe('planFixes ordering (M-BUG-29)', () => {
|
||||
it('applies a file rename after every other fix touching the same file', async () => {
|
||||
const dir = await createTmpCopy();
|
||||
const rulesDir = join(dir, '.claude', 'rules');
|
||||
mkdirSync(rulesDir, { recursive: true });
|
||||
// Both defects at once: undocumented `globs:` AND a non-.md extension.
|
||||
await writeFile(join(rulesDir, 'both.txt'), '---\nglobs: "**/*.ts"\n---\n\nBody.\n');
|
||||
|
||||
resetCounter();
|
||||
const envelope = await runAllScanners(dir, { includeGlobal: false });
|
||||
const { fixes } = planFixes(envelope);
|
||||
|
||||
const onFile = fixes.filter((f) => /both\.txt$/.test(f.file));
|
||||
assert.strictEqual(onFile.length, 2, 'Both defects on the file must be planned');
|
||||
assert.strictEqual(
|
||||
onFile[onFile.length - 1].type,
|
||||
FIX_TYPES.FILE_RENAME,
|
||||
'A rename moves the file out from under later fixes — it must run last',
|
||||
);
|
||||
|
||||
// …and the whole batch must therefore apply cleanly.
|
||||
const backupDir = join(dir, '.backup-test');
|
||||
mkdirSync(backupDir, { recursive: true });
|
||||
const result = await applyFixes(fixes, { dryRun: false, backupDir });
|
||||
assert.deepStrictEqual(result.failed, [], 'No fix may fail because of ordering');
|
||||
|
||||
await rm(dir, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe('verifyFixes scope (F5)', () => {
|
||||
it('re-scans with the same includeGlobal scope the fix run used', async () => {
|
||||
const dir = await createTmpCopy();
|
||||
const fakeHome = join(tmpdir(), `ca-verify-home-${Date.now()}`);
|
||||
mkdirSync(join(fakeHome, '.claude'), { recursive: true });
|
||||
// A user-scope CLAUDE.md long enough to trip the CML line-count finding.
|
||||
await writeFile(
|
||||
join(fakeHome, '.claude', 'CLAUDE.md'),
|
||||
Array.from({ length: 260 }, (_, i) => `Line ${i + 1}`).join('\n') + '\n',
|
||||
);
|
||||
|
||||
const originalHome = process.env.HOME;
|
||||
process.env.HOME = fakeHome;
|
||||
try {
|
||||
resetCounter();
|
||||
const envelope = await runAllScanners(dir, { includeGlobal: true });
|
||||
const globalOnly = envelope.scanners
|
||||
.flatMap((s) => s.findings)
|
||||
.filter((f) => f.file && !f.file.startsWith(dir));
|
||||
assert.ok(globalOnly.length > 0, 'Fixture must produce at least one global-scope finding');
|
||||
|
||||
const victim = globalOnly[0];
|
||||
// Nothing was changed on disk — claiming it was applied must NOT verify it.
|
||||
const res = await verifyFixes(
|
||||
envelope,
|
||||
[{ findingId: victim.id, file: victim.file, status: 'applied' }],
|
||||
{ includeGlobal: true },
|
||||
);
|
||||
assert.ok(
|
||||
!res.verified.includes(victim.id),
|
||||
'An untouched global-scope finding must not be reported as verified',
|
||||
);
|
||||
} finally {
|
||||
if (originalHome === undefined) delete process.env.HOME; else process.env.HOME = originalHome;
|
||||
await rm(fakeHome, { recursive: true, force: true });
|
||||
await rm(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue