merge-request: select one final integration result

This commit is contained in:
2026-08-16 21:03:30 +09:00
parent 7256d80514
commit 1479148f84
6 changed files with 459 additions and 769 deletions
File diff suppressed because it is too large Load Diff
+130 -36
View File
@@ -240,6 +240,18 @@ fn merge_result_is_idempotent_target_fenced_and_non_ff_reviewed_independently()
Err(MergeRequestError::MergeResultOperationConflict)
));
// Multiple valid candidates for the same target are retained as history. The
// most recently recorded candidate is the one explicit final result.
record_result(
&store,
"V1",
"T1",
"h1",
"h1",
MergeStrategy::FastForward,
MergeResolution::None,
"op-same-target-old",
);
record_result(
&store,
"V1",
@@ -258,6 +270,42 @@ fn merge_result_is_idempotent_target_fenced_and_non_ff_reviewed_independently()
pending.merge_result_review_status,
Some(ReviewStatus::Pending)
);
let candidates = store
.show_for_ticket_with_target("T1", Some("T1"))
.unwrap()
.unwrap();
assert_eq!(candidates.merge_results.len(), 3);
assert_eq!(
candidates
.final_merge_result
.as_ref()
.map(|result| result.merge_result_id.as_str()),
Some("result-op-merge")
);
store
.register_reviewer_child_session(RegisterReviewerChildSession {
parent_runtime_id: "runtime-orchestrator".into(),
parent_worker_id: "workspace-orchestrator".into(),
child_session_id: "child-old-candidate".into(),
now: "2026-07-26T00:00:04Z".into(),
})
.unwrap();
let old_candidate_review = store.register_review_attempt(RegisterReviewAttempt {
attempt_id: "attempt-old-candidate".into(),
ticket_id: "T1".into(),
revision_id: "V1".into(),
merge_result_id: Some("result-op-same-target-old".into()),
parent_assignment_id: "A1".into(),
parent_runtime_id: "runtime-orchestrator".into(),
parent_worker_id: "workspace-orchestrator".into(),
child_session_id: "child-old-candidate".into(),
capability_token: "token-old-candidate".into(),
now: "2026-07-26T00:00:04Z".into(),
});
assert!(matches!(
old_candidate_review,
Err(MergeRequestError::MergeResultNotFinal)
));
store
.register_reviewer_child_session(RegisterReviewerChildSession {
@@ -307,7 +355,7 @@ fn merge_result_is_idempotent_target_fenced_and_non_ff_reviewed_independently()
.unwrap();
assert_eq!(
applied
.applied_merge_result
.final_merge_result
.as_ref()
.map(|result| result.target_status),
Some(MergeResultTargetStatus::Applied)
@@ -454,10 +502,52 @@ fn request_changes_new_revision_resets_and_exact_completion_replay_converges() {
assert!(review(&store, "V1", "tok1", ReviewDecision::Approve).is_err());
attempt(&store, "AT2", "V2", "tok2", "child2");
review(&store, "V2", "tok2", ReviewDecision::Approve).unwrap();
let missing_result = store.complete(CompleteMergeRequest {
operation_id: "OP-missing-result".into(),
ticket_id: "T1".into(),
expected_revision_id: "V2".into(),
expected_merge_result_id: "missing".into(),
observed_target_commit: "h2".into(),
implementation_assignment_id: "A1".into(),
completion_actor_runtime_id: "OR".into(),
completion_actor_worker_id: "OW".into(),
now: "tc".into(),
});
assert!(matches!(
missing_result,
Err(MergeRequestError::FinalMergeResultMissing)
));
record_result(
&store,
"V2",
"base",
"h2",
"h2",
MergeStrategy::FastForward,
MergeResolution::None,
"final-v2",
);
let not_applied = store.complete(CompleteMergeRequest {
operation_id: "OP-not-applied".into(),
ticket_id: "T1".into(),
expected_revision_id: "V2".into(),
expected_merge_result_id: "result-final-v2".into(),
observed_target_commit: "base".into(),
implementation_assignment_id: "A1".into(),
completion_actor_runtime_id: "OR".into(),
completion_actor_worker_id: "OW".into(),
now: "tc".into(),
});
assert!(matches!(
not_applied,
Err(MergeRequestError::FinalMergeResultNotApplied)
));
let input = CompleteMergeRequest {
operation_id: "OP1".into(),
ticket_id: "T1".into(),
expected_revision_id: "V2".into(),
expected_merge_result_id: "result-final-v2".into(),
observed_target_commit: "h2".into(),
implementation_assignment_id: "A1".into(),
completion_actor_runtime_id: "OR".into(),
completion_actor_worker_id: "OW".into(),
@@ -465,30 +555,22 @@ fn request_changes_new_revision_resets_and_exact_completion_replay_converges() {
};
let first = store.complete(input.clone()).unwrap();
assert!(!first.replayed);
assert_eq!(
store.show_for_ticket("T1").unwrap().unwrap().state,
MergeRequestState::Merged
);
assert_eq!(
store
.show_for_ticket("T1")
.unwrap()
.unwrap()
.merged_at
.as_deref(),
Some("tc")
);
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);
assert_eq!(replay.ticket_state, "done");
let conn = Connection::open(store.db_path()).unwrap();
assert_eq!(
conn.query_row(
@@ -570,7 +652,7 @@ fn spoof_self_approval_replay_and_cross_workspace_are_rejected() {
}
#[test]
fn reopen_resets_approval_and_merge_requires_authenticated_explicit_user() {
fn reopen_resets_approval() {
let (_dir, store) = setup();
open(&store);
attempt(&store, "AT", "V1", "token", "child");
@@ -578,18 +660,6 @@ fn reopen_resets_approval_and_merge_requires_authenticated_explicit_user() {
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]
@@ -598,10 +668,22 @@ fn concurrent_exact_completion_replays_commit_one_ticket_side_effect() {
open(&store);
attempt(&store, "AT", "V1", "token", "child");
review(&store, "V1", "token", ReviewDecision::Approve).unwrap();
record_result(
&store,
"V1",
"base",
"h1",
"h1",
MergeStrategy::FastForward,
MergeResolution::None,
"final-concurrent",
);
let input = CompleteMergeRequest {
operation_id: "OP-concurrent".into(),
ticket_id: "T1".into(),
expected_revision_id: "V1".into(),
expected_merge_result_id: "result-final-concurrent".into(),
observed_target_commit: "h1".into(),
implementation_assignment_id: "A1".into(),
completion_actor_runtime_id: "OR".into(),
completion_actor_worker_id: "OW".into(),
@@ -641,10 +723,22 @@ fn operation_key_mismatch_and_actor_or_assignment_change_are_fenced() {
open(&store);
attempt(&store, "AT", "V1", "token", "child");
review(&store, "V1", "token", ReviewDecision::Approve).unwrap();
record_result(
&store,
"V1",
"base",
"h1",
"h1",
MergeStrategy::FastForward,
MergeResolution::None,
"final-operation",
);
let mut input = CompleteMergeRequest {
operation_id: "OP".into(),
ticket_id: "T1".into(),
expected_revision_id: "V1".into(),
expected_merge_result_id: "result-final-operation".into(),
observed_target_commit: "h1".into(),
implementation_assignment_id: "A1".into(),
completion_actor_runtime_id: "OR".into(),
completion_actor_worker_id: "OW".into(),
-4
View File
@@ -94,10 +94,6 @@ 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"
)]
+107 -88
View File
@@ -1314,10 +1314,6 @@ pub fn build_router(api: WorkspaceApi) -> Router {
"/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",
post(scoped_close_ticket_record),
@@ -3596,12 +3592,6 @@ struct RevisionTransitionRequest {
explicit_confirmation: bool,
}
#[derive(Debug, serde::Deserialize)]
struct ConfirmMergeRequestRequest {
expected_revision_id: String,
explicit_confirmation: bool,
}
fn parse_workspace_id(value: &str) -> ApiResult<String> {
if value.trim().is_empty() {
return Err(Error::InvalidInput("workspace_id must not be empty".to_string()).into());
@@ -4047,58 +4037,6 @@ async fn scoped_complete_merge_request(
.ok_or_else(|| {
Error::TicketAssignmentConflict("Ticket has no current assigned Coder".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,
implementation_assignment_id: assignment.assignment_id,
completion_actor_runtime_id: source.runtime_id,
completion_actor_worker_id: source.worker_id,
now: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true),
},
)?;
Ok(Json(outcome))
}
async fn scoped_reopen_merge_request(
State(api): State<WorkspaceApi>,
headers: HeaderMap,
AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>,
Json(input): Json<RevisionTransitionRequest>,
) -> ApiResult<Json<merge_request::MergeRequest>> {
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<WorkspaceApi>,
headers: HeaderMap,
AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>,
Json(input): Json<ConfirmMergeRequestRequest>,
) -> ApiResult<Json<merge_request::MergeRequest>> {
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 store = merge_request_store(&api, &workspace_id)?;
let current = store.show_for_ticket(&ticket_id)?.ok_or_else(|| {
Error::from(merge_request::MergeRequestError::NotFound(
@@ -4113,28 +4051,56 @@ async fn scoped_confirm_merge_request(
ticket_id.clone(),
))
})?;
if observed.applied_merge_result.is_none() {
return Err(Error::InvalidInput(
"Merge Request result is not the current target tip; target update is a separate prerequisite operation".into(),
).into());
let final_result = observed
.final_merge_result
.as_ref()
.ok_or(merge_request::MergeRequestError::FinalMergeResultMissing)?;
if final_result.target_status != merge_request::MergeResultTargetStatus::Applied {
return Err(merge_request::MergeRequestError::FinalMergeResultNotApplied.into());
}
let readiness = store.readiness_for_ticket_with_target(&ticket_id, target_commit.as_deref())?;
if !readiness.ready {
return Err(Error::InvalidInput(format!(
"Merge Request is not integration-ready: {}",
"Merge Request is not completion-ready: {}",
readiness.blockers.join("; ")
))
.into());
}
let mr = store.confirm_merge(merge_request::MergeConfirmation {
let outcome = store.complete(merge_request::CompleteMergeRequest {
operation_id: input.operation_id,
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,
expected_merge_result_id: final_result.merge_result_id.clone(),
observed_target_commit: target_commit
.ok_or(merge_request::MergeRequestError::FinalMergeResultNotApplied)?,
implementation_assignment_id: assignment.assignment_id,
completion_actor_runtime_id: source.runtime_id,
completion_actor_worker_id: source.worker_id,
now: Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true),
})?;
Ok(Json(mr))
Ok(Json(outcome))
}
async fn scoped_reopen_merge_request(
State(api): State<WorkspaceApi>,
headers: HeaderMap,
AxumPath((workspace_id, ticket_id)): AxumPath<(String, String)>,
Json(input): Json<RevisionTransitionRequest>,
) -> ApiResult<Json<merge_request::MergeRequest>> {
let workspace_id = parse_workspace_id(&workspace_id)?;
require_workspace_access(&workspace_id, &api)?;
if headers.contains_key("authorization") {
return Err(Error::BrowserReopenConfirmationRequired.into());
}
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),
)?))
}
async fn scoped_close_ticket_record(
@@ -11590,9 +11556,7 @@ impl ApiError {
impl IntoResponse for ApiError {
fn into_response(self) -> Response {
let status = match &self.error {
Error::BrowserMergeConfirmationRequired | Error::BrowserReopenConfirmationRequired => {
StatusCode::FORBIDDEN
}
Error::BrowserReopenConfirmationRequired => StatusCode::FORBIDDEN,
Error::TicketAssignmentConflict(_)
| Error::WorkdirAttachmentConflict(_)
| Error::WorkspaceConfigConflict(_) => StatusCode::CONFLICT,
@@ -11755,17 +11719,6 @@ 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 {
@@ -12412,6 +12365,46 @@ mod tests {
async fn merge_request_completion_endpoint_rejects_coder_and_accepts_orchestrator() {
let workspace = tempfile::tempdir().unwrap();
init_clean_git_workspace(workspace.path());
let git_output = |args: &[&str]| {
let output = std::process::Command::new("git")
.arg("-C")
.arg(workspace.path())
.args(args)
.output()
.unwrap();
assert!(output.status.success(), "git {:?} failed", args);
String::from_utf8(output.stdout).unwrap().trim().to_string()
};
let base_commit = git_output(&["rev-parse", "HEAD"]);
std::fs::write(workspace.path().join("README.md"), "completion candidate\n").unwrap();
assert!(
std::process::Command::new("git")
.arg("-C")
.arg(workspace.path())
.args(["add", "README.md"])
.status()
.unwrap()
.success()
);
assert!(
std::process::Command::new("git")
.arg("-C")
.arg(workspace.path())
.args(["commit", "-m", "candidate"])
.status()
.unwrap()
.success()
);
let head_commit = git_output(&["rev-parse", "HEAD"]);
assert!(
std::process::Command::new("git")
.arg("-C")
.arg(workspace.path())
.args(["reset", "--hard", &base_commit])
.status()
.unwrap()
.success()
);
let api = test_api(workspace.path()).await;
let workspace_id = api.config.workspace_id.clone();
let backend = browser_ticket_backend(&api).unwrap();
@@ -12452,8 +12445,8 @@ mod tests {
revision: merge_request::MergeRequestRevision {
revision_id: "V1".into(),
ordinal: 1,
base_commit: "base".into(),
head_commit: "head".into(),
base_commit: base_commit.clone(),
head_commit: head_commit.clone(),
diff_digest: "sha256:diff".into(),
changed_paths: vec!["src/lib.rs".into()],
@@ -12500,6 +12493,31 @@ mod tests {
now: "t3".into(),
})
.unwrap();
mr_store
.record_merge_result(merge_request::RecordMergeResult {
operation_id: "record-complete-result".into(),
merge_result_id: "result-complete".into(),
ticket_id: ticket.id.clone(),
expected_revision_id: "V1".into(),
target_commit: base_commit.clone(),
source_commit: head_commit.clone(),
result_commit: head_commit.clone(),
strategy: merge_request::MergeStrategy::FastForward,
resolution: merge_request::MergeResolution::None,
actor_runtime_id: coder.worker_ref.runtime_id.clone(),
actor_worker_id: coder.worker_ref.worker_id.clone(),
created_at: "t4".into(),
})
.unwrap();
assert!(
std::process::Command::new("git")
.arg("-C")
.arg(workspace.path())
.args(["reset", "--hard", &head_commit])
.status()
.unwrap()
.success()
);
let worker_headers = |worker: &RuntimeWorkerRef| {
let mut headers = HeaderMap::new();
@@ -13400,6 +13418,7 @@ mod tests {
for args in [
vec!["add", "README.md", ".gitignore"],
vec!["commit", "-m", "init"],
vec!["branch", "-M", "develop"],
] {
let status = std::process::Command::new("git")
.arg("-C")
@@ -248,8 +248,8 @@ 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("/merge-request/merge") &&
ticketDetailPage.includes("explicit_confirmation: true") &&
!ticketDetailPage.includes("/merge-request/merge") &&
ticketDetailPage.includes("final_merge_result") &&
!ticketDetailPage.includes('mutate("review", "/review"') &&
ticketDetailPage.includes('mutate("close", "/close"') &&
ticketDetailPage.includes("ticketWorkerLaunchHref") &&
@@ -25,17 +25,7 @@
observed_target_commit?: string | null;
current_revision: { revision_id: string; head_commit: string; diff_digest: string; changed_paths: string[]; summary: string };
current_review?: { decision: string; body: string; reviewer_effective_profile: string } | null;
current_merge_result?: {
merge_result_id: string;
target_commit: string;
source_commit: string;
result_commit: string;
strategy: "fast_forward" | "merge";
resolution: "none" | "clean" | "conflicts_resolved";
target_status: "current" | "applied" | "stale" | "unknown";
review_status: "pending" | "approved" | "changes_requested";
} | null;
applied_merge_result?: {
final_merge_result?: {
merge_result_id: string;
target_commit: string;
source_commit: string;
@@ -78,7 +68,6 @@
let transitionReason = $state("");
let threadRole = $state("comment");
let threadBody = $state("");
let confirmMerge = $state(false);
let resolution = $state("");
let busy = $state<string | null>(null);
let errorMessage = $state<string | null>(null);
@@ -168,29 +157,6 @@
) threadBody = "";
}
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) {
event.preventDefault();
if (!resolution.trim()) return;
@@ -395,36 +361,21 @@
{#if mergeRequest.observed_target_commit}<p>Target tip <code>{mergeRequest.observed_target_commit}</code></p>{/if}
<p><code>{mergeRequest.current_revision.revision_id}</code></p>
<p>Head <code>{mergeRequest.current_revision.head_commit}</code></p>
{#if mergeRequest.current_merge_result}
{#if mergeRequest.final_merge_result}
<p>
MergeResult <code>{mergeRequest.current_merge_result.merge_result_id}</code> ·
{mergeRequest.current_merge_result.strategy} / {mergeRequest.current_merge_result.resolution} ·
{mergeRequest.current_merge_result.target_status} / {mergeRequest.current_merge_result.review_status}
Final MergeResult <code>{mergeRequest.final_merge_result.merge_result_id}</code> ·
{mergeRequest.final_merge_result.strategy} / {mergeRequest.final_merge_result.resolution} ·
{mergeRequest.final_merge_result.target_status} / {mergeRequest.final_merge_result.review_status}
</p>
<p>Result <code>{mergeRequest.current_merge_result.result_commit}</code></p>
{:else if mergeRequest.applied_merge_result}
<p>
Applied MergeResult <code>{mergeRequest.applied_merge_result.merge_result_id}</code> ·
{mergeRequest.applied_merge_result.strategy} / {mergeRequest.applied_merge_result.resolution} ·
{mergeRequest.applied_merge_result.review_status}
</p>
<p>Result <code>{mergeRequest.applied_merge_result.result_commit}</code> is the current target tip.</p>
<p>Result <code>{mergeRequest.final_merge_result.result_commit}</code></p>
{:else if mergeRequest.state === "open"}
<p class="workspace-callout">No current MergeResult has been recorded for this target tip.</p>
<p class="workspace-callout">No final MergeResult has been selected for this revision.</p>
{/if}
{#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"
&& mergeRequest.applied_merge_result?.target_status === "applied"
&& (mergeRequest.applied_merge_result.strategy === "fast_forward"
|| mergeRequest.applied_merge_result.review_status === "approved")}
<label><input type="checkbox" bind:checked={confirmMerge} /> Explicitly confirm the externally applied result</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}