fix(review): a dropped engine finding keeps the gate not measured, round 1 included
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
a09e6c96a1
commit
3cb3958238
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
6 changed files with 438 additions and 7 deletions
|
|
@ -706,7 +706,9 @@ node ${CLAUDE_PLUGIN_ROOT}/lib/review/review-finalize.mjs --review "{project_dir
|
|||
node ${CLAUDE_PLUGIN_ROOT}/lib/review/review-answers.mjs carry --review "{project_dir}/review.md" --previous "{project_dir}/{previous}" --repo .
|
||||
```
|
||||
|
||||
In round 1, on exit 1 or 2 the file stays as `fill-finding-ids` wrote it; print the reason in one line and continue.
|
||||
In round 1 too, finalize exit 1 or 2 stops the review: print the reason and "not measured", and stop. The file stays
|
||||
as `fill-finding-ids` wrote it, and the verdict gate reads an engine MAJOR or BLOCKER missing from it as exit 2
|
||||
(`review-not-measured: engine finding not in review`).
|
||||
From round 2, finalize exit 1 or 2 stops the review: print the reason and "not measured", and stop. The previous
|
||||
findings reach review.md only through finalize's carry, so the verdict gate reads a round 2+ review without
|
||||
`finalized: true` as exit 2 (`review-not-measured: round 2+ review not finalized`), never green.
|
||||
|
|
|
|||
|
|
@ -432,7 +432,7 @@ missing `exit_code` (C15, `PROGRESS_FAILED_CRITERIA_KEY`).
|
|||
|
||||
**Consumer:** `/trekplan` Phase 1 when `--brief review.md` is supplied and the consumer detects `type: trekreview` in frontmatter. The plan command branches into a remediation-plan path: BLOCKER + MAJOR findings become plan goals, the produced `plan.md` carries a `source_findings: [<id>, ...]` frontmatter list as the audit trail back to the consumed findings. MINOR + SUGGESTION are skipped for plan-input. Since 1.1 every BLOCKER is also written as a numbered `### Step R<n>` under `## Remediation Steps` (Files · Source finding · Test first · Verify · On failure), so a consumer can append it as a plan step without rewording it.
|
||||
|
||||
**Second consumer — the verdict gate (K7, 1.1):** `lib/review/verdict-gate.mjs <review.md>` reads the frontmatter `verdict` and exits 0 = ALLOW/WARN, 1 = BLOCK, 2 = could not decide. WARN opens the merge only when the verdict gate finds every counting MAJOR answered. Exit 2 is never green: a missing file, a missing or unknown verdict, two verdict lines, a trailing JSON block that is not the last fence or lacks `verdict`/`counts`/`findings`, and a file whose JSON block, `## Findings (BLOCKER)` section or finding-id list contradicts a greener verdict all land there — and so does a WARN that exists because a reviewer or engine did not run (`missing-reviewer:` / `unattributable-payload` in the JSON block's `allow_blocked_by`, which a 1.1 review must carry): a review not performed in full stops a merge (operator decision 2026-10-06). Exactly one file per call. With `--repo <dir>` (`verdict-gate.mjs --json <review.md> --repo <dir>`, R6) an ALLOW/WARN review must also be of the commit being merged in `<dir>`: its `scope_sha_end`, read only as a full 40- or 64-hex commit id (a short id or a ref name is stale), is HEAD, or an ancestor of HEAD whose later commits change only the review lessons (`.claude/rules/voyage-review-lessons.md`, inside its managed block), the `CLAUDE.md` pointer and the approval log at the git top level (`.claude/voyage/approvals.jsonl`), there only by append-only answer records: K2 `review` records added after the log's existing bytes, each with exactly the record fields, its own hash, the next `seq` and a `prev` that links (`lib/review/reviewed-commit.mjs`); otherwise exit 2 with a reason that starts `review-stale:`, which `/trekexecute`'s merge lock (Phase 7.2 step 4 and Phase 8, both with `--repo .`) maps to the decline reason `review-stale`. When the gate cannot look (the `--repo` path is missing, it is no git repository, or a git call failed), the exit is 2 with a reason that starts `review-not-measured:`, never `review-stale:`: the merge lock reports it as not measured, never green. A `--repo` with no value is exit 2 as well; BLOCK stays exit 1. Whatever stops a merge or a release on a review reads this exit code, never a summary. Borrowed: the verdict goes to a file and a command gates on it (Kiro `feature-pipeline`); the executor reads the verdict but never writes it (Spec Kit).
|
||||
**Second consumer — the verdict gate (K7, 1.1):** `lib/review/verdict-gate.mjs <review.md>` reads the frontmatter `verdict` and exits 0 = ALLOW/WARN, 1 = BLOCK, 2 = could not decide. WARN opens the merge only when the verdict gate finds every counting MAJOR answered. Exit 2 is never green: a missing file, a missing or unknown verdict, two verdict lines, a trailing JSON block that is not the last fence or lacks `verdict`/`counts`/`findings`, and a file whose JSON block, `## Findings (BLOCKER)` section or finding-id list contradicts a greener verdict all land there — and so does a WARN that exists because a reviewer or engine did not run (`missing-reviewer:` / `unattributable-payload` in the JSON block's `allow_blocked_by`, which a 1.1 review must carry): a review not performed in full stops a merge (operator decision 2026-10-06). Exactly one file per call. With `--repo <dir>` (`verdict-gate.mjs --json <review.md> --repo <dir>`, R6) an ALLOW/WARN review must also be of the commit being merged in `<dir>`: its `scope_sha_end`, read only as a full 40- or 64-hex commit id (a short id or a ref name is stale), is HEAD, or an ancestor of HEAD whose later commits change only the review lessons (`.claude/rules/voyage-review-lessons.md`, inside its managed block), the `CLAUDE.md` pointer and the approval log at the git top level (`.claude/voyage/approvals.jsonl`), there only by append-only answer records: K2 `review` records added after the log's existing bytes, each with exactly the record fields, its own hash, the next `seq` and a `prev` that links (`lib/review/reviewed-commit.mjs`); otherwise exit 2 with a reason that starts `review-stale:`, which `/trekexecute`'s merge lock (Phase 7.2 step 4 and Phase 8, both with `--repo .`) maps to the decline reason `review-stale`. When the gate cannot look (the `--repo` path is missing, it is no git repository, or a git call failed), the exit is 2 with a reason that starts `review-not-measured:`, never `review-stale:`: the merge lock reports it as not measured, never green. An ALLOW/WARN review whose project folder holds a Phase 4.7 engine payload (`.review-engine-payload-*.json`) with a finding at MAJOR or BLOCKER missing from `review.md` is exit 2 as well (an engine finding returned on its own row at a lower severity is not caught), with the reason `review-not-measured: engine finding not in review` naming each missing one: finalize's put-back did not run, or `finalized: true` was written by hand. A `--repo` with no value is exit 2 as well; BLOCK stays exit 1. Whatever stops a merge or a release on a review reads this exit code, never a summary. Borrowed: the verdict goes to a file and a command gates on it (Kiro `feature-pipeline`); the executor reads the verdict but never writes it (Spec Kit).
|
||||
|
||||
**Path conventions:**
|
||||
- Project-dir mode (recommended): `{project_dir}/review.md` (one per review iteration; subsequent runs overwrite atomically).
|
||||
|
|
@ -474,7 +474,7 @@ Since 1.1 (ordre 5):
|
|||
- `## Remediation Steps` — required at `review_version` ≥ 1.1 when there is a BLOCKER: one `### Step R<n>` per BLOCKER carrying `Source finding: <id>` (strict error `REVIEW_MISSING_REMEDIATION_STEP`, soft warning). Steps are appended and numbered, never reworded across iterations (Spec Kit `/converge`).
|
||||
- `## Declined to Judge` — required at `review_version` ≥ 1.1 (strict error, soft warning; 1.0 reviews exempt): every finding set aside without a ruling, with its reason (including engine findings outside the review scope, `engine:out-of-scope`), plus `Trimmed: N`. **No finding is struck for length**: only MINOR/SUGGESTION text is trimmed, and counted (`trimmed` in the JSON block); a long BLOCKER/MAJOR keeps every character.
|
||||
- Findings from Claude Code's own `/code-review` and `/security-review` enter under the catalogue keys `CODE_REVIEW_DEFECT` (MAJOR), `CODE_REVIEW_IMPROVEMENT` (MINOR: a `/code-review` finding whose category is simplification, efficiency, reuse, style or readability) and `SECURITY_REVIEW_VULNERABILITY` (BLOCKER), or a narrower key, with `reviewer` `claude-code-review` / `claude-security-review`.
|
||||
- Working files of one review run, in the project folder (not part of `review.md`): the engine transcriptions `.review-engine-{code-review|security-review}.json` (`/trekreview` Phase 4.7) and, beside them, the converted payloads `.review-engine-payload-{engine}.json` (the `engine-findings.mjs` output, `{reviewer, findings, out_of_scope}`). `review-finalize.mjs` reads the payloads to record which engines ran and to put back, at its own severity, an engine finding at MAJOR or BLOCKER that no finding of `review.md` carries (the standard-path comparison); `review-rounds.mjs prepare` deletes the payloads at the start of each round.
|
||||
- Working files of one review run, in the project folder (not part of `review.md`): the engine transcriptions `.review-engine-{code-review|security-review}.json` (`/trekreview` Phase 4.7) and, beside them, the converted payloads `.review-engine-payload-{engine}.json` (the `engine-findings.mjs` output, `{reviewer, findings, out_of_scope}`). `review-finalize.mjs` reads the payloads to record which engines ran and to put back, at its own severity, an engine finding at MAJOR or BLOCKER that no finding of `review.md` carries (the standard-path comparison); the verdict gate reads the payloads too, to compare them with `review.md`: an engine finding at MAJOR or BLOCKER missing from it is exit 2 (`review-not-measured: engine finding not in review`). `review-rounds.mjs prepare` deletes the payloads at the start of each round.
|
||||
|
||||
Since 1.2 (every field and section optional; a malformed one present is a strict error, a soft warning):
|
||||
- Trailing JSON block, additive: per finding `pre_existing` (boolean: the range did not add or change the cited line and `scope_sha_start` is bound to the fork point from main (the fork point itself, or a later commit reached through process files only); otherwise it is never true), `carried_from` (the previous round's finding id) and `instruction_ref` (the repo path of the instruction file a CLAUDE.md-key finding cites); top level `pre_existing_counts` (`{SEVERITY: n}`), `resolved` (the previous round's finding ids no longer raised) and `ran` (`{engines: [code-review|security-review], reviewers: [string]}`, written with `finalized: true`). Codes `REVIEW_PRE_EXISTING_MALFORMED`, `REVIEW_ROUND_MALFORMED`, `REVIEW_POLICY_MALFORMED`.
|
||||
|
|
|
|||
|
|
@ -41,7 +41,13 @@
|
|||
// coordinator-contract `NOTED_REASONS`) is exit 2 as well (reason
|
||||
// `a policy or filter moved a BLOCKER, MAJOR or security finding`;
|
||||
// `movedSerious`): a policy or filter only ever moves MINOR and
|
||||
// SUGGESTION findings.
|
||||
// SUGGESTION findings. An ALLOW/WARN review is exit 2 too when an
|
||||
// engine finding at MAJOR or BLOCKER in a Phase 4.7 payload beside it
|
||||
// (`.review-engine-payload-*.json`, read by review-finalize.mjs
|
||||
// `roundEvidence`) is missing from it (reason `review-not-measured:
|
||||
// engine finding not in review: <severity> <rule_key> at
|
||||
// <file>:<line>[, …]`; `droppedEngineFindings`): finalize's put-back
|
||||
// did not run, or `finalized: true` was written by hand.
|
||||
// Exit 2 is NEVER green.
|
||||
//
|
||||
// The pure `decideVerdict` stays text-only: it never reads the answers. The
|
||||
|
|
@ -73,6 +79,9 @@ import { resolveRepo } from '../util/approval-log.mjs';
|
|||
import { answerRecords, archivedReader, decideAnswers, isSecurityFinding, reviewSubject } from './review-answers.mjs';
|
||||
import { preExistingCheck, scopeShas } from './pre-existing.mjs';
|
||||
import { frontmatterField, runId } from './review-rounds.mjs';
|
||||
// No cycle: neither module, nor any module they import, imports this gate.
|
||||
import { roundEvidence } from './review-finalize.mjs';
|
||||
import { putBack, normTitle } from './engine-compare.mjs';
|
||||
|
||||
export const EXIT = Object.freeze({ PASS: 0, BLOCK: 1, UNDECIDED: 2 });
|
||||
|
||||
|
|
@ -200,6 +209,7 @@ function blockerSectionEntries(body) {
|
|||
|
||||
const NOT_FINALIZED = 'review-not-measured: round 2+ review not finalized';
|
||||
const MOVED_SERIOUS = 'a policy or filter moved a BLOCKER, MAJOR or security finding';
|
||||
const DROPPED_ENGINE = 'review-not-measured: engine finding not in review';
|
||||
const NOTED_SEVERITIES = new Set(['MINOR', 'SUGGESTION']);
|
||||
|
||||
/** One field of a declined entry for the reason line: one line, at most 120 chars. */
|
||||
|
|
@ -249,6 +259,44 @@ export function unfinalizedRound(text, reviewPath) {
|
|||
return names.some((n) => archive.test(n)) ? NOT_FINALIZED : null;
|
||||
}
|
||||
|
||||
/** The trailing JSON block as decideVerdict reads it (the last fenced block of the body), or null. */
|
||||
function trailingPayload(text) {
|
||||
const body = validateReviewContent(text, { strict: false }).parsed?.body ?? '';
|
||||
const last = fencedBlocks(body).at(-1);
|
||||
if (!last || !last.closed || !isJsonBlock(last)) return null;
|
||||
try { return JSON.parse(last.body); } catch { return null; }
|
||||
}
|
||||
|
||||
/** Same place and title: the fields a carry fold keeps from the current finding. */
|
||||
const samePlace = (c, e) => typeof c.title === 'string' && typeof e.title === 'string'
|
||||
&& c.file === e.file && String(c.line) === String(e.line) && normTitle(c) === normTitle(e);
|
||||
|
||||
/**
|
||||
* The engine findings at MAJOR or BLOCKER in the Phase 4.7 payloads beside the
|
||||
* review (review-finalize.mjs `roundEvidence`: each regular
|
||||
* `.review-engine-payload-*.json` whose `reviewer` maps back through ENGINES)
|
||||
* that review.md does not hold: engine-compare.mjs `putBack` against the
|
||||
* trailing JSON block (an unreadable block holds nothing, so each such engine
|
||||
* finding is missing: the stricter side), without each one a
|
||||
* finding with `carried_from` matches on file, line and `normTitle` (a carry
|
||||
* fold can swap in a previous security `rule_key`, review-rounds.mjs `fold`,
|
||||
* so the row alone would miss it). A finalized review holds every put-back
|
||||
* on its row, so it gives none. An engine finding returned on its own row at
|
||||
* a lower severity is not caught: only a missing one is.
|
||||
* @param {string} text - review.md
|
||||
* @param {string|undefined} reviewPath
|
||||
* @returns {object[]} the missing engine findings, as putBack copies them
|
||||
*/
|
||||
export function droppedEngineFindings(text, reviewPath) {
|
||||
if (!reviewPath) return [];
|
||||
const { payloads } = roundEvidence(dirname(reviewPath));
|
||||
if (payloads.length === 0) return [];
|
||||
const review = trailingPayload(String(text ?? ''));
|
||||
const carried = (Array.isArray(review?.findings) ? review.findings : [])
|
||||
.filter((f) => f !== null && typeof f === 'object' && typeof f.carried_from === 'string' && f.carried_from !== '');
|
||||
return putBack(review, payloads).added.filter((e) => !carried.some((c) => samePlace(c, e)));
|
||||
}
|
||||
|
||||
/**
|
||||
* The open-findings check: every counting MAJOR of an ALLOW/WARN review must be
|
||||
* answered (lib/review/review-answers.mjs `decideAnswers`). K2 records come
|
||||
|
|
@ -262,8 +310,9 @@ export function unfinalizedRound(text, reviewPath) {
|
|||
* against its archive beside the review file (`archivedReader`); with no
|
||||
* review path no carried row answers. A finding with `carried_from` is never
|
||||
* pre-existing: it keeps its line from the previous round's end. A round 2+
|
||||
* review that was not finalized (`unfinalizedRound`) is exit 2 first, then a
|
||||
* BLOCKER, MAJOR or security finding moved by a policy or filter (`movedSerious`).
|
||||
* review that was not finalized (`unfinalizedRound`) is exit 2 first, then an
|
||||
* engine MAJOR or BLOCKER missing from the review (`droppedEngineFindings`),
|
||||
* then a BLOCKER, MAJOR or security finding moved by a policy or filter (`movedSerious`).
|
||||
* @param {string} text - review.md
|
||||
* @param {{ repo?: string, reviewPath: string }} opts
|
||||
* @returns {{ exit: 0|2, reason: string, unanswered: string[], fixed_pending: string[] }}
|
||||
|
|
@ -271,6 +320,15 @@ export function unfinalizedRound(text, reviewPath) {
|
|||
export function decideOpenFindings(text, { repo, reviewPath } = {}) {
|
||||
const notFinal = unfinalizedRound(text, reviewPath);
|
||||
if (notFinal) return { exit: EXIT.UNDECIDED, reason: notFinal, unanswered: [], fixed_pending: [] };
|
||||
const dropped = droppedEngineFindings(text, reviewPath);
|
||||
if (dropped.length > 0) {
|
||||
return {
|
||||
exit: EXIT.UNDECIDED,
|
||||
reason: `${DROPPED_ENGINE}: ${dropped.map((f) => `${field(f.severity) ?? '(no severity)'} ${field(f.rule_key) ?? '(no rule_key)'} at ${field(f.file) ?? '?'}:${field(f.line) ?? '?'}`).join(', ')}`,
|
||||
unanswered: [],
|
||||
fixed_pending: [],
|
||||
};
|
||||
}
|
||||
const hidden = movedSerious(text);
|
||||
if (hidden.length > 0) {
|
||||
return {
|
||||
|
|
|
|||
|
|
@ -11,6 +11,9 @@
|
|||
# review (review-rounds.mjs prepare, then review-finalize.mjs) keeps
|
||||
# exit 2 while the finding is carried and exits 0 once it is resolved;
|
||||
# 3. a pre-existing BLOCKER (a line the range did not change) exits 1;
|
||||
# 3b. an engine MAJOR in a payload beside a WARN review that does not hold it
|
||||
# keeps exit 2 (engine finding not in review, Step 15) until finalize puts
|
||||
# it back, then it is unanswered;
|
||||
# 4. merge-evidence.mjs write records a fixture run, and a
|
||||
# `git merge --no-ff feat` hook input into hooks/scripts/pre-bash-executor.mjs
|
||||
# answers "ask" with the Tested by and Undone by lines.
|
||||
|
|
@ -297,6 +300,45 @@ case_preexisting() {
|
|||
fi
|
||||
}
|
||||
|
||||
case_dropped_engine() {
|
||||
name="dropped engine finding"
|
||||
R="$TMP/dropped"
|
||||
mkrepo "$R" || { bad "$name: setup"; return; }
|
||||
P="$R/$PROJ_REL"
|
||||
# A WARN review: its one MAJOR is answered accepted, so the gate passes it alone.
|
||||
node "$TMP/mkreview.mjs" "$P/review.md" "$START" "$C1" "$MAJ" && node "$FILL" "$P/review.md" > /dev/null \
|
||||
|| { bad "$name: could not write the review"; return; }
|
||||
id="$(reviewjson "$P/review.md" 'j.findings[0].id')"
|
||||
printf 'the parser keeps the old contract on purpose' | node "$ANSWERS" record --review "$P/review.md" --finding "$id" \
|
||||
--answer accepted --repo "$R" --round 1 --store local --trailer "$TRAILER" > "$TMP/drop-rec.out" 2>&1
|
||||
rc=$?
|
||||
if [ "$rc" -ne 0 ]; then bad "$name: record exited $rc: $(cat "$TMP/drop-rec.out")"; return; fi
|
||||
c="$(gate "$P/review.md" "$R" "$TMP/drop-g0.out")"
|
||||
if [ "$c" -ne 0 ]; then bad "$name: the answered WARN alone exited $c: $(cat "$TMP/drop-g0.out")"; return; fi
|
||||
# Phase 4.7's payload: a /code-review MAJOR on b.mjs line 3 that the review does not hold; finalize not run.
|
||||
printf 'a.mjs\nb.mjs\n' > "$TMP/drop-scope.txt"
|
||||
printf '{"engine": "code-review", "findings": [{"file": "b.mjs", "line": 3, "summary": "The counter overflows past 2^31", "failure_scenario": "a long run wraps the count"}]}\n' \
|
||||
> "$TMP/drop-engine.json"
|
||||
node "$ROOT/lib/review/engine-findings.mjs" --scope "$TMP/drop-scope.txt" "$TMP/drop-engine.json" \
|
||||
> "$P/.review-engine-payload-code-review.json" 2> "$TMP/drop-eng.err" \
|
||||
|| { bad "$name: engine-findings: $(cat "$TMP/drop-eng.err")"; return; }
|
||||
c="$(gate "$P/review.md" "$R" "$TMP/drop-g1.out")"
|
||||
if [ "$c" -eq 2 ] && grep -q 'engine finding not in review' "$TMP/drop-g1.out"; then
|
||||
pass "$name: not finalized, the gate exits 2 (engine finding not in review)"
|
||||
else
|
||||
bad "$name: the dropped engine MAJOR exited $c: $(cat "$TMP/drop-g1.out")"
|
||||
fi
|
||||
reviewer "$P" '[]'
|
||||
node "$FINALIZE" --review "$P/review.md" --repo "$R" --project "$P" --round 1 > "$TMP/drop-fin.out" 2>&1 \
|
||||
|| { bad "$name: finalize: $(cat "$TMP/drop-fin.out")"; return; }
|
||||
c="$(gate "$P/review.md" "$R" "$TMP/drop-g2.out")"
|
||||
if [ "$c" -eq 2 ] && grep -q '"reason":"review-unanswered:' "$TMP/drop-g2.out"; then
|
||||
pass "$name: finalized, the put-back MAJOR is unanswered"
|
||||
else
|
||||
bad "$name: after finalize the gate exited $c: $(cat "$TMP/drop-g2.out")"
|
||||
fi
|
||||
}
|
||||
|
||||
case_guard() {
|
||||
name="merge question"
|
||||
R="$TMP/merge"
|
||||
|
|
@ -359,6 +401,7 @@ case_guard() {
|
|||
case_accepted
|
||||
case_fixed
|
||||
case_preexisting
|
||||
case_dropped_engine
|
||||
case_guard
|
||||
|
||||
if [ "$FAILED" -ne 0 ]; then
|
||||
|
|
|
|||
2
tests/fixtures/jsonl-schemas.md
vendored
2
tests/fixtures/jsonl-schemas.md
vendored
|
|
@ -25,7 +25,7 @@
|
|||
| trekexecute-stats (Phase 9 record) | ts, plan, plan_type, mode, result, steps_total, steps_passed, steps_failed, steps_skipped, failed_at_step | lib/stats/stats-append.mjs (trekexecute.md Phase 9) | trekexecute.md:3016-3038 | profile, phase_models, profile_source | none |
|
||||
| trekexecute-stats (autonomy events) | ts, event, known_event, payload | lib/stats/event-emit.mjs `emit()` | event-emit.mjs:78-99 | payload.profile, payload.phase_models, payload.profile_source | none |
|
||||
| trekexecute-stats (PostToolUse Bash) | ts, session_id, command_excerpt, duration_ms, success | hooks/scripts/post-bash-stats.mjs (Bash PostToolUse) | post-bash-stats.mjs:42-54 | none (hook is plugin-level, not profile-aware) | command_excerpt (CWE-212) |
|
||||
| trekreview-stats | ts, slug, verdict, counts (BLOCKER/MAJOR/MINOR/SUGGESTION), reviewed_files_count, mode, duration_ms | lib/stats/stats-append.mjs (trekreview.md Phase 8) | trekreview.md:758-811 | profile, phase_models, profile_source | none |
|
||||
| trekreview-stats | ts, slug, verdict, counts (BLOCKER/MAJOR/MINOR/SUGGESTION), reviewed_files_count, mode, duration_ms | lib/stats/stats-append.mjs (trekreview.md Phase 8) | trekreview.md:760-813 | profile, phase_models, profile_source | none |
|
||||
| trekcontinue-stats | ts, project, next_session_label, status | lib/stats/stats-append.mjs (trekcontinue.md Phase 5) | trekcontinue.md:304-308 | profile, profile_source | none |
|
||||
| token-usage | ts, session_id, scope, model, tokens_input, tokens_output, tokens_cache_creation, tokens_cache_read, cost_usd, is_estimate, price_table_version | lib/stats/token-usage.mjs `captureTokenUsage` (via hooks/scripts/otel-export.mjs Stop hook, opt-in `VOYAGE_TOKEN_METER`) | token-usage.mjs:buildRecord | n/a (SKAL-2 schema) | session_id (stripped at export) |
|
||||
|
||||
|
|
|
|||
328
tests/lib/review-finalized.test.mjs
Normal file
328
tests/lib/review-finalized.test.mjs
Normal file
|
|
@ -0,0 +1,328 @@
|
|||
// tests/lib/review-finalized.test.mjs
|
||||
// [FN1] fix round 4.5b2 part B, Step 15: an engine finding at MAJOR or BLOCKER
|
||||
// that review.md does not hold keeps the verdict gate at exit 2, not measured.
|
||||
// /trekreview Phase 4.7 saves each converted engine payload beside the review
|
||||
// (`{project_dir}/.review-engine-payload-{engine}.json`);
|
||||
// lib/review/review-finalize.mjs puts a dropped one back. When finalize did
|
||||
// not run (round 1 used to continue on its exit 1 or 2) or `finalized: true`
|
||||
// was written by hand, the gate compares the payloads with the review itself
|
||||
// (lib/review/verdict-gate.mjs `droppedEngineFindings`, run by
|
||||
// `decideOpenFindings`) and reads a missing one as
|
||||
// `review-not-measured: engine finding not in review: <severity> <rule_key> at <file>:<line>`.
|
||||
// Every case runs the real CLIs (engine-findings, finalize, gate) in a
|
||||
// throwaway git repo with no hooks: main holds the start commit, the run sits
|
||||
// on branch `feat`, so `scope_sha_start` is the fork point from main.
|
||||
// Pattern: tests/lib/engine-findings-compare.test.mjs.
|
||||
|
||||
import { after, test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { appendFileSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { computeFindingId, computeFindingIds } from '../../lib/parsers/finding-id.mjs';
|
||||
import { decideOpenFindings } from '../../lib/review/verdict-gate.mjs';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
const ENGINE_CLI = join(ROOT, 'lib', 'review', 'engine-findings.mjs');
|
||||
const FINALIZE = join(ROOT, 'lib', 'review', 'review-finalize.mjs');
|
||||
const GATE = join(ROOT, 'lib', 'review', 'verdict-gate.mjs');
|
||||
const REVIEW_CMD = join(ROOT, 'commands', 'trekreview.md');
|
||||
const CONTRACTS = join(ROOT, 'docs', 'HANDOVER-CONTRACTS.md');
|
||||
const REVIEW_REL = '.claude/projects/2026-11-03-fn1/review.md';
|
||||
const DROPPED = 'review-not-measured: engine finding not in review';
|
||||
|
||||
const made = [];
|
||||
after(() => { for (const b of made) rmSync(b, { recursive: true, force: true }); });
|
||||
|
||||
function g(cwd, ...args) {
|
||||
const r = spawnSync('git', args, { cwd, encoding: 'utf8', env: childEnv() });
|
||||
if (r.status !== 0) throw new Error(`git ${args.join(' ')}: ${r.stderr}`);
|
||||
return r.stdout.trim();
|
||||
}
|
||||
|
||||
function putIn(dir, rel, text) {
|
||||
mkdirSync(dirname(join(dir, rel)), { recursive: true });
|
||||
writeFileSync(join(dir, rel), text);
|
||||
}
|
||||
|
||||
const run = (args) => spawnSync(process.execPath, args, { encoding: 'utf8', env: childEnv() });
|
||||
const parse = (r) => { try { return JSON.parse(r.stdout); } catch { return {}; } };
|
||||
const ten = (tag) => Array.from({ length: 10 }, (_, i) => `export const ${tag}${i + 1} = ${i + 1};`).join('\n') + '\n';
|
||||
|
||||
/**
|
||||
* A throwaway repo: main holds src/a.mjs and src/b.mjs (the start commit);
|
||||
* branch `feat` changes line 1 of src/a.mjs and line 3 of src/b.mjs (the end
|
||||
* commit, HEAD). No REVIEW.md, so no policy.
|
||||
*/
|
||||
function fnRepo() {
|
||||
const base = mkdtempSync(join(tmpdir(), 'review-finalized-fn1-'));
|
||||
made.push(base);
|
||||
const repo = join(base, 'repo');
|
||||
mkdirSync(repo, { recursive: true });
|
||||
mkdirSync(join(base, 'hooks-none'));
|
||||
g(repo, 'init', '-q', '-b', 'main');
|
||||
appendFileSync(join(repo, '.git', 'config'), '[user]\n\tname = Test\n\temail = test@example.invalid\n'
|
||||
+ `[commit]\n\tgpgsign = false\n[core]\n\thooksPath = "${join(base, 'hooks-none')}"\n`);
|
||||
appendFileSync(join(repo, '.git', 'info', 'exclude'), '.claude/projects/\n');
|
||||
putIn(repo, 'src/a.mjs', ten('a'));
|
||||
putIn(repo, 'src/b.mjs', ten('b'));
|
||||
g(repo, 'add', '-A');
|
||||
g(repo, 'commit', '-q', '-m', 'chore: start');
|
||||
const start = g(repo, 'rev-parse', 'HEAD');
|
||||
g(repo, 'checkout', '-q', '-b', 'feat');
|
||||
putIn(repo, 'src/a.mjs', ten('a').replace('export const a1 = 1;', 'export const a1 = 11;'));
|
||||
putIn(repo, 'src/b.mjs', ten('b').replace('export const b3 = 3;', 'export const b3 = 33;'));
|
||||
g(repo, 'add', '-A');
|
||||
g(repo, 'commit', '-q', '-m', 'feat: the work');
|
||||
const end = g(repo, 'rev-parse', 'HEAD');
|
||||
const reviewPath = join(repo, ...REVIEW_REL.split('/'));
|
||||
const project = dirname(reviewPath);
|
||||
mkdirSync(project, { recursive: true });
|
||||
const scope = join(base, 'scope.txt');
|
||||
writeFileSync(scope, 'src/a.mjs\nsrc/b.mjs\n');
|
||||
return {
|
||||
base, repo, start, end, reviewPath, project,
|
||||
write: (text) => writeFileSync(reviewPath, text),
|
||||
read: () => readFileSync(reviewPath, 'utf8'),
|
||||
/** An engine's transcription converted by the real engine-findings.mjs CLI; saved beside the review as Phase 4.7 saves it unless `to` names another path. */
|
||||
engine: (engine, findings, to) => {
|
||||
const input = join(base, `.review-engine-${engine}.json`);
|
||||
writeFileSync(input, JSON.stringify({ engine, findings }));
|
||||
const r = run([ENGINE_CLI, '--scope', scope, input]);
|
||||
assert.equal(r.status, 0, `engine-findings: ${r.stdout}${r.stderr}`);
|
||||
const path = to ?? join(project, `.review-engine-payload-${engine}.json`);
|
||||
writeFileSync(path, r.stdout);
|
||||
return { payload: JSON.parse(r.stdout), path };
|
||||
},
|
||||
finalize: () => run([FINALIZE, '--review', reviewPath, '--repo', repo, '--project', project, '--round', '1']),
|
||||
gate: () => {
|
||||
const r = run([GATE, '--json', reviewPath, '--repo', repo]);
|
||||
return { status: r.status, out: parse(r), raw: `${r.stdout}${r.stderr}` };
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* A review_version 1.2 review of `start..end` as the coordinator wrote it and
|
||||
* fill-finding-ids filled it. `findings` are [{severity, file, line, rule_key,
|
||||
* engine?, title, pre_existing?, carried_from?}]; ids computed over (file,
|
||||
* line, rule_key). With `finalized`, the frontmatter says `finalized: true`.
|
||||
*/
|
||||
function fnReview({ start, end, findings, finalized = false }) {
|
||||
const list = findings.map((f, i) => ({
|
||||
severity: f.severity, rule_key: f.rule_key, ...(f.engine ? { engine: f.engine } : {}),
|
||||
file: f.file, line: f.line, title: f.title ?? `t${i + 1}`, detail: `d${i + 1}`, recommended_action: 'Add a test.',
|
||||
...(f.pre_existing !== undefined ? { pre_existing: f.pre_existing } : {}),
|
||||
...(f.carried_from !== undefined ? { carried_from: f.carried_from } : {}),
|
||||
}));
|
||||
const ids = computeFindingIds(list);
|
||||
const ordered = list.map((f, i) => ({ id: ids[i], ...f }));
|
||||
const counts = { BLOCKER: 0, MAJOR: 0, MINOR: 0, SUGGESTION: 0 };
|
||||
for (const f of ordered) counts[f.severity] += 1;
|
||||
const v = counts.BLOCKER > 0 ? 'BLOCK' : counts.MAJOR > 0 ? 'WARN' : 'ALLOW';
|
||||
const json = JSON.stringify({ verdict: v, counts, allow_blocked_by: [], declined_to_judge: [], findings: ordered }, null, 2);
|
||||
const sections = ['BLOCKER', 'MAJOR', 'MINOR', 'SUGGESTION'].map((sev) => {
|
||||
const of = ordered.filter((f) => f.severity === sev);
|
||||
if (of.length === 0) return '';
|
||||
return `## Findings (${sev})\n\n${of.map((f) => `### ${f.id}\n\n- **Rule:** \`${f.rule_key}\`\n- **Location:** \`${f.file}:${f.line}\`\n- **Title:** ${f.title}\n`).join('\n')}\n`;
|
||||
}).join('');
|
||||
return `---
|
||||
type: trekreview
|
||||
review_version: "1.2"
|
||||
task: "t"
|
||||
slug: fn1
|
||||
project_dir: .claude/projects/2026-11-03-fn1/
|
||||
brief_path: .claude/projects/2026-11-03-fn1/brief.md
|
||||
scope_sha_start: ${start}
|
||||
scope_sha_end: ${end}
|
||||
reviewed_files_count: 2
|
||||
verdict: ${v}
|
||||
${finalized ? 'finalized: true\n' : ''}findings:
|
||||
${ordered.map((f) => ` - ${f.id}`).join('\n')}
|
||||
---
|
||||
|
||||
# Review: t
|
||||
|
||||
## Executive Summary
|
||||
|
||||
x
|
||||
|
||||
## Coverage
|
||||
|
||||
| File | Treatment | Reason |
|
||||
|------|-----------|--------|
|
||||
| src/a.mjs | deep-review | default |
|
||||
| src/b.mjs | deep-review | default |
|
||||
|
||||
${sections}## Declined to Judge
|
||||
|
||||
None.
|
||||
|
||||
Trimmed: 0
|
||||
|
||||
## Remediation Summary
|
||||
|
||||
x
|
||||
|
||||
## Remediation Steps
|
||||
|
||||
None.
|
||||
|
||||
\`\`\`json
|
||||
${json}
|
||||
\`\`\`
|
||||
`;
|
||||
}
|
||||
|
||||
/** The text from `heading` up to the next `## ` heading ('' when absent). */
|
||||
function sectionOf(text, heading) {
|
||||
const start = text.indexOf(`\n${heading}`);
|
||||
if (start === -1) return '';
|
||||
const next = text.indexOf('\n## ', start + heading.length + 1);
|
||||
return text.slice(start + 1, next === -1 ? undefined : next);
|
||||
}
|
||||
|
||||
/** A reviewer MAJOR on a line the range did not change, marked pre-existing: it leaves the answer set, so the review is WARN and the gate passes. */
|
||||
const PRE_MAJOR = { severity: 'MAJOR', file: 'src/a.mjs', line: 5, rule_key: 'MISSING_TEST', title: 'An old helper has no test', pre_existing: true };
|
||||
/** A reviewer MINOR: a review holding only it is ALLOW. */
|
||||
const REVIEWER_MINOR = { severity: 'MINOR', file: 'src/b.mjs', line: 3, rule_key: 'MISSING_ERROR_HANDLING', title: 'unchecked return' };
|
||||
/** The /code-review finding the coordinator dropped: CODE_REVIEW_DEFECT, MAJOR, on a changed line. */
|
||||
const ENGINE_MAJOR = { file: 'src/a.mjs', line: 1, summary: 'The counter overflows past 2^31', failure_scenario: 'a long run wraps the count' };
|
||||
|
||||
// --- a dropped engine finding is not measured -----------------------------------
|
||||
|
||||
test('[FN1] a WARN review with an engine payload beside it whose MAJOR the review does not hold -> exit 2 review-not-measured: engine finding not in review, naming its file and line', () => {
|
||||
const fx = fnRepo();
|
||||
fx.write(fnReview({ start: fx.start, end: fx.end, findings: [PRE_MAJOR] }));
|
||||
// Control: without the payload the WARN passes (its one MAJOR is pre-existing).
|
||||
const before = fx.gate();
|
||||
assert.equal(before.status, 0, `the WARN review alone passes the gate: ${before.raw}`);
|
||||
assert.equal(before.out.verdict, 'WARN');
|
||||
fx.engine('code-review', [ENGINE_MAJOR]);
|
||||
const r = fx.gate();
|
||||
assert.equal(r.status, 2, `a dropped engine MAJOR is not measured: ${r.raw}`);
|
||||
const reason = String(r.out.reason);
|
||||
assert.ok(reason.startsWith(`${DROPPED}: `), `the reason names the dropped engine finding: ${reason}`);
|
||||
assert.ok(reason.includes('MAJOR CODE_REVIEW_DEFECT at src/a.mjs:1'), `the reason names its severity, rule_key, file and line: ${reason}`);
|
||||
// Control: after finalize the MAJOR is put back, so the reason is no longer that one: it is unanswered.
|
||||
const fin = fx.finalize();
|
||||
assert.equal(fin.status, 0, `finalize: ${fin.stdout}${fin.stderr}`);
|
||||
const after = fx.gate();
|
||||
assert.equal(after.status, 2, after.raw);
|
||||
assert.ok(!String(after.out.reason).startsWith(DROPPED), `a finalized review with the put-back is not read as dropped: ${after.out.reason}`);
|
||||
assert.match(String(after.out.reason), /^review-unanswered:/, `the put-back MAJOR counts as unanswered: ${after.raw}`);
|
||||
});
|
||||
|
||||
test('[FN1] the same review with finalized: true written by hand -> the same exit 2', () => {
|
||||
const fx = fnRepo();
|
||||
fx.write(fnReview({ start: fx.start, end: fx.end, findings: [PRE_MAJOR], finalized: true }));
|
||||
const before = fx.gate();
|
||||
assert.equal(before.status, 0, `control: the hand-finalized WARN alone passes the gate: ${before.raw}`);
|
||||
fx.engine('code-review', [ENGINE_MAJOR]);
|
||||
const r = fx.gate();
|
||||
assert.equal(r.status, 2, `finalized: true by hand does not hide a dropped engine MAJOR: ${r.raw}`);
|
||||
const reason = String(r.out.reason);
|
||||
assert.ok(reason.startsWith(`${DROPPED}: `), reason);
|
||||
assert.ok(reason.includes('MAJOR CODE_REVIEW_DEFECT at src/a.mjs:1'), reason);
|
||||
});
|
||||
|
||||
test('[FN1] an ALLOW review with a dropped engine BLOCKER -> exit 2 review-not-measured: engine finding not in review', () => {
|
||||
const fx = fnRepo();
|
||||
fx.write(fnReview({ start: fx.start, end: fx.end, findings: [REVIEWER_MINOR] }));
|
||||
const before = fx.gate();
|
||||
assert.equal(before.status, 0, `control: the ALLOW review alone passes the gate: ${before.raw}`);
|
||||
assert.equal(before.out.verdict, 'ALLOW');
|
||||
fx.engine('security-review', [{ file: 'src/a.mjs', line: 1, summary: 'A shell command is built from request input', failure_scenario: 'a crafted name runs a command' }]);
|
||||
const r = fx.gate();
|
||||
assert.equal(r.status, 2, `a dropped engine BLOCKER is not measured: ${r.raw}`);
|
||||
const reason = String(r.out.reason);
|
||||
assert.ok(reason.startsWith(`${DROPPED}: `), reason);
|
||||
assert.ok(reason.includes('BLOCKER SECURITY_REVIEW_VULNERABILITY at src/a.mjs:1'), reason);
|
||||
});
|
||||
|
||||
// --- the text -----------------------------------------------------------------------
|
||||
|
||||
test('[FN1] /trekreview Phase 7 stops round 1 on a finalize exit 1 or 2 with "not measured", and the continue sentence is gone', () => {
|
||||
const phase7 = sectionOf(readFileSync(REVIEW_CMD, 'utf8'), '## Phase 7 — Write review.md');
|
||||
assert.ok(phase7 !== '', 'commands/trekreview.md has a ## Phase 7 — Write review.md section');
|
||||
const flat = phase7.replace(/\s+/g, ' ');
|
||||
assert.ok(flat.includes('In round 1 too, finalize exit 1 or 2 stops the review: print the reason and "not measured", and stop.'),
|
||||
'round 1 stops on a finalize exit 1 or 2');
|
||||
assert.ok(flat.includes('`review-not-measured: engine finding not in review`'),
|
||||
'the sentence says the verdict gate reads a missing engine MAJOR or BLOCKER as exit 2');
|
||||
assert.ok(!flat.includes('In round 1, on exit 1 or 2 the file stays as `fill-finding-ids` wrote it; print the reason in one line and continue.'),
|
||||
'the round-1 continue sentence is gone');
|
||||
// Control: the round-2 sentence after it stays.
|
||||
assert.ok(flat.includes('From round 2, finalize exit 1 or 2 stops the review: print the reason and "not measured", and stop.'),
|
||||
'the round-2 sentence stays');
|
||||
});
|
||||
|
||||
test('[FN1] docs/HANDOVER-CONTRACTS.md names the exit 2 engine finding not in review and says the verdict gate reads the engine payloads', () => {
|
||||
const h6 = sectionOf(readFileSync(CONTRACTS, 'utf8'), '## Handover 6');
|
||||
assert.ok(h6 !== '', 'docs/HANDOVER-CONTRACTS.md has a ## Handover 6 section');
|
||||
const gatePara = h6.split('\n').find((l) => l.startsWith('**Second consumer — the verdict gate'));
|
||||
assert.ok(gatePara, 'Handover 6 has the verdict gate paragraph');
|
||||
assert.ok(gatePara.includes('`review-not-measured: engine finding not in review`'),
|
||||
'the verdict gate paragraph names the new exit 2');
|
||||
const working = h6.split('\n').find((l) => l.startsWith('- Working files of one review run'));
|
||||
assert.ok(working, 'Handover 6 has the working-files bullet');
|
||||
assert.match(working, /verdict gate[^\n]*\breads the payloads too\b/,
|
||||
'the working-files bullet says the verdict gate reads the engine payloads too');
|
||||
});
|
||||
|
||||
// --- kept behaviour -------------------------------------------------------------------
|
||||
|
||||
test('keep: no payload beside the review, an engine MINOR missing, an engine finding only in out_of_scope, or a symlinked payload -> the gate\'s result is unchanged', () => {
|
||||
const fx = fnRepo();
|
||||
fx.write(fnReview({ start: fx.start, end: fx.end, findings: [PRE_MAJOR] }));
|
||||
const bare = fx.gate();
|
||||
assert.equal(bare.status, 0, `no payload beside the review: the gate passes: ${bare.raw}`);
|
||||
const same = (label) => {
|
||||
const r = fx.gate();
|
||||
assert.deepEqual([r.status, r.out.verdict, r.out.reason], [bare.status, bare.out.verdict, bare.out.reason], `${label}: ${r.raw}`);
|
||||
};
|
||||
// An engine MINOR (a /code-review improvement) the review does not hold.
|
||||
const minor = fx.engine('code-review', [{ ...ENGINE_MAJOR, summary: 'The loop could be a map', category: 'simplification' }]);
|
||||
assert.equal(minor.payload.findings[0].severity, 'MINOR', 'the CLI made it a MINOR');
|
||||
same('an engine MINOR missing');
|
||||
rmSync(minor.path);
|
||||
// An engine MAJOR on a file outside the review scope: only in out_of_scope.
|
||||
const out = fx.engine('code-review', [{ file: 'lib/outside.mjs', line: 4, summary: 'An unchanged helper leaks a handle', failure_scenario: 'many calls exhaust handles' }]);
|
||||
assert.equal(out.payload.findings.length, 0, 'no in-scope finding');
|
||||
assert.equal(out.payload.out_of_scope.length, 1, 'the finding sits in out_of_scope');
|
||||
same('an engine finding only in out_of_scope');
|
||||
rmSync(out.path);
|
||||
// A symlinked payload holding a dropped MAJOR: finalize does not read it, nor does the gate.
|
||||
const real = fx.engine('code-review', [ENGINE_MAJOR], join(fx.base, 'elsewhere.json'));
|
||||
symlinkSync(real.path, join(fx.project, '.review-engine-payload-code-review.json'));
|
||||
same('a symlinked payload');
|
||||
});
|
||||
|
||||
test('keep: a current engine finding folded into a carried security finding is not reported as dropped', () => {
|
||||
const fx = fnRepo();
|
||||
// Round 2, as finalize's carry writes it (lib/review/review-rounds.mjs `fold`): the engine's
|
||||
// CODE_REVIEW_DEFECT folded into the previous round's SECURITY_INJECTION BLOCKER keeps the
|
||||
// engine's place, title and `engine`, but the previous rule_key and the higher severity, so
|
||||
// its (file, line, rule_key) row is not the payload's.
|
||||
const prevId = computeFindingId('src/a.mjs', 1, 'SECURITY_INJECTION');
|
||||
const text = fnReview({
|
||||
start: fx.start, end: fx.end, finalized: true,
|
||||
findings: [{ severity: 'BLOCKER', file: 'src/a.mjs', line: 1, rule_key: 'SECURITY_INJECTION', engine: 'code-review', title: ENGINE_MAJOR.summary, carried_from: prevId }],
|
||||
});
|
||||
fx.write(text);
|
||||
const bare = fx.gate();
|
||||
const bareOpen = decideOpenFindings(text, { repo: fx.repo, reviewPath: fx.reviewPath });
|
||||
assert.equal(bare.status, 1, `the carried security BLOCKER blocks: ${bare.raw}`);
|
||||
fx.engine('code-review', [ENGINE_MAJOR]);
|
||||
const r = fx.gate();
|
||||
assert.deepEqual([r.status, r.out.verdict, r.out.reason], [bare.status, bare.out.verdict, bare.out.reason],
|
||||
`the gate's result is unchanged: ${r.raw}`);
|
||||
// The open-findings check itself (also read by scripts/release-check.mjs and merge-evidence.mjs)
|
||||
// does not report the folded engine finding as dropped.
|
||||
const open = decideOpenFindings(text, { repo: fx.repo, reviewPath: fx.reviewPath });
|
||||
assert.deepEqual([open.exit, open.reason], [bareOpen.exit, bareOpen.reason],
|
||||
`decideOpenFindings is unchanged by the payload: ${JSON.stringify(open)}`);
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue