test(mcp): pin owner refusals as typed isError problems - #2319
Conversation
Since 21552e3 every graph-tool-owner refusal, including arguments the typed request parser rejects, answers as an isError tool result carrying the owner's whole problem record (an MCP 2025-11-25 tool-execution error) instead of a JSON-RPC error. 74 mcp_suite tests still pinned the JSON-RPC shape; they now assert the problem's literal kind, code and message. Non-object arguments, retained tools, source edits and callers still answer JSON-RPC errors, and their pins say so explicitly.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f165e1b65d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let problem = match &result["structuredContent"]["problem"] { | ||
| Value::Null => &result["problem"], | ||
| transported => transported, |
There was a problem hiding this comment.
Transport-level tests now call this helper, but the fallback to result["problem"] also accepts the dispatcher-private shape that rmcp is supposed to remove and relocate to structuredContent.problem. If that transport conversion regresses, these tests still pass even though MCP clients cannot read the problem from the standard field; use separate helpers for handler and wire results, with the wire helper requiring structuredContent.problem.
AGENTS.md reference: AGENTS.md:L213-L214
Useful? React with 👍 / 👎.
| assert_unavailable( | ||
| &retrieve(&fixture, json!({"handle": SHORT_HANDLE, "max_chars": 0})).await, | ||
| "response_handle_invalid_page_size", | ||
| "tracedecay_retrieve max_chars must be at least 1", |
There was a problem hiding this comment.
When a caller supplies max_chars: 0, the request is rejected by input validation before any authority becomes unavailable, and the previous assertion correctly treated it as non-retryable invalid parameters. Requiring kind == "unavailable" now pins a misleading typed state, causing clients to handle a correctable request as a service outage; map this refusal to invalid_request while retaining its specific code and message.
AGENTS.md reference: AGENTS.md:L9-L12
Useful? React with 👍 / 👎.
| fn assert_unavailable(response: &Value, code: &str, message: &str) { | ||
| let problem = crate::support::refusal_problem(&response["result"]); | ||
| assert_eq!(problem["kind"], "unavailable"); | ||
| assert_eq!(problem["code"], code); | ||
| assert_eq!(problem["message"], message); |
There was a problem hiding this comment.
This helper is used for both invalid page/offset refusals, which were asserted as retryable: false, and corrupt/unreadable cache failures, which were asserted as retryable: true, but it no longer checks retryability at all. A regression that removes or inverts the retry directive would therefore leave the suite green while changing client recovery behavior; accept the expected retryability as an argument and assert the typed retry fields.
AGENTS.md reference: AGENTS.md:L213-L214
Useful? React with 👍 / 👎.
1 participant
What was wrong
mcp_suitewas red onorigin/master: 511 passed, 74 failed of 585 (reproduced onf1d4e56631, with the CLI binary built first and the test process run outside any memory scope). This includes the two named tests (automation_run_list_refuses_a_non_directory_dashboard_root,schema_test::schema_required_arguments_match_representative_handler_parsers) and the fourgraph_query_testtests (redundancy_reports_ranked_repository_exact_families_with_bounded_pages,test_grep_missing_pattern_errors,test_rank_invalid_direction,tracedecay_by_qualified_name_returns_the_symbol_at_that_exact_name).All 74 have one root cause. 21552e3 removed the adapter that flattened a code-only graph-tool problem into a JSON-RPC error: every graph-tool-owner refusal now keeps the owner's whole problem record and is rendered as an
isErrortool result. That covers arguments the typed parser rejects and route/unavailable problems. Its unit testgraph_tool_adapter_preserves_a_code_only_problem_recordpins that intent.Decision per test: production correct, pins stale (all 74)
2025-11-25, where input-validation failures are tool-execution errors (isError) that the model can act on, not protocol errors.kind,code,retryable,legal_actions, the retry directive and the typed detail.No test showed a genuinely different outcome, so no production change was needed. Each stale pin now asserts the literal
kind,codeand exactmessageof the problem, e.g.Route problems keep their reason code, for example
automation_run_list_refuses_a_non_directory_dashboard_rootnow assertsunavailable/automation_run_ledger_unavailable/automation run ledger is unavailable during list: config error: automation dashboard root is not a directory.Four paths still answer JSON-RPC errors, and their pins now state that explicitly instead of assuming one shape:
arguments: rejected at the MCP boundary before any owner parses them (changelog,files,node,diff_context).fact_store_*,lcm_expand.insert_at.callers.schema_testnow declares the expected refusal shape per tool.Shared helpers are in
tests/mcp_suite/support.rs.refusal_problemreads the transport result (isError, withproblemorstructuredContent.problem).tool_result_problem/expect_tool_refusalread a handlerToolResult, which carries the semantic-error mark the transport later renders asisError.Evidence
origin/masterf1d4e56631, fullmcp_suite:test result: FAILED. 511 passed; 74 failed; 0 ignored.mcp_suite:test result: ok. 585 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 276.97s.isError/semantic error plus the exact problem literal, so reverting production to JSON-RPC errors (or changing kind, code or message) fails them.cargo fmt --all -- --check: clean.cargo clippy -p tracedecay --all-targets --features test-transport,test-helpers -- -D warnings: see the comment below. At the time of writing master is red onclippy::result_large_errintracedecay-daemon-protocol/tracedecay-runtime-core, owned by the in-flight large-error fix. No allow was added.Test-only change; no Windows-specific code touched.