Skip to content

Estimate cost from tokens and rank report findings by waste - #27

Merged
tjdonley merged 4 commits into
mainfrom
feat/estimated-costs-and-top-findings
Aug 12, 2026
Merged

tjdonley merged 4 commits into
mainfrom
feat/estimated-costs-and-top-findings

Conversation

@tjdonley

Copy link
Copy Markdown
Owner

Two gaps that both ended with reports users could not act on.

1. Token-based cost estimation

CostConfidence already declared an "estimated" tier that nothing produced. Normalization only read totalCost/costDetails.total, so an export carrying token counts and model names but no provider cost produced no cost at all — and every analyzer gates its waste math on cost_usd being present (cost/runner.py, retry_loop.py, multi_agent_waste.py). Those traces reported $0.000000000 of waste no matter what they actually spent. The README admitted this about the default fixture.

Spans that arrive without a provider cost are now priced from token counts and model name during normalization, and stored with cost_confidence = estimated. Spans that already carry a provider cost are never touched, so estimation can only add attribution where there was none.

Rates come from a pricing config block layered over a bundled default table:

pricing:
  enabled: true
  use_default_table: true
  models:
    - model: house-fine-tune-v3
      input_per_1m_usd: '0.90'
      output_per_1m_usd: '2.70'

Matching is exact first, then longest version-boundary prefix — gpt-4o-mini prices gpt-4o-mini-2024-07-18, while gpt-4 never prices a gpt-4o span. Provider prefixes (anthropic/claude-sonnet-4-5) match with and without. Configured entries beat bundled defaults.

Estimates are labelled rather than blended into provider cost:

  • normalize reports estimated coverage and names models it had tokens for but no price
  • cost ledger splits provider-reported from estimated cost
  • reports carry a per-row confidence column and a costs_by_confidence_usd summary field
  • agentprof pricing list shows the effective table and where each rate came from

The same export that previously produced an empty ledger:

  estimated cost for 2 span(s) without provider cost: $0.021450000
  │ Cost coverage (source)    │   0.0% │
  │ Cost coverage (estimated) │  50.0% │
No price configured for internal-router-v2.
  Add rates under `pricing.models` in agentprof.yml to attribute their cost.

⚠️ On the bundled table: these are public list prices recorded on a fixed date (DEFAULT_PRICE_TABLE_AS_OF), and provider pricing changes. Worth a look before merge. use_default_table: false prices only from your own entries; enabled: false turns estimation off.

2. Ranking and top findings

fetch_issues() ordered by issue_id, which is f"{kind}:{sha256[:16]}" — so reports listed issues alphabetically by kind, then by hash digest. The demo report leads with the multi-agent finding only because m sorts before r and s; on real data a $12 retry loop would sort below a $0.001 estimate.

Issues are now ordered by attributed waste, then severity, then confidence, then issue_id for stability. The severity/confidence tiebreakers are what rank findings when a trace carries no cost data at all.

Severity was also near-constant (multi_agent_waste hardcoded "medium"). It is now derived from each finding's own magnitude — retry-loop severity escalates with repeated wasted attempts, multi-agent severity tracks the cost multiple over baseline. Spec-violation severity already keyed off whether the span failed, so it is unchanged.

Both renderers now open with the Top Findings section docs/project-pop-spec.md:186-193 asks for: three leading issues, each with recommendation and one evidence line. JSON exposes the same list as top_findings, plus summary.top_issue_kind.

Validation

  • pytest — 168 passed (27 new)
  • New tests/test_pricing.py covers boundary matching, config precedence, provider-cost protection, disabled pricing, idempotent re-normalization, and the CLI surfaces
  • New tests/fixtures/langfuse_token_only_observations.json: a token-only export, including one deliberately unpriced model
  • agentprof demo — totals unchanged at 4 issues / $0.054000000, since the bundled fixture carries provider costs
  • Wheel builds; generated HTML verified well-formed; mypy clean on the new module
  • Checked-in examples/reports/demo.* regenerated for the new section and ledger column

Note on structure

This is one commit rather than two. cli.py and report/runner.py each genuinely contain code for both features, so a file-level split would have produced commits that either failed their own tests or mixed the features anyway. The two features are independent and can be reviewed separately from the sections above.

🤖 Generated with Claude Code

https://claude.ai/code/session_011Yyb1oKkpxm76dz7xGSjLz

Two gaps that both ended with reports users could not act on.

Estimate span cost from token counts
------------------------------------
CostConfidence already declared an "estimated" tier that nothing produced.
Normalization only read totalCost/costDetails.total, so an export carrying
token counts and model names but no provider cost produced no cost at all,
and every analyzer gates its waste math on cost_usd being present. Those
traces reported $0.000000000 of waste no matter what they actually spent.

Spans that arrive without a provider cost are now priced from their token
counts and model name during normalization and stored with
cost_confidence = estimated. Spans that already carry a provider cost are
never touched, so estimation only adds attribution where there was none.

Rates come from a `pricing` config block layered over a bundled default
table. Model matching is exact first, then longest version-boundary prefix,
so gpt-4o-mini prices gpt-4o-mini-2024-07-18 while gpt-4 never prices a
gpt-4o span. Configured entries beat bundled defaults.

Estimated spend is labelled everywhere rather than blended into provider
cost: normalize reports estimated coverage and names models it had tokens
for but no price, the ledger splits provider-reported from estimated cost,
and reports carry a per-row confidence column plus a costs_by_confidence_usd
summary field. `agentprof pricing list` shows the effective table.

Rank findings by attributed waste
---------------------------------
fetch_issues ordered by issue_id, which is "{kind}:{sha256[:16]}", so
reports listed issues alphabetically by kind and then by hash digest. The
demo report led with the multi-agent finding only because m sorts before r
and s; on real data a $12 retry loop would sort below a $0.001 estimate.

Issues are now ordered by attributed waste, then severity, then confidence,
then issue_id for stability. The severity and confidence tiebreakers are
what rank findings when a trace carries no cost data at all.

Severity was also close to constant - multi-agent waste hardcoded "medium".
It is now derived from each finding's own magnitude: retry-loop severity
escalates with repeated wasted attempts, multi-agent severity tracks the
cost multiple over baseline. Spec-violation severity already keyed off
whether the span failed, so it is unchanged.

Markdown and HTML reports now open with the Top Findings section the pop
spec asks for, covering the three leading issues with their recommendation
and one evidence line. The JSON payload exposes the same list as
top_findings alongside summary.top_issue_kind.

Demo totals are unchanged: the bundled fixture carries provider costs, so
estimation does not touch it. Checked-in example reports are regenerated
for the new section and ledger column.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Yyb1oKkpxm76dz7xGSjLz

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2057ee781

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

raw_rows = store.fetch_raw_spans(source=source)
spans = [_map_raw_span(row) for row in raw_rows if _is_supported_source(row.source)]
# Estimate before the trace rollup so trace totals include estimated spans.
spans, estimation = apply_estimated_costs(spans, build_pricing_table(pricing))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve source-priced ancestors when estimating descendants

When a provider-costed parent or root contains an uncosted token-bearing child, this estimates the child; the existing trace and ledger leaf-selection logic then treats the source-priced parent as an ancestor with a costed descendant and excludes it. Consequently, the authoritative provider amount is effectively replaced by the child estimate in trace totals, ledgers, and analyzer results, despite the promise that provider costs are never displaced. Estimation or leaf selection needs to account for source-priced ancestors.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 50b8d24. Reproduced before touching anything — a $1.00 provider-costed root with one uncosted 1500-token child collapsed to $0.00045 in trace totals, the ledger, and every analyzer:

trace total BEFORE estimation: 1.00
trace total AFTER  estimation: 0.000450000

Root cause is exactly as described: leaf selection drops a costed ancestor once a descendant carries cost, so the estimate did not sit alongside the provider amount, it replaced it.

Fixed at the estimation side rather than in leaf selection, since leaf selection is shared by three analyzers and correctly implements "count leaves only" — the wrong input was the estimate, not the selection. _provider_costed_branch_keys now walks both up to ancestors and down to descendants from every provider-costed span, and apply_estimated_costs skips anything on those paths. The descendant direction matters too: estimating a parent above a costed child was pointless (leaf selection excluded it anyway) but still inflated the reported estimated-span count.

Uncosted sibling branches are still priced, so no coverage is lost where provider spend is genuinely absent. Blocked spans are also dropped from the unpriced-model hint, which was otherwise inviting users to configure rates that would never be applied.

Four regression tests added: ancestor case, descendant case, sibling-branch-still-priced, and the unpriced-hint case.

Comment thread agentprof/report/runner.py Outdated
for record in costs:
if record.amount_usd is not None:
cost_types[record.cost_type] += record.amount_usd
cost_confidences[record.confidence] += record.amount_usd

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Separate token estimates from analyzer confidence totals

When analyzers have run, this sums every ledger attribution rather than only normalized span spend, so retry/spec records count source costs again and analytical estimates such as multi-agent overhead are reported as estimated spend. For example, the checked-in demo's provider-backed span ledger totals $0.096, but costs_by_confidence_usd reports $0.114 as source plus $0.042 as estimated. Consumers therefore cannot use this new field to distinguish token-estimated spend from provider-reported spend as documented.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and the numbers check out exactly. Broke the demo ledger down by attribution method before fixing:

("normalized_span_status", "source")   0.096000000
("retry_loop",             "source")   0.006000000
("spec_violation",         "source")   0.012000000
("multi_agent_waste",   "estimated")   0.042000000

So the published $0.114 source was $0.096 of real spend plus $0.018 of analyzer re-attribution counted a second time, and the $0.042 estimated was multi-agent orchestration overhead — an analytical estimate — masquerading as token-estimated spend. The field could not do the one job it was added for.

Fixed in 50b8d24 by restricting it to normalized_span_status, the only attribution method that measures spend rather than re-describing spend already measured, and renaming it to span_costs_by_confidence_usd so the scope is legible without reading the source. The demo now reports {"source": "0.096000000"}, matching its span ledger exactly.

Left costs_by_type_usd alone: it sums across methods too, but cost-type values do not collide between methods, so it is not double counting the way this was. Happy to revisit separately if you disagree.

README updated to state the scope and why analyzer attributions are excluded.

Comment thread agentprof/cli.py
table.add_column("Input", justify="right")
table.add_column("Output", justify="right")
table.add_column("Source")
for price in sorted(table_prices, key=lambda item: (item.source, item.model)):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove shadowed defaults from the effective price list

When a user overrides a bundled model such as gpt-4o-mini, PricingTable.prices retains both the configured and default entries, and this loop prints both conflicting rates. Lookup actually uses the configured entry, so agentprof pricing list does not show the promised effective table and leaves users unable to tell which displayed rate is applied; filter entries shadowed by configured models before rendering.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — fixed in 50b8d24. PricingTable.prices deliberately keeps configured and default entries together so lookup can resolve by specificity, but that is an internal ordering concern and should not have been what the command rendered.

Added PricingTable.effective_prices(), which drops default entries shadowed by an exact configured model name, and pointed pricing list at it. Shadowing is exact-name only, so configuring gpt-4o-mini correctly still leaves the bundled gpt-4o row visible — it is a genuinely different model that lookup can still return.

Two tests added: one asserting a single row with source = config survives for an overridden model while gpt-4o remains, and a CLI test asserting the shadowed bundled rate no longer appears next to the override.

Addresses Codex review on #27.

Never estimate inside a provider-costed branch (P1)
---------------------------------------------------
Cost attribution counts only leaf costs, dropping a costed ancestor as soon
as a descendant carries cost. Estimating a token-bearing child underneath a
provider-costed parent therefore swapped the authoritative amount for the
estimate: a $1.00 provider-reported trace with one uncosted 1500-token child
collapsed to $0.00045 in trace totals, the ledger, and every analyzer.

Estimation now skips any span that shares a trace path with a provider-costed
span, walking both up to ancestors and down to descendants. Uncosted sibling
branches are still priced, so the fix costs no coverage where provider spend
is absent. Blocked spans are also excluded from the unpriced-model hint,
which was inviting users to configure rates that would never be used.

Scope the confidence breakdown to measured spend (P2)
-----------------------------------------------------
costs_by_confidence_usd summed every ledger row, so analyzer attributions
that re-point at spend the span ledger already measured were counted a second
time as provider-reported, and multi-agent overhead - an analytical estimate,
not a token estimate - was reported as estimated spend. The demo's $0.096 of
provider-backed span spend was published as $0.114 source plus $0.042
estimated.

The field is now restricted to the normalized span ledger, the only
attribution method that measures spend rather than re-describing it, and
renamed to span_costs_by_confidence_usd to say so. The demo now reports
$0.096000000 source and nothing estimated, matching its span ledger exactly.

Show only effective rates in pricing list (P2)
----------------------------------------------
PricingTable.prices keeps configured and default entries together so lookup
can resolve by specificity, so overriding a bundled model printed both the
override and the shadowed default with no indication which one applied.
Added PricingTable.effective_prices(), which drops defaults shadowed by an
exact configured name, and pointed the command at it. Shadowing stays
exact-name only, so configuring gpt-4o-mini does not hide gpt-4o.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Yyb1oKkpxm76dz7xGSjLz
@tjdonley

Copy link
Copy Markdown
Owner Author

All three findings were real and are fixed in 50b8d24 — replies inline on each thread.

The P1 was the important one: I reproduced it before changing anything, and a $1.00 provider-costed trace with a single uncosted token-bearing child was collapsing to $0.00045. That directly contradicted this PR's central claim that provider cost is never displaced, and it would have silently destroyed cost data on a common trace shape rather than merely failing to add any. Good catch.

Summary of changes:

  • Estimation now skips any span sharing a trace path with a provider-costed span, walking both ancestors and descendants. Uncosted sibling branches are still priced, so no coverage is lost.
  • costs_by_confidence_usd is scoped to the normalized span ledger and renamed span_costs_by_confidence_usd. The demo went from a published $0.114 source / $0.042 estimated to $0.096 source, matching its actual span ledger.
  • pricing list renders effective_prices(), so an overridden model shows one row instead of the override plus its shadowed default.

Test suite is at 174 passing (6 new regression tests). Demo totals remain unchanged at 4 issues / $0.054000000, and the checked-in example reports are regenerated.

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 50b8d245d8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread agentprof/cost/pricing.py Outdated
if not key.startswith(model):
return False
remainder = key[len(model) :]
return not remainder or remainder[0] in BOUNDARY_CHARS

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Distinguish model variants from version suffixes

When a custom-only table configures a base model such as gpt-4o, a span named gpt-4o-mini passes this check because - is treated as a version boundary, so the mini model is silently priced at the much higher base-model rate. The same ambiguity affects names such as gpt-4.1-mini and claude-sonnet-4-5; prefix matching should recognize actual version suffixes or require explicit aliases rather than accepting every delimiter-prefixed suffix.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real, and worse than the description suggests — fixed in 91d7501. Reproduced against a table configuring only gpt-4o:

gpt-4o-mini resolves to: gpt-4o | rate (2.50, 10.00)
  correct mini rate would be: (0.15, 0.60)

That is a 16.7x overstatement on input, and it is the same class of bug as the boundary rule I added one level down: boundary matching stops gpt-4 from pricing gpt-4o because there is no separator, but -mini looks exactly like a version suffix to that rule.

Fix is to require the suffix to actually look like a version — a digit, or v followed by a digit. Date and release suffixes still resolve (gpt-4o-mini-2024-07-18, claude-opus-4-1-20250805, gemini-2.0-flash-001, house-model-v2); named variants (-mini, -nano, -turbo, -lite) do not. When a variant does not match it falls through to the unpriced-model hint, so the failure mode is "tell the user to configure a rate" rather than "invent a wrong one" — which is the right direction for a tool whose output is a dollar figure.

Worth flagging that this also corrected a wrong expectation in my original tests: I had asserted gpt-4-turbo should inherit the gpt-4 rate. It should not — different model, different pricing — so that test now asserts the opposite, with gpt-4-0613 covering the version-suffix case it was meant to exercise.

Three tests added: named variants rejected, five real dated/versioned names still resolving to the right base, and the v-prefix form.

Comment thread agentprof/cost/pricing.py Outdated
Comment on lines +164 to +166
input_tokens = span.input_tokens or 0
output_tokens = span.output_tokens or 0
if input_tokens <= 0 and output_tokens <= 0:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject negative token counts before pricing

When an imported span has one negative token count and one positive count, this condition allows estimation to proceed and the negative value is subtracted from the other component. Because the Langfuse mapper and NormalizedSpan do not constrain these integers, malformed provider data can therefore create understated or even negative ledger costs and distort report rankings; skip or reject any span containing a negative token count before calculating its price.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 91d7501. The guard was input <= 0 and output <= 0, so a single negative component sailed through:

estimate for (-100000 in, 10 out) -> cost_usd = -0.014994000

A negative amount would land in the cost ledger, in trace totals, and — thanks to the ranking change in this same PR — in the issue ordering, where it would sort a malformed span to the bottom while quietly reducing the totals above it.

_has_usable_tokens now rejects a span if either component is negative, and requires at least one to be positive. Applied in both estimate_span_cost and the unpriced-model hint, so the two stay consistent: a negative count is an unusable usage record, not a missing rate, and pointing the user at pricing.models for it would be misleading.

Deliberately left the sign unconstrained on NormalizedSpan itself rather than validating at the schema — normalization should keep faithfully recording what the provider actually sent, including bad data, and pricing is the layer that decides what it can safely act on. Happy to tighten the mapper instead if you would rather reject it at the boundary.

Two tests added: four negative-count shapes rejected, and one asserting such spans are not reported as unpriced.

Addresses the second Codex review round on #27.

Named variants no longer inherit a base model rate
--------------------------------------------------
Boundary matching stopped `gpt-4` pricing a `gpt-4o` span, but not `gpt-4o`
pricing a `gpt-4o-mini` span, because `-mini` starts with a delimiter like
any version suffix. Against a table configuring only `gpt-4o`, a
`gpt-4o-mini` span resolved to $2.50/$10.00 per 1M instead of $0.15/$0.60,
overstating its input cost by more than sixteen times.

A suffix now has to look like a version - a digit, or `v` followed by a
digit - so date and release suffixes still resolve (`gpt-4o-mini-2024-07-18`,
`claude-opus-4-1-20250805`, `gemini-2.0-flash-001`) while named variants such
as `-mini`, `-nano`, and `-turbo` do not. Unmatched models fall through to
the unpriced-model hint, so a missing rate surfaces as a prompt to configure
one rather than as a confidently wrong number.

This also corrects a wrong expectation in the original tests, which asserted
that `gpt-4-turbo` should inherit the `gpt-4` rate. It is a separate model
priced differently and now stays unpriced until configured.

Negative token counts are unusable, not cheap
---------------------------------------------
Nothing constrains the sign of imported token counts, and the usable-token
check only rejected spans where both counts were non-positive. A span with
-100000 input and 10 output tokens priced out at -$0.014994, putting a
negative amount into the ledger, into trace totals, and now into the issue
ranking this PR introduced.

Spans carrying a negative count on either component are left unpriced, and
excluded from the unpriced-model hint since the problem is the usage record
rather than a missing rate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Yyb1oKkpxm76dz7xGSjLz
@tjdonley

Copy link
Copy Markdown
Owner Author

Both round-two findings were real and are fixed in 91d7501 — replies inline.

The model-variant one was the more serious: against a table configuring only gpt-4o, a gpt-4o-mini span was resolving to the base rate and overstating its input cost by 16.7x. Same class as the boundary rule already in this PR, one level down — -mini is indistinguishable from a version suffix if the rule only checks for a delimiter. Suffixes now have to look like versions (a digit, or v plus a digit), so dated releases still resolve while named variants fall through to the unpriced-model hint.

The negative-token one produced a genuinely negative ledger amount (-$0.014994 for a span with -100000 input tokens), which would have propagated into trace totals and into the issue ranking this PR adds.

That review round also caught a wrong assertion in my own tests — I had asserted gpt-4-turbo should inherit the gpt-4 rate, which is incorrect since they are separate models with different pricing. Now inverted, with gpt-4-0613 covering the version-suffix behaviour it was meant to test.

Suite is at 181 passing (7 new). Demo totals unchanged at 4 issues / $0.054000000.

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 91d7501fa3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread agentprof/cost/pricing.py Outdated
Comment on lines +353 to +354
if suffix[0].isdigit():
return True

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject numeric model variants as release suffixes

Fresh evidence after the prior variant fix: when a custom-only table contains gpt-4, _is_version_suffix still accepts the .1 remainder in gpt-4.1, and a configured claude-sonnet-4 similarly accepts claude-sonnet-4-5. These are separately priced model variants, so their spans silently inherit the base rate instead of appearing in the unpriced-model hint; restrict fallback matching to recognizable release/date suffix shapes rather than accepting every digit-leading remainder.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right again, and my previous fix only moved the line rather than drawing it correctly. Reproduced against a table configuring gpt-4 and claude-sonnet-4:

gpt-4.1              -> gpt-4
gpt-4.1-mini         -> gpt-4
claude-sonnet-4-5    -> claude-sonnet-4

gpt-4 at $30/$60 pricing a gpt-4.1 span whose real rate is $2.00/$8.00 is a fifteenfold overstatement — worse than the gpt-4o-mini case from the last round.

Fixed in fb842e5 by requiring the suffix to be a date or build number rather than merely digit-leading: \d{4}-\d{2}-\d{2} or three-or-more digits. The distinction that actually matters is date-or-build stamp (same model, same price) versus release component (new model, priced on its own), and component length separates them cleanly.

Still resolving without configuration:

gpt-4o-mini-2024-07-18   -> gpt-4o-mini
claude-3-5-sonnet-20241022 -> claude-3-5-sonnet
claude-opus-4-1-20250805 -> claude-opus-4-1
gemini-2.0-flash-001     -> gemini-2.0-flash
gpt-4-0613               -> gpt-4

Now rejected: gpt-4.1, gpt-4.1-mini, claude-sonnet-4-5, gpt-4o-mini from gpt-4o, and every named variant.

This also drops the v2 form I accepted last round — same ambiguity, since a v bump is a new release rather than a build of the existing one, so that test is inverted too. Note the exact-match layer means the bundled table is unaffected: claude-sonnet-4-5 and gpt-4.1 are exact entries, so this only changes behaviour for partial custom tables, which is precisely where a wrong rate would have gone unnoticed.

Comment thread agentprof/cost/pricing.py Outdated
Comment on lines +219 to +220
estimated_spans += 1
estimated_cost += estimate.cost_usd

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Report only leaf-attributed estimated cost

When both an ancestor and its descendant have usable token counts, this loop adds both estimates to estimated_cost_usd, while build_normalized_traces and build_cost_ledger discard the ancestor because it has a costed descendant. Consequently agentprof normalize reports a larger estimated amount than the trace and ledger totals for the same import; calculate the returned amount using the same leaf-selection rule as the persisted cost rollups.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed — fixed in fb842e5. Two nested token-bearing spans with no provider cost anywhere:

reported estimated_cost_usd: 0.001350000
trace total (leaf-selected): 0.000900000

normalize was announcing spend that the ledger would never show. The provider-cost guard from the previous round does not cover this because neither span carries provider cost, so both are legitimately estimated — the divergence is purely in what gets reported.

The returned total now applies the same leaf-only rule as the persisted rollups, via a _cost_leaf_keys pass over the updated spans. Kept the per-span estimates in normalized_spans rather than suppressing the ancestor: normalization should record what each span is worth, and leaf selection is the rollup layer's job. Only the reported figure is leaf-attributed.

Two tests: one asserting the reported total equals the trace total, one asserting it equals build_cost_ledger's total for the same nested pair, so the three cannot drift apart again.

Worth noting this is now the fourth implementation of the leaf-selection walk in the codebase — cost/runner.py, retry_loop.py, multi_agent_waste.py, and now here. They agree today, but that is exactly the kind of duplication that drifts. I have left it alone to keep this PR scoped; happy to follow up with a shared helper if you want it.

Addresses the third Codex review round on #27.

Numeric releases no longer inherit a base rate
----------------------------------------------
Requiring a digit-leading suffix stopped `-mini` and `-turbo`, but still
accepted the short numeric components that mark a separate release. Against
a table configuring `gpt-4`, both `gpt-4.1` and `gpt-4.1-mini` resolved to
the `gpt-4` rate - $30/$60 per 1M against gpt-4.1's $2.00/$8.00, a fifteenfold
overstatement - and a configured `claude-sonnet-4` absorbed `claude-sonnet-4-5`.

A suffix now has to be a date or a build number: `\d{4}-\d{2}-\d{2}` or three
or more digits. Dated releases still resolve without configuration
(`gpt-4o-mini-2024-07-18`, `claude-3-5-sonnet-20241022`,
`claude-opus-4-1-20250805`, `gemini-2.0-flash-001`, `gpt-4-0613`), while
short components such as `.1` and `-5` do not. This drops the `v2` form
accepted in the previous round, which had the same ambiguity: a `v` bump is
a new release, not a build of the existing one.

Report the estimate the ledger will carry
-----------------------------------------
When an estimated span sat above another estimated span, both amounts were
added to the reported total while trace and ledger rollups keep leaf costs
only, so `agentprof normalize` announced more estimated spend than the
import actually produced - $0.001350000 against a $0.000900000 ledger.

The returned total now applies the same leaf-only rule as the persisted
rollups. Spans still each carry their own estimate in normalized_spans; only
the reported total is leaf-attributed, and a test asserts it equals both the
trace total and the ledger total for a nested pair.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Yyb1oKkpxm76dz7xGSjLz
@tjdonley

Copy link
Copy Markdown
Owner Author

Both round-three findings were real and are fixed in fb842e5 — replies inline.

The prefix-matching one is the third iteration on the same boundary, and each round found a real gap the previous rule left open: no-separator (gpt-4 / gpt-4o), then named variants (gpt-4o / gpt-4o-mini), now short numeric releases (gpt-4 / gpt-4.1, a fifteenfold overstatement). The rule is now positive rather than exclusionary — a suffix must be a date or build number (\d{4}-\d{2}-\d{2} or three-plus digits) rather than merely not look like a variant. Dated releases still resolve without configuration; everything else falls through to the unpriced hint.

The reporting one had normalize announcing $0.001350000 against a $0.000900000 ledger for nested estimated spans. The reported total is now leaf-attributed, with tests pinning it to both the trace total and the ledger total.

Suite is at 183 passing. Demo totals unchanged at 4 issues / $0.054000000.

One note carried over from the inline reply: this makes four copies of the leaf-selection walk in the codebase. They agree today, but a shared helper would be worth a follow-up.

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: fb842e583e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@tjdonley
tjdonley merged commit 3ce865e into main Aug 12, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant