fix(mcp): make the conformance harness survive a bad server, and tighten two claims - #376
Merged
Merged
Conversation
…ten two claims The harness could die before printing the verdict it exists to produce. `next(t for t in tools if ...)` raises StopIteration when a server omits a tool, so the one condition most worth reporting -- a missing tool -- killed the run instead of failing a check. Looked up by name with .get now, so an absent tool registers a failure and the run still reaches its verdict. The tasks section is wrapped in try/finally with both clients closed there. Without it any unexpected payload left spawned Burrow processes running and swallowed the verdict at the same time. `tc` is initialised to None first, so a failure inside Client() itself cannot turn the cleanup into a NameError. A missing binary argument now exits with usage text. It previously reached subprocess.Popen as None and failed with a TypeError that named neither the script nor the missing argument. testResourceRead_isPrivateAndShortLived compared the TTL against digestTTL (60s) when burrow://info declares liveTTL (5s) -- a bound twelve times looser than the thing it was checking, which would have held even if the resource regressed to minute-long caching of live machine state. `socketfilterfw` is not on the default PATH; it lives in /usr/libexec/ApplicationFirewall, so the documented command failed for anyone who pasted it.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe conformance harness now validates required inputs and tools, closes clients during failures, and updates cache and firewall command expectations. ChangesConformance harness robustness
Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-lands four review findings from #369. They were fixed there, then reverted because my push conflicted with unpushed local work on that branch, and the revert pair got dropped during the restack — so #369 merged without them. The four threads on #369 are still open; this closes them.
The harness could die before printing its verdict
next(t for t in tools if t["name"] == ...)raisesStopIterationwhen a server omits a tool — so the single condition most worth reporting killed the run instead of failing a check. Now looked up through aby_namedict with.get, so a missing tool registers a failure and the run still reaches its verdict.The tasks section is wrapped in
try/finallywith both clients closed there. Without it, any unexpected payload left spawned Burrow processes running and swallowed the verdict at the same time.tcis initialised toNonebefore thetry, so a failure insideClient()itself can't turn the cleanup into aNameError.A missing argument failed unhelpfully
No binary path meant
Nonereachedsubprocess.Popen, raising aTypeErrornaming neither the script nor the argument. It now exits with usage text. Verified by running it with no arguments.An assertion twelve times looser than the thing it checked
testResourceRead_isPrivateAndShortLivedcompared the TTL againstdigestTTL(60s), butburrow://infodeclaresliveTTL(5s) inMCPResources.swift. The bound would have held even if the resource regressed to caching live machine state for a full minute. Now pinned toliveTTL; the private cache-scope assertion is unchanged.A documented command that couldn't run
socketfilterfwisn't on the defaultPATH—which socketfilterfwfinds nothing; it lives at/usr/libexec/ApplicationFirewall/. Anyone pasting the documented command got "command not found". Now absolute. The other three status commands resolve fine and are untouched.Verification
Python compiles and the new usage guard was exercised directly. The Swift change is a one-line constant swap against
MCPProtocol.Cache.liveTTL; CI covers the build.Summary by CodeRabbit
Documentation
Bug Fixes