Add Codex App LLM setup support - #5810
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5810 +/- ##
==========================================
- Coverage 70.90% 70.89% -0.02%
==========================================
Files 685 686 +1
Lines 69696 69758 +62
==========================================
+ Hits 49421 49457 +36
- Misses 16650 16687 +37
+ Partials 3625 3614 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rdimitrov
left a comment
There was a problem hiding this comment.
Automated review focused on the new Codex desktop detection path. The change is solid and well-tested — the config-write case when ~/.codex is absent is correctly handled by configureCodexAuth's os.MkdirAll, and it's covered by the new E2E scenario. The comments below are robustness/quality issues rather than correctness-breaking bugs. Findings 1–2 (silent non-detection with no log) are the ones worth acting on before merge.
Generated with Claude Code
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Detect Codex desktop installations on macOS and Windows so app-only users can configure the shared Codex model provider through thv llm setup. Keep one canonical Codex configuration and teardown path for both the CLI and desktop app. Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
- Log detector errors at WARN instead of silently swallowing them - Invoke plutil by absolute path (/usr/bin/plutil) to avoid PATH resolution failures in stripped environments - Refactor to a per-client LLMInstalledDetector hook, removing the hardcoded Codex branch and duplicated CLI detection logic - Guard lookPath consistently in the shared detection path - Tighten the #nosec G204 justification
b590b16 to
9ef7064
Compare
PR size has been reduced below the XL threshold. Thank you for splitting this up!
|
✅ PR size has been reduced below the XL threshold. The size review has been dismissed and this PR can now proceed with normal review. Thank you for splitting this up! |
jhrozek
left a comment
There was a problem hiding this comment.
restoring previous approval
Summary
Codex desktop users could not run
thv llm setupunless the separate Codex CLI was installed and had already created~/.codex. The desktop app uses the same Codex user configuration, so requiring CLI installation prevented ToolHive from configuring otherwise valid app-only installations.codexsetup and teardown target because the CLI and desktop app share the same user configuration.Fixes #5785
Type of change
Test plan
task test)task test-e2e)task lint-fix)Additional verification:
task buildtask docsThe full E2E suite was attempted but could not complete because its API test server reported that no usable container runtime was available. The new Darwin app-only E2E scenario is included for CI/macOS execution.
API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
pkg/client/codex_desktop.gopkg/client/llm_gateway.gopkg/client/discovery.gopkg/client/*_test.gotest/e2e/cli_llm_all_clients_test.gocmd/thv/app/llm.go,docs/cli/thv_llm_setup.mdcodextarget.Does this introduce a user-facing change?
Yes.
thv llm setup --client codexnow detects Codex desktop installations on macOS and Windows even when the Codex CLI is absent. CLI and desktop installations continue to share onecodexconfiguration and teardown target.Implementation plan
Approved implementation plan
codexclient identity and reuse its~/.codex/config.tomlwriter and teardown logic.CFBundleIdentifier = com.openai.codex.WindowsAppspaths.Runtime model discoverability is not applicable because this is host application detection and local configuration.
Special notes for reviewers
The current unified macOS application is named
ChatGPT.appbut retains the Codex bundle identifiercom.openai.codex; the detector also supports the legacyCodex.apppath. Windows detection verifies the exact package familyOpenAI.Codex_2p2nqsd0c76g0and requires a Microsoft Store signature.