Skip to content

feat(migrate): preserve dynamic Oxlint and Oxfmt configs - #2483

Open
m0g3r wants to merge 6 commits into
voidzero-dev:mainfrom
m0g3r:fix/2430-dynamic-ox-config-migration
Open

feat(migrate): preserve dynamic Oxlint and Oxfmt configs#2483
m0g3r wants to merge 6 commits into
voidzero-dev:mainfrom
m0g3r:fix/2430-dynamic-ox-config-migration

Conversation

@m0g3r

@m0g3r m0g3r commented Aug 17, 2026

Copy link
Copy Markdown

Closes #2430

Problem

vp migrate detected and inlined only the JSON forms of the Oxlint and Oxfmt configs. A project configured with oxlint.config.ts or oxfmt.config.mts had its configuration silently dropped — the file stayed on disk but nothing referenced it after migration.

Changes

  • Detection now covers the dynamic .ts / .mts forms alongside the JSON ones, matching the candidate lists the bundled oxlint and oxfmt binaries themselves search for.
  • The existing tsdown config merger is generalized to target lint and fmt, so dynamic configs are preserved and imported into vite.config.* the same way dynamic tsdown configs already are.
  • Those configs' bare oxlint and oxfmt runtime imports are rewritten to complete vite-plus/lint and vite-plus/fmt subpaths.

That last part is what makes the rest usable. Migration removes the direct oxlint / oxfmt dependencies, so under a strict pnpm layout a preserved config that still imports them bare fails to resolve. This is the same failure #701 hit in its e2e run after merge. The new public subpaths and the migration snapshot cover it.

Testing

  • cargo test -p vp_migration — 315 passed
  • vitest run packages/cli/src/migration packages/cli/src/__tests__/exports-map.spec.ts — 409 passed across 11 files
  • New PTY snapshot fixture migration_dynamic_oxc_configs, including rerun idempotency
  • cargo clippy with warnings denied, Rust and JS formatting, and targeted type-aware lint/type checks

Re-verified green after merging current main, which had moved 8 commits since the branch was cut.

AI assistance

Claude Opus 5 wrote the implementation, the tests, and this description. The change is agent-authored and has not had a separate human review. The test results quoted above are from actual runs on this branch, not estimates.

m0g3r added 2 commits August 17, 2026 23:10
`vp migrate` detected and inlined only the JSON forms of the Oxlint and Oxfmt
configs, so a project using `oxlint.config.ts` or `oxfmt.config.mjs` silently
lost its configuration.

Detection now covers the dynamic `.ts`/`.mts`/`.cts`/`.js`/`.mjs`/`.cjs` forms,
and the existing tsdown merger is generalized to target `lint` and `fmt` so
those files are preserved and imported into `vite.config.*` the same way tsdown
configs already are.

Their bare `oxlint` and `oxfmt` runtime imports are rewritten to complete
`vite-plus/lint` and `vite-plus/fmt` subpaths, which migration otherwise leaves
unresolvable under strict pnpm layouts once the direct packages are removed.

Closes voidzero-dev#2430
@netlify

netlify Bot commented Aug 17, 2026

Copy link
Copy Markdown

Deploy Preview for viteplus-preview canceled.

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

@socket-security

socket-security Bot commented Aug 17, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addednpm/​oxfmt@​0.1.0511005792100

View full report

@fengmk2 fengmk2 self-assigned this Aug 18, 2026
@fengmk2

fengmk2 commented Aug 18, 2026

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3853a39fd5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/vp_migration/src/import_rewriter.rs Outdated
Comment on lines +887 to +891
// Reuse an existing default import when a partially migrated config
// already has one. Otherwise prepend it using JavaScript extensions for
// TypeScript source files, matching TypeScript module resolution.
let import_prefix = format!("import {import_name} from");
if !vite_config_content.contains(&import_prefix) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Choose a collision-free name for the injected config import

If vite.config.ts already declares oxlintConfig or oxfmtConfig without the exact text import <name> from—for example a local constant, named import alias, type-only import, or multiline default import—this branch prepends another binding with the same name and leaves the migrated config syntactically invalid. Detect bindings through the AST and either reuse a compatible import or generate a unique local identifier before inserting the config property.

Useful? React with 👍 / 👎.

Comment on lines +280 to +284
mergeDynamicConfigFile(
projectPath,
viteConfig,
configs.oxlintConfig,
'oxlintConfig',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Retain type-check defaults for dynamic lint configs

When oxlint.config.ts does not explicitly set options.typeAware or options.typeCheck, this branch inserts lint: oxlintConfig; the subsequent injectLintTypeCheckDefaults() sees that top-level key and returns without adding either default. This contradicts the migration behavior applied to JSON and config-less projects and silently makes vp check stop performing the type-aware lint and TypeScript checks that vp migrate enables by default. Merge the missing option defaults around the imported config while preserving explicit values and the baseUrl exception.

AGENTS.md reference: AGENTS.md:L185-L185

Useful? React with 👍 / 👎.

Comment on lines +283 to +287
configs.oxlintConfig,
'oxlintConfig',
'lint',
silent,
report,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add Svelte rune globals to dynamic lint configs

When a dynamic Oxlint config has an override targeting .svelte files but omits explicit rune globals, this path bypasses ensureSvelteRuneGlobals(), which is still applied to migrated JSON configs to prevent $state, $derived, and the other built-in runes from being reported as undefined. Importing the object unchanged therefore introduces false-positive lint failures specifically for Svelte projects using the newly supported config format; apply the same runtime normalization before assigning the imported config.

AGENTS.md reference: AGENTS.md:L185-L185

Useful? React with 👍 / 👎.

Comment on lines +280 to +284
mergeDynamicConfigFile(
projectPath,
viteConfig,
configs.oxlintConfig,
'oxlintConfig',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Rewrite imported configs even when gitignored

If a generated or machine-local oxlint.config.ts/oxfmt.config.ts is listed in .gitignore, detectConfigs() still selects it and this branch imports it into vite.config.ts, but the later rewriteAllImports() walk respects gitignore and never changes its bare oxlint/oxfmt helper import. Since migration removes those direct dependencies, strict pnpm/Yarn layouts then fail while loading the newly imported config. Rewrite the detected file directly regardless of ignore rules, or avoid removing its dependency when it was skipped.

Useful? React with 👍 / 👎.

@simulacre7

Copy link
Copy Markdown
Contributor

I was about to pick up #2430, found this PR already does it (nice!), and had a few empirical probes lying around that seem useful as review input. All verified against the bundled oxlint 1.79.0 / oxfmt 0.64.0 binaries on this repo's checkout.

1. PR description vs code: extension list. The description says detection covers .ts / .mts / .cts / .js / .mjs / .cjs, but the detector adds only oxlint.config.ts / oxlint.config.mts (and the oxfmt equivalents) — and the code is the correct side: the native binaries' candidate lists contain only the .ts/.mts forms, and an oxlint.config.js on disk is silently ignored (rules from it don't apply; default severities stay). Probably just worth fixing the description so nobody requests the extra extensions in review.

2. Post-migration shadowing (may deserve an info line in the summary). With both oxlint.config.ts and a vite.config.ts containing a lint block present, a direct oxlint invocation (editor integrations, plain CLI) uses oxlint.config.ts and silently ignores the lint block:

$ cat oxlint.config.ts   # rules: { 'no-debugger': 'error' }
$ cat vite.config.ts     # lint: { rules: { 'no-console': 'error' } }
$ oxlint a.js            # a.js has `debugger;` and `console.log(1)`
a.js:1:1: error eslint(no-debugger): ...        # from oxlint.config.ts
(no no-console diagnostic — the vite.config lint block is ignored)

Right after this migration the two are consistent because lint: imports the same object, so this is fine as-is. But a user who later edits the lint: value inline in vite.config.ts (e.g. lint: { ...oxlintConfig, rules: {...} }) gets a split brain: vp lint sees the edit, direct oxlint runs don't. Since the tsdown path always emits a "please manually merge …" info line, a similar one-liner here ("oxlint.config.ts remains the config used by direct oxlint invocations") might save some confusion.

3. Edge: both .oxlintrc.json and oxlint.config.ts present. oxlint itself hard-errors on that state ("Only one of .oxlintrc.json and oxlint.config.ts is allowed per directory"), so such projects are already broken — but the migrator's first-match detection picks the rc file, inlines + deletes it, and leaves oxlint.config.ts on disk unreferenced, where it then shadows the freshly inlined lint block per (2). A warning when both forms are detected would make the outcome self-explanatory. Definitely a nit.

@m0g3r

m0g3r commented Aug 23, 2026

Copy link
Copy Markdown
Author

Thanks — these are useful, and (1) was a real error in the description rather than the code.

1. Confirmed and fixed. detector.ts lists only .oxlintrc.json, .oxlintrc.jsonc, oxlint.config.ts, oxlint.config.mts (and the oxfmt equivalents), which matches what the bundled binaries actually search for. The description claimed .cts / .js / .mjs / .cjs as well, and also referred to an oxfmt.config.mjs. Both are corrected now, so the description matches the diff.

2. and 3. Both read as genuine, and I'd rather a maintainer decide whether they belong in this PR or a follow-up, since each widens the scope past what #2430 asked for. Happy to add either on request:

  • (2) an info line noting oxlint.config.ts remains the config used by direct oxlint invocations, mirroring the existing "please manually merge …" line on the tsdown path;
  • (3) a warning when both .oxlintrc.json and oxlint.config.ts are present, since first-match detection inlines and deletes the rc file and leaves the dynamic config on disk unreferenced.

Worth noting for (3) that oxlint hard-errors on that combination, so any project hitting it is already broken before migration — the warning would make the outcome legible rather than change it.

@KTrain5169

KTrain5169 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

(not a maintainer, but the person who created the linked issue)

in the cases of 2 and 3, just my two cents of "what makes the most sense":

  1. "Please manually merge..." shouldn't be the wording, it should say "We strongly recommend manually merging oxlint.config.ts immediately to ensure alignment with IDE integrations". The stronger wording compared to tsdown is intentional because tsdown doesn't have an editor integration of any kind (at least for now), so it's less important. This stronger wording should apply to oxfmt.config.ts as well.
  2. Since oxlint already hard errors on multi-config situations it should make intuitive sense for vp migrate to hard error there as well. However, there can also be the case of matching others' migrations flow for this edge case. I think a review of how other migration flows handle this is a good idea, but that is outside the scope of both the issue and the PR.

…imports

The Oxc rewrite pass was applied unconditionally, unlike the vite, vitest
and tsdown passes, which each honor the package-level `SkipPackages` flags.
A workspace package that intentionally declares `oxlint` or `oxfmt` as its
own runtime or peer dependency therefore had its library sources rewritten
to `vite-plus/lint` / `vite-plus/fmt` even though `rewritePackageJson` keeps
that dependency, so published consumers could receive an undeclared import.

Split the combined Oxc rule set into separate oxlint and oxfmt rule sets and
gate each on its own skip flag, computed from `peerDependencies` and
`dependencies` exactly like the existing three. The fast pre-filter honors
the same flags so a fully skipped package short-circuits before parsing.

Projects that do not declare either package are unaffected.

m0g3r commented Aug 24, 2026

Copy link
Copy Markdown
Author

Pushed fbd65ee for the Codex review. Triage of all five findings below, separating what I fixed from what I verified but deliberately left alone.

Fixed — P1 "Preserve declared Oxc package boundaries during rewrites"

Confirmed, and it is this PR's regression. The vite, vitest and tsdown passes in rewrite_import_content_full() are each gated on a SkipPackages flag; the new Oxc pass was applied unconditionally, and content_may_need_rewriting() matched oxlint/oxfmt with no skip check either. A package that declares oxlint or oxfmt in its own dependencies/peerDependencies keeps that dependency through rewritePackageJson, so its sources were rewritten to vite-plus/lint / vite-plus/fmt anyway.

The fix splits REWRITE_OXC_RULES into separate oxlint and oxfmt rule sets and gates each on its own flag, computed from peerDependencies/dependencies exactly like the existing three, at both the pre-filter and the apply site. Keeping them separate means a package that declares only one of the two still gets the other rewritten.

cargo test -p vp_migration goes 315 → 320. Verified in both directions: reverting only the new guards fails exactly the three skip tests, and test_oxc_imports_still_rewritten_when_not_declared passes either way, so the default path is unchanged.

Confirmed, not fixed here — P1 "Retain type-check defaults for dynamic lint configs" and P2 "Add Svelte rune globals"

Both are real, and they are two symptoms of one gap rather than two independent bugs. injectConfigDefaults() returns early when the top-level key already exists, and mergeDynamicConfigFile() has just inserted lint: oxlintConfig, so injectLintTypeCheckDefaults() no-ops. The same asymmetry drops two more steps Codex didn't mention: the dynamic branch also skips sanitizeMigratedOxlintConfig() and ensureVitePlusImportRuleDefaults().

The common cause: everything the JSON branch does, it does by mutating a parsed object before writing it back. When the config is an opaque imported binding there is nothing to mutate, so this can't be fixed by "merging the missing defaults around" the import — it needs either generated wrapper code at the call site, or an explicit warning that the defaults were not applied. That's a design choice with user-visible output either way, so I'd rather you pick. Happy to implement whichever, in this PR or a follow-up.

Confirmed but pre-existing — P2 "Choose a collision-free name for the injected config import"

Real: merge_dynamic_config_content() guards with the substring test content.contains("import oxlintConfig from"), which misses a local const, an aliased or type-only import, a namespace import, and a multiline default import — any of which yields a duplicate binding and invalid TypeScript.

Worth noting it is not introduced here: the same function already backs merge_tsdown_config (tsdownConfigpack), so the bug predates this PR, though this PR does widen exposure to two more identifiers. The repo already has the right machinery in has_conflicting_lazy_plugins_binding() — AST walk plus the destructured, multi-declarator and aliased-import regexes — but it is hard-coded to the literal lazyPlugins. Generalizing it to an arbitrary identifier is a shared-code refactor that changes the tsdown path too, so I've kept it out of this PR rather than widen it unasked. Say the word if you'd prefer it folded in.

Unverified — P2 "Rewrite imported configs even when gitignored"

Half-confirmed only. detectConfigs() uses plain fs.existsSync with no ignore awareness, so it will indeed select a gitignored oxlint.config.ts. I could not exercise the rewriteAllImports() side in this environment, so I'm flagging it as unconfirmed rather than asserting the full failure mode.

Validation limits

This sandbox can't complete just init (the gitignored vite/ and rolldown/ checkouts that sync-remote creates aren't present), so the NAPI binding and the JS suite couldn't be built here. What I actually ran for this commit: cargo test -p vp_migration (320 passed), cargo clippy -p vp_migration --all-targets (clean), and cargo fmt. The TypeScript suite and the PTY snapshot fixture were not re-run — the change is Rust-only and adds no new CLI output, but CI is the real check on those.


@KTrain5169 — thanks, that's exactly the steer I was asking for.

On (2): agreed on the stronger wording and on applying it to oxfmt.config.ts too, and the editor-integration rationale for making it stronger than the tsdown line is convincing. I've deliberately not pushed it in this commit, because it adds a line to the success path and the migration_dynamic_oxc_configs PTY snapshot records that output — changing it without being able to regenerate the snapshot here would just turn CI red. It needs an environment that can run UPDATE_SNAPSHOTS=1 just snapshot-test, so I'd like a maintainer's nod on the wording before spending that round trip.

On (3): agreed, and I'll leave it out. Your reasoning matches what I found — projects in that state are already broken because oxlint hard-errors on it — and surveying how other migrators handle the case is genuinely outside this issue.


Generated by Claude Code

@simulacre7

Copy link
Copy Markdown
Contributor

Both dispositions look right from where I sit — (1) matches the code now, and leaving (3) out is consistent with what I reproduced (oxlint hard-errors on that combination before migration ever sees it, so the warning would only narrate an already-broken state).

On (2): once a maintainer settles the wording, if the snapshot round trip is the blocker — I have a local checkout that runs the PTY suite fine, so I'm happy to run UPDATE_SNAPSHOTS=1 just snapshot-test migration_dynamic_oxc_configs against your branch and post the resulting .md diff here to save you the environment dance.

m0g3r commented Aug 24, 2026

Copy link
Copy Markdown
Author

Thanks — and that offer is genuinely useful, since it removes the half of the blocker I can't solve in my environment.

One clarification on what's actually left, though: the snapshot regeneration was the second gate, not the only one. The wording itself is still a maintainer call. @KTrain5169 proposed:

We strongly recommend manually merging oxlint.config.ts immediately to ensure alignment with IDE integrations

applied to oxfmt.config.ts as well, with the stronger-than-tsdown phrasing justified by oxlint having an editor integration where tsdown doesn't. That reasoning convinces me and I'd be happy to use it close to verbatim — but it's a user-facing string on the success path, and two non-maintainers agreeing on it isn't the same as it being the wording the project wants to ship.

So the order stands as: once a maintainer settles the wording, I'll push the info line and take you up on the UPDATE_SNAPSHOTS=1 just snapshot-test migration_dynamic_oxc_configs run against the branch. Posting the resulting .md diff here would also mean the recorded output gets reviewed before it lands, rather than after CI turns red — which is strictly better than the round trip I was originally planning.


Generated by Claude Code

@KTrain5169

Copy link
Copy Markdown
Contributor

@fengmk2 ^

@fengmk2

fengmk2 commented Aug 25, 2026

Copy link
Copy Markdown
Member

Since oxlint already hard errors on multi-config situations it should make intuitive sense for vp migrate to hard error there as well

Agreed, interrupting with an error is more reasonable, let user to manually address config conflicts first.

Oxlint refuses to run when a directory holds both a JSON config and a
dynamic one ("Only one of `.oxlintrc.json` and `oxlint.config.ts` is allowed
per directory"), so such a project cannot lint before migration either.
Migration had no non-destructive path through that state: first-match
detection inlined and deleted the JSON config while leaving the dynamic one
on disk unreferenced, where it then silently shadows the freshly inlined
`lint` block for direct `oxlint` invocations — the settings the user just
migrated stop applying, with no diagnostic.

Detect the ambiguity up front and interrupt instead, naming every directory
and the files that collide, so the user resolves the conflict before
anything is rewritten. The check runs before any file is touched and covers
the workspace root and every workspace package, on both the full-migration
and the already-Vite+ paths.

Two JSON forms are deliberately not treated as a conflict: the ambiguity
this guards against is JSON-vs-dynamic, which is the combination oxlint
itself rejects and the one that orphans a config across migration.

m0g3r commented Aug 25, 2026

Copy link
Copy Markdown
Author

Thanks — that settles (3), so I've implemented it. Pushed 239cd38.

What it does

vp migrate now refuses to start when a directory holds both a JSON and a dynamic config for the same Oxc tool, naming every offending directory:

✘ Conflicting Oxc configs:
  - the project root has `.oxlintrc.json` and `oxlint.config.ts` — oxlint allows only one config per directory.
  - packages/app has `.oxfmtrc.json` and `oxfmt.config.mts` — oxfmt allows only one config per directory.

then exits 1 via cancelAndExit. It runs immediately after workspace detection — before any file is touched — and covers the workspace root plus every workspace package, on both the full-migration and the already-Vite+ paths. That placement matters more than the message: the failure mode is losing settings mid-rewrite, so the interrupt has to land before the first write, not at the summary.

Two scope calls, both easy to trim if you disagree

  • Two JSON forms (.oxlintrc.json + .oxlintrc.jsonc) are deliberately not a conflict. The ambiguity being guarded is JSON-vs-dynamic, which is what oxlint itself rejects and what orphans a config across migration. Erroring on .json + .jsonc would be me guessing at behavior I could not verify.
  • oxfmt is included alongside oxlint. Being straight about the asymmetry in the evidence: for oxlint the hard-error is quoted upstream behavior, and I could not run the bundled binaries here to confirm oxfmt does the same. The reason I still included it is the migrator-side hazard, which is symmetric regardless of what the binary does — first-match detection inlines and deletes the rc file and leaves the dynamic config unreferenced either way. If you'd rather this stayed strictly oxlint-only until someone confirms oxfmt's behavior, deleting the oxfmt entry from the tools array in detectOxcConfigConflicts is the whole change.

Verification

  • 22 new unit tests in detector.spec.ts (27 in the file total), verified in both directions:
    • reverting detector.ts entirely → 22 failed / 5 passed (the 5 are the pre-existing detectConfigs tests, so the default path is untouched);
    • keeping the API but disabling only the conflict decision → 9 failed / 18 passed, i.e. the failures come from the behavior, not from a missing export.
  • No existing snapshot output changes, and this is checked rather than assumed: no fixture under cli_snapshots/fixtures/ pairs a JSON and a dynamic config for the same tool. The dirs with two configs are all oxlint+oxfmt pairs, which this guard ignores.
  • Ran the whole packages/cli/src/migration/__tests__ directory before and after and diffed the failure sets — identical, with +22 passing (397 → 419 tests). Absolute failures in that directory are my sandbox's NAPI stub, not results about this change.
  • oxfmt --check and oxlint clean on the changed files, each run in the same invocation as untouched control files so a silent misconfiguration would show up; tsc --strict clean on detector.ts and no non-module-resolution errors in bin.ts.

Still open

  • The PTY fixture for the new error path is not in this commit. I can't record a snapshot I can't run, and this sandbox has no built vp. @simulacre7 — if that offer still stands, this is a second, smaller thing to run alongside the (2) one: a fixture with .oxlintrc.json + oxlint.config.ts in the root asserting the interrupt.
  • (2) is unchanged and still needs a maintainer call on the wording — the stronger "we strongly recommend manually merging …" line for oxlint.config.ts / oxfmt.config.ts. It's independent of this commit.

Generated by Claude Code

@simulacre7

Copy link
Copy Markdown
Contributor

Ran the offer against 239cd38 — here is the PTY fixture for the new error path plus its recorded snapshot, ready to commit. Everything below was recorded and verified locally on macOS arm64 (UPDATE_SNAPSHOTS=1 just snapshot-test migration_oxc_config_conflict, then a clean re-run: 1 passed).

Also cross-checked your "no existing snapshot output changes" claim by running just snapshot-test migration_dynamic_oxc_configs on the branch: passes unchanged.

fixtures/migration_oxc_config_conflict/snapshots.toml

[[case]]
name = "migration_oxc_config_conflict"
vp = "global"
steps = [
  { argv = ["vp", "migrate", "--no-interactive"], comment = "migration should refuse to start on conflicting Oxc configs", continue-on-failure = true },
  { argv = ["vpt", "stat-file", ".oxlintrc.json", "--assert", "file"], comment = "both configs left untouched by the interrupt", continue-on-failure = true },
  { argv = ["vpt", "stat-file", "oxlint.config.ts", "--assert", "file"], continue-on-failure = true },
  { argv = ["vpt", "stat-file", "vite.config.ts", "--assert", "missing"], comment = "no file was written before the interrupt", continue-on-failure = true },
  { argv = ["vpt", "print-file", "package.json"], comment = "package.json unchanged", continue-on-failure = true },
]

fixtures/migration_oxc_config_conflict/package.json

{
  "name": "migration-oxc-config-conflict",
  "scripts": {
    "lint": "oxlint"
  },
  "devDependencies": {
    "oxlint": "^1.0.0",
    "vite": "^7.0.0"
  }
}

fixtures/migration_oxc_config_conflict/.oxlintrc.json

{
  "rules": {
    "no-console": "error"
  }
}

fixtures/migration_oxc_config_conflict/oxlint.config.ts

import { defineConfig } from 'oxlint';

export default defineConfig({
  rules: {
    eqeqeq: 'error',
  },
});

fixtures/migration_oxc_config_conflict/snapshots/migration_oxc_config_conflict.md (recorded)

# migration_oxc_config_conflict

## `vp migrate --no-interactive`

migration should refuse to start on conflicting Oxc configs

**Exit code:** 1

```
VITE+ - The Unified Toolchain for the Web

✘ Conflicting Oxc configs:
  - the project root has `.oxlintrc.json` and `oxlint.config.ts` — oxlint allows only one config per directory.
Keep a single config per directory, then run `vp migrate` again.
```

## `vpt stat-file .oxlintrc.json --assert file`

both configs left untouched by the interrupt

```
.oxlintrc.json: file
```

## `vpt stat-file oxlint.config.ts --assert file`

```
oxlint.config.ts: file
```

## `vpt stat-file vite.config.ts --assert missing`

no file was written before the interrupt

```
vite.config.ts: missing
```

## `vpt print-file package.json`

package.json unchanged

```
{
  "name": "migration-oxc-config-conflict",
  "scripts": {
    "lint": "oxlint"
  },
  "devDependencies": {
    "oxlint": "^1.0.0",
    "vite": "^7.0.0"
  }
}
```

The steps assert the "before any file is touched" placement, not just the message: both configs still present, no vite.config.ts created, package.json byte-identical. Still holding the (2) snapshot run for whenever the wording lands.

…pshot

Assert the fail-fast path end to end: `vp migrate` exits 1 naming the
offending directory, and — the part that matters more than the message —
nothing has been written when it does. The steps check both configs are
still on disk, no `vite.config.ts` was created, and `package.json` is
byte-identical, so a regression that moved the check after the first write
would fail here even if the error text stayed the same.

Recorded on macOS arm64 by simulacre7, who also confirmed the existing
`migration_dynamic_oxc_configs` case still passes unchanged on this branch.

Co-authored-by: simulacre7 <16968090+simulacre7@users.noreply.github.com>

m0g3r commented Aug 25, 2026

Copy link
Copy Markdown
Author

@simulacre7 thank you — landed as 13a11a9, with a Co-authored-by trailer since the recording is yours.

I didn't take the snapshot on trust, since I can't re-run the recorder here. What I did check before committing it:

  • Ran the real detector against the real fixture directory in this sandbox — collectOxcConfigConflicts + formatOxcConfigConflict on fixtures/migration_oxc_config_conflict/ returns exactly one conflict (oxlint, dir .) and renders byte-identically to the block you recorded, em-dash and backticks included.
  • The one structural difference from an existing recorded case is the second line inside log.error. prompts.log.error is a multi-line call here, and log.message defaults withGuide to false, so continuation lines get no bar prefix — the - indent is the one bin.ts supplies. Everything else in the frame (banner, blank line, line, then the bare cancelAndExit line) is pinned by migration_not_supported_npm8_2, which is an actual recorded run of the identical log.error + cancelAndExit pair.
  • vpt stat-file prints {path}: {state} and print-file cats raw bytes, so those four steps' outputs are exact.
  • The .md layout matches the writer in cli_snapshots/main.rs## heading, comment, **Exit code:** only when nonzero, fenced block.

Also worth saying explicitly, because it's the reason your step choice is the right one: the placement your steps assert is real, not incidental. assertNoOxcConfigConflicts runs immediately after detectWorkspace, before package-manager resolution and before the already-Vite+ early return. That's why the fixture needs no packageManager pin to reach the check, and why vite.config.ts: missing plus a byte-identical package.json are guaranteed rather than hopeful. A regression that moved the guard after the first write would fail those steps even if the error text were untouched — which is more than the message assertion alone would catch.

The one thing I could not do is run the PTY suite (no built vp in this sandbox), so the recorded .md is yours, unmodified. If CI's recorder disagrees with it on whitespace, the .md.new diff is the authority over my reading of the writer — please say so and I'll correct it rather than argue from source.

Still open and unchanged by this commit: (2)'s wording needs @fengmk2's call, and the oxfmt scope question from my last comment (whether the guard stays oxlint-only until someone can confirm oxfmt's own multi-config behaviour — deleting the oxfmt entry from the tools array is the entire change if so).


Generated by Claude Code

@simulacre7

Copy link
Copy Markdown
Contributor

Closing the oxfmt scope question with the bundled binary (oxfmt 0.63.0 from this branch's packages/cli/node_modules):

$ ls
.oxfmtrc.json  oxfmt.config.mts  a.js
$ oxfmt --check a.js
Failed to load configuration file.
Both '.oxfmtrc.json' and 'oxfmt.config.mts' found in <dir>.
(exit 1)

Control with only .oxfmtrc.json present passes (exit 0). So oxfmt enforces the same one-config-per-directory rule as oxlint, and keeping oxfmt in the tools array is upstream-behavior-backed, not just migrator-hazard-backed — no trim needed.

Snapshot side of 13a11a9: re-ran just snapshot-test migration_oxc_config_conflict against the branch after your commit — passes against the landed .md as-is.

The conflict guard fired only when a directory held a JSON *and* a dynamic
config for the same tool. Running the pinned binaries shows the rule both
tools enforce is one config per directory, not one config form: oxlint
1.78.0 and oxfmt 0.63.0 each fail with "Both '<a>' and '<b>' found in
<dir>" for `.oxlintrc.json` + `.oxlintrc.jsonc` and for
`oxlint.config.ts` + `oxlint.config.mts`, exactly as they do for the mixed
pair. Both of those shapes previously migrated through, consuming one
config and leaving the other on disk unreferenced.

Detection now flags any tool with more than one config present, and the
conflict carries a single `configs` list in the tool's candidate order
instead of the JSON/dynamic split. The rendered line is unchanged for the
two-config case and gains comma separation beyond it.

m0g3r commented Aug 25, 2026

Copy link
Copy Markdown
Author

@simulacre7 that settles oxfmt — thank you. I reproduced it here against the same pinned pair (oxfmt@0.63.0, oxlint@1.78.0 from this branch's catalog) and got your exact result, so the oxfmt entry in the tools array stays: no trim.

But running the matrix rather than just your one case turned up a real gap in my own guard, so I've pushed 31163c5.

I had written, in the comment above, that two JSON forms are deliberately not a conflict, on the grounds that "erroring on .json + .jsonc would be me guessing at behavior I could not verify." I could verify it once I installed the binaries, and the guess was wrong. Both tools reject any two configs in a directory, not just the mixed pair:

files in the directory oxlint 1.78.0 oxfmt 0.63.0 old guard
.oxlintrc.json only runs, exit 0 runs, exit 0
oxlint.config.mts only runs, exit 0 runs, exit 0
.oxlintrc.json + oxlint.config.mts Both '…' and '…' found in <dir> same fires
.oxlintrc.json + .oxlintrc.jsonc Both '…' and '…' found in <dir> same misses
oxlint.config.ts + oxlint.config.mts Both '…' and '…' found in <dir> same misses

So the rule is one config per directory, not one config form — and the two shapes the old predicate let through are exactly the ones migration would then walk into: first-match detection consumes one config and leaves the other on disk unreferenced. Same failure mode the guard exists to prevent, just reached by a different pair.

What changed

detectOxcConfigConflicts now flags any tool with more than one config present, and OxcConfigConflict carries a single configs list in the tool's own candidate order instead of the jsonConfigs/dynamicConfigs split. Net effect on the source is a simplification (−8 lines).

Verification

  • Both directions, on the repo's own detector.spec.ts: 31 passed with the fix; 4 failed / 27 passed with only the decision reverted to the old JSON-and-dynamic predicate while keeping the new configs API — the four failures are precisely the four newly covered shapes, so they come from behaviour, not a renamed field. A full revert of detector.ts gives 14 failed / 17 passed.
  • Whole packages/cli/src/migration/__tests__ directory, before and after: failure sets byte-identical (151 both sides, zero new, zero newly fixed), tests 419 → 423. The absolute failures there are my sandbox's NAPI stub, not results about this change.
  • Your snapshot is untouched and still correct. I re-ran the real collectOxcConfigConflicts + formatOxcConfigConflict over every one of the 1,702 directories in the checkout under the new rule: exactly one conflict, fixtures/migration_oxc_config_conflict, and its rendered line compares === true against the recorded .md. No other fixture newly trips the widened guard.
  • oxfmt --check and oxlint clean on the changed files, each run in the same invocation as untouched control files; tsc --strict --noEmit clean on both changed files, and no bin.ts error mentions the conflict API.

Message rendering is unchanged for the two-config case (so the snapshot stands as recorded) and gains comma separation once a directory holds three or more: has `a`, `b` and `c` .

Still not run here: the PTY suite, for the same reason as before — no built vp in this sandbox.

Unchanged and still needing @fengmk2: (2)'s wording, the stronger "manually merge" line. That's the only open item left on this PR from my side.


Generated by Claude Code

@simulacre7

Copy link
Copy Markdown
Contributor

PTY suite result for 31163c5 (the gap you flagged): all 154 migration* cases pass on macOS arm64, both flavors.

Full detail for honesty: the parallel run finished 151/154, with 3 failures whose names changed between runs (migration_dynamic_oxc_configs among them, which had passed twice before) — all 3 pass deterministically when run in isolation, so those are load-induced PTY flakes on my machine, not the diff. An earlier run also showed local-flavor failures that were purely my stale packages/cli/dist tripping the freshness guard; rebuilt and they cleared. Nothing attributable to 31163c5, and migration_oxc_config_conflict passes against the landed .md unchanged.

m0g3r commented Aug 25, 2026

Copy link
Copy Markdown
Author

@simulacre7 that closes the last gap on my side — thank you. The PTY suite was the one check I'd flagged as not runnable in my sandbox, so 31163c5 now has it covered.

Two things I could verify from this end that make the result a bit stronger than a bare pass:

The 154 is the complete migration set. Counting [[case]] entries across every migration_* fixture at 31163c5 gives exactly 154. So the run covered all of them, the new migration_oxc_config_conflict included — worth stating explicitly, because the count on its own rules out a filter that quietly skipped the new case.

The fixture that flaked isn't structurally unusual. migration_dynamic_oxc_configs is one of 21 fixtures that invoke vp migrate twice, and at 7 steps it sits mid-pack (19 migration fixtures have 7 steps or more). Nothing about it is heavier than its peers, which supports your reading — runner load, not something this PR's fixture does. Together with the failure names changing between runs and all three passing in isolation, I don't see a case for touching it.

If that non-deterministic set ever turns up on main outside this branch, it's probably worth its own issue. I can't chase it from here — no built vp in this environment.

Unchanged: (2)'s wording is the only item still open from my side, and it's a maintainer call rather than anything blocked on code or snapshots.


Generated by Claude Code

@simulacre7

Copy link
Copy Markdown
Contributor

Followed up on the flake question: it reproduces on main too (cold first run after a fresh build; warm re-runs fully green; every failed case passes in isolation). Filed as #2565 with the five-run matrix, so nothing further is attributable to this branch.

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.

Also migrate dynamic Oxlint & Oxfmt configurations if they exist

4 participants