feat(cli): walk the ledger from 0.0.0 when no marker is recorded - #198
Conversation
An absent `rules.reconciledTo` meant "nothing to walk", on the reasoning that a project created at the installed version has no history. That was right about new projects and wrong about every existing one. A project that predates the ledger has had NONE of its entries applied, so reading absence as up to date silently excused exactly the population the entries were written for: the 0.11.x entry would have reached nobody, because nothing has ever recorded a marker. Absent now means `0.0.0` and every section applies. New projects stay correct because `init` stamps the marker at creation, which is what makes the two distinguishable by construction rather than by guessing: present means accounted for, absent means predates the ledger. The stamp never overwrites, so re-running setup cannot reset a marker a real walk earned, and it swallows its own failures because the cost of missing it is one extra walk, which is the safe direction to be wrong in. `--reconciledTo=<version>` becomes `--rules`. The value was never load-bearing: the CLI knows its own version, the only sensible endpoint of a walk is the installed one, and accepting a value only made it possible to claim a walk that did not finish. Removing it removes the two guards that existed to police the value and every way of supplying it wrongly, including the garbage-input hole found in review. The backwards guard stays, because an older CLI on the same project would otherwise rewind the marker. Free to change: `--reconciledTo` reached `main` but stable is still 0.10.2, so no consumer has it. Four tests policed a supplied value and are gone with it. The version-ordering cases they covered moved to a unit test of `reconciliationStart`, where they can actually be exercised: an older CLI and a nightly against its release both depend on which build is running, which a spawned process cannot simulate. That includes the nightly-equals-release case, which exists to stop someone "correcting" the comparison into semver.
|
@claude /review |
|
Claude finished @thecodedrift's task in 4m 4s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
SummaryThe core idea — absent marker means
Nothing else stood out: the backwards-guard, the nightly-vs-release comparison, the moved test cases, and the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…watch for The 0.11.x ledger's hardest section said "re-run its fixtures and read the findings rather than the exit code", which tells an author to look without saying what for. Measuring both shapes against the two binaries (#199) turned each into an instruction. A rule using `nthChild` with a binding `ofRule` was INERT at 0.41.0 and now reports. The rule is not broken, it never ran, so its findings on this version have never been seen and may be numerous on a codebase that was never actually checked against it. Fixtures cannot surface this: a rule that matched nothing passed the `pass/` side of its own tests. A non-binding `ofRule`, `kind:` or `regex:`, is unaffected, which is the check that tells an author whether their rule is in scope. A metavariable bound inside a negated `not` and referenced in `message`, `fix` or a constraint now renders empty, because a `not` contributes no bindings and the old value was leaked from the candidate the negation REJECTED. This one changes no counts at all: same file, same range, same rule, different rendered output. So the instruction is to grep for a metavariable appearing both inside a `not` and in a `message` or `fix`, and the consequence is worth stating plainly, since a `fix` in that shape has been writing leaked text into files. Root metavariables and comments is named as unreproduced rather than dressed up as guidance, because no shape we tried showed a difference. Refs #180 Refs #184
From review of #198. Two findings, both real, and the first reintroduced the exact bug this PR exists to fix. `stampNewProjectRules` ran unconditionally after `ensureTasklessDirectory`, which `mkdir -p`s. By the time it executed, a pre-existing project and a fresh one were indistinguishable, so it fell back to "no marker means new". Its caller is `runNonInteractive`, which is also `init --no-interactive`, whose documented job is refreshing an EXISTING project. So an ordinary older project that had never walked the ledger, refreshed once after a CLI upgrade, got stamped as fully reconciled and skipped every entry. That is the silent skip this change was written to prevent, reached through setup instead of through the walk. Newness is now sampled BEFORE the directory is created, and that ordering is the fix. The `reconciledTo !== undefined` check inside stays as a second belt against resetting a marker a real walk earned. Covered by a regression test verified to bite: it fails with the guard removed and passes with it restored. A test that passes either way would have been worse than none here, since the failure is silent by construction. The recipe also told an agent to read `walk` from `info --json`, which did not report it: it was computed only for `update --json`. Rather than weaken the guidance, `info` now reports `rules.walk`, since `info` is already the command consulted for capability state and this keeps one place to look. An agent following the recipe literally would otherwise have found no field and fallen back to deriving the boundary by hand, which is what that paragraph exists to prevent. Refs #180
Both verified, both real, both fixed in One was bucketed low and is the most serious thing that has come up on this PR. So an ordinary older project that had never walked the ledger, refreshed once after a CLI upgrade, would be stamped as fully reconciled and skip every entry. That is the exact silent skip this PR was written to prevent, reached through setup instead of through the walk. The review also pointed at Newness is now sampled before the directory is created, and that ordering is the whole fix. The regression test was verified to bite, which mattered more than usual here: it fails with the guard removed and passes with it restored. This failure is silent by construction, so a test that passed either way would have been worse than no test at all. The second finding was my error. The recipe told an agent to read
— AI Coding Agent |
1 participant
Makes the ledger reach the projects it was written for, and drops a flag value that was never load-bearing.
The gap this closes
An absent
rules.reconciledTomeant "nothing to walk", on the reasoning that a project created at the installed version has no history to walk. That is right about new projects and wrong about every existing one.A project that predates the ledger has had none of its entries applied. Reading absence as up to date silently excused exactly the population the entries were written for. Since nothing has ever recorded a marker, the 0.11.x entry would have reached nobody: every upgrading project would record the current version and walk nothing, and the ast-grep changes would go unmentioned.
How both cases stay correct
Absent now means
0.0.0, so every section applies.New projects stay right because
initstamps the marker at creation. That is what makes the two distinguishable by construction rather than by heuristic:The stamp never overwrites, so re-running setup cannot reset a marker a real walk earned. It also swallows its own failures: setting up a project must not fail because a marker could not be written, and the cost of missing it is one extra walk, which is the safe direction to be wrong in.
--reconciledTo=<version>becomes--rulesThe value was never load-bearing. The CLI knows its own version, the only sensible endpoint of a walk is the installed one, and accepting a value only made it possible to claim a walk that did not finish.
Removing it removes both guards that existed to police it, and every way of supplying it wrongly, including the garbage-input hole found in review of #196 where
abcparsed as[0]and was written verbatim.The backwards guard stays. The stamp comes from whichever CLI is running, so an older build on the same project would otherwise rewind the marker and send the next walk back through entries already applied.
Free to change:
--reconciledToreachedmain, but stable is still0.10.2, so no consumer has it.Tests moved rather than deleted
Four tests policed a supplied value and go with it. The version-ordering cases they covered moved to a unit test of
reconciliationStart, where they can actually be exercised: an older CLI, and a nightly against its release, both depend on which build is running, which a spawned process cannot be made to simulate.That includes the nightly-equals-release case, which exists specifically to stop someone "correcting" the comparison into semver. Our nightlies are valid semver, and the spec ranks a prerelease below its release, so semver would call a project reconciled on the nightly behind the identical release and send it back through an entry it already walked.
Verified end to end
walk: {from: "0.0.0", to: "0.10.2"}update --rulesreconciledTo: "0.10.2",walk: nullinit{reconciledTo, engines},walk: nullpnpm typecheck,pnpm lint, 964 tests,pnpm cli checkexit 0.Refs #180