diff --git a/.changeset/reconcile-after-engine-bump.md b/.changeset/reconcile-after-engine-bump.md new file mode 100644 index 00000000..0e054985 --- /dev/null +++ b/.changeset/reconcile-after-engine-bump.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +`taskless init` now says when the project's rules need `update --rules`, on every run rather than only when the CLI version moves, with the same answer as `reconcile` (`{ required, engines, ruleMigrations }`) on `init --json` and `update --json`, and as `rules.reconcile` on `info --json`. Previously, moving between two nightlies of one release that bumped ast-grep or Vale reported nothing to walk, while `check` and `verify` refused with `RULES_ENGINE_MISMATCH` until `update --rules` ran. diff --git a/packages/cli/src/agent/update.md b/packages/cli/src/agent/update.md index 9b6169ca..fb09d153 100644 --- a/packages/cli/src/agent/update.md +++ b/packages/cli/src/agent/update.md @@ -1,4 +1,4 @@ -# Topic: update (CLI v%(CLI_VERSION)s / topic v21) +# Topic: update (CLI v%(CLI_VERSION)s / topic v22) %(ONE_COMMAND_PER_CALL)s @@ -49,6 +49,17 @@ that you finished. `null` when there is not. Prefer it to re-deriving the boundary yourself, so the offer and the enforcement cannot disagree. + **A `null` walk does not mean you are done.** Read + `rules.reconcile.required` too. It is `true` when there is a walk, + and also when `rules.reconcile.engines` lists an engine whose rules + are recorded behind the one this CLI bundles. That second case has no + ledger section: every nightly of one release reads as that release, + so an ast-grep or Vale bump between two nightlies leaves `walk` null + while `check` and `verify` refuse with `RULES_ENGINE_MISMATCH`. With + `required` true and `walk` null, skip steps 2 and 3 and go on to step + 4. With `required` false, the rules need nothing and step 5 has + nothing to record. + 2. **Walk forward, in order.** Start at the section after `reconciledTo` and continue to the installed version. Sections are cumulative, never replaceable: a later one can depend on an earlier diff --git a/packages/cli/src/commands/info.ts b/packages/cli/src/commands/info.ts index ce8ea360..bb95fe86 100644 --- a/packages/cli/src/commands/info.ts +++ b/packages/cli/src/commands/info.ts @@ -11,7 +11,7 @@ import { outputSchema as infoOutputSchema } from "../schemas/info"; import { makeErrorEnvelope } from "../types/errors"; import { resolveRepositoryContext } from "../util/git-remote"; import { readManifest } from "../filesystem/manifest"; -import { reconciliationStart } from "../rules/reconcile-marker"; +import { reconcileReport, reconcileStatus } from "../rules/reconcile-status"; import { TASKLESS_DIRECTORY } from "../rules/vale/formats"; export const infoCommand = defineCommand({ @@ -76,6 +76,8 @@ export const infoCommand = defineCommand({ } } + const reconcile = await reconcileStatus(cwd, manifest?.rules); + const result = { success: true as const, version: __VERSION__, @@ -111,7 +113,11 @@ export const infoCommand = defineCommand({ // The walk boundary, decided once here rather than by each caller. // A missing marker resolves to the baseline, so a project predating // the ledger reports a walk rather than "nothing to do". - walk: reconciliationStart(manifest?.rules?.reconciledTo) ?? null, + walk: reconcile.walk, + // Whether `update --rules` is due, which `walk` alone cannot say: an + // engine bump between two nightlies of one release owes no walk and + // still trips the engine gate. + reconcile: reconcileReport(reconcile), }, pinnedCli, }; diff --git a/packages/cli/src/commands/init.ts b/packages/cli/src/commands/init.ts index c74a57ed..a287b9e5 100644 --- a/packages/cli/src/commands/init.ts +++ b/packages/cli/src/commands/init.ts @@ -55,7 +55,6 @@ import { import { pathExists, recordReconciliation, - reconciliationStart, stampNewProjectRules, } from "../rules/reconcile-marker"; import { readManifest } from "../filesystem/manifest"; @@ -63,7 +62,12 @@ import { requireCurrentSchema, type MigrationReport, } from "../filesystem/migrate"; -import { pendingRuleMigrations } from "../rules/rule-migrate"; +import { + getReconcileNotice, + reconcileReport, + reconcileStatus, + type ReconcileStatus, +} from "../rules/reconcile-status"; import { TASKLESS_DIRECTORY } from "../rules/vale/formats"; import { CLIError } from "../util/cli-error"; import { buildInvocation } from "../util/invocation"; @@ -172,6 +176,10 @@ export const initCommand = defineCommand({ // Always present, so the agent can tell the user telemetry is on // and how to turn it off; the human path prints the notice instead. telemetry: { enabled: telemetryEnabled }, + // Whether the rules need `update --rules`, on every run rather + // than only on a version move. The shape `info --json` reports as + // `rules.reconcile` and `update --json` as `reconcile`. + reconcile: reconcileReport(result.rules), // 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. @@ -196,10 +204,21 @@ export const initCommand = defineCommand({ // version the project now records. A build with no launcher names // the invocation the person used. invocation: buildLauncherInvocation() ?? getCliPrefix(), + reconcileDue: result.rules.required, }); if (upgradeTrailer !== undefined) { console.log(upgradeTrailer); } + // Not gated on the run having changed anything, like the pin notice + // below. Between two nightlies, or on a re-run after an upgrade nobody + // reconciled, nothing moves and `check` still refuses. + const reconcileNotice = getReconcileNotice( + result.rules, + buildLauncherInvocation() ?? getCliPrefix() + ); + if (reconcileNotice !== undefined) { + console.log(reconcileNotice); + } // Not gated on the run having changed anything. A re-install that wrote // nothing still leaves the pin and the project disagreeing, and this is // the one place that looks. @@ -306,7 +325,7 @@ export const updateCommand = defineCommand({ const { manifest } = await readManifest( join(cwd, TASKLESS_DIRECTORY) ).catch(() => ({ manifest: undefined })); - const walk = reconciliationStart(manifest?.rules?.reconciledTo); + const status = await reconcileStatus(cwd, manifest?.rules); console.log( JSON.stringify({ ok: true, @@ -316,20 +335,26 @@ export const updateCommand = defineCommand({ // `null` when there is nothing to walk: either the project has // never recorded a reconciliation, which is not the same as being // behind, or it is already current. - walk: walk ?? null, + walk: status.walk, + // Whether to finish with `update --rules`, which a null `walk` + // does not settle: an engine bump between two nightlies of one + // release has no ledger section and still trips the engine gate. + // Without a project nothing is due and nothing would be rewritten. + reconcile: reconcileReport( + manifest === undefined + ? { + ...status, + required: false, + engines: [], + ruleMigrations: [], + } + : status + ), // The rewrites `update --rules` will apply before it stamps, so // the walk can tell its reader what it does not need to do by - // hand. Empty without a project, which has nothing to rewrite. - ruleMigrations: - manifest === undefined - ? [] - : pendingRuleMigrations(manifest.rules?.engines).map( - ({ engine, engineVersion, summary }) => ({ - engine, - engineVersion, - summary, - }) - ), + // hand. Also on `reconcile`; kept here for callers that read it + // from before `reconcile` existed. + ruleMigrations: manifest === undefined ? [] : status.ruleMigrations, recipe, }) ); @@ -421,6 +446,8 @@ async function runNonInteractive( permissions: PermissionEdit[]; /** User-level trust entries, read and never written. */ globalTrust: GlobalTrustOutcome[]; + /** Whether the rules need `update --rules`, read after any stamp. */ + rules: ReconcileStatus; }> { // Under `--json`, stdout carries only the envelope printed by the caller. // This per-target summary is not on that envelope (it is finer-grained than @@ -572,6 +599,11 @@ async function runNonInteractive( log(warning); } + // Read after `stampNewProjectRules`, so a project this run created reports + // nothing due. + const { manifest } = await readManifest(join(cwd, TASKLESS_DIRECTORY)); + const rules = await reconcileStatus(cwd, manifest.rules); + return { reloadNotice, // Read after every write above, so it lists what this run left behind. @@ -598,6 +630,7 @@ async function runNonInteractive( launcherVersionMoved(previousState, result.state) || versionMoved({ previousCliVersion, cliVersion }), pinnedCli: await findCliPins(cwd, cliVersion, { migrated }), + rules, }; } diff --git a/packages/cli/src/install/upgrade-trailer.ts b/packages/cli/src/install/upgrade-trailer.ts index 6316bcbd..a107fa6c 100644 --- a/packages/cli/src/install/upgrade-trailer.ts +++ b/packages/cli/src/install/upgrade-trailer.ts @@ -21,6 +21,12 @@ export interface UpgradeTrailerInput { cliVersion: string; /** The invocation to print in front of `update`, e.g. `./.taskless/bin/cli.mjs`. */ invocation: string; + /** + * Whether `init` prints the reconcile notice after this trailer. That notice + * names `update` and `update --rules` with the reason, so this one states + * only the move rather than sending the reader to `update` twice. + */ + reconcileDue?: boolean; } /** @@ -48,9 +54,11 @@ export function getUpgradeTrailer( ]; if (versionMoved(input)) { + const moved = `The CLI moved from ${input.previousCliVersion ?? ""} to ${input.cliVersion}.`; lines.push( - `The CLI moved from ${input.previousCliVersion ?? ""} to ${input.cliVersion}. ` + - `Run \`${input.invocation} update\` to learn what that changes for the rules already in this project.` + input.reconcileDue === true + ? moved + : `${moved} Run \`${input.invocation} update\` to learn what that changes for the rules already in this project.` ); } diff --git a/packages/cli/src/rules/engine-gate.ts b/packages/cli/src/rules/engine-gate.ts index 536874bb..0d623df2 100644 --- a/packages/cli/src/rules/engine-gate.ts +++ b/packages/cli/src/rules/engine-gate.ts @@ -24,9 +24,9 @@ import { compareVersions } from "../util/version-compare"; * must not be refused by the gate it exists to clear. */ -const ENGINE_LABELS = { sg: "ast-grep", vale: "Vale" } as const; +export const ENGINE_LABELS = { sg: "ast-grep", vale: "Vale" } as const; -type GatedEngine = keyof typeof ENGINE_LABELS; +export type GatedEngine = keyof typeof ENGINE_LABELS; const BUNDLED: Record = { sg: AST_GREP_VERSION, @@ -68,6 +68,18 @@ export function findEngineMismatches( return mismatches; } +/** The gated engines that have at least one rule in this project. */ +export async function findEnginesWithRules( + cwd: string +): Promise> { + const engines = new Set(); + for (const engine of Object.keys(ENGINE_LABELS) as GatedEngine[]) { + const ids = await listRuleIds(cwd, engine); + if (ids.length > 0) engines.add(engine); + } + return engines; +} + /** The refusal's text: what differs, and the remedy for each direction. */ export function engineMismatchMessage( mismatches: EngineMismatch[], @@ -118,13 +130,10 @@ export async function requireReconciledEngines(cwd: string): Promise { } if (recorded === undefined) return; - const enginesWithRules = new Set(); - for (const engine of Object.keys(ENGINE_LABELS) as GatedEngine[]) { - const ids = await listRuleIds(cwd, engine); - if (ids.length > 0) enginesWithRules.add(engine); - } - - const mismatches = findEngineMismatches(recorded, enginesWithRules); + const mismatches = findEngineMismatches( + recorded, + await findEnginesWithRules(cwd) + ); if (mismatches.length === 0) return; throw new CLIError( engineMismatchMessage(mismatches, getCliPrefix()), diff --git a/packages/cli/src/rules/reconcile-status.ts b/packages/cli/src/rules/reconcile-status.ts new file mode 100644 index 00000000..ce66d7ab --- /dev/null +++ b/packages/cli/src/rules/reconcile-status.ts @@ -0,0 +1,124 @@ +import { + ENGINE_LABELS, + findEngineMismatches, + findEnginesWithRules, + type EngineMismatch, +} from "./engine-gate"; +import { reconciliationStart } from "./reconcile-marker"; +import { pendingRuleMigrations } from "./rule-migrate"; +import type { TasklessRulesManifest } from "../filesystem/manifest"; +import { compareVersions } from "../util/version-compare"; + +/** + * Whether a project's rules still need `update --rules`, decided once. + * + * Two things can make a reconciliation due, and they move independently. The + * ledger walk is keyed on the CLI version, compared on its numeric core, so + * every nightly of one release reads as that release and owes no walk. The + * engine gate is keyed on the exact engine versions, and an ast-grep or Vale + * bump lands between two nightlies of one release as easily as between two + * releases. Asking only the first question is how a project on nightlies was + * told "nothing to walk" by `info` and `update` while `check` refused it with + * `RULES_ENGINE_MISMATCH`, so the advice and the enforcement disagreed and + * only the refusal said what to run. + * + * `walk` keeps its narrower meaning, the ledger sections to read. `required` + * is the answer a caller acts on: run the `update` recipe through to its last + * step, which applies `ruleMigrations` and records the engines. + */ +export interface ReconcileStatus { + /** + * Whether the `update` recipe has work to do, ending in `update --rules`. + * While `engines` is non-empty, `check` and `verify` also refuse until then. + */ + required: boolean; + /** The ledger sections to read, or `null` when there are none. */ + walk: { from: string; to: string } | null; + /** + * Engines with rules recorded BEHIND the bundled version: what `check` + * refuses on and `update --rules` clears. An engine recorded ahead is not + * listed, since reconciling with an older CLI does not clear it; the + * refusal itself names the remedy for that. + */ + engines: EngineMismatch[]; + /** The mechanical rewrites `update --rules` applies before it stamps. */ + ruleMigrations: Array<{ + engine: string; + engineVersion: string; + summary: string; + }>; +} + +export async function reconcileStatus( + cwd: string, + rules: TasklessRulesManifest | undefined +): Promise { + const walk = reconciliationStart(rules?.reconciledTo) ?? null; + // The same comparison the gate makes, so what this reports and what `check` + // enforces cannot drift apart. + const engines = findEngineMismatches( + rules?.engines, + await findEnginesWithRules(cwd) + ).filter(({ recorded, bundled }) => compareVersions(recorded, bundled) < 0); + const ruleMigrations = pendingRuleMigrations(rules?.engines).map( + ({ engine, engineVersion, summary }) => ({ engine, engineVersion, summary }) + ); + return { + required: walk !== null || engines.length > 0, + walk, + engines, + ruleMigrations, + }; +} + +/** + * The `reconcile` object every `--json` payload carries: `info` under + * `rules`, `init` and `update` at the top level. One shape, built here, so an + * agent reads the same path whichever command it ran. `walk` is left out + * because each payload already reports it where it always has. + */ +export function reconcileReport( + status: ReconcileStatus +): Omit { + return { + required: status.required, + engines: status.engines, + ruleMigrations: status.ruleMigrations, + }; +} + +/** + * The line `init` prints when a reconciliation is due, or `undefined` when it + * is not. Printed on every run that finds one due, not only on a version move: + * an install between two nightlies, or a re-run after an upgrade nobody + * reconciled, moves nothing and still leaves `check` refusing. + */ +export function getReconcileNotice( + status: ReconcileStatus, + invocation: string +): string | undefined { + if (!status.required) return undefined; + const reasons: string[] = []; + if (status.walk !== null) { + reasons.push( + `the ledger has entries from ${status.walk.from} to ${status.walk.to}` + ); + } + for (const { engine, recorded, bundled } of status.engines) { + reasons.push( + `${ENGINE_LABELS[engine]} moved from ${recorded} to ${bundled}` + ); + } + const rewrites = + status.ruleMigrations.length === 0 + ? "" + : ` It applies ${String(status.ruleMigrations.length)} mechanical rule rewrite(s) for you.`; + const refusal = + status.engines.length === 0 + ? "" + : ", so `check` and `verify` will refuse to run them until it is recorded"; + return ( + `This project's rules are not reconciled to this CLI (${reasons.join("; ")})${refusal}. ` + + `Run \`${invocation} update\` and follow it to its last step, \`${invocation} update --rules\`.${rewrites}` + ); +} diff --git a/packages/cli/src/schemas/info.ts b/packages/cli/src/schemas/info.ts index f3497f13..da8f0e6f 100644 --- a/packages/cli/src/schemas/info.ts +++ b/packages/cli/src/schemas/info.ts @@ -125,6 +125,39 @@ export const outputSchema = z.object({ .describe( "Where a ledger walk should start and end, or null when there is nothing to walk. Computed here so a caller does not re-derive the boundary" ), + reconcile: z + .object({ + required: z + .boolean() + .describe( + "Whether `update --rules` is due: true when there is a walk, or when an engine with rules is recorded behind the bundled one. An engine bump between two nightlies of one release makes this true with `walk` null" + ), + engines: z + .array( + z.object({ + engine: z.enum(["sg", "vale"]), + recorded: z.string(), + bundled: z.string(), + }) + ) + .describe( + "Engines with rules recorded behind the bundled version. `check` and `verify` refuse while this is non-empty" + ), + ruleMigrations: z + .array( + z.object({ + engine: z.string(), + engineVersion: z.string(), + summary: z.string(), + }) + ) + .describe( + "Mechanical rule rewrites `update --rules` applies before it records the reconciliation" + ), + }) + .describe( + "Whether the rules need reconciling, decided once so a caller does not combine `walk` and `engines` itself" + ), }) .describe( "What the rules are valid against. Distinct from `install`: these advance only on a completed reconciliation, never on an upgrade" diff --git a/packages/cli/test/reconcile-marker.test.ts b/packages/cli/test/reconcile-marker.test.ts index 3011714e..1868b943 100644 --- a/packages/cli/test/reconcile-marker.test.ts +++ b/packages/cli/test/reconcile-marker.test.ts @@ -31,6 +31,11 @@ async function runCli( } } +async function runJson(args: string[]): Promise { + const { stdout } = await runCli(args); + return JSON.parse(stdout) as unknown; +} + /** * The marker records that RULE CONTENT was reconciled, which nothing else in * the manifest does. `install.cliVersion` moves on a skills refresh and the @@ -376,6 +381,114 @@ describe("recording a rules reconciliation", () => { expect(payload.walk).toBeNull(); }); + /** + * Every nightly of one release reads as that release to the ledger, so a + * project moving between two of them owes no walk. An engine bump can still + * land between them, and the engine gate compares exact versions. `info` + * and `update` used to say "nothing to walk" while `check` refused, so an + * agent following the recipe stopped before `update --rules`. + */ + describe("an engine bump with nothing to walk", () => { + interface Reconcile { + required: boolean; + engines: { engine: string; recorded: string; bundled: string }[]; + } + + beforeEach(async () => { + await mkdir(join(cwd, ".taskless", "rules", "sg", "r"), { + recursive: true, + }); + await writeFile( + join(cwd, ".taskless", "rules", "sg", "r", "r.yml"), + "id: r\nlanguage: JavaScript\nseverity: error\nmessage: m\nrule:\n pattern: eval($A)\n" + ); + const version = await installedVersion(); + const manifestPath = join(cwd, ".taskless", "taskless.json"); + const manifest = JSON.parse( + await readFile(manifestPath, "utf8") + ) as Record; + manifest["rules"] = { + reconciledTo: version, + engines: { sg: "0.45.3", vale: VALE_VERSION }, + }; + await writeFile(manifestPath, JSON.stringify(manifest, undefined, 2)); + }); + + it("reports a reconcile as required on info and update, with no walk", async () => { + const info = (await runJson(["info", "--json", "-d", cwd])) as { + rules: { walk: unknown; reconcile: Reconcile }; + }; + expect(info.rules.walk).toBeNull(); + expect(info.rules.reconcile.required).toBe(true); + expect(info.rules.reconcile.engines).toEqual([ + { engine: "sg", recorded: "0.45.3", bundled: AST_GREP_VERSION }, + ]); + + const update = (await runJson(["update", "--json", "-d", cwd])) as { + walk: unknown; + reconcile: Reconcile; + }; + expect(update.walk).toBeNull(); + expect(update.reconcile.required).toBe(true); + }); + + it("has init name update --rules, on the human path and in --json", async () => { + // A version move as well, so the upgrade trailer prints beside the + // notice and has to leave the instruction to it. + const manifestPath = join(cwd, ".taskless", "taskless.json"); + const manifest = JSON.parse(await readFile(manifestPath, "utf8")) as { + install: Record; + }; + manifest.install["cliVersion"] = "0.0.1-previous"; + await writeFile(manifestPath, JSON.stringify(manifest, undefined, 2)); + + const human = await runCli(["init", "-d", cwd]); + expect(human.exitCode).toBe(0); + expect(human.stdout).toContain("not reconciled to this CLI"); + expect(human.stdout).toContain( + `ast-grep moved from 0.45.3 to ${AST_GREP_VERSION}` + ); + expect(human.stdout).toMatch(/update --rules/); + expect(human.stdout).toContain("will refuse"); + + // The upgrade trailer states the move without a second "Run update". + expect(human.stdout).toContain("moved from 0.0.1-previous to"); + expect(human.stdout.match(/update`/g)).toHaveLength(1); + + // A re-run moves no version and still has to say it. + const rerun = await runCli(["init", "-d", cwd]); + expect(rerun.stdout).toContain("not reconciled to this CLI"); + + const json = (await runJson(["init", "--json", "-d", cwd])) as { + reconcile: Reconcile; + }; + expect(json.reconcile.required).toBe(true); + // One shape across the three commands that report it. + const info = (await runJson(["info", "--json", "-d", cwd])) as { + rules: { reconcile: Reconcile }; + }; + const update = (await runJson(["update", "--json", "-d", cwd])) as { + reconcile: Reconcile; + }; + expect(update.reconcile).toEqual(info.rules.reconcile); + expect(json.reconcile).toEqual(info.rules.reconcile); + }); + + it("clears once update --rules records the engines", async () => { + const reconciled = await runCli(["update", "--rules", "-d", cwd]); + expect(reconciled.exitCode).toBe(0); + const info = (await runJson(["info", "--json", "-d", cwd])) as { + rules: { reconcile: Reconcile }; + }; + expect(info.rules.reconcile).toMatchObject({ + required: false, + engines: [], + }); + const human = await runCli(["init", "-d", cwd]); + expect(human.stdout).not.toContain("not reconciled to this CLI"); + }); + }); + it("leaves install untouched, since the two namespaces drift apart", async () => { const before = JSON.parse( await readFile(join(cwd, ".taskless", "taskless.json"), "utf8") diff --git a/packages/cli/test/reconcile-status.test.ts b/packages/cli/test/reconcile-status.test.ts new file mode 100644 index 00000000..67e65954 --- /dev/null +++ b/packages/cli/test/reconcile-status.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from "vitest"; + +import { + getReconcileNotice, + type ReconcileStatus, +} from "../src/rules/reconcile-status"; + +const NOTHING: ReconcileStatus = { + required: false, + walk: null, + engines: [], + ruleMigrations: [], +}; + +/** + * `init` prints this whenever a reconciliation is due, not only on a version + * move, so the wording has to say which kind is due and whether `check` will + * refuse in the meantime. + */ +describe("the reconcile notice", () => { + it("prints nothing when no reconcile is due", () => { + expect(getReconcileNotice(NOTHING, "taskless")).toBeUndefined(); + }); + + it("names an engine move and the refusal it causes", () => { + const notice = getReconcileNotice( + { + ...NOTHING, + required: true, + engines: [{ engine: "vale", recorded: "3.20.0", bundled: "3.24.0" }], + ruleMigrations: [ + { engine: "vale", engineVersion: "3.24.0", summary: "s" }, + ], + }, + "./.taskless/bin/cli.mjs" + ); + expect(notice).toContain("Vale moved from 3.20.0 to 3.24.0"); + expect(notice).toContain("will refuse"); + expect(notice).toContain("`./.taskless/bin/cli.mjs update --rules`"); + expect(notice).toContain("1 mechanical rule rewrite(s)"); + }); + + it("does not claim a refusal for a walk alone", () => { + // `check` gates on the engines, not on the ledger. + const notice = getReconcileNotice( + { ...NOTHING, required: true, walk: { from: "0.11.0", to: "0.12.0" } }, + "taskless" + ); + expect(notice).toContain("ledger has entries from 0.11.0 to 0.12.0"); + expect(notice).not.toContain("refuse"); + }); +});