fix(discovery): exhaust paginated tools/list before comparison - #633
fix(discovery): exhaust paginated tools/list before comparison#633noah-ing wants to merge 4 commits into
Conversation
Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
|
Reviewed the pagination exhaustion logic in discovery.py and its integration into proxy.py and stdio.py, cursor handling including the empty string vs absent distinction, duplicate and cycle detection, the page budget boundary, and stdio cancellation cleanup. All of that checks out and the 79 new tests cover it well. One thing worth flagging, not blocking, more a heads up on cost. _check_upstream_drift and _check_provenance each call _advertised_tools independently, with separate first contact caches, self._drift_checked and self._provenance. On the very first tool call against a server that has provenance_record_path configured, both caches are empty, so _call_tool_impl calls _check_upstream_drift directly, then later in the same call _forward_to_upstream calls _check_provenance, and both trigger the full paginated tools/list fetch. Before this PR that was cheap either way, one request per call. Now _advertised_tools can be up to MAX_DISCOVERY_PAGES round trips, and cold start against a server with provenance configured just paid that cost twice back to back instead of once, doubling stdio child process round trips too on that path. Given the PR's own note that the page limit bounds request count rather than elapsed time, this seems worth closing, maybe by caching the _advertised_tools() result itself per server per session rather than caching only the two downstream comparisons, so both callers share one fetch. Happy to see this land separately if you would rather not hold the PR on it. |
Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
|
Thanks for tracing the cold-call path — you're right. Fixed in 8b8ba3b. Drift and provenance now share one completed paginated acquisition per server/publisher authority per session. Concurrent readers wait for that acquisition, and a drift check is not marked complete while discovery is still in flight. Completed unchecked outcomes are cached distinctly from empty catalogs; cancellation is not cached. Session rebinding resets the acquisition and both comparison caches together, with old in-flight results discarded before reuse. Added 25 regressions. The 12 public-call transport cases use HTTP JSON/SSE or real stdio subprocess I/O and actual signed TRACE provenance verification. All 12 fail on the preceding commit with four discovery requests instead of two, then pass with this change. They also cover warm reuse, warn-only mismatch, and incomplete discovery with and without a required-provenance floor. The other cases cover concurrency, cancellation ownership, identity isolation, and session reset. Full host and clean no-cache Linux runs each pass 1,803 tests, with six unchanged optional/hardware skips. Ruff, mypy, Bandit, dependency/secret scans, and fresh wheel/sdist installation checks pass; all 29 newly changed executable lines are covered. The existing unchecked-call policy is unchanged, as are the page-count versus elapsed-time limit and non-atomic-snapshot caveats. The changelog credits your observation. |
What
Exhaust upstream
tools/listpagination before drift or provenance comparison, using one acquisition helper for HTTP and stdio.nextCursorcompletes discovery.Why
Closes #631. Credit to @solloek369-arch for the observation on #566 and @imran-siddique for confirming and filing the issue.
An approved tool on page two previously looked withdrawn; conversely, a matching first page could conceal failed acquisition of a later page. The initial transport regressions on unchanged production code produced 37 failures / 29 existing passes.
The duplicate-acquisition cost identified by qubeena07 is addressed in
8b8ba3b. Twelve additional public-call transport regressions fail on the preceding commit with four discovery requests instead of two, then pass with the shared cache. Both comparisons still run against the completed listing.Security impact
Incomplete acquisition is
unchecked, never a comparison against partial tools. The existing behavior allowing calls when discovery is unchecked remains unchanged; this PR does not turn that state into a fail-closed authorization gate. A configured required-provenance floor still prevents forwarding when provenance is unchecked.No changes to approval semantics, catalog hash construction, checkpoint binding, signature behavior, or #566 acceptance-matrix redesign. The page limit bounds request count, not total elapsed time or bytes. Pagination does not provide an atomic snapshot of a changing server. Validation covers acquisition shapes and unambiguous names, not the full MCP tool schema. First-contact caching is not continuous monitoring: completed unchecked acquisitions are cached too. Cancellation is not cached, but the existing stdio child-close behavior does not automatically restart that child. These limits are recorded in
LIMITATIONS.md.Test plan
pip check; runtime network disabled, non-root, all capabilities dropped.ruff check src/ tests/andmypy src/cmcp_runtime/ src/cmcp_verify/pass. Ruff formatting passes for the new test files and modified production-code range; unrelated pre-existing formatting is unchanged.VERIFIED, complete change →CATALOG_MISMATCH, incomplete discovery →UNCHECKED.--skip-editable), Gitleaks diff scan, detect-secrets changed-file scan, and whitespace checks pass.Host and container use the repository's hash-locked
requirements/dev.txt; no dependencies were added or changed. These are software protocol regressions, not hardware-attestation evidence.DCO sign-off