fix(server): use custom codex binary path in provider health checks#793
Open
binbandit wants to merge 5 commits intopingdotgg:mainfrom
Open
fix(server): use custom codex binary path in provider health checks#793binbandit wants to merge 5 commits intopingdotgg:mainfrom
binbandit wants to merge 5 commits intopingdotgg:mainfrom
Conversation
|
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
the
vouch:trusted
binbandit
force-pushed
the
fix/provider-health-custom-binary-path
branch
from
123c7ad to
a30cb08
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
The provider health check always spawned the bare 'codex' command, ignoring the user's custom binary path from Settings. This caused a persistent 'codex not found' error banner even when sessions worked correctly with the configured path. Three root causes are addressed: 1. checkCodexProviderStatus and runCodexCommand now accept an optional binaryPath parameter instead of hardcoding 'codex'. 2. ProviderHealth service exposes a recheckStatuses method, wired through a new server.recheckProviderHealth WS RPC, so the web client can trigger a re-check when the binary path setting changes. 3. The __root.tsx config-update dedup signature now includes provider statuses, so push messages with updated health results are no longer silently dropped. Closes pingdotgg#630
…d stale state - Validate binaryPath against a safe-character allowlist and double-quote it on Windows to prevent command injection via shell metacharacters and handle paths with spaces (e.g. C:\Program Files\...). - Read getStatuses from statusesRef instead of permanently returning the initial fiber result, so callers see updated data after recheckStatuses. - Load keybindings config state before broadcasting serverConfigUpdated so the push payload contains actual issues instead of an empty array.
After rebasing onto main, checkCodexProviderStatus now depends on FileSystem and Path (for hasCustomModelProvider config detection). The runCheck helper in ProviderHealthLive must provide these services alongside ChildProcessSpawner.
binbandit
force-pushed
the
fix/provider-health-custom-binary-path
branch
from
abbb6ff to
d499f5d
Compare
Replace manual useEffect/useRef/setTimeout debounce with useDebouncedValue from @tanstack/react-pacer and a useMutation backed by a serverRecheckProviderHealthMutationOptions factory, aligning with the established React Query patterns in the codebase.
Contributor
Author
|
@juliusmarminge Good to go |
…eMutation The provider health recheck is semantically a query — given a binary path, derive the health status. Model it as a useQuery keyed on the debounced path so React Query handles retries, deduplication, and cache lifecycle automatically, eliminating the need for manual useEffect/useRef orchestration.
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
2 participants
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
Fixes #630 — the persistent "codex not found" error banner that appeared even when Codex sessions worked correctly with a custom binary path configured in Settings.
Root Cause
Three issues conspired to produce this bug:
ProviderHealth.ts—runCodexCommand()hardcoded"codex"as the binary name. The health check always spawned the barecodexPATH lookup, ignoring the user's custom binary path from Settings.No recheck mechanism — The health check ran once at server boot via a forked fiber. There was no way to re-run it when the user changed the binary path in Settings afterward.
__root.tsxdedup bug — TheonServerConfigUpdateddedup signature only comparedpayload.issues(keybinding issues), notpayload.providers. So even if health check results changed, the config update push was silently dropped as a "duplicate" — the UI never learned the status improved.Changes
Server (
apps/server)ProviderHealth.ts:runCodexCommand()andcheckCodexProviderStatus()now accept an optionalbinaryPathparameter (defaults to"codex"). Error messages include the actual binary path in the label.isCommandMissingCause()simplified to generic ENOENT/not-found detection (no longer hardcodes"codex"in the match strings).Services/ProviderHealth.ts: AddedrecheckStatuses(binaryPath?)method to the service interface.ProviderHealth.ts(Layer):ProviderHealthLivecapturesChildProcessSpawnerat layer construction time, exposesrecheckStatuses()which re-runs the full health check pipeline with the given binary path and updates the cached statuses.wsServer.ts: Newserver.recheckProviderHealthRPC handler — re-runs the health check, updates the cachedproviders, broadcasts updated statuses to all connected clients, and returns the full server config.Contracts (
packages/contracts)server.ts: AddedRecheckProviderHealthInputschema ({ codexBinaryPath?: TrimmedNonEmptyString }).ws.ts: AddedserverRecheckProviderHealthmethod constant and tagged request body.ipc.ts: AddedrecheckProviderHealthto theNativeApi.serverinterface.Web (
apps/web)wsNativeApi.ts: WiredrecheckProviderHealththrough the WS transport._chat.settings.tsx: Added a debounced (600ms)useEffectthat triggersserver.recheckProviderHealthwhenevercodexBinaryPathchanges in Settings. Skips the initial mount to avoid unnecessary checks.__root.tsx: Fixed the config update dedup signature to includeproviders, so push messages with updated health results are no longer silently dropped.Tests
ProviderHealth.test.ts: Updated all 6 effect tests to callcheckCodexProviderStatus()as a function instead of yielding the raw Effect constant. All 9 tests pass.wsServer.test.ts: Updated the mockProviderHealthShapeto includerecheckStatuses. All 36 tests pass.Validation
bun lint— 0 errorsbun typecheck— passes for@t3tools/contracts,t3(server), and@t3tools/web(only pre-existing unrelated@dnd-kittype errors inSidebar.tsx)Note
Add
server.recheckProviderHealthRPC to run health checks with a custom Codex binary pathcheckCodexProviderStatusfrom a static Effect value to a function accepting an optionalbinaryPath, allowing health checks to target a custom Codex binary.recheckStatusesmethod toProviderHealthShapeand a newserver.recheckProviderHealthWebSocket RPC method that accepts an optionalcodexBinaryPath, re-runs health checks, and broadcasts updated server config to all clients.SAFE_BINARY_PATH_REand quotes Windows paths withshell: trueto handle spaces safely.serverConfigUpdatedevent handler where provider status changes were ignored if issues were unchanged.Macroscope summarized 46ebfdc.