ci(ci-scripts): post pr size as a pinned comment - #219
Merged
Merged
Conversation
`pr-describe` tallied the diff by hand with awk and tokei, then pasted the table into the PR body, where it went stale on the next push. A two-job workflow now recomputes it on every push and keeps one pinned comment. `Comment` holds the write token and never checks out PR code: GitHub's PR files API serves the per-file additions/deletions the table needs. `Measure` checks the PR out for tokei but holds no write token, and hands over three integers through a job output. Verified against the old pipeline over a 229-file range across all four buckets: byte-identical table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- `continue-on-error` on both jobs, so a failed download or merge-base cannot redden a PR over a cosmetic comment - `!cancelled()` instead of `always()`, so a run superseded by `cancel-in-progress` cannot post the new head's file counts beside the old head's tokei totals - handle `edited`, guarded on a base change, so retargeting a stacked PR recomputes instead of leaving the former base's table pinned - `xargs ... tokei --`: a root file named `--output` was read as an option and silently yielded an all-zero measurement (reproduced, then fixed) - stop overclaiming in comments: `pull_request` runs the workflow as the PR defines it, and binaries count at +0/−0 where the old awk dropped them Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Concurrency resolves at workflow-run level, before any job `if:`. A title or body edit therefore entered the group, cancelled a recompute still in flight, then skipped both of its own jobs — leaving the comment stale or absent. `cancel-in-progress` now carries the same condition as the job guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard that ran only on a base change could not be made race free. A run that will go on to skip still enters the concurrency group first and displaces a pending real one, because GitHub resolves concurrency at workflow-run level and replaces a queued run regardless of `cancel-in-progress`. Dropping the guard deletes both duplicated job conditions and the conditional `cancel-in-progress`, and costs about a minute of CI on a title or body edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Size
Changed files at head: 451 code, 344 comments, 157 blanks. |
Running the orchestrator against PR #219 under a PAT posted a second comment instead of editing the first: the author filter looked for `type === 'Bot'` and a user token is not one. CI always runs as `github-actions[bot]`, so this never fires in production — but it is the same duplicate-on-every-push failure the filter was supposed to prevent, just triggered by a different identity. The marker is enough. Dropping the author check removes the only way the lookup can miss, at the cost of a contrived case where someone prefixes their own comment with the marker and has it overwritten. Also records the bootstrap property found on #219: `Comment` runs main's ci-scripts, so the job fails with "no script" on the PR that adds it, while `continue-on-error` keeps the PR green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tokei has shipped no prebuilt binary since v12.1.2 in 2021, so pinning one only gets further out of date. ubuntu-latest already carries a Rust toolchain, and `cargo install tokei --locked` costs about a minute on a runner — measured at 18s wall / 83s CPU locally. Only the binary is cached, so restore is near instant and a stale entry just rebuilds. Now on v15.0.0. Verified v12.1.2, v14.0.0 and v15.0.0 return identical code, comment and blank totals on the same inputs, and that `--` and empty input still behave. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A cache entry is immutable at its key, so the previous comment's claim that a corrupt one "simply rebuilds" was wrong: it would have cost the code/prose split on every push until someone noticed and bumped the key. The restored binary is now probed and rebuilt on failure. Also moves the smoke check ahead of the save, so a bad build is never cached, and spells out `success()` in both conditions — an explicit `if:` drops the implicit one, which would otherwise let a failed build be saved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
haydenshively
marked this pull request as ready for review
September 15, 2026 04:13
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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be18d093ef
ℹ️ 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".
`set -e` was inherited from the runner's default shell (`bash -e {0}`) rather
than stated. Verified both ways: under `bash -e` a failed merge-base exits 128
before the echo, under plain bash it publishes zeroes that `Comment` cannot
tell from a real empty diff. Spelled out so the property is local.
Also drops one-time rollout history from the header, per the repo rule that
comments state a guarantee or a hazard and leave provenance to git.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![dev-carapulse[bot]](https://avatars.githubusercontent.com/in/3630305?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approving the CI-only scope at ae86126. I found no new blocking issue in the reviewed diff and surrounding helpers.
Validation performed with Node 24.14.1 / pnpm 11.1.1:
pnpm --filter @repo/ci-scripts exec vitest run: 94 tests passed, including the 31 PR-size tests.pnpm --filter @repo/ci-scripts run typecheck: passed.- Exercised the actual comment orchestrator with intercepted HTTP responses: creation, paginated lookup/update, malformed measurement fallback, missing PR payload, and API-error handling passed. These were offline checks, not live comment-write tests.
- Reconciled the current PR files API against the local three-dot diff, then checked that the existing size-comment table matches the current renderer.
- Read the workflow, default-branch setup path, existing review replies, and the upstream v15.0.0 Total-row renderer.
CI at review time: Lint, Typecheck, Dead-Code, and Measure succeeded; the full Test job is still running. Comment failed at Post size comment with exit code 1. The checked default-branch package does not yet define pr-size-comment, consistent with the documented bootstrap limitation; this review does not treat the writer as CI-verified.
Acknowledged deferred scope: BOTS-138 (workflow trust boundary) and BOTS-139 (base-only updates can leave the comment stale), both read in Linear and left as follow-ups rather than expanding this PR. The private-repository fork-policy probe returned HTTP 422 because this repository is public; it supplied no additional policy evidence.
After merge, complete the stated real-CI create/update-in-place smoke check on a subsequent PR, and let the full Test job finish before merging. No repository changes or merge performed.
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.
pr-describemade the agent measure the diff by hand — agit diff --numstat+ awk pipeline plustokei— and paste the result into the PR body under## Size. That number was wrong the moment another commit landed, and nothing corrected it. CI now owns the measurement and keeps one pinned comment per PR, recomputed on every push.Changes
packages/ci-scripts/src/pr-size/—summary.tsholds the pure logic (bucketing, tokei-totals validation, table rendering);orchestrate/comment.tsreads the PR files API and upserts a marker-tagged comment. Reuses the package's existinggithubApi/paginate/readPayloadhelpers rather than adding a dependency..github/workflows/pr-size.yml— two jobs.Commentholdspull-requests: write, checks outmain, and never touches PR code: GitHub's PR files API serves the per-file additions/deletions the table needs, so no checkout of the diff is required at all.Measurechecks out PR head fortokeibut holds no write token, and hands over three integers through a job output..claude/commands/pr-describe.md— Step 1.2 and the## Sizesection deleted; the agent no longer measures. Also repairs the unbalanced code fences in the Step 3 template, which were pre-existing.knip.jsonentry,README,docs/INDEX.md.Notes for review
The port is verified, not asserted. Run over a 229-file range spanning all four buckets, the new renderer produces a table byte-identical to the old awk. Against live PR #218, the rendered totals match GitHub's own
additions/deletions/changed_filesexactly.tokei is built from source, pinned to v15.0.0. It has shipped no prebuilt binary since v12.1.2 in 2021, so pinning one only gets further out of date.
ubuntu-latestalready carries a Rust toolchain, and the cold build measured 45s in CI on this PR; only the resulting binary is cached, so restore is near instant and a stale entry just rebuilds. Verified that v12.1.2, v14.0.0 and v15.0.0 return identical code/comment/blank totals on the same inputs. The workflow parses tokei's plain-textTotalrow rather than--output json, which is stable across all three versions.Two deliberate divergences from the old pipeline, both named in comments: a rename's content delta is attributed to the new path, and a binary file counts as one changed file at +0/−0 where the old awk dropped it from the file count entirely.
Cosmetic means cosmetic. Both jobs carry
continue-on-error, so a failed build or merge-base leaves the PR green and the comment still posts, saying the code/prose split is unavailable.Commentfails on this PR, by design. It runs main'sci-scripts, which has nopr-size-commentscript until this merges. The workflow run still reports success. It passes from the next PR onward; the writer path was verified end to end by running the orchestrator against this PR directly (created, then updated in place, exactly one comment).Deferred
BOTS-138 —
pull_requestruns the workflow as the PR defines it, so a branch can edit the job holding the write token. That grants a contributor nothing they lack here (pushing a branch already means repo write) and a fork's token is read-only, but it is not the boundarypull_request_targetwould give.pr-release-label-sync.ymlhas the identical exposure, so it is a repo-wide call rather than one to make in this PR. This workflow's header now states the exposure instead of claiming a boundary it does not have.Test Plan
git diff --numstat origin/main...HEAD🤖 Generated with Claude Code