fix(voyage): S23 — make /trekplan Phase 9 dedup executable (defect #1)
Phase 9's dedup hand-off was broken on two layers, both surfaced by the S22
dogfood: (a) plan-critic + scope-guardian (Read/Glob/Grep, no Write) were told
to write /tmp/*-out.json the dedup helper reads — they cannot; (b) even with
Write, their Output format emits markdown, not the helper's
{agent,findings:[{file,line,rule_key,text}]} schema. readJsonOrNull then
swallowed the absent files -> a silent empty merge that discarded every finding.
Fix (operator-chosen A'): keep the reviewers read-only; make the hand-off run.
- plan-review-dedup.mjs gains a --stdin mode reading {plan_critic,scope_guardian};
malformed stdin exits non-zero so a broken hand-off surfaces loudly instead of
collapsing into a silent empty merge. File mode + its tests are untouched.
- plan-critic.md + scope-guardian.md now emit a trailing machine-readable `json`
findings block (the inline hand-off; no Write tool needed).
- trekplan.md + planning-orchestrator.md Phase 9 rewritten in lockstep: extract
both blocks, pipe via heredoc into --stdin. No temp files, portable, no
pathguard dependency.
TDD: malformed-stdin test failed first (CLI ignored stdin -> exit 0 = the bug),
green after impl. New S23 doc-pin asserts both docs use --stdin (not the dead
/tmp paths) and both agents declare the json block. Suite 724 (722/2/0); live
HEAD baseline was 720, not the stale 705 STATE carried forward.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LqBYc8Ltrk7LipyJmGxXiB
This commit is contained in:
parent
a366e332b7
commit
b4edc12bec
7 changed files with 196 additions and 30 deletions
|
|
@ -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": "<plan path or source file>", "line": 0, "rule_key": "<dimension/short-id>", "severity": "blocker|major|minor", "text": "<one-line finding>" }
|
||||
]
|
||||
}
|
||||
```
|
||||
|
||||
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.
|
||||
|
|
|
|||
|
|
@ -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": <block>, "scope_guardian": <block> }`. 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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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": "<plan path or source file>", "line": 0, "rule_key": "<creep|gap|dependency|...>", "severity": "blocker|major|minor", "text": "<one-line finding>" }
|
||||
]
|
||||
}
|
||||
```
|
||||
|
||||
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.
|
||||
|
|
|
|||
|
|
@ -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": <plan-critic's json block>, "scope_guardian": <scope-guardian's json block> }
|
||||
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"
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
);
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue