Skip to content

Commit e180dff

Browse files
fix(mcp): return tool execution failures as isError results (#258)
## What type of PR is this? - [x] fix (bug fix) ## Which issue(s) this PR fixes Fixes #254 ## What this PR does / why we need it Tool execution failures currently become JSON-RPC errors or unmarked success text, making it difficult for clients to present actionable failures to the model. Return execution failures as tool results with `isError: true`, while keeping malformed tool-call envelopes and unknown tools as JSON-RPC `-32602` errors. - Share protocol validation and error-result construction across stdio, embedded HTTP, and remote execution. Preserve existing known hidden tool names. - Mark existing validation/rejection text as tool errors, preserve actionable validation and conflict messages, and redact typed internal/database/embedding and transport failure details from model-visible results. - Validate unknown tools before HTTP scope classification; known tools still require their existing scopes, and group write guards remain in place. - Keep `rpc_success` as the protocol outcome. Add nullable `tool_success` and `tool_error_kind` call-log columns with additive migrations and expose per-tool input/rejection/backend failure counts through admin statistics. Historical rows retain an unknown tool outcome. Expected input/rejection errors do not generate backend warning/health-error signals. - Preserve `arguments: null` compatibility, plain-text REST validation/rejection messages, and failure markers for duplicate snapshots/branches, missing branch deletion, and empty apply selections. REST delegation propagates marked tool failures as HTTP errors. - Limit partial-write warnings to tools that may mutate state; read-only failures do not imply a write. MCP remains at `2024-11-05`; this does not depend on #236 or the notification fix. ## Review and validation - Reviewed both dispatchers, remote response parsing, existing soft-error branches, HTTP admission, and the distinction between protocol and execution success. - `cargo test -p memoria-mcp -p memoria-api --lib --offline`: 138 tests passed (104 API, 34 MCP). - `cargo test -p memoria-mcp --test tools_unit --offline`: 9 tests passed. - `cargo clippy -p memoria-mcp -p memoria-api --lib --offline -- -D warnings`: passed. - `cargo test -p memoria-api --test api_e2e --no-run --offline`: passed; updated the unknown-tool assertions to expect `-32602`. - Branch and snapshot end-to-end test targets compile successfully (`--no-run --offline`). - Added regression coverage for null arguments, remote plain-text/JSON errors and HTML redaction, read-only error messaging, REST error propagation, and persisted per-tool error classification. - `git diff --check`: passed. - Database-backed end-to-end tests were not run locally because Docker is unavailable. Those tests remain for CI; local tests use lazy, unconnected pools and a local HTTP stub/transport failure to exercise protocol behavior. ## Follow-up CI fixes - Update the snapshot quota integration test to expect the REST error response, and verify that rejection does not create an extra snapshot. - Preserve legacy input-error text while carrying a typed input classification. Add database-independent regression coverage for the missing session-id message and its tool-error classification; the existing database integration assertions remain unchanged. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 9101313 commit e180dff

15 files changed

Lines changed: 1741 additions & 259 deletions

File tree

‎memoria/crates/memoria-api/src/auth.rs‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -898,19 +898,25 @@ pub struct RpcMeta {
898898
pub success: bool,
899899
/// JSON-RPC error code (e.g. -32601) when success = false; None otherwise.
900900
pub error_code: Option<i32>,
901+
pub tool_success: Option<bool>,
902+
pub tool_error_kind: Option<&'static str>,
901903
}
902904

903905
impl RpcMeta {
904906
pub fn ok() -> Self {
905907
Self {
906908
success: true,
907909
error_code: None,
910+
tool_success: None,
911+
tool_error_kind: None,
908912
}
909913
}
910914
pub fn err(code: i32) -> Self {
911915
Self {
912916
success: false,
913917
error_code: Some(code),
918+
tool_success: None,
919+
tool_error_kind: None,
914920
}
915921
}
916922
}
@@ -927,6 +933,8 @@ struct CallLogEntry {
927933
rpc_success: bool,
928934
/// JSON-RPC error code (e.g. -32601) when rpc_success = false; NULL otherwise.
929935
rpc_error_code: Option<i32>,
936+
tool_success: Option<bool>,
937+
tool_error_kind: Option<&'static str>,
930938
}
931939

932940
/// Accumulates call log entries in memory and flushes them in batches to DB.
@@ -997,6 +1005,8 @@ impl CallLogBatcher {
9971005
latency_ms,
9981006
rpc_success: rpc.success,
9991007
rpc_error_code: rpc.error_code,
1008+
tool_success: rpc.tool_success,
1009+
tool_error_kind: rpc.tool_error_kind,
10001010
});
10011011
}
10021012
}
@@ -1065,12 +1075,12 @@ async fn flush_call_log_chunked(
10651075
for chunk in entries.chunks(200) {
10661076
let placeholders: String = chunk
10671077
.iter()
1068-
.map(|_| "(?, ?, ?, ?, ?, ?, ?)")
1078+
.map(|_| "(?, ?, ?, ?, ?, ?, ?, ?, ?)")
10691079
.collect::<Vec<_>>()
10701080
.join(",");
10711081
let sql = format!(
10721082
"INSERT INTO {table} \
1073-
(user_id, method, path, status_code, latency_ms, rpc_success, rpc_error_code) \
1083+
(user_id, method, path, status_code, latency_ms, rpc_success, rpc_error_code, tool_success, tool_error_kind) \
10741084
VALUES {placeholders}"
10751085
);
10761086
let mut query = sqlx::query(&sql);
@@ -1082,7 +1092,9 @@ async fn flush_call_log_chunked(
10821092
.bind(e.status_code as i16)
10831093
.bind(e.latency_ms as i32)
10841094
.bind(e.rpc_success as i8)
1085-
.bind(e.rpc_error_code);
1095+
.bind(e.rpc_error_code)
1096+
.bind(e.tool_success.map(i8::from))
1097+
.bind(e.tool_error_kind);
10861098
}
10871099
query.execute(pool).await?;
10881100
}

‎memoria/crates/memoria-api/src/routes/admin.rs‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -594,12 +594,12 @@ pub async fn user_call_stats(
594594
// Aggregate totals for the requested time window.
595595
// Error counting unifies HTTP errors (/v1/*) and JSON-RPC errors (/mcp/*):
596596
// - /v1/* REST calls: HTTP status_code >= 400 signals an error
597-
// - /mcp/* JSON-RPC calls: HTTP is always 200; rpc_success = 0 signals an error
597+
// - /mcp/*: protocol and tool execution errors are independent dimensions
598598
let row = sqlx::query(&format!(
599599
"SELECT \
600600
CAST(COUNT(*) AS SIGNED) AS total, \
601601
CAST(COALESCE(AVG(latency_ms), 0) AS DOUBLE) AS avg_ms, \
602-
CAST(SUM(CASE WHEN status_code >= 400 OR rpc_success = 0 THEN 1 ELSE 0 END) AS SIGNED) AS errors \
602+
CAST(SUM(CASE WHEN status_code >= 400 OR rpc_success = 0 OR tool_success = 0 THEN 1 ELSE 0 END) AS SIGNED) AS errors \
603603
FROM {call_log} \
604604
WHERE user_id = ? AND called_at >= DATE_SUB(NOW(6), INTERVAL ? DAY)",
605605
))
@@ -624,7 +624,11 @@ pub async fn user_call_stats(
624624
CAST(COALESCE(AVG(latency_ms), 0) AS DOUBLE) AS avg_ms, \
625625
CAST(COALESCE(MAX(latency_ms), 0) AS SIGNED) AS max_ms, \
626626
CAST(SUM(CASE WHEN status_code >= 400 THEN 1 ELSE 0 END) AS SIGNED) AS err_cnt, \
627-
CAST(SUM(CASE WHEN rpc_success = 0 THEN 1 ELSE 0 END) AS SIGNED) AS rpc_err_cnt \
627+
CAST(SUM(CASE WHEN rpc_success = 0 THEN 1 ELSE 0 END) AS SIGNED) AS rpc_err_cnt, \
628+
CAST(SUM(CASE WHEN tool_success = 0 THEN 1 ELSE 0 END) AS SIGNED) AS tool_err_cnt, \
629+
CAST(SUM(CASE WHEN tool_error_kind = 'input' THEN 1 ELSE 0 END) AS SIGNED) AS tool_input_cnt, \
630+
CAST(SUM(CASE WHEN tool_error_kind = 'rejected' THEN 1 ELSE 0 END) AS SIGNED) AS tool_rejected_cnt, \
631+
CAST(SUM(CASE WHEN tool_error_kind = 'backend' THEN 1 ELSE 0 END) AS SIGNED) AS tool_backend_cnt \
628632
FROM {call_log} \
629633
WHERE user_id = ? AND called_at >= DATE_SUB(NOW(6), INTERVAL ? DAY) \
630634
GROUP BY method, path \
@@ -647,8 +651,9 @@ pub async fn user_call_stats(
647651
let max_ms: i64 = r.try_get("max_ms").unwrap_or(0);
648652
let err_cnt: i64 = r.try_get("err_cnt").unwrap_or(0);
649653
let rpc_err_cnt: i64 = r.try_get("rpc_err_cnt").unwrap_or(0);
650-
// Unified error count: HTTP errors for /v1/* + RPC errors for /mcp/*
651-
let total_err = err_cnt + rpc_err_cnt;
654+
// HTTP, protocol, and execution failures remain independently queryable.
655+
let tool_err_cnt: i64 = r.try_get("tool_err_cnt").unwrap_or(0);
656+
let total_err = err_cnt + rpc_err_cnt + tool_err_cnt;
652657
serde_json::json!({
653658
"method": method,
654659
"path": path,
@@ -657,6 +662,10 @@ pub async fn user_call_stats(
657662
"max_ms": max_ms,
658663
"error_count": total_err,
659664
"rpc_error_count": rpc_err_cnt,
665+
"tool_error_count": tool_err_cnt,
666+
"tool_input_error_count": r.try_get::<i64, _>("tool_input_cnt").unwrap_or(0),
667+
"tool_rejected_count": r.try_get::<i64, _>("tool_rejected_cnt").unwrap_or(0),
668+
"tool_backend_error_count": r.try_get::<i64, _>("tool_backend_cnt").unwrap_or(0),
660669
"error_rate": if cnt > 0 {
661670
(total_err as f64 / cnt as f64 * 100.0).round() / 100.0
662671
} else { 0.0 },
@@ -665,9 +674,9 @@ pub async fn user_call_stats(
665674
.collect();
666675

667676
// Most recent 50 calls for the live "Recent Calls" feed.
668-
// Include rpc_success so /mcp errors (HTTP 200 but RPC failure) show as "err".
677+
// Include both protocol and tool outcomes for MCP responses with HTTP 200.
669678
let recent_rows = sqlx::query(&format!(
670-
"SELECT method, path, status_code, latency_ms, called_at, rpc_success \
679+
"SELECT method, path, status_code, latency_ms, called_at, rpc_success, tool_success, tool_error_kind \
671680
FROM {call_log} \
672681
WHERE user_id = ? \
673682
ORDER BY called_at DESC \
@@ -690,7 +699,9 @@ pub async fn user_call_stats(
690699
.unwrap_or_else(|_| chrono::Utc::now());
691700
// rpc_success defaults to true (1) for /v1/* rows that predate the column.
692701
let rpc_success: i8 = r.try_get("rpc_success").unwrap_or(1);
693-
let is_err = status_code >= 400 || rpc_success == 0;
702+
let tool_success: Option<i8> = r.try_get("tool_success").unwrap_or(None);
703+
let tool_error_kind: Option<String> = r.try_get("tool_error_kind").unwrap_or(None);
704+
let is_err = status_code >= 400 || rpc_success == 0 || tool_success == Some(0);
694705
serde_json::json!({
695706
"method": method,
696707
"path": path,
@@ -699,6 +710,8 @@ pub async fn user_call_stats(
699710
"called_at": called_at.to_rfc3339(),
700711
// Unified status: HTTP error (/v1/*) OR JSON-RPC error (/mcp/*)
701712
"status": if is_err { "err" } else { "ok" },
713+
"tool_success": tool_success.map(|v| v != 0),
714+
"tool_error_kind": tool_error_kind,
702715
})
703716
})
704717
.collect();

‎memoria/crates/memoria-api/src/routes/mcp.rs‎

Lines changed: 138 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,26 @@ fn spawn_metrics_dirty_mark(
189189
});
190190
}
191191

192+
fn result_outcome(method: &str, result: &serde_json::Value) -> RpcMeta {
193+
let mut rpc = RpcMeta::ok();
194+
if method == "tools/call" {
195+
let kind = memoria_mcp::tool_result::error_kind(result);
196+
rpc.tool_success = Some(kind.is_none());
197+
rpc.tool_error_kind = kind.map(|kind| kind.as_str());
198+
}
199+
rpc
200+
}
201+
202+
fn report_tool_outcome(tool: Option<&str>, rpc: &RpcMeta) {
203+
if let Some(kind) = rpc.tool_error_kind {
204+
if kind == "backend" {
205+
tracing::warn!(tool, kind, "MCP tool backend failure");
206+
} else {
207+
tracing::debug!(tool, kind, "MCP tool input or operation rejected");
208+
}
209+
}
210+
}
211+
192212
pub async fn mcp_handler(
193213
State(state): State<AppState>,
194214
auth: AuthUser,
@@ -317,6 +337,17 @@ pub async fn mcp_handler(
317337
}
318338
};
319339
// Use the exact dispatch name, never a sanitized/truncated metrics label.
340+
if method == "tools/call" {
341+
if let Err(error) = memoria_mcp::validate_tool_call(params.as_ref()) {
342+
report_stats(&track_path, false);
343+
if req.get("id").is_none() {
344+
return StatusCode::NO_CONTENT.into_response();
345+
}
346+
return Json(json!({"jsonrpc": "2.0", "id": req["id"],
347+
"error": {"code": error.code, "message": error.message}}))
348+
.into_response();
349+
}
350+
}
320351
let authorization_error = if method == "tools/call" {
321352
let name = params
322353
.as_ref()
@@ -455,17 +486,24 @@ pub async fn mcp_handler(
455486
)
456487
.await;
457488
let rpc = match &dispatch_result {
458-
Ok(_) => RpcMeta::ok(),
489+
Ok(result) => result_outcome(&method, result),
459490
Err(e) => RpcMeta::err(e.code),
460491
};
461-
if dispatch_result.is_ok() {
492+
let tool_success = dispatch_result
493+
.as_ref()
494+
.is_ok_and(|result| !memoria_mcp::tool_result::is_error(result));
495+
if tool_success {
462496
if let Some(mask) = tracked_tool.as_deref().and_then(mcp_tool_dirty_mask) {
463497
spawn_metrics_dirty_mark(state.clone(), scope_id.clone(), mask);
464498
}
465499
}
466-
// Report accurate ops metrics using the real RPC path and success flag
467-
// (JSON-RPC errors still return HTTP 200, so is_success must come from rpc.success).
468-
report_stats(&track_path, rpc.success);
500+
// Service health excludes expected input/rejection errors; call logs
501+
// retain both protocol and tool outcomes.
502+
report_tool_outcome(tracked_tool.as_deref(), &rpc);
503+
report_stats(
504+
&track_path,
505+
rpc.success && rpc.tool_error_kind != Some("backend"),
506+
);
469507
record_call(204, rpc);
470508
return StatusCode::NO_CONTENT.into_response();
471509
}
@@ -495,9 +533,9 @@ pub async fn mcp_handler(
495533
return err_body.into_response();
496534
}
497535

498-
// JSON-RPC spec: the HTTP response is always 200 OK, even for RPC errors.
499-
// Business-level error tracking uses rpc_success / rpc_error_code in the call log.
500-
let (response, rpc) = match memoria_mcp::dispatch_http(
536+
// This endpoint returns HTTP 200 for dispatched JSON-RPC responses. Protocol
537+
// and tool execution outcomes must be tracked separately.
538+
let (response, rpc, tool_success) = match memoria_mcp::dispatch_http(
501539
method.clone(),
502540
params,
503541
state.service.clone(),
@@ -507,10 +545,13 @@ pub async fn mcp_handler(
507545
.await
508546
{
509547
Ok(v) => {
548+
let tool_success = !memoria_mcp::tool_result::is_error(&v);
549+
let rpc = result_outcome(&method, &v);
510550
let result = if v.is_null() { json!({}) } else { v };
511551
(
512552
Json(json!({"jsonrpc": "2.0", "id": id, "result": result})).into_response(),
513-
RpcMeta::ok(),
553+
rpc,
554+
tool_success,
514555
)
515556
}
516557
Err(e) => (
@@ -521,18 +562,23 @@ pub async fn mcp_handler(
521562
}))
522563
.into_response(),
523564
RpcMeta::err(e.code),
565+
false,
524566
),
525567
};
526568

527-
if rpc.success {
569+
if tool_success {
528570
if let Some(mask) = tracked_tool.as_deref().and_then(mcp_tool_dirty_mask) {
529571
spawn_metrics_dirty_mark(state.clone(), scope_id.clone(), mask);
530572
}
531573
}
532574

533-
// Report accurate ops metrics using the real RPC path and success flag
534-
// (JSON-RPC errors still return HTTP 200, so is_success must come from rpc.success).
535-
report_stats(&track_path, rpc.success);
575+
// Service health excludes input/rejection errors; the per-tool call log
576+
// retains every execution failure and its classification separately.
577+
report_tool_outcome(tracked_tool.as_deref(), &rpc);
578+
report_stats(
579+
&track_path,
580+
rpc.success && rpc.tool_error_kind != Some("backend"),
581+
);
536582
record_call(200, rpc);
537583

538584
response
@@ -544,6 +590,26 @@ mod tests {
544590
use crate::auth::{SCOPE_MEMORY_READ, SCOPE_MEMORY_WRITE};
545591
use serde_json::json;
546592

593+
#[test]
594+
fn protocol_and_tool_outcomes_remain_separate() {
595+
use memoria_mcp::tool_result::{classified_error, ErrorKind};
596+
for kind in [ErrorKind::Input, ErrorKind::Rejected, ErrorKind::Backend] {
597+
let rpc = super::result_outcome("tools/call", &classified_error(kind, "failure"));
598+
assert!(rpc.success);
599+
assert_eq!(rpc.error_code, None);
600+
assert_eq!(rpc.tool_success, Some(false));
601+
assert_eq!(rpc.tool_error_kind, Some(kind.as_str()));
602+
}
603+
assert_eq!(
604+
super::result_outcome("tools/call", &json!({"content":[]})).tool_success,
605+
Some(true)
606+
);
607+
assert_eq!(
608+
super::result_outcome("tools/list", &json!({"tools":[]})).tool_success,
609+
None
610+
);
611+
}
612+
547613
fn test_state() -> crate::state::AppState {
548614
let pool = sqlx::mysql::MySqlPoolOptions::new()
549615
.connect_lazy("mysql://test:test@127.0.0.1/test")
@@ -664,6 +730,65 @@ mod tests {
664730
);
665731
}
666732

733+
#[tokio::test]
734+
async fn http_tool_errors_preserve_protocol_success_and_scope_denials() {
735+
use axum::response::IntoResponse;
736+
let state = test_state();
737+
for (params, scopes, expected) in [
738+
(
739+
json!({"name":"memory_store","arguments":{"content":""}}),
740+
vec![crate::auth::SCOPE_MEMORY_WRITE.into()],
741+
None,
742+
),
743+
(
744+
json!({"name":"memory_future_tool"}),
745+
vec![crate::auth::SCOPE_MEMORY_WRITE.into()],
746+
Some(-32602),
747+
),
748+
(
749+
json!({"name":"memory_store","arguments":[]}),
750+
vec![crate::auth::SCOPE_MEMORY_WRITE.into()],
751+
Some(-32602),
752+
),
753+
(
754+
json!({"name":"memory_store","arguments":{"content":"denied"}}),
755+
vec![],
756+
Some(-32003),
757+
),
758+
] {
759+
let response = super::mcp_handler(
760+
axum::extract::State(state.clone()),
761+
test_auth(scopes),
762+
Default::default(),
763+
json!({"jsonrpc":"2.0","id":"request","method":"tools/call","params":params})
764+
.to_string(),
765+
)
766+
.await
767+
.into_response();
768+
assert_eq!(response.status(), 200);
769+
let body: serde_json::Value = serde_json::from_slice(
770+
&axum::body::to_bytes(response.into_body(), 4096)
771+
.await
772+
.unwrap(),
773+
)
774+
.unwrap();
775+
assert_eq!(body["id"], "request");
776+
if let Some(code) = expected {
777+
assert_eq!(body["error"]["code"], code, "{body}");
778+
assert!(body.get("result").is_none());
779+
} else {
780+
assert_eq!(body["result"]["isError"], true, "{body}");
781+
assert!(body.get("error").is_none());
782+
}
783+
}
784+
// Rejected protocol envelopes and scope denials do not provision per-user
785+
// storage. The execution error is logged as a successful RPC exchange.
786+
assert_eq!(
787+
state.call_log_batcher.pending_rpc_outcomes(),
788+
vec![(true, None)]
789+
);
790+
}
791+
667792
// ── tools/call — happy path ───────────────────────────────────────────────
668793

669794
#[test]

0 commit comments

Comments
 (0)