Add review hooks system (#180) #181
Merged
+1,141
−35
Conversation
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
Configurable hooks fire shell commands when reviews complete or fail.
Supports {var} template interpolation and a built-in "beads" type that
auto-creates bd issues for failed reviews.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address review #3698 findings: - Copy global hooks slice before appending repo hooks to prevent aliasing - Store subscription ID and unsubscribe on Stop to prevent broadcaster leak - Shell-escape interpolated template variables to prevent injection - Add tests for all three fixes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address review #3699: add TestInterpolateQuotedPlaceholders covering double-quoted and empty-value behavior. Update hooks guide to note that placeholders are auto-escaped and should not be manually quoted. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- TestHookRunnerGlobalAndRepoHooksBothFire: both fire for same event - TestHookRunnerRepoOnlyHooks: repo hooks fire with no global hooks - TestHookRunnerRepoHookDoesNotFireForOtherRepo: repo hooks scoped correctly Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address review #3707: - Move repoMarker to repoDir instead of globalDir (copy-paste fix) - Add writeRepoConfig helper that fatals on os.WriteFile error - Replace fixed sleep in negative tests with polling assertFileNotCreated Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- runHook uses cmd /C on Windows instead of sh -c - shellEscape uses double-quote escaping on Windows - Tests use touchCmd/pwdCmd helpers for cross-platform commands Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This was referenced Jan 31, 2026
Closed
Tests now use q() helper (wraps shellEscape) instead of hardcoded single-quote literals, so they pass on both Unix and Windows. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
TestInterpolateShellInjection now tests multiple injection payloads and asserts they are properly quote-enclosed rather than comparing against shellEscape output (which was testing the function against itself). Also adds injection payloads to TestShellEscape. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Display the daemon version in `roborev status` output so it's easy to tell if the running daemon matches the installed binary. Update the built-in beads hook for failing reviews to include a `roborev fix` hint alongside `roborev show`. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add a log line when hooks fire so daemon logs show hook activity. Add unit tests for logging behavior (fires/no-match). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
IsTaskJob() incorrectly classified branch reviews (commit ranges with ".." in git_ref) as task jobs, skipping verdict parsing. Add the range check and unit tests for IsTaskJob. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Restore previous log.Writer() instead of hardcoded os.Stderr in hook log tests. Add regression test proving verdict is parsed for branch range reviews (the user-visible bug). Add additional IsTaskJob test cases. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add length/prefix guards before slicing in injection and escape tests to fail cleanly instead of panicking on regressions - Assert payload content is preserved inside the quoted region - Escape % in Windows shellEscape to prevent env variable expansion - Use cmd /D /C on Windows to disable AutoRun registry entries - Refactor shellCommand to return arg slice for multi-flag support Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Restore the global logger immediately after handleEvent returns (before async goroutines finish) instead of via t.Cleanup, which runs after the test function exits. The "fired N hook(s)" log line is written synchronously by handleEvent, so the buffer is safe to read before restoring. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace 'type nul > path' with 'copy nul path' in touchCmd helper since cmd.exe handles quoted paths more reliably with copy. Inline platform shell logic in runHook with /D flag to disable AutoRun registry entries on Windows. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
cmd.exe's redirection and copy commands fail with quoted paths containing short names (8.3 format). Switch to PowerShell for touchCmd and pwdCmd helpers which handle all path formats correctly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
cmd.exe's path quoting with 8.3 short names is unreliable for creating files via redirection. Skip tests that spawn shell processes on Windows; the unit tests (matchEvent, interpolate, shellEscape, beadsCommand) cover all hooks logic without needing shell execution. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
cmd.exe's quoting and redirection are unreliable with 8.3 short paths.
Switch to PowerShell for hook execution on Windows: handles all path
formats correctly, uses single-quote escaping ('' for literal '),
and avoids cmd.exe's AutoRun and %variable% expansion issues.
This unifies escaping (single quotes on all platforms) and removes
the need to skip integration tests on Windows.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use forward slashes in touchCmd/pwdCmd paths to avoid TOML backslash escaping issues when embedding commands in .roborev.toml - Resolve 8.3 short paths via EvalSymlinks and use case-insensitive comparison for working directory test - Use -NoNewline in pwdCmd and TrimSpace to handle CRLF - Add noopCmd() helper (Write-Output on Windows, true on Unix) for log capture tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Configurable hooks system that runs shell commands on review events. Closes #180.
review.completed,review.failed, orreview.*wildcard{job_id},{repo},{repo_name},{sha},{agent},{verdict},{error}— auto shell-escapedtype = "beads"auto-creates bd issues for failures and failing verdicts (includesroborev fixhint)~/.roborev/config.tomlfire for all repos;.roborev.tomlhooks fire for that repo only; both fire independentlysh -con Unix,cmd /Con Windows with appropriate escapingroborev statusnow shows the daemon version for easier debuggingFiles
internal/config/config.goHookConfigstruct,Hooksfield onConfigandRepoConfiginternal/daemon/hooks.goHookRunner, event matching, interpolation, shell escaping, beads built-ininternal/daemon/hooks_test.gointernal/daemon/server.goHookRunnerinto daemon startup/shutdowncmd/roborev/main.goroborev statusoutputTest plan
go test ./...passestype = "beads"creates bd issues on review failure🤖 Generated with Claude Code