Skip to content

feat(DST-1543): add marigold migrate codemods for breaking releases#5675

Open
aromko wants to merge 13 commits into
beta-releasefrom
DST-1543_codemod-engine
Open

feat(DST-1543): add marigold migrate codemods for breaking releases#5675
aromko wants to merge 13 commits into
beta-releasefrom
DST-1543_codemod-engine

Conversation

@aromko

@aromko aromko commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Description

Adds marigold migrate [version] to @marigold/cli: data-driven codemods for breaking style and HTML-structure changes in consumer themes and application code.

The core design goal: per-version work is data, not code. The transforms in lib/codemod/primitives/ are version-agnostic; everything v18-specific lives in manifests/v18.ts (slot sets, swap baselines, icon renames, token changes), so a future v19 migration is a new manifest, not new transform code. Codegen for those manifests plus the CI drift check is the follow-up ticket DST-1650.

What the v18 migration does:

  • Theme files (anchored on ThemeComponent<'X'> with a verified @marigold/system import, file layout does not matter): restructures single-cva components to the new slot shapes (consumer classes move verbatim, never overridden), swaps baseline styles only on a byte-exact match with the old baseline (proof the slot was never customized, with a token diff report), stubs new slots as cva({}), and scaffolds required new components (BooleanField) next to the theme files that need them, including the barrel export.
  • Application code (anchored on the @marigold/components / @marigold/icons import): the official icon renames (direct rename of import plus usages when provably safe, alias fallback otherwise), Tabs.TabPanel to Tabs.Panel, Inset spacing props, acceptedFileType with array wrap, TextField min/max removal.
  • Design tokens (report-only, since token values are consumer property): renamed or removed tokens still referenced (bg-brand: use bg-primary), new tokens that component internals hardcode but the consumer CSS does not define (SelectList needs selected-bold/disabled-surface), and repurposed tokens that kept their name but changed meaning (disabled flipped background to text) with a remap recipe at the definition site.
  • Everything undecidable is a warning, never a guess: spreads, customized slots, structural JSX moves, DOM changes that break consumer CSS selectors. Warnings link to pinned GitHub permalinks with line numbers.
  • UX: the version parameter is optional (installed @marigold/components is detected and the migration proposed for confirmation), and interactive runs pre-analyze the target and offer the fired changes as a multiselect, with --only <names> as the non-interactive equivalent.

Validated against the portal repo: the full dry run edits 123 files, scaffolds BooleanField, and raises 50 warnings, all pointing at real definition sites (for example the --color-disabled role flip at their tailwind.css:110). Re-running is a no-op by design.

See packages/cli/src/lib/codemod/README.md for the architecture, invariants, and how to add a v19 manifest.

Closes DST-1543

Test Instructions

All commands run from the repo root unless noted. The CLI runs from source via tsx, no build needed.

  1. Unit tests (356, includes 14 token and 3 selection tests):

    pnpm --filter @marigold/cli exec vitest run
  2. Download a project which is using marigold e.g. ACC, Reporting, Core etc.

  3. Run node packages/cli/dist/bin/marigold.mjs migrate v18 --dry-run

  4. Run it again without param --dry-run (changes will be made review them)

or what claude suggests...

  1. Create a small demo consumer that triggers every codemod class:

    DEMO=$(mktemp -d)
    mkdir -p $DEMO/theme
    cat > $DEMO/theme/Card.styles.ts <<'EOF'
    import { cva, ThemeComponent } from '@marigold/system';
    export const Card: ThemeComponent<'Card'> = cva({ base: ['bg-white'] });
    EOF
    cat > $DEMO/theme/Switch.styles.ts <<'EOF'
    import { cva, ThemeComponent } from '@marigold/system';
    export const Switch: ThemeComponent<'Switch'> = {
      container: cva({ base: 'disabled:cursor-not-allowed disabled:text-disabled-foreground' }),
      track: cva({ base: 'flex h-6 w-10' }),
      thumb: cva({ base: 'block size-5' })
    };
    EOF
    printf "export * from './Card.styles';\nexport * from './Switch.styles';\n" > $DEMO/theme/index.ts
    cat > $DEMO/App.tsx <<'EOF'
    import { Inset, SelectList, Tooltip } from '@marigold/components';
    import { Pickup } from '@marigold/icons';
    export const App = () => (
      <Inset space={4}>
        <Pickup />
        <Tooltip open>hint</Tooltip>
        <SelectList aria-label="x" />
        <div className="bg-brand text-muted-foreground" />
      </Inset>
    );
    EOF
    printf ":root { --color-disabled: #eee; --color-disabled-foreground: #999; }\n" > $DEMO/tokens.css
  2. Dry run and read the report:

    cd packages/cli
    npx tsx src/bin/marigold.ts migrate v18 $DEMO --dry-run

    Expect: Card restructure, Switch.container baseline swap, Inset space to p rename, Pickup to Store icon rename, warnings for Tooltip open, the bg-brand/text-muted-foreground token renames, the SelectList token dependency, the --color-disabled repurpose recipe, and 1 scaffolded file (BooleanField.styles.ts).

  3. Pre-analysis and selection: run without --dry-run in your terminal. A multiselect lists the changes that fire, with counts. Everything is preselected, Enter applies all; deselect entries with space to skip them. Warnings always run.

    npx tsx src/bin/marigold.ts migrate v18 $DEMO
  4. Subset via flag (non-interactive equivalent, also try a typo to see the error):

    npx tsx src/bin/marigold.ts migrate v18 $DEMO --only rename-imports,scaffold-components
  5. Idempotence: run step 4 again and confirm Edited 0 file(s).

  6. Version auto-detection (needs a target with @marigold/components installed or declared, for example a real consumer checkout):

    npx tsx src/bin/marigold.ts migrate /path/to/consumer --dry-run

    Expect a confirm prompt naming the detected version and the proposed v18 migration. Piped runs fail with instructions to pass the version explicitly.

Breaking Changes

No. New CLI subcommand only; existing commands are untouched (doctor's highlightCode moved to lib/format.ts, same behavior).

Checklist

  • Storybook preview and Marigold docs preview are available (n/a, CLI only)
  • Stories added/updated (n/a, CLI only)
  • Unit tests added/updated
  • Component documentation added/updated (packages/cli/src/lib/codemod/README.md)
  • Accessibility reviewed (n/a, CLI only)
  • Visual regression tests updated (n/a, CLI only)
  • Changeset added (.changeset/migrate-codemods.md, minor for @marigold/cli)

aromko added 8 commits July 23, 2026 16:49
Version-agnostic primitives driven by a per-version manifest, anchored on
ThemeComponent<'X'> declarations verifiably imported from @marigold/system:

- restructure-to-slots: single-cva components become slot objects, the
  consumer's cva moves verbatim into the primary slot
- swap-exact-classes: baseline slots swap to the new baseline only on a
  byte-exact per-slot match (proof the slot was never customized), with a
  -/+ token diff so renamed tokens are visible at a glance
- stub-missing-slots: missing slot keys become cva({}) stubs; dropped and
  spread-hidden slots are reported, never guessed at
- scaffold-component: theme files for new components, fully derived from
  the manifest slot list, with load-bearing layout classes injected
- report primitives for dead theme keys and DOM-structure changes

The v18 manifest is hand-written (delivery step 2); slot sets extracted
from the Theme type, swap class strings extracted from the published
v17.9.1 theme-rui, links pinned to commit 946dc9f. Codegen (DST-1650)
will generate this file for future majors.

All edits are byte-preserving (magic-string): untouched consumer code is
never re-printed, and re-running a migration is a no-op.
JSX transforms anchored on imports from @marigold/components — a local
component that happens to share a Marigold name is never touched:

- rename-jsx-props: Inset space/spaceX/spaceY to p/px/py,
  acceptedFileType to acceptedFileTypes (with array wrap)
- rename-jsx-members: Tabs.TabPanel to Tabs.Panel,
  SelectList.Item to SelectList.Option (opening and closing tags)
- remove-jsx-props: TextField min/max (dropped in v18)
- rename-imports: the v18 icon migration, driven by the official mapping
  table in .changeset/iconography-docs.md. Renames import + usages
  directly when the file provably allows it; falls back to the
  release-notes-blessed alias form (Store as Pickup) on shadowing,
  shorthand properties, or name collisions, and names the reason
- report-jsx-usage: warnings for changes needing a human decision
  (Tooltip open, width="fit", Multiselect, Switch size="large",
  Card one-sided paddings, SelectList onChange signature)

Only lexically decidable changes are edits; everything requiring a
structural JSX move or a design decision stays a warning.
`marigold migrate <version> [path] [--dry-run]` runs the codemod
pipeline over a consumer repo: one read per file, a theme-component
inventory pass, the ordered per-file pipeline (restructure, swap, stub,
JSX transforms, reports), and a scaffold pass that creates missing
theme components next to the files that require them and registers them
in the local barrel.

The report is TTY-aware (plain when piped): green ~ changes, yellow !
warnings with cyan code spans and clickable pinned source links, red/
green -/+ token diffs for baseline swaps, and a closing reminder that
the consumer's typecheck is the completeness check.

Lazy-loaded in the bin like init/doctor so @babel/parser and
magic-string stay off the docs/list hot path.
Usage and report legend, the pipeline order and why it matters, the
invariants (never override, byte-preserving, warn-never-guess,
idempotent, Theme contract as a stable API), how to author a v19
manifest, when to add a primitive vs a one-off transform, and known
limitations.
v17 exported a custom Print icon (info/Print) but the migration table
in the iconography changeset has no row for it; Printer is the Lucide
equivalent. Found while generating the DST-1543 icon-rename codemod
from this table.
`marigold migrate` (version omitted) walks up from the target path to
find @marigold/components — installed node_modules first, the declared
package.json range as fallback — proposes the applicable migration(s)
in order, and runs them after an interactive confirm (Enter accepts).

The proposal includes the installed major itself (>=, not >): the
documented procedure is upgrade-then-migrate, so a consumer on
18.0.0-beta is exactly who needs the v18 migration, and re-running is
a no-op. Non-interactive sessions get the detected proposal plus the
explicit command to run instead of a hanging prompt; a repo with no
Marigold at all fails with guidance.
Token breakage compiles and typechecks fine and only shows up in the
browser, so the manifest gains a `tokens` section driving report-only
checks (never edits — token values are consumer property):

- renamed/removed tokens still referenced (`bg-brand` -> use `bg-primary`;
  text scan over every ts/tsx/css file, utilities and raw var() reads)
- new tokens components hardcode internally (v18: SelectList's selection
  indicator needs `selected-bold`/`disabled-surface`) but the consumer CSS
  does not define — the classes bypass the theme layer entirely
- repurposed tokens that kept their name but changed meaning (`disabled`
  flipped background->text, `secondary` surface->text, status tokens
  solid->muted): definition sites get a remap recipe until the `settledBy`
  token appears; consumers not defining them get old-role usage warnings

Warnings are suppressed per token when the consumer's own CSS defines it
(their vocabulary, still self-consistent), and theme-rui consumers count
every added token as defined by construction. All manifest data is
value-verified against theme-rui v17.9.1 vs v18 and codegen-able
(DST-1650). vendor/ and *.min.css are skipped as build artifacts.
Interactive `marigold migrate` runs now analyze the target first (a dry
run in memory) and offer the changes that actually fire — name,
description, change and file counts — as a multiselect. Everything is
preselected, so Enter still applies the full migration; deselected
entries are skipped. Report-only passes are not selectable and always
run: the warnings are the safety net, and a partial migration needs them
more, not less.

`runMigrate` returns the structured `summary` and takes `only` (edit
codemod names plus scaffold-components), so scripts and CI get the same
subset non-interactively via `--only a,b` — unknown names fail listing
the valid ones. A full run without a selection is byte-identical to
before.
@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marigold-storybook Ready Ready Preview, Comment Jul 27, 2026 11:15am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
marigold-production Ignored Ignored Comment Jul 27, 2026 11:15am
marigold-docs Skipped Skipped 1 resolved Jul 27, 2026 11:15am

Request Review

@changeset-bot

changeset-bot Bot commented Jul 24, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4446028

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@marigold/docs Patch
@marigold/cli Minor
@marigold/system Patch
@marigold/components Patch
@marigold/icons Patch
@marigold/theme-rui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions Bot added the type:feature New feature or component label Jul 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Accessibility tests executed. Download the report here.

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Coverage Report for Marigold Code Coverage

Status Category Percentage Covered / Total
🔵 Lines 98.72% 2792 / 2828
🔵 Statements 97.65% 2920 / 2990
🔵 Functions 97.96% 769 / 785
🔵 Branches 90.96% 1823 / 2004
File CoverageNo changed files found.
Generated in workflow #23242 for commit 4446028 by the Vitest Coverage Report Action

Review findings on #5675, all verified against the branch:

- a bare `export { Pickup }` forced through a direct rename left the
  specifier pointing at nothing; re-exported names now force the alias
  fallback, and `export { Pickup } from '@marigold/icons'` is rewritten
  to `Store as Pickup`, keeping the public name
- `<Icons.Pickup />` properties were renamed like usages; JSXMemberExpression
  properties are name positions now
- the scan includes .js/.jsx (and skips .min.js), so JS consumers get the
  codemods and token warnings too
- new report-only pass warns on namespace imports of affected packages,
  which every anchor silently misses
- a file-level parse error is reported once, not per codemod
- dropped the stray awaits on the synchronous runMigrate in tests
@github-actions

Copy link
Copy Markdown
Contributor

Accessibility tests executed. Download the report here.

@aromko
aromko marked this pull request as ready for review July 24, 2026 14:23
@github-actions

Copy link
Copy Markdown
Contributor

Accessibility tests executed. Download the report here.

@github-actions

Copy link
Copy Markdown
Contributor

Accessibility tests executed. Download the report here.

Comment thread .changeset/iconography-docs.md

@sarahgm sarahgm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Automated review — /simplify + /review-pr (DST-1543)

Solid, well-documented PR with an excellent data-driven architecture (generic engine, all component specificity as manifest data) and strong safety discipline. Nothing here blocks merge — all findings are quality/reuse refinements.

One note on verification: my first pass suspected a latent rename bug from the unreachable default branch in isBindingPosition, but I tested it (plain function-param shadow of a renamed icon) and the output is a correct uniform alpha-rename, with target-name collisions caught independently by namesInFile. So that item is demoted to a behaviour-neutral dead-code cleanup, not a bug.

Generated with Claude Code (review-pr + simplify skills).

Comment thread packages/cli/src/lib/codemod/primitives/tokens.ts Outdated
Comment thread packages/cli/src/bin/marigold.ts Outdated
Comment thread packages/cli/src/lib/codemod/primitives/jsx.ts Outdated
Comment thread packages/cli/src/lib/codemod/primitives/report.ts Outdated
Comment thread packages/cli/src/commands/migrate.ts
Comment thread packages/cli/src/commands/migrate.ts Outdated
Comment thread packages/cli/src/commands/migrate.ts
Comment thread packages/cli/src/lib/codemod/engine.ts Outdated
Comment thread packages/cli/src/lib/codemod/primitives/jsx.ts
Comment thread packages/cli/src/commands/migrate.test.ts

@sebald sebald left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code Review — local deep-dive + hands-on testing

Reviewed in depth at 064046d and re-checked against ee83609 (merge-only, no packages/cli changes). Findings are inline; summary here.

Overall: this is excellent work. The data-vs-code split (version-agnostic primitives/, per-version manifests/), the warn-never-guess discipline, the byte-preserving edits, and the codemod/README.md are all above bar. The CLI fit is exemplary — existing helpers (tsx-ast walk, edit-tsx insertImport, doctor's package-json/version utils, highlightCode) are reused and extended instead of duplicated, and every integration point (help text, commands-spec, telemetry, lazy imports) is updated coherently. Test quality is high: fixture-based, byte-preservation and idempotency asserted, and the hairy rename-imports edge cases (shadowing, shorthand props, re-exports, alias collisions) are covered.

Hands-on verification (demo consumer exercising every codemod class, CLI built and run from this branch):

  • ✅ 363 unit tests pass; typecheck + lint clean
  • ✅ Dry-run report: every documented change/warning class fired correctly (restructure, byte-exact swap with -/+ token diff, prop/member/import renames, removals, structure/token/namespace warnings, repurpose recipe at the definition site, scaffold announcement)
  • ✅ Applied run: consumer class strings survive byte-for-byte, newSource correctly re-indented to the consumer's 4-space style, all outputs re-parse, BooleanField.styles.ts scaffold + barrel export correct
  • ✅ Icon renames: direct rename (import + usages), alias fallback with stated reason (Trash2 as Delete because re-exported), re-export keeps the public name (export { Store as Pickup })
  • ✅ Idempotent (Edited 0 file(s) on re-run; the swap's token-diff warning correctly disappears once applied)
  • --only subset applies exactly the named changes; typo → helpful error listing valid names, exit 1
  • ✅ Non-TTY: auto-detection fails with actionable instructions; interactive confirm + multiselect verified via pty

Why "request changes" despite all green: the two inline 🔴 findings — the hand-written v18 manifest has drifted behind beta-release (Sidebar rail slots from #5654, ErrorState from #5666). Both confirmed empirically: the rail slots are not stubbed, and a themed ErrorState gets a false "not a themeable component in v18" warning. The product here is the manifest data, so this is the one thing worth blocking on. Everything else is quick fixes / follow-up material.

EmptyState: ['container', 'title', 'description', 'action'],
ToggleButton: ['group', 'button'],
SegmentedControl: ['group', 'list', 'field', 'option', 'indicator'],
Sidebar: [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔴 Manifest drift: Sidebar is missing the 9 rail slots added in #5654 (DST-1609): railRoot, railLayout, railColumn, railToggle, rail, railItem, railFooter, panel, panelTitle.

Verified programmatically against packages/system/src/types/theme.ts on this branch, and reproduced in a live run: stub-missing-slots stubs only the 12 missing slots from this list, so a migrated consumer still fails the exhaustive-Record typecheck on the rail slots — with no codemod assist and no warning.

The branch merged beta-release after this file was written, so the code saw the change but the hand-written data didn't — exactly the drift DST-1650's codegen will prevent. Until then, suggest re-syncing this entry and treating every beta-release merge as a manifest re-check trigger.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in afec448: all 9 rail slots added.

I also checked whether the drift was wider than the two you found. It was not, exactly ErrorState plus these 9 (64 components in the Theme type, 63 in the manifest).

To make it not depend on someone remembering: manifests/theme-drift.test.ts now parses packages/system/src/types/theme.ts and asserts slot parity with the manifest. I verified it bites by removing ErrorState and one rail slot again, which fails with expected [ 'ErrorState' ] and Sidebar: missing [railFooter]. The codemod README names it as a step when authoring a new manifest, so DST-1650's codegen inherits the check rather than replacing it.

'itemDescription',
'itemRemove',
],
EmptyState: ['container', 'title', 'description', 'action'],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔴 Manifest drift: ErrorState is missing from slots — added on beta-release in #5666 (DST-1641) with slots container | title | description | action.

Because it's absent here, report-dead-keys emits a false warning for a themed ErrorState (reproduced locally):

ErrorState: not a themeable component in v18 — these styles are silently unused

…which is actively misleading, since ErrorState is themeable in v18. Same root cause as the Sidebar comment above.

Suggested change
EmptyState: ['container', 'title', 'description', 'action'],
EmptyState: ['container', 'title', 'description', 'action'],
ErrorState: ['container', 'title', 'description', 'action'],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in afec448, applied your suggestion. Confirmed in a live dry run: a themed ErrorState now produces no warning at all instead of the false "not a themeable component in v18".

Same drift test as on the Sidebar thread covers this one, so the next component merged from beta-release fails CI instead of silently mis-warning.

// Utility prefixes that take a color token (`bg-brand`). A curated list
// keeps text scanning honest: matching bare `-brand` suffixes would also
// hit variants and unrelated identifiers.
// ponytail: covers the color utilities Marigold themes actually use; extend

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ What does the ponytail: marker mean? It reads like a personal TODO convention — it also appears in swap-exact-classes.ts:100. Suggest TODO:/Note: (greppable, self-explanatory) or dropping the prefix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair, that is a personal convention leaking into the repo. Both markers are Note: now (afec448).


// Render the change as a token diff (the report colorizes -/+ lines)
// so renamed tokens and new layout utilities are visible at a glance.
// ponytail: added utilities are flagged for manual verification; the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ Same ponytail: marker as in tokens.ts:27 — see the comment there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same, changed to Note: in afec448.

Comment thread packages/cli/src/lib/codemod/engine.ts Outdated
names.map(name => `\`${name}\``).join(', ');

/** the empty-stub property line shared by restructure and stubbing */
export const stubSlotLine = (slot: string, indent: string): string =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ stubSlotLine hardcodes cva, but the rest of the system is alias-aware. ensureCvaImport correctly resolves import { cva as c } from '@marigold/system' and returns the local name — but themeCodemod ignores the return value, and stubs always emit cva({}).

An aliasing consumer therefore gets stubs referencing an undefined cva and no import inserted (ensureCvaImport sees the aliased import and bails). Edge case — the consumer's typecheck catches it — but the fix is small: resolve the local name once per file and thread it into stubSlotLine/generateScaffold's stub rendering.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in afec448, smaller than you sized it: themeCodemod resolves the local name once per file into the visit context, and stubSlotLine takes it. generateScaffold writes its own import header into a brand-new file, so it never sees a consumer alias and did not need threading.

Verified against a consumer importing cva as c:

export const Card: ThemeComponent<'Card'> = {
  container: c({ base: ['bg-white'] }),
  header: c({}),
  ...
};

No duplicate import inserted, and re-running is still a no-op.


writeOutput(result.output);
if (result.hasErrors) exitCode = 1;
} else if (command === 'migrate') {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

💡 Consider moving the interactive flow into commands/migrate.ts. This block is ~110 lines including detection, confirm, and multiselect orchestration; init keeps its prompt flow in commands/init.ts and the router thin. Relocating would match that convention and make the interactive path unit-testable — right now only detectMigration/runMigrate have tests; the confirm/multiselect flow has none.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in afec448. runMigrateCommand now lives in commands/migrate.ts and owns detection, the confirm, the pre-analysis and the multiselect; the router keeps only argument parsing, telemetry and the lazy import. It takes a write callback and an interactive override so the whole flow is drivable from a test.

Coverage that did not exist before: explicit version runs without prompting, the non-TTY refusal, the up-to-date report, the missing-Marigold failure, a declined confirm returning 130, and a multiselect subset leaving the deselected files byte-identical.

const major = Number.parseInt(found.installed, 10);
const versions = Object.keys(MANIFESTS)
.map(v => ({ v, target: Number.parseInt(v.replace(/^v/, ''), 10) }))
.filter(({ target }) => Number.isFinite(target) && target >= major)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

💡 target >= major proposes the v18 migration to consumers still on v17. The comment justifies >= for the same-major case ("upgrade first, then migrate" → someone on 18.x needs v18), but the filter also fires for major = 17 — i.e. someone who has not upgraded yet. Running the theme migration while still on v17 leaves the theme mismatched until they upgrade.

Worth a deliberate decision: either detect the not-yet-upgraded case and say "upgrade @marigold/components first", or keep the behavior and state it in the confirm prompt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deliberately unchanged for now, and I would like your call on it.

The reason I lean toward keeping >=: migrating the theme before bumping the package is a legitimate order, since the theme edits are what makes the app compile after the bump. Someone on 17.x who runs this gets a theme shaped for v18 and a typecheck that tells them exactly what is left, which is the same end state as migrating after the bump. Blocking them would force the sequence "bump, break everything, then migrate".

What is genuinely missing is that we do not say any of this. My proposal is to keep the behavior and add a line to the confirm prompt when the installed major is below the target, along the lines of "you are on 17.9.1; this migrates your theme to the v18 shape, bump @marigold/components to v18 to complete the upgrade". One string, no behavior change.

Say the word and I will add it, or flip it to a hard "upgrade first" if you would rather not have a half-migrated state exist at all.

Comment thread packages/cli/src/bin/marigold.ts Outdated
const { positionals, values } = parseMigrateCommand(rest);
// the version positional is optional: `migrate ./src` treats the first
// positional as a path, `migrate v18 ./src` as version + path
const looksLikeVersion = (p: string | undefined): p is string =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

💡 marigold migrate 18.1 . silently treats 18.1 as a path (the regex only matches v?\d+) and dies with a raw filesystem error. A cheap nudge: if the first positional looks version-ish (/^v?\d+[.\d]*$/) but isn't an exact match, fail with did you mean v18?.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in afec448: 18.1, v18.2 and 18.0.0 all fail with Unknown migration '18.1' — migrations are named by major version. Did you mean v18?.

Worth flagging that my first attempt at this did not work: the positional-count check ran first, so migrate 18.1 ./src still died with the generic usage error. Caught it in manual testing, moved the hint above the count check, and pinned the ordering with a test in bin/marigold.test.ts so it cannot silently regress.

.map(slot => stubSlotLine(slot, inner))
.join('\n');
const original = source.slice(start, end);
s.overwrite(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

💡 Cosmetic, observed in the live run: the moved cva body keeps its original indentation, so after wrapping it sits under-indented one level relative to its new nesting (container: cva({ at depth 1, but base: still at the old depth). "Moved verbatim" is by design and the consumer's Prettier fixes it — but consider running the moved span through reindent, or noting the behavior in the README so nobody files it as a bug.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changed, and I would rather document it than fix it.

Running the moved span through reindent would mean rewriting bytes inside code we promised to move verbatim. That span can contain template literals and multi-line strings where leading whitespace is significant, so the transform would have to start reasoning about what is safe to re-indent. That is a real risk for a cosmetic gain the consumer's Prettier already covers.

Proposing a line in the codemod README's invariants section stating that the moved expression keeps its original indentation by design. Not in afec448 yet; say if you would rather have the reindent and I will do it instead.

`schemaVersion`, keep the old interpreter so chained migrations
(v17 to v19) still work.

## Adding a new migration (e.g. v19)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

💡 Suggestion: turn this section into an agent skill that authors manifests/v*.ts. The provenance rules documented here (slots from the Theme type, oldClasses from the previous major's published theme-rui, tokens from the release-notes tables / CSS diff, permalinks pinned to a tag) are already a mechanical, step-by-step recipe — exactly the shape of a good skill.

It would bridge the gap until DST-1650's codegen lands, and this PR contains the motivating example: the Sidebar/ErrorState drift (see the manifest comments) came from beta-release merges after the manifest was hand-written — a skill-driven re-check on merge would have caught it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, and worth its own ticket rather than growing this PR. I will file one linked to DST-1650 so the skill and the codegen land as one story.

One correction on the motivating example though: the Sidebar/ErrorState drift is now caught mechanically rather than by a skill-driven re-check. manifests/theme-drift.test.ts parses the live Theme type and fails CI on any slot mismatch, and the README section you are commenting on names it as step 3 when authoring a manifest. That covers the one part of the recipe a machine can verify; the parts the skill would own (oldClasses from the previous major's published theme, token tables from the release notes, pinned permalinks) stay unguarded, which is exactly where it earns its keep.

Manifest drift (the blocking findings):

- add `ErrorState` to the v18 slots (#5666) and `Sidebar`'s 9 rail slots
  (#5654). A themed ErrorState no longer gets a false "not a themeable
  component" warning, and the rail slots are stubbed like every other new
  slot.
- add `manifests/theme-drift.test.ts`: parses the live `Theme` type in
  @marigold/system and asserts slot parity with the manifest. Verified to
  catch both drift kinds (missing component, missing slot). This is the
  re-check trigger until DST-1650's codegen lands; the README names it as a
  step when authoring a new manifest.

Correctness and consistency:

- stubs honor an aliased cva import: `themeCodemod` resolves the local name
  once per file and threads it into `stubSlotLine`, so a consumer importing
  `cva as c` gets `c({})` instead of a reference to an undefined `cva`.
- the interactivity gate matches init's: stdout AND stdin must be a TTY,
  so a piped stdin can no longer hang on a prompt.
- `marigold migrate 18.1 ./src` now says "did you mean v18?" instead of
  dying on a filesystem error. Checked before the positional-count
  validation, which would otherwise reject it as "too many paths".
- reword the `Print` -> `Printer` manifest note: the mapping table gained
  the row in this PR, so it is no longer "missing" from it.

Structure:

- move the ~110-line interactive flow out of bin/marigold.ts into
  `runMigrateCommand` in commands/migrate.ts, matching how init keeps its
  prompt flow in the command and the router thin. The flow had no coverage;
  it now has tests for detection, the non-TTY refusal, the declined
  confirm (130) and the multiselect subset.
- `description` is a required field on `Codemod`, replacing the
  runner-side name -> description map whose `?? ''` degraded silently on a
  typo.
- fold report.ts's hand-rolled `analyze` into `themeCodemod`, which
  already returns `unchanged` carrying warnings when a visit pushes no
  changes.
- add a `jsxCodemod` frame behind the four @marigold/components-anchored
  primitives, and equivalent package guards to `renameImports`,
  `reportNamespaceImports` and `reportTokenDependencies`, so theme-only
  files are skipped before parsing instead of after.
- hoist the duplicated `escapeRegex` into lib/regex.ts (was byte-identical
  in edit-css.ts and codemod/primitives/tokens.ts).
- drop the dead `default` arm in `isBindingPosition` and two redundant
  intermediate casts; rename the `ponytail:` comment markers to `Note:`.
- blank lines between the AAA blocks in the codemod tests.

Docs:

- document `marigold migrate` on /getting-started/cli, which only listed
  the other commands.
@github-actions github-actions Bot added the type:docs Improvements or additions to documentation label Jul 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Accessibility tests executed. Download the report here.

@vercel
vercel Bot temporarily deployed to Preview – marigold-docs July 27, 2026 10:50 Inactive
@aromko

aromko commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Review feedback addressed (afec448, 4446028)

Thanks both, the two 🔴 findings were real and the reuse comments were worth taking. Every thread has an individual reply; summary here.

Blocking: manifest drift

ErrorState and Sidebar's 9 rail slots are in. I also checked whether the drift went further than the two you found: it did not, exactly those (64 components in the Theme type, 63 in the manifest).

More importantly it is no longer a "remember to re-check on merge" problem. manifests/theme-drift.test.ts parses the live Theme type in @marigold/system and asserts slot parity, verified to fail on both drift kinds. The codemod README names it as a step when authoring a manifest, so DST-1650's codegen inherits the guard instead of replacing it.

Fixed

  • Alias-aware cva stubs (sebald): themeCodemod resolves the local name once per file, stubSlotLine uses it. A consumer importing cva as c gets c({}), verified end to end.
  • Interactivity gate matches init's (stdout AND stdin).
  • migrate 18.1 now suggests v18. My first cut of this was unreachable behind the positional-count check; caught it in manual testing and pinned the ordering with a test.
  • Interactive flow moved into commands/migrate.ts, router stays thin. It had zero coverage and now has 7 tests, including the declined confirm returning 130 and a multiselect subset leaving deselected files byte-identical.
  • description is a required field on Codemod (sarahgm): the runner-side lookup and its silent ?? '' are gone, so a rename is a compile error.
  • report.ts's analyze folded into themeCodemod, which already returns unchanged carrying warnings. No engine change needed.
  • jsxCodemod frame + package guards: every primitive now skips files it cannot match before parsing rather than after.
  • escapeRegex hoisted to lib/regex.ts, dead default arm dropped, redundant casts dropped, ponytail: markers renamed to Note:, AAA blank lines in the codemod tests.
  • Docs: marigold migrate documented on /getting-started/cli (sarahgm's Vercel comment), with a @marigold/docs changeset.

Deliberately not changed, reasons on the threads

  • Double runMigrate on the interactive path: measured at ~0.6s for a full dry run over this repo's packages/ tree. Took the cheap subset (short-circuits) instead.
  • Per-file re-parse: same, largely mitigated by the short-circuits.
  • Token scan in two passes: the sets are exact complements by construction, and the in-pipeline placement is load-bearing (it runs on post-edit text, so it sees classes the swap introduced). Now stated in a comment.
  • Restructure indentation: proposing a README note over re-indenting a span we promised to move verbatim.
  • Manifest-authoring skill: filing a follow-up linked to DST-1650.

Open question for @sebald

The target >= major behavior for v17 consumers. I lean toward keeping it and adding one line to the confirm prompt when the installed major is below the target, rather than blocking. Reasoning on that thread, happy to go either way.

Verification: 377 CLI tests pass, repo tsc and eslint clean, and I re-ran the full dry run + apply + re-run against a demo consumer covering the aliased cva, the rail stubs and the ErrorState case.

@github-actions

Copy link
Copy Markdown
Contributor

Accessibility tests executed. Download the report here.

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

Labels

type:docs Improvements or additions to documentation type:feature New feature or component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants