Skip to content

Prune /setup and /submit-for-review; fix permission-audit cd false positive - #210

Merged
sebastientaggart merged 1 commit into
devfrom
feature/208-prune-setup-and-submit-fix-cd-false-positive
Aug 5, 2026
Merged

Prune /setup and /submit-for-review; fix permission-audit cd false positive#210
sebastientaggart merged 1 commit into
devfrom
feature/208-prune-setup-and-submit-fix-cd-false-positive

Conversation

@sebastientaggart

Copy link
Copy Markdown
Member

Follow-on to #206, extending the prune/keep test to the two largest remaining skills and fixing a permission-audit false positive surfaced during the audit. As anticipated in the ticket, the reduction here is modest — these files are mostly gates and policy (keep-side), not output templates.

The defect (fixed)

permissions.yaml listed cd in a single flat commands: list, but sync.py deliberately excluded it from generated allow rules via a hardcoded if c != 'cd'. /setup's Phase 2 read that same flat list and reported Bash(cd:*) as a missing allow rule on every project, every run — contradicting the skill's own hard rule to prefer false negatives, and appearing during first-run setup.

Fixed by splitting the data rather than hardcoding the exception twice:

  • permissions.yaml now has commands: (emitted as allow rules and validated) and validate_only: (validated but never emitted — cd).
  • sync.py's emitter reads commands:; the if c != 'cd' special case is removed — the exclusion is now a property of the data. A new _validated_commands() helper feeds the union to both validators, so cd stays legal in skill code blocks.
  • setup.md Phase 2 reads commands: only, so it structurally cannot report cd as missing.

The exclusion lives in one place; the emitter, validators, and setup audit can no longer drift. Verified by simulating Phase 2 against this project: zero missing rules, no Bash(cd:*). Five regression tests lock in the split.

Pruning

  • setup.md 491 → 392 (−20%): removed the fixed health-summary block, permission-report block, both signing confirmation boxes, and the label-list/change-preview boxes; deduped the doubled gh label list re-fetch and the starter-label baseline. Kept: every confirmation gate, state-detection ordering + stop-at-first-failure, the profile→values table and inference rules, the REVIEW_EFFORT policy note, and all hard rules.
  • submit-for-review.md 455 → 422 (−7%): four document templates (pr_body, resolution_comment, followup_body, followup_link_comment) replaced with descriptions of required content (keeping the resolution comment's non-technical-audience rule); the no-heredoc warning that appeared twice in immediate succession is now singular. Kept: the CC review contract and finding tags, all four verdict tiers and routing, the sensitive-area gate, Step 9's gating conditions, the branch guard and working-tree drift restore, the make -n check gate, and every closing-keyword note.

Reduction recorded

Combined the pair is 946 → 814 (−14%), below the notes' ~20–25% estimate. setup.md hit its ~380 target; submit-for-review.md reduced less because its listed prune surface (four small templates + one warning) totals ~33 lines — everything else is keep-side policy. Cutting further would remove the review contract or verdict tiers, which the ticket explicitly rules out. This is the modest-reduction outcome the ticket anticipated, not a shortfall to be closed by cutting deeper.

Verification

  • ./sync.py --validate clean (placeholder, permission, command-shape); all four adapters regenerated.
  • Test suite: 128 passing (+5 for the permission split).
  • Keep-list audited across both skills; no gate, ordering rule, profile mapping, verdict tier, review policy, prompt-avoidance instruction, or platform-behaviour note lost.
  • /submit-for-review exercised end to end by this PR. /setup's configured-path Phase 2 verified above; its unreachable first-run and partial-setup states reviewed by reading the rendered output.

Closes #208

@sebastientaggart
sebastientaggart force-pushed the feature/208-prune-setup-and-submit-fix-cd-false-positive branch from 9d781d9 to d636fe0 Compare August 5, 2026 18:07
@sebastientaggart

Copy link
Copy Markdown
Member Author

Code review — /code-review medium + Code Cannon policy

Reviewed the diff across the 8 finder angles, with attention on the real sync.py changes (the commands: / validate_only: split) alongside the two skill prunes.

Sensitive-area gate: considered, not triggered. The change touches permission-allowlist generation, but the emitted allowlist is byte-identical to before (cd was already excluded via the old hardcode) — no new command is blessed and no runtime permission surface changes. It is a refactor of where the exclusion lives, not what gets allowed.

Correctness: the sync.py change is behavior-preserving. Both validators (validate_permissions, validate_command_shapes) previously read commands: (which included cd) and now read the commands + validate_only union — so cd remains a recognized command exactly as before. Only the emitter changed, and its output is unchanged (verified: 11 rules, no Bash(cd:*)). The removed if c != 'cd' special case is re-established by cd no longer being in commands:. Well covered by the five new TestPermissionCommandSplit tests.

Findings

  • [WARNING] docs/index.md:126 — the recommended .claude/settings.json example led with "Bash(cd:*)", and /setup's Phase 2 points users to this doc "for a full example." That taught users to add the exact allow rule Code Cannon deliberately never emits and this PR's audit fix (correctly) will never request — an inconsistency between two parts of the project, caught by no test. Fixed in d636fe0 by removing the line and adding a one-sentence note explaining why cd is omitted.

No correctness, keep-list, or altitude findings on the skill prunes: the keep-list (confirmation gates, state-detection ordering, profile mapping, review contract, all four verdict tiers, sensitive-area gate, Step 9 gating, branch guard, closing-keyword notes) was audited present, and the removed content was display formatting and duplicated templates only.

Verdict: APPROVE

@sebastientaggart
sebastientaggart merged commit 3174106 into dev Aug 5, 2026
6 checks passed
@sebastientaggart
sebastientaggart deleted the feature/208-prune-setup-and-submit-fix-cd-false-positive branch August 5, 2026 18:08
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