chore: adopt fallow static analysis CI gate (#15) - #88
Conversation
Add fallow as a devDependency and a report-only static-analysis step to the CI verify job: `fallow audit --format sarif` plus a SARIF upload to GitHub code scanning. The step is non-blocking (`|| true`) for now; a follow-up flips it to a hard gate once the baseline is clean. Adds `security-events: write` to the job so upload-sarif can publish. Covers the CI lane only (Steps 1/3/4). Baseline triage and Stop-hook registration (Step 2) depend on #36 and are tracked there. No src/ files modified; build and test pass unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The first run failed at the SARIF upload: `fallow audit` is a diff gate and could not detect a base branch in CI's shallow/detached checkout, so it errored to stderr and wrote an empty fallow.sarif that upload-sarif rejected as invalid JSON. - checkout with fetch-depth: 0 so audit can resolve a merge-base. - pass --base origin/main explicitly. - guard: emit a valid empty SARIF if fallow writes nothing, so the upload step never fails on report-only runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
upload-sarif rejected the report: fallow emits one SARIF run per sub-analysis (dead-code/health/dupes), all with an empty category, and CodeQL refuses multiple runs sharing a category in one file. Post-process with node to give each run a distinct automationDetails.id before upload. Co-Authored-By: Claude Opus 4.8 <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. |
s-annam
left a comment
There was a problem hiding this comment.
PR Review: chore: adopt fallow static analysis CI gate (#15)
Summary
Clean, well-scoped CI-only adoption of fallow static analysis. Build/lint/typecheck green, 351 tests pass, no src/ files touched. The package is legit and the CI logic is sound. Approving — with one correction to the PR description before merge.
Verified
| Check | Result |
|---|---|
| Build / lint / typecheck | green |
| Tests | 351 passed (32 files) |
fallow provenance |
real fallow-rs/fallow (3649★, MIT), published via GitHub Actions OIDC trusted publisher; binaries integrity-pinned |
| Deps added | 10 — fallow + detect-libc + 8 @fallow-cli/* optional platform binaries |
--base flag |
valid (alias of --changed-since) |
| SARIF step | runs → valid SARIF, distinct automationDetails.id per run (solves CodeQL multi-run/category rejection) |
npm ci before npx fallow |
yes → uses pinned binary, not a fresh fetch |
fetch-depth: 0 |
correct — needed so checkout exposes origin/main for the merge-base |
Key Findings
-
[Suggestion — fix before merge] PR description misattributes dependencies/vulns to fallow. The body states "the fallow dependency tree adds 523 transitive packages; npm audit reports 4 vulnerabilities (1 critical, 2 high, 1 moderate)." Both are inaccurate:
- fallow adds 10 packages, not 523 (npm registry shows
deps: 1—detect-libc; the rest are optional per-platform binaries). - The audit vulns (now 5: dompurify, esbuild, protobufjs, vite, vitest) are all pre-existing and trace to
posthog-jsandvite/vitest— none originate from fallow:dompurify ← posthog-js protobufjs ← posthog-js → @opentelemetry/otlp-transformer esbuild ← vite@7 vite/vitest ← dev test stack
The change itself is clean; the description just shouldn't pin the repo's existing vulns on the new tool (on a public repo this misleads anyone reading merge history). Please correct the Notes section before merge.
- fallow adds 10 packages, not 523 (npm registry shows
-
[Nit] Stale test count. AC says "342 tests green"; it's 351 now after the merge from
main.
Highlights
- The empty-SARIF fallback + per-run
automationDetails.idstamping is a thoughtful touch — keeps the upload step from ever failing on an empty/multi-run file. fetch-depth: 0+ explicit--base origin/main+security-events: writescoped to the job: all the right plumbing for a report-only SARIF lane.- Correctly held Step 2 (baseline + Stop-hook) for #36 rather than over-scoping.
Verdict
Action: APPROVE
Rationale: Zero blocking code issues. Implementation is correct, dependencies are legit and integrity-pinned, and the CI gate is non-blocking as intended. The only ask is a factual correction to the PR description's dependency/vuln claims (and the stale test count) — neither is a code defect.
Closes #15 (CI lane — Steps 1/3/4). Report-only fallow audit → SARIF → code scanning, non-blocking. Adds fallow devDep (10 lockfile entries, integrity-pinned) + ci.yml step with security-events: write.
Closes #15 (CI lane — Steps 1/3/4). Report-only fallow audit → SARIF → code scanning, non-blocking. Adds fallow devDep (10 lockfile entries, integrity-pinned) + ci.yml step with security-events: write.
Closes #15 (CI lane — Steps 1/3/4).
Adopts fallow as resumelint's standing static-analysis tool, wiring up the CI lane only. Step 2 (baseline triage + Stop-hook registration) depends on #36 and is tracked there.
Changes
package.json—fallow^2.96.0added todevDependencies(+ lockfile).npx fallow --version→fallow 2.96.0(signed)..github/workflows/ci.yml— new report-only stepfallow audit --format sarif, uploaded to GitHub code scanning viagithub/codeql-action/upload-sarif@v3. Non-blocking (|| true); a follow-up flips it to a hard gate once the baseline is clean. Addedsecurity-events: writeto the job so the SARIF upload can publish.Acceptance criteria
npx fallow --versionworks (devDep committed topackage.json+ lockfile).gitignorecontains.fallow/(already present onmain)ci.ymlproduces SARIF and uploads it to code scanning, non-blockingsrc/files modified (tooling + CI only)npm run buildandnpm testpass unchanged (351 tests green).fallow/baseline.json+ findings summary — depends on Add fallow-stop.sh to repo as tracked hook script (prerequisite for #15) #36 (Step 2)fallow-stop.shin.claude/settings.local.json— depends on Add fallow-stop.sh to repo as tracked hook script (prerequisite for #15) #36Notes
fallow auditreturns 0 results locally —auditis the diff/risk gate, not the full dead-code/dupes inventory (that lands in the Add fallow-stop.sh to repo as tracked hook script (prerequisite for #15) #36 baseline), so the gate is green as intended for report-only.fallowitself, its one runtime depdetect-libc, and 8@fallow-cli/*optional per-platform binaries (only the host's installs). All are integrity-pinned. The npm registry lists fallow with a single dependency.npm audit: the 5 advisories on this branch (dompurify,esbuild,protobufjs,vite,vitest) are all pre-existing and unrelated to fallow — they trace toposthog-jsand thevite/vitestdev stack, are dev-only, and never reach the shipped client bundle. fallow contributes zero advisories.🤖 Generated with Claude Code