From db4b4a93ae326151247ec2dea72fb4ccd5ba50d6 Mon Sep 17 00:00:00 2001 From: elyafi Date: Thu, 4 Jun 2026 21:47:39 +0200 Subject: [PATCH] Isolate reviewer CLIs from project autoload; add per-model timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hardening fixes to the model-invocation path, both surfaced by reviewing the external xdev skill against diffsmith's adapters. diffsmith-4tz — reviewer CLIs ran in the caller's cwd, where codex autoloads .agents/skills/*/SKILL.md (and can *activate* a project skill whose workflow posts review comments), and gemini/claude onboard from AGENTS.md / CLAUDE.md. That silently risks diffsmith's no-auto-post guarantee. New provider.IsolatedRunner() runs each command in a fresh, per-call temp dir (removed after), which the codex/gemini/claude New(nil) defaults now use. The diff is piped via stdin so reviewers need no cwd access; $HOME-based auth/config is untouched. DefaultRunner (now sharing runCmd) keeps inheriting cwd for gh/glab, whose inbox flow needs it. diffsmith-ptr — models run in a parallel fan-out that joins on all of them, so one hung reviewer CLI (e.g. an MCP cold-start) blocked the whole review with no recovery. Each goroutine's Review now runs under its own context.WithTimeout, so a slow model is cancelled and dropped while siblings finish. The same cap guards the synthesis call: a hung lead model surfaces as a synthesis skip reason, never a silent fallback. New --model-timeout flag (default 10m; 0 disables); cancellation reaches the child via exec.CommandContext. Tests added TDD-first (each RED observed before GREEN): IsolatedRunner runs/cleans a temp dir; a slow model times out while a fast one succeeds; synthesis times out as a skip. Full suite + go vet + go test -race green. --- internal/app/multimodel.go | 15 ++++++-- internal/app/multimodel_test.go | 53 +++++++++++++++++++++++++++-- internal/app/review.go | 13 +++++-- internal/app/synthesis.go | 12 ++++++- internal/app/synthesis_test.go | 47 ++++++++++++++++++++++--- internal/model/claudecli/adapter.go | 4 ++- internal/model/codexcli/adapter.go | 5 ++- internal/model/geminicli/adapter.go | 5 ++- internal/provider/runner.go | 41 ++++++++++++++++++++-- internal/provider/runner_test.go | 44 ++++++++++++++++++++++++ 10 files changed, 220 insertions(+), 19 deletions(-) create mode 100644 internal/provider/runner_test.go diff --git a/internal/app/multimodel.go b/internal/app/multimodel.go index 07a5833..c6504e3 100644 --- a/internal/app/multimodel.go +++ b/internal/app/multimodel.go @@ -3,6 +3,7 @@ package app import ( "context" "sync" + "time" tea "github.com/charmbracelet/bubbletea" @@ -23,7 +24,7 @@ type modelOutcome struct { // streams ModelStatusMsg updates via send. Returns one modelOutcome // per input model after all have completed. Order is non-deterministic // (callers should look up by Name). -func runModelsInParallel(ctx context.Context, models []model.Model, input *review.ReviewInput, send func(tea.Msg)) []modelOutcome { +func runModelsInParallel(ctx context.Context, models []model.Model, input *review.ReviewInput, send func(tea.Msg), timeout time.Duration) []modelOutcome { if len(models) == 0 { return nil } @@ -33,8 +34,18 @@ func runModelsInParallel(ctx context.Context, models []model.Model, input *revie wg.Add(1) go func(idx int, m model.Model) { defer wg.Done() + // Each model gets its own deadline so one hung reviewer (e.g. + // an MCP cold-start) is cancelled and dropped without blocking + // the others — the join below waits on every goroutine. A + // non-positive timeout disables the cap. + runCtx := ctx + if timeout > 0 { + var cancel context.CancelFunc + runCtx, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } send(tui.ModelStatusMsg{Name: m.Name(), State: "running"}) - r, err := m.Review(ctx, input) + r, err := m.Review(runCtx, input) if err != nil { send(tui.ModelStatusMsg{Name: m.Name(), State: "failed", Err: err}) results[idx] = modelOutcome{Name: m.Name(), Err: err} diff --git a/internal/app/multimodel_test.go b/internal/app/multimodel_test.go index 4ead96d..1d565a6 100644 --- a/internal/app/multimodel_test.go +++ b/internal/app/multimodel_test.go @@ -5,6 +5,7 @@ import ( "errors" "sync/atomic" "testing" + "time" tea "github.com/charmbracelet/bubbletea" @@ -46,7 +47,7 @@ func TestRunModelsInParallel_AllSucceed(t *testing.T) { atomic.AddInt32(&statusCount, 1) } } - results := runModelsInParallel(context.Background(), models, &review.ReviewInput{}, send) + results := runModelsInParallel(context.Background(), models, &review.ReviewInput{}, send, 0) if len(results) != 2 { t.Fatalf("expected 2 results, got %d", len(results)) } @@ -66,7 +67,7 @@ func TestRunModelsInParallel_OneFails(t *testing.T) { fakeModel{name: "codex", result: &review.ModelReviewResult{}}, fakeModel{name: "claude", err: errors.New("simulated failure")}, } - results := runModelsInParallel(context.Background(), models, &review.ReviewInput{}, func(tea.Msg) {}) + results := runModelsInParallel(context.Background(), models, &review.ReviewInput{}, func(tea.Msg) {}, 0) if len(results) != 2 { t.Fatalf("expected 2 results, got %d", len(results)) } @@ -83,8 +84,54 @@ func TestRunModelsInParallel_OneFails(t *testing.T) { } func TestRunModelsInParallel_EmptyInput(t *testing.T) { - results := runModelsInParallel(context.Background(), nil, &review.ReviewInput{}, func(tea.Msg) {}) + results := runModelsInParallel(context.Background(), nil, &review.ReviewInput{}, func(tea.Msg) {}, 0) if len(results) != 0 { t.Errorf("empty input should give empty results; got %d", len(results)) } } + +// blockingModel.Review blocks until either its context is cancelled (then +// it reports ctx.Err()) or fallback elapses (then it succeeds). It lets a +// test prove a per-model timeout fires: with a timeout shorter than +// fallback the context cancels first; with no timeout the model "succeeds" +// after fallback, which is the RED state we want to see fail. +type blockingModel struct { + name string + fallback time.Duration +} + +func (b blockingModel) Name() string { return b.name } +func (b blockingModel) Preflight(context.Context) error { return nil } +func (b blockingModel) Review(ctx context.Context, _ *review.ReviewInput) (*review.ModelReviewResult, error) { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(b.fallback): + return &review.ModelReviewResult{Model: b.name}, nil + } +} + +var _ model.Model = blockingModel{} + +// TestRunModelsInParallel_SlowModelTimesOut proves diffsmith-ptr: a model +// that hangs past the per-model timeout drops out as an error while fast +// models still return their results. Without the timeout the whole call +// would block on the slow model (wg.Wait), defeating parallelism. +func TestRunModelsInParallel_SlowModelTimesOut(t *testing.T) { + models := []model.Model{ + fakeModel{name: "codex", result: &review.ModelReviewResult{}}, + blockingModel{name: "gemini", fallback: 500 * time.Millisecond}, + } + results := runModelsInParallel(context.Background(), models, &review.ReviewInput{}, func(tea.Msg) {}, 20*time.Millisecond) + + byName := map[string]modelOutcome{} + for _, r := range results { + byName[r.Name] = r + } + if byName["codex"].Err != nil { + t.Errorf("fast model codex should succeed; got %v", byName["codex"].Err) + } + if byName["gemini"].Err == nil { + t.Error("slow model gemini should drop out with a timeout error") + } +} diff --git a/internal/app/review.go b/internal/app/review.go index 604930a..4d7f9ef 100644 --- a/internal/app/review.go +++ b/internal/app/review.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "strings" + "time" tea "github.com/charmbracelet/bubbletea" "github.com/spf13/cobra" @@ -63,6 +64,7 @@ func registerPostFlowFlags(cmd *cobra.Command, flags *reviewFlags) { // flags (e.g. --max-files) should join here rather than be inlined. func registerModelFlowFlags(cmd *cobra.Command, flags *reviewFlags) { cmd.Flags().IntVar(&flags.inputBudget, "input-budget", 0, "override the per-adapter prompt-size cap in bytes (default: 1 MiB per adapter; 0 keeps the default)") + cmd.Flags().DurationVar(&flags.modelTimeout, "model-timeout", 10*time.Minute, "per-model wall-clock cap; a model exceeding it is cancelled and dropped from the review (0 disables)") } // renameMapFromFiles extracts the post-image → pre-image rename mapping @@ -112,6 +114,13 @@ type reviewFlags struct { // int flag) means "leave each adapter's default in place" — see // applyInputBudget for the no-op-on-zero contract. inputBudget int + // modelTimeout caps how long each model's Review may run before it is + // cancelled and dropped from the review. A reviewer CLI can hang + // (e.g. an MCP server cold-start), and because the models run in a + // parallel fan-out that joins on all of them, one hang would + // otherwise block the whole review. Zero disables the cap (inherit + // the parent context only). See runModelsInParallel. + modelTimeout time.Duration } func newReviewCmd(registry *provider.Registry, models map[string]model.Model) *cobra.Command { @@ -250,7 +259,7 @@ func runReviewByURL(ctx context.Context, cmd *cobra.Command, url string, flags * } send(tui.PhaseStatusMsg("Reviewing with selected models…")) - outcomes := runModelsInParallel(ctx, selected.All, input, send) + outcomes := runModelsInParallel(ctx, selected.All, input, send, flags.modelTimeout) surviving, dropped := splitOutcomes(outcomes) if len(surviving) == 0 { @@ -279,7 +288,7 @@ func runReviewByURL(ctx context.Context, cmd *cobra.Command, url string, flags * // to bail with "no matching model registered." send(tui.PhaseStatusMsg(fmt.Sprintf("Synthesizing with %s…", candidate.Model))) } - synth, skipReason := attemptSynthesis(ctx, leadModel, input, surviving) + synth, skipReason := attemptSynthesis(ctx, leadModel, input, surviving, flags.modelTimeout) if skipReason != "" { // Every skip surfaces a reason — including the // (nil, nil) silent-fallback that attemptSynthesis diff --git a/internal/app/synthesis.go b/internal/app/synthesis.go index 36d8789..d980eef 100644 --- a/internal/app/synthesis.go +++ b/internal/app/synthesis.go @@ -3,6 +3,7 @@ package app import ( "context" "fmt" + "time" "github.com/selyafi/diffsmith/internal/model" "github.com/selyafi/diffsmith/internal/review" @@ -25,7 +26,7 @@ import ( // 4. Synthesize returned (nil, nil) — undefined per the adapter // contract but legal Go. Without this branch the loop would // silently advance; diffsmith-4f8 introduced the explicit check. -func attemptSynthesis(ctx context.Context, leadModel model.Reviewer, input *review.ReviewInput, surviving []*review.ModelReviewResult) (*review.ModelReviewResult, string) { +func attemptSynthesis(ctx context.Context, leadModel model.Reviewer, input *review.ReviewInput, surviving []*review.ModelReviewResult, timeout time.Duration) (*review.ModelReviewResult, string) { if leadModel == nil { return nil, "no matching model registered in the picker selection" } @@ -33,6 +34,15 @@ func attemptSynthesis(ctx context.Context, leadModel model.Reviewer, input *revi if !ok { return nil, "model does not implement the Synthesizer capability" } + // Cap the lead model the same way the parallel reviewers are capped: + // a hung synthesis CLI must not block the whole review. A non-positive + // timeout disables the cap. A deadline surfaces as a skip reason via + // the err path below — never a silent fallback. + if timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } synth, err := leadSynth.Synthesize(ctx, input, surviving) if err != nil { return nil, fmt.Sprintf("synthesis failed: %v", err) diff --git a/internal/app/synthesis_test.go b/internal/app/synthesis_test.go index 3a79ece..61ac929 100644 --- a/internal/app/synthesis_test.go +++ b/internal/app/synthesis_test.go @@ -5,6 +5,7 @@ import ( "errors" "strings" "testing" + "time" "github.com/selyafi/diffsmith/internal/model" "github.com/selyafi/diffsmith/internal/review" @@ -37,12 +38,48 @@ func (s synthFake) Synthesize(context.Context, *review.ReviewInput, []*review.Mo return s.out, s.err } +// blockingSynth blocks inside Synthesize until its context is cancelled +// (then it surfaces ctx.Err()) or fallback elapses (then it "succeeds"). +// Lets a test prove the per-model timeout also guards the synthesis call. +type blockingSynth struct { + name string + fallback time.Duration +} + +func (b blockingSynth) Name() string { return b.name } +func (b blockingSynth) Preflight(context.Context) error { return nil } +func (b blockingSynth) Review(context.Context, *review.ReviewInput) (*review.ModelReviewResult, error) { + return nil, nil +} +func (b blockingSynth) Synthesize(ctx context.Context, _ *review.ReviewInput, _ []*review.ModelReviewResult) (*review.ModelReviewResult, error) { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(b.fallback): + return &review.ModelReviewResult{Model: b.name}, nil + } +} + +// TestAttemptSynthesis_TimesOutAsSkip proves a hung lead model is +// cancelled by the per-model timeout and surfaced as a skip reason rather +// than blocking the whole review. +func TestAttemptSynthesis_TimesOutAsSkip(t *testing.T) { + lead := blockingSynth{name: "codex", fallback: 500 * time.Millisecond} + got, skip := attemptSynthesis(context.Background(), lead, &review.ReviewInput{}, nil, 20*time.Millisecond) + if got != nil { + t.Errorf("expected nil result on timeout; got %+v", got) + } + if skip == "" { + t.Error("expected a skip reason when synthesis times out") + } +} + // TestAttemptSynthesis_SuccessReturnsResultNoSkip pins the happy path: // when the lead returns (result, nil), attemptSynthesis must return // that result and an empty skip reason. func TestAttemptSynthesis_SuccessReturnsResultNoSkip(t *testing.T) { want := &review.ModelReviewResult{Model: "codex"} - got, skip := attemptSynthesis(context.Background(), synthFake{name: "codex", out: want}, &review.ReviewInput{}, nil) + got, skip := attemptSynthesis(context.Background(), synthFake{name: "codex", out: want}, &review.ReviewInput{}, nil, 0) if skip != "" { t.Errorf("happy path must return empty skip; got %q", skip) } @@ -56,7 +93,7 @@ func TestAttemptSynthesis_SuccessReturnsResultNoSkip(t *testing.T) { // advance the loop. attemptSynthesis surfaces a clear skip reason so // the caller can log it instead of falling back unannounced. func TestAttemptSynthesis_NilNilTreatedAsSkip(t *testing.T) { - got, skip := attemptSynthesis(context.Background(), synthFake{name: "codex"}, &review.ReviewInput{}, nil) + got, skip := attemptSynthesis(context.Background(), synthFake{name: "codex"}, &review.ReviewInput{}, nil, 0) if got != nil { t.Errorf("got non-nil result %v from (nil, nil); want nil", got) } @@ -73,7 +110,7 @@ func TestAttemptSynthesis_NilNilTreatedAsSkip(t *testing.T) { // TestAttemptSynthesis_ErrorPropagatesAsSkip confirms a Synthesize // error becomes a skip with the error text embedded. func TestAttemptSynthesis_ErrorPropagatesAsSkip(t *testing.T) { - _, skip := attemptSynthesis(context.Background(), synthFake{name: "codex", err: errors.New("budget exceeded")}, &review.ReviewInput{}, nil) + _, skip := attemptSynthesis(context.Background(), synthFake{name: "codex", err: errors.New("budget exceeded")}, &review.ReviewInput{}, nil, 0) if !strings.Contains(skip, "budget exceeded") { t.Errorf("skip reason should include the underlying error; got %q", skip) } @@ -83,7 +120,7 @@ func TestAttemptSynthesis_ErrorPropagatesAsSkip(t *testing.T) { // confirms a lead that doesn't satisfy model.Synthesizer is skipped // with a reason that names the capability gap. func TestAttemptSynthesis_ReviewerOnlyLeadSkipsWithCapabilityReason(t *testing.T) { - _, skip := attemptSynthesis(context.Background(), reviewerOnlyFake{name: "agy"}, &review.ReviewInput{}, nil) + _, skip := attemptSynthesis(context.Background(), reviewerOnlyFake{name: "agy"}, &review.ReviewInput{}, nil, 0) if !strings.Contains(skip, "Synthesizer") { t.Errorf("skip reason should name the Synthesizer capability gap; got %q", skip) } @@ -92,7 +129,7 @@ func TestAttemptSynthesis_ReviewerOnlyLeadSkipsWithCapabilityReason(t *testing.T // TestAttemptSynthesis_NilLeadSkipsWithRegistryReason confirms a nil // lead model (registry miss) is skipped with an explanation. func TestAttemptSynthesis_NilLeadSkipsWithRegistryReason(t *testing.T) { - _, skip := attemptSynthesis(context.Background(), nil, &review.ReviewInput{}, nil) + _, skip := attemptSynthesis(context.Background(), nil, &review.ReviewInput{}, nil, 0) if skip == "" { t.Fatal("nil lead must produce a skip reason; got empty") } diff --git a/internal/model/claudecli/adapter.go b/internal/model/claudecli/adapter.go index c3d56cf..863d557 100644 --- a/internal/model/claudecli/adapter.go +++ b/internal/model/claudecli/adapter.go @@ -34,7 +34,9 @@ type Adapter struct { // (the package is internal-only). func New(run provider.Runner) *Adapter { if run == nil { - run = provider.DefaultRunner + // Isolate claude from the caller's cwd so it can't autoload a + // project CLAUDE.md / .claude config during a review. diffsmith-4tz. + run = provider.IsolatedRunner() } return &Adapter{ run: run, diff --git a/internal/model/codexcli/adapter.go b/internal/model/codexcli/adapter.go index a0ec89b..bbbc474 100644 --- a/internal/model/codexcli/adapter.go +++ b/internal/model/codexcli/adapter.go @@ -45,7 +45,10 @@ type Adapter struct { // (the package is internal-only). func New(run provider.Runner) *Adapter { if run == nil { - run = provider.DefaultRunner + // Isolate codex from the caller's cwd so it can't autoload a + // project's .agents/skills/ (and possibly activate a skill that + // posts comments). diffsmith-4tz. + run = provider.IsolatedRunner() } return &Adapter{ run: run, diff --git a/internal/model/geminicli/adapter.go b/internal/model/geminicli/adapter.go index 36cfad3..684de1f 100644 --- a/internal/model/geminicli/adapter.go +++ b/internal/model/geminicli/adapter.go @@ -30,7 +30,10 @@ type Adapter struct { // (the package is internal-only). func New(run provider.Runner) *Adapter { if run == nil { - run = provider.DefaultRunner + // Isolate gemini from the caller's cwd so it can't onboard from a + // project AGENTS.md / CLAUDE.md or autoload project MCP config. + // diffsmith-4tz. + run = provider.IsolatedRunner() } return &Adapter{ run: run, diff --git a/internal/provider/runner.go b/internal/provider/runner.go index 77b43ca..e540096 100644 --- a/internal/provider/runner.go +++ b/internal/provider/runner.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "os" "os/exec" "strings" ) @@ -21,11 +22,45 @@ import ( // data to the child (e.g. codex via ADR 0007) provide a Reader here. type Runner func(ctx context.Context, stdin io.Reader, name string, args ...string) ([]byte, error) -// DefaultRunner runs the command via os/exec and returns stdout. On a -// non-zero exit it returns an error including the exit code and a trimmed -// copy of stderr. +// DefaultRunner runs the command via os/exec and returns stdout. The +// child inherits diffsmith's working directory. Use this for provider +// CLIs (gh/glab) that operate on explicit URLs and for any caller that +// genuinely needs the real cwd. func DefaultRunner(ctx context.Context, stdin io.Reader, name string, args ...string) ([]byte, error) { + return runCmd(ctx, "", stdin, name, args...) +} + +// IsolatedRunner returns a Runner that executes each command in a fresh, +// empty temp directory (removed once the command returns). It exists for +// the model adapters (diffsmith-4tz): reviewer CLIs like codex/gemini/ +// claude autoload project context from their working directory — +// codex discovers .agents/skills/*/SKILL.md and may *activate* a project +// skill (e.g. one whose workflow posts review comments), gemini/claude +// onboard from AGENTS.md / CLAUDE.md. Running them in a neutral temp dir +// neutralizes that autoload, which protects diffsmith's no-auto-post +// guarantee and keeps reviews deterministic. The whole diff is piped via +// stdin, so reviewers need no access to the caller's cwd. +// +// Only the working directory is isolated; user-level config and auth +// (~/.codex, ~/.gemini, ~/.claude) live under $HOME and are found via the +// environment, not cwd, so they keep working. +func IsolatedRunner() Runner { + return func(ctx context.Context, stdin io.Reader, name string, args ...string) ([]byte, error) { + dir, err := os.MkdirTemp("", "diffsmith-iso-*") + if err != nil { + return nil, fmt.Errorf("create isolation dir: %w", err) + } + defer func() { _ = os.RemoveAll(dir) }() + return runCmd(ctx, dir, stdin, name, args...) + } +} + +// runCmd is the shared exec body. When workDir is non-empty the child runs +// there; otherwise it inherits the caller's cwd. On a non-zero exit it +// returns an error including the exit code and a trimmed copy of stderr. +func runCmd(ctx context.Context, workDir string, stdin io.Reader, name string, args ...string) ([]byte, error) { cmd := exec.CommandContext(ctx, name, args...) + cmd.Dir = workDir if stdin != nil { cmd.Stdin = stdin } diff --git a/internal/provider/runner_test.go b/internal/provider/runner_test.go new file mode 100644 index 0000000..bb2e466 --- /dev/null +++ b/internal/provider/runner_test.go @@ -0,0 +1,44 @@ +package provider_test + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/selyafi/diffsmith/internal/provider" +) + +// TestIsolatedRunnerRunsInFreshTempDir proves the core of diffsmith-4tz: +// reviewer CLIs must NOT run in the caller's working directory (where a +// project's .agents/skills/, AGENTS.md, or CLAUDE.md would be autoloaded). +// We exec `pwd` through the isolated runner and assert the child reported +// a directory that is (a) not the caller's cwd and (b) gone after the call +// returned — i.e. a fresh per-call temp dir that was cleaned up. +func TestIsolatedRunnerRunsInFreshTempDir(t *testing.T) { + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + cwdResolved, err := filepath.EvalSymlinks(cwd) + if err != nil { + t.Fatalf("evalsymlinks cwd: %v", err) + } + + run := provider.IsolatedRunner() + out, err := run(context.Background(), nil, "pwd") + if err != nil { + t.Fatalf("IsolatedRunner pwd: %v", err) + } + childDir := strings.TrimSpace(string(out)) + + if childDir == cwd || childDir == cwdResolved { + t.Errorf("child ran in caller cwd %q; want an isolated temp dir", childDir) + } + + // The per-call temp dir must be removed once the command returns. + if _, statErr := os.Stat(childDir); !os.IsNotExist(statErr) { + t.Errorf("temp dir %q still exists after run (stat err: %v); want cleaned up", childDir, statErr) + } +}