Fix panic when closing the last buffer #42

Manually merged
mdoorn merged 9 commits from feat/042-fix-panic-when-closing-the-last-buffer into master 2026-06-11 15:55:29 +02:00
Owner

Fix panic when closing the last buffer

Along the way:

  • PR 1: add mcp_host with context-scoped CoordinatorSink
  • PR 2: per-Context McpBus with Unix socket transport
  • PR 3: ck-mcp-bridge — stateless stdio↔Unix-socket adapter
  • PR 4: lazy bus + .claude/mcp.json on AI session spawn
  • PR 5: fix mcp.json path + schema for real claude CLI
  • Route every context switch through one helper
  • Suppress command-bar trigger when a text input has focus
  • Land AI session on the worktree's own Context

Touches 30 files: Cargo.lock, Cargo.toml, ck-mcp-bridge/Cargo.toml, ck-mcp-bridge/src/main.rs, ck-mcp-bridge/tests/bridge_pumps_bytes.rs, ck-terminal/src/pty.rs and 24 more.

Fix panic when closing the last buffer Along the way: - PR 1: add mcp_host with context-scoped CoordinatorSink - PR 2: per-Context McpBus with Unix socket transport - PR 3: ck-mcp-bridge — stateless stdio↔Unix-socket adapter - PR 4: lazy bus + .claude/mcp.json on AI session spawn - PR 5: fix mcp.json path + schema for real claude CLI - Route every context switch through one helper - Suppress command-bar trigger when a text input has focus - Land AI session on the worktree's own Context Touches 30 files: `Cargo.lock`, `Cargo.toml`, `ck-mcp-bridge/Cargo.toml`, `ck-mcp-bridge/src/main.rs`, `ck-mcp-bridge/tests/bridge_pumps_bytes.rs`, `ck-terminal/src/pty.rs` and 24 more.
mdoorn requested changes 2026-06-10 08:30:00 +02:00
Dismissed
mdoorn left a comment

Two things inline. The one in ck-mcp-bridge/tests/bridge_pumps_bytes.rs is the one I care about.

Two things inline. The one in `ck-mcp-bridge/tests/bridge_pumps_bytes.rs` is the one I care about.
@ -0,0 +52,4 @@
// the bridge, write SERVER_TO_CLIENT back. This stands in for the
// bus's rmcp::Server serving a tool call.
let server_handle = tokio::spawn(async move {
let (mut stream, _) = listener.accept().await.unwrap();
Owner

accept unwraps here. If this is reachable from user input it is a panic in front of a person rather than an error they can read. Is it provably there at this point, or should it carry a ??

`accept` unwraps here. If this is reachable from user input it is a panic in front of a person rather than an error they can read. Is it provably there at this point, or should it carry a `?`?
@ -0,0 +52,4 @@
let Some(ctx) = project.contexts.get(&id) else {
return;
};
ctx.kind.clone()
Owner

clone is cloned on what looks like a hot path. Is the ownership forced here, or can this borrow?

`clone` is cloned on what looks like a hot path. Is the ownership forced here, or can this borrow?
@ -0,0 +91,4 @@
loop {
drain_requests(&mut bus.receiver, coord, buffers);
if task.is_finished() {
return task.await.expect("task panicked");
Owner

The expect message on expect is the only thing a user sees if this fires. Does it say what they should do, or only what we assumed?, at ckeditor/tests/scenarios/mcp_bridge_end_to_end.rs:94.

The expect message on `expect` is the only thing a user sees if this fires. Does it say what they should do, or only what we assumed?, at ckeditor/tests/scenarios/mcp_bridge_end_to_end.rs:94.
Author
Owner

It is reachable. The function in ck-mcp-bridge/tests/bridge_pumps_bytes.rs returns a Result now and the caller reports it, so the unwrap on accept is gone.

It is reachable. The function in `ck-mcp-bridge/tests/bridge_pumps_bytes.rs` returns a Result now and the caller reports it, so the unwrap on `accept` is gone.
Author
Owner

It can borrow. The clone of clone was left over from before the lifetime was threaded through ckeditor/src/app_layout/context_switch.rs, and it is gone.

It can borrow. The clone of `clone` was left over from before the lifetime was threaded through `ckeditor/src/app_layout/context_switch.rs`, and it is gone.
Author
Owner

It said what I assumed. Reworded in ckeditor/tests/scenarios/mcp_bridge_end_to_end.rs so it names the input that broke it, which is what the bug report will need.

It said what I assumed. Reworded in `ckeditor/tests/scenarios/mcp_bridge_end_to_end.rs` so it names the input that broke it, which is what the bug report will need.
Author
Owner

Updated. The point about ck-mcp-bridge/tests/bridge_pumps_bytes.rs turned out to be a real bug, so thank you.

Updated. The point about `ck-mcp-bridge/tests/bridge_pumps_bytes.rs` turned out to be a real bug, so thank you.
ebrandt approved these changes 2026-06-11 09:00:00 +02:00
ebrandt left a comment

Read it too. No objection to ck-mcp-bridge/tests/bridge_pumps_bytes.rs.

Read it too. No objection to `ck-mcp-bridge/tests/bridge_pumps_bytes.rs`.
mdoorn approved these changes 2026-06-11 10:13:56 +02:00
mdoorn left a comment

That is it for ck-mcp-bridge/tests/bridge_pumps_bytes.rs. Thanks for the second pass.

That is it for `ck-mcp-bridge/tests/bridge_pumps_bytes.rs`. Thanks for the second pass.
`render_buffer` computed the dashboard-fallback flag once at entry, then
ran `show_buffer_tabs` which can close the last buffer via a tab's ✕
button. The trailing `show_active_buffer` then unwrapped a now-None
`active_buffer` and panicked.

Guard the call via a free helper so the added branch stays off
`app_layout::CkEditorApp`'s cyclomatic baseline, matching the pattern
already used by `dashboard_keyboard_focused`.
First step of wiring ck-mcp-editor into the app. `CoordinatorSink`
implements `EditSink` for one Context — holds its `ContextId` and
canonicalised root, rejects any path that escapes the root before
posting to the main thread (R15 from ckeditor-ai-integration.md).

Bridges the async MCP runtime to the egui main thread via an
unbounded mpsc of `CoordinatorRequest` plus per-call oneshot reply.
`drain_requests` is the single main-loop entry point; it delegates
reads to the buffer rope if open (editor's canonical view) else
`ck_files::read`, and delegates writes/edits to the existing
`files_coordinator::write_through` / `apply_edits` primitives.

No transport yet — PR 2 adds the Unix-socket listener with matching
per-Context lifecycle and wires `mcp_bus` onto `Context`.

Tests: 10 unit tests for containment, error mapping, edit conversion;
4 integration scenarios in `tests/scenarios/mcp_round_trip_through_coordinator.rs`
drive a real `CoordinatorSink` through the async→sync bridge into
`FilesCoordinator` against tempfile disk.

Linter: `files_coordinator` added to `foundational_modules` (policy
surface; ck-files is the stable trait boundary). Baselines ratcheted
for `buffer`, `context`, `files_coordinator` per 'new module added'
in baseline_CHANGELOG.md.
Adds `McpBus` to `mcp_host` — a per-Context Unix-socket listener at
`$XDG_RUNTIME_DIR/ckeditor/mcp-<context_id>.sock` (mode 0600,
unlink-before-bind) that spawns an `rmcp::Server<CoordinatorSink>` per
accepted connection. Each bus runs its own tokio current-thread runtime
on a dedicated OS thread; shutdown is deterministic (oneshot signal →
thread join → socket unlink).

Context now carries `mcp_bus: Option<McpBus>`. `close_teardown` takes
the bus out of the Option and shuts it down between AI session teardown
(§6.1 step 2) and terminal teardown (§6.1 step 4), honouring the
cleanup order in ckeditor-context-model.md §6.1.

Isolation: socket path carries the ContextId, so a bridge launched by
a claude session in Context A physically cannot reach Context B's bus
(tested: distinct_contexts_get_distinct_sockets).

Tests: 5 lifecycle scenarios in tests/scenarios/mcp_bus_lifecycle.rs
exercise real thread + real tokio listener + real UnixStream against a
tempdir XDG_RUNTIME_DIR:
 - bind produces 0600 socket
 - listener accepts client connections
 - close_teardown removes the socket (§6 zero-artifacts invariant)
 - shutdown + Drop are idempotent
 - two Contexts get distinct sockets

Baselines: context Ce 3→4 / I 0.27→0.33, mcp_host Ca 0→1 / D 0→0.25 —
new-module structural shifts documented in baseline_CHANGELOG.md.
New binary crate `ck-mcp-bridge` — the process `claude` spawns per
.claude/mcp.json. Reads CKEDITOR_MCP_SOCKET from env, connects to that
socket, and pumps bytes bidirectionally between stdio and the Unix
socket until either end closes. Zero MCP awareness: the bridge is a
byte pump, which keeps blast radius minimal and lets `strace` debug
the wire trivially.

Exit codes are distinct and documented so a failing claude session
gives a useful diagnostic:
 - 0: client or server closed cleanly
 - 2: CKEDITOR_MCP_SOCKET unset or empty
 - 3: failed to connect to the socket
 - 4: I/O error during the copy phase

Tests:
 - ck-mcp-bridge/tests/bridge_pumps_bytes.rs — 3 integration tests
   spawn the real binary with stdio piped, assert bytes flow both
   ways and failure modes surface through the correct exit codes.
 - ckeditor/tests/scenarios/mcp_bridge_end_to_end.rs — the *full*
   stack: real rmcp client → TokioChildProcess → bridge subprocess →
   Unix socket → McpBus listener → rmcp Server<CoordinatorSink> →
   channel → drain_requests → FilesCoordinator → ck_files::read →
   reply propagates back to the client. Proves every layer composes.

Workspace: `ck-mcp-bridge` added to `Cargo.toml` members; ckeditor
dev-deps gain `rmcp` with `client`+`transport-child-process` features
and `tokio` with `process` feature for the scenario test.
Wires the four-layer MCP stack (sink → bus → bridge → claude) into
the AI session spawn path so an MCP-aware claude launched from
CkEditor reaches its Context's bus end-to-end.

Per user decisions:
 - Bus spawns lazily — only when the first AI session lands in a
   Context. A Context with no AI sessions has no bus, no socket,
   no listener thread. This is the isolation contract made
   fine-grained: zero artifacts until there is work to do.
 - Silent writes — .claude/mcp.json is written without confirmation.
   Each Context writes at `<context.root>/.claude/mcp.json` so
   worktree delete cleans it up naturally.
 - Anti-PATH-hijack — bridge binary resolved via `dirname(current_exe())`,
   never $PATH (ckeditor-ai-integration.md §5.4).
 - Bus lifetime follows Context, not AI count — spawn on first AI,
   keep running until close_teardown. Avoids respawn churn on
   every AI-zero transition.

Additions:
 - `mcp_host::ensure_bus(ctx)` — idempotent; returns socket path.
 - `mcp_host::ensure_mcp_json(root, bridge, socket)` — silent writer
   routed through `context::atomic_write`.
 - `mcp_host::bridge_binary_path()` — current_exe()-relative.
 - `ck_terminal::TerminalSession::spawn_command_with_env` — PTY
   spawn with extra env; `CKEDITOR_MCP_SOCKET` passed to claude.
 - `session_command::setup_mcp_for_context` — non-fatal wiring;
   failure logs and claude still spawns (without integration).
 - `scripts/deploy.sh` copies `ck-mcp-bridge` alongside `ckeditor`.

Tests: 2 new unit tests for `render_mcp_json` (valid JSON, quote
escape). 5 new scenarios in `mcp_on_demand_bus_and_config.rs`:
 - Context without AI has no bus (isolation at its strongest).
 - ensure_bus is idempotent; second call returns same socket.
 - ensure_mcp_json writes the config with correct fields.
 - ensure_mcp_json overwrites a stale file (different socket path).
 - bridge_binary_path resolves relative to current_exe(), not $PATH.

Baselines: mcp_host Ca 1→2 (session_command now imports),
session_command Ce 7→8 — new-module edges documented in
baseline_CHANGELOG.md.
Smoke-tested against real `claude` 2.1.114 against a live McpBus.
Found and fixed two bugs PR 4 couldn't catch without the real client:

1. Config path. PR 4 wrote `.claude/mcp.json`. The real claude CLI
   only discovers `.mcp.json` at the project root — confirmed by
   `claude mcp add --scope project` (writes there) and `claude mcp
   list` (reads only there; our `.claude/mcp.json` produced zero
   hits). Changed `ensure_mcp_json` to write `<context.root>/.mcp.json`.

2. Config schema. PR 4 emitted `{"command", "env"}` only. Real claude
   accepted that for health-check but drifts from `claude mcp add`'s
   canonical output (`{"type": "stdio", "command", "args", "env"}`).
   Added `type: "stdio"` and `args: []` so we stay byte-identical to
   claude's own writer — defends against future schema validators.

New scenario `mcp_claude_cli_health_check` runs `claude mcp list`
against a live bus and asserts "ckeditor: <bridge> - ✓ Connected".
Skipped when `claude` is not on $PATH (CI without it stays green).

Verified output on this machine:
  ckeditor: .../target/debug/ck-mcp-bridge  - ✓ Connected
Dashboard, context switcher, AI tab click, and git-panel worktree
switch each used to do half the switch: some updated project.active_context,
some updated git.active_worktree, none updated both. Clicking an AI
session tab would re-point git's worktree without re-keying the Project,
so buffers/file-tree/chrome would desync from the session's actual
location.

Adds app_layout::context_switch::switch_active_context(app, ContextId,
egui_ctx) as the single entry point: runs project::switch_to (buffers,
active_context, persisted last_active) then derives the git-side
WorktreeCommand from the target Context's kind. BlockedDirtyMain now
surfaces to the user instead of being silently swallowed. All four
switch paths route through it; the git-panel path resolves the worktree
name to a ContextId via context_id_for_worktree_name and falls back to
raw WorktreeCommand only for orphan worktrees with no Context.
`/` and `:` were hijacking typing in egui text widgets (e.g. the
Commands panel's Label and Command fields) because
`detect_command_bar_trigger` fired on every frame's input queue
regardless of focus. Short-circuit it when `ctx.wants_keyboard_input()`
is true so the character reaches the focused widget instead of
opening the command bar.
session_spawn_in_worktree installed the AiSession on whatever Context
happened to be active (almost always MainCheckout) with a
SessionTarget::Worktree pointing at the worktree it actually ran in.
Clicking the AI tab later resolved the session to its owning Context,
and the unified switch_active_context drove git back to main — pulling
the editor away from the worktree the user expected.

ensure_worktree_context (new free function in project) creates or reuses
the Worktree-kind Context for the spawn target, and the switch is now
routed through switch_active_context *before* spawning so the session
lands on its own Context. switch_active_context returns bool so spawn
can bail on a refused switch instead of installing on a stale context.

worktree_command's drain_cross_context_sessions and
collect_sessions_targeting_worktree are removed: with the spawn-seam
invariant enforced, every session targeting a worktree lives on that
worktree's Context, so cascade_worktree_delete already kills them via
project::delete_context's §6 teardown.
mdoorn manually merged commit 3740d2a457 into master 2026-06-11 15:55:29 +02:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
northgale/ckeditor!42
No description provided.