Conversation
📚 Pull Request Stack
Managed by gh-stack |
There was a problem hiding this comment.
🟡 Changes recommended
The lint workflow introduces invalid GitHub Actions syntax (parallel:) and the publish/prepare changes contain issues that can break releases and unnecessarily expand CI work.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR overhauls modestbench’s GitHub Actions CI/release setup to align with practices from boneskull/boneskull-template, including tighter permissions scoping, added concurrency controls, and additional CI checks.
Changes:
- Refactors multiple workflows to use job-level permissions and adds concurrency groups for PR-related workflows.
- Introduces
zizmorconfiguration and adds zizmor scanning into the lint workflow. - Updates composite actions (
prepare,publish) and consolidates/restructures release/publish flow (including removing the standalonepublish.ymlworkflow).
File summaries
| File | Description |
|---|---|
| cspell.json | Adds CI/tooling-related words to the spellchecker allowlist. |
| .github/zizmor.yml | Adds zizmor configuration overrides. |
| .github/workflows/release.yml | Restructures release workflow permissions and adds workflow_dispatch behavior. |
| .github/workflows/publish.yml | Removes the standalone manual publish workflow. |
| .github/workflows/pr-assign-labels.yml | Adds concurrency + job-level permissions for labeling action. |
| .github/workflows/pr-assign-author.yml | Adds concurrency, scoping, and updates action pin; documents zizmor trigger ignore. |
| .github/workflows/nodejs.yml | Hardens checkout by disabling persisted credentials. |
| .github/workflows/lint.yml | Expands lint workflow triggers and adds more lint checks + zizmor scan. |
| .github/workflows/docs.yml | Switches setup to reuse prepare and scopes Pages deploy permissions to the deploy job. |
| .github/workflows/commitlint.yml | Adds concurrency and updates checkout usage; continues using shared prepare step. |
| .github/actions/publish/action.yml | Updates publish action dependencies and publishing flow. |
| .github/actions/prepare/action.yml | Changes prepare action to use npm ci and adds a build step. |
Review details
Suppressed comments (1)
.github/actions/publish/action.yml:30
- The token export guard is inverted: it currently sets
NODE_AUTH_TOKENonly when the token is empty, which breaks token-based publishing.
if [ -z "$INPUT_TOKEN" ]; then
export NODE_AUTH_TOKEN="${INPUT_TOKEN}"
fi
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Deploying modestbench with
|
| Latest commit: |
fbf1251
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0d8f0e3d.modestbench.pages.dev |
| Branch Preview URL: | https://fixup-ci.modestbench.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
The lint workflow currently uses an invalid parallel: step structure and the publish composite action’s token export logic is inverted, which will break CI/publishing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/lint.yml:35
parallel:is not a valid key for a GitHub Actions step, so this workflow will fail to parse; convert these into normal steps (or separate jobs) instead.
- run: npm run build
- parallel:
- name: ESLint
run: npm run lint:eslint
- name: markdownlint
- Files reviewed: 13/14 changed files
- Comments generated: 2
- Review effort level: Lite
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
The lint workflow contains invalid GitHub Actions syntax (parallel step) and the publish composite action has a broken token export condition that will cause publishing to fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
.github/workflows/lint.yml:32
- The workflow uses a
- parallel:step key, which is not valid GitHub Actions syntax (steps must userun/uses/nameetc.). As written, this workflow will fail to parse and none of the linters will run. Split these into individual steps (or separate jobs/matrix if true parallelism is required).
- parallel:
.github/workflows/lint.yml:31
./.github/actions/preparealready runsnpm run build, so this extranpm run buildstep is redundant and adds unnecessary CI time.
- run: npm run build
.github/actions/publish/action.yml:31
- The token export condition is inverted: this sets
NODE_AUTH_TOKENonly when the token is empty, and leaves it unset when a token is provided, causingnpm publishauth to fail.
run: |
if [ -z "$INPUT_TOKEN" ]; then
export NODE_AUTH_TOKEN="${INPUT_TOKEN}"
fi
npm publish --provenance --access public --registry https://registry.npmjs.org
- Files reviewed: 13/14 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The lint workflow introduces invalid GitHub Actions syntax (parallel:) and the publish composite action currently mishandles auth/setup-node in ways that can break releases.
Review details
Suppressed comments (4)
.github/workflows/lint.yml:34
stepsentries in GitHub Actions must userunoruses; the- parallel:block is not valid workflow syntax and will cause this workflow to fail validation. Since./.github/actions/preparealready runsnpm run build, the extranpm run buildstep is also redundant.
- uses: ./.github/actions/prepare
- run: npm run build
- parallel:
- name: ESLint
run: npm run lint:eslint
.github/actions/publish/action.yml:31
- The token-export guard is inverted:
NODE_AUTH_TOKENis only set whenINPUT_TOKENis empty, so publishing with a provided token will run without auth. This makesnpm publishfail unless some other auth mechanism is preconfigured.
run: |
if [ -z "$INPUT_TOKEN" ]; then
export NODE_AUTH_TOKEN="${INPUT_TOKEN}"
fi
npm publish --provenance --access public --registry https://registry.npmjs.org
package.json:106
installed-check@11requires Node^22.22.2 || ^24.15.0 || >=26(see package-lock), but this repo declaresengines.node >=20and CI tests Node 20. As written,npm run lint(which runs alllint:*scripts) can fail on a supported Node 20 environment. Consider either raising the repo's Node engine/CI minimum, or gatinglint:enginesto only run on Node 22+.
"lint": "run-p -sl --aggregate-output lint:*",
"lint-staged": "lint-staged",
"lint:engines": "installed-check --engine-check",
"lint:eslint": "eslint .",
.github/actions/publish/action.yml:23
- This action runs
actions/setup-nodetwice: once here (withregistry-urland pinned24.19.0), then again inside./.github/actions/prepare(defaulting to24and not settingregistry-url). The second setup can override the pinned version/config, making publish behavior dependent on the last setup-node invocation.
- name: Setup Node.js
uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
with:
node-version: 24.19.0
registry-url: https://registry.npmjs.org
- uses: ./.github/actions/prepare
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The updated lint workflow uses an invalid parallel: step structure and the publish composite action has a broken token-export condition that can prevent authenticated publishing.
Review details
Suppressed comments (3)
.github/actions/publish/action.yml:30
- The publish step never sets
NODE_AUTH_TOKENwhen a token is provided: the-z(empty) check is inverted, so token auth will be skipped wheninputs.tokenis non-empty (and an empty token may be exported instead).
if [ -z "$INPUT_TOKEN" ]; then
export NODE_AUTH_TOKEN="${INPUT_TOKEN}"
fi
.github/workflows/lint.yml:35
- This workflow uses a
- parallel:step, which isn’t a valid GitHub Actions step key (steps must userun:oruses:). As written, the workflow will fail YAML/schema validation and won’t execute the linters/zizmor.
- run: npm run build
- parallel:
- name: ESLint
run: npm run lint:eslint
- name: markdownlint
.github/workflows/release.yml:35
./.github/actions/publishalready checks out the repository internally, so this extra checkout step is redundant and adds time (and potential confusion about which checkout is used).
- if: ${{ steps.release.outputs.release_created || github.event_name == 'workflow_dispatch' }}
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
persist-credentials: false
- if: ${{ steps.release.outputs.release_created || github.event_name == 'workflow_dispatch' }}
uses: ./.github/actions/publish
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The lint workflow uses an invalid parallel: step structure and the publish composite action currently sets NODE_AUTH_TOKEN incorrectly, both of which will break CI/release execution.
Review details
Suppressed comments (2)
.github/workflows/lint.yml:35
parallel:is not a valid GitHub Actions step key, so this workflow will fail YAML/schema validation and never run. If the goal is to run linters concurrently, use a singlerunstep that invokes your existingnpm run lint(which already runslint:*in parallel) and keep zizmor as a separate step.
- run: npm run build
- parallel:
- name: ESLint
run: npm run lint:eslint
- name: markdownlint
.github/actions/publish/action.yml:30
- The token export logic is inverted: when
INPUT_TOKENis empty, this setsNODE_AUTH_TOKENto an empty string (potentially overriding a token already present in the environment) and will break publishing. Only setNODE_AUTH_TOKENwhen a non-empty token was provided.
shell: bash
using: composite
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
50e3da1 to
86e9741
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new lint workflow uses invalid GitHub Actions syntax (parallel: under steps), and the publish composite action’s token/auth handling is currently incorrect and likely to break releases.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/lint.yml:33
- The
parallel:key understepsis not valid GitHub Actions workflow syntax, so this workflow will fail to run. Also,./.github/actions/preparealready runsnpm run buildby default, making the explicitnpm run buildstep redundant here.
A simple fix is to run the existing npm run lint script (which already runs the lint:* scripts in parallel via npm-run-all2) and keep zizmor as a separate step.
- run: npm run build
- parallel:
- name: ESLint
- Files reviewed: 13/14 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The lint workflow contains invalid GitHub Actions syntax (parallel: step) and the publish composite action’s token-based auth wiring is likely broken, which can break CI and releases.
Review details
Suppressed comments (2)
.github/workflows/lint.yml:34
- The
stepslist contains aparallel:mapping, which is not valid GitHub Actions workflow syntax and will prevent this workflow from running. Also,./.github/actions/preparebuilds by default, so the explicitnpm run buildhere causes a duplicate build unless you disable it in the prepare action call.
- uses: ./.github/actions/prepare
- run: npm run build
- parallel:
- name: ESLint
run: npm run lint:eslint
.github/actions/publish/action.yml:26
npm publishwill not reliably authenticate with the registry just by exportingNODE_AUTH_TOKEN; npm typically needs an npmrc entry (orsetup-nodewithregistry-url) to wire the token into//registry.npmjs.org/:_authToken. As written, publishing with a provided token is likely to fail.
- name: Publish to npm
env:
INPUT_TOKEN: ${{ inputs.token }}
run: |
if [ -n "$INPUT_TOKEN" ]; then
export NODE_AUTH_TOKEN="${INPUT_TOKEN}"
fi
npm publish --provenance --access public --registry https://registry.npmjs.org
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Use best practices from `boneskull/boneskull-template`
There was a problem hiding this comment.
🟡 Changes recommended
The updated workflows include a GitHub Actions syntax/permissions regression that will break CI execution (invalid parallel: usage and missing contents: read where checkout is used).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/actions/publish/action.yml:26
- The action sets
NODE_AUTH_TOKENwheninputs.tokenis provided, but npm won't use it unless the registry auth token is configured (typically viaactions/setup-nodewithregistry-urlor an.npmrcentry). Since the repo.npmrcdoesn't configure auth and theprepareaction doesn't setregistry-url, token-based publishing is likely to fail.
env:
INPUT_TOKEN: ${{ inputs.token }}
run: |
if [ -n "$INPUT_TOKEN" ]; then
export NODE_AUTH_TOKEN="${INPUT_TOKEN}"
fi
npm publish --provenance --access public --registry https://registry.npmjs.org
- Files reviewed: 14/15 changed files
- Comments generated: 3
- Review effort level: Lite
|
|
||
| permissions: {} | ||
|
|
| - uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 | ||
| with: | ||
| node-version: 18 |
| - run: npm run build | ||
| - parallel: | ||
| - name: ESLint | ||
| run: npm run lint:eslint |
There was a problem hiding this comment.
🔵 Needs a closer look
The updated lint.yml workflow contains invalid GitHub Actions syntax (parallel: as a step), which will break CI.
Review details
Suppressed comments (3)
.github/workflows/lint.yml:31
parallel:is not a valid key for a step in GitHub Actions workflow syntax, so this workflow will fail to parse. If you want parallelization,npm run lintalready runslint:*scripts concurrently viarun-p; keep zizmor as a separate step.
- parallel:
examples/.github/workflows/benchmarks.yml:9
- This example workflow sets
permissions: {}but does not grantcontents: read, whichactions/checkoutrequires; as written, the checkout step will fail in repos which rely onGITHUB_TOKENauth (e.g., private repos).
permissions: {}
examples/.github/workflows/benchmarks.yml:21
- This example uses Node.js 18, but this package declares
engines.nodeas^22.22.2 || ^24.15.0 || >=26.0.0(see package.json). Using Node 18 here will fail (or at least not reflect supported usage).
node-version: 18
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
Use best practices from
boneskull/boneskull-template