fix(pi): separate typed final answers from activity text - #547
fix(pi): separate typed final answers from activity text#547lidge-jun wants to merge 14 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 897e3d35b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| function usesPiSettled(command: PiCommand): boolean { | ||
| const result = probePiCommandVersion(command, process.env); |
There was a problem hiding this comment.
Whenever a Pi runtime is created, usesPiSettled() synchronously launches <pi command> --version with a 15-second timeout; resolvePiCommand() may already have run its own probe immediately beforehand. With the npm-exec fallback or a slow/broken wrapper, each pooled-session creation—and every direct worker turn—can block the server event loop for seconds, delaying unrelated HTTP, SSE, and agent work. Cache the detected capability or perform this probe asynchronously rather than running spawnSync during each spawn.
Useful? React with 👍 / 👎.
| Pi final selection is owned by `src/agent/runtime/pi-turn.ts`, independently of Activity storage. The latest typed assistant message supplies a final only when its `stopReason` is `stop` and its content has no `toolCall`; text blocks preserve null, empty and whitespace distinctly. Earlier assistant commentary and accepted deltas stay in bounded `partialText`, while repeated message/turn/agent snapshots do not duplicate them. Error, abort, length and unknown stop reasons cannot promote an earlier answer. Pi invokes the existing explicit-outcome lifecycle handoff; adapters that omit an outcome retain legacy selection. | ||
|
|
||
| Upstream Pi versions from 0.80.4 finish at `agent_settled`, after automatic retry, compaction and queued continuations. Older or unrecognized version strings retain the legacy `agent_end` boundary, except `willRetry:true`; this compatibility path cannot promise session-level settlement. `willRetry` itself predates `agent_settled` and is not a capability flag. Version probing is bounded and writes no shared capability/profile settings. Activity message phases may remain `unknown`; UI and journal replay never select the final. |
There was a problem hiding this comment.
This activates explicit Pi outcome selection and changes the authoritative completion boundary, but structure/stream-events.md still describes Pi callbacks as legacy and says the observer does not affect the existing final/abort lifecycle. Update that companion document with the new partialText/typed-final and agent_settled behavior so consumers do not implement against the obsolete contract.
AGENTS.md reference: structure/AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
1 participant
Pi tool turns could include pre-tool commentary in the final answer because the runtime accumulated every assistant message. Select the latest completed typed assistant at the Pi settlement boundary, while retaining accepted text separately for interrupted-turn salvage. Preserve empty/absent finals and failure status, and avoid replaying streamed text from terminal snapshots.
The Pi lifecycle receives the explicit outcome before lease release, retains kill-steer provenance for interrupted MESSAGE storage, and invokes lifecycle delivery once. Upstream Pi 0.80.4+ uses
agent_settled; older or unrecognized versions retain the legacy completion boundary without timer guessing.This branch normally merges published parent
2c3dd0fa5188739aaa3802d272dd857e02fa296fand its test-only follow-upef1fb23bf125513fd9a595d46cded24c18107689. Source integration commits are6f27ce332and6577fcbae(first-parent diffs); private document pins are separate. Parent behavioral Pi cancellation and gateway/Stop tests are retained. Pi finality/old-version semantics remain unchanged.Validation:
PI_ACTIVITY_DONE, retaining commentary in partial output. Child exited and runtime pool was empty. The subsequent parent follow-up changes only tests, with production-tree equality verified.Validation history: the earlier Node run had 9,067 passes, one inherited
DOCS-CHECK-001failure, and 46 skips. Clean base7335600reproduced the same API count mismatch. Docs-onlyf3d1dc5ddchanged 256/255 to the extracted 258/257, with unchanged assertions passing 2/2. That repair is already in the parent, not an additional API/route change in this diff.