fix(mobile): one canonical session cost across list and detail - #4777
Conversation
…meState Add the persisted per-session cost column to the getWithRuntimeState output schema and return projection, thread it through FetchedSessionData as an optional field, and populate it in the mobile fetchSession mapping. The mobile session detail screen will use it to render the same canonical cost the list reads from the Postgres column. Web fetchSession behavior is unchanged (the new field is optional).
The list rendered the persisted column at 2 decimals with a </bin/zsh.01 floor while the detail rendered a client-side message sum at 4 decimals, plus a hand-rolled toFixed(4) duplicate in the header fallback, and spoken labels that dropped sub-cent costs entirely. The same session read as up to four different costs, and an empty-snapshot session showed its cost in the list but nothing on the detail screen. Derive every surface from one canonical microdollar total: the detail combines the persisted column and the live message sum via selectSessionCostInputs (max of two lower bounds, cost being monotonic), all surfaces render through formatSessionTotalCost, and all spoken labels through formatSpokenCost on the same threshold. The context sheet splits its total (combined microdollars) from its breakdown input (live USD sum) so no phantom subagent residual appears, and omits the Total cost row instead of rendering $0.0000 when no cost has been reported.
The best-effort total_cost_microdollars write sat after the unguarded O11Y ingestSessionMetrics call, so an analytics binding failure skipped the cost persist entirely and the column stayed NULL. Reorder so the cost write runs first; the O11Y call stays unguarded and the metricsEmitted dedup is unchanged, preserving the existing alarm retry semantics.
|
(bot) Standin code review of head Scope reviewed: complete Checks run by the reviewer (all clean): mobile vitest 226 files / 1876 tests pass; mobile typecheck, lint, check:unused, format:check; web typecheck; web Verified correct by inspection: omit band (1–49 µ$ → null, confirmed by the passing boundary vectors), the shared Finding 1 — minor — missing regression coverage for the DO reorder guarantee
Verdict: 1 actionable finding (minor). Triage: valid — a regression test pinning the persist-before-O11Y ordering will be added; the result will be recorded in a follow-up comment here. |
Regression coverage for the guarantee that the Postgres total_cost_microdollars persist completes before the unguarded O11Y.ingestSessionMetrics RPC: drives alarm() with O11Y rejecting and asserts the update chain (connection, value, session/user filter) was awaited first. Marker records at await time via a thenable, so a build-now-await-later refactor also fails.
|
(bot) Triage outcome for the standin review above. Finding 1 (minor, missing regression coverage for the DO reorder guarantee) was valid and is now repaired on head
|
|
(bot) Standin review round 2 on the final head Scope: full diff, 14 files / 5 commits — mobile components/helpers/tests, web router + SDK, session-ingest DO + the new ordering test. Surrounding code verified: DB column ( Checks run by the reviewer (all clean): mobile typecheck, lint, check:unused, format:check, full vitest (226 files / 1876 tests); session-ingest typecheck, lint, vitest (24 files / 596 tests including the new O11Y-ordering pin); web typecheck (with Result: no findings. Verdict: no actionable findings on the latest head. Residual testing risks noted by the reviewer (informational, not findings): (1) list-vs-detail convergence with a non-zero persisted cost is covered at unit level and by the on-device E2E run reported in the PR description, not by Maestro automation; (2) the new ordering test relies on a hand-rolled drizzle mock — it pins ordering and where-binding but would not catch a real drizzle SQL semantics change; (3) sub-half-cent visible/spoken rounding can diverge by ≤0.01 cent at exact decimal boundaries — deterministic, cosmetic, and consistent with the documented rounding rule. This completes the review loop: a fresh standin reviewer reports no valid actionable findings on the final head. The PR is ready for human review. |
|
(bot) Final state on head
Review loop is closed: two standin reviews found and fixed one valid minor finding (DO persist-before-O11Y regression coverage, hardened through two nit rounds), and the final fresh standin review is clean. Ready for human review — per workflow this PR is never self-approved or merged by the bot. |
2 participants
Problem
The cost shown for a session in the Agents list differed from the cost on the session
detail screen. Example (measured against API + DB and confirmed on device):
$0.01$0.0131<$0.01$0.0031The numbers were identical to the microdollar — the defect was display/derivation
divergence, not bad data. One quantity had two independent derivations (persisted DB
column vs live client sum over loaded messages) and four independent renderings (list
visible, list spoken, detail header, context sheet):
cli_sessions_v2.total_cost_microdollarsand formatted at 2 decimalswith a
<$0.01floor that hid real sub-cent values.second hand-rolled
toFixed(4)copy in the header fallback. It under-reported onpaginated sessions and showed no cost at all when the DO snapshot came back empty
(list
$0.07, detail blank), or a false$0.0000in the context sheet when loadedassistant cost was 0.
screen displayed.
Fix
One canonical value in integer microdollars, one visible formatter, one spoken
humanizer, one selector:
getWithRuntimeStateexposestotal_cost_microdollars(output schema andreturn projection), threaded through
FetchedSessionData(optional, web unchanged) tomobile
fetchSession. No new query — the detail screen already fetches this object.formatSessionTotalCost(µ$)is the only session-total formatter:>= $0.005renders 2 decimals; below renders the real 4-decimal figure instead of
<$0.01;values that would render a false
$0.0000(1–49 µ$) are omitted. Used by the listrow, detail header pill, header fallback (both inline
toFixed(4)copies deleted),and the context-sheet total.
formatSpokenCost(µ$)is the single humanizer for list and detail a11y, branchingon the same 5000 µ$ threshold: whole-cent phrasing above ("1 cent"), fractional cents
below ("0.31 cents"), and null exactly when the visible value is null — never "no
cost" for a session that has one.
selectSessionCostInputs(persisted, liveUsd)derives the detail total asmax(persisted µ$, live µ$). Session cost is monotonically non-decreasing, so bothare lower bounds and the max is strictly closer to truth. This kills all three detail
failure modes: empty snapshot (persisted wins), pagination (persisted wins), and new
turns while on screen (live wins). The context sheet receives the combined total for
its "Total cost" row but keeps the live USD sum for its per-model breakdown, so no
phantom "Subagents" residual appears on paginated sessions. When no cost exists, the
sheet omits the "Total cost" row entirely rather than showing
$0.0000.SessionIngestDOpersists the cost column before the O11Y metrics RPC. Thebest-effort Postgres write previously sat after the unguarded analytics call, so an
O11Y failure skipped it and the column stayed NULL (demonstrated locally with
cloudflare-o11ydown:Worker "o11y" not found, no persisted cost). The O11Y callstays unguarded and the
metricsEmitteddedup is unchanged.formatCost(4-decimal USD) remains only for attribution detail: per-model rows, thesubagent-residual row, and per-message rows.
Test plan
50/3081/4999/5000/9999/10000/13113/1234567 µ$), spoken vectors, the selector matrix
including the composition gate (
(500_000, 0.001)→ total500_000, breakdown0.001), and rewritten header/sheet/a11y expectations (microdollar inputs, humanizedspoken fragments,
cost: nullinstead of'$0.0000').pnpm format && pnpm typecheck && pnpm lint && pnpm check:unused && pnpm testclean inapps/mobile (1876 tests); apps/web typecheck + router/SDK jest suites (237 tests) +
lint; services/session-ingest typecheck + tests (595) + lint.
(≥ 1¢ and sub-cent), persisted cost proven non-null in Postgres first, list refetched,
then list row / detail header / context-sheet total compared for the identical string,
and spoken labels read from the accessibility tree.
Out of scope / follow-ups
/exitappears to change the session owner. Notreproducible across a 4-path on-device matrix (CLI 7.4.13 and 7.4.16 × typed
/exitand mobile-initiated exit);
kilo_user_id/organization_idunchanged in every path,typed
/exitissues no HTTP request, no writer ofkilo_user_idexists in the repo,and the composite PK
(session_id, kilo_user_id)makes owner-overwrite-by-upsertstructurally impossible. Nothing in this PR addresses it. Two real follow-ups found
along the way, reported here but deliberately not fixed in this PR:
UNKNOWNrows(separate empty sessions, not a re-owned session) — plausibly what the reporter
saw. Deserves its own investigation.
cli_sessions_v2.organization_idverbatim from the client-supplied
kilo_meta.orgIdwith no membership check(
services/session-ingest/src/ingest/metadata.ts), while the tRPC path checks viaensureOrganizationAccess. Can re-tenant a session away from its owner. Needs itsown change and security review.
CloudChatPagehand-rollstoFixed(4), the sidebar reads the persisted column). Shared-SDK changes here areadditive; aligning the web UI is a separate change.
legitimately differ for a few seconds; they converge after the turn closes. If the
user stays on the detail screen across new turns with older pages unloaded, both
selector inputs can be low at once — still strictly better than today, which showed
the partial live sum alone.