Skip to content

cli: stamp agent hook configs and warn when they fall behind#1027

Open
gtrrz-victor wants to merge 21 commits intomainfrom
stale-hooks-warning
Open

cli: stamp agent hook configs and warn when they fall behind#1027
gtrrz-victor wants to merge 21 commits intomainfrom
stale-hooks-warning

Conversation

@gtrrz-victor
Copy link
Copy Markdown
Contributor

@gtrrz-victor gtrrz-victor commented Apr 24, 2026

Summary

Adds CLI version tracking to every agent's installed hook configuration, then surfaces a clear "your hooks are out of date" warning the next time the user runs any visible entire command.

Until now there was no signal anywhere — when a CLI release changed hook wiring, existing users kept running stale hooks silently until something failed cryptically. This PR closes that loop.

Example

Claude code settings:
Screenshot 2026-04-27 at 2 22 26 pm

Warning shown on every entire command:
Screenshot 2026-04-27 at 2 21 59 pm

What it does

1. Stamp every agent hook config with the CLI version that wrote it.

entire enable (and entire configure) now writes a top-level entireMeta.cli_version field into each installed agent's config:

  • .claude/settings.json
  • .cursor/hooks.json
  • .codex/hooks.json
  • .github/hooks/entire.json (Copilot CLI)
  • .factory/settings.json
  • .gemini/settings.json
  • .opencode/plugins/entire.ts (as a // entireMeta: {...} comment line — TS, not JSON)

A new agent.HookVersionSupport capability lets drift.go read the stamp back without knowing the concrete config format. JSON agents share WriteJSONHookMeta / ReadJSONHookMeta; OpenCode's TS plugin uses TSHookMetaCommentLine / ReadTSHookMeta.

2. Detect drift against a single global floor.

agent.CheckHookDrift(ctx) walks every registered agent with hooks installed and reports any whose stamp normalizes below agent.MinCompatibleCLIVersion. Missing/unreadable stamps are treated as v0.0.0, which collapses the "no stamp at all" and "stamp older than floor" cases through the same semver.Compare gate.

The floor is one package-level constant in cmd/entire/cli/agent/hook_command.go, seeded at "0.0.0" so no drift warnings fire today. Bump it exactly when a hook-contract change in any agent warrants forcing users to re-run entire enable --force.

Dev builds (versioninfo.Version == versioninfo.DevVersion) short-circuit drift detection entirely.

3. Surface the warning broadly and clearly.

A root-level PersistentPreRun (cli.driftWarningPreRun) prints a yellow two-line warning on stderr before every visible subcommand:

Action required: agent hooks need updating (claude-code)
  Run: entire enable --force

Action-first wording — no "stale", no abstract "hooks". The user knows what to run.

The sessions command defines its own PersistentPreRunE, which Cobra does not chain to root's, so sessions.go calls driftWarningPreRun explicitly after its git-repo guard.

Skip rules

The warning is suppressed when any of:

  • The target command (or any ancestor) has Hidden: true. Covers internal / machine-invoked commands (entire hooks, entire migrate, dev helpers).
  • The target is the root command itself (bare entire prints help or runs first-time setup; neither is the right surface).
  • The target is entire enable / entire configure invoked with --force=true. The user is already running the exact remediation the warning would suggest. The check inspects the resolved boolean (Flags().GetBool), not just flag.Changed, so a wrapper that passes --force=false explicitly still gets the warning.
  • The current directory is outside a git worktree. Otherwise a non-repo dir with a stray .claude/ could warn and then the command itself would bail with "not a git repository".
  • Stderr is not a TTY. Scripted / CI callers shouldn't have their stderr polluted.
  • The CLI is a dev build (covered by CheckHookDrift's own short-circuit).

Forcing a re-stamp

Two ways to upgrade an old install:

  • entire enable --force — explicit, the message tells the user this.
  • entire enable against a config with no stamp at all (a pre-stamp install) is automatically promoted to a force-reinstall, so the first enable after this lands re-stamps without ceremony.

Test plan

  • go test ./cmd/entire/cli/... -race — unit tests cover the skip predicate, the renderer, and driftWarningPreRun with stubbed drift / TTY / git-repo so every gate is exercised including the positive emit path.
  • mise run lint — 0 issues.
  • Manual drift cycle via pty in a fixture repo:
    • built CLI with Version=1.0.0 via ldflag, bumped MinCompatibleCLIVersion to 0.9.0, edited .claude/settings.json to stamp entireMeta.cli_version = "0.0.1-stale".
    • visible commands (entire rewind, entire status, entire sessions list): yellow two-line warning on stderr.
    • hidden commands (entire hooks claude-code pre-task): silent.
    • non-TTY stderr (entire rewind 2>/tmp/x): silent.
    • entire enable --force: re-stamps to 1.0.0, subsequent entire status is clean.
  • E2E canary (mise run test:e2e:canary) — kicked off when this comes out of draft.

🤖 Generated with Claude Code

gtrrz-victor and others added 13 commits April 24, 2026 17:09
Adds an `entireMeta.cli_version` stamp to every hook config file
written by `entire enable` (across all 7 agents: claude-code, codex,
copilot-cli, cursor, factoryai-droid, gemini, opencode). A new
`CheckHookDrift` compares each installed stamp against a single global
`MinCompatibleCLIVersion` floor; `entire status` and `entire enable`
surface stale installs and suggest `entire enable --force`.

Seeded at "0.0.0" so no drift warnings fire today — bump the constant
in hook_command.go when a future release breaks a hook contract.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 44058d39a9cc
A plain `entire enable` on a pre-stamp config previously wrote just the
stamp and returned, marking the install as current even though the hook
payload on disk was untouched. Once MinCompatibleCLIVersion rises above
"0.0.0", that would let drift silently clear without refreshing the
actual hooks.

Now each agent promotes missing-stamp to force=true, so the hook payload
is removed and re-added alongside the stamp write. Gemini's
legacy-cleanup test seeds a stamp to keep exercising the cleanup-only
path in isolation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 9901fadee2b6
- drift.go: hoist "dev" and "v0.0.0" to named consts (devVersion,
  zeroSemver) so goconst no longer flags the three repetitions
- claudecode: maintidx nolint on InstallHooks with the same rationale
  already used by factoryaidroid
- All seven agents' ReadHookMeta: annotate the "file missing → no
  stamp" return with //nolint:nilerr

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 75d489a446f2
- ReadHookMeta now returns (HookMeta, bool). Every implementation was
  returning nil error unconditionally, and the two-value signature makes
  the drift.go call site simpler.
- WriteJSONHookMeta errors are now wrapped at all six call sites with
  fmt.Errorf("stamp entireMeta: %w", err) to satisfy wrapcheck.
- Updated ReadHookMeta callers in claudecode tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 1889ba519a71
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Running entire status fired the warning twice — once from the root
PersistentPreRun (stderr), once from the inline status.go block
(stdout). Worse, entire enable --agent <name> bypasses the interactive
selection flow and skipped the shared helper entirely, so the inline
setup.go callsite never fired for that path. Narrow the skip list to
just 'status' (whose card renders its own, two-space-indented block)
and let PreRun cover enable, configure, rewind, and every other visible
subcommand. Fix card indent in status.go at the same time.
- Split emitStaleHooksWarning into staleHooksWarningLines(sty, drifts)
  + an emitter. The status card passes its own TTY-aware statusStyles
  so the warning renders yellow in the card, fixing a color regression
  where status piped drifts through a strings.Builder whose derived
  styles were no-ops.
- sessions.go's PersistentPreRunE previously shadowed root's
  driftWarningPreRun (Cobra does not chain PersistentPreRun hooks).
  Call it explicitly so `entire sessions *` stays covered.
- Introduce isTerminalWriterFn package-level var so a positive-path
  test can simulate TTY stderr without shuffling real fds. Adds a test
  that asserts the warning IS emitted when all gates pass.
- Refresh stale doc comments: drop the setup.go reference (no longer a
  caller) and the "not enable/configure" note (they're not skipped).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 24, 2026 15:52
@gtrrz-victor gtrrz-victor requested a review from a team as a code owner April 24, 2026 15:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Broadens hook-drift (“stale hooks”) visibility across the Entire CLI by adding a root-level warning path and a shared renderer, while extending hook config stamping/reading to support drift detection across agents (including OpenCode’s TS plugin).

Changes:

  • Wire a root PersistentPreRun (driftWarningPreRun) to emit a TTY-only stale-hooks warning for visible commands, and ensure entire sessions * still triggers it.
  • Add a shared warning renderer (staleHooksWarningLines / emitStaleHooksWarning) and integrate the warning into entire status output.
  • Introduce/extend hook metadata stamping + drift detection utilities (entireMeta.cli_version) across multiple agents, plus tests.

Reviewed changes

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

Show a summary per file
File Description
cmd/entire/cli/root.go Adds root PersistentPreRun to run drift warning logic broadly.
cmd/entire/cli/sessions.go Ensures sessions subtree still runs drift warning despite its own PersistentPreRunE.
cmd/entire/cli/status.go Integrates stale-hooks warning lines into status short output.
cmd/entire/cli/drift_warning.go Implements skip predicate, renderer, and pre-run emission logic.
cmd/entire/cli/drift_warning_test.go Adds unit tests for skip predicate, renderer, and pre-run behavior with injectable TTY/drift stubs.
cmd/entire/cli/agent/drift.go Adds drift detection across installed agents based on stamped CLI versions.
cmd/entire/cli/agent/drift_test.go Adds tests for semver normalization and dev-build short-circuit.
cmd/entire/cli/agent/hook_command.go Adds hook metadata types/helpers and the global compatibility floor constant/interface.
cmd/entire/cli/agent/hook_command_test.go Adds tests for JSON/TS hook meta read/write round-trips.
cmd/entire/cli/agent/capabilities.go Adds AsHookVersionSupport capability helper.
cmd/entire/cli/agent/claudecode/hooks.go Forces reinstall on missing stamp, stamps meta on install, adds ReadHookMeta.
cmd/entire/cli/agent/claudecode/hooks_test.go Adds tests asserting stamping/backfill/forced reinstall behavior.
cmd/entire/cli/agent/cursor/hooks.go Forces reinstall on missing stamp, stamps meta, adds ReadHookMeta.
cmd/entire/cli/agent/copilotcli/hooks.go Forces reinstall on missing stamp, stamps meta, adds ReadHookMeta.
cmd/entire/cli/agent/codex/hooks.go Preserves top-level keys while stamping meta; adds ReadHookMeta.
cmd/entire/cli/agent/geminicli/hooks.go Forces reinstall on missing stamp, stamps meta in writer, adds ReadHookMeta.
cmd/entire/cli/agent/geminicli/hooks_test.go Updates a test fixture to include a current stamp to keep focus on legacy cleanup.
cmd/entire/cli/agent/factoryaidroid/hooks.go Forces reinstall on missing stamp, stamps meta, adds ReadHookMeta.
cmd/entire/cli/agent/opencode/plugin.go Adds TS template placeholder for entireMeta JSON.
cmd/entire/cli/agent/opencode/entire_plugin.ts Adds // entireMeta: __ENTIRE_META_JSON__ stamp line to plugin header.
cmd/entire/cli/agent/opencode/hooks.go Substitutes meta placeholder during install; adds ReadHookMeta for TS plugin.

Comment thread cmd/entire/cli/agent/drift.go Outdated
Comment thread cmd/entire/cli/status.go Outdated
Comment thread cmd/entire/cli/drift_warning.go
Comment thread cmd/entire/cli/sessions.go Outdated
Comment thread cmd/entire/cli/agent/drift.go Outdated
gtrrz-victor and others added 2 commits April 24, 2026 18:48
Re-running `entire enable --force` is the exact remediation the warning
tells the user to run, so firing the warning during that invocation is
noise — the user is already doing what the warning would ask.

Adds `forceRemediationInFlight(cmd)` which returns true for
`entire enable` / `entire configure` when the `--force` flag has been
set by the user, and gates it inside driftWarningPreRun between the
command-name skip check and the TTY check.

Tests cover: --force skip for both enable and configure, and enable
without --force still emits the warning.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 7262fcb10426
- agent/drift.go: hoist the MinCompatibleCLIVersion == "0.0.0" bail-out
  above the per-agent loop so CheckHookDrift stops touching the
  registry and the filesystem entirely while the floor is unbumped.
  This runs on every visible command via PersistentPreRun; the
  per-iteration check previously did one AreHooksInstalled() read per
  installed agent before returning. (Copilot, Cursor Bugbot)
- drift_warning.go: skip root command (`entire`) so bare `entire`
  doesn't print the warning before help or first-time-setup output.
  (Copilot)
- sessions.go: run the WorktreeRoot check before driftWarningPreRun
  so `entire sessions *` outside a git repo fails cleanly without
  emitting a warning it is about to contradict. (Copilot)
- status.go: move the drift block out from under `if s.Enabled`. A
  repo with Entire disabled but stale hook configs on disk now still
  shows the warning on `entire status` — the only surface that case
  appears on, since root PreRun skips `status`. (Copilot)
- Refresh the drift_warning.go doc comment to match the new
  top-of-function bail semantics in CheckHookDrift.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: f04d3487654e
@gtrrz-victor gtrrz-victor marked this pull request as draft April 24, 2026 17:01
gtrrz-victor and others added 2 commits April 24, 2026 19:04
Two P3 fixes:

- forceRemediationInFlight now inspects the resolved --force value via
  Flags().GetBool rather than just flag.Changed. Wrappers that emit
  every flag (`entire enable --force=false`) mark the flag Changed but
  don't actually run the remediation — the warning must still fire.

- Add an inGitRepoFn gate (delegates to paths.WorktreeRoot) so the
  warning is skipped entirely outside a git worktree. The agent hook
  detectors fall back to `.` when WorktreeRoot fails, so a non-repo
  dir that happens to contain a stray `.claude/` or `.codex/` would
  otherwise warn and then the command would bail with "not a git
  repository" — misleading.

Tests cover both: --force=false still emits, outside-repo skips.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@gtrrz-victor
Copy link
Copy Markdown
Contributor Author

bugbot run

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

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

Reviewed by Cursor Bugbot for commit c179401. Configure here.

gtrrz-victor and others added 4 commits April 24, 2026 19:29
Remove `status` from the shouldSkipDriftWarning list and drop the
inline drift block + staleHooksWarningLines helper. PersistentPreRun
now fires for `entire status` like every other visible command:
warning prints yellow on stderr above the status card (stdout), one
render path, no buffer-routed statusStyles inference, no indent
fixup. Tradeoff accepted: piping `entire status > file` no longer
captures the warning, but the user still sees it in the terminal.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rt-circuit

The explicit floor short-circuit (`if normalizeSemver(required) ==
zeroSemver { return nil }`) and the separate `Missing` field on
DriftReport existed only because the missing-stamp branch returned a
report unconditionally — which would have warned even when the floor
was unbumped (0.0.0).

Treat a missing/unreadable stamp as installed = "" (which
normalizeSemver coerces to "v0.0.0") and feed it through the same
semver.Compare gate as a present-but-old stamp:

- floor "0.0.0", missing stamp → Compare("v0.0.0", "v0.0.0") < 0
  is false → no report (correct).
- floor > "0.0.0", missing stamp → Compare("v0.0.0", floor) < 0
  is true → report (correct).
- floor > "0.0.0", old stamp → existing semver compare reports.

Drops the explicit short-circuit, the `zeroSemver` constant
(replaced by inline "v0.0.0" string in the two normalizeSemver
fallbacks), and the `Missing` bool from DriftReport.

Tradeoff: visible commands now do per-installed-agent FS reads
(one small JSON file each) on every invocation, vs the previous
constant-time bail when the floor was 0.0.0. Cost is bounded
(~7 agents max) and the dev-build short-circuit still covers the
local-development hot path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Aggregated findings from a parallel reuse/quality/efficiency review:

- versioninfo: export DevVersion sentinel; replace the local devVersion
  const in agent/drift.go and the bare "dev" literal in
  versioncheck.go. One source of truth for the dev-build sentinel.
- drift.go: comment why we discard the ok bool from ReadHookMeta — the
  zero HookMeta normalizes to "v0.0.0" and feeds the same comparison,
  which is the correct behavior; the discard isn't an oversight.
- drift_warning.go: extract staleHooksHeader/staleHooksFix constants so
  tests assert against the same source of truth as the renderer.
- drift_warning.go: tighten redundant doc comments (inGitRepoFn,
  shouldSkipDriftWarning rules) — the code already says what they
  restated.
- drift_warning_test.go: extract a `withTTY(t)` helper, replacing five
  near-identical 3-line blocks across the subtests.
- sessions.go: rewrite the misleading PersistentPreRunE comment — it
  claimed the call ordering protected against double-warning, but
  driftWarningPreRun has its own inGitRepoFn guard so the order is
  fine for a different (simpler) reason.

No behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@gtrrz-victor gtrrz-victor marked this pull request as ready for review April 27, 2026 12:32
@gtrrz-victor gtrrz-victor changed the title cli: broaden stale-hooks warning reach and clarify wording cli: stamp agent hook configs and warn when they fall behind Apr 27, 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