Skip to content

feat: harden into a type-safe, tested, CI-gated extension - #1

Open
mio-g wants to merge 1 commit into
liushihao456:mainfrom
mio-g:harden/pi-parallel-sessions
Open

feat: harden into a type-safe, tested, CI-gated extension#1
mio-g wants to merge 1 commit into
liushihao456:mainfrom
mio-g:harden/pi-parallel-sessions

Conversation

@mio-g

@mio-g mio-g commented Sep 1, 2026

Copy link
Copy Markdown

Hi — thanks for building this. It is the only pi multi-session tool that runs in-process, which makes it the only one that works in a plain terminal without tmux/Zellij/Ghostty. I have been using it daily and wanted to make it something I could depend on, so this PR adds the engineering scaffolding around the existing design.

No feature changes and no rewrite. The architecture and LockManager design are kept as-is; file names and the public surface are unchanged.

Blast radius: 🟡 Medium

  • Behavior change visible to users: inferToolPaths now recognizes mutating git, npm/pnpm/yarn/bun, make, cargo, terraform, and dbt commands, so more tool calls take path locks than before
  • peerDependencies narrows from * to >=0.84.4, and engines.node is declared as >=22.19.0 (mirroring pi's own) — installs on older hosts now warn instead of failing at runtime
  • Scope: whole package, but no cross-repo contract; runtime dependencies remain zero (all new tooling is devDependencies)

What this fixes

Silent drift against pi 0.84.4

The extension monkey-patched InteractiveMode.prototype.setWorkingIndicator. That method no longer exists in pi 0.84.4, and the patch was guarded, so it silently no-oped — the working/idle indicator in the switcher was already dead. It is now driven from the agent_start/agent_end events the extension already subscribes to.

It also dynamically imported pi's compiled internals (getPackageDir()/dist/core/model-resolver.js). That is replaced with the public resolveModelScopeWithDiagnostics export. A startup preflight now checks every symbol the package consumes and, on mismatch, warns via ctx.ui.notify naming the missing symbol instead of registering half-working behavior.

Lock coverage gap

The mutator whitelist did not include git, so two sessions running git checkout / reset --hard in the same repo were completely unguarded. Same for package installs, make, cargo, terraform, and dbt. Two related fixes: same-session acquisitions are reference-counted (one tool call finishing no longer releases a path another still holds), and commands are tokenized so sed -i 's|/usr/bin|/opt|' no longer locks the delimiter contents as paths.

Three bugs found while smoke-testing

  • Child runtimes get a fresh resource loader, which dropped this extension entirely in sessions opened outside the parent cwd — Ctrl-R stopped working after switching folders
  • The parent row was labelled with its cwd basename, making the protected row indistinguishable from a child
  • Killing the active child deadlocked, because the command awaited disposal of the runtime executing it

Type safety and validation

  • @ts-nocheck removed from both files; tsc --noEmit passes
  • All 69 any occurrences replaced with pi's exported types
  • Unit tests for LockManager, inferToolPaths, the compatibility preflight, and the widget
  • CI runs typecheck + tests on push and PR (publishing stays in its own workflow)
  • The 12 empty catch {} blocks now either report through a debug channel or carry a rationale

Notes

  • The kill key is configurable via ~/.pi/agent/pi-sessions.json ({"killKey": "ctrl+x"}), since some terminals — kitty, for one — consume Ctrl-K. Default is unchanged.
  • The README now states the real safety model: locks are a per-tool-call race guard, not isolation, and are process-local. That felt worth being explicit about.

Happy to split this into smaller PRs, drop any part, or adjust the conventions to match your preferences — just say the word.

The package worked but had no type checking, no tests, no CI gate, and
several silent-failure paths. This hardens it without changing its scope
or architecture.

Type safety:
- Remove `@ts-nocheck` from index.ts and ui.ts; both now pass `tsc --noEmit`
- Replace all 69 `any` occurrences with pi's exported types
- Add a pinned dev toolchain and strict tsconfig

Host compatibility:
- Narrow peerDependencies from `*` to `>=0.84.4`, and declare Node >=22.19.0
- Drop the dynamic import of pi's private `dist/core/model-resolver.js` in
  favour of the exported `resolveModelScopeWithDiagnostics`
- Add a startup preflight over every consumed export; on mismatch the
  extension warns via `ctx.ui.notify` naming the missing symbol instead of
  registering half-working behavior

Session activity:
- Drop the monkey-patch of `InteractiveMode.prototype.setWorkingIndicator`,
  which no longer exists in pi 0.84.4 and had been silently no-oping, so the
  working/idle indicator was dead. Drive it from `agent_start`/`agent_end`

Path locks:
- Recognize mutating `git`, npm/pnpm/yarn/bun, `make`, `cargo`, `terraform`,
  and `dbt` commands, which the previous regex whitelist missed entirely —
  two sessions running `git checkout` in one repo were unguarded
- Reference-count same-session acquisitions so one tool call finishing does
  not release a path another tool call still holds
- Tokenize shell commands so `sed -i 's|/usr/bin|/opt|'` no longer locks the
  delimiter contents as paths

Correctness fixes found while testing:
- Child runtimes get a fresh resource loader, which dropped this extension
  in sessions opened outside the parent cwd; re-register it explicitly
- Label the parent row `parent` rather than its cwd basename, which made the
  protected row indistinguishable from a child
- Killing the active child deadlocked awaiting disposal of the runtime
  executing the command; hand the terminal back first, then dispose

Also: no empty `catch {}` blocks remain (each reports via a debug channel or
carries a rationale), the kill key is configurable via
`~/.pi/agent/pi-sessions.json`, and the README states the real safety model —
locks are a per-tool-call race guard, not isolation, and are process-local.

Runtime dependencies remain zero.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant