Skip to content

feat(tui): show where a turn's time went, not just how long it took - #3024

Closed
kovtcharov wants to merge 4 commits into
amd:mainfrom
kovtcharov:claudia/task-71e40236
Closed

feat(tui): show where a turn's time went, not just how long it took#3024
kovtcharov wants to merge 4 commits into
amd:mainfrom
kovtcharov:claudia/task-71e40236

Conversation

@kovtcharov

Copy link
Copy Markdown
Contributor

The TUI already tells you a turn took 34 seconds. It has never been able to tell you why, and the reason is a number nothing measured: the system prompt plus tool schemas re-sent on every single LLM call. On this machine Lemonade prefills at roughly 390 tok/s, so a 17k fixed prefill is ~44 seconds of every turn before the model writes a word — invisible behind an elapsed-time figure that looks like the model is just slow. This adds an opt-in per-turn record that measures it, and shows it.

What this adds beyond today's stats line

Today --dev prints 12.0s · ttft 11.9s · 1 steps. Every one of those describes the outcome; none decomposes it. New:

  • Fixed prefill size on that same line… · 1 steps · 17.0k prefill. One number, and it is usually the answer.
  • Cached vs new input tokens per call, so you can see whether the KV-cache prefix was actually reused between steps or the whole prompt was re-read. prefill tok/s next to it shows which.
  • Where the seconds went — model vs tools vs agent overhead, split.
  • Absolute timestamps on the turn and every call, so a session lines up against Lemonade's own log or a screen recording.
  • A per-step table, because a turn is slow due to one step re-reading the prompt and an aggregate hides which.
  • A JSONL log (GAIA_TURN_LOG), append-only, so a session is greppable and two builds are diffable.

With GAIA_TURN_LOG unset none of this exists: the recorder is never constructed and the wire payload is byte-identical to before (asserted by a json.dumps equality test including key order).

34.5s · ttft 2.1s · 210 tokens · 6.5 tok/s · 2 steps · 1 tools · 17.0k prefill
  turn a3f1c2  22:47:35Z → 22:48:09Z  34.5s total
  model 28.4s · tools 4.8s · overhead 1.3s
  in 51,204 tok (38,110 cached, 13,094 new · 74% hit) · out 210 tok
  prefill 17.0k fixed · 66 tools · skills: gaia-voice
  model id Gemma-4-E4B-it-GGUF
  step 1   12.8s  ttft  4.9s  in 17,204 (0 cached)  3511 tok/s prefill
          └ run_shell_command 2.1s
  step 2    9.1s  ttft  0.4s  in 17,260 (17,204 cached)

⚠️ Required gate NOT run: gaia eval agent

This touches src/gaia/agents/base/agent.py, so CLAUDE.md requires an agent eval before merge. It has not been run. Lemonade is banned on the machine this was developed on (it crashes the host), and no query was sent to any local backend during this work.

The change is structurally low-risk for LLM behaviour — it adds no prompt text, changes no tool schema, and touches no prompt-assembly code — but that is an argument, not an eval. Someone with a working Lemonade box needs to run gaia eval agent against the committed baseline before this merges.

🔍 Bug found while instrumenting, NOT fixed here

In a Claude session the agent logs [PARSE] tool_call_path=native model_id=Gemma-4-E4B-it-GGUF while every request goes to api.anthropic.com. Agent.model_id keeps the configured local id under --use-claude; the substitution happens later, in AgentSDK.effective_model.

That stale id feeds is_tool_calling_model(), which selects the tool-call parsing strategy — so on Claude it currently picks Gemma's strategy and happens to work. That is luck, not design, and it will break the first time the two strategies diverge.

I fixed only the part inside this change: the turn record and the model id line above report AgentSDK.effective_model, the model actually used. The parsing-strategy selection is untouched and still wrong — it reaches into call sites this PR has no business editing, and deserves its own issue.

Conflicts with other open PRs

Test plan

# Go — build, vet, and the full suite including the ~4min integration tier
cd tui && go build ./... && go vet ./... && go test ./...

# Python — the wire path, both directions of the env gate
python -m pytest tests/unit/test_turn_metrics_wire.py -q

# The files that exercise every module this touches
python -m pytest tests/unit/test_sse_narration.py tests/unit/test_streaming_tool_calls.py \
  tests/unit/test_skill_binary_grants.py tests/unit/test_agent_token_usage.py -q
  • go build ./... && go vet ./... && go test ./... under tui/ — green (integration tier included)
  • pytest tests/unit/test_turn_metrics_wire.py — 19 pass
  • The 41 unit-test files touching the changed modules produce an identical failure set to a stashed clean tree (verified locally: same failures, +19 passing). tests/unit as a whole has ~600 failures on main on this machine that are unrelated to this change.
  • gaia eval agent — NOT RUN, see above. Blocks merge.
  • Eyeball the dev block: GAIA_TURN_LOG=~/.gaia/turns.jsonl gaia --dev, ask anything, confirm the block appears and jq . ~/.gaia/turns.jsonl parses
  • Confirm the quiet path is untouched: run without GAIA_TURN_LOG and check the footnote is exactly what it was

Reviewer notes

Four timing defects were found in review and fixed here, each with a test that fails without its fix (verified by reintroducing the defect): a cancelled turn abandoned its open LLM call and charged those seconds to overhead; _process_query_impl re-raises on the wrong-ctx reload and left the recorder attached to fold the retry's calls into the abandoned turn; every non-streaming call charged its /stats HTTP round-trip to model time; and nested tool calls (CodeAgent's orchestration does this) were timed twice while a tool that raised recorded as ok: true.

The cached/new token counts come from a client-side estimator (tiktoken when installed), not the model's own tokenizer — they are honest as ratios and deltas, not as exact model token counts. out is the backend's own number. This is stated in the docs and in the module docstring rather than left for someone to discover.

src/gaia/agents/base/turn_metrics.py swallows its own failures by design — a metrics bug must never be able to fail a user's turn. That exception is documented in the module docstring and is deliberately narrow: it covers recording only, never the agent's own work.

The TUI's per-turn line reported elapsed time, ttft, steps and tools — none
of which explain why a local turn is slow. The dominant cost is prefill: the
system prompt and tool schemas are re-sent on every LLM call, and at the
~390 tok/s this machine prefills at, prompt size converts almost directly to
seconds. That number was never measured, let alone shown.

Adds an opt-in per-turn record (GAIA_TURN_LOG) carrying fixed prefill size,
cached-vs-new input tokens per call, output tokens, the wall-time split
across model/tools/overhead, and absolute timestamps. It rides the existing
`answer` event under a `metrics` key and reaches the TUI through the
canonical translator, so the Go client and the HTTP surface both get it.
With the var unset the wire payload is byte-identical to before.

The record names the model the turn's requests actually went to
(AgentSDK.effective_model), not the configured local id — under
--use-claude those differ.
@github-actions github-actions Bot added documentation Documentation changes chat Chat SDK changes tests Test changes agents tui Go terminal UI (gaia-tui) labels Aug 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions

This adds an opt-in per-turn performance record — where a turn's seconds went, how big the fixed prefill is, and how much of the input the KV cache could reuse — written to a JSONL file and drawn under the answer in --dev. Off by default, and the tests actually prove "off" means byte-identical output rather than merely metrics-free, which is the right thing to have pinned.

One thing to fix before merge: the Agent UI SSE contract spec still lists the five fields a final event's usage object can carry, and this PR adds a sixth. The spec is the doc a client author reads to know what can show up on the wire, so it needs the new field named there alongside the code.

The rest are small and optional:

  • On the non-streaming path, closing the record reaches for backend stats even when the request just failed — including on a cancel. The streaming path deliberately avoids exactly that, so the two paths should agree.
  • The --dev breakdown hangs each tool under its step. A tool recorded against a step that has no backend call of its own vanishes from the display, even though its seconds still count in the totals.
  • The log file records each turn's full prompt text. Worth one line in the dev docs so nobody is surprised by what lands on disk.

Real-world evidence

No evidence-bundle.md was produced for this run, and the environment this review ran in had no working shell, so I could not read the PR description to check whether the author attached evidence there. Treat the verdict as resting on static review plus the committed tests.

For the record, the surface here is the TUI's --dev breakdown plus a new field on the Agent UI answer/final event. The unit tests cover both ends of that wire (the Python emitter and the Go decoder, including a malformed record), but no capture of the real block rendered under a real answer is present. A terminal capture of gaia --dev with GAIA_TURN_LOG set, and a peek at one line of the resulting JSONL, would close that gap — not a blocker, but it is the one artifact that would show the numbers are right rather than merely well-plumbed.

🔍 Technical details

Issues

🟡 The canonical SSE contract spec doesn't know about usage.metrics (docs/spec/agent-ui-query-sse-contract.md:263, :417)

The spec enumerates the final event's usage payload as // optional {steps?, tools_used?, elapsed?, tokens?, ttft?}, and §6.1's transform table spells out the answer → final mapping as elapsed/steps/tools_used/tokens/ttftusage. sse_translation.py:387 now adds a sixth pair, and sse_handler.py:846 adds metrics to the source answer event. Per CLAUDE.md ("a functional change must update EVERY doc that describes it"), both spots need the new field, with a note that it is present only when GAIA_TURN_LOG is set so a client treats absence as normal — which is exactly what canonical.go's doc comment already tells Go callers.

    "usage":  { "type": "object" }   // optional {steps?, tools_used?, elapsed?, tokens?, ttft?, metrics?}

🟢 Non-streaming close fetches /stats on the failure path (src/gaia/chat/sdk.py:339-345)

_recorder_end() with no argument falls through to self.get_stats(), which on Lemonade is a live HTTP round-trip. In the finally it fires even when llm_client.chat just raised, and when config.show_stats is on it fires twice per call (again at line 345). The streaming path takes the opposite position on purpose — _recorder_end(stats={}) with the comment "a cancel must start no HTTP request". Worth making the two agree; passing the already-fetched dict on the success path and {} on the exception path costs nothing.

🟢 Tool rows with no matching backend call are dropped from the breakdown (tui/internal/ui/chat/turnmetrics.go:52-77)

stepLines iterates t.LLMCalls and prints toolsByStep[c.Step]. Any tool recorded at a step with no llm_call of the same index — an agent whose chat is None, so _execute_tool stamps step=0, or a tool that ran outside the step loop — never renders, while its seconds still land in Totals.ToolS. That reads as unexplained tool time. A trailing "unattributed" row, or keying off the union of both step sets, would close it.

🟢 _publish_turn_metrics is skipped on the cancelled-turn path (src/gaia/agents/base/agent.py:6343)

The cancel branch seals the record (so the JSONL line is written) but returns before publishing, so a cancelled turn's breakdown never reaches a console. Probably intended — cancel emits no answer event for it to ride — but the asymmetry with line 6276-6279 isn't stated anywhere and the comment above it only explains the seal.

🟢 Dev docs don't say the log captures prompt text (docs/reference/dev.mdx:341-346)

TurnRecorder.finish stores "query": query verbatim (answers are reduced to answer_chars, which is the careful choice), and end_llm_call stores the backend's stats_raw unfiltered. The docs describe the record as sizes and timings. One line — "the record includes the turn's prompt text" — saves a developer from discovering that when they paste the file into an issue.

Strengths

  • The negative test is the one that matters, and it's the strongest test here. test_off_by_default_payload_is_unchanged compares serialized JSON rather than asserting "metrics" not in event, so a reordered or null-placeholder key fails it; test_a_stashed_record_alone_does_not_leak pins that the env var, not the record's presence, is the gate.
  • The Go decoder isolates the blast radius correctly. Keeping the record as json.RawMessage in usageWire means a record whose inner shape drifts costs only the breakdown, not steps/tokens/ttft — and turnmetrics_test.go has the malformed-record case to prove it.
  • The unhappy timing paths are genuinely handled, not just claimed. mark_llm_call_end stopping the clock before the /stats round-trip, the nested-tool depth guard, the abandoned-generator finally, and the idempotent seal each have a test that fails if the guard is removed — including one that fails loudly if a cancelled turn reaches for stats.

@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve

Clean implementation of opt-in per-turn performance recording. The zero-overhead default (recorder never constructed unless GAIA_TURN_LOG is set) is the right design for a developer diagnostic, and the code is careful about every exit path: finally blocks in _execute_tool, send_messages_stream, and process_query all guarantee the recorder is detached, and _finish_turn_record is idempotent so double-calls are harmless. No correctness issues found.

The test suite is thorough where it matters — the negative cases (an ordinary turn's payload is byte-identical to before, a stashed record doesn't leak without the env var) and the unhappy paths (abandoned streams, nested tool calls counted once, raising turns that still seal their record) are all covered.

🔍 Technical details

One observation worth noting for future readers: _common_prefix_len in turn_metrics.py:431 does a character-by-character scan over the rendered prompt (potentially 50 k+ chars) on every LLM call. Since this only runs when GAIA_TURN_LOG is set it is not a prod concern, but a os.path.commonprefix-style byte comparison or a zip-based short-circuit would be faster if the rendered prompts grow.

The json:"-" tag on CanonicalUsage.Metrics in canonical.go:1339 is intentional — the field is in-memory only and must not be re-marshalled — but it means any future json.Marshal(usage) call silently drops the metrics. Worth a comment if that path ever opens up.

Neither of these is a blocker.

Ovtcharov added 2 commits August 21, 2026 17:45
Unit tests and the lint gate both failed on this branch.

The coercion test read the source of `_execute_tool` looking for
`_coerce_tool_args`. Timing moved the dispatch body into
`_execute_tool_impl`, so the string it wanted was one function away and
the assertion failed even though coercion still runs before every tool.
It now reads the impl and asserts the delegation, so splitting the method
again fails loudly instead of quietly testing the wrong function.

`query`, `agent_name` and `model_id` were passed to TurnRecorder twice —
once to the constructor, which ignored all three, and again to finish().
That is what pylint reported as unused arguments, but the duplication was
the real problem: `_recorded_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 genuinely new.

Also drops an `import time` the module-level import made redundant, gives
`_tool_timing_depth` a class-level default beside the other per-instance
attributes, and marks the optional `print_turn_metrics` hook's unused
argument as deliberate.
A trailing pylint pragma pushed the def past the line limit, and black
will not move a comment to fix it.
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve

Per-turn performance recording is a well-designed opt-in developer feature. Several things stand out as correctly handled: the gate on GAIA_TURN_LOG means ordinary turns pay zero cost; the two-stage Go decode (usageWire / CanonicalTurnStats) isolates a malformed record so it can't erase the step/token counts the user sees on every turn; the _tool_timing_depth guard prevents double-counting nested tool calls; and the idempotent seal covers all exit paths — normal answer, early-exit, max-steps, and raised exceptions without a seal.

Test coverage is unusually thorough: cancelled streams, nested tools, raising tools, broken console hooks, stale-record-into-next-turn, and the ordering invariant (seal before print_final_answer) are all exercised with the right level of source-reading tests.

One 🟢 nit on the Go side: CanonicalTurnTotals.OutputTokensServer and CanonicalTurnCall.OutputTokens are declared float64, but token counts from the Python record are always integers. The render path converts back with int(...), which works, but storing a count as a float is a latent footgun if someone adds arithmetic against those fields later.

🟡 Real-world evidence: The --dev TUI metrics block is a user-visible surface (rendered to the terminal). Per the project's evidence policy, a CLI-visible change should include the real command output alongside the change. The docs show the expected output, and the rendering is unit-tested, but if the PR description lacks a real gaia --dev run showing the block, that's the one artifact worth adding.

🔍 Technical details

Float vs int token fields in Go (tui/internal/event/canonical.go:1449, tui/internal/event/canonical.go:1427):

// current
OutputTokensServer float64 `json:"output_tokens_server"`
// suggestion
OutputTokensServer int `json:"output_tokens_server"`

Same for CanonicalTurnCall.OutputTokens. JSON integers unmarshal to both float64 and int cleanly; the only downside of float64 is that future callers may not know to round before arithmetic.

Evidence gap (conditional): If the PR description already includes a gaia --dev session showing the metrics block, this is not a flag. The rubric asks for "the real gaia <subcommand> and its output" for CLI-visible changes. TestTurnMetricsBlockShowsTheBreakdown is a strong proxy but not a substitute.

Everything else reviewed and clean:

  • _execute_tool timing wrapper: correct — ok=False default propagates through exceptions, finally records, exception re-raises.
  • Idempotent _finish_turn_record: second call sees _turn_recorder is None (cleared by the first call) and returns None immediately. One write per turn confirmed by test_finish_is_idempotent.
  • _recorder_end called twice (normal path + finally): second call sees _open_call is None (cleared by end_llm_call) and is a no-op. Confirmed by test_closing_twice_records_one_call.
  • clockOf with fractional-second ISO timestamps: Go's time.Parse accepts fractional seconds even when the layout (time.RFC3339) doesn't declare them — the parser is more permissive than the format.
  • devPayloadStyle in turnmetrics.go: defined at model.go:101, pre-existing.
  • The # noqa: BLE001 exception pattern: deliberately broad but narrowly scoped to diagnostics, logged with context, and documented in the module docstring. Not the same as the pre-existing silent pass blocks.

@itomek itomek assigned itomek and unassigned itomek Aug 24, 2026
pull Bot pushed a commit to bhardwajRahul/gaia that referenced this pull request Aug 24, 2026
…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>
@kovtcharov-amd

Copy link
Copy Markdown
Collaborator

Closing as superseded: the per-turn timing breakdown is already live on main (landed via #3026) and now carries more fields than this PR did — the one genuine remainder, a turn log that records the local model id under --use-claude, will be filed separately rather than held behind 1,680 lines.

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 tests Test changes tui Go terminal UI (gaia-tui)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants