feat(hooks): automatic session-commit linking via git hooks#100
Merged
rsnodgrass merged 4 commits intomainfrom Mar 3, 2026
Merged
feat(hooks): automatic session-commit linking via git hooks#100rsnodgrass merged 4 commits intomainfrom
rsnodgrass merged 4 commits intomainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds deterministic Git prepare-commit-msg hooks, installation/uninstallation utilities, a doctor check to verify/install hooks, commit-message trailer appending logic (Co-Authored-By, SageOx-Session), session URL construction, session attribution support, and minor agent bootstrap session URL exposure. Changes
Sequence DiagramsequenceDiagram
participant Git as Git
participant Hook as prepare-commit-msg Hook
participant CLI as ox hooks command
participant Config as Project Config & Attribution
participant Trailers as git interpret-trailers
Git->>Hook: trigger prepare-commit-msg (file, source)
Hook->>CLI: invoke hidden `ox hooks commit-msg` with args
CLI->>Config: load project config & resolve attribution
Config-->>CLI: attribution + session state
CLI->>CLI: determine trailers (Co-Authored-By, SageOx-Session)
CLI->>Trailers: run `git interpret-trailers --in-place` to append trailers
Trailers-->>CLI: success/failure (logged)
CLI-->>Git: exit (do not block commit on failures)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/ox/integrate.go (1)
219-225: Differentiate “already installed” vs “installed now” for clearer UX.Because
InstallGitHooksis idempotent, repeated runs currently report a fresh install even when no change occurred. Consider checkingHasGitHooksfirst to keep status output accurate.Proposed refinement
- // install git commit hooks (prepare-commit-msg for trailers) - if err := InstallGitHooks(gitRoot); err != nil { + // install git commit hooks (prepare-commit-msg for trailers) + if HasGitHooks(gitRoot) { + fmt.Println(ui.PassStyle.Render("✓") + " Git commit hooks already installed") + fmt.Println(" - prepare-commit-msg (Co-Authored-By, SageOx-Session trailers)") + } else if err := InstallGitHooks(gitRoot); err != nil { cli.PrintWarning(fmt.Sprintf("Could not install git commit hooks: %v", err)) } else { fmt.Println(ui.PassStyle.Render("✓") + " Git commit hooks installed") fmt.Println(" - prepare-commit-msg (Co-Authored-By, SageOx-Session trailers)") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ox/integrate.go` around lines 219 - 225, The UX currently always prints "Git commit hooks installed" even when hooks were already present because InstallGitHooks is idempotent; call HasGitHooks(gitRoot) first to detect existing hooks and print a distinct message like "Git commit hooks already installed" when true, and only call InstallGitHooks when HasGitHooks returns false, handling and logging errors from InstallGitHooks as before and printing the success message only after a real install; reference the HasGitHooks and InstallGitHooks functions and the existing status printing logic for where to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ox/hooks_commit_msg_test.go`:
- Around line 13-40: Both tests mutate package globals hooksCommitMsgSource and
hooksCommitMsgFile without restoring them and ignore some I/O errors; fix by
saving original values of hooksCommitMsgSource and hooksCommitMsgFile at the
start of each test and restoring them in a defer, and replace ignored error
returns with require.NoError checks (e.g. check os.Getwd() error into origDirErr
with require.NoError, and check os.ReadFile/os.Chdir errors with
require.NoError) so runHooksCommitMsg tests clean up state and fail on I/O
errors; update TestRunHooksCommitMsg_SkipsMergeSource and
TestRunHooksCommitMsg_NoopWhenNotInitialized accordingly while still calling
runHooksCommitMsg.
In `@cmd/ox/hooks_git.go`:
- Around line 64-68: The current hook-install check only looks for
oxHookMarkerStart and treats partial hooks as valid; change the validation to
require both oxHookMarkerStart and oxHookMarkerEnd to be present in the existing
hook content (the variable existing) before returning success, and apply this
same stricter check in both places where the check is performed (the two
occurrences that currently call strings.Contains(existing, oxHookMarkerStart));
if either marker is missing, treat the hook as not installed so the
install/repair path proceeds.
In `@cmd/ox/session_url.go`:
- Around line 12-21: The buildSessionURL function interpolates cfg.RepoID and
sessionName directly into the path; update it to call url.PathEscape on both
values before constructing the final string so path segments are safely escaped
(use url.PathEscape(repoID) and url.PathEscape(sessionName)) and then build the
URL with fmt.Sprintf as before (ensuring you still call
endpoint.NormalizeEndpoint(cfg.GetEndpoint()) and keep the same
"/repo/%s/sessions/%s/view" format).
---
Nitpick comments:
In `@cmd/ox/integrate.go`:
- Around line 219-225: The UX currently always prints "Git commit hooks
installed" even when hooks were already present because InstallGitHooks is
idempotent; call HasGitHooks(gitRoot) first to detect existing hooks and print a
distinct message like "Git commit hooks already installed" when true, and only
call InstallGitHooks when HasGitHooks returns false, handling and logging errors
from InstallGitHooks as before and printing the success message only after a
real install; reference the HasGitHooks and InstallGitHooks functions and the
existing status printing logic for where to make this change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cmd/ox/agent_prime.gocmd/ox/doctor.gocmd/ox/doctor_git_commit_hooks.gocmd/ox/doctor_types.gocmd/ox/doctor_types_test.gocmd/ox/hooks_cmd.gocmd/ox/hooks_commit_msg.gocmd/ox/hooks_commit_msg_test.gocmd/ox/hooks_git.gocmd/ox/hooks_git_test.gocmd/ox/init.gocmd/ox/integrate.gocmd/ox/session_url.gocmd/ox/session_view_html.gointernal/config/attribution.gointernal/config/attribution_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/ox/session_url.go (1)
3-8:⚠️ Potential issue | 🟡 MinorEscape URL path segments before interpolation.
cfg.RepoIDandsessionNameare inserted directly into path segments. Escaping both withurl.PathEscapeavoids malformed links and edge-case path interpretation.🔧 Proposed fix
import ( "fmt" + "net/url" "github.com/sageox/ox/internal/config" "github.com/sageox/ox/internal/endpoint" ) @@ - return fmt.Sprintf("%s/repo/%s/sessions/%s/view", ep, cfg.RepoID, sessionName) + return fmt.Sprintf( + "%s/repo/%s/sessions/%s/view", + ep, + url.PathEscape(cfg.RepoID), + url.PathEscape(sessionName), + ) }#!/bin/bash # Verify how session names/repo IDs are sourced and whether they are guaranteed URL-safe. rg -nP --type=go -C3 '\bGetSessionName\s*\(|\bSessionPath\b|\bRepoID\b'Also applies to: 22-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ox/session_url.go` around lines 3 - 8, The code injects cfg.RepoID and sessionName directly into a URL path; modify the function that builds the session URL (reference symbols cfg.RepoID and sessionName, likely in a function like SessionPath/BuildSessionURL/GetSessionURL) to call url.PathEscape on both values before interpolation, add the net/url import, and use the escaped results when formatting the path so repo IDs and session names with special characters produce safe, well-formed URLs.cmd/ox/hooks_git.go (1)
70-71:⚠️ Potential issue | 🟠 MajorHarden marker validation for install/has/uninstall paths.
Using only the start marker to detect installation can misclassify broken hooks. It also makes uninstall unsafe if the end marker is missing, because removal may consume the rest of the file.
🔧 Proposed hardening
+func hasValidOxHook(content string) bool { + return strings.Contains(content, oxHookMarkerStart) && + strings.Contains(content, oxHookMarkerEnd) && + strings.Contains(content, "ox hooks commit-msg") +} + func InstallGitHooks(gitRoot string) error { @@ - if strings.Contains(string(existing), oxHookMarkerStart) { + if hasValidOxHook(string(existing)) { return nil // already installed } @@ - if !strings.Contains(string(content), oxHookMarkerStart) { + if !hasValidOxHook(string(content)) { return nil // ox section not present } @@ func HasGitHooks(gitRoot string) bool { @@ - return strings.Contains(string(content), oxHookMarkerStart) + return hasValidOxHook(string(content)) }Also applies to: 108-127, 150-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ox/hooks_git.go` around lines 70 - 71, The current marker check only looks for oxHookMarkerStart which can mis-detect broken hooks; update all install/has/uninstall logic to require both oxHookMarkerStart and oxHookMarkerEnd (or a strict regex that matches the full marker block) before treating the hook as installed or before removing it. Specifically, replace occurrences that call strings.Contains(existing, oxHookMarkerStart) with a check that both oxHookMarkerStart and oxHookMarkerEnd are present (or use a single compiled regexp matching the entire start..end block) and, in the uninstall path, refuse to modify the file if the end marker is missing to avoid stripping the rest of the file. Ensure you reference the oxHookMarkerStart and oxHookMarkerEnd symbols and update the code paths that currently assume only the start marker is sufficient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ox/hooks_git.go`:
- Around line 64-65: Replace the two uses of os.IsNotExist(err) in the hook-read
checks with errors.Is(err, os.ErrNotExist) (i.e., change the if conditions that
currently read "if err != nil && !os.IsNotExist(err) { ... }" to use
errors.Is(err, os.ErrNotExist)), and add the "errors" package to the imports so
the new checks compile; target the if-conditions that wrap the fmt.Errorf("read
existing hook: %w", err) returns and any equivalent existence checks in the same
file.
---
Duplicate comments:
In `@cmd/ox/hooks_git.go`:
- Around line 70-71: The current marker check only looks for oxHookMarkerStart
which can mis-detect broken hooks; update all install/has/uninstall logic to
require both oxHookMarkerStart and oxHookMarkerEnd (or a strict regex that
matches the full marker block) before treating the hook as installed or before
removing it. Specifically, replace occurrences that call
strings.Contains(existing, oxHookMarkerStart) with a check that both
oxHookMarkerStart and oxHookMarkerEnd are present (or use a single compiled
regexp matching the entire start..end block) and, in the uninstall path, refuse
to modify the file if the end marker is missing to avoid stripping the rest of
the file. Ensure you reference the oxHookMarkerStart and oxHookMarkerEnd symbols
and update the code paths that currently assume only the start marker is
sufficient.
In `@cmd/ox/session_url.go`:
- Around line 3-8: The code injects cfg.RepoID and sessionName directly into a
URL path; modify the function that builds the session URL (reference symbols
cfg.RepoID and sessionName, likely in a function like
SessionPath/BuildSessionURL/GetSessionURL) to call url.PathEscape on both values
before interpolation, add the net/url import, and use the escaped results when
formatting the path so repo IDs and session names with special characters
produce safe, well-formed URLs.
Comment on lines
+64
to
+65
| if err != nil && !os.IsNotExist(err) { | ||
| return fmt.Errorf("read existing hook: %w", err) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the file to check current state at mentioned lines
head -110 cmd/ox/hooks_git.go | tail -50Repository: sageox/ox
Length of output: 1422
🏁 Script executed:
#!/bin/bash
# Check for all os.IsNotExist occurrences in the file
rg -nP '\bos\.IsNotExist\s*\(' cmd/ox/hooks_git.goRepository: sageox/ox
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Check current imports in the file
head -20 cmd/ox/hooks_git.goRepository: sageox/ox
Length of output: 565
Use errors.Is(err, os.ErrNotExist) for consistency with repository error-handling guidelines.
The project standard requires errors.Is() for error checking. Replace both occurrences at lines 64 and 102 in cmd/ox/hooks_git.go with errors.Is(err, os.ErrNotExist) and add the errors import.
♻️ Proposed refactor
import (
+ "errors"
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
)
@@
- if err != nil && !os.IsNotExist(err) {
+ if err != nil && !errors.Is(err, os.ErrNotExist) {
return fmt.Errorf("read existing hook: %w", err)
}
@@
- if os.IsNotExist(err) {
+ if errors.Is(err, os.ErrNotExist) {
return nil // nothing to uninstall
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err != nil && !os.IsNotExist(err) { | |
| return fmt.Errorf("read existing hook: %w", err) | |
| import ( | |
| "errors" | |
| "fmt" | |
| "os" | |
| "os/exec" | |
| "path/filepath" | |
| "strings" | |
| ) | |
| if err != nil && !errors.Is(err, os.ErrNotExist) { | |
| return fmt.Errorf("read existing hook: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/ox/hooks_git.go` around lines 64 - 65, Replace the two uses of
os.IsNotExist(err) in the hook-read checks with errors.Is(err, os.ErrNotExist)
(i.e., change the if conditions that currently read "if err != nil &&
!os.IsNotExist(err) { ... }" to use errors.Is(err, os.ErrNotExist)), and add the
"errors" package to the imports so the new checks compile; target the
if-conditions that wrap the fmt.Errorf("read existing hook: %w", err) returns
and any equivalent existence checks in the same file.
rsnodgrass
added a commit
that referenced
this pull request
Mar 3, 2026
Co-Authored-By: SageOx <ox@sageox.ai>
This was referenced Mar 4, 2026
9 tasks
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
Implements automatic linking of ox session recordings to git commits via a
prepare-commit-msghook. The hook appends configurable trailers (Co-Authored-By, SageOx-Session) to commits, installed automatically as part ofox init,ox integrate install, andox doctorrepair workflows.What Changed
ox hooks commit-msg(deterministic hook called by git),ox hooks install/uninstall(helper functions)git-commit-hooksslug detects and auto-repairs missing or stale hook installationsbuildSessionURL()shared utility,Attribution.Sessionfield for per-field trailer controlox init,ox integrate install, andox doctor --fixTest Plan
Session Recording
View session recording
Co-Authored-By: SageOx ox@sageox.ai
Summary by CodeRabbit
New Features
Tests
Chores