[#313] docs: skill authoring effectiveness standard (task T2)#319
Conversation
Code Review — PR #319 (Independent, consolidated)Review Information
Review SummaryOverall Assessment
4 review rounds on this PR, driven by a genuine license-risk concern: the standard is conceptually grounded in the AC2 verificationAll mandated elements present and each carries a genuinely decidable "how to check" test (verified 7 of 9 directly decidable, 2 decidable-with-reasonable-judgment — acceptable for prose guidance). Manual read of all 7 rewritten passages confirms natural phrasing, no lost antecedents, and each principle still asserts the identical rule as before the rewording — this was a wording-only pass throughout. Findings — all resolved
Gates: mdlint 0 errors, docs-staleness PASS, links valid, knowledge-hub 56/56. VerdictApproved. Scope discipline held throughout (only 3 files, zero SKILL.md touched — clean mutex with PR #310); the license-risk concern that drove 3 remediation rounds is now genuinely closed, verified by independent reproduction rather than trust. 🤖 Independent review generated with Claude Code |
bf577bb to
c5551ac
Compare
c5551ac to
055583d
Compare
Pass esplorativo di riduzione testo su 16/35 skill (le altre 14 erano mutex con #310, ora mergiata). Risultato onesto: -1.5% parole, non il grande guadagno sperato — il corpus era già scritto in stile compresso. Ha portato alla scoperta che il guadagno vero è strutturale (description non trigger-oriented, zero progressive disclosure, duplicazione cross-skill), ora formalizzato come standard in #313 (T2 su PR #319), che riscriverà il corpus più a fondo nei task T3-T6 successivi.
- writing-skills.mdx: add nine-principle effectiveness standard (description=trigger, information hierarchy + progressive disclosure, completion criteria, pruning, leading words, positive phrasing, co-location, constraints, evaluation), each with a how-to-check test, plus review checklist usable for T3-T6 - skills-guide.md (dataset + root mirror, prefix transform applied): condensed Authoring Standard section - ADR-010 records the standard: conformance-only guideline + corpus audit gaps as context; applies to new skills immediately, existing corpus refactored via #313 T3-T6; numbering assumes adr-009 (unmerged branch) lands first Refs: #313 T2, AC2 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nce text, decidable checks
Round-1 fix was too narrow: rewrote 5 spots, missed further verbatim/ near-verbatim lifts from mattpocock/skills across the same section (intro, principles 1-4), incl 2 exact phrase matches. Full re-scan of effectiveness standard vs both source files; rewrote every passage sharing more than incidental structure/wording, preserving meaning. Applied same fixes to both skills-guide.md copies where echoed.
Both the frontmatter description and the Composition Interface section cited "(story #256)" — a Pair-repo-specific issue number, meaningless to any project that installs this KB-shipped skill. KB guidelines and skills must not reference this repo's own ADRs, decision-log entries, or issue-tracker numbers, since they distribute in isolation to other projects. Removed both citations, dataset + installed mirror. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fbe8b3c to
4e91b1c
Compare
Smoke test — review checklist applied to one existing skillPer T2's Testing line ("review checklist applied to one existing skill as smoke test"): applied the 9-box checklist from
Conclusion: checklist is decidable and discriminates — 4 real findings, each already scoped to T3/T6 by the story's task breakdown (no T2 change needed). Standard usable as the T3–T6 gate. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Independent hunk-by-hunk review of T2b commit 175b043Independent adversarial review, scope strictly Review InformationPR Number: #319 (commit Review SummaryOverall Assessment
Key Changes Summary28 files (14 skills x dataset+mirror), +62/−98 lines, −596 words (−2.8% over the 14 dataset files; same order as the precedent pass Verification method (all checks reproduced, not trusted)
Meaning-survival trace (every non-trivial cut)
No rule, step, argument-table row, output element, HALT, or edge case is lost. No Arguments table or Output Format block is touched. Business Value ValidationCompletes corpus coverage of the slimming pass now that the #310 mutex is lifted; keeps dataset/mirror sync and the #224 conformance contract intact. Matches the T2b scope: mechanical completion, not a rewrite. Code Review Checklist (applicable subset — docs-only change)
Testing ReviewDetailed Review CommentsPositive Feedback
Issues to AddressMinor Issues 💡
Nits (non-actionable, reported for transparency)
Risk Assessment
Tech Debt
Adoption ComplianceDocs-only change: no new dependencies, no new technical decisions → no ADR required. Templates followed (commit format, branch naming). Inline check level — no non-conformities. Verdict: Approved with Comments — 0 Critical / 0 Major / 1 Minor (+2 non-actionable nits). All 4 T2b acceptance criteria verified met (AC4 met for issue-number citations; R7.2 residue noted as pre-existing). Merge remains human-gated per workflow. |
…mediation) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
T2b remediation — review #319 (comment)
Gates re-run after fix: knowledge-hub tests 80/80 (incl. |
T2b remediation verified — ApproveRe-reviewed remediation commit
Finding 1 (the sole Minor) is resolved with no meaning loss and no new findings. All 4 T2b acceptance criteria now met cleanly, including the citation-clean AC. Verdict: Approve. Merge remains human-gated. |
Code Review — PR #319 (canonical /pair-process-review run)
Review Information
Review SummaryOverall Assessment
Key Changes Summary34 files, +279/−119. Two disjoint slices in one PR:
Business Value ValidationConfirmed. T2 lands the standard that gates the later structural passes (T3–T6); T2b completes the corpus-wide slimming now that #310's mutex is lifted. Both match the story's task breakdown; scope is disciplined (no forward-task work leaked in). Phase 1 — ValidationPR open and mergeable; story #313 loaded with AC + task breakdown. Review type classified docs+refactor. Proceeded. Phase 2 — Technical ReviewQuality gates (re-run on
|
| Cut | Verified survives in same file |
|---|---|
assess-code-quality Step 2 Verify "If not found, use built-in thresholds" |
Graceful Degradation L155 — with the concrete thresholds (cyclomatic > 10, fn > 50, file > 300, cov > 70%), strictly more precise |
assess-code-quality + assess-debt "Resolution cascade" Notes bullet |
Step 1 Resolution Cascade sections (Path A/B[/C]) + frontmatter descriptions |
record-decision "ADR/ADL/DDR mutually exclusive" |
## Core Rule: ADR, ADL, and DDR Are Mutually Exclusive (L24) |
record-decision DDR-with-no-context-map GD bullet |
Step 4 L129–130 + Edge Cases L176 |
record-decision "adoption always updated / single source" |
overview L10, Core Rule L29, Step 4 L105 |
assess-stack "Lifecycle-spanning" / "Section ownership" Notes |
frontmatter + ## Lifecycle Modes (L31) + Step 0; ## Section Ownership (L195), L173–174, L249 |
bootstrap "stop between phases / day one / idempotent" Notes |
Notes L310, Idempotent section L285, L199 ("executable from day one") |
| 8× Step-4 Verify "This skill writes no files." | each file's Composed-Skills/overview output-only statement |
No rule, step, argument row, output block, HALT, or edge case is lost. No Arguments table or Output Format block touched.
Mirror consistency
Diff-of-diffs (dataset hunks vs .claude/skills/pair-* hunks, reversing the name-prefix transform): byte-identical (56 changed lines each side). The assess-output-only.test.ts guard (which greps both dataset and mirror) is green, enforcing sync at test time.
Design-rule / standards scan
Markdown-only change; no code, no dependencies, no API/DB surface. No Design Rule (DR-*) applies. Positive-phrasing, co-location, and pruning principles the PR documents are themselves respected by the T2b cuts (they remove sediment/duplication, not load-bearing rules).
Phase 3 — Adoption Compliance
- New technical decision → ADR present. ADR-010 records the standard (Decision / Rationale / 3 alternatives / Consequences / Adoption Impact / References). Satisfies the "record decisions" rule → no missing-ADR HALT.
- Portability rule respected. No assistant-specific frontmatter added; T2b touches body prose only. The standard itself codifies the core-frontmatter-only rule (principle 8).
- No dependency/tech-stack delta → nothing for assess-stack. Templates (ADR, commit
[#313] type:, branchdocs/#313-authoring-standard) followed. - Citations clean. Corpus grep for
R7.2,#100,#104,#105,#256over dataset + mirror post-state: zero (only generic#42example placeholders remain). Satisfies T2b's citation-clean AC.
Degradation: inline (verify-adoption/assess-stack not required for a docs+refactor change with no stack delta).
Phase 4 — Completeness (DoD vs #313 T2 + T2b)
T2 · AC2 — all mandated elements present in writing-skills.mdx: description=trigger, information hierarchy + progressive disclosure, checkable+exhaustive completion criteria, the four pruning tests (no-op / sediment / duplication / single source of truth), leading words, positive phrasing, co-location, the agentskills.io constraints, portability. Each principle carries a decidable "How to check". Review checklist present and usable as the T3–T6 gate (AC2's stated bar). ADR recorded. Condensed mirror in skills-guide.md (dataset + root) consistent under the name transform. Met.
Note: the PR is more accurate than AC2's wording — AC2 calls "name+description ≤1024" an agentskills.io constraint; the PR correctly distinguishes the spec's separate caps (name ≤64, description ≤1024) from Pair's stricter combined ≤1024 bound. This is the round-1 remediation, not a defect.
T2b · AC — 14 skills hunk-verified against origin/main; every non-trivial cut traced to same-file survival (above); dataset+mirror byte-consistent under transform; citation grep clean. Met.
Story-level DoD items not covered here (trigger eval, next routes every skill, all-AC) belong to T1/T3/T7 and are correctly out of this PR's scope — #313 is an umbrella delivered one-PR-per-task.
Tech Debt (assess-debt — output-only, surfaced not blocked)
- No new debt introduced.
- Pre-existing corpus nits surfaced: prose↔test literal coupling (
assess-debtprose ↔assess-output-only.test.tsgreps) — by design (that is what a conformance guard is for); mirror name-transform also normalizes trailing slashes on some directory links — pre-existing on main. Neither is introduced by this PR.
Detailed Review Comments
Positive Feedback
- The standard is genuinely decidable, not aspirational — every principle ends in a check two reviewers couldn't disagree on. That is what makes it a usable T3–T6 gate rather than prose.
- Round-1 spec correction (separating the spec's per-field caps from Pair's combined bound;
version/authorreframed as a tolerated extension) is precise and honest about where Pair diverges from agentskills.io. - T2b cut discipline is exact: every removed sentence is redundant elsewhere in the same file; the
assess-code-qualityfallback even survives in a more precise form.
Issues to Address
Minor Issues 💡
-
.pair/adoption/tech/adr/adr-010-...md:7— stale numbering-rationale note. The note reads "adr-009 exists on an unmerged branch … this ADR takes the next free number … Renumber to 009 only if that branch is abandoned." As of nowmaincarries adr-009 (merged via [#224] refactor: assess-* output-only; record-decision sole adoption writer #310) and adr-011/012/013, so adr-010 is a valid, collision-free gap-fill — but it is no longer "the next free number", and the "renumber to 009" instruction is moot. Impact: none functional (010 is unique; docs-staleness and links green); purely an out-of-date explanatory note. Recommendation: drop the note (or trim to "fills the 010 gap between merged 009 and 011") — non-blocking; fine to fold into a later ADR-hygiene pass.
Nits (nonActionable — reported for transparency)
- Prose↔test literal coupling (
assess-debt/SKILL.md↔assess-output-only.test.tsgrepsno auto-creation/never block): by design — a conformance guard; future rewordings that trip it are the guard doing its job. - Mirror transform trailing-slash normalization on some directory links: pre-existing on main, byte-untouched here; "fixing" it would exceed the meaning-preserving scope.
Risk Assessment
| Risk | Impact | Probability | Mitigation |
|---|---|---|---|
| Latent meaning loss in a T2b cut | Low | Low | Full same-file survival trace re-verified in post-state; #224 guard green |
| Dataset/mirror drift | Low | Very Low | Diff-of-diffs byte-identical; guard test covers both copies |
| Standard's external-spec claims drift from agentskills.io over time | Low | Low | Spec claims already remediated + independently checked in a prior round; ADR cites the source |
Verdict
Approved with Comments — 0 Critical / 0 Major / 1 Minor (+2 nonActionable nits). All T2 and T2b acceptance criteria verified met; all quality gates independently re-run green on fea700d. The single Minor (stale ADR numbering note) is non-blocking. Merge remains human-gated (Phase 6 not run).
🤖 Canonical review generated with Claude Code via /pair-process-review
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Remediation — canonical /pair-process-review finding (Minor)
Before: Gates green: knowledge-hub tests 80/80 + check:links valid, |
Skill authoring effectiveness standard (#313, task T2)
Story: Part of #313 (task T2 of T1-T7) — standalone reviewable slice
Type: Documentation · Priority: P1 · Risk: Low (docs + 1 ADR, no skill content changed)
Motivation
An audit of the 35-skill corpus (best practices from agentskills.io + Claude Code skill docs + the
mattpocock/skillsreference model) found the corpus is exemplary on internal conformance (uniform scaffold, Check/Skip/Act/Verify, HALT, composition contracts) but the internal guideline never covered effectiveness: trigger-oriented descriptions, information hierarchy/progressive disclosure, checkable completion criteria, pruning discipline, leading words, positive phrasing. This task lands the standard itself; T3-T6 (later, sequenced after PR #310 merges) apply it to the corpus.Changes Made
apps/website/content/docs/contributing/writing-skills.mdx: mechanics (types/structure/anatomy/composition) kept verbatim; new "The effectiveness standard" section with 9 numbered principles (description=trigger, information hierarchy + progressive disclosure, completion criteria, pruning discipline, leading words, positive phrasing, co-location, constraints, evaluation), each with a decidable "how to check" test, plus a review checklist usable as the T3-T6 gate..pair/knowledge/skills-guide.md(dataset + root mirror).adr-010-skill-authoring-effectiveness-standard.mdrecording the decision/rationale/consequences.version/authoraren't top-level spec fields, only examplemetadata:keys; now explicitly whitelisted as a tolerated Pair extension); fixed the size-limit attribution (spec caps name/description separately, Pair's combined bound is stricter); made the leading-words check decidable; softened an overclaim ("worked examples" → "illustrative examples"); rewrote 5 passages that were verbatim/near-verbatim lifts from the CC-licensed reference model.Files Changed
apps/website/content/docs/contributing/writing-skills.mdxpackages/knowledge-hub/dataset/.pair/knowledge/skills-guide.md+ root mirror.pair/knowledge/skills-guide.md.pair/adoption/tech/adr/adr-010-skill-authoring-effectiveness-standard.md(new)Testing / Quality Gates
Risk Assessment
Low — pure documentation/guideline addition, zero skill files touched (mutex with PR #310's 14 skills fully respected).
Reviewer notes (disclosures)
mattpocock/skillsreference — round 4's mechanical scan is a genuine methodology upgrade over rounds 1-3's eyeball-only approach, and the reviewer independently reproduced the "0 matches" claim with their own script rather than trusting it.🤖 Generated with Claude Code