diff --git a/Makefile b/Makefile index 5c82b71..fd36aec 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build test lint skill +.PHONY: build test lint skill release-validation build: go build ./... @@ -11,3 +11,11 @@ lint: skill: go run ./cmd/genskill + +release-validation: + GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=commit.gpgsign GIT_CONFIG_VALUE_0=false go test -race -shuffle=on -count=1 ./... + @if test -n "$$MADE_GITHUB_SMOKE_REPO" && test -n "$$MADE_GITHUB_SMOKE_PR_URL"; then \ + GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=commit.gpgsign GIT_CONFIG_VALUE_0=false go test -race -shuffle=on -count=1 ./internal/github -run '^TestLive_CIWorkflowOwnershipSmoke$$' -v; \ + else \ + echo 'release-validation: GitHub smoke skipped; set MADE_GITHUB_SMOKE_REPO and MADE_GITHUB_SMOKE_PR_URL'; \ + fi diff --git a/README.md b/README.md index dfb2f0f..e8e885c 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,14 @@ made is an independent synthesis, not a dependency bundle or a one-to-one copy o See `plans/made-rewrite.md` for the full design and build plan. +CI check policy + +The trusted `.made.yml` copy may set `ci.check_scope` to `required` or `all`; the default is `required`. +`ci.rerun_budget` counts rerun rounds, not individual checks. +Made polls pending checks without spending a round, reruns each unique failed GitHub Actions workflow run only, and reports bounded evidence by failed check and run. +External checks are reported by name and link and are never rerun. +The opt-in disposable-repository smoke contract is `MADE_GITHUB_SMOKE_REPO` plus `MADE_GITHUB_SMOKE_PR_URL`, and `make release-validation` runs it when both are set. + ## Versioned daemon contract `made capabilities --json` reports the public protocol and command schema. diff --git a/internal/config/config.go b/internal/config/config.go index 9bfe935..34e8ca6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,33 +1,18 @@ package config import ( - "bytes" - "errors" "fmt" - "io" - "os" - "path/filepath" - "strings" "time" "github.com/douglasjarquin/made/internal/agent" - "gopkg.in/yaml.v3" + "github.com/douglasjarquin/made/internal/github" ) const ( - defaultCIRerunBudget = 2 defaultStageTimeout = 30 * time.Minute - maxStageTimeoutSeconds = 2 * 60 * 60 defaultEvidenceRetention = 4 << 20 - maxEvidenceRetention = 64 << 20 - maxConfigBytes = 1 << 20 ) -var validStageNames = map[string]struct{}{ - "intent": {}, "rebase": {}, "review": {}, "test": {}, "document": {}, - "lint": {}, "push": {}, "pr": {}, "ci": {}, -} - type Config struct { Version int `yaml:"version"` Document Document `yaml:"document"` @@ -99,8 +84,9 @@ type Review struct { } type CI struct { - Required bool `yaml:"required"` - RerunBudget int `yaml:"rerun_budget"` + Required bool `yaml:"required"` + RerunBudget int `yaml:"rerun_budget"` + CheckScope github.CheckScope `yaml:"check_scope"` } type Test struct { @@ -119,55 +105,6 @@ type Commands struct { Lint string `yaml:"lint"` } -// LoadEffectiveConfig resolves a gate run's effective configuration from a -// trusted source (the default-branch copy) and a pushed source (the branch -// being validated). trustedPath or pushedPath may be "" to indicate that -// source has no config at all. -func LoadEffectiveConfig(trustedPath, pushedPath string) (Config, error) { - trusted, trustedExists, err := loadConfigFile(trustedPath) - if err != nil { - return Config{}, fmt.Errorf("config: trusted copy at %q could not be read: %w", trustedPath, err) - } - - pushed, _, err := loadConfigFile(pushedPath) - if err != nil { - return Config{}, fmt.Errorf("config: pushed copy at %q could not be read: %w", pushedPath, err) - } - - effective := Config{ - Version: trusted.Version, - Document: trusted.Document, - Review: trusted.Review, - DisableProjectSettings: trusted.DisableProjectSettings, - NoCI: trusted.NoCI, - CI: trusted.CI, - AllowRepoCommands: trusted.AllowRepoCommands, - Stages: trusted.Stages, - } - effective.Test.Evidence = trusted.Test.Evidence - - if effective.CI.RerunBudget == 0 { - effective.CI.RerunBudget = defaultCIRerunBudget - } - - // Trust boundary: Commands/Agent/Agents execute inside the gate worktree, - // so a pushed branch must never control them unless the trusted copy - // itself opted in via allow_repo_commands. Absence of a trusted copy - // (trustedExists == false) always resolves these to zero-value; it must - // never fall through to the pushed copy. - if trustedExists && trusted.AllowRepoCommands { - effective.Commands = pushed.Commands - effective.Agent = pushed.Agent - effective.Agents = pushed.Agents - } else { - effective.Commands = trusted.Commands - effective.Agent = trusted.Agent - effective.Agents = trusted.Agents - } - - return effective, nil -} - func (c Config) TestCommand() []string { return shellCommand(c.Commands.Test) } @@ -193,93 +130,3 @@ func (c Config) AgentKind() (agent.Kind, error) { return "", fmt.Errorf("config: invalid agent %q: must be %q or %q", c.Agent, agent.KindClaude, agent.KindCodex) } } - -func loadConfigFile(path string) (cfg Config, exists bool, err error) { - if path == "" { - return Config{}, false, nil - } - - data, exists, err := readConfigBytes(path, nil) - if err != nil { - return Config{}, exists, err - } - if !exists { - return Config{}, false, nil - } - - decoder := yaml.NewDecoder(bytes.NewReader(data)) - decoder.KnownFields(true) - if err := decoder.Decode(&cfg); err != nil { - return Config{}, true, err - } - var extra any - if err := decoder.Decode(&extra); err != io.EOF { - if err == nil { - return Config{}, true, fmt.Errorf("configuration must contain one YAML document") - } - return Config{}, true, err - } - if filepath.Base(path) == ".made.yml" || strings.HasSuffix(filepath.Base(path), ".made.yml") { - if cfg.Version != 1 { - return Config{}, true, fmt.Errorf("versioned .made.yml requires version: 1, got %d", cfg.Version) - } - for name := range cfg.Stages { - if _, ok := validStageNames[name]; !ok { - return Config{}, true, fmt.Errorf("versioned .made.yml has unknown stage %q", name) - } - stage := cfg.Stages[name] - if stage.TimeoutSeconds != nil && (*stage.TimeoutSeconds <= 0 || *stage.TimeoutSeconds > maxStageTimeoutSeconds) { - return Config{}, true, fmt.Errorf("versioned .made.yml stage %q timeout_seconds must be between 1 and %d", name, maxStageTimeoutSeconds) - } - } - if retention := cfg.Test.Evidence.RetentionBytes; retention != nil && (*retention <= 0 || *retention > maxEvidenceRetention) { - return Config{}, true, fmt.Errorf("versioned .made.yml test.evidence.retention_bytes must be between 1 and %d", maxEvidenceRetention) - } - if !cfg.hasConfiguredValue() { - return Config{}, true, fmt.Errorf("versioned .made.yml must configure at least one non-version field") - } - return cfg, true, nil - } - return cfg, true, nil -} - -func readConfigBytes(path string, beforeRead func()) ([]byte, bool, error) { - file, err := os.Open(path) - if errors.Is(err, os.ErrNotExist) { - return nil, false, nil - } - if err != nil { - return nil, false, err - } - defer func() { _ = file.Close() }() - - info, err := file.Stat() - if err != nil { - return nil, true, err - } - if !info.Mode().IsRegular() { - return nil, true, fmt.Errorf("config: %s is not a regular file", path) - } - if info.Size() > maxConfigBytes { - return nil, true, fmt.Errorf("config: %s exceeds %d bytes", path, maxConfigBytes) - } - if beforeRead != nil { - beforeRead() - } - data, err := io.ReadAll(io.LimitReader(file, maxConfigBytes+1)) - if err != nil { - return nil, true, err - } - if len(data) > maxConfigBytes { - return nil, true, fmt.Errorf("config: %s exceeds %d bytes", path, maxConfigBytes) - } - return data, true, nil -} - -func (c Config) hasConfiguredValue() bool { - return len(c.Document.Rules) > 0 || c.Review.Required || c.DisableProjectSettings || c.NoCI || - c.CI.Required || c.CI.RerunBudget != 0 || len(c.Test.Evidence.Branch) > 0 || c.Test.Evidence.RetentionBytes != nil || - c.Test.Evidence.StoreInRepo || len(c.Test.Evidence.Dir) > 0 || len(c.Commands.Test) > 0 || - len(c.Commands.Lint) > 0 || len(c.Agent) > 0 || len(c.Agents) > 0 || c.AllowRepoCommands || - len(c.Stages) > 0 -} diff --git a/internal/config/config_extended_test.go b/internal/config/config_extended_test.go index 9b31264..486059a 100644 --- a/internal/config/config_extended_test.go +++ b/internal/config/config_extended_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/douglasjarquin/made/internal/agent" + "github.com/douglasjarquin/made/internal/github" ) const trustedFixtureWithEvidenceModeAndBudget = ` @@ -73,6 +74,36 @@ func TestCI_RerunBudgetHonorsExplicitValue(t *testing.T) { } } +func TestCI_CheckScopeCanBeConfiguredAsAll(t *testing.T) { + dir := t.TempDir() + trustedPath := writeConfigFile(t, dir, "trusted.yaml", `version: 1 +ci: + required: true + check_scope: all +`) + + cfg, err := LoadEffectiveConfig(trustedPath, "") + if err != nil { + t.Fatalf("LoadEffectiveConfig rejected configured CI check scope: %v", err) + } + if cfg.CI.CheckScope != github.CheckScopeAll { + t.Fatalf("CI.CheckScope = %q, want %q", cfg.CI.CheckScope, github.CheckScopeAll) + } +} + +func TestCI_CheckScopeDefaultsToRequired(t *testing.T) { + dir := t.TempDir() + trustedPath := writeConfigFile(t, dir, "trusted.yaml", trustedFixture) + + cfg, err := LoadEffectiveConfig(trustedPath, "") + if err != nil { + t.Fatalf("LoadEffectiveConfig: %v", err) + } + if cfg.CI.CheckScope != github.CheckScopeRequired { + t.Fatalf("CI.CheckScope = %q, want %q", cfg.CI.CheckScope, github.CheckScopeRequired) + } +} + func TestConfig_TestCommandTokenizesNonEmptyString(t *testing.T) { cfg := Config{Commands: Commands{Test: "go test ./..."}} diff --git a/internal/config/file.go b/internal/config/file.go new file mode 100644 index 0000000..09629f5 --- /dev/null +++ b/internal/config/file.go @@ -0,0 +1,162 @@ +package config + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" + + "github.com/douglasjarquin/made/internal/github" + "gopkg.in/yaml.v3" +) + +const ( + defaultCIRerunBudget = 2 + maxStageTimeoutSeconds = 2 * 60 * 60 + maxEvidenceRetention = 64 << 20 + maxConfigBytes = 1 << 20 +) + +var validStageNames = map[string]struct{}{ + "intent": {}, "rebase": {}, "review": {}, "test": {}, "document": {}, + "lint": {}, "push": {}, "pr": {}, "ci": {}, +} + +func LoadEffectiveConfig(trustedPath, pushedPath string) (Config, error) { + trusted, trustedExists, err := loadConfigFile(trustedPath) + if err != nil { + return Config{}, fmt.Errorf("config: trusted copy at %q could not be read: %w", trustedPath, err) + } + + pushed, _, err := loadConfigFile(pushedPath) + if err != nil { + return Config{}, fmt.Errorf("config: pushed copy at %q could not be read: %w", pushedPath, err) + } + + effective := Config{ + Version: trusted.Version, + Document: trusted.Document, + Review: trusted.Review, + DisableProjectSettings: trusted.DisableProjectSettings, + NoCI: trusted.NoCI, + CI: trusted.CI, + AllowRepoCommands: trusted.AllowRepoCommands, + Stages: trusted.Stages, + } + effective.Test.Evidence = trusted.Test.Evidence + + if effective.CI.RerunBudget == 0 { + effective.CI.RerunBudget = defaultCIRerunBudget + } + if effective.CI.CheckScope == "" { + effective.CI.CheckScope = github.CheckScopeRequired + } + + if trustedExists && trusted.AllowRepoCommands { + effective.Commands = pushed.Commands + effective.Agent = pushed.Agent + effective.Agents = pushed.Agents + } else { + effective.Commands = trusted.Commands + effective.Agent = trusted.Agent + effective.Agents = trusted.Agents + } + + return effective, nil +} + +func loadConfigFile(path string) (cfg Config, exists bool, err error) { + if path == "" { + return Config{}, false, nil + } + + data, exists, err := readConfigBytes(path, nil) + if err != nil { + return Config{}, exists, err + } + if !exists { + return Config{}, false, nil + } + + decoder := yaml.NewDecoder(bytes.NewReader(data)) + decoder.KnownFields(true) + if err := decoder.Decode(&cfg); err != nil { + return Config{}, true, err + } + var extra any + if err := decoder.Decode(&extra); err != io.EOF { + if err == nil { + return Config{}, true, fmt.Errorf("configuration must contain one YAML document") + } + return Config{}, true, err + } + if filepath.Base(path) == ".made.yml" || strings.HasSuffix(filepath.Base(path), ".made.yml") { + if cfg.Version != 1 { + return Config{}, true, fmt.Errorf("versioned .made.yml requires version: 1, got %d", cfg.Version) + } + for name := range cfg.Stages { + if _, ok := validStageNames[name]; !ok { + return Config{}, true, fmt.Errorf("versioned .made.yml has unknown stage %q", name) + } + stage := cfg.Stages[name] + if stage.TimeoutSeconds != nil && (*stage.TimeoutSeconds <= 0 || *stage.TimeoutSeconds > maxStageTimeoutSeconds) { + return Config{}, true, fmt.Errorf("versioned .made.yml stage %q timeout_seconds must be between 1 and %d", name, maxStageTimeoutSeconds) + } + } + if retention := cfg.Test.Evidence.RetentionBytes; retention != nil && (*retention <= 0 || *retention > maxEvidenceRetention) { + return Config{}, true, fmt.Errorf("versioned .made.yml test.evidence.retention_bytes must be between 1 and %d", maxEvidenceRetention) + } + if cfg.CI.CheckScope != "" && !cfg.CI.CheckScope.Valid() { + return Config{}, true, fmt.Errorf("versioned .made.yml ci.check_scope must be %q or %q, got %q", github.CheckScopeRequired, github.CheckScopeAll, cfg.CI.CheckScope) + } + if !cfg.hasConfiguredValue() { + return Config{}, true, fmt.Errorf("versioned .made.yml must configure at least one non-version field") + } + return cfg, true, nil + } + return cfg, true, nil +} + +func readConfigBytes(path string, beforeRead func()) ([]byte, bool, error) { + file, err := os.Open(path) + if errors.Is(err, os.ErrNotExist) { + return nil, false, nil + } + if err != nil { + return nil, false, err + } + defer func() { _ = file.Close() }() + + info, err := file.Stat() + if err != nil { + return nil, true, err + } + if !info.Mode().IsRegular() { + return nil, true, fmt.Errorf("config: %s is not a regular file", path) + } + if info.Size() > maxConfigBytes { + return nil, true, fmt.Errorf("config: %s exceeds %d bytes", path, maxConfigBytes) + } + if beforeRead != nil { + beforeRead() + } + data, err := io.ReadAll(io.LimitReader(file, maxConfigBytes+1)) + if err != nil { + return nil, true, err + } + if len(data) > maxConfigBytes { + return nil, true, fmt.Errorf("config: %s exceeds %d bytes", path, maxConfigBytes) + } + return data, true, nil +} + +func (c Config) hasConfiguredValue() bool { + return len(c.Document.Rules) > 0 || c.Review.Required || c.DisableProjectSettings || c.NoCI || + c.CI.Required || c.CI.RerunBudget != 0 || c.CI.CheckScope != "" || len(c.Test.Evidence.Branch) > 0 || c.Test.Evidence.RetentionBytes != nil || + c.Test.Evidence.StoreInRepo || len(c.Test.Evidence.Dir) > 0 || len(c.Commands.Test) > 0 || + len(c.Commands.Lint) > 0 || len(c.Agent) > 0 || len(c.Agents) > 0 || c.AllowRepoCommands || + len(c.Stages) > 0 +} diff --git a/internal/github/checks.go b/internal/github/checks.go new file mode 100644 index 0000000..b1dc839 --- /dev/null +++ b/internal/github/checks.go @@ -0,0 +1,105 @@ +package github + +import ( + "net/url" + "strconv" + "strings" +) + +type CheckScope string + +const ( + CheckScopeRequired CheckScope = "required" + CheckScopeAll CheckScope = "all" +) + +func (s CheckScope) Valid() bool { + return s == CheckScopeRequired || s == CheckScopeAll +} + +type CheckResult struct { + Name string `json:"name"` + State string `json:"state"` + Bucket string `json:"bucket"` + DetailsLink string `json:"link"` + WorkflowRunID string `json:"-"` + ActionsBacked bool `json:"-"` + Rerunnable bool `json:"-"` + Required bool `json:"-"` + InScope bool `json:"-"` +} + +type ChecksResult struct { + Checks []CheckResult + ExitCode int +} + +func enrichCheck(check *CheckResult, required, inScope bool, prURL string) { + check.Required = required + check.InScope = inScope + check.ActionsBacked, check.WorkflowRunID = workflowRunOwnership(check.DetailsLink, prURL) + check.Rerunnable = check.ActionsBacked && check.WorkflowRunID != "" +} + +func annotateRequired(checks, required []CheckResult) { + for i := range checks { + checks[i].InScope = true + for _, requiredCheck := range required { + if sameCheck(checks[i], requiredCheck) { + checks[i].Required = true + break + } + } + } +} + +func sameCheck(left, right CheckResult) bool { + if left.Name != right.Name { + return false + } + if left.DetailsLink == "" || right.DetailsLink == "" { + return true + } + return left.DetailsLink == right.DetailsLink +} + +func workflowRunOwnership(link, prURL string) (bool, string) { + prHost, prRepo, ok := repositoryPath(prURL) + if !ok || prHost != "github.com" { + return false, "" + } + linkHost, linkRepo, ok := repositoryPath(link) + if !ok || linkHost != prHost || linkRepo != prRepo { + return false, "" + } + + parsed, err := url.Parse(link) + if err != nil { + return false, "" + } + parts := strings.Split(strings.Trim(parsed.Path, "/"), "/") + if len(parts) < 5 || parts[2] != "actions" || parts[3] != "runs" { + return false, "" + } + runID := parts[4] + if _, err := strconv.ParseUint(runID, 10, 64); err != nil { + return true, "" + } + return true, runID +} + +func repositoryPath(raw string) (string, string, bool) { + parsed, err := url.Parse(raw) + if err != nil { + return "", "", false + } + parts := strings.Split(strings.Trim(parsed.Path, "/"), "/") + if parsed.Hostname() == "" || len(parts) < 2 || parts[0] == "" || parts[1] == "" { + return "", "", false + } + host := strings.ToLower(parsed.Hostname()) + if host == "www.github.com" { + host = "github.com" + } + return host, strings.ToLower(strings.Join(parts[:2], "/")), true +} diff --git a/internal/github/checks_client.go b/internal/github/checks_client.go new file mode 100644 index 0000000..99bdaec --- /dev/null +++ b/internal/github/checks_client.go @@ -0,0 +1,118 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "strings" +) + +const maxCheckLogBytes = 64 * 1024 + +func (c *Client) PRChecks(ctx context.Context, prURL string, scope CheckScope) (ChecksResult, error) { + if strings.TrimSpace(prURL) == "" { + return ChecksResult{}, fmt.Errorf("github: pull request URL is required for checks") + } + if !scope.Valid() { + return ChecksResult{}, fmt.Errorf("github: unsupported check scope %q", scope) + } + if err := c.AuthStatus(ctx); err != nil { + return ChecksResult{}, err + } + + checks, err := c.prChecks(ctx, prURL, scope == CheckScopeRequired) + if err != nil { + return ChecksResult{}, err + } + if scope == CheckScopeRequired { + for i := range checks.Checks { + enrichCheck(&checks.Checks[i], true, true, prURL) + } + return checks, nil + } + + required, err := c.prChecks(ctx, prURL, true) + if err != nil { + return ChecksResult{}, err + } + annotateRequired(checks.Checks, required.Checks) + for i := range checks.Checks { + enrichCheck(&checks.Checks[i], checks.Checks[i].Required, true, prURL) + } + return checks, nil +} + +func (c *Client) prChecks(ctx context.Context, prURL string, required bool) (ChecksResult, error) { + args := []string{"pr", "checks", prURL} + if required { + args = append(args, "--required") + } + args = append(args, "--json", "name,state,bucket,link") + res, err := c.run(ctx, args...) + if err != nil { + return ChecksResult{}, fmt.Errorf("github: run gh pr checks: %w", err) + } + if isRateLimitDetail(string(res.Stderr)) { + return ChecksResult{}, &RateLimitError{Operation: "gh pr checks", Detail: strings.TrimSpace(string(res.Stderr))} + } + if res.ExitCode != 0 && strings.TrimSpace(string(res.Stderr)) != "" { + return ChecksResult{}, commandFailure("gh pr checks", res) + } + if len(strings.TrimSpace(string(res.Stdout))) == 0 { + return ChecksResult{}, fmt.Errorf("github: gh pr checks returned no JSON (exit %d): %s", res.ExitCode, strings.TrimSpace(string(res.Stderr))) + } + + var checks []CheckResult + if err := json.Unmarshal(res.Stdout, &checks); err != nil { + return ChecksResult{}, fmt.Errorf("github: parse gh pr checks output: %w: stdout=%s", err, res.Stdout) + } + if res.ExitCode == 0 { + for _, check := range checks { + bucket := strings.ToLower(strings.TrimSpace(check.Bucket)) + if bucket != "pass" && bucket != "skipping" && bucket != "neutral" { + return ChecksResult{}, fmt.Errorf("github: gh pr checks exit 0 with non-success bucket %q for %q", check.Bucket, check.Name) + } + } + } + return ChecksResult{Checks: checks, ExitCode: res.ExitCode}, nil +} + +func (c *Client) CheckLogs(ctx context.Context, runID string) (string, error) { + if err := validateWorkflowRunID(runID); err != nil { + return "", err + } + if err := c.AuthStatus(ctx); err != nil { + return "", err + } + + res, err := c.run(ctx, "run", "view", runID, "--log") + if err != nil { + return "", fmt.Errorf("github: run gh run view: %w", err) + } + if res.ExitCode != 0 { + return "", commandFailure("gh run view", res) + } + output := res.Stdout + if len(output) > maxCheckLogBytes { + output = append(append([]byte(nil), output[:maxCheckLogBytes]...), []byte("\n[truncated]\n")...) + } + return string(output), nil +} + +func (c *Client) RerunCheck(ctx context.Context, runID string) error { + if err := validateWorkflowRunID(runID); err != nil { + return err + } + if err := c.AuthStatus(ctx); err != nil { + return err + } + + res, err := c.run(ctx, "run", "rerun", runID, "--failed") + if err != nil { + return fmt.Errorf("github: run gh run rerun: %w", err) + } + if res.ExitCode != 0 { + return commandFailure("gh run rerun", res) + } + return nil +} diff --git a/internal/github/checks_test.go b/internal/github/checks_test.go new file mode 100644 index 0000000..e8c6fb0 --- /dev/null +++ b/internal/github/checks_test.go @@ -0,0 +1,143 @@ +package github_test + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/douglasjarquin/made/internal/github" +) + +func TestPRChecks_ParsesJSON(t *testing.T) { + c := newClient(t, []string{`FAKE_GH_CHECKS_JSON=[{"name":"build","state":"COMPLETED","bucket":"pass","link":"https://github.com/example/repo/actions/runs/42"}]`}, "") + + checks, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/42", github.CheckScopeRequired) + if err != nil { + t.Fatalf("PRChecks: %v", err) + } + if checks.ExitCode != 0 || len(checks.Checks) != 1 { + t.Fatalf("unexpected checks result: %+v", checks) + } + if checks.Checks[0].Bucket != "pass" || checks.Checks[0].WorkflowRunID != "42" { + t.Fatalf("unexpected check fields: %+v", checks.Checks[0]) + } +} + +func TestPRChecks_UsesRequiredScopeInvocation(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "invocations.log") + c := newClient(t, nil, logPath) + + if _, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/43", github.CheckScopeRequired); err != nil { + t.Fatalf("PRChecks: %v", err) + } + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read invocation log: %v", err) + } + if !strings.Contains(string(data), "pr checks") || !strings.Contains(string(data), "--required") { + t.Fatalf("expected required-check invocation, log:\n%s", data) + } +} + +func TestPRChecks_AllAnnotatesRequiredAndActionOwnership(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build","state":"SUCCESS","bucket":"pass","link":"https://github.com/example/repo/actions/runs/101"},{"name":"scanner","state":"FAILURE","bucket":"fail","link":"https://scanner.example/check/7"}]`, + `FAKE_GH_REQUIRED_CHECKS_JSON=[{"name":"build","state":"SUCCESS","bucket":"pass","link":"https://github.com/example/repo/actions/runs/101"}]`, + }, "") + + checks, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/45", github.CheckScopeAll) + if err != nil { + t.Fatalf("PRChecks: %v", err) + } + if len(checks.Checks) != 2 { + t.Fatalf("expected two checks, got %+v", checks.Checks) + } + if !checks.Checks[0].Required || !checks.Checks[0].InScope || !checks.Checks[0].ActionsBacked || !checks.Checks[0].Rerunnable || checks.Checks[0].WorkflowRunID != "101" { + t.Fatalf("required Actions ownership was not modeled: %+v", checks.Checks[0]) + } + if checks.Checks[1].Required || !checks.Checks[1].InScope || checks.Checks[1].ActionsBacked || checks.Checks[1].Rerunnable { + t.Fatalf("external optional ownership was not modeled: %+v", checks.Checks[1]) + } +} + +func TestPRChecks_MarksMalformedActionsLinkNonRerunnable(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/not-a-number"}]`, + }, "") + + checks, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/46", github.CheckScopeRequired) + if err != nil { + t.Fatalf("PRChecks: %v", err) + } + if !checks.Checks[0].ActionsBacked || checks.Checks[0].Rerunnable || checks.Checks[0].WorkflowRunID != "" { + t.Fatalf("malformed Actions link should be owned but not rerunnable: %+v", checks.Checks[0]) + } +} + +func TestPRChecks_DoesNotTreatThirdPartyActionsPathAsOwned(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"scanner","state":"FAILURE","bucket":"fail","link":"https://scanner.example/actions/runs/99"}]`, + }, "") + + checks, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/49", github.CheckScopeRequired) + if err != nil { + t.Fatalf("PRChecks: %v", err) + } + if checks.Checks[0].ActionsBacked || checks.Checks[0].Rerunnable || checks.Checks[0].WorkflowRunID != "" { + t.Fatalf("third-party Actions-shaped link should not be rerunnable: %+v", checks.Checks[0]) + } +} + +func TestPRChecks_DoesNotTreatOtherGitHubRepositoryAsOwned(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"scanner","state":"FAILURE","bucket":"fail","link":"https://github.com/other-owner/other-repo/actions/runs/99"}]`, + }, "") + + checks, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/50", github.CheckScopeRequired) + if err != nil { + t.Fatalf("PRChecks: %v", err) + } + if checks.Checks[0].ActionsBacked || checks.Checks[0].Rerunnable || checks.Checks[0].WorkflowRunID != "" { + t.Fatalf("other GitHub repository should not be rerunnable: %+v", checks.Checks[0]) + } +} + +func TestPRChecks_SurfacesAPIErrorWithJSONPayload(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build","state":"SUCCESS","bucket":"pass","link":"https://github.com/example/repo/actions/runs/47"}]`, + "FAKE_GH_CHECKS_EXIT_CODE=1", + "FAKE_GH_STDERR=GitHub API unavailable", + }, "") + + _, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/47", github.CheckScopeRequired) + if err == nil || !strings.Contains(err.Error(), "GitHub API unavailable") { + t.Fatalf("expected API failure, got %v", err) + } +} + +func TestPRChecks_SurfacesRateLimitFailureWithJSONPayload(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build","state":"SUCCESS","bucket":"pass","link":"https://github.com/example/repo/actions/runs/44"}]`, + "FAKE_GH_CHECKS_EXIT_CODE=1", + "FAKE_GH_STDERR=API rate limit exceeded", + }, "") + + _, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/44", github.CheckScopeRequired) + if err == nil || !strings.Contains(strings.ToLower(err.Error()), "rate limit") { + t.Fatalf("expected rate-limit error, got %v", err) + } +} + +func TestPRChecks_AllowsEmptyRequiredSet(t *testing.T) { + c := newClient(t, []string{"FAKE_GH_CHECKS_JSON=[]"}, "") + + checks, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/42", github.CheckScopeRequired) + if err != nil { + t.Fatalf("PRChecks rejected an empty required check set: %v", err) + } + if len(checks.Checks) != 0 { + t.Fatalf("expected no applicable required checks, got %+v", checks.Checks) + } +} diff --git a/internal/github/client.go b/internal/github/client.go index e56827c..1f53fd8 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -4,10 +4,10 @@ import ( "context" "encoding/json" "fmt" - "net/url" "os" "strconv" "strings" + "sync" "time" "github.com/douglasjarquin/made/internal/exec" @@ -18,9 +18,12 @@ type Client struct { Binary string ExtraEnv []string Timeout time.Duration + + authMu sync.Mutex + authUntil time.Time } -const maxCheckLogBytes = 64 * 1024 +const authCacheTTL = time.Minute type AuthError struct { Detail string @@ -30,6 +33,15 @@ func (e *AuthError) Error() string { return fmt.Sprintf("github: not authenticated: %s", e.Detail) } +type RateLimitError struct { + Operation string + Detail string +} + +func (e *RateLimitError) Error() string { + return fmt.Sprintf("github: rate limit during %s: %s", e.Operation, e.Detail) +} + type CreatePROptions struct { Title string Body string @@ -37,27 +49,25 @@ type CreatePROptions struct { Head string } -type CheckResult struct { - Name string `json:"name"` - State string `json:"state"` - Bucket string `json:"bucket"` - Link string `json:"link"` - RunID string `json:"-"` -} - -type ChecksResult struct { - Checks []CheckResult - ExitCode int -} - func (c *Client) AuthStatus(ctx context.Context) error { + c.authMu.Lock() + defer c.authMu.Unlock() + if time.Now().Before(c.authUntil) { + return nil + } + res, err := c.run(ctx, "auth", "status") if err != nil { return fmt.Errorf("github: run gh auth status: %w", err) } if res.ExitCode != 0 { - return &AuthError{Detail: strings.TrimSpace(string(res.Stderr))} + detail := strings.TrimSpace(string(res.Stderr)) + if isRateLimitDetail(detail) { + return &RateLimitError{Operation: "gh auth status", Detail: detail} + } + return &AuthError{Detail: detail} } + c.authUntil = time.Now().Add(authCacheTTL) return nil } @@ -134,83 +144,6 @@ func (c *Client) MergeableState(ctx context.Context, prURL string) (string, erro return payload.MergeStateStatus, nil } -func (c *Client) PRChecks(ctx context.Context, prURL string) (ChecksResult, error) { - if strings.TrimSpace(prURL) == "" { - return ChecksResult{}, fmt.Errorf("github: pull request URL is required for checks") - } - if err := c.AuthStatus(ctx); err != nil { - return ChecksResult{}, err - } - - res, err := c.run(ctx, "pr", "checks", prURL, "--json", "name,state,bucket,link") - if err != nil { - return ChecksResult{}, fmt.Errorf("github: run gh pr checks: %w", err) - } - if len(strings.TrimSpace(string(res.Stdout))) == 0 { - return ChecksResult{}, fmt.Errorf("github: gh pr checks returned no JSON (exit %d): %s", res.ExitCode, strings.TrimSpace(string(res.Stderr))) - } - - var checks []CheckResult - if err := json.Unmarshal(res.Stdout, &checks); err != nil { - return ChecksResult{}, fmt.Errorf("github: parse gh pr checks output: %w: stdout=%s", err, res.Stdout) - } - if len(checks) == 0 { - return ChecksResult{}, fmt.Errorf("github: gh pr checks returned an empty check set") - } - for i := range checks { - checks[i].RunID = workflowRunID(checks[i].Link) - } - if res.ExitCode == 0 { - for _, check := range checks { - bucket := strings.ToLower(strings.TrimSpace(check.Bucket)) - if bucket != "pass" && bucket != "skipping" && bucket != "neutral" { - return ChecksResult{}, fmt.Errorf("github: gh pr checks exit 0 with non-success bucket %q for %q", check.Bucket, check.Name) - } - } - } - return ChecksResult{Checks: checks, ExitCode: res.ExitCode}, nil -} - -func (c *Client) CheckLogs(ctx context.Context, runID string) (string, error) { - if err := validateWorkflowRunID(runID); err != nil { - return "", err - } - if err := c.AuthStatus(ctx); err != nil { - return "", err - } - - res, err := c.run(ctx, "run", "view", runID, "--log") - if err != nil { - return "", fmt.Errorf("github: run gh run view: %w", err) - } - if res.ExitCode != 0 { - return "", fmt.Errorf("github: gh run view failed: %s", strings.TrimSpace(string(res.Stderr))) - } - output := res.Stdout - if len(output) > maxCheckLogBytes { - output = append(append([]byte(nil), output[:maxCheckLogBytes]...), []byte("\n[truncated]\n")...) - } - return string(output), nil -} - -func (c *Client) RerunCheck(ctx context.Context, runID string) error { - if err := validateWorkflowRunID(runID); err != nil { - return err - } - if err := c.AuthStatus(ctx); err != nil { - return err - } - - res, err := c.run(ctx, "run", "rerun", runID, "--failed") - if err != nil { - return fmt.Errorf("github: run gh run rerun: %w", err) - } - if res.ExitCode != 0 { - return fmt.Errorf("github: gh run rerun failed: %s", strings.TrimSpace(string(res.Stderr))) - } - return nil -} - func (c *Client) run(ctx context.Context, args ...string) (*exec.Result, error) { binary := c.Binary if binary == "" { @@ -241,18 +174,15 @@ func validateWorkflowRunID(runID string) error { return nil } -func workflowRunID(link string) string { - parsed, err := url.Parse(link) - if err != nil { - return "" +func commandFailure(operation string, res *exec.Result) error { + detail := strings.TrimSpace(string(res.Stderr)) + if isRateLimitDetail(detail) { + return &RateLimitError{Operation: operation, Detail: detail} } - parts := strings.Split(strings.Trim(parsed.Path, "/"), "/") - for i := 0; i+1 < len(parts); i++ { - if parts[i] == "runs" { - if _, err := strconv.ParseUint(parts[i+1], 10, 64); err == nil { - return parts[i+1] - } - } - } - return "" + return fmt.Errorf("github: %s failed: %s", operation, detail) +} + +func isRateLimitDetail(detail string) bool { + lower := strings.ToLower(detail) + return strings.Contains(lower, "rate limit") || strings.Contains(lower, "api rate limit exceeded") } diff --git a/internal/github/client_test.go b/internal/github/client_test.go index 148f63e..fa39a92 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -100,34 +100,11 @@ func TestCreatePR_SuccessReturnsURL(t *testing.T) { } } -func TestPRChecks_ParsesJSON(t *testing.T) { - c := newClient(t, []string{`FAKE_GH_CHECKS_JSON=[{"name":"build","state":"COMPLETED","bucket":"pass","link":"https://github.com/example/repo/actions/runs/42"}]`}, "") - - checks, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/42") - if err != nil { - t.Fatalf("PRChecks: %v", err) - } - if checks.ExitCode != 0 || len(checks.Checks) != 1 { - t.Fatalf("unexpected checks result: %+v", checks) - } - if checks.Checks[0].Bucket != "pass" || checks.Checks[0].RunID != "42" { - t.Fatalf("unexpected check fields: %+v", checks.Checks[0]) - } -} - -func TestPRChecks_RejectsEmptySuccessfulPayload(t *testing.T) { - c := newClient(t, []string{"FAKE_GH_CHECKS_JSON=[]"}, "") - - if _, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/42"); err == nil { - t.Fatal("PRChecks accepted an empty successful payload") - } -} - func TestMergeableState_AuthFailurePreventsCall(t *testing.T) { logPath := filepath.Join(t.TempDir(), "invocations.log") c := newClient(t, []string{"FAKE_GH_AUTH_EXIT_CODE=1"}, logPath) - _, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/42") + _, err := c.PRChecks(context.Background(), "https://github.com/example/repo/pull/42", github.CheckScopeRequired) if err == nil { t.Fatal("expected an error when auth fails") } diff --git a/internal/github/live_test.go b/internal/github/live_test.go index 3c5e455..d056ff0 100644 --- a/internal/github/live_test.go +++ b/internal/github/live_test.go @@ -66,9 +66,38 @@ func TestLive_AuthStatusAndPRCreation(t *testing.T) { } t.Logf("created PR: %s", url) - checks, err := c.PRChecks(context.Background(), url) + checks, err := c.PRChecks(context.Background(), url, github.CheckScopeAll) if err != nil { t.Fatalf("PRChecks: %v", err) } t.Logf("checks: %+v", checks) } + +func TestLive_CIWorkflowOwnershipSmoke(t *testing.T) { + repoDir := os.Getenv("MADE_GITHUB_SMOKE_REPO") + prURL := os.Getenv("MADE_GITHUB_SMOKE_PR_URL") + if repoDir == "" || prURL == "" { + t.Skip("set MADE_GITHUB_SMOKE_REPO and MADE_GITHUB_SMOKE_PR_URL for the disposable-repository smoke") + } + + c := &github.Client{Dir: repoDir} + if err := c.AuthStatus(context.Background()); err != nil { + t.Fatalf("AuthStatus: %v", err) + } + checks, err := c.PRChecks(context.Background(), prURL, github.CheckScopeAll) + if err != nil { + t.Fatalf("PRChecks: %v", err) + } + if len(checks.Checks) == 0 { + t.Fatal("expected the disposable PR to expose at least one check") + } + for _, check := range checks.Checks { + if !check.InScope { + t.Fatalf("all-scope check was not marked in scope: %+v", check) + } + if check.Rerunnable && (!check.ActionsBacked || check.WorkflowRunID == "") { + t.Fatalf("rerunnable check lacks Actions ownership: %+v", check) + } + t.Logf("check name=%q required=%t actions_backed=%t rerunnable=%t run_id=%q link=%q", check.Name, check.Required, check.ActionsBacked, check.Rerunnable, check.WorkflowRunID, check.DetailsLink) + } +} diff --git a/internal/github/testdata/fakegh/main.go b/internal/github/testdata/fakegh/main.go index 6c4334a..7af69db 100644 --- a/internal/github/testdata/fakegh/main.go +++ b/internal/github/testdata/fakegh/main.go @@ -40,9 +40,20 @@ func main() { failIfScripted() fmt.Fprint(os.Stdout, envOr("FAKE_GH_PR_LIST_JSON", "[]")) case len(args) == 5 && args[0] == "pr" && args[1] == "checks" && args[3] == "--json" && args[4] == "name,state,bucket,link": - payload := checksResponse() + payload := checksResponse(false) fmt.Fprint(os.Stdout, payload) if code := envExitCode("FAKE_GH_CHECKS_EXIT_CODE"); code != 0 { + fmt.Fprintln(os.Stderr, envOr("FAKE_GH_STDERR", "fakegh: scripted checks failure")) + os.Exit(code) + } + if checksFail(payload) { + os.Exit(1) + } + case len(args) == 6 && args[0] == "pr" && args[1] == "checks" && args[3] == "--required" && args[4] == "--json" && args[5] == "name,state,bucket,link": + payload := checksResponse(true) + fmt.Fprint(os.Stdout, payload) + if code := envExitCode("FAKE_GH_CHECKS_EXIT_CODE"); code != 0 { + fmt.Fprintln(os.Stderr, envOr("FAKE_GH_STDERR", "fakegh: scripted checks failure")) os.Exit(code) } if checksFail(payload) { @@ -50,7 +61,7 @@ func main() { } case len(args) == 4 && args[0] == "run" && args[1] == "view" && isRunID(args[2]) && args[3] == "--log": failIfScripted() - fmt.Fprint(os.Stdout, envOr("FAKE_GH_RUN_LOG", "log line 1\nlog line 2\n")) + fmt.Fprint(os.Stdout, runLog()) case len(args) == 4 && args[0] == "run" && args[1] == "rerun" && isRunID(args[2]) && args[3] == "--failed": failIfScripted() default: @@ -70,6 +81,19 @@ func failIfScripted() { } } +func runLog() string { + rawSize := os.Getenv("FAKE_GH_RUN_LOG_SIZE") + if rawSize == "" { + return envOr("FAKE_GH_RUN_LOG", "log line 1\nlog line 2\n") + } + size, err := strconv.Atoi(rawSize) + if err != nil || size < 0 { + fmt.Fprintln(os.Stderr, "fakegh: FAKE_GH_RUN_LOG_SIZE must be a non-negative integer") + os.Exit(2) + } + return strings.Repeat("x", size) +} + func envExitCode(key string) int { code := os.Getenv(key) if code == "" || code == "0" { @@ -118,8 +142,11 @@ func envOr(key, fallback string) string { return fallback } -func checksResponse() string { +func checksResponse(required bool) string { raw := envOr("FAKE_GH_CHECKS_JSON", `[{"name":"build","state":"COMPLETED","bucket":"pass","link":"https://github.com/example/repo/actions/runs/12345"}]`) + if required { + raw = envOr("FAKE_GH_REQUIRED_CHECKS_JSON", raw) + } var checks []map[string]string if err := json.Unmarshal([]byte(raw), &checks); err != nil { fmt.Fprintf(os.Stderr, "fakegh: invalid FAKE_GH_CHECKS_JSON: %v\n", err) diff --git a/internal/orchestrator/workfunc.go b/internal/orchestrator/workfunc.go index b344de2..399753e 100644 --- a/internal/orchestrator/workfunc.go +++ b/internal/orchestrator/workfunc.go @@ -423,7 +423,7 @@ func (c *chain) ciStage(prURL string) error { ciCtx, cancel := context.WithTimeout(c.ctx, c.rc.Config.StageTimeout(stageNameCI)) defer cancel() - result, err := ci.Run(ciCtx, c.rc.GitHub, prURL, c.rc.Config.CI.RerunBudget, ciPollInterval) + result, err := ci.Run(ciCtx, c.rc.GitHub, prURL, c.rc.Config.CI.CheckScope, c.rc.Config.CI.RerunBudget, ciPollInterval) if err != nil { return err } diff --git a/internal/pipeline/ci/ci.go b/internal/pipeline/ci/ci.go index c13c5c5..755a8af 100644 --- a/internal/pipeline/ci/ci.go +++ b/internal/pipeline/ci/ci.go @@ -1,8 +1,3 @@ -// Package ci is stage 9 of made's pipeline (Intent -> Rebase -> Review -> -// Test -> Document -> Lint -> Push -> PR -> CI): after a PR is opened it -// polls the PR's checks and, within a hard rerun budget, auto-reruns -// failures that might be transient (flaky infra, a network blip in CI) -// before giving up and reporting a final failure with a log excerpt. package ci import ( @@ -14,34 +9,28 @@ import ( "github.com/douglasjarquin/made/internal/github" ) -const ( - defaultPollInterval = 2 * time.Second -) +const defaultPollInterval = 2 * time.Second type Result struct { - OK bool - Message string - RerunsUsed int - LogExcerpt string + OK bool + Message string + RerunRoundsUsed int + FailureEvidence []FailureEvidence } -// Run's error return is reserved for infrastructure/configuration failures -// (a nil client, missing PR URL, negative budget); a check that fails on -// GitHub - even after exhausting the rerun budget - is a normal outcome -// reported via Result.OK, not an error, following the pr stage's convention -// (internal/pipeline/pr). -// -// rerunBudget is a hard cap on auto-reruns: without one, a genuinely broken -// (non-transient) check would rerun forever, burning CI minutes and GitHub -// API quota. pollInterval controls the wait between status checks; pass 0 -// to use a production-sized default, or a short duration in tests. -func Run(ctx context.Context, ghClient *github.Client, prURL string, rerunBudget int, pollInterval time.Duration) (Result, error) { +func Run(ctx context.Context, ghClient *github.Client, prURL string, scope github.CheckScope, rerunBudget int, pollInterval time.Duration) (Result, error) { if ghClient == nil { return Result{}, fmt.Errorf("ci: ghClient must not be nil") } if strings.TrimSpace(prURL) == "" { return Result{}, fmt.Errorf("ci: prURL must not be empty") } + if scope == "" { + scope = github.CheckScopeRequired + } + if !scope.Valid() { + return Result{}, fmt.Errorf("ci: unsupported check scope %q", scope) + } if rerunBudget < 0 { return Result{}, fmt.Errorf("ci: rerunBudget must not be negative") } @@ -49,86 +38,100 @@ func Run(ctx context.Context, ghClient *github.Client, prURL string, rerunBudget pollInterval = defaultPollInterval } - reruns := 0 + rounds := 0 for { - checks, err := ghClient.PRChecks(ctx, prURL) + checks, err := ghClient.PRChecks(ctx, prURL, scope) if err != nil { + if ctx.Err() != nil { + return Result{OK: false, Message: ctx.Err().Error(), RerunRoundsUsed: rounds}, nil + } return Result{}, err } - if hasPendingChecks(checks.Checks) { - select { - case <-ctx.Done(): - return Result{OK: false, Message: ctx.Err().Error(), RerunsUsed: reruns}, nil - case <-time.After(pollInterval): - continue + if len(checks.Checks) == 0 { + return Result{OK: false, Message: fmt.Sprintf("no applicable %s checks for %s", scope, prURL), RerunRoundsUsed: rounds}, nil + } + + pending, failures := terminalFailures(checks.Checks) + if pending { + if err := waitForPoll(ctx, pollInterval); err != nil { + return Result{OK: false, Message: err.Error(), RerunRoundsUsed: rounds}, nil } + continue } - if checks.ExitCode == 0 { + if len(failures) == 0 { return Result{ - OK: true, - Message: fmt.Sprintf("checks passed for %s after %d rerun(s)", prURL, reruns), - RerunsUsed: reruns, + OK: true, + Message: fmt.Sprintf("checks passed for %s after %d rerun round(s)", prURL, rounds), + RerunRoundsUsed: rounds, }, nil } - if reruns >= rerunBudget { - runID := firstWorkflowRunID(checks.Checks) - if runID == "" { - return Result{ - OK: false, - Message: fmt.Sprintf("checks failed for %s after exhausting rerun budget (%d), but no workflow run ID was present in gh pr checks output", prURL, rerunBudget), - RerunsUsed: reruns, - }, nil - } - excerpt, logErr := ghClient.CheckLogs(ctx, runID) - if logErr != nil { - return Result{}, logErr + if rounds >= rerunBudget { + logs, err := fetchFailureLogs(ctx, ghClient, failures) + if err != nil { + if ctx.Err() != nil { + return Result{OK: false, Message: ctx.Err().Error(), RerunRoundsUsed: rounds}, nil + } + return Result{}, err } + evidence := collectFailureEvidence(failures, logs) return Result{ - OK: false, - Message: fmt.Sprintf("checks still failing for %s after exhausting rerun budget (%d)", prURL, rerunBudget), - RerunsUsed: reruns, - LogExcerpt: excerpt, + OK: false, + Message: formatFailureMessage(prURL, rounds, rerunBudget, evidence), + RerunRoundsUsed: rounds, + FailureEvidence: evidence, }, nil } - runID := firstWorkflowRunID(checks.Checks) - if runID == "" { + runIDs := rerunnableRunIDs(failures) + if len(runIDs) == 0 { + evidence := collectFailureEvidence(failures, nil) return Result{ - OK: false, - Message: fmt.Sprintf("checks failed for %s but gh pr checks returned no workflow run ID for rerun", prURL), - RerunsUsed: reruns, + OK: false, + Message: formatFailureMessage(prURL, rounds, rerunBudget, evidence), + RerunRoundsUsed: rounds, + FailureEvidence: evidence, }, nil } - if err := ghClient.RerunCheck(ctx, runID); err != nil { - return Result{}, err + for _, runID := range runIDs { + if err := ghClient.RerunCheck(ctx, runID); err != nil { + if ctx.Err() != nil { + return Result{OK: false, Message: ctx.Err().Error(), RerunRoundsUsed: rounds}, nil + } + return Result{}, err + } } - reruns++ + rounds++ - select { - case <-ctx.Done(): - return Result{OK: false, Message: ctx.Err().Error(), RerunsUsed: reruns}, nil - case <-time.After(pollInterval): + if err := waitForPoll(ctx, pollInterval); err != nil { + return Result{OK: false, Message: err.Error(), RerunRoundsUsed: rounds}, nil } } } -func firstWorkflowRunID(checks []github.CheckResult) string { - for _, check := range checks { - if check.RunID != "" { - return check.RunID +func fetchFailureLogs(ctx context.Context, ghClient *github.Client, failures []github.CheckResult) (map[string]string, error) { + logs := make(map[string]string) + for index, runID := range rerunnableRunIDs(failures) { + if index >= maxFailureLogRuns { + logs[runID] = omittedFailureLog + continue } + excerpt, err := ghClient.CheckLogs(ctx, runID) + if err != nil { + return nil, err + } + logs[runID] = excerpt } - return "" + return logs, nil } -func hasPendingChecks(checks []github.CheckResult) bool { - for _, check := range checks { - state := strings.ToUpper(strings.TrimSpace(check.State)) - bucket := strings.ToLower(strings.TrimSpace(check.Bucket)) - if bucket == "pending" || state == "PENDING" || state == "QUEUED" || state == "IN_PROGRESS" || state == "WAITING" || state == "EXPECTED" { - return true - } +func waitForPoll(ctx context.Context, interval time.Duration) error { + timer := time.NewTimer(interval) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil } - return false } diff --git a/internal/pipeline/ci/ci_contract_test.go b/internal/pipeline/ci/ci_contract_test.go index a91f253..012a5f0 100644 --- a/internal/pipeline/ci/ci_contract_test.go +++ b/internal/pipeline/ci/ci_contract_test.go @@ -21,7 +21,7 @@ func TestRun_UsesPrChecksJSONContract(t *testing.T) { ExtraEnv: append(os.Environ(), "FAKE_GH_LOG_FILE="+logPath), } - _, _ = ci.Run(context.Background(), c, "https://github.com/example/repo/pull/7", 0, 0) + _, _ = ci.Run(context.Background(), c, "https://github.com/example/repo/pull/7", github.CheckScopeRequired, 0, 0) data, err := os.ReadFile(logPath) if err != nil { t.Fatalf("read invocation log: %v", err) @@ -48,7 +48,7 @@ func TestRun_PassesWorkflowRunIDToLogsAndRerun(t *testing.T) { ), } - _, _ = ci.Run(context.Background(), c, prURL, 1, 0) + _, _ = ci.Run(context.Background(), c, prURL, github.CheckScopeRequired, 1, 0) data, err := os.ReadFile(logPath) if err != nil { t.Fatalf("read invocation log: %v", err) diff --git a/internal/pipeline/ci/ci_evidence_test.go b/internal/pipeline/ci/ci_evidence_test.go new file mode 100644 index 0000000..3e28578 --- /dev/null +++ b/internal/pipeline/ci/ci_evidence_test.go @@ -0,0 +1,74 @@ +package ci_test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/douglasjarquin/made/internal/github" + "github.com/douglasjarquin/made/internal/pipeline/ci" +) + +func TestRun_BoundsAggregatedFailureEvidence(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/901"}]`, + "FAKE_GH_RUN_LOG_SIZE=131072", + }, "") + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/23", github.CheckScopeRequired, 0, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + messageTail := result.Message + if len(messageTail) > 32 { + messageTail = messageTail[len(messageTail)-32:] + } + if result.OK || len(result.FailureEvidence) != 1 || len(result.FailureEvidence[0].Excerpt) > 64*1024+len("\n[truncated]\n") || !strings.Contains(result.FailureEvidence[0].Excerpt, "[truncated]") || strings.Contains(result.Message, "[truncated]") { + t.Fatalf("failure evidence was not bounded outside the durable message: len=%d message suffix=%q evidence=%+v", len(result.Message), messageTail, result.FailureEvidence) + } +} + +func TestRun_BoundsFailureLogFetchesAcrossRuns(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "invocations.log") + checks := make([]string, 0, 5) + for runID := 901; runID <= 905; runID++ { + checks = append(checks, fmt.Sprintf(`{"name":"build-%d","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/%d"}`, runID, runID)) + } + c := newClient(t, []string{ + "FAKE_GH_CHECKS_JSON=[" + strings.Join(checks, ",") + "]", + "FAKE_GH_RUN_LOG=workflow failed\n", + }, logPath) + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/24", github.CheckScopeRequired, 0, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read invocation log: %v", err) + } + if got := strings.Count(string(data), "run view "); got != 4 { + t.Fatalf("expected at most four bounded log fetches, got %d, log:\n%s", got, data) + } + if len(result.FailureEvidence) != 5 || !strings.Contains(result.Message, "905") || !strings.Contains(result.FailureEvidence[4].Excerpt, "[log excerpt omitted after evidence limit]") { + t.Fatalf("bounded evidence omitted the skipped run identity or marker: message=%q evidence=%+v", result.Message, result.FailureEvidence) + } +} + +func TestRun_RedactsSecretsFromFailureEvidence(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/906"}]`, + "FAKE_GH_RUN_LOG=token=workflow-secret\n", + }, "") + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/25", github.CheckScopeRequired, 0, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + if strings.Contains(result.Message, "workflow-secret") || strings.Contains(result.FailureEvidence[0].Excerpt, "workflow-secret") || !strings.Contains(result.FailureEvidence[0].Excerpt, "token=[REDACTED]") { + t.Fatalf("failure evidence did not redact the secret: message=%q evidence=%+v", result.Message, result.FailureEvidence) + } +} diff --git a/internal/pipeline/ci/ci_issue3_test.go b/internal/pipeline/ci/ci_issue3_test.go new file mode 100644 index 0000000..a1d248b --- /dev/null +++ b/internal/pipeline/ci/ci_issue3_test.go @@ -0,0 +1,264 @@ +package ci_test + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/douglasjarquin/made/internal/github" + "github.com/douglasjarquin/made/internal/pipeline/ci" +) + +func TestRun_DoesNotRerunPendingChecks(t *testing.T) { + stateDir := t.TempDir() + logPath := filepath.Join(t.TempDir(), "invocations.log") + c := newClient(t, []string{ + "FAKE_GH_CHECKS_BUCKETS=pending,pass", + "FAKE_GH_STATE_DIR=" + stateDir, + }, logPath) + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/11", github.CheckScopeRequired, 2, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + if !result.OK || result.RerunRoundsUsed != 0 { + t.Fatalf("pending check was rerun: %+v", result) + } + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read invocation log: %v", err) + } + if strings.Contains(string(data), "run rerun") { + t.Fatalf("pending check triggered a rerun: %s", data) + } +} + +func TestRun_RerunsFailedActionsInsteadOfEarlierPassingActions(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "invocations.log") + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"lint","state":"COMPLETED","bucket":"pass","link":"https://github.com/example/repo/actions/runs/101"},{"name":"test","state":"COMPLETED","bucket":"fail","link":"https://github.com/example/repo/actions/runs/202"}]`, + }, logPath) + + _, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/12", github.CheckScopeRequired, 1, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read invocation log: %v", err) + } + if !strings.Contains(string(data), "run rerun 202") { + t.Fatalf("expected failed test workflow 202 to be rerun, log:\n%s", data) + } + if strings.Contains(string(data), "run rerun 101") { + t.Fatalf("passing lint workflow 101 was rerun, log:\n%s", data) + } +} + +func TestRun_RerunsUniqueFailedActionRunsInOneRound(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "invocations.log") + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build-linux","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/201"},{"name":"build-macos","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/201"},{"name":"deploy","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/202"}]`, + }, logPath) + + _, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/13", github.CheckScopeRequired, 1, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read invocation log: %v", err) + } + if strings.Count(string(data), "run rerun 201") != 1 || strings.Count(string(data), "run rerun 202") != 1 { + t.Fatalf("expected one rerun for each unique failed workflow, log:\n%s", data) + } +} + +func TestRun_DoesNotRerunExternalFailureOrPassingActions(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "invocations.log") + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build","state":"SUCCESS","bucket":"pass","link":"https://github.com/example/repo/actions/runs/301"},{"name":"security-scan","state":"FAILURE","bucket":"fail","link":"https://scanner.example/check/44"}]`, + }, logPath) + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/14", github.CheckScopeRequired, 1, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + if result.OK { + t.Fatalf("expected external failure to remain failed: %+v", result) + } + if !strings.Contains(result.Message, "security-scan") || !strings.Contains(result.Message, "https://scanner.example/check/44") { + t.Fatalf("failure message omitted external check identity: %q", result.Message) + } + + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read invocation log: %v", err) + } + if strings.Contains(string(data), "run rerun") { + t.Fatalf("passing Actions or external check was rerun, log:\n%s", data) + } +} + +func TestRun_NoApplicableChecksIsClearFailure(t *testing.T) { + c := newClient(t, []string{"FAKE_GH_CHECKS_JSON=[]"}, "") + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/15", github.CheckScopeRequired, 1, testPollInterval) + if err != nil { + t.Fatalf("Run returned infrastructure error for no applicable checks: %v", err) + } + if result.OK || !strings.Contains(strings.ToLower(result.Message), "no applicable") { + t.Fatalf("expected clear no-applicable-check failure, got %+v", result) + } +} + +func TestRun_SkippedAndNeutralChecksAreTerminalSuccess(t *testing.T) { + for _, tc := range []struct { + name string + state string + bucket string + }{ + {name: "skipped", state: "SKIPPED", bucket: "skipping"}, + {name: "neutral", state: "NEUTRAL", bucket: "neutral"}, + } { + t.Run(tc.name, func(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"optional-check","state":"` + tc.state + `","bucket":"` + tc.bucket + `","link":"https://scanner.example/check/55"}]`, + }, "") + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/16", github.CheckScopeRequired, 0, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + if !result.OK || result.RerunRoundsUsed != 0 { + t.Fatalf("expected %s check to be terminal success without rerun: %+v", tc.name, result) + } + }) + } +} + +func TestRun_ExplicitTerminalFailureStatesNameTheCheck(t *testing.T) { + for _, tc := range []struct { + name string + state string + }{ + {name: "canceled", state: "CANCELLED"}, + {name: "timed-out", state: "TIMED_OUT"}, + {name: "action-required", state: "ACTION_REQUIRED"}, + {name: "stale", state: "STALE"}, + } { + t.Run(tc.name, func(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"` + tc.name + `-check","state":"` + tc.state + `","bucket":"fail","link":"https://scanner.example/check/66"}]`, + }, "") + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/17", github.CheckScopeRequired, 0, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + if result.OK || !strings.Contains(strings.ToLower(result.Message), tc.name) { + t.Fatalf("terminal status was not surfaced by name: %+v", result) + } + }) + } +} + +func TestRun_FailureEvidenceNamesCheckAndRun(t *testing.T) { + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/501"}]`, + "FAKE_GH_RUN_LOG=build failed at step 3\n", + }, "") + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/18", github.CheckScopeRequired, 0, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + if result.OK || !strings.Contains(result.Message, "build") || !strings.Contains(result.Message, "501") { + t.Fatalf("failure message omitted check/run identity: %+v", result) + } + if len(result.FailureEvidence) != 1 || !strings.Contains(result.FailureEvidence[0].Excerpt, "build failed at step 3") { + t.Fatalf("failure evidence omitted workflow output: %+v", result.FailureEvidence) + } +} + +func TestRun_FailureEvidenceFetchesEachFailedRunOnce(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "invocations.log") + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"linux","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/701"},{"name":"macos","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/701"},{"name":"windows","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/702"}]`, + "FAKE_GH_RUN_LOG=workflow failed\n", + }, logPath) + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/20", github.CheckScopeRequired, 0, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + if result.OK || !strings.Contains(result.Message, "linux, macos") || !strings.Contains(result.Message, "windows") { + t.Fatalf("failure evidence did not associate names with runs: %+v", result) + } + + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read invocation log: %v", err) + } + if strings.Count(string(data), "run view 701") != 1 || strings.Count(string(data), "run view 702") != 1 { + t.Fatalf("expected one bounded log fetch per failed run, log:\n%s", data) + } +} + +func TestRun_PendingThenFailureDoesNotConsumeRound(t *testing.T) { + stateDir := t.TempDir() + logPath := filepath.Join(t.TempDir(), "invocations.log") + c := newClient(t, []string{ + "FAKE_GH_CHECKS_BUCKETS=pending,fail", + "FAKE_GH_STATE_DIR=" + stateDir, + }, logPath) + + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/21", github.CheckScopeRequired, 1, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + if result.RerunRoundsUsed != 1 { + t.Fatalf("pending observation consumed rerun budget: %+v", result) + } +} + +func TestRun_CancellationDuringPendingStopsPolling(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond) + defer cancel() + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build","state":"PENDING","bucket":"pending","link":"https://github.com/example/repo/actions/runs/801"}]`, + }, "") + + result, err := ci.Run(ctx, c, "https://github.com/example/repo/pull/22", github.CheckScopeRequired, 1, time.Second) + if err != nil { + t.Fatalf("Run returned infrastructure error during cancellation: %v", err) + } + if result.OK || !strings.Contains(result.Message, "deadline exceeded") || result.RerunRoundsUsed != 0 { + t.Fatalf("cancellation did not stop pending polling cleanly: %+v", result) + } +} + +func TestRun_CachesSuccessfulAuthAcrossChecksRerunsAndLogs(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "invocations.log") + c := newClient(t, []string{ + `FAKE_GH_CHECKS_JSON=[{"name":"build","state":"FAILURE","bucket":"fail","link":"https://github.com/example/repo/actions/runs/601"}]`, + "FAKE_GH_RUN_LOG=build failed\n", + }, logPath) + + _, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/19", github.CheckScopeRequired, 1, testPollInterval) + if err != nil { + t.Fatalf("Run: %v", err) + } + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read invocation log: %v", err) + } + if got := strings.Count(string(data), "auth status"); got != 1 { + t.Fatalf("expected one successful auth preflight across the run, got %d, log:\n%s", got, data) + } +} diff --git a/internal/pipeline/ci/ci_test.go b/internal/pipeline/ci/ci_test.go index 8c7d41f..29a4ce1 100644 --- a/internal/pipeline/ci/ci_test.go +++ b/internal/pipeline/ci/ci_test.go @@ -37,15 +37,15 @@ func TestRun_TransientFailureRecoversWithinBudget(t *testing.T) { "FAKE_GH_STATE_DIR=" + stateDir, }, logPath) - result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/7", 2, testPollInterval) + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/7", github.CheckScopeRequired, 2, testPollInterval) if err != nil { t.Fatalf("Run: %v", err) } if !result.OK { t.Fatalf("expected OK=true, got %+v", result) } - if result.RerunsUsed != 1 { - t.Fatalf("expected exactly one rerun, got %d", result.RerunsUsed) + if result.RerunRoundsUsed != 1 { + t.Fatalf("expected exactly one rerun round, got %d", result.RerunRoundsUsed) } data, readErr := os.ReadFile(logPath) @@ -58,30 +58,6 @@ func TestRun_TransientFailureRecoversWithinBudget(t *testing.T) { } } -func TestRun_DoesNotRerunPendingChecks(t *testing.T) { - stateDir := t.TempDir() - logPath := filepath.Join(t.TempDir(), "invocations.log") - c := newClient(t, []string{ - "FAKE_GH_CHECKS_BUCKETS=pending,pass", - "FAKE_GH_STATE_DIR=" + stateDir, - }, logPath) - - result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/11", 2, testPollInterval) - if err != nil { - t.Fatalf("Run: %v", err) - } - if !result.OK || result.RerunsUsed != 0 { - t.Fatalf("pending check was rerun: %+v", result) - } - data, err := os.ReadFile(logPath) - if err != nil { - t.Fatalf("read invocation log: %v", err) - } - if strings.Contains(string(data), "run rerun") { - t.Fatalf("pending check triggered a rerun: %s", data) - } -} - func TestRun_BudgetExhaustionSurfacesFinalFailure(t *testing.T) { c := newClient(t, []string{ "FAKE_GH_CHECKS_BUCKETS=fail", @@ -89,35 +65,35 @@ func TestRun_BudgetExhaustionSurfacesFinalFailure(t *testing.T) { }, "") const rerunBudget = 2 - result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/8", rerunBudget, testPollInterval) + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/8", github.CheckScopeRequired, rerunBudget, testPollInterval) if err != nil { t.Fatalf("Run: %v", err) } if result.OK { t.Fatalf("expected OK=false, got %+v", result) } - if result.RerunsUsed != rerunBudget { - t.Fatalf("expected RerunsUsed == rerunBudget (%d), got %d", rerunBudget, result.RerunsUsed) + if result.RerunRoundsUsed != rerunBudget { + t.Fatalf("expected RerunRoundsUsed == rerunBudget (%d), got %d", rerunBudget, result.RerunRoundsUsed) } - if result.LogExcerpt == "" { - t.Fatal("expected a non-empty log excerpt on final failure") + if len(result.FailureEvidence) == 0 { + t.Fatal("expected failure evidence on final failure") } - if !strings.Contains(result.LogExcerpt, "build failed at step 3") { - t.Fatalf("expected log excerpt to contain the check's log output, got %q", result.LogExcerpt) + if len(result.FailureEvidence) != 1 || !strings.Contains(result.FailureEvidence[0].Excerpt, "build failed at step 3") { + t.Fatalf("expected failure evidence to contain the check's log output, got %+v", result.FailureEvidence) } } func TestRun_RejectsEmptyPRURL(t *testing.T) { c := newClient(t, nil, "") - _, err := ci.Run(context.Background(), c, "", 2, testPollInterval) + _, err := ci.Run(context.Background(), c, "", github.CheckScopeRequired, 2, testPollInterval) if err == nil { t.Fatal("expected an error when prURL is empty") } } func TestRun_RejectsNilClient(t *testing.T) { - _, err := ci.Run(context.Background(), nil, "https://github.com/example/repo/pull/9", 2, testPollInterval) + _, err := ci.Run(context.Background(), nil, "https://github.com/example/repo/pull/9", github.CheckScopeRequired, 2, testPollInterval) if err == nil { t.Fatal("expected an error when ghClient is nil") } @@ -130,7 +106,7 @@ func TestRun_NeverExceedsBudgetEvenWithAlwaysFailingChecks(t *testing.T) { const rerunBudget = 3 start := time.Now() - result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/10", rerunBudget, testPollInterval) + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/10", github.CheckScopeRequired, rerunBudget, testPollInterval) elapsed := time.Since(start) if err != nil { t.Fatalf("Run: %v", err) @@ -138,8 +114,8 @@ func TestRun_NeverExceedsBudgetEvenWithAlwaysFailingChecks(t *testing.T) { if result.OK { t.Fatal("expected persistent failure to remain OK=false") } - if result.RerunsUsed != rerunBudget { - t.Fatalf("expected exactly rerunBudget reruns (%d), got %d - budget was not respected", rerunBudget, result.RerunsUsed) + if result.RerunRoundsUsed != rerunBudget { + t.Fatalf("expected exactly rerunBudget rounds (%d), got %d - budget was not respected", rerunBudget, result.RerunRoundsUsed) } if elapsed > 30*time.Second { t.Fatalf("Run took too long (%s) - suspect it looped past the budget", elapsed) diff --git a/internal/pipeline/ci/failure.go b/internal/pipeline/ci/failure.go new file mode 100644 index 0000000..438bb6c --- /dev/null +++ b/internal/pipeline/ci/failure.go @@ -0,0 +1,165 @@ +package ci + +import ( + "fmt" + "slices" + "sort" + "strings" + + "github.com/douglasjarquin/made/internal/evidence" + "github.com/douglasjarquin/made/internal/github" +) + +const maxFailureEvidenceBytes = 256 * 1024 + +const maxFailureLogRuns = 4 + +const omittedFailureLog = "[log excerpt omitted after evidence limit]" + +type FailureEvidence struct { + CheckNames []string + State string + Bucket string + DetailsLink string + WorkflowRunID string + Excerpt string +} + +func terminalFailures(checks []github.CheckResult) (pending bool, failures []github.CheckResult) { + for _, check := range checks { + if !check.InScope { + continue + } + if checkPending(check) { + pending = true + continue + } + if !checkSuccessful(check) { + failures = append(failures, check) + } + } + sort.Slice(failures, func(i, j int) bool { + if failures[i].Name != failures[j].Name { + return failures[i].Name < failures[j].Name + } + if failures[i].WorkflowRunID != failures[j].WorkflowRunID { + return failures[i].WorkflowRunID < failures[j].WorkflowRunID + } + return failures[i].DetailsLink < failures[j].DetailsLink + }) + return pending, failures +} + +func checkPending(check github.CheckResult) bool { + state := strings.ToUpper(strings.TrimSpace(check.State)) + bucket := strings.ToLower(strings.TrimSpace(check.Bucket)) + if bucket == "pending" { + return true + } + switch state { + case "PENDING", "QUEUED", "IN_PROGRESS", "WAITING", "EXPECTED": + return true + default: + return false + } +} + +func checkSuccessful(check github.CheckResult) bool { + bucket := strings.ToLower(strings.TrimSpace(check.Bucket)) + if bucket == "fail" || bucket == "cancel" { + return false + } + if bucket == "pass" || bucket == "skipping" || bucket == "neutral" { + return true + } + switch strings.ToUpper(strings.TrimSpace(check.State)) { + case "SUCCESS", "COMPLETED", "SKIPPED", "NEUTRAL": + return true + default: + return false + } +} + +func rerunnableRunIDs(checks []github.CheckResult) []string { + seen := make(map[string]struct{}, len(checks)) + ids := make([]string, 0, len(checks)) + for _, check := range checks { + if !check.Rerunnable || check.WorkflowRunID == "" { + continue + } + if _, ok := seen[check.WorkflowRunID]; ok { + continue + } + seen[check.WorkflowRunID] = struct{}{} + ids = append(ids, check.WorkflowRunID) + } + sort.Strings(ids) + return ids +} + +func collectFailureEvidence(checks []github.CheckResult, logs map[string]string) []FailureEvidence { + result := make([]FailureEvidence, 0, len(checks)) + indexes := make(map[string]int, len(checks)) + for _, check := range checks { + if check.Rerunnable && check.WorkflowRunID != "" { + key := "run:" + check.WorkflowRunID + if index, ok := indexes[key]; ok { + result[index].CheckNames = appendUnique(result[index].CheckNames, check.Name) + continue + } + indexes[key] = len(result) + result = append(result, FailureEvidence{ + CheckNames: []string{check.Name}, + State: check.State, + Bucket: check.Bucket, + DetailsLink: check.DetailsLink, + WorkflowRunID: check.WorkflowRunID, + Excerpt: evidence.RedactString(logs[check.WorkflowRunID]), + }) + continue + } + result = append(result, FailureEvidence{ + CheckNames: []string{check.Name}, + State: check.State, + Bucket: check.Bucket, + DetailsLink: check.DetailsLink, + }) + } + return result +} + +func appendUnique(values []string, value string) []string { + if slices.Contains(values, value) { + return values + } + return append(values, value) +} + +func formatFailureMessage(prURL string, rounds, budget int, evidence []FailureEvidence) string { + var builder strings.Builder + fmt.Fprintf(&builder, "checks failed for %s after %d rerun round(s) (budget %d)", prURL, rounds, budget) + for _, item := range evidence { + fmt.Fprintf(&builder, "\n- %s [state=%s bucket=%s]", strings.Join(item.CheckNames, ", "), item.State, item.Bucket) + if item.WorkflowRunID != "" { + fmt.Fprintf(&builder, " Actions run %s", item.WorkflowRunID) + } + if item.DetailsLink != "" { + fmt.Fprintf(&builder, " (%s)", item.DetailsLink) + } + if builder.Len() >= maxFailureEvidenceBytes { + break + } + } + return boundText(builder.String(), maxFailureEvidenceBytes) +} + +func boundText(value string, maxBytes int) string { + if len(value) <= maxBytes { + return value + } + marker := "\n[truncated]\n" + if len(marker) >= maxBytes { + return marker[:maxBytes] + } + return value[:maxBytes-len(marker)] + marker +} diff --git a/internal/pipeline/ci/remediation_contract_test.go b/internal/pipeline/ci/remediation_contract_test.go index 8c74f56..1546738 100644 --- a/internal/pipeline/ci/remediation_contract_test.go +++ b/internal/pipeline/ci/remediation_contract_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/douglasjarquin/made/internal/github" "github.com/douglasjarquin/made/internal/pipeline/ci" ) @@ -14,7 +15,7 @@ func TestRun_AuthenticationFailureIsInfrastructureError(t *testing.T) { "FAKE_GH_AUTH_STDERR=not authenticated", }, "") - result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/42", 0, time.Millisecond) + result, err := ci.Run(context.Background(), c, "https://github.com/example/repo/pull/42", github.CheckScopeRequired, 0, time.Millisecond) if err == nil { t.Fatalf("authentication failure was reported as a failed check: result=%+v", result) } diff --git a/internal/skill/skill.go b/internal/skill/skill.go index cc4e35c..cffb387 100644 --- a/internal/skill/skill.go +++ b/internal/skill/skill.go @@ -119,6 +119,12 @@ and the run resumes. reports the PR as open and awaiting merge, without waiting for a human to merge it. +The trusted ` + "`.made.yml`" + ` copy may set ` + "`ci.check_scope`" + ` to ` + "`required`" + ` or ` + "`all`" + `; the default is ` + "`required`" + `. +` + "`ci.rerun_budget`" + ` counts rerun rounds, not individual checks. +Made polls pending checks without spending a round, reruns each unique failed GitHub Actions workflow run only, and reports bounded evidence by failed check and run. +External checks are reported by name and link and are never rerun. +The opt-in disposable-repository smoke contract is ` + "`MADE_GITHUB_SMOKE_REPO`" + ` plus ` + "`MADE_GITHUB_SMOKE_PR_URL`" + `, and ` + "`make release-validation`" + ` runs it when both are set. + ## Outcomes ` + "`made run status --json `" + `'s ` + "`state`" + ` field is one of ` + "`queued`" + `, diff --git a/skills/made/SKILL.md b/skills/made/SKILL.md index 19af6c4..55a5830 100644 --- a/skills/made/SKILL.md +++ b/skills/made/SKILL.md @@ -89,6 +89,12 @@ and the run resumes. reports the PR as open and awaiting merge, without waiting for a human to merge it. +The trusted `.made.yml` copy may set `ci.check_scope` to `required` or `all`; the default is `required`. +`ci.rerun_budget` counts rerun rounds, not individual checks. +Made polls pending checks without spending a round, reruns each unique failed GitHub Actions workflow run only, and reports bounded evidence by failed check and run. +External checks are reported by name and link and are never rerun. +The opt-in disposable-repository smoke contract is `MADE_GITHUB_SMOKE_REPO` plus `MADE_GITHUB_SMOKE_PR_URL`, and `make release-validation` runs it when both are set. + ## Outcomes `made run status --json `'s `state` field is one of `queued`,