diff --git a/agents/plan-critic.md b/agents/plan-critic.md index ec02e18..5c08407 100644 --- a/agents/plan-critic.md +++ b/agents/plan-critic.md @@ -273,5 +273,27 @@ quality is not scored and Headless readiness returns to 0.15. - Verdict: [APPROVE | APPROVE_WITH_NOTES | REVISE | REPLAN] ``` +### Machine-readable findings block (REQUIRED) + +After the human-readable markdown above, emit **one** fenced `json` block as the +**last thing in your response**. The /trekplan orchestrator extracts it verbatim +and pipes it (with scope-guardian's) into +`lib/review/plan-review-dedup.mjs --stdin`. You have **no Write tool** — returning +this block inline *is* the hand-off; do not attempt to write a file. + +```json +{ + "agent": "plan-critic", + "findings": [ + { "file": "", "line": 0, "rule_key": "", "severity": "blocker|major|minor", "text": "" } + ] +} +``` + +One object per finding. `file`/`line` point at the plan section (use the plan +path + the step's line) or the source the finding concerns; `rule_key` is a +short, stable id for the finding class (used for exact-match dedup). Emit +`"findings": []` when the plan is clean. + Be specific. Reference exact plan sections, step numbers, and file paths. Never use "generally" or "usually" — cite the specific problem in this specific plan. diff --git a/agents/planning-orchestrator.md b/agents/planning-orchestrator.md index 25965d7..bc2ff90 100644 --- a/agents/planning-orchestrator.md +++ b/agents/planning-orchestrator.md @@ -417,16 +417,21 @@ have zero data dependencies; serializing them wastes 30–60 seconds per run. missing error handling, scope creep, underspecified steps, AND manifest quality (dimension 10: every step has a valid, regex-compilable, path-verified manifest). Missing or invalid manifest = **major** finding. - Write structured JSON to `/tmp/plan-critic-out.json`. + Returns its findings as a trailing machine-readable `json` block (it has no + Write tool — the block is the hand-off). - `scope-guardian` — verify plan matches the brief's requirements, find scope creep (plan does more than the brief specifies) and scope gaps (plan misses brief requirements), validate file/function references. Confirm every Success Criterion in the brief is covered by the plan's Verification section. - Write structured JSON to `/tmp/scope-guardian-out.json`. + Returns its findings as a trailing machine-readable `json` block (no Write tool). -After both complete, run an inline dedup pass via -`node ${CLAUDE_PLUGIN_ROOT}/lib/review/plan-review-dedup.mjs --plan-critic /tmp/plan-critic-out.json --scope-guardian /tmp/scope-guardian-out.json > /tmp/plan-review-merged.json`. -The merged array attributes each finding to `[plan-critic, scope-guardian]` +After both complete, extract each reviewer's trailing `json` findings block and +pipe both into the dedup helper via **stdin** (the reviewers are read-only and +cannot write temp files; the orchestrator persists by piping their inline blocks): +`node ${CLAUDE_PLUGIN_ROOT}/lib/review/plan-review-dedup.mjs --stdin` fed +`{ "plan_critic": , "scope_guardian": }`. The helper exits +non-zero on malformed stdin, so a broken hand-off cannot hide behind an empty +merge. The merged array attributes each finding to `[plan-critic, scope-guardian]` if both reviewers raised it. Revise the plan once for the merged set, not twice for the duplicates. Source: research/05 R1 + R2. diff --git a/agents/scope-guardian.md b/agents/scope-guardian.md index 789c228..bd3f425 100644 --- a/agents/scope-guardian.md +++ b/agents/scope-guardian.md @@ -122,3 +122,25 @@ Evaluate: - Dependency issues: N - Overall: [ALIGNED | CREEP — plan does too much | GAP — plan does too little | MIXED] ``` + +### Machine-readable findings block (REQUIRED) + +After the human-readable markdown above, emit **one** fenced `json` block as the +**last thing in your response**. The /trekplan orchestrator extracts it verbatim +and pipes it (with plan-critic's) into +`lib/review/plan-review-dedup.mjs --stdin`. You have **no Write tool** — returning +this block inline *is* the hand-off; do not attempt to write a file. + +```json +{ + "agent": "scope-guardian", + "findings": [ + { "file": "", "line": 0, "rule_key": "", "severity": "blocker|major|minor", "text": "" } + ] +} +``` + +One object per scope-creep / gap / dependency finding. `file`/`line` point at the +plan step (or the brief requirement) the finding concerns; `rule_key` is a short, +stable id for the finding class (used for exact-match dedup against plan-critic). +Emit `"findings": []` when the plan is fully aligned. diff --git a/commands/trekplan.md b/commands/trekplan.md index c058d80..f6f2b1f 100644 --- a/commands/trekplan.md +++ b/commands/trekplan.md @@ -712,8 +712,9 @@ Prompt: "Review this implementation plan for the task: {task}. Plan file: {plan path}. Read it and find every problem — missing steps, wrong ordering, fragile assumptions, missing error handling, scope creep, underspecified steps. Rate each finding as blocker, major, or minor. -Write the structured JSON output to `/tmp/plan-critic-out.json` so the -dedup helper can merge with scope-guardian's findings." +End your response with the REQUIRED machine-readable `json` findings block +(schema in `agents/plan-critic.md`) so the orchestrator can pipe it to the +dedup helper. You have no Write tool — return the block inline, do not write a file." **scope-guardian** — scope alignment check. Prompt: "Check this implementation plan against the brief. @@ -721,23 +722,28 @@ Task: {task}. Brief file: {brief_path}. Plan file: {plan path}. Find scope creep (plan does more than the brief requires) and scope gaps (plan misses brief requirements). Check that referenced files and functions exist. Verify that every Success Criterion in the brief is covered by the -plan's Verification section. Write structured JSON output to -`/tmp/scope-guardian-out.json`." +plan's Verification section. End your response with the REQUIRED machine-readable +`json` findings block (schema in `agents/scope-guardian.md`). You have no Write +tool — return the block inline, do not write a file." -After both complete, run an inline dedup pass: +After both complete, **extract each reviewer's trailing `json` findings block** +and pipe both into the dedup helper via **stdin** — the reviewers are read-only +(`Read/Glob/Grep`, no `Write`), so they cannot persist temp files; the +orchestrator does the persistence by piping their inline blocks: ```bash -node ${CLAUDE_PLUGIN_ROOT}/lib/review/plan-review-dedup.mjs \ - --plan-critic /tmp/plan-critic-out.json \ - --scope-guardian /tmp/scope-guardian-out.json \ - > /tmp/plan-review-merged.json +node ${CLAUDE_PLUGIN_ROOT}/lib/review/plan-review-dedup.mjs --stdin <<'JSON' +{ "plan_critic": , "scope_guardian": } +JSON ``` -The merged array attributes each finding to `[plan-critic, scope-guardian]` -when both reviewers raised the same issue (exact match on -`file:line:rule_key`, or Jaccard ≥ 0.7 on text tokens). Revise the plan -once for the merged set, not twice for the duplicates. Source: research/05 -R1 + R2. +The helper **exits non-zero on malformed stdin** — a broken hand-off surfaces +loudly instead of collapsing into a silently-empty merge (the historical Phase-9 +defect, where the read-only reviewers never wrote the files and the dedup ran on +nothing). The merged array attributes each finding to `[plan-critic, scope-guardian]` +when both reviewers raised the same issue (exact match on `file:line:rule_key`, +or Jaccard ≥ 0.7 on text tokens). Revise the plan once for the merged set, not +twice for the duplicates. Source: research/05 R1 + R2. After both complete: - If **blockers** are found: revise the plan to address them. Add a "Revisions" diff --git a/lib/review/plan-review-dedup.mjs b/lib/review/plan-review-dedup.mjs index cdfe7b2..18ac867 100644 --- a/lib/review/plan-review-dedup.mjs +++ b/lib/review/plan-review-dedup.mjs @@ -9,12 +9,18 @@ // // Provenance is preserved on the surviving finding's `raised_by` array. // -// CLI shim: -// node lib/review/plan-review-dedup.mjs \ -// --plan-critic /tmp/x.json --scope-guardian /tmp/y.json +// CLI shim — two input modes: +// file mode: node lib/review/plan-review-dedup.mjs \ +// --plan-critic /tmp/x.json --scope-guardian /tmp/y.json +// stdin mode: node lib/review/plan-review-dedup.mjs --stdin (reads fd 0: +// one object {plan_critic, scope_guardian} of agent payloads) // → stdout: deduped JSON, exit 0 on success. // -// Empty / missing inputs are tolerated (single-agent review still works). +// File mode tolerates empty / missing inputs (single-agent review still works). +// stdin mode is the Phase-9 path: the read-only reviewers (plan-critic, +// scope-guardian) cannot write temp files, so /trekplan pipes their inline JSON +// blocks here. Malformed stdin exits NON-ZERO — a broken hand-off must surface +// loudly, never collapse into a silent empty merge. import { readFileSync } from 'node:fs'; import { jaccardSimilarity, meetsThreshold } from '../parsers/jaccard.mjs'; @@ -138,6 +144,7 @@ function parseArgs(argv) { if (a === '--plan-critic') out.planCritic = argv[++i]; else if (a === '--scope-guardian') out.scopeGuardian = argv[++i]; else if (a === '--threshold') out.threshold = Number(argv[++i]); + else if (a === '--stdin') out.stdin = true; } return out; } @@ -151,12 +158,46 @@ function readJsonOrNull(path) { } } +// --stdin mode reads ONE object {plan_critic, scope_guardian} from fd 0. Unlike +// the file path mode (which tolerates absent files so single-agent review still +// works), malformed stdin is a hard error: --stdin means the caller intended to +// pipe both inline review blocks, so a parse failure is a broken hand-off that +// must surface loudly — not collapse into a silent empty merge (the original +// Phase-9 defect: read-only reviewers never wrote the temp files, the helper +// swallowed the absence, and the dedup ran on nothing). +function readStdinSourcesOrExit() { + let raw; + try { + raw = readFileSync(0, 'utf-8'); + } catch (err) { + process.stderr.write(`plan-review-dedup --stdin: cannot read stdin: ${err.message}\n`); + process.exit(1); + } + let parsed; + try { + parsed = JSON.parse(raw); + } catch (err) { + process.stderr.write(`plan-review-dedup --stdin: malformed JSON on stdin: ${err.message}\n`); + process.exit(1); + } + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + process.stderr.write('plan-review-dedup --stdin: expected an object {plan_critic, scope_guardian}\n'); + process.exit(1); + } + return [ + { agent: 'plan-critic', payload: parsed.plan_critic ?? null }, + { agent: 'scope-guardian', payload: parsed.scope_guardian ?? null }, + ]; +} + if (import.meta.url === `file://${process.argv[1]}`) { const args = parseArgs(process.argv.slice(2)); - const sources = [ - { agent: 'plan-critic', payload: readJsonOrNull(args.planCritic) }, - { agent: 'scope-guardian', payload: readJsonOrNull(args.scopeGuardian) }, - ]; + const sources = args.stdin + ? readStdinSourcesOrExit() + : [ + { agent: 'plan-critic', payload: readJsonOrNull(args.planCritic) }, + { agent: 'scope-guardian', payload: readJsonOrNull(args.scopeGuardian) }, + ]; const opts = {}; if (Number.isFinite(args.threshold)) opts.threshold = args.threshold; const result = dedupFindings(sources, opts); diff --git a/tests/lib/doc-consistency.test.mjs b/tests/lib/doc-consistency.test.mjs index f5dd9cc..cc84616 100644 --- a/tests/lib/doc-consistency.test.mjs +++ b/tests/lib/doc-consistency.test.mjs @@ -304,6 +304,37 @@ test('Phase 9 prose mandates parallel single-message dispatch + inline dedup', ( ); }); +// --- S23 — Phase-9 dedup hand-off is executable (defect #1 fix) --- +// +// The read-only reviewers (plan-critic, scope-guardian: Read/Glob/Grep, no Write) +// cannot write the temp files the old prose told them to. Pin the A′ fix so the +// broken /tmp-write instruction cannot creep back: +// 1. Phase-9 prose in BOTH docs must use --stdin, not the dead /tmp/*-out.json +// file path the reviewers can't produce. +// 2. BOTH reviewer agents must declare a machine-readable json findings block +// with the keys the dedup helper consumes (agent + findings + rule_key). +test('S23: Phase-9 dedup hand-off uses --stdin and the reviewers emit a json findings block', () => { + const cmd = read('commands/trekplan.md'); + const orch = read('agents/planning-orchestrator.md'); + for (const [name, doc] of [['commands/trekplan.md', cmd], ['agents/planning-orchestrator.md', orch]]) { + assert.ok( + doc.includes('--stdin'), + `${name} Phase 9 must pipe reviewer findings into plan-review-dedup.mjs via --stdin (reviewers are read-only)`, + ); + assert.ok( + !doc.includes('/tmp/plan-critic-out.json') && !doc.includes('/tmp/scope-guardian-out.json'), + `${name} must NOT instruct the read-only reviewers to Write /tmp/*-out.json — they have no Write tool (defect #1)`, + ); + } + for (const agentFile of ['agents/plan-critic.md', 'agents/scope-guardian.md']) { + const a = read(agentFile); + assert.ok( + a.includes('"findings"') && a.includes('"rule_key"') && a.includes('"agent"'), + `${agentFile} must declare a machine-readable json findings block (agent + findings + rule_key) for the Phase-9 dedup helper`, + ); + } +}); + // --- S20 — anti-false-claim: orchestrator docs must not claim ALL sub-agents run on Sonnet --- // // All named swarm agents are pinned `model: opus` (operator decision 40d8742; diff --git a/tests/lib/plan-review-dedup.test.mjs b/tests/lib/plan-review-dedup.test.mjs index 4604eda..89ed27f 100644 --- a/tests/lib/plan-review-dedup.test.mjs +++ b/tests/lib/plan-review-dedup.test.mjs @@ -132,3 +132,42 @@ test('CLI shim tolerates missing input files (returns empty deduped JSON)', () = assert.equal(parsed.findings.length, 0); assert.equal(parsed.dedup_stats.total_in, 0); }); + +// --- S23 — --stdin mode (Phase-9 dedup fix: read-only reviewers can't write +// temp files, so the orchestrator pipes both inline JSON blocks via stdin) --- + +test('CLI --stdin merges {plan_critic, scope_guardian} payloads, provenance preserved', () => { + const stdin = JSON.stringify({ + plan_critic: { + agent: 'plan-critic', + findings: [{ file: 'plan.md', line: 5, rule_key: 'PC1', text: 'duplicate finding shared by both' }], + }, + scope_guardian: { + agent: 'scope-guardian', + findings: [{ file: 'plan.md', line: 5, rule_key: 'PC1', text: 'duplicate finding shared by both' }], + }, + }); + const out = execFileSync(process.execPath, [SHIM, '--stdin'], { input: stdin, encoding: 'utf-8' }); + const parsed = JSON.parse(out); + assert.equal(parsed.findings.length, 1); + assert.deepEqual(parsed.findings[0].raised_by.sort(), ['plan-critic', 'scope-guardian']); + assert.equal(parsed.dedup_stats.total_out, 1); +}); + +test('CLI --stdin tolerates a single present payload (other key absent)', () => { + const stdin = JSON.stringify({ + plan_critic: { agent: 'plan-critic', findings: [{ file: 'a.md', line: 1, rule_key: 'PC1', text: 'lonely finding' }] }, + }); + const out = execFileSync(process.execPath, [SHIM, '--stdin'], { input: stdin, encoding: 'utf-8' }); + const parsed = JSON.parse(out); + assert.equal(parsed.findings.length, 1); + assert.deepEqual(parsed.findings[0].raised_by, ['plan-critic']); +}); + +test('CLI --stdin exits non-zero on malformed input (loud, not a silent empty merge)', () => { + assert.throws( + () => execFileSync(process.execPath, [SHIM, '--stdin'], { input: 'not json{', encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }), + /Command failed|status 1/, + 'malformed --stdin must fail loudly so a broken hand-off cannot hide behind an empty merge', + ); +});