feat: Tauri 2 desktop GUI (M2) — kernel as stdio sidecar - #16
Merged
Merged
Conversation
M2 starts here: apps/gui is the Tauri 2 + React + Vite frontend slot that DESIGN.md reserved. This slice is toolchain-only (no src-tauri yet) so the workspace lands with every repo gate green before any GUI logic exists. The webview needs the DOM lib, which would poison the Bun-typed root tsconfig project, so apps/gui is excluded from the root project and typechecks as its own project (tsc -p apps/gui) — the root typecheck script now runs both. Biome needed tailwindDirectives enabled to parse Tailwind v4's CSS syntax. shadcn is configured (components.json) but no components are vendored yet; the cn() helper and its deps arrive with the first component so knip stays clean at every slice boundary.
…inimal chat Proves the full M2 pipe end to end: webview → Tauri IPC → Rust-spawned kernel (minerva acp on stdio) → session/update stream → SessionStore render. Verified live: tauri dev creates a real session through the kernel, a Vite full reload reattaches to the same kernel process instead of spawning a second one, and killing the app leaves no orphan (kernel exits on stdin EOF). Rust owns the child process — a webview-held child would be orphaned on every HMR reload — and does its own byte-level line framing so plugin event chunking and UTF-8 chunk boundaries can't corrupt frames. The webview gets one event per complete frame (minerva://line) plus minerva://exit; all restart/reconnect policy stays in TS. No shell capability is granted to the webview: the sidecar bridge is app-defined commands only. The webview-side Transport adapter is DI-split (sidecar-bridge.ts is the only module touching @tauri-apps/api) so the framing codec is unit-testable with a fake bridge under bun test. protocol: stdio.ts now uses web-standard TextDecoder instead of node:string_decoder — the module loads in the GUI webview via the package index, where Vite externalizes node: builtins and the import throws at module eval (this was invisible until the webview went blank).
…ri build
De-risks the one open assumption in the M2 design early: Tauri does strip
the target-triple suffix from externalBin at bundle time and co-locates the
files with the app executable (Contents/MacOS/{minerva-gui, minerva, rg}),
so the kernel's process.execPath sits beside rg and resolveRgPath works
with zero kernel changes. Verified live: the bundled .app launched via
Finder (no terminal env) spawns the compiled kernel, completes the protocol
round-trip against settings-stored credentials, and Cmd+Q kills the kernel
through the RunEvent::Exit hook.
prepare-sidecar.ts reuses the repo's release build (the host-only
minerva+rg pair) and derives the triple from rustc -vV, which is
authoritative and always present. It is only needed before `tauri build`:
with externalBin configured, tauri-build hard-fails compilation when the
binaries are missing, which would have broken `tauri dev` on fresh clones —
so the dev script layers tauri.dev.conf.json over the config to drop
externalBin from the dev loop (dev spawns the kernel from source anyway).
Brings the GUI transcript to feature parity with the TUI: markdown assistant messages (model output only, DOMPurify-sanitized — assistant HTML is untrusted input), streaming-thought rolling tail with a collapsible full text once done, tool items with status dot / subagent task-progress line / output preview, unified inline diffs on the TUI's 20-line budget, the plan checklist, the usage/context status line, and Esc-to-cancel. The pure display helpers the TUI already had (diffLines/clipDiff line diff, clipLines/thoughtTail/formatTokens) move from packages/cli into @minerva/client rather than being duplicated: they are exactly the "shared frontend core" that package exists for (design decision #8), and both frontends must render identical previews and token counts. Their tests move with them; the CLI now imports them from the client package.
`minerva acp --allow-unconfigured` (acp-only, parse-time enforced) skips the API-key hard exit so a GUI host can reach the kernel before any key exists — the first-run config dialog (next slice) drives minerva/config/set_model over the protocol, exactly like the TUI's /config. The kernel already tolerates a keyless provider; only the gate moved. Conformance-tested over a real process boundary: keyless + flag answers initialize, keyless without the flag still exits 1. The GUI spawns with the flag in both dev and release. The permission dialog replicates the TUI bridge seam (client constructed before React mounts, requests resolved by whatever UI is attached) but as a FIFO queue, because parallel tool calls can stack requests in a GUI. Hotkey parity with the TUI — y/a/n by option KIND, Esc cancels the whole turn (ACP cancelled outcome) — and the same field-sniffed rawInput preview (command / edit diff / new-file content / URL) so MCP tools with matching shapes get previews for free. Hand-rolled overlay rather than a generic dialog component: hotkey-first modal flow is the whole interaction.
Adds minerva/config/state — the read half of config/set_model. The TUI computes provider choices host-side (registry + where each key was found), but a frontend on the far side of a pipe can't, so the kernel now answers with the live model ref (the provider id is the ref truth), a per-provider keySource (env | settings | none, blank-aware to match resolveApiKey), and needsApiKey for the live provider. A live provider outside the registry reports needsApiKey: false — there is nothing a config dialog could usefully demand a key for. The GUI boots through it: connect → getConfigState → if needsApiKey, hold before any session exists and open the config dialog (one-screen provider/key/model form, mirroring the TUI's /config steps); submitting drives the existing minerva/config/set_model persist + hot-swap, then the first session opens — no restart. The same dialog reopens from a footer button for later model switches. Verified live: with a scrubbed env and empty data dir the keyless kernel stays up and the app holds at the dialog with no session created.
The GUI now covers the TUI's whole session surface: a native folder picker (tauri-plugin-dialog) opens another project, the session browser lists the kernel's JSONL index for the cwd (TUI-created sessions included) and resumes via session/load replay, plus new-session, a mode picker, a profile picker, compact, and clickable /skill autocomplete in the composer (skill text goes to the kernel verbatim — it expands known skills, same as the TUI). client: newSession/loadSession now surface the kernel's already-typed SessionModeState instead of dropping it after seeding the store — a mode picker needs the available list, not just the current id. Additive change, covered by a new client test. Switching sessions cancels any running turn and detaches the old store (loadSession refuses to overwrite a live registration by design). Skills and profiles are fetched per-cwd and failure-tolerant: a project without them must never block the session.
One kernel, many projects: tabs each hold a cwd + persisted sessionId (pure reducer, localStorage-persisted), multiplexed over the single connection — session/new takes cwd per session, so sidecar-per-tab would buy nothing but N crash domains. Sessions materialize lazily: only the active tab holds a live store; background tabs rebuild from JSONL replay on activation. That same property makes crash recovery trivial — drop every Session object, keep tab state, re-ensure on demand. The kernel lifecycle moves into a manager (kernel-manager.ts) that owns spawn, client construction, and recovery: on kernel death it cancels all pending permission requests (a modal must not outlive its kernel), builds a FRESH client over a fresh transport (a dead Connection's pending state is unrecoverable), and auto-respawns exactly once — the allowance is only replenished by a manual restart, so a kernel that dies after every boot can't spin forever. ensureTabSession falls back from resume to a fresh session, making stale persisted ids self-healing. Verified live: kill -9 of the kernel auto-respawned a new one within seconds; killing the respawn stayed down awaiting the banner's restart. Manager, tab reducer, persistence serde, fallback, and permission cancellation are unit-tested against a fake JSON-RPC-answering bridge.
Split view over the same shared unified diff (diffLines/clipDiff from @minerva/client): alignDiffRows pairs the i-th removed line with the i-th added line inside each changed region — the classic split-view pairing — with the longer side finishing against blanks and gap/note lines becoming full-width bands. The unified/split choice is a persisted preference; both views share the transcript's 20-line clipping budget so a huge diff can't flood the transcript in either mode.
A GUI can do what the terminal bell can't: tell the user their turn finished while they were in another app. decideNotification is a pure matrix — notify only when the window is unfocused, the turn ran at least 5s (shorter means the user hasn't left), it wasn't user-cancelled, and notifications aren't muted; abnormal stops (token/request limits, refusal) escalate the title. Delivery goes through tauri-plugin-notification with the OS permission requested on first use and denial treated as silence. A footer bell toggle persists the mute.
The M2 build is done, so the docs stop saying "planned": PROTOCOL.md's transport table names the GUI sidecar as a live stdio consumer and gains the minerva/config/state reference (the read half of config/set_model, including why it exists — remote frontends can't compute provider choices host-side); DESIGN.md's M2 milestone records the decisions made during the build (Rust-owned child with Rust-side framing, one-kernel-many-tabs with lazy per-tab stores, protocol-driven first-run config, externalBin pair packaging with release-CI deferred); README gains the GUI quickstart; and CONTRIBUTING adds the Rust prerequisite, the apps/gui test-layout rows with the pure-modules-only coverage rationale, the GUI manual smoke checklist, and fixes the stale Bun pin (CI runs 1.3.14).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughAdds a Tauri 2 React desktop GUI with a Rust-managed kernel sidecar, shared protocol/client utilities, configuration-state handling, session and tab management, chat rendering, native integrations, CLI keyless startup, tests, and updated documentation. ChangesGUI platform and sidecar
Shared protocol and CLI support
Documentation and repository configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (10)
CONTRIBUTING.md (1)
16-17: 🩺 Stability & Availability | 🔵 TrivialConsider adding a native Rust compile check to CI.
As documented,
bun run verifydoes not validate the Tauri backend, so Rust regressions can pass CI unnoticed. A platform-neutralcargo checkor dedicated macOS build job would catch these without requiring release bundling/signing.🤖 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 `@CONTRIBUTING.md` around lines 16 - 17, Add a platform-neutral Rust validation step to the CI workflow, using cargo check or an equivalent dedicated Tauri backend compile check, so the Rust side is validated alongside the existing bun run verify gate without requiring release bundling or signing. Update the CONTRIBUTING.md documentation to reflect the new CI coverage.apps/gui/src/components/HeaderBar.tsx (1)
60-88: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProfile/mode
<select>elements have no accessible name beyondtitle.
titlealone is an unreliable accessible-name source across screen readers. Consider addingaria-label="Profile"/aria-label="Session mode"alongside the existingtitlefor a proper accessible name.🤖 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 `@apps/gui/src/components/HeaderBar.tsx` around lines 60 - 88, Add explicit aria-label attributes to the profile and session mode select elements in HeaderBar, using “Profile” and “Session mode” respectively, while preserving the existing title attributes and selection behavior.apps/gui/src/components/ConfigDialog.tsx (1)
60-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo client-side gate for providers that require a key but have none found.
canSubmitnever checksselected?.requiresApiKey && selected.keySource === "none" && !apiKey. A user can submit with a provider that needs a key and none configured, only to have the backend reject it. Adding this check would avoid a needless round-trip and give feedback before the async call.🤖 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 `@apps/gui/src/components/ConfigDialog.tsx` around lines 60 - 64, Add a client-side requirement to canSubmit in ConfigDialog: prevent submission when the selected provider requires an API key, keySource is "none", and no apiKey is entered. Preserve the existing busy, name, model, and custom-base-URL checks.apps/gui/src/app.tsx (1)
493-505: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueVerify
outline-nonestill yields the intended focus-visible reset under Tailwind v4.Tailwind v4 changed
outline-noneto a plainoutline-style: nonereset; the accessible "invisible-but-present" outline reset (useful for Windows High Contrast Mode) is nowoutline-hidden. This textarea combinesoutline-nonewith a customfocus:ring-2 focus:ring-ring, the exact pattern the v4 migration guide calls out. Visually the ring still shows on focus, but the semantic outline-removal behavior differs from v3.🤖 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 `@apps/gui/src/app.tsx` around lines 493 - 505, Update the textarea’s focus reset utility in the JSX element identified by its placeholder and focus ring classes: replace the Tailwind v3-style outline reset with the Tailwind v4-compatible utility that preserves an invisible outline for accessibility, while keeping the existing focus:ring-2 focus:ring-ring behavior unchanged.apps/gui/src/components/chat/DiffView.tsx (2)
24-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winToggle buttons don't expose pressed state to assistive tech.
The active view is conveyed only via background color; adding
aria-pressed={view === option}would let screen readers announce the current selection.🤖 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 `@apps/gui/src/components/chat/DiffView.tsx` around lines 24 - 47, Add aria-pressed={view === option} to each toggle button rendered in DiffView’s view-option map, preserving the existing visual state and toggle behavior so assistive technologies can identify the active view.
15-22: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDiff computation reruns on every render, including view-toggle clicks.
clipDiff(diffLines(...))runs unconditionally on Line 17 even thoughdiff.oldText/diff.newTextare stable across re-renders (e.g. togglingview, or a parent re-render elsewhere in the transcript). Memoizing avoids redoing the diff/clip work when only the view mode changes.♻️ Proposed fix
-import { clipDiff, type DiffLine, diffLines } from "`@minerva/client`"; -import { useState } from "react"; +import { clipDiff, type DiffLine, diffLines } from "`@minerva/client`"; +import { useMemo, useState } from "react"; import { alignDiffRows, type SplitCell } from "../../lib/diff-rows"; ... const [view, setView] = useState(loadPreference); - const lines = clipDiff(diffLines(diff.oldText, diff.newText), DIFF_LINE_CAP); + const lines = useMemo( + () => clipDiff(diffLines(diff.oldText, diff.newText), DIFF_LINE_CAP), + [diff.oldText, diff.newText], + );🤖 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 `@apps/gui/src/components/chat/DiffView.tsx` around lines 15 - 22, Memoize the computed `lines` value in `DiffView` so `diffLines` and `clipDiff` rerun only when `diff.oldText` or `diff.newText` changes. Preserve the existing line-cap behavior and rendering while avoiding recomputation during view toggles or unrelated re-renders.apps/gui/src/components/chat/ToolItem.tsx (1)
32-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiff
pathis dropped when rendering tool diffs.
item.diffcarries apathfield, but it's discarded here —DiffViewonly rendersoldText/newText. Users can't tell which file was changed from the tool-call view.💡 Suggested fix: surface the file path above the diff
{item.diff ? ( <div className="ml-5"> + <div className="font-mono text-xs text-muted-foreground">{item.diff.path}</div> <DiffView diff={item.diff} /> </div> ) : preview ? (🤖 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 `@apps/gui/src/components/chat/ToolItem.tsx` around lines 32 - 36, Update the item.diff rendering branch in ToolItem to surface item.diff.path above or alongside the DiffView, ensuring users can identify the changed file while preserving the existing oldText/newText diff rendering.packages/kernel/src/kernel.ts (1)
796-810: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
keySourceprecedence duplicatesresolveApiKey's logic inline.The blank-aware env→settings→none precedence here is hand-rolled and must stay manually in sync with
resolveApiKey(imported and already used just below forneedsApiKey). A future change to key-resolution precedence inresolveApiKeycould silently diverge from whatkeySourcereports.Consider deriving
keySourcefromresolveApiKey's own resolution (e.g. have it optionally report which source satisfied it, or checkenv/storedKeysmembership using the same helper) to keep the two paths from drifting.🤖 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 `@packages/kernel/src/kernel.ts` around lines 796 - 810, The keySource calculation in the providers mapping duplicates resolveApiKey’s env-to-settings precedence and can drift. Update resolveApiKey or add a shared resolution helper so the mapping derives both the resolved key source and needsApiKey from the same blank-aware logic, preserving env, settings, and none precedence without maintaining separate checks.apps/gui/src/lib/notify.ts (1)
21-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an exhaustive switch instead of a ternary chain.
Logic is correct today, but the final
elsebranch silently absorbs any futureStopReasonaddition as "The model refused the request." Aswitchwith asatisfies neverdefault would fail to compile instead of silently mislabeling a new stop reason.♻️ Suggested refactor
- const title = - input.stopReason === "end_turn" ? "Minerva finished a turn" : "Minerva needs attention"; - const detail = - input.stopReason === "end_turn" - ? "The reply is ready." - : input.stopReason === "max_tokens" - ? "The turn hit the output-token limit." - : input.stopReason === "max_turn_requests" - ? "The turn hit the request limit." - : "The model refused the request."; - return { title, body: `${input.project} — ${detail}` }; + const detail = ((): string => { + switch (input.stopReason) { + case "end_turn": return "The reply is ready."; + case "max_tokens": return "The turn hit the output-token limit."; + case "max_turn_requests": return "The turn hit the request limit."; + case "refusal": return "The model refused the request."; + case "cancelled": throw new Error("unreachable: filtered above"); + default: { + const _exhaustive: never = input.stopReason; + return _exhaustive; + } + } + })(); + const title = input.stopReason === "end_turn" ? "Minerva finished a turn" : "Minerva needs attention"; + return { title, body: `${input.project} — ${detail}` };🤖 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 `@apps/gui/src/lib/notify.ts` around lines 21 - 35, Update decideNotification’s stopReason-to-detail mapping to use an exhaustive switch rather than the nested ternary chain. Handle each existing StopReason explicitly and add a satisfies-never default so future StopReason additions produce a compile-time error instead of falling through to the refusal message.apps/gui/src/lib/tab-session.ts (1)
16-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent fallback loses the reason a resumed session failed to load.
The comment explains several plausible causes (deleted log, stale id, mid-life kernel restart), but the empty
catchgives no way to tell which one occurred without attaching a debugger. Given session recovery is a headline feature of this PR, a lightweight diagnostic would help triage user reports of "my tab always opens fresh."♻️ Suggested tweak
if (tab.sessionId) { try { return { session: await ops.load(tab.sessionId, tab.cwd), resumed: true }; - } catch { + } catch (cause) { + console.warn(`ensureTabSession: failed to resume ${tab.sessionId}`, cause); // Fall through to a fresh session in the tab's project. } }🤖 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 `@apps/gui/src/lib/tab-session.ts` around lines 16 - 28, Update ensureTabSession to capture the error from ops.load when a tab.sessionId is present and emit a lightweight diagnostic before falling back to ops.create. Include the tab/session context and error details in the existing logging mechanism, while preserving the current fresh-session fallback 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.
Inline comments:
In `@apps/gui/scripts/prepare-sidecar.ts`:
- Around line 26-37: Update the rustc result handling before parsing stdout:
validate rustc.success and ensure rustc.stdout exists before calling toString().
Preserve the existing failure message and exit behavior, while only extracting
the host triple after the spawn succeeded with readable stdout.
In `@apps/gui/src-tauri/src/sidecar.rs`:
- Around line 42-66: Update the release branch of kernel_command to resolve the
bundled externalBin sidecar using the artifact’s actual runtime name instead of
hardcoding dir.join("minerva"). Preserve the existing acp arguments and debug
behavior, and ensure packaged builds locate the sidecar next to the executable.
In `@apps/gui/src/app.tsx`:
- Around line 267-293: Preserve unsent drafts when switching tabs by lifting
Chat’s local draft state into the parent tab state. Add per-tab drafts storage
near the active tab/session state, pass the active tab’s draft and an
onDraftChange handler into Chat, and update the handler to store text by
activeTab.id; then remove Chat’s local draft useState and use the new props
throughout Chat.
- Around line 400-407: Update the stick-to-bottom useEffect around vm.items to
force scrolling to the latest message on its initial mount or first populated
transcript render, before applying the nearBottom guard. Preserve the existing
behavior of following subsequent updates only when the reader remains near the
bottom.
In `@apps/gui/src/components/ConfigDialog.tsx`:
- Around line 39-53: Preserve bare model identifiers when initializing the model
state in the ConfigDialog component. Update the model initializer around
currentProvider and providerName so a state.model without a slash uses the full
state.model value, while provider/model references continue using the segment
after the slash; ensure submitting without editing the field does not fall back
to selected.defaultModel.
- Around line 86-198: Update the ConfigDialog modal container to use dialog
semantics with role="dialog", aria-modal="true", and an accessible label or
title. Add Escape-key handling that invokes onClose when available, place
initial focus on the dialog or an appropriate first control when it opens, and
ensure keyboard focus is contained within the modal so users cannot tab into the
background overlay.
In `@apps/gui/src/components/PermissionDialog.tsx`:
- Around line 60-91: Update the PermissionDialog component’s modal container to
expose dialog semantics with role="dialog" and aria-modal="true", and implement
focus management that initially focuses an element inside the dialog and traps
Tab/Shift+Tab within its focusable controls until the dialog is answered. Keep
focus handling scoped to the dialog lifecycle and preserve the existing option
response behavior.
- Around line 39-55: Update the hotkey handling in the PermissionDialog
useEffect’s onKeyDown function so recognized options call preventDefault() and
stopPropagation() before responding. Keep the existing option lookup and
selected outcome behavior unchanged, while continuing to ignore unrecognized
keys.
In `@apps/gui/src/components/SessionBrowser.tsx`:
- Around line 39-92: The modal container in SessionBrowser should expose dialog
semantics and receive initial keyboard focus when opened. Update the inner
dialog element to use role="dialog", aria-modal="true", and an appropriate
accessible label, and add a ref with an effect that focuses it on mount while
preserving the existing Escape handling and focus cleanup behavior.
In `@apps/gui/src/lib/kernel-manager.ts`:
- Around line 56-79: Update the run function’s client.initialize/getConfigState
handshake to enforce a finite timeout, and ensure timeout handling terminates
the active bridge/sidecar process before transitioning to the existing "down"
error state. Preserve generation checks and prevent late handshake completion
from updating state after timeout or supersession.
In `@apps/gui/src/lib/native.ts`:
- Around line 14-22: Update notify() to enforce its documented best-effort
contract by wrapping the permission checks, permission request, and
sendNotification call in error handling that suppresses any thrown errors.
Preserve the existing permission flow and notification arguments, but ensure
notify() resolves without rejection when any native notification operation
fails.
In `@docs/DESIGN.md`:
- Around line 84-104: Update the introduction in DESIGN.md to reflect that the
Tauri 2 GUI shipped post-v0.3, replacing the outdated “CLI now, GUI later” and
v0.1 design-record framing. Align the introductory architecture/status summary
with the shipped GUI details described in the M2 section, while leaving the
technical decisions unchanged.
---
Nitpick comments:
In `@apps/gui/src/app.tsx`:
- Around line 493-505: Update the textarea’s focus reset utility in the JSX
element identified by its placeholder and focus ring classes: replace the
Tailwind v3-style outline reset with the Tailwind v4-compatible utility that
preserves an invisible outline for accessibility, while keeping the existing
focus:ring-2 focus:ring-ring behavior unchanged.
In `@apps/gui/src/components/chat/DiffView.tsx`:
- Around line 24-47: Add aria-pressed={view === option} to each toggle button
rendered in DiffView’s view-option map, preserving the existing visual state and
toggle behavior so assistive technologies can identify the active view.
- Around line 15-22: Memoize the computed `lines` value in `DiffView` so
`diffLines` and `clipDiff` rerun only when `diff.oldText` or `diff.newText`
changes. Preserve the existing line-cap behavior and rendering while avoiding
recomputation during view toggles or unrelated re-renders.
In `@apps/gui/src/components/chat/ToolItem.tsx`:
- Around line 32-36: Update the item.diff rendering branch in ToolItem to
surface item.diff.path above or alongside the DiffView, ensuring users can
identify the changed file while preserving the existing oldText/newText diff
rendering.
In `@apps/gui/src/components/ConfigDialog.tsx`:
- Around line 60-64: Add a client-side requirement to canSubmit in ConfigDialog:
prevent submission when the selected provider requires an API key, keySource is
"none", and no apiKey is entered. Preserve the existing busy, name, model, and
custom-base-URL checks.
In `@apps/gui/src/components/HeaderBar.tsx`:
- Around line 60-88: Add explicit aria-label attributes to the profile and
session mode select elements in HeaderBar, using “Profile” and “Session mode”
respectively, while preserving the existing title attributes and selection
behavior.
In `@apps/gui/src/lib/notify.ts`:
- Around line 21-35: Update decideNotification’s stopReason-to-detail mapping to
use an exhaustive switch rather than the nested ternary chain. Handle each
existing StopReason explicitly and add a satisfies-never default so future
StopReason additions produce a compile-time error instead of falling through to
the refusal message.
In `@apps/gui/src/lib/tab-session.ts`:
- Around line 16-28: Update ensureTabSession to capture the error from ops.load
when a tab.sessionId is present and emit a lightweight diagnostic before falling
back to ops.create. Include the tab/session context and error details in the
existing logging mechanism, while preserving the current fresh-session fallback
behavior.
In `@CONTRIBUTING.md`:
- Around line 16-17: Add a platform-neutral Rust validation step to the CI
workflow, using cargo check or an equivalent dedicated Tauri backend compile
check, so the Rust side is validated alongside the existing bun run verify gate
without requiring release bundling or signing. Update the CONTRIBUTING.md
documentation to reflect the new CI coverage.
In `@packages/kernel/src/kernel.ts`:
- Around line 796-810: The keySource calculation in the providers mapping
duplicates resolveApiKey’s env-to-settings precedence and can drift. Update
resolveApiKey or add a shared resolution helper so the mapping derives both the
resolved key source and needsApiKey from the same blank-aware logic, preserving
env, settings, and none precedence without maintaining separate checks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b445a4e-ed0c-46a4-a6ca-593a34a14bfd
⛔ Files ignored due to path filters (8)
apps/gui/src-tauri/Cargo.lockis excluded by!**/*.lockapps/gui/src-tauri/icons/128x128.pngis excluded by!**/*.pngapps/gui/src-tauri/icons/128x128@2x.pngis excluded by!**/*.pngapps/gui/src-tauri/icons/32x32.pngis excluded by!**/*.pngapps/gui/src-tauri/icons/64x64.pngis excluded by!**/*.pngapps/gui/src-tauri/icons/icon.icois excluded by!**/*.icoapps/gui/src-tauri/icons/icon.pngis excluded by!**/*.pngbun.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
.gitignoreCONTRIBUTING.mdREADME.mdapps/gui/components.jsonapps/gui/index.htmlapps/gui/package.jsonapps/gui/scripts/prepare-sidecar.tsapps/gui/src-tauri/Cargo.tomlapps/gui/src-tauri/build.rsapps/gui/src-tauri/capabilities/default.jsonapps/gui/src-tauri/icons/icon.icnsapps/gui/src-tauri/src/lib.rsapps/gui/src-tauri/src/main.rsapps/gui/src-tauri/src/sidecar.rsapps/gui/src-tauri/tauri.conf.jsonapps/gui/src-tauri/tauri.dev.conf.jsonapps/gui/src/app.tsxapps/gui/src/components/ConfigDialog.tsxapps/gui/src/components/HeaderBar.tsxapps/gui/src/components/PermissionDialog.tsxapps/gui/src/components/ProjectTabs.tsxapps/gui/src/components/SessionBrowser.tsxapps/gui/src/components/chat/DiffView.tsxapps/gui/src/components/chat/Markdown.tsxapps/gui/src/components/chat/PlanItem.tsxapps/gui/src/components/chat/ThoughtItem.tsxapps/gui/src/components/chat/ToolItem.tsxapps/gui/src/components/chat/Transcript.tsxapps/gui/src/components/chat/UsageFooter.tsxapps/gui/src/hooks/use-session-store.tsapps/gui/src/index.cssapps/gui/src/lib/diff-rows.tsapps/gui/src/lib/kernel-manager.tsapps/gui/src/lib/native.tsapps/gui/src/lib/notify.tsapps/gui/src/lib/permission-queue.tsapps/gui/src/lib/sidecar-bridge.tsapps/gui/src/lib/tab-session.tsapps/gui/src/lib/tabs.tsapps/gui/src/lib/tauri-transport.tsapps/gui/src/main.tsxapps/gui/test/diff-rows.test.tsapps/gui/test/kernel-manager.test.tsapps/gui/test/notify.test.tsapps/gui/test/permission-queue.test.tsapps/gui/test/tab-session.test.tsapps/gui/test/tabs.test.tsapps/gui/test/tauri-transport.test.tsapps/gui/tsconfig.jsonapps/gui/vite.config.tsbiome.jsondocs/DESIGN.mddocs/PROTOCOL.mdknip.jsonpackage.jsonpackages/cli/src/app.tsxpackages/cli/src/args.tspackages/cli/src/index.tsxpackages/cli/test/acp.test.tspackages/cli/test/app.test.tsxpackages/cli/test/args.test.tspackages/client/src/client.tspackages/client/src/diff.tspackages/client/src/format.tspackages/client/src/index.tspackages/client/test/client.test.tspackages/client/test/diff.test.tspackages/client/test/format.test.tspackages/kernel/src/kernel.tspackages/kernel/test/config.test.tspackages/protocol/src/stdio.tspackages/protocol/src/types.tstsconfig.json
Comment on lines
+42
to
+66
| fn kernel_command() -> Result<Command, String> { | ||
| if cfg!(debug_assertions) { | ||
| // CARGO_MANIFEST_DIR = <repo>/apps/gui/src-tauri at compile time. | ||
| let repo_root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) | ||
| .join("../../..") | ||
| .canonicalize() | ||
| .map_err(|e| format!("cannot resolve repo root: {e}"))?; | ||
| let entry = repo_root.join("packages/cli/src/index.tsx"); | ||
| let mut cmd = Command::new("bun"); | ||
| cmd.arg("run") | ||
| .arg(entry) | ||
| .arg("acp") | ||
| .arg("--allow-unconfigured"); | ||
| cmd.current_dir(repo_root); | ||
| Ok(cmd) | ||
| } else { | ||
| let exe = std::env::current_exe().map_err(|e| format!("cannot resolve app path: {e}"))?; | ||
| let dir = exe | ||
| .parent() | ||
| .ok_or("app executable has no parent directory")?; | ||
| let mut cmd = Command::new(dir.join("minerva")); | ||
| cmd.arg("acp").arg("--allow-unconfigured"); | ||
| Ok(cmd) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether build.rs or tauri.conf.json handle stripping/renaming the target-triple suffix.
cat apps/gui/src-tauri/build.rs 2>/dev/null
rg -n 'externalBin' apps/gui/src-tauri/tauri.conf.json
fd -HI 'minerva-*' apps/gui/src-tauriRepository: hutusi/minerva
Length of output: 686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- apps/gui/src-tauri/tauri.conf.json ---'
cat -n apps/gui/src-tauri/tauri.conf.json | sed -n '1,220p'
printf '\n%s\n' '--- apps/gui/src-tauri/src/sidecar.rs ---'
cat -n apps/gui/src-tauri/src/sidecar.rs | sed -n '1,240p'
printf '\n%s\n' '--- other packaging/build files mentioning binaries/minerva or externalBin ---'
rg -n 'binaries/minerva|externalBin|minerva-' apps/gui/src-tauri -g '!target/**'
printf '\n%s\n' '--- repo files named minerva* outside target ---'
fd -HI 'minerva*' . -E targetRepository: hutusi/minerva
Length of output: 9996
Resolve the release sidecar path in apps/gui/src-tauri/src/sidecar.rs:62
Command::new(dir.join("minerva")) does not match the bundled externalBin artifact, so packaged builds will fail to start the kernel unless the sidecar name is resolved at runtime or the bundle renames it.
🤖 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 `@apps/gui/src-tauri/src/sidecar.rs` around lines 42 - 66, Update the release
branch of kernel_command to resolve the bundled externalBin sidecar using the
artifact’s actual runtime name instead of hardcoding dir.join("minerva").
Preserve the existing acp arguments and debug behavior, and ensure packaged
builds locate the sidecar next to the executable.
…ad fallback) Six findings from the codex review of PR #16, all confirmed against the code before fixing: - App exit now shuts the kernel down through its durability path instead of SIGKILL: close stdin (the acp host's shutdown signal — kernel.close() cancels, drains up to 5s, flushes session logs), wait up to 7s, kill only as fallback. Cmd+Q was ironically the only violent exit path; SIGTERM already went through stdin EOF. Verified live: quit logs "kernel exited gracefully on stdin close". - Model refs with slashes survive the config dialog: provider is everything before the FIRST slash (registry contract), and submission always qualifies with the selected provider instead of slash-sniffing — TUI parity; openrouter-style ids like meta-llama/llama-3 now work. The form logic moved to a pure, tested module (config-form.ts). - Custom keyless providers persist requiresApiKey: apiKey !== "" (TUI parity); previously the registry defaulted them to key-required and resolution rejected them. - A tab's persisted session only falls back to a fresh one when the id is actually dead ("unknown session"); load-while-prompt-active, failed flushes, and transient I/O now surface via the notice bar and retry on tab re-activation — the kernel throws those loudly by design, and the blanket catch was silently replacing conversations with blank sessions. - The session browser no longer reverses the kernel's newest-first list. - Dropped the unused clipLines import left over from the helper move.
Owner
Author
Codex review round — dispositionAll six findings verified against the code and confirmed; fixed in 5707467.
Gates: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/gui/src/app.tsx (1)
279-281: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemounting
Chaton tab switch still discards the unsent draft.This was flagged in a previous review and persists:
key={activeTab?.id}unmounts/remountsChaton every tab switch, anddraftremains localuseStateinsideChat. Switching away and back loses any typed-but-unsent text.🤖 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 `@apps/gui/src/app.tsx` around lines 279 - 281, Remove the activeTab-dependent key from the Chat rendering in the app component so tab switches do not unmount and remount Chat, preserving its local draft state. Keep the existing activeSession and client conditional rendering unchanged.
🧹 Nitpick comments (1)
apps/gui/src/app.tsx (1)
200-218: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUnhandled rejection from
client.closeSessioninswitchWithinTab.Line 207 calls
client.closeSession(previous.id)without a catch handler. If the close fails (e.g., kernel transport error), the unhandled rejection could surface as an uncaught promise warning. The same pattern appears incloseTab(line 230), but there at least the session is being removed from the map regardless. Here, a failed close leaves the previous session in an indeterminate state while the new one is already active.♻️ Suggested fix: add error handling
if (previous && previous.id !== session.id && client) { if (previous.store.snapshot.busy) client.cancel(previous.id); - client.closeSession(previous.id); + client.closeSession(previous.id).catch(() => { + // best-effort close; session is already replaced + }); }🤖 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 `@apps/gui/src/app.tsx` around lines 200 - 218, Handle the promise returned by client.closeSession in switchWithinTab by attaching error handling that reports close failures through the existing error-reporting mechanism. Preserve the current session replacement, dispatch, and browser-closing flow regardless of whether closing the previous session succeeds.
🤖 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.
Duplicate comments:
In `@apps/gui/src/app.tsx`:
- Around line 279-281: Remove the activeTab-dependent key from the Chat
rendering in the app component so tab switches do not unmount and remount Chat,
preserving its local draft state. Keep the existing activeSession and client
conditional rendering unchanged.
---
Nitpick comments:
In `@apps/gui/src/app.tsx`:
- Around line 200-218: Handle the promise returned by client.closeSession in
switchWithinTab by attaching error handling that reports close failures through
the existing error-reporting mechanism. Preserve the current session
replacement, dispatch, and browser-closing flow regardless of whether closing
the previous session succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e814b359-881b-4e80-9399-d9b9bbcb20ae
📒 Files selected for processing (9)
apps/gui/src-tauri/src/sidecar.rsapps/gui/src/app.tsxapps/gui/src/components/ConfigDialog.tsxapps/gui/src/components/SessionBrowser.tsxapps/gui/src/lib/config-form.tsapps/gui/src/lib/tab-session.tsapps/gui/test/config-form.test.tsapps/gui/test/tab-session.test.tspackages/cli/src/app.tsx
💤 Files with no reviewable changes (1)
- packages/cli/src/app.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/gui/src/components/SessionBrowser.tsx
- apps/gui/src/components/ConfigDialog.tsx
- apps/gui/src-tauri/src/sidecar.rs
…imeout) Ten of thirteen findings confirmed and fixed; two refuted with evidence, one already fixed in the codex round (see the PR disposition comment). - Unsent composer drafts survive tab switches: Chat still remounts per tab (scroll/effect isolation), but drafts live in an App-held per-tab map the component initializes from and writes back to. - A resumed or switched-to transcript now lands on the latest message: the first paint with content always jumps to the bottom (a fresh scroll container starts at the top, so near-bottom stickiness never engaged). - Permission hotkeys no longer leak into whatever had focus: handled keys preventDefault + stopPropagation, editable targets are exempt (typing wins if focus escaped the modal), and focus moves to the first option so Enter confirms it. All three overlays gain role="dialog"/aria-modal and initial focus; the config dialog closes on Escape when dismissible. - The kernel handshake is bounded (15s): a sidecar that spawns but never answers used to wedge the app in "starting" forever with start() a no-op; it now lands in "down" with the wedged process killed, and the manual restart button recovers. Regression-tested with a mute fake kernel. - notify() honors its never-throws contract (callers fire-and-forget), the rustc triple probe fails with the install hint instead of a stack trace when rustc is missing, and DESIGN.md's intro stops saying "GUI later".
Owner
Author
CodeRabbit review round — disposition13 findings across both reviews, each verified against the code before acting: 10 fixed in e40aefa, 1 already fixed, 2 refuted.
Gates: |
…focus return) All three findings confirmed. - Intentional kernel shutdown no longer masquerades as a crash: each spawn gets a monotonic generation, and the stdout thread's EOF path only reaps and emits minerva://exit when ITS OWN generation still sits in state. A killed kernel therefore emits nothing (no spurious auto-restart after a handshake timeout), and a lingering EOF thread can no longer steal a newly spawned child out of state during the shutdown grace. sidecar_kill became an async command running the grace wait on a blocking thread — callers await it, so the manager only shows the restart button once the old child is fully gone, and the 7s wait never blocks the main thread. - The stale-session predicate now matches errors the kernel actually throws: session/load wraps Session.load messages verbatim, which are "no persisted session …" (deleted log / other data dir) and "invalid session id: …" — not the "unknown session" family the previous predicate (and its fabricated test) assumed, which had silently disabled the self-heal. The predicate moved next to ensureTabSession and is now pinned by a real-kernel round-trip test so it can't drift from the wire again. - Dialogs return focus on close: all three overlays capture document.activeElement on open and restore it in effect cleanup, so resolving a permission (or closing config/sessions) puts keyboard users back in the composer instead of on <body>. The permission dialog hands focus along correctly across a stacked request queue.
Owner
Author
Codex round 2 — dispositionAll three findings confirmed and fixed in 4559cbb.
Gates: |
…ll races) Three confirmed concurrency races, all in the seams between async work and the state that outlives it. - Rust: start/kill are now serialized by an async lifecycle mutex held for the FULL operation, and shutdown joins the stdout forwarder after reaping. Previously the slot emptied before the ≤7s drain, so a webview-reload start could spawn a replacement while the old kernel was still draining — and the never-joined forwarder could emit the dying kernel's lines into the replacement's freshly subscribed listeners (Tauri events are app-global), risking response/id collisions on the new connection. The two locks have strictly separated roles (deadlock audit in the code): the reader thread never takes lifecycle, and shutdown never holds the slot lock while joining, so a crash racing a kill cannot deadlock. The exit hook block_ons the lifecycle lock — blocking quit until the drain finishes is the point. - TS: every async session install (lazy ensure or user switch) now begins a per-tab token (session-slots.ts, single monotonic counter that never resets) and commits only while still current. A superseded result — newer switch, closed tab, replaced client — is closed instead of installed, fixing both the invisible "already open" registration leak and completion-order-beats-user-order overwrites. Commits read the previous session from a synchronously updated authoritative ref, not the render closure, so back-to-back installs can't skip closing the loser.
Owner
Author
Codex round 3 — dispositionAll three concurrency findings confirmed and fixed in d3c6c25.
Gates: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/gui/test/stale-session.test.ts (1)
18-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd teardown for the temp dirs and kernel/client handles.
createKernel()returns a kernel withclose(), andMinervaClientexposesclose(). Clean both up after the test, and removecwd/dataDirwithrmSyncto avoid leaving temp artifacts behind.🤖 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 `@apps/gui/test/stale-session.test.ts` around lines 18 - 26, Update setup and the stale-session test teardown to retain the kernel, client, cwd, and dataDir handles. After the test completes, call the kernel and MinervaClient close() methods and remove both temporary directories with rmSync, ensuring cleanup runs even when assertions fail.
🤖 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.
Nitpick comments:
In `@apps/gui/test/stale-session.test.ts`:
- Around line 18-26: Update setup and the stale-session test teardown to retain
the kernel, client, cwd, and dataDir handles. After the test completes, call the
kernel and MinervaClient close() methods and remove both temporary directories
with rmSync, ensuring cleanup runs even when assertions fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bacc5e10-2ea0-4993-ac40-f685b9b5c696
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
apps/gui/package.jsonapps/gui/scripts/prepare-sidecar.tsapps/gui/src-tauri/src/sidecar.rsapps/gui/src/app.tsxapps/gui/src/components/ConfigDialog.tsxapps/gui/src/components/PermissionDialog.tsxapps/gui/src/components/SessionBrowser.tsxapps/gui/src/lib/kernel-manager.tsapps/gui/src/lib/native.tsapps/gui/src/lib/session-slots.tsapps/gui/src/lib/tab-session.tsapps/gui/test/kernel-manager.test.tsapps/gui/test/session-slots.test.tsapps/gui/test/stale-session.test.tsapps/gui/test/tab-session.test.tsdocs/DESIGN.md
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/gui/src/lib/native.ts
- apps/gui/src/lib/tab-session.ts
- docs/DESIGN.md
- apps/gui/scripts/prepare-sidecar.ts
- apps/gui/test/kernel-manager.test.ts
- apps/gui/package.json
- apps/gui/src/components/ConfigDialog.tsx
- apps/gui/src/components/SessionBrowser.tsx
- apps/gui/src/components/PermissionDialog.tsx
- apps/gui/src/lib/kernel-manager.ts
- apps/gui/src-tauri/src/sidecar.rs
- apps/gui/src/app.tsx
…n patches) Two codex findings — both follow-through gaps in the round-3 mechanisms — plus a CodeRabbit test-hygiene nitpick. - The dead-child reap branch in sidecar_start now applies the same discipline round 3 gave the kill path: take the dead Running out of the slot, release the slot lock (the reader's EOF path takes it — joining while holding it would deadlock), reap, JOIN the reader, then re-acquire and spawn. Previously the dead reader's JoinHandle was dropped and the successor spawned immediately, so a reader still draining buffered stdout could emit the dead kernel's frames into the successor's connection. The lifecycle lock (held for all of start) keeps the briefly empty slot private. - Profile updates now go through updateSession, the single write path for in-place session patches: it applies only while the SAME session is still installed in the tab and updates sessionsRef and React state in lockstep. The old handler wrote React state directly from a captured session object — the ref/state divergence meant any later commit or tab-close rebuilt from the stale ref and silently REVERTED the profile, and a response delayed past a session switch could reinstall the detached old store. - stale-session.test.ts closes its kernels and clients in afterEach (matching the acp/app test precedent). Temp dirs stay on the OS tmp cleaner like the rest of the suite.
Owner
Author
Review round 4 — disposition (codex ×2 + CodeRabbit nitpick)Fixed in f220cad.
Gates: |
Tag sidecar exits with process generations so stale webview events cannot tear down replacements, and prevent superseded startup attempts from attaching clients. Queue profile mutations per session in request order and cover both races with deterministic regression tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kernel/src/kernel.ts (1)
455-477: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject profile mutations after session replacement.
loadSettings()yields at Line 455, allowingsession/loadto replace this session in#sessions. This then updates and persists the detached instance while returning success, leaving the active loaded session with the old profile.Proposed fix
} else { const settings = await loadSettings(this.#runtime, this.#dataDir, session.cwd); try { resolved = resolveProfile(settings, profile); } catch (error) { throw new RpcError( JSON_RPC_ERROR_CODES.INVALID_PARAMS, error instanceof Error ? error.message : String(error), ); } } + if (this.#sessions.get(session.id) !== session) { + throw new RpcError( + JSON_RPC_ERROR_CODES.INVALID_REQUEST, + "session was reloaded while switching profile", + ); + } if (session.promptActive) {🤖 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 `@packages/kernel/src/kernel.ts` around lines 455 - 477, After the await to load settings in the profile-switch flow, verify that the current session is still the active instance registered in `#sessions` before resolving or mutating it. If session/load replaced it, reject the request with the appropriate invalid-request RpcError; otherwise preserve the existing resolveProfile, session.profile assignment, and session.append 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.
Inline comments:
In `@apps/gui/src/lib/sidecar-generation.ts`:
- Around line 29-35: Update the exit handler in the sidecar generation bridge so
every exit whose generation does not match active is stored in pending,
including when another generation is currently active. Remove the active ===
null restriction from the nonmatching-generation branch, preserving the existing
direct delivery for the active generation; activate() will discard stale
buffered exits.
---
Outside diff comments:
In `@packages/kernel/src/kernel.ts`:
- Around line 455-477: After the await to load settings in the profile-switch
flow, verify that the current session is still the active instance registered in
`#sessions` before resolving or mutating it. If session/load replaced it, reject
the request with the appropriate invalid-request RpcError; otherwise preserve
the existing resolveProfile, session.profile assignment, and session.append
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cb713b9-ab7c-4d08-9ff9-de74abdc5d6a
📒 Files selected for processing (10)
apps/gui/src-tauri/src/sidecar.rsapps/gui/src/app.tsxapps/gui/src/lib/kernel-manager.tsapps/gui/src/lib/sidecar-bridge.tsapps/gui/src/lib/sidecar-generation.tsapps/gui/test/kernel-manager.test.tsapps/gui/test/sidecar-generation.test.tsapps/gui/test/stale-session.test.tspackages/kernel/src/kernel.tspackages/kernel/test/profiles.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/gui/test/stale-session.test.ts
- apps/gui/src/lib/sidecar-bridge.ts
- apps/gui/src-tauri/src/sidecar.rs
- apps/gui/test/kernel-manager.test.ts
- apps/gui/src/lib/kernel-manager.ts
- apps/gui/src/app.tsx
Comment on lines
+29
to
+35
| exit(event: SidecarExitEvent) { | ||
| if (event.generation === active) { | ||
| active = null; | ||
| deliver(event.code); | ||
| } else if (active === null) { | ||
| pending.set(event.generation, event.code); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Retain replacement exits while another generation is active.
An early exit for generation 2 is discarded while generation 1 is active. If 2 is a replacement that dies before activate(2), the bridge attaches without receiving its exit. Buffer every nonmatching generation; activate() already discards stale entries.
Proposed fix
- } else if (active === null) {
+ } else {
pending.set(event.generation, event.code);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| exit(event: SidecarExitEvent) { | |
| if (event.generation === active) { | |
| active = null; | |
| deliver(event.code); | |
| } else if (active === null) { | |
| pending.set(event.generation, event.code); | |
| } | |
| exit(event: SidecarExitEvent) { | |
| if (event.generation === active) { | |
| active = null; | |
| deliver(event.code); | |
| } else { | |
| pending.set(event.generation, event.code); | |
| } |
🤖 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 `@apps/gui/src/lib/sidecar-generation.ts` around lines 29 - 35, Update the exit
handler in the sidecar generation bridge so every exit whose generation does not
match active is stored in pending, including when another generation is
currently active. Remove the active === null restriction from the
nonmatching-generation branch, preserving the existing direct delivery for the
active generation; activate() will discard stale buffered exits.
…round A 26-agent review of the whole branch (final state, not diffs) surfaced 10 verified findings; all accepted. Rust lock discipline — the review caught the file breaking its own rule: - sidecar_send no longer holds the slot mutex across the pipe write (a kernel that stops draining stdin filled the OS buffer and wedged kill and app exit behind the block). stdin is now Arc-shared: sends clone it under a brief slot lock and write on the blocking pool, with a fair writes mutex preserving frame order across concurrent invokes. - The reader's EOF path takes its Running under the slot lock but WAITS outside it — stdout can close while the process lives on, and the slot must never be hostage to that wait. - sidecar_kill is generation-targeted: a stale kill (handshake-timeout catch racing crash recovery) no-ops instead of reaping the freshly spawned replacement; the bridge passes the generation it owns. GUI behavior: - A kernel crash during session load no longer leaves a permanent, misleading "couldn't open" notice (the rejection microtask beat the client-change effect that would have invalidated its token; the handler now defers to the crash banner when the manager's client has moved on). - Double-clicking a session row no longer errors with "already open in this client": same-target switches dedup before the load even starts. - Chat is keyed by session (drafts stay per-tab via the drafts map), so a within-tab switch gets a fresh initial-scroll to the latest message. - Diff rendering is memoized and transcript items are React.memo'd — the store reuses untouched item objects, so streaming stops re-running the quadratic LCS for every historical diff on every token. - The dark palette is now reachable: the OS color scheme toggles the .dark class the theme was written against. Duplication with drift risk: - providerKeyStatuses (providers) is now the single home of the key-detection policy consumed by both the kernel's config/state and the TUI's /config rows; splitModelRef (client) is the single frontend home of the ref grammar, used by the GUI dialog and the TUI panel.
Owner
Author
Final consolidated review roundTwo things happened in this round: 1. Review of the externally contributed commit 82126b0 ("preserve lifecycle and profile ordering"): approved. Both fixes are real residual races — the webview-side exit-generation gate (a legitimate crash exit from an old kernel could reach a new webview's bridge and tear down its replacement; an own-generation exit racing the start invoke could be lost) and the kernel's per-session profile-mutation queue (rapid set_profile calls could apply out of arrival order across the settings-I/O await). Both are deterministically regression-tested; gates and live drills re-verified before this round built on top. 2. A 26-agent consolidated review of the branch's FINAL STATE (not diffs — the concurrency trio has been patched across four rounds by two agents, and layered diffs hide drift). 10 findings survived adversarial verification; all accepted and fixed in 7b77504:
Gates: 550 tests, knip, With five external review rounds plus this consolidated pass all dispositioned, the remaining pre-merge gate is the human smoke walk (CONTRIBUTING's GUI checklist). |
Keep sidecar children reachable until stdout failures are reaped so retries cannot leave orphaned kernels. Reuse in-flight session loads across rapid A-B-A selections while preserving the user's latest intent and isolating client generations. Add focused Rust and TypeScript regression coverage for both concurrency sequences.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
M2 as designed in docs/DESIGN.md: a Tauri 2 desktop app at
apps/guithat spawns the existing kernel as a stdio sidecar (minerva acp --allow-unconfigured) and reuses@minerva/clientend to end. Full TUI parity — streaming markdown, thoughts, tool calls with diffs, permission prompts (y/a/n hotkey parity), first-run config, session browser/resume, modes, profiles, compact, /skills — plus GUI-native extras: multi-project tabs over one kernel, a side-by-side diff viewer, and native notifications when a long turn finishes unfocused.Architecture notes
shell:*capability is granted to the webview; all restart policy lives in TS (kernel-manager.ts, one auto-respawn per death).session/newtakes cwd); only the active tab holds a live store, so crash recovery is "drop stores, keep tab state, re-ensure lazily" with a resume→create fallback for stale ids.minerva/config/state(read half ofconfig/set_model, for frontends across a pipe) and the acp-only--allow-unconfiguredflag; client now surfaces the kernel'smodesit previously dropped.packages/protocol/stdio.tsnow uses web-standardTextDecoderinstead ofnode:string_decoder— the module loads in the webview via the package index, where Vite externalizes node: builtins.prepare-sidecar.tsfeeds the releasedist/{minerva, rg}pair to Tauri as externalBin; Tauri strips the target-triple suffixes and co-locates both beside the app binary, soresolveRgPathworks unchanged (verified in the bundle). Localtauri buildonly; release-CI bundling/signing deferred. Dev uses a config overlay sotauri devnever needs the release pair.Verification
bun run verify+bun run knipgreen at every one of the 11 commits (528 tests at HEAD; new coverage: transport framing, kernel manager against a fake JSON-RPC bridge, tabs reducer + persistence, permission queue, diff alignment, notification matrix, keyless acp conformance over a real process boundary,minerva/config/statekernel tests).kill -9auto-respawns once and stays down on the second kill; SIGTERM of the app leaves no orphan (stdin-EOF safety net); first-run flow holds keyless at the config dialog with no session created; packaged .app launched via Finder spawns the bundled kernel and Cmd+Q kills it via the exit hook.Notes for review
com.hutusi.minerva; placeholder icons — happy to swap for real branding.Summary by CodeRabbit
minerva/config/stateflow enabling live provider/model state and keylessacp --allow-unconfigured.