Skip to content

Commit aa07b56

Browse files
committed
fix(cli): correct the delete recipe's engine claim, and test the command wiring
Three review findings, all real. The delete recipe still told agents the opposite of what the command does. The Errors table was corrected in the previous commit while the Goal and Preconditions eight lines above it still read "rule delete resolves ast-grep rules only ... A Vale or runtime rule is deleted by removing its directory by hand", and Step 1 told the reader to list `.taskless/rules/sg/`. That is the pre-fix assumption this whole change exists to remove, in agent-facing text embedded in the bundle. It is worse than stale. An agent following it would delete a rule directory by hand, which skips the `.taskless/rule-metadata/<id>.yml` cleanup the CLI performs and bypasses the ambiguity check added here. The recipe now describes cross-engine resolution, says why deleting by hand is not equivalent, and states that an id is not unique across engines. Topic bumped v3 to v4, which the previous commit should have done when it changed the same file. The ambiguous branch had no test through the command. The unit test covers what `deleteRuleFiles` returns; it cannot catch the command wiring that outcome to the wrong code, a malformed message, or a missing `process.exitCode`. Added a `runCli` test beside the existing RULE_NOT_FOUND one that seeds two engine directories, asserts `RULE_ID_AMBIGUOUS`, asserts both paths appear in the message, and asserts both directories still exist. The scan and the `rm` are not atomic, and that is now recorded as accepted rather than left to look like an oversight. Closing the window needs a lock over `.taskless/rules/`, which is a large mechanism for a local single-user operation that no command runs concurrently with itself. 1297 tests pass, typecheck and lint clean.
1 parent 31bd62d commit aa07b56

3 files changed

Lines changed: 58 additions & 12 deletions

File tree

‎packages/cli/src/agent/delete-rule.txt‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,38 @@
1-
# Topic: delete-rule (CLI v%(CLI_VERSION)s / topic v3)
1+
# Topic: delete-rule (CLI v%(CLI_VERSION)s / topic v4)
22

33
## Goal
4-
Remove an ast-grep rule and its associated test files from
5-
`.taskless/`. Does not contact the Taskless API; purely a local
6-
filesystem operation.
4+
Remove a rule and its associated test files from `.taskless/`. Does not
5+
contact the Taskless API; purely a local filesystem operation.
76

8-
`rule delete` resolves ast-grep rules only. It takes a bare id, not a
9-
path, and looks for it under `.taskless/rules/sg/`. A Vale or runtime
10-
rule is deleted by removing its directory by hand.
7+
`rule delete` takes a bare id, not a path, and resolves which engine
8+
holds it: it searches `.taskless/rules/sg/`, `vale/` and `runtime/`.
9+
Deleting the directory by hand instead skips the metadata sidecar the
10+
CLI also removes, and skips the ambiguity check below.
11+
12+
An id is not unique across engines. Nothing enforces uniqueness, so two
13+
engines can hold the same id, and the CLI refuses that case rather than
14+
picking one.
1115

1216
## Preconditions
1317
- `.taskless/` directory exists.
14-
- The target rule directory exists at `.taskless/rules/sg/<id>/`.
18+
- The target rule directory exists under `.taskless/rules/<engine>/<id>/`
19+
for exactly one engine.
1520
- No auth required.
1621

1722
## Steps
1823

1924
1. **Identify the rule.** If the user named one, use it. Otherwise,
20-
list `.taskless/rules/sg/` and ask which one. Confirm the user's
21-
intent, deletion is destructive.
25+
list the engine directories under `.taskless/rules/` and ask which
26+
rule. Confirm the user's intent, deletion is destructive.
2227

2328
2. **Invoke the CLI.** Run:
2429
```
2530
%(TASKLESS_CLI)s rule delete <id>
2631
```
2732

2833
The CLI removes:
29-
- `.taskless/rules/sg/<id>/`: the whole directory, which holds the
30-
rule, its `.tests/`, and any per-rule config
34+
- `.taskless/rules/<engine>/<id>/`: the whole directory, which holds
35+
the rule, its `.tests/`, and any per-rule config
3136
- `.taskless/rule-metadata/<id>.yml`, a sidecar the CLI never
3237
writes. The removal is there for a directory a person created by
3338
hand; expect it to be absent.

‎packages/cli/src/rules/files.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,14 @@ export async function deleteRuleFiles(
285285
// invisible while ast-grep was the only engine a rule could be delivered
286286
// for: a vale or runtime rule could be written and then not removed, and
287287
// `delete` reported "not found" for a rule plainly on disk.
288+
// The scan and the `rm` below are not atomic, and that is accepted rather
289+
// than overlooked. A second engine's directory for this id could appear
290+
// between them, and the delete would then proceed on a resolution that has
291+
// just gone stale. Closing it would need a lock over `.taskless/rules/`,
292+
// which is a large mechanism for a local single-user filesystem operation
293+
// that no CLI command runs concurrently with itself. The window is narrow
294+
// and the cost of the fix is not.
295+
//
288296
// Destructured rather than indexed so `engine` narrows to a single engine
289297
// for the rest of the function: `engines[0]` is `EngineName | undefined`
290298
// under `noUncheckedIndexedAccess`, and asserting it away here would be

‎packages/cli/test/error-envelope.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { execFile } from "node:child_process";
22
import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
3+
import { existsSync } from "node:fs";
34
import { tmpdir } from "node:os";
45
import { join, resolve } from "node:path";
56
import { promisify } from "node:util";
@@ -220,6 +221,38 @@ describe("standardized error envelope (--json)", () => {
220221
expect(result.exitCode).toBe(0);
221222
expect(result.stdout.trim()).toBe("");
222223
});
224+
225+
it("emits RULE_ID_AMBIGUOUS, and deletes nothing, when two engines hold the id", async () => {
226+
// Through the command, not through `deleteRuleFiles`. The unit test
227+
// covers the outcome the function returns; this covers whether the
228+
// command wires that outcome to the right code, message and exit status,
229+
// which a correct return value does not guarantee.
230+
const directories = ["sg", "vale"].map((engine) =>
231+
join(cwd, ".taskless", "rules", engine, "shared-id")
232+
);
233+
for (const directory of directories) {
234+
await mkdir(directory, { recursive: true });
235+
await writeFile(join(directory, "marker.txt"), "x");
236+
}
237+
238+
const result = await runCli([
239+
"rule",
240+
"delete",
241+
"shared-id",
242+
"--json",
243+
"-d",
244+
cwd,
245+
]);
246+
247+
expect(result.exitCode).not.toBe(0);
248+
const env = parseEnvelope(result.stdout);
249+
expect(env.code).toBe("RULE_ID_AMBIGUOUS");
250+
expect(env.message).toContain("shared-id");
251+
for (const directory of directories) {
252+
expect(env.message).toContain(directory);
253+
expect(existsSync(directory)).toBe(true);
254+
}
255+
});
223256
});
224257

225258
describe("auth login", () => {

0 commit comments

Comments
 (0)