Conversation
Capture the architecture decisions from the design interview before any code exists: protocol-everywhere process model (forced by Tauri's Rust backend), ACP core + minerva/* extensions, AI SDK v5 behind a kernel-owned provider interface, kernel-enforced permissions, JSONL event-sourced sessions, CLI-first with a shared client core, and Bun-first-Node-tolerant runtime policy. Future slices implement against this record.
Workspaces under packages/* and apps/* so the kernel, protocol, and frontends develop against each other without a build step (packages export TypeScript source directly; Bun runs it, tsc typechecks it). The verify gate (typecheck + biome lint + bun test) exists from the first commit so every slice boundary has the same green bar. Biome over eslint for zero-config speed on a greenfield repo.
The protocol is the load-bearing seam of the architecture: every frontend (CLI in-proc today, Tauri sidecar and remote WS later) speaks it, so the Connection is symmetric — either side can issue requests, because the kernel must call session/request_permission on the frontend mid-turn. Types follow ACP naming and payload shapes (pinned via PROTOCOL_VERSION) so slice 3's Zed conformance work is a mapping exercise, not a redesign. The in-proc transport defers delivery to a microtask so it behaves like a real wire and can't hide reentrancy bugs. Also picks up biome migrate/format fixes surfaced by the first lint run.
The kernel owns the model abstraction (TurnRequest in, TurnEvent stream out) so 'model-agnostic' is a property of the kernel, not of whichever SDK is fashionable. The AI SDK adapter is the sole importer of 'ai' — provider quirks (tool-call streaming, message shapes, usage accounting) stop at this boundary. Tools are passed without execute() on purpose: the kernel runs tools itself so permissions and audit logging can't be bypassed by SDK auto-execution. createAiSdkProvider is split from createAnthropicProvider so tests can inject MockLanguageModelV3; createScriptedProvider gives kernel and e2e tests a deterministic model with zero network.
…rovals The loop is deliberately provider-dumb: it consumes the kernel-owned TurnEvent stream, so any model that speaks ModelProvider drives the same permission, audit, and update machinery. Every side effect flows through one choke point (executeToolCall) that appends tool.call / permission.decision / tool.result events before anything touches disk — the JSONL log is the source of truth the future resume/replay and audit features build on. Slice-1 permission policy: read-only tools allowed by policy, everything else round-trips an ACP session/request_permission to the frontend, with deny-by-default when the frontend can't answer. The rule engine + modes replace that branch in slice 2 without moving the choke point. Tools go through a runtime-adapter seam (node:* implementations that run on both Bun and Node) per the Bun-first-Node-tolerant decision. bash uses pipes, not a PTY — documented v1 limitation.
…store The client package is the layer both frontends share (design decision #8): MinervaClient owns the connection and permission-request routing, SessionStore reduces protocol updates into an immutable, renderable view-model with zero UI imports. Ink consumes snapshots today; the Tauri webview consumes the identical store in M2, so update-handling never forks per frontend. Permission handling defaults to deny (cancelled outcome) when no handler is wired — a frontend must opt in to approving side effects. Integration tests drive a real kernel + scripted provider over the in-proc transport, asserting the store reaches the exact expected item sequence; the kernel is a devDependency only, keeping the runtime graph clean.
First real frontend: streams assistant text, renders tool calls with status and output preview, and answers kernel permission requests with a y/n prompt. The CLI embeds the kernel but still speaks only the protocol via the in-proc transport pair — the same messages a Tauri sidecar will see — so running the CLI exercises the wire contract on every keystroke. PermissionBridge decouples the client (constructed before React mounts) from the UI's lifetime; with no UI attached, requests deny by default. After Ink unmounts, stdin's raw-mode listener keeps Bun alive, so the entrypoint awaits waitUntilExit and exits explicitly. Verified under a real PTY (expect script): banner and input render on Bun, /exit terminates cleanly.
…on, audit) An xhigh multi-agent review of the slice confirmed 15 defects; this applies all of them. Four groups: Cancellation plumbing — the prompt's AbortSignal now reaches permission requests (Connection.request accepts a signal) and tool execution (ToolContext.signal → runtime.exec), a cancel arriving during the permission round-trip wins over a late approval, and cancelled turns record streamed text and synthesize results for every pending tool call so the session history never contains dangling tool_use blocks that would 400 all later prompts. The ACP cancelled outcome now cancels the turn per spec instead of masquerading as a user denial. exec hardening — children run in their own process group so kills reach grandchildren; 'exit' plus a pipe-grace window prevents backgrounded daemons from hanging the CLI forever; setEncoding keeps multi-byte UTF-8 intact across chunk boundaries; per-stream accumulation is capped. Input validation — edit_file rejects non-string new_string instead of silently deleting code, replacement uses a replacer function so $-patterns can't corrupt files, bash falls back to the default timeout for 0/negative/NaN, and read_file is confined to the workspace because policy auto-allows it with no permission prompt. Audit fidelity — failed turns append turn.failed, permission decisions record who actually decided (policy/user/frontend/error), and one failed JSONL write no longer poisons the log chain (it fails that prompt loudly once, then recovers). The client also rejects overlapping prompts before touching the shared store.
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughMinerva’s initial monorepo implementation adds JSON-RPC/ACP transports, provider adapters, a persistent kernel with tools and permissions, client session state, a Bun CLI with TUI and ACP modes, configuration, documentation, and integration/regression tests. ChangesRepository foundation
Protocol and provider foundation
Kernel and client runtime
CLI
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant MinervaClient
participant MinervaKernel
participant ModelProvider
participant PermissionPrompt
participant KernelTool
User->>CLI: enter prompt
CLI->>MinervaClient: prompt(sessionId, text)
MinervaClient->>MinervaKernel: session/prompt
MinervaKernel->>ModelProvider: streamTurn
ModelProvider-->>MinervaKernel: text and tool-call events
MinervaKernel->>PermissionPrompt: request permission
PermissionPrompt-->>MinervaKernel: allow or reject
MinervaKernel->>KernelTool: execute approved tool
KernelTool-->>MinervaKernel: tool output
MinervaKernel-->>MinervaClient: session updates
MinervaClient-->>CLI: update session view
CLI-->>User: render response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 review |
✅ Action performedReview finished.
|
…sessions/list Slice 2 needs four protocol surfaces: session/load for resume (kernel replays the transcript as session/update notifications before answering, per ACP), session/set_mode + a modes state on session/new for the permission modes, plan and current_mode_update session-update variants for todo lists and mode display, and minerva/sessions/list — the first namespaced extension method — because resume needs session discovery and ACP has no listing surface. All ACP-shaped except the explicitly namespaced minerva/* method, keeping the slice-3 conformance mapping clean.
Completes the built-in tool surface planned for slice 2. glob and grep are read-only and therefore policy-allowed without prompts, so like read_file they are confined to the workspace; both cap output (200 paths, 100 matches, 1MB/file) so a broad search can't flood the model context. grep is pure TypeScript over tinyglobby rather than shelling out — portable across Bun/Node and testable, at some speed cost that a bundled ripgrep can address later. write_file is permission-gated like edit_file. todo_write turns plans into ACP plan updates: the tool validates entries and hands them to a loop-provided hook (ToolContext.updateTodos) so the tool itself never touches session or connection state.
…eplay
The two halves of slice 2 ('permissions & persistence') land together
because they share the event schema.
Permissions: a kernel-enforced engine evaluates allow/deny/ask rules
(bash(git *), edit_file(src/*)) against a per-tool permission value,
layered under four session modes (plan/default/acceptEdits/auto). Deny
is absolute — it outranks read-only policy, allow rules, and modes — so
a project deny list can't be talked around. Rules merge from global
(<dataDir>/settings.json) and project (.minerva/settings.json) layers;
an allow_always answer becomes a persisted project rule, and every
decision is audited with the rule that made it. A corrupt settings file
fails loudly rather than silently granting or dropping permissions.
Persistence: session/load replays the JSONL event log to rebuild both
the provider messages and the frontend transcript. assistant.message
events now carry their toolCalls so a turn is reconstructible from the
log alone; tool calls a crash left unresolved get synthesized error
results so the resumed history is well-formed for the provider, and a
torn final line (kill -9 mid-write) is skipped instead of blocking
resume. A per-project index.jsonl backs minerva/sessions/list.
…rendering The shared client grows loadSession (store registered before the request so replayed updates land in it), listSessions, and setMode; the store renders plan updates as a single in-place todo item and tracks the current mode. The CLI wires them up: --continue/-c resumes the latest session for the directory, --resume <id> a specific one, /mode switches modes (shown next to the prompt when not default), the permission prompt gains 'a' for allow-always, and todo lists render as a checklist. Integration tests cover the v0.1 exit criterion directly: a session killed mid-tool-call (dangling tool call + torn log line) resumes with a well-formed history and keeps working.
✅ Action performedReview finished.
|
…ume integrity) The slice-boundary review (finder phase complete, verification partially run before the workflow was stopped) surfaced defects in three areas; all confirmed by inspection and now regression-tested. Permission engine ordering — plan mode now outranks allow rules, so 'mutating tools are blocked' holds even for calls covered by an earlier allow_always; ask rules now outrank the read-only fast path so a project can force confirmation on reads. allow_always escapes wildcard characters before persisting (approving 'git add *' no longer grants 'git add --force anything'), with backslash-escape support added to the rule matcher. Workspace escapes — glob/grep patterns are validated (no absolute paths, no '..' segments) since the glob engine never routes them through path resolution; write_file and edit_file are confined to the workspace because acceptEdits auto-allows edit-kind tools, making path confinement the backstop. Resume integrity — session/load validates the log's recorded cwd (slug collisions could resume a foreign project's session), flushes a live session's pending writes before rebuilding, and no longer replays the log twice; set_mode settles its write before acknowledging; the index is appended on resume and deduped on list, so --continue means most recently used, not created; loadSession refuses to clobber a live store registration. Plus CLI argv validation, surrogate-safe previews, and awaited rejects assertions in tests.
Newline-delimited JSON-RPC over any Readable/Writable pair, per the ACP
transport spec ('messages are delimited by newlines and MUST NOT contain
embedded newlines' — JSON.stringify guarantees the latter by escaping).
This is the second of the three planned transports: in-proc (CLI) landed
in slice 1, stdio serves 'minerva acp' and the future Tauri sidecar.
Partial chunks buffer until their newline arrives, batched messages in
one chunk all dispatch, and a malformed line is skipped rather than
killing the connection — there is no id to answer it with.
The third seam of design decision #2 becomes real: the same kernel that the Ink UI drives in-proc is now spawnable by an editor over stdio with ACP framing. stdout belongs to the protocol, so the host path renders no UI and keeps diagnostics on stderr; MINERVA_DATA_DIR overrides the session root so hosts and tests can isolate state. The conformance harness spawns a real child process and drives it through MinervaClient over the stream transport: initialize, session/new, a full prompt with a tool call and permission round-trip — every byte crossing a genuine process boundary. A second test exercises the actual "minerva acp" entrypoint. Live Zed validation still needs a human with Zed installed; the harness covers the wire contract.
Second provider, completing the v0.1 exit criterion of two switchable providers. Model selection is now a reference — "openai/gpt-5.2", "anthropic/claude-opus-4-8" — with bare model ids defaulting to Anthropic so existing invocations keep working. The OpenAI adapter goes through the same createAiSdkProvider wrapper, so the kernel-owned ModelProvider boundary is untouched: adding the provider changed zero kernel code. The CLI resolves the ref before starting and requires the matching key env var (ANTHROPIC_API_KEY / OPENAI_API_KEY) instead of hardcoding Anthropic. @ai-sdk/openai is pinned to the v3 line — its v4 targets a newer language-model spec than ai@6 accepts.
… engine Completes the "any model, any tool" half of the design: MCP servers declared in settings (mcpServers, project overriding global by name) are connected per session over stdio, and their tools join the registry as mcp__<server>__<tool> so permission rules can target them. Two deliberate policy choices: server-provided readOnly hints are not trusted for permission bypass — MCP tools always go through the engine (deny/ask/allow rules and modes apply, ask-by-default in default mode) — and a server that fails to start degrades to a stderr warning instead of failing session creation, because a broken config must not brick the project. Every MCP call lands in the audit log under its namespaced tool name like any built-in. Integration test spawns a real MCP server (SDK fixture) from project settings and drives a full prompt through permission approval to the tool result.
minerva/session/compact runs one summarization turn through the same provider, appends a session.compacted event, and resets the model context to the summary. The event log keeps the full history — compaction changes what the model sees, not the record — so replay rebuilds the compacted context on resume while the UI transcript still shows everything the user saw. Guarded against running mid-prompt or on an empty session, and cancellable like any prompt. The CLI grows a proper command palette: /help, /mode, /compact, /sessions (recent sessions with previews), /new (fresh session), /exit.
README now documents what actually shipped: flags and slash commands, provider refs and their key env vars, the settings schema (permission rules with precedence, defaultMode, mcpServers), session logs as the audit trail, and Zed agent-server setup for the acp host. build:release compiles the single-file executable per the runtime decision (#9). Honestly documented caveat: on this macOS arm64 setup Bun 1.3.12 emits an unsigned binary that the kernel SIGKILLs, and codesign rejects the format for re-signing — recorded in the design watchlist to revisit on a newer Bun; the CLI runs via bun in the meantime. DESIGN.md slice checklist updated: all four v0.1 slices built; exit criteria marked with what remains human-gated (live-model smoke, live Zed interop).
…guards)
Manual review of the slice 3+4 diff surfaced four defects:
- MCP server env: passing config.env to StdioClientTransport REPLACES
the environment (the SDK only applies its safe defaults when env is
unset), so a config that set one variable stripped PATH/HOME and broke
the server's spawn. Now merged over getDefaultEnvironment().
- MCP connection lifecycle: re-loading a session overwrote its map entry
without closing the old connection, leaking server processes for the
kernel's lifetime. connectMcp now closes any existing connection first.
- client.compact() was missing the overlap guard prompt() has: called
during an active prompt, its finally would clear the busy flag
mid-stream and split the streaming assistant message. Same guard, with
a regression test.
- parseModelRef("openai") silently became an Anthropic model named
"openai" and failed confusingly at request time; a bare provider name
now errors with the provider/<model-id> hint.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
packages/kernel/src/runtime.ts (1)
106-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a truncation marker when stream output is capped.
Once
stdoutorstderrreachesMAX_STREAM_CHARS, subsequent chunks are silently dropped. The consumer receives truncated output with no indication that data was lost, which could confuse the agent or user into thinking the command produced incomplete output intentionally.♻️ Optional: append a truncation marker
let stdout = ""; let stderr = ""; + let stdoutCapped = false; + let stderrCapped = false; let timedOut = false; let aborted = false; let exitCode: number | null = null; let settled = false; let graceTimer: ReturnType<typeof setTimeout> | undefined;child.stdout.on("data", (chunk: string) => { - if (stdout.length < MAX_STREAM_CHARS) stdout += chunk; + if (stdout.length < MAX_STREAM_CHARS) { + stdout += chunk; + if (stdout.length >= MAX_STREAM_CHARS && !stdoutCapped) { + stdout += "\n[output truncated]"; + stdoutCapped = true; + } + } }); child.stderr.on("data", (chunk: string) => { - if (stderr.length < MAX_STREAM_CHARS) stderr += chunk; + if (stderr.length < MAX_STREAM_CHARS) { + stderr += chunk; + if (stderr.length >= MAX_STREAM_CHARS && !stderrCapped) { + stderr += "\n[output truncated]"; + stderrCapped = true; + } + } });🤖 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/runtime.ts` around lines 106 - 111, Add truncation markers in the stdout and stderr handlers within the child process stream setup. When either buffer first exceeds MAX_STREAM_CHARS, retain the limit, append a clear marker indicating output was truncated, and ignore subsequent chunks without repeatedly appending the marker.packages/kernel/src/settings.ts (1)
64-75: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueBe aware of TOCTOU in
persistAllowRuleunder concurrent access.The read-modify-write pattern in
persistAllowRulecould lose an allow rule if two calls race: both read the same file, both append different rules, and the second write overwrites the first. This is low-risk in practice since permission persistence is typically user-initiated and sequential, but worth noting if the kernel ever persists rules from concurrent tool approvals.🤖 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/settings.ts` around lines 64 - 75, Address the TOCTOU race in persistAllowRule by serializing concurrent read-modify-write operations for the same settings path, using an appropriate per-path lock or mutex around reading, checking, and writing the settings. Preserve the existing duplicate-rule behavior and ensure the lock is released even when filesystem operations fail.packages/kernel/src/replay.ts (1)
152-159: 📐 Maintainability & Code Quality | 🔵 Trivial
titleForis duplicated inagent-loop.tswith a different signature.The same helper exists in
packages/kernel/src/agent-loop.ts(lines 373-380) but takes(tool, call: ProviderToolCall)instead of(tool, toolName, input). Consider extracting a shared utility into a common module (e.g.,tools/utils.ts) to keep the fallback logic in one place.🤖 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/replay.ts` around lines 152 - 159, Extract the duplicated title fallback logic from replay.ts’s titleFor and agent-loop.ts’s titleFor into a shared utility in tools/utils.ts, adapting the utility’s inputs or adding a shared call shape as needed. Update both callers to use the common helper while preserving their existing behavior and fallback values.packages/kernel/src/permissions.ts (1)
131-146: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider guarding against ReDoS in
wildcardToRegex.Each
*in a rule pattern expands to[\s\S]*. A rule with multiple*segments (e.g.,bash(git * * * *)) creates a regex with multiple greedy quantifiers that can exhibit polynomial or exponential backtracking against long non-matching inputs. While rule patterns are user-defined, tool input values matched against them can be influenced by prompt injection. Adding a length cap on the compiled pattern or input value, or using a bounded matcher, would eliminate the risk.🤖 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/permissions.ts` around lines 131 - 146, The wildcardToRegex implementation is vulnerable to excessive backtracking from multiple unbounded wildcard quantifiers. Replace the regex-based matching with a bounded wildcard matcher, or enforce strict length limits on both compiled patterns and matched input before constructing or executing the regex; update the caller(s) to reject or safely handle oversized values while preserving escaped characters, * and ? semantics.Source: Linters/SAST tools
packages/cli/src/app.tsx (1)
295-318: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePermission prompt doesn't guard against repeated resolution.
If the user presses a key (e.g.,
y) and then another key before React re-renders and unmountsPermissionPrompt,pending.resolvewould be called twice. While the underlying Promise ignores the second resolution,setPending(null)would also fire twice — harmless but wasteful. More importantly, theuseInputhook remains active until unmount, so rapid key presses could trigger multiple resolve attempts.Consider adding a resolved guard:
🛡️ Optional guard
function PermissionPrompt({ pending }: { pending: PendingPermission }) { + const [resolved, setResolved] = useState(false); useInput((input, key) => { + if (resolved) return; const answer = input.toLowerCase(); if (answer === "y") { + setResolved(true); pending.resolve({ outcome: { outcome: "selected", optionId: "allow" } }); } else if (answer === "a") { + setResolved(true); pending.resolve({ outcome: { outcome: "selected", optionId: "allow_always" } }); } else if (answer === "n") { + setResolved(true); pending.resolve({ outcome: { outcome: "selected", optionId: "reject" } }); } else if (key.escape) { + setResolved(true); pending.resolve({ outcome: { outcome: "cancelled" } }); } });🤖 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/cli/src/app.tsx` around lines 295 - 318, PermissionPrompt can resolve the same pending permission multiple times during rapid input. Add a closure- or ref-based resolved guard inside PermissionPrompt’s useInput handler, return early once resolved, and set the guard before calling pending.resolve for y, a, n, or Escape.packages/client/src/client.ts (1)
120-132: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding
compactagainst overlapping prompts.
compactsetsstore?.setBusy(true)and laterstore?.setBusy(false)infinallywithout checking#activePrompts. If a prompt is in-flight, thefinallywould set busy to false prematurely, clobbering the prompt's view state. The CLI UI prevents this by hiding input while busy, but a direct client caller (e.g., another UI or test) could trigger this race.🛡️ Optional guard
async compact(sessionId: string): Promise<string> { + if (this.#activePrompts.has(sessionId)) { + throw new Error("a prompt is already running in this session"); + } const store = this.#stores.get(sessionId); store?.setBusy(true);🤖 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/client/src/client.ts` around lines 120 - 132, Guard Client.compact against overlapping prompts by checking the existing `#activePrompts` state before starting compaction and rejecting or otherwise preventing the request when a prompt is in flight. Preserve the prompt’s busy state and ensure compact’s cleanup cannot set the session store idle while an active prompt remains; update the relevant active-prompt tracking around compact as needed.packages/kernel/src/tools/grep.ts (1)
74-78: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTruncate lines before regex testing to mitigate ReDoS and ensure output consistency.
regex.test(line)runs against the full line (up to ~1M chars), but the output is sliced toMAX_LINE_CHARS(500). A match beyond position 500 would be invisible in the output yet still reported, and a catastrophic-backtracking pattern on a long line could hang the agent loop. Testing the truncated line aligns match detection with displayed output and bounds regex input size.♻️ Proposed refactor
for (let index = 0; index < lines.length && matches.length < MAX_MATCHES; index++) { const line = lines[index] ?? ""; - if (regex.test(line)) { - matches.push(`${file}:${index + 1}: ${line.slice(0, MAX_LINE_CHARS).trimEnd()}`); + const truncated = line.slice(0, MAX_LINE_CHARS); + if (regex.test(truncated)) { + matches.push(`${file}:${index + 1}: ${truncated.trimEnd()}`); } }🤖 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/tools/grep.ts` around lines 74 - 78, In the loop within the grep tool’s line-matching logic, truncate each line to MAX_LINE_CHARS before calling regex.test, then use that same truncated value for the displayed match output. This keeps detection aligned with output and bounds regex input size; update the relevant line handling around the matches.push call without changing match limits or line numbering.
🤖 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 `@packages/cli/src/app.tsx`:
- Around line 109-111: Prevent the Chat-level escape handler from calling
client.cancel while PermissionPrompt is active. Update the useInput handler in
app.tsx to detect the active permission prompt and only cancel the session when
no permission request is being displayed.
In `@packages/kernel/src/agent-loop.ts`:
- Around line 138-140: Before throwing streamError in the agent loop, mirror the
existing cancellation cleanup: call recordAssistantMessage() to persist partial
streamed text and tool calls, then call cancelToolBatch() to create matching
error results for any pending tool calls. Preserve the existing error conversion
and throw behavior so runPrompt can still append the actual turn.failed error.
In `@packages/kernel/src/kernel.ts`:
- Around line 114-122: Close any existing MCP connection for the session before
replacing it in `#connectMcp`: retrieve the current value from this.#mcp and await
its close() method when present, then establish and store the new connection.
Ensure this cleanup occurs before connectMcpServers and also handles sessions
with no configured servers as appropriate.
- Around line 219-221: Fix the session ordering logic in the sessions-list
implementation: replace the forward population of bySessionId followed by
reverse() with a reverse traversal of entries, adding only the first unseen
sessionId and stopping after 20 unique sessions. Preserve each selected entry as
its latest summary so the resulting list is ordered by most-recently-used
session.
In `@packages/kernel/src/mcp.ts`:
- Around line 30-36: Update wrapMcpTool.execute() to forward the received
ToolContext.signal into client.callTool(...), preserving the abort signal
through MCP tool execution so cancelled prompts stop waiting on the server.
In `@packages/kernel/src/permissions.ts`:
- Around line 108-110: Update escapeRuleValue to also prefix backslashes before
literal '(' and ')' characters, alongside the existing '\\', '*' and '?'
escaping, so formatRule-generated values remain valid and wildcardToRegex treats
these delimiters as literals.
In `@packages/kernel/src/replay.ts`:
- Around line 35-54: Update the synthesized failed update in flushToolBatch to
include a content field containing the same interruption message as the
generated result output, matching the content shape used by real tool.result
updates.
In `@packages/kernel/src/settings.ts`:
- Around line 84-87: Strengthen validation in readSettingsFile before casting
parsed JSON to MinervaSettings: verify the expected settings structure,
especially that permissions.allow, permissions.deny, and permissions.ask are
arrays of valid permission rules, and reject malformed values by returning {}.
Ensure loadSettings only receives structurally valid settings, preventing
spreads or string values from altering policy.
In `@packages/protocol/src/stdio.ts`:
- Around line 24-25: Replace per-chunk Buffer.toString("utf8") decoding in the
onData stream handler with a persistent StringDecoder configured for UTF-8, and
append decoder.write(chunk) to buffer so incomplete multi-byte sequences carry
across chunks; flush the decoder at stream completion if applicable. Add a
stdio.test.ts case that splits a JSON-RPC message containing multi-byte text
across chunks and verifies the decoded message remains intact.
In `@packages/providers/src/ai-sdk.ts`:
- Around line 41-53: Handle rejections from streamText/fullStream in streamTurn
by wrapping the stream setup and iteration in try/catch. When an error occurs,
yield the documented terminal event with type "error" and the caught error, then
return; preserve normal event conversion and yielding through toTurnEvent.
---
Nitpick comments:
In `@packages/cli/src/app.tsx`:
- Around line 295-318: PermissionPrompt can resolve the same pending permission
multiple times during rapid input. Add a closure- or ref-based resolved guard
inside PermissionPrompt’s useInput handler, return early once resolved, and set
the guard before calling pending.resolve for y, a, n, or Escape.
In `@packages/client/src/client.ts`:
- Around line 120-132: Guard Client.compact against overlapping prompts by
checking the existing `#activePrompts` state before starting compaction and
rejecting or otherwise preventing the request when a prompt is in flight.
Preserve the prompt’s busy state and ensure compact’s cleanup cannot set the
session store idle while an active prompt remains; update the relevant
active-prompt tracking around compact as needed.
In `@packages/kernel/src/permissions.ts`:
- Around line 131-146: The wildcardToRegex implementation is vulnerable to
excessive backtracking from multiple unbounded wildcard quantifiers. Replace the
regex-based matching with a bounded wildcard matcher, or enforce strict length
limits on both compiled patterns and matched input before constructing or
executing the regex; update the caller(s) to reject or safely handle oversized
values while preserving escaped characters, * and ? semantics.
In `@packages/kernel/src/replay.ts`:
- Around line 152-159: Extract the duplicated title fallback logic from
replay.ts’s titleFor and agent-loop.ts’s titleFor into a shared utility in
tools/utils.ts, adapting the utility’s inputs or adding a shared call shape as
needed. Update both callers to use the common helper while preserving their
existing behavior and fallback values.
In `@packages/kernel/src/runtime.ts`:
- Around line 106-111: Add truncation markers in the stdout and stderr handlers
within the child process stream setup. When either buffer first exceeds
MAX_STREAM_CHARS, retain the limit, append a clear marker indicating output was
truncated, and ignore subsequent chunks without repeatedly appending the marker.
In `@packages/kernel/src/settings.ts`:
- Around line 64-75: Address the TOCTOU race in persistAllowRule by serializing
concurrent read-modify-write operations for the same settings path, using an
appropriate per-path lock or mutex around reading, checking, and writing the
settings. Preserve the existing duplicate-rule behavior and ensure the lock is
released even when filesystem operations fail.
In `@packages/kernel/src/tools/grep.ts`:
- Around line 74-78: In the loop within the grep tool’s line-matching logic,
truncate each line to MAX_LINE_CHARS before calling regex.test, then use that
same truncated value for the displayed match output. This keeps detection
aligned with output and bounds regex input size; update the relevant line
handling around the matches.push call without changing match limits or line
numbering.
🪄 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: a5d3fb13-d0a0-4385-b4f3-c7fa4189a34f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (65)
.gitignoreREADME.mdbiome.jsondocs/DESIGN.mdpackage.jsonpackages/cli/package.jsonpackages/cli/src/acp.tspackages/cli/src/app.tsxpackages/cli/src/index.tsxpackages/cli/src/permission-bridge.tspackages/cli/test/acp.test.tspackages/cli/test/fixtures/acp-scripted.tspackages/client/package.jsonpackages/client/src/client.tspackages/client/src/index.tspackages/client/src/store.tspackages/client/test/client.test.tspackages/client/test/compact.test.tspackages/client/test/resume.test.tspackages/client/test/store.test.tspackages/kernel/package.jsonpackages/kernel/src/agent-loop.tspackages/kernel/src/compact.tspackages/kernel/src/events.tspackages/kernel/src/index.tspackages/kernel/src/kernel.tspackages/kernel/src/mcp.tspackages/kernel/src/permissions.tspackages/kernel/src/replay.tspackages/kernel/src/runtime.tspackages/kernel/src/session.tspackages/kernel/src/settings.tspackages/kernel/src/tools/bash.tspackages/kernel/src/tools/edit-file.tspackages/kernel/src/tools/glob.tspackages/kernel/src/tools/grep.tspackages/kernel/src/tools/index.tspackages/kernel/src/tools/read-file.tspackages/kernel/src/tools/todo.tspackages/kernel/src/tools/types.tspackages/kernel/src/tools/write-file.tspackages/kernel/test/fixtures/mcp-server.tspackages/kernel/test/kernel.test.tspackages/kernel/test/mcp.test.tspackages/kernel/test/permissions.test.tspackages/kernel/test/regressions.test.tspackages/kernel/test/tools-slice2.test.tspackages/kernel/test/tools.test.tspackages/protocol/package.jsonpackages/protocol/src/index.tspackages/protocol/src/jsonrpc.tspackages/protocol/src/stdio.tspackages/protocol/src/transport.tspackages/protocol/src/types.tspackages/protocol/test/connection.test.tspackages/protocol/test/stdio.test.tspackages/providers/package.jsonpackages/providers/src/ai-sdk.tspackages/providers/src/index.tspackages/providers/src/registry.tspackages/providers/src/scripted.tspackages/providers/src/types.tspackages/providers/test/ai-sdk.test.tspackages/providers/test/registry.test.tstsconfig.json
| useInput((_input, key) => { | ||
| if (key.escape && viewModel.busy) client.cancel(session.id); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Escape during permission prompt fires both cancel and permission cancellation.
When the PermissionPrompt is visible, viewModel.busy is true, so the Chat's useInput escape handler fires client.cancel(session.id) — sending a session/cancel notification to the kernel — at the same time PermissionPrompt's escape handler resolves the permission as cancelled. The kernel is blocked awaiting the permission response; receiving a cancel notification in that state could cause unexpected behavior depending on processing order.
🛡️ Proposed fix: skip cancel when permission prompt is active
useInput((_input, key) => {
- if (key.escape && viewModel.busy) client.cancel(session.id);
+ if (key.escape && viewModel.busy && !pending) client.cancel(session.id);
});📝 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.
| useInput((_input, key) => { | |
| if (key.escape && viewModel.busy) client.cancel(session.id); | |
| }); | |
| useInput((_input, key) => { | |
| if (key.escape && viewModel.busy && !pending) client.cancel(session.id); | |
| }); |
🤖 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/cli/src/app.tsx` around lines 109 - 111, Prevent the Chat-level
escape handler from calling client.cancel while PermissionPrompt is active.
Update the useInput handler in app.tsx to detect the active permission prompt
and only cancel the session when no permission request is being displayed.
| if (streamError !== undefined) { | ||
| throw streamError instanceof Error ? streamError : new Error(String(streamError)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stream errors discard partial output already shown to the UI.
When the provider stream emits an error event, streamError is stored and thrown at line 139 without calling recordAssistantMessage() or cancelToolBatch(). This discards any partial text and tool calls that were already streamed to the frontend. The design comment at lines 80-82 states "Everything streamed to the UI must also be recorded, even when the turn is cancelled mid-stream," but this guarantee is only honored for cancellation (lines 133-137), not for stream errors. On replay after a crash, the user would see a user.message followed by turn.failed with no assistant output, even though text was displayed.
Additionally, if the model emitted tool calls before the error, the assistant message in session.messages would have tool calls without matching tool results, producing an invalid provider message history for the next turn.
🔧 Proposed fix: record partial output before throwing stream error
if (streamError !== undefined) {
+ recordAssistantMessage();
+ cancelToolBatch();
throw streamError instanceof Error ? streamError : new Error(String(streamError));
}This mirrors the cancellation handling at lines 133-137. recordAssistantMessage() preserves the partial assistant text/tool calls in the event log and session.messages, while cancelToolBatch() synthesizes error tool results so the provider history stays well-formed. The turn.failed event appended by runPrompt still records the actual error.
📝 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.
| if (streamError !== undefined) { | |
| throw streamError instanceof Error ? streamError : new Error(String(streamError)); | |
| } | |
| if (streamError !== undefined) { | |
| recordAssistantMessage(); | |
| cancelToolBatch(); | |
| throw streamError instanceof Error ? streamError : new Error(String(streamError)); | |
| } |
🤖 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/agent-loop.ts` around lines 138 - 140, Before throwing
streamError in the agent loop, mirror the existing cancellation cleanup: call
recordAssistantMessage() to persist partial streamed text and tool calls, then
call cancelToolBatch() to create matching error results for any pending tool
calls. Preserve the existing error conversion and throw behavior so runPrompt
can still append the actual turn.failed error.
| const bySessionId = new Map<string, SessionSummary>(); | ||
| for (const entry of entries) bySessionId.set(entry.sessionId, entry); | ||
| const recent = [...bySessionId.values()].reverse().slice(0, 20); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Sessions list returns wrong order — Map iteration order ≠ most-recently-used order.
Map preserves the insertion order of a key's first occurrence; calling set on an existing key updates the value but does not move the key. So [...bySessionId.values()] yields sessions in first-use order (each with their latest data), and .reverse() flips first-use order — not last-use order.
Concrete example: index entries [A@t1, B@t2, A@t3] → Map iterates [A(t3), B(t2)] → reversed [B(t2), A(t3)]. But A was used at t3 (more recent than B's t2), so the expected order is [A(t3), B(t2)].
The fix: iterate entries in reverse file order and take the first occurrence of each session ID — that gives true most-recently-used order.
🐛 Proposed fix: iterate in reverse, take first occurrence per session
const bySessionId = new Map<string, SessionSummary>();
- for (const entry of entries) bySessionId.set(entry.sessionId, entry);
- const recent = [...bySessionId.values()].reverse().slice(0, 20);
+ for (const entry of [...entries].reverse()) {
+ if (!bySessionId.has(entry.sessionId)) {
+ bySessionId.set(entry.sessionId, entry);
+ }
+ }
+ const recent = [...bySessionId.values()].slice(0, 20);📝 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.
| const bySessionId = new Map<string, SessionSummary>(); | |
| for (const entry of entries) bySessionId.set(entry.sessionId, entry); | |
| const recent = [...bySessionId.values()].reverse().slice(0, 20); | |
| const bySessionId = new Map<string, SessionSummary>(); | |
| for (const entry of [...entries].reverse()) { | |
| if (!bySessionId.has(entry.sessionId)) { | |
| bySessionId.set(entry.sessionId, entry); | |
| } | |
| } | |
| const recent = [...bySessionId.values()].slice(0, 20); |
🤖 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 219 - 221, Fix the session
ordering logic in the sessions-list implementation: replace the forward
population of bySessionId followed by reverse() with a reverse traversal of
entries, adding only the first unseen sessionId and stopping after 20 unique
sessions. Preserve each selected entry as its latest summary so the resulting
list is ordered by most-recently-used session.
| await client.connect( | ||
| new StdioClientTransport({ | ||
| command: config.command, | ||
| args: config.args, | ||
| env: config.env, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify KernelTool execute signature and MCP SDK callTool/connect options.
# Check KernelTool interface for abort signal support
ast-grep outline packages/kernel/src/tools/types.ts --items all --type interface --match 'KernelTool'
# Search for execute method signature in tools/types.ts
rg -nC5 'execute' packages/kernel/src/tools/types.ts
# Check if MCP SDK callTool accepts RequestOptions with signal
rg -nC5 'callTool|RequestOptions|signal' node_modules/@modelcontextprotocol/sdk/dist/esm/client/index.d.ts 2>/dev/null || \
rg -nC5 'callTool|RequestOptions|signal' node_modules/@modelcontextprotocol/sdk/src/client/index.ts 2>/dev/null || \
echo "MCP SDK source not found - check manually"
# Check how agent-loop.ts calls tool execute
rg -nC5 '\.execute\(' packages/kernel/src/agent-loop.tsRepository: hutusi/minerva
Length of output: 1208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/kernel/src/mcp.ts =="
cat -n packages/kernel/src/mcp.ts | sed -n '1,180p'
echo
echo "== packages/kernel/src/tools/types.ts =="
cat -n packages/kernel/src/tools/types.ts | sed -n '1,140p'
echo
echo "== package.json / lockfile entries for `@modelcontextprotocol/sdk` =="
rg -n '"`@modelcontextprotocol/sdk`"|modelcontextprotocol' package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null
echo
echo "== any local SDK type definitions =="
fd -a 'index.d.ts' . | rg '`@modelcontextprotocol/sdk`|modelcontextprotocol' || true
fd -a 'client.*' . | rg '`@modelcontextprotocol/sdk`|modelcontextprotocol' || trueRepository: hutusi/minerva
Length of output: 7562
🌐 Web query:
@modelcontextprotocol/sdk Client.connect signal callTool signal RequestOptions
💡 Result:
In the Model Context Protocol (MCP) TypeScript SDK, the concepts of signal and RequestOptions are used to manage and configure requests, such as calling tools [1][2]. Client.connect The client.connect(transport, options?) method is used to establish a connection to an MCP server [3][4]. It initiates the initialization handshake and resolves once the protocol version, server capabilities, and instructions are negotiated [3]. While client.connect accepts RequestOptions [5][4], the signal property within RequestOptions is typically used to cancel in-flight requests rather than the connection process itself [2]. callTool The client.callTool(params, options?) method invokes a tool on a connected server [5][6]. It accepts an optional RequestOptions object as its second argument [5][7]. RequestOptions and signal The RequestOptions type is defined to configure the behavior of requests [2]. Key properties include: signal (AbortSignal): This can be used to cancel an in-flight request [1][2]. If the signal is aborted, the SDK raises an AbortError and notifies the remote end [1][2]. onprogress: A callback function that receives progress notifications from the server if supported [2][7]. timeout: A duration in milliseconds for the request [2]. resetTimeoutOnProgress: A boolean that, if true, resets the timeout timer whenever a progress notification is received [1][2][7]. Example usage for cancellation: const controller = new AbortController; const result = await client.callTool( { name: 'my-tool', arguments: {} }, { signal: controller.signal }); // To cancel: controller.abort('User cancelled'); [1]
Citations:
- 1: https://modelcontextprotocol.github.io/typescript-sdk/documents/protocol.html
- 2: https://github.com/modelcontextprotocol/typescript-sdk/blob/main/packages/core/src/shared/protocol.ts
- 3: https://ts.sdk.modelcontextprotocol.io/v2/clients/connect.html
- 4: https://ts.sdk.modelcontextprotocol.io/classes/client.Client.html
- 5: https://ts.sdk.modelcontextprotocol.io/v2/classes/_modelcontextprotocol_client.client_client.Client.html
- 6: https://ts.sdk.modelcontextprotocol.io/v2/api/@modelcontextprotocol/client/client/client.html
- 7: https://ts.sdk.modelcontextprotocol.io/v2/clients/calling.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== connectMcpServers usages =="
rg -nC3 'connectMcpServers\(' packages
echo
echo "== client.callTool occurrences =="
rg -nC3 'callTool\(' packages/kernel/src
echo
echo "== Client.connect type info from installed SDK docs if present =="
rg -nC4 'connect\(|callTool\(' node_modules/@modelcontextprotocol/sdk 2>/dev/null | head -n 80 || trueRepository: hutusi/minerva
Length of output: 1632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sessionCancel / signal plumbing =="
rg -nC4 'sessionCancel|AbortController|AbortSignal|signal' packages/kernel/src
echo
echo "== kernel initialization around connectMcpServers =="
cat -n packages/kernel/src/kernel.ts | sed -n '100,160p'
echo
echo "== MCP SDK docs or types in repo or cache mentioning connect request options =="
rg -nC3 'connect\(.*options|RequestOptions.*signal|callTool\(.*options' . 2>/dev/null | head -n 120 || trueRepository: hutusi/minerva
Length of output: 14103
Forward the prompt abort signal into MCP tool calls
packages/kernel/src/mcp.ts:85-89 already receives ToolContext.signal at the KernelTool level, but wrapMcpTool.execute() drops it before client.callTool(...). Pass context.signal through so a cancelled prompt can stop waiting on a hung MCP server.
🤖 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/mcp.ts` around lines 30 - 36, Update
wrapMcpTool.execute() to forward the received ToolContext.signal into
client.callTool(...), preserving the abort signal through MCP tool execution so
cancelled prompts stop waiting on the server.
| export function escapeRuleValue(value: string): string { | ||
| return value.replace(/[\\*?]/g, (char) => `\\${char}`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
escapeRuleValue should escape ( and ) to prevent malformed "always allow" rules.
The function escapes \, *, and ? but not the rule-format delimiters ( and ). If a command value contains an unbalanced ( (e.g., echo (test), formatRule produces bash(echo (test which doesn't end with ), so ruleMatches returns false — the persisted "always allow" rule silently never matches. Escaping ( and ) ensures the wildcardToRegex parser treats them as literals via the \ escape path.
🛡️ Proposed fix
export function escapeRuleValue(value: string): string {
- return value.replace(/[\\*?]/g, (char) => `\\${char}`);
+ return value.replace(/[\\*?()]/g, (char) => `\\${char}`);
}📝 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.
| export function escapeRuleValue(value: string): string { | |
| return value.replace(/[\\*?]/g, (char) => `\\${char}`); | |
| } | |
| export function escapeRuleValue(value: string): string { | |
| return value.replace(/[\\*?()]/g, (char) => `\\${char}`); | |
| } |
🤖 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/permissions.ts` around lines 108 - 110, Update
escapeRuleValue to also prefix backslashes before literal '(' and ')'
characters, alongside the existing '\\', '*' and '?' escaping, so
formatRule-generated values remain valid and wildcardToRegex treats these
delimiters as literals.
| const flushToolBatch = () => { | ||
| if (expected.length === 0) return; | ||
| for (const call of expected) { | ||
| if (results.some((result) => result.toolCallId === call.toolCallId)) continue; | ||
| results.push({ | ||
| toolCallId: call.toolCallId, | ||
| toolName: call.toolName, | ||
| output: "Tool call was interrupted before completing (session resumed).", | ||
| isError: true, | ||
| }); | ||
| updates.push({ | ||
| sessionUpdate: "tool_call_update", | ||
| toolCallId: call.toolCallId, | ||
| status: "failed", | ||
| }); | ||
| } | ||
| messages.push({ role: "tool", results }); | ||
| expected = []; | ||
| results = []; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Synthesized error updates in flushToolBatch omit content, unlike real tool.result updates.
The tool_call_update pushed for interrupted tool calls (line 45-49) sets status: "failed" but does not include a content field with the error message. Compare with the tool.result case (line 106-111) which always includes content. On replay, the UI would render interrupted calls as failed without any explanatory text, even though the synthesized result has the message "Tool call was interrupted before completing (session resumed)."
🛡️ Proposed fix: include content in synthesized update
updates.push({
sessionUpdate: "tool_call_update",
toolCallId: call.toolCallId,
status: "failed",
+ content: [{ type: "content", content: { type: "text", text: "Tool call was interrupted before completing (session resumed)." } }],
});📝 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.
| const flushToolBatch = () => { | |
| if (expected.length === 0) return; | |
| for (const call of expected) { | |
| if (results.some((result) => result.toolCallId === call.toolCallId)) continue; | |
| results.push({ | |
| toolCallId: call.toolCallId, | |
| toolName: call.toolName, | |
| output: "Tool call was interrupted before completing (session resumed).", | |
| isError: true, | |
| }); | |
| updates.push({ | |
| sessionUpdate: "tool_call_update", | |
| toolCallId: call.toolCallId, | |
| status: "failed", | |
| }); | |
| } | |
| messages.push({ role: "tool", results }); | |
| expected = []; | |
| results = []; | |
| }; | |
| const flushToolBatch = () => { | |
| if (expected.length === 0) return; | |
| for (const call of expected) { | |
| if (results.some((result) => result.toolCallId === call.toolCallId)) continue; | |
| results.push({ | |
| toolCallId: call.toolCallId, | |
| toolName: call.toolName, | |
| output: "Tool call was interrupted before completing (session resumed).", | |
| isError: true, | |
| }); | |
| updates.push({ | |
| sessionUpdate: "tool_call_update", | |
| toolCallId: call.toolCallId, | |
| status: "failed", | |
| content: [{ type: "content", content: { type: "text", text: "Tool call was interrupted before completing (session resumed)." } }], | |
| }); | |
| } | |
| messages.push({ role: "tool", results }); | |
| expected = []; | |
| results = []; | |
| }; |
🤖 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/replay.ts` around lines 35 - 54, Update the synthesized
failed update in flushToolBatch to include a content field containing the same
interruption message as the generated result output, matching the content shape
used by real tool.result updates.
| try { | ||
| const parsed = JSON.parse(raw) as unknown; | ||
| if (typeof parsed !== "object" || parsed === null) return {}; | ||
| return parsed as MinervaSettings; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if zod is a dependency of `@minerva/kernel`
cat packages/kernel/package.json | jq '.dependencies, .devDependencies'Repository: hutusi/minerva
Length of output: 323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first.
ast-grep outline packages/kernel/src/settings.ts --view expanded
# Locate the MinervaSettings and related types.
rg -n "type MinervaSettings|interface MinervaSettings|PermissionRules|McpServerConfig|loadSettings|readSettingsFile" packages/kernel/src -S
# Show the relevant slice of settings.ts with line numbers.
sed -n '1,220p' packages/kernel/src/settings.ts
# Inspect the type definitions if they live elsewhere.
fd -a "settings.ts" packages/kernel/srcRepository: hutusi/minerva
Length of output: 6130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the file around the reported lines.
nl -ba packages/kernel/src/settings.ts | sed -n '1,220p'
# Find all references to readSettingsFile and loadSettings.
rg -n "readSettingsFile|loadSettings|permissions\.allow|permissions\.deny|permissions\.ask|defaultMode|mcpServers" packages/kernel/src -SRepository: hutusi/minerva
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the settings implementation.
nl -ba packages/kernel/src/settings.ts | sed -n '1,220p'Repository: hutusi/minerva
Length of output: 192
Validate parsed settings before casting
readSettingsFile only checks for a non-null object, so malformed permissions.allow/deny/ask can still reach loadSettings and either throw on spread or turn strings into per-character permission rules. A structural validator here would prevent bad settings from changing the effective policy.
🤖 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/settings.ts` around lines 84 - 87, Strengthen validation
in readSettingsFile before casting parsed JSON to MinervaSettings: verify the
expected settings structure, especially that permissions.allow,
permissions.deny, and permissions.ask are arrays of valid permission rules, and
reject malformed values by returning {}. Ensure loadSettings only receives
structurally valid settings, preventing spreads or string values from altering
policy.
| const onData = (chunk: Buffer | string) => { | ||
| buffer += typeof chunk === "string" ? chunk : chunk.toString("utf8"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Multi-byte UTF-8 characters are corrupted at chunk boundaries.
Buffer.toString("utf8") emits U+FFFD replacement characters for incomplete trailing byte sequences. When a stream chunk splits a multi-byte character (e.g., CJK text, emoji, accented characters in file contents or tool I/O), the character is permanently corrupted — the next chunk's orphaned continuation bytes produce a different character or more replacement characters. This silently corrupts any JSON-RPC message containing non-ASCII text that spans more than one chunk.
Use StringDecoder, which buffers incomplete multi-byte sequences across chunks:
🔧 Proposed fix
import type { JsonRpcMessage, Transport } from "./jsonrpc";
+import { StringDecoder } from "node:string_decoder";
+
/**
* Stream-based transport with ACP stdio framing: one JSON-RPC message per
* line, delimited by `\n`, no embedded newlines (JSON.stringify guarantees
* that — newlines inside strings are escaped). Used for `minerva acp`
* (kernel on stdin/stdout) and, later, the Tauri sidecar.
*/
export function createStreamTransport(
input: NodeJS.ReadableStream,
output: NodeJS.WritableStream,
): Transport {
const messageHandlers: Array<(message: JsonRpcMessage) => void> = [];
const closeHandlers: Array<() => void> = [];
+ const decoder = new StringDecoder("utf8");
let buffer = "";
let closed = false;
const emitClose = () => {
if (closed) return;
closed = true;
for (const handler of closeHandlers) handler();
};
const onData = (chunk: Buffer | string) => {
- buffer += typeof chunk === "string" ? chunk : chunk.toString("utf8");
+ buffer += typeof chunk === "string" ? chunk : decoder.write(chunk);Consider adding a test in stdio.test.ts that writes a message containing multi-byte characters (e.g., "héllo wörld 🎉") split across two chunks to verify the fix.
📝 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.
| const onData = (chunk: Buffer | string) => { | |
| buffer += typeof chunk === "string" ? chunk : chunk.toString("utf8"); | |
| import type { JsonRpcMessage, Transport } from "./jsonrpc"; | |
| import { StringDecoder } from "node:string_decoder"; | |
| /** | |
| * Stream-based transport with ACP stdio framing: one JSON-RPC message per | |
| * line, delimited by `\n`, no embedded newlines (JSON.stringify guarantees | |
| * that — newlines inside strings are escaped). Used for `minerva acp` | |
| * (kernel on stdin/stdout) and, later, the Tauri sidecar. | |
| */ | |
| export function createStreamTransport( | |
| input: NodeJS.ReadableStream, | |
| output: NodeJS.WritableStream, | |
| ): Transport { | |
| const messageHandlers: Array<(message: JsonRpcMessage) => void> = []; | |
| const closeHandlers: Array<() => void> = []; | |
| const decoder = new StringDecoder("utf8"); | |
| let buffer = ""; | |
| let closed = false; | |
| const emitClose = () => { | |
| if (closed) return; | |
| closed = true; | |
| for (const handler of closeHandlers) handler(); | |
| }; | |
| const onData = (chunk: Buffer | string) => { | |
| buffer += typeof chunk === "string" ? chunk : decoder.write(chunk); |
🤖 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/protocol/src/stdio.ts` around lines 24 - 25, Replace per-chunk
Buffer.toString("utf8") decoding in the onData stream handler with a persistent
StringDecoder configured for UTF-8, and append decoder.write(chunk) to buffer so
incomplete multi-byte sequences carry across chunks; flush the decoder at stream
completion if applicable. Add a stdio.test.ts case that splits a JSON-RPC
message containing multi-byte text across chunks and verifies the decoded
message remains intact.
| async *streamTurn(request) { | ||
| const result = streamText({ | ||
| model, | ||
| system: request.system, | ||
| messages: request.messages.map(toModelMessage), | ||
| tools: toToolSet(request.tools), | ||
| abortSignal: request.abortSignal, | ||
| }); | ||
|
|
||
| for await (const part of result.fullStream) { | ||
| const event = toTurnEvent(part); | ||
| if (event) yield event; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ai-sdk.ts ==\n'
sed -n '1,140p' packages/providers/src/ai-sdk.ts
printf '\n== ModelProvider contract ==\n'
rg -n "interface ModelProvider|streamTurn" packages -g '!**/dist/**' -g '!**/build/**'
printf '\n== turn event conversion ==\n'
rg -n "function toTurnEvent|const toTurnEvent|type: \"error\"|type: \"finish\"" packages/providers/src -g '!**/dist/**' -g '!**/build/**'Repository: hutusi/minerva
Length of output: 5301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== packages/providers/src/types.ts ==\n'
sed -n '1,140p' packages/providers/src/types.ts
printf '\n== packages/kernel/src/agent-loop.ts ==\n'
sed -n '1,180p' packages/kernel/src/agent-loop.ts
printf '\n== packages/kernel/src/compact.ts ==\n'
sed -n '1,120p' packages/kernel/src/compact.ts
printf '\n== package/providers/test/ai-sdk.test.ts relevant ==\n'
sed -n '1,220p' packages/providers/test/ai-sdk.test.tsRepository: hutusi/minerva
Length of output: 14117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== packages/providers/src/types.ts ==\n'
sed -n '1,140p' packages/providers/src/types.ts
printf '\n== packages/kernel/src/agent-loop.ts ==\n'
sed -n '1,180p' packages/kernel/src/agent-loop.ts
printf '\n== packages/kernel/src/compact.ts ==\n'
sed -n '1,120p' packages/kernel/src/compact.ts
printf '\n== packages/providers/test/ai-sdk.test.ts relevant ==\n'
sed -n '1,220p' packages/providers/test/ai-sdk.test.tsRepository: hutusi/minerva
Length of output: 14118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== packages/providers/src/types.ts ==\n'
sed -n '1,140p' packages/providers/src/types.ts
printf '\n== packages/kernel/src/agent-loop.ts ==\n'
sed -n '1,180p' packages/kernel/src/agent-loop.ts
printf '\n== packages/kernel/src/compact.ts ==\n'
sed -n '1,120p' packages/kernel/src/compact.ts
printf '\n== packages/providers/test/ai-sdk.test.ts ==\n'
sed -n '1,220p' packages/providers/test/ai-sdk.test.tsRepository: hutusi/minerva
Length of output: 14109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "provider\.streamTurn\(" packages -g '!**/dist/**' -g '!**/build/**' -A8 -B4Repository: hutusi/minerva
Length of output: 4743
Catch fullStream rejections and emit an error event. for await can reject on network/auth/abort failures, so the provider can exit without the documented terminal error event; wrap the streamText/fullStream block and yield { type: "error", error } before returning.
🤖 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/providers/src/ai-sdk.ts` around lines 41 - 53, Handle rejections
from streamText/fullStream in streamTurn by wrapping the stream setup and
iteration in try/catch. When an error occurs, yield the documented terminal
event with type "error" and the caught error, then return; preserve normal event
conversion and yielding through toTurnEvent.
…rkflow First tagged release. The repo has never been tagged despite package.json sitting at 0.1.0; the CHANGELOG already treats [0.1.0] (PR #1) as the kernel milestone and stacks all later CLI/agent work under [Unreleased], so this cut promotes that backlog rather than folding it into 0.1.0. - Bump all five workspace packages 0.1.0 → 0.2.0. - Promote the [Unreleased] changelog to [0.2.0] — 2026-07-12; open a fresh empty [Unreleased]. - Add a --version/-v flag so the CLI can report its own version (read from the cli package.json, inlined by `bun build --compile`). - Add a tag-triggered release workflow: guards tag == package version, runs the verify + knip gate, and cuts a GitHub Release from the changelog section. - Record the "stays private, license deferred" decision in the DESIGN watchlist. Repo stays private; distribution is source-run (no binaries, no npm publish).
1 participant
Why
Minerva is a cross-platform, model-agnostic code agent: a headless kernel behind a wire protocol, with multiple frontends (CLI now, Tauri GUI later). The architecture is forced by one constraint — Tauri 2's backend is Rust, so a TypeScript kernel can never run in-process in the GUI — which makes the protocol the load-bearing seam from day one. Full design record and locked decisions: docs/DESIGN.md.
Per the agreed workflow, all of v0.1 lands on this one branch and merges as a single non-squashed PR; review happens at each internal slice boundary, and this PR stays a draft until the last slice is in. The commits are the story —
git logon this branch reads as the development narrative.Progress
@minerva/protocol(bidirectional JSON-RPC, in-proc transport, ACP-shaped types);@minerva/providers(kernel-ownedModelProvider, AI SDK v6 Anthropic adapter, scripted test provider);@minerva/kernel(agent loop, JSONL event-sourced sessions, read/edit/bash tools, ask-every-time approvals over ACP);@minerva/client(shared protocol client + view-model store);@minerva/cli(Ink REPL). Slice-boundary review: 15 confirmed findings fixed with regression tests (cancellation plumbing, exec hardening, input validation, audit fidelity).bash(git *)allow/deny/ask patterns) + session modes (plan/default/acceptEdits/auto), session resume via log replay (--continue/--resume, kill-9 safe), write_file/glob/grep/todo_write tools, always-allow persistence to project settings,minerva/sessions/listextension. Slice-boundary review findings fixed with regression tests (permission ordering, wildcard escaping, workspace confinement, resume integrity).minerva acphost command, conformance harness driving a real spawned process through initialize/session/prompt/permission flows, OpenAI provider viaprovider/modelrefs (openai/gpt-5.2) with per-provider key env vars. Live Zed interop still needs a human with Zed installed.mcpServerssettings; tools join the registry asmcp__server__tool, never auto-allowed, fully audited), manual/compact(summary replaces model context, event log keeps full history, replay-aware), slash-command palette (/help /mode /compact /sessions /new /exit),build:releasescript. Known issue: Bun 1.3.12 on macOS arm64 emits unsigned compiled binaries (SIGKILLed by the kernel) — documented in README + design watchlist.Verification
bun run verify— typecheck + biome + 84 tests (unit, golden event-log assertions, full-stack client→kernel integration over the in-proc transport with a scripted provider)/exitterminates cleanly on BunANTHROPIC_API_KEY+bun run --cwd packages/cli dev) pending — no credentials in the dev environmentSummary by CodeRabbit