Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76db498c66
ℹ️ 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".
|
Addressed the P2 about duplicate tar members.
|
viyatb-oai
left a comment
There was a problem hiding this comment.
Looks good overall. I left two P2 follow-ups that seem worth tightening before this builder becomes a shared release path.
| print("+", " ".join(cmd)) | ||
| subprocess.run(cmd, cwd=CODEX_RS_ROOT, check=True) | ||
|
|
||
| output_dir = cargo_profile_output_dir(spec, profile) |
There was a problem hiding this comment.
P2 Honor CARGO_TARGET_DIR when locating the freshly built artifacts
This cargo build can legitimately write outside codex-rs/target when the caller sets CARGO_TARGET_DIR, but the lookup below always reconstructs CODEX_RS_ROOT / "target" / .... In that setup the build succeeds and the packager then fails with cargo build did not produce expected binary, even though Cargo did produce it. The repo already has other tooling that preserves this override, so I think this builder should do the same.
| output_dir = cargo_profile_output_dir(spec, profile) | |
| output_dir = cargo_profile_output_dir(spec, profile) |
with cargo_profile_output_dir() resolving from CARGO_TARGET_DIR when it is set, and falling back to CODEX_RS_ROOT / "target" otherwise.
There was a problem hiding this comment.
Fixed in 164fbc8. cargo_profile_output_dir() now resolves from CARGO_TARGET_DIR when it is set, using absolute paths as-is and interpreting relative paths from codex-rs, then falls back to codex-rs/target. I also verified this with a fake-Cargo package build.
| ) -> Path: | ||
| if explicit_path is not None: | ||
| path = explicit_path.resolve() | ||
| if not path.is_file(): |
There was a problem hiding this comment.
P2 Reject a non-executable --rg-bin instead of packaging it as valid
Right now --rg-bin only has to be a regular file. On Unix, copy_executable() then adds execute bits to the copy, and validate_package_dir() only checks that the destination is executable afterward. That means something like --rg-bin README.md can still produce a package that passes validation even though the shipped rg payload is unusable.
| if not path.is_file(): | |
| if not path.is_file(): | |
| raise RuntimeError(f"{description} does not exist: {path}") | |
| if not path.stat().st_mode & 0o111: | |
| raise RuntimeError(f"{description} is not executable: {path}") |
That keeps the package validation tied to the input artifact rather than making any regular file look executable during copying.
There was a problem hiding this comment.
Fixed in 164fbc8. Explicit --rg-bin inputs now have to be regular executable files before package assembly; otherwise the builder fails with a ripgrep executable is not executable error. I verified this with a non-executable override in the fake-Cargo package check.
Why
Codex CLI packaging is currently split across npm staging, standalone installers, and release bundle creation, which makes it hard to define and validate a single valid package directory. This adds the first standalone package builder so later release paths can converge on the same canonical layout.
What changed
scripts/build_codex_package.pyas the stable executable wrapper aroundscripts/codex_package.codex-package.json,bin/,codex-resources/, andcodex-path, and can serialize it as.tar.gz,.tar.zst, or.zip.cargo build:codexfor all targets,bwrapfor Linux, and the Windows sandbox helpers for Windows.rgremains an input because it is vendored from upstream rather than built from this repo.scripts/codex_package/README.mdto document the package layout, source-built artifacts, and cargo profile behavior.Verification
scripts/build_codex_package.py --helpfrom/private/tmp.Stack created with Sapling. Best reviewed with ReviewStack.