-
Notifications
You must be signed in to change notification settings - Fork 0
plan: parity — mac vs win, codex vs claude — and onboarding that works from a new user's device (mac/win) #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
0ce4677
2ccb6e8
173508a
f900af1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,282 @@ | ||
| # Codex ↔ claude parity — subtasks | ||
|
|
||
| Architecture recap (why most parity is already free): one actor, two transports. | ||
| `src/engines/model/src/session_actor.rs` is provider-blind; `claude/` speaks | ||
| stream-json, `codex/` speaks `codex app-server` JSON-RPC and **normalizes everything | ||
| into the same BusEvents** (`codex/protocol/notify.rs`, `items.rs`, `prompts.rs`) — | ||
| so the whole timeline/rendering stack is shared. Context delivery, canvas-at-spawn, | ||
| and the hermetic boundary all landed for codex in W6 with tests. What remains is | ||
| below. | ||
|
|
||
| At parity already (verified — do not spend subtasks): CLI discovery + readiness; | ||
| create/resume; streaming (reasoning, exec/patch/MCP tools, plan→TodoWrite, token | ||
| usage, rate limits); interrupt; workspace preamble (`AGENTS.md`); scoped/gated | ||
| context via `developerInstructions` on start AND resume; canvas at spawn; spec-30 | ||
| byte-identical plain prompt; context ledger/indicator; approval prompts incl. MCP | ||
| elicitation; hooks exclusion; turn capture; vendor handover both directions; | ||
| SetupGate/provider cards; codex-only AskCard (multiple choice). | ||
|
|
||
| Deliberate non-parity (keep, do not "fix"): fork/rewind/aside buttons hidden on | ||
| codex (`spine/capabilities.ts:25-27`, MANUAL-PARITY G1 reserves the gate-opening | ||
| path); `CODEX_HOME` and account-default model inherited (auth-adjacent, symmetric | ||
| with claude's `~/.claude`); codex hermetic seal is a key-level denylist | ||
| (`codex/spawn.rs:150-153` documents the residue — `shell_environment_policy`, | ||
| `personality`, future keys) because `app-server` has no `--ignore-user-config`; | ||
| revisit only if codex ships that flag. | ||
|
|
||
| --- | ||
|
|
||
| ## Wave 2 — correctness | ||
|
|
||
| ### C1 — Internal runs on codex get a hermetic internal profile (bug) | ||
|
|
||
| - **Why:** The codex arm of `open_transport` (`src-tauri/src/transport.rs:171-215`) | ||
| **ignores `plan.internal`**. Claude internal runs (title-gen, side-ask fallback) | ||
| get DontAsk + empty allowlist + `max_turns 1` + no persistence | ||
| (`transport.rs:124-130`); codex internal runs get `interactive()` — on-request | ||
| approvals, a **write-capable sandbox**, a **persisted thread** in `~/.codex`, and | ||
| unlimited turns. The only guard left is actor-level `PermissionPolicy::Headless` | ||
| auto-deny. | ||
| - **Suggested implementation:** Give the codex arm an internal profile mirroring | ||
| `CodexSessionSettings::headless()` (`codex/protocol.rs:82-86`: `approvalPolicy: | ||
| never`, read-only sandbox) plus ephemeral/no-persist thread if the app-server | ||
| supports it; codex has no `max_turns`, but the app drives turns — the caller | ||
| simply never sends a second turn (assert that in a test, not in prose). | ||
| - **Do NOT build:** a general profile framework; it is one branch on `plan.internal` | ||
| in the codex arm, symmetric with the claude arm ten lines above it. | ||
| - **Verify:** `cargo test -p brains-desktop-tauri` (or the crate holding | ||
| `transport.rs` tests) exit 0, with a new test asserting the codex internal argv/ | ||
| handshake carries never+read-only and no persistence; existing | ||
| `codex_model` transport tests still green. | ||
| - **Manual check (walk and confirm — provider set to openai):** | ||
| - [ ] Start a new chat, send one message → an LLM title appears on the tab (the | ||
| internal run still works). | ||
| - [ ] List codex's own sessions (`codex resume` picker or `~/.codex` sessions | ||
| dir) → **no** new thread was persisted by the title run. | ||
| - [ ] The title run made no file writes outside the workspace (spot-check the | ||
| run's tool events — there should be none). | ||
|
|
||
| ### C2 — Side-ask fallback: stop hardcoding `gpt-4o-mini` (bug) | ||
|
|
||
| - **Why:** `src/layout/core/runtime/spine/side-ask.ts:26,113` pins | ||
| `SIDE_ASK_MODEL_OPENAI = "gpt-4o-mini"`, but `codex/spawn.rs:17-21` documents that | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 |
||
| fixed models 400 on ChatGPT-subscription accounts ("model not supported"). The | ||
| spawn path's own rule is "send no model, let the account resolve" — this one | ||
| caller violates it, so side-ask fallback likely fails on subscription accounts. | ||
| - **Suggested implementation:** Send no model on the codex side-ask fallback path; | ||
| delete the constant. Leave the claude constant (`claude-sonnet-4-20250514`) alone. | ||
| - **Verify:** `npx vitest run src/layout/core/runtime/spine/__tests__/side-ask.test.ts` | ||
| exit 0, updated to assert the codex fallback plan carries no model field. | ||
| - **Manual check (walk and confirm):** | ||
| - [ ] On a **ChatGPT-subscription** codex account: open a side question on a | ||
| settled codex turn → an answer arrives (no silent failure, no 400 in logs). | ||
| - [ ] Same on claude → unchanged. | ||
|
|
||
| ### C3 — "Allow always" on codex cards is a lie (mis-affordance) | ||
|
|
||
| - **Why:** Claude persists card answers into `--allowedTools` merged on next spawn | ||
| (`claude/approvals.rs`, `transport.rs:32-34,136`). Codex gates by **policy**, not | ||
| allowlist — the allowlist is deliberately not merged (`transport.rs:172-176`), and | ||
| no session-scoped accept is emitted. Yet the UI's `allowAlways` | ||
| (`ask-actions.ts:35-48`) still remembers: the user clicks "Always" on a codex card | ||
| and it silently only affects future *claude* sessions; codex re-asks every time. | ||
| - **Suggested implementation (smallest honest fix):** Hide the "Always" option on | ||
| approval cards when the run's provider is openai; the per-request allow/deny stays. | ||
| Wire from the run's provider already present in run meta — the capability-gating | ||
| pattern for branch buttons (`spine/capabilities.ts`) is the template. | ||
| - **Do NOT build:** a translation of `Bash(git status:*)`-style allowlist entries | ||
| into codex config — that is inventing semantics. If codex's app-server later | ||
| exposes an `acceptForSession`-class decision, that's a new subtask. | ||
| - **Verify:** `npx vitest run` on the ask-actions/pending-ask tests exit 0, with a | ||
| new case: provider openai ⇒ no allow-always affordance; provider anthropic ⇒ | ||
| unchanged. | ||
| - **Manual check (walk and confirm):** | ||
| - [ ] In a codex session, trigger a command approval (e.g. ask it to run a | ||
| shell command) → the card offers allow/deny only, **no "Always"**. | ||
| - [ ] Allow it → the command runs; the next similar command asks again (honest | ||
| per-request behavior, not a silent remember). | ||
| - [ ] In a claude session → "Always" still there and still persists across a | ||
| new session. | ||
|
|
||
| ### C4 — Vendor-aware model picker | ||
|
|
||
| - **Why:** With openai selected, the picker still lists claude models | ||
| (`model-options.ts:78-92` filters to claude-vendor ids only; | ||
| `ModelMenu.svelte`/`ChatHeader.svelte` have zero provider awareness) and the | ||
| native side **silently drops** the anthropic model name (`transport.rs:225-232`) — | ||
| codex runs its account default while the header claims a claude model. The UI | ||
| actively lies. | ||
| - **Suggested implementation:** Two acceptable shapes — pick ONE and state it in the | ||
| subtask PR: | ||
| 1. *Minimal honest:* under provider openai, hide the picker and show the model the | ||
| session actually reports after start (`session_init`/thread metadata) as a | ||
| read-only label. | ||
| 2. *Full:* wire the app-server's `model/list` (`codex/protocol.rs:261`, unused) | ||
| through a probe like claude's `model_probe` (`src-tauri/src/commands/system.rs:52-58`) | ||
| and populate real codex options. | ||
| Shape 1 is the recommended default (smaller, and respects the account-default rule | ||
| from C2); shape 2 only if the picker is judged product-critical for codex. | ||
| - **Do NOT build:** shape 2's probe *and* shape 1's fallback both; one shape. | ||
| - **Verify:** `npx vitest run` on model-options + header tests exit 0: provider | ||
| openai ⇒ no claude ids offered, header shows reported model; provider anthropic ⇒ | ||
| unchanged. `npm run check` exit 0. | ||
| - **Manual check (walk and confirm):** | ||
| - [ ] Switch provider to openai, open a new session → no claude model name | ||
| anywhere in the header/picker; after the first turn the header shows the | ||
| model the session actually reported. | ||
| - [ ] Switch back to anthropic → the picker returns with the claude options and | ||
| selecting one takes effect on the next run. | ||
| - [ ] Vendor handover mid-session (claude→codex) → the header follows the | ||
| provider honestly at the divider. | ||
|
|
||
| ### C4b — Per-session vendor choice in the model picker (builds on C4) | ||
|
|
||
| - **Why:** Today the vendor is chosen once, globally (`settings.provider` → | ||
| `model.selectedProvider`, "WHICH VENDOR new sessions open against", | ||
| `stores/model.svelte.ts:57-63`); the model picker only ever picks among claude | ||
| models. The product want: open a new session and choose **codex or claude, and | ||
| their models, in the model selection we already have** — per tab, not per app. | ||
| - **The rule:** the choice exists **only while the session hasn't started**. Once | ||
| a session has its first turn, it is **locked to its vendor** — the picker then | ||
| offers only that vendor's options (for codex, the read-only reported model from | ||
| C4). No mid-session vendor switching through the picker; the existing | ||
| settings-level switch + handover behavior stays exactly as it is. | ||
| - **Suggested implementation:** Extend the existing `ModelMenu` with two vendor | ||
| sections when both providers are ready: the claude models it already lists, and | ||
| a codex section (the C4 shape decides its contents — a single "Codex (account | ||
| default)" entry under shape 1, or `model/list` results under shape 2). The | ||
| chosen vendor+model stamp the new run's plan — the plumbing already exists, | ||
| provider is a per-run property (`RunMeta.provider`); only the selection feed is | ||
| new. `settings.provider` remains the default preselection for new sessions. A | ||
| vendor whose readiness pair isn't green is shown disabled with the reason, not | ||
| hidden. After first turn: the other vendor's section disappears (locked). | ||
| - **Do NOT build:** mid-session vendor switching via the picker (that path is the | ||
| settings-level handover, unchanged); per-message model routing; any change to | ||
| which vendor scheduled/internal runs use. | ||
| - **Verify:** `npx vitest run` on the model-options/model-picker tests exit 0 with | ||
| new cases: both-ready ⇒ two sections, default follows `settings.provider`; | ||
| codex-not-ready ⇒ disabled with reason; after first turn ⇒ locked to the run's | ||
| vendor. A transport-level test asserting the run plan carries the picker's | ||
| provider. `npm run check` exit 0; extend `13-codex.yaml` with the stamped- | ||
| provider assertion. | ||
| - **Manual check (walk and confirm — both vendors signed in):** | ||
| - [ ] Open a new session → the model menu offers both vendors; pick codex → | ||
| send → the run is codex (header honest per C4). | ||
| - [ ] Open another new session → pick a claude model → that model answers; | ||
| the two tabs run side by side on different vendors. | ||
| - [ ] In the started codex tab, open the picker → no claude section anymore | ||
| (locked); same lock mirrored in a started claude tab. | ||
| - [ ] Sign out of codex → new session's picker shows the codex section | ||
| disabled with the reason, not vanished. | ||
| - [ ] Flip `settings.provider` → a fresh session's picker pre-selects the new | ||
| default; existing tabs unaffected until their next message (the unchanged | ||
| handover behavior). | ||
|
|
||
| ### C5 — Codex test depth: fixtures + warm-resume | ||
|
|
||
| - **Why:** Claude has four protocol-replay goldens; codex has **one** | ||
| (`fixtures/codex/simple_turn.jsonl`) — no multi-turn, no MCP-tool-use, no | ||
| interrupt golden. Warm-session live tests (`tests/live_warm_sessions.rs`) are | ||
| claude-only, so warm reopen on codex is untested. Ralph loop tests only drive the | ||
| claude scripted CLI. | ||
| - **Suggested implementation:** Record/craft three codex goldens (multi-turn, | ||
| MCP tool use, interrupt mid-turn) for `tests/codex_replay.rs`; add a codex arm to | ||
| the warm-sessions live suite (marked `--ignored` like the claude one); add one | ||
| scripted-codex ralph test if the scripted-CLI harness supports the app-server | ||
| handshake cheaply — if it doesn't, say so in the PR and skip, don't build a | ||
| harness. | ||
| - **Verify:** `cargo test -p brains-model` exit 0 (replay suite includes the new | ||
| goldens); live suite documented as `cargo test -p brains-model --test | ||
| codex_live -- --ignored` for machines with a codex login. | ||
| - **Manual check (walk and confirm — on a codex-logged machine):** | ||
| - [ ] Run the ignored live warm suite once → green. | ||
| - [ ] In the app: close a codex session tab, reopen it from the library → the | ||
| history is intact and the next turn continues the same thread (warm reopen, | ||
| not a fresh session). | ||
| - [ ] Interrupt a codex turn mid-stream → the timeline settles cleanly and the | ||
| next message works. | ||
|
|
||
| ### C6 — Condensed context index for codex sessions | ||
|
|
||
| - **Why:** Claude sessions get progressive disclosure via materialized skills | ||
| (`materializer.rs:99-118` → `<data-root>/.claude/skills/`); codex has no skill | ||
| mechanism (`canvas.rs:7-10` records this), so a plain codex session gets ONLY the | ||
| 14-line AGENTS.md preamble — none of the brains detail (query-tool guidance, | ||
| `list_calendar_events`, sites/brains-apps contexts). | ||
| - **Suggested implementation:** Per the W6 rule ("always-on budget is spent on what a | ||
| session MOST LIKELY uses"), emit a **condensed index block** for codex — the | ||
| contexts' names + one-line descriptions + "ask for the detail" instruction — | ||
| appended to the AGENTS.md the materializer already writes (`materializer.rs:80-85`), | ||
| NOT the full details (that would re-inflate what W6 just deflated for claude). | ||
| Scoped/gated contexts already work via `developerInstructions` — untouched. | ||
| - **Tips:** codex 0.142 emits `skills/changed` (`notify.rs:224`) — codex is growing | ||
| a skills surface. Note it in the PR as the future replacement; do not adopt it yet. | ||
| - **Do NOT build:** full skill materialization for codex, or any change to the | ||
| claude path. Spec 30 (byte-identical plain prompt) applies per-provider — add the | ||
| codex twin of that assertion. | ||
| - **Verify:** `cargo test -p brains-context` (or its crate) exit 0 with new tests: | ||
| codex AGENTS.md contains the index, gated-off contexts absent from it (spec-30 | ||
| twin); claude outputs byte-identical to before (regression assert). | ||
| - **Manual check (walk and confirm):** | ||
| - [ ] Open the data root's `AGENTS.md` → the index lists the unscoped contexts | ||
| with one-liners, and nothing from a gated-off pack appears in it. | ||
| - [ ] In a plain codex session, ask a brains question the index covers (e.g. | ||
| "what's on my calendar this week") → the session reaches for the right brains | ||
| tool instead of guessing. | ||
| - [ ] In a claude session, confirm skills still materialize under | ||
| `<data-root>/.claude/skills/` and the prompt is unchanged. | ||
|
|
||
| ### C2.eval — Wave 2 eval subtask | ||
|
|
||
| - **What:** Extend `scripts/eval/specs/13-codex.yaml`: a codex session titled by an | ||
| internal run leaves no persisted thread and no write outside the workspace; an | ||
| approval card on codex shows no "Always"; the header never names a claude model | ||
| under provider openai. Runs with `BRAINS_API_TOKEN` + a codex login; `--no-agent` | ||
| skips it loudly. | ||
| - **Manual check (walk and confirm):** | ||
| - [ ] Run the extended spec once on a codex-logged machine and confirm each new | ||
| assertion executed against real behavior (would go red if C1–C4 regressed). | ||
|
|
||
| --- | ||
|
|
||
| ## Wave 4 — behind decisions | ||
|
|
||
| ### C7 — Codex headless runner (decision #2 in PLAN.md; L) | ||
|
|
||
| - **Why:** `src/engines/agents/local/src/actor_runner.rs:37-39,96-99,250-259` | ||
| imports `brains_model::claude::spawn` + `ClaudeProtocol` directly; | ||
| `RunnerSettings.claude_binary` is the only binary. With provider=openai selected, | ||
| every scheduled agent still requires a working claude CLI; a codex-only machine | ||
| gets zero agents. `CodexSessionSettings::headless()` exists but is used only in a | ||
| unit test. | ||
| - **The design decision that gates it:** the claude runner's OS sandbox rides a | ||
| `CommandWrapper` seam that exists only on claude's `SpawnPlan` | ||
| (`claude/spawn.rs:59-87`); `CodexSpawnPlan` has none — but codex brings its OWN | ||
| sandbox (`sandbox: read-only` in the handshake). The right shape is probably | ||
| codex-native confinement rather than wrapping, which changes the engine's "one | ||
| engine-global sandbox policy" story. Decide the shape BEFORE dispatching; this is | ||
| not a copy-paste of the claude runner. | ||
| - **Suggested implementation (once decided):** provider on `RunnerSettings` (+ | ||
| `codex_binary`), a provider-generic runner path over the existing actor (the actor | ||
| is already provider-blind), codex-native sandbox settings in the headless | ||
| handshake, and the scheduler's readiness gate keyed to the selected provider's | ||
| pair (already true at the index level). | ||
| - **Verify:** `cargo test -p brains-agents-local` exit 0 with a scripted codex | ||
| app-server stand-in (mirror `fake-claude`); the fire-agent script drives one | ||
| declared agent end-to-end on codex. | ||
| - **Manual check (walk and confirm):** | ||
| - [ ] On a machine with **only** codex working (no claude CLI on PATH), arm a | ||
| declared agent → the run executes, its artifact lands, and lifecycle health | ||
| reports it — with the codex sandbox confining it (spot-check: the run cannot | ||
| write outside its allowed roots). | ||
|
|
||
| ### C8 — Steering (decision #3), live `thread/fork`, ingest naming (decision #4) | ||
|
|
||
| - `turn/steer` implemented in protocol (`codex/protocol.rs:244-256`), never called — | ||
| no `ActorCommand::Steer`, no IPC, no UI. Exposing it = per-vendor composer | ||
| behavior mid-turn (join vs queue). M if wanted. | ||
| - Live `thread/fork` exists in the app-server API; would give branch-while-live for | ||
| codex (different UX than claude's branch-anywhere). MANUAL-PARITY G1 already | ||
| reserves the gate: no UI change needed, the gate just opens by provider. M. | ||
| - `/ingest/claude` receives codex turns (`src/engines/brains/native/src/ingest.rs:1`) | ||
| — server-side coordination, not a desktop subtask. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡
codex/spawn.rs:150-153is cited as documenting the hermetic-seal residue (shell_environment_policy,personality, future keys), but those lines are thecodex mcp listinvocation and neither key appears anywhere in the repo at head; the isolation at spawn is an MCP-server disable table (mcp_servers_table), not a config-key denylist. Fix the pointer or drop the residue claim so the "keep, do not fix" ruling rests on something verifiable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're both right — about different trees. The residue rustdoc landed in w6 (
77d9cc1, #46): at current dev (13c0afc),spawn.rs:151-152are exactly the two keys — "shell_environment_policy— affects how the shell inherits env vars" / "personality— no handshake field; wiping to""errors" — sitting directly aboveisolation_args()(:154), with a second mention in the file-top comment (:24). The tree you checked — this branch's own code snapshot, and the plan's stated3ccbab9baseline — both predate w6, so there the keys don't exist and :150-153 lands in thecodex mcp listfallback, exactly as you describe.So the "keep, do not fix" ruling does rest on something verifiable — at dev head, where the work happens; no doc change needed beyond re-stamping the baseline SHA (suggested in the main review). One sharpening while here: the seal is three parts —
-c projects={}+ the fivefeatures.*=falsekills (isolation_args, :154-176) + themcp_serversdisable table (:106-129, appended last inbuild_args:195-200) — not the disable table alone.