feat(token-usage): attribute subagent usage to the subagent's own session - #2241
Conversation
83bd64f to
bfce097
Compare
| Self { | ||
| session_id, | ||
| external_session_id, | ||
| session_id: stream.session_id.clone(), |
There was a problem hiding this comment.
[code-review] Leaf attribution silently drops subagent cost from git-ai analyze
enrich_token_usage (src/commands/analyze/sessions.rs:941) joins the token-usage cube on exact session_id (dimensions: vec![format!("{}.session_id", TOKEN_USAGE_CUBE)]), and the default sessions listing excludes subagent sessions (let parent_filter = if include_subagents { None } else { Some("") };, line 315). On main, subagent usage rolled up to the parent session id, so it surfaced in the parent's cost_usd/token columns. With this change it lands under leaf session ids that the default listing never pulls, and no parent-rollup was added on either the client or the query side — subagent tokens/cost vanish from the default analyze output (and even with --include-subagents they are not attributed to the parent).
Suggested fix: roll child usage into parents in enrich_token_usage (group by parent_session_id when present, or query the cube with an IN filter over parent+children and aggregate client-side).
There was a problem hiding this comment.
Not a regression from this PR: git-ai analyze reads the v1 public_v1_token_usage cube, which is populated by the SessionEvent ETL — and SessionEvents have always used leaf session ids (stream.session_id with parent attrs), unchanged here. The v2 TokenUsage events this PR re-attributes have no consumer in analyze (or any dashboard) yet. The underlying product gap — default parent listings not rolling subagent cost up — therefore predates this stack and applies to the v1 path; worth a follow-up that aggregates by the parent relationship (now uniformly available on both event families), tracked separately.
| { | ||
| attrs = attrs | ||
| .external_parent_session_id(parent_ext.to_string()) | ||
| .parent_session_id(generate_session_id(parent_ext, &identity.tool)); |
There was a problem hiding this comment.
[code-review] Plain Codex forks get leaf attribution but never carry the parent relationship the spec promises
CodexAgent::detect_subagent_parent (src/streams/agents/codex.rs:153) requires payload.thread_source == "subagent" before it reads forked_from_id, so a plain fork (session_meta with forked_from_id but no thread_source) produces a stream record with external_parent_session_id = None — and these attributes are never emitted. Meanwhile the extractor itself happily extracts the same forked_from_id for parent-prefix matching, and docs/token-usage-events-spec.md now states usage is leaf-attributed "Codex fork/subagent rollouts included — with the parent carried as the parent_session_id/external_parent_session_id attributes" and that the parent-inclusive total is "derivable through the parent relationship".
For plain forks that derived rollup permanently misses the fork's usage. The new integration test forked_codex_session_counts_only_its_own_usage exercises exactly this shape and its emitted event has no parent linkage. Either widen detect_subagent_parent to cover plain forked_from_id (matching fork_parent_id in src/token_usage/codex.rs) or scope the spec claim to subagent spawns.
There was a problem hiding this comment.
Intentional, and now stated in the spec (9d2ba34): a plain user-initiated fork (forked_from_id without a subagent thread_source) is a new top-level conversation, not a sub-task — linking it to its parent would also hide it from default session listings, which filter on parent_session_id. Subagent spawns carry the parent attributes; the replayed prefix is removed either way, so no usage is double-counted.
|
|
||
| INSERT INTO schema_version (version) VALUES (4); | ||
| "#, | ||
| // Version 5: leaf session attribution. Usage is owned by the transcript's |
There was a problem hiding this comment.
[code-review] Table rebuild leaves previously-emitted parent-attributed server buckets permanently unzeroed (double count for flag-enabled installs)
Dropping bucket_state deletes the only record that buckets were already emitted, and emptied buckets are re-emitted as zero only from surviving bucket_state rows. Any install that had token_usage_metrics enabled (config/env on a release build, or any debug build — the flag is debug-on) emitted subagent usage under generate_session_id(parent_ext, tool); after this migration the same usage re-emits under leaf session ids while the old parent-attributed server rows are never zeroed — double counted in any parent-inclusive rollup. The "pre-release reset" comment is accurate for release defaults, but debug-build emitters (every dev machine) hit this deterministically. Worth confirming the server side can tolerate or purge the orphaned rows.
There was a problem hiding this comment.
Accepted by design: the v3/v5 rebuilds are explicitly pre-release resets (the flag is release-off; the project owner decided existing data — including anything a debug build uploaded — is disposable and will be cleared server-side). The migration comments document this; production-grade identity migrations would need the source-rollout ownership the finding describes, which is exactly why we are not attempting an in-place reassignment.
bfce097 to
9d2ba34
Compare
cb9895a to
d1d9395
Compare
d1d9395 to
c2a04e7
Compare
| // the rolled-up parent session and cannot be reassigned (they don't | ||
| // record their source rollout), so the tables are rebuilt and the next | ||
| // pass re-extracts everything under leaf identities. | ||
| r#" |
There was a problem hiding this comment.
[code-review] v5 restates ~70 lines of DDL where ALTER TABLE + DELETE is equivalent — and the restatement already drifted once
v5's only structural delta over v4 is one nullable column (tracked_files.external_parent_session_id TEXT) plus the pre-release data reset, but it re-declares all three tables and four indexes. That copy must stay in sync with v3+v4 by hand, and it already failed once on this branch: v5 initially recreated v3's plain bucket index instead of v4's COALESCE(speed, 0) expression index, silently putting the reconciliation GROUP BY back on a temp B-tree (fixed in c2a04e7). A delete-based reset can't have that bug because it preserves v4's definitions by construction, and migrations run strictly sequentially so v4's schema is fixed at this point (v2 already uses ALTER TABLE ... ADD COLUMN):
ALTER TABLE tracked_files ADD COLUMN external_parent_session_id TEXT;
DELETE FROM tracked_files;
DELETE FROM usage_entries;
DELETE FROM bucket_state;
INSERT INTO schema_version (version) VALUES (5);There was a problem hiding this comment.
Acknowledged — the restatement did drift once, which is why the v4/v5 index parity is now pinned by tests (expression-index presence + the forward-version guard). Kept as a rebuild because v5's point is the pre-production data reset (leaf attribution re-keys sessions): DELETE FROM x3 + ALTER expresses the same end state with the same risk surface, and this schema's convention (v3, v5) is reset-by-rebuild. Post-1.0, migrations will be additive ALTERs.
c2a04e7 to
72cc8e3
Compare
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
⚠️ 1 issue in files not directly in the diff
⚠️ Last-only usage permits replay reset
After a last_token_usage-only event, prev_totals remains empty and a repeated fork marker rearms replay filtering. Later genuine usage can disappear.
There was a problem hiding this comment.
🟡 Large parent metadata breaks fork accounting
When parent metadata exceeds 64 KiB, records_session_id rejects the valid parent. Timing-based fallback can misattribute the child’s usage.
(Refers to this code)
Prompt for agents
The new 64 KiB cap in src/daemon/token_usage_worker.rs records_session_id can reject valid Codex session_meta lines. Modern session metadata includes fields such as base_instructions, whose text is not bounded by this code and can push the first JSONL record beyond the cap. Rejection makes resolve_parent_prefix treat the parent as unavailable and invoke the rewritten-burst timing heuristic, which can misclassify replayed or genuine child usage. Bound memory without imposing a smaller semantic limit on valid metadata, for example by streaming only the fields needed from the first JSON object or by using a documented upstream maximum with explicit handling. Add coverage for a valid oversized session_meta record.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 7d34106: the cap is now 1MiB — codex session_meta embeds the session's instructions (AGENTS.md contents) and can legitimately exceed 64KiB, so the probe bounds memory without rejecting real parents (a line truncated at the cap still fails the JSON id parse rather than matching).
8dbc716 to
f5d45c9
Compare
…sion
Usage is now leaf-attributed for every tool: subagent and fork
transcripts own their usage under their own session, carrying the
parent as the parent_session_id/external_parent_session_id attributes
— the same identity model SessionEvents already use, so TokenUsage and
SessionEvents finally agree about which session a transcript is. The
parent-inclusive total becomes a derived rollup over the parent
relationship instead of the stored identity, keeping per-agent cost
visible; recording everything under the parent was lossy in the other
direction.
Previously the worker rolled every stream with an
external_parent_session_id into its parent. For Codex forks that
contradicted both the spec ("sessions are per rollout file") and
ccusage, hiding fork children entirely; for Claude sidechains it
matched ccusage but differed from git-ai's own SessionEvents. This is
now a single standardized rule, with the Claude difference from
ccusage documented (per-session groupings differ; aggregate totals
match). Cross-file dedup of sidechain replays is unaffected — entry
dedup is global, not session-scoped.
tracked_files persists the parent id so DB-only reconciliation carries
the same relationship attributes; schema v5 is a pre-release reset
(existing rows are keyed by the rolled-up parent and don't record
their source rollout, so they cannot be reassigned in place). The
server already ingests both parent attributes into
token_usage_v2_events columns; no wire change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The v5 rebuild recreated the plain (session_id, model, bucket_ts) bucket index, losing v4's expression index and putting the COALESCE(speed, 0) reconciliation GROUP BY back on a temp B-tree; v5 now carries the expression index. The spec also states plain user-initiated Codex forks precisely: a fork without a subagent thread_source is a new top-level conversation that owns its usage with no parent linkage (staying visible in default session listings), while subagent spawns carry the parent attributes — the replayed prefix is removed either way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f5d45c9 to
03c7a58
Compare
Usage is now leaf-attributed for every tool: subagent and fork
transcripts own their usage under their own session, carrying the
parent as the parent_session_id/external_parent_session_id attributes
— the same identity model SessionEvents already use, so TokenUsage and
SessionEvents finally agree about which session a transcript is. The
parent-inclusive total becomes a derived rollup over the parent
relationship instead of the stored identity, keeping per-agent cost
visible; recording everything under the parent was lossy in the other
direction.
Previously the worker rolled every stream with an
external_parent_session_id into its parent. For Codex forks that
contradicted both the spec ("sessions are per rollout file") and
ccusage, hiding fork children entirely; for Claude sidechains it
matched ccusage but differed from git-ai's own SessionEvents. This is
now a single standardized rule, with the Claude difference from
ccusage documented (per-session groupings differ; aggregate totals
match). Cross-file dedup of sidechain replays is unaffected — entry
dedup is global, not session-scoped.
tracked_files persists the parent id so DB-only reconciliation carries
the same relationship attributes; schema v5 is a pre-release reset
(existing rows are keyed by the rolled-up parent and don't record
their source rollout, so they cannot be reassigned in place). The
server already ingests both parent attributes into
token_usage_v2_events columns; no wire change.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Stack created with GitHub Stacks CLI • Give Feedback 💬
🤖 Generated with Claude Code