From 34916a0ac2c242e0899d643126a22765fde209e6 Mon Sep 17 00:00:00 2001 From: Alex Barclay Date: Tue, 8 Sep 2026 14:54:02 -0600 Subject: [PATCH 1/2] feat(fleetd): transplant reqdrive's prompt discipline into the engine (W2) The engine ran on two-to-three sentence prompts. steps.rs was 116 lines, and the convergence plan called W2 "the single highest-leverage de-risk of the 0%-proven live path" and W2 + W4 together "the whole thesis". W4 landed in July; W2 never did, so the engine advanced on real evidence while being driven by the stubs the plan set out to replace. What ported is the discipline, not the text: oracle must fail against the current tree - a test that already passes defines nothing; assert observable behaviour, not names it is about to invent; ordinary case plus a boundary plus an implied failure; record ambiguity as a comment. It is hashed and frozen the moment it finishes, so a vague oracle is not a weak test but a wrong definition of done that nothing downstream can correct. build one thing, smallest change, address findings first, run the real test command rather than claiming success, never edit a frozen test - and, unlike reqdrive, do NOT commit: this harness owns version control, and an agent that commits corrupts the diff review and merge read. review reqdrive's four named criteria (security, correctness, scope, quality) in place of "review for correctness and quality", plus the standing reminder that a green suite is not evidence, since the tests predate the code. What did NOT port, and why it matters: reqdrive's prompts are welded to its own artifacts - prd.json, progress.txt, .reqdrive/runs/, per-story ids, the iteration-summary block. This harness has none of them and no story concept at all. So W2 as the plan worded it DEPENDS ON W6, which the plan schedules later. That dependency was never stated. The transferable half is here; the story-shaped half waits on a story model. Five prompt-invariant tests. A prompt is the least reviewable thing in the engine - no type, no compiler - so the rules that make it safe are asserted rather than trusted. They pin the discipline, not the wording; reword freely, but drop a rule and they fail. Each was driven red: no-commit rule removed -> builder test FAILED BLOCKERS contract removed -> review test FAILED oracle told tests may pass -> oracle test FAILED Also found and documented, not fixed: parse_blockers treats an ABSENT marker as zero, so a reviewer that crashed or wrote prose scores clean and satisfies the review gate - stronger than allowing the turn, it actively passes it. Under I1 as amended these are two different failures and must not share an outcome. The fix is Option with None routed to NeedsHuman, a state-machine change that does not belong in a prompt commit; it is asserted as a named known gap so it is a decision rather than a surprise. The prompt half of the mitigation is in place. fleetd 87 -> 92 tests. Workspace check clean. Co-Authored-By: Claude Opus 5 (1M context) --- crates/fleetd/src/driver.rs | 22 +++++ crates/fleetd/src/steps.rs | 184 +++++++++++++++++++++++++++++++++--- 2 files changed, 194 insertions(+), 12 deletions(-) diff --git a/crates/fleetd/src/driver.rs b/crates/fleetd/src/driver.rs index cd50363..4d147cd 100644 --- a/crates/fleetd/src/driver.rs +++ b/crates/fleetd/src/driver.rs @@ -1328,6 +1328,28 @@ mod tests { assert_eq!(parse_blockers(&["nothing here".into()]), 0); } + #[test] + fn parse_blockers_treats_an_absent_marker_as_clean() { + // ⚠ KNOWN GAP, asserted so it is a decision rather than a surprise. + // + // An absent marker is indistinguishable from a clean review, so a reviewer + // that crashed, ran out of budget mid-sentence, or simply wrote prose scores + // ZERO blockers and satisfies the review gate. That is stronger than allowing + // the turn: it actively passes it. + // + // Under DOCTRINE I1 as amended 2026-09-03 these are two different failures - + // a check that ran and found nothing versus a check that could not report - + // and they must not share an outcome. Fixing it means returning Option + // and routing None to NeedsHuman, which is a state-machine change and so is + // deliberately NOT bundled with the W2 prompt transplant. + // + // The prompt half of the mitigation is in place: `steps::review` now demands + // the line be emitted even when N is 0, pinned by + // `steps::tests::the_review_prompt_states_the_parse_contract_the_driver_relies_on`. + assert_eq!(parse_blockers(&["the model wrote an essay and stopped".into()]), 0); + assert_eq!(parse_blockers(&[]), 0); + } + #[tokio::test(start_paused = true)] async fn rate_limited_step_retries_then_succeeds() { // Oracle rate-limits once (signal on stderr), then succeeds; floor-1 cycle. diff --git a/crates/fleetd/src/steps.rs b/crates/fleetd/src/steps.rs index 864b374..39584ce 100644 --- a/crates/fleetd/src/steps.rs +++ b/crates/fleetd/src/steps.rs @@ -4,8 +4,26 @@ //! //! NOTE: the in-container `claude` is a stock install without the host's //! `code-review` skill, so the review step approximates it with a review prompt -//! that emits a `BLOCKERS=N` line. Installing the real skill in the image is a -//! later refinement. +//! that emits a `BLOCKERS=N` line. Installing the real skill in the image is +//! **weave item W5**, still unbuilt: writer and grader are the same model in the +//! same container, which is `WORKFLOW` B1 unmet at the engine level. No wording in +//! the review prompt can fix that; only running the grader elsewhere can. +//! +//! ## W2 - what ported, and what could not +//! +//! ADR 0001 retired `reqdrive` as a tool and kept its prompts as the thing worth +//! transplanting. What ports is the *discipline*: implement exactly one thing, never +//! touch the frozen tests, run the real checks, report in a shape a machine can +//! parse, and review against named categories rather than "look for problems". +//! +//! What does not port is everything welded to reqdrive's own artifacts - `prd.json`, +//! `progress.txt`, `.reqdrive/runs/`, per-story ids, and the `iteration-summary` +//! block. This harness has none of them: it owns state in event-sourced SQLite, and +//! it has no story concept at all. +//! +//! **So W2 as the plan worded it depends on W6 (per-story decomposition), which the +//! plan schedules later.** That dependency was never stated. Ported here is the +//! transferable half; the story-shaped half waits on a story model. use crate::runner::UnitSpec; @@ -35,30 +53,121 @@ fn claude_argv(prompt: String, remaining_usd: f64, wall_secs: u64) -> Vec Vec { + // The oracle is written BEFORE any implementation, then content-hashed and + // frozen (W3), so everything downstream is measured against it. A vague oracle + // is not a weak test - it is a wrong definition of done that nothing later can + // correct, because tampering with it halts the unit. let prompt = format!( - "You are the test oracle. Write a minimal but meaningful automated test \ - (a `*.test.js` file runnable by `node --test`) that objectively defines \ - when this task is done. Do NOT implement the solution itself. Task: {}", + "# Test oracle\n\ + \n\ + You define when this task is done. You do NOT implement it.\n\ + \n\ + ## Task\n\ + \n\ + {}\n\ + \n\ + ## What to produce\n\ + \n\ + A single `*.test.js` file runnable by `node --test`, containing a minimal but \ + meaningful set of automated tests.\n\ + \n\ + ## Rules\n\ + \n\ + 1. Write ONLY the test file. Do not implement the solution, and do not stub \ + it so the tests pass vacuously.\n\ + 2. The tests MUST fail against the tree as it stands. A test that already \ + passes defines nothing.\n\ + 3. Assert on observable behaviour - return values, thrown errors, written \ + files. Never on internal names you are also about to invent.\n\ + 4. Cover the ordinary case, at least one boundary, and at least one failure \ + the task implies. Prefer four sharp cases to twenty shallow ones.\n\ + 5. Where the task is ambiguous, choose the reading a careful reviewer would, \ + encode that choice as a test, and say so in a comment at the top.\n\ + \n\ + This file is frozen and hashed when you finish. Later phases may add files \ + but may never edit it - a change is detected and halts the run for a human.", spec.task ); claude_argv(prompt, remaining_usd, spec.wall_clock_secs) } pub fn build(spec: &UnitSpec, findings: &str, remaining_usd: f64) -> Vec { + // reqdrive's implementation discipline, minus its artifacts: one thing at a + // time, real checks, never touch the frozen tests. The harness owns commits and + // state, so - unlike reqdrive - the agent is told NOT to manage either. let prompt = format!( - "Implement the task so the test suite passes. You may ADD files but must \ - NOT modify or delete existing test files. Task: {}. Outstanding review \ - findings to address: {}", + "# Implement\n\ + \n\ + ## Task\n\ + \n\ + {}\n\ + \n\ + ## Outstanding review findings\n\ + \n\ + {}\n\ + \n\ + ## Rules\n\ + \n\ + 1. Make the existing test suite pass. It is the definition of done and it \ + was written before you; do not negotiate with it.\n\ + 2. You may ADD files. You must NOT modify or delete any existing test file. \ + Doing so is detected by content hash and halts the run - it does not \ + merely fail.\n\ + 3. Address every outstanding finding above before adding anything new.\n\ + 4. Smallest change that earns a green suite. No speculative abstraction, no \ + unrelated refactor, no scope the task did not ask for - scope creep is a \ + review finding in its own right.\n\ + 5. Run the project test command yourself before you finish, and fix what it \ + reports. Do not claim success you have not observed.\n\ + 6. Do NOT commit, branch, or tag. The harness owns version control and \ + records state itself; leave your work in the tree.\n\ + \n\ + If the task cannot be done without editing a frozen test, stop and say so \ + plainly rather than working around it.", spec.task, findings ); claude_argv(prompt, remaining_usd, spec.wall_clock_secs) } pub fn review(remaining_usd: f64, wall_secs: u64) -> Vec { - let prompt = "Review the current working-tree diff for correctness and quality. \ - Finish your reply with a single line `BLOCKERS=N` where N is the count of \ - must-fix issues (0 if none)." - .to_string(); + // reqdrive's four review criteria, which are the part worth keeping: it reviewed + // against named categories rather than 'look for problems'. The BLOCKERS=N line + // is this harness's parse contract (driver::parse_blockers) and must survive any + // edit here. + // + // W5 caveat, unfixable from inside this prompt: the reviewer is the same model in + // the same container as the writer. Asking it to be adversarial is not the same as + // it being independent. + let prompt = + "# Review\n\ + \n\ + Review the current working-tree diff against the base branch. You did not \ + write it; do not defend it.\n\ + \n\ + ## Look for, in this order\n\ + \n\ + 1. Security - injection, auth bypass, secrets committed, unsafe operations.\n\ + 2. Correctness - logic errors, unhandled edge cases, off-by-one, null and \ + error paths.\n\ + 3. Scope - anything changed that the task did not ask for.\n\ + 4. Quality - dead code, needless complexity, missing error handling.\n\ + \n\ + A passing test suite is not evidence of correctness: the tests were written \ + before the code and only cover what they cover.\n\ + \n\ + ## Output\n\ + \n\ + For each must-fix issue, one line: `SEVERITY file:line - what is wrong`, \ + where SEVERITY is CRITICAL or WARNING. Only issues you can point at in the \ + diff. Do not pad the list, and do not raise style preferences.\n\ + \n\ + Then finish your reply with exactly one line:\n\ + \n\ + BLOCKERS=N\n\ + \n\ + where N is the count of must-fix issues, 0 if none. This line is parsed by \ + machine. Emit it even when N is 0, and emit it last." + .to_string(); claude_argv(prompt, remaining_usd, wall_secs) } @@ -99,6 +208,57 @@ mod tests { assert_eq!(argv[i + 1], "4.2500"); } + // ── W2 prompt invariants ──────────────────────────────────────────────── + // These pin the DISCIPLINE, not the wording. A prompt is the least reviewable + // thing in the engine - it has no type and no compiler - so the rules that make + // it safe are asserted rather than trusted. Reword freely; drop a rule and this + // fails. + + fn prompt_of(argv: &[String]) -> String { + // The prompt is the argument right after `-p`. + let i = argv.iter().position(|a| a == "-p").expect("a -p flag"); + argv[i + 1].clone() + } + + #[test] + fn the_oracle_is_told_not_to_implement_and_to_fail_first() { + let p = prompt_of(&oracle(&spec(), 1.0)); + assert!(p.contains("do NOT implement") || p.contains("You do NOT implement")); + assert!(p.contains("MUST fail"), "a test that already passes defines nothing"); + assert!(p.contains("add sum(a,b)"), "the task must reach the agent"); + } + + #[test] + fn the_builder_may_not_edit_frozen_tests_or_touch_version_control() { + let p = prompt_of(&build(&spec(), "none", 1.0)); + assert!(p.contains("must NOT modify or delete any existing test file")); + // The harness owns commits. An agent that commits corrupts the diff the + // review and merge steps read. + assert!(p.contains("Do NOT commit")); + assert!(p.contains("add sum(a,b)"), "the task must reach the agent"); + } + + #[test] + fn the_builder_is_given_the_outstanding_findings() { + // Review findings that never reach the next build round make the review loop + // decorative - it would re-raise the same blockers forever. + let p = prompt_of(&build(&spec(), "CRITICAL src/x.js:12 - unchecked null", 1.0)); + assert!(p.contains("CRITICAL src/x.js:12 - unchecked null")); + } + + #[test] + fn the_review_prompt_states_the_parse_contract_the_driver_relies_on() { + // `driver::parse_blockers` scans for a line starting `BLOCKERS=`. If this + // instruction is ever dropped, every review silently reads as 0 blockers + // (see `parse_blockers_treats_an_absent_marker_as_clean`). + let p = prompt_of(&review(1.0, 0)); + assert!(p.contains("BLOCKERS=N")); + assert!(p.contains("Emit it even when N is 0")); + for criterion in ["Security", "Correctness", "Scope", "Quality"] { + assert!(p.contains(criterion), "missing review criterion {criterion}"); + } + } + #[test] fn check_splits_test_command() { assert_eq!(check(&spec()), vec!["npm".to_string(), "test".to_string()]); From 3ad40743313126a11cb7f7b20b276b13a3d24264 Mon Sep 17 00:00:00 2001 From: Alex Barclay Date: Tue, 8 Sep 2026 15:19:26 -0600 Subject: [PATCH 2/2] style: rustfmt after merging main --- crates/fleetd/src/driver.rs | 5 ++++- crates/fleetd/src/steps.rs | 21 +++++++++++++++------ 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/crates/fleetd/src/driver.rs b/crates/fleetd/src/driver.rs index 4d147cd..78f5028 100644 --- a/crates/fleetd/src/driver.rs +++ b/crates/fleetd/src/driver.rs @@ -1346,7 +1346,10 @@ mod tests { // The prompt half of the mitigation is in place: `steps::review` now demands // the line be emitted even when N is 0, pinned by // `steps::tests::the_review_prompt_states_the_parse_contract_the_driver_relies_on`. - assert_eq!(parse_blockers(&["the model wrote an essay and stopped".into()]), 0); + assert_eq!( + parse_blockers(&["the model wrote an essay and stopped".into()]), + 0 + ); assert_eq!(parse_blockers(&[]), 0); } diff --git a/crates/fleetd/src/steps.rs b/crates/fleetd/src/steps.rs index 39584ce..ca6df2c 100644 --- a/crates/fleetd/src/steps.rs +++ b/crates/fleetd/src/steps.rs @@ -138,8 +138,7 @@ pub fn review(remaining_usd: f64, wall_secs: u64) -> Vec { // W5 caveat, unfixable from inside this prompt: the reviewer is the same model in // the same container as the writer. Asking it to be adversarial is not the same as // it being independent. - let prompt = - "# Review\n\ + let prompt = "# Review\n\ \n\ Review the current working-tree diff against the base branch. You did not \ write it; do not defend it.\n\ @@ -167,7 +166,7 @@ pub fn review(remaining_usd: f64, wall_secs: u64) -> Vec { \n\ where N is the count of must-fix issues, 0 if none. This line is parsed by \ machine. Emit it even when N is 0, and emit it last." - .to_string(); + .to_string(); claude_argv(prompt, remaining_usd, wall_secs) } @@ -224,7 +223,10 @@ mod tests { fn the_oracle_is_told_not_to_implement_and_to_fail_first() { let p = prompt_of(&oracle(&spec(), 1.0)); assert!(p.contains("do NOT implement") || p.contains("You do NOT implement")); - assert!(p.contains("MUST fail"), "a test that already passes defines nothing"); + assert!( + p.contains("MUST fail"), + "a test that already passes defines nothing" + ); assert!(p.contains("add sum(a,b)"), "the task must reach the agent"); } @@ -242,7 +244,11 @@ mod tests { fn the_builder_is_given_the_outstanding_findings() { // Review findings that never reach the next build round make the review loop // decorative - it would re-raise the same blockers forever. - let p = prompt_of(&build(&spec(), "CRITICAL src/x.js:12 - unchecked null", 1.0)); + let p = prompt_of(&build( + &spec(), + "CRITICAL src/x.js:12 - unchecked null", + 1.0, + )); assert!(p.contains("CRITICAL src/x.js:12 - unchecked null")); } @@ -255,7 +261,10 @@ mod tests { assert!(p.contains("BLOCKERS=N")); assert!(p.contains("Emit it even when N is 0")); for criterion in ["Security", "Correctness", "Scope", "Quality"] { - assert!(p.contains(criterion), "missing review criterion {criterion}"); + assert!( + p.contains(criterion), + "missing review criterion {criterion}" + ); } }