diff --git a/.changeset/no-implicit-migration.md b/.changeset/no-implicit-migration.md new file mode 100644 index 00000000..10d5c726 --- /dev/null +++ b/.changeset/no-implicit-migration.md @@ -0,0 +1,33 @@ +--- +"@taskless/cli": patch +--- + +`check`, `verify` and `test` no longer migrate `.taskless/` as a side effect of +reading it. + +Migration `0005` moves and deletes tracked files, and these three commands +performed it on the way to doing their real work. So a command whose whole job +is to report rewrote the repository, with nothing on the human path to say so: +the diff landed in whatever commit came next, and in CI it ran on every +checkout. + +It also made a migration impossible to verify. Comparing findings before and +after cannot be done when asking the question performs the change, so a +migration that silently dropped a rule could not be caught by the one check +that would catch it. + +These commands now refuse a project whose scaffold is behind, name +`taskless init` as the fix, and leave the working tree untouched. The refusal +carries `SCAFFOLD_MIGRATION_REQUIRED` on the `--json` envelope, distinct from +the existing `SCAFFOLD_VERSION_MISMATCH`, which is the opposite direction and +asks the caller to upgrade the CLI instead. + +The cost is a wall the user meets once after an upgrade, where before they met +nothing. That is the visible version of the same event. + +`init --json` is new, and carries the `migrated` field that `check`, `verify` +and `test` used to report. The field followed the behaviour rather than being +dropped: a CI script still needs to know the working tree was rewritten and +what moved. It is gone from those three envelopes, where it can no longer +occur; it was always optional and conditional, so nothing that read it +correctly breaks. diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 8824423b..a696f2c4 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -1,4 +1,4 @@ -import { resolve, join, isAbsolute, relative } from "node:path"; +import { resolve, isAbsolute, relative } from "node:path"; import { stat } from "node:fs/promises"; import { defineCommand } from "citty"; @@ -6,15 +6,16 @@ import { hasValeRules, runEngines } from "../rules/dispatch"; import { assembleEngineConfigs } from "../rules/assemble"; import { splitRawArguments } from "../util/argv"; import { formatText } from "../util/format"; -import { ensureTasklessDirectory } from "../filesystem/directory"; import { listRuleIds, planEngineDispatch } from "../rules/engines"; import { getTelemetry } from "../telemetry"; import { outputSchema as checkOutputSchema } from "../schemas/check"; -import { makeErrorEnvelope } from "../types/errors"; +import { makeErrorEnvelope, writeJsonError } from "../types/errors"; +import { CLIError } from "../util/cli-error"; import { getToken } from "../auth/token"; import { resolveOrgSubject } from "../auth/org"; import { resolveRepositoryUrl } from "../util/git-remote"; import { getCliPrefix } from "../util/package-manager"; +import { requireCurrentSchema } from "../filesystem/migrate"; import { reconcile } from "../api/reconcile"; import type { ReconcileResponse } from "../api/reconcile"; import { restoreRule } from "../api/restore"; @@ -404,35 +405,36 @@ export const checkCommand = defineCommand({ return; } - // Rules dispatch by the engine directory that contains them. This is also - // the migration trigger: no config is generated on the check path any - // more, so without this call an upgraded CLI would keep reading a stale - // layout. + // REFUSES rather than migrates. This used to call + // `ensureTasklessDirectory`, so a command whose entire job is to report + // rewrote the repository as a side effect: `0005` moves and deletes + // tracked files, and the change landed in whatever commit came next. In + // CI it ran on every checkout. // - // Only an existing `.taskless/` is migrated. `ensureTasklessDirectory` - // creates the scaffold, and `check` is a read-only command — running it in - // a project that has none should report that, not write one (and not fail - // on a read-only filesystem). - // - // The report is carried into the `--json` envelope below: migrating - // rewrites files in the caller's working tree, and a consumer reading - // `{"success":true}` would otherwise have nothing to attribute that diff - // to. - const migrated = (await pathExists(join(cwd, ".taskless"))) - ? await ensureTasklessDirectory(cwd, { - // Suppressed under `--json` for the same reason every other notice - // in this command is: the information is on the envelope's - // `migrated` field, and a machine consumer reading stderr gets - // prose it cannot parse. This one grew from a single line to a - // file-by-file summary, so leaving it ungated would hand a CI - // script that logs or fails on stderr a much noisier surprise than - // the one-liner it tolerated before. - onNotice: (message: string) => { - if (!args.json) console.error(message); - }, - }) - : undefined; - const migratedField = migrated === undefined ? {} : { migrated }; + // It also made a migration unverifiable. Comparing findings before and + // after is impossible when asking the question performs the change, so + // a migration that silently dropped a rule could not be caught by the + // one check that would catch it. + try { + await requireCurrentSchema(cwd); + } catch (error) { + // Handled here rather than left to the outer handler, which prints + // prose: `--json` callers branch on the code, and this refusal asks + // for a different response from a scan that blew up. + if (error instanceof CLIError) { + if (args.json) { + // `requireCurrentSchema` always sets a code, so the fallback is + // dead either way — which is exactly why the two call sites had + // drifted to different dead values. One helper, one answer. + writeJsonError(error.code ?? "INTERNAL_ERROR", error.message); + } else { + console.error(`Error: ${error.message}`); + } + process.exitCode = 1; + return; + } + throw error; + } const dispatch = await planEngineDispatch(cwd); // Static rules (trusted ast-grep YAML) always run; runtime rules @@ -473,7 +475,6 @@ export const checkCommand = defineCommand({ checkOutputSchema.parse({ success: true, results: [], - ...migratedField, }) ) ); @@ -537,7 +538,6 @@ export const checkCommand = defineCommand({ const output = checkOutputSchema.parse({ success: exitCode === 0, results, - ...migratedField, ...(plan.skipped.length > 0 ? { skipped: plan.skipped } : {}), ...(dispatched.failures.length > 0 ? { failures: dispatched.failures } @@ -559,10 +559,17 @@ export const checkCommand = defineCommand({ } } catch (error) { const message = `Error: ${error instanceof Error ? error.message : String(error)}`; + // A `CLIError` already carries the code an agent branches on, and + // flattening every failure to `SCAN_FAILED` threw it away. The scaffold + // refusal is the case that made this visible: "migrate your project" and + // "the scan blew up" want different responses and were arriving as the + // same one. + const code = + error instanceof CLIError + ? (error.code ?? "SCAN_FAILED") + : "SCAN_FAILED"; if (args.json) { - console.log( - JSON.stringify(makeErrorEnvelope("SCAN_FAILED", message)) - ); + console.log(JSON.stringify(makeErrorEnvelope(code, message))); } else { console.error(message); } diff --git a/packages/cli/src/commands/init.ts b/packages/cli/src/commands/init.ts index 26a48dd0..58cd4c86 100644 --- a/packages/cli/src/commands/init.ts +++ b/packages/cli/src/commands/init.ts @@ -31,6 +31,7 @@ import { stampNewProjectRules, } from "../rules/reconcile-marker"; import { readManifest } from "../filesystem/migrate"; +import type { MigrationReport } from "../filesystem/migrate"; import { TASKLESS_DIRECTORY } from "../rules/vale/formats"; import { CLIError } from "../util/cli-error"; import { makeErrorEnvelope } from "../types/errors"; @@ -55,6 +56,12 @@ export const initCommand = defineCommand({ alias: "d", description: "Working directory", }, + json: { + type: "boolean", + description: + "Emit the install result as JSON, including what a migration moved", + default: false, + }, "no-interactive": { type: "boolean", description: @@ -88,12 +95,27 @@ export const initCommand = defineCommand({ } const result = await runNonInteractive(cwd); - if (result.reloadNotice !== undefined) { - console.log(result.reloadNotice); + if (args.json) { + console.log( + JSON.stringify({ + success: true, + commandsInstalled: result.commandsInstalled, + // Absent when nothing ran, so a caller distinguishes "the tree was + // rewritten" from "nothing happened" by presence, never by reading + // empty arrays out of it. + ...(result.migrated === undefined + ? {} + : { migrated: result.migrated }), + }) + ); + } else { + if (result.reloadNotice !== undefined) { + console.log(result.reloadNotice); + } + console.log( + getOnboardTrailer({ commandsInstalled: result.commandsInstalled }) + ); } - console.log( - getOnboardTrailer({ commandsInstalled: result.commandsInstalled }) - ); // Concrete state event: skills/commands were installed (non-interactive). telemetry.capture("cli_installed"); }, @@ -223,9 +245,11 @@ export const updateCommand = defineCommand({ }, }); -async function runNonInteractive( - cwd: string -): Promise<{ commandsInstalled: boolean; reloadNotice: string | undefined }> { +async function runNonInteractive(cwd: string): Promise<{ + commandsInstalled: boolean; + reloadNotice: string | undefined; + migrated: MigrationReport | undefined; +}> { // Sampled BEFORE the directory is created, and that order is the whole // point. `ensureTasklessDirectory` mkdir -p's, so afterwards a pre-existing // project is indistinguishable from a fresh one. @@ -235,7 +259,11 @@ async function runNonInteractive( // never walked the ledger as fully reconciled and skip every entry, which is // the silent skip this feature exists to prevent. const wasNewProject = !(await pathExists(join(cwd, TASKLESS_DIRECTORY))); - await ensureTasklessDirectory(cwd); + // `init` is now the ONLY command that migrates, so it is the only one that + // can report what a migration moved. `check`, `verify` and `test` used to + // carry this on their own envelopes and refuse rather than migrate now, so + // the field followed the behaviour rather than being dropped. + const migrated = await ensureTasklessDirectory(cwd); if (wasNewProject) { // A project this CLI just created has no entries to walk: everything the // ledger describes is already true of the scaffold it wrote. @@ -335,7 +363,7 @@ async function runNonInteractive( } } - return { commandsInstalled, reloadNotice }; + return { commandsInstalled, reloadNotice, migrated }; } function groupValuesByTarget( diff --git a/packages/cli/src/commands/verify.ts b/packages/cli/src/commands/verify.ts index fd7cd128..121a4a27 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -3,6 +3,7 @@ import { resolve } from "node:path"; import { defineCommand } from "citty"; import { ensureTasklessDirectory } from "../filesystem/directory"; +import { requireCurrentSchema } from "../filesystem/migrate"; import { testOneRule, verifyOneRule, @@ -15,7 +16,8 @@ import { RuleNotFoundError, } from "../rules/resolve-path"; import { outputSchema as verifyTestOutputSchema } from "../schemas/verify-test"; -import { makeErrorEnvelope } from "../types/errors"; +import { makeErrorEnvelope, writeJsonError } from "../types/errors"; +import { CLIError } from "../util/cli-error"; /** * The shared body of `verify` and `test`. @@ -38,17 +40,39 @@ async function runOverPath(options: { }): Promise { const { cwd, target, json, label, run } = options; - // Both commands need a current layout before a path means anything, so this - // migrates as a precondition. The report is what lets the run say it did: - // carried into the `--json` envelope below, and printed for a person. - const migrated = await ensureTasklessDirectory(cwd, { - // Suppressed under `--json` for the same reason every other notice - // in this command is: the information is on the envelope's - // `migrated` field, and a machine consumer reading stderr gets - // prose it cannot parse. This one grew from a single line to a - // file-by-file summary, so leaving it ungated would hand a CI - // script that logs or fails on stderr a much noisier surprise than - // the one-liner it tolerated before. + // REFUSES rather than migrates, for both `verify` and `test`. + // + // A current layout is still a precondition — a path means nothing against + // the wrong tree — but establishing it by migrating made two reporting + // commands rewrite the repository. `0005` moves and deletes tracked files, + // and it happened with nothing on the human path to say so, landing in + // whatever commit came next. + // + // A wall the user meets once after an upgrade is the visible version of the + // same cost, and it is the trade this CLI already makes elsewhere: refuse, + // and name the thing that fixes it. + try { + await requireCurrentSchema(cwd); + } catch (error) { + if (error instanceof CLIError) { + if (json) { + writeJsonError(error.code ?? "INTERNAL_ERROR", error.message); + } else { + console.error(`Error: ${error.message}`); + } + process.exitCode = 1; + return; + } + throw error; + } + // Still creates a scaffold that is absent. That writes a fresh directory + // rather than rewriting an existing one, so it is not the refusal above. + // + // The notice stays suppressed under `--json`. Scaffolding a brand-new + // project runs every migration from 0, and its file-by-file summary went to + // stderr unconditionally once this call lost its handler — handing a machine + // consumer prose it cannot parse, on the one path that still writes. + await ensureTasklessDirectory(cwd, { onNotice: (message: string) => { if (!json) console.error(message); }, @@ -86,7 +110,6 @@ async function runOverPath(options: { verifyTestOutputSchema.parse({ ok: true, rules: [], - ...(migrated === undefined ? {} : { migrated }), }) ) ); @@ -109,7 +132,6 @@ async function runOverPath(options: { verifyTestOutputSchema.parse({ ok: failed.length === 0, rules: results, - ...(migrated === undefined ? {} : { migrated }), }) ) ); diff --git a/packages/cli/src/filesystem/migrate.ts b/packages/cli/src/filesystem/migrate.ts index 02ea5fd9..c34ffa2c 100644 --- a/packages/cli/src/filesystem/migrate.ts +++ b/packages/cli/src/filesystem/migrate.ts @@ -2,6 +2,8 @@ import { readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { CLIError } from "../util/cli-error"; +import { buildInvocation } from "../util/invocation"; +import { pathExists } from "../rules/reconcile-marker"; import type { Migrations } from "./types"; import { diffSnapshots, snapshotPaths, type TreeChanges } from "./snapshot"; import init from "./migrations/0001-init"; @@ -306,6 +308,87 @@ export interface RunMigrationsOptions { export const LATEST_SCHEMA_VERSION: number = sortedMigrations(migrations).at(-1)?.[0] ?? 0; +/** + * What a migration WOULD do, without doing it. + * + * Read-only on purpose. A command that reports on a project must be able to + * find out that the project is behind without changing it, and until this + * existed the only way to learn the version was to run the migration that + * changes it. That made a migration unverifiable by construction: comparing + * findings before and after is impossible when asking the question performs + * the change, so a migration that silently dropped a rule could not be caught + * by the one check that would catch it. + * + * `undefined` means nothing is pending, which includes a project newer than + * this CLI understands. That case is a different failure with its own message, + * and it is {@link runMigrations}' to report. + */ +export async function pendingMigration( + cwd: string +): Promise<{ from: number; to: number } | undefined> { + const tasklessDirectory = join(cwd, ".taskless"); + // Keyed on the DIRECTORY, not the manifest. A `.taskless/` with no manifest + // reads as version 0, which is behind — and it is exactly the case that must + // not be waved through, because its tree is the pre-`0004` layout that a + // current CLI finds no rules in. Waving it through would report "no rules + // configured" for a project full of them, which is the silent answer this + // whole change exists to stop giving. + // + // A project with no `.taskless/` at all has nothing to migrate and is not + // this function's business. + if (!(await pathExists(tasklessDirectory))) return undefined; + const { version } = await readRawManifest(tasklessDirectory); + if (version >= LATEST_SCHEMA_VERSION) return undefined; + return { from: version, to: LATEST_SCHEMA_VERSION }; +} + +/** + * Refuse to read a project whose scaffold is behind this CLI, and say what to + * run. + * + * `check` and `verify` used to migrate as a precondition, which made two + * read-only commands rewrite the repository: `0005` moves and deletes tracked + * files, and it did so with no output on the human path unless someone was + * reading stderr closely. The change landed in whatever commit came next, and + * in CI it ran on every checkout. + * + * A wall the user hits once after an upgrade is the worse-sounding option and + * the better one. It is visible, it happens when they are looking, and it is + * the same trade this CLI already makes for an unsupported request and the + * service makes for a client below the version floor: refuse, and name the + * thing that fixes it. + */ +export async function requireCurrentSchema(cwd: string): Promise { + // BOTH directions. `check` used to reach `runMigrations` through + // `ensureTasklessDirectory`, which is where the newer-than-this-CLI refusal + // lived; it does not any more, so this is the only precondition left and it + // has to carry that case too. Without it a version-99 scaffold read as + // "nothing pending" and `check` reported "No rules configured" for a project + // whose layout it simply could not parse — the same silent answer this + // change exists to stop giving, reintroduced by the change itself. + const tasklessDirectory = join(cwd, ".taskless"); + if (await pathExists(tasklessDirectory)) { + const { version } = await readRawManifest(tasklessDirectory); + if (version > LATEST_SCHEMA_VERSION && !hasVersionMismatchOverride()) { + throw new CLIError( + `This project's .taskless/ scaffold is version ${String(version)}, but this CLI only understands version ${String(LATEST_SCHEMA_VERSION)}. ` + + `Upgrade the CLI to continue, or re-run with ${ALLOW_VERSION_MISMATCHES_FLAG} to proceed without migrating.`, + "SCAFFOLD_VERSION_MISMATCH" + ); + } + } + + const pending = await pendingMigration(cwd); + if (pending === undefined) return; + throw new CLIError( + `This project's .taskless/ is at schema version ${String(pending.from)}, and this ` + + `CLI expects ${String(pending.to)}. Migrating moves and deletes files, so it is ` + + `not done as a side effect of a command that only reads.\n\n` + + `Run \`${buildInvocation()} init\` to migrate, then run this again.`, + "SCAFFOLD_MIGRATION_REQUIRED" + ); +} + /** * Run any pending migrations against the .taskless/ directory. * Reads the current version from taskless.json and runs migrations diff --git a/packages/cli/src/schemas/check.ts b/packages/cli/src/schemas/check.ts index 35d4b5b7..68afea9f 100644 --- a/packages/cli/src/schemas/check.ts +++ b/packages/cli/src/schemas/check.ts @@ -1,7 +1,5 @@ import { z } from "zod"; -import { migratedSchema } from "./migration"; - /** Schema for a single check result */ const checkResultSchema = z.object({ source: z.string().describe("Scanner that produced this result"), @@ -51,12 +49,6 @@ export const outputSchema = z.object({ .array(z.string()) .optional() .describe("Advisory messages: engines that could not run"), - // `check` migrates `.taskless/` before it can dispatch, and that rewrites - // files in the working tree. Absent unless it happened, so a consumer reads - // presence rather than guessing from an empty list. - migrated: migratedSchema - .optional() - .describe("Present only when this run migrated the .taskless/ layout"), }); /** Error schema for `taskless check --json` on failure */ diff --git a/packages/cli/src/schemas/verify-test.ts b/packages/cli/src/schemas/verify-test.ts index 587cbc35..04a92817 100644 --- a/packages/cli/src/schemas/verify-test.ts +++ b/packages/cli/src/schemas/verify-test.ts @@ -1,7 +1,5 @@ import { z } from "zod"; -import { migratedSchema } from "./migration"; - /** * One rule's verdict, as `verify` and `test` both report it. * @@ -30,9 +28,4 @@ const ruleResultSchema = z.object({ export const outputSchema = z.object({ ok: z.boolean(), rules: z.array(ruleResultSchema).describe("Per-rule results"), - // Both commands migrate `.taskless/` before resolving a path, which rewrites - // files in the working tree. Absent unless it happened. - migrated: migratedSchema - .optional() - .describe("Present only when this run migrated the .taskless/ layout"), }); diff --git a/packages/cli/src/types/errors.ts b/packages/cli/src/types/errors.ts index 635d3351..51aaf6b9 100644 --- a/packages/cli/src/types/errors.ts +++ b/packages/cli/src/types/errors.ts @@ -36,6 +36,13 @@ export type CLIErrorCode = | "ENGINE_UNAVAILABLE" | "RECONCILE_FAILED" | "SCAFFOLD_VERSION_MISMATCH" + // The mirror of the code above, and a different remedy. That one is a + // scaffold NEWER than this CLI understands, which asks the caller to upgrade + // the CLI. This one is a scaffold BEHIND it, which asks them to migrate the + // project — and it exists as its own code because `check` and `verify` no + // longer migrate as a side effect of reading, so an agent needs to tell + // "run the migration" apart from "your CLI is too old". + | "SCAFFOLD_MIGRATION_REQUIRED" | "SCAFFOLD_CONFLICT" | "INTERNAL_ERROR"; diff --git a/packages/cli/test/anonymous-flag.test.ts b/packages/cli/test/anonymous-flag.test.ts index 489e82fe..1e1347b3 100644 --- a/packages/cli/test/anonymous-flag.test.ts +++ b/packages/cli/test/anonymous-flag.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { promisify } from "node:util"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { migrateFixture } from "./support/current-project"; const execFileAsync = promisify(execFile); const binPath = resolve(import.meta.dirname, "../dist/index.js"); @@ -13,6 +14,8 @@ async function runCli( env?: Record, spawnCwd?: string ): Promise<{ stdout: string; stderr: string; exitCode: number }> { + await migrateFixture(args); + try { const { stdout, stderr } = await execFileAsync("node", [binPath, ...args], { env: { ...process.env, ...env }, diff --git a/packages/cli/test/check-gitignore.test.ts b/packages/cli/test/check-gitignore.test.ts index 8d0e6ba4..79d80283 100644 --- a/packages/cli/test/check-gitignore.test.ts +++ b/packages/cli/test/check-gitignore.test.ts @@ -12,6 +12,7 @@ import { listGitIgnoredEntries, } from "../src/rules/git-ignored"; import { findValeBinary } from "../src/rules/vale/binary"; +import { migrateFixture } from "./support/current-project"; const execFileAsync = promisify(execFile); const binPath = resolve(import.meta.dirname, "../dist/index.js"); @@ -46,6 +47,8 @@ const fixturesDirectory = resolve( async function runCli( arguments_: string[] ): Promise<{ stdout: string; exitCode: number }> { + await migrateFixture(arguments_); + try { const { stdout } = await execFileAsync("node", [binPath, ...arguments_]); return { stdout, exitCode: 0 }; diff --git a/packages/cli/test/check.test.ts b/packages/cli/test/check.test.ts index 8b7d8e6f..c56e490f 100644 --- a/packages/cli/test/check.test.ts +++ b/packages/cli/test/check.test.ts @@ -4,6 +4,7 @@ import { resolve, join } from "node:path"; import { tmpdir } from "node:os"; import { promisify } from "node:util"; import { describe, expect, it, beforeEach, afterEach } from "vitest"; +import { migrateFixture } from "./support/current-project"; const execFileAsync = promisify(execFile); const binPath = resolve(import.meta.dirname, "../dist/index.js"); @@ -16,6 +17,8 @@ const fixturesDirectory = resolve( async function runCli( args: string[] ): Promise<{ stdout: string; stderr: string; exitCode: number }> { + await migrateFixture(args); + try { const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); return { stdout, stderr, exitCode: 0 }; diff --git a/packages/cli/test/engine-dispatch.test.ts b/packages/cli/test/engine-dispatch.test.ts index 1522e8ec..3854690c 100644 --- a/packages/cli/test/engine-dispatch.test.ts +++ b/packages/cli/test/engine-dispatch.test.ts @@ -37,6 +37,7 @@ import { } from "../src/rules/runtime/run-set"; import type { GeneratedRule } from "../src/api/rules"; import { CLIError } from "../src/util/cli-error"; +import { migrateFixture } from "./support/current-project"; const execFileAsync = promisify(execFile); const binPath = resolve(import.meta.dirname, "../dist/index.js"); @@ -71,6 +72,8 @@ const RUNTIME_CHECK = "export default async function () {\n return [];\n}\n"; async function runCli( args: string[] ): Promise<{ stdout: string; stderr: string; exitCode: number }> { + await migrateFixture(args); + try { const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); return { stdout, stderr, exitCode: 0 }; diff --git a/packages/cli/test/example-project.test.ts b/packages/cli/test/example-project.test.ts index dc72d0e8..566f8bae 100644 --- a/packages/cli/test/example-project.test.ts +++ b/packages/cli/test/example-project.test.ts @@ -6,6 +6,7 @@ import { promisify } from "node:util"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { findValeBinary } from "../src/rules/vale/binary"; +import { migrateFixture } from "./support/current-project"; const execFileAsync = promisify(execFile); const binPath = resolve(import.meta.dirname, "../dist/index.js"); @@ -37,6 +38,8 @@ afterEach(async () => { }); async function runCli(args: string[]) { + await migrateFixture(args); + try { const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); return { stdout, stderr, exitCode: 0 }; diff --git a/packages/cli/test/migrate-round-trip.test.ts b/packages/cli/test/migrate-round-trip.test.ts index 28950ddc..c659fd64 100644 --- a/packages/cli/test/migrate-round-trip.test.ts +++ b/packages/cli/test/migrate-round-trip.test.ts @@ -7,6 +7,7 @@ import { promisify } from "node:util"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { findValeBinary } from "../src/rules/vale/binary"; +import { migrateFixture } from "./support/current-project"; const execFileAsync = promisify(execFile); const binPath = resolve(import.meta.dirname, "../dist/index.js"); @@ -59,6 +60,11 @@ async function sha256(path: string): Promise { } async function runCli(args: string[]) { + // The migration is EXPLICIT now. `check` and `verify` used to perform it on + // the way to reporting, which is what this file leaned on; they refuse + // instead, so the upgrade happens here, where the subject of these tests + // begins: a project that HAS been migrated still reports. + await migrateFixture(args); try { const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); return { stdout, stderr, exitCode: 0 }; diff --git a/packages/cli/test/migrated-envelope.test.ts b/packages/cli/test/migrated-envelope.test.ts index 55e91585..e0e36a48 100644 --- a/packages/cli/test/migrated-envelope.test.ts +++ b/packages/cli/test/migrated-envelope.test.ts @@ -1,5 +1,12 @@ import { execFile } from "node:child_process"; -import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { + mkdir, + mkdtemp, + readFile, + rm, + stat, + writeFile, +} from "node:fs/promises"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { promisify } from "node:util"; @@ -59,7 +66,7 @@ function expectSeededMigration(migrated: unknown): void { expect(field.files.modified).toContain(".taskless/taskless.json"); } -describe("the migrated field on the --json envelope", () => { +describe("who migrates, and who refuses", () => { let temporaryDirectory: string; let tasklessDirectory: string; @@ -93,44 +100,82 @@ describe("the migrated field on the --json envelope", () => { ); } - for (const command of ["check", "verify", "test"] as const) { - it(`${command} --json reports the migration it performed`, async () => { - await seedVersion3(); + it("init --json reports the migration it performed", async () => { + // `init` is the only command that migrates now, so it is the only one that + // can report one. The field moved with the behaviour rather than being + // dropped: a CI script still needs to know the working tree was rewritten + // and what moved. + await seedVersion3(); - const { stdout } = await runCli([ - command, - "--json", - "-d", - temporaryDirectory, - ]); + const { stdout } = await runCli([ + "init", + "--no-interactive", + "--json", + "-d", + temporaryDirectory, + ]); + + const envelope = parseEnvelope(stdout); + expect(envelope.migrated).toBeDefined(); + expectSeededMigration(envelope.migrated); + }); + + it("init --json omits the field when nothing migrated", async () => { + // Absence is the signal, so a consumer never reads empty arrays to decide. + await seedVersion3(); + await runCli(["init", "--no-interactive", "-d", temporaryDirectory]); - const envelope = parseEnvelope(stdout); - expect(envelope.migrated).toBeDefined(); - expectSeededMigration(envelope.migrated); - }); + const { stdout } = await runCli([ + "init", + "--no-interactive", + "--json", + "-d", + temporaryDirectory, + ]); - it(`${command} --json omits the field when nothing migrated`, async () => { + expect(parseEnvelope(stdout)).not.toHaveProperty("migrated"); + }); + + it.each(["check", "verify", "test"] as const)( + "%s refuses a project behind the CLI rather than migrating it", + async (command) => { + // The behaviour this file used to assert, inverted. These commands + // report; migrating as a side effect meant a read rewrote the + // repository, and it made a migration unverifiable, since asking the + // question performed the change. await seedVersion3(); - // First run migrates; the second finds the scaffold current. Absence is - // the signal, so a consumer never has to read empty arrays to decide. - await runCli([command, "--json", "-d", temporaryDirectory]); - const { stdout } = await runCli([ + const { stdout, stderr } = await runCli([ command, "--json", "-d", temporaryDirectory, ]); - const envelope = parseEnvelope(stdout); - expect(envelope).not.toHaveProperty("migrated"); - }); - } + const output = `${stdout}${stderr}`; + expect(output).toContain("SCAFFOLD_MIGRATION_REQUIRED"); + expect(output).toMatch(/init/); + + // And it left the project alone: still version 3, rule still flat. + const manifest = JSON.parse( + await readFile(join(tasklessDirectory, "taskless.json"), "utf8") + ) as { version: number }; + expect(manifest.version).toBe(SEEDED_FROM); + await expect( + stat(join(tasklessDirectory, "rules", "no-eval.yml")) + ).resolves.toBeDefined(); + } + ); it("names the versions and the files on human stderr", async () => { await seedVersion3(); - const { stderr } = await runCli(["check", "-d", temporaryDirectory]); + const { stderr } = await runCli([ + "init", + "--no-interactive", + "-d", + temporaryDirectory, + ]); const span = `from schema version ${String(SEEDED_FROM)} to ${String(LATEST_SCHEMA_VERSION)}`; expect(stderr).toContain(`Migrating .taskless/ ${span}`); @@ -141,9 +186,14 @@ describe("the migrated field on the --json envelope", () => { it("says nothing on stderr when the scaffold is already current", async () => { await seedVersion3(); - await runCli(["check", "-d", temporaryDirectory]); - - const { stderr } = await runCli(["check", "-d", temporaryDirectory]); + await runCli(["init", "--no-interactive", "-d", temporaryDirectory]); + + const { stderr } = await runCli([ + "init", + "--no-interactive", + "-d", + temporaryDirectory, + ]); expect(stderr).not.toContain("Migrat"); }); diff --git a/packages/cli/test/mixed-engine-check.test.ts b/packages/cli/test/mixed-engine-check.test.ts index 363190e9..2304d5ec 100644 --- a/packages/cli/test/mixed-engine-check.test.ts +++ b/packages/cli/test/mixed-engine-check.test.ts @@ -7,6 +7,7 @@ import { promisify } from "node:util"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { findValeBinary } from "../src/rules/vale/binary"; +import { migrateFixture } from "./support/current-project"; const execFileAsync = promisify(execFile); const binPath = resolve(import.meta.dirname, "../dist/index.js"); @@ -32,6 +33,8 @@ const fixturesDirectory = resolve( async function runCli( args: string[] ): Promise<{ stdout: string; stderr: string; exitCode: number }> { + await migrateFixture(args); + try { const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); return { stdout, stderr, exitCode: 0 }; diff --git a/packages/cli/test/no-implicit-migration.test.ts b/packages/cli/test/no-implicit-migration.test.ts new file mode 100644 index 00000000..d743cd02 --- /dev/null +++ b/packages/cli/test/no-implicit-migration.test.ts @@ -0,0 +1,243 @@ +import { execFile } from "node:child_process"; +import { + mkdir, + mkdtemp, + readFile, + rm, + stat, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { promisify } from "node:util"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { LATEST_SCHEMA_VERSION } from "../src/filesystem/migrate"; + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); + +/** + * A command that reports must not rewrite the repository. + * + * `check`, `verify` and `test` used to migrate `.taskless/` on their way to + * doing their real work. `0005` moves and deletes tracked files, so running a + * read-only command changed the working tree, with nothing on the human path to + * say so — the diff landed in whatever commit came next, and in CI it happened + * on every checkout. + * + * It also made a migration unverifiable. Comparing findings before and after is + * impossible when asking the question performs the change, so a migration that + * silently dropped a rule could not be caught by the one check that would catch + * it. + * + * The cost is a wall the user meets once after an upgrade, where before they + * met nothing. That is the visible version of the same event, and it is the + * trade this CLI already makes for an unsupported request. + */ + +async function runCli( + args: string[] +): Promise<{ stdout: string; stderr: string; exitCode: number }> { + try { + const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); + return { stdout, stderr, exitCode: 0 }; + } catch (error) { + const failure = error as { + stdout?: string; + stderr?: string; + code?: number; + }; + return { + stdout: failure.stdout ?? "", + stderr: failure.stderr ?? "", + exitCode: failure.code ?? 1, + }; + } +} + +const FLAT_RULE = + "id: no-eval\nlanguage: TypeScript\nseverity: error\nmessage: no eval\nrule:\n pattern: eval($A)\n"; + +describe("a reporting command never migrates", () => { + let directory: string; + let taskless: string; + + beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), "tskl-no-migrate-")); + taskless = join(directory, ".taskless"); + await mkdir(join(taskless, "rules"), { recursive: true }); + await writeFile( + join(taskless, "taskless.json"), + JSON.stringify({ version: 3, install: {} }), + "utf8" + ); + await writeFile(join(taskless, "rules", "no-eval.yml"), FLAT_RULE, "utf8"); + }); + + afterEach(async () => { + await rm(directory, { recursive: true, force: true }); + }); + + it.each(["check", "verify", "test"] as const)( + "%s leaves the working tree exactly as it found it", + async (command) => { + // The whole point. Not "it warns" — it must not have MOVED anything. + const { exitCode } = await runCli([command, "-d", directory]); + expect(exitCode).toBe(1); + + const manifest = JSON.parse( + await readFile(join(taskless, "taskless.json"), "utf8") + ) as { version: number }; + expect(manifest.version).toBe(3); + await expect( + stat(join(taskless, "rules", "no-eval.yml")) + ).resolves.toBeDefined(); + // And nothing was created where the migration would have put it. + await expect(stat(join(taskless, "rules", "sg"))).rejects.toThrow(); + } + ); + + it.each(["check", "verify", "test"] as const)( + "%s names the command that fixes it", + async (command) => { + // A refusal that does not say what to run is just a wall. + const { stderr } = await runCli([command, "-d", directory]); + expect(stderr).toContain("schema version 3"); + expect(stderr).toContain(String(LATEST_SCHEMA_VERSION)); + expect(stderr).toMatch(/init/); + } + ); + + it.each(["check", "verify", "test"] as const)( + "%s --json carries the code an agent branches on", + async (command) => { + const { stdout } = await runCli([command, "--json", "-d", directory]); + const envelope = JSON.parse(stdout.trim().split("\n").at(-1) ?? "{}") as { + ok?: boolean; + code?: string; + }; + expect(envelope.ok).toBe(false); + // Distinct from SCAFFOLD_VERSION_MISMATCH, which is the opposite + // direction and asks the caller to upgrade the CLI instead. + expect(envelope.code).toBe("SCAFFOLD_MIGRATION_REQUIRED"); + } + ); + + it.each(["check", "verify", "test"] as const)( + "%s still refuses a scaffold NEWER than the CLI", + async (command) => { + // The opposite direction, and a regression this change introduced once. + // `check` used to reach that refusal through `ensureTasklessDirectory`, + // and dropping the call dropped the check with it: a version-99 scaffold + // read as "nothing pending" and `check` reported "No rules configured" + // for a layout it could not parse. + await writeFile( + join(taskless, "taskless.json"), + JSON.stringify({ version: 99, install: {} }), + "utf8" + ); + + const { stdout, stderr } = await runCli([ + command, + "--json", + "-d", + directory, + ]); + const output = `${stdout}${stderr}`; + expect(output).toContain("SCAFFOLD_VERSION_MISMATCH"); + expect(output).toMatch(/Upgrade the CLI/); + } + ); + + it("proceeds past a newer scaffold when explicitly told to", async () => { + // The documented escape hatch has to survive the refusal above, or the + // flag is a promise the CLI stopped keeping. + await writeFile( + join(taskless, "taskless.json"), + JSON.stringify({ version: 99, install: {} }), + "utf8" + ); + + const { stderr } = await runCli([ + "check", + "-d", + directory, + "--allow-version-mismatches", + ]); + expect(stderr).not.toContain("SCAFFOLD_VERSION_MISMATCH"); + }); + + it("still runs against a project that is already current", async () => { + // The refusal is about being BEHIND, not about having a scaffold. + await runCli(["init", "--no-interactive", "-d", directory]); + + const { stderr, exitCode } = await runCli(["check", "-d", directory]); + expect(stderr).not.toContain("schema version"); + expect(exitCode).not.toBe(1); + }); + + it.each([ + ["verify", true], + ["test", true], + ])("%s --json keeps the scaffold migration off stderr", async (command) => { + // The one path that still writes: scaffolding a brand-new project runs + // every migration from 0, and its file-by-file summary is prose a + // machine consumer cannot parse. It went to stderr unconditionally once + // this call lost its notice handler. + const bare = await mkdtemp(join(tmpdir(), "tskl-bare-json-")); + try { + const { stderr } = await runCli([command, "--json", "-d", bare]); + expect(stderr).not.toContain("Migrating .taskless/"); + expect(stderr).not.toContain("Migrated .taskless/"); + } finally { + await rm(bare, { recursive: true, force: true }); + } + }); + + it("still explains the scaffold migration to a person", async () => { + // Suppressed for machines, not removed. Without `--json` the summary is + // the only thing telling someone their working tree just changed. + const bare = await mkdtemp(join(tmpdir(), "tskl-bare-human-")); + try { + const { stderr } = await runCli(["verify", "-d", bare]); + expect(stderr).toContain("Migrating .taskless/"); + } finally { + await rm(bare, { recursive: true, force: true }); + } + }); + + it("says nothing about schemas in a project with no .taskless at all", async () => { + // Nothing to migrate, so nothing to refuse. `check` reports the absence of + // rules, which is its existing behaviour and not this error. + const bare = await mkdtemp(join(tmpdir(), "tskl-bare-")); + try { + const { stderr } = await runCli(["check", "-d", bare]); + expect(stderr).not.toContain("schema version"); + } finally { + await rm(bare, { recursive: true, force: true }); + } + }); + + it("migrates when asked, and reports what moved", async () => { + // The other half of the trade: the migration still happens, on a command + // whose job is to change the project. + const { stdout } = await runCli([ + "init", + "--no-interactive", + "--json", + "-d", + directory, + ]); + + const envelope = JSON.parse(stdout.trim().split("\n").at(-1) ?? "{}") as { + migrated?: { from: number; to: number }; + }; + expect(envelope.migrated?.from).toBe(3); + expect(envelope.migrated?.to).toBe(LATEST_SCHEMA_VERSION); + await expect( + stat(join(taskless, "rules", "sg", "no-eval", "no-eval.yml")) + ).resolves.toBeDefined(); + }); +}); diff --git a/packages/cli/test/repair-integration.test.ts b/packages/cli/test/repair-integration.test.ts index 66277919..64ad1afa 100644 --- a/packages/cli/test/repair-integration.test.ts +++ b/packages/cli/test/repair-integration.test.ts @@ -8,6 +8,7 @@ import { promisify } from "node:util"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { canonicalHash } from "../src/rules/rule-hash"; +import { migrateFixture } from "./support/current-project"; const execFileAsync = promisify(execFile); const binPath = resolve(import.meta.dirname, "../dist/index.js"); @@ -80,6 +81,12 @@ async function runCli( args: string[], env: Record ): Promise<{ stdout: string; exitCode: number }> { + // The fixture writes a current-layout tree but no manifest, and `check` + // refuses a project it cannot confirm is current — a tree without a + // manifest reads as version 0, and it cannot tell this one from a + // pre-`0004` project by looking. So the scaffold is completed here, which + // is what a real project has. + await migrateFixture(args); try { const { stdout } = await execFileAsync("node", [binPath, ...args], { env: { ...process.env, ...env }, diff --git a/packages/cli/test/runtime-check.test.ts b/packages/cli/test/runtime-check.test.ts index 63c151e2..c370cee5 100644 --- a/packages/cli/test/runtime-check.test.ts +++ b/packages/cli/test/runtime-check.test.ts @@ -5,6 +5,7 @@ import { resolve, join } from "node:path"; import { tmpdir } from "node:os"; import { promisify } from "node:util"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { migrateFixture } from "./support/current-project"; const execFileAsync = promisify(execFile); const binPath = resolve(import.meta.dirname, "../dist/index.js"); @@ -71,6 +72,8 @@ async function runCli( args: string[], env: Record = {} ): Promise<{ stdout: string; stderr: string; exitCode: number }> { + await migrateFixture(args); + try { const { stdout, stderr } = await execFileAsync("node", [binPath, ...args], { env: { ...process.env, ...env }, diff --git a/packages/cli/test/support/current-project.ts b/packages/cli/test/support/current-project.ts new file mode 100644 index 00000000..b2b7bbdc --- /dev/null +++ b/packages/cli/test/support/current-project.ts @@ -0,0 +1,34 @@ +import { stat } from "node:fs/promises"; +import { join } from "node:path"; + +import { ensureTasklessDirectory } from "../../src/filesystem/directory"; + +/** + * Bring a test fixture up to the current scaffold version before the CLI reads + * it, the way a user now does. + * + * `check`, `verify` and `test` used to migrate on their way to doing their real + * work, so a fixture written in the pre-`0004` layout was silently modernised + * mid-command and every one of these suites depended on that without saying so. + * Those commands refuse now, which is the point of the change: a command that + * reports must not rewrite the repository, and a migration nobody watches is a + * migration nobody can verify. + * + * So the migration moves into the fixture setup, where it is visible. These + * suites are about what `check` and `verify` REPORT; the refusal itself has its + * own tests, and putting it here too would only stop them testing their subject. + * + * Only migrates a project that already has a `.taskless/`. Creating one would + * break the several tests whose subject is a project that has none. + */ +export async function migrateFixture(args: string[]): Promise { + const index = args.indexOf("-d"); + const cwd = index === -1 ? undefined : args[index + 1]; + if (cwd === undefined) return; + try { + await stat(join(cwd, ".taskless")); + } catch { + return; + } + await ensureTasklessDirectory(cwd, { onNotice: () => {} }); +}