Repository navigation
fix(OMN-20610): MCP tool execution reports a 402 as a typed payment refusal - #4632
Conversation
…efusal, no breaker failure, no server body Onex-Lane: omn20610-4d3b Onex-Session: 079122af71a540fc8e99c987c450d067
…omnibase_infra#4632 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 23687ffc3255aff4c89bf5c77c28b14698b6a6a8.
…base_infra (#13050) * evidence(OMN-20610, OMN-19743): author OCC companion for OmniNode-ai/omnibase_infra#4632 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 23687ffc3255aff4c89bf5c77c28b14698b6a6a8. * evidence(OMN-20610, OMN-19743): self-bind OCC#13050 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: qwen3-review, gpt-oss-review
Models failed: none
New finding threads: 0
Deduped (already posted on this PR): 0
Below quorum (one model only, reported not threaded): 10
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Below quorum: 10 finding(s) raised by one model only (OMN-18479)
These are reported and NOT dropped, but they get no thread and do not block: a single model's finding no other model reproduced is not evidence enough to stop a merge. Read them; act on them if they are right.
- [CRITICAL]
tests/unit/adapters/test_adapter_onex_tool_execution_402.py(qwen3-review) — InfraPaymentRequiredError constructor contract violated in test | The testtest_typed_error_escaping_executor_is_also_fixed_sentenceconstructs `InfraPaymentRequiredError(f"Payment required {PAY_TO} - [MAJOR]
src/omnibase_infra/handlers/mcp/adapter_onex_to_mcp.py(qwen3-review) — Redundant and fragile dual-path payment refusal handling in invoke_tool | Theinvoke_toolmethod inadapter_onex_to_mcp.pyhandles payment refusal in two places: (1) catching `InfraPaymentRequired - [MAJOR]
src/omnibase_infra/adapters/adapter_onex_tool_execution.py(qwen3-review) — Circuit breaker state not updated on payment refusal is an unverified assumption | The diff explicitly excludesInfraPaymentRequiredErrorfrom the circuit breaker failure path. The rationale is impl - [MAJOR]
src/omnibase_infra/adapters/adapter_onex_tool_execution.py: lines 398-405(gpt-oss-review) — Potential leakage of payment details via exception payload | InfraPaymentRequiredError is instantiated with raw_header and raw_body containing the payment-required header and response body. If this ex - [MAJOR]
src/omnibase_infra/adapters/adapter_onex_tool_execution.py: lines 424-432(gpt-oss-review) — Timeout exceptions from httpx are not caught | The code catches built‑in TimeoutError, but httpx raises httpx.TimeoutException (and its subclasses). As a result, timeouts will propagate uncaught, bypa - [MAJOR]
src/omnibase_infra/adapters/adapter_onex_tool_execution.py: lines 221-260(gpt-oss-review) — Uncaught generic exceptions bypass circuit‑breaker accounting | The execute method only catches specific Infra* errors. Any unexpected exception (e.g., ValueError, JSONDecodeError) will escape, leavin - [MINOR]
tests/unit/adapters/test_adapter_onex_tool_execution_402.py(qwen3-review) — Missing test for 402 with empty or missing payment-required header | The production code readsresponse.headers.get("payment-required", "")and passes it to the error constructor. No test covers the - [MINOR]
tests/integration/adapters/test_adapter_onex_tool_execution_402_integration.py(qwen3-review) — Integration test uses a real HTTP server but does not verify thread safety of the counter | The_Serverclass in the integration test usesouter.hits += 1in the request handler, which runs in a s - [MINOR]
src/omnibase_infra/adapters/adapter_onex_tool_execution.py: lines 236-244(gpt-oss-review) — Inconsistent presence of "payment_refused" field in execute response | The execute method includes a "payment_refused" key only when handling InfraPaymentRequiredError. Other success or error paths om - [MINOR]
src/omnibase_infra/adapters/adapter_onex_tool_execution.py: line 511(gpt-oss-review) — Exporting internal constant via all | PAYMENT_REFUSED_MESSAGE is added to the module's all, exposing an internal implementation detail to external importers without clear necessity. | Evidence
✅ Hostile Reviewer — REVIEWEDCritical findings: 1
Semantics (OMN-17492 — the model finds, thread resolution gates)
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review: qwen3-review (Qwen3.8-27B), gpt-oss-review (gpt-oss-120b) (OMN-8468/OMN-8524/OMN-17492) |
OMN-20610. A 402 from a tool endpoint used to become InfraConnectionError in AdapterONEXToolExecution._dispatch_with_client, which recorded a circuit breaker failure and reached the MCP caller as "Connection error: ..." with isError true. Now _dispatch_with_client raises InfraPaymentRequiredError (OMN-20608) on a 402 before raise_for_status, execute() catches it ahead of the broad handlers, records no breaker failure and returns success false, payment_refused true and a fixed sentence, and invoke_tool returns isError true with that same fixed sentence. The server's 402 body and payment-required header never enter the result; only their digests live on the typed error. A typed error that escapes the executor is mapped to the same fixed result.
Acceptance criteria
_circuit_breaker_failuresis 0 and exactly one request was made; a 500 still records one failure.Lab: host=h201 (scratch checkout of the pushed branch, in-process pytest only; the h201 dev lane was not touched, no restart, no deploy), lane=omn20610-4d3b, command=
onex_lab_run.py --via ssh --lane omn20610-4d3b --repo omnibase_infra --ref jonah/omn-20610-mcp-402-typed-refusal -- uv run pytest tests/unit/adapters/test_adapter_onex_tool_execution_402.py tests/unit/handlers/mcp/test_adapter_onex_to_mcp.py tests/integration/adapters/test_adapter_onex_tool_execution_402_integration.py, sha=23687ffc3255 exit=0, observed 44 passed; the integration test runs both adapters in process against a loopback HTTP server answering every POST with a 402 and observed 1 request, 0 breaker failures and the fixed sentence. (--via bus exited 69, no ONEX_LAB_WORK_PYTHON, so the ssh probe was used.) Tests run RED first (import of the new fixed-sentence constant failed on dev).Evidence-Ticket: OMN-20610
Evidence-Source: OCC#13050