fix(agent): bill tool-approval time to the human, not the tool - #3007
Conversation
…metrics The flagship re-sent ~17,000 tokens of prompt to a 4B model on every LLM call, 2-5 times per turn, so even a trivial question paid tens of seconds of prefill before the model produced a word. This cuts the fixed prefill 53% — 17,014 to ~8,007 tokens — without removing a capability. Three sources, all measured offline with tiktoken (no model contacted): - The tool list went out twice. Gemma takes native tool_calls, so all 66 JSON schemas ship in `tools=`; the system prompt then restated every name and summary in an `==== AVAILABLE TOOLS ====` block — 1,678 duplicate tokens per call. `_compose_system_prompt` now gates that block on the same condition that already gated `_response_format_template`. Non-native models still get it; for them it is the only place the tool names appear. - `gaia-voice` is always on and was 2,145 tokens of rationale prose — every rule followed by the incident that motivated it. Rewritten as instructions: 692 tokens, all 24 behavioural rules intact. - Dynamic tool loading was built, tested, and switched off for the flagship by two independent gates. `FULL_CORE_TOOLS`/`FULL_BUNDLES` now cover all 67 tools under the same union-equality drift guard, and the agent shows the model <=26 per turn instead of all of them. Prompt sections are also reordered static-first. llama.cpp reuses its KV cache only up to the first differing token, and the memory block sat at the very top, so a single `remember()` invalidated ~3,900 tokens that had not changed. `VOLATILE_PROMPT_FRAGMENTS` names the fragments that now compose last. `GAIA_TURN_LOG=<path>` turns on per-turn recording: fixed prefill, input tokens split cached vs new, output tokens, ttft, the wall-time split across model / tools / agent overhead, and absolute timestamps — one JSON object per turn, so two builds are diffable. Off by default. `--dev` draws the same breakdown under each answer in the TUI. Tool timing lives in `_execute_tool_timed`, which the agent loop calls, rather than inside `_execute_tool`: that method is copied onto test stand-ins by attribute assignment and read back with `inspect.getsource` by the coercion contract test, so wrapping it in place breaks both. Not done, and blocked rather than skipped: `gaia eval agent` against the committed baseline, and any post-change latency measurement. Both need Lemonade on Gemma-4-E4B, which is currently banned on this machine; a Claude run is not a substitute for either. docs/plans/gaia-agent-latency.md lists exactly what is owed and how to take it.
github-actions
Bot
added
documentation
Request changesThis cuts the flagship agent's per-call prompt roughly in half — it stops re-sending the tool list twice, moves the parts that change mid-session to the end so the model's cache survives, trims the always-on voice skill, and only shows the model the tools a turn actually needs. It also adds an opt-in per-turn latency log. The engineering is careful and unusually well tested; the problems are in what hasn't been checked yet. Three things to fix before merge:
Real-world evidenceN/A — no evidence bundle was produced for this PR, and I could not read the PR description in this environment (the GitHub CLI was unavailable), so I can't tell whether evidence lives there. The verdict rests on static review alone. This change is user-visible on two surfaces: the TUI's 🔍 Technical details🟡 Issues1. Four LLM-affecting surfaces changed with no
CLAUDE.md → "Run agent evals when changing LLM-affecting code paths — do NOT skip" names system prompts, prompt-assembly order, and the tool schema sent to Lemonade explicitly. gaia eval agent --category rag_quality --agent-type doc
gaia eval agent --compare \
tests/fixtures/eval_baselines/gemma-4-e4b-d71cd914/scorecard_rag_quality.json \
<run>/scorecard.json2. assert "AVAILABLE TOOLS" in agent.system_prompt
assert "mcp_memory_create_entities" in agent.system_prompt
The right fix is asserting the schemas rather than the prose, since that is now where MCP tools reach the model: Worth a grep for other 3.
Failure scenario: "can you make me a Word document?" — the selector matches on "document" and pulls Two options: promote 4. W='C:\Users\14255\Work\gaia\.claudia-worktrees\claudia-task-25e62f25'
PY='C:\Users\14255\Work\gaia\.venv\Scripts\python.exe'
...
*claudia-task-25e62f25*) ;;
*) echo "ABORT: gaia resolves to $resolved (not this worktree)"; exit 2;;
(It also embeds a local username in a public repo.) 🟢 Nits
Strengths
|
Driving the real TUI on Claude Haiku 4.5 — the first end-to-end run of the per-turn recorder — showed it misreporting three things. Every one passed the unit suite, because each needs a real multi-step turn with a real human at the keyboard to appear. Cache reuse was understated 3.6x. The rendered prefix used for the cached-vs-new split appended the tool schemas AFTER the conversation, but chat templates inject them alongside the system block at the front. The shared prefix therefore stopped at the first history message: a turn whose entire 12.2k system+tools header was reusable reported 27% cache hit. It now reports 98.7% for the same turn. Pinned by a regression test that fails at 62% on the old ordering. Time spent waiting for a human was billed to the tool. The confirmation prompt blocks inside _execute_tool, so a shell command that ran in 0.1s was recorded as 322.6s because that is how long approval took. Approval is now timed separately and surfaced as its own `waiting on you` figure, leaving tool_s and overhead_s meaning what they say. A missing ttft rendered as 0.0s. Claude reports no time-to-first-token, and the step row printed the absent value as an instant first token — the fake zero the recorder itself is careful never to emit. It now reads `--`. Also verified in that run: the ladder passes on Haiku (arithmetic, cross-turn conversation history without memory, a real shell tool call returning the true directory), the reordered prompt composes and is accepted, the AVAILABLE TOOLS block is absent on the native path, and the dev-mode metrics block renders. The smoke test that found this ships as .perf/haiku_smoke.py and now forces GAIA_MEMORY_DISABLED=1: the memory subsystem embeds through Lemonade, and an earlier run of it spawned a llama-server for the embedding model while Lemonade was under a ban. That reach is now closed at the source. Still owed and still blocked: gaia eval agent and any Gemma latency number. Haiku validates logic only.
|
Verdict: Approve with suggestions This is a large, well-reasoned PR: it cuts the flagship's fixed prefill by ~53% through four coordinated changes (duplicate tool-block removal, static-before-volatile reordering, Two items need resolution before or shortly after merge. 🟡 The eval is mandatory for this change, and the plan explicitly says it hasn't run. Four LLM-affecting surfaces changed: the system prompt lost 2,832 tokens, its section order changed (volatile moved from first to last), the The Lemonade ban is a real circumstance, not a deliberate skip — but the merge gate still needs the eval before this can be called done. Please run 🟡 The script is designed to guard against a real problem (PYTHONPATH falling through to the main checkout). As committed it will 🟢 Nit: The plan doc mentions 🔍 Technical detailsEval requirement ( The four changed surfaces are:
Each of (1)–(4) is a change the base class makes to what the model reads.
# .perf/runtests.sh lines 5-6
W='C:\Users\14255\Work\gaia\.claudia-worktrees\claudia-task-25e62f25'
PY='C:\Users\14255\Work\gaia\.venv\Scripts\python.exe'The guard at lines 10–13 checks SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -W 2>/dev/null || pwd)"
W="$(dirname "$SCRIPT_DIR")" # repo root
PY="${VIRTUAL_ENV}/Scripts/python.exe" # or python from PATHThe What looks correct (no action needed):
|
The quality gate failed on this branch. `query`, `agent_name` and `model_id` were passed to TurnRecorder twice — once to the constructor, which ignored all three, and again to finish(). Pylint reported the unused arguments, but the duplication mattered more: the model id was resolved a second time at the end of the turn and could disagree with the model the turn actually ran on. The recorder now holds them from construction and finish() takes only what is new by then. Also drops an `import time` that the module-level import made redundant, gives `_tool_timing_depth`, `_confirmation_wait_s` and `_mixin_prompt_origins` class-level defaults beside the other per-instance attributes, and marks the optional `print_turn_metrics` hook's unused argument as deliberate — it is non-abstract precisely so existing consoles need not implement it.
|
Verdict: Request changes Note: The 🔴 Eight tests in Fix: strip the extra kwargs from every 🟡 LLM-affecting surfaces changed but 🟢 🔍 Technical detailsThe
def finish(self, *, answer: str, steps: int) -> Dict[str, Any]:Test calls (all raise
The test at Eval gate: CLAUDE.md §"Run agent evals when changing LLM-affecting code paths" lists exactly these triggers. The plan doc ( gaia eval agent --category <cat>
gaia eval agent --compare tests/fixtures/eval_baselines/gemma-4-e4b-d71cd914/scorecard_<cat>.json <run>/scorecard.jsonruntests.sh: |
Request changesThis branch cuts the flagship agent's fixed prompt roughly in half — it stops restating the tool list in prose for models that already receive JSON schemas, moves the parts of the prompt that change mid-session to the end so the model's cache survives them, turns on per-turn tool selection for the flagship, and adds an opt-in per-turn timing record that surfaces in the terminal UI under The blocker is not the code, it's the missing behavioural check. This PR rewrites the default prompt for the whole chat-agent family — the flagship, document Q&A, and the Agent UI's chat — and the project's own rule is that a change to those surfaces must be scored against the committed baseline before merge. That run hasn't happened; the branch documents it as blocked by unavailable local hardware. A maintainer needs to either run it or knowingly waive it, because nothing in the test suite can tell you whether answer quality moved. Two smaller things to fix before merge:
Real-world evidenceNo I also couldn't read the PR description from this runner (the GitHub CLI wasn't available), so if the Haiku output or screenshots are posted there, treat that as covered. The verdict rests on static review for two surfaces: the TUI's new 🔍 Technical details🟡 Important1. Four LLM-affecting surfaces changed with no
Blast radius is wider than the flagship:
2. Per 🟢 Minor3.
4. 5. Two comments added in this PR disagree on the slot budget. 6. Metrics never reach the TUI on the exits that matter most. 7. Strengths
|
finish() no longer takes the turn's identity, so tests/unit/test_turn_metrics.py had to move with it. The one test that asserts a distinct query per record now constructs a recorder per turn, which is where the query belongs — it is fixed when the turn opens.
|
Verdict: Request changes — one policy blocker before merge; the rest is strong work. This PR does four things that are explicitly gated by CLAUDE.md's mandatory eval requirement, and the eval has not been run. The plan doc is honest about this — "Numbers still owed — blocked, not skipped" — but CLAUDE.md's rule is clear: these changes cannot be called done until a 🟡 Four LLM-affecting surfaces changed in this PR: the system-prompt section order, the tool-block gate (suppressed for native tool-calling models), the The plan doc acknowledges this is owed and explains why it is blocked (Lemonade banned due to machine instability). The Haiku smoke test validates recorder logic, and the plan doc correctly states that Haiku is not a substitute for Gemma-4-E4B quality numbers. Until the eval runs and the scorecard compares clean (or a regenerated baseline is committed with an explicit call-out that capability changed), merging this introduces a quality regression that will be invisible to every unit test. When Lemonade is available again: gaia eval agent --category rag_quality --agent-type doc
gaia eval agent --compare \
tests/fixtures/eval_baselines/gemma-4-e4b-d71cd914/scorecard_rag_quality.json \
<printed-output-path>/scorecard.jsonRun serially for each relevant category. 🟢 The committed script sets 🔍 Technical detailsLLM-affecting surfaces (CLAUDE.md checklist):
All four are in CLAUDE.md's explicit list: "Tool registration, tool docstrings, or the JSON tool schema sent to Lemonade" and "ChatAgent / DocumentQAAgent / FileIOAgent / ChatAgentLite system prompts or any mixin prompt fragment." runtests.sh hardcoded paths: W='C:\Users\14255\Work\gaia\.claudia-worktrees\claudia-task-25e62f25'
PY='C:\Users\14255\Work\gaia\.venv\Scripts\python.exe'The guard on lines 11–13 will always ABORT for a contributor not on this exact worktree. The plan doc cites this script as the right way to run tests to avoid the PYTHONPATH trap — a reader who follows that advice gets ABORT immediately. |
…md#3026) > **Stacked branch.** Cut from the shared `integration/full-tui` base, so the diff against `main` also shows 17 commits already covered by amd#3022-amd#3024. This change is 7 files: `src/gaia/llm/providers/claude.py`, `src/gaia/agents/base/turn_metrics.py`, `tui/internal/event/canonical.go`, `tui/internal/ui/chat/turnmetrics.go` (+ test), and the two unit-test files. It depends on amd#3024 for the turn-metrics plumbing. Every LLM call the Claude backend made re-sent the same ~13,700 tokens of system prompt and tool schemas at full price — Anthropic prompt caching is opt-in, and nothing in the provider ever asked for it. A ReAct turn is 2-5 calls, so the largest single cost in the product was being paid over and over for bytes that never changed. This turns it on, and fixes the usage parsing that would otherwise have reported the win as zero: the provider read only `input_tokens` and `output_tokens`, never the cache counters, so the `--dev` metrics line printed `0 cached` whether caching worked or not. Measured on `claude-haiku-4-5` through the flagship TUI: a cold turn writes 13,696 tokens to the cache, and **every turn after it reads them back — 96-98% of input served from cache, on every step of a multi-step turn**. Cached input bills at ~0.1x, so the repeated prefix now costs about a tenth of what it did. The cache also survives a TUI restart within its TTL, so relaunching costs nothing. This is complementary to the open token-reduction work (amd#3007 prefill, amd#3008 dynamic tools, the gaia-voice trim): those shrink the prefix, this stops paying for it on every repeat. If both land the effects multiply rather than add — a smaller prefix, charged at a tenth, once instead of per call. <details> <summary>🔍 Technical details</summary> **Two breakpoints, not one.** Anthropic renders `tools` → `system` → `messages`, so a marker on the system block covers the whole fixed prefill. A second marker at the end of the tools segment is there because caching gives no partial credit: with only the system marker, one byte of drift anywhere in the system prompt would discard the tool schemas too. Block-level markers rather than top-level `cache_control=` on `messages.create()` — top-level auto-placement marks the *last* cacheable block, which in an agent loop is the newest tool result, so every request would write a fresh entry and read almost nothing. **`input_tokens` is the uncached remainder, not the prompt size.** `prompt_tokens` now sums it with the cache reads and writes; left alone, a working cache would have read as a prompt that shrank by 98%. The streaming path takes the counters off `message_start`, the only event that carries them. **Metrics source.** The turn record and the `--dev` block prefer the backend's own cache accounting wherever it reports any (a cold turn that only *wrote* counts, so turn 1 and turn 2 are on the same scale). Lemonade reports none, so the local prefix estimate still drives that display unchanged — the two sources are recorded separately and never summed. **Prefix-stability audit — what still limits the hit rate.** Caching is a prefix match, so anything volatile ahead of a breakpoint caps the ceiling. Clean: no timestamp, uuid, session id, pid, or cwd anywhere in the system prompt or tool descriptions (the one `datetime.now()` is already correctly prepended to the *user* message, `memory.py:2114` — do not "fix" it into the system prompt). Outstanding, in severity order, none addressed here: 1. `dynamic_tools=True` with a 26-slot LRU cap over a ~66-tool registry (`hub/agents/gaia/python/gaia_agent/agent.py:162,171`) rewrites the `tools` array per turn once at the cap. Tools render first, so an eviction invalidates everything. Worth considering pinning off under `--use-claude`: a cached full registry at 0.1x beats an uncached rotating subset at 1.0x. 2. `indexed_docs_section` is volatile but sits ~600 tokens from the top of the system prompt, above ~2,500 tokens of static rules (`gaia_agent_chat/profiles.py:141`, `agent.py:769-842`). Indexing one document invalidates everything below it. Moving it to `VOLATILE_PROMPT_FRAGMENTS` helps llama.cpp too. 3. The memory block prints `(confidence: 0.87)` per fact (`memory.py:2044`), and every `recall` bumps confidence by 0.02 and can reorder facts — a guaranteed per-turn byte change for no model benefit. 4. Splitting `system` into a static head and volatile tail with a breakpoint between would salvage the static ~2,500 tokens from all of the above. The measured run above hit 96-98% because that session took the full-registry path, so the tools array was byte-stable. </details> ## Test plan - [ ] `pytest tests/unit/test_claude_provider.py tests/unit/test_turn_metrics.py -q` — 54 tests, including breakpoint placement on the outgoing request, no mutation of the caller's tool list, cache-field parsing on both the streaming and non-streaming paths, and an older SDK response with no cache fields. - [ ] `cd tui && go test ./internal/ui/chat/ ./internal/event/` - [ ] Full `pytest tests/unit -q` was run and diffed against the unmodified base commit: **no new failures** (605 vs 610 pre-existing environmental failures — this branch has 5 fewer, none related). - [ ] Live, two identical consecutive turns: ``` GAIA_TURN_LOG=/tmp/turns.jsonl gaia run gaia --dev --use-claude --claude-model claude-haiku-4-5 ``` Before / after `cache_read_input_tokens`, straight from the turn log: | | prompt_tokens | cache_read | cache_write | |---|---:|---:|---:| | before (any turn) | 14,034 | **0** | 0 | | after, turn 1 (cold) | 14,034 | 0 | 13,696 | | after, turn 2 (identical) | 14,057 | **13,696** | 0 | | after, 2-step tool turn | 28,403 | **27,392** (both steps) | 0 | **Not run: `gaia eval agent`.** This touches an LLM-affecting surface, so CLAUDE.md requires it, and it has not been run — Lemonade is currently banned on this machine (the user's PC crashes), and a Claude-backed run is not a valid substitute for a local-model baseline. It remains outstanding and should be run before merge on a machine where Lemonade is available. Nothing here changes prompt *text*, tool schemas, or the tool-call envelope — only where the cache breakpoints sit and how usage is parsed — but that is an argument for expecting it to pass, not evidence that it did. --------- Co-authored-by: Ovtcharov <kovtchar@amd.com> Co-authored-by: kovtcharov-amd <kalin.ovtcharov@amd.com>
The prompt-shrinking half of this branch already reached main with the amd#3022-amd#3026 stack, and amd#3026 rebuilt the turn recorder on top of it to add Anthropic cache accounting. Resolved to main everywhere that work landed -- keeping the branch's older copies would have reverted the cache counters, the confirmation-summary disclosure limits, ChatAgent's output_handler, and the execute_python_file confirmation gate. What survives is what never landed: approval time billed separately from tool time, the rendered-prefix ordering fix behind the cached/new split, and the absent-ttft rendering.
kovtcharov-amd
changed the title
|
Verdict: Approve Three distinct fixes land here cleanly — approval-wait excluded from tool timing, cache-proxy ordering corrected, A few observations worth noting, none of them blocking:
Cache-proxy ordering fix — putting tools alongside
🔍 Technical detailsRe-entrancy guard (agent.py:3237): The
Go TTFT alignment: Binary search correctness: |
3 participants
Most of this branch already reached
mainwith the #3022–#3026 stack. It has been merged down to what never landed: 13 files instead of 35, +478/−261 instead of +3738.Three things the
--devturn breakdown reported wrongly. Every one needed a real turn with a real human at the keyboard to appear, which is why the unit suite was green:waiting on youfigure, leavingtool_sandoverhead_smeaning what they say.0.0s, which reads as an instant first token. It now reads--.It also clears the reviewer findings that rode onto
mainunfixed with that stack. The one contributors actually hit:.perf/runtests.shwas pinned to one developer's worktree — and leaked a local username — so the guard the plan doc tells people to use aborted for every one of them.On the eval. All five reviews blocked on
gaia eval agent. That gate was about the prompt changes, which are now onmain, merged without it. Nothing left here touches prompt composition, tool schemas, or model selection, so the gate does not apply to this PR — but the eval is still owed for what merged, and it is no longer hardware-blocked.🔍 Technical details
Two reviewer findings were checked and are not valid as written:
AVAILABLE TOOLSassertion intests/mcp/test_mcp_cli_to_agent_workflow.pyis fine. The premise was that an agent with nomodel_idfalls back toDEFAULT_MODEL_NAMEand so takes the native path; in factAgent.__init__stores the rawNone(agent.py:816) and only the client gets the default (:848), so_uses_native_tool_calls()is False and the prose block is still emitted. Applying the suggested rewrite would have broken the test — it asserts on_openai_tools, which isNonethere.os.path.commonprefixis not C-speed; it is a Python loop like the one it would replace. Measured on a 99K shared prefix: 5.61ms → 4.71ms. Binary search over slice equality gets 0.09ms, so that is what landed, fuzzed against the naive implementation over 200k random pairs.That investigation exposed a real drift risk, now fixed:
_openai_toolsre-derived the predicate that_uses_native_tool_callsdocuments itself as "the single source of truth" for, so the schema path and the prose gate could disagree. Both read one predicate now, pinned by a parametrised test.Still open, deliberately not fixed here —
_publish_turn_metricsis called only from the printed-answer branch, so a turn that burns its step budget shows no--devbreakdown. The obvious fix (publish on the tail too) introduces a cross-turn leak:SSEOutputHandler.print_turn_metricsonly stashes, andprint_final_answerconsumes-and-clears, so stashing on a path with no answer event would attach that record to a later turn. Worth its own change.Test plan
python -m pytest tests/unit/test_turn_metrics.py tests/unit/test_turn_metrics_wire.py tests/unit/test_system_prompt_composition.py tests/unit/test_dynamic_tool_filtering.py hub/agents/gaia/python/tests/test_full_tool_bundles.py— 154 passed locallycd tui && go build ./... && go test ./internal/ui/chat/ ./internal/event/bash .perf/runtests.sh tests/unit/test_turn_metrics.py -q --collect-onlyprints[guard] gaia -> …and collects, instead of aborting; run from outside a checkout it still aborts with exit 2git diff main...HEADtouches 13 files and reverts nothing from perf(claude): cache the fixed prefill, and report the cache counters #3026 — the cache counters, theexecute_python_fileconfirmation gate, ChatAgent'soutput_handler, and the confirmation-summary disclosure limits are all still main's