Review CLI Repository Pull Requests

I found 11 open PRs by stale2000. I'll review each one — checking whether the claimed issue is real/relevant and whether the implementation is sound. Since these are independent, I'll fan out parallel review agents (each one fetches the PR, validates the claimed bug against the actual codebase, and assesses the fix).

I now have enough to assess all 11. PR #1391 is complete (base tree has huh.NewForm( in exactly 3 production files; the PR converts the two that aren't uiform.go, so its own guard test passes). No import cycle since uiform imports only huh. PR #1390 is a large but mechanical test-only refactor matching the documented testutil.InitRepo convention.

Here's the full review.

Review: 11 open PRs by stale2000

A cross-cutting note first: #1407 should land first. Five of these PRs (#1408, #1396, #1391, #1390, #1386) report CI blocked by a failing TestRunAuthStatus_RendersSessionsTable date test — which is exactly the timezone bug #1407 fixes. Merging it unblocks the others.

Bug fixes — all sound, recommend merge

#1408 — rewind of tracked deletions — merge, with a design note

• Issue real? ✅ Yes. I verified buildTreeWithChanges applies changes onto baseTreeHash, so checkpoint trees are full snapshots, not partial diffs — confirming the premise that "tracked in HEAD but absent from checkpoint" genuinely means a deletion. Old rewind left such files stale on disk.
• Implementation: Good. Deletion is scoped to HEAD-tracked files absent from the checkpoint, skips isProtectedPath (verified it covers .git, .entire, all agent dirs), tolerates ErrNotExist, and PreviewRewind now reports them. Clean refactor into helpers.
• Design note worth raising: when rewinding across a commit (HEAD advanced past the session base), files committed after the checkpoint are tracked-in-HEAD-but-absent-from-checkpoint, so they get deleted. That matches true "make working tree match checkpoint" semantics and is surfaced in the preview, but it's more destructive than before — worth a maintainer eyeball. The author flagged "I don't understand how the fix works," but the fix itself is correct.
• Nit: new test uses git.PlainInit instead of testutil.InitRepo.
• Verdict: merge (confirm the cross-commit behavior is intended).

Cleanups

#1386 — Lefthook pre-push safety net — request changes / needs maintainer call on approach

• Issue #1349 real? ✅ Yes, and well-documented: lefthook silently reclaims pre-push on any lefthook.yml change, killing Entire's session push with no error.
• But the approach diverges from the issue. #1349 proposes two fixes: (1) idempotent/manager-aware install, or (2) decouple the handler from hook ownership. This PR implements a third path — auto-writing an entire-push-sessions command into lefthook-local.yml (priority -100) that re-invokes Entire's handler when the marker is missing, plus a .git/info/exclude entry.
• Implementation quality: solid and well-tested (preserves existing local commands, clean removal on disable, .config/lefthook.yml detection, YAML-only guard). It does close the "push with no intervening commit" gap.
• Concerns: (a) it auto-mutates the user's working tree (writes lefthook-local.yml) on entire enable — invasive, even if git-excluded; (b) it sidesteps the root cause (Entire still owns the hook and fights lefthook) rather than the idempotent-install or decouple options the issue author preferred. This is an opinionated design choice that warrants maintainer agreement before merge.
• Verdict: request changes / discuss approach — the code is good, but the strategy should be a deliberate decision, not defaulted in.

Bottom line: 9 of 11 are clean merges (land #1407 first to unblock CI). #1408 is correct but worth confirming the cross-commit deletion semantics. #1386 is well-built but takes an approach the issue didn't propose and auto-edits the repo — that one needs a maintainer decision before merging.

Want me to post any of these as PR review comments, or dig deeper into #1408's cross-commit behavior or #1386's approach?

All green on PR #1390 with main merged in:

The earlier-blocking TestRunAuthStatus_RendersSessionsTable failure is gone now that #1407 is in main, confirming that was the only thing holding this PR's CI back. The test-setup standardization itself is clean — no behavior regressions across the converted packages (checkpoint, strategy, cli, agent).

This matches my earlier "merge once CI is green" verdict — it's now green. The only thing I haven't run is the E2E canary (test:ci would add it), but that exercises the Vogon agent rather than the converted test fixtures, so it's not specifically relevant to this PR. Want me to run it anyway, or anything else?

Note: you're now on the codex/standardize-git-test-setup branch locally (was push-compact-for-v1). Let me know if you'd like me to switch back.

Verdict: don't merge as-is — request changes on the approach (the code itself is fine).

The bug (#1349) is real and worth fixing. My objection is strategic, not about code quality:

What's good:

• Closes the real gap, including the hard case (a push with no intervening commit to re-trigger a self-heal).
• Genuinely well-tested — marker-guarded skip/run, preserves existing local commands, clean removal on disable, .config/lefthook.yml detection, YAML-only guard.
• Reversible: git-excluded, removed on entire disable.

Why I'd still push back:

1. It auto-writes a file into the user's repo on entire enable. entire enable going from "installs .git/hooks/*" to "also materializes lefthook-local.yml in the working tree and edits .git/info/exclude" is a new, surprising side effect. Even git-excluded, it's the kind of thing that makes people distrust a tool touching their repo.

2. It doesn't address the root cause — it layers a second mechanism on top of it. Entire still owns pre-push and still loses the fight with lefthook; this adds a fallback that fires after lefthook wins. So you end up with two independent paths that can invoke the handler, reconciled only by a marker grep. More moving parts around an already-fragile interaction.

3. The issue author already scoped the clean fixes and this isn't either of them: (a) idempotent/manager-aware install (detect lefthook/husky/core.hooksPath, don't blindly re-wrap), or (b) decouple the handler from hook ownership so lefthook can call entire hooks git pre-push as a job. Both attack the conflict directly; this routes around it.

Recommendation: prefer option (a) — manager-aware, idempotent install. It's less invasive (no file written into the repo), fixes the silent re-wrap-of-lefthook's-bare-hook problem the issue calls out, and doesn't create a parallel invocation path. If the maintainers decide the lefthook-local approach is acceptable, the implementation is solid enough to merge with minor polish — but that's a deliberate product call, not a default.

So: the one PR of the eleven I would not merge without a maintainer decision on direction. Everything else stands as reviewed.

Want me to sketch what the manager-aware idempotent-install version would look like, or check out #1386 and run its tests too?

All green:

Notably the new uiform package passes, including the guard test (TestProductionFormsUseUIFormHelper) that fails if any production file outside uiform.go uses huh.NewForm( directly — confirming the PR converted every call site. The cmd/entire/cli package (which holds the auth-date test) passed too; the timezone bug didn't trip on this runner.

I didn't run integration tests — this PR only touches interactive form construction (uiform, versioncheck, strategy rewind prompt), nothing that the integration suite exercises, so they're not relevant here.

This confirms my earlier verdict: merge (clean cleanup, no import cycle, complete conversion). The only nit remains the now-unused isAccessibleMode kept alive with //nolint:unused in manual_commit_reset.go — worth deleting rather than annotating, but not a blocker.

One caveat: this branch is based on an older main and doesn't include #1407, so GitHub CI could still hit the auth-date test depending on the runner's timezone. Worth merging main in (as you did for #1390) before trusting the CI signal. Want me to do anything else with it?

Verdict: merge. This is a clean, correct bug fix — one of the most straightforward of the eleven.

Issue is real: ToRelativePath (paths.go) and the two checkpoint metadata walkers (committed.go, temporary.go) used strings.HasPrefix(rel, "..") to detect path traversal. That predicate wrongly rejects legitimate repo-relative names that just happen to start with .. — e.g. ..generated/schema.json, which Git reports verbatim. Those files were silently filtered out of checkpoints. The conflation of "a .. parent segment" with "a filename beginning with .." is a genuine false-positive.

Implementation is good:

• New IsRelativeTraversal helper is segment-aware and cross-platform — checks exact .., ../, and ..\ (handling backslashes is correct here since ToRelativePath deals with MSYS/Windows paths).
• Does not reintroduce the documented sibling-directory bug (state_test.go / CLAUDE.md) — that's a caller concern (passing cwd instead of repoRoot), and this PR touches no callers, only the predicate. Genuine escapes like ../sibling are still rejected.
• The IsSubpath refactor onto the same helper is behavior-preserving.
• Tests cover the right cases (exact .., slash/backslash traversal, dot-dot-prefixed names, the walkers).

Only nit: the new checkpoint walker test uses git.PlainInit directly rather than testutil.InitRepo (CLAUDE.md prefers the helper), though it's read-only metadata-tree work so it's harmless.

Net: real bug, minimal correct fix, no security or sibling-dir regression. Merge (optionally clean up the test-init nit first).

Want me to check it out and run tests like the others, or move on?

Here's where everything stands. 3 merged, 8 still open.

✅ Already merged

🟢 Open — clean merge, ready to go

🟡 Open — merge with a minor nit

🔴 Open — don't merge as-is

Suggested order: the five 🟢 PRs can go in anytime (rebase/merge-main first where needed). #1396 and #1408 just need the noted check/eyeball. #1386 is the only one I'd hold for a design decision.

Want me to check out and run tests on any of the still-untested open ones (#1415, #1413, #1394, #1396, #1408)?