Skip to content

fix(client-runtime): subagent threads load over T3 Connect without two 401s - #17037

Open
SunkenInTime wants to merge 1 commit into
pingdotgg:mainfrom
SunkenInTime:t3/thread-http-dpop-url
Open

SunkenInTime wants to merge 1 commit into
pingdotgg:mainfrom
SunkenInTime:t3/thread-http-dpop-url

Conversation

@SunkenInTime

Copy link
Copy Markdown
Contributor

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_mismatch for 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.ts and threadHistoryHttp.ts signed a URL built by pasting the raw thread ID into the path. The request itself goes through Effect's HttpApiClient, which runs path parameters through encodeURIComponent. Delegated IDs come from IdAllocator.joinId and look like thread: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…. verifyDpopProof compares 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 builder pullRequestDiffHttp.ts already 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 its cursor query because the builder requires it. The web and mobile signers and the server all strip the query from htu, so the proof is unchanged. Server verification is untouched.

I checked the other signers. Session, shell, WebSocket ticket and /oauth/token have no path parameters, and the relay proofs in managedRelay.ts already 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 with vp 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 from POST /oauth/token with a one-time pairing token and a DPoP proof. "Before" reverts only the three source files.

Before:

token exchange 200 DPoP
bounded: failed EnvironmentAuthInvalidError in 41 ms
full: failed EnvironmentAuthInvalidError in 21 ms
history: failed EnvironmentAuthInvalidError in 31 ms
GET /api/orchestration/threads/thread%3Adelegated-task%3Acommand%253Amcp%253A…/bounded -> 401
GET /api/orchestration/threads/thread%3Adelegated-task%3Acommand%253Amcp%253A…/bounded -> 401
  401 body: {"_tag":"EnvironmentAuthInvalidError","code":"auth_invalid","reason":"invalid_credential","dpopFailureReason":"request_mismatch",…}
(snapshot and history: same two 401s each)

After:

token exchange 200 DPoP
bounded: ok in 168 ms
full: ok in 84 ms
GET /api/orchestration/threads/thread%3Adelegated-task%3Acommand%253Amcp%253A…/bounded -> 200
GET /api/orchestration/threads/thread%3Adelegated-task%3Acommand%253Amcp%253A… -> 200
GET /api/orchestration/threads/thread%3Adelegated-task%3Acommand%253Amcp%253A…/history -> 400

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.ts run each loader with a delegated ID, sign with a real P-256 key, and check the proof with verifyDpopProof from packages/shared/src/dpop.ts against the URL fetch received. Two existing cases now assert that the proof signs the exact requested URL.

$ cd packages/client-runtime
$ vp test run src/state/environmentHttpAuth.test.ts src/state/boundedThreadSnapshotHttp.test.ts src/state/pullRequestDiffHttp.test.ts
 Test Files  3 passed (3)
      Tests  43 passed (43)

With the three source files reverted, the three new cases fail with "ok": false from the verifier. tsc --noEmit in packages/client-runtime, vp lint and vp fmt --check on 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.

  • A real T3 Connect relay connection. That the relay forwards the encoded path unchanged comes from the Mac traces, where the server received the encoded path and logged url_mismatch.
  • A history 200 for a delegated thread with older pages.
  • The mobile signer at runtime. It strips the query with the same normalizeDpopHtu the server uses.

Found along the way, not fixed here. On Node 24.13.1, POST /oauth/token with a one-time pairing token returns 500 access_token_issuance_failed. AuthPairingLinks.consumeAvailable binds ${requestedScopes === undefined}, a JS boolean, and node:sqlite rejects 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 to 1 : 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

… 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>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 8, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-merge checks failed. Please resolve the failing checks before merging.

@coderabbitai

coderabbitai Bot commented Oct 8, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 7d8457bb-7176-4b09-9e85-d50f8b9f3b23
📥 Commits

Reviewing files that changed from the base of the PR and between d720210 and b99259c.

📒 Files selected for processing (4)
  • packages/client-runtime/src/state/boundedThreadSnapshotHttp.ts
  • packages/client-runtime/src/state/environmentHttpAuth.test.ts
  • packages/client-runtime/src/state/threadHistoryHttp.ts
  • packages/client-runtime/src/state/threadSnapshotHttp.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Thread request URL construction

Layer / File(s) Summary
Build thread request URLs
packages/client-runtime/src/state/boundedThreadSnapshotHttp.ts, packages/client-runtime/src/state/threadHistoryHttp.ts, packages/client-runtime/src/state/threadSnapshotHttp.ts
The three request functions use the orchestration API URL builder to construct thread URLs. The history request passes its cursor as a query parameter.
Verify proofs against request URLs
packages/client-runtime/src/state/environmentHttpAuth.test.ts
Authentication tests verify proofs against captured request URLs and check percent encoding of delegated thread IDs.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to b9925

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 Review

Security architecture risk: 🔵 Low · up to b9925

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected outcome is reading thread snapshots and history from the selected environment under the existing orchestration read scope. Correctly encoded delegated IDs become usable over HTTP, but the comparison shows no new production route, broader credential scope, or alternate authorization path. Per-thread policy intent remains outside the established evidence.

Trust Boundaries and Controls

  • observed — Production verification derives the method and URL from the received HTTP request, validates the proof against the expected key and access token, and records replay markers. These controls are unchanged. The added test uses real ES256 signatures but does not provide expected token/key values or exercise production replay storage, so it proves URL/signature compatibility rather than complete production authentication.

Resilience and Maintainability Implications

  • observed — The unchanged authentication loop rebuilds the URL and creates a fresh proof after resolving renewed credentials and origin, permits only one credential-renewal retry, and retains the outer timeout. Shared renewal remains service-owned with pending-state cleanup. Source inspection found no introduced identity, ordering, or cleanup violation; combined delegated-ID retry and concurrent-loader cancellation tests remain absent from the inspected coverage.

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore (reviewers only)

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Approvability ❌ Error The pull request matches the rule "Changes authentication". It changes the URL used by executeAuthenticatedEnvironmentHttpRequest and the DPoP signer in `packages/client-runtime/src/state/threadSnap… This pull request needs a maintainer's review before CodeRabbit approves it.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the client-runtime fix for delegated subagent threads that caused two 401 responses over T3 Connect.
Description check ✅ Passed The description includes all required sections. It explains the failure, the URL-encoding cause, the contract-builder fix, the scope, detailed verification results, limitations, and unrelated findings…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Approvability

Explanation

The pull request matches the rule "Changes authentication". It changes the URL used by executeAuthenticatedEnvironmentHttpRequest and the DPoP signer in packages/client-runtime/src/state/threadSnapshotHttp.ts, packages/client-runtime/src/state/boundedThreadSnapshotHttp.ts, and packages/client-runtime/src/state/threadHistoryHttp.ts. The added tests verify real DPoP proofs against the received request URLs. This changes authentication behavior for delegated thread requests.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

aliceisjustplaying added a commit to aliceisjustplaying/t3code that referenced this pull request Oct 8, 2026
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>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant