Skip to content

feat: add design-review Claude Code skill #884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ravisuhag wants to merge 1 commit into
base: main
Choose a base branch
from
+134 −0
Open

feat: add design-review Claude Code skill #884

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 134 additions & 0 deletions .claude/skills/design-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
---
name: design-review
description: Review the design of a change or a codebase — architecture, API surface, and whether the machinery fits the problem — rather than code quality. Use to analyse a PR's design, self-review your own branch before opening a PR, or audit an existing module or package. For bug-hunting line review use /review or /code-review instead.
---

# Design Review

Review a design: what it exposes, what it costs to maintain, and
whether the machinery fits the size of the problem. The same questions
apply whether you are reviewing someone else's PR, your own work before
you open one, or a module that already shipped. Only the input and the
output change.

## Phase 0 — Pick the mode

- **Reviewer mode**: the target is a PR. Output is a proposal comment
on the PR.
- **Author mode**: the target is your own branch or working tree,
before the PR exists. Output is fixes applied now, plus material for
the PR description. Cheapest time to run this review — every cliff
found here is one no reviewer has to argue about.
- **Audit mode**: the target is an existing module, package, or public
API. Output is a written report with ranked proposals.

## Phase 1 — Understand, then explain simply

Get the real code, not a description of it:

- Reviewer: `gh pr view <n> --json title,body,files,additions,deletions`
and `git fetch origin pull/<n>/head:pr-<n>`.
- Author: diff against the merge base
(`git diff $(git merge-base HEAD main)...HEAD`).
Comment on lines +29 to +32

@coderabbitai coderabbitai Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Bind validation to the ref selected in Phase 1.

Reviewer mode fetches pr-<n>, while author mode assumes a branch named main. Phase 2 then refers to the “target ref” without defining it. A reviewer can inspect the wrong revision. Define TARGET_REF for each mode and use it in every git show and git grep command. Resolve the repository default branch instead of hardcoding main.

Also applies to: 41-45

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/skills/design-review/SKILL.md around lines 29 - 32, Define a
mode-specific TARGET_REF in Phase 1: use the fetched pr-<n> ref for reviewer
mode and resolve the repository’s default branch for author mode instead of
assuming main. Update all Phase 2 git show and git grep commands to use
TARGET_REF consistently, including the related lines 41–45.

- Audit: start from the entry points — `package.json` `exports`, the
barrel files, the README.
Comment on lines +33 to +34

@coderabbitai coderabbitai Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Make audit entry-point discovery language-agnostic.

Audit mode hardcodes package.json and barrel files. This does not work reliably for Python, Rust, Go, or Java modules. Start with repository metadata and language-specific entry points when present. Treat package.json and barrel files as conditional examples.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/skills/design-review/SKILL.md around lines 33 - 34, Update the audit
guidance in the design-review skill to begin entry-point discovery from
repository metadata and language-specific entry points, supporting Python, Rust,
Go, Java, and other module layouts. Make package.json exports and barrel files
conditional examples rather than universal starting points, while preserving the
existing README check where appropriate.


Before any judgment, explain in plain terms: what problem does this
solve, and what is the approach? If you cannot explain it simply, you
do not understand it yet. In author mode, write this explanation down —
it becomes the PR description's opening.

## Phase 2 — Verify every quantitative claim

Claims come from the PR description, the design doc, the README — or,
in author mode, from your own head. All get checked against the code
(`git show`, `git grep` on the target ref).

The key split: what does the code use internally vs what does it
export for consumers? The gap between those two numbers is usually
where the design question lives.

## Phase 3 — Interrogate the API surface

For each public export ask:

- Who needs this, and what breaks for them if it goes away?
- What is the maintenance contract? Every exported name is API to keep
stable forever.
- **Find the cliff**: what happens to the user who needs one more thing
than the curated set provides? Check the escape hatch actually exists
— is the factory/helper exported? Are the types open or a closed
union? Read the entry-point file, not the docs.

## Phase 4 — The value question

What does this layer add over the underlying dependency? If a consumer
bypassed it and used the dependency directly, what would they lose?
If the answer is "consistency and a stable contract", say exactly that
— do not inflate it into capability the layer doesn't have.

## Phase 5 — Right-size the machinery

Does implementation complexity match problem size? Any supporting
machinery — extra build steps, generated files, custom tooling or
checks — earns its keep at scale N; ask what N actually is here. A
plain, committed artifact that existing tooling (compiler, tests) can
check often replaces all of it. Rule of thumb: machinery that exists
to avoid writing K lines by hand is only justified when K is large.

## Phase 6 — Stress-test the alternative against the roadmap

Before proposing something simpler, check it still supports the known
future directions — ask the user what those are; in author mode, list
them yourself. A simplification that blocks the roadmap is not a
simplification. Prefer designs where the future feature "falls out for
free" from the same mechanism. Typing rule: narrow types can widen
later without breaking; wide types can never narrow back. When unsure,
ship narrow.
Comment on lines +85 to +87

@coderabbitai coderabbitai Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "== file context =="
sed -n '1,140p' .claude/skills/design-review/SKILL.md

echo
echo "== search for typing/compatibility rules in repo =="
rg -n "narrow|wide|compatibility|public type|type.*rule|union|switch|exhaustive" .claude/skills/design-review/SKILL.md .cl

Repository: raystack/apsara

Length of output: 6479

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "== search public typing guidance repo-wide =="
rg -n -i "narrow types can widen|wide types can never|typing rule|type compatibility|unstable|compatibility|exhaustive|discriminated union" .

echo
echo "== repo files under .claude =="
find .claude -maxdepth 4 -type f 2>/dev/null | sort

Repository: raystack/apsara

Length of output: 4100

🌐 Web query:

TypeScript discriminant union exhaustiveness adding new union member breaks switch consumers

💡 Result:

It is intended behavior for TypeScript to flag an error when a new member is added to a union type that is used in a switch statement, provided you have implemented an exhaustive check pattern [1][2][3]. This is a powerful feature that turns potential runtime bugs—such as unhandled cases—into compile-time errors [2][4][5]. The standard approach to enforce this is to leverage the never type in the default clause of your switch statement [1][6][5]. Because TypeScript narrows the type of the union to only those members not yet handled in the preceding case blocks, if you have covered every member, the type in the default block will be never [2][3][4]. There are two primary ways to implement this: 1. Exhaustive Assignment: Assign the switch value to a variable typed as never [7][2][5]. typescript function handle(u: MyUnion) { switch (u.kind) { case 'A': /* ... */ break; case 'B': /* ... */ break; default: const _exhaustiveCheck: never = u; // Error here if new union member is not handled return _exhaustiveCheck; } } 2. Exhaustive Assertion Function: Pass the switch value to a function that accepts only never [1][6][5]. typescript function assertNever(x: never): never { throw new Error("Unexpected object: " + x); } function handle(u: MyUnion) { switch (u.kind) { case 'A': /* ... */ break; case 'B': /* ... */ break; default: return assertNever(u); // Error here if new union member is not handled } } When you add a new member to the union, TypeScript's control flow analysis will no longer resolve the type in the default case to never (it will include the new member) [1][3]. Since the new member is not assignable to never, the compiler will immediately highlight the default clause as an error, identifying the missing case [1][2][8]. Additionally, you can use the @typescript-eslint/switch-exhaustiveness-check rule to enforce this pattern via linting [6][9].

Citations:

🌐 Web query:

TypeScript return position contravariance function parameter input position variant

💡 Result:

In TypeScript, function types exhibit different variance behaviors based on the position of the type parameters [1][2]. Function parameter (input) positions are contravariant, while return type (output) positions are covariant [3][2]. 1. Contravariance in Input Positions Function parameters are contravariant, meaning a function type (A) => void is assignable to (B) => void if B is a subtype of A (or conversely, if the parameter type A is a supertype of B) [4][5][2]. This is because a function that accepts a broader type (e.g., Animal) is safer than one that requires a narrower type (e.g., Dog). If you expect a function that can handle a Dog, you can safely provide a function that handles any Animal, as it will know how to process a Dog if one is passed to it [5][2]. 2. Covariance in Return Positions Return types are covariant, meaning a function type => B is assignable to => A if B is a subtype of A [2]. This is intuitive: if a consumer expects a function to return an Animal, providing a function that returns a Dog is safe because a Dog is an Animal [2]. 3. Strict Function Types Historically, TypeScript treated function parameters bivariantly for flexibility, but the introduction of the --strictFunctionTypes compiler flag (now enabled by default in --strict mode) enforces strict contravariance for function parameters [6][7]. Methods and constructors are a notable exception to this rule and are still checked bivariantly to ensure generic classes and interfaces, like Array, remain usable [6][7]. 4. Variance Annotations Since TypeScript 4.7, developers can use optional variance annotations (in, out) on generic type parameters to explicitly control or document variance [8][9]. - out T: Indicates the type parameter is covariant (used in output positions) [8][9]. - in T: Indicates the type parameter is contravariant (used in input positions) [8][9]. - in out T: Indicates the type parameter is invariant [8]. These annotations do not change structural behavior but can assist the compiler in checking and verifying variance in complex generic types [10][9].

Citations:

🌐 Web query:

TypeScript API type widening narrow widening breaking change public union member

💡 Result:

In TypeScript, concerns about "widening," "narrowing," and "public" members in the context of union types often relate to how the compiler maintains or loses type safety during exported destructuring or object manipulation. A notable issue involving exported discriminated unions was the loss of narrowed types when variables were destructured and exported [1]. Specifically, when variables were extracted from a discriminated union object and exported, the type information—which should have been narrowed based on the discriminant—was lost, causing the variables to revert to their un-narrowed, wider union types [1]. This was addressed and resolved in recent versions of TypeScript (e.g., via PR #59673) to ensure that exported destructured variables preserve their narrowed types [2][1]. Regarding the broader context of breaking changes and public API surface, tools that perform automated semantic versioning checks often categorize these behaviors differently [3][4]: 1. Narrowing of a return type or parameter is generally classified as a breaking change because it may restrict the inputs or outputs that consumers are expecting to handle [5][3][4]. 2. Widening of a return type or parameter (e.g., adding a new member to a union) is typically classified as a non-breaking (additive) change, as it allows existing consumers to continue working while accepting a broader range of values [5][3][4]. 3. The visibility of members in union types is also a historical area of interest; for example, it has been noted that union types may allow public access to members that are private in one of the constituent types [6]. This behavior is a fundamental aspect of how TypeScript resolves property access across diverse types in a union [6]. Finally, "widening" contexts—such as let variables, function returns, or object literals—can cause literal types to revert to their primitive supertypes (e.g., 'A' | 'B' widening to string) [7]. While this is generally by design, it can sometimes manifest as unexpected behavior in generic type inference [8][7].

Citations:

🌐 Web query:

TypeScript TypeScript API surface adding union member breaking type narrowing widening API compatibility

💡 Result:

Adding a new member to a union type in TypeScript is generally considered a breaking change when that union is used in a way that relies on exhaustiveness checking or specific narrowing logic [1][2]. While adding a new property to an object type is often non-breaking, modifying a union type changes the set of possible states a value can inhabit [1][3]. Breakage typically occurs in the following areas: Exhaustiveness Checking: If you use a switch statement or other narrowing pattern to handle all members of a union (often using a 'never' type in a default branch to ensure completeness), adding a new union member will cause the 'never' check to fail because the new member is unhandled [1]. This is a common and intentional safety feature to force developers to update their code when the API's contract changes [1]. Narrowing Logic: TypeScript's control flow analysis relies on narrowing the union based on discriminant properties or other type guards [1]. Adding a new member can alter how TypeScript performs this narrowing, sometimes leading to unexpected behavior if the new member overlaps with existing ones or if the narrowing logic is not exhaustive [4][5]. Type Inference and Widening: In generic contexts, adding members to a union can impact type inference [6][7]. TypeScript sometimes performs "widening" (e.g., converting a literal type to its primitive supertype) [8]. Changes to union members can affect the candidate set for type inference, potentially leading to a more general (widened) type being inferred than was previously the case, which might cause downstream type errors [6][7][8]. API Compatibility: From the perspective of library authors, changing a union type is considered a breaking change because it forces consuming code to handle the new member [2]. Strategies such as using interface merging, module augmentation, or providing optional properties are sometimes used to manage these changes without forcing an immediate break for all users [9]. For more information, see the official TypeScript documentation on Narrowing [1] and guidelines on semantic versioning for TypeScript types [2][3][10].

Citations:

Qualify the typing compatibility rule for public types.

Do not state that narrowing can happen back. Add a concrete exception: widening a public union by adding a member must be checked against exhaustive consumers and language-specific input/output variance rules.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/skills/design-review/SKILL.md around lines 85 - 87, Update the
typing compatibility guidance near the “narrow types can widen” rule to qualify
it for public types: explicitly state that adding a member to a public union
requires checking exhaustive consumers and language-specific input/output
variance rules, without implying that narrowing can later occur.


## Phase 7 — Ripple effects

If the proposal changes the design, follow it through docs, migration
guides, and tests. Ask: does the docs structure still match the new
center of gravity? (A smaller surface usually means less
documentation to maintain, not just less code.)

## Phase 8 — Deliver

**Reviewer mode** — a proposal comment on the PR. Draft in the
scratchpad and keep the file; the comment will be updated several
times via
`gh api repos/<owner>/<repo>/issues/comments/<id> -X PATCH -F body=@file`.

Structure:

1. Lead with the question ("Do we need to export all of this?") — no
praise preamble, no throat-clearing.
2. The proposal, numbered, with a short code sample showing the
user-facing result.
3. Implementation consequences (what machinery gets deleted).
4. Where this goes later (roadmap fit).
5. An honest "What we give up" close — every proposal costs something;
name it yourself before a reviewer does.

Show the draft to the user before posting; post only on approval. As
discussion continues, PATCH the same comment rather than posting new
ones — one coherent proposal, not a thread of fragments. Keep new
sections in reading order (insert before the trade-offs close, not
appended after it).

**Author mode** — apply what you agree with now, while it's cheap.
What you decide *not* to change goes into the PR description with its
reasoning: state the cliff and its escape hatch, the machinery
trade-off, and the "what we give up" yourself. A PR that answers the
design questions before they're asked reviews faster.

**Audit mode** — a report (markdown file or artifact) with the same
structure as the reviewer comment, but with proposals ranked by
value-for-effort, since nothing is gated on a merge.

## Phase 9 — Record

Save a memory note: the target, the proposal summary, links (comment
URL or report path), and status (awaiting author / accepted /
rejected / applied). Update it as things evolve.
Loading