Skip to content

fix(ai): skip no-op thinking repair resends - #4389

Closed
jason931225 wants to merge 2 commits into
Yeachan-Heo:mainfrom
jason931225:fix/skip-noop-thinking-repair-resends
Closed

fix(ai): skip no-op thinking repair resends#4389
jason931225 wants to merge 2 commits into
Yeachan-Heo:mainfrom
jason931225:fix/skip-noop-thinking-repair-resends

Conversation

@jason931225

Copy link
Copy Markdown

Problem

The invalid-signature and blocks-immutable 400 arms of the Anthropic thinking-replay repair (streamAnthropic) fire unconditionally. But the repair can only change the request when native thinking/redacted_thinking blocks are actually in flight — without them, prepareParams() rebuilds a byte-identical replay. Each resend draws the same deterministic 400 and burns the repair budget (ANTHROPIC_MAX_THINKING_REPAIRS = 2) on no-ops before the error finally surfaces: up to two wasted full-size round trips per rejection.

The masked proxy-rejection arm already carries exactly this guard ("without signed thinking blocks in flight there is nothing to repair and the error must surface") — the other two classes were missing it.

Change

hasNativeThinkingBlocks(params.messages) is now the shared gate for all three trigger classes. A rejection that provably cannot be repaired surfaces immediately.

Test fallout (pre-existing vacuous fixture)

keeps thinking repair active when a later forced-tool_choice fallback rebuilds params relied on the removed no-op: prepareParams proactively strips native thinking whenever tool choice is forced ("A forced tool choice strips thinking from the request"), so that fixture's attempt 1 never carried sig_history, its "repair" resend never changed a byte, and its not.toContain("sig_history") assertions were vacuously true. Debug capture of its first request body confirms: only the text block goes to the wire.

The test's real intent — repair state survives a later fallback rebuild — is preserved with a fallback that genuinely coexists with replayed thinking: fast mode (serviceTier: "priority"speed 400 → rebuild). The reshaped test additionally asserts the first attempt really carried the signature, so it can never go vacuous again.

Tests

  • new: invalid-signature 400 with no thinking blocks in flight → surfaces on attempt 1, no resend
  • new: mutation 400 with no thinking blocks in flight → surfaces on attempt 1, no resend
  • reshaped: repair + fast-mode fallback composition (non-vacuous)
  • regenerated: issue-3670 eval source binding (sha256 of anthropic.ts) via WRITE_ISSUE_3670_EVAL=1, same as 31cea9d

bun test packages/ai/test/: 2316 pass / 0 fail (337 env-gated skips). Biome + tsc --noEmit clean.

The invalid-signature and blocks-immutable 400 arms of the Anthropic
thinking-replay repair fired unconditionally, but a repair can only
change the request when native thinking/redacted_thinking blocks are
actually in flight. Without them the rebuilt replay is byte-identical,
so each resend drew the same deterministic rejection and burned the
repair budget (ANTHROPIC_MAX_THINKING_REPAIRS) on no-ops before the
error finally surfaced — up to two wasted full-size round trips. The
masked proxy-rejection arm already carried this guard; it is now the
shared gate for every trigger class, and a rejection that provably
cannot be repaired surfaces immediately.

The forced-tool_choice composition test relied on the removed no-op:
prepareParams strips native thinking proactively whenever tool choice
is forced, so its 'repair' resend never changed a byte and its
signature assertions were vacuous. It now exercises the repair against
a fast-mode fallback rebuild, which genuinely coexists with replayed
thinking, and asserts the first attempt really carried the signature.

Regenerate the issue-3670 eval source binding for anthropic.ts.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f72e4776e

ℹ️ 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".

Comment on lines +9 to +10
"providerSourceBlobOid": "a654fcc1d261547c5fa525763058a57cb3dca230",
"providerSourceSha256": "c1fa95dcafdbb5e7f4bcc1863c73656bc0184c6614f7b4ac81f382a1bfacf449",
"providerSourceSha256": "8debae1adf66b013b4ce3b03c396915a4867bf551ac111acd1951827ec461188",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Bind the artifact to the changed provider blob

When this commit is checked out, HEAD:packages/ai/src/providers/anthropic.ts has blob OID f32963c12f37671f2158042ff87382cb9b72e502, but the regenerated artifact still records the parent blob a654fcc1d261547c5fa525763058a57cb3dca230. Consequently, anthropic-cache-eval.integration.test.ts builds an artifact using currentSourceIdentity() and unconditionally fails at expect(artifact).toEqual(derivedArtifact) (and later validateSource) even though the SHA-256 was updated; update providerSourceBlobOid alongside it.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 61588d3 — regenerated the artifact at the committed HEAD so providerSourceBlobOid now records f32963c12f37671f2158042ff87382cb9b72e502 (matching HEAD:packages/ai/src/providers/anthropic.ts). Both cache-eval integration tests pass against the committed identity.

The artifact was regenerated before the provider change was committed, so
providerSourceBlobOid still recorded the parent blob while the SHA-256 was
already updated. Regenerated at the committed HEAD so both identity fields
match HEAD:packages/ai/src/providers/anthropic.ts.
@Yeachan-Heo

Copy link
Copy Markdown
Owner

감사합니다, @jason931225. 이 문제는 dev 대상 PR #4388에서 먼저 수정되어 8e5c0e8a63200dafaae9fde2b5826e030fe83cbd로 머지됐고, #4382도 completed 처리됐습니다.

#4389는 main을 대상으로 하면서 현재 dev와 크게 분기되어 실제 수정 외 대규모 삭제·역행 diff가 포함되므로 그대로 통합할 수 없습니다. no-op thinking repair 차단이라는 핵심 방향과 artifact blob identity 보정 확인은 기록으로 남기고, 이미 terminal인 #4388에 의해 superseded된 중복 PR로 닫습니다.


[repo owner's gaebal-gajae (clawdbot) 🦞]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants