Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/plan-aware-recovery.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
Comment thread
theCodeDrift marked this conversation as resolved.
"@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.
10 changes: 6 additions & 4 deletions openspec/changes/cli-plan-aware-recovery/design.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ git: \`git log -- <dir>\` lists the commits that changed it, and
\`git restore --source=<commit> -- <dir>\` puts it back as of one of them.`

`<dir>` is `.taskless/rules/<engine>/<ruleId>/`, or the quoted glob pathspec
`'.taskless/rules/*/<ruleId>/'` when a `missing` verdict carries no known engine. The
`'.taskless/rules/*/<ruleId>/*'` 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.
Expand All @@ -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

Expand Down
4 changes: 2 additions & 2 deletions openspec/changes/cli-plan-aware-recovery/tasks.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
28 changes: 26 additions & 2 deletions packages/cli/src/agent/check.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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 <ruleId>`.
`%(TASKLESS_CLI)s rule restore <ruleId>`, 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
Expand Down Expand Up @@ -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=<commit> -- .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 (`<commit>~1`). A rule
whose engine is not known is given as a quoted pathspec,
`'.taskless/rules/*/<ruleId>/*'`; 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,
Expand Down
9 changes: 7 additions & 2 deletions packages/cli/src/rules/plan-check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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(
Expand Down
74 changes: 74 additions & 0 deletions packages/cli/src/rules/recovery-advice.ts
Original file line number Diff line number Diff line change
@@ -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 <purpose>", 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=<commit> -- ${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}`) +
".";
}
58 changes: 40 additions & 18 deletions packages/cli/src/rules/verdicts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

/**
Expand Down Expand Up @@ -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<string, unknown>[] {
return Array.isArray(value) ? value.filter((entry) => isRecord(entry)) : [];
}
Expand Down Expand Up @@ -195,7 +200,8 @@ function applyCopy(
rule: ReportedRule,
copy: Extract<CopyOfRead, { status: "copy" }>,
sourceMissing: boolean,
restoreCommand: (ruleId: string) => string
sourceEngine: EngineName | undefined,
recovery: Recovery
): void {
const { ruleId, engine } = rule;
const source = copy.ruleId;
Expand All @@ -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({
Expand Down Expand Up @@ -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);
Expand All @@ -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.
Expand Down Expand Up @@ -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") {
Expand Down Expand Up @@ -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({
Expand All @@ -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;
Expand All @@ -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({
Expand All @@ -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",
}
)}`
);
}

Expand Down
Loading
Loading