Consolidate hook system into internal/githook, fix early exit bug#300
Merged
Consolidate hook system into internal/githook, fix early exit bug#300
Conversation
Move all hook install/upgrade/uninstall logic from cmd/roborev/main.go and internal/daemon/server.go into a new internal/githook package, eliminating ~300 lines of duplicated code. Fix #298: hooks embedded in existing scripts (e.g. husky) now use a function wrapper inserted after the shebang instead of appending at the end, so they run before any `exit 0` in the existing hook. Key changes: - New internal/githook package with Install, InstallAll, Uninstall, NeedsUpgrade, Missing, and content generators - Embeddable snippets use _roborev_hook()/_roborev_remap() function wrappers with `return 0` instead of `exit 0` - embedSnippet() inserts after shebang, not at end of file - Bump version markers to v3/v2 to trigger upgrades - Auto-install missing/outdated hooks from `roborev review` (non-quiet) - Uninstall handles v3 function-wrapped format (} and return 0) - Integration test for husky-style early exit hook Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…edge case 1. initCmd now warns and continues (instead of failing) when a non-shell hook exists, so daemon setup still completes. 2. autoInstallHooks uses new NotInstalled() instead of Missing() so a completely absent post-commit hook is also auto-installed. Missing() only detects absent dependent hooks (post-rewrite) when post-commit already has roborev content. 3. embedSnippet normalizes the shebang line to end with a newline before concatenating the snippet, preventing corrupted output like "#!/bin/sh# roborev ..." when the input lacks a trailing newline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1. initCmd now only downgrades ErrNonShellHook to a warning; real install failures (permission, I/O) still abort init. Install() wraps the sentinel ErrNonShellHook so callers can use errors.Is() to distinguish it. 2. NotInstalled returns true only for os.IsNotExist errors; other read failures (permission, I/O) return false to avoid masking problems and triggering repeated auto-install attempts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
InstallAll now uses errors.Join to attempt all hooks instead of stopping at first error. autoInstallHooks silently suppresses ErrNonShellHook warnings. Added TestInitCmdWarnsOnNonShellHook and TestInitCmdFailsOnHookWriteError for init branch coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sking - autoInstallHooks uses Missing (not NotInstalled) so explicit uninstall-hook is respected — review only upgrades outdated hooks and installs companion hooks when roborev is already present. - Move autoInstallHooks after CLI argument validation so invalid flag combinations don't cause hook side effects. - Add HasRealErrors helper to distinguish joined errors containing only ErrNonShellHook from those with real failures. init now correctly fails when a non-shell warning is joined with an actual write error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Missing returns false on non-ENOENT read errors (e.g. permission denied, is-a-directory) instead of treating them as missing. - Add TestInitCmdFailsOnMixedHookErrors: non-shell post-commit + unwritable post-rewrite produces a fatal error, not a warning. - Add TestReviewInvalidArgsNoSideEffects: invalid flag combos (--branch --dirty) produce no hook writes or daemon contact. - Add TestMissing non-ENOENT subtest for the read-error edge case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
roborev: Combined Review (
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cmd/roborev/main.goandinternal/daemon/server.gointo a newinternal/githook/package, eliminating ~300 lines of duplicated coderoborev inithook fails when appended after hooks with early exits #298): embedded hook snippets are now function-wrapped and inserted after the shebang line instead of appended at the end, so husky/lint-staged hooks withexit 0no longer block roborevroborev review, without conflicting with explicituninstall-hookHasRealErrorshelper soinitcorrectly distinguishes non-shell hook warnings from real install failures in joined errorsautoInstallHooksafter CLI argument validation so invalid flag combos don't cause hook side effectsMissingandNotInstallednow return false on non-ENOENT read errors instead of triggering false positivesTest plan
go test ./internal/githook/...— 1390 lines of new package tests covering install, uninstall, embed, upgrade, version markers,HasRealErrors,Missing/NotInstallededge casesgo test ./cmd/roborev/...— updated CLI tests including mixed-error init, non-shell warning, review side-effect assertionsgo test -tags integration ./cmd/roborev/...— integration tests for init upgrade, early exit hook, hooks directory creationgolangci-lint run— 0 issuesgo vet ./...— clean🤖 Generated with Claude Code