Hare/yoi
1
1
Fork 0
Code Actions Packages Releases Activity

feat: align merge request review workflow with selector refs

Browse Source
This commit is contained in:
Hare
2026-08-26 15:03:26 +09:00
parent ccf7de1a55
commit 1d140be715
17 changed files with 376 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/flow/src/builtin.rs
+26 -2
View File
@@ -24,7 +24,7 @@ pub fn builtin_flow_source(slug: &str) -> Option<BuiltinFlowSource> {
match slug {
CODER_REVIEW_FLOW_SLUG => Some(BuiltinFlowSource {
slug: CODER_REVIEW_FLOW_SLUG,
revision: 3,
revision: 4,
path: "builtin/flows/coder-review.dcdl",
content: CODER_REVIEW_FLOW_SOURCE,
}),
@@ -35,7 +35,7 @@ pub fn builtin_flow_source(slug: &str) -> Option<BuiltinFlowSource> {
pub fn builtin_flow_sources() -> &'static [BuiltinFlowSource] {
const SOURCES: &[BuiltinFlowSource] = &[BuiltinFlowSource {
slug: CODER_REVIEW_FLOW_SLUG,
revision: 3,
revision: 4,
path: "builtin/flows/coder-review.dcdl",
content: CODER_REVIEW_FLOW_SOURCE,
}];
@@ -46,6 +46,30 @@ pub fn builtin_flow_sources() -> &'static [BuiltinFlowSource] {
mod tests {
use super::*;
#[test]
fn coder_review_flow_uses_current_selector_ref_review_contract() {
let source = builtin_flow_source(CODER_REVIEW_FLOW_SLUG).expect("coder review Flow");
for required in [
"OpenMergeRequest",
"ShowMergeRequest",
"ReviewMergeRequest",
"CompleteMergeRequest",
"existing Merge Request `selector_from`",
"Target-only movement does not invalidate",
] {
assert!(source.content.contains(required), "missing {required}");
}
for stale in [
"MergeRequestOpen",
"MergeRequestShow",
"MergeRequestReview",
"MergeRequestComplete",
"new immutable revision",
] {
assert!(!source.content.contains(stale), "stale contract {stale}");
}
}
#[test]
fn every_builtin_flow_compiles_and_matches_catalog_identity() {
assert!(!builtin_flow_sources().is_empty());
crates/merge-request/src/lib.rs
+27 -5
View File
@@ -416,7 +416,7 @@ impl MergeRequestStore {
let conflict:bool=t.query_row("SELECT EXISTS(SELECT 1 FROM merge_request_ticket_relations rel JOIN merge_requests mr ON mr.workspace_id=rel.workspace_id AND mr.merge_request_id=rel.merge_request_id WHERE rel.workspace_id=?1 AND rel.ticket_id=?2 AND mr.state='open')",params![i.auth.workspace_id,i.ticket_id],|r|r.get(0))?;
if conflict {
return Err(MergeRequestError::Conflict(
"Ticket already has an open Merge Request".into(),
"Ticket already has an open Merge Request; use ShowMergeRequest and advance the existing selector_from with a normal non-force push instead of opening a replacement Merge Request or adding a revision".into(),
));
}
let now = i.now.to_rfc3339();
@@ -575,12 +575,16 @@ impl MergeRequestStore {
));
};
if subject != i.current_subject_ref {
let reason = format!(
"selector_from moved from requested subject {subject} to current subject {}; fresh review of the exact current source ref is required",
i.current_subject_ref
);
let e = ReviewCancelledEvent {
event_id: Uuid::now_v7().to_string(),
sequence: next_seq(&t, &ws, &mr)?,
request_event_id: req,
subject_ref: subject,
reason: "selector_from moved before submission".into(),
reason,
created_at: i.now,
};
insert_event(&t, &ws, &mr, "review_cancelled", &e, i.now, None)?;
@@ -667,9 +671,27 @@ impl MergeRequestStore {
}
match (&i.current_subject_ref, &review) {
(None, _) => b.push("selector_from could not be resolved".into()),
(Some(_), None) => b.push("current source ref has no valid review".into()),
(_, Some(r)) if r.decision == ReviewDecision::RequestChanges => {
b.push("current source ref requests changes".into())
(Some(subject_ref), None) => {
let previous_review_subject = mr.thread.iter().rev().find_map(|event| match event {
MergeRequestThreadEvent::ReviewRequested(value) => {
Some(value.subject_ref.as_str())
}
MergeRequestThreadEvent::Review(value) => Some(value.subject_ref.as_str()),
_ => None,
});
match previous_review_subject.filter(|previous| *previous != subject_ref) {
Some(previous) => b.push(format!(
"selector_from moved from reviewed/requested subject {previous} to current subject {subject_ref}; request a fresh review for this exact source ref (selector_to movement alone does not invalidate source approval)"
)),
None => b.push(format!(
"current source ref {subject_ref} has no valid review; request a fresh review for this exact source ref"
)),
}
}
(Some(subject_ref), Some(r)) if r.decision == ReviewDecision::RequestChanges => {
b.push(format!(
"current source ref {subject_ref} requests changes; advance the existing selector_from with a normal non-force push, then request a fresh review for the exact new source ref"
))
}
_ => {}
}
crates/merge-request/tests/store.rs
+72 -3
View File
@@ -181,16 +181,85 @@ fn source_move_cancels_submission_and_old_approval_is_reusable_when_source_retur
.is_err()
);
let mr = s.get("W", "T").unwrap();
let cancellation = mr.thread.iter().find_map(|event| match event {
MergeRequestThreadEvent::ReviewCancelled(value) => Some(value),
_ => None,
});
assert!(
mr.thread
.iter()
.any(|e| matches!(e, MergeRequestThreadEvent::ReviewCancelled(_)))
cancellation
.as_ref()
.is_some_and(|value| value.reason.contains("selector_from moved")
&& value.reason.contains("fresh review"))
);
assert_eq!(
mr.effective_review("source-a").map(|r| &r.event_id),
Some(&approved.event_id)
);
}
#[test]
fn same_selector_source_advancement_requires_fresh_review_and_preserves_target_only_approval() {
let (_d, s) = fixture();
open(&s);
let first = approve(&s, "source-1", "one");
let stale = s
.readiness(ReadinessCheck {
ticket_id: "T".into(),
current_subject_ref: Some("source-2".into()),
auth: auth(),
})
.unwrap();
assert!(!stale.ready);
assert!(stale.review.is_none());
assert!(stale.blockers.iter().any(|blocker| {
blocker.contains("selector_from moved from reviewed/requested subject source-1")
&& blocker.contains("current subject source-2")
&& blocker.contains("fresh review")
}));
assert_eq!(
s.get("W", "T")
.unwrap()
.effective_review("source-1")
.map(|review| &review.event_id),
Some(&first.event_id)
);
let second = approve(&s, "source-2", "two");
let ready = s
.readiness(ReadinessCheck {
ticket_id: "T".into(),
current_subject_ref: Some("source-2".into()),
auth: auth(),
})
.unwrap();
assert!(ready.ready);
assert_eq!(
ready.review.as_ref().map(|review| &review.event_id),
Some(&second.event_id)
);
// The target can move from target-1 to target-2 without changing selector_from
// or invalidating the exact-source approval. Completion consumes refreshed
// integration evidence for the current target pair.
let merged = s
.complete(CompleteMergeRequest {
operation_id: "target-moved".into(),
ticket_id: "T".into(),
current_subject_ref: "source-2".into(),
target_ref_before: "target-2".into(),
target_ref_after: "integrated-target-2".into(),
approval_event_id: second.event_id,
strategy: MergeStrategy::FastForward,
resolution: ConflictResolution::None,
auth: auth(),
now: at(5),
})
.unwrap();
assert_eq!(merged.approved_source_ref, "source-2");
assert_eq!(merged.target_ref_before, "target-2");
assert_eq!(merged.target_ref_after, "integrated-target-2");
}
#[test]
fn review_revocation_invalidates_readiness() {
let (_d, s) = fixture();
crates/worker/src/feature/builtin/merge_request.rs
+66 -39
View File
@@ -50,11 +50,11 @@ struct MergeRequestTool {
kind: Kind,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct ShowInput {
struct TicketInput {
ticket: String,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct OpenInput {
struct OpenMergeRequestInput {
ticket: String,
repository_id: String,
selector_from: String,
@@ -63,7 +63,7 @@ struct OpenInput {
summary: String,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct CompleteInput {
struct CompleteMergeRequestInput {
ticket: String,
operation_id: String,
approval_event_id: String,
@@ -86,7 +86,7 @@ enum MergeResolutionInput {
ConflictsResolved,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct ReviewInput {
struct ReviewMergeRequestInput {
decision: ReviewDecisionInput,
#[serde(default)]
body: String,
@@ -133,19 +133,19 @@ impl Kind {
fn name(self) -> &'static str {
match self {
Self::Show => "MergeRequestShow",
Self::Readiness => "MergeRequestReadinessCheck",
Self::Open => "MergeRequestOpen",
Self::Complete => "MergeRequestComplete",
Self::Review => "MergeRequestReview",
Self::Show => "ShowMergeRequest",
Self::Readiness => "CheckMergeRequestReadiness",
Self::Open => "OpenMergeRequest",
Self::Complete => "CompleteMergeRequest",
Self::Review => "ReviewMergeRequest",
}
}
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::Complete => json!(schemars::schema_for!(CompleteInput)),
Self::Review => json!(schemars::schema_for!(ReviewInput)),
Self::Show | Self::Readiness => json!(schemars::schema_for!(TicketInput)),
Self::Open => json!(schemars::schema_for!(OpenMergeRequestInput)),
Self::Complete => json!(schemars::schema_for!(CompleteMergeRequestInput)),
Self::Review => json!(schemars::schema_for!(ReviewMergeRequestInput)),
}
}
}
@@ -156,13 +156,13 @@ impl Tool for MergeRequestTool {
ToolError::ExecutionFailed("Merge Request tools require Workspace identity".into())
})?;
if matches!(self.kind, Kind::Show) {
let value: ShowInput = parse(input)?;
let value: TicketInput = parse(input)?;
nonempty(&value.ticket)?;
return self.show_current_merge_request(ws, &value.ticket);
}
let (method, path, body) = match self.kind {
Kind::Readiness => {
let v: ShowInput = parse(input)?;
let v: TicketInput = parse(input)?;
nonempty(&v.ticket)?;
(
WorkspaceRequestMethod::Get,
@@ -170,9 +170,9 @@ impl Tool for MergeRequestTool {
None,
)
}
Kind::Show => unreachable!("MergeRequestShow is handled above"),
Kind::Show => unreachable!("ShowMergeRequest is handled above"),
Kind::Open => {
let v: OpenInput = parse(input)?;
let v: OpenMergeRequestInput = parse(input)?;
nonempty(&v.ticket)?;
(
WorkspaceRequestMethod::Post,
@@ -183,7 +183,7 @@ impl Tool for MergeRequestTool {
)
}
Kind::Complete => {
let v: CompleteInput = parse(input)?;
let v: CompleteMergeRequestInput = parse(input)?;
nonempty(&v.ticket)?;
(
WorkspaceRequestMethod::Post,
@@ -194,7 +194,7 @@ impl Tool for MergeRequestTool {
)
}
Kind::Review => {
let v: ReviewInput = parse(input)?;
let v: ReviewMergeRequestInput = parse(input)?;
let ctx = self.client.reviewer_context().ok_or_else(|| {
ToolError::ExecutionFailed(
"Review submit requires injected Reviewer capability".into(),
@@ -397,19 +397,21 @@ impl FeatureModule for MergeRequestFeature {
pub fn description(n: &str) -> Option<&'static str> {
match n {
"MergeRequestShow" => Some("Read the selector-based Merge Request and append-only thread."),
"MergeRequestReadinessCheck" => {
Some("Resolve current provider refs and derive readiness from valid review events.")
}
"MergeRequestOpen" => {
Some("Open a Merge Request with immutable source and target selectors.")
}
"MergeRequestComplete" => {
Some("Complete using an approved review event and final target-ref evidence.")
}
"MergeRequestReview" => {
Some("Submit the injected Reviewer capability result for its captured subject ref.")
}
"ShowMergeRequest" => Some(
"Read the selector-based Merge Request, append-only thread, source-review freshness, and target-integration evidence before review, fix, or handoff decisions.",
),
"CheckMergeRequestReadiness" => Some(
"Resolve current provider refs and derive readiness from exact-source review evidence; source movement requires fresh review while target-only movement preserves unchanged-source approval.",
),
"OpenMergeRequest" => Some(
"Open the Ticket's one Merge Request with immutable source and target selectors; reuse it and advance only selector_from with a normal non-force push for later fixes.",
),
"CompleteMergeRequest" => Some(
"Record Orchestrator-owned integration using unchanged-source approval and refreshed final target-ref evidence.",
),
"ReviewMergeRequest" => Some(
"Submit the injected Reviewer capability result for its captured exact source ref; source movement cancels it, while target-only movement does not.",
),
_ => None,
}
}
@@ -469,6 +471,31 @@ mod tests {
}
}
#[test]
fn model_facing_operations_use_only_verb_first_names() {
for name in [
"ShowMergeRequest",
"CheckMergeRequestReadiness",
"OpenMergeRequest",
"CompleteMergeRequest",
"ReviewMergeRequest",
] {
assert!(description(name).is_some(), "missing operation {name}");
}
for legacy in [
"MergeRequestShow",
"MergeRequestReadinessCheck",
"MergeRequestOpen",
"MergeRequestComplete",
"MergeRequestReview",
] {
assert!(
description(legacy).is_none(),
"legacy alias {legacy} must not remain registered"
);
}
}
#[tokio::test]
async fn show_resolves_ticket_projection_then_reads_canonical_resource() {
let client = Arc::new(RecordingWorkspaceClient::new(vec![
@@ -546,7 +573,7 @@ mod tests {
open: true,
..Default::default()
};
assert_eq!(tool_names(coder), ["MergeRequestShow", "MergeRequestOpen"]);
assert_eq!(tool_names(coder), ["ShowMergeRequest", "OpenMergeRequest"]);
let reviewer = MergeRequestFeatureConfig {
show: true,
@@ -555,7 +582,7 @@ mod tests {
};
assert_eq!(
tool_names(reviewer),
["MergeRequestShow", "MergeRequestReview"]
["ShowMergeRequest", "ReviewMergeRequest"]
);
let orchestrator = MergeRequestFeatureConfig {
@@ -567,9 +594,9 @@ mod tests {
assert_eq!(
tool_names(orchestrator),
[
"MergeRequestShow",
"MergeRequestReadinessCheck",
"MergeRequestComplete"
"ShowMergeRequest",
"CheckMergeRequestReadiness",
"CompleteMergeRequest"
]
);
assert_eq!(install(coder).1, [FEATURE_PROMPT_REF]);
@@ -581,8 +608,8 @@ mod tests {
#[test]
fn schemas_hide_revision_and_commit_authority() {
let schemas = [
schemars::schema_for!(OpenInput),
schemars::schema_for!(CompleteInput),
schemars::schema_for!(OpenMergeRequestInput),
schemars::schema_for!(CompleteMergeRequestInput),
];
for s in schemas {
let j = serde_json::to_string(&s).unwrap();
crates/worker/src/prompt/catalog.rs
+41
View File
@@ -609,6 +609,47 @@ mod tests {
assert!(!prompt.contains("use the Ticket repository `origin` transport"));
}
#[test]
fn merge_request_prompts_use_selector_refs_and_verb_first_operations() {
let catalog = PromptCatalog::builtins_only().unwrap();
let templates = &catalog.projection.templates;
let common = &templates["common.merge_request"];
let coder = &templates["role.coder"];
let orchestrator = &templates["role.orchestrator"];
let reviewer = &templates["role.reviewer"];
let combined = format!("{common}\n{coder}\n{orchestrator}\n{reviewer}");
for operation in [
"OpenMergeRequest",
"ShowMergeRequest",
"ReviewMergeRequest",
"CheckMergeRequestReadiness",
"CompleteMergeRequest",
] {
assert!(combined.contains(operation), "missing {operation}");
}
for stale_operation in [
"MergeRequestOpen",
"MergeRequestShow",
"MergeRequestReview",
"MergeRequestReadinessCheck",
"MergeRequestComplete",
"MergeRequestAddRevision",
] {
assert!(
!combined.contains(stale_operation),
"stale operation {stale_operation} remains in prompt authority"
);
}
assert!(common.contains("selector_from"));
assert!(common.contains("normal non-force push"));
assert!(common.contains("Moving only the target ref does not invalidate approval"));
assert!(common.contains("take precedence over stale Memory"));
assert!(coder.contains("Never invent an add-revision operation"));
assert!(orchestrator.contains("Target-only movement preserves source approval"));
assert!(reviewer.contains("target-only movement does not invalidate approval"));
}
#[test]
fn graph_rejects_dynamic_legacy_missing_and_cycles() {
let invalid = BTreeMap::from([
crates/workspace-server/src/server.rs
+7 -6
View File
@@ -4451,7 +4451,7 @@ async fn scoped_transition_ticket_state(
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(),
"done is guarded by CompleteMergeRequest with an approved exact source ref and operation_id".to_string(),
).into());
}
let current = api.authority.ticket(&path.id)?;
@@ -4573,7 +4573,7 @@ fn reject_unguarded_ticket_completion(operation: &TicketBackendOperation) -> Res
reject_unguarded_ticket_start(operation)?;
if generic_ticket_state_change(operation).is_some_and(|change| change.to == "done") {
return Err(Error::TicketAssignmentConflict(
"done is guarded by MergeRequestComplete with an approved immutable revision and operation_id".to_string(),
"done is guarded by CompleteMergeRequest with an approved exact source ref and operation_id".to_string(),
));
}
Ok(())
@@ -5229,7 +5229,7 @@ fn require_completed_target_observation(
}
if observed == target_ref_before {
return Err(Error::InvalidInput(
"target selector is still at target_ref_before; push the verified result from the Orchestrator Workdir before MergeRequestComplete".into(),
"target selector is still at target_ref_before; Orchestrator must integrate the approved source into the current target and verify the resulting target ref before CompleteMergeRequest (target-only movement does not invalidate approval for an unchanged source)".into(),
)
.into());
}
@@ -14696,7 +14696,7 @@ mod tests {
},
] {
let error = reject_unguarded_ticket_completion(&operation).unwrap_err();
assert!(error.to_string().contains("MergeRequestComplete"));
assert!(error.to_string().contains("CompleteMergeRequest"));
}
}
@@ -14901,7 +14901,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, 3);
assert_eq!(builtin.revision, 4);
assert_eq!(
api.store
.list_flow_sources(&api.config.workspace_id)
@@ -15077,7 +15077,8 @@ mod tests {
assert!(matches!(
not_pushed.error,
Error::InvalidInput(ref message)
if message.contains("push the verified result from the Orchestrator Workdir")
if message.contains("Orchestrator must integrate the approved source into the current target")
&& message.contains("target-only movement does not invalidate approval")
));
let moved = require_completed_target_observation("other", "before", "after").unwrap_err();
crates/yoi/src/ticket_cli.rs
+2 -2
View File
@@ -671,7 +671,7 @@ fn state(
StateTarget::InProgress => TicketWorkflowState::InProgress,
StateTarget::Done => {
return Err(TicketCliError::new(
"done is guarded by MergeRequestComplete with an approved immutable revision and operation_id",
"done is guarded by CompleteMergeRequest with an approved exact source ref and operation_id",
));
}
StateTarget::Closed => {
@@ -1366,7 +1366,7 @@ mod tests {
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"));
assert!(done_error.to_string().contains("CompleteMergeRequest"));
let closed = run(
&temp,