mirror of
https://bitbucket.org/siakitem/my-pi.git
synced 2026-08-28 08:35:57 +00:00
243 lines
20 KiB
Markdown
243 lines
20 KiB
Markdown
---
|
||
issue: 302
|
||
issue_title: "Child subagent shutdown unpublishes the parent's global PermissionsService"
|
||
---
|
||
|
||
# Scope the global PermissionsService slot so a child cannot clobber the parent
|
||
|
||
## Problem Statement
|
||
|
||
Every extension instance — the top-level parent and each in-process subagent child — runs the same factory in `packages/pi-permission-system/src/index.ts`.
|
||
That factory unconditionally publishes the `PermissionsService` to a single process-global slot (`Symbol.for("@gotgenes/pi-permission-system:service")`) at init and deletes that slot on `session_shutdown`.
|
||
Two defects follow.
|
||
First, a child's init **overwrites** the parent's published service, so mid-run `getPermissionsService()` resolves the child's service (different runtime/config) rather than the parent's.
|
||
Second, the child's `session_shutdown` **deletes** the slot entirely, so after the first subagent finishes a still-live parent (and any third-party consumer) gets `undefined`.
|
||
|
||
The fix must keep `/reload` working: the slot is intentionally overwrite-safe for reload today, so whichever path we take cannot regress re-publication of a reloaded parent's service.
|
||
|
||
## Goals
|
||
|
||
- A live parent's `PermissionsService` survives an in-process child subagent's shutdown.
|
||
- Mid-run, `getPermissionsService()` resolves the parent's service, never a child's.
|
||
- Teardown removes only the slot the instance actually owns.
|
||
- `/reload` still re-publishes the reloaded parent's service (overwrite-safe), even if reload re-runs the factory.
|
||
- **Breaking:** `unpublishPermissionsService` gains a required `service` parameter (identity compare-and-delete).
|
||
The package's only public export is `service.ts`, so this is a public API change → `feat!:`.
|
||
|
||
## Non-Goals
|
||
|
||
- No change to process-based subagent extensions (env-hint / filesystem detection).
|
||
They run in their own OS process with their own `globalThis`; each still publishes normally.
|
||
- No change to the event-bus RPC fallback (`permissions:rpc:check` / `:prompt`).
|
||
Those handlers stay registered at init and remain available at load.
|
||
- No change to the `SubagentSessionRegistry` storage, keying, or lifecycle-event subscription (#296, #298 already settled those).
|
||
- No change to the `permissions:decision` event or the gate/forwarding logic.
|
||
|
||
## Background
|
||
|
||
Relevant modules:
|
||
|
||
- `src/index.ts` — the composition root.
|
||
Constructs `permissionsService`, calls `publishPermissionsService(...)` and `emitReadyEvent(pi.events)` at init, and wires a `session_shutdown` cleanup closure that calls `unpublishPermissionsService()`.
|
||
- `src/service.ts` — the public accessor (`publishPermissionsService`, `getPermissionsService`, `unpublishPermissionsService`, the `PermissionsService` interface).
|
||
The package `exports` map points `.` at this file.
|
||
- `src/subagent-context.ts` — `isSubagentExecutionContext(ctx, subagentSessionsDir, registry?)`.
|
||
Branch 1 (registry by session id) is the only signal that identifies an **in-process** child; branches 2–3 (env, filesystem) identify **process-based** subagents.
|
||
- `src/subagent-registry.ts` — the process-global `SubagentSessionRegistry`, populated by the parent's `subagents:child:session-created` subscription before the child's `bindExtensions()`.
|
||
- `src/handlers/lifecycle.ts` — `SessionLifecycleHandler`. `handleSessionStart(event, ctx)` is the first lifecycle event that carries a `ctx` (and therefore a session id). `handleSessionShutdown()` runs the injected cleanup closure.
|
||
- `src/permission-events.ts` — `emitReadyEvent` / `PERMISSIONS_READY_CHANNEL`, documented as "emitted once on extension load."
|
||
|
||
Key constraint discovered during investigation: **the factory has no `ctx` at init**, so it cannot read the child's session id and cannot consult the registry to know whether it is an in-process child.
|
||
`FakePi` / `ExtensionAPI` expose no session id to the factory body.
|
||
The earliest moment child-ness can be determined is `session_start`, where `ctx.sessionManager.getSessionId()` is available and the parent has already registered the child.
|
||
|
||
Constraint from `AGENTS.md` / the package skill: `globalThis` + `Symbol.for()` is the prescribed cross-extension channel; module-scoped singletons do not survive jiti isolation.
|
||
The fix stays on that channel.
|
||
|
||
The `#297` composition-root suite (`test/composition-root.test.ts`) added a `multi-instance global service interplay` block that characterizes this exact bug: one test asserts the buggy `undefined`, and an `it.fails("DESIRED: ...")` asserts the fixed behavior.
|
||
|
||
## Design Overview
|
||
|
||
### Decision model
|
||
|
||
The publish decision moves from factory-init to `session_start`, gated on a precise **in-process child** check (registry only — not the full `isSubagentExecutionContext`).
|
||
Teardown becomes identity-scoped: an instance removes the slot only if it still holds that instance's own service object.
|
||
|
||
Why the registry-only gate (not `isSubagentExecutionContext`): the env-hint and filesystem branches identify **process-based** subagents, which run in a separate OS process with their own `globalThis` and *should* publish (they are the sole owner of their process's slot).
|
||
Only the registry branch identifies an **in-process** child sharing the parent's `globalThis` — the one case that must skip publishing.
|
||
Using the full detector would wrongly suppress publication inside a process-based subagent's own process.
|
||
|
||
Why publish at `session_start` rather than init: init has no `ctx`, so the in-process child cannot be distinguished from a reloaded parent at that point — both present as "slot already occupied."
|
||
The registry signal (the only reliable discriminator) needs a session id, which first appears at `session_start`.
|
||
|
||
Why compare-and-delete on teardown: if `/reload` re-runs the factory, the old instance's `session_shutdown` can fire after the new instance's `session_start` re-publish.
|
||
An unconditional delete (or a `didPublish` boolean) would wipe the new generation's service.
|
||
Comparing object identity makes teardown safe regardless of shutdown/init ordering, and also makes a child's shutdown a no-op (the child never owned the slot).
|
||
|
||
### Lifecycle trace (multi-instance, shared `globalThis`)
|
||
|
||
```text
|
||
parent init → construct adapter A (no publish, no ready yet)
|
||
parent session_start → not in registry → publish(A); emit ready slot = A
|
||
child init → construct adapter B (no publish, no ready yet)
|
||
child session_start → in registry → skip publish; emit ready slot = A
|
||
child session_shutdown → unpublish(B): current(A) !== B → no-op slot = A
|
||
parent still live → getPermissionsService() === A ✓
|
||
```
|
||
|
||
### Reload trace (factory re-runs for the top-level session)
|
||
|
||
```text
|
||
new parent init → construct adapter A' (no publish)
|
||
new parent session_start → not in registry → publish(A') slot = A'
|
||
old parent session_shutdown → unpublish(A): current(A') !== A → no-op slot = A'
|
||
```
|
||
|
||
(If the old shutdown instead fires first: `unpublish(A)` deletes A, then the new `session_start` publishes A'.
|
||
Either order ends at A'.)
|
||
|
||
### New / changed shapes
|
||
|
||
`unpublishPermissionsService` becomes identity-scoped (breaking signature):
|
||
|
||
```typescript
|
||
/**
|
||
* Remove the service from globalThis, but only when the current slot still
|
||
* holds `service`. A child instance (which never published) and a superseded
|
||
* reload generation are therefore no-ops.
|
||
*/
|
||
export function unpublishPermissionsService(service: PermissionsService): void {
|
||
if (getPermissionsService() === service) {
|
||
delete (globalThis as Record<symbol, unknown>)[SERVICE_KEY];
|
||
}
|
||
}
|
||
```
|
||
|
||
New exported helper in `subagent-context.ts` (extracted from branch 1, returns a value, reused by two call sites):
|
||
|
||
```typescript
|
||
export function isRegisteredSubagentChild(
|
||
ctx: ExtensionContext,
|
||
registry: SubagentSessionRegistry,
|
||
): boolean {
|
||
try {
|
||
const sessionId = ctx.sessionManager.getSessionId();
|
||
if (!sessionId) {
|
||
return false;
|
||
}
|
||
return registry.has(sessionId);
|
||
} catch {
|
||
return false; // getSessionId() unavailable → treat as not-a-registered-child
|
||
}
|
||
}
|
||
```
|
||
|
||
Composition-root wiring in `index.ts` (the new collaborator passed into the lifecycle handler):
|
||
|
||
```typescript
|
||
const activateServiceForSession = (ctx: ExtensionContext): void => {
|
||
if (!isRegisteredSubagentChild(ctx, subagentRegistry)) {
|
||
publishPermissionsService(permissionsService);
|
||
}
|
||
emitReadyEvent(pi.events);
|
||
};
|
||
// ...
|
||
const lifecycle = new SessionLifecycleHandler(session, activateServiceForSession, () => {
|
||
rpcHandles.unsubCheck();
|
||
rpcHandles.unsubPrompt();
|
||
unsubSubagentLifecycle();
|
||
unpublishPermissionsService(permissionsService);
|
||
});
|
||
```
|
||
|
||
`SessionLifecycleHandler.handleSessionStart` calls `this.activateService(ctx)` at the end of its body (after config refresh / reset, so the published adapter sees current runtime state).
|
||
This is a Tell-Don't-Ask seam: the handler announces "session started"; the composition root decides whether to publish.
|
||
Dependency width of the handler goes from 2 → 3 constructor deps — within the design-review threshold.
|
||
|
||
### `permissions:ready` semantics (confirmed with maintainer)
|
||
|
||
`emitReadyEvent` moves from init to `session_start` (emitted after the gated publish).
|
||
This preserves the #297 ordering contract — a consumer reacting to `permissions:ready` can still resolve the service immediately — at the cost of changing the event's timing from "once on load" to "once per `session_start`."
|
||
For an in-process child, `ready` still fires at its `session_start`, and `getPermissionsService()` then resolves the parent's service (already published), so the contract holds for children too.
|
||
|
||
### Edge cases
|
||
|
||
1. No session id (`getSessionId()` throws or returns empty) → `isRegisteredSubagentChild` returns `false` → instance publishes.
|
||
Safe default for top-level / process-based contexts.
|
||
2. Process-based subagent (env hint / filesystem) → not in registry → publishes into its own process's slot.
|
||
Unchanged from today.
|
||
3. `/reload` (`session_start` reason `"reload"`) → `activateServiceForSession` runs again, re-publishing the same adapter (idempotent) and re-emitting `ready`.
|
||
Acceptable under the new per-`session_start` semantics.
|
||
4. Concurrent sibling children → none publishes; the slot stays the parent's throughout. (The unsound stash/restore alternative was rejected for exactly this case.)
|
||
|
||
## Module-Level Changes
|
||
|
||
| File | Change |
|
||
| ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||
| `src/service.ts` | `unpublishPermissionsService(service)` → required param, identity compare-and-delete. Update doc comments (drop "overwrites … safe for /reload" framing on publish; describe identity-scoped delete). |
|
||
| `src/subagent-context.ts` | Add exported `isRegisteredSubagentChild(ctx, registry)`; refactor `isSubagentExecutionContext` branch 1 to call it. |
|
||
| `src/handlers/lifecycle.ts` | `SessionLifecycleHandler` gains a third constructor dep `activateService: (ctx) => void`; `handleSessionStart` calls it after refresh/reset. |
|
||
| `src/index.ts` | Remove init-time `publishPermissionsService(...)` and `emitReadyEvent(...)`. Add `activateServiceForSession`, pass it to `SessionLifecycleHandler`. Change cleanup closure to `unpublishPermissionsService(permissionsService)`. Swap the `isSubagentExecutionContext` import usage as needed (the existing `canRequestPermissionConfirmation` call still uses the full detector). |
|
||
| `src/permission-events.ts` | Update `PERMISSIONS_READY_CHANNEL` / `emitReadyEvent` doc comments: emitted at `session_start`, not load. |
|
||
| `docs/cross-extension-api.md` | Update: service published at `session_start` (not startup); `permissions:ready` timing in the events table and the "Ready Event" section; the reload note; the "publishes a fresh service on re-initialization" note. Add a short note that an in-process child does not publish and that `getPermissionsService()` resolves the parent's service. |
|
||
| `docs/architecture/architecture.md` | Line ~457: "publishes … during startup" → at `session_start`, gated for in-process children. |
|
||
|
||
Grep sweep confirmed no other `src/` consumers of `unpublishPermissionsService` (sole caller is the `index.ts` cleanup closure) and no other `emitReadyEvent` caller.
|
||
The package skill (`.pi/skills/package-pi-permission-system/SKILL.md`) does not document publish-at-load / ready-at-load specifics, so no skill edit is required (re-grep before finalizing the docs commit).
|
||
|
||
## Test Impact Analysis
|
||
|
||
1. **New tests the change enables.**
|
||
- `service.test.ts`: identity compare-and-delete — `unpublish(A)` clears only when the slot is `A`; `unpublish(B)` while slot is `A` is a no-op.
|
||
- `subagent-context.test.ts`: `isRegisteredSubagentChild` unit coverage (registered hit, miss, empty/absent session id, `getSessionId` throw).
|
||
- `composition-root.test.ts`: the `it.fails("DESIRED: the parent's service survives a child's shutdown")` flips to a real pass; a new assertion that mid-run `getPermissionsService()` resolves the **parent's** adapter, not the child's.
|
||
2. **Tests that become redundant.**
|
||
- `composition-root.test.ts`: the `it("currently leaves the parent without a resolvable service after the child shuts down")` characterization test asserts the bug and must be removed (its premise is now false).
|
||
3. **Tests that stay but need wiring updates (not redundant — they exercise the moved seam).**
|
||
- `composition-root.test.ts` "ready emitted after service publication" and "shutdown teardown chain": must now fire `session_start` (publish/ready moved off init).
|
||
Their contracts are unchanged.
|
||
- `handlers/lifecycle.test.ts`: the `new SessionLifecycleHandler(session, cleanupRpc)` instantiation gains the `activateService` stub; add a case asserting `handleSessionStart` invokes it.
|
||
- `service.test.ts` `afterEach` and the existing no-arg `unpublishPermissionsService()` calls: update to pass a service (e.g. `afterEach` clears via `const s = getPermissionsService(); if (s) unpublishPermissionsService(s);`).
|
||
|
||
## TDD Order
|
||
|
||
1. **`refactor: extract isRegisteredSubagentChild from subagent detector` (`test:` + `refactor:`)** Red: add `subagent-context.test.ts` cases for `isRegisteredSubagentChild` (hit / miss / empty id / throw).
|
||
Green: add the exported helper and rewire `isSubagentExecutionContext` branch 1 to call it.
|
||
Existing `isSubagentExecutionContext` tests must stay green (behavior unchanged).
|
||
Commit: `refactor: extract isRegisteredSubagentChild seam (#302)`.
|
||
|
||
2. **`feat!: identity-scoped unpublishPermissionsService`** This step removes the no-arg signature, so the sole `src/` caller (`index.ts` cleanup closure) and all `service.test.ts` no-arg calls break at the type level — fold them into this one commit.
|
||
Red: add `service.test.ts` cases — `unpublish(A)` clears only when slot is `A`; `unpublish(B)` while slot is `A` is a no-op; update `afterEach` and existing no-arg calls to pass a service.
|
||
Green: change `unpublishPermissionsService(service)` to compare-and-delete; update the `index.ts` cleanup closure to pass `permissionsService`.
|
||
Commit: `feat!: scope service teardown to the publishing instance (#302)`.
|
||
|
||
3. **`fix: publish the service at session_start, gated for in-process children`** This is the bug fix and the user-visible behavior change.
|
||
Adding the `activateService` dep to `SessionLifecycleHandler` breaks its sole production instantiation and the `handlers/lifecycle.test.ts` instantiation at the type level — fold those into this commit.
|
||
Red: update `composition-root.test.ts` — remove the bug-characterization test; convert `it.fails(...)` to a passing test; fire `session_start` (with a non-child parent ctx and a registry-registered child ctx) in the multi-instance helper, the teardown-chain test, and the ready-ordering test; add the "mid-run resolves the parent's adapter" assertion.
|
||
Update `handlers/lifecycle.test.ts` instantiation + add an "invokes activateService on session_start" case.
|
||
Green: in `index.ts`, remove init-time publish + `emitReadyEvent`, add `activateServiceForSession`, pass it into `SessionLifecycleHandler`; in `lifecycle.ts`, accept and invoke `activateService` in `handleSessionStart`.
|
||
Commit: `fix: keep the parent's service published across child shutdown (#302)`.
|
||
|
||
4. **`docs: align service publication and ready-event timing`** Update `service.ts` / `permission-events.ts` doc comments, `docs/cross-extension-api.md` (events table, Ready Event section, reload + re-initialization notes, in-process child note), and `docs/architecture/architecture.md`.
|
||
Re-grep the package skill before committing; include it only if it documents the old timing.
|
||
Commit: `docs: document session_start service publication and ready timing (#302)`.
|
||
|
||
## Risks and Mitigations
|
||
|
||
- **Risk:** an in-process child never fires `session_start`, so it never reaches the gate.
|
||
**Mitigation:** that is the desired outcome — the child simply never publishes; the parent's slot is untouched.
|
||
The parent always fires `session_start`, so it always publishes.
|
||
- **Risk:** a load-time consumer that resolved the service at extension load now sees `undefined` until `session_start`.
|
||
**Mitigation:** the documented best practice is to resolve per-use, and `permissions:ready` (the documented readiness signal) moves to `session_start` alongside the publish, so a `ready`-driven consumer is unaffected.
|
||
Called out in the docs commit.
|
||
- **Risk:** `unpublishPermissionsService` signature change breaks an external caller.
|
||
**Mitigation:** it is a provider-side lifecycle function; external consumers use only `getPermissionsService()`.
|
||
Shipped as `feat!:` so release-please bumps major and the change is visible in the changelog.
|
||
- **Risk:** using the wrong detector (full `isSubagentExecutionContext`) would suppress publication inside a process-based subagent's own process.
|
||
**Mitigation:** the gate deliberately uses the registry-only `isRegisteredSubagentChild`; the distinction is documented in Design Overview and covered by the helper's unit tests.
|
||
|
||
## Open Questions
|
||
|
||
- Should `permissions:ready` be emitted at most once per instance (guarded) rather than on every `session_start` including reloads?
|
||
Deferred: re-emitting on reload is harmless and arguably correct ("re-readied for this session"); revisit only if a consumer reports duplicate-`ready` churn.
|