Skip to content

Commit 12d2e47

Browse files
Bojun-VvibeBojun-Vvibe
committed
review: drip-182 batch 1 — anomalyco/opencode#24984, openai/codex#20243+20240
- anomalyco/opencode#24984 fix(core): reconnect editor context for session directory (merge-after-nits) - openai/codex#20243 Add codex-core public API listing (merge-as-is) - openai/codex#20240 [linux sandbox] Isolate IPC namespace in bubblewrap (merge-as-is)
1 parent 084700c commit 12d2e47

3 files changed

Lines changed: 165 additions & 0 deletions

File tree

‎reviews/2026-W18/drip-182/openai-codex-pr-20240.md‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
---
2+
pr: openai/codex#20240
3+
sha: 0ea76144e4e81125d5d0a54d17ed0e4f52acd50b
4+
verdict: merge-as-is
5+
reviewed_at: 2026-04-29T18:31:00Z
6+
---
7+
8+
# [linux sandbox] Isolate IPC namespace in bubblewrap
9+
10+
## Context
11+
12+
The Linux bubblewrap sandbox (`codex-rs/linux-sandbox/src/bwrap.rs`) was
13+
already passing `--unshare-user` and `--unshare-pid`, but it left the
14+
host IPC namespace shared. That meant a sandboxed command could
15+
`shmget`/`shmat` into a host-side System V shared memory segment owned
16+
by the same uid — an unmediated host communication channel that bypasses
17+
the filesystem and network controls bubblewrap is supposed to provide.
18+
Linear ticket BUGB-16097, Bugcrowd `6e34212b-…`.
19+
20+
## What's good
21+
22+
- The fix is the smallest possible: one extra `"--unshare-ipc".to_string()`
23+
pushed into both `create_bwrap_flags_full_filesystem` (line 158) and
24+
`create_bwrap_flags` (line 202). No new conditional, no new option flag,
25+
no opt-out. That's the right call — there's no legitimate reason a
26+
sandboxed agent command needs to attach to host SysV IPC, and adding
27+
a knob would just create a footgun.
28+
- The regression test in `codex-rs/linux-sandbox/tests/suite/landlock.rs`
29+
is doing the right thing: `HostSysvSharedMemory::new(secret)` allocates
30+
a real `IPC_PRIVATE` segment in the test process, then runs a
31+
re-exec'd helper (`sysv_ipc_probe_helper`) inside the sandbox that
32+
attempts `libc::shmat(shmid, …)`. The helper `panic!`s if the
33+
attach succeeds and reads the secret back — so the test fails loudly
34+
if the namespace isolation regresses. Drop impl on `HostSysvSharedMemory`
35+
uses `IPC_RMID` to clean up even on test failure. Solid.
36+
- The `bwrap_preserves_writable_dev_shm_bind_mount` test (already in
37+
the suite, line 291) covers the inverse concern: that `--unshare-ipc`
38+
doesn't break `/dev/shm` workflows that depend on POSIX shm bind-mounts.
39+
The author kept that test green, which is the only non-obvious risk
40+
of this change.
41+
- The `linux_run_main_tests.rs` argv-ordering test was updated in lockstep
42+
(line 79) so the argv-builder unit test continues to assert exact flag
43+
order. No drift.
44+
45+
## Concerns / nits
46+
47+
- README update is one sentence (`--unshare-ipc`); could mention that
48+
POSIX shm via `/dev/shm` bind-mount is unaffected, since that's the
49+
most likely "is this going to break my workflow?" question.
50+
51+
## Verdict
52+
53+
`merge-as-is` — security fix, smallest possible diff, regression test
54+
proves the boundary is closed and the `/dev/shm` carve-out still works.
55+
This should land.

‎reviews/2026-W18/drip-182/openai-codex-pr-20243.md‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
pr: openai/codex#20243
3+
sha: b86e0dcad5ab419daa8c9c65c5d4dc15f16799a0
4+
verdict: merge-as-is
5+
reviewed_at: 2026-04-29T18:31:00Z
6+
---
7+
8+
# Add codex-core public API listing
9+
10+
## Context
11+
12+
Adds a checked-in `codex-rs/core/public-api.txt` (934 lines of `pub use …`,
13+
`pub fn …` declarations) generated by `cargo-public-api` plus a CI gate
14+
(`scripts/regen-public-api.sh --check`) that fails if the actual surface
15+
drifts from the snapshot. Pinned to `nightly-2025-09-18` via
16+
`CODEX_PUBLIC_API_TOOLCHAIN`. Two workflow files updated: `rust-ci.yml`
17+
(PR-scoped, with a `changed.outputs.public_api` filter so unrelated PRs
18+
skip it) and `rust-ci-full.yml` (always-on).
19+
20+
## What's good
21+
22+
- The change-detection filter in `rust-ci.yml` is the right call:
23+
`[[ $f == codex-rs/* || $f == scripts/regen-public-api.sh || $f == justfile ]] && public_api=true`.
24+
PRs that touch only docs or `tools/` won't pay the nightly toolchain
25+
install cost. The aggregate `if [[ … != 'true' && … != 'true' && … != 'true' && … != 'true' ]]; then echo 'No relevant changes'; exit 0; fi`
26+
block is updated symmetrically — easy to miss, but it's there.
27+
- Toolchain pinning via env var (`CODEX_PUBLIC_API_TOOLCHAIN=nightly-2025-09-18`)
28+
rather than baked into the script lets local devs override it without
29+
editing the script. Good ergonomics.
30+
- The aggregator job's status echo (`echo "pubapi : ${{ needs.public_api.result }}"`)
31+
and the explicit `[[ … == 'success' ]] || { echo 'public_api failed'; exit 1; }`
32+
guard mean a green aggregator can't hide a yellow `public_api` job. Both
33+
workflows updated identically.
34+
35+
## Concerns / nits
36+
37+
- The 934-line `public-api.txt` is going to generate noisy review diffs
38+
on legitimate API changes. Consider documenting in the script that
39+
PRs adding `pub` items should commit the regenerated file in the same
40+
PR (the `--check` mode will tell them, but a `CONTRIBUTING.md` note
41+
would shorten the loop).
42+
- Nightly toolchain pinning is unavoidable for `cargo-public-api`, but
43+
the version number is now duplicated in two workflow files and the
44+
script's env default. A small risk of drift; consider sourcing it from
45+
one place (e.g. `rust-toolchain` file or a workflow-shared env).
46+
47+
## Verdict
48+
49+
`merge-as-is` — the workflow plumbing is correct, the aggregator logic
50+
is symmetric, and the change-detection filter prevents unnecessary
51+
nightly installs. The maintenance overhead concerns are follow-ups.

‎reviews/2026-W18/drip-182/sst-opencode-pr-24984.md‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
---
2+
pr: sst/opencode#24984
3+
sha: 62b7694465283e95adb556b60acbd6ac004dcf23
4+
verdict: merge-after-nits
5+
reviewed_at: 2026-04-29T18:31:00Z
6+
---
7+
8+
# fix(core): reconnect editor context for session directory
9+
10+
## Context
11+
12+
Before this PR, `EditorContextProvider` in `packages/opencode/src/cli/cmd/tui/context/editor.ts` resolved
13+
the editor lock file once at process startup using `process.cwd()`. After a
14+
session switch, opencode kept talking to whatever editor server was attached
15+
when the binary first started — wrong file tree, wrong selection ranges, sometimes
16+
a dead socket. The author moves the connection state out of `onMount` into
17+
provider-scoped `let` bindings and adds an explicit `directory` variable so
18+
the provider can re-resolve `resolveEditorConnection(directory)` whenever
19+
the active session changes.
20+
21+
## What's good
22+
23+
- The lift from `onMount` closure to provider-scope (`let socket`,
24+
`let directory = process.cwd()`, etc.) is the right shape — re-mounting
25+
was never the trigger; *session change* is. The reconnect call site
26+
ends up colocated with `setStore("server", …)`, which is the only place
27+
state actually needs to be invalidated.
28+
- `WebSocketImpl` injection (`init: (props: { WebSocketImpl?: typeof WebSocket })`)
29+
is the testability hook the existing test suite needed — the new
30+
`editor-context.test.tsx` (referenced in the PR body) can stub the socket
31+
without monkey-patching the global. Good restraint: it falls back to the
32+
real `WebSocket` at the call site rather than at module load.
33+
- Same-directory short-circuit (avoid socket churn when nothing changed)
34+
is mentioned in the PR body and is the right safety: editor servers
35+
often have warm-up costs, and an unconditional reconnect on every
36+
session activation would be a regression.
37+
38+
## Concerns / nits
39+
40+
1. The `socket.readyState !== 1` literal in the new `send()` replaces
41+
`WebSocket.OPEN`. That works but loses self-documentation and risks
42+
diverging if `WebSocketImpl` is a polyfill that uses a different enum.
43+
Prefer `WebSocketImpl.OPEN` (or pull it onto a local `const OPEN = WebSocketImpl.OPEN`).
44+
2. `lastSubmittedEditorSelectionKey` in `prompt/index.tsx` is a closure-scoped
45+
`let` inside `Prompt(props)`. If `<Prompt>` ever remounts (route change,
46+
keyed re-render), the dedupe state resets and the same selection will be
47+
re-submitted as a fresh part. Worth a comment or a `createSignal` so
48+
intent is explicit.
49+
3. The PR drops `editor.clearSelection()` after submit and replaces it with
50+
the dedupe-key assignment. That changes semantics: the editor selection
51+
stays "live" across submits. Confirm with the design owner that this is
52+
intentional — for users who want each prompt to start clean, this is a
53+
subtle UX shift.
54+
55+
## Verdict
56+
57+
`merge-after-nits` — fix the `WebSocket.OPEN` literal, add a one-line
58+
comment explaining the `lastSubmittedEditorSelectionKey` lifecycle, and
59+
double-check the `clearSelection` removal is intentional.

0 commit comments

Comments
 (0)