Skip to content

plan: parity — mac vs win, codex vs claude — and onboarding that works from a new user's device (mac/win) - #47

Open
liorrutenberg wants to merge 4 commits into
devfrom
plan/parity-waves
Open

plan: parity — mac vs win, codex vs claude — and onboarding that works from a new user's device (mac/win)#47
liorrutenberg wants to merge 4 commits into
devfrom
plan/parity-waves

Conversation

@liorrutenberg

@liorrutenberg liorrutenberg commented Aug 11, 2026

Copy link
Copy Markdown

Summary

This PR's goal is parity — mac vs win, codex vs claude — and onboarding that works from a new user's device (mac/win).

It adds the scope-of-work plan for that goal as docs/parity/ (docs only, no code):

  • PLAN.md — the goal, the three groups, the wave structure (0: unblockers → 1: Windows core → 2: codex correctness → 3: onboarding repairs → 4: big rocks behind decisions), and the four open decisions (Windows signing, codex headless, steering, ingest naming).
  • WINDOWS.md / CODEX.md / ONBOARDING.md — every subtask with: why it exists, suggested implementation with file:line and origin/main prior-art pointers, tips/gotchas, what NOT to build, how to verify by exit codes, and a Manual check — a walk-and-confirm checklist (MANUAL-PARITY style) the developer goes through to validate the functionality and feed back on; an item is not done until its checklist has been walked. Each wave carries an eval subtask that proves the wave end-to-end.

Drawn from a three-track deep review (2026-08-11): onboarding on origin/main vs the rebuild; a full Windows-compatibility sweep including PR #45; a codex↔claude capability audit. Load-bearing findings re-verified on dev@3ccbab9.

Headline conclusions the plan encodes:

  • Windows: the app already builds/boots there in CI; the core product fails for one dominant reason (model-engine PATH composed with :) with solved prior art on origin/main; PR audio: a dead input is one whose level never moves — the Windows run, three cross-platform defects, and a CI leg that can go red #45 + its two carve-outs + the CI npm ci root cause are Wave 0.
  • Codex: core runtime is at parity; the fixes are two real bugs (internal-run profile, hardcoded side-ask model), one mis-affordance ("Allow always"), one lying surface (model picker) — extended to a per-session vendor choice: pick codex/claude and their models in the existing model menu when opening a session, locked to the vendor once started (C4b), test depth, and a condensed context index — headless agents on codex is the one L item, parked behind a design decision.
  • Onboarding: keep our gate model (it's better than main's), add main's in-app repairs (runs-probe, installer, login, token-migration shim) — and never port main's global-config provisioning; it would undo the W6 hermetic boundary.

Test Plan

Docs-only PR — no code paths change.

  • file:line references spot-checked against dev@3ccbab9 (PATH :-split, claude-only agents runner, hardcoded side-ask model all still present)

🤖 Generated with Claude Code

https://claude.ai/code/session_017CqsGQBCrEbpAjY9cR9S3q

liorrutenberg and others added 4 commits August 11, 2026 17:03
… a clean device

Handover plan from the 2026-08-11 three-track review: docs/parity/PLAN.md
(goal, waves, dev process, open decisions) + one doc per group (WINDOWS,
CODEX, ONBOARDING), every subtask specified with reason, suggested
implementation with prior-art pointers, tips, what NOT to build, and
exit-code verification. Baseline re-verified on dev@3ccbab9.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017CqsGQBCrEbpAjY9cR9S3q
MANUAL-PARITY style: real build, human eye. Walking the checklist and
confirming (or reporting the diverging box) is part of finishing the item.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017CqsGQBCrEbpAjY9cR9S3q
Comment thread docs/parity/CODEX.md
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`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 codex/spawn.rs:150-153 is cited as documenting the hermetic-seal residue (shell_environment_policy, personality, future keys), but those lines are the codex mcp list invocation 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.

Copy link
Copy Markdown

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-152 are exactly the two keys — "shell_environment_policy — affects how the shell inherits env vars" / "personality — no handshake field; wiping to "" errors" — sitting directly above isolation_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 stated 3ccbab9 baseline — both predate w6, so there the keys don't exist and :150-153 lands in the codex mcp list fallback, 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 five features.*=false kills (isolation_args, :154-176) + the mcp_servers disable table (:106-129, appended last in build_args :195-200) — not the disable table alone.

Comment thread docs/parity/CODEX.md
### 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 codex/spawn.rs:17-21 is cited as documenting that fixed models 400 on ChatGPT-subscription accounts ("model not supported"); those lines are use statements and no such note exists anywhere at head — the closest rule is codex_model() in src-tauri/src/transport.rs:218-224, which drops only Anthropic-looking names (so the hardcoded gpt-4o-mini passes through to codex) and gives a stale-setting rationale, not a 400. Re-ground C2's premise on real code or mark the 400 behavior as unverified.

@sebastian-ssvlabs sebastian-ssvlabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-at: f900af1

Comment thread docs/parity/ONBOARDING.md
silent spinner.
- [ ] On Windows (post-Wave-1): same walk via the winget/PowerShell arm.

### O3 — In-app vendor login (M)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 O3/O4 don't call out Windows the way O1/O2 do. O1 explicitly ties in W1.1 (claude.cmd resolution) and W1.2 (no_console()); O2 explicitly walks the winget/PowerShell arm "post-Wave-1". O3 spawns a login child process and keeps its stdin open — exactly the kind of spawn that needs W1.1's PATH fix and W1.2's no_console() to work cleanly on Windows — but neither the suggested implementation nor the manual check mentions Windows at all. Worth a one-line cross-reference (or an explicit "inherits W1.1/W1.2" note) so whoever picks up O3 doesn't have to rediscover the dependency.

Comment thread docs/parity/ONBOARDING.md
- [ ] The old path still works: sign in from a terminal instead, refocus the
window → the card notices and goes green.

### O4 — Token migration shim for existing users (S)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Same gap for O4: the manual check is entirely macOS-Keychain-specific ("At no point does macOS raise a keychain password dialog"), with no mention of whether an equivalent migration is needed/possible on Windows (Credential Manager, per WINDOWS.md's "already fine" list) or whether the old app never shipped there (in which case say so explicitly, since the title promises mac/win onboarding parity).

Comment thread docs/parity/WINDOWS.md

## Wave 0 — unblockers

### W0.1 — Land PR #45 (Amir: Windows recording run + dead-input detector)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Nit: "PR is mergeable, zero reviews so far" is already stale as of this writing — PR #45 currently shows a review (Chris-ssvlabs, COMMENTED) and mergeable: CONFLICTING via gh pr view 45. Harmless for a living plan doc, but whoever picks up W0.1 should re-check current PR state before assuming "review and merge, no new work" still holds.

@stefan-ssv-labs stefan-ssv-labs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against dev@13c0afc. One framing fact that explains a whole class of corrections below: the plan's stated baseline 3ccbab9 — and this branch's own code snapshot — both predate w6 (77d9cc1, #46), so several items are drift, not errors (C4's picker, W1.4's names, the C2/seal citations). Worth re-stamping the docs' baseline to a post-w6 SHA and re-running the cites in one pass. Every file:line below was re-verified today against 13c0afc; the review itself went through three adversarial passes before posting.

Verdict up front: the plan is right in structure and mostly right in substance. The wave shape, the S/M/L-no-dates convention, the hermetic rulings, and the eval subtasks all hold. What follows: two new blockers found after drafting, one new item the plan can't see from its baseline, per-doc corrections, and one sequencing constraint that forces a reorder.

Two post-plan blockers (Discord, 2026-08-12)

Proposed W0.6 — dev does not compile on Windows. src-tauri/src/lib.rs:8 declares pub mod browser_host; ungated, and browser_host.rs:69 calls window.ns_window() — a method that exists only when compiling for macOS. Landed with PR #28 (merge f80e4e0), before this plan's baseline, so no plan item covers it. Everything else in src-tauri checks out clean for Windows by inspection (tauri-nspanel target-gated in Cargo.toml, native_clip has explicit windows arms, set_overlay cfg-split).

Fix shape: return None from the Host impl on every non-macOS target (not cfg(windows) — a Windows-only arm leaves the future ubuntu leg hitting the same missing method). Host::ns_window is documented as an NSWindow* and its only consumer is AppKit-typed CEF code, so None is the smallest true change; the CEF path already falls back to the OS webview when cef::is_ready() is false. The alternative hwnd shim (window.hwnd().ok().map(|h| WindowPtr(h.0)) — note bare .map(WindowPtr) does not type-check at the pinned windows 0.61.3) compiles but does NOT actually pre-wire Windows CEF (cef/views.rs stays AppKit-typed) and needs a third arm for Linux anyway.

Verify: cargo check --workspace green on a windows-latest leg. The fix is entirely in src-tauri/src/browser_host.rssrc/engines/browser is PR #28's territory per the boundary section. W0.6 unblocks W0.5's cargo legs: the widened workspace check cannot be born green without it.

ledger-net.test.ts on Windows — fix, don't platform-skip. The W0.5 tip says "fix or platform-skip". Fix: the BUILDERS allowlist is forward-slash literals while sources() builds real paths with join() (backslashes on Windows) — normalize before compare: relative(process.cwd(), path).split(sep).join("/") (also retires the manual cwd slice at :301). ~3 lines. Skipping would blind the builder-allowlist net on exactly the OS where a path-separator bug is most likely to slip in. CI caveat: a Rust Windows leg never runs vitest, so this fix is only verified by an explicit targeted vitest invocation on the Windows leg.

Proposed W0.7 — release/label artifact paths are stale

The cargo workspace root is the repo root (root Cargo.toml [workspace] lists src-tauri as a member; no CARGO_TARGET_DIR/target-dir override anywhere), so build output lands in repo-root target/. dev-release.yml reads target/... and its publishes are proven by run history. But release.yml's smoke/upload steps read src-tauri/target/... at 8 sites (:150, :193, :206-207, :260, :299, :335-336), and ci.yml's label-gated artifact upload at :285 does the same — paths a clean runner never produces. On the next v* tag the mac smoke-launch, both upload steps, and the entire Windows artifact glob come up empty.

This also undercuts the plan's baseline: WINDOWS.md leans on "the release job already builds and smoke-launches Windows" — that job proves nothing today and hasn't run since #28 merged. Worth rewording the baseline alongside the fix. The fix itself is a plain path edit (copy dev-release's paths).

Why nothing caught either one: between releases there is NO automatic Windows compile signal — ci.yml's rust matrix is macos-only, release.yml fires only on v* tags (none since #28), dev-release.yml is macos-15 only. W0.5's Windows leg isn't just about tests — it's the tripwire. Suggested one-liner for PLAN.md's Waves intro: "until W0.5-6 land, treat any Windows-touching merge as unverified."

WINDOWS.md corrections

  • W0.4 — premise stale, close it. The Frontend job passes on green PRs today; the killer was a stale Sync-SvelteKit step PR #28 removed. Keep only the npm ls --depth=0 guard, folded into W0.5's ci.yml PR.
  • W0.2 — re-scope. The legacy marker PARSES fine at head; the actual bug is check_with() feeding the legacy:... version string into the backup DIRECTORY NAME (data_guard.rs:203 → backups.rs:57), and : is invalid in Windows filenames. Replacing : alone isn't the fix — arbitrary malformed legacy content can carry any forbidden character, trailing dots/spaces, reserved device names, ... One centralized filename sanitizer + check_with() regression tests (arbitrary marker data, traversal, backup-listing round-trip). The existing legacy test only exercises parsing.
  • W0.5 — half done, re-scope. Three policy-dependent tests, not one: roots_reach_the_profile_as_real_paths (sandbox.rs:464, hard-asserts /private/tmp), an_unresolvable_root_refuses_to_produce_a_policy (:495-505, expects Err(Path) but policy() short-circuits to Unsupported on non-mac), and the integration test unwrapping a real policy (tests/headless_sandbox.rs:567-579). Remedy per test: rewrite the first with portable temp paths, make the second call roots.params() directly, gate only the real Seatbelt one. Plus the integration TARGET has a second compile blocker — ungated probing_cli() writes a #!/bin/sh shim and imports std::os::unix::fs::PermissionsExt (:107) — so the helper surface needs gating too, or the target won't compile on Windows at all. Also: the brains-storage clippy debt is already clear at head (re-verify on CI's pinned toolchain, then drop the item), and cargo check -p brains-browser --features chromium must stay macOS-only when the matrix widens (chromium pulls objc2/objc2-app-kit).
  • W1.1 — real, unchanged, still the product blocker. Five non-test split(':') sites in claude/path.rs plus codex/path.rs:51-52. Sequencing consequence below.
  • W1.2 — the list of five has drifted to 11 production Command::new sites at head (claude spawn ×2/control/path/side_question, codex spawn ×2/models, agents prelude/sandbox, recording sidecar); ~9-10 Windows-reachable.
  • W1.4 — names moved. PR #50 renamed packs→apps: BRAINS_ENABLE_APPS, scripts/dev/installed-apps.mjs. The doc says BRAINS_ENABLE_PACKS / installed-packs.mjs.
  • W1.5 — the per-platform-config suggestion doesn't survive merge semantics. Tauri merges configs with JSON Merge Patch (RFC 7396): arrays replace wholesale, so a tauri.windows.conf.json overlay must restate the entire windows[0] object and silently stops inheriting future base edits; dev:app also merges tauri.dev.conf.json last, overriding the overlay in dev. "Platform keys inside one config" isn't available either — app.windows is a flat array with no OS-conditional keys. Real choices: complete overlay (drift risk stated), runtime window mutation at setup, or a config-generation step. W1.5 also needs a frontend OS signal (root class / caption-button layout have no Windows detection today).
  • W1.6 — mostly done already. Jakarta Sans is self-hosted (fonts.css:29-68, tokens.css:39) and tauri-utils already defaults webviewInstallMode to DownloadBootstrapper { silent: true }. The icon remainder is unproven too: icons/icon.ico is already in the bundle icon list (tauri.conf.json:54). Re-scope validation-led: build, inspect the actual NSIS artifact, override only if wrong; keep the WebView2-less clean-VM walk.
  • Scheduled agents deserve a named Wave-1 item. They fail closed on non-mac by default: sandbox::policy() returns Unsupported (sandbox.rs:294-305) → readiness Unmet → the engine's preflight refuses the run (engine.rs:164-172). Only the dev-only BRAINS_HEADLESS_NO_SANDBOX=1 escape hatch permits unconfined runs. "Behaves the same on Windows" is unreachable until this is decided — even "agents are mac-only for v1, the readiness UI says so" is a decision. (Entangled with decision 2 — see decisions below.)
  • Strip the leftover calendar estimates at WINDOWS.md:287-299 (the execution plan removed estimates deliberately). Also flag: "Amir" at WINDOWS.md:19 — tracked files carry no names per CLAUDE.md; ownership lives in PR assignment.

CODEX.md corrections

  • C1 — real, and bigger than written. The codex arm builds CodexSessionSettings::interactive() unconditionally — no internal branch — AND always supplies the brains MCP endpoint + token (transport.rs:190-203, :216-230). A read-only filesystem sandbox does not contain a remote mutation-capable MCP server. Acceptance must be structural: for internal runs assert mcp_servers={}, no brains token in the child env, no MCP schema injection. A model-behavior test can't be the boundary. approvalPolicy=never + sandbox=read-only alone is not the fix.
  • C2 — mechanism confirmed, premise needs re-grounding. I verified this independently and land exactly where @Chris-ssvlabs's second comment does: SIDE_ASK_MODEL_OPENAI = "gpt-4o-mini" (side-ask.ts:26) reaches the CLI — codex_model() in transport.rs drops Anthropic-looking names only — and "fixed ids are documented to 400 on subscription accounts" is unverified; the spawn.rs:17-21 citation lands on use statements. The fix (send no model, symmetric with the picker's sentinel) is right regardless. +1 to re-grounding the premise or marking the 400 as unverified.
  • C4 — suggest closing as landed (decision for Lior, below). w6 (#46) shipped the provider-scoped picker, the live catalog probe (codex_model_probe), and end-to-end reported-model plumbing (notify.rs:36-50 → protocol-reduce.ts:162-168 → model.svelte.ts:138-160). The picker labels itself "MODEL FOR NEW SESSIONS / Used when this tab starts a new session" (ModelMenu.svelte:72, :99), so it never claims to show the running model. A read-only current-model badge would be a new, separately-justified ask — the plumbing makes it cheap. The doc's pointer model-options.ts:78-92 describes the pre-w6 file.
  • C4b — scaffolded but disconnected, and understated as M. StartOptions already carries provider+effort (session-store.svelte.ts:72-92) and Rust accepts both (commands/runs.rs:44-60, :145-176) — but the serialized run_start omits them (:275-294). The missing middle, plus hidden dependencies: effort is lost on resume (RunMeta has no effort field; resume hardcodes effort: None, commands/runs.rs:280 — verify the "codex re-applies from its own session settings" comment or persist effort); dual-provider readiness must be a NON-gating per-provider probe (a second vendor domain in the existing report would make isMet() demand both and block SetupGate for single-provider users); persisted tabs store only model, no vendor; side-question fallback never passes the parent provider. And it lands in two files already at/over the 400-line ceiling (session-store 554, workspace-tabs 407) — extract, don't grow. Honest M/L.
  • C5 — narrower. The replay-coverage gap stands (one golden vs claude's set), but warm resume is already live-tested (codex_live.rs:67-168, :276-279) — drop that subtask, keep replay/MCP/interrupt fixtures. New fixtures need invented users/prompts/payloads (the no-personal-data rule covers fixtures; the fixtures README currently prescribes only machine/token redaction).
  • C6 — the gating rule as written violates rule 6, and two dependencies are unnamed. "Gated-off contexts absent from the root index" is the wrong line: a gated context is SCOPED even when its gate is open — the codex root index must hold only UNGATED contexts (the materializer already draws exactly this line for skills, materializer.rs:95-105/:132-139) and must be manifest-generated so no base file names a removable app. Unnamed dependencies: (1) the index must tell codex WHERE a detail lives — details materialize only as .claude/skills/<name>/SKILL.md and codex can't invoke frontend IPC, so each entry needs a stable readable path or a codex-native retrieval route; (2) appending codex-only content requires splitting the writer — CLAUDE.md and AGENTS.md currently go through one identical writer+preamble loop (materializer.rs:126). Manual check should target settings.workspace_root (default <data-root>/workspace), not "data root's AGENTS.md".
  • Nits: CODEX.md:47 names brains-desktop-tauri (package is brains-desktop); C7 says brains-agents-local (crate is brains-local-agents).

ONBOARDING.md corrections

  • O4's "once, at first token lookup" is undefined as written. The token lookup is synchronous and called from many places (keychain.rs:33-87), while whoami validation is async (readiness.rs:67-86). Needs an explicit async startup/migration seam plus a persisted marker — written only on terminal outcomes (invalid token, definitive no-mirrors); timeout/offline/5xx stay retryable; single-flight so concurrent startup callers can't race; the marker never contains the token. With that scope O4 isn't an honest S. (Complementary to @sebastian-ssvlabs's O4 comment — his point is the missing Windows/Credential-Manager arm, mine is that the mac arm's trigger semantics are undefined; both need answering.) Everything else in O1–O4 holds against the tree, and the no-global-writes ruling matches the W6 boundary exactly.

Sequencing constraint (forces one reorder)

The full Windows test matrix cannot precede W1.1: the path tests themselves split tempdir-built PATH strings on ':' (claude/path.rs:371, :500), and a Windows drive letter breaks the assertions. So: minimal windows-latest cargo check --workspace leg first (the tripwire — with W0.6, or it's born red), W1.1 next (production code + tests in one PR), full clippy/test/3-OS-matrix widening after. The ubuntu leg additionally needs the native packages origin/main's matrix installs (WebKitGTK, GTK, AppIndicator, librsvg, patchelf).

Decisions (Lior)

  1. C4: close as landed? (see above — the remainder is a new product ask, not a parity gap)
  2. The non-mac confinement story — one decision, not two. The scheduled-agents gap and decision 2 (codex headless sandbox) are the same question: readiness is tied to Seatbelt, so a codex-native sandbox alone doesn't unlock Windows agents. Deciding them separately risks two half-answers.
  3. W1.5 approach: complete overlay vs runtime mutation vs config generation (trade-offs above) — fine to leave to the implementer with the trade-offs stated, but say so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants