feat: add PR requirements spec and PR template (#106 Phase 2)#108
feat: add PR requirements spec and PR template (#106 Phase 2)#108
Conversation
|
Flight Review: PR #108 VERDICT: APPROVE FINDINGS: ? ARCHITECTURE ALIGNMENT (#106 PRD)
? CATEGORY COMPLETENESS
? BREAKING CHANGE POLICY
? WAIVER PROCESS
? PR METADATA
? AUTHOR CHECKLIST (PULL_REQUEST_TEMPLATE.md)
MINOR NOTES (informational, not blocking):
MERGE READY. This PR successfully codifies PR review standards as a versioned, auditable, maintenance-free specification. Reviewer judgment is consistent with the requirements matrix. No gaps in coverage. Approved for merge to dev. |
PAO Review: PR #108Verdict✅ APPROVED — Excellent documentation quality. Merge recommended. FindingsStrengths (Exemplary)Markdown Formatting & Readability
Template UX
Documentation Conventions
Minor Findings1. Line-break artifact (PR_REQUIREMENTS.md, around line 104)
2. Template: Migration PR callout (Testing section)
3. Waiver section in template
SummaryThis is production-ready documentation that balances rigor with usability. The PR_REQUIREMENTS.md is exceptional for new contributor onboarding, and the template is practical and scannable. Markdown formatting is consistent, content reflects good judgment on what to enforce vs. waive. Recommendation: Merge. Optional: fix line-break artifact before merge (non-blocking). |
FIDO Review: PR #108Verdict: ✅ APPROVEDPR #108 delivers Phase 2 of #106: versioned PR requirements spec and author-facing PR template. Spec is comprehensive, well-structured, testable, and enforceable. Review findings below. REQUIRED Categories Assessmenta) Git Hygiene ✅
Pass: All checks satisfied. b) CI / Build ✅
Pass: CI green. Timeout failures are unrelated to this PR. c) Code Quality ✅
Exemption: Documentation-only changes exempt from code quality (per Edge Case Exemptions in PR_REQUIREMENTS.md) Pass: Exempt. d) Documentation ✅ + 🟢 Self-Reference Check
Pass: Self-referential documentation deliverable. Properly structured PR description. e) Package / Exports ✅
Exemption: Documentation-only changes exempt from exports (per Edge Case Exemptions) Pass: Exempt. f) Samples ✅
Exemption: Documentation-only changes exempt from samples (per Edge Case Exemptions) Pass: Exempt. Testability of Requirements AnalysisFinding: Testability is high. Automated (CI-Gatable) Requirements
Manual-Only Requirements (Reasonable)
No surprise manual-only requirements. Spec is transparent about enforceability gaps. Enforceability of Each Category Analysis
Finding: No category is unenforceable. All have a clear owner (automated or manual reviewer). No ambiguous "maybe later" items. CI Gate FeasibilityFinding: All automated gates are feasible and within current CI infrastructure.
No unfeasible gates proposed. All can be implemented incrementally in #104. Waiver Process CompletenessFinding: Waiver process is complete and operationalized. Documented Elements ✅
Operationalization Check ✅
Finding: Waiver process is ready for #100 integration (reviewer checklist). Edge Case Exemptions CompletenessFinding: Exemptions are comprehensive and non-overlapping. Coverage ✅
Gotcha Prevention ✅
Finding: No gaps in exemption coverage. Spec prevents loopholes. PR #108 Self-Verification: Does It Follow #106 Requirements?✅ YES. PR follows its own spec template. Required Sections Present
All REQUIRED Sections Complete
Finding: PR is a model example of following the spec it introduces. (This is intended — dogfooding the template increases confidence.) Known Limitations TransparencyFinding: Spec acknowledges limitations honestly. Documented Gaps
Recommendation: Add follow-up issue references to each mitigation:
Finding: Limitations are documented transparently. No hidden debt. Relationship to Upstream IssuesFinding: Dependencies correctly documented.
Finding: No circular dependencies. #108 provides clear input for #100 and #104. Edge Cases & Risks✅ No Risk: Markdown Content
✅ No Risk: Backcompat
🟡 Low Risk: Adoption
🟡 Low Risk: Drift
Summary of Findings
RecommendationAPPROVE and MERGE. This PR delivers Phase 2 of #106 completely. Spec is testable, enforced, and transparent about limitations. Waiver process is operationalized. PR dogfoods its own template, increasing confidence. Next Steps (for team, not this PR):
FIDO Sign-Off |
Challenger Review: PR #108Executive Summary[MEDIUM-HIGH RISK] PR #108 defines PR requirements but contains 6 critical adversarial findings:
Retroactive Test Against PR bradygaster#640 High GapsWould the requirements CATCH #640s 4 HIGH gaps? Partial: 2 yes, 1 ambiguous, 1 no.
Problem: Requirement (e) requires "package.json subpath exports updated when new modules are added." But what defines a "module"? The PR_REQUIREMENTS file says in Known Limitations Section 4: "Module definition: new directory under packages/squad-sdk/src/ or new export namespace. Refactoring existing module does not trigger export requirement." PR bradygaster#640 added /packages/squad-sdk/src/storage/ directory. By the definition above, that IS a new module. BUT: No reviewers applied this judgment consistently pre-requirement-doc. This suggests the definition is precise-in-retrospect but was ambiguous when enforcement was needed. Risk: Requirements that require consistent judgment calls fail when reviewers are overloaded. A binary gate (export audit tool in #104) would have prevented bradygaster#640. Finding 1: [HIGH] Waiver Process Is Self-Veto, Not ApprovalIssue: The waiver section says: Problem: "[Flight/FIDO to be stated in PR review comment]" is not approval authority -- it is a placeholder. This lets authors write: Then claim they followed the process. The requirement says "Self-waiving is not allowed" but the template invites it by making the approver field optional/aspirational. Impact: Waivers become "I am flagging this for waiver" instead of "I have waiver approval." Over time, authors learn to be optimistic in the approval field and reviewers get fatigued approving backfilled waivers. Gaming opportunity: "We will get FIDO approval after merge" becomes standard practice. Finding 2: [HIGH] User-Facing Detection Is Still ManualIssue: Category (d) says requirements apply "REQUIRED for user-facing changes" but user-facing is defined as:
But category (d) itself requires a CHANGELOG entry "when packages/squad-sdk/src/ or packages/squad-cli/src/ changes." Gap: These are two different detection gates:
A PR could touch packages/squad-sdk/src/internals/ with NO public API change and trigger CHANGELOG requirement by file path alone. Conversely: A PR could add a new export that is internal-only, bypass the file-path gate, and not require CHANGELOG because it is not user-facing. Problem: Reviewers will disagree on borderline cases. The spec tries to handle this with "Definition of User-Facing Change" section but then ALSO includes a file-path heuristic that conflicts. Impact on bradygaster#640: StorageProvider PR touches many files. Did reviewer apply export-based gate ("new API") or file-path gate ("touches /src/")? The audit had to discover the gap, meaning reviewers did not apply consistent judgment. Finding 3: [MEDIUM] "Module" Definition Is Precise-in-Hindsight, Ambiguous-in-Real-TimeIssue: Known Limitations section says:
Problem: Is /packages/squad-sdk/src/storage/ a "new module" if storage logic already existed elsewhere and was consolidated? Is it a "new namespace" export if the exports are /storage/provider, /storage/adapter? Real case: PR bradygaster#640 created /packages/squad-sdk/src/storage/. The export requirement would apply IF reviewers agreed this was a "new module." But "module" is contextual:
Impact: Three different reviewers could reach three different conclusions. The requirement is clear only after the fact. Mitigation exists (in #104): An export-audit bot that checks "all new exports are in package.json" is binary and unambiguous. This requirement spec tries to be humans-first (reviewers decide with judgment) but that judgment is expensive and inconsistent. Finding 4: [MEDIUM] Exemption Matrix Creates "Anything Goes" CategoryIssue: Edge Case Exemptions table says PRs are exempt from requirements in specific categories. But the exemption for "Internal refactor (no public API change)" is categorical:
Problem: "Internal refactor" is self-identified by the author. There is no CI gate to confirm the PR is truly internal. An author can claim "internal refactor" and skip CHANGELOG/samples. Worse: The Exemptions table rows are ORed with the Waiver Process. A PR author can:
Both paths exist in the spec. This creates ambiguity about which gate applies. Impact: Inconsistent application. FIDO will block one PR for missing CHANGELOG on an "internal refactor" and approve another via exemption for identical changes. Finding 5: [MEDIUM] No Test Gate on the Spec ItselfIssue: PR #108 ships:
Both are markdown-only. The PR description claims no tests needed ("no code changes -- markdown files only"). Problem: The PR_REQUIREMENTS file ITSELF should follow the requirements it defines:
More pragmatically: Who reviews this file for correctness? The PR has no code diffs for automated linting, type-checking, or spell-checking. It is a 213-line policy doc with:
Risk: The spec becomes outdated the moment any cross-referenced issue changes scope. Finding 6: [MEDIUM-LOW] Theater Risk: Half of "REQUIRED" Items Are UnenforceableIssue: Category breakdown:
= 9 of 18 items are manual-only with no CI gate. The spec says they are "REQUIRED" but provides no mechanism to block merge if they are missing (other than reviewer judgment). Problem: This is honest ("Enforced by: Manual PR review") but risks becoming cargo cult. Over time:
Examples: "README section updated if adding a new feature" is 100% dependent on reviewer memory and attention to detail. After 50 PRs, someone forgets to check. Then it is not a requirement anymore; it is a nice-to-have. Mitigation mentioned: Phase 4 audit cycle ("Quarterly review cycle to resync") but this is not testable in the spec itself. Finding 7: [MEDIUM] Perverse Incentive in PR Size GuidelinesIssue: Size guidelines say:
Problem: This incentivizes artificial splitting of logically related work: Example scenario:
This is exactly what is suspected in bradygaster#640: StorageProvider PR shipped with all 4 gaps (CHANGELOG, README, docs page, export). Were these 4 separate review touchpoints that fell through gaps-in-splitting, or was the PR oversize and author deprioritized non-code items? Counter-example: Git has a philosophy: "If parts of the change are logically related, keep them together even if the file count is high. Splitting for size alone defeats review." Impact: Requirements that encourage artificial splitting risk creating MORE review gaps, not fewer. Retroactive Application: Would These Requirements Have Caught PR bradygaster#640?Outcome: 2 of 4 gaps yes, 1 ambiguous, 1 no.
Verdict: Requirements are directionally correct but would have required consistent judgment from reviewers on the bradygaster#640 PR review. The 4 gaps were discovered in post-audit (automated export scan), not pre-merge (manual review). This suggests that even with the spec in place, human reviewers applying manual gates had gaps. Implication: #104 (automated gates) is more important than #108 (spec). A spec is necessary but not sufficient without enforcement machinery. Risk SummaryTheater Risk: Calling items "REQUIRED" without automation creates illusion of rigor. Enforceability Gap: 50% of requirements are manual judgment calls. These will drift without active team culture. Ambiguity Risk: "Module," "user-facing," and "sample completeness" require context. Different reviewers will apply these differently. Waiver Theater: Waiver process allows optimistic author self-flagging ("approval by: [FIDO to approve in comment]") without blocking merge. Splitting Incentive: PR size limits encourage artificial splitting, which can create MORE review gaps. RecommendationAPPROVE WITH CONDITIONS:
Risk Triage
Overall: Solid foundation. Approve with waiver-process clarification. |
- Waiver template: require explicit reviewer approval before merge - Add 'module' definition examples (new module vs not) - Add #104 automation dependency note to Known Limitations Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed Challenger conditions: waiver self-veto fixed, module examples added, #104 dependency noted. |
Anchors the definition to the two concrete package boundaries (squad-sdk exports and squad-cli commands) instead of abstract 'public API' language. Covers create, update, and delete operations on the exposed TypeScript surface. Closes #106 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Definition of 'User-Facing Change' has been refined and committed to branch \squad/106-pr-requirements. The updated definition now uses a CRUD framework anchored to two concrete package boundaries:
This clarifies what counts as user-facing for CHANGELOG, documentation, and samples requirements. See commit \�918748e\ for the full definition with examples. |
Phase 2-3 of #104. Adds two new CI checks: - CHANGELOG gate: requires CHANGELOG.md update when SDK/CLI source changes - Exports map check: verifies package.json exports match barrel files Both are feature-flagged (vars.SQUAD_CHANGELOG_CHECK, vars.SQUAD_EXPORTS_CHECK) and can be skipped per-PR with labels (skip-changelog, skip-exports-check). Part 2 of 2 for repo health (Part 1: PR #108 added requirements spec). Refs #104 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Part 1 of 2: Repo Health -- PR Requirements
What
Adds the canonical PR requirements spec and PR template for the Squad repo.
Why
Issue #106 defines PR requirements. Phase 2 delivers them as versioned files.
Addresses Challenger FATAL finding: spec must be versioned, not mutable issue.
Closes Phase 2 of #106.
How
.github/PR_REQUIREMENTS.md: versioned spec (6 categories, waiver process, exemptions).github/PULL_REQUEST_TEMPLATE.md: author-facing checklistRelated Issues
Testing
npm run buildpassesnpm testpasses (179/183 suites green; 4 failures are vitest worker timeouts unrelated to markdown-only changes)Docs
N/A -- these ARE the docs
Breaking Changes
None
Waivers
None