voyage/scripts/trekreview-armB.workflow.mjs
Kjell Tore Guttormsen 40f6ae1177
fix(review): close the gate's fail-open paths an independent review found
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>
2026-10-06 20:03:16 +02:00

209 lines
9.3 KiB
JavaScript
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

// scripts/trekreview-armB.workflow.mjs
// NW2 bake-off — Arm B: /trekreview Phase 5–6 reimplemented as a Workflow.
//
// This is the T2 §5 Arm B substrate: where the prose path (Arm A) has main
// spawn two reviewers, hand-parse their trailing JSON, JS-dedup, then spawn the
// coordinator, Arm B expresses the SAME pipeline as one Workflow —
// parallel([conformance, correctness]) with a findings SCHEMA (StructuredOutput
// forces typed findings — no JSON.parse dance, F2 win)
// → plain-JS dedup by (file,line,rule_key) [inlined; scripts have no imports]
// → agent(review-coordinator) with a verdict SCHEMA
// → return { verdict, findings } structured (Phase 7 rendering stays shared/prose).
//
// Phases 1–4 (parse/triage) + 7–8 (render/validate/stats) stay prose in BOTH
// arms; the bake-off pins their output and passes it in via `args`. The reviewer
// agents carry Read, so paths are passed (not inlined content):
// args = { briefPath: <path to brief.md>, diffPath: <path to unified diff>,
// triage: <"path → treatment" lines>, quick?: bool,
// enginePayloads?: [<lib/review/engine-findings.mjs payload>, ...],
// engineNotRun?: [<reviewer name>, ...] }
//
// Ordre 5: the Claude Code engine payloads (Phase 4.7, run in prose before this
// script) are merged with the reviewers' findings; a reviewer or engine that
// returned nothing forbids ALLOW (fail-closed, as on the prose path); and no
// finding is struck for length.
//
// Run via the Workflow tool: Workflow({ scriptPath: this file, args }).
// The structured result is logged as a single JSON line (prefix RESULT_JSON:)
// AND returned, so the bake-off harness can recover {verdict, findings}.
export const meta = {
name: 'trekreview-armB',
description: 'NW2 bake-off Arm B — /trekreview Phase 5–6 as a Workflow (schema-validated parallel reviewers, JS triplet-dedup, coordinator verdict)',
phases: [
{ title: 'Review', detail: 'parallel conformance + correctness reviewers, findings schema-forced' },
{ title: 'Coordinate', detail: 'JS dedup-by-triplet, then review-coordinator verdict' },
],
};
const RULE_KEYS = [
'MISSING_BRIEF_REF', 'UNIMPLEMENTED_CRITERION', 'SCOPE_CREEP_BUILT', 'NON_GOAL_VIOLATED',
'MISSING_TEST', 'SECURITY_INJECTION', 'PLACEHOLDER_IN_CODE', 'MISSING_ERROR_HANDLING',
'UNDECLARED_DEPENDENCY', 'PLAN_EXECUTE_DRIFT', 'BROKEN_SUCCESS_CRITERION', 'COVERAGE_SILENT_SKIP',
'CODE_REVIEW_DEFECT', 'SECURITY_REVIEW_VULNERABILITY',
];
const SEVERITIES = ['BLOCKER', 'MAJOR', 'MINOR', 'SUGGESTION'];
// Findings schema — mirrors lib/review/findings-schema.mjs (NW1). The enum on
// rule_key enforces the catalogue at the StructuredOutput layer (stronger than
// NW1's post-hoc check: the agent cannot emit an out-of-catalogue key at all).
const FINDINGS_SCHEMA = {
type: 'object',
additionalProperties: true,
required: ['reviewer', 'findings'],
properties: {
reviewer: { type: 'string' },
findings: {
type: 'array',
items: {
type: 'object',
additionalProperties: true,
required: ['severity', 'rule_key', 'file', 'line', 'title', 'detail'],
properties: {
severity: { type: 'string', enum: SEVERITIES },
rule_key: { type: 'string', enum: RULE_KEYS },
file: { type: 'string', minLength: 1 },
line: { type: 'integer', minimum: 0 },
brief_ref: { type: 'string' },
title: { type: 'string', minLength: 1 },
detail: { type: 'string' },
recommended_action: { type: 'string' },
},
},
},
},
};
const VERDICT_SCHEMA = {
type: 'object',
additionalProperties: true,
required: ['verdict', 'findings'],
properties: {
verdict: { type: 'string', enum: ['BLOCK', 'WARN', 'ALLOW'] },
rationale: { type: 'string' },
findings: {
type: 'array',
items: {
type: 'object',
additionalProperties: true,
required: ['severity', 'rule_key', 'file', 'line'],
properties: {
severity: { type: 'string', enum: SEVERITIES },
rule_key: { type: 'string', enum: RULE_KEYS },
file: { type: 'string', minLength: 1 },
line: { type: 'integer', minimum: 0 },
title: { type: 'string' },
detail: { type: 'string' },
},
},
},
},
};
// Inlined dedup by (file,line,rule_key) — the JS-side replacement for the
// coordinator's pass-1 triplet dedup (T2 §5 / lib/review/plan-review-dedup.mjs
// pattern; scripts cannot import, so the triplet key is inlined).
function dedupByTriplet(findings) {
const seen = new Map();
for (const f of findings) {
const key = `${f.file}\u0000${f.line}\u0000${f.rule_key}`;
if (!seen.has(key)) seen.set(key, f);
}
return [...seen.values()];
}
function reviewerPrompt(kind, a) {
return [
`You are reviewing delivered code for /trekreview (bake-off Arm B, ${kind}).`,
`Read the brief (the contract) at: ${a.briefPath}`,
`Read the unified diff (the delivered change under review) at: ${a.diffPath}`,
'Emit findings via the StructuredOutput tool, conforming to the provided schema.',
'Only emit findings whose rule_key is in the catalogue. Cite file:line from the diff.',
'',
'=== TRIAGE MAP (respect skip decisions) ===',
a.triage || '(no triage map supplied — treat all changed files as summary-only)',
].join('\n');
}
function coordinatorPrompt(merged, a) {
return [
'You are the review-coordinator (Judge Agent) for /trekreview, Arm B.',
'The reviewers have already run and their findings are PRE-DEDUPLICATED by',
'(file,line,rule_key) in JS. Do NOT re-dedup. Apply ONLY bounded operations:',
'HubSpot Judge filters (accuracy / actionability; succinctness NEVER removes a',
'finding — trim only MINOR/SUGGESTION text, keep BLOCKER/MAJOR whole), Cloudflare',
'reasonableness (drop speculative or catalogue-violating findings), then compute',
'the verdict (BLOCK if any BLOCKER; WARN if any MAJOR; else ALLOW).',
'Return {verdict, findings} via StructuredOutput. Synthesis across files is forbidden.',
`The brief (the contract) is at: ${a.briefPath} — Read it if needed for context.`,
'',
'=== PRE-DEDUPED FINDINGS (JSON) ===',
JSON.stringify(merged, null, 2),
].join('\n');
}
let a = args || {};
if (typeof a === 'string') {
try { a = JSON.parse(a); } catch { a = {}; }
}
log(`Arm B args: type=${typeof args}; keys=${Object.keys(a || {}).join(',') || '(none)'}`);
if (!a || !a.briefPath || !a.diffPath) {
throw new Error('trekreview-armB: args must include { briefPath, diffPath } (pinned Phase 1-4 output)');
}
phase('Review');
const reviewerThunks = [
() => agent(reviewerPrompt('brief-conformance', a), {
agentType: 'voyage:brief-conformance-reviewer', schema: FINDINGS_SCHEMA, label: 'conformance', phase: 'Review',
}),
() => agent(reviewerPrompt('code-correctness', a), {
agentType: 'voyage:code-correctness-reviewer', schema: FINDINGS_SCHEMA, label: 'correctness', phase: 'Review',
}),
];
const toRun = a.quick ? [reviewerThunks[1]] : reviewerThunks;
const rawResults = await parallel(toRun);
const reviewerResults = rawResults.filter(Boolean);
// A reviewer that returned nothing is an absent one, not a clean one. The
// engines that were due are counted from what actually arrived, never from
// what the caller remembered to list as not run.
const enginePayloads = Array.isArray(a.enginePayloads) ? a.enginePayloads.filter(Boolean) : [];
const expectedEngines = a.quick ? ['claude-code-review'] : ['claude-code-review', 'claude-security-review'];
const arrivedEngines = new Set(enginePayloads.map((p) => p.reviewer));
const missingEngines = expectedEngines.filter((name) => !arrivedEngines.has(name));
const missingReviewers = (rawResults.length - reviewerResults.length) + missingEngines.length;
const allFindings = [...reviewerResults, ...enginePayloads]
.flatMap((r) => (Array.isArray(r?.findings) ? r.findings : []));
const merged = dedupByTriplet(allFindings);
log(`Arm B: ${reviewerResults.length} reviewer(s); ${allFindings.length} raw → ${merged.length} after triplet-dedup`);
phase('Coordinate');
let coord = null;
if (merged.length === 0) {
coord = { verdict: missingReviewers > 0 ? 'WARN' : 'ALLOW', findings: [] };
log(`Arm B: zero findings → verdict ${coord.verdict} (coordinator skipped; ${missingReviewers} reviewer(s) missing)`);
} else {
coord = await agent(coordinatorPrompt(merged, a), {
agentType: 'voyage:review-coordinator', schema: VERDICT_SCHEMA, label: 'coordinator', phase: 'Coordinate',
});
}
// The verdict is mechanical: recompute its floor from the findings the
// coordinator returned, so an LLM-written ALLOW over a BLOCKER cannot pass.
const returned = Array.isArray(coord?.findings) ? coord.findings : merged;
const has = (sev) => returned.some((f) => f && f.severity === sev);
let verdict = coord?.verdict ?? null;
if (has('BLOCKER')) verdict = 'BLOCK';
else if (has('MAJOR') && verdict !== 'BLOCK') verdict = 'WARN';
if (verdict === 'ALLOW' && missingReviewers > 0) verdict = 'WARN';
const result = {
verdict,
missing_reviewers: missingReviewers,
missing_engines: missingEngines,
findings: returned,
raw_reviewer_count: reviewerResults.length,
raw_finding_count: allFindings.length,
deduped_count: merged.length,
};
log(`RESULT_JSON:${JSON.stringify(result)}`);
return result;