Skip to content

refactor(hooks): stage absolute_git_hook_path scoping, and detect a dead pin - #2085

Open
Soph wants to merge 3 commits into
mainfrom
soph/absolute-hook-path-scope
Open

refactor(hooks): stage absolute_git_hook_path scoping, and detect a dead pin#2085
Soph wants to merge 3 commits into
mainfrom
soph/absolute-hook-path-scope

Conversation

@Soph

@Soph Soph commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Two residuals from the local_dev removal (#1999), both about absolute_git_hook_path — the setting that makes git hooks embed an absolute binary path instead of bare entire, for GUI git clients that never source a shell profile.

The scope change is staged. Phase one warns and migrates; a later release flips it. See below for why a hard break was wrong.

Why staged

Verified against the pre-change binary from main:

$ entire configure --absolute-git-hook-path
✓ Settings updated (.entire/settings.json)
project: {"enabled":true,"absolute_git_hook_path":true}
local:   (none)

The documented way to enable the feature wrote it exclusively to the tracked file. So the affected population isn't an edge case — it's every user of it.

And the failure mode is the worst available: the setting exists because their GUI git client can't find entire on PATH. Unpinning means the guard takes else : there, capturing nothing, with no error. They'd lose checkpoints in exactly the tool they enabled it for.

This is a robustness fix, not a security one — the value only chooses between entire and os.Executable(), a binary the user already invoked. There's no case for breaking anyone quickly.

Phase one (this PR)

  • The committed value is still honored. Nobody's hooks change.
  • It reports a deprecation, in entire status and entire doctor.
  • doctor offers the migration, reusing the confirm/--force/non-interactive degradation already here.
  • New writes already go local, so the population stops growing now rather than at phase two.

Copy, not move. Editing a tracked file would land an unexpected change in the user's next commit. Once the local file sets the key the committed one is redundant, so it simply stops mattering when the scope is retired — and the warning goes quiet at that point, which the "no warning once the local file sets the key" case pins. Verified git status shows no tracked-file change after migrating.

Phase two (a release after this reaches stable)

recordAbsoluteGitHookPathDeprecation becomes the enforcing version and the table in absolute_hook_path_scope_test.go flips. Silent for anyone who ran doctor.

A dead pin failed silently

An upgrade into a new version directory, a binary built in a temp dir, a go install that relocates it — the guard takes its else branch, so git keeps working while nothing is captured. Verified before fixing:

$ git commit -m "after the binary moved"
Entire-Checkpoint trailers: 0
$ entire doctor
✓ Git hooks: OK          ← reported healthy

Now GitHooksOutdated, which makes it self-healing: reinstalling re-pins to the running binary. Nothing revalidated a pinned path after install before this. This helps the same population as the staged change above.

That gave GitHooksOutdated two causes needing different advice, so the check reports which — collapsing them told a user with a moved binary that their hook "runs Entire from the working tree", which is false and points at the wrong fix.

Found by /simplify, all reproduced first

  • The flag wrote the key into the committed file. The struct assignment stayed and saveSettingsToTarget marshals the whole struct to the target, which defaults to project. My own earlier check missed it because the fixture already contained the key.
  • configure --force decided pinning from an ungated read. settings.LoadFromFile reads one file verbatim, so it both missed a local-only value and honored a committed one directly. Now reads the merged view; LoadFromFile's doc says it's display-only.
  • The pin parser was wrong: strings.Index(rest, " ]") matched inside a path, so /Users/a ]b/entire parsed as '/Users/a — a permanent false "outdated" plus a reinstall every turn-start that could never repair it.
  • Nothing coupled the parser to the generator. Every existing test installed with absolutePath=false, so changing the guard would have silently disabled dead-pin detection with the suite green. Verified by swapping [ -x for test -x and watching the new round-trip test fail. Fixtures now come from buildHookSpecs, which also deleted a tc.name branch inside a table test.

Plus: one pass over the invocation line instead of two and one os.Stat instead of five; the mid-loop continue removed (it sat below two Absent returns, so a future Absent check would have been skipped for hooks 2–5); setEnabledRaw generalized to setRawKey, restoring a doc comment an earlier edit had orphaned and carrying a nil-map guard; provenance from rawHasKey rather than call order; three exported entry points → two; the settings test moved to loadMergedSettings + t.Parallel like its sibling (0.21s → 0.00s, no git subprocess per case).

Deliberately not done

  • saveToFile stripping machine-local fields. Tempting as a single choke point, but while the committed value is still honored it would delete a working setting out of a tracked file as a side effect of an unrelated entire configure. Revisit at phase two, when the key is inert.
  • A declarative per-field scope registry. Two gates with different verification depth and failure policy aren't a table's worth, and there's no seam to hang it on.
  • Unified rejection reporting across the three existing shapes, and adding it to status --json. That's where a fourth such field's cost lands, but it's a separate change.
  • A machine-readable pin marker in the hook format, which would remove the parse coupling entirely — needs a transition release, and the round-trip test buys the protection now.

Testing

mise run fmt && mise run lint && mise run test:ci pass. Every new guard was verified to fail against the defect it protects against.

Verified end to end with a built binary: an existing user's hooks stay pinned and they get a warning with a one-command fix; doctor --force migrates with no tracked-file change; the warning then stops and the hooks are still pinned; a moved binary is diagnosed with the right message and repaired.

Two residuals from the local_dev removal, both about the same setting.

absolute_git_hook_path replaces bare "entire" in every generated git hook with
the absolute path of the binary that ran `entire enable`. It was merged from any
scope including the committed project file, so a repository could impose it on
everyone who cloned — pinning their hooks to whatever binary they happened to
run, which is strictly more brittle than resolving through PATH. Reproduced: a
committed `absolute_git_hook_path: true` pinned the hook to a scratchpad build.

It is now honored only from .entire/settings.local.json, following the existing
enforceOPFCommandTrust pattern. Unlike that gate this does not re-verify the local
file with the deep index-and-HEAD check, and the difference is deliberate: the OPF
command becomes argv[0] of an exec, while this only chooses between "entire" and
os.Executable() — a binary the user already invoked — so the worst outcome is a
fragile hook, not running someone else's payload.

The write side had to move with it. The default target is the project file
whenever one exists, so `entire configure --absolute-git-hook-path` would have
written to a file the loader now ignores: the hook would be pinned by that run and
silently revert on the next EnsureSetup. The flag now writes only that key to the
local file, whatever scope the command is otherwise writing.

Second: a pinned path that stops resolving — an upgrade into a new version
directory, a binary built in a temp dir, a `go install` that relocates it — failed
silently. The guard takes its else branch, so git keeps working while nothing is
captured, and `entire doctor` reported "✓ Git hooks: OK". Verified: zero
Entire-Checkpoint trailers while doctor called the hooks healthy. A dead pin now
reads as GitHooksOutdated, which makes it self-healing, since reinstalling re-pins
to the binary now running.

That gave GitHooksOutdated two causes needing different advice, so the check now
reports which one. Collapsing them told a user with a moved binary that their hook
"runs Entire from the working tree" — false, and pointing at the wrong fix.
CheckGitHookState keeps its signature; CheckGitHookStateWithReason is additive.

A rejection is only reported when it changed the outcome: if the local file
enables the setting anyway, the project value was redundant rather than
overridden, and saying "ignored" beside a hook that is in fact pinned reads as a
contradiction. Surfaced in `entire status` and `entire doctor`, not logged — the
settings loader is reached from logging.Init, and a line in .entire/logs is not a
signal anyone sees.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 20, 2026 12:57
@Soph
Soph requested a review from a team as a code owner August 20, 2026 12:57

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 1711857. Configure here.

Comment thread cmd/entire/cli/setup.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR improves Git hook health reporting by distinguishing why hooks are considered outdated, and enforces absolute_git_hook_path as a machine-local-only setting (with user-visible feedback when rejected from the committed settings file).

Changes:

  • Add hook-state “reason” plumbing and update doctor output to show actionable, cause-specific guidance.
  • Detect and report drift when hooks are pinned to a missing absolute binary path.
  • Enforce absolute_git_hook_path scope to .entire/settings.local.json, including new tests and status/doctor messaging.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
cmd/entire/cli/strategy/hooks.go Adds CheckGitHookStateWithReason, dead-pin detection, and pinned-path parsing helpers.
cmd/entire/cli/strategy/hooks_test.go Updates hook-state tests for new return signature; adds tests for dead pinned paths and reason strings.
cmd/entire/cli/doctor.go Displays absolute_git_hook_path rejection and prints cause-specific outdated reasons.
cmd/entire/cli/status.go Surfaces absolute_git_hook_path rejection in detailed status output.
cmd/entire/cli/setup.go Writes absolute_git_hook_path to local settings and emits messaging about where it was written.
cmd/entire/cli/settings/settings.go Drops committed-scope absolute_git_hook_path, records rejection reason, and exposes it via accessor.
cmd/entire/cli/settings/absolute_hook_path_scope.go Implements scope enforcement and rejection reason for committed absolute_git_hook_path.
cmd/entire/cli/settings/absolute_hook_path_scope_test.go Adds tests pinning the intended scope/rejection and guarding serialization behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cmd/entire/cli/setup.go Outdated
Comment thread cmd/entire/cli/strategy/hooks.go Outdated
Comment thread cmd/entire/cli/strategy/hooks.go Outdated
Comment thread cmd/entire/cli/strategy/hooks.go Outdated
Soph and others added 2 commits August 20, 2026 15:47
… generator

Review of the previous commit found the scope gate enforced one layer too high:
both of absolute_git_hook_path's real consumers reached around the merged loader
it gates. Two live bypasses, each reproduced before fixing.

The flag wrote the key into the committed file. The struct assignment stayed and
saveSettingsToTarget marshals the whole struct to the target, which defaults to
the project file whenever one exists — so `entire configure
--absolute-git-hook-path` announced "written to settings.local.json" and put it
in settings.json too. One commit later, every cloner's doctor reports a warning
our own writer created. My earlier end-to-end check missed it because the fixture
already contained the key.

Fixed at the writer rather than the three call sites: saveToFile now strips
machine-local fields from a project-scope save, so no caller can leak them by
forgetting. setEnabledFlag's doc already named this field as the canonical
local-only override; ~8 tests assert it, all seeding the local file, so none
covered the flag path. The pre-existing flag test asserts only the merged view
and passed either way.

`entire configure --force` honored a committed pin. updateGlobalSettings loads
its target with settings.LoadFromFile, which applies no scope gating, and fed
that to InstallGitHook — reaching the same imposition actively that the loader
blocks passively. The decision now comes from the merged view, as
setupAgentHooksNonInteractive already did. LoadFromFile's doc now says it is
display-only.

The pin parser was wrong and untested against the generator. `strings.Index(rest,
" ]")` matched inside a path, so /Users/a ]b/entire parsed as '/Users/a — a
permanent false "outdated" plus a reinstall every turn-start that could never
repair it. Cut at "; then" and take the last delimiter instead. Worse, every
existing test installed with absolutePath=false, so nothing installed a pinned
hook and read it back: changing the guard would have silently disabled dead-pin
detection with the suite green. Added that round-trip, and test fixtures now come
from buildHookSpecs, which also removed a branch on tc.name inside a table test.

Also from review: one pass over the invocation line instead of two, and the same
path stat'ed once instead of once per hook; the mid-loop continue replaced (it sat
below two Absent returns, so a future Absent check would have been skipped for
hooks 2-5); setEnabledRaw generalized to setRawKey, restoring the doc comment my
insertion had orphaned and carrying a nil-map guard my writer omitted; the
rejection computed once from the final state instead of set-then-clear 37 lines
apart; reasons single-line, dropping doctor's split loop; three exported state
entry points collapsed to two; GitHooksOutdated's doc updated, since a dead pin is
a shape we still write; the rejection notice moved behind the never-opted-in guard
so it no longer fires in repos the check deliberately stays silent about; and the
settings test moved to loadMergedSettings + t.Parallel like its sibling, 0.21s to
0.00s with no git subprocess per case.

Flag help and README now state the local-only rule; it existed only in a Go
comment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Turning the scope rule on immediately would have broken everyone who uses the
feature, silently and in the worst possible place.

Verified against the pre-change binary: `entire configure
--absolute-git-hook-path` reported "Settings updated (.entire/settings.json)" and
created no local file at all. The documented way to enable the feature wrote it
exclusively to the tracked file, so the affected population is not an edge case —
it is every user of it. And the setting exists because their GUI git client cannot
find `entire` on PATH, so unpinning means the guard takes its else branch there
and captures nothing, with no error. They would lose checkpoints in exactly the
tool they enabled it for.

This is a robustness fix, not a security one — the value only chooses between
"entire" and os.Executable(), a binary the user already invoked — so there is no
case for breaking anyone quickly.

So phase one: the committed value is still honored, and reports a deprecation
instead. `entire doctor` offers to copy it to the local file, reusing the
confirm/--force/non-interactive degradation already here. Copy rather than move:
editing a tracked file would land an unexpected change in the user's next commit,
and once the local file sets the key the committed one is redundant — it simply
stops mattering when the scope is retired. The warning goes quiet at that point,
which the "no warning once the local file sets the key" case pins.

Phase two, a release after this reaches stable, stops honoring the project scope:
recordAbsoluteGitHookPathDeprecation becomes the enforcing version and the table
in absolute_hook_path_scope_test.go flips.

New writes already go local, so the population stops growing now rather than at
phase two. Provenance now comes from the local file's raw keys via the package's
own rawHasKey rather than from call order, which also removes the two-site
set-then-clear the review flagged.

Dropped the saveToFile stripping from the previous commit: while the committed
value is still honored, stripping it there would delete a working setting out of a
tracked file as a side effect of an unrelated `entire configure`. The three call
sites keep it off the struct instead, which is what the review suggested first.

Everything non-breaking from the previous commit stands: the dead-pin detection
(which helps this same population), the parser fix, the generator round-trip test,
setRawKey, and the merged-view fix for the hook-install decision — retargeted to
assert what it actually guarantees, that a local-only value is honored, since a
committed one legitimately still is in phase one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Soph Soph changed the title fix(hooks): scope absolute_git_hook_path locally and detect a dead pin refactor(hooks): stage absolute_git_hook_path scoping, and detect a dead pin Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants