Skip to content

feat(llm-cli): modularize CLI binary resolution#795

Open
muddlebee wants to merge 4 commits intoTracer-Cloud:mainfrom
muddlebee:feat/llm-cli-generic-resolver
Open

feat(llm-cli): modularize CLI binary resolution#795
muddlebee wants to merge 4 commits intoTracer-Cloud:mainfrom
muddlebee:feat/llm-cli-generic-resolver

Conversation

@muddlebee
Copy link
Copy Markdown
Contributor

@muddlebee muddlebee commented Apr 23, 2026

Refactor and modularize CLI path resolution #781 to be used in generic way by other CLI providers.

Describe the changes you have made in this PR -

  • Added app/integrations/llm_cli/binary_resolver.py as a shared resolver for CLI binaries.
  • Refactored CodexAdapter in app/integrations/llm_cli/codex.py to use the shared resolver (env -> PATH -> fallback paths) while preserving compatibility behavior.
  • Updated app/integrations/llm_cli/AGENTS.md with up-to-date guidance, optional env handling (CODEX_MODEL, CODEX_BIN), and a provider checklist.
  • Kept resolver behavior aligned with issue intent: CODEX_BIN and CODEX_MODEL remain optional and fallback resolution remains active when unset/invalid.

Screenshots of the UI changes (If any) -

  • N/A (no UI changes)

Code Understanding and AI Usage

Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?

  • No, I wrote all the code myself
  • Yes, I used AI assistance (continue below)

If you used AI assistance:

  • I have reviewed every single line of the AI-generated code
  • I can explain the purpose and logic of each function/component I added
  • I have tested edge cases and understand how the code handles them
  • I have modified the AI output to follow this project's coding standards and conventions

Explain your implementation approach:
I extracted executable lookup concerns from the Codex adapter into a reusable utility module so new CLI adapters can share one consistent resolution strategy. The resolver keeps explicit override support (*_BIN) while preserving robust PATH and conventional install path fallback behavior.

I chose this approach over duplicating resolver code into future adapters because it reduces repeated platform-specific logic and makes behavior changes easier to maintain in one place. I retained compatibility hooks that existing tests expect in codex.py to avoid regressions while still reducing adapter complexity.


Checklist before requesting a review

  • I have added proper PR title and linked to the issue
  • I have performed a self-review of my code
  • I can explain the purpose of every function, class, and logic block I added
  • I understand why my changes work and have tested them thoroughly
  • I have considered potential edge cases and how my code handles them
  • If it is a core feature, I have added thorough tests
  • My code follows the project's style guidelines and conventions

Concise explainer: this PR introduces a shared CLI binary resolver and updates Codex integration/docs so optional envs and fallback behavior are explicit and reusable for upcoming CLI provider sub-issues.

- add shared binary resolver utilities for env, PATH, and fallback path resolution
- refactor Codex adapter to use the shared resolver while keeping compatibility behavior
- update llm_cli AGENTS guidance with optional env usage and provider implementation checklist

Made-with: Cursor
Comment thread app/integrations/llm_cli/codex.py Fixed
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 23, 2026

Greptile Summary

This PR extracts binary resolution logic from CodexAdapter into a new shared utility module (binary_resolver.py) so future CLI adapters can reuse a single, consistent env → PATH → fallback resolution strategy. The refactoring is structurally clean and the new module is well-designed.

Key findings:

  • Broken test compatibility (P1): The PR moves all platform-specific branching (sys.platform checks) into binary_resolver.py, but five existing tests patch app.integrations.llm_cli.codex.sys.platform expecting to influence that logic. The _ = sys.platform shim added to _fallback_codex_paths is a no-op — it references the name in codex's namespace but default_cli_fallback_paths and npm_prefix_bin_dirs read from binary_resolver.sys.platform, which remains unpatched. The affected tests (test_fallback_paths_include_macos_defaults, test_fallback_paths_include_windows_defaults, test_fallback_paths_include_env_and_npm_prefix, test_npm_prefix_bin_dirs_windows_uses_prefix_root, test_npm_prefix_bin_dirs_unix_uses_prefix_bin) will fail on any CI host whose real platform does not match the patched value.
  • lru_cache cross-test risk (P2): npm_prefix_bin_dirs is now a shared, module-level cached function. Existing tests already cache_clear() it manually, but as more adapters adopt this module the pattern will need to be enforced via a shared test fixture to prevent contamination.
  • No new unit tests for binary_resolver.py: The testing convention requires new features to have corresponding tests; binary_resolver.py is only indirectly exercised via the codex tests, and those tests are partially broken by the refactor.

Confidence Score: 2/5

Not safe to merge as-is — existing tests will fail on Linux/macOS CI because the sys.platform patch target moved but the tests were not updated.

The refactoring intent and the new module are well-structured, but the compatibility shim introduced in _fallback_codex_paths does not actually preserve test isolation for platform-specific branching. Five tests that rely on patch("...codex.sys.platform", ...) now test the wrong module namespace and will produce wrong results on non-matching platforms, which is a concrete CI reliability issue rather than a theoretical one.

Focus on app/integrations/llm_cli/codex.py (lines 85–91) and tests/integrations/llm_cli/test_codex_adapter.py (all five sys.platform-patching tests).

Important Files Changed

Filename Overview
app/integrations/llm_cli/binary_resolver.py New shared resolver module — logic is correct and well-structured; the lru_cache on npm_prefix_bin_dirs may cause cross-test contamination as more adapters adopt this module.
app/integrations/llm_cli/codex.py Refactored to delegate to the shared resolver, but the _ = sys.platform compatibility shim is a no-op — existing tests that patch codex.sys.platform no longer work because all platform checks now run in binary_resolver.sys.platform.
app/integrations/llm_cli/AGENTS.md Documentation update — adds binary_resolver.py to layout table, binary resolution order, env quick reference, and a provider checklist. Content is accurate and helpful.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CodexAdapter._resolve_binary] --> B[resolve_cli_binary in binary_resolver.py]
    B --> C{CODEX_BIN set and runnable?}
    C -- Yes --> D[Return explicit path]
    C -- No --> E[shutil.which each candidate_binary_names]
    E --> F{Found on PATH?}
    F -- Yes --> G[Return PATH hit]
    F -- No --> H[_fallback_codex_paths in codex.py]
    H --> I[default_cli_fallback_paths in binary_resolver.py]
    I --> J{npm_prefix_bin_dirs cached lookup}
    J --> K[Return ordered fallback list]
    K --> L{Any candidate runnable?}
    L -- Yes --> M[Return first runnable]
    L -- No --> N[Return None]
Loading

Reviews (1): Last reviewed commit: "feat(llm-cli): centralize cli binary res..." | Re-trigger Greptile

Comment thread app/integrations/llm_cli/codex.py
Comment thread app/integrations/llm_cli/codex.py Outdated
Comment thread app/integrations/llm_cli/binary_resolver.py
@muddlebee
Copy link
Copy Markdown
Contributor Author

muddlebee commented Apr 23, 2026

@Devesh36 @rrajan94 @yashksaini-coder quick refactor on top of my recent CLI PR :D
just making the "path finding" utility modular so no duplication for further CLI integration..

then we can open issues for new CLI like gemini and others..

@muddlebee muddlebee changed the title feat(llm-cli): centralize CLI binary resolution feat(llm-cli): modularize CLI binary resolution Apr 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants