Skip to content

feat(check): fail on a copy of an issued rule (copyOf) - #423

Merged
thecodedrift merged 3 commits into
mainfrom
feat/cli-copy-of-issued-rule
Sep 30, 2026
Merged

thecodedrift merged 3 commits into
mainfrom
feat/cli-copy-of-issued-rule

Conversation

@thecodedrift

@thecodedrift thecodedrift commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

This closes the directory-rename hole in tamper detection. Copy an issued rule to a new id, loosen it, and delete the original: reconcile answered the copy unknown (static rules run silently) and the original missing (a warning only), so check passed with a tampered rule running.

The service now marks such an unknown rule with copyOf: { ruleId, revisionId, files }. It's proposed in taskless/taskless#262 and published in the v2 schema by taskless/taskless#264. This PR implements the CLI's side of it for 0.12.0.

Behavior

  • sg or Vale unknown with copyOf: not run, removed from the snapshot, and check fails, naming the source rule and the differing files.
  • Source also missing: reported once, as a rename, naming taskless rule restore <source>. There's no separate missing warning.
  • Runtime: unchanged, never executed and no exit-code change. The skip reason now names the source, and a runtime rename is one notice.
  • A copyOf that is present but unreadable fails closed (unaccounted). The service sends copyOf only when it found issued content, so ignoring an unreadable one would run exactly the rule the field exists to stop.
  • check --json: integrity gains an optional copyOf: { ruleId, revisionId?, sourceMissing }, and engine.log records each copy.

Notes

Checks

pnpm typecheck, pnpm lint, and openspec validate --all --strict (30) pass. The CLI suite passes: 112 files, 1,889 tests, including 9 new verdict cases and one end-to-end rename against the mock v2 reconcile.

Refs taskless/taskless#255

v2 reconcile marks an unknown rule that carries an issued rule's file with
copyOf (taskless/taskless#262, #264). An sg or Vale copy no longer runs and
fails check naming its source; when the source is also missing the pair is
one rename naming `rule restore <source>`. Runtime copies are unchanged
(never executed) but name the source. A malformed copyOf fails closed.
Picks up copyOf on reconcile's unknown rules (taskless/taskless#264), the
revisions route (#261), whoami's per-org entitlements (#265), and
EntitlementAnnotation on served sets (#260). All additive; copyOf is still
read defensively, so nothing in the CLI changes.
@thecodedrift

Copy link
Copy Markdown
Member Author

@claude /review

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Claude finished @thecodedrift's task in 5m 9s —— View job


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

.prior-review.json contained no reviewThreads, no reviews, and no comments other than the @claude /review trigger itself. This is the first review of this PR — everything below is assessed fresh, nothing is "unchanged since last review."

  • Read .prior-review.json (empty — first review)
  • Fetch PR diff (gh pr diff 423) and metadata
  • Review schema/type changes (api-v2.d.ts, api-v2.schema.json, schemas/check.ts)
  • Review core policy logic (verdicts.ts, plan-check.ts)
  • Review docs/recipes (check.md, recover-rule.md)
  • Review OpenSpec deltas and archived specs
  • Review tests (verdicts.test.ts, runtime-check.test.ts)
  • Post findings

Assessment

This is a clean, well-scoped implementation. I traced applyVerdicts/applyCopy in packages/cli/src/rules/verdicts.ts line by line against every scenario in both the archived and standing OpenSpec deltas (cli-check and cli-rule-reconciliation), and against packages/cli/src/agent/check.md's worked example (bar-2/foo-1) — the generated failure/fix strings match exactly, character for character.

Specifically verified, with no issues found:

  • Rename dedup logic (missingIds / renamed sets, verdicts.ts lines 287–298, 440–457): missingIds is computed once, before the main loop, from verdicts filtered to verdict === "missing" rules not in reportedIds, so ordering between the two loops can't cause a stale read. The suppression of the source's own missing notice (but not its integrity entry) correctly matches design doc D3 ("both facts are true... integrity is per-rule state").
  • Fail-closed on malformed copyOf (readCopyOf, lines 144–157): a string, an object without ruleId, and an object with an empty ruleId are all correctly routed to unaccounted (not run, run fails) for both static and runtime engines — matches the "present but unreadable" contract in design doc D5, and is covered by it.each in verdicts.test.ts.
  • Runtime vs. static asymmetry: a plain runtime copy adds no extra notice (already covered by the standard skip-reason path) while a runtime rename adds exactly one notice and never touches plan.failures/exit code; a static copy or rename always fails via plan.failures. This is exercised end-to-end in the new runtime-check.test.ts case (mock reconcile + engine.log assertions + treeDigest proving nothing was written to .taskless/rules/).
  • Schema/type churn (Entitlement → EntitlementAnnotation on rule/{ruleId} GET, rollback, and restore responses): every consumer (plan-check.ts, recover.ts, generate.ts) reads these fields through parseEntitlementV2(value: unknown), so the narrower vendored type can't break compilation or introduce a runtime gap — confirmed by reading all three call sites.
  • --json integrity shape (schemas/check.ts): the new copyOf object and updated verdict enum description line up with both the Zod schema and the IntegrityCopyOf/IntegrityEntry TS types; CopyOfRead is correctly left unexported since it never appears in an exported signature (per .conventions/STYLEGUIDE-CODE.md's exported-types rule), while IntegrityCopyOf is exported because IntegrityEntry.copyOf references it.
  • Docs: check.md and recover-rule.md updates accurately describe the new behavior and correctly note their topic versions don't need a bump (both already new/bumped this cycle).

I did not find a correctness, security, or test-coverage issue worth flagging inline. No inline comments posted.

One thing I couldn't verify (noted per the review constraints, not treated as a gap): whether the vendored api-v2.schema.json/api-v2.d.ts actually match production byte-for-byte, since that depends on the live service (taskless/taskless#264) and history I don't have access to — the PR description states this was checked directly, which I have no way to independently confirm from the diff alone.

@thecodedrift

Copy link
Copy Markdown
Member Author

Re: @claude — "Claude finished @thecodedrift's task in 5m 9s"…
#423 (comment)

Thanks — no findings to address. On the one unverifiable point: the vendored api-v2 schema was re-fetched from the live service after taskless/taskless#264 shipped copyOf, per the PR description.

— AI Coding Agent

@thecodedrift
thecodedrift merged commit 9c32700 into main Sep 30, 2026
12 checks passed
@thecodedrift
thecodedrift deleted the feat/cli-copy-of-issued-rule branch September 30, 2026 05:24
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.

1 participant