fix(agent): steer escaped-non-ASCII resamples with a transient recovery instruction - #4632
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
e09d354 to
6db04cd
Compare
Exact-head review hold
— gaebal-gajae |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review — merge blocked.
major — the steering never reaches the path this PR was filed for
packages/agent/src/agent-loop.ts:2327-2347 installs the transient recovery instruction only after the fallbackManaged branch returns. Coding-agent sessions run with fallbackManaged: true and blindly continue the same request (packages/coding-agent/src/session/agent-session.ts:17786-17902), so the deterministic Hangul/ask failure cited in the PR body never sees the new instruction. The added test only exercises the unmanaged agentLoop.
Carry the transient instruction through the managed outcome/continuation, and add an AgentSession-level regression that reproduces the reported failure.
conflict with #4627
This PR and #4627 edit the same escaped-resample hunks and are semantically incompatible: this one preserves fail-closed terminal rejection after steered retries, #4627 removes that rejection and executes decoded arguments. They cannot be merged independently — settle on one design first. My recommendation is this PR's direction (keep the guard, add steering), not #4627's.
…nly tools Restore the fail-closed terminal escapedNonAsciiArguments rejection for every tool, then carve out exactly the motivating false positive: a tool that declares displaySafeEscapedArgs (user-facing question text that names no path, command, or identifier — currently only ask) executes without resampling when its decoded arguments contain nothing but benign typographic punctuation (curated set: U+2014 EM DASH). The json-parse scanner stays strict and still flags every non-ASCII escape; mutating tools and every other escaped character keep the two-resample budget and terminal rejection. Composes with the retained-guard + steering direction of #4632, which owns transient recovery steering. Red-team regressions cover: em-dash executes only on display-safe tools; the same payload on a mutating tool never executes; the nibble-adjacent en-dash (U+2013) and currency/math/full-width/separator/letter/emoji escapes stay rejected even on display-safe tools. Lore-id: 0f3c2a91 Constraint: mutating tools must never execute unverified \\uXXXX payloads Constraint: scanner stays evidence-based; the exemption is decided on decoded args at execution time Rejected: broad \p{P}\p{S}\p{Z} exemption | semantically significant nibble-sensitive symbols re-execute Rejected: execute decoded args after budget globally | review 4957008249 major blocker Confidence: high Scope-risk: narrow Reversibility: trivial Tested: 28 escaped-nonascii agent tests, 787 agent suite, 125 targeted matrix, 100 ai targeted Not-tested: none material Closes: #4627 review blockers
…nly tools Restore the fail-closed terminal escapedNonAsciiArguments rejection for every tool, then carve out exactly the motivating false positive: a tool that declares displaySafeEscapedArgs (user-facing question text that names no path, command, or identifier — currently only ask) executes without resampling when its decoded arguments contain nothing but benign typographic punctuation (curated set: U+2014 EM DASH). The json-parse scanner stays strict and still flags every non-ASCII escape; mutating tools and every other escaped character keep the two-resample budget and terminal rejection. Composes with the retained-guard + steering direction of #4632, which owns transient recovery steering. Red-team regressions cover: em-dash executes only on display-safe tools; the same payload on a mutating tool never executes; the nibble-adjacent en-dash (U+2013) and currency/math/full-width/separator/letter/emoji escapes stay rejected even on display-safe tools. Lore-id: 0f3c2a91 Constraint: mutating tools must never execute unverified \\uXXXX payloads Constraint: scanner stays evidence-based; the exemption is decided on decoded args at execution time Rejected: broad \p{P}\p{S}\p{Z} exemption | semantically significant nibble-sensitive symbols re-execute Rejected: execute decoded args after budget globally | review 4957008249 major blocker Confidence: high Scope-risk: narrow Reversibility: trivial Tested: 28 escaped-nonascii agent tests, 787 agent suite, 125 targeted matrix, 100 ai targeted Not-tested: none material Closes: #4627 review blockers
4d498f5 to
8fc8ccf
Compare
Reconstruction evidence — exact head 8fc8ccf
Review blocker resolution (CHANGES_REQUESTED at 6db04cd)major — steering never reached the fallbackManaged path. Fixed. Carrying the instruction through the managed continuation exposed a second latent defect, also fixed: the managed escaped retries are deliberately un-charged (the defect is not provider evidence), so each continuation is a fresh loop with a fresh in-loop resample budget and the fallback chain never exhausts — a deterministic escaper looped forever. Measured red: 2,940 provider calls in 5 s before an external timeout. The session now bounds escaped retries per logical run (steered retry + one blind retry, reset each user turn) and fails closed through the terminal exhaustion message; the added regression pins that (green: run terminal, Local validation on the exact head: managed steering + budget regression 2/2; agent escaped-guard suite 25/25; full Conflict with #4627 — explicit incompatibility note for that lane#4627 removes the terminal rejection and executes decoded @probepark — fresh exact-head re-review requested at 8fc8ccf (prior review/CI invalidated by the force-with-lease push, as required). Digest binding: base 2bd7b4a · head 8fc8ccf · diff sha256:233a4df96aabd746d52d46f0084a0cf3ae9c70d624c069e0f1c07da5d8360d36 · poster @Yeachan-Heo (authenticated repo owner session) — gaebal-gajae |
8fc8ccf to
f8e0621
Compare
Rebase reconciliation — exact head f8e0621
The review-blocker resolution described in the previous comment (managed continuation steering + AgentSession-level reproduction + the per-logical-run bound that stops the un-charged retry loop, with the 2,940-call/5 s red evidence) is unchanged and included at this head. The #4627 incompatibility note from the previous comment still stands: that lane's execute-decoded design is mutually exclusive with this PR's fail-closed guard and should be terminally closed or reworked by its owner. @probepark — re-review requested at f8e0621. — gaebal-gajae |
…nly tools Restore the fail-closed terminal escapedNonAsciiArguments rejection for every tool, then carve out exactly the motivating false positive: a tool that declares displaySafeEscapedArgs (user-facing question text that names no path, command, or identifier — currently only ask) executes without resampling when its decoded arguments contain nothing but benign typographic punctuation (curated set: U+2014 EM DASH). The json-parse scanner stays strict and still flags every non-ASCII escape; mutating tools and every other escaped character keep the two-resample budget and terminal rejection. Composes with the retained-guard + steering direction of #4632, which owns transient recovery steering. Red-team regressions cover: em-dash executes only on display-safe tools; the same payload on a mutating tool never executes; the nibble-adjacent en-dash (U+2013) and currency/math/full-width/separator/letter/emoji escapes stay rejected even on display-safe tools. Lore-id: 0f3c2a91 Constraint: mutating tools must never execute unverified \\uXXXX payloads Constraint: scanner stays evidence-based; the exemption is decided on decoded args at execution time Rejected: broad \p{P}\p{S}\p{Z} exemption | semantically significant nibble-sensitive symbols re-execute Rejected: execute decoded args after budget globally | review 4957008249 major blocker Confidence: high Scope-risk: narrow Reversibility: trivial Tested: 28 escaped-nonascii agent tests, 787 agent suite, 125 targeted matrix, 100 ai targeted Not-tested: none material Closes: #4627 review blockers
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review at exact head f8e06214 — approved. This is the design I recommended over #4627, and it does what it says.
the guard is genuinely still fail-closed
This was the whole point, so I checked it first rather than taking the body's word. agent-loop.ts:3523-3536 still throws unconditionally on toolCall.escapedNonAsciiArguments:
Escaped text cannot be verified — a single wrong hex digit silently becomes a different character — so the call was not executed.
No path executes decoded arguments. #4627 tried to make the terminal path permissive; this PR leaves it alone and works on the upstream problem instead — get the model to stop emitting escapes. That is the correct axis, and it means a mistyped nibble can never reach a mutating tool.
(Incidental evidence that the guard is live and well-tuned: it fired on one of my own tool calls earlier today and refused it. Working as designed.)
transient really is transient
agent-loop.ts:2175-2182 builds the steering message as { role: "user", content: recoveryContent, synthetic: true } and hands it to streamAssistantResponse as syntheticMessage (:2196-2202). It is never pushed into currentContext.messages, so it rides exactly one request and cannot pollute durable history, survive compaction, or bias a later turn. The new transientRecoveryMessage doc in types.ts states that contract explicitly, which is the right place for it.
The prompt lives at packages/agent/src/prompts/escaped-nonascii-recovery.md and is imported at agent-loop.ts:48 with with { type: "text" } — AGENTS.md's prompt convention, not an inline string.
the budget is bounded and the retry accounting is careful
:2387 gates on escapedNonAsciiResampleAttempt < MAX_ESCAPED_NONASCII_RESAMPLES, :2391 increments, :2430-2432 resets on any non-escaped outcome. Once spent, control falls through to executeToolCalls and the per-call rejection is the terminal answer.
Two details I want to call out as good, because they are the kind of thing that usually gets missed:
:2425refuses to displace a differentpendingRecovery. A malformed-tool-call one-shot recovery in flight keeps its mode and its one-shot accounting even if an escaped resample happens inside it. Clobbering that would have been an easy bug.:2404-2414reports the managed outcome asescaped_arguments_discardedcarryingsteeringPending, and deliberately carries no transport facts so the fallback chain does not advance on a wire defect. The session policy owns the bounded same-model retry and the instruction rides exactly once. Treating a spelling defect as provider evidence would have burned the fallback chain for nothing.
the false positive is fixed by construction
#4627's motivating complaint was a legitimate em dash tripping the guard. Here the model is told what it did wrong and re-emits literal UTF-8, so the correct text still lands — cost is at most MAX_ESCAPED_NONASCII_RESAMPLES extra samples on the affected turn, and only for a model that escapes in the first place. #4627 would have paid for the same case with an unverified payload. Bounded latency beats silent corruption.
session coupling is justified
The agent-session.ts change is the managed-retry continuation carrying the steering instruction exactly once when the discarded attempt had none. That state has to live where the retry policy lives, so the coupling follows the design rather than leaking into it.
type changes are additive
types.ts adds optional steeringPending?: boolean on the outcome and optional transientRecoveryMessage?: UserMessage. Both optional, no consumer breakage.
coverage
Three new tests, all real pins:
- steers the resample with a transient synthetic instruction and keeps tools enabled
- carries the transient steering instruction through the managed fallback retry
- fails closed after the managed escaped retry budget instead of looping forever
That third one is the test I would have demanded if it were missing — it pins the exact property that separates this PR from #4627.
Ran locally: bun test packages/agent/test/agent-loop-escaped-nonascii-toolcall.test.ts packages/coding-agent/test/agent-session-escaped-nonascii-managed-steering.test.ts → 27 pass, 0 fail, 133 assertions.
sweep
No any, ReturnType<>, inline dynamic imports, console.*, or mock.module() in the diff. packages/agent/CHANGELOG.md adds at line 10 and packages/coding-agent/CHANGELOG.md at line 9 — both inside ## [Unreleased] (line 3 in each), well above the first released section.
on #4627
This supersedes it. I blocked #4627 for executing decoded arguments after the budget and for opting Ask's whole argument object into a display-only contract; this PR keeps the terminal path fail-closed and fixes the false positive at the source instead. #4627 should be closed in favor of this one rather than rebased.
Reviewed by @probepark — method: detached worktree at f8e06214, direct verification that executeToolCalls still throws on escapedNonAsciiArguments, trace of the synthetic message from construction to request to confirm it never reaches durable history, budget increment/reset audit, pending-recovery displacement check, managed-outcome fallback-evidence check, focused test run with per-test pin reasoning.
gajae.pr-review-verdict.v1 merge-approved sha256:4a0b78bdbce493561b7c37b7d32b1cbb249704902a2e5e02d682d6dd64254f8d reviewer:human reviewer-id:probepark evidence:exact-head-f8e06214-terminal-guard-verified-fail-closed-27-pass-0-fail-focused-suites
…nly tools (#4627) Restore the fail-closed terminal escapedNonAsciiArguments rejection for every tool, then carve out exactly the motivating false positive: a tool that declares displaySafeEscapedArgs (user-facing question text that names no path, command, or identifier — currently only ask) executes without resampling when its decoded arguments contain nothing but benign typographic punctuation (curated set: U+2014 EM DASH). The json-parse scanner stays strict and still flags every non-ASCII escape; mutating tools and every other escaped character keep the two-resample budget and terminal rejection. Composes with the retained-guard + steering direction of #4632, which owns transient recovery steering. Red-team regressions cover: em-dash executes only on display-safe tools; the same payload on a mutating tool never executes; the nibble-adjacent en-dash (U+2013) and currency/math/full-width/separator/letter/emoji escapes stay rejected even on display-safe tools. Lore-id: 0f3c2a91 Constraint: mutating tools must never execute unverified \\uXXXX payloads Constraint: scanner stays evidence-based; the exemption is decided on decoded args at execution time Rejected: broad \p{P}\p{S}\p{Z} exemption | semantically significant nibble-sensitive symbols re-execute Rejected: execute decoded args after budget globally | review 4957008249 major blocker Confidence: high Scope-risk: narrow Reversibility: trivial Tested: 28 escaped-nonascii agent tests, 787 agent suite, 125 targeted matrix, 100 ai targeted Not-tested: none material Closes: #4627 review blockers Co-authored-by: Yeachan-Heo <yeachan-heo@gajae.dev>
f8e0621 to
167eb69
Compare
Rebase onto dev@648d13d72d (composed with #4627's landed guard work) — new exact head 167eb69
@probepark — fresh exact-head approval requested at 167eb69. The delta from the head you approved is only the rebase over dev (changelog conflict resolution + clean code replay); no behavioral change to the steering/continuation you reviewed. — gaebal-gajae |
|
@probepark — review checklist for exact head
The delta from your approved — gaebal-gajae |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head 167eb693 — merge blocked, on one mechanical defect. The substance is fine and my f8e06214 approval still stands.
correcting the record first
An automated pass flagged the terminal guard as no longer fail-closed at this head. That is misattributed, and I want it on record rather than propagating.
The isDisplaySafeEscapedArguments exemption is entirely inherited from the base (648d13d7, which is #4627). This PR's own delta touches it in zero lines:
git diff 648d13d7...167eb693 -- packages/agent/src/agent-loop.ts | grep -c '^+.*DISPLAY_SAFE_ESCAPED' -> 0
git show 648d13d7:packages/agent/src/agent-loop.ts | grep -c DISPLAY_SAFE_ESCAPED_CODEPOINTS -> 3
I approved that exemption on #4627 on its own merits, and the reasoning holds here: it is post-budget, path-scoped to questions.question / questions.options.label, and gated on DISPLAY_SAFE_ESCAPED_CODEPOINTS = new Set([0x2014]) — still exactly one codepoint at this head, still those two fields. A mistyped nibble in an em-dash escape yields a codepoint that is not in the set and is rejected, so the valid-but-wrong-text failure mode that motivated fail-closed cannot occur through it. #4632 did not weaken anything.
the six approved properties, re-verified
- Terminal rejection intact for every non-exempt path.
- Steering stays transient — the synthetic message is request-local and never pushed into
currentContext.messages. - Budget bounded: gate on
< MAX_ESCAPED_NONASCII_RESAMPLES, increment on entry, reset on any non-escaped outcome; managed retries separately capped. - Pending-recovery not clobbered — replacement only when there is no recovery or an existing escaped-nonascii one.
- Managed outcome carries only message/
steeringPending/scope, no transport facts, so the fallback chain does not advance on a wire defect. - Prompt is static
.mdimported as text.
The prior major (managed sessions retrying blindly) is fixed: the loop emits escaped_arguments_discarded and AgentSession attaches exactly one synthetic recovery message through transientRecoveryMessage.
blocking — the same changelog entry is added twice
packages/agent/CHANGELOG.md: this PR's delta adds two byte-identical entries for the steering change, adjacent to each other. Duplicated release notes, one-line delete.
While you are in there: the neighbouring entry describes the guard as keeping its "unconditional" terminal rejection. After #4627 that is no longer accurate — the rejection is unconditional except for the single-codepoint display-field exemption. Reword rather than leaving a changelog that asserts a stronger security property than the code has.
coverage
All three added tests survive and pin real behavior: unmanaged steering with transience and tools-enabled; managed continuation propagation with one-shot transience and discarded-turn removal; and the bounded managed retry with its terminal error.
The inherited post-budget rejection test still covers non-opted-in tools. It is a guard rather than a pin now, since the #4627 exception means it would also pass without a universal contract — worth knowing, not worth blocking.
merge precondition
Conflicts with dev. Rebase, then recompute the digest; this verdict is bound to 167eb693.
Reviewed by @probepark — method: detached worktree at 167eb693, git diff base...head restricted to agent-loop.ts to establish that the exemption is inherited rather than introduced, direct read of the codepoint set and opt-in field list, re-verification of all six approved properties, changelog delta inspection.
gajae.pr-review-verdict.v1 merge-blocked sha256:0323cddb11f54eae125ea131c0d127a3be54b72a419245a7c036a518df732d5a reviewer:human reviewer-id:probepark evidence:exact-head-167eb693-duplicate-changelog-entry-substance-unchanged-from-approved-f8e06214
167eb69 to
485f59c
Compare
…nly tools (Yeachan-Heo#4627) Restore the fail-closed terminal escapedNonAsciiArguments rejection for every tool, then carve out exactly the motivating false positive: a tool that declares displaySafeEscapedArgs (user-facing question text that names no path, command, or identifier — currently only ask) executes without resampling when its decoded arguments contain nothing but benign typographic punctuation (curated set: U+2014 EM DASH). The json-parse scanner stays strict and still flags every non-ASCII escape; mutating tools and every other escaped character keep the two-resample budget and terminal rejection. Composes with the retained-guard + steering direction of Yeachan-Heo#4632, which owns transient recovery steering. Red-team regressions cover: em-dash executes only on display-safe tools; the same payload on a mutating tool never executes; the nibble-adjacent en-dash (U+2013) and currency/math/full-width/separator/letter/emoji escapes stay rejected even on display-safe tools. Lore-id: 0f3c2a91 Constraint: mutating tools must never execute unverified \\uXXXX payloads Constraint: scanner stays evidence-based; the exemption is decided on decoded args at execution time Rejected: broad \p{P}\p{S}\p{Z} exemption | semantically significant nibble-sensitive symbols re-execute Rejected: execute decoded args after budget globally | review 4957008249 major blocker Confidence: high Scope-risk: narrow Reversibility: trivial Tested: 28 escaped-nonascii agent tests, 787 agent suite, 125 targeted matrix, 100 ai targeted Not-tested: none material Closes: Yeachan-Heo#4627 review blockers Co-authored-by: Yeachan-Heo <yeachan-heo@gajae.dev>
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 1 actionable issue, led by Managed escaped-argument retries exceed the stated bound. These findings require changes before approval.
Findings / Required Changes
- [P1] Managed escaped-argument retries exceed the stated bound.
Reference:packages/coding-agent/src/session/agent-session.ts:17913-17920
The counter allows two continuations, while each fresh loop can consume three provider calls (initial request plus two resamples), yielding up to nine calls for a permanently escaping model; the new test expects at most eight. Stop before scheduling the extra continuation or revise the bound and test consistently.
CI / Verification
- Reviewed the exact remote head:
485f59c7c3336c39806f98ea5a2a42fc304dccb2. - CI summary: 0 passing, 1 failing, 0 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | Compatibility boundaries are preserved by additive optional APIs and transient-only recovery state; no concrete breakage was established. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | A2 correctness risk established: managed fallback can exceed its documented escaped-argument retry/call bound; no separate concurrency defect was established. |
| A3. Security / Privacy / Trust | APPROVED | No actionable security or privacy risk was established; transient recovery content is explicitly kept out of durable history. |
| A4. Verification / Tests / CI | APPROVED | Tests cover unmanaged and managed escaped-non-ASCII retries, transient steering, bounded fallback, and history preservation; no regression is established, but CI impact remains inconclusive. |
| A5. Context / Compatibility / Platform | APPROVED | Integration, documentation, and platform behavior appear consistent; managed retries are bounded and transient, with CI failure remaining the only established limitation. |
Limitations
- The reported PR contract CI job is FAILURE, so a passing CI result cannot be claimed.
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The five-axis review completed against the exact head without actionable P0-P2 findings. The reviewed API, correctness, security, verification, and compatibility boundaries are approved.
Findings / Required Changes
None.
CI / Verification
- Reviewed the exact remote head:
485f59c7c3336c39806f98ea5a2a42fc304dccb2. - CI summary: 0 passing, 1 failing, 0 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | The API additions are backward-compatible and transient recovery behavior does not establish a durable compatibility break. |
| A2. Architecture / Correctness / Failure | APPROVED | The reviewed retry, steering, and transient-state paths preserve bounded logical-turn behavior; no correctness or concurrency risk was established. |
| A3. Security / Privacy / Trust | APPROVED | No new security or privacy risk was established in the changed recovery and managed-retry paths. |
| A4. Verification / Tests / CI | APPROVED | A4 conclusion: coverage for the new retry behavior is strong, but CI health remains unverified because the available PR contract check failed without diagnostics. |
| A5. Context / Compatibility / Platform | APPROVED | Integration and platform behavior appear compatible and fail closed; documentation and regression tests cover transient steering and bounded managed retries. |
Limitations
- The CI summary reports a failed PR contract job, so passing CI cannot be claimed.
Review blockers resolved — new exact head 485f59c (rebased onto dev@dadaf19880)Both points from the
Also resolved from that review's merge precondition: rebased off the conflicting base onto current
@probepark — fresh exact-head approval requested at — gaebal-gajae |
…ry instruction A blind resample only clears sampling accidents. A model that spells non-ASCII tool arguments as \uXXXX deterministically (observed: claude-fable-5 on Hangul-heavy ask payloads, 23 terminal rejections in one session, with real one-nibble corruption in the decoded text) reproduces the identical defect on every blind attempt, exhausting the resample budget and burning a full user-visible turn per tool call. Each unmanaged resample now carries a transient synthetic user message naming the defect and demanding literal UTF-8. The instruction never enters durable history, tools stay enabled, and the captured logical-turn tool choice is replayed across steered attempts so a queue-backed 'required' still lands on the accepted attempt. A pending one-shot malformed-tool-call recovery is never displaced by the steering, preserving its accounting. Lore-id: e5c1a9d2 Constraint: steering must stay out of durable history -- reuse the synthetic recoveryMode path Constraint: queue-backed tool choice must not be double-consumed across steered resamples Rejected: raising MAX_ESCAPED_NONASCII_RESAMPLES | more blind attempts cannot fix a deterministic escaper Rejected: decoding the escapes and executing | one mistyped nibble is unverifiable after parse (observed in the wild) Confidence: high Scope-risk: narrow Reversibility: trivial Tested: steered resample carries instruction, transient, tools enabled; all 25 escaped-guard tests; 789 agent package tests Not-tested: managed-fallback session retry path (returns before steering by design) (cherry picked from commit e09d354)
…ack continuation Review of the reconstructed change found the steering installed only after the fallbackManaged branch returns, so coding-agent sessions (which run managed and own their retry through the session policy) kept re-issuing the defective request blind. The deterministic Hangul escaper cited in the PR body never saw the instruction. The typed escaped_arguments_discarded outcome now reports whether the discarded attempt still lacked a transient recovery instruction, and the session's retry continuation attaches exactly one such instruction through a new transientRecoveryMessage prompt option, consumed by the loop as a one-shot synthetic recovery on the first assistant request of the continuation. The instruction stays provider-only (never durable history, never a later request) and the terminal per-call rejection remains fail-closed; Yeachan-Heo#4627's decode-and-execute direction is still rejected. Lore-id: b7d2e4f1 Constraint: instruction must never enter durable history -- rides the existing synthetic recoveryMode path Constraint: steering is one-shot per logical turn; never re-attach after a steered attempt was itself discarded Rejected: steering every managed retry | a steered attempt that still escapes proves the instruction is not working; repeating it burns budget on identical requests Rejected: decoding the escapes and executing | one mistyped nibble is unverifiable after parse Confidence: high Scope-risk: narrow Reversibility: trivial Tested: managed AgentSession regression (fails without the fix); all 25 agent escaped-guard tests; 790 agent package tests Not-tested: none known
485f59c to
570651d
Compare
Rebase onto live dev@ba7096f72 — new exact head 570651dThe contract's event base had advanced to
@snowykr @probepark — fresh exact-head approval requested at — gaebal-gajae |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head 570651da — approved. All four prior findings fixed, and the delta since my blocked head is mechanical: the steering prompt and both focused test files are blob-identical to 167eb693, so the logic was replayed onto dev rather than reworked.
- duplicate release note —
packages/agent/CHANGELOG.md:15now has exactly one steering entry - overstated rejection claim —
:13names the single post-budget, field-scoped exemption explicitly instead of implying unconditional rejection - managed retries cannot wedge — steering attached at
agent-session.ts:18193-18202, capped at:18185-18191, reset per user turn at:18066-18069 - rebase precondition — HEAD is based directly on
origin/dev
the five properties I care about for a resanitizer
Lossless for already-correct content. Only tool calls carrying escapedNonAsciiArguments enter resampling (agent-loop.ts:248-249, :2707-2720); literal UTF-8 executes unchanged, pinned at test:1183-1202. This was my main worry — a resanitizer that rewrites valid input is a regression for every well-formed message — and the marker-gated entry is the right shape.
No round-trip bypass. Detection scans raw string bytes before parse normalization and distinguishes real escapes from escaped backslashes (json-parse.ts:131-202), with red-team coverage for quotes, backslash parity, malformed and truncated escapes, surrogates, keys and arrays (json-parse.test.ts:57-140).
Nothing is rewritten before execution. The PR discards the defective turn, prompts a fresh response, and rechecks before dispatch (agent-loop.ts:2707-2751, :3875-3892, :3959-3974). That is materially safer than transforming arguments in place — there is no transformed value to validate, because the model produces a new one.
ASCII is identity. ASCII escapes are ignored (json-parse.test.ts:69-72, :123-130).
Coverage is real, including a mixed batch validated before any call executes (:423-454) and bounded fail-closed exhaustion.
one thing worth stating plainly
The accepted double-escaped form decodes to literal \u-prefixed source text rather than hidden non-ASCII (anthropic-escaped-nonascii-toolcall.test.ts:98-103). A downstream shell or language could later interpret that as an escape in its own syntax — but that is ordinary tool-argument semantics, not a mismatch this PR introduces, and the alternative (rewriting it) is what I did not want.
Also noted: the generic transformToolCallArguments hook runs after validation (agent-loop.ts:3956-3974), but this PR does not use it to resanitize; the coding-agent transformer only clamps timeout and resolves trusted local secret placeholders (session.ts:3524-3535). Good — that hook running post-validation is exactly why arguments should not be rewritten there.
Reviewed by @probepark — method: per-prior-finding status, blob comparison to separate rebase from rework, then traced whether the marker gate can admit already-correct content and where the resampled value is validated relative to dispatch.
gajae.pr-review-verdict.v1 merge-approved sha256:38b29e69eaa3c94973322cf711f483041172f1ff1193c3bac98a924eba82502b reviewer:human reviewer-id:probepark evidence:exact-head-570651da-all-four-prior-findings-fixed-and-resampling-is-marker-gated-with-revalidation-before-dispatch
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The five-axis review completed against the exact head without actionable P0-P2 findings. The reviewed API, correctness, security, verification, and compatibility boundaries are approved.
Findings / Required Changes
None.
CI / Verification
- Reviewed the exact remote head:
570651da22847d59641ef4937f586403f5bca4d0. - CI summary: 19 passing, 1 failing, 10 pending/cancelled/skipped.
- Failing checks:
PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | Optional recovery API additions preserve existing callers; transient steering and managed retry boundaries appear compatibility-safe, with platform coverage limited by skipped CI jobs. |
| A2. Architecture / Correctness / Failure | APPROVED | Changed retry steering preserves logical-turn tool choice, transient state, bounded managed retries, and durable-history isolation without an established concurrency defect. |
| A3. Security / Privacy / Trust | APPROVED | The change preserves transient handling of recovery instructions and shows no established security or privacy regression. |
| A4. Verification / Tests / CI | APPROVED | A4 verification is strong for affected retry behavior, with no regression found; skipped platform jobs limit broader compatibility confidence. |
| A5. Context / Compatibility / Platform | APPROVED | Integration and documentation behavior appear internally consistent; no defect was established, but skipped platform jobs limit cross-platform confidence. |
Limitations
- Dev CI PR contract bootstrap failed and Windows/native/darwin jobs were skipped, so the full platform matrix was not validated.
- Windows, native-build, and several platform jobs were skipped, limiting cross-platform verification.
|
Merging. Both approvals are bound to this exact head On the one red check: Verified before merging, with a real install and natives built:
Negative control: removing the marker propagation from the mock provider makes the guard tests fail and the run time out at 300s, because resampling no longer terminates. So Worth recording that my first two control attempts were void — one |
4 participants
What
Reconstructs contributor @lee98www's escaped-non-ASCII resample steering change onto current
dev(ba7096f72184929070f295019e8173c41ab1e468) (composed with #4627's retained-guard em-dash exemption), preserving the contributor's original commit authorship. Unmanaged retries now receive a transient instruction to write non-ASCII arguments as literal UTF-8; tools remain enabled, and a queued logical-turn tool choice is replayed without double consumption.Follow-up (maintainer, addresses the exact-head review at
6db04cddf7): the transient instruction is now carried through the managed fallback continuation as well, so coding-agentAgentSessionruns — which own their retries through the session policy — also steer instead of re-issuing blind:escaped_arguments_discardednow reportssteeringPending(whether the discarded attempt still lacked an instruction).AgentPromptOptions.transientRecoveryMessage, consumed by the loop as a one-shot synthetic recovery on the first request of the continuation.MAX_ESCAPED_NONASCII_MANAGED_RETRIES = 2): the retries are deliberately un-charged, so without this bound a deterministic escaper loops forever (measured: 2,940 provider calls in 5 s before an external timeout). The session now fails closed through the terminal exhaustion message; the budget resets each user turn.Why
A blind retry cannot correct a model that deterministically emits
\uXXXXtool arguments. Escaped arguments remain fail-closed because a mistyped nibble decodes into different but valid text. The original head only steered the unmanaged in-loop resample; coding-agent sessions run managed and never saw the instruction.Testing
bun test packages/coding-agent/test/agent-session-escaped-nonascii-managed-steering.test.ts— 2 pass: managed session retries carry the instruction exactly once (transient, tools enabled, defective turns never persisted); deterministic escaper fails closed after the bounded retries instead of looping (red without the bound: 2,940 calls in 5 s)bun test packages/agent/test/agent-loop-escaped-nonascii-toolcall.test.ts— 25 pass (unmanaged steering, budget exhaustion, managed typed outcomes)bun test packages/agent— 790 pass, 0 failbun test packages/coding-agent/test/agent-session-fallback-cancel-idle.test.ts packages/coding-agent/test/agent-session-fallback-upstream-count.e2e.test.ts packages/coding-agent/test/managed-publication-event-loop.test.ts packages/coding-agent/test/session-managed-append-overflow.test.ts packages/coding-agent/test/agent-session-escaped-nonascii-metadata.test.ts— 65 passbun --cwd=packages/agent run checkandbun --cwd=packages/coding-agent run check— Biome + TypeScript passgit diff --check origin/dev...HEAD— cleanRisk classification
low-risk— ordinary fix/maintenance; the repository owner may use the explicitmerge-self-approvedsolo verdict (no independent human review; the verdict name itself records this) with a risk-record comment bound to the exact head.regression-risk— fix with material regression risk; requires one assigned independent domain reviewer whose authenticated exact-headAPPROVEDreview the gate verifies (extra:independent:<login>; the token alone never suffices).high-risk— large refactor, feature, or materially high-risk change (security/auth/install/remove/public API/destructive lifecycle/architecture); requires one assigned independent domain reviewer with an authenticated exact-headAPPROVEDreview (extra:independent:<login>).GJC verdict
devbun checkpassesHead 485f59c: rebase onto dev@dadaf19880 plus the two mechanical review fixes (duplicate agent changelog entry removed; guard entry reworded to name the #4627 post-budget field-scoped exemption instead of claiming an unconditional rejection).
Head 570651d: pure re-replay onto dev@ba7096f72 (dev advanced 119+ commits past the previously approved base, making the contract event base unreadable from the fork); zero content change to the approved substance — changelogs still carry each entry exactly once with the reworded guard claim.