From 3a8e132f42a42da70a729a2231f2ea031161379b Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 29 Sep 2026 09:32:26 -0700 Subject: [PATCH 01/10] feat(check): give every run its own run directory, remove it after, and add --preserve-logs --- openspec/changes/cli-v2-rule-api/design.md | 25 ++ openspec/changes/cli-v2-rule-api/proposal.md | 4 + .../cli-v2-rule-api/specs/cli-check/spec.md | 61 +++++ .../specs/cli-rule-reconciliation/spec.md | 5 +- openspec/changes/cli-v2-rule-api/tasks.md | 18 ++ packages/cli/src/agent/check.md | 6 + packages/cli/src/commands/check.ts | 22 +- packages/cli/src/rules/dispatch.ts | 52 +++- packages/cli/src/rules/plan-check.ts | 64 ++++- packages/cli/src/rules/recover.ts | 12 +- packages/cli/src/rules/run-directory.ts | 245 ++++++++++++++++++ packages/cli/src/rules/scan.ts | 14 + packages/cli/src/rules/snapshot.ts | 34 +-- packages/cli/src/rules/vale/run.ts | 17 +- packages/cli/src/schemas/check.ts | 6 + packages/cli/test/check-snapshot.test.ts | 39 ++- packages/cli/test/run-directory.test.ts | 232 +++++++++++++++++ packages/cli/test/runtime-check.test.ts | 22 ++ 18 files changed, 827 insertions(+), 51 deletions(-) create mode 100644 packages/cli/src/rules/run-directory.ts create mode 100644 packages/cli/test/run-directory.test.ts diff --git a/openspec/changes/cli-v2-rule-api/design.md b/openspec/changes/cli-v2-rule-api/design.md index f97299fb..8905b7d9 100644 --- a/openspec/changes/cli-v2-rule-api/design.md +++ b/openspec/changes/cli-v2-rule-api/design.md @@ -115,6 +115,31 @@ vale, which run whatever the verdict for `unknown`. `verify` and `test` keep assembling from the live tree at the existing paths. +### 2a. Every run gets its own run directory, removed when the run ends + +The snapshot started as one shared `.taskless/.run/snapshot/`, replaced at the start of every +run. Two concurrent runs (a pre-commit hook during an editor's on-save `check`) then shared +it: the second deleted and re-copied the tree the first was still reading, so the first could +run a half-copied tree, or a copy whose signatures it never checked. That is the exact failure +the snapshot exists to prevent. + +So each run works in `.taskless/.run//`: `-`, sortable and unique. +It holds the snapshot, the assembled configs, an `owner` record (pid, host, start time), and +four logs: `engine.log` (the plan), `sg.log`, `vale.log`, `runtime.log`. It is removed in a +`finally`, and synchronously on SIGINT/SIGTERM before the signal is re-raised. +`--preserve-logs` (`-l`) keeps the WHOLE directory rather than only the logs, because the logs +name files by their snapshot paths and "what exactly ran" is usually the question +(product decision, 2026-09-29). + +A SIGKILL skips all cleanup, so every run first sweeps directories whose owner process is gone +on this host, plus ownerless ones left by earlier versions (0.11's `runtime-rules/`). Liveness, +not age, is the test (product decision): an age limit either deletes a slow live run or keeps +junk for hours. A directory owned by another host is left alone; that only arises on a shared +filesystem, and this host cannot tell whether the process lives. + +`.taskless/.run/.gitignore` (`*`) is written only if missing, since concurrent runs would race +on it. + ### 3. What is reported for a rule One `{ ruleId, files }` per directory under `.taskless/rules//`, where diff --git a/openspec/changes/cli-v2-rule-api/proposal.md b/openspec/changes/cli-v2-rule-api/proposal.md index 067099a0..ae95c19f 100644 --- a/openspec/changes/cli-v2-rule-api/proposal.md +++ b/openspec/changes/cli-v2-rule-api/proposal.md @@ -122,6 +122,10 @@ Slices, each targeting the one below it: 4. **Recovery**: `rule restore`, `rule rollback`, the refusal. 5. **Retire v1**: delete v1 code and tests, recipes, the end-to-end round trip against production, and the archive. +6. **Run directories**: a per-run `.taskless/.run//` holding the + snapshot and engine logs, removed when the run ends unless + `--preserve-logs`, with abandoned ones swept. Fixes concurrent `check`s + sharing one snapshot. The changeset is `minor` and lives on slice 1. Two reasons, either sufficient: `check` now fails on an edited sg or vale rule, and `rule create --json` renames diff --git a/openspec/changes/cli-v2-rule-api/specs/cli-check/spec.md b/openspec/changes/cli-v2-rule-api/specs/cli-check/spec.md index 0a111490..6d916873 100644 --- a/openspec/changes/cli-v2-rule-api/specs/cli-check/spec.md +++ b/openspec/changes/cli-v2-rule-api/specs/cli-check/spec.md @@ -259,6 +259,67 @@ name the command that repairs the rule, `taskless rule restore `. The on - **THEN** `check` SHALL NOT call any restore or fetch endpoint - **AND** SHALL NOT create the rule's directory +### Requirement: Each check run works in its own run directory + +`taskless check` SHALL do its work (the snapshot, the assembled engine configs, and its logs) +in a directory of its own, `.taskless/.run//`, where `` is unique per run and +sorts by start time. Two runs SHALL never share a run directory. The directory SHALL hold an +`owner` record naming the process and host using it, and the logs `engine.log` (the plan: what +was copied, reported, judged, excluded, and run), `sg.log` and `vale.log` (each engine's +command line, output, and exit code), and `runtime.log` (each runtime rule's duration, +findings, and any error). No log SHALL contain a credential. The CLI SHALL remove the run +directory when the run ends, whether it succeeded or failed, and on SIGINT or SIGTERM, unless +`--preserve-logs` is set. `.taskless/.run/` SHALL ignore itself with its own `.gitignore`, so +that no run rewrites a tracked file. `rule restore` SHALL take its snapshot the same way. + +#### Scenario: Concurrent runs do not disturb each other + +- **WHEN** several `taskless check` runs execute at the same time in one project +- **THEN** each SHALL report the same findings it reports alone +- **AND** none SHALL read another's snapshot + +#### Scenario: Nothing is left behind + +- **WHEN** a `taskless check` run ends, successfully or not, without `--preserve-logs` +- **THEN** its run directory SHALL no longer exist + +### Requirement: Check accepts --preserve-logs to keep its run directory + +`taskless check` SHALL accept `--preserve-logs` (alias `-l`), which keeps the run directory +instead of removing it: the snapshot that ran, the assembled configs, the `owner` record, and +the logs. Human output SHALL name the kept directory on stderr. Under `--json`, the output +SHALL carry an additive, optional `runDirectory` field, the directory's path relative to the +project root, present only when the flag is set. + +#### Scenario: A preserved run is named and complete + +- **WHEN** a user runs `taskless check --json --preserve-logs` +- **THEN** stdout SHALL include `runDirectory` +- **AND** that directory SHALL hold `engine.log`, `sg.log`, `vale.log`, `runtime.log`, `owner`, and the snapshot + +#### Scenario: A preserved authenticated run holds no credential + +- **WHEN** an authenticated `check --preserve-logs` reconciles +- **THEN** no file in the kept run directory SHALL contain the token + +### Requirement: Abandoned run directories are swept + +At the start of every run, the CLI SHALL remove each directory under `.taskless/.run/` whose +`owner` names a process on this host that is no longer alive, and each directory with no +`owner` record (left by an earlier version). It SHALL NOT remove a directory whose owning +process is alive, or one owned by another host, since this host cannot tell whether that +process lives. A directory's age SHALL NOT be the test. + +#### Scenario: A killed run's directory is swept + +- **WHEN** a run directory's `owner` names a process on this host that has exited +- **THEN** the next run SHALL remove it + +#### Scenario: A live run is never swept + +- **WHEN** a run directory's owning process is still running, or it is owned by another host +- **THEN** no other run SHALL remove it + ### Requirement: Check reports rule integrity under --json Under `--json`, `taskless check` SHALL carry an additive, optional `integrity` array with one diff --git a/openspec/changes/cli-v2-rule-api/specs/cli-rule-reconciliation/spec.md b/openspec/changes/cli-v2-rule-api/specs/cli-rule-reconciliation/spec.md index dd2474f7..813e1d6f 100644 --- a/openspec/changes/cli-v2-rule-api/specs/cli-rule-reconciliation/spec.md +++ b/openspec/changes/cli-v2-rule-api/specs/cli-rule-reconciliation/spec.md @@ -230,8 +230,9 @@ outside this check. ### Requirement: The CLI runs the bytes it reported -Before signing anything, `check` SHALL copy `.taskless/rules/` into a snapshot under -`.taskless/.run/`, replacing any previous snapshot and dereferencing symbolic links. +Before signing anything, `check` SHALL copy `.taskless/rules/` into a snapshot inside its own +run directory under `.taskless/.run/` (per the `cli-check` capability), dereferencing symbolic +links. It SHALL compute every reported signature from the snapshot and SHALL run every engine from the snapshot, with the assembled configs written under `.taskless/.run/`. A rule the verdict excludes SHALL be removed from the snapshot before any engine configuration is diff --git a/openspec/changes/cli-v2-rule-api/tasks.md b/openspec/changes/cli-v2-rule-api/tasks.md index 5267edbf..5e2ce268 100644 --- a/openspec/changes/cli-v2-rule-api/tasks.md +++ b/openspec/changes/cli-v2-rule-api/tasks.md @@ -182,6 +182,24 @@ upgradeUrl }`, strip C0/C1 control characters except newline from - [x] 8.4 Run `pnpm typecheck` and `pnpm lint` (which rebuilds and runs `pnpm cli check`) from the repository root; both pass. +## 10. Run directories (slice 6) + +- [x] 10.1 Add `rules/run-directory.ts`: `.taskless/.run//` per run with an + `owner` record, four logs, removal on close and on SIGINT/SIGTERM, a sweep of + directories whose owner is gone (or that have no owner), and a self-ignoring + `.taskless/.run/.gitignore` written only if missing. Unit tests cover unique + ids, close, preserve, sweeping a dead owner, sparing a live or foreign one, + and the legacy ownerless directories. +- [x] 10.2 Move the snapshot into the run directory; `check` and `rule restore` + each open a run and always close it. +- [x] 10.3 Thread the logs through: ast-grep and Vale take an optional `log` + (command line, raw output, exit), dispatch times each runtime rule into + `runtime.log`, and the planner writes the plan to `engine.log`. +- [x] 10.4 Add `check --preserve-logs` / `-l` and the `--json` `runDirectory` + field. End-to-end tests: nothing left behind, four concurrent checks match a + lone check, a preserved run holds the logs and snapshot, and no log from an + authenticated run contains the token. + ## 9. End to end, then archive (slice 5) - [ ] 9.1 From a nightly stamped `0.12.0-*`, against production v2, run the full diff --git a/packages/cli/src/agent/check.md b/packages/cli/src/agent/check.md index 53390992..1367ad00 100644 --- a/packages/cli/src/agent/check.md +++ b/packages/cli/src/agent/check.md @@ -112,6 +112,12 @@ delete the rules to make `check` pass, and do not suggest - `--anonymous`: run only static rules; skip runtime rules. - `--dangerously-run-scripts`: run runtime `check.ts` unverified. - `--timeout `: per-runtime-check wall-clock bound (default 10). +- `--preserve-logs` / `-l`: keep this run's directory under + `.taskless/.run/` (the snapshot that ran, the engine configs, and + `engine.log`, `sg.log`, `vale.log`, `runtime.log`) instead of removing it. + Its path is printed on stderr, or returned as `runDirectory` under + `--json`. Use it to debug a rule that behaves unexpectedly; the logs hold + matched source, so treat the directory like any local build output. ## Steps diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index cb54455e..534c6417 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -16,6 +16,7 @@ import { requireCurrentSchema } from "../filesystem/migrate"; import { discoverRuntimeRules } from "../rules/runtime/discover"; import { resolveRuleSelection, type RuleSelection } from "../rules/rule-filter"; import { planCheck } from "../rules/plan-check"; +import { openRun } from "../rules/run-directory"; import { fromProjectRoot } from "../rules/snapshot"; import { markNotice } from "../util/notices"; @@ -147,6 +148,13 @@ export const checkCommand = defineCommand({ type: "string", description: "Run only the named rule; repeatable (--rule a --rule b)", }, + "preserve-logs": { + type: "boolean", + alias: "l", + description: + "Keep this run's directory under .taskless/.run/ (snapshot, configs, and engine logs) instead of removing it", + default: false, + }, }, async run({ args, rawArgs }) { const cwd = resolve(args.dir ?? process.cwd()); @@ -332,12 +340,17 @@ export const checkCommand = defineCommand({ return; } + // This run's own directory under `.taskless/.run/`: the snapshot, the + // assembled configs, and the logs. Removed when the run ends, however + // it ends, unless `--preserve-logs` keeps it for debugging. + const preserve = Boolean(args["preserve-logs"]); + const run = await openRun(cwd, { preserve }); try { // Planned before dispatch, not during it: planning snapshots the rules // tree and consults auth and reconcile state, which decides WHAT runs. // Everything below reads the snapshot, never `.taskless/rules/`, so // the bytes that run are the bytes that were judged. - const plan = await planCheck(cwd, { + const plan = await planCheck(cwd, run, { anonymous: args.anonymous, dangerouslyRunScripts: Boolean(args["dangerously-run-scripts"]), }); @@ -391,6 +404,7 @@ export const checkCommand = defineCommand({ vale: valeAssembly, runtimeRules: runtimeExecute, runtimeTimeoutMs: parseTimeoutMs(args.timeout), + logs: run.logs, }); const results = dispatched.results; @@ -467,10 +481,14 @@ export const checkCommand = defineCommand({ ? {} : { entitlement: plan.entitlement }), ...(plan.integrity.length > 0 ? { integrity: plan.integrity } : {}), + ...(preserve ? { runDirectory: run.relativePath } : {}), }); console.log(JSON.stringify(output)); } else { console.log(formatText(results)); + if (preserve) { + console.error(`Run directory kept: ${run.relativePath}`); + } } if (exitCode !== 0) { @@ -493,6 +511,8 @@ export const checkCommand = defineCommand({ console.error(message); } process.exitCode = 1; + } finally { + await run.close(); } } finally { // Concrete state event: a scan completed; counts only, no matched code. diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index 7c0d8383..833b3a53 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -4,6 +4,7 @@ import type { RefusedValeConfig, ValeAssembly } from "./assemble"; import { engineRulesDirectory } from "./engines"; import { type EngineName } from "./layout"; import { isMissingDirectory } from "./errno"; +import type { RunLogs } from "./run-directory"; import { executeRuntimeRules } from "./runtime/harness"; import type { RuntimeRule } from "./runtime/discover"; import { runAstGrepScan } from "./scan"; @@ -119,6 +120,12 @@ export interface DispatchOptions { runtimeRules: RuntimeRule[]; runtimeTimeoutMs?: number; valeTimeoutMs?: number; + /** + * The run's logs, when there is a run directory to write them to. Engines + * log through here and never read it back, so a run without logs behaves + * identically. + */ + logs?: RunLogs; } export interface DispatchResult { @@ -164,6 +171,9 @@ async function runAstGrepEngine( return { engine: "sg", results: [], notices: [] }; } const scan = await runAstGrepScan(options.cwd, options.paths, { + ...(options.logs === undefined + ? {} + : { log: (text: string) => options.logs?.sg.write(text) }), configPath: options.astGrepConfigPath, ...(options.astGrepRuleIds === undefined ? {} @@ -221,6 +231,9 @@ async function runValeEngine(options: DispatchOptions): Promise { paths: options.paths, configPath: options.vale.path, timeoutMs: options.valeTimeoutMs, + ...(options.logs === undefined + ? {} + : { log: (text: string) => options.logs?.vale.write(text) }), }); // What the schema said about the configs without refusing them travels with @@ -285,10 +298,32 @@ async function runRuntimeEngine( if (options.runtimeRules.length === 0) { return { engine: "runtime", results: [], notices: [] }; } - const results = await executeRuntimeRules(options.cwd, options.runtimeRules, { - paths: options.paths, - timeoutMs: options.runtimeTimeoutMs, - }); + // One rule at a time, as the harness runs them anyway, so each gets its own + // duration and outcome in `runtime.log`. + const results: CheckResult[] = []; + for (const rule of options.runtimeRules) { + const started = Date.now(); + options.logs?.runtime.write(`${rule.name}: started (${rule.dir})`); + try { + const found = await executeRuntimeRules(options.cwd, [rule], { + paths: options.paths, + timeoutMs: options.runtimeTimeoutMs, + }); + results.push(...found); + options.logs?.runtime.write( + `${rule.name}: ${String(found.length)} finding(s) in ${String(Date.now() - started)}ms` + ); + } catch (error) { + options.logs?.runtime.write( + `${rule.name}: failed after ${String(Date.now() - started)}ms: ${ + error instanceof Error + ? (error.stack ?? error.message) + : String(error) + }` + ); + throw error; + } + } return { engine: "runtime", results, notices: [] }; } @@ -335,6 +370,15 @@ export async function runEngines( }; }); + for (const outcome of outcomes) { + options.logs?.engine.write( + `${outcome.engine}: ${String(outcome.results.length)} finding(s)` + + (outcome.failure === undefined ? "" : `; failed: ${outcome.failure}`) + + (outcome.notices.length === 0 + ? "" + : `; notices: ${outcome.notices.join(" | ")}`) + ); + } const results = outcomes.flatMap((outcome) => outcome.results); const failures = outcomes.flatMap((outcome) => outcome.failure ?? []); diff --git a/packages/cli/src/rules/plan-check.ts b/packages/cli/src/rules/plan-check.ts index a5bdc990..ba26dca8 100644 --- a/packages/cli/src/rules/plan-check.ts +++ b/packages/cli/src/rules/plan-check.ts @@ -4,6 +4,7 @@ import { reconcileRules } from "../api/v2"; import { resolveRepositoryUrl } from "../util/git-remote"; import { getCliPrefix } from "../util/package-manager"; import { reportRules } from "./report"; +import type { RunDirectory } from "./run-directory"; import { RUN_SCRIPTS_WARNING } from "./runtime/harness"; import { discoverRuntimeRulesIn, type RuntimeRule } from "./runtime/discover"; import { @@ -76,9 +77,12 @@ export interface PlanOptions { */ export async function planCheck( cwd: string, + run: RunDirectory, options: PlanOptions ): Promise { - const snapshot = await takeSnapshot(cwd); + const log = run.logs.engine; + const snapshot = await takeSnapshot(cwd, run); + log.write(`snapshot taken at ${snapshot.base}`); const runtimeRoot = snapshotEngineDirectory(snapshot, "runtime"); const discovered = await discoverRuntimeRulesIn(runtimeRoot); const empty = { @@ -91,14 +95,20 @@ export async function planCheck( }; if (options.dangerouslyRunScripts) { + log.write( + "--dangerously-run-scripts: no reconcile; every rule runs unverified" + ); return { ...empty, execute: discovered, notices: [RUN_SCRIPTS_WARNING] }; } - const unverified = (reason: string, notice?: string): CheckPlan => ({ - ...empty, - skipped: discovered.map((rule) => ({ rule: rule.name, reason })), - notices: notice === undefined ? [] : [notice], - }); + const unverified = (reason: string, notice?: string): CheckPlan => { + log.write(`unverified run: ${reason}`); + return { + ...empty, + skipped: discovered.map((rule) => ({ rule: rule.name, reason })), + notices: notice === undefined ? [] : [notice], + }; + }; if (options.anonymous) { return unverified( @@ -121,6 +131,23 @@ export async function planCheck( } const report = await reportRules(snapshot); + log.write( + `reporting ${String(report.rules.length)} rule(s): ` + + report.rules + .map( + (rule) => + `${rule.engine}/${rule.ruleId} (${String(rule.files.length)} files)` + ) + .join(", ") + ); + for (const duplicate of report.duplicates) { + log.write( + `duplicate id ${duplicate.ruleId} across ${duplicate.engines.join(", ")}` + ); + } + for (const rule of report.unreadable) { + log.write(`unreadable ${rule.engine}/${rule.ruleId}: ${rule.reason}`); + } const failures: string[] = []; const integrity: IntegrityEntry[] = []; @@ -171,6 +198,17 @@ export async function planCheck( rules: report.rules.map(({ ruleId, files }) => ({ ruleId, files })), }); + log.write( + outcome.status === "ok" + ? "reconcile answered" + : `reconcile did not answer: ${outcome.status}${ + outcome.status === "error" + ? ` (${outcome.code})` + : outcome.status === "unavailable" + ? ` (${outcome.reason})` + : "" + }` + ); if (outcome.status !== "ok") { const cause = outcome.status === "unauthorized" @@ -203,6 +241,13 @@ export async function planCheck( (ruleId) => `${getCliPrefix()} rule restore ${ruleId}` ); for (const disposition of verdicts.dispositions) { + log.write( + `${disposition.engine}/${disposition.ruleId}: ${ + disposition.run + ? "runs" + : `excluded (${disposition.reason ?? "not verified"})` + }` + ); if (!disposition.run) { await excludeFromSnapshot( snapshot, @@ -211,6 +256,13 @@ export async function planCheck( ); } } + for (const entry of verdicts.integrity) { + if (entry.verdict === "missing") { + log.write( + `missing: ${entry.ruleId} (revision ${entry.revisionId ?? "unknown"})` + ); + } + } const execute = await discoverRuntimeRulesIn(runtimeRoot); const executing = new Set(execute.map((rule) => rule.name)); diff --git a/packages/cli/src/rules/recover.ts b/packages/cli/src/rules/recover.ts index 6de846b2..753b9bcb 100644 --- a/packages/cli/src/rules/recover.ts +++ b/packages/cli/src/rules/recover.ts @@ -16,6 +16,7 @@ import { writeServedRule } from "./files"; import { orgNotFoundMessage } from "./generate"; import { isKnownEngine, type EngineName } from "./layout"; import { reportRules } from "./report"; +import { openRun } from "./run-directory"; import { takeSnapshot } from "./snapshot"; import { verifyServedRule } from "./verify-delivery"; @@ -130,8 +131,15 @@ export async function beginRestore( identity: Identity, ruleId: string ): Promise { - const snapshot = await takeSnapshot(cwd); - const report = await reportRules(snapshot); + // Its own run directory, for the snapshot only: restore needs the report, + // never the copy, so the directory goes as soon as the report is taken. + const run = await openRun(cwd); + let report; + try { + report = await reportRules(await takeSnapshot(cwd, run)); + } finally { + await run.close(); + } const duplicate = report.duplicates.find((entry) => entry.ruleId === ruleId); if (duplicate !== undefined) { throw new CLIError( diff --git a/packages/cli/src/rules/run-directory.ts b/packages/cli/src/rules/run-directory.ts new file mode 100644 index 00000000..b1bb97d1 --- /dev/null +++ b/packages/cli/src/rules/run-directory.ts @@ -0,0 +1,245 @@ +import { randomBytes } from "node:crypto"; +import { rmSync } from "node:fs"; +import { + appendFile, + mkdir, + readdir, + readFile, + rm, + writeFile, +} from "node:fs/promises"; +import { hostname } from "node:os"; +import { join, relative } from "node:path"; +import process from "node:process"; + +/** + * The transient verification space one `check` (or `rule restore`) works in: + * `.taskless/.run//`. + * + * **One directory per run.** A single shared snapshot let a second `check` + * delete and re-copy the tree a first one was still reading, so the first + * could run a half-copied tree, or a copy whose signatures it never checked, + * which is exactly what the snapshot exists to prevent. A pre-commit hook + * firing during an editor's on-save `check` is enough. + * + * **Always removed**, on success, on failure, and on SIGINT/SIGTERM (best + * effort), unless `--preserve-logs` asks to keep it. What it holds is the copy + * that ran and the logs of running it, which is what debugging a run needs and + * nothing a later run reads. + * + * **Abandoned directories are swept** at the start of every run: a run killed + * with SIGKILL never gets to clean up. A directory is abandoned when the + * process named in its `owner` file is gone on this host. Age is not the test, + * because it would either delete a slow run still in progress or keep junk for + * hours. A directory owned by another host (a shared filesystem) is left + * alone, since this host cannot tell whether that process lives. A directory + * with no `owner` file predates run ids (0.11's `runtime-rules/`, the first + * 0.12 `snapshot/`) and is swept too. + */ + +/** `.taskless/.run`, relative to the project root. */ +const RUN_ROOT = join(".taskless", ".run"); + +/** Who is using a run directory, so a later run can tell if it was abandoned. */ +interface Owner { + pid: number; + hostname: string; + startedAt: string; +} + +/** One append-only log file in a run directory. */ +export class RunLog { + private pending: Promise = Promise.resolve(); + + constructor(readonly path: string) {} + + /** Append one timestamped entry. Never throws: a log must not fail a run. */ + write(text: string): void { + const entry = `[${new Date().toISOString()}] ${text}\n`; + this.pending = this.pending + .then(() => appendFile(this.path, entry, "utf8")) + .catch(() => {}); + } + + /** Wait for every entry written so far to reach the disk. */ + flush(): Promise { + return this.pending; + } +} + +/** The logs of one run. */ +export interface RunLogs { + /** The plan: what was copied, reported, judged, excluded, and run. */ + engine: RunLog; + /** ast-grep's command line, output, and exit code. */ + sg: RunLog; + /** Vale's command line, output, and exit code, per attempt. */ + vale: RunLog; + /** Per runtime rule: duration, findings, and any error. */ + runtime: RunLog; +} + +export interface RunDirectory { + /** `-`: sortable by start time, unique across runs. */ + id: string; + /** Absolute path of `.taskless/.run//`. */ + path: string; + /** The path relative to the project root, for printing. */ + relativePath: string; + logs: RunLogs; + /** + * Remove the directory, unless it is being preserved. Idempotent. Always + * call it, in a `finally`. + */ + close(): Promise; +} + +export interface OpenRunOptions { + /** Keep the directory after the run (`--preserve-logs`). */ + preserve?: boolean; +} + +function newRunId(now: Date): string { + const stamp = now.toISOString().replaceAll(/[-:]/g, "").replace(/\.\d+/, ""); + return `${stamp}-${randomBytes(3).toString("hex")}`; +} + +/** Whether `pid` names a live process on this host. */ +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + // EPERM: it exists, it just is not ours to signal. + return (error as NodeJS.ErrnoException).code === "EPERM"; + } +} + +async function readOwner(directory: string): Promise { + try { + const parsed = JSON.parse( + await readFile(join(directory, "owner"), "utf8") + ) as Partial; + return typeof parsed.pid === "number" && typeof parsed.hostname === "string" + ? (parsed as Owner) + : undefined; + } catch { + return undefined; + } +} + +/** + * Remove every run directory whose owner is gone. Returns what was removed, + * for the engine log. Never throws: a sweep that cannot finish leaves junk, + * which is not a reason to fail a run. + */ +export async function sweepAbandonedRuns(cwd: string): Promise { + const root = join(cwd, RUN_ROOT); + let entries; + try { + entries = await readdir(root, { withFileTypes: true }); + } catch { + return []; + } + const removed: string[] = []; + for (const entry of entries) { + if (!entry.isDirectory()) continue; + const directory = join(root, entry.name); + const owner = await readOwner(directory); + if (owner !== undefined) { + if (owner.hostname !== hostname()) continue; + if (isAlive(owner.pid)) continue; + } + try { + await rm(directory, { recursive: true, force: true }); + removed.push(entry.name); + } catch { + // Left for the next sweep. + } + } + return removed; +} + +/** + * Create this run's directory, after sweeping abandoned ones. + */ +export async function openRun( + cwd: string, + options: OpenRunOptions = {} +): Promise { + const swept = await sweepAbandonedRuns(cwd); + + const root = join(cwd, RUN_ROOT); + await mkdir(root, { recursive: true }); + // The run root ignores itself, so `check` never rewrites a tracked file. + // Written only when missing: concurrent runs would otherwise race on it. + await writeFile(join(root, ".gitignore"), "*\n", { flag: "wx" }).catch( + () => {} + ); + + const now = new Date(); + const id = newRunId(now); + const path = join(root, id); + await mkdir(path, { recursive: true }); + const owner: Owner = { + pid: process.pid, + hostname: hostname(), + startedAt: now.toISOString(), + }; + await writeFile(join(path, "owner"), `${JSON.stringify(owner)}\n`, "utf8"); + + const logs: RunLogs = { + engine: new RunLog(join(path, "engine.log")), + sg: new RunLog(join(path, "sg.log")), + vale: new RunLog(join(path, "vale.log")), + runtime: new RunLog(join(path, "runtime.log")), + }; + logs.engine.write(`run ${id} started (pid ${String(process.pid)})`); + if (swept.length > 0) { + logs.engine.write(`swept abandoned run directories: ${swept.join(", ")}`); + } + + const preserve = options.preserve === true; + let closed = false; + + // A signal skips `finally`, so the directory would outlive the run. Removed + // synchronously, then the signal re-raised so the process still exits the + // way the signal asked. + const onSignal = (signal: NodeJS.Signals): void => { + if (!closed && !preserve) { + try { + rmSync(path, { recursive: true, force: true }); + } catch { + // The next run's sweep removes it. + } + } + closed = true; + process.off("SIGINT", onSignal); + process.off("SIGTERM", onSignal); + process.kill(process.pid, signal); + }; + process.once("SIGINT", onSignal); + process.once("SIGTERM", onSignal); + + return { + id, + path, + relativePath: relative(cwd, path), + logs, + async close() { + if (closed) return; + closed = true; + process.off("SIGINT", onSignal); + process.off("SIGTERM", onSignal); + logs.engine.write( + preserve ? "run finished; directory preserved" : "run finished" + ); + await Promise.all( + [logs.engine, logs.sg, logs.vale, logs.runtime].map((log) => + log.flush() + ) + ); + if (!preserve) await rm(path, { recursive: true, force: true }); + }, + }; +} diff --git a/packages/cli/src/rules/scan.ts b/packages/cli/src/rules/scan.ts index 6f746f42..5486555b 100644 --- a/packages/cli/src/rules/scan.ts +++ b/packages/cli/src/rules/scan.ts @@ -125,6 +125,12 @@ export function findSgBinary(): string { } export interface ScanOptions { + /** + * Receives the command line, every raw stdout line, stderr, and the exit + * code, for the run's `sg.log`. Optional: nothing about the scan depends on + * whether anyone is listening. + */ + log?: (text: string) => void; /** * ast-grep config to scan with, relative to `cwd`. Defaults to the committed * `sg` engine config — the source of truth for the ast-grep engine, read @@ -281,6 +287,7 @@ export async function runAstGrepScan( ...sgWalkArgv(paths), ...(paths.length > 0 ? ["--", ...paths] : []), ]; + options.log?.(`$ ${sgBinary} ${argv.join(" ")} (cwd ${cwd})`); const child = spawn(sgBinary, argv, { cwd, stdio: ["ignore", "pipe", "pipe"], @@ -304,6 +311,7 @@ export async function runAstGrepScan( const rl = createInterface({ input: child.stdout }); rl.on("line", (line) => { + options.log?.(`stdout: ${line}`); const trimmed = line.trim(); if (trimmed === "") return; try { @@ -338,6 +346,12 @@ export async function runAstGrepScan( // stream that ends mid-character contributes its replacement char once // rather than leaving bytes unaccounted for. stderrChunks.push(stderrDecoder.end()); + const stderrText = stderrChunks.join(""); + if (stderrText.trim() !== "") + options.log?.(`stderr: ${stderrText.trim()}`); + options.log?.( + `exit ${String(code)}; ${String(results.length)} finding(s)` + ); // ast-grep exits 1 when error-severity matches found — that's expected // Only treat spawn/binary failures (exit > 1) as errors diff --git a/packages/cli/src/rules/snapshot.ts b/packages/cli/src/rules/snapshot.ts index d4d64a7d..79634e81 100644 --- a/packages/cli/src/rules/snapshot.ts +++ b/packages/cli/src/rules/snapshot.ts @@ -1,15 +1,8 @@ -import { - copyFile, - mkdir, - readdir, - realpath, - rm, - stat, - writeFile, -} from "node:fs/promises"; +import { copyFile, mkdir, readdir, realpath, rm, stat } from "node:fs/promises"; import { join, relative } from "node:path"; import { isMissingDirectory } from "./errno"; +import type { RunDirectory } from "./run-directory"; import { engineRulesDirectory, ruleDirectory, rulesRoot } from "./engines"; import type { EngineName } from "./layout"; @@ -22,8 +15,11 @@ import type { EngineName } from "./layout"; * side. Taking the copy first and never reading the live tree again closes both: * whatever the verdict describes is exactly what runs. * + * It lives inside the run's own directory (see `run-directory.ts`), so + * concurrent runs never share one and it goes when the run does. + * * **The snapshot mirrors the project's layout** under a base directory: - * `.taskless/.run/snapshot/.taskless/rules/`. Every path helper in this package + * `.taskless/.run//snapshot/.taskless/rules/`. Every path helper in this package * takes a project root and appends `.taskless/rules/...`, and both config * assemblers write root-relative paths (`StylesPath`, `ruleDirs`), so handing * them the base instead of the project root points all of it at the snapshot @@ -32,9 +28,6 @@ import type { EngineName } from "./layout"; * Vale rule scoped to a subdirectory glob. */ -/** The snapshot base, relative to `.taskless/`. Gitignored with the rest of `.run/`. */ -const SNAPSHOT_DIRECTORY = join(".run", "snapshot"); - /** * Operating-system metadata that is neither copied nor reported. * @@ -62,7 +55,7 @@ export interface Snapshot { } /** - * Replace the snapshot with a fresh copy of `.taskless/rules/`. + * Copy `.taskless/rules/` into the run's directory. * * Symbolic links are DEREFERENCED: what is signed has to be what runs, and a * link resolved at run time is bytes nobody signed. A link that does not @@ -73,15 +66,12 @@ export interface Snapshot { * NOT a cycle, and each is copied in full: skipping the second would drop a * rule's files from the snapshot without any verdict saying so. */ -export async function takeSnapshot(cwd: string): Promise { - const base = join(cwd, ".taskless", SNAPSHOT_DIRECTORY); - await rm(base, { recursive: true, force: true }); +export async function takeSnapshot( + cwd: string, + run: RunDirectory +): Promise { + const base = join(run.path, "snapshot"); await mkdir(join(base, ".taskless"), { recursive: true }); - // The run directory ignores ITSELF. Adding `.run/` to `.taskless/.gitignore` - // instead would make every `check` rewrite a tracked file, and `check` - // writes nothing under `.taskless/` outside `.taskless/.run/`. git, and the - // ignore walkers ast-grep and Vale use, all honor a nested `.gitignore`. - await writeFile(join(cwd, ".taskless", ".run", ".gitignore"), "*\n"); const source = rulesRoot(cwd); const target = rulesRoot(base); diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index a2131846..8a8c5557 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -309,9 +309,11 @@ async function spawnVale( argv: string[], cwd: string, timeoutMs: number, - skipped: string | undefined + skipped: string | undefined, + log?: (text: string) => void ): Promise { return new Promise((settlePromise) => { + log?.(`$ ${binary} ${argv.join(" ")} (cwd ${cwd})`); const child = spawn(binary, argv, { cwd, stdio: ["ignore", "pipe", "pipe"], @@ -336,6 +338,9 @@ async function spawnVale( const settle = (outcome: ValeAttempt): void => { if (settled) return; settled = true; + log?.( + `attempt ${outcome.status}${"message" in outcome ? `: ${outcome.message}` : ""}` + ); clearTimeout(timer); settlePromise(outcome); }; @@ -377,6 +382,11 @@ async function spawnVale( // rather than leaving bytes unaccounted for. stdoutChunks.push(stdoutDecoder.end()); stderrChunks.push(stderrDecoder.end()); + log?.(`exit ${String(code)}`); + const rawStdout = stdoutChunks.join("").trim(); + const rawStderr = stderrChunks.join("").trim(); + if (rawStdout !== "") log?.(`stdout: ${rawStdout}`); + if (rawStderr !== "") log?.(`stderr: ${rawStderr}`); // With --no-exit, a non-zero code is Vale failing, not Vale finding. if (code !== null && code !== 0) { @@ -463,6 +473,8 @@ export interface ValeRunOptions { /** Config path relative to `cwd`. Defaults to the assembled run config. */ configPath?: string; timeoutMs?: number; + /** Receives each attempt's command line, output, and exit, for `vale.log`. */ + log?: (text: string) => void; } /** @@ -647,7 +659,8 @@ export async function runVale( argv, options.cwd, timeoutMs, - skipped + skipped, + options.log ); if (attempt.status === "ok") { diff --git a/packages/cli/src/schemas/check.ts b/packages/cli/src/schemas/check.ts index 681a0800..000aa841 100644 --- a/packages/cli/src/schemas/check.ts +++ b/packages/cli/src/schemas/check.ts @@ -77,6 +77,12 @@ export const outputSchema = z.object({ // a runtime rule the service never issued, unaccounted for, or an id shared // across engines. Locally written ast-grep and Vale rules are `unknown` too // and are deliberately NOT listed: they run, and every run would repeat them. + runDirectory: z + .string() + .optional() + .describe( + "Present only with --preserve-logs: the kept run directory, relative to the project root. It holds the snapshot that ran, the assembled engine configs, and engine.log, sg.log, vale.log, and runtime.log" + ), integrity: z .array( z.object({ diff --git a/packages/cli/test/check-snapshot.test.ts b/packages/cli/test/check-snapshot.test.ts index 34ed3cf4..019b6a6c 100644 --- a/packages/cli/test/check-snapshot.test.ts +++ b/packages/cli/test/check-snapshot.test.ts @@ -16,6 +16,7 @@ import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { assembleEngineConfigs } from "../src/rules/assemble"; import { runEngines } from "../src/rules/dispatch"; import { reportRules } from "../src/rules/report"; +import { openRun, type RunDirectory } from "../src/rules/run-directory"; import type { CheckResult } from "../src/types/check"; import { canonicalHash } from "../src/rules/rule-hash"; import { @@ -24,6 +25,20 @@ import { takeSnapshot, } from "../src/rules/snapshot"; +/** Runs opened by a test, closed after it so no signal handler outlives it. */ +const runs: RunDirectory[] = []; + +/** Take a snapshot inside a fresh run directory, as `check` does. */ +async function snap(cwd: string) { + const run = await openRun(cwd); + runs.push(run); + return takeSnapshot(cwd, run); +} + +afterEach(async () => { + for (const run of runs.splice(0)) await run.close(); +}); + describe("the check snapshot", () => { let cwd: string; const rules = () => join(cwd, ".taskless", "rules"); @@ -48,8 +63,8 @@ describe("the check snapshot", () => { }); it("mirrors the project layout under the base, and ignores itself", async () => { - const snapshot = await takeSnapshot(cwd); - expect(snapshot.base).toBe(join(cwd, ".taskless", ".run", "snapshot")); + const snapshot = await snap(cwd); + expect(snapshot.base).toBe(join(runs.at(-1)!.path, "snapshot")); expect( existsSync( join( @@ -68,12 +83,12 @@ describe("the check snapshot", () => { ).toBe("*\n"); expect(existsSync(join(cwd, ".taskless", ".gitignore"))).toBe(false); expect(fromProjectRoot(snapshot, ".taskless/.vale.ini")).toBe( - join(".taskless", ".run", "snapshot", ".taskless", ".vale.ini") + join(runs.at(-1)!.relativePath, "snapshot", ".taskless", ".vale.ini") ); }); it("is what gets signed: an edit after the snapshot does not reach the report", async () => { - const snapshot = await takeSnapshot(cwd); + const snapshot = await snap(cwd); await writeFile( join(rules(), "sg", "no-eval-3fa9c21b", "no-eval-3fa9c21b.yml"), "id: edited\n" @@ -95,7 +110,7 @@ describe("the check snapshot", () => { outside, join(rules(), "sg", "no-eval-3fa9c21b", "no-eval-3fa9c21b.yml") ); - const snapshot = await takeSnapshot(cwd); + const snapshot = await snap(cwd); await writeFile(outside, "id: changed after the snapshot\n"); const report = await reportRules(snapshot); expect(report.rules[0]?.files[0]?.signature).toBe( @@ -108,7 +123,7 @@ describe("the check snapshot", () => { join(cwd, "nowhere.yml"), join(rules(), "sg", "no-eval-3fa9c21b", "dangling.yml") ); - const report = await reportRules(await takeSnapshot(cwd)); + const report = await reportRules(await snap(cwd)); expect(report.rules[0]?.files.map((file) => file.path)).toEqual([ "no-eval-3fa9c21b.yml", ]); @@ -141,7 +156,7 @@ describe("the check snapshot", () => { it("neither copies nor reports operating-system metadata", async () => { await writeFile(join(rules(), "sg", "no-eval-3fa9c21b", ".DS_Store"), "x"); - const snapshot = await takeSnapshot(cwd); + const snapshot = await snap(cwd); const report = await reportRules(snapshot); expect(report.rules[0]?.files.map((file) => file.path)).toEqual([ "no-eval-3fa9c21b.yml", @@ -164,7 +179,7 @@ describe("the check snapshot", () => { const nested = join(rules(), "sg", "no-eval-3fa9c21b", "extra", ".tests"); await mkdir(nested, { recursive: true }); await writeFile(join(nested, "a.yml"), "id: a\n"); - const report = await reportRules(await takeSnapshot(cwd)); + const report = await reportRules(await snap(cwd)); expect(report.rules[0]?.files.map((file) => file.path)).toEqual([ "extra/.tests/a.yml", "no-eval-3fa9c21b.yml", @@ -177,7 +192,7 @@ describe("the check snapshot", () => { join(rules(), "vale", "no-eval-3fa9c21b", "no-eval-3fa9c21b.yml"), "extends: existence\n" ); - const report = await reportRules(await takeSnapshot(cwd)); + const report = await reportRules(await snap(cwd)); expect(report.rules).toEqual([]); expect(report.duplicates).toEqual([ { ruleId: "no-eval-3fa9c21b", engines: ["sg", "vale"] }, @@ -185,7 +200,7 @@ describe("the check snapshot", () => { }); it("excluding a rule removes it from the snapshot only", async () => { - const snapshot = await takeSnapshot(cwd); + const snapshot = await snap(cwd); await excludeFromSnapshot(snapshot, "sg", "no-eval-3fa9c21b"); const { rules: reported } = await reportRules(snapshot); expect(reported).toEqual([]); @@ -194,7 +209,7 @@ describe("the check snapshot", () => { it("an empty project snapshots to no rules", async () => { await rm(rules(), { recursive: true }); - const report = await reportRules(await takeSnapshot(cwd)); + const report = await reportRules(await snap(cwd)); expect(report).toEqual({ rules: [], duplicates: [], unreadable: [] }); }); }); @@ -256,7 +271,7 @@ describe("engines read the snapshot exactly as they read the live tree", () => { runtimeRules: [], }); - const snapshot = await takeSnapshot(cwd); + const snapshot = await snap(cwd); const assembled = await assembleEngineConfigs(snapshot.base); const fromSnapshot = await runEngines({ cwd, diff --git a/packages/cli/test/run-directory.test.ts b/packages/cli/test/run-directory.test.ts new file mode 100644 index 00000000..f56e7d96 --- /dev/null +++ b/packages/cli/test/run-directory.test.ts @@ -0,0 +1,232 @@ +import { execFile, spawnSync } from "node:child_process"; +import { existsSync } from "node:fs"; +import { + cp, + mkdir, + mkdtemp, + readdir, + readFile, + rm, + writeFile, +} from "node:fs/promises"; +import { hostname, tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { promisify } from "node:util"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { openRun, sweepAbandonedRuns } from "../src/rules/run-directory"; +import { migrateFixture } from "./support/current-project"; + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); + +/** A pid that certainly belonged to a process that has exited. */ +function deadPid(): number { + const child = spawnSync(process.execPath, ["-e", "0"]); + return child.pid ?? 0; +} + +async function runDirectories(cwd: string): Promise { + try { + const entries = await readdir(join(cwd, ".taskless", ".run"), { + withFileTypes: true, + }); + return entries + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name); + } catch { + return []; + } +} + +describe("run directories", () => { + let cwd: string; + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "tskl-run-dir-")); + await mkdir(join(cwd, ".taskless"), { recursive: true }); + }); + + afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); + }); + + it("gives every run its own directory, owned by this process", async () => { + const [a, b] = await Promise.all([openRun(cwd), openRun(cwd)]); + try { + expect(a.id).not.toBe(b.id); + expect(a.id).toMatch(/^\d{8}T\d{6}Z-[0-9a-f]{6}$/); + const owner = JSON.parse( + await readFile(join(a.path, "owner"), "utf8") + ) as { pid: number; hostname: string }; + expect(owner).toMatchObject({ pid: process.pid, hostname: hostname() }); + expect( + await readFile(join(cwd, ".taskless", ".run", ".gitignore"), "utf8") + ).toBe("*\n"); + } finally { + await a.close(); + await b.close(); + } + }); + + it("removes the directory on close", async () => { + const run = await openRun(cwd); + run.logs.engine.write("hello"); + await run.close(); + expect(existsSync(run.path)).toBe(false); + await run.close(); // idempotent + }); + + it("keeps the directory, logs flushed, when preserved", async () => { + const run = await openRun(cwd, { preserve: true }); + run.logs.sg.write("$ sg scan"); + run.logs.vale.write("$ vale"); + run.logs.runtime.write("rule: 0 finding(s)"); + await run.close(); + expect(await readFile(join(run.path, "sg.log"), "utf8")).toContain( + "$ sg scan" + ); + expect(await readFile(join(run.path, "engine.log"), "utf8")).toContain( + "directory preserved" + ); + }); + + it("sweeps a directory whose owner process is gone", async () => { + const stale = join(cwd, ".taskless", ".run", "20200101T000000Z-aaaaaa"); + await mkdir(stale, { recursive: true }); + await writeFile( + join(stale, "owner"), + JSON.stringify({ pid: deadPid(), hostname: hostname(), startedAt: "x" }) + ); + expect(await sweepAbandonedRuns(cwd)).toEqual(["20200101T000000Z-aaaaaa"]); + expect(existsSync(stale)).toBe(false); + }); + + it("never sweeps a live run, or one owned by another host", async () => { + const live = await openRun(cwd); + const foreign = join(cwd, ".taskless", ".run", "20200101T000000Z-bbbbbb"); + await mkdir(foreign, { recursive: true }); + await writeFile( + join(foreign, "owner"), + JSON.stringify({ + pid: deadPid(), + hostname: "some-other-host", + startedAt: "x", + }) + ); + try { + expect(await sweepAbandonedRuns(cwd)).toEqual([]); + expect(existsSync(live.path)).toBe(true); + expect(existsSync(foreign)).toBe(true); + } finally { + await live.close(); + } + }); + + it("sweeps the ownerless directories earlier versions left", async () => { + for (const legacy of ["runtime-rules", "snapshot"]) { + await mkdir(join(cwd, ".taskless", ".run", legacy, "x"), { + recursive: true, + }); + } + const run = await openRun(cwd); + await run.close(); + expect(await runDirectories(cwd)).toEqual([]); + }); + + it("does not rewrite a .gitignore that is already there", async () => { + await mkdir(join(cwd, ".taskless", ".run"), { recursive: true }); + await writeFile( + join(cwd, ".taskless", ".run", ".gitignore"), + "*\n# kept\n" + ); + const run = await openRun(cwd); + await run.close(); + expect( + await readFile(join(cwd, ".taskless", ".run", ".gitignore"), "utf8") + ).toBe("*\n# kept\n"); + }); +}); + +describe("check and its run directory", () => { + const fixture = join( + import.meta.dirname, + "fixtures", + "mixed-engines-project" + ); + let cwd: string; + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "tskl-run-check-")); + await cp(fixture, cwd, { recursive: true }); + await execFileAsync("git", ["init", "-q"], { cwd }); + await migrateFixture(["-d", cwd]); + }); + + afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); + }); + + async function check(...extra: string[]) { + const { stdout } = await execFileAsync( + "node", + [binPath, "check", "-d", cwd, "--json", ...extra], + { env: { ...process.env, TASKLESS_TOKEN: "" } } + ).catch((error: { stdout: string }) => ({ stdout: error.stdout })); + const line = stdout + .trim() + .split("\n") + .findLast((l) => l.startsWith("{")); + const output = JSON.parse(line ?? "{}") as { + results: { ruleId: string; file: string }[]; + runDirectory?: string; + }; + // An error envelope has no `results`; a test must not pass on one. + expect(output.results, stdout).toBeDefined(); + return output; + } + + it("leaves nothing behind", async () => { + await check(); + expect(await runDirectories(cwd)).toEqual([]); + }); + + it("concurrent checks do not disturb each other", async () => { + const alone = await check(); + const key = (output: typeof alone) => + output.results.map((r) => `${r.file}:${r.ruleId}`).toSorted(); + const together = await Promise.all([check(), check(), check(), check()]); + for (const output of together) { + expect(key(output)).toEqual(key(alone)); + } + expect(key(alone).length).toBeGreaterThan(0); + expect(await runDirectories(cwd)).toEqual([]); + }); + + it("--preserve-logs keeps the run directory, snapshot, and logs, and names it", async () => { + const output = await check("-l"); + expect(output.runDirectory).toMatch( + /^\.taskless\/\.run\/\d{8}T\d{6}Z-[0-9a-f]{6}$/ + ); + const directory = join(cwd, output.runDirectory ?? ""); + const files = await readdir(directory); + for (const name of [ + "engine.log", + "sg.log", + "vale.log", + "owner", + "snapshot", + ]) { + expect(files).toContain(name); + } + const sgLog = await readFile(join(directory, "sg.log"), "utf8"); + expect(sgLog).toContain("scan --config"); + expect(sgLog).toMatch(/exit \d/); + expect(await readFile(join(directory, "engine.log"), "utf8")).toContain( + "unverified run" + ); + // The next run sweeps it only once its owner is gone, which it is. + await check(); + expect(await runDirectories(cwd)).toEqual([]); + }); +}); diff --git a/packages/cli/test/runtime-check.test.ts b/packages/cli/test/runtime-check.test.ts index 18f9a0a0..8882dd97 100644 --- a/packages/cli/test/runtime-check.test.ts +++ b/packages/cli/test/runtime-check.test.ts @@ -671,4 +671,26 @@ describe("check: static vs runtime dispatch", () => { expect(exitCode).toBe(0); expect(parseJson(stdout)).not.toHaveProperty("entitlement"); }); + + it("--preserve-logs keeps an authenticated run's logs, and no log holds the token", async () => { + const { stdout } = await authedCheck( + (request) => ({ + statusCode: 200, + body: answer(request, { "no-console": "run", demo: "run" }), + }), + ["--json", "--preserve-logs"] + ); + const output = parseJson(stdout) as CheckJson & { runDirectory?: string }; + expect(output.runDirectory).toBeDefined(); + const kept = join(directory, output.runDirectory ?? ""); + const logs = await Promise.all( + ["engine.log", "sg.log", "runtime.log"].map((name) => + readFile(join(kept, name), "utf8") + ) + ); + expect(logs[0]).toContain("reconcile answered"); + expect(logs[0]).toContain("runtime/demo: runs"); + expect(logs[2]).toMatch(/demo: \d+ finding\(s\) in \d+ms/); + for (const log of logs) expect(log).not.toContain("fake.token"); + }); }); From 9649a70836edb9bdc6f1b171768837c874003686 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 29 Sep 2026 10:43:01 -0700 Subject: [PATCH 02/10] build: add build:next, which stamps the version the pending changesets will release --- CLAUDE.md | 2 ++ package.json | 1 + 2 files changed, 3 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 1a8cf4f7..eec8a944 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -21,6 +21,8 @@ This is not hypothetical. An agent followed `pnpm cli agent create-sg-rule` from **The installed Taskless skill pins a published nightly**, recorded as `install.cliVersion` in `.taskless/taskless.json`, and every command in `.taskless/skills/taskless/SKILL.md` carries that pin. The pin and `pnpm cli` disagree exactly when `dist/` is behind HEAD, and neither is automatically right: the pin is a real build of some commit, `pnpm cli` is this tree only after you rebuild it. Rebuild, then prefer `pnpm cli`: it is the only one that can reflect uncommitted work. Note that the nightly package is blocked by a deny rule here, so `pnpm build` is the practical way to get current recipes, not a fallback. +**Before `pnpm cli` talks to the Taskless service, build with `pnpm build:next`, not `pnpm build`.** The service and the rule generator decide what a client may do from its `x-taskless-cli-version` header: which API it may call, and whether it may be sent runtime rules. A plain build reports the last RELEASED version from `package.json`, so a tree carrying unreleased API work presents itself as the old client and trips the "unsupported" and upgrade gates meant for one. `build:next` stamps the version the pending changesets will release, as `-next-` (for example `0.12.0-next-fa9ae7c`), which the service reads as that release. It builds the same nightly target CI publishes, so the only difference from a real nightly is the suffix. `pnpm lint` runs a plain `pnpm build`, so rebuild with `build:next` after linting if you are about to call the service. + When running OpenSpec commands in this repo, use `pnpm openspec` instead of a bare `openspec`. The bare command is not on `PATH` here and is blocked by a deny rule. ## Git Command Help for Agents diff --git a/package.json b/package.json index 82620f42..33df544a 100644 --- a/package.json +++ b/package.json @@ -7,6 +7,7 @@ "scripts": { "build": "pnpm build:compile", "build:compile": "turbo run build", + "build:next": "TASKLESS_NIGHTLY_VERSION=$(tsx scripts/next-version.ts)-next-$(git rev-parse --short=7 HEAD) pnpm --filter @taskless/cli build:nightly", "build:nightly": "pnpm --filter @taskless/cli build:nightly", "build:self": "run-s build:self:compile build:self:install", "build:self:compile": "TASKLESS_SELF_BASE_VERSION=$(tsx scripts/next-version.ts) pnpm --filter @taskless/cli build:self", From 0d97caf7f95fcd48e01d45ad3f8dac37f670b0ec Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 29 Sep 2026 11:02:58 -0700 Subject: [PATCH 03/10] fix(check): keep Vale out of other runs' directories, and record each verdict in engine.log --- openspec/changes/cli-v2-rule-api/design.md | 13 ++++++++ openspec/changes/cli-v2-rule-api/tasks.md | 9 ++++++ packages/cli/src/rules/plan-check.ts | 2 +- packages/cli/src/rules/recover.ts | 5 +++- packages/cli/src/rules/run-directory.ts | 35 ++++++++++++++++++++-- packages/cli/src/rules/vale/run.ts | 30 +++++++++++++++++-- packages/cli/src/rules/verdicts.ts | 34 ++++++++++++++++++--- packages/cli/test/rule-recovery.test.ts | 15 ++++++++++ packages/cli/test/run-directory.test.ts | 15 ++++++++++ packages/cli/test/runtime-check.test.ts | 2 +- packages/cli/test/verdicts.test.ts | 3 +- 11 files changed, 151 insertions(+), 12 deletions(-) diff --git a/openspec/changes/cli-v2-rule-api/design.md b/openspec/changes/cli-v2-rule-api/design.md index 8905b7d9..9d6b6dc1 100644 --- a/openspec/changes/cli-v2-rule-api/design.md +++ b/openspec/changes/cli-v2-rule-api/design.md @@ -140,6 +140,19 @@ filesystem, and this host cannot tell whether the process lives. `.taskless/.run/.gitignore` (`*`) is written only if missing, since concurrent runs would race on it. +Two defects the concurrency test found once it was run repeatedly, both fixed: + +- **Vale walked other runs' directories.** Vale's `--glob` exclusion filters which files it + lints, not where it walks, so it `lstat`ed directories other runs were deleting and died + with `E100` (4 of 24 concurrent runs lost every Vale finding). A whole-project Vale run is + now handed each top-level entry except `.taskless/` and `.git/` instead of `.`, so it never + enters `.taskless/`. ast-grep's walker honors the nested `.gitignore` and needed nothing. + Measured after: 0 of 48. +- **A starting run looked abandoned.** A run creates its directory and writes `owner` as two + steps, so a concurrent sweep could see a live run with no owner. Only non-run-id names + (the legacy `snapshot/`, `runtime-rules/`) are swept for being ownerless; a run-id + directory without an owner is swept only after a one-minute grace. + ### 3. What is reported for a rule One `{ ruleId, files }` per directory under `.taskless/rules//`, where diff --git a/openspec/changes/cli-v2-rule-api/tasks.md b/openspec/changes/cli-v2-rule-api/tasks.md index 5e2ce268..86052ec2 100644 --- a/openspec/changes/cli-v2-rule-api/tasks.md +++ b/openspec/changes/cli-v2-rule-api/tasks.md @@ -200,6 +200,15 @@ upgradeUrl }`, strip C0/C1 control characters except newline from lone check, a preserved run holds the logs and snapshot, and no log from an authenticated run contains the token. +- [x] 10.5 Found by running the concurrency test repeatedly: keep Vale's + whole-project walk out of `.taskless/` (it `lstat`s directories other runs + delete), and give an ownerless run-id directory a grace period before it + is swept. 0 of 48 concurrent runs fail, against 4 of 24 before. +- [x] 10.6 Found by the production round trip: `engine.log` records each rule's + verdict (a local `unknown` and a `run` both run and read the same + otherwise), and a restore refusal no longer repeats an upgrade link the + service already wrote into its message. + ## 9. End to end, then archive (slice 5) - [ ] 9.1 From a nightly stamped `0.12.0-*`, against production v2, run the full diff --git a/packages/cli/src/rules/plan-check.ts b/packages/cli/src/rules/plan-check.ts index ba26dca8..c0eeb04d 100644 --- a/packages/cli/src/rules/plan-check.ts +++ b/packages/cli/src/rules/plan-check.ts @@ -242,7 +242,7 @@ export async function planCheck( ); for (const disposition of verdicts.dispositions) { log.write( - `${disposition.engine}/${disposition.ruleId}: ${ + `${disposition.engine}/${disposition.ruleId}: ${disposition.verdict}, ${ disposition.run ? "runs" : `excluded (${disposition.reason ?? "not verified"})` diff --git a/packages/cli/src/rules/recover.ts b/packages/cli/src/rules/recover.ts index 753b9bcb..8728b8a1 100644 --- a/packages/cli/src/rules/recover.ts +++ b/packages/cli/src/rules/recover.ts @@ -65,8 +65,11 @@ function failure( switch (outcome.status) { case "refused": { const { message, upgradeUrl } = outcome.refusal; + // The service writes the upgrade link into `message` itself (measured + // against production, 2026-09-29), so it is added only when absent. + // Printing it twice reads as two different links. return new CLIError( - upgradeUrl === undefined + upgradeUrl === undefined || message.includes(upgradeUrl) ? message : `${message}\n\nUpgrade: ${upgradeUrl}`, "RULE_RECOVERY_NOT_IN_PLAN" diff --git a/packages/cli/src/rules/run-directory.ts b/packages/cli/src/rules/run-directory.ts index b1bb97d1..77cfe39a 100644 --- a/packages/cli/src/rules/run-directory.ts +++ b/packages/cli/src/rules/run-directory.ts @@ -6,6 +6,7 @@ import { readdir, readFile, rm, + stat, writeFile, } from "node:fs/promises"; import { hostname } from "node:os"; @@ -33,13 +34,28 @@ import process from "node:process"; * because it would either delete a slow run still in progress or keep junk for * hours. A directory owned by another host (a shared filesystem) is left * alone, since this host cannot tell whether that process lives. A directory - * with no `owner` file predates run ids (0.11's `runtime-rules/`, the first - * 0.12 `snapshot/`) and is swept too. + * whose name is not a run id predates run ids (0.11's `runtime-rules/`, the + * first 0.12 `snapshot/`) and is swept too. A run-id directory with no `owner` + * is swept only after a grace period, because that is also what a run looks + * like in the instant between creating its directory and recording its owner. */ /** `.taskless/.run`, relative to the project root. */ const RUN_ROOT = join(".taskless", ".run"); +/** A run id, as {@link newRunId} makes them. */ +const RUN_ID = /^\d{8}T\d{6}Z-[0-9a-f]{6}$/; + +/** + * How long a run-id directory may go without an `owner` record before it + * counts as abandoned. A run creates its directory and writes its owner as two + * steps, so for an instant a LIVE run has no owner; a sweep landing there must + * leave it alone. Measured: four concurrent `check`s swept each other's + * directories when ownerless meant abandoned. A run killed inside that instant + * is caught by the next sweep after this grace. + */ +const OWNERLESS_GRACE_MS = 60_000; + /** Who is using a run directory, so a later run can tell if it was abandoned. */ interface Owner { pid: number; @@ -128,6 +144,15 @@ async function readOwner(directory: string): Promise { } } +async function olderThan(path: string, ms: number): Promise { + try { + const { mtimeMs } = await stat(path); + return Date.now() - mtimeMs > ms; + } catch { + return false; + } +} + /** * Remove every run directory whose owner is gone. Returns what was removed, * for the engine log. Never throws: a sweep that cannot finish leaves junk, @@ -149,6 +174,12 @@ export async function sweepAbandonedRuns(cwd: string): Promise { if (owner !== undefined) { if (owner.hostname !== hostname()) continue; if (isAlive(owner.pid)) continue; + } else if ( + // A run id with no owner yet is most likely a run starting right now. + RUN_ID.test(entry.name) && + !(await olderThan(directory, OWNERLESS_GRACE_MS)) + ) { + continue; } try { await rm(directory, { recursive: true, force: true }); diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index 8a8c5557..5977b643 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -1,5 +1,5 @@ import { spawn } from "node:child_process"; -import { stat } from "node:fs/promises"; +import { readdir, stat } from "node:fs/promises"; import { isAbsolute, join, resolve as resolvePath } from "node:path"; import process from "node:process"; import { StringDecoder } from "node:string_decoder"; @@ -465,6 +465,27 @@ async function spawnVale( }); } +/** + * Top-level directories Vale is never handed on a whole-project walk. + * + * `.taskless/` holds the per-run directories under `.taskless/.run/`, which + * other `check`s create and delete while this one runs. Vale's `--glob` + * exclusion filters which files it LINTS, not where it WALKS: it still lists + * and `lstat`s every directory it passes through, and a path that vanishes + * between the two is fatal (`E100: lstat …: no such file or directory`, exit 2). + * Measured: 4 of 24 concurrent whole-project `check`s lost every Vale finding + * that way. `.git/` is never a lint target either. Every other top-level entry + * is handed over as-is, so the files Vale lints are exactly the ones a walk of + * `.` would have linted, less those two trees. + */ +const NEVER_WALKED = new Set([TASKLESS_DIRECTORY, ".git"]); + +/** The targets that stand in for `.` on a whole-project walk. */ +async function wholeProjectTargets(cwd: string): Promise { + const entries = await readdir(cwd); + return entries.filter((name) => !NEVER_WALKED.has(name)).toSorted(); +} + export interface ValeRunOptions { /** Project root. Vale runs here, so the config's relative paths resolve. */ cwd: string; @@ -520,7 +541,12 @@ export async function runVale( // `.taskless/` on any `check .`, independently of the ast-grep fix in this // change. Same defect, same signal, one line apart. const wholeProject = isWholeProjectWalk(paths); - const targets = wholeProject ? ["."] : paths; + const targets = wholeProject ? await wholeProjectTargets(options.cwd) : paths; + // A project with nothing but `.taskless/` and `.git/` has nothing to lint. + // Handing Vale an empty target list would make it walk `.` after all. + if (targets.length === 0) { + return { status: "ok", blocking: false, results: [], notices: [] }; + } // Two exclusions reach Vale, and they have to travel together because Vale // accepts exactly one `--glob` and the last one wins — pass two flags and the diff --git a/packages/cli/src/rules/verdicts.ts b/packages/cli/src/rules/verdicts.ts index 0d34b00a..8ede493d 100644 --- a/packages/cli/src/rules/verdicts.ts +++ b/packages/cli/src/rules/verdicts.ts @@ -59,6 +59,12 @@ export interface RuleDisposition { engine: EngineName; /** Whether it stays in the snapshot the engines read. */ run: boolean; + /** + * What the service answered, for the run's `engine.log`. `run` and a local + * static rule's `unknown` both run, and the log is where telling them apart + * matters. + */ + verdict: "run" | "unsafe" | "unknown" | "withheld" | "unaccounted"; /** Why it did not run, for `skipped` (runtime) and notices. */ reason?: string; } @@ -112,7 +118,13 @@ export function describeDifferences(files: readonly DifferingFile[]): string { /** Record a reported rule the answer did not account for: it does not run, and the run fails. */ function unaccounted(plan: VerdictPlan, rule: ReportedRule, why: string): void { const { ruleId, engine } = rule; - plan.dispositions.push({ ruleId, engine, run: false, reason: why }); + plan.dispositions.push({ + ruleId, + engine, + run: false, + verdict: "unaccounted", + reason: why, + }); plan.integrity.push({ ruleId, engine, verdict: "unaccounted" }); plan.failures.push(`${engine} rule ${ruleId} did not run: ${why}.`); } @@ -175,6 +187,7 @@ export function applyVerdicts( ruleId, engine, run: false, + verdict: "withheld", reason: NOT_IN_PLAN_REASON, }); continue; @@ -184,13 +197,24 @@ export function applyVerdicts( if (engine === "runtime") { const reason = "not issued by the rule service for this repository, so it runs only with --dangerously-run-scripts"; - plan.dispositions.push({ ruleId, engine, run: false, reason }); + plan.dispositions.push({ + ruleId, + engine, + run: false, + verdict: "unknown", + reason, + }); plan.integrity.push({ ruleId, engine, verdict: "unknown" }); } else { // Locally written static rules are first-class, and every one of them // is `unknown`. A notice per rule per run would be noise that trains // people to skip notices. - plan.dispositions.push({ ruleId, engine, run: true }); + plan.dispositions.push({ + ruleId, + engine, + run: true, + verdict: "unknown", + }); } continue; } @@ -214,7 +238,7 @@ export function applyVerdicts( switch (answer.verdict) { case "run": { - plan.dispositions.push({ ruleId, engine, run: true }); + plan.dispositions.push({ ruleId, engine, run: true, verdict: "run" }); break; } case "unsafe": { @@ -226,6 +250,7 @@ export function applyVerdicts( ruleId, engine, run: false, + verdict: "unsafe", reason: `edited since Taskless issued it (${changes})`, }); plan.notices.push( @@ -236,6 +261,7 @@ export function applyVerdicts( ruleId, engine, run: false, + verdict: "unsafe", reason: `edited since Taskless issued it (${changes})`, }); plan.failures.push( diff --git a/packages/cli/test/rule-recovery.test.ts b/packages/cli/test/rule-recovery.test.ts index 4d922a81..4b65e60a 100644 --- a/packages/cli/test/rule-recovery.test.ts +++ b/packages/cli/test/rule-recovery.test.ts @@ -418,6 +418,8 @@ describe("rule restore / rule rollback", () => { }); expect(String(output.message)).toContain("Recover it with git"); expect(String(output.message)).toContain(REFUSAL.upgradeUrl); + // Not in the service's message here, so the CLI adds it, once. + expect(String(output.message).split(REFUSAL.upgradeUrl)).toHaveLength(2); expect(String(output.message)).not.toContain("\u001B"); expect(await readFile(ruleFile(), "utf8")).toBe(EDITED); }); @@ -516,4 +518,17 @@ describe("rule restore / rule rollback", () => { expect(notice).toContain("will not run"); expect(notice).not.toContain("next `check` verifies"); }); + + it("does not repeat an upgrade link the service already wrote into the message", async () => { + await writeLocal(EDITED); + stub({ + verdict: { kind: "unsafe", expected: await canonicalHash(ISSUED) }, + served: { + ...REFUSAL, + message: `Not in your plan. See plan options at ${REFUSAL.upgradeUrl}`, + }, + }); + const output = await run(["restore", RULE_ID]); + expect(String(output.message).split(REFUSAL.upgradeUrl)).toHaveLength(2); + }); }); diff --git a/packages/cli/test/run-directory.test.ts b/packages/cli/test/run-directory.test.ts index f56e7d96..e2a78960 100644 --- a/packages/cli/test/run-directory.test.ts +++ b/packages/cli/test/run-directory.test.ts @@ -7,6 +7,7 @@ import { readdir, readFile, rm, + utimes, writeFile, } from "node:fs/promises"; import { hostname, tmpdir } from "node:os"; @@ -146,6 +147,20 @@ describe("run directories", () => { await readFile(join(cwd, ".taskless", ".run", ".gitignore"), "utf8") ).toBe("*\n# kept\n"); }); + it("spares a run-id directory with no owner yet, which is a run starting", async () => { + const starting = join(cwd, ".taskless", ".run", "20260101T000000Z-cccccc"); + await mkdir(starting, { recursive: true }); + expect(await sweepAbandonedRuns(cwd)).toEqual([]); + expect(existsSync(starting)).toBe(true); + }); + + it("sweeps a run-id directory that stayed ownerless past the grace", async () => { + const orphan = join(cwd, ".taskless", ".run", "20200101T000000Z-dddddd"); + await mkdir(orphan, { recursive: true }); + const old = new Date(Date.now() - 5 * 60_000); + await utimes(orphan, old, old); + expect(await sweepAbandonedRuns(cwd)).toEqual(["20200101T000000Z-dddddd"]); + }); }); describe("check and its run directory", () => { diff --git a/packages/cli/test/runtime-check.test.ts b/packages/cli/test/runtime-check.test.ts index 8882dd97..cf1e2053 100644 --- a/packages/cli/test/runtime-check.test.ts +++ b/packages/cli/test/runtime-check.test.ts @@ -689,7 +689,7 @@ describe("check: static vs runtime dispatch", () => { ) ); expect(logs[0]).toContain("reconcile answered"); - expect(logs[0]).toContain("runtime/demo: runs"); + expect(logs[0]).toContain("runtime/demo: run, runs"); expect(logs[2]).toMatch(/demo: \d+ finding\(s\) in \d+ms/); for (const log of logs) expect(log).not.toContain("fake.token"); }); diff --git a/packages/cli/test/verdicts.test.ts b/packages/cli/test/verdicts.test.ts index 87caccd0..c93eddf9 100644 --- a/packages/cli/test/verdicts.test.ts +++ b/packages/cli/test/verdicts.test.ts @@ -115,7 +115,7 @@ describe("applyVerdicts", () => { restore ); expect(plan.dispositions).toEqual([ - { ruleId: SG.ruleId, engine: "sg", run: true }, + { ruleId: SG.ruleId, engine: "sg", run: true, verdict: "unknown" }, expect.objectContaining({ ruleId: RT.ruleId, run: false }), ]); expect(plan.notices).toEqual([]); @@ -143,6 +143,7 @@ describe("applyVerdicts", () => { ruleId: RT.ruleId, engine: "runtime", run: false, + verdict: "withheld", reason: NOT_IN_PLAN_REASON, }, ]); From b94ae093dffdddec3902f022eb1aef43b137bdbd Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 29 Sep 2026 11:20:18 -0700 Subject: [PATCH 04/10] fix(migrate): add migration 10, adopting tests 0005 left behind for timestamped rule ids --- .changeset/cli-v2-rule-api.md | 2 + .taskless/taskless.json | 2 +- openspec/changes/cli-v2-rule-api/tasks.md | 7 + packages/cli/src/filesystem/migrate.ts | 2 + .../migrations/0010-adopt-stray-tests.ts | 140 ++++++++++++++++ .../test/migrate-adopt-stray-tests.test.ts | 151 ++++++++++++++++++ 6 files changed, 303 insertions(+), 1 deletion(-) create mode 100644 packages/cli/src/filesystem/migrations/0010-adopt-stray-tests.ts create mode 100644 packages/cli/test/migrate-adopt-stray-tests.test.ts diff --git a/.changeset/cli-v2-rule-api.md b/.changeset/cli-v2-rule-api.md index 9bc65076..5e2e217f 100644 --- a/.changeset/cli-v2-rule-api.md +++ b/.changeset/cli-v2-rule-api.md @@ -12,4 +12,6 @@ What you may need to react to: New: `taskless rule restore ` repairs a rule to its current revision, and `taskless rule rollback ` makes an earlier revision current. Both verify every file before writing it. On a plan that does not include rule recovery, they print how to recover the rule from your git history instead. +Upgrading also tidies a `.taskless/` that an earlier migration left half-moved: test files still sitting in `.taskless/sg/rule-tests/` (from rules whose ids ended in a timestamp) are moved into their rule's `.tests/`, and the empty legacy directory is removed. A test that matches no rule is left where it is. + Rules generated by 0.11.x or earlier were issued through the v1 API and are treated as locally written: ast-grep and Vale rules keep running, and runtime rules need to be regenerated. diff --git a/.taskless/taskless.json b/.taskless/taskless.json index 693fa8d5..91a2aa71 100644 --- a/.taskless/taskless.json +++ b/.taskless/taskless.json @@ -1,5 +1,5 @@ { - "version": 9, + "version": 10, "install": { "targets": { ".taskless": { diff --git a/openspec/changes/cli-v2-rule-api/tasks.md b/openspec/changes/cli-v2-rule-api/tasks.md index 86052ec2..c96a34fd 100644 --- a/openspec/changes/cli-v2-rule-api/tasks.md +++ b/openspec/changes/cli-v2-rule-api/tasks.md @@ -209,6 +209,13 @@ upgradeUrl }`, strip C0/C1 control characters except newline from otherwise), and a restore refusal no longer repeats an upgrade link the service already wrote into its message. +- [x] 10.7 Found by the production round trip: migration 10 moves ast-grep tests + that 0005 left in `.taskless/sg/rule-tests/` (rules whose ids end in a + timestamp, tested as `-test.yml`) into the matching rule's `.tests/`, + by longest matching rule id, never overwriting, leaving anything it cannot + match. Verified on the sandbox clone that exposed it; this repository's + scaffold moves to version 10. + ## 9. End to end, then archive (slice 5) - [ ] 9.1 From a nightly stamped `0.12.0-*`, against production v2, run the full diff --git a/packages/cli/src/filesystem/migrate.ts b/packages/cli/src/filesystem/migrate.ts index d1b4bf9c..2c388895 100644 --- a/packages/cli/src/filesystem/migrate.ts +++ b/packages/cli/src/filesystem/migrate.ts @@ -16,6 +16,7 @@ import refreshReadme from "./migrations/0006-refresh-readme"; import ignoreScratchFiles from "./migrations/0007-ignore-scratch-files"; import dropBasedOnStyles from "./migrations/0008-drop-based-on-styles"; import uniqueRuleIds from "./migrations/0009-unique-rule-ids"; +import adoptStrayTests from "./migrations/0010-adopt-stray-tests"; const migrations: Migrations = { "1": init, @@ -27,6 +28,7 @@ const migrations: Migrations = { "7": ignoreScratchFiles, "8": dropBasedOnStyles, "9": uniqueRuleIds, + "10": adoptStrayTests, }; /** Global flag that downgrades a too-new scaffold from an error to a skip. */ diff --git a/packages/cli/src/filesystem/migrations/0010-adopt-stray-tests.ts b/packages/cli/src/filesystem/migrations/0010-adopt-stray-tests.ts new file mode 100644 index 00000000..030410ec --- /dev/null +++ b/packages/cli/src/filesystem/migrations/0010-adopt-stray-tests.ts @@ -0,0 +1,140 @@ +import { mkdir, readdir, rename, rm } from "node:fs/promises"; +import { join } from "node:path"; + +import { pathExists } from "../../rules/reconcile-marker"; +import { + ENGINES, + RULES_DIRECTORY, + RULE_TESTS_DIRECTORY, + type EngineName, +} from "../../rules/layout"; +import type { Migration } from "../types"; + +/** + * Move test files `0005` left behind into the rule they belong to. + * + * `0005` filed an ast-grep test by parsing its name as `-YYYYMMDD-test.yml` + * and taking everything before the date as the rule id. An older CLI named + * both rule and test after a full timestamp, `-YYYYMMDD-HHMMSS`, and its + * test was `-test.yml` with no date of its own, so the pattern never + * matched. The file stayed in `.taskless/sg/rule-tests/`, which was then kept + * because it was not empty. Measured on a real schema-0 project migrated by + * 0.12.0: the rule moved and its only test did not, so the rule had nothing + * exercising it and the stray directory outlived the layout it belonged to. + * + * **A new migration, not a fix to `0005`.** A project already past version 5 + * never re-runs it, so only a new version reaches the projects that already + * carry the stray files. + * + * **Matched against the rules that exist, not parsed.** For a test named + * `-test.yml`, the rule is the LONGEST rule id `R` under the same engine + * where `` is `R` or starts with `R-`. That covers both shapes (`R-test` + * and `R-YYYYMMDD-test`) without a second pattern that could be wrong the same + * way the first was. A directory entry (Vale and runtime keep one per rule) is + * matched by exact name. Anything that matches no rule is left where it is, + * never guessed at and never deleted, and so is its directory. + * + * Never overwrites: a destination that already exists keeps its bytes. + */ + +/** Where `0004` kept tests, relative to `.taskless/`. */ +const PRIOR_TESTS: Record = { + sg: join("sg", "rule-tests"), + vale: join("vale", "rule-tests"), + runtime: join("runtime", "rule-tests"), +}; + +async function directoryNames(path: string): Promise { + try { + const entries = await readdir(path, { withFileTypes: true }); + return entries + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name); + } catch { + return []; + } +} + +/** The rule a flat test file belongs to, or `undefined`. Exported for tests. */ +export function ruleForTestFile( + fileName: string, + ruleIds: readonly string[] +): string | undefined { + const stem = /^(?.+)-test\.ya?ml$/.exec(fileName)?.groups?.stem; + if (stem === undefined) return undefined; + return ruleIds + .filter((id) => stem === id || stem.startsWith(`${id}-`)) + .toSorted((a, b) => b.length - a.length)[0]; +} + +/** Remove `path` if it holds nothing but a `.gitkeep`. */ +async function pruneIfEmpty(path: string): Promise { + let entries; + try { + entries = await readdir(path); + } catch { + return; + } + if (entries.every((name) => name === ".gitkeep")) { + await rm(path, { recursive: true, force: true }); + } +} + +const migration: Migration = async (directory) => { + for (const engine of ENGINES) { + const from = join(directory, PRIOR_TESTS[engine]); + let entries; + try { + entries = await readdir(from, { withFileTypes: true }); + } catch { + continue; + } + const ruleIds = await directoryNames( + join(directory, RULES_DIRECTORY, engine) + ); + + for (const entry of entries) { + if (entry.name === ".gitkeep") continue; + const ruleId = entry.isDirectory() + ? ruleIds.includes(entry.name) + ? entry.name + : undefined + : ruleForTestFile(entry.name, ruleIds); + if (ruleId === undefined) continue; + + const testsDirectory = join( + directory, + RULES_DIRECTORY, + engine, + ruleId, + RULE_TESTS_DIRECTORY + ); + if (entry.isDirectory()) { + // A per-rule test directory becomes the rule's `.tests/`, unless it + // already has one, in which case its entries join it one by one. + if (!(await pathExists(testsDirectory))) { + await rename(join(from, entry.name), testsDirectory); + continue; + } + for (const child of await readdir(join(from, entry.name))) { + const destination = join(testsDirectory, child); + if (await pathExists(destination)) continue; + await rename(join(from, entry.name, child), destination); + } + await pruneIfEmpty(join(from, entry.name)); + continue; + } + + const destination = join(testsDirectory, entry.name); + if (await pathExists(destination)) continue; + // `.tests/` may not exist yet for a rule that never had one. + await mkdir(testsDirectory, { recursive: true }); + await rename(join(from, entry.name), destination); + } + + await pruneIfEmpty(from); + await pruneIfEmpty(join(directory, engine)); + } +}; + +export default migration; diff --git a/packages/cli/test/migrate-adopt-stray-tests.test.ts b/packages/cli/test/migrate-adopt-stray-tests.test.ts new file mode 100644 index 00000000..e3cc77e6 --- /dev/null +++ b/packages/cli/test/migrate-adopt-stray-tests.test.ts @@ -0,0 +1,151 @@ +import { existsSync } from "node:fs"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { ensureTasklessDirectory } from "../src/filesystem/directory"; +import adoptStrayTests, { + ruleForTestFile, +} from "../src/filesystem/migrations/0010-adopt-stray-tests"; + +/** The rule id and test name an older CLI wrote, exactly as found in the wild. */ +const TIMESTAMPED = "vy-first-param-must-be-is-awesome-20260305-001246"; + +describe("ruleForTestFile", () => { + it("matches a test named after the whole id, as older CLIs wrote them", () => { + expect(ruleForTestFile(`${TIMESTAMPED}-test.yml`, [TIMESTAMPED])).toBe( + TIMESTAMPED + ); + }); + + it("matches a dated test, as 0005 expected", () => { + expect(ruleForTestFile("no-eval-20260101-test.yml", ["no-eval"])).toBe( + "no-eval" + ); + }); + + it("prefers the longest rule id when one id prefixes another", () => { + expect( + ruleForTestFile("no-eval-call-20260101-test.yml", [ + "no-eval", + "no-eval-call", + ]) + ).toBe("no-eval-call"); + }); + + it("matches nothing rather than guessing", () => { + expect(ruleForTestFile("orphan-test.yml", ["no-eval"])).toBeUndefined(); + expect(ruleForTestFile("notes.md", ["notes"])).toBeUndefined(); + }); +}); + +describe("migration 10: adopting tests 0005 left behind", () => { + let tasklessDirectory: string; + + beforeEach(async () => { + tasklessDirectory = join( + await mkdtemp(join(tmpdir(), "tskl-0010-")), + ".taskless" + ); + await mkdir(join(tasklessDirectory, "rules", "sg", TIMESTAMPED), { + recursive: true, + }); + await mkdir(join(tasklessDirectory, "sg", "rule-tests"), { + recursive: true, + }); + await writeFile( + join(tasklessDirectory, "sg", "rule-tests", `${TIMESTAMPED}-test.yml`), + "id: x\n" + ); + }); + + afterEach(async () => { + await rm(join(tasklessDirectory, ".."), { recursive: true, force: true }); + }); + + it("moves the test into the rule's .tests/ and removes the emptied layout", async () => { + await adoptStrayTests(tasklessDirectory); + expect( + await readFile( + join( + tasklessDirectory, + "rules", + "sg", + TIMESTAMPED, + ".tests", + `${TIMESTAMPED}-test.yml` + ), + "utf8" + ) + ).toBe("id: x\n"); + expect(existsSync(join(tasklessDirectory, "sg"))).toBe(false); + }); + + it("leaves a test that matches no rule, and its directory, where they are", async () => { + await writeFile( + join(tasklessDirectory, "sg", "rule-tests", "orphan-test.yml"), + "id: y\n" + ); + await adoptStrayTests(tasklessDirectory); + expect( + existsSync(join(tasklessDirectory, "sg", "rule-tests", "orphan-test.yml")) + ).toBe(true); + }); + + it("never overwrites a test the rule already has", async () => { + const existing = join( + tasklessDirectory, + "rules", + "sg", + TIMESTAMPED, + ".tests", + `${TIMESTAMPED}-test.yml` + ); + await mkdir(join(existing, ".."), { recursive: true }); + await writeFile(existing, "id: kept\n"); + await adoptStrayTests(tasklessDirectory); + expect(await readFile(existing, "utf8")).toBe("id: kept\n"); + }); + + it("is a no-op on a project with nothing stray", async () => { + await rm(join(tasklessDirectory, "sg"), { recursive: true }); + await adoptStrayTests(tasklessDirectory); + expect( + existsSync(join(tasklessDirectory, "rules", "sg", TIMESTAMPED)) + ).toBe(true); + }); +}); + +describe("a schema-0 project with timestamped ids, migrated end to end", () => { + let cwd: string; + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "tskl-0010-e2e-")); + // The layout found in taskless-sandbox/nextjs-sass-starter. + await mkdir(join(cwd, ".taskless", "rules"), { recursive: true }); + await mkdir(join(cwd, ".taskless", "rule-tests"), { recursive: true }); + await writeFile( + join(cwd, ".taskless", "rules", `${TIMESTAMPED}.yml`), + `id: ${TIMESTAMPED}\nlanguage: TypeScript\nrule:\n pattern: foo\n` + ); + await writeFile( + join(cwd, ".taskless", "rule-tests", `${TIMESTAMPED}-test.yml`), + `id: ${TIMESTAMPED}\nvalid: []\ninvalid: []\n` + ); + }); + + afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); + }); + + it("files the test inside the rule and leaves no legacy directory", async () => { + await ensureTasklessDirectory(cwd); + const rule = join(cwd, ".taskless", "rules", "sg", TIMESTAMPED); + expect(existsSync(join(rule, `${TIMESTAMPED}.yml`))).toBe(true); + expect(existsSync(join(rule, ".tests", `${TIMESTAMPED}-test.yml`))).toBe( + true + ); + expect(existsSync(join(cwd, ".taskless", "sg"))).toBe(false); + }); +}); From 8d3b8eece7558acad72618ab1ce81a8f933ff6d6 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 29 Sep 2026 14:30:27 -0700 Subject: [PATCH 05/10] docs(openspec): archive cli-v2-rule-api --- .../.openspec.yaml | 0 .../2026-09-29-cli-v2-rule-api}/design.md | 0 .../2026-09-29-cli-v2-rule-api}/proposal.md | 0 .../specs/cli-check/spec.md | 0 .../specs/cli-generated-rule-delivery/spec.md | 0 .../specs/cli-rule-format/spec.md | 0 .../specs/cli-rule-reconciliation/spec.md | 0 .../specs/cli-rule-recovery/spec.md | 0 .../specs/cli-rules/spec.md | 0 .../specs/cli-runtime-rule-execution/spec.md | 0 .../2026-09-29-cli-v2-rule-api}/tasks.md | 27 +- openspec/specs/cli-check/spec.md | 247 ++++++++++---- .../specs/cli-generated-rule-delivery/spec.md | 74 +++- openspec/specs/cli-rule-format/spec.md | 50 ++- .../specs/cli-rule-reconciliation/spec.md | 321 ++++++++++-------- openspec/specs/cli-rule-recovery/spec.md | 110 ++++++ openspec/specs/cli-rules/spec.md | 210 ++++-------- .../specs/cli-runtime-rule-execution/spec.md | 13 +- 18 files changed, 658 insertions(+), 394 deletions(-) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/.openspec.yaml (100%) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/design.md (100%) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/proposal.md (100%) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/specs/cli-check/spec.md (100%) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/specs/cli-generated-rule-delivery/spec.md (100%) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/specs/cli-rule-format/spec.md (100%) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/specs/cli-rule-reconciliation/spec.md (100%) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/specs/cli-rule-recovery/spec.md (100%) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/specs/cli-rules/spec.md (100%) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/specs/cli-runtime-rule-execution/spec.md (100%) rename openspec/changes/{cli-v2-rule-api => archive/2026-09-29-cli-v2-rule-api}/tasks.md (92%) create mode 100644 openspec/specs/cli-rule-recovery/spec.md diff --git a/openspec/changes/cli-v2-rule-api/.openspec.yaml b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/.openspec.yaml similarity index 100% rename from openspec/changes/cli-v2-rule-api/.openspec.yaml rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/.openspec.yaml diff --git a/openspec/changes/cli-v2-rule-api/design.md b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/design.md similarity index 100% rename from openspec/changes/cli-v2-rule-api/design.md rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/design.md diff --git a/openspec/changes/cli-v2-rule-api/proposal.md b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/proposal.md similarity index 100% rename from openspec/changes/cli-v2-rule-api/proposal.md rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/proposal.md diff --git a/openspec/changes/cli-v2-rule-api/specs/cli-check/spec.md b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-check/spec.md similarity index 100% rename from openspec/changes/cli-v2-rule-api/specs/cli-check/spec.md rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-check/spec.md diff --git a/openspec/changes/cli-v2-rule-api/specs/cli-generated-rule-delivery/spec.md b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-generated-rule-delivery/spec.md similarity index 100% rename from openspec/changes/cli-v2-rule-api/specs/cli-generated-rule-delivery/spec.md rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-generated-rule-delivery/spec.md diff --git a/openspec/changes/cli-v2-rule-api/specs/cli-rule-format/spec.md b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-rule-format/spec.md similarity index 100% rename from openspec/changes/cli-v2-rule-api/specs/cli-rule-format/spec.md rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-rule-format/spec.md diff --git a/openspec/changes/cli-v2-rule-api/specs/cli-rule-reconciliation/spec.md b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-rule-reconciliation/spec.md similarity index 100% rename from openspec/changes/cli-v2-rule-api/specs/cli-rule-reconciliation/spec.md rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-rule-reconciliation/spec.md diff --git a/openspec/changes/cli-v2-rule-api/specs/cli-rule-recovery/spec.md b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-rule-recovery/spec.md similarity index 100% rename from openspec/changes/cli-v2-rule-api/specs/cli-rule-recovery/spec.md rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-rule-recovery/spec.md diff --git a/openspec/changes/cli-v2-rule-api/specs/cli-rules/spec.md b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-rules/spec.md similarity index 100% rename from openspec/changes/cli-v2-rule-api/specs/cli-rules/spec.md rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-rules/spec.md diff --git a/openspec/changes/cli-v2-rule-api/specs/cli-runtime-rule-execution/spec.md b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-runtime-rule-execution/spec.md similarity index 100% rename from openspec/changes/cli-v2-rule-api/specs/cli-runtime-rule-execution/spec.md rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/specs/cli-runtime-rule-execution/spec.md diff --git a/openspec/changes/cli-v2-rule-api/tasks.md b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/tasks.md similarity index 92% rename from openspec/changes/cli-v2-rule-api/tasks.md rename to openspec/changes/archive/2026-09-29-cli-v2-rule-api/tasks.md index c96a34fd..b008bb13 100644 --- a/openspec/changes/cli-v2-rule-api/tasks.md +++ b/openspec/changes/archive/2026-09-29-cli-v2-rule-api/tasks.md @@ -218,16 +218,21 @@ upgradeUrl }`, strip C0/C1 control characters except newline from ## 9. End to end, then archive (slice 5) -- [ ] 9.1 From a nightly stamped `0.12.0-*`, against production v2, run the full - round trip in an organization we own: `rule create` → `check` shows `run` - → edit a signed file → `check` fails with `unsafe` → `rule restore` → - `check` shows `run`. Repeat the edit on a vale rule's `.vale.ini`. Record - commands and outputs in the PR. -- [ ] 9.2 Report the round trip to the cloud team so they can close TSKL-307. -- [ ] 9.3 File the follow-ups as issues: a rule-revisions list endpoint (enables - rollback without the dashboard), plan features on `whoami`, the - directory-swap gap, the superseded-revision signal, and the v1 - `Entitlement` type on served file sets. -- [ ] 9.4 Archive the change on the tip branch (`pnpm openspec archive +- [x] 9.1 Round trip against production v2 from a `0.12.0-next-*` build (`pnpm + build:next`; a nightly publishes only from `main`, which the stack reaches + last), in `taskless-sandbox/nextjs-sass-starter` because the `taskless` + installation did not then cover `taskless/cli`. Generated two sg rules and + one Vale rule; `check` returned `run`; a loosened sg pattern and a + disabled `.vale.ini` each returned `unsafe` and failed the run; `rule + restore` on the Free plan returned the git-recovery refusal and wrote + nothing; the issued bytes put back returned `run`. The served-restore + success path needs a paid plan and is covered by stubbed tests only. +- [x] 9.2 Round-trip report drafted for the cloud team to close TSKL-307 (sent + by hand). The Vale `BasedOnStyles =` defect it found is + taskless/taskless#258. +- [x] 9.3 Follow-ups filed: taskless/taskless#252 (v1 `Entitlement` on served + file sets), #253 (rule revisions listing), #254 (plan features on + `whoami`), #255 (directory-swap gap), #256 (superseded-revision signal). +- [x] 9.4 Archive the change on the tip branch (`pnpm openspec archive cli-v2-rule-api`), then re-run the scenario-survival check from 1.1 against the archived specs. diff --git a/openspec/specs/cli-check/spec.md b/openspec/specs/cli-check/spec.md index 8f07df2c..1b08a369 100644 --- a/openspec/specs/cli-check/spec.md +++ b/openspec/specs/cli-check/spec.md @@ -113,7 +113,13 @@ When the `--json` flag is set, the CLI SHALL output each `CheckResult` as a JSON ### Requirement: Check subcommand exit codes reflect error severity -The CLI SHALL exit with code 0 when no error-severity matches are found (including when only warnings, info, or hints exist) and no runtime rule was withheld for entitlement. The CLI SHALL exit with code 1 when at least one error-severity match is found. The CLI SHALL also exit with code 1 when reconciliation completed and the response's `entitlement.withheld` is non-empty, whatever the findings, in both human and `--json` modes. +The CLI SHALL exit with code 0 when no error-severity matches are found (including when only warnings, info, or hints exist) and no reconcile outcome below requires failure. The CLI SHALL exit with code 1 when at least one error-severity match is found. The CLI SHALL also exit with code 1, whatever the findings and in both human and `--json` modes, when a completed reconcile: + +- returned a non-empty `entitlement.withheld`; +- returned an `unsafe` verdict for an `sg` or `vale` rule; or +- left a reported rule unaccounted for (in none, or more than one, of `rules`, `unknown`, and `entitlement.withheld`). + +On an authenticated run that would reconcile, the CLI SHALL also exit with code 1 when two rule directories under different engines share an id. A logged-out run verifies nothing and does not fail on it. Under `--json`, `success` SHALL be `false` whenever the exit code is non-zero. #### Scenario: Exit 0 when clean @@ -141,6 +147,21 @@ The CLI SHALL exit with code 0 when no error-severity matches are found (includi - **WHEN** reconciliation returns `entitlement.runtimeSignatures: false` with an empty or absent `withheld`, and the scan produces zero results - **THEN** the process SHALL exit with code 0 +#### Scenario: Exit 1 when a static rule was edited + +- **WHEN** reconciliation returns an `unsafe` verdict for an `sg` or `vale` rule and the scan produces zero results +- **THEN** the process SHALL exit with code 1 + +#### Scenario: Exit 1 when a reported rule is unaccounted for + +- **WHEN** a reported rule appears in none of `rules`, `unknown`, and `entitlement.withheld` +- **THEN** the process SHALL exit with code 1 + +#### Scenario: Missing does not fail + +- **WHEN** reconciliation returns only `run` and `missing` verdicts and the scan produces zero results +- **THEN** the process SHALL exit with code 0 + ### Requirement: Check subcommand respects global working directory flag The `check` subcommand SHALL use the resolved working directory from the global `-d` flag (or `process.cwd()` if not specified) as the target directory for `.taskless/` validation and scanner execution. @@ -208,14 +229,14 @@ Before forwarding, the CLI SHALL silently drop any path that does not exist on d The `taskless check` command SHALL accept the global `--anonymous` flag (per the `cli` capability). Because `check` reconciles against the Taskless API when authenticated, `--anonymous` SHALL force the logged-out path: it SHALL suppress the reconcile network call -and run all local rule files. Aside from forcing the logged-out path, `--anonymous` SHALL NOT +and run all local static rules. Aside from forcing the logged-out path, `--anonymous` SHALL NOT change scan behavior, output shape, or exit codes relative to an unauthenticated `check`. #### Scenario: check --anonymous skips reconciliation - **WHEN** a user runs `taskless check --anonymous` -- **THEN** the CLI SHALL NOT call `POST /cli/api/reconcile` -- **AND** SHALL scan all local rule files +- **THEN** the CLI SHALL NOT call `POST /cli/api/v2/reconcile` +- **AND** SHALL scan all local static rules #### Scenario: check --anonymous matches an unauthenticated check @@ -235,69 +256,76 @@ When `taskless check --json` exits with an error, the output SHALL conform to th ### Requirement: Check selects what it runs from auth state -`taskless check` SHALL NOT require authentication, and it SHALL choose what it runs from the -current auth state. **Static ast-grep rules** (single `*.yml` files under `.taskless/sg/rules/`, or the pre-migration `.taskless/rules/`) -SHALL always run without contacting the server, on every path (the offline linter posture). -**Runtime rules** (directories with `metadata.taskless.kind: runtime`) SHALL run only on a -signature-validated path: when a token is available and `--anonymous` is not set the CLI SHALL -reconcile and execute the runtime rules fully returned in `run`; when no token is available, -when `--anonymous` is set, or when reconciliation cannot complete, the CLI SHALL skip runtime -execution unless `--dangerously-run-scripts` is set. The unauthenticated path SHALL succeed -with no network access for static rules and SHALL NOT emit a warning about missing -authentication. +`taskless check` SHALL NOT require authentication, and it SHALL choose what it verifies from +the current auth state. When a token is available and `--anonymous` is not set, the CLI SHALL +reconcile **every** rule (ast-grep, Vale, and runtime) and apply the verdict policy of the +`cli-rule-reconciliation` capability. When no token is available, when `--anonymous` is set, or +when reconciliation cannot complete, the CLI SHALL run every static rule (ast-grep and Vale) +unverified and SHALL skip runtime execution unless `--dangerously-run-scripts` is set. The +unauthenticated path SHALL succeed with no network access and SHALL NOT emit a warning about +missing authentication. #### Scenario: Unauthenticated check runs static rules and skips runtime rules - **WHEN** a user runs `taskless check` with no available token -- **THEN** the CLI SHALL scan all static rule files -- **AND** SHALL NOT call `POST /cli/api/reconcile` for the purpose of running static rules +- **THEN** the CLI SHALL scan all static rules +- **AND** SHALL NOT call `POST /cli/api/v2/reconcile` - **AND** SHALL skip runtime rules - **AND** SHALL NOT emit a warning about missing authentication #### Scenario: Authenticated check reconciles runtime rules - **WHEN** a user runs `taskless check` with an available token and without `--anonymous` -- **THEN** the CLI SHALL reconcile and execute the runtime rules fully returned in `run` -- **AND** SHALL run static rules unconditionally +- **THEN** the CLI SHALL reconcile its ast-grep, Vale, and runtime rules in one request +- **AND** SHALL run or execute each rule according to its verdict #### Scenario: Anonymous forces the logged-out path - **WHEN** a user runs `taskless check --anonymous` while a token is available - **THEN** the CLI SHALL behave exactly as an unauthenticated `check` (static rules run, runtime rules skipped, no reconcile call) +#### Scenario: Logged out, an edited static rule still runs + +- **WHEN** a user runs `taskless check` with no available token and an issued sg or vale rule has been edited +- **THEN** the edited rule SHALL run with no signature enforcement +- **AND** runtime rules SHALL be skipped +- **AND** the exit code SHALL NOT change because the rule was edited + +#### Scenario: Logged in, an edited rule of any engine does not run + +- **WHEN** an authenticated `check` reconciles and a rule of any engine is `unsafe` +- **THEN** that rule SHALL NOT run or execute + ### Requirement: Check reconciles rule files before scanning -`taskless check` SHALL reconcile before running runtime rules whenever a bearer token and a -`repositoryUrl` are resolvable and `--anonymous` is not set. It SHALL compute the signature -envelope for the `check.ts` of every runtime rule under `.taskless/rules/runtime/`, call -`POST /cli/api/reconcile` with `{ repositoryUrl, files }`, and then execute **only** the -runtime rules whose `check.ts` is returned in the `run` set, matched back to local files by -signature (per the `cli-rule-reconciliation` capability). Capture `*.yml` and static rules -SHALL NOT be gated by reconciliation. +`taskless check` SHALL reconcile before running any rule whenever a bearer token and a +`repositoryUrl` are resolvable and `--anonymous` is not set. It SHALL snapshot the rules tree, +report every rule directory from the snapshot to `POST /cli/api/v2/reconcile` (per the +`cli-rule-reconciliation` capability), remove from the snapshot every rule its verdict +excludes, and only then assemble engine configs and run. #### Scenario: Only rules with a blessed check.ts execute -- **WHEN** a user runs `taskless check` while authenticated and reconciliation returns a `run` - set covering the `check.ts` of some runtime rules and not others -- **THEN** the CLI SHALL execute only the runtime rules whose `check.ts` is in `run` -- **AND** SHALL run all static rules regardless of the `run` set +- **WHEN** a user runs `taskless check` while authenticated and reconciliation returns `run` for some rules, `unsafe` for a static rule, and `unknown` for a runtime rule +- **THEN** the CLI SHALL run the `run` rules +- **AND** SHALL NOT run the `unsafe` static rule +- **AND** SHALL NOT execute the `unknown` runtime rule ### Requirement: Check degrades to a local scan when reconciliation cannot complete `taskless check` SHALL NOT fail solely because an attempted reconciliation cannot complete. When a token is available and `--anonymous` is not set but reconciliation cannot complete (no -resolvable git remote, or the reconcile endpoint is unreachable or not yet deployed, or a -transport error), the CLI SHALL warn that rule verification could not be performed, SHALL fall -back to scanning all **static** rule files, and SHALL **skip runtime rules** (their `check.ts` -SHALL NOT run) unless `--dangerously-run-scripts` is set. The CLI SHALL NOT exit with a -non-zero code solely because reconciliation failed, and the warning SHALL be suppressed under -`--json`. +resolvable git remote, the endpoint unreachable, a `401`, a `404 organization_not_found`, or a +transport error), the CLI SHALL warn that rule verification could not be performed, SHALL run +every **static** rule from the snapshot unverified, and SHALL **skip runtime rules** unless +`--dangerously-run-scripts` is set. The CLI SHALL NOT exit with a non-zero code solely because +reconciliation failed, and the warning SHALL be suppressed under `--json`. #### Scenario: Endpoint unreachable degrades static and skips runtime -- **WHEN** an authenticated `check` attempts reconciliation and the endpoint is unreachable or returns a not-deployed error +- **WHEN** an authenticated `check` attempts reconciliation and the endpoint is unreachable or returns an error - **THEN** the CLI SHALL warn that verification could not be performed -- **AND** SHALL scan all static rule files +- **AND** SHALL scan all static rules - **AND** SHALL NOT execute any runtime rule's `check.ts` - **AND** SHALL NOT exit with a non-zero code solely due to the reconcile failure @@ -319,24 +347,22 @@ non-zero code solely because reconciliation failed, and the warning SHALL be sup ### Requirement: Check runs runtime rules only on a signature-validated path -`taskless check` SHALL execute a runtime rule's `check.ts` only when that `check.ts` has been -validated by the server — returned in the reconciliation `run` set — or when -`--dangerously-run-scripts` is set. When a token is available and `--anonymous` is not set, the -CLI SHALL reconcile and execute every runtime rule whose `check.ts` is in `run`. An API key -SHALL be treated identically to an interactive token. On any path where the runtime rule's -`check.ts` signature is not validated — logged out, `--anonymous`, or a reconciliation that -cannot complete — the CLI SHALL NOT execute the rule's `check.ts`. +`taskless check` SHALL execute a runtime rule's `check.ts` only when reconciliation returned a +`run` verdict for that rule, or when `--dangerously-run-scripts` is set. An API key SHALL be +treated identically to an interactive token. On any path where the rule was not validated — +logged out, `--anonymous`, a reconciliation that cannot complete, or a verdict other than +`run` — the CLI SHALL NOT execute the rule's `check.ts`. #### Scenario: Authenticated check runs blessed runtime rules -- **WHEN** an authenticated `check` reconciles and a runtime rule's `check.ts` is returned in `run` +- **WHEN** an authenticated `check` reconciles and a runtime rule's verdict is `run` - **THEN** the CLI SHALL execute that runtime rule through the harness #### Scenario: A rule whose check.ts is not blessed is withheld -- **WHEN** reconciliation does not return a runtime rule's `check.ts` in `run` (it lands in `unsafe`/`unknown`/`missing`) +- **WHEN** reconciliation returns `unsafe` for a runtime rule, lists it in `unknown`, or withholds it for entitlement - **THEN** the CLI SHALL NOT execute that runtime rule -- **AND** SHALL surface it as an advisory mismatch +- **AND** SHALL report it as skipped with a reason naming the verdict #### Scenario: API key behaves like a token @@ -374,12 +400,13 @@ did not run. ### Requirement: Check accepts --dangerously-run-scripts to run runtime rules without server validation -`taskless check` SHALL accept a `--dangerously-run-scripts` flag that runs **all** runtime -rules without server validation, regardless of auth state. +`taskless check` SHALL accept a `--dangerously-run-scripts` flag that runs **all** rules without +server validation, regardless of auth state. When the flag is set the CLI SHALL NOT reconcile — it SHALL skip the network entirely (matching -how `--anonymous` forces the no-network path) and execute every present runtime rule. The CLI -SHALL emit a prominent warning that runtime rule code is being executed unverified. The flag -SHALL be the only way to execute runtime rules on an unverified path. +how `--anonymous` forces the no-network path), SHALL compute and enforce no signatures for any +engine, run every present static rule, and execute every present runtime rule. The CLI SHALL +emit a prominent warning that runtime rule code is being executed unverified. The flag SHALL be +the only way to execute runtime rules on an unverified path. #### Scenario: Dangerously-run-scripts executes runtime rules offline @@ -393,6 +420,13 @@ SHALL be the only way to execute runtime rules on an unverified path. - **THEN** stdout SHALL contain only the existing `{ success, results }` JSON shape - **AND** the unverified-execution warning SHALL NOT appear in stdout +#### Scenario: No signature is enforced while logged in + +- **WHEN** an authenticated user runs `taskless check --dangerously-run-scripts` and an issued vale rule has been edited +- **THEN** the CLI SHALL NOT call reconcile +- **AND** the edited rule SHALL run +- **AND** the exit code SHALL NOT change because the rule was edited + ### Requirement: Check runs engines concurrently and merges their results `taskless check` SHALL run its per-engine executors concurrently and merge their `CheckResult`s into a single result set. A missing or unavailable engine SHALL NOT abort the others; its absence SHALL be reported while the remaining engines still produce results. @@ -507,12 +541,12 @@ A notice SHALL NOT affect the exit code. ### Requirement: Check reports runtime rules withheld for entitlement as a plan outcome -When reconciliation completes and returns a non-empty `entitlement.withheld`, `taskless check` SHALL NOT execute any withheld rule, SHALL report each local runtime rule whose reported `check.ts` path appears in `withheld` as skipped with a reason stating that runtime rules are not included in the organization's plan, and SHALL NOT describe it as unsafe, unknown, drifted, or tampered. The human output SHALL include one notice naming the withheld rules, the `entitlement.reason`, and the `entitlement.upgradeUrl` when present. Under `--json`, the output SHALL carry an additive, optional `entitlement` object with `runtimeSignatures`, `reason`, `upgradeUrl`, and `withheld` (the local rule names), present only when reconciliation returned `runtimeSignatures: false`. This is a verified outcome and SHALL NOT be treated as one of the unverified paths that leave the exit code unchanged. +When reconciliation completes and returns a non-empty `entitlement.withheld`, `taskless check` SHALL NOT execute any withheld rule, SHALL report each local runtime rule whose `ruleId` appears in `withheld` as skipped with a reason stating that runtime rules are not included in the organization's plan, and SHALL NOT describe it as unsafe, unknown, drifted, or tampered. The human output SHALL include one notice naming the withheld rules, the `entitlement.reason`, and the `entitlement.upgradeUrl` when present. Under `--json`, the output SHALL carry an additive, optional `entitlement` object with `runtimeSignatures`, `reason`, `upgradeUrl`, and `withheld` (the local rule names), present only when reconciliation returned `runtimeSignatures: false`. This is a verified outcome and SHALL NOT be treated as one of the unverified paths that leave the exit code unchanged. #### Scenario: Withheld rule is named with its cause -- **WHEN** an authenticated `check` reconciles and `entitlement.withheld` lists the reported `check.ts` of runtime rule `no-env-leak` -- **THEN** `no-env-leak` SHALL NOT execute +- **WHEN** an authenticated `check` reconciles and `entitlement.withheld` lists runtime rule `no-env-leak-3fa9c21b` +- **THEN** `no-env-leak-3fa9c21b` SHALL NOT execute - **AND** its skip reason SHALL state that runtime rules are not included in the plan - **AND** its skip reason SHALL NOT mention unsafe, unknown, or drift @@ -529,10 +563,109 @@ When reconciliation completes and returns a non-empty `entitlement.withheld`, `t #### Scenario: A server without the entitlement object is unchanged -- **WHEN** reconciliation returns the four arrays and no `entitlement` object -- **THEN** `check` SHALL behave exactly as before this change, including its exit code and skip reasons +- **WHEN** reconciliation returns `entitlement: { runtimeSignatures: true }`, or (against its schema) no `entitlement` object at all +- **THEN** `check --json` SHALL omit the `entitlement` field +- **AND** no rule SHALL be skipped for its plan #### Scenario: Degrade paths still never fail - **WHEN** `check` runs logged out, with `--anonymous`, or reconciliation cannot complete - **THEN** the exit code SHALL NOT change because runtime rules were skipped, as before this change + +### Requirement: Check never writes to the rules tree + +`taskless check` SHALL NOT create, modify, or delete anything under `.taskless/rules/`. It +SHALL NOT call restore, rollback, or rule fetch. For an `unsafe` or `missing` verdict it SHALL +name the command that repairs the rule, `taskless rule restore `. The only files +`check` writes under `.taskless/` SHALL be under `.taskless/.run/`. + +#### Scenario: An edited rule is reported, not repaired + +- **WHEN** reconciliation returns `unsafe` for a rule +- **THEN** `.taskless/rules/` SHALL be byte-identical before and after the run +- **AND** the output SHALL name `taskless rule restore ` + +#### Scenario: A missing rule is not fetched + +- **WHEN** reconciliation returns `missing` for a rule +- **THEN** `check` SHALL NOT call any restore or fetch endpoint +- **AND** SHALL NOT create the rule's directory + +### Requirement: Each check run works in its own run directory + +`taskless check` SHALL do its work (the snapshot, the assembled engine configs, and its logs) +in a directory of its own, `.taskless/.run//`, where `` is unique per run and +sorts by start time. Two runs SHALL never share a run directory. The directory SHALL hold an +`owner` record naming the process and host using it, and the logs `engine.log` (the plan: what +was copied, reported, judged, excluded, and run), `sg.log` and `vale.log` (each engine's +command line, output, and exit code), and `runtime.log` (each runtime rule's duration, +findings, and any error). No log SHALL contain a credential. The CLI SHALL remove the run +directory when the run ends, whether it succeeded or failed, and on SIGINT or SIGTERM, unless +`--preserve-logs` is set. `.taskless/.run/` SHALL ignore itself with its own `.gitignore`, so +that no run rewrites a tracked file. `rule restore` SHALL take its snapshot the same way. + +#### Scenario: Concurrent runs do not disturb each other + +- **WHEN** several `taskless check` runs execute at the same time in one project +- **THEN** each SHALL report the same findings it reports alone +- **AND** none SHALL read another's snapshot + +#### Scenario: Nothing is left behind + +- **WHEN** a `taskless check` run ends, successfully or not, without `--preserve-logs` +- **THEN** its run directory SHALL no longer exist + +### Requirement: Check accepts --preserve-logs to keep its run directory + +`taskless check` SHALL accept `--preserve-logs` (alias `-l`), which keeps the run directory +instead of removing it: the snapshot that ran, the assembled configs, the `owner` record, and +the logs. Human output SHALL name the kept directory on stderr. Under `--json`, the output +SHALL carry an additive, optional `runDirectory` field, the directory's path relative to the +project root, present only when the flag is set. + +#### Scenario: A preserved run is named and complete + +- **WHEN** a user runs `taskless check --json --preserve-logs` +- **THEN** stdout SHALL include `runDirectory` +- **AND** that directory SHALL hold `engine.log`, `sg.log`, `vale.log`, `runtime.log`, `owner`, and the snapshot + +#### Scenario: A preserved authenticated run holds no credential + +- **WHEN** an authenticated `check --preserve-logs` reconciles +- **THEN** no file in the kept run directory SHALL contain the token + +### Requirement: Abandoned run directories are swept + +At the start of every run, the CLI SHALL remove each directory under `.taskless/.run/` whose +`owner` names a process on this host that is no longer alive, and each directory with no +`owner` record (left by an earlier version). It SHALL NOT remove a directory whose owning +process is alive, or one owned by another host, since this host cannot tell whether that +process lives. A directory's age SHALL NOT be the test. + +#### Scenario: A killed run's directory is swept + +- **WHEN** a run directory's `owner` names a process on this host that has exited +- **THEN** the next run SHALL remove it + +#### Scenario: A live run is never swept + +- **WHEN** a run directory's owning process is still running, or it is owned by another host +- **THEN** no other run SHALL remove it + +### Requirement: Check reports rule integrity under --json + +Under `--json`, `taskless check` SHALL carry an additive, optional `integrity` array with one +entry per rule whose outcome is `unsafe`, `missing`, runtime `unknown`, `unaccounted`, or +`duplicate`, each `{ ruleId, engine?, verdict, files?, revisionId? }`. `files` SHALL list each +differing path with `expected` and `got` as the server returned them. Static `unknown` rules +and `run` rules SHALL NOT appear. The field SHALL be omitted when there is nothing to report. + +#### Scenario: An edited rule appears with its differing files + +- **WHEN** reconciliation returns `unsafe` for vale rule `no-simply-1a2b3c4d` with `.vale.ini` changed +- **THEN** `integrity` SHALL include `{ ruleId: "no-simply-1a2b3c4d", engine: "vale", verdict: "unsafe", files: [{ path: ".vale.ini", expected, got }] }` + +#### Scenario: A clean run omits the field + +- **WHEN** every reported rule is `run` or static `unknown` and nothing is `missing` +- **THEN** `check --json` SHALL NOT include `integrity` diff --git a/openspec/specs/cli-generated-rule-delivery/spec.md b/openspec/specs/cli-generated-rule-delivery/spec.md index 438f89db..c980d6bb 100644 --- a/openspec/specs/cli-generated-rule-delivery/spec.md +++ b/openspec/specs/cli-generated-rule-delivery/spec.md @@ -8,13 +8,12 @@ How a rule generated by the service arrives on disk as a file set, and which des ### Requirement: A delivered rule is a file set -The CLI SHALL accept a generated rule as a set of files, each with a path relative to -`.taskless/rules///` and its content as text. One shape SHALL serve every engine, -validated against `ENGINE_LAYOUTS` — the table the CLI already holds — so that "is this a complete -rule" is answered from data rather than from per-engine prose. - -A response entry carrying the legacy single `content` object SHALL remain valid and SHALL continue -to be filed as an ast-grep rule. `files` and `content` SHALL be mutually exclusive. +The CLI SHALL accept a rule served by the v2 API (`GET /cli/api/v2/rule/{ruleId}`, restore, +or rollback) as exactly one file set `{ id, engine, files, signatures }`, each file with a path +relative to `.taskless/rules///` and its content as text. One shape SHALL serve every +engine, validated against `ENGINE_LAYOUTS` — the table the CLI already holds — so that "is this a +complete rule" is answered from data rather than from per-engine prose. The legacy single +`content` object is not part of v2 and SHALL NOT be accepted. #### Scenario: A runtime rule arrives complete @@ -34,6 +33,11 @@ to be filed as an ast-grep rule. `files` and `content` SHALL be mutually exclusi - **THEN** the CLI SHALL refuse the rule and name what is missing - **AND** SHALL NOT write a partial rule directory +#### Scenario: A response with more than one rule is refused + +- **WHEN** a v2 rule response carries a `rules` array whose length is not exactly one, or whose one set's `id` is not the requested rule id +- **THEN** the CLI SHALL refuse the response and write nothing + ### Requirement: Delivered paths are refused before they are written The CLI SHALL reject an absolute path, any `..` segment, and any path the engine layout does not @@ -49,7 +53,7 @@ whose destination the client computed itself. ### Requirement: A runtime rule written under a plan without runtime signatures says it will not run -When the CLI writes a runtime rule from a response whose `entitlement.runtimeSignatures` is exactly `false` (from `rule create`, `rule improve`, or a restore during `check`), it SHALL still write the rule, and SHALL emit a warning that the rule is on disk but will not run on the organization's current plan, including the `upgradeUrl` when it is an absolute `https:` URL. The warning SHALL be carried in the command's existing notices, so it appears in human output and under `--json`. A restore under such a response SHALL NOT state or imply that the next `check` will bless or run the rule. A response with no `entitlement` object, or with `runtimeSignatures: true`, SHALL produce no such warning. +When the CLI writes a runtime rule from a response whose `entitlement.runtimeSignatures` is exactly `false` (from `rule create`, `rule improve`, `rule restore`, or `rule rollback`), it SHALL still write the rule, and SHALL emit a warning that the rule is on disk but will not run on the organization's current plan, including the `upgradeUrl` when it is an absolute `https:` URL. The warning SHALL be carried in the command's existing notices, so it appears in human output and under `--json`. A restore or rollback under such a response SHALL NOT state or imply that the next `check` will run the rule. A response with no `entitlement` object, or with `runtimeSignatures: true`, SHALL produce no such warning. #### Scenario: Created runtime rule under an unentitled plan @@ -60,9 +64,9 @@ When the CLI writes a runtime rule from a response whose `entitlement.runtimeSig #### Scenario: Restore under an unentitled plan does not promise blessing -- **WHEN** a restore during `check` writes a runtime rule and the restore response carries `entitlement.runtimeSignatures: false` -- **THEN** the restore notice SHALL say the rule's bytes were restored but will not run on the current plan -- **AND** SHALL NOT say the next `check` blesses it +- **WHEN** `rule restore` writes a runtime rule and the restore response carries `entitlement.runtimeSignatures: false` +- **THEN** the notice SHALL say the rule's bytes were restored but will not run on the current plan +- **AND** SHALL NOT say the next `check` runs it #### Scenario: Entitled or legacy responses do not warn @@ -73,3 +77,51 @@ When the CLI writes a runtime rule from a response whose `entitlement.runtimeSig - **WHEN** `rule create` writes an `sg` or `vale` rule - **THEN** the CLI SHALL emit no entitlement warning + +### Requirement: A delivered file set is verified against its signatures + +Before writing any file of a served rule, the CLI SHALL verify that every signature names a +delivered file, that every delivered file outside `.tests/` has exactly one signature, and that +the algoVersion-1 `canonicalHash` of each such file's content equals its signature. For a +runtime rule it SHALL also verify that the singular `signature` equals the `check.ts` entry. +Any failure SHALL refuse the whole rule and write nothing. + +#### Scenario: A corrupted file is refused + +- **WHEN** a served file's content does not hash to its entry in `signatures` +- **THEN** the CLI SHALL refuse the rule naming the file +- **AND** SHALL write nothing for it + +#### Scenario: An unsigned file is refused + +- **WHEN** a served file outside `.tests/` has no entry in `signatures` +- **THEN** the CLI SHALL refuse the rule + +#### Scenario: Fixtures need no signature + +- **WHEN** a served rule carries files under `.tests/` that have no entry in `signatures` +- **THEN** the CLI SHALL accept them + +### Requirement: A delivered rule replaces its directory + +After verification, the CLI SHALL write a served rule so that its directory holds exactly the +served files: every file in the directory that the served set does not contain SHALL be removed, +including under `.tests/`, since a served set always carries the rule's fixtures. The CLI SHALL +create each served file's parent directories as needed, so a nested path such as +`captures/env.yml` or `.tests/fail/case.ts` is written wherever it lands. When a stale file cannot be removed, the CLI SHALL say that the served +bytes were written and name each entry it could not remove. + +#### Scenario: A local extra file does not survive + +- **WHEN** a rule directory holds `captures/extra.yml` and the served set does not +- **THEN** after the write `captures/extra.yml` SHALL NOT exist + +#### Scenario: Nested paths are written + +- **WHEN** a served set carries `.tests/fail/case.ts` and the rule directory does not exist +- **THEN** the CLI SHALL create `.tests/fail/` and write the file + +#### Scenario: A generated rule's revision is confirmed + +- **WHEN** `rule create` or `rule improve` fetches a produced rule whose served `revisionId` differs from the one the request reported +- **THEN** the CLI SHALL refuse that rule and write nothing for it diff --git a/openspec/specs/cli-rule-format/spec.md b/openspec/specs/cli-rule-format/spec.md index 0c8b1275..7ef45c87 100644 --- a/openspec/specs/cli-rule-format/spec.md +++ b/openspec/specs/cli-rule-format/spec.md @@ -29,37 +29,17 @@ The migration to the engine-partitioned layout SHALL move the existing `.taskles - **WHEN** the migration runs - **THEN** `.taskless/vale/` is created with empty `rules/` and `rule-tests/`, and `.taskless/runtime-rules/` becomes `.taskless/runtime/rules/` with byte-identical contents -### Requirement: Service-delivered rules without an engine are written as ast-grep - -The rule ingest path SHALL write a service-delivered rule into the engine directory its payload identifies. The current API carries **no** engine discriminator — `/cli/api/rule/{ruleId}` returns `rules[].content` documented as an ast-grep rule definition — so a payload that does not identify an engine SHALL be written as ast-grep, under `.taskless/sg/rules/.yml`, with its tests under `.taskless/sg/rule-tests/`. - -This default is permanent, not a migration window: published CLIs and stored payloads without an engine field continue to exist indefinitely, and the default matches what the migration does to the same rules already on disk. - -Absence of an engine and an **unrecognized** engine are distinct. If a payload identifies an engine the installed CLI does not know, ingest SHALL fail with an error naming the engine and instructing the user to upgrade, and SHALL NOT fall back to ast-grep. - -#### Scenario: Engine-less payload is filed under sg - -- **WHEN** a rule is delivered by the service with no engine identified in its payload -- **THEN** it is written to `.taskless/sg/rules/.yml` and its tests to `.taskless/sg/rule-tests/`, and a subsequent `check` dispatches it to ast-grep - -#### Scenario: Ingest and migration agree on destination - -- **WHEN** a rule that predates the engine-partitioned layout is migrated, and an equivalent rule is delivered fresh by the service -- **THEN** both come to rest at the same path under `.taskless/sg/rules/` - -#### Scenario: Unrecognized engine fails loudly - -- **WHEN** a payload identifies an engine the installed CLI does not support -- **THEN** ingest exits with an error naming the engine and directing the user to upgrade, and no rule file is written under any engine directory - ### Requirement: Reconciliation survives the relayout -The CLI SHALL report rule files to the reconcile endpoint at their post-migration repo-relative paths. Because the server joins reported files by content signature rather than by path, moving a rule without editing it SHALL NOT change its reconciled state. +The CLI SHALL report each rule to the reconcile endpoint by its `ruleId` (the rule's directory +name) with file paths relative to the rule's own directory, so where the engine-partitioned +layout places a rule's directory is not part of what is reported. Moving a rule directory +without renaming or editing it SHALL NOT change its reconciled state. #### Scenario: Moved rules reconcile unchanged -- **WHEN** `check` reconciles after the migration has moved rules from `.taskless/rules/` to `.taskless/sg/rules/` and runtime rules to `.taskless/runtime/rules/` -- **THEN** each file's signature is unchanged, the server resolves it to the same rule, and no rule is reported as new or missing +- **WHEN** `check` reconciles after the migration has moved rules into `.taskless/rules///` +- **THEN** each rule is reported under the same `ruleId` with the same relative paths and signatures, the server resolves it to the same rule, and no rule is reported as new or missing ### Requirement: The CLI refuses a scaffold newer than it understands unless overridden @@ -173,3 +153,21 @@ Each engine SHALL have one canonical on-disk location per rule — the rule dire - **WHEN** `verify` or `test` is given `.taskless/rules///` - **THEN** it operates on exactly that rule - **AND** the engine is determined from the path without reading the rule + +### Requirement: A served rule is filed under the engine it names + +The CLI SHALL write a rule served by the v2 API into `.taskless/rules///`, where +`` is the file set's own `engine`, which v2 always sends. If the set names an engine +the installed CLI does not know, the CLI SHALL refuse it with an error naming the engine and +directing the user to upgrade, SHALL write nothing under any engine directory, and SHALL NOT +fall back to ast-grep. + +#### Scenario: A served rule lands under its engine + +- **WHEN** a served file set declares engine `vale` +- **THEN** it is written under `.taskless/rules/vale//` + +#### Scenario: An unrecognized engine fails loudly + +- **WHEN** a served file set declares an engine the installed CLI does not support +- **THEN** the CLI exits with an error naming the engine and directing the user to upgrade, and no rule file is written under any engine directory diff --git a/openspec/specs/cli-rule-reconciliation/spec.md b/openspec/specs/cli-rule-reconciliation/spec.md index 6e373e95..e27c8145 100644 --- a/openspec/specs/cli-rule-reconciliation/spec.md +++ b/openspec/specs/cli-rule-reconciliation/spec.md @@ -2,7 +2,7 @@ ## Purpose -Defines the CLI-side contract for server-owned rule reconciliation: how the CLI computes a canonical signature envelope for each rule file, how it calls the `POST /cli/api/reconcile` endpoint, and how it executes only the server's `run` set. The server-side record is authoritative; local signatures are advisory only. +Defines the CLI-side contract for server-owned rule reconciliation: how the CLI computes a canonical signature envelope for each file of a rule, how it reports every rule of every engine to `POST /cli/api/v2/reconcile` from a snapshot it then runs, and how it applies the per-rule verdicts, accounts for every rule it reported, and reads the plan entitlement. The server-side record is authoritative; local signatures are advisory only. ## Requirements @@ -15,16 +15,15 @@ before the **first** `;` is the algoVersion and SHALL be read up to that one del detect the version (and therefore the normalization procedure and hash algorithm) before any `key=value` parameters are parsed. Signatures SHALL be compared as whole strings. -The algoVersion SHALL also determine **what the signature covers**. A signature at -algoVersion `1` covers exactly one file: the engine's `ruleFile` from the rule layout -table, which is `check.ts` for the runtime engine. An engine that requires more than one -file to be signed SHALL do so under a later algoVersion. +The algoVersion SHALL also determine **what one signature covers**. A signature at +algoVersion `1` covers exactly one file. **Which** files of a rule are signed is set by the +reconcile contract, not by the envelope: every file in the rule's directory except those +under `.tests/`, each with its own signature. -**Rationale.** Coverage is a property of the signature scheme, not of the delivery that -carries a signature. Stating it on the version keeps a payload from having to say which -of its files is the signed one, and gives a future multi-file scheme a mechanism that -already exists rather than a new payload shape. Leaving it unstated is what let both -teams hold the same binding as a private assumption. +**Rationale.** Coverage per signature is a property of the signature scheme; the set of +signed files is a property of the rule contract. Keeping them apart is what lets the rule +contract grow from "`check.ts` only" to "every non-fixture file" without a new algoVersion, +because no single signature's meaning changed. #### Scenario: Envelope is emitted for algoVersion 1 @@ -42,10 +41,21 @@ teams hold the same binding as a private assumption. - **WHEN** the CLI compares two signatures for equality - **THEN** it SHALL compare the full envelope strings, not the bare digests +#### Scenario: A v1 signature covers exactly one file + +- **WHEN** the CLI computes an algoVersion-1 signature for a rule +- **THEN** that signature SHALL be over one file's bytes and over no other file + #### Scenario: A v1 signature covers the engine's rule file -- **WHEN** a runtime rule carries a signature at algoVersion 1 -- **THEN** that signature SHALL be over `check.ts` and over no other file in the rule directory +- **WHEN** a served runtime rule carries its singular `signature` +- **THEN** that signature SHALL be over `check.ts` and SHALL equal the `check.ts` entry in `signatures` + +#### Scenario: Every non-fixture file of a rule is signed + +- **WHEN** the CLI signs a rule directory for reconcile +- **THEN** it SHALL compute one signature per file in the directory +- **AND** SHALL NOT sign any file under `.tests/` ### Requirement: Signature normalization procedure (algoVersion 1) @@ -103,7 +113,7 @@ reproduces the server's reference implementation byte-for-byte. ### Requirement: Conformance vectors are fetched and asserted The CLI SHALL consume the cross-repo conformance vectors served at -`GET /cli/api/rule-hash-vectors` (unauthenticated) as `{ vectors: [{ name, input, signature }] }`, +`GET /cli/api/v2/rule-hash-vectors` (unauthenticated) as `{ vectors: [{ name, input, signature }] }`, commit a copy as a fixture, and assert in its test suite that its independent `normalize()`-plus-hash reproduces every vector's `signature` exactly. Non-ASCII `input` SHALL be parsed as JSON (decoding `\uXXXX` escapes) before hashing. A vector mismatch SHALL @@ -120,190 +130,225 @@ be a release blocker (the test SHALL fail the build). - **THEN** the test SHALL fail - **AND** the build SHALL NOT pass -### Requirement: Reconcile reports every runtime rule's check.ts +#### Scenario: Vectors come from the v2 surface -Reconciliation SHALL be scoped to the **`check.ts` of runtime rules** — the only artifact that -carries arbitrary code execution. Static ast-grep rules and runtime-rule capture `*.yml` are -inert data, always available, and SHALL NOT be reported to or gated by reconciliation. The CLI -SHALL reconcile by sending `POST /cli/api/reconcile` with an `Authorization: Bearer ` -header and a JSON body `{ repositoryUrl, files }`, where `repositoryUrl` is the full repository -URL and `files` is an array of `{ file, signature }` covering the `check.ts` of **every** -runtime rule the CLI holds under `.taskless/rules/runtime/`. `file` SHALL be the `check.ts`'s -delivered path as it exists on disk and `signature` SHALL be the full envelope computed for its -bytes. The CLI SHALL send the whole signature envelope, not a bare digest. +- **WHEN** the build refreshes the committed vectors +- **THEN** it SHALL fetch `GET /cli/api/v2/rule-hash-vectors` +- **AND** SHALL NOT call a v1 route -#### Scenario: Every runtime rule's check.ts is reported +### Requirement: Reconcile is scoped to the token's organization -- **WHEN** the CLI reconciles with runtime rules present under `.taskless/rules/runtime/` -- **THEN** the request body SHALL include one `{ file, signature }` entry for the `check.ts` of each runtime rule +The reconcile endpoint SHALL be authorized by the bearer token and SHALL be scoped to the +organization named by the optional `orgId` (a Taskless org UUID, preferred, or a numeric +GitHub org id) or, when absent, by the token. The CLI SHALL handle the documented edges: a +`401` with `{ error: "unauthorized" }` for a missing or invalid token; a `404` with +`{ error: "organization_not_found" }` when the organization is not accessible or the +repository is not covered by its installation (deliberately indistinguishable); an empty +corpus (every reported rule in `unknown`, nothing runs on a verdict); and an empty report +(every issued rule in `missing`). -#### Scenario: Inert files are not reported +#### Scenario: Unauthorized token -- **WHEN** the CLI reconciles and static `*.yml` rules and capture `*.yml` are also present -- **THEN** the request body SHALL NOT include entries for those inert files -- **AND** the static rules SHALL run regardless of the reconcile response +- **WHEN** the CLI calls reconcile without a valid bearer token +- **THEN** the server SHALL return `401` with `{ error: "unauthorized" }` +- **AND** the CLI SHALL NOT execute any runtime rule on the basis of that call -#### Scenario: Full envelope is sent +#### Scenario: Empty corpus runs nothing -- **WHEN** the CLI reports a file's signature -- **THEN** it SHALL send the complete `1;h=sha-256;d=` envelope, not only the digest +- **WHEN** the repository has no issued rules and the CLI reports rules +- **THEN** every reported rule SHALL be returned in `unknown` +- **AND** the CLI SHALL execute no runtime rule -### Requirement: Reconcile response buckets drive execution +#### Scenario: Organization not found is not a service outage -The reconcile response SHALL be interpreted as four buckets — `run`, `unsafe`, `unknown`, -and `missing` — and each SHALL drive a specific CLI action: +- **WHEN** reconcile answers `404` with `{ error: "organization_not_found" }` +- **THEN** the CLI SHALL name both causes (the installation does not cover this repository, or the login lost access) +- **AND** SHALL treat the run as unverified rather than as a verdict -- `run`: the file's content matches a rule the server blessed. The CLI SHALL execute it. This - is the complete allow-list. -- `unsafe`: a rule held by delivered name whose content differs from what the server blessed - (`expected` vs `got`). The CLI SHALL NOT run it and SHALL surface it as tamper/drift. -- `unknown`: a reported file the server never issued. The CLI SHALL NOT run it and SHALL - surface it as advisory. -- `missing`: a rule the server expected that the CLI did not report. It is not actionable for - execution and SHALL be treated as advisory/audit only. +### Requirement: Local signatures are advisory only -The CLI SHALL match each `run` entry back to a local file by its `signature` (content-based -join), so a file that was moved but not changed still resolves. +Any signature the CLI persists into the repository (for example as sidecar metadata) SHALL be +treated as an offline-fallback convenience only and SHALL NOT be treated as an authorization +signal. The server-side record is authoritative and the server decides what runs. -#### Scenario: Run entries are matched by signature +#### Scenario: Sidecar signature is not authorization -- **WHEN** the CLI processes a `run` entry -- **THEN** it SHALL locate the corresponding local file by matching the `signature`, not the path +- **WHEN** a rule file has a locally stored signature that matches its content +- **THEN** the CLI SHALL NOT run the file on that basis alone +- **AND** SHALL rely on the server's `run` set for authorization -#### Scenario: Unsafe is surfaced and not run +### Requirement: Reconcile reports every rule directory -- **WHEN** a reported file lands in `unsafe` -- **THEN** the CLI SHALL NOT execute it -- **AND** SHALL surface it as tamper/drift +When `check` reconciles, the CLI SHALL send `POST /cli/api/v2/reconcile` with +`{ repositoryUrl, orgId?, rules }`, where `rules` holds exactly one +`{ ruleId, files: [{ path, signature }] }` for **every** rule directory under +`.taskless/rules//`, of every engine, whatever `--rule` selects to run. `ruleId` +SHALL be the directory name. `files` SHALL list every regular file under the directory, +recursively, except files under `.tests/` and the operating-system metadata files +`.DS_Store`, `Thumbs.db`, and `desktop.ini`. `path` SHALL be relative to the rule directory +with `/` separators on every platform, and `signature` SHALL be the full algoVersion-1 +envelope of the snapshot's bytes for that file. -#### Scenario: Unknown is surfaced and not run +#### Scenario: Every engine is reported -- **WHEN** a reported file lands in `unknown` -- **THEN** the CLI SHALL NOT execute it -- **AND** SHALL surface it as an advisory notice +- **WHEN** `.taskless/rules/sg/`, `.taskless/rules/vale/`, and `.taskless/rules/runtime/` each hold rules +- **THEN** the request SHALL carry one entry per rule directory across all three engines -#### Scenario: Missing is advisory only +#### Scenario: A rule's files are reported relative to its directory -- **WHEN** the response includes `missing` entries -- **THEN** the CLI SHALL treat them as advisory/audit only and SHALL NOT fail execution on them +- **WHEN** runtime rule `no-env-leak-3fa9c21b` holds `check.ts` and `captures/env.yml` +- **THEN** its entry SHALL be `{ ruleId: "no-env-leak-3fa9c21b", files: [{ path: "check.ts", … }, { path: "captures/env.yml", … }] }` -### Requirement: The CLI executes only the server run set +#### Scenario: Fixtures are not reported -When reconciliation succeeds, the CLI SHALL execute a runtime rule only if its **`check.ts`** -is present in the server's `run` set, and SHALL execute no runtime rule whose `check.ts` is -absent from `run`. The CLI SHALL NOT run a runtime rule on the basis of its own local -comparison of signatures. This replaces any local classification of runtime rules. Static -ast-grep rules and capture `*.yml` are outside this gate. +- **WHEN** a rule directory holds files under `.tests/` +- **THEN** no reported path SHALL begin with `.tests/` -#### Scenario: Only rules with a blessed check.ts execute +#### Scenario: --rule does not narrow the report -- **WHEN** reconciliation returns a `run` set covering the `check.ts` of some runtime rules but not others -- **THEN** the CLI SHALL execute only the runtime rules whose `check.ts` is in `run` -- **AND** SHALL withhold any runtime rule whose `check.ts` is absent from `run` +- **WHEN** a user runs `check --rule a` in a project holding rules `a` and `b` +- **THEN** the reconcile request SHALL report both `a` and `b` +- **AND** only `a` SHALL run -#### Scenario: No local self-classification +### Requirement: Rule ids are unique across engines -- **WHEN** the CLI holds a local signature or sidecar for a runtime rule's `check.ts` -- **THEN** it SHALL NOT treat that local value as authorization to execute the rule +Before reconciling, the CLI SHALL refuse the run when two rule directories under different +engines share a directory name, naming both directories. It SHALL NOT report either rule +and SHALL NOT resolve the collision by skipping one of them. -### Requirement: Reconcile is scoped to the token's organization +#### Scenario: A duplicate id stops the run -The reconcile endpoint SHALL be authorized by the same bearer-token / `orgId`-claim scheme as -all `/cli/api/*` endpoints and SHALL be scoped to the organization the token owns. The CLI -SHALL handle the documented edges: a `401` with `{ error: "unauthorized" }` for a missing or -invalid token; an empty corpus (empty `run`/`missing`, every reported file in `unknown`, -nothing runs); and an empty report (empty `run`/`unsafe`/`unknown`, the full corpus in -`missing`). +- **WHEN** both `.taskless/rules/sg/foo-3fa9c21b/` and `.taskless/rules/vale/foo-3fa9c21b/` exist +- **THEN** `check` SHALL exit non-zero naming both directories +- **AND** SHALL NOT call reconcile -#### Scenario: Unauthorized token +#### Scenario: A decoy cannot neutralize an issued rule -- **WHEN** the CLI calls reconcile without a valid bearer token -- **THEN** the server SHALL return `401` with `{ error: "unauthorized" }` -- **AND** the CLI SHALL NOT execute any rule from a `run` set +- **WHEN** someone creates a directory under a second engine with the id of an issued rule +- **THEN** the issued rule SHALL NOT run as though it were locally authored -#### Scenario: Empty corpus runs nothing +### Requirement: Reconcile verdicts are applied per engine -- **WHEN** the repository has no blessed rules and the CLI reports files -- **THEN** every reported file SHALL be returned in `unknown` -- **AND** the CLI SHALL execute nothing +The CLI SHALL read the v2 reconcile response as a list of per-rule verdicts +(`rules[]`, each `{ ruleId, engine, verdict }` with `verdict` one of `run`, `unsafe`, +`missing`), a list of `unknown` rule ids, and `entitlement.withheld`, and SHALL apply this +policy: -### Requirement: Local signatures are advisory only +| Verdict | runtime | sg / vale | +| ---------- | -------------------------------------------------- | --------------------------------------------- | +| `run` | execute | run | +| `withheld` | do not execute; fail `check` | (never sent) | +| `unsafe` | do not execute; name `rule restore` | do not run; fail `check`; name `rule restore` | +| `missing` | warn; name `rule restore` | warn; name `rule restore` | +| `unknown` | do not execute (needs `--dangerously-run-scripts`) | run | -Any signature the CLI persists into the repository (for example as sidecar metadata) SHALL be -treated as an offline-fallback convenience only and SHALL NOT be treated as an authorization -signal. The server-side record is authoritative and the server decides what runs. +The engine SHALL be taken from the verdict's `engine` for `rules[]` entries and from the +reporting directory for `unknown` entries. An `unsafe` notice SHALL name the rule and each +differing path, saying whether it changed, was removed, or was added. A signature SHALL +authorize running a runtime rule only through a `run` verdict, never by local comparison. -#### Scenario: Sidecar signature is not authorization +#### Scenario: An edited static rule fails and does not run -- **WHEN** a rule file has a locally stored signature that matches its content -- **THEN** the CLI SHALL NOT run the file on that basis alone -- **AND** SHALL rely on the server's `run` set for authorization +- **WHEN** reconcile returns `{ ruleId: "no-simply-1a2b3c4d", engine: "vale", verdict: "unsafe", files: [{ path: ".vale.ini", expected, got }] }` +- **THEN** the rule SHALL NOT run +- **AND** `check` SHALL exit non-zero naming the rule and `.vale.ini` as changed -### Requirement: The reported file path is contractual +#### Scenario: A locally written static rule runs -The CLI SHALL report each runtime check to reconcile as a **repo-root-relative POSIX path**, -`.taskless/rules/runtime//check.ts`, with platform separators normalized so every host -reports the same string for the same rule. +- **WHEN** reconcile lists a static rule's id in `unknown` +- **THEN** that rule SHALL run +- **AND** the CLI SHALL emit no notice for it -This is a cross-team contract, not an implementation detail. Reconcile matches `run` by content -digest and is path-independent, but the `unsafe` versus `unknown` split is a lookup on the -reported name: a spelling mismatch downgrades a tampered file from `unsafe` ("content changed in -place") to `unknown` ("never issued") — a diagnostic loss on the one path where the diagnosis -matters. +#### Scenario: A locally written runtime rule does not execute -#### Scenario: The reported path is repo-relative and POSIX +- **WHEN** reconcile lists a runtime rule's id in `unknown` and `--dangerously-run-scripts` is not set +- **THEN** the rule SHALL NOT execute +- **AND** its skip reason SHALL say it was not issued by the rule service -- **WHEN** the CLI reports a runtime check to reconcile -- **THEN** the `file` SHALL be `.taskless/rules/runtime//check.ts` -- **AND** the separators SHALL be `/` regardless of host platform +#### Scenario: Missing warns and does not fail -### Requirement: A withheld rule can be re-fetched +- **WHEN** reconcile returns a `missing` verdict for any engine +- **THEN** the CLI SHALL warn naming the rule and `taskless rule restore ` +- **AND** SHALL NOT change the exit code because of it -When reconcile reports a rule as `unsafe` or `unknown`, the CLI SHALL be able to request the -blessed bytes for that rule rather than only warning. The request SHALL carry the rule id **and** -the signature the client holds, scoped like reconcile itself, so that a bare digest cannot be used -to retrieve content across organizations. +#### Scenario: An edited runtime rule is withheld, not failed -The response SHALL be the bytes matching the held signature, never the newest generation. -Answering with newer bytes would upgrade a rule in the middle of a `check` without anyone asking; -upgrading is regeneration and SHALL remain an explicit action. +- **WHEN** reconcile returns an `unsafe` verdict for a runtime rule +- **THEN** the rule SHALL NOT execute +- **AND** the exit code SHALL NOT change because of that verdict alone -#### Scenario: An unsafe rule is repaired +### Requirement: Every reported rule is accounted for -- **WHEN** reconcile reports a rule as `unsafe` -- **THEN** the CLI SHALL be able to re-fetch the blessed bytes for that rule -- **AND** the rule SHALL reconcile as `run` after the bytes are restored +After a reconcile completes, every `ruleId` the CLI reported SHALL appear in exactly one of +`rules[]`, `unknown[]`, or `entitlement.withheld[]`. A reported rule that appears in none, +or in more than one, SHALL be treated as unaccounted: it SHALL NOT run or execute, and +`check` SHALL fail naming it. `missing` verdicts name rules that were not reported and are +outside this check. -#### Scenario: Re-fetch does not upgrade +#### Scenario: A dropped rule fails the run -- **WHEN** a newer generation of the same rule exists server-side -- **THEN** re-fetch SHALL still return the bytes matching the signature the client reported +- **WHEN** the CLI reports rule `a` and the response names `a` in none of `rules`, `unknown`, or `entitlement.withheld` +- **THEN** `a` SHALL NOT run +- **AND** `check` SHALL exit non-zero naming `a` -### Requirement: The CLI reads the reconcile entitlement object +#### Scenario: A rule answered twice fails the run -The CLI SHALL read the optional `entitlement` object on a successful reconcile response. It SHALL treat the organization as unentitled only when `entitlement.runtimeSignatures` is exactly `false`; an absent, malformed, or `true` value SHALL be treated as no entitlement outcome, leaving the four buckets to drive execution exactly as before. For an unentitled response the CLI SHALL read `reason`, `upgradeUrl`, and `withheld` (a list of `{ ruleId, file }`), SHALL drop a `withheld` entry without a string `file`, and SHALL surface `upgradeUrl` only when it is an absolute `https:` URL. +- **WHEN** a reported rule appears both in `rules[]` and in `entitlement.withheld` +- **THEN** it SHALL NOT run +- **AND** `check` SHALL exit non-zero naming it -`entitlement.withheld` SHALL be a disposition distinct from the four buckets: a withheld file SHALL NOT be executed, SHALL NOT be surfaced as tamper, drift, or never-issued, and SHALL NOT be sent to restore. The CLI SHALL match a withheld entry to a local runtime rule by the `file` it reported, since the entry carries no signature. A withheld entry that matches no reported file SHALL still count as withheld for the purpose of the exit code. +### Requirement: The CLI runs the bytes it reported -#### Scenario: Absent entitlement changes nothing +Before signing anything, `check` SHALL copy `.taskless/rules/` into a snapshot inside its own +run directory under `.taskless/.run/` (per the `cli-check` capability), dereferencing symbolic +links. +It SHALL compute every reported signature from the snapshot and SHALL run every engine +from the snapshot, with the assembled configs written under `.taskless/.run/`. A rule the +verdict excludes SHALL be removed from the snapshot before any engine configuration is +assembled. The snapshot SHALL be taken on every path, including unauthenticated and +`--anonymous` runs. -- **WHEN** a reconcile response carries no `entitlement` object -- **THEN** the CLI SHALL drive execution from `run`, `unsafe`, `unknown`, and `missing` exactly as before +#### Scenario: An edit after signing does not run -#### Scenario: Entitled response changes nothing +- **WHEN** a rule file under `.taskless/rules/` is edited after `check` has signed the snapshot +- **THEN** the engines SHALL run the snapshot's bytes, not the edited file -- **WHEN** a reconcile response carries `entitlement: { runtimeSignatures: true }` -- **THEN** the CLI SHALL drive execution from the four buckets exactly as before +#### Scenario: Static rules run from the snapshot + +- **WHEN** `check` runs sg and vale rules +- **THEN** the ast-grep and Vale configs SHALL point into the snapshot under `.taskless/.run/` +- **AND** SHALL NOT point into `.taskless/rules/` + +#### Scenario: An excluded rule is absent from what runs + +- **WHEN** a static rule's verdict is `unsafe` +- **THEN** its directory SHALL be absent from the snapshot the engines read + +### Requirement: The CLI reads the v2 reconcile entitlement -#### Scenario: Withheld is matched by reported path +The CLI SHALL read the reconcile response's `entitlement` object, which v2 always sends. It +SHALL treat the organization as unentitled only when `entitlement.runtimeSignatures` is +exactly `false`, SHALL read `reason`, `upgradeUrl`, and `withheld` as a list of +`{ ruleId, revisionId }`, SHALL match a withheld entry to a reported rule by `ruleId`, and +SHALL surface `upgradeUrl` only when it is an absolute `https:` URL. A withheld entry SHALL +NOT be dropped for lacking any field other than `ruleId`. A withheld rule SHALL NOT be +executed, SHALL NOT be described as unsafe, unknown, or drifted, and SHALL NOT be offered +restore. -- **WHEN** `entitlement.withheld` lists `{ ruleId, file }` and `file` equals the path the CLI reported for a runtime rule's `check.ts` -- **THEN** the CLI SHALL classify that rule as withheld for entitlement and SHALL NOT execute it +#### Scenario: Withheld is matched by rule id + +- **WHEN** `entitlement.withheld` lists `{ ruleId: "no-env-leak-3fa9c21b", revisionId }` and the CLI reported that rule +- **THEN** the rule SHALL be classified as withheld for entitlement and SHALL NOT execute + +#### Scenario: Entitled response withholds nothing + +- **WHEN** a reconcile response carries `entitlement: { runtimeSignatures: true }` +- **THEN** no rule SHALL be classified as withheld -#### Scenario: Withheld is not repaired +#### Scenario: Withheld is not offered restore - **WHEN** a runtime rule is withheld for entitlement -- **THEN** the CLI SHALL NOT request a restore for it +- **THEN** no notice SHALL suggest restoring it #### Scenario: A malformed upgrade URL is not shown diff --git a/openspec/specs/cli-rule-recovery/spec.md b/openspec/specs/cli-rule-recovery/spec.md new file mode 100644 index 00000000..72002dfc --- /dev/null +++ b/openspec/specs/cli-rule-recovery/spec.md @@ -0,0 +1,110 @@ +# cli-rule-recovery Specification + +## Purpose + +How a rule Taskless issued is put back on disk after `check` reports it edited or missing (`rule restore`), or moved to an earlier revision (`rule rollback`): what each command asks the service for, how every served byte is verified before anything is written, and how a plan that does not include recovery is reported as an answer rather than a failure. `check` itself never does either. + +## Requirements + +### Requirement: Rule restore repairs a rule to its current revision + +The CLI SHALL provide `taskless rule restore `, which requires authentication and a +resolvable repository. It SHALL snapshot and reconcile the rules tree exactly as `check` would, +read only the named rule's outcome, and act on it: + +- `run` or withheld for entitlement: nothing to restore; say so and exit 0. +- `unknown`: say the rule was not issued for this repository and cannot be restored; exit non-zero. +- `unsafe` or `missing`: call `POST /cli/api/v2/rule/{ruleId}/restore` with `{ repositoryUrl, orgId? }`. + +It SHALL NOT change any other rule. + +#### Scenario: An edited rule is restored + +- **WHEN** a user runs `taskless rule restore no-simply-1a2b3c4d` and reconcile returns `unsafe` for it +- **THEN** the CLI SHALL call restore for that rule +- **AND** after a verified write the next `check` SHALL reconcile it as `run` + +#### Scenario: An intact rule is left alone + +- **WHEN** reconcile returns `run` for the named rule +- **THEN** the CLI SHALL NOT call restore +- **AND** SHALL exit 0 saying the rule already matches an issued revision + +#### Scenario: A locally written rule cannot be restored + +- **WHEN** reconcile lists the named rule in `unknown` +- **THEN** the CLI SHALL exit non-zero saying the rule was not issued for this repository + +### Requirement: Restore writes only what reconcile expected + +Before writing a restored rule the CLI SHALL verify the served file set against its own +`signatures` (per `cli-generated-rule-delivery`) and against reconcile's expectation: + +- for `unsafe`: the served signature map SHALL equal the local signature map with each reported + file's `expected` applied and each `got`-only path removed; +- for `missing`: the served `revisionId` SHALL equal the verdict's `revisionId`. + +On any mismatch it SHALL write nothing and exit non-zero, saying restore repairs a rule and does +not upgrade one. + +#### Scenario: A newer revision is not written by restore + +- **WHEN** the served set's signatures differ from reconcile's expected map for an `unsafe` rule +- **THEN** the CLI SHALL write nothing +- **AND** SHALL exit non-zero naming the rule + +#### Scenario: A missing rule is restored to the revision reconcile named + +- **WHEN** reconcile returns `missing` with `revisionId: "r1"` and restore serves `revisionId: "r1"` with valid signatures +- **THEN** the CLI SHALL write the rule directory + +### Requirement: Rule rollback makes an earlier revision current + +The CLI SHALL provide `taskless rule rollback `, which requires +authentication and a resolvable repository and calls +`POST /cli/api/v2/rule/{ruleId}/rollback` with `{ repositoryUrl, revisionId, orgId? }`. It SHALL +write the served set only when the served `revisionId` equals the requested one and every file +verifies against its signature. `404 revision_not_found` and `404 rule_not_found` SHALL each be +reported as such. + +#### Scenario: Rollback writes the requested revision + +- **WHEN** a user rolls rule `no-eval-3fa9c21b` back to revision `r1` and the server serves `r1` with valid signatures +- **THEN** the CLI SHALL replace the rule directory with the served set + +#### Scenario: A different revision is refused + +- **WHEN** rollback serves a `revisionId` other than the one requested +- **THEN** the CLI SHALL write nothing and exit non-zero + +### Requirement: A plan refusal is an answer, not a failure of the service + +When restore or rollback answers `200` with `restoreRules: false`, the CLI SHALL print the +response's `message` verbatim with C0 and C1 control characters other than newline removed, then +`upgradeUrl` when it is an absolute `https:` URL, and SHALL write nothing. It SHALL exit non-zero +with code `RULE_RECOVERY_NOT_IN_PLAN` and SHALL NOT describe the outcome as the service being +unavailable. An unrecognized `reason` SHALL be handled the same way. + +#### Scenario: Free plan gets git guidance + +- **WHEN** restore answers `{ restoreRules: false, reason: "RESTORE_RULES_NOT_IN_PLAN", message, upgradeUrl }` +- **THEN** the CLI SHALL print `message` and the upgrade URL +- **AND** SHALL exit non-zero with `RULE_RECOVERY_NOT_IN_PLAN` + +#### Scenario: Control characters are stripped + +- **WHEN** a refusal `message` contains an ANSI escape sequence +- **THEN** the printed message SHALL NOT contain the escape character + +### Requirement: Recovery commands report under --json + +Under `--json`, `rule restore` and `rule rollback` SHALL print +`{ success: true, ruleId, revisionId, files, notices? }` on success and the standardized error +envelope `{ ok: false, code, message }` otherwise, with `code` distinguishing +`RULE_RECOVERY_NOT_IN_PLAN`, `RULE_NOT_FOUND`, `REVISION_NOT_FOUND`, `RULE_RESTORE_MISMATCH`, +`AUTH_REQUIRED`, and `NETWORK_ERROR`. + +#### Scenario: A refusal is machine-readable + +- **WHEN** `rule restore --json` is refused for the plan +- **THEN** stdout SHALL be `{ ok: false, code: "RULE_RECOVERY_NOT_IN_PLAN", message }` with `message` carrying the server's guidance diff --git a/openspec/specs/cli-rules/spec.md b/openspec/specs/cli-rules/spec.md index 929fde80..4d46d731 100644 --- a/openspec/specs/cli-rules/spec.md +++ b/openspec/specs/cli-rules/spec.md @@ -83,7 +83,12 @@ When the git remote cannot yield a GitHub repository URL, the command SHALL fail ### Requirement: Rules create submits to API and polls for results -`taskless rule create` without `--anonymous` SHALL submit the request to the API and poll for the result per the existing requirement. (Renamed to singular.) +`taskless rule create` without `--anonymous` SHALL submit the request to +`POST /cli/api/v2/request`, poll `GET /cli/api/v2/request/{requestId}` until the request reaches +`generated`, `failed`, or `unsupported`, and then fetch each produced rule's head with +`GET /cli/api/v2/rule/{ruleId}` (without `revision`), in parallel, per the +`cli-generated-rule-delivery` capability. On `failed` or `unsupported` it SHALL print the +response's `error` as given. #### Scenario: Submission returns a request to poll @@ -91,19 +96,16 @@ When the git remote cannot yield a GitHub repository URL, the command SHALL fail - **THEN** the CLI SHALL submit the request to the API - **AND** it SHALL poll for the result until the generation completes or fails -### Requirement: Rules create uses a network interface with stub +#### Scenario: Each produced rule is fetched by its id -The API calls for rule generation (`POST /cli/api/request` and `GET /cli/api/request/:requestId`) SHALL be defined as a TypeScript interface. The initial implementation SHALL use a stub that returns an error indicating the API is not yet available. This allows the CLI UX to be built and tested independently of the API. +- **WHEN** polling reaches `generated` with `revisions: [{ ruleId, revisionId }, …]` +- **THEN** the CLI SHALL fetch `GET /cli/api/v2/rule/{ruleId}` for each, without `revision` +- **AND** SHALL write each rule only after confirming its served `revisionId` -#### Scenario: Stub implementation returns an error +#### Scenario: A plan refusal is printed as given -- **WHEN** `rule create` is run against the stub network layer -- **THEN** the stub SHALL return an error indicating rule generation is not yet available - -#### Scenario: Interface is swappable - -- **WHEN** the real API becomes available -- **THEN** the stub SHALL be replaceable with a real HTTP implementation without changing the command logic +- **WHEN** polling reaches `failed` or `unsupported` with an `error` +- **THEN** the CLI SHALL print that `error` verbatim (control characters stripped) and exit non-zero ### Requirement: Rules create writes rule files to disk @@ -126,13 +128,24 @@ The API calls for rule generation (`POST /cli/api/request` and `GET /cli/api/req ### Requirement: Rules create outputs results -`taskless rule create` SHALL output results per the existing requirement. (Renamed to singular.) Output SHALL be human-readable by default; `--json` produces machine-readable output. On failure with `--json` set, the output SHALL be the standardized error envelope `{ ok: false, code: "", message: "<...>" }` per the `cli` capability requirements. +`taskless rule create` SHALL output results human-readable by default; `--json` produces +machine-readable output `{ success, requestId, rules, files, notices? }`, where `requestId` is the +generation request id and `rules` lists the produced rule ids (their directory names). It SHALL +NOT emit a field named `ruleId`. On failure with `--json` set, the output SHALL be the +standardized error envelope `{ ok: false, code: "", message: "<...>" }` per the `cli` +capability requirements. #### Scenario: Failure under --json uses the error envelope - **WHEN** `taskless rule create --json` fails - **THEN** the CLI SHALL print `{ ok: false, code, message }` rather than prose +#### Scenario: Success under --json names the request and the rules + +- **WHEN** `taskless rule create --json` produces rule `no-eval-3fa9c21b` +- **THEN** stdout SHALL include `requestId` and `rules: ["no-eval-3fa9c21b"]` +- **AND** SHALL NOT include `ruleId` + ### Requirement: Rules create shows progress during polling `taskless rule create` SHALL show progress while polling the API. The `--anonymous` branch polls nothing and SHOULD show progress for the local agent-driven steps where applicable. (Renamed to singular.) @@ -144,13 +157,20 @@ The API calls for rule generation (`POST /cli/api/request` and `GET /cli/api/req ### Requirement: Rules improve reads request from file -`taskless rule improve` SHALL accept a `--from ` flag specifying a JSON file containing the iterate request. (Renamed to singular.) +`taskless rule improve` SHALL accept a `--from ` flag specifying a JSON file containing the +iterate request `{ ruleId, guidance, references? }`, where `ruleId` is the rule's directory name +under `.taskless/rules//`, the id v2 addresses a rule by. (Renamed to singular.) #### Scenario: The request is read from the named file - **WHEN** a user runs `taskless rule improve --from request.json` - **THEN** the CLI SHALL read the iterate request from that file +#### Scenario: The rule id is the directory name + +- **WHEN** the request names `ruleId: "no-eval-3fa9c21b"` +- **THEN** the CLI SHALL iterate the rule at `.taskless/rules//no-eval-3fa9c21b/` + ### Requirement: Rules improve requires authentication `taskless rule improve` SHALL require authentication unless `--anonymous` is set. (Renamed; new anonymous branch.) @@ -167,7 +187,10 @@ The API calls for rule generation (`POST /cli/api/request` and `GET /cli/api/req ### Requirement: Rules improve submits to iterate API and polls for results -`taskless rule improve` without `--anonymous` SHALL submit to the iterate API and poll for the result per the existing requirement. (Renamed.) +`taskless rule improve` without `--anonymous` SHALL submit to +`POST /cli/api/v2/rule/{ruleId}/iterate`, poll the returned `requestId` exactly as `rule create` +does, and fetch and write the produced revision the same way. A `404 rule_not_found` SHALL be +reported as `RULE_NOT_FOUND`, not as a network error. #### Scenario: Submission returns a request to poll @@ -175,6 +198,11 @@ The API calls for rule generation (`POST /cli/api/request` and `GET /cli/api/req - **THEN** the CLI SHALL submit to the iterate API - **AND** it SHALL poll until the iteration completes or fails +#### Scenario: An unknown rule id is reported as such + +- **WHEN** iterate answers `404` with `{ error: "rule_not_found" }` +- **THEN** the CLI SHALL fail with code `RULE_NOT_FOUND` + ### Requirement: Rules improve writes updated files to disk `taskless rule improve` SHALL write updated rule files to disk in both branches. (Renamed; strengthened.) @@ -343,144 +371,16 @@ When `taskless rule improve --anonymous` is invoked, the CLI SHALL execute the l heading: a second `##` inside this section ends it, and everything after it stops being read as a requirement. -### Requirement: Rule generation request endpoint accepts a request and returns a requestId - -The server SHALL expose `POST /cli/api/rule` that accepts an authenticated request with a JSON body containing `orgId` (number, required), `repositoryUrl` (string, required), `prompt` (string, required), `successCases` (array of strings, optional), and `failureCases` (array of strings, optional). The endpoint SHALL return a JSON response containing `ruleId` (string) and `status` set to `"accepted"`. - -#### Scenario: Valid request returns a ruleId - -- **WHEN** an authenticated client sends a POST to `/cli/api/rule` with valid `orgId`, `repositoryUrl`, and `prompt` -- **THEN** the server SHALL return HTTP 200 with `{ ruleId: string, status: "accepted" }` - -#### Scenario: Request with example arrays - -- **WHEN** an authenticated client includes `successCases` and `failureCases` arrays -- **THEN** the server SHALL accept the arrays and use them for rule generation context - -#### Scenario: Missing required fields - -- **WHEN** a client sends a POST missing `orgId`, `repositoryUrl`, or `prompt` -- **THEN** the server SHALL return HTTP 400 with `{ error: "validation_error", details: string[] }` - -#### Scenario: Unauthenticated request - -- **WHEN** a client sends a POST without a valid `Authorization: Bearer ` header -- **THEN** the server SHALL return HTTP 401 - -#### Scenario: Repository not accessible - -- **WHEN** the `repositoryUrl` is not accessible to the specified organization -- **THEN** the server SHALL return HTTP 403 with `{ error: "repository_not_accessible" }` - -#### Scenario: Organization not found - -- **WHEN** the `orgId` does not match a known organization -- **THEN** the server SHALL return HTTP 404 with `{ error: "organization_not_found" }` - -### Requirement: Iterate endpoint accepts guidance and returns a requestId - -The server SHALL expose `POST /cli/api/rule/{ruleId}/iterate` that accepts an authenticated request with a JSON body containing `orgId` (number, required), `guidance` (string, required), and `references` (array of `{ filename: string, content: string }`, optional). The endpoint SHALL return a JSON response containing `requestId` (string) and `status` set to `"accepted"`. The `requestId` SHALL be usable with the existing `GET /cli/api/rule/{requestId}` polling endpoint. - -#### Scenario: Valid iterate request returns a requestId - -- **WHEN** an authenticated client sends a POST to `/cli/api/rule/{ruleId}/iterate` with valid `orgId` and `guidance` -- **THEN** the server SHALL return HTTP 200 with `{ requestId: string, status: "accepted" }` - -#### Scenario: Missing required fields - -- **WHEN** a client sends a POST missing `orgId` or `guidance` -- **THEN** the server SHALL return HTTP 400 with `{ error: "validation_error", details: string[] }` - -#### Scenario: Rule not found - -- **WHEN** the `ruleId` does not match a known rule generation request -- **THEN** the server SHALL return HTTP 404 with `{ error: "request_not_found" }` - -#### Scenario: Access denied - -- **WHEN** the authenticated user does not have access to the specified rule -- **THEN** the server SHALL return HTTP 403 with `{ error: "access_denied" }` - -#### Scenario: Organization not found - -- **WHEN** the `orgId` does not match a known organization -- **THEN** the server SHALL return HTTP 404 with `{ error: "organization_not_found" }` - -### Requirement: Request status endpoint returns generation progress - -The server SHALL expose `GET /cli/api/request/:requestId` that accepts an authenticated request and returns the current status of the rule generation job. The status SHALL progress through `accepted` → `building` → `generated` (or `failed`). - -#### Scenario: Generation accepted - -- **WHEN** the rule generation job has been queued but not started -- **THEN** the server SHALL return `{ requestId, status: "accepted" }` - -#### Scenario: Generation building - -- **WHEN** the rule generation job is actively processing -- **THEN** the server SHALL return `{ requestId, status: "building" }` - -#### Scenario: Generation complete - -- **WHEN** the rule generation job has completed successfully -- **THEN** the server SHALL return `{ requestId, status: "generated", rules: GeneratedRule[] }` - -#### Scenario: Generation failed - -- **WHEN** the rule generation job has failed -- **THEN** the server SHALL return `{ requestId, status: "failed", error: string }` - -#### Scenario: Unknown requestId - -- **WHEN** a client requests a requestId that does not exist -- **THEN** the server SHALL return HTTP 404 with `{ error: "request_not_found" }` - -#### Scenario: Access denied - -- **WHEN** the authenticated user does not have access to the specified request -- **THEN** the server SHALL return HTTP 403 with `{ error: "access_denied" }` - -#### Scenario: Unauthenticated request - -- **WHEN** a client sends a GET without a valid `Authorization: Bearer ` header -- **THEN** the server SHALL return HTTP 401 - -### Requirement: Generated rule content follows ast-grep schema - -Each rule in the `rules` array SHALL contain an `id` (string), a `content` object matching the ast-grep rule schema, and an optional `tests` object. The `content` object SHALL include at minimum `id` (string), `language` (string), and `rule` (object). It MAY include `severity`, `message`, `note`, `fix`, `constraints`, `utils`, `transform`, `metadata`, `files`, `ignores`, and `url`. - -#### Scenario: Minimal rule content - -- **WHEN** a rule is generated with minimal configuration -- **THEN** `content` SHALL contain `id`, `language`, and `rule` - -#### Scenario: Full rule content - -- **WHEN** a rule is generated with all optional fields -- **THEN** `content` SHALL contain all applicable fields from the ast-grep schema - -### Requirement: Generated rules may include test cases - -Each rule in the `rules` array MAY include a `tests` object. When present it SHALL contain `valid` (array of strings, code that must not trigger the rule) and `invalid` (array of strings, code that must trigger it). - -#### Scenario: Rule with test cases - -- **WHEN** the generator produces test cases for a rule -- **THEN** the rule SHALL include `tests` with non-empty `valid` and `invalid` arrays - -#### Scenario: Rule without test cases - -- **WHEN** the generator does not produce test cases -- **THEN** the `tests` field SHALL be absent or undefined - ### Requirement: Whoami endpoint returns user identity and organizations -The server SHALL expose `GET /cli/api/whoami` that accepts an authenticated request and returns the user's identity and associated organizations. +The server SHALL expose `GET /cli/api/v2/whoami` that accepts an authenticated request and returns +the user's identity and associated organizations, and the CLI SHALL call it rather than any v1 +route. #### Scenario: Authenticated user -- **WHEN** an authenticated client sends a GET to `/cli/api/whoami` -- **THEN** the server SHALL return `{ user: string, email?: string, orgs: [{ orgId: number, name: string, installationId: number }] }` +- **WHEN** an authenticated client sends a GET to `/cli/api/v2/whoami` +- **THEN** the server SHALL return `{ user: string, email?: string, orgs: [{ orgId: number, id: string, name: string, source: "github", url: string }] }` #### Scenario: Unauthenticated request @@ -524,3 +424,19 @@ Local rule authoring SHALL complete with no GitHub remote present, in all three - **WHEN** a user runs `taskless verify` or `taskless test` in any of the three no-remote populations - **THEN** the command SHALL run to completion without a GitHub precondition error + +### Requirement: Every API call is a v2 call carrying the CLI version + +Every Taskless API call the CLI makes outside `/cli/auth/*` SHALL go to a path under +`/cli/api/v2/` and SHALL carry the `x-taskless-cli-version` header with the running CLI's +version. The CLI SHALL NOT call any v1 data route. + +#### Scenario: A v1 route is never called + +- **WHEN** any command in this release talks to the Taskless API +- **THEN** the request path SHALL begin with `/cli/api/v2/` or `/cli/auth/` + +#### Scenario: The version header is always sent + +- **WHEN** the CLI calls any `/cli/api/v2/` route +- **THEN** the request SHALL carry `x-taskless-cli-version` diff --git a/openspec/specs/cli-runtime-rule-execution/spec.md b/openspec/specs/cli-runtime-rule-execution/spec.md index 50d74fac..9350a72b 100644 --- a/openspec/specs/cli-runtime-rule-execution/spec.md +++ b/openspec/specs/cli-runtime-rule-execution/spec.md @@ -151,16 +151,21 @@ toward the exit code identically to static findings. `Finding.severity` (`error` ### Requirement: Blessed runtime rules execute from the materialized run directory When a runtime rule is executed on a validated path, the CLI SHALL execute it from the -ephemeral, gitignored `.taskless/.run/` materialization of the blessed bytes, not from the -live `.taskless/rules/runtime/` tree, so the bytes executed are the exact bytes reconciliation -blessed (read-hash-execute ordering). +snapshot `check` took under `.taskless/.run/` **before** signing, not from the live +`.taskless/rules/runtime/` tree and not from a copy made after reconciliation, so the bytes +executed are exactly the bytes that were reported and judged (copy, sign, report, execute). #### Scenario: Execution uses the blessed bytes - **WHEN** a runtime rule is blessed and executed -- **THEN** the CLI SHALL invoke the `check.ts` materialized under `.taskless/.run/` +- **THEN** the CLI SHALL invoke the `check.ts` in the snapshot under `.taskless/.run/` - **AND** SHALL NOT execute a copy modified in `.taskless/rules/runtime/` after reconciliation +#### Scenario: No copy is made between the verdict and execution + +- **WHEN** a runtime rule's `check.ts` is edited in `.taskless/rules/runtime/` after the snapshot was signed and before execution +- **THEN** the executed `check.ts` SHALL be the snapshot's bytes that reconcile judged + ### Requirement: A runtime rule has exactly one executable file The CLI SHALL refuse a runtime rule whose directory contains any module file other than From fa7fabb1424647db19321607fdc1367b26b7355c Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 29 Sep 2026 15:38:03 -0700 Subject: [PATCH 06/10] test(check): take the converging-link snapshots through a run directory --- packages/cli/test/check-snapshot.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/cli/test/check-snapshot.test.ts b/packages/cli/test/check-snapshot.test.ts index 019b6a6c..641c8517 100644 --- a/packages/cli/test/check-snapshot.test.ts +++ b/packages/cli/test/check-snapshot.test.ts @@ -135,7 +135,7 @@ describe("the check snapshot", () => { await writeFile(join(shared, "helper.yml"), "x: 1\n"); await symlink(shared, join(rules(), "sg", "no-eval-3fa9c21b", "a")); await symlink(shared, join(rules(), "sg", "no-eval-3fa9c21b", "b")); - const report = await reportRules(await takeSnapshot(cwd)); + const report = await reportRules(await snap(cwd)); expect(report.rules[0]?.files.map((file) => file.path)).toEqual([ "a/helper.yml", "b/helper.yml", @@ -148,7 +148,7 @@ describe("the check snapshot", () => { join(rules(), "sg", "no-eval-3fa9c21b"), join(rules(), "sg", "no-eval-3fa9c21b", "loop") ); - const report = await reportRules(await takeSnapshot(cwd)); + const report = await reportRules(await snap(cwd)); expect(report.rules[0]?.files.map((file) => file.path)).toEqual([ "no-eval-3fa9c21b.yml", ]); From 67d55809a7b82dd788633a3e23cbf4d1d6fa2f3f Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 29 Sep 2026 15:39:28 -0700 Subject: [PATCH 07/10] fix(check): flush a preserved run's logs before re-raising a signal, and never throw from close onSignal re-raised at once, before queued RunLog appends landed: a preserved run interrupted by Ctrl-C kept 1 of 52 engine entries. It now flushes first, bounded by SIGNAL_FLUSH_MS. close() swallowed only a missing path; an EBUSY or EPERM from rm escaped a bare finally after the run's output was written. --- packages/cli/src/rules/run-directory.ts | 52 ++++++++++++++++-- packages/cli/test/run-directory.test.ts | 73 +++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/rules/run-directory.ts b/packages/cli/src/rules/run-directory.ts index 77cfe39a..ab4fa52b 100644 --- a/packages/cli/src/rules/run-directory.ts +++ b/packages/cli/src/rules/run-directory.ts @@ -56,6 +56,12 @@ const RUN_ID = /^\d{8}T\d{6}Z-[0-9a-f]{6}$/; */ const OWNERLESS_GRACE_MS = 60_000; +/** + * How long a signal waits for a preserved run's logs to reach the disk before + * re-raising anyway. + */ +const SIGNAL_FLUSH_MS = 2000; + /** Who is using a run directory, so a later run can tell if it was abandoned. */ interface Owner { pid: number; @@ -236,18 +242,43 @@ export async function openRun( // A signal skips `finally`, so the directory would outlive the run. Removed // synchronously, then the signal re-raised so the process still exits the // way the signal asked. + // + // A preserved directory is kept for its logs, and `RunLog.write` only + // queues: re-raising at once, with no handler left, terminates before the + // queued appends land. Measured: 1 of 52 engine entries survived. So a + // preserved run flushes first, bounded, since a hung disk must not turn + // Ctrl-C into a hang. A second signal meanwhile finds no handler and + // terminates at once, which is what pressing it twice should do. const onSignal = (signal: NodeJS.Signals): void => { - if (!closed && !preserve) { + const wasClosed = closed; + closed = true; + process.off("SIGINT", onSignal); + process.off("SIGTERM", onSignal); + const reraise = (): void => { + process.kill(process.pid, signal); + }; + if (wasClosed) { + reraise(); + return; + } + if (!preserve) { try { rmSync(path, { recursive: true, force: true }); } catch { // The next run's sweep removes it. } + reraise(); + return; } - closed = true; - process.off("SIGINT", onSignal); - process.off("SIGTERM", onSignal); - process.kill(process.pid, signal); + logs.engine.write(`run interrupted by ${signal}; directory preserved`); + void Promise.race([ + Promise.all( + [logs.engine, logs.sg, logs.vale, logs.runtime].map((log) => + log.flush() + ) + ), + new Promise((resolve) => setTimeout(resolve, SIGNAL_FLUSH_MS).unref()), + ]).finally(reraise); }; process.once("SIGINT", onSignal); process.once("SIGTERM", onSignal); @@ -270,7 +301,16 @@ export async function openRun( log.flush() ) ); - if (!preserve) await rm(path, { recursive: true, force: true }); + if (preserve) return; + // `close` runs in a `finally` after the run's output is written, so a + // throw here would replace a finished run's exit code with a crash. + // `force` only forgives a missing path; EBUSY or EPERM from a child + // still letting go of a file must be forgiven too. + try { + await rm(path, { recursive: true, force: true }); + } catch { + // The next run's sweep removes it. + } }, }; } diff --git a/packages/cli/test/run-directory.test.ts b/packages/cli/test/run-directory.test.ts index e2a78960..07f9f43a 100644 --- a/packages/cli/test/run-directory.test.ts +++ b/packages/cli/test/run-directory.test.ts @@ -1,6 +1,7 @@ import { execFile, spawnSync } from "node:child_process"; import { existsSync } from "node:fs"; import { + chmod, cp, mkdir, mkdtemp, @@ -20,6 +21,13 @@ import { migrateFixture } from "./support/current-project"; const execFileAsync = promisify(execFile); const binPath = resolve(import.meta.dirname, "../dist/index.js"); +// `--import tsx` rather than the `tsx` binary, which relays a signal death as +// an exit code and so hides the signal a test asserts on. +const packageRoot = resolve(import.meta.dirname, ".."); +const runDirectorySource = resolve( + import.meta.dirname, + "../src/rules/run-directory.ts" +); /** A pid that certainly belonged to a process that has exited. */ function deadPid(): number { @@ -161,6 +169,71 @@ describe("run directories", () => { await utimes(orphan, old, old); expect(await sweepAbandonedRuns(cwd)).toEqual(["20200101T000000Z-dddddd"]); }); + + it("close does not throw when the directory cannot be removed", async () => { + const run = await openRun(cwd); + const root = join(cwd, ".taskless", ".run"); + // A read-only parent makes the removal fail with something other than + // ENOENT, which `force` does not forgive. + await chmod(root, 0o555); + try { + await expect(run.close()).resolves.toBeUndefined(); + } finally { + await chmod(root, 0o755); + } + expect(existsSync(run.path)).toBe(true); + }); + + it("a signal flushes a preserved run's logs before the process exits", async () => { + // Every entry is queued and none awaited, then SIGINT: without a flush + // the default disposition terminates before the appends land. + const script = join(cwd, "interrupt.mts"); + await writeFile( + script, + [ + `import { openRun } from ${JSON.stringify(runDirectorySource)};`, + `const run = await openRun(process.argv[2], { preserve: true });`, + `for (let i = 0; i < 50; i++) run.logs.engine.write("entry " + i);`, + `run.logs.sg.write("sg line");`, + `process.kill(process.pid, "SIGINT");`, + `setTimeout(() => {}, 10_000);`, + ].join("\n") + ); + const child = spawnSync( + process.execPath, + ["--import", "tsx", script, cwd], + { cwd: packageRoot, encoding: "utf8" } + ); + expect(child.signal, child.stderr).toBe("SIGINT"); + const [id] = await runDirectories(cwd); + const directory = join(cwd, ".taskless", ".run", id ?? ""); + const engine = await readFile(join(directory, "engine.log"), "utf8"); + expect(engine).toContain("entry 49"); + expect(engine).toContain("run interrupted by SIGINT; directory preserved"); + expect(await readFile(join(directory, "sg.log"), "utf8")).toContain( + "sg line" + ); + }); + + it("a signal removes an unpreserved run's directory", async () => { + const script = join(cwd, "interrupt.mts"); + await writeFile( + script, + [ + `import { openRun } from ${JSON.stringify(runDirectorySource)};`, + `await openRun(process.argv[2]);`, + `process.kill(process.pid, "SIGTERM");`, + `setTimeout(() => {}, 10_000);`, + ].join("\n") + ); + const child = spawnSync( + process.execPath, + ["--import", "tsx", script, cwd], + { cwd: packageRoot, encoding: "utf8" } + ); + expect(child.signal, child.stderr).toBe("SIGTERM"); + expect(await runDirectories(cwd)).toEqual([]); + }); }); describe("check and its run directory", () => { From 8594687bc5da0928ab33023514d5ee0eb3d2c802 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 29 Sep 2026 15:45:11 -0700 Subject: [PATCH 08/10] fix(check): sweep any run directory a day old, whatever its owner Liveness stays the test, but it compares only host and pid: a dead run's pid recycled by an unrelated process kept its directory forever, as did a foreign host that never returned. A run that started more than 24 hours ago is now swept regardless. The cli-check spec drops "age SHALL NOT be the test" for this one backstop. --- openspec/specs/cli-check/spec.md | 10 +++++- packages/cli/src/rules/run-directory.ts | 48 +++++++++++++++++++++---- packages/cli/test/run-directory.test.ts | 21 +++++++++++ 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/openspec/specs/cli-check/spec.md b/openspec/specs/cli-check/spec.md index 1b08a369..84ca20a5 100644 --- a/openspec/specs/cli-check/spec.md +++ b/openspec/specs/cli-check/spec.md @@ -640,7 +640,9 @@ At the start of every run, the CLI SHALL remove each directory under `.taskless/ `owner` names a process on this host that is no longer alive, and each directory with no `owner` record (left by an earlier version). It SHALL NOT remove a directory whose owning process is alive, or one owned by another host, since this host cannot tell whether that -process lives. A directory's age SHALL NOT be the test. +process lives. Liveness SHALL be the test, not age, with one backstop: +a directory whose run started more than 24 hours ago SHALL be removed whatever its owner. That +covers a dead run's process id recycled by an unrelated process, and a host that never returns. #### Scenario: A killed run's directory is swept @@ -650,8 +652,14 @@ process lives. A directory's age SHALL NOT be the test. #### Scenario: A live run is never swept - **WHEN** a run directory's owning process is still running, or it is owned by another host +- **AND** its run started less than 24 hours ago - **THEN** no other run SHALL remove it +#### Scenario: A day-old directory is swept whatever its owner + +- **WHEN** a run directory's run started more than 24 hours ago +- **THEN** the next run SHALL remove it, even if its process id names a live process or it is owned by another host + ### Requirement: Check reports rule integrity under --json Under `--json`, `taskless check` SHALL carry an additive, optional `integrity` array with one diff --git a/packages/cli/src/rules/run-directory.ts b/packages/cli/src/rules/run-directory.ts index ab4fa52b..930c987d 100644 --- a/packages/cli/src/rules/run-directory.ts +++ b/packages/cli/src/rules/run-directory.ts @@ -30,10 +30,16 @@ import process from "node:process"; * * **Abandoned directories are swept** at the start of every run: a run killed * with SIGKILL never gets to clean up. A directory is abandoned when the - * process named in its `owner` file is gone on this host. Age is not the test, - * because it would either delete a slow run still in progress or keep junk for - * hours. A directory owned by another host (a shared filesystem) is left - * alone, since this host cannot tell whether that process lives. A directory + * process named in its `owner` file is gone on this host. Liveness is the + * test, not age, which alone would either delete a slow run still in progress + * or keep junk for hours. A directory owned by another host (a shared + * filesystem) is left alone, since this host cannot tell whether that process + * lives. + * + * **Age is only a backstop.** Any owned directory whose run started more than + * {@link ABANDONED_AFTER_MS} ago is swept whatever its owner. That covers what + * liveness cannot: a dead run's pid recycled by an unrelated process, a + * foreign host that never came back. No `check` runs for a day. A directory * whose name is not a run id predates run ids (0.11's `runtime-rules/`, the * first 0.12 `snapshot/`) and is swept too. A run-id directory with no `owner` * is swept only after a grace period, because that is also what a run looks @@ -56,6 +62,12 @@ const RUN_ID = /^\d{8}T\d{6}Z-[0-9a-f]{6}$/; */ const OWNERLESS_GRACE_MS = 60_000; +/** + * How old a run may be before its directory is swept whatever its owner: live + * pid or other host. See the module comment. + */ +const ABANDONED_AFTER_MS = 24 * 60 * 60 * 1000; + /** * How long a signal waits for a preserved run's logs to reach the disk before * re-raising anyway. @@ -150,6 +162,21 @@ async function readOwner(directory: string): Promise { } } +/** + * Whether the run that owns `directory` started more than `ms` ago, from the + * owner's `startedAt`, or the directory's own mtime when that does not parse. + */ +async function ownerOlderThan( + owner: Owner, + directory: string, + ms: number +): Promise { + const started = Date.parse(owner.startedAt); + return Number.isNaN(started) + ? olderThan(directory, ms) + : Date.now() - started > ms; +} + async function olderThan(path: string, ms: number): Promise { try { const { mtimeMs } = await stat(path); @@ -178,8 +205,15 @@ export async function sweepAbandonedRuns(cwd: string): Promise { const directory = join(root, entry.name); const owner = await readOwner(directory); if (owner !== undefined) { - if (owner.hostname !== hostname()) continue; - if (isAlive(owner.pid)) continue; + const expired = await ownerOlderThan( + owner, + directory, + ABANDONED_AFTER_MS + ); + if (!expired) { + if (owner.hostname !== hostname()) continue; + if (isAlive(owner.pid)) continue; + } } else if ( // A run id with no owner yet is most likely a run starting right now. RUN_ID.test(entry.name) && @@ -214,6 +248,7 @@ export async function openRun( () => {} ); + const preserve = options.preserve === true; const now = new Date(); const id = newRunId(now); const path = join(root, id); @@ -236,7 +271,6 @@ export async function openRun( logs.engine.write(`swept abandoned run directories: ${swept.join(", ")}`); } - const preserve = options.preserve === true; let closed = false; // A signal skips `finally`, so the directory would outlive the run. Removed diff --git a/packages/cli/test/run-directory.test.ts b/packages/cli/test/run-directory.test.ts index 07f9f43a..914ff0cc 100644 --- a/packages/cli/test/run-directory.test.ts +++ b/packages/cli/test/run-directory.test.ts @@ -132,6 +132,27 @@ describe("run directories", () => { } }); + it("sweeps any owned directory a day old: live pid or other host", async () => { + const dayAgo = new Date(Date.now() - 25 * 60 * 60 * 1000).toISOString(); + const owners = { + "20200101T000000Z-cccccc": { pid: process.pid, hostname: hostname() }, + "20200101T000000Z-dddddd": { + pid: deadPid(), + hostname: "some-other-host", + }, + }; + for (const [name, owner] of Object.entries(owners)) { + const directory = join(cwd, ".taskless", ".run", name); + await mkdir(directory, { recursive: true }); + await writeFile( + join(directory, "owner"), + JSON.stringify({ ...owner, startedAt: dayAgo }) + ); + } + const swept = await sweepAbandonedRuns(cwd); + expect(swept.toSorted()).toEqual(Object.keys(owners)); + }); + it("sweeps the ownerless directories earlier versions left", async () => { for (const legacy of ["runtime-rules", "snapshot"]) { await mkdir(join(cwd, ".taskless", ".run", legacy, "x"), { From 1ce848d3b8e259522ee58e3d71d1fb78941800df Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 29 Sep 2026 15:48:27 -0700 Subject: [PATCH 09/10] fix(check): keep a --preserve-logs directory past the next run A preserved run's owner exits by design, so the next run's sweep deleted it as abandoned: an editor's on-save check wiped what --preserve-logs kept within seconds. --preserve-logs now writes a preserve marker beside owner, and the sweep keeps a marked directory until the day-old backstop. owner still answers whether the run is alive; the marker answers whether the directory outlives it. Deleting the marker releases the directory to the next sweep. --- openspec/specs/cli-check/spec.md | 26 ++++++++++++++----- packages/cli/src/agent/check.md | 4 ++- packages/cli/src/rules/run-directory.ts | 33 ++++++++++++++++++++++--- packages/cli/test/run-directory.test.ts | 33 ++++++++++++++++++++++--- 4 files changed, 83 insertions(+), 13 deletions(-) diff --git a/openspec/specs/cli-check/spec.md b/openspec/specs/cli-check/spec.md index 84ca20a5..4383c596 100644 --- a/openspec/specs/cli-check/spec.md +++ b/openspec/specs/cli-check/spec.md @@ -621,13 +621,26 @@ that no run rewrites a tracked file. `rule restore` SHALL take its snapshot the instead of removing it: the snapshot that ran, the assembled configs, the `owner` record, and the logs. Human output SHALL name the kept directory on stderr. Under `--json`, the output SHALL carry an additive, optional `runDirectory` field, the directory's path relative to the -project root, present only when the flag is set. +project root, present only when the flag is set. A kept directory SHALL hold a `preserve` marker +beside its `owner` record, and SHALL survive later runs while the marker is there, until it is +swept for age (see "Abandoned run directories are swept"). Deleting the marker SHALL release the +directory to the next run's sweep. #### Scenario: A preserved run is named and complete - **WHEN** a user runs `taskless check --json --preserve-logs` - **THEN** stdout SHALL include `runDirectory` -- **AND** that directory SHALL hold `engine.log`, `sg.log`, `vale.log`, `runtime.log`, `owner`, and the snapshot +- **AND** that directory SHALL hold `engine.log`, `sg.log`, `vale.log`, `runtime.log`, `owner`, `preserve`, and the snapshot + +#### Scenario: A preserved run survives the next run + +- **WHEN** a `taskless check --preserve-logs` run has ended and another `taskless check` starts +- **THEN** the kept run directory SHALL still exist + +#### Scenario: Deleting the marker releases a kept directory + +- **WHEN** the `preserve` marker is deleted from a kept run directory whose run has ended +- **THEN** the next run SHALL remove the directory #### Scenario: A preserved authenticated run holds no credential @@ -639,10 +652,11 @@ project root, present only when the flag is set. At the start of every run, the CLI SHALL remove each directory under `.taskless/.run/` whose `owner` names a process on this host that is no longer alive, and each directory with no `owner` record (left by an earlier version). It SHALL NOT remove a directory whose owning -process is alive, or one owned by another host, since this host cannot tell whether that -process lives. Liveness SHALL be the test, not age, with one backstop: +process is alive, one owned by another host, since this host cannot tell whether that process +lives, or one holding a `preserve` marker. Liveness SHALL be the test, not age, with one backstop: a directory whose run started more than 24 hours ago SHALL be removed whatever its owner. That -covers a dead run's process id recycled by an unrelated process, and a host that never returns. +covers a dead run's process id recycled by an unrelated process, a host that never returns, and +a kept directory nobody went back to. #### Scenario: A killed run's directory is swept @@ -658,7 +672,7 @@ covers a dead run's process id recycled by an unrelated process, and a host that #### Scenario: A day-old directory is swept whatever its owner - **WHEN** a run directory's run started more than 24 hours ago -- **THEN** the next run SHALL remove it, even if its process id names a live process or it is owned by another host +- **THEN** the next run SHALL remove it, even if its process id names a live process, it is owned by another host, or it was kept by `--preserve-logs` ### Requirement: Check reports rule integrity under --json diff --git a/packages/cli/src/agent/check.md b/packages/cli/src/agent/check.md index 1367ad00..d38fb016 100644 --- a/packages/cli/src/agent/check.md +++ b/packages/cli/src/agent/check.md @@ -117,7 +117,9 @@ delete the rules to make `check` pass, and do not suggest `engine.log`, `sg.log`, `vale.log`, `runtime.log`) instead of removing it. Its path is printed on stderr, or returned as `runDirectory` under `--json`. Use it to debug a rule that behaves unexpectedly; the logs hold - matched source, so treat the directory like any local build output. + matched source, so treat the directory like any local build output. It + survives later runs and is removed after a day; delete its `preserve` + file to let the next run remove it sooner. ## Steps diff --git a/packages/cli/src/rules/run-directory.ts b/packages/cli/src/rules/run-directory.ts index 930c987d..36453bfd 100644 --- a/packages/cli/src/rules/run-directory.ts +++ b/packages/cli/src/rules/run-directory.ts @@ -34,12 +34,18 @@ import process from "node:process"; * test, not age, which alone would either delete a slow run still in progress * or keep junk for hours. A directory owned by another host (a shared * filesystem) is left alone, since this host cannot tell whether that process - * lives. + * lives. A preserved directory is left alone too: `--preserve-logs` writes a + * `preserve` marker beside `owner`, because its owner exits by design and the + * next on-save `check` must not delete what the user asked to keep. The two + * files answer different questions (`owner`: is the run still going; + * `preserve`: should the directory outlive it), and deleting the marker is how + * a user releases a kept directory to the next sweep. * * **Age is only a backstop.** Any owned directory whose run started more than * {@link ABANDONED_AFTER_MS} ago is swept whatever its owner. That covers what * liveness cannot: a dead run's pid recycled by an unrelated process, a - * foreign host that never came back. No `check` runs for a day. A directory + * foreign host that never came back, and a preserved directory nobody went + * back to. No `check` runs for a day. A directory * whose name is not a run id predates run ids (0.11's `runtime-rules/`, the * first 0.12 `snapshot/`) and is swept too. A run-id directory with no `owner` * is swept only after a grace period, because that is also what a run looks @@ -64,7 +70,7 @@ const OWNERLESS_GRACE_MS = 60_000; /** * How old a run may be before its directory is swept whatever its owner: live - * pid or other host. See the module comment. + * pid, other host, or preserved. See the module comment. */ const ABANDONED_AFTER_MS = 24 * 60 * 60 * 1000; @@ -81,6 +87,9 @@ interface Owner { startedAt: string; } +/** The marker `--preserve-logs` writes beside `owner`. */ +const PRESERVE_MARKER = "preserve"; + /** One append-only log file in a run directory. */ export class RunLog { private pending: Promise = Promise.resolve(); @@ -177,6 +186,15 @@ async function ownerOlderThan( : Date.now() - started > ms; } +async function exists(path: string): Promise { + try { + await stat(path); + return true; + } catch { + return false; + } +} + async function olderThan(path: string, ms: number): Promise { try { const { mtimeMs } = await stat(path); @@ -211,6 +229,7 @@ export async function sweepAbandonedRuns(cwd: string): Promise { ABANDONED_AFTER_MS ); if (!expired) { + if (await exists(join(directory, PRESERVE_MARKER))) continue; if (owner.hostname !== hostname()) continue; if (isAlive(owner.pid)) continue; } @@ -258,6 +277,14 @@ export async function openRun( hostname: hostname(), startedAt: now.toISOString(), }; + if (preserve) { + // Before `owner`, so no sweep can see this run owned but unmarked. + await writeFile( + join(path, PRESERVE_MARKER), + "Kept by --preserve-logs. A run sweeps this directory once it is a day old; delete this file to let the next run sweep it sooner.\n", + "utf8" + ); + } await writeFile(join(path, "owner"), `${JSON.stringify(owner)}\n`, "utf8"); const logs: RunLogs = { diff --git a/packages/cli/test/run-directory.test.ts b/packages/cli/test/run-directory.test.ts index 914ff0cc..a844d386 100644 --- a/packages/cli/test/run-directory.test.ts +++ b/packages/cli/test/run-directory.test.ts @@ -132,7 +132,7 @@ describe("run directories", () => { } }); - it("sweeps any owned directory a day old: live pid or other host", async () => { + it("sweeps any owned directory a day old: live pid, other host, or preserved", async () => { const dayAgo = new Date(Date.now() - 25 * 60 * 60 * 1000).toISOString(); const owners = { "20200101T000000Z-cccccc": { pid: process.pid, hostname: hostname() }, @@ -140,6 +140,7 @@ describe("run directories", () => { pid: deadPid(), hostname: "some-other-host", }, + "20200101T000000Z-eeeeee": { pid: deadPid(), hostname: hostname() }, }; for (const [name, owner] of Object.entries(owners)) { const directory = join(cwd, ".taskless", ".run", name); @@ -149,10 +150,32 @@ describe("run directories", () => { JSON.stringify({ ...owner, startedAt: dayAgo }) ); } + await writeFile( + join(cwd, ".taskless", ".run", "20200101T000000Z-eeeeee", "preserve"), + "" + ); const swept = await sweepAbandonedRuns(cwd); expect(swept.toSorted()).toEqual(Object.keys(owners)); }); + it("keeps a dead run's directory while its preserve marker is there", async () => { + const kept = join(cwd, ".taskless", ".run", "20200101T000000Z-ffffff"); + await mkdir(kept, { recursive: true }); + await writeFile( + join(kept, "owner"), + JSON.stringify({ + pid: deadPid(), + hostname: hostname(), + startedAt: new Date().toISOString(), + }) + ); + await writeFile(join(kept, "preserve"), ""); + expect(await sweepAbandonedRuns(cwd)).toEqual([]); + // Deleting the marker releases it to the next sweep. + await rm(join(kept, "preserve")); + expect(await sweepAbandonedRuns(cwd)).toEqual(["20200101T000000Z-ffffff"]); + }); + it("sweeps the ownerless directories earlier versions left", async () => { for (const legacy of ["runtime-rules", "snapshot"]) { await mkdir(join(cwd, ".taskless", ".run", legacy, "x"), { @@ -324,6 +347,7 @@ describe("check and its run directory", () => { "sg.log", "vale.log", "owner", + "preserve", "snapshot", ]) { expect(files).toContain(name); @@ -334,8 +358,11 @@ describe("check and its run directory", () => { expect(await readFile(join(directory, "engine.log"), "utf8")).toContain( "unverified run" ); - // The next run sweeps it only once its owner is gone, which it is. + // Its owner has exited, but it was kept on purpose: the next run, such as + // an editor's on-save `check`, must not delete it. It goes after a day. await check(); - expect(await runDirectories(cwd)).toEqual([]); + expect(await runDirectories(cwd)).toEqual([ + output.runDirectory?.split("/").at(-1), + ]); }); }); From ca358c93e940f7d90e21d52de3c7bc68bd2f1af4 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 29 Sep 2026 16:19:32 -0700 Subject: [PATCH 10/10] fix(check): time run directories in unix ms, give the preserve marker its own keepUntil, and create every log up front - owner.startedAt is unix milliseconds. A well-formed owner whose start time is not a number counts as past the day-old backstop; a half-written one still counts as absent, so a starting run keeps its grace. Nothing an owned directory's age depends on reads a filesystem timestamp any more. - The preserve marker records keepUntil (start + 24h). A kept directory's lifetime depends only on that file: raising it keeps the directory past the backstop, and a marker that is deleted, expired, or unreadable holds nothing. - All four logs are created when a run opens, so a kept directory always holds runtime.log, as the spec says, even when no runtime rule ran. --- openspec/specs/cli-check/spec.md | 35 +++-- packages/cli/src/agent/check.md | 5 +- packages/cli/src/rules/run-directory.ts | 100 +++++++++----- packages/cli/test/run-directory.test.ts | 168 ++++++++++++++++++------ 4 files changed, 231 insertions(+), 77 deletions(-) diff --git a/openspec/specs/cli-check/spec.md b/openspec/specs/cli-check/spec.md index 4383c596..b0688867 100644 --- a/openspec/specs/cli-check/spec.md +++ b/openspec/specs/cli-check/spec.md @@ -622,9 +622,11 @@ instead of removing it: the snapshot that ran, the assembled configs, the `owner the logs. Human output SHALL name the kept directory on stderr. Under `--json`, the output SHALL carry an additive, optional `runDirectory` field, the directory's path relative to the project root, present only when the flag is set. A kept directory SHALL hold a `preserve` marker -beside its `owner` record, and SHALL survive later runs while the marker is there, until it is -swept for age (see "Abandoned run directories are swept"). Deleting the marker SHALL release the -directory to the next run's sweep. +beside its `owner` record, recording `keepUntil` in unix milliseconds, set to the run's start plus +24 hours. The directory SHALL survive later runs while the current time is before `keepUntil`, +whatever its owner or age, so raising `keepUntil` keeps it longer. A marker that is deleted, +past its `keepUntil`, or unreadable SHALL hold nothing, and the directory SHALL fall to the +ordinary sweep (see "Abandoned run directories are swept"). #### Scenario: A preserved run is named and complete @@ -642,6 +644,18 @@ directory to the next run's sweep. - **WHEN** the `preserve` marker is deleted from a kept run directory whose run has ended - **THEN** the next run SHALL remove the directory +#### Scenario: A raised keepUntil outlasts the day + +- **WHEN** a kept run directory's run started more than 24 hours ago +- **AND** its marker's `keepUntil` is still in the future +- **THEN** the next run SHALL NOT remove it + +#### Scenario: An unreadable marker holds nothing + +- **WHEN** a kept run directory's `preserve` marker does not parse, or its `keepUntil` is not a number +- **AND** its run has ended +- **THEN** the next run SHALL remove the directory + #### Scenario: A preserved authenticated run holds no credential - **WHEN** an authenticated `check --preserve-logs` reconciles @@ -653,10 +667,14 @@ At the start of every run, the CLI SHALL remove each directory under `.taskless/ `owner` names a process on this host that is no longer alive, and each directory with no `owner` record (left by an earlier version). It SHALL NOT remove a directory whose owning process is alive, one owned by another host, since this host cannot tell whether that process -lives, or one holding a `preserve` marker. Liveness SHALL be the test, not age, with one backstop: -a directory whose run started more than 24 hours ago SHALL be removed whatever its owner. That -covers a dead run's process id recycled by an unrelated process, a host that never returns, and -a kept directory nobody went back to. +lives, or one whose `preserve` marker still holds it. Liveness SHALL be the test, not age, with one +backstop: a directory whose run started more than 24 hours ago SHALL be removed whatever its +owner, unless its `preserve` marker still holds it. That covers a dead run's process id recycled +by an unrelated process, and a host that never returns. A run's start time SHALL be read from its +`owner` record, as unix milliseconds, and never from a filesystem timestamp; a well-formed +`owner` record whose start time is not a number SHALL count as past the backstop. An `owner` +record that does not parse SHALL count as absent, since a run writes it just after creating its +directory. #### Scenario: A killed run's directory is swept @@ -672,7 +690,8 @@ a kept directory nobody went back to. #### Scenario: A day-old directory is swept whatever its owner - **WHEN** a run directory's run started more than 24 hours ago -- **THEN** the next run SHALL remove it, even if its process id names a live process, it is owned by another host, or it was kept by `--preserve-logs` +- **AND** its `preserve` marker, if any, no longer holds it +- **THEN** the next run SHALL remove it, even if its process id names a live process or it is owned by another host ### Requirement: Check reports rule integrity under --json diff --git a/packages/cli/src/agent/check.md b/packages/cli/src/agent/check.md index d38fb016..99977da6 100644 --- a/packages/cli/src/agent/check.md +++ b/packages/cli/src/agent/check.md @@ -118,8 +118,9 @@ delete the rules to make `check` pass, and do not suggest Its path is printed on stderr, or returned as `runDirectory` under `--json`. Use it to debug a rule that behaves unexpectedly; the logs hold matched source, so treat the directory like any local build output. It - survives later runs and is removed after a day; delete its `preserve` - file to let the next run remove it sooner. + survives later runs until the `keepUntil` in its `preserve` file, a + day out in unix milliseconds. Raise it to keep the directory longer, or + delete the file to let the next run remove it. ## Steps diff --git a/packages/cli/src/rules/run-directory.ts b/packages/cli/src/rules/run-directory.ts index 36453bfd..4c864538 100644 --- a/packages/cli/src/rules/run-directory.ts +++ b/packages/cli/src/rules/run-directory.ts @@ -13,6 +13,8 @@ import { hostname } from "node:os"; import { join, relative } from "node:path"; import process from "node:process"; +import { isRecord } from "../util/is-record"; + /** * The transient verification space one `check` (or `rule restore`) works in: * `.taskless/.run//`. @@ -43,9 +45,12 @@ import process from "node:process"; * * **Age is only a backstop.** Any owned directory whose run started more than * {@link ABANDONED_AFTER_MS} ago is swept whatever its owner. That covers what - * liveness cannot: a dead run's pid recycled by an unrelated process, a - * foreign host that never came back, and a preserved directory nobody went - * back to. No `check` runs for a day. A directory + * liveness cannot: a dead run's pid recycled by an unrelated process, and a + * foreign host that never came back. No `check` runs for a day. Every time is + * unix milliseconds recorded by the run itself, never a filesystem timestamp, + * which copies, checkouts, and backups rewrite. A preserved directory is kept + * by its marker's own `keepUntil`, which `--preserve-logs` sets to the same day + * and a user may raise. A directory * whose name is not a run id predates run ids (0.11's `runtime-rules/`, the * first 0.12 `snapshot/`) and is swept too. A run-id directory with no `owner` * is swept only after a grace period, because that is also what a run looks @@ -84,12 +89,24 @@ const SIGNAL_FLUSH_MS = 2000; interface Owner { pid: number; hostname: string; - startedAt: string; + /** Unix milliseconds. A number, not a date string, so reading it cannot fail softly. */ + startedAt: number; } /** The marker `--preserve-logs` writes beside `owner`. */ const PRESERVE_MARKER = "preserve"; +/** + * What the marker holds. `keepUntil` is the directory's own deadline, in unix + * milliseconds, so a kept directory's lifetime depends on nothing but this + * file: not on `owner`, and never on a filesystem timestamp. Raising it keeps + * the directory longer; deleting the file releases it. + */ +interface PreserveMarker { + keepUntil: number; + note: string; +} + /** One append-only log file in a run directory. */ export class RunLog { private pending: Promise = Promise.resolve(); @@ -158,32 +175,47 @@ function isAlive(pid: number): boolean { } } -async function readOwner(directory: string): Promise { +async function readJson(path: string): Promise> { try { - const parsed = JSON.parse( - await readFile(join(directory, "owner"), "utf8") - ) as Partial; - return typeof parsed.pid === "number" && typeof parsed.hostname === "string" - ? (parsed as Owner) - : undefined; + const parsed: unknown = JSON.parse(await readFile(path, "utf8")); + return isRecord(parsed) ? parsed : {}; } catch { + return {}; + } +} + +/** + * The `owner` record, or `undefined` when there is none or it is not a record + * yet. A run writes `owner` just after creating its directory, so a sweep can + * read it half-written; that must look ownerless (and get the grace), never + * old. A well-formed record whose `startedAt` is not a finite number reads as + * starting at the epoch, past the day-old backstop: an age that cannot be + * known is treated as old, never as new. + */ +async function readOwner(directory: string): Promise { + const parsed = await readJson(join(directory, "owner")); + if (typeof parsed.pid !== "number" || typeof parsed.hostname !== "string") { return undefined; } + return { + pid: parsed.pid, + hostname: parsed.hostname, + startedAt: Number.isFinite(parsed.startedAt) + ? (parsed.startedAt as number) + : 0, + }; } /** - * Whether the run that owns `directory` started more than `ms` ago, from the - * owner's `startedAt`, or the directory's own mtime when that does not parse. + * Whether the directory's `preserve` marker still holds it. A marker that is + * missing, unreadable, or has no numeric `keepUntil` holds nothing, and the + * directory falls back to the ordinary liveness rules. */ -async function ownerOlderThan( - owner: Owner, - directory: string, - ms: number -): Promise { - const started = Date.parse(owner.startedAt); - return Number.isNaN(started) - ? olderThan(directory, ms) - : Date.now() - started > ms; +async function isPreserved(directory: string): Promise { + const path = join(directory, PRESERVE_MARKER); + if (!(await exists(path))) return false; + const { keepUntil } = await readJson(path); + return Number.isFinite(keepUntil) && Date.now() < (keepUntil as number); } async function exists(path: string): Promise { @@ -221,15 +253,11 @@ export async function sweepAbandonedRuns(cwd: string): Promise { for (const entry of entries) { if (!entry.isDirectory()) continue; const directory = join(root, entry.name); + if (await isPreserved(directory)) continue; const owner = await readOwner(directory); if (owner !== undefined) { - const expired = await ownerOlderThan( - owner, - directory, - ABANDONED_AFTER_MS - ); + const expired = Date.now() - owner.startedAt > ABANDONED_AFTER_MS; if (!expired) { - if (await exists(join(directory, PRESERVE_MARKER))) continue; if (owner.hostname !== hostname()) continue; if (isAlive(owner.pid)) continue; } @@ -275,13 +303,17 @@ export async function openRun( const owner: Owner = { pid: process.pid, hostname: hostname(), - startedAt: now.toISOString(), + startedAt: now.getTime(), }; if (preserve) { // Before `owner`, so no sweep can see this run owned but unmarked. + const marker: PreserveMarker = { + keepUntil: now.getTime() + ABANDONED_AFTER_MS, + note: "Kept by --preserve-logs until keepUntil (unix ms). Raise it to keep this directory longer; delete this file to let the next run remove it.", + }; await writeFile( join(path, PRESERVE_MARKER), - "Kept by --preserve-logs. A run sweeps this directory once it is a day old; delete this file to let the next run sweep it sooner.\n", + `${JSON.stringify(marker, undefined, 2)}\n`, "utf8" ); } @@ -293,6 +325,14 @@ export async function openRun( vale: new RunLog(join(path, "vale.log")), runtime: new RunLog(join(path, "runtime.log")), }; + // Every log exists from the start, so a kept directory always holds all + // four, and an empty one says that engine had nothing to do rather than + // leaving the reader to wonder whether it was ever logged. + await Promise.all( + [logs.engine, logs.sg, logs.vale, logs.runtime].map(async (log) => + writeFile(log.path, "", { flag: "a" }) + ) + ); logs.engine.write(`run ${id} started (pid ${String(process.pid)})`); if (swept.length > 0) { logs.engine.write(`swept abandoned run directories: ${swept.join(", ")}`); diff --git a/packages/cli/test/run-directory.test.ts b/packages/cli/test/run-directory.test.ts index a844d386..cbaed513 100644 --- a/packages/cli/test/run-directory.test.ts +++ b/packages/cli/test/run-directory.test.ts @@ -35,6 +35,30 @@ function deadPid(): number { return child.pid ?? 0; } +const HOUR = 60 * 60 * 1000; + +/** Lay out a run directory with the given `owner` and, optionally, `preserve`. */ +async function plantRun( + cwd: string, + name: string, + owner: unknown, + preserve?: unknown +): Promise { + const directory = join(cwd, ".taskless", ".run", name); + await mkdir(directory, { recursive: true }); + await writeFile( + join(directory, "owner"), + typeof owner === "string" ? owner : JSON.stringify(owner) + ); + if (preserve !== undefined) { + await writeFile( + join(directory, "preserve"), + typeof preserve === "string" ? preserve : JSON.stringify(preserve) + ); + } + return directory; +} + async function runDirectories(cwd: string): Promise { try { const entries = await readdir(join(cwd, ".taskless", ".run"), { @@ -105,7 +129,11 @@ describe("run directories", () => { await mkdir(stale, { recursive: true }); await writeFile( join(stale, "owner"), - JSON.stringify({ pid: deadPid(), hostname: hostname(), startedAt: "x" }) + JSON.stringify({ + pid: deadPid(), + hostname: hostname(), + startedAt: Date.now(), + }) ); expect(await sweepAbandonedRuns(cwd)).toEqual(["20200101T000000Z-aaaaaa"]); expect(existsSync(stale)).toBe(false); @@ -120,7 +148,7 @@ describe("run directories", () => { JSON.stringify({ pid: deadPid(), hostname: "some-other-host", - startedAt: "x", + startedAt: Date.now(), }) ); try { @@ -132,50 +160,115 @@ describe("run directories", () => { } }); - it("sweeps any owned directory a day old: live pid, other host, or preserved", async () => { - const dayAgo = new Date(Date.now() - 25 * 60 * 60 * 1000).toISOString(); - const owners = { - "20200101T000000Z-cccccc": { pid: process.pid, hostname: hostname() }, - "20200101T000000Z-dddddd": { - pid: deadPid(), - hostname: "some-other-host", - }, - "20200101T000000Z-eeeeee": { pid: deadPid(), hostname: hostname() }, - }; - for (const [name, owner] of Object.entries(owners)) { - const directory = join(cwd, ".taskless", ".run", name); - await mkdir(directory, { recursive: true }); - await writeFile( - join(directory, "owner"), - JSON.stringify({ ...owner, startedAt: dayAgo }) - ); - } - await writeFile( - join(cwd, ".taskless", ".run", "20200101T000000Z-eeeeee", "preserve"), - "" - ); + it("sweeps any owned directory a day old, live pid or other host", async () => { + const dayAgo = Date.now() - 25 * HOUR; + await plantRun(cwd, "20200101T000000Z-cccccc", { + pid: process.pid, + hostname: hostname(), + startedAt: dayAgo, + }); + await plantRun(cwd, "20200101T000000Z-dddddd", { + pid: deadPid(), + hostname: "some-other-host", + startedAt: dayAgo, + }); const swept = await sweepAbandonedRuns(cwd); - expect(swept.toSorted()).toEqual(Object.keys(owners)); + expect(swept.toSorted()).toEqual([ + "20200101T000000Z-cccccc", + "20200101T000000Z-dddddd", + ]); }); - it("keeps a dead run's directory while its preserve marker is there", async () => { - const kept = join(cwd, ".taskless", ".run", "20200101T000000Z-ffffff"); - await mkdir(kept, { recursive: true }); - await writeFile( - join(kept, "owner"), - JSON.stringify({ - pid: deadPid(), - hostname: hostname(), - startedAt: new Date().toISOString(), - }) - ); - await writeFile(join(kept, "preserve"), ""); + it("treats an owner whose start time is not a number as a day old", async () => { + // A live pid on this host would otherwise keep it forever. + await plantRun(cwd, "20200101T000000Z-eeeeee", { + pid: process.pid, + hostname: hostname(), + startedAt: "2026-09-29T00:00:00.000Z", + }); + expect(await sweepAbandonedRuns(cwd)).toEqual(["20200101T000000Z-eeeeee"]); + }); + + it("spares a run whose owner record is still half-written", async () => { + // A run writes `owner` just after creating its directory; a sweep landing + // in between must see a run starting, not a day-old one. + await plantRun(cwd, "20200101T000000Z-e0e0e0", "{"); + expect(await sweepAbandonedRuns(cwd)).toEqual([]); + }); + + it("keeps a dead run's directory until its marker's keepUntil", async () => { + const owner = { + pid: deadPid(), + hostname: hostname(), + startedAt: Date.now(), + }; + const kept = await plantRun(cwd, "20200101T000000Z-ffffff", owner, { + keepUntil: Date.now() + HOUR, + }); expect(await sweepAbandonedRuns(cwd)).toEqual([]); // Deleting the marker releases it to the next sweep. await rm(join(kept, "preserve")); expect(await sweepAbandonedRuns(cwd)).toEqual(["20200101T000000Z-ffffff"]); }); + it("keeps a directory past the day-old backstop while keepUntil is raised", async () => { + await plantRun( + cwd, + "20200101T000000Z-a1a1a1", + { + pid: deadPid(), + hostname: hostname(), + startedAt: Date.now() - 48 * HOUR, + }, + { keepUntil: Date.now() + HOUR } + ); + expect(await sweepAbandonedRuns(cwd)).toEqual([]); + }); + + it("releases a directory whose marker has expired or cannot be read", async () => { + const owner = { + pid: deadPid(), + hostname: hostname(), + startedAt: Date.now(), + }; + await plantRun(cwd, "20200101T000000Z-b1b1b1", owner, { + keepUntil: Date.now() - 1, + }); + await plantRun(cwd, "20200101T000000Z-c1c1c1", owner, "not json"); + await plantRun(cwd, "20200101T000000Z-d1d1d1", owner, { + keepUntil: "tomorrow", + }); + const swept = await sweepAbandonedRuns(cwd); + expect(swept.toSorted()).toEqual([ + "20200101T000000Z-b1b1b1", + "20200101T000000Z-c1c1c1", + "20200101T000000Z-d1d1d1", + ]); + }); + + it("records unix-ms times, and a preserved run's deadline a day out", async () => { + const before = Date.now(); + const run = await openRun(cwd, { preserve: true }); + await run.close(); + const owner = JSON.parse( + await readFile(join(run.path, "owner"), "utf8") + ) as { startedAt: number }; + const marker = JSON.parse( + await readFile(join(run.path, "preserve"), "utf8") + ) as { keepUntil: number }; + expect(owner.startedAt).toBeGreaterThanOrEqual(before); + expect(marker.keepUntil).toBe(owner.startedAt + 24 * HOUR); + }); + + it("creates all four logs when the run opens, even ones never written", async () => { + const run = await openRun(cwd, { preserve: true }); + await run.close(); + for (const name of ["engine.log", "sg.log", "vale.log", "runtime.log"]) { + expect(existsSync(join(run.path, name))).toBe(true); + } + expect(await readFile(join(run.path, "runtime.log"), "utf8")).toBe(""); + }); + it("sweeps the ownerless directories earlier versions left", async () => { for (const legacy of ["runtime-rules", "snapshot"]) { await mkdir(join(cwd, ".taskless", ".run", legacy, "x"), { @@ -346,6 +439,7 @@ describe("check and its run directory", () => { "engine.log", "sg.log", "vale.log", + "runtime.log", "owner", "preserve", "snapshot",