From 1814c3570185ef17b8c8176751f64d45693fd3f0 Mon Sep 17 00:00:00 2001 From: Hare Date: Tue, 11 Aug 2026 19:57:03 +0900 Subject: [PATCH] feat: add merge request review authority --- .yoi/workflow/ticket-intake-workflow.md | 2 +- Cargo.lock | 12 + Cargo.toml | 3 + crates/merge-request/Cargo.toml | 14 + crates/merge-request/src/lib.rs | 1312 +++++++++++++++++ crates/merge-request/tests/store.rs | 404 +++++ crates/ticket/src/lib.rs | 165 +-- crates/ticket/src/sqlite_schema.rs | 67 +- crates/ticket/src/tool.rs | 95 +- crates/tui/src/dashboard/tests.rs | 8 +- crates/worker/src/feature/builtin.rs | 1 + .../src/feature/builtin/merge_request.rs | 299 ++++ crates/worker/src/feature/builtin/ticket.rs | 57 +- crates/worker/src/internal_worker.rs | 4 + crates/worker/src/spawn/tool.rs | 171 ++- crates/worker/src/worker.rs | 118 ++ crates/workspace-server/Cargo.toml | 1 + crates/workspace-server/src/lib.rs | 10 + crates/workspace-server/src/server.rs | 559 ++++++- crates/workspace-server/src/store.rs | 1 + crates/yoi/src/ticket_cli.rs | 138 +- docs/development/work-items.md | 12 +- resources/flows/coder-review.dcdl | 20 +- resources/profiles/reviewer.dcdl | 2 +- resources/prompts/role/coder.md | 8 +- resources/prompts/role/reviewer.md | 8 +- .../console/worker-console.ui.test.ts | 4 +- .../tickets/[ticketId]/+page.svelte | 80 +- .../[workspaceId]/tickets/[ticketId]/+page.ts | 49 +- 29 files changed, 3069 insertions(+), 555 deletions(-) create mode 100644 crates/merge-request/Cargo.toml create mode 100644 crates/merge-request/src/lib.rs create mode 100644 crates/merge-request/tests/store.rs create mode 100644 crates/worker/src/feature/builtin/merge_request.rs diff --git a/.yoi/workflow/ticket-intake-workflow.md b/.yoi/workflow/ticket-intake-workflow.md index e195c70b..f2f044e5 100644 --- a/.yoi/workflow/ticket-intake-workflow.md +++ b/.yoi/workflow/ticket-intake-workflow.md @@ -76,7 +76,7 @@ Intake は以下を行う。 - `TicketComment`: 既存 Ticket refinement / decision / plan の記録。 - `TicketDoctor`: 必要に応じた整合性確認。 -Intake は `TicketReview`, `TicketWorkflowState`, `TicketClose` を通常使わない。review / state transition / close は Orchestrator または reviewer / maintainer workflow の責務である。 +Intake は `MergeRequest*`, `TicketWorkflowState`, `TicketClose` を通常使わない。review authority は assigned Coder が起動した read-only direct-child Reviewer の immutable Merge Request attempt に属し、completion / merge / close は各guarded workflowの責務である。 Ticket tools が利用できない環境では、勝手に file write で代替しない。ユーザーまたは Orchestrator に「Ticket tools がないため materialize できない」と報告し、必要なら `yoi ticket` を使える人間/親 workflow に戻す。 diff --git a/Cargo.lock b/Cargo.lock index 14427abf..5b925ccb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2481,6 +2481,17 @@ dependencies = [ "uuid", ] +[[package]] +name = "merge-request" +version = "0.1.0" +dependencies = [ + "rusqlite", + "serde", + "sha2 0.11.0", + "tempfile", + "thiserror 2.0.18", +] + [[package]] name = "mime" version = "0.3.17" @@ -6164,6 +6175,7 @@ dependencies = [ "futures", "manifest", "memory", + "merge-request", "project-record", "protocol", "reqwest", diff --git a/Cargo.toml b/Cargo.toml index b7cec2ce..e8f55dcd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,6 +23,7 @@ members = [ "crates/tui", "crates/memory", "crates/ticket", + "crates/merge-request", "crates/project-record", "crates/workspace-server", "tests/e2e", @@ -50,6 +51,7 @@ default-members = [ "crates/tui", "crates/memory", "crates/ticket", + "crates/merge-request", "crates/project-record", "crates/workspace-server", ] @@ -67,6 +69,7 @@ manifest = { path = "crates/manifest" } mcp = { path = "crates/mcp" } lint-common = { path = "crates/lint-common" } memory = { path = "crates/memory" } +merge-request = { path = "crates/merge-request" } ticket = { path = "crates/ticket" } project-record = { path = "crates/project-record" } worker = { path = "crates/worker" } diff --git a/crates/merge-request/Cargo.toml b/crates/merge-request/Cargo.toml new file mode 100644 index 00000000..d5865a9c --- /dev/null +++ b/crates/merge-request/Cargo.toml @@ -0,0 +1,14 @@ +[package] +name = "merge-request" +version = "0.1.0" +edition.workspace = true +license.workspace = true + +[dependencies] +rusqlite.workspace = true +serde = { workspace = true, features = ["derive"] } +sha2.workspace = true +thiserror.workspace = true + +[dev-dependencies] +tempfile.workspace = true diff --git a/crates/merge-request/src/lib.rs b/crates/merge-request/src/lib.rs new file mode 100644 index 00000000..22844355 --- /dev/null +++ b/crates/merge-request/src/lib.rs @@ -0,0 +1,1312 @@ +//! Workspace-scoped Merge Request authority. +//! +//! Merge Requests deliberately do not reuse Ticket thread review events. A review is +//! evidence for one immutable revision and can only be committed with a one-shot +//! capability registered from an actual Runtime-owned direct-child reviewer session. + +use rusqlite::{Connection, OptionalExtension, params}; +use serde::{Deserialize, Serialize}; +use sha2::{Digest, Sha256}; +use std::path::{Path, PathBuf}; +use std::time::Duration; +use thiserror::Error; + +const SCHEMA_VERSION: i64 = 7; +const REVIEWER_PROFILE: &str = "builtin:reviewer"; +const MAX_SUMMARY_BYTES: usize = 16 * 1024; +const MAX_REVIEW_BODY_BYTES: usize = 64 * 1024; +const MAX_CHANGED_PATHS: usize = 1_000; +const MAX_FINDINGS: usize = 1_000; +const MAX_FIELD_BYTES: usize = 4 * 1024; + +pub type Result = std::result::Result; + +#[derive(Debug, Error)] +pub enum MergeRequestError { + #[error("merge request database error: {0}")] + Database(String), + #[error("{0} must not be empty")] + Empty(&'static str), + #[error("{field} exceeds its bounded limit of {max} bytes/items")] + TooLarge { field: &'static str, max: usize }, + #[error("merge request not found for ticket {0}")] + NotFound(String), + #[error("merge request already exists for ticket {0}")] + AlreadyExists(String), + #[error("immutable revision {0} already exists with different content")] + RevisionConflict(String), + #[error("stale merge request revision: expected {expected}, current {current}")] + StaleRevision { expected: String, current: String }, + #[error("current Ticket assignment does not match the authenticated Coder")] + AssignmentMismatch, + #[error("reviewer must be an actual direct-child with effective profile builtin:reviewer")] + InvalidReviewer, + #[error("review attempt is invalid, revoked, already used, or belongs to another revision")] + InvalidReviewAttempt, + #[error("review result cannot be supplied by the assigned Coder itself")] + SelfApproval, + #[error("merge request current revision is not approved")] + NotApproved, + #[error("merge request is {0}, expected open")] + NotOpen(String), + #[error("completion operation id was reused with different input")] + OperationConflict, + #[error("Ticket must be inprogress before Merge Request completion (current: {0})")] + TicketStateConflict(String), + #[error("only an authenticated user with explicit confirmation may merge")] + MergeConfirmationRequired, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum MergeRequestState { + Draft, + Open, + Closed, + Merged, +} + +impl MergeRequestState { + fn as_str(self) -> &'static str { + match self { + Self::Draft => "draft", + Self::Open => "open", + Self::Closed => "closed", + Self::Merged => "merged", + } + } + + fn parse(value: &str) -> Self { + match value { + "draft" => Self::Draft, + "closed" => Self::Closed, + "merged" => Self::Merged, + _ => Self::Open, + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ReviewDecision { + Approve, + RequestChanges, +} + +impl ReviewDecision { + fn as_str(self) -> &'static str { + match self { + Self::Approve => "approve", + Self::RequestChanges => "request_changes", + } + } + + fn parse(value: &str) -> Self { + match value { + "approve" => Self::Approve, + _ => Self::RequestChanges, + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ReviewStatus { + Pending, + Approved, + ChangesRequested, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct MergeRequestRevision { + pub revision_id: String, + pub ordinal: u64, + pub base_commit: String, + pub head_commit: String, + pub head_tree: String, + pub diff_digest: String, + pub changed_paths: Vec, + pub summary: String, + pub assignment_id: String, + pub created_at: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewFinding { + pub severity: String, + pub code: Option, + pub path: Option, + pub line: Option, + pub body: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct MergeRequestReview { + pub attempt_id: String, + pub revision_id: String, + pub decision: ReviewDecision, + pub body: String, + pub findings: Vec, + pub parent_assignment_id: String, + pub parent_runtime_id: String, + pub parent_worker_id: String, + pub reviewer_child_session_id: String, + pub reviewer_effective_profile: String, + pub submitted_at: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct MergeRequest { + pub merge_request_id: String, + pub workspace_id: String, + pub ticket_id: String, + pub repository_id: String, + pub state: MergeRequestState, + pub lifecycle_generation: u64, + pub current_revision: MergeRequestRevision, + pub review_status: ReviewStatus, + pub current_review: Option, + pub created_at: String, + pub updated_at: String, + pub merged_by_account_id: Option, + pub merged_at: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct OpenMergeRequest { + pub merge_request_id: String, + pub ticket_id: String, + pub repository_id: String, + pub revision: MergeRequestRevision, + pub authenticated_runtime_id: String, + pub authenticated_worker_id: String, + pub now: String, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AddRevision { + pub ticket_id: String, + pub expected_current_revision_id: String, + pub revision: MergeRequestRevision, + pub authenticated_runtime_id: String, + pub authenticated_worker_id: String, + pub now: String, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RegisterReviewerChildSession { + pub parent_runtime_id: String, + pub parent_worker_id: String, + pub child_session_id: String, + pub now: String, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RegisterReviewAttempt { + pub attempt_id: String, + pub ticket_id: String, + pub revision_id: String, + pub parent_assignment_id: String, + pub parent_runtime_id: String, + pub parent_worker_id: String, + pub child_session_id: String, + /// A secret generated by the trusted spawn layer and injected only into the child client. + pub capability_token: String, + pub now: String, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SubmitReview { + pub ticket_id: String, + pub revision_id: String, + pub capability_token: String, + pub decision: ReviewDecision, + pub body: String, + pub findings: Vec, + pub now: String, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct CompleteMergeRequest { + pub operation_id: String, + pub ticket_id: String, + pub expected_revision_id: String, + pub assignment_id: String, + pub authenticated_runtime_id: String, + pub authenticated_worker_id: String, + pub now: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct CompletionOutcome { + pub operation_id: String, + pub ticket_id: String, + pub revision_id: String, + pub ticket_state: String, + pub replayed: bool, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct MergeRequestReadiness { + pub ticket_id: String, + pub merge_request_id: String, + pub revision_id: String, + pub ready: bool, + pub review_status: ReviewStatus, + pub blockers: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct MergeConfirmation { + pub ticket_id: String, + pub expected_revision_id: String, + pub authenticated_account_id: String, + pub actor_kind: String, + pub explicit_confirmation: bool, + pub now: String, +} + +#[derive(Clone, Debug)] +pub struct SqliteMergeRequestStore { + db_path: PathBuf, + workspace_id: String, +} + +impl SqliteMergeRequestStore { + pub fn open(db_path: impl Into, workspace_id: impl Into) -> Result { + let store = Self { + db_path: db_path.into(), + workspace_id: workspace_id.into(), + }; + let conn = store.connect()?; + migrate(&conn)?; + Ok(store) + } + + pub fn open_verified( + db_path: impl Into, + workspace_id: impl Into, + ) -> Result { + let store = Self { + db_path: db_path.into(), + workspace_id: workspace_id.into(), + }; + verify(&store.connect()?)?; + Ok(store) + } + + pub fn db_path(&self) -> &Path { + &self.db_path + } + + pub fn workspace_id(&self) -> &str { + &self.workspace_id + } + + fn connect(&self) -> Result { + let conn = Connection::open(&self.db_path).map_err(db)?; + conn.busy_timeout(Duration::from_secs(5)).map_err(db)?; + conn.pragma_update(None, "foreign_keys", "ON").map_err(db)?; + Ok(conn) + } + + fn write(&self, op: impl FnOnce(&Connection) -> Result) -> Result { + let conn = self.connect()?; + verify(&conn)?; + conn.execute_batch("BEGIN IMMEDIATE").map_err(db)?; + match op(&conn) { + Ok(value) => { + conn.execute_batch("COMMIT").map_err(db)?; + Ok(value) + } + Err(error) => { + let _ = conn.execute_batch("ROLLBACK"); + Err(error) + } + } + } + + pub fn show_for_ticket(&self, ticket_id: &str) -> Result> { + nonempty("ticket_id", ticket_id)?; + let conn = self.connect()?; + verify(&conn)?; + load_merge_request(&conn, &self.workspace_id, ticket_id) + } + + pub fn readiness_for_ticket(&self, ticket_id: &str) -> Result { + let mr = self + .show_for_ticket(ticket_id)? + .ok_or_else(|| MergeRequestError::NotFound(ticket_id.to_string()))?; + let mut blockers = Vec::new(); + if mr.state != MergeRequestState::Open { + blockers.push(format!("merge request is {}", mr.state.as_str())); + } + match mr.review_status { + ReviewStatus::Pending => blockers.push("current revision has no review result".into()), + ReviewStatus::ChangesRequested => { + blockers.push("current revision has request_changes".into()) + } + ReviewStatus::Approved => {} + } + Ok(MergeRequestReadiness { + ticket_id: ticket_id.to_string(), + merge_request_id: mr.merge_request_id, + revision_id: mr.current_revision.revision_id, + ready: blockers.is_empty(), + review_status: mr.review_status, + blockers, + }) + } + + pub fn open_merge_request(&self, input: OpenMergeRequest) -> Result { + validate_revision(&input.revision)?; + for (name, value) in [ + ("merge_request_id", input.merge_request_id.as_str()), + ("ticket_id", input.ticket_id.as_str()), + ("repository_id", input.repository_id.as_str()), + ("runtime_id", input.authenticated_runtime_id.as_str()), + ("worker_id", input.authenticated_worker_id.as_str()), + ] { + nonempty(name, value)?; + } + self.write(|conn| { + validate_current_assignment( + conn, + &self.workspace_id, + &input.ticket_id, + &input.revision.assignment_id, + &input.authenticated_runtime_id, + &input.authenticated_worker_id, + )?; + conn.execute( + "INSERT INTO merge_requests (workspace_id, merge_request_id, repository_id, state, lifecycle_generation, current_revision_id, created_at, updated_at) VALUES (?1, ?2, ?3, 'open', 1, ?4, ?5, ?5)", + params![self.workspace_id, input.merge_request_id, input.repository_id, input.revision.revision_id, input.now], + ).map_err(db)?; + conn.execute( + "INSERT INTO merge_request_ticket_relations (workspace_id,merge_request_id,ticket_id,relation_kind,created_at) VALUES (?1,?2,?3,'implements',?4)", + params![self.workspace_id,input.merge_request_id,input.ticket_id,input.now], + ).map_err(db)?; + insert_revision(conn, &self.workspace_id, &input.merge_request_id, &input.revision)?; + load_merge_request(conn, &self.workspace_id, &input.ticket_id)?.ok_or_else(|| MergeRequestError::NotFound(input.ticket_id.clone())) + }) + } + + pub fn add_revision(&self, input: AddRevision) -> Result { + validate_revision(&input.revision)?; + self.write(|conn| { + let current = load_merge_request(conn, &self.workspace_id, &input.ticket_id)? + .ok_or_else(|| MergeRequestError::NotFound(input.ticket_id.clone()))?; + ensure_open(¤t)?; + if current.current_revision.revision_id != input.expected_current_revision_id { + return Err(MergeRequestError::StaleRevision { + expected: input.expected_current_revision_id.clone(), + current: current.current_revision.revision_id, + }); + } + validate_current_assignment( + conn, + &self.workspace_id, + &input.ticket_id, + &input.revision.assignment_id, + &input.authenticated_runtime_id, + &input.authenticated_worker_id, + )?; + if input.revision.ordinal != current.current_revision.ordinal + 1 { + return Err(MergeRequestError::RevisionConflict(input.revision.revision_id.clone())); + } + let existing: Option<(String, String, String, String)> = conn.query_row( + "SELECT base_commit, head_commit, head_tree, diff_digest FROM merge_request_revisions WHERE workspace_id=?1 AND merge_request_id=?2 AND revision_id=?3", + params![self.workspace_id, current.merge_request_id, input.revision.revision_id], + |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?, row.get(3)?)), + ).optional().map_err(db)?; + if let Some(existing) = existing { + if existing == (input.revision.base_commit.clone(), input.revision.head_commit.clone(), input.revision.head_tree.clone(), input.revision.diff_digest.clone()) { + return Ok(current); + } + return Err(MergeRequestError::RevisionConflict(input.revision.revision_id.clone())); + } + insert_revision(conn, &self.workspace_id, ¤t.merge_request_id, &input.revision)?; + conn.execute( + "UPDATE merge_requests SET current_revision_id=?3, updated_at=?4 WHERE workspace_id=?1 AND merge_request_id=?2 AND current_revision_id=?5", + params![self.workspace_id, current.merge_request_id, input.revision.revision_id, input.now, input.expected_current_revision_id], + ).map_err(db)?; + load_merge_request(conn, &self.workspace_id, &input.ticket_id)?.ok_or_else(|| MergeRequestError::NotFound(input.ticket_id.clone())) + }) + } + + pub fn register_reviewer_child_session( + &self, + input: RegisterReviewerChildSession, + ) -> Result<()> { + nonempty("runtime_id", &input.parent_runtime_id)?; + nonempty("worker_id", &input.parent_worker_id)?; + nonempty("child_session_id", &input.child_session_id)?; + self.write(|conn| { + conn.execute( + "INSERT INTO merge_request_reviewer_child_sessions (workspace_id,child_session_id,parent_runtime_id,parent_worker_id,effective_profile,registered_at) VALUES (?1,?2,?3,?4,'builtin:reviewer',?5)", + params![self.workspace_id,input.child_session_id,input.parent_runtime_id,input.parent_worker_id,input.now], + ).map_err(|_| MergeRequestError::InvalidReviewer)?; + Ok(()) + }) + } + + pub fn register_review_attempt(&self, input: RegisterReviewAttempt) -> Result<()> { + for (name, value) in [ + ("attempt_id", input.attempt_id.as_str()), + ("capability_token", input.capability_token.as_str()), + ("child_session_id", input.child_session_id.as_str()), + ] { + nonempty(name, value)?; + } + if input.child_session_id == input.parent_worker_id { + return Err(MergeRequestError::SelfApproval); + } + self.write(|conn| { + let mr = load_merge_request(conn, &self.workspace_id, &input.ticket_id)? + .ok_or_else(|| MergeRequestError::NotFound(input.ticket_id.clone()))?; + ensure_open(&mr)?; + if mr.current_revision.revision_id != input.revision_id { + return Err(MergeRequestError::StaleRevision { expected: input.revision_id.clone(), current: mr.current_revision.revision_id }); + } + validate_current_assignment(conn, &self.workspace_id, &input.ticket_id, &input.parent_assignment_id, &input.parent_runtime_id, &input.parent_worker_id)?; + let effective_profile: Option = conn.query_row( + "SELECT effective_profile FROM merge_request_reviewer_child_sessions WHERE workspace_id=?1 AND child_session_id=?2 AND parent_runtime_id=?3 AND parent_worker_id=?4", + params![self.workspace_id,input.child_session_id,input.parent_runtime_id,input.parent_worker_id], + |row| row.get(0), + ).optional().map_err(db)?; + if effective_profile.as_deref() != Some(REVIEWER_PROFILE) { + return Err(MergeRequestError::InvalidReviewer); + } + conn.execute( + "INSERT INTO merge_request_review_attempts (workspace_id, attempt_id, merge_request_id, ticket_id, revision_id, lifecycle_generation, parent_assignment_id, parent_runtime_id, parent_worker_id, child_session_id, child_effective_profile, capability_token_sha256, status, created_at) VALUES (?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,'open',?13)", + params![self.workspace_id, input.attempt_id, mr.merge_request_id, input.ticket_id, input.revision_id, mr.lifecycle_generation as i64, input.parent_assignment_id, input.parent_runtime_id, input.parent_worker_id, input.child_session_id, REVIEWER_PROFILE, token_hash(&input.capability_token), input.now], + ).map_err(|_| MergeRequestError::InvalidReviewAttempt)?; + Ok(()) + }) + } + + pub fn revoke_review_attempt( + &self, + attempt_id: &str, + child_session_id: &str, + now: &str, + ) -> Result { + self.write(|conn| { + let changed = conn.execute( + "UPDATE merge_request_review_attempts SET status='revoked', consumed_at=?4 WHERE workspace_id=?1 AND attempt_id=?2 AND child_session_id=?3 AND status='open'", + params![self.workspace_id, attempt_id, child_session_id, now], + ).map_err(db)?; + Ok(changed == 1) + }) + } + + pub fn submit_review(&self, input: SubmitReview) -> Result { + nonempty("capability_token", &input.capability_token)?; + validate_review_input(&input)?; + self.write(|conn| { + let token = token_hash(&input.capability_token); + let attempt: Option<(String,String,String,String,String,String,String,String,i64)> = conn.query_row( + "SELECT attempt_id, merge_request_id, parent_assignment_id, parent_runtime_id, parent_worker_id, child_session_id, child_effective_profile, status, lifecycle_generation FROM merge_request_review_attempts WHERE workspace_id=?1 AND ticket_id=?2 AND revision_id=?3 AND capability_token_sha256=?4", + params![self.workspace_id, input.ticket_id, input.revision_id, token], + |row| Ok((row.get(0)?,row.get(1)?,row.get(2)?,row.get(3)?,row.get(4)?,row.get(5)?,row.get(6)?,row.get(7)?,row.get(8)?)), + ).optional().map_err(db)?; + let Some((attempt_id, mr_id, assignment_id, runtime_id, worker_id, child_session_id, effective_profile, status, lifecycle_generation)) = attempt else { + return Err(MergeRequestError::InvalidReviewAttempt); + }; + if status != "open" || effective_profile != REVIEWER_PROFILE || child_session_id == worker_id { + return Err(MergeRequestError::InvalidReviewAttempt); + } + let mr = load_merge_request(conn, &self.workspace_id, &input.ticket_id)? + .ok_or_else(|| MergeRequestError::NotFound(input.ticket_id.clone()))?; + if lifecycle_generation != mr.lifecycle_generation as i64 { + return Err(MergeRequestError::InvalidReviewAttempt); + } + ensure_open(&mr)?; + if mr.current_revision.revision_id != input.revision_id { + return Err(MergeRequestError::StaleRevision { expected: input.revision_id.clone(), current: mr.current_revision.revision_id }); + } + validate_current_assignment(conn, &self.workspace_id, &input.ticket_id, &assignment_id, &runtime_id, &worker_id)?; + conn.execute( + "INSERT INTO merge_request_reviews (workspace_id, attempt_id, merge_request_id, revision_id, decision, body, submitted_at) VALUES (?1,?2,?3,?4,?5,?6,?7)", + params![self.workspace_id, attempt_id, mr_id, input.revision_id, input.decision.as_str(), input.body, input.now], + ).map_err(|_| MergeRequestError::InvalidReviewAttempt)?; + for (ordinal, finding) in input.findings.iter().enumerate() { + nonempty("finding.body", &finding.body)?; + conn.execute( + "INSERT INTO merge_request_review_findings (workspace_id, attempt_id, ordinal, severity, code, path, line, body) VALUES (?1,?2,?3,?4,?5,?6,?7,?8)", + params![self.workspace_id, attempt_id, ordinal as i64, finding.severity, finding.code, finding.path, finding.line.map(|v| v as i64), finding.body], + ).map_err(db)?; + } + conn.execute( + "UPDATE merge_request_review_attempts SET status='submitted', consumed_at=?3 WHERE workspace_id=?1 AND attempt_id=?2 AND status='open'", + params![self.workspace_id, attempt_id, input.now], + ).map_err(db)?; + load_review(conn, &self.workspace_id, &attempt_id)?.ok_or(MergeRequestError::InvalidReviewAttempt) + }) + } + + pub fn complete(&self, input: CompleteMergeRequest) -> Result { + for (name, value) in [ + ("operation_id", input.operation_id.as_str()), + ("ticket_id", input.ticket_id.as_str()), + ("revision_id", input.expected_revision_id.as_str()), + ] { + nonempty(name, value)?; + } + let fingerprint = completion_fingerprint(&input); + self.write(|conn| { + if let Some((stored, status, state)) = conn.query_row( + "SELECT fingerprint, status, result_ticket_state FROM merge_request_completion_operations WHERE workspace_id=?1 AND operation_id=?2", + params![self.workspace_id, input.operation_id], + |row| Ok((row.get::<_,String>(0)?, row.get::<_,String>(1)?, row.get::<_,Option>(2)?)), + ).optional().map_err(db)? { + if stored != fingerprint { return Err(MergeRequestError::OperationConflict); } + if status == "completed" { + return Ok(CompletionOutcome { operation_id: input.operation_id.clone(), ticket_id: input.ticket_id.clone(), revision_id: input.expected_revision_id.clone(), ticket_state: state.unwrap_or_else(|| "done".into()), replayed: true }); + } + } else { + conn.execute( + "INSERT INTO merge_request_completion_operations (workspace_id, operation_id, ticket_id, revision_id, assignment_id, fingerprint, status, created_at, updated_at) VALUES (?1,?2,?3,?4,?5,?6,'pending',?7,?7)", + params![self.workspace_id, input.operation_id, input.ticket_id, input.expected_revision_id, input.assignment_id, fingerprint, input.now], + ).map_err(db)?; + } + let mr = load_merge_request(conn, &self.workspace_id, &input.ticket_id)? + .ok_or_else(|| MergeRequestError::NotFound(input.ticket_id.clone()))?; + ensure_open(&mr)?; + if mr.current_revision.revision_id != input.expected_revision_id { + return Err(MergeRequestError::StaleRevision { expected: input.expected_revision_id.clone(), current: mr.current_revision.revision_id }); + } + validate_current_assignment(conn, &self.workspace_id, &input.ticket_id, &input.assignment_id, &input.authenticated_runtime_id, &input.authenticated_worker_id)?; + if mr.review_status != ReviewStatus::Approved { return Err(MergeRequestError::NotApproved); } + let current_state: String = conn.query_row( + "SELECT workflow_state FROM typed_tickets WHERE workspace_id=?1 AND ticket_id=?2", + params![self.workspace_id, input.ticket_id], |row| row.get(0), + ).optional().map_err(db)?.ok_or_else(|| MergeRequestError::NotFound(input.ticket_id.clone()))?; + if current_state != "inprogress" { + return Err(MergeRequestError::TicketStateConflict(current_state)); + } + let changed = conn.execute( + "UPDATE typed_tickets SET workflow_state='done', workflow_state_explicit=1, updated_at=?3 WHERE workspace_id=?1 AND ticket_id=?2 AND workflow_state='inprogress'", + params![self.workspace_id, input.ticket_id, input.now], + ).map_err(db)?; + if changed != 1 { return Err(MergeRequestError::TicketStateConflict("concurrent_change".into())); } + append_completion_event(conn, &self.workspace_id, &input)?; + conn.execute( + "UPDATE merge_request_completion_operations SET status='completed', result_ticket_state='done', updated_at=?3 WHERE workspace_id=?1 AND operation_id=?2 AND status='pending'", + params![self.workspace_id, input.operation_id, input.now], + ).map_err(db)?; + Ok(CompletionOutcome { operation_id: input.operation_id, ticket_id: input.ticket_id, revision_id: input.expected_revision_id, ticket_state: "done".into(), replayed: false }) + }) + } + + pub fn close( + &self, + ticket_id: &str, + expected_revision_id: &str, + now: &str, + ) -> Result { + self.transition_open(ticket_id, expected_revision_id, "closed", now) + } + + pub fn reopen( + &self, + ticket_id: &str, + expected_revision_id: &str, + now: &str, + ) -> Result { + self.write(|conn| { + let mr = load_merge_request(conn, &self.workspace_id, ticket_id)?.ok_or_else(|| MergeRequestError::NotFound(ticket_id.into()))?; + if mr.state != MergeRequestState::Closed { return Err(MergeRequestError::NotOpen(mr.state.as_str().into())); } + if mr.current_revision.revision_id != expected_revision_id { return Err(MergeRequestError::StaleRevision { expected: expected_revision_id.into(), current: mr.current_revision.revision_id }); } + conn.execute("UPDATE merge_requests SET state='open', lifecycle_generation=lifecycle_generation+1, updated_at=?3 WHERE workspace_id=?1 AND merge_request_id=?2 AND state='closed'", params![self.workspace_id, mr.merge_request_id, now]).map_err(db)?; + load_merge_request(conn, &self.workspace_id, ticket_id)?.ok_or_else(|| MergeRequestError::NotFound(ticket_id.into())) + }) + } + + pub fn confirm_merge(&self, input: MergeConfirmation) -> Result { + if !input.explicit_confirmation + || input.actor_kind != "user" + || input.authenticated_account_id.trim().is_empty() + { + return Err(MergeRequestError::MergeConfirmationRequired); + } + self.write(|conn| { + let mr = load_merge_request(conn, &self.workspace_id, &input.ticket_id)?.ok_or_else(|| MergeRequestError::NotFound(input.ticket_id.clone()))?; + ensure_open(&mr)?; + if mr.current_revision.revision_id != input.expected_revision_id { return Err(MergeRequestError::StaleRevision { expected: input.expected_revision_id.clone(), current: mr.current_revision.revision_id }); } + if mr.review_status != ReviewStatus::Approved { return Err(MergeRequestError::NotApproved); } + let ticket_state: String = conn.query_row("SELECT workflow_state FROM typed_tickets WHERE workspace_id=?1 AND ticket_id=?2", params![self.workspace_id, input.ticket_id], |row| row.get(0)).map_err(db)?; + if ticket_state != "done" { return Err(MergeRequestError::TicketStateConflict(ticket_state)); } + conn.execute("UPDATE merge_requests SET state='merged', merged_by_account_id=?3, merged_at=?4, updated_at=?4 WHERE workspace_id=?1 AND merge_request_id=?2 AND state='open'", params![self.workspace_id, mr.merge_request_id, input.authenticated_account_id, input.now]).map_err(db)?; + load_merge_request(conn, &self.workspace_id, &input.ticket_id)?.ok_or_else(|| MergeRequestError::NotFound(input.ticket_id.clone())) + }) + } + + fn transition_open( + &self, + ticket_id: &str, + expected_revision_id: &str, + state: &str, + now: &str, + ) -> Result { + self.write(|conn| { + let mr = load_merge_request(conn, &self.workspace_id, ticket_id)?.ok_or_else(|| MergeRequestError::NotFound(ticket_id.into()))?; + ensure_open(&mr)?; + if mr.current_revision.revision_id != expected_revision_id { return Err(MergeRequestError::StaleRevision { expected: expected_revision_id.into(), current: mr.current_revision.revision_id }); } + conn.execute("UPDATE merge_requests SET state=?3, updated_at=?4 WHERE workspace_id=?1 AND merge_request_id=?2 AND state='open'", params![self.workspace_id, mr.merge_request_id, state, now]).map_err(db)?; + load_merge_request(conn, &self.workspace_id, ticket_id)?.ok_or_else(|| MergeRequestError::NotFound(ticket_id.into())) + }) + } +} + +pub fn migrate(conn: &Connection) -> Result<()> { + conn.pragma_update(None, "foreign_keys", "ON").map_err(db)?; + conn.execute_batch("CREATE TABLE IF NOT EXISTS merge_request_schema_migrations (version INTEGER PRIMARY KEY, applied_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP);").map_err(db)?; + let version: i64 = conn + .query_row( + "SELECT COALESCE(MAX(version),0) FROM merge_request_schema_migrations", + [], + |row| row.get(0), + ) + .map_err(db)?; + archive_incompatible_legacy_tables(conn, version)?; + conn.execute_batch(SCHEMA_V1).map_err(db)?; + if version < 1 { + conn.execute( + "INSERT INTO merge_request_schema_migrations(version) VALUES (1)", + [], + ) + .map_err(db)?; + } + if version < SCHEMA_VERSION { + // Version 6 is the fresh bounded-context authority marker. Versions 1..=5 + // were emitted by the retired implementation; their relational evidence is + // preserved and revalidated by the current typed store rather than rewritten. + if column_exists(conn, "merge_request_schema_migrations", "name")? { + conn.execute( + "INSERT OR IGNORE INTO merge_request_schema_migrations(version,name) VALUES (?1,'fresh_bounded_context_authority')", + params![SCHEMA_VERSION], + ).map_err(db)?; + } else { + conn.execute( + "INSERT OR IGNORE INTO merge_request_schema_migrations(version) VALUES (?1)", + params![SCHEMA_VERSION], + ) + .map_err(db)?; + } + } + verify(conn) +} + +pub fn verify(conn: &Connection) -> Result<()> { + let version: i64 = conn + .query_row( + "SELECT COALESCE(MAX(version),0) FROM merge_request_schema_migrations", + [], + |row| row.get(0), + ) + .map_err(db)?; + if !(1..=SCHEMA_VERSION).contains(&version) { + return Err(MergeRequestError::Database(format!( + "unsupported merge request schema version {version}, expected at most {SCHEMA_VERSION}" + ))); + } + for table in [ + "merge_requests", + "merge_request_ticket_relations", + "merge_request_revisions", + "merge_request_revision_paths", + "merge_request_reviewer_child_sessions", + "merge_request_review_attempts", + "merge_request_reviews", + "merge_request_review_findings", + "merge_request_completion_operations", + ] { + let present: Option = conn + .query_row( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name=?1", + params![table], + |row| row.get(0), + ) + .optional() + .map_err(db)?; + if present.is_none() { + return Err(MergeRequestError::Database(format!( + "missing table {table}" + ))); + } + } + for (table, required) in [ + ( + "merge_requests", + &[ + "workspace_id", + "merge_request_id", + "repository_id", + "state", + "lifecycle_generation", + "current_revision_id", + ] as &[_], + ), + ( + "merge_request_ticket_relations", + &[ + "workspace_id", + "merge_request_id", + "ticket_id", + "relation_kind", + ] as &[_], + ), + ( + "merge_request_revisions", + &[ + "workspace_id", + "merge_request_id", + "revision_id", + "ordinal", + "base_commit", + "head_commit", + "head_tree", + "diff_digest", + "assignment_id", + ] as &[_], + ), + ( + "merge_request_reviewer_child_sessions", + &[ + "workspace_id", + "child_session_id", + "parent_runtime_id", + "parent_worker_id", + "effective_profile", + ] as &[_], + ), + ( + "merge_request_review_attempts", + &[ + "workspace_id", + "attempt_id", + "merge_request_id", + "ticket_id", + "revision_id", + "lifecycle_generation", + "parent_assignment_id", + "parent_runtime_id", + "parent_worker_id", + "child_session_id", + "child_effective_profile", + "capability_token_sha256", + "status", + ] as &[_], + ), + ( + "merge_request_reviews", + &[ + "workspace_id", + "attempt_id", + "merge_request_id", + "revision_id", + "decision", + "body", + ] as &[_], + ), + ( + "merge_request_completion_operations", + &[ + "workspace_id", + "operation_id", + "ticket_id", + "revision_id", + "assignment_id", + "fingerprint", + "status", + "result_ticket_state", + ] as &[_], + ), + ] { + let mut statement = conn + .prepare(&format!("PRAGMA table_info({table})")) + .map_err(db)?; + let columns = statement + .query_map([], |row| row.get::<_, String>(1)) + .map_err(db)? + .collect::, _>>() + .map_err(db)?; + for column in required { + if !columns.iter().any(|actual| actual == column) { + return Err(MergeRequestError::Database(format!( + "schema drift: table {table} is missing required column {column}" + ))); + } + } + } + Ok(()) +} + +const SCHEMA_V1: &str = r#" +CREATE TABLE IF NOT EXISTS merge_requests ( + workspace_id TEXT NOT NULL, merge_request_id TEXT NOT NULL, + repository_id TEXT NOT NULL, state TEXT NOT NULL CHECK(state IN ('draft','open','closed','merged')), + lifecycle_generation INTEGER NOT NULL, current_revision_id TEXT NOT NULL, + created_at TEXT NOT NULL, updated_at TEXT NOT NULL, merged_by_account_id TEXT, merged_at TEXT, + PRIMARY KEY(workspace_id,merge_request_id), + FOREIGN KEY(workspace_id,repository_id) REFERENCES repositories(workspace_id,repository_id) +); +CREATE TABLE IF NOT EXISTS merge_request_ticket_relations ( + workspace_id TEXT NOT NULL, merge_request_id TEXT NOT NULL, ticket_id TEXT NOT NULL, + relation_kind TEXT NOT NULL CHECK(relation_kind='implements'), created_at TEXT NOT NULL, + PRIMARY KEY(workspace_id,merge_request_id,ticket_id), + FOREIGN KEY(workspace_id,merge_request_id) REFERENCES merge_requests(workspace_id,merge_request_id) ON DELETE CASCADE, + FOREIGN KEY(workspace_id,ticket_id) REFERENCES typed_tickets(workspace_id,ticket_id) ON DELETE CASCADE +); +CREATE TABLE IF NOT EXISTS merge_request_revisions ( + workspace_id TEXT NOT NULL, merge_request_id TEXT NOT NULL, revision_id TEXT NOT NULL, + ordinal INTEGER NOT NULL, base_commit TEXT NOT NULL, head_commit TEXT NOT NULL, head_tree TEXT NOT NULL, diff_digest TEXT NOT NULL, + summary TEXT NOT NULL, assignment_id TEXT NOT NULL, created_at TEXT NOT NULL, + PRIMARY KEY(workspace_id,merge_request_id,revision_id), UNIQUE(workspace_id,merge_request_id,ordinal), + FOREIGN KEY(workspace_id,merge_request_id) REFERENCES merge_requests(workspace_id,merge_request_id) ON DELETE CASCADE +); +CREATE TABLE IF NOT EXISTS merge_request_revision_paths ( + workspace_id TEXT NOT NULL, merge_request_id TEXT NOT NULL, revision_id TEXT NOT NULL, ordinal INTEGER NOT NULL, path TEXT NOT NULL, + PRIMARY KEY(workspace_id,merge_request_id,revision_id,ordinal), + FOREIGN KEY(workspace_id,merge_request_id,revision_id) REFERENCES merge_request_revisions(workspace_id,merge_request_id,revision_id) ON DELETE CASCADE +); +CREATE TABLE IF NOT EXISTS merge_request_reviewer_child_sessions ( + workspace_id TEXT NOT NULL, child_session_id TEXT NOT NULL, parent_runtime_id TEXT NOT NULL, + parent_worker_id TEXT NOT NULL, effective_profile TEXT NOT NULL CHECK(effective_profile='builtin:reviewer'), registered_at TEXT NOT NULL, + PRIMARY KEY(workspace_id,child_session_id) +); +CREATE TABLE IF NOT EXISTS merge_request_review_attempts ( + workspace_id TEXT NOT NULL, attempt_id TEXT NOT NULL, merge_request_id TEXT NOT NULL, ticket_id TEXT NOT NULL, + revision_id TEXT NOT NULL, lifecycle_generation INTEGER NOT NULL, + parent_assignment_id TEXT NOT NULL, parent_runtime_id TEXT NOT NULL, parent_worker_id TEXT NOT NULL, + child_session_id TEXT NOT NULL, child_effective_profile TEXT NOT NULL CHECK(child_effective_profile='builtin:reviewer'), + capability_token_sha256 TEXT NOT NULL, status TEXT NOT NULL CHECK(status IN ('open','submitted','revoked')), + created_at TEXT NOT NULL, consumed_at TEXT, + PRIMARY KEY(workspace_id,attempt_id), UNIQUE(workspace_id,capability_token_sha256), UNIQUE(workspace_id,child_session_id), + FOREIGN KEY(workspace_id,merge_request_id,revision_id) REFERENCES merge_request_revisions(workspace_id,merge_request_id,revision_id), + FOREIGN KEY(workspace_id,ticket_id,parent_assignment_id) REFERENCES ticket_worker_assignments(workspace_id,ticket_id,assignment_id) +); +CREATE TABLE IF NOT EXISTS merge_request_reviews ( + workspace_id TEXT NOT NULL, attempt_id TEXT NOT NULL, merge_request_id TEXT NOT NULL, revision_id TEXT NOT NULL, + decision TEXT NOT NULL CHECK(decision IN ('approve','request_changes')), body TEXT NOT NULL, submitted_at TEXT NOT NULL, + PRIMARY KEY(workspace_id,attempt_id), + FOREIGN KEY(workspace_id,attempt_id) REFERENCES merge_request_review_attempts(workspace_id,attempt_id), + FOREIGN KEY(workspace_id,merge_request_id,revision_id) REFERENCES merge_request_revisions(workspace_id,merge_request_id,revision_id) +); +CREATE TABLE IF NOT EXISTS merge_request_review_findings ( + workspace_id TEXT NOT NULL, attempt_id TEXT NOT NULL, ordinal INTEGER NOT NULL, severity TEXT NOT NULL, + code TEXT, path TEXT, line INTEGER, body TEXT NOT NULL, PRIMARY KEY(workspace_id,attempt_id,ordinal), + FOREIGN KEY(workspace_id,attempt_id) REFERENCES merge_request_reviews(workspace_id,attempt_id) ON DELETE CASCADE +); +CREATE TABLE IF NOT EXISTS merge_request_completion_operations ( + workspace_id TEXT NOT NULL, operation_id TEXT NOT NULL, ticket_id TEXT NOT NULL, revision_id TEXT NOT NULL, + assignment_id TEXT NOT NULL, fingerprint TEXT NOT NULL, status TEXT NOT NULL CHECK(status IN ('pending','completed')), + result_ticket_state TEXT, created_at TEXT NOT NULL, updated_at TEXT NOT NULL, + PRIMARY KEY(workspace_id,operation_id), + FOREIGN KEY(workspace_id,ticket_id) REFERENCES typed_tickets(workspace_id,ticket_id) +); +"#; + +fn archive_incompatible_legacy_tables(conn: &Connection, version: i64) -> Result<()> { + if version == 0 || !table_exists(conn, "merge_requests")? { + return Ok(()); + } + let incompatible = column_exists(conn, "merge_requests", "ticket_id")? + || !table_has_columns( + conn, + "merge_requests", + &[ + "workspace_id", + "merge_request_id", + "repository_id", + "state", + "lifecycle_generation", + "current_revision_id", + ], + )? + || !table_has_columns( + conn, + "merge_request_ticket_relations", + &[ + "workspace_id", + "merge_request_id", + "ticket_id", + "relation_kind", + ], + )? + || !table_has_columns( + conn, + "merge_request_revisions", + &[ + "workspace_id", + "merge_request_id", + "revision_id", + "ordinal", + "base_commit", + "head_commit", + "head_tree", + "diff_digest", + "assignment_id", + ], + )?; + if !incompatible { + return Ok(()); + } + let tables = [ + "merge_request_review_findings", + "merge_request_reviews", + "merge_request_review_attempts", + "merge_request_reviewer_child_sessions", + "merge_request_completion_operations", + "merge_request_revision_paths", + "merge_request_ticket_relations", + "merge_request_revisions", + "merge_requests", + ]; + conn.pragma_update(None, "foreign_keys", "OFF") + .map_err(db)?; + for table in tables { + if !table_exists(conn, table)? { + continue; + } + let archive = format!("legacy_v6_{table}"); + if table_exists(conn, &archive)? { + conn.pragma_update(None, "foreign_keys", "ON").map_err(db)?; + return Err(MergeRequestError::Database(format!( + "legacy archive table {archive} already exists" + ))); + } + conn.execute_batch(&format!("ALTER TABLE {table} RENAME TO {archive};")) + .map_err(db)?; + } + conn.pragma_update(None, "foreign_keys", "ON").map_err(db)?; + Ok(()) +} + +fn table_has_columns(conn: &Connection, table: &str, required: &[&str]) -> Result { + if !table_exists(conn, table)? { + return Ok(false); + } + for column in required { + if !column_exists(conn, table, column)? { + return Ok(false); + } + } + Ok(true) +} + +fn table_exists(conn: &Connection, table: &str) -> Result { + let present: Option = conn + .query_row( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name=?1", + params![table], + |row| row.get(0), + ) + .optional() + .map_err(db)?; + Ok(present.is_some()) +} + +fn column_exists(conn: &Connection, table: &str, column: &str) -> Result { + let mut statement = conn + .prepare(&format!("PRAGMA table_info({table})")) + .map_err(db)?; + let names = statement + .query_map([], |row| row.get::<_, String>(1)) + .map_err(db)? + .collect::, _>>() + .map_err(db)?; + Ok(names.iter().any(|name| name == column)) +} + +fn load_merge_request( + conn: &Connection, + workspace_id: &str, + ticket_id: &str, +) -> Result> { + let row: Option<(String,String,String,String,i64,String,String,String,Option,Option)> = conn.query_row( + "SELECT mr.merge_request_id,rel.ticket_id,mr.repository_id,mr.state,mr.lifecycle_generation,mr.current_revision_id,mr.created_at,mr.updated_at,mr.merged_by_account_id,mr.merged_at FROM merge_requests mr JOIN merge_request_ticket_relations rel ON rel.workspace_id=mr.workspace_id AND rel.merge_request_id=mr.merge_request_id WHERE mr.workspace_id=?1 AND rel.ticket_id=?2 AND rel.relation_kind='implements' ORDER BY mr.updated_at DESC,mr.merge_request_id DESC LIMIT 1", + params![workspace_id,ticket_id], |r| Ok((r.get(0)?,r.get(1)?,r.get(2)?,r.get(3)?,r.get(4)?,r.get(5)?,r.get(6)?,r.get(7)?,r.get(8)?,r.get(9)?)), + ).optional().map_err(db)?; + let Some(( + mr_id, + ticket_id, + repository_id, + state, + generation, + revision_id, + created_at, + updated_at, + merged_by_account_id, + merged_at, + )) = row + else { + return Ok(None); + }; + let revision = load_revision(conn, workspace_id, &mr_id, &revision_id)?; + let current_review = load_latest_review(conn, workspace_id, &mr_id, &revision_id, generation)?; + let review_status = match current_review.as_ref().map(|review| review.decision) { + Some(ReviewDecision::Approve) => ReviewStatus::Approved, + Some(ReviewDecision::RequestChanges) => ReviewStatus::ChangesRequested, + None => ReviewStatus::Pending, + }; + Ok(Some(MergeRequest { + merge_request_id: mr_id, + workspace_id: workspace_id.into(), + ticket_id, + repository_id, + state: MergeRequestState::parse(&state), + lifecycle_generation: generation as u64, + current_revision: revision, + review_status, + current_review, + created_at, + updated_at, + merged_by_account_id, + merged_at, + })) +} + +fn load_revision( + conn: &Connection, + workspace_id: &str, + mr_id: &str, + revision_id: &str, +) -> Result { + let mut revision: MergeRequestRevision = conn.query_row( + "SELECT revision_id,ordinal,base_commit,head_commit,head_tree,diff_digest,summary,assignment_id,created_at FROM merge_request_revisions WHERE workspace_id=?1 AND merge_request_id=?2 AND revision_id=?3", + params![workspace_id,mr_id,revision_id], |r| Ok(MergeRequestRevision { revision_id:r.get(0)?, ordinal:r.get::<_,i64>(1)? as u64, base_commit:r.get(2)?, head_commit:r.get(3)?, head_tree:r.get(4)?, diff_digest:r.get(5)?, changed_paths:Vec::new(), summary:r.get(6)?, assignment_id:r.get(7)?, created_at:r.get(8)? }), + ).map_err(db)?; + let mut statement = conn.prepare("SELECT path FROM merge_request_revision_paths WHERE workspace_id=?1 AND merge_request_id=?2 AND revision_id=?3 ORDER BY ordinal").map_err(db)?; + revision.changed_paths = statement + .query_map(params![workspace_id, mr_id, revision_id], |r| r.get(0)) + .map_err(db)? + .collect::, _>>() + .map_err(db)?; + Ok(revision) +} + +fn insert_revision( + conn: &Connection, + workspace_id: &str, + mr_id: &str, + revision: &MergeRequestRevision, +) -> Result<()> { + conn.execute("INSERT INTO merge_request_revisions (workspace_id,merge_request_id,revision_id,ordinal,base_commit,head_commit,head_tree,diff_digest,summary,assignment_id,created_at) VALUES (?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11)", params![workspace_id,mr_id,revision.revision_id,revision.ordinal as i64,revision.base_commit,revision.head_commit,revision.head_tree,revision.diff_digest,revision.summary,revision.assignment_id,revision.created_at]).map_err(db)?; + for (ordinal, path) in revision.changed_paths.iter().enumerate() { + conn.execute("INSERT INTO merge_request_revision_paths (workspace_id,merge_request_id,revision_id,ordinal,path) VALUES (?1,?2,?3,?4,?5)", params![workspace_id,mr_id,revision.revision_id,ordinal as i64,path]).map_err(db)?; + } + Ok(()) +} + +fn load_latest_review( + conn: &Connection, + workspace_id: &str, + mr_id: &str, + revision_id: &str, + generation: i64, +) -> Result> { + let attempt: Option = conn.query_row("SELECT r.attempt_id FROM merge_request_reviews r JOIN merge_request_review_attempts a ON a.workspace_id=r.workspace_id AND a.attempt_id=r.attempt_id WHERE r.workspace_id=?1 AND r.merge_request_id=?2 AND r.revision_id=?3 AND a.lifecycle_generation=?4 ORDER BY r.submitted_at DESC, r.attempt_id DESC LIMIT 1", params![workspace_id,mr_id,revision_id,generation], |r| r.get(0)).optional().map_err(db)?; + match attempt { + Some(id) => load_review(conn, workspace_id, &id), + None => Ok(None), + } +} + +fn load_review( + conn: &Connection, + workspace_id: &str, + attempt_id: &str, +) -> Result> { + let row: Option<(String,String,String,String,String,String,String,String,String)> = conn.query_row( + "SELECT r.revision_id,r.decision,r.body,a.parent_assignment_id,a.parent_runtime_id,a.parent_worker_id,a.child_session_id,a.child_effective_profile,r.submitted_at FROM merge_request_reviews r JOIN merge_request_review_attempts a ON a.workspace_id=r.workspace_id AND a.attempt_id=r.attempt_id WHERE r.workspace_id=?1 AND r.attempt_id=?2", + params![workspace_id,attempt_id], |r| Ok((r.get(0)?,r.get(1)?,r.get(2)?,r.get(3)?,r.get(4)?,r.get(5)?,r.get(6)?,r.get(7)?,r.get(8)?)), + ).optional().map_err(db)?; + let Some(( + revision_id, + decision, + body, + assignment, + runtime, + worker, + child, + profile, + submitted_at, + )) = row + else { + return Ok(None); + }; + let mut stmt=conn.prepare("SELECT severity,code,path,line,body FROM merge_request_review_findings WHERE workspace_id=?1 AND attempt_id=?2 ORDER BY ordinal").map_err(db)?; + let findings = stmt + .query_map(params![workspace_id, attempt_id], |r| { + Ok(ReviewFinding { + severity: r.get(0)?, + code: r.get(1)?, + path: r.get(2)?, + line: r.get::<_, Option>(3)?.map(|v| v as u64), + body: r.get(4)?, + }) + }) + .map_err(db)? + .collect::, _>>() + .map_err(db)?; + Ok(Some(MergeRequestReview { + attempt_id: attempt_id.into(), + revision_id, + decision: ReviewDecision::parse(&decision), + body, + findings, + parent_assignment_id: assignment, + parent_runtime_id: runtime, + parent_worker_id: worker, + reviewer_child_session_id: child, + reviewer_effective_profile: profile, + submitted_at, + })) +} + +fn validate_current_assignment( + conn: &Connection, + workspace_id: &str, + ticket_id: &str, + assignment_id: &str, + runtime_id: &str, + worker_id: &str, +) -> Result<()> { + let valid: Option = conn.query_row("SELECT 1 FROM ticket_current_worker_assignments WHERE workspace_id=?1 AND ticket_id=?2 AND assignment_id=?3 AND runtime_id=?4 AND worker_id=?5", params![workspace_id,ticket_id,assignment_id,runtime_id,worker_id], |r| r.get(0)).optional().map_err(db)?; + if valid.is_none() { + return Err(MergeRequestError::AssignmentMismatch); + } + Ok(()) +} + +fn append_completion_event( + conn: &Connection, + workspace_id: &str, + input: &CompleteMergeRequest, +) -> Result<()> { + let index:i64=conn.query_row("SELECT COALESCE(MAX(event_index),-1)+1 FROM typed_ticket_events WHERE workspace_id=?1 AND ticket_id=?2",params![workspace_id,input.ticket_id],|r|r.get(0)).map_err(db)?; + conn.execute("INSERT INTO typed_ticket_events (workspace_id,ticket_id,event_index,kind,author,at,from_state,to_state,heading,body) VALUES (?1,?2,?3,'state_changed',?4,?5,'inprogress','done','Merge Request completed',?6)",params![workspace_id,input.ticket_id,index,format!("worker:{}:{}",input.authenticated_runtime_id,input.authenticated_worker_id),input.now,format!("Approved immutable revision `{}` completed implementation.",input.expected_revision_id)]).map_err(db)?; + for (key, value) in [ + ("assignment_id", input.assignment_id.as_str()), + ( + "merge_request_revision_id", + input.expected_revision_id.as_str(), + ), + ("operation_id", input.operation_id.as_str()), + ("runtime_id", input.authenticated_runtime_id.as_str()), + ("worker_id", input.authenticated_worker_id.as_str()), + ] { + conn.execute("INSERT INTO typed_ticket_event_attributes (workspace_id,ticket_id,event_index,key,value) VALUES (?1,?2,?3,?4,?5)",params![workspace_id,input.ticket_id,index,key,value]).map_err(db)?; + } + Ok(()) +} + +fn validate_revision(revision: &MergeRequestRevision) -> Result<()> { + for (name, value) in [ + ("revision_id", revision.revision_id.as_str()), + ("base_commit", revision.base_commit.as_str()), + ("head_commit", revision.head_commit.as_str()), + ("head_tree", revision.head_tree.as_str()), + ("diff_digest", revision.diff_digest.as_str()), + ("assignment_id", revision.assignment_id.as_str()), + ] { + nonempty(name, value)?; + } + if revision.ordinal == 0 { + return Err(MergeRequestError::Empty("revision.ordinal")); + } + if revision.summary.len() > MAX_SUMMARY_BYTES { + return Err(MergeRequestError::TooLarge { + field: "revision.summary", + max: MAX_SUMMARY_BYTES, + }); + } + if revision.changed_paths.len() > MAX_CHANGED_PATHS { + return Err(MergeRequestError::TooLarge { + field: "revision.changed_paths", + max: MAX_CHANGED_PATHS, + }); + } + for path in &revision.changed_paths { + nonempty("changed_path", path)?; + if path.len() > MAX_FIELD_BYTES { + return Err(MergeRequestError::TooLarge { + field: "changed_path", + max: MAX_FIELD_BYTES, + }); + } + if Path::new(path).is_absolute() || path.split('/').any(|p| p == "..") { + return Err(MergeRequestError::Empty("changed_path")); + } + } + Ok(()) +} + +fn validate_review_input(input: &SubmitReview) -> Result<()> { + if input.body.len() > MAX_REVIEW_BODY_BYTES { + return Err(MergeRequestError::TooLarge { + field: "review.body", + max: MAX_REVIEW_BODY_BYTES, + }); + } + if input.findings.len() > MAX_FINDINGS { + return Err(MergeRequestError::TooLarge { + field: "review.findings", + max: MAX_FINDINGS, + }); + } + for finding in &input.findings { + nonempty("finding.severity", &finding.severity)?; + nonempty("finding.body", &finding.body)?; + for (field, value) in [ + ("finding.severity", Some(finding.severity.as_str())), + ("finding.code", finding.code.as_deref()), + ("finding.path", finding.path.as_deref()), + ("finding.body", Some(finding.body.as_str())), + ] { + if value.is_some_and(|value| value.len() > MAX_FIELD_BYTES) { + return Err(MergeRequestError::TooLarge { + field, + max: MAX_FIELD_BYTES, + }); + } + } + } + Ok(()) +} + +fn ensure_open(mr: &MergeRequest) -> Result<()> { + if mr.state != MergeRequestState::Open { + Err(MergeRequestError::NotOpen(mr.state.as_str().into())) + } else { + Ok(()) + } +} +fn nonempty(name: &'static str, value: &str) -> Result<()> { + if value.trim().is_empty() { + Err(MergeRequestError::Empty(name)) + } else { + Ok(()) + } +} +fn token_hash(token: &str) -> String { + Sha256::digest(token.as_bytes()) + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() +} +fn completion_fingerprint(input: &CompleteMergeRequest) -> String { + token_hash(&format!( + "{}\0{}\0{}\0{}\0{}", + input.ticket_id, + input.expected_revision_id, + input.assignment_id, + input.authenticated_runtime_id, + input.authenticated_worker_id + )) +} +fn db(error: rusqlite::Error) -> MergeRequestError { + MergeRequestError::Database(error.to_string()) +} diff --git a/crates/merge-request/tests/store.rs b/crates/merge-request/tests/store.rs new file mode 100644 index 00000000..8763799c --- /dev/null +++ b/crates/merge-request/tests/store.rs @@ -0,0 +1,404 @@ +use merge_request::*; +use rusqlite::{Connection, params}; +use tempfile::TempDir; + +fn setup() -> (TempDir, SqliteMergeRequestStore) { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("server.db"); + let conn = Connection::open(&path).unwrap(); + conn.execute_batch(r#" + PRAGMA foreign_keys=ON; + CREATE TABLE repositories(workspace_id TEXT NOT NULL,repository_id TEXT NOT NULL,PRIMARY KEY(workspace_id,repository_id)); + CREATE TABLE typed_tickets(workspace_id TEXT NOT NULL,ticket_id TEXT NOT NULL,workflow_state TEXT NOT NULL,workflow_state_explicit INTEGER NOT NULL DEFAULT 1,updated_at TEXT NOT NULL,PRIMARY KEY(workspace_id,ticket_id)); + CREATE TABLE typed_ticket_events(workspace_id TEXT NOT NULL,ticket_id TEXT NOT NULL,event_index INTEGER NOT NULL,kind TEXT NOT NULL,author TEXT,at TEXT,status TEXT,from_state TEXT,to_state TEXT,heading TEXT,body TEXT,PRIMARY KEY(workspace_id,ticket_id,event_index)); + CREATE TABLE typed_ticket_event_attributes(workspace_id TEXT NOT NULL,ticket_id TEXT NOT NULL,event_index INTEGER NOT NULL,key TEXT NOT NULL,value TEXT NOT NULL,PRIMARY KEY(workspace_id,ticket_id,event_index,key)); + CREATE TABLE ticket_worker_assignments(workspace_id TEXT NOT NULL,ticket_id TEXT NOT NULL,assignment_id TEXT NOT NULL,runtime_id TEXT NOT NULL,worker_id TEXT NOT NULL,PRIMARY KEY(workspace_id,ticket_id,assignment_id)); + CREATE TABLE ticket_current_worker_assignments(workspace_id TEXT NOT NULL,ticket_id TEXT NOT NULL,assignment_id TEXT NOT NULL,runtime_id TEXT NOT NULL,worker_id TEXT NOT NULL,PRIMARY KEY(workspace_id,ticket_id)); + "#).unwrap(); + for ws in ["ws-a", "ws-b"] { + conn.execute("INSERT INTO repositories VALUES(?1,'repo')", params![ws]) + .unwrap(); + conn.execute( + "INSERT INTO typed_tickets VALUES(?1,'T1','inprogress',1,'t0')", + params![ws], + ) + .unwrap(); + conn.execute( + "INSERT INTO ticket_worker_assignments VALUES(?1,'T1','A1','R1','W1')", + params![ws], + ) + .unwrap(); + conn.execute( + "INSERT INTO ticket_current_worker_assignments VALUES(?1,'T1','A1','R1','W1')", + params![ws], + ) + .unwrap(); + } + drop(conn); + let store = SqliteMergeRequestStore::open(&path, "ws-a").unwrap(); + (dir, store) +} +fn revision(id: &str, ordinal: u64, head: &str) -> MergeRequestRevision { + MergeRequestRevision { + revision_id: id.into(), + ordinal, + base_commit: "base".into(), + head_commit: head.into(), + head_tree: format!("tree-{head}"), + diff_digest: format!("sha256:diff-{head}"), + changed_paths: vec!["src/lib.rs".into()], + summary: format!("revision {id}"), + assignment_id: "A1".into(), + created_at: format!("t{ordinal}"), + } +} +fn open(store: &SqliteMergeRequestStore) { + store + .open_merge_request(OpenMergeRequest { + merge_request_id: "MR1".into(), + ticket_id: "T1".into(), + repository_id: "repo".into(), + revision: revision("V1", 1, "h1"), + authenticated_runtime_id: "R1".into(), + authenticated_worker_id: "W1".into(), + now: "t1".into(), + }) + .unwrap(); +} +fn attempt(store: &SqliteMergeRequestStore, id: &str, revision: &str, token: &str, child: &str) { + store + .register_reviewer_child_session(RegisterReviewerChildSession { + parent_runtime_id: "R1".into(), + parent_worker_id: "W1".into(), + child_session_id: child.into(), + now: "t".into(), + }) + .unwrap(); + store + .register_review_attempt(RegisterReviewAttempt { + attempt_id: id.into(), + ticket_id: "T1".into(), + revision_id: revision.into(), + parent_assignment_id: "A1".into(), + parent_runtime_id: "R1".into(), + parent_worker_id: "W1".into(), + child_session_id: child.into(), + capability_token: token.into(), + now: "t".into(), + }) + .unwrap(); +} +fn review( + store: &SqliteMergeRequestStore, + revision: &str, + token: &str, + decision: ReviewDecision, +) -> Result { + store.submit_review(SubmitReview { + ticket_id: "T1".into(), + revision_id: revision.into(), + capability_token: token.into(), + decision, + body: "evidence".into(), + findings: vec![], + now: "tr".into(), + }) +} + +#[test] +fn storage_allows_multiple_merge_requests_for_one_ticket() { + let (_dir, store) = setup(); + open(&store); + store + .open_merge_request(OpenMergeRequest { + merge_request_id: "MR2".into(), + ticket_id: "T1".into(), + repository_id: "repo".into(), + revision: revision("V2", 1, "h2"), + authenticated_runtime_id: "R1".into(), + authenticated_worker_id: "W1".into(), + now: "t2".into(), + }) + .unwrap(); + let conn = Connection::open(store.db_path()).unwrap(); + let count:i64=conn.query_row("SELECT COUNT(*) FROM merge_request_ticket_relations WHERE workspace_id='ws-a' AND ticket_id='T1'",[],|row|row.get(0)).unwrap(); + assert_eq!(count, 2); + assert_eq!( + store + .show_for_ticket("T1") + .unwrap() + .unwrap() + .merge_request_id, + "MR2" + ); +} + +#[test] +fn bounded_context_rejects_oversized_revision_evidence() { + let (_dir, store) = setup(); + let mut oversized = revision("V1", 1, "h1"); + oversized.changed_paths = (0..=1_000).map(|i| format!("src/{i}.rs")).collect(); + let result = store.open_merge_request(OpenMergeRequest { + merge_request_id: "MR1".into(), + ticket_id: "T1".into(), + repository_id: "repo".into(), + revision: oversized, + authenticated_runtime_id: "R1".into(), + authenticated_worker_id: "W1".into(), + now: "t".into(), + }); + assert!(matches!( + result, + Err(MergeRequestError::TooLarge { + field: "revision.changed_paths", + .. + }) + )); +} + +#[test] +fn rejected_v6_schema_missing_diff_digest_is_archived_before_fresh_v7() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("legacy.db"); + let conn = Connection::open(&path).unwrap(); + conn.execute_batch( + "CREATE TABLE merge_request_schema_migrations(version INTEGER PRIMARY KEY,name TEXT NOT NULL,applied_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP);\ + INSERT INTO merge_request_schema_migrations(version,name) VALUES(6,'rejected_merge_request_v6');\ + CREATE TABLE repositories(workspace_id TEXT NOT NULL,repository_id TEXT NOT NULL,PRIMARY KEY(workspace_id,repository_id));\ + CREATE TABLE typed_tickets(workspace_id TEXT NOT NULL,ticket_id TEXT NOT NULL,workflow_state TEXT NOT NULL,workflow_state_explicit INTEGER NOT NULL DEFAULT 1,updated_at TEXT NOT NULL,PRIMARY KEY(workspace_id,ticket_id));\ + CREATE TABLE ticket_worker_assignments(workspace_id TEXT NOT NULL,ticket_id TEXT NOT NULL,assignment_id TEXT NOT NULL,runtime_id TEXT NOT NULL,worker_id TEXT NOT NULL,PRIMARY KEY(workspace_id,ticket_id,assignment_id));\ + CREATE TABLE merge_requests(workspace_id TEXT NOT NULL,merge_request_id TEXT NOT NULL,repository_id TEXT NOT NULL,state TEXT NOT NULL,lifecycle_generation INTEGER NOT NULL,current_revision_id TEXT NOT NULL,created_at TEXT NOT NULL,updated_at TEXT NOT NULL,PRIMARY KEY(workspace_id,merge_request_id));\ + CREATE TABLE merge_request_ticket_relations(workspace_id TEXT NOT NULL,merge_request_id TEXT NOT NULL,ticket_id TEXT NOT NULL,relation_kind TEXT NOT NULL,created_at TEXT NOT NULL,PRIMARY KEY(workspace_id,merge_request_id,ticket_id));\ + CREATE TABLE merge_request_revisions(workspace_id TEXT NOT NULL,merge_request_id TEXT NOT NULL,revision_id TEXT NOT NULL,ordinal INTEGER NOT NULL,base_commit TEXT NOT NULL,head_commit TEXT NOT NULL,head_tree TEXT NOT NULL,assignment_id TEXT NOT NULL,created_at TEXT NOT NULL,PRIMARY KEY(workspace_id,merge_request_id,revision_id));", + ).unwrap(); + drop(conn); + let store = SqliteMergeRequestStore::open(&path, "ws-a").unwrap(); + assert!(store.show_for_ticket("missing").unwrap().is_none()); + let conn = Connection::open(&path).unwrap(); + let archived: i64 = conn.query_row("SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='legacy_v6_merge_requests'",[],|row|row.get(0)).unwrap(); + assert_eq!(archived, 1); + for table in [ + "merge_request_review_attempts", + "merge_request_completion_operations", + ] { + let present: i64 = conn + .query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name=?1", + params![table], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(present, 1); + } +} + +#[test] +fn request_changes_new_revision_resets_and_exact_completion_replay_converges() { + let (_dir, store) = setup(); + open(&store); + attempt(&store, "AT1", "V1", "tok1", "child1"); + review(&store, "V1", "tok1", ReviewDecision::RequestChanges).unwrap(); + assert_eq!( + store.show_for_ticket("T1").unwrap().unwrap().review_status, + ReviewStatus::ChangesRequested + ); + store + .add_revision(AddRevision { + ticket_id: "T1".into(), + expected_current_revision_id: "V1".into(), + revision: revision("V2", 2, "h2"), + authenticated_runtime_id: "R1".into(), + authenticated_worker_id: "W1".into(), + now: "t2".into(), + }) + .unwrap(); + assert_eq!( + store.show_for_ticket("T1").unwrap().unwrap().review_status, + ReviewStatus::Pending + ); + assert!(review(&store, "V1", "tok1", ReviewDecision::Approve).is_err()); + attempt(&store, "AT2", "V2", "tok2", "child2"); + review(&store, "V2", "tok2", ReviewDecision::Approve).unwrap(); + let input = CompleteMergeRequest { + operation_id: "OP1".into(), + ticket_id: "T1".into(), + expected_revision_id: "V2".into(), + assignment_id: "A1".into(), + authenticated_runtime_id: "R1".into(), + authenticated_worker_id: "W1".into(), + now: "tc".into(), + }; + let first = store.complete(input.clone()).unwrap(); + assert!(!first.replayed); + let replay = store.complete(input).unwrap(); + assert!(replay.replayed); + assert!(matches!( + store.confirm_merge(MergeConfirmation { + ticket_id: "T1".into(), + expected_revision_id: "V2".into(), + authenticated_account_id: "runtime".into(), + actor_kind: "worker".into(), + explicit_confirmation: true, + now: "tm".into() + }), + Err(MergeRequestError::MergeConfirmationRequired) + )); + let merged = store + .confirm_merge(MergeConfirmation { + ticket_id: "T1".into(), + expected_revision_id: "V2".into(), + authenticated_account_id: "account-1".into(), + actor_kind: "user".into(), + explicit_confirmation: true, + now: "tm".into(), + }) + .unwrap(); + assert_eq!(merged.state, MergeRequestState::Merged); + let conn = Connection::open(store.db_path()).unwrap(); + assert_eq!( + conn.query_row( + "SELECT workflow_state FROM typed_tickets WHERE workspace_id='ws-a' AND ticket_id='T1'", + [], + |r| r.get::<_, String>(0) + ) + .unwrap(), + "done" + ); + assert_eq!( + conn.query_row( + "SELECT COUNT(*) FROM typed_ticket_events WHERE workspace_id='ws-a' AND ticket_id='T1'", + [], + |r| r.get::<_, i64>(0) + ) + .unwrap(), + 1 + ); +} + +#[test] +fn spoof_self_approval_replay_and_cross_workspace_are_rejected() { + let (_dir, store) = setup(); + open(&store); + let mut bad = RegisterReviewAttempt { + attempt_id: "bad".into(), + ticket_id: "T1".into(), + revision_id: "V1".into(), + parent_assignment_id: "A1".into(), + parent_runtime_id: "R1".into(), + parent_worker_id: "W1".into(), + child_session_id: "W1".into(), + capability_token: "bad".into(), + now: "t".into(), + }; + assert!(matches!( + store.register_review_attempt(bad.clone()), + Err(MergeRequestError::SelfApproval) + )); + bad.child_session_id = "child".into(); + assert!(matches!( + store.register_review_attempt(bad), + Err(MergeRequestError::InvalidReviewer) + )); + attempt(&store, "AT", "V1", "secret", "child"); + assert!(review(&store, "V1", "spoof", ReviewDecision::Approve).is_err()); + review(&store, "V1", "secret", ReviewDecision::Approve).unwrap(); + assert!(review(&store, "V1", "secret", ReviewDecision::Approve).is_err()); + let other = SqliteMergeRequestStore::open_verified(store.db_path(), "ws-b").unwrap(); + assert!(other.show_for_ticket("T1").unwrap().is_none()); +} + +#[test] +fn reopen_resets_approval_and_merge_requires_authenticated_explicit_user() { + let (_dir, store) = setup(); + open(&store); + attempt(&store, "AT", "V1", "token", "child"); + review(&store, "V1", "token", ReviewDecision::Approve).unwrap(); + store.close("T1", "V1", "tc").unwrap(); + let reopened = store.reopen("T1", "V1", "tr").unwrap(); + assert_eq!(reopened.review_status, ReviewStatus::Pending); + let denied = store.confirm_merge(MergeConfirmation { + ticket_id: "T1".into(), + expected_revision_id: "V1".into(), + authenticated_account_id: "user".into(), + actor_kind: "user".into(), + explicit_confirmation: false, + now: "tm".into(), + }); + assert!(matches!( + denied, + Err(MergeRequestError::MergeConfirmationRequired) + )); +} + +#[test] +fn concurrent_exact_completion_replays_commit_one_ticket_side_effect() { + let (_dir, store) = setup(); + open(&store); + attempt(&store, "AT", "V1", "token", "child"); + review(&store, "V1", "token", ReviewDecision::Approve).unwrap(); + let input = CompleteMergeRequest { + operation_id: "OP-concurrent".into(), + ticket_id: "T1".into(), + expected_revision_id: "V1".into(), + assignment_id: "A1".into(), + authenticated_runtime_id: "R1".into(), + authenticated_worker_id: "W1".into(), + now: "t".into(), + }; + let left_store = store.clone(); + let left_input = input.clone(); + let left = std::thread::spawn(move || left_store.complete(left_input)); + let right_store = store.clone(); + let right = std::thread::spawn(move || right_store.complete(input)); + let outcomes = [ + left.join().unwrap().unwrap(), + right.join().unwrap().unwrap(), + ]; + assert_eq!( + outcomes.iter().filter(|outcome| !outcome.replayed).count(), + 1 + ); + assert_eq!( + outcomes.iter().filter(|outcome| outcome.replayed).count(), + 1 + ); + let conn = Connection::open(store.db_path()).unwrap(); + let events: i64 = conn + .query_row( + "SELECT COUNT(*) FROM typed_ticket_events WHERE workspace_id='ws-a' AND ticket_id='T1'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(events, 1); +} + +#[test] +fn operation_key_mismatch_and_assignment_takeover_are_fenced() { + let (_dir, store) = setup(); + open(&store); + attempt(&store, "AT", "V1", "token", "child"); + review(&store, "V1", "token", ReviewDecision::Approve).unwrap(); + let mut input = CompleteMergeRequest { + operation_id: "OP".into(), + ticket_id: "T1".into(), + expected_revision_id: "V1".into(), + assignment_id: "A1".into(), + authenticated_runtime_id: "R1".into(), + authenticated_worker_id: "W1".into(), + now: "t".into(), + }; + let conn = Connection::open(store.db_path()).unwrap(); + conn.execute("UPDATE ticket_current_worker_assignments SET assignment_id='A2',runtime_id='R2',worker_id='W2' WHERE workspace_id='ws-a' AND ticket_id='T1'",[]).unwrap(); + assert!(matches!( + store.complete(input.clone()), + Err(MergeRequestError::AssignmentMismatch) + )); + conn.execute("UPDATE ticket_current_worker_assignments SET assignment_id='A1',runtime_id='R1',worker_id='W1' WHERE workspace_id='ws-a' AND ticket_id='T1'",[]).unwrap(); + store.complete(input.clone()).unwrap(); + input.expected_revision_id = "other".into(); + assert!(matches!( + store.complete(input), + Err(MergeRequestError::OperationConflict) + )); +} diff --git a/crates/ticket/src/lib.rs b/crates/ticket/src/lib.rs index 29f6f244..cde48ef7 100644 --- a/crates/ticket/src/lib.rs +++ b/crates/ticket/src/lib.rs @@ -295,7 +295,6 @@ pub enum TicketEventKind { Plan, Decision, ImplementationReport, - Review, StateChanged, IntakeSummary, StatusChanged, @@ -311,7 +310,6 @@ impl TicketEventKind { Self::Plan => "plan", Self::Decision => "decision", Self::ImplementationReport => "implementation_report", - Self::Review => "review", Self::StateChanged => "state_changed", Self::IntakeSummary => "intake_summary", Self::StatusChanged => "status_changed", @@ -327,7 +325,6 @@ impl TicketEventKind { Self::Plan => "Plan".to_string(), Self::Decision => "Decision".to_string(), Self::ImplementationReport => "Implementation report".to_string(), - Self::Review => "Review".to_string(), Self::StateChanged => "State changed".to_string(), Self::IntakeSummary => "Intake summary".to_string(), Self::StatusChanged => "Status changed".to_string(), @@ -345,7 +342,7 @@ impl From<&str> for TicketEventKind { "plan" => Self::Plan, "decision" => Self::Decision, "implementation_report" => Self::ImplementationReport, - "review" => Self::Review, + "review" => Self::Comment, "state_changed" => Self::StateChanged, "intake_summary" => Self::IntakeSummary, "status_changed" => Self::StatusChanged, @@ -355,42 +352,6 @@ impl From<&str> for TicketEventKind { } } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "snake_case")] -pub enum TicketReviewResult { - Approve, - RequestChanges, - Other(String), -} - -impl TicketReviewResult { - pub fn as_str(&self) -> &str { - match self { - Self::Approve => "approve", - Self::RequestChanges => "request_changes", - Self::Other(value) => value.as_str(), - } - } - - fn heading(&self) -> String { - match self { - Self::Approve => "Review: approve".to_string(), - Self::RequestChanges => "Review: request changes".to_string(), - Self::Other(value) => format!("Review: {value}"), - } - } -} - -impl From<&str> for TicketReviewResult { - fn from(value: &str) -> Self { - match value { - "approve" => Self::Approve, - "request_changes" => Self::RequestChanges, - other => Self::Other(other.to_string()), - } - } -} - #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct TicketReference { pub kind: String, @@ -461,31 +422,6 @@ impl TicketIntakeSummary { } } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct TicketReview { - pub result: TicketReviewResult, - pub author: Option, - pub body: MarkdownText, -} - -impl TicketReview { - pub fn approve(body: impl Into) -> Self { - Self { - result: TicketReviewResult::Approve, - author: None, - body: body.into(), - } - } - - pub fn request_changes(body: impl Into) -> Self { - Self { - result: TicketReviewResult::RequestChanges, - author: None, - body: body.into(), - } - } -} - #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct NewTicket { pub title: String, @@ -1578,7 +1514,6 @@ pub trait TicketBackend { change: TicketStateChange, ) -> Result<()>; fn queue_ready(&self, id: TicketIdOrSlug, queued_by: &str) -> Result<()>; - fn review(&self, id: TicketIdOrSlug, review: TicketReview) -> Result<()>; fn close(&self, id: TicketIdOrSlug, resolution: MarkdownText) -> Result<()>; fn add_ticket_relation( &self, @@ -1656,10 +1591,6 @@ pub enum TicketBackendOperation { id: TicketIdOrSlug, queued_by: String, }, - Review { - id: TicketIdOrSlug, - review: TicketReview, - }, Close { id: TicketIdOrSlug, resolution: MarkdownText, @@ -1763,10 +1694,6 @@ where backend.queue_ready(id, &queued_by)?; TicketBackendOperationResult::Unit } - TicketBackendOperation::Review { id, review } => { - backend.review(id, review)?; - TicketBackendOperationResult::Unit - } TicketBackendOperation::Close { id, resolution } => { backend.close(id, resolution)?; TicketBackendOperationResult::Unit @@ -3201,34 +3128,6 @@ impl TicketBackend for SqliteTicketBackend { }) } - fn review(&self, id: TicketIdOrSlug, review: TicketReview) -> Result<()> { - self.with_write(|conn| { - let ticket_id = self.resolve_ticket_id(conn, id)?; - let at = now_utc(); - let mut attributes = BTreeMap::new(); - attributes.insert("result".to_string(), review.result.as_str().to_string()); - self.insert_event( - conn, - &ticket_id, - &TicketEvent { - kind: TicketEventKind::Review, - author: Some(review.author.unwrap_or_else(default_author)), - at: Some(at.clone()), - status: Some(review.result.as_str().to_string()), - from: None, - to: None, - reason: None, - state_field: None, - heading: Some(review.result.heading()), - body: review.body, - references: Vec::new(), - attributes, - }, - )?; - self.touch_ticket(conn, &ticket_id, &at) - }) - } - fn close(&self, id: TicketIdOrSlug, resolution: MarkdownText) -> Result<()> { self.with_write(|conn| { let ticket_id = self.resolve_ticket_id(conn, id)?; @@ -3804,21 +3703,6 @@ impl TicketBackend for LocalTicketBackend { ) } - fn review(&self, id: TicketIdOrSlug, review: TicketReview) -> Result<()> { - let _lock = self.acquire_lock()?; - let dir = self.find_ticket_dir(&id)?; - let author = review.author.unwrap_or_else(default_author); - self.append_thread_event( - &dir, - "review", - &review.result.heading(), - &author, - Some(review.result.as_str()), - &[], - &review.body, - ) - } - fn close(&self, id: TicketIdOrSlug, resolution: MarkdownText) -> Result<()> { let _lock = self.acquire_lock()?; self.ensure_backend_dirs()?; @@ -5337,7 +5221,8 @@ fn parse_thread(path: &Path) -> Result> { .strip_prefix("")) { - let attrs = parse_event_comment(comment); + let mut attrs = parse_event_comment(comment); + let legacy_review = attrs.get("event").is_some_and(|value| value == "review"); let kind = attrs .get("event") .map(|value| TicketEventKind::from(value.as_str())) @@ -5369,11 +5254,22 @@ fn parse_thread(path: &Path) -> Result> { while body.ends_with('\n') { body.pop(); } + if legacy_review { + heading = Some("Legacy review (non-authoritative)".to_string()); + attrs.remove("status"); + attrs.remove("result"); + attrs.insert("event".to_string(), "comment".to_string()); + attrs.insert("legacy_event_kind".to_string(), "review".to_string()); + } events.push(TicketEvent { kind, author: attrs.get("author").cloned(), at: attrs.get("at").cloned(), - status: attrs.get("status").cloned(), + status: if legacy_review { + None + } else { + attrs.get("status").cloned() + }, from: attrs.get("from").cloned(), to: attrs.get("to").cloned(), reason: attrs.get("reason").cloned(), @@ -6379,12 +6275,6 @@ state: planning NewTicketEvent::new(TicketEventKind::Comment, "Imported into SQLite."), ) .unwrap(); - backend - .review( - TicketIdOrSlug::Id(created.id.clone()), - TicketReview::approve("Looks good."), - ) - .unwrap(); backend .close( TicketIdOrSlug::Id(created.id.clone()), @@ -6404,13 +6294,6 @@ state: planning assert!(ticket.events.iter().any(|event| { event.kind == TicketEventKind::Comment && event.body.0.contains("Imported into SQLite") })); - assert!( - ticket - .events - .iter() - .any(|event| event.kind == TicketEventKind::Review - && event.body.0.contains("Looks good")) - ); assert!( ticket .resolution @@ -6524,7 +6407,7 @@ state: planning } #[test] - fn add_event_review_status_and_close_preserve_local_layout() { + fn add_event_status_and_close_preserve_local_layout() { let tmp = TempDir::new().unwrap(); let backend = backend(&tmp); let ticket = backend.create(NewTicket::new("Flow Ticket")).unwrap(); @@ -6534,12 +6417,6 @@ state: planning NewTicketEvent::new(TicketEventKind::Plan, "Implementation plan."), ) .unwrap(); - backend - .review( - TicketIdOrSlug::Id(ticket.id.clone()), - TicketReview::approve("Looks good."), - ) - .unwrap(); let mut summary = TicketIntakeSummary::new("Ready for queue."); summary.author = Some("test".to_string()); let mut change = TicketStateChange::new( @@ -6563,8 +6440,6 @@ state: planning let closed_dir = tmp.path().join("tickets").join(&ticket.id); assert!(closed_dir.join("resolution.md").exists()); let thread = fs::read_to_string(closed_dir.join("thread.md")).unwrap(); - assert!(thread.contains("author".into()); - assert!(matches!( - backend.review(TicketIdOrSlug::Id(ticket.id.clone()), review), - Err(TicketError::Conflict(_)) - )); - assert_eq!(fs::read_to_string(&thread_path).unwrap(), original); - let invalid_kind = NewTicketEvent::new( TicketEventKind::Other("bad\nevent".into()), "Invalid event kind.", diff --git a/crates/ticket/src/sqlite_schema.rs b/crates/ticket/src/sqlite_schema.rs index 87536232..4f876294 100644 --- a/crates/ticket/src/sqlite_schema.rs +++ b/crates/ticket/src/sqlite_schema.rs @@ -7,7 +7,7 @@ use crate::{Result, TicketError, sqlite_err}; const MIGRATION_TABLE: &str = "ticket_schema_migrations"; const MAX_SCHEMA_DIAGNOSTICS: usize = 32; -pub const LATEST_SQLITE_TICKET_SCHEMA_VERSION: i64 = 2; +pub const LATEST_SQLITE_TICKET_SCHEMA_VERSION: i64 = 3; #[derive(Clone, Copy)] struct Migration { @@ -27,6 +27,11 @@ const MIGRATIONS: &[Migration] = &[ name: "add_ticket_repository_target", apply: add_ticket_repository_target, }, + Migration { + version: 3, + name: "convert_legacy_reviews_to_comments", + apply: retire_legacy_ticket_review_events, + }, ]; #[derive(Clone, Copy)] @@ -475,6 +480,33 @@ fn add_ticket_repository_target(connection: &Connection) -> Result<()> { add_column_if_missing(connection, "typed_tickets", "ref_selector", "TEXT") } +fn retire_legacy_ticket_review_events(connection: &Connection) -> Result<()> { + // Historical prose remains visible for audit, but it is explicitly converted to a + // non-authoritative comment. Approval authority now lives only in Merge Requests. + connection + .execute_batch( + r#" + INSERT OR REPLACE INTO typed_ticket_event_attributes + (workspace_id, ticket_id, event_index, key, value) + SELECT workspace_id, ticket_id, event_index, 'legacy_event_kind', 'review' + FROM typed_ticket_events WHERE kind = 'review'; + UPDATE typed_ticket_events + SET kind = 'comment', status = NULL, heading = 'Legacy review (non-authoritative)' + WHERE kind = 'review'; + DELETE FROM typed_ticket_event_attributes + WHERE key IN ('result', 'review_result', 'status') + AND EXISTS ( + SELECT 1 FROM typed_ticket_events event + WHERE event.workspace_id = typed_ticket_event_attributes.workspace_id + AND event.ticket_id = typed_ticket_event_attributes.ticket_id + AND event.event_index = typed_ticket_event_attributes.event_index + AND event.heading = 'Legacy review (non-authoritative)' + ); + "#, + ) + .map_err(sqlite_err) +} + fn add_column_if_missing( connection: &Connection, table: &str, @@ -809,10 +841,10 @@ mod tests { verify_sqlite_ticket_schema(&connection).unwrap(); let versions = load_applied_migrations(&connection).unwrap(); - assert_eq!(versions.len(), 2); + assert_eq!(versions.len(), 3); assert_eq!( versions.get(&LATEST_SQLITE_TICKET_SCHEMA_VERSION), - Some(&"add_ticket_repository_target".to_string()) + Some(&"convert_legacy_reviews_to_comments".to_string()) ); } @@ -957,7 +989,7 @@ mod tests { .to_string() .contains("unsupported Ticket schema migration version 99") ); - assert_eq!(load_applied_migrations(&connection).unwrap().len(), 3); + assert_eq!(load_applied_migrations(&connection).unwrap().len(), 4); } #[test] @@ -1050,6 +1082,31 @@ mod tests { assert!(!migration_table_exists); } + #[test] + fn legacy_review_upgrade_preserves_prose_as_non_authoritative_comment() { + let connection = Connection::open_in_memory().unwrap(); + migrate_sqlite_ticket_schema(&connection).unwrap(); + connection.execute("INSERT INTO typed_tickets (workspace_id,ticket_id,slug,title,status,kind,priority,body,workflow_state,workflow_state_explicit) VALUES ('workspace-1','ticket-1','ticket-1','title','open','task','medium','body','inprogress',1)",[]).unwrap(); + connection.execute("INSERT INTO typed_ticket_events (workspace_id,ticket_id,event_index,kind,author,at,status,heading,body) VALUES ('workspace-1','ticket-1',0,'review','reviewer','2026-08-11T00:00:00Z','approve','Review','legacy evidence')",[]).unwrap(); + connection.execute("INSERT INTO typed_ticket_event_attributes (workspace_id,ticket_id,event_index,key,value) VALUES ('workspace-1','ticket-1',0,'result','approve')",[]).unwrap(); + connection + .execute("DELETE FROM ticket_schema_migrations WHERE version=3", []) + .unwrap(); + migrate_sqlite_ticket_schema(&connection).unwrap(); + let (kind,status,heading,body):(String,Option,Option,Option)=connection.query_row("SELECT kind,status,heading,body FROM typed_ticket_events WHERE workspace_id='workspace-1' AND ticket_id='ticket-1' AND event_index=0",[],|row|Ok((row.get(0)?,row.get(1)?,row.get(2)?,row.get(3)?))).unwrap(); + assert_eq!(kind, "comment"); + assert_eq!(status, None); + assert_eq!( + heading.as_deref(), + Some("Legacy review (non-authoritative)") + ); + assert_eq!(body.as_deref(), Some("legacy evidence")); + let attributes:i64=connection.query_row("SELECT COUNT(*) FROM typed_ticket_event_attributes WHERE workspace_id='workspace-1' AND ticket_id='ticket-1'",[],|row|row.get(0)).unwrap(); + assert_eq!(attributes, 1); + let legacy:String=connection.query_row("SELECT value FROM typed_ticket_event_attributes WHERE workspace_id='workspace-1' AND ticket_id='ticket-1' AND key='legacy_event_kind'",[],|row|row.get(0)).unwrap(); + assert_eq!(legacy, "review"); + } + #[test] fn concurrent_migrators_converge_on_one_version_history() { let directory = tempdir().unwrap(); @@ -1072,6 +1129,6 @@ mod tests { let connection = Connection::open(database).unwrap(); verify_sqlite_ticket_schema(&connection).unwrap(); - assert_eq!(load_applied_migrations(&connection).unwrap().len(), 2); + assert_eq!(load_applied_migrations(&connection).unwrap().len(), 3); } } diff --git a/crates/ticket/src/tool.rs b/crates/ticket/src/tool.rs index 90e6938a..f0819548 100644 --- a/crates/ticket/src/tool.rs +++ b/crates/ticket/src/tool.rs @@ -17,8 +17,7 @@ use crate::{ Result as TicketResult, Ticket, TicketBackend, TicketBodyReplacement, TicketDoctorDiagnostic, TicketDoctorReport, TicketDoctorSeverity, TicketError, TicketEventKind, TicketIdOrSlug, TicketIntakeSummary, TicketListState, TicketRef, TicketRelation, TicketRelationKind, - TicketRelationView, TicketReview, TicketReviewResult, TicketStateChange, TicketSummary, - TicketWorkflowState, default_author, + TicketRelationView, TicketStateChange, TicketSummary, TicketWorkflowState, default_author, }; const DEFAULT_LIST_LIMIT: usize = 50; @@ -34,7 +33,7 @@ const MAX_BODY_MAX_BYTES: usize = 64 * 1024; const DEFAULT_DIAGNOSTIC_LIMIT: usize = 100; const MAX_DIAGNOSTIC_LIMIT: usize = 500; -pub const TICKET_BASE_TOOL_NAMES: [&str; 15] = [ +pub const TICKET_BASE_TOOL_NAMES: [&str; 14] = [ "TicketCreate", "TicketEditItem", "TicketList", @@ -43,7 +42,6 @@ pub const TICKET_BASE_TOOL_NAMES: [&str; 15] = [ "TicketPlan", "TicketDecision", "TicketImplementationReport", - "TicketReview", "TicketIntakeReady", "TicketQueue", "TicketWorkflowState", @@ -69,7 +67,7 @@ pub const TICKET_ORCHESTRATION_TOOL_NAMES: [&str; 4] = [ pub const TICKET_ORCHESTRATION_READ_ONLY_TOOL_NAMES: [&str; 2] = ["TicketRelationQuery", "TicketOrchestrationPlanQuery"]; -pub const TICKET_TOOL_NAMES: [&str; 19] = [ +pub const TICKET_TOOL_NAMES: [&str; 18] = [ "TicketCreate", "TicketEditItem", "TicketList", @@ -78,7 +76,6 @@ pub const TICKET_TOOL_NAMES: [&str; 19] = [ "TicketPlan", "TicketDecision", "TicketImplementationReport", - "TicketReview", "TicketIntakeReady", "TicketQueue", "TicketWorkflowState", @@ -100,14 +97,13 @@ pub const TICKET_READ_ONLY_TOOL_NAMES: [&str; 6] = [ "TicketOrchestrationPlanQuery", ]; -pub const TICKET_MUTATING_TOOL_NAMES: [&str; 13] = [ +pub const TICKET_MUTATING_TOOL_NAMES: [&str; 12] = [ "TicketCreate", "TicketEditItem", "TicketComment", "TicketPlan", "TicketDecision", "TicketImplementationReport", - "TicketReview", "TicketIntakeReady", "TicketQueue", "TicketWorkflowState", @@ -134,8 +130,6 @@ const PLAN_DESCRIPTION: &str = "Append a typed Ticket plan event. `body` is Mark const DECISION_DESCRIPTION: &str = "Append a typed Ticket decision event. `body` is Markdown."; const IMPLEMENTATION_REPORT_DESCRIPTION: &str = "Append a typed Ticket implementation_report event. `body` is Markdown."; -const REVIEW_DESCRIPTION: &str = "Append a Ticket review event. `result` must be `approve` or \ -`request_changes`; `body` is Markdown. Writes stay inside the configured Ticket backend root."; const INTAKE_READY_DESCRIPTION: &str = "Mark an existing Ticket planning lane ready through the typed \ Ticket backend. The tool appends a bounded `intake_summary`, appends a typed `state_changed` event \ for `state`, and transitions state to `ready`."; @@ -175,7 +169,6 @@ fn base_tool_description(name: &str) -> &'static str { "TicketPlan" => PLAN_DESCRIPTION, "TicketDecision" => DECISION_DESCRIPTION, "TicketImplementationReport" => IMPLEMENTATION_REPORT_DESCRIPTION, - "TicketReview" => REVIEW_DESCRIPTION, "TicketIntakeReady" => INTAKE_READY_DESCRIPTION, "TicketQueue" => QUEUE_DESCRIPTION, "TicketWorkflowState" => WORKFLOW_STATE_DESCRIPTION, @@ -319,10 +312,6 @@ impl TicketBackend for TicketToolBackend { self.backend.queue_ready(id, queued_by) } - fn review(&self, id: TicketIdOrSlug, review: TicketReview) -> TicketResult<()> { - self.backend.review(id, review) - } - fn close(&self, id: TicketIdOrSlug, resolution: MarkdownText) -> TicketResult<()> { self.backend.close(id, resolution) } @@ -554,23 +543,6 @@ struct TicketThreadEventParams { body: String, } -#[derive(Debug, Deserialize, schemars::JsonSchema)] -#[serde(rename_all = "snake_case")] -enum TicketReviewResultParam { - Approve, - RequestChanges, -} - -#[derive(Debug, Deserialize, schemars::JsonSchema)] -struct TicketReviewParams { - /// Ticket id. - ticket: String, - /// Review result: `approve` or `request_changes`. - result: TicketReviewResultParam, - /// Markdown review body. - body: String, -} - #[derive(Debug, Deserialize, schemars::JsonSchema)] struct TicketIntakeReadyParams { /// Ticket id. @@ -839,11 +811,6 @@ struct TicketImplementationReportTool { backend: TicketToolBackend, } -#[derive(Clone)] -struct TicketReviewTool { - backend: TicketToolBackend, -} - #[derive(Clone)] struct TicketIntakeReadyTool { backend: TicketToolBackend, @@ -1117,34 +1084,6 @@ impl_ticket_thread_event_tool!( TicketEventKind::ImplementationReport ); -#[async_trait] -impl Tool for TicketReviewTool { - async fn execute( - &self, - input_json: &str, - _ctx: llm_engine::tool::ToolExecutionContext, - ) -> Result { - let params: TicketReviewParams = parse_input("TicketReview", input_json)?; - let result = match params.result { - TicketReviewResultParam::Approve => TicketReviewResult::Approve, - TicketReviewResultParam::RequestChanges => TicketReviewResult::RequestChanges, - }; - let result_str = result.as_str().to_string(); - let review = TicketReview { - result, - author: None, - body: MarkdownText::new(params.body), - }; - self.backend - .review(TicketIdOrSlug::Query(params.ticket.clone()), review) - .map_err(|error| backend_error("TicketReview", error))?; - Ok(json_output( - format!("Appended {result_str} review to ticket {}", params.ticket), - json!({ "ticket": params.ticket, "review": result_str, "ok": true }), - )) - } -} - #[async_trait] impl Tool for TicketIntakeReadyTool { async fn execute( @@ -1731,7 +1670,6 @@ fn input_schema(name: &str) -> Value { "TicketComment" | "TicketPlan" | "TicketDecision" | "TicketImplementationReport" => { serde_json::to_value(schemars::schema_for!(TicketThreadEventParams)) } - "TicketReview" => serde_json::to_value(schemars::schema_for!(TicketReviewParams)), "TicketIntakeReady" => serde_json::to_value(schemars::schema_for!(TicketIntakeReadyParams)), "TicketQueue" => serde_json::to_value(schemars::schema_for!(TicketQueueParams)), "TicketWorkflowState" => { @@ -1777,7 +1715,6 @@ impl_from_backend!(TicketCommentTool); impl_from_backend!(TicketPlanTool); impl_from_backend!(TicketDecisionTool); impl_from_backend!(TicketImplementationReportTool); -impl_from_backend!(TicketReviewTool); impl_from_backend!(TicketIntakeReadyTool); impl_from_backend!(TicketQueueTool); impl_from_backend!(TicketWorkflowStateTool); @@ -1804,7 +1741,6 @@ pub fn ticket_tools(backend: impl Into) -> Vec("TicketReview", backend.clone()), tool_definition::("TicketIntakeReady", backend.clone()), tool_definition::("TicketQueue", backend.clone()), tool_definition::("TicketWorkflowState", backend.clone()), @@ -1880,7 +1816,6 @@ mod tests { "TicketPlan", "TicketDecision", "TicketImplementationReport", - "TicketReview", "TicketIntakeReady", "TicketQueue", "TicketWorkflowState", @@ -2373,12 +2308,11 @@ mod tests { } #[tokio::test] - async fn ticket_tools_comment_review_state_and_close_are_doctor_clean() { + async fn ticket_tools_report_state_and_close_are_doctor_clean() { let temp = TempDir::new().unwrap(); let backend = backend(&temp); let created = backend.create(NewTicket::new("Flow Tool")).unwrap(); let report = tool_by_name(backend.clone(), "TicketImplementationReport"); - let review = tool_by_name(backend.clone(), "TicketReview"); let close = tool_by_name(backend.clone(), "TicketClose"); let doctor = tool_by_name(backend.clone(), "TicketDoctor"); @@ -2393,18 +2327,6 @@ mod tests { ) .await .unwrap(); - review - .execute( - &json!({ - "ticket": created.id.clone(), - "result": "approve", - "body": "Looks good." - }) - .to_string(), - Default::default(), - ) - .await - .unwrap(); close .execute( &json!({ "ticket": created.id, "resolution": "Done via TicketClose.\n" }) @@ -2427,12 +2349,6 @@ mod tests { .iter() .any(|event| event.kind == TicketEventKind::ImplementationReport) ); - assert!( - closed - .events - .iter() - .any(|event| event.kind == TicketEventKind::Review) - ); assert!( closed .events @@ -2852,7 +2768,6 @@ mod tests { "TicketPlan", "TicketDecision", "TicketImplementationReport", - "TicketReview", "TicketIntakeReady", "TicketQueue", "TicketRelationRecord", diff --git a/crates/tui/src/dashboard/tests.rs b/crates/tui/src/dashboard/tests.rs index b0e21082..72ab7eb2 100644 --- a/crates/tui/src/dashboard/tests.rs +++ b/crates/tui/src/dashboard/tests.rs @@ -795,13 +795,7 @@ async fn ticket_review_action_does_not_silently_approve() { .unwrap_err(); assert!(error.to_string().contains("current action is Queue")); - let ticket = backend.show(TicketIdOrSlug::Id(ticket_id)).unwrap(); - assert!( - !ticket - .events - .iter() - .any(|event| event.kind == TicketEventKind::Review) - ); + let _ticket = backend.show(TicketIdOrSlug::Id(ticket_id)).unwrap(); } #[test] diff --git a/crates/worker/src/feature/builtin.rs b/crates/worker/src/feature/builtin.rs index 28c248fe..77c9732b 100644 --- a/crates/worker/src/feature/builtin.rs +++ b/crates/worker/src/feature/builtin.rs @@ -9,6 +9,7 @@ pub mod manage_workdir; pub mod manage_worker; pub mod memory; pub mod memory_extract; +pub mod merge_request; pub mod objective; pub mod session_explore; pub mod task; diff --git a/crates/worker/src/feature/builtin/merge_request.rs b/crates/worker/src/feature/builtin/merge_request.rs new file mode 100644 index 00000000..104c4db5 --- /dev/null +++ b/crates/worker/src/feature/builtin/merge_request.rs @@ -0,0 +1,299 @@ +use crate::feature::ToolDefinition; +use crate::worker::{WorkspaceClient, WorkspaceRequest, WorkspaceRequestMethod}; +use async_trait::async_trait; +use llm_engine::tool::{Tool, ToolError, ToolExecutionContext, ToolMeta, ToolOutput}; +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", + "MergeRequestAddRevision", + "MergeRequestComplete", +]; +pub const MERGE_REQUEST_REVIEW_TOOL_NAME: &str = "MergeRequestReviewSubmit"; +#[derive(Clone, Copy)] +enum Kind { + Show, + Readiness, + Open, + AddRevision, + Complete, + Review, +} +#[derive(Clone)] +struct MergeRequestTool { + client: Arc, + kind: Kind, +} + +#[derive(Debug, Deserialize, JsonSchema)] +struct ShowInput { + ticket: String, +} +#[derive(Debug, Deserialize, JsonSchema)] +struct OpenInput { + ticket: String, + repository_id: String, + revision_id: String, + base_commit: String, + head_commit: String, + head_tree: String, + diff_digest: String, + #[serde(default)] + changed_paths: Vec, + #[serde(default)] + summary: String, +} +#[derive(Debug, Deserialize, JsonSchema)] +struct AddRevisionInput { + ticket: String, + expected_current_revision_id: String, + revision_id: String, + base_commit: String, + head_commit: String, + head_tree: String, + diff_digest: String, + #[serde(default)] + changed_paths: Vec, + #[serde(default)] + summary: String, +} +#[derive(Debug, Deserialize, JsonSchema)] +struct CompleteInput { + ticket: String, + operation_id: String, + expected_revision_id: String, +} +#[derive(Debug, Deserialize, JsonSchema)] +struct ReviewInput { + decision: ReviewDecisionInput, + #[serde(default)] + body: String, + #[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, +} + +impl Kind { + fn name(self) -> &'static str { + match self { + Self::Show => "MergeRequestShow", + Self::Readiness => "MergeRequestReadinessCheck", + Self::Open => "MergeRequestOpen", + Self::AddRevision => "MergeRequestAddRevision", + 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::Complete => json!(schemars::schema_for!(CompleteInput)), + Self::Review => json!(schemars::schema_for!(ReviewInput)), + } + } +} + +#[async_trait] +impl Tool for MergeRequestTool { + async fn execute( + &self, + input: &str, + _context: ToolExecutionContext, + ) -> Result { + let workspace_id = self.client.workspace_id().ok_or_else(|| { + ToolError::ExecutionFailed("Merge Request tools require Workspace identity".into()) + })?; + let (method, path, body) = match self.kind { + Kind::Show => { + let v: ShowInput = parse(input)?; + nonempty(&v.ticket)?; + ( + WorkspaceRequestMethod::Get, + format!("/api/w/{workspace_id}/tickets/{}/merge-request", v.ticket), + None, + ) + } + Kind::Readiness => { + let v: ShowInput = parse(input)?; + nonempty(&v.ticket)?; + ( + WorkspaceRequestMethod::Get, + format!( + "/api/w/{workspace_id}/tickets/{}/merge-request/readiness", + v.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,"head_tree":v.head_tree,"diff_digest":v.diff_digest,"changed_paths":v.changed_paths,"summary":v.summary}), + ), + ) + } + Kind::AddRevision => { + let v: AddRevisionInput = parse(input)?; + nonempty(&v.ticket)?; + ( + WorkspaceRequestMethod::Post, + format!( + "/api/w/{workspace_id}/tickets/{}/merge-request/revisions", + v.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,"head_tree":v.head_tree,"diff_digest":v.diff_digest,"changed_paths":v.changed_paths,"summary":v.summary}), + ), + ) + } + Kind::Complete => { + let v: CompleteInput = parse(input)?; + nonempty(&v.ticket)?; + ( + 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}), + ), + ) + } + Kind::Review => { + let v: ReviewInput = parse(input)?; + let context = self.client.reviewer_attempt_context().ok_or_else(|| { + ToolError::ExecutionFailed( + "MergeRequestReviewSubmit is available only to an attested Reviewer child" + .into(), + ) + })?; + ( + WorkspaceRequestMethod::Post, + format!( + "/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::>() }), + ), + ) + } + }; + let request = match body { + Some(body) => WorkspaceRequest::json(method, path, body.to_string()), + None => WorkspaceRequest::get(path), + }; + let response = self + .client + .execute(request) + .map_err(|e| ToolError::ExecutionFailed(e.to_string()))?; + if !response.is_success() { + return Err(ToolError::ExecutionFailed(format!( + "Merge Request API returned HTTP {}: {}", + response.status, response.body + ))); + } + Ok(ToolOutput { + summary: self.kind.name().to_string(), + content: Some(response.body), + attachments: Vec::new(), + }) + } +} +fn parse(value: &str) -> Result { + serde_json::from_str(value).map_err(|e| ToolError::InvalidArgument(e.to_string())) +} +fn nonempty(value: &str) -> Result<(), ToolError> { + if value.trim().is_empty() { + Err(ToolError::InvalidArgument( + "ticket must not be empty".into(), + )) + } else { + Ok(()) + } +} +fn definition(client: Arc, kind: Kind) -> ToolDefinition { + Arc::new(move || { + let meta = ToolMeta::new(kind.name()) + .description(kind.description()) + .input_schema(kind.schema()); + let tool: Arc = Arc::new(MergeRequestTool { + client: client.clone(), + kind, + }); + (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, Kind::Complete), + ] +} +pub fn reviewer_tools(client: Arc) -> Vec { + if client.reviewer_attempt_context().is_some() { + vec![ + definition(client.clone(), Kind::Show), + definition(client, Kind::Review), + ] + } else { + 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.", + ), + "MergeRequestReadinessCheck" => { + Some("Check derived merge readiness for the current immutable revision.") + } + "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.", + ), + _ => None, + } +} diff --git a/crates/worker/src/feature/builtin/ticket.rs b/crates/worker/src/feature/builtin/ticket.rs index d017850b..32d3eb30 100644 --- a/crates/worker/src/feature/builtin/ticket.rs +++ b/crates/worker/src/feature/builtin/ticket.rs @@ -14,12 +14,13 @@ use ticket::{ NewTicketRelation, OrchestrationPlanKind, OrchestrationPlanRecord, Result as TicketResult, Ticket, TicketBackend, TicketBackendOperation, TicketBackendOperationResult, TicketDoctorReport, TicketError, TicketIdOrSlug, TicketIntakeSummary, TicketListQuery, - TicketRef, TicketRelation, TicketRelationKind, TicketRelationView, TicketReview, - TicketStateChange, TicketSummary, + TicketRef, TicketRelation, TicketRelationKind, TicketRelationView, TicketStateChange, + TicketSummary, config::{DEFAULT_TICKET_BACKEND_RELATIVE_PATH, TicketConfig}, tool::{TICKET_TOOL_NAMES, TicketToolBackend, ticket_tool_description, ticket_tools}, }; +use super::merge_request; use crate::feature::{ FeatureDescriptor, FeatureDiagnostic, FeatureInstallContext, FeatureInstallError, FeatureInstructionContribution, FeatureInstructionDeclaration, FeatureInstructionId, @@ -100,7 +101,7 @@ impl TicketFeatureAccess { pub const fn review() -> Self { Self { authoring: false, - thread: true, + thread: false, intake: false, orchestration_control: false, } @@ -141,7 +142,7 @@ const AUTHORING_TOOL_NAMES: &[&str] = &[ "TicketRelationRecord", ]; -const THREAD_TOOL_NAMES: &[&str] = &["TicketComment", "TicketReview"]; +const THREAD_TOOL_NAMES: &[&str] = &["TicketComment"]; const INTAKE_TOOL_NAMES: &[&str] = &["TicketIntakeReady"]; @@ -152,7 +153,6 @@ const WORKSPACE_AUTHORING_TOOL_NAMES: &[&str] = &[ "TicketList", "TicketShow", "TicketComment", - "TicketReview", "TicketQueue", "TicketClose", "TicketDependencyCheck", @@ -167,7 +167,6 @@ const ORCHESTRATION_CONTROL_TOOL_NAMES: &[&str] = &[ "TicketList", "TicketShow", "TicketComment", - "TicketReview", "TicketWorkflowState", "TicketClose", "TicketDependencyCheck", @@ -340,6 +339,22 @@ 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_attempt_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 } @@ -373,6 +388,17 @@ 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() { + 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(()) } } @@ -611,14 +637,6 @@ impl WorkspaceHttpTicketBackend { format!("{base}/{}/workflow/queue", Self::ticket_path(&id)), None, ), - TicketBackendOperation::Review { id, review } => Self::request_unit( - client, - WorkspaceRequestMethod::Post, - format!("{base}/{}/workflow/review", Self::ticket_path(&id)), - Some(serde_json::to_value(review).map_err(|error| { - TicketError::Conflict(format!("serialize Ticket review: {error}")) - })?), - ), TicketBackendOperation::Close { id, resolution } => Self::request_unit( client, WorkspaceRequestMethod::Post, @@ -844,15 +862,6 @@ impl TicketBackend for WorkspaceHttpTicketBackend { } } - fn review(&self, id: TicketIdOrSlug, review: TicketReview) -> TicketResult<()> { - match self.invoke(TicketBackendOperation::Review { id, review })? { - TicketBackendOperationResult::Unit => Ok(()), - other => Err(TicketError::Conflict(format!( - "unexpected ticket backend response: {other:?}" - ))), - } - } - fn close(&self, id: TicketIdOrSlug, resolution: MarkdownText) -> TicketResult<()> { match self.invoke(TicketBackendOperation::Close { id, resolution })? { TicketBackendOperationResult::Unit => Ok(()), @@ -1075,7 +1084,6 @@ mod tests { .map(|tool| tool.name.as_str()) .collect::>(); assert!(work_report_tools.contains(&"TicketComment")); - assert!(work_report_tools.contains(&"TicketReview")); assert!(!work_report_tools.contains(&"TicketWorkflowState")); let review = ticket_tools_feature_with_access(temp.path(), TicketFeatureAccess::review()); @@ -1085,7 +1093,6 @@ mod tests { .iter() .map(|tool| tool.name.as_str()) .collect::>(); - assert!(review_tools.contains(&"TicketReview")); assert!(!review_tools.contains(&"TicketWorkflowState")); } diff --git a/crates/worker/src/internal_worker.rs b/crates/worker/src/internal_worker.rs index 7f7811db..3e637b47 100644 --- a/crates/worker/src/internal_worker.rs +++ b/crates/worker/src/internal_worker.rs @@ -259,6 +259,10 @@ pub(crate) struct InternalWorkerSessionHandle { } impl InternalWorkerSessionHandle { + pub(crate) fn session_id_string(&self) -> String { + self.session_id.to_string() + } + pub(crate) fn status(&self) -> InternalWorkerSessionStatus { InternalWorkerSessionStatus::decode(self.status.load(std::sync::atomic::Ordering::Acquire)) } diff --git a/crates/worker/src/spawn/tool.rs b/crates/worker/src/spawn/tool.rs index 9ead8643..21fbc182 100644 --- a/crates/worker/src/spawn/tool.rs +++ b/crates/worker/src/spawn/tool.rs @@ -27,7 +27,10 @@ use crate::internal_worker::{ }; use crate::prompt::catalog::PromptCatalog; use crate::spawn::registry::SpawnedWorkerRegistry; -use crate::worker::{Worker, WorkerFilesystemAuthority}; +use crate::worker::{ + ReviewerAttemptContext, ReviewerChildWorkspaceClient, Worker, WorkerFilesystemAuthority, + WorkspaceRequest, WorkspaceRequestMethod, +}; use protocol::Method; #[derive(Debug, Deserialize, schemars::JsonSchema)] @@ -55,6 +58,16 @@ 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. + #[serde(default)] + review: Option, +} + +#[derive(Debug, Deserialize, schemars::JsonSchema)] +struct ReviewerHandoffInput { + ticket_id: String, + revision_id: String, } #[derive(Debug, Deserialize, schemars::JsonSchema)] @@ -320,6 +333,32 @@ impl SubWorkerSpawnTool { } } +fn validate_reviewer_handoff(input: &SubWorkerSpawnInput) -> Result<(), ToolError> { + let Some(review) = &input.review else { + return Ok(()); + }; + if review.ticket_id.trim().is_empty() || review.revision_id.trim().is_empty() { + return Err(ToolError::InvalidArgument( + "reviewer handoff requires non-empty ticket_id and revision_id".to_string(), + )); + } + if input.profile.as_deref() != Some("builtin:reviewer") { + return Err(ToolError::InvalidArgument( + "reviewer handoff requires the explicit effective profile builtin:reviewer".to_string(), + )); + } + if input + .scope + .iter() + .any(|rule| matches!(rule.permission, PermissionInput::Write)) + { + return Err(ToolError::InvalidArgument( + "Merge Request Reviewer SubWorkers must have read-only delegated scope".to_string(), + )); + } + Ok(()) +} + #[async_trait] impl Tool for SubWorkerSpawnTool { async fn execute( @@ -340,6 +379,7 @@ impl Tool for SubWorkerSpawnTool { input.name ))); } + validate_reviewer_handoff(&input)?; let name_reservation = self .registry .reserve_internal_name(input.name.clone()) @@ -378,6 +418,48 @@ impl Tool for SubWorkerSpawnTool { .map_err(|error| { ToolError::ExecutionFailed(format!("resolve child manifest: {error}")) })?; + let reviewer_attempt = input.review.as_ref().map(|review| { + ( + review.ticket_id.clone(), + review.revision_id.clone(), + uuid::Uuid::now_v7().to_string(), + format!( + "{}{}", + uuid::Uuid::now_v7().simple(), + uuid::Uuid::now_v7().simple() + ), + ) + }); + let child_workspace_context = + if let Some((ticket_id, revision_id, _, capability_token)) = &reviewer_attempt { + let workspace_id = + self.workspace_context + .workspace_id() + .cloned() + .ok_or_else(|| { + ToolError::InvalidArgument( + "reviewer handoff requires Workspace identity".to_string(), + ) + })?; + let parent_client = self.workspace_context.client_handle(); + if !parent_client.is_available() { + return Err(ToolError::InvalidArgument( + "reviewer handoff requires Workspace API authority".to_string(), + )); + } + let child_client: Arc = + Arc::new(ReviewerChildWorkspaceClient::new( + parent_client.clone(), + ReviewerAttemptContext { + ticket_id: ticket_id.clone(), + revision_id: revision_id.clone(), + }, + capability_token.clone(), + )); + crate::worker::WorkerWorkspaceContext::with_client(Some(workspace_id), child_client) + } else { + self.workspace_context.clone() + }; let store = EphemeralSessionStore::default(); let filesystem_authority = WorkerFilesystemAuthority::local(self.workspace_root.clone(), child_cwd.clone()); @@ -385,7 +467,7 @@ impl Tool for SubWorkerSpawnTool { child_manifest, store.clone(), self.prompt_loader.clone(), - self.workspace_context.clone(), + child_workspace_context, filesystem_authority, self.internal_client_override .as_ref() @@ -465,6 +547,66 @@ impl Tool for SubWorkerSpawnTool { } }; + if let Some((ticket_id, revision_id, attempt_id, capability_token)) = &reviewer_attempt { + let workspace_id = self.workspace_context.workspace_id().ok_or_else(|| { + ToolError::ExecutionFailed("reviewer attempt lost Workspace identity".to_string()) + })?; + let child_session_id = session.session_id_string(); + let child_registration = WorkspaceRequest::json( + WorkspaceRequestMethod::Post, + format!( + "/api/w/{}/internal/reviewer-child-sessions", + workspace_id.as_str() + ), + serde_json::json!({"child_session_id": child_session_id}).to_string(), + ); + let child_response = self + .workspace_context + .client() + .execute(child_registration) + .map_err(|error| { + ToolError::ExecutionFailed(format!( + "register Runtime-owned Reviewer child session: {error}" + )) + })?; + if !child_response.is_success() { + let _ = session.stop().await; + return Err(ToolError::ExecutionFailed(format!( + "register Runtime-owned Reviewer child session failed with status {}: {}", + child_response.status, child_response.body + ))); + } + let body = serde_json::json!({ + "attempt_id": attempt_id, + "revision_id": revision_id, + "child_session_id": child_session_id, + "capability_token": capability_token, + }); + let request = WorkspaceRequest::json( + WorkspaceRequestMethod::Post, + format!( + "/api/w/{}/tickets/{}/merge-request/review-attempts", + workspace_id.as_str(), + ticket_id + ), + body.to_string(), + ); + let response = self + .workspace_context + .client() + .execute(request) + .map_err(|error| { + ToolError::ExecutionFailed(format!("register reviewer attempt: {error}")) + })?; + if !response.is_success() { + let _ = session.stop().await; + return Err(ToolError::ExecutionFailed(format!( + "register reviewer attempt failed with status {}: {}", + response.status, response.body + ))); + } + } + let record = crate::spawn::registry::InternalSpawnedWorkerRecord::new( input.name.clone(), scope_allow, @@ -899,6 +1041,31 @@ mod tests { WorkspaceClient, WorkspaceClientError, WorkspaceRequest, WorkspaceResponse, }; + #[test] + fn reviewer_handoff_requires_explicit_builtin_profile_and_read_only_scope() { + 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"} + })) + .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"} + })) + .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"} + })) + .unwrap(); + assert!(validate_reviewer_handoff(&writable).is_err()); + } + fn abs_rule(path: &Path, permission: Permission) -> ScopeRule { ScopeRule { target: path.to_path_buf(), diff --git a/crates/worker/src/worker.rs b/crates/worker/src/worker.rs index 48ec8b53..26d857f7 100644 --- a/crates/worker/src/worker.rs +++ b/crates/worker/src/worker.rs @@ -223,6 +223,94 @@ pub trait WorkspaceClient: std::fmt::Debug + Send + Sync { fn is_available(&self) -> bool; fn execute(&self, request: WorkspaceRequest) -> Result; + + /// Trusted review-attempt 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> { + None + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ReviewerAttemptContext { + pub ticket_id: String, + pub revision_id: String, +} + +#[derive(Debug)] +pub struct ReviewerChildWorkspaceClient { + inner: Arc, + context: ReviewerAttemptContext, + capability_token: String, +} + +impl ReviewerChildWorkspaceClient { + pub fn new( + inner: Arc, + context: ReviewerAttemptContext, + capability_token: String, + ) -> Self { + Self { + inner, + context, + capability_token, + } + } +} + +impl WorkspaceClient for ReviewerChildWorkspaceClient { + fn workspace_id(&self) -> Option<&str> { + self.inner.workspace_id() + } + fn kind(&self) -> &str { + "runtime-reviewer-child" + } + fn is_available(&self) -> bool { + self.inner.is_available() + } + fn reviewer_attempt_context(&self) -> Option<&ReviewerAttemptContext> { + Some(&self.context) + } + + fn execute( + &self, + mut request: WorkspaceRequest, + ) -> Result { + let expected_path = format!( + "/api/w/{}/tickets/{}/merge-request/reviews", + self.workspace_id().unwrap_or_default(), + self.context.ticket_id + ); + if request.method == WorkspaceRequestMethod::Post && request.path == expected_path { + let body = request.body.take().ok_or_else(|| { + WorkspaceClientError::Request("review submission requires a JSON body".to_string()) + })?; + let mut value: serde_json::Value = serde_json::from_str(&body) + .map_err(|error| WorkspaceClientError::Request(error.to_string()))?; + let object = value.as_object_mut().ok_or_else(|| { + WorkspaceClientError::Request( + "review submission body must be an object".to_string(), + ) + })?; + object.insert( + "revision_id".to_string(), + serde_json::Value::String(self.context.revision_id.clone()), + ); + object.insert( + "capability_token".to_string(), + serde_json::Value::String(self.capability_token.clone()), + ); + request.body = Some( + serde_json::to_string(&value) + .map_err(|error| WorkspaceClientError::Request(error.to_string()))?, + ); + } else if request.method != WorkspaceRequestMethod::Get { + return Err(WorkspaceClientError::Unavailable( + "Reviewer child Workspace authority is read-only except for its one attested Merge Request review submission".to_string(), + )); + } + self.inner.execute(request) + } } /// HTTP forwarding client created by Runtime for one concrete Worker execution. @@ -365,6 +453,36 @@ impl WorkspaceClient for MarkerWorkspaceClient { } } +#[cfg(test)] +mod reviewer_client_tests { + use super::*; + + #[test] + fn reviewer_child_client_denies_non_review_workspace_mutations() { + let inner: Arc = Arc::new(MarkerWorkspaceClient { + workspace_id: Some("ws".to_string()), + kind: "marker".to_string(), + available: true, + reason: "forwarded".to_string(), + }); + let client = ReviewerChildWorkspaceClient::new( + inner, + ReviewerAttemptContext { + ticket_id: "T1".into(), + revision_id: "V1".into(), + }, + "secret".into(), + ); + let request = WorkspaceRequest::json( + WorkspaceRequestMethod::Post, + "/api/w/ws/tickets/T1/comments", + "{}".to_string(), + ); + let error = client.execute(request).unwrap_err(); + assert!(error.to_string().contains("read-only")); + } +} + pub fn unavailable_workspace_client( workspace_id: Option<&WorkspaceId>, reason: impl Into, diff --git a/crates/workspace-server/Cargo.toml b/crates/workspace-server/Cargo.toml index eda17ca8..4f7f7e4f 100644 --- a/crates/workspace-server/Cargo.toml +++ b/crates/workspace-server/Cargo.toml @@ -32,6 +32,7 @@ sha2.workspace = true thiserror.workspace = true ticket.workspace = true memory.workspace = true +merge-request.workspace = true tokio = { workspace = true, features = ["fs", "macros", "net", "rt-multi-thread", "sync", "time"] } tokio-tungstenite.workspace = true worker.workspace = true diff --git a/crates/workspace-server/src/lib.rs b/crates/workspace-server/src/lib.rs index 2e2d67ca..32708b85 100644 --- a/crates/workspace-server/src/lib.rs +++ b/crates/workspace-server/src/lib.rs @@ -55,6 +55,8 @@ pub enum Error { Sqlite(#[from] rusqlite::Error), #[error("ticket error: {0}")] Ticket(#[from] ticket::TicketError), + #[error("merge request error: {0}")] + MergeRequest(#[from] merge_request::MergeRequestError), #[error("yaml error: {0}")] Yaml(#[from] serde_yaml::Error), #[error("invalid input: {0}")] @@ -88,6 +90,14 @@ pub enum Error { }, #[error("unknown local repository `{0}`")] UnknownRepository(String), + #[error( + "merge confirmation requires an authenticated Browser session; API tokens and Worker actors are not accepted" + )] + BrowserMergeConfirmationRequired, + #[error( + "Merge Request reopen requires an authenticated Browser session and explicit confirmation" + )] + BrowserReopenConfirmationRequired, #[error("workspace id does not match this Workspace backend")] WorkspaceIdMismatch, #[error("Ticket assignment conflict: {0}")] diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 6529ad6d..9dfe907a 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -24,8 +24,7 @@ use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use ticket::{ MarkdownText, NewTicketEvent, TicketBackend, TicketBodyReplacement, TicketEventKind, - TicketIdOrSlug, TicketItemEdit, TicketReview, TicketReviewResult, TicketStateChange, - TicketTargetEdit, TicketWorkflowState, + TicketIdOrSlug, TicketItemEdit, TicketStateChange, TicketTargetEdit, TicketWorkflowState, }; use ticket::{ SqliteTicketBackend, TicketBackendOperation, TicketBackendOperationResult, @@ -861,8 +860,40 @@ pub fn build_router(api: WorkspaceApi) -> Router { post(scoped_queue_ticket_record), ) .route( - "/api/w/{workspace_id}/tickets/{id}/workflow/review", - post(scoped_review_ticket_record), + "/api/w/{workspace_id}/tickets/{id}/merge-request", + get(scoped_show_merge_request).post(scoped_open_merge_request), + ) + .route( + "/api/w/{workspace_id}/tickets/{id}/merge-request/readiness", + get(scoped_merge_request_readiness), + ) + .route( + "/api/w/{workspace_id}/tickets/{id}/merge-request/revisions", + post(scoped_add_merge_request_revision), + ) + .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), + ) + .route( + "/api/w/{workspace_id}/tickets/{id}/merge-request/reviews", + post(scoped_submit_merge_request_review), + ) + .route( + "/api/w/{workspace_id}/tickets/{id}/merge-request/complete", + post(scoped_complete_merge_request), + ) + .route( + "/api/w/{workspace_id}/tickets/{id}/merge-request/reopen", + post(scoped_reopen_merge_request), + ) + .route( + "/api/w/{workspace_id}/tickets/{id}/merge-request/merge", + post(scoped_confirm_merge_request), ) .route( "/api/w/{workspace_id}/tickets/{id}/workflow/close", @@ -902,10 +933,6 @@ pub fn build_router(api: WorkspaceApi) -> Router { "/api/w/{workspace_id}/tickets/{id}/events", post(scoped_append_ticket_event), ) - .route( - "/api/w/{workspace_id}/tickets/{id}/reviews", - post(scoped_review_ticket), - ) .route( "/api/w/{workspace_id}/tickets/{id}/queue", post(scoped_queue_ticket), @@ -2419,14 +2446,6 @@ struct BrowserAppendTicketEventRequest { author: Option, } -#[derive(Debug, Deserialize)] -#[serde(deny_unknown_fields)] -struct BrowserReviewTicketRequest { - result: TicketReviewResult, - body: String, - author: Option, -} - #[derive(Debug, Deserialize)] #[serde(deny_unknown_fields)] struct BrowserQueueTicketRequest { @@ -2505,6 +2524,11 @@ async fn scoped_transition_ticket_state( Json(request): Json, ) -> ApiResult> { validate_workspace_scope(&api, &path.workspace_id)?; + if request.state == TicketWorkflowState::Done { + return Err(Error::TicketAssignmentConflict( + "done is guarded by MergeRequestComplete with an approved immutable revision and operation_id".to_string(), + ).into()); + } let current = api.authority.ticket(&path.id)?; let mut change = TicketStateChange::new( current.state, @@ -2535,25 +2559,6 @@ async fn scoped_append_ticket_event( browser_ticket_detail(&api, &path.id) } -async fn scoped_review_ticket( - State(api): State, - AxumPath(path): AxumPath, - Json(request): Json, -) -> ApiResult> { - validate_workspace_scope(&api, &path.workspace_id)?; - browser_ticket_backend(&api)? - .review( - TicketIdOrSlug::Id(path.id.clone()), - TicketReview { - result: request.result, - body: MarkdownText::new(request.body), - author: request.author, - }, - ) - .map_err(Error::from)?; - browser_ticket_detail(&api, &path.id) -} - async fn scoped_queue_ticket( State(api): State, AxumPath(path): AxumPath, @@ -2602,6 +2607,16 @@ async fn scoped_close_ticket( browser_ticket_detail(&api, &path.id) } +fn reject_unguarded_ticket_completion(operation: &TicketBackendOperation) -> Result<()> { + if matches!(operation, TicketBackendOperation::SetWorkflowState { change, .. } if change.to == "done") + { + return Err(Error::TicketAssignmentConflict( + "done is guarded by MergeRequestComplete with an approved immutable revision and operation_id".to_string(), + )); + } + Ok(()) +} + async fn execute_worker_ticket_rest_operation( api: &WorkspaceApi, workspace_id: &str, @@ -2621,6 +2636,7 @@ async fn execute_worker_ticket_rest_operation( let is_mutation = operation_kind != "read"; let target = ticket_mutation_target(&operation).cloned(); let source = authenticate_worker_mutation_source(api, workspace_id, &headers)?; + reject_unguarded_ticket_completion(&operation)?; validate_ticket_repository_operation(api, &operation)?; let before = target.as_ref().and_then(|id| backend.show(id.clone()).ok()); let previous_state = before @@ -2969,23 +2985,393 @@ async fn scoped_queue_ticket_record( ticket_rest_unit(result) } -async fn scoped_review_ticket_record( - State(api): State, - AxumPath((workspace_id, id)): AxumPath<(String, String)>, - headers: HeaderMap, - Json(review): Json, -) -> ApiResult { - let result = execute_worker_ticket_rest_operation( - &api, - &workspace_id, - headers, - TicketBackendOperation::Review { - id: TicketIdOrSlug::Query(id), - review, - }, +#[derive(Debug, serde::Deserialize)] +struct OpenMergeRequestRequest { + repository_id: String, + revision_id: String, + base_commit: String, + head_commit: String, + head_tree: String, + diff_digest: String, + #[serde(default)] + changed_paths: Vec, + #[serde(default)] + summary: String, +} + +#[derive(Debug, serde::Deserialize)] +struct AddMergeRequestRevisionRequest { + expected_current_revision_id: String, + revision_id: String, + base_commit: String, + head_commit: String, + head_tree: String, + diff_digest: String, + #[serde(default)] + changed_paths: Vec, + #[serde(default)] + summary: String, +} + +#[derive(Debug, serde::Deserialize)] +struct RegisterReviewerChildSessionRequest { + child_session_id: String, +} + +#[derive(Debug, serde::Deserialize)] +struct RegisterMergeRequestReviewAttemptRequest { + attempt_id: String, + revision_id: String, + child_session_id: String, + capability_token: String, +} + +#[derive(Debug, serde::Deserialize)] +struct SubmitMergeRequestReviewRequest { + revision_id: String, + capability_token: String, + decision: merge_request::ReviewDecision, + #[serde(default)] + body: String, + #[serde(default)] + findings: Vec, +} + +#[derive(Debug, serde::Deserialize)] +struct CompleteMergeRequestRequest { + operation_id: String, + expected_revision_id: String, +} + +#[derive(Debug, serde::Deserialize)] +struct RevisionTransitionRequest { + expected_revision_id: String, + explicit_confirmation: bool, +} + +#[derive(Debug, serde::Deserialize)] +struct ConfirmMergeRequestRequest { + expected_revision_id: String, + explicit_confirmation: bool, +} + +fn parse_workspace_id(value: &str) -> ApiResult { + if value.trim().is_empty() { + return Err(Error::InvalidInput("workspace_id must not be empty".to_string()).into()); + } + Ok(value.to_string()) +} + +fn require_workspace_access(workspace_id: &str, api: &WorkspaceApi) -> ApiResult<()> { + if workspace_id != api.workspace_id() { + return Err(Error::WorkspaceIdMismatch.into()); + } + Ok(()) +} + +fn merge_request_store( + api: &WorkspaceApi, + workspace_id: &str, +) -> ApiResult { + require_workspace_access(workspace_id, api)?; + merge_request::SqliteMergeRequestStore::open_verified( + api.config.database_path.clone(), + workspace_id, ) - .await?; - ticket_rest_unit(result) + .map_err(Error::from) + .map_err(Into::into) +} + +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 value = store + .show_for_ticket(&ticket_id)? + .ok_or_else(|| Error::from(merge_request::MergeRequestError::NotFound(ticket_id)))?; + Ok(Json(value)) +} + +async fn scoped_merge_request_readiness( + State(api): State, + AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, +) -> ApiResult> { + let workspace_id = parse_workspace_id(&workspace_id)?; + Ok(Json( + merge_request_store(&api, &workspace_id)?.readiness_for_ticket(&ticket_id)?, + )) +} + +async fn scoped_open_merge_request( + State(api): State, + headers: HeaderMap, + AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, + 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)?; + let assignment = api + .store + .get_current_ticket_worker_assignment(&workspace_id, &ticket_id)? + .ok_or_else(|| { + Error::TicketAssignmentConflict("Ticket has no current assigned Coder".to_string()) + })?; + 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(), + ) + .into()); + } + let now = Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true); + let revision = merge_request::MergeRequestRevision { + revision_id: input.revision_id, + ordinal: 1, + base_commit: input.base_commit, + head_commit: input.head_commit, + head_tree: input.head_tree, + diff_digest: input.diff_digest, + changed_paths: input.changed_paths, + summary: input.summary, + assignment_id: assignment.assignment_id.clone(), + created_at: now.clone(), + }; + let mr = merge_request_store(&api, &workspace_id)?.open_merge_request( + merge_request::OpenMergeRequest { + merge_request_id: format!("mr_{}", Uuid::now_v7().simple()), + ticket_id, + repository_id: input.repository_id, + revision, + authenticated_runtime_id: source.runtime_id, + authenticated_worker_id: source.worker_id, + now, + }, + )?; + Ok(Json(mr)) +} + +async fn scoped_add_merge_request_revision( + State(api): State, + headers: HeaderMap, + AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, + 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)?; + let assignment = api + .store + .get_current_ticket_worker_assignment(&workspace_id, &ticket_id)? + .ok_or_else(|| { + 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".into(), + ) + .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 now = Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true); + let mr = + merge_request_store(&api, &workspace_id)?.add_revision(merge_request::AddRevision { + 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, + base_commit: input.base_commit, + head_commit: input.head_commit, + head_tree: input.head_tree, + diff_digest: input.diff_digest, + 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)) +} + +async fn scoped_register_reviewer_child_session( + State(api): State, + headers: HeaderMap, + AxumPath(workspace_id): AxumPath, + 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)?; + merge_request_store(&api, &workspace_id)?.register_reviewer_child_session( + merge_request::RegisterReviewerChildSession { + 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), + }, + )?; + Ok(StatusCode::NO_CONTENT) +} + +async fn scoped_register_merge_request_review_attempt( + State(api): State, + headers: HeaderMap, + AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, + 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)?; + let assignment = api + .store + .get_current_ticket_worker_assignment(&workspace_id, &ticket_id)? + .ok_or_else(|| { + 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".into(), + ) + .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), + }, + )?; + Ok(StatusCode::NO_CONTENT) +} + +async fn scoped_submit_merge_request_review( + State(api): State, + AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, + Json(input): Json, +) -> 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)) +} + +async fn scoped_complete_merge_request( + State(api): State, + headers: HeaderMap, + AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, + 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)?; + let assignment = api + .store + .get_current_ticket_worker_assignment(&workspace_id, &ticket_id)? + .ok_or_else(|| { + 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".into(), + ) + .into()); + } + let outcome = merge_request_store(&api, &workspace_id)?.complete( + merge_request::CompleteMergeRequest { + operation_id: input.operation_id, + ticket_id, + expected_revision_id: input.expected_revision_id, + assignment_id: assignment.assignment_id, + authenticated_runtime_id: source.runtime_id, + authenticated_worker_id: source.worker_id, + now: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), + }, + )?; + Ok(Json(outcome)) +} + +async fn scoped_reopen_merge_request( + State(api): State, + headers: HeaderMap, + AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, + Json(input): Json, +) -> ApiResult> { + let workspace_id = parse_workspace_id(&workspace_id)?; + require_workspace_access(&workspace_id, &api)?; + reject_non_browser_merge_auth(&headers) + .map_err(|_| Error::BrowserReopenConfirmationRequired)?; + let _actor = require_actor(&api, &headers).await?; + 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), + )?)) +} + +fn reject_non_browser_merge_auth(headers: &HeaderMap) -> Result<()> { + if headers.contains_key("authorization") { + return Err(Error::BrowserMergeConfirmationRequired); + } + Ok(()) +} + +async fn scoped_confirm_merge_request( + State(api): State, + headers: HeaderMap, + AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>, + Json(input): Json, +) -> ApiResult> { + let workspace_id = parse_workspace_id(&workspace_id)?; + require_workspace_access(&workspace_id, &api)?; + reject_non_browser_merge_auth(&headers)?; + let actor = require_actor(&api, &headers).await?; + let mr = merge_request_store(&api, &workspace_id)?.confirm_merge( + merge_request::MergeConfirmation { + ticket_id, + expected_revision_id: input.expected_revision_id, + authenticated_account_id: actor.account_id, + actor_kind: "user".to_string(), + explicit_confirmation: input.explicit_confirmation, + now: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), + }, + )?; + Ok(Json(mr)) } async fn scoped_close_ticket_record( @@ -3157,7 +3543,6 @@ fn ticket_mutation_target(operation: &TicketBackendOperation) -> Option<&TicketI | TicketBackendOperation::SetWorkflowState { id, .. } | TicketBackendOperation::MarkIntakeReady { id, .. } | TicketBackendOperation::QueueReady { id, .. } - | TicketBackendOperation::Review { id, .. } | TicketBackendOperation::Close { id, .. } | TicketBackendOperation::AddTicketRelation { id, .. } | TicketBackendOperation::AddOrchestrationPlanRecord { id, .. } => Some(id), @@ -3203,7 +3588,6 @@ fn bind_worker_ticket_operation_source( change.author = Some(author); } TicketBackendOperation::QueueReady { queued_by, .. } => *queued_by = author, - TicketBackendOperation::Review { review, .. } => review.author = Some(author), TicketBackendOperation::AddTicketRelation { relation, .. } => { relation.author = Some(author) } @@ -3225,7 +3609,6 @@ fn ticket_mutation_operation_kind(operation: &TicketBackendOperation) -> &'stati TicketBackendOperation::SetWorkflowState { .. } => "set_workflow_state", TicketBackendOperation::MarkIntakeReady { .. } => "mark_intake_ready", TicketBackendOperation::QueueReady { .. } => "queue_ready", - TicketBackendOperation::Review { .. } => "review", TicketBackendOperation::Close { .. } => "close", TicketBackendOperation::AddTicketRelation { .. } => "add_relation", TicketBackendOperation::AddOrchestrationPlanRecord { .. } => "add_plan_record", @@ -7299,10 +7682,7 @@ struct RuntimeConfigBundleAvailabilityQuery { digest: String, } -fn reject_workdir_for_embedded_runtime( - runtime_id: &str, - has_workdir: bool, -) -> std::result::Result<(), ApiError> { +fn reject_workdir_for_embedded_runtime(runtime_id: &str, has_workdir: bool) -> ApiResult<()> { if runtime_id != EMBEDDED_WORKER_RUNTIME_ID || !has_workdir { return Ok(()); } @@ -7320,9 +7700,7 @@ fn reject_workdir_for_embedded_runtime( )) } -fn reject_no_workdir_for_non_embedded_runtime( - runtime_id: &str, -) -> std::result::Result<(), ApiError> { +fn reject_no_workdir_for_non_embedded_runtime(runtime_id: &str) -> ApiResult<()> { if runtime_id == EMBEDDED_WORKER_RUNTIME_ID { return Ok(()); } @@ -9974,6 +10352,12 @@ struct ApiErrorLog { diagnostics: Vec, } +impl From for ApiError { + fn from(error: merge_request::MergeRequestError) -> Self { + Error::MergeRequest(error).into() + } +} + impl From for ApiError { fn from(error: Error) -> Self { let diagnostics = match &error { @@ -10013,6 +10397,9 @@ impl ApiError { impl IntoResponse for ApiError { fn into_response(self) -> Response { let status = match &self.error { + Error::BrowserMergeConfirmationRequired | Error::BrowserReopenConfirmationRequired => { + StatusCode::FORBIDDEN + } Error::TicketAssignmentConflict(_) | Error::WorkdirAttachmentConflict(_) => { StatusCode::CONFLICT } @@ -10020,7 +10407,14 @@ impl IntoResponse for ApiError { Error::InvalidRuntimeIdentifier { .. } | Error::ReservedWorkerName(_) => { StatusCode::BAD_REQUEST } - Error::Ticket(ticket::TicketError::NotFound(_)) => StatusCode::NOT_FOUND, + Error::Ticket(ticket::TicketError::NotFound(_)) + | Error::MergeRequest(merge_request::MergeRequestError::NotFound(_)) => { + StatusCode::NOT_FOUND + } + Error::MergeRequest(merge_request::MergeRequestError::Empty(_)) => { + StatusCode::BAD_REQUEST + } + Error::MergeRequest(_) => StatusCode::CONFLICT, Error::Ticket( ticket::TicketError::Ambiguous { .. } | ticket::TicketError::Locked { .. } @@ -10161,6 +10555,32 @@ mod tests { ObjectiveTicketLinkRecord, SqliteWorkspaceStore, WorkspaceRecord, }; + #[test] + fn merge_confirmation_rejects_api_token_actor_before_session_resolution() { + let mut headers = HeaderMap::new(); + headers.insert("authorization", "Bearer api-token".parse().unwrap()); + assert!(matches!( + reject_non_browser_merge_auth(&headers), + Err(Error::BrowserMergeConfirmationRequired) + )); + assert!(reject_non_browser_merge_auth(&HeaderMap::new()).is_ok()); + } + + #[test] + fn flow_or_generic_worker_state_change_is_not_ticket_completion_authority() { + let operation = TicketBackendOperation::SetWorkflowState { + id: TicketIdOrSlug::Query("T1".to_string()), + change: TicketStateChange::new( + "inprogress", + "done", + "flow reached terminal state", + "terminal flow state", + ), + }; + let error = reject_unguarded_ticket_completion(&operation).unwrap_err(); + assert!(error.to_string().contains("MergeRequestComplete")); + } + #[test] fn failed_api_log_is_structured_and_omits_query_values() { let uri = "/api/w/workspace/tickets?access_token=secret" @@ -12596,21 +13016,6 @@ mod tests { .unwrap(); assert_eq!(queued.state, "queued"); assert_eq!(queued.queued_by.as_deref(), Some("browser-user")); - let Json(reviewed) = scoped_review_ticket( - State(api.clone()), - AxumPath(path()), - Json(BrowserReviewTicketRequest { - result: TicketReviewResult::Approve, - body: "API review".to_string(), - author: Some("reviewer".to_string()), - }), - ) - .await - .unwrap(); - assert!(reviewed.events.iter().any(|event| { - event.kind == "review" && event.body.as_deref() == Some("API review") - })); - let Json(closed) = scoped_close_ticket( State(api), AxumPath(path()), diff --git a/crates/workspace-server/src/store.rs b/crates/workspace-server/src/store.rs index e98b77b2..92cdfd03 100644 --- a/crates/workspace-server/src/store.rs +++ b/crates/workspace-server/src/store.rs @@ -765,6 +765,7 @@ impl SqliteWorkspaceStore { configure_sqlite(&conn)?; apply_migrations(&conn)?; ticket::migrate_sqlite_ticket_schema(&conn)?; + merge_request::migrate(&conn).map_err(|error| Error::Store(error.to_string()))?; validate_workspace_repository_references(&conn)?; Ok(Self { conn: Arc::new(Mutex::new(conn)), diff --git a/crates/yoi/src/ticket_cli.rs b/crates/yoi/src/ticket_cli.rs index c51ca8a8..cad60e55 100644 --- a/crates/yoi/src/ticket_cli.rs +++ b/crates/yoi/src/ticket_cli.rs @@ -12,8 +12,8 @@ use ticket::config::{ use ticket::{ LocalTicketBackend, MarkdownText, NewTicket, NewTicketEvent, NewTicketRelation, SqliteTicketBackend, TicketBackend, TicketDoctorSeverity, TicketEventKind, TicketIdOrSlug, - TicketIntakeSummary, TicketListQuery, TicketListState, TicketRelationKind, TicketReview, - TicketReviewResult, TicketSummary, TicketWorkflowState, + TicketIntakeSummary, TicketListQuery, TicketListState, TicketRelationKind, TicketSummary, + TicketWorkflowState, }; const DEFAULT_LIST_LIMIT: usize = 50; @@ -35,7 +35,6 @@ pub enum TicketCommand { List(ListOptions), Show { query: String }, Comment(CommentOptions), - Review(ReviewOptions), State(StateOptions), Close(CloseOptions), Relation(RelationOptions), @@ -67,13 +66,6 @@ pub struct CommentOptions { pub body: BodySource, } -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct ReviewOptions { - pub query: String, - pub result: TicketReviewResult, - pub body: BodySource, -} - #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum StateTarget { Planning, @@ -194,7 +186,6 @@ pub fn parse_ticket_args(args: &[String]) -> Result { query: parse_one_positional("show", &args[1..])?, }, "comment" => TicketCommand::Comment(parse_comment(&args[1..])?), - "review" => TicketCommand::Review(parse_review(&args[1..])?), "state" => TicketCommand::State(parse_state(&args[1..])?), "close" => TicketCommand::Close(parse_close(&args[1..])?), "relation" => TicketCommand::Relation(parse_relation(&args[1..])?), @@ -249,7 +240,6 @@ fn run_command( TicketCommand::List(options) => list(backend.as_ref(), options), TicketCommand::Show { query } => show(backend.as_ref(), query), TicketCommand::Comment(options) => comment(backend.as_ref(), options), - TicketCommand::Review(options) => review(backend.as_ref(), options), TicketCommand::State(options) => state(backend.as_ref(), options), TicketCommand::Close(options) => close(backend.as_ref(), options), TicketCommand::Relation(options) => relation(backend.as_ref(), options), @@ -633,23 +623,6 @@ fn comment( Ok(success(format!("appended\t{}\t{}\n", options.query, role))) } -fn review( - backend: &dyn TicketBackend, - options: ReviewOptions, -) -> Result { - let result = options.result.as_str().to_string(); - let review = TicketReview { - result: options.result, - author: Some(default_author()), - body: MarkdownText::new(read_body_source(&options.body)?), - }; - backend.review(TicketIdOrSlug::Query(options.query.clone()), review)?; - Ok(success(format!( - "reviewed\t{}\t{}\n", - options.query, result - ))) -} - fn state( backend: &dyn TicketBackend, options: StateOptions, @@ -660,7 +633,11 @@ fn state( StateTarget::Ready => TicketWorkflowState::Ready, StateTarget::Queued => TicketWorkflowState::Queued, StateTarget::InProgress => TicketWorkflowState::InProgress, - StateTarget::Done => TicketWorkflowState::Done, + StateTarget::Done => { + return Err(TicketCliError::new( + "done is guarded by MergeRequestComplete with an approved immutable revision and operation_id", + )); + } StateTarget::Closed => { return Err(TicketCliError::new( "yoi ticket state closed cannot write resolution.md; use `yoi ticket close --resolution ` instead", @@ -965,64 +942,6 @@ fn parse_comment(args: &[String]) -> Result { }) } -fn parse_review(args: &[String]) -> Result { - if args.is_empty() || args[0].starts_with('-') { - return Err(TicketCliError::new("review requires ")); - } - let query = args[0].clone(); - let mut approve = false; - let mut request_changes = false; - let mut file = None; - let mut message = None; - let mut i = 1; - while i < args.len() { - match args[i].as_str() { - "--approve" => { - approve = true; - i += 1; - } - "--request-changes" => { - request_changes = true; - i += 1; - } - _ => match option_with_value(args, &mut i)? { - Some(("--file", value)) => file = Some(PathBuf::from(value)), - Some(("--message", value)) => message = Some(value), - Some((name, _)) => { - return Err(TicketCliError::new(format!( - "unknown review argument: {name}" - ))); - } - None => { - return Err(TicketCliError::new(format!( - "unknown review argument: {}", - args[i] - ))); - } - }, - } - } - let result = match (approve, request_changes) { - (true, false) => TicketReviewResult::Approve, - (false, true) => TicketReviewResult::RequestChanges, - (false, false) => { - return Err(TicketCliError::new( - "review requires exactly one of --approve or --request-changes", - )); - } - (true, true) => { - return Err(TicketCliError::new( - "review accepts exactly one of --approve or --request-changes", - )); - } - }; - Ok(ReviewOptions { - query, - result, - body: exactly_one_body("review", file, message)?, - }) -} - fn parse_state(args: &[String]) -> Result { if args.len() != 2 { return Err(TicketCliError::new( @@ -1244,7 +1163,7 @@ fn default_author() -> String { } fn help_text() -> &'static str { - "yoi ticket\n\nUsage:\n yoi ticket init\n yoi ticket import-local\n yoi ticket create --title \n yoi ticket list [--state active|all|planning|ready|queued|inprogress|done|closed[,..]] [--limit <n>]\n yoi ticket show <id>\n yoi ticket comment <id> [--role comment|plan|decision|implementation_report] (--file <path>|--message <text>)\n yoi ticket review <id> (--approve|--request-changes) (--file <path>|--message <text>)\n yoi ticket state <id> <planning|ready|queued|inprogress|done|closed>\n yoi ticket close <id> (--resolution <text>|--file <path>)\n yoi ticket relation add --ticket <id> --kind <depends_on|blocks|related|supersedes|duplicate_of> --target <id> [--note <text>]\n yoi ticket relation list [--ticket <id>] [--kind <kind>]\n yoi ticket doctor\n\nOptions:\n -h, --help Print help\n\nBackend:\n Tickets are stored in the workspace SQLite DB under the Yoi data directory.\n `yoi ticket import-local` imports the legacy .yoi/tickets backend root configured in .yoi/workspace.toml.\n `yoi ticket init` writes explicit fixed role profiles and optional [ticket].language into .yoi/workspace.toml, but does not create .yoi/tickets.\n" + "yoi ticket\n\nUsage:\n yoi ticket init\n yoi ticket import-local\n yoi ticket create --title <title>\n yoi ticket list [--state active|all|planning|ready|queued|inprogress|done|closed[,..]] [--limit <n>]\n yoi ticket show <id>\n yoi ticket comment <id> [--role comment|plan|decision|implementation_report] (--file <path>|--message <text>)\n yoi ticket state <id> <planning|ready|queued|inprogress|closed>\n yoi ticket close <id> (--resolution <text>|--file <path>)\n yoi ticket relation add --ticket <id> --kind <depends_on|blocks|related|supersedes|duplicate_of> --target <id> [--note <text>]\n yoi ticket relation list [--ticket <id>] [--kind <kind>]\n yoi ticket doctor\n\nOptions:\n -h, --help Print help\n\nBackend:\n Tickets are stored in the workspace SQLite DB under the Yoi data directory.\n `yoi ticket import-local` imports the legacy .yoi/tickets backend root configured in .yoi/workspace.toml.\n `yoi ticket init` writes explicit fixed role profiles and optional [ticket].language into .yoi/workspace.toml, but does not create .yoi/tickets.\n" } #[cfg(test)] @@ -1375,7 +1294,7 @@ mod tests { } #[test] - fn ticket_cli_create_list_show_comment_review_state_close_and_doctor() { + fn ticket_cli_create_list_show_comment_state_close_and_doctor() { let temp = TempDir::new().unwrap(); let created = run(&temp, &["create", "--title", "CLI Created"]); @@ -1416,22 +1335,6 @@ mod tests { .contains(&format!("appended\t{}\timplementation_report", ticket_id)) ); - let reviewed = run( - &temp, - &[ - "review", - &ticket_id, - "--approve", - "--message", - "Looks good.", - ], - ); - assert!( - reviewed - .stdout - .contains(&format!("reviewed\t{}\tapprove", ticket_id)) - ); - let ready = run(&temp, &["state", &ticket_id, "ready"]); assert_eq!(ready.stdout, format!("state\t{}\tready\n", ticket_id)); let ready_listed = run(&temp, &["list", "--state", "ready"]); @@ -1450,10 +1353,10 @@ mod tests { let inprogress_listed = run(&temp, &["list", "--state", "inprogress"]); assert!(inprogress_listed.stdout.contains(&ticket_id)); - let done = run(&temp, &["state", &ticket_id, "done"]); - assert_eq!(done.stdout, format!("state\t{}\tdone\n", ticket_id)); - let done_listed = run(&temp, &["list", "--state", "done"]); - assert!(done_listed.stdout.contains(&ticket_id)); + let done_error = parse_ticket_args(&args(&["state", &ticket_id, "done"])) + .and_then(|cli| run_in_workspace(cli, temp.path())) + .unwrap_err(); + assert!(done_error.to_string().contains("MergeRequestComplete")); let closed = run( &temp, @@ -1469,7 +1372,6 @@ mod tests { assert!(final_show.stdout.contains("State: closed")); assert!(final_show.stdout.contains("Done via yoi ticket.")); assert!(final_show.stdout.contains("implementation_report")); - assert!(final_show.stdout.contains("review")); } #[test] @@ -1594,20 +1496,6 @@ mod tests { assert!(err.to_string().contains("exactly one")); } - #[test] - fn ticket_cli_rejects_ambiguous_review_result() { - let err = parse_ticket_args(&args(&[ - "review", - "ticket", - "--approve", - "--request-changes", - "--message", - "body", - ])) - .unwrap_err(); - assert!(err.to_string().contains("exactly one")); - } - #[test] fn ticket_cli_state_closed_requires_close_command() { let temp = TempDir::new().unwrap(); diff --git a/docs/development/work-items.md b/docs/development/work-items.md index 940ac58f..fdd78af5 100644 --- a/docs/development/work-items.md +++ b/docs/development/work-items.md @@ -24,7 +24,7 @@ Use the highest-level interface that matches the work: - Use `yoi panel` for the Ticket/Intake/Orchestrator workspace Dashboard and role-launch actions. - Use `yoi objective ...` for lightweight medium-term Objective records and their non-blocking canonical Ticket links. -- Inside Workers, use typed Ticket tools to create, inspect, comment, review, and close Tickets. +- Inside Workers, use typed Ticket tools for Ticket records and typed Merge Request tools for immutable implementation/review/completion evidence. - For multi-step work, follow the typed Ticket role surfaces and recorded Ticket lifecycle gates. Maintainers can inspect the local `.yoi/tickets/` files directly when debugging storage, but normal user instructions should go through `yoi panel`, Ticket tools, or `yoi ticket ...`. @@ -37,8 +37,8 @@ Workers with the Ticket built-in feature can use typed Ticket tools: - `TicketList` — lightweight bounded overview for selecting ids; it returns short summaries only and must not be used as body/thread/artifact authority. - `TicketShow` — detailed authority for a single Ticket, including body/thread/artifact metadata/resolution context subject to its own bounds. - `TicketComment` -- `TicketReview` -- `TicketWorkflowState` +- `MergeRequestShow`, `MergeRequestOpen`, `MergeRequestAddRevision`, `MergeRequestComplete` +- `MergeRequestReviewSubmit` — available only inside the attested direct-child Reviewer attempt; attempt/revision capability material is not model input. - `TicketClose` - `TicketRelationRecord` - `TicketRelationQuery` @@ -52,7 +52,7 @@ Use them when a Worker needs to materialize or update project records: - Intake creates a new Ticket after user agreement. - Orchestrator records routing decisions and intent packets. -- Reviewer records approve/request-changes review results. +- Reviewer commits an approve/request-changes result against one immutable Merge Request revision. - Maintainer closes a Ticket with a resolution when merge/validation/cleanup evidence is complete. Do not bypass Ticket lifecycle gates just because Ticket tools are available. Ticket mutation is a project-record operation and should remain auditable. @@ -241,9 +241,9 @@ Implementation normally happens in a child git worktree created by the Orchestra ### 5. Review -Reviewer Workers should be sibling Workers, not children of coder Workers. They should read the Ticket, intent packet, diff, implementation report, and validation evidence. +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. -Review results should be recorded with the `TicketReview` tool. Maintainers working directly with the local backend can use the `yoi ticket` CLI documented later. +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. 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 ec4abdcd..c39db17c 100644 --- a/resources/flows/coder-review.dcdl +++ b/resources/flows/coder-review.dcdl @@ -5,7 +5,7 @@ states = { implement = { - instructions = "Implement the requested Ticket scope, run the narrow and dependent validation required by the changed contracts, and record the concrete repository/test evidence. When the implementation is ready for independent review, request a Flow transition."; + instructions = "Open or update the Ticket Merge Request with immutable repository revision evidence, run the narrow and dependent validation required by the changed contracts, and record concrete evidence. When the current MR revision is ready for independent review, request a Flow transition. A Flow transition is never Ticket completion authority."; transitions = { review = { target = "review"; @@ -15,11 +15,11 @@ }; review = { - instructions = "Spawn one independent Reviewer SubWorker with bounded Ticket, repository, diff, and validation context. Read its committed review through worker observation. Do not review your own implementation or treat a prose status as approval. After the Reviewer returns a typed approval or concrete requested changes, 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 MergeRequestReviewSubmit; prose output and Worker observation are not approval authority. After the structured current-revision result exists, request a Flow transition."; transitions = { approved = { - target = "done"; - condition = "The latest independent Reviewer attempt for the current implementation completed and approved it, with no later unresolved request_changes finding."; + target = "complete"; + condition = "The authoritative Merge Request current revision has a structured approve result from its registered direct-child builtin:reviewer attempt, with no later unresolved request_changes finding. The Flow transition itself does not complete the Ticket."; }; changes_requested = { target = "fix"; @@ -38,8 +38,18 @@ }; }; + 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."; + 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."; + }; + }; + }; + done = { - instructions = "The Coder implementation and independent review loop is complete."; + instructions = "The guarded Merge Request completion operation committed Ticket state done. Flow terminal state only reflects that durable authority."; terminal = true; }; }; diff --git a/resources/profiles/reviewer.dcdl b/resources/profiles/reviewer.dcdl index d3938983..e86a7406 100644 --- a/resources/profiles/reviewer.dcdl +++ b/resources/profiles/reviewer.dcdl @@ -9,6 +9,6 @@ import "./base.dcdl" // { web = { enabled = true; }; sub_worker = { enabled = false; }; worker = { enabled = false; }; - ticket = { enabled = true; thread = true; }; + ticket = { enabled = true; thread = false; }; }; } diff --git a/resources/prompts/role/coder.md b/resources/prompts/role/coder.md index df051409..3cd33586 100644 --- a/resources/prompts/role/coder.md +++ b/resources/prompts/role/coder.md @@ -1,7 +1,7 @@ -You are the Ticket Coder role. +You are the assigned Coder. Implement the requested scope in the provided Workdir and keep durable evidence on the Ticket and its Merge Request. -Keep role behavior here and treat the first committed user message as concrete Ticket/action context only. Implement only within the delegated worktree/branch and authority scope. Treat the Ticket, intent packet, binding decisions/invariants, implementation latitude, validation expectations, and report expectations as the contract. +Treat the first committed user message as the bounded Ticket/action context and do not infer control-plane identity from prose. -Choose local implementation tactics within that contract. Escalate to the Orchestrator instead of expanding scope when design, permission, dependency, prompt-boundary, or Ticket-boundary questions appear. Do not merge, push, close Tickets, delete worktrees, or create generated memory/local/runtime/log/lock/cache/socket/secret-like `.yoi` state. +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. -Keep the repository operational throughout the work unless the Ticket explicitly permits a bounded incomplete state. Report the implementation and proportionate validation through the available typed Ticket tools; do not edit Ticket storage directly. +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. diff --git a/resources/prompts/role/reviewer.md b/resources/prompts/role/reviewer.md index c23429e7..67334119 100644 --- a/resources/prompts/role/reviewer.md +++ b/resources/prompts/role/reviewer.md @@ -1,7 +1,7 @@ -You are the Ticket Reviewer role. +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 concrete Ticket/action context only. Review the implementation against the Ticket intent, binding decisions/invariants, acceptance criteria, and project design boundaries. Prefer read-only inspection and focused validation; do not merge, close, clean up worktrees, or take over implementation unless explicitly asked. +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. -Report clear approve/request-changes evidence with risks, validation performed, and any unresolved requirement or design-boundary concern. When a workflow is invoked, follow that workflow as the procedural authority for reviewer handoff and report shape. +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. -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 behavior or compatibility. +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/lib/workspace/console/worker-console.ui.test.ts b/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts index 9108d2be..0550e7f9 100644 --- a/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts +++ b/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts @@ -224,7 +224,9 @@ Deno.test("workspace Tickets surface provides Kanban and lifecycle controls", as ticketDetailLoad.includes("/repositories") && ticketDetailPage.includes('mutate("state", "/state"') && ticketDetailPage.includes('mutate("queue", "/queue"') && - ticketDetailPage.includes('mutate("review", "/review"') && + ticketDetailPage.includes("/merge-request/merge") && + ticketDetailPage.includes("explicit_confirmation: true") && + !ticketDetailPage.includes('mutate("review", "/review"') && ticketDetailPage.includes('mutate("close", "/close"') && ticketDetailPage.includes("ticketWorkerLaunchHref") && ticketDetailPage.includes("ticket.relations.outgoing"), 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 ed096582..0f808103 100644 --- a/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.svelte +++ b/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.svelte @@ -17,6 +17,16 @@ TicketDetail, } from "$lib/workspace/sidebar/types"; + type MergeRequestDetail = { + state: "draft" | "open" | "closed" | "merged"; + review_status: "pending" | "approved" | "changes_requested"; + current_revision: { revision_id: string; head_commit: string; head_tree: string; diff_digest: string; changed_paths: string[]; summary: string }; + current_review?: { decision: string; body: string; reviewer_effective_profile: string } | null; + merged_at?: string | null; + }; + + const MUTABLE_TICKET_STATES = TICKET_STATES.filter((state) => state !== "done"); + const { data } = $props<{ data: { workspaceId: string; @@ -24,6 +34,7 @@ ticket: ApiResult<TicketDetail>; repositories: ApiResult<RepositoryListResponse>; orchestrator: ApiResult<WorkspaceOrchestratorStatus>; + mergeRequest: ApiResult<MergeRequestDetail | null>; }; }>(); @@ -34,6 +45,7 @@ const orchestratorOnline = initialData.orchestrator.data?.online ?? false; let ticket = $state<TicketDetail>(loadedTicket); + let mergeRequest = $state<MergeRequestDetail | null>(initialData.mergeRequest.data ?? null); let editing = $state(false); let editTitle = $state(loadedTicket.title); let editBody = $state(loadedTicket.body); @@ -43,8 +55,7 @@ let transitionReason = $state(""); let threadRole = $state("comment"); let threadBody = $state(""); - let reviewResult = $state("approve"); - let reviewBody = $state(""); + let confirmMerge = $state(false); let resolution = $state(""); let busy = $state<string | null>(null); let errorMessage = $state<string | null>(null); @@ -134,15 +145,27 @@ ) threadBody = ""; } - async function review(event: SubmitEvent) { - event.preventDefault(); - if (!reviewBody.trim()) return; - if ( - await mutate("review", "/review", { - result: reviewResult, - body: reviewBody.trim(), - }) - ) reviewBody = ""; + async function mergeConfirmedRevision() { + if (!mergeRequest || !confirmMerge || busy) return; + busy = "merge"; + errorMessage = null; + try { + mergeRequest = await workspaceApiJsonWithBody<MergeRequestDetail>( + `${ticketPath}/merge-request/merge`, + { + method: "POST", + body: JSON.stringify({ + expected_revision_id: mergeRequest.current_revision.revision_id, + explicit_confirmation: true, + }), + }, + ); + confirmMerge = false; + } catch (error) { + errorMessage = error instanceof Error ? error.message : String(error); + } finally { + busy = null; + } } async function closeTicket(event: SubmitEvent) { @@ -277,7 +300,6 @@ <p>The Orchestrator is online. Start a role-specific Worker with the Ticket target below.</p> <div class="ticket-role-actions"> <a class="workspace-primary-button" href={ticketWorkerLaunchHref(data.workspaceId, ticket, "coder")}>Coder</a> - <a class="workspace-secondary-button" href={ticketWorkerLaunchHref(data.workspaceId, ticket, "reviewer")}>Reviewer</a> </div> {:else} <p class="workspace-callout">Start the Workspace Orchestrator from the Ticket panel before launching Ticket Workers.</p> @@ -311,7 +333,7 @@ <form class="ticket-control-form" onsubmit={transition}> <label>State <select bind:value={nextState}> - {#each TICKET_STATES as state}<option value={state}>{state}</option>{/each} + {#each MUTABLE_TICKET_STATES as state}<option value={state}>{state}</option>{/each} </select> </label> <label>Reason<input bind:value={transitionReason} placeholder="Optional decision context" /></label> @@ -340,17 +362,27 @@ </form> </details> - <details class="ticket-control-card"> - <summary>Record review</summary> - <form class="ticket-control-form" onsubmit={review}> - <label>Result<select bind:value={reviewResult}> - <option value="approve">Approve</option> - <option value="request_changes">Request changes</option> - </select></label> - <label>Review body<textarea bind:value={reviewBody} rows="5" required></textarea></label> - <button class="workspace-secondary-button" type="submit" disabled={busy === "review" || !reviewBody.trim()}>Record review</button> - </form> - </details> + <section class="ticket-control-card"> + <header><h2>Merge Request</h2></header> + {#if data.mergeRequest.error} + <p class="workspace-callout is-error">{data.mergeRequest.error}</p> + {:else if mergeRequest} + <p><strong>{mergeRequest.state}</strong> · {mergeRequest.review_status}</p> + <p><code>{mergeRequest.current_revision.revision_id}</code></p> + <p>Head <code>{mergeRequest.current_revision.head_commit}</code></p> + {#if mergeRequest.current_revision.summary}<p>{mergeRequest.current_revision.summary}</p>{/if} + {#if mergeRequest.current_review} + <p><strong>{mergeRequest.current_review.decision}</strong> by {mergeRequest.current_review.reviewer_effective_profile}</p> + {#if mergeRequest.current_review.body}<RichMarkdown text={mergeRequest.current_review.body} />{/if} + {/if} + {#if mergeRequest.state === "open" && mergeRequest.review_status === "approved"} + <label><input type="checkbox" bind:checked={confirmMerge} /> Explicitly confirm merge of this revision</label> + <button class="workspace-primary-button" type="button" disabled={!confirmMerge || busy !== null} onclick={mergeConfirmedRevision}>Confirm merge</button> + {/if} + {:else} + <p class="workspace-empty-copy">The assigned Coder has not opened a Merge Request.</p> + {/if} + </section> {#if ticket.state !== "closed"} <details class="ticket-control-card ticket-close-card"> diff --git a/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.ts b/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.ts index 14fa8c45..a5dc7973 100644 --- a/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.ts +++ b/web/workspace/src/routes/w/[workspaceId]/tickets/[ticketId]/+page.ts @@ -1,35 +1,26 @@ import { loadJson, workspaceApiPath } from "$lib/workspace/api/http"; import type { WorkspaceOrchestratorStatus } from "$lib/workspace/tickets/ticket-panel"; -import type { - RepositoryListResponse, - TicketDetail, -} from "$lib/workspace/sidebar/types"; +import type { RepositoryListResponse, TicketDetail } from "$lib/workspace/sidebar/types"; import type { PageLoad } from "./$types"; -export const load = (async ({ fetch, params }) => { - const [ticket, repositories, orchestrator] = await Promise.all([ - loadJson<TicketDetail>( - fetch, - workspaceApiPath( - params.workspaceId, - `/tickets/${encodeURIComponent(params.ticketId)}`, - ), - ), - loadJson<RepositoryListResponse>( - fetch, - workspaceApiPath(params.workspaceId, "/repositories"), - ), - loadJson<WorkspaceOrchestratorStatus>( - fetch, - workspaceApiPath(params.workspaceId, "/orchestrator"), - ), - ]); +async function loadOptionalJson<T>(fetcher: typeof fetch, path: string): Promise<{ data: T | null; error: string | null }> { + try { + const response = await fetcher(path); + if (response.status === 404) return { data: null, error: null }; + if (!response.ok) return { data: null, error: await response.text() || `HTTP ${response.status}` }; + return { data: await response.json() as T, error: null }; + } catch (error) { + return { data: null, error: error instanceof Error ? error.message : String(error) }; + } +} - return { - workspaceId: params.workspaceId, - ticketId: params.ticketId, - ticket, - repositories, - orchestrator, - }; +export const load = (async ({ fetch, params }) => { + const ticketPath = workspaceApiPath(params.workspaceId, `/tickets/${encodeURIComponent(params.ticketId)}`); + const [ticket, repositories, orchestrator, mergeRequest] = await Promise.all([ + loadJson<TicketDetail>(fetch, ticketPath), + loadJson<RepositoryListResponse>(fetch, workspaceApiPath(params.workspaceId, "/repositories")), + loadJson<WorkspaceOrchestratorStatus>(fetch, workspaceApiPath(params.workspaceId, "/orchestrator")), + loadOptionalJson<Record<string, unknown>>(fetch, `${ticketPath}/merge-request`), + ]); + return { workspaceId: params.workspaceId, ticketId: params.ticketId, ticket, repositories, orchestrator, mergeRequest }; }) satisfies PageLoad;