yield: single-owner commit attribution, ambiguous bucket, heuristic caveat#692
Conversation
iamtoruk
left a comment
There was a problem hiding this comment.
The core design reviewed well: smallest-window single-owner attribution is deterministic in practice, the ambiguous bucket keeps its cost in totals, the heuristic caveat lands on both the text and JSON surfaces, and the parallel-session test genuinely fails on old code. Two scoped items before merge.
-
The single-owner invariant does not hold across project entries sharing the cwd fallback. Attribution runs per project entry, but every entry whose projectPath is missing or not a git repo resolves to the same cwd and reuses the same commit list, so two such entries can each award the same commit to one of their sessions, two productive sessions from one commit. That is the exact fallback path #641 called out. Either group attribution by resolved projectCwd and attribute once per repo, or scope the claim explicitly in the body and file the gap.
-
Add a single-session regression pin. computeYield had no behavioral tests before this PR, so the common case is protected by reasoning only; one fixture asserting identical classification for a lone session would pin it. An equal-window tie determinism test would be welcome too, ideally tie-breaking on sessionId rather than array index since session order is cache-state dependent.
One semantic question to confirm rather than change: a zero-commit session goes ambiguous whenever its window merely overlaps a credited session, even if no commit was ever inside its own window. The sharper condition, was a losing candidate for at least one commit, is available for free inside attributeCommits. As written, heavy parallel users will see genuinely abandoned spend drain into ambiguous. If the broad semantics are intentional, say so in the body; the sharper rule looks strictly better.
Also worth a line in the body: sessions migrating abandoned to ambiguous will read as a spurious improvement against pre-existing act report baselines. Nits: the README yield JSON docs still omit the new bucket; a couple of redundant guards flagged inline by the norms we strip.
1f56111 to
24017d9
Compare
|
All three items addressed, rebased on current main:
Also: README yield docs now list the ambiguous bucket and the timestamp-window methodology line. One body note as requested: sessions migrating |
iamtoruk
left a comment
There was a problem hiding this comment.
Re-reviewed the update in depth, including running the suite and mutation checks locally since CI does not execute tests. All three review items landed cleanly: attribution now groups by resolved repo so the single-owner invariant holds across cwd-fallback entries (breaking the grouping fails exactly the new cross-entry test), ambiguous now requires losing candidacy for a real commit rather than mere overlap (disabling lostCandidacy fails three tests), and identical-window ties break on sessionId with an order-independence pin. Full suite green apart from the pre-existing app/electron failures tracked in #681, tsc clean. Nice turnaround, and the lostCandidacy naming reads better than what I had sketched for the same fix.
Problem
A commit is attributed to every session whose
[firstTimestamp, lastTimestamp + 1h]window contains it. Two sessions overlapping in the same repo each get credited with the other's commits, so a session that shipped nothing can land in the productive bucket, and the report states the classification as fact ("sessions shipped to main") with no caveat. (#641)Fix
commitCount.ambiguouscategory: a session that received no commits but whose window overlapped a credited session's window is nowambiguousinstead of silentlyabandoned— it makes no shipped/never-committed claim. New summary bucket in text and JSON (deliberate public-shape change; JSON tests updated).Attribution: timestamp-window based (heuristic)— and the JSON gainsmethodology: "timestamp-window".Branch-match / author-time tightening from the issue's optional item 3 is intentionally out of scope.
Tests
tests/yield-overlap-attribution.test.ts(written red-first against a real temp git repo: the zero-commit overlapping session was classifiedproductivebefore the fix;ambiguousafter).tests/yield.test.tsupdated for the new bucket + methodology field.npx tsc --noEmitclean; full suite 1711 passed / 0 failed.Closes #641
Post-review updates (see review response comment): attribution grouped per resolved repo (fixes cwd-fallback double-credit), ambiguous = lost-candidacy (sharper rule), sessionId tie-breaking with determinism tests, README docs updated. Note: sessions migrating abandoned → ambiguous will read as a one-time spurious improvement against pre-existing act report baselines.