feat: default Coder ticket work to review flow

crates/worker/src/feature/builtin/orchestration.rs

@@ -21,7 +21,7 @@ use crate::feature::{
 
 const FEATURE_ID: &str = "orchestration";
 const TOOL_NAME: &str = "SpawnTicketWorker";
-const TOOL_DESCRIPTION: &str = "Spawn and atomically assign a generic Ticket Worker for the natural-language initial request. Profile is optional (defaults to builtin:ticket-worker); Flow and repository/Workdir attachments are optional. Results and return choices follow user intent, not a universal approval or MR gate. Orchestrator starts must supply the observed orchestrator_assignment_id; stale delegations cannot start new work. Assign is enough (no Queue/ready); unresolved dependencies are diagnostics for partial work and never start implicitly. Backend validates claims and derives attachment access from Ticket targets; acceptance follows spawn, initial input, assignment, and resource finalization. Profile/Flow selection grants neither trusted review nor merge authority.";
+const TOOL_DESCRIPTION: &str = "Spawn and atomically assign a generic Ticket Worker for the natural-language initial request. Profile is optional (defaults to builtin:ticket-worker); repository/Workdir attachments are optional. For builtin:coder (or coder), omitted flow defaults to builtin:coder-review so the Coder owns the Reviewer SubWorker cycle and MR verdict. Explicit flow: null disables Flow; a string selects another Flow. Other profiles get no default Flow. Results and return choices follow user intent, not a universal approval or MR gate. Orchestrator starts must supply the observed orchestrator_assignment_id; stale delegations cannot start new work. Assign is enough (no Queue/ready); unresolved dependencies are diagnostics for partial work and never start implicitly. Backend validates claims and derives attachment access from Ticket targets; acceptance follows spawn, initial input, assignment, and resource finalization. Profile/Flow selection grants neither trusted review nor merge authority.";
 
 #[derive(Debug, Default)]
 pub struct OrchestrationFeature;
@@ -88,15 +88,26 @@ struct SpawnTicketWorkerInput {
     profile: Option<String>,
     /// Initial user request delivered through the normal typed submission path.
     initial_request: String,
-    /// Optional Flow selector, delivered as a typed Flow segment before the request.
-    #[serde(default)]
-    flow: Option<String>,
+    /// Flow selector, delivered before the request. Omission defaults to
+    /// builtin:coder-review for builtin:coder (or coder); other profiles get no Flow.
+    /// Explicit null disables Flow; an explicit string selects that Flow instead.
+    #[serde(default, deserialize_with = "deserialize_present_flow")]
+    flow: Option<Option<String>>,
     /// Alias-keyed Workdir selections; omit or leave empty for a Worker without
     /// Workdirs. Backend derives effective capabilities; callers cannot request them.
     #[serde(default)]
     workdir_attachments: Vec<SpawnTicketWorkerWorkdirInput>,
 }
 
+// Serde's ordinary Option would collapse explicit null and an absent field.
+// Only an absent field selects the profile-specific default.
+fn deserialize_present_flow<'de, D>(deserializer: D) -> Result<Option<Option<String>>, D::Error>
+where
+    D: serde::Deserializer<'de>,
+{
+    Option::<String>::deserialize(deserializer).map(Some)
+}
+
 struct SpawnTicketWorkerTool {
     ticket_service: Arc<dyn TicketService>,
     worker_service: Arc<dyn WorkerLifecycleService>,
@@ -125,7 +136,13 @@ impl Tool for SpawnTicketWorkerTool {
             .transpose()?
             .unwrap_or_else(|| "builtin:ticket-worker".into());
         let initial_request = non_empty(input.initial_request, "initial_request")?;
-        let flow = input.flow.map(|flow| non_empty(flow, "flow")).transpose()?;
+        let flow = match input.flow {
+            Some(flow) => flow.map(|flow| non_empty(flow, "flow")).transpose()?,
+            None if matches!(profile.as_str(), "builtin:coder" | "coder") => {
+                Some("builtin:coder-review".to_string())
+            }
+            None => None,
+        };
         let workdir_attachments = validate_workdir_attachments(input.workdir_attachments)?;
         let mut initial_submit = Vec::new();
         if let Some(selector) = flow {
@@ -445,6 +462,107 @@ mod tests {
         }
     }
 
+    #[tokio::test]
+    async fn spawn_ticket_worker_defaults_coder_flow_but_preserves_explicit_selection() {
+        // Exercise the LLM JSON boundary and the typed request passed to lifecycle.
+        // Lifecycle/network execution is recorded, not launched by this test.
+        let cases = [
+            ("builtin:coder", None, Some("builtin:coder-review")),
+            ("coder", None, Some("builtin:coder-review")),
+            (" builtin:coder ", None, Some("builtin:coder-review")),
+            ("builtin:coder", Some(serde_json::Value::Null), None),
+            ("coder", Some(serde_json::Value::Null), None),
+            (
+                "builtin:coder",
+                Some(serde_json::json!(" project:investigation ")),
+                Some("project:investigation"),
+            ),
+            (
+                "builtin:coder",
+                Some(serde_json::json!("builtin:coder-review")),
+                Some("builtin:coder-review"),
+            ),
+            ("builtin:ticket-worker", None, None),
+            ("builtin:intake", None, None),
+            ("project:coder", None, None),
+            (
+                "project:coder",
+                Some(serde_json::json!("builtin:coder-review")),
+                Some("builtin:coder-review"),
+            ),
+        ];
+        for (profile, flow, expected_flow) in cases {
+            let service = Arc::new(RecordingService::default());
+            let (meta, tool) = definition(
+                Arc::new(FixedTicketService(TicketState::Open)),
+                service.clone(),
+            )();
+            let mut input = input();
+            input["profile"] = serde_json::json!(profile);
+            input["orchestrator_assignment_id"] = serde_json::json!("delegation-742");
+            if let Some(flow) = flow {
+                input["flow"] = flow;
+            }
+            assert!(jsonschema::is_valid(&meta.input_schema, &input));
+            tool.execute(
+                &input.to_string(),
+                ToolExecutionContext::new("call-coder-flow", "batch-1", 0),
+            )
+            .await
+            .unwrap();
+            let requests = service.requests.lock().unwrap();
+            assert_eq!(requests.len(), 1);
+            let request = &requests[0];
+            let mut expected = Vec::new();
+            if let Some(selector) = expected_flow {
+                expected.push(Segment::Flow {
+                    selector: selector.into(),
+                });
+            }
+            expected.push(Segment::text("Investigate Ticket T-719."));
+            assert_eq!(
+                request.initial_submit, expected,
+                "profile={profile}, input={input}"
+            );
+            assert_eq!(request.profile, profile.trim());
+            assert_eq!(
+                request.orchestrator_assignment_id.as_deref(),
+                Some("delegation-742")
+            );
+        }
+    }
+
+    #[tokio::test]
+    async fn spawn_ticket_worker_rejects_invalid_coder_flow_without_falling_back() {
+        let service = Arc::new(RecordingService::default());
+        let (meta, tool) = definition(
+            Arc::new(FixedTicketService(TicketState::Open)),
+            service.clone(),
+        )();
+        for flow in [
+            serde_json::json!(" "),
+            serde_json::json!(false),
+            serde_json::json!({"selector": "builtin:coder-review"}),
+            serde_json::json!([]),
+        ] {
+            let mut input = input();
+            input["profile"] = serde_json::json!("builtin:coder");
+            input["flow"] = flow.clone();
+            if !flow.is_string() {
+                assert!(!jsonschema::is_valid(&meta.input_schema, &input));
+            }
+            let error = tool
+                .execute(
+                    &input.to_string(),
+                    ToolExecutionContext::new("call-invalid-flow", "batch-1", 0),
+                )
+                .await
+                .unwrap_err();
+            assert!(matches!(error, ToolError::InvalidArgument(_)), "{error}");
+        }
+        assert!(service.requests.lock().unwrap().is_empty());
+    }
+
     #[tokio::test]
     async fn spawn_ticket_worker_defers_claim_eligibility_to_backend_not_case_state() {
         for state in [TicketState::Open, TicketState::Closed] {

resources/prompts/role/coder.md

@@ -1,5 +1,5 @@
 You are the assigned Coder. Implement and validate the requested scope in the bound Workdir. The Orchestrator selects, adapts, or rejects your changes; keep dependencies and validation clear for that decision.
 
-Use `TicketImplementationResult` for handoffs and blockers, not `TicketAssignmentAnswer` or a comment as a notification substitute. MR approval is not required for a handoff. Use `builtin:coder-review` only for a requested formal review cycle.
+Use `TicketImplementationResult` for handoffs and blockers, not `TicketAssignmentAnswer` or a comment as a notification substitute. `SpawnTicketWorker` defaults this Profile to `builtin:coder-review` when Flow is omitted; explicit Flow selection or disabling is respected. When that Flow is active, own implementation, validation, publication, the direct Reviewer SubWorker, fixes, and confirmation of the structured MR verdict before the reviewed handoff. Do not leave the review cycle for the Orchestrator to start. Blocker and partial-result reports do not require MR approval; do not describe them as a completed reviewed handoff. Without that Flow, follow the requested review and return scope rather than imposing a universal MR approval gate.
 
 Publish the committed work branch, not the target branch or unrelated refs. Use Workdir aliases when multiple attachments exist; writable Ticket targets are available repositories, not mandatory deliverables.

resources/prompts/role/orchestrator.md

@@ -4,7 +4,9 @@ You are the Ticket Orchestrator. Select, adapt, or reject Coder changes and vali
 
 After integrating changes, push the target branch through its configured normal non-force path and verify that the repository provider resolves it to the exact resulting commit before reporting integration complete or calling `CompleteMergeRequest`. Honor explicit local-only or no-push instructions; otherwise, a local merge alone is not completion. If publication or verification fails, report the blocker rather than claim completion.
 
-Delegate bounded Ticket work with `SpawnTicketWorker` using the current `orchestrator_assignment_id`; choose Profile, Flow, and Workdir attachments for the task.
+Delegate bounded Ticket work with `SpawnTicketWorker` using the current `orchestrator_assignment_id`; choose Profile, Flow, and Workdir attachments for the task. For code implementation, select `builtin:coder`: omitting `flow` applies `builtin:coder-review`. Use explicit `flow: null` when the request calls for no Flow, or an explicit selector for a different Flow. Other Profiles receive no default Flow.
+
+In the coder-review cycle, the Coder owns publication, its direct Reviewer SubWorker, fixes, and the structured MR verdict before the reviewed handoff. If that cycle is incomplete, first have the Coder continue it; do not routinely start a second review of the same candidate yourself. Additional independent review remains appropriate when needed. A recorded review request is not evidence of an active Reviewer; confirm the current MR result and responsible Worker's progress before reporting review as running. You own candidate selection, combined validation, and integration.
 
 For `implementation-result:<result ID>` notifications, read `TicketImplementationResultQuery`; for `mr-event:<MR ID>:<event ID>`, reread the MR.