fix(mcp): return tool execution failures as isError results

[loveRhythm1990]

What type of PR is this?

• 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.

[aptend]

Requesting changes for two error-classification issues:

1. memoria-mcp/src/tools.rs:820-824 and 930-934 stringify MemoriaError/sqlx::Error into new anyhow messages. This erases the source type that tool_result::execution_error relies on for redaction, so database details can be returned in isError content. Please preserve the underlying error chain, for example with anyhow::Error::from or context that retains the source.

2. memoria-mcp/src/tools.rs:427-431 similarly erases MemoriaError::InvalidTrustTier. An invalid trust_tier is consequently classified as a backend failure, gets a misleading partial-write warning, and pollutes backend health metrics even though execution has not begun. Please preserve the typed input error.

The relevant unit tests pass, but these paths are not covered by the current classification/redaction tests.

[aptend]

Deep re-review at exact head 99280745e43307963a99ff0dcb8f48976e3d4dc0 (base/merge-base 99629991e8872b187e1b5831e997311d55cb64d4): REQUEST_CHANGES.

The previous typed-error-chain findings are closed: InvalidTrustTier, MemoriaError, and sqlx::Error now retain their source types; classification/redaction walks the chain; and focused coverage confirms the behavior. One separate remote-dispatch blocker remains below.

Evidence on this head:

• cargo test -p memoria-mcp -p memoria-api --lib --offline: 145 passed.
• cargo test -p memoria-mcp --test tools_unit --offline: 9 passed.
• A temporary local-HTTP counterexample reproduced the remote branch-delete false success; the test was removed afterward.
• git diff --check passed; exact-head Check & Clippy, Unit Tests, and DB Tests are green.

[aptend]

Deep re-review at exact head 976a336a8e6da8770597e4d5b7b3470f389d074f (base/merge-base 99629991e8872b187e1b5831e997311d55cb64d4): REQUEST_CHANGES.

The previous 409 branch-delete and 5xx rebuild false-success paths are now routed through parse_response, and their focused regressions pass. The latest change introduces one separate fail-open case: empty 2xx bodies are now accepted globally, including for tools whose successful contract requires JSON. The inline comment has the concrete counterexample and impact.

I re-read the complete review/comment/thread history, checked the incremental diff from 99280745e43307963a99ff0dcb8f48976e3d4dc0, and revalidated the full PR diff. Local exact-head evidence: all 41 memoria-mcp library tests pass; a temporary local-HTTP regression test reproduces the empty-body false success and was removed afterward; git diff --check passes. Exact-head Check & Clippy and Unit Tests are green; DB Tests were still running at submission time.

[aptend]

Deep re-review of exact head 8f442c8 against base/merge-base 9962999: REQUEST_CHANGES. I read all earlier reviews/inline threads and the delta from my previous head 976a336. The prior empty-204 bug is fixed for memory_store; branch delete and rebuild now map HTTP failures correctly. Two incomplete-success paths remain in the new per-tool validation:

1. [P2] memoria/crates/memoria-mcp/src/remote.rs:865-872: normal (non-dry-run) memory_pick is documented by the API route at routes/snapshots.rs:983-997 to return {"result":"..."}, but any nonempty object lacking result/dry_run is accepted as success. Reproducer: a local POST /v1/branches/topic/pick stub returns HTTP 200 {"unexpected":1}; RemoteClient::call("memory_pick", {"source":"topic","target":"main","selector":{"type":"key_list","keys":["m1"]}}) returns non-error MCP JSON instead of a backend error. A malformed/truncated acknowledgement can therefore be reported as a successful branch mutation even though the server did not confirm that anything was picked. Require the normal result string, or a validated dry-run schema; add this negative case.

2. [P2] memoria/crates/memoria-mcp/src/remote.rs:676-713: memory_reflect checks only whether the candidates/scenes_created key exists, then defaults invalid values to zero. Reproducer: HTTP 200 {"candidates":null} for mode=candidates returns a non-error 'Reflection complete: scenes_created=0, candidates_found=0'. The API's actual successful candidates response uses an array (routes/governance.rs:196-215), so this masks an incomplete response as completed work. Validate the value type and the matching success variant before returning success; add a null/wrong-type negative test.

Both counterexamples were reproduced with temporary local HTTP tests on this head and then removed. Current 42 MCP library tests, 9 tools_unit tests, exact-head CI checks and git diff --check pass; none covers these malformed-success shapes.

[aptend]

Reviewed exact head 9463ac91ad694a7b3397c3f7e26f489ceb53be6f, all prior review threads, the delta since 8f442c8, and the full PR diff. The previous pick/reflect incomplete-response issues are fixed, but two remote-mode execution acknowledgements still need correction:

1. memoria/crates/memoria-mcp/src/remote.rs:874-883 — dry-run pick can report a real mutation as a successful preview request. The branch is selected from the response dry_run flag, not from the caller's requested args.dry_run. A remote API/proxy returning HTTP 200 {"result":"Picked 1 memory"} for memory_pick with {"dry_run":{}} is accepted as success. This is especially dangerous with a mixed-version backend that ignores the preview option: a caller explicitly asking only to preview may have changed the target branch, with no error or warning. I reproduced Ok("Picked 1 memory") on this exact head using a local HTTP stub. Require a valid preview envelope whenever dry_run was requested; an execution acknowledgement must be treated as a backend contract failure (and warn that mutation may have occurred).

2. memoria/crates/memoria-mcp/src/remote.rs:923-924 — selective apply accepts an unconfirmed response as success. /v1/branches/:source/apply returns a serialized ApplyResult with applied_adds, skipped_adds, etc., but this branch accepts any valid JSON. With HTTP 200 {} (truncated/mismatched gateway response), memory_apply returns a normal MCP result containing {} and is recorded as a successful write, although no applied/skipped outcome is known. A local HTTP-stub counterexample reproduced Ok("{}"). Validate the expected result shape before returning success, as this PR now does for other mutating tools.

The temporary counterexample tests were removed after verification. cargo test -p memoria-mcp -p memoria-api --lib --offline passed on the clean head (42 MCP unit tests plus API unit tests); database-dependent integration tests could not run without a local database.

[aptend]

Deep re-review at exact head 33d14aa (base 9101313). I read all five historical reviews and both resolved inline threads, compared the 9463ac9→current increment, and rechecked the full MCP/API error-dispatch diff. The two prior blockers are closed: memory_pick now compares the caller's requested dry-run mode with the response envelope and rejects a mode swap with a partial-write warning; memory_apply requires all eight ApplyResult outcome arrays before reporting a successful mutation. The added memory_merge result check matches the API's {result} contract. I found no remaining blocking issue. Exact-head local evidence: memoria-mcp library 43/43, tools_unit 9/9, memoria-api library 106/106; git diff --check passed. Exact-head GitHub Check & Clippy, Unit Tests, and DB Tests are green.