diff --git a/.changeset/plan-aware-recovery.md b/.changeset/plan-aware-recovery.md new file mode 100644 index 00000000..b0e9a0d2 --- /dev/null +++ b/.changeset/plan-aware-recovery.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +`taskless check` stops suggesting `taskless rule restore` to an organization whose plan does not include restoring rules. For an edited or missing rule, and for a renamed one, it gives the `git log` and `git restore` steps for the rule's directory instead, so a user is no longer sent to run a command the service will refuse. The plan is read from the `whoami` call the CLI already makes, so no request is added. When the plan is unknown, the suggestion is `rule restore` as before, and `rule restore` still asks the service on every plan. diff --git a/openspec/changes/cli-plan-aware-recovery/design.md b/openspec/changes/cli-plan-aware-recovery/design.md index 8498477a..cdea7146 100644 --- a/openspec/changes/cli-plan-aware-recovery/design.md +++ b/openspec/changes/cli-plan-aware-recovery/design.md @@ -53,7 +53,7 @@ git: \`git log -- \` lists the commits that changed it, and \`git restore --source= -- \` puts it back as of one of them.` `` is `.taskless/rules///`, or the quoted glob pathspec -`'.taskless/rules/*//'` when a `missing` verdict carries no known engine. The +`'.taskless/rules/*//*'` when a `missing` verdict carries no known engine. The rendering lives in a small pure function beside `applyVerdicts` so it is tested as a table like the rest of the policy. Considered: passing the tri-state into `applyVerdicts`. Rejected to keep `verdicts.ts` free of CLI prefix and plan concerns, which is why the callback exists. @@ -80,9 +80,11 @@ excludes recovery; follow them rather than running a command that will be refuse case is one run with a stale suggestion. - [A `false` from whoami that the service would not refuse] → The user is sent to git, which still works. Nothing is blocked, so a wrong hint costs a detour, never a capability. -- [Glob pathspec on an unknown engine] → Quoted so the shell does not expand it; git applies - glob pathspecs by default. Only reachable when reconcile omits the engine, which it rarely - does. +- [Glob pathspec on an unknown engine] → Quoted so the shell does not expand it, and ending + in `/*`: git matches a glob against file paths, so a glob ending in the directory's `/` + matches nothing (measured, in a scratch repository: `log` returned no commits, while `/*` + found both the add and the delete, and `restore` from the delete's parent brought the + files back). Only reachable when reconcile omits the engine. ## Migration Plan diff --git a/openspec/changes/cli-plan-aware-recovery/tasks.md b/openspec/changes/cli-plan-aware-recovery/tasks.md index 45bf2eb5..cbd89d6f 100644 --- a/openspec/changes/cli-plan-aware-recovery/tasks.md +++ b/openspec/changes/cli-plan-aware-recovery/tasks.md @@ -5,8 +5,8 @@ ## 2. Suggestions in check -- [ ] 2.1 Replace `applyVerdicts`' `restoreCommand` with a `recovery({ ruleId, engine?, purpose })` sentence callback, and add the pure renderer for the restore and git variants (engine-less `missing` uses the quoted any-engine pathspec); verify every existing `verdicts.test.ts` case passes unchanged with `restoreRules` unknown -- [ ] 2.2 Pass the tri-state from `resolveActingOrg` into the callback in `plan-check.ts`; verify with `verdicts.test.ts` cases for `unsafe` (runtime and static), `missing` (with and without engine), and a rename, each under `false` giving git steps and not naming `rule restore` +- [x] 2.1 Replace `applyVerdicts`' `restoreCommand` with a `recovery({ ruleId, engine?, purpose })` sentence callback, and add the pure renderer for the restore and git variants (engine-less `missing` uses the quoted any-engine pathspec); verify every existing `verdicts.test.ts` case passes unchanged with `restoreRules` unknown +- [x] 2.2 Pass the tri-state from `resolveActingOrg` into the callback in `plan-check.ts`; verify with `verdicts.test.ts` cases for `unsafe` (runtime and static), `missing` (with and without engine), and a rename, each under `false` giving git steps and not naming `rule restore` ## 3. Suggestions in rule revisions diff --git a/packages/cli/src/agent/check.md b/packages/cli/src/agent/check.md index bde02a25..480c216e 100644 --- a/packages/cli/src/agent/check.md +++ b/packages/cli/src/agent/check.md @@ -1,4 +1,4 @@ -# Topic: check (CLI v%(CLI_VERSION)s / topic v4) +# Topic: check (CLI v%(CLI_VERSION)s / topic v5) ## Goal Run the applicable rules against the codebase and report matches. Two @@ -48,7 +48,9 @@ logged in: `check` NEVER changes `.taskless/rules/`. An edited or missing rule is reported with the command that repairs it: -`%(TASKLESS_CLI)s rule restore `. +`%(TASKLESS_CLI)s rule restore `, or, when the organization's +plan does not include restoring rules, the git steps that do instead +(see "When the plan does not include restoring rules"). Notices about skipped runtime rules are human-readable stderr only. Under `--json` they do NOT appear as warnings; instead an additive @@ -119,6 +121,28 @@ rules the answer did not account for (`unaccounted`), and ids shared across engines (`duplicate`). Locally written ast-grep and Vale rules are never listed; they run. A copy is listed with its `copyOf`. +## When the plan does not include restoring rules + +When the organization's plan is known not to include restoring rules, +every notice above gives git steps where it would name `rule restore`: + +``` +sg rule no-eval-3fa9c21b was edited since Taskless issued it (changed no-eval-3fa9c21b.yml), so it did not run and `check` fails. Restoring rules is not included in your organization's plan, so recover no-eval-3fa9c21b from git: `git log -- .taskless/rules/sg/no-eval-3fa9c21b/` lists the commits that changed it, and `git restore --source= -- .taskless/rules/sg/no-eval-3fa9c21b/` puts it back as of one of them. +``` + +Follow the git steps, then run `check` again. Do not run +`rule restore` or `rule rollback` instead: the service refuses both on +this plan and answers with the same git steps. `integrity` is the same +on every plan. + +Choosing the commit: for an edited rule, restore from the commit just +before the edit, or from `HEAD` when the edit is not committed yet. For +a deleted rule, the newest commit is the one +that deleted it, so restore from its parent (`~1`). A rule +whose engine is not known is given as a quoted pathspec, +`'.taskless/rules/*//*'`; pass it to git as written. For a +rename, recover the source, then delete the copy, as above. + ## Withheld for the plan When the organization's Taskless plan does not include runtime rules, diff --git a/packages/cli/src/rules/plan-check.ts b/packages/cli/src/rules/plan-check.ts index 2db16831..e96988b8 100644 --- a/packages/cli/src/rules/plan-check.ts +++ b/packages/cli/src/rules/plan-check.ts @@ -3,6 +3,7 @@ import { resolveActingOrg } from "../auth/org"; import { reconcileRules } from "../api/v2"; import { resolveRepositoryUrl } from "../util/git-remote"; import { getCliPrefix } from "../util/package-manager"; +import { recoveryAdvice } from "./recovery-advice"; import { reportRules } from "./report"; import type { RunDirectory } from "./run-directory"; import { RUN_SCRIPTS_WARNING } from "./runtime/harness"; @@ -37,7 +38,8 @@ import { * excludes is removed from the snapshot before any engine is configured. * * `check` never writes `.taskless/rules/`. An edited or missing rule gets a - * notice naming `rule restore`; nothing here fetches bytes. + * notice naming `rule restore`, or the git steps when the plan is known not + * to serve it (`recovery-advice.ts`); nothing here fetches bytes. */ /** A runtime rule that will not run, with why. */ @@ -238,7 +240,10 @@ export async function planCheck( const verdicts = applyVerdicts( report.rules, outcome.data, - (ruleId) => `${getCliPrefix()} rule restore ${ruleId}` + recoveryAdvice( + org.restoreRules, + (ruleId) => `${getCliPrefix()} rule restore ${ruleId}` + ) ); for (const disposition of verdicts.dispositions) { log.write( diff --git a/packages/cli/src/rules/recovery-advice.ts b/packages/cli/src/rules/recovery-advice.ts new file mode 100644 index 00000000..e049e41f --- /dev/null +++ b/packages/cli/src/rules/recovery-advice.ts @@ -0,0 +1,74 @@ +import type { EngineName } from "./layout"; + +/** + * How a notice tells the user to put back an issued rule. + * + * The plan decides which answer is honest. When the acting organization's + * `restoreRules` entitlement is exactly `false`, the service will refuse + * `rule restore`, so suggesting it only sends the user to be told to use git. + * They get the git steps directly instead. Unknown (whoami failed, no + * entitlement sent, or no organization matched) keeps suggesting + * `rule restore`, word for word as before: the service answers a restore on + * its own, so a wrong guess costs one refused call, never a lost capability. + * + * This decides only what is suggested. Nothing here stops a user running + * `rule restore`, which always asks the service. + */ + +/** A rule a notice points at, and what putting it back is for. */ +export interface RecoveryTarget { + ruleId: string; + /** The rule's engine, when known. Unknown widens the git pathspec. */ + engine?: EngineName; + /** Completes "Run `…` to ", e.g. "put back the issued version". */ + purpose: string; + /** A step after recovering, e.g. "delete .taskless/rules/vale/bar-2/". */ + afterwards?: string; + /** What to do instead of recovering, e.g. "ignore this if …". */ + otherwise?: string; +} + +/** Renders the sentence a notice ends with. */ +export type Recovery = (target: RecoveryTarget) => string; + +/** + * The directory `git` should look at. Quoted when the engine is unknown, so + * the shell passes the glob to git as a pathspec rather than expanding it. + * The glob matches the rule's files, not its directory, because git matches + * a glob against file paths: measured, a glob ending in the directory's slash + * finds no commits in `git log`, while one ending in a file wildcard finds + * them and works for `restore` too. An exact directory needs no glob. + */ +function ruleDirectory(ruleId: string, engine?: EngineName): string { + return engine === undefined + ? `'.taskless/rules/*/${ruleId}/*'` + : `.taskless/rules/${engine}/${ruleId}/`; +} + +/** The sentence for a plan known not to include rule recovery. */ +function gitSteps({ ruleId, engine, afterwards, otherwise }: RecoveryTarget) { + const directory = ruleDirectory(ruleId, engine); + return ( + `Restoring rules is not included in your organization's plan, so recover ${ruleId} from git: ` + + `\`git log -- ${directory}\` lists the commits that changed it, and ` + + `\`git restore --source= -- ${directory}\` puts it back as of one of them.` + + (afterwards === undefined ? "" : ` Then ${afterwards}.`) + + (otherwise === undefined ? "" : ` Or ${otherwise}.`) + ); +} + +/** + * Build the recovery sentence for a plan. `restoreCommand` renders the + * `rule restore` invocation, so this stays free of how the CLI was invoked. + */ +export function recoveryAdvice( + restoreRules: boolean | undefined, + restoreCommand: (ruleId: string) => string +): Recovery { + if (restoreRules === false) return gitSteps; + return ({ ruleId, purpose, afterwards, otherwise }) => + `Run \`${restoreCommand(ruleId)}\` to ${purpose}` + + (afterwards === undefined ? "" : `, then ${afterwards}`) + + (otherwise === undefined ? "" : `, or ${otherwise}`) + + "."; +} diff --git a/packages/cli/src/rules/verdicts.ts b/packages/cli/src/rules/verdicts.ts index ff09c29a..f8ce86e2 100644 --- a/packages/cli/src/rules/verdicts.ts +++ b/packages/cli/src/rules/verdicts.ts @@ -2,6 +2,7 @@ import { parseEntitlementV2, type EntitlementV2 } from "../api/entitlement"; import { isRecord } from "../util/is-record"; import type { EngineName } from "./layout"; import { isKnownEngine } from "./layout"; +import type { Recovery } from "./recovery-advice"; import type { ReportedRule } from "./report"; /** @@ -106,6 +107,10 @@ export interface VerdictPlan { /** The reason a runtime rule the plan withholds did not run. */ export const NOT_IN_PLAN_REASON = "not included in your Taskless plan"; +function readEngine(value: unknown): EngineName | undefined { + return typeof value === "string" && isKnownEngine(value) ? value : undefined; +} + function records(value: unknown): Record[] { return Array.isArray(value) ? value.filter((entry) => isRecord(entry)) : []; } @@ -195,7 +200,8 @@ function applyCopy( rule: ReportedRule, copy: Extract, sourceMissing: boolean, - restoreCommand: (ruleId: string) => string + sourceEngine: EngineName | undefined, + recovery: Recovery ): void { const { ruleId, engine } = rule; const source = copy.ruleId; @@ -206,7 +212,12 @@ function applyCopy( ? `is a copy of Taskless rule ${source}, which was deleted${changes}` : `is a copy of Taskless rule ${source}${changes}`; const fix = sourceMissing - ? `Run \`${restoreCommand(source)}\` to put back the issued rule, then delete ${directory}.` + ? recovery({ + ruleId: source, + ...(sourceEngine === undefined ? {} : { engine: sourceEngine }), + purpose: "put back the issued rule", + afterwards: `delete ${directory}`, + }) : `Delete ${directory}, or rewrite the files it carries from ${source} so it is your own rule.`; plan.integrity.push({ @@ -254,13 +265,13 @@ function applyCopy( /** * Apply a reconcile response to the rules that were reported. * - * `restoreCommand` renders the command a notice points at, so this stays free - * of how the CLI was invoked. + * `recovery` renders how a notice says to put a rule back, so this stays free + * of how the CLI was invoked and of what the organization's plan serves. */ export function applyVerdicts( reported: readonly ReportedRule[], response: unknown, - restoreCommand: (ruleId: string) => string + recovery: Recovery ): VerdictPlan { const body = isRecord(response) ? response : {}; const entitlement = parseEntitlementV2(body.entitlement); @@ -285,13 +296,13 @@ export function applyVerdicts( }; const reportedIds = new Set(reported.map((rule) => rule.ruleId)); - // Rules answered `missing` that were not reported: a copy naming one of - // these as its source is a rename. - const missingIds = new Set( + // Rules answered `missing` that were not reported, with their engine when + // known: a copy naming one of these as its source is a rename. + const missingIds = new Map( verdicts .filter((entry) => entry.verdict === "missing") - .map((entry) => entry.ruleId as string) - .filter((id) => !reportedIds.has(id)) + .filter((entry) => !reportedIds.has(entry.ruleId as string)) + .map((entry) => [entry.ruleId as string, readEngine(entry.engine)]) ); // Sources already reported as half of a rename, so their `missing` // notice is not repeated. @@ -343,7 +354,14 @@ export function applyVerdicts( if (copy.status === "copy") { const sourceMissing = missingIds.has(copy.ruleId); if (sourceMissing) renamed.add(copy.ruleId); - applyCopy(plan, rule, copy, sourceMissing, restoreCommand); + applyCopy( + plan, + rule, + copy, + sourceMissing, + missingIds.get(copy.ruleId), + recovery + ); continue; } if (engine === "runtime") { @@ -406,7 +424,7 @@ export function applyVerdicts( reason: `edited since Taskless issued it (${changes})`, }); plan.notices.push( - `runtime rule ${ruleId} was edited since Taskless issued it (${changes}), so it did not run. Run \`${restoreCommand(ruleId)}\` to put back the issued version.` + `runtime rule ${ruleId} was edited since Taskless issued it (${changes}), so it did not run. ${recovery({ ruleId, engine, purpose: "put back the issued version" })}` ); } else { plan.dispositions.push({ @@ -417,7 +435,7 @@ export function applyVerdicts( reason: `edited since Taskless issued it (${changes})`, }); plan.failures.push( - `${engine} rule ${ruleId} was edited since Taskless issued it (${changes}), so it did not run and \`check\` fails. Run \`${restoreCommand(ruleId)}\` to put back the issued version.` + `${engine} rule ${ruleId} was edited since Taskless issued it (${changes}), so it did not run and \`check\` fails. ${recovery({ ruleId, engine, purpose: "put back the issued version" })}` ); } break; @@ -438,10 +456,7 @@ export function applyVerdicts( if (entry.verdict !== "missing") continue; const ruleId = entry.ruleId as string; if (reportedIds.has(ruleId)) continue; // already failed as unaccounted - const engine = - typeof entry.engine === "string" && isKnownEngine(entry.engine) - ? entry.engine - : undefined; + const engine = readEngine(entry.engine); const revisionId = typeof entry.revisionId === "string" ? entry.revisionId : undefined; plan.integrity.push({ @@ -453,7 +468,14 @@ export function applyVerdicts( // Half of a rename: the copy's own message already says it was deleted. if (renamed.has(ruleId)) continue; plan.notices.push( - `${engine ?? "A"} rule ${ruleId} was issued for this repository but is not in .taskless/rules/. Run \`${restoreCommand(ruleId)}\` to bring it back, or ignore this if it was removed on purpose.` + `${engine ?? "A"} rule ${ruleId} was issued for this repository but is not in .taskless/rules/. ${recovery( + { + ruleId, + ...(engine === undefined ? {} : { engine }), + purpose: "bring it back", + otherwise: "ignore this if it was removed on purpose", + } + )}` ); } diff --git a/packages/cli/test/runtime-check.test.ts b/packages/cli/test/runtime-check.test.ts index 5491aeb7..473664ce 100644 --- a/packages/cli/test/runtime-check.test.ts +++ b/packages/cli/test/runtime-check.test.ts @@ -24,6 +24,7 @@ interface ReportedRule { } interface ReconcileRequestBody { repositoryUrl: string; + orgId?: string | number; rules: ReportedRule[]; } type Responder = (request: ReconcileRequestBody) => { @@ -39,13 +40,29 @@ interface MockServer { close: () => Promise; } -/** Start a mock v2 reconcile endpoint on a random port. */ -function startMockServer(responder: Responder): Promise { +/** + * Start a mock v2 reconcile endpoint on a random port. `whoami`, when given, + * is served as `GET /cli/api/v2/whoami`; otherwise that route is a 404, which + * the CLI reads as an unknown organization. + */ +function startMockServer( + responder: Responder, + whoami?: unknown +): Promise { const requests: ReconcileRequestBody[] = []; const paths: string[] = []; const headers: Record[] = []; const server: Server = createServer((request, response) => { paths.push(`${request.method ?? ""} ${request.url ?? ""}`); + if ( + whoami !== undefined && + request.method === "GET" && + request.url === "/cli/api/v2/whoami" + ) { + response.writeHead(200, { "content-type": "application/json" }); + response.end(JSON.stringify(whoami)); + return; + } if (request.method !== "POST" || request.url !== "/cli/api/v2/reconcile") { response.writeHead(404).end("{}"); return; @@ -315,9 +332,10 @@ describe("check: static vs runtime dispatch", () => { /** Run \`check\` authenticated against a mock that answers with \`responder\`. */ async function authedCheck( responder: Responder, - extraArguments: string[] = ["--json"] + extraArguments: string[] = ["--json"], + whoami?: unknown ) { - const server = await startMockServer(responder); + const server = await startMockServer(responder, whoami); try { const result = await runCli( ["check", "-d", directory, ...extraArguments], @@ -488,6 +506,52 @@ describe("check: static vs runtime dispatch", () => { expect(output.notices?.join("\n")).toMatch(/rule restore gone-3fa9c21b/); }); + it("on a plan without rule recovery, an edited or missing rule gets git steps, not rule restore", async () => { + const whoami = { + user: "Ada", + orgs: [ + { + id: "uuid-acme", + name: "acme", + source: "github", + url: "https://github.com/acme", + entitlements: { + remoteGeneration: true, + runtimeSignatures: true, + restoreRules: false, + }, + }, + ], + }; + const { stdout, server } = await authedCheck( + (request) => ({ + statusCode: 200, + body: answer( + request, + { + "no-console": "run", + demo: { + unsafe: [{ path: "captures/extra.yml", got: "1;h=sha-256;d=22" }], + }, + }, + { + missing: [ + { ruleId: "gone-3fa9c21b", engine: "vale", revisionId: "rev-9" }, + ], + } + ), + }), + ["--json"], + whoami + ); + const notices = parseJson(stdout).notices?.join("\n") ?? ""; + // The acting org came from the same whoami call that carried the plan. + expect(server.requests[0]?.orgId).toBe("uuid-acme"); + expect(notices).toContain("git log -- .taskless/rules/runtime/demo/"); + expect(notices).toContain("git log -- .taskless/rules/vale/gone-3fa9c21b/"); + expect(notices).not.toContain("rule restore"); + }); + it("a renamed copy of an issued rule does not run, fails once as a rename, and logs it", async () => { await migrateFixture(["-d", directory]); const before = await treeDigest(directory); diff --git a/packages/cli/test/verdicts.test.ts b/packages/cli/test/verdicts.test.ts index b88aa2b7..c9cafdb0 100644 --- a/packages/cli/test/verdicts.test.ts +++ b/packages/cli/test/verdicts.test.ts @@ -1,9 +1,12 @@ import { describe, expect, it } from "vitest"; import type { ReportedRule } from "../src/rules/report"; +import { recoveryAdvice } from "../src/rules/recovery-advice"; import { applyVerdicts, NOT_IN_PLAN_REASON } from "../src/rules/verdicts"; const restore = (id: string) => `taskless rule restore ${id}`; +// Unknown plan: today's `rule restore` suggestions, word for word. +const recovery = recoveryAdvice(undefined, restore); const SG: ReportedRule = { ruleId: "no-eval-3fa9c21b", @@ -41,7 +44,7 @@ describe("applyVerdicts", () => { unknown: [], entitlement: entitled, }, - restore + recovery ); expect(plan.dispositions.every((d) => d.run)).toBe(true); expect(plan.failures).toEqual([]); @@ -68,7 +71,7 @@ describe("applyVerdicts", () => { unknown: [], entitlement: entitled, }, - restore + recovery ); expect(plan.dispositions).toMatchObject([{ run: false }]); expect(plan.failures).toHaveLength(1); @@ -97,7 +100,7 @@ describe("applyVerdicts", () => { unknown: [], entitlement: entitled, }, - restore + recovery ); expect(plan.dispositions).toMatchObject([{ run: false }]); expect(plan.failures).toEqual([]); @@ -112,7 +115,7 @@ describe("applyVerdicts", () => { unknown: [{ ruleId: SG.ruleId }, { ruleId: RT.ruleId }], entitlement: entitled, }, - restore + recovery ); expect(plan.dispositions).toEqual([ { ruleId: SG.ruleId, engine: "sg", run: true, verdict: "unknown" }, @@ -146,7 +149,7 @@ describe("applyVerdicts", () => { unknown: [{ ruleId: VALE.ruleId, copyOf: COPY_OF }], entitlement: entitled, }, - restore + recovery ); expect(plan.dispositions).toEqual([ { @@ -180,7 +183,7 @@ describe("applyVerdicts", () => { unknown: [{ ruleId: VALE.ruleId, copyOf: COPY_OF }], entitlement: entitled, }, - restore + recovery ); expect(plan.dispositions).toMatchObject([{ run: false }]); expect(plan.failures).toEqual([ @@ -211,7 +214,7 @@ describe("applyVerdicts", () => { unknown: [{ ruleId: SG.ruleId, copyOf: COPY_OF }], entitlement: entitled, }, - restore + recovery ); expect(plan.failures).toHaveLength(1); expect(plan.failures[0]).toContain(`sg rule ${SG.ruleId} is a copy of`); @@ -228,7 +231,7 @@ describe("applyVerdicts", () => { unknown: [{ ruleId: RT.ruleId, copyOf: { ...COPY_OF, files: [] } }], entitlement: entitled, }, - restore + recovery ); expect(plan.failures).toEqual([]); expect(plan.notices).toEqual([]); @@ -255,7 +258,7 @@ describe("applyVerdicts", () => { unknown: [{ ruleId: RT.ruleId, copyOf: COPY_OF }], entitlement: entitled, }, - restore + recovery ); expect(plan.failures).toEqual([]); expect(plan.notices).toEqual([ @@ -271,7 +274,7 @@ describe("applyVerdicts", () => { unknown: [{ ruleId: SG.ruleId, copyOf: null }], entitlement: entitled, }, - restore + recovery ); expect(plan.dispositions).toMatchObject([{ run: true }]); expect(plan.failures).toEqual([]); @@ -292,7 +295,7 @@ describe("applyVerdicts", () => { unknown: [{ ruleId: rule.ruleId, copyOf }], entitlement: entitled, }, - restore + recovery ); expect(plan.dispositions).toMatchObject([ { run: false, verdict: "unaccounted" }, @@ -323,7 +326,7 @@ describe("applyVerdicts", () => { withheld: [{ ruleId: RT.ruleId, revisionId: "r1" }], }, }, - restore + recovery ); expect(plan.withheld).toEqual([RT.ruleId]); expect(plan.dispositions).toEqual([ @@ -342,7 +345,7 @@ describe("applyVerdicts", () => { const plan = applyVerdicts( [SG], { rules: [], unknown: [], entitlement: entitled }, - restore + recovery ); expect(plan.dispositions).toMatchObject([{ run: false }]); expect(plan.failures[0]).toContain("did not account for it"); @@ -362,7 +365,7 @@ describe("applyVerdicts", () => { withheld: [{ ruleId: RT.ruleId, revisionId: "r1" }], }, }, - restore + recovery ); expect(plan.failures[0]).toContain("more than once"); expect(plan.withheld).toEqual([]); @@ -376,7 +379,7 @@ describe("applyVerdicts", () => { unknown: [], entitlement: entitled, }, - restore + recovery ); expect(plan.dispositions).toMatchObject([{ run: false }]); expect(plan.failures[0]).toContain("judged it as a vale rule"); @@ -397,7 +400,7 @@ describe("applyVerdicts", () => { unknown: [], entitlement: entitled, }, - restore + recovery ); expect(plan.failures).toEqual([]); expect(plan.integrity).toEqual([ @@ -426,15 +429,224 @@ describe("applyVerdicts", () => { unknown: [], entitlement: entitled, }, - restore + recovery ); expect(plan.dispositions).toMatchObject([{ run: false }]); expect(plan.failures).toHaveLength(1); }); it("a malformed body accounts for nothing, so every reported rule fails", () => { - const plan = applyVerdicts([SG, RT], "not an object", restore); + const plan = applyVerdicts([SG, RT], "not an object", recovery); expect(plan.dispositions.every((d) => !d.run)).toBe(true); expect(plan.failures).toHaveLength(2); }); }); + +describe("recoveryAdvice", () => { + const target = { + ruleId: "no-eval-3fa9c21b", + engine: "sg" as const, + purpose: "put back the issued version", + }; + + it.each([true, undefined])( + "names rule restore when restoreRules is %s", + (restoreRules) => { + expect(recoveryAdvice(restoreRules, restore)(target)).toBe( + "Run `taskless rule restore no-eval-3fa9c21b` to put back the issued version." + ); + } + ); + + it("gives the git steps for the rule's directory when restoreRules is false", () => { + expect(recoveryAdvice(false, restore)(target)).toBe( + "Restoring rules is not included in your organization's plan, so recover no-eval-3fa9c21b from git: " + + "`git log -- .taskless/rules/sg/no-eval-3fa9c21b/` lists the commits that changed it, and " + + "`git restore --source= -- .taskless/rules/sg/no-eval-3fa9c21b/` puts it back as of one of them." + ); + }); + + it("widens to a quoted any-engine pathspec when the engine is unknown", () => { + const sentence = recoveryAdvice( + false, + restore + )({ + ruleId: "gone-3fa9c21b", + purpose: "bring it back", + }); + expect(sentence).toContain( + "`git log -- '.taskless/rules/*/gone-3fa9c21b/*'`" + ); + }); + + it("keeps afterwards and otherwise as sentences of their own", () => { + const sentence = recoveryAdvice( + false, + restore + )({ + ...target, + afterwards: "delete .taskless/rules/vale/bar-2/", + otherwise: "ignore this if it was removed on purpose", + }); + expect(sentence).toMatch( + / Then delete \.taskless\/rules\/vale\/bar-2\/\. Or ignore this if it was removed on purpose\.$/ + ); + }); +}); + +describe("applyVerdicts on a plan without rule recovery", () => { + const noRecovery = recoveryAdvice(false, restore); + const GIT = "Restoring rules is not included in your organization's plan"; + + it("an unsafe static rule fails with git steps, not rule restore", () => { + const plan = applyVerdicts( + [VALE], + { + rules: [ + { + ruleId: VALE.ruleId, + engine: "vale", + verdict: "unsafe", + files: [{ path: ".vale.ini", expected: "e", got: "g" }], + }, + ], + unknown: [], + entitlement: entitled, + }, + noRecovery + ); + expect(plan.failures).toHaveLength(1); + expect(plan.failures[0]).toContain(GIT); + expect(plan.failures[0]).toContain( + `git log -- .taskless/rules/vale/${VALE.ruleId}/` + ); + expect(plan.failures[0]).not.toContain("rule restore"); + }); + + it("an unsafe runtime rule's notice gives git steps, not rule restore", () => { + const plan = applyVerdicts( + [RT], + { + rules: [ + { + ruleId: RT.ruleId, + engine: "runtime", + verdict: "unsafe", + files: [], + }, + ], + unknown: [], + entitlement: entitled, + }, + noRecovery + ); + expect(plan.notices[0]).toContain( + `git log -- .taskless/rules/runtime/${RT.ruleId}/` + ); + expect(plan.notices[0]).not.toContain("rule restore"); + }); + + it.each([ + ["sg", ".taskless/rules/sg/gone-3fa9c21b/"], + [undefined, "'.taskless/rules/*/gone-3fa9c21b/*'"], + ])( + "a missing rule (engine %s) gets git steps and may still be ignored", + (engine, directory) => { + const plan = applyVerdicts( + [], + { + rules: [ + { + ruleId: "gone-3fa9c21b", + ...(engine === undefined ? {} : { engine }), + verdict: "missing", + revisionId: "r9", + }, + ], + unknown: [], + entitlement: entitled, + }, + noRecovery + ); + expect(plan.notices).toHaveLength(1); + expect(plan.notices[0]).toContain(`git log -- ${directory}`); + expect(plan.notices[0]).toContain( + "Or ignore this if it was removed on purpose." + ); + expect(plan.notices[0]).not.toContain("rule restore"); + } + ); + + it("a rename gives git steps for the source, then says to delete the copy", () => { + const SOURCE = "no-simply-00000000"; + const plan = applyVerdicts( + [VALE], + { + rules: [ + { + ruleId: SOURCE, + engine: "vale", + verdict: "missing", + revisionId: "r8", + }, + ], + unknown: [ + { + ruleId: VALE.ruleId, + copyOf: { + ruleId: SOURCE, + revisionId: "r7", + files: [{ path: ".vale.ini", expected: "e", got: "g" }], + }, + }, + ], + entitlement: entitled, + }, + noRecovery + ); + expect(plan.failures).toEqual([ + `vale rule ${VALE.ruleId} is a copy of Taskless rule ${SOURCE}, which was deleted (changed .vale.ini), so it did not run and \`check\` fails. ` + + `${GIT}, so recover ${SOURCE} from git: ` + + `\`git log -- .taskless/rules/vale/${SOURCE}/\` lists the commits that changed it, and ` + + `\`git restore --source= -- .taskless/rules/vale/${SOURCE}/\` puts it back as of one of them. ` + + `Then delete .taskless/rules/vale/${VALE.ruleId}/.`, + ]); + expect(plan.notices).toEqual([]); + }); + + it("a rename whose source has no known engine widens the source's pathspec, and still names the copy's directory", () => { + const SOURCE = "no-simply-00000000"; + const plan = applyVerdicts( + [VALE], + { + rules: [{ ruleId: SOURCE, verdict: "missing", revisionId: "r8" }], + unknown: [ + { + ruleId: VALE.ruleId, + copyOf: { + ruleId: SOURCE, + revisionId: "r7", + files: [{ path: ".vale.ini", expected: "e", got: "g" }], + }, + }, + ], + entitlement: entitled, + }, + noRecovery + ); + expect(plan.failures).toHaveLength(1); + expect(plan.failures[0]).toContain( + `\`git log -- '.taskless/rules/*/${SOURCE}/*'\`` + ); + expect(plan.failures[0]).toContain( + `\`git restore --source= -- '.taskless/rules/*/${SOURCE}/*'\`` + ); + expect( + plan.failures[0]?.endsWith( + ` Then delete .taskless/rules/vale/${VALE.ruleId}/.` + ) + ).toBe(true); + // Still one rename: the source's missing warning is not repeated. + expect(plan.notices).toEqual([]); + }); +});