fix: take an unsafe entry's rule id from the entry, not its path

[thecodedrift]

Rule repair no longer reads a rule id out of a filesystem path.

The fallback

repairTargets in packages/cli/src/rules/runtime/repair.ts parsed the id out of the reported check.ts path with ruleIdFromCheckPath, matching .taskless/rules/runtime/<id>/check.ts from the right. It existed only because a reconcile unsafe entry did not carry ruleId, and it tied repair to a filesystem layout that has already moved twice (0004, 0005). Its own comment named the failure and the exit: a further move yields a wrong id, restore 404s, and the rule stays unrepaired and unexecuted — with nothing on stdout, nothing on stderr, and nothing on the --json envelope. A rule that never fires looks exactly like a clean pass.

The evidence that retires it

GET https://app.taskless.io/cli/api/__schema (public, no auth), fetched from the deployed service:

unsafe   required = [expected, file, got, ruleId]
missing  required = [file, ruleId]
unknown  required = [file]

Our hand-written UnsafeEntry in packages/cli/src/api/reconcile.ts declared only { file, expected, got }, so ruleId was on the wire and invisible to our code.

What changed

• UnsafeEntry gains ruleId.
• repairTargets reads it off the entry for both repairable buckets, exactly as missing already did.
• Deleted: ruleIdFromCheckPath, the TASKLESS_DIRECTORY constant and RULES_DIRECTORY import it needed, the unidentifiable bucket it fed, and the check.ts notice that reported it. An entry can no longer fail to identify itself, so repairTargets returns a plain RepairTarget[] instead of { targets, unidentifiable }.
• Deleted from test/repair.test.ts: the describe("reading a rule id out of a check path") block and the unidentifiable-entry case.

Deliberately untouched:

• verifyRestoredCheck still compares restored bytes against unsafe.expected, the signature reconcile sent, not the one the restore response carries. That is what makes "restore never returns newer bytes" testable, and the test asserting it still passes.
• unknown still routes nowhere. There is nothing on the server to fetch for a file it never issued, which is why that entry alone carries no rule id.
• Nothing repaired runs in the pass that repaired it.

The disagreeing-id test

test/repair-integration.test.ts gains a case whose reconcile fixture reports ruleId: "logs-abc12345" on a file at .taskless/rules/runtime/demo/check.ts — the entry and the path name different rules. It asserts restore is called with logs-abc12345 and that the blessed bytes land under that id's directory. A path-parsing implementation asks for demo and fails it. test/repair.test.ts covers the same disagreement as a pure function, over both unsafe and missing, plus a new case for an unsafe entry whose file sits outside the rules tree entirely — repairable now, dropped as unidentifiable before.

Proven to bite. Reinstating the parse (ruleId: entry.file.split("/").at(-2) ?? entry.ruleId) fails exactly the three new assertions and nothing else: 3 failed / 14 passed. Restored, all 17 pass.

Checks

pnpm build, pnpm typecheck, pnpm lint, pnpm --filter @taskless/cli exec vitest run (1169 passed, 73 files), and pnpm cli check against a fresh build — 4 warnings, the known pre-existing no-hedging set (3 in route.txt, 1 in onboard.txt).

Fixes #240

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 2m 27s —— View job

---

Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contains no threads, reviews, or comments, so this is the first review of this PR — nothing to treat as already addressed.

• Read .prior-review.json
• Understand the request (/review on PR fix: take an unsafe entry's rule id from the entry, not its path #242)
• Fetch PR diff and metadata (gh pr diff 242, gh pr view 242)
• Review changed files for correctness, security, performance, test adequacy, clarity
• Post inline comments for concrete issues
• Post final top-level summary

Summary

This is a clean, well-scoped fix. repairTargets now reads ruleId straight off the reconcile entry for both unsafe and missing buckets instead of parsing it out of the reported check.ts path, matching how missing already worked. The path-parsing fallback (ruleIdFromCheckPath), the TASKLESS_DIRECTORY constant, the unidentifiable bucket, and its notice are all cleanly removed with no dangling references left behind (verified via grep — nothing else imports or reads any of the deleted symbols). RepairTarget[]'s new flat return shape is simpler than the old { targets, unidentifiable } pair and check.ts is updated accordingly.

Test coverage is solid: test/repair.test.ts covers the id-from-entry behavior as pure functions for both buckets (including an entry outside the rules tree, now repairable), and test/repair-integration.test.ts adds an end-to-end case where the entry's ruleId disagrees with its path's directory name, asserting both the exact restoreCalls value and that the blessed bytes land under the entry's id. The PR body's claim that reinstating the parse fails exactly these three new assertions is a good sanity check of test adequacy.

Deliberately-untouched behavior (verifyRestoredCheck comparing against reconcile's expected rather than the restore response's own signature; unknown routing nowhere; nothing repaired executing in the same pass) is unaffected by this diff and still correct.

One finding posted inline (packages/cli/src/rules/runtime/repair.ts:48-57, marked [New], minor/plausible): neither unsafe nor missing entries get any runtime validation that ruleId is actually present/non-empty — reconcile.ts decodes the response with a bare cast, not a schema. If a server response ever omitted ruleId on an unsafe entry (rollback, canary, future regression), the old code degraded gracefully to a clear "could not identify" notice; now restoreRule would be called with ruleId: undefined, producing a literal /cli/api/request/undefined/restore request instead. This gap already existed for missing before this PR, so it's not a new class of bug, but this PR does widen it to the (likely more common) unsafe bucket. Flagging as worth a defensive check, not a blocker — it contradicts the server's own published schema, which this PR's evidence establishes is solid ground to stand on.

No other correctness, security, or performance issues found in the diff.

[thecodedrift]

Addressed in dd2a991 (rebased onto main @ effee6d). Refs #240.

The one finding was real. I reproduced it before fixing it: with the guard reverted, an integration test serving an unsafe entry with no ruleId records the mock receiving the literal string undefined as the rule id —

AssertionError: expected [ 'undefined' ] to deeply equal []

— which is /cli/api/request/undefined/restore exactly as you predicted.

What changed. repairTargets partitions instead of trusting entry.ruleId. Entries whose id is absent, empty, or whitespace-only go to an unidentified bucket, never become a request, and are reported as a notice on the --json envelope. Four properties held deliberately:

• Both buckets. You flagged that missing had the same gap before this PR; it is covered too, so the pre-existing hole closes alongside the one this PR widened.
• No path parsing. The fallback stays deleted — a test asserts the entry is not rescued by the directory name sitting in its own path. The answer to a missing id is to report it, not to guess one.
• A notice, never a failed check. A rule that could not be repaired stays withheld, which is already the safe state.
• Consistent with existing precedent. writeRuleFile keeps a runtime check for a payload carrying both files and content even though the published union makes it unrepresentable. Same reasoning: the type states what the service promises; the check defends against it breaking that promise. Your point that the schema is solid ground but reconcile decodes with a cast — so the type is not a fact about the bytes — is the crux, and it is right.

Proven to bite. Reverting repairTargets to the pre-fix mapping fails 10 assertions (9 unit, 1 integration) and nothing else. Restored, all pass.

Checks, re-run after the rebase rather than assuming the pre-rebase run held: pnpm build, pnpm typecheck, pnpm lint, vitest run (1180 passed, 73 files — up from 1169, +10 new and +1 inherited from #244), and pnpm cli check against a fresh build: 4 warnings, the known pre-existing no-hedging set (3 in route.txt, 1 in onboard.txt).

— AI Coding Agent