[PR #5470] [MERGED] fix(mcp): guide agents through tool error recovery #6932

Closed
opened 2026-11-01 21:44:09 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5470
Author: @gensokyo-dev
Created: 10/20/2026
Status: ✅ Merged
Merged: 10/20/2026
Merged by: @gensokyo-dev

Base: main ← Head: feat/5431-tool-recovery-errors


📝 Commits (6)

  • 4df946a fix(mcp): add agent recovery guidance to tool errors
  • 6a43bc9 test(mcp): assert structured adb input recovery
  • bade159 test(mcp): assert structured depth validation recovery
  • 12c5dc5 test(mcp): assert bounded browser input recovery
  • 4a9af96 chore: merge main and preserve MCP recovery envelopes
  • 8d048ad fix(mcp): preserve validation diagnostics within wire budgets

📊 Changes

20 files changed (+1007 additions, -66 deletions)

View changed files

📝 docs/error-contract.schema.json (+540 -0)
📝 docs/mcp-contracts.md (+18 -1)
📝 docs/user-visible-errors.csv (+2 -2)
📝 src/contracts/errorSchemas.ts (+4 -1)
📝 src/domain/analysisErrorPresentation.ts (+10 -1)
📝 src/domain/analysisErrorProjection.test.ts (+46 -0)
📝 src/domain/analysisErrorProjection.ts (+21 -0)
➕ src/domain/analysisErrorRecovery.ts (+36 -0)
📝 src/server/EvidenceMcpServer.test.ts (+90 -16)
📝 src/server/EvidenceMcpServer.ts (+122 -22)
📝 tests/acceptance/applications/javascriptAnalysisLimits.test.ts (+12 -1)
📝 tests/boundary/cli/browserInputDiagnostics.test.ts (+7 -1)
📝 tests/boundary/filesystem/processCliFileInput.test.ts (+8 -1)
📝 tests/boundary/mcp/adbMcp.test.ts (+21 -4)
📝 tests/boundary/mcp/browserMcp.test.ts (+10 -0)
📝 tests/boundary/mcp/browserScenarioInputDiagnosticsMcp.test.ts (+8 -1)
📝 tests/boundary/mcp/ghidraInventoryMcp.test.ts (+9 -3)
📝 tests/boundary/mcp/largeBytePattern.test.ts (+11 -7)
📝 tests/boundary/mcp/sessionMcp.test.ts (+20 -4)
📝 tests/boundary/mcp/webBundleContainmentMcp.test.ts (+12 -1)

📄 Description

Summary

Fixes #5431. Wrong MCP argument types currently produce raw SDK text, unknown tool names lack discovery guidance, and generic timeouts do not explain which deadline to adjust. This change adds compact, typed recovery guidance shared by CLI/MCP and normalizes SDK input errors at the MCP delivery boundary.

Problem and expected behavior

For example, inspect_wasm_artifact({path:42}) now returns canonical invalid_request with recovery: {kind:"inspect_input_contract", operation:"inspect_wasm_artifact"} and directs the caller to the advertised input schema or tools/list before correcting the request. Timeout errors identify the server/provider deadline and recommend inspecting state before retrying. Increasing the MCP client deadline is appropriate only when that client ended the call first.

Change and scope

  • Add optional inspect_input_contract / inspect_timeout_state recovery metadata to the existing error projection and JSON Schema.
  • Normalize SDK argument errors according to the actual wire budget, preserving complete rejected-field diagnostics when they fit. Retain original Evidence before shortening a response that overflows after wrapping/escaping, and preserve protocol metadata and target-content markers.
  • Direct unknown-tool callers to tools/list; preserve JSON-RPC -32602.

Contract and boundary impact

  • Semantic owner: domain error projection/presentation; transport owns SDK-boundary normalization.
  • Contribution scope: ordinary usability fix within existing contracts; additional provider/authority decision not applicable.
  • CLI/MCP: additive optional recovery metadata; bounded SDK validation errors now use canonical JSON.
  • Provider/platform: provider-specific remediation remains authoritative.
  • Evidence: retained oversized originals and provenance remain available.
  • Target selection, execution authority, and cleanup: unchanged.
  • Generated output: authored docs/error-contract.schema.json regenerated; no package or installation changes.

Expected effects:

  • Documentation or usability only; no target interaction.

Evidence and regression coverage

The baseline returned raw SDK text for a wrong inspect_wasm_artifact argument. Regression tests cover canonical validation errors, unknown tools, oversized retention, provenance markers, protocol envelopes, CLI/MCP equivalence, deep JSON input, and real JavaScript/browser-bundle timeouts.

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • Full GitHub CI on earlier 12c5dc553: run 39420774558 passed, including six portable shards, Apple/browser, native, SQLite and other selected environment lanes. Latest main c374c2fe7 was integrated; final 8d048ad34 was squash-merged as e50a271c7. Their complete file trees are identical. Final PR CI run 39432049829 passed build/docs; its source tests passed 12,273 tests with only the three reproduced main-baseline path assertions failing. Static checks failed on the reproduced main-baseline duplicate threshold. The compiled suite was not green: one failed shard cancelled four peers. Four legacy recovery-text assertions introduced by the main update are corrected in follow-up #5536, with all five CLI tests passing locally. Remaining observed compiled failures were reproduced on the pre-change baseline below.
  • Latest local typecheck/lint, format and docs passed. Domain/server source tests: 63 passed. Four compiled MCP suites: 13 passed. Earlier focused ADB/session/browser suites: 22 passed. Coverage includes complete >5,000-character diagnostics at a 16 KiB wire budget and retained quote-expansion overflow on legacy/modern protocols. Architecture guards and knip passed before the main integration.
  • Real JavaScript timeout acceptance: 6 tests passed; elapsed 40.33s, peak process RSS approximately 552 MiB. Large input/stdio input diagnostics passed, peak RSS approximately 1.23 GiB. Repository-wide static checks peaked near 4.76 GiB RSS without OOM.
  • Codex CLI with temporary REA MCP configuration completed actual recovery: a 1ms small-app timeout led to a session-state check and successful bounded analysis in three MCP calls; a wrong input_path type led to correction and successful analysis in two calls. Application coverage was complete; semantic unknowns remained explicit. The second task used 128,076 input tokens (89,984 cached) and 632 output tokens, 39.67s wall / 12.46s CPU, maximum individual-process RSS 440 MiB. Concurrent process-family peak was not sampled; no before/after or statistical efficiency improvement is claimed.
  • Additional real-package acceptance used Commander 14.0.3 (252 KiB), Codex CLI 0.161.0 / gpt-6.1-sol, Node 24.18.0 and only the analysis/session MCP tools. A forced 1ms timeout led to a session-state check and successful 30s retry in three calls, preserving the actual max_heap_mb:128 and summary view while lowering semantic limits. The tool reported 12 relevant files, 10 parsed JS files and 108 findings; application/semantic coverage remained explicitly partial, with 452 retained unknowns. Timeout/full/summary Evidence references survived. Whole-agent usage: 241,221 input tokens (189,440 cached), 985 output tokens, 56.52s wall / 15.57s CPU, maximum individual-process RSS 659 MiB; concurrent process-family peak was not sampled. This is a recovery success, not a token-efficiency improvement claim.
  • Independent Codex CLI review identified lost diagnostics; the producing boundary was fixed and a second review reported no actionable regressions. Both related GitHub review comments are addressed.
  • Known main-baseline CI failures: duplicate detection is 3.5279% on c374c2fe7, versus 3.5255% on this change (both exceed 3.5%); three cliJsonPaths.test.ts message assertions reproduce unchanged on the baseline. A further compiled comparison reproduced the same five failures before and after this change: TypeScript application graph, two EVM cancellation-warning cases, malformed Protobuf trailer deadline and Electron path-message wording. The isolated baseline run passed 51 tests and failed those five, with two named skips. A separate worker cancellation assertion failed once in CI and passed in the focused baseline run; it is outside changed execution code. These unrelated failures are recorded rather than changing their owning code in this PR.
  • Complete local MCP suite was stopped after 15 minutes and is not claimed as passing; the completed GitHub suite provides broader verification. BYO Hopper/Ghidra/IDA runtime qualification was unavailable. Unknown-tool server guidance was checked through direct stdio because Codex rejects invented bindings before dispatch.

Compatibility, safety, and release

Recovery is optional. Clients that depended on SDK prose should parse the documented canonical error instead. Recovery fields do not echo input arguments or embed the tool catalog; bounded SDK diagnostic text can include validation values. Raw oversized diagnostics retain the existing Evidence access path. No new provider, execution, injection, mutation, or protection-recovery capability is added.

Author readiness

Human owner: @gensokyo-dev

Recorded at the human owner's explicit instruction and authorization in the task conversation; implementation review and executed validation were performed by Codex.

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/5470 **Author:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Created:** 10/20/2026 **Status:** ✅ Merged **Merged:** 10/20/2026 **Merged by:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Base:** `main` ← **Head:** `feat/5431-tool-recovery-errors` --- ### 📝 Commits (6) - [`4df946a`](https://github.com/morluto/rea/commit/4df946afbc9d8a59e56cebb9e9ca84284ea6f090) fix(mcp): add agent recovery guidance to tool errors - [`6a43bc9`](https://github.com/morluto/rea/commit/6a43bc907494c017823583e20e25c900a8f63dbe) test(mcp): assert structured adb input recovery - [`bade159`](https://github.com/morluto/rea/commit/bade1599cbd45aa0a1c936d9ee77f30c4fc87e44) test(mcp): assert structured depth validation recovery - [`12c5dc5`](https://github.com/morluto/rea/commit/12c5dc553f00778ab472f8d080c8b949d678abf6) test(mcp): assert bounded browser input recovery - [`4a9af96`](https://github.com/morluto/rea/commit/4a9af968c33f9566407a8d65b6e8e1aa0fcf563b) chore: merge main and preserve MCP recovery envelopes - [`8d048ad`](https://github.com/morluto/rea/commit/8d048ad34ed9ef0abd38cac64bb12d52cc07d960) fix(mcp): preserve validation diagnostics within wire budgets ### 📊 Changes **20 files changed** (+1007 additions, -66 deletions) <details> <summary>View changed files</summary> 📝 `docs/error-contract.schema.json` (+540 -0) 📝 `docs/mcp-contracts.md` (+18 -1) 📝 `docs/user-visible-errors.csv` (+2 -2) 📝 `src/contracts/errorSchemas.ts` (+4 -1) 📝 `src/domain/analysisErrorPresentation.ts` (+10 -1) 📝 `src/domain/analysisErrorProjection.test.ts` (+46 -0) 📝 `src/domain/analysisErrorProjection.ts` (+21 -0) ➕ `src/domain/analysisErrorRecovery.ts` (+36 -0) 📝 `src/server/EvidenceMcpServer.test.ts` (+90 -16) 📝 `src/server/EvidenceMcpServer.ts` (+122 -22) 📝 `tests/acceptance/applications/javascriptAnalysisLimits.test.ts` (+12 -1) 📝 `tests/boundary/cli/browserInputDiagnostics.test.ts` (+7 -1) 📝 `tests/boundary/filesystem/processCliFileInput.test.ts` (+8 -1) 📝 `tests/boundary/mcp/adbMcp.test.ts` (+21 -4) 📝 `tests/boundary/mcp/browserMcp.test.ts` (+10 -0) 📝 `tests/boundary/mcp/browserScenarioInputDiagnosticsMcp.test.ts` (+8 -1) 📝 `tests/boundary/mcp/ghidraInventoryMcp.test.ts` (+9 -3) 📝 `tests/boundary/mcp/largeBytePattern.test.ts` (+11 -7) 📝 `tests/boundary/mcp/sessionMcp.test.ts` (+20 -4) 📝 `tests/boundary/mcp/webBundleContainmentMcp.test.ts` (+12 -1) </details> ### 📄 Description ## Summary Fixes #5431. Wrong MCP argument types currently produce raw SDK text, unknown tool names lack discovery guidance, and generic timeouts do not explain which deadline to adjust. This change adds compact, typed recovery guidance shared by CLI/MCP and normalizes SDK input errors at the MCP delivery boundary. ## Problem and expected behavior For example, `inspect_wasm_artifact({path:42})` now returns canonical `invalid_request` with `recovery: {kind:"inspect_input_contract", operation:"inspect_wasm_artifact"}` and directs the caller to the advertised input schema or `tools/list` before correcting the request. Timeout errors identify the server/provider deadline and recommend inspecting state before retrying. Increasing the MCP client deadline is appropriate only when that client ended the call first. ## Change and scope - Add optional `inspect_input_contract` / `inspect_timeout_state` recovery metadata to the existing error projection and JSON Schema. - Normalize SDK argument errors according to the actual wire budget, preserving complete rejected-field diagnostics when they fit. Retain original Evidence before shortening a response that overflows after wrapping/escaping, and preserve protocol metadata and target-content markers. - Direct unknown-tool callers to `tools/list`; preserve JSON-RPC `-32602`. ## Contract and boundary impact - Semantic owner: domain error projection/presentation; transport owns SDK-boundary normalization. - Contribution scope: ordinary usability fix within existing contracts; additional provider/authority decision not applicable. - CLI/MCP: additive optional recovery metadata; bounded SDK validation errors now use canonical JSON. - Provider/platform: provider-specific remediation remains authoritative. - Evidence: retained oversized originals and provenance remain available. - Target selection, execution authority, and cleanup: unchanged. - Generated output: authored `docs/error-contract.schema.json` regenerated; no package or installation changes. Expected effects: - [x] Documentation or usability only; no target interaction. ## Evidence and regression coverage The baseline returned raw SDK text for a wrong `inspect_wasm_artifact` argument. Regression tests cover canonical validation errors, unknown tools, oversized retention, provenance markers, protocol envelopes, CLI/MCP equivalence, deep JSON input, and real JavaScript/browser-bundle timeouts. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - Full GitHub CI on earlier `12c5dc553`: [run 39420774558](https://github.com/morluto/rea/actions/runs/39420774558) passed, including six portable shards, Apple/browser, native, SQLite and other selected environment lanes. Latest main `c374c2fe7` was integrated; final `8d048ad34` was squash-merged as `e50a271c7`. Their complete file trees are identical. Final [PR CI run 39432049829](https://github.com/morluto/rea/actions/runs/39432049829) passed build/docs; its source tests passed 12,273 tests with only the three reproduced main-baseline path assertions failing. Static checks failed on the reproduced main-baseline duplicate threshold. The compiled suite was not green: one failed shard cancelled four peers. Four legacy recovery-text assertions introduced by the main update are corrected in [follow-up #5536](https://github.com/morluto/rea/pull/5536), with all five CLI tests passing locally. Remaining observed compiled failures were reproduced on the pre-change baseline below. - Latest local typecheck/lint, format and docs passed. Domain/server source tests: 63 passed. Four compiled MCP suites: 13 passed. Earlier focused ADB/session/browser suites: 22 passed. Coverage includes complete >5,000-character diagnostics at a 16 KiB wire budget and retained quote-expansion overflow on legacy/modern protocols. Architecture guards and knip passed before the main integration. - Real JavaScript timeout acceptance: 6 tests passed; elapsed 40.33s, peak process RSS approximately 552 MiB. Large input/stdio input diagnostics passed, peak RSS approximately 1.23 GiB. Repository-wide static checks peaked near 4.76 GiB RSS without OOM. - Codex CLI with temporary REA MCP configuration completed actual recovery: a 1ms small-app timeout led to a session-state check and successful bounded analysis in three MCP calls; a wrong `input_path` type led to correction and successful analysis in two calls. Application coverage was complete; semantic unknowns remained explicit. The second task used 128,076 input tokens (89,984 cached) and 632 output tokens, 39.67s wall / 12.46s CPU, maximum individual-process RSS 440 MiB. Concurrent process-family peak was not sampled; no before/after or statistical efficiency improvement is claimed. - Additional real-package acceptance used Commander 14.0.3 (252 KiB), Codex CLI 0.161.0 / `gpt-6.1-sol`, Node 24.18.0 and only the analysis/session MCP tools. A forced 1ms timeout led to a session-state check and successful 30s retry in three calls, preserving the actual `max_heap_mb:128` and summary view while lowering semantic limits. The tool reported 12 relevant files, 10 parsed JS files and 108 findings; application/semantic coverage remained explicitly partial, with 452 retained unknowns. Timeout/full/summary Evidence references survived. Whole-agent usage: 241,221 input tokens (189,440 cached), 985 output tokens, 56.52s wall / 15.57s CPU, maximum individual-process RSS 659 MiB; concurrent process-family peak was not sampled. This is a recovery success, not a token-efficiency improvement claim. - Independent Codex CLI review identified lost diagnostics; the producing boundary was fixed and a second review reported no actionable regressions. Both related GitHub review comments are addressed. - Known main-baseline CI failures: duplicate detection is 3.5279% on `c374c2fe7`, versus 3.5255% on this change (both exceed 3.5%); three `cliJsonPaths.test.ts` message assertions reproduce unchanged on the baseline. A further compiled comparison reproduced the same five failures before and after this change: TypeScript application graph, two EVM cancellation-warning cases, malformed Protobuf trailer deadline and Electron path-message wording. The isolated baseline run passed 51 tests and failed those five, with two named skips. A separate worker cancellation assertion failed once in CI and passed in the focused baseline run; it is outside changed execution code. These unrelated failures are recorded rather than changing their owning code in this PR. - Complete local MCP suite was stopped after 15 minutes and is not claimed as passing; the completed GitHub suite provides broader verification. BYO Hopper/Ghidra/IDA runtime qualification was unavailable. Unknown-tool server guidance was checked through direct stdio because Codex rejects invented bindings before dispatch. ## Compatibility, safety, and release Recovery is optional. Clients that depended on SDK prose should parse the documented canonical error instead. Recovery fields do not echo input arguments or embed the tool catalog; bounded SDK diagnostic text can include validation values. Raw oversized diagnostics retain the existing Evidence access path. No new provider, execution, injection, mutation, or protection-recovery capability is added. ## Author readiness Human owner: @gensokyo-dev Recorded at the human owner's explicit instruction and authorization in the task conversation; implementation review and executed validation were performed by Codex. - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:44:09 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#6932
No description provided.