Embedded: stop replaying historical session replies#52614
Conversation
Greptile SummaryThis PR fixes duplicate outward delivery of historical assistant replies when Key observations:
Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-embedded-subscribe.handlers.messages.ts
Line: 48-50
Comment:
**Redundant `initialReplayInProgress` guard**
The `ctx.state.initialReplayInProgress` check here is now unreachable. The outer handler in `pi-embedded-subscribe.handlers.ts` already returns early for all event types when `initialReplayInProgress` is `true`, so `handleMessageStart`, `handleMessageUpdate`, and `handleMessageEnd` are never invoked during replay. Only the `preexistingMessages.has(message)` guard below provides unique value in `resolveEligibleAssistantMessage` — it protects against a preexisting message object appearing in a post-subscribe event.
Consider removing the redundant lines to keep the intent clear, or add a comment explaining the belt-and-suspenders rationale:
```suggestion
if (ctx.state.preexistingMessages.has(message)) {
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Embedded: stop replaying historical sess..." | Re-trigger Greptile |
| if (ctx.state.initialReplayInProgress) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
initialReplayInProgress guard
The ctx.state.initialReplayInProgress check here is now unreachable. The outer handler in pi-embedded-subscribe.handlers.ts already returns early for all event types when initialReplayInProgress is true, so handleMessageStart, handleMessageUpdate, and handleMessageEnd are never invoked during replay. Only the preexistingMessages.has(message) guard below provides unique value in resolveEligibleAssistantMessage — it protects against a preexisting message object appearing in a post-subscribe event.
Consider removing the redundant lines to keep the intent clear, or add a comment explaining the belt-and-suspenders rationale:
| if (ctx.state.initialReplayInProgress) { | |
| return null; | |
| } | |
| if (ctx.state.preexistingMessages.has(message)) { | |
| return null; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-subscribe.handlers.messages.ts
Line: 48-50
Comment:
**Redundant `initialReplayInProgress` guard**
The `ctx.state.initialReplayInProgress` check here is now unreachable. The outer handler in `pi-embedded-subscribe.handlers.ts` already returns early for all event types when `initialReplayInProgress` is `true`, so `handleMessageStart`, `handleMessageUpdate`, and `handleMessageEnd` are never invoked during replay. Only the `preexistingMessages.has(message)` guard below provides unique value in `resolveEligibleAssistantMessage` — it protects against a preexisting message object appearing in a post-subscribe event.
Consider removing the redundant lines to keep the intent clear, or add a comment explaining the belt-and-suspenders rationale:
```suggestion
if (ctx.state.preexistingMessages.has(message)) {
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.|
This pull request has been automatically marked as stale due to inactivity. |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. The PR remains useful but is not merge-ready: current main still lacks the requested attach-time historical replay guard, while the branch is stale against newer transcript-only assistant suppression and still lacks after-fix real behavior proof for a supported replaying embedded session path. Canonical path: Close this PR as superseded by #45489. So I’m closing this here and keeping the remaining discussion on #45489. Review detailsBest possible solution: Close this PR as superseded by #45489. Do we have a high-confidence way to reproduce the issue? No high-confidence real runtime reproduction is established. The synthetic PR harness demonstrates the failure shape for a replaying subscribe implementation, but the discussion still lacks proof of a supported current embedded replay source producing the duplicate outward delivery. Is this the best way to solve the issue? Unclear as submitted. The intended boundary is reasonable, but the branch needs rebase work to preserve current transcript-only suppression and maintainer confirmation or real proof for the synchronous replay contract. Security review: Security review cleared: The diff only changes embedded subscribe TypeScript logic and colocated tests, with no concrete security or supply-chain concern found. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 996d07ee466c. |
|
Codex review: needs maintainer review before merge. What this changes: The PR snapshots preexisting embedded session messages, suppresses initial replay/preexisting assistant lifecycle delivery, and adds a regression test that only fresh assistant replies emit after subscription. Maintainer follow-up before merge: This is an open implementation PR with a narrow plausible fix already attached; the remaining action is maintainer replay-contract review, rebase/conflict resolution, and normal validation rather than a separate automated repair branch. Review detailsBest possible solution: Land or replace a narrow embedded subscribe fix that snapshots preexisting session messages, suppresses only historical replay/preexisting assistant outward delivery, preserves fresh assistant replies and current transcript-only filters, and keeps focused regression coverage for the attach-to-existing-session case. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against e46dccb35374. |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
ClawSweeper applied the proposed close for this PR.
|
Summary
This prevents embedded subscribe from replaying historical assistant replies when attaching to an existing session.
Problem
When
subscribeEmbeddedPiSession(...)subscribes to a session that already contains assistant messages, the initial replay can include historical assistant lifecycle events. Those events currently flow through the same outward-facing assistant delivery path as new replies.That means a subscriber attaching to an existing session can emit an old assistant final again, even though it was already delivered before subscription.
Fix
Track whether embedded subscribe is still processing the initial replay and keep a set of preexisting session messages. While initial replay is in progress, and for messages already present when subscription started, assistant lifecycle handlers now ignore those events instead of treating them like fresh outward replies.
This keeps historical transcript state intact while preventing duplicate outward delivery.
Tests
Add a regression test that subscribes to an existing session containing a historical assistant reply, replays those historical events, then emits a new assistant reply. The subscriber should only emit the new reply.
Validation
pnpm test -- src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.subscribeembeddedpisession.test.ts -t "does not replay historical assistant finals when subscribing to an existing session"Note
This PR is intentionally separate from the transcript-only
gateway-injectedfiltering fix. Both touch embedded subscribe delivery, but they address different duplicate-reply failure modes.