Repository navigation
Add pull request size check - #12974
PerfectSlayer wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 893558f10e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| id: count-added-lines | ||
| run: | | ||
| # The first parent of the pull request merge commit is the base branch | ||
| ADDED_LINES=$(.github/scripts/count-added-lines.sh HEAD^1 HEAD) |
There was a problem hiding this comment.
On any PR that modifies .github/scripts/count-added-lines.sh, the default pull_request checkout supplies the PR merge commit, so this line executes attacker-controlled shell code while the job has id-token: write. GitHub exposes OIDC request credentials to jobs with that permission (OIDC reference); the script can therefore exfiltrate a JWT accepted by the new STS policy and obtain its issues: write/pull_requests: write token. Even without stealing credentials, replacing the script with echo 0 bypasses the size limit. Run a counter sourced from the trusted base revision, ideally before or separately from the OIDC-enabled job.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
| filter: blob:none | ||
| sparse-checkout: .github/scripts |
There was a problem hiding this comment.
On every workflow run, filter: blob:none causes this pinned version of actions/checkout to ignore the following sparse-checkout input—the action explicitly documents that filter overrides sparse checkout. Consequently the checkout materializes the repository's entire roughly 12,000-file, 111 MiB snapshot instead of only .github/scripts, adding avoidable I/O to a check triggered for every PR update and label change. Remove the conflicting filter or configure partial and sparse checkout manually so the intended path restriction is honored.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
The workflow executes PR-controlled shell code with OIDC access capable of obtaining repository-wide issue and pull-request write permissions. Its comment lookup can also target contributor comments instead of the bot’s warning.
🤖 Bits Code Review · Commit 893558f
| id: count-added-lines | ||
| run: | | ||
| # The first parent of the pull request merge commit is the base branch | ||
| ADDED_LINES=$(.github/scripts/count-added-lines.sh HEAD^1 HEAD) |
There was a problem hiding this comment.
A PR modifying count-added-lines.sh executes author-controlled shell code in a job with id-token: write. That code can request an OIDC token and exchange it under the new policy, which accepts PR merge refs and grants repository-wide issue and pull-request write permissions. Disabling persisted checkout credentials does not restrict OIDC access. Separate untrusted counting from credentialed commenting and bind token issuance to a trusted workflow revision.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| owner: context.repo.owner, | ||
| repo: context.repo.repo | ||
| }) | ||
| const existingComment = comments.find(comment => comment.body?.includes(managedMarker)) |
There was a problem hiding this comment.
If a contributor quotes a warning containing the managed marker, this lookup can select their comment instead of the bot's. Subsequent runs target it for overwriting when oversized or deletion when below the limit or overridden, leaving the actual warning untouched. Focused mocked execution confirmed all three selection paths. Require the expected issuing app's identity together with the marker before modifying a comment.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
| ## PR conventions | ||
|
|
||
| - Title: imperative verb sentence describing user-visible change (e.g. "Fix span sampling rule parsing") | ||
| - Size: keep PRs small (under 1000 added lines), split larger changes into stacked PRs. |
There was a problem hiding this comment.
note: I suggest to explicitly write that agents are not allowed to add this tag. I.e. Only human judgment should be involved in this tag!
There was a problem hiding this comment.
Do you think so? I would be fine having a bot adding the label on mechanical changes for example.
It's still up to the reviewer to asses the labels, like the groovy enforcement override.
Having the label added by a bot or a human without rationale help justifying a request for change review.
What Does This Do
This PR adds a pull request size check to prevent large change sets and difficult code reviews.
It promotes split and stacked PRs instead, which are a very nice way to with review.
Coding agents now how to deal with them so it should not be an issue for both humans and agent authors.
Motivation
When change set is too big, it becomes very hard for reviewers to verify the content.
Additionally, the automatic agentic review only raise 3 to 5 errors. So for a change of 3k LOC, it's about only one feedback per 600 to 1000 LOC. Limit to 1k LOC would also bring more automatic review comments (about 1 comment per 200 LOC at most).
Additional Notes
Line moves, deletions, version lock files does not count toward the change set size limit.
And there is an exceptional
tag: override pr size checklabel to unblock PR if needed.The check will run first as a non blocking workflow for evaluation.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]