[malibio]

Closes #1220.

Reconciles the PTY agent-session code to match ADR-034: AIChat Provider Modes (nodespace-docs PR #47). PTY sessions are no longer a standalone subsystem — they are provider mode 2d of the ai-chat node. The conversation node exists up front; the terminal is a view onto it; capture backfills it instead of minting a new node. ADR-032's PTY mechanics are unchanged.

What changed

Backend

• proto: add optional string node_id to LaunchSessionRequest, threaded through the daemon launch handler, the Tauri command binding, and the CLI launch command (CLI passes None — not node-bound).
• capture_service.rs: mint → backfill. At session end, capture merges capture:* metadata + status/last_active onto the existing ai-chat node via update_node_unchecked (additive deep-merge preserves provider/messages). When no node_id is present (e.g. CLI session), capture is skipped rather than minting a node.
  • Chose update_node_unchecked over OCC update_node: capture is a single, additive, fire-and-forget writer, and a spurious version conflict from a concurrent viewer edit must not silently drop the capture.

Frontend

• ai-chat-node-viewer.svelte now dispatches on properties.provider:
  • native/ollama/openai → existing message UI (ADR-028), unchanged.
  • pty → embedded terminal via the new ai-chat-pty-view.svelte (the terminal is the node's viewer).
  • unset (placeholder) → provider-mode picker (a fresh /ai-chat node has no provider yet).
• ai-chat-pty-view.svelte (new): inline agent picker + availability + Launch (passing nodeId), then hosts the xterm terminal. Re-attaches to a persisted session id on reopen.
• Deleted the standalone agent-sessions tab and its agent-sessions-panel / agent-launch-panel / sessions-panel cluster, plus all nav/tab wiring (navigation.ts, layout.ts, pane-content.svelte, navigation-sidebar.svelte, tab-persistence-service.ts). pty-terminal.svelte is kept and re-hosted by the pty view.

Acceptance criteria

• /ai-chat creates a placeholder ai-chat node (no provider chosen)
• User can select any of the four provider modes on the node before launch
• For provider: pty, the embedded terminal renders as the node's viewer (no separate tab/panel)
• Capture backfills the existing node on session end instead of minting
• The standalone agent-sessions tab and its panels are removed
• Multiple concurrent ai-chat sessions = multiple nodes
• Mode 2d capture limited to transcript/session-id/metadata (documented, not a bug)
• No regressions to PTY mechanics (daemon gRPC integration tests pass: launch/stream/write/resize/terminate)

Notes

• Rebased onto main after Migrate local DB from SurrealDB embedded to SQLite (libsql) #1222 (SurrealDB → libsql migration). Re-verified the full workspace build + tests against the new DB backend.
• Out of scope (per issue): context pre-assembly (deferred to a later design session).

Testing

• bun run test:all: frontend 3936 passed, agent-tools 17, mcp 10, skill 13, nodespace-core 1128 — all green.
• cargo test -p nodespace-daemon: 14 lib (incl. 7 capture) + 9 gRPC integration — all pass.
• bun run quality:fix: clean (eslint + tsc + svelte-check 0 problems; rustfmt + clippy clean across workspace).

🤖 Generated with Claude Code

[malibio]

Code Review — PR #1223 (Reconcile PTY sessions into unified AIChat provider-mode model)

Review Type: Initial Review
Reviewer: Principal Engineer AI Reviewer (Pragmatic Quality framework)
Recommendation: COMMENT — net-positive, mergeable. Two non-blocking UX gaps and a couple of nits noted below.

Summary

Clean, well-scoped execution of ADR-034. The change correctly reframes PTY as provider mode 2d of the ai-chat node: the proto/daemon/tauri/cli launch path threads node_id, capture switches from mint→backfill via deep-merge, the viewer dispatches on provider, and the entire standalone agent-sessions cluster (tab + 3 panels + nav/tab wiring, ~1057 lines) is deleted with no live references left behind. Doc comments are unusually good and explain the "why" (ADR references, the update_node_unchecked rationale). The namespace/round-trip behavior is sound (verified below). I traced the highest-risk paths and found no blockers.

Requirements Check (issue #1220)

• ✅ /ai-chat creates a placeholder node with no provider — slash command (core-plugins.ts:609) uses empty contentTemplate and sets no provider; viewer treats provider === undefined as placeholder.
• ✅ User can select any of the four provider modes before launch — PROVIDER_OPTIONS picker in ai-chat-node-viewer.svelte, selectProvider() persists onto the node.
• ✅ provider: pty renders embedded terminal as the node's viewer — AiChatPtyView re-hosts pty-terminal.svelte; no separate tab/panel.
• ✅ Capture backfills instead of minting — capture_service.rs uses update_node_unchecked on session.node_id.
• ✅ Standalone agent-sessions tab + launch/sessions panels removed — all three panels deleted; nav/tab/type wiring removed; only doc-comment mentions remain.
• ✅ Multiple concurrent sessions = N nodes — each node carries its own capture:session_id; no shared/global session state.
• ✅ Mode 2d capture limited to transcript/session-id/metadata — build_capture_properties emits only capture:* + status/last_active; new test backfill_does_not_emit_provider_or_messages locks this in.
• ✅ No PTY-mechanics regression — pty-terminal.svelte launch/stream/write/resize/terminate paths untouched; only re-hosted.

Correctness deep-dive (verified, no defects found)

• Backfill via update_node_unchecked vs OCC update_node: Justified. Capture is a single fire-and-forget writer touching only capture:* + status/last_active; bypassing the optimistic-concurrency version is correct here, since a spurious version conflict from a concurrent viewer edit must not silently drop the transcript. The merge is additive (deep_merge_namespaced_properties, node_service.rs:1446), so provider/model/messages are preserved.
• Namespace round-trip: The flat keys written by both frontend (provider, capture:session_id, status) and daemon are normalized into the ai-chat namespace on write (normalize_flat_properties_to_namespace, :1487) and flattened back out on read (flatten_properties_for_api, models/mod.rs:130). capture:-prefixed keys survive both directions (they don't start with _). Writer and reader are consistent.
• Launch-vs-capture race on status: Frontend writes status: active at launch; daemon writes status: archived at session end. End strictly follows launch, so archived wins — correct terminal state.
• Message-UI lifecycle for pty/placeholder: Safe. onMount → loadMessagesFromNode() no-ops on a node without messages; chatStore.sendMessage is only reachable via ChatInput, rendered only in the isMessageProvider branch; the scroll $effect guards on if (messagesContainer), which is bound only in the message branch. No streaming side effects fire for pty/unset.

Findings

🟡 Important — Re-attach to a dead session renders a blank, silent terminal

📁 packages/desktop-app/src/lib/components/viewers/ai-chat-pty-view.svelte:55-58 (sessionId derivation) + packages/desktop-app/src/lib/components/agent/pty-terminal.svelte:20-69

sessionId = activeSessionId ?? persistedSessionId, where persistedSessionId comes from capture:session_id. After capture runs (session ended, status: archived), reopening the node still derives a sessionId and mounts PtyTerminal, which subscribes to pty-output-${id} / pty-closed-${id} events that will never fire again and never checks liveness or replays backscroll. The user sees an empty black terminal with no indication the session is over, and there is no affordance to start a fresh session on that node.

Recommendation: gate the terminal on liveness — e.g., if status === 'archived' (or a daemon "is this session alive?" check fails), render a read-only transcript/summary view (from capture:transcript/capture:summary) plus a "Start new session" button instead of an inert terminal. Engineering principle: fail visibly — a dead UI surface that silently waits on events that can't arrive is worse than an explicit terminal state.

🟡 Important — No frontend reactivity to the daemon's backfill

📁 packages/daemon/src/services/capture_service.rs:84-95

The daemon backfills the node out-of-band via NodeService, bypassing the frontend sharedNodeStore. After capture completes, an open viewer holds a stale node (still status: active, no capture:transcript) until a manual reload or store refresh. This isn't a data-integrity bug (persistence is correct), but combined with the finding above it means the archived/transcript state may not surface without a reload. Worth confirming whether an existing store-sync/subscription path picks this up; if not, consider emitting a node-changed event on backfill. Engineering principle: keep the read model eventually consistent with writes the user can observe.

🟢 Suggestion — archived pty nodes are effectively terminal with no relaunch path

📁 ai-chat-pty-view.svelte:55-66

Once capture:session_id is set, the config/launch step is unreachable (the {#if sessionId} branch always wins). A user cannot launch a second session on the same node. This may be intentional (one node = one session, create a new /ai-chat for another), but it isn't stated. A one-line comment documenting the intent, or a relaunch affordance, would prevent future confusion.

🟢 Suggestion — optional string node_id semantics carry a dual meaning

📁 packages/proto/proto/agent_session_service.proto:41-49 and capture_service.rs:36-40

node_id is documented as "always set in the unified model," but the only in-tree producer that sets it is the Tauri/frontend path; the CLI deliberately passes None, and the daemon treats None as "skip capture." That's a reasonable design, but the comment "always set" is slightly contradicted by a first-class caller (CLI) that never sets it. Consider rewording to "set whenever the session is a view onto an ai-chat node (GUI launches); absent for node-less CLI launches, in which case capture is skipped." Engineering principle: comments should describe the actual contract, not the aspirational one.

🟢 Nit — github-client.ts worktree fix is correct; one robustness note

📁 scripts/github-client.ts:632-653

resolveGitDir() correctly distinguishes directory vs file .git, parses the gitdir: pointer, and resolves relative pointers against process.cwd() — this is right for both normal checkouts and linked worktrees, and isWorkingDirectoryClean() reusing it is consistent. Note: the relative-pointer resolution assumes the pointer is relative to the directory containing .git (i.e., cwd), which matches git's behavior here. Two minor points: (a) existsSync(headPath) after resolving is slightly redundant since a resolved worktree gitdir always has HEAD, but it's a cheap safety net — fine to keep; (b) isWorkingDirectoryClean() still only checks that the git dir resolves (the pre-existing TODO behavior) — unchanged by this PR, just noting it remains a stub.

Standards compliance

• ✅ No backward-compat/dual-mode/deprecation — old model deleted outright (greenfield rules honored).
• ✅ No console.* — createLogger used in both new/changed Svelte files.
• ✅ No any, no {@html}.
• ✅ Namespace prefixes — agent-session fields use capture:; core props (provider/model/status/messages) unprefixed as intended.
• ✅ Component naming — AiChatPtyView is a sub-view of AiChatNodeViewer, not a direct BaseNode usage.
• ✅ No #[allow(dead_code)] / lint suppression introduced.

Testing

• ✅ Rust capture tests updated for the new signature; added backfill_does_not_emit_provider_or_messages — good regression guard for the core invariant.
• ✅ layout.test.ts updated (5→4 nav items) with an explanatory comment.
• 🟢 Gap: no test covers the re-attach/archived-session path (finding Set up Tauri + Svelte project structure #1) or AiChatPtyView's launch→persist flow. Given the UX gaps above, a component test asserting that an archived node does not render a live terminal would be valuable. Not a merge blocker.

Verdict

Net-positive, cohesive, atomic change that faithfully implements ADR-034 and meets every acceptance criterion. No blockers. The two 🟡 items (dead-session terminal, store staleness after backfill) are real UX/consistency gaps worth a follow-up but do not compromise data integrity. Recommend addressing finding #1 before or shortly after merge.

[malibio]

Automated pragmatic code review posted as a PR comment. Recommendation: COMMENT (net-positive, mergeable). Two non-blocking UX/consistency gaps noted: (1) re-attach to a dead/archived session renders a blank silent terminal; (2) no frontend reactivity to the daemon's out-of-band backfill. All 8 acceptance criteria met; standards compliant. See the full report comment for details.

[malibio]

Addressed review feedback (commit b5b55d0)

✅ Addressed 4 of 5 findings:

• 🟡 Set up Tauri + Svelte project structure #1 — Dead-session terminal on reopen: The pty view no longer mounts a blank, silent terminal on a session whose PTY has exited. When the node is archived (capture backfill) or the session's exit is observed live, it now renders a read-only summary/transcript card + a Start new session button.
• 🟡 Create design system foundation #2 — No live reactivity to the daemon's out-of-band backfill: The view now listens for pty-closed-<sessionId> on the session it launched and flips to the ended state immediately, without waiting for a reload.
• 🟢 Desktop shell with resizable panels #3 — No relaunch path for archived pty nodes: Subsumed by Set up Tauri + Svelte project structure #1 — Start new session clears the stale capture:session_id, sets status: active, and returns to the config step.
• 🟢 Enhanced BaseNode with Hierarchical Circle Indicators + TextNode Implementation #4 — node_id "always set" doc: Reworded the proto + tauri-commands.ts docs — the desktop app always sets it; other callers (CLI) omit it and capture is then skipped.

⏭️ Skipped 1 finding:

• 🟢 Basic storage integration #5 — isWorkingDirectoryClean() stub: Pre-existing in github-client.ts, unchanged by this PR. Out of scope.

Verification: bun run --cwd packages/desktop-app quality:fix clean (0 errors/0 warnings); bun run test 3936 passed; daemon rebuild clean.

[malibio]

Re-Review — PR #1223 (commit b5b55d04 only)

Review Type: Re-review (delta 4ecdfd18..b5b55d04)
Reviewer: Principal Engineer AI Reviewer (Pragmatic Quality)
Recommendation: COMMENT — net-positive. Prior 🟡 findings are substantively addressed. One genuine correctness gap in startNewSession's persisted-key clearing (delete-by-omission is a no-op against the backend's additive merge), plus minor nits. Not a merge blocker, but should be fixed or explicitly acknowledged.

Verification of prior findings

1. 🟡 Dead-session terminal on reopen — ADDRESSED.
isEnded = sessionEnded || status === 'archived' and sessionId = isEnded ? null : (activeSessionId ?? persistedSessionId). Reopening an archived node now renders the read-only pty-ended summary/transcript card instead of mounting a blank PtyTerminal. The primary case (capture has run, status: archived) is correctly handled.

2. 🟡 No live reactivity to daemon backfill — ADDRESSED (for the launching client).
The $effect registers listen('pty-closed-<activeSessionId>') and latches sessionEnded = true, flipping to the ended view without a reload. Listener lifecycle is correct (see below). Note this only covers the viewer that launched the session via activeSessionId; a second viewer open on the same node that did not launch still relies on status === 'archived' from a store refresh — consistent with the prior finding's scope, acceptable.

3. 🟢 Relaunch path — partially effective (see Critical-ish finding below). The startNewSession() affordance and UI reset work in-memory, but the persisted capture:session_id is NOT actually cleared.

4. 🟢 node_id doc — ADDRESSED. Both proto files + tauri-commands.ts now describe the real contract ("desktop app always sets it; CLI omits it, capture skipped"). Accurate.

5. 🟢 isWorkingDirectoryClean stub — reasonably SKIPPED. Pre-existing, untouched by this PR, out of scope. Agreed.

$effect listener teardown — verified correct

• Effect depends only on activeSessionId. On change/unmount, the returned cleanup sets cancelled = true, calls unlistenClosed?.(), nulls it.
• Async race: if listen() resolves after cleanup, .then sees cancelled === true and immediately calls fn() — no leak, no dangling subscription.
• No double-unlisten: unlistenClosed is nulled after unlisten; onDestroy re-call is a no-op.
• Re-run teardown: previous listener is torn down before the next registers. Correct.

New finding

🟡 Important — startNewSession() does not clear the persisted capture:session_id (delete-by-omission is a no-op against the backend merge)

📁 ai-chat-pty-view.svelte (startNewSession) + packages/core/src/services/node_service.rs:1446 (deep_merge_namespaced_properties)

startNewSession() rebuilds the properties object with delete props['capture:session_id'] and persists via sharedNodeStore.updateNode(...). This clears the key only in the in-memory optimistic node. The persistence path normalizes the flat props into the ai-chat namespace and applies deep_merge_namespaced_properties, which is strictly additive — it inserts every key present in the incoming map but never removes a key that is absent. Omitting capture:session_id therefore leaves the stored value untouched in the DB. (Sending capture:session_id: null would not help either: normalize+merge would store the literal null, not delete the key.)

The status: 'active' part of the same update does persist (it overwrites archived), so the two halves of the reset diverge: in the DB you end up with status: active but a stale capture:session_id still pointing at the dead session.

Consequence: after Start-new-session → launch → reload (before the new session's capture backfill overwrites the id), the view re-derives persistedSessionId = the old dead session id, isEnded is false (status is active again), so sessionId = the dead id and PtyTerminal re-attaches to a terminated PTY — re-introducing exactly the blank-silent-terminal symptom finding #1 set out to eliminate. The window closes once the new session ends and capture overwrites the id, so this is a windowed regression, not a persistent corruption.

Recommendation (pick one):

• Add a backend tombstone/removal semantic (e.g. honor an explicit null value in the merge as a delete) and send capture:session_id: null; or
• Have startNewSession overwrite capture:session_id with the new session id at launch time rather than trying to delete it, so it is never stale; or
• At minimum, gate persistedSessionId re-attach on a daemon liveness check so a stale id can never mount a dead terminal.

Engineering principle: persistence semantics must match the code's apparent intent. A delete that silently doesn't delete is a correctness hazard for any future reader, and the commit message's claim that this "clears the stale session id" is not what actually happens at the storage layer.

Nits

• Nit: onDestroy(() => unlistenClosed?.()) is redundant — Svelte 5 already runs the $effect cleanup on unmount, after which unlistenClosed is null. Harmless, but the $effect return is the single source of truth; the extra onDestroy can be removed.
• Nit: summary/transcript/agentType are read straight off node.properties['capture:*']; if a future capture mode emits non-string values these as string | undefined casts would silently mis-type. Fine given current capture only emits strings.

Standards

Clean: no any, no console.*, createLogger('AiChatPtyView') used, no {@html}. Transcript renders via <pre>{transcript}</pre> (Svelte text interpolation auto-escapes) — confirmed safe, no XSS surface.

[malibio]

Re-review posted as a PR comment (delta 4ecdfd1..b5b55d0). Summary: prior 🟡 findings substantively addressed; one new 🟡 — startNewSession's delete-by-omission does not actually clear the persisted capture:session_id against the backend's additive deep-merge (windowed re-attach-to-dead-session regression). Net-positive, non-blocking. Details in the comment.

[malibio]

Re-review posted as a PR comment (delta 4ecdfd1..b5b55d0). Net-positive; one new 🟡 on startNewSession persisted-key clearing. See comment for details.

[malibio]

Delta re-review (commit 82a40151) — 🟢 Bug fixed

Verified the fix for the stale capture:session_id re-attach bug. The previous delete-by-omission approach could not remove the key (deep_merge_namespaced_properties is additive-only), leaving a dead session id that re-mounted a blank terminal after relaunch+reload.

The new configuring flag approach is correct. Traced all derived-state combinations:

Scenario	configuring	activeSessionId	sessionEnded	status	isEnded	sessionId	View
After Start new session	true	null	false	active	false	null	Config ✓
After launch succeeds	false	newId	false	active	false	newId	Live terminal ✓
Reopen archived node	false	null	false	archived	true	(n/a)	Ended ✓
Reopen still-live session	false	null	false	active	false	persistedId	Re-attach ✓ (ADR-032)
Live session exits	false	oldId	true	active	true	(n/a)	Ended ✓
Start new session → launch FAILS	true	null	false	active	false	null	Config + error ✓

The key win: in the window between Start-new-session and a successful (or failed) launch, configuring short-circuits the persistedSessionId fallback, so the stale dead id is never mounted. launch() then overwrites capture:session_id with the new id, so the stale value is gone after the next reload regardless.

Also verified:

• Redundant onDestroy removed; listener teardown now scoped to the $effect cleanup (locally-scoped unlisten), which runs on both activeSessionId change and unmount. The cancelled-before-registration race is handled. No leak.
• Imports clean: onMount still used (line 154), UnlistenFn still used (line 111), onDestroy removed.
• Atomic single-file change.

No remaining issues. Approving.

[malibio]

Delta re-review of 82a40151: the stale session-id re-attach bug is fully fixed. The configuring flag correctly suppresses the persistedSessionId fallback during the config window (including the launch-failure path), and launch() overwrites the persisted id on success. Listener teardown is now correctly scoped to the $effect cleanup with the redundant onDestroy removed. Imports clean. No remaining issues.