feat(llm): virtualize provider interface behind a normalized Adapter - #1683
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds normalized streaming adapters and content parsing, routes ChatBase execution through them, enables opt-in Anthropic extended thinking for reasoning models, exposes the setting in service profiles, and synchronizes the field based on model capabilities. ChangesUnified LLM streaming and Anthropic thinking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ChatBase
participant Adapter
participant Provider
Caller->>ChatBase: chat_string(prompt)
ChatBase->>Adapter: stream(prompt)
Adapter->>Provider: consume provider stream
Provider-->>Adapter: thinking and text events
Adapter-->>ChatBase: normalized events
ChatBase-->>Caller: callbacks and final text
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
2f016e1 to
46d6a16
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/llm_anthropic/services.json`:
- Around line 508-515: Remove "extendedThinking" from the properties array for
the anthropic.claude-3-haiku entry, leaving its API key and modelSource
properties unchanged.
In `@packages/ai/src/ai/common/chat.py`:
- Around line 406-411: Update the native OpenAI Responses path around
NativeOpenAIResponsesAdapter and drive_adapter to raise an exception when no
text is produced, matching the Anthropic native stream behavior. Ensure this
occurs before on_finish and return, so failed streams with finish_reason='error'
enter the existing fallback logic, while successful responses containing text
retain the current flow.
In `@packages/ai/src/ai/common/llm_adapter.py`:
- Around line 173-278: Remove the unused AnthropicAdapter and OpenAIAdapter
classes from this module, since production routing uses NativeAnthropicAdapter
and NativeOpenAIResponsesAdapter instead. Preserve LangChainAdapter and any
shared functionality that is actively referenced.
- Around line 288-322: Update NativeOpenAIResponsesAdapter.stream() at the
responses.create(...) call to pass store=False explicitly, matching
OpenAIAdapter’s stateless behavior. Add or synchronize the nearby
rationale/documentation so it states that Responses API requests and responses
should not be stored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1739171e-5c99-4817-aea9-b8ba208604b8
📒 Files selected for processing (15)
nodes/src/nodes/llm_anthropic/anthropic.pynodes/src/nodes/llm_anthropic/services.jsonnodes/test/agent_crewai/test_stop_words.pypackages/ai/src/ai/common/chat.pypackages/ai/src/ai/common/llm_adapter.pypackages/ai/src/ai/common/llm_native_stream.pypackages/ai/tests/ai/common/test_chat_content.pypackages/ai/tests/ai/common/test_chat_string_wiring.pypackages/ai/tests/ai/common/test_llm_adapter.pypackages/ai/tests/ai/common/test_llm_adapter_anthropic.pypackages/ai/tests/ai/common/test_llm_adapter_drive.pypackages/ai/tests/ai/common/test_llm_adapter_langchain.pypackages/ai/tests/ai/common/test_llm_adapter_openai.pypackages/ai/tests/ai/common/test_native_anthropic_adapter.pypackages/ai/tests/ai/common/test_native_openai_responses_adapter.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nodes/src/nodes/llm_anthropic/services.json (1)
319-366: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPlace
extendedThinkingbeforellm.cloud.modelSource.The affected arrays currently append
extendedThinkingafterllm.cloud.modelSource, buttools/sync_models/src/core/patcher.pyLines 338-345 andtools/sync_models/test/test_extended_thinking_field.pyLines 26-28 requirellm.cloud.modelSourceto be last. Reorder every affected array so the checked-in schema matches the synchronization contract.Proposed fix
- "llm.cloud.modelSource", - "extendedThinking" + "extendedThinking", + "llm.cloud.modelSource"Also applies to: 500-505, 519-608
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/llm_anthropic/services.json` around lines 319 - 366, Reorder the properties arrays for every affected Anthropic model entry, including anthropic.custom and the listed Claude models, so extendedThinking appears before llm.cloud.modelSource and llm.cloud.modelSource remains the final property. Apply this consistently across all occurrences covered by the review.tools/sync_models/src/core/patcher.py (1)
280-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
profilesargument.
_repair_field_objectsnow acceptsprofilesand uses it to add or removeextendedThinking, but the docstring only documentsfields. Add the argument and its capability-aware behavior.Proposed documentation update
Args: fields: The ``"fields"`` dict to mutate in-place + profiles: Optional complete profile mapping used to determine whether + each model supports reasoning🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/sync_models/src/core/patcher.py` around lines 280 - 305, Update the _repair_field_objects docstring to document the profiles argument, including that it is used to determine capability-aware additions or removals of the extendedThinking field. Also describe the expected optional mapping shape or absence behavior consistently with the function signature, without changing implementation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@nodes/src/nodes/llm_anthropic/services.json`:
- Around line 319-366: Reorder the properties arrays for every affected
Anthropic model entry, including anthropic.custom and the listed Claude models,
so extendedThinking appears before llm.cloud.modelSource and
llm.cloud.modelSource remains the final property. Apply this consistently across
all occurrences covered by the review.
In `@tools/sync_models/src/core/patcher.py`:
- Around line 280-305: Update the _repair_field_objects docstring to document
the profiles argument, including that it is used to determine capability-aware
additions or removals of the extendedThinking field. Also describe the expected
optional mapping shape or absence behavior consistently with the function
signature, without changing implementation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc76eb4a-7050-44b9-b7c4-842bcc34a8f3
📒 Files selected for processing (6)
nodes/src/nodes/llm_anthropic/services.jsonpackages/ai/src/ai/common/chat.pypackages/ai/src/ai/common/llm_adapter.pytools/sync_models/src/core/patcher.pytools/sync_models/src/providers/base.pytools/sync_models/test/test_extended_thinking_field.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/sync_models/src/core/patcher.py (1)
283-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new toggle behavior in both helper docstrings.
_repair_field_objectsnow acceptsprofiles, but itsArgssection omits that parameter._update_fields_for_addedalso conditionally addsextendedThinking, while its docstring still describes only the original three entries. Document the profile shape,Nonebehavior, and conditional property ordering.Also applies to: 357-390
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/sync_models/src/core/patcher.py` around lines 283 - 305, Update the docstrings for _repair_field_objects and _update_fields_for_added to document the profiles parameter, including its expected shape and behavior when it is None. In _update_fields_for_added, describe that extendedThinking is conditionally added and specify the resulting property ordering, while preserving documentation of the existing entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/sync_models/test/test_extended_thinking_field.py`:
- Around line 45-54: Update test_repair_skips_custom to include a custom object
whose properties omit extendedThinking, then run _repair_field_objects and
assert the property remains absent. Preserve the existing custom-object coverage
while ensuring the test fails if repair incorrectly adds the toggle to custom
objects.
---
Outside diff comments:
In `@tools/sync_models/src/core/patcher.py`:
- Around line 283-305: Update the docstrings for _repair_field_objects and
_update_fields_for_added to document the profiles parameter, including its
expected shape and behavior when it is None. In _update_fields_for_added,
describe that extendedThinking is conditionally added and specify the resulting
property ordering, while preserving documentation of the existing entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f0b6ef9-caac-40ab-92b9-09dd682dfa17
📒 Files selected for processing (2)
tools/sync_models/src/core/patcher.pytools/sync_models/test/test_extended_thinking_field.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nodes/src/nodes/llm_anthropic/services.json (1)
319-367: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
llm.cloud.modelSourceas the final property.The synchronization contract in
tools/sync_models/src/core/patcher.py:283-354explicitly keepsllm.cloud.modelSourcelast, but these new arrays appendextendedThinkingafter it. Reorder the entries consistently to avoid schema/UI ordering being repaired only later.Proposed fix
"properties": [ "llm.cloud.apikey", - "llm.cloud.modelSource", - "extendedThinking" + "extendedThinking", + "llm.cloud.modelSource" ]Apply this ordering to every affected profile.
Also applies to: 500-506, 515-609
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/llm_anthropic/services.json` around lines 319 - 367, Reorder the properties arrays for every affected Anthropic profile, including anthropic.custom and the listed Claude model profiles, so extendedThinking appears before llm.cloud.modelSource and llm.cloud.modelSource remains the final entry. Preserve all existing properties and apply the same ordering consistently across the affected profiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@nodes/src/nodes/llm_anthropic/services.json`:
- Around line 319-367: Reorder the properties arrays for every affected
Anthropic profile, including anthropic.custom and the listed Claude model
profiles, so extendedThinking appears before llm.cloud.modelSource and
llm.cloud.modelSource remains the final entry. Preserve all existing properties
and apply the same ordering consistently across the affected profiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e8289913-f116-420b-8a5c-dde9ee339616
📒 Files selected for processing (1)
nodes/src/nodes/llm_anthropic/services.json
f673c95 to
8314681
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nodes/src/nodes/llm_anthropic/services.json (1)
645-657: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose the setting for both new reasoning profiles.
claude-opus-5andclaude-opus-5-fastare reasoning-capable, but their field objects omitextendedThinking; users cannot opt in until a later sync repair. Insert it beforellm.cloud.modelSourcein both lists.Proposed fix
"properties": [ "llm.cloud.apikey", + "extendedThinking", "llm.cloud.modelSource" ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/llm_anthropic/services.json` around lines 645 - 657, Update the field lists for anthropic.claude-opus-5 and anthropic.claude-opus-5-fast to include extendedThinking immediately before llm.cloud.modelSource, preserving the existing properties in both profiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/llm_anthropic/services.json`:
- Around line 367-373: Remove or gate the extendedThinking property for the
Claude Haiku 4.5 service profile at
nodes/src/nodes/llm_anthropic/services.json:367-373 and the Claude Haiku Latest
profile at nodes/src/nodes/llm_anthropic/services.json:565-571. Use an
extended-thinking capability check rather than the general reasoning capability,
ensuring Haiku profiles do not expose an inert toggle.
In `@packages/ai/src/ai/common/llm_adapter.py`:
- Around line 210-253: Update NativeOpenAIResponsesAdapter.stream by wrapping
the event-processing loop over stream in a try/finally block, and call
stream.close() in finally. Preserve all existing event handling, history
updates, and completion behavior while ensuring the OpenAI stream closes on
normal, partial, and exception paths.
---
Outside diff comments:
In `@nodes/src/nodes/llm_anthropic/services.json`:
- Around line 645-657: Update the field lists for anthropic.claude-opus-5 and
anthropic.claude-opus-5-fast to include extendedThinking immediately before
llm.cloud.modelSource, preserving the existing properties in both profiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 970a0f54-66f4-401a-ade9-437eda7462c5
📒 Files selected for processing (16)
nodes/src/nodes/llm_anthropic/anthropic.pynodes/src/nodes/llm_anthropic/services.jsonnodes/test/agent_crewai/test_stop_words.pypackages/ai/src/ai/common/chat.pypackages/ai/src/ai/common/llm_adapter.pypackages/ai/src/ai/common/llm_native_stream.pypackages/ai/tests/ai/common/test_chat_content.pypackages/ai/tests/ai/common/test_chat_string_wiring.pypackages/ai/tests/ai/common/test_llm_adapter.pypackages/ai/tests/ai/common/test_llm_adapter_drive.pypackages/ai/tests/ai/common/test_llm_adapter_langchain.pypackages/ai/tests/ai/common/test_native_anthropic_adapter.pypackages/ai/tests/ai/common/test_native_openai_responses_adapter.pytools/sync_models/src/core/patcher.pytools/sync_models/src/providers/base.pytools/sync_models/test/test_extended_thinking_field.py
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks @dsapandora — this is a solid piece of work. The Event/Adapter split is the right shape for this problem, the RFC-first approach in #1679 paid off, and moving thinking to a per-call payload makes the "never on the agent path" rule structural instead of a comment.
Requesting changes on four points:
_chatnow streams, which silently disables the existing "falling back to non-streaming response" recovery inchat_string— and removes the only non-streaming route the agent /expectJsonpath had. Inline onchat.py.- CodeRabbit's still-current Major on the unclosed OpenAI Responses stream: #1683 (comment) — worth matching the
try/finally+close()thatNativeAnthropicAdapteralready has in the same PR. - Missing tests for the new behaviour: the no-text guard in
_chat_string_responses,response.failed→error,store=False, and theextendedThinkingconfig wiring in the node. Inline. nodes/src/nodes/llm_anthropic/README.mdis stale. The PR adds a public config field but the doc is not updated in the same change (co-located documentation rule). The Fields table has noextendedThinkingrow, and both the intro and the "Extended thinking" section still say the node "enables extended thinking automatically" fromcapabilities.reasoning. That is no longer true: it is opt-in, default off, and only on the interactive streaming path.
Rest of the detail is in the inline comments. CodeRabbit's Haiku toggle note (#1683 (comment)) is also worth a look — I found the same inert-toggle problem on anthropic.custom.
Non-blocking follow-up: now that LangChainAdapter strips <think> centrally, the _chat override in nodes/src/nodes/llm_minimax/minimax.py:87 is a redundant second implementation of the same strip (and it drops the stop-sequence kwarg) — a good candidate to delete, here or separately.
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks @dsapandora — this is a clean, well-tested refactor, and every finding from my earlier review is addressed at 8c5dacb5 (the non-streaming fallback now drains through LangChainAdapter.collect()/invoke(), parse_bool gates the toggle, claude-opus-5/-fast got the toggle, the custom profile is patcher-managed, and the OpenAI Responses path is covered by tests).
Requesting changes for one still-valid item: CodeRabbit's Major finding holds at head — NativeOpenAIResponsesAdapter.stream() never close()s the OpenAI Stream on the exception / partial-consumption path, while the sibling NativeAnthropicAdapter already wraps its loop in try/finally + close(): #1683 (comment)
Non-blocking: the inert Extended Thinking toggle on Haiku (CodeRabbit Minor #1683 (comment)) traces back to the model patcher gating on capabilities.reasoning alone, so it re-adds the toggle to reasoning-capable-but-thinking-incapable Haiku on every sync — worth an extended-thinking-specific gate rather than the general reasoning flag. CodeRabbit's earlier Major on the missing "no text" guard (#1683 (comment)) is already fixed at head (chat.py:404), so it does not block.
Address review on #1683: - NativeOpenAIResponsesAdapter.stream() now wraps the loop in try/finally and close()s the OpenAI Stream on raise / early GeneratorExit, matching NativeAnthropicAdapter. Prevents leaking the HTTP stream on partial reads. - build_anthropic_thinking_kwargs only rejects legacy Claude 3/3.5 Haiku; Haiku 4.5+ now gets extended thinking, so the extendedThinking toggle is no longer inert on those profiles. Tests: stream close-on-exception / close-on-early-break; Haiku 4.5 + latest get thinking, Claude 3 Haiku does not.
|
@asclearuc thanks for the thorough pass — both items are addressed in
Both covered by new unit tests. Ready for another look. |
…back Address review nit on #1683: the non-streaming fallback in _chat_string_responses did invoke()+flatten_content() by hand, a second copy of what LangChainAdapter.collect() already does. Route it through collect() so the "invoke and normalize" step lives in one place. Behaviour identical.
Pull the provider-shape → (text, reasoning) walker out of chat_string into _make_stream_content_parser, pinned by a characterization test. No behavior change.
Event (thinking/text/done) + Adapter Protocol with opaque history items — the seam ChatBase will consume. Not wired yet. RFC: discussion #1679.
Relocate _make_stream_content_parser / _make_think_tag_splitter into llm_adapter so adapters can reuse them without a chat↔adapter import cycle.
Wraps a LangChain chat model as an Event stream (reusing the shared parser); covers the non-reasoning providers. done.items is the assistant text turn.
Streams the Messages API (thinking/text deltas → Events); done.items is the assembled content with signatures intact — appended to history verbatim.
Streams the Responses API (reasoning/text deltas → Events); done.items are the output items with encrypted reasoning, extended into history verbatim.
Consumes an adapter's Event stream into the existing on_chunk/on_reasoning_chunk callbacks and returns (answer_text, opaque done.items). Not wired yet.
chat_string now streams via LangChainAdapter + drive_adapter instead of the inline walker; stop sequences and finish_reason preserved. Native/Responses paths unchanged.
_chat returned Anthropic's typed-block list verbatim, crashing agents/expectJson on .strip(); flatten_content keeps text blocks, drops thinking. Fixes the #1658 class.
NativeAnthropicAdapter yields Events over the same payload + create(stream=True) path; try_anthropic_native_chat_stream now drives it. Behavior preserved.
NativeOpenAIResponsesAdapter yields Events over responses.create(stream=True); _chat_string_responses drives it, keeping the non-streaming fallback. Behavior preserved.
_chat now drains the adapter (same iterator/normalization as streaming) instead of invoke+flatten, so content handling is one mechanism. Fallback kept for stream-less backends.
…gate Replace the interim hard-off (#1681) with an opt-in `extendedThinking` node toggle (default off). Thinking is no longer baked into the client; the native adapter adds it per call, so it rides the interactive streaming path only — agent/expectJson never gets it.
…ading _stream_anthropic_messages_api folded into NativeAnthropicAdapter on this branch; update the stop-threading test to drive the adapter. Same payload wiring, no behavior change.
The model sync now adds the extendedThinking UI toggle to reasoning-capable profiles (incl. models added on future syncs) and removes it from non-reasoning ones — the UI stays self-maintaining. Also drops the inert toggle from claude-3-haiku (non-reasoning).
…nses fallback - Remove unused AnthropicAdapter/OpenAIAdapter (.stream() wrappers) + their tests; production routes through the Native* adapters. - NativeOpenAIResponsesAdapter now passes store=False (no server-side retention). - _chat_string_responses raises on empty output so it falls back (matching the Anthropic path) instead of returning '' silently.
…ine the field The capabilities-aware repair only adds/manages extendedThinking on nodes whose fields define it (today: llm_anthropic), so it never leaks into other providers (openai, gemini, …) that neither define nor read the flag.
_chat drains through LangChainAdapter.collect() (invoke) instead of stream(), so the streaming fallback in chat_string recovers with a genuinely different mechanism again — while content normalization stays unified through the adapter. Addresses review #1.
…es tests - llm_anthropic: parse_bool for the extendedThinking config (handles 'false'/'0'/'no'). - services.json: add the toggle to claude-opus-5/-fast, drop it from the inert custom profile, and order extendedThinking before modelSource (the sync invariant). - patcher: manage 'custom' too (non-reasoning → toggle removed), so it self-heals. - NativeAnthropic/NativeOpenAIResponses adapters drop the unused history constructor arg. - Add Responses tests: failed→error, store=False, and the no-text → invoke fallback.
Address review on #1683: - NativeOpenAIResponsesAdapter.stream() now wraps the loop in try/finally and close()s the OpenAI Stream on raise / early GeneratorExit, matching NativeAnthropicAdapter. Prevents leaking the HTTP stream on partial reads. - build_anthropic_thinking_kwargs only rejects legacy Claude 3/3.5 Haiku; Haiku 4.5+ now gets extended thinking, so the extendedThinking toggle is no longer inert on those profiles. Tests: stream close-on-exception / close-on-early-break; Haiku 4.5 + latest get thinking, Claude 3 Haiku does not.
…back Address review nit on #1683: the non-streaming fallback in _chat_string_responses did invoke()+flatten_content() by hand, a second copy of what LangChainAdapter.collect() already does. Route it through collect() so the "invoke and normalize" step lives in one place. Behaviour identical.
The non-streaming fallback dropped the reasoning text after routing through LangChainAdapter.collect(), which returned visible text only. collect() now drains both parts and chat_string() forwards the reasoning to its own sink, keeping the block vocabulary in the adapter rather than a second flattener.
43cf4a5 to
989a6e7
Compare
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks @dsapandora — and thanks for taking the collect() suggestion further than I asked. 989a6e78 routing adapter.reasoning to on_reasoning_chunk is the right instinct: reasoning reaching the visible text is exactly the failure this PR exists to prevent.
Re-reviewed at 989a6e78. The collect() reuse is done and I have resolved that thread.
The rebase onto the newer develop is what changes the picture. While this PR was open, develop merged ai/common/utils/content_blocks.py — a shared flatten_content_blocks helper that centralised exactly the block vocabulary this PR reimplements. The rebase resolved that collision in favour of this branch's copy, which I do not think is what anyone intended. Two blockers come out of it, both inline:
- The block vocabulary is duplicated again.
chat.pyno longer importsflatten_content_blocks;llm_adapter.pycarries a private copy.agent/_internal/utils.pystill uses the shared one, so the repo ends up with two implementations of the same thing — the statedevelophad just finished removing. - A
developtest breaks.test_chat_return_contract.py::TestStringContent::test_plain_string_passes_throughfails against this head. I ran it to confirm. CI's build/test jobs are stillpendingon this commit, so it has not surfaced yet.
Still open from the last round, unchanged:
nodes/src/nodes/llm_anthropic/README.mdis still not updated. Repeating the table since the thread was lost in a dismissed review:
| Line | Currently says | Should say |
|---|---|---|
| 17-19 | the node "enables extended thinking automatically" | opt-in per node, default off, interactive streaming path only |
| 33-39 | Fields table | needs an extendedThinking row |
| 64-66 | "driven by the model's capabilities.reasoning flag" |
capabilities.reasoning and the new toggle |
| 71 | "Any Haiku model → None" | wrong since the Haiku fix — only legacy Claude 3/3.5 Haiku |
- The node-init test — still outstanding, inline again below.
Everything from my first review remains fixed; I re-ran the capability-vs-toggle audit after the rebase and it is still 0 mismatches across 22 profiles. Once the two rebase items and the two carried-over items land, this is ready.
The rebase onto develop left the adapter with a private copy of the block vocabulary that content_blocks.py had just centralised, and the two had already diverged: an unexpected content type was stringified there and dropped here. The parser now delegates the list case and keeps only the stateful <think> splitter, which is the split the shared helper was designed for. collect() also handed invoke() a one-element message list on a single turn, breaking the contract test that pins _chat passing the prompt string through. It now sends the string on a first turn and the history only once there is one.
test(llm_anthropic): cover the toggle from node config to native handler
|
Thanks for re-reviewing at 3.
I left the generated Summary of the two rebase items: Verified with the local engine: On the rebase itself — you read it right, and the duplicate was my resolution, not an intent. Worth flagging for whoever rebases next: |
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks @dsapandora — everything from the previous round is fixed, and I checked each one against the code rather than the commit message. This reads as ready.
The block vocabulary is shared again (fe436378). _make_stream_content_parser now delegates the list case to flatten_content_blocks and keeps only the <think> splitter, which is exactly the split the helper's docstring describes. The divergence I flagged is gone with it — flatten_content(42) now returns '42' like the shared helper, instead of silently dropping the content.
The develop contract test is fixed (fe436378). collect() hands the backend the prompt string on a single turn and the message list only once there is prior history. Worth recording that this one was real: the previous head 989a6e78 went red on all three platforms with 1 failed, 1863 passed, and the single failure was exactly this test:
packages/ai/tests/ai/common/test_chat_return_contract.py:59: in test_plain_string_passes_through
assert llm.prompts == ['what is 2+2?']
E AssertionError: assert [[{'role': 'user', 'content': 'what is 2+2?'}, ...]] == ['what is 2+2?']
I re-ran that case against this head and it passes, along with the rest of the contract file. test_collect_sends_the_history_once_the_turn_is_not_the_first pins the multi-turn half, which is the right place to put it.
The README is accurate now (9c318a08). All four rows I listed are corrected, including the Haiku row that the earlier gate change had invalidated, and the "Both must be on" wording removes the ambiguity the old text had.
The node-init test landed (9c318a08). nodes/test/llm_anthropic/test_extended_thinking_init.py drives the real chain from config through to _native_stream_provider, and parametrising the falsy strings ('false', 'False', '0', '') is a better guard than the single case I asked for — that is the one that fails the moment parse_bool is swapped back for bool().
No new findings. The only thing outstanding is mechanical: Build / Ubuntu 22.04, macOS, and Windows are still running on 9c318a08. I verified the previously failing test locally against this head, but worth letting the three builds go green before merging, given the last head was red.
2 participants
Summary
Virtualizes the LLM provider interface behind a normalized
Event/Adapterlayer soChatBasenever touches provider-native content shapes (RFC — discussion #1679). Single-turn.Event(thinking|text|done)+ anAdapterprotocol that owns provider-nativehistorywith opaqueitems. Adrive_adaptershim fans the Event stream into the existingon_chunk/on_reasoning_chunkcallbacks, so the node / SSEthinkingsurface is unchanged.LangChainAdapter(the ~18 non-reasoning providers, no raw-SDK rewrite),NativeAnthropicAdapter(Messages API),NativeOpenAIResponsesAdapter(Responses API). Every provider path now flows through the same interface.expectJson(agent) path returned Anthropic's typed-block list verbatim and crashed on.strip()._chatnow drains the adapter and returns a plain string; content handling is one mechanism on every path (the "unify" —_chat_with_retries's invoke branch is gone).extendedThinkingtoggle (default off). Thinking is no longer baked into the client; the native adapter adds it per call, so it rides the interactive streaming path only — agent /expectJsoncalls never get it (context gate). Model-gated to reasoning-capable models.isinstance(content, list), thinking/text/signature block parsing, betas,reasoning_content, Responses vs Messages) move into the adapters; the stream content parser is a shared, tested seam.Type
refactor + feat + fix
Testing
drive_adaptershim,chat_stringwiring, native adapters, per-call thinking injection, non-streaming drain.packages/ai/tests/ai/commonsuite green (605 passed), no regressions.Checklist
develop: thinking stays off by default (as in fix(llm_anthropic): disable extended thinking at the node (interim stop-gap) #1681); this adds the opt-in toggle.Notes
tool_use. Capture ofdone.itemsis already structural viadrive_adapter.Linked Issue
Fixes #1658
Summary by CodeRabbit