fix: close what an independent review found in the trial 2 fixes
- The pre-compact hook read only a checkpoint that started with `git commit`,
so every plan in the new `git add … && git commit` form rebuilt nothing, and
an empty match also skipped the session-state refresh. It now finds the
commit anywhere in the span and always refreshes; first test for the hook.
- A sub-bullet that carries its own command stays a criterion (folding it
left the command unrun); the shallowest bullet sets the base indent.
- Only a MINOR/SUGGESTION /code-review finding outside the range is released
from forbidding ALLOW: a MAJOR one may be a break cited at an unchanged
caller.
- `reuse` is an improvement category; the session-spec template stages too;
a negated bash 3.2 constraint, an env prefix and `bash -c` are handled.
- Stale comment, Declined-to-Judge wording and rule-key counts brought in line.
Tests fail on 7c11c2a first (9 of 24).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
7c11c2aa9f
commit
b0d96a2d53
13 changed files with 194 additions and 54 deletions
72
tests/hooks/pre-compact-flush.test.mjs
Normal file
72
tests/hooks/pre-compact-flush.test.mjs
Normal file
|
|
@ -0,0 +1,72 @@
|
|||
// tests/hooks/pre-compact-flush.test.mjs
|
||||
// The PreCompact hook rebuilds progress.json from git and refreshes the
|
||||
// session state before compaction. An independent review (2026-10-08) found
|
||||
// that the checkpoint form `git add … && git commit -m "…"` (plan template,
|
||||
// lekerepo trial 2 point A7) matched nothing, and that an empty match skipped
|
||||
// the session-state refresh too. No test covered the hook until now.
|
||||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
|
||||
const HERE = dirname(fileURLToPath(import.meta.url));
|
||||
const HOOK = join(HERE, '..', '..', 'hooks', 'scripts', 'pre-compact-flush.mjs');
|
||||
|
||||
function git(cwd, ...args) {
|
||||
const r = spawnSync('git', args, { cwd, encoding: 'utf8' });
|
||||
assert.equal(r.status, 0, `git ${args.join(' ')}: ${r.stderr}`);
|
||||
return r.stdout.trim();
|
||||
}
|
||||
|
||||
function project(checkpoint, { stateStatus = 'in_progress' } = {}) {
|
||||
const repo = mkdtempSync(join(tmpdir(), 'voyage-precompact-'));
|
||||
git(repo, 'init', '-q');
|
||||
git(repo, 'config', 'user.email', 't@example.invalid');
|
||||
git(repo, 'config', 'user.name', 'test');
|
||||
writeFileSync(join(repo, 'a.txt'), 'a\n');
|
||||
git(repo, 'add', 'a.txt');
|
||||
git(repo, 'commit', '-q', '-m', 'base');
|
||||
const base = git(repo, 'rev-parse', 'HEAD');
|
||||
writeFileSync(join(repo, 'a.txt'), 'b\n');
|
||||
git(repo, 'add', 'a.txt');
|
||||
git(repo, 'commit', '-q', '-m', 'feat(a): change a');
|
||||
const dir = join(repo, '.claude', 'projects', 'p');
|
||||
mkdirSync(dir, { recursive: true });
|
||||
writeFileSync(join(dir, 'plan.md'), ['# Plan', '', '### Step 1: Change a', '', `- **Checkpoint:** ${checkpoint}`, ''].join('\n'));
|
||||
writeFileSync(join(dir, 'progress.json'), JSON.stringify({ status: 'in_progress', current_step: 0, steps: {}, session_start_sha: base }));
|
||||
writeFileSync(join(dir, '.session-state.local.json'), JSON.stringify({ status: stateStatus, updated_at: '2000-01-01T00:00:00.000Z' }));
|
||||
return { repo, dir };
|
||||
}
|
||||
|
||||
function run(repo) {
|
||||
return spawnSync(process.execPath, [HOOK], { input: '{}', encoding: 'utf8', env: { ...process.env, CLAUDE_PROJECT_DIR: repo } });
|
||||
}
|
||||
|
||||
const progress = (dir) => JSON.parse(readFileSync(join(dir, 'progress.json'), 'utf8'));
|
||||
const state = (dir) => JSON.parse(readFileSync(join(dir, '.session-state.local.json'), 'utf8'));
|
||||
|
||||
for (const [name, cp] of [
|
||||
['the old form', '`git commit -m "feat(a): change a"`'],
|
||||
['the template form with git add', '`git add a.txt && git commit -m "feat(a): change a"`'],
|
||||
]) {
|
||||
test(`pre-compact: ${name} -> progress.json is rebuilt from git`, () => {
|
||||
const { repo, dir } = project(cp);
|
||||
try {
|
||||
assert.equal(run(repo).status, 0);
|
||||
assert.equal(progress(dir).current_step, 1);
|
||||
} finally { rmSync(repo, { recursive: true, force: true }); }
|
||||
});
|
||||
}
|
||||
|
||||
test('pre-compact: a plan with no recognisable checkpoint still refreshes the session state', () => {
|
||||
const { repo, dir } = project('`make commit`');
|
||||
try {
|
||||
assert.equal(run(repo).status, 0);
|
||||
assert.equal(progress(dir).current_step, 0);
|
||||
assert.notEqual(state(dir).updated_at, '2000-01-01T00:00:00.000Z');
|
||||
} finally { rmSync(repo, { recursive: true, force: true }); }
|
||||
});
|
||||
|
|
@ -72,14 +72,21 @@ test('A2: /trekreview asks the transcription for the engine\'s fix text and forb
|
|||
|
||||
// --- A3. an out-of-range /code-review finding does not lock the verdict -----
|
||||
|
||||
test('A3: a /code-review finding outside the exact range it was given is declined, and does not hold ALLOW back', () => {
|
||||
const out = { ...engineFinding({ file: 'tests/cli.test.mjs', line: 1 }), engine: 'code-review', rule_key: 'CODE_REVIEW_DEFECT', severity: 'MAJOR', title: 'No CLI test for --sep' };
|
||||
test('A3: a small /code-review finding outside the exact range it was given is declined, and does not hold ALLOW back', () => {
|
||||
const out = { ...engineFinding({ file: 'src/util.mjs', line: 4 }), engine: 'code-review', rule_key: 'CODE_REVIEW_IMPROVEMENT', severity: 'MINOR', title: 'Simplify the helper' };
|
||||
const c = runContract([], { engineOutOfScope: [out] });
|
||||
assert.equal(c.verdict, 'ALLOW', JSON.stringify(c.allow_blocked_by));
|
||||
assert.equal(c.declined_to_judge.length, 1);
|
||||
assert.equal(c.declined_to_judge[0].reason, 'engine:out-of-scope');
|
||||
});
|
||||
|
||||
test('A3 (independent review): a MAJOR /code-review defect cited at an unchanged file still forbids ALLOW — a cross-file break is cited there', () => {
|
||||
const out = { ...engineFinding({ file: 'tests/cli.test.mjs', line: 1 }), engine: 'code-review', rule_key: 'CODE_REVIEW_DEFECT', severity: 'MAJOR', title: 'No CLI test for --sep' };
|
||||
const c = runContract([], { engineOutOfScope: [out] });
|
||||
assert.notEqual(c.verdict, 'ALLOW');
|
||||
assert.equal(c.declined_to_judge.length, 1);
|
||||
});
|
||||
|
||||
test('A3: a /security-review finding outside the range still forbids ALLOW (its base is wider and not ours)', () => {
|
||||
const out = { ...engineFinding({ file: 'lib/auth.mjs' }), engine: 'security-review', rule_key: 'SECURITY_REVIEW_VULNERABILITY', severity: 'BLOCKER', title: 'Token logged' };
|
||||
const c = runContract([], { engineOutOfScope: [out] });
|
||||
|
|
@ -170,3 +177,49 @@ test('A7: every checkpoint the plan template and /trekplan\'s example show stage
|
|||
assert.ok(lines.length >= 3, `found ${lines.length}`);
|
||||
for (const l of lines) assert.match(l, /`git add [^`]+ && git commit -m /, l);
|
||||
});
|
||||
|
||||
// --- the independent review of these fixes (2026-10-08) ----------------------
|
||||
|
||||
test('A1 (review): a `reuse` finding is an improvement too', () => {
|
||||
const r = toReviewerPayload({ engine: 'code-review', findings: [engineFinding({ category: 'Reuse ' })] });
|
||||
assert.equal(r.payload.findings[0].severity, 'MINOR');
|
||||
});
|
||||
|
||||
test('A5 (review): a sub-bullet that carries its own command stays a criterion; only command-less sub-points fold', () => {
|
||||
const sc = parseSuccessCriteria(brief(['## Success Criteria', '',
|
||||
'- Behaviour:', ' - `npm test` exits 0', ' - `bash tests/e2e.sh` exits 0', ' - reads well', '']));
|
||||
assert.deepEqual(sc.map((c) => c.command), [null, 'npm test', 'bash tests/e2e.sh']);
|
||||
assert.equal(sc[0].subpoints, 1);
|
||||
});
|
||||
|
||||
test('A5 (review): an indented first bullet does not turn later top-level bullets into sub-points', () => {
|
||||
const sc = parseSuccessCriteria(brief(['## Success Criteria', '',
|
||||
' - The first, indented by accident.', '- The second:', ' - a sub-point of the second', '- `npm test` exits 0.', '']));
|
||||
assert.equal(sc.length, 3, JSON.stringify(sc.map((c) => c.text)));
|
||||
assert.equal(sc[1].subpoints, 1);
|
||||
});
|
||||
|
||||
test('A6 (review): a negated bash 3.2 constraint, an env prefix and `bash -c` are handled', () => {
|
||||
const neg = validateBriefContent(brief(['## Constraints', '', '- bash 3.2 is not required here.', '',
|
||||
'## Success Criteria', '', '- `bash tests/contract.sh` exits 0.', '']));
|
||||
assert.ok(!codes(neg.warnings).includes('BRIEF_SC_BASH_NOT_PINNED'), 'negated constraint');
|
||||
const env = validateBriefContent(brief([...BASH32, '## Success Criteria', '', '- `LC_ALL=C bash tests/contract.sh` exits 0.', '']));
|
||||
assert.ok(codes(env.warnings).includes('BRIEF_SC_BASH_NOT_PINNED'), 'env-prefixed bash');
|
||||
const dashC = validateBriefContent(brief([...BASH32, '## Success Criteria', '', '- `bash -c true` exits 0.', '']));
|
||||
for (const w of dashC.warnings.filter((x) => x.code === 'BRIEF_SC_BASH_NOT_PINNED')) {
|
||||
assert.doesNotMatch(w.hint, /\/bin\/bash -c/, 'never suggests a form the runner refuses');
|
||||
}
|
||||
});
|
||||
|
||||
test('A7 (review): the session-spec template stages before it commits too', () => {
|
||||
const l = read('templates/session-spec-template.md').split('\n').filter((x) => /\*\*Checkpoint:\*\*/.test(x) && /`git [^`]*commit -m /.test(x));
|
||||
assert.ok(l.length >= 1);
|
||||
for (const x of l) assert.match(x, /`git add [^`]+ && git commit -m /, x);
|
||||
});
|
||||
|
||||
test('docs (review): the counts and the Declined-to-Judge rule match the code', () => {
|
||||
assert.match(read('docs/architecture.md'), /rule catalogue \(15 keys\)/);
|
||||
assert.match(read('docs/HANDOVER-CONTRACTS.md'), /three engine rule keys/);
|
||||
assert.doesNotMatch(read('lib/review/engine-findings.mjs'), /it forbids ALLOW and is listed under/);
|
||||
assert.match(read('agents/review-coordinator.md'), /every `\[unverified\]` one, and every\s+out-of-scope engine finding/);
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue