Skip to content

Commit dfacd42

Browse files
committed
review(drip-171): batch 1 — opencode + codex (4 PRs)
- anomalyco/opencode#24933 merge-after-nits: --file MIME detection fix - anomalyco/opencode#24935 request-changes: in-tree pet+CAVA bundle, ship as plugin only - openai/codex#20176 merge-after-nits: SessionSource via JSON deserialize is brittle - openai/codex#20178 merge-as-is: 1714-line obsolete adapter deletion
1 parent ce4fa26 commit dfacd42

4 files changed

Lines changed: 245 additions & 0 deletions

File tree

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
# openai/codex#20176`TUI: Remove core protocol dependency [5/7]`
2+
3+
- URL: https://github.com/openai/codex/pull/20176
4+
- Head SHA: `5564c076b3da`
5+
- Author: etraut-openai
6+
- Size: +2 / -2 in `codex-rs/tui/src/onboarding/auth.rs`
7+
- Stack: 5 of 7 in the "remove direct `codex_protocol::protocol`
8+
usage from `codex-tui`" series
9+
10+
## Summary
11+
12+
Onboarding auth tests had one lingering `use codex_protocol::protocol::SessionSource;`
13+
import after the rest of the TUI had been migrated to the
14+
app-server `SessionSource` equivalent. This PR removes that last
15+
import in the test module and constructs the `SessionSource` value
16+
via JSON deserialization instead.
17+
18+
## Specific reference
19+
20+
`codex-rs/tui/src/onboarding/auth.rs:1016-1050` (test module, post-patch):
21+
22+
```rust
23+
- use codex_protocol::protocol::SessionSource;
24+
use pretty_assertions::assert_eq;
25+
...
26+
- session_source: SessionSource::Cli,
27+
+ session_source: serde_json::from_value(serde_json::json!("cli"))
28+
+ .expect("cli session source should deserialize"),
29+
```
30+
31+
## Risks
32+
33+
- The new construction is `serde_json::from_value(json!("cli"))`
34+
it relies on the field type implementing `Deserialize` for a
35+
bare string, with the contract "the canonical wire form for the
36+
CLI variant is the literal string `"cli"`". That's brittle:
37+
anyone renaming the variant or adding `#[serde(rename_all = ...)]`
38+
silently breaks this test at runtime instead of compile time.
39+
- It would be cleaner to import the app-server enum directly:
40+
`use codex_app_server_protocol::SessionSource;` and then
41+
`session_source: SessionSource::Cli,`. The PR description says
42+
the rest of onboarding moved to the app-server type, so the
43+
enum should already be in scope or trivially importable. Using
44+
JSON-string construction in tests defeats half the type system.
45+
46+
## Verdict
47+
48+
`merge-after-nits`
49+
50+
## Suggested nit
51+
52+
Replace the `serde_json::from_value(json!("cli"))` call with a
53+
direct constructor of the app-server `SessionSource` enum (the
54+
same type used non-test elsewhere after this stack lands). That
55+
keeps the cargo-check verification meaningful and avoids hiding
56+
a future rename behind a runtime panic.
57+
58+
Otherwise the change is mechanical and matches the stated stack
59+
goal of confining `codex_protocol::protocol` usage out of `codex-tui`.
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
# openai/codex#20178`TUI: Remove core protocol dependency [6/7]`
2+
3+
- URL: https://github.com/openai/codex/pull/20178
4+
- Head SHA: `55eeb5bd0af4`
5+
- Author: etraut-openai
6+
- Size: +0 / -1714 (single deletion of `codex-rs/tui/src/app/app_server_adapter.rs`)
7+
- Stack: 6 of 7
8+
9+
## Summary
10+
11+
Deletes the obsolete `app_server_adapter.rs` bridge between the
12+
app-server event stream and the TUI's direct-core path, now that
13+
PRs 1–5 in the stack have moved every TUI flow onto the app-server
14+
surface directly. The file's own header comment (`tui/src/app/app_server_adapter.rs`
15+
lines 1–11 of the deletion) explicitly calls itself a "temporary
16+
adapter layer ... [that] should shrink and eventually disappear."
17+
This PR is that disappearance.
18+
19+
## Specific references
20+
21+
- `codex-rs/tui/src/app/app_server_adapter.rs` deleted in full
22+
(1714 lines).
23+
- The file's leading doc comment said: *"As more TUI flows move
24+
onto the app-server surface directly, this adapter should
25+
shrink and eventually disappear."* — this PR is the closing
26+
step of that plan.
27+
- PR description claims `cargo check -p codex-tui` and grep for
28+
`codex_protocol::protocol` in `codex-tui` are both clean. With
29+
this stack and #20176, the entire `codex-tui` crate's direct
30+
dependency on `codex_protocol::protocol` is gone.
31+
32+
## Risks
33+
34+
- The diff is a pure deletion, but the dangerous bit is what
35+
*imports* this file. If anything outside `codex-rs/tui` pulled
36+
`app_server_adapter` symbols (e.g. via `pub use`), this breaks
37+
them. PR description says only the TUI consumed it, but a quick
38+
`rg "app_server_adapter"` across the workspace would be a
39+
one-line confirmation worth pasting into the PR.
40+
- Test artifacts that imported via `#[cfg(test)] use ... split_command_string`
41+
(visible in the file header at line 21 of the deleted file)
42+
need to live somewhere. The PR doesn't say where they moved.
43+
If they were unused, that's fine; if they migrated, a sentence
44+
pointing at PR 5 or PR 7 in the stack would help the reviewer.
45+
46+
## Verdict
47+
48+
`merge-as-is` — assuming the prior PRs in the stack landed and
49+
`cargo check --workspace` is green.
50+
51+
## Suggested nits
52+
53+
- Add a one-line `rg "app_server_adapter"` confirmation in the PR
54+
body to prove no external consumer remains.
55+
- Optional: in the merge commit message, link to the original
56+
introduction PR of the adapter so future archaeologists can
57+
see the lifecycle.
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
# sst/opencode#24933`fix(cli): detect MIME type for --file flag instead of hardcoding text/plain`
2+
3+
- URL: https://github.com/sst/opencode/pull/24933
4+
- Head SHA: `0d1dade4d0d7`
5+
- Author: rogerdigital
6+
- Size: +1 / -1 in `packages/opencode/src/cli/cmd/run.ts`
7+
- Closes: #24698
8+
9+
## Summary
10+
11+
`opencode run --file <path>` always tagged the attached file with
12+
`text/plain`, regardless of actual content. That caused image
13+
attachments (and any other binary) to be passed to the model as if
14+
they were UTF-8 text, defeating multimodal routing on providers that
15+
key off the MIME type. The fix replaces the hardcoded literal with
16+
the existing `Filesystem.mimeType()` helper.
17+
18+
## Specific reference
19+
20+
`packages/opencode/src/cli/cmd/run.ts:331` (line 328 in the diff
21+
context, post-patch line 331):
22+
23+
```ts
24+
- const mime = (await Filesystem.isDir(resolvedPath)) ? "application/x-directory" : "text/plain"
25+
+ const mime = (await Filesystem.isDir(resolvedPath)) ? "application/x-directory" : await Filesystem.mimeType(resolvedPath)
26+
```
27+
28+
The directory branch is preserved; only the non-directory fallback
29+
changes. `Filesystem.mimeType()` already exists and is the same
30+
helper used by other ingestion paths, so this is the obvious
31+
single-line fix.
32+
33+
## Risks
34+
35+
- `mime-types` returns `false` for unknown extensions. The follow-up
36+
question is what `Filesystem.mimeType()` does in that case — if it
37+
falls back to `application/octet-stream`, multimodal-incapable
38+
providers may now reject what used to silently work as text.
39+
Worth a one-liner test for "unknown extension → reasonable
40+
fallback (text/plain or octet-stream consistent with other call
41+
sites)".
42+
- No new test added. For a fix that's literally about wrong MIME
43+
classification, a small unit test exercising at least
44+
`image/png`, `text/plain` (e.g. `.txt`), and unknown extension
45+
would lock the behavior.
46+
47+
## Verdict
48+
49+
`merge-after-nits` — add a regression test or at minimum a snippet
50+
in the PR description verifying the three cases above. The change
51+
itself is correct and minimal.
52+
53+
## Suggested nits
54+
55+
- Add a `run.test.ts` case for `--file foo.png` asserting
56+
`mime === "image/png"`.
57+
- Confirm `Filesystem.mimeType()`'s unknown-extension behavior in
58+
the PR description so reviewers don't have to chase it.
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
# sst/opencode#24935`feat(tui): add terminal pet companion with CAVA audio visualizer`
2+
3+
- URL: https://github.com/sst/opencode/pull/24935
4+
- Head SHA: `d74e8bf24b2f`
5+
- Author: ZoeImport
6+
- Size: +1448 / -0 across 10 files
7+
- Closes: #24937
8+
9+
## Summary
10+
11+
Adds an ASCII pet to the TUI sidebar with a 6-state behavior
12+
machine, weather influence, and an optional CAVA audio
13+
visualizer. Ships under `examples/terminal-pet/` plus new feature
14+
plugin files under `packages/opencode/src/cli/cmd/tui/feature-plugins/sidebar/`.
15+
Two top-level design docs (`PR-UI-EXTENSION.md`, `PR_DESCRIPTION.md`)
16+
are added at repo root.
17+
18+
## Specific references
19+
20+
- `PR-UI-EXTENSION.md` and `PR_DESCRIPTION.md` are added at the
21+
repository root. These are PR-meta files — they belong in the PR
22+
description box, not the tree. Likely to be rejected on style
23+
alone.
24+
- `packages/opencode/src/cli/cmd/tui/feature-plugins/sidebar/pet.tsx`
25+
and `spectrum.tsx` are introduced as first-class TUI feature
26+
plugins. Per the PR's own analysis (`PR-UI-EXTENSION.md` lines
27+
~10–25 listing existing slots), the project already exposes a
28+
`sidebar_content` plugin slot. A whole-cloth pet should live in
29+
an external `~/.config/opencode/plugins/...` package using that
30+
slot (which the PR even creates as `examples/terminal-pet/`),
31+
not be merged into the core TUI tree as a `feature-plugin`.
32+
- CAVA dependency: `spectrum.tsx` shells out to `cava`. There's
33+
no detection / graceful-skip wired into the sidebar mount path
34+
visible in the diff — the PR description claims a fallback exists
35+
but the test surface is +0 lines, so nothing exercises it.
36+
- `examples/terminal-pet/pets.config.json` and
37+
`pet.config.schema.json` add a config schema with no validator
38+
hooked up in the loader path.
39+
40+
## Risks
41+
42+
- Scope: this is two features (pet + audio visualizer) plus a
43+
proposed new "UI Widget Extension System" doc rolled into one
44+
1448-line PR. The maintainers' usual policy on this repo is one
45+
small change per PR.
46+
- Maintenance burden: a stateful animation widget in the core TUI
47+
binary is forever the maintainer's problem. The
48+
`examples/terminal-pet/` external plugin form already
49+
demonstrates the same thing without taking on that burden.
50+
- Zero tests added on a PR introducing a state machine, weather
51+
system, and audio integration.
52+
- Top-level `.md` files at repo root will collide with future
53+
PR-template tooling.
54+
55+
## Verdict
56+
57+
`request-changes`
58+
59+
## Suggested direction
60+
61+
1. Drop the in-tree `feature-plugins/sidebar/pet.tsx` and
62+
`spectrum.tsx` additions; ship the whole thing as the external
63+
plugin under `examples/terminal-pet/` (already in the PR).
64+
2. Move the two `PR-*.md` files into the PR description / a single
65+
`examples/terminal-pet/README.md`.
66+
3. Hard-gate the CAVA path on a runtime `which cava` check with a
67+
silent no-op fallback, plus one snapshot test asserting the
68+
widget renders without `cava` present.
69+
4. If the maintainers do want pet-as-core, split into three PRs:
70+
plugin-slot exposure (if any new slot is needed), pet widget,
71+
audio visualizer.

0 commit comments

Comments
 (0)