From 4ebb5050de63ef165032f99f0bc6df310383306b Mon Sep 17 00:00:00 2001 From: Decebal Dobrica <352761+decebal@users.noreply.github.com> Date: Mon, 9 Nov 2026 09:58:18 +0000 Subject: [PATCH] fix(spec): let claude reviews finish and name failure evidence `allstack spec review --host claude` timed out at its 240-second deadline on a 217-line production plan, and the failure message ended with an empty path because a timed-out host has empty stderr. Raise the review deadline to 280 seconds, below the 290-second limit on the enclosing operation, and limit the reviewer to 12 code locations so it reports before the deadline. Record the process evidence directory on `Outcome` and include exit state and that directory in the error. Verified: the same plan now completes on the Claude host in 203s with a receipt. Full test suite and clippy pass. --- docs/reference/cli.md | 2 +- src/application/spec.rs | 2 +- src/infrastructure/process.rs | 3 +++ src/infrastructure/spec.rs | 9 +++++++-- tests/spec.rs | 11 +++++++++++ 5 files changed, 23 insertions(+), 4 deletions(-) diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 9362446..b13ecc9 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -34,7 +34,7 @@ allstack -C /path/to/repository spec approve docs/plans/change-design.md --by "A allstack -C /path/to/repository spec check docs/plans/change-design.md ``` -- `review` starts a read-only host process (`codex exec --sandbox read-only` or `claude --print` with Read, Glob, and Grep only) with a 240-second deadline. Its prompt contains the draft path and repository root. The receipt is written under `.allstack/spec/reviews/-.json`, and the JSON output includes its `receipt` path and `verdict` (`clean`, `findings`, or `unknown`). A failed host exits nonzero and writes no receipt. +- `review` starts a read-only host process (`codex exec --sandbox read-only` or `claude --print` with Read, Glob, and Grep only) with a 280-second deadline. Its prompt contains the draft path and repository root and limits the reviewer to 12 code locations. The receipt is written under `.allstack/spec/reviews/-.json`, and the JSON output includes its `receipt` path and `verdict` (`clean`, `findings`, or `unknown`). A failed host exits nonzero, writes no receipt, and names its exit state and process evidence directory in the error. - `approve` records the file's SHA-256 digest, the approver, and the matching receipt path. It refuses when no receipt exists for the current digest. `--without-review` records an explicit override with no receipt. Approval requires a receipt whose verdict is `clean`; `findings` and `unknown` receipts are refused. `--without-review` is the only override. Approval history per design holds at most 1000 events; a further approval fails with `incomplete or oversized fact history`. - `check` exits 0 only when the latest approval's digest equals the current digest and its receipt, if one was cited, still exists, names that digest, and has verdict `clean`. Otherwise it exits nonzero with the reason: `unapproved`, `changed since approval`, or `unreviewed`. `check` takes the run coordinator lock and fails with `run coordinator busy` during a managed run. When `.allstack` exists it may create store files in it. diff --git a/src/application/spec.rs b/src/application/spec.rs index 3d4265c..df00e74 100644 --- a/src/application/spec.rs +++ b/src/application/spec.rs @@ -3,7 +3,7 @@ /// The reviewer receives the draft path and repository root only, never the author's conversation. pub fn review_prompt(path: &str, root: &str) -> String { format!( - "You are an independent design reviewer with no prior conversation. Review the design document at `{path}` in the repository at `{root}`. Read that document and the repository code it names; do not read anything it does not point to. Do not edit files or run commands that change state. Report each defect as: severity, location, consequence, and evidence. End with exactly one line: `VERDICT: clean` when no defect remains, otherwise `VERDICT: findings`." + "You are an independent design reviewer with no prior conversation. Review the design document at `{path}` in the repository at `{root}`. Read that document and the repository code it names; do not read anything it does not point to. The review has a fixed deadline and output appears only when you finish: check at most 12 code locations, starting with the claims the design depends on most, then write the report and name any claim you left unchecked. Do not edit files or run commands that change state. Report each defect as: severity, location, consequence, and evidence. End with exactly one line: `VERDICT: clean` when no defect remains, otherwise `VERDICT: findings`." ) } diff --git a/src/infrastructure/process.rs b/src/infrastructure/process.rs index f9e284d..4eb2cda 100644 --- a/src/infrastructure/process.rs +++ b/src/infrastructure/process.rs @@ -29,6 +29,8 @@ pub struct Outcome { pub elapsed_ms: u128, pub stdout: String, pub stderr: String, + #[serde(default)] + pub evidence: PathBuf, } impl Outcome { @@ -187,6 +189,7 @@ pub fn supervise() -> Result<()> { elapsed_ms: start.elapsed().as_millis(), stdout: read("stdout")?, stderr: read("stderr")?, + evidence: spec.directory.clone(), }; atomic_write( &spec.directory.join("result.json"), diff --git a/src/infrastructure/spec.rs b/src/infrastructure/spec.rs index 2ed6432..b8c25ba 100644 --- a/src/infrastructure/spec.rs +++ b/src/infrastructure/spec.rs @@ -71,14 +71,19 @@ pub async fn review(repo: &Path, path: &str, host: Host) -> Result { let outcome = process::run(process::Spec { argv: hosts::read_only_argv(host), cwd: repo.into(), - seconds: 240, + // Stays below the 290-second limit on the enclosing operation so a timeout is reported. + seconds: 280, environment: BTreeMap::new(), input: Some(rules::review_prompt(path, &repo.to_string_lossy())), directory: repo.join(".allstack/process"), })?; ensure!( outcome.success(), - "review host failed, timed out, or lost owner; inspect private process evidence: {}", + "review host failed (exit {:?}, timed out {}, owner lost {}); inspect private process evidence at {}: {}", + outcome.code, + outcome.timed_out, + outcome.owner_lost, + outcome.evidence.display(), outcome.stderr ); let findings = hosts::final_text(host, &outcome.stdout)?; diff --git a/tests/spec.rs b/tests/spec.rs index ae5c0ca..43a97eb 100644 --- a/tests/spec.rs +++ b/tests/spec.rs @@ -91,6 +91,7 @@ fn review_runs_fresh_host_and_approval_binds_the_digest() { let prompt = f.captured_prompt(); assert!(prompt.contains(DESIGN), "{prompt}"); assert!(prompt.contains("no prior conversation"), "{prompt}"); + assert!(prompt.contains("at most 12 code locations"), "{prompt}"); assert!(prompt.len() < 2000, "reviewer prompt carries extra context"); let approved = f.json(&["spec", "approve", DESIGN, "--by", "reviewer"]); @@ -137,6 +138,16 @@ fn failed_host_fails_the_review_and_writes_no_receipt() { let f = Fixture::new(FAIL); let (ok, _, err) = f.run(&["spec", "review", DESIGN]); assert!(!ok && err.contains("review host failed"), "{err}"); + assert!(err.contains("exit Some(3)"), "{err}"); + let evidence = err + .split("evidence at ") + .nth(1) + .and_then(|rest| rest.split(':').next()) + .unwrap_or_else(|| panic!("no evidence path: {err}")); + assert!( + PathBuf::from(evidence).join("result.json").is_file(), + "{err}" + ); assert!(!f.repo.join(".allstack/spec/reviews").exists()); }