Skip to content

Check JSON response for knip to avoid false positives#7465

Merged
gonzaloriestra merged 1 commit into
mainfrom
solid-knip
May 5, 2026
Merged

Check JSON response for knip to avoid false positives#7465
gonzaloriestra merged 1 commit into
mainfrom
solid-knip

Conversation

@gonzaloriestra

Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

The Knip (unused code check) job can pass silently in CI even when knip never actually analyzes the codebase. We hit this in PR #7463: the job exited 0 with no output and was reported as green, even though running pnpm knip locally on the exact same SHA reproducibly fails with two unused dependencies. A false-green knip job lets real findings slip into main.

WHAT is this pull request doing?

  • Adds bin/run-knip-ci.js, a small Node script that runs knip with --reporter json and validates the output before trusting the exit code.
    • Knip with --reporter json always emits a structured object on success ({"files":[],"issues":[]}), so an empty / unparseable stdout is a reliable signal that knip didn't actually run — in which case the script fails the job with a ::error:: annotation.
    • When knip reports findings, the script re-runs it with the default reporter so the human-readable report still appears in the build log.
  • Simplifies the Run knip step in .github/workflows/tests-pr.yml to a single line: node bin/run-knip-ci.js.

How to test your changes?

I verified all three relevant scenarios locally against the real workspace:

  1. Clean run (current main): node bin/run-knip-ci.js exits 0 with no output.
  2. Findings: temporarily remove @graphql-typed-document-node/core from knip.workspaces["packages/app"].ignoreDependencies in package.json → script re-runs knip in default reporter mode, prints Unused dependencies (1) ..., and exits 1.
  3. Silent-pass bug (the regression we're guarding against): temporarily replace the root scripts.knip with node -e "" so pnpm knip exits 0 with empty stdout → script prints ::error::knip exited N but produced no parseable JSON output. Failing CI to avoid a false green. and exits 1.

You can reproduce 2 and 3 by editing package.json as described above, running node bin/run-knip-ci.js, and reverting the edit.

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing, so I've added a changelog entry with pnpm changeset add

@github-actions github-actions Bot added the no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users. label May 5, 2026
@gonzaloriestra gonzaloriestra temporarily deployed to breaking-change-approval May 5, 2026 09:25 — with GitHub Actions Inactive
@gonzaloriestra gonzaloriestra marked this pull request as ready for review May 5, 2026 09:49
@gonzaloriestra gonzaloriestra requested a review from a team as a code owner May 5, 2026 09:49

gonzaloriestra commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

Merge activity

  • May 5, 10:34 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • May 5, 10:34 AM UTC: @gonzaloriestra added this pull request to the GitHub merge queue with Graphite.

@gonzaloriestra gonzaloriestra added this pull request to the merge queue May 5, 2026
Merged via the queue into main with commit 04eac90 May 5, 2026
27 of 29 checks passed
@gonzaloriestra gonzaloriestra deleted the solid-knip branch May 5, 2026 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants