harden pr-size reusable: batch check-attr, --source probe, git timeouts, comment-token scope

.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

scripts/check-pr-size/main.go

@@ -1,13 +1,16 @@
 package main
 
 import (
+	"bytes"
+	"context"
 	"flag"
 	"fmt"
 	"io"
 	"os"
 	"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: "<path>: linguist-generated: <value>". 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 <ref>:<path>`) 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