Skip to content

yield: single-owner commit attribution, ambiguous bucket, heuristic caveat#692

Merged
iamtoruk merged 2 commits into
getagentseal:mainfrom
avs-io:fix/641-yield-attribution
Jul 16, 2026
Merged

yield: single-owner commit attribution, ambiguous bucket, heuristic caveat#692
iamtoruk merged 2 commits into
getagentseal:mainfrom
avs-io:fix/641-yield-attribution

Conversation

@avs-io

@avs-io avs-io commented Jul 16, 2026

Copy link
Copy Markdown
Member

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

  1. Single-owner attribution: each commit goes to at most one session — the containing window with the smallest span (tie-break: earlier start, then session order). No commit can flip two sessions productive or double into commitCount.
  2. ambiguous category: a session that received no commits but whose window overlapped a credited session's window is now ambiguous instead of silently abandoned — it makes no shipped/never-committed claim. New summary bucket in text and JSON (deliberate public-shape change; JSON tests updated).
  3. Honesty caveat: the text report carries one line — Attribution: timestamp-window based (heuristic) — and the JSON gains methodology: "timestamp-window".

Branch-match / author-time tightening from the issue's optional item 3 is intentionally out of scope.

Tests

  • New tests/yield-overlap-attribution.test.ts (written red-first against a real temp git repo: the zero-commit overlapping session was classified productive before the fix; ambiguous after).
  • tests/yield.test.ts updated for the new bucket + methodology field.
  • npx tsc --noEmit clean; 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.

@iamtoruk iamtoruk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

  1. 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.

  2. 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.

@avs-io
avs-io force-pushed the fix/641-yield-attribution branch from 1f56111 to 24017d9 Compare July 16, 2026 18:21
@avs-io

avs-io commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

All three items addressed, rebased on current main:

  1. cwd-fallback double-credit: attribution now groups sessions by resolved projectCwd and runs once per repo, so project entries sharing the fallback can no longer each award the same commit. Regression test added (two projectPath-less projects, one commit → exactly one productive session).
  2. Regression pins: single-session productive pin added, plus an equal-window tie determinism test asserted under both array orders — tie-break switched from array index to sessionId as suggested.
  3. Sharper ambiguous rule adopted: a zero-commit session is ambiguous only when it was a losing candidate for at least one commit (tracked inside attributeCommits), not on mere window overlap — genuinely abandoned parallel spend stays abandoned. The overlap-based check and its windowsOverlap helper are gone, which also removed the redundant guards.

Also: README yield docs now list the ambiguous bucket and the timestamp-window methodology line.

One body note as requested: sessions migrating abandonedambiguous will read as a spurious improvement against pre-existing act report baselines; the effect is one-time and only affects users with parallel sessions whose commits fit multiple windows.

@iamtoruk iamtoruk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@avs-io
avs-io marked this pull request as ready for review July 16, 2026 19:02
@iamtoruk
iamtoruk merged commit f2864ff into getagentseal:main Jul 16, 2026
3 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.

yield: timestamp-window commit attribution can cross-credit parallel sessions and states results as fact

2 participants