Skip to content

Make server settings authoritative for runtime behavior#1421

Merged
juliusmarminge merged 30 commits intomainfrom
t3code/server-settings-authority
Mar 26, 2026
Merged

Make server settings authoritative for runtime behavior#1421
juliusmarminge merged 30 commits intomainfrom
t3code/server-settings-authority

Conversation

@juliusmarminge
Copy link
Member

@juliusmarminge juliusmarminge commented Mar 25, 2026

Closes #1033
Closes #1289

Summary

  • Moves provider startup and assistant streaming decisions to server-owned settings instead of client-passed state.
  • Adds a persistent serverSettings service with disk-backed load/save, change notifications, and WebSocket exposure.
  • Updates orchestration and runtime ingestion to read authoritative settings when starting sessions and processing assistant output.
  • Splits client-only settings from server settings and keeps the existing app settings hook as a compatibility shim.
  • Extends tests and harnesses to cover server settings wiring and streaming behavior.

Testing

  • Not run (PR content only).
  • Existing test updates in the diff cover ProviderRuntimeIngestion, ProviderCommandReactor, and main wiring.
  • Project lint/typecheck/fmt were not run in this session.

Note

Medium Risk
Changes core runtime behavior by moving provider startup parameters, text-generation model selection, and assistant streaming decisions into persisted server settings; misconfiguration or settings migration issues could affect provider launch and message delivery mode.

Overview
Makes server-owned settings the source of truth for runtime behavior. Introduces ServerSettingsService (disk-backed settings.json, validation, change stream, debounced file watching) and wires it into server startup and test harnesses.

Provider and git text-generation flows now read from server settings instead of client-supplied fields or hard-coded defaults: Codex/Claude CLI binaryPath/homePath are pulled from settings, GitManager/ProviderCommandReactor use the settings’ textGenerationModelSelection, and assistant delta delivery switches between buffered/streaming based on enableAssistantStreaming.

Replaces the old startup-time provider health layer with a live ProviderRegistry backed by new CodexProvider/ClaudeProvider managed-provider layers (status/version/auth probing + model capability/normalization helpers), and updates keybindings/config writing to be more robust (lenient JSON parse, atomic write cleanup, debounced watch events).

Written by Cursor Bugbot for commit 21838dc. This will update automatically on new commits. Configure here.

Note

Make server settings authoritative for provider config, model selection, and assistant delivery mode

  • Introduces a ServerSettingsService with persistent JSON storage, deep-merge patch semantics, and a change stream; settings are now the authoritative source for provider binary paths, model selection, and assistant streaming mode.
  • Adds ProviderRegistry aggregating Codex and Claude snapshots (installation status, version, auth, models) with periodic refresh and change broadcasting; replaces the old ProviderHealth service.
  • Stacked actions (GitManager), branch name generation (ProviderCommandReactor), and assistant delivery mode (ProviderRuntimeIngestion) now read configuration from server settings instead of request payloads.
  • Removes providerOptions and assistantDeliveryMode from orchestration contracts (ProviderSessionStartInput, ThreadTurnStartCommand, GitRunStackedActionInput); callers can no longer override these per-request.
  • Adds a useSettings/useUpdateSettings hook pair in the web client that merges server and client settings and routes patches to the correct backing store.
  • The Settings UI is overhauled to show live per-provider cards (status, version, enable/disable switch, binary path inputs, model list with capability tooltips) sourced from server provider snapshots.
  • Risk: resolveModelSlug no longer falls back to a provider default when the slug is unknown; it returns the normalized slug unconditionally, which may surface unresolved slugs where callers previously received a safe default.

Macroscope summarized 21838dc.

- Move settings persistence and validation into the server
- Route runtime/provider behavior through server settings
- Update client and tests to use the unified settings flow
@github-actions github-actions bot added size:XL 500-999 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Mar 25, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 25, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 40249626-4df1-4bfe-9580-777f04448241

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/server-settings-authority

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

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Decider test asserts removed event field
    • Removed the assertion for assistantDeliveryMode in the test since the decider no longer emits that field in the thread.turn-start-requested event payload.
  • ✅ Fixed: Stale dependency in useCallback after removing usage
    • Removed settings.enableAssistantStreaming from the onSendOrRetry useCallback dependency array since it is no longer referenced in the callback body.

Create PR

Or push these changes by commenting:

@cursor push e35da91f49
Preview (e35da91f49)
diff --git a/apps/server/src/orchestration/decider.projectScripts.test.ts b/apps/server/src/orchestration/decider.projectScripts.test.ts
--- a/apps/server/src/orchestration/decider.projectScripts.test.ts
+++ b/apps/server/src/orchestration/decider.projectScripts.test.ts
@@ -188,7 +188,6 @@
     if (turnStartEvent?.type !== "thread.turn-start-requested") {
       return;
     }
-    expect(turnStartEvent.payload.assistantDeliveryMode).toBe("buffered");
     expect(turnStartEvent.payload).toMatchObject({
       threadId: ThreadId.makeUnsafe("thread-1"),
       messageId: asMessageId("message-user-1"),

diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -2972,7 +2972,6 @@
       selectedProvider,
       setComposerDraftInteractionMode,
       setThreadError,
-      settings.enableAssistantStreaming,
       selectedModel,
     ],
   );

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Empty string binary path breaks CLI command spawning
    • Fixed by guarding deriveProviderStartOptions on settings.claude.binaryPath being truthy (not just the object) and switching from ?? to || for binaryPath fallbacks in ClaudeTextGeneration and CodexTextGeneration so empty strings correctly fall back to default CLI names.
  • ✅ Fixed: Migration skips old flat binary path keys
    • Added explicit migration of old flat localStorage keys (claudeBinaryPath, codexBinaryPath, codexHomePath) into the new nested codex/claude objects in migrateLocalSettingsToServer.

Create PR

Or push these changes by commenting:

@cursor push f8c9d71187
Preview (f8c9d71187)
diff --git a/apps/server/src/git/Layers/ClaudeTextGeneration.ts b/apps/server/src/git/Layers/ClaudeTextGeneration.ts
--- a/apps/server/src/git/Layers/ClaudeTextGeneration.ts
+++ b/apps/server/src/git/Layers/ClaudeTextGeneration.ts
@@ -95,7 +95,7 @@
 
       const runClaudeCommand = Effect.gen(function* () {
         const command = ChildProcess.make(
-          claudeSettings?.binaryPath ?? "claude",
+          claudeSettings?.binaryPath || "claude",
           [
             "-p",
             "--output-format",

diff --git a/apps/server/src/git/Layers/CodexTextGeneration.ts b/apps/server/src/git/Layers/CodexTextGeneration.ts
--- a/apps/server/src/git/Layers/CodexTextGeneration.ts
+++ b/apps/server/src/git/Layers/CodexTextGeneration.ts
@@ -153,7 +153,7 @@
         const reasoningEffort =
           modelSelection.options?.reasoningEffort ?? CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT;
         const command = ChildProcess.make(
-          codexSettings?.binaryPath ?? "codex",
+          codexSettings?.binaryPath || "codex",
           [
             "exec",
             "--ephemeral",

diff --git a/apps/server/src/serverSettings.ts b/apps/server/src/serverSettings.ts
--- a/apps/server/src/serverSettings.ts
+++ b/apps/server/src/serverSettings.ts
@@ -97,7 +97,7 @@
           },
         }
       : {}),
-    ...(settings.claude
+    ...(settings.claude?.binaryPath
       ? {
           claudeAgent: {
             binaryPath: settings.claude.binaryPath,

diff --git a/apps/web/src/hooks/useSettings.ts b/apps/web/src/hooks/useSettings.ts
--- a/apps/web/src/hooks/useSettings.ts
+++ b/apps/web/src/hooks/useSettings.ts
@@ -159,6 +159,33 @@
       }
     }
 
+    // Migrate old flat binary-path keys into their new nested shape
+    const codexBinaryPath = old["codexBinaryPath"];
+    const codexHomePath = old["codexHomePath"];
+    const claudeBinaryPath = old["claudeBinaryPath"];
+
+    if (
+      (typeof codexBinaryPath === "string" && codexBinaryPath) ||
+      (typeof codexHomePath === "string" && codexHomePath)
+    ) {
+      const existing = (serverPatch["codex"] as Record<string, unknown> | undefined) ?? {};
+      serverPatch["codex"] = {
+        ...existing,
+        ...(typeof codexBinaryPath === "string" && codexBinaryPath
+          ? { binaryPath: codexBinaryPath }
+          : {}),
+        ...(typeof codexHomePath === "string" && codexHomePath ? { homePath: codexHomePath } : {}),
+      };
+    }
+
+    if (typeof claudeBinaryPath === "string" && claudeBinaryPath) {
+      const existing = (serverPatch["claude"] as Record<string, unknown> | undefined) ?? {};
+      serverPatch["claude"] = {
+        ...existing,
+        binaryPath: claudeBinaryPath,
+      };
+    }
+
     if (Object.keys(serverPatch).length > 0) {
       void ensureNativeApi()
         .server.updateSettings(serverPatch as ServerSettingsPatch)

- Move Codex and Claude config lookups to `settings.providers`
- Update web composer, settings UI, and model selection to use unified settings
- Refresh browser fixtures and contracts for the new settings shape
@github-actions github-actions bot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Mar 25, 2026
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Empty string binary path not handled by nullish coalescing
    • Replaced ?? with || in both ClaudeTextGeneration.ts and CodexTextGeneration.ts so that an empty string binaryPath correctly falls back to the default CLI name.

Create PR

Or push these changes by commenting:

@cursor push f74ee02af1
Preview (f74ee02af1)
diff --git a/apps/server/src/git/Layers/ClaudeTextGeneration.ts b/apps/server/src/git/Layers/ClaudeTextGeneration.ts
--- a/apps/server/src/git/Layers/ClaudeTextGeneration.ts
+++ b/apps/server/src/git/Layers/ClaudeTextGeneration.ts
@@ -95,7 +95,7 @@
 
       const runClaudeCommand = Effect.gen(function* () {
         const command = ChildProcess.make(
-          claudeSettings?.binaryPath ?? "claude",
+          claudeSettings?.binaryPath || "claude",
           [
             "-p",
             "--output-format",

diff --git a/apps/server/src/git/Layers/CodexTextGeneration.ts b/apps/server/src/git/Layers/CodexTextGeneration.ts
--- a/apps/server/src/git/Layers/CodexTextGeneration.ts
+++ b/apps/server/src/git/Layers/CodexTextGeneration.ts
@@ -153,7 +153,7 @@
         const reasoningEffort =
           modelSelection.options?.reasoningEffort ?? CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT;
         const command = ChildProcess.make(
-          codexSettings?.binaryPath ?? "codex",
+          codexSettings?.binaryPath || "codex",
           [
             "exec",
             "--ephemeral",

- Allow server settings to decode without an explicit providers block
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Shallow merge in updateSettings overwrites nested providers object
    • Deep-merged the providers object in updateSettings so that when a patch includes providers, each sub-provider (codex, claudeAgent) is merged with the current values instead of being replaced wholesale.

Create PR

Or push these changes by commenting:

@cursor push bba0a8a3d0
Preview (bba0a8a3d0)
diff --git a/apps/server/src/serverSettings.ts b/apps/server/src/serverSettings.ts
--- a/apps/server/src/serverSettings.ts
+++ b/apps/server/src/serverSettings.ts
@@ -271,7 +271,19 @@
       writeSemaphore.withPermits(1)(
         Effect.gen(function* () {
           const current = yield* getSettingsFromCache;
-          const next: ServerSettings = { ...current, ...patch };
+          const next: ServerSettings = {
+            ...current,
+            ...patch,
+            providers: patch.providers
+              ? {
+                  codex: { ...current.providers.codex, ...patch.providers.codex },
+                  claudeAgent: {
+                    ...current.providers.claudeAgent,
+                    ...patch.providers.claudeAgent,
+                  },
+                }
+              : current.providers,
+          };
           yield* writeSettingsAtomically(next);
           yield* Cache.set(settingsCache, cacheKey, next);
           yield* emitChange(next);

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 4 total unresolved issues (including 3 from previous reviews).

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Empty string binary path breaks nullish coalescing fallback
    • Replaced ?? with || in both CodexTextGeneration.ts and ClaudeTextGeneration.ts so the empty string default from Schema.withDecodingDefault(() => "") correctly falls back to "codex" and "claude" respectively.

Create PR

Or push these changes by commenting:

@cursor push 98e6f51693
Preview (98e6f51693)
diff --git a/apps/server/src/git/Layers/ClaudeTextGeneration.ts b/apps/server/src/git/Layers/ClaudeTextGeneration.ts
--- a/apps/server/src/git/Layers/ClaudeTextGeneration.ts
+++ b/apps/server/src/git/Layers/ClaudeTextGeneration.ts
@@ -95,7 +95,7 @@
 
       const runClaudeCommand = Effect.gen(function* () {
         const command = ChildProcess.make(
-          claudeSettings?.binaryPath ?? "claude",
+          claudeSettings?.binaryPath || "claude",
           [
             "-p",
             "--output-format",

diff --git a/apps/server/src/git/Layers/CodexTextGeneration.ts b/apps/server/src/git/Layers/CodexTextGeneration.ts
--- a/apps/server/src/git/Layers/CodexTextGeneration.ts
+++ b/apps/server/src/git/Layers/CodexTextGeneration.ts
@@ -153,7 +153,7 @@
         const reasoningEffort =
           modelSelection.options?.reasoningEffort ?? CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT;
         const command = ChildProcess.make(
-          codexSettings?.binaryPath ?? "codex",
+          codexSettings?.binaryPath || "codex",
           [
             "exec",
             "--ephemeral",

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: deepMerge corrupts arrays by merging them as objects
    • Added Array.isArray guards at both the top-level early return and the per-key recursive branch in deepMerge so arrays are replaced wholesale instead of being spread into plain objects and merged index-by-index.

Create PR

Or push these changes by commenting:

@cursor push 2388e7d43f
Preview (2388e7d43f)
diff --git a/packages/shared/src/Struct.ts b/packages/shared/src/Struct.ts
--- a/packages/shared/src/Struct.ts
+++ b/packages/shared/src/Struct.ts
@@ -1,7 +1,12 @@
 import * as P from "effect/Predicate";
 
 export function deepMerge<T>(current: T, patch: unknown): T {
-  if (!P.isObject(current) || !P.isObject(patch)) {
+  if (
+    !P.isObject(current) ||
+    !P.isObject(patch) ||
+    Array.isArray(current) ||
+    Array.isArray(patch)
+  ) {
     return patch as T;
   }
 
@@ -10,7 +15,10 @@
     if (value === undefined) continue;
 
     const existing = next[key];
-    next[key] = P.isObject(existing) && P.isObject(value) ? deepMerge(existing, value) : value;
+    next[key] =
+      P.isObject(existing) && P.isObject(value) && !Array.isArray(existing) && !Array.isArray(value)
+        ? deepMerge(existing, value)
+        : value;
   }
 
   return next as T;

- Read Codex and Claude CLI paths from server settings
- Add provider registry and status layers for health checks
- Update session startup, model selection, and UI status handling
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Child process env missing inherited process.env variables
    • Spread process.env into the env option before the CODEX_HOME override, matching the pattern used in codexAppServerManager.ts and CodexProvider.ts.

Create PR

Or push these changes by commenting:

@cursor push 42afbb2266
Preview (42afbb2266)
diff --git a/apps/server/src/git/Layers/CodexTextGeneration.ts b/apps/server/src/git/Layers/CodexTextGeneration.ts
--- a/apps/server/src/git/Layers/CodexTextGeneration.ts
+++ b/apps/server/src/git/Layers/CodexTextGeneration.ts
@@ -172,7 +172,10 @@
             "-",
           ],
           {
-            env: codexSettings?.homePath ? { CODEX_HOME: codexSettings.homePath } : {},
+            env: {
+              ...process.env,
+              ...(codexSettings?.homePath ? { CODEX_HOME: codexSettings.homePath } : {}),
+            },
             cwd,
             shell: process.platform === "win32",
             stdin: {

Comment on lines +46 to +48
yield* Stream.runForEach(input.streamSettings, (nextSettings) =>
Effect.asVoid(reconcileSnapshot(nextSettings)),
).pipe(Effect.forkScoped);
Copy link
Contributor

Choose a reason for hiding this comment

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

🟠 High provider/makeManagedServerProvider.ts:46

The settings stream fiber crashes on reconcileSnapshot failure and stops processing settings changes. Unlike the periodic refresh fiber (lines 50-55) which catches and logs errors with Effect.ignoreCause, the stream handler on lines 46-48 propagates errors unhandled, causing the fiber to terminate. Consider wrapping reconcileSnapshot with Effect.ignoreCause({ log: true }) so stream processing continues after errors.

-    yield* Stream.runForEach(input.streamSettings, (nextSettings) =>
-      Effect.asVoid(reconcileSnapshot(nextSettings)),
-    ).pipe(Effect.forkScoped);
+    yield* Stream.runForEach(input.streamSettings, (nextSettings) =>
+      reconcileSnapshot(nextSettings).pipe(Effect.ignoreCause({ log: true })),
+    ).pipe(Effect.forkScoped);
Also found in 1 other location(s)

apps/server/src/orchestration/Layers/ProviderCommandReactor.ts:432

The yield* Effect.map(serverSettingsService.getSettings, ...) on line 432-435 can fail with ServerSettingsError, but this error is not caught by the .pipe(Effect.catch(...), Effect.catchCause(...)) handlers on lines 437-468. The yield* for getSettings executes before the generateBranchName call, so if it fails, the error propagates up through the generator and bypasses the error handlers that were intended to log warnings for failures in this function. Before this change, modelSelection was a static object that couldn't fail. The fix should either move the getSettings call outside the object literal and wrap the whole block in error handling, or use Effect.flatMap to sequence the operations so failures are captured by the existing catchCause.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/makeManagedServerProvider.ts around lines 46-48:

The settings stream fiber crashes on `reconcileSnapshot` failure and stops processing settings changes. Unlike the periodic refresh fiber (lines 50-55) which catches and logs errors with `Effect.ignoreCause`, the stream handler on lines 46-48 propagates errors unhandled, causing the fiber to terminate. Consider wrapping `reconcileSnapshot` with `Effect.ignoreCause({ log: true })` so stream processing continues after errors.

Evidence trail:
- `apps/server/src/provider/makeManagedServerProvider.ts` lines 46-48 (settings stream without error handling) vs lines 50-55 (periodic refresh with `Effect.ignoreCause({ log: true })`)
- `apps/server/src/serverSettings.ts` lines 267-271 shows correct pattern: `Stream.runForEach(...).pipe(Effect.ignoreCause({ log: true }), Effect.forkIn(...))`
- `apps/server/src/keybindings.ts` lines 836-840 uses same correct pattern
- Commit: REVIEWED_COMMIT

Also found in 1 other location(s):
- apps/server/src/orchestration/Layers/ProviderCommandReactor.ts:432 -- The `yield* Effect.map(serverSettingsService.getSettings, ...)` on line 432-435 can fail with `ServerSettingsError`, but this error is not caught by the `.pipe(Effect.catch(...), Effect.catchCause(...))` handlers on lines 437-468. The `yield*` for `getSettings` executes *before* the `generateBranchName` call, so if it fails, the error propagates up through the generator and bypasses the error handlers that were intended to log warnings for failures in this function. Before this change, `modelSelection` was a static object that couldn't fail. The fix should either move the `getSettings` call outside the object literal and wrap the whole block in error handling, or use `Effect.flatMap` to sequence the operations so failures are captured by the existing `catchCause`.

- Track settings updates in the test service
- Point Codex tests at the fake binary via server settings instead of env vars
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Duplicate server settings layer creates inconsistent state
    • Replaced the duplicate ServerSettingsService.layerTest() call at line 327 with the existing serverSettingsLayer variable so all layers share the same Ref state.

Create PR

Or push these changes by commenting:

@cursor push a70e7d7e49
Preview (a70e7d7e49)
diff --git a/apps/server/integration/OrchestrationEngineHarness.integration.ts b/apps/server/integration/OrchestrationEngineHarness.integration.ts
--- a/apps/server/integration/OrchestrationEngineHarness.integration.ts
+++ b/apps/server/integration/OrchestrationEngineHarness.integration.ts
@@ -324,7 +324,7 @@
     );
     const layer = orchestrationReactorLayer.pipe(
       Layer.provide(persistenceLayer),
-      Layer.provideMerge(ServerSettingsService.layerTest()),
+      Layer.provideMerge(serverSettingsLayer),
       Layer.provideMerge(ServerConfig.layerTest(workspaceDir, rootDir)),
       Layer.provideMerge(NodeServices.layer),
     );

- Route Codex and Claude health checks through ServerSettingsService
- Reuse layerTest helpers in provider tests and orchestration tests
- Update provider snapshots and structs for the new settings authority
- Pass the full process environment to the Codex child process
- Keep CODEX_HOME override when a custom settings home is configured
- Guard `start()` with an atomic started flag
- Await the existing deferred when startup has already begun
- compare provider snapshots before broadcasting registry updates
- add regression coverage for equal provider lists
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Settings error escapes error handlers in branch rename
    • Moved the serverSettingsService.getSettings call into the Effect chain via Effect.flatMap so that settings failures are now caught by the downstream Effect.catch/Effect.catchCause handlers instead of escaping the generator.

Create PR

Or push these changes by commenting:

@cursor push 361b51678a
Preview (361b51678a)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -424,48 +424,52 @@
     const oldBranch = input.branch;
     const cwd = input.worktreePath;
     const attachments = input.attachments ?? [];
-    yield* textGeneration
-      .generateBranchName({
-        cwd,
-        message: input.messageText,
-        ...(attachments.length > 0 ? { attachments } : {}),
-        modelSelection: yield* Effect.map(
-          serverSettingsService.getSettings,
-          (settings) => settings.textGenerationModelSelection,
+    yield* Effect.flatMap(
+      Effect.map(
+        serverSettingsService.getSettings,
+        (settings) => settings.textGenerationModelSelection,
+      ),
+      (modelSelection) =>
+        textGeneration.generateBranchName({
+          cwd,
+          message: input.messageText,
+          ...(attachments.length > 0 ? { attachments } : {}),
+          modelSelection,
+        }),
+    ).pipe(
+      Effect.catch((error) =>
+        Effect.logWarning(
+          "provider command reactor failed to generate worktree branch name; skipping rename",
+          { threadId: input.threadId, cwd, oldBranch, reason: error.message },
         ),
-      })
-      .pipe(
-        Effect.catch((error) =>
-          Effect.logWarning(
-            "provider command reactor failed to generate worktree branch name; skipping rename",
-            { threadId: input.threadId, cwd, oldBranch, reason: error.message },
-          ),
-        ),
-        Effect.flatMap((generated) => {
-          if (!generated) return Effect.void;
+      ),
+      Effect.flatMap((generated) => {
+        if (!generated) return Effect.void;
 
-          const targetBranch = buildGeneratedWorktreeBranchName(generated.branch);
-          if (targetBranch === oldBranch) return Effect.void;
+        const targetBranch = buildGeneratedWorktreeBranchName(generated.branch);
+        if (targetBranch === oldBranch) return Effect.void;
 
-          return Effect.flatMap(
-            git.renameBranch({ cwd, oldBranch, newBranch: targetBranch }),
-            (renamed) =>
-              orchestrationEngine.dispatch({
-                type: "thread.meta.update",
-                commandId: serverCommandId("worktree-branch-rename"),
-                threadId: input.threadId,
-                branch: renamed.branch,
-                worktreePath: cwd,
-              }),
-          );
+        return Effect.flatMap(
+          git.renameBranch({ cwd, oldBranch, newBranch: targetBranch }),
+          (renamed) =>
+            orchestrationEngine.dispatch({
+              type: "thread.meta.update",
+              commandId: serverCommandId("worktree-branch-rename"),
+              threadId: input.threadId,
+              branch: renamed.branch,
+              worktreePath: cwd,
+            }),
+        );
+      }),
+      Effect.catchCause((cause) =>
+        Effect.logWarning("provider command reactor failed to generate or rename worktree branch", {
+          threadId: input.threadId,
+          cwd,
+          oldBranch,
+          cause: Cause.pretty(cause),
         }),
-        Effect.catchCause((cause) =>
-          Effect.logWarning(
-            "provider command reactor failed to generate or rename worktree branch",
-            { threadId: input.threadId, cwd, oldBranch, cause: Cause.pretty(cause) },
-          ),
-        ),
-      );
+      ),
+    );
   });
 
   const processTurnStartRequested = Effect.fnUntraced(function* (

- Expose `server.refreshProviders` over WebSocket and NativeApi
- Add a settings UI refresh button with last-checked relative time
- Refresh provider cache and invalidate server config queries
- Read text generation model selection from server settings before branch generation
- Simplify branch rename flow and keep failures logged at the reactor boundary
- Remove leftover temp files after renaming keybindings and server settings
- Keep atomic write flow while avoiding stale temporary artifacts
@juliusmarminge juliusmarminge merged commit 9403429 into main Mar 26, 2026
11 checks passed
@juliusmarminge juliusmarminge deleted the t3code/server-settings-authority branch March 26, 2026 06:59
aaditagrawal pushed a commit to aaditagrawal/t3code that referenced this pull request Mar 26, 2026
bulbulogludemir added a commit to bulbulogludemir/krabbycode that referenced this pull request Mar 26, 2026
…ings architecture

Major upstream changes merged:
- Server-authoritative settings (pingdotgg#1421): appSettings.ts deleted, settings
  moved to serverSettings.ts + contracts/settings.ts + useSettings hook
- Provider-aware model selections (pingdotgg#1371): model→modelSelection refactor,
  new composerDraftStore, unified TraitsPicker, migration 016
- Context window usage UI (pingdotgg#1351): ContextWindowMeter component
- Claude as TextGenerationProvider (pingdotgg#1323): git text generation via Claude
- Bootstrap FD for security (pingdotgg#1398): sensitive config sent over file descriptor
- Sidebar recency sorting (pingdotgg#1372): projects/threads sorted by recency
- ProviderHealth replaced by ProviderRegistry + per-provider services
- Word wrapping, terminal history preservation, various fixes

Krabby-specific adaptations:
- Migration 016 renumbered to 024 (Krabby 016-023 preserved)
- 20+ Krabby settings ported to new ClientSettingsSchema
- KrabbyProviderOptions schema for chrome/enterprise/budget fields
- All @t3tools@kRaBby, t3code→krabbycode, service keys t3/→krabby/

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ 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.

move some settings to server authorative

2 participants