Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1
Fix Codex request-level turn accounting (#504)#512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
willwashburn
wants to merge
10
commits into
mainChoose a base branch
from
issue-504-codex-turn-accounting
base:main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+534
−69
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
8dd7fd3
fix: count Codex model requests per turn
willwashburn 44f11fd
fix: align Codex inference request cardinality
willwashburn af89225
fix: preserve Codex inference semantics
willwashburn 56b0245
fix: tighten request-count edge cases
willwashburn 442ebaa
docs: clarify compare request cardinality
willwashburn 8db778c
docs(compare): clarify logical sample flags
willwashburn da4e863
Merge remote-tracking branch 'origin/main' into issue-504-codex-turn-…
willwashburn 5bcb8d8
test: thread request_count through turn fixtures added on main
willwashburn 602dfd2
Merge remote-tracking branch 'origin/main' into issue-504-codex-turn-…
willwashburn b702957
test: thread request_count through fixtures from merged main
willwashburn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -40,13 +40,15 @@ pub struct CompareCell { | ||
| pub one_shot_rate: Option<f64>, | ||
| pub cache_hit_rate: Option<f64>, | ||
| pub median_retries: Option<f64>, | ||
| /// True when the cell has zero turns. Distinct from `insufficient_sample` | ||
| /// so JSON consumers can tell "we never saw this combination" apart | ||
| /// from "we have data but the sample is small." Only one of `no_data` / | ||
| /// `insufficient_sample` is ever true at a time. | ||
| /// True when the cell has zero matching logical turn records. Distinct | ||
| /// from `insufficient_sample` so JSON consumers can tell "we never saw | ||
| /// this combination" apart from "we have data but the sample is small." | ||
| /// Only one of `no_data` / `insufficient_sample` is ever true at a time. | ||
| pub no_data: bool, | ||
| /// True when `0 < turns < min_sample`. A cell with `no_data == true` | ||
| /// always has `insufficient_sample == false`. | ||
| /// True when `0 < logical turn records < min_sample`. This sample gate is | ||
| /// independent of `turns`, which reports model requests and may be zero | ||
| /// for an observed Codex record. A cell with `no_data == true` always has | ||
| /// `insufficient_sample == false`. | ||
| pub insufficient_sample: bool, | ||
| } | ||
| @@ -89,6 +91,7 @@ impl<'a> CompareOptions<'a> { | ||
| #[derive(Debug, Default)] | ||
| struct Accum { | ||
| turns: u64, | ||
| logical_turns: u64, | ||
| edit_turns: u64, | ||
| one_shot_turns: u64, | ||
| priced_turns: u64, | ||
| @@ -151,11 +154,13 @@ pub fn build_compare_table(turns: &[EnrichedTurn], opts: &CompareOptions<'_>) -> | ||
| .expect("model just inserted"); | ||
| let acc = by_cat.entry(cat).or_default(); | ||
| acc.turns += 1; | ||
| let request_count = t.effective_request_count(); | ||
| acc.turns += request_count; | ||
| acc.logical_turns += 1; | ||
| let mt = model_totals.get_mut(model).expect("model just inserted"); | ||
| mt.turns += 1; | ||
| mt.turns += request_count; | ||
| if let Some(c) = cost_for_turn(t, opts.pricing) { | ||
| acc.priced_turns += 1; | ||
| acc.priced_turns += request_count; | ||
| acc.total_cost += c.total; | ||
| mt.total_cost += c.total; | ||
| } | ||
| @@ -239,7 +244,7 @@ fn to_cell(acc: Option<&Accum>, min_sample: u64) -> CompareCell { | ||
| let Some(acc) = acc else { | ||
| return empty_cell(); | ||
| }; | ||
| if acc.turns == 0 { | ||
| if acc.logical_turns == 0 { | ||
willwashburn marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| return empty_cell(); | ||
| } | ||
| CompareCell { | ||
| @@ -272,7 +277,7 @@ fn to_cell(acc: Option<&Accum>, min_sample: u64) -> CompareCell { | ||
| None | ||
| }, | ||
| no_data: false, | ||
| insufficient_sample: acc.turns < min_sample, | ||
| insufficient_sample: acc.logical_turns < min_sample, | ||
| } | ||
| } | ||
| @@ -340,6 +345,7 @@ mod tests { | ||
| session_path: None, | ||
| message_id: id, | ||
| turn_index: 0, | ||
| request_count: 1, | ||
| ts: "2026-04-20T00:00:00.000Z".into(), | ||
| model: model.into(), | ||
| project: None, | ||
| @@ -638,6 +644,51 @@ mod tests { | ||
| assert!((cpt - cell.total_cost / 2.0).abs() < 1e-9); | ||
| } | ||
| #[test] | ||
| fn uses_request_count_as_codex_cost_per_turn_denominator() { | ||
| let pricing = load_builtin_pricing(); | ||
| let mut turns = vec![turn( | ||
| "claude-sonnet-4-6", | ||
| Some(ActivityCategory::Coding), | ||
| default_usage(), | ||
| Some(false), | ||
| None, | ||
| )]; | ||
| turns[0].turn.source = SourceKind::Codex; | ||
| turns[0].turn.request_count = 7; | ||
| let t = build_compare_table(&turns, &CompareOptions::new(&pricing)); | ||
| let cell = &t.cells["claude-sonnet-4-6"]["coding"]; | ||
| assert_eq!(cell.turns, 7); | ||
| assert_eq!(cell.priced_turns, 7); | ||
| assert_eq!(cell.cost_per_turn, Some(cell.total_cost / 7.0)); | ||
| assert!( | ||
| cell.insufficient_sample, | ||
| "request cardinality must not satisfy the logical sample-size gate" | ||
| ); | ||
| } | ||
| #[test] | ||
| fn zero_request_logical_turn_is_insufficient_not_no_data() { | ||
| let pricing = load_builtin_pricing(); | ||
| let mut turns = vec![turn( | ||
| "claude-sonnet-4-6", | ||
| Some(ActivityCategory::Coding), | ||
| Usage::default(), | ||
| Some(false), | ||
| None, | ||
| )]; | ||
| turns[0].turn.source = SourceKind::Codex; | ||
| turns[0].turn.request_count = 0; | ||
| let table = build_compare_table(&turns, &CompareOptions::new(&pricing)); | ||
| let cell = &table.cells["claude-sonnet-4-6"]["coding"]; | ||
| assert_eq!(cell.turns, 0); | ||
| assert!(!cell.no_data); | ||
| assert!(cell.insufficient_sample); | ||
| assert_eq!(cell.cost_per_turn, None); | ||
| } | ||
| #[test] | ||
| fn groups_unclassified_turns_under_unclassified() { | ||
| let pricing = load_builtin_pricing(); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -258,7 +258,7 @@ pub(crate) fn aggregate_by_provider( | ||
| let row = by_provider | ||
| .entry(provider.clone()) | ||
| .or_insert_with(|| empty_provider_row(&provider)); | ||
| row.turns += 1; | ||
| row.turns += t.effective_request_count(); | ||
cubic-dev-ai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| row.usage.input += t.usage.input; | ||
| row.usage.output += t.usage.output; | ||
| row.usage.reasoning += t.usage.reasoning; | ||
| @@ -385,6 +385,7 @@ mod tests { | ||
| session_path: None, | ||
| message_id: "m-provider".into(), | ||
| turn_index: 0, | ||
| request_count: 1, | ||
| ts: "2026-04-20T00:00:00.000Z".into(), | ||
| model: model.into(), | ||
| project: None, | ||
| @@ -498,17 +499,13 @@ mod tests { | ||
| #[test] | ||
| fn aggregate_falls_through_to_collector_for_non_synthetic_turns() { | ||
| let pricing = pricing_fixture(); | ||
| let rows = aggregate_by_provider( | ||
| &[turn( | ||
| "gpt-5", | ||
| SourceKind::Codex, | ||
| one_million_in_one_million_out(), | ||
| )], | ||
| AggregateByProviderOptions::new(&pricing), | ||
| ); | ||
| let mut codex = turn("gpt-5", SourceKind::Codex, one_million_in_one_million_out()); | ||
| codex.request_count = 7; | ||
| let rows = aggregate_by_provider(&[codex], AggregateByProviderOptions::new(&pricing)); | ||
| assert_eq!(rows.len(), 1); | ||
| assert_eq!(rows[0].provider, "openai"); | ||
| assert_eq!(rows[0].turns, 7); | ||
| assert_eq!(rows[0].cost.total, 9.0); | ||
| } | ||
| @@ -614,6 +611,7 @@ mod cost_lookup_via_reattribution_tests { | ||
| session_path: None, | ||
| message_id: "m".into(), | ||
| turn_index: 0, | ||
| request_count: 1, | ||
| ts: "2026-04-20T00:00:00.000Z".into(), | ||
| model: model.into(), | ||
| project: None, | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.