Skip to content

Commit 5bbf71a

Browse files
wesrisenmay-mxwesrisenmay-mx
committed
added a review skill to ensure a PR is following the ADRs
1 parent e27f219 commit 5bbf71a

2 files changed

Lines changed: 228 additions & 0 deletions

File tree

‎.claude/skills/adr-review/SKILL.md‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
---
2+
name: adr-review
3+
description: Review a pull/merge request's new code against the connect-widget Architecture Decision Records (ADRs). Use when asked to "ADR review", "review this PR against the ADRs", "check ADR compliance", or before approving a PR/MR in the connect-widget (GitHub) or the sibling GitLab repo that follows the same standards. Reviews styling (CSS Modules / MUI), testing (Vitest/MSW/Cypress), folder structure (screaming architecture), PR size, and undocumented architecture choices.
4+
---
5+
6+
# ADR Review
7+
8+
Review the **new code** in a pull request (GitHub) or merge request (GitLab) against
9+
the connect-widget Architecture Decision Records and report any violations.
10+
11+
The connect-widget ADRs are the single **canonical** source of truth. Two repos are
12+
expected to follow them: `connect-widget` (GitHub) and a sibling GitLab repo. This
13+
skill can review either — it always evaluates against the connect-widget ADRs.
14+
15+
## Scope: what to review
16+
17+
Review **only the code the PR adds or changes** (the diff), not the whole repo. ADR
18+
0002 states that a PR is judged on whether its *new* code adheres to the ADRs; you
19+
are not auditing pre-existing code except where the PR modifies it. When a PR edits a
20+
line that was already non-conforming, note that conforming it would be ideal but is
21+
not blocking unless the PR is making that area worse.
22+
23+
## Step 1 — Load the canonical ADRs (always do this first)
24+
25+
The ADRs evolve, so read them live rather than relying on this skill's summary. Load
26+
them from the connect-widget repo, trying these sources in order until one works:
27+
28+
1. If `architectureDecisionRecords/` exists in the current working directory (you are
29+
in the connect-widget repo), read every `*.md` file in it.
30+
2. If the env var `CONNECT_WIDGET_ADR_PATH` is set, read the `*.md` files there.
31+
3. If a local connect-widget checkout is known, read its `architectureDecisionRecords/`.
32+
4. Fall back to fetching them from GitHub (works from any repo, e.g. the GitLab one):
33+
```bash
34+
gh api repos/mxenabled/connect-widget/contents/architectureDecisionRecords \
35+
--jq '.[] | select(.name|endswith(".md")) | .name' \
36+
| while read -r f; do
37+
echo "===== $f =====";
38+
gh api "repos/mxenabled/connect-widget/contents/architectureDecisionRecords/$f" \
39+
--jq '.content' | base64 --decode;
40+
done
41+
```
42+
43+
Read `reference/adr-checklist.md` (next to this file) for the distilled, checkable
44+
rules. The live ADR files win if they ever disagree with the checklist — if you spot
45+
drift, mention it.
46+
47+
## Step 2 — Determine the target and get the diff
48+
49+
Detect the platform from the git remote (`git remote -v`): `github.com` → GitHub,
50+
`gitlab` in the host → GitLab.
51+
52+
Figure out what the user wants reviewed, in this priority:
53+
54+
- **Explicit PR/MR number or URL** in the request → fetch that.
55+
- GitHub: `gh pr diff <number>` and `gh pr view <number> --json title,body,files,baseRefName,additions,deletions`
56+
- GitLab (if `glab` is installed): `glab mr diff <number>` and `glab mr view <number>`
57+
- GitLab (no `glab`): tell the user glab isn't installed and fall back to the local
58+
diff below, or ask them to check out the MR branch.
59+
- **A branch/PR is open for the current branch** → `gh pr view --json ...` + `gh pr diff`.
60+
- **Otherwise review the current branch** against its base:
61+
```bash
62+
base=$(gh pr view --json baseRefName -q .baseRefName 2>/dev/null \
63+
|| git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null | sed 's@^origin/@@' \
64+
|| echo master)
65+
git fetch -q origin "$base" 2>/dev/null || true
66+
git diff "origin/$base...HEAD" --stat
67+
git diff "origin/$base...HEAD"
68+
```
69+
70+
Also gather the list of changed files (`--name-status`) and the added-lines only
71+
(`git diff ... --unified=0`) so you can cite precise `file:line` locations.
72+
73+
If you cannot obtain a diff, stop and tell the user what's missing (e.g. wrong repo,
74+
private MR needing glab auth) rather than reviewing nothing.
75+
76+
## Step 3 — Review the diff against each ADR
77+
78+
Go through the checklist in `reference/adr-checklist.md`. For each added/changed hunk,
79+
check every applicable ADR. Only flag things you can point to in the diff. Prefer
80+
being specific and actionable over exhaustive nitpicking.
81+
82+
Assign each finding a severity:
83+
- **Blocking** — clearly violates an ADR's decision (would fail review per ADR 0002).
84+
- **Should fix** — likely violation or strongly discouraged pattern; confirm intent.
85+
- **Consider** — judgment call, style, or a heads-up (e.g. PR getting large).
86+
87+
For anything ambiguous (folder-structure judgment calls, "is this new code or a hotfix"),
88+
say why it's ambiguous rather than asserting a violation.
89+
90+
## Step 4 — Report the findings in chat
91+
92+
Print a structured report. Do **not** post to the PR/MR unless the user later asks.
93+
94+
Format:
95+
96+
```
97+
# ADR Review — <PR title / branch> (<N files, +X/-Y>)
98+
99+
**Verdict:** <Conforms ✅ | Changes needed ⚠️ | Blocking issues ❌>
100+
101+
## Blocking
102+
- **[ADR 0001 Styling]** `src/Foo/Foo.tsx:42` — Uses `sx` prop for styling.
103+
→ Move to a CSS Module; `sx`/`xs` are only allowed for breakpoint-specific code.
104+
105+
## Should fix
106+
- ...
107+
108+
## Consider
109+
- ...
110+
111+
## Notes
112+
- <ADR drift, hotfix exceptions, or "no test file added for new component", etc.>
113+
```
114+
115+
If everything conforms, say so plainly and list what you checked so the user has
116+
confidence the review was real. If nothing in the diff is in scope for a given ADR
117+
(e.g. no styling changes), note that you checked and it didn't apply.
118+
119+
## Notes & exceptions
120+
121+
- **Hotfix exception (ADR 0002):** urgent production hotfixes may bypass the ADRs but
122+
must be followed by a conforming PR. If the PR looks like a hotfix, flag violations
123+
as "acceptable only if this is an urgent hotfix — file a follow-up ticket."
124+
- **Legacy code:** the repo is mid-migration (e.g. `@kyper/*` → MXUI). Editing legacy
125+
files doesn't require rewriting them, but new code must conform.
126+
- Keep the review grounded in the *diff* — never invent violations you can't cite.

‎.claude/skills/adr-review/reference/adr-checklist.md‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
# ADR Compliance Checklist
2+
3+
Distilled, checkable rules from the connect-widget ADRs. The **live ADR markdown files
4+
are authoritative** — if this checklist disagrees with them, follow the ADRs and note
5+
the drift. Each rule lists what to look for in a diff and how confidently it can be
6+
flagged from static inspection.
7+
8+
Cite the ADR number in every finding (e.g. `[ADR 0001 Styling]`).
9+
10+
---
11+
12+
## ADR 0001 — Styling our HTML
13+
14+
Decision: style with **CSS Modules**; use **MUI `Stack`** for spacing/layout.
15+
16+
Check added/changed `.tsx`/`.jsx`/`.css` code:
17+
18+
- **CSS Modules required.** New stylesheets must be `*.module.css` and imported as a
19+
module (`import styles from './Foo.module.css'`). Flag new plain `.css`/global CSS
20+
files, or Tailwind / other global CSS-framework classes, or styled-components.
21+
- **No `sx` prop for styling.** Flag `sx={...}` on MUI/MXUI components. *Exception:*
22+
`xs` is allowed **only** for breakpoint-specific code (MUI doesn't expose breakpoints
23+
as CSS variables). Ordinary styling via `sx`/`xs` → move to a CSS Module.
24+
- **Spacing between elements → MUI `<Stack spacing={n}>`.** Flag margins/padding added
25+
purely to space sibling elements when a `Stack` would be idiomatic. (Judgment call —
26+
mark as "Consider" unless obvious.)
27+
- **On a `Stack`, don't use `gap` or `flexDirection` props.** Use `spacing` and
28+
`direction` instead. Flag `<Stack ... gap=` and `<Stack ... flexDirection=`. Other
29+
flexbox props directly on `Stack` are fine.
30+
31+
Related lint (not an ADR, but reinforces intent): `.eslintrc.cjs` restricts some
32+
`@kyper/*` and `@mui/material/TextField` imports (use `src/privacy/input`). New
33+
`@kyper/*` usage is discouraged (migrate to MXUI) per project docs.
34+
35+
---
36+
37+
## ADR 0002 — Document architecture decisions
38+
39+
Decision: new code must adhere to the ADRs; significant technical choices need an ADR.
40+
41+
- **PR-blocking rule:** new code that violates any ADR should not be approved (unless
42+
it's an urgent hotfix, which must be followed by a conforming PR).
43+
- **New library / major pattern without an ADR.** If the diff adds a dependency to
44+
`package.json` or introduces a notably new architectural pattern (new state lib, new
45+
styling approach, new test framework, etc.), check whether a supporting ADR exists.
46+
If not, flag: "introduces <X>; ADR 0002 expects significant choices to be documented."
47+
- **Non-conforming code being modified:** ADR 0002 says to conform it if feasible, or
48+
at minimum file a ticket and add tests covering the new code. Note this when a PR
49+
touches non-conforming areas without doing either.
50+
51+
---
52+
53+
## ADR 0003 — Automated testing (frontend)
54+
55+
Decision: **Vitest** (unit/integration), **MSW** (API mocking), **Cypress** (e2e).
56+
Prefer integration tests; mock as little as possible; render real components.
57+
58+
- **New code should have tests.** Flag new components/hooks/util modules added without
59+
a corresponding `*.test.ts(x)` (or `*.cy.ts` for e2e) in the same PR. Colocated test
60+
next to source is expected (see ADR 0004).
61+
- **Prefer integration over heavy mocking.** Flag heavy use of `vi.mock(...)` to stub
62+
out real components/modules — the ADR prefers rendering real components so context
63+
and side-effects are wired. Mark as "Should fix" / "Consider" with a note.
64+
- **Use MSW for API mocking.** Flag tests that mock `fetch`/`axios` directly (e.g.
65+
`vi.fn()` on the network, `global.fetch = ...`) instead of MSW handlers.
66+
- **Right tool for the layer:** many edge cases belong in Vitest integration tests
67+
(with MSW), not Cypress. e2e is for verifying frontend↔backend/API wiring.
68+
69+
---
70+
71+
## ADR 0004 — Folder structure (screaming architecture)
72+
73+
Decision: organize by **domain**, not by framework/technical type. Keep files that are
74+
used together in close proximity; move code to `shared/` only once actually shared.
75+
76+
- **New files organized by domain.** A new feature's component, `api.ts`, and tests
77+
should live together in a domain folder (e.g. `src/Institutions/Institution/...`),
78+
not scattered across generic technical folders. Flag new files dropped into generic
79+
buckets (`components/`, `hooks/`, `utils/`, `redux/`) purely by file type when a
80+
domain folder would be clearer. (Judgment call — the existing repo predates this ADR,
81+
so weigh against surrounding structure; mark most as "Consider"/"Should fix".)
82+
- **Colocation.** Tests and `api.ts` live next to the code they cover, not in a
83+
separate mirror tree. Flag new tests placed far from their subject.
84+
- **`shared/` is for genuinely shared code.** Flag brand-new code placed directly in
85+
`shared/` that only one domain uses (premature sharing).
86+
87+
Because the current repo is mid-migration, treat structure findings as guidance for
88+
*new* domains/files rather than demanding relocation of existing ones.
89+
90+
---
91+
92+
## ADR 0005 — Small pull requests
93+
94+
Decision: strive for small, focused PRs that serve a single purpose.
95+
96+
- **Size heuristic.** Consider flagging when the diff is large or unfocused, e.g.
97+
roughly >~400 changed lines or >~15–20 files of production code, OR the PR clearly
98+
bundles unrelated concerns (e.g. a refactor + a feature + a dependency bump).
99+
- Mechanical/generated changes (lockfiles, snapshots, i18n) don't count against size
100+
the same way — note them separately.
101+
- This is almost always a **"Consider"** (advisory), not blocking. Frame it as "could
102+
this be split?" and point to the distinct concerns you see.

0 commit comments

Comments
 (0)