|
| 1 | +# PR Requirements — Squad Repository |
| 2 | + |
| 3 | +> **This file is the canonical source of truth for PR requirements.** |
| 4 | +> Issue #106 tracks the PRD; this file is the versioned spec. |
| 5 | +> Changes to this file must go through PR review (audit trail). |
| 6 | +
|
| 7 | +--- |
| 8 | + |
| 9 | +## Definition of "User-Facing Change" |
| 10 | + |
| 11 | +A PR contains a **user-facing change** if it introduces a CRUD operation to the CLI or SDK layer that is exposed in TypeScript or true executable functionality. |
| 12 | + |
| 13 | +"User-facing" is anchored to two concrete package boundaries: |
| 14 | +- **SDK layer**: `packages/squad-sdk/src/` exports exposed in `package.json` subpath exports |
| 15 | +- **CLI layer**: `packages/squad-cli/src/cli/` commands and subcommands |
| 16 | + |
| 17 | +### CRUD Operations — What Counts as User-Facing |
| 18 | + |
| 19 | +| Operation | SDK Layer | CLI Layer | |
| 20 | +|-----------|-----------|-----------| |
| 21 | +| **Create** | New export added to `packages/squad-sdk/src/`; new subpath export added to `package.json` | New CLI command or subcommand added to `packages/squad-cli/src/cli/commands/` | |
| 22 | +| **Read** | N/A — reading doesn't change the surface | N/A — reading doesn't change the surface | |
| 23 | +| **Update** | Change to existing public API signature (parameters, return types, behavior) | Change to CLI command flags, behavior, or subcommand structure | |
| 24 | +| **Delete** | Remove a previously public export from `package.json` | Remove a CLI command or subcommand | |
| 25 | + |
| 26 | +### What Is NOT User-Facing |
| 27 | + |
| 28 | +- `.squad/` state files (decisions, history, skills, templates) |
| 29 | +- Internal refactors that don't change the public API surface |
| 30 | +- Infrastructure (CI, GitHub Actions, workflows) |
| 31 | +- Documentation-only changes |
| 32 | +- Dependency updates that don't change API surface |
| 33 | + |
| 34 | +Why this matters: The requirement categories below apply conditionally to user-facing changes. Knowing what counts as user-facing prevents disputes about "does this need a CHANGELOG entry?" |
| 35 | + |
| 36 | +### Examples of "new module" |
| 37 | + |
| 38 | +- Adding a new directory under packages/squad-sdk/src/ (e.g., src/storage/, src/casting/) |
| 39 | +- Adding a new subpath export to package.json (e.g., ./storage, ./casting) |
| 40 | +- Adding a new CLI command file under packages/squad-cli/src/cli/commands/ |
| 41 | + |
| 42 | +### NOT a new module |
| 43 | + |
| 44 | +- Adding a new function to an existing module |
| 45 | +- Refactoring internals without changing the public API surface |
| 46 | +- Adding test files |
| 47 | + |
| 48 | +--- |
| 49 | + |
| 50 | +## PR Requirements by Category |
| 51 | + |
| 52 | +Requirements are ordered by review flow. All marked **REQUIRED** must be satisfied before merge. Some are automatically enforced (CI gates); others require human review. |
| 53 | + |
| 54 | +### a) Git Hygiene (REQUIRED — automated) |
| 55 | + |
| 56 | +- [ ] Commits reference the issue number: `Closes #N` or `Part of #N` in commit message |
| 57 | +- [ ] No binary files or build artifacts committed (`dist/`, `node_modules/`, `.DS_Store`, etc.) |
| 58 | +- [ ] No unrelated file changes (bleed check: only files needed for the stated goal) |
| 59 | +- [ ] Clean commit history: logical commits or squashed to a single coherent commit |
| 60 | +- [ ] No force-pushes to shared branches (`dev`, `main`) |
| 61 | + |
| 62 | +**Enforced by**: squad-ci.yml bleed check + git hooks |
| 63 | + |
| 64 | +### b) CI / Build (REQUIRED — automated) |
| 65 | + |
| 66 | +- [ ] Build passes: `npm run build` completes successfully |
| 67 | +- [ ] All existing tests pass: `npm test` or `npm run test` |
| 68 | +- [ ] No new build warnings introduced without justification in PR description |
| 69 | +- [ ] Feature flags documented in code and CHANGELOG if gating new behavior |
| 70 | + |
| 71 | +**Enforced by**: GitHub Actions CI pipeline |
| 72 | + |
| 73 | +### c) Code Quality (REQUIRED — manual review for completeness, automated for linting) |
| 74 | + |
| 75 | +- [ ] All new public APIs have JSDoc/TSDoc comments |
| 76 | +- [ ] No lint errors: `eslint` passes |
| 77 | +- [ ] No type errors: `tsc --noEmit` passes |
| 78 | +- [ ] Tests written for all new functionality |
| 79 | +- [ ] All tests pass: `vitest` exits 0 |
| 80 | + |
| 81 | +**Enforced by**: eslint in CI + manual code review |
| 82 | + |
| 83 | +### d) Documentation (REQUIRED for user-facing changes — manual review) |
| 84 | + |
| 85 | +- [ ] CHANGELOG.md entry under `[Unreleased]` following Keep-a-Changelog format |
| 86 | + - Example: `### Added — FeatureName (#N) — short description` |
| 87 | + - Required when: a **user-facing change** (see "Definition of \"User-Facing Change\"") affects the SDK or CLI public surface (e.g., exports in `packages/squad-sdk/src/` or commands in `packages/squad-cli/src/cli/`) |
| 88 | +- [ ] README.md section updated if adding a new feature or module to the SDK |
| 89 | +- [ ] Docs feature page added under `docs/src/content/docs/features/` if adding a user-facing capability |
| 90 | +- [ ] `docs/src/navigation.ts` updated if a new docs page is added |
| 91 | +- [ ] Docs build test updated if a new feature page is added (`test/docs-build.test.ts`, run via `npm test`) |
| 92 | + |
| 93 | +**Enforced by**: Manual PR review (FIDO, Flight) + future automated checks in #104 |
| 94 | + |
| 95 | +### e) Package / Exports (REQUIRED for new modules — manual review) |
| 96 | + |
| 97 | +- [ ] `package.json` subpath exports updated when new modules are added |
| 98 | + - Both types (`.d.ts`) and import (`.js`) entries required per export |
| 99 | + - Example: the `/storage` export was missing from #640 until the audit caught it |
| 100 | +- [ ] No accidental dependency additions: `package-lock.json` changes reviewed for unexpected deps |
| 101 | +- [ ] No export regressions: existing exports remain present and functional |
| 102 | + |
| 103 | +**Enforced by**: Manual PR review + future export audit gate in #104 |
| 104 | + |
| 105 | +### f) Samples (REQUIRED when API changes — manual review) |
| 106 | + |
| 107 | +- [ ] Existing sample projects updated if the PR changes a public API they use |
| 108 | +- [ ] New user-facing features include at least one sample demonstrating usage |
| 109 | +- [ ] Sample README.md explains what the sample does and how to run it |
| 110 | +- [ ] Samples build and run: `npm run build` in sample directory succeeds |
| 111 | + |
| 112 | +**Enforced by**: Manual PR review + #103 (Samples CI coverage) |
| 113 | + |
| 114 | +--- |
| 115 | + |
| 116 | +## Breaking Change Policy |
| 117 | + |
| 118 | +Any PR that changes the public API in a backward-incompatible way (removal, signature change, behavior change) must: |
| 119 | + |
| 120 | +- [ ] Document the breaking change in CHANGELOG.md under a `[BREAKING]` section |
| 121 | +- [ ] Add a migration guide as a docs note in the affected feature page |
| 122 | +- [ ] Update all sample projects to use the new API |
| 123 | +- [ ] Include the migration path in the PR description (`## Breaking Changes` section) |
| 124 | + |
| 125 | +**Examples of breaking changes:** |
| 126 | +- Removing an export that was previously public |
| 127 | +- Changing function signature (parameter name, order, type) |
| 128 | +- Changing CLI command name or subcommand structure |
| 129 | +- Changing configuration file format in backward-incompatible way |
| 130 | + |
| 131 | +**Examples NOT breaking:** |
| 132 | +- Adding new optional parameters |
| 133 | +- Adding new exports |
| 134 | +- Adding new subcommands (existing ones unchanged) |
| 135 | +- Fixing documented behavior that was incorrect |
| 136 | + |
| 137 | +--- |
| 138 | + |
| 139 | +## Exception / Waiver Process |
| 140 | + |
| 141 | +Emergencies (security fixes, critical bugs) require flexibility. Any requirement can be waived if documented and approved. |
| 142 | + |
| 143 | +### How to Request a Waiver |
| 144 | + |
| 145 | +Add to PR description under a `## Waivers` section: |
| 146 | + |
| 147 | +``` |
| 148 | +## Waivers |
| 149 | +
|
| 150 | +- Item waived: (d) Documentation CHANGELOG entry |
| 151 | +- Reason: Security fix needs urgent release (CVE-2024-XXXXX) |
| 152 | +- Approval by: [Flight/FIDO to be stated in PR review comment] |
| 153 | +``` |
| 154 | + |
| 155 | +### Waiver Approval Authority |
| 156 | + |
| 157 | +- **Flight (architecture decisions)**: Internal refactors, infrastructure, module reorganization |
| 158 | +- **FIDO (quality decisions)**: Bug fixes, test improvements, samples, CI changes |
| 159 | +- **Both**: External API changes, breaking changes |
| 160 | + |
| 161 | +### Waiver Approval Examples |
| 162 | + |
| 163 | +| Scenario | Waivable Items | Approver | |
| 164 | +|----------|----------------|----------| |
| 165 | +| Internal refactor (no API change) | Documentation, Samples | Flight | |
| 166 | +| Security fix | Documentation, Samples (with follow-up issue) | FIDO | |
| 167 | +| Test infrastructure change | Documentation, Exports, Samples | FIDO | |
| 168 | +| CI/GitHub Actions update | Documentation, Exports, Samples | FIDO | |
| 169 | +| Documentation-only change | Code Quality (tests), Exports, Samples | FIDO | |
| 170 | +| Non-breaking SDK bugfix | Samples | FIDO | |
| 171 | + |
| 172 | +### Self-Waiving Is Not Allowed |
| 173 | + |
| 174 | +- Do NOT use skip labels without explicit reviewer approval in PR comments |
| 175 | +- Do NOT remove requirements from checklist without documented waiver |
| 176 | +- Do NOT claim "documentation will follow" without a tracked follow-up issue |
| 177 | + |
| 178 | +--- |
| 179 | + |
| 180 | +## Edge Case Exemptions |
| 181 | + |
| 182 | +Certain PR types are categorically exempt from specific requirements by nature. These are **NOT** waivers (no approval needed); they are structural exemptions. |
| 183 | + |
| 184 | +| PR Type | Exempt From | Reason | |
| 185 | +|---------|-------------|--------| |
| 186 | +| Internal refactor (no public API change) | (d) Docs, (f) Samples | No user-visible change | |
| 187 | +| Test-only changes | (d) Docs, (e) Exports, (f) Samples | No production code changed | |
| 188 | +| CI/GitHub Actions workflow changes | (d) Docs, (e) Exports, (f) Samples | Infrastructure only | |
| 189 | +| Documentation-only changes | (c) Code Quality (tests), (e) Exports, (f) Samples | No code changes | |
| 190 | +| Dependency bumps (routine maintenance) | (d) Docs feature page, (f) Samples | Routine dependency update | |
| 191 | + |
| 192 | +**Important**: Exemptions apply only when the PR genuinely affects NO code in the exempted categories. A PR touching both infrastructure AND public APIs is NOT exempt. |
| 193 | + |
| 194 | +--- |
| 195 | + |
| 196 | +## PR Size Guidelines |
| 197 | + |
| 198 | +| Size | Files Changed | Guidance | |
| 199 | +|------|---------------|----------| |
| 200 | +| Small | < 10 files | Preferred. Fast to review, easy to revert. | |
| 201 | +| Medium | 10–20 files | Acceptable. Justify scope in PR description. | |
| 202 | +| Large | > 20 files | Must be split or justified with written explanation. | |
| 203 | +| Red flag | > 50 files | STOP — split the PR or get explicit written approval before proceeding. | |
| 204 | + |
| 205 | +For migration PRs (> 20 files): include test output summary in the PR description. |
| 206 | + |
| 207 | +--- |
| 208 | + |
| 209 | +## Known Limitations |
| 210 | + |
| 211 | +### 1. Enforceability Gap |
| 212 | + |
| 213 | +9 of 18 requirements are manual-only with no CI gate (README updates, docs page relevance, sample completeness, migration guide quality). Enforcement depends on consistent reviewer judgment. |
| 214 | + |
| 215 | +**Automation dependency**: 9 of 18 requirements are manual-only. Issue #104 (PR completeness gates) is the critical path to automated enforcement. Until #104 ships, this spec depends on reviewer discipline. |
| 216 | + |
| 217 | +**Mitigation**: Phase 4 (Flight/FIDO checklist) codifies consistent judgment. Regular enforcement audits catch drift. |
| 218 | + |
| 219 | +### 2. Theater Risk |
| 220 | + |
| 221 | +Calling something "REQUIRED" without automated enforcement creates appearance of rigor without substance. Phase 3 (#104) prioritizes high-leverage automated gates (CHANGELOG, exports, build). Manual-only items are clearly labeled. |
| 222 | + |
| 223 | +### 3. Silent Drift Risk |
| 224 | + |
| 225 | +This requirements file is the source of truth. CI gate scripts (#104) and reviewer checklists (#100) must be manually updated if requirements change. Quarterly review cycle to resync. |
| 226 | + |
| 227 | +### 4. Ambiguities |
| 228 | + |
| 229 | +- **"Module" definition**: new directory under `packages/squad-sdk/src/` or new export namespace. Refactoring existing module does not trigger export requirement. |
| 230 | +- **"User-facing" detection for CHANGELOG**: Currently manual. #104 will implement export-diff detection. |
| 231 | + |
| 232 | +--- |
| 233 | + |
| 234 | +## Relationship to Other Issues |
| 235 | + |
| 236 | +| Issue | Role | |
| 237 | +|-------|------| |
| 238 | +| #100 (Review completeness) | Defines the review *process* — this file defines *what reviewers check against* | |
| 239 | +| #103 (Samples CI coverage) | Implements the "samples" requirement category (f) | |
| 240 | +| #104 (PR completeness gates) | Implements automated enforcement of REQUIRED items as CI gates | |
| 241 | + |
| 242 | +This file is the source of truth. #100 and #104 derive their checklists from it. |
0 commit comments