From 1b1f062d2f1ab61d694009f2ab5c21d85cf4fc4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Muhammet=20=C5=9Eafak?= Date: Thu, 30 Jul 2026 19:14:05 +0300 Subject: [PATCH] feat: add --timeout to bound a run and raise provider caps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every provider shipped an invisible hard cap and a long review had no recourse: the CLI-tool providers kill their subprocess after 5 minutes, ollama's http.Client after 5, and the Anthropic SDK refuses outright past 10 ("streaming is required for operations that may take longer than 10 minutes"). doctor's per-provider probe fast-fails at 5 seconds. A context deadline alone does not fix this — it can only shorten a run. clireview derives its child context from the caller's, so min(5m, 20m) still dies at five; http.Client.Timeout fires independently of ctx; and the SDK's ceiling is a pre-flight refusal, not a wait. So the resolved value is also handed down to the provider through a new optional provider.TimeoutSetter (the same additive marker shape as PlainTextEmitter), implemented by exactly clireview, ollama and anthropic — the three with a cap to raise — and reached only through the newProviderWithTimeout / setup.TestConnectionTimeout seams. --timeout accepts a Go duration (90s, 10m, 1h30m) or a bare whole number of seconds (600), since that is what CI authors type. Resolution is --timeout > review.timeout > built-in, so --timeout 0 restores the built-ins for a single run. Invalid or negative values fail in resolveContext, before the diff is read. Unset, behavior is unchanged. The budget covers the whole run — diff, provider call, render, and time spent at a confirmation prompt. doctor keeps its impatient 5s default but it is now overridable, because on a slow link 5 seconds reports merely slow as unreachable. mcp gets a per-tool-call budget rather than a deadline on the long-lived server, by preserving global.timeout across the runReviewForMCP flag reset — which also fixes guard --timeout silently losing the flag. Expiry checks the run's own ctx (providers report deadlines in three different dialects) and rewrites the error, so a self-inflicted deadline is never mistaken for a provider outage. Exit code stays 1; the cache key is unchanged. See ADR-0038. --- CHANGELOG.md | 23 +++ README.md | 31 ++- internal/cli/commit.go | 9 +- internal/cli/compress.go | 7 +- internal/cli/config.go | 14 +- internal/cli/config_test.go | 31 +++ internal/cli/context.go | 16 ++ internal/cli/doctor.go | 13 +- internal/cli/guard.go | 5 + internal/cli/integration_test.go | 58 ++++++ internal/cli/leaks.go | 3 + internal/cli/map.go | 4 + internal/cli/mcp.go | 6 + internal/cli/providers.go | 6 +- internal/cli/remote_pr.go | 21 +- internal/cli/review.go | 17 +- internal/cli/root.go | 2 + internal/cli/summary.go | 9 +- internal/cli/timeout.go | 114 +++++++++++ internal/cli/timeout_test.go | 180 ++++++++++++++++++ internal/config/config.go | 14 ++ internal/doctor/checks.go | 5 +- internal/doctor/doctor.go | 19 ++ internal/doctor/doctor_test.go | 19 ++ internal/i18n/messages.en.yml | 2 + internal/i18n/messages.tr.yml | 2 + internal/provider/anthropic/anthropic_test.go | 28 +++ internal/provider/anthropic/client.go | 31 ++- internal/provider/clireview/clireview.go | 12 ++ internal/provider/clireview/clireview_test.go | 38 ++++ internal/provider/ollama/client.go | 11 ++ internal/provider/ollama/ollama_test.go | 26 +++ internal/provider/provider.go | 25 ++- internal/setup/test.go | 16 ++ 34 files changed, 782 insertions(+), 35 deletions(-) create mode 100644 internal/cli/timeout.go create mode 100644 internal/cli/timeout_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index dfe0a21..2744539 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,29 @@ and the project adheres to [Semantic Versioning 2.0.0](https://semver.org/spec/v ## [Unreleased] +### Added +- **`--timeout` — bound a run, and raise the ceilings that used to end it early + (ADR-0038).** Every provider shipped a hard, invisible cap: the CLI-tool + providers killed their subprocess after 5 minutes, ollama's HTTP client after + 5, and the Anthropic SDK refuses a non-streaming request that could exceed 10. + A large diff or a slow local model hit those, and there was no way to ask for + more time. `--timeout ` now bounds the **whole command run** — diff, + provider call, render, and time spent at a confirmation prompt — and, crucially, + hands the value down to the provider, which is the only way to *lengthen* a run + (a context deadline can only cut one short). Accepts a Go duration (`90s`, + `10m`, `1h30m`) or a bare number of seconds (`600`) so CI can say + `--timeout 600`. Resolution is `--timeout` > `review.timeout` config > the + built-in, and `--timeout 0` restores the built-ins for a single run. Applies to + every command that can spend real time, including `doctor` (whose provider + probes otherwise fast-fail at 5 seconds) and `providers test`; on + `commitbrief mcp` it becomes a per-tool-call budget instead of a lifetime for + the long-lived server. Expiring is a normal exit-1 failure with a message that + names the duration and points back at the flag, so a self-inflicted deadline is + never mistaken for a provider outage. +- **`review.timeout` config key** — the persistent half of `--timeout`, settable + with `commitbrief config set review.timeout 15m`. Validated on write, so a + typo fails at `config set` rather than on every later run. + ## [1.15.0] - 2026-07-26 ### Added diff --git a/README.md b/README.md index dff151c..d48b25e 100644 --- a/README.md +++ b/README.md @@ -301,9 +301,37 @@ then exit — no provider call, no cost; honours `--output`), `--no-flaky` sandbox-rerun confirmation of flagged flaky tests; see below), `--no-architecture` (skip architecture-aware review; see below), `--update-baseline` / -`--no-baseline` (signal-control baseline; see below), `--color`. See +`--no-baseline` (signal-control baseline; see below), `--color`, +`--timeout ` (bound the whole run; see below). See `commitbrief --help`. +### Timeouts (`--timeout`, `review.timeout`) + +Every provider ships a built-in ceiling: the CLI-tool providers +(`claude-cli` / `gemini-cli` / `codex-cli`) kill their subprocess after 5 +minutes, ollama's HTTP client after 5, and the Anthropic SDK refuses a +non-streaming request that could run past 10. On a large diff or a slow +local model that is exactly when the run dies. + +`--timeout` replaces those ceilings for one run: + +```sh +commitbrief --staged --cli claude --timeout 20m # give the host CLI 20 minutes +commitbrief --staged --timeout 600 # bare integer = seconds +commitbrief config set review.timeout 15m # make it the default +``` + +The value is a Go duration (`90s`, `10m`, `1h30m`) or a whole number of +seconds. It bounds the **whole command run** — diff acquisition, provider +call, render, and any time you spend at a confirmation prompt — and it is +the one knob that can *lengthen* a run, since a deadline alone can only +cut one short. Resolution is `--timeout` → `review.timeout` → the +provider's built-in, so `--timeout 0` restores the built-ins for a single +run. Expiring is a normal failure: exit code 1 with a message naming the +duration. It also applies to `doctor` (whose provider probes otherwise +fast-fail at 5 seconds) and `providers test`; on `commitbrief mcp` it +becomes a per-tool-call budget rather than a lifetime for the server. + ### Flaky-test detection (deterministic, ADR-0022) Before the model is called, a **static pre-pass** scans the added lines of any @@ -843,6 +871,7 @@ review: baseline: true # apply the user-private signal-control baseline (ADR-0027); --no-baseline overrides per-run, --update-baseline rewrites it architecture: true # architecture-aware review (ADR-0030): read architecture.json into the prompt; --no-architecture overrides per-run architecture_file: "" # override the architecture.json discovery path (relative to repo root, or absolute); empty = auto-discover + timeout: "" # bound the whole run: "10m", "90s", "600" (seconds); empty/"0" = keep each provider's built-in cap; --timeout overrides per-run ``` ### Default command (`command.default`) diff --git a/internal/cli/commit.go b/internal/cli/commit.go index f386416..acccf39 100644 --- a/internal/cli/commit.go +++ b/internal/cli/commit.go @@ -62,11 +62,13 @@ func newCommitCmd() *cobra.Command { } func runCommit(cmd *cobra.Command) error { - ctx := cmd.Context() app, err := resolveContext(true) if err != nil { return err } + ctx, cancel := app.withTimeout(cmd.Context()) + defer cancel() + cmd.SetContext(ctx) // Resolve format + count (flag > config > built-in default) and validate // up front so a typo fails before any provider call. @@ -182,7 +184,7 @@ func runCommit(cmd *cobra.Command) error { prog.Resume() prog.Start(app.Catalog.T("progress.preparing")) - prov, err := provider.New(app.Config.Provider, app.Config.Providers[app.Config.Provider]) + prov, err := newProviderWithTimeout(app.Config.Provider, app.Config.Providers[app.Config.Provider], app.Timeout) if err != nil { prog.Fail(err) return err @@ -258,8 +260,9 @@ func runCommit(cmd *cobra.Command) error { FreeForm: true, }) if callErr != nil { + callErr = wrapTimeoutErr(ctx, fmt.Errorf("provider %s: %w", prov.Name(), callErr), app.Catalog, app.Timeout) prog.Fail(callErr) - return fmt.Errorf("provider %s: %w", prov.Name(), callErr) + return callErr } prog.Finish() content = resp.Content diff --git a/internal/cli/compress.go b/internal/cli/compress.go index 82539f7..b0419de 100644 --- a/internal/cli/compress.go +++ b/internal/cli/compress.go @@ -13,7 +13,6 @@ import ( "github.com/spf13/cobra" "github.com/CommitBrief/commitbrief/internal/compress" - "github.com/CommitBrief/commitbrief/internal/provider" "github.com/CommitBrief/commitbrief/internal/rules" "github.com/CommitBrief/commitbrief/internal/ui" ) @@ -39,6 +38,9 @@ func newCompressCmd() *cobra.Command { if err != nil { return err } + ctx, cancel := app.withTimeout(cmd.Context()) + defer cancel() + cmd.SetContext(ctx) level, err := compress.ParseLevel(levelFlag) if err != nil { @@ -54,7 +56,7 @@ func newCompressCmd() *cobra.Command { return fmt.Errorf("compress: read %s: %w", rulesPath, err) } - prov, err := provider.New(app.Config.Provider, app.Config.Providers[app.Config.Provider]) + prov, err := newProviderWithTimeout(app.Config.Provider, app.Config.Providers[app.Config.Provider], app.Timeout) if err != nil { return err } @@ -79,6 +81,7 @@ func newCompressCmd() *cobra.Command { Model: model, }) if err != nil { + err = wrapTimeoutErr(ctx, err, app.Catalog, app.Timeout) prog.Fail(err) return err } diff --git a/internal/cli/config.go b/internal/cli/config.go index fc0dba9..e57bf55 100644 --- a/internal/cli/config.go +++ b/internal/cli/config.go @@ -252,8 +252,10 @@ func configFieldGet(cfg *config.Config, path string) (string, error) { return strconv.Itoa(cfg.Review.SandboxRerun), nil case "sandbox_command": return strings.Join(cfg.Review.SandboxCommand, " "), nil + case "timeout": + return cfg.Review.Timeout, nil default: - return "", fmt.Errorf("config: unknown field %q in review (allowed: flaky, baseline, architecture, architecture_file, sandbox_rerun, sandbox_command)", parts[1]) + return "", fmt.Errorf("config: unknown field %q in review (allowed: flaky, baseline, architecture, architecture_file, sandbox_rerun, sandbox_command, timeout)", parts[1]) } case "version": @@ -477,8 +479,16 @@ func configFieldSet(cfg *config.Config, path, value string) error { // the YAML directly, mirroring guard.secret_patterns and // per-model pricing — the other non-scalar surfaces. return errors.New("config: review.sandbox_command is a list of argv elements; edit the config file directly (commitbrief config show prints its path)") + case "timeout": + // Validate before writing so the YAML never grows a value that + // would fail every subsequent run. The stored form is the user's + // spelling ("10m", "600"); parseTimeout normalizes at read time. + if _, err := parseTimeout(value); err != nil { + return fmt.Errorf("config: review.timeout: %w", err) + } + cfg.Review.Timeout = strings.TrimSpace(value) default: - return fmt.Errorf("config: unknown field %q in review (allowed: flaky, baseline, architecture, architecture_file, sandbox_rerun, sandbox_command)", parts[1]) + return fmt.Errorf("config: unknown field %q in review (allowed: flaky, baseline, architecture, architecture_file, sandbox_rerun, sandbox_command, timeout)", parts[1]) } return nil diff --git a/internal/cli/config_test.go b/internal/cli/config_test.go index 5699c49..705318d 100644 --- a/internal/cli/config_test.go +++ b/internal/cli/config_test.go @@ -437,3 +437,34 @@ func loadCfg(t *testing.T, home string) *config.Config { } return &cfg } + +func TestConfigReviewTimeoutRoundTrips(t *testing.T) { + e := newCLIEnv(t) + if err := e.run("config", "get", "review.timeout"); err != nil { + t.Fatalf("config get review.timeout: %v", err) + } + if got := strings.TrimSpace(e.out.String()); got != "" { + t.Errorf("review.timeout default = %q, want empty (no deadline)", got) + } + + if err := e.run("config", "set", "review.timeout", "10m"); err != nil { + t.Fatalf("config set review.timeout 10m: %v", err) + } + cfg := loadCfg(t, e.homeDir) + if cfg.Review.Timeout != "10m" { + t.Errorf("review.timeout = %q, want %q", cfg.Review.Timeout, "10m") + } +} + +func TestConfigReviewTimeoutRejectsGarbage(t *testing.T) { + // Validating on write keeps the YAML from growing a value that would + // fail every later run — the failure belongs at `config set` time. + e := newCLIEnv(t) + err := e.run("config", "set", "review.timeout", "ten-minutes") + if err == nil { + t.Fatal("want error for an unparseable review.timeout, got nil") + } + if !strings.Contains(err.Error(), "review.timeout") { + t.Errorf("error %q should name the key", err.Error()) + } +} diff --git a/internal/cli/context.go b/internal/cli/context.go index b8686c9..3c043f2 100644 --- a/internal/cli/context.go +++ b/internal/cli/context.go @@ -5,6 +5,7 @@ package cli import ( "fmt" "os" + "time" "github.com/CommitBrief/commitbrief/internal/config" "github.com/CommitBrief/commitbrief/internal/git" @@ -22,6 +23,11 @@ type appContext struct { RawGlobal *config.Config Lang lang.Resolution Catalog *i18n.Catalog + // Timeout is the resolved --timeout / review.timeout value; zero means + // "no deadline, keep every built-in provider cap" (the historical + // behavior). Consumed by appContext.withTimeout and + // newProviderWithTimeout. + Timeout time.Duration } func resolveContext(requireRepo bool) (*appContext, error) { @@ -70,6 +76,15 @@ func resolveContext(requireRepo bool) (*appContext, error) { cfg.Providers[cfg.Provider] = pc } + // Timeout resolution is deliberately here, not at the call site: every + // command that can spend real time reads app.Timeout, and resolving it + // once means a malformed value fails the run before the diff is read, + // let alone sent to a provider. + timeout, err := resolveTimeout(global.timeout, cfg) + if err != nil { + return nil, err + } + // Language resolution (ADR-0021) is independent of the merged config: it // reads the raw per-file configs so each level (--lang flag → repo → user // → English) is judged on its own value, with invalid/empty values falling @@ -95,6 +110,7 @@ func resolveContext(requireRepo bool) (*appContext, error) { RawGlobal: rawGlobal, Lang: langRes, Catalog: cat, + Timeout: timeout, }, nil } diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index bb9b404..a0d2881 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -33,13 +33,16 @@ run produces no output.`, if err != nil { return err } + ctx, cancel := app.withTimeout(cmd.Context()) + defer cancel() runner := &doctor.Runner{ - RepoRoot: app.RepoRoot, - Home: userHome(), - Config: app.Config, - Catalog: app.Catalog, + RepoRoot: app.RepoRoot, + Home: userHome(), + Config: app.Config, + Catalog: app.Catalog, + ConnTimeout: app.Timeout, } - results := runner.RunAll(cmd.Context()) + results := runner.RunAll(ctx) summary := doctor.Summarize(results) w := cmd.OutOrStdout() diff --git a/internal/cli/guard.go b/internal/cli/guard.go index 8d50d85..418506f 100644 --- a/internal/cli/guard.go +++ b/internal/cli/guard.go @@ -82,6 +82,11 @@ func runGuard(cmd *cobra.Command) error { if err != nil { return err } + // Bounds the whole gate, including the review it drives through the MCP + // seam (which reads cmd.Context()). + ctx, cancel := app.withTimeout(cmd.Context()) + defer cancel() + cmd.SetContext(ctx) policyPath := resolveGuardPolicyPath(cmd, app.RepoRoot) pol, err := policy.Load(policyPath) diff --git a/internal/cli/integration_test.go b/internal/cli/integration_test.go index 5ea1d75..7b72fb5 100644 --- a/internal/cli/integration_test.go +++ b/internal/cli/integration_test.go @@ -2559,3 +2559,61 @@ func TestMapExitsZeroOnSuccess(t *testing.T) { t.Fatalf("map must exit 0 on a successful render: %v", err) } } + +// ---------- --timeout ---------- + +func TestTimeoutFlagRunsCleanWhenGenerous(t *testing.T) { + // A generous budget must be invisible: the review completes exactly as + // it does without the flag. + e := newCLIEnv(t) + if err := e.run("--staged", "--no-cache", "--timeout", "10m"); err != nil { + t.Fatalf("--timeout 10m should not disturb the pipeline: %v\nstderr:\n%s", err, e.errOut.String()) + } +} + +func TestTimeoutFlagAcceptsBareSeconds(t *testing.T) { + // `--timeout 600` is what a CI author types; rejecting it for a + // missing unit would be a papercut. + e := newCLIEnv(t) + if err := e.run("--staged", "--no-cache", "--timeout", "600"); err != nil { + t.Fatalf("--timeout 600 should parse as 600s: %v\nstderr:\n%s", err, e.errOut.String()) + } +} + +func TestTimeoutFlagRejectsGarbageBeforeAnyProviderCall(t *testing.T) { + // resolveContext validates the value, so the run dies before the diff + // is even read — no tokens, no cost. + e := newCLIEnv(t) + err := e.run("--staged", "--no-cache", "--timeout", "abc") + if err == nil { + t.Fatal("want error for an unparseable --timeout, got nil") + } + if !strings.Contains(err.Error(), "abc") { + t.Errorf("error %q should quote the offending value", err.Error()) + } +} + +func TestTimeoutFlagRejectsNegative(t *testing.T) { + e := newCLIEnv(t) + // `--` stops cobra flag parsing so the negative value reaches + // validation as the flag's value rather than a shorthand flag. + err := e.run("--staged", "--no-cache", "--timeout=-5s") + if err == nil { + t.Fatal("want error for a negative --timeout, got nil") + } + if !strings.Contains(err.Error(), "negative") { + t.Errorf("error %q should say it cannot be negative", err.Error()) + } +} + +func TestTimeoutConfigDrivesRunWithoutFlag(t *testing.T) { + // review.timeout is the persistent half of the pair; a bad stored + // value must fail the run the same way a bad flag does. + e := newCLIEnv(t) + if err := e.run("config", "set", "review.timeout", "10m"); err != nil { + t.Fatalf("config set review.timeout: %v", err) + } + if err := e.run("--staged", "--no-cache"); err != nil { + t.Fatalf("a configured review.timeout should not disturb the pipeline: %v\nstderr:\n%s", err, e.errOut.String()) + } +} diff --git a/internal/cli/leaks.go b/internal/cli/leaks.go index 495f92b..5e989f2 100644 --- a/internal/cli/leaks.go +++ b/internal/cli/leaks.go @@ -75,6 +75,9 @@ func runLeaks(cmd *cobra.Command, f leaksFlags, args []string) error { if err != nil { return err } + tctx, tcancel := app.withTimeout(cmd.Context()) + defer tcancel() + cmd.SetContext(tctx) if f.noWorktree && f.noHistory { return errors.New(app.Catalog.T("leaks.nothing_to_scan")) } diff --git a/internal/cli/map.go b/internal/cli/map.go index 139445e..508027f 100644 --- a/internal/cli/map.go +++ b/internal/cli/map.go @@ -71,6 +71,10 @@ func runMap(cmd *cobra.Command, f mapFlags, args []string) error { return err } + ctx, cancel := app.withTimeout(cmd.Context()) + defer cancel() + cmd.SetContext(ctx) + // map draws a graph, not findings. Rejecting the findings-oriented output // flags up front beats emitting something that isn't what the flag // promised. A graph JSON would be a new semver-locked schema; that is diff --git a/internal/cli/mcp.go b/internal/cli/mcp.go index 5ca879d..9a674f7 100644 --- a/internal/cli/mcp.go +++ b/internal/cli/mcp.go @@ -289,6 +289,12 @@ func runReviewForMCP(ctx context.Context, args reviewToolArgs) (string, string, // tool error, which is the correct, safe behavior. global.json = true global.quiet = true + // --timeout survives the reset. For `guard` it is the flag the user just + // typed; for a long-lived `commitbrief mcp --timeout 10m` it becomes a + // PER-TOOL-CALL budget, which is the only sane reading — the server + // itself must never carry a deadline. When neither set it, runReview + // still falls back to review.timeout from config. + global.timeout = savedGlobal.timeout // The host is an agent, not a person at a TTY. Executing repository code // (sandbox rerun) requires a human in the loop, so it is forced off here // regardless of --sandbox-rerun or review.sandbox_rerun. This is a diff --git a/internal/cli/providers.go b/internal/cli/providers.go index 46f83cf..898e4c6 100644 --- a/internal/cli/providers.go +++ b/internal/cli/providers.go @@ -162,6 +162,8 @@ func newProvidersTestCmd() *cobra.Command { if !isRegistered(name) { return errors.New(app.Catalog.T("providers.test.unknown", name, provider.Names())) } + ctx, cancel := app.withTimeout(cmd.Context()) + defer cancel() pc := app.Config.Providers[name] // Single network step, shown through the shared staged-tree // progress (a spinner while the ping is in flight). Close keeps @@ -170,8 +172,8 @@ func newProvidersTestCmd() *cobra.Command { defer prog.Close() prog.Start(app.Catalog.T("providers.test.pinging", name)) start := time.Now() - if err := setup.TestConnection(cmd.Context(), name, pc); err != nil { - e := errors.New(app.Catalog.T("providers.test.failed", name, err.Error())) + if err := setup.TestConnectionTimeout(ctx, name, pc, app.Timeout); err != nil { + e := wrapTimeoutErr(ctx, errors.New(app.Catalog.T("providers.test.failed", name, err.Error())), app.Catalog, app.Timeout) prog.Fail(e) return e } diff --git a/internal/cli/remote_pr.go b/internal/cli/remote_pr.go index ae6919a..0b4de04 100644 --- a/internal/cli/remote_pr.go +++ b/internal/cli/remote_pr.go @@ -101,12 +101,13 @@ func runRemotePR(cmd *cobra.Command, prID string, f remotePRFlags, runner remote return runRemotePRLocal(cmd, prID, f, runner) } - ctx := cmd.Context() - app, err := resolveContext(false) if err != nil { return err } + ctx, cancel := app.withTimeout(cmd.Context()) + defer cancel() + cmd.SetContext(ctx) cat := app.Catalog // Local-render flags have no meaning here — the output channel is GitHub. @@ -136,7 +137,7 @@ func runRemotePR(cmd *cobra.Command, prID string, f remotePRFlags, runner remote return errors.New(cat.T("remote.gh_missing")) } - prov, err := provider.New(app.Config.Provider, app.Config.Providers[app.Config.Provider]) + prov, err := newProviderWithTimeout(app.Config.Provider, app.Config.Providers[app.Config.Provider], app.Timeout) if err != nil { return err } @@ -231,11 +232,13 @@ func runRemotePR(cmd *cobra.Command, prID string, f remotePRFlags, runner remote // aborts (you can't fix another author's PR locally, and aborting a // read-only review is unhelpful), matching the posting path's posture. func runRemotePRLocal(cmd *cobra.Command, prID string, f remotePRFlags, runner remote.Runner) error { - ctx := cmd.Context() app, err := resolveContext(false) if err != nil { return err } + ctx, cancel := app.withTimeout(cmd.Context()) + defer cancel() + cmd.SetContext(ctx) cat := app.Catalog if _, _, err := parseMinSeverity(global.minSeverity); err != nil { @@ -253,7 +256,7 @@ func runRemotePRLocal(cmd *cobra.Command, prID string, f remotePRFlags, runner r return errors.New(cat.T("remote.gh_missing")) } - prov, err := provider.New(app.Config.Provider, app.Config.Providers[app.Config.Provider]) + prov, err := newProviderWithTimeout(app.Config.Provider, app.Config.Providers[app.Config.Provider], app.Timeout) if err != nil { return err } @@ -427,8 +430,9 @@ func runRemotePRLocal(cmd *cobra.Command, prID string, f remotePRFlags, runner r if plainText { resp, callErr := prov.Review(ctx, req) if callErr != nil { + callErr = wrapTimeoutErr(ctx, fmt.Errorf("provider %s: %w", prov.Name(), callErr), cat, app.Timeout) prog.Fail(callErr) - return fmt.Errorf("provider %s: %w", prov.Name(), callErr) + return callErr } content, usage, format = resp.Content, resp.Usage, cache.FormatPlainText } else { @@ -437,8 +441,9 @@ func runRemotePRLocal(cmd *cobra.Command, prID string, f remotePRFlags, runner r prog.Start(cat.T("progress.retrying")) }) if callErr != nil { + callErr = wrapTimeoutErr(ctx, fmt.Errorf("provider %s: %w", prov.Name(), callErr), cat, app.Timeout) prog.Fail(callErr) - return fmt.Errorf("provider %s: %w", prov.Name(), callErr) + return callErr } content, usage, format = outcome.Content, outcome.Usage, outcome.Format retries, degrade = outcome.Retries, outcome.DegradeReason @@ -603,7 +608,7 @@ func reviewOnePRDiff(ctx context.Context, runner remote.Runner, prID string, f r start := time.Now() outcome, err := tryStructuredReview(ctx, prov, req, func() {}) if err != nil { - return prReviewResult{}, err + return prReviewResult{}, wrapTimeoutErr(ctx, err, app.Catalog, app.Timeout) } latency := time.Since(start) if outcome.Format != cache.FormatJSON { diff --git a/internal/cli/review.go b/internal/cli/review.go index 3571482..66e3eaf 100644 --- a/internal/cli/review.go +++ b/internal/cli/review.go @@ -49,11 +49,18 @@ func bindScopeFlags(cmd *cobra.Command) { } func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) error { - ctx := cmd.Context() app, err := resolveContext(true) if err != nil { return err } + // --timeout / review.timeout bounds the WHOLE run, not just the + // provider round-trip: a review that stalls on a huge diff or on a + // confirmation nobody is there to answer is just as stuck. Pushed back + // onto cmd so the helpers that read cmd.Context() (sandbox rerun, + // suggest-commit) inherit the same deadline without threading it. + ctx, cancel := app.withTimeout(cmd.Context()) + defer cancel() + cmd.SetContext(ctx) // Validate --min-severity up front so a typo fails fast instead of // silently showing every finding after a paid provider round-trip. @@ -238,7 +245,7 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er prog.Start(app.Catalog.T("progress.preparing")) - prov, err := provider.New(app.Config.Provider, app.Config.Providers[app.Config.Provider]) + prov, err := newProviderWithTimeout(app.Config.Provider, app.Config.Providers[app.Config.Provider], app.Timeout) if err != nil { prog.Fail(err) return err @@ -462,8 +469,9 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er // review which we stream straight to stdout after Clear. resp, callErr := prov.Review(ctx, req) if callErr != nil { + callErr = wrapTimeoutErr(ctx, fmt.Errorf("provider %s: %w", prov.Name(), callErr), app.Catalog, app.Timeout) prog.Fail(callErr) - return fmt.Errorf("provider %s: %w", prov.Name(), callErr) + return callErr } content, usage, format = resp.Content, resp.Usage, cache.FormatPlainText } else { @@ -477,8 +485,9 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er prog.Start(app.Catalog.T("progress.retrying")) }) if callErr != nil { + callErr = wrapTimeoutErr(ctx, fmt.Errorf("provider %s: %w", prov.Name(), callErr), app.Catalog, app.Timeout) prog.Fail(callErr) - return fmt.Errorf("provider %s: %w", prov.Name(), callErr) + return callErr } content, usage, format = outcome.Content, outcome.Usage, outcome.Format retries, degrade = outcome.Retries, outcome.DegradeReason diff --git a/internal/cli/root.go b/internal/cli/root.go index abe9fe8..67bb789 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -54,6 +54,7 @@ type globalFlags struct { provider string model string color string + timeout string // --timeout ; bounds the whole command run and raises provider-internal caps; "" / "0" = built-in behavior cli string // --cli ; shorthand that resolves to provider "-cli" withContext bool // --with-context; CLI providers only — let the host CLI read project files beyond the diff (ADR-0017) showPrompt bool // --show-prompt; print the assembled system+user prompt and exit (no provider call) @@ -140,6 +141,7 @@ func newRootCmd() *cobra.Command { flags.StringVar(&global.provider, "provider", "", "override configured provider") flags.StringVar(&global.model, "model", "", "override configured model") flags.StringVar(&global.color, "color", "auto", "color output: auto, always, never") + flags.StringVar(&global.timeout, "timeout", "", "bound the whole run to this `duration` — 90s, 10m, 1h30m, or a bare number of seconds (600). Covers diff, provider call, render, and time spent at a confirmation prompt. Also RAISES the provider's own cap (claude/gemini/codex-cli 5m, ollama 5m, Anthropic SDK 10m), which a deadline alone cannot do. Unset or 0 keeps those built-ins. Resolution: --timeout → review.timeout → built-in") flags.StringSliceVarP(&global.files, "file", "f", nil, "review only these files or globs (e.g. `*.go`, `internal/**/*.ts`; repeatable, one pattern per flag — patterns can't be comma-joined); combines with the active scope flag") flags.StringSliceVarP(&global.dirs, "dir", "d", nil, "review only files under these directories or matching dir globs (e.g. `internal/**`; repeatable, one pattern per flag); combines with the active scope flag") flags.StringSliceVar(&global.excludeFiles, "exclude-file", nil, "skip these files or globs (repeatable, one pattern per flag); same matching rules as --file, applied after it so an exclusion wins") diff --git a/internal/cli/summary.go b/internal/cli/summary.go index 8ab9e11..1235976 100644 --- a/internal/cli/summary.go +++ b/internal/cli/summary.go @@ -56,11 +56,13 @@ func newSummaryCmd() *cobra.Command { } func runSummary(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) error { - ctx := cmd.Context() app, err := resolveContext(true) if err != nil { return err } + ctx, cancel := app.withTimeout(cmd.Context()) + defer cancel() + cmd.SetContext(ctx) // summary emits prose, not findings. Reject the structured-output flags // (--json/--markdown drive the findings renderers) and the findings-only @@ -156,7 +158,7 @@ func runSummary(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) e prog.Resume() prog.Start(app.Catalog.T("progress.preparing")) - prov, err := provider.New(app.Config.Provider, app.Config.Providers[app.Config.Provider]) + prov, err := newProviderWithTimeout(app.Config.Provider, app.Config.Providers[app.Config.Provider], app.Timeout) if err != nil { prog.Fail(err) return err @@ -256,8 +258,9 @@ func runSummary(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) e }, }) if callErr != nil { + callErr = wrapTimeoutErr(ctx, fmt.Errorf("provider %s: %w", prov.Name(), callErr), app.Catalog, app.Timeout) prog.Fail(callErr) - return fmt.Errorf("provider %s: %w", prov.Name(), callErr) + return callErr } prog.Finish() content = resp.Content diff --git a/internal/cli/timeout.go b/internal/cli/timeout.go new file mode 100644 index 0000000..833193e --- /dev/null +++ b/internal/cli/timeout.go @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package cli + +import ( + "context" + "errors" + "fmt" + "strconv" + "strings" + "time" + + "github.com/CommitBrief/commitbrief/internal/config" + "github.com/CommitBrief/commitbrief/internal/i18n" + "github.com/CommitBrief/commitbrief/internal/provider" +) + +// parseTimeout maps a raw --timeout / review.timeout value to a duration. +// +// Two spellings are accepted on purpose: a Go duration ("90s", "10m", +// "1h30m") reads best in a config file, while a bare integer is what CI +// authors reach for — so "600" is taken as 600 seconds rather than +// rejected for a missing unit. +// +// "" and "0" both mean "no deadline"; the zero return is the documented +// signal for "leave every built-in provider timeout exactly as it is", +// which keeps an unset flag byte-for-byte backwards compatible. A +// negative or unparseable value is an error so a typo surfaces before +// the paid round-trip instead of silently disabling the guard. +func parseTimeout(raw string) (time.Duration, error) { + s := strings.TrimSpace(raw) + if s == "" { + return 0, nil + } + if d, err := time.ParseDuration(s); err == nil { + if d < 0 { + return 0, fmt.Errorf("invalid timeout %q (must not be negative)", raw) + } + return d, nil + } + secs, err := strconv.Atoi(s) + if err != nil { + return 0, fmt.Errorf("invalid timeout %q (expected a duration like 90s, 10m, 1h30m, or a whole number of seconds)", raw) + } + if secs < 0 { + return 0, fmt.Errorf("invalid timeout %q (must not be negative)", raw) + } + return time.Duration(secs) * time.Second, nil +} + +// resolveTimeout applies the precedence chain --timeout > review.timeout +// > built-in (0 = off). An explicit `--timeout 0` therefore cancels a +// configured value for one run, which is the only way to get the built-in +// provider defaults back without editing config. +func resolveTimeout(flagVal string, cfg *config.Config) (time.Duration, error) { + if strings.TrimSpace(flagVal) != "" { + return parseTimeout(flagVal) + } + if cfg == nil { + return 0, nil + } + return parseTimeout(cfg.Review.Timeout) +} + +// withTimeout derives the command's working context. With no timeout +// resolved it is a plain cancel-only child (same lifetime as today); with +// one it carries the deadline that bounds the WHOLE command run — diff +// acquisition, prompt build, provider call, render, and any interactive +// confirmation in between. Callers must defer the returned cancel. +func (app *appContext) withTimeout(ctx context.Context) (context.Context, context.CancelFunc) { + if app == nil || app.Timeout <= 0 { + return context.WithCancel(ctx) + } + return context.WithTimeout(ctx, app.Timeout) +} + +// wrapTimeoutErr replaces a provider/pipeline error with a self-explaining +// timeout message when the run's own deadline is what actually fired. +// Providers report a deadline in their own dialect (a wrapped +// context.DeadlineExceeded, an SDK error string, or clireview's formatted +// "timed out after") so checking ctx is the one reliable signal. A nil +// error, or a failure with the deadline still unexpired, passes through +// untouched. +func wrapTimeoutErr(ctx context.Context, err error, cat *i18n.Catalog, d time.Duration) error { + if err == nil || d <= 0 { + return err + } + if !errors.Is(ctx.Err(), context.DeadlineExceeded) { + return err + } + if cat == nil { + return fmt.Errorf("timed out after %s; pass a larger --timeout (or set review.timeout) to allow more time", d) + } + return errors.New(cat.T("timeout.exceeded", d.String())) +} + +// newProviderWithTimeout builds a provider and, when a timeout is +// resolved, hands it down to the providers that impose a hard cap of +// their own. A context deadline can only SHORTEN a run — the 5-minute +// clireview cap, ollama's http.Client timeout and the Anthropic SDK's +// 10-minute non-streaming ceiling would still fire first — so raising +// --timeout has to reach the provider itself. See provider.TimeoutSetter. +func newProviderWithTimeout(name string, cfg config.ProviderConfig, d time.Duration) (provider.Provider, error) { + p, err := provider.New(name, cfg) + if err != nil { + return nil, err + } + if d > 0 { + if ts, ok := p.(provider.TimeoutSetter); ok { + ts.SetTimeout(d) + } + } + return p, nil +} diff --git a/internal/cli/timeout_test.go b/internal/cli/timeout_test.go new file mode 100644 index 0000000..31fc532 --- /dev/null +++ b/internal/cli/timeout_test.go @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package cli + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/CommitBrief/commitbrief/internal/config" + "github.com/CommitBrief/commitbrief/internal/i18n" +) + +// ---------- parseTimeout ---------- + +func TestParseTimeoutAcceptedValues(t *testing.T) { + cases := []struct { + in string + want time.Duration + }{ + {"", 0}, + {"0", 0}, + {"0s", 0}, + {"90s", 90 * time.Second}, + {"10m", 10 * time.Minute}, + {"1h30m", 90 * time.Minute}, + {"600", 600 * time.Second}, // bare integer = seconds (CI ergonomics) + {" 5m ", 5 * time.Minute}, + {"1500ms", 1500 * time.Millisecond}, + } + for _, tc := range cases { + t.Run(tc.in, func(t *testing.T) { + got, err := parseTimeout(tc.in) + if err != nil { + t.Fatalf("parseTimeout(%q) returned error: %v", tc.in, err) + } + if got != tc.want { + t.Errorf("parseTimeout(%q) = %v, want %v", tc.in, got, tc.want) + } + }) + } +} + +func TestParseTimeoutRejectsBadValues(t *testing.T) { + bad := []string{"abc", "-5s", "-600", "5 minutes", "m10", "1.5.2"} + for _, in := range bad { + t.Run(in, func(t *testing.T) { + if _, err := parseTimeout(in); err == nil { + t.Fatalf("parseTimeout(%q) accepted an invalid value", in) + } + }) + } +} + +// ---------- resolveTimeout ---------- + +func TestResolveTimeoutPrecedence(t *testing.T) { + cfgWith := func(v string) *config.Config { + c := config.Default() + c.Review.Timeout = v + return c + } + cases := []struct { + name string + flag string + cfg *config.Config + want time.Duration + }{ + {"nothing set", "", config.Default(), 0}, + {"config only", "", cfgWith("10m"), 10 * time.Minute}, + {"flag only", "45s", config.Default(), 45 * time.Second}, + {"flag beats config", "2m", cfgWith("10m"), 2 * time.Minute}, + {"explicit zero cancels config", "0", cfgWith("10m"), 0}, + {"nil config", "30s", nil, 30 * time.Second}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := resolveTimeout(tc.flag, tc.cfg) + if err != nil { + t.Fatalf("resolveTimeout: %v", err) + } + if got != tc.want { + t.Errorf("resolveTimeout(%q) = %v, want %v", tc.flag, got, tc.want) + } + }) + } +} + +func TestResolveTimeoutSurfacesBadConfigValue(t *testing.T) { + cfg := config.Default() + cfg.Review.Timeout = "ten minutes" + if _, err := resolveTimeout("", cfg); err == nil { + t.Fatal("want error for an unparseable review.timeout, got nil") + } +} + +// ---------- withTimeout ---------- + +func TestWithTimeoutSetsDeadlineOnlyWhenConfigured(t *testing.T) { + off := &appContext{} + ctx, cancel := off.withTimeout(context.Background()) + defer cancel() + if _, ok := ctx.Deadline(); ok { + t.Error("unset timeout must not attach a deadline") + } + + on := &appContext{Timeout: time.Minute} + ctx2, cancel2 := on.withTimeout(context.Background()) + defer cancel2() + deadline, ok := ctx2.Deadline() + if !ok { + t.Fatal("configured timeout must attach a deadline") + } + if remaining := time.Until(deadline); remaining <= 0 || remaining > time.Minute { + t.Errorf("deadline is %v away, want (0, 1m]", remaining) + } +} + +// ---------- wrapTimeoutErr ---------- + +func TestWrapTimeoutErr(t *testing.T) { + cat, err := i18n.Load("en") + if err != nil { + t.Fatal(err) + } + providerErr := errors.New("provider anthropic: connection reset") + + expired, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancel() + + t.Run("nil error passes through", func(t *testing.T) { + if got := wrapTimeoutErr(expired, nil, cat, time.Minute); got != nil { + t.Errorf("got %v, want nil", got) + } + }) + + t.Run("live context passes through", func(t *testing.T) { + got := wrapTimeoutErr(context.Background(), providerErr, cat, time.Minute) + if got != providerErr { + t.Errorf("got %v, want the original error untouched", got) + } + }) + + t.Run("no timeout configured passes through", func(t *testing.T) { + got := wrapTimeoutErr(expired, providerErr, cat, 0) + if got != providerErr { + t.Errorf("got %v, want the original error untouched", got) + } + }) + + t.Run("expired deadline is rewritten", func(t *testing.T) { + got := wrapTimeoutErr(expired, providerErr, cat, 90*time.Second) + if got == nil { + t.Fatal("got nil, want a timeout error") + } + if !strings.Contains(got.Error(), "1m30s") { + t.Errorf("message %q should name the configured duration", got.Error()) + } + if !strings.Contains(got.Error(), "--timeout") { + t.Errorf("message %q should point at --timeout", got.Error()) + } + }) + + t.Run("nil catalog still explains itself", func(t *testing.T) { + got := wrapTimeoutErr(expired, providerErr, nil, 30*time.Second) + if got == nil || !strings.Contains(got.Error(), "30s") { + t.Errorf("got %v, want a message naming 30s", got) + } + }) +} + +// ---------- newProviderWithTimeout ---------- + +func TestNewProviderWithTimeoutRejectsUnknownProvider(t *testing.T) { + if _, err := newProviderWithTimeout("nope-not-a-provider", config.ProviderConfig{}, time.Minute); err == nil { + t.Fatal("want error for an unregistered provider, got nil") + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 82afab1..f943a61 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -53,6 +53,19 @@ type Config struct { // caller-provided), and only the flaky pre-pass is affected. Precedence is // --sandbox-rerun[=N] > review.sandbox_rerun config > built-in (0 / off). // +// Timeout bounds a whole command run — diff acquisition, prompt build, +// provider call, render, and any interactive confirmation in between. It +// is a STRING, not a duration, so the YAML stays readable (`timeout: +// "10m"`) instead of serializing as a nanosecond count; both a Go +// duration ("90s", "10m", "1h30m") and a bare whole number of seconds +// ("600") parse. Empty or "0" (the default) means no deadline, leaving +// each provider's built-in cap in place — the historical behaviour. +// Precedence is --timeout > review.timeout > built-in, so `--timeout 0` +// cancels a configured value for a single run. Beyond the deadline it +// also RAISES the caps that would otherwise fire first (clireview's 5 +// minutes, ollama's http.Client timeout, the Anthropic SDK's 10-minute +// non-streaming ceiling) via provider.TimeoutSetter. +// // SandboxCommand is the argv of the command that re-runs a single flagged // test in isolation (ADR-0033). Each element is a Go text/template over // {{.File}}, {{.Line}}, {{.Test}} and is passed to exec as an argv element @@ -66,6 +79,7 @@ type ReviewConfig struct { ArchitectureFile string `yaml:"architecture_file"` SandboxRerun int `yaml:"sandbox_rerun"` SandboxCommand []string `yaml:"sandbox_command"` + Timeout string `yaml:"timeout,omitempty"` } // CommitConfig sets defaults for the `commit` command (ADR-0019) so a repo diff --git a/internal/doctor/checks.go b/internal/doctor/checks.go index 04502c9..b75eeba 100644 --- a/internal/doctor/checks.go +++ b/internal/doctor/checks.go @@ -197,13 +197,14 @@ func (r *Runner) checkProviderConnections(ctx context.Context) []Result { wg.Add(1) go func(i int, providerName string) { defer wg.Done() - ctx2, cancel := context.WithTimeout(ctx, connectionTimeout) + budget := r.connTimeoutOrDefault() + ctx2, cancel := context.WithTimeout(ctx, budget) defer cancel() pc := r.Config.Providers[providerName] label := r.t("doctor.check.provider_connection", providerName) start := time.Now() - err := setup.TestConnection(ctx2, providerName, pc) + err := setup.TestConnectionTimeout(ctx2, providerName, pc, r.ConnTimeout) elapsed := time.Since(start).Round(time.Millisecond) if err != nil { results[i] = Result{Name: label, Status: StatusWarn, Detail: err.Error()} diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index ac83fa4..135aabc 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -14,6 +14,8 @@ package doctor import ( + "time" + "github.com/CommitBrief/commitbrief/internal/config" "github.com/CommitBrief/commitbrief/internal/i18n" ) @@ -80,6 +82,23 @@ type Runner struct { // Catalog provides i18n translations for check names. Nil falls // back to a no-op catalog (returns the key verbatim). Catalog *i18n.Catalog + + // ConnTimeout overrides the per-provider connection-probe budget with + // the user's resolved --timeout / review.timeout. Zero — the default — + // keeps the fast-failing [connectionTimeout]. It exists because the + // built-in 5 seconds reports a Warn on a link that is merely slow, and + // "my provider is unreachable" is exactly the wrong diagnosis to hand + // someone on a high-latency network. + ConnTimeout time.Duration +} + +// connTimeoutOrDefault resolves the per-provider probe budget: the +// caller's override when set, else the fast-fail built-in. +func (r *Runner) connTimeoutOrDefault() time.Duration { + if r.ConnTimeout > 0 { + return r.ConnTimeout + } + return connectionTimeout } // t looks up an i18n key with optional format args, defaulting to the diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 0e0ecbb..ac09e43 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -9,6 +9,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/CommitBrief/commitbrief/internal/config" "github.com/CommitBrief/commitbrief/internal/i18n" @@ -386,3 +387,21 @@ func TestStatusString(t *testing.T) { // guard against unused-import drift if I refactor away one of these // later — the test file itself is what justifies them. var _ = provider.ErrUnauthorized + +func TestConnTimeoutOrDefault(t *testing.T) { + r := minimalRunner(t) + if got := r.connTimeoutOrDefault(); got != connectionTimeout { + t.Errorf("unset ConnTimeout = %v, want the built-in %v", got, connectionTimeout) + } + // The built-in 5s reports "unreachable" for a link that is merely + // slow; --timeout is what lets a user on a high-latency network get a + // truthful answer. + r.ConnTimeout = 30 * time.Second + if got := r.connTimeoutOrDefault(); got != 30*time.Second { + t.Errorf("ConnTimeout override = %v, want 30s", got) + } + r.ConnTimeout = -time.Second + if got := r.connTimeoutOrDefault(); got != connectionTimeout { + t.Errorf("negative ConnTimeout = %v, want the built-in %v", got, connectionTimeout) + } +} diff --git a/internal/i18n/messages.en.yml b/internal/i18n/messages.en.yml index 0c859e0..76ac869 100644 --- a/internal/i18n/messages.en.yml +++ b/internal/i18n/messages.en.yml @@ -311,3 +311,5 @@ leaks.clean: "No credentials found." leaks.found: "%d possible credential(s) found. Rotate them — a key in git history stays reachable in every existing clone." leaks.nothing_to_scan: "--no-worktree and --no-history together leave nothing to scan." leaks.gate_failed: "failing: %d credential finding(s). Pass --fail-on none to report without failing." + +timeout.exceeded: "timed out after %s — the run was stopped by --timeout / review.timeout, not by the provider. Pass a larger --timeout (e.g. --timeout 20m) to allow more time, or --timeout 0 to fall back to the provider's own limit." diff --git a/internal/i18n/messages.tr.yml b/internal/i18n/messages.tr.yml index df229eb..8b7ffbf 100644 --- a/internal/i18n/messages.tr.yml +++ b/internal/i18n/messages.tr.yml @@ -310,3 +310,5 @@ leaks.clean: "Kimlik bilgisi bulunamadı." leaks.found: "%d olası kimlik bilgisi bulundu. Bunları döndürün (rotate) — git geçmişindeki bir anahtar mevcut tüm klonlarda erişilebilir kalır." leaks.nothing_to_scan: "--no-worktree ve --no-history birlikte kullanılınca taranacak bir şey kalmıyor." leaks.gate_failed: "başarısız: %d kimlik bilgisi bulgusu. Hata vermeden raporlamak için --fail-on none kullanın." + +timeout.exceeded: "%s sonunda zaman aşımına uğradı — koşuyu durduran --timeout / review.timeout, provider değil. Daha fazla süre için daha büyük bir --timeout verin (örn. --timeout 20m) ya da provider'ın kendi sınırına dönmek için --timeout 0 kullanın." diff --git a/internal/provider/anthropic/anthropic_test.go b/internal/provider/anthropic/anthropic_test.go index f587e0f..40e999f 100644 --- a/internal/provider/anthropic/anthropic_test.go +++ b/internal/provider/anthropic/anthropic_test.go @@ -10,6 +10,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/CommitBrief/commitbrief/internal/config" "github.com/CommitBrief/commitbrief/internal/provider" @@ -362,3 +363,30 @@ func TestTestConnectionSuccess(t *testing.T) { t.Errorf("TestConnection: %v", err) } } + +func TestSetTimeoutDrivesTheRequestOption(t *testing.T) { + // The SDK derives its own non-streaming timeout from max_tokens and + // refuses outright past 10 minutes ("streaming is required…"), so a + // user allowing 20 needs an explicit per-request timeout to get them. + p, err := New(config.ProviderConfig{APIKey: "sk-ant-test"}) + if err != nil { + t.Fatal(err) + } + c := p.(*Client) + if opts := c.requestOpts(); len(opts) != 0 { + t.Errorf("unset timeout should add no request options; got %d", len(opts)) + } + c.SetTimeout(20 * time.Minute) + if c.timeout != 20*time.Minute { + t.Errorf("timeout = %v, want 20m", c.timeout) + } + if opts := c.requestOpts(); len(opts) != 1 { + t.Errorf("configured timeout should add exactly one request option; got %d", len(opts)) + } + c.SetTimeout(0) + if c.timeout != 20*time.Minute { + t.Errorf("timeout = %v, want the previous 20m left untouched", c.timeout) + } +} + +var _ provider.TimeoutSetter = (*Client)(nil) diff --git a/internal/provider/anthropic/client.go b/internal/provider/anthropic/client.go index 461d6b3..7d29060 100644 --- a/internal/provider/anthropic/client.go +++ b/internal/provider/anthropic/client.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "strings" + "time" sdk "github.com/anthropics/anthropic-sdk-go" "github.com/anthropics/anthropic-sdk-go/option" @@ -27,6 +28,10 @@ type Client struct { sdk sdk.Client model string baseURL string + // timeout is the resolved --timeout / review.timeout value, applied + // per request via option.WithRequestTimeout. Zero leaves the SDK's own + // timeout policy in charge. See SetTimeout. + timeout time.Duration } func New(cfg config.ProviderConfig) (provider.Provider, error) { @@ -70,9 +75,31 @@ func (c *Client) Pricing(model string) provider.Pricing { return pricingFor(model) } +// SetTimeout implements provider.TimeoutSetter. The SDK derives its own +// non-streaming timeout from max_tokens and caps it at 10 minutes — +// worse, it refuses outright ("streaming is required for operations that +// may take longer than 10 minutes") rather than waiting. Passing an +// explicit request timeout short-circuits that calculation, so a user who +// allows 20 minutes actually gets them. A non-positive d is ignored. +func (c *Client) SetTimeout(d time.Duration) { + if d > 0 { + c.timeout = d + } +} + +// requestOpts returns the per-request options for a call: the resolved +// timeout when one is set, nothing otherwise (leaving SDK defaults +// untouched). +func (c *Client) requestOpts() []option.RequestOption { + if c.timeout <= 0 { + return nil + } + return []option.RequestOption{option.WithRequestTimeout(c.timeout)} +} + func (c *Client) Review(ctx context.Context, req provider.Request) (provider.Response, error) { params := c.buildParams(req) - msg, err := c.sdk.Messages.New(ctx, params) + msg, err := c.sdk.Messages.New(ctx, params, c.requestOpts()...) if err != nil { return provider.Response{}, mapError(err) } @@ -105,7 +132,7 @@ func (c *Client) TestConnection(ctx context.Context) error { sdk.NewUserMessage(sdk.NewTextBlock(testPingPrompt)), }, } - if _, err := c.sdk.Messages.New(ctx, params); err != nil { + if _, err := c.sdk.Messages.New(ctx, params, c.requestOpts()...); err != nil { return mapError(err) } return nil diff --git a/internal/provider/clireview/clireview.go b/internal/provider/clireview/clireview.go index 4e4d62f..5c3e8a7 100644 --- a/internal/provider/clireview/clireview.go +++ b/internal/provider/clireview/clireview.go @@ -128,6 +128,18 @@ func New(spec Spec) *Backend { // interface review.go uses to branch into the CLI-output path. func (b *Backend) EmitsPlainText() {} +// SetTimeout implements provider.TimeoutSetter, replacing the Spec's +// per-invocation cap with the user's --timeout / review.timeout value. +// Without it a host CLI chewing through a large diff would still be +// killed at the Spec default no matter how much time the user allowed, +// because a context deadline can only shorten the window. A +// non-positive d is ignored so callers can pass "unset" unconditionally. +func (b *Backend) SetTimeout(d time.Duration) { + if d > 0 { + b.spec.Timeout = d + } +} + func (b *Backend) Name() string { return b.spec.Name } // DefaultModel returns a stable identifier for the cache key. CLI diff --git a/internal/provider/clireview/clireview_test.go b/internal/provider/clireview/clireview_test.go index 93f3ddc..155feb3 100644 --- a/internal/provider/clireview/clireview_test.go +++ b/internal/provider/clireview/clireview_test.go @@ -390,3 +390,41 @@ func TestBackendDefaultModelMemoisesVersionCall(t *testing.T) { // guard: keep import for fmt usage in formatted assertions var _ = fmt.Sprint + +func TestBackendSetTimeoutRaisesTheSpecCap(t *testing.T) { + // --timeout must be able to LENGTHEN a CLI invocation, not just + // shorten it: a ctx deadline can only cut a run short, so a host CLI + // still chewing on a large diff would die at Spec.Timeout regardless. + scriptPath(t, "slow-cli", "sleep 0.4; echo done") + + b := New(Spec{ + Name: "slow-cli", + Binary: "slow-cli", + PromptArgs: func(p string, _ bool) []string { return []string{p} }, + Timeout: 50 * time.Millisecond, + }) + if _, err := b.Review(context.Background(), provider.Request{UserPrompt: "x"}); err == nil { + t.Fatal("expected the 50ms spec cap to fire before the script finished") + } + + b.SetTimeout(5 * time.Second) + resp, err := b.Review(context.Background(), provider.Request{UserPrompt: "x"}) + if err != nil { + t.Fatalf("after SetTimeout the same call should succeed; got: %v", err) + } + if resp.Content != "done" { + t.Errorf("Content = %q, want %q", resp.Content, "done") + } +} + +func TestBackendSetTimeoutIgnoresNonPositive(t *testing.T) { + b := New(Spec{Name: "x-cli", Binary: "x", Timeout: 2 * time.Minute}) + b.SetTimeout(0) + b.SetTimeout(-time.Second) + if b.spec.Timeout != 2*time.Minute { + t.Errorf("spec.Timeout = %v, want the original 2m left untouched", b.spec.Timeout) + } +} + +// Compile-time proof that the CLI layer's type assertion will succeed. +var _ provider.TimeoutSetter = (*Backend)(nil) diff --git a/internal/provider/ollama/client.go b/internal/provider/ollama/client.go index 3870416..3b31cb8 100644 --- a/internal/provider/ollama/client.go +++ b/internal/provider/ollama/client.go @@ -43,6 +43,17 @@ func New(cfg config.ProviderConfig) (provider.Provider, error) { }, nil } +// SetTimeout implements provider.TimeoutSetter. http.Client.Timeout is a +// hard whole-request ceiling that fires independently of the caller's +// context, so a local model that needs more than [requestTimeout] to +// think would be cut off even under a generous --timeout. A non-positive +// d is ignored so callers can pass "unset" unconditionally. +func (c *Client) SetTimeout(d time.Duration) { + if d > 0 { + c.http.Timeout = d + } +} + func (c *Client) Name() string { return Name } func (c *Client) DefaultModel() string { diff --git a/internal/provider/ollama/ollama_test.go b/internal/provider/ollama/ollama_test.go index a6a3051..36aaeeb 100644 --- a/internal/provider/ollama/ollama_test.go +++ b/internal/provider/ollama/ollama_test.go @@ -11,6 +11,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/CommitBrief/commitbrief/internal/config" "github.com/CommitBrief/commitbrief/internal/provider" @@ -218,3 +219,28 @@ func TestTestConnectionUnreachable(t *testing.T) { t.Error("expected error against closed server") } } + +func TestSetTimeoutOverridesTheHTTPCap(t *testing.T) { + // http.Client.Timeout is a hard whole-request ceiling that fires + // regardless of the caller's context, so a local model that needs more + // than requestTimeout to answer would be cut off under any --timeout. + p, err := New(config.ProviderConfig{}) + if err != nil { + t.Fatal(err) + } + c := p.(*Client) + if c.http.Timeout != requestTimeout { + t.Fatalf("baseline http timeout = %v, want %v", c.http.Timeout, requestTimeout) + } + c.SetTimeout(20 * time.Minute) + if c.http.Timeout != 20*time.Minute { + t.Errorf("http timeout = %v, want 20m", c.http.Timeout) + } + // Non-positive is "unset" — callers pass it unconditionally. + c.SetTimeout(0) + if c.http.Timeout != 20*time.Minute { + t.Errorf("http timeout = %v, want the previous 20m left untouched", c.http.Timeout) + } +} + +var _ provider.TimeoutSetter = (*Client)(nil) diff --git a/internal/provider/provider.go b/internal/provider/provider.go index fe58cd9..357dd51 100644 --- a/internal/provider/provider.go +++ b/internal/provider/provider.go @@ -2,7 +2,10 @@ package provider -import "context" +import ( + "context" + "time" +) type Provider interface { Name() string @@ -36,3 +39,23 @@ type PlainTextEmitter interface { Provider EmitsPlainText() } + +// TimeoutSetter is the optional interface for providers that enforce a +// hard timeout of their OWN in addition to honouring the caller's +// context — clireview's per-invocation cap, ollama's http.Client +// timeout, the Anthropic SDK's 10-minute non-streaming ceiling. +// +// It exists because a context deadline can only SHORTEN a run. A user +// who passes `--timeout 20m` on a slow review would still be cut off at +// the provider's built-in five or ten minutes, so the value has to reach +// the provider itself. Implement it only when there is such a cap to +// raise; providers that purely follow ctx (openai and its +// OpenAI-compatible siblings, gemini, mock) deliberately do not. +// +// SetTimeout is called at most once, right after construction and before +// any Review/TestConnection call, so implementations may simply assign +// to a field without synchronization. +type TimeoutSetter interface { + Provider + SetTimeout(d time.Duration) +} diff --git a/internal/setup/test.go b/internal/setup/test.go index 901bb38..b7b6879 100644 --- a/internal/setup/test.go +++ b/internal/setup/test.go @@ -4,6 +4,7 @@ package setup import ( "context" + "time" "github.com/CommitBrief/commitbrief/internal/config" "github.com/CommitBrief/commitbrief/internal/provider" @@ -14,9 +15,24 @@ import ( // preserved); a nil error means the credentials reached the backend and // got a non-error response. func TestConnection(ctx context.Context, name string, cfg config.ProviderConfig) error { + return TestConnectionTimeout(ctx, name, cfg, 0) +} + +// TestConnectionTimeout is TestConnection with the caller's resolved +// --timeout / review.timeout value handed to the provider. It matters for +// the same reason it does on the review path: a probe against a slow +// ollama host or a CLI binary would otherwise die at the provider's own +// built-in cap no matter how much time the user allowed. A zero d keeps +// every built-in exactly as it is. +func TestConnectionTimeout(ctx context.Context, name string, cfg config.ProviderConfig, d time.Duration) error { p, err := provider.New(name, cfg) if err != nil { return err } + if d > 0 { + if ts, ok := p.(provider.TimeoutSetter); ok { + ts.SetTimeout(d) + } + } return p.TestConnection(ctx) }