🤖 refactor: convert providerService mutation internals to Effect

[ThomasK33]

Summary

Phase 2b of the progressive Effect migration: converts the mutation internals of ProviderService to Effect while keeping the public API and observable behavior byte-identical via thin Effect.runPromise facades. All 105 existing providerService.test.ts tests pass unchanged, and the provider oRPC mutation procedures now ride handlerGen so client aborts interrupt the fiber.

Background

Follows #4022 (Effect v4-rc + oRPC 1.14 spike, effectBridge/handlerGen), #4025 (memory subsystem), and #4027 (retryManager + muxGatewayOauthService). Same house pattern: Effect.gen internals, Schema.TaggedError only where callers genuinely branch, Promise facades so pre-Effect callers (codexOauthService, coderOauthService, muxGatewayOauthService, tests) stay untouched. Phase 2c owns providerModelFactory.ts; streamManager.ts placement is decided at Phase 4.

Implementation

Converted pipelines (each is an Effect.gen program; the wire Result unions stay in the success channel exactly as before):

• addCustomProvider / removeCustomProvider (public *Effect variants exported for the router)
• setModels / setConfig (public *Effect variants exported for the router)
• setConfigValue / updateConfigValue / updateProviderSection (private *Effect internals; Promise facades remain the entry points for the OAuth services)
• syncGatewayLifecycle → syncGatewayLifecycleEffect: the synchronous config/policy read now computes the main-config edit to apply (or null) inside one guarded Effect.try, followed by a guarded editConfig
• afterAppliedMutation → afterAppliedMutationEffect: best-effort semantics preserved via Effect.catch + log.error

Error tags: exactly one — ProviderPersistenceError { message }, carrying the getErrorMessage(cause) string. No caller branches on which write failed, only on the folded wire Result, so per-method Effect.catchTag folds reproduce the old per-method try/catch strings (persistence_failed codes, Failed to set models: …, Failed to set/update provider config: …). removeCustomProvider folds twice at different pipeline points because its lock failure (persistence_failed) and config-repair failure (config_repair_failed, with the notify-before-return side effect) carry different wire codes — matching the original two try/catch blocks.

Deliberately not converted:

• Sync read paths (list, getConfig, validateRouteOverrides): no async composition, so Effect conversion would add fiber overhead without benefit (noted in the module doc comment).
• Watcher/emitter plumbing (onConfigChanged, fingerprint self-write suppression): not a fallible pipeline.
• updateRoutePreferences: thin delegation to Config.
• providers.onConfigChanged subscription: event iterator, awaits an Effect Stream bridge (existing backlog item).
• OAuthFlowManager: shared by 4 OAuth services; its Scope conversion stays deferred (per 🤖 refactor: convert retryManager and muxGatewayOauthService internals to Effect #4027).

Router: addCustomProvider, removeCustomProvider, setProviderConfig, setModels now delegate through handlerGen to the fully-handled (Effect<A, never>) service effects — one-line return yield* handlers, wire contracts unchanged.

Security note: key material handling is unchanged — the tagged error carries only getErrorMessage(cause) strings (the exact strings the old catch blocks already surfaced), and no secrets are added to logs or error payloads.

Validation

• make static-check green locally.
• Targeted suites, all passing unchanged: providerService.test.ts (105), providerModelFactory.test.ts + muxGatewayOauthService.test.ts (138), agentSession.preStreamError + agentSession.startupAutoRetry (57), streamManager.test.ts (119), coderOauthService + codexOauthService (102).
• Error-injection tests (read-only disk, failing editConfig mock, throwing lock callbacks) exercise the new Effect.tryPromise/Effect.try error channels and pass unchanged, including the notify-once-on-repair-failure ordering assertion.

Risks

Medium-touch refactor of the single service that owns providers.jsonc writes (credentials, models, routePriority). Mitigations: the wire Result shapes, lock scoping, notify ordering, and best-effort post-write semantics were preserved statement-for-statement; the 105-test suite covers every mutation path including policy revalidation inside the lock, gateway lifecycle sync, and partial-failure orderings. Highest residual risk is subtle rejection-value changes on previously-unguarded sync throws (now surfaced as FiberFailure via runPromise) — these paths were already crash paths with no callers branching on the rejection value.

Lessons for Phase 2c (providerModelFactory.ts, ~2953 lines)

• One persistence tag is enough when folds differ only in message: callers branch on the folded wire shape, not the tag. Study the catch sites first; providerService needed exactly 1 tag across 7 pipelines. Expect providerModelFactory to be similar — it mostly reads config and constructs SDK model instances, so its error taxonomy is probably Result<_, string> all the way.
• Sync-plan/async-commit split works well: syncGatewayLifecycleEffect wraps the synchronous decision logic in one Effect.try that returns the async edit to apply (or null). This preserves comments and logic verbatim while giving clean yield points. Useful for factory functions that compute options synchronously then await credential resolution.
• Effect.try around reload+notify blocks: when an old try/catch covered trailing sync work (reload, emitter notify), wrap that segment in a single Effect.try returning the wire result rather than splitting into multiple yields — smaller diff, identical coverage.
• Async thunks in Effect.tryPromise remain essential: spyOn(...).mockImplementationOnce(() => { throw ... }) produces synchronous throws from Promise-typed methods; try: async () => … routes them into the error channel exactly like the old await. Two providerService tests depend on this.
• Facades can drop async: public foo(): Promise<T> { return Effect.runPromise(this.fooEffect()); } passes lint here and avoids require-await debates.
• Sync read paths are not worth converting (fiber overhead, no composition win) — document the decision in the module doc so reviewers don't flag omission.
• providerModelFactory consumes ProviderService via sync getConfig()/loadProvidersConfig() reads plus async credential resolution — the Effect seam should sit at credential/token fetch boundaries (resolveProviderCredentials, OAuth token refresh), not at the sync config plumbing.

---

Generated with xum • Model: anthropic:claude-fable-5 • Thinking: xhigh • Cost: $9.99

[ThomasK33]

@codex review

[ThomasK33]

@codex security review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 412dcec3d7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[ThomasK33]

@codex review

Addressed the atomicity finding: mutation pipelines are now uninterruptible (asAtomicMutation) with a red/green interruption test. Please take another look.

[ThomasK33]

@codex security review