feat(cli): say on the envelope that the layout was migrated by thecodedrift · Pull Request #195 · taskless/cli

thecodedrift · 2026-08-26T19:46:24Z

Makes the .taskless/ layout migration visible to the machine interface.

The problem

Running check, verify or test against an older .taskless/ tree migrates it: files move, taskless.json and .gitignore are rewritten. On this repository's own tree that is six tracked files changed.

This is not a request to stop migrating. check and verify require a specific layout to function at all, so the alternative for anyone on @latest is a hard failure on every run, which is worse. Migration is a precondition, not a side effect.

The problem is that nothing the caller can read says it happened:

surface	signal before
human stderr	one line, `Migrating to latest .taskless/ schema...`
human stdout	nothing
`--json` stdout	nothing
exit code	unchanged

A CI script reading {"success":true,"results":[]} had no way to learn the working tree was rewritten underneath it, and a person looking at six changed tracked files had nothing connecting them to the command they ran. The notice named no file, no source version and no target version.

What this adds

A migrated field on the --json envelope, present only when a migration actually ran:

"migrated": {
  "from": 3,
  "to": 5,
  "applied": [4, 5],
  "files": {
    "added": [".taskless/rules/sg/no-eval/no-eval.yml"],
    "modified": [".taskless/taskless.json"],
    "removed": [".taskless/rules/no-eval.yml", ".taskless/sgconfig.yml"]
  }
}

Paths are project-root-relative, forward-slashed and sorted. The field is absent, not empty, when nothing migrated, so a caller distinguishes "nothing happened" from "something happened" without guessing.

Emitted by check, verify and test: the three commands that both emit --json and call ensureTasklessDirectory at their own boundary.

Human stderr improved too, since it named neither the files nor the versions:

Migrated .taskless/ from schema version 3 to 5: 5 added, 1 modified, 2 removed.
  + .taskless/rules/sg/no-eval/no-eval.yml
  ~ .taskless/taskless.json
  - .taskless/sgconfig.yml

Capped at 20 paths then ... and N more. Silent when nothing runs, as before.

How the file list is produced

By hashing .taskless/ and the repo-root .gitignore before and after the run and diffing the two maps, rather than by per-migration bookkeeping. The five migrations write through plain fs calls in separate modules, so a self-reported list would only ever be as honest as its bookkeeping. A snapshot reports what actually changed.

verify and test previously printed a hand-built JSON object with no schema at all. That payload is now validated like its siblings.

Why this matters here specifically

The invisibility bit three times in one session:

twice staging a layout migration into an unrelated commit, caught by inspection before either shipped
once causing 6 unrelated test failures, because a suite ran against a half-migrated layout. That is the expensive kind: the natural response is to search the diff, and the diff is innocent

Known limits

rule create and rule improve can still migrate silently. Their migration happens inside writeRuleFile, not at the command boundary, so their envelopes carry no migrated. Fixing it means an explicit ensureTasklessDirectory at the top of each run plus network-mocked tests. Filed separately rather than grown into this PR.
A partial migration still reports nothing. If a migration throws, runMigrations rethrows and no report is produced, so the error envelope carries no migrated. No worse than before, but a partially applied migration is exactly the case the test-failure story above describes.
The snapshot watches .taskless/ and the root .gitignore only. Migration 0001 writes the root .gitignore, which is why it is included. A future migration writing elsewhere needs WATCHED_PATHS extended, or the report quietly under-reports.

Verification

pnpm typecheck, pnpm lint, pnpm test (57 files, 937 tests), pnpm cli check (exit 0).

8 new tests spawn the built CLI against a seeded version-3 scaffold: for each of check/verify/test, a migration case asserting from/to/applied and the specific added, modified and removed paths, plus a no-migration case on the second run asserting the field is absent. Two more cover the human stderr.

Fixes #178

`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

thecodedrift · 2026-08-27T01:30:18Z

@claude /review

claude · 2026-08-27T01:30:33Z

Claude finished @thecodedrift's task in 6m 0s —— View job

Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained no review threads and no reviews; its one top-level comment is the @claude /review trigger itself. This is the first substantive review of this PR, so everything below is [New].

Read .prior-review.json
Read the full diff (origin/main...HEAD, 10 files, +491/-18)
Traced snapshot.ts hashing/diffing logic
Traced migrate.ts report construction and onNotice call sites
Traced check.ts / verify.ts envelope wiring and schema changes
Posted inline findings
Posted final summary

Assessment

The core mechanism — snapshotting .taskless/ + root .gitignore before/after a migration and diffing content hashes — is sound and correctly ordered (the manifest's version bump is written before the "after" snapshot, so taskless.json itself shows up as modified, as the example in the PR body promises). The migrated-absent-vs-present envelope contract, the schema validation now applied to verify/test JSON output, and the check/verify wiring all check out against their call sites and callers (init, onboard, wizard, rules/* still just await the call and ignore the new return value, which is fine and matches the PR's stated "known limits").

Five inline findings, none blocking, roughly in severity order:

migrate.ts — the human-readable migration notice (now up to ~20+ lines with the full file list) still prints to stderr unconditionally, even under --json, unlike every other notice/failure in check.ts/verify.ts which is deliberately suppressed under --json because it's carried in the envelope instead. The migration notice is now also carried in the envelope, but the stderr duplicate wasn't gated to match — worth a second look given the PR's own motivation is making CI scripts' lives easier.
directory.ts — EnsureOptions.onNotice's doc comment says "called once"; it's now called twice (start + completion summary). The sibling doc in migrate.ts was updated to say so; this one wasn't. Concretely observable: the wizard's custom onNotice now renders two log.info blocks instead of one.
snapshot.ts — walk() silently skips symlinked files/directories (Dirent.isFile()/isDirectory() don't follow symlinks), so a migration touching something reachable only via a symlink under a watched path would under-report, the same failure mode the PR's "Known limits" section already documents for paths outside WATCHED_PATHS.
snapshot.ts — file hashing in walk/snapshotPaths is sequential rather than concurrent, doubled per migration (before + after); low severity for typical .taskless/ tree sizes but worth naming since it's now a hot path.
migrated-envelope.test.ts — snapshotPaths/diffSnapshots are pure functions but only exercised indirectly through one built-CLI scenario; a few cheap direct unit tests (identical-content no-op, remove+add vs. rename, path-separator normalization) aren't covered.

No correctness bugs found in the envelope contract, schema wiring, or manifest/report ordering itself.

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.

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.

thecodedrift · 2026-08-27T04:39:38Z

Re: @claude[bot] — "Review complete — Review mode: incremental…"
#195 (comment)

All six verified, all six real, all six addressed. Threads replied to and resolved individually.

The --json gate was the important one. The notice printed to stderr unconditionally before this PR too, but it printed one line; this PR grew it to a file-by-file summary. Since check already suppresses every other notice under that flag for exactly this reason, it was inconsistent in a way the PR made worse. Measured after the fix: a --json run writes zero bytes to stderr and still carries migrated on the envelope, while a human run keeps its summary.

Symlinks were the subtlest. Dirent predicates describe the entry itself, so a symlink under a watched path was neither walked nor hashed in either snapshot: a migration that created, removed or retargeted one reported nothing. Same quiet under-reporting the "Known limits" section already documents for unwatched paths, reached by a different route, which is what made it worth fixing rather than documenting. Now recorded by target path rather than followed, so there is no cycle risk and no double-counting a target that is itself watched.

The doc was observably wrong, not merely stale: the wizard passes that callback and was rendering two blocks against a comment promising one.

Both low findings were taken as well. Batch hashing because repositories are larger than this one, bounded at 32 rather than unbounded, since EMFILE would land inside hashFile and be swallowed as "not part of the snapshot" rather than raised. And snapshot.ts gained the direct unit tests it never had, which felt worth doing unprompted: a module whose job is saying what actually changed, left untested, is invisible in the way the whole feature exists to prevent.

Verification: pnpm typecheck, pnpm lint, 948 tests, pnpm cli check exit 0, plus an end-to-end run against a genuine version-2 layout confirming the report is unchanged by the batching.

— AI Coding Agent

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.

thecodedrift force-pushed the fix/migrated-envelope branch from feb78e0 to 9cc2f64 Compare August 27, 2026 01:12