391 lines
20 KiB
JavaScript
391 lines
20 KiB
JavaScript
// 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}`;
|
||
|
||
// 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-seen one stays.
|
||
// 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();
|
||
const sourceOf = new Map();
|
||
function dedupByTriplet(findings, { perSource = true } = {}) {
|
||
const byTriplet = new Map();
|
||
for (const f of findings) {
|
||
const key = tripletOf(f);
|
||
if (!byTriplet.has(key)) byTriplet.set(key, []);
|
||
const subs = byTriplet.get(key);
|
||
const src = sourceOf.get(f) ?? null;
|
||
const fits = (g) => g.every((m) => (sourceOf.get(m) ?? null) !== src || sameTitle(m, f));
|
||
let sub = !perSource ? subs[0]
|
||
: subs.find((g) => fits(g) && g.some((m) => m.title !== undefined && f.title !== undefined && sameTitle(m, f)))
|
||
?? subs.find(fits);
|
||
if (!sub) {
|
||
sub = [];
|
||
subs.push(sub);
|
||
}
|
||
sub.push(f);
|
||
}
|
||
const groups = [...byTriplet.values()].flat();
|
||
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;
|
||
}
|
||
out.push(severity === survivor.severity ? survivor : { ...survivor, severity });
|
||
}
|
||
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 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.`,
|
||
'',
|
||
'=== 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 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',
|
||
});
|
||
}
|
||
// 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, same file/line/rule_key) for each
|
||
// returned finding's counterpart: (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.
|
||
// - `allFindings` (before dedup, so an engine finding a reviewer finding
|
||
// shadowed in its triplet counts too) for the put-back: (c) every engine
|
||
// finding with no returned finding at its exact triplet is appended at its
|
||
// own severity, one copy per triplet (the higher severity, else the first
|
||
// seen) (operator decision 23, review ab1b5d9f). So the coordinator's drop,
|
||
// move or relabel of an engine finding no longer lowers the verdict.
|
||
// 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 counterparts = new Map(merged
|
||
.filter((m) => m && typeof m === 'object')
|
||
.map((m) => [tripletOf(m), m]));
|
||
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 = counterparts.get(tripletOf(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 returnedTriplets = new Set(returned.filter((f) => f && typeof f === 'object').map(tripletOf));
|
||
// Engine findings only, so dedupByTriplet keeps the higher severity, else the
|
||
// first seen: one copy per triplet (decision 23), so not split per source here.
|
||
const putBack = dedupByTriplet(allFindings.filter((f) =>
|
||
f && typeof f === 'object' && f.engine && !returnedTriplets.has(tripletOf(f))), { 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;
|