Kjell Tore Guttormsen
40f6ae1177
An adversarial read of d88245c reproduced 15 problems; all fixed here, each
with a test that fails on the old code.
- verdict-gate: a greener verdict must be backed by the LAST fence being a
`json` block of the right shape, with no BLOCKER in the JSON (any case),
in `## Findings (BLOCKER)`, or a differing finding-id list; two verdict
lines and more than one file argument are exit 2. ({}, null, lowercase
"blocker", a trailing ```jsonc and a later json fence all gave exit 0.)
- engine-findings: a transcribed "narrower" key can no longer lower an
engine tier; absolute paths under --repo-root match the scope; "12" lines
parse; an empty scope is an error; out-of-scope engine findings are
unverified (`engine:out-of-scope`) and forbid ALLOW.
- recurrence: the catch-all engine keys and keys normalised from unknown
ones are never proposed; the same unfixed finding in two iterations is not
a recurrence; proposals are validated before they are offered; \r/U+2028
and damaged markers are refused; writes go through a symlinked CLAUDE.md.
Prose points --repo at the session directory, not the git toplevel.
- Workflow substrate: engines are counted from what arrived, and the
verdict's floor is recomputed from the coordinator's findings; now tested
by running the script with stubbed Workflow globals. Two literal NUL bytes
written as \u0000 (git saw the file as binary).
- Trimming keeps Pass 1's "Also flagged by" provenance; `## Declined to
Judge` and remediation steps are required from review_version 1.1 only
(N-1); the zero-diff review now carries the JSON block the gate needs.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
224 lines
9.2 KiB
JavaScript
224 lines
9.2 KiB
JavaScript
import { test } from 'node:test';
|
|
import { strict as assert } from 'node:assert';
|
|
import { spawnSync } from 'node:child_process';
|
|
import { mkdtempSync, writeFileSync, rmSync, symlinkSync } from 'node:fs';
|
|
import { tmpdir } from 'node:os';
|
|
import { join, dirname } from 'node:path';
|
|
import { fileURLToPath } from 'node:url';
|
|
import { validateReviewContent } from '../../lib/validators/review-validator.mjs';
|
|
|
|
const VALIDATOR = join(dirname(fileURLToPath(import.meta.url)), '..', '..', 'lib', 'validators', 'review-validator.mjs');
|
|
|
|
const GOOD_REVIEW = `---
|
|
type: trekreview
|
|
review_version: "1.0"
|
|
created: 2026-05-01
|
|
task: "Add JWT auth"
|
|
slug: jwt-auth
|
|
project_dir: .claude/projects/2026-04-30-jwt-auth/
|
|
brief_path: .claude/projects/2026-04-30-jwt-auth/brief.md
|
|
scope_sha_start: abc123
|
|
scope_sha_end: def456
|
|
reviewed_files_count: 7
|
|
verdict: ALLOW
|
|
findings:
|
|
- 0123456789abcdef0123456789abcdef01234567
|
|
- fedcba9876543210fedcba9876543210fedcba98
|
|
---
|
|
|
|
# Review
|
|
|
|
## Executive Summary
|
|
|
|
Verdict: ALLOW.
|
|
|
|
## Coverage
|
|
|
|
| File | Treatment | Reason |
|
|
|------|-----------|--------|
|
|
| lib/foo.mjs | deep-review | risk |
|
|
|
|
## Remediation Summary
|
|
|
|
None.
|
|
`;
|
|
|
|
test('validateReview — happy path', () => {
|
|
const r = validateReviewContent(GOOD_REVIEW, { strict: true });
|
|
assert.equal(r.valid, true, JSON.stringify(r.errors));
|
|
});
|
|
|
|
test('validateReview — wrong type rejected (REVIEW_WRONG_TYPE)', () => {
|
|
const t = GOOD_REVIEW.replace('type: trekreview', 'type: trekbrief');
|
|
const r = validateReviewContent(t);
|
|
assert.equal(r.valid, false);
|
|
assert.ok(r.errors.find(e => e.code === 'REVIEW_WRONG_TYPE'));
|
|
});
|
|
|
|
test('validateReview — missing required field (REVIEW_MISSING_FIELD)', () => {
|
|
const t = GOOD_REVIEW.replace(/^brief_path: .*\n/m, '');
|
|
const r = validateReviewContent(t);
|
|
assert.equal(r.valid, false);
|
|
assert.ok(r.errors.find(e => e.code === 'REVIEW_MISSING_FIELD' && /brief_path/.test(e.message)));
|
|
});
|
|
|
|
test('validateReview — missing required body section in strict (REVIEW_MISSING_SECTION)', () => {
|
|
const t = GOOD_REVIEW.replace(/## Coverage[\s\S]*?(?=## Remediation)/m, '');
|
|
const r = validateReviewContent(t, { strict: true });
|
|
assert.equal(r.valid, false);
|
|
assert.ok(r.errors.find(e => e.code === 'REVIEW_MISSING_SECTION' && /Coverage/.test(e.message)));
|
|
});
|
|
|
|
test('validateReview — Coverage section is REQUIRED (no soft demotion to make Coverage optional)', () => {
|
|
const t = GOOD_REVIEW.replace(/## Coverage[\s\S]*?(?=## Remediation)/m, '');
|
|
const r = validateReviewContent(t, { strict: true });
|
|
assert.equal(r.valid, false);
|
|
});
|
|
|
|
test('validateReview — soft mode demotes section errors to warnings', () => {
|
|
const t = GOOD_REVIEW.replace(/## Remediation Summary[\s\S]*$/m, '');
|
|
const r = validateReviewContent(t, { strict: false });
|
|
assert.equal(r.valid, true);
|
|
assert.ok(r.warnings.find(w => w.code === 'REVIEW_MISSING_SECTION'));
|
|
});
|
|
|
|
test('validateReview — missing frontmatter is hard error (FM_MISSING)', () => {
|
|
const r = validateReviewContent('# review\n\nno frontmatter\n');
|
|
assert.equal(r.valid, false);
|
|
assert.ok(r.errors.find(e => e.code === 'FM_MISSING'));
|
|
});
|
|
|
|
test('validateReview — findings not an array → REVIEW_BAD_FINDINGS_TYPE', () => {
|
|
// Replace block-style list with scalar → parser yields string
|
|
const t = GOOD_REVIEW.replace(
|
|
/findings:\n - 0123[\s\S]*?- fedcba[0-9a-f]+/,
|
|
'findings: not-an-array',
|
|
);
|
|
const r = validateReviewContent(t);
|
|
assert.equal(r.valid, false);
|
|
assert.ok(
|
|
r.errors.find(e => e.code === 'REVIEW_BAD_FINDINGS_TYPE'),
|
|
`expected REVIEW_BAD_FINDINGS_TYPE, got: ${JSON.stringify(r.errors)}`,
|
|
);
|
|
});
|
|
|
|
test('validateReview — finding-ID not 40-char hex → REVIEW_BAD_FINDING_ID', () => {
|
|
const t = GOOD_REVIEW.replace(
|
|
'0123456789abcdef0123456789abcdef01234567',
|
|
'NOT-A-VALID-HEX-ID',
|
|
);
|
|
const r = validateReviewContent(t);
|
|
assert.equal(r.valid, false);
|
|
assert.ok(r.errors.find(e => e.code === 'REVIEW_BAD_FINDING_ID'));
|
|
});
|
|
|
|
test('validateReview — empty findings array is acceptable (no findings = ALLOW verdict)', () => {
|
|
const t = GOOD_REVIEW.replace(
|
|
/findings:\n - 0123[\s\S]*?- fedcba[0-9a-f]+/,
|
|
'findings: []',
|
|
);
|
|
const r = validateReviewContent(t);
|
|
assert.equal(r.valid, true, JSON.stringify(r.errors));
|
|
});
|
|
|
|
// ---- K7: the verdict is required in review.md (ordre 5) ----------------------
|
|
|
|
const NO_VERDICT = GOOD_REVIEW.replace(/^verdict: .*\n/m, '');
|
|
|
|
test('validateReview — strict mode rejects a review.md without verdict (REVIEW_MISSING_VERDICT)', () => {
|
|
assert.ok(!/^verdict:/m.test(NO_VERDICT), 'fixture control: the verdict line is gone');
|
|
const r = validateReviewContent(NO_VERDICT, { strict: true });
|
|
assert.equal(r.valid, false, 'a review without a verdict must not pass the write-time check');
|
|
assert.ok(r.errors.find(e => e.code === 'REVIEW_MISSING_VERDICT'), JSON.stringify(r.errors));
|
|
});
|
|
|
|
test('validateReview — soft (read-time) mode only warns on a missing verdict (N-1 window for 1.0 reviews)', () => {
|
|
const r = validateReviewContent(NO_VERDICT, { strict: false });
|
|
assert.equal(r.valid, true, JSON.stringify(r.errors));
|
|
assert.ok(r.warnings.find(w => w.code === 'REVIEW_MISSING_VERDICT'));
|
|
});
|
|
|
|
test('validateReview — a verdict outside ALLOW|WARN|BLOCK is an error in both modes', () => {
|
|
const t = GOOD_REVIEW.replace('verdict: ALLOW', 'verdict: PASS');
|
|
for (const strict of [true, false]) {
|
|
const r = validateReviewContent(t, { strict });
|
|
assert.equal(r.valid, false);
|
|
assert.ok(r.errors.find(e => e.code === 'REVIEW_BAD_VERDICT'));
|
|
}
|
|
});
|
|
|
|
// ---- A BLOCKER is written as a plan step Handover 6 can append (ordre 5) ----
|
|
|
|
const BLOCKER_ID = '763d174e6c519fafbadcba5d1706708479e36e61';
|
|
const WITH_BLOCKER = GOOD_REVIEW.replace('verdict: ALLOW', 'verdict: BLOCK')
|
|
.replace('review_version: "1.0"', 'review_version: "1.1"')
|
|
.replace('## Remediation Summary', '## Declined to Judge\n\nNone.\n\nTrimmed: 0\n\n## Remediation Summary') + `
|
|
\`\`\`json
|
|
{"verdict": "BLOCK", "counts": {"BLOCKER": 1, "MAJOR": 0, "MINOR": 0, "SUGGESTION": 0},
|
|
"findings": [{"id": "${BLOCKER_ID}", "severity": "BLOCKER", "rule_key": "UNIMPLEMENTED_CRITERION", "file": "lib/login.mjs", "line": 23}]}
|
|
\`\`\`
|
|
`;
|
|
const STEP = `
|
|
## Remediation Steps
|
|
|
|
### Step R1: Return 401 on a bad password
|
|
|
|
- **Files:** \`lib/login.mjs\`
|
|
- **Source finding:** \`${BLOCKER_ID}\`
|
|
- **Verify:** re-run /trekreview → finding \`${BLOCKER_ID}\` absent
|
|
`;
|
|
|
|
test('validateReview — strict: a BLOCKER without a numbered remediation step is an error', () => {
|
|
const r = validateReviewContent(WITH_BLOCKER, { strict: true });
|
|
assert.equal(r.valid, false);
|
|
assert.ok(r.errors.find(e => e.code === 'REVIEW_MISSING_REMEDIATION_STEP'), JSON.stringify(r.errors));
|
|
});
|
|
|
|
test('validateReview — strict: a BLOCKER with its "Step R1 … Source finding" passes', () => {
|
|
const t = WITH_BLOCKER.replace('## Remediation Summary', `${STEP}\n## Remediation Summary`);
|
|
const r = validateReviewContent(t, { strict: true });
|
|
assert.equal(r.valid, true, JSON.stringify(r.errors));
|
|
});
|
|
|
|
test('validateReview — a 1.0 review with a BLOCKER is not held to the 1.1 remediation-step rule (N-1)', () => {
|
|
const v10 = WITH_BLOCKER.replace('review_version: "1.1"', 'review_version: "1.0"');
|
|
assert.equal(validateReviewContent(v10, { strict: true }).valid, true);
|
|
});
|
|
|
|
test('validateReview — soft: a missing remediation step only warns', () => {
|
|
const r = validateReviewContent(WITH_BLOCKER, { strict: false });
|
|
assert.equal(r.valid, true);
|
|
assert.ok(r.warnings.find(w => w.code === 'REVIEW_MISSING_REMEDIATION_STEP'));
|
|
});
|
|
|
|
test('validateReview — a clean review with zero findings validates with `findings: []` (a bare `findings:` does not)', () => {
|
|
const ids = /findings:\n(?: - [0-9a-f]{40}\n)+/;
|
|
assert.match(GOOD_REVIEW, ids, 'fixture control');
|
|
assert.equal(validateReviewContent(GOOD_REVIEW.replace(ids, 'findings: []\n'), { strict: true }).valid, true);
|
|
assert.equal(validateReviewContent(GOOD_REVIEW.replace(ids, 'findings:\n'), { strict: true }).valid, false);
|
|
});
|
|
|
|
test('validateReview — strict: a 1.1 review needs ## Declined to Judge; a 1.0 review does not', () => {
|
|
const v11 = GOOD_REVIEW.replace('review_version: "1.0"', 'review_version: "1.1"');
|
|
const r = validateReviewContent(v11, { strict: true });
|
|
assert.equal(r.valid, false);
|
|
assert.ok(r.errors.find(e => e.code === 'REVIEW_MISSING_SECTION' && /Declined to Judge/.test(e.message)));
|
|
const fixed = v11.replace('## Remediation Summary', '## Declined to Judge\n\nNone.\n\nTrimmed: 0\n\n## Remediation Summary');
|
|
assert.equal(validateReviewContent(fixed, { strict: true }).valid, true);
|
|
assert.equal(validateReviewContent(GOOD_REVIEW, { strict: true }).valid, true, '1.0 stays valid');
|
|
});
|
|
|
|
test('review-validator CLI — run through a symlink and a path with a space it still validates (exit 1, not a silent 0)', () => {
|
|
const dir = mkdtempSync(join(tmpdir(), 'review validator '));
|
|
try {
|
|
const link = join(dir, 'validator link.mjs');
|
|
symlinkSync(VALIDATOR, link);
|
|
const p = join(dir, 'no verdict.md');
|
|
writeFileSync(p, NO_VERDICT);
|
|
const r = spawnSync(process.execPath, [link, p], { encoding: 'utf-8' });
|
|
assert.equal(r.status, 1, r.stdout + r.stderr);
|
|
assert.match(r.stderr, /REVIEW_MISSING_VERDICT/);
|
|
} finally {
|
|
rmSync(dir, { recursive: true, force: true });
|
|
}
|
|
});
|