fix(runner): safe hardening from the stack review pass#5248
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe runner updates MCP shim bundling and uploads, hardens relay removal and watch accounting, authenticates MCP HTTP requests before method checks, adds JSON-RPC error handling, and expands sandbox-agent validation tests. ChangesRunner MCP and relay behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 12, 2026
Merged
Closed
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Addresses the mechanical review findings on #5243, #5244, and #5245 that carry no design decision and need no live verification. Landed as one commit on top of the stack because GitButler cannot retro-assign a new delta onto a lower applied lane; each fix is mapped to its PR below. Relay (#5243): - relay.ts: bound every relay-file removal with a timeout so a hung daemon deletion can no longer wedge pickup, stale-sweep, or shutdown; a timed-out removal is treated as failed and the loop continues. - relay-watch.ts: tie a safety-poll miss to the active watch generation so a later zero-exit of the same generation cannot erase it (the demotion counter now accumulates); validate the whole watch-window string instead of parseInt accepting partial garbage. Shim (#5244): - tool-mcp-stdio.ts: answer MCP 'ping' with an empty result; return -32700 on unparseable input and -32600 on an invalid request; document the at-least-once relay delivery contract. Protocol-version negotiation is left unchanged (that needs a live check). - tool-mcp-assets.ts: cache the shim bundle by path and write bundle + specs concurrently, removing avoidable serial Daytona round trips. - restored the direct client-only Daytona refusal test; pinned the Pi vs non-Pi ask guard wiring; updated the build:extension docs. Continuation (#5245): - tool-mcp-http.ts: authenticate before the method check so an unauthenticated GET/DELETE gets 401, not 405. - added auth tests: unauthorized GET/DELETE, wrong scheme, empty and whitespace tokens, and that neither the callback executor nor the client relay runs on an unauthorized call. Deferred (need a design decision or a live check, not in this commit): the non-Pi ask authorization model (#5244), the bearer-in-argv delivery (#5245), MCP protocol-version negotiation (#5244). Claude-Session: https://claude.ai/code/session_0127AM79khCdvD2b8BG2joZL
dosubot
Bot
added
size:L
mmabrouk
added
lgtm
mmabrouk
force-pushed
the
feat-client-tool-continuation-v1
branch
from
6ab5d45 to
43ed55a
Compare
mmabrouk
force-pushed
the
feat-stack-review-fixes
branch
from
ba3e125 to
fff45dc
Compare
dosubot
Bot
added
the
size:L
mmabrouk
changed the base branch from
feat-client-tool-continuation-v1
to
big-agents
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.
Context
Three Codex reviews ran over the stack below (#5243, #5244, #5245). Each found a mix of real bugs, design questions, and small gaps. This PR lands only the fixes that are mechanical, carry no design decision, and need no live check. The design calls (the
askauthorization model, how the bearer token reaches the harness) and the one change that needs a live Claude run (MCP version negotiation) are left for you to decide on their own PRs.It sits on top of the stack as a separate PR for one reason: GitButler cannot move a new edit down onto an already-applied lower lane, and the tree is shared with other sessions right now, so retro-fitting each fix into its own feature PR was not safe. Every fix is mapped to its PR below, and the whole set merges with the stack.
Changes
Grouped by the PR whose review each fix answers.
Relay (#5243)
relay.ts: every relay-file removal now runs under a timeout. A hung DaytonamoveFs/delete used to block pickup, the stale sweep, and shutdown forever, and the 30s safety poll could never fire because the loop was stuck before it. A timed-out removal is now treated as a failed removal, kept in the retry set, and the loop keeps going.relay-watch.ts: a safety-poll miss is now tied to the watch generation that missed it, so a later clean exit of that same generation cannot erase it. Before this, repeated missed notifications could sit at one miss forever and never demote to polling. Also, the window-size fallback now rejects partial garbage ("30000junk"used to parse as30000).Shim (#5244)
tool-mcp-stdio.ts: answer MCPpingwith{}instead of a-32601error (a health-checking client could otherwise treat the shim as dead and tear it down). Return proper JSON-RPC errors for bad input (-32700for unparseable,-32600for an invalid request) instead of silently dropping it. Added a comment stating the at-least-once delivery contract. Version negotiation is unchanged on purpose (that needs a live Claude run).tool-mcp-assets.ts: cache the shim bundle by path and write the bundle and specs concurrently, instead of three serial Daytona round trips and a fresh disk read on every acquire.askguard wiring so a future change that flattened it would fail. Updated thebuild:extensiondocs to say it bundles both the Pi extension and the shim.Continuation (#5245)
tool-mcp-http.ts: authenticate before the method check, so an unauthenticatedGETorDELETEgets401rather than405. Added auth tests: missing token, wrong scheme, empty and whitespace-only tokens, and a check that an unauthorized call reaches neither the callback executor nor the client relay.Tests
cd services/runner && pnpm test && pnpm run typecheck. All new and changed tests pass. The four failures that remain are pre-existing on this stack and unrelated to these fixes:code-tool.test.ts,sandbox-agent-qa-transcript-replay(x2), andsandbox-agent-run-planhasApiKey.Not in this PR
Left for a decision or a live check, and called out on the feature PRs:
askauthorization model (feat(runner): deliver gateway tools to Claude on Daytona via an in-sandbox stdio MCP shim #5244): fail closed, or keep the current residual. A design call.https://claude.ai/code/session_0127AM79khCdvD2b8BG2joZL