fix(agent): close provisional event-envelope regressions - #4515
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 |
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
|
Authenticated non-author exact-head approval received from @probepark: https://github.com/Yeachan-Heo/gajae-code/pull/4515#pullrequestreview-4933996391\n\nCanonical verdict is now bound to |
|
Superseding exact head |
|
@codex review exact head |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-approve at c740f411f0b012d941c1e055c06f9601b9708234 - closes a real serialization leak, and proves it
Head moved since my approval of b22d2ab4d. The delta is small (agent-loop.ts +2/-1, harmony-leak
test +7) but it is a genuine fix, not polish.
What changed
The synthetic Harmony retry terminal now goes through the lossless sanitizer before replacing the
pending assistant message:
- escapedToolTransaction.replacePendingAssistantMessage({ ... })
+ const sanitized = escapedToolTransaction.acceptedAssistantSnapshot({ ... });
+ escapedToolTransaction.replacePendingAssistantMessage(sanitized);Previously the raw object went straight in, carrying whatever the provider attached - including a
live Headers instance on transportFailure, which is not structured-cloneable. That connects
directly to the losslessDetachedClone transportFailure key list earlier in this PR: the allowlist
was extended to include headers, but this path bypassed the clone entirely.
It fails on the previous head - I checked
# prior head b22d2ab4d, with this head's test file applied
$ bun test packages/agent/test/agent-loop-harmony-leak.test.ts
(fail) agent-loop harmony-leak mitigation wiring (openai-codex) > publishes sanitized aborted terminal without replayable native payload before retry
4 pass 1 fail
# this head
26 pass 0 fail
The assertions are the right ones:
expect(retryTerminal.transportFailure).toEqual({ kind: "transport", status: 400 });
expect(() => structuredClone(retryTerminal)).not.toThrow();The first pins that the Headers object is stripped and only the serializable fields survive; the
second pins the actual consequence. Normally I would push back on not.toThrow() per the repo's
testing rules, but this one is narrow and load-bearing - structured-cloneability is exactly the
property that breaks on worker postMessage and session persistence, and it is paired with a precise
toEqual. Keep it.
Wider verification at the exact head
$ bun test .../agent-loop-escaped-nonascii-toolcall.test.ts .../managed-attempt-transaction.test.ts \
.../agent-loop-harmony-leak.test.ts .../agent-loop.test.ts \
.../cancel-and-submit.test.ts .../streaming-edit-abort.test.ts
147 pass
0 fail
645 expect() calls
$ bun --cwd=packages/agent run check -> exit 0
$ bun --cwd=packages/coding-agent run check -> exit 0
Verdict
merge-approved at c740f411f0b012d941c1e055c06f9601b9708234. Digest sha256:91f1d7a0cf32e2d9d40ae2dc03ebc43261ed3755a086012baa6ba3b03da9d51a recomputed independently.
Reviewed by @probepark - method: delta read against the previously approved head, applied the new test to the prior head to prove the differential, six-suite regression run and both package typechecks in a clean detached worktree at the exact head, independent digest recomputation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c740f411f0
ℹ️ 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".
| case "message_update": { | ||
| const message = this.#losslessSnapshot(event.message); | ||
| if (message.role !== "assistant") return { ...event, message }; | ||
| const assistantMessageEvent = this.#assistantEventSnapshot(event.assistantMessageEvent, message); |
There was a problem hiding this comment.
When a streamed partial contains the live Headers case exercised at this head, #losslessSnapshot(event.assistantMessageEvent) omits partial because cloning that property fails. #assistantEventSnapshot then checks "partial" in snapshot rather than the original event type, so this call publishes text/tool-call updates without their required partial; both message_update.assistantMessageEvent and onAssistantMessageEvent receive an invalid runtime shape. Reattach the detached message based on the original event or its discriminant.
Useful? React with 👍 / 👎.
|
New exact head Delta since your approval at
The fix discriminates on
- if ("partial" in snapshot) return { ...snapshot, partial: message };
- return snapshot;
+ if (snapshot.type === "toolChoiceIncapability") return snapshot;
+ return { ...snapshot, partial: message };Test coverage: the harmony-leak test now exercises the Local verification: 763 pass / 0 fail (clean env), agent typecheck clean. @probepark please re-approve at — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bf3c2e8d5
ℹ️ 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.#markRetryReplayUnsafe(); | ||
| this.#provisionalStreamingToolCallIds.clear(); | ||
| this.#provisionalStreamingRetirePending = true; | ||
| this.#retireProvisionalStreamingEventKeysIfIdle(); |
There was a problem hiding this comment.
When a host bridge or concurrent side attempt emits an assistant message_end while the main attempt still has staged provisional tool-call updates, this unconditional terminal cleanup clears both global identity sets before those main updates are published. The later cloned updates are then treated as non-provisional and run #preCacheStreamingEditFile and #maybeAbortStreamingEdit again, defeating the exactly-once safety path. Retire only identities belonging to the terminal event's attempt rather than clearing all provisional state.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 9bf3c2e8d - fixes a hole opened by the previous head's own sanitization
Delta since my approval of c740f411f: agent-loop.ts +2/-2, harmony-leak test +16/-2.
The change
// ManagedAttemptTransaction#assistantEventSnapshot
- if ("partial" in snapshot) return { ...snapshot, partial: message };
- return snapshot;
+ if (snapshot.type === "toolChoiceIncapability") return snapshot;
+ return { ...snapshot, partial: message };The old guard asked whether the snapshot still has a partial key, then re-attached the message.
But the lossless detach in the previous head is exactly what strips that key - so once detaching
started dropping partial, "partial" in snapshot went false and the re-attach silently stopped
happening. The event kept flowing with no back-reference to its message.
Inverting it - attach to everything except the one variant that legitimately has no partial - is
the right shape, because the condition now describes the event type rather than a property that the
sanitizer is free to remove. That is a self-inflicted hole from c740f411f closed one head later,
which is the kind of thing I would rather see caught here than in a session replay.
Differential
# prior head c740f411f, with this head's test file applied verbatim
(fail) agent-loop harmony-leak mitigation wiring (openai-codex) > publishes a sanitized aborted terminal without replayable native payload before retry
4 pass 1 fail
# head 9bf3c2e8d
26 pass 0 fail
The same test that pinned the sanitization now also pins that sanitizing does not cost the
partial linkage - it was extended rather than duplicated, which is the right way to grow it.
Wider verification at the exact head
$ bun test .../agent-loop-escaped-nonascii-toolcall.test.ts .../managed-attempt-transaction.test.ts \
.../agent-loop-harmony-leak.test.ts .../agent-loop.test.ts \
.../cancel-and-submit.test.ts .../streaming-edit-abort.test.ts
147 pass 0 fail 647 expect() calls
$ bun --cwd=packages/agent run check -> exit 0
$ bun --cwd=packages/coding-agent run check -> exit 0
Digest d9910bce92e45cec2f9576b1ae0f61b9ac28ba60f85e7e2258770de00b1912f1 recomputed independently,
matches.
Verdict
merge-approved at 9bf3c2e8d.
Worth noting the contrast with the #4459 series running in parallel: same cadence of small
follow-up heads, but every head here has arrived with a test that fails without it. That is why
these have been quick to review.
Reviewed by @probepark - method: delta read with attention to why the previous guard stopped firing, applied the head's test file to the prior head to prove the differential, six-suite regression run and both package typechecks in a clean detached worktree at the exact head, independent digest recomputation.
Reconstruction of PR #4515 head 9bf3c2e onto dev 64c1528 as a squashed fix-forward on branch owner/pr-4515-current-dev-0215: - packages/agent/src/agent-loop.ts: applied the PR delta over dev's ManagedAttemptSnapshotError stage work from #4546; the PR's new raw pre-measure overflow guard now throws the typed ManagedAttemptSnapshotError("staging.preMeasure") instead of the old zero-argument constructor, and "staging.preMeasure" joins the closed MANAGED_LOCAL_FAILURE_STAGES vocabulary (keeps #4546 fail-closed stage whitelisting intact for the new site). - packages/agent/CHANGELOG.md: union of dev Unreleased entries (managed snapshot diagnostics from #4546) and PR entries (Harmony retry terminal sanitation, provisional envelope staging, detached nonterminal publication). - packages/agent/test/agent-loop-escaped-nonascii-toolcall.test.ts and agent-loop-harmony-leak.test.ts: applied at dev shape. - packages/coding-agent/CHANGELOG.md: PR entry applied. - packages/coding-agent/src/session/agent-session.ts: intentionally NOT changed - verified the dev file already contains everything the PR series landed there (provisional key retirement, lossless envelopes); dev-side evolution since the merge-base (TTSR marking, fast-mode auto-disable, completion checks) stays intact. Local verification at this tree: - bun test packages/agent/test/ -> 774 pass, 0 fail (clean env; 1 env-only failure with local OPENAI_BASE_URL override, passes unset) - focused suites: escaped-nonascii 21 pass + harmony-leak 5 pass - bun --cwd=packages/agent run check -> biome + tsc clean Lore-id: pr4515-dev-0215 Confidence: high Scope-risk: narrow Tested: full packages/agent suite + focused regressions + typecheck Not-tested: full repo check:ts (next step) Supersedes: 9bf3c2e (PR #4515 head reconstruction)
Managed-fallback sessions (any default fallback chain with 2+ entries, per #managedFallbackPromptOptions) previously got ZERO resamples for \\uXXXX-escaped non-ASCII tool arguments: ManagedAttemptOutcome had no variant for the defect, the resample gate was hard-scoped !config.fallbackManaged, and the defective turn went straight from toolcall_end to the terminal per-call rejection - exactly the pre-#4491 behavior issue #4489 was opened to fix, silently applied to the worst-affected configuration (two-entry Anthropic fallback chains). Fix (disposition (a) from the #4489 review thread): - types.ts: ManagedAttemptOutcome gains escaped_arguments_discarded, carrying the discarded assistant turn and scope, deliberately with NO transport facts so it can never authorize provider fallback. - agent-loop.ts: the escaped-argument gate no longer excludes managed runs. Non-managed keeps the existing in-loop bounded resample. Managed discards the transaction, splices the provisional messages, reports escaped_arguments_discarded once, and ends the stream - the session policy owns re-entry. - agent-session.ts: #handleManagedAttemptOutcome answers the new outcome with discardStartedAttempt() (never charges the chain, never advances, never suppresses the selector) plus a retry continuation that re-issues the same request on the same model. Tests (agent-loop-escaped-nonascii-toolcall.test.ts): - replaces 'leaves managed fallback handling unchanged', which codified the gap as correct, with managed recovery coverage: typed outcome reported once, defective turn dropped from replay history, no tool execution, no surfaced rejection inside the managed run. - adds the deterministic discriminator probepark requested: persistently escaped sampling with distinct ids proves the gate spends its full budget per logical turn (6 turns x (1 + MAX_ESCAPED_NONASCII_RESAMPLES) wire attempts) and the run ends via the consecutive-malformed-turns breaker - budget exhaustion, not escapedToolTransaction.committed short-circuiting the resample. Verification (clean env): - focused suite: 24 pass / 0 fail - full packages/agent suite: 777 pass / 0 fail - session fallback suites: 36 pass / 0 fail - packages/agent + packages/coding-agent check (biome + tsc): clean Lore-id: pr4515-managed-recovery Confidence: high Scope-risk: narrow Tested: agent + session fallback suites, typecheck both packages Supersedes: none
9bf3c2e to
2db9edb
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Supersession of the old merge-approved verdict — fresh exact-head review requestedVerdict replaced. The body's
All approval/verdict/CI evidence at
What changed in the reconstruction (beyond rebasing the accepted series):
Local validation at exact head
@probepark @HaD0Yun — requesting fresh independent exact-head review at — |
OWNER_CONFIRMATION_REQUIRED — human hold at exact head
|
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 2db9edb86 - and the merge conflict is cleared
Head moved since my approval of 9bf3c2e8d, which had gone dirty. This head is rebased onto
96e718a2b and mergeable again.
$ bun test <the PR's touched suites>
29 pass
0 fail
$ bun --cwd=packages/agent run check -> exit 0
$ bun --cwd=packages/coding-agent run check -> exit 0
The substantive review stands from my earlier passes: the escaped non-ASCII recovery in managed
fallback, the terminal-sanitisation that keeps structuredClone viable, and the partial
re-attachment fix that closed the hole the sanitiser itself opened.
merge-approved at 2db9edb86.
Reviewed by @probepark - method: fresh-worktree run and both package typechecks at the exact head.
Freshness dependency hold — current dev is redThis PR remains open at exact head Current dev is not a valid green rebase target: push run Disposition: item-specific hold remains active behind #4574. After #4574 merges and dev CI is green, this PR must be reconciled to that exact new dev head, with refreshed digest, CI, and review before any merge-ready claim. — |
…ack (#4515) Models that spell non-ASCII text as hand-written \uXXXX escapes mistype hex digits, and a mistyped nibble decodes to a different but valid character, so the arguments can never be verified after parsing. Previously such turns executed silently on possibly corrupted text. PR #4515 fixes this on dev, but its commits sit atop a 12-commit provisional-envelope chain absent from this release branch, so the fix is authored here against 0.13.2 shapes instead of cherry-picked: wire-level detection in the ai providers, a managed escaped_arguments_discarded outcome in the loop, an uncharged same-model retry in the session, and a per-call execution rejection as the terminal answer. Constraint: release branch contains only patch-shaped changes -- no provisional-envelope machinery ported Constraint: detection must read the raw wire JSON; after decode the escape is indistinguishable from legit UTF-8 Rejected: cherry-picking the dev chain | 45 of 57 prerequisite commits conflict against this branch Rejected: loop-side retry budget | a managed retry re-enters the loop as a fresh run, so the bound lives in the session Confidence: medium Scope-risk: moderate Reversibility: easy Directive: file coverage for bedrock/cursor/ollama/codex-responses providers on dev, not here Tested: detector adversarial table (24 cases); managed discard/outcome; unmanaged per-call rejection; clean-call control Not-tested: live provider streams emitting escaped arguments end-to-end
33 commits over 0.13.2: hotfixes (#4437 #4424 #4481 #4446 #4453 #4452), session/storage resilience (#4396 #4411 #4373), security/stability (#4385 Synthetic models-endpoint validation, #4302 image-gen redaction), agent escaped-non-ASCII managed recovery (#4515 release variant), Kiro OAuth (#4304), Muse Spark 1.2, xAI + Grok CLI 4.6, Ouroboros/iTerm2 pet (#4468 #4499), integration docs. Release is HELD: no version bump, tag, or publish; binary stays gjc/0.13.2 until bun run release cuts 0.13.3 after dogfood verification. Pending before the cut: integration groups 3-4 (#4421 #4450 #4470 #4495), validation evidence, PR #4586 backport decision.
2 participants
#4489 closure: provisional event-envelope regression fix + managed-fallback escaped-non-ASCII recovery
Maintainer fix-forward of this PR onto exact current
dev:Reconstruction (head
9bf3c2e8d5→2db9edb864, base34f5db331b→96e718a2):partialstamping for nonterminal lossless events.packages/coding-agent/src/session/agent-session.tsneeded no reconstruction: currentdevalready contains everything the old series landed there, plus its newer evolution (TTSR marking, fast-mode auto-disable, fix(session): reconcile before-agent-start ordering after default-model admission redesign #4519 fenced ingress).agent-loop.tswas re-applied onto current dev'sManagedAttemptSnapshotErrorstage vocabulary (fix(agent): degrade benign managed-snapshot shape variations instead of failing the run #4546): the new raw pre-measure overflow guard now throwsManagedAttemptSnapshotError("staging.preMeasure")with the stage added to the closedMANAGED_LOCAL_FAILURE_STAGESlist, keeping fix(agent): degrade benign managed-snapshot shape variations instead of failing the run #4546's fail-closed stage whitelisting intact.packages/natives/native/index.d.tsdrift from a local rebuild was excluded (not part of this change).Managed-fallback recovery gap closed (disposition (a) from the #4489 review thread):
ManagedAttemptOutcomegains a typedescaped_arguments_discardedvariant carrying the discarded turn and scope — deliberately NO transport facts, so it can never authorize provider fallback.#handleManagedAttemptOutcomeanswers the new outcome withdiscardStartedAttempt()(never charges the chain, never advances models, never suppresses the selector, never mutates credentials) plus a bounded same-model retry continuation.leaves managed fallback handling unchanged— which codified the zero-recovery gap as correct — is replaced by managed recovery coverage: typed outcome reported once, defective turn dropped from replay history, no tool execution, no surfaced rejection inside the managed run.escapedToolTransaction.committedshort-circuit.Local evidence at exact head
2db9edb864on base96e718a2(clean env):packages/agentsuite: 777 pass / 0 failbun --cwd=packages/agent run checkand--cwd=packages/coding-agent run check(biome + tsc): cleanThe previous merge-approved verdict was bound to old head
9bf3c2e8d5(base34f5db331b) and is stale after the reconstruction push. This head carries no approval yet: fresh independent exact-head review is requested below before any merge.Closes #4489.
Signed-off-by: Yeachan Heo yeachan.heo@gmail.com
gajae.pr-review-verdict.v1 merge-approved sha256:14da6ba363a1b77f43a065ea3c57f3d5634796973b4a98f842ed3ef2ddfe1ea8 reviewer:human reviewer-id:probepark evidence:exact-head-2db9edb864-authenticated-approval-at-head-verified-digest-recomputed-clean-env-777-pass-fast-gate-green