Skip to content

fix(agents): derive AgentConfig mode from client and warn on mismatch - #293

Open
Andiii208 wants to merge 5 commits into
Eigenwise:mainfrom
Andiii208:fix/282-agent-config-mode-consistency
Open

Andiii208 wants to merge 5 commits into
Eigenwise:mainfrom
Andiii208:fix/282-agent-config-mode-consistency

Conversation

@Andiii208

Copy link
Copy Markdown

Fixes #282.

Problem

Connecting an OpenAI-compatible Chat Completions host that does not speak the tools protocol requires Mode.JSON in two places: on instructor.from_openai(...) and on AgentConfig(...). Setting only one either fails at request time (factory left at the default Mode.TOOLS) or silently misaccounts tokens when max_context_tokens is set. Nothing explains the relationship between the two knobs, and a mismatched pair raises no warning.

Repro (JSON-mode host, max_context_tokens=8000, mode left at default on AgentConfig):

client.mode                = Mode.JSON
agent.mode (from config)   = Mode.TOOLS     # defaulted
token count (wrong)   = 115
token count (correct) = 198               # with mode aligned
# no warning or error raised

The wrong accounting undercounts, so a max_context_tokens user believes they are within budget while the real request is larger, and trimming never triggers.

Root cause

The two knobs serve different purposes:

  • The mode set at factory time on the Instructor client decides the API call format. AtomicAgent.run() never passes a mode to Instructor, so this is fixed at client creation.
  • AgentConfig.mode (on main: atomic-agents/atomic_agents/agents/atomic_agent.py:94, default Mode.TOOLS) only drives token accounting (_build_tools_definition(), line 395) and multimodal serialization. That path is only observable when max_context_tokens is set.

Also found while investigating: _serialize_history_for_token_count() hardcoded item.to_openai(Mode.JSON) (line 456) regardless of self.mode.

Changes

  1. AgentConfig.mode follows the client by default. The field is now Optional[Mode] = None; when omitted, AtomicAgent._resolve_mode() takes the client's mode, so setting the factory mode alone is enough. Explicit values keep their meaning — this only widens the field.
  2. Warn on mismatch. An explicitly configured mode whose family (tools-family vs JSON-family) disagrees with the client's mode logs an actionable warning. Family comparison avoids false positives for equivalent provider-specific modes (e.g. GENAI_TOOLS on the client with TOOLS on the config); clients that expose no Mode attribute (or test doubles of them) fall back to TOOLS as before, with no warning.
  3. Fix the hardcoded serialization mode. Multimodal history serialization now uses the agent's effective mode instead of Mode.JSON.
  4. Docs. The two knobs and their relationship are now stated in the providers reference (claude-plugin/atomic-agents/skills/framework/references/providers.md), the API reference (docs/api/agents.md, where mode was missing from the AgentConfig field list), and the quickstart guide.

Out of scope: the Instructor call path itself and other providers' behavior.

Tests

New file atomic-agents/tests/agents/test_agent_mode_consistency.py (10 cases):

  • mode derivation from JSON / TOOLS clients, and that the derived mode drives accounting (_build_tools_definition())
  • clients without an exposed mode fall back to TOOLS and keep explicit values without warning
  • same-family explicit modes never warn; cross-family modes warn with both mode names in the message
  • multimodal serialization passes the agent's effective mode to to_openai()

Verification

  • uv run pytest --cov=atomic_agents atomic-agents — 365 passed, 3 skipped (baseline was 355 passed, 3 skipped; the 3 skips are pre-existing live-integration skips)
  • uv run black --check and uv run flake8 --extend-exclude=.venv atomic-agents atomic-assembler atomic-examples atomic-forge — clean

Notes

  • I deliberately chose a warning over an exception for the mismatch case: mode families leave room for equivalent-but-different modes across providers, and the reported pain is silence. Happy to escalate to ValueError if you prefer.
  • I'm claiming this in the issue thread; if you'd rather I drop the default-derivation change (1) and keep only the warning plus docs, say so and I'll trim the PR.
  • Developed with an AI coding agent (ZCode) assisting; all analysis and changes above were verified by running the code and tests locally.

@Eigenwise Eigenwise left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Historical review of 68b71bf9. The findings below describe that head.

Two regressions block this head:

  • atomic_agent.py:529 passes Responses-format input_image content into the chat token counter. With a real Responses-mode Instructor client and image history, get_context_token_count() raises TokenCountError. Keep token-count media normalized for the counter, and test actual counting rather than only the mode passed to to_openai().
  • The derived native tools modes at :240/:276-277 are outside _build_tools_definition()'s whitelist. An ANTHROPIC_TOOLS client is accounted for with JSON-schema prose and no tools, while Instructor's prepared request has a tool and no such prose. Add native-provider accounting tests against prepared requests and align the mode families with the accounting behavior.

Both were reproduced offline. The 10 new tests and four existing focused cases pass, but omit these consumers. Deriving the mode is useful; these accounting paths need to agree before merge.

These two accounting findings are superseded by the offline check of afddb6e2, which exercised real Instructor request preparation and LiteLLM counting. The later source-only review of dbf421ed preserves those repairs and identifies the remaining input-type contract at atomic_agent.py:579. This historical test result does not establish a current-head pass.

Addresses review feedback on the mode-consistency fix for issue Eigenwise#282:

- Normalize multimodal history back to chat-format content parts for the
  token counter regardless of the agent's mode. The Responses-format parts
  (input_image) that item.to_openai(RESPONSES_TOOLS) produces raise
  TokenCountError in LiteLLM's chat counter, so deriving the mode from a
  Responses client made get_context_token_count() fail on image history.
- Replace the name-suffix mode family with an explicit set of modes whose
  prepared request carries the schema as a tool/function definition. The
  previous whitelist (TOOLS, TOOLS_STRICT, PARALLEL_TOOLS) left native
  provider modes such as ANTHROPIC_TOOLS counting the schema as JSON-schema
  prose while Instructor's prepared request sends a tool, so accounting
  silently disagreed with the request. COHERE_TOOLS stays out of the set:
  despite its name it embeds the schema in an instruction message.
- Cover both with tests that count real tokens and that prepare a request
  per Instructor mode and assert the accounting set matches what is sent.

Simplifications: _mode_family is gone (the warning now compares membership
in the same set that drives _build_tools_definition()), and the spy-based
serialization test is replaced by real counting tests.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@Andiii208
Andiii208 force-pushed the fix/282-agent-config-mode-consistency branch from 68b71bf to 478ef45 Compare October 4, 2026 14:20
@Andiii208

Copy link
Copy Markdown
Author

Thanks for the detailed review — both regressions reproduced offline and are fixed. Head is now 478ef45, rebased onto d2b61b9, CI green (395 passed, 17 skipped).

1. Responses-format input_image reaching the chat counter

_serialize_history_for_token_count() goes back to serializing media with a fixed chat-format mode (item.to_openai(Mode.JSON)) instead of the agent's mode, since LiteLLM's counter only accepts chat-format parts — Responses-format parts such as input_image raise. Deriving the mode from the client no longer changes how media is serialized.

Following your point about testing actual counting, the old spy test (which only asserted the mode passed to to_openai()) is replaced with tests that run LiteLLM's real counter:

  • image history counts in RESPONSES_TOOLS mode — the exact failure you found (get_context_token_count() used to raise TokenCountError);
  • the serialized content parts stay ["text", "image_url"] in that mode;
  • image history counts in TOOLS mode with tools > 0.

2. Derived native tools modes outside the whitelist

The name-suffix family helper is gone, together with the {TOOLS, TOOLS_STRICT, PARALLEL_TOOLS} whitelist. Both are replaced by one explicit set, _TOOL_MODES, that now drives _build_tools_definition() and the mismatch warning — so "family" and accounting behavior are the same thing by construction and cannot drift apart. The set contains every mode whose prepared request carries the schema as a tool/function definition (tools, functions, or toolConfig), verified mode by mode against handle_response_model(). One non-obvious case: COHERE_TOOLS is deliberately excluded — despite its name, its prepared request embeds the schema in an instruction message and sends no tool.

Tests against prepared requests:

  • a parametrized test prepares the request Instructor would send for every mode and asserts sends a tool definition == (mode in _TOOL_MODES). On the previous whitelist this fails for ANTHROPIC_TOOLS, ANTHROPIC_REASONING_TOOLS, MISTRAL_TOOLS, CEREBRAS_TOOLS, FIREWORKS_TOOLS, WRITER_TOOLS, BEDROCK_TOOLS, FUNCTIONS, RESPONSES_TOOLS, and RESPONSES_TOOLS_WITH_INBUILT_TOOLS — i.e. it pins the regression class you reported. Modes whose provider SDK isn't in the test environment (Gemini/GenAI/Vertex/xAI/Cohere) or that need an Iterable response model (the parallel modes) skip with the reason recorded;
  • ANTHROPIC_TOOLS also has a dedicated test: prepared request carries tools + tool_choice and no json_schema prose, and at agent level get_context_token_count().tools > 0 for real.

3. Simplification / complexity & coverage

_mode_family is removed entirely; _resolve_mode now compares set membership directly; the touched functions measure 0 cyclomatic complexity (flake8 --select=C901 --max-complexity=10 on both touched files reports nothing). Coverage on atomic_agent.py is 98%, with the only misses (540-548, 556) being the pre-existing media-serialization fallback branches, which this PR doesn't change.

Known approximation, unchanged from main: structured-outputs modes (JSON_SCHEMA, MISTRAL_STRUCTURED_OUTPUTS, GENAI_STRUCTURED_OUTPUTS, COHERE_JSON_SCHEMA, FIREWORKS_JSON, WRITER_JSON, PERPLEXITY_JSON, OPENROUTER_STRUCTURED_OUTPUTS) transmit the schema inside a response_format-style parameter that the counter doesn't count, and they still take the schema-prose path — a conservative overcount, same as before this PR. Say the word if you'd prefer they not be counted as prose.

The two questions from my first comment still stand: is the L1 default-derivation acceptable, and warning vs. ValueError on mismatch?

I developed this with an AI coding agent assisting, as before.

@Eigenwise

Copy link
Copy Markdown
Owner

Default derivation from the wrapped client's mode is acceptable. Keep explicit overrides and the mismatch warning; preserve the current structured-output approximation in this PR. The revised head still needs an independent final check. One measurement correction: a quiet C901 check at max-complexity10 doesn't establish CC0. The measured per-function complexity and coverage gate still applies to every touched function.

_serialize_history_for_token_count is a touched function, and the
per-function coverage gate applies to it: it measured 75% (6 missing
statements), all of them the media-serialization-failure and
unknown-part fallback branches. Add two tests exercising both
fallbacks, bringing the function to 100% (403 -> 405 passed).
@Andiii208

Copy link
Copy Markdown
Author

You're right on the measurement — a quiet C901 at max-complexity=10 only shows nothing exceeds 10, and I shouldn't have claimed "0 cyclomatic complexity". Measured values below are from radon (radon cc -s), per touched function in atomic_agent.py, with main's numbers for comparison:

function main head
AtomicAgent.__init__ 6 6
AtomicAgent._resolve_mode (new) — 6
AtomicAgent._build_tools_definition 2 2
AtomicAgent._serialize_history_for_token_count 8 8

File average is A (3.08); every touched function is within the configured max-complexity of 10.

On the coverage side the gate was genuinely failing, and you were right that it applies to the touched function: _serialize_history_for_token_count measured 75% — the 6 missing statements were exactly the media-serialization-failure fallback (540-548) and the unknown-part fallback (556), and "pre-existing" didn't excuse them since the function is touched. New commit ef38ea6 adds two tests exercising both fallbacks (a real Image whose to_openai fails → placeholder part plus warning; an unrecognized content part → text placeholder). Per-function statement coverage now measured from the full-suite run:

function stmts coverage
__init__ 17 100%
_resolve_mode 10 100%
_build_tools_definition 5 100%
_serialize_history_for_token_count 24 100%

Full suite: 405 passed (was 403), 17 skipped (the pre-existing live-integration skips), black and flake8 clean, CI green on ef38ea6.

Independent check on the two regressions, re-run from scratch rather than relying on the PR's tests:

  • Media: reproducing the old serialization (image serialized with the agent's mode) emits {'type': 'input_image', ...}, which the counter rejects with TokenCountError: Invalid content item type. On this head media is normalized to chat-format parts and counts fine (same as main's behavior).
  • ANTHROPIC_TOOLS: with the pre-PR usage the issue describes (explicit AgentConfig(mode=ANTHROPIC_TOOLS)), main accounts the schema as prose with no tools while the prepared request carries a tool definition; this head counts it as tools.

Design unchanged from what you approved: default derivation, explicit override, mismatch warning, and the structured-output approximation all stay as they are; ef38ea6 is test-only.

@Eigenwise

Eigenwise commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Historical complexity feedback on the then-current revision: the reported complexity 6, 6 and 8 did not meet our strictly-below-6 per-function requirement, even with full coverage. I asked for simpler initialization, mode resolution and media serialization while keeping the useful fallback regressions and override/warning behavior.

That production-complexity request is superseded: the offline review of afddb6e2 measured all ten changed production functions at CRAP 1 through 5. The later source-only dbf421ed review confirms the warning-test improvement and named lambda replacement; its remaining request is the accurate input contract at atomic_agent.py:579. The earlier execution results are historical; current-head tests remain unverified.

The per-function complexity gate (CRAP must stay strictly below 6 for each
modified function) left three touched functions over the bar: __init__ and
_resolve_mode at cyclomatic complexity 6, and the media serialization
inside _serialize_history_for_token_count at 8. Split each at meaningful
boundaries so the orchestrating functions read as a sequence of named
steps, with no behavior change:

- __init__ delegates the Gemini tool-result-role default to
  _default_tool_result_role(); the explicit-None check is preserved.
- _resolve_mode orchestrates _client_mode (mode detection),
  _modes_disagree (transmission comparison) and _warn_mode_mismatch
  (the warning), keeping derivation, override and warning behavior.
- _serialize_history_for_token_count maps each content part through
  _token_count_content_part(), which dispatches media to
  _serialize_media_for_token_count() (chat-format normalization plus the
  placeholder fallback on serialization failure).

Measured with radon (radon cc -s) on the touched file, per function,
with atomic_agent.py at 100% statement coverage from the full suite:

    __init__                          5 -> CRAP 5
    _default_tool_result_role (new)   2
    _client_mode (new)                2
    _modes_disagree (new)             2
    _warn_mode_mismatch (new)         1
    _resolve_mode                     6 -> 4
    _build_tools_definition           2
    _serialize_history_for_token_count 8 -> 4
    _token_count_content_part (new)   4
    _serialize_media_for_token_count  2

Full suite: 405 passed, 17 skipped; black and flake8 clean.
@Andiii208

Copy link
Copy Markdown
Author

Head is now afddb6e — a behavior-preserving simplification of exactly the three functions you flagged, split at meaningful boundaries. Design, the two regression fixes and the agreed override/warning behavior are all unchanged.

Per-function measurements (radon cc -s on the touched file; atomic_agent.py is at 100% statement coverage from the full suite, so CRAP = CC):

function before after
__init__ 6 5
_resolve_mode 6 4
_serialize_history_for_token_count 8 4

The named steps they now delegate to (new code, also measured): _default_tool_result_role 2, _client_mode 2, _modes_disagree 2, _warn_mode_mismatch 1, _token_count_content_part 4, _serialize_media_for_token_count 2. Every modified and added function is at CRAP ≤ 5, strictly below 6. File average moved from B (3.08) to A (2.78).

What the boundaries are:

  • __init__ now delegates only the Gemini tool-result-role default to _default_tool_result_role(); the explicit is not None override check is preserved verbatim.
  • _resolve_mode orchestrates three named steps — _client_mode (does the client expose a comparable Mode), _modes_disagree (do the two modes transmit the schema differently), _warn_mode_mismatch (the warning) — instead of inlining all three decisions.
  • _serialize_history_for_token_count maps each content part through _token_count_content_part(); media objects dispatch to _serialize_media_for_token_count(), which owns the chat-format normalization (to_openai(Mode.JSON)) and the placeholder-plus-warning fallback on serialization failure.

Verification:

  • Full suite: 405 passed, 17 skipped (unchanged from ef38ea6); atomic_agent.py at 100% statement coverage; black and flake8 clean.
  • Cross-checked the touched functions with flake8's mccabe at max-complexity 5 as well — the only flags in the file are _trim_context (8) and a module-level block (12), both untouched by this PR and pre-existing on main.
  • Independent check from scratch rather than the PR's tests, in a fresh process: deriving Mode.JSON from a JSON client and the both-sites-Mode.JSON setup issue OpenAI-compat: AtomicAgent Mode.JSON on factory and AgentConfig, not default tools mode #282 asks for both work with no warning; the mismatch case warns exactly once naming both modes; image history in RESPONSES_TOOLS mode counts (history/total > 0, parts stay ["text", "image_url"]); ANTHROPIC_TOOLS counts the schema as tools (tools > 0); JSON mode still builds no tools definition.

I developed this with an AI coding agent assisting, as before.

@Eigenwise Eigenwise left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Rechecked afddb6e2 with genuine offline calls to locked Instructor1.14.5 request preparation and LiteLLM1.80.9 counting behind a fail-closed guard. The old Responses image-history and native-tools accounting failures are repaired in the exercised paths. The focused suite ran42 passed/14 explicitly skipped; seven selected existing role/schema/media consumers also passed. No provider requests were sent.

All ten changed production functions have measured statement coverage and independent native Radon 6.0.1 CRAP scores from 1 through 5. The previous production complexity blockers are resolved. Preserve the revised behavior and the useful assertions.

At this head, the new client/content helper annotations still needed accurate contracts. The later source-only review of dbf421ed confirms the client and media types are repaired, the warning assertions are preserved and strengthened, and a named parametrization function replaces the lambda. The remaining supported type-contract finding is _token_count_content_part at atomic_agent.py:579, whose annotation excludes its unknown-object fallback.

Fourteen optional provider-preparation paths remain explicitly skipped, including unavailable SDK/configuration-dependent paths. Branch coverage, static typing, full-suite/style/docs and live E2E were not verified in this focused run. No source changes, installs, real credentials or provider requests were used. These counts and measurements belong to afddb6e2; the changed dbf421ed head was not executed in the later source check.

Replace the three new Any boundaries with accurate types: _client_mode takes
the Instructor client AgentConfig actually validates, and the content-part
helpers take the media union they dispatch on.
test_mismatching_accounting_warns measured CRAP 8: the caplog filtering and the
per-fragment assertions all lived in the test body. Extract them into
_agent_warnings/_assert_warns_once so every assertion is kept and each function
measures on its own, and name the parametrization id function so it can be
measured too.
@Andiii208

Copy link
Copy Markdown
Author

Head is now dbf421e (two commits on top of afddb6e). Both items from your review are addressed; the behavior and every assertion are unchanged.

1. The new production Any boundaries

  • _client_mode (was :272) takes instructor.Instructor — the type AgentConfig.client actually validates (a raw provider client is rejected by the config, so that annotation is the real contract). The getattr/isinstance guard stays for client doubles and custom clients that don't expose a comparable mode; the docstring now says that.
  • _token_count_content_part (was :575) takes Union[str, _InstructorMedia, Dict[str, Any]], where _InstructorMedia = Union[Image, Audio, PDF] — exactly the three branches it dispatches on, with the unknown-part branch documented as the string fallback.
  • _serialize_media_for_token_count (was :599) takes _InstructorMedia, the union its single call site narrows to.

The remaining Anys in the file are the pre-existing legacy Dict[str, Any] message/kwargs annotations you didn't flag.

2. Test-quality acceptance

One caveat up front: I could not find the approved assertion-oracle adjustment anywhere I can see — not in either review body, the issue thread, inline comments (there are none on this PR), or my mail. So I implemented what the phrase describes and kept the constraint you restated: every assertion is preserved, and the test is not reshaped around the diagnostic.

The caplog filtering and the per-fragment assertions are extracted into two named oracles at module level, _agent_warnings and _assert_warns_once, and the three tests that assert a warning mentioning mode names now call _assert_warns_once(caplog, ...). test_mismatching_accounting_warns keeps both of its own assertions (agent.mode and the oracle call). The two sibling tests gain a stronger check as a side effect: they previously took caplog.records[...][0] without a count, and now assert exactly one warning was logged.

The parametrization lambda at :248 is now the named _mode_id function, so radon measures it directly.

Measurements (uvx radon cc -s, native scores; atomic_agent.py is at 100% statement coverage — 314 statements, 0 missing — so CRAP = CC for every function in it):

Touched production functions, atomic_agent.py:

function CC CRAP
__init__ 5 5
_resolve_mode 4 4
_serialize_history_for_token_count 4 4
_token_count_content_part 4 4
_build_tools_definition 2 2
_default_tool_result_role 2 2
_client_mode 2 2
_modes_disagree 2 2
_warn_mode_mismatch 1 1
_serialize_media_for_token_count 2 2

Test file: test_mismatching_accounting_warns 8 → 2; test_native_provider_mode_against_json_accounting_warns 5 → 1; test_warning_mentions_both_directions 5 → 1; new _agent_warnings 4, _assert_warns_once 4, _mode_id 1. Every other function in the file was already ≤ 5 and is untouched.

Verification

  • Focused suite: 42 passed, 14 skipped — same as your run on afddb6e.
  • Full suite with coverage: 405 passed, 17 skipped (unchanged); atomic_agent.py 100%.
  • black and flake8 clean on both changed files.

I developed this with an AI coding agent assisting, as before.

@Eigenwise Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Rechecked dbf421ed. The client and media types are repaired. The warning tests keep their useful assertions, and the named parametrization function replaces the lambda. The earlier warning-test complexity finding is resolved.

One type-contract gap remains at atomic_agent.py:579: _token_count_content_part still uses Dict[str, Any], and its input annotation excludes the unknown objects it deliberately converts to text. The UnknownPart regression at test_agent_mode_consistency.py:247-255 covers that fallback. Please use an object input with narrowing, or another accurate contract that includes this supported path, without casts or broad ignores. The runtime fallback itself remains intact.

This recheck was source-only. I haven't run this head's tests, coverage, type checking or style checks. The earlier 42 passing tests, 14 skips and seven passing consumers belong to afddb6e2.

This branch has not been deployed

No deployments
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.

OpenAI-compat: AtomicAgent Mode.JSON on factory and AgentConfig, not default tools mode

2 participants