feat: bundle Codex and Claude Code CLIs#1093
Conversation
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 3m 15s —— View job Code Review
Overall this is a solid, well-structured PR. The layering — R2 upload tooling → build manifest → runtime binary resolution — is clean and the test coverage is thorough. A few findings below, ordered by severity. Bugs / Correctness Issues1.
# storage.py — _process_claude_code_platform ✅
if not binary_name.endswith(".exe"):
local_path.chmod(0o755)
# storage.py — _extract_bun_file ❌
dest.chmod(0o755) # always, even for bun.exeThe chmod succeeds because uploads run from macOS/Linux, but it's inconsistent and would break if this ever ran on Windows. Fix this → Dead Code2.
Options: (a) use the hardcoded value for validation/assertion against what the metadata says, or (b) remove the field entirely and trust the archive metadata. Fix this → Minor / Nits3. def _build_claude_code_manifest(...):
return {
...
"binary_shas_upstream": binary_shas,
"r2_object_shas": binary_shas, # same object
}For Claude Code this is technically correct (no zip extraction — upstream binary == R2 object), but it's subtly inconsistent with the Codex manifest where the two fields are semantically distinct ( 4.
case 'needs-install':
return `${input.displayName} adapter cannot launch because neither bundled Bun nor npx is available.`This message is emitted when 5.
(async () =>
resolveBundledNative({...}) ?? (await resolveBinary(config.nativeBinary)))().catch(() => null),The closing async function resolveNativeCli(...): Promise<ResolvedHostBinary | null> {
return resolveBundledNative({...}) ?? resolveBinary(config.nativeBinary)
}Test CoverageCoverage is good overall. A few gaps worth noting:
Overall AssessmentThe approach is sound: bundled native CLIs are preferred for health probes (avoiding PATH pollution) while Bun/npx remains the adapter launch mechanism. The upload pipeline for Claude Code and Codex follows the same pattern as Bun, with appropriate per-tool customization. The cross-contract storage key test in The blocking issue is the |
Greptile SummaryThis PR bundles Codex (
Confidence Score: 4/5The change is safe to merge; all new code paths are covered by unit and integration tests, SHA verification guards every upload, and rollback logic protects R2 on partial failures. The core runtime, detection, and upload logic is well-structured and well-tested. The two findings are a dead
Important Files Changed
Sequence DiagramsequenceDiagram
participant RT as AcpxRuntime
participant NB as withBundledNativeBinaryPath
participant BN as resolveBundledNativeBinary
participant DH as detectHostAdapter
participant RB as resolveHostBinary (fallback)
RT->>NB: prepend bin/third_party to PATH (commandEnv)
NB-->>RT: updated env
RT->>DH: detect adapter (resourcesDir, env)
DH->>BN: resolveBundledNativeBinary(adapter, resourcesDir)
alt bundled binary found
BN-->>DH: "{path, env} with prepended dir"
else not found
BN-->>DH: null
DH->>RB: resolveHostBinary(config.nativeBinary)
RB-->>DH: "{path, env} or null"
end
DH-->>RT: nativeCli + launch config
RT->>RT: wrapCommandWithEnv(launch.command, commandEnv)
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros/build/cli/storage.py:101-136
The `entrypoint` field is declared in `CodexPlatform` and populated in every `CODEX_PLATFORMS` entry, but it is never read anywhere in the codebase — `_extract_codex_file` discovers the entrypoint dynamically by reading `codex-package.json` from the tarball rather than using this static value. Per the repo rule on removing unused/dead code, the field should be dropped.
```suggestion
@dataclass(frozen=True)
class CodexPlatform:
"""BrowserOS target plus the upstream Codex package coordinates."""
target: str
upstream: str
CODEX_PLATFORMS: Tuple[CodexPlatform, ...] = (
CodexPlatform(
target="darwin-arm64",
upstream="aarch64-apple-darwin",
),
CodexPlatform(
target="darwin-x64",
upstream="x86_64-apple-darwin",
),
CodexPlatform(
target="linux-arm64",
upstream="aarch64-unknown-linux-musl",
),
CodexPlatform(
target="linux-x64",
upstream="x86_64-unknown-linux-musl",
),
CodexPlatform(
target="windows-x64",
upstream="x86_64-pc-windows-msvc",
),
)
```
### Issue 2 of 2
packages/browseros/build/cli/storage.py:1650-1657
`binary_shas_upstream` and `r2_object_shas` are assigned the same dictionary reference. For Bun and Codex the analogous fields carry *different* values (archive checksum vs extracted binary checksum), so a reader will expect these to differ here too. While they happen to be equal for Claude Code (the binary is uploaded without transformation), sharing the same object makes the manifest structure misleading and means any future in-place mutation of one key silently affects the other.
Reviews (1): Last reviewed commit: "fix(build): preserve bundled Bun platfor..." | Re-trigger Greptile |
Greptile SummaryThis PR bundles Codex (rust-v0.136.0) and Claude Code (2.1.159) CLIs alongside the existing Bun runtime, extending the existing bundled-binary infrastructure to cover Linux and Windows in addition to macOS. The runtime now prepends
Confidence Score: 4/5Safe to merge; all new code paths have tests and the bundled-binary fallback is non-destructive. The TypeScript changes are clean and well-tested. The main concerns are in storage.py: CodexPlatform.entrypoint is a dead field never read by the extraction logic, and _extract_bun_file / _extract_codex_file inconsistently apply chmod to Windows .exe files while the Claude Code path correctly skips it. Neither causes a runtime failure. packages/browseros/build/cli/storage.py — dead CodexPlatform.entrypoint field and inconsistent chmod calls for Windows EXEs. Important Files Changed
Sequence DiagramsequenceDiagram
participant Runtime as AcpxRuntime
participant Detection as detectHostAdapter
participant BundledNative as resolveBundledNativeBinary
participant BundledBun as resolveBundledBun
participant HostPath as resolveBinary (host PATH)
participant Subprocess as ACP Adapter Process
Runtime->>Detection: "detectHostAdapter(adapter, {resourcesDir, ...})"
Detection->>BundledNative: "resolveBundledNativeBinary({adapter, resourcesDir})"
alt bundled binary present and executable
BundledNative-->>Detection: "{path, env with dir prepended}"
Detection-->>Runtime: "nativeCli = bundled binary"
else bundled binary absent
BundledNative-->>Detection: null
Detection->>HostPath: resolveBinary(config.nativeBinary)
HostPath-->>Detection: nativeCli from host PATH
Detection-->>Runtime: "nativeCli = host binary"
end
Runtime->>BundledNative: "withBundledNativeBinaryPath({env, resourcesDir})"
Note over Runtime: Prepends bin/third_party to commandEnv PATH
alt addBundledBunAdapterEnv
Runtime->>BundledBun: withBundledBunAcpAdapterEnv(commandEnv, browserosDir)
BundledBun-->>Runtime: env with bundled bun prepended
end
Runtime->>Subprocess: spawn(command, enriched env)
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
packages/browseros/build/cli/storage.py:102-108
**Unused `entrypoint` field on `CodexPlatform`**
`CodexPlatform.entrypoint` is declared but never read anywhere in the codebase. `_extract_codex_file` derives the entrypoint by parsing `codex-package.json` inside the tarball via `_read_codex_package_entrypoint`, so the field on the dataclass is completely ignored at runtime. Every instantiation of `CodexPlatform` passes a value for it (e.g. `entrypoint="bin/codex"`) that is silently discarded, creating a misleading contract. The field should be removed.
### Issue 2 of 4
packages/browseros/build/cli/storage.py:747-749
`_extract_bun_file` unconditionally calls `dest.chmod(0o755)`, including when `binary_name` is `bun.exe` for the Windows target. Only `_process_claude_code_platform` correctly guards against setting execute bits on `.exe` files. While harmless on a Unix build server, this is inconsistent with the rest of the codebase.
```suggestion
if not str(dest).endswith(".exe"):
dest.chmod(0o755)
return
raise RuntimeError(f"{binary_name} not found in Bun zip")
```
### Issue 3 of 4
packages/browseros/build/cli/storage.py:769-771
`_extract_codex_file` always applies `dest.chmod(0o755)`, including for `codex-windows-x64.exe`. This is inconsistent with how `_process_claude_code_platform` handles Windows EXEs. The entrypoint string returned by `_read_codex_package_entrypoint` already ends in `.exe` for Windows packages, so the same guard can be applied here.
```suggestion
if not entrypoint.endswith(".exe"):
dest.chmod(0o755)
return
raise RuntimeError(f"Codex entrypoint {entrypoint} not found in package")
```
### Issue 4 of 4
packages/browseros/build/cli/storage.py:912-916
**`binary_shas_upstream` and `r2_object_shas` reference the same dict object**
Both manifest keys are assigned the same `binary_shas` dict. For Claude Code the values happen to be identical (no transformation), but aliasing the same object means any future in-place mutation would silently update both fields. Passing separate copies would make the intent clearer.
Reviews (2): Last reviewed commit: "fix(build): preserve bundled Bun platfor..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed review feedback in 19888b3:
Verification:
Also rebased on latest main to pick up BrowserOS#1090 and BrowserOS#1094; Git dropped the redundant bundled-Bun compatibility commit because it is now upstream. |
Nikhil (shadowfax92)
force-pushed
the
fix/0601-1722-pony
branch
from
4e86c3e to
550185b
Compare
✅ Tests passed — 1112/1117
|
Nikhil (shadowfax92)
force-pushed
the
fix/0601-1722-pony
branch
from
550185b to
19888b3
Compare
1 participant
Summary
Verification