Skip to content

Apply the skill-design philosophy to /setup and /submit-for-review, and fix the permission audit's false positive #208

Description

@sebastientaggart

Problem to Fix

The skill-design philosophy adopted in #206 has been applied to two skills so far. Two remain untouched, and they are now the two largest in the library — together roughly a third of everything left. Neither has been assessed against the prune/keep test.

They need assessing, but this ticket should not be mistaken for a repeat of #206. The audit behind it found that these two files are long for a different reason than the ones already done, and the honest expectation is a much smaller reduction.

The skill that guides first-time configuration is long because nearly every step writes to a file and every write sits behind a human confirmation. Its state-detection ordering, its profile-to-settings mapping, and its confirmation gates are all firmly on the keep side of the test. What can actually go is display formatting — the health summary block, the permission audit block, the label listing, the change-preview boxes — plus some genuine duplication in the label phase.

The skill that runs the submit-and-review flow is long because most of it is policy: the review contract, the four-tier verdict routing, the conditions gating follow-up ticket creation, the sensitive-area rules. All of that stays. What can go is a set of near-identical document templates and one warning block that appears twice in immediate succession.

The audit also surfaced a real defect. The setup skill's permission audit reads the shared command list and reports anything not covered by an allow rule. One command on that list is deliberately never emitted as an allow rule — that exclusion was an intentional decision in the work that shipped with #202. The result is that this check reports a false positive on every project, every time it runs, which directly contradicts the skill's own hard rule that diagnostics must prefer false negatives over false positives.

Why it Matters

Applying the philosophy to only part of the library leaves it half-stated. A contributor reading the skills to learn the house style currently sees two shapes and no way to tell which is current.

The false positive matters more than its size suggests. It appears during first-run setup, which is the first thing a new user ever sees, and it tells them their configuration is incomplete when it is correct. That is a bad first impression from the one skill whose entire job is to build confidence that the setup is sound.

The expectation-setting matters too. Judging this work by lines removed would push toward cutting exactly the things the test says to keep — approval gates on a skill that writes config files, and policy on the skill that decides whether code merges. Both of those are among the most consequential rules in the project. A modest reduction here is the correct outcome, not a failure.

General Approach

Apply the same test used in #206: prune where variance produces a different-but-fine result, keep where variance produces a wrong result.

For the setup skill, treat the prune surface as display formatting only. Remove the fixed output blocks and let the agent lay out the health summary, permission report, and label list itself. Collapse the duplicated label-listing work and the repeated starter-label baseline. Leave untouched: the ordering of state detection and the stop-at-first-failure rule, the profile-to-settings mapping, the profile inference rules, every confirmation gate, and the hard rules. This skill both writes configuration and forms a new user's first impression, so it gets the most conservative treatment in the library.

Fix the permission-audit false positive as part of this work. The audit needs to account for the command that is intentionally excluded from generated allow rules rather than reporting it as missing.

For the submit-and-review skill, prune the document templates and let the agent compose those bodies from a description of what each must contain. Remove the warning block that appears twice in immediate succession, keeping one. Leave untouched: the review contract and its finding tags, all four verdict tiers and their routing, the sensitive-area gate, the follow-up gating conditions, the branch guard and the working-tree drift restore, the check gate, and every platform-behaviour note about issue-closing keywords.

Set expectations in the ticket's own terms: a reduction of roughly a fifth to a quarter across the two, not the more than half achieved on the previous pair. If the work trends much beyond that, it is a signal that something on the keep side is being cut.

Verify by exercising both skills. The submit-and-review flow runs on any real ticket. The setup skill needs its already-configured path exercised here, and its first-run and partial-setup paths reviewed by reading, since this project cannot reach those states.

Complexity

Verification / QA effort: significant

Both skills are consequential — one writes configuration and git settings, the other decides whether code merges — and neither is covered by automated tests. Correctness is judged by running the workflows and by careful reading of the paths this project cannot reach. The setup skill has three top-level states and several partial-setup branches, only one of which is reachable here.

Acceptance Criteria

  • Both skills are assessed against the prune/keep test, with fixed display formatting removed and the agent left to lay out its own output.
  • Duplicated label-listing work and the repeated starter-label baseline in the setup skill are collapsed to one occurrence each.
  • The duplicated warning block in the submit-and-review skill appears once rather than twice.
  • Document templates in the submit-and-review skill are replaced by descriptions of required content.
  • The permission audit no longer reports the intentionally-excluded command as missing, verified by running setup on this project and seeing a clean result.
  • No confirmation gate, state-detection ordering rule, profile mapping, verdict tier, review policy, prompt-avoidance instruction, or platform-behaviour note is lost. Verified deliberately against the keep list, not assumed.
  • The submit-and-review flow is exercised end to end on a real ticket; the setup skill is exercised on its configured path and its unreachable paths reviewed by reading.
  • The reduction achieved is recorded, with any shortfall against the rough expectation explained rather than closed by cutting further.

Metadata

Metadata

Labels

bugSomething isn't workingenhancementNew feature or requestready-for-qaIssue is ready to be QA tested

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions