git.iko.soy/crush
on main help sign in
feat(hooks): run via shell.Run instead of sh -c
22d6f44Christian Rocha
internal/hooks/hooks_test.go+60
compare with main
unifiedsplit
@@ -2,11 +2,14 @@ package hooks
2
2
3import (
3import (
4 "context"
4 "context"
5+ "io"
5 "strings"
6 "strings"
7+ "sync"
6 "testing"
8 "testing"
7 "time"
9 "time"
8
10
9 "github.com/charmbracelet/crush/internal/config"
11 "github.com/charmbracelet/crush/internal/config"
12+ "github.com/charmbracelet/crush/internal/shell"
10 "github.com/stretchr/testify/require"
13 "github.com/stretchr/testify/require"
11)
14)
12
15
@@ -568,6 +571,63 @@ func TestAggregationUpdatedInput(t *testing.T) {
568 })
571 })
569}
572}
570
573
574+// TestRunnerAbandonRaceSafety verifies that if a hook's shell execution
575+// does not yield to ctx cancellation within abandonGrace, runOne returns
576+// promptly and never touches the shared stdout/stderr buffers again —
577+// even while the abandoned goroutine continues to write to them.
578+//
579+// The substitute shell executor ignores ctx entirely, writes to Stdout
580+// both before and after the abandon deadline, and only then returns.
581+// Under -race this catches any code path in runOne that reads those
582+// buffers after returning the DecisionNone abandon result.
583+func TestRunnerAbandonRaceSafety(t *testing.T) {
584+ origRunShell := runShell
585+ t.Cleanup(func() { runShell = origRunShell })
586+
587+ // Synchronize shutdown with the abandoned goroutine so the test
588+ // exits cleanly even under -race.
589+ var wg sync.WaitGroup
590+ release := make(chan struct{})
591+ t.Cleanup(func() {
592+ close(release)
593+ wg.Wait()
594+ })
595+
596+ runShell = func(_ context.Context, opts shell.RunOptions) error {
597+ wg.Add(1)
598+ defer wg.Done()
599+ // Write before the caller observes ctx.Done(); the caller will
600+ // not read the buffer while we still own it.
601+ _, _ = io.WriteString(opts.Stdout, "before\n")
602+ // Hold past ctx deadline + abandonGrace so the caller takes
603+ // the abandon branch, then continue writing. If the caller
604+ // reads these buffers after abandoning, -race will flag it.
605+ select {
606+ case <-time.After(5 * time.Second):
607+ case <-release:
608+ }
609+ _, _ = io.WriteString(opts.Stdout, "after\n")
610+ return nil
611+ }
612+
613+ hookCfg := config.HookConfig{
614+ Command: "# irrelevant; runShell is stubbed",
615+ Timeout: 1,
616+ }
617+ r := NewRunner([]config.HookConfig{hookCfg}, t.TempDir(), t.TempDir())
618+
619+ start := time.Now()
620+ result, err := r.Run(context.Background(), EventPreToolUse, "sess", "bash", `{}`)
621+ elapsed := time.Since(start)
622+
623+ require.NoError(t, err)
624+ require.Equal(t, DecisionNone, result.Decision)
625+ // Abandon must happen at ~timeout + abandonGrace. Allow generous
626+ // slack so CI noise doesn't flake the test.
627+ require.Less(t, elapsed, 3500*time.Millisecond,
628+ "runOne should return within timeout+abandonGrace+slack")
629+}
630+
571func TestRunnerUpdatedInput(t *testing.T) {
631func TestRunnerUpdatedInput(t *testing.T) {
572 t.Parallel()
632 t.Parallel()
573 hookCfg := config.HookConfig{
633 hookCfg := config.HookConfig{
internal/hooks/runner.go+61−16
@@ -4,14 +4,26 @@ import (
4 "bytes"
4 "bytes"
5 "context"
5 "context"
6 "log/slog"
6 "log/slog"
7− "os/exec"
8 "strings"
7 "strings"
9 "sync"
8 "sync"
10 "time"
9 "time"
11
10
12 "github.com/charmbracelet/crush/internal/config"
11 "github.com/charmbracelet/crush/internal/config"
12+ "github.com/charmbracelet/crush/internal/shell"
13)
13)
14
14
15+// abandonGrace is how long runOne waits after ctx cancellation for the
16+// shell goroutine to yield before returning control to the caller and
17+// letting the goroutine finish on its own. Mirrors the historical
18+// cmd.WaitDelay = time.Second behavior of the previous os/exec path.
19+const abandonGrace = time.Second
20+
21+// runShell is the shell executor used by runOne. It is a package-level
22+// variable so tests can substitute a blocking or non-yielding
23+// implementation to exercise the abandon-on-timeout path without
24+// depending on the scheduling behavior of the real interpreter.
25+var runShell = shell.Run
26+
15// Runner executes hook commands and aggregates their results.
27// Runner executes hook commands and aggregates their results.
16type Runner struct {
28type Runner struct {
17 hooks []config.HookConfig
29 hooks []config.HookConfig
@@ -102,24 +114,59 @@ func (r *Runner) matchingHooks(toolName string) []config.HookConfig {
102}
114}
103
115
104// runOne executes a single hook command and returns its result.
116// runOne executes a single hook command and returns its result.
117+//
118+// Execution goes through Crush's embedded POSIX shell (shell.Run) so the
119+// same interpreter, builtins, and coreutils are visible to hooks as to
120+// the bash tool. BlockFuncs are intentionally omitted: hooks are
121+// user-authored config that carry the same trust as a shell alias.
122+//
123+// A hook that fails to yield after its deadline has passed is abandoned
124+// after abandonGrace so the caller never blocks longer than
125+// timeout + abandonGrace. Ownership of the stdout and stderr buffers is
126+// strictly single-goroutine:
127+// - before receiving from `done`, only the goroutine writes to them;
128+// - after `done` delivers a value, the goroutine is finished and the
129+// outer frame reads them;
130+// - on the abandon path, the goroutine may still be writing and the
131+// outer frame must not touch them again.
105func (r *Runner) runOne(parentCtx context.Context, hook config.HookConfig, envVars []string, payload []byte) HookResult {
132func (r *Runner) runOne(parentCtx context.Context, hook config.HookConfig, envVars []string, payload []byte) HookResult {
106 timeout := hook.TimeoutDuration()
133 timeout := hook.TimeoutDuration()
107 ctx, cancel := context.WithTimeout(parentCtx, timeout)
134 ctx, cancel := context.WithTimeout(parentCtx, timeout)
108 defer cancel()
135 defer cancel()
109
136
110− cmd := exec.CommandContext(ctx, "sh", "-c", hook.Command)
111− cmd.WaitDelay = time.Second
112− cmd.Env = envVars
113− cmd.Dir = r.cwd
114− cmd.Stdin = bytes.NewReader(payload)
115−
116 var stdout, stderr bytes.Buffer
137 var stdout, stderr bytes.Buffer
138+ done := make(chan error, 1)
139+ go func() {
140+ done <- runShell(ctx, shell.RunOptions{
141+ Command: hook.Command,
142+ Cwd: r.cwd,
143+ Env: envVars,
144+ Stdin: bytes.NewReader(payload),
117− cmd.Stdout = &stdout
145+ Stdout: &stdout,
118− cmd.Stderr = &stderr
146+ Stderr: &stderr,
119−
147+ })
120− err := cmd.Run()
148+ }()
149+
150+ var err error
151+ select {
152+ case err = <-done:
153+ // Normal path: goroutine has finished, buffers are safe to read.
154+ case <-ctx.Done():
155+ select {
156+ case err = <-done:
157+ // Interpreter yielded within the grace period; safe to read.
158+ case <-time.After(abandonGrace):
159+ slog.Warn("Hook did not yield after cancel; abandoning goroutine",
160+ "command", hook.Command,
161+ "timeout", timeout,
162+ )
163+ // The goroutine may still be writing to stdout/stderr; do
164+ // not read either buffer below this point.
165+ return HookResult{Decision: DecisionNone}
166+ }
167+ }
121
168
122− if ctx.Err() != nil {
169+ if shell.IsInterrupt(err) {
123 // Distinguish timeout from parent cancellation.
170 // Distinguish timeout from parent cancellation.
124 if parentCtx.Err() != nil {
171 if parentCtx.Err() != nil {
125 slog.Debug("Hook cancelled by parent context", "command", hook.Command)
172 slog.Debug("Hook cancelled by parent context", "command", hook.Command)
@@ -130,10 +177,7 @@ func (r *Runner) runOne(parentCtx context.Context, hook config.HookConfig, envVa
130 }
177 }
131
178
132 if err != nil {
179 if err != nil {
133− exitCode := -1
180+ exitCode := shell.ExitCode(err)
134− if cmd.ProcessState != nil {
135− exitCode = cmd.ProcessState.ExitCode()
136− }
137 switch exitCode {
181 switch exitCode {
138 case 2:
182 case 2:
139 // Exit code 2 = block this tool call. Stderr is the reason.
183 // Exit code 2 = block this tool call. Stderr is the reason.
@@ -162,6 +206,7 @@ func (r *Runner) runOne(parentCtx context.Context, hook config.HookConfig, envVa
162 "command", hook.Command,
206 "command", hook.Command,
163 "exit_code", exitCode,
207 "exit_code", exitCode,
164 "stderr", strings.TrimSpace(stderr.String()),
208 "stderr", strings.TrimSpace(stderr.String()),
209+ "error", err,
165 )
210 )
166 return HookResult{Decision: DecisionNone}
211 return HookResult{Decision: DecisionNone}
167 }
212 }