feat(dashboard): account for every LLM call on the comparison page #92

Merged

rcsheets merged 1 commit from feat/per-pass-telemetry into main 2026-07-28 06:07:41 +00:00

Conversation 2 Commits 1 Files changed 15 +1035 -43

rcsheets commented 2026-07-28 05:04:38 +00:00

Owner

Copy link

A full review makes four kinds of call — preflight, context discovery, 0–5 parallel fragment extracts, then one review per enabled config. A two-config fan-out with five fragments is nine calls; the comparison page could account for two.

Every telemetry field on review_events (input_tokens, output_tokens, duration_ms, finish_reason, prompt_*) describes the review call and nothing else. The other passes' numbers existed only in a log.Info line.

Two problems, not one

Cost was incomplete. A column reading 1,200+800 tok omitted preflight, discovery, and up to five extracts. On a full review with fragments the shared spend can exceed the review call it's being compared against.

What was shown was misattributed. Preflight and discovery run on different models than the config in the column. Most visibly complexity: preflight's output, on preflight's model, rendered inside every config's meta line as though that config produced it (admin_compare.html:45, fed by shadowEventFrom copying it to every row).

Schema

review_passes, one row per call, anchored on comparison_group_id — preflight/discovery/extract run once and feed every config, so they belong to the group, not to a row. config_id is NULL for those and set for review calls. seq orders the extract fan-out, which completes out of order.

Page

• Shared pipeline section above the columns: each pass with model, tokens, duration, failure if any, and a drill-down to what it was sent and what it returned — matching what the review pass already offered.
• Complexity and observations move there, since they are preflight's.
• Columns keep their review call and gain a standalone figure (own call + shared), labelled as not summable across columns, because the shared passes ran once for the whole fan-out. Adding two columns would double-count them.
• Page header carries the review-wide total, every call counted once — the number that could not be produced at all before.

Groups predating the migration have no pass rows: the section and totals disappear, columns render as before.

Notes

Prompts are captured pre-call, with the same helpers the backends use, so a pass that fails still records what it was asked. That required lifting the extract user message out of all three backends — which had built it inline and identically — into reviewer.BuildExtractUserMessage. A second rendering that merely looked similar would be a reconstruction, and would drift.

Two smaller things fell out:

• Runner.PassSink seam, so the telemetry a review emits is assertable without a database.
• The compare page's template data is now a named compareView. It was anonymous, so every render test restated it — adding a field broke all four at render time rather than compile time. Now they bind the handler's own type.

Storage: prompts stored per pass, following migration 000011's precedent. Expect roughly 2–3x the prompt bytes per review. The admin storage page reads pg_stat_user_tables, so the new table appears there with no changes — I checked rather than assumed.

Tests: pass rows emitted per call including failed calls, group/config attribution, splitPasses, pipeline rendering, drill-down, failed-pass display, the pre-migration empty case, and a guard that complexity appears once rather than per column.

🤖 Generated with Claude Code

A full review makes four kinds of call — preflight, context discovery, 0–5 parallel fragment extracts, then one review per enabled config. A two-config fan-out with five fragments is **nine calls**; the comparison page could account for **two**. Every telemetry field on `review_events` (`input_tokens`, `output_tokens`, `duration_ms`, `finish_reason`, `prompt_*`) describes the review call and nothing else. The other passes' numbers existed only in a `log.Info` line. ## Two problems, not one **Cost was incomplete.** A column reading `1,200+800 tok` omitted preflight, discovery, and up to five extracts. On a full review with fragments the shared spend can exceed the review call it's being compared against. **What was shown was misattributed.** Preflight and discovery run on *different models* than the config in the column. Most visibly `complexity`: preflight's output, on preflight's model, rendered inside every config's meta line as though that config produced it (`admin_compare.html:45`, fed by `shadowEventFrom` copying it to every row). ## Schema `review_passes`, one row per call, anchored on `comparison_group_id` — preflight/discovery/extract run **once** and feed every config, so they belong to the group, not to a row. `config_id` is NULL for those and set for review calls. `seq` orders the extract fan-out, which completes out of order. ## Page - **Shared pipeline section** above the columns: each pass with model, tokens, duration, failure if any, and a drill-down to what it was sent and what it returned — matching what the review pass already offered. - **Complexity and observations move there**, since they are preflight's. - **Columns** keep their review call and gain a standalone figure (own call + shared), labelled as *not* summable across columns, because the shared passes ran once for the whole fan-out. Adding two columns would double-count them. - **Page header** carries the review-wide total, every call counted once — the number that could not be produced at all before. Groups predating the migration have no pass rows: the section and totals disappear, columns render as before. ## Notes Prompts are captured **pre-call**, with the same helpers the backends use, so a pass that fails still records what it was asked. That required lifting the extract user message out of all three backends — which had built it inline and identically — into `reviewer.BuildExtractUserMessage`. A second rendering that merely looked similar would be a reconstruction, and would drift. Two smaller things fell out: - `Runner.PassSink` seam, so the telemetry a review emits is assertable without a database. - The compare page's template data is now a named `compareView`. It was anonymous, so every render test restated it — adding a field broke all four at *render* time rather than compile time. Now they bind the handler's own type. **Storage:** prompts stored per pass, following migration 000011's precedent. Expect roughly **2–3x the prompt bytes per review**. The admin storage page reads `pg_stat_user_tables`, so the new table appears there with no changes — I checked rather than assumed. Tests: pass rows emitted per call including failed calls, group/config attribution, `splitPasses`, pipeline rendering, drill-down, failed-pass display, the pre-migration empty case, and a guard that complexity appears once rather than per column. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

rcsheets added 1 commit 2026-07-28 05:04:38 +00:00

feat(dashboard): account for every LLM call on the comparison page ... 709c565a7c

A full review makes four kinds of call — preflight, context discovery,
0-5 parallel fragment extracts, then one review per enabled config — so a
two-config fan-out with five fragments is nine calls. The comparison page
could account for two of them. Every telemetry field on review_events
(input_tokens, output_tokens, duration_ms, finish_reason, prompt_*)
describes the review call and nothing else; the other passes' numbers
existed only in a log line.

Two things were wrong as a result. Per-config cost omitted the shared
passes entirely, which on a full review with fragments can exceed the
review call being compared. And because preflight and discovery run on
different models than the config in the column, what cost was shown was
not attributable to that column in the first place — most visibly the
complexity, which is preflight's output on preflight's model, rendered
inside every config's meta line as though the config produced it.

review_passes stores one row per call, anchored on comparison_group_id
because preflight/discovery/extract run once and feed every config;
config_id is NULL for those and set for review calls. seq orders the
extract fan-out, which completes out of order.

The page grows a shared-pipeline section above the columns — each pass
with its model, tokens, duration, failure if any, and a drill-down to
what it was sent and what it returned, matching what the review pass
already offered. Complexity and observations move there. Columns keep
their review call and gain a standalone figure (own call plus shared),
labelled as not summable across columns, since the shared passes ran once
for the whole fan-out. The page header carries the review-wide total,
counted once — the number that could not be produced at all before.

Prompts are captured pre-call with the same helpers the backends use, so
a pass that fails still records what it was asked. That required lifting
the extract user message out of all three backends, which had built it
inline and identically, into reviewer.BuildExtractUserMessage — a second
rendering that merely looked similar would be a reconstruction, and would
drift.

Groups predating the migration have no pass rows: the section and the
totals disappear and the columns render as before.

Two smaller things fell out. Runner grows a PassSink seam so the
telemetry a review emits is assertable without a database. And the
compare page's template data becomes a named compareView — it was
anonymous, so each render test restated it, and adding a field broke all
of them at render time rather than compile time.

Storage: prompts are stored per pass following the precedent of migration
000011, so expect roughly 2-3x the prompt bytes per review. The admin
storage page reads pg_stat_user_tables, so the new table shows up there
without changes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

pr-reviewer-bot commented 2026-07-28 05:04:39 +00:00

Collaborator

Copy link

Automated review by pr-reviewer v0.41.5 | Safety Check | Claude Sonnet 5 | tracking id r-683867-aab533
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

---

⚠️ The primary review (Mistral Small) failed. The review below is from Claude Sonnet 5, which reviewed the same diff.

✅ Verdict: LGTM — Large but well-tested, backward-compatible feature addition with additive migration, best-effort telemetry writes, and no signs of secrets, injection, or breaking changes.

Went through the diff carefully given the size and the migration touch.

• Migration 000013 is additive (new table + index), with a matching down migration that drops both. No destructive changes to existing tables.
• RecordPass/ListPassesForGroup are guarded for s.pool == nil (degraded store), and pass recording throughout runner.go and discovery.go is explicitly best-effort — errors are logged and swallowed, never propagated to fail a review. This matches the stated goal of not letting telemetry break the actual review flow.
• The passCols/placeholder consistency is directly tested (TestPassColsMatchPlaceholders), which is a nice guard against the classic INSERT column/placeholder drift bug.
• BuildExtractUserMessage extraction removes duplicated inline logic from three backends into one shared helper — behavior-preserving refactor, and it's exercised by existing backend tests presumably still passing since the format string is identical.
• Template changes look additive; Complexity was removed from the per-column meta line (intentionally, per description) and moved to the shared/page-level section — consistent with the PR's stated fix for misattribution, and covered by TestComparePageDoesNotAttributeComplexityToConfigs.
• compareView struct is now named instead of anonymous, reducing test/handler drift risk — reasonable.
• No secrets, credentials, or debug artifacts. No obviously dangerous SQL (uses parameterized queries throughout, array_position with a Go slice param for ordering — fine, not injection prone).
• Nil safety in recordPass: it bails out if t.Event == nil || t.Event.ComparisonGroupID == nil, avoiding a nil pointer dereference for reviews that fail before a group is minted.

Nothing here looks like it would break existing functionality or leak sensitive data; the storage growth (2-3x prompt bytes) is called out explicitly and is a capacity/cost consideration rather than a bug. Tests cover the new code paths reasonably (pass emission, failure paths, split logic, template rendering, degraded-store behavior).

This is a large, well-tested, backward-compatible feature addition with no signs of accidentally committed files, secrets, or logic that would break production.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.41.5 | Safety Check | Claude Sonnet 5 | tracking id `r-683867-aab533`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- > ⚠️ The primary review (**Mistral Small**) failed. The review below is from **Claude Sonnet 5**, which reviewed the same diff. **✅ Verdict: LGTM** — Large but well-tested, backward-compatible feature addition with additive migration, best-effort telemetry writes, and no signs of secrets, injection, or breaking changes. Went through the diff carefully given the size and the migration touch. - Migration 000013 is additive (new table + index), with a matching down migration that drops both. No destructive changes to existing tables. - `RecordPass`/`ListPassesForGroup` are guarded for `s.pool == nil` (degraded store), and pass recording throughout `runner.go` and `discovery.go` is explicitly best-effort — errors are logged and swallowed, never propagated to fail a review. This matches the stated goal of not letting telemetry break the actual review flow. - The `passCols`/placeholder consistency is directly tested (`TestPassColsMatchPlaceholders`), which is a nice guard against the classic INSERT column/placeholder drift bug. - `BuildExtractUserMessage` extraction removes duplicated inline logic from three backends into one shared helper — behavior-preserving refactor, and it's exercised by existing backend tests presumably still passing since the format string is identical. - Template changes look additive; `Complexity` was removed from the per-column meta line (intentionally, per description) and moved to the shared/page-level section — consistent with the PR's stated fix for misattribution, and covered by `TestComparePageDoesNotAttributeComplexityToConfigs`. - `compareView` struct is now named instead of anonymous, reducing test/handler drift risk — reasonable. - No secrets, credentials, or debug artifacts. No obviously dangerous SQL (uses parameterized queries throughout, `array_position` with a Go slice param for ordering — fine, not injection prone). - Nil safety in `recordPass`: it bails out if `t.Event == nil || t.Event.ComparisonGroupID == nil`, avoiding a nil pointer dereference for reviews that fail before a group is minted. Nothing here looks like it would break existing functionality or leak sensitive data; the storage growth (2-3x prompt bytes) is called out explicitly and is a capacity/cost consideration rather than a bug. Tests cover the new code paths reasonably (pass emission, failure paths, split logic, template rendering, degraded-store behavior). This is a large, well-tested, backward-compatible feature addition with no signs of accidentally committed files, secrets, or logic that would break production.

rcsheets force-pushed feat/per-pass-telemetry from 709c565a7c to a144776b79 2026-07-28 06:05:10 +00:00 Compare

pr-reviewer-bot commented 2026-07-28 06:05:11 +00:00

Collaborator

Copy link

Automated review by pr-reviewer v0.41.7 | Safety Check | Claude Sonnet 5 | tracking id r-684697-31cde4
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

---

⚠️ The primary review (Mistral Small) failed. The review below is from Claude Sonnet 5, which reviewed the same diff.

✅ Verdict: LGTM — Additive migration and best-effort telemetry plumbing, well-tested including a guard against the column/placeholder mismatch risk in the raw SQL; nothing alarming found.

Went through the diff carefully given its size.

Key things checked:

• Migration: 000013_review_passes.up/down.sql is additive (new table + index), with a matching down migration that drops both. No destructive change to existing tables.
• Degraded-store handling: RecordPass/ListPassesForGroup both check s.pool == nil and no-op/return nil rather than erroring, consistent with the "telemetry must never fail a review" invariant, and covered by TestPassesDegradedStoreIsInert.
• Column/placeholder mismatch risk: this pattern (building an INSERT from a string list of columns plus a hardcoded placeholder count) is exactly the kind of thing that silently breaks at runtime. The PR added TestPassColsMatchPlaceholders guarding against drift, which is a reasonable mitigation. I double-checked passCols has 16 comma-separated fields and the VALUES clause has $1..$16 — they match.
• recordPass/recordReviewPass/discovery/preflight pass-recording: all wrapped as best-effort (log and continue), never propagated as fatal errors — consistent with "telemetry must never fail a review."
• Nil safety: t.Event == nil || t.Event.ComparisonGroupID == nil guarded before dereferencing in recordPass. rr (configRun) fields used in recordReviewPass (rr.start, rr.end, rr.promptSystem, etc.) aren't shown being initialized in this diff, but that's pre-existing code (configRun struct isn't touched here) so out of scope for this diff's introduced risk.
• No secrets, no committed credentials, no large binaries. Only source, SQL, and one HTML template touched.
• Refactor of BuildExtractUserMessage: mechanical extraction of previously duplicated inline logic into a shared helper, used identically by all three backends — low risk, well justified for the drift concern.
• Template changes: additive sections (Shared pipeline), gated behind {{if .SharedPasses}} / {{if .TotalCalls}}, so old rows (pre-migration groups) fall through gracefully to prior behavior, matching the described backward-compat story and covered by TestComparePageHidesPipelineWhenNoPasses.
• Test coverage is thorough for the new functionality (split logic, template rendering, failure display, degraded store, column-count guard).

Nothing here looks like an accidental commit, injection vector, auth bypass, or a change likely to take down the service — it's additive schema plus best-effort telemetry capture with tests backing the trickiest bit (column/placeholder alignment).

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.41.7 | Safety Check | Claude Sonnet 5 | tracking id `r-684697-31cde4`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- > ⚠️ The primary review (**Mistral Small**) failed. The review below is from **Claude Sonnet 5**, which reviewed the same diff. **✅ Verdict: LGTM** — Additive migration and best-effort telemetry plumbing, well-tested including a guard against the column/placeholder mismatch risk in the raw SQL; nothing alarming found. Went through the diff carefully given its size. Key things checked: - **Migration**: `000013_review_passes.up/down.sql` is additive (new table + index), with a matching down migration that drops both. No destructive change to existing tables. - **Degraded-store handling**: `RecordPass`/`ListPassesForGroup` both check `s.pool == nil` and no-op/return nil rather than erroring, consistent with the "telemetry must never fail a review" invariant, and covered by `TestPassesDegradedStoreIsInert`. - **Column/placeholder mismatch risk**: this pattern (building an INSERT from a string list of columns plus a hardcoded placeholder count) is exactly the kind of thing that silently breaks at runtime. The PR added `TestPassColsMatchPlaceholders` guarding against drift, which is a reasonable mitigation. I double-checked `passCols` has 16 comma-separated fields and the VALUES clause has $1..$16 — they match. - **`recordPass`/`recordReviewPass`/discovery/preflight pass-recording**: all wrapped as best-effort (log and continue), never propagated as fatal errors — consistent with "telemetry must never fail a review." - **Nil safety**: `t.Event == nil || t.Event.ComparisonGroupID == nil` guarded before dereferencing in `recordPass`. `rr` (configRun) fields used in `recordReviewPass` (`rr.start`, `rr.end`, `rr.promptSystem`, etc.) aren't shown being initialized in this diff, but that's pre-existing code (configRun struct isn't touched here) so out of scope for this diff's introduced risk. - **No secrets, no committed credentials, no large binaries.** Only source, SQL, and one HTML template touched. - **Refactor of `BuildExtractUserMessage`**: mechanical extraction of previously duplicated inline logic into a shared helper, used identically by all three backends — low risk, well justified for the drift concern. - **Template changes**: additive sections (`Shared pipeline`), gated behind `{{if .SharedPasses}}` / `{{if .TotalCalls}}`, so old rows (pre-migration groups) fall through gracefully to prior behavior, matching the described backward-compat story and covered by `TestComparePageHidesPipelineWhenNoPasses`. - Test coverage is thorough for the new functionality (split logic, template rendering, failure display, degraded store, column-count guard). Nothing here looks like an accidental commit, injection vector, auth bypass, or a change likely to take down the service — it's additive schema plus best-effort telemetry capture with tests backing the trickiest bit (column/placeholder alignment).

rcsheets merged commit 1a7e4a7352 into main 2026-07-28 06:07:41 +00:00

rcsheets deleted branch feat/per-pass-telemetry 2026-07-28 06:07:41 +00:00

rcsheets referenced this pull request from a commit 2026-07-28 06:07:43 +00:00

Merge pull request 'feat(dashboard): account for every LLM call on the comparison page' (#92) from feat/per-pass-telemetry into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

brooktrails/pr-reviewer!92

No description provided.