From 71cd58f86864cf9269b0d5fcc7e2ab88220e37c6 Mon Sep 17 00:00:00 2001 From: Hare Date: Mon, 17 Aug 2026 22:53:09 +0900 Subject: [PATCH] feat: scope merge request tools by profile flags --- crates/manifest/src/config.rs | 103 +++++++++- crates/manifest/src/lib.rs | 24 +++ crates/manifest/src/profile.rs | 73 ++++++++ crates/worker/src/controller.rs | 15 ++ .../src/feature/builtin/merge_request.rs | 176 +++++++++++++++--- crates/worker/src/feature/builtin/ticket.rs | 28 --- crates/worker/src/prompt/catalog.rs | 33 ++++ crates/worker/src/prompt/system.rs | 43 +++++ crates/worker/src/worker.rs | 22 +++ docs/development/work-items.md | 9 +- resources/flows/coder-review.dcdl | 8 +- resources/profiles/base.dcdl | 7 + resources/profiles/coder.dcdl | 7 + resources/profiles/orchestrator.dcdl | 7 + resources/profiles/reviewer.dcdl | 7 + resources/prompts/catalog.dcdl | 2 + resources/prompts/common/merge-request.md | 19 ++ resources/prompts/role/coder.md | 4 +- resources/prompts/role/reviewer.md | 2 +- 19 files changed, 522 insertions(+), 67 deletions(-) create mode 100644 resources/prompts/common/merge-request.md diff --git a/crates/manifest/src/config.rs b/crates/manifest/src/config.rs index 8be096c7..33f2b295 100644 --- a/crates/manifest/src/config.rs +++ b/crates/manifest/src/config.rs @@ -18,9 +18,10 @@ use crate::model::{AuthRef, ModelManifest, ReasoningControl}; use crate::plugin::PluginConfig; use crate::{ CompactionConfig, EngineManifest, FeatureConfig, FeatureFlagConfig, FileUploadLimits, - McpConfig, McpEnvValue, McpStdioCwdPolicy, MemoryConfig, MemoryFeatureConfig, ScopeConfig, - SessionConfig, SkillsConfig, TicketFeatureConfig, ToolOutputLimits, ToolPermissionConfig, - ToolPermissionRule, WebConfig, WorkerFeatureConfig, WorkerManifest, WorkerMeta, + McpConfig, McpEnvValue, McpStdioCwdPolicy, MemoryConfig, MemoryFeatureConfig, + MergeRequestFeatureConfig, ScopeConfig, SessionConfig, SkillsConfig, TicketFeatureConfig, + ToolOutputLimits, ToolPermissionConfig, ToolPermissionRule, WebConfig, WorkerFeatureConfig, + WorkerManifest, WorkerMeta, }; /// Partial-form Worker manifest. Every field is optional; one or more @@ -97,6 +98,8 @@ pub struct FeatureConfigPartial { #[serde(default)] pub ticket: Option, #[serde(default)] + pub merge_request: Option, + #[serde(default)] pub orchestration: Option, #[serde(default)] pub plugins: Option, @@ -127,6 +130,11 @@ impl FeatureConfigPartial { FeatureFlagConfigPartial::merge, ), ticket: merge_option(self.ticket, other.ticket, TicketFeatureConfigPartial::merge), + merge_request: merge_option( + self.merge_request, + other.merge_request, + MergeRequestFeatureConfigPartial::merge, + ), orchestration: merge_option( self.orchestration, other.orchestration, @@ -216,6 +224,28 @@ impl TicketFeatureConfigPartial { } } +#[derive(Debug, Clone, Default, Deserialize, Serialize, PartialEq, Eq)] +#[serde(default, deny_unknown_fields)] +pub struct MergeRequestFeatureConfigPartial { + pub show: Option, + pub open: Option, + pub review: Option, + pub readiness_check: Option, + pub complete: Option, +} + +impl MergeRequestFeatureConfigPartial { + fn merge(self, other: Self) -> Self { + Self { + show: other.show.or(self.show), + open: other.open.or(self.open), + review: other.review.or(self.review), + readiness_check: other.readiness_check.or(self.readiness_check), + complete: other.complete.or(self.complete), + } + } +} + impl From for FeatureConfig { fn from(value: FeatureConfigPartial) -> Self { Self { @@ -247,6 +277,10 @@ impl From for FeatureConfig { .ticket .map(TicketFeatureConfig::from) .unwrap_or_default(), + merge_request: value + .merge_request + .map(MergeRequestFeatureConfig::from) + .unwrap_or_default(), orchestration: value .orchestration .map(FeatureFlagConfig::from) @@ -326,6 +360,30 @@ impl From for TicketFeatureConfigPartial { } } +impl From for MergeRequestFeatureConfig { + fn from(value: MergeRequestFeatureConfigPartial) -> Self { + Self { + show: value.show.unwrap_or_default(), + open: value.open.unwrap_or_default(), + review: value.review.unwrap_or_default(), + readiness_check: value.readiness_check.unwrap_or_default(), + complete: value.complete.unwrap_or_default(), + } + } +} + +impl From for MergeRequestFeatureConfigPartial { + fn from(value: MergeRequestFeatureConfig) -> Self { + Self { + show: Some(value.show), + open: Some(value.open), + review: Some(value.review), + readiness_check: Some(value.readiness_check), + complete: Some(value.complete), + } + } +} + impl From for FeatureConfigPartial { fn from(value: FeatureConfig) -> Self { Self { @@ -339,6 +397,7 @@ impl From for FeatureConfigPartial { objective: Some(value.objective.into()), manage_workdir: Some(value.manage_workdir.into()), ticket: Some(value.ticket.into()), + merge_request: Some(value.merge_request.into()), orchestration: Some(value.orchestration.into()), plugins: Some(value.plugins.into()), } @@ -1880,6 +1939,7 @@ worker_max_turns = 7 assert!(!manifest.feature.objective.enabled); assert!(!manifest.feature.manage_workdir.enabled); assert!(!manifest.feature.ticket.enabled); + assert!(!manifest.feature.merge_request.any()); } #[test] @@ -1899,6 +1959,13 @@ thread = false intake = false workflow = false +[feature.merge_request] +show = true +open = false +review = true +readiness_check = false +complete = false + [feature.orchestration] enabled = false "#, @@ -1934,6 +2001,14 @@ enabled = false assert!(!manifest.feature.ticket.thread); assert!(!manifest.feature.ticket.intake); assert!(!manifest.feature.ticket.workflow); + assert_eq!( + manifest.feature.merge_request, + MergeRequestFeatureConfig { + show: true, + review: true, + ..Default::default() + } + ); assert!(!manifest.feature.orchestration.enabled); assert!(!manifest.feature.memory.enabled); assert!(!manifest.feature.memory.staging); @@ -1957,6 +2032,13 @@ thread = false intake = false workflow = false +[feature.merge_request] +show = true +open = false +review = true +readiness_check = false +complete = false + [feature.orchestration] enabled = false "#, @@ -1968,6 +2050,11 @@ enabled = false thread = true workflow = true +[feature.merge_request] +open = true +review = false +readiness_check = true + [feature.orchestration] enabled = true @@ -2017,6 +2104,16 @@ enabled = true assert!(manifest.feature.ticket.thread); assert!(!manifest.feature.ticket.intake); assert!(manifest.feature.ticket.workflow); + assert_eq!( + manifest.feature.merge_request, + MergeRequestFeatureConfig { + show: true, + open: true, + review: false, + readiness_check: true, + complete: false, + } + ); assert!(manifest.feature.orchestration.enabled); assert!(manifest.feature.objective.enabled); assert!(manifest.feature.web.enabled); diff --git a/crates/manifest/src/lib.rs b/crates/manifest/src/lib.rs index 1f6d1f74..7c7c418c 100644 --- a/crates/manifest/src/lib.rs +++ b/crates/manifest/src/lib.rs @@ -125,6 +125,8 @@ pub struct FeatureConfig { #[serde(default)] pub ticket: TicketFeatureConfig, #[serde(default)] + pub merge_request: MergeRequestFeatureConfig, + #[serde(default)] pub orchestration: FeatureFlagConfig, #[serde(default)] pub plugins: FeatureFlagConfig, @@ -143,6 +145,7 @@ impl Default for FeatureConfig { objective: FeatureFlagConfig::disabled(), manage_workdir: FeatureFlagConfig::disabled(), ticket: TicketFeatureConfig::default(), + merge_request: MergeRequestFeatureConfig::default(), orchestration: FeatureFlagConfig::disabled(), plugins: FeatureFlagConfig::disabled(), } @@ -252,6 +255,27 @@ pub struct TicketFeatureConfig { pub workflow: bool, } +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Default)] +#[serde(deny_unknown_fields)] +pub struct MergeRequestFeatureConfig { + #[serde(default)] + pub show: bool, + #[serde(default)] + pub open: bool, + #[serde(default)] + pub review: bool, + #[serde(default)] + pub readiness_check: bool, + #[serde(default)] + pub complete: bool, +} + +impl MergeRequestFeatureConfig { + pub fn any(self) -> bool { + self.show || self.open || self.review || self.readiness_check || self.complete + } +} + /// External Agent Skills (`SKILL.md`) ingest configuration. Skills are /// loaded *only* from the directories listed here — there is no /// implicit `$config_dir/skills/` or builtin probe. Profile and Manifest diff --git a/crates/manifest/src/profile.rs b/crates/manifest/src/profile.rs index 65b8d99c..3ace3501 100644 --- a/crates/manifest/src/profile.rs +++ b/crates/manifest/src/profile.rs @@ -919,6 +919,37 @@ fn apply_role_profile( _ => serde_json::json!({ "enabled": true, "authoring": true, "thread": true }), }; value["feature"]["ticket"] = ticket; + let merge_request = match slug { + "coder" => serde_json::json!({ + "show": true, + "open": true, + "review": false, + "readiness_check": false, + "complete": false + }), + "reviewer" => serde_json::json!({ + "show": true, + "open": false, + "review": true, + "readiness_check": false, + "complete": false + }), + "orchestrator" => serde_json::json!({ + "show": true, + "open": false, + "review": false, + "readiness_check": true, + "complete": true + }), + _ => serde_json::json!({ + "show": false, + "open": false, + "review": false, + "readiness_check": false, + "complete": false + }), + }; + value["feature"]["merge_request"] = merge_request; } fn reject_manifest_shaped_profile(value: &serde_json::Value) -> Result<(), ProfileError> { @@ -1361,6 +1392,7 @@ mod tests { assert!(companion.feature.objective.enabled); assert!(!companion.feature.ticket.intake); assert!(!companion.feature.orchestration.enabled); + assert!(!companion.feature.merge_request.any()); assert_eq!( companion.compaction.as_ref().unwrap().threshold, Some(240000) @@ -1401,6 +1433,15 @@ mod tests { assert!(!orchestrator.feature.sub_worker.enabled); assert!(orchestrator.feature.worker.enabled); assert!(!orchestrator.feature.worker.direct_spawn); + assert_eq!( + orchestrator.feature.merge_request, + crate::MergeRequestFeatureConfig { + show: true, + readiness_check: true, + complete: true, + ..Default::default() + } + ); assert!(orchestrator.feature.ticket.enabled); assert!(orchestrator.feature.ticket.enabled); assert!(!orchestrator.feature.ticket.authoring); @@ -1424,6 +1465,14 @@ mod tests { assert!(coder.feature.sub_worker.enabled); assert!(coder.feature.flow.enabled); assert!(!coder.feature.worker.enabled); + assert_eq!( + coder.feature.merge_request, + crate::MergeRequestFeatureConfig { + show: true, + open: true, + ..Default::default() + } + ); assert!(coder.scope.allow.is_empty()); assert!(coder.delegation_scope.allow.is_empty()); assert_eq!(coder.model.ref_.as_deref(), Some("codex-oauth/gpt-5.5")); @@ -1442,6 +1491,14 @@ mod tests { assert!(!reviewer.feature.sub_worker.enabled); assert!(!reviewer.feature.flow.enabled); assert!(!reviewer.feature.worker.enabled); + assert_eq!( + reviewer.feature.merge_request, + crate::MergeRequestFeatureConfig { + show: true, + review: true, + ..Default::default() + } + ); assert!(reviewer.feature.ticket.enabled); assert!(reviewer.feature.ticket.enabled); assert!(!reviewer.feature.ticket.authoring); @@ -1604,6 +1661,14 @@ enabled = true authoring = false thread = false intake = false + +[feature.merge_request] +show = true +open = false +review = true +readiness_check = false +complete = false + [feature.orchestration] enabled = false "#, @@ -1626,6 +1691,14 @@ enabled = false assert!(!resolved.manifest.feature.ticket.authoring); assert!(!resolved.manifest.feature.ticket.thread); assert!(!resolved.manifest.feature.ticket.intake); + assert_eq!( + resolved.manifest.feature.merge_request, + crate::MergeRequestFeatureConfig { + show: true, + review: true, + ..Default::default() + } + ); assert!(!resolved.manifest.feature.orchestration.enabled); assert_eq!( resolved.manifest.delegation_scope.allow[0].target, diff --git a/crates/worker/src/controller.rs b/crates/worker/src/controller.rs index ebb07952..e952a46b 100644 --- a/crates/worker/src/controller.rs +++ b/crates/worker/src/controller.rs @@ -769,6 +769,21 @@ where ), ); } + if feature_config.merge_request.any() { + let workspace_client = worker.workspace_client_handle(); + if !workspace_client.is_available() || workspace_client.workspace_id().is_none() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "Merge Request tools require Backend Workspace API authority", + )); + } + feature_registry.add_module( + crate::feature::builtin::merge_request::MergeRequestFeature::new( + workspace_client, + feature_config.merge_request, + ), + ); + } if feature_config.manage_workdir.enabled { // Workdir lifecycle is Workspace control-plane authority. The Worker // receives only the injected WorkspaceClient and never Runtime URLs, diff --git a/crates/worker/src/feature/builtin/merge_request.rs b/crates/worker/src/feature/builtin/merge_request.rs index b2dd5c57..832789d2 100644 --- a/crates/worker/src/feature/builtin/merge_request.rs +++ b/crates/worker/src/feature/builtin/merge_request.rs @@ -1,19 +1,41 @@ -use crate::feature::ToolDefinition; +use crate::feature::{ + FeatureDescriptor, FeatureInstallContext, FeatureInstallError, FeatureInstructionContribution, + FeatureInstructionDeclaration, FeatureInstructionId, FeatureModule, ToolContribution, + ToolDeclaration, ToolDefinition, +}; use crate::worker::{WorkspaceClient, WorkspaceRequest, WorkspaceRequestMethod}; use async_trait::async_trait; use llm_engine::tool::{Tool, ToolError, ToolExecutionContext, ToolMeta, ToolOutput}; +use manifest::MergeRequestFeatureConfig; use schemars::JsonSchema; use serde::Deserialize; use serde_json::json; use std::sync::Arc; -pub const MERGE_REQUEST_COMMON_TOOL_NAMES: &[&str] = &[ - "MergeRequestShow", - "MergeRequestReadinessCheck", - "MergeRequestOpen", - "MergeRequestComplete", +pub const FEATURE_ID: &str = "merge_request"; +const FEATURE_NAME: &str = "Merge Request tools"; +const FEATURE_DESCRIPTION: &str = + "Operation-specific Merge Request workflow tools over Workspace authority."; +const FEATURE_INSTRUCTION_ID: &str = "merge_request.workflow"; +pub const FEATURE_PROMPT_REF: &str = "common.merge_request"; + +fn workflow_instruction() -> FeatureInstructionDeclaration { + FeatureInstructionDeclaration::new( + FeatureInstructionId::builtin(FEATURE_INSTRUCTION_ID), + FEATURE_PROMPT_REF, + "Operation-specific Merge Request workflow guidance", + ) + .expect("static Merge Request workflow instruction declaration is valid") +} + +const ALL_KINDS: [Kind; 5] = [ + Kind::Show, + Kind::Open, + Kind::Review, + Kind::Readiness, + Kind::Complete, ]; -pub const MERGE_REQUEST_REVIEW_TOOL_NAME: &str = "MergeRequestReviewSubmit"; + #[derive(Clone, Copy)] enum Kind { Show, @@ -89,13 +111,23 @@ struct ReviewFindingInput { body: String, } impl Kind { + fn enabled(self, config: MergeRequestFeatureConfig) -> bool { + match self { + Self::Show => config.show, + Self::Open => config.open, + Self::Review => config.review, + Self::Readiness => config.readiness_check, + Self::Complete => config.complete, + } + } + fn name(self) -> &'static str { match self { Self::Show => "MergeRequestShow", Self::Readiness => "MergeRequestReadinessCheck", Self::Open => "MergeRequestOpen", Self::Complete => "MergeRequestComplete", - Self::Review => "MergeRequestReviewSubmit", + Self::Review => "MergeRequestReview", } } fn schema(self) -> serde_json::Value { @@ -218,24 +250,53 @@ fn definition(client: Arc, kind: Kind) -> ToolDefinition { ) }) } -pub fn common_tools(c: Arc) -> Vec { - vec![ - definition(c.clone(), Kind::Show), - definition(c.clone(), Kind::Readiness), - definition(c.clone(), Kind::Open), - definition(c, Kind::Complete), - ] +pub struct MergeRequestFeature { + client: Arc, + config: MergeRequestFeatureConfig, } -pub fn reviewer_tools(c: Arc) -> Vec { - if c.reviewer_context().is_some() { - vec![ - definition(c.clone(), Kind::Show), - definition(c, Kind::Review), - ] - } else { - vec![] + +impl MergeRequestFeature { + pub fn new(client: Arc, config: MergeRequestFeatureConfig) -> Self { + Self { client, config } + } + + fn kinds(&self) -> impl Iterator + '_ { + ALL_KINDS + .into_iter() + .filter(|kind| kind.enabled(self.config)) } } + +impl FeatureModule for MergeRequestFeature { + fn descriptor(&self) -> FeatureDescriptor { + let mut descriptor = FeatureDescriptor::builtin(FEATURE_ID, FEATURE_NAME) + .with_description(FEATURE_DESCRIPTION); + if self.config.any() { + descriptor = descriptor.with_instruction(workflow_instruction()); + } + for kind in self.kinds() { + descriptor = descriptor.with_tool(ToolDeclaration::new( + kind.name(), + description(kind.name()).unwrap_or("Merge Request operation."), + )); + } + descriptor + } + + fn install(&self, ctx: &mut FeatureInstallContext<'_>) -> Result<(), FeatureInstallError> { + if self.config.any() { + ctx.instructions() + .register(FeatureInstructionContribution::new(workflow_instruction()))?; + } + let mut tools = ctx.tools(); + for kind in self.kinds() { + let definition = definition(self.client.clone(), kind); + tools.register(ToolContribution::new(kind.name(), definition))?; + } + Ok(()) + } +} + pub fn description(n: &str) -> Option<&'static str> { match n { "MergeRequestShow" => Some("Read the selector-based Merge Request and append-only thread."), @@ -248,7 +309,7 @@ pub fn description(n: &str) -> Option<&'static str> { "MergeRequestComplete" => { Some("Complete using an approved review event and final target-ref evidence.") } - "MergeRequestReviewSubmit" => { + "MergeRequestReview" => { Some("Submit the injected Reviewer capability result for its captured subject ref.") } _ => None, @@ -257,6 +318,72 @@ pub fn description(n: &str) -> Option<&'static str> { #[cfg(test)] mod tests { use super::*; + use crate::feature::FeatureRegistryBuilder; + use crate::hook::HookRegistryBuilder; + use crate::worker::TestWorkspaceHttpClient; + + fn install(config: MergeRequestFeatureConfig) -> (Vec, Vec) { + let client: Arc = + Arc::new(TestWorkspaceHttpClient::new("workspace", "http://unused")); + let mut pending_tools = Vec::new(); + let mut hook_builder = HookRegistryBuilder::default(); + let report = FeatureRegistryBuilder::new() + .with_module(MergeRequestFeature::new(client, config)) + .install_into_pending(&mut pending_tools, &mut hook_builder); + assert!(!report.has_errors(), "{}", report.error_message()); + ( + report.installed_tool_names(), + report + .installed_instruction_contributions() + .into_iter() + .map(|instruction| instruction.prompt_ref) + .collect(), + ) + } + + fn tool_names(config: MergeRequestFeatureConfig) -> Vec { + install(config).0 + } + + #[test] + fn flags_define_the_exact_registered_tool_surface() { + let coder = MergeRequestFeatureConfig { + show: true, + open: true, + ..Default::default() + }; + assert_eq!(tool_names(coder), ["MergeRequestShow", "MergeRequestOpen"]); + + let reviewer = MergeRequestFeatureConfig { + show: true, + review: true, + ..Default::default() + }; + assert_eq!( + tool_names(reviewer), + ["MergeRequestShow", "MergeRequestReview"] + ); + + let orchestrator = MergeRequestFeatureConfig { + show: true, + readiness_check: true, + complete: true, + ..Default::default() + }; + assert_eq!( + tool_names(orchestrator), + [ + "MergeRequestShow", + "MergeRequestReadinessCheck", + "MergeRequestComplete" + ] + ); + assert_eq!(install(coder).1, [FEATURE_PROMPT_REF]); + let unspecified = install(MergeRequestFeatureConfig::default()); + assert!(unspecified.0.is_empty()); + assert!(unspecified.1.is_empty()); + } + #[test] fn schemas_hide_revision_and_commit_authority() { let schemas = [ @@ -276,6 +403,5 @@ mod tests { assert!(!j.contains(banned), "{banned} in {j}") } } - assert!(!MERGE_REQUEST_COMMON_TOOL_NAMES.contains(&"MergeRequestRequestReview")); } } diff --git a/crates/worker/src/feature/builtin/ticket.rs b/crates/worker/src/feature/builtin/ticket.rs index ac461c13..cb2fb57c 100644 --- a/crates/worker/src/feature/builtin/ticket.rs +++ b/crates/worker/src/feature/builtin/ticket.rs @@ -24,7 +24,6 @@ use ticket::{ tool::{TICKET_TOOL_NAMES, TicketToolBackend, ticket_tool_description, ticket_tools}, }; -use super::merge_request; use crate::feature::{ FeatureDescriptor, FeatureDiagnostic, FeatureInstallContext, FeatureInstallError, FeatureInstructionContribution, FeatureInstructionDeclaration, FeatureInstructionId, @@ -588,22 +587,6 @@ impl FeatureModule for TicketFeature { ticket_tool_description(name, self.record_language.as_deref()), )); } - if let TicketFeatureBackend::WorkspaceClient(client) = &self.backend { - let names: Vec<&str> = if client.reviewer_context().is_some() { - vec![ - "MergeRequestShow", - merge_request::MERGE_REQUEST_REVIEW_TOOL_NAME, - ] - } else { - merge_request::MERGE_REQUEST_COMMON_TOOL_NAMES.to_vec() - }; - for name in names { - descriptor = descriptor.with_tool(ToolDeclaration::new( - name, - merge_request::description(name).unwrap_or("Merge Request operation."), - )); - } - } descriptor } @@ -661,17 +644,6 @@ impl FeatureModule for TicketFeature { }; tools.register(ToolContribution::new(name, definition))?; } - if let TicketFeatureBackend::WorkspaceClient(client) = &self.backend { - let definitions = if client.reviewer_context().is_some() { - merge_request::reviewer_tools(client.clone()) - } else { - merge_request::common_tools(client.clone()) - }; - for definition in definitions { - let (meta, _) = definition(); - tools.register(ToolContribution::new(meta.name.clone(), definition))?; - } - } Ok(()) } } diff --git a/crates/worker/src/prompt/catalog.rs b/crates/worker/src/prompt/catalog.rs index 82518b90..53981905 100644 --- a/crates/worker/src/prompt/catalog.rs +++ b/crates/worker/src/prompt/catalog.rs @@ -527,6 +527,39 @@ mod tests { ); } + #[test] + fn merge_request_instruction_matches_the_exposed_operations() { + let catalog = PromptCatalog::builtins_only().unwrap(); + let coder = catalog + .render_name( + "common.merge_request", + Value::from_serialize(serde_json::json!({ + "tools": ["MergeRequestShow", "MergeRequestOpen"] + })), + ) + .unwrap(); + assert!(coder.contains("Reread the current Merge Request")); + assert!(coder.contains("Open the Merge Request only after")); + assert!(!coder.contains("Submit the authoritative verdict")); + assert!(!coder.contains("Complete integration only after")); + + let orchestrator = catalog + .render_name( + "common.merge_request", + Value::from_serialize(serde_json::json!({ + "tools": [ + "MergeRequestShow", + "MergeRequestReadinessCheck", + "MergeRequestComplete" + ] + })), + ) + .unwrap(); + assert!(orchestrator.contains("Use `MergeRequestReadinessCheck`")); + assert!(orchestrator.contains("Complete integration only after")); + assert!(!orchestrator.contains("Open the Merge Request only after")); + } + #[test] fn commit_capable_roles_classify_commits_by_change_type() { let catalog = PromptCatalog::builtins_only().unwrap(); diff --git a/crates/worker/src/prompt/system.rs b/crates/worker/src/prompt/system.rs index d043b703..32f291bd 100644 --- a/crates/worker/src/prompt/system.rs +++ b/crates/worker/src/prompt/system.rs @@ -319,6 +319,7 @@ fn append_trailing_section( #[cfg(test)] mod tests { use super::*; + use crate::feature::FeatureInstructionId; use chrono::TimeZone; use manifest::{Permission, ScopeConfig, ScopeRule}; use tempfile::TempDir; @@ -400,6 +401,48 @@ mod tests { } } + #[test] + fn merge_request_role_prompts_match_operation_specific_tool_surfaces() { + fn render(role: &str, tools: &[&str]) -> String { + let tmp = TempDir::new().unwrap(); + let scope = build_scope(tmp.path()); + let prompts = PromptCatalog::builtins_only().unwrap(); + let template = + SystemPromptTemplate::parse(role, PromptCatalogSource::builtins_only()).unwrap(); + let instruction = FeatureInstructionDeclaration::new( + FeatureInstructionId::builtin("merge_request.workflow"), + "common.merge_request", + "Merge Request workflow", + ) + .unwrap(); + let mut ctx = context(tmp.path(), &scope, &prompts); + ctx.tool_names = tools.iter().map(|name| (*name).to_string()).collect(); + ctx.feature_instructions = std::slice::from_ref(&instruction); + template.render(&ctx).unwrap() + } + + let coder = render("role.coder", &["MergeRequestShow", "MergeRequestOpen"]); + assert!(coder.contains("Open the Merge Request only after")); + assert!(!coder.contains("Complete integration only after")); + assert!(coder.contains("Do not call `MergeRequestComplete`")); + + let reviewer = render("role.reviewer", &["MergeRequestShow", "MergeRequestReview"]); + assert!(reviewer.contains("Submit the authoritative verdict")); + assert!(!reviewer.contains("Open the Merge Request only after")); + + let orchestrator = render( + "role.orchestrator", + &[ + "MergeRequestShow", + "MergeRequestReadinessCheck", + "MergeRequestComplete", + ], + ); + assert!(orchestrator.contains("Use `MergeRequestReadinessCheck`")); + assert!(orchestrator.contains("Complete integration only after")); + assert!(!orchestrator.contains("Submit the authoritative verdict")); + } + #[test] fn role_templates_are_selected_without_filesystem_resolution() { let loader = PromptCatalogSource::builtins_only(); diff --git a/crates/worker/src/worker.rs b/crates/worker/src/worker.rs index 40a0ec7e..2830014f 100644 --- a/crates/worker/src/worker.rs +++ b/crates/worker/src/worker.rs @@ -5890,6 +5890,13 @@ model_id = "claude-sonnet-4-20250514" [engine] instruction = "saved" +[feature.merge_request] +show = true +open = false +review = true +readiness_check = false +complete = false + [[scope.allow]] target = "/snapshot/workspace" permission = "read" @@ -5912,6 +5919,13 @@ model_id = "claude-sonnet-4-20250514" [engine] instruction = "current" +[feature.merge_request] +show = true +open = true +review = true +readiness_check = true +complete = true + [[scope.allow]] target = "/current/workspace" permission = "write" @@ -5931,6 +5945,14 @@ permission = "write" .unwrap(); assert_eq!(restored.engine.instruction, "saved"); + assert_eq!( + restored.feature.merge_request, + manifest::MergeRequestFeatureConfig { + show: true, + review: true, + ..Default::default() + } + ); assert_eq!(restored.scope.allow.len(), 1); assert_eq!( restored.scope.allow[0].target, diff --git a/docs/development/work-items.md b/docs/development/work-items.md index 8e096fea..ebd29c50 100644 --- a/docs/development/work-items.md +++ b/docs/development/work-items.md @@ -31,14 +31,15 @@ Maintainers can inspect the local `.yoi/tickets/` files directly when debugging ## Ticket tools inside Workers -Workers with the Ticket built-in feature can use typed Ticket tools: +Workers with the Ticket and operation-specific Merge Request built-in features can use typed workflow tools: - `TicketCreate` - `QueryTicket` — bounded authoritative Ticket discovery with typed state/text/event/evidence/relation/Objective/time/attention filters, stable snippets, and cursor metadata. - `ShowTicket` — detailed authority for one Ticket, including item revision, bounded thread/event references, relations, linked Objectives, implementation reports, and current Merge Request/review evidence. - `TicketComment` -- `MergeRequestShow`, `MergeRequestOpen`, `MergeRequestAddRevision`, `MergeRequestComplete` -- `MergeRequestReviewSubmit` — available only inside the attested direct-child Reviewer attempt; attempt/revision capability material is not model input. +- Coder: `MergeRequestShow`, `MergeRequestOpen` +- Reviewer: `MergeRequestShow`, `MergeRequestReview` — available only inside the attested direct-child Reviewer request; grant and subject-ref capability material are not model input. +- Orchestrator: `MergeRequestShow`, `MergeRequestReadinessCheck`, `MergeRequestComplete` - `TicketClose` - `TicketRelationRecord` @@ -243,7 +244,7 @@ Implementation normally happens in a child git worktree created by the Orchestra 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 Reviewer records the structured result with `MergeRequestReviewSubmit`. Request changes requires a new immutable revision and a fresh child attempt. `MergeRequestComplete` performs guarded Ticket completion with operation-id dedupe/CAS semantics; Flow transitions are not completion authority. +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. Blockers must be fixed or explicitly escalated before merge-ready submission. diff --git a/resources/flows/coder-review.dcdl b/resources/flows/coder-review.dcdl index c191cd1b..c01b1d1d 100644 --- a/resources/flows/coder-review.dcdl +++ b/resources/flows/coder-review.dcdl @@ -15,7 +15,7 @@ }; review = { - instructions = "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 child must commit MergeRequestReviewSubmit; prose output and Worker observation are not approval authority. After the structured current-revision result exists, request a Flow transition."; + instructions = "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 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"; @@ -39,17 +39,17 @@ }; complete = { - instructions = "Call MergeRequestComplete with a fresh operation_id and the approved current revision. The Server must revalidate current assignment, immutable revision, registered Reviewer attempt, and Ticket inprogress CAS. Only after the authoritative operation returns Ticket state done, request a Flow transition."; + instructions = "Leave concise implementation and validation evidence on the Ticket, then hand off the exact approved Merge Request revision to the Orchestrator for readiness and integration. Do not call MergeRequestComplete; Coder approval handoff is not Ticket completion authority. After durable handoff evidence exists, request a Flow transition."; transitions = { completed = { target = "done"; - condition = "MergeRequestComplete durably returned done for this exact operation_id and current approved revision. A Flow state or prose report alone is never sufficient."; + condition = "The exact approved Merge Request revision and implementation evidence have been durably handed off to the Orchestrator. A Flow state or prose report alone is never Ticket completion authority."; }; }; }; done = { - instructions = "The guarded Merge Request completion operation committed Ticket state done. Flow terminal state only reflects that durable authority."; + instructions = "The approved implementation has been handed off for Orchestrator-owned readiness and integration. Flow terminal state only reflects that handoff."; terminal = true; }; }; diff --git a/resources/profiles/base.dcdl b/resources/profiles/base.dcdl index 04148c7b..2e960d15 100644 --- a/resources/profiles/base.dcdl +++ b/resources/profiles/base.dcdl @@ -30,6 +30,13 @@ feature = { worker = { enabled = false; }; objective = { enabled = true; }; ticket = { enabled = true; authoring = true; thread = true; }; + merge_request = { + show = false; + open = false; + review = false; + readiness_check = false; + complete = false; + }; }; memory = { diff --git a/resources/profiles/coder.dcdl b/resources/profiles/coder.dcdl index 27a85776..93713b55 100644 --- a/resources/profiles/coder.dcdl +++ b/resources/profiles/coder.dcdl @@ -12,5 +12,12 @@ import "./base.dcdl" // { flow = { enabled = true; }; worker = { enabled = true; }; ticket = { enabled = true; thread = true; }; + merge_request = { + show = true; + open = true; + review = false; + readiness_check = false; + complete = false; + }; }; } diff --git a/resources/profiles/orchestrator.dcdl b/resources/profiles/orchestrator.dcdl index feb7f402..cb654b63 100644 --- a/resources/profiles/orchestrator.dcdl +++ b/resources/profiles/orchestrator.dcdl @@ -12,6 +12,13 @@ import "./base.dcdl" // { worker = { enabled = true; direct_spawn = false; }; manage_workdir = { enabled = true; }; ticket = { enabled = true; thread = true; workflow = true; }; + merge_request = { + show = true; + open = false; + review = false; + readiness_check = true; + complete = true; + }; orchestration = { enabled = true; }; }; } diff --git a/resources/profiles/reviewer.dcdl b/resources/profiles/reviewer.dcdl index 82270494..b41f9627 100644 --- a/resources/profiles/reviewer.dcdl +++ b/resources/profiles/reviewer.dcdl @@ -11,5 +11,12 @@ import "./base.dcdl" // { sub_worker = { enabled = false; }; worker = { enabled = false; }; ticket = { enabled = true; thread = false; }; + merge_request = { + show = true; + open = false; + review = true; + readiness_check = false; + complete = false; + }; }; } diff --git a/resources/prompts/catalog.dcdl b/resources/prompts/catalog.dcdl index 84dd4b2f..1144f3a3 100644 --- a/resources/prompts/catalog.dcdl +++ b/resources/prompts/catalog.dcdl @@ -6,6 +6,7 @@ let defaultDocument = import "./default.md"; commonLanguage = import "./common/language.md"; commonGit = import "./common/git.md"; +commonMergeRequest = import "./common/merge-request.md"; commonTickets = import "./common/tickets.md"; commonToolUsage = import "./common/tool-usage.md"; commonWorkerObservation = import "./common/worker-observation.md"; @@ -36,6 +37,7 @@ in common = { git = commonGit.content; language = commonLanguage.content; + merge_request = commonMergeRequest.content; tickets = commonTickets.content; tool_usage = commonToolUsage.content; worker_observation = commonWorkerObservation.content; diff --git a/resources/prompts/common/merge-request.md b/resources/prompts/common/merge-request.md new file mode 100644 index 00000000..4e7a8996 --- /dev/null +++ b/resources/prompts/common/merge-request.md @@ -0,0 +1,19 @@ +## Merge Request workflow + +Use only the exposed Merge Request operations; their availability expresses this Worker's workflow responsibility, not authorization to bypass Backend validation. +{% if "MergeRequestShow" in tools %} +- Reread the current Merge Request and append-only thread with `MergeRequestShow` before making review or integration decisions. +{% endif %} +{% if "MergeRequestOpen" in tools %} +- Open the Merge Request only after all intended changes are committed and the Workdir is clean. Use immutable source and target selectors; do not infer target authority from a branch name or cwd. +- Before requesting independent review, make the exact current MR revision authoritative. +{% endif %} +{% if "MergeRequestReview" in tools %} +- Review the exact current immutable MR revision independently. Submit the authoritative verdict through `MergeRequestReview`; prose alone is not approval. +{% endif %} +{% if "MergeRequestReadinessCheck" in tools %} +- Use `MergeRequestReadinessCheck` to resolve current refs and authoritative review readiness before integration. +{% endif %} +{% if "MergeRequestComplete" in tools %} +- Complete integration only after readiness confirms approval for the exact current revision and all target/ref guards pass. Merge completion is separate from implementation and review evidence. +{% endif %} diff --git a/resources/prompts/role/coder.md b/resources/prompts/role/coder.md index adfa0e54..29932418 100644 --- a/resources/prompts/role/coder.md +++ b/resources/prompts/role/coder.md @@ -4,6 +4,6 @@ Treat the first committed user message as the bounded Ticket/action context and {% 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 `MergeRequestReviewSubmit` 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 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. -A request-changes result requires a fresh Reviewer child request. Flow terminal state is not Ticket completion authority. Complete only through `MergeRequestComplete` with a unique operation id, the approved `Review` event id, and final target-ref evidence; the Server re-resolves selectors, revalidates assignment, and fences Ticket state side effects. +A request-changes result requires a fresh Reviewer child request. Flow terminal state is not Ticket completion authority. After the exact current Merge Request revision has authoritative approval, leave concise implementation evidence on the Ticket and hand off integration to the Orchestrator. Do not call `MergeRequestComplete`. diff --git a/resources/prompts/role/reviewer.md b/resources/prompts/role/reviewer.md index 112c412c..8f2c05cc 100644 --- a/resources/prompts/role/reviewer.md +++ b/resources/prompts/role/reviewer.md @@ -2,6 +2,6 @@ You are the Ticket Reviewer role running as an actual Runtime-owned direct child Keep role behavior here and treat the first committed user message as bounded Ticket/Merge Request context only. 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, or take over implementation. -Your prose response is not review authority. Before finishing, call `MergeRequestReviewSubmit` 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. +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. Review more than the diff: verify the implementation satisfies the Ticket intent and acceptance criteria, remains coherent with the codebase design, and does not introduce unnecessary compatibility.