Repository navigation
Exit with code 2 when a plugin config file fails to load - #1947
Conversation
commit: |
|
Although this is a bug fix, changing the exit code could cause existing services to suddenly start failing after a minor or patch upgrade. It may therefore be safer to ship this in the next major release, but I’ll leave that decision to the maintainers. 🙇 |
webpro
left a comment
There was a problem hiding this comment.
Thanks for the pull request, and for tracking down why the existing test was green.
The gap is real. With the knip.json from this PR in place, main reports the failure and still exits 0:
$ knip --no-progress
ERROR: Error loading vite.config.ts (Cannot find module './missing-bootstrap-output.js')
ERROR: Please fix or visit https://knip.dev/reference/known-issues
$ echo $?
0
Without it the exit code comes from the unused vite devDependency, the unlisted binary and the unresolved import, so the CLI test never proved anything about the load failure. Your reading is correct.
On the release question from your comment: exiting non-zero here has been the intended behavior since 0914bd383 added that test, so this is a bug fix rather than a breaking change and does not need to wait for a major.
The code should be 2 rather than 1 though, and --no-exit-code should not suppress it. A vite.config.ts that fails to load is not a finding about the user's code. It means the analysis input was incomplete, so every remaining result may be wrong. Knip already treats that class this way: a knip.config.ts that fails to load exits 2 even with --no-exit-code, because the error escapes to the catch block in cli.ts, which sets the code regardless of the flag. A plugin config file is the same failure, only non-fatal, and reference/cli.md already describes 2 as "Knip did not run successfully due to bad input or internal error". Anyone who cannot fix the file keeps the documented escape hatch of overriding the plugin's config, which skips the load attempt entirely.
That moves the assertions in load-error.test.ts: 1 becomes 2 in the CLI test and in the cache test, and the --no-exit-code case expects 2 rather than 0, with a name to match.
P2 The exit code table in reference/cli.md and the "Exceptions from config files" section in reference/known-issues.md both need the new behavior: a plugin config file that fails to load exits 2, and --no-exit-code does not suppress it.
The branch also conflicts with main in cli.ts, which now reads options.maxIssues instead of parsing the argument inline. Merging main into the branch updates the pull request without a force push.
| } catch (error) { | ||
| if (!(error instanceof Error)) throw error; | ||
| hasLoadConfigError = true; | ||
| if (isLoaderError(error)) hasConfigLoadErrors = true; |
There was a problem hiding this comment.
P1 This narrows to LoaderError, but the try block also covers plugin.resolveConfig and filterTransitiveDependencies, and both logError calls run for anything they throw. A plugin throwing a plain Error prints the same two lines, discards the cache entry, and still exits 0.
hasLoadConfigError on the line above already carries this fact for the cache. Set the outer flag from that one and drop the isLoaderError import. The cache already treats every error here as a failed load; the exit code should agree.
There was a problem hiding this comment.
Thanks for the detailed feedback! Your approach makes more sense. Applied in 4765a10.
| debugLogArray(wsName, 'Plugin dependencies', () => compact(inputs.map(input => toDebugString(input, rootCwd)))); | ||
|
|
||
| return inputs; | ||
| return { inputs, hasConfigLoadErrors }; |
There was a problem hiding this comment.
P1 Returning a boolean here changes the return type and ripples through build, run and cli. IssueCollector already carries run-level facts of exactly this kind in tagHints and configurationHints, collector is in scope in build, and WorkspaceWorker already takes callbacks such as handleInput. One more callback keeps runPlugins(): Input[] and leaves build and run untouched.
It also puts the fact in results, within reach of main() and the reporters. As submitted, the github-actions reporter has nothing to annotate, so the job turns red with no summary entry explaining why. That annotation can be a separate change.
| if ( | ||
| !args['no-exit-code'] && | ||
| (totalErrorCount > Number(args['max-issues'] ?? 0) || | ||
| hasConfigLoadErrors || |
There was a problem hiding this comment.
P1 This sits inside the !args['no-exit-code'] guard and behind totalErrorCount, so the flag suppresses the load failure and a findings exit can claim the code ahead of it. Both halves matter: incomplete input then reports as a clean run for anyone who has opted out of exit codes, and otherwise reports as ordinary findings.
Set it before this branch instead, after the reporters have run, reading the flag from results once it moves into IssueCollector:
if (hasConfigLoadErrors) {
process.exitCode = 2;
return;
}| @@ -0,0 +1,8 @@ | |||
| { | |||
There was a problem hiding this comment.
P3 This turns the fixture into the clean-report case and drops the case it covered before, a load failure alongside genuine issues. Both are worth keeping: a copy under fixtures/plugin-config/load-error-only carrying this file leaves the original fixture and its assertions intact.
# Conflicts: # packages/knip/src/cli.ts
| export const finalizeConfigurationHints = ( | ||
| results: Results, | ||
| results: Pick<Results, 'counters' | 'issues' | 'includedWorkspaceDirs' | 'configurationHints'>, |
There was a problem hiding this comment.
Results now requires hasConfigLoadErrors, which isn't in ReporterOptions. Narrowed this parameter to the four fields it uses. It can be extended when GitHub Actions annotations are added.
|
Added docs in 81e0474. English isn't my first language, so feel free to tweak the wording as needed. |
webpro
left a comment
There was a problem hiding this comment.
Every finding from the previous round is addressed, and the behavior holds up under direct checks rather than just the diff. A vite.config.ts that loads and then throws inside resolveConfig now exits 2, where the isLoaderError narrowing let it exit 0, and the same fixture with the load failure removed still exits 0. Thanks for the quick turnaround.
Two smaller things and a nit, all inline.
|
|
||
| export const finalizeConfigurationHints = ( | ||
| results: Results, | ||
| results: Pick<Results, 'counters' | 'issues' | 'includedWorkspaceDirs' | 'configurationHints'>, |
There was a problem hiding this comment.
P2 This narrowing works around a gap that is better closed directly. hasConfigLoadErrors reaches Results but not ReporterOptions, which cli.ts assembles by hand, so reporters and preprocessors still cannot see it. Reaching them was half the reason for moving the fact into IssueCollector.
finalizeConfigurationHints is also exported from knip/session and consumed by packages/mcp-server/src/tools.js, so this changes a published signature as a side effect of the type gap.
Adding hasConfigLoadErrors to ReporterOptions and passing it through initialData closes the gap and makes this narrowing unnecessary.
There was a problem hiding this comment.
Updated ReporterOptions to include hasConfigLoadErrors and restored the Results parameter type. After merging main, toReporterOptions() passes the flag through to reporters and preprocessors, applied it in 4c75139
| } catch (error) { | ||
| if (!(error instanceof Error)) throw error; | ||
| hasLoadConfigError = true; | ||
| this.handleConfigLoadError(); |
There was a problem hiding this comment.
P2 Nothing pins this line. Restoring if (isLoaderError(error)) in front of it leaves all 1096 tests passing, and that guard is the behavior the fix exists to remove.
A config file that loads and then throws inside resolveConfig covers it. A vite.config.ts of export default { test: { projects: 42 } } is enough:
ERROR: Error loading vite.config.ts (config.test.projects is not iterable)
That exits 2 on this branch and 0 with the guard back.
| "ignoreBinaries": ["vite"], | ||
| "ignoreIssues": { | ||
| "vite.config.ts": ["unresolved"], | ||
| "missing-bootstrap-output.js": ["files"] |
There was a problem hiding this comment.
P3 This entry does nothing here. It carries the cache recovery test in the sibling fixture, which writes missing-bootstrap-output.js partway through; nothing creates that file in this fixture. Removing the line leaves the output identical.
Include hasConfigLoadErrors in ReporterOptions and the data assembled by the CLI. Restore the Results parameter type for finalizeConfigurationHints and pass all required fields through its caller.
|
Thank you! 🌱 |
|
🚀 This pull request is included in v6.35.1. See Release 6.35.1 for release notes. Using Knip in a commercial project? Please consider becoming a sponsor. |
… config knip loads plugin configs through its own CJS transpile, which cannot call ESM-only @vitejs/plugin-react, so loading electron.vite.config.js has been failing all along. knip >= 6.35.1 turns that failure into exit code 2 (webpro-nl/knip#1947), which breaks the Check workflow on the 6.36.0 bump. Disable both the vite and electron-vite plugins (electron-vite auto-detects the same config file) and add the config as an entry so @vitejs/plugin-react and ./build/global-externals.js are still counted as used. Co-authored-by: Leo Capuano <60476114+leo-capvano@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When a plugin config file (e.g.
vite.config.ts) fails to load, Knip logs the error and continues, but the exit code previously ignored it.Before:
--no-exit-codeAfter: every case above exits
2.Analysis still runs to completion and findings are still reported.
--no-exit-codecontinues to suppress lint findings, but not config load errors, consistent with how Knip already handles failures in its own configuration.The existing CLI test was only green because its fixture also had unrelated findings. A new
load-error-onlyfixture verifies that a config load error alone exits2.Docs are updated accordingly.