harden pr-size reusable: batch check-attr, --source probe, git timeouts, comment-token scope#51
Merged
Merged
Conversation
…alls Ports two robustness fixes onto the reusable's check-pr-size tool. Both were raised as follow-ups on the original implementation in Comfy-Org/cloud (cloud#5163 / BE-3247 and cloud#5164 / BE-3248) and belong here, in the shared tool, so every consuming repo inherits them instead of each carrying a fork. BE-3247 — distinguish an unsupported `check-attr --source` from unrelated probe failures. checkAttrSourceSupported previously treated ANY probe error as "flag unsupported", so a transient error or an unreadable base ref on a modern git would wrongly disable every base linguist-generated exclusion and print a misleading "your git is too old" note. It now inspects git's stderr and only treats git's own unknown-option/unknown-switch wording as unsupported; the capture forces LC_ALL=C so the match sees canonical English. Fail direction stays conservative (over-count, never under-count). BE-3248 — resolve linguist-generated for all paths in ONE `git check-attr -z --stdin` pass (attrGeneratedBatch) instead of one subprocess per file, and put a 60s deadline on every git invocation (runGitStdin + runGitCapped via exec.CommandContext) so a wedged git can't stall the job until the runner-wide timeout. The two cloud PRs each rewrote runGit independently; here they are unified into a single runGitStdin (stdin + timeout + bounded stderr capture + C locale), and the deadline is extended to the reusable's runGitCapped blob-read path too. Base-only attribute reading (BE-3220) is preserved unchanged. Tests: TestAttrGeneratedBatchMultiplePaths, TestIsUnknownFlagError, TestCheckAttrSourceSupported, TestCapWriter; TestAttrGeneratedSourceIsolatesBase updated to the batch API. gofmt/vet/build/test all clean (git 2.50.1, --source exercised). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR hardens git subprocess execution with timeouts and capped stderr, batches linguist-generated attribute checks, expands coverage for these paths, and makes the PR-size workflow tolerate bot-token failures and ambient Go workspace files. ChangesPR-size robustness
Sequence Diagram(s)sequenceDiagram
participant classify
participant attrGeneratedBatch
participant runGitStdin
participant git
classify->>attrGeneratedBatch: provide non-binary paths
attrGeneratedBatch->>runGitStdin: send NUL-separated paths
runGitStdin->>git: run git check-attr --stdin
git-->>runGitStdin: return attributes and diagnostics
runGitStdin-->>attrGeneratedBatch: return command result
attrGeneratedBatch-->>classify: return generated-path map
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
…-effort The comment job minted the bot token requesting both issues:write and pull-requests:write, but the sticky-comment upsert only ever calls the issues comments API (GET/POST/PATCH /issues/.../comments). On an installation not granted pull-requests:write — e.g. Comfy-Org/cloud's Cloud Code Bot — the mint returns 422 "permissions requested are not granted to this installation" and the comment job fails, turning a check red purely on the best-effort comment path even though the size verdict (in the pr-size job) is fine. - Request only permission-issues: write — exactly what the upsert uses. - Mark the mint continue-on-error so a consumer whose bot lacks even that scope (or has creds misconfigured) degrades to status + step summary instead of a red check. The size verdict never depended on this job. - Broaden the degraded-mode note to fire on a failed mint too, not just absent creds, so the log always explains why no comment was posted. Surfaced by cloud#5335 (the migration of Comfy-Org/cloud onto this reusable). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mattmillerai
changed the title
…sumers The tool is checked out into the consumer's working tree (_pr_size_tool/) and built with `go build`. When the consumer is a go.work monorepo — e.g. Comfy-Org/cloud — `go build` walks up, discovers the consumer's go.work, and fails: "current directory is contained in a module that is not one of the workspace modules listed in go.work." The tool's module is not (and should not be) part of the consumer's workspace. Set GOWORK=off on the build step so the ambient workspace is ignored and the tool builds as its own standalone module (it has its own go.mod). No effect on non-workspace consumers. Caught by cloud#5335: the reusable's first go.work-based consumer. Without this the pr-size job never even evaluates size — it dies at the build step. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mattmillerai
marked this pull request as ready for review
July 21, 2026 18:37
mattmillerai
added
cursor-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 4 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 2 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/check-pr-size/main_test.go (1)
82-227: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a regression test for the git timeout path
scripts/check-pr-size/main_test.gocovers batching, probe detection, and base isolation, but not thegitTimeout/runGitStdindeadline behavior. Add one test that forces a hung git call and asserts the timeout error so this guard doesn’t go feral in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-pr-size/main_test.go` around lines 82 - 227, Add a regression test in scripts/check-pr-size/main_test.go that exercises the gitTimeout/runGitStdin deadline path by forcing a git invocation to hang, then assert it returns the expected timeout error. Keep the test deterministic and bounded so it reliably detects deadline enforcement without risking a lingering process or extended CI runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-size.yml:
- Around line 317-321: Update the degraded-mode reporting step around steps.dl
and steps.bot so it also runs when steps.dl.outcome != 'success'. Emit a
distinct note for download failures, while preserving the existing
credentials/token failure message when the download succeeds but
steps.bot.outcome is not successful.
---
Outside diff comments:
In `@scripts/check-pr-size/main_test.go`:
- Around line 82-227: Add a regression test in
scripts/check-pr-size/main_test.go that exercises the gitTimeout/runGitStdin
deadline path by forcing a git invocation to hang, then assert it returns the
expected timeout error. Keep the test deterministic and bounded so it reliably
detects deadline enforcement without risking a lingering process or extended CI
runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aa5b8c2b-4f21-424a-908f-2ecca5b5b70d
📒 Files selected for processing (3)
.github/workflows/pr-size.ymlscripts/check-pr-size/main.goscripts/check-pr-size/main_test.go
…iagnostics Address cursor-review panel findings on the git helper refactor: - Set cmd.WaitDelay (Go 1.20+) on runGitStdin and runGitCapped so a git descendant that inherited the stdout/stderr pipes (filter driver, textconv, pager, credential helper) can't hold them open and block Wait() past the SIGKILL deadline, keeping the BE-3248 timeout robust (Medium). - runGit now folds runGitStdin's captured stderr into its returned error. runGitStdin sets cmd.Stderr, so *exec.ExitError no longer carries git's stderr; diffFiles and other callers were losing git's diagnostics (Low). - Wrap runGitCapped's deadline hit as %w of ctx.Err() for parity with runGitStdin so errors.Is(err, context.DeadlineExceeded) works (Nit). - Add TestRunGitFoldsStderrIntoError regression guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The comment job's "Note degraded mode" step only fired when the report download succeeded but the bot token was absent/unmintable. When actions/download-artifact itself failed (continue-on-error), every downstream step — including this note — was skipped, so the job went green with no explanation of why no comment was posted. Broaden the condition to also cover steps.dl.outcome != 'success' and emit a distinct message pointing at the pr-size job logs (CodeRabbit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 1 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…false A bare `off` is a YAML boolean; GitHub Actions stringifies it into the env as "false", and `go build` with GOWORK=false (neither off, empty, nor an absolute path) aborts — breaking the build for every consumer, including the go.work monorepo case this env was added to fix. Quote it as "off". Flagged by cursor-review panel (2/8 reviewers, High).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardens the reusable PR-size cap so every consumer inherits the improvements from one place. Three tool fixes + one workflow fix, all surfaced while migrating
Comfy-Org/cloudonto this reusable (cloud#5335).Tool:
scripts/check-pr-size/(Go)Ports two follow-ups that were open against cloud's now-deleted local copy — they belong here, in the shared tool:
BE-3247 (was cloud#5163) —
checkAttrSourceSupportedpreviously treated any probe error as "flag unsupported," so a transient error or unreadable base ref on a modern git would drop every baselinguist-generatedexclusion and print a misleading "your git is too old" note. It now inspects git stderr and only treats git's ownunknown option/unknown switchwording as unsupported (LC_ALL=Cforces canonical English). Fail direction stays conservative: any other failure keeps--sourcetrusted and the read fails closed → over-count, never under.BE-3248 (was cloud#5164) —
attrGenerated(one subprocess per file) becomesattrGeneratedBatch: a singlegit check-attr -z --stdinpass. Every git call now runs under a 60scontext.WithTimeout(runGitStdin+runGitCapped), so a wedged git can't stall the job.The two cloud PRs each rewrote
runGit; here they're unified into onerunGitStdin(stdin + timeout + bounded stderr capture + C locale), and the deadline extends to the reusable'srunGitCappedblob path too. Base-only reading (BE-3220) preserved.Workflow:
pr-size.ymlcomment-token scopeThe comment job minted the bot token requesting
issues:writeandpull-requests:write, but the sticky-comment upsert only calls the issues comments API. On an installation not grantedpull-requests:write(cloud's Cloud Code Bot), the mint 422s and the comment job fails — a red check from the best-effort comment path alone. Now:permission-issues: write(exactly what's used);continue-on-erroron the mint so a consumer whose bot lacks the scope degrades to status + step summary, never a red check;Testing
gofmt -l,go vet,go build,go test ./...,actionlint pr-size.ymlall clean locally on git 2.50.1 (--sourceexercised, not skipped). New/updated tests:TestAttrGeneratedBatchMultiplePaths,TestIsUnknownFlagError,TestCheckAttrSourceSupported,TestCapWriter,TestAttrGeneratedSourceIsolatesBase(→ batch API).After this merges
Re-pin cloud#5335's
uses:+workflows_refto the resulting SHA, then close cloud#5163 / cloud#5164.🤖 Generated with Claude Code