From 9cc2f645ae99e029e7d77da636d5cb38bdb535d7 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 26 Aug 2026 12:26:22 -0700 Subject: [PATCH 1/3] feat(cli): say on the envelope that the layout was migrated `check`, `verify`, and `test` migrate `.taskless/` before they can do their real work, which rewrites files in the caller's working tree. The only trace was one line of prose on stderr: nothing on stdout, nothing in `--json`, and an unchanged exit code, so a script reading `{"success":true}` could not learn its checkout had changed underneath it. That silence cost three incidents in one session - twice a migration staged into an unrelated commit, once a half migrated layout failed six tests that had nothing to do with the change. Those commands now carry an optional `migrated` field: the source and target schema versions, the migrations applied, and the files added, modified, and removed relative to the project root. It is absent when nothing ran, so presence is the signal and no consumer reads empty arrays to decide. The file list is observed rather than self-reported. The migrations write through plain `fs` calls in five modules, so asking each to keep a list would make the report only as honest as its bookkeeping; hashing `.taskless/` and the root `.gitignore` before and after answers what actually changed on disk. The migration stays automatic, because these commands need a known layout to run at all and the alternative is a hard failure on every upgrade. The human notice improves to match: it names both versions up front and prints the files it touched on completion. `verify` and `test` printed a hand built JSON object; their envelope is now validated against a zod schema before printing, as the sibling payloads are. Fixes #178 --- .changeset/migrated-envelope-field.md | 42 ++++++ packages/cli/src/commands/check.ts | 19 ++- packages/cli/src/commands/verify.ts | 26 +++- packages/cli/src/filesystem/directory.ts | 10 +- packages/cli/src/filesystem/migrate.ts | 95 ++++++++++++-- packages/cli/src/filesystem/snapshot.ts | 105 +++++++++++++++ packages/cli/src/schemas/check.ts | 8 ++ packages/cli/src/schemas/migration.ts | 28 ++++ packages/cli/src/schemas/verify-test.ts | 38 ++++++ packages/cli/test/migrated-envelope.test.ts | 138 ++++++++++++++++++++ 10 files changed, 491 insertions(+), 18 deletions(-) create mode 100644 .changeset/migrated-envelope-field.md create mode 100644 packages/cli/src/filesystem/snapshot.ts create mode 100644 packages/cli/src/schemas/migration.ts create mode 100644 packages/cli/src/schemas/verify-test.ts create mode 100644 packages/cli/test/migrated-envelope.test.ts diff --git a/.changeset/migrated-envelope-field.md b/.changeset/migrated-envelope-field.md new file mode 100644 index 00000000..eca4dfa6 --- /dev/null +++ b/.changeset/migrated-envelope-field.md @@ -0,0 +1,42 @@ +--- +"@taskless/cli": patch +--- + +Report the `.taskless/` layout migration on the `--json` envelope. + +`check`, `verify`, and `test` migrate the scaffold before they can do their real +work, and that rewrites files in the working tree: rules move into per-rule +directories, configs are deleted, `taskless.json` and `.gitignore` are rewritten. +Until now the only trace was one line of prose on stderr, so a CI script reading +`{"success":true}` had no way to learn that its checkout had changed underneath +it. The migration would then land in an unrelated commit, or run mid-suite and +fail tests that had nothing to do with the change being made. + +Those commands now carry a `migrated` field when, and only when, a migration +ran: + +```json +{ + "success": true, + "results": [], + "migrated": { + "from": 3, + "to": 5, + "applied": [4, 5], + "files": { + "added": [".taskless/rules/sg/no-eval/no-eval.yml"], + "modified": [".taskless/taskless.json"], + "removed": [".taskless/sgconfig.yml"] + } + } +} +``` + +The field is absent when nothing happened, so presence is the signal and no +consumer has to read empty arrays to decide. Paths are relative to the project +root and sorted. + +The migration keeps happening automatically, because these commands need a known +layout to run at all and the alternative is a hard failure on every upgrade. The +human notice improved to match the new field: it names the source and target +versions up front, and prints the files it touched on completion. diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index d6c5b87c..400b6619 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -295,9 +295,15 @@ export const checkCommand = defineCommand({ // 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). - if (await pathExists(join(cwd, ".taskless"))) { - await ensureTasklessDirectory(cwd); - } + // + // 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) + : undefined; + const migratedField = migrated === undefined ? {} : { migrated }; const dispatch = await planEngineDispatch(cwd); // Static rules (trusted ast-grep YAML) always run; runtime rules @@ -335,7 +341,11 @@ export const checkCommand = defineCommand({ if (args.json) { console.log( JSON.stringify( - checkOutputSchema.parse({ success: true, results: [] }) + checkOutputSchema.parse({ + success: true, + results: [], + ...migratedField, + }) ) ); } else { @@ -397,6 +407,7 @@ 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 } diff --git a/packages/cli/src/commands/verify.ts b/packages/cli/src/commands/verify.ts index 4520705d..b0257990 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -14,6 +14,7 @@ import { resolveRulePath, RuleNotFoundError, } from "../rules/resolve-path"; +import { outputSchema as verifyTestOutputSchema } from "../schemas/verify-test"; import { makeErrorEnvelope } from "../types/errors"; /** @@ -37,7 +38,10 @@ async function runOverPath(options: { }): Promise { const { cwd, target, json, label, run } = options; - await ensureTasklessDirectory(cwd); + // 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); let rules; try { @@ -66,7 +70,15 @@ async function runOverPath(options: { // has not written a rule yet, and failing here would make `verify` unusable // in CI on a fresh install. if (json) { - console.log(JSON.stringify({ ok: true, rules: [] })); + console.log( + JSON.stringify( + verifyTestOutputSchema.parse({ + ok: true, + rules: [], + ...(migrated === undefined ? {} : { migrated }), + }) + ) + ); } else { console.log(`No rules found under ${target}.`); } @@ -81,7 +93,15 @@ async function runOverPath(options: { const failed = results.filter((result) => !result.ok); if (json) { - console.log(JSON.stringify({ ok: failed.length === 0, rules: results })); + console.log( + JSON.stringify( + verifyTestOutputSchema.parse({ + ok: failed.length === 0, + rules: results, + ...(migrated === undefined ? {} : { migrated }), + }) + ) + ); } else { for (const result of results) { const mark = result.ok ? "✓" : "✗"; diff --git a/packages/cli/src/filesystem/directory.ts b/packages/cli/src/filesystem/directory.ts index 5b1fa210..1089a425 100644 --- a/packages/cli/src/filesystem/directory.ts +++ b/packages/cli/src/filesystem/directory.ts @@ -1,7 +1,7 @@ import { mkdir } from "node:fs/promises"; import { join } from "node:path"; -import { runMigrations } from "./migrate"; +import { runMigrations, type MigrationReport } from "./migrate"; export interface EnsureOptions { /** @@ -22,14 +22,18 @@ export interface EnsureOptions { * Ensure the .taskless/ directory exists and is up-to-date by running * any pending migrations. Safe to call repeatedly — returns immediately * if already current. + * + * Returns the {@link MigrationReport} when migrations ran, so the command that + * triggered them can say so in its own output, and `undefined` when the + * scaffold was already current. */ export async function ensureTasklessDirectory( cwd: string, options: EnsureOptions = {} -): Promise { +): Promise { const tasklessDirectory = join(cwd, ".taskless"); await mkdir(tasklessDirectory, { recursive: true }); - await runMigrations(tasklessDirectory, { + return runMigrations(tasklessDirectory, { onNotice: options.onNotice, allowVersionMismatches: options.allowVersionMismatches, }); diff --git a/packages/cli/src/filesystem/migrate.ts b/packages/cli/src/filesystem/migrate.ts index 77bf2c67..61a7667d 100644 --- a/packages/cli/src/filesystem/migrate.ts +++ b/packages/cli/src/filesystem/migrate.ts @@ -3,6 +3,7 @@ import { join } from "node:path"; import { CLIError } from "../util/cli-error"; import type { Migrations } from "./types"; +import { diffSnapshots, snapshotPaths, type TreeChanges } from "./snapshot"; import init from "./migrations/0001-init"; import installMigration from "./migrations/0002-install"; import dropInstalledAt from "./migrations/0003-drop-installed-at"; @@ -158,11 +159,62 @@ function isPlainObject(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } +/** + * What one migration run changed, in a form a caller can print or hand to a + * machine consumer. + * + * The migration is a precondition of `check` and `verify` rather than a side + * effect, so it will keep happening automatically. What this makes possible is + * for the run that triggered it to *say so*: without a report, a caller reading + * `{"success":true}` has no way to learn that the working tree was rewritten + * underneath it, and an unexplained diff is left looking like someone else's. + */ +export interface MigrationReport { + /** Scaffold version found on disk before anything ran. */ + from: number; + /** Scaffold version written on completion. */ + to: number; + /** Every migration applied, in the order they ran. */ + applied: number[]; + /** Files the run added, rewrote, or deleted, relative to the project root. */ + files: TreeChanges; +} + +/** Paths a migration can write, relative to the project root. */ +const WATCHED_PATHS = [".taskless", ".gitignore"]; + +/** Cap on how many paths the human notice lists before summarizing. */ +const NOTICE_PATH_LIMIT = 20; + +/** Render {@link MigrationReport} as the completion notice a person reads. */ +export function formatMigrationNotice(report: MigrationReport): string { + const { added, modified, removed } = report.files; + const lines = [ + ...added.map((path) => ` + ${path}`), + ...modified.map((path) => ` ~ ${path}`), + ...removed.map((path) => ` - ${path}`), + ]; + const total = lines.length; + const shown = + total > NOTICE_PATH_LIMIT + ? [ + ...lines.slice(0, NOTICE_PATH_LIMIT), + ` ... and ${String(total - NOTICE_PATH_LIMIT)} more`, + ] + : lines; + const headline = + `Migrated .taskless/ from schema version ${String(report.from)} to ` + + `${String(report.to)}: ${String(added.length)} added, ` + + `${String(modified.length)} modified, ${String(removed.length)} removed.`; + return total === 0 ? headline : [headline, ...shown].join("\n"); +} + export interface RunMigrationsOptions { /** - * Called once before the first pending migration runs. Defaults to a - * bare `console.error` notice. Callers that own their own UI can pass a - * custom handler to route the notice through their logger. + * Called once before the first pending migration runs and once after the + * run completes, the second time with the file-by-file summary. Defaults to + * a bare `console.error` notice. Callers that own their own UI can pass a + * custom handler to route the notices through their logger. */ onNotice?: (message: string) => void; /** @@ -181,20 +233,24 @@ export interface RunMigrationsOptions { * Throws when the manifest's version is *newer* than the highest migration * this CLI knows: an older CLI cannot safely read a layout written by a newer * one, so it fails loudly rather than half-reading it. + * + * Returns a {@link MigrationReport} when something ran, and `undefined` when + * nothing did. The distinction is the point: a caller must be able to tell + * "the tree was rewritten" from "nothing happened" without guessing. */ export async function runMigrations( tasklessDirectory: string, options: RunMigrationsOptions = {} -): Promise { +): Promise { const sorted = sortedMigrations(migrations); - if (sorted.length === 0) return; + if (sorted.length === 0) return undefined; const maxVersion = sorted.at(-1)![0]; const { version } = await readRawManifest(tasklessDirectory); if (version > maxVersion) { if (options.allowVersionMismatches ?? hasVersionMismatchOverride()) { - return; + return undefined; } throw new CLIError( `This project's .taskless/ scaffold is version ${String(version)}, but this CLI only understands version ${String(maxVersion)}. ` + @@ -204,12 +260,23 @@ export async function runMigrations( } if (version === maxVersion) { - return; + return undefined; } const pending = sorted.filter(([v]) => v > version); const notice = options.onNotice ?? ((message) => console.error(message)); - notice("Migrating to latest .taskless/ schema..."); + notice( + `Migrating .taskless/ from schema version ${String(version)} to ${String(maxVersion)}...` + ); + + // Observed rather than self-reported: the migrations write through plain + // `fs` calls in five separate modules, and asking each to keep a list of + // what it touched would leave the report only as honest as its bookkeeping. + // Hashing the tree before and after answers the question the caller is + // actually asking, which is what changed on disk. + const projectRoot = join(tasklessDirectory, ".."); + const before = await snapshotPaths(projectRoot, WATCHED_PATHS); + for (const [v, migrate] of pending) { try { await migrate(tasklessDirectory); @@ -238,4 +305,16 @@ export async function runMigrations( ...latestRaw, version: maxVersion, }); + + const report: MigrationReport = { + from: version, + to: maxVersion, + applied: pending.map(([v]) => v), + files: diffSnapshots( + before, + await snapshotPaths(projectRoot, WATCHED_PATHS) + ), + }; + notice(formatMigrationNotice(report)); + return report; } diff --git a/packages/cli/src/filesystem/snapshot.ts b/packages/cli/src/filesystem/snapshot.ts new file mode 100644 index 00000000..28182b9b --- /dev/null +++ b/packages/cli/src/filesystem/snapshot.ts @@ -0,0 +1,105 @@ +import { createHash } from "node:crypto"; +import { readdir, readFile } from "node:fs/promises"; +import { join, relative, sep } from "node:path"; + +/** + * What a migration run did to the working tree, as paths relative to the + * project root. Every list is sorted, so two runs over the same tree produce + * byte-identical output. + */ +export interface TreeChanges { + added: string[]; + modified: string[]; + removed: string[]; +} + +/** Content hash of one file, or `undefined` when it cannot be read. */ +async function hashFile(path: string): Promise { + try { + return createHash("sha256") + .update(await readFile(path)) + .digest("hex"); + } catch { + // Missing, or a special file we have no business hashing. Either way it + // is not part of the snapshot. + return undefined; + } +} + +async function walk( + projectRoot: string, + absolute: string, + into: Map +): Promise { + let entries; + try { + entries = await readdir(absolute, { withFileTypes: true }); + } catch { + // Not a directory, or not there. `snapshotPaths` handles the file case. + return; + } + for (const entry of entries) { + const child = join(absolute, entry.name); + if (entry.isDirectory()) { + await walk(projectRoot, child, into); + continue; + } + if (!entry.isFile()) continue; + const hash = await hashFile(child); + if (hash !== undefined) { + into.set(toProjectPath(projectRoot, child), hash); + } + } +} + +/** Project-relative path with forward slashes, so output is platform-stable. */ +function toProjectPath(projectRoot: string, absolute: string): string { + return relative(projectRoot, absolute).split(sep).join("/"); +} + +/** + * Hash every file under `paths` (each relative to `projectRoot`), which may + * name a directory or a single file. Paths that do not exist contribute + * nothing, which is what makes a file created by a migration read as `added`. + */ +export async function snapshotPaths( + projectRoot: string, + paths: string[] +): Promise> { + const snapshot = new Map(); + for (const path of paths) { + const absolute = join(projectRoot, path); + await walk(projectRoot, absolute, snapshot); + // A directory yields its files above; a plain file yields only itself. + const hash = await hashFile(absolute); + if (hash !== undefined) { + snapshot.set(toProjectPath(projectRoot, absolute), hash); + } + } + return snapshot; +} + +/** Compare two {@link snapshotPaths} results. */ +export function diffSnapshots( + before: Map, + after: Map +): TreeChanges { + const added: string[] = []; + const modified: string[] = []; + const removed: string[] = []; + + for (const [path, hash] of after) { + const previous = before.get(path); + if (previous === undefined) added.push(path); + else if (previous !== hash) modified.push(path); + } + for (const path of before.keys()) { + if (!after.has(path)) removed.push(path); + } + + return { + added: added.toSorted(), + modified: modified.toSorted(), + removed: removed.toSorted(), + }; +} diff --git a/packages/cli/src/schemas/check.ts b/packages/cli/src/schemas/check.ts index 68afea9f..35d4b5b7 100644 --- a/packages/cli/src/schemas/check.ts +++ b/packages/cli/src/schemas/check.ts @@ -1,5 +1,7 @@ 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"), @@ -49,6 +51,12 @@ 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/migration.ts b/packages/cli/src/schemas/migration.ts new file mode 100644 index 00000000..47f5fa5a --- /dev/null +++ b/packages/cli/src/schemas/migration.ts @@ -0,0 +1,28 @@ +import { z } from "zod"; + +/** + * The `migrated` field carried by every `--json` envelope whose command can + * migrate `.taskless/` on the way to doing its real work. + * + * **Absent when nothing ran.** A caller distinguishes "the working tree was + * rewritten underneath me" from "nothing happened" by the presence of the + * field, never by reading empty arrays out of it. Migration remains automatic + * because `check` and `verify` need a known layout to work at all; this is + * what makes it observable to something other than a person watching stderr. + */ +export const migratedSchema = z.object({ + from: z + .number() + .describe("Scaffold schema version found on disk before migrating"), + to: z.number().describe("Scaffold schema version after migrating"), + applied: z + .array(z.number()) + .describe("Migration versions applied, in the order they ran"), + files: z + .object({ + added: z.array(z.string()), + modified: z.array(z.string()), + removed: z.array(z.string()), + }) + .describe("Files the migration touched, relative to the project root"), +}); diff --git a/packages/cli/src/schemas/verify-test.ts b/packages/cli/src/schemas/verify-test.ts new file mode 100644 index 00000000..587cbc35 --- /dev/null +++ b/packages/cli/src/schemas/verify-test.ts @@ -0,0 +1,38 @@ +import { z } from "zod"; + +import { migratedSchema } from "./migration"; + +/** + * One rule's verdict, as `verify` and `test` both report it. + * + * The two commands share an implementation and differ only in what they run, + * so they share one envelope. `ran` is `test`-only: `verify` never reaches a + * test run, and `test` reports `false` when verification failed first. + */ +const ruleResultSchema = z.object({ + engine: z.enum(["sg", "vale", "runtime"]), + ruleId: z.string(), + ok: z.boolean(), + errors: z.array(z.string()).describe("Human-readable failure messages"), + ran: z + .boolean() + .optional() + .describe("`test` only: whether the rule's tests actually ran"), + notice: z + .string() + .optional() + .describe( + "Something true about the rule that does not make it a failure, reported even on a pass" + ), +}); + +/** Output schema for `taskless verify --json` and `taskless test --json`. */ +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/test/migrated-envelope.test.ts b/packages/cli/test/migrated-envelope.test.ts new file mode 100644 index 00000000..381fa847 --- /dev/null +++ b/packages/cli/test/migrated-envelope.test.ts @@ -0,0 +1,138 @@ +import { execFile } from "node:child_process"; +import { mkdir, mkdtemp, rm, 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"; + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); + +interface MigratedField { + from: number; + to: number; + applied: number[]; + files: { added: string[]; modified: string[]; removed: string[] }; +} + +/** Run the built CLI, tolerating a non-zero exit. */ +async function runCli( + args: string[] +): Promise<{ stdout: string; stderr: string }> { + try { + const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); + return { stdout, stderr }; + } catch (error) { + const execError = error as { stdout?: string; stderr?: string }; + return { stdout: execError.stdout ?? "", stderr: execError.stderr ?? "" }; + } +} + +function parseEnvelope(stdout: string): Record { + const line = stdout.trim().split("\n").at(-1) ?? ""; + return JSON.parse(line) as Record; +} + +/** Assert the field describes the 3 to 5 migration of the seeded project. */ +function expectSeededMigration(migrated: unknown): void { + const field = migrated as MigratedField; + expect(field.from).toBe(3); + expect(field.to).toBe(5); + expect(field.applied).toEqual([4, 5]); + // The rule's new home, its old home, and the manifest that records the + // version: the three facts that turn an unexplained diff into an explained + // one. + expect(field.files.added).toContain(".taskless/rules/sg/no-eval/no-eval.yml"); + expect(field.files.removed).toContain(".taskless/rules/no-eval.yml"); + expect(field.files.removed).toContain(".taskless/sgconfig.yml"); + expect(field.files.modified).toContain(".taskless/taskless.json"); +} + +describe("the migrated field on the --json envelope", () => { + let temporaryDirectory: string; + let tasklessDirectory: string; + + beforeEach(async () => { + temporaryDirectory = await mkdtemp(join(tmpdir(), "taskless-migrated-")); + tasklessDirectory = join(temporaryDirectory, ".taskless"); + await mkdir(tasklessDirectory, { recursive: true }); + }); + + afterEach(async () => { + await rm(temporaryDirectory, { recursive: true, force: true }); + }); + + /** A version-3 scaffold: one flat ast-grep rule and its committed config. */ + async function seedVersion3(): Promise { + await writeFile( + join(tasklessDirectory, "taskless.json"), + JSON.stringify({ version: 3, install: {} }), + "utf8" + ); + await writeFile( + join(tasklessDirectory, "sgconfig.yml"), + "ruleDirs:\n - rules\ntestConfigs:\n - testDir: rule-tests\n", + "utf8" + ); + await mkdir(join(tasklessDirectory, "rules"), { recursive: true }); + await writeFile( + join(tasklessDirectory, "rules", "no-eval.yml"), + "id: no-eval\nlanguage: TypeScript\nseverity: error\nmessage: no eval\nrule:\n pattern: eval($A)\n", + "utf8" + ); + } + + for (const command of ["check", "verify", "test"] as const) { + it(`${command} --json reports the migration it performed`, async () => { + await seedVersion3(); + + const { stdout } = await runCli([ + command, + "--json", + "-d", + temporaryDirectory, + ]); + + const envelope = parseEnvelope(stdout); + expect(envelope.migrated).toBeDefined(); + expectSeededMigration(envelope.migrated); + }); + + it(`${command} --json omits the field when nothing migrated`, async () => { + 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([ + command, + "--json", + "-d", + temporaryDirectory, + ]); + + const envelope = parseEnvelope(stdout); + expect(envelope).not.toHaveProperty("migrated"); + }); + } + + it("names the versions and the files on human stderr", async () => { + await seedVersion3(); + + const { stderr } = await runCli(["check", "-d", temporaryDirectory]); + + expect(stderr).toContain("Migrating .taskless/ from schema version 3 to 5"); + expect(stderr).toContain("Migrated .taskless/ from schema version 3 to 5:"); + expect(stderr).toContain("+ .taskless/rules/sg/no-eval/no-eval.yml"); + expect(stderr).toContain("- .taskless/sgconfig.yml"); + }); + + 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]); + + expect(stderr).not.toContain("Migrat"); + }); +}); From ef9f9d31d91807f673c90f7f71f47fa13a5732c9 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 26 Aug 2026 21:31:19 -0700 Subject: [PATCH 2/3] fix(cli): gate the migration notice under --json, and see symlinks Three findings from review of #195, all verified before acting. The notice printed to stderr unconditionally, including under `--json`. That was true before this PR too, but it printed one line; now it prints a file-by-file summary capped at twenty paths. `check` already suppresses every other notice under that flag, for the reason that applies here exactly: the information is on the envelope's `migrated` field, and a machine consumer reading stderr gets prose it cannot parse. A CI script that logs or fails on stderr was handed a much noisier surprise than the one-liner it tolerated. Measured after the fix: a `--json` run writes zero bytes to stderr and still carries `migrated`, and a human run keeps its summary. `EnsureOptions.onNotice` still documented itself as called "once when a migration run is about to start". It is now called twice, on start and on completion. `migrate.ts` had its copy of that doc updated and this one was missed, which is observable rather than cosmetic: the wizard passes the callback and renders two blocks instead of one. The snapshot could not see symlinks. `Dirent` predicates describe the entry itself and do not follow links, so a symlink under `.taskless` was neither walked nor hashed, in `before` or `after`. A migration that created, removed or retargeted one reported nothing for it, which is the same quiet under-reporting this module exists to prevent, reached by a different route than the watched-paths limit already documented. A symlink is now recorded by its TARGET PATH rather than by following it. Following would risk a cycle and would double-count a target that is itself under a watched path; the link text catches creation, removal and retargeting, which are the three things a migration can do to one. The value is prefixed so it cannot collide with a sha256. `snapshot.ts` gains direct unit tests, which it had none of: it was exercised only through a CLI run, and its whole job is saying what actually changed, so a gap there is invisible in the way the report exists to prevent. Ten cases, including the three symlink shapes that were previously invisible, and the no-op rewrite that must NOT read as modified. --- packages/cli/src/commands/check.ts | 13 ++- packages/cli/src/commands/verify.ts | 13 ++- packages/cli/src/filesystem/directory.ts | 13 ++- packages/cli/src/filesystem/snapshot.ts | 31 +++++- packages/cli/test/snapshot.test.ts | 133 +++++++++++++++++++++++ 5 files changed, 196 insertions(+), 7 deletions(-) create mode 100644 packages/cli/test/snapshot.test.ts diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 400b6619..1fb2d0a3 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -301,7 +301,18 @@ export const checkCommand = defineCommand({ // `{"success":true}` would otherwise have nothing to attribute that diff // to. const migrated = (await pathExists(join(cwd, ".taskless"))) - ? await ensureTasklessDirectory(cwd) + ? 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 }; const dispatch = await planEngineDispatch(cwd); diff --git a/packages/cli/src/commands/verify.ts b/packages/cli/src/commands/verify.ts index b0257990..fd7cd128 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -41,7 +41,18 @@ async function runOverPath(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); + 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); + }, + }); let rules; try { diff --git a/packages/cli/src/filesystem/directory.ts b/packages/cli/src/filesystem/directory.ts index 1089a425..5ca56c55 100644 --- a/packages/cli/src/filesystem/directory.ts +++ b/packages/cli/src/filesystem/directory.ts @@ -5,10 +5,15 @@ import { runMigrations, type MigrationReport } from "./migrate"; export interface EnsureOptions { /** - * Called once when a migration run is about to start. Callers that render - * their own UI (e.g., the interactive wizard using clack) can use this to - * keep the message inside their visual tree. When omitted, the migration - * runner falls back to its default `console.error` message. + * Called TWICE when migrations run: once as the run starts, and once on + * completion with the file-by-file summary. Callers that render their own + * UI (e.g., the interactive wizard using clack) can use this to keep both + * messages inside their visual tree. When omitted, the migration runner + * falls back to writing them with `console.error`. + * + * Callers that emit `--json` should pass a callback that suppresses output + * under that flag: the same information is on the envelope's `migrated` + * field, and a machine consumer reading stderr gets prose it cannot parse. */ onNotice?: (message: string) => void; /** diff --git a/packages/cli/src/filesystem/snapshot.ts b/packages/cli/src/filesystem/snapshot.ts index 28182b9b..88105d60 100644 --- a/packages/cli/src/filesystem/snapshot.ts +++ b/packages/cli/src/filesystem/snapshot.ts @@ -1,5 +1,5 @@ import { createHash } from "node:crypto"; -import { readdir, readFile } from "node:fs/promises"; +import { readdir, readFile, readlink } from "node:fs/promises"; import { join, relative, sep } from "node:path"; /** @@ -44,6 +44,21 @@ async function walk( await walk(projectRoot, child, into); continue; } + // A symlink is recorded by its TARGET PATH rather than by following it. + // `Dirent` predicates describe the entry itself, so a symlink is neither + // a file nor a directory here and would otherwise be invisible in both + // snapshots: a migration that created, removed or retargeted one would + // report nothing for it, which is the quiet under-reporting this whole + // module exists to avoid. Hashing the link text catches all three without + // following the link, which would risk a cycle and would double-count a + // target that is itself under a watched path. + if (entry.isSymbolicLink()) { + const target = await symlinkTarget(child); + if (target !== undefined) { + into.set(toProjectPath(projectRoot, child), target); + } + continue; + } if (!entry.isFile()) continue; const hash = await hashFile(child); if (hash !== undefined) { @@ -52,6 +67,20 @@ async function walk( } } +/** + * A marker standing in for a symlink's content: the path it points at. + * + * Prefixed so it cannot collide with a real sha256 hash, which is what the + * map otherwise holds. A symlink whose target changed reads as `modified`. + */ +async function symlinkTarget(path: string): Promise { + try { + return `symlink:${await readlink(path)}`; + } catch { + return undefined; + } +} + /** Project-relative path with forward slashes, so output is platform-stable. */ function toProjectPath(projectRoot: string, absolute: string): string { return relative(projectRoot, absolute).split(sep).join("/"); diff --git a/packages/cli/test/snapshot.test.ts b/packages/cli/test/snapshot.test.ts new file mode 100644 index 00000000..6d5b0676 --- /dev/null +++ b/packages/cli/test/snapshot.test.ts @@ -0,0 +1,133 @@ +import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { diffSnapshots, snapshotPaths } from "../src/filesystem/snapshot"; + +/** + * These are the pure half of the migration report, and worth testing directly + * rather than only through a CLI run: the report's whole value is that it says + * what actually changed, so a gap here is invisible in exactly the way the + * report exists to prevent. + */ +describe("snapshotPaths and diffSnapshots", () => { + let root: string; + + beforeEach(async () => { + root = await mkdtemp(join(tmpdir(), "taskless-snapshot-")); + await mkdir(join(root, ".taskless"), { recursive: true }); + }); + + afterEach(async () => { + await rm(root, { recursive: true, force: true }); + }); + + const snap = () => snapshotPaths(root, [".taskless", ".gitignore"]); + + it("reports an added file", async () => { + const before = await snap(); + await writeFile(join(root, ".taskless", "new.txt"), "hello"); + expect(diffSnapshots(before, await snap())).toEqual({ + added: [".taskless/new.txt"], + modified: [], + removed: [], + }); + }); + + it("reports a modified file, and only when contents change", async () => { + await writeFile(join(root, ".taskless", "a.txt"), "one"); + const before = await snap(); + + // Rewriting identical bytes is not a change. Reporting it would make + // every run look like it touched the tree. + await writeFile(join(root, ".taskless", "a.txt"), "one"); + expect(diffSnapshots(before, await snap()).modified).toEqual([]); + + await writeFile(join(root, ".taskless", "a.txt"), "two"); + expect(diffSnapshots(before, await snap()).modified).toEqual([ + ".taskless/a.txt", + ]); + }); + + it("reports a removed file", async () => { + await writeFile(join(root, ".taskless", "gone.txt"), "x"); + const before = await snap(); + await rm(join(root, ".taskless", "gone.txt")); + expect(diffSnapshots(before, await snap()).removed).toEqual([ + ".taskless/gone.txt", + ]); + }); + + it("recurses into nested directories", async () => { + const before = await snap(); + await mkdir(join(root, ".taskless", "rules", "sg"), { recursive: true }); + await writeFile(join(root, ".taskless", "rules", "sg", "r.yml"), "id: r"); + expect(diffSnapshots(before, await snap()).added).toEqual([ + ".taskless/rules/sg/r.yml", + ]); + }); + + it("watches a named file as well as a directory", async () => { + const before = await snap(); + // Migration 0001 writes the root `.gitignore`, which is why a bare file + // is a watched path at all. + await writeFile(join(root, ".gitignore"), "node_modules\n"); + expect(diffSnapshots(before, await snap()).added).toEqual([".gitignore"]); + }); + + it("sees a created symlink", async () => { + // `Dirent` predicates describe the entry itself and do not follow links, + // so a symlink is neither a file nor a directory. Left unhandled it is + // invisible in BOTH snapshots, and a migration that created one would + // report nothing for it. + await writeFile(join(root, ".taskless", "real.txt"), "x"); + const before = await snap(); + await symlink("real.txt", join(root, ".taskless", "link.txt")); + expect(diffSnapshots(before, await snap()).added).toEqual([ + ".taskless/link.txt", + ]); + }); + + it("sees a symlink retargeted, without following it", async () => { + await writeFile(join(root, ".taskless", "real.txt"), "x"); + await symlink("real.txt", join(root, ".taskless", "link.txt")); + const before = await snap(); + + await rm(join(root, ".taskless", "link.txt")); + await symlink("other.txt", join(root, ".taskless", "link.txt")); + + // Modified rather than added/removed: the path still exists, its target + // changed. Recorded from the link text, so a dangling target is fine and + // no cycle is possible. + expect(diffSnapshots(before, await snap()).modified).toEqual([ + ".taskless/link.txt", + ]); + }); + + it("sees a removed symlink", async () => { + await symlink("nowhere.txt", join(root, ".taskless", "dangling.txt")); + const before = await snap(); + await rm(join(root, ".taskless", "dangling.txt")); + expect(diffSnapshots(before, await snap()).removed).toEqual([ + ".taskless/dangling.txt", + ]); + }); + + it("contributes nothing for a path that does not exist", async () => { + const snapshot = await snapshotPaths(root, ["does-not-exist"]); + expect(snapshot.size).toBe(0); + }); + + it("sorts every list, so two runs over one tree agree", async () => { + const before = await snap(); + for (const name of ["c.txt", "a.txt", "b.txt"]) { + await writeFile(join(root, ".taskless", name), name); + } + expect(diffSnapshots(before, await snap()).added).toEqual([ + ".taskless/a.txt", + ".taskless/b.txt", + ".taskless/c.txt", + ]); + }); +}); From e236aa2e9fba6bc9c68a5bbb90a288087de56d19 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 26 Aug 2026 21:38:47 -0700 Subject: [PATCH 3/3] perf(cli): hash a snapshot in bounded batches Hashing ran one file at a time through a sequential `for`/`await`, twice per migration. That is fine for this repository's own `.taskless/`, which holds a handful of rule files, and it is the wrong shape for a tree that is not: the work is IO-bound and there is no reason for each file to wait on the last. Traversal and hashing are now separate. `walk` collects entries without resolving them, since directory reads are cheap and inherently sequential, and the collected work is then resolved concurrently. **Bounded at 32, not a bare `Promise.all` over the whole tree.** Unbounded concurrency on a large repository opens every file at once and hits the process descriptor limit. That would surface as `EMFILE` inside `hashFile`, which swallows a read failure as "not part of the snapshot", so the report would come back silently incomplete rather than erroring. Quiet under-reporting is the exact failure this module exists to prevent, so it has to be designed out rather than caught. Covered by a test that crosses the batch boundary in both directions: 80 files in one run, all 80 accounted for and still sorted across batches, so a partition bug cannot pass as a smaller result. Verified end to end against a genuine version-2 layout: `from 2 to 5`, `applied [3, 4, 5]`, 5 added, 1 modified, 1 removed, unchanged from before the batching. --- packages/cli/src/filesystem/snapshot.ts | 77 ++++++++++++++++++++----- packages/cli/test/snapshot.test.ts | 24 ++++++++ 2 files changed, 86 insertions(+), 15 deletions(-) diff --git a/packages/cli/src/filesystem/snapshot.ts b/packages/cli/src/filesystem/snapshot.ts index 88105d60..e050d314 100644 --- a/packages/cli/src/filesystem/snapshot.ts +++ b/packages/cli/src/filesystem/snapshot.ts @@ -26,10 +26,27 @@ async function hashFile(path: string): Promise { } } +/** + * One entry to be resolved into a snapshot value: a file to hash, or a + * symlink to read. + */ +interface PendingEntry { + absolute: string; + kind: "file" | "symlink"; +} + +/** + * Collect the entries under `absolute` WITHOUT resolving them. + * + * Directory traversal is cheap and inherently sequential; hashing is neither. + * Separating the two lets the hashing run with bounded concurrency below, + * which is what keeps this usable on a tree much larger than this repository's + * own. A snapshot runs twice per migration, so the cost is paid twice. + */ async function walk( projectRoot: string, absolute: string, - into: Map + into: PendingEntry[] ): Promise { let entries; try { @@ -53,17 +70,11 @@ async function walk( // following the link, which would risk a cycle and would double-count a // target that is itself under a watched path. if (entry.isSymbolicLink()) { - const target = await symlinkTarget(child); - if (target !== undefined) { - into.set(toProjectPath(projectRoot, child), target); - } + into.push({ absolute: child, kind: "symlink" }); continue; } if (!entry.isFile()) continue; - const hash = await hashFile(child); - if (hash !== undefined) { - into.set(toProjectPath(projectRoot, child), hash); - } + into.push({ absolute: child, kind: "file" }); } } @@ -91,20 +102,56 @@ function toProjectPath(projectRoot: string, absolute: string): string { * name a directory or a single file. Paths that do not exist contribute * nothing, which is what makes a file created by a migration read as `added`. */ +/** + * How many entries are resolved at once. + * + * Bounded rather than a bare `Promise.all` over the whole tree: a large + * repository would open every file at once and hit the process descriptor + * limit with `EMFILE`, which would surface as an unreadable file and be + * swallowed into "not part of the snapshot". That is the quiet + * under-reporting this module exists to avoid, so the failure mode has to be + * designed out rather than caught. + */ +const HASH_CONCURRENCY = 32; + +/** Resolve pending entries in bounded-concurrency batches. */ +async function resolveEntries( + projectRoot: string, + pending: PendingEntry[], + into: Map +): Promise { + for (let start = 0; start < pending.length; start += HASH_CONCURRENCY) { + const batch = pending.slice(start, start + HASH_CONCURRENCY); + const resolved = await Promise.all( + batch.map(async (entry) => ({ + path: toProjectPath(projectRoot, entry.absolute), + value: + entry.kind === "symlink" + ? await symlinkTarget(entry.absolute) + : await hashFile(entry.absolute), + })) + ); + for (const { path, value } of resolved) { + if (value !== undefined) into.set(path, value); + } + } +} + export async function snapshotPaths( projectRoot: string, paths: string[] ): Promise> { const snapshot = new Map(); + const pending: PendingEntry[] = []; for (const path of paths) { const absolute = join(projectRoot, path); - await walk(projectRoot, absolute, snapshot); - // A directory yields its files above; a plain file yields only itself. - const hash = await hashFile(absolute); - if (hash !== undefined) { - snapshot.set(toProjectPath(projectRoot, absolute), hash); - } + await walk(projectRoot, absolute, pending); + // A directory yields its entries above; a watched path that is itself a + // plain file yields only itself, which is how the root `.gitignore` is + // covered. + pending.push({ absolute, kind: "file" }); } + await resolveEntries(projectRoot, pending, snapshot); return snapshot; } diff --git a/packages/cli/test/snapshot.test.ts b/packages/cli/test/snapshot.test.ts index 6d5b0676..90324e78 100644 --- a/packages/cli/test/snapshot.test.ts +++ b/packages/cli/test/snapshot.test.ts @@ -114,6 +114,30 @@ describe("snapshotPaths and diffSnapshots", () => { ]); }); + it("handles a tree larger than one hash batch", async () => { + // Hashing runs in bounded batches (32 at a time) rather than one + // `Promise.all` over the whole tree, because a large repository would + // otherwise open every file at once and hit `EMFILE`. An unreadable file + // is swallowed as "not part of the snapshot", so that failure would show + // up as a silently incomplete report rather than an error. 80 files + // crosses the boundary in both directions. + const before = await snap(); + const names = Array.from( + { length: 80 }, + (_, index) => `f${String(index).padStart(3, "0")}.txt` + ); + for (const name of names) { + await writeFile(join(root, ".taskless", name), name); + } + + const changes = diffSnapshots(before, await snap()); + expect(changes.added).toHaveLength(80); + // Every one accounted for, and still sorted across batch boundaries. + expect(changes.added).toEqual( + names.map((name) => `.taskless/${name}`).toSorted() + ); + }); + it("contributes nothing for a path that does not exist", async () => { const snapshot = await snapshotPaths(root, ["does-not-exist"]); expect(snapshot.size).toBe(0);