feat: add 4 mattpocock-inspired skills + tdd horizontal-slicing boost - #21
Conversation
…ture Adds four new skills inspired by mattpocock/skills (MIT) and boosts developing-test-first with a Horizontal Slicing anti-pattern. New skills: - grilling-plans: adversarial pre-implementation interrogation, decision-tree walk with recommended answers, codebase-exploration-first - building-shared-vocabulary: maintains domain glossary (CONTEXT.md) and ADRs as living artifacts with lazy creation and a 3-criterion ADR gate - zooming-out-on-code: system-level map of unfamiliar code in the project's domain vocabulary (responsibility / callers / dependencies / siblings) - improving-architecture: surfaces deep-module refactor candidates using Ousterhout's depth/seam framing and the deletion test New slash commands: - /skill-set:plan:grill - /skill-set:code:zoom-out Improves developing-test-first with Horizontal Slicing anti-pattern section forbidding bulk-write tests-first then implementations-after. Bumps version 1.11.1 to 1.12.0. Attribution in README and CHANGELOG.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRelease 1.12.0 adds four new skills ( ChangesRelease 1.12.0: Skill & Command Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 44 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/skill-set/skills/developing-test-first/SKILL.md (1)
1-207:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFile slightly exceeds the 200-line limit.
The file is currently 207 lines, just over the 200-line progressive disclosure guideline. Consider moving the "Common Rationalizations" table (currently referenced at line 157) or the "When Stuck" table to the existing
reference/anti-patterns.mdfile to stay within the limit. As per coding guidelines, "All SKILL.md files must stay under 200 lines using progressive disclosure pattern, with detailed content in separate reference files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/skill-set/skills/developing-test-first/SKILL.md` around lines 1 - 207, The SKILL.md exceeds the 200-line limit; move the "Common Rationalizations" section (the paragraph and any tables referenced at or around the line mentioning reference/anti-patterns.md) and/or the "When Stuck" table out of SKILL.md into reference/anti-patterns.md, replace their content in SKILL.md with a short pointer like "See reference/anti-patterns.md for details", and ensure headings or cross-reference text (e.g., "Common Rationalizations", "When Stuck", and the existing reference link `reference/anti-patterns.md`) remain intact so readers can find the migrated content.plugins/skill-set/skills/improving-architecture/SKILL.md (1)
6-166: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd required packages and availability verification to this SKILL.
This SKILL.md doesn’t list required packages or how availability is verified in the execution environment. Please add a short section for that. As per coding guidelines, “List required packages in SKILL.md and verify they are available in the code execution environment.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/skill-set/skills/improving-architecture/SKILL.md` around lines 6 - 166, The SKILL.md (“Improving Architecture”) is missing a requirements section; add a short "Required packages and availability" section under the Overview (or after Glossary) that lists required runtime packages (names and minimal versions) and a concise availability verification procedure: describe how the execution environment should check for those packages (e.g., verify via the package manager or attempt to import/require at startup and fail with a clear message), state expected exit/fallback behavior if a package is missing, and include any environment variables or feature flags used to toggle the skill; reference the SKILL.md headings ("Overview", "Process") so reviewers can place the new section consistently.
🧹 Nitpick comments (6)
plugins/skill-set/skills/zooming-out-on-code/SKILL.md (3)
50-70: ⚡ Quick winAdd language identifier to output template code block.
The code block at line 50 lacks a language identifier. Adding
markdownwill improve rendering and satisfy linting rules.📝 Proposed fix
## Output template -``` +```markdown **<Module name in domain vocabulary>**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/skill-set/skills/zooming-out-on-code/SKILL.md` around lines 50 - 70, The fenced code block that contains the module template (the block starting with "**<Module name in domain vocabulary>**" in SKILL.md) is missing a language identifier; update the opening fence from ``` to ```markdown so the snippet is rendered as markdown and passes linters—locate the block in plugins/skill-set/skills/zooming-out-on-code/SKILL.md (the triple-backtick fence immediately before the template) and add the word "markdown" after the backticks.
40-40: ⚡ Quick winClarify LSP availability assumptions.
Line 40 mentions "use
LSP findReferencesif an LSP is available for the language" but doesn't clarify whether LSP tools are expected to be present or how to fall back if unavailable. Consider adding a note in the Troubleshooting section about what to do when LSP isn't available.💡 Suggested addition to Troubleshooting
Add a row to the troubleshooting table:
| LSP not available for the language | Manual grep/search needed | Use `rg` or `grep` to search for references to the public API name |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/skill-set/skills/zooming-out-on-code/SKILL.md` at line 40, The doc mentions "use `LSP findReferences` if an LSP is available for the language" but doesn't say what to do if an LSP isn't present; update SKILL.md (zooming-out-on-code) Troubleshooting to document the fallback: add a table row or short note explaining "LSP not available for the language" and prescribe using manual search tools like `rg`/`grep` or editor-wide search to find references to the public API name, and mention that `LSP findReferences` is optional only when an LSP is installed.
76-100: ⚡ Quick winAdd language identifier to example code block.
The code block at line 76 lacks a language identifier. Adding
markdownwill improve rendering and satisfy linting rules.📝 Proposed fix
User asks about `src/orders/intake/validate.ts`. -``` +```markdown **Order Intake — Validation**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/skill-set/skills/zooming-out-on-code/SKILL.md` around lines 76 - 100, The fenced code block that begins with the "Order Intake — Validation" header in SKILL.md is missing a language identifier; update the opening fence from ``` to ```markdown so the block becomes a markdown code block (i.e., add "markdown" after the first triple backticks around the block that starts with "**Order Intake — Validation**") to satisfy linting and improve rendering.plugins/skill-set/skills/improving-architecture/reference/deep-modules.md (1)
44-50: ⚡ Quick winAdd language identifier to depth diagram.
The ASCII diagram at line 44 lacks a language identifier. Adding
textwill improve rendering and satisfy linting rules.📝 Proposed fix
The ratio between leverage gained and interface complexity carried. -``` +```text deep: [ ============= IMPLEMENTATION ============= ] [ interface ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/skill-set/skills/improving-architecture/reference/deep-modules.md` around lines 44 - 50, The fenced ASCII diagram using the lines that start with "deep:" and "shallow:" needs a language identifier to satisfy linting; update the opening triple-backtick before the diagram to include "text" (i.e., change ``` to ```text) so the block containing the "deep: [ ============= IMPLEMENTATION ============= ]" and "shallow: [ implementation ]" lines is treated as plain text.plugins/skill-set/skills/grilling-plans/SKILL.md (2)
118-130: ⚡ Quick winAdd language identifier to process flow diagram.
The process flow pseudocode at line 118 lacks a language identifier. Adding
textwill improve rendering and satisfy linting rules.📝 Proposed fix
## Process Flow -``` +```text receive plan → Domain Awareness (read CONTEXT.md / ADRs / code)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/skill-set/skills/grilling-plans/SKILL.md` around lines 118 - 130, The fenced pseudocode block beginning with "receive plan" in SKILL.md is missing a language identifier; update the opening triple-backtick to include "text" (i.e., ```text) so the process flow renders correctly and satisfies linting; locate the block containing the "receive plan" flow and change its opening fence accordingly.
59-65: ⚡ Quick winAdd language identifiers to example code blocks.
Multiple example code blocks (lines 59-65, 73-77, 85-89, 95-99, 107-112) lack language identifiers. Adding
textormarkdownwill improve rendering and satisfy linting rules.📝 Proposed fix for first example (apply similar pattern to others)
### Rule 2 — Provide a Recommended Answer With Every Question Every question must include the model's own recommendation, with reasoning. This forces commitment instead of polite hedging. -``` +```text WRONG: "What should happen when the cache is empty?" RIGHT: "What should happen when the cache is empty? My recommendation:Also applies to: 73-77, 85-89, 95-99, 107-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/skill-set/skills/grilling-plans/SKILL.md` around lines 59 - 65, The fenced example code blocks in SKILL.md are missing language identifiers (e.g., the blocks showing WRONG/RIGHT example text); update each of those fenced code blocks to include a language tag like ```text or ```markdown so the examples render correctly and satisfy linters—specifically modify the example block that begins with WRONG: "What should happen when the cache is empty?" and apply the same change to the other similar example blocks in the file (the ones showing WRONG/RIGHT pairs) to ensure consistent formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/skill-set/skills/improving-architecture/reference/deepening.md`:
- Around line 45-50: The fenced code block containing the four lines starting
with "Pure computation? → Category 1. Just merge." should include a language tag
to satisfy markdown linting; update the opening fence from ``` to ```text (i.e.,
add "text" after the backticks) so the block is explicitly marked as text.
In `@plugins/skill-set/skills/improving-architecture/SKILL.md`:
- Around line 70-79: The fenced code blocks showing the examples (the block
starting with "**N. <Candidate name in domain vocabulary>**", the block starting
with "**1. Order Intake validation cluster**", and the block beginning with
"orient (read CONTEXT.md and relevant ADRs)") lack a language token; update each
opening triple-backtick to include a language identifier (e.g., change ``` to
```text) so the markdown linter is satisfied and rendering is explicit.
- Line 62: The reference to the MCP tool `Agent` must use the repository's
server-qualified naming convention; update the text that says "dispatch an
`Explore` agent (`Agent` tool with `subagent_type=Explore`)" to use the fully
qualified tool name (e.g., replace `Agent` with the server-prefixed form like
`ServerName:Agent`) and ensure any inline code or example that mentions the tool
follows that same pattern; keep the `Explore` subagent_type as-is but show it in
the context of the server-qualified tool (e.g., `ServerName:Agent` with
`subagent_type=Explore`) and update the related reference to
`superpowers:dispatching-parallel-agents` if it includes unqualified tool
mentions.
- Line 3: The SKILL.md description contains a Korean trigger phrase "리팩토링 거리
찾아"; replace that phrase with an English equivalent (e.g., "find refactor
opportunities" or "find refactoring opportunities") or move the Korean phrase
into a localized documentation file, keeping the default SKILL.md content
English; update the trigger list inside the description paragraph (the string
after "phrases like") to remove the Korean token and use the chosen English
phrase so the repository policy is respected.
- Around line 46-149: Convert each main phase heading ("1. Orient", "2.
Explore", "3. Present candidates", "4. Classify dependencies", "5. (Optional)
Design It Twice", "6. Hand off") and the "Process Flow" into checklist-style
steps by inserting explicit markdown task items (- [ ] ...) that enumerate the
sequential actions to perform; for each phase (e.g., "Present candidates") add a
top-level checklist item and nested checklist items for its substeps (e.g., read
CONTEXT.md, run deletion test, prepare numbered candidates, ask user), ensure
the filled example stays as an example but add a checklist prompting the author
to fill Files/Problem/Solution/Locality gain/Leverage gain/Test impact, and add
a final checklist in "Process Flow" that reflects the decision branches (user
picks one? yes/no → next steps) so reviewers can copy/paste and tick progress
while following the existing prose in SKILL.md.
---
Outside diff comments:
In `@plugins/skill-set/skills/developing-test-first/SKILL.md`:
- Around line 1-207: The SKILL.md exceeds the 200-line limit; move the "Common
Rationalizations" section (the paragraph and any tables referenced at or around
the line mentioning reference/anti-patterns.md) and/or the "When Stuck" table
out of SKILL.md into reference/anti-patterns.md, replace their content in
SKILL.md with a short pointer like "See reference/anti-patterns.md for details",
and ensure headings or cross-reference text (e.g., "Common Rationalizations",
"When Stuck", and the existing reference link `reference/anti-patterns.md`)
remain intact so readers can find the migrated content.
In `@plugins/skill-set/skills/improving-architecture/SKILL.md`:
- Around line 6-166: The SKILL.md (“Improving Architecture”) is missing a
requirements section; add a short "Required packages and availability" section
under the Overview (or after Glossary) that lists required runtime packages
(names and minimal versions) and a concise availability verification procedure:
describe how the execution environment should check for those packages (e.g.,
verify via the package manager or attempt to import/require at startup and fail
with a clear message), state expected exit/fallback behavior if a package is
missing, and include any environment variables or feature flags used to toggle
the skill; reference the SKILL.md headings ("Overview", "Process") so reviewers
can place the new section consistently.
---
Nitpick comments:
In `@plugins/skill-set/skills/grilling-plans/SKILL.md`:
- Around line 118-130: The fenced pseudocode block beginning with "receive plan"
in SKILL.md is missing a language identifier; update the opening triple-backtick
to include "text" (i.e., ```text) so the process flow renders correctly and
satisfies linting; locate the block containing the "receive plan" flow and
change its opening fence accordingly.
- Around line 59-65: The fenced example code blocks in SKILL.md are missing
language identifiers (e.g., the blocks showing WRONG/RIGHT example text); update
each of those fenced code blocks to include a language tag like ```text or
```markdown so the examples render correctly and satisfy linters—specifically
modify the example block that begins with WRONG: "What should happen when the
cache is empty?" and apply the same change to the other similar example blocks
in the file (the ones showing WRONG/RIGHT pairs) to ensure consistent
formatting.
In `@plugins/skill-set/skills/improving-architecture/reference/deep-modules.md`:
- Around line 44-50: The fenced ASCII diagram using the lines that start with
"deep:" and "shallow:" needs a language identifier to satisfy linting; update
the opening triple-backtick before the diagram to include "text" (i.e., change
``` to ```text) so the block containing the "deep: [ =============
IMPLEMENTATION ============= ]" and "shallow: [ implementation ]" lines is
treated as plain text.
In `@plugins/skill-set/skills/zooming-out-on-code/SKILL.md`:
- Around line 50-70: The fenced code block that contains the module template
(the block starting with "**<Module name in domain vocabulary>**" in SKILL.md)
is missing a language identifier; update the opening fence from ``` to
```markdown so the snippet is rendered as markdown and passes linters—locate the
block in plugins/skill-set/skills/zooming-out-on-code/SKILL.md (the
triple-backtick fence immediately before the template) and add the word
"markdown" after the backticks.
- Line 40: The doc mentions "use `LSP findReferences` if an LSP is available for
the language" but doesn't say what to do if an LSP isn't present; update
SKILL.md (zooming-out-on-code) Troubleshooting to document the fallback: add a
table row or short note explaining "LSP not available for the language" and
prescribe using manual search tools like `rg`/`grep` or editor-wide search to
find references to the public API name, and mention that `LSP findReferences` is
optional only when an LSP is installed.
- Around line 76-100: The fenced code block that begins with the "Order Intake —
Validation" header in SKILL.md is missing a language identifier; update the
opening fence from ``` to ```markdown so the block becomes a markdown code block
(i.e., add "markdown" after the first triple backticks around the block that
starts with "**Order Intake — Validation**") to satisfy linting and improve
rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7cf2556-6ac2-4fb2-b55c-0fe89bdd9d7c
📒 Files selected for processing (18)
CHANGELOG.mdREADME.mdplugins/skill-set/.claude-plugin/plugin.jsonplugins/skill-set/commands/code/zoom-out.mdplugins/skill-set/commands/plan/grill.mdplugins/skill-set/skills/building-shared-vocabulary/SKILL.mdplugins/skill-set/skills/building-shared-vocabulary/reference/adr-format.mdplugins/skill-set/skills/building-shared-vocabulary/reference/context-format.mdplugins/skill-set/skills/developing-test-first/SKILL.mdplugins/skill-set/skills/grilling-plans/SKILL.mdplugins/skill-set/skills/grilling-plans/reference/codebase-cross-reference.mdplugins/skill-set/skills/grilling-plans/reference/decision-tree-walk.mdplugins/skill-set/skills/improving-architecture/SKILL.mdplugins/skill-set/skills/improving-architecture/reference/deep-modules.mdplugins/skill-set/skills/improving-architecture/reference/deepening.mdplugins/skill-set/skills/improving-architecture/reference/deletion-test.mdplugins/skill-set/skills/improving-architecture/reference/interface-design.mdplugins/skill-set/skills/zooming-out-on-code/SKILL.md
| ## Process | ||
|
|
||
| ### 1. Orient | ||
|
|
||
| If `CONTEXT.md` and `docs/adr/` exist (per `building-shared-vocabulary`), read them first — `CONTEXT.md` gives names to good seams, ADRs record decisions to not re-litigate. If absent, infer domain vocabulary from package/module names, test descriptions, and recent commit messages, and proceed. | ||
|
|
||
| ### 2. Explore | ||
|
|
||
| Walk the codebase looking for friction. Don't apply rigid heuristics — observe organically and note where understanding is hard: | ||
|
|
||
| - Where does understanding one concept require bouncing between many small files? | ||
| - Where is a module's interface nearly as complex as its implementation? (shallow) | ||
| - Where have pure functions been extracted just for testability, while the real bugs hide in *how* they're called? (no locality) | ||
| - Where do tightly-coupled modules leak across their seams? | ||
| - Which parts are untested or hard to test through their current interface? | ||
|
|
||
| For broader sweeps, dispatch an `Explore` agent (`Agent` tool with `subagent_type=Explore`) to walk a directory or feature area in parallel — see `superpowers:dispatching-parallel-agents`. | ||
|
|
||
| Apply the **deletion test** to anything you suspect is shallow: imagine deleting it. If complexity vanishes, it was a pass-through. If complexity reappears across N callers, it was earning its keep. Full procedure: `reference/deletion-test.md`. | ||
|
|
||
| ### 3. Present candidates | ||
|
|
||
| Number them. For each: | ||
|
|
||
| ``` | ||
| **N. <Candidate name in domain vocabulary>** | ||
|
|
||
| - **Files:** <paths> | ||
| - **Problem:** <why the current architecture causes friction> | ||
| - **Solution:** <plain English description of what would change> | ||
| - **Locality gain:** <what maintenance becomes easier> | ||
| - **Leverage gain:** <what callers stop having to think about> | ||
| - **Test impact:** <which tests survive, which become possible> | ||
| ``` | ||
|
|
||
| **Do not propose specific interfaces yet.** That belongs to the next phase. | ||
|
|
||
| **Vocabulary discipline:** Use `CONTEXT.md` terms for domain concepts ("the Order intake module") and the glossary above for architecture concepts ("a deep seam over the rate limiter"). Do not invent new architectural vocabulary; use the canonical terms. | ||
|
|
||
| **Filled example:** | ||
|
|
||
| ``` | ||
| **1. Order Intake validation cluster** | ||
|
|
||
| - **Files:** src/orders/intake/promotion-check.ts, inventory-check.ts, | ||
| credit-check.ts, route.ts (calls all three) | ||
| - **Problem:** Three shallow validators each export a single function; | ||
| every caller has to remember the right ordering and aggregate errors | ||
| manually. Two of three are also called by the admin Manual Order tool, | ||
| which currently re-aggregates errors with a different shape. | ||
| - **Solution:** Collapse the three validators behind a single | ||
| `validateOrder(order) → ValidationResult` interface that owns the | ||
| ordering, the aggregation, and the error shape. | ||
| - **Locality gain:** Adding a new check (e.g., fraud scoring) becomes | ||
| one edit inside the validation module; today it is three. | ||
| - **Leverage gain:** Both the HTTP route and the admin tool stop | ||
| re-implementing aggregation; both consume the same `ValidationResult`. | ||
| - **Test impact:** Existing per-validator unit tests can stay as | ||
| internal helpers; new tests assert against `validateOrder` outcomes — | ||
| closer to user-observable behavior. | ||
| ``` | ||
|
|
||
| **ADR conflicts:** If a candidate contradicts an existing ADR, only surface it when the friction is real enough to warrant reopening the decision. Mark it: _"Contradicts ADR-0007 — but worth reopening because…"_. Do not list every theoretical refactor an ADR forbids. | ||
|
|
||
| Ask the user: "Which of these would you like to explore?" | ||
|
|
||
| ### 4. Classify dependencies | ||
|
|
||
| Before designing a new interface, classify the candidate's dependencies (see `reference/deepening.md`): | ||
|
|
||
| 1. **In-process** — pure computation; merge and test directly | ||
| 2. **Local-substitutable** — has a local stand-in (PGLite, in-memory FS); use it | ||
| 3. **Remote but owned** — your services across a network; define a port with 2+ adapters | ||
| 4. **True external** — third-party (Stripe, Twilio); inject port, mock in tests | ||
|
|
||
| The category determines the seam strategy and what tests look like. | ||
|
|
||
| ### 5. (Optional) Design It Twice | ||
|
|
||
| If interface shape is non-obvious, run a parallel sub-agent design generation pass — see `reference/interface-design.md`. Spawn 3+ agents with radically different design constraints (minimal interface / maximum flexibility / optimize-for-common-caller / ports-and-adapters), present the results sequentially, then commit to one. | ||
|
|
||
| Skip this step when the right interface is already obvious. Use it when the user is unsure or when the candidate's interface shape would set a long-term direction. | ||
|
|
||
| ### 6. Hand off | ||
|
|
||
| Hand off to `grilling-plans` to interrogate the chosen design. Do not jump to writing implementation plans directly — the candidate is still under-specified, and grilling will surface what is unclear. | ||
|
|
||
| After grilling, ADRs and `CONTEXT.md` updates are owned by `building-shared-vocabulary` — this skill does not write them. | ||
|
|
||
| ## Process Flow | ||
|
|
||
| ``` | ||
| orient (read CONTEXT.md and relevant ADRs) | ||
| → explore codebase for friction (optionally via Explore subagents) | ||
| → apply deletion test to suspect shallow modules | ||
| → present numbered candidates with locality / leverage / test impact | ||
| → user picks one? no → end (note for later) | ||
| yes → classify dependencies | ||
| (in-process / local-sub / remote-owned / true-external) | ||
| → interface shape obvious? yes → hand off to grilling-plans | ||
| no → Design It Twice | ||
| → hand off to grilling-plans | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Convert process steps into checklist-friendly instructions.
The process is clear, but it isn’t presented as copyable checklists for multi-stage execution. Please add explicit checklist blocks (- [ ]) for the main phases. As per coding guidelines, “Break complex operations into sequential steps with checklists Claude can copy and track progress through.”
🧰 Tools
🪛 LanguageTool
[style] ~60-~60: To elevate your writing, try using a synonym here.
Context: ...ir seams? - Which parts are untested or hard to test through their current interface...
(HARD_TO)
[style] ~108-~108: Using “real” as an adverb is considered informal. Consider using “really” or “very”.
Context: ...R, only surface it when the friction is real enough to warrant reopening the decisio...
(REAL_REALLY)
🪛 markdownlint-cli2 (0.22.1)
[warning] 70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 87-87: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 137-137: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/skill-set/skills/improving-architecture/SKILL.md` around lines 46 -
149, Convert each main phase heading ("1. Orient", "2. Explore", "3. Present
candidates", "4. Classify dependencies", "5. (Optional) Design It Twice", "6.
Hand off") and the "Process Flow" into checklist-style steps by inserting
explicit markdown task items (- [ ] ...) that enumerate the sequential actions
to perform; for each phase (e.g., "Present candidates") add a top-level
checklist item and nested checklist items for its substeps (e.g., read
CONTEXT.md, run deletion test, prepare numbered candidates, ask user), ensure
the filled example stays as an example but add a checklist prompting the author
to fill Files/Problem/Solution/Locality gain/Leverage gain/Test impact, and add
a final checklist in "Process Flow" that reflects the decision branches (user
picks one? yes/no → next steps) so reviewers can copy/paste and tick progress
while following the existing prose in SKILL.md.
There was a problem hiding this comment.
Skipping this suggestion. The numbered ### N. Phase form is consistent with other skills in this repo (shipping-pr/SKILL.md, creating-skills/SKILL.md, developing-test-first/SKILL.md). Switching to checklists in one skill would diverge from the project's authoring style; if desired, it should be a project-wide stylistic decision applied uniformly.
There was a problem hiding this comment.
@ether-moon, that's a fair point — consistency within a project's authoring style takes precedence over per-file deviations. Acknowledged, and I'll keep this convention in mind for future reviews of skills in this repo.
✏️ Learnings added
Learnt from: ether-moon
Repo: ether-moon/skill-set PR: 21
File: plugins/skill-set/skills/improving-architecture/SKILL.md:46-149
Timestamp: 2026-05-04T09:41:27.059Z
Learning: In the `ether-moon/skill-set` repository, skill SKILL.md files use a numbered `### N. Phase` heading structure (not markdown checklist items `- [ ]`) for process steps. This is the established authoring convention across skills such as `shipping-pr/SKILL.md`, `creating-skills/SKILL.md`, and `developing-test-first/SKILL.md`. Do not suggest converting process phases to checklists in individual skill files — any such change would need to be applied project-wide.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| - Where do tightly-coupled modules leak across their seams? | ||
| - Which parts are untested or hard to test through their current interface? | ||
|
|
||
| For broader sweeps, dispatch an `Explore` agent (`Agent` tool with `subagent_type=Explore`) to walk a directory or feature area in parallel — see `superpowers:dispatching-parallel-agents`. |
There was a problem hiding this comment.
Use a fully qualified MCP tool name.
Agent is referenced without a server prefix. Please use the server-qualified form expected by the repo convention. As per coding guidelines, “Use fully qualified MCP tool names with server prefix (e.g., ServerName:tool_name...).”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/skill-set/skills/improving-architecture/SKILL.md` at line 62, The
reference to the MCP tool `Agent` must use the repository's server-qualified
naming convention; update the text that says "dispatch an `Explore` agent
(`Agent` tool with `subagent_type=Explore`)" to use the fully qualified tool
name (e.g., replace `Agent` with the server-prefixed form like
`ServerName:Agent`) and ensure any inline code or example that mentions the tool
follows that same pattern; keep the `Explore` subagent_type as-is but show it in
the context of the server-qualified tool (e.g., `ServerName:Agent` with
`subagent_type=Explore`) and update the related reference to
`superpowers:dispatching-parallel-agents` if it includes unqualified tool
mentions.
There was a problem hiding this comment.
Skipping this suggestion. Agent is a Claude Code built-in tool, not an MCP server tool. The AGENTS.md MCP-naming rule ("ServerName:tool_name") applies only to MCP-provided tools. The same unprefixed reference exists in plugins/skill-set/skills/shipping-pr/reference/blocker-resolution.md already on main, confirming the convention.
There was a problem hiding this comment.
@ether-moon, thanks for the clear explanation — you're right, and I was mistaken. The ServerName:tool_name convention applies only to MCP-provided tools, not to Claude Code built-in tools like Agent. The existing usage in plugins/skill-set/skills/shipping-pr/reference/blocker-resolution.md on main is a solid confirmation of the established convention. I'll disregard this comment.
✏️ Learnings added
Learnt from: ether-moon
Repo: ether-moon/skill-set PR: 21
File: plugins/skill-set/skills/improving-architecture/SKILL.md:62-62
Timestamp: 2026-05-04T09:41:27.699Z
Learning: In the `ether-moon/skill-set` repository, `Agent` (with `subagent_type=Explore` etc.) is a Claude Code built-in tool, NOT an MCP server tool. The `ServerName:tool_name` fully-qualified naming rule defined in AGENTS.md applies only to MCP-provided tools. Built-in Claude Code tools like `Agent` should remain unprefixed. Confirmed by existing usage in `plugins/skill-set/skills/shipping-pr/reference/blocker-resolution.md` on main.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Add `text` language identifier to four fenced code blocks flagged by markdownlint rule MD040 (fenced-code-language): - SKILL.md:70 — candidate template block - SKILL.md:87 — filled candidate example block - SKILL.md:137 — process-flow diagram block - reference/deepening.md:45 — strategy-selection diagram block Addresses CodeRabbit OBVIOUS review comments on PR #21.
Removes Korean-language trigger phrases from descriptions and "When to Use"
sections of the four new skills, per AGENTS.md repository language policy
("All repository content MUST be written in English by default"). Korean
runtime triggers were CodeRabbit review feedback on PR #21.
Affected skills:
- grilling-plans
- building-shared-vocabulary
- zooming-out-on-code
- improving-architecture
ship cycle 1 summaryPre-cycle HEAD: Applied
Skipped (with rationale)
Entering cycle 2 to wait for CodeRabbit incremental review on |
ship cycle 2 summary — convergence reachedPre-cycle HEAD: State
OutcomePR is ready to merge. The two open threads are intentional skip decisions with documented rationale on the threads themselves; reviewer can mark resolved as part of approval. shipping-pr terminating with clean exit (cycle 2/3, max-cycles not exhausted). |
1 participant
Summary
Adds four new skills inspired by mattpocock/skills (MIT) and boosts
developing-test-firstwith a Horizontal Slicing anti-pattern.Changes
New skills
CONTEXT.mdand architecture decision records indocs/adr/as living artifacts. Lazy file creation, inline updates, three-criterion ADR gate (hard-to-reverse + surprising-without-context + real trade-off).grilling-plansfor the chosen candidate's design. Includes a Design It Twice parallel sub-agent pattern.Existing skill boost
New slash commands
/skill-set:plan:grill→grilling-plans/skill-set:code:zoom-out→zooming-out-on-codeMeta
Review history
This branch went through three audit rounds before submission:
skill-set:creating-skillsaudit (4 parallel subagents) — caught voodoo constants, trigger over-breadth, missing TOC, dot-block token tax, missing recursion terminator. All 11 OBVIOUS items auto-fixed; 4 AMBIGUOUS groups user-approved (CONTEXT.md fallback wording, dot-block ASCII compression, worked examples, minor wording).skill-creator:skill-creatoraudit (4 parallel subagents) — flagged additional voodoo constants, MUST-without-why framing, gating-rather-than-pushy descriptions. All addressed in the B-group fixes. Evals/scripts deferred for skill-set-wide consistency.Test plan
/skill-set:plan:grilltriggersgrilling-planswith the new pushy description/skill-set:code:zoom-outtriggerszooming-out-on-codegrilling-planswithoutsimplifycompetingimproving-architectureCONTEXT.md, all three CONTEXT.md-aware skills (grilling-plans, zooming-out-on-code, improving-architecture) gracefully fall back to inferring vocabularydeveloping-test-firststill triggers on "TDD", "red/green", and shows the new Horizontal Slicing sectionSummary by CodeRabbit
Release Notes v1.12.0
New Features
/skill-set:plan:grilland/skill-set:code:zoom-outslash commandsDocumentation