fix(adapters): consumer adapter hygiene (CON-01, CON-03..CON-13)
- CON-01: from_mcp discovery follows tools/list pagination (rmcp list_all_tools); three-page paginating-server test - CON-03: from_wss refuses ws:// with a Bearer token unless FromWss::allow_plaintext() is called explicitly (tests: refusal, opt-in, token-less passthrough) - CON-04: audio variant of content_block_union_schema requires ["type","data","mimeType"]; jsonschema-validated audio block - CON-05/07: import-time credential documented on both adapters; dead per-call capability read removed - CON-06: 401 classification typed-first (downcast to rmcp StreamableHttpError<reqwest::Error>; AuthRequired/InsufficientScope/ Client with status 401); a :40101 URL no longer misclassifies (tested) - CON-11: transport tools/call failures declare MCP_TRANSPORT_ERROR; rmcp JSON-RPC errors preserve code (MCP_JRPC_<code>) and data - CON-12: tool names validated at import (/, whitespace, empty → SchemaParse); unit + integration tests - CON-13: tokens held as alkcall Secret<String> (zeroize, redacted Debug) - CON-08/09: no close handles; explicit-limitation notes in from_mcp module docs, from_wss module docs, and ADR-070 - CON-10: full_surface [[test]] required-features = ["mcp","test-support"]; cargo test --features mcp now compiles and passes Verified: cargo test; cargo test --features mcp; cargo test --all-features; cargo clippy (--all-features) --all-targets -- -D warnings; cargo fmt --check
This commit is contained in:
@@ -1,7 +1,7 @@
|
||||
---
|
||||
id: review-001-consumer-adapter-hygiene
|
||||
name: from_wss/from_mcp consumer fixes — plaintext ws://, 401 classification, token hygiene (CON-03, CON-05..CON-13)
|
||||
status: pending
|
||||
status: completed
|
||||
depends_on: []
|
||||
scope: moderate
|
||||
risk: low
|
||||
@@ -59,15 +59,15 @@ cleanup list:
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Paginated `tools/list` fully imported (paginating-server test)
|
||||
- [ ] `ws://` + token refused (or explicit opt-out) with a clear error (test)
|
||||
- [ ] Audio variant of `content_block_union_schema` satisfied by a valid block (test)
|
||||
- [ ] No substring-based 401 classification on typed paths; a URL containing `:401x` no longer misclassifies (test)
|
||||
- [ ] CON-05/07 docs corrected; dead capability read removed
|
||||
- [ ] CON-11/12: transport-failure error declared; remote tool names sanitized (test)
|
||||
- [ ] Tokens held as `Secret<String>`
|
||||
- [ ] `cargo test --features mcp` compiles and passes (CON-10), as does `cargo test --all-features`
|
||||
- [ ] CON-08/09: close handles exist or docs/ADR-070 note state the limitation
|
||||
- [x] Paginated `tools/list` fully imported (paginating-server test)
|
||||
- [x] `ws://` + token refused (or explicit opt-out) with a clear error (test)
|
||||
- [x] Audio variant of `content_block_union_schema` satisfied by a valid block (test)
|
||||
- [x] No substring-based 401 classification on typed paths; a URL containing `:401x` no longer misclassifies (test)
|
||||
- [x] CON-05/07 docs corrected; dead capability read removed
|
||||
- [x] CON-11/12: transport-failure error declared; remote tool names sanitized (test)
|
||||
- [x] Tokens held as `Secret<String>`
|
||||
- [x] `cargo test --features mcp` compiles and passes (CON-10), as does `cargo test --all-features`
|
||||
- [x] CON-08/09: close handles exist or docs/ADR-070 note state the limitation
|
||||
|
||||
## References
|
||||
|
||||
@@ -76,10 +76,89 @@ cleanup list:
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills during implementation. Independent of the WS tasks.
|
||||
> Independent of the WS tasks.
|
||||
> The from_wss notify/pending fixes are tracked separately in
|
||||
> review-001-ws-robustness (WS-02/CON-02 share one mechanism).
|
||||
>
|
||||
> Implementation notes:
|
||||
> - CON-03 chose the safer default: `ws://` is refused **whenever a token
|
||||
> is present** unless `FromWss::allow_plaintext()` is called explicitly
|
||||
> (unconditional refusal would break every existing test dial of a
|
||||
> token-less local producer; without a token there is no credential to
|
||||
> leak). `WssSession::connect` grew an `allow_plaintext` parameter; the
|
||||
> refusal lives in the config/validation path, before the dial.
|
||||
> - CON-06: typed matching on rmcp's `StreamableHttpError<reqwest::Error>`
|
||||
> via `DynamicTransportError` downcast (`AuthRequired`,
|
||||
> `InsufficientScope`, `Client(e)` with `e.status() == 401`), with a
|
||||
> word-boundary string fallback (`AuthRequired` / `InsufficientScope` /
|
||||
> `www-authenticate` / the word "unauthorized") — a URL containing
|
||||
> `:40101` misclassifies no more (tested with a live 401 server and a
|
||||
> dead `:40101` endpoint).
|
||||
> - CON-13: alkcall already exports `Secret<T>` (zeroizing, `[REDACTED]`
|
||||
> Debug) from `alkcall::core::types` — reused it for both adapters'
|
||||
> builder-held tokens; no new type. Debug-suppression test added.
|
||||
> - CON-11: transport-level `tools/call` failures declare
|
||||
> `MCP_TRANSPORT_ERROR` in each op's `error_schemas` (retryable); rmcp
|
||||
> `ServiceError::McpError` is preserved as `MCP_JRPC_<rrrr0>` with the
|
||||
> message and `data` carried into the CallError (JSON-RPC fidelity).
|
||||
> - CON-12: sanitization is validate-and-refuse (not rewrite): a tool
|
||||
> name that is empty, contains `/` or whitespace fails the whole
|
||||
> import with `AdapterError::SchemaParse` — silently renames would
|
||||
> break the remote's own `tools/call` round-trip.
|
||||
> - CON-08/09: explicit-limitation notes, no close handles and no
|
||||
> reconnect layer — module docs on both adapters plus an "Explicit
|
||||
> session-lifetime limitation" section in ADR-070.
|
||||
|
||||
## Summary
|
||||
|
||||
> Filled on completion.
|
||||
> Filled on completion.
|
||||
|
||||
**Status: complete.** All eleven consumer-adapter findings from review 001
|
||||
Part G are resolved; see the checkbox list plus the implementation notes
|
||||
above.
|
||||
|
||||
- **CON-01**: `from_mcp` discovery now uses rmcp's `list_all_tools()`
|
||||
(follows `next_cursor`); integration test `import_follows_tools_list_pagination`
|
||||
exercises a three-page paginating server.
|
||||
- **CON-03**: `WssSession::connect` refuses `ws://` when a token is
|
||||
present unless `FromWss::allow_plaintext()` was called; clear
|
||||
`AdapterError::Transport` naming CON-03. Tests: refusal, explicit
|
||||
opt-in, and token-less passthrough.
|
||||
- **CON-04**: audio variant of `content_block_union_schema` now requires
|
||||
`["type", "data", "mimeType"]`; jsonschema-validated audio block test.
|
||||
- **CON-05**: dead `_token_present` capability read removed; module docs
|
||||
rewritten to state the import-time (transport-pinned) credential
|
||||
semantics.
|
||||
- **CON-06**: 401 classification is typed-first (downcast to rmcp's
|
||||
`StreamableHttpError<reqwest::Error>`, matching `AuthRequired`,
|
||||
`InsufficientScope`, and `Client` errors with `status() == 401`);
|
||||
the string fallback checks auth wording, not bare "401". Tests cover a
|
||||
live 401, a dead `:40101` endpoint whose URL would substring-match,
|
||||
and the wording cases.
|
||||
- **CON-07**: `from_wss` docs now state the reality (dial-time token,
|
||||
imported handlers carry no capabilities at all).
|
||||
- **CON-08**: rmcp session teardown remains absent (no close handle);
|
||||
documented as an explicit limitation in the `from_mcp` module docs
|
||||
(import-once guidance, fire-and-forget leak named).
|
||||
- **CON-09**: same-disposition ADR-070 note: no teardown handle in v1,
|
||||
reconnect stacks duplicate sessions by design; no reconnect layer built.
|
||||
- **CON-10**: `[[test]] full_surface` `required-features` now
|
||||
`["mcp", "test-support"]`; `cargo test --features mcp` compiles and
|
||||
passes (verified).
|
||||
- **CON-11**: transport-level `tools/call` failures map to the newly
|
||||
declared `MCP_TRANSPORT_ERROR` error schema (retryable); rmcp
|
||||
JSON-RPC errors preserve code (`MCP_JRPC_<code>`) and `data` (as
|
||||
error details). Integration test kills the server mid-call.
|
||||
- **CON-12**: `sanitize_tool_name` validates at import — `/` (which
|
||||
would break the two-segment `ns/op` convention), whitespace, and
|
||||
empty names fail the import with `SchemaParse`; unit + integration
|
||||
tests.
|
||||
- **CON-13**: both adapters hold tokens as alkcall's `Secret<String>`
|
||||
(zeroizing on drop, `[REDACTED]` Debug); redaction is tested.
|
||||
|
||||
Verification: `cargo test` (265 lib), `cargo test --features mcp`
|
||||
(313 lib + 9 from_mcp_integration — the CON-10 gate),
|
||||
`cargo test --all-features` (329 lib + all integration suites),
|
||||
`cargo clippy --all-targets -- -D warnings`,
|
||||
`cargo clippy --all-features --all-targets -- -D warnings`,
|
||||
`cargo fmt --check` — all pass.
|
||||
Reference in New Issue
Block a user