Attribution Bugs and Refactoring in CLI
can you look at https://github.com/entireio/cli/pull/1305 can you check the branch out locally?
Done. PR #1305 is checked out locally.
PR #1305 — "Add Entire blame and why commands" by suhaanthayyil
You're now on branch codex/entire-blame-why. The PR adds two AI-aware line attribution commands:
entire blame <file> [--line N|N-M] [--json]— likegit blame, but labels lines[AI],[HU],[MX], or[??](uncommitted)entire why <file[:line]> [--json]— resolves a line back to its Entire checkpoint, agent prompt, session, model, etc.
It works by running git blame --line-porcelain, then enriching the blamed commits with Entire-Checkpoint trailers and checkpoint/session metadata from the entire/checkpoints/v1 branch.
Want me to look at the actual diff, run the tests, or try the commands out?
can we make the commands hidden and advertise them in labs
Done. blame and why are now hidden Labs commands. Here's what changed:
cmd/entire/cli/attribution.go — added Hidden: true to both newBlameCmd and newWhyCmd, with the same "hidden during maturation" comment style used by review/org/etc.
cmd/entire/cli/labs.go — registered blame and why in the experimentalCommands slice and added entire blame --help / entire why --help to the "Try:" block in the labs overview.
README.md — removed the two command-table rows (no other Labs command appears in that table) and reworded the prose to point users at entire labs for discovery.
Verified behavior:
entire --help→ blame/why absent from root help ✓entire labs→ both listed as experimental commands ✓entire blame --help/entire why --help→ still fully functional ✓- Existing tests pass (
TestRootHelp_ShowsLabsButHidesReview,TestLabsRegistryCommandsExistAtCanonicalPaths, etc.),mise run fmt && mise run lintclean.
The PR's own labs_test.go already enforces the two key invariants — that hidden Labs commands stay out of root help, and that every experimentalCommands entry resolves at its canonical path — so the new entries are covered by existing tests. Want me to commit this, or add a dedicated test asserting blame/why specifically show up in entire labs?
please commit and then let's do one last proper review
Committed. Now let me run a proper review of the full PR. This branch adds a substantial amount of code (the attribution feature plus my labs change), so I'll launch a thorough multi-dimensional review.
Review target: --target main...HEAD high
high effort → 3+4 angles × 6 candidates → 1-vote verify (recall-biased) → ≤10 findings
You are reviewing for recall at high effort: catch every real bug a careful reviewer would catch in one sitting. At this level, catching real bugs matters more than avoiding false positives. Err on the side of surfacing.
Phase 0 — Gather the diff
Run git diff @{upstream}...HEAD (or git diff main...HEAD / git diff HEAD~1
if there's no upstream) to get the unified diff under review. If there are
uncommitted changes, or the range diff is empty, also run git diff HEAD and
include the working-tree changes in scope — the review often runs before the
commit. If a PR number, branch name, or file path was passed as an argument,
review that target instead. Treat this diff as the review scope.
Phase 1 — Find candidates (3 correctness angles + 3 cleanup angles + 1 altitude angle, up to 6 each)
Run 7 independent finder angles via the Agent tool. Each
surfaces up to 6 candidate findings with file, line, a one-line
summary, and a concrete failure_scenario.
Angle A — line-by-line diff scan
Read every hunk in the diff, line by line. Then Read the enclosing function for
each hunk — bugs in unchanged lines of a touched function are in scope (the PR
re-exposes or fails to fix them). For every line ask: what input, state, timing,
or platform makes this line wrong? Look for inverted/wrong conditions,
off-by-one, null/undefined deref, missing await, falsy-zero checks,
wrong-variable copy-paste, error swallowed in catch, unescaped regex metachars.
Angle B — removed-behavior auditor
For every line the diff DELETES or replaces, name the invariant or behavior it enforced, then search the new code for where that invariant is re-established. If you can't find it, that's a candidate: a removed guard, a dropped error path, a narrowed validation, a deleted test that was covering a real case.
Angle C — cross-file tracer
For each function the diff changes, find its callers (Grep for the symbol) and check whether the change breaks any call site: a new precondition, a changed return shape, a new exception, a timing/ordering dependency. Also check callees: does a parallel change in the same PR make a call unsafe?
Reuse
The angles above hunt for bugs; this one and the next two hunt for cleanup in the changed code. Flag new code that re-implements something the codebase already has — Grep shared/utility modules and files adjacent to the change, and name the existing helper to call instead.
Simplification
Flag unnecessary complexity the diff adds: redundant or derivable state, copy-paste with slight variation, deep nesting, dead code left behind. Name the simpler form that does the same job.
Efficiency
Flag wasted work the diff introduces: redundant computation or repeated I/O, independent operations run sequentially, blocking work added to startup or hot paths. Name the cheaper alternative.
Altitude
Check that each change is implemented at the right depth, not as a fragile bandaid. Special cases layered on shared infrastructure are a sign the fix isn't deep enough — prefer generalizing the underlying mechanism over adding special cases.
Cleanup and altitude candidates use the same file/line/summary shape; in
failure_scenario, state the concrete cost (what is duplicated, wasted, or
harder to maintain) instead of a crash. Correctness bugs always outrank
cleanup and altitude findings when the output cap forces a cut.
Pass every candidate with a nameable failure scenario through — finders that silently drop half-believed candidates bypass the verify step and are the dominant cause of misses.
Phase 2 — Verify (1-vote, recall-biased)
Dedup near-duplicates (same defect, same location, same reason → keep one). For each remaining candidate, run one verifier via the Agent tool: give it the diff, the relevant file(s), and the candidate; it returns exactly one of CONFIRMED / PLAUSIBLE / REFUTED.
PLAUSIBLE by default — do not refute a candidate for being "speculative" or "depends on runtime state" when the state is realistic: concurrency races, nil/undefined on a rare-but-reachable path (error handler, cold cache, missing optional field), falsy-zero treated as missing, off-by-one on a boundary the code does not exclude, retry storms / partial failures, regex/allowlist that lost an anchor. These are PLAUSIBLE.
REFUTED only when constructible from the code: factually wrong (quote the actual line); provably impossible (type/constant/invariant — show it); already handled in this diff (cite the guard); or pure style with no observable effect.
Keep CONFIRMED and PLAUSIBLE. Drop REFUTED.
Output
Return findings as a JSON array of at most 10 objects:
Ranked most-severe first. If more than 10 survive, keep the 10 most
severe. If nothing survives verification, return [].
I have candidates from all angles. I manually confirmed the key ones against the code (resolveLine vs enrich, the Mixed-flag scoping, the path matching). Let me dedup, drop the refuted ones, and present the verified findings.
A few candidates I'm refuting:
- "fetchCheckpointContext store-swap not panic-safe" — there's no
recoveranywhere, so a panic crashes the command regardless; no concurrency exists. Not a real failure. - "resolveLine marks [AI] even when metadata missing" — by design;
MetadataMissingis surfaced to the user with an explicit "trailer-level attribution only" message. - The
maxAttributionLineNumber/writeJSON/go max()nits — too low-value to surface at this scope.
Review: PR #1305 — entire blame / entire why
The feature is well-structured and the labs/hidden wiring is correct (verified: TestLabsRegistryCommandsExistAtCanonicalPaths, TestRootHelp_ShowsLabsButHidesReview all pass with blame/why added). Cross-file calls all check out against real signatures. Findings below are about the attribution logic, ranked most severe first.
Correctness
1. [AI] flips to [MX] between blame and why — inconsistent authorship rules (attribution.go:479 vs :372)
resolveLine marks a line Mixed only if the preferred candidate is Mixed (if preferred != nil && preferred.Mixed). But enrichAttributionLineWithFetch (the fetch path used by entire why when metadata is missing locally) sets authorship = attributionMixed if any candidate is Mixed. So a line with two candidates — preferred not-mixed, secondary mixed — renders [AI] in entire blame file:N but [MX] in entire why file:N. Same line, same data, two different tags. The two code paths should share one authorship rule.
2. Mixed is checkpoint-scoped, not session/file-scoped — over-reports [MX] (attribution.go:420)
readCheckpointContext sets ctx.Mixed = true if the checkpoint's CombinedAttribution is mixed or any session is mixed, then that flag rides onto the per-line candidate. But the line then picks selected — the session that actually touched this file. If a checkpoint touched two files (one AI-only, one human-only) the checkpoint is "mixed" overall, so a line whose own session was pure single-authorship still gets tagged [MX]. The Mixed determination should be scoped to the selected session, not the whole checkpoint.
3. Renamed files silently fall back to the wrong session (attribution.go:437, pathsContainFile at :1081)
Session selection matches the blamed file against FilesTouched by exact normalized-string equality. git blame follows renames and reports the current path, but a checkpoint's FilesTouched records the path at checkpoint time. After a rename, no session matches, and the code silently falls back to the first session (:442-444) — showing that session's agent/model/prompt, which may be unrelated to the line. No indication to the user that the match was a fallback.
4. All-sessions-read-failure yields a blank-but-confident [AI] line (attribution.go:442)
If ReadCommittedCheckpoint succeeds but every readSessionForCheckpoint errors (:428 continue), selected and fallback both stay empty. The line keeps Authorship = AI and a CheckpointID, but SessionID/Agent/Model are blank and MetadataMissing is never set — so the user gets no "metadata unavailable" hint, just silently empty fields. Consider setting MetadataMissing = true when no session could be read.
Cleanup (non-blocking)
5. Three structurally identical structs (attribution.go:73, :85, partially :516)
attributionCandidate and attributionCheckpointContext have identical fields and JSON-compatible shapes — candidateFromContext is literally attributionCandidate(ctx). The cast is clever but signals these should be one type. Adding a field means editing both.
6. renderAttributionBlameCompact / renderAttributionBlameLong duplicate scaffolding (attribution.go:743, :784)
Header print, empty-check, lineWidth calc, per-line loop, and summary call are near-identical; only the column set differs. A shared table-render helper parameterized by columns would remove ~40 lines.
7. Summary percentages can sum to 99% (attribution.go:727)
Integer truncation: 1 AI + 1 Human + 1 Mixed of 3 lines renders 33% · 33% · 33%. Cosmetic and user-visible. (Absolute counts are also shown, so it's not misleading — just looks off.)
My take: Findings 1 and 2 are the ones I'd want addressed before merge — they produce genuinely contradictory/wrong tags, which undercuts the whole point of the feature ("which lines are AI?"). 3 and 4 are real but lower-frequency edge cases. 5–7 are polish.
Want me to fix #1 and #2 (unify the authorship rule and scope Mixed to the selected session)? Those are small, surgical changes and I can add test coverage for the divergence.
keep going on all. do meaningful commits
Full cli package passes. Here's the summary of the work.
Done — all 7 review findings fixed across 5 meaningful commits
(Plus the earlier fed0f65 hiding blame/why and advertising them in labs.)
What changed
Correctness (#1, #2) — Extracted one authorshipForPreferred() rule used by both resolveLine and the why-time enrichment path, so a line can no longer show [AI] in blame and [MX] in why. Scoped the Mixed flag to the session that actually touched the file instead of OR-combining the whole checkpoint, so a line from an agent-only file in a "combined" checkpoint stays [AI]. New regression test covers the pure-AI-session / mixed-checkpoint case.
Honest degradation (#3, #4) — Added a SessionFallback flag set when a blamed file matches none of a checkpoint's sessions (e.g. after a rename); entire why now prints a "may have been renamed" hint instead of presenting a guessed session as exact. When all of a checkpoint's sessions are unreadable, the line is now marked MetadataMissing so the user sees the "trailer-level only" hint and the remote-fetch retry fires. New regression test.
Cleanup (#5, #6, #7) — Collapsed the two identical attributionCandidate/attributionCheckpointContext structs into one (alias) and dropped the no-op conversion; extracted renderAttributionBlameTable for the duplicated header/empty/summary scaffolding (output unchanged); and replaced the drifting integer-division percentages with the largest-remainder method so AI/Human/Mixed sum to 100.
Verification
mise run fmt— no changes;mise run lint— 0 issues- Full
./cmd/entire/clipackage test suite — pass (incl. 3 new regression tests)
Each commit builds and tests green independently. Want me to push the branch, or run the canary (mise run test:e2e:canary) before pushing?