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

feat: expose selector based merge request threads

Browse Source
This commit is contained in:
Hare
2026-08-17 07:09:20 +09:00
parent aeb2727bea
commit 9e48cae759
10 changed files with 547 additions and 452 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/worker/src/feature/builtin/merge_request.rs
+127 -75
View File
@@ -11,19 +11,21 @@ pub const MERGE_REQUEST_COMMON_TOOL_NAMES: &[&str] = &[
"MergeRequestShow",
"MergeRequestReadinessCheck",
"MergeRequestOpen",
"MergeRequestAddRevision",
"MergeRequestRequestReview",
"MergeRequestComplete",
];
pub const MERGE_REQUEST_REVIEW_TOOL_NAME: &str = "MergeRequestReviewSubmit";
#[derive(Clone, Copy)]
enum Kind {
Show,
Readiness,
Open,
AddRevision,
RequestReview,
Complete,
Review,
}
#[derive(Clone)]
struct MergeRequestTool {
client: Arc<dyn WorkspaceClient>,
@@ -34,11 +36,13 @@ struct MergeRequestTool {
struct ShowInput {
ticket: String,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct OpenInput {
ticket: String,
repository_id: String,
revision_id: String,
selector_from: String,
selector_to: String,
base_commit: String,
head_commit: String,
#[serde(default)]
@@ -46,11 +50,11 @@ struct OpenInput {
#[serde(default)]
summary: String,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct AddRevisionInput {
struct RequestReviewInput {
ticket: String,
expected_current_revision_id: String,
revision_id: String,
expected_head_commit: String,
base_commit: String,
head_commit: String,
#[serde(default)]
@@ -58,23 +62,26 @@ struct AddRevisionInput {
#[serde(default)]
summary: String,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct CompleteInput {
ticket: String,
operation_id: String,
expected_revision_id: String,
expected_head_commit: String,
target_commit: String,
source_commit: String,
result_commit: 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 {
@@ -82,6 +89,7 @@ enum MergeResolutionInput {
Clean,
ConflictsResolved,
}
#[derive(Debug, Deserialize, JsonSchema)]
struct ReviewInput {
decision: ReviewDecisionInput,
@@ -90,22 +98,22 @@ 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<u64>,
body: String,
line: Option<u32>,
message: String,
}
impl Kind {
@@ -114,19 +122,21 @@ impl Kind {
Self::Show => "MergeRequestShow",
Self::Readiness => "MergeRequestReadinessCheck",
Self::Open => "MergeRequestOpen",
Self::AddRevision => "MergeRequestAddRevision",
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::AddRevision => json!(schemars::schema_for!(AddRevisionInput)),
Self::RequestReview => json!(schemars::schema_for!(RequestReviewInput)),
Self::Complete => json!(schemars::schema_for!(CompleteInput)),
Self::Review => json!(schemars::schema_for!(ReviewInput)),
}
@@ -145,59 +155,75 @@ impl Tool for MergeRequestTool {
})?;
let (method, path, body) = match self.kind {
Kind::Show => {
let v: ShowInput = parse(input)?;
nonempty(&v.ticket)?;
let value: ShowInput = parse(input)?;
nonempty(&value.ticket)?;
(
WorkspaceRequestMethod::Get,
format!("/api/w/{workspace_id}/tickets/{}/merge-request", v.ticket),
format!(
"/api/w/{workspace_id}/tickets/{}/merge-request",
value.ticket
),
None,
)
}
Kind::Readiness => {
let v: ShowInput = parse(input)?;
nonempty(&v.ticket)?;
let value: ShowInput = parse(input)?;
nonempty(&value.ticket)?;
(
WorkspaceRequestMethod::Get,
format!(
"/api/w/{workspace_id}/tickets/{}/merge-request/readiness",
v.ticket
value.ticket
),
None,
)
}
Kind::Open => {
let v: OpenInput = parse(input)?;
nonempty(&v.ticket)?;
(
WorkspaceRequestMethod::Post,
format!("/api/w/{workspace_id}/tickets/{}/merge-request", v.ticket),
Some(
json!({"repository_id":v.repository_id,"revision_id":v.revision_id,"base_commit":v.base_commit,"head_commit":v.head_commit,"changed_paths":v.changed_paths,"summary":v.summary}),
),
)
}
Kind::AddRevision => {
let v: AddRevisionInput = parse(input)?;
nonempty(&v.ticket)?;
let value: OpenInput = parse(input)?;
nonempty(&value.ticket)?;
(
WorkspaceRequestMethod::Post,
format!(
"/api/w/{workspace_id}/tickets/{}/merge-request/revisions",
v.ticket
"/api/w/{workspace_id}/tickets/{}/merge-request",
value.ticket
),
Some(
json!({"expected_current_revision_id":v.expected_current_revision_id,"revision_id":v.revision_id,"base_commit":v.base_commit,"head_commit":v.head_commit,"changed_paths":v.changed_paths,"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 v: CompleteInput = parse(input)?;
nonempty(&v.ticket)?;
let strategy = match v.strategy {
let value: CompleteInput = parse(input)?;
nonempty(&value.ticket)?;
let strategy = match value.strategy {
MergeStrategyInput::FastForward => "fast_forward",
MergeStrategyInput::Merge => "merge",
};
let resolution = match v.resolution {
let resolution = match value.resolution {
MergeResolutionInput::None => "none",
MergeResolutionInput::Clean => "clean",
MergeResolutionInput::ConflictsResolved => "conflicts_resolved",
@@ -206,16 +232,22 @@ impl Tool for MergeRequestTool {
WorkspaceRequestMethod::Post,
format!(
"/api/w/{workspace_id}/tickets/{}/merge-request/complete",
v.ticket
),
Some(
json!({"operation_id":v.operation_id,"expected_revision_id":v.expected_revision_id,"target_commit":v.target_commit,"source_commit":v.source_commit,"result_commit":v.result_commit,"strategy":strategy,"resolution":resolution}),
value.ticket
),
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 v: ReviewInput = parse(input)?;
let context = self.client.reviewer_attempt_context().ok_or_else(|| {
let value: ReviewInput = parse(input)?;
let context = self.client.reviewer_context().ok_or_else(|| {
ToolError::ExecutionFailed(
"MergeRequestReviewSubmit is available only to an attested Reviewer child"
.into(),
@@ -227,9 +259,19 @@ impl Tool for MergeRequestTool {
"/api/w/{workspace_id}/tickets/{}/merge-request/reviews",
context.ticket_id
),
Some(
json!({"decision":match v.decision{ReviewDecisionInput::Approve=>"approve",ReviewDecisionInput::RequestChanges=>"request_changes"},"body":v.body,"findings":v.findings.into_iter().map(|f|json!({"severity":f.severity,"code":f.code,"path":f.path,"line":f.line,"body":f.body})).collect::<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<_>>(),
})),
)
}
};
@@ -240,7 +282,7 @@ impl Tool for MergeRequestTool {
let response = self
.client
.execute(request)
.map_err(|e| ToolError::ExecutionFailed(e.to_string()))?;
.map_err(|error| ToolError::ExecutionFailed(error.to_string()))?;
if !response.is_success() {
return Err(ToolError::ExecutionFailed(format!(
"Merge Request API returned HTTP {}: {}",
@@ -254,9 +296,11 @@ impl Tool for MergeRequestTool {
})
}
}
fn parse<T: serde::de::DeserializeOwned>(value: &str) -> Result<T, ToolError> {
serde_json::from_str(value).map_err(|e| ToolError::InvalidArgument(e.to_string()))
serde_json::from_str(value).map_err(|error| ToolError::InvalidArgument(error.to_string()))
}
fn nonempty(value: &str) -> Result<(), ToolError> {
if value.trim().is_empty() {
Err(ToolError::InvalidArgument(
@@ -266,6 +310,7 @@ 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())
@@ -278,17 +323,19 @@ fn definition(client: Arc<dyn WorkspaceClient>, kind: Kind) -> ToolDefinition {
(meta, tool)
})
}
pub fn common_tools(client: 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::AddRevision),
definition(client.clone(), Kind::RequestReview),
definition(client, Kind::Complete),
]
}
pub fn reviewer_tools(client: Arc<dyn WorkspaceClient>) -> Vec<ToolDefinition> {
if client.reviewer_attempt_context().is_some() {
if client.reviewer_context().is_some() {
vec![
definition(client.clone(), Kind::Show),
definition(client, Kind::Review),
@@ -297,26 +344,27 @@ pub fn reviewer_tools(client: Arc<dyn WorkspaceClient>) -> Vec<ToolDefinition> {
Vec::new()
}
}
pub fn description(name: &str) -> Option<&'static str> {
match name {
"MergeRequestShow" => Some(
"Read the authoritative Merge Request, immutable current revision, and structured review status.",
"Read the authoritative Merge Request, selector pair, append-only thread, and current review status.",
),
"MergeRequestReadinessCheck" => {
Some("Check derived merge readiness for the current immutable revision.")
Some("Check derived merge readiness for the current review request.")
}
"MergeRequestOpen" => {
Some("Open an immutable Merge Request revision for the current assigned Coder.")
}
"MergeRequestAddRevision" => {
Some("Append an immutable revision; prior approval cannot carry to the new revision.")
}
"MergeRequestComplete" => {
Some("CAS-complete an approved revision with operation-id replay and crash fencing.")
}
"MergeRequestReviewSubmit" => Some(
"Submit the attested direct-child Reviewer result bound to its immutable revision.",
"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.")
}
_ => None,
}
}
@@ -326,16 +374,20 @@ mod tests {
use super::*;
#[test]
fn merge_request_tool_contract_omits_redundant_revision_evidence_and_candidate_result_tool() {
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 add = serde_json::to_string(&schemars::schema_for!(AddRevisionInput)).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("head_tree"));
assert!(!add.contains("head_tree"));
assert!(!open.contains("diff_digest"));
assert!(!add.contains("diff_digest"));
assert!(complete.contains("result_commit"));
assert!(complete.contains("conflicts_resolved"));
assert!(!MERGE_REQUEST_COMMON_TOOL_NAMES.contains(&"MergeRequestRecordMergeResult"));
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"));
}
assert!(!MERGE_REQUEST_COMMON_TOOL_NAMES.contains(&"MergeRequestAddRevision"));
}
}
crates/worker/src/feature/builtin/ticket.rs
+2 -2
View File
@@ -366,7 +366,7 @@ impl FeatureModule for TicketFeature {
));
}
if let TicketFeatureBackend::WorkspaceClient(client) = &self.backend {
let names: Vec<&str> = if client.reviewer_attempt_context().is_some() {
let names: Vec<&str> = if client.reviewer_context().is_some() {
vec![
"MergeRequestShow",
merge_request::MERGE_REQUEST_REVIEW_TOOL_NAME,
@@ -426,7 +426,7 @@ impl FeatureModule for TicketFeature {
tools.register(ToolContribution::new(name, definition))?;
}
if let TicketFeatureBackend::WorkspaceClient(client) = &self.backend {
let definitions = if client.reviewer_attempt_context().is_some() {
let definitions = if client.reviewer_context().is_some() {
merge_request::reviewer_tools(client.clone())
} else {
merge_request::common_tools(client.clone())
crates/worker/src/spawn/tool.rs
+21 -22
View File
@@ -28,7 +28,7 @@ use crate::internal_worker::{
use crate::prompt::catalog::PromptCatalog;
use crate::spawn::registry::SpawnedWorkerRegistry;
use crate::worker::{
ReviewerAttemptContext, ReviewerChildWorkspaceClient, Worker, WorkerFilesystemAuthority,
ReviewerChildWorkspaceClient, ReviewerContext, Worker, WorkerFilesystemAuthority,
WorkspaceRequest, WorkspaceRequestMethod,
};
use protocol::Method;
@@ -58,8 +58,8 @@ struct SubWorkerSpawnInput {
/// spawner's explicit delegation authority; direct tool scope alone is not
/// sufficient. Omit `recursive` for normal workspace/worktree delegation; it defaults to true.
scope: Vec<ScopeRuleInput>,
/// Binds an actual read-only builtin Reviewer child to an immutable Merge Request revision.
/// Review attempt identity and capability material are generated by the trusted spawn layer.
/// Binds an actual read-only builtin Reviewer child to the current Merge Request candidate.
/// Review capability material is generated by the trusted spawn layer.
#[serde(default)]
review: Option<ReviewerHandoffInput>,
}
@@ -67,7 +67,7 @@ struct SubWorkerSpawnInput {
#[derive(Debug, Deserialize, schemars::JsonSchema)]
struct ReviewerHandoffInput {
ticket_id: String,
revision_id: String,
expected_head_commit: String,
}
#[derive(Debug, Deserialize, schemars::JsonSchema)]
@@ -337,9 +337,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.revision_id.trim().is_empty() {
if review.ticket_id.trim().is_empty() || review.expected_head_commit.trim().is_empty() {
return Err(ToolError::InvalidArgument(
"reviewer handoff requires non-empty ticket_id and revision_id".to_string(),
"reviewer handoff requires non-empty ticket_id and expected_head_commit".to_string(),
));
}
if input.profile.as_deref() != Some("builtin:reviewer") {
@@ -418,11 +418,10 @@ impl Tool for SubWorkerSpawnTool {
.map_err(|error| {
ToolError::ExecutionFailed(format!("resolve child manifest: {error}"))
})?;
let reviewer_attempt = input.review.as_ref().map(|review| {
let reviewer_capability = input.review.as_ref().map(|review| {
(
review.ticket_id.clone(),
review.revision_id.clone(),
uuid::Uuid::now_v7().to_string(),
review.expected_head_commit.clone(),
format!(
"{}{}",
uuid::Uuid::now_v7().simple(),
@@ -431,7 +430,8 @@ impl Tool for SubWorkerSpawnTool {
)
});
let child_workspace_context =
if let Some((ticket_id, revision_id, _, capability_token)) = &reviewer_attempt {
if let Some((ticket_id, expected_head_commit, capability_token)) = &reviewer_capability
{
let workspace_id =
self.workspace_context
.workspace_id()
@@ -450,9 +450,9 @@ impl Tool for SubWorkerSpawnTool {
let child_client: Arc<dyn crate::worker::WorkspaceClient> =
Arc::new(ReviewerChildWorkspaceClient::new(
parent_client.clone(),
ReviewerAttemptContext {
ReviewerContext {
ticket_id: ticket_id.clone(),
revision_id: revision_id.clone(),
expected_head_commit: expected_head_commit.clone(),
},
capability_token.clone(),
));
@@ -547,9 +547,9 @@ impl Tool for SubWorkerSpawnTool {
}
};
if let Some((ticket_id, revision_id, attempt_id, capability_token)) = &reviewer_attempt {
if let Some((ticket_id, expected_head_commit, capability_token)) = &reviewer_capability {
let workspace_id = self.workspace_context.workspace_id().ok_or_else(|| {
ToolError::ExecutionFailed("reviewer attempt lost Workspace identity".to_string())
ToolError::ExecutionFailed("review capability lost Workspace identity".to_string())
})?;
let child_session_id = session.session_id_string();
let child_registration = WorkspaceRequest::json(
@@ -577,15 +577,14 @@ impl Tool for SubWorkerSpawnTool {
)));
}
let body = serde_json::json!({
"attempt_id": attempt_id,
"revision_id": revision_id,
"expected_head_commit": expected_head_commit,
"child_session_id": child_session_id,
"capability_token": capability_token,
});
let request = WorkspaceRequest::json(
WorkspaceRequestMethod::Post,
format!(
"/api/w/{}/tickets/{}/merge-request/review-attempts",
"/api/w/{}/tickets/{}/merge-request/review-capabilities",
workspace_id.as_str(),
ticket_id
),
@@ -596,12 +595,12 @@ impl Tool for SubWorkerSpawnTool {
.client()
.execute(request)
.map_err(|error| {
ToolError::ExecutionFailed(format!("register reviewer attempt: {error}"))
ToolError::ExecutionFailed(format!("register review capability: {error}"))
})?;
if !response.is_success() {
let _ = session.stop().await;
return Err(ToolError::ExecutionFailed(format!(
"register reviewer attempt failed with status {}: {}",
"register review capability failed with status {}: {}",
response.status, response.body
)));
}
@@ -1044,21 +1043,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","revision_id":"V1"}
"review":{"ticket_id":"T1","expected_head_commit":"V1"}
}))
.unwrap();
assert!(validate_reviewer_handoff(&valid).is_ok());
let wrong_profile: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({
"name":"reviewer","task":"review","profile":"builtin:coder",
"scope":[{"target":"/tmp/work","permission":"read"}],
"review":{"ticket_id":"T1","revision_id":"V1"}
"review":{"ticket_id":"T1","expected_head_commit":"V1"}
}))
.unwrap();
assert!(validate_reviewer_handoff(&wrong_profile).is_err());
let writable: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({
"name":"reviewer","task":"review","profile":"builtin:reviewer",
"scope":[{"target":"/tmp/work","permission":"write"}],
"review":{"ticket_id":"T1","revision_id":"V1"}
"review":{"ticket_id":"T1","expected_head_commit":"V1"}
}))
.unwrap();
assert!(validate_reviewer_handoff(&writable).is_err());
crates/worker/src/worker.rs
+11 -11
View File
@@ -238,30 +238,30 @@ pub trait WorkspaceClient: std::fmt::Debug + Send + Sync {
))
}
/// Trusted review-attempt context is injected by the Internal SubWorker spawn layer.
/// Trusted review capability context is injected by the Internal SubWorker spawn layer.
/// It is never accepted from a model-visible tool argument.
fn reviewer_attempt_context(&self) -> Option<&ReviewerAttemptContext> {
fn reviewer_context(&self) -> Option<&ReviewerContext> {
None
}
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ReviewerAttemptContext {
pub struct ReviewerContext {
pub ticket_id: String,
pub revision_id: String,
pub expected_head_commit: String,
}
#[derive(Debug)]
pub struct ReviewerChildWorkspaceClient {
inner: Arc<dyn WorkspaceClient>,
context: ReviewerAttemptContext,
context: ReviewerContext,
capability_token: String,
}
impl ReviewerChildWorkspaceClient {
pub fn new(
inner: Arc<dyn WorkspaceClient>,
context: ReviewerAttemptContext,
context: ReviewerContext,
capability_token: String,
) -> Self {
Self {
@@ -282,7 +282,7 @@ impl WorkspaceClient for ReviewerChildWorkspaceClient {
fn is_available(&self) -> bool {
self.inner.is_available()
}
fn reviewer_attempt_context(&self) -> Option<&ReviewerAttemptContext> {
fn reviewer_context(&self) -> Option<&ReviewerContext> {
Some(&self.context)
}
@@ -307,8 +307,8 @@ impl WorkspaceClient for ReviewerChildWorkspaceClient {
)
})?;
object.insert(
"revision_id".to_string(),
serde_json::Value::String(self.context.revision_id.clone()),
"expected_head_commit".to_string(),
serde_json::Value::String(self.context.expected_head_commit.clone()),
);
object.insert(
"capability_token".to_string(),
@@ -450,9 +450,9 @@ mod reviewer_client_tests {
});
let client = ReviewerChildWorkspaceClient::new(
inner,
ReviewerAttemptContext {
ReviewerContext {
ticket_id: "T1".into(),
revision_id: "V1".into(),
expected_head_commit: "head".into(),
},
"secret".into(),
);