From 1fb29495615d4def4a01fa0ec9159c100e7e8763 Mon Sep 17 00:00:00 2001 From: Hare Date: Tue, 25 Aug 2026 04:45:17 +0900 Subject: [PATCH] fix: give reviewer write-scoped command tools --- crates/workdir/src/delegation.rs | 43 +++++++++++++++++++++++++++++- crates/worker/src/spawn/tool.rs | 35 ++++++++++++------------ docs/development/work-items.md | 2 +- resources/flows/coder-review.dcdl | 4 +-- resources/prompts/role/coder.md | 2 +- resources/prompts/role/reviewer.md | 2 +- 6 files changed, 64 insertions(+), 24 deletions(-) diff --git a/crates/workdir/src/delegation.rs b/crates/workdir/src/delegation.rs index f16f86ee..4a5b313f 100644 --- a/crates/workdir/src/delegation.rs +++ b/crates/workdir/src/delegation.rs @@ -311,7 +311,10 @@ impl DelegatingWorkdirSession { if !self.capabilities.supports(WorkdirSessionCapability::Read) || (writable && (!self.capabilities.supports(WorkdirSessionCapability::Write) - || !self.capabilities.supports(WorkdirSessionCapability::Edit))) + || !self.capabilities.supports(WorkdirSessionCapability::Edit) + || !self + .capabilities + .supports(WorkdirSessionCapability::Command))) { return Err(WorkdirError::Denied( "parent workdir session cannot delegate the requested capabilities".into(), @@ -342,6 +345,7 @@ impl DelegatingWorkdirSession { if writable { delegated.push(WorkdirSessionCapability::Write); delegated.push(WorkdirSessionCapability::Edit); + delegated.push(WorkdirSessionCapability::Command); } Ok(WorkdirSessionCapabilities::from_capabilities(delegated)) } @@ -929,6 +933,43 @@ mod tests { .delegate(request("leased", WorkdirDelegationPermission::Write)) .await .unwrap(); + assert!( + child + .capabilities + .supports(WorkdirSessionCapability::Command) + ); + let command = child + .scoped_session + .start_command(CommandRequest { + command: "printf child-command".into(), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("delegated-child-command".into()), + }) + .await + .unwrap(); + let command_output = child + .scoped_session + .command_output(CommandOutputRequest { + handle: command, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert_eq!(command_output.content, "child-command"); + assert!( + parent + .start_command(CommandRequest { + command: "printf parent-command".into(), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("blocked-parent-command".into()), + }) + .await + .is_err() + ); assert!(matches!( parent.write(write("leased/file", "parent")).await, diff --git a/crates/worker/src/spawn/tool.rs b/crates/worker/src/spawn/tool.rs index 842160e9..3d2d8782 100644 --- a/crates/worker/src/spawn/tool.rs +++ b/crates/worker/src/spawn/tool.rs @@ -329,13 +329,13 @@ fn validate_reviewer_handoff(input: &SubWorkerSpawnInput) -> Result<(), ToolErro "reviewer handoff requires the explicit effective profile builtin:reviewer".to_string(), )); } - if input + if !input .scope .iter() .any(|rule| matches!(rule.permission, PermissionInput::Write)) { return Err(ToolError::InvalidArgument( - "Merge Request Reviewer SubWorkers must have read-only delegated scope".to_string(), + "Merge Request Reviewer SubWorkers must include writable delegated scope".to_string(), )); } Ok(()) @@ -1008,28 +1008,28 @@ mod tests { } #[test] - fn reviewer_handoff_requires_explicit_builtin_profile_and_read_only_scope() { + fn reviewer_handoff_requires_explicit_builtin_profile_and_writable_scope() { let valid: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({ "name":"reviewer","task":"review","profile":"builtin:reviewer", - "scope":[{"target":"work","permission":"read"}], + "scope":[{"target":"work","permission":"write"}], "review":{"ticket_id":"T1"} })) .unwrap(); assert!(validate_reviewer_handoff(&valid).is_ok()); let wrong_profile: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({ "name":"reviewer","task":"review","profile":"builtin:coder", - "scope":[{"target":"work","permission":"read"}], - "review":{"ticket_id":"T1"} - })) - .unwrap(); - assert!(validate_reviewer_handoff(&wrong_profile).is_err()); - let writable: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({ - "name":"reviewer","task":"review","profile":"builtin:reviewer", "scope":[{"target":"work","permission":"write"}], "review":{"ticket_id":"T1"} })) .unwrap(); - assert!(validate_reviewer_handoff(&writable).is_err()); + assert!(validate_reviewer_handoff(&wrong_profile).is_err()); + let read_only: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({ + "name":"reviewer","task":"review","profile":"builtin:reviewer", + "scope":[{"target":"work","permission":"read"}], + "review":{"ticket_id":"T1"} + })) + .unwrap(); + assert!(validate_reviewer_handoff(&read_only).is_err()); } fn abs_rule(path: &Path, permission: Permission) -> ScopeRule { @@ -1079,7 +1079,7 @@ extract_threshold = 4000 } #[tokio::test] - async fn reviewer_profile_spawns_and_notifies_parent_controller() { + async fn reviewer_profile_write_scope_exposes_command_tools_and_notifies_parent_controller() { let runtime = TempDir::new().unwrap(); let workspace_root = runtime.path().join("project"); let available_profiles = write_project_profile_registry( @@ -1140,7 +1140,7 @@ extract_threshold = 4000 "task": "review immutable commit", "scope": [{ "target": ".", - "permission": "read", + "permission": "write", "recursive": true }] }); @@ -1171,11 +1171,10 @@ extract_threshold = 4000 let record = registry .get_internal("reviewer-child") .expect("Internal reviewer registry record"); - assert!(record.installed_tools.iter().any(|name| name == "Read")); - for denied in ["Write", "Edit", "Bash"] { + for required in ["Read", "Write", "Edit", "Glob", "Grep", "Bash"] { assert!( - !record.installed_tools.iter().any(|name| name == denied), - "read-only child unexpectedly received {denied}: {:?}", + record.installed_tools.iter().any(|name| name == required), + "write-scoped child is missing {required}: {:?}", record.installed_tools ); } diff --git a/docs/development/work-items.md b/docs/development/work-items.md index ebd29c50..9f2f7926 100644 --- a/docs/development/work-items.md +++ b/docs/development/work-items.md @@ -242,7 +242,7 @@ Implementation normally happens in a child git worktree created by the Orchestra ### 5. Review -The assigned Coder launches the Reviewer as an actual direct-child `builtin:reviewer` SubWorker with read-only scope and a structured handoff bound to the current immutable Merge Request revision. Server authority revalidates the parent assignment, Runtime-owned child session, effective profile, one-shot review attempt, and revision; prose output is not approval. +The assigned Coder launches the Reviewer as an actual direct-child `builtin:reviewer` SubWorker with write scope, so it can use the Workdir command tools required for inspection and validation, and a structured handoff bound to the current immutable Merge Request revision. Server authority revalidates the parent assignment, Runtime-owned child session, effective profile, one-shot review attempt, and revision; prose output is not approval. The Reviewer records the structured result with `MergeRequestReview`. Request changes requires a new immutable revision and a fresh child attempt. The Orchestrator uses `MergeRequestReadinessCheck` and then `MergeRequestComplete` for guarded integration with operation-id dedupe/CAS semantics; Flow transitions are not completion authority. diff --git a/resources/flows/coder-review.dcdl b/resources/flows/coder-review.dcdl index e90b63df..9e4c2814 100644 --- a/resources/flows/coder-review.dcdl +++ b/resources/flows/coder-review.dcdl @@ -15,7 +15,7 @@ }; review = { - instructions = "Use the current Ticket Merge Request as review authority. Confirm its immutable source selector resolves to the exact committed implementation HEAD, then spawn one actual direct-child SubWorker with profile builtin:reviewer, read-only scope, and a structured review handoff bound to the current immutable Merge Request revision. The trusted spawn layer records `ReviewRequested`; do not place commit/ref identity, capability material, or a prewritten verdict in model input. The child must commit MergeRequestReview; prose output and Worker observation are not approval authority. After the structured current-revision result exists, request a Flow transition."; + instructions = "Use the current Ticket Merge Request as review authority. Confirm its immutable source selector resolves to the exact committed implementation HEAD, then spawn one actual direct-child SubWorker with profile builtin:reviewer, write scope for Workdir inspection and command validation, and a structured review handoff bound to the current immutable Merge Request revision. The trusted spawn layer records `ReviewRequested`; do not place commit/ref identity, capability material, or a prewritten verdict in model input. The child must commit MergeRequestReview; prose output and Worker observation are not approval authority. After the structured current-revision result exists, request a Flow transition."; transitions = { approved = { target = "complete"; @@ -29,7 +29,7 @@ }; fix = { - instructions = "Resolve every open Reviewer finding on the same Ticket work branch, rerun the validation affected by the fixes, commit the corrected implementation as a new revision, and preserve concrete evidence. Publish only the updated Ticket work branch with a normal non-force push, verify that the configured repository provider resolves the published source ref to the exact new HEAD, and update the linked Merge Request so its current revision records that same subject. Request review from a fresh read-only Reviewer child so the trusted spawn layer captures the new immutable subject. Do not rewrite the previously reviewed commit, claim approval from the prior request_changes review, push the target branch, push tags or unrelated refs, force-push, merge, delete branches, or discard pre-existing changes. Request a Flow transition only after the corrected committed revision is published and ready for a new independent review."; + instructions = "Resolve every open Reviewer finding on the same Ticket work branch, rerun the validation affected by the fixes, commit the corrected implementation as a new revision, and preserve concrete evidence. Publish only the updated Ticket work branch with a normal non-force push, verify that the configured repository provider resolves the published source ref to the exact new HEAD, and update the linked Merge Request so its current revision records that same subject. Request review from a fresh Reviewer child with write scope so it can use the Workdir command tools required for inspection and validation while the trusted spawn layer captures the new immutable subject. Do not rewrite the previously reviewed commit, claim approval from the prior request_changes review, push the target branch, push tags or unrelated refs, force-push, merge, delete branches, or discard pre-existing changes. Request a Flow transition only after the corrected committed revision is published and ready for a new independent review."; transitions = { review = { target = "review"; diff --git a/resources/prompts/role/coder.md b/resources/prompts/role/coder.md index b8e2dbd3..7f4fdeaa 100644 --- a/resources/prompts/role/coder.md +++ b/resources/prompts/role/coder.md @@ -6,6 +6,6 @@ Before opening a Merge Request, publish only the committed Ticket work branch wi {% include "common.git" %} -Before review, open a Merge Request with immutable `selector_from` / `selector_to`. Spawn the Reviewer only as your actual direct-child `builtin:reviewer` SubWorker, delegate read-only scope, and pass only the Ticket id in the structured review handoff. The host resolves `selector_from`, captures the immutable `subject_ref`, appends `ReviewRequested`, and injects the review capability; commit/ref identity is not model input. Reviewer prose is not approval: the child must commit `MergeRequestReview` through its injected capability authority. +Before review, open a Merge Request with immutable `selector_from` / `selector_to`. Spawn the Reviewer only as your actual direct-child `builtin:reviewer` SubWorker, delegate write scope so it can use the Workdir command tools required for inspection and validation, and pass only the Ticket id in the structured review handoff. The host resolves `selector_from`, captures the immutable `subject_ref`, appends `ReviewRequested`, and injects the review capability; commit/ref identity is not model input. Reviewer prose is not approval: the child must commit `MergeRequestReview` through its injected capability authority. A request-changes result requires a freshly published immutable subject and a fresh Reviewer child request. Flow terminal state is not Ticket completion authority. After the exact current Merge Request subject has authoritative approval, keep that source ref immutable, leave concise implementation evidence on the Ticket when useful, and hand off integration to the Orchestrator. Do not update the target selector. Do not call `MergeRequestComplete`. diff --git a/resources/prompts/role/reviewer.md b/resources/prompts/role/reviewer.md index 24677be6..c9cb1383 100644 --- a/resources/prompts/role/reviewer.md +++ b/resources/prompts/role/reviewer.md @@ -1,6 +1,6 @@ You are the Ticket Reviewer role running as an actual Runtime-owned direct child of the assigned Coder. -Keep role behavior here and treat the first committed user message as bounded Ticket/Merge Request context only, never as a supplied verdict. Review the host-captured `ReviewRequested.subject_ref` against Ticket intent, binding decisions/invariants, acceptance criteria, and project design boundaries. Use read-only inspection and focused validation; do not merge, close, mutate the Workdir, update a repository ref, or take over implementation. +Keep role behavior here and treat the first committed user message as bounded Ticket/Merge Request context only, never as a supplied verdict. Review the host-captured `ReviewRequested.subject_ref` against Ticket intent, binding decisions/invariants, acceptance criteria, and project design boundaries. Use the available Workdir inspection and command tools for focused validation, but do not intentionally modify implementation files, merge, close, update a repository ref, or take over implementation. Your prose response is not review authority. Before finishing, call `MergeRequestReview` exactly once with `approve` or `request_changes`, a bounded evidence summary, and concrete structured findings. Capability authority and subject identity are injected by your child Workspace client and are not model inputs. The Server re-resolves `selector_from`; if it moved, submission records cancellation and fails rather than approving stale work.