Skip to content

fix: add workflow permissions and npm provenance #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
evansantos merged 4 commits into from
Feb 19, 2026
Merged

fix: add workflow permissions and npm provenance #16

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be372f9
fix: add workflow permissions and npm provenance for publish
evansantos Feb 19, 2026
f7b6043
fix: make npm audit non-fatal in CI
evansantos Feb 19, 2026
316b686
fix: address GitSniff review warnings
evansantos Feb 19, 2026
7e42f54
fix: implement GitSniff suggestions properly
evansantos Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ on:
pull_request:
branches: [ main, develop ]
Comment on lines 6 to 7
Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Warning

The current CI workflow for ci.yml is triggered only on pull_request events for main and develop branches. This means that direct pushes to these branches, or any other branch, will not trigger the CI workflow. This can lead to situations where code is pushed directly to the main or develop branch without being fully tested by the CI pipeline, potentially introducing regressions or untested code.

Fix: Add push events as a trigger for the CI workflow to ensure that all pushes to the specified branches (main, develop) also run the CI checks. This provides more comprehensive testing and prevents untested code from being merged or deployed. It also provides immediate feedback on the health of the branch after a direct push.

🤖 Prompt for AI Agents 
In .github/workflows/ci.yml around line 6:

Issue: The current CI workflow for `ci.yml` is triggered only on `pull_request` events for `main` and `develop` branches. This means that direct pushes to these branches, or any other branch, will not trigger the CI workflow. This can lead to situations where code is pushed directly to the main or develop branch without being fully tested by the CI pipeline, potentially introducing regressions or untested code.

Add `push` events as a trigger for the CI workflow to ensure that all pushes to the specified branches (main, develop) also run the CI checks. This provides more comprehensive testing and prevents untested code from being merged or deployed. It also provides immediate feedback on the health of the branch after a direct push.

Replace with:
on:
  push:
    branches: [ main, develop ]
  pull_request:
    branches: [ main, develop ]


Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Security

The ci.yml workflow specifies permissions for pull_request events, but not for push events. While contents: read is a sensible default, it's good practice to explicitly define permissions for all trigger events if they differ or if default permissions are insufficient for certain jobs. This ensures consistent and explicit permission management.

Fix: Add a permissions: block directly under the on:push: event or at the top-level on: block to cover all events where permissions might be required. In this case, contents: read is likely sufficient for the push event as well.

permissions:
contents: read

jobs:
test:
runs-on: ubuntu-latest
Expand Down Expand Up @@ -42,7 +45,7 @@ jobs:
run: npm run build

- name: Check for vulnerabilities
run: npm audit --omit=dev
run: npm audit --omit=dev || echo "::warning::npm audit found vulnerabilities (see above)"
Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Security

The npm audit command is used to check for vulnerabilities, and currently, it allows the workflow to proceed even if vulnerabilities are found by echoing a warning. While this prevents the CI from failing due to minor vulnerabilities, it might lead to overlooking critical security issues if not explicitly configured to fail on high or critical vulnerabilities. For a high strictness level, it's recommended that npm audit should fail the build if critical or high severity vulnerabilities are detected, ensuring that all security-related issues are addressed before deployment.

Fix: Consider configuring npm audit to fail on high or critical severity vulnerabilities. This can be achieved by removing || echo "::warning::npm audit found vulnerabilities (see above)" and relying on the default exit code of npm audit, or by adding a specific flag like --audit-level=high if available in the npm version being used. If npm audit does not support this level of granularity directly, consider using npm audit --json and parsing the output to enforce a specific failure threshold.


- name: Upload coverage to Codecov (if exists)
uses: codecov/codecov-action@v3
Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Info

The condition if: matrix.node-version == '22.x' for uploading coverage to Codecov is now redundant. With the removal of the build matrix and a single Node.js version set in env.NODE_VERSION, this condition will always evaluate to true (or the equivalent of always running), and it is no longer necessary to explicitly check the Node.js version.

Fix: Remove the redundant if: condition from the Codecov action, as it no longer serves a purpose with the simplified Node.js versioning.

Suggested change
uses: codecov/codecov-action@v3
uses: codecov/codecov-action@v3

Expand All @@ -60,10 +63,10 @@ jobs:
steps:
- uses: actions/checkout@v4

- name: Use Node.js 20.x
- name: Use Node.js 22.x
uses: actions/setup-node@v4
with:
node-version: '20.x'
node-version: '22.x'
Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Info

The build-check job uses Node.js 22.x, but the lint, typecheck, and security jobs explicitly set node-version: ${{ env.NODE_VERSION }}, which is defined as 22 at the workflow level. For consistency and to avoid potential discrepancies, all jobs should reference the NODE_VERSION environment variable. This ensures a single source of truth for the Node.js version across the CI.

Fix: Modify the build-check job to use node-version: ${{ env.NODE_VERSION }} instead of a hardcoded '22.x'. This makes the Node.js version consistent across all jobs that use it and simplifies future updates.

Suggested change
- name: Use Node.js 22.x
uses: actions/setup-node@v4
with:
node-version: '20.x'
node-version: '22.x'
- name: Use Node.js ${{ env.NODE_VERSION }}
uses: actions/setup-node@v4
with:
node-version: ${{ env.NODE_VERSION }}

cache: 'npm'

- name: Install dependencies
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/publish.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ jobs:
runs-on: ubuntu-latest
permissions:
contents: write
id-token: write

steps:
- uses: actions/checkout@v4
Expand All @@ -48,7 +49,7 @@ jobs:

- name: Publish to npm
if: ${{ github.event.inputs.dry_run != 'true' }}
run: npm publish --access public
run: npm publish --access public --provenance
env:
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}

Expand Down
Loading