Conversation
![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: 633df7c989
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tmp_link="$BIN_DIR/.codex.$$" | ||
|
|
||
| replace_path_with_symlink "$BIN_PATH" "$CURRENT_LINK/codex" "$tmp_link" | ||
| replace_path_with_symlink "$BIN_PATH" "$CURRENT_LINK/bin/codex" "$tmp_link" |
There was a problem hiding this comment.
current/codex compatibility path
Switching the visible shim to "$CURRENT_LINK/bin/codex" means fresh installs and upgrades no longer leave a binary at CODEX_HOME/packages/standalone/current/codex, but app-server daemon code still hard-codes that legacy path (codex-rs/app-server-daemon/src/managed_install.rs::managed_codex_bin). In that state, daemon flows that call ensure_managed_codex_bin() fail even immediately after a successful install, because they check for a file that no longer exists. Keep a compatibility shim at current/codex (or update daemon path resolution in the same change) to avoid breaking managed daemon startup/update flows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the latest push. Package installs now create current/codex as a compatibility symlink to bin/codex, so the managed daemon path continues to work while the visible shim can still target the package layout.
| download_file "$checksum_url" "$checksum_path" | ||
| expected_digest="$(package_archive_digest "$asset" "$checksum_path")" | ||
| download_file "$download_url" "$archive_path" | ||
| verify_archive_digest "$archive_path" "$expected_digest" |
There was a problem hiding this comment.
The script now derives expected_digest directly from the downloaded codex-package_SHA256SUMS file and uses it to validate the archive, but it never validates that manifest against an independent trusted digest. This makes integrity verification circular: if an attacker can tamper with both downloads, they can provide a malicious archive plus matching manifest and still pass verify_archive_digest. Validate the checksum manifest itself (for example via the release API digest) before using it as the trust source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the latest push. install.sh now verifies codex-package_SHA256SUMS against the checksum asset's GitHub release metadata digest before using it to validate the selected package archive.
viyatb-oai
left a comment
viyatb-oai
left a comment
There was a problem hiding this comment.
Two P2 notes from the installer/package-archive review pass.
|
|
||
| step "Downloading Codex CLI" | ||
| expected_digest="$(release_asset_digest "$asset" "$resolved_version")" | ||
| download_file "$checksum_url" "$checksum_path" |
There was a problem hiding this comment.
[P2] This weakens the Unix installer’s integrity chain. Before this stack, install.sh checked the downloaded archive against GitHub release metadata’s asset.digest; now it trusts a checksum manifest fetched from the same release asset channel as the archive itself. If that response path is swapped, an attacker can replace both the archive and the manifest and still pass verification. The PowerShell path already avoids this by authenticating the manifest against release metadata first.
Please keep the release-asset digest check for the checksum manifest here, then consume the package digest from the verified manifest.
| download_file "$checksum_url" "$checksum_path" | |
| checksum_digest="$(release_asset_digest "$checksum_asset" "$resolved_version")" | |
| download_file "$checksum_url" "$checksum_path" | |
| verify_archive_digest "$checksum_path" "$checksum_digest" | |
| expected_digest="$(package_archive_digest "$asset" "$checksum_path")" | |
| download_file "$download_url" "$archive_path" | |
| verify_archive_digest "$archive_path" "$expected_digest" |
There was a problem hiding this comment.
Addressed in the latest push. The Unix installer now authenticates codex-package_SHA256SUMS via release_asset_digest before reading the package archive digest from it, matching the PowerShell integrity chain.
|
|
||
| resolved_version="$(resolve_version)" | ||
| asset="codex-npm-$npm_tag-$resolved_version.tgz" | ||
| asset="codex-package-$vendor_target.tar.gz" |
There was a problem hiding this comment.
[P2] This makes pinned historical installs fail. Older Rust releases such as rust-v0.39.0 and rust-v0.40.0 publish the legacy codex-npm-<version>.tgz asset, but not codex-package-* or codex-package_SHA256SUMS, so install.sh --release 0.40.0 now 404s before extraction. install.ps1 has the same cutover issue.
Please keep a legacy fallback when the new package archive family is unavailable, or gate the new asset names on the release version and retain the prior npm-tarball flow below that cutover.
| asset="codex-package-$vendor_target.tar.gz" | |
| asset="codex-package-$vendor_target.tar.gz" | |
| checksum_asset="codex-package_SHA256SUMS" | |
| if ! release_asset_exists "$asset" "$resolved_version" || ! release_asset_exists "$checksum_asset" "$resolved_version"; then | |
| asset="codex-npm-$npm_tag-$resolved_version.tgz" | |
| checksum_asset="" | |
| fi |
There was a problem hiding this comment.
Addressed for the relevant recent-release shape in the latest push. The installers now fall back to the per-platform codex-npm-<platform>-<version>.tgz assets when codex-package-* assets are unavailable. After discussion, I did not add special handling for the much older single-root codex-npm-<version>.tgz releases like 0.39.0/0.40.0.
There was a problem hiding this comment.
I also updated the PR body's compatibility notes to make the fallback boundary explicit: recent pre-package releases that have per-platform npm artifacts remain supported, while much older releases that only have the single root codex-npm-<version>.tgz archive are intentionally out of scope.
## Summary The Linux sandbox should find bundled `bwrap` through the same package-layout abstraction as the rest of the runtime, instead of maintaining a separate standalone-specific lookup path. This adds an `InstallContext` helper for bundled resources and updates `codex-linux-sandbox` to ask the current install context for `codex-resources/bwrap` before falling back to the old executable-relative probes. The tests cover npm-style, standalone, and canonical package layouts so `bwrap` lookup follows the package structure introduced earlier in the stack. ## Test plan - `cargo test -p codex-install-context` - `cargo test -p codex-linux-sandbox --lib` - `just fix -p codex-install-context -p codex-linux-sandbox` - `just bazel-lock-check` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/23634). * #23638 * #23637 * #23636 * #23635 * __->__ #23634
## Summary Standalone installers and other downstream package consumers need a stable checksum source for the canonical package archives. Relying on per-asset metadata makes that harder to consume uniformly, especially when several package archives are produced in the same release. This keeps the `codex-package-*.tar.gz` and `codex-app-server-package-*.tar.gz` assets in the GitHub Release upload set and adds `codex-package_SHA256SUMS` to `dist/` before the release is created. The manifest contains one SHA-256 line per package archive and fails the release job if no package archives are present. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/23635). * #23638 * #23637 * #23636 * __->__ #23635
bolinfest
force-pushed
the
pr23636
branch
4 times, most recently
from
2a174f4 to
7c42f21
Compare
efrazer-oai
left a comment
efrazer-oai
left a comment
There was a problem hiding this comment.
The specifics look good here, but it appears that we're missing smth on the rust side that we might want to fix before updating the installer.
Right now, windows finds its helpers (command-runner, sandbox-setup.exe) by doing
let dir = exe.parent()?;
let resource_candidate =
dir.join(RESOURCES_DIRNAME).join(file_name);
But now that exe is in bin, this will give us bin/codex-resources which isn't quite right as the installer locates this in top level codex-resources
## Summary
Standalone installs should exercise the same canonical package archive layout that release builds produce, rather than unpacking npm platform packages and reconstructing a parallel install tree.
This updates `install.sh` and `install.ps1` to prefer `codex-package-<target>.tar.gz` plus `codex-package_SHA256SUMS`, authenticate the checksum manifest against GitHub release metadata, verify the selected package archive against the authenticated manifest, and install the package archive directly. Package installs now also leave a compatibility command at `current/codex` for managed daemon flows while visible command shims point at `bin/codex`.
Recent releases that predate package archives still publish per-platform npm artifacts, so both installers keep a legacy platform npm fallback for those versions and verify those archives against release metadata directly. Releases old enough to publish only the single root `codex-npm-<version>.tgz` archive are intentionally out of scope; the installers fail clearly when neither package archives nor per-platform npm archives are present.
On Windows, the runtime helper lookups now also recognize the package layout when `codex.exe` runs from `bin/`, so `codex-command-runner.exe` and `codex-windows-sandbox-setup.exe` resolve from the top-level `codex-resources/` directory while preserving the direct-sibling and older sibling-resource fallbacks.
## Test plan
- `sh -n scripts/install/install.sh`
- `bash -n scripts/install/install.sh`
- `pwsh -NoProfile -Command '$tokens=$null; $errors=$null; $null = [System.Management.Automation.Language.Parser]::ParseFile("scripts/install/install.ps1", [ref]$tokens, [ref]$errors); if ($errors.Count) { $errors | Format-List *; exit 1 }'`
- `HOME="$home_dir" CODEX_HOME="$tmp_dir/codex-home" CODEX_INSTALL_DIR="$bin_dir" PATH="$bin_dir:$PATH" sh scripts/install/install.sh --release 0.125.0`
- Verified the 0.125.0 isolated install leaves the visible command pointed at `current/codex` and includes the legacy `codex-resources/rg` payload.
- `cargo test -p codex-windows-sandbox`
- `just fix -p codex-windows-sandbox`
|
@efrazer-oai addressed in I also added Windows-only unit coverage for those lookup paths. |
|
Windows sandbox changes look correct. There is a little duplication of the "find the exe" logic between |
4 participants
Summary
Standalone installs should exercise the same canonical package archive layout that release builds produce, rather than unpacking npm platform packages and reconstructing a parallel install tree.
This updates
install.shandinstall.ps1to prefercodex-package-<target>.tar.gzpluscodex-package_SHA256SUMSintroduced in #23635, authenticate the checksum manifest against GitHub release metadata, verify the selected package archive against the authenticated manifest, and install the package archive directly.Compatibility Notes
Package installs still leave a compatibility command at
current/codexfor managed daemon flows, while visible command shims point atbin/codexinside the package layout.Recent releases that predate package archives still publish per-platform npm artifacts, so both installers keep a legacy platform npm fallback for those versions and verify those archives against release metadata directly.
Releases old enough to publish only the single root
codex-npm-<version>.tgzarchive are intentionally out of scope. The installers fail clearly when neither package archives nor per-platform npm archives are present.On Windows, the runtime helper lookups now recognize package-layout installs where
codex.exeruns frombin/, socodex-command-runner.exeandcodex-windows-sandbox-setup.exeresolve from the top-levelcodex-resources/directory. The direct-sibling and older sibling-resource fallbacks are preserved.Test plan
sh -n scripts/install/install.shbash -n scripts/install/install.shpwsh -NoProfile -Command '$tokens=$null; $errors=$null; $null = [System.Management.Automation.Language.Parser]::ParseFile("scripts/install/install.ps1", [ref]$tokens, [ref]$errors); if ($errors.Count) { $errors | Format-List *; exit 1 }'HOME="$home_dir" CODEX_HOME="$tmp_dir/codex-home" CODEX_INSTALL_DIR="$bin_dir" PATH="$bin_dir:$PATH" sh scripts/install/install.sh --release 0.125.0current/codexand includes the legacycodex-resources/rgpayload.cargo test -p codex-windows-sandboxjust fix -p codex-windows-sandboxStack created with Sapling. Best reviewed with ReviewStack.