refactor(cli): cover migration with effect lint (CLI-2413)

[7ttp]

TL;DR

brings the supabase migration command family under the effect lint

whats introduced?

effect lint applied to migration:

• two allow list entries in .oxlintrc.effect.json for the command family and its live test helper
• handler failures yield their tagged errors directly instead of wrapping them in Effect.fail
• down and repair acquire cache dependencies directly, keeping the same finalizer ordering
• integration tests move onto the FileSystem/Path services, with fixtures written inside the effect
• env cases run through the shared sandbox helper
• cause assertions use typed predicates to check failure tags and payload fields
• e2e tests run through the effect native harness with a scoped temp project dir per test
• live tests run through the effect fixture, with typed DB promise errors and owned cleanup

ref:

• closes: CLI-2413

[github-actions]

🤖 AI Review

Both independent reviews completed. Their seven raw findings deduplicate to six, all confirmed. The most consequential issue is deterministic failure in two live tests: each manually deletes a scoped temp directory before its non-forced scope finalizer runs. The remaining findings are one test-collision risk and four nits concerning assertion strength and ordering.

Findings

Severity	Location	Category	Sources	Claim
🟠 MAJOR	apps/cli/src/commands/migration/fetch/fetch.live.test.ts:86	test-lifecycle	claude+codex	Manually deleting both scoped temporary directories causes their registered scope finalizers to fail when they later try to remove the now-absent paths.
🟠 MAJOR	apps/cli/src/commands/migration/list/list.live.test.ts:46	test-lifecycle	codex	The test deletes a scoped temporary directory before scope closure, causing its registered finalizer to fail on the absent path.
🟡 MINOR	apps/cli/src/commands/migration/fetch/fetch.live.test.ts:13	test-quality	claude	The local migration-version generator can collide because it emits only a 14-digit, second-resolution timestamp instead of using the shared randomized helper.
⚪ NIT	apps/cli/src/commands/migration/squash/squash.integration.test.ts:833	test-quality	claude	The rewritten environment-leak assertion treats an empty leaked value as absent, making it weaker than the previous direct undefined assertion.
⚪ NIT	apps/cli/src/commands/migration/fetch/fetch.live.test.ts:1	style	claude	The changed tests place local imports before package imports, and the squash diff test places a Node builtin after packages, contrary to the repository's established builtin/package/local grouping.
⚪ NIT	.oxlintrc.effect.json:52	style	claude	The new migration-live.ts allow-list entry breaks the alphabetical ordering of the helper block.

Stats

Claude findings: 5 · Codex findings: 2 · Confirmed: 6 · Refuted: 0 · Uncertain: 0

---

Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run

This review runs once per PR. A maintainer can request another with a /ai-review comment.

[Coly010]

Reviewed the production diff line-by-line and audited the test rewrite for weakened assertions. Nothing blocking — one minor should-fix below, plus a PR-description correction.

What I verified

Production code is behaviour-preserving. The Effect.fail(new X()) → yield* new X() conversions are mechanical. The one non-mechanical change is the LinkedProjectCache hoist in down/repair, and it holds up: the old code assigned cacheLinkedRef from a yield* Effect.gen(...) that returned the un-run cache(ref) effect, so loadProjectRef already executed before the flow ran. Moving the acquisition below the const downFlow = Effect.gen(...) declaration doesn't change execution order (the declaration is lazy), and Effect.ensuring is still attached to the whole flow — so the cache write continues to run on the --last, invalid-version and cancel failure paths, and a loadProjectRef failure still short-circuits before the flow with no cache write. Same for repair.

No assertions were weakened. expect() counts are identical in all 16 changed test files (39→39, 44→44, 60→60, 168→168, …). The readdirSync/readFileSync/existsSync → FileSystem service swaps keep every matcher and expected value; the Effect.orElseSucceed(() => false) wrappers reproduce existsSync's false-on-error semantics without masking a real assertion. Two cause assertions got stronger, moving off serialized-substring matching onto typed field reads (up.integration.test.ts on suggestion, migration.integration.test.ts on commandPath).

Fixture ordering is sound. The "fixture written inside the effect but after the code reads it" hazard is real in principle, since Effect.provide builds layers before the generator body runs — but every setup() here is pure layer construction over mocks, and mockCommandSettings performs no filesystem read, so the in-effect config.toml/.env writes still land before the handler reads them. withEnvVar is piped outermost in the env cases, preserving the old mutate-then-build ordering. The faultyFsLayer re-keying resolves to the same absolute paths, and seeding runs through real's underlying closures, so failMigrationsReadDirectoryAtCall: 2 still counts only squash's own calls.

Live cleanup is a net improvement. Dropping the manual fs.remove of scoped temp dirs is the right fix for the previously-flagged finalizer double-delete — makeTempDirectoryScoped already owns removal. Effect.uninterruptibleMask gives cleanup failure-path execution and interrupt protection the old try/finally couldn't. The SQLSTATE discriminator survives the Effect.tryPromise wrapping: migration-live.ts keeps the foreign error on cause, and up.live.test.ts still gates on 42P01. repair.live.test.ts's Effect.suspend around the versionReverted check is correct — a plain ternary would have captured false at construction.

No floating effects, and enabling the lint on migration/** now guarantees that structurally going forward, since floating-effect-in-vitest is an error and the glob covers the co-located test files.

Worth calling out that this PR carries zero lint suppressions and zero file exclusions — the family is genuinely covered, not bypassed.

PR description correction

The description says "cause assertions move to Cause.pretty, and the ones that read payload fields use typed predicates instead, since Cause.pretty drops those fields." That paragraph doesn't describe this diff — there is no Cause.pretty anywhere under apps/cli/src/commands/migration/ (it uses Cause.squash, which preserves payload fields), and both former JSON.stringify(exit.cause) sites became typed predicates. It looks carried over from the sibling sso PR. Since the payload-field-loss caveat is the thing a reviewer would go looking for, it's worth fixing so the description doesn't send the next reader hunting for a risk that isn't here.