Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions internal/cli/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package cli
import (
"flag"
"fmt"
"math"
"os"
"strings"
"time"

"github.com/protonspy/spec-claude-code/internal/finding"
"github.com/protonspy/spec-claude-code/internal/gate"
Expand Down Expand Up @@ -66,7 +68,8 @@ func checkUsage() {
fmt.Fprintf(os.Stderr, `%s check — the project's own commands, as a gate

Usage:
%s check [<gate>] [--json] Run every recorded gate, or one; exit 2 on findings
%s check [<gate>] [--json] [--timeout <minutes>]
Run every recorded gate, or one; exit 2 on findings
%s check set <gate> "<command>" Record a gate's command (--min <percent> for test)
%s check skip <gate> This project has no such step; the gate goes quiet
%s check show Print what this workspace records
Expand All @@ -91,6 +94,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.
Expand All @@ -104,7 +110,27 @@ 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. 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 || 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
}
return time.Duration(minutes) * time.Minute, true
}

// checkReport is the frozen JSON shape of a run, and the same values the human
Expand All @@ -128,10 +154,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
Expand All @@ -141,6 +172,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()
Expand Down
34 changes: 34 additions & 0 deletions internal/cli/check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,40 @@ 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)
}
// 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)
}
}
}

// --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".
Expand Down
14 changes: 13 additions & 1 deletion internal/cli/validate_all.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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 {
Expand Down
28 changes: 21 additions & 7 deletions internal/gate/gate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -321,15 +335,15 @@ 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()
}
if !cfg.Recorded(k) {
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)
Expand Down
35 changes: 35 additions & 0 deletions internal/gate/gate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"runtime"
"strings"
"testing"
"time"
)

// The report is the whole interface between scc and a project's tests, so what
Expand Down Expand Up @@ -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".
Expand Down
11 changes: 10 additions & 1 deletion internal/validate/all.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package validate

import (
"sort"
"time"

"github.com/protonspy/spec-claude-code/internal/finding"
)
Expand Down Expand Up @@ -55,6 +56,7 @@ type Option func(*options)
type options struct {
pr bool
checks bool
timeout time.Duration
noAttribution bool
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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})
Expand Down
9 changes: 7 additions & 2 deletions internal/validate/checks.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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))

Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 4 additions & 2 deletions internal/validate/checks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand Down
Loading
Loading