feat(cli): add tailordb migration validate command#1891
Conversation
Extract the deploy-time TailorDB schema checks (migration file integrity, local types vs. migration snapshot, remote schema vs. migration checkpoint) into a shared validation module and expose them as a read-only 'tailordb migration validate' command with per-namespace reports, --json output, and a non-zero exit code on drift, so the checks can run in CI without applying a deploy.
Extract the remote-drift remediation hints into the shared validation module so deploy and 'tailordb migration validate' print the same guidance, and record why remote verification was skipped (no remote migration label, or no snapshot at the remote migration number) so validate reports skipped checks instead of showing them as passing.
Propagate remote metadata lookup failures instead of treating them as a missing migration label (only NotFound now reads as first apply), report a remote migration checkpoint that does not exist in the local history as a validation failure, and fold malformed migration file contents into the per-namespace report so one broken namespace cannot suppress the reports of the others.
Distinguish an undeployed namespace from a deployed namespace whose migration state is missing when the remote check is skipped, and fail validation when a migration whose diff requires a data migration script has no migrate.ts on disk.
Run the same local TailorDB type-name uniqueness assertion as deploy after loading services, so a schema deploy would deterministically reject cannot pass validation.
Document 'tailordb migration validate' in the schema-verification and CI/CD sections, note the deploy-side lookup-failure propagation in the changeset, and consolidate the migration schema fixtures shared by the sync and validate test suites into a test helper.
🦋 Changeset detectedLatest commit: 8a85291 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
…on-validate # Conflicts: # packages/sdk/docs/cli-reference.md # packages/sdk/docs/cli/tailordb.md # packages/sdk/src/cli/commands/deploy/tailordb/index.ts
commit: |
This comment has been minimized.
This comment has been minimized.
| * @param {string} migrationsDir - Migrations directory path | ||
| * @param {string} namespace - TailorDB namespace (for error messages) | ||
| */ | ||
| function assertRequiredMigrationScripts(migrationsDir: string, namespace: string): void { |
There was a problem hiding this comment.
Is scanning the entire migration history for the migrate.ts requirement intentional?
deploy enforces this contract only for pending migrations, inside detectPendingMigrations (deploy/tailordb/migration.ts:147). So when 0003 was applied and its migrate.ts was deleted afterwards, deploy passes while validate alone fails in CI.
If it is intentional, please align the wording with reality: this JSDoc says "matching the contract deploy enforces for pending migrations", and the command description says "Runs the same checks as deploy".
The gap exists in the other direction too - planTypes publishEvents check (deploy/tailordb/index.ts:1471) does not run under validate - so wording like "runs deploy migration / schema-drift checks" would set a more accurate expectation.
There was a problem hiding this comment.
Yes, scanning the full history is intentional for CI validation. I clarified the command description and generated CLI docs accordingly in d5a0ea1.
| @@ -0,0 +1,468 @@ | |||
| /** | |||
There was a problem hiding this comment.
I would suggest placing this shared module under migrate/ instead.
migrate/validate.ts -> deploy/tailordb/validation.ts adds an edge that reverses the previous deploy -> migrate dependency direction.
Moving it to something like cli/commands/tailordb/migrate/schema-checks.ts keeps the dependency one-way, and it fits the content: most of the imports here are migrate/snapshot and migrate/diff-calculator.
| if (file.type !== "diff") continue; | ||
| const diff = loadDiff(file.path); | ||
| if (!diff.requiresMigrationScript || diff.scriptSkipped) continue; | ||
| if (!fs.existsSync(getMigrationFilePath(migrationsDir, file.number, "migrate"))) { |
There was a problem hiding this comment.
nits: getMigrationFiles(migrationsDir) right above already returns type: "migrate" entries, so collecting them into a Set<number> would remove the extra existsSync.
Behavior is unchanged either way, so purely a preference.
There was a problem hiding this comment.
getMigrationFiles() currently returns only schema and diff entries, so it does not expose the existing migrate.ts files needed to build that set. I’d prefer to keep the direct existsSync check rather than broaden that helper’s contract for this local lookup.
| const tailorDBInputs = application.tailorDBServices.map(toTailorDBDeployInput); | ||
| const typesByNamespace = new Map(tailorDBInputs.map((input) => [input.namespace, input.types])); | ||
|
|
||
| const accessToken = await loadAccessToken({ |
There was a problem hiding this comment.
Could the local checks run before this?
loadAccessToken / loadWorkspaceId throw hard when credentials are missing (shared/context.ts:640-645, :513-517), so a repo with broken migration files currently fails with "Tailor Platform token not found." instead of the actual file error - even though the migration-file and local-schema checks are fully local.
Erroring out when the remote state cannot be read is fine. The point is that the local findings should still be reported.
Moving lines 144-151 and verifyRemoteSchema below the migration-file loop and checkMigrationDiffs, and reporting the local part before propagating a remote failure, would cover it.
…on-validate # Conflicts: # packages/sdk/src/cli/commands/tailordb/migrate/index.ts
This comment has been minimized.
This comment has been minimized.
Code Metrics Report (packages/sdk)
Details | | main (dbcb032) | #1891 (07129f5) | +/- |
|--------------------|----------------|-----------------|-------|
+ | Coverage | 75.0% | 75.1% | +0.0% |
| Files | 462 | 465 | +3 |
| Lines | 17406 | 17538 | +132 |
+ | Covered | 13063 | 13178 | +115 |
- | Code to Test Ratio | 1:0.4 | 1:0.4 | -0.1 |
| Code | 117982 | 118752 | +770 |
+ | Test | 56044 | 56361 | +317 |Code coverage of files in pull request scope (84.7% → 84.9%)
SDK Configure Bundle Size
Runtime Performance
Type Performance (instantiations)
Reported by octocov |
| logger.log( | ||
| ` Remote schema: ${styles.success("OK")} (remote migration: ${formatMigrationNumber(remote.remoteMigrationNumber)})`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
When remoteSchema is undefined every branch above is skipped, so nothing is printed for the remote row at all.
A reader then cannot tell "remote check passed" from "remote check never ran" - the same failure mode the skipped reasons were introduced to avoid.
A trailing branch would keep it consistent:
} else {
logger.log(` Remote schema: ${styles.error("not checked")}`);
}| if (remoteResults === undefined) { | ||
| return { | ||
| namespace: target.namespace, | ||
| valid: false, |
There was a problem hiding this comment.
valid: false here has no counterpart in the JSON payload explaining why.
A --json consumer sees migrationFiles.valid: true and localSchema.hasDiff: false yet still valid: false, with the cause only on stderr.
An explicit marker such as remoteSchema: { skipped: "check_failed" } would reuse the existing skipped semantics and keep the report self-describing.
| }; | ||
|
|
||
| let remoteResults: Awaited<ReturnType<typeof verifyRemoteSchema>>; | ||
| try { |
There was a problem hiding this comment.
nits: when checkableNamespaces is empty (every namespace has invalid migration files) there is nothing to verify remotely, yet loadAccessToken still runs and can throw.
The propagated error then becomes "token not found" rather than the migration file failures that were just collected.
Skipping this block when checkableNamespaces.length === 0 would avoid that.
toiroakr
left a comment
There was a problem hiding this comment.
LGTM.
All four points from the previous round are addressed. The getMigrationFiles one was my mistake - it returns only schema / diff entries, so keeping the direct existsSync is the right call.
Three new nits are posted as inline comments. All are low severity and none blocks merging; a follow-up or a separate PR is fine. They share one theme: extending this PR's own principle of "never show a check that did not run as a pass" to the case where remote verification throws.
Verified locally on 8a85291: vitest 459 passed with no type errors, oxlint clean, docs:check passing, check:import-cycles reporting no cycles, and knip clean.
Summary
Add a read-only
tailordb migration validatecommand that runs the same schema checks asdeploywithout applying anything, so migration problems and schema drift fail CI before a deploy instead of during one.Usage
The command exits with a non-zero code when any check fails, reports issues per namespace with type/field-level detail, and supports
--jsonfor machine-readable output and--namespaceto target a single namespace.Checks
migrate.ts(or a recorded--no-scriptacknowledgment) for every diff that requires a scriptNotes
deployinto a shared module and reused by both paths, so they cannot drift apart.deploybehavior change: remote migration state lookup failures now abort the deploy instead of being treated as a first apply (a not-yet-deployed namespace still is).not_deployed/no_migration_label/no_snapshot) instead of showing it as passing.