crashlabs/huskies
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 31 Wiki Activity

fix(1102): require non-empty origin.id on create_* MCP tools

Browse Source 
bug 1102 was created today with origin={kind:user, id:""} because
build_origin silently defaulted id to empty when the caller didn't pass
one — we couldn't tell who filed it. Bug 1088's origin field is useless
as audit if every caller can omit themselves.

Changes:
- build_origin (server/src/http/mcp/story_tools/mod.rs) now returns
  Result<String, String> and rejects missing/empty/whitespace-only id
  with an instructional error pointing at bug 1102 / story 1104.
- 5 create_* tool handlers (bug, spike, refactor, epic, story) now
  resolve origin BEFORE create_*_file so an attribution-less call
  leaves no half-state behind.
- 5 tool input schemas now advertise origin as a required object via
  a shared origin_schema() helper. The schema description gives every
  caller (coder agent, chat bot, user, system) a concrete example so
  the LLM populates the field correctly on first sight.
- Test fixtures pass origin = {kind:"test", id:"test-suite"}.

Story 1104 (signed actions) is the longer-term replacement; this is the
quick attribution win agreed for master ahead of that design work.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Timmy
2026-05-15 23:13:54 +01:00
parent 26527e7dae
commit 6fbe239313
8 changed files with 170 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/http/mcp/story_tools/bug.rs
+11 -4
View File
@@ -26,6 +26,10 @@ pub(crate) fn tool_create_bug(args: &Value, ctx: &AppContext) -> Result<String,
let acs = req.acceptance_criteria_strings();
let depends_on = req.depends_on_ids();
// Bug 1102: resolve and validate origin BEFORE creating the bug file so a
// missing-attribution call leaves no half-state behind.
let origin = super::build_origin(args)?;
let root = ctx.state.get_project_root()?;
let bug_id = create_bug_file(
&root,
@@ -38,7 +42,7 @@ pub(crate) fn tool_create_bug(args: &Value, ctx: &AppContext) -> Result<String,
depends_on.as_deref(),
)?;
crate::crdt_state::set_origin(&bug_id, &super::build_origin(args));
crate::crdt_state::set_origin(&bug_id, &origin);
let _ = ctx
.watcher_tx
@@ -243,7 +247,8 @@ mod tests {
"steps_to_reproduce": "1. Open app\n2. Click login",
"actual_result": "500 error",
"expected_result": "Successful login",
"acceptance_criteria": ["Login succeeds without error"]
"acceptance_criteria": ["Login succeeds without error"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
)
@@ -364,7 +369,8 @@ mod tests {
"steps_to_reproduce": "s",
"actual_result": "a",
"expected_result": "e",
"acceptance_criteria": ["Bug is fixed"]
"acceptance_criteria": ["Bug is fixed"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
@@ -406,7 +412,8 @@ mod tests {
"steps_to_reproduce": "s",
"actual_result": "a",
"expected_result": "e",
"acceptance_criteria": ["TODO", "Real AC"]
"acceptance_criteria": ["TODO", "Real AC"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
server/src/http/mcp/story_tools/epic.rs
+12 -3
View File
@@ -13,6 +13,10 @@ use serde_json::{Value, json};
pub(crate) fn tool_create_epic(args: &Value, ctx: &AppContext) -> Result<String, String> {
let req = CreateEpicRequest::from_json(args)?;
// Bug 1102: resolve and validate origin BEFORE creating the epic so a
// missing-attribution call leaves no half-state behind.
let origin = super::build_origin(args)?;
let root = ctx.state.get_project_root()?;
let success_criteria = req.success_criteria_strings();
@@ -29,7 +33,7 @@ pub(crate) fn tool_create_epic(args: &Value, ctx: &AppContext) -> Result<String,
},
)?;
crate::crdt_state::set_origin(&epic_id, &super::build_origin(args));
crate::crdt_state::set_origin(&epic_id, &origin);
Ok(format!("Created epic: {epic_id}"))
}
@@ -170,7 +174,8 @@ mod tests {
"name": "My Test Epic",
"goal": "Achieve something great",
"motivation": "Because it matters",
"success_criteria": ["All stories done", "Tests pass"]
"success_criteria": ["All stories done", "Tests pass"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
@@ -223,7 +228,11 @@ mod tests {
// Create an epic.
tool_create_epic(
&json!({"name": "List Epics Test Epic", "goal": "Testing list"}),
&json!({
"name": "List Epics Test Epic",
"goal": "Testing list",
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
)
.unwrap();
server/src/http/mcp/story_tools/mod.rs
+38 -17
View File
@@ -14,29 +14,50 @@ mod story;
/// Build a compact origin JSON string for a newly-created work item (story 1088).
///
/// `args` may contain an `"origin"` object with `kind`, `id`, and `ts` fields
/// supplied by the caller (e.g. a coder agent passing its own identity). When
/// absent the default is `{"kind":"user","id":"","ts":<now>}`.
/// `args` must contain an `"origin"` object with a non-empty `id` field and an
/// optional `kind` (defaulting to `"user"`) and `ts` (defaulting to now). The
/// `id` MUST identify the calling actor — e.g. `coder-1@story=42` for a coder
/// agent, `chat-bot:Timmy@<room_id>` for a chat-bot session, or a human's user
/// id for a CLI/MCP-direct call. Empty / whitespace-only `id` is rejected so
/// that every work item carries a usable provenance trail (bug 1102 — we lost
/// 1102's attribution because the default was `id=""`).
///
/// Callers that create items on behalf of system automation (e.g. gate-failure
/// auto-filing) should pass `kind = "system"` and `id = "<automation-name>"`.
pub(super) fn build_origin(args: &serde_json::Value) -> String {
/// Returns the canonical origin JSON string on success. Returns `Err` with a
/// human-readable explanation when the caller failed to identify itself; the
/// caller (`tool_create_*` handlers) must propagate the error without creating
/// the work item, so a missing-attribution call leaves no half-state behind.
pub(super) fn build_origin(args: &serde_json::Value) -> Result<String, String> {
let ts = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.unwrap_or_default()
.as_secs_f64();
if let Some(origin_obj) = args.get("origin").and_then(|v| v.as_object()) {
let kind = origin_obj
.get("kind")
.and_then(|v| v.as_str())
.unwrap_or("user");
let id = origin_obj.get("id").and_then(|v| v.as_str()).unwrap_or("");
let ts_val = origin_obj.get("ts").and_then(|v| v.as_f64()).unwrap_or(ts);
serde_json::json!({"kind": kind, "id": id, "ts": ts_val}).to_string()
} else {
serde_json::json!({"kind": "user", "id": "", "ts": ts}).to_string()
}
let origin_obj = args.get("origin").and_then(|v| v.as_object()).ok_or(
"Missing required argument: 'origin'. Every create_* MCP call must \
identify the calling actor. Pass origin = { \"kind\": \"agent\" | \
\"chat-bot\" | \"user\" | \"system\", \"id\": \"<your-identifier>\", \
\"ts\": <unix-seconds, optional> }. Example: { \"kind\": \"agent\", \
\"id\": \"coder-1@story=42\" }.",
)?;
let id = origin_obj
.get("id")
.and_then(|v| v.as_str())
.map(str::trim)
.filter(|s| !s.is_empty())
.ok_or(
"origin.id must be a non-empty string identifying the calling \
actor (e.g. \"coder-1@story=42\", \"chat-bot:Timmy@!room:home\", \
or a human user id). See bug 1102 / story 1104 for the rationale.",
)?;
let kind = origin_obj
.get("kind")
.and_then(|v| v.as_str())
.unwrap_or("user");
let ts_val = origin_obj.get("ts").and_then(|v| v.as_f64()).unwrap_or(ts);
Ok(serde_json::json!({"kind": kind, "id": id, "ts": ts_val}).to_string())
}
pub(crate) use bug::{tool_close_bug, tool_create_bug, tool_list_bugs};
server/src/http/mcp/story_tools/refactor.rs
+15 -3
View File
@@ -27,6 +27,10 @@ pub(crate) fn tool_create_refactor(args: &Value, ctx: &AppContext) -> Result<Str
let description = req.description.as_ref().map(|d| d.as_str());
let depends_on = req.depends_on_ids();
// Bug 1102: resolve and validate origin BEFORE creating the refactor file
// so a missing-attribution call leaves no half-state behind.
let origin = super::build_origin(args)?;
let root = ctx.state.get_project_root()?;
let refactor_id = create_refactor_file(
&root,
@@ -36,7 +40,7 @@ pub(crate) fn tool_create_refactor(args: &Value, ctx: &AppContext) -> Result<Str
depends_on.as_deref(),
)?;
crate::crdt_state::set_origin(&refactor_id, &super::build_origin(args));
crate::crdt_state::set_origin(&refactor_id, &origin);
let _ = ctx
.watcher_tx
@@ -114,7 +118,11 @@ mod tests {
let tmp = tempfile::tempdir().unwrap();
let ctx = test_ctx(tmp.path());
let result = tool_create_refactor(
&json!({"name": "Single Criterion Refactor", "acceptance_criteria": ["Code is clean"]}),
&json!({
"name": "Single Criterion Refactor",
"acceptance_criteria": ["Code is clean"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
assert!(result.is_ok(), "expected ok: {result:?}");
@@ -141,7 +149,11 @@ mod tests {
let tmp = tempfile::tempdir().unwrap();
let ctx = test_ctx(tmp.path());
let result = tool_create_refactor(
&json!({"name": "Mixed Refactor", "acceptance_criteria": ["TODO", "Real AC"]}),
&json!({
"name": "Mixed Refactor",
"acceptance_criteria": ["TODO", "Real AC"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
assert!(result.is_ok(), "expected ok for mixed AC: {result:?}");
server/src/http/mcp/story_tools/spike.rs
+30 -7
View File
@@ -27,6 +27,10 @@ pub(crate) fn tool_create_spike(args: &Value, ctx: &AppContext) -> Result<String
let description = req.description.as_ref().map(|d| d.as_str());
let depends_on = req.depends_on_ids();
// Bug 1102: resolve and validate origin BEFORE creating the spike file so
// a missing-attribution call leaves no half-state behind.
let origin = super::build_origin(args)?;
let root = ctx.state.get_project_root()?;
let spike_id = create_spike_file(
&root,
@@ -36,7 +40,7 @@ pub(crate) fn tool_create_spike(args: &Value, ctx: &AppContext) -> Result<String
depends_on.as_deref(),
)?;
crate::crdt_state::set_origin(&spike_id, &super::build_origin(args));
crate::crdt_state::set_origin(&spike_id, &origin);
let _ = ctx
.watcher_tx
@@ -85,8 +89,14 @@ mod tests {
fn tool_create_spike_rejects_empty_name() {
let tmp = tempfile::tempdir().unwrap();
let ctx = test_ctx(tmp.path());
let result =
tool_create_spike(&json!({"name": "!!!", "acceptance_criteria": ["AC"]}), &ctx);
let result = tool_create_spike(
&json!({
"name": "!!!",
"acceptance_criteria": ["AC"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
assert!(result.is_err());
assert!(result.unwrap_err().contains("alphanumeric"));
}
@@ -115,7 +125,8 @@ mod tests {
&json!({
"name": "Compare Encoders",
"description": "Which encoder is fastest?",
"acceptance_criteria": ["Encoder comparison is documented"]
"acceptance_criteria": ["Encoder comparison is documented"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
)
@@ -140,7 +151,11 @@ mod tests {
let ctx = test_ctx(tmp.path());
let result = tool_create_spike(
&json!({"name": "My Spike", "acceptance_criteria": ["Spike findings documented"]}),
&json!({
"name": "My Spike",
"acceptance_criteria": ["Spike findings documented"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
)
.unwrap();
@@ -190,7 +205,11 @@ mod tests {
let tmp = tempfile::tempdir().unwrap();
let ctx = test_ctx(tmp.path());
let result = tool_create_spike(
&json!({"name": "Single Criterion Spike", "acceptance_criteria": ["Findings documented"]}),
&json!({
"name": "Single Criterion Spike",
"acceptance_criteria": ["Findings documented"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
assert!(result.is_ok(), "expected ok: {result:?}");
@@ -217,7 +236,11 @@ mod tests {
let tmp = tempfile::tempdir().unwrap();
let ctx = test_ctx(tmp.path());
let result = tool_create_spike(
&json!({"name": "Mixed Spike", "acceptance_criteria": ["TODO", "Real AC"]}),
&json!({
"name": "Mixed Spike",
"acceptance_criteria": ["TODO", "Real AC"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
assert!(result.is_ok(), "expected ok for mixed AC: {result:?}");
server/src/http/mcp/story_tools/story/create.rs
+19 -5
View File
@@ -15,6 +15,10 @@ use serde_json::Value;
pub(crate) fn tool_create_story(args: &Value, ctx: &AppContext) -> Result<String, String> {
let req = CreateStoryRequest::from_json(args)?;
// Bug 1102: resolve and validate origin BEFORE creating the story file so
// a missing-attribution call leaves no half-state behind.
let origin = super::super::build_origin(args)?;
let root = ctx.state.get_project_root()?;
let depends_on_ids = req.depends_on_ids();
@@ -31,7 +35,7 @@ pub(crate) fn tool_create_story(args: &Value, ctx: &AppContext) -> Result<String
false,
)?;
crate::crdt_state::set_origin(&story_id, &super::super::build_origin(args));
crate::crdt_state::set_origin(&story_id, &origin);
let _ = ctx
.watcher_tx
@@ -255,7 +259,11 @@ mod tests {
let tmp = tempfile::tempdir().unwrap();
let ctx = test_ctx(tmp.path());
let result = tool_create_story(
&json!({"name": "Single Criterion Story", "acceptance_criteria": ["It works"]}),
&json!({
"name": "Single Criterion Story",
"acceptance_criteria": ["It works"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
assert!(result.is_ok(), "expected ok: {result:?}");
@@ -278,7 +286,11 @@ mod tests {
let tmp = tempfile::tempdir().unwrap();
let ctx = test_ctx(tmp.path());
let result = tool_create_story(
&json!({"name": "Mixed Story", "acceptance_criteria": ["TODO", "Real AC"]}),
&json!({
"name": "Mixed Story",
"acceptance_criteria": ["TODO", "Real AC"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
assert!(result.is_ok(), "expected ok for mixed AC: {result:?}");
@@ -294,7 +306,8 @@ mod tests {
&json!({
"name": "Story With Description",
"description": "This is the background context.",
"acceptance_criteria": ["Described well"]
"acceptance_criteria": ["Described well"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
)
@@ -361,7 +374,8 @@ mod tests {
let result = tool_create_story(
&json!({
"name": "Story with <b>bold</b> name",
"acceptance_criteria": ["AC1"]
"acceptance_criteria": ["AC1"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
);
server/src/http/mcp/story_tools/story/query.rs
+5 -1
View File
@@ -130,7 +130,11 @@ mod tests {
let ctx = test_ctx(tmp.path());
let result = super::super::tool_create_story(
&json!({"name": "Test Story", "acceptance_criteria": ["AC1", "AC2"]}),
&json!({
"name": "Test Story",
"acceptance_criteria": ["AC1", "AC2"],
"origin": {"kind": "test", "id": "test-suite"}
}),
&ctx,
)
.unwrap();
server/src/http/mcp/tools_list/story_tools.rs
+40 -10
View File
@@ -2,6 +2,31 @@
use serde_json::{Value, json};
/// JSON schema fragment for the `origin` argument required by every `create_*`
/// tool (bug 1102). The caller MUST identify itself — empty `id` is rejected
/// server-side so every work item carries a usable provenance trail.
fn origin_schema() -> Value {
json!({
"type": "object",
"description": "Required: identifies the calling actor so every work item carries provenance. Empty/missing id is rejected (bug 1102). Examples: { \"kind\": \"agent\", \"id\": \"coder-1@story=42\" }, { \"kind\": \"chat-bot\", \"id\": \"Timmy@!room:home.local\" }, { \"kind\": \"user\", \"id\": \"dave\" }.",
"properties": {
"kind": {
"type": "string",
"description": "One of: \"agent\" (LLM coder/mergemaster/qa), \"chat-bot\" (Timmy or other chat-routed bot), \"user\" (human via CLI/MCP), \"system\" (server-automation)."
},
"id": {
"type": "string",
"description": "Non-empty identifier of the caller. For agents include the story id (e.g. \"coder-1@story=42\"); for chat-bots include the room/session (e.g. \"Timmy@!room:home.local\"); for users the user id or short name."
},
"ts": {
"type": "number",
"description": "Optional unix-seconds timestamp. Defaults to the server's clock when absent."
}
},
"required": ["kind", "id"]
})
}
/// Returns tool schemas for story/work-item lifecycle management.
pub(super) fn story_tools() -> Vec<Value> {
vec![
@@ -37,9 +62,10 @@ pub(super) fn story_tools() -> Vec<Value> {
"commit": {
"type": "boolean",
"description": "If true, git-add and git-commit the new story file to the current branch"
}
},
"origin": origin_schema()
},
"required": ["name", "acceptance_criteria"]
"required": ["name", "acceptance_criteria", "origin"]
}
}),
json!({
@@ -282,9 +308,10 @@ pub(super) fn story_tools() -> Vec<Value> {
"items": { "type": "string" },
"minItems": 1,
"description": "List of acceptance criteria (at least one required)"
}
},
"origin": origin_schema()
},
"required": ["name", "acceptance_criteria"]
"required": ["name", "acceptance_criteria", "origin"]
}
}),
json!({
@@ -323,9 +350,10 @@ pub(super) fn story_tools() -> Vec<Value> {
"type": "array",
"items": { "type": "integer" },
"description": "Optional list of story numbers this bug depends on (e.g. [42, 43]). Persisted as depends_on in YAML front matter."
}
},
"origin": origin_schema()
},
"required": ["name", "description", "steps_to_reproduce", "actual_result", "expected_result", "acceptance_criteria"]
"required": ["name", "description", "steps_to_reproduce", "actual_result", "expected_result", "acceptance_criteria", "origin"]
}
}),
json!({
@@ -360,9 +388,10 @@ pub(super) fn story_tools() -> Vec<Value> {
"type": "array",
"items": { "type": "integer" },
"description": "Optional list of story numbers this refactor depends on (e.g. [42, 43]). Persisted as depends_on in YAML front matter."
}
},
"origin": origin_schema()
},
"required": ["name", "acceptance_criteria"]
"required": ["name", "acceptance_criteria", "origin"]
}
}),
json!({
@@ -399,9 +428,10 @@ pub(super) fn story_tools() -> Vec<Value> {
"type": "array",
"items": { "type": "string" },
"description": "Optional: list of high-level success criteria for the epic"
}
},
"origin": origin_schema()
},
"required": ["name", "goal"]
"required": ["name", "goal", "origin"]
}
}),
json!({