chore(repo): add FxA review skills and update PR template#20217
chore(repo): add FxA review skills and update PR template#20217
Conversation
There was a problem hiding this comment.
Pull request overview
Adds new .claude skills intended to help with FXA-specific simplification and PR/commit review, and updates the GitHub PR template to prompt for self-review and review guidance.
Changes:
- Update
.github/PULL_REQUEST_TEMPLATE.mdto add an “Author has self-reviewed” checkbox and a “How to review” section. - Add new Claude skills:
fxa-simplify,fxa-review, andfxa-review-quick.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| .github/PULL_REQUEST_TEMPLATE.md | Adds self-review checkbox and a placeholder “How to review” section. |
| .claude/skills/fxa-simplify/SKILL.md | New skill describing FXA-specific code simplification conventions and process. |
| .claude/skills/fxa-review/SKILL.md | New “thorough review” skill with parallel specialist-agent workflow. |
| .claude/skills/fxa-review-quick/SKILL.md | New “quick review” skill with a single-pass checklist workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| - [ ] If applicable, I have modified or added tests which pass locally. | ||
| - [ ] I have added necessary documentation (if appropriate). | ||
| - [ ] I have verified that my changes render correctly in RTL (if appropriate). | ||
| - [ ] Author has self-reviewed this PR. |
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| ## How to review | ||
|
|
||
| - |
| @@ -0,0 +1,119 @@ | |||
| --- | |||
| name: fxa-simplify | |||
| description: Simplifies and refines code in the FXA monorepo using project-specific conventions. Use when asked to simplify, clean up, or refine recently written code. Focuses on recently modified code unless instructed otherwise. | |||
| 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. | ||
| allowed-tools: Bash, Read, Grep, Glob, Agent | ||
| argument-hint: [commit-ref] | ||
| user-invocable: true |
| --- | ||
| name: fxa-review-quick | ||
| description: Fast single-pass FXA-specific commit review covering security, conventions, logic/bugs, tests, and migrations. No subagents — runs directly in the main context. | ||
| allowed-tools: Bash, Read, Grep, Glob | ||
| argument-hint: [commit-ref] | ||
| user-invocable: true | ||
| --- |
| --- | ||
| name: fxa-review-quick | ||
| description: Fast single-pass FXA-specific commit review covering security, conventions, logic/bugs, tests, and migrations. No subagents — runs directly in the main context. | ||
| allowed-tools: Bash, Read, Grep, Glob | ||
| argument-hint: [commit-ref] | ||
| user-invocable: true | ||
| --- |
|
|
||
| Evaluate the diff through these lenses, in order of priority: | ||
|
|
||
| **1. Security** |
There was a problem hiding this comment.
We already have a security skill. Maybe update that one instead?
There was a problem hiding this comment.
Turns out that skills can't invoke other skills, but they can reference similar ones. There is a lot of overlap between the two but I'll make this reference the other one.
|
|
||
| ## Step 2: Read Changed Files | ||
|
|
||
| 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. |
There was a problem hiding this comment.
I like this. I think all skills with this should be prefixed with review- since this clearly about code review.
|
|
||
| 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. | ||
|
|
||
| ## Step 3: Review |
There was a problem hiding this comment.
I think it'd be nice to have each sub section be its own skill. That way people can use them in more general ways too. For example apply them to a file that hasn't been changed yet. I'm not entirely sure about this, but I also think this will be faster if context: fork is used since each can be done with an agent thus parallelizing the process.
| - Over-mocked tests that only test mock wiring | ||
|
|
||
| **5. Database Migrations** | ||
| - Edits to existing published migration files — CRITICAL, never allowed |
There was a problem hiding this comment.
We also should be making sure the test db patches are aligned with the current test db state (ie /fxa-shared/test/db/models/**/*.sql). Personally I'd like to just do away with those at some point, but not sure exactly when that will happen.
|
|
||
| ## Verdict | ||
|
|
||
| Recommendation: APPROVE, REQUEST CHANGES, or NEEDS DISCUSSION. |
|
|
||
| ## Guidelines | ||
|
|
||
| - Be pragmatic, not pedantic. Flag real problems, not style preferences. |
There was a problem hiding this comment.
Can we add something about watch out for ai slop?
|
|
||
| --- | ||
|
|
||
| **Agent 1 — FXA Security Review** |
There was a problem hiding this comment.
Essentially same comments as above. I think these would be better as sub skills, and there is probably overlap with existing skills.
|
|
||
| Tell this agent it is a senior TypeScript engineer familiar with FXA's mixed JS/TS codebase. It should: | ||
|
|
||
| - 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 |
There was a problem hiding this comment.
I also think there's cases where for tests it's okay. @nshirley thoughts?
There was a problem hiding this comment.
Yeah, there are arguments for and against allowing any in tests. Using any can provide some flexibility and freedom in writing tests, and restricting it can sometimes make tests more difficult to write and read.
A few thoughts on both sides:
Allowing it can:
- make it easier to test runtime inputs and validation (e.g. deliberately passing malformed or malicious data)
- be useful when mocking third-party libraries or complex objects where strict typing adds noise but not value
Disallowing it can:
- prevent hiding incorrect assumptions in mocks
- avoid tests passing shapes that would never exist in real code
- preserve the contracts afforded by TypeScript, making refactors safer
Also, using satisfies more for mocks and fixtures could be really valuable here!
A good principle is: use any in tests only when the looseness is part of the test case, not just a shortcut.
Sorry, long way of saying — I mostly agree with this rule for the agent: flag it and suggest better types or more explicit casting where possible.
There was a problem hiding this comment.
One comment I have here, is I have noticed that Claude loves to set the Integration type to any, when we have an established pattern to create an integration subset, e.g. CompleteResetPasswordIntegration. Maybe we need a note for that at least to use type sub sets?
| - Check mock type safety — mocks should be typed against real interfaces where possible | ||
| - Flag missing return types on exported functions | ||
| - Look for simplification: optional chaining, nullish coalescing, early returns, destructuring | ||
| - Check import style: `@fxa/<domain>/<package>` for cross-package, relative within package |
There was a problem hiding this comment.
Also comment that we shouldn't be using require in TS files.
| - Check mock quality: tests that only assert output matches what the mock returns (tautological) | ||
| - Verify interaction assertions: `.toHaveBeenCalledWith()` on mocked dependencies, not just checking return values | ||
| - Flag `jest.clearAllMocks()` in `beforeEach` — unnecessary, `clearMocks: true` is global | ||
| - Check for shared mutable state between tests, missing `await`, `setTimeout` in tests |
There was a problem hiding this comment.
We should also have something about using jest.useFakeTimers and jest.setSystemTime instead of using setTimeout directly or mocking Date.now...
|
|
||
| --- | ||
|
|
||
| **Agent 5 — FXA Architecture Review** |
There was a problem hiding this comment.
Should we incorporate the check-smells skill here?
There was a problem hiding this comment.
I'll have it reference the skill here
.claude/skills/fxa-review/SKILL.md
Outdated
| - Check migration direction compliance: | ||
| - Mocha → Jest (no new Mocha tests) | ||
| - `proxyquire` → `jest.mock()` | ||
| - Callbacks → async/await |
There was a problem hiding this comment.
I don't consider this architecture.
| - Sequential patch numbering has no gaps | ||
| - Index changes separate from schema changes | ||
| - Check cross-package boundary violations: relative imports across package boundaries instead of `@fxa/*` aliases | ||
| - Flag colliding exports: new exports that duplicate existing ones elsewhere in the monorepo |
There was a problem hiding this comment.
We should also check for circular/bi-directional dependencies. Very important with library code for architectural cleanliness.
|
|
||
| ## 1. Preserve Functionality | ||
|
|
||
| Never change what the code does — only how it does it. All original features, outputs, and behaviors must remain intact. |
There was a problem hiding this comment.
I feel like if we are having AI do this, especially to existing code (there's no diff check on this skill), we should make sure there's test coverage to ensure that it really isn't the input output behavior of the code.
There was a problem hiding this comment.
I haven't noticed an issue of it introducing different behaviors but I've also run the /fxa-review skill and tests before pushing it out.
| ### Imports | ||
| - Use `@fxa/<domain>/<package>` path aliases for cross-package imports (e.g., `@fxa/accounts/errors`, `@fxa/shared/cloud-tasks`) | ||
| - Use relative imports within a package — auth-server ESLint enforces this (`@typescript-eslint/no-restricted-imports` blocks `fxa-auth-server/**`) | ||
| - `require()` is acceptable in auth-server (mixed JS/TS codebase, `@typescript-eslint/no-var-requires: off`) |
There was a problem hiding this comment.
Is it acceptable in TS files though?
| ### Naming & Structure | ||
| - Consistent naming: `camelCase` for variables/functions, `PascalCase` for classes/types/React components | ||
| - Auth-server factory pattern: modules often export `(log, config, db) => { ... }` or class constructors | ||
| - TypeDI `Container.set()`/`Container.get()` for dependency injection in auth-server |
There was a problem hiding this comment.
I don't quite understand how this would be applied? Like what would it changed based on the guidelines here and the line above?
| - TypeDI `Container.set()`/`Container.get()` for dependency injection in auth-server | ||
|
|
||
| ### Config | ||
| - Config is Convict-based (`config/index.ts`, ~2900 lines) — access via `config.get('key')` or `config.getProperties()` |
There was a problem hiding this comment.
What is ~2900 lines? Also we have a ticket to remove convict...
.claude/skills/fxa-simplify/SKILL.md
Outdated
|
|
||
| ## 6. FXA-Specific Guardrails | ||
|
|
||
| - **Never edit published DB migration files** — migrations are append-only |
There was a problem hiding this comment.
I feel like this kind of stuff belongs in our main claude md.
| - `console.log` instead of the `log` object (mozlog format) | ||
| - Cross-package imports using relative paths instead of `@fxa/<domain>/<package>` aliases | ||
| - Auth-server code importing from `fxa-auth-server/**` (ESLint blocks this) | ||
| - New code added to legacy packages (`fxa-content-server`, `fxa-payments-server`) — should be in `fxa-settings` or SubPlat 3.0 |
There was a problem hiding this comment.
Also anything in fxa-shared should be in libs/.
| - Hardcoded values that should come from Convict config | ||
| - `require()` vs `import` — CJS in auth-server is fine, ESM in libs/settings | ||
| - Missing MPL-2.0 license header on new files | ||
|
|
There was a problem hiding this comment.
Also add somewhere that:
- Using non-semantic elements to replicate native HTML controls instead of using the appropriate built-in elements (e.g.,
<ul>/<li>,<button>,<select>) - React code that directly manipulates the DOM. There are edge cases where this is OK but it should be questioned (something about following best React practices)
- UI modifications that are not also reflected in Storybook updates
As Dan has sort of said, maybe some of these things should be pulled out and put into CLAUDE.md so they're done right the first time too?
There was a problem hiding this comment.
I'll add this here, I think pulling into the CLAUDE.md is ok but I think having another discussion on improvements to the file/structure would be helpful. For example, I think we should have a smaller CLAUDE.md file and have references to domain specific things like React, Playwright etc.
| - Hapi route handlers that catch and re-throw instead of letting the error pipeline handle it | ||
|
|
||
| **4. Tests** | ||
| - New source files without co-located `*.spec.ts` |
There was a problem hiding this comment.
This should probably say code in fxa-settings follows a *.test.* convention
|
|
||
| **4. Tests** | ||
| - New source files without co-located `*.spec.ts` | ||
| - Hardcoded translation strings in assertions — should assert localization occurred, not exact values |
There was a problem hiding this comment.
This is so specific, I think we have only a couple of tests that do this in the entire repo so not sure if it's worth having hmm
.claude/skills/fxa-review/SKILL.md
Outdated
| - model: opus | ||
| - description: FXA security review | ||
|
|
||
| Tell this agent it is a senior security engineer with deep knowledge of FXA's authentication and payments platform. It should: |
There was a problem hiding this comment.
Not saying to remove, but I wonder how useful this actually is to tell the agent that it possesses deep knowledge of the system. I get how telling it that it's a senior security engineer would purposefully bias it how we want it though.
| - Index changes separate from schema changes | ||
| - Check cross-package boundary violations: relative imports across package boundaries instead of `@fxa/*` aliases | ||
| - Flag colliding exports: new exports that duplicate existing ones elsewhere in the monorepo | ||
|
|
There was a problem hiding this comment.
Maybe also add something about following best React and Tailwind practices?
| - Flag `jest.clearAllMocks()` in `beforeEach` — unnecessary, `clearMocks: true` is global | ||
| - Check for shared mutable state between tests, missing `await`, `setTimeout` in tests | ||
| - Flag over-mocking: mocking internal functions in the same package instead of at system boundaries | ||
|
|
There was a problem hiding this comment.
One other thing that would be nice is to make sure we don't introduce new open handle warnings. I know the migration has been setup to use --force-exit, but it'd be nice to fix whatever is causing those errors and not introduce new ones
Oh, and having it keep an eye out for act() warnings in React tests.
vbudhram
force-pushed
the
add-fxa-skills
branch
from
d634ac0 to
8aa2798
Compare
vbudhram
force-pushed
the
add-fxa-skills
branch
from
8aa2798 to
a16f0ff
Compare
vbudhram
force-pushed
the
add-fxa-skills
branch
from
a16f0ff to
bc86f51
Compare
|
Thanks for the review! I think I've covered all the requested changes. My goal here is that it is used as a first past before all human reviews are requested. It definately won't be perfect, but hopefully provides more value than not. |
Because
This pull request
Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13306
Checklist