From 503b0a8b9b2dee17d8d3d92372aade4191285f97 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 1 Sep 2026 00:37:01 -0700 Subject: [PATCH 1/4] fix(check): refuse a project behind the CLI instead of migrating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raised by the generator team as N10, after `check` rewrote their working tree and `git add -A` swept the result into six unrelated pull requests. `check`, `verify` and `test` called `ensureTasklessDirectory`, so three commands whose job is to REPORT performed migration `0005`, which moves and deletes tracked files. Nothing on the human path said so, the diff landed in whatever commit came next, and in CI it ran on every checkout. The argument that settled it is theirs and is not about tidiness: a migration cannot be verified. Comparing findings before and after is impossible when asking the question performs the change, so a migration that silently dropped a rule is invisible to the one check that would catch it. So `pendingMigration` reads the version without touching anything, and `requireCurrentSchema` refuses and names `init`. Keyed on the `.taskless/` DIRECTORY rather than on the manifest file: a directory with no manifest reads as version 0, which is behind, and waving it through would report "no rules configured" for a project full of them — the silent answer this change exists to stop giving. A project with no `.taskless/` at all has nothing to migrate and is unaffected. `SCAFFOLD_MIGRATION_REQUIRED` is a new code rather than a reuse of `SCAFFOLD_VERSION_MISMATCH`. That one is a scaffold NEWER than the CLI, which asks the caller to upgrade the CLI; this asks them to migrate the project, and an agent has to tell them apart. Adding a code is not a breaking change to the contract. The `migrated` field moves to `init --json`, which is new. `init` is now the only command that migrates, so it is the only one that can report one, and a CI script still needs to know the tree was rewritten and what moved. Removed from the other three envelopes, where it can no longer occur; it was always optional and conditional. Two things fell out of the tests. Nine suites were leaning on the implicit migration without saying so — their fixtures are written in the pre-0004 layout — which is the same invisibility, showing up in our own suite. They migrate explicitly now, in setup, where it can be seen. `migrated-envelope.test.ts` was rewritten rather than repaired: its subject moved to `init`, and the inverse property (these commands leave the tree exactly as they found it) is now asserted directly. Also fixes a smaller loss on the way: `check`'s catch flattened every failure to SCAN_FAILED, discarding the code a `CLIError` already carried. "Migrate your project" and "the scan blew up" were arriving as the same answer. --- .changeset/no-implicit-migration.md | 33 ++++ packages/cli/src/commands/check.ts | 81 +++++---- packages/cli/src/commands/init.ts | 48 +++-- packages/cli/src/commands/verify.ts | 51 ++++-- packages/cli/src/filesystem/migrate.ts | 64 +++++++ packages/cli/src/schemas/check.ts | 5 - packages/cli/src/schemas/verify-test.ts | 7 - packages/cli/src/types/errors.ts | 7 + packages/cli/test/anonymous-flag.test.ts | 3 + packages/cli/test/check-gitignore.test.ts | 3 + packages/cli/test/check.test.ts | 3 + packages/cli/test/engine-dispatch.test.ts | 3 + packages/cli/test/example-project.test.ts | 3 + packages/cli/test/migrate-round-trip.test.ts | 6 + packages/cli/test/migrated-envelope.test.ts | 106 ++++++++--- packages/cli/test/mixed-engine-check.test.ts | 3 + .../cli/test/no-implicit-migration.test.ts | 169 ++++++++++++++++++ packages/cli/test/runtime-check.test.ts | 3 + packages/cli/test/support/current-project.ts | 34 ++++ 19 files changed, 530 insertions(+), 102 deletions(-) create mode 100644 .changeset/no-implicit-migration.md create mode 100644 packages/cli/test/no-implicit-migration.test.ts create mode 100644 packages/cli/test/support/current-project.ts 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..5adbe55f 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 { 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,40 @@ 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) { + console.log( + JSON.stringify( + makeErrorEnvelope( + error.code ?? "SCAFFOLD_MIGRATION_REQUIRED", + 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 +479,6 @@ export const checkCommand = defineCommand({ checkOutputSchema.parse({ success: true, results: [], - ...migratedField, }) ) ); @@ -537,7 +542,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 +563,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..2d320c40 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, @@ -16,6 +17,7 @@ import { } from "../rules/resolve-path"; import { outputSchema as verifyTestOutputSchema } from "../schemas/verify-test"; import { makeErrorEnvelope } from "../types/errors"; +import { CLIError } from "../util/cli-error"; /** * The shared body of `verify` and `test`. @@ -38,21 +40,38 @@ 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. - onNotice: (message: string) => { - if (!json) console.error(message); - }, - }); + // 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) { + console.log( + JSON.stringify( + makeErrorEnvelope(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 case above. + await ensureTasklessDirectory(cwd); let rules; try { @@ -86,7 +105,6 @@ async function runOverPath(options: { verifyTestOutputSchema.parse({ ok: true, rules: [], - ...(migrated === undefined ? {} : { migrated }), }) ) ); @@ -109,7 +127,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..27bdae7d 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,68 @@ 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 { + 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..70cf307a 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"), @@ -54,9 +52,6 @@ export const outputSchema = z.object({ // `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..4c45b138 --- /dev/null +++ b/packages/cli/test/no-implicit-migration.test.ts @@ -0,0 +1,169 @@ +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("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("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/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: () => {} }); +} From 81e27c95890e1a66215ee390ac3fc8677d0ee767 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 1 Sep 2026 10:37:00 -0700 Subject: [PATCH 2/4] fix(check): keep refusing a scaffold newer than the CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #234, and the first item is a regression this PR introduced. `check` used to reach the newer-than-this-CLI refusal through `ensureTasklessDirectory` → `runMigrations`. Dropping that call to stop the implicit migration dropped the check with it, and `pendingMigration` only ever asked "is the version BEHIND", so a scaffold ahead of this CLI satisfied `>=` and read as "nothing pending". Reproduced: a version-99 project made `check` report "No rules configured" for a layout it could not parse. That is the same silent answer this change exists to remove, reintroduced by the change itself. `requireCurrentSchema` now carries both directions, since it is the only precondition these commands have left. `--allow-version-mismatches` still works, and is tested, because a documented escape hatch that quietly stops working is worse than never having offered it. `verify` and `test` gain something too: the refusal now reaches their `--json` envelope with its code, where it previously escaped to the top-level handler as prose. Also from review: - Dropped a comment in `schemas/check.ts` describing the `migrated` field that this PR removed. It said `check` migrates before it can dispatch, which is now the opposite of true. The sibling file removed field and comment together; this one kept the comment. - Both refusal sites use the existing `writeJsonError` rather than hand-rolling it. The reviewer's point lands: the two copies had already drifted to different dead fallback codes, which is what duplication does before it does anything worse. --- packages/cli/src/commands/check.ts | 14 +++--- packages/cli/src/commands/verify.ts | 8 +--- packages/cli/src/filesystem/migrate.ts | 19 ++++++++ packages/cli/src/schemas/check.ts | 3 -- .../cli/test/no-implicit-migration.test.ts | 44 +++++++++++++++++++ 5 files changed, 70 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 5adbe55f..a696f2c4 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -9,7 +9,7 @@ import { formatText } from "../util/format"; 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"; @@ -423,14 +423,10 @@ export const checkCommand = defineCommand({ // for a different response from a scan that blew up. if (error instanceof CLIError) { if (args.json) { - console.log( - JSON.stringify( - makeErrorEnvelope( - error.code ?? "SCAFFOLD_MIGRATION_REQUIRED", - error.message - ) - ) - ); + // `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}`); } diff --git a/packages/cli/src/commands/verify.ts b/packages/cli/src/commands/verify.ts index 2d320c40..78fc2e5a 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -16,7 +16,7 @@ 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"; /** @@ -56,11 +56,7 @@ async function runOverPath(options: { } catch (error) { if (error instanceof CLIError) { if (json) { - console.log( - JSON.stringify( - makeErrorEnvelope(error.code ?? "INTERNAL_ERROR", error.message) - ) - ); + writeJsonError(error.code ?? "INTERNAL_ERROR", error.message); } else { console.error(`Error: ${error.message}`); } diff --git a/packages/cli/src/filesystem/migrate.ts b/packages/cli/src/filesystem/migrate.ts index 27bdae7d..c34ffa2c 100644 --- a/packages/cli/src/filesystem/migrate.ts +++ b/packages/cli/src/filesystem/migrate.ts @@ -359,6 +359,25 @@ export async function pendingMigration( * 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( diff --git a/packages/cli/src/schemas/check.ts b/packages/cli/src/schemas/check.ts index 70cf307a..68afea9f 100644 --- a/packages/cli/src/schemas/check.ts +++ b/packages/cli/src/schemas/check.ts @@ -49,9 +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. }); /** Error schema for `taskless check --json` on failure */ diff --git a/packages/cli/test/no-implicit-migration.test.ts b/packages/cli/test/no-implicit-migration.test.ts index 4c45b138..d4180977 100644 --- a/packages/cli/test/no-implicit-migration.test.ts +++ b/packages/cli/test/no-implicit-migration.test.ts @@ -125,6 +125,50 @@ describe("a reporting command never migrates", () => { } ); + 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]); From 2be1b585dd5465fe33c925b79dd08abb9d174416 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 1 Sep 2026 10:41:34 -0700 Subject: [PATCH 3/4] test(repair): give the repair fixture the manifest a real project has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #230 merged while this was in flight, and its integration fixture writes a current-layout tree with no `taskless.json`. A tree without a manifest reads as version 0, so `check` refuses it, and every repair notice the test asserts on went missing. The refusal is right and stays. Nothing can tell a current tree with no manifest from a pre-`0004` project by looking, and the safe answer to "I cannot confirm this is current" is to say so rather than to read it and report whatever falls out — which in that fixture's case was an empty envelope. So the fixture completes its scaffold, the way every other suite here now does. --- packages/cli/test/repair-integration.test.ts | 7 +++++++ 1 file changed, 7 insertions(+) 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 }, From 55a952c20cf071989a3c8070b5734e5cf8cd710d Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 1 Sep 2026 10:43:38 -0700 Subject: [PATCH 4/4] fix(verify): keep the scaffold migration off stderr under --json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #234, and a second regression from this PR. Replacing the migrate-then-report call dropped its `onNotice` handler, so `ensureTasklessDirectory` fell back to writing every notice with `console.error`. On the one path that still writes — scaffolding a project with no `.taskless/` at all — that runs every migration from 0 and puts the file-by-file summary on stderr unconditionally, handing a machine consumer prose it cannot parse. Reproduced on a bare directory: eight lines of migration summary under `--json`, none of it parseable. Suppressed again, and tested both ways, because the point is not silence: without `--json` that summary is the only thing telling someone their working tree just changed. --- packages/cli/src/commands/verify.ts | 13 ++++++-- .../cli/test/no-implicit-migration.test.ts | 30 +++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/verify.ts b/packages/cli/src/commands/verify.ts index 78fc2e5a..121a4a27 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -66,8 +66,17 @@ async function runOverPath(options: { 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 case above. - await ensureTasklessDirectory(cwd); + // 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); + }, + }); let rules; try { diff --git a/packages/cli/test/no-implicit-migration.test.ts b/packages/cli/test/no-implicit-migration.test.ts index d4180977..d743cd02 100644 --- a/packages/cli/test/no-implicit-migration.test.ts +++ b/packages/cli/test/no-implicit-migration.test.ts @@ -178,6 +178,36 @@ describe("a reporting command never migrates", () => { 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.