fix(subagents): harden Pi adapter lifecycle - #219
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The current discardRunEvents reset in startRun() can allow late events from an interrupted run to affect a restarted run, undermining the stated lifecycle isolation guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR strengthens the production pi subagent backend’s lifecycle handling and adds end-to-end integration tests that drive real adapter logic via a minimal fake AgentSession, targeting the reliability gaps described in #176 (cancellation, late events, timeouts, cleanup, and restart behavior).
Changes:
- Introduces
makePiBackend()with test-only seams (sessionFactory,shutdownTimeoutMs) to enable production-adapter lifecycle integration coverage. - Adds a minimal controllable
PiAgentSessionharness for emitting native Pi SDK events and recording SDK calls without duplicating manager state. - Adds lifecycle-focused integration tests that exercise startup/streaming/tools/usage, restart vs steer behavior, cancellation with late events, watchdog timeouts, and bounded cleanup.
File summaries
| File | Description |
|---|---|
| tests/support/pi-agent-session-harness.ts | Adds a minimal fake AgentSession harness for driving native events and recording SDK calls in integration-style tests. |
| tests/extensions/subagents/pi-backend-lifecycle.test.ts | Adds end-to-end lifecycle tests that run through the production Pi adapter and real SubagentManager behaviors. |
| extensions/subagents/src/backends/pi.ts | Adds backend construction options and hardens lifecycle handling (event discarding after interruption, prompt-rejection generation guard). |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4da7544 to
477fcdd
Compare
|
Maintainer update on exact head 477fcdd, rebased linearly onto main@25fd64d41f0fb0546cef62862939f15bc200b9ba. The prior cancel/restart isolation remained unsafe during Pi preflight because native events have no run identity. The adapter now enforces one prompt Promise lease per AgentSession, blocks cancelled or closed preflight at Pi preflightResult before _runAgentPrompt, invalidates pending restarts on manager cancellation or Effect AbortSignal, and captures terminal evidence per invocation rather than from the compaction-mutable session transcript. Streaming partial output remains bounded and is preserved on cancellation. Impact gate: no tool schema, configuration, permission, session-file format, model selection, or worktree ownership changes. Normal same-session restart and streaming steer behavior remain. Cancellation now waits for the exact prompt Promise; the existing 5-second manager deadline force-closes fail-closed, and a released late preflight cannot enter provider execution. Verification on this exact source state:
The branch was updated with an explicit expected-SHA lease from 4da7544. Required GitHub CI and an independent GitHub approval are still pending; please review the exact head above. |
477fcdd to
e813de1
Compare
|
Merge-gate refresh: exact head is now |
|
Thanks for the update and for improving the lifecycle handling. This review helped me understand that I need to reason about prompt ownership and the full async lifecycle, not only event filtering. I’ll apply this more systematically in future contributions. I’m happy for the remaining approval checks to proceed. |
tt-a1i
left a comment
There was a problem hiding this comment.
按精确 head e813de1 做了真实 Pi lifecycle 对抗审查。现有 18 个测试和 CI 都通过,但 harness 把关键事件顺序约简掉了,仍有三个生产生命周期缺口:agent_settled 早于 prompt() rejection 时会先发布错误的 Completed;合法的 extension command/input 可让 prompt() 成功返回却完全不产生 agent lifecycle,当前永不 RunSettled;Pi preflight hook 没有 AbortSignal,强制关闭后仍可继续副作用,而 finalizer 已经回收 worktree。它们分别破坏终态真实性、slot 释放和工作区所有权,当前不应合并。建议以 exact prompt invocation 为唯一 lease:终态在 prompt Promise 完成后结合捕获的 lifecycle evidence 决定;无 lifecycle 成功返回要显式 fail;未证明 prompt quiescent 时保留 worktree并给出清理 receipt。
| state.settled = true; | ||
| const last = lastAssistantMessage(session); | ||
| const partialText = finalOutput(session) || undefined; | ||
| const activePrompt = state.activePrompt; |
There was a problem hiding this comment.
[P1] Pi 的真实顺序是 agent_settled 在 _runAgentPrompt.finally 中发出,外层 session.prompt() Promise 的 rejection 随后才可见。事件处理器立即调用此 settle() 时 state.runError 仍为空,会发布 Completed("");后到的 catch 无法撤回单一终态。请等待 exact prompt Promise 后再仲裁 terminal outcome,并加真实 SDK 顺序的 rejection 回归。
There was a problem hiding this comment.
已复核,问题确认存在。agent_settled 不能作为独立终态依据,必须等 exact prompt() Promise 完成后,再结合 rejection 和 assistant lifecycle evidence 仲裁最终 outcome。当前实现会在 prompt rejection 到达前错误发布 Completed;这条建议保留,暂不认为已解决。
| // agent_settled will arrive for them. | ||
| if (!session.isStreaming) settle(); | ||
| }) | ||
| .finally(() => { |
There was a problem hiding this comment.
[P1] prompt() 也可能被 extension command/input 处理并正常 resolve,而不进入 provider、因此没有 agent_start/agent_settled。当前 finally 只清 lease,不发 RunSettled,manager slot 最终只能被 silent-provider watchdog 误分类。请在 Promise 成功后检查本 invocation 是否观察到完整 agent lifecycle;没有就显式 Failed,并覆盖 handled command/input。
There was a problem hiding this comment.
已复核,问题确认存在。prompt() 可能被 extension command/input 正常处理并 resolve,但完全不进入 agent lifecycle;当前 finally 只释放 active prompt,不会发出 RunSettled,最终依赖 watchdog 并产生错误分类。需要增加显式失败终态及 handled command/input 回归测试。
| await shutdownAndDisposeChildSession(session, { | ||
| abort: true, | ||
| timeoutMs: CHILD_SHUTDOWN_TIMEOUT_MS, | ||
| timeoutMs: options.shutdownTimeoutMs ?? CHILD_SHUTDOWN_TIMEOUT_MS, |
There was a problem hiding this comment.
[P1] 这个 shutdown deadline 只能限制等待时间,不能取消 Pi preflight hook;hook 没有 AbortSignal,session.abort() 在 preflight 期间可能是 idle no-op。deadline 后 finalizer 仍继续回收 worktree,随后恢复的 preflight 可在已释放/复用路径继续副作用。实测 scope close 时 side effect=0,释放 preflight 后 side effect=1。未确认 exact prompt quiescent 时必须保留 worktree并暴露 cleanup uncertainty。
There was a problem hiding this comment.
已复核,问题确认存在。shutdown timeout 只能限制等待,不能证明 preflight 已停止;在 exact prompt() Promise 仍未 quiescent 时继续 reclaim worktree 会允许后续 preflight 在已释放或复用的路径上产生副作用。这里需要保留 worktree 并暴露 cleanup uncertainty/receipt,不能仅依赖当前 finalizer timeout。
|
Thanks for the review. Addressed the lifecycle issues in d85b9c2:
|
Problem
The production Pi adapter had limited lifecycle coverage. Existing manager tests relied on a scripted stub and could not verify how the real adapter handled cancellation, late
AgentSessionevents, prompt rejections, cleanup, or restart behavior.After cancellation, late events such as
agent_start, assistant/tool completion, oragent_settledcould potentially revive a run or produce duplicate terminalization.Closes #176
Value
This improves subagent lifecycle reliability by ensuring that:
Approach
sessionFactoryseam for injecting a controllable fake PiAgentSession.send()and idle restart;Validation
node --test --experimental-strip-types tests/extensions/subagents/pi-backend.test.ts tests/extensions/subagents/pi-backend-lifecycle.test.ts— 10 tests passed.git diff --check— passed.bun run checkandbun run testcould not be run directly because Bun is unavailable in the local PATH.Impact
createAgentSession()and the existing default shutdown timeout.