Skip to content

fix(install): restore a Taskless permission rule removed from the file - #628

Merged
theCodeDrift merged 1 commit into
mainfrom
fix/624-restore-removed-permission-rules
Nov 8, 2026
Merged

theCodeDrift merged 1 commit into
mainfrom
fix/624-restore-removed-permission-rules

Conversation

@theCodeDrift

Copy link
Copy Markdown
Member

What changed

syncPermissions decided which Taskless allow rules to add to a harness permission file by comparing against the manifest record in .taskless/taskless.json, while it decided what to remove from the file itself. When a recorded rule was taken out of the file (a hand edit, a revert, a merge), init and the wizard believed it was still there: re-running init changed nothing and the wizard preview showed no edit.

  • add is now computed from the rules actually in the file (harness.found(text), called once and reused for both sides). The record keeps its one job: whether Taskless created the file and may delete it once empty.
  • A recorded file that was deleted is recreated with the rules, and is now recorded as created: true, since this run made it. Previously a created: false record would have stuck, and the recreated file could never be cleaned up once empty.
  • The rules written back to the record are deduplicated, so a restored rule that the record already listed is not listed twice.

The cli-init requirement "Each permission file holds one Taskless rule set per installed package" already said the record decides only whether a file may be deleted. The OpenSpec change (archived in this PR) restates it in full, adds that presence is read from the file, and adds three scenarios: a removed rule is written again, a deleted file is written again, and a file that agrees with the record is not edited. All 10 prior scenarios are preserved (checked after archive).

Testing

  • New a recorded rule the file no longer holds suite in packages/cli/test/permissions.test.ts, run for every harness with a file (Claude Code, Cursor editor, Cursor CLI, OpenCode, Codex):
    • record lists the rule, file lacks it: the rule is re-added, reported as an edit, recorded once, and a further run is a no-op;
    • record lists the rule, file was deleted: the file is recreated with the rule, reported as created, and recorded as created;
    • record and file agree: no edit, file unchanged.
  • 10 of the 15 new tests fail against the old code (the agreement cases pass both before and after, as intended).
  • pnpm typecheck, pnpm lint, and pnpm --filter @taskless/cli test (2981 passed) all pass locally.

Fixes #624

syncPermissions decided what to add by comparing against the manifest
record, so a recorded rule taken out of a harness permission file was
never written back. Read presence from the file for adding as well as
removing, record a recreated file as created by Taskless, and never list
a restored rule twice in the record.

The cli-init permission requirement now states that presence is read
from the file, with scenarios for a removed rule and a deleted file.

Refs #624
@theCodeDrift

Copy link
Copy Markdown
Member Author

@claude /review

@github-actions

github-actions Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @theCodeDrift's task in 2m 38s —— View job


Review mode: incremental — read 0 prior review thread(s) before reviewing.

.prior-review.json contained no review threads, reviews, or top-level comments beyond the @claude /review trigger itself, so this is the first substantive review of this PR — there is nothing previously-raised to classify as addressed or still-open.

Scope

  • Read .prior-review.json
  • Fetch PR diff and changed files
  • Review packages/cli/src/install/permissions.ts logic change
  • Review test additions in packages/cli/test/permissions.test.ts
  • Review OpenSpec spec changes
  • Post final review

Assessment

I traced the core change in packages/cli/src/install/permissions.ts (syncPermissions, lines 905-948) by hand against both the new and pre-existing test scenarios:

  • add now derives from present (harness.found(text)) instead of had (the manifest record) (lines 911-915). This is the actual bug fix: previously a rule the record believed present but the file had lost (hand edit/revert/merge) was never re-added, because add = want.filter(rule => !had.includes(rule)) only consulted the record. Tracing through all four pre-existing scenarios (version upgrade, unrecorded rule replaced, colon-form replaced, nightly/release coexistence) confirms none of them relied on the old had-based exclusion in a way this change breaks — they only differ from the new behavior in the one case this PR targets (rule in want and had, absent from present).
  • created computation (line 923): text === undefined || recorded?.created === true correctly fixes the case where a previously-existing (not Taskless-created) file was deleted — old code (recorded?.created ?? text === undefined) would keep created: false because ?? only falls through on nullish, not on an explicit false, so a deleted non-Taskless file could never be recreated/cleaned up. The new logic correctly treats any deleted file as created-by-this-run while still respecting a prior created: true.
  • Dedup via new Set([...]) (lines 942-947) correctly prevents a restored rule already listed in had from being double-counted against result.added.
  • The reported PermissionEdit.created (line 952, text === undefined) is intentionally distinct from the persisted record's created (line 923) — one reports whether this run wrote a fresh file, the other decides future deletability. Verified this distinction is exercised correctly by the new tests (created: false on "added back", created: true on "recreated from deletion").

Test additions are solid: the new describe("a recorded rule the file no longer holds", ...) suite parametrizes over all five harnesses (Claude Code, Cursor editor, Cursor CLI, OpenCode, Codex) and covers exactly the three cases the fix changes behavior for (rule missing from file, file deleted, record/file in agreement), including a double-run no-op check and a record-not-duplicated check. The Codex case setup (synthetic CODEX_MARK-only file simulating "existed before Taskless") is a bit artificial relative to how a real Codex rules file would come to exist, but it correctly exercises the created persisted-record path and the assertions hold up against the harness's actual found/edit implementation.

The OpenSpec spec delta (openspec/specs/cli-init/spec.md) accurately reflects the new behavior ("Whether a file holds a rule SHALL be read from the file, never from the record...") and the three new scenarios map directly onto the three new test cases. Nothing in the spec diff contradicts the implementation.

No correctness, security, or test-adequacy issues found. No inline comments posted — I found nothing to flag.

@theCodeDrift

Copy link
Copy Markdown
Member Author

Re: @github-actions — "Claude finished @theCodeDrift's task in 2m 38s"…
#628 (comment)

Thanks. The review raised no findings, so there is nothing to change; the PR stands as reviewed.

— AI Coding Agent

@theCodeDrift
theCodeDrift merged commit 17bfded into main Nov 8, 2026
8 checks passed
@theCodeDrift
theCodeDrift deleted the fix/624-restore-removed-permission-rules branch November 8, 2026 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Install never restores a Taskless allow rule removed from a harness settings file

1 participant