Fix Checkpoint Push and Doctor Command Issues

Claude Code·

Soph·2mo ago·53min·3 Checkpoints·5 file changes·+67/-25·80.5K tokens

Review target: GitHub pull request https://github.com/entireio/cli/pull/1643.

Gather this target's diff with (instead of any local git diff):

gh pr view https://github.com/entireio/cli/pull/1643 --json title,body,author,baseRefName,headRefName,state,additions,deletions,changedFiles,labels for context
gh pr diff https://github.com/entireio/cli/pull/1643 for the unified diff

The PR's diff is the only review scope — local working-tree changes are out of scope. When an angle needs surrounding code, Read the files in this checkout if it matches the PR's branch, otherwise fetch file contents via gh.

medium effort → 3+5 angles × 6 candidates → 1-vote verify → ≤8 findings

You are reviewing for precision at medium effort: every finding you surface should be one a maintainer would act on.

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 + 1 conventions angle, up to 6 each)

Run 8 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. Also flag long-lived objects built from closures or captured environments — they keep the entire enclosing scope alive for the object's lifetime (a memory leak when that scope holds large values); prefer a class/struct that copies only the fields it needs. 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.

Conventions (CLAUDE.md)

Find the CLAUDE.md files that govern the changed code: the user-level ~/.claude/CLAUDE.md, the repo-root CLAUDE.md, plus any CLAUDE.md or CLAUDE.local.md in a directory that is an ancestor of a changed file (a directory's CLAUDE.md only applies to files at or below it). Read each one that exists, then check the diff for clear violations of the rules they state.

Only flag a violation when you can quote the exact rule and the exact line that breaks it — no style preferences, no vague "spirit of the doc" inferences. In the finding, name the CLAUDE.md path and quote the rule so the report can cite it. If no CLAUDE.md applies, return nothing for this angle.

Cleanup, altitude, and conventions candidates use the same file/line/summary shape; in failure_scenario, state the concrete cost (what is duplicated, wasted, harder to maintain, or which CLAUDE.md rule is broken) instead of a crash. Correctness bugs always outrank cleanup, altitude, and conventions 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, 3-state)

Dedup candidates that point at the same line/mechanism, keeping the one with the most concrete failure scenario. For each remaining candidate, run one verifier via the Agent tool: give it the diff, the relevant file(s), and the candidate, and have it return exactly one of:

CONFIRMED — can name the inputs/state that trigger it and the wrong output or crash. Quote the line.
PLAUSIBLE — mechanism is real, trigger is uncertain (timing, env, config). State what would confirm it.
REFUTED — factually wrong (code doesn't say that) or guarded elsewhere. Quote the line that proves it.

Keep candidates where the vote is CONFIRMED or PLAUSIBLE.

Output

Return findings as a JSON array of at most 8 objects:

Ranked most-severe first. If more than 8 survive, keep the 8 most severe. If nothing survives verification, return [].

Present the review

After the final phase, do not reply with the raw JSON findings array. Present a readable review: a 2-3 sentence overview of what the PR does, then the surviving findings most-severe first as file:line — summary (failure scenario), or a note that nothing survived verification.

2mo ago·2m

I've completed a thorough line-by-line analysis, grounded in the actual go-git source (../../go-git) and the consumer code in this checkout. Here's the review.

Overview

PR #1643 replaces three go-git worktree.Status() calls on hook hot paths with a new GitCLIStatus(ctx) helper that shells out to git status --porcelain=v1 -z -uall --no-renames and parses the output into a go-git-compatible git.Status map. The motivation is sound: go-git stats and hashes every on-disk file (including large gitignored trees), causing 40–55s hook latency; native git prunes ignored dirs. I verified the core mechanics are correct:

StatusCode mapping is exact — go-git's StatusCode constants (status.go:75-82) are byte-identical to porcelain v1 letters, so the drop-in claim holds.
--no-renames is the right choice for parity — go-git's Status derives changes via merkletrie diff (worktree_status.go:86-130) and splits renames into Delete+Add rather than emitting R; --no-renames makes the CLI do the same, so both sides agree.
Parser is robust — XY<space>path layout, -z disables quoting, trailing-NUL empty entry filtered by len < 4, entry[2] != ' ' guard is safe for all real entries including unmerged (UU).
No import cycle / unused vars — cli → strategy is allowed and already imported; worktree is still used for worktree.Filesystem().Root(); paths.WorktreeRoot (git rev-parse --show-toplevel) and go-git's worktree root resolve to the same top-level, so joined file reads in the attribution path stay correct.
Test change is sound — testutil.InitRepo (already the established pattern in the file) creates a real .git with refs/ that native git recognizes; empty-HEAD git status works fine.

Findings

cmd/entire/cli/state.go — DetectFileChanges switch (~line 298) — unmerged (merge-conflict) status codes are silently dropped, a behavior change from the go-git path (PLAUSIBLE).

The switch only categorizes Untracked / Deleted / Modified / Added. Native git status reports merge-conflicted files with unmerged codes (UU, AA, DU, etc.) → Staging='U', Worktree='U', which match none of the cases, so a conflicted file lands in no bucket (New/Modified/Deleted). go-git's Status() never emits U — its derivation (worktree_status.go:92-129) only produces Deleted/Added/Modified/Untracked/Unmodified, so a conflicted file previously surfaced as Modified and was captured. Failure scenario: an agent turn ends while a merge/rebase/cherry-pick conflict is unresolved in the worktree → the turn-end checkpoint's file-change set omits the conflicted file(s) until the conflict is resolved. Self-healing once resolved, and merge conflicts mid-turn are uncommon, so this is low-severity — but it's a genuine, PR-introduced difference worth a case ... == git.UpdatedButUnmerged arm or an explicit note. (The parallel loop in calculatePromptAttributionAtStart is not affected — it filters on "any non-Unmodified code," so UU passes through there.)

Nothing else survived scrutiny — the reuse/altitude story is actually a plus here (the PR consolidates the per-call-site CLI-status pattern into one shared helper rather than adding a bandaid), and the measurements and test alignment check out.

Traced it end to end. Here's what actually happens when a file is dropped from DetectFileChanges' output, and it turns out the finding is a bit broader and better-evidenced than I first framed it.

Two distinct effects

1. Metadata (minor). The file won't be added to state.FilesTouched / NewFiles / DeletedFiles, and won't count toward totalChanges. If it happens to be the only change in the turn, the whole checkpoint is skipped (lifecycle.go:884 → "no files modified, skipping checkpoint"). Normally other files keep the checkpoint alive, so this is just an incomplete file list. Note the line-attribution path (calculatePromptAttributionAtStart) is not affected — it includes any non-Unmodified code, so UU passes through there.

2. Content capture (the real risk). For subsequent checkpoints, store.Write builds the shadow-branch tree from exactly allFiles = ModifiedFiles + NewFiles layered on the base tree (ephemeral.go:94-98,130 → buildTreeWithChanges reads each listed file's worktree content). A file that's in none of the lists is not written into the tree — it keeps the base commit's version. So the checkpoint records the pre-change content, and a later rewind to that checkpoint silently restores the old version, discarding the conflict-resolution (or type-change) work.

What masks it

Modified files are primarily transcript-derived (lifecycle.go:868-872 — git status is explicitly "a fallback"). If the agent edited the file via a recognized Edit/Write tool, it's captured regardless. The loss only bites when the change came from outside recognized tool calls — the merge operation itself, a shell git checkout --theirs, a rebase, or a manual edit in another terminal.
The first checkpoint of a session is safe — it ignores the passed lists entirely and calls collectChangedFiles (ephemeral.go:76-88), which does its own scan.

The clincher

That collectChangedFiles scan — the codebase's other git status --porcelain -z -uall parser, sitting in the same file — handles exactly the codes the new path drops:

So it's not just merge conflicts — DetectFileChanges' switch also drops type changes (T), and the authors already decided both matter (explicit cases + the "include current file state" comment). go-git surfaced these as Modified; native git labels them U/T, which match none of DetectFileChanges' four arms.

Net: a narrow but genuine data-loss window — a 2nd+ checkpoint taken mid-conflict or after a type change, where the change originated outside recognized tool edits, checkpoints the stale version and rewind loses the work. Trivially fixable: mirror collectChangedFiles' U/T handling in the DetectFileChanges switch. Since the reference implementation is right there in the same package, that's a concrete thing to ask the PR author to add.

Here's a ready-to-paste comment. It leads with the positives, then raises the one substantive issue with evidence and a concrete fix.

Nice change — the go-git Status() → native CLI swap is well-motivated, and I like that GitCLIStatus consolidates the pattern into one helper rather than open-coding the parse at each call site. I verified the mapping is sound: go-git's StatusCode constants are byte-identical to porcelain v1 letters, and passing --no-renames correctly matches go-git's behavior of splitting renames into delete+add (its merkletrie diff never emits R), so parity holds there.

One behavioral gap worth addressing before merge:

DetectFileChanges drops unmerged (U) and type-change (T) files that go-git previously surfaced as Modified.

The switch in DetectFileChanges only categorizes Untracked / Deleted / Modified / Added. Native git status reports merge-conflicted files as UU/AU/DU/… and type changes (file↔symlink) as T — none of which match any arm, so those files fall into no bucket. go-git's Status() never emits U/T; during a conflict its diff-based derivation reported the file as Modified, so it was included. So this is a parity regression, not pre-existing behavior.

Concrete impact: for subsequent (non-first) checkpoints, store.Write builds the shadow tree from ModifiedFiles + NewFiles only (ephemeral.go:94-98,130) — a file in none of the lists keeps the base commit's version. So a checkpoint taken mid-conflict (or after a type change) records the stale content, and a rewind to it silently discards that work. It's a narrow window — modified files are primarily transcript-derived (git status is the fallback per lifecycle.go:868-872), and the first checkpoint is unaffected because it uses collectChangedFiles instead — but it's real when the change originates outside recognized tool edits (the merge itself, a shell git checkout --theirs, a manual edit).

The fix is right next door: collectChangedFiles (the other git status --porcelain -z -uall parser in ephemeral.go) already handles both, deliberately:

Mirroring those two cases in the DetectFileChanges switch (treating U/T as modified) would restore parity. Might also be worth a quick test with a conflicted file to lock it in.

Everything else checks out — imports/no-cycle, the retained worktree var is still used for Filesystem().Root(), paths.WorktreeRoot and the go-git root resolve to the same top-level, and the testutil.InitRepo test change matches the established pattern. 👍

Want me to post this to the PR with gh, or would you rather paste it yourself?

Thanks for the PR — the go-git → native CLI swap is well-motivated and cleanly done, and consolidating the parse into one GitCLIStatus helper is nice.

One open issue before merge: DetectFileChanges drops unmerged (U) and type-change (T) files. Its switch only handles Untracked/Deleted/Modified/Added, but native git status reports merge conflicts as UU/AU/… and type changes (file↔symlink) as T — none match any arm, so those files land in no bucket. go-git's Status() never emitted U/T (it surfaced them as Modified), so this is a parity regression.

Impact: for non-first checkpoints, store.Write builds the tree from ModifiedFiles + NewFiles only (ephemeral.go:94-98,130), so a dropped file keeps the base commit's version — a checkpoint taken mid-conflict records stale content, and rewinding to it silently discards that work. Narrow window (modified files are primarily transcript-derived, and the first checkpoint uses collectChangedFiles instead), but real when the change comes from outside recognized tool edits.

The fix is right next door — collectChangedFiles in the same file already handles both deliberately:

Mirroring those two cases in the DetectFileChanges switch restores parity; a quick test with a conflicted file would lock it in.

Want me to post it with gh?

Short answer: the casing is fine, but the name is slightly off-convention on two counts.

1. Word order. Exported helpers in strategy/common.go are verb-first — GetGitCommonDir, GetMetadataRefTree, CollectUntrackedFiles, ReadSessionPromptFromTree, HardResetWithProtection. GitCLIStatus is noun-first, so it reads against the grain.

2. It names the mechanism, not the result. The two closest siblings do exactly what this helper does — shell out to native git as a faster replacement for worktree.Status() — and neither puts "CLI" in the identifier:

getStagedFiles — doc comment says "using native git CLI", name says what it returns.
CollectUntrackedFiles (git ls-files) — same: mechanism in the comment, result in the name.

GitCLIStatus bakes the "how" (CLI) into the "what", which the house style deliberately avoids.

(The CLI all-caps initialism itself is correct Go style, so no issue there.)

A better-aligned name would be GetWorktreeStatus, CollectWorktreeStatus, or just WorktreeStatus(ctx) — matches the verb-first pattern and describes the result. Keep the "native git CLI, faster than go-git's worktree.Status()" rationale in the doc comment, exactly as getStagedFiles and CollectUntrackedFiles already do.

There's a mild counter-argument that CLI signals "the CLI-backed equivalent of go-git's Status()" — but the sibling helpers show that contrast belongs in the comment, not the name. Worth a nit on the PR if you want the surface consistent.

Want me to fold this into the PR comment?

Review target: GitHub pull request https://github.com/entireio/cli/pull/1665.