From 75d0c709f1d7c2c9b4d9aebd2afc3d7dcd3e8aec Mon Sep 17 00:00:00 2001 From: reviewer Date: Tue, 21 Jul 2026 05:15:40 +0200 Subject: [PATCH] sessions: round-4 hardening of subagent PR attribution Fourth adversarial review pass. - sessionFingerprint now covers the COMPLETE linkage-relevant payload, not just headline stats: a canonical (sorted-key) serialization of agentSpawnLinks, spawnPrSets, prRefsAtRangeStart, ambiguousSpawnAgentIds, parent/agent identity, and the per-turn prRefs timeline. Two records that share an id and headline stats but map the child to different spawns/PRs now fingerprint DISTINCT, so the ambiguity rule fires and they fold into neither, deterministically rather than order-dependent first-wins. - A date/day filter recomputes prRefsAtRangeStart at the new slice boundary by replaying the original full turn sequence, instead of copying the wide range's value. A PR switch between the wide start and the slice start (July 1 A, July 10 B, slice July 20) now carries B, not a stale A; a turn exactly on the boundary stays in-slice and applies its own refs. The recompute selects by timestamp, so it is order-independent. Non-contiguous day selections are documented as treated contiguous from the earliest selected day (a single session-level seed cannot represent multiple segments; the menubar selection is a single day or a run). - The anchor-carry path drops an anchor that duplicates a surviving session id, so malformed merged input cannot double-count. Also: rebuilt filtered sessions were losing their PR/subagent-linkage metadata (a child its parentSessionId, a parent its prLinks), which carryLinkageFields now restores, so by-PR and folding work on any filtered slice. Every fix mutation-verified. A fresh real-data drive re-proves the no-double-count identity and reconciliation to the cent for both a lifetime scan and a day-filtered slice (anchors created, subagents fold through the filter). --- src/parser.ts | 67 +++++++++++++++++++++-- src/sessions-report.ts | 30 +++++++++- tests/sessions-subagent-fold.test.ts | 82 ++++++++++++++++++++++++++-- 3 files changed, 167 insertions(+), 12 deletions(-) diff --git a/src/parser.ts b/src/parser.ts index fd478c6..cd4e946 100644 --- a/src/parser.ts +++ b/src/parser.ts @@ -3144,7 +3144,8 @@ function isSpawnParent(session: SessionSummary): boolean { function carryLinkageFields(rebuilt: SessionSummary, original: SessionSummary): void { if (original.everHadBranch) rebuilt.everHadBranch = true if (original.prLinks?.length) rebuilt.prLinks = original.prLinks - if (original.prRefsAtRangeStart?.length) rebuilt.prRefsAtRangeStart = original.prRefsAtRangeStart + // prRefsAtRangeStart is NOT copied here: a narrower slice needs it recomputed at + // the new boundary (see recomputeRangeStartPrRefs), not the wide range's value. if (original.parentSessionId) rebuilt.parentSessionId = original.parentSessionId if (original.agentId) rebuilt.agentId = original.agentId if (original.agentSpawnLinks) rebuilt.agentSpawnLinks = original.agentSpawnLinks @@ -3154,7 +3155,52 @@ function carryLinkageFields(rebuilt: SessionSummary, original: SessionSummary): if (original.agentType) rebuilt.agentType = original.agentType } +// The "PR active entering this slice", recomputed by replaying the ORIGINAL full +// turn sequence up to `sliceStartMs`, seeded from the original range-start state. +// A narrower filter must NOT reuse the wide range's range-start PR: a PR switch +// between the wide start and the slice start would otherwise be lost, mis-seeding +// both spend attribution and the subagent grace fallback. A turn exactly ON the +// boundary stays in the slice and applies its own prRefs there, so the walk stops +// strictly before it. +function recomputeRangeStartPrRefs(original: SessionSummary, sliceStartMs: number): string[] | undefined { + // The carried PR is the refs of the LATEST turn (by timestamp) strictly before the + // slice that referenced any PR; a turn exactly on the boundary is inside the slice + // and applies its own refs there. Selected by timestamp, not array position, so + // the result does not depend on turn ordering. Falls back to the original + // range-start state when nothing referenced a PR before the slice. + let current = original.prRefsAtRangeStart + let bestMs = -Infinity + for (const turn of original.turns) { + if (!turn.prRefs?.length) continue + const ts = turn.assistantCalls[0]?.timestamp + if (!ts) continue + const tMs = new Date(ts).getTime() + if (Number.isNaN(tMs) || tMs >= sliceStartMs) continue + if (tMs >= bestMs) { bestMs = tMs; current = turn.prRefs } + } + return current +} + +// Apply a recomputed range-start PR state to a rebuilt session (or clear it). +function applyRecomputedRangeStart(rebuilt: SessionSummary, original: SessionSummary, sliceStartMs: number): void { + const rs = recomputeRangeStartPrRefs(original, sliceStartMs) + if (rs?.length) rebuilt.prRefsAtRangeStart = rs + else delete rebuilt.prRefsAtRangeStart +} + +// Local-midnight epoch of the EARLIEST selected day. Range-start PR state is +// recomputed at this boundary. Non-contiguous day selections are treated as +// CONTIGUOUS from the earliest day: a PR switch inside an unselected gap between two +// selected days is not reflected in the carried state (a single session-level seed +// cannot represent multiple segments). The menubar day selection is a single day or +// a contiguous run, so this is exact in practice. +function earliestDayStartMs(days: Set): number { + const earliest = [...days].sort()[0] + return earliest ? new Date(`${earliest}T00:00:00`).getTime() : NaN +} + export function filterProjectsByDays(projects: ProjectSummary[], days: Set): ProjectSummary[] { + const sliceStartMs = earliestDayStartMs(days) const filtered: ProjectSummary[] = [] for (const project of projects) { const sessions: SessionSummary[] = [] @@ -3163,6 +3209,7 @@ export function filterProjectsByDays(projects: ProjectSummary[], days: Set() for (const session of project.sessions) { const turns = session.turns.filter(turn => { const ds = turnDayString(turn) @@ -3174,10 +3221,15 @@ export function filterProjectsByDays(projects: ProjectSummary[], days: Set !survivingIds.has(a.sessionId)) : anchors + if (sessions.length === 0 && dedupedAnchors.length === 0) continue + filtered.push(summarizeProject(project.project, project.projectPath, sessions, dedupedAnchors)) } return filtered.sort((a, b) => b.totalCostUSD - a.totalCostUSD) } @@ -3230,12 +3282,14 @@ export function filterProjectsByClaudeConfigSource(projects: ProjectSummary[], s } export function filterProjectsByDateRange(projects: ProjectSummary[], dateRange: DateRange): ProjectSummary[] { + const sliceStartMs = dateRange.start.getTime() const filtered: ProjectSummary[] = [] for (const project of projects) { const sessions: SessionSummary[] = [] // Carry existing anchors and convert a spawn parent whose in-range turns are all // filtered out into one (see filterProjectsByDays). const anchors: SessionSummary[] = [...(project.subagentAnchors ?? [])] + const survivingIds = new Set() for (const session of project.sessions) { const turns = session.turns.filter(turn => turnIsInDateRange(turn, dateRange)) if (turns.length === 0) { @@ -3244,10 +3298,13 @@ export function filterProjectsByDateRange(projects: ProjectSummary[], dateRange: } const rebuilt = buildSessionSummary(session.sessionId, session.project, turns, session.mcpInventory, session.source) carryLinkageFields(rebuilt, session) + applyRecomputedRangeStart(rebuilt, session, sliceStartMs) + survivingIds.add(session.sessionId) sessions.push(rebuilt) } - if (sessions.length === 0 && anchors.length === 0) continue - filtered.push(summarizeProject(project.project, project.projectPath, sessions, anchors)) + const dedupedAnchors = survivingIds.size ? anchors.filter(a => !survivingIds.has(a.sessionId)) : anchors + if (sessions.length === 0 && dedupedAnchors.length === 0) continue + filtered.push(summarizeProject(project.project, project.projectPath, sessions, dedupedAnchors)) } return filtered.sort((a, b) => b.totalCostUSD - a.totalCostUSD) } diff --git a/src/sessions-report.ts b/src/sessions-report.ts index 332977d..af14528 100644 --- a/src/sessions-report.ts +++ b/src/sessions-report.ts @@ -239,12 +239,36 @@ function rowSessionKey(session: SessionSummary): string { return `${linkageProvider(session)}${KEY_SEP}${session.project}${KEY_SEP}${session.sessionId}` } +// Sorted-key canonical serialization of a Record, so two records that map the same +// entries fingerprint EQUAL regardless of insertion order. +function canonicalRecord(rec: Record | undefined): string { + if (!rec) return '' + return Object.keys(rec).sort().map(k => { + const v = rec[k]! + return `${k}=${Array.isArray(v) ? v.join(',') : v}` + }).join(';') +} + // Distinguishes two DIFFERENT records that happen to share a session id (duplicate // or imported data): identical copies produce the same fingerprint and fold once, -// any difference (cost, calls, span, PR links) marks the key ambiguous so it folds -// into NEITHER parent/subtree. +// any difference marks the key ambiguous so it folds into NEITHER parent/subtree. +// The fingerprint covers EVERY field that changes fold behavior, not just headline +// stats: two parents mapping the same child to different spawns/PRs (agentSpawnLinks +// / spawnPrSets) must differ here, or the ambiguity rule never fires and a stale +// first-wins returns an order-dependent result. function sessionFingerprint(s: SessionSummary): string { - return [s.totalCostUSD, s.apiCalls, s.firstTimestamp, s.lastTimestamp, (s.prLinks ?? []).join(',')].join(KEY_SEP) + return JSON.stringify([ + s.totalCostUSD, s.apiCalls, s.firstTimestamp, s.lastTimestamp, + s.parentSessionId ?? '', s.agentId ?? '', + (s.prLinks ?? []), + (s.prRefsAtRangeStart ?? []), + [...(s.ambiguousSpawnAgentIds ?? [])].sort(), + canonicalRecord(s.agentSpawnLinks), + canonicalRecord(s.spawnPrSets), + // The per-turn PR-ref timeline: the launch turn's PR is what a child inherits, + // so two records with different prRefs sequences are distinct folds. + s.turns.map(t => (t.prRefs ?? []).join('|')).join('>'), + ]) } /// Index every sidechain (subagent) session by the parent that spawned it, keyed diff --git a/tests/sessions-subagent-fold.test.ts b/tests/sessions-subagent-fold.test.ts index 926845a..7535585 100644 --- a/tests/sessions-subagent-fold.test.ts +++ b/tests/sessions-subagent-fold.test.ts @@ -6,6 +6,7 @@ import { prLinkedTotals, resolveSubagentAttribution, } from '../src/sessions-report.js' +import { filterProjectsByDateRange, filterProjectsByDays } from '../src/parser.js' import type { ClassifiedTurn, ParsedApiCall, ProjectSummary, SessionSummary, TokenUsage } from '../src/types.js' const A = 'https://github.com/o/r/pull/1' @@ -367,6 +368,23 @@ describe('MAJOR: id-collision contamination', () => { expect(totals.attributedCost).toBeCloseTo(30, 6) // no child double-charge }) + it('folds nowhere when two parents share id + headline stats but map the child to different spawns/PRs', () => { + // Same cost/calls/prLinks/turn-refs, but P1 maps c1 -> spawn x (PR A) and P2 maps + // c1 -> spawn y (PR B). A fingerprint over headline stats alone would miss this + // and first-wins by input order; the full linkage fingerprint makes it ambiguous. + const mk = (order: 'p1' | 'p2') => { + const p1 = parent({ id: 'P', prLinks: [A, B], turns: [turn({ cost: 10, ts: '2026-07-01T10:00:00Z', prRefs: [A] })], agentSpawnLinks: { c1: 'x' }, spawnPrSets: { x: [A] } }) + const p2 = parent({ id: 'P', prLinks: [A, B], turns: [turn({ cost: 10, ts: '2026-07-01T10:00:00Z', prRefs: [A] })], agentSpawnLinks: { c1: 'y' }, spawnPrSets: { y: [B] } }) + const c = child({ agentId: 'c1', parentId: 'P', cost: 100, firstTs: '2026-07-01T10:05:00Z' }) + return [project(order === 'p1' ? [p1, p2, c] : [p2, p1, c])] + } + for (const order of ['p1', 'p2'] as const) { + const totals = prLinkedTotals(mk(order)) + expect(totals.subagentSessions).toBe(0) // ambiguous identity -> child folds nowhere + expect(totals.attributedCost).toBeCloseTo(20, 6) // only the two parents own $10 turns + } + }) + it('folds nowhere when a PR-bearing parent shares its id with a PR-LESS parent', () => { // Only ONE of the two colliding parents has prLinks, but the identity is still // ambiguous: count ALL candidates, not just PR-bearing ones. @@ -418,6 +436,50 @@ describe('MAJOR: ambiguous pairing + late child grace window', () => { }) }) +describe('MAJOR: range-start PR state is recomputed at a filter boundary', () => { + const A_TURN = () => turn({ cost: 10, ts: '2026-07-01T10:00:00Z', prRefs: [A] }) + const B_TURN = () => turn({ cost: 10, ts: '2026-07-10T10:00:00Z', prRefs: [B] }) + const LATE_TURN = () => turn({ cost: 10, ts: '2026-07-20T10:00:00Z' }) // no refs, carries the active PR + // The wide parse recorded PR A as active entering the range; a switch to B lands + // July 10. A slice starting July 20 must attribute the ref-less July 20 turn to B, + // NOT the stale A. Tested under both turn orderings. + const build = (turns: ClassifiedTurn[]) => [project([parent({ id: 'S', prLinks: [A, B], prRefsAtRangeStart: [A], turns })])] + + it('recomputes to B (July 1 A -> July 10 B, slice July 20), both turn orders', () => { + const range = { start: new Date('2026-07-20T00:00:00Z'), end: new Date('2026-07-21T23:59:59Z') } + for (const turns of [[A_TURN(), B_TURN(), LATE_TURN()], [LATE_TURN(), B_TURN(), A_TURN()]]) { + const rows = aggregateByPr(filterProjectsByDateRange(build(turns), range)) + expect(rowFor(rows, B)?.cost ?? 0).toBeCloseTo(10, 6) // ref-less July 20 turn -> B + expect(rowFor(rows, A)).toBeUndefined() // NOT the stale range-start A + } + }) + + it('a slice starting exactly ON the switch turn attributes that turn to the new PR', () => { + // Switch to B lands July 10 10:00; slice starts July 10 00:00. The July 10 turn is + // inside the slice and applies B; the recomputed seed (from before) is A. + const range = { start: new Date('2026-07-10T00:00:00Z'), end: new Date('2026-07-21T23:59:59Z') } + const rows = aggregateByPr(filterProjectsByDateRange(build([A_TURN(), B_TURN(), LATE_TURN()]), range)) + expect(rowFor(rows, B)!.cost).toBeCloseTo(20, 6) // July 10 (B) + July 20 (carried B) + expect(rowFor(rows, A)).toBeUndefined() // July 1 A turn is out of slice + }) + + it('day filter recomputes the same way (menubar path)', () => { + const rows = aggregateByPr(filterProjectsByDays(build([A_TURN(), B_TURN(), LATE_TURN()]), new Set(['2026-07-20']))) + expect(rowFor(rows, B)?.cost ?? 0).toBeCloseTo(10, 6) + expect(rowFor(rows, A)).toBeUndefined() + }) + + it('drops an anchor that duplicates a surviving session id (malformed merged input)', () => { + // A surviving in-range session and a stray anchor carry the SAME id: the real + // session wins, the duplicate anchor is dropped. + const surviving = parent({ id: 'dup', prLinks: [A], turns: [turn({ cost: 10, ts: '2026-07-20T10:00:00Z', prRefs: [A] })], spawnPrSets: { x: [A] } }) + const strayAnchor = parent({ id: 'dup', prLinks: [A], turns: [], spawnPrSets: { x: [A] } }) + const proj = { ...project([surviving]), subagentAnchors: [strayAnchor] } + const filtered = filterProjectsByDays([proj], new Set(['2026-07-20'])) + expect(filtered.flatMap(p => p.subagentAnchors ?? []).some(a => a.sessionId === 'dup')).toBe(false) + }) +}) + describe('MINOR: row session-key delimiter does not collide on names with spaces', () => { it('counts two sessions whose space-joined keys would collide as distinct', () => { // "a b" + "c" and "a" + "b c" both become "a b c" under a space delimiter, @@ -451,10 +513,22 @@ describe('MAJOR: recursion dedup and conflicting duplicates', () => { } }) - it('identical duplicate ids fold exactly once', () => { - // Same id, same fingerprint (cost/span/links): one logical session, folds once. + it('a truly identical duplicate child (same parent, same record) folds exactly once', () => { + // Two copies of the SAME child under the SAME parent, identical in every + // fingerprinted field: one logical session, folds once (not doubled). + const mk = () => [project([ + parent({ id: 'P', prLinks: [A], turns: [turn({ cost: 10, ts: '2026-07-01T10:00:00Z', prRefs: [A] })], agentSpawnLinks: { c1: 'x' }, spawnPrSets: { x: [A] } }), + child({ agentId: 'c1', parentId: 'P', cost: 100, firstTs: '2026-07-01T10:05:00Z' }), + child({ agentId: 'c1', parentId: 'P', cost: 100, firstTs: '2026-07-01T10:05:00Z' }), // exact duplicate + ])] + expect(rowFor(aggregateByPr(mk()), A)!.cost).toBeCloseTo(110, 6) // 10 + 100 (once, not 210) + expect(prLinkedTotals(mk()).subagentSessions).toBe(1) + }) + + it('the SAME id under DIFFERENT parents is ambiguous even at equal cost (distinct identity)', () => { + // Different parentSessionId is part of the fingerprint, so these are NOT the + // same logical session: fold neither. const rows = aggregateByPr(diamond(50, 50, 'c1first')) - expect(rowFor(rows, A)!.cost).toBeCloseTo(260, 6) // 10 + 100 + 100 + 50 (once) - expect(prLinkedTotals(diamond(50, 50, 'c1first')).subagentSessions).toBe(3) + expect(rowFor(rows, A)!.cost).toBeCloseTo(210, 6) // grandchild folds nowhere }) })