Repository navigation
ci: post test vs non-test LoC on pull requests - #14738
Conversation
Count changed lines from the Pull Request Files API so the job does not clone the repo, then pin the totals at the top of the PR body.
New workflow files are not evaluated on the opening pull_request event.
Replace the one-line header with a Files/Added/Deleted/Net table and a hidden note that the block is programmatic.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds utilities to classify changed files and calculate test and non-test LoC totals. Adds Markdown rendering and PR-body block merging. Adds paginated GitHub file retrieval, CLI validation, Actions outputs, local JSON input, and optional PR updates. Adds a pull request workflow that downloads scripts from the PR head and runs the updater. Adds tests for calculations, pagination, CLI behavior, body merging, and workflow configuration. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b4707a29-87e0-45f7-b9b6-18d5639768cb
📒 Files selected for processing (4)
.github/scripts/pr-test-loc-summary.mjs.github/scripts/pr-test-loc-table.mjs.github/workflows/pr-test-loc.ymlconfig/scripts/pr-test-loc-summary.test.mjs
| const response = await fetchImpl(url, { headers }) | ||
| if (response.status === 403) { | ||
| console.log('Skipping PR body update: token cannot write (likely a fork PR).') | ||
| return 0 | ||
| } | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to read PR #${pullNumber}: ${response.status} ${response.statusText}`) | ||
| } | ||
|
|
||
| const pull = await response.json() | ||
| const nextBody = mergeLocBlock(pull.body, totals) | ||
| if (nextBody === (pull.body ?? '')) { | ||
| console.log(`PR #${pullNumber} LoC header already current.`) | ||
| return 0 | ||
| } | ||
|
|
||
| const update = await fetchImpl(url, { | ||
| method: 'PATCH', | ||
| headers: { | ||
| ...headers, | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify({ body: nextBody }) | ||
| }) | ||
| if (update.status === 403) { | ||
| console.log('Skipping PR body update: token cannot write (likely a fork PR).') | ||
| return 0 | ||
| } | ||
| if (!update.ok) { | ||
| throw new Error(`Failed to update PR #${pullNumber}: ${update.status} ${update.statusText}`) | ||
| } | ||
|
|
||
| console.log(`Updated LoC header on PR #${pullNumber}.`) | ||
| return 0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does the GitHub REST API endpoint PATCH /repos/{owner}/{repo}/pulls/{pull_number}support conditional requests, such asIf-Match with an ETag from a preceding GET, to prevent lost updates to a pull request body?
💡 Result:
The GitHub REST API endpoint PATCH /repos/{owner}/{repo}/pulls/{pull_number} does not support conditional requests using the If-Match header to prevent lost updates [1][2]. While GitHub's official documentation states that conditional requests for unsafe methods like PATCH are generally not supported unless explicitly noted for a specific endpoint [1], community reports and testing confirm that providing an If-Match header to this endpoint does not prevent the update from occurring even if the ETag is outdated [2][3]. Therefore, you cannot rely on If-Match to ensure atomicity or prevent race conditions when updating a pull request body via this endpoint [2].
Citations:
- 1: https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api?apiVersion=2026-03-10
- 2: Patch request doesn't follow conditional headers cli/cli#7167
- 3: https://github.com/orgs/community/discussions/50084
🏁 Script executed:
#!/bin/bash
set -eu
file=".github/scripts/pr-test-loc-summary.mjs"
sed -n '1,150p' "$file"
printf '\n-- updatePullRequest call sites --\n'
rg -n "updatePullRequest|mergeLocBlock|fetchImpl" "$file"Repository: stablyai/orca
Length of output: 5178
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '-- references to pr-test-loc-summary.mjs --'
rg -n -C 4 "pr-test-loc-summary\.mjs|--update-pr|update-pr" .github
printf '%s\n' '-- workflow concurrency settings --'
rg -n -C 3 "^[[:space:]]*concurrency:|cancel-in-progress:" .github/workflows .github 2>/dev/null || trueRepository: stablyai/orca
Length of output: 19862
Prevent lost PR-body updates.
updatePullRequest reads the full body and later replaces it. This can overwrite a manual edit. GitHub does not support If-Match for this pull-request PATCH endpoint, so refetch-and-retry cannot provide atomic protection. Serialize all body writers or use a bot-owned comment instead of replacing the full body.
| for (let i = 0; i < argv.length; i += 1) { | ||
| const arg = argv[i] | ||
| if (arg === '--files-json') { | ||
| filesJsonPath = argv[i + 1] | ||
| i += 1 | ||
| continue | ||
| } | ||
| if (arg === '--merge-body') { | ||
| mergeBodyPath = argv[i + 1] | ||
| i += 1 | ||
| continue | ||
| } | ||
| if (arg === '--from-pr') { | ||
| fromPrNumber = argv[i + 1] | ||
| i += 1 | ||
| continue | ||
| } | ||
| if (arg === '--update-pr') { | ||
| updatePrNumber = argv[i + 1] | ||
| i += 1 | ||
| continue | ||
| } | ||
| printUsage() | ||
| return 2 | ||
| } | ||
|
|
||
| const pullNumber = updatePrNumber ?? fromPrNumber | ||
| if ( | ||
| (argv.includes('--files-json') && filesJsonPath == null) || | ||
| (argv.includes('--merge-body') && mergeBodyPath == null) || | ||
| (argv.includes('--from-pr') && fromPrNumber == null) || | ||
| (argv.includes('--update-pr') && updatePrNumber == null) || | ||
| (filesJsonPath == null && pullNumber == null) | ||
| ) { | ||
| printUsage() | ||
| return 2 | ||
| } | ||
|
|
||
| let files | ||
| if (filesJsonPath != null) { | ||
| files = JSON.parse(readFileSync(filesJsonPath, 'utf8')) | ||
| } else { | ||
| const repository = resolveRepository() | ||
| const token = resolveToken() | ||
| if (repository == null || token == null) { | ||
| console.error('GITHUB_REPOSITORY and GITHUB_TOKEN are required to read a pull request.') | ||
| return 2 | ||
| } | ||
| files = await listPullFiles({ | ||
| ...repository, | ||
| pullNumber: Number(pullNumber), | ||
| token | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate pull request numbers before API calls.
Lines 185 and 213 convert arbitrary input with Number(...). Values such as --from-pr abc become NaN and cause an API failure instead of returning usage error code 2.
Require a positive integer for --from-pr and --update-pr. Add tests for non-numeric and missing option values.
Also applies to: 204-216
| fetchImpl: async (url) => { | ||
| const page = pages[url] | ||
| if (page == null) { | ||
| throw new Error(`unexpected url ${url}`) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the static-analysis warning.
Line 88 interpolates url directly. The static analysis check reports an invalid template-literal expression type.
Convert the value explicitly before interpolation.
Proposed fix
- throw new Error(`unexpected url ${url}`)
+ throw new Error(`unexpected url ${String(url)}`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fetchImpl: async (url) => { | |
| const page = pages[url] | |
| if (page == null) { | |
| throw new Error(`unexpected url ${url}`) | |
| } | |
| fetchImpl: async (url) => { | |
| const page = pages[url] | |
| if (page == null) { | |
| throw new Error(`unexpected url ${String(url)}`) | |
| } |
🧰 Tools
🪛 GitHub Actions: PR Checks / static analysis
[warning] 88-88: Oxlint: Invalid type used in template literal expression.
🪛 GitHub Check: static analysis
[warning] 88-88: typescript(restrict-template-expressions)
config/scripts/pr-test-loc-summary.test.mjs:88:45: Invalid type used in template literal expression.
Sources: Linters/SAST tools, Pipeline failures
… test (stablyai#14755) stablyai#14738 landed a template literal interpolating an untyped fetch url, which fails audit:code-quality:type-aware. That audit runs in the static analysis job, so main is currently red and every open PR inherits the failure.
ELI5
Every PR commit posts how many test vs non-test lines changed at the top of the description.
What Changed
PR test LoCworkflow onubuntu-latestactions/checkout: it readsfilename/additions/deletionsfrom the Pull Request Files APIWhy
Reviewers can see test vs product churn without a full clone or a self-hosted runner. GitHub-hosted minutes on this public repo are free; the job should finish in a few seconds.
Linked Issue
N/A — experiment on
nwparker/test-locVisual Proof
Live example of the table (this PR, rewritten on every commit): #14738
Testing
pnpm exec vitest run --config config/vitest.config.ts config/scripts/pr-test-loc-summary.test.mjs--from-pr 14736against the Files APIReview
Security:
pull_requestnotpull_request_target; no extra secrets; fork PRs cannot edit the description if GitHub issues a read-only token.Checklist
N/Awith reason