remove package-lock.json and add Snyk SCA GHA to CI#5669
Conversation
Renovate fails to update bun.lock when package-lock.json is also present, causing all frontend dependency PRs to fail CI with "lockfile had changes, but lockfile is frozen". Removing the npm lockfile lets Renovate detect bun as the package manager and regenerate bun.lock correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds a reusable GitHub Actions workflow that runs Snyk SCA against all three project components (frontend, backend API, data tools), replacing the non-functional SCM integration that required a committed npm lockfile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
johndeange
left a comment
There was a problem hiding this comment.
Code Review
Overview
This PR solves a real pain point: all Renovate frontend dependency PRs are failing because npm's package-lock.json and bun's bun.lock coexist, causing lockfile conflicts. The fix removes frontend/package-lock.json, gitignores it, and replaces the Snyk SCM integration with a CI-driven workflow that generates the lockfile ephemerally.
Strengths
- Well-written PR description — clearly explains the problem, root cause, and tradeoffs
- Good observability — distinguishes "vulnerability found" vs "infrastructure failure" via SARIF file existence check
- Pin-by-SHA for all third-party actions — solid supply chain practice
- Minimal permissions scoped per job
continue-on-error: trueon snyk test is correct — vulnerabilities shouldn't block CI, but infra failures should
Suggestions (non-blocking)
-
SNYK_TOKEN chicken-and-egg: All 3 Snyk jobs currently fail because the secret isn't configured yet. Consider making the token validation a soft failure (warning + skip) so this PR can merge cleanly and the token configured afterward — or ensure the secret is added before merge.
-
Reuse composite action: The project has
.github/actions/setup-python/action.ymlthat handles Python/pipenv setup. Thesnyk-backend-apijob could use it to reduce duplication. (The data-tools job can't directly since the composite action hardcodesworking-directory: ./backend/ops_api.) -
Add
timeout-minutes: These jobs depend on the external Snyk API. Addingtimeout-minutes: 10would prevent hangs during Snyk outages (default is 6 hours). -
External Snyk check:
security/snyk (Flexion-OPRE-OPS)also fails — the legacy SCM integration will likely need to be reconfigured/disabled as a follow-up.
Security
- No secrets leaked in logs —
SNYK_TOKENpassed viaenvblock - SHA-pinned actions prevent supply chain attacks
- Removing the lockfile from git does NOT reduce security — Snyk still scans the ephemeral lockfile, and
bun.lockprovides reproducible builds
LGTM — architecture is sound and unblocks Renovate. 👍
josbell
left a comment
There was a problem hiding this comment.
Review: Request Changes
Thanks for this PR! The investigation confirms this correctly solves the Renovate lockfile conflict. However, I identified one critical issue that should be addressed before merge.
❗ Required: Add Timeout Protection
Issue: All three Snyk jobs lack timeout-minutes configuration. If Snyk API experiences an outage, jobs could hang for 6 hours (GitHub Actions default).
Fix: Add to each job (lines 16, 78, 140):
```yaml
snyk-frontend:
name: Snyk SCA for Frontend
runs-on: ubuntu-latest
timeout-minutes: 10 # Add this
permissions:
contents: read
security-events: write
```
Repeat for snyk-backend-api and snyk-backend-data-tools jobs.
Rationale: 10 minutes is sufficient for dependency scanning; longer runtime indicates API failure. Only 1 of 12+ workflows in our codebase has timeouts, making this even more important.
💡 Optional Suggestion: Use Composite Action for backend-api
Lines 84-97: Consider using the existing composite action instead of manual Python setup:
```yaml
Replace lines 84-97 with:
- name: Setup Python
uses: ./.github/actions/setup-python
```
Benefits:
- Automatic pipenv caching (faster CI)
- Consistent with other workflows
- Reduces maintenance burden
Note: Can't use for data_tools job due to hardcoded path in composite action (separate issue to address later).
📋 Follow-up (Out of Scope)
Python 3.14 + setup-python@v4.3.0: Systemic issue affecting 4+ workflows. The v4.3.0 action predates Python 3.14 release, likely causing slower CI. Consider upgrading to v5.x in a separate PR.
Once the timeout is added, this is good to merge! 🚀
📍 Specific Line CommentsFile: |
|
Line 16-24 (snyk-frontend job): Missing |
|
Line 78-86 (snyk-backend-api job): Missing |
|
Line 140-148 (snyk-backend-data-tools job): Missing |
|
Lines 84-97 (optional): Consider replacing manual Python setup with composite action |
- Add timeout-minutes: 10 to all three Snyk jobs to prevent hung jobs on Snyk API outages (default is 6 hours) - Replace manual Python setup in snyk-backend-api with the existing setup-python composite action for consistency and pipenv caching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@josbell those were great suggestions. I implemetend the "required" and "optional" changes. Please do re-review when you can, thanks! |
react-currency-format@1.1.0 declares a peer dep on React <=17, which conflicts with our React 19. npm strict mode refuses to generate a lockfile. Adding --legacy-peer-deps allows npm to produce the lockfile for Snyk scanning despite the peer dep mismatch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
The snyk/actions/python action runs inside a Docker container that does not have pipenv installed. Pointing at Pipfile.lock instead of Pipfile lets Snyk parse the resolved dependency tree directly without needing pipenv. This also removes the now-unnecessary Python/pipenv setup steps. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The snyk/actions/python Docker action lacks pipenv inside its container, causing "Failed to run pipenv" errors. Switch to snyk/actions/setup (installs CLI on the runner) + shell commands so Snyk can access the runner's pipenv installation and properly resolve the Pipfile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🎉 This PR is included in version 1.402.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
frontend/package-lock.jsonfrom git tracking and adds it to.gitignoreProblem
Every Renovate PR that updates
frontend/package.jsonfails with:The
renovate/artifactsstatus showsFAILUREon all of them (PRs #5624, #5643, #5645, #5649, and earlier #5625).Root cause: When both
package-lock.json(npm) andbun.lock(bun) exist in the same directory, Renovate uses npm to update lockfiles. It updatespackage-lock.jsonbut notbun.lock. Since CI runsbun install --frozen-lockfile, the stalebun.lockcauses every frontend job to fail.Snyk SCA Replacement
Since
package-lock.jsonwas added in PR #5604 to support Snyk SCA scanning (which doesn't natively supportbun.lock), removing it breaks Snyk's SCM integration. This PR replaces that with a GitHub Actions workflow (.github/workflows/security_snyk.yml) that:package-lock.jsonephemerally vianpm install --package-lock-only, then runssnyk testandsnyk monitorbackend/ops_api/Pipfilewithsnyk/actions/pythonbackend/data_tools/Pipfilewithsnyk/actions/pythonResults upload as SARIF to GitHub's Security tab (Code Scanning), alongside existing CodeQL and Semgrep findings.
Observability safeguards:
SNYK_TOKENexists before scanning (fail-fast on missing secret)::warning::annotations ifsnyk monitorfails to register the projectPrerequisite: A
SNYK_TOKENrepository secret must be added in Settings > Secrets and variables > Actions before the workflow will function. Also will need to modify the Snyk settings to not be concerned with the absence of the nom lockfile?What this fixes
Once merged, Renovate will detect only
bun.lockand use bun to regenerate it when dependencies change. The existing open Renovate PRs should automatically rebase and succeed after this lands.Test plan
SNYK_TOKENsecret and verify the Snyk SCA workflow produces SARIF findings in the Security tabSNYK_TOKENis missingrenovate/artifactspasses