Native Codex App Server integration + workspace/thread persistence#8
Conversation
Drives `codex app-server --listen stdio://` over JSONL with hand-rolled frame, pending-response table, and child-process driver under src/codex_spike/. Tests cover JSON-RPC framing (header-less wire format), pending correlation, and a live #[ignore]-gated end-to-end flow against real codex (initialize → account/read → thread/start → turn/start → agent deltas → turn/completed → clean shutdown). Shutdown drops stdin to send EOF, since the user-installed `codex` is typically a node shim that orphans the rust binary on SIGKILL.
Replaces the Phase 0 spike with the real src/app/codex/ module tree: rpc (frame + pending), schema (status mapping + sandbox enum), thread (reducer for thread/turn/item notifications), auth, approval (placeholder), process (child driver), and runtime (CodexRuntimeHandle, command/event loop, account/read on EnsureStarted, agent-message-delta coalescer). AppView gains codex_runtime/status/account fields plus handle_codex_event, wired through a new flume consumer in app::run mirroring dirty/exited/bell. Sidebar renders a Codex status row (disconnected / starting / ready / signed out / rate limited / failed). Runtime is lazy: spawned at startup but the codex app-server child isn't launched until something sends EnsureStarted. Tests: 57 unit tests + one #[ignore] live integration test that drives the runtime end-to-end (initialize → account/read → status events → shutdown).
Phase 2 wires a native Codex pane that replaces the terminal canvas
when active:
- AppView gains content_mode, codex_threads, active_codex_thread,
next_codex_thread_id, and a codex_composer buffer.
- New CodexCommand variants StartThread/SubmitPrompt/InterruptTurn
plus CodexEvent::ThreadStarted/ThreadStartFailed reconcile a local
Draft thread with the server's threadId.
- thread/start uses sandbox "workspace-write" (kebab-case) and
turn/start sends `input` (not `items`) — both confirmed by Phase 0.
- The sidebar shows a "Codex" group per project with thread rows,
running/waiting status dots, unread markers, and a + button to
start a new thread.
- Ctrl+Shift+X opens the pane and lazy-starts the runtime;
Ctrl+Shift+Enter submits; Esc returns to the terminal. Composer
keys (printable chars, backspace, newline) are captured locally.
- ServerNotification routing finds the right thread by server_thread_id
via the new notification_thread_id helper and delegates to the
existing thread::apply_notification reducer.
Phase 3 lifts pending approvals into the UI:
- ApprovalDecision + decision_payload move into codex/approval.rs.
Permissions approvals send { permissions } (granting the requested
subset on Accept and nothing on Decline); command/file-change
approvals send { decision }, omitting acceptForSession per spec.
- dispatch_codex_server_request parses the three supported approval
methods, stashes a CodexApproval on the matching thread, and lifts
status to WaitingForApproval.
- The Codex pane renders Accept/Decline/Cancel cards inline; the
sidebar shows an approval count badge on background threads.
- serverRequest/resolved clears pending approvals across threads,
even when the server resolves without a user decision (turn
interrupt).
Adds CodexCommand::ResolveServerRequest plumbing through the runtime
so user decisions reach the app-server as JSON-RPC responses to the
original request id.
69 unit tests pass (+5 for approval decision payloads, +6 for the
new runtime helpers); the live #[ignore] integration test still
passes in 0.34s.
Implements the Phase 4 surface from the spec: ChatGPT browser login, device-code fallback, rate-limit overlay, and `thread/list` / `resume` / `fork` for prior sessions. Wire / runtime: - `account/login/start` (chatgpt + chatgptDeviceCode), `account/login/ cancel`, and the `xdg-open` browser opener live in the runtime; failure to open the browser surfaces as `browser_open_failed` so the UI can fall through to device code without a user round-trip. - `account/login/completed`, `account/updated`, and `account/rateLimits/ updated` notifications drive the AppView's account state directly. Rate-limit overlays preserve the underlying account so the email stays visible. - `thread/list` is filtered by project cwd. `thread/resume` and `thread/fork` reuse the existing draft-thread reconciliation path — the runtime echoes `ThreadStarted` carrying the same `local_id` the GPUI side allocated. UI: - Codex pane gates the transcript behind a sign-in card when the account state is `SignedOutRequiresAuth` or a login is pending. The card surfaces the browser URL, falls back to device code on browser- open failure, and shows the user code prominently. - Pane header gains History + Fork buttons; History toggles a drawer that renders the `thread/list` result and lets the user resume a thread into a fresh local record. - Rate-limit banner renders above the transcript when the account state is `RateLimited`. Tests: - 19 new pure-helper tests cover login params/parsing, rate-limit payload parsing + state overlay, and thread-list/resume/fork wire shapes (88 unit tests pass overall). - Live `runtime_ensure_started_round_trip` still passes in ~0.33s.
Tightly-scoped polish pass focused on the highest-value items from the spec's Phase 5 list. The settings UI (nested config-shape migration), file-diff transcript rendering, skill mentions, inline rename, and the 5-minute approval expiry stay deferred and are tracked in `implementation-notes.html`. Server version capture: `parse_server_version` reads `serverInfo.version` from the `initialize` response; the runtime emits `CodexEvent::ServerInfo` and the sidebar status row surfaces it next to the account state. Useful both as a debug signal and as a starting point for the spec's "version compatibility" guidance. Thread archive: `CodexCommand::ArchiveThread` wraps `thread/archive` / `thread/unarchive`. The reducer already reacted to the resulting `thread/archived` notification (Phase 1 work), so the client side is just an inline pane-header button that returns the user to the terminal once the command is queued. Desktop notifications: background-thread approval requests and turn completions fire a `notify-rust` toast. Foreground activity stays silent — the existing unread/approval-count sidebar badges still hold for the active view, the toast just covers "user is in another pane when the long-running task finishes".
Four real-world issues showed up once the pane was used against a live codex run; this fixes all four together since they cluster around the item-render path. Tool rows were empty because extract_item_text only knew agentMessage and userMessage. It now dispatches on kind: commandExecution probes command (string or argv array), cwd, exit code (both wire shapes), and stdout/stderr; fileChange probes paths or path plus unifiedDiff; reasoning probes summary or parts[].text. All probes tolerate missing fields. Reasoning summaries lingered as empty rows. Added two reducer arms for item/reasoning/summaryTextDelta and summaryPartAdded that accumulate into a transient Reasoning item, and made item/completed for a Reasoning item remove it from the turn instead of marking it complete — so the row appears while the model is thinking and disappears as soon as it moves on. Long transcript text pushed the composer's Send button off-screen. The scroll container, every item/approval row, and the composer now carry w_full() + min_w_0(); the composer adds overflow_hidden() on the textarea and flex_shrink_0() on Send so the button stays anchored no matter the response length. Tool rows now render as a one-line "Tool - Bash <cmd>" / "Tool - Edit <path>" summary backed by a per-item-id HashSet on AppView; clicking toggles a monospace expanded body below the row. AgentMessage items route through a new codex_markdown module that parses with markdown-rs (1.0) and walks the mdast into GPUI blocks: paragraphs, headings (depth -> size offset), bulleted/ordered lists with indented tail blocks, fenced code on surface_background, block quotes with a border_l_2, thematic breaks. Inline emphasis/strong/ code/links flatten into their containing paragraph with markdown markers preserved so word-wrap stays correct (GPUI 0.2.2 doesn't make per-span styling easy inside a wrapping paragraph). 103 unit tests pass (+12 new) and the live integration test still passes in ~0.36s.
- Drop the dedicated "Codex" subheader; render terminals and threads as one flat list under each workspace, with a palette icon to distinguish threads. - The "+" button next to a workspace now opens a popup with two options: New Terminal / New Codex Thread. Click-outside dismisses. - Wire localImage attachments through turn/start input. - Composer key handling (paste, navigation, submit) and richer transcript rendering (tool groups, linkable file paths).
Adds a third entry next to New Terminal / New Codex Thread that spawns a regular PTY in the project root with the user's login shell running 'claude' directly via -lc, so the interactive session starts without having to type the command.
Default "New thread" placeholder is replaced with a short single-line title derived from the first user message — both eagerly at submit time and again when the server echoes the userMessage item. Custom titles set by the server (thread/name/updated) or the user are preserved.
Saved list of project root paths lives at ~/.config/quackcode/workspaces.json. On startup the app loads the saved set (skipping paths whose directories no longer exist) and falls back to the launch cwd when no file is present. add_project writes the file each time the list changes, so the project sidebar reopens to the same state on next launch.
Threads (with their turn/item transcripts) are saved to ~/.config/quackcode/codex_threads.json each time the reducer mutates them or the user performs a thread action (new/fork/archive/resume/ submit). On launch the saved set is reloaded and relinked to current projects by path; threads whose project is no longer open are skipped. Local thread ids are reassigned on load, the server-side thread ids are preserved so the user can keep talking to an existing thread.
📝 WalkthroughWalkthroughThis PR introduces a complete Codex AI assistant integration into Quackcode. The implementation adds a native app-server subprocess integration over JSON-RPC, client-side thread/turn/approval state management with disk persistence, a Markdown-rendered Codex pane UI, and wiring for workspace persistence, input handling, and app startup. The feature enables users to create Codex threads within projects, compose prompts with optional image attachments, view agent responses with rendered formatting, handle approval requests, and persist workspace state across sessions. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/app_view.rs`:
- Around line 465-469: The current handle_codex_event unconditionally calls
persist_codex_threads on every CodexEvent, causing excessive disk I/O; change it
to persist only when the event actually mutates durable thread state by either
(a) making handle_codex_event_inner return a bool (or enum) like
did_persist_needed indicating whether state changed and then call
persist_codex_threads only when true, or (b) match on event variants inside
handle_codex_event and only call persist for mutation events; alternatively,
implement a debounce/coalescing mechanism that batches writes (e.g., schedule a
single delayed persist) if high-frequency notification events occur—update
handle_codex_event, handle_codex_event_inner, and any scheduled-persist helper
accordingly.
In `@src/app/codex_view.rs`:
- Around line 155-160: The click handler is being registered even when the
control is disabled; change the fluent builder so handler registration is gated
by the `disabled` flag: move the `.on_mouse_down(MouseButton::Left, listener)`
call into the `when(!disabled, |el| { ... })` branch (or conditionally call
`.on_mouse_down` only when `!disabled`) and ensure the
`.cursor_pointer().hover(|s| s.opacity(0.8))` and `.on_mouse_down(...)` are both
inside that same `when(!disabled, |el| { ... })` closure; apply the same change
for the other occurrence around the `1152-1168` block so `disabled`,
`.on_mouse_down`, `MouseButton::Left`, and `listener` are never registered when
`disabled` is true.
- Around line 1191-1194: The InteractiveText element id is currently built only
from stable_text_id(&text) (in InteractiveText::new / ElementId::Name), which
causes duplicate messages to share the same id; change the id construction to
include a per-item stable key (for example combine the server item id and the
item's index or another unique per-item identifier along with stable_text_id) so
each rendered message gets a unique ElementId::Name; update the call site that
creates InteractiveText (the block using InteractiveText::new and
StyledText::new.with_highlights) to concatenate or format the id from
stable_text_id(&text) plus the item-specific key (e.g.,
"{server_id}-{index}-{stable_text_id}") so interaction targeting is stable and
unique.
In `@src/app/codex/approval.rs`:
- Around line 38-47: Currently the Accept branch returns the raw
approval.params["permissions"]; instead, use the typed/normalized permissions
stored on the CodexApproval model (e.g., a field like CodexApproval::permissions
or similar) and build the response from that normalized structure; update the
code path that handles ApprovalDecision::Accept (the block checking
approval.method == "item/permissions/requestApproval") to read the permissions
from the CodexApproval instance, serialize only the approved/validated fields,
and return json!({ "permissions": codex_approval.permissions }) rather than
echoing approval.params so unvalidated fields are never granted.
In `@src/app/codex/persistence.rs`:
- Around line 135-141: The current save path uses fs::write(path, bytes) which
can corrupt the live codex_threads.json if interrupted; change the save logic in
the function that calls serde_json::to_vec_pretty(&saved) (the block handling
Ok(bytes)) to perform an atomic replace: create a temporary file in the same
directory (e.g., derive tmp name from path), write the bytes there and
flush/sync the file, then rename (fs::rename) the temp file over path only after
the write succeeds; ensure any write/sync errors are handled the same way you
currently log fs::write errors and that the temp file is removed on failure.
In `@src/app/codex/process.rs`:
- Around line 163-170: The request() call registers a waiter via
self.pending.register() before sending; if self.write_frame(&frame) fails the
waiter is left in PendingTable. Fix by unregistering/clearing the pending entry
on write failure (after obtaining id from self.pending.register()), e.g. call
the appropriate removal method on PendingTable (self.pending.unregister(id) or
self.pending.remove(id)) immediately when write_frame() returns Err, then return
the write error so you don't leak the waiter; ensure the original error is
propagated.
In `@src/app/codex/runtime.rs`:
- Around line 584-619: The relay currently forwards ServerEvent->CodexEvent 1:1
because Coalescer::absorb is stateless; change Coalescer into a stateful
coalescer that buffers and merges consecutive item/agentMessage/delta
notifications by itemId and only emits merged CodexEvent(s) when a different
event arrives or a flush condition is met. Concretely: make Coalescer hold an
internal map or last_pending entry keyed by itemId (or the identifying field in
ServerEvent notifications), update/merge delta payloads inside Coalescer::absorb
when it sees an item/agentMessage/delta for the same itemId, and return an empty
Vec until a non-mergeable event forces emission of the merged CodexEvent(s);
update relay_server_events to call coalescer.absorb for each received
ServerEvent, send any returned CodexEvent(s) to app, and on loop exit (channel
closed) call a Coalescer::flush method to emit any remaining buffered events
before returning.
In `@src/app/input.rs`:
- Around line 95-99: When ContentMode::Codex is active and the composer has
focus, keystrokes must not fall through to the PTY; update the input-handling
logic so that when this.content_mode == ContentMode::Codex &&
!this.sidebar_focused you do not forward keys to the terminal: either (A) if
handle_codex_composer_key(this, ks, mods, cx) is called, make the Codex path
always consume the key (return) even when handle_codex_composer_key returns
false, or (B) keep handle_codex_composer_key as-is but add a guard around the
PTY/terminal dispatch (the code currently forwarding keystrokes to the PTY on
lines 122-124) so it only runs when this.content_mode != ContentMode::Codex; use
the identifiers this.content_mode, ContentMode::Codex, this.sidebar_focused,
handle_codex_composer_key, and the PTY/terminal dispatch site to locate and
change the logic.
In `@src/app/mod.rs`:
- Around line 55-58: The code unconditionally calls
codex_handle.send(CodexCommand::EnsureStarted) which breaks lazy-start; change
it so EnsureStarted is sent only when Codex is likely to be used (e.g., behind
the existing lazy-start condition or a user-action hook). Locate the call to
codex_handle.send and wrap it with the appropriate conditional (the same
predicate used elsewhere for lazy-start, or a new flag like should_prewarm_codex
derived from user config/UI intent) or move the send into the handler that runs
when the user opens Codex; ensure the symbol CodexCommand::EnsureStarted remains
unchanged and only its invocation is gated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1137773f-f7c9-40b1-a7b0-b461dd1405e2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.tomlsrc/app/app_view.rssrc/app/codex/approval.rssrc/app/codex/auth.rssrc/app/codex/mod.rssrc/app/codex/persistence.rssrc/app/codex/process.rssrc/app/codex/rpc.rssrc/app/codex/runtime.rssrc/app/codex/schema.rssrc/app/codex/thread.rssrc/app/codex_markdown.rssrc/app/codex_view.rssrc/app/input.rssrc/app/mod.rssrc/app/project.rssrc/app/session.rssrc/app/sidebar.rssrc/app/workspace_state.rs
- Persist codex_threads.json only at durable checkpoints (thread/turn/item events), not on every notification — keeps per-token deltas off the disk. - Atomic write for codex_threads.json via temp file + rename so an interrupted save can't leave a partial JSON that load_codex_threads_from silently treats as empty state. - Clean up the pending RPC waiter when write_frame fails so repeated send failures don't leak entries until cancel_all. - Activate delta coalescing in the live relay path: drain bursts via try_recv and run coalesce_agent_message_deltas before pushing into the bounded app channel. - Stop forwarding keystrokes to the PTY when ContentMode::Codex owns focus — unhandled keys like Up/Tab/Ctrl-C no longer interrupt the hidden terminal. - Gate header_button and composer_button click handlers behind !disabled so disabled controls are truly non-interactive. - Build unique InteractiveText element ids from a per-item stable key (server_item_id) so duplicate messages don't share an id.
CI was failing on `cargo clippy --all-targets -- -D warnings`: - derive Default for CodexAccountState / SandboxMode via #[default] - start_client takes &Path, not &PathBuf - compare PathBuf with &Path / DEFAULT_CODEX_BINARY str directly - replace `while let Ok(_) = …` with `.is_ok()` - box ErrorObject inside ClientError::Rpc so the enum stays small - allow too_many_arguments on spawn_pty_session_with_command Also addresses the two remaining CodeRabbit findings: - CodexApproval now captures the permission grant at construction time (CodexApproval::new) into `granted_permissions`. Accept builds its response from that field, so a permissions field the approval UI didn't render can't be granted by a later raw-params read. Adds a regression test that mutates `params` post-construction and confirms the response still reflects the originally-captured grant. - Drop the unconditional CodexCommand::EnsureStarted prewarm at app launch. ensure_codex_started() already runs on every entry point that opens Codex (open_codex_pane / new_codex_thread / new_codex_thread_in_project / login flow), and the worker is a single FIFO queue, so the prewarm only saved binary-spawn latency at the cost of running the child for users who never open Codex.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/app_view.rs (1)
640-657:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRecompute thread status after resolving the last approval.
This branch removes the pending approval but never mirrors the
recompute_status(...)call used when approvals are added below. If that was the last pending approval, the thread can stay stuck inWaitingForApprovaluntil some later notification happens.Suggested fix
if method == "serverRequest/resolved" { if let Some(req_id) = params.get("requestId").and_then(|v| v.as_u64()) { let mut changed = false; for t in &mut self.codex_threads { if let Some(idx) = t .pending_approvals .iter() .position(|a| a.request_id == req_id) { t.pending_approvals.remove(idx); + t.recompute_status(crate::app::codex::ServerThreadStatus::Active); changed = true; } } if changed { cx.notify();Also applies to: 738-740
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/app_view.rs` around lines 640 - 657, When removing a pending approval in the "serverRequest/resolved" branch, the code updates codex_threads but never calls recompute_status for the affected thread(s), which can leave a thread stuck in WaitingForApproval; after removing the approval (inside the loop where pending_approvals.remove(idx) and changed = true), call the same recompute_status(...) used elsewhere for that thread (use the same parameter pattern as the add-approval branch) so the thread's status is recalculated immediately, and keep the existing cx.notify() behavior; apply the same fix to the other identical spot referenced (the branch around the 738-740 area).src/app/mod.rs (1)
65-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't overwrite the saved workspace list with the filtered startup set.
Filtering
load_workspaces()down to existing directories and then immediately callingsave_workspaces(&initial_paths)turns a transiently missing workspace into a permanent config loss. If all saved paths are unavailable at launch, this also replaces the whole list withcwd.💡 Minimal fix
- let saved_workspaces: Vec<PathBuf> = load_workspaces() - .into_iter() + let saved_workspaces = load_workspaces(); + let restored_workspaces: Vec<PathBuf> = saved_workspaces + .iter() + .cloned() .filter(|p| p.is_dir()) .collect(); - let initial_paths: Vec<PathBuf> = if saved_workspaces.is_empty() { + let initial_paths: Vec<PathBuf> = if restored_workspaces.is_empty() { vec![cwd.clone()] } else { - saved_workspaces + restored_workspaces }; @@ - save_workspaces(&initial_paths); + if saved_workspaces.is_empty() { + save_workspaces(&initial_paths); + }Also applies to: 100-102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/mod.rs` around lines 65 - 73, The code currently replaces the persisted workspace list by saving the filtered startup set (initial_paths), which can permanently lose entries that are temporarily missing; change the startup logic so load_workspaces() is filtered for runtime use but do not call save_workspaces(&initial_paths) unconditionally on startup — keep saved_workspaces intact and only call save_workspaces(...) when the user explicitly updates workspaces (or when you intentionally prune and persist invalid entries after explicit confirmation); locate symbols saved_workspaces, initial_paths, load_workspaces, save_workspaces, and cwd to implement this conditional save behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/app/app_view.rs`:
- Around line 640-657: When removing a pending approval in the
"serverRequest/resolved" branch, the code updates codex_threads but never calls
recompute_status for the affected thread(s), which can leave a thread stuck in
WaitingForApproval; after removing the approval (inside the loop where
pending_approvals.remove(idx) and changed = true), call the same
recompute_status(...) used elsewhere for that thread (use the same parameter
pattern as the add-approval branch) so the thread's status is recalculated
immediately, and keep the existing cx.notify() behavior; apply the same fix to
the other identical spot referenced (the branch around the 738-740 area).
In `@src/app/mod.rs`:
- Around line 65-73: The code currently replaces the persisted workspace list by
saving the filtered startup set (initial_paths), which can permanently lose
entries that are temporarily missing; change the startup logic so
load_workspaces() is filtered for runtime use but do not call
save_workspaces(&initial_paths) unconditionally on startup — keep
saved_workspaces intact and only call save_workspaces(...) when the user
explicitly updates workspaces (or when you intentionally prune and persist
invalid entries after explicit confirmation); locate symbols saved_workspaces,
initial_paths, load_workspaces, save_workspaces, and cwd to implement this
conditional save behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51870583-ed1a-45f0-9a8b-41be7159923c
📒 Files selected for processing (12)
src/app/app_view.rssrc/app/codex/approval.rssrc/app/codex/auth.rssrc/app/codex/persistence.rssrc/app/codex/process.rssrc/app/codex/rpc.rssrc/app/codex/runtime.rssrc/app/codex/schema.rssrc/app/codex_view.rssrc/app/input.rssrc/app/mod.rssrc/app/session.rs
Summary
Brings native Codex App Server support to QuackCode and adds the missing persistence/UX layer around it.
Codex integration (Phases 0–5):
New in the tail of this branch:
Test plan
Summary by CodeRabbit
New Features
Improvements