Skip to content

fix(subagents): harden Pi adapter lifecycle - #219

Open
CuSO41108 wants to merge 6 commits into
openpi-dev:mainfrom
CuSO41108:codex/issue-176-pi-adapter-lifecycle-tests
Open

fix(subagents): harden Pi adapter lifecycle#219
CuSO41108 wants to merge 6 commits into
openpi-dev:mainfrom
CuSO41108:codex/issue-176-pi-adapter-lifecycle-tests

Conversation

@CuSO41108

Copy link
Copy Markdown
Contributor

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 AgentSession events, prompt rejections, cleanup, or restart behavior.

After cancellation, late events such as agent_start, assistant/tool completion, or agent_settled could potentially revive a run or produce duplicate terminalization.

Closes #176

Value

This improves subagent lifecycle reliability by ensuring that:

  • cancelled runs do not publish stale results;
  • each run reaches a single terminal state;
  • silent providers do not hold concurrency slots forever;
  • cleanup and disposal remain bounded;
  • restart/send behavior remains isolated between runs.

Approach

  • Added a test-only sessionFactory seam for injecting a controllable fake Pi AgentSession.
  • Added a test-only shutdown timeout override; production defaults remain unchanged.
  • Added a minimal fake Session harness that emits native Pi session events and records SDK calls without duplicating manager state.
  • Added integration coverage through the production Pi adapter and real subagent manager for:
    • startup, first response, tool lifecycle, usage, and final settlement;
    • streaming send() and idle restart;
    • cancellation during prompt/tool execution;
    • late completion after cancellation;
    • silent-provider first-response timeout;
    • cleanup timeout and cleanup errors;
    • model, thinking level, cwd, and tool allowlist propagation.
  • Hardened the adapter by ignoring stale events after interruption and ignoring late prompt rejections from older run generations.

Validation

  • node --test --experimental-strip-types tests/extensions/subagents/pi-backend.test.ts tests/extensions/subagents/pi-backend-lifecycle.test.ts — 10 tests passed.
  • Biome lint — passed.
  • TypeScript typecheck — passed; existing Effect diagnostics remain unchanged.
  • git diff --check — passed.
  • The full repository test suite was attempted. This Windows environment still reports unrelated path-separator, symlink-permission, and file-mode failures.
  • bun run check and bun run test could not be run directly because Bun is unavailable in the local PATH.

Impact

  • User-visible behavior: no intentional UI changes.
  • Model-visible context: no schema changes.
  • Runtime/lifecycle: cancelled runs ignore stale events and preserve single terminalization.
  • Persistence/configuration: no changes.
  • Compatibility: production still uses createAgentSession() and the existing default shutdown timeout.

Copilot AI lite review requested due to automatic review settings August 28, 2026 06:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 PiAgentSession harness 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.

Comment thread extensions/subagents/src/backends/pi.ts
@tt-a1i
tt-a1i force-pushed the codex/issue-176-pi-adapter-lifecycle-tests branch from 4da7544 to 477fcdd Compare August 29, 2026 15:33
@github-actions github-actions Bot added the area:subagents Subagent delegation, skills, or tests label Aug 29, 2026
@tt-a1i

tt-a1i commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

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:

  • focused Pi backend/lifecycle: 18 passed, 0 failed
  • bun run check: passed
  • bun run test: 1024 passed, 1 platform skip, 0 failed; Vitest 30/30
  • git diff --check: clean
  • two independent final implementation reviews: no findings

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.

@tt-a1i
tt-a1i force-pushed the codex/issue-176-pi-adapter-lifecycle-tests branch from 477fcdd to e813de1 Compare August 29, 2026 15:44
@tt-a1i

tt-a1i commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Merge-gate refresh: exact head is now e813de125f81b1bba3bbcdcbf15f5f33e1979b28, linearly rebased onto verified main@274097153320c3ae23be8e3da31d98626d2c3223. git range-diff shows all three implementation commits are patch-equivalent to the prior reviewed source. On this exact source state: focused Pi backend/lifecycle tests 18/18 pass, bun run check passes, git diff --check is clean, and GitHub Node 22, Node 24, Windows, and label checks are green. Independent GitHub approval is the only remaining gate; please review this exact head.

@CuSO41108

Copy link
Copy Markdown
Contributor Author

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 tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

按精确 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。

Comment thread extensions/subagents/src/backends/pi.ts Outdated
state.settled = true;
const last = lastAssistantMessage(session);
const partialText = finalOutput(session) || undefined;
const activePrompt = state.activePrompt;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Pi 的真实顺序是 agent_settled_runAgentPrompt.finally 中发出,外层 session.prompt() Promise 的 rejection 随后才可见。事件处理器立即调用此 settle()state.runError 仍为空,会发布 Completed("");后到的 catch 无法撤回单一终态。请等待 exact prompt Promise 后再仲裁 terminal outcome,并加真实 SDK 顺序的 rejection 回归。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已复核,问题确认存在。agent_settled 不能作为独立终态依据,必须等 exact prompt() Promise 完成后,再结合 rejection 和 assistant lifecycle evidence 仲裁最终 outcome。当前实现会在 prompt rejection 到达前错误发布 Completed;这条建议保留,暂不认为已解决。

// agent_settled will arrive for them.
if (!session.isStreaming) settle();
})
.finally(() => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已复核,问题确认存在。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,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已复核,问题确认存在。shutdown timeout 只能限制等待,不能证明 preflight 已停止;在 exact prompt() Promise 仍未 quiescent 时继续 reclaim worktree 会允许后续 preflight 在已释放或复用的路径上产生副作用。这里需要保留 worktree 并暴露 cleanup uncertainty/receipt,不能仅依赖当前 finalizer timeout。

@CuSO41108

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed the lifecycle issues in d85b9c2:

  • prompt() rejection now takes precedence over early agent_settled.
  • Missing lifecycle events produce an explicit failure.
  • Unsafe cleanup preserves the worktree and exposes a cleanup receipt.
    Please re-review the current head.

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

Labels

area:subagents Subagent delegation, skills, or tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

subagents: 为生产 Pi adapter 补齐生命周期集成覆盖

4 participants