voyage/scripts/trekreview-armB.workflow.mjs
2026-10-24 20:41:53 +02:00

465 lines
23 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. A coordinator that returns no verdict and
// findings counts as a missing reviewer, and every reviewer, engine and
// coordinator that did not deliver is named in `allow_blocked_by` (review edf2ec56).
//
// 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 SEVERITIES = ['BLOCKER', 'MAJOR', 'MINOR', 'SUGGESTION'];
// Every catalogue rule_key -> its tier (lib/review/rule-catalogue.mjs; scripts
// cannot import, so it is inlined and pinned equal by the workflow test).
const RULE_TIERS = {
MISSING_BRIEF_REF: 'MAJOR', UNIMPLEMENTED_CRITERION: 'BLOCKER', SCOPE_CREEP_BUILT: 'MAJOR',
NON_GOAL_VIOLATED: 'BLOCKER', MISSING_TEST: 'MAJOR', SECURITY_INJECTION: 'BLOCKER',
PLACEHOLDER_IN_CODE: 'MAJOR', MISSING_ERROR_HANDLING: 'MINOR', UNDECLARED_DEPENDENCY: 'MAJOR',
PLAN_EXECUTE_DRIFT: 'MAJOR', BROKEN_SUCCESS_CRITERION: 'BLOCKER', COVERAGE_SILENT_SKIP: 'MAJOR',
CODE_REVIEW_DEFECT: 'MAJOR', CODE_REVIEW_IMPROVEMENT: 'MINOR', SECURITY_REVIEW_VULNERABILITY: 'BLOCKER',
};
const RULE_KEYS = Object.keys(RULE_TIERS);
// Lower number = higher severity; an unknown severity ranks last (as
// lib/review/coordinator-contract.mjs severityRank does).
const sevRank = (s) => (SEVERITIES.includes(s) ? SEVERITIES.indexOf(s) : SEVERITIES.length);
// The catalogue tier of a rule_key, or null when the key is missing or unknown.
const tierOf = (key) => (typeof key === 'string' && Object.prototype.hasOwnProperty.call(RULE_TIERS, key) ? RULE_TIERS[key] : null);
// The (file,line,rule_key) triplet a finding is deduped and matched on.
const tripletOf = (f) => `${f.file}\u0000${f.line}\u0000${f.rule_key}`;
// The row a finding is matched to after the coordinator: its triplet plus its
// title, normalized as `sameTitle` below reads it (trimmed, lower case). An
// untitled finding gets a key no titled row has.
const rowOf = (f) => `${tripletOf(f)}\u0000${f.title === undefined ? '\u0001' : String(f.title).trim().toLowerCase()}`;
// 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' },
engine: { type: 'string', enum: ['code-review', 'security-review'] },
contradicts_brief: { type: 'string' },
brief_lowered_from: { type: 'string', enum: ['MAJOR'] },
},
},
},
},
};
// 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). The survivor
// order is coordinator-contract Pass 1's (review bc7f4e23): the catalogue tier
// first (for a catalogue key it always ties inside one triplet, since the
// rule_key is shared), then a reviewer finding (no `engine`) over an engine
// finding (decision 13, operator 12.10), then the higher asserted severity;
// the workflow has no conformance rule, so otherwise the first in the group's
// order stays (the order the two passes below place the members in).
// The survivor carries the highest severity any finding in its group asserted,
// so an engine's mark never lowers what a reviewer agent asserted.
// As in coordinator-contract Pass 1, one source never duplicates itself: its
// second finding on a triplet under another title opens a group of its own
// (voyage-lekerepo trial 5, finding 7). `sourceOf` names the reviewer or engine.
const sameTitle = (a, b) => a.title === undefined || b.title === undefined ||
String(a.title).trim().toLowerCase() === String(b.title).trim().toLowerCase();
// As there too, a triplet's findings are grouped in two passes, so the order
// they arrive in never changes which row a finding joins: (1) every titled
// finding joins the class of its normalized title, over all of the triplet's
// findings; (2) the classes, in alphabetical order of the normalized title,
// each join the first earlier group whose every member fits every finding of
// the class (another source, or the same title), else they form a group of
// their own; untitled findings come last, in canonical order (source, detail,
// the whole finding), each joining the first group it fits. Members keep the
// order the passes place them in (the classes alphabetically, each class's
// findings in canonical order, the untitled findings last), so a full tie on
// the survivor never follows arrival order either; a triplet's groups keep the
// order of their earliest input index. With `perSource: false` everything on a
// triplet is one group, as before. A `rows` Map, when given, is filled with
// each row written -> the findings of its group (the put-back reads it).
const sourceOf = new Map();
const cmpStr = (a, b) => (a < b ? -1 : a > b ? 1 : 0);
function groupTriplet(entries) {
const srcOf = (f) => sourceOf.get(f) ?? null;
const fits = (m, f) => srcOf(m) !== srcOf(f) || sameTitle(m, f);
const canon = (x, y) => cmpStr(String(srcOf(x.f) ?? ''), String(srcOf(y.f) ?? ''))
|| cmpStr(String(x.f.detail ?? ''), String(y.f.detail ?? ''))
|| cmpStr(JSON.stringify(x.f), JSON.stringify(y.f));
const classes = new Map();
const untitled = [];
for (const e of entries) {
if (e.f.title === undefined) {
untitled.push(e);
continue;
}
const t = String(e.f.title).trim().toLowerCase();
if (!classes.has(t)) classes.set(t, []);
classes.get(t).push(e);
}
const groups = [];
const place = (members) => {
let g = groups.find((grp) => grp.every((m) => members.every((e) => fits(m.f, e.f))));
if (!g) {
g = [];
groups.push(g);
}
g.push(...members);
};
for (const t of [...classes.keys()].sort(cmpStr)) place(classes.get(t).sort(canon));
for (const e of untitled.sort(canon)) place([e]);
return groups
.map((g) => ({ first: Math.min(...g.map((e) => e.i)), members: g.map((e) => e.f) }))
.sort((a, b) => a.first - b.first)
.map((g) => g.members);
}
function dedupByTriplet(findings, { perSource = true, rows = null } = {}) {
const byTriplet = new Map();
findings.forEach((f, i) => {
const key = tripletOf(f);
if (!byTriplet.has(key)) byTriplet.set(key, []);
byTriplet.get(key).push({ f, i });
});
const groups = [...byTriplet.values()].flatMap((entries) => (perSource
? groupTriplet(entries)
: [entries.map((e) => e.f)]));
const tier = (f) => sevRank(tierOf(f.rule_key) ?? f.severity);
const out = [];
for (const group of groups) {
let survivor = group[0];
for (const f of group.slice(1)) {
const tf = tier(f);
const ts = tier(survivor);
if (tf !== ts) {
if (tf < ts) survivor = f;
continue;
}
const ef = Boolean(f.engine);
const es = Boolean(survivor.engine);
if (ef !== es) {
if (!ef) survivor = f;
continue;
}
if (sevRank(f.severity) < sevRank(survivor.severity)) survivor = f;
}
let severity = survivor.severity;
for (const f of group) {
if (sevRank(f.severity) < sevRank(severity)) severity = f.severity;
}
const row = severity === survivor.severity ? survivor : { ...survivor, severity };
if (rows) rows.set(row, group);
out.push(row);
}
return out;
}
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 — deep-review every changed file except docs/**, root Markdown other than CLAUDE.md and AGENTS.md, and tests/fixtures/**)',
].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 a finding with no file:line citation or whose file is in neither the tree nor the diff; keep, never drop, a finding whose rule_key is not in the catalogue or whose file cannot be checked), 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.`,
'',
'=== BRIEF CHOICES (the brief-conflict rule) ===',
`Read the brief's choices at ${a.briefPath}: \`## Non-Goals\`, \`## Constraints\`, \`## Preferences\`,`,
'and the operator decisions in `## Open Questions / Assumptions`.',
'Only an engine finding (`engine`: code-review or security-review) whose recommended action',
'contradicts one of those choices is marked, with `contradicts_brief: "<the Non-Goal or choice, verbatim>"`.',
'Only a /code-review finding at the MAJOR tier and at severity MAJOR then drops to MINOR, with',
"`brief_lowered_from: 'MAJOR'`. Every other marked finding keeps its severity. A reviewer-agent finding",
'(no `engine`) and a finding whose rule_key is not in the catalogue are never marked. A brief-conflicting',
'finding is kept and marked, never removed. Each returned finding keeps its `engine` and `rule_key`.',
'',
'=== 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',
}),
];
// The reviewer agent names in run order, so a reviewer that returned nothing is named.
const reviewerNames = a.quick ? ['code-correctness-reviewer'] : ['brief-conformance-reviewer', 'code-correctness-reviewer'];
const toRun = a.quick ? [reviewerThunks[1]] : reviewerThunks;
const rawResults = await parallel(toRun);
const reviewerResults = rawResults.filter(Boolean);
const missingReviewerNames = reviewerNames.filter((_, i) => !rawResults[i]);
// 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));
let missingReviewers = (rawResults.length - reviewerResults.length) + missingEngines.length;
// Provenance, before dedup: "came from an engine" is read from the source,
// never from an `engine` field. A reviewer finding loses any `engine` it
// carries; an engine payload's findings get `engine` from its `reviewer`
// (the names lib/review/engine-findings.mjs writes); any other payload gives
// its findings no `engine`. So an `engine` in `merged` means an engine payload.
// Inlined (scripts cannot import) and pinned equal to ENGINES by the workflow test.
const ENGINE_OF_PAYLOAD = {
'claude-code-review': 'code-review', 'claude-security-review': 'security-review',
};
const engineOf = (reviewer) => (Object.prototype.hasOwnProperty.call(ENGINE_OF_PAYLOAD, reviewer) ? ENGINE_OF_PAYLOAD[reviewer] : null);
const withEngine = (f, eng) => {
if (!f || typeof f !== 'object') return f;
const { engine: _dropped, ...rest } = f;
return eng ? { ...rest, engine: eng } : rest;
};
const tagged = (f, src) => {
if (f && typeof f === 'object') sourceOf.set(f, src);
return f;
};
const allFindings = [
...reviewerResults.flatMap((r, i) => (Array.isArray(r?.findings) ? r.findings : [])
.map((f) => tagged(withEngine(f, null), `reviewer:${r?.reviewer ?? i}`))),
...enginePayloads.flatMap((p) => (Array.isArray(p?.findings) ? p.findings : [])
.map((f) => tagged(withEngine(f, engineOf(p?.reviewer)), `engine:${p?.reviewer}`))),
];
const rowMembers = new Map();
const merged = dedupByTriplet(allFindings, { rows: rowMembers });
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',
});
}
// A coordinator that returns no verdict and findings is a missing reviewer, not
// a clean one (the zero-findings skip above always sets a valid verdict).
const coordinatorMissing = merged.length > 0 && !(coord && ['BLOCK', 'WARN', 'ALLOW'].includes(coord.verdict) && Array.isArray(coord.findings));
if (coordinatorMissing) missingReviewers += 1;
// 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 = coordinatorMissing ? merged : coord.findings;
// The floor reads two inputs, never the labels the coordinator wrote:
// - `merged` (the coordinator's input, one finding per row) for each returned
// finding's counterpart, its own row: the row with its `rowOf` key (triplet
// plus normalized title); with no title match, the only row on its triplet,
// else the strictest row there (highest tier, then highest severity; the
// first in `merged` on a tie), never a lower one. `engine` is taken from
// that row only. (a) an engine counterpart raises it to the
// engine's severity, unless the one legal lowering; (b) a reviewer
// counterpart, labelled or not, raises it to the lower of the counterpart's
// asserted severity and its catalogue tier (the coordinator may reset a
// finding to its tier, never below; for an unknown key the bound is the
// counterpart's severity), and a labelled finding is raised as before.
// - the rows' members (before dedup, so an engine finding a reviewer finding
// shadowed in its row counts too) for the put-back: (c) each engine finding
// in a row whose `rowOf` key no returned finding carries is
// put back at its own severity, one copy per row (the higher severity, else
// the first seen) (operator decision 23, review ab1b5d9f). So the coordinator's drop,
// move, rename or relabel of an engine finding no longer lowers the verdict
// (a renamed row is put back beside the renamed finding: a duplicate, the
// stricter side).
// The raises and put-backs are written into `returned`, the marks kept, so the
// rendered review.md carries them.
// A coordinator that failed (no verdict and findings) is not trusted with a
// partial list: `returned` is then every merged finding, each first raised to
// its catalogue tier when the key is known (never lowered), as the default
// path's Pass 3 would reset it; that raise stands after `raiseTo` is declared.
const rowsOn = (map, key, m) => {
if (!map.has(key)) map.set(key, []);
map.get(key).push(m);
};
const counterparts = new Map();
const rowsOnTriplet = new Map();
for (const m of merged) {
if (!m || typeof m !== 'object') continue;
rowsOn(counterparts, rowOf(m), m);
rowsOn(rowsOnTriplet, tripletOf(m), m);
}
const rowTier = (m) => sevRank(tierOf(m.rule_key) ?? m.severity);
const strictest = (rows) => rows.reduce((best, m) => (rowTier(m) < rowTier(best) ||
(rowTier(m) === rowTier(best) && sevRank(m.severity) < sevRank(best.severity)) ? m : best));
const counterpartOf = (f) => {
const own = counterparts.get(rowOf(f));
if (own) return strictest(own);
const onTriplet = rowsOnTriplet.get(tripletOf(f));
return onTriplet ? strictest(onTriplet) : null;
};
const present = (f, field) => f[field] !== undefined && f[field] !== null;
const raiseTo = (f, sev) => { if (sevRank(sev) < sevRank(f.severity)) f.severity = sev; };
const lowerOf = (s1, s2) => (sevRank(s1) >= sevRank(s2) ? s1 : s2);
if (coordinatorMissing) {
for (const f of returned) {
if (!f || typeof f !== 'object') continue;
const tier = tierOf(f.rule_key);
if (tier !== null) raiseTo(f, tier);
}
}
for (const f of returned) {
if (!f || typeof f !== 'object') continue;
const c = counterpartOf(f);
const labelled = present(f, 'engine') || present(f, 'contradicts_brief') || present(f, 'brief_lowered_from');
if (c && c.engine) {
// (a) an engine counterpart: raised to its severity, unless the one legal lowering.
const legal = c.engine === 'code-review' && c.severity === 'MAJOR' && tierOf(c.rule_key) === 'MAJOR' &&
f.severity === 'MINOR' && typeof f.contradicts_brief === 'string' && f.contradicts_brief.trim().length > 0 &&
f.brief_lowered_from === 'MAJOR';
if (!legal) raiseTo(f, c.severity);
f.engine = c.engine;
continue;
}
if (c) {
// (b) a reviewer counterpart: never below the lower of its asserted severity and the tier.
const tier = tierOf(c.rule_key);
raiseTo(f, tier === null ? c.severity : lowerOf(c.severity, tier));
}
if (labelled) {
// A labelled reviewer finding, or one with no counterpart: an unknown key raises to BLOCKER.
raiseTo(f, tierOf(f.rule_key) ?? 'BLOCKER');
if (c) raiseTo(f, c.severity);
delete f.engine;
}
}
// (c) the put-back (decision 23).
const returnedRows = new Set(returned.filter((f) => f && typeof f === 'object').map(rowOf));
// Each row not returned: its engine findings only, so dedupByTriplet keeps the
// higher severity, else the first seen: one copy per row, so not split per
// source here.
const putBack = merged
.filter((m) => m && typeof m === 'object' && !returnedRows.has(rowOf(m)))
.flatMap((m) => dedupByTriplet((rowMembers.get(m) ?? [m]).filter((f) =>
f && typeof f === 'object' && f.engine), { perSource: false }));
for (const f of putBack) {
const { brief_lowered_from: _lowered, ...copy } = f;
returned.push(copy);
}
const has = (sev) => returned.some((f) => f && f.severity === sev);
let verdict = coordinatorMissing ? (coord?.verdict === 'BLOCK' ? 'BLOCK' : 'WARN') : coord.verdict;
if (has('BLOCKER')) verdict = 'BLOCK';
else if (has('MAJOR') && verdict !== 'BLOCK') verdict = 'WARN';
if (verdict === 'ALLOW' && missingReviewers > 0) verdict = 'WARN';
// Every reviewer, engine and coordinator that did not deliver, in the form the
// default path writes and lib/review/verdict-gate.mjs counts as not run.
const allowBlockedBy = [
...missingReviewerNames.map((name) => `missing-reviewer:${name}`),
...missingEngines.map((name) => `missing-reviewer:${name}`),
...(coordinatorMissing ? ['missing-reviewer:review-coordinator'] : []),
];
const result = {
verdict,
missing_reviewers: missingReviewers,
missing_engines: missingEngines,
allow_blocked_by: allowBlockedBy,
findings: returned,
raw_reviewer_count: reviewerResults.length,
raw_finding_count: allFindings.length,
deduped_count: merged.length,
};
log(`RESULT_JSON:${JSON.stringify(result)}`);
return result;