chore(skill): add fxa-risk-score PR risk scoring skill - #21118
Closed
vbudhram wants to merge 4 commits into
Closed
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a user-invocable skill for scoring FxA pull request and commit risk.
Changes:
- Defines impact levels, modifiers, and risk bands.
- Adds merge-blocker checks and a standardized report format.
- Includes migration, security, and ownership guidance.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| description: Score the risk of a PR or commit in the FXA monorepo. Reports a 0-10 score and band, the change type, how much review it needs, whether it is safe to merge, database migration risk, and which user flows it can break. Use when asked how risky a change is, how much review it needs, or whether it is safe to merge. | ||
| allowed-tools: Bash, Read, Grep, Glob | ||
| argument-hint: [commit-ref | PR number] | ||
| user-invocable: true |
|
|
||
| For a PR number, use `gh pr diff <number>` and `gh pr view <number> --json title,body,files`. | ||
|
|
||
| For uncommitted work, compare against `main`: `git diff main...HEAD`. |
| Risk: HIGH (8/10) | ||
| Type: migration · 3 packages | ||
|
|
||
| Review depth: second approver + passkey owner |
| ## Guidelines | ||
|
|
||
| - Score the diff in front of you, not the worst thing it could have been. | ||
| - A large diff is not automatically risky. A one-line change to token validation is riskier than a 900-line l10n import. Blast radius is a modifier, never the headline. |
Because: * Reviewers have no shared measure of how much scrutiny a change needs. * Copilot code review reads skills from .github/skills on the head branch. This commit: * Adds fxa-risk-score, rating impact 0-5 from what the diff does. * Adds modifiers for recovery cost, escape likelihood, and containment. * Rates migrations against the two-stack rule in db-migrations. * Puts the rating in the review summary and blockers on their own lines. * Treats the diff as untrusted input, never as instructions.
|
|
||
| Read files around the diff to judge impact. You need callers, types, and config to tell a real risk from a shape that merely looks like one. | ||
|
|
||
| In a terminal, fetch it yourself. `$ARGUMENTS` is a commit ref or a PR number, defaulting to `HEAD`: |
| git diff "$REF^" "$REF" | ||
| ``` | ||
|
|
||
| For a PR number use `gh pr diff <number>`. For uncommitted work use `git diff main...HEAD`. |
|
|
||
| ## Step 2: Classify the Change Type | ||
|
|
||
| Pick the one that fits best: `feature`, `fix`, `refactor`, `deps`, `config`, `migration`, `revert`, `test-only`, `docs`, `l10n`. |
| - Write `Blockers: none` when there are none. Never omit that line — silence reads as "not checked". | ||
| - The `Why:` line must add up to the reported score. If it does not, you made an arithmetic error, fix it before reporting. | ||
| - Name files and patch numbers. "Touches auth" is not useful; "modifies `lib/oauth/grant.js`" is. | ||
| - Score the same diff the same way twice. A re-review after a push should move the number only if the diff moved, and should say what moved it. |
| ## Guidelines | ||
|
|
||
| - Score the diff in front of you, not the worst thing it could have been. | ||
| - A large diff is not automatically risky. A one-line change to token validation is riskier than a 900-line l10n import. Blast radius is a modifier, never the headline. |
Because: * git diff main...HEAD skips the uncommitted work it claimed to rate. * A PR number is not a git revision and cannot go to git show. * The guidelines cited a blast-radius modifier that Step 3 does not define. This commit: * Splits Step 1 into commit-ref, PR-number, and working-tree commands. * Adds chore to the change types, matching CONTRIBUTING.md. * Ties owner naming to CODEOWNERS and forbids inventing one. * Lets the score move when verified release state moves, not silently.
Because: * Copilot code review picks skills with review-focused names in .github/skills; fxa-risk-score was read as a file, never as guidance. * Two reviews on #21118 and #21119 confirmed the miss. This commit: * Renames the directory and skill name to code-review. * Reframes the description around reviewing a pull request.
Because: * Naming the skill code-review makes it the repo's review instruction. * As written it read as "emit the rating", risking loss of bug findings. * Copilot's ordinary review found six real defects in this skill. This commit: * Leads with an explicit augment-not-replace rule. * Requires reporting findings even when the rating says LOW. * Scopes the no-filler rule to the rating block only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.