Skip to content

fix(migrate): report lint rules dropped during oxlint sanitization - #2542

Open
m0g3r wants to merge 1 commit into
voidzero-dev:mainfrom
m0g3r:fix/2231-warn-dropped-lint-rules
Open

m0g3r wants to merge 1 commit into
voidzero-dev:mainfrom
m0g3r:fix/2231-warn-dropped-lint-rules

Conversation

@m0g3r

@m0g3r m0g3r commented Aug 23, 2026

Copy link
Copy Markdown

Refs #2231. This fixes the "silently" half of that issue and deliberately leaves the namespace-resolution half open — see What this leaves for #2231 below.

Problem

When vp migrate merges .oxlintrc.json into vite.config.ts, sanitizeMigratedOxlintConfig removes every rule whose namespace no surviving plugin contributes. Dropped jsPlugins and dropped plugins each produce a warning; dropped rules produced nothing at all.

The case #2231 reports makes it concrete. A local plugin is kept by the sanitizer (partitionJsPlugins passes ./-style specifiers through, because Oxlint resolves them itself), but deriveJsPluginNamespace has only the path to work with and returns ./lint/kumo.js verbatim. No rule key can ever match that, so every kumo/* rule is filtered out of the migrated config and the user is told nothing.

Why the rules still have to be dropped

I checked what Oxlint actually does before assuming the filtering could simply be relaxed, using the bundled oxlint 1.79.0 on a scratch project.

A rule under a namespace no plugin backs is not a warning — it stops Oxlint from starting at all:

$ cat .oxlintrc.json
{ "jsPlugins": ["./lint/kumo.js"], "rules": { "kumo/no-foo": "error", "ghostplugin/some-rule": "error" } }
$ oxlint a.js
Failed to parse oxlint configuration file.

  x Plugin 'ghostplugin' not found
$ echo $?
1

So keeping unattributable rules would trade a silent config loss for vp lint failing outright on the whole project. Dropping them is the right behaviour; doing it without a word is not.

Two other things that same probe established, both of which shape the fix:

  • Oxlint really does take a local plugin's namespace from its meta.name — with meta: { name: 'kumo' }, kumo/no-foo resolves and fires (a.js:1:7: error kumo(no-foo): ...). The migrator has no way to know that from the path alone.
  • The jsPlugins object form is a working escape hatch, and name sets the namespace: { "name": "kumo", "specifier": "./lint/kumo.js" } resolves identically, and { "name": "renamed", ... } re-namespaces the same plugin to renamed/no-foo. The migrator already handles this form correctly (jsPluginsToNamespaces reads entry.name), so it is a real remedy to point users at.

Changes

  • filterRulesAgainstNamespaces records every key it removes into a caller-supplied set.
  • sanitizeMigratedOxlintConfig accumulates those keys across the base config and every override, then warns once, naming the rules and pointing at the { name, specifier } form.

Behaviour is otherwise unchanged: the same rules are dropped as before, and nothing new is kept.

What this leaves for #2231

The issue suggests resolving the real namespace by importing the plugin and reading meta.name, which is exactly what Oxlint does. I have not done that here, because it is a design decision I do not think a contributor should make unilaterally:

  • sanitizeMigratedOxlintConfig and its caller mergeViteConfigFiles are synchronous, so import() means making that chain async.
  • It would have vp migrate execute arbitrary user plugin code, immediately after the ESLint cleanup has removed packages that plugin may well import.

Alternatives that avoid both — a synchronous require of the plugin, statically extracting meta.name, or having the migrator rewrite local jsPlugins into the object form so the namespace is recorded explicitly — each trade differently. Happy to implement whichever you prefer, here or as a follow-up. In the meantime this at least means the rules no longer vanish silently.

Testing

Three tests added next to the existing sanitizer tests in packages/cli/src/migration/__tests__/migrator.spec.ts: the #2231 local-plugin case, an override-rules case, and a negative case asserting no warning when nothing is dropped.

Verified in both directions — with migrator/eslint.ts reverted to main and the tests kept, the two positive tests fail (AssertionError: expected undefined to be defined); with the change applied they pass. The negative test passes either way, as it should.

Checks actually run on this branch:

  • vitest run packages/cli/src/migration — 411 passed across 10 files (408 on main here, plus these 3)
  • vitest run (full unit suite) — 1025 passed, 1 skipped; 7 pre-existing snapshot failures in packages/prompts/src/__tests__/render.spec.ts that also fail on an unmodified tree in this environment
  • vp lint --type-aware --type-check — no diagnostics in either touched file
  • vp fmt --check — both touched files clean

One caveat on the lint run, in case anyone reproduces it: pnpm build generates an untracked packages/cli/src/migration/versions.ts, which in a local build carries only { vite, vitest }. While it is present, type-checking reports two errors in migrator/eslint.ts:139-141 (an unused @ts-expect-error and a missing versions.oxlint) that have nothing to do with this change — that code deliberately resolves versions.js from dist/ at runtime. With that generated file removed, both errors disappear and the only remaining diagnostics are 19 in the unbuilt docs/ workspace.

Not run: the PTY snapshot suite and ecosystem e2e. This changes no CLI command output beyond adding a migration warning, which is reported through MigrationReport and asserted directly in the tests.

AI assistance

Claude Opus 5 wrote the implementation, the tests, the oxlint probes, and this description. The change is agent-authored and has not had a separate human review. Every result quoted above is from an actual run on this branch, not an estimate.


Generated by Claude Code

@netlify

netlify Bot commented Aug 23, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for viteplus-preview canceled.

Name Link
🔨 Latest commit af44ac9
🔍 Latest deploy log https://app.netlify.com/projects/viteplus-preview/deploys/6a8b14f9306dbe000864082f

`sanitizeMigratedOxlintConfig` removes rules whose namespace no surviving
plugin contributes. That removal is necessary — Oxlint refuses to start on a
rule naming a plugin it cannot resolve ("Plugin 'x' not found") — but it
happened silently, while dropped plugins and jsPlugins already warned.

A local jsPlugin makes this visible. Its real namespace comes from the
plugin's `meta.name`, which cannot be derived from a path specifier such as
`./lint/kumo.js`, so every `kumo/*` rule was filtered out of the migrated
config with no output at all.

Collect the removed rule keys and warn with them, pointing at the object
form (`{ name, specifier }`) that lets a user name the namespace explicitly.

Refs voidzero-dev#2231
@m0g3r
m0g3r force-pushed the fix/2231-warn-dropped-lint-rules branch from af44ac9 to a6f1298 Compare September 28, 2026 13:01
fengmk2 pushed a commit that referenced this pull request Oct 15, 2026
<!-- claude-projects-attribution: {"github_login":"Brooooooklyn"} -->
_Requested by **Brooklyn** · [project
thread](https://claude.ai/code/project/chan_018s9RxRdtH424j2EhXdzV3N?thread=cmsg_018s9RxRdtH424j2EhXdzV3NQxbeR8Pt7qaqSQwoq9rEFo)_

<!-- routine:prune-tests:duplicate-shell-and-helper-tests -->

Before: `vp_global_cli` and `vp_pm_cli` carried Rust unit tests that
could not fail when the product broke: an `assert!(true)` in
`create.rs`, four `env use` shell-detection tests that repeat
`commands::shell` tests for code `env use` only imports, and three tests
of test-only helpers. `upgrade_check.rs` had 20 hand-unrolled lookup
tests.

After: those 8 tests are gone and the 20 lookup tests are 2 table-driven
tests that keep every row and comment, with the failing row named in the
assertion message. Only test code changes: every hunk is inside a
`#[cfg(test)]` module or the `#[cfg(test)]`-only
`resolution::test_utils` module.

How: each candidate was defended by mutating product code with the most
realistic regression only it would catch, running the crate's tests, and
reverting. No deleted test failed alone on any mutation.

### Totals

| | Before (origin/main c04bcf6) | After |
|---|---|---|
| Files touched | – | 5 |
| `vp_global_cli` cases collected (Linux) | 502 (+2 ignored) | 479 (+2
ignored) |
| `vp_pm_cli` cases collected | 923 (+2 ignored) | 920 (+2 ignored) |

- Cases deleted: **8** of 1,425 collected in the two touched crates.
Repo-wide: about 2,432 `#[test]`/`#[tokio::test]` attributes in tracked
Rust (static count) and 1,492 static `it`/`test` calls in 101 JS spec
files (not collected; Vitest needs a full `packages/core` build).
- Cases folded: **20 into 2**, every row and comment kept.
Runner-collected total: −26.
- Net lines: **−159** (+42 / −201).
- Runtime saved: about 0.03 s (the 8 deleted cases each report 0.000 s;
the 20 folded cases summed 0.028 s against 0.011 s for the 2
replacements).
- Test-binary time on this shared machine: `vp_global_cli` 3.13 s → 3.32
s, `vp_pm_cli` 27.81 s → 29.06 s (noise).

### Per file

1. **`crates/vp_global_cli/src/commands/create.rs` ›
`test_create_command_module_exists`**
   - Class: tautology (`assert!(true)`).
- Best defense: M1 (delegate `"new"` instead of `"create"`) passes all
502 tests, so this test cannot fail. The whole `mod tests` goes.
- Coverage gap (pre-existing, unchanged by this PR): no unit test covers
`vp create` delegation. It may be covered by the PTY snapshot suite,
which was not run.
2. **`crates/vp_global_cli/src/commands/env/use.rs` ›
`test_detect_shell_vp_shell_{powershell,fish,nu}`,
`test_detect_shell_posix_default`**
- Class: duplicates of stronger tests. `detect_shell` /
`Shell::from_str` live in `commands/shell.rs`; `env use` only imports
them.
- Still covered: `fish` is byte-identical to
`commands::shell::tests::test_detect_shell_vp_shell_fish`; `nu` to
`test_detect_shell_vp_shell_explicit`; lowercase `powershell` by
`test_shell_from_str` and `test_detect_shell_vp_shell_case_insensitive`;
`posix_default` by `test_detect_shell_defaults_without_vp_shell`.
- Best defenses M2–M5: each failed the nominee together with at least
one `commands::shell` test.
- The `use vp_shared::env_vars;` import is now `#[cfg(windows)]`, since
only the three Windows-only tests in the module still use it.
3. **`crates/vp_pm_cli/src/resolution/test_utils.rs` ›
`parses_args_without_a_program_name`,
`parses_subcommands_without_a_program_name`**
   - Class: test tooling testing test tooling.
- Best defense: M7 fails 122 tests and M8 fails 17; every resolution
parser test exercises these helpers.
4. **`crates/vp_pm_cli/src/resolution/commands/list.rs` ›
`list_args_helper_keeps_pattern`** (and its test-only `list_args`
helper, which had no other caller)
- Class: test tooling testing test tooling; it only checked struct
construction.
- Best defenses: M9 fails only
`test_list_with_pattern_and_pass_through_args`; M10 fails
`test_parser_accepts_short_dev_and_global_flags` and
`cli::tests::projects_managed_global_commands`. The nominee passes both.

### Folds (`crates/vp_global_cli/src/upgrade_check.rs`)

- The 9 `is_newer_version_*` tests → `is_newer_version_compares_semver`
(12 rows, original comments kept, one comment added for the `0.0.0`
dev-build row).
- The 11 `should_run_for_*` / `should_not_run_for_*` tests →
`should_run_for_command_skips_excluded_and_machine_readable_commands`
(11 rows).
- M11, M12 and M13 still fail the folded tables.

### Kept after defense — do not re-nominate

None: no nominee survived its defense.

### Case counts

| file | before | after | delta | expected delta |
|---|---|---|---|---|
| vp_global_cli `commands/create.rs` | 1 | 0 | −1 | −1 |
| vp_global_cli `commands/env/use.rs` (Linux) | 16 | 12 | −4 | −4 |
| vp_global_cli `upgrade_check.rs` | 36 | 18 | −18 | −18 (20 folded into
2) |
| vp_pm_cli `resolution/commands/list.rs` | 45 | 44 | −1 | −1 |
| vp_pm_cli `resolution/test_utils.rs` | 2 | 0 | −2 | −2 |
| **Crates total** | 1,425 | 1,399 | −26 | −26 |

**0 new failures** (0 failed and 4 ignored, before and after).

### Mutations

Each was temporary and reverted (`git status` clean afterwards).

1. M1 `create.rs:15`: delegate `"new"` instead of `"create"`. Failed:
none.
2. M2 `shell.rs:30`: `"fish"` → `"fishshell"`. Failed: `use::…_fish`,
`shell::test_shell_from_str`, `shell::…_fish`.
3. M3 `shell.rs:31`: `"nu" | "nushell"` → `"nushell"`. Failed:
`use::…_nu`, `shell::…_explicit`, `shell::test_shell_from_str`.
4. M4 `shell.rs:32`: drop `"powershell"`. Failed: `use::…_powershell`,
`shell::…_case_insensitive`, `shell::test_shell_from_str`.
5. M5 `shell.rs:54`: default always `Cmd`. Failed:
`use::…_posix_default`, `shell::…_defaults_without_vp_shell`,
`shell::…_invalid_vp_shell_falls_back_to_default`.
6. M6 `shell.rs:49`: upper-case the value. Failed: none (equivalent
mutation; `from_str` lowercases).
7. M7 `test_utils.rs:70`: `test_argv` no longer prepends a program name.
Failed: 122 tests, including both nominees.
8. M8 `test_utils.rs:57`: `parse_subcommand` gets raw args. Failed: 17
tests (`config`, `dist_tag`, `owner`, `stage`, `token`, plus the
nominee).
9. M9 `list.rs:78-80`: pnpm drops the pattern. Failed:
`test_list_with_pattern_and_pass_through_args` only.
10. M10 `list.rs:11`: `pattern` becomes `#[arg(long)]`. Failed:
`test_parser_accepts_short_dev_and_global_flags`,
`cli::tests::projects_managed_global_commands`.
11. M11 `upgrade_check.rs:170` (after the fold): `>` → `>=`. Failed:
`is_newer_version_compares_semver`.
12. M12 `upgrade_check.rs:338` (after the fold): `Upgrade` removed from
the excluded commands. Failed:
`should_run_for_command_skips_excluded_and_machine_readable_commands`.
13. M13 `upgrade_check.rs:165` (after the fold): the `0.0.0` early
return removed. Failed: `is_newer_version_compares_semver`.

### What ran and what did not

- Ran, at origin/main and on this branch: `cargo test -p vp_global_cli`
and `cargo test -p vp_pm_cli` with `--report-time`; `cargo clippy -p
vp_global_cli -p vp_pm_cli --all-targets --all-features -- --deny
warnings` with the `just lint` allow-list; `cargo fmt --all -- --check`;
`git diff --check`. All pass.
- Not run: the full workspace `just test` / `just lint` / `just check`;
Windows targets (none installed here, so the `#[cfg(windows)]` import
gate was reviewed but not compiled; Windows CI will compile it); the
JS/Vitest suite; the PTY snapshot suite.

### Excluded pull requests

55 open PRs touch 737 files between them; none of the 5 files here is
among them. #3055 #3052 #3051 #3049 #3047 #3046 #3044 #3040 #3038 #3037
#3020 #3018 #3014 #3012 #3009 #3008 #3000 #2986 #2981 #2966 #2936 #2922
#2900 #2863 #2862 #2810 #2792 #2740 #2721 #2699 #2695 #2574 #2557 #2552
#2549 #2543 #2542 #2541 #2483 #2475 #2463 #2462 #2461 #2456 #2428 #2418
#2357 #2267 #2125 #1958 #1783 #1782 #1614 #1517 #1180.

### Age check (cutoff: 30 days, 2026-09-14)

First-added dates from `git log -S`, followed across the `vite_*` →
`vp_*` crate rename.

| Test | First added |
|---|---|
| `test_create_command_module_exists` | 2026-02-10 (a29ac83, #570) |
| `test_detect_shell_posix_default` | 2026-02-07 (194c0dc, #524) |
| `test_detect_shell_vp_shell_{powershell,fish,nu}` | 2026-05-24
(53ae721, #1658) |
| `test_utils` tests and `list_args_helper_keeps_pattern` | 2026-08-03
(c1574dd, #2140) |
| The 20 folded `upgrade_check` tests | 2026-03-30 (db75675) |

None was added for a bug or incident, and none is in a permission,
security or entitlement suite.

### Checks

| Check | Before | After |
|---|---|---|
| Tests `vp_global_cli` | 502 pass | 479 pass |
| Tests `vp_pm_cli` | 923 pass | 920 pass |
| Clippy (deny warnings) | pass | pass |
| rustfmt | – | pass |
| `git diff --check` | – | clean |

To revert: `git revert 687935d`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01PxVsmvB9xfZKfVnUonY8kf

---
_Generated by [Claude
Code](https://claude.ai/code/session_01PxVsmvB9xfZKfVnUonY8kf)_

Co-authored-by: Claude <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant