Conversation
Promote unresolved security review items into a canonical maintainer backlog, move the threat model under maintainer docs, and link both from the docs index and package maintenance guide. Co-authored-by: Codex <noreply@openai.com>
Ensure the pacman package step does not preserve ownership from the staged package tree, so installed system paths remain root-owned and the updater can trust the packaged builder bundle. Co-authored-by: Codex <noreply@openai.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads provenance-aware CLI resolution and source metadata from installer through updater; adds maintainer-facing threat model and security-backlog docs; enforces loud failure for invalid configured CLI paths; hardens preflight, discovery, launcher flow, packaging copy ownership, tests, and CI workflow. Changes
sequenceDiagram
participant Installer as Installer (install.sh)
participant Updater as Updater (codex-app-updater)
participant Resolver as CLI Resolver
participant FS as Filesystem
participant Codex as Codex CLI
Installer->>Updater: run cli-preflight (maybe --cli-path)
Updater->>Resolver: resolve_runtime_cli_path(&RuntimeConfig, persisted_state, explicit_arg)
alt explicit/env/config candidate
Resolver->>FS: stat candidate (absolute? regular file + exec bit?)
FS-->>Resolver: ok / invalid
Resolver-->>Updater: (path, cli_path_source) or InvalidConfiguredCliPath
else persisted / PATH / known fallbacks
Resolver->>FS: check persisted path, PATH entries, known locations
FS-->>Resolver: found / not found
Resolver-->>Updater: (path, cli_path_source) or none (install needed)
end
alt install required
Updater->>Updater: perform install / auto-install
Updater-->>Resolver: installed path or persist failure
end
Updater->>Updater: persist cli_path and cli_path_source (or persist failure)
Updater-->>Installer: PreflightOutcome {cli_path, cli_path_source, logs, exit_code}
alt invalid configured candidate detected
Installer->>Installer: exit 78, export PREFLIGHT_* env vars, print stored error
else allowed
Installer->>Codex: launch using resolved path
Codex-->>Installer: started / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves Codex CLI discovery by persisting the resolved CLI path (and its provenance), supporting a partial runtime config overlay (cli_path), and updating the launcher to let the updater preflight determine CODEX_CLI_PATH before direct fallbacks.
Changes:
- Add
cli_path_sourcetracking to persisted state and surface it viastatus. - Make
config.tomlload as an overlay (including optionalcli_path) rather than requiring full config. - Expand CLI discovery precedence (explicit/env/config/persisted/PATH/known locations) and update the launcher + smoke tests accordingly.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| updater/src/state.rs | Persist cli_path_source alongside cli_path for reporting/debugging and backward-compatible loading. |
| updater/src/config.rs | Parse config as an overlay on defaults; add optional cli_path. |
| updater/src/codex_cli.rs | Implement multi-source CLI path resolution with provenance tracking and stricter executability checks. |
| updater/src/app.rs | Thread RuntimeConfig into CLI preflight/status flows; include CLI path/source in status output. |
| updater/src/builder.rs | Update tests/fixtures to include new cli_path field in RuntimeConfig. |
| install.sh | Let updater preflight populate CODEX_CLI_PATH before direct PATH fallback logic. |
| packaging/linux/PKGBUILD.template | Preserve ownership behavior in pacman packaging via cp --no-preserve=ownership. |
| tests/scripts_smoke.sh | Extend smoke tests for PKGBUILD copy flags and launcher updater-driven CLI discovery. |
| docs/usage/troubleshooting.md | Document cli_path config override and cli_path_source reporting. |
| docs/maintainers/* | Add maintainer-facing threat model + security backlog and cross-link them. |
| docs/README.md | Add index links for threat model and security backlog. |
| security_best_practices_report.md | Remove point-in-time report from repo root (replaced by backlog/threat model). |
Comments suppressed due to low confidence (1)
updater/src/codex_cli.rs:189
- Same as preflight: when no CLI is found, the persisted
cli_error_messageclaims the updater only checkedCODEX_CLI_PATH/PATH, but resolution also considers config, persisted state, and known fallback locations. Adjust this message to match the actual search behavior.
state.cli_path = None;
state.cli_path_source = CliPathSource::Unknown;
state.cli_installed_version = None;
state.cli_status = CliStatus::Unknown;
state.cli_error_message =
Some("Codex CLI not found in CODEX_CLI_PATH or PATH".to_string());
persist_state(paths, state)?;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac2da3376e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 673-677: The launcher currently assumes CODEX_CLI_PATH is set when
codex-app-updater exists, which can cause an unbound variable error under set -u
after calling run_cli_preflight; update the conditional and any later expansions
(references: run_cli_preflight, CODEX_CLI_PATH, codex-app-updater) to safely
test and expand the variable (e.g., check -n "${CODEX_CLI_PATH:-}" or use
parameter expansion like "${CODEX_CLI_PATH:-}" wherever it's used) so fallback
discovery still runs even if run_cli_preflight failed to populate CODEX_CLI_PATH
and no unbound-variable exits occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc9c3456-3a19-45c3-aa6b-59f7669a1722
📒 Files selected for processing (14)
docs/README.mddocs/maintainers/package-runtime-maintenance.mddocs/maintainers/security-backlog.mddocs/maintainers/threat-model.mddocs/usage/troubleshooting.mdinstall.shpackaging/linux/PKGBUILD.templatesecurity_best_practices_report.mdtests/scripts_smoke.shupdater/src/app.rsupdater/src/builder.rsupdater/src/codex_cli.rsupdater/src/config.rsupdater/src/state.rs
💤 Files with no reviewable changes (1)
- security_best_practices_report.md
nisavid
force-pushed
the
codex-cli-discovery
branch
from
ac2da33 to
6a789fa
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@updater/src/codex_cli.rs`:
- Around line 958-987: Add a third case to the existing test
invalid_explicit_and_config_paths_fail_loudly that mirrors the explicit and
config checks but passes the same non-executable file as env_path in the
ResolveCliPathRequest; call resolve_cli_path(&ResolveCliPathRequest {
explicit_path: None, env_path: Some(invalid.as_path()), config_path: None,
persisted_path: None, path_env: None, home: Some(temp.path()), fallback_env:
Vec::new() }) and assert it returns an error whose message contains "env"
(similar to the other two assertions) so env_path is covered by the same
validation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d319a34-313a-4b9c-ab44-47c3eaa08720
📒 Files selected for processing (9)
docs/maintainers/package-runtime-maintenance.mddocs/usage/troubleshooting.mdinstall.shtests/scripts_smoke.shupdater/src/app.rsupdater/src/builder.rsupdater/src/codex_cli.rsupdater/src/config.rsupdater/src/state.rs
nisavid
force-pushed
the
codex-cli-discovery
branch
from
6a789fa to
5c695aa
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@updater/src/codex_cli.rs`:
- Around line 997-999: The test currently checks
env_error.to_string().contains("env"), which is fragile; update the assertion to
check the full label returned by cli_path_source_label for clarity — either
assert that env_error.to_string().contains("environment") or, better,
assert_eq!(env_error.to_string(), cli_path_source_label("environment")) so the
test exactly matches the label produced by cli_path_source_label and ensures
consistency with that function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac0c319c-c27d-4eb5-92c3-19223c62c00c
📒 Files selected for processing (9)
docs/maintainers/package-runtime-maintenance.mddocs/usage/troubleshooting.mdinstall.shtests/scripts_smoke.shupdater/src/app.rsupdater/src/builder.rsupdater/src/codex_cli.rsupdater/src/config.rsupdater/src/state.rs
nisavid
force-pushed
the
codex-cli-discovery
branch
from
5c695aa to
cad07d6
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.sh (1)
675-689:⚠️ Potential issue | 🟠 MajorDon't fall back to
PATHafter an explicit/configured CLI path fails validation.
run_cli_preflight 0still downgrades updater errors to a warning, and the next block at Line 677 immediately falls back tofind_codex_cli. That means a badCODEX_CLI_PATHorcli_pathcan quietly launch against some othercodexonPATH, which defeats the new provenance/“fail loudly” behavior documented indocs/maintainers/package-runtime-maintenance.md. Keep the best-effort fallback for transient updater failures, but stop before PATH fallback when preflight rejects an explicit, environment, or configured path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 675 - 689, The script currently calls run_cli_preflight and then unconditionally falls back to find_codex_cli, which lets a user-supplied or configured CODEX_CLI_PATH be silently replaced by a PATH candidate when preflight rejects it; change the control flow so that if CODEX_CLI_PATH was explicitly provided (env/config/cli_path) and run_cli_preflight indicates it’s invalid, do not attempt find_codex_cli on PATH but instead surface the failure (exit or prompt to reinstall) — i.e., detect whether CODEX_CLI_PATH was set prior to the run_cli_preflight call and only attempt find_codex_cli when it was empty initially; keep the existing best-effort fallback behavior for transient updater failures but stop before PATH fallback when preflight fails for an explicit CODEX_CLI_PATH (referencing run_cli_preflight, CODEX_CLI_PATH, find_codex_cli, and prompt_install_missing_cli).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@updater/src/app.rs`:
- Around line 199-225: The formatted status output is using Debug ({:?}) for
enum fields and prints Rust variant names; implement a stable snake_case string
representation for those enums (e.g., implement Display for CliStatus and
CliPathSource to return "up_to_date", "known_path", etc.) and then change
status_text to use {} for cli_status and cli_path_source (or call
.to_string()/as_str() if you add a helper method). Update the enums' Display
implementations to return the canonical snake_case names and use those symbols
(CliStatus, CliPathSource, status_text) so the human-readable output matches the
JSON/state stable names.
In `@updater/src/codex_cli.rs`:
- Around line 678-683: Document the limitation of the permission check in the
is_executable function: add a short doc comment above fn is_executable(path:
&Path) explaining that mode & 0o111 != 0 only checks that some execute bit is
set and does not guarantee the current user can execute the file (e.g.,
root-owned files with execute bit for owner only), and mention that a more
robust alternative would be nix::unistd::access(path, AccessFlags::X_OK) if you
wanted to add that dependency. Ensure the comment clearly states that this is a
deliberate tradeoff and that actual execution will fail at runtime if
permissions prevent it.
In `@updater/src/config.rs`:
- Around line 31-43: The RuntimeConfigOverlay struct currently allows unknown
keys during deserialization which hides typos like "cli_pth"; add the Serde
attribute to deny unknown fields on the RuntimeConfigOverlay definition (i.e.,
annotate the struct with #[serde(deny_unknown_fields)]) so serde will error on
unexpected keys during parsing; update any imports if needed and run the config
deserialization code paths (RuntimeConfigOverlay) to ensure the failure surfaces
appropriately.
---
Outside diff comments:
In `@install.sh`:
- Around line 675-689: The script currently calls run_cli_preflight and then
unconditionally falls back to find_codex_cli, which lets a user-supplied or
configured CODEX_CLI_PATH be silently replaced by a PATH candidate when
preflight rejects it; change the control flow so that if CODEX_CLI_PATH was
explicitly provided (env/config/cli_path) and run_cli_preflight indicates it’s
invalid, do not attempt find_codex_cli on PATH but instead surface the failure
(exit or prompt to reinstall) — i.e., detect whether CODEX_CLI_PATH was set
prior to the run_cli_preflight call and only attempt find_codex_cli when it was
empty initially; keep the existing best-effort fallback behavior for transient
updater failures but stop before PATH fallback when preflight fails for an
explicit CODEX_CLI_PATH (referencing run_cli_preflight, CODEX_CLI_PATH,
find_codex_cli, and prompt_install_missing_cli).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01f9f179-5f8b-4ab0-8624-a98d685af555
📒 Files selected for processing (9)
docs/maintainers/package-runtime-maintenance.mddocs/usage/troubleshooting.mdinstall.shtests/scripts_smoke.shupdater/src/app.rsupdater/src/builder.rsupdater/src/codex_cli.rsupdater/src/config.rsupdater/src/state.rs
nisavid
force-pushed
the
codex-cli-discovery
branch
from
cad07d6 to
9b88af2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b88af2794
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.sh (1)
496-504:⚠️ Potential issue | 🟠 MajorValidate
CODEX_CLI_PATHlocally when preflight didn’t verify it.If
codex-app-updateris missing—or exits before it can produce the structured “Invalid … Codex CLI path” error—the launcher only checks whetherCODEX_CLI_PATHis non-empty. A stale env override then suppressesfind_codex_cliand is passed straight to Electron, so checkout installs lose the new loud-failure behavior.Add a local executable-file check before honoring
CODEX_CLI_PATH, regardless of whether preflight ran.Suggested fix
+is_valid_cli_path() { + [ -n "${1:-}" ] && [ -f "$1" ] && [ -x "$1" ] +} + @@ if ! run_cli_preflight 0; then if is_configured_cli_preflight_failure "$CODEX_CLI_PREFLIGHT_ERROR"; then notify_error "$CODEX_CLI_PREFLIGHT_ERROR" exit 1 @@ fi + +if [ -n "${CODEX_CLI_PATH:-}" ] && ! is_valid_cli_path "$CODEX_CLI_PATH"; then + notify_error "Invalid environment Codex CLI path $CODEX_CLI_PATH: expected an executable regular file" + exit 1 +fi if [ -z "${CODEX_CLI_PATH:-}" ]; then CODEX_CLI_PATH="$(find_codex_cli || true)" export CODEX_CLI_PATH fiAlso applies to: 687-713
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 496 - 504, The preflight path logic in run_cli_preflight (and the similar block later) must validate any CODEX_CLI_PATH env override locally before using it: if CODEX_CLI_PATH is set but does not point to an existing executable file (use test -f and -x), clear/unset it (or set CODEX_CLI_PATH="") so find_codex_cli can run or produce the proper error; update run_cli_preflight to perform this local executable-file check before returning success for a non-empty CODEX_CLI_PATH, and mirror the same check in the other block handling CODEX_CLI_PATH.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@updater/src/codex_cli.rs`:
- Around line 341-350: The env-derived fallback roots (in current_fallback_env)
must be filtered to reject empty or relative values: change the filter_map so
that after mapping std::env::var_os(name) to a PathBuf you only return (name,
path) when the OsString is non-empty and path.is_absolute() is true; apply the
same check to the other fallback-building logic referenced around lines 354-389
so HOME and package-manager root env vars never produce empty or relative paths.
- Around line 268-317: resolve_cli_path can return relative or caller-relative
paths (e.g. "./codex" or PATH hits) which get persisted; canonicalize the chosen
path before returning so the stored state is an absolute, stable path. In
resolve_cli_path (and where it constructs CliPathResolution for any source:
Explicit, Env, Config, Persisted, Path, KnownPath), call std::fs::canonicalize
on the selected Path (or if canonicalize fails, make it absolute via
std::env::current_dir().join(path).canonicalize fallback) and use that
canonical/absolute PathBuf in the CliPathResolution returned; keep using
is_executable/find_in_path/known_cli_paths for discovery but always normalize
the final path before constructing CliPathResolution.
In `@updater/src/state.rs`:
- Around line 343-377: The PR is missing the required validation step running
"cargo check -p codex-app-updater"; update the CI/validation configuration to
include this command (so changes to updater state/CLI are type-checked).
Specifically, ensure the updater job that runs tests for
PersistedState::load_or_default and tests like
loads_state_without_cli_path_source also runs "cargo check -p codex-app-updater"
(or add a new step) so the codex-app-updater package is type-checked as part of
PR validation.
---
Outside diff comments:
In `@install.sh`:
- Around line 496-504: The preflight path logic in run_cli_preflight (and the
similar block later) must validate any CODEX_CLI_PATH env override locally
before using it: if CODEX_CLI_PATH is set but does not point to an existing
executable file (use test -f and -x), clear/unset it (or set CODEX_CLI_PATH="")
so find_codex_cli can run or produce the proper error; update run_cli_preflight
to perform this local executable-file check before returning success for a
non-empty CODEX_CLI_PATH, and mirror the same check in the other block handling
CODEX_CLI_PATH.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e05d3176-5f25-43e1-9a39-70073719dce1
📒 Files selected for processing (9)
docs/maintainers/package-runtime-maintenance.mddocs/usage/troubleshooting.mdinstall.shtests/scripts_smoke.shupdater/src/app.rsupdater/src/builder.rsupdater/src/codex_cli.rsupdater/src/config.rsupdater/src/state.rs
nisavid
force-pushed
the
codex-cli-discovery
branch
from
9b88af2 to
b4380b8
Compare
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/usage/troubleshooting.md`:
- Around line 85-90: The docs omission: add "unknown" to the listed
cli_path_source values in docs/usage/troubleshooting.md because
updater/src/app.rs::status_text() emits "unknown" when no CLI is resolved;
update the paragraph describing resolver sources to include "unknown" and a
brief sentence that this indicates no CLI was found (recommended place: the
block that currently lists env, config, persisted, path, known_path) and mirror
the suggested wording so users see the missing-CLI case when troubleshooting.
In `@install.sh`:
- Around line 625-631: The fallback should ignore relative PATH hits from `type
-P codex` so the shell resolver matches the Rust updater behavior: after
obtaining `resolved="$(type -P codex 2>/dev/null || true)"`, detect whether
`resolved` is an absolute path (starts with '/'); if it is absolute, call
`absolute_path "$resolved"` and proceed to `is_valid_cli_path "$resolved"`, but
if it is a relative path (does not start with '/'), clear or leave `resolved`
empty so the relative PATH entry is skipped and not converted to a cwd-dependent
absolute path.
- Around line 525-540: The failure branch captures CODEX_CLI_PREFLIGHT_ERROR
from preflight_stderr but never writes the raw stderr to stderr before cleanup;
update the block handling preflight failures (the branch that checks if [
"$preflight_status" -ne 0 ]) to emit the contents of "$preflight_stderr" to
stderr (e.g., cat "$preflight_stderr" >&2) before removing the file and
returning, so diagnostic details are preserved; keep the existing assignments to
CODEX_CLI_PREFLIGHT_STATUS and CODEX_CLI_PREFLIGHT_ERROR, preserve the
refreshed_path/is_valid_cli_path logic, and ensure the cat happens before rm -f
"$preflight_stderr" and the return statements (including the
allow_install_missing early-return).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0fd95509-8753-4f60-9d86-3f5ab80ab2f3
📒 Files selected for processing (11)
.github/workflows/updater.ymldocs/maintainers/package-runtime-maintenance.mddocs/usage/troubleshooting.mdinstall.shtests/scripts_smoke.shupdater/src/app.rsupdater/src/builder.rsupdater/src/codex_cli.rsupdater/src/config.rsupdater/src/main.rsupdater/src/state.rs
nisavid
force-pushed
the
codex-cli-discovery
branch
from
51e5a90 to
115e9ab
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
updater/src/state.rs (1)
343-377:⚠️ Potential issue | 🟡 MinorPlease include
cargo check -p codex-app-updaterin validation evidence.The listed validation commands include updater tests but not the required updater type-check step.
As per coding guidelines, "For updater changes, run
cargo check -p codex-app-updaterand targeted updater tests; run full updater tests for state, install, or CLI changes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@updater/src/state.rs` around lines 343 - 377, Update the PR validation evidence to run the required type-check for the updater: add the command `cargo check -p codex-app-updater` to the validation steps (in addition to the existing updater tests), and re-run the targeted tests (e.g., the PersistedState tests that call PersistedState::load_or_default and check CliPathSource) so the CI output includes both the cargo check and the test results; ensure the updated evidence explicitly shows the cargo check command and its successful completion alongside the test logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/scripts_smoke.sh`:
- Around line 993-1046: Update the test
test_launcher_rejects_invalid_configured_cli_before_path_fallback to record and
assert the updater invocation: change the fake updater (the file created at
"$bin_dir/codex-app-updater") to write its argv to a recorder file in the
workspace (e.g. "$workspace/updater-invocation.log") before exiting, run the
generated launcher as before, then add an assertion that the recorder contains
the expected preflight invocation (e.g. contains "cli-preflight" and the
configured path argument or pattern used by other discovery tests). Keep the
existing exit code and log assertions (launcher_status == 78, launcher_log
contains "Invalid config Codex CLI path") and add assert_contains
"$workspace/updater-invocation.log" "cli-preflight" (and/or the configured path)
to ensure the preflight call actually happened.
---
Duplicate comments:
In `@updater/src/state.rs`:
- Around line 343-377: Update the PR validation evidence to run the required
type-check for the updater: add the command `cargo check -p codex-app-updater`
to the validation steps (in addition to the existing updater tests), and re-run
the targeted tests (e.g., the PersistedState tests that call
PersistedState::load_or_default and check CliPathSource) so the CI output
includes both the cargo check and the test results; ensure the updated evidence
explicitly shows the cargo check command and its successful completion alongside
the test logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6fad672-412b-41d3-aafe-37019e1f2cb6
📒 Files selected for processing (11)
.github/workflows/updater.ymldocs/maintainers/package-runtime-maintenance.mddocs/usage/troubleshooting.mdinstall.shtests/scripts_smoke.shupdater/src/app.rsupdater/src/builder.rsupdater/src/codex_cli.rsupdater/src/config.rsupdater/src/main.rsupdater/src/state.rs
nisavid
force-pushed
the
codex-cli-discovery
branch
from
115e9ab to
ed75234
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed75234009
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
nisavid
force-pushed
the
codex-cli-discovery
branch
from
ed75234 to
afbed70
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
nisavid
force-pushed
the
codex-cli-discovery
branch
from
afbed70 to
2fbb446
Compare
nisavid
dismissed
coderabbitai[bot]’s stale review
Stale CodeRabbit review on an older commit. The actionable test-recorder feedback was addressed, validation passed, and current PR checks are green.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@updater/src/codex_cli.rs`:
- Around line 118-124: Move the state update/persist so the resolved path is
saved before attempting to read the installed version: set state.cli_path =
Some(cli_path.clone()) and state.cli_path_source = resolution.source.clone() and
call persist_state(paths, state) immediately after computing cli_path (before
calling read_installed_version(&cli_path)); then call
read_installed_version(&cli_path) and, if it succeeds, set
state.cli_installed_version = Some(installed_version.clone()) and
persist_state(paths, state) again. Ensure you still clone/use resolution.path as
cli_path and keep error propagation from read_installed_version unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56389438-c6d2-4e18-9025-80cadfc9eaf2
📒 Files selected for processing (11)
.github/workflows/updater.ymldocs/maintainers/package-runtime-maintenance.mddocs/usage/troubleshooting.mdinstall.shtests/scripts_smoke.shupdater/src/app.rsupdater/src/builder.rsupdater/src/codex_cli.rsupdater/src/config.rsupdater/src/main.rsupdater/src/state.rs
Add source-aware Codex CLI resolution across explicit paths, CODEX_CLI_PATH, updater config, persisted state, launch PATH, and known user package-manager fallback paths. Let the generated launcher rely on updater discovery before direct command lookup, and surface the selected CLI path/source in updater status. Co-authored-by: Codex <noreply@openai.com>
nisavid
force-pushed
the
codex-cli-discovery
branch
from
2fbb446 to
254b912
Compare
nisavid
dismissed
coderabbitai[bot]’s stale review
Stale CodeRabbit review on the previous commit. The resolved-path persistence issue is fixed in 254b912 with a regression test, and local updater validation passed.
Summary
Make Codex App resolve and persist the Codex CLI path the user intentionally installed. The updater now records where the selected path came from, supports a partial config overlay with
cli_path, and falls back through launch PATH plus common user package-manager install locations.The generated launcher now lets
codex-app-updater cli-preflight --print-pathresolve the CLI before using its direct fallback, so package-installed updater services can teach Electron the rightCODEX_CLI_PATHwithout sourcing shell startup files.Impact
Users can set only
cli_path = "/path/to/codex"in~/.config/codex-app-updater/config.toml, andcodex-app-updater statusreports bothcli_pathandcli_path_source. Old state files remain compatible.Validation
cargo test -p codex-app-updater codex_clicargo test -p codex-app-updaterbash -n install.sh tests/scripts_smoke.shtests/scripts_smoke.shgit diff --checkSummary by CodeRabbit
Greptile Summary
This PR introduces structured Codex CLI path discovery and persistence across the updater daemon, preflight command, and generated launcher. The resolver now walks a priority chain (explicit → env → config → persisted → PATH → known fallback paths), records the winning source in
CliPathSource, persists it to state, and surfaces bothcli_pathandcli_path_sourceincodex-app-updater status. Invalid env or config paths now fail fast with exit code 78 instead of silently falling through.Confidence Score: 5/5
Safe to merge; remaining findings are P2 style/cleanup items that do not affect correctness.
All logic changes are covered by unit and smoke tests. Backward compatibility is preserved via #[serde(default)]. The inline comments flag a redundant branch and a Debug-vs-Display UX nit — neither blocks the feature.
updater/src/main.rs (Debug format on user-facing error), install.sh (minor cleanup opportunities)
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[resolve_runtime_cli_path] --> B{explicit_path set?} B -- yes --> C{absolute?} C -- no --> ERR1[Err: must be absolute] C -- yes --> D{executable?} D -- yes --> RES1[✓ Explicit] D -- no --> ERR2[Err: not executable] B -- no --> E{env CODEX_CLI_PATH set?} E -- yes --> F{absolute?} F -- no --> ERR3[Err: must be absolute] F -- yes --> G{executable?} G -- yes --> RES2[✓ Env] G -- no --> ERR4[Err: not executable] E -- no --> H{config cli_path set?} H -- yes --> I{absolute?} I -- no --> ERR5[Err: must be absolute] I -- yes --> J{executable?} J -- yes --> RES3[✓ Config] J -- no --> ERR6[Err: not executable] H -- no --> K{persisted cli_path set?} K -- yes --> L{absolute and executable?} L -- yes --> RES4[✓ Persisted] L -- no --> M[skip - stale] K -- no --> M M --> N{codex in PATH?} N -- yes --> RES5[✓ Path] N -- no --> O{known fallback paths} O -- found --> RES6[✓ KnownPath] O -- not found --> NONE[None - CLI missing]Comments Outside Diff (2)
install.sh, line 383-388 (link)When
codex-app-updaterexits non-zero but prints a path to stdout (while leaving stderr empty),CODEX_CLI_PREFLIGHT_ERRORends up set to the CLI path string (e.g.,/home/user/.local/bin/codex) rather than a human-readable error description. This string then gets passed tonotify_errorin the caller, potentially showing a path as the error notification text.Prompt To Fix With AI
install.sh, line 482-483 (link)local candidatedeclarationcandidateis already declared aslocal candidate=""a few lines above the IFS loop. The second barelocal candidatebefore the array loop is redundant and does not reset the value in bash.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "feat(updater): persist discovered Codex ..." | Re-trigger Greptile