fix(review): convert refuses a partial or scoped Claude Security report; the stamp check is a trace (decisions 223, 224)
Two fail-closed checks in security-engine.mjs convert (decision 224), both "not measured" (exit 2, the reason on stderr, nothing on stdout): - the stamp's findings.total is not a non-negative integer, or the JSONL holds another number of records (a JSONL that lost the only BLOCKER line converted clean before; part 3 verifier, F3); - the stamp's scope is not an empty list: a scan limited by --scope, an absent scope or a non-list scope (Claude Security 0.12.0 writes scope: [] for an unscoped scan; F4). The real Claude Security 0.12.0 probe report still converts: exit 0, one BLOCKER, two below_threshold. Decision 223, text only: the module header and SECURITY.md keep the head/base check and say it is a trace, not a signature: code the reviewed run executed can write a report folder of its own (convert reads the matching one whose name sorts last) or delete the real one first (F2, a known issue). /trekreview's not-measured list names the two new reasons. The test helper writes the real stamp shape by default (findings.total = the records in the JSONL written, scope: []). New tests for behaviour that had none (F6): no fallback from an unverified newest match, a two-stamp folder passed over, a symlinked stamp, JSONL or folder refused, a malformed or absent scan_prefix, an unrecorded empty_diff or empty_scope. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
3be2bd3386
commit
f6e8529bdd
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
5 changed files with 179 additions and 13 deletions
10
SECURITY.md
10
SECURITY.md
|
|
@ -86,9 +86,13 @@ is "not measured" and the verdict gate exits 2 (never green).
|
|||
Security's report (`CLAUDE-SECURITY-<ts>/`, written by Claude Security in the
|
||||
repo root behind its own `.gitignore`) as data: nothing in it is run or
|
||||
followed, and a stamp counts only when it names exactly the reviewed head and
|
||||
base. A Claude Security finding blocks only at confidence `high`; the rest
|
||||
are listed under Declined to Judge and keep the review from `ALLOW`. Neither
|
||||
engine edits code.
|
||||
base. That check is a trace, not a signature (a known issue): code the
|
||||
reviewed run executed can write a report folder of its own, which `convert`
|
||||
reads when its name sorts last among the matching ones, or delete the real
|
||||
report before `convert` reads it, and Voyage does not detect either. A
|
||||
Claude Security finding blocks only at confidence `high`; the rest are listed
|
||||
under Declined to Judge and keep the review from `ALLOW`. Neither engine
|
||||
edits code.
|
||||
|
||||
## Out of scope
|
||||
|
||||
|
|
|
|||
|
|
@ -427,7 +427,9 @@ coordinator as well: each one is listed under `## Declined to Judge`
|
|||
not measured (no report stamp of a changes scan of exactly
|
||||
`{before_sha}..{after_sha}` — an empty diff writes none —, a scan not
|
||||
verified, researchers that did not all return, an empty diff or scope, a
|
||||
malformed record): print the reason and record Claude Security as not run;
|
||||
scan limited by `--scope`, a malformed record, fewer or more records than
|
||||
the stamp's `findings.total`): print the reason and record Claude Security
|
||||
as not run;
|
||||
the empty file the redirect leaves is no payload. Exit 1 = the scope list is
|
||||
empty: rebuild it from Phase 3 and re-run.
|
||||
|
||||
|
|
|
|||
|
|
@ -51,13 +51,22 @@
|
|||
// stamp (an empty diff writes none), verification.status not "verified",
|
||||
// researchers_returned or researchers_dispatched missing or returned <
|
||||
// dispatched, run_shape.empty_diff or empty_scope true or not recorded, a
|
||||
// malformed scan_prefix, a missing or unreadable JSONL, a record that does not
|
||||
// parse or lacks its file, title or a confidence of low|medium|high.
|
||||
// malformed scan_prefix, findings.total not a non-negative integer, a scope
|
||||
// that is not an empty list (a scan limited by `--scope` looked at part of
|
||||
// the range; Claude Security 0.12.0 writes `scope: []` for an unscoped scan;
|
||||
// an absent or non-list scope counts the same) (decision 224), a missing or
|
||||
// unreadable JSONL, a record that does not parse or lacks its file, title or
|
||||
// a confidence of low|medium|high, a JSONL whose number of records is not
|
||||
// the stamp's findings.total (decision 224).
|
||||
// exit 1 = the scope list is empty (as engine-findings.mjs); bad usage = 2.
|
||||
//
|
||||
// The report folder lives in the repo under review, so its text is data: a
|
||||
// stamp counts only when it names this exact head and merge base, and a
|
||||
// record's text is carried, never followed.
|
||||
// record's text is carried, never followed. That check is a trace, not a
|
||||
// signature (decision 223, a known issue): code the reviewed run executed can
|
||||
// write a report folder of its own (convert reads the matching one whose name
|
||||
// sorts last) or delete the real one before convert reads it, and nothing
|
||||
// here detects either.
|
||||
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { existsSync, lstatSync, readFileSync, readdirSync } from 'node:fs';
|
||||
|
|
@ -192,6 +201,13 @@ function stampProblem(stamp) {
|
|||
|| prefix.includes('\\') || prefix.split('/').slice(0, -1).some((seg) => seg === '' || seg === '.' || seg === '..')))) {
|
||||
return `scan_prefix ${JSON.stringify(prefix ?? null)} is not a path prefix`;
|
||||
}
|
||||
const total = isPlainObject(stamp.findings) ? stamp.findings.total : undefined;
|
||||
if (!Number.isInteger(total) || total < 0) {
|
||||
return `findings.total ${JSON.stringify(total ?? null)} is not a count of findings`;
|
||||
}
|
||||
if (!Array.isArray(stamp.scope) || stamp.scope.length !== 0) {
|
||||
return `scope ${JSON.stringify(stamp.scope ?? null)} is not [] (a scan limited by --scope, or none recorded), not a scan of the whole range`;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
|
|
@ -247,6 +263,9 @@ export function convertReport({ reportDir, base, head, scopeFiles } = {}) {
|
|||
if (problem) return notMeasured(`${found.name}/${found.stampName}: ${problem}`);
|
||||
const read = readRecords(found.folder, found.stamp.scan_prefix);
|
||||
if (read.reason) return notMeasured(`${found.name}: ${read.reason}`);
|
||||
if (read.records.length !== found.stamp.findings.total) {
|
||||
return notMeasured(`${found.name}: ${JSONL_NAME} holds ${read.records.length} record(s), the stamp's findings.total is ${found.stamp.findings.total}`);
|
||||
}
|
||||
const high = read.records.filter((r) => r.confidence === 'high').map((r) => r.finding);
|
||||
const rest = read.records.filter((r) => r.confidence !== 'high').map((r) => r.finding);
|
||||
const counted = toReviewerPayload({ engine: ENGINE, findings: high }, { scopeFiles });
|
||||
|
|
|
|||
|
|
@ -12,6 +12,11 @@
|
|||
//
|
||||
// `writeReport` takes the commit range and lets a test change any part of the
|
||||
// stamp (`stamp: (s) => ...`) or write the JSONL text itself (`jsonl`).
|
||||
// By default the stamp has the real shape of an unscoped changes scan:
|
||||
// `findings.total` is the number of records in the JSONL written (the
|
||||
// non-empty lines of `jsonl` when a test gives it; render_report.py writes
|
||||
// `len(findings)`), and `scope` is `[]` (write_scan_meta.py: no `--scope`).
|
||||
// A test changes either through `stamp`.
|
||||
|
||||
import { mkdirSync, writeFileSync } from 'node:fs';
|
||||
import { join } from 'node:path';
|
||||
|
|
@ -42,8 +47,8 @@ export function record(over = {}) {
|
|||
};
|
||||
}
|
||||
|
||||
/** The stamp of a verified changes scan of `base..head`. */
|
||||
export function stampOf({ head, base, dirty = false, records = [] }) {
|
||||
/** The stamp of a verified, unscoped changes scan of `base..head` whose JSONL holds `total` records. */
|
||||
export function stampOf({ head, base, dirty = false, records = [], total = records.length }) {
|
||||
const count = (sev) => records.filter((r) => r.severity === sev).length;
|
||||
return {
|
||||
generated_at: '2026-11-08T12:00:00+00:00',
|
||||
|
|
@ -61,7 +66,7 @@ export function stampOf({ head, base, dirty = false, records = [] }) {
|
|||
diff_files: 2, diff_lines: 30, pre_existing: 0, scope_files: 2,
|
||||
empty_diff: false, empty_scope: false, researchers_dispatched: 3, verification_runs: 1,
|
||||
},
|
||||
findings: { total: records.length, critical: count('CRITICAL'), high: count('HIGH'), medium: count('MEDIUM'), low: count('LOW') },
|
||||
findings: { total, critical: count('CRITICAL'), high: count('HIGH'), medium: count('MEDIUM'), low: count('LOW') },
|
||||
verification: {
|
||||
status: 'verified', candidates: records.length, candidates_deduped: records.length,
|
||||
panel_votes: 3 * records.length, panel_reviewed_findings: records.length,
|
||||
|
|
@ -89,7 +94,8 @@ export function writeReport(dir, { head, base, ts = '20261108-120000', dirty = f
|
|||
writeFileSync(join(folder, 'CLAUDE-SECURITY-RESULTS.jsonl'),
|
||||
jsonl ?? records.map((r) => `${JSON.stringify(r)}\n`).join(''));
|
||||
}
|
||||
const total = typeof jsonl === 'string' ? jsonl.split('\n').filter((l) => l.trim() !== '').length : records.length;
|
||||
const name = stampName ?? `CLAUDE-SECURITY-REVISION-${head.slice(0, 12)}${dirty ? '-dirty' : ''}.json`;
|
||||
writeFileSync(join(folder, name), JSON.stringify(stamp(stampOf({ head, base, dirty, records })), null, 2));
|
||||
writeFileSync(join(folder, name), JSON.stringify(stamp(stampOf({ head, base, dirty, records, total })), null, 2));
|
||||
return folder;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -18,9 +18,9 @@
|
|||
import { after, test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { appendFileSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { appendFileSync, copyFileSync, mkdirSync, mkdtempSync, renameSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { basename, dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
import { record, writeReport } from '../helpers/claude-security-report.mjs';
|
||||
|
|
@ -286,6 +286,141 @@ test('[CNV] not measured: exit 2, the reason on stderr, nothing on stdout', () =
|
|||
}
|
||||
});
|
||||
|
||||
/** convert's CLI on one fresh report folder per case: [name, exit, nothing on stdout, the reason says not measured]. */
|
||||
function convertEach(cases) {
|
||||
return cases.map(([name, opts]) => {
|
||||
const dir = reportDir();
|
||||
writeReport(dir, { head: HEAD, base: BASE, ...opts });
|
||||
const r = run(['convert', '--report-dir', dir, '--base', BASE, '--head', HEAD]);
|
||||
return [name, r.status, r.stdout === '', /not measured/.test(r.stderr)];
|
||||
});
|
||||
}
|
||||
const notMeasuredEach = (cases) => cases.map(([name]) => [name, 2, true, true]);
|
||||
|
||||
test('[CNV] decision 224: a JSONL that does not hold exactly the stamp\'s findings.total records, or a total that is not a non-negative integer, is not measured', () => {
|
||||
const setTotal = (v) => (s) => ({ ...s, findings: { ...s.findings, total: v } });
|
||||
const cases = [
|
||||
['fewer records than the total', { records: [record(), MEDIUM], stamp: setTotal(3) }],
|
||||
['more records than the total', { records: [record(), MEDIUM], stamp: setTotal(1) }],
|
||||
['an empty JSONL, total 2', { records: [record(), MEDIUM], jsonl: '', stamp: setTotal(2) }],
|
||||
['findings.total absent', { stamp: (s) => { const f = { ...s.findings }; delete f.total; return { ...s, findings: f }; } }],
|
||||
['findings absent', { stamp: (s) => { const t = { ...s }; delete t.findings; return t; } }],
|
||||
['findings.total 1.5', { stamp: setTotal(1.5) }],
|
||||
['findings.total "1"', { stamp: setTotal('1') }],
|
||||
['findings.total null', { stamp: setTotal(null) }],
|
||||
['findings.total -1, no records', { records: [], stamp: setTotal(-1) }],
|
||||
];
|
||||
// Control: the records and the total agree.
|
||||
assert.deepEqual(convertEach([['two records, total 2', { records: [record(), MEDIUM] }]]), [['two records, total 2', 0, false, false]]);
|
||||
assert.deepEqual(convertEach(cases), notMeasuredEach(cases));
|
||||
});
|
||||
|
||||
test('[CNV] decision 224: a stamp whose scope is not [] (a scan limited by --scope, or no scope recorded) is not measured', () => {
|
||||
const setScope = (v) => (s) => ({ ...s, scope: v });
|
||||
const cases = [
|
||||
['scope ["bin"]', { stamp: setScope(['bin']) }],
|
||||
['scope absent', { stamp: (s) => { const t = { ...s }; delete t.scope; return t; } }],
|
||||
['scope "bin"', { stamp: setScope('bin') }],
|
||||
['scope {}', { stamp: setScope({}) }],
|
||||
['scope null', { stamp: setScope(null) }],
|
||||
];
|
||||
// Control: an unscoped scan (Claude Security 0.12.0 writes scope: []).
|
||||
assert.deepEqual(convertEach([['scope []', { stamp: setScope([]) }]]), [['scope []', 0, false, false]]);
|
||||
assert.deepEqual(convertEach(cases), notMeasuredEach(cases));
|
||||
});
|
||||
|
||||
test('[CNV] the newest matching folder is judged: an unverified newest scan is not measured, never a fallback to an older verified one', () => {
|
||||
const dir = reportDir();
|
||||
writeReport(dir, { head: HEAD, base: BASE, ts: '20261108-100000', records: [record({ title: 'older verified scan' })] });
|
||||
const alone = convertReport({ reportDir: dir, base: BASE, head: HEAD });
|
||||
assert.equal(alone.exit, 0, `Control: the older scan alone is read: ${alone.reason}`);
|
||||
writeReport(dir, { head: HEAD, base: BASE, ts: '20261108-110000', stamp: (s) => ({ ...s, verification: { ...s.verification, status: 'unverified' } }) });
|
||||
const r = run(['convert', '--report-dir', dir, '--base', BASE, '--head', HEAD]);
|
||||
assert.equal(r.status, 2, `exit 2, got ${r.status}: ${r.stdout}${r.stderr}`);
|
||||
assert.equal(r.stdout, '', 'nothing on stdout');
|
||||
assert.match(r.stderr, /CLAUDE-SECURITY-20261108-110000\/.*not "verified"/, r.stderr);
|
||||
});
|
||||
|
||||
test('[CNV] a folder with two stamps is passed over: an older one-stamp folder is read; alone, it is not measured', () => {
|
||||
const stampName = `CLAUDE-SECURITY-REVISION-${HEAD.slice(0, 12)}.json`;
|
||||
const twoStamps = (dir) => {
|
||||
const folder = writeReport(dir, { head: HEAD, base: BASE, ts: '20261108-110000', records: [record({ title: 'two-stamp scan' })] });
|
||||
copyFileSync(join(folder, stampName), join(folder, `CLAUDE-SECURITY-REVISION-${HEAD.slice(0, 12)}-dirty.json`));
|
||||
};
|
||||
const dir = reportDir();
|
||||
writeReport(dir, { head: HEAD, base: BASE, ts: '20261108-100000', records: [record({ title: 'one-stamp scan' })] });
|
||||
twoStamps(dir);
|
||||
const r = convertReport({ reportDir: dir, base: BASE, head: HEAD });
|
||||
assert.equal(r.exit, 0, r.reason);
|
||||
assert.deepEqual(r.payload.findings.map((f) => f.title), ['one-stamp scan'], 'the two-stamp folder is passed over');
|
||||
const lone = reportDir();
|
||||
twoStamps(lone);
|
||||
const n = run(['convert', '--report-dir', lone, '--base', BASE, '--head', HEAD]);
|
||||
assert.equal(n.status, 2, `exit 2, got ${n.status}: ${n.stdout}${n.stderr}`);
|
||||
assert.equal(n.stdout, '', 'nothing on stdout');
|
||||
assert.match(n.stderr, /not measured/, n.stderr);
|
||||
});
|
||||
|
||||
test('[CNV] a symlinked stamp, JSONL or report folder is refused: not measured', () => {
|
||||
const stampName = `CLAUDE-SECURITY-REVISION-${HEAD.slice(0, 12)}.json`;
|
||||
const kinds = ['stamp', 'JSONL', 'folder'];
|
||||
/** A report dir whose `what` is a symlink to a regular, valid one. */
|
||||
const linked = (what) => {
|
||||
const dir = reportDir();
|
||||
if (what === 'folder') {
|
||||
const elsewhere = reportDir();
|
||||
const real = writeReport(elsewhere, { head: HEAD, base: BASE });
|
||||
const followed = convertReport({ reportDir: elsewhere, base: BASE, head: HEAD });
|
||||
assert.equal(followed.exit, 0, `Control: the folder the link names is a valid report: ${followed.reason}`);
|
||||
symlinkSync(real, join(dir, basename(real)));
|
||||
return dir;
|
||||
}
|
||||
const folder = writeReport(dir, { head: HEAD, base: BASE });
|
||||
const name = what === 'stamp' ? stampName : 'CLAUDE-SECURITY-RESULTS.jsonl';
|
||||
renameSync(join(folder, name), join(folder, `${what}.real`));
|
||||
symlinkSync(`${what}.real`, join(folder, name));
|
||||
return dir;
|
||||
};
|
||||
const got = kinds.map((what) => {
|
||||
const r = run(['convert', '--report-dir', linked(what), '--base', BASE, '--head', HEAD]);
|
||||
return [what, r.status, r.stdout === '', /not measured/.test(r.stderr)];
|
||||
});
|
||||
assert.deepEqual(got, kinds.map((what) => [what, 2, true, true]));
|
||||
});
|
||||
|
||||
test('[CNV] a malformed or absent scan_prefix is not measured', () => {
|
||||
const setPrefix = (v) => (s) => ({ ...s, scan_prefix: v });
|
||||
const cases = [
|
||||
['scan_prefix absent', { stamp: (s) => { const t = { ...s }; delete t.scan_prefix; return t; } }],
|
||||
['scan_prefix null', { stamp: setPrefix(null) }],
|
||||
['scan_prefix 7', { stamp: setPrefix(7) }],
|
||||
['scan_prefix "../"', { stamp: setPrefix('../') }],
|
||||
['scan_prefix "src/../../"', { stamp: setPrefix('src/../../') }],
|
||||
['scan_prefix "/abs/"', { stamp: setPrefix('/abs/') }],
|
||||
['scan_prefix "src" (no trailing slash)', { stamp: setPrefix('src') }],
|
||||
['scan_prefix "./"', { stamp: setPrefix('./') }],
|
||||
['scan_prefix "src//"', { stamp: setPrefix('src//') }],
|
||||
['scan_prefix with a backslash', { stamp: setPrefix('src\\lib/') }],
|
||||
];
|
||||
// Control: an empty prefix and a plain one are read.
|
||||
assert.deepEqual(convertEach([['scan_prefix ""', { stamp: setPrefix('') }], ['scan_prefix "src/"', { stamp: setPrefix('src/') }]]),
|
||||
[['scan_prefix ""', 0, false, false], ['scan_prefix "src/"', 0, false, false]]);
|
||||
assert.deepEqual(convertEach(cases), notMeasuredEach(cases));
|
||||
});
|
||||
|
||||
test('[CNV] a stamp that does not record whether the diff or the scope was empty is not measured', () => {
|
||||
const dropShape = (key) => (s) => { const sh = { ...s.run_shape }; delete sh[key]; return { ...s, run_shape: sh }; };
|
||||
const setShape = (key, value) => (s) => ({ ...s, run_shape: { ...s.run_shape, [key]: value } });
|
||||
const cases = [
|
||||
['run_shape.empty_diff absent', { stamp: dropShape('empty_diff') }],
|
||||
['run_shape.empty_scope absent', { stamp: dropShape('empty_scope') }],
|
||||
['run_shape absent', { stamp: (s) => { const t = { ...s }; delete t.run_shape; return t; } }],
|
||||
['run_shape.empty_diff "false"', { stamp: setShape('empty_diff', 'false') }],
|
||||
['run_shape.empty_scope null', { stamp: setShape('empty_scope', null) }],
|
||||
];
|
||||
assert.deepEqual(convertEach(cases), notMeasuredEach(cases));
|
||||
});
|
||||
|
||||
test('[CNV] an empty scope list is exit 1 (the review scope is unknown), as engine-findings.mjs', () => {
|
||||
const dir = reportDir();
|
||||
writeReport(dir, { head: HEAD, base: BASE });
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue