Repository navigation
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. |
4e86c3e to
550185b
Compare
✅ Tests passed — 1112/1117
|
550185b to
19888b3
Compare
Ran two workers concurrently for the first time - browseros-ai#1093 and browseros-ai#1098, two minutes of real overlap, each on its own branch and boundary, neither touching the other's files. The result was green; the value was what it exposed. The ownership rule compared boundaries as strings, so `docs` and `docs/live` looked unrelated and both claims were admitted - while a write is judged by containment, so the bee owning `docs` was entitled to write the file the other bee owned. Both rules now share pathsOverlap, which compares by path component: nested clashes, siblings do not, and a shared prefix is not containment. Six reports arriving at once persisted three. Each caller appended to the same main-actor buffer, so memory was always complete, then each passed its own whole-array snapshot to save - and the file kept whichever finished last rather than whichever held the most. Writes to her chat are now queued and snapshot at run time. That one hid behind a guessed 300ms sleep that was enough on an idle machine. The sleep is now a bounded wait, and the race has a deterministic check: the mock holds the first write open while a second overtakes it, which fails without the fix and passes with it. Also: sixteen prototype exemptions named files 939028c had already deleted, and the gate was failing on them. Closes gHashTag/trios#1098
Ran two workers concurrently for the first time - browseros-ai#1093 and browseros-ai#1098, two minutes of real overlap, each on its own branch and boundary, neither touching the other's files. The result was green; the value was what it exposed. The ownership rule compared boundaries as strings, so `docs` and `docs/live` looked unrelated and both claims were admitted - while a write is judged by containment, so the bee owning `docs` was entitled to write the file the other bee owned. Both rules now share pathsOverlap, which compares by path component: nested clashes, siblings do not, and a shared prefix is not containment. Six reports arriving at once persisted three. Each caller appended to the same main-actor buffer, so memory was always complete, then each passed its own whole-array snapshot to save - and the file kept whichever finished last rather than whichever held the most. Writes to her chat are now queued and snapshot at run time. That one hid behind a guessed 300ms sleep that was enough on an idle machine. The sleep is now a bounded wait, and the race has a deterministic check: the mock holds the first write open while a second overtakes it, which fails without the fix and passes with it. Also: sixteen prototype exemptions named files 939028c had already deleted, and the gate was failing on them. Closes gHashTag/trios#1099
…y bees Two delegated workers, neither of them me. The Queen wrote the boundary and the contract; the code below is theirs. `queen/1093-pin-the-specification-above-the-chat-so` added PinnedSpecHeader to ChatPanelView: the active task's criteria stay above the conversation with a verdict beside each one and the acceptance blocker named, so neither is something you have to run `/verify` to see. It draws nothing in a chat with no delegated task. `queen/1100-ask-a-reviewer-agent-for-a-verdict-on` added QueenReviewVerdictRequest and wired it into review. Mechanical verdicts settle criteria that name a file; every other criterion had no way to be answered and stayed "never checked", which blocked acceptance permanently - that is why browseros-ai#1093's three prose criteria scored zero verdicts out of three. The reviewer agent is asked instead, and the parser refuses to read a garbled or empty answer as a pass. Both branches compile and all three gates pass with them in the tree, which is worth saying because nothing in the Queen's review builds a bee's work before judging it. Refs gHashTag/trios#1100 - the reviewer is asked now, but every answer still parses to unchecked, so nothing can be accepted yet
1 participant
Summary
Verification