facebook / facebook/astryx

astryx upgrade: integration-codemod discovery throws (drops the whole integration) when codemods/ contains test or helper modules

Open
#4,975 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
13.1k
Forks
1.1k
Avg merge
1d 14h
Merged PRs (30d)
669

Description

## Summary

`astryx upgrade`'s integration-codemod discovery (`assets/codemods/integration-discovery.mjs` → `collectCodemodFiles`) recursively loads **every** `.ts/.mjs/.js` file under an integration's `codemods//` directory and parses each as a codemod. Because discovery is strict (any file that fails to parse as a codemod throws a hard error), a single non-codemod module in that tree makes `discoverIntegrationCodemods` throw — which drops the **entire** integration's codemods for that run. The core codemods still run, so the upgrade reports success while the integration's codemods silently never execute.

This bites any integration that co-locates test files or shared transform helpers alongside its codemods (a very common layout).

## Repro

An integration package whose manifest declares `codemods: './codemods'`, with this (normal-looking) layout:

```
codemods/
0.1.0/
migrate-theme-default.mjs # real codemod (default-exports an envelope)
migrate-theme-default.transform.mjs # helper module, NAMED exports only
__tests__/
migrate-theme-default.transform.test.mjs # vitest test
```

Running `astryx upgrade --from --to --path --apply` (from → to spans the codemod's version) does **not** apply the codemod. Calling discovery directly shows why:

```
discoverIntegrationCodemods([{ name: '@scope/pkg', codemods: '/codemods' }])

// THROWS, depending on which file the walker hits first:
// "Cannot find package 'vitest' imported from .../__tests__/*.test.mjs"
// "Integration ... codemod 0.1.0/migrate-*.transform (.../*.transform.mjs) is invalid: (root): Invalid input: expected object, received undefined"
```

Root cause is in `collectCodemodFiles`: it `walk()`s recursively and only skips `node_modules`/`.git`, then treats every `.ts/.mjs/.js` as a codemod candidate and feeds it to `loadModuleWithParser` + `parseCodemod`. So:

1. `__tests__/*.test.mjs` → imports `vitest` (not a dependency of consumer apps) → module load throws.
2. `*.transform.mjs` shared helper → no default codemod envelope → `parseCodemod` throws.

Either one makes the whole discovery throw, and the strict "fail loudly" contract then discards every codemod from that integration.

## Impact

- Any integration that keeps tests or transform/helper modules next to its codemods (a natural, idiomatic layout) silently loses **all** of its integration codemods during `astryx upgrade`.
- It fails **quietly from the user's perspective**: core codemods still run and the run reports success, so the missing integration migrations are easy to miss until a downstream build breaks on un-migrated code.

## Discussion (what's the right contract?)

Filing this for a team decision rather than prescribing a fix. Some options that have come up:

1. **Discovery should identify codemods by shape, not by "every module in the tree."** e.g. skip modules whose default export doesn't parse as a codemod envelope (skip, don't throw), and/or skip conventional non-codemod paths (`__tests__/`, `*.test.*`, `*.spec.*`).
2. **Define an explicit codemod-file convention** (e.g. only load files matching a documented pattern, or only files directly in `codemods//` and not nested), and document that helpers/tests must live outside it.
3. **Keep strict loading but narrow the walk** (exclude test/spec globs + a documented helper suffix).
4. **Manifest-driven**: let the integration manifest list its codemod entry points explicitly instead of directory-scanning.

There's a real tension between "fail loudly on a genuinely broken codemod" and "don't explode on an adjacent test/helper file." Interested in the team's view on where that line should be, and whether the fix belongs in the walk filter, the parse/skip behavior, or the documented authoring convention.

In the meantime we're working around it on the integration-package side by keeping `codemods//` limited to loadable codemod modules only (moving tests + shared transform helpers out of the scanned tree), but that's a workaround for the current discovery behavior rather than the intended contract.

## Environment
- `@astryxdesign/cli` 0.3.0
- Observed while upgrading Nest apps that consume an internal integration package; reproduced deterministically.

Contributor guide

Open the contributing guide

Research direction

Start in assets/codemods/integration-discovery.mjs at collectCodemodFiles, then trace loadModuleWithParser, parseCodemod, and discoverIntegrationCodemods. Reproduce with the integration layout described in the issue and the astryx upgrade command. Done means the agreed discovery contract distinguishes codemod entry points from adjacent tests/helpers without silently dropping valid integration codemods, with coverage for the reported failure modes.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
cli, tooling
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.