Isolate reviewer CLIs from project autoload; add per-model timeout

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}

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")
+	}
+}

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

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,14 +26,23 @@ 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"
 	}
 	leadSynth, ok := leadModel.(model.Synthesizer)
 	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)

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")
 	}

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,

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,

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,

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
 	}