Skip to content

perf(claude): cache the fixed prefill, and report the cache counters - #3026

Merged
kovtcharov-amd merged 21 commits into
amd:mainfrom
kovtcharov:claudia/task-a56ea9bd
Aug 24, 2026
Merged

perf(claude): cache the fixed prefill, and report the cache counters#3026
kovtcharov-amd merged 21 commits into
amd:mainfrom
kovtcharov:claudia/task-a56ea9bd

Conversation

@kovtcharov

@kovtcharov kovtcharov commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Stacked branch. Cut from the shared integration/full-tui base, so the diff against main also shows 17 commits already covered by #3022-#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 #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 (#3007 prefill, #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.

🔍 Technical details

Two breakpoints, not one. Anthropic renders toolssystemmessages, 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.

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.

Ovtcharov added 17 commits August 18, 2026 17:57
A long line in the agent's work log lost its tail. A shell command showed
up to its first flags and stopped; a failed tool call showed "permission
denied writing to..." and cut off before the sentence that says what to do
about it. Turns run 25-160s, so this log is most of what a user looks at
while they wait, and the clipped half was routinely the actionable half.

The lines wrap now, reusing the transcript's own wrap (components.WrapText)
rather than a third wrapping path. The region's height ceiling is unchanged
- still min(workLogMaxRows, viewport/2) - so a wrapped line costs older
HISTORY, never height: on a 12-row terminal the log still gets two rows.
Per-action row caps stop one 300-character command from spending the whole
region on itself, and what those caps do cut ends in an ellipsis.

Three things fell out of doing it properly:

- WrapText only breaks on spaces, so a URL or a Windows path - the lines
  most likely to run long - had no break to wrap at and overran the pane.
  Rows wider than the measure are now hard-broken at a column boundary.
- Text was being cut to one row's width at CAPTURE time, before the
  renderer ever saw it: at 74 columns for a narration, at 66 for a tool
  error's own message, at 60 on the legacy transport. Those bounds now sit
  where the renderer can still use them, so the wrap decides where a line
  ends. A tool error keeps its second line, which is where the remedy is.
- The old height trim dropped the NEWEST action when it alone exceeded the
  budget, so a wrapped command on a short terminal rendered an empty
  region. It is clipped to fit and marked instead.

The region's height is also held at each turn's peak. It grows as work
happens, which reads as progress, but shrinking dropped the transcript
above back down mid-sentence - and wrapping makes those swings bigger.

Covered by table-driven tests at 60x12, 80x24, 100x40 and 200x50: tail
survival on both event dialects and both failure paths, height within
budget and under half the viewport, no row wider than the terminal, one
long action unable to fill the region, and the height never shrinking
mid-turn.
…de model

`gaia run gaia --use-claude --claude-model claude-haiku-4-5` started
LemonadeServer.exe anyway, and held the first answer behind a multi-minute
`gaia init` before answering anything.

The first-boot gate ran `gaia init` on every flagship launch, and `gaia init`
auto-starts the local server unconditionally (_auto_start_server,
src/gaia/installer/init_command.py). Claude mode only added --skip-chat-model,
which skips the model download and nothing else — so the one flag whose purpose
is to avoid the local backend was the flag that reliably brought it up. Claude
sessions now skip the gate entirely and say on screen that it was skipped, what
still needs Lemonade, and how to run it later.

Alongside that:

- The header names the model — `claude · haiku-4.5`, not a bare `claude` chip.
  Seeded from the launch flag, because the agent's model-state ping is not read
  until the first turn, so a session opened and read had no model info at all.
- `--claude-model` and `/model <id>` validate against one list shared with the
  agent's own CLAUDE_MODELS, and a test parses stdio.py so the two cannot drift.
  An unknown id is refused with the accepted ids instead of reaching Anthropic
  to come back a 404 mid-turn.
- A local `/model` switch with the server known-down is refused with both ways
  forward, phrased for where the session actually is, and fires once rather than
  forever — nothing refreshes that cached state between pings, so a sticky
  refusal would be a dead end no retry could clear.
- Typing the space in `/model ` turns the slash palette into a model picker.
  Local ids stay behind bare `/model`; only the agent knows what is downloaded.
…n" reports

Twice in one session a monochrome TUI was investigated as a rendering
regression. Both times the cause was the launcher: a cmd.exe window spawned by
Start-Process lands in legacy conhost, which reports no colour support, so
detectStyle() resolves NoTTYStyle and glamour drops every chroma token. The
process emits zero escape sequences — nothing in the renderer is wrong.

The skill also said colour could not be judged from a capture. It can:
format=ansi returns the raw frame, and counting escapes settles it in one call.

- name Windows Terminal as the launch path, with the two wt.exe traps (a spaced
  --title breaks its parser; a new tab inherits the running Terminal's env, not
  the shell's, so env vars belong in the .bat)
- replace the "cannot judge colour" bullet with the format=ansi check and what
  zero escapes actually means
…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.
Review follow-ups on the wrap change, all in what the log actually shows:

- A cut on the live row put its ellipsis after the elapsed clock, where it
  read as part of the timer. The row budget now reaches renderActivityItem,
  so the cut is made in the text before the clock is ever appended - and the
  marker no longer spends two columns of the viewport's edge reserve.
- Only the live row carries the clock, but every wrapped row of that action
  was paying the eight columns it takes. Continuations use the full measure,
  which gives a long command back about two words a row.
- A long argument could push the words that follow it out of its own phrase:
  "Looking for <query> in your documents" rendered without "in your
  documents". The argument is budgeted against the template now.
- A tool error's second line survives on both error paths, not just the one
  that goes through a render card. A `summary` stays first-line-only: one
  that runs to several lines is a payload dump, not a sentence.
- The held height pads above the log rather than below, so the blank rows
  land in the space that already separates it from the transcript instead of
  opening a gap between the log and the answer streaming under it.

Each fix has a test that fails without it - verified by reverting the
behaviour and watching the test go red.
…de model

`gaia run gaia --use-claude --claude-model claude-haiku-4-5` started
LemonadeServer.exe anyway, and held the first answer behind a multi-minute
`gaia init` before answering anything.

The first-boot gate ran `gaia init` on every flagship launch, and `gaia init`
auto-starts the local server unconditionally (_auto_start_server,
src/gaia/installer/init_command.py). Claude mode only added --skip-chat-model,
which skips the model download and nothing else — so the one flag whose purpose
is to avoid the local backend was the flag that reliably brought it up. Claude
sessions now skip the gate entirely and say on screen that it was skipped, what
still needs Lemonade, and how to run it later.

Alongside that:

- The header names the model — `claude · haiku-4.5`, not a bare `claude` chip.
  Seeded from the launch flag, because the agent's model-state ping is not read
  until the first turn, so a session opened and read had no model info at all.
- `--claude-model` and `/model <id>` validate against one list shared with the
  agent's own CLAUDE_MODELS, and a test parses stdio.py so the two cannot drift.
  An unknown id is refused with the accepted ids instead of reaching Anthropic
  to come back a 404 mid-turn.
- A local `/model` switch with the server known-down is refused with both ways
  forward, phrased for where the session actually is, and fires once rather than
  forever — nothing refreshes that cached state between pings, so a sticky
  refusal would be a dead end no retry could clear.
- Typing the space in `/model ` turns the slash palette into a model picker.
  Local ids stay behind bare `/model`; only the agent knows what is downloaded.
One-line status prints, tool previews and work-log rows were cut off well
before the window's right edge on a wide terminal. The layout now follows the
real width, so a 240-column window shows a 238-column tool line instead of 78.

The visible cap in logWidth was only half of it: toolNarration and
toolResultDetail cut text to 74/66 columns when the event ARRIVED, before any
width was known, so widening the layout alone would have shown nothing new.
Capture bounds now exist only to stop an agent's runaway payload living in the
model; layout decides what prints, from m.width, at render time.

Prose deliberately does not follow the window. answerMeasure stays at 88 —
for a paragraph, extra columns lose the eye on the carriage return, and a
question wrapped to the pane above an answer capped at 88 reads as two
unrelated blocks. For a single row, extra columns are simply more of that row,
and for the rows whose tail carries the reason or the remedy, the tail is the
point. Both constants now cross-reference each other so the split reads as
chosen rather than overlooked.

Measuring everything against the terminal exposed three rows that measured
against nothing: the live line appended its clock after truncating, logWidth's
floor of 16 overran any terminal under 20 columns, and the "still working"
hint was a fixed 50 columns that wrapped to two rows on a narrow window and
broke the height budget that had already been decremented for it. Also bounds
setLiveStatus at capture, the one agent-supplied string with no bound at all.
Review follow-up on the terminal-width work. Four rows could still print past
the last column, and the test that was supposed to catch them could not.

wrapLog raised any measure under 8 columns back up to 8, overriding the caller's
budget, so on a 20-column terminal a live row drew 21 and sheared the row below.
A measure is what the caller can afford; it is floored at 1 now, never raised.

The idle live line — "Getting started", the opening frame of every turn before
any tool event exists — went through no measure at all. "Thinking about the next
step" drew 39 columns and sheared anything narrower, which is most split panes.
It now wraps like every other row.

A resize reflows the log, which it could not before this branch: the measure was
pinned at 74, so widening from 100 to 240 columns moved nothing. Now it moves 94
to 234, straight into the peak-height hold that exists to stop the log shrinking
under a reader mid-sentence. A deliberate resize is not that, and holding the old
peak stranded blank rows under the log for the rest of the turn. The hold is
released on a width change only — the composer growing a row must not release it.

widestLogMeasure now derives from the widest window the TUI accepts anywhere
(control/server.go's 20-500 resize range) rather than a guessed 240, so no window
this program will lay out can outrun what capture kept for it.

The old cramped-terminal test asserted logWidth()+4 fits and never rendered a
row, certifying a property the rendered rows did not have. It now draws every row
type — idle, live, wrapped, outcome, --dev payload, repeat counter — at every
width from the declared minimum of 20 up.
Asked to file an issue, the flagship could only draft one and tell the user
to paste it themselves — the `shell:execute:gh` grant was strictly read-only,
so `gh issue create` came back as an error. A triage that can never post is
half a triage.

The grant now sorts every invocation into three tiers instead of two:

- ALLOW — reads, unchanged: they run with no prompt, because loading the
  skill is the consent and a triage is five to ten reads.
- CONFIRM — `issue create|comment|edit`, `pr comment`, `label create|edit`.
  The user sees the exact command and answers yes / no / always-for-that-verb
  through the prompt the TUI already has.
- REFUSE — never runs and never raises a prompt: `auth token`, `alias`,
  `extension`, `config`, every `gh api` write, `pr merge`, `issue close`,
  `label delete`, `repo delete`.

Keeping the last two apart is the design, not an implementation detail. One
prompt covering everything is a prompt users learn to click through, and then
the click covers the credential print too.

`validate_invocation` keeps its name but narrows to one question — "may this
run with nobody asked?" — and answers no for CONFIRM as well as REFUSE, so a
caller that has not been taught the third tier fails closed.

Two holes found while reviewing the change, both closed here:

The action must now be the first token after the subcommand. gh strips a value
for any flag it cannot prove boolean at that level, including one it has never
heard of, so `gh label -f list create -c FF0000 -R x/y` was classified as the
read `gh label list` and ran unprompted while gh dispatched `create`. No
allowlist of value-flags closes that — the next gh release adds a flag the
table has never seen — so a leading flag is refused instead.

A granted CLI is executed as argv rather than a shell string. Every check runs
on shlex tokens while Windows handed cmd.exe the original string, so
`--search x|echo …` was one argument to the gate and two commands to the
shell, and `%VAR%` expanded into a value the approval prompt never displayed.
A granted binary is a real executable and needs nothing shell=True provides.
Pipelines and every ungranted command keep the old path.

Also: the confirmation prompt no longer elides a comment body at 180
characters without saying so — "approve this exact command" has to be true.
… commands portable

"Triage my inbox" had no procedure to follow — the skill only knew how to walk a
named repo's backlog — so the agent answered from memory instead of calling gh,
and invented an answer that looked plausible.

- add an inbox procedure that ranks by the notification `reason` field, which is
  what actually answers "who is blocked on me?"
- widen the frontmatter description so inbox phrasing selects this skill at all
- one-line, double-quoted commands only: Windows hands the raw string to cmd.exe,
  where POSIX quoting dies as "The system cannot find the path specified" — an
  error the agent misread as "gh is not installed"
- add a real gh preflight (`gh --version`, `gh auth status`) with per-OS install
  steps, and a rule that any other failure is a command bug, never a missing gh
- trim the author-facing "Fork this" prose, which costs tokens every turn and
  tells the running agent nothing
# Conflicts:
#	hub/skills/github-triage/SKILL.md
Every Claude call re-sent the same ~13,700-token system prompt and tool
schemas at full price, and the turn metrics could not have revealed it:
the provider never set a `cache_control` breakpoint, so nothing was ever
cached, and never read the cache fields back off `usage`, so the display
would have printed "0 cached" either way.

Two breakpoints, not one — end of the tools segment and end of the system
block. Caching gives no partial credit, so with only the system marker any
drift in the prompt (a memory confidence score, a skill body swapping in)
would throw away the tool schemas as well.

`usage.input_tokens` is the uncached *remainder*, not the prompt size, so
`prompt_tokens` now sums it with the cache reads and writes — otherwise a
working cache reads as a prompt that shrank by 98%. The streaming path
takes the counters off `message_start`, the only event carrying them.

The turn record and the --dev metrics block prefer the backend's own cache
accounting wherever it reports any. Lemonade reports none, so the local
prefix estimate still drives that display unchanged.

Measured on claude-haiku-4-5 through the flagship TUI: a cold turn writes
13,696 tokens, and every turn after it reads them back — 96-98% of input
served from cache, on every step of a multi-step turn.
@github-actions github-actions Bot added documentation Documentation changes chat Chat SDK changes llm LLM backend changes tests Test changes performance Performance-critical changes agents tui Go terminal UI (gaia-tui) labels Aug 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Request changes

This PR does three good things at once — it cuts the flagship agent's per-call prompt roughly in half, adds an opt-in per-turn latency recorder, and upgrades the gh skill grant from read-only to a three-tier read / ask / refuse gate (including a genuine command-injection fix on Windows). The engineering and the test coverage are strong. The blocker is not the code.

Four things that change how the model behaves shipped without an agent eval. The system prompt lost ~2,800 tokens, its section order changed, the tool list is no longer restated in prose, and the flagship now shows the model at most 26 of its 67 tools per turn, picked by similarity to the question. None of that is visible to a unit test — a tool the model needed but wasn't shown looks like the model just answering badly. The PR's own plan document lists the eval as owed and blocked on the local backend being unavailable, which is honest, but the project treats this eval as a merge gate specifically because a prompt change that passed every unit test once broke document Q&A in production. Either run it against the committed baseline before merge, or land the observability, prompt-caching and gh-tier work now and hold the two commits that change prompt content and tool selection until it can run.

A committed script only works on one person's machine. The new perf test runner has a developer's absolute Windows path and worktree name baked in, and its own safety guard makes it abort for anyone else. Take the path from the environment, or leave the script out of the commit.

Real-world evidence

No evidence-bundle.md was produced for this PR, so the verdict rests on static review plus the tests in the diff. The PR does carry reproducible offline token measurements, which is real evidence for the prefill numbers and I've credited them. What is missing is evidence on the surfaces a user touches: the terminal UI gained a model-name header chip, a /model picker, a --dev metrics block, work-log wrapping, and a new write-confirmation modal, and none of that was exercised here. The Go tests cover the rendering logic well; a short capture of the modal and the header would close the gap. Rendered screenshots are reasonably deferred to the strix-halo lane.

🔍 Technical details

🟡 Important

1. LLM-affecting surfaces changed with no eval against the baseline

Four gates in CLAUDE.md's "Run agent evals when changing LLM-affecting code paths" list are tripped by this diff:

  • _compose_system_prompt reordering + the AVAILABLE TOOLS block gate (src/gaia/agents/base/agent.py:938-990)
  • GaiaAgentConfig.dynamic_tools = True / dynamic_tools_max = 26 (hub/agents/gaia/python/gaia_agent/agent.py:1530-1539)
  • FULL_CORE_TOOLS / FULL_BUNDLES deciding what the model can see (hub/agents/chat/python/gaia_agent_chat/tool_bundles.py:1206+)
  • gaia-voice SKILL.md trimmed 2,129 → 676 tokens

docs/plans/gaia-agent-latency.md row 1 of "Numbers still owed" names exactly this. The riskiest of the four is the tool-selection default: ToolLoader falls back safely when the embedder is unreachable (session-disable → full registry, tool_loader.py:266-272), but nothing catches a successful selection that simply missed the right bundle. load_tools is the escape hatch and it is in CORE, which is the right design — but whether the model actually uses it is a behaviour question only the eval answers.

gaia eval agent --category <cat>
gaia eval agent --compare \
  tests/fixtures/eval_baselines/gemma-4-e4b-d71cd914/scorecard_<cat>.json \
  <run>/scorecard.json

2. .perf/runtests.sh hardcodes one machine (.perf/runtests.sh:5-6)

W='C:\Users\14255\Work\gaia\.claudia-worktrees\claudia-task-25e62f25'
PY='C:\Users\14255\Work\gaia\.venv\Scripts\python.exe'
...
case "$resolved" in
  *claudia-task-25e62f25*) ;;

The guard is a good idea; the constant it checks makes the script abort for every other contributor and every CI runner. Derive W from git rev-parse --show-toplevel and check gaia.__file__ against that instead, or drop the file from the commit — .perf/.gitignore already un-ignores it deliberately.

🟢 Minor

3. The write-flag denylist also refuses --web on reads, and the comment says it can't (src/gaia/skills/binaries.py:3771-3790)

_gh(actions, confirm) attaches _GH_WRITE_DENIED_FLAGS to the whole subcommand, so gh issue list --web and gh pr view 42 --web are now refused. That is defensible (--web returns nothing to an agent, and the denial reason says so), but:

    *confirm* is empty for a purely read-only subcommand. When it is not, the
    write-flag denylist comes with it. Note this also refuses ``--web`` on that
    subcommand's READS — ``gh issue list --web`` used to be allowed — which is
    intended: ``--web`` opens a browser and returns no output to the agent.

4. Stale slot count in the new drift guard (hub/agents/gaia/python/tests/test_full_tool_bundles.py:37)

Says dynamic_tools_max "leaves 12"; it is 26 − 10 CORE = 16, which is what agent.py says.

#: slots on its own (GaiaAgentConfig.dynamic_tools_max leaves 16).

5. The new elision notice can itself be truncated (src/gaia/ui/sse_translation.py:630-638)

The per-value cut now ends with … [+N more characters not shown], which is the right fix. The clause-level cut two lines below is still a bare at 1200 chars, so with two long arguments the second value's notice is what gets cut off — the same "user approves text they were never shown" shape, one level up. Single-argument calls (run_shell_command) are unaffected, so this is latent rather than live.

6. Sample output in the docs shows the pre-change numbers (docs/reference/dev.mdx:1044)

prefill 17.0k fixed · 66 tools is the baseline this PR replaces with ~8k / ≤26. Harmless, but it is the line a user will compare their own output against.

Strengths

  • The Windows argv fix is a real security fix, not a refactor. gh issue list --search "x|echo pwned>marker" is five argv tokens to every check in the policy and two commands to cmd.exe — on the one path that skips the confirmation prompt, reachable from an issue body the triage skill reads. shell_tools.py:927-949 restricts the exemption correctly (lone segment, granted binary only) and test_skill_binary_grants.py asserts shell is False and the metacharacter staying inside one token, rather than asserting the call was made.
  • CONFIRM and REFUSE are kept genuinely separate, and validate_invocation answers "no" for both so an un-updated caller fails closed (binaries.py:674-690). test_every_confirmable_action_is_deliberate pins the exact write surface as a tripwire — that is the right shape for this.
  • The action-must-be-first rule (binaries.py:769-777) is a structural fix for the cobra flag-swallowing divergence rather than an allowlist of the flags known to cause it today, and the test names the gh version it was verified against.
  • Claude prompt-cache accounting is correct: prompt_tokens = uncached + cache_read + cache_creation (providers/claude.py:_capture_usage) — reporting Anthropic's input_tokens alone would have made a working cache look like a 90% prompt shrink. Two breakpoints, and the streaming path — the one the flagship actually runs — is tested for the cache counters.
  • Turn-recorder tests cover the paths that usually get skipped: byte-identical payload when off, idempotent seal, nested tool timed once, a raising tool not recorded ok, and a cancelled stream that must not fetch stats.

@github-actions

Copy link
Copy Markdown
Contributor

Approve with suggestions

This PR cuts the flagship agent's fixed prefill by 53% (17K → ~8K tokens) through four coordinated changes — removing the redundant tool-prose block for native tool-calling models, reordering the system prompt to put volatile fragments last, enabling dynamic tool loading for the full-profile, and trimming the gaia-voice skill body. On top of that it adds Claude prompt-caching breakpoints, a three-tier ALLOW/CONFIRM/REFUSE gate for the gh CLI grant, a shell-injection fix on Windows (granted CLIs now use argv not a shell string), and a dev-mode turn-metrics recorder. The implementation is well-structured and the test coverage is thorough.

One 🟡 item must be resolved before this lands; one 🟢 nit is worth fixing.


🟡 The CLAUDE.md-required agent eval has not been run. Four LLM-affecting surfaces changed in this PR — the system prompt lost 2,832 tokens and was reordered, the AVAILABLE TOOLS prose block was removed for native models, and the flagship now sees at most 26 of 67 tools per turn instead of all 66. Per CLAUDE.md, gaia eval agent against the committed baseline is mandatory before any of those land. The plan doc (docs/plans/gaia-agent-latency.md) acknowledges the eval is owed and explains why it is blocked (Lemonade banned mid-task). That is an honest accounting, but CLAUDE.md is explicit: the block does not waive the requirement, it defers it — and a prompt change that halves the token count is exactly the kind of change that passes every unit test and then regresses quality in ways only the eval catches. Run the eval on a machine where Lemonade is available before merging.

🟢 .perf/runtests.sh was committed with hardcoded personal machine paths (C:\Users\14255\Work\gaia\...). The file is tracked (the .gitignore exempts it by name) and is useless on any other machine. Either generalize it to accept the worktree path as an argument, or keep it gitignored rather than committed.

🔍 Technical details

🟡 Eval — what to run

# Terminal 1
python -m gaia.ui.server --port 4200 --host 127.0.0.1

# Terminal 2 — run serially, one category at a time
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.json

The categories relevant to this diff are at minimum rag_quality (document Q&A exercises the reordered prompt and the dynamic tool filter) and whichever category exercises the flagship's full profile. The plan doc (docs/plans/gaia-agent-latency.md, "Numbers still owed") describes exactly what is owed and why each number needs Lemonade rather than Claude.

🟢 runtests.sh generalization

.perf/runtests.sh:3-4 — replace the two hardcoded variables with:

W="${1:?Usage: runtests.sh <worktree-path> [pytest args...]}"
PY="${VIRTUAL_ENV:-}/Scripts/python.exe"   # Windows .venv; adjust for the platform

or simply remove it from tracked files and let each developer keep their own local copy.

Implementation correctness notes (no action needed)

  • The three-tier ALLOW/CONFIRM/REFUSE logic in binaries.py is implemented correctly: validate_invocation delegates to classify_invocation and returns None only for ALLOW, so callers that predate the CONFIRM tier fail closed. skill_grant_covers_call in shell_tools calls validate_invocation (the narrow question), so CONFIRM writes still raise a prompt. The _validate_shell_command pre-flight returns None for CONFIRM, letting it reach the prompt instead of dying before anyone can approve it.
  • The _execute_tool_timed depth guard (_tool_timing_depth) prevents double-counting when a tool body calls another tool (e.g. CodeAgent orchestration). The default is handled via getattr(self, "_tool_timing_depth", 0) so no class-level initializer is needed.
  • Calling _recorder_end twice on stream cancel (once at the native-tool-call early-return, once in the finally) is safe: end_llm_call sets _open_call = None on first close and no-ops on subsequent calls.
  • The cache_control placement on the last tool definition and on the system block follows the documented Anthropic API shape and is tested for both the non-streaming and streaming paths.
  • The FULL_CORE_TOOLS ∪ FULL_BUNDLES drift guard in test_full_tool_bundles.py correctly covers the flagship's 67-tool registry (the load_tools escape hatch is the one CORE tool absent from every bundle by design, asserted in test_core_is_subset_of_bundle_union).

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 the per-turn attributes class-level defaults beside their
neighbours, and marks the optional `print_turn_metrics` hook's unused
argument as deliberate.
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions — two 🟡 findings; the core implementation is correct, security-sound, and well-tested.


🟡 The eval baseline comparison required by CLAUDE.md is missing for every LLM-affecting change here. The system prompt lost 2,832 tokens, the tool block is now suppressed for native tool-calling models, the prompt order changed (static before volatile), and the flagship now sees at most 26 of 67 tools per ReAct step. CLAUDE.md is explicit: all four of those surfaces require a gaia eval agent run against the committed baseline before the change can be called done, and the PR itself names this gap ("Numbers still owed — blocked, not skipped") and explains Lemonade was banned due to hardware crashes. That context is valid, but it means this PR should hold at merge until the hardware is operational and the eval runs. A quality regression would affect every user of the flagship agent and is invisible to every unit test in this PR.


🟡 .perf/runtests.sh is hardcoded to one developer's local Windows machine and committed to the public repo. The file contains C:\Users\14255\Work\gaia\.claudia-worktrees\claudia-task-25e62f25 and C:\Users\14255\Work\gaia\.venv\Scripts\python.exe — a personal username and a session-specific worktree path that no other contributor can use. The worktree-guard this script provides is genuinely useful (the plan doc describes the exact mismeasurement it prevents), but the paths should come from environment variables with a clear error when unset, not literal strings. As committed, every other developer who tries to use it gets an abort on the gaia.__file__ check.


Everything else looks good. The three-tier binary bridge (ALLOW/CONFIRM/REFUSE) is correctly implemented end-to-end: Subcommand.__post_init__ refuses an action in both tiers at definition time, _validate_shell_command blocks REFUSE before any prompt is raised, skill_grant_covers_call uses validate_invocation (ALLOW-only) so CONFIRM falls through to the confirmation gate, and the leading-flag-before-action check prevents cobra-dispatch divergence. The _uses_native_tool_calls() refactor is a De Morgan equivalent of the original gate. The FULL_OPTIONAL_TOOLS mechanism is fail-safe. Tests for bundle coverage, turn metrics, prompt composition, Claude caching, and binary tiers are well-structured.

🔍 Technical details

Eval requirement (🟡 — docs/plans/gaia-agent-latency.md, Phase 2 table)

The plan doc's own "Numbers still owed" table at the end of Phase 2 lists the eval as item 1. The CLAUDE.md rule: "Changes 1, 3 and 4 all touch LLM-affecting surfaces (system prompt, tool schemas) and require gaia eval agent against the committed baseline before they can be called done." The specific gate: gaia eval agent --category <cat> then --compare tests/fixtures/eval_baselines/gemma-4-e4b-d71cd914/scorecard_<cat>.json <run>/scorecard.json. Subscription auth (inside a Claude Code session) is sufficient; ANTHROPIC_API_KEY not required.

Hardcoded paths (🟡 — .perf/runtests.sh lines 6–7)

W='C:\Users\14255\Work\gaia\.claudia-worktrees\claudia-task-25e62f25'
PY='C:\Users\14255\Work\gaia\.venv\Scripts\python.exe'

A parameterized version:

W="${GAIA_WORKTREE:-$(pwd)}"
PY="${GAIA_PYTHON:-python}"
if [ -z "$GAIA_WORKTREE" ]; then
    echo "Set GAIA_WORKTREE to the absolute path of this worktree"; exit 1
fi

The skill audit returned REVIEW on a high finding — the body reads as
instructing the model to conceal an action from the user.

The intent was the opposite: the sentence before already requires
reporting the refusal openly and naming the command. Phrased as "do not
tell the user that approving it is an option", it is genuinely ambiguous.
Saying what it means instead — these are refused rather than gated, so no
approval exists — keeps the behaviour and clears the finding.
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions

This PR ships three well-engineered improvements: a 53% cut to the flagship's fixed LLM prefill per call, opt-in Anthropic prompt caching, and an upgrade of the gh skill gate from read-only-only to a proper three-tier ALLOW/CONFIRM/REFUSE model. The architectural work is careful — the FULL_CORE_TOOLS/FULL_BUNDLES drift guard, the Subcommand.__post_init__ overlap check, the Windows shell=False fix for skill-granted CLIs, and the idempotent turn-recorder teardown all show deliberate design. Test coverage is thorough for the structural and security properties.

🟡 Eval not run — required by CLAUDE.md before merge. Four LLM-affecting surfaces changed: the system prompt section order reversed (volatile last), the ==== AVAILABLE TOOLS ==== block removed for native-tool-calling models, dynamic tool loading enabled for the flagship, and gaia-voice trimmed from ~2,145 to ~692 tokens. CLAUDE.md states: "Skipping the eval is how regressions that pass every unit test still ship to users." The plan doc (docs/plans/gaia-agent-latency.md) honestly names this as "blocked, not skipped" due to the machine crash — but the gate still has to close before merge. Once Lemonade is available: gaia eval agent --category <cat>, then --compare tests/fixtures/eval_baselines/gemma-4-e4b-d71cd914/scorecard_<cat>.json <run>/scorecard.json.

🟢 .perf/runtests.sh has hardcoded developer-specific paths. The guard script includes absolute Windows paths (C:\Users\14255\Work\gaia\.claudia-worktrees\claudia-task-25e62f25, C:\Users\14255\Work\gaia\.venv\Scripts\python.exe) that exist only on the author's machine. The .gitignore in .perf/ explicitly commits this file, so another contributor running it gets an immediate abort. Either parametrize the worktree root (e.g. derive it from $(dirname "$(dirname "$0")") on a POSIX box, or add a comment that the two variables must be edited before use) or drop it from version control — the offline_prefill.py is the reproducible artifact, not the runner.

🔍 Technical details

Eval gate — which categories matter most

The surfaces that changed are exactly the ones rag_quality and file_io eval categories exercise. At minimum those two need a green comparison before merge; if gaia_agent is a separate category it should run too.

.perf/runtests.sh — simplest portable fix

# Replace the two hardcoded lines with:
W="$(cd "$(dirname "$0")/.." && pwd)"   # repo root, works on POSIX
PY="${VIRTUAL_ENV}/Scripts/python.exe"  # or just `python` on the PATH

On Windows under Git Bash / MSYS the cd-based derivation still works; a comment noting "edit PY to your venv" is enough for the Windows case.

Security: Windows shell=False for skill-granted CLIs — confirmed correct

lone_granted_segment properly guards the single case where shell=True must be suppressed: one segment, the binary is granted, no pipeline. The comment in shell_tools.py:928–1149 correctly identifies that cmd.exe would expand %VAR% and treat | as an operator on the raw string, bypassing all validation. Pipeline segments remain gated by the len(segments) == 1 check.

_mixin_prompt_origins keyed on fragment text — theoretical collision

If two different get_*_system_prompt methods return byte-identical strings, the second one's method name overwrites the first in origins, and the first fragment may be misclassified (volatile when it should be static or vice versa). Extremely unlikely in practice, but worth noting: the dict is keyed on content, not identity.

_recorder_end(stats={}) in finally of send_messages_stream

stats={} is not None, so it's passed to end_llm_call. If the call is already closed (_open_call is None), end_llm_call is a no-op. If the stream was cancelled mid-flight, the call records empty stats. Both paths are correct; the comment in the code explains the intent.

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.
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions

The system-prompt optimization is well-engineered — _uses_native_tool_calls() as a single source of truth, the static-before-volatile reordering for KV-cache coherence, the 3-tier binary grant model with validate_invocation correctly returning CONFIRM as non-None so pre-authorization stays read-only, and the offline measurement harness that stubs the embedder and arms every requests verb to raise. Two things need attention before merge.


🟡 gaia eval agent is required but not yet run — four LLM-affecting surfaces changed: the system prompt lost 2,832 tokens and was reordered; the ==== AVAILABLE TOOLS ==== block is now suppressed for native tool-calling models; the flagship model now sees ≤26 of 67 tools per turn. CLAUDE.md requires gaia eval agent against the committed baseline before any of these can be called done. The plan doc acknowledges this explicitly ("Lemonade was banned") — that is an honest accounting, but the policy is unambiguous. The eval must run and the scorecard diff must be in the PR before merge, or a dedicated follow-up PR must land the baseline update the moment Lemonade is back.

🟡 .perf/runtests.sh commits personal machine paths — the script is explicitly tracked (.perf/.gitignore exempts it), and the plan doc says to use it rather than bare pytest to avoid measuring the wrong worktree. But the file contains W='C:\Users\14255\Work\gaia\...' and a hardcoded PY= path that only exist on the author's machine. The guard is correct and will abort loudly; the problem is the message will say "gaia resolves to C:\Users\... (not this worktree)" to a developer who has never seen that path, with no hint that the script itself needs editing. Either document it as a template to copy-and-edit (a header comment stating the two lines to update), parametrise from $WORKTREE / $PYTHON, or exclude it from tracking and ship only offline_prefill.py.


🔍 Technical details

eval gatesrc/gaia/agents/base/agent.py::_compose_system_prompt (new _uses_native_tool_calls gate, new static/volatile split), hub/agents/gaia/python/gaia_agent/agent.py (dynamic_tools=True, dynamic_tools_max=26), hub/agents/gaia/python/gaia_agent/skills/gaia-voice/SKILL.md (body trimmed from ~2,145 → ~692 tiktoken tokens). Per CLAUDE.md: "changes that REQUIRE an eval run before merge" include system-prompt composition order, tool schemas sent, and is_tool_calling_model mapping effects. The plan doc's docs/plans/gaia-agent-latency.md explicitly lists the eval as row 1 of "Numbers still owed", so the gap is documented — it just needs to be resolved, not acknowledged.

runtests.sh.perf/runtests.sh:5-6 hardcodes W and PY. The guard at line 10 (case "$resolved" in *claudia-task-25e62f25*) ;;) references the specific worktree name, so any other developer's copy of the branch would see ABORT: gaia resolves to ... (not this worktree). Adding two lines at the top of the script is the cheapest fix:

# Edit these two lines for your machine before running:
W='<absolute path to this worktree>'
PY='<absolute path to your Python executable>'

Alternatively, replace with W="$(git -C "$(dirname "$0")/.." rev-parse --show-toplevel)" (works on both platforms when running from a worktree) and PY="$(which python)".

test_optional_tools_are_present_on_a_full_installhub/agents/gaia/python/tests/test_full_tool_bundles.py:918. FULL_OPTIONAL_TOOLS includes search_documentation, which only registers when npx is on PATH. The test asserts every FULL_OPTIONAL_TOOLS name is present in the flagship registry. If CI lacks npx, this test fails with "declared optional but absent even on a full install: ['search_documentation']" — which is exactly the condition the optional set is designed to tolerate. Consider skipping the search_documentation check when shutil.which("npx") is None, or adding a CI gate that ensures npx is available before this test file runs.

@itomek itomek assigned itomek and unassigned itomek Aug 24, 2026
@kovtcharov-amd
kovtcharov-amd added this pull request to the merge queue Aug 24, 2026
Merged via the queue into amd:main with commit a4b84fa Aug 24, 2026
75 checks passed
kovtcharov-amd pushed a commit to kovtcharov/gaia that referenced this pull request Aug 24, 2026
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 added a commit to kovtcharov/gaia that referenced this pull request Aug 25, 2026
)

Most of this branch already reached `main` with the amd#3022amd#3026 stack.
It has been merged down to what never landed: **13 files instead of 35,
+478/−261 instead of +3738**.

Three things the `--dev` turn 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:

- A shell command that ran in 1.3s was recorded as **322.6s** — the
approval prompt blocks inside the tool call, so the time someone spent
deciding was billed to the tool. Approval is now timed separately and
shown as its own `waiting on you` figure, leaving `tool_s` and
`overhead_s` meaning what they say.
- Cache reuse was understated **3.6×**. The proxy prompt appended the
tool schemas *after* the conversation, but chat templates inject them
alongside the system block at the front, so the shared prefix stopped at
the first history message. A turn whose entire 12.2k system+tools header
was reusable reported 27%.
- A missing time-to-first-token drew as `0.0s`, which reads as an
instant first token. It now reads `--`.

It also clears the reviewer findings that rode onto `main` unfixed with
that stack. The one contributors actually hit: `.perf/runtests.sh` was
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 on `main`, 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.

<details>
<summary>🔍 Technical details</summary>

Two reviewer findings were checked and are **not** valid as written:

- The `AVAILABLE TOOLS` assertion in
`tests/mcp/test_mcp_cli_to_agent_workflow.py` is fine. The premise was
that an agent with no `model_id` falls back to `DEFAULT_MODEL_NAME` and
so takes the native path; in fact `Agent.__init__` stores the raw `None`
(`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 is `None` there.
- `os.path.commonprefix` is 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_tools`
re-derived the predicate that `_uses_native_tool_calls` documents 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_metrics` is
called only from the printed-answer branch, so a turn that burns its
step budget shows no `--dev` breakdown. The obvious fix (publish on the
tail too) introduces a **cross-turn leak**:
`SSEOutputHandler.print_turn_metrics` only stashes, and
`print_final_answer` consumes-and-clears, so stashing on a path with no
answer event would attach that record to a later turn. Worth its own
change.
</details>

## 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
locally
- [ ] `cd tui && go build ./... && go test ./internal/ui/chat/
./internal/event/`
- [ ] `bash .perf/runtests.sh tests/unit/test_turn_metrics.py -q
--collect-only` prints `[guard] gaia -> …` and collects, instead of
aborting; run from outside a checkout it still aborts with exit 2
- [ ] `git diff main...HEAD` touches 13 files and reverts nothing from
amd#3026 — the cache counters, the `execute_python_file` confirmation gate,
ChatAgent's `output_handler`, and the confirmation-summary disclosure
limits are all still main's

---------

Co-authored-by: Ovtcharov <kovtchar@amd.com>
Co-authored-by: kovtcharov-amd <kalin.ovtcharov@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents chat Chat SDK changes documentation Documentation changes llm LLM backend changes performance Performance-critical changes tests Test changes tui Go terminal UI (gaia-tui)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants