Repository navigation
Conversation
There was a problem hiding this comment.
Historical review of 68b71bf9. The findings below describe that head.
Two regressions block this head:
atomic_agent.py:529passes Responses-formatinput_imagecontent 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 toto_openai().- The derived native tools modes at
:240/:276-277are 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>
68b71bf to
478ef45
Compare
|
Thanks for the detailed review — both regressions reproduced offline and are fixed. Head is now 1. Responses-format
Following your point about testing actual counting, the old spy test (which only asserted the mode passed to
2. Derived native tools modes outside the whitelist The name-suffix family helper is gone, together with the Tests against prepared requests:
3. Simplification / complexity & coverage
Known approximation, unchanged from The two questions from my first comment still stand: is the L1 default-derivation acceptable, and warning vs. I developed this with an AI coding agent assisting, as before. |
|
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).
|
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 (
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:
Full suite: 405 passed (was 403), 17 skipped (the pre-existing live-integration skips), black and flake8 clean, CI green on Independent check on the two regressions, re-run from scratch rather than relying on the PR's tests:
Design unchanged from what you approved: default derivation, explicit override, mismatch warning, and the structured-output approximation all stay as they are; |
|
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 |
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.
|
Head is now Per-function measurements (
The named steps they now delegate to (new code, also measured): What the boundaries are:
Verification:
I developed this with an AI coding agent assisting, as before. |
There was a problem hiding this comment.
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.
|
Head is now 1. The new production
The remaining 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, The parametrization lambda at Measurements ( Touched production functions,
Test file: Verification
I developed this with an AI coding agent assisting, as before. |
Eigenwise
left a comment
There was a problem hiding this comment.
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.
Fixes #282.
Problem
Connecting an OpenAI-compatible Chat Completions host that does not speak the tools protocol requires
Mode.JSONin two places: oninstructor.from_openai(...)and onAgentConfig(...). Setting only one either fails at request time (factory left at the defaultMode.TOOLS) or silently misaccounts tokens whenmax_context_tokensis 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 onAgentConfig):The wrong accounting undercounts, so a
max_context_tokensuser believes they are within budget while the real request is larger, and trimming never triggers.Root cause
The two knobs serve different purposes:
AtomicAgent.run()never passes a mode to Instructor, so this is fixed at client creation.AgentConfig.mode(onmain:atomic-agents/atomic_agents/agents/atomic_agent.py:94, defaultMode.TOOLS) only drives token accounting (_build_tools_definition(), line 395) and multimodal serialization. That path is only observable whenmax_context_tokensis set.Also found while investigating:
_serialize_history_for_token_count()hardcodeditem.to_openai(Mode.JSON)(line 456) regardless ofself.mode.Changes
AgentConfig.modefollows the client by default. The field is nowOptional[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.GENAI_TOOLSon the client withTOOLSon the config); clients that expose noModeattribute (or test doubles of them) fall back toTOOLSas before, with no warning.Mode.JSON.claude-plugin/atomic-agents/skills/framework/references/providers.md), the API reference (docs/api/agents.md, wheremodewas missing from theAgentConfigfield 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):_build_tools_definition())TOOLSand keep explicit values without warningto_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 --checkanduv run flake8 --extend-exclude=.venv atomic-agents atomic-assembler atomic-examples atomic-forge— cleanNotes
ValueErrorif you prefer.