From 8520b689de318a4071b80f174b4dd375f6e969bc Mon Sep 17 00:00:00 2001 From: Adam Hamrick Date: Thu, 27 Aug 2026 15:47:22 -0400 Subject: [PATCH 1/3] feat(githooks): prompt breaking up large PRs --- lefthook.yml | 4 + tools/githooks/README.md | 25 ++ tools/githooks/cmd/cmd_bench_test.go | 1 + tools/githooks/cmd/lint_test.go | 1 + tools/githooks/cmd/pr_size.go | 69 +++ tools/githooks/cmd/pr_size_test.go | 54 +++ tools/githooks/cmd/root.go | 1 + tools/githooks/internal/prsize/prsize.go | 404 ++++++++++++++++++ .../internal/prsize/prsize_bench_test.go | 55 +++ tools/githooks/internal/prsize/prsize_test.go | 343 +++++++++++++++ tools/githooks/skills/split-pr/SKILL.md | 149 +++++++ 11 files changed, 1106 insertions(+) create mode 100644 tools/githooks/cmd/pr_size.go create mode 100644 tools/githooks/cmd/pr_size_test.go create mode 100644 tools/githooks/internal/prsize/prsize.go create mode 100644 tools/githooks/internal/prsize/prsize_bench_test.go create mode 100644 tools/githooks/internal/prsize/prsize_test.go create mode 100644 tools/githooks/skills/split-pr/SKILL.md diff --git a/lefthook.yml b/lefthook.yml index dad9fc43f70..f105aa3a868 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -107,3 +107,7 @@ pre-push: - "go.mod" - "go.sum" run: tools/githooks/.bin/githooks test {push_files} + + pr-size-guard: + tags: [guard, diff] + run: tools/githooks/.bin/githooks pr-size diff --git a/tools/githooks/README.md b/tools/githooks/README.md index 69975e6c513..cb28d2af2fa 100644 --- a/tools/githooks/README.md +++ b/tools/githooks/README.md @@ -11,6 +11,7 @@ - **Parallel Module Tidy:** Runs `go mod tidy` in parallel across all affected modules. - **Targeted Linting:** Runs `golangci-lint` only against the exact changed packages within affected modules instead of scanning whole modules or the entire repository. - **Targeted Unit Testing:** Discovers changed test packages and executes `tools/test` with `-short` directly on those packages (aligned with CI unit test scope). +- **PR Diff Size Guard:** Calculates branch diff against the default branch merge-base, classifies size (`small`, `medium`, `large`), warns or fails on large diffs, and prompts developers to break PRs into smaller chunks. - **Dependency Changes:** Automatically runs on all packages (`./...`) if a module's `go.mod` or `go.sum` is modified. - **Lefthook Integration:** Seamlessly works with Lefthook staged/push file filters and `stage_fixed`. @@ -115,6 +116,30 @@ go -C tools/githooks run . test core/logger/logger_test.go tools/ci-testshard/ma go -C tools/githooks run . test --short=false ``` +### `pr-size` (aliases: `big-pr-guard`, `pr-guard`, `diff-guard`, `diff-size`) + +Calculates the git diff of the current branch against the default branch (the PR diff), classifies it into size categories (`small`, `medium`, `large`), and prompts the developer with recommendations to split large PRs. + +```bash +# Check PR diff size with default settings +go -C tools/githooks run . pr-size + +# Check with custom thresholds +go -C tools/githooks run . pr-size --small-limit=100 --medium-limit=300 + +# Fail the command on large PRs (useful in CI or strict pre-push mode) +go -C tools/githooks run . pr-size --fail-on-large + +# Select calculation strategy: per-file-max (default), sum, max, or weighted +go -C tools/githooks run . pr-size --strategy=per-file-max + +# Diff against a specific base branch or ref +go -C tools/githooks run . pr-size --base=origin/develop + +# Automatically ignores generated files defined in .gitattributes (linguist-generated=true) +go -C tools/githooks run . pr-size --ignore-generated=true +``` + ## Running Tests & Benchmarks ```bash diff --git a/tools/githooks/cmd/cmd_bench_test.go b/tools/githooks/cmd/cmd_bench_test.go index 3f0a8934892..4b05577bd14 100644 --- a/tools/githooks/cmd/cmd_bench_test.go +++ b/tools/githooks/cmd/cmd_bench_test.go @@ -18,6 +18,7 @@ func BenchmarkCommandsHelp(b *testing.B) { {"generate", "--help"}, {"end-of-file-fixer", "--help"}, {"whitespace-fixer", "--help"}, + {"pr-size", "--help"}, } for _, args := range commands { diff --git a/tools/githooks/cmd/lint_test.go b/tools/githooks/cmd/lint_test.go index d552d4a6098..7315186fb3b 100644 --- a/tools/githooks/cmd/lint_test.go +++ b/tools/githooks/cmd/lint_test.go @@ -28,6 +28,7 @@ func TestRootCmd(t *testing.T) { assert.Contains(t, buf.String(), "generate") assert.Contains(t, buf.String(), "end-of-file-fixer") assert.Contains(t, buf.String(), "whitespace-fixer") + assert.Contains(t, buf.String(), "pr-size") } func TestLintCmdHelp(t *testing.T) { diff --git a/tools/githooks/cmd/pr_size.go b/tools/githooks/cmd/pr_size.go new file mode 100644 index 00000000000..e650eb4659b --- /dev/null +++ b/tools/githooks/cmd/pr_size.go @@ -0,0 +1,69 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" + + "github.com/smartcontractkit/chainlink/v2/tools/githooks/internal/prsize" +) + +func newPRSizeCmd() *cobra.Command { + var ( + strategy string + smallLimit int + mediumLimit int + failOnLarge bool + baseRef string + ignoreLockfiles bool + ignoreGenerated bool + includeUncommitted bool + ) + + cmd := &cobra.Command{ + Use: "pr-size", + Aliases: []string{"big-pr-guard", "pr-guard", "diff-guard", "diff-size"}, + Short: "Check PR diff size against default branch and guard against oversized PRs", + Long: "Calculates the git diff of the current branch against the default branch (the PR diff), " + + "classifies the diff as small, medium, or large, and prompts developers with recommendations " + + "to split large PRs into smaller, reviewable chunks.", + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + + repoRoot, err := findRepoRoot(ctx) + if err != nil { + return fmt.Errorf("could not find repo root: %w", err) + } + + cfg := prsize.Config{ + RepoRoot: repoRoot, + BaseRef: baseRef, + Strategy: prsize.Strategy(strategy), + SmallLimit: smallLimit, + MediumLimit: mediumLimit, + FailOnLarge: failOnLarge, + IgnoreLockfiles: ignoreLockfiles, + IgnoreGenerated: ignoreGenerated, + IncludeUncommitted: includeUncommitted, + Stdout: cmd.OutOrStdout(), + Stderr: cmd.ErrOrStderr(), + } + + return prsize.Run(ctx, cfg) + }, + } + + cmd.Flags().StringVar(&strategy, "strategy", string(prsize.StrategyPerFileMax), "Diff size strategy: per-file-max, sum, max, weighted") + cmd.Flags().IntVar(&smallLimit, "small-limit", prsize.DefaultSmallLimit, "Maximum effective lines for small classification") + cmd.Flags().IntVar(&smallLimit, "max-small", prsize.DefaultSmallLimit, "Alias for --small-limit") + cmd.Flags().IntVar(&mediumLimit, "medium-limit", prsize.DefaultMediumLimit, "Maximum effective lines for medium classification") + cmd.Flags().IntVar(&mediumLimit, "max-medium", prsize.DefaultMediumLimit, "Alias for --medium-limit") + cmd.Flags().BoolVar(&failOnLarge, "fail-on-large", false, "Exit with error code if PR is classified as large") + cmd.Flags().StringVar(&baseRef, "base", "", "Base branch or ref to diff against (default: origin default branch merge-base)") + cmd.Flags().StringVar(&baseRef, "rev", "", "Alias for --base") + cmd.Flags().BoolVar(&ignoreLockfiles, "ignore-lockfiles", true, "Ignore lockfiles (go.sum, package-lock.json, etc.) from diff line count") + cmd.Flags().BoolVar(&ignoreGenerated, "ignore-generated", true, "Ignore generated changes marked in .gitattributes (linguist-generated)") + cmd.Flags().BoolVar(&includeUncommitted, "include-uncommitted", false, "Include uncommitted staged and unstaged working-tree changes") + + return cmd +} diff --git a/tools/githooks/cmd/pr_size_test.go b/tools/githooks/cmd/pr_size_test.go new file mode 100644 index 00000000000..e8115d24d74 --- /dev/null +++ b/tools/githooks/cmd/pr_size_test.go @@ -0,0 +1,54 @@ +package cmd_test + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smartcontractkit/chainlink/v2/tools/githooks/cmd" +) + +func TestPRSizeCmdHelp(t *testing.T) { + t.Parallel() + + root := cmd.NewRootCmd() + buf := new(bytes.Buffer) + root.SetOut(buf) + root.SetErr(buf) + root.SetArgs([]string{"pr-size", "--help"}) + + err := root.Execute() + require.NoError(t, err) + + out := buf.String() + assert.Contains(t, buf.String(), "Calculates the git diff of the current branch against the default branch") + assert.Contains(t, out, "--strategy") + assert.Contains(t, out, "--small-limit") + assert.Contains(t, out, "--medium-limit") + assert.Contains(t, out, "--fail-on-large") + assert.Contains(t, out, "--base") + assert.Contains(t, out, "--ignore-lockfiles") + assert.Contains(t, out, "--ignore-generated") +} + +func TestPRSizeCmdAliases(t *testing.T) { + t.Parallel() + + aliases := []string{"big-pr-guard", "pr-guard", "diff-guard", "diff-size"} + for _, alias := range aliases { + t.Run(alias, func(t *testing.T) { + t.Parallel() + root := cmd.NewRootCmd() + buf := new(bytes.Buffer) + root.SetOut(buf) + root.SetErr(buf) + root.SetArgs([]string{alias, "--help"}) + + err := root.Execute() + require.NoError(t, err) + assert.Contains(t, buf.String(), "Calculates the git diff of the current branch against the default branch") + }) + } +} diff --git a/tools/githooks/cmd/root.go b/tools/githooks/cmd/root.go index 64ee605715c..ada3d836aad 100644 --- a/tools/githooks/cmd/root.go +++ b/tools/githooks/cmd/root.go @@ -24,6 +24,7 @@ func NewRootCmd() *cobra.Command { rootCmd.AddCommand(newGenerateCmd()) rootCmd.AddCommand(newEOFCmd()) rootCmd.AddCommand(newWhitespaceCmd()) + rootCmd.AddCommand(newPRSizeCmd()) return rootCmd } diff --git a/tools/githooks/internal/prsize/prsize.go b/tools/githooks/internal/prsize/prsize.go new file mode 100644 index 00000000000..fd4415e7fe6 --- /dev/null +++ b/tools/githooks/internal/prsize/prsize.go @@ -0,0 +1,404 @@ +package prsize + +import ( + "bytes" + "context" + "fmt" + "io" + "os/exec" + "path/filepath" + "strconv" + "strings" + + "github.com/smartcontractkit/chainlink/v2/tools/githooks/internal/modules" +) + +// Classification represents the size category of a PR diff. +type Classification string + +const ( + SizeSmall Classification = "SMALL" + SizeMedium Classification = "MEDIUM" + SizeLarge Classification = "LARGE" +) + +// Strategy defines how lines changed are calculated from additions and deletions. +type Strategy string + +const ( + StrategyPerFileMax Strategy = "per-file-max" + StrategySum Strategy = "sum" + StrategyMax Strategy = "max" + StrategyWeighted Strategy = "weighted" +) + +// Default limits for diff classification. +const ( + DefaultSmallLimit = 200 + DefaultMediumLimit = 500 +) + +var lockfileNames = map[string]bool{ + "go.sum": true, + "package-lock.json": true, + "pnpm-lock.yaml": true, + "yarn.lock": true, + "gemfile.lock": true, + "cargo.lock": true, + "poetry.lock": true, + "composer.lock": true, + "flake.lock": true, +} + +// FileStat contains diff statistics for a single file. +type FileStat struct { + Path string + Additions int + Deletions int + IsBinary bool +} + +// DiffStat summarizes the diff analysis results. +type DiffStat struct { + Files []FileStat + IgnoredFiles []string + FilesChanged int + Additions int + Deletions int + EffectiveLines int + MergeBase string + BaseRef string +} + +// Config controls PR size calculation and reporting behavior. +type Config struct { + RepoRoot string + BaseRef string + Strategy Strategy + SmallLimit int + MediumLimit int + FailOnLarge bool + IgnoreLockfiles bool + IgnoreGenerated bool + IgnoreGlobs []string + IncludeUncommitted bool + Stdout io.Writer + Stderr io.Writer +} + +func isLockfile(path string) bool { + base := strings.ToLower(filepath.Base(path)) + return lockfileNames[base] +} + +// CalculateEffectiveLines computes the effective lines changed based on the chosen strategy. +func CalculateEffectiveLines(files []FileStat, strategy Strategy) int { + if len(files) == 0 { + return 0 + } + + switch strategy { + case StrategySum: + total := 0 + for _, f := range files { + total += f.Additions + f.Deletions + } + return total + + case StrategyMax: + totalAdd := 0 + totalDel := 0 + for _, f := range files { + totalAdd += f.Additions + totalDel += f.Deletions + } + return max(totalAdd, totalDel) + + case StrategyWeighted: + totalAdd := 0 + totalDel := 0 + for _, f := range files { + totalAdd += f.Additions + totalDel += f.Deletions + } + return totalAdd + int(0.5*float64(totalDel)) + + case StrategyPerFileMax: + fallthrough + default: + total := 0 + for _, f := range files { + total += max(f.Additions, f.Deletions) + } + return total + } +} + +// Classify categorizes line count into Small, Medium, or Large. +func Classify(lines int, cfg Config) Classification { + smallLimit := cfg.SmallLimit + if smallLimit <= 0 { + smallLimit = DefaultSmallLimit + } + mediumLimit := cfg.MediumLimit + if mediumLimit <= 0 { + mediumLimit = DefaultMediumLimit + } + if smallLimit > mediumLimit { + smallLimit = mediumLimit + } + + if lines <= smallLimit { + return SizeSmall + } + if lines <= mediumLimit { + return SizeMedium + } + return SizeLarge +} + +func resolveMergeBase(ctx context.Context, repoRoot, baseRef string) (string, error) { + if baseRef != "" { + cmd := exec.CommandContext(ctx, "git", "merge-base", baseRef, "HEAD") + cmd.Dir = repoRoot + out, err := cmd.Output() + if err == nil { + if sha := strings.TrimSpace(string(out)); sha != "" { + return sha, nil + } + } + return baseRef, nil + } + + mb := modules.GetMergeBase(ctx, repoRoot) + return mb, nil +} + +func checkGeneratedFiles(ctx context.Context, repoRoot string, files []string) (map[string]bool, error) { + if len(files) == 0 { + return nil, nil + } + + cmd := exec.CommandContext(ctx, "git", "check-attr", "--stdin", "linguist-generated") + cmd.Dir = repoRoot + cmd.Stdin = strings.NewReader(strings.Join(files, "\n") + "\n") + out, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("git check-attr failed: %w", err) + } + + generatedMap := make(map[string]bool) + for line := range strings.SplitSeq(string(out), "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + continue + } + const prefix = ": linguist-generated: " + if filePath, val, found := strings.Cut(trimmed, prefix); found { + val = strings.TrimSpace(val) + if val == "true" || val == "set" || val == "1" { + generatedMap[strings.TrimSpace(filePath)] = true + } + } + } + return generatedMap, nil +} + +// Analyze inspects git diff against the merge-base with the default branch and calculates stats. +func Analyze(ctx context.Context, cfg Config) (*DiffStat, Classification, error) { + repoRoot := cfg.RepoRoot + if repoRoot == "" { + repoRoot = "." + } + + mergeBase, err := resolveMergeBase(ctx, repoRoot, cfg.BaseRef) + if err != nil { + return nil, "", fmt.Errorf("failed to resolve merge-base: %w", err) + } + + diffArgs := []string{"diff", "--numstat", mergeBase} + if !cfg.IncludeUncommitted { + diffArgs = append(diffArgs, "HEAD") + } + + cmd := exec.CommandContext(ctx, "git", diffArgs...) + cmd.Dir = repoRoot + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + if runErr := cmd.Run(); runErr != nil { + // If HEAD diff failed (e.g. initial commit or uncommitted only), fallback to diffing mergeBase + if cfg.IncludeUncommitted { + return nil, "", fmt.Errorf("git diff failed: %w (stderr: %s)", runErr, stderr.String()) + } + cmd = exec.CommandContext(ctx, "git", "diff", "--numstat", mergeBase) + cmd.Dir = repoRoot + stdout.Reset() + stderr.Reset() + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if fallbackErr := cmd.Run(); fallbackErr != nil { + return nil, "", fmt.Errorf("git diff failed: %w (stderr: %s)", fallbackErr, stderr.String()) + } + } + + type rawDiffEntry struct { + filePath string + addStr string + delStr string + } + + var rawEntries []rawDiffEntry + var allPaths []string + + for line := range strings.SplitSeq(stdout.String(), "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + continue + } + + parts := strings.Split(trimmed, "\t") + if len(parts) < 3 { + continue + } + + addStr, delStr, filePath := parts[0], parts[1], parts[2] + + // Handle renames formatted as "old => new" or "{dir => dir2}/file" + if idx := strings.LastIndex(filePath, " => "); idx != -1 { + filePath = strings.TrimSpace(filePath[idx+4:]) + filePath = strings.TrimSuffix(filePath, "}") + } + + rawEntries = append(rawEntries, rawDiffEntry{filePath: filePath, addStr: addStr, delStr: delStr}) + allPaths = append(allPaths, filePath) + } + + var generatedMap map[string]bool + if cfg.IgnoreGenerated && len(allPaths) > 0 { + generatedMap, err = checkGeneratedFiles(ctx, repoRoot, allPaths) + if err != nil { + return nil, "", err + } + } + + stat := &DiffStat{ + MergeBase: mergeBase, + BaseRef: cfg.BaseRef, + } + + for _, entry := range rawEntries { + filePath := entry.filePath + + // Check gitattributes linguist-generated ignore + if cfg.IgnoreGenerated && generatedMap[filePath] { + stat.IgnoredFiles = append(stat.IgnoredFiles, filePath) + continue + } + + // Check lockfile ignore + if cfg.IgnoreLockfiles && isLockfile(filePath) { + stat.IgnoredFiles = append(stat.IgnoredFiles, filePath) + continue + } + + // Check custom ignore globs + matchedGlob := false + for _, glob := range cfg.IgnoreGlobs { + if matched, _ := filepath.Match(glob, filePath); matched { + matchedGlob = true + break + } + } + if matchedGlob { + stat.IgnoredFiles = append(stat.IgnoredFiles, filePath) + continue + } + + fileStat := FileStat{Path: filePath} + if entry.addStr == "-" && entry.delStr == "-" { + fileStat.IsBinary = true + } else { + fileStat.Additions, _ = strconv.Atoi(entry.addStr) + fileStat.Deletions, _ = strconv.Atoi(entry.delStr) + } + + stat.Files = append(stat.Files, fileStat) + stat.FilesChanged++ + stat.Additions += fileStat.Additions + stat.Deletions += fileStat.Deletions + } + + diffStrategy := cfg.Strategy + if diffStrategy == "" { + diffStrategy = StrategyPerFileMax + } + stat.EffectiveLines = CalculateEffectiveLines(stat.Files, diffStrategy) + classification := Classify(stat.EffectiveLines, cfg) + + return stat, classification, nil +} + +// FormatReport generates the human-readable diff size summary and guidance. +func FormatReport(stat *DiffStat, class Classification, cfg Config) string { + var sb strings.Builder + + mediumLimit := cfg.MediumLimit + if mediumLimit <= 0 { + mediumLimit = DefaultMediumLimit + } + + diffStrategy := cfg.Strategy + if diffStrategy == "" { + diffStrategy = StrategyPerFileMax + } + + if class != SizeLarge { + fmt.Fprintf(&sb, "PR Diff Size: %d effective lines (+%d, -%d in %d files) [strategy: %s] -> Classification: %s [OK]\n", + stat.EffectiveLines, stat.Additions, stat.Deletions, stat.FilesChanged, diffStrategy, class) + if len(stat.IgnoredFiles) > 0 { + fmt.Fprintf(&sb, " (Excluded %d lock/ignored files)\n", len(stat.IgnoredFiles)) + } + return sb.String() + } + + // Large diff report & prompt + fmt.Fprintf(&sb, "⚠️ [pr-size] LARGE PR: %d effective lines (+%d/-%d across %d files) exceeds limit (%d)\n", + stat.EffectiveLines, stat.Additions, stat.Deletions, stat.FilesChanged, mediumLimit) + if len(stat.IgnoredFiles) > 0 { + fmt.Fprintf(&sb, " (Excluded %d lock/ignored files: %s)\n", len(stat.IgnoredFiles), strings.Join(stat.IgnoredFiles, ", ")) + } + sb.WriteString(" Please split into smaller, focused PRs or a GitHub PR stack.\n") + sb.WriteString(" 📖 PR Split Guide & Stack Recipes: tools/githooks/skills/split-pr/SKILL.md\n") + + return sb.String() +} + +// Run performs PR size analysis and reports results according to config. +func Run(ctx context.Context, cfg Config) error { + if cfg.Stdout == nil { + cfg.Stdout = io.Discard + } + if cfg.Stderr == nil { + cfg.Stderr = io.Discard + } + + stat, class, err := Analyze(ctx, cfg) + if err != nil { + return err + } + + report := FormatReport(stat, class, cfg) + if class == SizeLarge { + if cfg.FailOnLarge { + fmt.Fprint(cfg.Stderr, report) + return fmt.Errorf("PR size check failed: diff is classified as LARGE (%d effective lines > %d limit)", stat.EffectiveLines, cfg.MediumLimit) + } + fmt.Fprint(cfg.Stdout, report) + return nil + } + + fmt.Fprint(cfg.Stdout, report) + return nil +} diff --git a/tools/githooks/internal/prsize/prsize_bench_test.go b/tools/githooks/internal/prsize/prsize_bench_test.go new file mode 100644 index 00000000000..a98bfa799f8 --- /dev/null +++ b/tools/githooks/internal/prsize/prsize_bench_test.go @@ -0,0 +1,55 @@ +package prsize_test + +import ( + "testing" + + "github.com/smartcontractkit/chainlink/v2/tools/githooks/internal/prsize" +) + +func BenchmarkCalculateEffectiveLines(b *testing.B) { + files := make([]prsize.FileStat, 100) + for i := range files { + files[i] = prsize.FileStat{ + Path: "core/services/feature/file.go", + Additions: i * 2, + Deletions: i, + } + } + + strategies := []prsize.Strategy{ + prsize.StrategyPerFileMax, + prsize.StrategySum, + prsize.StrategyMax, + prsize.StrategyWeighted, + } + + for _, strategy := range strategies { + b.Run(string(strategy), func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + _ = prsize.CalculateEffectiveLines(files, strategy) + } + }) + } +} + +func BenchmarkFormatReport(b *testing.B) { + stat := &prsize.DiffStat{ + FilesChanged: 25, + Additions: 650, + Deletions: 120, + EffectiveLines: 700, + MergeBase: "0123456789abcdef", + IgnoredFiles: []string{"go.sum", "package-lock.json"}, + } + cfg := prsize.Config{ + SmallLimit: 200, + MediumLimit: 500, + Strategy: prsize.StrategyPerFileMax, + } + + b.ReportAllocs() + for b.Loop() { + _ = prsize.FormatReport(stat, prsize.SizeLarge, cfg) + } +} diff --git a/tools/githooks/internal/prsize/prsize_test.go b/tools/githooks/internal/prsize/prsize_test.go new file mode 100644 index 00000000000..335a10325ce --- /dev/null +++ b/tools/githooks/internal/prsize/prsize_test.go @@ -0,0 +1,343 @@ +package prsize_test + +import ( + "bytes" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smartcontractkit/chainlink/v2/tools/githooks/internal/prsize" +) + +// git runs a git command in dir and fails the test on error. +func git(t *testing.T, dir string, args ...string) string { + t.Helper() + + base := make([]string, 0, 6+len(args)) + base = append(base, "-c", "commit.gpgsign=false", "-c", "user.email=test@example.com", "-c", "user.name=test") + cmd := exec.CommandContext(t.Context(), "git", append(base, args...)...) //nolint:gosec // test helper + cmd.Dir = dir + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %s: %s", strings.Join(args, " "), out) + return strings.TrimSpace(string(out)) +} + +func writeFile(t *testing.T, dir, name, content string) { + t.Helper() + p := filepath.Join(dir, name) + require.NoError(t, os.MkdirAll(filepath.Dir(p), 0o700)) + require.NoError(t, os.WriteFile(p, []byte(content), 0o600)) +} + +func generateLines(n int, prefix string) string { + var sb strings.Builder + for i := 1; i <= n; i++ { + sb.WriteString(prefix) + sb.WriteString(" line\n") + } + return sb.String() +} + +func TestDiffCalculationStrategies(t *testing.T) { + t.Parallel() + + files := []prsize.FileStat{ + {Path: "core/services/app.go", Additions: 100, Deletions: 80}, + {Path: "core/logger/logger.go", Additions: 50, Deletions: 10}, + {Path: "README.md", Additions: 20, Deletions: 0}, + } + + tests := []struct { + strategy prsize.Strategy + expected int + }{ + { + strategy: prsize.StrategyPerFileMax, + // max(100, 80) + max(50, 10) + max(20, 0) = 100 + 50 + 20 = 170 + expected: 170, + }, + { + strategy: prsize.StrategySum, + // (100 + 80) + (50 + 10) + (20 + 0) = 180 + 60 + 20 = 260 + expected: 260, + }, + { + strategy: prsize.StrategyMax, + // total adds: 170, total dels: 90 -> max(170, 90) = 170 + expected: 170, + }, + { + strategy: prsize.StrategyWeighted, + // total adds: 170 + 0.5 * 90 = 215 + expected: 215, + }, + } + + for _, tc := range tests { + t.Run(string(tc.strategy), func(t *testing.T) { + t.Parallel() + got := prsize.CalculateEffectiveLines(files, tc.strategy) + assert.Equal(t, tc.expected, got) + }) + } +} + +func TestClassify(t *testing.T) { + t.Parallel() + + cfg := prsize.Config{ + SmallLimit: 200, + MediumLimit: 500, + } + + tests := []struct { + lines int + expected prsize.Classification + }{ + {lines: 0, expected: prsize.SizeSmall}, + {lines: 50, expected: prsize.SizeSmall}, + {lines: 200, expected: prsize.SizeSmall}, + {lines: 201, expected: prsize.SizeMedium}, + {lines: 450, expected: prsize.SizeMedium}, + {lines: 500, expected: prsize.SizeMedium}, + {lines: 501, expected: prsize.SizeLarge}, + {lines: 1200, expected: prsize.SizeLarge}, + } + + for _, tc := range tests { + got := prsize.Classify(tc.lines, cfg) + assert.Equal(t, tc.expected, got, "lines=%d", tc.lines) + } +} + +func TestAnalyze_GitRepo(t *testing.T) { + t.Parallel() + + t.Run("classifies small diff correctly", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + git(t, dir, "init") + writeFile(t, dir, "base.go", generateLines(10, "base")) + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "initial commit") + baseSHA := git(t, dir, "rev-parse", "HEAD") + + git(t, dir, "update-ref", "refs/remotes/origin/develop", baseSHA) + git(t, dir, "symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/develop") + + writeFile(t, dir, "feature.go", generateLines(50, "feature")) + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "small feature") + + cfg := prsize.Config{ + RepoRoot: dir, + Strategy: prsize.StrategyPerFileMax, + SmallLimit: 200, + MediumLimit: 500, + IgnoreLockfiles: true, + } + + stat, class, err := prsize.Analyze(t.Context(), cfg) + require.NoError(t, err) + assert.Equal(t, prsize.SizeSmall, class) + assert.Equal(t, 50, stat.EffectiveLines) + assert.Equal(t, 50, stat.Additions) + assert.Equal(t, 0, stat.Deletions) + assert.Equal(t, 1, stat.FilesChanged) + }) + + t.Run("classifies medium diff correctly", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + git(t, dir, "init") + writeFile(t, dir, "base.go", generateLines(10, "base")) + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "initial commit") + baseSHA := git(t, dir, "rev-parse", "HEAD") + + git(t, dir, "update-ref", "refs/remotes/origin/develop", baseSHA) + git(t, dir, "symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/develop") + + writeFile(t, dir, "feature.go", generateLines(350, "feature")) + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "medium feature") + + cfg := prsize.Config{ + RepoRoot: dir, + Strategy: prsize.StrategyPerFileMax, + SmallLimit: 200, + MediumLimit: 500, + IgnoreLockfiles: true, + } + + stat, class, err := prsize.Analyze(t.Context(), cfg) + require.NoError(t, err) + assert.Equal(t, prsize.SizeMedium, class) + assert.Equal(t, 350, stat.EffectiveLines) + assert.Equal(t, 1, stat.FilesChanged) + }) + + t.Run("classifies large diff correctly and respects lockfile ignore", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + git(t, dir, "init") + writeFile(t, dir, "base.go", generateLines(10, "base")) + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "initial commit") + baseSHA := git(t, dir, "rev-parse", "HEAD") + + git(t, dir, "update-ref", "refs/remotes/origin/develop", baseSHA) + git(t, dir, "symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/develop") + + // Add 100 lines of code and 1000 lines of go.sum lockfile + writeFile(t, dir, "feature.go", generateLines(100, "feature")) + writeFile(t, dir, "go.sum", generateLines(1000, "checksum")) + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "add feature and deps") + + cfgWithIgnore := prsize.Config{ + RepoRoot: dir, + Strategy: prsize.StrategyPerFileMax, + SmallLimit: 200, + MediumLimit: 500, + IgnoreLockfiles: true, + } + + stat, class, err := prsize.Analyze(t.Context(), cfgWithIgnore) + require.NoError(t, err) + assert.Equal(t, prsize.SizeSmall, class) + assert.Equal(t, 100, stat.EffectiveLines) + assert.Len(t, stat.IgnoredFiles, 1) + + cfgWithoutIgnore := prsize.Config{ + RepoRoot: dir, + Strategy: prsize.StrategyPerFileMax, + SmallLimit: 200, + MediumLimit: 500, + IgnoreLockfiles: false, + } + + stat2, class2, err2 := prsize.Analyze(t.Context(), cfgWithoutIgnore) + require.NoError(t, err2) + assert.Equal(t, prsize.SizeLarge, class2) + assert.Equal(t, 1100, stat2.EffectiveLines) + assert.Empty(t, stat2.IgnoredFiles) + }) + + t.Run("ignores generated changes defined in .gitattributes", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + git(t, dir, "init") + writeFile(t, dir, ".gitattributes", "*.generated.go linguist-generated=true\ngen/** linguist-generated\n") + writeFile(t, dir, "base.go", generateLines(10, "base")) + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "initial commit") + baseSHA := git(t, dir, "rev-parse", "HEAD") + + git(t, dir, "update-ref", "refs/remotes/origin/develop", baseSHA) + git(t, dir, "symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/develop") + + writeFile(t, dir, "regular.go", generateLines(50, "regular")) + writeFile(t, dir, "service.generated.go", generateLines(800, "generated")) + writeFile(t, dir, "gen/docs.txt", generateLines(400, "docs")) + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "feature and generated code") + + cfgWithGeneratedIgnore := prsize.Config{ + RepoRoot: dir, + Strategy: prsize.StrategyPerFileMax, + SmallLimit: 200, + MediumLimit: 500, + IgnoreGenerated: true, + } + + stat, class, err := prsize.Analyze(t.Context(), cfgWithGeneratedIgnore) + require.NoError(t, err) + assert.Equal(t, prsize.SizeSmall, class) + assert.Equal(t, 50, stat.EffectiveLines) + assert.ElementsMatch(t, []string{"gen/docs.txt", "service.generated.go"}, stat.IgnoredFiles) + + cfgWithoutGeneratedIgnore := prsize.Config{ + RepoRoot: dir, + Strategy: prsize.StrategyPerFileMax, + SmallLimit: 200, + MediumLimit: 500, + IgnoreGenerated: false, + } + + stat2, class2, err2 := prsize.Analyze(t.Context(), cfgWithoutGeneratedIgnore) + require.NoError(t, err2) + assert.Equal(t, prsize.SizeLarge, class2) + assert.Equal(t, 1250, stat2.EffectiveLines) + assert.Empty(t, stat2.IgnoredFiles) + }) +} + +func TestRun(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + git(t, dir, "init") + writeFile(t, dir, "base.go", generateLines(10, "base")) + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "initial commit") + baseSHA := git(t, dir, "rev-parse", "HEAD") + + git(t, dir, "update-ref", "refs/remotes/origin/develop", baseSHA) + git(t, dir, "symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/develop") + + // Commit 600 lines -> Large + writeFile(t, dir, "huge.go", generateLines(600, "huge")) + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "large commit") + + t.Run("warns on large diff when FailOnLarge is false", func(t *testing.T) { + t.Parallel() + var stdout, stderr bytes.Buffer + cfg := prsize.Config{ + RepoRoot: dir, + Strategy: prsize.StrategyPerFileMax, + SmallLimit: 200, + MediumLimit: 500, + FailOnLarge: false, + IgnoreLockfiles: true, + Stdout: &stdout, + Stderr: &stderr, + } + + err := prsize.Run(t.Context(), cfg) + require.NoError(t, err) + + out := stdout.String() + stderr.String() + assert.Contains(t, out, "LARGE PR") + assert.Contains(t, out, "tools/githooks/skills/split-pr/SKILL.md") + }) + + t.Run("fails on large diff when FailOnLarge is true", func(t *testing.T) { + t.Parallel() + var stdout, stderr bytes.Buffer + cfg := prsize.Config{ + RepoRoot: dir, + Strategy: prsize.StrategyPerFileMax, + SmallLimit: 200, + MediumLimit: 500, + FailOnLarge: true, + IgnoreLockfiles: true, + Stdout: &stdout, + Stderr: &stderr, + } + + err := prsize.Run(t.Context(), cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "PR size check failed") + + out := stdout.String() + stderr.String() + assert.Contains(t, out, "LARGE PR") + assert.Contains(t, out, "tools/githooks/skills/split-pr/SKILL.md") + }) +} diff --git a/tools/githooks/skills/split-pr/SKILL.md b/tools/githooks/skills/split-pr/SKILL.md new file mode 100644 index 00000000000..338dfcda11e --- /dev/null +++ b/tools/githooks/skills/split-pr/SKILL.md @@ -0,0 +1,149 @@ +--- +name: split-pr +description: Context, strategies, and step-by-step workflows for an agent or developer to break a large PR/branch into smaller, reviewable chunks and GitHub stacked pull requests. +--- + +# Splitting Large PRs & GitHub Stacked Pull Requests + +Use this skill when a Git branch or pull request is classified as **LARGE** (>500 effective lines) or when changes span multiple layers, modules, or concerns. + +## Why PRs Must Be Small + +1. **Review Speed & Quality**: Reviewers review diffs $\le 200$ lines in minutes with high scrutiny. Diff sizes $> 500$ lines experience $3\times$ higher review latency and higher defect escape rates. +2. **CI Isolation**: In the `chainlink` monorepo, smaller package-targeted diffs trigger focused test shards (`tools/githooks test` and CI shards) rather than entire module test cascades. +3. **Merge Safety**: Smaller PRs minimize merge conflict windows and make rollbacks trivial. + +--- + +## PR Splitting Strategies + +Choose the strategy that matches the nature of the branch: + +### Strategy 1: Stacked Layers (Recommended for Features) +Split by architectural dependency order from foundational to high-level: +1. **Layer 1 (Bottom)**: Interfaces, proto definitions, database schemas, core types, shared models. +2. **Layer 2 (Middle)**: Core service logic, handlers, internal adapters. +3. **Layer 3 (Top)**: API routes, CLI commands, documentation, UI/integration tests. + +```text + ┌── feat/part3-api-and-cli → PR #3 (base: feat/part2-service-logic) ← Top + ┌── feat/part2-service-logic → PR #2 (base: feat/part1-interfaces-types) + ┌── feat/part1-interfaces-types → PR #1 (base: develop) ← Bottom +develop (default branch) +``` + +### Strategy 2: By Go Module / Package (Recommended for Refactors & Sweeps) +If changes touch multiple Go modules (e.g. root `.`, `deployment`, `integration-tests`, `tools/`): +- PR 1: Module A changes (`./deployment/...`) +- PR 2: Module B changes (`./core/...`) +- PR 3: Shared tooling / config updates + +### Strategy 3: Refactor / Cleanup First, Feature Second +Separate mechanical refactoring (renaming, reformatting, signature changes) from behavioral additions: +- PR 1: Pure refactor (existing behavior preserved, tests green). +- PR 2: New feature logic built on top of refactored code. + +### Strategy 4: Generated Files & Dependencies +Put massive generated files (mocks from mockery, protobuf generated `.pb.go`, config docs) into their own stacked PR layer after the generator input files. + +--- + +## How to Create GitHub Stacked PRs Fast + +GitHub supports stacked pull requests natively. Each PR targets the branch below it. + +### Step 1: Identify the Commits or File Sets +List commits on the current large branch: +```bash +git log --oneline origin/develop..HEAD +``` + +### Step 2: Create Stacked Branches + +#### Option A: Using Git Cherry-Pick (Commit-Based) +```bash +# 1. Branch 1: Base layer from default branch +git checkout -b /layer-1-types origin/develop +git cherry-pick +git push -u origin /layer-1-types + +# 2. Branch 2: Next layer stacked ON TOP of layer 1 +git checkout -b /layer-2-logic /layer-1-types +git cherry-pick +git push -u origin /layer-2-logic + +# 3. Branch 3: Top layer stacked ON TOP of layer 2 +git checkout -b /layer-3-cli /layer-2-logic +git cherry-pick +git push -u origin /layer-3-cli +``` + +#### Option B: Using Mixed Reset (File-Set Based) +If commits are messy or everything is uncommitted: +```bash +# Save branch reference +git branch backup-large-branch + +# Reset to merge-base +git reset $(git merge-base origin/develop HEAD) + +# Layer 1: Stage foundational files +git checkout -b /layer-1-types origin/develop +git add core/types/ schema/ +git commit -m "feat(types): foundational interfaces and types" +git push -u origin /layer-1-types + +# Layer 2: Stage core logic +git checkout -b /layer-2-logic /layer-1-types +git add core/services/ +git commit -m "feat(services): implement core logic" +git push -u origin /layer-2-logic +``` + +--- + +### Step 3: Open PRs Targeting Previous Layers + +Use GitHub CLI (`gh`) to open the pull requests with the correct base: + +```bash +# PR 1 targets develop +gh pr create --base develop --head /layer-1-types \ + --title "[] Part 1/2: Interfaces & Types" \ + --body "Stacked PR 1 of 2. Foundational types." + +# PR 2 targets layer-1-types +gh pr create --base /layer-1-types --head /layer-2-logic \ + --title "[] Part 2/2: Service Logic" \ + --body "Stacked PR 2 of 2. Builds on top of #." +``` + +*(Note: When PR #1 merges into `develop`, GitHub automatically re-targets PR #2's base branch to `develop` and cascades rebases.)* + +--- + +## Rebasing a Stacked PR Chain + +When code in a lower layer changes (e.g. PR review feedback on Layer 1): +```bash +# 1. Update Layer 1 +git checkout /layer-1-types +# make edits... +git commit --amend # or git commit +git push origin /layer-1-types --force-with-lease + +# 2. Rebase Layer 2 onto updated Layer 1 +git checkout /layer-2-logic +git rebase /layer-1-types +git push origin /layer-2-logic --force-with-lease +``` + +--- + +## Agent Verification Checklist + +When splitting a PR: +- [ ] Each layer compiles independently (`go build ./...`). +- [ ] Unit tests pass in each layer (`tools/githooks test`). +- [ ] PR size guard classifies each individual layer as **SMALL** or **MEDIUM** (`tools/githooks pr-size`). +- [ ] The PR description references the stack order (e.g. "Part 1 of 3: base for #..."). From 0f3be581f408b51357c5c6f5617b03d6d4563a98 Mon Sep 17 00:00:00 2001 From: Adam Hamrick Date: Thu, 27 Aug 2026 17:09:36 -0400 Subject: [PATCH 2/3] chore(githooks): improve skill --- tools/githooks/skills/split-pr/SKILL.md | 187 +++++++++--------------- 1 file changed, 70 insertions(+), 117 deletions(-) diff --git a/tools/githooks/skills/split-pr/SKILL.md b/tools/githooks/skills/split-pr/SKILL.md index 338dfcda11e..19f53ba8149 100644 --- a/tools/githooks/skills/split-pr/SKILL.md +++ b/tools/githooks/skills/split-pr/SKILL.md @@ -1,149 +1,102 @@ --- name: split-pr -description: Context, strategies, and step-by-step workflows for an agent or developer to break a large PR/branch into smaller, reviewable chunks and GitHub stacked pull requests. +description: Break large git branches into small, reviewable pull requests. --- -# Splitting Large PRs & GitHub Stacked Pull Requests +# Split PR & Stacked PRs -Use this skill when a Git branch or pull request is classified as **LARGE** (>500 effective lines) or when changes span multiple layers, modules, or concerns. +Use when branch is **LARGE** (>500 lines), touches multiple modules, or mixes refactoring with feature code. -## Why PRs Must Be Small +## 0. Rules -1. **Review Speed & Quality**: Reviewers review diffs $\le 200$ lines in minutes with high scrutiny. Diff sizes $> 500$ lines experience $3\times$ higher review latency and higher defect escape rates. -2. **CI Isolation**: In the `chainlink` monorepo, smaller package-targeted diffs trigger focused test shards (`tools/githooks test` and CI shards) rather than entire module test cascades. -3. **Merge Safety**: Smaller PRs minimize merge conflict windows and make rollbacks trivial. +- Commits must be signed with a touch-only GPG key. You must ask the user to make the commit, or prompt the user to tap their key. +- Generated code doesn't count towards layer size; generated code must be fully up to date. ---- - -## PR Splitting Strategies +## 1. Splitting Heuristics -Choose the strategy that matches the nature of the branch: - -### Strategy 1: Stacked Layers (Recommended for Features) -Split by architectural dependency order from foundational to high-level: -1. **Layer 1 (Bottom)**: Interfaces, proto definitions, database schemas, core types, shared models. -2. **Layer 2 (Middle)**: Core service logic, handlers, internal adapters. -3. **Layer 3 (Top)**: API routes, CLI commands, documentation, UI/integration tests. +Order layers bottom (trunk-adjacent) to top: +- **Layered Architecture**: Interfaces/types/schemas (bottom) -> Core service logic -> APIs/CLI/routes (top). +- **Module / Package**: Slice by Go module (`deployment/`, `core/`, `tools/`). +- **Refactor vs Feature**: Pure non-breaking refactor first -> new behavior on top. ```text - ┌── feat/part3-api-and-cli → PR #3 (base: feat/part2-service-logic) ← Top - ┌── feat/part2-service-logic → PR #2 (base: feat/part1-interfaces-types) - ┌── feat/part1-interfaces-types → PR #1 (base: develop) ← Bottom -develop (default branch) -``` - -### Strategy 2: By Go Module / Package (Recommended for Refactors & Sweeps) -If changes touch multiple Go modules (e.g. root `.`, `deployment`, `integration-tests`, `tools/`): -- PR 1: Module A changes (`./deployment/...`) -- PR 2: Module B changes (`./core/...`) -- PR 3: Shared tooling / config updates - -### Strategy 3: Refactor / Cleanup First, Feature Second -Separate mechanical refactoring (renaming, reformatting, signature changes) from behavioral additions: -- PR 1: Pure refactor (existing behavior preserved, tests green). -- PR 2: New feature logic built on top of refactored code. - -### Strategy 4: Generated Files & Dependencies -Put massive generated files (mocks from mockery, protobuf generated `.pb.go`, config docs) into their own stacked PR layer after the generator input files. - ---- - -## How to Create GitHub Stacked PRs Fast - -GitHub supports stacked pull requests natively. Each PR targets the branch below it. - -### Step 1: Identify the Commits or File Sets -List commits on the current large branch: -```bash -git log --oneline origin/develop..HEAD +develop (trunk) <- layer-1-types <- layer-2-logic <- layer-3-cli (top) ``` -### Step 2: Create Stacked Branches +## 2. Break Existing Branch into Stacked Layers -#### Option A: Using Git Cherry-Pick (Commit-Based) +### Option A: Reset & Stage by File Set (Messy/Uncommitted History) ```bash -# 1. Branch 1: Base layer from default branch -git checkout -b /layer-1-types origin/develop -git cherry-pick -git push -u origin /layer-1-types - -# 2. Branch 2: Next layer stacked ON TOP of layer 1 -git checkout -b /layer-2-logic /layer-1-types -git cherry-pick -git push -u origin /layer-2-logic - -# 3. Branch 3: Top layer stacked ON TOP of layer 2 -git checkout -b /layer-3-cli /layer-2-logic -git cherry-pick -git push -u origin /layer-3-cli -``` - -#### Option B: Using Mixed Reset (File-Set Based) -If commits are messy or everything is uncommitted: -```bash -# Save branch reference -git branch backup-large-branch +# 1. Backup current branch +git branch backup-feature-branch -# Reset to merge-base +# 2. Reset merge-base without losing changes git reset $(git merge-base origin/develop HEAD) -# Layer 1: Stage foundational files -git checkout -b /layer-1-types origin/develop -git add core/types/ schema/ -git commit -m "feat(types): foundational interfaces and types" -git push -u origin /layer-1-types +# 3. Layer 1 (Bottom): Types/Interfaces +git checkout -b /1-types origin/develop +git add path/to/types/ path/to/schemas/ +git commit -m "feat(types): foundational interfaces" -# Layer 2: Stage core logic -git checkout -b /layer-2-logic /layer-1-types -git add core/services/ +# 4. Layer 2: Core Logic +git checkout -b /2-logic /1-types +git add path/to/services/ git commit -m "feat(services): implement core logic" -git push -u origin /layer-2-logic -``` - ---- - -### Step 3: Open PRs Targeting Previous Layers -Use GitHub CLI (`gh`) to open the pull requests with the correct base: +# 5. Layer 3 (Top): APIs / CLI +git checkout -b /3-cli /2-logic +git add path/to/cli/ +git commit -m "feat(cli): endpoints and wiring" +``` +### Option B: Cherry-Pick Commits (Clean Commit History) ```bash -# PR 1 targets develop -gh pr create --base develop --head /layer-1-types \ - --title "[] Part 1/2: Interfaces & Types" \ - --body "Stacked PR 1 of 2. Foundational types." - -# PR 2 targets layer-1-types -gh pr create --base /layer-1-types --head /layer-2-logic \ - --title "[] Part 2/2: Service Logic" \ - --body "Stacked PR 2 of 2. Builds on top of #." +git checkout -b /1-types origin/develop && git cherry-pick +git checkout -b /2-logic /1-types && git cherry-pick +git checkout -b /3-cli /2-logic && git cherry-pick ``` -*(Note: When PR #1 merges into `develop`, GitHub automatically re-targets PR #2's base branch to `develop` and cascades rebases.)* - ---- +## 3. Submit & Manage Stack with `gh stack` -## Rebasing a Stacked PR Chain +Always use non-interactive flags to prevent agent hangs: -When code in a lower layer changes (e.g. PR review feedback on Layer 1): ```bash -# 1. Update Layer 1 -git checkout /layer-1-types -# make edits... -git commit --amend # or git commit -git push origin /layer-1-types --force-with-lease - -# 2. Rebase Layer 2 onto updated Layer 1 -git checkout /layer-2-logic -git rebase /layer-1-types -git push origin /layer-2-logic --force-with-lease +# Initialize stack from existing branches (bottom to top) +gh stack init /1-types /2-logic /3-cli + +# Submit/update PRs non-interactively (--auto avoids interactive editor) +gh stack submit --auto + +# View stack state (JSON prevents TUI freeze) +gh stack view --json + +# Navigate stack (never use interactive `gh stack switch`) +gh stack bottom +gh stack up +gh stack down +gh stack top + +# Edit lower layer & propagate changes upstack +gh stack checkout /1-types +# ...make edits... +git commit --amend --no-edit +gh stack rebase --upstack +gh stack submit --auto ``` ---- +### Fallback: Native `gh pr create` +If `gh stack` is unavailable: +```bash +gh pr create --base develop --head /1-types --title "[] Part 1/3: Types" --body "Base layer." +gh pr create --base /1-types --head /2-logic --title "[] Part 2/3: Logic" --body "Stacked on #." +gh pr create --base /2-logic --head /3-cli --title "[] Part 3/3: CLI" --body "Stacked on #." +``` -## Agent Verification Checklist +## 4. Verification Checklist -When splitting a PR: -- [ ] Each layer compiles independently (`go build ./...`). -- [ ] Unit tests pass in each layer (`tools/githooks test`). -- [ ] PR size guard classifies each individual layer as **SMALL** or **MEDIUM** (`tools/githooks pr-size`). -- [ ] The PR description references the stack order (e.g. "Part 1 of 3: base for #..."). +Before PR submission: +- [ ] Generated code up to date: `make rm-mocked & make generate` +- [ ] Each layer compiles and passes lints independently: `tools/githooks lint` +- [ ] Unit tests pass per layer: `tools/githooks test` +- [ ] Layer size is **SMALL** or **MEDIUM**: `tools/githooks pr-size` +- [ ] Check that all commits are signed with a GPG key: `git log --show-signature`. If not, prompt the user to sign them. From 5b7e06b5c53959a8dbadfe73c178f38427f1b7c7 Mon Sep 17 00:00:00 2001 From: Adam Hamrick Date: Thu, 27 Aug 2026 17:16:53 -0400 Subject: [PATCH 3/3] chore(githooks): improve skill --- tools/githooks/internal/prsize/prsize.go | 4 ++-- tools/githooks/skills/split-pr/SKILL.md | 12 ++++-------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/tools/githooks/internal/prsize/prsize.go b/tools/githooks/internal/prsize/prsize.go index fd4415e7fe6..be4881d2a0c 100644 --- a/tools/githooks/internal/prsize/prsize.go +++ b/tools/githooks/internal/prsize/prsize.go @@ -369,8 +369,8 @@ func FormatReport(stat *DiffStat, class Classification, cfg Config) string { if len(stat.IgnoredFiles) > 0 { fmt.Fprintf(&sb, " (Excluded %d lock/ignored files: %s)\n", len(stat.IgnoredFiles), strings.Join(stat.IgnoredFiles, ", ")) } - sb.WriteString(" Please split into smaller, focused PRs or a GitHub PR stack.\n") - sb.WriteString(" 📖 PR Split Guide & Stack Recipes: tools/githooks/skills/split-pr/SKILL.md\n") + sb.WriteString(" Split into smaller, focused PRs or a GitHub PR stack.\n") + sb.WriteString(" AI skill to split PR: tools/githooks/skills/split-pr/SKILL.md\n") return sb.String() } diff --git a/tools/githooks/skills/split-pr/SKILL.md b/tools/githooks/skills/split-pr/SKILL.md index 19f53ba8149..435ed8c4653 100644 --- a/tools/githooks/skills/split-pr/SKILL.md +++ b/tools/githooks/skills/split-pr/SKILL.md @@ -11,6 +11,7 @@ Use when branch is **LARGE** (>500 lines), touches multiple modules, or mixes re - Commits must be signed with a touch-only GPG key. You must ask the user to make the commit, or prompt the user to tap their key. - Generated code doesn't count towards layer size; generated code must be fully up to date. +- If `gh` or `gh stack` is unavailable, STOP! Prompt the user to install them before continuing. ## 1. Splitting Heuristics @@ -27,6 +28,9 @@ develop (trunk) <- layer-1-types <- layer-2-logic <- layer-3-cli (top) ### Option A: Reset & Stage by File Set (Messy/Uncommitted History) ```bash +# 0. Help for stack CLI +gh stack --help + # 1. Backup current branch git branch backup-feature-branch @@ -84,14 +88,6 @@ gh stack rebase --upstack gh stack submit --auto ``` -### Fallback: Native `gh pr create` -If `gh stack` is unavailable: -```bash -gh pr create --base develop --head /1-types --title "[] Part 1/3: Types" --body "Base layer." -gh pr create --base /1-types --head /2-logic --title "[] Part 2/3: Logic" --body "Stacked on #." -gh pr create --base /2-logic --head /3-cli --title "[] Part 3/3: CLI" --body "Stacked on #." -``` - ## 4. Verification Checklist Before PR submission: