From c6a476d65d60ae1fa0dc14a771176efc368aa95f Mon Sep 17 00:00:00 2001 From: Hare Date: Mon, 17 Aug 2026 21:26:42 +0900 Subject: [PATCH] feat: derive ticket readiness from merge requests --- crates/flow/src/builtin.rs | 9 +- crates/worker/src/feature/builtin/ticket.rs | 14 +- crates/worker/src/prompt/catalog.rs | 8 + crates/workspace-server/src/authority.rs | 631 +++++++++++++----- crates/workspace-server/src/lib.rs | 3 +- crates/workspace-server/src/records.rs | 10 +- crates/workspace-server/src/server.rs | 19 +- resources/flows/coder-review.dcdl | 4 +- resources/prompts/common/tickets.md | 2 +- resources/prompts/role/coder.md | 2 +- resources/prompts/role/orchestrator.md | 2 + web/workspace/src/lib/generated/ticket-api.ts | 10 +- 12 files changed, 507 insertions(+), 207 deletions(-) diff --git a/crates/flow/src/builtin.rs b/crates/flow/src/builtin.rs index 0f40b52e..5fac24a8 100644 --- a/crates/flow/src/builtin.rs +++ b/crates/flow/src/builtin.rs @@ -24,7 +24,7 @@ pub fn builtin_flow_source(slug: &str) -> Option { match slug { CODER_REVIEW_FLOW_SLUG => Some(BuiltinFlowSource { slug: CODER_REVIEW_FLOW_SLUG, - revision: 2, + revision: 3, path: "builtin/flows/coder-review.dcdl", content: CODER_REVIEW_FLOW_SOURCE, }), @@ -35,7 +35,7 @@ pub fn builtin_flow_source(slug: &str) -> Option { pub fn builtin_flow_sources() -> &'static [BuiltinFlowSource] { const SOURCES: &[BuiltinFlowSource] = &[BuiltinFlowSource { slug: CODER_REVIEW_FLOW_SLUG, - revision: 2, + revision: 3, path: "builtin/flows/coder-review.dcdl", content: CODER_REVIEW_FLOW_SOURCE, }]; @@ -69,7 +69,7 @@ mod tests { } #[test] - fn coder_review_starts_on_a_ticket_branch_and_requires_committed_review_evidence() { + fn coder_review_preserves_branch_policy_and_hands_approval_to_orchestrator() { let source = builtin_flow_source(CODER_REVIEW_FLOW_SLUG).expect("coder-review Flow must exist"); @@ -89,5 +89,8 @@ mod tests { "coder-review Flow must preserve branch/commit policy token {required:?}" ); } + assert!(source.content.contains("hand off to the Orchestrator")); + assert!(source.content.contains("Do not call MergeRequestComplete")); + assert!(!source.content.contains("Call MergeRequestComplete with")); } } diff --git a/crates/worker/src/feature/builtin/ticket.rs b/crates/worker/src/feature/builtin/ticket.rs index ac461c13..a44e37ca 100644 --- a/crates/worker/src/feature/builtin/ticket.rs +++ b/crates/worker/src/feature/builtin/ticket.rs @@ -54,7 +54,7 @@ impl WorkspaceTicketReadKind { "Query authoritative Workspace Tickets with bounded typed filters, stable snippets, evidence summaries, and cursor metadata." } Self::Show => { - "Show one authoritative Workspace Ticket with its item revision, paged thread, links, implementation reports, and current Merge Request review evidence." + "Show one authoritative Workspace Ticket with its item revision, paged thread, links, historical implementation reports, and current Merge Request readiness evidence." } } } @@ -83,8 +83,6 @@ enum WorkspaceTicketStateFilter { #[derive(Debug, Deserialize, Serialize, JsonSchema)] #[serde(rename_all = "snake_case")] enum WorkspaceTicketEvidenceFilter { - ImplementationReport, - ImplementationReportAfterRescope, MergeRequest, Commit, ApprovedReview, @@ -104,8 +102,6 @@ enum WorkspaceTicketReviewFilter { #[serde(rename_all = "snake_case")] enum WorkspaceTicketAttentionFilter { DoneNotClosed, - ImplementationReportNotClosed, - ReportAfterRescope, UnresolvedReview, MissingCommit, Blocked, @@ -147,15 +143,15 @@ struct WorkspaceQueryTicketInput { /// Exact typed event kinds that must occur in the bounded thread window. #[serde(default)] event_kinds: Vec, - /// Required evidence kinds: implementation_report, implementation_report_after_rescope, - /// merge_request, commit, or approved_review. + /// Required evidence kinds: merge_request, commit, or approved_review. #[serde(default)] evidence: Vec, /// Current authoritative Merge Request review status: none, pending, approved, /// request_changes, or unresolved_changes. review_status: Option, - /// Attention filters include done_not_closed, implementation_report_not_closed, - /// report_after_rescope, unresolved_review, missing_commit, blocked, and unblocked. + /// Attention filters include done_not_closed, unresolved_review, missing_commit, + /// blocked, unblocked, ready, awaiting_review, unresolved_changes, + /// stale_after_rescope, and missing_evidence. #[serde(default)] attention: Vec, related_ticket_id: Option, diff --git a/crates/worker/src/prompt/catalog.rs b/crates/worker/src/prompt/catalog.rs index 82518b90..614ffd26 100644 --- a/crates/worker/src/prompt/catalog.rs +++ b/crates/worker/src/prompt/catalog.rs @@ -632,6 +632,14 @@ mod tests { "Do not spawn, restore, assign, or route work to Backend/Runtime Reviewer Workers" )); assert!(prompt.contains("never compensate by creating an independent Reviewer Worker")); + assert!( + prompt.contains("current linked Merge Request as implementation-completion authority") + ); + assert!(prompt.contains("do not require an `implementation_report`")); + assert!(prompt.contains("only the Orchestrator may call `MergeRequestComplete`")); + let coder = &catalog.projection.templates["role.coder"]; + assert!(coder.contains("hand off to the Orchestrator")); + assert!(coder.contains("Do not call `MergeRequestComplete`")); assert!(!prompt.contains("sibling Coder/Reviewer Workers")); } diff --git a/crates/workspace-server/src/authority.rs b/crates/workspace-server/src/authority.rs index 9070db63..c0c09566 100644 --- a/crates/workspace-server/src/authority.rs +++ b/crates/workspace-server/src/authority.rs @@ -1,6 +1,6 @@ use std::{path::PathBuf, sync::Arc}; -use chrono::Utc; +use chrono::{DateTime, Utc}; use merge_request::{ MergeRequest, MergeRequestError, MergeRequestState, MergeRequestStore, MergeRequestThreadEvent, ReviewDecision, @@ -167,12 +167,25 @@ impl merge_request::RepositorySource for AuthorityMergeRequestSource { } } +pub trait TicketMergeRevisionSource: Send + Sync { + fn resolve_subject_ref(&self, repository_id: &str, selector: &str) -> Option; +} + +struct UnresolvedTicketMergeRevisionSource; + +impl TicketMergeRevisionSource for UnresolvedTicketMergeRevisionSource { + fn resolve_subject_ref(&self, _repository_id: &str, _selector: &str) -> Option { + None + } +} + #[derive(Clone)] pub struct SqliteWorkspaceAuthority { workspace_id: String, store: SqliteWorkspaceStore, ticket_backend: SqliteTicketBackend, merge_request_store: Arc, + merge_revision_source: Arc, } impl SqliteWorkspaceAuthority { @@ -198,9 +211,18 @@ impl SqliteWorkspaceAuthority { ) .map_err(|error| Error::Store(error.to_string()))?, ), + merge_revision_source: Arc::new(UnresolvedTicketMergeRevisionSource), }) } + pub fn with_merge_revision_source( + mut self, + merge_revision_source: Arc, + ) -> Self { + self.merge_revision_source = merge_revision_source; + self + } + fn objective_record(&self, id: &str) -> Result { self.store .get_objective(&self.workspace_id, id)? @@ -367,11 +389,21 @@ impl SqliteWorkspaceAuthority { worker_id: assignment.worker.worker_id, }); let merge_request = match self.merge_request_store.get(&self.workspace_id, id) { - Ok(request) => Some(merge_request_summary(request)), + Ok(request) => { + let current_subject_ref = request.selector_from.as_deref().and_then(|selector| { + self.merge_revision_source + .resolve_subject_ref(&request.repository_id, selector) + }); + Some(merge_request_summary(request, current_subject_ref)) + } Err(MergeRequestError::NotFound) => None, Err(error) => return Err(Error::Store(error.to_string())), }; - let evidence = ticket_evidence_summary(&ticket.events, merge_request.as_ref()); + let evidence = ticket_evidence_summary( + ticket.meta.repository_id.as_deref(), + &ticket.events, + merge_request.as_ref(), + ); let item_revision = ticket .events .iter() @@ -1023,17 +1055,45 @@ fn ticket_evidence_event(sequence: usize, event: &TicketEvent) -> TicketEvidence } } -fn merge_request_summary(request: MergeRequest) -> TicketMergeRequestSummary { - let review_subject_ref = request.thread.iter().rev().find_map(|event| match event { - MergeRequestThreadEvent::ReviewRequested(review) => Some(review.subject_ref.as_str()), +fn merge_request_summary( + request: MergeRequest, + current_subject_ref: Option, +) -> TicketMergeRequestSummary { + let latest_review_request = request.thread.iter().rev().find_map(|event| match event { + MergeRequestThreadEvent::ReviewRequested(review) => Some(review), _ => None, }); - let current_review = - review_subject_ref.and_then(|subject_ref| request.effective_review(subject_ref)); + let current_review = current_subject_ref + .as_deref() + .and_then(|subject_ref| request.effective_review(subject_ref)); + let current_review_request = current_review + .and_then(|review| { + request.thread.iter().find_map(|event| match event { + MergeRequestThreadEvent::ReviewRequested(review_request) + if review_request.event_id == review.request_event_id => + { + Some(review_request) + } + _ => None, + }) + }) + .or_else(|| { + current_subject_ref.as_deref().and_then(|subject_ref| { + request.thread.iter().rev().find_map(|event| match event { + MergeRequestThreadEvent::ReviewRequested(review) + if review.subject_ref == subject_ref => + { + Some(review) + } + _ => None, + }) + }) + }); let review_status = match current_review.map(|review| &review.decision) { Some(ReviewDecision::Approve) => "approved", Some(ReviewDecision::RequestChanges) => "changes_requested", - None => "pending", + None if latest_review_request.is_some() => "pending", + None => "none", } .to_string(); let state = match request.state { @@ -1045,68 +1105,132 @@ fn merge_request_summary(request: MergeRequest) -> TicketMergeRequestSummary { TicketMergeRequestSummary { merge_request_id: request.merge_request_id.clone(), + repository_id: request.repository_id.clone(), state, review_status, selector_from: request.selector_from.clone(), selector_to: request.selector_to.clone(), updated_at: request.updated_at.to_rfc3339(), - review_subject_ref: review_subject_ref.map(str::to_string), + current_subject_ref, + review_subject_ref: latest_review_request.map(|review| review.subject_ref.clone()), + review_requested_at: current_review_request.map(|review| review.created_at.to_rfc3339()), review_submitted_at: current_review.map(|review| review.created_at.to_rfc3339()), review_excerpt: current_review.map(|review| truncate_body(&review.body, 240).0), } } +fn substantive_item_edit(event: &TicketEvent) -> bool { + if event.kind.as_str() != "item_edit" { + return false; + } + let Some(changes) = event.attributes.get("changes") else { + // Legacy item-edit events do not identify changed fields. Treat them as + // substantive so readiness fails closed rather than accepting a stale review. + return true; + }; + changes + .split(',') + .map(str::trim) + .any(|field| matches!(field, "title" | "body" | "target")) +} + +fn event_timestamp(event: &TicketEvent) -> Option> { + event + .at + .as_deref() + .and_then(|value| DateTime::parse_from_rfc3339(value).ok()) + .map(|value| value.with_timezone(&Utc)) +} + fn ticket_evidence_summary( + ticket_repository_id: Option<&str>, events: &[TicketEvent], merge_request: Option<&TicketMergeRequestSummary>, ) -> TicketEvidenceSummary { - let latest_report = events - .iter() - .enumerate() - .rev() - .find(|(_, event)| event.kind.as_str() == "implementation_report") - .map(|(sequence, _)| sequence); - let latest_rescope = events - .iter() - .enumerate() - .rev() - .find(|(_, event)| event.kind.as_str() == "item_edit") - .map(|(sequence, _)| sequence); - let report_after_rescope = - latest_report.is_some_and(|report| latest_rescope.is_none_or(|rescope| report > rescope)); - let has_commit = merge_request.is_some_and(|request| { + let linked_merge_request = merge_request.filter(|request| { + request.state == "open" + && ticket_repository_id + .is_some_and(|repository_id| repository_id == request.repository_id) + }); + let has_merge_request = linked_merge_request.is_some(); + let has_current_subject_ref = linked_merge_request.is_some_and(|request| { + request + .current_subject_ref + .as_deref() + .is_some_and(|subject_ref| !subject_ref.is_empty()) + }); + let has_review_request = linked_merge_request.is_some_and(|request| { request .review_subject_ref - .as_ref() + .as_deref() .is_some_and(|subject_ref| !subject_ref.is_empty()) - }) || events.iter().any(|event| { - event.attributes.contains_key("commit") || event.attributes.contains_key("head_commit") }); - let review_status = merge_request.map(|request| request.review_status.clone()); - let approved = review_status.as_deref() == Some("approved"); + let has_commit = has_current_subject_ref; + let review_status = linked_merge_request.map(|request| request.review_status.clone()); + let approved_current_subject = review_status.as_deref() == Some("approved"); let unresolved_request_changes = review_status.as_deref() == Some("changes_requested"); + let latest_rescope = events + .iter() + .filter(|event| substantive_item_edit(event)) + .last(); + let review_after_rescope = approved_current_subject + && latest_rescope.is_none_or(|rescope| { + let Some(rescope_at) = event_timestamp(rescope) else { + return false; + }; + let Some(requested_at) = linked_merge_request + .and_then(|request| request.review_requested_at.as_deref()) + .and_then(|value| DateTime::parse_from_rfc3339(value).ok()) + .map(|value| value.with_timezone(&Utc)) + else { + return false; + }; + let Some(reviewed_at) = linked_merge_request + .and_then(|request| request.review_submitted_at.as_deref()) + .and_then(|value| DateTime::parse_from_rfc3339(value).ok()) + .map(|value| value.with_timezone(&Utc)) + else { + return false; + }; + requested_at > rescope_at && reviewed_at > rescope_at + }); + let mut missing = Vec::new(); - if latest_report.is_none() { - missing.push("implementation_report".to_string()); - } else if !report_after_rescope { - missing.push("implementation_report_after_rescope".to_string()); + match merge_request { + None => missing.push("merge_request".to_string()), + Some(request) if request.state != "open" => missing.push("open_merge_request".to_string()), + Some(request) + if ticket_repository_id + .is_none_or(|repository_id| repository_id != request.repository_id) => + { + missing.push("merge_request_repository".to_string()) + } + Some(_) => {} + } + if !has_current_subject_ref { + missing.push("current_subject_ref".to_string()); } if !has_commit { missing.push("commit".to_string()); } - if merge_request.is_none() { - missing.push("merge_request".to_string()); + if unresolved_request_changes { + missing.push("unresolved_request_changes".to_string()); } - if !approved { - missing.push("approved_review".to_string()); + if !approved_current_subject { + missing.push("approved_current_subject".to_string()); } + if approved_current_subject && !review_after_rescope { + missing.push("review_after_rescope".to_string()); + } + TicketEvidenceSummary { - has_implementation_report: latest_report.is_some(), - implementation_report_after_rescope: report_after_rescope, - has_merge_request: merge_request.is_some(), + has_merge_request, + has_current_subject_ref, + has_review_request, has_commit, review_status, - approved, + approved_current_subject, + review_after_rescope, unresolved_request_changes, complete_for_integration: missing.is_empty(), missing, @@ -1124,11 +1248,7 @@ fn validate_ticket_query(query: &TicketQueryRequest) -> Result<()> { for evidence in &query.evidence { if !matches!( evidence.as_str(), - "implementation_report" - | "implementation_report_after_rescope" - | "merge_request" - | "commit" - | "approved_review" + "merge_request" | "commit" | "approved_review" ) { return Err(Error::InvalidRecordId(format!( "unsupported Ticket evidence filter `{evidence}`" @@ -1139,8 +1259,6 @@ fn validate_ticket_query(query: &TicketQueryRequest) -> Result<()> { if !matches!( attention.as_str(), "done_not_closed" - | "implementation_report_not_closed" - | "report_after_rescope" | "unresolved_review" | "missing_commit" | "blocked" @@ -1260,6 +1378,41 @@ fn parse_query_cursor(cursor: &str) -> Result<(String, String)> { Ok((value[..length].to_string(), value[length..].to_string())) } +fn ticket_evidence_matches(evidence: &TicketEvidenceSummary, filter: &str) -> bool { + match filter { + "merge_request" => evidence.has_merge_request, + "commit" => evidence.has_commit, + "approved_review" => evidence.approved_current_subject, + _ => false, + } +} + +fn ticket_attention_matches( + state: &str, + evidence: &TicketEvidenceSummary, + is_blocked: bool, + authoritative_events: &[TicketEvent], + attention: &str, +) -> bool { + match attention { + "done_not_closed" => state == "done", + "unresolved_review" => evidence.unresolved_request_changes, + "missing_commit" => !evidence.has_commit, + "blocked" => is_blocked, + "unblocked" => !is_blocked, + "ready" => state == "ready" && !is_blocked, + "awaiting_review" => { + evidence.has_review_request && evidence.review_status.as_deref() == Some("pending") + } + "unresolved_changes" => evidence.unresolved_request_changes, + "stale_after_rescope" => { + authoritative_events.iter().any(substantive_item_edit) && !evidence.review_after_rescope + } + "missing_evidence" => !evidence.complete_for_integration, + _ => false, + } +} + fn ticket_matches_query( summary: &TicketSummary, detail: &TicketDetail, @@ -1293,7 +1446,10 @@ fn ticket_matches_query( } if let Some(review_status) = &query.review_status { let matches = match review_status.as_str() { - "none" => detail.evidence.review_status.is_none(), + "none" => matches!( + detail.evidence.review_status.as_deref(), + None | Some("none") + ), "request_changes" | "unresolved_changes" => { detail.evidence.review_status.as_deref() == Some("changes_requested") } @@ -1306,43 +1462,19 @@ fn ticket_matches_query( if !query .evidence .iter() - .all(|evidence| match evidence.as_str() { - "implementation_report" => detail.evidence.has_implementation_report, - "implementation_report_after_rescope" => { - detail.evidence.implementation_report_after_rescope - } - "merge_request" => detail.evidence.has_merge_request, - "commit" => detail.evidence.has_commit, - "approved_review" => detail.evidence.approved, - _ => false, - }) + .all(|filter| ticket_evidence_matches(&detail.evidence, filter)) { return false; } - if !query - .attention - .iter() - .all(|attention| match attention.as_str() { - "done_not_closed" => summary.state == "done", - "implementation_report_not_closed" => { - detail.evidence.has_implementation_report && summary.state != "closed" - } - "report_after_rescope" => detail.evidence.implementation_report_after_rescope, - "unresolved_review" => detail.evidence.unresolved_request_changes, - "missing_commit" => !detail.evidence.has_commit, - "blocked" => !detail.relations.blockers.is_empty(), - "unblocked" => detail.relations.blockers.is_empty(), - "ready" => summary.state == "ready" && detail.relations.blockers.is_empty(), - "awaiting_review" => detail.evidence.review_status.as_deref() == Some("pending"), - "unresolved_changes" => detail.evidence.unresolved_request_changes, - "stale_after_rescope" => { - detail.evidence.has_implementation_report - && !detail.evidence.implementation_report_after_rescope - } - "missing_evidence" => !detail.evidence.complete_for_integration, - _ => false, - }) - { + if !query.attention.iter().all(|attention| { + ticket_attention_matches( + &summary.state, + &detail.evidence, + !detail.relations.blockers.is_empty(), + authoritative_events, + attention, + ) + }) { return false; } if query.linked_objective_id.as_ref().is_some_and(|id| { @@ -1812,31 +1944,63 @@ fn workspace_action_priority_name(priority: TicketWorkspaceActionPriority) -> &' #[cfg(test)] mod tests { + use std::collections::BTreeMap; use std::fs; use std::path::Path; use super::*; use crate::store::{ObjectiveRecord, ObjectiveTicketLinkRecord, WorkspaceRecord}; - #[test] - fn merge_request_summary_tracks_the_latest_requested_subject() { - let actor = merge_request::WorkerIdentity { + fn actor() -> merge_request::WorkerIdentity { + merge_request::WorkerIdentity { runtime_id: "runtime-1".to_string(), worker_id: "worker-1".to_string(), - }; - let at = Utc::now(); - let first_review = merge_request::ReviewEvent { - event_id: "review-1".to_string(), - sequence: 2, - request_event_id: "request-1".to_string(), - subject_ref: "commit-1".to_string(), - decision: ReviewDecision::Approve, - body: "approved".to_string(), - findings: Vec::new(), - reviewer: actor.clone(), - created_at: at, - }; - let mut request = MergeRequest { + } + } + + fn reviewed_merge_request(decision: ReviewDecision, revoked: bool) -> MergeRequest { + let requested_at = DateTime::parse_from_rfc3339("2026-01-01T00:03:00Z") + .unwrap() + .with_timezone(&Utc); + let reviewed_at = DateTime::parse_from_rfc3339("2026-01-01T00:04:00Z") + .unwrap() + .with_timezone(&Utc); + let reviewer = actor(); + let mut thread = vec![ + MergeRequestThreadEvent::ReviewRequested(merge_request::ReviewRequestedEvent { + event_id: "request-1".to_string(), + sequence: 1, + subject_ref: "commit-1".to_string(), + requested_by: reviewer.clone(), + reviewer: reviewer.clone(), + created_at: requested_at, + }), + MergeRequestThreadEvent::Review(merge_request::ReviewEvent { + event_id: "review-1".to_string(), + sequence: 2, + request_event_id: "request-1".to_string(), + subject_ref: "commit-1".to_string(), + decision, + body: "review body".to_string(), + findings: Vec::new(), + reviewer: reviewer.clone(), + created_at: reviewed_at, + }), + ]; + if revoked { + thread.push(MergeRequestThreadEvent::ReviewRevoked( + merge_request::ReviewRevokedEvent { + event_id: "revoke-1".to_string(), + sequence: 3, + review_event_id: "review-1".to_string(), + subject_ref: "commit-1".to_string(), + reason: "stale".to_string(), + revoked_by: reviewer, + created_at: reviewed_at + chrono::Duration::minutes(1), + }, + )); + } + MergeRequest { workspace_id: "workspace-1".to_string(), merge_request_id: "mr-1".to_string(), repository_id: "main".to_string(), @@ -1844,59 +2008,21 @@ mod tests { selector_from: Some("work/ticket-1".to_string()), selector_to: "orchestration".to_string(), ticket_ids: vec!["ticket-1".to_string()], - thread: vec![ - MergeRequestThreadEvent::ReviewRequested(merge_request::ReviewRequestedEvent { - event_id: "request-1".to_string(), - sequence: 1, - subject_ref: "commit-1".to_string(), - requested_by: actor.clone(), - reviewer: actor.clone(), - created_at: at, - }), - MergeRequestThreadEvent::Review(first_review), - MergeRequestThreadEvent::ReviewRequested(merge_request::ReviewRequestedEvent { - event_id: "request-2".to_string(), - sequence: 3, - subject_ref: "commit-2".to_string(), - requested_by: actor.clone(), - reviewer: actor.clone(), - created_at: at, - }), - ], - created_at: at, - updated_at: at, - }; - - let pending = merge_request_summary(request.clone()); - assert_eq!(pending.review_status, "pending"); - assert_eq!(pending.review_subject_ref.as_deref(), Some("commit-2")); - assert_eq!(pending.review_submitted_at, None); - - request.thread.push(MergeRequestThreadEvent::Review( - merge_request::ReviewEvent { - event_id: "review-2".to_string(), - sequence: 4, - request_event_id: "request-2".to_string(), - subject_ref: "commit-2".to_string(), - decision: ReviewDecision::Approve, - body: "current approval".to_string(), - findings: Vec::new(), - reviewer: actor, - created_at: at, - }, - )); - let approved = merge_request_summary(request); - assert_eq!(approved.review_status, "approved"); - assert_eq!(approved.review_subject_ref.as_deref(), Some("commit-2")); - assert_eq!(approved.review_excerpt.as_deref(), Some("current approval")); + thread, + created_at: requested_at, + updated_at: reviewed_at, + } } - #[test] - fn ticket_evidence_summary_requires_report_after_latest_rescope_and_approved_revision() { - let event = |kind: &str| TicketEvent { + fn ticket_event(kind: &str, at: &str, changes: Option<&str>) -> TicketEvent { + let mut attributes = BTreeMap::new(); + if let Some(changes) = changes { + attributes.insert("changes".to_string(), changes.to_string()); + } + TicketEvent { kind: ticket::TicketEventKind::Other(kind.to_string()), author: Some("coder".to_string()), - at: Some("2026-01-01T00:00:00Z".to_string()), + at: Some(at.to_string()), status: None, from: None, to: None, @@ -1904,43 +2030,186 @@ mod tests { state_field: None, heading: None, body: ticket::MarkdownText::new(kind), - attributes: Default::default(), + attributes, references: Vec::new(), - }; - let request = TicketMergeRequestSummary { - merge_request_id: "mr-1".to_string(), - state: "open".to_string(), - review_status: "approved".to_string(), - selector_from: Some("work/ticket".to_string()), - selector_to: "orchestration".to_string(), - updated_at: "2026-01-01T00:00:00Z".to_string(), - review_subject_ref: Some("head".to_string()), - review_submitted_at: Some("2026-01-01T00:00:00Z".to_string()), - review_excerpt: Some("approved".to_string()), - }; - let stale = ticket_evidence_summary( - &[event("implementation_report"), event("item_edit")], - Some(&request), + } + } + + #[test] + fn merge_request_summary_uses_the_provider_resolved_current_subject() { + let approved = merge_request_summary( + reviewed_merge_request(ReviewDecision::Approve, false), + Some("commit-1".to_string()), ); - assert!(!stale.implementation_report_after_rescope); - assert!(!stale.complete_for_integration); - assert!( - stale - .missing - .contains(&"implementation_report_after_rescope".to_string()) + assert_eq!(approved.review_status, "approved"); + assert_eq!(approved.current_subject_ref.as_deref(), Some("commit-1")); + assert_eq!( + approved.review_requested_at.as_deref(), + Some("2026-01-01T00:03:00+00:00") ); - let current = ticket_evidence_summary( + + let moved = merge_request_summary( + reviewed_merge_request(ReviewDecision::Approve, false), + Some("commit-2".to_string()), + ); + assert_eq!(moved.review_status, "pending"); + assert_eq!(moved.current_subject_ref.as_deref(), Some("commit-2")); + assert_eq!(moved.review_subject_ref.as_deref(), Some("commit-1")); + assert_eq!(moved.review_submitted_at, None); + } + + #[test] + fn ticket_readiness_requires_current_unrevoked_approval_without_a_report() { + let approved = merge_request_summary( + reviewed_merge_request(ReviewDecision::Approve, false), + Some("commit-1".to_string()), + ); + let evidence = ticket_evidence_summary(Some("main"), &[], Some(&approved)); + assert!(evidence.complete_for_integration); + assert!(evidence.approved_current_subject); + assert!(evidence.review_after_rescope); + assert!(evidence.has_commit); + + let with_audit_events = ticket_evidence_summary( + Some("main"), &[ - event("implementation_report"), - event("item_edit"), - event("implementation_report"), + ticket_event("comment", "2026-01-01T00:05:00Z", None), + ticket_event("implementation_report", "2026-01-01T00:06:00Z", None), ], - Some(&request), + Some(&approved), ); - assert!(current.implementation_report_after_rescope); - assert!(current.has_commit); - assert!(current.approved); - assert!(current.complete_for_integration); + assert!(with_audit_events.complete_for_integration); + + let revoked = merge_request_summary( + reviewed_merge_request(ReviewDecision::Approve, true), + Some("commit-1".to_string()), + ); + let evidence = ticket_evidence_summary(Some("main"), &[], Some(&revoked)); + assert!(!evidence.approved_current_subject); + assert!(!evidence.complete_for_integration); + + let changes = merge_request_summary( + reviewed_merge_request(ReviewDecision::RequestChanges, false), + Some("commit-1".to_string()), + ); + let evidence = ticket_evidence_summary(Some("main"), &[], Some(&changes)); + assert!(evidence.unresolved_request_changes); + assert!(!evidence.complete_for_integration); + } + + #[test] + fn ticket_readiness_fails_closed_for_missing_or_closed_current_merge_request() { + let unresolved = + merge_request_summary(reviewed_merge_request(ReviewDecision::Approve, false), None); + let evidence = ticket_evidence_summary(Some("main"), &[], Some(&unresolved)); + assert!(!evidence.has_current_subject_ref); + assert!(!evidence.has_commit); + assert!(!evidence.complete_for_integration); + + let mut closed_request = reviewed_merge_request(ReviewDecision::Approve, false); + closed_request.state = MergeRequestState::Closed; + let closed = merge_request_summary(closed_request, Some("commit-1".to_string())); + let evidence = ticket_evidence_summary(Some("main"), &[], Some(&closed)); + assert!(!evidence.has_merge_request); + assert!(!evidence.complete_for_integration); + assert!(evidence.missing.contains(&"open_merge_request".to_string())); + } + + #[test] + fn ticket_readiness_requires_request_and_approval_after_substantive_rescope() { + let approved = merge_request_summary( + reviewed_merge_request(ReviewDecision::Approve, false), + Some("commit-1".to_string()), + ); + let fresh = ticket_evidence_summary( + Some("main"), + &[ticket_event( + "item_edit", + "2026-01-01T00:02:00Z", + Some("body"), + )], + Some(&approved), + ); + assert!(fresh.review_after_rescope); + assert!(fresh.complete_for_integration); + + let stale = ticket_evidence_summary( + Some("main"), + &[ticket_event( + "item_edit", + "2026-01-01T00:05:00Z", + Some("title"), + )], + Some(&approved), + ); + assert!(!stale.review_after_rescope); + assert!(!stale.complete_for_integration); + assert!(stale.missing.contains(&"review_after_rescope".to_string())); + + let metadata_only = ticket_evidence_summary( + Some("main"), + &[ticket_event( + "item_edit", + "2026-01-01T00:05:00Z", + Some("formatter"), + )], + Some(&approved), + ); + assert!(metadata_only.complete_for_integration); + } + + #[test] + fn ticket_query_filters_map_to_current_merge_request_evidence() { + let approved_summary = merge_request_summary( + reviewed_merge_request(ReviewDecision::Approve, false), + Some("commit-1".to_string()), + ); + let approved = ticket_evidence_summary(Some("main"), &[], Some(&approved_summary)); + assert!(ticket_evidence_matches(&approved, "merge_request")); + assert!(ticket_evidence_matches(&approved, "commit")); + assert!(ticket_evidence_matches(&approved, "approved_review")); + assert!(!ticket_attention_matches( + "inprogress", + &approved, + false, + &[], + "missing_evidence", + )); + + let pending_summary = merge_request_summary( + reviewed_merge_request(ReviewDecision::Approve, false), + Some("commit-2".to_string()), + ); + let pending = ticket_evidence_summary(Some("main"), &[], Some(&pending_summary)); + assert!(ticket_attention_matches( + "inprogress", + &pending, + false, + &[], + "awaiting_review", + )); + assert!(!ticket_evidence_matches(&pending, "approved_review")); + + let stale_events = vec![ticket_event( + "item_edit", + "2026-01-01T00:05:00Z", + Some("target"), + )]; + let stale = ticket_evidence_summary(Some("main"), &stale_events, Some(&approved_summary)); + assert!(ticket_attention_matches( + "inprogress", + &stale, + false, + &stale_events, + "stale_after_rescope", + )); + assert!(ticket_attention_matches( + "inprogress", + &stale, + false, + &stale_events, + "missing_evidence", + )); } #[tokio::test] diff --git a/crates/workspace-server/src/lib.rs b/crates/workspace-server/src/lib.rs index 4724a852..d504bb11 100644 --- a/crates/workspace-server/src/lib.rs +++ b/crates/workspace-server/src/lib.rs @@ -31,7 +31,8 @@ mod workspace_subscription; pub use authority::{ MemoryAuthority, MemoryDocument, MemoryStagingEntry, MemoryStagingResolution, - ObjectiveAuthority, SqliteWorkspaceAuthority, TicketAuthority, WorkspaceAuthority, + ObjectiveAuthority, SqliteWorkspaceAuthority, TicketAuthority, TicketMergeRevisionSource, + WorkspaceAuthority, }; pub use config::{ BackendRuntimesConfigFile, ConfigDiff, ResolvedWorkspaceBackendConfig, diff --git a/crates/workspace-server/src/records.rs b/crates/workspace-server/src/records.rs index 35348f60..739b5f6f 100644 --- a/crates/workspace-server/src/records.rs +++ b/crates/workspace-server/src/records.rs @@ -239,12 +239,15 @@ pub struct TicketAssignmentSummary { #[cfg_attr(feature = "typescript", derive(ts_rs::TS))] pub struct TicketMergeRequestSummary { pub merge_request_id: String, + pub repository_id: String, pub state: String, pub review_status: String, pub selector_from: Option, pub selector_to: String, pub updated_at: String, + pub current_subject_ref: Option, pub review_subject_ref: Option, + pub review_requested_at: Option, pub review_submitted_at: Option, pub review_excerpt: Option, } @@ -252,12 +255,13 @@ pub struct TicketMergeRequestSummary { #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Default)] #[cfg_attr(feature = "typescript", derive(ts_rs::TS))] pub struct TicketEvidenceSummary { - pub has_implementation_report: bool, - pub implementation_report_after_rescope: bool, pub has_merge_request: bool, + pub has_current_subject_ref: bool, + pub has_review_request: bool, pub has_commit: bool, pub review_status: Option, - pub approved: bool, + pub approved_current_subject: bool, + pub review_after_rescope: bool, pub unresolved_request_changes: bool, pub complete_for_integration: bool, pub missing: Vec, diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index c2d7654a..d3d793a0 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -62,7 +62,7 @@ use crate::auth::{ }; use crate::authority::{ MemoryAuthority, ObjectiveAuthority, ObjectiveCreateInput, ObjectiveEditInput, - SqliteWorkspaceAuthority, TicketAuthority, + SqliteWorkspaceAuthority, TicketAuthority, TicketMergeRevisionSource, }; use crate::companion::{ CompanionCancelRequest, CompanionConsole, CompanionMessageRequest, CompanionMessageResponse, @@ -809,7 +809,11 @@ impl WorkspaceApi { authority: SqliteWorkspaceAuthority::new( config.database_path.clone(), config.workspace_id.clone(), - )?, + )? + .with_merge_revision_source(Arc::new(MergeRequestRepositorySource { + workspace_id: config.workspace_id.clone(), + reader: RepositoryRegistryReader::new(config.repositories.clone()), + })), config, store, runtime, @@ -3721,6 +3725,15 @@ struct MergeRequestRepositorySource { reader: RepositoryRegistryReader, } +impl TicketMergeRevisionSource for MergeRequestRepositorySource { + fn resolve_subject_ref(&self, repository_id: &str, selector: &str) -> Option { + self.reader + .observe_merge_target(repository_id, Some(selector)) + .ok() + .map(|target| target.commit) + } +} + impl merge_request::RepositorySource for MergeRequestRepositorySource { fn repository_belongs_to_workspace( &self, @@ -12291,7 +12304,7 @@ mod tests { assert_eq!(builtin.definition.name, "coder-review"); assert_eq!(builtin.selector.to_string(), "builtin:coder-review"); assert_eq!(builtin.flow_id, "builtin:coder-review"); - assert_eq!(builtin.revision, 2); + assert_eq!(builtin.revision, 3); assert_eq!( api.store .list_flow_sources(&api.config.workspace_id) diff --git a/resources/flows/coder-review.dcdl b/resources/flows/coder-review.dcdl index c191cd1b..abaa6491 100644 --- a/resources/flows/coder-review.dcdl +++ b/resources/flows/coder-review.dcdl @@ -39,11 +39,11 @@ }; complete = { - instructions = "Call MergeRequestComplete with a fresh operation_id and the approved current revision. The Server must revalidate current assignment, immutable revision, registered Reviewer attempt, and Ticket inprogress CAS. Only after the authoritative operation returns Ticket state done, request a Flow transition."; + instructions = "The exact current Merge Request subject has authoritative approval. Leave a concise human-facing summary only when useful, then hand off to the Orchestrator. Do not call MergeRequestComplete; approval and the current MR revision are the durable completion evidence. Request a Flow transition only after the Orchestrator's authoritative completion changes the Ticket to done."; transitions = { completed = { target = "done"; - condition = "MergeRequestComplete durably returned done for this exact operation_id and current approved revision. A Flow state or prose report alone is never sufficient."; + condition = "The Orchestrator completed the current approved Merge Request and the authoritative Ticket state is done. A Flow state or prose report alone is never sufficient."; }; }; }; diff --git a/resources/prompts/common/tickets.md b/resources/prompts/common/tickets.md index c9e5e1e1..5a927d30 100644 --- a/resources/prompts/common/tickets.md +++ b/resources/prompts/common/tickets.md @@ -1,6 +1,6 @@ ## Ticket workflow -Use the available typed Ticket tools as the authority for Ticket reads and mutations. Use `QueryTicket` for bounded discovery and filtering, then `ShowTicket` for the authoritative item revision, thread/evidence, relations, linked Objectives, and current Merge Request context before routing, review, or closure decisions. Do not invoke a Ticket CLI or edit backend storage directly as an alternative implementation of those tools. +Use the available typed Ticket tools as the authority for Ticket reads and mutations. Use `QueryTicket` for bounded discovery and filtering, then `ShowTicket` for the authoritative item revision, thread/evidence, relations, linked Objectives, and current Merge Request context before routing, review, or closure decisions. Current linked-MR evidence is completion authority; `implementation_report` entries are optional historical/audit context and must not be required for integration readiness. Do not invoke a Ticket CLI or edit backend storage directly as an alternative implementation of those tools. Read the relevant Ticket before making implementation, routing, review, state, or closure decisions. Do not infer the current contract from an id, title, notification, or remembered summary alone. Check related or potentially duplicate Tickets when creating or materially rescoping work. diff --git a/resources/prompts/role/coder.md b/resources/prompts/role/coder.md index adfa0e54..60e887e1 100644 --- a/resources/prompts/role/coder.md +++ b/resources/prompts/role/coder.md @@ -6,4 +6,4 @@ Treat the first committed user message as the bounded Ticket/action context and Before review, open a Merge Request with immutable `selector_from` / `selector_to`. Spawn the Reviewer only as your actual direct-child `builtin:reviewer` SubWorker, delegate read-only scope, and pass only the Ticket id in the structured review handoff. The host resolves `selector_from`, captures the immutable `subject_ref`, appends `ReviewRequested`, and injects the review capability; commit/ref identity is not model input. Reviewer prose is not approval: the child must commit `MergeRequestReviewSubmit` through its injected capability authority. -A request-changes result requires a fresh Reviewer child request. Flow terminal state is not Ticket completion authority. Complete only through `MergeRequestComplete` with a unique operation id, the approved `Review` event id, and final target-ref evidence; the Server re-resolves selectors, revalidates assignment, and fences Ticket state side effects. +A request-changes result requires a fresh Reviewer child request. Flow terminal state is not Ticket completion authority. After the exact current Merge Request subject has an authoritative approval, leave a concise human-facing summary when useful and hand off to the Orchestrator. Do not call `MergeRequestComplete`; approval and the current MR revision are the durable completion evidence. diff --git a/resources/prompts/role/orchestrator.md b/resources/prompts/role/orchestrator.md index 32092653..41b3ee5f 100644 --- a/resources/prompts/role/orchestrator.md +++ b/resources/prompts/role/orchestrator.md @@ -6,6 +6,8 @@ Keep durable orchestration behavior here and treat the first committed user mess The assigned Coder owns its review/fix loop and launches Reviewer SubWorkers itself. Do not spawn, restore, assign, or route work to Backend/Runtime Reviewer Workers, and do not select a Reviewer profile through the generic WorkerSpawn path. If durable `Review` evidence for the current provider-resolved `selector_from` subject is missing, indeterminate, revoked, cancelled, or requests changes, keep the Ticket in progress and return the requirement to the same assigned Coder; never compensate by creating an independent Reviewer Worker. +Treat the current linked Merge Request as implementation-completion authority. A current provider-resolved source ref, commit/repository evidence, an effective approval for that exact subject, review freshness after the latest substantive Ticket item edit, and no unresolved request-changes are sufficient; do not require an `implementation_report`. Human summaries remain optional audit context. Recheck `ShowTicket` and `MergeRequestReadinessCheck` immediately before guarded completion, and only the Orchestrator may call `MergeRequestComplete`. + Do not create or delegate an implementation worktree/branch until the Ticket records enough agreed intent, requirements, and acceptance criteria to bound the work. Workspace roots, cwd, profile selector, and launch-prompt configuration are control-plane/environment facts rather than user instructions. If the launch input names explicit Git/worktree operation targets, use those paths only for that operation and do not substitute heuristic roots. diff --git a/web/workspace/src/lib/generated/ticket-api.ts b/web/workspace/src/lib/generated/ticket-api.ts index a24976be..f3e14081 100644 --- a/web/workspace/src/lib/generated/ticket-api.ts +++ b/web/workspace/src/lib/generated/ticket-api.ts @@ -69,23 +69,27 @@ export type TicketAssignmentSummary = { export type TicketMergeRequestSummary = { merge_request_id: string; + repository_id: string; state: string; review_status: string; selector_from: string | null; selector_to: string; updated_at: string; + current_subject_ref: string | null; review_subject_ref: string | null; + review_requested_at: string | null; review_submitted_at: string | null; review_excerpt: string | null; }; export type TicketEvidenceSummary = { - has_implementation_report: boolean; - implementation_report_after_rescope: boolean; has_merge_request: boolean; + has_current_subject_ref: boolean; + has_review_request: boolean; has_commit: boolean; review_status: string | null; - approved: boolean; + approved_current_subject: boolean; + review_after_rescope: boolean; unresolved_request_changes: boolean; complete_for_integration: boolean; missing: Array;