diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index f2587a7..bc2cbf2 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -181,6 +181,17 @@ jobs: cache: false - name: Build check-pr-size + env: + # The tool is checked out INTO the consumer's working tree (_pr_size_tool/), + # so if that consumer is a go.work monorepo (e.g. Comfy-Org/cloud), a plain + # `go build` walks up, finds the consumer's go.work, and fails with + # "current directory is contained in a module that is not one of the + # workspace modules". GOWORK=off ignores any ambient workspace and builds + # the tool as its own standalone module (it has its own go.mod). + # Quote "off": a bare `off` is a YAML boolean, which Actions stringifies + # to "false" in the env — and `GOWORK=false` (neither off, empty, nor an + # absolute path) makes `go build` abort. The quotes keep it the literal off. + GOWORK: "off" run: | # Drop anything the PR checkout could have pre-seeded under the tool # path (such files are untracked in the tool repo, which checkout @@ -258,14 +269,21 @@ jobs: - name: Mint bot token if: steps.dl.outcome == 'success' && env.BOT_CONFIGURED == 'true' id: bot + # Best-effort: the size verdict lives in the pr-size job, and the report + # is always in that job's step summary. A consumer whose bot app lacks + # the requested scope (or has creds misconfigured) must degrade to + # status + summary, never get a red check from the comment path — so a + # failed mint is tolerated here rather than failing the job. + continue-on-error: true uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 with: app-id: ${{ inputs.bot_app_id }} private-key: ${{ secrets.BOT_APP_PRIVATE_KEY }} - # Scope the minted token to comment upserts only, not the app's full - # installation permissions (PR comments ride the issues API). + # Scope the token to exactly what the upsert uses: the sticky comment + # rides the issues comments API, so issues:write is the only permission + # required. Requesting more (e.g. pull-requests:write) makes the mint + # 422 on an installation not granted it, for no gain. permission-issues: write - permission-pull-requests: write - name: Upsert sticky size comment if: steps.bot.outcome == 'success' @@ -299,5 +317,15 @@ jobs: fi - name: Note degraded mode - if: steps.dl.outcome == 'success' && steps.bot.outcome == 'skipped' - run: echo "bot_app_id/BOT_APP_PRIVATE_KEY not configured — no PR comment; the report is in the pr-size job's step summary." + # Covers every path where no comment is posted so the job never goes + # green silently: "download failed" (dl errored under continue-on-error, + # e.g. the size tool crashed before uploading the report), "creds absent" + # (bot step skipped) and "mint failed" (bot step errored). Whenever the + # report artifact exists it still lives in the pr-size job's step summary. + if: steps.dl.outcome != 'success' || steps.bot.outcome != 'success' + run: | + if [ "${{ steps.dl.outcome }}" != "success" ]; then + echo "No bot comment posted: the size report artifact could not be downloaded (the pr-size job likely failed before uploading it). Check the pr-size job logs." + else + echo "No bot comment posted (credentials absent, or the app token could not be minted for this repo). The full report is in the pr-size job's step summary." + fi diff --git a/scripts/check-pr-size/check-pr-size b/scripts/check-pr-size/check-pr-size new file mode 100755 index 0000000..998fb92 Binary files /dev/null and b/scripts/check-pr-size/check-pr-size differ diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 60d69e2..753a814 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -1,6 +1,8 @@ package main import ( + "bytes" + "context" "flag" "fmt" "io" @@ -8,6 +10,7 @@ import ( "os/exec" "strconv" "strings" + "time" ) const ( @@ -21,6 +24,11 @@ const ( // marker (which sits at the top). It caps memory/time so a PR cannot point a // "generated" path at an unbounded or blocking target and hang the job. maxScanBytes = 4 << 20 // 4 MiB + // maxStderrBytes caps captured git stderr. cmd.Output() installs a 32 KiB + // prefixSuffixSaver on stderr only when it is nil; runGitStdin sets its own + // stderr writer, so it re-adds an equivalent cap to keep a git command that + // floods stderr from buffering unbounded memory. + maxStderrBytes = 32 << 10 // 32 KiB ) func main() { @@ -59,7 +67,7 @@ func main() { // shrink its own counted LoC. Two defense-in-depth conditions disable the // attribute path entirely (unless the bypass label is present): git too old // to honor `check-attr --source`, or the PR diff touching .gitattributes at - // all. See attrGenerated / TouchesGitattributes. + // all. See attrGeneratedBatch / TouchesGitattributes. attr := attrPolicy{source: *base, useSource: checkAttrSourceSupported(*base)} attrModified := TouchesGitattributes(files) attr.trusted = attrTrusted(attr.useSource, attrModified, *bypassFlag) @@ -132,52 +140,115 @@ func attrTrusted(useSource, attrModified, bypass bool) bool { // lockfile name lists (built-in + extras), the caller-supplied generated globs, // the linguist-generated git attribute (read from the base ref per attr), and // the canonical Go generated marker in the file's content. +// +// The linguist-generated attribute is resolved for every non-binary path in a +// SINGLE `git check-attr` pass (attrGeneratedBatch) rather than one subprocess +// per file, so a large PR pays constant process-creation cost instead of O(N). func classify(files []FileChange, base, head string, attr attrPolicy, extras Extras) { + var attrGen map[string]bool + if attr.trusted { + paths := make([]string, 0, len(files)) + for i := range files { + if !files[i].Binary { + paths = append(paths, files[i].Path) + } + } + attrGen = attrGeneratedBatch(paths, attr.source, attr.useSource) + } for i := range files { f := &files[i] if f.Binary { continue } if IsLockfile(f.Path) || extras.Generated(f.Path) || - (attr.trusted && attrGenerated(f.Path, attr.source, attr.useSource)) || + attrGen[f.Path] || contentGenerated(f.Path, base, head) { f.Generated = true } } } -// attrGenerated reports whether .gitattributes marks the path linguist-generated. -// When useSource is set, the attribute rules are read from source (the base ref) -// via `git check-attr --source`, so .gitattributes rules introduced by the PR -// head are never consulted — a PR cannot mark arbitrary hand-written files -// generated to escape the size count. `--source` needs git >= 2.40 (GitHub -// runners ship newer); on older git useSource is false and the attribute path is -// left untrusted (see attrPolicy) rather than reading the PR checkout. -func attrGenerated(path, source string, useSource bool) bool { - args := []string{"check-attr", "linguist-generated"} +// attrGeneratedBatch reports, per path, whether .gitattributes marks it +// linguist-generated, resolving every path in ONE `git check-attr` pass (paths +// fed on stdin) instead of one subprocess per file. The returned map contains an +// entry only for paths git reported as generated ("true" for an explicit "=true" +// or "set" for a bare attribute); callers treat an absent path as not-generated, +// matching the old err→false fallback. +// +// When useSource is set the rules are read from source (the base ref) via +// `--source`, so .gitattributes rules introduced by the PR head are never +// consulted — a PR cannot mark arbitrary hand-written files generated to escape +// the size count. `--source` needs git >= 2.40 (GitHub runners ship newer); on +// older git useSource is false and `--source` is omitted (the caller only +// invokes this on a runner where the attribute path is trusted; see attrPolicy). +func attrGeneratedBatch(paths []string, source string, useSource bool) map[string]bool { + result := make(map[string]bool, len(paths)) + if len(paths) == 0 { + return result + } + // `-z` uses NUL as the separator for both stdin paths and output fields, so a + // path containing spaces/newlines (already possible from the -z numstat that + // produced these) round-trips safely. + args := []string{"check-attr", "-z"} if useSource { args = append(args, "--source", source) } - args = append(args, "--", path) - out, err := runGit(args...) + args = append(args, "--stdin", "linguist-generated") + + var stdin bytes.Buffer + for _, p := range paths { + stdin.WriteString(p) + stdin.WriteByte(0) + } + out, _, err := runGitStdin(stdin.Bytes(), args...) if err != nil { - return false + return result + } + // `-z` output is a flat NUL-separated stream of (path, attribute, value) + // triples, with a trailing NUL after the final value (so Split yields an + // empty tail element the triple loop ignores). + fields := strings.Split(out, "\x00") + for i := 0; i+2 < len(fields); i += 3 { + if v := fields[i+2]; v == "true" || v == "set" { + result[fields[i]] = true + } } - // Format: ": linguist-generated: ". git reports "true" for an - // explicit "=true" and "set" for a bare attribute; both mean generated. - s := strings.TrimSpace(out) - return strings.HasSuffix(s, ": true") || strings.HasSuffix(s, ": set") + return result } // checkAttrSourceSupported reports whether the installed git honors // `check-attr --source` (added in git 2.40). Older git rejects the flag as an // unknown option and exits non-zero, so we probe once rather than parse the -// version string. A false result forces the attribute path onto its untrusted -// fallback (see attrPolicy). The probe path need not exist; check-attr resolves -// rules for arbitrary path strings. +// version string. The probe path need not exist; check-attr resolves rules for +// arbitrary path strings. +// +// Only git's own "unknown flag" signature counts as unsupported. Any OTHER probe +// failure (a transient git error, an unreadable base ref) leaves --source +// trusted: reporting a modern-but-flaky git as "too old" would silently drop +// every base linguist-generated exclusion and emit a misleading "runner's git is +// too old" note (BE-3247). On such an unrelated failure the reads stay +// conservative anyway — attrGeneratedBatch's own --source call fails closed, so +// files are over-counted (never under-counted), and the size cap can only be too +// strict, never too loose. func checkAttrSourceSupported(source string) bool { - _, err := runGit("check-attr", "--source", source, "linguist-generated", "--", ".gitattributes") - return err == nil + _, stderr, err := runGitStdin(nil, "check-attr", "--source", source, "linguist-generated", "--", ".gitattributes") + if err == nil { + return true + } + // A genuine unknown-flag rejection => --source truly unsupported. Anything + // else is an unrelated failure and must not disable the base-ref attribute path. + return !isUnknownFlagError(stderr) +} + +// isUnknownFlagError reports whether git's stderr indicates it rejected an option +// as unrecognized. git's parse-options prints "unknown option" for long flags and +// "unknown switch" for short ones; matching is case-insensitive and substring-based +// so it tolerates the surrounding usage text and minor wording drift across git +// versions. The stderr comes from runGitStdin, which forces LC_ALL=C, so this +// only ever sees git's canonical English wording (BE-3247). +func isUnknownFlagError(stderr string) bool { + s := strings.ToLower(stderr) + return strings.Contains(s, "unknown option") || strings.Contains(s, "unknown switch") } // contentGenerated reports whether a .go file carries Go's canonical generated @@ -301,22 +372,101 @@ func writeGitHubOutputs(res Result) { fmt.Fprintf(f, "over_cap=%t\ncounted=%d\n", !res.OK, res.Counted) } +// gitTimeout bounds every git invocation so a hung git (a wedged credential +// prompt, a stuck pack read, an unresponsive filter) cannot stall the job until +// the global CI runner timeout. It is deliberately generous — the heaviest call +// here is a single blob read — so a healthy git never trips it. +const gitTimeout = 60 * time.Second + +// gitWaitDelay bounds how long Wait() may block after the timeout fires and the +// child is killed. exec.CommandContext only SIGKILLs the immediate git process; +// a descendant that inherited the stdout/stderr pipes (a diff/filter driver, +// textconv, pager, or credential helper) can hold them open, so the copy +// goroutines behind cmd.Output()/StdoutPipe never see EOF and Wait() would block +// past the deadline. WaitDelay (Go 1.20+) forces those pipes closed and Wait to +// return after the grace period, keeping the BE-3248 timeout robust. +const gitWaitDelay = 10 * time.Second + func runGit(args ...string) (string, error) { - cmd := exec.Command("git", args...) - out, err := cmd.Output() + out, stderr, err := runGitStdin(nil, args...) + // runGitStdin sets cmd.Stderr, so a returned *exec.ExitError no longer + // carries git's stderr (Cmd.Output only populates ExitError.Stderr when + // Stderr is nil). Fold the captured stderr back into the error so callers + // like diffFiles keep git's diagnostics. if err != nil { - return "", err + if s := strings.TrimSpace(stderr); s != "" { + err = fmt.Errorf("%w: %s", err, s) + } } - return string(out), nil + return out, err } +// runGitStdin runs git with the given args and optional stdin, returning stdout +// and stderr separately. Three behaviors matter to callers: +// - stderr is captured (a plain cmd.Output() discards it) and bounded to +// maxStderrBytes, so checkAttrSourceSupported can tell an old git's +// unknown-flag rejection from an unrelated failure (BE-3247). +// - the call runs under a gitTimeout deadline via exec.CommandContext; when it +// fires the child is killed and the timeout surfaces as an error, which +// callers already treat as "git failed" rather than returning stale data +// (BE-3248). +// - stdin, when non-nil, is fed to git — used for `check-attr --stdin`. +// +// git is forced into the C locale so its parse-options diagnostics (localized +// via gettext under LANG/LC_MESSAGES) stay in the canonical English wording +// isUnknownFlagError matches; the porcelain output we parse (-z numstat, -z +// check-attr, blob contents) is locale-independent. +func runGitStdin(stdin []byte, args ...string) (stdout, stderr string, err error) { + ctx, cancel := context.WithTimeout(context.Background(), gitTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, "git", args...) + cmd.WaitDelay = gitWaitDelay + cmd.Env = append(os.Environ(), "LC_ALL=C") + if stdin != nil { + cmd.Stdin = bytes.NewReader(stdin) + } + errBuf := &capWriter{cap: maxStderrBytes} + cmd.Stderr = errBuf + out, err := cmd.Output() + if err != nil && ctx.Err() == context.DeadlineExceeded { + err = fmt.Errorf("git %v timed out after %s: %w", args, gitTimeout, ctx.Err()) + } + return string(out), errBuf.String(), err +} + +// capWriter is an io.Writer that retains at most cap bytes, silently discarding +// the overflow while still reporting a full write so the child process never +// sees a short-write error. It re-adds the stderr cap that cmd.Output() would +// otherwise provide (see maxStderrBytes). +type capWriter struct { + buf bytes.Buffer + cap int +} + +func (w *capWriter) Write(p []byte) (int, error) { + if room := w.cap - w.buf.Len(); room > 0 { + if len(p) > room { + w.buf.Write(p[:room]) + } else { + w.buf.Write(p) + } + } + return len(p), nil +} + +func (w *capWriter) String() string { return w.buf.String() } + // runGitCapped runs git and reads at most maxBytes of its stdout, killing the // process rather than blocking on Wait() if it had more to write. Used for // reads of ref-addressed blobs (e.g. `git show :`) whose size we // don't control, so a single oversized blob can't spike job memory the way an -// unbounded cmd.Output() would. +// unbounded cmd.Output() would. Bounded by gitTimeout like every other git call +// (BE-3248), so a stuck blob read can't hang the job. func runGitCapped(maxBytes int64, args ...string) ([]byte, error) { - cmd := exec.Command("git", args...) + ctx, cancel := context.WithTimeout(context.Background(), gitTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, "git", args...) + cmd.WaitDelay = gitWaitDelay stdout, err := cmd.StdoutPipe() if err != nil { return nil, err @@ -340,6 +490,11 @@ func runGitCapped(maxBytes int64, args ...string) ([]byte, error) { return data, nil } if err := cmd.Wait(); err != nil { + // Wrap a timeout hit as %w of ctx.Err() so errors.Is(err, + // context.DeadlineExceeded) works, mirroring runGitStdin (BE-3248). + if ctx.Err() == context.DeadlineExceeded { + err = fmt.Errorf("git %v timed out after %s: %w", args, gitTimeout, ctx.Err()) + } return nil, err } return data, nil diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index 9f847ab..fb195bd 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -79,11 +79,11 @@ func TestTouchesGitattributes(t *testing.T) { } } -// TestAttrGeneratedSourceIsolatesBase proves attrGenerated reads .gitattributes -// from the base tree (useSource=true), not the working tree (the PR head). It -// checks both directions: a base rule the head removed is still honored via -// --source, and a rule only the head adds is ignored via --source but WOULD -// have been honored reading the working tree. +// TestAttrGeneratedSourceIsolatesBase proves attrGeneratedBatch reads +// .gitattributes from the base tree (useSource=true), not the working tree (the +// PR head). It checks both directions: a base rule the head removed is still +// honored via --source, and a rule only the head adds is ignored via --source +// but WOULD have been honored reading the working tree. func TestAttrGeneratedSourceIsolatesBase(t *testing.T) { // Direction 1: base HAS the rule, head REMOVED it. t.Run("base rule honored despite head removal", func(t *testing.T) { @@ -98,12 +98,12 @@ func TestAttrGeneratedSourceIsolatesBase(t *testing.T) { if !checkAttrSourceSupported(base) { t.Skip("git too old for check-attr --source") } - if !attrGenerated("foo.go", base, true) { - t.Error("attrGenerated should read the base rule via --source") + if !attrGeneratedBatch([]string{"foo.go"}, base, true)["foo.go"] { + t.Error("attrGeneratedBatch should read the base rule via --source") } // Reading the working tree (head) must NOT see the removed rule. - if attrGenerated("foo.go", base, false) { - t.Error("attrGenerated without --source read the head tree, expected no rule") + if attrGeneratedBatch([]string{"foo.go"}, base, false)["foo.go"] { + t.Error("attrGeneratedBatch without --source read the head tree, expected no rule") } }) @@ -120,15 +120,112 @@ func TestAttrGeneratedSourceIsolatesBase(t *testing.T) { if !checkAttrSourceSupported(base) { t.Skip("git too old for check-attr --source") } - if attrGenerated("foo.go", base, true) { - t.Error("attrGenerated via --source must not see the head-introduced rule") + if attrGeneratedBatch([]string{"foo.go"}, base, true)["foo.go"] { + t.Error("attrGeneratedBatch via --source must not see the head-introduced rule") } - if !attrGenerated("foo.go", base, false) { + if !attrGeneratedBatch([]string{"foo.go"}, base, false)["foo.go"] { t.Error("sanity: reading the working tree should see the head rule (the vulnerability)") } }) } +// TestAttrGeneratedBatchMultiplePaths proves the single-pass batch resolves each +// path independently against the base ref: a matched path is reported generated, +// an unmatched one is absent from the map, and an empty input never shells out. +func TestAttrGeneratedBatchMultiplePaths(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, ".gitattributes", "gen/*.go linguist-generated=true\n") + writeFile(t, dir, "gen/a.go", "package gen\n") + writeFile(t, dir, "gen/b.go", "package gen\n") + writeFile(t, dir, "hand.go", "package main\n") + base := commitAll(t, dir, "base with rule") + t.Chdir(dir) + + if !checkAttrSourceSupported(base) { + t.Skip("git too old for check-attr --source") + } + got := attrGeneratedBatch([]string{"gen/a.go", "gen/b.go", "hand.go"}, base, true) + if !got["gen/a.go"] || !got["gen/b.go"] { + t.Errorf("matched paths should be generated, got %v", got) + } + if got["hand.go"] { + t.Errorf("unmatched path should not be generated, got %v", got) + } + if len(attrGeneratedBatch(nil, base, true)) != 0 { + t.Error("empty input should return an empty map") + } +} + +// TestIsUnknownFlagError checks that only git's unrecognized-option wording is +// treated as the "unsupported --source flag" signal, and unrelated git errors +// (e.g. a bad ref) are not — the core BE-3247 distinction. +func TestIsUnknownFlagError(t *testing.T) { + t.Parallel() + tests := []struct { + name string + stderr string + want bool + }{ + {"long option wording", "error: unknown option `source'\nusage: git check-attr ...", true}, + {"short switch wording", "error: unknown switch `s'\n", true}, + {"mixed case tolerated", "ERROR: Unknown Option `source'", true}, + {"unrelated fatal is not a flag error", "fatal: no-such-ref: not a valid tree-ish source\n", false}, + {"empty stderr", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := isUnknownFlagError(tt.stderr); got != tt.want { + t.Errorf("isUnknownFlagError(%q) = %v, want %v", tt.stderr, got, tt.want) + } + }) + } +} + +// TestCheckAttrSourceSupported proves the BE-3247 fix end to end against real +// git: a valid probe reports supported, and a probe that fails for a reason +// OTHER than an unknown --source flag (here an unresolvable base ref) is NOT +// misreported as unsupported — which would otherwise drop legit base exclusions +// and emit the misleading "git is too old" note. +func TestCheckAttrSourceSupported(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, "foo.go", "package foo\n") + commitAll(t, dir, "base") + t.Chdir(dir) + + if !checkAttrSourceSupported("HEAD") { + t.Skip("git too old for check-attr --source") + } + // An unresolvable source ref makes git fatal with "not a valid tree-ish + // source" (not an unknown-flag error), so --source must stay trusted. + if !checkAttrSourceSupported("this-ref-does-not-exist") { + t.Error("checkAttrSourceSupported must stay true when the probe fails for a reason other than an unknown --source flag") + } +} + +// TestCapWriter checks that captured git stderr is bounded: writes past the cap +// are dropped (never buffered), while each Write still reports its full length so +// the child process never observes a short write. +func TestCapWriter(t *testing.T) { + t.Parallel() + w := &capWriter{cap: 4} + if n, err := w.Write([]byte("ab")); n != 2 || err != nil { + t.Fatalf("Write(ab) = (%d, %v), want (2, nil)", n, err) + } + // Straddles the cap: only "cd" fits, but the writer must report all 4 bytes + // written so exec does not treat it as an error. + if n, err := w.Write([]byte("cdef")); n != 4 || err != nil { + t.Fatalf("Write(cdef) = (%d, %v), want (4, nil)", n, err) + } + // Fully past the cap: dropped entirely, still reported as fully written. + if n, err := w.Write([]byte("gh")); n != 2 || err != nil { + t.Fatalf("Write(gh) = (%d, %v), want (2, nil)", n, err) + } + if got := w.String(); got != "abcd" { + t.Errorf("capWriter retained %q, want %q", got, "abcd") + } +} + // TestClassifyPRAddedGitattributesDoesNotReduceCount is the anti-gaming // regression: a PR that adds `*.go linguist-generated=true` to .gitattributes // must not shrink the counted lines of its hand-written .go changes. @@ -395,3 +492,21 @@ func TestRunGitCappedLimitsBlobRead(t *testing.T) { t.Errorf("len(data) = %d, want 100 (capped read)", len(data)) } } + +// TestRunGitFoldsStderrIntoError guards the refactor regression: runGit +// delegates to runGitStdin, which sets cmd.Stderr, so a returned *exec.ExitError +// no longer carries git's stderr. runGit must fold the captured stderr back into +// its error so callers (e.g. diffFiles) keep git's diagnostics. +func TestRunGitFoldsStderrIntoError(t *testing.T) { + dir := initTestRepo(t) + t.Chdir(dir) + + // A bogus revision makes git exit non-zero and write a diagnostic to stderr. + _, err := runGit("show", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef:nope") + if err == nil { + t.Fatal("runGit on a bogus revision should return an error") + } + if !strings.Contains(strings.ToLower(err.Error()), "fatal") { + t.Errorf("runGit error should carry git's stderr diagnostic, got: %v", err) + } +}