From 9e48cae75984471ce50a3656ce0b16e09fc43f08 Mon Sep 17 00:00:00 2001 From: Hare Date: Mon, 17 Aug 2026 07:09:20 +0900 Subject: [PATCH] feat: expose selector based merge request threads --- .../src/feature/builtin/merge_request.rs | 202 +++--- crates/worker/src/feature/builtin/ticket.rs | 4 +- crates/worker/src/spawn/tool.rs | 43 +- crates/worker/src/worker.rs | 22 +- crates/workspace-server/src/server.rs | 621 +++++++++--------- resources/prompts/common/git.md | 2 +- resources/prompts/role/coder.md | 4 +- resources/prompts/role/orchestrator.md | 2 +- resources/prompts/role/reviewer.md | 4 +- .../tickets/[ticketId]/+page.svelte | 95 ++- 10 files changed, 547 insertions(+), 452 deletions(-) diff --git a/crates/worker/src/feature/builtin/merge_request.rs b/crates/worker/src/feature/builtin/merge_request.rs index 78dbd339..56a60bb0 100644 --- a/crates/worker/src/feature/builtin/merge_request.rs +++ b/crates/worker/src/feature/builtin/merge_request.rs @@ -11,19 +11,21 @@ pub const MERGE_REQUEST_COMMON_TOOL_NAMES: &[&str] = &[ "MergeRequestShow", "MergeRequestReadinessCheck", "MergeRequestOpen", - "MergeRequestAddRevision", + "MergeRequestRequestReview", "MergeRequestComplete", ]; pub const MERGE_REQUEST_REVIEW_TOOL_NAME: &str = "MergeRequestReviewSubmit"; + #[derive(Clone, Copy)] enum Kind { Show, Readiness, Open, - AddRevision, + RequestReview, Complete, Review, } + #[derive(Clone)] struct MergeRequestTool { client: Arc, @@ -34,11 +36,13 @@ struct MergeRequestTool { struct ShowInput { ticket: String, } + #[derive(Debug, Deserialize, JsonSchema)] struct OpenInput { ticket: String, repository_id: String, - revision_id: String, + selector_from: String, + selector_to: String, base_commit: String, head_commit: String, #[serde(default)] @@ -46,11 +50,11 @@ struct OpenInput { #[serde(default)] summary: String, } + #[derive(Debug, Deserialize, JsonSchema)] -struct AddRevisionInput { +struct RequestReviewInput { ticket: String, - expected_current_revision_id: String, - revision_id: String, + expected_head_commit: String, base_commit: String, head_commit: String, #[serde(default)] @@ -58,23 +62,26 @@ struct AddRevisionInput { #[serde(default)] summary: String, } + #[derive(Debug, Deserialize, JsonSchema)] struct CompleteInput { ticket: String, operation_id: String, - expected_revision_id: String, + expected_head_commit: String, target_commit: String, source_commit: String, result_commit: String, strategy: MergeStrategyInput, resolution: MergeResolutionInput, } + #[derive(Debug, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] enum MergeStrategyInput { FastForward, Merge, } + #[derive(Debug, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] enum MergeResolutionInput { @@ -82,6 +89,7 @@ enum MergeResolutionInput { Clean, ConflictsResolved, } + #[derive(Debug, Deserialize, JsonSchema)] struct ReviewInput { decision: ReviewDecisionInput, @@ -90,22 +98,22 @@ struct ReviewInput { #[serde(default)] findings: Vec, } + #[derive(Debug, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] enum ReviewDecisionInput { Approve, RequestChanges, } + #[derive(Debug, Deserialize, JsonSchema)] struct ReviewFindingInput { severity: String, #[serde(default)] - code: Option, - #[serde(default)] path: Option, #[serde(default)] - line: Option, - body: String, + line: Option, + message: String, } impl Kind { @@ -114,19 +122,21 @@ impl Kind { Self::Show => "MergeRequestShow", Self::Readiness => "MergeRequestReadinessCheck", Self::Open => "MergeRequestOpen", - Self::AddRevision => "MergeRequestAddRevision", + Self::RequestReview => "MergeRequestRequestReview", Self::Complete => "MergeRequestComplete", Self::Review => "MergeRequestReviewSubmit", } } + fn description(self) -> &'static str { description(self.name()).unwrap_or("Merge Request operation.") } + fn schema(self) -> serde_json::Value { match self { Self::Show | Self::Readiness => json!(schemars::schema_for!(ShowInput)), Self::Open => json!(schemars::schema_for!(OpenInput)), - Self::AddRevision => json!(schemars::schema_for!(AddRevisionInput)), + Self::RequestReview => json!(schemars::schema_for!(RequestReviewInput)), Self::Complete => json!(schemars::schema_for!(CompleteInput)), Self::Review => json!(schemars::schema_for!(ReviewInput)), } @@ -145,59 +155,75 @@ impl Tool for MergeRequestTool { })?; let (method, path, body) = match self.kind { Kind::Show => { - let v: ShowInput = parse(input)?; - nonempty(&v.ticket)?; + let value: ShowInput = parse(input)?; + nonempty(&value.ticket)?; ( WorkspaceRequestMethod::Get, - format!("/api/w/{workspace_id}/tickets/{}/merge-request", v.ticket), + format!( + "/api/w/{workspace_id}/tickets/{}/merge-request", + value.ticket + ), None, ) } Kind::Readiness => { - let v: ShowInput = parse(input)?; - nonempty(&v.ticket)?; + let value: ShowInput = parse(input)?; + nonempty(&value.ticket)?; ( WorkspaceRequestMethod::Get, format!( "/api/w/{workspace_id}/tickets/{}/merge-request/readiness", - v.ticket + value.ticket ), None, ) } Kind::Open => { - let v: OpenInput = parse(input)?; - nonempty(&v.ticket)?; - ( - WorkspaceRequestMethod::Post, - format!("/api/w/{workspace_id}/tickets/{}/merge-request", v.ticket), - Some( - json!({"repository_id":v.repository_id,"revision_id":v.revision_id,"base_commit":v.base_commit,"head_commit":v.head_commit,"changed_paths":v.changed_paths,"summary":v.summary}), - ), - ) - } - Kind::AddRevision => { - let v: AddRevisionInput = parse(input)?; - nonempty(&v.ticket)?; + let value: OpenInput = parse(input)?; + nonempty(&value.ticket)?; ( WorkspaceRequestMethod::Post, format!( - "/api/w/{workspace_id}/tickets/{}/merge-request/revisions", - v.ticket + "/api/w/{workspace_id}/tickets/{}/merge-request", + value.ticket ), - Some( - json!({"expected_current_revision_id":v.expected_current_revision_id,"revision_id":v.revision_id,"base_commit":v.base_commit,"head_commit":v.head_commit,"changed_paths":v.changed_paths,"summary":v.summary}), + Some(json!({ + "repository_id": value.repository_id, + "selector_from": value.selector_from, + "selector_to": value.selector_to, + "base_commit": value.base_commit, + "head_commit": value.head_commit, + "changed_paths": value.changed_paths, + "summary": value.summary, + })), + ) + } + Kind::RequestReview => { + let value: RequestReviewInput = parse(input)?; + nonempty(&value.ticket)?; + ( + WorkspaceRequestMethod::Post, + format!( + "/api/w/{workspace_id}/tickets/{}/merge-request/review-requests", + value.ticket ), + Some(json!({ + "expected_head_commit": value.expected_head_commit, + "base_commit": value.base_commit, + "head_commit": value.head_commit, + "changed_paths": value.changed_paths, + "summary": value.summary, + })), ) } Kind::Complete => { - let v: CompleteInput = parse(input)?; - nonempty(&v.ticket)?; - let strategy = match v.strategy { + let value: CompleteInput = parse(input)?; + nonempty(&value.ticket)?; + let strategy = match value.strategy { MergeStrategyInput::FastForward => "fast_forward", MergeStrategyInput::Merge => "merge", }; - let resolution = match v.resolution { + let resolution = match value.resolution { MergeResolutionInput::None => "none", MergeResolutionInput::Clean => "clean", MergeResolutionInput::ConflictsResolved => "conflicts_resolved", @@ -206,16 +232,22 @@ impl Tool for MergeRequestTool { WorkspaceRequestMethod::Post, format!( "/api/w/{workspace_id}/tickets/{}/merge-request/complete", - v.ticket - ), - Some( - json!({"operation_id":v.operation_id,"expected_revision_id":v.expected_revision_id,"target_commit":v.target_commit,"source_commit":v.source_commit,"result_commit":v.result_commit,"strategy":strategy,"resolution":resolution}), + value.ticket ), + Some(json!({ + "operation_id": value.operation_id, + "expected_head_commit": value.expected_head_commit, + "target_commit": value.target_commit, + "source_commit": value.source_commit, + "result_commit": value.result_commit, + "strategy": strategy, + "resolution": resolution, + })), ) } Kind::Review => { - let v: ReviewInput = parse(input)?; - let context = self.client.reviewer_attempt_context().ok_or_else(|| { + let value: ReviewInput = parse(input)?; + let context = self.client.reviewer_context().ok_or_else(|| { ToolError::ExecutionFailed( "MergeRequestReviewSubmit is available only to an attested Reviewer child" .into(), @@ -227,9 +259,19 @@ impl Tool for MergeRequestTool { "/api/w/{workspace_id}/tickets/{}/merge-request/reviews", context.ticket_id ), - Some( - json!({"decision":match v.decision{ReviewDecisionInput::Approve=>"approve",ReviewDecisionInput::RequestChanges=>"request_changes"},"body":v.body,"findings":v.findings.into_iter().map(|f|json!({"severity":f.severity,"code":f.code,"path":f.path,"line":f.line,"body":f.body})).collect::>() }), - ), + Some(json!({ + "decision": match value.decision { + ReviewDecisionInput::Approve => "approve", + ReviewDecisionInput::RequestChanges => "request_changes", + }, + "body": value.body, + "findings": value.findings.into_iter().map(|finding| json!({ + "severity": finding.severity, + "path": finding.path, + "line": finding.line, + "message": finding.message, + })).collect::>(), + })), ) } }; @@ -240,7 +282,7 @@ impl Tool for MergeRequestTool { let response = self .client .execute(request) - .map_err(|e| ToolError::ExecutionFailed(e.to_string()))?; + .map_err(|error| ToolError::ExecutionFailed(error.to_string()))?; if !response.is_success() { return Err(ToolError::ExecutionFailed(format!( "Merge Request API returned HTTP {}: {}", @@ -254,9 +296,11 @@ impl Tool for MergeRequestTool { }) } } + fn parse(value: &str) -> Result { - serde_json::from_str(value).map_err(|e| ToolError::InvalidArgument(e.to_string())) + serde_json::from_str(value).map_err(|error| ToolError::InvalidArgument(error.to_string())) } + fn nonempty(value: &str) -> Result<(), ToolError> { if value.trim().is_empty() { Err(ToolError::InvalidArgument( @@ -266,6 +310,7 @@ fn nonempty(value: &str) -> Result<(), ToolError> { Ok(()) } } + fn definition(client: Arc, kind: Kind) -> ToolDefinition { Arc::new(move || { let meta = ToolMeta::new(kind.name()) @@ -278,17 +323,19 @@ fn definition(client: Arc, kind: Kind) -> ToolDefinition { (meta, tool) }) } + pub fn common_tools(client: Arc) -> Vec { vec![ definition(client.clone(), Kind::Show), definition(client.clone(), Kind::Readiness), definition(client.clone(), Kind::Open), - definition(client.clone(), Kind::AddRevision), + definition(client.clone(), Kind::RequestReview), definition(client, Kind::Complete), ] } + pub fn reviewer_tools(client: Arc) -> Vec { - if client.reviewer_attempt_context().is_some() { + if client.reviewer_context().is_some() { vec![ definition(client.clone(), Kind::Show), definition(client, Kind::Review), @@ -297,26 +344,27 @@ pub fn reviewer_tools(client: Arc) -> Vec { Vec::new() } } + pub fn description(name: &str) -> Option<&'static str> { match name { "MergeRequestShow" => Some( - "Read the authoritative Merge Request, immutable current revision, and structured review status.", + "Read the authoritative Merge Request, selector pair, append-only thread, and current review status.", ), "MergeRequestReadinessCheck" => { - Some("Check derived merge readiness for the current immutable revision.") + Some("Check derived merge readiness for the current review request.") } - "MergeRequestOpen" => { - Some("Open an immutable Merge Request revision for the current assigned Coder.") - } - "MergeRequestAddRevision" => { - Some("Append an immutable revision; prior approval cannot carry to the new revision.") - } - "MergeRequestComplete" => { - Some("CAS-complete an approved revision with operation-id replay and crash fencing.") - } - "MergeRequestReviewSubmit" => Some( - "Submit the attested direct-child Reviewer result bound to its immutable revision.", + "MergeRequestOpen" => Some( + "Open a Merge Request with immutable source/target selectors and its first review request.", ), + "MergeRequestRequestReview" => Some( + "Append a RequestForReview event for new candidate evidence; prior approval cannot carry forward.", + ), + "MergeRequestComplete" => Some( + "CAS-complete the approved current candidate with operation-id replay and crash fencing.", + ), + "MergeRequestReviewSubmit" => { + Some("Submit the attested direct-child Reviewer result for the current candidate.") + } _ => None, } } @@ -326,16 +374,20 @@ mod tests { use super::*; #[test] - fn merge_request_tool_contract_omits_redundant_revision_evidence_and_candidate_result_tool() { + fn merge_request_tool_contract_uses_selectors_and_commit_fences_without_revision_ids() { let open = serde_json::to_string(&schemars::schema_for!(OpenInput)).unwrap(); - let add = serde_json::to_string(&schemars::schema_for!(AddRevisionInput)).unwrap(); + let request = serde_json::to_string(&schemars::schema_for!(RequestReviewInput)).unwrap(); let complete = serde_json::to_string(&schemars::schema_for!(CompleteInput)).unwrap(); - assert!(!open.contains("head_tree")); - assert!(!add.contains("head_tree")); - assert!(!open.contains("diff_digest")); - assert!(!add.contains("diff_digest")); - assert!(complete.contains("result_commit")); - assert!(complete.contains("conflicts_resolved")); - assert!(!MERGE_REQUEST_COMMON_TOOL_NAMES.contains(&"MergeRequestRecordMergeResult")); + assert!(open.contains("selector_from")); + assert!(open.contains("selector_to")); + assert!(request.contains("expected_head_commit")); + assert!(complete.contains("expected_head_commit")); + for schema in [&open, &request, &complete] { + assert!(!schema.contains("revision_id")); + assert!(!schema.contains("attempt_id")); + assert!(!schema.contains("head_tree")); + assert!(!schema.contains("diff_digest")); + } + assert!(!MERGE_REQUEST_COMMON_TOOL_NAMES.contains(&"MergeRequestAddRevision")); } } diff --git a/crates/worker/src/feature/builtin/ticket.rs b/crates/worker/src/feature/builtin/ticket.rs index 099b7978..31236c7b 100644 --- a/crates/worker/src/feature/builtin/ticket.rs +++ b/crates/worker/src/feature/builtin/ticket.rs @@ -366,7 +366,7 @@ impl FeatureModule for TicketFeature { )); } if let TicketFeatureBackend::WorkspaceClient(client) = &self.backend { - let names: Vec<&str> = if client.reviewer_attempt_context().is_some() { + let names: Vec<&str> = if client.reviewer_context().is_some() { vec![ "MergeRequestShow", merge_request::MERGE_REQUEST_REVIEW_TOOL_NAME, @@ -426,7 +426,7 @@ impl FeatureModule for TicketFeature { tools.register(ToolContribution::new(name, definition))?; } if let TicketFeatureBackend::WorkspaceClient(client) = &self.backend { - let definitions = if client.reviewer_attempt_context().is_some() { + let definitions = if client.reviewer_context().is_some() { merge_request::reviewer_tools(client.clone()) } else { merge_request::common_tools(client.clone()) diff --git a/crates/worker/src/spawn/tool.rs b/crates/worker/src/spawn/tool.rs index 5f29ef5f..0583daaf 100644 --- a/crates/worker/src/spawn/tool.rs +++ b/crates/worker/src/spawn/tool.rs @@ -28,7 +28,7 @@ use crate::internal_worker::{ use crate::prompt::catalog::PromptCatalog; use crate::spawn::registry::SpawnedWorkerRegistry; use crate::worker::{ - ReviewerAttemptContext, ReviewerChildWorkspaceClient, Worker, WorkerFilesystemAuthority, + ReviewerChildWorkspaceClient, ReviewerContext, Worker, WorkerFilesystemAuthority, WorkspaceRequest, WorkspaceRequestMethod, }; use protocol::Method; @@ -58,8 +58,8 @@ struct SubWorkerSpawnInput { /// spawner's explicit delegation authority; direct tool scope alone is not /// sufficient. Omit `recursive` for normal workspace/worktree delegation; it defaults to true. scope: Vec, - /// Binds an actual read-only builtin Reviewer child to an immutable Merge Request revision. - /// Review attempt identity and capability material are generated by the trusted spawn layer. + /// Binds an actual read-only builtin Reviewer child to the current Merge Request candidate. + /// Review capability material is generated by the trusted spawn layer. #[serde(default)] review: Option, } @@ -67,7 +67,7 @@ struct SubWorkerSpawnInput { #[derive(Debug, Deserialize, schemars::JsonSchema)] struct ReviewerHandoffInput { ticket_id: String, - revision_id: String, + expected_head_commit: String, } #[derive(Debug, Deserialize, schemars::JsonSchema)] @@ -337,9 +337,9 @@ fn validate_reviewer_handoff(input: &SubWorkerSpawnInput) -> Result<(), ToolErro let Some(review) = &input.review else { return Ok(()); }; - if review.ticket_id.trim().is_empty() || review.revision_id.trim().is_empty() { + if review.ticket_id.trim().is_empty() || review.expected_head_commit.trim().is_empty() { return Err(ToolError::InvalidArgument( - "reviewer handoff requires non-empty ticket_id and revision_id".to_string(), + "reviewer handoff requires non-empty ticket_id and expected_head_commit".to_string(), )); } if input.profile.as_deref() != Some("builtin:reviewer") { @@ -418,11 +418,10 @@ impl Tool for SubWorkerSpawnTool { .map_err(|error| { ToolError::ExecutionFailed(format!("resolve child manifest: {error}")) })?; - let reviewer_attempt = input.review.as_ref().map(|review| { + let reviewer_capability = input.review.as_ref().map(|review| { ( review.ticket_id.clone(), - review.revision_id.clone(), - uuid::Uuid::now_v7().to_string(), + review.expected_head_commit.clone(), format!( "{}{}", uuid::Uuid::now_v7().simple(), @@ -431,7 +430,8 @@ impl Tool for SubWorkerSpawnTool { ) }); let child_workspace_context = - if let Some((ticket_id, revision_id, _, capability_token)) = &reviewer_attempt { + if let Some((ticket_id, expected_head_commit, capability_token)) = &reviewer_capability + { let workspace_id = self.workspace_context .workspace_id() @@ -450,9 +450,9 @@ impl Tool for SubWorkerSpawnTool { let child_client: Arc = Arc::new(ReviewerChildWorkspaceClient::new( parent_client.clone(), - ReviewerAttemptContext { + ReviewerContext { ticket_id: ticket_id.clone(), - revision_id: revision_id.clone(), + expected_head_commit: expected_head_commit.clone(), }, capability_token.clone(), )); @@ -547,9 +547,9 @@ impl Tool for SubWorkerSpawnTool { } }; - if let Some((ticket_id, revision_id, attempt_id, capability_token)) = &reviewer_attempt { + if let Some((ticket_id, expected_head_commit, capability_token)) = &reviewer_capability { let workspace_id = self.workspace_context.workspace_id().ok_or_else(|| { - ToolError::ExecutionFailed("reviewer attempt lost Workspace identity".to_string()) + ToolError::ExecutionFailed("review capability lost Workspace identity".to_string()) })?; let child_session_id = session.session_id_string(); let child_registration = WorkspaceRequest::json( @@ -577,15 +577,14 @@ impl Tool for SubWorkerSpawnTool { ))); } let body = serde_json::json!({ - "attempt_id": attempt_id, - "revision_id": revision_id, + "expected_head_commit": expected_head_commit, "child_session_id": child_session_id, "capability_token": capability_token, }); let request = WorkspaceRequest::json( WorkspaceRequestMethod::Post, format!( - "/api/w/{}/tickets/{}/merge-request/review-attempts", + "/api/w/{}/tickets/{}/merge-request/review-capabilities", workspace_id.as_str(), ticket_id ), @@ -596,12 +595,12 @@ impl Tool for SubWorkerSpawnTool { .client() .execute(request) .map_err(|error| { - ToolError::ExecutionFailed(format!("register reviewer attempt: {error}")) + ToolError::ExecutionFailed(format!("register review capability: {error}")) })?; if !response.is_success() { let _ = session.stop().await; return Err(ToolError::ExecutionFailed(format!( - "register reviewer attempt failed with status {}: {}", + "register review capability failed with status {}: {}", response.status, response.body ))); } @@ -1044,21 +1043,21 @@ mod tests { let valid: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({ "name":"reviewer","task":"review","profile":"builtin:reviewer", "scope":[{"target":"/tmp/work","permission":"read"}], - "review":{"ticket_id":"T1","revision_id":"V1"} + "review":{"ticket_id":"T1","expected_head_commit":"V1"} })) .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":"/tmp/work","permission":"read"}], - "review":{"ticket_id":"T1","revision_id":"V1"} + "review":{"ticket_id":"T1","expected_head_commit":"V1"} })) .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":"/tmp/work","permission":"write"}], - "review":{"ticket_id":"T1","revision_id":"V1"} + "review":{"ticket_id":"T1","expected_head_commit":"V1"} })) .unwrap(); assert!(validate_reviewer_handoff(&writable).is_err()); diff --git a/crates/worker/src/worker.rs b/crates/worker/src/worker.rs index a9b5bd6d..9f7fa723 100644 --- a/crates/worker/src/worker.rs +++ b/crates/worker/src/worker.rs @@ -238,30 +238,30 @@ pub trait WorkspaceClient: std::fmt::Debug + Send + Sync { )) } - /// Trusted review-attempt context is injected by the Internal SubWorker spawn layer. + /// Trusted review capability context is injected by the Internal SubWorker spawn layer. /// It is never accepted from a model-visible tool argument. - fn reviewer_attempt_context(&self) -> Option<&ReviewerAttemptContext> { + fn reviewer_context(&self) -> Option<&ReviewerContext> { None } } #[derive(Debug, Clone, PartialEq, Eq)] -pub struct ReviewerAttemptContext { +pub struct ReviewerContext { pub ticket_id: String, - pub revision_id: String, + pub expected_head_commit: String, } #[derive(Debug)] pub struct ReviewerChildWorkspaceClient { inner: Arc, - context: ReviewerAttemptContext, + context: ReviewerContext, capability_token: String, } impl ReviewerChildWorkspaceClient { pub fn new( inner: Arc, - context: ReviewerAttemptContext, + context: ReviewerContext, capability_token: String, ) -> Self { Self { @@ -282,7 +282,7 @@ impl WorkspaceClient for ReviewerChildWorkspaceClient { fn is_available(&self) -> bool { self.inner.is_available() } - fn reviewer_attempt_context(&self) -> Option<&ReviewerAttemptContext> { + fn reviewer_context(&self) -> Option<&ReviewerContext> { Some(&self.context) } @@ -307,8 +307,8 @@ impl WorkspaceClient for ReviewerChildWorkspaceClient { ) })?; object.insert( - "revision_id".to_string(), - serde_json::Value::String(self.context.revision_id.clone()), + "expected_head_commit".to_string(), + serde_json::Value::String(self.context.expected_head_commit.clone()), ); object.insert( "capability_token".to_string(), @@ -450,9 +450,9 @@ mod reviewer_client_tests { }); let client = ReviewerChildWorkspaceClient::new( inner, - ReviewerAttemptContext { + ReviewerContext { ticket_id: "T1".into(), - revision_id: "V1".into(), + expected_head_commit: "head".into(), }, "secret".into(), ); diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index d24e8dd5..19067849 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -1324,16 +1324,16 @@ pub fn build_router(api: WorkspaceApi) -> Router { get(scoped_merge_request_readiness), ) .route( - "/api/w/{workspace_id}/tickets/{id}/merge-request/revisions", - post(scoped_add_merge_request_revision), + "/api/w/{workspace_id}/tickets/{id}/merge-request/review-requests", + post(scoped_request_merge_request_review), ) .route( "/api/w/{workspace_id}/internal/reviewer-child-sessions", post(scoped_register_reviewer_child_session), ) .route( - "/api/w/{workspace_id}/tickets/{id}/merge-request/review-attempts", - post(scoped_register_merge_request_review_attempt), + "/api/w/{workspace_id}/tickets/{id}/merge-request/review-capabilities", + post(scoped_register_merge_request_review_capability), ) .route( "/api/w/{workspace_id}/tickets/{id}/merge-request/reviews", @@ -1343,6 +1343,10 @@ pub fn build_router(api: WorkspaceApi) -> Router { "/api/w/{workspace_id}/tickets/{id}/merge-request/complete", post(scoped_complete_merge_request), ) + .route( + "/api/w/{workspace_id}/tickets/{id}/merge-request/close", + post(scoped_close_merge_request), + ) .route( "/api/w/{workspace_id}/tickets/{id}/merge-request/reopen", post(scoped_reopen_merge_request), @@ -3577,7 +3581,8 @@ async fn scoped_queue_ticket_record( #[derive(Debug, serde::Deserialize)] struct OpenMergeRequestRequest { repository_id: String, - revision_id: String, + selector_from: String, + selector_to: String, base_commit: String, head_commit: String, #[serde(default)] @@ -3587,9 +3592,8 @@ struct OpenMergeRequestRequest { } #[derive(Debug, serde::Deserialize)] -struct AddMergeRequestRevisionRequest { - expected_current_revision_id: String, - revision_id: String, +struct RequestMergeRequestReviewRequest { + expected_head_commit: String, base_commit: String, head_commit: String, #[serde(default)] @@ -3604,16 +3608,15 @@ struct RegisterReviewerChildSessionRequest { } #[derive(Debug, serde::Deserialize)] -struct RegisterMergeRequestReviewAttemptRequest { - attempt_id: String, - revision_id: String, +struct RegisterMergeRequestReviewCapabilityRequest { + expected_head_commit: String, child_session_id: String, capability_token: String, } #[derive(Debug, serde::Deserialize)] struct SubmitMergeRequestReviewRequest { - revision_id: String, + expected_head_commit: String, capability_token: String, decision: merge_request::ReviewDecision, #[serde(default)] @@ -3625,17 +3628,18 @@ struct SubmitMergeRequestReviewRequest { #[derive(Debug, serde::Deserialize)] struct CompleteMergeRequestRequest { operation_id: String, - expected_revision_id: String, + expected_head_commit: String, target_commit: String, source_commit: String, result_commit: String, strategy: merge_request::MergeStrategy, - resolution: merge_request::MergeResolution, + resolution: merge_request::ConflictResolution, } #[derive(Debug, serde::Deserialize)] -struct RevisionTransitionRequest { - expected_revision_id: String, +struct MergeRequestStateRequest { + #[serde(default)] + body: String, explicit_confirmation: bool, } @@ -3653,14 +3657,84 @@ fn require_workspace_access(workspace_id: &str, api: &WorkspaceApi) -> ApiResult Ok(()) } +#[derive(Clone)] +struct MergeRequestAssignmentSource { + store: Arc, +} + +impl merge_request::AssignmentSource for MergeRequestAssignmentSource { + fn current_assignment( + &self, + workspace_id: &str, + ticket_id: &str, + ) -> std::result::Result, String> { + self.store + .get_current_ticket_worker_assignment(workspace_id, ticket_id) + .map(|value| { + value.map(|assignment| merge_request::CurrentAssignment { + assignment_id: assignment.assignment_id, + ticket_id: ticket_id.to_string(), + runtime_id: assignment.worker.runtime_id, + worker_id: assignment.worker.worker_id, + }) + }) + .map_err(|error| error.to_string()) + } +} + +#[derive(Clone)] +struct MergeRequestRepositorySource { + workspace_id: String, + reader: RepositoryRegistryReader, +} + +impl merge_request::RepositorySource for MergeRequestRepositorySource { + fn repository_belongs_to_workspace( + &self, + workspace_id: &str, + repository_id: &str, + ) -> std::result::Result { + if workspace_id != self.workspace_id { + return Ok(false); + } + Ok(self.reader.summary(repository_id).is_ok()) + } + + fn is_ancestor( + &self, + workspace_id: &str, + repository_id: &str, + ancestor: &str, + descendant: &str, + ) -> std::result::Result { + if workspace_id != self.workspace_id { + return Ok(false); + } + match self + .reader + .ensure_ancestor(repository_id, ancestor, descendant) + { + Ok(()) => Ok(true), + Err(RepositoryLookupError::InvalidCommitRelation { .. }) => Ok(false), + Err(error) => Err(format!("{error:?}")), + } + } +} + fn merge_request_store( api: &WorkspaceApi, workspace_id: &str, -) -> ApiResult { +) -> ApiResult { require_workspace_access(workspace_id, api)?; - merge_request::SqliteMergeRequestStore::open_verified( + merge_request::MergeRequestStore::open( api.config.database_path.clone(), - workspace_id, + Arc::new(MergeRequestAssignmentSource { + store: api.store.clone(), + }), + Arc::new(MergeRequestRepositorySource { + workspace_id: workspace_id.to_string(), + reader: api.repository_reader(), + }), ) .map_err(Error::from) .map_err(Into::into) @@ -3724,51 +3798,34 @@ fn validate_revision_evidence( Ok((base.commit, source.commit)) } -fn observe_merge_request_target( - api: &WorkspaceApi, - mr: &merge_request::MergeRequest, -) -> Option { - let selector = mr.target_ref_selector.as_deref()?; - api.repository_reader() - .observe_merge_target(&mr.repository_id, Some(selector)) - .ok() - .map(|target| target.commit) -} - async fn scoped_show_merge_request( State(api): State, AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, ) -> ApiResult> { let workspace_id = parse_workspace_id(&workspace_id)?; - let store = merge_request_store(&api, &workspace_id)?; - let current = store.show_for_ticket(&ticket_id)?.ok_or_else(|| { - Error::from(merge_request::MergeRequestError::NotFound( - ticket_id.clone(), - )) - })?; - let target_commit = observe_merge_request_target(&api, ¤t); - let value = store - .show_for_ticket_with_target(&ticket_id, target_commit.as_deref())? - .ok_or_else(|| Error::from(merge_request::MergeRequestError::NotFound(ticket_id)))?; - Ok(Json(value)) + Ok(Json( + merge_request_store(&api, &workspace_id)?.get(&workspace_id, &ticket_id)?, + )) } async fn scoped_merge_request_readiness( State(api): State, AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, -) -> ApiResult> { +) -> ApiResult> { let workspace_id = parse_workspace_id(&workspace_id)?; let store = merge_request_store(&api, &workspace_id)?; - let current = store.show_for_ticket(&ticket_id)?.ok_or_else(|| { - Error::from(merge_request::MergeRequestError::NotFound( - ticket_id.clone(), - )) - })?; - let target_commit = observe_merge_request_target(&api, ¤t); - Ok(Json(store.readiness_for_ticket_with_target( - &ticket_id, - target_commit.as_deref(), - )?)) + let mr = store.get(&workspace_id, &ticket_id)?; + Ok(Json(store.readiness(merge_request::ReadinessCheck { + ticket_id, + expected_head_commit: None, + auth: merge_request::MergeRequestAuth { + workspace_id, + repository_id: mr.repository_id, + runtime_id: String::new(), + worker_id: String::new(), + assignment_id: String::new(), + }, + })?)) } async fn scoped_open_merge_request( @@ -3784,13 +3841,13 @@ async fn scoped_open_merge_request( .store .get_current_ticket_worker_assignment(&workspace_id, &ticket_id)? .ok_or_else(|| { - Error::TicketAssignmentConflict("Ticket has no current assigned Coder".to_string()) + Error::TicketAssignmentConflict("Ticket has no current assigned Coder".into()) })?; if assignment.worker.runtime_id != source.runtime_id || assignment.worker.worker_id != source.worker_id { return Err(Error::TicketAssignmentConflict( - "authenticated Worker is not the current Ticket assignee".to_string(), + "authenticated Worker is not the current Ticket assignee".into(), ) .into()); } @@ -3801,38 +3858,54 @@ async fn scoped_open_merge_request( &input.base_commit, &input.head_commit, )?; - let now = Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true); - let revision = merge_request::MergeRequestRevision { - revision_id: input.revision_id, - ordinal: 1, - base_commit, - head_commit: source_commit, - changed_paths: input.changed_paths, - summary: input.summary, - assignment_id: assignment.assignment_id.clone(), - created_at: now.clone(), - }; + if input.selector_to != target.selector { + return Err(Error::InvalidInput( + "selector_to must match the authoritative Ticket target selector".into(), + ) + .into()); + } + let observed_source = api + .repository_reader() + .observe_merge_target(&input.repository_id, Some(&input.selector_from)) + .map_err(repository_merge_evidence_error)?; + if observed_source.commit != source_commit { + return Err(Error::InvalidInput( + "selector_from does not resolve to the nominated head commit".into(), + ) + .into()); + } let mr = merge_request_store(&api, &workspace_id)?.open_merge_request( merge_request::OpenMergeRequest { - merge_request_id: format!("mr_{}", Uuid::now_v7().simple()), + merge_request_id: Uuid::now_v7().to_string(), ticket_id, - repository_id: input.repository_id, - target_ref_selector: target.selector, - revision, - authenticated_runtime_id: source.runtime_id, - authenticated_worker_id: source.worker_id, - now, + repository_id: input.repository_id.clone(), + selector_from: input.selector_from, + selector_to: input.selector_to, + request: merge_request::RequestForReview { + base_commit, + head_commit: source_commit, + changed_paths: input.changed_paths, + summary: input.summary, + }, + auth: merge_request::MergeRequestAuth { + workspace_id, + repository_id: input.repository_id, + runtime_id: source.runtime_id, + worker_id: source.worker_id, + assignment_id: assignment.assignment_id, + }, + now: Utc::now(), }, )?; Ok(Json(mr)) } -async fn scoped_add_merge_request_revision( +async fn scoped_request_merge_request_review( State(api): State, headers: HeaderMap, AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, - Json(input): Json, -) -> ApiResult> { + Json(input): Json, +) -> ApiResult> { let workspace_id = parse_workspace_id(&workspace_id)?; require_workspace_access(&workspace_id, &api)?; let source = authenticate_worker_mutation_source(&api, &workspace_id, &headers)?; @@ -3850,39 +3923,44 @@ async fn scoped_add_merge_request_revision( ) .into()); } - let current = merge_request_store(&api, &workspace_id)? - .show_for_ticket(&ticket_id)? - .ok_or_else(|| { - Error::from(merge_request::MergeRequestError::NotFound( - ticket_id.clone(), - )) - })?; - let (base_commit, head_commit) = validate_revision_evidence( + let store = merge_request_store(&api, &workspace_id)?; + let current = store.get(&workspace_id, &ticket_id)?; + let (base_commit, source_commit) = validate_revision_evidence( &api, ¤t.repository_id, &input.base_commit, &input.head_commit, )?; - let now = Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true); - let mr = - merge_request_store(&api, &workspace_id)?.add_revision(merge_request::AddRevision { + let observed_source = api + .repository_reader() + .observe_merge_target(¤t.repository_id, Some(¤t.selector_from)) + .map_err(repository_merge_evidence_error)?; + if observed_source.commit != source_commit { + return Err(Error::InvalidInput( + "selector_from does not resolve to the nominated head commit".into(), + ) + .into()); + } + Ok(Json(store.request_review( + merge_request::RequestMergeRequestReview { ticket_id, - expected_current_revision_id: input.expected_current_revision_id, - revision: merge_request::MergeRequestRevision { - revision_id: input.revision_id, - ordinal: current.current_revision.ordinal + 1, + expected_head_commit: input.expected_head_commit, + request: merge_request::RequestForReview { base_commit, - head_commit, + head_commit: source_commit, changed_paths: input.changed_paths, summary: input.summary, - assignment_id: assignment.assignment_id, - created_at: now.clone(), }, - authenticated_runtime_id: source.runtime_id, - authenticated_worker_id: source.worker_id, - now, - })?; - Ok(Json(mr)) + auth: merge_request::MergeRequestAuth { + workspace_id, + repository_id: current.repository_id, + runtime_id: source.runtime_id, + worker_id: source.worker_id, + assignment_id: assignment.assignment_id, + }, + now: Utc::now(), + }, + )?)) } async fn scoped_register_reviewer_child_session( @@ -3896,20 +3974,22 @@ async fn scoped_register_reviewer_child_session( let source = authenticate_worker_mutation_source(&api, &workspace_id, &headers)?; merge_request_store(&api, &workspace_id)?.register_reviewer_child_session( merge_request::RegisterReviewerChildSession { + workspace_id, parent_runtime_id: source.runtime_id, parent_worker_id: source.worker_id, child_session_id: input.child_session_id, - now: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), + reviewer_profile: "builtin:reviewer".into(), + now: Utc::now(), }, )?; Ok(StatusCode::NO_CONTENT) } -async fn scoped_register_merge_request_review_attempt( +async fn scoped_register_merge_request_review_capability( State(api): State, headers: HeaderMap, AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, - Json(input): Json, + Json(input): Json, ) -> ApiResult { let workspace_id = parse_workspace_id(&workspace_id)?; require_workspace_access(&workspace_id, &api)?; @@ -3928,19 +4008,22 @@ async fn scoped_register_merge_request_review_attempt( ) .into()); } - merge_request_store(&api, &workspace_id)?.register_review_attempt( - merge_request::RegisterReviewAttempt { - attempt_id: input.attempt_id, - ticket_id, - revision_id: input.revision_id, - parent_assignment_id: assignment.assignment_id, - parent_runtime_id: source.runtime_id, - parent_worker_id: source.worker_id, - child_session_id: input.child_session_id, - capability_token: input.capability_token, - now: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), + let store = merge_request_store(&api, &workspace_id)?; + let current = store.get(&workspace_id, &ticket_id)?; + store.register_review_capability(merge_request::RegisterReviewCapability { + ticket_id, + expected_head_commit: input.expected_head_commit, + child_session_id: input.child_session_id, + capability_token: input.capability_token, + auth: merge_request::MergeRequestAuth { + workspace_id, + repository_id: current.repository_id, + runtime_id: source.runtime_id, + worker_id: source.worker_id, + assignment_id: assignment.assignment_id, }, - )?; + now: Utc::now(), + })?; Ok(StatusCode::NO_CONTENT) } @@ -3948,19 +4031,21 @@ async fn scoped_submit_merge_request_review( State(api): State, AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, Json(input): Json, -) -> ApiResult> { +) -> ApiResult> { let workspace_id = parse_workspace_id(&workspace_id)?; - let review = - merge_request_store(&api, &workspace_id)?.submit_review(merge_request::SubmitReview { - ticket_id, - revision_id: input.revision_id, - capability_token: input.capability_token, - decision: input.decision, - body: input.body, - findings: input.findings, - now: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), - })?; - Ok(Json(review)) + Ok(Json( + merge_request_store(&api, &workspace_id)?.submit_review( + merge_request::SubmitMergeRequestReview { + ticket_id, + expected_head_commit: input.expected_head_commit, + capability_token: input.capability_token, + decision: input.decision, + body: input.body, + findings: input.findings, + now: Utc::now(), + }, + )?, + )) } async fn scoped_complete_merge_request( @@ -3968,7 +4053,7 @@ async fn scoped_complete_merge_request( headers: HeaderMap, AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, Json(input): Json, -) -> ApiResult> { +) -> ApiResult> { let workspace_id = parse_workspace_id(&workspace_id)?; require_workspace_access(&workspace_id, &api)?; let source = authenticate_worker_mutation_source(&api, &workspace_id, &headers)?; @@ -3980,182 +4065,101 @@ async fn scoped_complete_merge_request( Error::TicketAssignmentConflict("Ticket has no current assigned Coder".into()) })?; let store = merge_request_store(&api, &workspace_id)?; - let mr = store - .show_for_ticket(&ticket_id)? - .ok_or_else(|| merge_request::MergeRequestError::NotFound(ticket_id.clone()))?; - if mr.current_revision.revision_id != input.expected_revision_id { - return Err(merge_request::MergeRequestError::StaleRevision { - expected: input.expected_revision_id, - current: mr.current_revision.revision_id, - } - .into()); - } - if mr.review_status != merge_request::ReviewStatus::Approved { - return Err(merge_request::MergeRequestError::NotApproved.into()); - } - if input.source_commit != mr.current_revision.head_commit { - return Err(merge_request::MergeRequestError::InvalidMergeOutcome( - "source commit does not match the current approved revision".into(), + let mr = store.get(&workspace_id, &ticket_id)?; + let current_request = mr.current_request().ok_or_else(|| { + Error::InvalidInput("Merge Request has no current RequestForReview event".into()) + })?; + if current_request.head_commit != input.expected_head_commit + || input.source_commit != current_request.head_commit + { + return Err(Error::InvalidInput( + "source commit does not match the current review request".into(), ) .into()); } - if mr.state == merge_request::MergeRequestState::Merged { - return Ok(Json(store.complete( - merge_request::CompleteMergeRequest { - operation_id: input.operation_id, - ticket_id, - expected_revision_id: mr.current_revision.revision_id, - target_commit: input.target_commit, - source_commit: input.source_commit, - result_commit: input.result_commit, - strategy: input.strategy, - resolution: input.resolution, - implementation_assignment_id: assignment.assignment_id, - completion_actor_runtime_id: source.runtime_id, - completion_actor_worker_id: source.worker_id, - now: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), - }, - )?)); - } - let selector = mr - .target_ref_selector - .as_deref() - .ok_or(merge_request::MergeRequestError::UnknownTarget)?; let repositories = api.repository_reader(); let observed_target = repositories - .observe_merge_target(&mr.repository_id, Some(selector)) + .observe_merge_target(&mr.repository_id, Some(&mr.selector_to)) .map_err(repository_merge_evidence_error)?; - let source_commit = repositories - .observe_commit(&mr.repository_id, &input.source_commit) - .map_err(repository_merge_evidence_error)?; - if source_commit.commit != input.source_commit { - return Err(Error::InvalidInput("source commit must be canonical".into()).into()); - } - let result_commit = repositories - .observe_commit(&mr.repository_id, &input.result_commit) - .map_err(repository_merge_evidence_error)?; - if result_commit.commit != input.result_commit { - return Err(Error::InvalidInput("result commit must be canonical".into()).into()); - } - match input.strategy { - merge_request::MergeStrategy::FastForward => { - if input.resolution != merge_request::MergeResolution::None - || input.result_commit != input.source_commit - { - return Err(merge_request::MergeRequestError::InvalidMergeOutcome( - "fast-forward result must equal the approved source and use resolution=none" - .into(), - ) - .into()); - } - repositories - .ensure_ancestor( - &mr.repository_id, - &input.target_commit, - &input.source_commit, - ) - .map_err(repository_merge_evidence_error)?; - } - merge_request::MergeStrategy::Merge => { - if input.resolution == merge_request::MergeResolution::None - || result_commit.parents - != vec![input.target_commit.clone(), input.source_commit.clone()] - { - return Err(merge_request::MergeRequestError::InvalidMergeOutcome( - "merge result must have the expected target and approved source as its two ordered parents" - .into(), - ) - .into()); - } - } - } - let target_was_already_updated = observed_target.commit == input.result_commit; - if observed_target.commit != input.target_commit && !target_was_already_updated { + if observed_target.commit != input.target_commit + && observed_target.commit != input.result_commit + { return Err(Error::InvalidInput(format!( "Merge Request target moved: expected {}, observed {}", input.target_commit, observed_target.commit )) .into()); } + let target_was_already_updated = observed_target.commit == input.result_commit; if !target_was_already_updated { repositories .update_merge_target( &mr.repository_id, - selector, + &mr.selector_to, &input.target_commit, &input.result_commit, ) .map_err(repository_merge_evidence_error)?; } - let verified_target = repositories.observe_merge_target(&mr.repository_id, Some(selector)); - let verified_result = matches!( - verified_target.as_ref(), - Ok(target) if target.commit == input.result_commit - ); - if !verified_result { - if !target_was_already_updated { - if let Err(rollback_error) = repositories.update_merge_target( - &mr.repository_id, - selector, - &input.result_commit, - &input.target_commit, - ) { - return Err(Error::InvalidInput(format!( - "post-update target verification failed and guarded rollback also failed: verification={verified_target:?}; rollback={rollback_error:?}" - )) - .into()); - } - } - return Err(Error::InvalidInput(format!( - "post-update target verification failed: expected result {}, observed {:?}", - input.result_commit, - verified_target - .as_ref() - .map(|target| target.commit.as_str()) - .map_err(|error| error) - )) - .into()); - } let completion = merge_request::CompleteMergeRequest { - operation_id: input.operation_id, ticket_id, - expected_revision_id: mr.current_revision.revision_id, + expected_head_commit: input.expected_head_commit, + operation_id: input.operation_id, target_commit: input.target_commit.clone(), source_commit: input.source_commit, result_commit: input.result_commit.clone(), strategy: input.strategy, resolution: input.resolution, - implementation_assignment_id: assignment.assignment_id, - completion_actor_runtime_id: source.runtime_id, - completion_actor_worker_id: source.worker_id, - now: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), + auth: merge_request::MergeRequestAuth { + workspace_id, + repository_id: mr.repository_id.clone(), + runtime_id: source.runtime_id, + worker_id: source.worker_id, + assignment_id: assignment.assignment_id, + }, + now: Utc::now(), }; match store.complete(completion) { - Ok(outcome) => Ok(Json(outcome)), + Ok(event) => Ok(Json(event)), Err(error) => { if !target_was_already_updated { - if let Err(rollback_error) = repositories.update_merge_target( + let _ = repositories.update_merge_target( &mr.repository_id, - selector, + &mr.selector_to, &input.result_commit, &input.target_commit, - ) { - return Err(Error::InvalidInput(format!( - "merge finalization failed after target update and guarded rollback also failed: finalization={error}; rollback={rollback_error:?}" - )) - .into()); - } + ); } Err(error.into()) } } } +async fn scoped_close_merge_request( + State(api): State, + headers: HeaderMap, + AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, + Json(input): Json, +) -> ApiResult> { + scoped_change_merge_request_state(api, headers, workspace_id, ticket_id, input, false).await +} + async fn scoped_reopen_merge_request( State(api): State, headers: HeaderMap, AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, - Json(input): Json, + Json(input): Json, +) -> ApiResult> { + scoped_change_merge_request_state(api, headers, workspace_id, ticket_id, input, true).await +} + +async fn scoped_change_merge_request_state( + api: WorkspaceApi, + headers: HeaderMap, + workspace_id: String, + ticket_id: String, + input: MergeRequestStateRequest, + reopen: bool, ) -> ApiResult> { let workspace_id = parse_workspace_id(&workspace_id)?; require_workspace_access(&workspace_id, &api)?; @@ -4164,11 +4168,25 @@ async fn scoped_reopen_merge_request( if !input.explicit_confirmation { return Err(Error::BrowserReopenConfirmationRequired.into()); } - Ok(Json(merge_request_store(&api, &workspace_id)?.reopen( - &ticket_id, - &input.expected_revision_id, - &Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), - )?)) + let store = merge_request_store(&api, &workspace_id)?; + let mr = store.get(&workspace_id, &ticket_id)?; + let operation = merge_request::ChangeMergeRequestState { + ticket_id, + body: input.body, + auth: merge_request::MergeRequestAuth { + workspace_id, + repository_id: mr.repository_id, + runtime_id: "browser".into(), + worker_id: "authenticated-user".into(), + assignment_id: String::new(), + }, + now: Utc::now(), + }; + Ok(Json(if reopen { + store.reopen(operation)? + } else { + store.close(operation)? + })) } fn reject_non_browser_reopen_auth(headers: &HeaderMap) -> Result<()> { @@ -11904,10 +11922,10 @@ impl IntoResponse for ApiError { StatusCode::BAD_REQUEST } Error::Ticket(ticket::TicketError::NotFound(_)) - | Error::MergeRequest(merge_request::MergeRequestError::NotFound(_)) => { + | Error::MergeRequest(merge_request::MergeRequestError::NotFound) => { StatusCode::NOT_FOUND } - Error::MergeRequest(merge_request::MergeRequestError::Empty(_)) => { + Error::MergeRequest(merge_request::MergeRequestError::Validation(_)) => { StatusCode::BAD_REQUEST } Error::MergeRequest(_) => StatusCode::CONFLICT, @@ -12796,53 +12814,59 @@ mod tests { merge_request_id: "MR-server-completion".into(), ticket_id: ticket.id.clone(), repository_id: TEST_REPOSITORY_ID.into(), - target_ref_selector: target_ref.clone(), - revision: merge_request::MergeRequestRevision { - revision_id: "V1".into(), - ordinal: 1, + selector_from: source_commit.clone(), + selector_to: target_ref.clone(), + request: merge_request::RequestForReview { base_commit: target_commit.clone(), head_commit: source_commit.clone(), - changed_paths: vec!["src/lib.rs".into()], - summary: "approved revision".into(), - assignment_id: assignment.assignment_id.clone(), - created_at: "t1".into(), + summary: "approved candidate".into(), }, - authenticated_runtime_id: coder.worker_ref.runtime_id.clone(), - authenticated_worker_id: coder.worker_ref.worker_id.clone(), - now: "t1".into(), + auth: merge_request::MergeRequestAuth { + workspace_id: workspace_id.clone(), + repository_id: TEST_REPOSITORY_ID.into(), + runtime_id: coder.worker_ref.runtime_id.clone(), + worker_id: coder.worker_ref.worker_id.clone(), + assignment_id: assignment.assignment_id.clone(), + }, + now: Utc::now(), }) .unwrap(); mr_store .register_reviewer_child_session(merge_request::RegisterReviewerChildSession { + workspace_id: workspace_id.clone(), parent_runtime_id: coder.worker_ref.runtime_id.clone(), parent_worker_id: coder.worker_ref.worker_id.clone(), child_session_id: "reviewer-child".into(), - now: "t2".into(), + reviewer_profile: "builtin:reviewer".into(), + now: Utc::now(), }) .unwrap(); mr_store - .register_review_attempt(merge_request::RegisterReviewAttempt { - attempt_id: "attempt".into(), + .register_review_capability(merge_request::RegisterReviewCapability { ticket_id: ticket.id.clone(), - revision_id: "V1".into(), - parent_assignment_id: assignment.assignment_id.clone(), - parent_runtime_id: coder.worker_ref.runtime_id.clone(), - parent_worker_id: coder.worker_ref.worker_id.clone(), + expected_head_commit: source_commit.clone(), child_session_id: "reviewer-child".into(), capability_token: "review-token".into(), - now: "t2".into(), + auth: merge_request::MergeRequestAuth { + workspace_id: workspace_id.clone(), + repository_id: TEST_REPOSITORY_ID.into(), + runtime_id: coder.worker_ref.runtime_id.clone(), + worker_id: coder.worker_ref.worker_id.clone(), + assignment_id: assignment.assignment_id.clone(), + }, + now: Utc::now(), }) .unwrap(); mr_store - .submit_review(merge_request::SubmitReview { + .submit_review(merge_request::SubmitMergeRequestReview { ticket_id: ticket.id.clone(), - revision_id: "V1".into(), + expected_head_commit: source_commit.clone(), capability_token: "review-token".into(), decision: merge_request::ReviewDecision::Approve, body: "approved".into(), findings: Vec::new(), - now: "t3".into(), + now: Utc::now(), }) .unwrap(); @@ -12860,12 +12884,12 @@ mod tests { }; let request = || CompleteMergeRequestRequest { operation_id: "complete-operation".into(), - expected_revision_id: "V1".into(), + expected_head_commit: source_commit.clone(), target_commit: target_commit.clone(), source_commit: source_commit.clone(), result_commit: source_commit.clone(), strategy: merge_request::MergeStrategy::FastForward, - resolution: merge_request::MergeResolution::None, + resolution: merge_request::ConflictResolution::None, }; let coder_error = scoped_complete_merge_request( State(api.clone()), @@ -12897,7 +12921,7 @@ mod tests { ) .await .unwrap(); - assert!(!completed.replayed); + assert_eq!(completed.result_commit, source_commit); assert_eq!( backend .show(ticket.id.clone().into()) @@ -12913,19 +12937,16 @@ mod tests { .commit, source_commit ); - let merged = mr_store.show_for_ticket(&ticket.id).unwrap().unwrap(); + let mr = mr_store.get(&api.config.workspace_id, &ticket.id).unwrap(); + assert_eq!(mr.state, merge_request::MergeRequestState::Merged); + let merge = mr.thread.iter().find_map(|event| match event { + merge_request::MergeRequestThreadEvent::Merge(value) => Some(value), + _ => None, + }); assert_eq!( - merged.merged_target_commit.as_deref(), - Some(target_commit.as_str()) - ); - assert_eq!( - merged.merged_result_commit.as_deref(), + merge.map(|value| value.result_commit.as_str()), Some(source_commit.as_str()) ); - assert_eq!( - merged.merge_strategy, - Some(merge_request::MergeStrategy::FastForward) - ); let Json(replayed) = scoped_complete_merge_request( State(api.clone()), worker_headers(&orchestrator), @@ -12934,7 +12955,7 @@ mod tests { ) .await .unwrap(); - assert!(replayed.replayed); + assert_eq!(replayed.result_commit, source_commit); let conn = rusqlite::Connection::open(&api.config.database_path).unwrap(); let actor: String = conn .query_row( diff --git a/resources/prompts/common/git.md b/resources/prompts/common/git.md index 2b05bc52..cdb63df7 100644 --- a/resources/prompts/common/git.md +++ b/resources/prompts/common/git.md @@ -10,4 +10,4 @@ When creating a commit, use the change type as the subject prefix, not the affec A change made because review, validation, or user feedback found a defect is a `fix:` even when it belongs to the same feature Ticket and has not been merged yet. Do not keep reusing a domain prefix such as `merge-request:`, `runtime:`, or `worker:` across a series; those labels identify where the code lives rather than why each commit exists. If one prospective commit contains distinct change types, split it into coherent validated commits when practical; otherwise name it for the dominant intent. -Before opening or appending an immutable Merge Request revision, inspect the proposed commit subjects and correct misclassified local, unshared commits when safe. Do not rewrite shared history solely to rename existing commits unless the user explicitly requests it. +Before opening a Merge Request or appending a `RequestForReview` event, inspect the proposed commit subjects and correct misclassified local, unshared commits when safe. Do not rewrite shared history solely to rename existing commits unless the user explicitly requests it. diff --git a/resources/prompts/role/coder.md b/resources/prompts/role/coder.md index 339c7b57..1823cd7c 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 or append an immutable Merge Request revision containing the exact base/head/tree and changed-path evidence. Spawn the Reviewer only as your actual direct-child `builtin:reviewer` SubWorker, delegate read-only scope, and include the structured `review` handoff with the Ticket id and current MR revision id. Reviewer prose is not approval: the child must commit `MergeRequestReviewSubmit` through its injected attempt authority. +Before review, open a Merge Request with immutable `selector_from` / `selector_to`, then append a `RequestForReview` thread event containing the exact base/head commit and changed-path evidence. Spawn the Reviewer only as your actual direct-child `builtin:reviewer` SubWorker, delegate read-only scope, and include the structured review handoff with the Ticket id and current candidate head commit. Reviewer prose is not approval: the child must commit `MergeRequestReviewSubmit` through its injected capability authority. -A request-changes result requires a new immutable revision and a fresh Reviewer child attempt. Flow terminal state is not Ticket completion authority. Complete only through `MergeRequestComplete` with a unique operation id and the currently approved revision; the Server revalidates assignment and fences Ticket state side effects. +A request-changes result requires a new `RequestForReview` event and a fresh Reviewer child capability. Flow terminal state is not Ticket completion authority. Complete only through `MergeRequestComplete` with a unique operation id and the currently approved candidate head commit; the Server revalidates assignment and fences Ticket state side effects. diff --git a/resources/prompts/role/orchestrator.md b/resources/prompts/role/orchestrator.md index dc3e889b..08ac7478 100644 --- a/resources/prompts/role/orchestrator.md +++ b/resources/prompts/role/orchestrator.md @@ -4,7 +4,7 @@ You are the Ticket Orchestrator role. Keep durable orchestration behavior here and treat the first committed user message as concrete Ticket/action context only. Use typed Ticket tools and current repository state as authority. Record `inprogress` before implementation side effects, then use `SpawnTicketCoder` so Worker creation, the fixed Coder profile/Flow, and the current Ticket assignment are one guarded operation. After spawn, reread the Ticket and verify its current assignment names that Coder before asking it to implement; never route implementation to an unassigned Coder. Route implementation work to sibling Coder Workers, and stop for human authority when merge/closure is not explicitly delegated. -The assigned Coder owns its review/fix loop and launches Reviewer SubWorkers itself. Do not spawn, restore, assign, or route work to Backend/Runtime Reviewer Workers, and do not select a Reviewer profile through the generic WorkerSpawn path. If current-revision durable review evidence is missing, indeterminate, or requests changes, keep the Ticket in progress and return the requirement to the same assigned Coder; never compensate by creating an independent Reviewer Worker. +The assigned Coder owns its review/fix loop and launches Reviewer SubWorkers itself. Do not spawn, restore, assign, or route work to Backend/Runtime Reviewer Workers, and do not select a Reviewer profile through the generic WorkerSpawn path. If durable review evidence for the current `RequestForReview` candidate is missing, indeterminate, or requests changes, keep the Ticket in progress and return the requirement to the same assigned Coder; never compensate by creating an independent Reviewer Worker. Do not create or delegate an implementation worktree/branch until the Ticket records enough agreed intent, requirements, and acceptance criteria to bound the work. diff --git a/resources/prompts/role/reviewer.md b/resources/prompts/role/reviewer.md index 67334119..4e29746b 100644 --- a/resources/prompts/role/reviewer.md +++ b/resources/prompts/role/reviewer.md @@ -1,7 +1,7 @@ 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. Review the immutable current Merge Request revision 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. +Keep role behavior here and treat the first committed user message as bounded Ticket/Merge Request context only. Review the current `RequestForReview` candidate 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. Attempt identity and revision identity are injected by your child Workspace client and are not model inputs. If the authoritative revision changed, submission must fail rather than approving stale work. +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 the expected candidate head commit are injected by your child Workspace client and are not model inputs. If a newer `RequestForReview` event supersedes the candidate, submission must fail 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. diff --git a/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.svelte b/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.svelte index 5696a486..673cd1ff 100644 --- a/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.svelte +++ b/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.svelte @@ -17,22 +17,37 @@ TicketDetail, } from "$lib/workspace/sidebar/types"; + type MergeRequestThreadEvent = + | { + kind: "request_for_review"; + event_seq: number; + head_commit: string; + changed_paths: string[]; + summary: string; + } + | { + kind: "review"; + event_seq: number; + request_event_seq: number; + decision: "approve" | "request_changes"; + body: string; + reviewer_profile: string; + } + | { + kind: "merge"; + event_seq: number; + result_commit: string; + strategy: "fast_forward" | "merge"; + resolution: "none" | "clean" | "conflicts_resolved"; + merged_by: { runtime_id: string; worker_id: string }; + } + | { kind: "reopen" | "close"; event_seq: number; body: string }; + type MergeRequestDetail = { - state: "draft" | "open" | "closed" | "merged"; - review_status: "pending" | "approved" | "changes_requested"; - target_ref_selector?: string | null; - target_status: "known" | "unknown"; - observed_target_commit?: string | null; - current_revision: { revision_id: string; head_commit: string; changed_paths: string[]; summary: string }; - current_review?: { decision: string; body: string; reviewer_effective_profile: string } | null; - merged_revision_id?: string | null; - merged_target_commit?: string | null; - merged_result_commit?: string | null; - merge_strategy?: "fast_forward" | "merge" | null; - merge_resolution?: "none" | "clean" | "conflicts_resolved" | null; - merged_by_runtime_id?: string | null; - merged_by_worker_id?: string | null; - merged_at?: string | null; + state: "open" | "closed" | "merged"; + selector_from: string; + selector_to: string; + thread: MergeRequestThreadEvent[]; }; const MUTABLE_TICKET_STATES = TICKET_STATES.filter((state) => state !== "done"); @@ -56,6 +71,21 @@ let ticket = $state(loadedTicket); let mergeRequest = $state(initialData.mergeRequest.data ?? null); + const currentReviewRequest = $derived( + mergeRequest?.thread.findLast((event) => event.kind === "request_for_review") ?? null, + ); + const currentReview = $derived( + currentReviewRequest + ? mergeRequest?.thread.findLast( + (event) => + event.kind === "review" && + event.request_event_seq === currentReviewRequest.event_seq, + ) ?? null + : null, + ); + const mergeEvent = $derived( + mergeRequest?.thread.findLast((event) => event.kind === "merge") ?? null, + ); let editing = $state(false); let editTitle = $state(loadedTicket.title); let editBody = $state(loadedTicket.body); @@ -353,28 +383,21 @@ {#if data.mergeRequest.error}

{data.mergeRequest.error}

{:else if mergeRequest} -

{mergeRequest.state} · {mergeRequest.review_status}

-

Target {mergeRequest.target_ref_selector ?? "unknown"} · {mergeRequest.target_status}

- {#if mergeRequest.observed_target_commit}

Target tip {mergeRequest.observed_target_commit}

{/if} -

{mergeRequest.current_revision.revision_id}

-

Head {mergeRequest.current_revision.head_commit}

- {#if mergeRequest.merged_result_commit} -

- Final merge · {mergeRequest.merge_strategy} / {mergeRequest.merge_resolution} -

-

- Target before {mergeRequest.merged_target_commit} · result - {mergeRequest.merged_result_commit} -

-

- Revision {mergeRequest.merged_revision_id} · completed by - {mergeRequest.merged_by_runtime_id}/{mergeRequest.merged_by_worker_id} -

+

{mergeRequest.state}

+

From {mergeRequest.selector_from}

+

To {mergeRequest.selector_to}

+ {#if currentReviewRequest?.kind === "request_for_review"} +

Candidate {currentReviewRequest.head_commit}

+ {#if currentReviewRequest.summary}

{currentReviewRequest.summary}

{/if} {/if} - {#if mergeRequest.current_revision.summary}

{mergeRequest.current_revision.summary}

{/if} - {#if mergeRequest.current_review} -

{mergeRequest.current_review.decision} by {mergeRequest.current_review.reviewer_effective_profile}

- {#if mergeRequest.current_review.body}{/if} + {#if currentReview?.kind === "review"} +

{currentReview.decision} by {currentReview.reviewer_profile}

+ {#if currentReview.body}{/if} + {/if} + {#if mergeEvent?.kind === "merge"} +

Final merge · {mergeEvent.strategy} / {mergeEvent.resolution}

+

Result {mergeEvent.result_commit}

+

Completed by {mergeEvent.merged_by.runtime_id}/{mergeEvent.merged_by.worker_id}

{/if} {:else}

The assigned Coder has not opened a Merge Request.