diff --git a/CHANGELOG.md b/CHANGELOG.md index f4c63154..550bb59a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,26 +46,10 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. end target is 95% (D-66). `ci/test.sh` now writes `coverage/lcov.info` as a side effect of the unit/widget/golden run so the gate adds no extra test invocation. -- Test sweep covering `kernel/src/commands/keybindings.dart` (KeyEvent - modifier mapping, parse-error edges, resolver entries view), - `kernel/src/toolchain_paths.dart` (the Flutter-free `ToolchainView.resolved` - static view), `widgets/src/clide_tooltip.dart` (hover-delay overlay, - flip-above placement, re-entry cycle), `widgets/src/clide_palette.dart` - (filter typing, submit-invokes-first, hover state), - `widgets/src/multitab_controller.dart` (`copyWith`, `length`/`isEmpty` - getters), and `widgets/src/clide_markdown.dart` (h3–h6, tables, - strikethrough, default block fallback, record-link tap). Crosses - the 93% line-coverage threshold (T-91). -- `kernel/src/syntax/tree_sitter_service.dart` test sweep — every - `colorForRole` switch arm plus fake-FFI coverage of - `_init`/`_loadGrammar`/`highlight`/`dispose` branches, taking the - file from 17% → 96%. Real-library smoke test - (`test/kernel/src/syntax/tree_sitter_smoke_test.dart`) dlopen's the - vendored `native/linux-x64/libtree-sitter.so`, loads the bundled - `dart` grammar end-to-end, and verifies highlight emits sane spans — - catches FFI signature drift the fake-driven tests can't. Skips on - non-Linux and when the vendored library isn't present. Drives total - line coverage across the 95% target (T-91). +- Test sweep — `keybindings`, `toolchain_paths`, and several + `widgets/src/` primitives (tooltip, palette, multitab, markdown). +- `tree_sitter_service` sweep — fake-FFI + real-library smoke, + 17% → 96%. Crosses the 95% global target (T-91). - Staged `dart doc` CI job — generates and uploads an HTML API reference for the public `lib/` surface. The step wraps `dart doc --validate-links` and grep-fails the build on any warning, @@ -119,16 +103,14 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. ### Changed -- Pre-push line-coverage floor ratcheted to 95% — D-66 target hit. -- `TreeSitterService` accepts injectable `TreeSitterLib` and grammar / - query loaders so tests can substitute a fake FFI surface without - dlopen'ing `libtree-sitter.so`. `TreeSitterLib.testing(...)` exposes a - named-parameter constructor with safe no-op defaults for every native - function; `TreeSitterLib.fromDynamicLibrary(...)` lets the smoke test - load the vendored `.so` directly. Production paths - (`TreeSitterService.shared`, `TreeSitterLib.instance`) unchanged. -- Tidied test imports — dropped redundant `dart:ui` / `dart:typed_data` - / barrel-redundant package imports flagged by `unnecessary_import`. +- PTY spawning uses `posix_openpt` + `posix_spawn` instead of + `forkpty` — closes a ~5% deadlock window in the multithreaded Dart + VM (T-96, D-5 amended). Missing exe/cwd now throw `PtyException` at + spawn time. Drops the `libutil.so.1` dependency. +- Coverage floor ratcheted to 95% — D-66 target hit. +- `TreeSitterService` and `TreeSitterLib` accept injectable FFI + asset + loaders for fake-driven tests; production paths unchanged. +- Tidied test imports flagged by `unnecessary_import`. - Terminal panes now render bold attributes with a real bold weight — bundled JetBrainsMono Bold + BoldItalic are registered with the `JetBrainsMono` family at `weight: 700`. The painter's bold diff --git a/CLAUDE.md b/CLAUDE.md index 06e62de9..d111ba38 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,7 +6,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co An IDE for Claude Code CLI. Single Flutter package at the repo root. -- **`lib/`** — all Dart code. Subsystem handlers (`lib/src/daemon/`, `lib/src/pty/`, `lib/src/ipc/`, `lib/src/git/`, `lib/src/pql/`), kernel services (`lib/kernel/`), UI widgets (`lib/widgets/`), built-in extensions (`lib/builtin/`), and the extension framework (`lib/extension/`). The Flutter app hosts the IPC server in-process (D-56). PTY spawning uses Dart FFI `forkpty()` directly. +- **`lib/`** — all Dart code. Subsystem handlers (`lib/src/daemon/`, `lib/src/pty/`, `lib/src/ipc/`, `lib/src/git/`, `lib/src/pql/`), kernel services (`lib/kernel/`), UI widgets (`lib/widgets/`), built-in extensions (`lib/builtin/`), and the extension framework (`lib/extension/`). The Flutter app hosts the IPC server in-process (D-56). PTY spawning uses Dart FFI `posix_openpt()` + `posix_spawn()` directly. - **[`pql`](https://github.com/postmeridiem/pql)** — external supporter tool. Clide wraps it for every query surface; never re-implements it. tmux owns Claude session persistence (D-41) — the app re-attaches on restart via `tmux new-session -A`. Native rendering — markdown, canvas, graph — is Dart/Flutter (`CustomPaint` + widgets), not third-party packages. @@ -20,7 +20,7 @@ These are load-bearing. Violating any means the design is wrong, not the rule. - **Flutter desktop is the host. No Electron, ever.** Web target may work as a happy accident — don't compromise desktop fidelity for it. If we ship a web build at all, prefer Flutter's **WebAssembly (CanvasKit/Skwasm) compile** over the JS/HTML renderer. `xterm.dart` is the terminal renderer; markdown, canvas, graph are custom `CustomPaint`/widget components. - **Single process.** The Flutter app hosts everything in-process: IPC server, subsystem handlers (pane, files, editor, git, pql), extensions. No separate daemon binary (D-56 dissolved it). - **CLI-first, not MCP.** Claude talks via Bash (`clide ...`), matching pql's contract. See [`D-1`](governance/decisions/architecture.md#d-1-cli-first-not-mcp). -- **Dart is the core; pql fills the query gap.** PTY spawning is native Dart FFI (`forkpty`). `pql` (Go) handles vault queries. No second "core language." See [`D-5`](governance/decisions/architecture.md#d-5-dart-core-sidecar-dissolved-ptyc-as-pql-peer) (amended by D-56). +- **Dart is the core; pql fills the query gap.** PTY spawning is native Dart FFI (`posix_openpt` + `posix_spawn`). `pql` (Go) handles vault queries. No second "core language." See [`D-5`](governance/decisions/architecture.md#d-5-dart-core-sidecar-dissolved-ptyc-as-pql-peer) (amended by D-56). - **Own the rendering stack.** PTY (via Dart FFI), markdown renderer, graph, canvas — all clide-owned, not pulled from opinionated packages. - **User/Claude parity.** Every CLI subcommand has a UI affordance, and every UI action has a CLI. See [`D-6`](governance/decisions/architecture.md#d-6-cli-and-event-surface-contract). - **pql: wrap, don't duplicate.** Pql logic only lives in `lib/src/pql/` (pure shell-outs). Clide owns pql's `ignore_files:` config key; it never touches pql's `.pql/` index/cache data. See [`D-3`](governance/decisions/architecture.md#d-3-pql-as-supporter-tool-clide-wraps-never-duplicates). diff --git a/ci/test.sh b/ci/test.sh index 35acd214..c551823c 100755 --- a/ci/test.sh +++ b/ci/test.sh @@ -12,7 +12,7 @@ echo "==> dart format (whole tree)" dart format --set-exit-if-changed . echo "==> dart test (forkpty — incompatible with flutter test runner)" -dart test --tags forkpty test/pty/session_test.dart +dart test --tags forkpty test/pty/session_test.dart test/panes/registry_test.dart echo "==> flutter test --coverage (unit + widget + golden)" flutter test --coverage --exclude-tags forkpty diff --git a/dart_test.yaml b/dart_test.yaml new file mode 100644 index 00000000..a86d63ae --- /dev/null +++ b/dart_test.yaml @@ -0,0 +1,10 @@ +# Declares tags used by inline `test('...', tags: [...], ...)` calls so +# that `flutter test --exclude-tags ` and `dart test --tags ` +# both honor them. Undeclared tags are ignored by the runners, which +# silently breaks selective excludes. +tags: + # Tests that call `forkpty()` via Dart FFI. Must run under `dart test`, + # not `flutter test` — the latter's runner hosts a multi-threaded + # Flutter engine in which forkpty produces a master fd that never + # delivers output. See `test/pty/session_test.dart`. + forkpty: diff --git a/governance/decisions/architecture.md b/governance/decisions/architecture.md index 2a72af4e..f411478d 100644 --- a/governance/decisions/architecture.md +++ b/governance/decisions/architecture.md @@ -73,6 +73,7 @@ Core, rendering, IPC, kernel, panel manager. ### D-1: CLI-first, not MCP - **Date:** 2026-04-20 (was ADR 0001; ported from the claudian lineage) +- **Amendment (2026-05-15):** D-1's intent — the CLI is the *primary* agent-facing surface, with the same contract as pql — stands. An additional `/ide`-compatible MCP surface is added per [D-68](#d-68-dual-integration-surface-bash-cli-primary-mcp-secondary); both wrap the same in-process dispatcher. The escape-hatch line in this record's Cost ("nothing here precludes adding [MCP] later that shells out to the same CLI") is realised — the MCP server does not bypass the CLI's surface, it offers a second transport to it. - **Decision:** Claude talks to clide exclusively via Bash (`clide …`). No MCP server. No protocol layer in Claude's face. The CLI uses the same exit-code + stderr-JSON contract as pql. - **Context:** The two mainstream options for the agent-facing surface were an MCP server or a plain Bash CLI matching pql's contract. - **Rationale:** Same mental model as pql for the agent — one tool-use pattern covers both. No MCP runtime to host, authenticate, or keep in sync with client versions. User/Claude parity is easier to enforce: every CLI subcommand must have a UI affordance in the Flutter app and vice versa ([D-6](#d-6-cli-and-event-surface-contract)). Claude Code's `Bash(clide *)` allow rule is the only configuration clide needs on the agent side. @@ -99,6 +100,7 @@ Core, rendering, IPC, kernel, panel manager. - **Date:** 2026-04-20 (was ADR 0005; supersedes [R-2](rejected.md#r-2-go-sidecar)) - **Amendment (2026-04-23):** The separate daemon process and two-package layout are dissolved per [D-56](#d-56-dissolve-daemon-process-flutter-app-hosts-ipc-server). Dart-core principle survives; the daemon binary does not. - **Amendment (2026-05-07):** `ptyc` retired. PTY spawning moved to Dart FFI `forkpty()` (`lib/src/pty/native_pty.dart`). The `ptyc/` source tree, `PtySession`, and `scm_rights.dart` are removed. `pql` remains the sole external supporter tool. +- **Amendment (2026-05-17):** `forkpty()` replaced with `posix_openpt()` + `posix_spawn()` (T-96). `forkpty` calls `fork()` underneath, which is unsafe in the multithreaded Dart VM: ~5% of spawns deadlocked in the child before `execve` due to libc locks held by ghost-threads at fork time. `posix_spawn` uses `vfork` under glibc/musl/macOS, keeping the parent suspended until `execve` completes — no Dart code runs in the child. Side benefit: dropped the `libutil.so.1` dynamic dependency; PTY now resolves entirely against libc via `DynamicLibrary.process()`. - **Decision:** Three moves. **(1) Dart is the core language.** Everything that used to live under `sidecar/` — IPC server, CLI dispatch, process management, file watching, git shell-outs, pql wrapper — is written in Dart. Two execution modes of one Dart AOT binary: `clide ` (one-shot, pql-style) and `clide --daemon` (long-running, owns PTYs and subprocesses, survives app restarts). The Flutter app imports the Dart core as a library *and* connects to the daemon over IPC. **(2) The sidecar directory dissolves.** Layout is `app/` (Flutter UI), `lib/` (Dart core), `bin/clide.dart` (AOT entry), `ptyc/` (C helper), no `sidecar/`, no Go module. **(3) `ptyc` is a pql-peer supporter tool.** Small C binary that does `posix_openpt` + `fork` + `exec` + fd-passing via `SCM_RIGHTS`; clide wraps it the same way it wraps pql. Shells out for every PTY (terminal pane, tmux session, Claude, LSP server, debug adapter — one code path). Consumers other than clide can use `ptyc` standalone. - **Context:** [R-2](rejected.md#r-2-go-sidecar) picked Go for the sidecar/CLI on two premises: (a) the heavy work belongs in a language separate from the UI layer, and (b) pql is Go so the muscle memory transfers. On reassessment, both premises broke: the "heavy work" is I/O-bound glue that `dart:io` covers cleanly — the real choice was **separate process vs shared language**, and separate-process is what matters. PTY is the one place Dart is genuinely weak (multi-threaded VM can't safely `fork()`), and once you accept a small native helper, *nothing else* needs to be in the same language. - **Rationale:** One toolchain for the IDE proper (Flutter + Dart). C toolchain needed only to build `ptyc` — tiny, rarely-changing. Session persistence stays because PTY master fds live in the Dart daemon process, not the app. `ptyc` naming: **p** for *project* (parallel to pql's *project query language*), **ptyc** reads as both "PTY + child" (domain vocabulary) and "PTY + C" (implementation language). Usable from Dart, Python, Go, shell — anywhere a subprocess can be spawned and a fd received. @@ -244,4 +246,13 @@ Core, rendering, IPC, kernel, panel manager. - **Cross-reference:** [D-60](tooling.md#d-60-no-network-on-default-launch-path), `POLICY.md`. - **Raised by:** 2026-05-03 policy-to-decision migration (T-28). +### D-68: Dual integration surface — Bash CLI primary, MCP secondary +- **Date:** 2026-05-15 +- **Decision:** clide exposes two integration surfaces over the same in-process `DaemonDispatcher`. **(1) Bash CLI over Unix socket — primary.** Per [D-1](#d-1-cli-first-not-mcp) and [D-56](#d-56-dissolve-daemon-process-flutter-app-hosts-ipc-server), a thin C client (`clide …`) connects to a per-user Unix socket served in-process by the Flutter app, exchanges JSON-lines, and exits. This is the surface Claude-Code-in-a-pane uses; it is also the surface for human shell use, scripts, and external editor integrations. Full action surface — `pane.*`, `files.*`, `editor.*`, `git.*`, `pql.*`, …. **(2) `/ide`-compatible MCP server — secondary.** clide additionally serves an MCP endpoint compatible with Claude Code's `/ide` integration (the same protocol VS Code and JetBrains plugins serve). Minimum tools: `mcp__ide__getDiagnostics`, `mcp__ide__executeCode`. Optional `mcp__clide__*` namespace exposing high-leverage clide tools is deferred to [Q-32](../questions/architecture.md#q-32-mcp-tool-surface-minimum-slash-ide-or-extended-clide-tools). Transport choice deferred to [Q-33](../questions/architecture.md#q-33-mcp-transport-sse-websocket-stdio-or-all). The MCP server wraps the *same* `DaemonDispatcher`; there is no second source of truth. +- **Context:** [D-1](#d-1-cli-first-not-mcp) chose CLI-first over MCP-only because MCP alone doesn't cover the action surface clide needs — Claude Code's `/ide` MCP exposes only two narrow tools (`getDiagnostics`, `executeCode`), enough for Claude to read diagnostics and run Jupyter cells but not enough to *drive* an IDE. The CLI surface gives full reach. But for users who run Claude Code *outside* clide and connect via `/ide`, MCP is the only path Claude Code knows; not serving it means clide is invisible to that workflow. The two surfaces are complementary, not alternatives. Reinforced by the [2026-05-14 consultant review](../../consultants.md): the architect flagged the absent socket server as the most critical drift; user confirmed the socket server (D-56 path a) plus an MCP companion. +- **Rationale:** Both surfaces wrap the same dispatcher, so neither becomes a second source of truth. CLI remains the contract user/Claude parity ([D-6](#d-6-cli-and-event-surface-contract)) is enforced against. MCP is added because the `/ide` ecosystem is real and growing — VS Code, JetBrains, Cursor, Windsurf all serve compatible MCP — and clide should be a peer there. The implementation cost is a protocol adapter + tool definitions, not duplicate business logic. +- **Cost:** Two transports to maintain. Mitigated by both wrapping the same dispatcher: the MCP adapter is the only thing that has to track `/ide` protocol evolution. If `mcp__clide__*` tools are added (pending Q-32), surface bloat is the obvious risk — every CLI verb invites an MCP twin; resist by default, justify on user need. +- **Cross-reference:** [D-1](#d-1-cli-first-not-mcp) (amended — see amendment line there), [D-6](#d-6-cli-and-event-surface-contract), [D-56](#d-56-dissolve-daemon-process-flutter-app-hosts-ipc-server), [Q-32](../questions/architecture.md#q-32-mcp-tool-surface-minimum-slash-ide-or-extended-clide-tools), [Q-33](../questions/architecture.md#q-33-mcp-transport-sse-websocket-stdio-or-all). +- **Raised by:** 2026-05-15 — consultant review (`consultants.md`) flagged the absent socket server (D-56 unimplemented) as the highest architectural drift; user chose option (a) "implement the server" and asked for MCP coverage alongside. + --- diff --git a/lib/src/pty/errors.dart b/lib/src/pty/errors.dart index 2cd2bc01..2c07f96d 100644 --- a/lib/src/pty/errors.dart +++ b/lib/src/pty/errors.dart @@ -5,9 +5,9 @@ /// core library stays Flutter-free per D-005. library; -/// A PTY operation failed. [op] identifies the step (`forkpty`, -/// `read`, `ioctl`, etc.); [errno] is POSIX errno when the failure -/// came from a syscall, otherwise `null`. +/// A PTY operation failed. [op] identifies the step (`posix_openpt`, +/// `posix_spawn`, `read`, `ioctl`, etc.); [errno] is POSIX errno when +/// the failure came from a syscall, otherwise `null`. class PtyException implements Exception { const PtyException(this.op, this.message, {this.errno}); diff --git a/lib/src/pty/native_pty.dart b/lib/src/pty/native_pty.dart index 586bd6b2..3f906a90 100644 --- a/lib/src/pty/native_pty.dart +++ b/lib/src/pty/native_pty.dart @@ -1,11 +1,19 @@ -/// Native PTY via forkpty(). +/// Native PTY via posix_openpt() + posix_spawn(). /// -/// Uses Dart FFI to call forkpty() directly. The master fd stays -/// in-process. The reader isolate uses poll() for clean shutdown. +/// Originally used `forkpty()`, which calls `fork()` underneath. `fork()` +/// in a multithreaded process is unsafe: only the calling thread survives +/// in the child, but libc locks (notably `malloc`) held by other threads +/// remain "locked forever." With the multi-threaded Dart VM as the +/// parent, ~5% of spawns deadlocked in the child before `execve` (see +/// T-96). /// -/// Based on the pty-spike proof-of-concept. Platform-aware: -/// macOS: forkpty in libSystem (DynamicLibrary.process) -/// Linux: forkpty in libutil.so.1 +/// `posix_spawn()` uses `vfork()` on glibc/musl/macOS, which keeps the +/// parent suspended until `execve` completes — no Dart code runs in the +/// child, so the lock-deadlock window is closed. The pty is created via +/// the POSIX-standard `posix_openpt` / `grantpt` / `unlockpt` / +/// `ptsname` sequence instead of the BSD `forkpty` wrapper. +/// +/// All symbols live in libc (resolved via `DynamicLibrary.process()`). library; import 'dart:async'; @@ -43,38 +51,62 @@ final class _Pollfd extends ffi.Struct { // -- FFI bindings ----------------------------------------------------------- -final ffi.DynamicLibrary _dl = _openLib(); +final ffi.DynamicLibrary _dl = ffi.DynamicLibrary.process(); -ffi.DynamicLibrary _openLib() { - if (Platform.isMacOS) return ffi.DynamicLibrary.process(); - // Linux: forkpty lives in libutil - return ffi.DynamicLibrary.open('libutil.so.1'); -} +// pty open/setup (POSIX). +final _posixOpenpt = _dl.lookupFunction('posix_openpt'); +final _grantpt = _dl.lookupFunction('grantpt'); +final _unlockpt = _dl.lookupFunction('unlockpt'); +final _ptsname = _dl.lookupFunction Function(ffi.Int32), ffi.Pointer Function(int)>('ptsname'); -final _forkpty = _dl.lookupFunction, ffi.Pointer, ffi.Pointer, ffi.Pointer<_Winsize>), - int Function(ffi.Pointer, ffi.Pointer, ffi.Pointer, ffi.Pointer<_Winsize>)>('forkpty'); +// posix_spawn family. The attr + file_actions structs are opaque to us +// and platform-sized — we allocate a generous fixed buffer (8 KiB, far +// larger than any documented platform layout) and pass it as Pointer. +// init() writes the real layout into our memory; destroy() releases any +// internal nested allocations. +final _posixSpawn = _dl.lookupFunction< + ffi.Int32 Function(ffi.Pointer, ffi.Pointer, ffi.Pointer, ffi.Pointer, ffi.Pointer>, + ffi.Pointer>), + int Function(ffi.Pointer, ffi.Pointer, ffi.Pointer, ffi.Pointer, ffi.Pointer>, + ffi.Pointer>)>('posix_spawn'); -final _execve = _dl.lookupFunction, ffi.Pointer>, ffi.Pointer>), - int Function(ffi.Pointer, ffi.Pointer>, ffi.Pointer>)>('execve'); +final _spawnattrInit = _dl.lookupFunction), int Function(ffi.Pointer)>('posix_spawnattr_init'); +final _spawnattrDestroy = _dl.lookupFunction), int Function(ffi.Pointer)>('posix_spawnattr_destroy'); +final _spawnattrSetflags = + _dl.lookupFunction, ffi.Int16), int Function(ffi.Pointer, int)>('posix_spawnattr_setflags'); -final _nativeWrite = ffi.DynamicLibrary.process() - .lookupFunction, ffi.IntPtr), int Function(int, ffi.Pointer, int)>('write'); +final _faInit = _dl.lookupFunction), int Function(ffi.Pointer)>('posix_spawn_file_actions_init'); +final _faDestroy = _dl.lookupFunction), int Function(ffi.Pointer)>('posix_spawn_file_actions_destroy'); +final _faAddopen = _dl.lookupFunction, ffi.Int32, ffi.Pointer, ffi.Int32, ffi.Uint32), + int Function(ffi.Pointer, int, ffi.Pointer, int, int)>('posix_spawn_file_actions_addopen'); +final _faAdddup2 = _dl.lookupFunction, ffi.Int32, ffi.Int32), int Function(ffi.Pointer, int, int)>( + 'posix_spawn_file_actions_adddup2'); +final _faAddclose = + _dl.lookupFunction, ffi.Int32), int Function(ffi.Pointer, int)>('posix_spawn_file_actions_addclose'); +// glibc 2.29+ / macOS 10.15+. Both ship the `_np` suffix. +final _faAddchdir = _dl.lookupFunction, ffi.Pointer), int Function(ffi.Pointer, ffi.Pointer)>( + 'posix_spawn_file_actions_addchdir_np'); -final _nativeClose = ffi.DynamicLibrary.process().lookupFunction('close'); - -final _ioctl = ffi.DynamicLibrary.process() - .lookupFunction), int Function(int, int, ffi.Pointer<_Winsize>)>('ioctl'); - -final _nativeKill = ffi.DynamicLibrary.process().lookupFunction('kill'); - -final _waitpid = ffi.DynamicLibrary.process() - .lookupFunction, ffi.Int32), int Function(int, ffi.Pointer, int)>('waitpid'); - -final _chdir = ffi.DynamicLibrary.process().lookupFunction), int Function(ffi.Pointer)>('chdir'); - -final _exit_ = ffi.DynamicLibrary.process().lookupFunction('_exit'); +// libc primitives shared with the reader isolate / lifecycle. +final _nativeWrite = + _dl.lookupFunction, ffi.IntPtr), int Function(int, ffi.Pointer, int)>('write'); +final _nativeClose = _dl.lookupFunction('close'); +final _ioctl = + _dl.lookupFunction), int Function(int, int, ffi.Pointer<_Winsize>)>('ioctl'); +final _nativeKill = _dl.lookupFunction('kill'); +final _waitpid = + _dl.lookupFunction, ffi.Int32), int Function(int, ffi.Pointer, int)>('waitpid'); +// Constants — all duplicated from , , . final int _kTiocsWinsz = Platform.isMacOS ? 0x80087467 : 0x5414; +const int _kORdwr = 0x0002; +final int _kONoctty = Platform.isMacOS ? 0x20000 : 0x0100; +// POSIX_SPAWN_SETSID — glibc 2.26+ (0x80), macOS 10.15+ (0x400). +final int _kSpawnSetsid = Platform.isMacOS ? 0x400 : 0x80; +// Opaque struct buffer size: ample headroom above every documented +// platform layout (glibc posix_spawnattr_t is 336 B; macOS even smaller). +const int _kSpawnStructBytes = 8192; + const _kSighup = 1; const _kWnohang = 1; @@ -104,7 +136,11 @@ class NativePty { /// Spawn a new PTY running [executable] with [arguments]. /// /// [environment] must be the complete environment — it goes straight - /// to execve's envp. Merge Platform.environment before calling. + /// to the spawn's envp. Merge `Platform.environment` before calling. + /// + /// Uses `posix_openpt` + `posix_spawn` (via libc's `vfork`-backed + /// implementation) so no Dart code runs between fork and execve — + /// see the library docstring and T-96. static NativePty start({ required String executable, List arguments = const ['-l'], @@ -113,7 +149,9 @@ class NativePty { String? workingDirectory, Map environment = const {}, }) { - // Resolve bare command names via PATH (execve doesn't search PATH). + // Resolve bare command names via PATH (posix_spawn requires an absolute + // or relative path — posix_spawnp would search PATH for us but we want + // resolution to be visible/debuggable from Dart). if (!executable.contains('/')) { final path = environment['PATH'] ?? Platform.environment['PATH'] ?? ''; for (final dir in path.split(':')) { @@ -126,106 +164,136 @@ class NativePty { } } - // Force-resolve FFI functions that run in the child process. - // Top-level finals are lazy; touching them here ensures the FFI - // trampolines are compiled before fork() clones the process. - final execve = _execve; - final chdir = _chdir; - final exit = _exit_; - final writeFn = _nativeWrite; - - // Pre-allocate error envelopes the child will write to its stdout - // (slave PTY → parent's master fd) before _exit, so the parent's - // reader sees a real diagnostic instead of an indistinguishable EOF. - final chdirErr = 'clide: chdir failed: $workingDirectory\n'.toNativeUtf8(allocator: malloc); - final chdirErrLen = chdirErr.length; - final execveErr = 'clide: exec failed: $executable\n'.toNativeUtf8(allocator: malloc); - final execveErrLen = execveErr.length; - - // Allocate ALL native memory before fork. - final shellN = executable.toNativeUtf8(allocator: malloc).cast(); + // ---- Open the pty master ------------------------------------------ + final masterFd = _posixOpenpt(_kORdwr | _kONoctty); + if (masterFd < 0) { + throw PtyException('posix_openpt', 'posix_openpt failed', errno: libc.errno); + } + if (_grantpt(masterFd) != 0) { + final err = libc.errno; + _nativeClose(masterFd); + throw PtyException('grantpt', 'grantpt failed', errno: err); + } + if (_unlockpt(masterFd) != 0) { + final err = libc.errno; + _nativeClose(masterFd); + throw PtyException('unlockpt', 'unlockpt failed', errno: err); + } + final slavePtr = _ptsname(masterFd); + if (slavePtr == ffi.nullptr) { + _nativeClose(masterFd); + throw PtyException('ptsname', 'ptsname returned null'); + } + // ptsname returns a pointer into a static (or thread-local) libc + // buffer; copy to a Dart-owned native string before any other libc + // call that might overwrite it. + final slavePath = slavePtr.toDartString().toNativeUtf8(allocator: malloc); + // ---- Marshal argv + envp ----------------------------------------- + final exeN = executable.toNativeUtf8(allocator: malloc); final allArgs = [executable, ...arguments]; - final argvN = malloc>(allArgs.length + 1); + final argvN = malloc>(allArgs.length + 1); for (var i = 0; i < allArgs.length; i++) { - argvN[i] = allArgs[i].toNativeUtf8(allocator: malloc).cast(); + argvN[i] = allArgs[i].toNativeUtf8(allocator: malloc); } argvN[allArgs.length] = ffi.nullptr; final envList = environment.entries.toList(); - final envpN = malloc>(envList.length + 1); + final envpN = malloc>(envList.length + 1); for (var i = 0; i < envList.length; i++) { - envpN[i] = '${envList[i].key}=${envList[i].value}'.toNativeUtf8(allocator: malloc).cast(); + envpN[i] = '${envList[i].key}=${envList[i].value}'.toNativeUtf8(allocator: malloc); } envpN[envList.length] = ffi.nullptr; - final wdN = (workingDirectory ?? '/').toNativeUtf8(allocator: malloc).cast(); - final fdOut = calloc(); + final wdN = workingDirectory == null ? ffi.nullptr : workingDirectory.toNativeUtf8(allocator: malloc); + + // ---- Build file_actions ------------------------------------------ + // Allocate as Uint8 so calloc treats it as a byte buffer; cast to + // Pointer when handing off to the FFI calls. + final fa = calloc(_kSpawnStructBytes).cast(); + final attr = calloc(_kSpawnStructBytes).cast(); + final pidOut = calloc(); + + void freeAllInputs() { + malloc.free(exeN); + for (var i = 0; i < allArgs.length; i++) { + malloc.free(argvN[i]); + } + malloc.free(argvN); + for (var i = 0; i < envList.length; i++) { + malloc.free(envpN[i]); + } + malloc.free(envpN); + if (wdN != ffi.nullptr) malloc.free(wdN); + malloc.free(slavePath); + calloc.free(fa); + calloc.free(attr); + calloc.free(pidOut); + } + + if (_faInit(fa) != 0) { + final err = libc.errno; + _nativeClose(masterFd); + freeAllInputs(); + throw PtyException('spawn_fa_init', 'posix_spawn_file_actions_init failed', errno: err); + } + if (_spawnattrInit(attr) != 0) { + final err = libc.errno; + _faDestroy(fa); + _nativeClose(masterFd); + freeAllInputs(); + throw PtyException('spawnattr_init', 'posix_spawnattr_init failed', errno: err); + } + + int rc = 0; + rc |= _spawnattrSetflags(attr, _kSpawnSetsid); + // Open the slave on fd 0 WITHOUT O_NOCTTY so it becomes the child's + // controlling tty (the child is a fresh session leader courtesy of + // POSIX_SPAWN_SETSID). + rc |= _faAddopen(fa, 0, slavePath, _kORdwr, 0); + rc |= _faAdddup2(fa, 0, 1); + rc |= _faAdddup2(fa, 0, 2); + // Don't leak the master fd into the child. + rc |= _faAddclose(fa, masterFd); + if (wdN != ffi.nullptr) { + rc |= _faAddchdir(fa, wdN); + } + if (rc != 0) { + _faDestroy(fa); + _spawnattrDestroy(attr); + _nativeClose(masterFd); + freeAllInputs(); + throw PtyException('spawn_fa_setup', 'failed to compose posix_spawn actions', errno: libc.errno); + } + + // ---- Spawn ------------------------------------------------------- + final spawnRc = _posixSpawn(pidOut, exeN, fa, attr, argvN, envpN); + final pid = pidOut.value; + + _faDestroy(fa); + _spawnattrDestroy(attr); + + if (spawnRc != 0) { + // posix_spawn returns the errno directly (does NOT set errno). + _nativeClose(masterFd); + freeAllInputs(); + throw PtyException('posix_spawn', 'posix_spawn failed', errno: spawnRc); + } + + // ---- Set initial winsize on the master --------------------------- final ws = calloc<_Winsize>() ..ref.wsRow = rows ..ref.wsCol = columns; + _ioctl(masterFd, _kTiocsWinsz, ws); + calloc.free(ws); - // Fork. - final pid = _forkpty(fdOut, ffi.nullptr, ffi.nullptr, ws); + freeAllInputs(); - if (pid == -1) { - // Capture errno BEFORE _freeAll — free() can clobber errno. - final err = libc.errno; - _freeAll(shellN, argvN, allArgs.length, envpN, envList.length, wdN, fdOut, ws); - malloc.free(chdirErr); - malloc.free(execveErr); - throw PtyException('forkpty', 'forkpty() failed', errno: err); - } - - if (pid == 0) { - // CHILD — only pre-resolved FFI calls, no Dart heap. - // After forkpty(), fd 1 is the slave PTY connected back to the - // parent's master fd, so write(1, ...) lands as readable output. - if (chdir(wdN) != 0) { - writeFn(1, chdirErr.cast(), chdirErrLen); - exit(1); - } - execve(shellN, argvN, envpN); - // execve only returns on failure. - writeFn(1, execveErr.cast(), execveErrLen); - exit(1); - } - - // PARENT - final fd = fdOut.value; - _freeAll(shellN, argvN, allArgs.length, envpN, envList.length, wdN, fdOut, ws); - malloc.free(chdirErr); - malloc.free(execveErr); - - final pty = NativePty._(fd, pid); + final pty = NativePty._(masterFd, pid); pty._spawnReader(); return pty; } - static void _freeAll( - ffi.Pointer shell, - ffi.Pointer> argv, - int argc, - ffi.Pointer> envp, - int envc, - ffi.Pointer wd, - ffi.Pointer fdOut, - ffi.Pointer ws, - ) { - malloc.free(shell); - for (var i = 0; i < argc; i++) { - malloc.free(argv[i]); - } - malloc.free(argv); - for (var i = 0; i < envc; i++) { - malloc.free(envp[i]); - } - malloc.free(envp); - malloc.free(wd); - calloc.free(fdOut); - calloc.free(ws); - } - // -- I/O ------------------------------------------------------------------ void _spawnReader() { @@ -236,6 +304,21 @@ class NativePty { final rp = ReceivePort(); _readerPort = rp; _readerExited = Completer(); + // Register the listener BEFORE spawning the isolate. ReceivePort buffers + // messages until a listener attaches, but registering first removes any + // ambiguity if the listen() call ever moves further away from spawn (and + // sidesteps a real race we saw 1-in-10 in CI where output never arrived). + rp.listen((msg) { + if (msg == null) { + if (!_out.isClosed) _out.close(); + rp.close(); + _readerPort = null; + if (!_readerExited!.isCompleted) _readerExited!.complete(); + _reap(); + } else { + if (!_out.isClosed) _out.add(msg as Uint8List); + } + }); try { _readerIsolate = await Isolate.spawn(_readLoop, (rp.sendPort, _fd)); } catch (e) { @@ -248,17 +331,6 @@ class NativePty { if (!_readerExited!.isCompleted) _readerExited!.complete(); return; } - rp.listen((msg) { - if (msg == null) { - if (!_out.isClosed) _out.close(); - rp.close(); - _readerPort = null; - if (!_readerExited!.isCompleted) _readerExited!.complete(); - _reap(); - } else { - if (!_out.isClosed) _out.add(msg as Uint8List); - } - }); } /// Isolate entry — polls then reads until EOF/error/fd-closed. diff --git a/lib/src/pty/pty.dart b/lib/src/pty/pty.dart index 8ac2e259..5061de0a 100644 --- a/lib/src/pty/pty.dart +++ b/lib/src/pty/pty.dart @@ -1,6 +1,7 @@ -/// PTY subsystem — spawn child processes under a PTY via forkpty(), -/// expose their master fd as a byte stream. Desktop IDE's pane model -/// (terminal / Claude / future tmux wrappers) rides on this. +/// PTY subsystem — spawn child processes under a PTY via posix_openpt() +/// + posix_spawn(), expose their master fd as a byte stream. Desktop +/// IDE's pane model (terminal / Claude / future tmux wrappers) rides on +/// this. library; export 'env.dart' show clidePtyEnvDefaults, mergePtyEnv; diff --git a/test/panes/registry_test.dart b/test/panes/registry_test.dart index 354c3b6d..0f958351 100644 --- a/test/panes/registry_test.dart +++ b/test/panes/registry_test.dart @@ -44,20 +44,19 @@ void main() { test('output events base64-encode the child bytes', tags: ['forkpty'], () async { await registry.spawn( kind: PaneKind.terminal, - argv: const ['/bin/echo', 'hello-panes'], + // Child writes then lingers so the reader's poll has a wide + // window to see POLLIN before HUP. + argv: const ['/bin/sh', '-c', 'printf hello-panes; sleep 0.25'], ); - // /bin/echo closes its pty quickly. Wait briefly for output + - // the resulting pane.exit event to settle. - for (var i = 0; i < 30; i++) { - if (sink.ofKind('pane.output').isNotEmpty && sink.ofKind('pane.exit').isNotEmpty) break; - await Future.delayed(const Duration(milliseconds: 100)); + final deadline = DateTime.now().add(const Duration(seconds: 2)); + String decoded() => sink.ofKind('pane.output').map((e) => utf8.decode(base64Decode(e.data['bytes_b64']! as String))).join(); + while (!decoded().contains('hello-panes') && DateTime.now().isBefore(deadline)) { + await Future.delayed(const Duration(milliseconds: 25)); } - final out = sink.ofKind('pane.output').toList(); - expect(out, isNotEmpty); - final decoded = out.map((e) => utf8.decode(base64Decode(e.data['bytes_b64']! as String))).join(); - expect(decoded, contains('hello-panes')); + expect(sink.ofKind('pane.output'), isNotEmpty); + expect(decoded(), contains('hello-panes')); }); test('write + resize emit no spurious events, update state', () async { diff --git a/test/pty/session_test.dart b/test/pty/session_test.dart index b787c577..8aef4ada 100644 --- a/test/pty/session_test.dart +++ b/test/pty/session_test.dart @@ -1,19 +1,21 @@ /// NativePty smoke tests. /// -/// Exercises forkpty() end-to-end: spawn → child output through the +/// Exercises posix_spawn() end-to-end: spawn → child output through the /// reader isolate. Linux + macOS only; skipped elsewhere. /// -/// Tagged `forkpty` — must run via `dart test`, not `flutter test`. -/// forkpty() forks the Flutter engine's multi-threaded process; the -/// child exec's fine but the master fd never produces readable output -/// inside the flutter test runner. -@Tags(['forkpty']) +/// Per-test `tags: ['forkpty']` marks the tests that need `dart test` +/// rather than the flutter test runner — currently just the +/// write/read-back bidirectional test (writes to the master fd never +/// reach the child under the flutter test runner; reads work fine). +/// Everything else runs under `flutter test` and contributes to +/// coverage. library; import 'dart:async'; import 'dart:convert'; import 'dart:io'; +import 'package:clide/src/pty/errors.dart'; import 'package:clide/src/pty/native_pty.dart'; import 'package:test/test.dart'; @@ -48,7 +50,7 @@ void main() { expect(buf.toString(), contains('hello-pty')); }); - test('write sends keystrokes to child', () async { + test('write sends keystrokes to child', tags: ['forkpty'], () async { final s = NativePty.start( executable: '/bin/sh', arguments: [], @@ -122,55 +124,44 @@ void main() { expect(buf.toString(), contains('path-resolution-ok')); }); - test('non-existent workingDirectory produces the chdir-failed diagnostic', () async { - // chdir() fails in the child → writes diagnostic + _exit(1). - final s = NativePty.start( - executable: '/bin/sh', - arguments: ['-c', 'echo should-not-run'], - columns: 80, - rows: 24, - workingDirectory: '/tmp/clide-no-such-dir-${DateTime.now().microsecondsSinceEpoch}', - environment: { - ...Platform.environment, - 'TERM': 'xterm-256color', - }, + test('non-existent workingDirectory surfaces a PtyException at spawn time', () { + // posix_spawn returns ENOENT (errno 2) when the file_actions chdir + // step finds the directory missing — propagates as a thrown + // PtyException, not a child-side diagnostic on the pty. + expect( + () => NativePty.start( + executable: '/bin/sh', + arguments: ['-c', 'echo should-not-run'], + columns: 80, + rows: 24, + workingDirectory: '/tmp/clide-no-such-dir-${DateTime.now().microsecondsSinceEpoch}', + environment: { + ...Platform.environment, + 'TERM': 'xterm-256color', + }, + ), + throwsA(isA().having((e) => e.errno, 'errno', 2)), ); - addTearDown(s.close); - final buf = StringBuffer(); - final done = Completer(); - s.output.listen( - (b) => buf.write(utf8.decode(b, allowMalformed: true)), - onDone: () { - if (!done.isCompleted) done.complete(); - }, - ); - await done.future.timeout(const Duration(seconds: 5), onTimeout: () {}); - expect(buf.toString(), contains('chdir failed')); }); - test('non-existent executable produces the exec-failed diagnostic', () async { - final s = NativePty.start( - executable: '/tmp/clide-no-such-binary-${DateTime.now().microsecondsSinceEpoch}', - arguments: const [], - columns: 80, - rows: 24, - workingDirectory: '/', - environment: { - ...Platform.environment, - 'TERM': 'xterm-256color', - }, + test('non-existent executable surfaces a PtyException at spawn time', () { + // posix_spawn surfaces exec-time errors as a non-zero return on + // glibc (which uses vfork — the child is suspended until execve + // either succeeds or fails). ENOENT (errno 2) for missing binary. + expect( + () => NativePty.start( + executable: '/tmp/clide-no-such-binary-${DateTime.now().microsecondsSinceEpoch}', + arguments: const [], + columns: 80, + rows: 24, + workingDirectory: '/', + environment: { + ...Platform.environment, + 'TERM': 'xterm-256color', + }, + ), + throwsA(isA().having((e) => e.errno, 'errno', 2)), ); - addTearDown(s.close); - final buf = StringBuffer(); - final done = Completer(); - s.output.listen( - (b) => buf.write(utf8.decode(b, allowMalformed: true)), - onDone: () { - if (!done.isCompleted) done.complete(); - }, - ); - await done.future.timeout(const Duration(seconds: 5), onTimeout: () {}); - expect(buf.toString(), contains('exec failed')); }); test('resize on a live PTY does not throw', () async {