From 05753324e922df3d27e7deeb4450e7970757c1c8 Mon Sep 17 00:00:00 2001 From: prode Date: Sun, 4 Oct 2026 02:47:36 -0300 Subject: [PATCH 1/2] feat(gate): stop a gate after 30 minutes by default, and take --timeout in minutes The bound on one gate run moves from 10 to 30 minutes, so a suite that legitimately takes longer than ten is no longer killed as check.timed-out. scc check and scc validate accept --timeout to move it for one run; on validate it implies --checks. The value is never written to the manifest, and a timed-out finding now names the limit it was held to. Plan: plans/gate-timeout-30m.md --- internal/cli/check.go | 33 ++++++++++++++++++++++-- internal/cli/check_test.go | 33 ++++++++++++++++++++++++ internal/cli/validate_all.go | 14 +++++++++- internal/gate/gate.go | 28 +++++++++++++++----- internal/gate/gate_test.go | 35 +++++++++++++++++++++++++ internal/validate/all.go | 11 +++++++- internal/validate/checks.go | 9 +++++-- internal/validate/checks_test.go | 6 +++-- plans/gate-timeout-30m.md | 44 ++++++++++++++++++++++++++++++++ 9 files changed, 198 insertions(+), 15 deletions(-) create mode 100644 plans/gate-timeout-30m.md diff --git a/internal/cli/check.go b/internal/cli/check.go index dc50f8b..3fb7f64 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "strings" + "time" "github.com/protonspy/spec-claude-code/internal/finding" "github.com/protonspy/spec-claude-code/internal/gate" @@ -66,7 +67,8 @@ func checkUsage() { fmt.Fprintf(os.Stderr, `%s check — the project's own commands, as a gate Usage: - %s check [] [--json] Run every recorded gate, or one; exit 2 on findings + %s check [] [--json] [--timeout ] + Run every recorded gate, or one; exit 2 on findings %s check set "" Record a gate's command (--min for test) %s check skip This project has no such step; the gate goes quiet %s check show Print what this workspace records @@ -91,6 +93,9 @@ total is how many tests ran and coverage is the percentage — a string like coverage says, since a suite that does not exist covers nothing. The floor is %s%% unless this workspace records another. +Each gate is stopped after --timeout minutes, %d unless given. A gate that hangs +would otherwise hang the push that ran it. + A gate nobody has recorded is a finding; one you have skipped is silence. That is the difference between a project that has no formatter and a project that forgot to say what its formatter is. @@ -104,7 +109,25 @@ shell. They are committed, so treat them the way you treat a Makefile target: something a fresh clone of this repository is expected to run. `, prog(), prog(), prog(), prog(), prog(), prog(), gate.Build.What(), gate.Format.What(), gate.Lint.What(), gate.Test.What(), - gate.Percent(gate.DefaultMinCoverage), prog(), prog()) + gate.Percent(gate.DefaultMinCoverage), defaultMinutes(), prog(), prog()) +} + +// defaultMinutes is gate.DefaultTimeout in the unit --timeout takes. +func defaultMinutes() int { return int(gate.DefaultTimeout / time.Minute) } + +// addTimeout binds --timeout, in minutes, for every command that runs the gates. +func addTimeout(fs *flag.FlagSet) *int { + return fs.Int("timeout", defaultMinutes(), "minutes each gate may run before it is stopped") +} + +// gateLimit turns --timeout into a duration. Zero or less is refused rather than +// read as the default: somebody typed a bound, and it was not one. +func gateLimit(minutes int) (time.Duration, bool) { + if minutes <= 0 { + render.Err(fmt.Sprintf("--timeout must be a whole number of minutes above 0, got %d", minutes)) + return 0, false + } + return time.Duration(minutes) * time.Minute, true } // checkReport is the frozen JSON shape of a run, and the same values the human @@ -128,10 +151,15 @@ func runCheckRun(args []string) int { fs.SetOutput(os.Stderr) root := addRoot(fs) jsonOut := addJSON(fs) + timeout := addTimeout(fs) rest, err := parseFlags(fs, args) if err != nil { return exitFor(err) } + limit, ok := gateLimit(*timeout) + if !ok { + return ExitError + } target, ok := resolveRoot(*root) if !ok || !requireWorkspace(target) { return ExitError @@ -141,6 +169,7 @@ func runCheckRun(args []string) int { render.Err(fmt.Sprintf("check: %v", err)) return ExitError } + cfg.Timeout = limit // A named gate runs alone; no name runs the pipeline. want := gate.Kinds() diff --git a/internal/cli/check_test.go b/internal/cli/check_test.go index 6146752..090b8b8 100644 --- a/internal/cli/check_test.go +++ b/internal/cli/check_test.go @@ -200,6 +200,39 @@ func TestTestRunReportsAndPasses(t *testing.T) { } } +// --timeout takes minutes above zero; anything else is a usage error rather than +// a gate with no bound. +func TestCheckTimeoutTakesPositiveMinutes(t *testing.T) { + root := initWorkspace(t) + skipRest(t, root, "test") + setTest(t, root, echoJSON(412, "88.4")) + + if _, stderr, code := run(t, "check", "--root", root, "--timeout", "300"); code != ExitOK { + t.Fatalf("--timeout 300: exit = %d, want %d (stderr: %s)", code, ExitOK, stderr) + } + for _, bad := range []string{"0", "-5", "ten"} { + if _, _, code := run(t, "check", "--root", root, "--timeout", bad); code != ExitError { + t.Errorf("--timeout %s: exit = %d, want %d", bad, code, ExitError) + } + } +} + +// --timeout on validate bounds the gates, so it asks for them: a flag accepted and +// then ignored is how somebody believes they set it. +func TestValidateTimeoutRunsTheGates(t *testing.T) { + root := initWorkspace(t) + skipRest(t, root, "test") + marker := filepath.Join(root, "ran.txt") + setTest(t, root, "echo ran > "+filepath.ToSlash(marker)) + + if _, stderr, code := run(t, "validate", "--root", root, "--timeout", "45"); code != ExitFindings { + t.Fatalf("validate --timeout: exit = %d, want %d (stderr: %s)", code, ExitFindings, stderr) + } + if _, err := os.Stat(marker); err != nil { + t.Errorf("--timeout did not run the gates: %v", err) + } +} + // Under the floor is exit 2 — a finding, not a crash. CI and an agent both branch // on that code, and collapsing it into 1 would make "the coverage is low" // indistinguishable from "scc could not run". diff --git a/internal/cli/validate_all.go b/internal/cli/validate_all.go index 5b85dd2..1b0eb20 100644 --- a/internal/cli/validate_all.go +++ b/internal/cli/validate_all.go @@ -31,11 +31,23 @@ func runValidateAll(args []string) int { // delivery gate, so the pre-push hook passes it — the moment a branch becomes a // pull request — and a bare pre-commit run never waits on a build. withChecks := fs.Bool("checks", false, "also run this workspace's build, format, lint and test commands") + timeout := addTimeout(fs) jsonOut := addJSON(fs) rest, err := parseFlags(fs, helpWord(args)) if err != nil { return exitFor(err) } + // --timeout bounds the gates, so it is itself a request for them: accepting it + // and then running nothing it could bound is how somebody believes they set it. + fs.Visit(func(f *flag.Flag) { + if f.Name == "timeout" { + *withChecks = true + } + }) + limit, ok := gateLimit(*timeout) + if !ok { + return ExitError + } if !noPositionals(rest, "validate") { return ExitError } @@ -49,7 +61,7 @@ func runValidateAll(args []string) int { opts = append(opts, validate.WithPR()) } if *withChecks { - opts = append(opts, validate.WithChecks()) + opts = append(opts, validate.WithChecks(), validate.WithTimeout(limit)) } set, results, err := validate.Everything(target, opts...) if err != nil { diff --git a/internal/gate/gate.go b/internal/gate/gate.go index 625183b..67f0e04 100644 --- a/internal/gate/gate.go +++ b/internal/gate/gate.go @@ -123,10 +123,10 @@ const Skipped = "skipped" // where a weakened gate should have to be argued for. const DefaultMinCoverage = 86.0 -// Timeout bounds one run. A suite or a linter that hangs would otherwise hang the -// push that called it, and a gate that hangs is a gate somebody removes rather -// than debugs. -const Timeout = 10 * time.Minute +// DefaultTimeout bounds one run unless the caller asks for another. A suite or a +// linter that hangs would otherwise hang the push that called it, and a gate that +// hangs is a gate somebody removes rather than debugs. +const DefaultTimeout = 30 * time.Minute // Config is what a workspace recorded about its own commands. type Config struct { @@ -137,6 +137,10 @@ type Config struct { // DefaultMinCoverage — the manifest key is absent in every workspace written // before this existed, and an absent floor has to mean the default. MinCoverage float64 `json:"min_coverage,omitempty"` + // Timeout bounds each gate of this run; zero means DefaultTimeout. Never + // written to the manifest: a suite that needs longer today is a flag on the + // command, not a change to a committed file. + Timeout time.Duration `json:"-"` } // Command is what this gate runs, or "" when nothing is recorded. @@ -174,6 +178,14 @@ func (c Config) Floor() float64 { return c.MinCoverage } +// Limit is how long one gate may run before it is stopped. +func (c Config) Limit() time.Duration { + if c.Timeout <= 0 { + return DefaultTimeout + } + return c.Timeout +} + // Load reads the recorded commands out of this workspace's manifests. // // A repo worked on from two harnesses has two manifests and one set of commands, @@ -293,8 +305,10 @@ type Result struct { // fails. The whole output would put a build log into a JSON document and into // a session's context, which is what the report exists to avoid. Tail string `json:"tail,omitempty"` - // TimedOut says the command was killed at Timeout rather than finishing. + // TimedOut says the command was killed at Limit rather than finishing. TimedOut bool `json:"timed_out,omitempty"` + // Limit is how long the command was allowed, so a timeout can say after what. + Limit time.Duration `json:"-"` } // Passed reports whether this run clears every bar that applies to its gate: the @@ -321,7 +335,7 @@ func (r Result) Passed() bool { // that ran and failed is a Result, not an error: it is an answer to the question, // and the caller turns answers into findings. func Run(root string, k Kind, cfg Config, out io.Writer) (Result, error) { - res := Result{Kind: k, Command: cfg.Command(k)} + res := Result{Kind: k, Command: cfg.Command(k), Limit: cfg.Limit()} if k == Test { res.Floor = cfg.Floor() } @@ -329,7 +343,7 @@ func Run(root string, k Kind, cfg Config, out io.Writer) (Result, error) { return res, fmt.Errorf("no %s command is recorded for this workspace", k) } - ctx, cancel := context.WithTimeout(context.Background(), Timeout) + ctx, cancel := context.WithTimeout(context.Background(), res.Limit) defer cancel() cmd := command(ctx, res.Command) diff --git a/internal/gate/gate_test.go b/internal/gate/gate_test.go index 86af335..8eebbf5 100644 --- a/internal/gate/gate_test.go +++ b/internal/gate/gate_test.go @@ -10,6 +10,7 @@ import ( "runtime" "strings" "testing" + "time" ) // The report is the whole interface between scc and a project's tests, so what @@ -84,6 +85,40 @@ func TestFloorDefaults(t *testing.T) { } } +// The bound is the default until a run asks for another, and zero is "unset" for +// the same reason the floor's is: a run cannot turn the bound off by passing zero. +func TestLimitDefaults(t *testing.T) { + if got := (Config{}).Limit(); got != DefaultTimeout { + t.Errorf("limit = %v, want the default %v", got, DefaultTimeout) + } + if DefaultTimeout != 30*time.Minute { + t.Errorf("DefaultTimeout = %v, want 30m", DefaultTimeout) + } + if got := (Config{Timeout: 5 * time.Minute}).Limit(); got != 5*time.Minute { + t.Errorf("limit = %v, want the requested 5m", got) + } +} + +// A command still running at the limit is stopped and reported as such, with the +// limit it was held to, rather than hanging the push that ran it. +func TestRunStopsAtTheLimit(t *testing.T) { + sleep := "sleep 3" + if runtime.GOOS == "windows" { + sleep = "ping -n 4 127.0.0.1 >nul" + } + cfg := Config{Commands: map[Kind]string{Build: sleep}, Timeout: 200 * time.Millisecond} + res, err := Run(t.TempDir(), Build, cfg, io.Discard) + if err != nil { + t.Fatalf("Run: %v", err) + } + if !res.TimedOut || res.Passed() { + t.Errorf("result = %+v, want a timed-out failure", res) + } + if res.Limit != 200*time.Millisecond { + t.Errorf("limit = %v, want the configured 200ms", res.Limit) + } +} + // A gate nobody recorded and a gate somebody declined are different states, and // keeping them apart is the whole reason Skipped is a word rather than an absent // key: one is "decide this", the other is "already decided, there is none". diff --git a/internal/validate/all.go b/internal/validate/all.go index b13aa74..52d558e 100644 --- a/internal/validate/all.go +++ b/internal/validate/all.go @@ -2,6 +2,7 @@ package validate import ( "sort" + "time" "github.com/protonspy/spec-claude-code/internal/finding" ) @@ -55,6 +56,7 @@ type Option func(*options) type options struct { pr bool checks bool + timeout time.Duration noAttribution bool } @@ -82,6 +84,10 @@ func WithPR() Option { return func(o *options) { o.pr = true } } // request. func WithChecks() Option { return func(o *options) { o.checks = true } } +// WithTimeout bounds each gate WithChecks runs by d instead of gate.DefaultTimeout. +// It adds no check of its own, so it is not a third reason to pay a cost. +func WithTimeout(d time.Duration) Option { return func(o *options) { o.timeout = d } } + func extra(opts []Option) []Validator { var o options for _, fn := range opts { @@ -91,7 +97,10 @@ func extra(opts []Option) []Validator { // Checks before pr: they are the slow ones, and a run that is going to fail on // a broken build should say so before it spends a network round trip on the forge. if o.checks { - out = append(out, Validator{Name: "checks", Run: Checks}) + limit := o.timeout + out = append(out, Validator{Name: "checks", Run: func(root string) (*finding.Set, error) { + return checksWithin(root, limit) + }}) } if o.pr { out = append(out, Validator{Name: "pr", Run: AttributionPR}) diff --git a/internal/validate/checks.go b/internal/validate/checks.go index 25c5971..2de052f 100644 --- a/internal/validate/checks.go +++ b/internal/validate/checks.go @@ -3,6 +3,7 @@ package validate import ( "fmt" "os" + "time" "github.com/protonspy/spec-claude-code/internal/finding" "github.com/protonspy/spec-claude-code/internal/gate" @@ -24,11 +25,15 @@ import ( // --checks` and the pre-push hook run them, and those are the two moments they // are worth their cost: the second is where a branch becomes a pull request, // which is where this methodology says the claim is actually made. -func Checks(root string) (*finding.Set, error) { +func Checks(root string) (*finding.Set, error) { return checksWithin(root, 0) } + +// checksWithin is Checks with each gate bounded by limit; zero is the default. +func checksWithin(root string, limit time.Duration) (*finding.Set, error) { cfg, err := gate.Load(root) if err != nil { return nil, err } + cfg.Timeout = limit set := &finding.Set{} at := rel(root, gate.ManifestPath(root)) @@ -91,7 +96,7 @@ func CheckFindings(root string, res gate.Result) *finding.Set { if res.TimedOut { set.Addf(at, 0, "check.timed-out", "the %s gate — `%s` — was still running after %s and was stopped", - res.Kind, res.Command, gate.Timeout) + res.Kind, res.Command, res.Limit) return set } if res.ExitCode != 0 { diff --git a/internal/validate/checks_test.go b/internal/validate/checks_test.go index 1fff283..5e2bdc0 100644 --- a/internal/validate/checks_test.go +++ b/internal/validate/checks_test.go @@ -4,6 +4,7 @@ import ( "os" "strings" "testing" + "time" "github.com/protonspy/spec-claude-code/internal/assets" "github.com/protonspy/spec-claude-code/internal/gate" @@ -39,9 +40,10 @@ func TestCheckFindingsTellsTheFailuresApart(t *testing.T) { }{ { name: "timed out", - res: gate.Result{Kind: gate.Test, Command: "go test ./...", TimedOut: true}, + res: gate.Result{Kind: gate.Test, Command: "go test ./...", TimedOut: true, Limit: 45 * time.Minute}, rule: "check.timed-out", - says: []string{"go test ./...", "still running"}, + // The limit it was held to, since --timeout can move it. + says: []string{"go test ./...", "still running", "45m0s"}, }, { name: "failed", diff --git a/plans/gate-timeout-30m.md b/plans/gate-timeout-30m.md new file mode 100644 index 0000000..2a09901 --- /dev/null +++ b/plans/gate-timeout-30m.md @@ -0,0 +1,44 @@ +--- +autonomy: auto +ci: no-wait +--- + +# Gate timeout 30m + +Raise the timeout that bounds one gate run from 10 to 30 minutes, and let a run move +it with `--timeout `. + +## Why + +A test suite that legitimately takes longer than ten minutes is killed and reported as +`check.timed-out`, failing the push for a reason that is not a defect. The bound still +exists so a hanging command cannot hang the push; the default moves to 30 minutes, and +a run that needs another bound asks for it on the command line. Done when +`gate.DefaultTimeout` is 30 minutes, `scc check` and `scc validate` accept +`--timeout`, and the suite and lint pass. + +## Paths + +- `internal/gate/gate.go` +- `internal/validate/checks.go` +- `internal/validate/all.go` +- `internal/cli/check.go` +- `internal/cli/validate_all.go` + +## Out of scope + +- A per-gate timeout: one bound applies to build, format, lint and test alike. +- Recording the timeout in the manifest: it is a flag on the run, never a committed value. + +## Tasks + +- [x] 1.1 (Unit) Set the default gate timeout to 30 minutes +- [x] 1.2 (Unit) Carry a per-run timeout on `gate.Config` and report it on a timed-out `gate.Result` + _Depends 1.1_ +- [x] 1.3 (Unit) Add `--timeout ` to `scc check` and `scc validate`, where it implies `--checks` + _Depends 1.2_ + +## Done when + +- `go test ./...` and `golangci-lint run` exit 0. +- `scc validate --checks --pr` exits 0. From 8a36c63d994656964fe8e9c76d9becdb13a6a6a9 Mon Sep 17 00:00:00 2001 From: prode Date: Sun, 4 Oct 2026 02:49:24 -0300 Subject: [PATCH 2/2] fix(cli): refuse a --timeout too large to fit a time.Duration A minute count above MaxInt64/time.Minute wrapped when multiplied, giving a bound nobody asked for. It is now a usage error, like zero or a negative. Plan: plans/gate-timeout-30m.md --- internal/cli/check.go | 7 +++++-- internal/cli/check_test.go | 3 ++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/cli/check.go b/internal/cli/check.go index 3fb7f64..1a7e092 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -3,6 +3,7 @@ package cli import ( "flag" "fmt" + "math" "os" "strings" "time" @@ -121,9 +122,11 @@ func addTimeout(fs *flag.FlagSet) *int { } // gateLimit turns --timeout into a duration. Zero or less is refused rather than -// read as the default: somebody typed a bound, and it was not one. +// read as the default: somebody typed a bound, and it was not one. So is a number +// too large to multiply into a time.Duration, which would wrap to a bound nobody +// asked for. func gateLimit(minutes int) (time.Duration, bool) { - if minutes <= 0 { + if minutes <= 0 || int64(minutes) > math.MaxInt64/int64(time.Minute) { render.Err(fmt.Sprintf("--timeout must be a whole number of minutes above 0, got %d", minutes)) return 0, false } diff --git a/internal/cli/check_test.go b/internal/cli/check_test.go index 090b8b8..c20e5ff 100644 --- a/internal/cli/check_test.go +++ b/internal/cli/check_test.go @@ -210,7 +210,8 @@ func TestCheckTimeoutTakesPositiveMinutes(t *testing.T) { if _, stderr, code := run(t, "check", "--root", root, "--timeout", "300"); code != ExitOK { t.Fatalf("--timeout 300: exit = %d, want %d (stderr: %s)", code, ExitOK, stderr) } - for _, bad := range []string{"0", "-5", "ten"} { + // The last wraps when multiplied into a time.Duration. + for _, bad := range []string{"0", "-5", "ten", "9223372036854775807"} { if _, _, code := run(t, "check", "--root", root, "--timeout", bad); code != ExitError { t.Errorf("--timeout %s: exit = %d, want %d", bad, code, ExitError) }