sdk%fix(ci): narrow CodeQL cache, place 100 commit limit per PR, update codecov scope, restore MSRV artifact caching, try to fix false-fail due to concurrent runs#9
Conversation
|
Warning Review limit reached
More reviews will be available in 30 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds MAX_PR_COMMITS=100 and enforces it across CI workflows; refactors util.getMergeableState → getPullDetail to return mergeable_state and commits; updates pr_comment.js/pr_tag.js to use the detail and show commit-count banners; adjusts workflow caching and codecov/ESLint configs. ChangesPR Commit Validation and CI Improvements
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Note This pull request has no conflicts! 🎊 🎉 🎊 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/scripts/pr_comment.js:
- Line 12: The MAX_COMMITS constant is computed with
Number(process.env.MAX_PR_COMMITS) which yields NaN for unset/non-numeric values
and silently disables the check; validate process.env.MAX_PR_COMMITS at startup
and fail fast if it's missing or not a finite positive integer: after obtaining
the env value (used to set MAX_COMMITS) check Number.isFinite(parsed) and parsed
> 0 (or parsed >= 0 if zero is allowed) and, on invalid input, log a clear error
referencing MAX_PR_COMMITS and exit with non-zero status (e.g., throw or
process.exit(1)) so the CI/runner fails early; update references to MAX_COMMITS
in this file (the constant) accordingly.
In @.github/scripts/util.js:
- Around line 49-55: The retry loop currently returns commits: 0 when
mergeable_state remains "unknown", which hides the last observed commit count;
update the loop in the function around the mergeable check to track the last
seen commit count (e.g., maintain a variable like lastCommits and set it from
detail.commits each iteration) and use that value in the fallback return instead
of 0 so the final return is { mergeable_state: "unknown", commits: lastCommits }
(ensure the variable is initialized before the loop and updated before awaiting
MERGEABLE_DELAY_MS).
In @.github/workflows/build_stable.yml:
- Around line 45-53: The "Run pull request checks" step uses bash-only syntax
(source, [ -gt ]) but doesn't set the shell, so on Windows runners it will run
in PowerShell and fail; update the step named "Run pull request checks" to
explicitly specify shell: bash so the run: block executes with Bash (ensuring
variables like commits and MAX_PR_COMMITS and the test [ "${commits}" -gt
"${MAX_PR_COMMITS}" ] work as written) and keep the existing run script
unchanged.
In `@codecov.yml`:
- Around line 31-34: Remove the duplicate "default" mapping under
coverage.status.patch by keeping a single default entry and deleting the
repeated one; locate the repeated "default: target: auto" blocks under the
coverage.status.patch mapping in codecov.yml and ensure only one default key
remains (merge or preserve any differing values if applicable).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3413c74a-8253-452a-aa28-89a4cee6222c
📒 Files selected for processing (11)
.github/ci.env.github/scripts/pr_comment.js.github/scripts/pr_tag.js.github/scripts/util.js.github/workflows/build_msrv.yml.github/workflows/build_nightly.yml.github/workflows/build_stable.yml.github/workflows/pr_comment.yml.github/workflows/pr_tag.ymlcodecov.ymlcontrib/js/eslint.config.mjs
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/scripts/pr_comment.js (1)
12-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidation for
MAX_PR_COMMITSremains unaddressed.The past review comment requesting validation of
MAX_PR_COMMITSat startup has not been implemented. Without validation, an undefined or non-numeric value will result inNaN, causing the commit-count check at line 75 to silently fail.🤖 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 @.github/scripts/pr_comment.js at line 12, MAX_COMMITS is set directly from process.env.MAX_PR_COMMITS, which can produce NaN and break the commit-count check; validate and normalize this value at startup by parsing it with Number.parseInt or Number() then checking Number.isFinite and >= 0, and if invalid either set a safe default (e.g., 10) or log an explicit error and exit; update the constant initialization of MAX_COMMITS to perform this validation and include a clear processLogger.error/message when the env var is missing or not a valid non-negative integer so the later commit-count logic that references MAX_COMMITS behaves deterministically.
🧹 Nitpick comments (2)
.github/workflows/build_msrv.yml (1)
26-35: ⚡ Quick winConsider extracting PR commit validation to a reusable composite action.
This PR check logic is duplicated across multiple workflows (
build_msrv.yml,build_nightly.yml,build_stable.yml). Extracting it to a composite action would improve maintainability and ensure consistent enforcement.🤖 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 @.github/workflows/build_msrv.yml around lines 26 - 35, Extract the duplicated PR commit-count check into a reusable composite action (e.g., pr-commit-count) and replace the inline step named "Run pull request checks" in build_msrv.yml, build_nightly.yml and build_stable.yml with a uses: call to that composite action; have the composite action accept an input MAX_PR_COMMITS, read github.event.pull_request.commits (or accept commits as an input), run the same shell logic that compares commits to MAX_PR_COMMITS and exits with an error if exceeded, and preserve the existing error message that mentions commits and MAX_PR_COMMITS so all workflows share the same behavior..github/workflows/pr_comment.yml (1)
27-28: GITHUB_ENV append looks low risk with current.github/ci.env; optional hardening available.The static analysis concern is theoretical here:
.github/ci.envcurrently contains onlyMAX_PR_COMMITS=100, matches the expectedKEY=NUMBERformat, and has no shell metacharacters. Defense-in-depth would still be straightforward if the file may grow in the future.🛡️ Optional validation approach
- name: Read environment - run: cat .github/ci.env >> "$GITHUB_ENV" + run: grep -E '^[A-Z_]+=.+$' .github/ci.env >> "$GITHUB_ENV"🤖 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 @.github/workflows/pr_comment.yml around lines 27 - 28, The workflow step "Read environment" appends .github/ci.env directly to GITHUB_ENV which is fine now but could be hardened: replace the simple cat >> "$GITHUB_ENV" step with a guarded validation that reads .github/ci.env line-by-line, skips empty/comments, and validates each line against a safe pattern (e.g., key uses A-Z0-9_ and value contains only expected chars like digits/letters/underscore/dash) before appending to GITHUB_ENV; if any line fails validation, fail the job with a clear error. Use the step name "Read environment", reference the source file .github/ci.env and the target GITHUB_ENV when implementing the check so future additions cannot inject shell metacharacters or malformed KEY=VALUE entries.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In @.github/scripts/pr_comment.js:
- Line 12: MAX_COMMITS is set directly from process.env.MAX_PR_COMMITS, which
can produce NaN and break the commit-count check; validate and normalize this
value at startup by parsing it with Number.parseInt or Number() then checking
Number.isFinite and >= 0, and if invalid either set a safe default (e.g., 10) or
log an explicit error and exit; update the constant initialization of
MAX_COMMITS to perform this validation and include a clear
processLogger.error/message when the env var is missing or not a valid
non-negative integer so the later commit-count logic that references MAX_COMMITS
behaves deterministically.
---
Nitpick comments:
In @.github/workflows/build_msrv.yml:
- Around line 26-35: Extract the duplicated PR commit-count check into a
reusable composite action (e.g., pr-commit-count) and replace the inline step
named "Run pull request checks" in build_msrv.yml, build_nightly.yml and
build_stable.yml with a uses: call to that composite action; have the composite
action accept an input MAX_PR_COMMITS, read github.event.pull_request.commits
(or accept commits as an input), run the same shell logic that compares commits
to MAX_PR_COMMITS and exits with an error if exceeded, and preserve the existing
error message that mentions commits and MAX_PR_COMMITS so all workflows share
the same behavior.
In @.github/workflows/pr_comment.yml:
- Around line 27-28: The workflow step "Read environment" appends .github/ci.env
directly to GITHUB_ENV which is fine now but could be hardened: replace the
simple cat >> "$GITHUB_ENV" step with a guarded validation that reads
.github/ci.env line-by-line, skips empty/comments, and validates each line
against a safe pattern (e.g., key uses A-Z0-9_ and value contains only expected
chars like digits/letters/underscore/dash) before appending to GITHUB_ENV; if
any line fails validation, fail the job with a clear error. Use the step name
"Read environment", reference the source file .github/ci.env and the target
GITHUB_ENV when implementing the check so future additions cannot inject shell
metacharacters or malformed KEY=VALUE entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bc29b487-d9f2-4ed0-9be5-e020a955ff02
📒 Files selected for processing (10)
.github/ci.env.github/scripts/pr_comment.js.github/scripts/pr_tag.js.github/scripts/util.js.github/workflows/build_msrv.yml.github/workflows/build_nightly.yml.github/workflows/build_stable.yml.github/workflows/pr_comment.ymlcodecov.ymlcontrib/js/eslint.config.mjs
✅ Files skipped from review due to trivial changes (1)
- .github/ci.env
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/scripts/pr_tag.js
- contrib/js/eslint.config.mjs
- .github/scripts/util.js
kwvg
force-pushed
the
ci_fixes
branch
2 times, most recently
from
e60c3f8 to
8772e5b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/build_msrv.yml (1)
99-113: ⚡ Quick winConsider adding
persist-credentials: falseto the checkout step.The checkout action at lines 99-102 does not explicitly set
persist-credentials: false. By default, credentials are persisted, which means the GitHub token remains available to subsequent steps and could be exposed through build artifacts or logs if not carefully managed.🔒 Proposed security hardening
- name: Checkout uses: actions/checkout@v6 with: fetch-depth: 1 + persist-credentials: false🤖 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 @.github/workflows/build_msrv.yml around lines 99 - 113, The Checkout step using actions/checkout@v6 is persisting credentials by default; update that step to explicitly set persist-credentials: false so the GITHUB_TOKEN is not left available to later steps — locate the Checkout block (uses: actions/checkout@v6) and add the persist-credentials: false input alongside fetch-depth to harden credentials handling.Source: Linters/SAST tools
.github/workflows/pr_comment.yml (1)
27-28: ⚡ Quick winConsider parsing specific variables instead of appending the entire file.
Appending
.github/ci.envdirectly to$GITHUB_ENVmeans any line in that file becomes an environment variable. While the file is version-controlled and reviewed, this pattern can be fragile—comments, malformed lines, or unintended key-value pairs could inject unexpected variables or break the workflow.A more explicit approach would parse only the known variables (e.g.,
MAX_PR_COMMITS) usinggreporawk, reducing the attack surface and making the dependency clearer.🔒 Alternative: parse specific variables
- name: Read environment - run: cat .github/ci.env >> "$GITHUB_ENV" + run: | + grep '^MAX_PR_COMMITS=' .github/ci.env >> "$GITHUB_ENV"Or, for multiple variables:
- name: Read environment - run: cat .github/ci.env >> "$GITHUB_ENV" + run: | + grep -E '^(MAX_PR_COMMITS)=' .github/ci.env >> "$GITHUB_ENV"🤖 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 @.github/workflows/pr_comment.yml around lines 27 - 28, The current "Read environment" step appends the entire .github/ci.env into $GITHUB_ENV which can introduce malformed or unintended variables; change the step to explicitly extract and export only the known keys (e.g., MAX_PR_COMMITS and any other required vars) by grepping/awk-ing those variable names from .github/ci.env and appending those filtered key=value lines to $GITHUB_ENV so only the specific variables are set; update the step named "Read environment" to perform this targeted parse rather than a blind cat.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In @.github/workflows/build_msrv.yml:
- Around line 99-113: The Checkout step using actions/checkout@v6 is persisting
credentials by default; update that step to explicitly set persist-credentials:
false so the GITHUB_TOKEN is not left available to later steps — locate the
Checkout block (uses: actions/checkout@v6) and add the persist-credentials:
false input alongside fetch-depth to harden credentials handling.
In @.github/workflows/pr_comment.yml:
- Around line 27-28: The current "Read environment" step appends the entire
.github/ci.env into $GITHUB_ENV which can introduce malformed or unintended
variables; change the step to explicitly extract and export only the known keys
(e.g., MAX_PR_COMMITS and any other required vars) by grepping/awk-ing those
variable names from .github/ci.env and appending those filtered key=value lines
to $GITHUB_ENV so only the specific variables are set; update the step named
"Read environment" to perform this targeted parse rather than a blind cat.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8fc9988f-73e3-4d4a-86b1-c0f125c46dd7
📒 Files selected for processing (10)
.github/ci.env.github/scripts/pr_comment.js.github/scripts/pr_tag.js.github/scripts/util.js.github/workflows/build_msrv.yml.github/workflows/build_nightly.yml.github/workflows/build_stable.yml.github/workflows/pr_comment.ymlcodecov.ymlcontrib/js/eslint.config.mjs
✅ Files skipped from review due to trivial changes (1)
- .github/ci.env
🚧 Files skipped from review as they are similar to previous changes (3)
- contrib/js/eslint.config.mjs
- .github/scripts/util.js
- .github/scripts/pr_comment.js
`dash-dev` is omitted from codecov, the README and other documentation as it's a purely internal development package with no public guarantees
1 participant
Additional Information
Codecov has troubles with measuring coverage if the base branch reports CI failures. The PR comment runner is triggered by both pull requests and pushes to
developand under concurrency rules, generally, thedevelop-triggered instance is terminated, which is reported as a failure.An adjustment to the triggers were made in 86a5512 in base-sdk#5 which did not help. This is a second pass to restore Codecov functionality.
The
dash-typescrate was added in base-sdk#1 but Codecov was not updated to recognise it, this has been remedied in this pull request.The CodeQL per-build cache is wasteful as they're most likely to churn compared to pack-based caching which has far lesser churn. Likewise, base-sdk#5 changed how MSRV builds were done but the caching directives did not fully make it through, which has also been resolved.
Pull requests are now limited to 100 commits, refusing to run CI otherwise. This is part of optimising CI performance by limiting fetch depth and encouraging more atomic pull requests.
Breaking Changes
None expected.
How Has This Been Tested?
kwvg/base-sdk#1 and
./contrib/lint/all_lint.pyChecklist