Repository navigation
Add startup Codex provider health checks and surface status in server config/chat banner + Windows BS - #163
Merged
Conversation
- add startup ProviderHealth service/layer to probe Codex install and auth state - extend server contracts and WS payloads to include provider statuses - show non-ready provider warnings in ChatView and cover with server/web tests
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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: Exported function
makeProviderHealthLiveis never used- Removed the unused
makeProviderHealthLivefunction, which was confirmed to have no consumers anywhere in the codebase.
- Removed the unused
Or push these changes by commenting:
@cursor push 4b7e2957f3
Preview (4b7e2957f3)
diff --git a/apps/server/src/provider/Layers/ProviderHealth.ts b/apps/server/src/provider/Layers/ProviderHealth.ts
--- a/apps/server/src/provider/Layers/ProviderHealth.ts
+++ b/apps/server/src/provider/Layers/ProviderHealth.ts
@@ -240,7 +240,3 @@
});
export const ProviderHealthLive = Layer.effect(ProviderHealth, makeProviderHealth());
-
-export function makeProviderHealthLive(run?: CommandRunner) {
- return Layer.effect(ProviderHealth, makeProviderHealth(run));
-}- Parse nested/array JSON auth status output from `codex auth status --json` - Return `unauthenticated` when JSON contains `authenticated: false` - Return warning when JSON is parseable but lacks an auth marker
juliusmarminge
changed the title
Contributor
There was a problem hiding this comment.
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: Windows turbo command regression removes platform handling
- Added
shell: process.platform === "win32"to theChildProcess.makeoptions for the turbo spawn call, consistent with all other spawn calls in the PR.
- Added
Or push these changes by commenting:
@cursor push 54822a3a0c
Preview (54822a3a0c)
diff --git a/scripts/dev-runner.ts b/scripts/dev-runner.ts
--- a/scripts/dev-runner.ts
+++ b/scripts/dev-runner.ts
@@ -415,6 +415,7 @@
stderr: "inherit",
env,
extendEnv: false,
+ shell: process.platform === "win32",
},
);
Comment on lines
+44
to
+54
| function detailFromResult(result: ProcessRunResult): string | undefined { | ||
| if (result.timedOut) return "Timed out while running command."; | ||
| const stderr = nonEmptyMessage(result.stderr); | ||
| if (stderr) return stderr; | ||
| const stdout = nonEmptyMessage(result.stdout); | ||
| if (stdout) return stdout; | ||
| if (result.code !== null && result.code !== 0) { | ||
| return `Command exited with code ${result.code}.`; | ||
| } | ||
| return undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
🟢 Low Layers/ProviderHealth.ts:44
When a process is terminated by a signal (e.g., SIGSEGV, SIGABRT), result.code is null and result.signal is populated. The check at line 50 skips error reporting when code is null, so detailFromResult returns undefined for signal-terminated processes. This causes checkCodexProviderStatus to display a generic "failed to run" message instead of indicating the process crashed.
function detailFromResult(result: ProcessRunResult): string | undefined {
if (result.timedOut) return "Timed out while running command.";
+ if (result.signal !== null) {
+ return `Process terminated by signal ${result.signal}.`;
+ }
const stderr = nonEmptyMessage(result.stderr);
if (stderr) return stderr;
const stdout = nonEmptyMessage(result.stdout);
if (stdout) return stdout;
if (result.code !== null && result.code !== 0) {
return `Command exited with code ${result.code}.`;
}
return undefined;
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/ProviderHealth.ts around lines 44-54:
When a process is terminated by a signal (e.g., SIGSEGV, SIGABRT), `result.code` is `null` and `result.signal` is populated. The check at line 50 skips error reporting when `code` is `null`, so `detailFromResult` returns `undefined` for signal-terminated processes. This causes `checkCodexProviderStatus` to display a generic "failed to run" message instead of indicating the process crashed.
Evidence trail:
apps/server/src/provider/Layers/ProviderHealth.ts lines 44-54 (detailFromResult function with the check at line 50: `if (result.code !== null && result.code !== 0)`), apps/server/src/processRunner.ts lines 13-21 (ProcessRunResult interface showing both `code: number | null` and `signal: NodeJS.Signals | null`), apps/server/src/provider/Layers/ProviderHealth.ts lines 198-208 (checkCodexProviderStatus showing how undefined detail leads to generic message)
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Duplicated Windows process kill helper across two files
- Removed the duplicate killChildTree from codexAppServerManager.ts and replaced it with an import of the now-exported killChild from processRunner.ts, consolidating the Windows taskkill /T logic in a single location.
Or push these changes by commenting:
@cursor push 17aee2c430
Preview (17aee2c430)
diff --git a/apps/server/src/codexAppServerManager.ts b/apps/server/src/codexAppServerManager.ts
--- a/apps/server/src/codexAppServerManager.ts
+++ b/apps/server/src/codexAppServerManager.ts
@@ -1,8 +1,10 @@
-import { type ChildProcessWithoutNullStreams, spawn, spawnSync } from "node:child_process";
+import { type ChildProcessWithoutNullStreams, spawn } from "node:child_process";
import { randomUUID } from "node:crypto";
import { EventEmitter } from "node:events";
import readline from "node:readline";
+import { killChild } from "./processRunner.js";
+
import {
ApprovalRequestId,
EventId,
@@ -104,23 +106,6 @@
"does not exist",
];
-/**
- * On Windows with `shell: true`, `child.kill()` only terminates the `cmd.exe`
- * wrapper, leaving the actual command running. Use `taskkill /T` to kill the
- * entire process tree instead.
- */
-function killChildTree(child: ChildProcessWithoutNullStreams): void {
- if (process.platform === "win32" && child.pid !== undefined) {
- try {
- spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" });
- return;
- } catch {
- // fallback to direct kill
- }
- }
- child.kill();
-}
-
export function normalizeCodexModelSlug(
model: string | undefined | null,
preferredId?: string,
@@ -493,7 +478,7 @@
context.output.close();
if (!context.child.killed) {
- killChildTree(context.child);
+ killChild(context.child);
}
this.updateSession(context, {
diff --git a/apps/server/src/processRunner.ts b/apps/server/src/processRunner.ts
--- a/apps/server/src/processRunner.ts
+++ b/apps/server/src/processRunner.ts
@@ -85,7 +85,7 @@
* wrapper, leaving the actual command running. Use `taskkill /T` to kill the
* entire process tree instead.
*/
-function killChild(child: ChildProcessHandle, signal: NodeJS.Signals = "SIGTERM"): void {
+export function killChild(child: ChildProcessHandle, signal: NodeJS.Signals = "SIGTERM"): void {
if (process.platform === "win32" && child.pid !== undefined) {
try {
spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" });| } | ||
| } | ||
| child.kill(); | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicated Windows process kill helper across two files
Low Severity
killChildTree in codexAppServerManager.ts and killChild in processRunner.ts are near-identical implementations of the same Windows taskkill /T process-tree kill logic. The only difference is that killChild accepts an optional signal parameter. This duplication means a future bug fix in one copy could easily be missed in the other.
Additional Locations (1)
Comment on lines
+112
to
+122
| function killChildTree(child: ChildProcessWithoutNullStreams): void { | ||
| if (process.platform === "win32" && child.pid !== undefined) { | ||
| try { | ||
| spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" }); | ||
| return; | ||
| } catch { | ||
| // fallback to direct kill | ||
| } | ||
| } | ||
| child.kill(); | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Medium src/codexAppServerManager.ts:112
When taskkill fails, spawnSync returns an error object instead of throwing, so the function returns early and never falls back to child.kill(). This leaves the codex process running on Windows when taskkill fails or is not found. Check result.error or result.status after the spawnSync call and only return early on success, otherwise fall through to child.kill().
function killChildTree(child: ChildProcessWithoutNullStreams): void {
if (process.platform === "win32" && child.pid !== undefined) {
- try {
- spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" });
- return;
- } catch {
- // fallback to direct kill
- }
+ const result = spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" });
+ if (result.error === undefined && result.status === 0) {
+ return;
+ }
}
child.kill();
}Also found in 1 other location(s)
apps/server/src/processRunner.ts:88
spawnSyncdoes not throw an exception when the process fails to spawn (e.g.,ENOENT) or exits with a non-zero code; instead, it returns an object containingerrororstatusproperties. Thetry...catchblock aroundspawnSyncis therefore ineffective at detecting failures. Iftaskkillfails to execute (for example, due to permission issues or missing executable), the code hits thereturn;statement and exits the function early. This causes the fallbackchild.kill(signal)to be skipped, potentially leaving the process running. You should check the return value ofspawnSync(e.g.,result.errororresult.status) before returning.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/codexAppServerManager.ts around lines 112-122:
When `taskkill` fails, `spawnSync` returns an error object instead of throwing, so the function returns early and never falls back to `child.kill()`. This leaves the codex process running on Windows when taskkill fails or is not found. Check `result.error` or `result.status` after the `spawnSync` call and only return early on success, otherwise fall through to `child.kill()`.
Evidence trail:
apps/server/src/codexAppServerManager.ts lines 112-120 at REVIEWED_COMMIT show that `spawnSync` is called followed by unconditional `return;` inside a try block, with catch intended to fall through to `child.kill()`. Node.js documentation (https://nodejs.org/api/child_process.html#child_processspawnsynccommand-args-options) confirms that `spawnSync` returns an object with `error` and `status` properties rather than throwing on command failure.
Also found in 1 other location(s):
- apps/server/src/processRunner.ts:88 -- `spawnSync` does not throw an exception when the process fails to spawn (e.g., `ENOENT`) or exits with a non-zero code; instead, it returns an object containing `error` or `status` properties. The `try...catch` block around `spawnSync` is therefore ineffective at detecting failures. If `taskkill` fails to execute (for example, due to permission issues or missing executable), the code hits the `return;` statement and exits the function early. This causes the fallback `child.kill(signal)` to be skipped, potentially leaving the process running. You should check the return value of `spawnSync` (e.g., `result.error` or `result.status`) before returning.
Comment on lines
226
to
+238
| export const launchDetached = (launch: EditorLaunch) => | ||
| Effect.callback<void, OpenError>((resume) => { | ||
| let child; | ||
| try { | ||
| child = spawn(launch.command, [...launch.args], { | ||
| detached: true, | ||
| stdio: "ignore", | ||
| }); | ||
| } catch (error) { | ||
| return resume( | ||
| Effect.fail(new OpenError({ message: "failed to spawn detached process", cause: error })), | ||
| ); | ||
| Effect.gen(function* () { | ||
| if (!isCommandAvailable(launch.command)) { | ||
| return yield* new OpenError({ message: `Editor command not found: ${launch.command}` }); | ||
| } | ||
|
|
||
| const handleSpawn = () => { | ||
| child.unref(); | ||
| resume(Effect.void); | ||
| }; | ||
| yield* Effect.callback<void, OpenError>((resume) => { | ||
| let child; | ||
| try { | ||
| child = spawn(launch.command, [...launch.args], { | ||
| detached: true, | ||
| stdio: "ignore", | ||
| shell: process.platform === "win32", |
Contributor
There was a problem hiding this comment.
🔴 Critical src/open.ts:226
When shell: true is set on Windows, launch.args containing user-provided cwd values are passed through the shell without guaranteed sanitization, allowing command injection via shell metacharacters in the path. Remove shell: true and resolve the .cmd extension manually before spawning, or use cross-spawn to handle Windows executable resolution safely.
- shell: process.platform === "win32",🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/open.ts around lines 226-238:
When `shell: true` is set on Windows, `launch.args` containing user-provided `cwd` values are passed through the shell without guaranteed sanitization, allowing command injection via shell metacharacters in the path. Remove `shell: true` and resolve the `.cmd` extension manually before spawning, or use `cross-spawn` to handle Windows executable resolution safely.
Evidence trail:
apps/server/src/open.ts lines 235-239: `spawn(launch.command, [...launch.args], { ... shell: process.platform === "win32" })`
apps/server/src/open.ts lines 205-224: `resolveEditorLaunch` creates args containing `input.cwd`
packages/contracts/src/editor.ts lines 14-17: `OpenInEditorInput` schema with `cwd: TrimmedNonEmptyString`
packages/contracts/src/baseSchemas.ts line 4: `TrimmedNonEmptyString = TrimmedString.check(Schema.isNonEmpty())` - no shell metacharacter sanitization
packages/contracts/src/ws.ts line 106: `tagRequestBody(WS_METHODS.shellOpenInEditor, OpenInEditorInput)` - confirms cwd is user-controlled via WebSocket
…g/t3code into t3code/codex-health-error-state
- Run Codex-related child processes with `shell` on Windows for reliable command execution - Switch provider auth probe to `codex login status` and update health-check expectations - Use Node PTY on Windows instead of Bun PTY in server runtime setup
…d ENOENT) Add tree-kill via taskkill /T /F in processRunner and codexAppServerManager so timeout enforcement and session stop properly terminate the entire process tree instead of just the cmd.exe wrapper. Detect Windows command not found (exit 9009) in normalizeExitError. Pre-validate editor commands with isCommandAvailable in open.ts before spawning. Fix ProviderHealth test import and dev-runner test path assertion for Windows. Made-with: Cursor
TomCohenDev
pushed a commit
to TomCohenDev/t3code
that referenced
this pull request
Oct 18, 2026
…bhook-retry fix(fork): repair webhook retries for 15 minutes and dedupes only delivered calls
TomCohenDev
pushed a commit
to TomCohenDev/t3code
that referenced
this pull request
Oct 18, 2026
The repair webhook runs as the "Fork repair" task on the VPS. The docs said only "a T3 server". Also note that each VPS auto-update restarts its server for about 20 seconds, which the 15-minute retry of pingdotgg#163 covers.
HaukeSchnau
pushed a commit
to HaukeSchnau/t3code
that referenced
this pull request
Nov 13, 2026
…entities Answers CodeRabbit on pingdotgg#163 and pingdotgg#164: each sign-in gets its own binding cookie, a full pending map turns new starts away instead of pushing out live ones, the token exchange times out including its body, and logs name a fixed stage instead of provider data, subjects or emails. The app sends a random client_state that must come back with the result, and a sheet that cannot open fails instead of throwing. A managed policy without remote clients stops the server from offering sign-in. The add-environment dialog ignores a lookup for a host the field no longer shows, and the route tests run under it.effect.
HaukeSchnau
pushed a commit
to HaukeSchnau/t3code
that referenced
this pull request
Nov 13, 2026
…entities Answers CodeRabbit on pingdotgg#163 and pingdotgg#164: each sign-in gets its own binding cookie, a full pending map turns new starts away instead of pushing out live ones, the token exchange times out including its body, and logs name a fixed stage instead of provider data, subjects or emails. The app sends a random client_state that must come back with the result, and a sheet that cannot open fails instead of throwing. A managed policy without remote clients stops the server from offering sign-in. The add-environment dialog ignores a lookup for a host the field no longer shows, and the route tests run under it.effect.
HaukeSchnau
pushed a commit
to HaukeSchnau/t3code
that referenced
this pull request
Nov 14, 2026
…nID Connect provider' (pingdotgg#163) from feat/oidc-sign-in into main
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ProviderHealthservice/layer that runs startup-time Codex CLI health probes (codex --versionandcodex auth status --json) and caches provider statusServerProviderStatus) and propagate it throughserver.getConfigandserverConfigUpdatedProviderHealthLiveinto server startup so status is available to WebSocket handlersreadyTesting
apps/server/src/provider/Layers/ProviderHealth.test.ts: covers ready, missing CLI, unauthenticated, and unsupported auth command casesapps/server/src/wsServer.test.ts: verifiesprovidersis included inserver.getConfigandserverConfigUpdatedpayloadsapps/web/src/wsNativeApi.test.ts: verifiesprovidersis handled and forwarded with server config updatesbun lintbun typecheckNote
Medium Risk
Expands the
ServerConfigcontract and WebSocket payloads to include provider status, which can break older clients, and changes process spawning/termination behavior (notably on Windows). The health probing runs at startup and shells out tocodex, so failures/timeouts could impact perceived readiness and logging.Overview
Adds a new startup
ProviderHealthlayer that probes the Codex CLI (codex --version+codex login status), parses auth/readiness, and caches a provider status snapshot.Extends contracts so
server.getConfigandserver.configUpdatednow includeproviders: ServerProviderStatuses, wires this intowsServer, and updates the webChatViewto display a warning/error banner when the active provider is notready.Improves Windows compatibility for spawned processes by using
shell: truewhere needed and killing full process trees viataskkill(Codex app-server manager, text generation, detached editor launch, genericrunProcess), adjusts PTY adapter selection on Windows, and tweaks dev-runner/devscript behavior; adds/updates tests covering provider health and config propagation.Written by Cursor Bugbot for commit a2c8f59. This will update automatically on new commits. Configure here.
Note
Add startup Codex provider health checks and display provider status in server config and chat banner while fixing Windows process spawning and termination across server utilities
Introduce a
ProviderHealthservice that computes Codex CLI availability/auth on startup and returns statuses, expose statuses in server config and push updates, and render a provider health banner in chat; standardize Windows process spawning withshell: process.platform === "win32"and kill full process trees viataskkill, with clearer Windows "command not found" errors. Key entry points:ProviderHealth.ts,wsServer.ts, andChatView.tsx.📍Where to Start
Start with the provider status computation in
ProviderHealthLiveandcheckCodexProviderStatusin apps/server/src/provider/Layers/ProviderHealth.ts, then follow how results flow into server responses in apps/server/src/wsServer.ts and UI rendering in apps/web/src/components/ChatView.tsx.Macroscope summarized a2c8f59.