fix(pi-plugin): support pi-web multi-session and RPC hosts - #350
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…amer registry, session-scoped drains, presenter ctx capture; NOT a security blocker — rpc-server untouched) Co-Authored-By: Alfonso <alfonso@cortexkit.io>
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
alfonso-magic-context
left a comment
alfonso-magic-context
left a comment
There was a problem hiding this comment.
Thanks for this — especially as a first contribution. The diagnosis is right: the #247 process-global latch is what makes the second pi-web session skip Magic Context entirely, and routing child suppression through subagents:child:session-created/disposed plus AsyncLocalStorage is the correct seam. Once-per-process startup maintenance, not reusing a non-Pi argv[1], Dreamer sibling ownership, and keeping ctx-status entries model-invisible while presenting them in RPC are all the right instincts. And noted that you already pushed the Dreamer owner-handoff stabilization mid-review — that resolves one of the items we had flagged, and that kind of responsiveness makes this easy to shepherd.
Two clarifications so we don't talk past each other:
- "RPC hosts" here is Pi RPC mode (
ctx.ui.notify/ctx.ui.custom). It does not change Magic Context's RPC server, which must stay on127.0.0.1with a bearer token. We checked; this PR does not touch that. - The old "second init in the process is a no-op" test should change — that contract is the bug for pi-web. Please keep the child-only skip test (you did).
Before we can merge:
- Dreamer
registeredProjectsonglobalThis(same jitimoduleCache:falsereason as the child marker), so two sessions in one repo don't start two timers. session_shutdowndraining only that session's in-flight work — in pi-web, shutdown is not process exit.- RPC presentation using the command's live
ctx, not asession_startclosure. - The #177 "never spawn bare
pi" test kept alongside the new embedded-host test. packages/pi-plugin/PARITY.mdupdated for RPC dialogs, the multi-session process model, and the latch → ALS change.
We've approved CI for this PR so your next push gets the full check suite. Really solid work — happy to re-review quickly.
There was a problem hiding this comment.
All reported issues were addressed across 21 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
870ff86 to
abc835f
Compare
|
Rebased onto the latest master ( |
Self-review against the upstream six-axis standard (calibrated on the PR cortexkit#350 review) found and fixes: - dreamer project registry moves to a globalThis Symbol.for holder so jiti moduleCache:false re-imports share one timer per project (duplicate-timer class, cortexkit#350-review must-fix 1); runtimeKeys ref-counting now actually spans module instances (+ test) - session_shutdown drains historian/recomp scoped to the shutting-down sessionId (awaitInFlightHistoriansFor/awaitInFlightRecompsFor), keeping process-wide variants as fallback; comments no longer assume shutdown == process exit; dream drain stays process-wide (project- scoped work, bounded wait) - PARITY.md §7a documents the embedded host model, adapter trust tier, init-gating split, and the known in-process-child limitation (F1); §13 points at scoped drains Known limitations documented rather than fixed: embedded-mode third- party in-process children still fully initialize (needs lifecycle-event marking, coordinate with upstream PR cortexkit#350). Test-depth gaps G1/G2 recorded in the self-review report (.cortexkit/alfonso/task-outputs/, untracked). Verification: pi-plugin typecheck clean, 809/809 tests passing.
alfonso-magic-context
left a comment
alfonso-magic-context
left a comment
There was a problem hiding this comment.
Thanks for the rework — the quality jump since the last round is real, and most of it verified clean under a full re-review (all five prior items confirmed addressed at source; ALS suppression held up under a four-concurrent-child probe with an async hop; both suites green on the PR merged onto current master; no public adapter surface rides in, which matters because the API question is deliberately deferred to #353).
Four items block the merge, two of them reproduced by probe rather than read off the diff:
-
Stale sibling-worktree dreamer timer.
registerPiDreamerProjectkeeps the old owner in the shared owners map when a different owner re-registers the same project identity from another directory (src/dreamer/index.ts:157-185), and the retired timer's predicate checks only that old owner's entry and directory (:190-194) — not the current registration generation/activeOwner. A probe with owner A/worktree A then owner B/worktree B showed the old A client still prompts. The committed test (dreamer/index.test.ts:428-470) covers one owner changing directories and misses this. Bind scheduled clients to the current registration generation (or activeOwner+projectDir) and add the sibling-owner regression. -
Shutdown timeout abandons rather than cancels.
src/index.ts:2337-2373stops waiting after 5s whiletimeout.ts:1-14leaves the underlying promise alive; detached recomp/upgrade retainsctx(commands/ctx-recomp.ts:177-278) andpi-recomp-runner.ts:69-84callsonStatusChangefromfinallywithout a stale-context guard. Pi 0.83 invalidates command contexts after disposal, so long recomp work can outlive shutdown and throw on dead ctx/UI. Capture immutable session data up front, fence or cancel on shutdown, and guard post-await presentation. -
Native Pi 0.83 RPC dialogs are silently vacuous.
pi-command-utils.ts:180-190assumesctx.ui.customrenders or rejects, but Pi 0.83 rpc-mode implements it asPromise.resolve(undefined)— the catch fallback never runs, so native/external RPC hosts lose the detailed results entirely. It works in your host because pi-web supplies its own custom UI. Add a capability check with a portable notification fallback (and a test against the real 0.83 shape, not a functional fake), or scope the PARITY.md claim explicitly to hosts that provideui.custom. -
Missing #247 storm regression. The process-global latch you removed originally existed for the four-child in-process init storm. Our probe of your ALS scoping passed it — the implementation looks right — but no committed test reproduces the storm. Please add the parallel multi-child regression asserting every child registers no tools/events/background scans.
Nothing else stands between this and merge — the internals-first direction is settled on our side, and #353 tracks the public API question separately.
abc835f to
329dddf
Compare
|
Thanks for the detailed review. I addressed the four remaining items, rebased the branch onto
Additional points from the earlier review remain covered:
After rebasing onto The new CI and Smoke runs are currently waiting for workflow approval. Cubic reports that it skipped automatic review because the force-push rewrote the branch history, so it will need to be triggered manually. |
Scope in-process child detection to lifecycle AsyncLocalStorage so independent sessions initialize normally, and release lifecycle subscriptions on shutdown. Run startup maintenance once per process and defer session-history reads until the backfill lease is acquired. Resolve the child Pi CLI independently from embedded host argv, present command output through RPC notifications and dialogs, and ensure Dreamer registration before manual runs.
Handle multiple Pi sessions running in the same embedded host. - share Dreamer registration across plugin instances and hand its timer to the most recently registered remaining session - wait only for each session's historian, recomp, and Dreamer jobs during session_shutdown - keep agent_end synchronous so it does not delay turn completion - use the current command context to display status in Pi RPC mode - keep standalone Pi and embedded-host subagent launch regression tests - document the behavior in PARITY.md Tests: - NODE_ENV=test bun test src (813 passed) - NODE_ENV=test bun test session-project-backfill.test.ts (9 passed) - bun run build - bun run typecheck - bun run format:check
- reject Dreamer prompts after their registration owner is removed or switches worktrees - track complete manual runs, including domain lease waits, during shutdown - unregister the session owner before draining its Dreamer work - notify every registered owner after successful adjunct updates - add regression coverage for shutdown and multi-worktree races
- resolve manual runs through the active registration owner's options - reject ownerless and deregistered-owner requests - preserve process-shared argument order across extension reloads - cover refreshed owners, stale owners, and lease-wait draining
- fence Dreamer timers and late results across reloads and worktree handoffs - wait up to five seconds for recomp and upgrade tasks before canceling them - capture session state at command start and block late UI updates - fall back to notifications when RPC dialogs are unavailable - add regression coverage for concurrent child initialization
329dddf to
1425715
Compare
|
Reviewed the lifecycle rework (head 1425715) — strong pass. Three of the four requested changes are cleanly addressed and well-tested: the RPC dialog's factory-invocation tracking with the notify fallback, the AsyncLocalStorage child marker replacing the process-global latch (independent sessions now initialize normally — nice resolution of the #247 tension), and the fenced per-session shutdown drain with owner-scoped awaits. One item still open before this can merge, on blocker 1: the stale-timer fix fences the client by generation, but timer cleanup identity is still directory-only. In the A→B→A sequence, the first A registration's canceled timer sees the final registration also at directory A, skips Two smaller asks alongside: a shutdown integration test with two live sessions (retire A, assert B's historian/dreamer continue working), and a fresh CI run on the current head (the force-push left the checks neutral/empty). With those, this is mergeable — the rework's shape is exactly right, and the new owner/generation surfaces read well. |
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/plugin/dream-timer.test.ts">
<violation number="1" location="packages/plugin/src/plugin/dream-timer.test.ts:69">
P3: The first test mocks setInterval/clearInterval but leaves setTimeout real. startDreamScheduleTimer's second registration (after activeTimer is already set) takes the `else if (...) scheduleInitialProjectRun` path, which calls scheduleAfterBootQuiet → real setTimeout with a bootQuietRemainingMs delay (~30s). The test then asserts stale-cleanup behavior against a non-hermetic timer that is unref'd and merely cleared in the finally block. Mock setTimeout (as the second test does) so the code under test is deterministic and the real ~30s timers aren't created during the run.</violation>
<violation number="2" location="packages/plugin/src/plugin/dream-timer.test.ts:69">
P3: Both new tests call startDreamScheduleTimer, which internally calls the real openDatabase() with no dbPath (via openTimerDatabaseOrNull). That opens/creates the process-wide global context.db — getMagicContextStorageDir() → real data dir unless MAGIC_CONTEXT_TEST_DATA_DIR is set or NODE_ENV=test reaches the backstop — running migrations on shared real user storage. The tests create a mkdtemp dir but never point the DB there (no dbPath is passed; openDatabase has no dbPath parameter exposed here). Mock openDatabase or set MAGIC_CONTEXT_TEST_DATA_DIR so these tests are isolated and can't touch the real shared database.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| let cleanupReplacement: (() => void) | undefined; | ||
|
|
||
| try { | ||
| const cleanupStale = await startDreamScheduleTimer({ ...base }); |
There was a problem hiding this comment.
P3: The first test mocks setInterval/clearInterval but leaves setTimeout real. startDreamScheduleTimer's second registration (after activeTimer is already set) takes the else if (...) scheduleInitialProjectRun path, which calls scheduleAfterBootQuiet → real setTimeout with a bootQuietRemainingMs delay (~30s). The test then asserts stale-cleanup behavior against a non-hermetic timer that is unref'd and merely cleared in the finally block. Mock setTimeout (as the second test does) so the code under test is deterministic and the real ~30s timers aren't created during the run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/plugin/dream-timer.test.ts, line 69:
<comment>The first test mocks setInterval/clearInterval but leaves setTimeout real. startDreamScheduleTimer's second registration (after activeTimer is already set) takes the `else if (...) scheduleInitialProjectRun` path, which calls scheduleAfterBootQuiet → real setTimeout with a bootQuietRemainingMs delay (~30s). The test then asserts stale-cleanup behavior against a non-hermetic timer that is unref'd and merely cleared in the finally block. Mock setTimeout (as the second test does) so the code under test is deterministic and the real ~30s timers aren't created during the run.</comment>
<file context>
@@ -43,6 +44,111 @@ describe("schema-fence null-DB contract", () => {
+ let cleanupReplacement: (() => void) | undefined;
+
+ try {
+ const cleanupStale = await startDreamScheduleTimer({ ...base });
+ cleanupReplacement = await startDreamScheduleTimer({ ...base });
+ expect(cleanupStale).toBeFunction();
</file context>
| let cleanupReplacement: (() => void) | undefined; | ||
|
|
||
| try { | ||
| const cleanupStale = await startDreamScheduleTimer({ ...base }); |
There was a problem hiding this comment.
P3: Both new tests call startDreamScheduleTimer, which internally calls the real openDatabase() with no dbPath (via openTimerDatabaseOrNull). That opens/creates the process-wide global context.db — getMagicContextStorageDir() → real data dir unless MAGIC_CONTEXT_TEST_DATA_DIR is set or NODE_ENV=test reaches the backstop — running migrations on shared real user storage. The tests create a mkdtemp dir but never point the DB there (no dbPath is passed; openDatabase has no dbPath parameter exposed here). Mock openDatabase or set MAGIC_CONTEXT_TEST_DATA_DIR so these tests are isolated and can't touch the real shared database.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/plugin/dream-timer.test.ts, line 69:
<comment>Both new tests call startDreamScheduleTimer, which internally calls the real openDatabase() with no dbPath (via openTimerDatabaseOrNull). That opens/creates the process-wide global context.db — getMagicContextStorageDir() → real data dir unless MAGIC_CONTEXT_TEST_DATA_DIR is set or NODE_ENV=test reaches the backstop — running migrations on shared real user storage. The tests create a mkdtemp dir but never point the DB there (no dbPath is passed; openDatabase has no dbPath parameter exposed here). Mock openDatabase or set MAGIC_CONTEXT_TEST_DATA_DIR so these tests are isolated and can't touch the real shared database.</comment>
<file context>
@@ -43,6 +44,111 @@ describe("schema-fence null-DB contract", () => {
+ let cleanupReplacement: (() => void) | undefined;
+
+ try {
+ const cleanupStale = await startDreamScheduleTimer({ ...base });
+ cleanupReplacement = await startDreamScheduleTimer({ ...base });
+ expect(cleanupStale).toBeFunction();
</file context>
|
Verified the lifecycle round (0c0c029) — the remaining blocker is closed: cleanup now fences on generation identity (and the A→B→A test asserts the first timer's cleanup actually runs), and the two-live-session shutdown test covers the retire-A/B-continues path properly. The added dream-timer singleton lifecycle handling reads well too. I've approved the blocked CI workflows on your head; once they come back green this is mergeable from my side — it will land after the v0.40.1 patch currently in flight so your change rides a clean base. |
|
Final verification on the refreshed head (post master merge): Pi suite 881/0, plugin suite 4184/0, typecheck clean. All review rounds resolved — the generation-fenced timer cleanup closed the last blocker, and the master refresh integrates cleanly with this week's parity and cache work. Merging with the boundary we discussed intact: the multi-session host plumbing lands as internal wiring; the public adapter surface stays experimental until the pi-web contract settles. Thank you for the persistence across the review rounds — the A→B→A lifecycle test and the two-live-session shutdown coverage made this landable. |
3 participants
Summary
This PR adapts the Pi plugin for
pi-web, where multiple Pi sessions share one persistent Node.js process and commands run through Pi’s RPC mode.It addresses shared-process session isolation, duplicate startup work, unsafe subagent CLI detection, RPC command feedback, and
/ctx-dreamfailures before the first model turn.Changes
Session isolation
AsyncLocalStorage<boolean>marker.subagents:child:session-createdandsubagents:child:disposedevents.pi-webprocess to initialize normally.session_shutdownto prevent stale handlers after reloads.Process-wide startup maintenance
Safer subagent CLI detection
process.argv[1]only when it identifies a supported Pi CLI; otherwise use the packaged executable, bundled CLI, orPATHfallback as appropriate.RPC command presentation
ctx.ui.notify.ctx.ui.custom./ctx-status,/ctx-embed,/ctx-recomp,/ctx-session-upgrade, and/ctx-dream.Dreamer registration
/ctx-dreamrun./ctx-dreamto work before the firstbefore_agent_startevent.Verification
bun run --cwd packages/pi-plugin buildbun run --cwd packages/pi-plugin test— 804 passed, 0 failedbun test packages/plugin/src/features/magic-context/session-project-backfill.test.ts— 9 passed, 0 failedgit diff --checkRegression coverage includes:
/ctx-dreamregistration synchronizationNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Supports
packages/pi-pluginin pi-web multi-session and RPC hosts. Old behavior suppressed all same-process inits and drained all jobs on shutdown; new behavior suppresses only in-process child subagents, drains only the shutting-down session's historian/recomp and the active owner's Dreamer jobs (cancels after ~5s), routes command output through the live RPC UI, and isolates the shared Dreamer lifecycle across plugin instances and worktrees.New Features
AsyncLocalStorage, unregisters lifecycle listeners onsession_shutdown, and keepsagent_endsynchronous./ctx-status,/ctx-embed,/ctx-recomp,/ctx-session-upgrade,/ctx-wrapup,/ctx-flush, and/ctx-dream.process.argv[1]only when the host is the Pi CLI, preferring packaged/bundled binaries otherwise, and never spawning with a shell.Written for commit fd89658. Summary will update on new commits.
Greptile Summary
The PR adapts the Pi integration for multiple sessions sharing one RPC host process.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD P[Persistent Pi RPC host] --> S1[Session A extension] P --> S2[Session B extension] P --> C[In-process child session] C --> G[AsyncLocalStorage child guard] G -->|Child context| N[Suppress full initialization] S1 --> M[Shared process maintenance] S2 --> M M --> O[Run startup work once] S1 --> D[Shared Dreamer project registration] S2 --> D D --> T[Active owner timer] S1 --> J1[Session-scoped detached jobs] S2 --> J2[Session-scoped detached jobs] J1 --> U1[Session A RPC UI] J2 --> U2[Session B RPC UI]Reviews (10): Last reviewed commit: "Merge branch 'master' into fix/pi-web-co..." | Re-trigger Greptile
Context used (5)