feat(server): t3_thread_launch takes clientRequestId#16654

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Test receipt replay through the real launch service.

This test says a retry replays the first launch, but clientLaunchHarness mocks ThreadLaunchService.launch and returns the supplied ID on every call. The assertions therefore cannot detect a failure in receipt replay. Keep the ID-derivation assertions, but add a service-level test that accepts a launch and retries its command ID against the receipt store.

As per coding guidelines, “Tests exercise behavior through the service, with test layers only for external dependencies. Don't mock the logic under test.” The matching path instructions require testing behavior through the service.

Sources: Coding guidelines, Path instructions

✅ Confirmed as addressed by @juliusmarminge

Fixed. Added a real launch-service test: an agent's launch with the same key, from a narrower caller, cannot start work on the broader thread the first launch created. It runs through ThreadLaunchService.launch and the real orchestrator receipts, and fails without the in-lock check.

@juliusmarminge Thanks for the fix. The reported test exercises ThreadLaunchService.launch with real orchestrator receipts instead of mocking the replay path. It also checks that a narrower caller cannot start work on the broader thread.

✅ Review thread resolved.

You are interacting with an AI system.

🟠 High project/handlers.ts:74

A clientRequestId containing an unpaired surrogate (for example, "key-\ud800") throws URIError here, aborting the launch instead of returning an invalid_request failure. encodeURIComponent rejects lone surrogates; validate the ID before encoding or use an encoding that accepts arbitrary UTF-16.

Fixed in the takeover head: the caller/key tuple is JSON-encoded and SHA-256 hashed, so lone UTF-16 surrogates neither throw nor collide with U+FFFD. Fixed-length IDs also avoid long-key Scratch folder failures. The focused regression covers both keys, and the isolated HTTP MCP proof in the updated PR body verifies successful distinct launches and receipt replay.

The current head still uses encodeURIComponent, which throws on lone surrogates. The surrogate regression was also removed, so the original issue remains open.

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Move request-key ID derivation into the launch service.

The handler now defines how a request key maps to a launch command. Put that mapping in the domain service so other entrypoints use the same retry behavior. Keep the handler responsible for passing the decoded key and mapping typed errors.

As per coding guidelines, “A transport handler does three things: decode the request, call one service method, and map the service's typed errors to the transport's error. Nothing else.” The matching path instructions also place capability logic in the service.

Sources: Coding guidelines, Path instructions

Not changing. The handler is the only entry point that takes a client key; every other launch path already supplies its own command id. Moving the mapping would add a service parameter with one caller. The behavior the key protects (replay and the mode ceiling) does live in ThreadLaunchService.

@juliusmarminge Thanks for the clarification. The mapping adapts the MCP client key to the existing command-ID contract. It does not implement replay or the mode ceiling. Moving it would add an MCP-specific service parameter without a shared caller. I withdraw this finding.

✅ Review thread resolved.

You are interacting with an AI system.

🟠 High project/handlers.ts:73

With scratch:true, a valid long clientRequestId can make a launch fail with ENAMETOOLONG: the full key is embedded in commandId, and when folderForThread falls back to a full folder name after a short-name collision, that name can exceed the filesystem’s component limit. Derive the ID from a bounded digest of the caller and request key instead of embedding the complete key.

🟠 High project/handlers.ts:69

An identical retry of an accepted launch returns invalid_request instead of the original thread identity if its workspace changes: deleting the local base branch makes checkWorktreeBase reject a worktree retry, and removing the path makes the existing_worktree membership check reject it. These preflight checks run before launchThread can replay the accepted command; consult the accepted receipt before revalidating the original workspace, while still validating changed paths.

No longer relevant as of 071e6616495613844a1ff062446e41b4fc586d22