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

Activity

1. Andiii208 commented on Oct 3, 2026

   

   Andiii208

   on Oct 3, 2026

   Last edited by Andiii208

   More actions

   Hi, I'd like to take this issue. I've read the relevant code on main and reproduced the problem; here's my analysis and proposed scope, and I'd appreciate your take on two decisions before/while I implement.

   Root cause (verified against atomic_agents/agents/atomic_agent.py)

   There are two independent Mode knobs, and they serve different purposes:

   1. The mode on the Instructor client (set at instructor.from_openai(..., mode=Mode.JSON)) determines the actual API call format. AtomicAgent.run() never passes a mode to Instructor, so this is decided entirely at factory time.
   2. AgentConfig.mode (atomic_agent.py:94, default Mode.TOOLS, introduced with the accurate token counting work in a978204) only drives the token accounting: _build_tools_definition() (atomic_agent.py:395) uses it to decide whether to count the schema as a tools definition or append it to the system message. That path only has an observable effect when max_context_tokens is set.

   Consequences, worst first:

   • Factory not set to Mode.JSON (default TOOLS): requests still go out with tools and the host rejects them. This is the failure mode reported here.
   • Modes disagree (e.g. client JSON, AgentConfig.mode left at default TOOLS): silent wrong token accounting. My repro shows the mismatch undercounting by 83 tokens for the same context (115 vs 198), so max_context_tokens users think they are within budget when the real request is larger, and trimming never triggers. No warning is raised.
   • Related latent inconsistency in the same area: _serialize_history_for_token_count() hardcodes item.to_openai(Mode.JSON) (atomic_agent.py:456) regardless of self.mode.

   Proposed scope

   • L1 — Derive the default. Make AgentConfig.mode Optional[Mode] = None; when None, AtomicAgent.__init__ derives it from the client (getattr(client, "mode", Mode.TOOLS)), so a single instructor.from_openai(..., mode=Mode.JSON) is enough for the common path. Backward compatible: explicit values keep their current meaning; making the field optional only widens it.
   • L2 — Warn on mismatch. When an explicitly passed mode disagrees with the client's mode, log an actionable warning. I'd compare by semantic family (tools-family vs JSON-family) so Mode.GENAI_TOOLS on the client vs Mode.TOOLS on the config is not flagged, and skip clients whose mode attribute is missing or not a Mode instance (test mocks use Mock(spec=...), and mode is an instance attribute, so it is not exposed on the spec'd mock).
   • L3 — Fix token accounting. Use the agent's effective mode instead of the hardcoded Mode.JSON in _serialize_history_for_token_count(), after verifying to_openai() behaves sanely across modes.
   • L4 — Docs. Make the "two knobs, must match" rule explicit in claude-plugin/atomic-agents/skills/framework/references/providers.md (the current note is one sentence under the matrix), docs/api/agents.md (AgentConfig.mode is not in the field list), docs/guides/quickstart.md, and the AgentConfig.mode field description itself.

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

   Two decisions for you

   1. L1 acceptable? Deriving the default from the client removes the "two places, must match" trap for new users; the alternative is to leave the default as TOOLS and only do L2–L4.
   2. Warn vs raise on mismatch (L2)? I lean warning: a raise risks breaking valid same-family setups, and the reported pain is silence, not absence of an exception. Happy to escalate to ValueError if you prefer.

   I'm starting the implementation now and will open the PR with the repro, the analysis above, and tests covering mode derivation, the family-aware warning, and mock-client safety. The PR is a request for review, so if you'd rather I adjust the direction (especially L1), just say so and I'll rework it.

   I'm developing this with an AI coding agent assisting, which I'll state in the PR description as well.
2. Andiii208 commented on Oct 3, 2026

   

   Andiii208

   on Oct 3, 2026

   More actions

   Update: the PR is open — #293

   It implements all four items from the proposed scope:

   1. L1 — AgentConfig.mode is now Optional[Mode] = None and follows the client's mode when omitted, so a single instructor.from_openai(..., mode=Mode.JSON) is enough. Explicit values keep their current meaning (the change only widens the field).
   2. L2 — an explicitly configured mode whose family disagrees with the client's 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 fall back to TOOLS as before.
   3. L3 — _serialize_history_for_token_count() now uses the agent's effective mode instead of a hardcoded Mode.JSON.
   4. L4 — the two knobs and their relationship are documented in the providers reference, the API reference (mode was missing from the AgentConfig field list), and the quickstart guide.

   The PR includes the repro from my analysis and 10 new tests; the full core suite passes (365 passed, 3 skipped — same 3 pre-existing live-integration skips as before, 355 was the count on a clean main).

   The PR is a request for review rather than a merge request, so the two questions from my first comment still stand:

   1. Is the L1 default-derivation acceptable, or would you rather keep the TOOLS default and only land L2–L4?
   2. Warning vs ValueError for the mismatch case — I chose warning to avoid false positives across provider-specific modes and to stay compatible with existing configurations.

   Happy to trim the PR to just L2–L4, or to escalate the warning to a raise, whichever you prefer.
3. Eigenwise

   added

   bugSomething isn't workingSomething isn't working

   on Oct 4, 2026
4. Eigenwise commented on Oct 4, 2026

   

   Eigenwise

   on Oct 4, 2026

   Owner

   More actions

   Confirmed with a mocked request: the Instructor factory sets the wire format, while AgentConfig.mode affects accounting and can silently disagree. For now, set Mode.JSON on both when using JSON mode. Keeping this open while the mode-consistency fix is reviewed.
5. Andiii208

   added a commit that references this issue on Oct 4, 2026

   fix(agents): align mode-driven token accounting with prepared requests

   ...

   478ef45