Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
entire-brain moves Entire's pre-push hook to pre-push.pre-entire-brain and puts its own hook in front, which execs that copy with git's stdin and arguments. The hook in pre-push lacks Entire's marker, so the hook state read Absent: `entire doctor` reported "Git hooks: NOT INSTALLED" on a working setup and offered `--force`, and EnsureSetup re-wrapped Brain's hook on the next agent turn. When pre-push is Brain's wrapper, classify the moved copy it runs instead: a current copy reads Current, an old one Outdated, and anything that is not Entire's hook still reads Absent. Brain's wrapper only counts when it is a runnable regular file in pre-push whose marker, backup= assignment and exec line each appear as whole lines, and the copy it runs is a regular file with an execute bit (not checked on Windows). installHooks leaves Brain's wrapper alone and refreshes the copy it runs in place, chained to the user's own pre-push backup when there is one, but only when that copy reads Outdated or is exactly the hook Entire writes today; a copy the user has added lines to is left as it is. It also refreshes the other moved copies of Entire's hook first, so a hook manager's wrapper that runs an older copy cannot chain back into pre-push.pre-entire forever. In every case the wrapper does not qualify, install takes the existing re-wrap path.
727ff09 to
5e67636
Compare
|
@peyton-alt @Soph a heads-up on this PR: I pushed one more change (now |
Fixes #2949
@peyton-alt @Soph: I asked on the issue to be assigned (#2949 (comment)) and haven't had a reply yet. I'm opening this so it can be reviewed. If you'd rather fix it on the Brain side instead (giving Brain's wrapper a marker
doctorrecognizes), tell me and I'll close this.Why
entire brain connectinstalls its ownpre-pushhook. It moves the hook it finds (Entire's) topre-push.pre-entire-brain, and its own hook ends with:So Entire's hook still runs, with git's stdin and arguments. But the new
pre-pushdoesn't have the# Entire CLI hooksmarker, sohookStateIntreats it as foreign and reports the whole set asGitHooksAbsent. I checked what happens next with the real plugin (brain v0.4.0, see Verification):entire doctorshowsGit hooks: NOT INSTALLED/ "Commits in this repository are not captured as checkpoints" on a setup that works, and suggests--forceto fix it.EnsureSetupruns on every agent turn and reinstalls wheneverIsGitHookInstalledis false. On the first turn afterbrain connect, it backs Brain's hook up topre-push.pre-entire, puts Entire's hook in front, and prints[entire] Your pre-push hook still runs, after Entire's (moved to .git/hooks/pre-push.pre-entire).into the agent's hook output. After that, doctor says OK again.Pushes keep working after that re-wrap. Entire's new
pre-pushruns Entire, then Brain's hook frompre-push.pre-entire. Brain's$0-relative chain then looks forpre-push.pre-entire.pre-entire-brain, which doesn't exist, so it stops there (Brain's source says that's deliberate, so the host's hook doesn't run twice). The old Entire copy inpre-push.pre-entire-brainis left behind and nothing runs it. So the bug users see is doctor's false report (and its--forceadvice) betweenbrain connectand the next agent turn, plus a re-wrap that isn't needed. Commits and pushes were always captured.The fix
The changes are in
cmd/entire/cli/strategy/hooks.go, plus askipparameter onrefreshMovedEntireHooksinhook_backup.go.hookStateIn: ifpre-pushdoesn't have Entire's marker but is Brain's wrapper, it classifies the copy the wrapper runs (pre-push.pre-entire-brain) the same way it classifiespre-pushtoday:GitHooksCurrent, and doctor says OK.GitHooksOutdated, and it gets reinstalled (see below).GitHooksAbsent, as before. That includes: no copy; a copy that isn't Entire's; a symlink in either place (still read NoFollow); a wrapper git won't run, or a copy Brain's[ -x ]guard skips, because it has no execute bit (not checked on Windows, which has no execute bit); a wrapper whoseexecline was removed or commented out; and Brain's wrapper in any hook other thanpre-push.Brain's wrapper only counts when its marker (
# entire-brain push hook), thebackup="$0.pre-entire-brain"line and theif [ -x "$backup" ]; then exec "$backup" "$@"; filine each appear as whole lines. Any other wrapper without Entire's marker still reads Absent, so the existing reclaim path for husky, lefthook and other hook managers doesn't change.installHooks: whenpre-pushis Brain's wrapper and the copy it runs is Entire's (same check), it writes Entire's hook into that copy and leaves the wrapper alone, instead of backing the wrapper up topre-push.pre-entire. If Entire had already backed up the user's ownpre-pushtopre-push.pre-entirebefore Brain was installed, the copy gets the chained hook, the same content Entire would write at the hook path. Its$(dirname "$0")-relative chain still finds the user's hook from there. Before writing that copy, it refreshes any other moved copies of Entire's hook to the hook that never chains, the same way the normal install path does. Without that, a manager's wrapper inpre-push.pre-entirethat runs an older Entire copy could chain back topre-push.pre-entireforever. This covers an agent turn with a stale copy (Outdated) and a re-run ofentire enable. The copy is only rewritten when it reads Outdated or is exactly the pre-push hook Entire writes today (chained or not, for the prefix its own line runs). If the user added lines to it, it's left as it is, onenableand on agent turns, the same ruleTestCheckGitHookState_UserAdditionsAreNotDriftand #2878 set for Entire's own hook: it still reads Current, so doctor says OK and nothing re-wraps. Before this change, both re-wrapped Brain's hook. In every Absent case above, install still takes the old re-wrap path, which puts Entire's hook in front where it runs.I also updated the
ReinstallGitHooksdoc comment, which said a wrapper without the marker always reads Absent.The diff is 2 production files (+135/-5) and 2 test files (+289).
Overlap: #2640, #2643 and #2388 (still open) touch the same functions in
strategy/hooks.go. None of them covers the Brain layout. I rangit merge-treeagainst each PR's head: each already conflicts with currentmain, and this branch adds no new conflicting files or hunks (same files, 3/6/3 hunks with or without this branch).Tests
strategy/hooks_test.goTestCheckGitHookState_BrainPrePushWrapper(table, parallel, filesystem-only):execline commented out reads AbsentTestCheckGitHookState_BrainWrapperOnlyCountsForPrePush: Brain's wrapper inpost-commit's place reads Absent.TestInstallGitHooks_BrainPrePushWrapperRefreshesOtherMovedCopies: Brain's layout, plus a manager wrapper inpre-push.pre-entirethat runspre-push.old, an Entire hook from before the chain guard that chains straight back. AfterInstallGitHook,pre-push.oldis the hook that never chains, the copy Brain runs is chained, and both wrappers are unchanged.TestInstallGitHooks_KeepsBrainPrePushWrapper: realInstallGitHook, with and without the user's ownpre-pushthere first, then Brain's layout with a stale moved copy. BothReinstallGitHooks(agent turn) andInstallGitHook(enable) leave the wrapper byte-for-byte. They rewrite the copy to the current hook (chained when the user's hook is inpre-push.pre-entire) and leave the user's hook where it was, or create nopre-push.pre-entirewhen there wasn't one. Afterwards the set reads Current.TestInstallGitHooks_BrainCopyKeepsUserAdditions: Brain's layout with a line the user added to the copy Brain runs (current hook, chained current hook, hook from before the ref list, current hook missing the chain).InstallGitHook(enable) andReinstallGitHooks(agent turn) leave the copy byte-for-byte and the set reads Current. A copy that is exactly Entire's hook is still rewritten: from before the ref list or run from a working tree (Outdated, both paths), and today's hook for another prefix or without the chain to the user's hook (enable).TestInstallGitHooks_BrainCopyWithUserLineStillRefreshesOtherMovedCopies: the layout from the test above, but the copy Brain runs has a line the user added.InstallGitHookkeeps that line and still rewritespre-push.old, so it can't chain back.doctor_test.goTestCheckGitHooks_BrainPrePushWrapper: a realInstallGitHook, then the Brain layout. AssertscheckGitHooksprints✓ Git hooks: OKboth without and with--force, and that Brain's wrapper is left byte-for-byte as it was.The wrapper text in the tests matches what the real brain v0.4.0 writes, except for the plugin path (compared against a real install, see below).
Failing without the fix
The new tests at the branch head, with
hooks.goandhook_backup.goreverted tomain@ 38ef300:(1 =
GitHooksCurrent, 2 =GitHooksOutdated, 0 =GitHooksAbsent.)TestInstallGitHooks_BrainCopyKeepsUserAdditionsfails on the first version of this PR (727ff09) in its fourenablecases with a user line, andTestInstallGitHooks_BrainCopyWithUserLineStillRefreshesOtherMovedCopiesfails there too: the copy comes back as Entire's template with the line gone. Its agent-turn cases already passed there, because a copy with the user's lines reads Current and an agent turn only rewrites stale hooks. Its exact-template cases fail if the "exactly today's hook" check is removed. The Absent cases pass onmaintoo, becausemainreads every Brain layout as Absent. They're there to keep the new check from accepting a layout that doesn't run Entire's hook. Each one fails withexpected: 0, actual: 1without its check: each with only its own check removed (the moved-copy execute bit, the exec line, the wrapper execute bit, and thehook != prePushHookcondition for thepost-commitcase). Everything passes at the branch head.Verification
Linux amd64, git 2.56.0, go 1.27.3, mise 2026.10.21, golangci-lint 2.15.0. Clean environment:
env -iwith only HOME/PATH/Go cache variables, and no agent, plugin or telemetry variables. The real-plugin runs also used a throwaway HOME, config dir and cache.At the branch head (5e67636, on
main@ 38ef300):mise run fmt: no changes.mise run lint:0 issues, and gofmt, gomod, shellcheck, secretpatterns and mise all passed.mise run test:ci: 1 failure,cmd/entire/cli/apiTestClient_Get_NamesUnresolvableHost. It expects a DNS failure forentire-netcause-test.invalid, and my sandbox's resolver returns something else. Cleanmain@ 38ef300 gives the same result: the same single failure, with the same 92 packages ok. Because that step fails, the task stops before the canary, so I ranmise run test:e2e:canaryseparately. It passed.mise run test: 16584 tests, the same 1 failure.GOOS=windows go test -c ./cmd/entire/cli/strategy/compiles. CI on my fork passed (test, test-windows, integration a/b/c, canary, canary-windows, lint, CodeQL, govulncheck).controlplanefailed there because the fork doesn't have the E2E secrets.Real Brain, end to end. I installed the real release (
entire plugin install brain, v0.4.0) into a temp HOME, then ranentire enable -y --agent claude-codein a fresh repo, thenentire brain connect --repo-id … --api-url https://127.0.0.1:9. No account: connect writes the hook before its first sync, and the sync fails with "Not logged in". Brain's hook came out as quoted above, with Entire's hook inpre-push.pre-entire-brain. Then:entire hooks claude-code user-prompt-submit(what.claude/settings.jsonruns; it callsEnsureSetup). On main, Brain's wrapper moved topre-push.pre-entireand the[entire] Your pre-push hook still runs…line printed. On this branch,pre-pushstayed byte-for-byte Brain's wrapper and nothing printed.pre-push.pre-entire-brain, thenentire enable, an agent turn andentire doctor --force: the line is still there after each, doctor says✓ Git hooks: OK, and a push runs Entire'shooks git pre-push originonce and then the user's line. On 727ff09,entire enabledropped the line.git pushto a local bare remote through Brain's wrapper, with a stubentireon PATH that logs its calls: Entire'shooks git pre-push originran exactly once, and Brain'smemory notifywas launched.pre-push.pre-entire-brain), then an agent turn: doctor saysOUT OF DATE, the turn rewrites the copy to the current hook, and Brain's wrapper stays. Runningentire enableagain also leaves the wrapper in place.chmod -x pre-push.pre-entire-brain, Brain'sexecline commented out, orchmod -x pre-push(git then skips the hook withhint: The '.git/hooks/pre-push' hook was ignored…): doctor saysNOT INSTALLEDand the turn re-wraps, so Entire's hook runs on the next push, same as main.pre-pushfirst, thenentire enable,brain connect, andentire enableagain: a push runs Entire's hook and then the user's, both before and after the second enable. (On main, the second enable re-wraps Brain's hook and rotates the user's hook topre-push.pre-entire.<timestamp>, which no longer runs, with a warning.)pre-push.pre-entirethat runs an Entire copy from before the chain guard (pre-push.old), thenentire enable: the next push finishes, withentire hooks git pre-pushcalled twice, once from the copy Brain runs and once from the refreshedpre-push.oldthrough the manager's wrapper.What I didn't run:
Windows|MSYS, so none of these tests run there. The new code is aLstatthrough the existingos.Rootplus string matching, and it skips the execute-bit check on Windows.entireto count calls.Known limits / open questions
doctorunderstands. I asked which you prefer on the issue and haven't heard back yet.entire disable --uninstallleaves Entire's hook running behind Brain's wrapper, on this branch and onmainafter its re-wrap.restoreWrappedmatches wrappers by literal file name, and Brain uses$0. That bug was already there before this PR and is filed separately asentire disable --uninstallleaves Entire's pre-push hook running behind entire-brain's wrapper #2957.maintoday, and I reproduced both on cleanmain. (1) When the user's own hook is inpre-push.pre-entirebehind Brain's layout, the re-wrap rotates it topre-push.pre-entire.<timestamp>, where nothing runs it. Onmainthat happens on the first agent turn afterbrain connect. With this PR it only happens when Brain's wrapper has lost its execute bit, which still takes the existing re-wrap path. (2) Refreshing moved Entire copies (refreshMovedEntireHooks) rewrites them from the template and silently drops lines a user added. Onmainthat includespre-push.pre-entire-brain. This PR no longer does that topre-push.pre-entire-brain(see The fix), but other moved copies such as a hook manager'spre-push.oldare still refreshed as onmain. Separately, if the user's own hook lands inpre-push.pre-entireafter they edited the copy Brain runs, Entire doesn't add the chain to that copy; adding the chain would mean rewriting their file. I'd rather handle both as separate issues than grow this PR.Missing Entire-Checkpoint trailers
None of these commits has an
Entire-Checkpointtrailer. CONTRIBUTING asks for them, but this work wasn't done in an Entire-captured session that produced one, and I won't make one up. If you need checkpoint linkage, I can redo the commits inside a captured session.