feat(schema): add max_tokens field to AgentDef for per-agent override - #471
feat(schema): add max_tokens field to AgentDef for per-agent override#471nskun (nskun) wants to merge 6 commits into
Conversation
|
@microsoft-github-policy-service agree |
ea17315 to
cd9a25b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #471 +/- ##
=======================================
Coverage ? 91.90%
=======================================
Files ? 144
Lines ? 23272
Branches ? 0
=======================================
Hits ? 21389
Misses ? 1883
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
The schema work here is solid. The field sits in the right place next to max_agent_iterations and session_key, the bounds agree with both RuntimeConfig.max_tokens and the runtime assertion at claude.py:365, and using is not None instead of copying the truthiness check on the line above was the right call. Ruff, ty and the full test_config suite are all green.
My concerns are all about what happens once the schema has accepted the value.
agent_builder.py:307 is the only place in the repo that reads this field, and it is reachable from claude.py alone. copilot, which is the default provider, has no max_tokens reference anywhere. hermes:283 reads only the runtime value. aca's AcaAgentPayload is extra="forbid" and does not list the field, so it cannot cross the wire. On four of five providers this validates clean and then quietly does nothing. The two fields immediately above it in AgentDef are plumbed into all five (copilot:1296,1301, hermes:247,252, claude_agent_sdk:883,891, aca:763,764), so the surrounding convention sets a different expectation.
On the one provider that does read it, _coerce_for_thinking can raise the value without saying so. max_tokens=1 with reasoning.effort=max comes out as 64000, and where the clamp-down branch at agent_builder.py:244 logs at INFO, the up-bump at :240-242 logs nothing at any level. A small per-agent cap on an agent that also sets reasoning is the most likely way anyone reaches for this field, so in its most probable configuration it does the reverse of what it advertises.
There is also an asymmetry with claude-agent-sdk. factory.py:195 raises ProviderError for runtime.max_tokens, and the comment above it says silently dropping the value "would quietly violate user intent". The per-agent field never reaches that check, so moving one line two levels deeper in the same YAML file gets past the guard. That also makes docs/providers/comparison.md:170 inaccurate, since it currently states both settings are rejected at the factory.
A ProviderCapabilities flag plus a check in _check_agent_capabilities would settle the first and third points together. session_key, max_session_seconds, working_dir and skills all use that shape already, and it would catch claude-agent-sdk at validate time rather than partway into a run.
Smaller things: the restriction lists in docs/workflow-syntax.md (lines 936, 1022, 1162) enumerate every rejected field and now omit this one, and the same lists appear in plugins/conductor/skills/conductor/references/authoring.md and yaml-schema.md, which ship to agents as skill content. There is no CHANGELOG.md entry under Unreleased.
Nothing above is a criticism of the mechanics, which are clean. Happy to look again once the provider question is settled.
| max_agent_iterations: 200 instead of using the default limit. | ||
| """ | ||
|
|
||
| max_tokens: int | None = Field(None, ge=1, le=200000) |
There was a problem hiding this comment.
This validates clean on copilot, hermes, aca and claude-agent-sdk, and then has no effect on any of them. agent_builder.py:307 is the only read in the repo and it only runs under claude.
The two fields directly above this one are honoured by every provider (copilot:1296,1301, hermes:247,252, claude_agent_sdk:883,891, aca:763,764), so anyone reading AgentDef would expect the same treatment here.
Two ways to close it. Either wire up the remaining four providers, or declare the support explicitly and let validation refuse the rest. The second is what session_key and max_session_seconds already do:
# providers/capabilities.py
max_tokens: bool = False
"""``True`` when the provider applies a per-agent ``max_tokens`` output cap.
``False`` means the value would be silently ignored, so workflows that set it
fail validation instead."""# config/validator.py, inside _check_agent_capabilities
if agent.max_tokens is not None and not caps.max_tokens:
errors.append(
f"Agent '{agent.name}' sets max_tokens={agent.max_tokens!r} but provider "
f"'{provider_name}' does not apply per-agent output token caps "
f"(capabilities.max_tokens=False). Remove it, use runtime.max_tokens where "
f"the provider honours it, or override the agent to a provider that does."
)Then set max_tokens=True on ClaudeProvider and leave the default everywhere else. That also picks up claude-agent-sdk, which today refuses runtime.max_tokens at factory.py:195 but lets this one through.
| Overrides the workflow-level runtime.max_tokens for this agent. | ||
| Only applies to provider-backed agents (not script or human_gate). |
There was a problem hiding this comment.
human_gate accepts max_tokens as things stand. I checked against a valid gate fixture and it takes the field without complaint, even though it rejects reasoning and session_key. questions accepts it too, which is odder still given it rejects model with the reason "no provider is invoked".
max_session_seconds and max_agent_iterations have the same hole, so this is inherited rather than introduced. But this is the line where max_tokens's contract gets written down, and it currently names the one type the code does not cover.
| Overrides the workflow-level runtime.max_tokens for this agent. | |
| Only applies to provider-backed agents (not script or human_gate). | |
| Overrides the workflow-level runtime.max_tokens for this agent. Controls | |
| response length, not the context window (that budget is context.max_tokens). | |
| Rejected on script, workflow, wait, set, and terminate steps. |
| if self.max_tokens is not None: | ||
| raise ValueError("script agents cannot have 'max_tokens'") |
There was a problem hiding this comment.
This block is repeated verbatim five times, and the copying is what let human_gate and questions slip through.
validate_agent_type already has a standalone-guard idiom for this, at lines 1929, 1944 and 1956. The comment on the stdin one states the reasoning outright: being a standalone guard rather than a per-branch check, it also covers the types that have no branch of their own.
The same shape replaces all five and closes the gap:
if self.type not in (None, "agent") and self.max_tokens is not None:
raise ValueError(
f"'{self.type}' agents cannot have 'max_tokens' "
"(only provider-backed agents support this field)"
)It is a net reduction in lines, and nothing existing can break: extra="forbid" meant no workflow could carry the field at all before this PR, so the new rejection is strictly tighter than nothing. validator.py:1733 already defines _LLM_AGENT_TYPES = frozenset({None, "agent"}) if you would rather have one source of truth, and schema.py does not import validator.py, so there is no cycle.
Your five existing rejection tests still pass against this, since they only match on the field name.
| assert agent.max_session_seconds == 90.0 | ||
|
|
||
|
|
||
| class TestAgentDefMaxTokens: |
There was a problem hiding this comment.
All eleven tests here exercise AgentDef.__init__. None of them reach the line that consumes the field, which was dead code until this PR. Delete agent_builder.py:307-308 and this suite still goes green.
tests/test_integration/test_parameter_flow_verification.py was written for this exact worry (its module docstring names it) and already covers the workflow-level value. The cheapest addition is two synchronous tests in test_pydantic_ai_agent_builder.py::TestSamplingSettings:
def test_agent_max_tokens_overrides_workflow_default(self) -> None:
"""A per-agent max_tokens must win over the workflow-level default."""
agent_def = AgentDef(name="sampler", max_tokens=1000)
pydantic_agent = build_agent(
agent_def, system_prompt="", rendered_prompt="", default_max_tokens=4096
)
assert pydantic_agent.model_settings["max_tokens"] == 1000
def test_workflow_default_used_when_agent_max_tokens_unset(self) -> None:
"""With no per-agent override the workflow default still applies."""
agent_def = AgentDef(name="sampler")
pydantic_agent = build_agent(
agent_def, system_prompt="", rendered_prompt="", default_max_tokens=4096
)
assert pydantic_agent.model_settings["max_tokens"] == 4096I ran both against this branch and they pass.
One gap in the range coverage too: 0, -100 and 200001 are all tested, but the accepted endpoints 1 and 200000 are not, so swapping ge/le for gt/lt would go unnoticed. TestAgentDefMaxSessionSeconds has test_minimum_boundary for the same reason.
Worth adding a case for reasoning as well. max_tokens=1000 with reasoning.effort=low currently produces model_settings["max_tokens"] == 6144, which may be correct for the Anthropic API but is worth pinning so it cannot drift unnoticed.
| """Test that script agents cannot have max_tokens.""" | ||
| with pytest.raises(ValidationError) as exc_info: | ||
| AgentDef(name="s", type="script", command="echo hi", max_tokens=8192) | ||
| assert "max_tokens" in str(exc_info.value) |
There was a problem hiding this comment.
This assertion cannot fail for the reason it looks like it is checking. Pydantic v2 echoes the input dict into the error message, so "max_tokens" in str(exc_info.value) is true for any ValidationError raised on this input. Drop the command kwarg and it still passes, on a completely unrelated "script agents require 'command'" error.
pytest.raises is carrying the test on its own here. The range tests further up (line 687) already match on real message text, so this is inconsistent within the same class rather than a house style:
| assert "max_tokens" in str(exc_info.value) | |
| assert "script agents cannot have 'max_tokens'" in str(exc_info.value) |
Same applies to the workflow, wait, set and terminate cases below.
|
Thank you so much for the incredibly thorough review — I really appreciate the time and care you put into it. |
|
Thank you again for the detailed review. I’ve pushed an initial update addressing the concrete issues you identified. My original intent was to support the stable-tier providers, Copilot and Claude. Claude can apply a per-agent output-token setting through Pydantic AI, but the current Copilot SDK does not expose an equivalent setting that Conductor can pass through. The initial update therefore followed the second approach you suggested: it enabled per-agent The current update includes:
The directly related test selection passes ( This is an interim update. I’m still working through the remaining changes and will post a follow-up when the PR is ready for another review. |
…c-ai providers (claude, openai) (#503) * build(deps): add pydantic-ai-harness and bump pydantic-ai-slim floor * feat(providers): add context-window and output-limit resolution cascades for compaction * feat(providers): assemble tiered compaction capability for pydantic-ai agents * feat(providers): enable always-on tiered compaction for claude and openai providers * feat(providers): emit compaction lifecycle events for pydantic-ai agents * feat(cli): render compaction lifecycle events in console output - Add ConsoleEventSubscriber branches for agent_compaction_config, start, and complete. - Use styled()/join() for markup-safe interpolation per AGENTS.md rules. - Extend test_logging.py with config/start/success/failure render tests. - Add JSONL verbatim round-trip test and replay skip-list guard. * feat(dashboard): show compaction lifecycle events in the activity stream - Add agent_compaction_config/start/complete to the EventType union and payload interfaces. - Append activity-log entries for compaction lifecycle events on the active node. - Add Vitest coverage for config/start/complete/success/error and replay-state neutrality. - Regenerate static/ assets via make build-frontend. * docs: document automatic context compaction for pydantic-ai providers * feat(providers): unify default max_tokens at 16384 for claude and openai * feat(providers): resolve effective anthropic max_tokens and read model output cap for compaction * fix(providers): additive compaction reserve from effective max_tokens and tool-output config * docs: align compaction reserve docs with effective-max-tokens formula * feat(providers): add vendor model-listing token-limit parser for compaction * refactor(providers): drop the global compaction context-window env override * fix(providers): paginate the full Anthropic model listing for token-limit metadata * feat(providers): read vendor-advertised model token limits in the OpenAI provider * fix(providers): handle OpenAI SDK AsyncPaginator in model metadata fetch * test(providers): pin compaction cascade behavior with provider-advertised limits * docs: document provider-advertised token limits and drop the window env override * fix(providers): harden compaction metadata parsing and close lifecycle Defensive model-listing parser for hostile Mapping/property access. Broad exception handling in Claude metadata cache population. Reset Claude unavailable-listing latches in close(). Restore OpenAI get_model_capabilities token fields to None and plain in membership. Add regression tests for parser hostility, proxy-prefix negative pin, and close reset. * fix(providers): emit compaction start event before the inner strategy runs The fail-open wrapper emitted agent_compaction_start only after the inner tiered strategy (including the summarizer's network call) had already returned, and hardcoded elapsed to 0.0, so console/JSONL/dashboard showed an instant start/complete pair instead of a live lifecycle. Emit start immediately before delegating when the estimate crosses the trigger, and measure the real elapsed time. * fix(providers): warn only for explicit models and single-flight model listing The available-models warning fired for the hardcoded provider default (e.g. gpt-5-mini) even when every agent overrides it with its own model:, producing a misleading warning on proxies that don't list the default. Warn only when the model was explicitly requested via the constructor. Also move the models.list() fetch under the cache lock with a double-checked pattern so concurrent first-callers issue exactly one round-trip instead of a stampede, and honor a per-agent max_tokens attribute (issue #471 groundwork) when resolving the compaction output limit in both providers. * test(providers): pin explicit-model warning, listing single-flight, per-agent compaction limit Cover the companion provider fixes: no warning for the hardcoded default model when agents override it (warning retained for an explicitly requested model), exactly one models.list() call under concurrent first-callers, and the compaction output limit honoring a per-agent max_tokens attribute with source "settings". * fix(providers): address PR #503 review findings on context compaction Blocking: - measure post-compaction tokens via heuristic reclaim (mirroring TieredCompaction._escalate) so tokens_saved is non-zero on real compactions instead of always reporting before == after - replace the degenerate trigger=1 floor with resolve_compaction_plan: the tool buffer is clamped to 25% of the window, the target keeps a window-scaled 5% hysteresis margin below the trigger, and a plan with no viable headroom is disabled (reported on agent_compaction_config via enabled/disabled_reason) instead of compacting on every request - keep max_tokens off the OpenAI wire when unset; the compaction reserve still falls back to the 16384 default internally, so reasoning models keep the server's full output allowance Recommended: - fold ANTHROPIC_BASE_URL into ClaudeProvider._base_url so has_custom_base_url gates registry lookups for env-configured proxies - bound Anthropic/OpenAI model-listing drains at 2000 entries, handle partial listings explicitly, never cache an empty listing, and narrow the catch to transport errors so parser bugs surface - pin pydantic-ai-harness (<0.25) and pydantic-ai-slim (<3), and declare genai-prices as a direct dependency - delete the dead _ThresholdGatedCompaction gate (its branches were identical and duplicated the wrapper's own estimate) - split the wrapper's failure handling into three zones so a telemetry failure no longer latches compaction off or reports false failure, and name degraded tiers / still-over-trigger on agent_compaction_complete - implement AgentProvider.get_max_output_tokens on the Copilot provider - style the four compaction activity types in the dashboard, render tokens_saved, and surface disabled/degraded states - rebuild examples/compaction.yaml around a multi-turn MCP tool loop (loop-back iterations never accumulated provider history) and fix the inverted trigger-direction comment - correct docs claiming ModelInfo.max_tokens caps the wire value, the dashboard-bar refresh timing, and split the CHANGELOG entry into Added/Changed (dropping the nonexistent 64k-fallback removal note)
Summary
Add max_tokens to AgentDef, enabling per-agent output tokens limit configuration.
Background
agent_builder.py already has the fallback logic, but the field was missing from the schema:
Since AgentDef uses extra="forbid", specifying max_tokens in YAML caused a validation error, and getattr always returned None.
Changes
・Add max_tokens: int | None = Field(None, ge=1, le=200000) to AgentDef (matches RuntimeConfig.max_tokens constraints)
・Add forbidden-field checks in validate_agent_type for non-LLM agent types: script, workflow, wait, set, terminate
・Add TestAgentDefMaxTokens(11 tests)
Usage
When omitted inherits runtime.max_tokens as before. No logic change needed in agent_builder.py
Closes #470