Fix git hook failures breaking refine loop

[wesm]

Summary

Closes #258.

• Suppress post-checkout hooks during worktree creation by passing -c core.hooksPath=/dev/null to git worktree add. User hooks are irrelevant in internal worktrees and shouldn't kill the refine loop.
• Add commitWithHookRetry helper that retries commits up to 3 times when a pre-commit hook rejects, running the agent between attempts to fix the flagged issues.
• Only retry when a pre-commit hook is actually installed (HasExecutableHook check). Non-hook commit failures (missing identity, empty commit, lockfile) return immediately.
• Add HEAD/branch safety checks before and after each agent fix run to abort if repo state drifts during the retry loop.

Test plan

• TestCreateTempWorktreeIgnoresHooks — worktree creation succeeds despite a failing post-checkout hook
• TestCommitWithHookRetrySucceeds — retry succeeds when hook fails once then passes
• TestCommitWithHookRetryExhausted — proper error after 3 failed attempts
• TestCommitWithHookRetrySkipsNonHookError — non-hook failures return immediately without retrying
• TestCreateCommitPreCommitHookOutput — CreateCommit includes hook stderr in error messages
• Full suite: go test ./...

🤖 Generated with Claude Code

[roborev-ci]

roborev: Combined Review

Verdict: Changes should not merge yet; there are unresolved High and Medium issues in the new hook-retry flow.

High

• Hook-bypass path during auto-fix retries (security control bypass)
  Refs: cmd/roborev/refine.go:787, cmd/roborev/refine.go:817, cmd/roborev/refine.go:820, cmd/roborev/refine.go:845, cmd/roborev/refine.go:856, cmd/roborev/refine.go:872
verifyRepoState validates HEAD/branch but not hook integrity/config, so the fix agent can make commits pass by altering hook behavior (for example via .git/hooks or hooks config) instead of fixing code.

• Prompt-injection risk from untrusted hook stderr passed to autonomous agent
  Refs: cmd/roborev/refine.go:825, cmd/roborev/refine.go:844
  Raw hookErr is injected into the fix prompt, allowing malicious hook output to steer agent behavior.

Medium

• Sensitive data egress risk via hook stderr in prompts/logging
  Refs: cmd/roborev/refine.go:521, cmd/roborev/refine.go:525, cmd/roborev/refine.go:810, cmd/roborev/refine.go:825, cmd/roborev/refine.go:828
  Hook stderr may contain secrets or sensitive snippets and is forwarded to the LLM/log output without redaction.

• Retry logic keyed to hook presence, not hook-caused failure
  Ref: cmd/roborev/refine.go:800
  Retries can run even when commit failure is unrelated to pre-commit hooks, causing unnecessary agent runs and misleading error attribution.

---

Synthesized from 4 reviews (agents: codex, gemini | types: security, default)

[roborev-ci]

roborev: Combined Review

Verdict: Changes are directionally good, but there are 2 actionable issues (1 High, 1 Medium) that should be addressed before merge.

High

1. Potential secret exfiltration via raw hook stderr in LLM prompt
   • Location: cmd/roborev/refine.go:819, cmd/roborev/refine.go:836, cmd/roborev/refine.go:853
   • Issue: Raw pre-commit stderr is inserted into the fix prompt and sent to fixAgent.Review(...). Hook output can include sensitive material (e.g., secret scanner findings) and untrusted content, which may be transmitted to external providers.
   • Recommended fix: Default to sanitized/structured summaries (redaction + truncation), treat hook output as untrusted/delimited input, and require explicit opt-in for forwarding full raw stderr.

Medium

1. Retry logic may trigger on non-pre-commit commit failures
   • Location: cmd/roborev/refine.go:804, cmd/roborev/refine.go:814
   • Issue: Retry is gated by commit-phase failure + presence of an executable pre-commit hook, which can incorrectly retry for unrelated failures (e.g., commit-msg, signing/identity errors) and mislabel the final error as pre-commit-related.
   • Recommended fix: Classify retryability by actual failure cause and only retry when failure is confirmed to come from pre-commit.

---

Synthesized from 4 reviews (agents: codex, gemini | types: security, default)

[roborev-ci]

roborev: Combined Review

Verdict: Changes introduce a meaningful security/regression risk in hook-failure handling and prompt construction that should be addressed before merge.

High

• Prompt injection via raw hook output in agent prompt
  • Files: cmd/roborev/refine.go:833, cmd/roborev/refine.go:816, cmd/roborev/refine.go:850
  • Issue: Untrusted git commit/hook stderr is embedded directly into fixPrompt and sent to fixAgent.Review(...). Malicious hook output can inject adversarial instructions into an unsandboxed refine flow, with potential unintended code edits or data exposure.
  • Recommendation: Treat hook output as untrusted data: delimit/quote as inert text, redact likely secrets, truncate length, and require explicit user confirmation before forwarding raw hook output.

Medium

• Hook failure classification is based on re-executing the hook, which can misclassify failures

  • Files: internal/git/git.go:824, cmd/roborev/refine.go:805
  • Issue: HookFailed is inferred by running the hook again after commit failure. Stateful hooks can fail on first run and pass on second run, causing false negatives and skipping intended retry/fix behavior.
  • Recommendation: Classify from the original git commit failure context (stderr + hook presence/phase) rather than probe result alone.

• Direct hook execution does not match Git’s hook runtime and adds side-effect risk

  • Files: internal/git/git.go:709, internal/git/git.go:722, internal/git/git.go:724
  • Issue: exec.Command(hookFile) runs repository-controlled hook code outside Git’s normal environment (and effectively runs hooks twice), which can produce platform/env mismatches (notably Windows/script handling) and extra side effects.
  • Recommendation: Prefer git hook run pre-commit where available, or avoid direct re-execution and infer hook failure from the original commit failure path.

---

Synthesized from 4 reviews (agents: codex, gemini | types: security, default)

[roborev-ci]

roborev: Combined Review

Verdict: Changes introduce one High and three Medium issues that should be addressed before merge.

High

• Prompt injection risk from untrusted hook output
  • Locations: cmd/roborev/refine.go:812, cmd/roborev/refine.go:828, cmd/roborev/refine.go:834, cmd/roborev/refine.go:847
  • Issue: Raw pre-commit hook stderr (hookErr) is interpolated into fixPrompt and then used in an unsandboxed agent retry path. Malicious hook/log text can steer agent behavior.
  • Recommended fix: Treat hook output as untrusted data (strict delimiting + explicit “data only” instruction), sanitize/control characters, truncate size, and gate autonomous hook-fix retries behind explicit opt-in.

Medium

• Potential secret/data exposure via hook stderr logging and forwarding

  • Locations: cmd/roborev/refine.go:812, cmd/roborev/refine.go:816, cmd/roborev/refine.go:828
  • Issue: Full hook failure text is printed and forwarded to the model; hook/tool stderr can include credentials or sensitive environment details.
  • Recommended fix: Redact common secret patterns, truncate output, and default to safer summarized error forwarding.

• Unbounded stderr capture can cause memory/context blowups

  • Locations: internal/git/git.go:786, internal/git/git.go:797, cmd/roborev/refine.go:834
  • Issue: Capturing full stderr into bytes.Buffer and passing it downstream can cause large memory growth and LLM context-limit failures.
  • Recommended fix: Hard-cap captured stderr (e.g., tail N KB), and truncate before prompt construction.

• Hook-failure misclassification can trigger incorrect retries

  • Locations: internal/git/git.go:689, internal/git/git.go:801, cmd/roborev/refine.go:790
  • Issue: Using git commit --dry-run --no-verify to detect pre-commit failures can misclassify non-pre-commit failures as hook failures, causing misleading retry behavior/errors.
  • Recommended fix: Classify as hook-related only when confidence is high (or broaden label to unknown hook-related failure) and avoid retry on ambiguous/non-hook failures.

---

Synthesized from 4 reviews (agents: codex, gemini | types: security, default)

[roborev-ci]

roborev: Combined Review

Verdict: Changes are not yet merge-ready due to 3 Medium-severity issues.

Medium

1. Untrusted hook stderr is injected into the LLM prompt without bounds/sanitization
   Locations: cmd/roborev/refine.go:812, cmd/roborev/refine.go:816, cmd/roborev/refine.go:833
   Issue: Raw hookErr is forwarded into fixPrompt and sent to the fix agent. This creates prompt-injection risk, potential sensitive-data exfiltration, and unbounded token/cost exposure for large hook output.
   Suggested fix: Truncate output (bytes/lines), sanitize/redact sensitive patterns, and wrap as data (clear delimiters). Consider opt-in for sending full raw hook output.

2. Hook-failure classification is unreliable and can misroute retries
   Locations: internal/git/git.go:695, internal/git/git.go:804, cmd/roborev/refine.go:805
   Issue: HookFailed is inferred from git commit --dry-run --no-verify success. This can misclassify non-hook commit failures (e.g., write/lock-path failures) as hook failures, causing unnecessary agent-fix retries.
   Suggested fix: Require explicit hook evidence from original stderr and/or use a probe that preserves relevant commit-phase failure modes while disabling hooks robustly.

3. Retry loop treats message-policy hook failures as code-fixable
   Locations: cmd/roborev/refine.go:469, cmd/roborev/refine.go:795
   Issue: Retries keep a constant commitMsg, so commit-msg policy failures are unlikely to be resolved by code edits, yet the agent loop still runs and may make irrelevant changes.
   Suggested fix: Detect message-only hook failures and either regenerate/adjust commit message or skip code-fix retries for that class.

---

Synthesized from 4 reviews (agents: codex, gemini | types: security, default)

[roborev-ci]

roborev: Combined Review

Verdict: 1 Medium-severity regression to address before merge; no security issues were identified.

Medium

1. Non-hook commit failures can be misclassified as hook failures, causing incorrect retry behavior
   • Files: internal/git/git.go:689, internal/git/git.go:802, cmd/roborev/refine.go:805
   • What happens: isHookCausingFailure() can infer “hook-caused” from a successful git commit --dry-run --no-verify probe, but some non-hook failures (e.g., identity/signing-related commit failures) may still pass dry-run.
   • Impact: HookFailed may be set incorrectly, and commitWithHookRetry() may invoke agent-based retries for failures the agent cannot fix.
   • Suggested direction: tighten classification by explicitly checking known non-hook commit failures (identity/signing, etc.) and treating ambiguous outcomes as non-retryable.

---

Synthesized from 4 reviews (agents: codex, gemini | types: security, default)