From a8bbaffffa6945f66421f2aa320f9d4d1d8ee99d Mon Sep 17 00:00:00 2001 From: Bo Wu Date: Wed, 16 Sep 2026 13:55:11 -0700 Subject: [PATCH] Make read-only shortcut review optional and enforce completion gates --- docs/TODO.md | 11 + .../shortcut-safety-assessment.md | 103 +++++++++ docs/architecture/system-architecture.md | 16 +- prompts/playbooks/orchestration-routing.md | 12 +- prompts/roles/read-only-integrity-reviewer.md | 4 +- runtime/src/authority.rs | 8 +- runtime/src/runtime.rs | 17 +- runtime/src/workflow.rs | 203 ++++++++++-------- tests/lifecycle.sh | 108 ++++++++++ 9 files changed, 381 insertions(+), 101 deletions(-) create mode 100644 docs/architecture/shortcut-safety-assessment.md diff --git a/docs/TODO.md b/docs/TODO.md index ebf34c4..abda9bd 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -10,6 +10,17 @@ applicable, and relevant evidence are complete. ## Architecture Target-State Work +- [ ] Validate reviewer-free read-only completion in the deployed Linux runtime: + a real confined reader must finish through `--read-only` and `--auto` without + a reviewer; premature completion and writable launches must be rejected. + Retain evidence for UID/Landlock enforcement and the actual prod-mcp direct + read versus reviewed-ops boundaries. Local fixture tests do not prove deployment. +- [ ] Harden external-only completion accounting: reconcile every pending + reviewed request and live role against terminal receipts before sealing the + session, while retaining the persistent ops identity/resume protocol. Add + regressions for unfinished non-verifier roles and requests lacking receipts; + current external completion scans receipts and verifier/TODO gates only. + - [ ] Add stale runtime cleanup, artifact materialization, and child-process reaping around the file-backed thread manifest and existing S3 trace lifecycle. diff --git a/docs/architecture/shortcut-safety-assessment.md b/docs/architecture/shortcut-safety-assessment.md new file mode 100644 index 0000000..3b6997f --- /dev/null +++ b/docs/architecture/shortcut-safety-assessment.md @@ -0,0 +1,103 @@ +# Shortcut completion safety assessment + +Baseline: `fcf23b6`, synchronized with fetched `origin/main` on September 16, +2026. The assessment started at `3097ce3` (plan-alignment #91, bounded +Executions #86); the subsequent #96 commit only reconciles production TODO +status and is now included without changing the local implementation. The original +`/Volumes/Extreme/projects/multiagent` checkout was inspected read-only; its +uncommitted lifecycle, prompt, and evaluation changes were not imported or +modified. This worktree already includes newer Execution and terminal-outcome +behavior absent from that checkout. The read-only reviewer's self-wait wording +was also already fixed here. + +## Decision + +Make the independent answer reviewer optional for repository read-only +investigations. Keep explicit reviewer requests verified, including their output +seal. Apply the existing repository shortcut checks to successful observe/auto +completion, add the existing blocking-finding/TODO gate, and reject pending +reviewed operation publications. Do not change operation authorization, +pre-execution ops review, source plan alignment, or technical review. + +The supervisor owns these checks because it owns workflow authority and trusted +launch state. No provider, runbook procedure, credential, or new execution +authority is added to the orchestrator. AD-014 records the decision. + +## Actual entry points + +| Entry | Trigger and authority | Transition and review | Mechanical checks | +| --- | --- | --- | --- | +| `complete --direct-response` | Conversation-only result; supervisor-authenticated orchestrator in confined deployment | Pristine pre-implementation to complete/succeeded; no reviewer | Original task digest, no started source lifecycle, no launches, no active workflow TODOs or outstanding blocking findings/subagent TODOs, empty repository diff, no reviewed receipts or published requests, persisted bounded result | +| `--clarification`, `--auto-clarification` | Bounded question; automatic variant is a no-op for a non-question | Same direct route; cannot silently finish an active implementation | Same direct checks plus question validation | +| `complete --read-only` | Completed repository reader; reader/reviewer launches only | Pristine pre-implementation to complete/succeeded; reviewer now optional | Same common shortcut checks; every launch read-only and completed; at least one reader. Explicit reviewer requires matching completed launch, workflow-bound evidence, matching output hash and exact passing diff marker | +| `complete --observe`, read-only `--auto` | Current immutable Execution has no source-write/reviewed-ops effects; user or external observe origin | Pristine pre-implementation to complete/succeeded, no reviewer | Common shortcut checks plus actual read-only Execution and every workflow launch read-only/completed; no mandatory reader. Read-only scout or other confined evidence roles remain allowed | +| `complete --external-only` | Reviewed runbook operation(s), no source lifecycle started | Pre-implementation to complete; succeeded only when all counted terminal receipts succeed, otherwise failed | Original task, source-state fields, workflow TODOs, terminal reviewed receipt/disposition, existing verifier/finding/TODO gate. Independent ops review happens before execution, not at this completion command | +| `complete --request-review` | Read-only Execution proposes exact paths and/or reviewed-ops in a bounded question | Terminal review_requested; immutable proposal for human decision, not successful work | Read-only authority, pre-implementation phase, original task, bounded question/effects; unresolved work may remain because escalation does not claim completion | +| `complete --human-review` | A finalized independent reviewer cannot approve and asks one bounded question | Terminal review_requested, not succeeded; pending work is retained | Workflow-bound sealed reviewer identity/output hash and exact persisted question. No operation permit is authorized by this outcome | +| Plain `complete` / `execute-iteration` | Source implementation | Plan alignment, worker graph completion/finalization, frozen candidate, post-implementation technical review, supervisor completion | Remains the source lifecycle; no shortcut introduced | + +`--auto` in an effect-bearing Execution retains the existing automatic +clarification behavior. It does not turn an implementation result into a +reviewer-free source completion. + +## Boundary and evidence analysis + +- **Source writes:** completed read-only permissions and unchanged diffs are + independent checks. Neither an empty diff nor an LLM verdict proves that no + transient/ignored/submodule change occurred. Linux UID/Landlock confinement, + supervisor-owned grants/launches, and path-bound worker ownership remain the + preventive boundary. The canonical diff is supplementary and requires a Git + repository. Non-isolated local CLI fixtures are not an adversarial authority + boundary or a deployed security test. +- **External effects:** local filesystem read-only access never grants production + read authority. `ops read` separately validates the live prod-mcp capability + (`read`/`materialize`, mutation=false, no approval roles), caller, task, + runbook/schema/target and permit, and persists `direct-request.json` plus + `receipt.redacted.json`. These records are deliberately distinct from reviewed + `receipt.json` and published `operations/requests`. A read operation requiring + an approval role still uses reviewed ops. No execution or signing code changes. +- **Incomplete work:** successful local shortcuts cannot absorb a sealed source + plan, source lifecycle state, pending workflow/subagent TODO, blocking finding, + running reader, writable launch, reviewed receipt or pending publication. + The initial observe path previously lacked these completion checks despite + having pre-execution mutation controls. This change closes that accounting gap. +- **Answer quality:** removing the mandatory reviewer removes an independent + semantic check that the answer covers the request and is supported by evidence. + Mechanical completion does not promise semantic correctness. Reader output is + not an authorization artifact; current supervisor sealing creates independent + evidence copies for reviewers/scouts, not readers. Even the former reviewer + prompt could not make public reader output immutable. Optional quality review + remains available; source and ops reviews remain mandatory where applicable. +- **Evidence seals:** the explicit read-only reviewer path formerly checked + metadata and pass markers without recomputing its recorded output hash. It now + verifies that hash. This is evidence integrity, not an additional permission. +- **External-only limitation:** its terminal receipt scan does not reconcile every + pending request and non-verifier live launch. It also supports non-Git roots, + so source exclusion relies on lifecycle/confinement rather than a repository + snapshot. This existing gap is not used to justify relaxing ops review; fuller + completion accounting is recorded in `docs/TODO.md` as separate work. + +## Validation and limits + +`cargo test -p multiagent`: 94 tests passed. `tests/lifecycle.sh`: passed, +including reviewer-free read-only/observe/auto success and rejection for running +or writable launches, sealed source state, changed original-task binding, +workflow/subagent TODOs, blocking findings, tracked/untracked changes, reviewed +receipts, pending requests, missing required reader, and substituted optional +reviewer evidence. Explicit reviewed compatibility completion still passes. +Direct-read fixtures demonstrate routing separation, not prod-mcp authorization. +`test_migration_contracts.py`: 20 tests passed. The complete `tests/run.sh` +shell contract suite and `mock_orchestration_e2e.sh` passed using the system +`/bin/bash` (with `/usr/bin:/bin` first in PATH for nested scripts). The earlier +mock setup stall was specific to the initially selected Homebrew Bash invocation; +it did not reproduce with system Bash. Formatting and diff whitespace checks +passed. A further regression confirms human escalation preserves an unresolved +TODO while sealing `review_requested`; successful completion still rejects it. +After the final code cleanup, all 94 Rust tests and the lifecycle suite passed +again. The normal reviewer-free route no longer computes the same repository +diff twice. + +Linux UID/Landlock and real provider execution were not verified on this macOS +host. Docker is installed but its daemon is unavailable. No deployment, merge, +production read, or external operation was performed. Deployed reader lifecycle +and operation-boundary acceptance remains in the canonical TODO backlog. diff --git a/docs/architecture/system-architecture.md b/docs/architecture/system-architecture.md index 3db9ea4..998cf84 100644 --- a/docs/architecture/system-architecture.md +++ b/docs/architecture/system-architecture.md @@ -808,9 +808,19 @@ The primary Session and Execution transitions are small and mechanically selecte still requires the runbook, independent reviewer, signed permit, target allowlist, receipt, Logger, and trace gates. -The older direct-response and reviewed read-only completion commands remain -compatibility routes for existing callers, not requirements for read-only -sessions. Route prose never grants authority: UID separation, Landlock, +The older direct-response and read-only completion commands remain +compatibility routes. Repository read-only completion does not require an +independent model reviewer; an explicitly supplied reviewer remains a checked +quality gate. Its semantic answer assessment is not mutation authority. +All successful read-only routes reject started source lifecycles, active TODOs +and blocking findings, changed repository diffs, unfinished or writable launches, +and reviewed operation requests or receipts. The repository investigation route +also requires a completed reader; direct-response forbids launches. Observe +completion additionally requires a read-only Execution. These completion checks +supplement, rather than replace, pre-execution confinement. Direct evidence +reads under AD-020 retain their separate capability checks and receipts; they +are not generic reviewed operations, even when both return read-only data. +Route prose never grants authority: UID separation, Landlock, immutable session grants, assignment ownership, diff binding, and the supervisor completion gate enforce these transitions. diff --git a/prompts/playbooks/orchestration-routing.md b/prompts/playbooks/orchestration-routing.md index c0873a3..e00a6fc 100644 --- a/prompts/playbooks/orchestration-routing.md +++ b/prompts/playbooks/orchestration-routing.md @@ -71,10 +71,14 @@ validation-scheduling.md and hold one validation lease per package. Give technic - A source worker needs an approved implementation context and active implementation permit. - Observe completion is available only while the current Execution is - mechanically read-only. - Source writes and mutating production operations are denied before execution, - so completion does not infer safety from role count, a second model, or a - post-hoc diff check. + mechanically read-only. Wait for every launched role to complete before + requesting completion. The supervisor also rejects changed source, started + implementation, unresolved TODOs/findings, and reviewed operation artifacts. + Pre-execution confinement remains the mutation boundary. +- The repository investigation compatibility command is + `multiagent orchestrator complete --read-only --result-file PATH`. It requires + a completed reader but no independent model reviewer. Use `--reviewer NAME` + only for an explicitly requested additional quality gate. - A repair review request must contain one bounded question and at least one explicit effect: exact repository-relative source paths, `reviewed-ops`, or both. Approval starts a fresh user Session whose first Execution contains diff --git a/prompts/roles/read-only-integrity-reviewer.md b/prompts/roles/read-only-integrity-reviewer.md index a660d44..5419593 100644 --- a/prompts/roles/read-only-integrity-reviewer.md +++ b/prompts/roles/read-only-integrity-reviewer.md @@ -1,6 +1,8 @@ # Read-Only Integrity Reviewer -Independently verify one repository read-only shortcut. You are a read-only +When explicitly requested as an optional quality gate, independently verify +one repository read-only shortcut. Ordinary read-only completion does not +require this role. You are a read-only reviewer and must not modify the repository, workflow state, launch manifests, reader outputs, or traces. diff --git a/runtime/src/authority.rs b/runtime/src/authority.rs index 6828930..a97feba 100644 --- a/runtime/src/authority.rs +++ b/runtime/src/authority.rs @@ -90,6 +90,7 @@ impl AuthorityRequest { && matches!( args[1].as_str(), "--external-only" + | "--read-only" | "--direct-response" | "--auto" | "--clarification" @@ -600,7 +601,12 @@ mod tests { ) .expect("direct completion request"); assert!(direct_completion.authorized_for(config::ORCHESTRATOR_UID)); - for route in ["--clarification", "--auto-clarification", "--auto"] { + for route in [ + "--clarification", + "--auto-clarification", + "--auto", + "--read-only", + ] { let clarification_completion = AuthorityRequest::from_cli( "orchestrator", &strings(&[ diff --git a/runtime/src/runtime.rs b/runtime/src/runtime.rs index b4252e3..663d385 100644 --- a/runtime/src/runtime.rs +++ b/runtime/src/runtime.rs @@ -1209,7 +1209,7 @@ pub fn orchestrator(args: &[String]) -> Result { .iter() .any(|arg| matches!(arg.as_str(), "-h" | "--help")) { - println!("Usage:\n multiagent orchestrator request-mutation [--path REPO_PATH ...] [--reviewed-ops]\n multiagent orchestrator complete\n multiagent orchestrator complete --auto --result-file PATH\n multiagent orchestrator complete --observe --result-file PATH\n multiagent orchestrator complete --request-review --result-file PATH [--path REPO_PATH ...] [--reviewed-ops]\n multiagent orchestrator complete --direct-response --result-file PATH\n multiagent orchestrator complete --clarification --result-file PATH\n multiagent orchestrator complete --auto-clarification --result-file PATH\n multiagent orchestrator complete --read-only --result-file PATH --reviewer NAME\n multiagent orchestrator complete --human-review --result-file PATH --reviewer NAME\n multiagent orchestrator complete --external-only --result-file PATH\n\nEach user session starts with a read-only Execution. The orchestrator may request exact source paths and/or reviewed-ops from the Supervisor without starting another session. External observe sessions must request human review instead."); + println!("Usage:\n multiagent orchestrator request-mutation [--path REPO_PATH ...] [--reviewed-ops]\n multiagent orchestrator complete\n multiagent orchestrator complete --auto --result-file PATH\n multiagent orchestrator complete --observe --result-file PATH\n multiagent orchestrator complete --request-review --result-file PATH [--path REPO_PATH ...] [--reviewed-ops]\n multiagent orchestrator complete --direct-response --result-file PATH\n multiagent orchestrator complete --clarification --result-file PATH\n multiagent orchestrator complete --auto-clarification --result-file PATH\n multiagent orchestrator complete --read-only --result-file PATH [--reviewer NAME]\n multiagent orchestrator complete --human-review --result-file PATH --reviewer NAME\n multiagent orchestrator complete --external-only --result-file PATH\n\nEach user session starts with a read-only Execution. The orchestrator may request exact source paths and/or reviewed-ops from the Supervisor without starting another session. External observe sessions must request human review instead."); return Ok(ExitCode::SUCCESS); } if let Some((paths, reviewed_ops)) = mutation_request_options(args) { @@ -1229,8 +1229,14 @@ pub fn orchestrator(args: &[String]) -> Result { RequestReview(&'a str), Clarification(&'a str), AutoClarification(&'a str), - ReadOnly { result: &'a str, reviewer: &'a str }, - HumanReview { result: &'a str, reviewer: &'a str }, + ReadOnly { + result: &'a str, + reviewer: Option<&'a str>, + }, + HumanReview { + result: &'a str, + reviewer: &'a str, + }, External(&'a str), } let route = if args.len() == 1 && args[0] == "complete" { @@ -1277,15 +1283,14 @@ pub fn orchestrator(args: &[String]) -> Result { && args[2] == "--result-file" { CompletionRoute::AutoClarification(&args[3]) - } else if args.len() == 6 + } else if (args.len() == 4 || (args.len() == 6 && args[4] == "--reviewer")) && args[0] == "complete" && args[1] == "--read-only" && args[2] == "--result-file" - && args[4] == "--reviewer" { CompletionRoute::ReadOnly { result: &args[3], - reviewer: &args[5], + reviewer: args.get(5).map(String::as_str), } } else if args.len() == 6 && args[0] == "complete" diff --git a/runtime/src/workflow.rs b/runtime/src/workflow.rs index dcfb2e6..9872b1c 100644 --- a/runtime/src/workflow.rs +++ b/runtime/src/workflow.rs @@ -1551,7 +1551,8 @@ pub fn supervisor_complete_observe(id: &str) -> Result { let store = Store::configured()?; let p = store.paths(id)?; let _lock = store.lock(&p)?; - let mut state = observe_completion_state(&p, id)?; + let mut state = shortcut_completion_state(&store, &p, id)?; + validate_observation_launches(&workflow_launches(&store, id)?)?; let result_hash = shortcut_result_hash(&store)?; let result = format!("observe:{result_hash}"); complete_shortcut(&p, &mut state, "observe", &result)?; @@ -1649,6 +1650,8 @@ fn require_observe_authority() -> Result<(), String> { } } +// Escalation does not claim success or clear pending work. Keep its gate +// separate from successful read-only completion so findings can reach a human. fn observe_completion_state(paths: &Paths, id: &str) -> Result, String> { let state = read_env(&paths.state, id)?; if state_value(&state, "phase") != "pre-implementation" { @@ -1839,75 +1842,79 @@ fn parse_human_review_request(report: &str) -> Result, Ok(Some((question.into(), reason.into()))) } -/// Completes a repository investigation whose workers and independent reviewer -/// were all mechanically read-only and whose repository diff stayed empty. -pub fn supervisor_complete_read_only(id: &str, reviewer: &str) -> Result { +/// Completes a mechanically read-only investigation. An explicitly requested +/// semantic reviewer remains a quality gate, but is not required for authority. +pub fn supervisor_complete_read_only(id: &str, reviewer: Option<&str>) -> Result { require_supervisor_completion_authority()?; - valid_id("read-only reviewer", reviewer)?; + if let Some(reviewer) = reviewer { + valid_id("read-only reviewer", reviewer)?; + } let store = Store::configured()?; let p = store.paths(id)?; let _lock = store.lock(&p)?; let mut state = shortcut_completion_state(&store, &p, id)?; - let diff = crate::snapshot::canonical_diff(&config::root()?, "HEAD")?; - let diff_hash = format!("{:x}", Sha256::digest(&diff)); let launches = workflow_launches(&store, id)?; if launches.is_empty() { return Err("read-only completion requires at least one reader launch".into()); } validate_read_only_launches(&launches, reviewer)?; - let evidence_dir = store.state_dir.join("reviewer-evidence").join(reviewer); - let evidence = read_simple_env(&evidence_dir.join("evidence.env"))?; - if state_value(&evidence, "role") != "reviewer" - || state_value(&evidence, "access") != "read-only" - || state_value(&evidence, "workflow_id") != id - || state_value(&evidence, "state") != "completed" - { - return Err( - "read-only reviewer evidence is not supervisor-sealed for this workflow".into(), - ); - } - let marker = format!("review-record: type=read-only-integrity verdict=pass diff={diff_hash}"); - let findings_marker = - format!("review-record: type=read-only-integrity verdict=findings diff={diff_hash}"); - let report = fs::read_to_string(evidence_dir.join("last-message.txt")) - .map_err(io_error("read read-only reviewer evidence"))?; - let passing_markers = report - .lines() - .filter(|line| review_marker_matches(line, &marker)) - .count(); - let findings_markers = report - .lines() - .filter(|line| review_marker_matches(line, &findings_marker)) - .count(); - if passing_markers != 1 || findings_markers != 0 { - return Err( - "read-only reviewer evidence requires exactly one passing integrity marker and no findings marker" - .into(), - ); - } - let review_id = format!("read-only-integrity-{}", state_value(&state, "iteration")); - let mut reviews = read_reviews(&p.reviews)?; - if !reviews.iter().any(|row| row.get(0) == review_id) { - reviews.push(Review { - fields: [ - review_id.clone(), - "read-only-integrity".into(), - "pass".into(), - diff_hash.clone(), - evidence_dir.display().to_string(), - state_value(&state, "iteration").into(), - timestamp(), - reviewer.into(), - ], - }); - write_reviews(&p.reviews, &reviews)?; - event( - &p.events, - "review_recorded", - &format!( + if let Some(reviewer) = reviewer { + let diff = crate::snapshot::canonical_diff(&config::root()?, "HEAD")?; + let diff_hash = format!("{:x}", Sha256::digest(&diff)); + let evidence_dir = store.state_dir.join("reviewer-evidence").join(reviewer); + let evidence = read_simple_env(&evidence_dir.join("evidence.env"))?; + if state_value(&evidence, "role") != "reviewer" + || state_value(&evidence, "access") != "read-only" + || state_value(&evidence, "workflow_id") != id + || state_value(&evidence, "state") != "completed" + { + return Err( + "read-only reviewer evidence is not supervisor-sealed for this workflow".into(), + ); + } + let marker = + format!("review-record: type=read-only-integrity verdict=pass diff={diff_hash}"); + let findings_marker = + format!("review-record: type=read-only-integrity verdict=findings diff={diff_hash}"); + let report = fs::read_to_string(evidence_dir.join("last-message.txt")) + .map_err(io_error("read read-only reviewer evidence"))?; + if format!("{:x}", Sha256::digest(report.as_bytes())) + != state_value(&evidence, "output_sha256") + { + return Err("read-only reviewer evidence failed its supervisor output seal".into()); + } + let passing_markers = report + .lines() + .filter(|line| review_marker_matches(line, &marker)) + .count(); + let findings_markers = report + .lines() + .filter(|line| review_marker_matches(line, &findings_marker)) + .count(); + if passing_markers != 1 || findings_markers != 0 { + return Err("read-only reviewer evidence requires exactly one passing integrity marker and no findings marker".into()); + } + let review_id = format!("read-only-integrity-{}", state_value(&state, "iteration")); + let mut reviews = read_reviews(&p.reviews)?; + if !reviews.iter().any(|row| row.get(0) == review_id) { + reviews.push(Review { + fields: [ + review_id.clone(), + "read-only-integrity".into(), + "pass".into(), + diff_hash.clone(), + evidence_dir.display().to_string(), + state_value(&state, "iteration").into(), + timestamp(), + reviewer.into(), + ], + }); + write_reviews(&p.reviews, &reviews)?; + let detail = format!( "review_id={review_id}\ttype=read-only-integrity\tverdict=pass\tdiff_hash={diff_hash}" - ), - )?; + ); + event(&p.events, "review_recorded", &detail)?; + } } let result_hash = shortcut_result_hash(&store)?; let result = format!("read-only:{result_hash}"); @@ -1917,24 +1924,15 @@ pub fn supervisor_complete_read_only(id: &str, reviewer: &str) -> Result)], - reviewer: &str, + reviewer: Option<&str>, ) -> Result<(), String> { + validate_observation_launches(launches)?; let mut readers = 0usize; let mut reviewer_launch = None; for (name, launch) in launches { - if state_value(launch, "access") != "read-only" { - return Err(format!( - "read-only completion found non-read-only launch: {name}" - )); - } - if state_value(launch, "state") != "completed" { - return Err(format!( - "read-only completion found unfinished launch: {name}" - )); - } match state_value(launch, "role") { "reader" => readers += 1, - "reviewer" if name == reviewer => reviewer_launch = Some(launch), + "reviewer" if Some(name.as_str()) == reviewer => reviewer_launch = Some(launch), "reviewer" => {} role => { return Err(format!( @@ -1946,10 +1944,30 @@ fn validate_read_only_launches( if readers == 0 { return Err("read-only completion requires at least one completed reader".into()); } - if reviewer_launch.is_none() { - return Err(format!( - "read-only completion requires completed reviewer launch: {reviewer}" - )); + if let Some(reviewer) = reviewer { + if reviewer_launch.is_none() { + return Err(format!( + "read-only completion requires completed reviewer launch: {reviewer}" + )); + } + } + Ok(()) +} + +fn validate_observation_launches( + launches: &[(String, BTreeMap)], +) -> Result<(), String> { + for (name, launch) in launches { + if state_value(launch, "access") != "read-only" { + return Err(format!( + "read-only completion found non-read-only launch: {name}" + )); + } + if state_value(launch, "state") != "completed" { + return Err(format!( + "read-only completion found unfinished launch: {name}" + )); + } } Ok(()) } @@ -1998,6 +2016,16 @@ fn shortcut_completion_state( if reviewed_operation_receipt_exists(&store.state_dir)? { return Err("shortcut completion forbids external operation receipts".into()); } + let requests = store.state_dir.join("operations/requests"); + if requests.is_dir() + && fs::read_dir(requests) + .map_err(io_error("list operation requests"))? + .next() + .is_some() + { + return Err("shortcut completion forbids reviewed operation requests".into()); + } + crate::subagent::external_completion_gate_check()?; Ok(state) } @@ -2933,28 +2961,31 @@ mod tests { launch("reviewer", "read-only", "completed"), ), ]; - assert!(validate_read_only_launches(&valid, "read-only-integrity-reviewer-01").is_ok()); + assert!( + validate_read_only_launches(&valid, Some("read-only-integrity-reviewer-01")).is_ok() + ); + + assert!(validate_read_only_launches(&valid[..1], None).is_ok()); + assert!(validate_read_only_launches(&valid[..1], Some("missing-reviewer")).is_err()); + assert!(validate_read_only_launches(&[], None).is_err()); + assert!(validate_observation_launches(&[]).is_ok()); let mut writer = valid.clone(); writer.push(( "worker-01".into(), launch("worker", "workspace-write", "completed"), )); - assert!( - validate_read_only_launches(&writer, "read-only-integrity-reviewer-01") - .unwrap_err() - .contains("non-read-only") - ); + assert!(validate_read_only_launches(&writer, None) + .unwrap_err() + .contains("non-read-only")); let unfinished = vec![ ("reader-01".into(), launch("reader", "read-only", "running")), valid[1].clone(), ]; - assert!( - validate_read_only_launches(&unfinished, "read-only-integrity-reviewer-01") - .unwrap_err() - .contains("unfinished") - ); + assert!(validate_read_only_launches(&unfinished, None) + .unwrap_err() + .contains("unfinished")); } #[test] diff --git a/tests/lifecycle.sh b/tests/lifecycle.sh index 92c9ab5..76631d1 100755 --- a/tests/lifecycle.sh +++ b/tests/lifecycle.sh @@ -26,6 +26,7 @@ printf 'test\n' >"$TEST_REPO/README.md" git -C "$TEST_REPO" add README.md git -C "$TEST_REPO" commit -q -m initial TEST_BRANCH="$(git -C "$TEST_REPO" branch --show-current)" +export MULTIAGENT_ROOT="$TEST_REPO" BYPASS_STATE="$TEST_TMP/bypass-state" if MULTIAGENT_ROOT="$TEST_REPO" MULTIAGENT_STATE_DIR="$BYPASS_STATE" \ @@ -154,12 +155,16 @@ printf 'Diagnose the alert and propose a bounded repair if necessary.\n' >"$REPA printf 'Approve repairing deploy/service.yaml and allowing reviewed operations?\n' >"$REPAIR_RESULT" MULTIAGENT_STATE_DIR="$REPAIR_STATE" MULTIAGENT_ORIGINAL_TASK_FILE="$REPAIR_TASK" \ "$MULTIAGENT" workflow init WF-REPAIR >/dev/null +# Escalation must preserve unresolved work rather than claiming success. +mkdir -p "$REPAIR_STATE/todos/NEEDS-HUMAN" +printf 'open\n' >"$REPAIR_STATE/todos/NEEDS-HUMAN/status" MULTIAGENT_STATE_DIR="$REPAIR_STATE" MULTIAGENT_WORKFLOW_ID=WF-REPAIR \ MULTIAGENT_LIFECYCLE_ENFORCEMENT=1 MULTIAGENT_AUTHORITY_SCOPE=observe \ "$MULTIAGENT" orchestrator complete --request-review --result-file "$REPAIR_RESULT" \ --path deploy/service.yaml --reviewed-ops >/dev/null assert_contains "$REPAIR_STATE/workflows/WF-REPAIR/lifecycle/lifecycle.env" \ "terminal_outcome=review_requested" +assert_contains "$REPAIR_STATE/todos/NEEDS-HUMAN/status" "open" assert_contains "$REPAIR_STATE/workflows/WF-REPAIR/lifecycle/human-review-repair-paths.json" \ '["deploy/service.yaml"]' assert_contains "$REPAIR_STATE/workflows/WF-REPAIR/lifecycle/human-review-effects.json" '["source-write","reviewed-ops"]' @@ -576,6 +581,8 @@ EMPTY_DIFF_SHA256="e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b printf 'review-record: type=read-only-integrity verdict=pass diff=%s\n' \ "$EMPTY_DIFF_SHA256" \ >"$READ_ONLY_STATE/reviewer-evidence/$READ_ONLY_REVIEWER/last-message.txt" +printf 'output_sha256=%s\n' "$(shasum -a 256 "$READ_ONLY_STATE/reviewer-evidence/$READ_ONLY_REVIEWER/last-message.txt" | awk '{print $1}')" \ + >>"$READ_ONLY_STATE/reviewer-evidence/$READ_ONLY_REVIEWER/evidence.env" READ_ONLY_RESULT="$READ_ONLY_STATE/read-only-result.md" printf 'The repository evidence supports the bounded answer.\n' >"$READ_ONLY_RESULT" MULTIAGENT_ROOT="$SHORTCUT_REPO" MULTIAGENT_STATE_DIR="$READ_ONLY_STATE" \ @@ -612,6 +619,107 @@ if MULTIAGENT_ROOT="$SHORTCUT_REPO" MULTIAGENT_STATE_DIR="$READ_ONLY_WRITE_STATE fi assert_contains "$TEST_TMP/read-only-write.out" "non-read-only launch" +# A caller that explicitly asks for a reviewer cannot substitute its report. +REVIEW_SEAL_STATE="$TEST_TMP/read-only-review-seal" +cp -R "$READ_ONLY_STATE" "$REVIEW_SEAL_STATE" +sed -i.bak \ + -e 's/phase=complete/phase=pre-implementation/' \ + -e 's/^candidate_diff_hash=.*/candidate_diff_hash=/' \ + -e 's/^reviewed_diff_hash=.*/reviewed_diff_hash=/' \ + "$REVIEW_SEAL_STATE/workflows/WF-READ-ONLY/lifecycle/lifecycle.env" +printf 'changed report\n' >>"$REVIEW_SEAL_STATE/reviewer-evidence/$READ_ONLY_REVIEWER/last-message.txt" +if MULTIAGENT_ROOT="$SHORTCUT_REPO" MULTIAGENT_STATE_DIR="$REVIEW_SEAL_STATE" \ + MULTIAGENT_WORKFLOW_ID=WF-READ-ONLY MULTIAGENT_LIFECYCLE_ENFORCEMENT=1 \ + "$MULTIAGENT" orchestrator complete --read-only --result-file "$REVIEW_SEAL_STATE/read-only-result.md" \ + --reviewer "$READ_ONLY_REVIEWER" >"$TEST_TMP/read-only-review-seal.out" 2>&1; then + echo 'expected requested reviewer evidence seal to be checked' >&2 + exit 1 +fi +assert_contains "$TEST_TMP/read-only-review-seal.out" 'failed its supervisor output seal' + +# All supported read-only routes enforce completion without a model verdict. +for shortcut_route in --read-only --observe --auto; do + for shortcut_case in valid direct-evidence running writer source-state task-digest lifecycle-todo todo finding dirty untracked reviewed pending-request no-reader; do + shortcut_state="$TEST_TMP/shortcut-${shortcut_route#--}-$shortcut_case" + mkdir -p "$shortcut_state/launch-authorizations/reader-01" + MULTIAGENT_STATE_DIR="$shortcut_state" MULTIAGENT_ORIGINAL_TASK_FILE="$SHORTCUT_TASK" \ + "$MULTIAGENT" workflow init WF-SHORTCUT >/dev/null + shortcut_lifecycle="$shortcut_state/workflows/WF-SHORTCUT/lifecycle" + printf '%s\n' 'name=reader-01' 'role=reader' 'access=read-only' \ + 'workflow_id=WF-SHORTCUT' 'state=completed' \ + >"$shortcut_state/launch-authorizations/reader-01/launch.env" + printf 'The completed reader supports this answer.\n' >"$shortcut_state/result.md" + expected_rejection="" + case "$shortcut_case" in + valid) ;; + direct-evidence) + mkdir -p "$shortcut_state/operations/direct-read" + printf '{}\n' >"$shortcut_state/operations/direct-read/direct-request.json" + printf '{}\n' >"$shortcut_state/operations/direct-read/receipt.redacted.json" + ;; + running) + sed -i.bak 's/state=completed/state=running/' "$shortcut_state/launch-authorizations/reader-01/launch.env" + expected_rejection='unfinished launch' ;; + writer) + sed -i.bak 's/access=read-only/access=workspace-write/' "$shortcut_state/launch-authorizations/reader-01/launch.env" + expected_rejection='non-read-only launch' ;; + source-state) + sed -i.bak 's/^iteration_plan_sha256=.*/iteration_plan_sha256=sealed-plan/' "$shortcut_lifecycle/lifecycle.env" + expected_rejection='started source lifecycle' ;; + task-digest) + sed -i.bak 's/^original_task_sha256=.*/original_task_sha256=wrong-digest/' "$shortcut_lifecycle/lifecycle.env" + expected_rejection='original task' ;; + lifecycle-todo) + printf 'TODO-A\tdirect\tunfinished work\ttest\topen\n' >>"$shortcut_lifecycle/todos.tsv" + expected_rejection='active TODOs' ;; + todo) + mkdir -p "$shortcut_state/todos/TODO-OPEN" + printf 'open\n' >"$shortcut_state/todos/TODO-OPEN/status" + expected_rejection='open-todo' ;; + finding) + mkdir -p "$shortcut_state/findings/FINDING-OPEN" + printf 'severity=blocking\n' >"$shortcut_state/findings/FINDING-OPEN/finding.env" + expected_rejection='unqueued-blocking-finding' ;; + dirty) + printf 'changed\n' >>"$SHORTCUT_REPO/README.md" + expected_rejection='unchanged repository diff' ;; + untracked) + printf 'new source\n' >"$SHORTCUT_REPO/new-source.txt" + expected_rejection='unchanged repository diff' ;; + reviewed) + mkdir -p "$shortcut_state/operations/reviewed-op" + printf '{}\n' >"$shortcut_state/operations/reviewed-op/receipt.json" + expected_rejection='external operation receipts' ;; + pending-request) + mkdir -p "$shortcut_state/operations/requests/digest" + printf '{}\n' >"$shortcut_state/operations/requests/digest/request.json" + expected_rejection='reviewed operation requests' ;; + no-reader) + rm -rf "$shortcut_state/launch-authorizations/reader-01" + if [[ "$shortcut_route" == --read-only ]]; then + expected_rejection='at least one reader' + fi ;; + esac + shortcut_exit=0 + MULTIAGENT_ROOT="$SHORTCUT_REPO" MULTIAGENT_STATE_DIR="$shortcut_state" \ + MULTIAGENT_WORKFLOW_ID=WF-SHORTCUT MULTIAGENT_LIFECYCLE_ENFORCEMENT=1 \ + MULTIAGENT_AUTHORITY_SCOPE=user \ + "$MULTIAGENT" orchestrator complete "$shortcut_route" --result-file "$shortcut_state/result.md" \ + >"$shortcut_state/completion.out" 2>&1 || shortcut_exit=$? + git -C "$SHORTCUT_REPO" restore README.md + rm -f "$SHORTCUT_REPO/new-source.txt" + if [[ -n "$expected_rejection" ]]; then + [[ "$shortcut_exit" -ne 0 ]] || { echo "unexpected completion: $shortcut_route $shortcut_case" >&2; exit 1; } + assert_contains "$shortcut_state/completion.out" "$expected_rejection" + assert_contains "$shortcut_lifecycle/lifecycle.env" 'phase=pre-implementation' + else + [[ "$shortcut_exit" -eq 0 ]] || { cat "$shortcut_state/completion.out" >&2; exit 1; } + assert_contains "$shortcut_lifecycle/lifecycle.env" 'terminal_outcome=succeeded' + [[ "$(wc -l <"$shortcut_lifecycle/reviews.tsv")" -eq 1 ]] || exit 1 + fi + done +done + EXTERNAL_STATE="$TEST_TMP/external-state" EXTERNAL_ROOT="$TEST_TMP/external-non-git-root" mkdir -p "$EXTERNAL_ROOT" "$EXTERNAL_STATE/operations/OP-EXTERNAL"