Skip to content

Commit adbcdac

Browse files
vbudhramvbudhram
authored
Merge pull request #20217 from mozilla/add-fxa-skills
chore(repo): add FxA review skills and update PR template
2 parents a3ff313 + 28e9e53 commit adbcdac

5 files changed

Lines changed: 479 additions & 21 deletions

File tree

‎.claude/skills/fxa-review-quick/SKILL.md‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
---
2+
name: fxa-review-quick
3+
description: Fast single-pass FXA-specific commit review covering security, conventions, logic/bugs, tests, and migrations. No subagents — runs directly in the main context.
4+
allowed-tools: Bash, Read, Grep, Glob
5+
argument-hint: [commit-ref]
6+
user-invocable: true
7+
context: fork
8+
---
9+
10+
# FXA Quick Review
11+
12+
Review the most recent commit (or the commit specified in `$ARGUMENTS`) in a single pass, using FXA-specific knowledge.
13+
14+
## Step 1: Get Commit Info
15+
16+
```bash
17+
COMMIT_REF="${ARGUMENTS:-HEAD}"
18+
git show "$COMMIT_REF" --format="%H%n%an%n%ae%n%s%n%b"
19+
```
20+
21+
```bash
22+
COMMIT_REF="${ARGUMENTS:-HEAD}"
23+
git show --stat "$COMMIT_REF"
24+
```
25+
26+
## Step 2: Read Changed Files
27+
28+
Use Read and Grep to examine the changed files and their surrounding context. Look at imports, callers, and related types to understand the full picture before judging.
29+
30+
## Step 3: Review
31+
32+
Evaluate the diff through these lenses, in order of priority:
33+
34+
**1. Security**
35+
- Hardcoded secrets, injection (SQL/XSS/command), missing input validation, auth bypasses
36+
- Sensitive data in logs or error messages (PII: emails, UIDs, tokens) — note: UIDs and emails in API response bodies are expected, focus on logs and error messages
37+
- Missing rate limiting on new public endpoints
38+
- Session token handling that bypasses established Hapi auth schemes
39+
- New endpoints missing `Content-Type` validation
40+
- User-controlled input passed to Redis keys without prefix/namespace
41+
42+
**2. FXA Conventions**
43+
- Raw `Error` thrown in route handlers instead of `AppError` from `@fxa/accounts/errors`
44+
- `console.log` instead of the `log` object (mozlog format)
45+
- Cross-package imports using relative paths instead of `@fxa/<domain>/<package>` aliases
46+
- Circular or bi-directional dependencies between packages/libs — breaks build ordering
47+
- Auth-server code importing from `fxa-auth-server/**` (ESLint blocks this)
48+
- New code added to legacy packages (`fxa-content-server`, `fxa-payments-server`) — should be in `fxa-settings` or SubPlat 3.0
49+
- No new GraphQL — `fxa-graphql-api` was removed, `admin-server` GraphQL is legacy. Exception: CMS-related GraphQL.
50+
- Hardcoded values that should come from Convict config
51+
- New `require()` in `.ts` files — use `import` instead. Existing CJS patterns in auth-server `.js` files are fine.
52+
- Missing MPL-2.0 license header on new files
53+
- Prefer `async/await` over `.then()` promise chains
54+
- Flag new `Container.get()`/`Container.set()` usage — linting rules to disallow these are coming
55+
56+
**3. Logic & Bugs**
57+
- Missing `await` on async calls — note: some fire-and-forget patterns (metrics, logging) are intentional, check context before flagging
58+
- Null/undefined mishandling
59+
- Race conditions, shared mutable state
60+
- Swallowed errors (empty catch blocks, catch-and-rethrow without context)
61+
- Off-by-one, wrong comparisons, missing break/return in switch
62+
- Hapi route handlers that catch and re-throw instead of letting the error pipeline handle it
63+
64+
**4. Tests**
65+
- New auth-server source files without co-located `*.spec.ts`; fxa-settings uses `*.test.tsx` convention
66+
- `jest.clearAllMocks()` in `beforeEach` — unnecessary, `clearMocks: true` is global
67+
- `proxyquire` in new test code — should use `jest.mock()`
68+
- New Mocha tests in `test/local/` or `test/remote/` — new tests must be Jest
69+
- Over-mocked tests that only test mock wiring
70+
- Prefer `jest.useFakeTimers()` and `jest.setSystemTime()` over `setTimeout` or mocking `Date.now` directly
71+
- Flag patterns likely to cause open handle warnings (unclosed connections, uncleared timers)
72+
- Flag missing `act()` wrapping in React test state updates
73+
74+
**5. Database Migrations**
75+
- Edits to existing published migration files — CRITICAL, never allowed
76+
- New migration without corresponding rollback file
77+
- Verify test DB patches are aligned with current test DB state (`/fxa-shared/test/db/models/**/*.sql`)
78+
- `DELETE`/`UPDATE` without `WHERE` clause
79+
- `ALTER TABLE` on large tables without online DDL consideration
80+
- Index changes bundled with schema changes — should be separate migrations
81+
- Data type changes that could truncate data
82+
83+
**6. Migration Direction**
84+
- Mocha → Jest (no new Mocha tests)
85+
- `proxyquire` → `jest.mock()`
86+
- Callbacks → `async/await`
87+
- `fxa-shared` → `libs/*` (migration in progress, check both locations for existing code before adding new)
88+
89+
**7. AI Slop Detection**
90+
- Overly verbose or obvious comments that describe what the code does, not why
91+
- Unnecessary abstractions or helper functions for one-time operations
92+
- Excessive error handling for scenarios that cannot happen
93+
- Redundant validation or fallbacks that duplicate framework guarantees
94+
- Generic variable names or boilerplate patterns that suggest auto-generated code
95+
96+
## Step 4: Output
97+
98+
## Commit Summary
99+
100+
**Commit:** hash
101+
**Author:** name
102+
**Message:** commit message
103+
**Files Changed:** count
104+
105+
## Changes Overview
106+
107+
Write a brief summary of what the commit does based on the diff. Do not repeat the commit message.
108+
109+
## Issues Found
110+
111+
Use a table with columns: #, Severity, Category, File, Line, Issue, Recommendation.
112+
113+
Severity definitions:
114+
- CRITICAL — security vulnerabilities, data loss, auth bypasses, editing published migrations. Must fix.
115+
- HIGH — bugs that will cause production issues, missing auth schemes on routes. Should fix.
116+
- MEDIUM — convention violations, code quality, moderate risk. Consider fixing.
117+
- LOW — style, minor improvements. Optional.
118+
119+
If no issues are found, skip the table and write: "No issues found."
120+
121+
## Verdict
122+
123+
Recommendation: APPROVE, REQUEST CHANGES, or NEEDS DISCUSSION.
124+
125+
Include blocking issue count (CRITICAL + HIGH) and total issue count.
126+
127+
If clean: "This commit is ready to merge."
128+
If not: "Please address the CRITICAL and HIGH issues before merging."
129+
130+
## Guidelines
131+
132+
- Be pragmatic, not pedantic. Flag real problems, not style preferences.
133+
- Consider the context — read surrounding code before flagging something.
134+
- Do not flag missing tests for trivial changes (config values, enum additions, comment updates).
135+
- One or two missing edge-case tests is MEDIUM at most, not HIGH.
136+
- Always explain WHY something is a problem, not just what.
137+
- If the commit is clean, say so clearly and approve. A short review is a good review.

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

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
---
2+
name: fxa-review
3+
description: Thorough FXA-specific commit review using parallel specialist agents. Covers security, TypeScript, logic/bugs, test quality, and architecture. Agents explore call sites, git history, and monorepo conventions.
4+
allowed-tools: Bash, Read, Grep, Glob, Agent
5+
argument-hint: [commit-ref]
6+
user-invocable: true
7+
context: fork
8+
---
9+
10+
# FXA Thorough Review
11+
12+
Review the most recent commit (or the commit specified in `$ARGUMENTS`) using specialized parallel agents.
13+
14+
## Step 1: Get Commit Information
15+
16+
```bash
17+
COMMIT_REF="${ARGUMENTS:-HEAD}"
18+
git show "$COMMIT_REF" --format="%H%n%an%n%ae%n%s%n%b"
19+
```
20+
21+
```bash
22+
COMMIT_REF="${ARGUMENTS:-HEAD}"
23+
git show --stat "$COMMIT_REF"
24+
```
25+
26+
Save the full diff output and commit metadata. You will pass these to each agent in Step 2.
27+
28+
## Step 2: Spawn Parallel Review Agents
29+
30+
Launch ALL FIVE agents **in parallel** using the Agent tool in a **single message**. Each agent receives the full diff and commit metadata in its prompt.
31+
32+
Tell each agent to use Read/Grep/Glob to examine surrounding code in changed files for context. Provide the repo working directory path.
33+
34+
Each agent MUST output findings as a JSON array ONLY (no markdown, no extra text). Each item has: severity (CRITICAL/HIGH/MEDIUM/LOW), category, subcategory, file, line, issue, recommendation. If no issues, return: []
35+
36+
---
37+
38+
**Agent 1 — FXA Security Review**
39+
- subagent_type: general-purpose
40+
- model: opus
41+
- description: FXA security review
42+
43+
Tell this agent it is a senior security engineer. It should:
44+
45+
- Trace user input through changed code paths to check for injection (SQL, Redis, command, XSS)
46+
- Verify auth schemes on new route handlers — FXA auth-server uses 13 custom Hapi auth schemes, new routes must declare one
47+
- Check for PII exposure in logs and error messages (emails, UIDs, session tokens) — note: UIDs and emails in API response bodies are expected, focus on logs and error messages
48+
- Verify parameterized queries — no string concatenation in SQL or Redis commands
49+
- Check rate limiting on new public endpoints (customs server integration)
50+
- Verify session token rotation on privilege escalation (login, 2FA, password change)
51+
- Check CORS configuration — no `*` on credentialed endpoints
52+
- Verify OTP/TOTP handling: constant-time comparison, immediate invalidation, rate limiting
53+
- Check that secrets are accessed via Convict config, not hardcoded or read from env directly
54+
55+
Output JSON array with fields: severity, category ("Security"), subcategory, file, line, issue, recommendation.
56+
57+
---
58+
59+
**Agent 2 — FXA TypeScript Review**
60+
- subagent_type: general-purpose
61+
- model: opus
62+
- description: FXA TypeScript review
63+
64+
Tell this agent it is a senior TypeScript engineer familiar with FXA's mixed JS/TS codebase. It should:
65+
66+
- Flag `any` usage — suggest `unknown`, interfaces, or generics instead. Note: `any` is permitted in auth-server during migration but should not be introduced in new code unnecessarily
67+
- Check for non-null assertions (`!` postfix) — `@typescript-eslint/no-non-null-assertion: error`
68+
- Verify `as any` is justified — needed for CJS/ESM interop or type system limitations, not hiding real type issues
69+
- Check mock type safety — mocks should use `satisfies` against real interfaces where possible
70+
- Flag missing return types on exported functions
71+
- Look for simplification: optional chaining, nullish coalescing, early returns, destructuring
72+
- Check import style: `@fxa/<domain>/<package>` for cross-package, relative within package
73+
- Flag new `require()` in `.ts` files — use `import` instead. Existing CJS patterns in auth-server `.js` files are fine.
74+
- Prefer `async/await` over `.then()` promise chains
75+
- For `.js` files: review general code patterns, no TypeScript-specific feedback
76+
77+
Output JSON array with fields: severity, category ("TypeScript"), subcategory, file, line, issue, recommendation, code_before, code_after.
78+
79+
---
80+
81+
**Agent 3 — FXA Logic and Bugs Review**
82+
- subagent_type: general-purpose
83+
- model: opus
84+
- description: FXA logic and bugs review
85+
86+
Tell this agent it is a senior software engineer who knows FXA's auth-server patterns deeply. It should:
87+
88+
- Follow data flow through changed functions — read callers and callees to verify assumptions
89+
- Check async correctness: missing `await`, unhandled promise rejections, race conditions — note: some fire-and-forget patterns (metrics, logging) are intentional, check context
90+
- Verify error propagation: Hapi route handlers should throw, not catch-and-rethrow
91+
- Check `AppError` usage — HTTP errors must use `AppError` from `@fxa/accounts/errors`
92+
- Verify logging uses the `log` object (mozlog format), not `console.log`
93+
- Check edge cases: empty arrays, zero values, null UIDs, expired tokens
94+
- Flag new `Container.get()`/`Container.set()` usage — linting rules to disallow these are coming
95+
- Check Convict config access: verify `config.get('key')` keys exist in `config/index.ts`
96+
- Flag catch blocks that swallow errors or re-throw without context
97+
98+
Output JSON array with fields: severity, category ("Logic/Bugs"), subcategory, file, line, issue, recommendation.
99+
100+
---
101+
102+
**Agent 4 — FXA Test Quality Review**
103+
- subagent_type: general-purpose
104+
- model: opus
105+
- description: FXA test quality review
106+
107+
Tell this agent it is a QA engineer who understands FXA's testing patterns and common flakiness causes. It should:
108+
109+
- Check new auth-server source files have co-located `*.spec.ts`; fxa-settings uses `*.test.tsx` convention
110+
- Flag new Mocha tests — all new tests must be Jest
111+
- Flag `proxyquire` in new code — should use `jest.mock()`
112+
- Check mock quality: tests that only assert output matches what the mock returns (tautological)
113+
- Verify interaction assertions: `.toHaveBeenCalledWith()` on mocked dependencies, not just checking return values
114+
- Flag `jest.clearAllMocks()` in `beforeEach` — unnecessary, `clearMocks: true` is global
115+
- Check for shared mutable state between tests, missing `await`, `setTimeout` in tests
116+
- Prefer `jest.useFakeTimers()` and `jest.setSystemTime()` over `setTimeout` or mocking `Date.now` directly
117+
- Flag patterns likely to cause open handle warnings (unclosed connections, uncleared timers)
118+
- Flag missing `act()` wrapping in React test state updates
119+
- Flag over-mocking: mocking internal functions in the same package instead of at system boundaries
120+
121+
Output JSON array with fields: severity, category ("Test Quality"), subcategory, file, line, issue, recommendation.
122+
123+
---
124+
125+
**Agent 5 — FXA Architecture Review**
126+
- subagent_type: general-purpose
127+
- model: opus
128+
- description: FXA architecture review
129+
130+
Tell this agent it is a senior architect who knows FXA's monorepo structure and migration directions. It should:
131+
132+
- Check if new shared/reusable code belongs in `libs/*` or `fxa-shared` instead of app-local — search the monorepo for existing helpers before flagging. Note: `fxa-shared` is migrating to `libs/`, check both locations.
133+
- Detect duplication: search `libs/**`, `fxa-shared/**`, and `packages/**` for functions/types with the same name or purpose
134+
- Flag new code in legacy packages:
135+
- `fxa-content-server` — should be in `fxa-settings`
136+
- `fxa-payments-server` — should target SubPlat 3.0 (`libs/payments/*`, `apps/payments/*`)
137+
- No new GraphQL — `fxa-graphql-api` was removed, `admin-server` GraphQL is legacy. Exception: CMS-related GraphQL.
138+
- Verify database migrations:
139+
- Never edit existing published migration files
140+
- New migration has corresponding rollback
141+
- Sequential patch numbering has no gaps
142+
- Index changes separate from schema changes
143+
- Test DB patches aligned with current test DB state (`/fxa-shared/test/db/models/**/*.sql`)
144+
- Check cross-package boundary violations: relative imports across package boundaries instead of `@fxa/*` aliases
145+
- Check for circular or bi-directional dependencies between packages/libs — these break build ordering and are hard to untangle later
146+
- Flag colliding exports: new exports that duplicate existing ones elsewhere in the monorepo
147+
- For fxa-settings changes: check React and Tailwind best practices (component composition, accessibility, responsive design)
148+
- Also apply the code smell checks from the `/check-smells` skill (`.claude/skills/check-smells/SKILL.md`) — read that file and incorporate its design, implementation, test, and dependency smell checks into this review
149+
150+
**Migration direction compliance:**
151+
- Mocha → Jest (no new Mocha tests)
152+
- `proxyquire` → `jest.mock()`
153+
- Callbacks → `async/await`
154+
- `fxa-shared` → `libs/*` (in progress)
155+
156+
Output JSON array with fields: severity, category ("Architecture"), subcategory, file, line, issue, recommendation.
157+
158+
---
159+
160+
## Step 3: Aggregate and Report
161+
162+
After all agents return, parse each JSON output. Combine all findings into one list sorted by severity: CRITICAL then HIGH then MEDIUM then LOW. Number sequentially. If an agent returns malformed output, extract what you can and note the issue.
163+
164+
Deduplicate issues flagged by multiple agents (keep the more detailed one, note which reviewers flagged it).
165+
166+
Present the unified report:
167+
168+
**Commit Review Summary** — commit hash, author, message, files changed count.
169+
170+
**Changes Overview** — write your own summary of what the commit does from the diff (do not repeat the commit message).
171+
172+
**AI Slop Check** — flag overly verbose comments, unnecessary abstractions, excessive error handling for impossible cases, redundant validation, or other signs of auto-generated code.
173+
174+
**Issues Found** — markdown table with columns: #, Severity, Reviewer, Category, File, Line, Issue, Recommendation.
175+
176+
Severity definitions:
177+
- CRITICAL — security vulnerabilities, data loss, editing published migrations, auth bypasses. Must fix.
178+
- HIGH — bugs, missing auth schemes, unsafe type assertions hiding real issues. Should fix.
179+
- MEDIUM — convention violations, code quality, missing tests. Consider fixing.
180+
- LOW — style, minor improvements. Optional.
181+
182+
**Detailed Findings by Reviewer** — one subsection per reviewer (Security, TypeScript, Logic/Bugs, Test Quality, Architecture). Include code before/after examples for HIGH+ issues. If a reviewer found nothing, say "No issues found."
183+
184+
**Verdict** — APPROVE, REQUEST CHANGES, or NEEDS DISCUSSION. Include blocking issue count (CRITICAL + HIGH), total issue count, and which reviewers reported issues. If all agents returned empty arrays: "All specialist reviewers agree — this commit is ready to merge."
185+
186+
## Guidelines
187+
188+
- Always launch all 5 agents in parallel in a single message
189+
- Be thorough but fair — not every commit needs to be perfect
190+
- Focus on actual problems, not style preferences
191+
- Deduplicate cross-agent findings
192+
- Write the Changes Overview yourself from the diff
193+
- Include code examples for HIGH+ issues in the detailed findings

0 commit comments

Comments
 (0)