Skip to content

fix(coderd): preserve signed reasoning during provider-switch sanitization#27424

Open
ibetitsmike wants to merge 1 commit into
mainfrom
mike/chatd-provider-switch-signed-reasoning
Open

fix(coderd): preserve signed reasoning during provider-switch sanitization#27424
ibetitsmike wants to merge 1 commit into
mainfrom
mike/chatd-provider-switch-signed-reasoning

Conversation

@ibetitsmike

Copy link
Copy Markdown
Collaborator

Problem

Chats (including subagents) on dev.coder.com were permanently stuck with:

messages.N.content.M: `thinking` or `redacted_thinking` blocks in the latest
assistant message cannot be modified. These blocks must remain as they were
in the original response.

Anthropic validates that the latest assistant message replays byte-for-byte. Production logs and DB state show the failing chats hit this sequence on every generation attempt:

  1. Origin lookup for historical assistant rows used GetChatModelConfigByID, which filters deleted = FALSE. A model-config re-sync had soft-deleted the referenced config row, so resolution failed with ErrNoRows even though the row pointed at the same ai_provider_id as the active target config.
  2. Provider-switch sanitization fails closed: unknown origin means foreign, so it stripped the provider-executed web_search tool blocks out of assistant rows, including the latest assistant message carrying signed thinking blocks.
  3. Anthropic rejected the mutated replay with a 400, deterministically, on every retry.

Fix

  • Resolve origin identity with a new GetChatModelConfigByIDIncludeDeleted query and take only AIProviderID. A soft-deleted config still records which provider instance produced the row; enabled/deleted/model-resolvability gates are target-usability concerns and stay strict on the target path. Legacy configs without a provider FK remain fail-closed.
  • Defense in depth: when the wire provider is Anthropic (model.Provider(), so Bedrock-routed Anthropic counts), never strip rows in the trailing run of consecutive assistant rows when that run carries Anthropic signed or redacted reasoning. That run serializes into the latest assistant message on the wire. Rows or reasoning metadata that fail to parse inside the run fail closed and protect it. A WARN log with a protected_rows count fires when the protection engages.

Stripping only ever mutated in-memory prompt rows, never DB rows, so already-stuck chats recover on deploy without intervention.

Testing

  • New DB-level test for GetChatModelConfigByIDIncludeDeleted (soft-deleted row hidden from the strict query, returned by the new one).
  • New unit tests: signed/redacted/undecodable reasoning and unparseable rows protect the latest assistant run; signed rows outside the run still strip; nil predicate (non-Anthropic wire) strips as before; provider gate returns nil for openai/bedrock/empty; origin identity tolerates deleted+disabled configs and stays unresolved without a provider FK.
  • Full coderd/x/chatd, chatprompt, and dbauthz suites pass; make lint and make gen clean.

This PR was researched and written by Mux, an AI coding agent, on Mike's behalf. Root cause was confirmed against production logs and database state before the fix was written.

…ation

Provider-switch sanitization treated assistant rows whose model config was
soft-deleted (e.g. by a model re-sync) as foreign because origin lookup
used GetChatModelConfigByID, which filters deleted rows. It then stripped
provider-executed tool blocks from the latest assistant message, mutating
signed thinking blocks and causing Anthropic to reject every retry with
"thinking blocks in the latest assistant message cannot be modified".

Resolve origin identity with a new GetChatModelConfigByIDIncludeDeleted
query that ignores enabled/deleted state, since a soft-deleted config
still records which provider instance produced the row. As defense in
depth, never strip rows in the trailing run of consecutive assistant
rows when it carries Anthropic signed or redacted reasoning and the wire
provider is Anthropic; that run serializes into the latest assistant
message, which the API validates for replay fidelity. Rows or metadata
that fail to parse inside that run fail closed and protect the run.
@ibetitsmike

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 23e0b6d1ae

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

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.

1 participant