fix(agent): close provisional event-envelope regressions - #4515
fix(agent): close provisional event-envelope regressions#4515Yeachan-Heo wants to merge 14 commits into
Conversation
196ed3a to
9afe806
Compare
![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: 196ed3aa0a
ℹ️ 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".
| (event.type === "message_end" && event.message.role === "assistant") || | ||
| event.type === "turn_end" || | ||
| event.type === "agent_end" | ||
| ) { | ||
| this.#provisionalStreamingToolCallIds.clear(); |
There was a problem hiding this comment.
When session-event persistence or extension delivery delays a message_update handler at await this.#emitSessionEvent(...), the synchronous Agent listener can start the later assistant message_end handler and clear this set before that update resumes. The resumed update then fails #isProvisionalStreamingToolEvent and runs #preCacheStreamingEditFile and #maybeAbortStreamingEdit a second time, defeating the intended exactly-once provisional safety path and duplicating edit-guard work for accepted tool-call streams. Track provisional status on the event itself or defer cleanup until all update handlers for the turn have completed.
Useful? React with 👍 / 👎.
7553883 to
845a300
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 845a300a11
ℹ️ 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".
| const eventKey = this.#provisionalStreamingEventKey(message, assistantMessageEvent); | ||
| if (eventKey) this.#provisionalStreamingEventKeys.add(eventKey); |
There was a problem hiding this comment.
Every provisional tool-call update adds a payload-bearing string to #provisionalStreamingEventKeys, but no terminal path or handler-settlement path ever removes entries from that set (only #provisionalStreamingToolCallIds is cleared). Long-lived sessions therefore retain every streamed tool delta indefinitely; moreover, an externally emitted or replayed update that later reuses the same tool ID, event type, index, and payload is incorrectly treated as provisional, causing #preCacheStreamingEditFile and #maybeAbortStreamingEdit to be skipped. Retain keys only until the corresponding public handler has settled, using reference counts if duplicate keys may be in flight.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7518f6f92c
ℹ️ 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".
| #handleAgentEvent = async (event: AgentEvent, activePromptHandle?: string): Promise<void> => { | ||
| const attemptScope = (event as AgentEvent & { scope?: AttemptScope }).scope; | ||
| const provisionalStreamingEvent = this.#isProvisionalStreamingToolEvent(event); | ||
| if (provisionalStreamingEvent) this.#provisionalStreamingHandlers++; |
There was a problem hiding this comment.
When an interrupting TTSR rule matches a provisional toolcall_delta, the handler returns at line 5021 before reaching the new finally, so this increment is never balanced. The later terminal event sets retirement pending but cannot clear the keys while the leaked count remains positive; all subsequent turns then accumulate keys indefinitely, and reused event keys can incorrectly skip streaming-edit guards. Fresh evidence beyond the earlier retention report is that the final counter-based fix still leaves this existing TTSR early-return path outside its settlement scope.
Useful? React with 👍 / 👎.
c1bc919 to
200598f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 200598fa55
ℹ️ 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".
| if (loopSignal.aborted) message.stopReason = "aborted"; | ||
| if (stream.hasActiveConsumer) await stream.waitForConsumerDrain(new AbortController().signal); | ||
| if (loopSignal.aborted) break; | ||
| if (loopSignal.aborted) message.stopReason = "aborted"; |
There was a problem hiding this comment.
When an event consumer aborts during waitForConsumerDrain—for example, when the streaming-edit guard rejects an accepted tool call—the transaction has already flushed a detached message_end whose message still has stopReason: "toolUse". Mutating only this separate accepted snapshot afterward does not update that queued event, and Agent appends the stale message to its state in packages/agent/src/agent.ts:1868-1881; the subsequent turn_end and placeholder tool result therefore describe an aborted turn while the public and persisted assistant lifecycle records a normal tool-use completion. Publish or replace the terminal message_end with the aborted snapshot before finalizing the turn.
Useful? React with 👍 / 👎.
| // can then abort the run before any tool execute() is entered. | ||
| if (message.stopReason !== "aborted" && message.stopReason !== "error") { | ||
| if (loopSignal.aborted) break; | ||
| if (loopSignal.aborted) message.stopReason = "aborted"; |
There was a problem hiding this comment.
This commit changes shipped behavior in both packages/agent and packages/coding-agent, but it updates neither package changelog, so the provisional-event and cancellation fixes will be omitted from the release notes. Add entries under each affected package's ## [Unreleased] section as required by the repository contract.
AGENTS.md reference: AGENTS.md:L188-L188
Useful? React with 👍 / 👎.
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c3d366396
ℹ️ 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".
| escapedToolTransaction.replacePendingAssistantMessage(message); | ||
| escapedToolTransaction.flush(); |
There was a problem hiding this comment.
When an accepted tool-call response streamed text first, the text path at agent-loop.ts:2778 has already committed the transaction, so its message_end is published with stopReason: "toolUse" before this drain. If a consumer then aborts while draining a later tool-call update, replacePendingAssistantMessage() has an empty batch and flush() returns because the transaction is committed, while the loop emits aborted placeholder results and turn_end; persisted/public assistant state therefore remains inconsistent. Fresh evidence at this exact head is that the terminal replacement only works for transactions that were never committed by visible text.
Useful? React with 👍 / 👎.
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d6e65b8f1
ℹ️ 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".
|
|
||
| - A turn whose tool arguments arrive flagged `escapedNonAsciiArguments` is now resampled instead of being reported as a tool failure: the defective assistant turn is dropped from history and the request is re-issued, up to twice per turn, before the terminal per-call rejection takes over. Hand-spelled `\uXXXX` arguments decode into valid-looking but silently wrong text (observed as garbled Hangul in `ask` prompts) and no post-parse repair can recover them, but the defect is a wire-format accident that resampling clears - surfacing it as a tool error instead burned the whole turn and fed the literal escape syntax back into the context the model samples from next. Scoped to the non-managed session path, matching the existing `invalid_prompt` and reasoning-content repairs; managed fallback keeps owning its own retry policy. | ||
| - Unmanaged escaped-non-ASCII resampling now stages a detached, provider-metadata-preserving assistant lifecycle until validation, publishes live safety updates before dispatch, and defers terminal `message_end` publication until subscriber-triggered cancellation is resolved so persisted assistant state and aborted tool-result pairing cannot disagree. | ||
| - A turn whose tool arguments arrive flagged `escapedNonAsciiArguments` is now resampled instead of being reported as a tool failure: the defective assistant turn is dropped from history and the request is re-issued, up to twice per turn, before the terminal per-call rejection takes over. Hand-spelled `\uXXXX` arguments decode into valid-looking but silently wrong text (observed as garbled Hangul in `ask` prompts) and no post-parse repair can recover them, but the defect is a wire-format accident that resampling clears - surfacing it as a tool error instead burned the whole turn and fed the literal escape syntax back into the context the model samples from next. Scoped to the non-managed session path, matching the existing `invalid_prompt` and reasoning-cont… |
There was a problem hiding this comment.
The existing escaped-non-ASCII release note is shortened mid-word to reasoning-cont…, deleting its explanation that the behavior matches the reasoning-content repairs while managed fallback retains its own retry policy. Because release notes are generated from this Unreleased section, this commit would ship a visibly corrupted and incomplete changelog entry; retain the original text while adding the new entry.
Useful? React with 👍 / 👎.
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8daf191e3
ℹ️ 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".
| if (event.type === "message_end" || event.type === "turn_end") { | ||
| this.#batch.push({ type: "event", event }); | ||
| return; |
There was a problem hiding this comment.
When a response streams visible text and then triggers an unrecoverable text-surface Harmony leak, the text path marks this transaction committed, so its subsequent message_end is retained here. The abort_retry branch at line 1993 then continues with a new transaction without flushing or discarding this batch, leaving subscribers with a published message_start/message_update lifecycle that never receives message_end and potentially leaving leaked text orphaned in the TUI. Settle the retained terminal—or explicitly reconcile the visible attempt—before taking that retry path.
Useful? React with 👍 / 👎.
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf5ad07190
ℹ️ 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".
| } | ||
| await emitHarmonyAudit(config, err, "truncate_resume", harmonyRetryAttempt); | ||
| } else { | ||
| if (escapedToolTransaction?.committed) escapedToolTransaction.flush(); |
There was a problem hiding this comment.
When a Harmony leak occurs after visible text commits the transaction, this flush publishes the queued message_end containing the contaminated assistant message. The real Agent consumer appends every message_end to #state.messages (agent.ts:1868-1881), and AgentSession persists it, while the later splice only edits the loop's copied currentContext; consequently the clean retry succeeds for the current request but the leaked control text remains in durable session history and is replayed on the next prompt. Close the visible lifecycle without committing the contaminated message as accepted history.
Useful? React with 👍 / 👎.
| } | ||
| await emitHarmonyAudit(config, err, "truncate_resume", harmonyRetryAttempt); | ||
| } else { | ||
| if (escapedToolTransaction?.committed) escapedToolTransaction.flush(); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier changelog finding is that exact head bf5ad07 adds a separate shipped behavior change for visible Harmony retries, but the current packages/agent/CHANGELOG.md entry only describes escaped-non-ASCII resampling and post-flush cancellation, so this retry-lifecycle fix is still absent from the Unreleased release notes. Add an Unreleased entry covering the Harmony terminal settlement.
AGENTS.md reference: AGENTS.md:L188-L188
Useful? React with 👍 / 👎.
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b6cc286ca
ℹ️ 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".
| } | ||
| await emitHarmonyAudit(config, err, "truncate_resume", harmonyRetryAttempt); | ||
| } else { | ||
| if (escapedToolTransaction?.committed) escapedToolTransaction.flush(); |
There was a problem hiding this comment.
When an unrecoverable Harmony leak follows visible text, this flush publishes the retained message_end containing the contaminated assistant message; Agent then unconditionally appends it to state in agent.ts:1868-1881, and AgentSession persists the same event in agent-session.ts:4735-4769. The subsequent splice only changes the loop's copied currentContext, so the leaked control text remains in durable session history and is replayed on the next prompt. Fresh evidence at exact head 0b6cc286 is that the added flush closes the lifecycle but still performs no corresponding durable-state reconciliation before retrying.
Useful? React with 👍 / 👎.
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcd36a8362
ℹ️ 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".
| escapedToolTransaction.replacePendingAssistantMessage({ | ||
| ...contaminated, | ||
| content: [], | ||
| stopReason: "aborted", | ||
| }); |
There was a problem hiding this comment.
When an OpenAI Codex response leaks Harmony after visible text, this spread retains contaminated.providerPayload in the supposedly sanitized aborted message. That payload contains the raw native output items (packages/ai/src/providers/openai-codex-responses.ts:1824), is persisted with the replacement message_end, and is preferred over the emptied content during later history conversion (packages/ai/src/providers/openai-codex-responses.ts:2735-2751), so leaks such as the <invoke> envelope remain durable and are replayed on the next prompt. Fresh evidence at exact head fcd36a8362 is that only content is cleared; also remove providerPayload, as the existing recovered-Harmony path does.
Useful? React with 👍 / 👎.
|
@codex review exact head |
|
@codex review exact head |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Independent exact-head approval requested for |
|
@codex review exact head |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Requested authenticated non-author exact-head approval from @probepark for |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Request changes - a rewrite of attempt-transaction terminal semantics with zero tests
Reviewed b12461971d9d721df9e6159c75c1a38bfcca47de against base 38f3b4077.
Let me start with the thing the diff view gets wrong. +850/-682 looks alarming; almost all
of it is indentation from wrapping #handleAgentEvent in try/finally:
$ git diff --stat 38f3b4077...b12461971
packages/coding-agent/src/session/agent-session.ts | 1396 ++++++++++----------
$ git diff -w --stat 38f3b4077...b12461971
packages/coding-agent/src/session/agent-session.ts | 70 ++++++++++-
packages/agent/src/agent-loop.ts | 131 +++++++++++++++++++--
4 files changed, 187 insertions(+), 19 deletions(-)
So this is ~187 substantive lines. I reviewed those, and the logic holds up. The problem is
everything around it.
BLOCKER - the PR changes four transaction invariants and adds no test for any of them
$ git diff --numstat 38f3b4077...b12461971
4 2 packages/agent/CHANGELOG.md
131 ... packages/agent/src/agent-loop.ts
1 0 packages/coding-agent/CHANGELOG.md
1396 ... packages/coding-agent/src/session/agent-session.ts
No test file. Not one. And these are the semantics that moved:
flush()no longer early-returns when committed (agent-loop.ts:1086-1087), so it can now
run a second time aftercommitCallbacksAndUpdates().push()divertsmessage_end/turn_endinto the batch after commit
(agent-loop.ts:1058-1062) instead of pushing them straight to the stream. Terminals are
now retained and depend on a laterflush()to ever reach a consumer.- New
flushNonTerminal(),commitCallbacksAndUpdates()andreplacePendingAssistantMessage()
split what used to be one atomic flush into three phases. if (loopSignal.aborted) break;becameif (loopSignal.aborted) message.stopReason = "aborted";
(agent-loop.ts:2176,2178) - an abort now falls through instead of exiting the loop.
Point 2 is the one that worries me. If a committed transaction is ever discarded or abandoned
without a second flush(), message_end/turn_end are silently swallowed and the consuming
session waits forever for a terminal that is sitting in #batch. There is no test pinning
"terminals retained after commit are delivered exactly once".
I checked point 4 before flagging it, and it is actually an improvement: break skipped the
tool_use/tool_result pairing block at agent-loop.ts:2189+, whereas falling through with
stopReason = "aborted" produces properly paired placeholder results and a clean
stream.end(newMessages). It is still an unlabelled behaviour change that nothing tests.
Existing tests cannot catch any of it - they are identical on both sides:
# base 38f3b4077
$ bun test packages/agent/test/agent-loop-escaped-nonascii-toolcall.test.ts packages/agent/test/agent-loop.test.ts
57 pass 0 fail
# head b12461971
$ bun test <same>
57 pass 0 fail
Same suites, same counts. No test distinguishes pre-fix from post-fix code, which is the
definition of an unproven change.
BLOCKER - CI never ran the suite that owns this contract
$ gh api .../commits/b12461971.../check-runs --jq '.check_runs[]|select(.name|startswith("Affected path validation / test"))'
success test:packages/agent/test/agent-loop.test.ts
success test:packages/coding-agent/test/notifications-live-stream.test.ts
success test:packages/coding-agent/test/session-manager-resident-cache.test.ts
Three files. packages/agent/test/agent-loop-escaped-nonascii-toolcall.test.ts - 693 lines,
the suite that owns issue #4489's escaped-non-ASCII resample contract, the thing this branch
is named after (fix/issue-4489-escaped-nonascii-resample-successor) - was not selected by
the affected-path planner and did not run. The green checkmark on this PR does not mean what
it looks like it means.
Worth a separate issue: the planner picked agent-loop.test.ts but not
agent-loop-escaped-nonascii-toolcall.test.ts for a diff that rewrites
ManagedAttemptTransaction. That selection heuristic has a hole.
MAJOR - the verdict digest in the body is stale, so the gate cannot pass as written
body: sha256:0997ca43061c01c22ef8b86b35fb075dbd3291f77e2088e7baee828672e85c59
computed: sha256:f7b51bd4d1b6151bc41000ec156abd0ec3b9ac6eb8f3c386e9ef957280a60855
(git diff --binary --full-index --no-ext-diff 38f3b4077...b12461971 | shasum -a 256)
The head moved after that line was written. Even with an approval it would fail with
Stale verdict digest. I verified my computation method against #4459 and #4523, where my
independently computed digests matched their existing body lines exactly - so the method is
right and this line is stale.
MINOR - a defensive guard was dropped without explanation
const producedIndex = newMessages.lastIndexOf(message);
-if (producedIndex >= 0) newMessages[producedIndex] = acceptedMessage;
+newMessages[producedIndex] = acceptedMessage;I traced it and it is safe today: newMessages.push(message) at agent-loop.ts:2121 runs
unconditionally before this point, and the only reassignment of message in between is inside
if (attemptTransaction) at 2155-2160, which also writes it to the last index. So
lastIndexOf cannot return -1 on any current path.
But escapedToolTransaction only exists when config.fallbackManaged is false
(agent-loop.ts:1784-1786), which is precisely when attemptTransaction is undefined - so
the guarantee comes from a push 40 lines away rather than from anything local. If it ever
does return -1, newMessages[-1] = ... sets a string property named "-1" on the array,
silently drops the accepted snapshot, and throws nothing. Either restore the guard or replace
it with an assertion that makes the invariant explicit.
Observation on reviewability
Reindenting a 1,400-line region in the same commit as a subtle correctness fix hides ~190
substantive lines inside ~1,400 changed lines. Nobody reviewing by eye finds them. Split
mechanical reindentation into its own commit next time - git diff -w should not be required
to see what a PR does.
What I need to approve this
- Regression tests for the retained-terminal path: terminals staged after
commitCallbacksAndUpdates()are delivered exactly once by the laterflush(), are dropped
ondiscard(), and the abort fall-through yields paired tool results. - A test that fails on the base commit. Right now nothing does.
- Recompute the body verdict digest against the current head.
- Ideally: make the escaped-non-ASCII suite part of the affected-path selection for changes to
agent-loop.ts, so the owning contract actually runs.
Verdict
merge-blocked. I am not claiming the code is wrong - I read the 187 substantive lines and
they are coherent, and I confirmed the abort change is a genuine improvement. I am saying the
change is unproven, and this particular area (attempt transactions, terminal delivery, session
persistence ordering) is the last place in this codebase where "looks right" should be enough.
Reviewed by @probepark - method: whitespace-normalized diff to isolate substantive changes, line-level read of ManagedAttemptTransaction and #handleAgentEvent at the head sha, manual invariant tracing for the dropped guard, base-vs-head bun test in a detached worktree with real bun install, CI test-selection enumeration, independent digest recomputation cross-validated against #4459 and #4523.
Addendum: widened the coverage check from 2 suites to 6 - the result is stronger, not weakerMy blocker above ("no test distinguishes pre-fix from post-fix code") rested on two suites. That Clean detached worktree, fresh Identical down to the expect() count. Six suites, 107 tests, and not one of them changes A static check points the same way - across every test directory at this head, nothing references So This does not change my verdict, it just removes the last way to argue with it. merge-blocked For the record on process: the earlier line in my review that the abort change is a genuine |
Final adversarial review found that non-cloneable metadata fallback could drop required AgentEvent envelope fields, typed transport classification, and post-flush abort pairing. Snapshot event envelopes explicitly, retain known transport facts, convert post-flush cancellation into an aborted turn, and scope provisional tool-call dedupe to the active turn. Lore-id: 9c41ad72 Constraint: public provisional event envelopes remain valid Constraint: typed provider failures survive non-cloneable siblings Constraint: post-flush cancellation emits paired aborted results Constraint: streaming safety dedupe is turn-scoped Confidence: high Scope-risk: medium Reversibility: easy Tested: full 760-test agent suite, 127 lifecycle regressions, affected package checks
A delayed public message_update can resume after message_end clears turn-scoped tool IDs. Mark the assistant event object itself during private provisional observation so the in-flight handler retains exactly-once safety identity regardless of terminal interleaving, while turn cleanup still bounds future IDs. Lore-id: 9c41ad72 Constraint: delayed update handlers do not rerun streaming safety Constraint: terminal cleanup cannot erase in-flight event identity Confidence: high Scope-risk: low Reversibility: easy Tested: message pipeline, streaming edit, cancel-and-submit, metadata, escaped-turn suites
Private provisional callbacks and public message updates are independently cloned, so object identity cannot survive delayed handler interleavings. Derive a stable tool-call event key from call id, event type, index, and payload; keep it for handler lifetime while retaining turn-scoped raw-id cleanup. Lore-id: 9c41ad72 Constraint: cloned provisional events retain exactly-once safety identity Constraint: delayed handlers remain deduped after terminal cleanup Confidence: high Scope-risk: low Reversibility: easy Tested: streaming edit, cancel-and-submit, message pipeline, metadata, escaped-turn suites
Turn terminal events may interleave while an earlier message_update handler is suspended. Count provisional handlers at entry, defer stable-key retirement until terminal cleanup is pending and every handler settles, then clear keys so successor turns cannot inherit false provisional identity. Lore-id: 9c41ad72 Constraint: delayed handlers retain provisional identity Constraint: successor turns cannot reuse stale provisional keys Confidence: high Scope-risk: low Reversibility: easy Tested: message pipeline, streaming edit, cancel-and-submit package suites
Provisional update handlers can return or throw before the streaming safety section. Wrap the complete handler lifetime in the retirement finally so every admitted provisional event decrements exactly once and terminal cleanup can bound stable-key state. Lore-id: 9c41ad72 Constraint: early returns and errors cannot leak provisional handler ownership Confidence: high Scope-risk: low Reversibility: easy Tested: message pipeline, streaming edit, cancel-and-submit, metadata, escaped-turn suites
Keep accepted tool-call terminal events staged until consumer drain resolves. When a callback aborts after provisional commit, replace the pending assistant terminal snapshot with stopReason aborted before publishing message_end, preserving persisted/public lifecycle consistency with placeholder tool results and turn_end. Document both affected packages. Lore-id: 9c41ad72 Constraint: public message_end matches post-flush cancellation Constraint: normal tool-use publication remains ordered and exactly once Confidence: high Scope-risk: medium Reversibility: easy Tested: 127 focused lifecycle tests and affected package checks
Visible text commits callbacks and streaming updates but must not make the accepted assistant terminal immutable. Keep message_end and turn_end queued after text publication so a subscriber abort during later tool-call drain replaces the public terminal snapshot with aborted state while retaining exactly-once visible callbacks. Lore-id: 9c41ad72 Constraint: visible text remains live and exactly once Constraint: post-text cancellation rewrites pending terminal state Confidence: high Scope-risk: medium Reversibility: easy Tested: 830 affected tests and package checks
The successor changelog accidentally carried a truncated duplicate of the original escaped-argument note. Keep the complete current behavior note and the new terminal-ordering note without shipping corrupted release text. Lore-id: 9c41ad72 Confidence: high Scope-risk: low Reversibility: easy Tested: agent package check
A visible-text Harmony leak can retry only after its already-published lifecycle is closed. Flush the retained terminal event before dropping contaminated history and starting the retry, preventing orphaned message_start/message_update publication. Lore-id: 9c41ad72 Constraint: every visible attempt publishes one terminal boundary Confidence: high Scope-risk: low Reversibility: easy Tested: 830 affected tests and package checks
Document the visible-attempt terminal settlement added for Harmony leak retries so Unreleased accurately covers every shipped behavior change in this successor. Lore-id: 9c41ad72 Confidence: high Scope-risk: low Reversibility: easy Tested: agent package check
Close a visible contaminated attempt with an empty aborted assistant snapshot before retry. This preserves the public lifecycle boundary without persisting or replaying leaked Harmony control text. Lore-id: 9c41ad72 Constraint: contaminated Harmony text never enters durable history Confidence: high Scope-risk: low Reversibility: easy Tested: full 760-test agent suite and package check
The empty aborted Harmony retry terminal must not retain provider-native output items because replay prefers providerPayload over content. Remove that raw payload before publishing and persisting the sanitized terminal. Lore-id: 9c41ad72 Constraint: Harmony control envelopes cannot survive in provider replay metadata Confidence: high Scope-risk: low Reversibility: easy Tested: full 760-test agent suite and package check
Pin the attempt-transaction semantics that #4515 changes: callback abort after an accepted tool-call publishes exactly one aborted assistant terminal, emits paired placeholder tool results and turn_end, and never dispatches the tool. Cover the same contract after visible text has committed callbacks and updates, and restore the defensive accepted-message index guard. Lore-id: 9c41ad72 Constraint: retained terminals publish exactly once after commit Constraint: abort fall-through preserves tool call/result pairing Confidence: high Scope-risk: low Reversibility: easy Tested: 172 focused lifecycle tests, affected package checks, native and coding-agent builds
Add a base-failing Harmony integration regression proving visible leaked output closes as an empty aborted terminal without replayable provider payload before retry. Also pin consumer-drain abort ordering and exactly-once paired terminal/tool-result delivery after visible text. Lore-id: 9c41ad72 Constraint: regression fails on dev@34f5db331b with stop instead of aborted Constraint: retry request cannot contain leaked invoke envelope Confidence: high Scope-risk: low Reversibility: easy Tested: 763 agent tests, 172 focused tests, affected package checks, coding-agent build
b124619 to
b22d2ab
Compare
|
Superseding exact-head evidence for
@probepark exact-head re-review requested. Prior — gaebal-gajae |
|
@codex review exact head |
2 participants
Final postmerge correction for #4500 / #4489 after newer exact-head review and dev-shard evidence arrived after #4500 merged.
This successor contains only the reviewed closure beyond merged dev
3be6cd0e4e:Local evidence:
Closes #4489.
Signed-off-by: Yeachan Heo yeachan.heo@gmail.com
gajae.pr-review-verdict.v1 needs-human sha256:6f27ff774e9da3c6452854df0067811a12092f59c16a8a57a7869b6ae209ea16 reviewer:human reviewer-id:probepark evidence:#4515 (review)