Commit graph inspector/core/mcp/listSalvage.ts
Author SHA1 Message Date
cliffhall
71cbe27d83 fix: address Copilot round 5 on the list-salvage path (#1909)
Both findings are the same shape: the fallback accepting what the strict path
would not.

- `ModernResultEnvelopeSchema` typed `ttlMs` as any number, where the codec
  requires a non-negative integer. `ttlMs: -1` (or `0.5`) plus one bad entry
  would salvage, reporting the entry and hiding the envelope violation. Tightened
  to `z.int().min(0)`.

- The item schema was the SDK's era-NEUTRAL `ToolSchema`, and the negotiated
  era's wire schema is stricter in ways the SDK does not export. Verified
  against SDK 2.0.0 on a real connection: on legacy, a tool with
  `outputSchema: { type: "array" }` is refused by the strict aggregate but
  accepted by the neutral schema; on modern, `execution` is STRIPPED by the
  strict path but kept by the neutral one. Either way a salvaged list could show
  — and let the user call — a tool the connection's era says isn't valid.

  `toolItemSchemaForEra` closes both. Replicating rules that live in the SDK's
  internals is a real cost; `listSalvage-era.test.ts` is what makes it safe to
  pay, pinning each rule to the strict path's OBSERVED behavior so a divergence
  that closes or widens fails loudly instead of rotting.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
2026-08-15 18:46:54 -04:00
cliffhall
573b14e14e fix: address Copilot round 4 on the list-salvage path (#1909)
Two findings, both about the fallback accepting more than it should.

- The lenient page schema validated pagination and nothing else, so a response
  carrying a malformed entry AND an unrelated top-level violation salvaged: the
  entry was reported and the other violation vanished with the strict error that
  caught it. The page schema is now DERIVED from the method's real result schema
  with only its item array relaxed (`lenientListPageSchema`), so every other
  field — `_meta`, `nextCursor`, anything a future revision adds — is still
  validated. The modern era envelope the raw-wire path bypasses (`resultType`,
  `ttlMs`, `cacheScope`) is checked explicitly for the same reason.

  Salvage is also narrowed to `InvalidResult`: `UnsupportedResultType` means the
  codec doesn't recognize what KIND of frame arrived, so it isn't a list with a
  bad entry and reporting an item-level reason would hide a protocol failure.
  Marking the Protocol entry still applies to both, hence two predicates.

- On every rethrow path the re-walk had already answered the method, so the
  caller's own `markResponseRejected` (in `ManagedListState.refresh`) landed on
  the lenient page that succeeded and left the refused response clean. The
  correlation is restored before rethrowing — deleted when nothing had answered
  before, so the outer mark is a no-op rather than a lie.

`nextCursorOf` / `rawItemsOf` narrow the page's fields instead of casting: the
runtime-keyed `extend()` loses the shape, and asserting it back would be exactly
the unjustified cast the last round removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
2026-08-15 18:22:10 -04:00
cliffhall
1889e04135 fix: address Copilot round 2 on the list-salvage path (#1909)
All four findings taken (one visible, three in the suppressed block), plus the
`excludedTools` sibling leak the round-1 reply had deferred:

- `rawItemsOf` read a missing or non-array list member as an empty page, so a
  top-level violation (`{ tools: "invalid" }`) on a later page was discarded
  and the call returned a silently truncated list. It now returns `undefined`
  for "not a list at all", and the walk rethrows the original strict error.

- A failure of the re-walk ITSELF (a malformed `nextCursor`, a connection lost
  mid-walk) escaped in place of the original error, whose response is the one
  `rejectedResponseId` was captured for. The walk is now wrapped: the original
  surfaces, the secondary is logged so it stays diagnosable.

- The exclusion scan's lenient fallback discarded its malformed entries despite
  the comment claiming they were reported. Observable when the aggregate is
  served from cache: the salvage never runs, so nothing else reports them. The
  scan now returns them and `refreshExcludedTools` reports them — only ever
  adding, so a clean scan can't clear what the salvage path just wrote.

- An empty-string cursor is a valid cursor, and `cursor ? {cursor} : {}` dropped
  it, re-requesting page one and duplicating its entries until the
  repeated-cursor guard fired. Both walks now test `!== undefined`.

- `excludedTools` had the same session leak fixed for `malformedListItems` last
  round: cleared only on an explicit `disconnect()`, so the `onerror`-with-no-
  `onclose` route carried the previous server's set into the next connection.
  Cleared on the start-clean path too — safe because every `listAllTools`
  recomputes it, so this can only drop a stale set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
2026-08-15 17:52:36 -04:00
cliffhall
fe5b622fd1 fix: salvage a list result's valid entries when one is malformed (#1909)
The SDK validates a list result as a whole, so a single non-conforming entry
rejected the entire response and the panel showed "Couldn't load resources"
with zero items — the exact opposite of what a debugging tool should do with a
server that is 90% right. Reported against a PHP server whose empty
`annotations` object reaches the wire as `[]` rather than `{}`.

`listAll*` now falls back to a lenient re-walk when — and only when — the SDK
rejects a result it received (`InvalidResult` / `UnsupportedResultType`), keeps
the entries that validate, and reports the rest. A conforming server takes the
strict, cache-aware path exactly as before.

Salvaging is not accepting: the dropped entries are named with their reason in
a warning above the list, and the Protocol entry is still marked rejected, so
the wire truth survives. When every entry validates, the original error is
rethrown rather than swallowed — the rejection was about something the
per-item pass cannot explain.

`isClientDecodeRejection` moves to the new module and is now shared with
managedListState, which had the only copy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
2026-08-15 16:52:11 -04:00