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

fix: align merge requests with selector thread contract

Browse Source
This commit is contained in:
Hare
2026-08-17 07:41:03 +09:00
parent 9e48cae759
commit 1fe1b7463f
13 changed files with 1354 additions and 2658 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/worker/src/feature/builtin/merge_request.rs
+98 -210
View File
@@ -11,77 +11,51 @@ pub const MERGE_REQUEST_COMMON_TOOL_NAMES: &[&str] = &[
"MergeRequestShow",
"MergeRequestReadinessCheck",
"MergeRequestOpen",
"MergeRequestRequestReview",
"MergeRequestComplete",
];
pub const MERGE_REQUEST_REVIEW_TOOL_NAME: &str = "MergeRequestReviewSubmit";
#[derive(Clone, Copy)]
enum Kind {
Show,
Readiness,
Open,
RequestReview,
Complete,
Review,
}
#[derive(Clone)]
struct MergeRequestTool {
client: Arc<dyn WorkspaceClient>,
kind: Kind,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct ShowInput {
ticket: String,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct OpenInput {
ticket: String,
repository_id: String,
selector_from: String,
selector_to: String,
base_commit: String,
head_commit: String,
#[serde(default)]
changed_paths: Vec<String>,
#[serde(default)]
summary: String,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct RequestReviewInput {
ticket: String,
expected_head_commit: String,
base_commit: String,
head_commit: String,
#[serde(default)]
changed_paths: Vec<String>,
#[serde(default)]
summary: String,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct CompleteInput {
ticket: String,
operation_id: String,
expected_head_commit: String,
target_commit: String,
source_commit: String,
result_commit: String,
approval_event_id: String,
target_ref_before: String,
target_ref_after: String,
strategy: MergeStrategyInput,
resolution: MergeResolutionInput,
}
#[derive(Debug, Deserialize, JsonSchema)]
#[serde(rename_all = "snake_case")]
enum MergeStrategyInput {
FastForward,
Merge,
}
#[derive(Debug, Deserialize, JsonSchema)]
#[serde(rename_all = "snake_case")]
enum MergeResolutionInput {
@@ -89,7 +63,6 @@ enum MergeResolutionInput {
Clean,
ConflictsResolved,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct ReviewInput {
decision: ReviewDecisionInput,
@@ -98,211 +71,133 @@ struct ReviewInput {
#[serde(default)]
findings: Vec<ReviewFindingInput>,
}
#[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<String>,
#[serde(default)]
path: Option<String>,
#[serde(default)]
line: Option<u32>,
message: String,
body: String,
}
impl Kind {
fn name(self) -> &'static str {
match self {
Self::Show => "MergeRequestShow",
Self::Readiness => "MergeRequestReadinessCheck",
Self::Open => "MergeRequestOpen",
Self::RequestReview => "MergeRequestRequestReview",
Self::Complete => "MergeRequestComplete",
Self::Review => "MergeRequestReviewSubmit",
}
}
fn description(self) -> &'static str {
description(self.name()).unwrap_or("Merge Request operation.")
}
fn schema(self) -> serde_json::Value {
match self {
Self::Show | Self::Readiness => json!(schemars::schema_for!(ShowInput)),
Self::Open => json!(schemars::schema_for!(OpenInput)),
Self::RequestReview => json!(schemars::schema_for!(RequestReviewInput)),
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<ToolOutput, ToolError> {
let workspace_id = self.client.workspace_id().ok_or_else(|| {
async fn execute(&self, input: &str, _: ToolExecutionContext) -> Result<ToolOutput, ToolError> {
let ws = 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 value: ShowInput = parse(input)?;
nonempty(&value.ticket)?;
Kind::Show | Kind::Readiness => {
let v: ShowInput = parse(input)?;
nonempty(&v.ticket)?;
(
WorkspaceRequestMethod::Get,
format!(
"/api/w/{workspace_id}/tickets/{}/merge-request",
value.ticket
),
None,
)
}
Kind::Readiness => {
let value: ShowInput = parse(input)?;
nonempty(&value.ticket)?;
(
WorkspaceRequestMethod::Get,
format!(
"/api/w/{workspace_id}/tickets/{}/merge-request/readiness",
value.ticket
"/api/w/{ws}/tickets/{}/merge-request{}",
v.ticket,
if matches!(self.kind, Kind::Readiness) {
"/readiness"
} else {
""
}
),
None,
)
}
Kind::Open => {
let value: OpenInput = parse(input)?;
nonempty(&value.ticket)?;
let v: OpenInput = parse(input)?;
nonempty(&v.ticket)?;
(
WorkspaceRequestMethod::Post,
format!(
"/api/w/{workspace_id}/tickets/{}/merge-request",
value.ticket
format!("/api/w/{ws}/tickets/{}/merge-request", v.ticket),
Some(
json!({"repository_id":v.repository_id,"selector_from":v.selector_from,"selector_to":v.selector_to,"summary":v.summary}),
),
Some(json!({
"repository_id": value.repository_id,
"selector_from": value.selector_from,
"selector_to": value.selector_to,
"base_commit": value.base_commit,
"head_commit": value.head_commit,
"changed_paths": value.changed_paths,
"summary": value.summary,
})),
)
}
Kind::RequestReview => {
let value: RequestReviewInput = parse(input)?;
nonempty(&value.ticket)?;
(
WorkspaceRequestMethod::Post,
format!(
"/api/w/{workspace_id}/tickets/{}/merge-request/review-requests",
value.ticket
),
Some(json!({
"expected_head_commit": value.expected_head_commit,
"base_commit": value.base_commit,
"head_commit": value.head_commit,
"changed_paths": value.changed_paths,
"summary": value.summary,
})),
)
}
Kind::Complete => {
let value: CompleteInput = parse(input)?;
nonempty(&value.ticket)?;
let strategy = match value.strategy {
MergeStrategyInput::FastForward => "fast_forward",
MergeStrategyInput::Merge => "merge",
};
let resolution = match value.resolution {
MergeResolutionInput::None => "none",
MergeResolutionInput::Clean => "clean",
MergeResolutionInput::ConflictsResolved => "conflicts_resolved",
};
let v: CompleteInput = parse(input)?;
nonempty(&v.ticket)?;
(
WorkspaceRequestMethod::Post,
format!(
"/api/w/{workspace_id}/tickets/{}/merge-request/complete",
value.ticket
format!("/api/w/{ws}/tickets/{}/merge-request/complete", v.ticket),
Some(
json!({"operation_id":v.operation_id,"approval_event_id":v.approval_event_id,"target_ref_before":v.target_ref_before,"target_ref_after":v.target_ref_after,"strategy":match v.strategy{MergeStrategyInput::FastForward=>"fast_forward",MergeStrategyInput::Merge=>"merge"},"resolution":match v.resolution{MergeResolutionInput::None=>"none",MergeResolutionInput::Clean=>"clean",MergeResolutionInput::ConflictsResolved=>"conflicts_resolved"}}),
),
Some(json!({
"operation_id": value.operation_id,
"expected_head_commit": value.expected_head_commit,
"target_commit": value.target_commit,
"source_commit": value.source_commit,
"result_commit": value.result_commit,
"strategy": strategy,
"resolution": resolution,
})),
)
}
Kind::Review => {
let value: ReviewInput = parse(input)?;
let context = self.client.reviewer_context().ok_or_else(|| {
let v: ReviewInput = parse(input)?;
let ctx = self.client.reviewer_context().ok_or_else(|| {
ToolError::ExecutionFailed(
"MergeRequestReviewSubmit is available only to an attested Reviewer child"
.into(),
"Review submit requires injected Reviewer capability".into(),
)
})?;
(
WorkspaceRequestMethod::Post,
format!(
"/api/w/{workspace_id}/tickets/{}/merge-request/reviews",
context.ticket_id
"/api/w/{ws}/tickets/{}/merge-request/reviews",
ctx.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::<Vec<_>>() }),
),
Some(json!({
"decision": match value.decision {
ReviewDecisionInput::Approve => "approve",
ReviewDecisionInput::RequestChanges => "request_changes",
},
"body": value.body,
"findings": value.findings.into_iter().map(|finding| json!({
"severity": finding.severity,
"path": finding.path,
"line": finding.line,
"message": finding.message,
})).collect::<Vec<_>>(),
})),
)
}
};
let request = match body {
Some(body) => WorkspaceRequest::json(method, path, body.to_string()),
let req = match body {
Some(v) => WorkspaceRequest::json(method, path, v.to_string()),
None => WorkspaceRequest::get(path),
};
let response = self
let res = self
.client
.execute(request)
.map_err(|error| ToolError::ExecutionFailed(error.to_string()))?;
if !response.is_success() {
.execute(req)
.map_err(|e| ToolError::ExecutionFailed(e.to_string()))?;
if !res.is_success() {
return Err(ToolError::ExecutionFailed(format!(
"Merge Request API returned HTTP {}: {}",
response.status, response.body
res.status, res.body
)));
}
Ok(ToolOutput {
summary: self.kind.name().to_string(),
content: Some(response.body),
attachments: Vec::new(),
summary: self.kind.name().into(),
content: Some(res.body),
attachments: vec![],
})
}
}
fn parse<T: serde::de::DeserializeOwned>(value: &str) -> Result<T, ToolError> {
serde_json::from_str(value).map_err(|error| ToolError::InvalidArgument(error.to_string()))
fn parse<T: serde::de::DeserializeOwned>(v: &str) -> Result<T, ToolError> {
serde_json::from_str(v).map_err(|e| ToolError::InvalidArgument(e.to_string()))
}
fn nonempty(value: &str) -> Result<(), ToolError> {
if value.trim().is_empty() {
fn nonempty(v: &str) -> Result<(), ToolError> {
if v.trim().is_empty() {
Err(ToolError::InvalidArgument(
"ticket must not be empty".into(),
))
@@ -310,84 +205,77 @@ fn nonempty(value: &str) -> Result<(), ToolError> {
Ok(())
}
}
fn definition(client: Arc<dyn WorkspaceClient>, kind: Kind) -> ToolDefinition {
Arc::new(move || {
let meta = ToolMeta::new(kind.name())
.description(kind.description())
.input_schema(kind.schema());
let tool: Arc<dyn Tool> = Arc::new(MergeRequestTool {
client: client.clone(),
kind,
});
(meta, tool)
(
ToolMeta::new(kind.name())
.description(description(kind.name()).unwrap_or("Merge Request operation."))
.input_schema(kind.schema()),
Arc::new(MergeRequestTool {
client: client.clone(),
kind,
}) as Arc<dyn Tool>,
)
})
}
pub fn common_tools(client: Arc<dyn WorkspaceClient>) -> Vec<ToolDefinition> {
pub fn common_tools(c: Arc<dyn WorkspaceClient>) -> Vec<ToolDefinition> {
vec![
definition(client.clone(), Kind::Show),
definition(client.clone(), Kind::Readiness),
definition(client.clone(), Kind::Open),
definition(client.clone(), Kind::RequestReview),
definition(client, Kind::Complete),
definition(c.clone(), Kind::Show),
definition(c.clone(), Kind::Readiness),
definition(c.clone(), Kind::Open),
definition(c, Kind::Complete),
]
}
pub fn reviewer_tools(client: Arc<dyn WorkspaceClient>) -> Vec<ToolDefinition> {
if client.reviewer_context().is_some() {
pub fn reviewer_tools(c: Arc<dyn WorkspaceClient>) -> Vec<ToolDefinition> {
if c.reviewer_context().is_some() {
vec![
definition(client.clone(), Kind::Show),
definition(client, Kind::Review),
definition(c.clone(), Kind::Show),
definition(c, Kind::Review),
]
} else {
Vec::new()
vec![]
}
}
pub fn description(name: &str) -> Option<&'static str> {
match name {
"MergeRequestShow" => Some(
"Read the authoritative Merge Request, selector pair, append-only thread, and current review status.",
),
pub fn description(n: &str) -> Option<&'static str> {
match n {
"MergeRequestShow" => Some("Read the selector-based Merge Request and append-only thread."),
"MergeRequestReadinessCheck" => {
Some("Check derived merge readiness for the current review request.")
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.")
}
"MergeRequestOpen" => Some(
"Open a Merge Request with immutable source/target selectors and its first review request.",
),
"MergeRequestRequestReview" => Some(
"Append a RequestForReview event for new candidate evidence; prior approval cannot carry forward.",
),
"MergeRequestComplete" => Some(
"CAS-complete the approved current candidate with operation-id replay and crash fencing.",
),
"MergeRequestReviewSubmit" => {
Some("Submit the attested direct-child Reviewer result for the current candidate.")
Some("Submit the injected Reviewer capability result for its captured subject ref.")
}
_ => None,
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn merge_request_tool_contract_uses_selectors_and_commit_fences_without_revision_ids() {
let open = serde_json::to_string(&schemars::schema_for!(OpenInput)).unwrap();
let request = serde_json::to_string(&schemars::schema_for!(RequestReviewInput)).unwrap();
let complete = serde_json::to_string(&schemars::schema_for!(CompleteInput)).unwrap();
assert!(open.contains("selector_from"));
assert!(open.contains("selector_to"));
assert!(request.contains("expected_head_commit"));
assert!(complete.contains("expected_head_commit"));
for schema in [&open, &request, &complete] {
assert!(!schema.contains("revision_id"));
assert!(!schema.contains("attempt_id"));
assert!(!schema.contains("head_tree"));
assert!(!schema.contains("diff_digest"));
fn schemas_hide_revision_and_commit_authority() {
let schemas = [
schemars::schema_for!(OpenInput),
schemars::schema_for!(CompleteInput),
];
for s in schemas {
let j = serde_json::to_string(&s).unwrap();
for banned in [
"revision_id",
"attempt_id",
"base_commit",
"head_commit",
"source_commit",
"result_commit",
] {
assert!(!j.contains(banned), "{banned} in {j}")
}
}
assert!(!MERGE_REQUEST_COMMON_TOOL_NAMES.contains(&"MergeRequestAddRevision"));
assert!(!MERGE_REQUEST_COMMON_TOOL_NAMES.contains(&"MergeRequestRequestReview"));
}
}
crates/worker/src/spawn/tool.rs
+7 -12
View File
@@ -67,7 +67,6 @@ struct SubWorkerSpawnInput {
#[derive(Debug, Deserialize, schemars::JsonSchema)]
struct ReviewerHandoffInput {
ticket_id: String,
expected_head_commit: String,
}
#[derive(Debug, Deserialize, schemars::JsonSchema)]
@@ -337,9 +336,9 @@ fn validate_reviewer_handoff(input: &SubWorkerSpawnInput) -> Result<(), ToolErro
let Some(review) = &input.review else {
return Ok(());
};
if review.ticket_id.trim().is_empty() || review.expected_head_commit.trim().is_empty() {
if review.ticket_id.trim().is_empty() {
return Err(ToolError::InvalidArgument(
"reviewer handoff requires non-empty ticket_id and expected_head_commit".to_string(),
"reviewer handoff requires non-empty ticket_id".to_string(),
));
}
if input.profile.as_deref() != Some("builtin:reviewer") {
@@ -421,7 +420,6 @@ impl Tool for SubWorkerSpawnTool {
let reviewer_capability = input.review.as_ref().map(|review| {
(
review.ticket_id.clone(),
review.expected_head_commit.clone(),
format!(
"{}{}",
uuid::Uuid::now_v7().simple(),
@@ -430,8 +428,7 @@ impl Tool for SubWorkerSpawnTool {
)
});
let child_workspace_context =
if let Some((ticket_id, expected_head_commit, capability_token)) = &reviewer_capability
{
if let Some((ticket_id, capability_token)) = &reviewer_capability {
let workspace_id =
self.workspace_context
.workspace_id()
@@ -452,7 +449,6 @@ impl Tool for SubWorkerSpawnTool {
parent_client.clone(),
ReviewerContext {
ticket_id: ticket_id.clone(),
expected_head_commit: expected_head_commit.clone(),
},
capability_token.clone(),
));
@@ -547,7 +543,7 @@ impl Tool for SubWorkerSpawnTool {
}
};
if let Some((ticket_id, expected_head_commit, capability_token)) = &reviewer_capability {
if let Some((ticket_id, capability_token)) = &reviewer_capability {
let workspace_id = self.workspace_context.workspace_id().ok_or_else(|| {
ToolError::ExecutionFailed("review capability lost Workspace identity".to_string())
})?;
@@ -577,7 +573,6 @@ impl Tool for SubWorkerSpawnTool {
)));
}
let body = serde_json::json!({
"expected_head_commit": expected_head_commit,
"child_session_id": child_session_id,
"capability_token": capability_token,
});
@@ -1043,21 +1038,21 @@ mod tests {
let valid: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({
"name":"reviewer","task":"review","profile":"builtin:reviewer",
"scope":[{"target":"/tmp/work","permission":"read"}],
"review":{"ticket_id":"T1","expected_head_commit":"V1"}
"review":{"ticket_id":"T1"}
}))
.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","expected_head_commit":"V1"}
"review":{"ticket_id":"T1"}
}))
.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","expected_head_commit":"V1"}
"review":{"ticket_id":"T1"}
}))
.unwrap();
assert!(validate_reviewer_handoff(&writable).is_err());
crates/worker/src/worker.rs
-6
View File
@@ -248,7 +248,6 @@ pub trait WorkspaceClient: std::fmt::Debug + Send + Sync {
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ReviewerContext {
pub ticket_id: String,
pub expected_head_commit: String,
}
#[derive(Debug)]
@@ -306,10 +305,6 @@ impl WorkspaceClient for ReviewerChildWorkspaceClient {
"review submission body must be an object".to_string(),
)
})?;
object.insert(
"expected_head_commit".to_string(),
serde_json::Value::String(self.context.expected_head_commit.clone()),
);
object.insert(
"capability_token".to_string(),
serde_json::Value::String(self.capability_token.clone()),
@@ -452,7 +447,6 @@ mod reviewer_client_tests {
inner,
ReviewerContext {
ticket_id: "T1".into(),
expected_head_commit: "head".into(),
},
"secret".into(),
);