fix: provenance & oidc - #15
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR modernizes the project's development toolchain and CI/CD infrastructure by upgrading GitHub Actions to v6 and replacing matrix-based Node.js strategies with a version file, introducing Husky Git hooks for pre-commit and commit-message linting using commitlint, extracting tool configurations from package.json into dedicated files, adding CodeQL and dependency-review workflows, and updating Node.js LTS version. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.yaml(1 hunks)package.json(2 hunks)
🔇 Additional comments (2)
package.json (2)
16-16: Repository field simplification LGTM.String URL is valid and resolves correctly on npm.
142-144: provenance enabled in package.json; ensure runtime matches.This is correct, but it only takes effect with npm>=9.5 (Node 18/20) and Actions id-token: write (added). Align the workflow to Node 20+ and setup-node@v4 so provenance is actually attached.
After the next release, verify the npm package page shows the “provenance” badge. If it doesn’t, check the workflow’s Node/npm versions in the logs.
Dependency ReviewThe following issues were found:
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
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)
.github/workflows/ci.yaml (1)
52-54: Do not pin actions to @masterReferencing coveralls@master is a supply-chain risk; pin to a version or SHA.
- - name: Coveralls - uses: coverallsapp/github-action@master + - name: Coveralls + uses: coverallsapp/github-action@v2 with: github-token: ${{ secrets.GITHUB_TOKEN }}Optionally pin to a specific commit SHA for maximum safety.
🧹 Nitpick comments (5)
.github/workflows/dependency-review.yaml.yml (2)
33-39: Enforce failure on vulnerable dependenciesWithout fail-on-severity, merges won’t be blocked even if vulns are found. Recommend failing on high+ and retrying snapshots.
- name: 'Dependency Review' uses: actions/dependency-review-action@v4 # Commonly enabled options, see https://github.com/actions/dependency-review-action#configuration-options for all available options. with: comment-summary-in-pr: always - # fail-on-severity: moderate + fail-on-severity: high + retry-on-snapshot-warnings: true # deny-licenses: GPL-1.0-or-later, LGPL-2.0-or-later - # retry-on-snapshot-warnings: true
30-33: Align on checkout v5 across workflowsUse actions/checkout@v5 for consistency and latest fixes.
- - name: 'Checkout repository' - uses: actions/checkout@v4 + - name: 'Checkout repository' + uses: actions/checkout@v5.github/workflows/ci.yaml (1)
12-20: Speed up and harden installs: npm ci + built-in cacheUse deterministic installs and enable caching in setup-node.
- - uses: actions/checkout@v5 + - uses: actions/checkout@v5 - name: Use Node.js uses: actions/setup-node@v5 with: node-version-file: '.nvmrc' + cache: 'npm' - name: Install Dependencies - run: npm install + run: npm ci- - uses: actions/checkout@v5 + - uses: actions/checkout@v5 - name: Use Node.js uses: actions/setup-node@v5 with: node-version-file: '.nvmrc' + cache: 'npm' - - run: npm install + - run: npm ci - - run: npm i -D warframe-worldstate-data@latest- - uses: actions/checkout@v5 + - uses: actions/checkout@v5 - name: Use Node.js uses: actions/setup-node@v5 with: node-version-file: '.nvmrc' + cache: 'npm' - - run: npm install + - run: npm ciAlso applies to: 26-33, 42-47
.github/workflows/codeql.yaml.yml (2)
59-61: Use checkout v5 for consistencyMinor alignment with the rest of CI.
- - name: Checkout repository - uses: actions/checkout@v4 + - name: Checkout repository + uses: actions/checkout@v5
68-80: Enable extended security queries for JS/TSBroader coverage at low cost.
- name: Initialize CodeQL uses: github/codeql-action/init@v3 with: languages: ${{ matrix.language }} build-mode: ${{ matrix.build-mode }} + queries: security-extended # If you wish to specify custom queries, you can do so here or in a config file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yaml(3 hunks).github/workflows/codeql.yaml.yml(1 hunks).github/workflows/dependency-review.yaml.yml(1 hunks).github/workflows/release.yaml(1 hunks).nvmrc(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .nvmrc
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release.yaml
🔇 Additional comments (1)
.github/workflows/ci.yaml (1)
1-7: OIDC and provenance configuration verified Release workflow (release.yaml) definespermissions.id-token: writeandpackage.jsonhaspublishConfig.provenance: true.
There was a problem hiding this comment.
Pull request overview
This PR adds npm provenance and OIDC support to enable secure, verifiable package publishing while modernizing the project's tooling configuration. The changes refactor inline configurations from package.json into dedicated config files and update CI/CD workflows to use OIDC authentication.
- Added
publishConfig.provenance: trueto package.json and OIDC permissions (id-token: write) to the release workflow - Migrated all tool configurations (ESLint, NYC, Mocha, semantic-release) from package.json to separate YAML/JS config files
- Integrated Husky with commitlint and lint-staged for pre-commit quality checks
- Added dependency review and CodeQL security scanning workflows
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Enabled npm provenance, added commitlint/husky dev dependencies, migrated configs to external files |
| .releaserc.yml | Extracted semantic-release configuration from package.json |
| .nycrc.yml | Extracted nyc (code coverage) configuration from package.json |
| .nvmrc | Updated Node.js version to LTS Jod (from Erbium) |
| .npmignore | Added config files to ignore list for published package |
| .mocharc.yml | Extracted Mocha test configuration from package.json |
| .lintstagedrc.yml | New lint-staged configuration for pre-commit hooks |
| .husky/pre-commit | Added pre-commit hook to run lint-staged |
| .husky/commit-msg | Added commit-msg hook to enforce conventional commits |
| .github/workflows/release.yaml | Added OIDC permissions, updated to Node.js from .nvmrc, removed matrix strategy, switched to direct semantic-release execution |
| .github/workflows/dependency-review.yaml.yml | New workflow for automated dependency vulnerability scanning on PRs |
| .github/workflows/codeql.yaml | New CodeQL workflow for automated code security analysis |
| .github/workflows/ci.yaml | Standardized Node.js version from .nvmrc, replaced npm install with npm ci, added repository check for Coveralls |
| .eslintrc.yml | Extracted ESLint configuration from package.json |
| .eslintignore | Extracted ESLint ignore patterns from package.json |
| .commitlintrc.mjs | New commitlint configuration for conventional commits |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
587949e to
7923552
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.lintstagedrc.yml (1)
1-3: Runningnpm testin lint-staged is inefficient.The current configuration runs
npm testonce for each staged JavaScript file, which will execute the entire test suite multiple times and significantly slow down commits. This approach can make the developer experience frustrating, especially with larger test suites.Consider one of these alternatives:
Option 1 (Recommended): Remove
npm testfrom lint-staged and rely on CI to run tests:'*.js': - eslint --cache --fix - - npm testOption 2: Run tests once for all staged files by moving it to a separate hook:
'*.js': - eslint --cache --fix +'*': + - npm testNote: The past review comment about
lint-stagedmissing from devDependencies is still relevant. Ensure it's added to package.json.
🧹 Nitpick comments (8)
.npmignore (1)
11-19: Consider adding .husky to .npmignore.The configuration files are correctly excluded from npm packages. However, the
.husky/directory containing Git hooks should also be excluded as it's not needed in published packages.Apply this diff:
# Configuration files +.husky/ .nycrc.yml .eslintignore .eslintrc.yml .mocharc.yml .releaserc.yml .lintstagedrc.yml .commitlintrc.mjs.eslintrc.yml (1)
27-27: Consider the implications of disablingimport/no-unresolved.Setting
import/no-unresolvedto 0 disables checking for unresolved imports, which could hide legitimate import path errors and lead to runtime failures.If this was disabled to avoid false positives, consider configuring the rule with specific ignorePatterns instead of disabling it entirely.
.releaserc.yml (1)
1-14: Semantic‑release plugin set looks good; tweak changelog plugin config for clarityThe overall plugin list and
branches: masterare aligned with your workflows. One small improvement:@semantic-release/changelogtypically expectschangelogFile(and already defaults toCHANGELOG.md), so theassetsoption here is non-standard and can be confusing.You can either rely on the default or make the intent explicit:
plugins: - "@semantic-release/github" - - - "@semantic-release/changelog" - - assets: - - CHANGELOG.md + - - "@semantic-release/changelog" + - changelogFile: CHANGELOG.md - - "@semantic-release/git" - assets: - package.json - CHANGELOG.md - app.min.jsAlso worth double-checking that
app.min.jsexists before@semantic-release/gitruns (see release workflow comment)..husky/commit-msg (1)
1-1: Commit‑msg hook is fine; optionally quote the edit file argumentThe Husky hook is wired correctly and uses
--noto avoid network installs. For robustness, you can quote the edit file path:-npx --no -- commitlint --edit $1 +npx --no -- commitlint --edit "$1".github/workflows/ci.yaml (1)
30-36: Pinwarframe-worldstate-datainstead of using@latestin CIUsing
npm i -D warframe-worldstate-data@latestafternpm cireintroduces non-determinism into the build and can cause flaky CI when a new release of that package lands. This also partially defeats the point ofnpm ci’s lockfile-based reproducibility.Prefer pinning a specific version:
- Add
warframe-worldstate-datawith a concrete version todevDependenciesand letnpm ciinstall it, or- If you must install it here, pin it explicitly, e.g.
warframe-worldstate-data@X.Y.Z.- - run: npm i -D warframe-worldstate-data@latest + # Prefer a pinned devDependency and rely on `npm ci`, or explicitly pin: + # - run: npm i -D warframe-worldstate-data@X.Y.Z[ duplicate_comment ]
.github/workflows/release.yaml (1)
21-32: Ensureapp.min.jsexists before@semantic-release/gitruns, or drop it from git assetsWith the current setup:
prepublishOnlyrunsnpm run minify(perpackage.json), which only executes whennpm publishis called by@semantic-release/npmduring the publish phase.@semantic-release/git(configured in.releaserc.ymlwithassets: [package.json, CHANGELOG.md, app.min.js]) typically runs during the prepare phase, beforenpm publish.That means
app.min.jsmay not exist yet when the git plugin runs, or may be stale if it isn’t built elsewhere.Consider one of:
- Add an explicit build/minify step before
npx semantic-releasein this workflow, soapp.min.jsis present and up to date when the git plugin runs, or- Move minification to a script that semantic-release calls in
prepare(or a dedicated build step), or- If you no longer want to commit built assets, remove
app.min.jsfrom the git pluginassetslist.This keeps the repo state and published package expectations in sync.
package.json (2)
14-16: Prepublish and Husky prepare scripts are coherent; confirm you’re okay publishing full manifestSwitching
prepublishOnlyto justnpm run minifyand usingprepare: huskymatches the rest of the tooling and keeps builds simple.Without
clean-package, your publishedpackage.jsonwill now include dev fields (devDependencies, scripts, etc.), which is common practice and usually fine. If your prior intent was to ship a minimal manifest to npm, you may want either to reintroduce a trimming step or explicitly accept the larger manifest.
36-37: Dependency updates look reasonable; keep Node support expectations in mind
- Bumping
node-fetchto^2.7.0is a safe 2.x update and should remain compatible with your existing runtime API surface.- Adding
@commitlint/*andhuskyas devDependencies aligns with the new CI/Husky hooks.- Updating Mocha to
^11.7.5is fine for development, but note that modern Mocha versions target newer Node LTS releases.Given
.nvmrcnow points to a modern LTS while"engines.node"still says>=8.17.0, the declared minimum Node version is no longer exercised by your test matrix. That’s not a blocker, but consider tightening theengines.noderange in a future breaking release if you officially drop very old Node versions.Also applies to: 39-41, 46-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.commitlintrc.mjs(1 hunks).eslintignore(1 hunks).eslintrc.yml(1 hunks).github/workflows/ci.yaml(2 hunks).github/workflows/codeql.yaml(1 hunks).github/workflows/dependency-review.yaml(1 hunks).github/workflows/release.yaml(1 hunks).husky/commit-msg(1 hunks).husky/pre-commit(1 hunks).lintstagedrc.yml(1 hunks).mocharc.yml(1 hunks).npmignore(1 hunks).nycrc.yml(1 hunks).releaserc.yml(1 hunks)package.json(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- .nycrc.yml
- .eslintignore
🔇 Additional comments (10)
.mocharc.yml (1)
1-3: LGTM!The Mocha configuration is well-structured with appropriate settings for test execution.
.commitlintrc.mjs (1)
1-6: LGTM!The commitlint configuration correctly extends the conventional config and appropriately disables the body-max-line-length rule for flexibility in commit messages.
.github/workflows/dependency-review.yaml (1)
1-39: LGTM!The dependency review workflow is properly configured with appropriate permissions and uses current action versions. The filename issue mentioned in previous reviews appears to have been resolved.
.github/workflows/codeql.yaml (1)
1-100: LGTM!The CodeQL workflow is well-configured with appropriate language detection (actions and javascript-typescript), correct permissions, and current action versions. The matrix strategy and build-mode settings are appropriate for this project type.
.eslintrc.yml (1)
6-19: I'll help you verify the claim about thevalid-jsdocESLint rule. Let me start by investigating the ESLint version in use and confirm the deprecation status.
<function_calls>
#!/bin/bashFind package.json files to check ESLint version
find . -name "package.json" -type f | head -5
</function_calls>
<function_calls>
ESLint valid-jsdoc rule deprecated removed version 7
</function_calls>.github/workflows/ci.yaml (2)
12-24: Modernized lint job and title linting look goodUpgrading to
actions/checkout@v6/actions/setup-node@v6, usingnode-version-file: '.nvmrc', switching tonpm ci, and linting the PR title viacommitlintall look consistent with the rest of the toolchain.Just ensure
.nvmrcis committed and set to the Node LTS version you intend to support in CI.
46-51: Coverage job updates are consistent with lint/test and Coveralls integrationMirroring the updated checkout/setup-node usage with
.nvmrcandnpm ciin the coverage job looks good. The conditional Coveralls step for the main repo usingcoverallsapp/github-action@v2is a reasonable setup.Just make sure the
GH_TOKENsecret has the scopes Coveralls expects (or switch tosecrets.GITHUB_TOKENif a PAT isn’t required).Also applies to: 56-57
.github/workflows/release.yaml (2)
10-20: Release job permissions and Node setup align with OIDC/provenance goalsThe explicit
permissionsblock (includingid-token: write) plus the upgrade toactions/checkout@v6andactions/setup-node@v6with.nvmrcare exactly what you need for modern npm provenance support, and they keep the workflow aligned with your CI jobs.This should pair cleanly with
publishConfig.provenance: trueinpackage.jsonand the new.releaserc.yml.
21-25: Let me verify how semantic-release handles npm authentication, particularly with OIDC:
<function_calls>
#!/bin/bashCheck for semantic-release documentation or comments in the repo
echo "=== Checking for semantic-release docs/comments ==="
rg -i 'semantic.release|npm.publish' --type md --type txt 2>/dev/null | head -20echo -e "\n=== Checking package.json for semantic-release plugins ==="
cat package.json 2>/dev/null | grep -E '(semantic|release|@semantic)' | head -20echo -e "\n=== Checking if there's a .github/workflows directory with other configs ==="
ls -la .github/workflows/ 2>/dev/nullecho -e "\n=== Looking for any README or docs about publishing ==="
fd -name 'README*' -o -name 'CONTRIBUTING*' | xargs grep -l 'publish|release|npm' 2>/dev/null || echo "No release docs found"
</function_calls>Now let me search for information about semantic-release npm plugin authentication:
<function_calls>
semantic-release npm plugin OIDC authentication 2024 2025
</function_calls><function_calls>
@semantic-release/npm provenance github actions authentication
</function_calls>package.json (1)
53-55:publishConfig.provenance: trueties correctly into the provenance/OIDC setupEnabling npm provenance via:
"publishConfig": { "provenance": true }is consistent with:
- The release workflow’s
permissions.id-token: write, and- The move to a modern Node/npm toolchain via
.nvmrcandactions/setup-node@v6.Once npm auth for
@semantic-release/npmis confirmed (see release workflow comment), you should get signed provenance metadata on new publishes.
|
🎉 This PR is included in version 1.3.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What did you fix?
provenance & oidc support
Reproduction steps
do a release
Evidence/screenshot/link to line
Considerations
Summary by CodeRabbit
Release Notes
Chores
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.