feat(claude): reauthenticate failed V2 threads - #9179

1. browse_code — apps/server/src/provider/claudeAuthFlow.ts

2. browse_code — apps/server/src/provider/claudeAuthentication.ts

💭 Good, claudeAuthFlow.ts now matches conventions with inline interface, make/layer pattern, and proper service typing. Now I'm checking whether the makeError helper violates the rule against trivial error-construction wrappers—since it just builds the error object with no real normalization logic, it should probably be removed in favor of constructing errors directly at the failure site. The convention allows mappers that add reusable context, so this one probably passes. Looking at ServerProviderReauthenticateError's fields — provider, reason as a trimmed string, optional cause — the reason is really a message string rather than a discriminator literal, so it likely doesn't trip the double-encoding rule. I want to check whether existing sibling errors like ServerProviderUpdateError follow this same pattern before flagging anything, then move on to checking providerMaintenanceRunner. ts for how it acquires dependencies and constructs its layer, confirming the ClaudeAuthFlow dependency issue I noted earlier is resolved. I'm also scanning for other convention violations like errors missing cause, misuse of catchTag, or Effect.option silently swallowing failures in the runner and Orchestrator changes. Same question applies to the OrchestratorV2 import — these are named imports of service tags, not aliasing, so it's not quite the same violation as the flagged alias pattern, but it might still be a deviation from the namespace-import convention. I should check how existing files in this codebase actually import these services to see if this pattern is already the norm.

3. browse_code — apps/server/src/provider/providerMaintenanceRunner.ts

4. browse_code — apps/server/src/provider/providerMaintenanceRunner.ts

💭 The pre-existing ProviderMaintenanceRunnerShape interface predates the convention against standalone Shape interfaces, so I won't flag it since the PR just extends it. Now I'm checking how other consumers import ServerSettingsService and OrchestratorV2 to see if the named imports here are consistent.

5. browse_code — How do production (non-test) modules in apps/server/src import the ServerSettingsService tag from serverSettings.ts and the OrchestratorV2 tag from orchestration-v2/Orchestrator.ts? Show a few representative import statements (named vs namespace imports).

💭 That named import style is standard practice here, so nothing to flag. Now I'm checking the Orchestrator.ts changes, verifying that layerUnavailable follows the existing pattern of failing prepareReauthentication with a placeholder threadId and cause, and that continueAfterReauthentication's use of OrchestratorDispatchError with the CommandId is consistent with how other members are implemented. I'm looking at whether mapping runtimePolicy.resolve failures into OrchestratorProjectionError misrepresents the actual stage where the failure occurs, versus wrapping a whole pipeline in one generic error — this could be a real convention violation, but I want to compare it against other spots in Orchestrator that map runtimePolicy.resolve failures before flagging it.

6. browse_code — In apps/server/src/orchestration-v2/Orchestrator.ts, how are failures from runtimePolicy.resolve(...) mapped elsewhere in the file (which error class is used, e.g. OrchestratorProjectionError vs another)? Also list the error classes defined for the orchestrator (OrchestratorProjectionError, OrchestratorDispatchError, etc.) with their fields.

💭 I'm second-guessing whether wrapping a runtimePolicy.resolve failure as OrchestratorProjectionError is actually a mislabel — the naming suggests a projection-store issue when it's really a RuntimePolicyResolveError, which conflicts with the convention that errors carry structured operation/stage attributes. Given the file's own conventions constrain the error union to six classes with no better fit for a non-dispatch path, I'm leaning toward not flagging this since it's a weak, low-severity finding rather than a clear convention violation. I'm now scanning other areas of the Orchestrator diff for similar issues — the deterministic message id in continueAfterReauthentication versus freshly allocated command id doesn't seem like an Effect convention concern, and the providerSessions.release error mapping to OrchestratorDispatchError with a synthetic CommandId looks consistent with existing patterns. Next I want to check serverSettings.ts, specifically whether the new withSettingsLock field fits cleanly into the existing Context.Service interface shape around line 199.

7. browse_code — apps/server/src/serverSettings.ts

💭 Checking ws.ts and server.ts, the changes look like minor wiring—moving ProviderMaintenanceRunner.layer into RuntimeCoreDependenciesBaseLive, which doesn't conflict with the per-feature runtime convention. In ClaudeAdapterV2.ts, providerFailureFromResult is now exported for testing purposes. I'm also checking supportsClaudeSubscriptionLogin, which is a pure helper with a fine named import, and its export purely for tests doesn't clash with any stated convention. claudeReauthentication.ts is pure too, and the state/server.ts atom additions look fine. In contracts/server.ts, there's a new ServerProviderReauthenticateError class with provider, reason, and an optional cause field, deriving its message from provider and reason—this is fine since cause being optional accounts for cases with no underlying failure. Now I'm looking at claudeAuthFlow.ts for potential convention violations, particularly around a trivial makeError helper that just wraps arguments into a new error—checking whether it does real normalization or just constructs the error redundantly. The makeError helper fixes provider to CLAUDE_DRIVER, which arguably counts as adding reusable context rather than being a bare passthrough constructor, though the file also constructs the same error type directly elsewhere with a cause. I'm inclined to treat this as a soft finding not worth flagging, since the "adds reusable context" exemption seems to apply. I'm also checking the cause field usage in getAttempt's error at line 345—no underlying failure, which is fine for a domain error—and noting the catchCause patterns that swallow causes, which aren't explicitly addressed by the stated conventions beyond the guidance on catching statically known tagged failures. The intentional swallowing of causes in fire-and-forget spots, along with watch's catchCause block that mostly does nothing except check for interrupts, seems fine even though it makes the Cause import barely used—none of this maps to a listed convention violation, and the deliberate dropping of cause aligns with keeping error context safe and bounded. Non-null assertions on state.child and the mutable AuthAttemptState object mutated across fibers instead of using a Ref also don't seem to violate anything on the list, since the mutation is local to make rather than a module-level global. Both diagnostic-disabling directives have adjacent explanatory comments, satisfying that rule. Now I'm checking whether make yielding Scope.Scope from its environment interacts safely with Layer.effect(ClaudeAuthFlow, make)—wondering if this Effect version's Layer.effect actually threads through the scope requirement or if it leaks into the resulting layer's requirements. I recall that in v4, Layer builds always happen within a scope, so Layer.effect should have access to it. Looking at the convention that real construction logic belongs in a genuine make function rather than a fake one just to satisfy Layer.effect, and noting other services in the repo like ClaudeAuthFlow and ProviderMaintenanceRunner also use Layer.effect this way, I conclude the pattern is consistent and since CI typechecks passed, it's fine as written. Now I'm narrowing down the in-scope changed files, excluding tsx files since the glob patterns only match .ts: the relevant list includes RpcAuthorization, ClaudeAdapterV2, Orchestrator, claudeAuthFlow, claudeAuthentication, providerMaintenanceRunner, server.ts, and serverSettings.ts. I'm also checking the client-runtime and contracts packages for related changes, plus a test fixture file that's technically in scope though it's assertion-only. Next I want to look at how ClaudeAdapterV2 handles the firstPartyOAuth gate and its capability checks. The subscription login helper and provider failure function both look like straightforward exported utilities, nothing concerning there. Now I'm verifying whether providerMaintenanceRunner.ts still exports both layer and the earlier layerWithThreadContinuation, and checking that removing the layer provision from ws.ts and adding it to server.ts stays consistent across the RuntimeCoreDependenciesBase setup. Since CI passed, the OrchestratorV2 dependency must already be satisfied in Live, so I want to double-check the remaining section of providerMaintenanceRunner.ts and the relevant part of ws.ts just to be thorough.

8. browse_code — apps/server/src/provider/providerMaintenanceRunner.ts

💭 These look conventionally fine on the surface. But I'm noticing that wrapping the config resolution with Effect.option swallows the typed failure entirely, including transient settings-read failures, treating any error as if config simply changed and was skipped — that's a real concern about losing failure information rather than an imperative bypass issue. Still, since the whole error channel is being handled here, this arguably satisfies the convention that catching is fine when the entire error channel is addressed, so it's borderline rather than a clear violation. I should check the Orchestrator diff around lines 7212-7370 to see if there's anything else worth flagging there.

9. browse_code — apps/server/src/orchestration-v2/Orchestrator.ts

💭 These match repo patterns. Now I'm checking the layerUnavailable additions in Orchestrator—verifying prepareReauthentication throws the OrchestratorProjectionError with the sentinel threadId and the expected cause string consistently with existing entries in that layer.

10. browse_code — apps/server/src/orchestration-v2/Orchestrator.ts

💭 The OrchestratorV2Shape and claudeAuthentication.ts additions look consistent with existing patterns, and the state atom factories in server.ts seem fine too. I'm now checking ws.ts to see what happened with the provider maintenance runner removal. Removing Layer.provide(ProviderMaintenanceRunner.layer) from websocketRpcRouteLayer and relying on RuntimeCoreDependenciesBaseLive instead aligns with the convention against per-feature managed runtimes. There's no dependency issue between ClaudeAuthFlow and ws either. I'm weighing whether claudeAuthFlow.ts's Scope acquisition pattern with Layer.effect is a concern, but in Effect 4 that's probably fine, and the state mutation via Ref map isn't covered by the convention anyway. The more interesting question is makeError, which just wraps constructing a ServerProviderReauthenticateError — I need to decide if that counts as the kind of trivial constructor-wrapping helper the convention warns against. Since this file existed before the PR and prior review passes already went over it heavily without flagging this pattern, I'll treat it as borderline and skip it. Similarly, extending the existing ProviderMaintenanceRunnerShape with new members isn't the same as introducing a new standalone shape, so that's not a finding either — older code predating the convention shouldn't trigger repo-wide cleanup demands. The claudeAuthFlow.ts service tag and namespace import naming look fine too. Checking the ordering within claudeAuthFlow.ts against convention (imports, schemas, tag, make, layer) — helpers appear before the tag which is a minor deviation, not worth reporting. Tests look sufficient given the behavior changes. I'm now scanning for any newly added disabling directives like eslint-disable, @ts-expect-error, or @effect-diagnostics comments in the diff — the two I found in claudeAuthFlow.ts both include explanations, and I don't see any unexplained ones elsewhere in the changed lines, aside from a non-null assertion without a directive. Now I want to check the ClaudeAdapterV2 changes as well. The fixture output.ts change is just a testkit assertion update, not a service convention issue. Checking the new atoms in client-runtime's server.ts, they follow existing repo patterns for RPC command/query atom families, and the import in server.ts adding ProviderMaintenanceRunner.layer looks correctly wired. There's one more thing I want to verify before wrapping up this pass. I'm wondering whether providerMaintenanceRunner's layer is being provided in more than one place now — ws.ts removed it while server.ts adds it, but if it's also constructed elsewhere like serverRuntimeStartup or http routes, that could create duplicate instances with separate attempt state rather than a shared one. Layer memoization should handle this within a single composition, but I want to search for other places that provide ProviderMaintenanceRunner.layer to be sure.

11. browse_code — Where is ProviderMaintenanceRunner.layer (from apps/server/src/provider/providerMaintenanceRunner.ts) provided/composed in production code? List every Layer.provide / mergeAll site and whether those compositions are memoized within one runtime (i.e. could two distinct ProviderMaintenanceRunner instances be built, each with its own ClaudeAuthFlow state)?

💭 The wiring checks out fine, and prior findings have been resolved with no new convention violations introduced. Since there's nothing to flag, I'll record this as "All clear" through the complete_check mechanism, keeping the summary concise to match that verdict.