Repository navigation
fix(client-runtime): subagent threads load over T3 Connect without two 401s - #17037
SunkenInTime wants to merge 1 commit into
Conversation
… are sent Thread snapshot, bounded snapshot, and history loaders built the DPoP proof URL with the raw thread ID, while the HttpApi client percent-encodes the path parameter. Delegated thread IDs contain ":" and "%3A", so the proof never matched and every open failed twice with 401 before falling back to the socket. Build the signed URL with the contract URL builder, which uses the same path encoding as the request. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This is a narrowly scoped fix that aligns DPoP proof URLs with encoded delegated-thread request URLs and adds targeted tests. It changes production authentication behavior, so a human review is required. You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe thread snapshot, bounded snapshot, and history requests now use the orchestration API URL builder. Authentication tests verify DPoP proofs against captured request URLs, including requests with percent-encoded delegated thread IDs. ChangesThread request URL construction
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The URL changes address the reported authentication mismatch, with no actionable issue identified in the reviewed change. Normal checks remain appropriate before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change fixes proof-to-request URL mismatches without an identified authorization bypass or increase in credential authority. Server authentication and read-scope checks remain intact. End-to-end relay handling and delegated-thread requests during credential renewal remain incompletely verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
Full details: ApprovabilityExplanation The pull request matches the rule "Changes authentication". It changes the URL used by
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Keep upstream migration IDs 59 and 60; move YSK to 61 and repair historical fork ledgers atomically after applying missing schema changes. Adapt selected upstream fixes for Pi approvals, MCP images, workspace discovery, provider worker starvation, concurrent worktree launches, primary runtime ownership, session refresh, DPoP URLs and embedded-render scrolling. Preserve fork lifecycle and notice behavior, and close the approval and authentication gaps found in review. Adapted from pingdotgg#16854, pingdotgg#15879, pingdotgg#17190, pingdotgg#16790, pingdotgg#17197, pingdotgg#17172, pingdotgg#17075, pingdotgg#17037, pingdotgg#17065. Co-authored-by: Adamulek123 <adam.bogucki2018@gmail.com> Co-authored-by: anntnzrb <anntnzrb@proton.me> Co-authored-by: Wout Stiens <71498452+StiensWout@users.noreply.github.com> Co-authored-by: Lakshmi Tanmay <lakshmi@voltcrash.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Co-authored-by: Jake Leventhal <jakeleventhal@me.com> Co-authored-by: Malte Sussdorff <malte.sussdorff@cognovis.de> Co-authored-by: sheehanmunim <sheehanmunim@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Dara Adedeji <daraadedeji07@gmail.com> Co-authored-by: Joseph Vidal <josephv4000@gmail.com>
Problem
Over T3 Connect, opening a subagent (delegated) thread fails the HTTP snapshot fetch with two 401s and then falls back to the socket. On a Mac desktop connected through the relay, four such opens lost 610 ms, 880 ms, 990 ms and 1,680 ms this way. The server logged
environment.dpop.failure_code=url_mismatchfor each 401. Tracked in #15772.This is the same source change as #15813, which I found after writing it. The three source files are byte-identical. This PR adds a live before/after against a running server and a test that checks real ES256 proofs with the server's own verifier, which #15813 lists as unchecked. If #15813 lands first, close this one or take the test from it.
Cause
threadSnapshotHttp.ts,boundedThreadSnapshotHttp.tsandthreadHistoryHttp.tssigned a URL built by pasting the raw thread ID into the path. The request itself goes through Effect'sHttpApiClient, which runs path parameters throughencodeURIComponent. Delegated IDs come fromIdAllocator.joinIdand look likethread:delegated-task:command%3Amcp%3A…, so the proof signs/threads/thread:delegated-task:command%3Amcp…while the request goes to/threads/thread%3Adelegated-task%3Acommand%253Amcp….verifyDpopProofcompares the two exactly and rejects the request. The token-renewal retry signs the same wrong URL, which is why each open costs two 401s. UUID thread IDs encode to themselves, so normal threads never hit this.Change
The three loaders now build the signed URL with
makeEnvironmentHttpApiUrlBuilder, the contract-derived builderpullRequestDiffHttp.tsalready uses. It runs the same path compiler as the request, so the signed and sent URLs come from one source. The history URL now carries itscursorquery because the builder requires it. The web and mobile signers and the server all strip the query fromhtu, so the proof is unchanged. Server verification is untouched.I checked the other signers. Session, shell, WebSocket ticket and
/oauth/tokenhave no path parameters, and the relay proofs inmanagedRelay.tsalready use the contract builder.Scope and approval
Bug fix for #15772, which triage confirmed on main. One problem: the URL the client signs for thread loads differs from the URL it sends.
Verification
Live run, before and after. Windows 11, Node 24.13.1. Dev server from this branch (
vp run dev:server), seeded withvp run migrate-dev-db, so the thread is a real delegated thread:thread:delegated-task:command%3Amcp%3Aea546c22-4ee9-4a85-a96f-af18fab3c3f1%3Adelegate-task%3Aclaude-wake-replay-review-r1. The client side is the real client-runtime loaders with a WebCrypto ES256 signer. The DPoP-bound access token comes fromPOST /oauth/tokenwith a one-time pairing token and a DPoP proof. "Before" reverts only the three source files.Before:
After:
The history 400 is expected: this thread has no older page, so I sent a placeholder cursor. It shows the request got past DPoP authentication, where before it got 401.
Focused tests. The new cases in
environmentHttpAuth.test.tsrun each loader with a delegated ID, sign with a real P-256 key, and check the proof withverifyDpopProoffrompackages/shared/src/dpop.tsagainst the URLfetchreceived. Two existing cases now assert that the proof signs the exact requested URL.With the three source files reverted, the three new cases fail with
"ok": falsefrom the verifier.tsc --noEmitinpackages/client-runtime,vp lintandvp fmt --checkon the changed files are clean. GPT-6 Astra reviewed the diff, found no blockers, and independently reproduced the 3 failures without the fix.Not checked.
url_mismatch.normalizeDpopHtuthe server uses.Found along the way, not fixed here. On Node 24.13.1,
POST /oauth/tokenwith a one-time pairing token returns 500access_token_issuance_failed.AuthPairingLinks.consumeAvailablebinds${requestedScopes === undefined}, a JS boolean, andnode:sqliterejects booleans ("Provided value cannot be bound to SQLite parameter 1"). The binding came in with #10298. For the live run above I patched it locally to1 : 0; that patch is not in this PR.Implemented with Claude Opus 5.5 in Claude Code (T3 Code), reviewed by GPT-6 Astra.
🤖 Generated with Claude Code