Skip to content

Commit 2b6e989

Browse files
committed
docs(skill): add PR size + feature flag rules to pr-review
Adds Step 4.6 (PR size — soft >400 lines / hard >800 lines, propose decomposition) and Step 4.7 (risky change requires off-by-default env flag with .env.example + docs sync). Updates verdict table and comment skeleton to surface both new gates.
1 parent 76cdba9 commit 2b6e989

1 file changed

Lines changed: 101 additions & 6 deletions

File tree

.claude/skills/pr-review/SKILL.md

Lines changed: 101 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,98 @@ Walk `.ai-factory/RULES.md` rules explicitly. For each rule, flag any clear viol
315315
the diff. Examples from this repo: 70 % coverage, SOLID/DRY, reuse UI primitives, sync
316316
Docker, no expensive CSS, sync adapter docs.
317317

318-
### 4.6 Context Gates (Read-Only)
318+
### 4.6 PR Size — Decomposition Check
319+
320+
A PR that is too large is hard to review carefully and easy to merge with hidden defects. Flag
321+
size as a **must-fix** when the PR clearly exceeds healthy limits, and recommend decomposition.
322+
323+
Rough thresholds (use judgment — generated lockfiles, snapshots, and pure rename diffs should
324+
be excluded from the count before applying):
325+
326+
- **Soft limit (warn):** > 400 changed lines OR > 15 changed files OR > 5 distinct concerns.
327+
- **Hard limit (must-fix):** > 800 changed lines OR > 30 changed files OR mixes unrelated
328+
refactors / features / fixes in one PR.
329+
330+
Compute the size:
331+
332+
```bash
333+
gh pr view $PR --repo $OWNER/$REPO --json additions,deletions,files \
334+
--jq '{additions, deletions, fileCount: (.files | length)}'
335+
```
336+
337+
Exclude noise before deciding:
338+
339+
- Lockfiles: `package-lock.json`, `pnpm-lock.yaml`, `yarn.lock`, `bun.lockb`, `Cargo.lock`,
340+
`poetry.lock`, `go.sum`.
341+
- Generated artifacts: `**/*.generated.ts`, `**/dist/**`, snapshot files matched by the
342+
project's snapshot config.
343+
- Pure-rename diffs (file moved, contents unchanged) — count as 1 line each, not the
344+
full file size.
345+
346+
When the hard limit is hit, the verdict comment MUST include a "Decomposition" section
347+
under **Must fix** that:
348+
349+
1. States the size (lines / files / concerns) and why it is over the bar.
350+
2. Proposes a concrete split — typically along these axes:
351+
- **By concern:** one PR per feature, refactor, or fix (never bundle).
352+
- **By layer:** schema/migration → data layer → API → UI as separate PRs when the
353+
downstream layer can land independently.
354+
- **By package:** one PR per `packages/*/` root when the changes are independently mergeable.
355+
- **By risk:** isolate risky changes (DB migrations, auth, payment paths) into their own
356+
small PR so they can be reviewed and rolled back independently.
357+
3. Names the suggested PR boundaries with file globs or commit ranges so the author can act
358+
on it without further clarification.
359+
360+
Soft-limit hits go under **Should fix** with the same structure but without blocking the verdict.
361+
362+
### 4.7 Risky Change — Feature Flag Required
363+
364+
Any change that introduces non-trivial new behavior, alters a hot path, touches an external
365+
contract, or could plausibly break existing flows MUST be guarded by an environment-driven
366+
feature flag that defaults to **off** (`false`). This lets the change ship dark, get rolled
367+
out per-environment, and be killed instantly if something regresses in production.
368+
369+
Treat the following as **risky by default** (must be flag-gated unless the author provides
370+
a strong reason otherwise):
371+
372+
- New runtime adapter, new transport, or change to existing adapter capabilities.
373+
- Schema migration that backfills, transforms, or deletes data (additive `ADD COLUMN` with a
374+
default is usually safe without a flag).
375+
- Changes on the request hot path: WebSocket frame handling, polling coordinator cadence,
376+
rate limiter, auth middleware, request logger.
377+
- New external dependency call (network, MCP server, OS process spawn) added to an existing
378+
flow that previously did not make that call.
379+
- Behavioral change to an existing API route's response shape, error code, or side effects.
380+
- New cron job, queue consumer, or background worker.
381+
- Replacing a stable algorithm with a new one (sort, scoring, scheduling, retry policy).
382+
383+
Required flag shape — look for ALL of these in the diff:
384+
385+
1. **Env declaration:** the flag is added to `packages/shared/src/env.ts` (or the
386+
project's central env-validation module) with a `boolean` schema and a default of `false`.
387+
2. **`.env.example` entry:** the variable is documented with a short comment explaining what
388+
it gates.
389+
3. **Docs update:** `docs/configuration.md` (or the doc surface that lists env vars) lists
390+
the new flag, its default, and the rollout intent.
391+
4. **Single read site:** the flag is read once at module init or via a small accessor — not
392+
`process.env.FOO` re-read on every call inside hot loops.
393+
5. **Off path is the existing behavior:** when the flag is `false`, the new code path is not
394+
reachable. No partial enabling, no "off but still imports the new module's side effects".
395+
6. **Naming:** `AIF_<AREA>_<FEATURE>_ENABLED` (e.g., `AIF_USAGE_LIMITS_ENABLED`,
396+
`AIF_RUNTIME_OPENROUTER_ENABLED`). Avoid bare names like `NEW_FEATURE`.
397+
398+
Findings:
399+
400+
- Risky change with no flag → **must-fix**. Suggest the exact env name, default, and the
401+
branch in code where the flag check should sit.
402+
- Flag exists but defaults to `true`, or off-path is broken → **must-fix**. Cite the line.
403+
- Flag exists, defaults to `false`, but `.env.example` / docs missing → **should-fix**.
404+
- Flag is read in a hot loop instead of cached at init → **should-fix**. Suggest hoisting.
405+
406+
When risk is genuinely low (formatting, comment-only edits, dependency bumps with no API
407+
change, doc-only PRs), skip this section — do not invent risk where there is none.
408+
409+
### 4.8 Context Gates (Read-Only)
319410

320411
Produce a gate verdict for each:
321412

@@ -344,11 +435,13 @@ Use:
344435

345436
### 5.2 Choose Verdict
346437

347-
| Condition | Verdict |
348-
| ---------------------------------------------------------- | ----------------- |
349-
| No `must-fix` and CI green | `APPROVE` |
350-
| Any `must-fix`, or red CI caused by PR changes | `REQUEST_CHANGES` |
351-
| Only `should-fix` / `nit`, or flaky CI unrelated to the PR | `COMMENT` |
438+
| Condition | Verdict |
439+
| -------------------------------------------------------------- | ----------------- |
440+
| No `must-fix` and CI green | `APPROVE` |
441+
| Any `must-fix`, or red CI caused by PR changes | `REQUEST_CHANGES` |
442+
| PR exceeds the hard size limit (Step 4.6) | `REQUEST_CHANGES` |
443+
| Risky change without an off-by-default feature flag (Step 4.7) | `REQUEST_CHANGES` |
444+
| Only `should-fix` / `nit`, or flaky CI unrelated to the PR | `COMMENT` |
352445

353446
In follow-up mode, if all previously-raised `must-fix` are `ADDRESSED` and no new `must-fix`
354447
appeared, prefer `APPROVE`.
@@ -381,6 +474,8 @@ Use this skeleton (in the resolved UI language; English by default):
381474
- Roadmap: <pass/warn/error> — <note>
382475
- CHECKLIST compliance (touched packages: <list>): <pass/warn/error>
383476
- Docs sync: <pass/warn/error>
477+
- PR size (<lines> / <files> / <concerns>): <pass/warn/error> — <note or proposed split>
478+
- Risk gating (feature flag): <pass/warn/error/n/a> — <flag name + default, or why no flag is needed>
384479

385480
<optional "Positive notes" section when something is genuinely well done>
386481
```

0 commit comments

Comments
 (0)