Define PR requirements for healthy repo ecosystem (definition of done)

[diberry]

Scope

Define the canonical set of PR requirements (definition of done) for the Squad repository. This is the source of truth that reviewers check against, that CI gates enforce, and that the review process (#100) operationalizes.

Problem

PRs ship without documentation, changelog entries, export updates, or test coverage -- and reviewers have no canonical checklist to enforce. The recent bradygaster#640 (StorageProvider) ecosystem audit found 4 HIGH severity gaps (CHANGELOG missing, README section missing, docs page missing, /storage export missing) that should have been caught before review. Without a definition of done, authors don't know what to include and reviewers don't know what to check.

Related issues exist for process (#100) and automated enforcement (#104), but nothing defines the actual requirements those systems check against. This issue is that definition.

---

Definition of "User-Facing Change"

A PR contains a user-facing change if it introduces:

• A new public API export (added to subpath exports in package.json)
• A new CLI command or subcommand
• A change to an existing public API surface (breaking or new parameters)

Internal refactors, infrastructure changes, and dependency updates are NOT user-facing.

Why this matters: The requirements categories below apply conditionally to user-facing changes. Knowing what counts as user-facing prevents half of all disputes about "does this need a CHANGELOG entry?"

---

PR Requirements by Category

Requirements are ordered by review flow (how a reviewer checks them), not by importance. All marked REQUIRED must be satisfied before merge. Some are automatically enforced (CI gates); others require human review.

a) Git Hygiene (REQUIRED -- automated)

• Commits reference the issue number: "Closes #N" or "Part of #N" in commit message
• No binary files or build artifacts committed (dist/, node_modules/, .DS_Store, etc.)
• No unrelated file changes (bleed check: only files needed for the stated goal)
• Clean commit history: logical commits or squashed to a single coherent commit
• No force-pushes to shared branches (dev, main)

Enforced by: squad-ci.yml bleed check + git hooks

---

b) CI / Build (REQUIRED -- automated)

• Build passes:
  pm run build completes successfully
• All existing tests pass:
  pm test or
  pm run test
• No new build warnings introduced without justification in PR description
• Feature flags documented in code and CHANGELOG if gating new behavior

Enforced by: GitHub Actions CI pipeline

---

c) Code Quality (REQUIRED -- manual review for completeness, automated for linting)

• All new public APIs have JSDoc/TSDoc comments
• No lint errors: �slint passes
• No type errors: sc --noEmit passes
• Tests written for all new functionality
• All tests pass: �itest exits 0

Enforced by: �slint in CI + manual code review

---

d) Documentation (REQUIRED for user-facing changes -- manual review)

• CHANGELOG.md entry under [Unreleased] following Keep-a-Changelog format
  • Example: ### Added -- FeatureName (#N) -- short description
  • Required when: packages/squad-sdk/src/ or packages/squad-cli/src/ changes
• README.md section updated if adding a new feature or module to the SDK
• Docs feature page added under docs/src/content/docs/features/ if adding a user-facing capability
• docs/src/navigation.ts updated if a new docs page is added
• Docs build test updated if a new feature page is added (playwright test in CI)

Enforced by: Manual PR review (FIDO, Flight) + future automated checks in #104

---

e) Package / Exports (REQUIRED for new modules -- manual review)

• package.json subpath exports updated when new modules are added
  • Both ypes (.d.ts) and import (.js) entries required per export
  • Example: the /storage export was missing from feat(sdk): StorageProvider abstraction — complete migration + example providers bradygaster/squad#640 until the audit caught it
• No accidental dependency additions: package-lock.json changes reviewed for unexpected deps
• No export regressions: existing exports remain present and functional

Enforced by: Manual PR review + future export audit gate in #104

---

f) Samples (REQUIRED when API changes -- manual review)

• Existing sample projects updated if the PR changes a public API they use
• New user-facing features include at least one sample demonstrating usage
• Sample README.md explains what the sample does and how to run it
• Samples build and run:
  pm run build in sample directory succeeds

Enforced by: Manual PR review + #103 (Samples CI coverage)

---

Breaking Change Policy

Any PR that changes the public API in a backward-incompatible way (removal, signature change, behavior change) must:

• Document the breaking change in CHANGELOG.md under a [BREAKING] section
• Add a migration guide as a docs note in the affected feature page
• Update all sample projects to use the new API
• Include the migration path in the PR description (## Breaking Changes section)

Examples of breaking changes:

• Removing an export that was previously public
• Changing function signature (parameter name, order, type)
• Changing CLI command name or subcommand structure
• Changing configuration file format in backward-incompatible way

Examples NOT breaking:

• Adding new optional parameters
• Adding new exports
• Adding new subcommands (existing ones unchanged)
• Fixing documented behavior that was incorrect

---

Exception / Waiver Process

Recognized that emergencies (security fixes, critical bugs) require flexibility. Any requirement can be waived if documented and approved.

How to Request a Waiver

Add to PR description under a ## Waivers section:

`

Waivers

• Item waived: (b) Documentation CHANGELOG entry
• Reason: Security fix needs urgent release (CVE-2024-XXXXX)
• Approval by: [Flight/FIDO to be stated in PR review comment]
  `

Waiver Approval Authority

• Flight (architecture decisions): Internal refactors, infrastructure, module reorganization
• FIDO (quality decisions): Bug fixes, test improvements, samples, CI changes
• Both: External API changes, breaking changes

Waiver Approval Examples

Scenario	Waivable Items	Approver
Internal refactor (no API change)	Documentation, Samples	Flight
Security fix	Documentation, Samples (with follow-up issue)	FIDO
Test infrastructure change	Documentation, Exports, Samples	FIDO
CI/GitHub Actions update	Documentation, Exports, Samples	FIDO
Documentation-only change	Code Quality (tests), Exports, Samples	FIDO
Non-breaking SDK bugfix	Samples	FIDO

Documentation of Approved Waivers

Approver adds a comment to the PR:

`
Approved waiver:

• Category: (b) Documentation
• Item: CHANGELOG entry
• Reason: [brief]
• Follow-up: [if required for backfill, e.g., "Follow-up issue due by Friday"]
  `

This audit trail ensures:

1. No silent waiving (hidden in PR title or description)
2. Clear approval authority (author can't self-waive)
3. Trackability for future compliance review

Self-Waiving Is Not Allowed

• Do NOT use skip labels without explicit reviewer approval in PR comments
• Do NOT remove requirements from checklist without documented waiver
• Do NOT claim "documentation will follow" without a tracked follow-up issue

---

Edge Case Exemptions

Certain PR types are categorically exempt from specific requirements by nature. These are NOT waivers (no approval needed); they are structural exemptions.

PR Type	Exempt From	Reason
Internal refactor (no public API change)	(d) Docs, (f) Samples	No user-visible change
Test-only changes	(d) Docs, (e) Exports, (f) Samples	No production code changed
CI/GitHub Actions workflow changes	(d) Docs, (e) Exports, (f) Samples	Infrastructure only
Documentation-only changes	(c) Code Quality (tests), (e) Exports, (f) Samples	No code changes
Dependency bumps (routine maintenance)	(d) Docs feature page, (f) Samples	Routine dependency update

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.

---

PR Size Guidelines

Size	Files Changed	Guidance
Small	< 10 files	Preferred. Fast to review, easy to revert.
Medium	10-20 files	Acceptable. Justify scope in PR description.
Large	> 20 files	Must be split or justified with written explanation.
Red flag	> 50 files	STOP -- split the PR or get explicit written approval before proceeding.

The large-deletion-guard in squad-ci.yml already enforces the 50-file deletion threshold. The size guidelines above are the human-review equivalent. If splitting feels artificial (e.g., splitting a coherent 5-file feature into parts to meet size limits), that's a signal to request an exception rather than fragment the PR.

---

PR Description Template

Every PR description should answer these questions. This template will be published at .github/PULL_REQUEST_TEMPLATE.md (Phase 2 deliverable).

`

What

[One paragraph: what does this PR change?]

Why

[Problem being solved. Link the issue: Closes #N]

How

[Approach taken. Key design decisions and trade-offs.]

Testing

[What was tested and how. Manual steps or automated test names. "No changes" only if truly no user-facing change.]

Docs

[What documentation was updated. List the files. "N/A" only if truly no user-facing change.]

Breaking Changes

[Any backward-incompatible changes to public APIs. "None" if clean.]

Waivers

[If any requirements are waived (see Exception / Waiver Process section). Empty if none.]
`

---

Implementation Phases

Phase 1 -- Document requirements (THIS ISSUE)

Deliverable: Accept the requirements above as the canonical PR requirements for this repo.

Also in Phase 1: Create .github/PR_REQUIREMENTS.md as the versioned, committed source of truth. The GitHub issue becomes a discussion forum; the committed file is the enforced spec.

Why: The GitHub issue is mutable and unversioned. Anyone with write access can change it silently without PR review. A committed markdown file ensures:

• Changes go through PR review (audit trail)
• Requirements are versioned with the repo history
• CI scripts can deterministically reference a specific version

Acceptance: Team agrees on requirements + .github/PR_REQUIREMENTS.md file committed to repo.

Phase 2 -- PR template

Deliverable: Create .github/PULL_REQUEST_TEMPLATE.md with the description template above (Section: PR Description Template).

Outcome: Authors see the structured checklist when opening a PR on GitHub.

Phase 3 -- Automated CI gates (via #104)

Deliverable: Implement CI checks that enforce the REQUIRED items in categories (a)-(f).

Key gates:

• Git hygiene: bleed check, commit message validation
• Build:
  pm run build,
  pm test
• Code quality: �slint, sc --noEmit
• Documentation: CHANGELOG entry check (detects user-facing API changes), docs page presence for new features
• Exports: export map audit against src/ modules
• Samples: updated if API changed

Outcome: PRs that skip requirements fail CI before review, reducing reviewer burden.

Phase 4 -- Reviewer checklist integration (via #100)

Deliverable: Add a structured reviewer checklist to the Flight/FIDO review process that maps to these categories.

Outcome: Human review has a structured rubric, not ad-hoc judgment.

---

Known Limitations and Scope Boundaries

This section acknowledges constraints and trade-offs reflected in the requirements above.

1. Enforceability Gap

9 of 18 requirements are manual-only with no CI gate:

• README section updates (detection requires NLP)
• Docs page relevance (subjective)
• Sample completeness (subjective)
• API migration guide quality (subjective)

Consequence: Enforcement depends on consistent reviewer judgment. Without clear precedent, reviewers will judge the same item differently across PRs.

Mitigation:

• Challenger findings (Define PR requirements for healthy repo ecosystem (definition of done) #106 comments) documented for precedent
• Phase 4 (Flight/FIDO checklist) codifies consistent judgment
• Regular enforcement audits to catch drift

2. Theater Risk: "REQUIRED" vs "ENFORCED"

Calling something "REQUIRED" without automated enforcement creates an appearance of rigor without substance. Reviewers may:

• Skip items when under time pressure
• Apply inconsistently
• Create undocumented workarounds

Mitigation:

• Phase 3 prioritizes high-leverage automated gates (CHANGELOG, exports, build)
• Manual-only items clearly labeled REQUIRED (manual review) in Phase 1
• Waiver process provides escape hatch with audit trail

3. Over-Engineering for a 5-Person Team

Some requirements (samples, docs pages, feature announcement) are enterprise-grade. For a small team:

• Timelines squeeze -> requirements waived
• Samples first casualty under deadline
• Docs quality suffers

Acknowledgment: This is a conscious trade-off. The requirements define "healthy ecosystem" not "sustainable sprint velocity." Individual PRs may waive items legitimately. The goal is that the sum of PRs ships with healthy ecosystem properties, not that every PR is perfect.

Mitigation:

• Waiver process allows deliberate trade-offs
• Follow-up issues ensure backfilling (e.g., "documentation follow-up due by Friday")
• Quarterly review (Phase 1+ decision) to revisit proportionality

4. Silent Drift Risk

The requirements file in Phase 1 (.github/PR_REQUIREMENTS.md) is the source of truth. But:

• Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 CI gate scripts must be manually updated if requirements change
• Squad product: Expand team review to include doc coverage and CI improvements #100 reviewer checklist must be manually updated
• No automated sync mechanism

Consequence: Over time, requirements diverge from enforcement.

Mitigation:

• Quarterly review cycle to resync
• CI warning if PR_REQUIREMENTS.md modified but CI scripts not touched (future enhancement)
• Owner responsibility: whoever updates requirements files must coordinate with Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 and Squad product: Expand team review to include doc coverage and CI improvements #100 owners

5. Ambiguities Not Yet Resolved

• "Module" definition: Does refactoring an existing module count as "adding a module"? (Relevant for exports requirement)

  • Guidance: "Module" = new directory under packages/squad-sdk/src/ or new export namespace. Refactoring existing module does not trigger export requirement.
  • Follow-up decision: Document in .squad/decisions.md once finalized

• "User-facing" detection for CHANGELOG: Currently manual. False positives on internal refactors, false negatives on config-driven behavior changes.

  • Mitigation: Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 will implement export-diff detection (more precise than file path heuristics)

---

Relationship to Existing Issues

Issue	Role
#100 (Review completeness)	Defines the review process -- this issue defines what reviewers check against
#103 (Samples CI coverage)	Implements the "samples" requirement category (f) above
#104 (PR completeness gates)	Implements automated enforcement of REQUIRED items as CI gates

This issue is the source of truth. #100 and #104 derive their checklists from it.

---

Acceptance Criteria

• Team agrees on the requirements above as the canonical definition of done
• FIDO's Exception/Waiver Process (Condition 4) consolidated into Section 6
• FIDO's Edge Case Exemptions (Condition 5) consolidated into Section 7
• Challenger findings addressed in known limitations section (Section 8)
• .github/PR_REQUIREMENTS.md created with Phase 1 content (Phase 1 deliverable)
• .github/PULL_REQUEST_TEMPLATE.md created (Phase 2 deliverable)
• Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 references this issue and .github/PR_REQUIREMENTS.md for gate definitions
• Squad product: Expand team review to include doc coverage and CI improvements #100 references this issue for the reviewer checklist source

[diberry]

Flight -- Architecture Assessment

This issue fills a critical gap. We have enforcement machinery (#104) and process machinery (#100) but no canonical requirements document -- which means both of those issues have been building gates around an implicit, undocumented standard. That is backwards. Requirements first, enforcement second. #106 fixes that ordering.

What I like about this structure:

The six categories (Code Quality, Documentation, Package/Exports, Samples, CI/Build, Git Hygiene) map cleanly to the layers of the system. Each category has a clear owner: FIDO owns (a) and (e), PAO owns (b), EECOM owns (c) and (d), the Git Hygiene section is a shared baseline for everyone. The phase plan (Document -> Template -> Gates -> Reviewer Checklist) is the right sequence: you can't automate what you haven't defined.

Architecture concern -- scope creep risk:

The "REQUIRED for user-facing changes" qualifier on the Documentation category needs a tighter definition of "user-facing." Without it, PRs that add internal utilities will get flagged by #104 gates that check CHANGELOG diffs. Recommendation: define "user-facing" as any change to a public API, any new export in package.json, or any new CLI command. Internal refactors and test-only changes are exempt. This should be documented in the gate implementation in #104.

Architecture concern -- the PR template (Phase 2) is load-bearing:

The "Docs: N/A only if truly no user-facing change" line in the template will be the most argued-about line in every PR review. We need a clear decision on who has final authority to accept "N/A" -- I recommend Flight (architecture) for Phase 2 and #104 gate enforcement for Phase 3.

Missing item: breaking change policy

The template has a "Breaking Changes" section but there is no corresponding requirement about what to DO when there are breaking changes. Recommendation: add a requirement that breaking changes require a migration guide entry in CHANGELOG.md and a docs note in the affected feature page. This should be added to category (b) before Phase 2.

Verdict: Approve the structure. Two action items before Phase 2 begins:

1. Define "user-facing" with a precise rule (can go in this issue or in .squad/decisions.md)
2. Add breaking-change policy to category (b)

-- Flight

[diberry]

Assigning to team for review: Flight (architecture), FIDO (quality/completeness), EECOM (implementation feasibility), PAO (docs requirements).

Areas for each reviewer:

• Flight -- architecture assessment posted above. Two open action items: define "user-facing" precisely, add breaking-change policy to category (b).
• FIDO -- review categories (a) Code Quality and (e) CI/Build. Are the requirements complete? Are any unenforceable by vitest or tsc alone?
• EECOM -- review categories (c) Package/Exports and (d) Samples. Implementation feasibility of the export map check and sample build check as CI gates (feeds into Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104).
• PAO -- review category (b) Documentation. Are the CHANGELOG format, docs page, and navigation.ts requirements accurate and complete? Any docs conventions missing?

Target: team agreement on requirements before #104 gate implementation begins. Please comment with your assessment or any objections.

[diberry]

ARCHITECTURE REVIEW: Issue #106 PR Requirements Definition

VERDICT: APPROVE WITH CONDITIONS

Assessment based on:

• Repo state: No PR template exists, CI checks limited scope, exports map is manually maintained
• Cross-reference analysis: Squad product: Expand team review to include doc coverage and CI improvements #100 (review process) and Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 (CI gates) depend on this as source of truth
• Dina's (owner) early feedback already calling out two critical action items

ASSESSMENT BY SECTION:

1. SIX REQUIREMENT CATEGORIES - COMPLETE AND WELL-STRUCTURED

The breakdown (Code Quality, Documentation, Package/Exports, Samples, CI/Build, Git Hygiene) is sound and maps cleanly to ownership domains:

• Code Quality (a) + CI/Build (e) = FIDO
• Documentation (b) = content team
• Package/Exports (c) + Samples (d) = SDK maintainers
• Git Hygiene (f) = shared baseline

Each category has clear, actionable items. No obvious gaps within categories. The PR size guidance (< 10 files preferred, > 50 hard stop) is practical and already partially enforced by squad-ci.yml (50-file deletion guard exists).

2. REQUIRED VS RECOMMENDED DISTINCTIONS - CORRECT BUT NEEDS CLARIFICATION

"REQUIRED for user-facing changes" and "REQUIRED when API changes affect them" use correct conditional logic, but "user-facing" needs a precise definition before automated enforcement (issue #104). Dina is correct to flag this as risk. Recommend: document as "Public API, new exports, or CLI command additions" to avoid flag creep on internal refactors.

3. PR TEMPLATE SPEC ALIGNMENT WITH ACTUAL REPO - GOOD FIT

Template structure (What/Why/How/Testing/Docs/Breaking Changes) matches GitHub's standard PR template pattern. The "Docs: N/A only if truly no user-facing change" line will be the enforcement hotspot (Dina correctly identifies this). The template doesn't exist yet; phase 2 should add it to .github/PULL_REQUEST_TEMPLATE.md.

4. FOUR IMPLEMENTATION PHASES - SEQUENCE IS CORRECT, DEPENDENCIES SOLID

Phase 1 (this issue) -> Phase 2 (template) -> Phase 3 (CI gates via #104) -> Phase 4 (reviewer checklist via #100) is the right order. You cannot automate what you haven't defined. However:

• Phase 2 (PR template) is BLOCKING for Phase 3. Without the template in place, authors won't see the checklist and CI gates will feel arbitrary.
• Phase 3 depends on Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 and should reference this issue explicitly.
• Phase 4 depends on Squad product: Expand team review to include doc coverage and CI improvements #100 and should reference this issue explicitly.

These back-references need to be explicit in the issues themselves (already called out in the "Acceptance Criteria" section, which is good).

5. CONNECTION TO Squad product: Expand team review to include doc coverage and CI improvements #100 AND Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 - PROPER SEPARATION, NO OVERLAP

• Define PR requirements for healthy repo ecosystem (definition of done) #106 (this issue) = source of truth: WHAT the requirements are
• Squad product: Expand team review to include doc coverage and CI improvements #100 = process/people layer: how reviewers enforce them
• Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 = automation layer: how CI enforces them

Clear ownership. No overlap. #100 and #104 derive from #106 (dependency chain is backwards from normal but correct). Risk: if #106 requirements change post-#104 implementation, #104 gates could become stale. Recommend: add a decision note in .squad/decisions.md that explains the dependency chain and update cadence.

6. MISSING ITEMS IN Define PR requirements for healthy repo ecosystem (definition of done) #106

One significant gap:

• Breaking change policy: Category (b) Documentation lists a "Breaking Changes" section in the template but does NOT specify what to DO when breaking changes exist. Should add: "Breaking changes require (1) entry in CHANGELOG.md, (2) migration guide link, (3) deprecation period note if applicable." This prevents merging breaking changes without a path for users to follow.

CONDITIONAL APPROVAL - ACTION ITEMS BEFORE PHASE 2:

1. Define "user-facing" precisely in category (b) or .squad/decisions.md. Recommend: "User-facing = new public API export, new CLI command, or new subcommand. Internal refactors and test utilities exempt."

2. Add breaking change policy to category (b): "Breaking changes must include (a) CHANGELOG.md entry explaining the change, (b) migration guide or deprecation path in docs, (c) note in the affected feature page."

3. Add explicit back-references in Squad product: Expand team review to include doc coverage and CI improvements #100 and Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 pointing to Define PR requirements for healthy repo ecosystem (definition of done) #106 as the source of truth for requirement definitions.

Once those three action items are documented, the requirements definition is solid and gates/review process can be built on it.

TEAM FEEDBACK NEEDED:

• FIDO: Are categories (a) Code Quality and (e) CI/Build complete? Any unenforceable items?
• EECOM: Are categories (c) Exports and (d) Samples feasible to automate per Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 scope?
• PAO: Is category (b) Documentation complete? Any conventions missing?

VERDICT: Approve structure pending the three action items above. Phase 1 acceptance can proceed after those are resolved.

-- Flight

[diberry]

FIDO Quality Review - Issue #106

QUALITY VERDICT: APPROVE WITH CONDITIONS

This issue provides a solid foundation for PR requirements, but has gaps in testability, edge-case handling, and process clarity. Recommend addressing 3 items before Phase 2.

---

1. TESTABILITY & MEASURABILITY ASSESSMENT

Strong points:

• Code Quality (a): All items are machine-verifiable (eslint, tsc, vitest exit codes)
• CI/Build (e): All items machine-verifiable (npm run build, npm test)
• PR Size guidance: Quantified thresholds (< 10 preferred, > 50 red flag)

Weak points:

• Documentation (b): CHANGELOG format is measurable, but "README section updated if adding feature" is vague. Who decides when README update is "required"? Needs a gate or rule.
• Samples (d): "Sample README explains what it does" is subjective. No format, length, or clarity standard defined.
• Git Hygiene (f): "Clean commit history: logical commits or squashed" is subjective - no measurable standard (1 commit? N commits per feature unit?).
• Code Quality (a): JSDoc/TSDoc requirement says "All new public APIs" - but doesn't define whether this includes internal exports, re-exports, or only top-level exports from package.json.

Recommendation: Before Phase 2, add measurable acceptance rules:

• README: "Feature + API section added (code example required)"
• Samples: "Sample README includes: purpose, dependencies, build command, expected output"
• Commits: "Per-feature unit max 5 commits, or 1 squashed commit if >5"
• JSDoc: "Applies to exports defined in package.json exports map only"

---

2. EDGE CASES NOT COVERED

Critical gaps:

1. Internal refactor / no user-facing change

   • PR touches packages/squad-sdk/src/ but only reorganizes code (no new API, no new export)
   • Issue Define PR requirements for healthy repo ecosystem (definition of done) #106 requires CHANGELOG entry for ANY SDK src changes, but no exception for refactors
   • Gate Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 checks file path, not intent - will false-positive on refactors
   • Recommendation: Add exemption rule: "If PR only modifies internal implementation (no exports map changes, no CHANGELOG-worthy change), document in PR description: 'Internal refactor - no user-facing change'"

2. Documentation-only changes

   • PR adds docs page for existing feature (no code change)
   • Category (b) requires "README.md section updated if adding feature" - but who decides if docs change requires README update?
   • Issue doesn't clarify: docs-only PRs should have lower bar (no CHANGELOG, no exports check)

3. Bug fix to .squad/ or test infrastructure

   • PR fixes .squad/team.md or .squad/decisions.md or modifies test-fixtures/
   • Category (f) says "No unrelated file changes" - but .squad/ is internal team state, not user-facing
   • Recommendation: Define "user-facing" scope clearly - suggest: ".squad/, test-fixtures/, scripts/ are exempt from CHANGELOG/export requirements"

4. CI/workflow-only change

   • PR modifies .github/workflows/ or squad-ci.yml (no SDK/CLI change)
   • Category (b) doc requirements don't apply, but (e) CI/Build still requires commit message reference
   • Recommendation: Clarify that workflow-only PRs skip categories (a)-(d), follow only (e) and (f)

5. Breaking change handling

   • Flight already flagged: "The template has a Breaking Changes section but no corresponding requirement about what to DO"
   • Recommendation: Add to category (b): "Breaking changes require a migration guide entry in CHANGELOG.md and a deprecation note in the affected feature docs page (3-sprint transition documented)"

---

3. VALIDATION METHOD: MANUAL VS AUTOMATED

Matrix of requirements:

Requirement	Manual/Auto	Gate in #104	Enforcement
(a) Code Quality - JSDoc	Auto (linter?)	No - not mentioned	Review only
(a) eslint pass	Auto	Yes - squad-ci.yml runs today	CI blocks
(a) tsc --noEmit pass	Auto	Yes - squad-ci.yml runs today	CI blocks
(a) vitest pass	Auto	Yes - squad-ci.yml runs today	CI blocks
(b) CHANGELOG entry	Auto	Yes - Gate 1 in #104	CI blocks (when implemented)
(b) README.md updated	Manual	No	Review only
(b) Docs page added	Manual	No	Review only
(c) package.json exports	Auto	Yes - Gate 2 in #104	CI blocks (when implemented)
(c) No dependency drift	Manual	No (partial - deletion guard exists)	Review only
(d) Samples build	Auto	Yes - Gate 3 in #104 (#103)	CI blocks (when implemented)
(d) Sample README quality	Manual	No	Review only
(e) Build passes	Auto	Yes - squad-ci.yml	CI blocks
(e) Tests pass	Auto	Yes - squad-ci.yml	CI blocks
(e) No build warnings	Manual	No	Review only
(f) Issue reference in commit	Auto	Yes - repo requires signed commits	N/A
(f) No unrelated changes	Manual	Partial - bleed check only	Review only
(f) No build artifacts	Manual	Yes - deletion guard checks dist/	CI blocks
(f) Clean history	Manual	No	Review only

Gap identified: Of 18 distinct items, only 9 are or will be automated. 9 are manual-only. This is fine for quality criteria (review needed), but problematic if they're also listed as "REQUIRED" - the word "required" implies checkable.

Recommendation: Reclassify requirements as:

• REQUIRED (automated) - CI will block if missing
• REQUIRED (manual review) - reviewers must validate before approval
• STRONGLY RECOMMENDED - nice-to-have but not blocking

Current issue doesn't distinguish these clearly.

---

4. PR SIZE GUIDELINE SANITY CHECK

Brady's recent 10 PRs on bradygaster/squad:

• Median: 8.5 files (< 10 preferred)
• Range: 2-100 files
• Outliers: PR Add git safety rules to copilot-instructions.md bradygaster/squad#635 (100 files), PR CI: Add source tree canary and large deletion guard bradygaster/squad#634 (100 files), PR fix(scribe): add HARD GATE archival with two-tier thresholds to Scribe workflow bradygaster/squad#637 (10 files)

Context on outliers: PRs bradygaster#635 and bradygaster#634 are likely bulk refactors (migrations). The 100-file threshold in #106 makes sense - "red flag, split or justify."

Assessment: Guidelines are appropriate for this repo. However:

• Current CI only has "deletion guard at 50 files" - no pre-emptive warning at < 20
• Recommendation: Add a CI warning (non-blocking) at > 20 files with suggested split approach

Finding: PR size guideline matches Brady's historical patterns. No change needed.

---

5. REQUIREMENT PRIORITY ORDERING

Current order:

1. Code Quality (a)
2. Documentation (b)
3. Package/Exports (c)
4. Samples (d)
5. CI/Build (e)
6. Git Hygiene (f)

Issue: This order does not reflect review flow or priority.

Recommended reorder (by review timing):

1. Git Hygiene (f) - Check first: does the PR reference an issue? Are there unintended files? This filters noise before deep review.
2. CI/Build (e) - Check early: does it compile and test? If CI fails, stop here.
3. Code Quality (a) - Check next: is the code sound? JSDoc present?
4. Documentation (b) - Check in parallel: docs added for user-facing changes?
5. Package/Exports (c) - Check if SDK: did exports get updated?
6. Samples (d) - Check if SDK: do samples still build?

Current order puts Documentation 2nd but it's dependent on Code Quality being sound.

Recommendation: Reorder categories to reflect review funnel (git hygiene -> build -> code -> docs+exports+samples) or add a note: "Categories do not imply review order; reviewers may validate in parallel."

---

6. EXCEPTION / WAIVER PROCESS

Status: NOT DEFINED.

Issue does NOT address:

• Who can waive a "REQUIRED" item?
• How is a waiver documented?
• Example: PR adds internal test utilities (no CHANGELOG). Can Flight/FIDO waive the CHANGELOG check?

Gaps exposed in #104 (related issue):

• "All new gates have skip labels (per-PR override)" mentioned, but who approves a skip label?
• No clear approval chain for exceptions

Recommendation: Add to #106:
`

Exception Process

Waivers of "REQUIRED" items must be:

1. Requested in the PR description with justification
2. Approved by Flight (architecture decisions) or FIDO (quality decisions)
3. Documented in PR description: "Waived: {category}, reason: {justification}"
4. Not silenced via --skip-{gate} labels without explicit approval

Examples of approvable waivers:

• Skip CHANGELOG for internal refactors (Flight approval)
• Skip samples for non-breaking SDK fixes (FIDO approval)
• Skip docs for bug fixes to undocumented internals (PAO + FIDO approval)
  `

---

7. MAPPING: REQUIREMENTS IN #106 -> GATES IN #104

Mapping table:

#106 Category	Requirement	#104 Gate Status
(a) Code Quality	JSDoc/TSDoc	Not mentioned in #104 - manual only
(a)	eslint	Already CI'd - linter built into build
(a)	tsc --noEmit	Already CI'd - type check in build
(a)	vitest pass	Already CI'd
(b) Documentation	CHANGELOG entry	Gate 1 (PR template + check)
(b)	README section	Mentioned in template checklist, no gate
(b)	Docs feature page	Mentioned in template checklist, no gate
(b)	docs/navigation.ts	Mentioned in template checklist, no gate
(c) Package/Exports	exports updated	Gate 2 (check-exports-map.js)
(c)	dependency drift	Mentioned in template, no gate
(d) Samples	existing samples updated	Gate 3 (samples build check)
(d)	new sample + README	Gate 3 + template checklist
(e) CI/Build	build passes	Already CI'd
(e)	tests pass	Already CI'd
(e)	no new warnings	Template checklist, no gate
(f) Git Hygiene	Issue reference	Existing via signed commits
(f)	no unrelated changes	Partial - deletion guard only
(f)	no artifacts	Already CI'd - deletion guard
(f)	clean history	Template checklist, manual only
(f)	no force-push	GitHub branch protection - external

Finding: Requirements map partially to gates. 11 items have no automated gate - they depend on manual review via template checklist.

This is acceptable IF:

• PR template checklist is mandatory (Phase 2 delivery)
• Review skill in Squad product: Expand team review to include doc coverage and CI improvements #100 includes these items
• Flight/FIDO review explicitly check all non-gated items

Recommendation: Add to #106 Acceptance Criteria:
[ ] All REQUIRED items in categories (a)-(f) are either: - Automated gate in #104 CI, OR - Explicit checkbox in #100 review skill, OR - Listed in Phase 2 PR template checklist

---

8. KEY FINDINGS - COMPLETENESS GAPS

Critical issues blocking approval:

1. "User-facing change" undefined (Flight flagged) - Recommend adding decision to .squad/decisions.md before Phase 2.
2. Breaking change policy missing (Flight flagged) - Recommend adding to category (b) before Phase 2.
3. Exception waiver process undefined - Need approval chain before gate automation in Phase 3.

Important but not blocking:

4. JSDoc requirement for public APIs - Currently manual-only, not in any CI gate. Recommend adding to FIDO review skill (Squad product: Expand team review to include doc coverage and CI improvements #100).
5. "Clean commit history" standard undefined - Currently subjective. Recommend adding measurable rule (N commits per feature unit).
6. Git hygiene order - Recommend reordering categories to match review funnel.

These do NOT prevent approval of #106, but should be resolved before Phase 2 (PR template) ships.

---

FINAL ASSESSMENT

Categories (a) Code Quality and (e) CI/Build:

• Testable: YES
• Complete: YES (requirements match CI gates)
• Enforceable: YES (all automated or in review skill)

Verdict for my domain: APPROVE - Code quality requirements are solid and measurable. CI requirements map to existing checks.

---

CONDITIONS FOR APPROVAL

1. Flight must resolve "user-facing" definition (add to Define PR requirements for healthy repo ecosystem (definition of done) #106 or .squad/decisions.md)
2. Flight must add breaking-change policy to category (b) before Phase 2
3. This issue should add exception waiver process section
4. Phase 2 PR template must reference this issue (Define PR requirements for healthy repo ecosystem (definition of done) #106) in comments

If these items are added, issue becomes APPROVED.

[diberry]

Condition 4: Exception/Waiver Process

Waivers of "REQUIRED" items must be:

1. Requested in the PR description with justification under a "## Waivers" section
2. Approved by Flight (architecture decisions) or FIDO (quality decisions) via PR review comment
3. Documented in PR description: "Waived: {category item}, reason: {justification}, approved by: {reviewer}"
4. Not silenced via skip labels without explicit reviewer approval in the PR thread

Approvable waiver examples:

• Internal refactors: skip CHANGELOG, skip docs (Flight approval)
• Bug fixes to undocumented internals: skip docs (PAO + FIDO approval)
• Non-breaking SDK fixes: skip samples update (FIDO approval)
• CI-only changes: skip CHANGELOG, skip docs, skip samples (FIDO approval)
• Docs-only changes: skip Code Quality tests if no code changed (FIDO approval)

---

Condition 5: Edge Case Exemptions

Specific PR types that are automatically exempt from certain categories:

PR Type	Exempt From	Reason
Internal refactor (no public API change)	(b) Docs, (d) Samples	No user-visible change
Test-only changes	(b) Docs, (c) Exports, (d) Samples	No production code
CI/workflow changes	(b) Docs, (c) Exports, (d) Samples	Infrastructure only
Docs-only changes	(a) Code Quality (tests), (c) Exports, (d) Samples	No code changes
Dependency bumps	(b) Docs feature page, (d) Samples	Routine maintenance

---

Non-Blocking Recommendation

Consider reordering the PR requirement categories for improved clarity:

1. Git Hygiene
2. CI/Build
3. Code Quality
4. Documentation
5. Exports/SDK
6. Samples/Examples

This ordering flows from infrastructure checks -> code safety -> developer experience.

[diberry]

Challenger Review: #106 PR Requirements

Flight approved. FIDO approved. Both "with conditions." That is polite reviewer language for "this has holes but I don't want to block." My job is not to be polite. My job is to find the failure modes that sink you in 3 months.

---

[FATAL] The "source of truth" is a GitHub issue body, not a versioned artifact

The issue says "#106 is the source of truth" and "#100 and #104 derive their checklists from it." But a GitHub issue body is a mutable, unversioned document. Anyone with write access can edit it silently -- no PR, no diff, no review. If someone tweaks category (b) wording after #104 gates are implemented, the gates and the spec diverge with zero notification. You've built a canonical reference on a medium with no change control. The source of truth for a definition-of-done should be a file in the repo (e.g., docs/pr-requirements.md or .squad/pr-requirements.md) that goes through its own PR review process. Otherwise, your "source of truth" is actually "whatever the issue body said last time someone looked at it."

Recommendation: Phase 1 deliverable should be a committed markdown file, not an accepted issue. The issue becomes the discussion; the file becomes the artifact.

---

[HIGH] Acceptance criteria mix definition with implementation -- this issue can never close cleanly

Look at the acceptance criteria:

• "Team agrees on requirements" (Phase 1 -- definition)
• "Phase 2 (PR template) delivered as a follow-up PR" (implementation)
• "Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 references this issue" (external dependency)
• "Squad product: Expand team review to include doc coverage and CI improvements #100 references this issue" (external dependency)

This means #106 cannot be closed until Phase 2 ships AND #104 AND #100 are updated. But the issue description says Phase 2 is "a follow-up PR." So is #106 done when the team agrees (Phase 1) or when Phase 2 ships? If it's the former, the acceptance criteria are wrong. If it's the latter, this issue is blocked on its own follow-up work -- a dependency cycle. In practice, the issue will either stay open forever (becoming stale) or be closed prematurely (making "accepted" meaningless).

Recommendation: Split acceptance criteria into two groups: (1) criteria that close THIS issue (team agreement + committed spec file) and (2) criteria that are tracked in separate issues (#104, #100, Phase 2 template PR).

---

[HIGH] Half the "REQUIRED" items are unenforceable -- creating requirement theater

FIDO's matrix found 9 of 18 items are manual-only. But FIDO undersold the implication. Here is the failure mode: Reviewer A approves a PR that skips "README section updated" because they judged it wasn't necessary. Reviewer B blocks a different PR for the same reason. Author B points to Author A's merged PR as precedent. Now you have an inconsistently enforced "REQUIRED" item that breeds resentment and erodes trust in the entire system.

The word "REQUIRED" implies a gate. If you can't gate it, don't call it "REQUIRED" -- call it "EXPECTED" or "REVIEWED." This is not pedantic. It determines whether contributors treat the requirements as real constraints or as suggestions they can negotiate around.

Recommendation: Adopt FIDO's three-tier classification (REQUIRED/automated, REQUIRED/manual-review, STRONGLY RECOMMENDED) before Phase 2. Without this, the PR template will list 18 checkboxes that all say "required" but half are honor-system.

---

[HIGH] No waiver/exception process creates a ratchet that will jam

FIDO flagged this: no one is defined as able to waive a "REQUIRED" item. Here is the concrete failure: an urgent security fix needs to ship NOW. It touches packages/squad-sdk/src/ but there's no time to write a CHANGELOG entry, update docs, and create a sample. The CI gate (#104) blocks the merge. Who overrides? The issue doesn't say. Options: (a) someone uses a skip label -- but who approves it? (b) someone merges to a different branch -- circumventing the entire system. (c) the fix sits for hours while someone writes docs for a security patch.

Every real-world process needs an escape hatch with an audit trail. Without one, people will create their own workarounds, and those workarounds won't be documented.

Recommendation: Define a waiver process: (1) who can approve (e.g., any two team members), (2) how it's documented (e.g., skip label + comment explaining why), (3) SLA for backfilling the skipped item (e.g., follow-up issue within 48 hours).

---

[HIGH] Perverse incentive: PR size limits encourage artificial splitting

The size guidelines say < 10 files is "preferred" and > 20 "must be split or justified." A coherent feature that touches 15 files (code + tests + types + docs + CHANGELOG + exports) now faces pressure to split into two PRs: one for code, one for docs/exports. But that means the first PR ships without documentation -- which violates category (b). You've created a rule that conflicts with another rule.

Worse: an author who splits a PR into two 8-file PRs can game both the size limit AND the CHANGELOG check (by claiming "docs will follow in part 2"). Meanwhile, "part 2" never ships because the pressure to write it dissipates after "part 1" merges.

This is exactly what happened with bradygaster#640 -- the StorageProvider code shipped but the ecosystem items (CHANGELOG, docs, exports) were missing. PR splitting doesn't fix this; it accelerates it.

Recommendation: Measure PR size by category, not by total file count. A 15-file PR where 5 are code, 3 are tests, 2 are docs, and 5 are config/exports is a healthy PR -- not one that needs splitting. The size guide should say: "If > 20 files AND more than 3 unrelated concerns, split."

---

[HIGH] Dependency chain is a house of cards with no propagation mechanism

The dependency graph is: #106 (requirements) -> #104 (CI gates) -> CI enforcement, and #106 -> #100 (reviewer checklists). Flight recommended "add a decision note explaining the update cadence." That is hand-waving. Here is the real question: when someone adds a new requirement to #106 in six months, what mechanically ensures #104's CI scripts and #100's reviewer checklists get updated?

Answer: nothing. There is no automated cross-reference check. There is no "requirements version" field. There is no CI gate that says "the requirements doc was updated but the CI gate script was not." The dependency chain works on day one and silently drifts from day two onward.

Recommendation: Either (a) generate CI gate configs FROM the requirements file (single source, derived enforcement), or (b) add a CI check that compares the requirements doc's last-modified date against the gate scripts' last-modified date and warns on drift. Option (a) is better but harder.

---

[MEDIUM] CHANGELOG path trigger is trivially gameable and will false-positive on refactors

Gate 1 in #104 checks: "Did files in packages/squad-sdk/src/ change? If yes, did CHANGELOG.md change?" This has two failure modes:

1. False positive: An internal refactor (rename, reorganize, extract helper) touches SDK src files but has zero user-facing impact. The gate demands a CHANGELOG entry. The author writes "Internal refactor" in the CHANGELOG to satisfy the gate. Your CHANGELOG fills with noise entries, and users stop reading it.

2. False negative: A contributor modifies the CLI's behavior by changing a config file or a script outside packages/squad-cli/src/. The gate doesn't trigger. User-facing change ships without CHANGELOG entry.

FIDO flagged the refactor case but not the false-negative case. Both matter.

Recommendation: Gate logic should check export map changes and public API surface diffs, not file paths. If the public API didn't change, no CHANGELOG entry needed. If it did, entry is required regardless of which files changed.

---

[MEDIUM] "User-facing" is STILL undefined after two reviewers flagged it

Both Flight and FIDO independently said "define user-facing." Dina's assignment comment repeated it. The FIDO review even proposed a definition. And yet the issue body still says "REQUIRED for user-facing changes" without defining the term. This is the single most important qualifier in the entire document and it remains ambiguous.

Every downstream system (#100 reviewer checklists, #104 CI gates, the PR template) inherits this ambiguity. Every PR review will re-litigate "is this user-facing?" from scratch. This is not a gap that can be fixed later -- it is the gap that makes everything else either too strict or too lenient.

Recommendation: Do not close Phase 1 without a committed, precise definition. Flight's proposal ("new public API export, new CLI command, or new subcommand; internal refactors exempt") is fine. Pick it, write it down, commit it.

---

[MEDIUM] Retroactive test against bradygaster#640: requirements would catch 3 of 4 gaps, but would also block good work

The issue claims bradygaster#640 had 4 HIGH gaps: CHANGELOG missing, README missing, docs page missing, /storage export missing. Under #106 requirements:

• CHANGELOG missing: caught by category (b) -- YES
• README missing: caught by category (b) -- YES
• Docs page missing: caught by category (b) -- YES
• /storage export missing: caught by category (c) -- MAYBE

The export gap is a "maybe" because category (c) says "package.json subpath exports updated when new modules are added." But was StorageProvider a "new module" or a "new file in an existing module"? The spec doesn't define "module." If I add src/storage/provider.ts to an existing barrel export, is that a "new module"? The requirement is ambiguous at exactly the boundary where bradygaster#640 failed.

More concerning: would #106 have BLOCKED good work? If the author had been writing the StorageProvider implementation iteratively across multiple PRs (code first, docs later), the documentation requirement would have blocked the first PR. Is that the desired behavior? For a 5-person team iterating fast, blocking code PRs on docs completion slows velocity for uncertain quality gain.

Recommendation: Clarify "module" with the same precision as "user-facing." Consider allowing a "docs-to-follow" label that creates a tracking issue for docs debt, with a 1-sprint SLA.

---

[MEDIUM] Cargo culting: sample requirement is enterprise overhead for a 5-person team

Category (d) requires "New user-facing features include at least one sample demonstrating usage." For a team of 5 building an AI framework, this means every feature PR must also ship a sample project. In practice, samples are the first thing cut under deadline pressure. Once one PR ships without a sample (with a "will add later" comment), it becomes precedent for every subsequent PR.

The real question is: does this team actually use samples as a development or testing tool? If yes, the requirement is justified. If samples are documentation artifacts (which is what they look like in the samples/ directory), they should be tracked separately with their own cadence -- not blocking feature PRs.

Recommendation: Make samples a RECOMMENDED item, not REQUIRED. Track sample coverage as a separate metric. If sample coverage drops below a threshold, escalate -- but don't block individual PRs.

---

[LOW] Category ordering implies priority but the issue says it doesn't

Categories are labeled (a) through (f), which implies precedence. FIDO recommended reordering by review flow. The issue doesn't state whether ordering matters. If a reviewer reads (a)-(f) as priority-ordered, they'll spend time on JSDoc comments before checking if the build passes -- which is backwards. If ordering doesn't matter, say so explicitly. If it does, reorder to match review flow.

---

[LOW] No sunset or versioning mechanism for requirements

Requirements accrete. In 12 months this list will have 30 items and no one will remember why half of them exist. There is no mechanism to review, prune, or version the requirements. Consider adding a "reviewed on" date and a quarterly review cadence.

---

Overall Risk Assessment: MEDIUM-HIGH

The structure is sound. The categories are reasonable. The phase plan is the right sequence. But the execution will fail on three fronts:

1. Ambiguity -- "user-facing" and "module" are undefined, making half the rules uninterpretable
2. Enforceability -- 9 of 18 items are manual-only with no waiver process, creating inconsistent enforcement
3. Drift -- the source of truth is mutable and unversioned, with no mechanism to propagate changes to downstream systems

Top 3 Recommendations

1. Commit the spec to a file in the repo (not just an issue body). Make changes go through PR review. This is the single highest-leverage fix.
2. Define "user-facing" and "module" precisely before closing Phase 1. Everything downstream inherits this ambiguity.
3. Add a waiver process with approval authority, documentation requirements, and backfill SLA. Without this, the system will jam on its first urgent fix.

-- Challenger

[diberry]

PAO Review: Category (b) Documentation

Scope

Reviewed the documentation requirements in issue #106, category (b), with focus on:

• CHANGELOG format and update requirements
• Conditions for when docs are required (user-facing definition)
• Docs page structure and navigation.ts integration
• Docs build test practicality
• Breaking change policy (CHANGELOG + migration guide + deprecation note)
• Missing documentation conventions

Findings

[GOOD] CHANGELOG Format is Correct

The Keep-a-Changelog format specified in the issue matches the actual repo CHANGELOG.md. The example given ("### Added -- FeatureName (#N) -- short description") aligns with entries like "### Added -- StorageProvider Abstraction Layer" and sub-entries with (bradygaster#640) issue references. This is consistent and enforced by maintainers on current PRs.

Verdict: APPROVE

[GOOD] Keep-a-Changelog Nested Structure Works

Current CHANGELOG uses two-tier structure: main category (### Added) with feature grouping (-- Feature Name), then bullet points with issue refs. This works well for complex features with multiple related entries (e.g., StorageProvider PR bradygaster#640 has 6 sub-items). The proposed format scales.

Verdict: APPROVE

[NEEDS WORK] "User-Facing" Definition is Too Vague

Flight defined "user-facing" as "any change in packages/squad-sdk/src/ or packages/squad-cli/src/" BUT the category header says "REQUIRED for user-facing changes" (not all SDK/CLI changes). Internal utility refactors in squad-sdk should not require CHANGELOG entries, but the rule would force them.

Recommendation: Clarify with explicit examples:

• User-facing INCLUDE: new APIs, CLI commands, config options, behavior changes visible to SDK users
• User-facing EXCLUDE: internal refactors, test-only utilities, private modules not exported in package.json

Verdict: NEEDS WORK -- must disambiguate before Phase 2

[GOOD] README Update Requirement is Sound

The condition "if adding a new feature or module" is clear. Current README.md documents high-level concepts well (Team Setup, Extensibility, etc. are present). This aligns with the navigation.ts Features section which has 33 documented features.

Verdict: APPROVE

[GOOD] Docs Page Structure Matches Repo Reality

Requirement says "add docs page under docs/src/content/docs/features/" -- this exists and is actively used. Navigation.ts explicitly references this: { title: 'Features', dir: 'features', items: [...] } with 33 current feature pages.

Verdict: APPROVE

[GOOD] Navigation.ts Update Requirement is Complete

When a new feature page is added (e.g., docs/src/content/docs/features/my-feature.md), navigation.ts must include a NavItem:

{ title: 'My Feature', slug: 'features/my-feature' }

This is the authoritative nav system. The requirement correctly identifies the single point of integration.

Verdict: APPROVE

[GOOD] Docs Build Test Requirement is Practical

Playwright tests exist in docs/tests/ (search.spec.mjs, astro-features.test.mjs, api-reference.spec.mjs) with config at docs/playwright.config.mjs targeting 'http://localhost:4321/squad/'. Adding a new feature page should trigger a regression test (e.g., verify the new page renders, appears in search index, has correct sidebar entry). This is practical for CI.

Verdict: APPROVE

[GOOD] Breaking Change Policy is Sound

The requirement "CHANGELOG entry + migration guide + deprecation note" is correct:

1. CHANGELOG entry documents the breaking change and issue reference
2. Migration guide (separate doc page like docs/src/content/docs/get-started/migration.md) explains upgrade path
3. Deprecation note (inline code comment or docs warning) signals the old API is unsafe

This matches mature OSS practice (React, Vue, TypeScript all follow this). Current CHANGELOG "Migration Guide" entry under [0.9.0] shows this is already practiced.

Verdict: APPROVE

[MISSING] No Frontmatter Format Specified

Feature docs like team-setup.md start with # Team Setup & Init Mode (Markdown H1), not YAML frontmatter. If new docs pages must have:

• Title (H1 Markdown)
• Optional: "Experimental" warning (shown in team-setup.md)
• Optional: "Try this" example blocks

The issue should specify frontmatter conventions (or explicitly say "use Markdown H1, no YAML frontmatter").

Recommendation: Add to category (b):

• "New docs pages use Markdown H1 as title (no YAML frontmatter)"
• "Mark experimental features with > WARNING -- Experimental block"
• "If feature has quick examples, use 'Try this' prompt blocks (see team-setup.md)"

Verdict: NEEDS WORK -- document conventions for consistency

[MISSING] No Cross-Linking Standard

The issue doesn't specify how to link between docs pages. Should feature pages link back to README? Should migration guides link to affected feature pages? Current nav is hierarchical but not interconnected in the markdown source.

Recommendation (optional): Consider adding:

• "Link related feature pages using relative markdown links: Storage Provider"
• Or defer to Phase 4 (reviewer checklist) for human judgment

Verdict: COULD IMPROVE (lower priority)

[MISSING] Sidebar Labels Not Addressed

Navigation.ts controls sidebar labels (e.g., { title: 'Storage Provider', slug: 'features/storage-provider' }). If a feature page H1 doesn't match the nav title, readers see inconsistent labeling. Should the requirement specify: "H1 title must match navigation.ts title"?

Recommendation: Add clarity: "Feature page H1 title should match navigation.ts entry title for UI consistency"

Verdict: COULD IMPROVE (cosmetic)

Risk Assessment

Overall Risk: MEDIUM

The category (b) requirements are 85% solid. CHANGELOG, navigation, and test structure are correctly specified and verified to work. However, "user-facing" ambiguity will cause future disputes about what requires CHANGELOG entries. This needs resolution before Phase 2 (PR template) or reviewers will enforce inconsistently.

Verdict

APPROVE WITH CONDITIONS

Approve category (b) documentation requirements on condition that:

1. "User-facing" is clarified with explicit examples (SDK internal utilities EXCLUDED from CHANGELOG requirement)
2. Frontmatter/formatting conventions are documented: Markdown H1, experimental warning format, etc.
3. Both clarifications added to issue description before Phase 2 closes

Breaking change policy, CHANGELOG format, navigation.ts integration, and docs build tests are all correct as stated.

Recommendation for Follow-Up

Add to issue description under category (b):

### Clarifications Added After Review

- User-facing INCLUDE: new APIs, CLI commands, config options, behavior changes in public exports
- User-facing EXCLUDE: internal refactors, test utilities, private modules not in package.json exports
- Feature docs frontmatter: Use Markdown H1 title (no YAML). Mark experimental with `> WARNING` block.
- Navigation.ts: Feature page H1 should match navigation.ts title for UI consistency.

Then Phase 2 (PR template) can reference this unambiguous definition.

[diberry]

EECOM Review: Categories (c) Package/Exports and (d) Samples

=== CATEGORY (C): PACKAGE/EXPORTS ===

VERDICT: APPROVE WITH IMPORTANT CONDITIONS

Assessment Summary:
I reviewed this against the SDK's current 38+ subpath exports in squad-sdk/package.json and the StorageProvider incident from bradygaster#640. The category requirements are sound but need precision before #104 automation.

FINDINGS:

1. EXPORT REQUIREMENT COMPLETENESS: YES, with automation caveats

   • The "both types and import entries required" rule is correct and would have caught the feat(sdk): StorageProvider abstraction — complete migration + example providers bradygaster/squad#640 /storage export omission
   • Current squad-sdk has 38+ subpath exports, all properly structured with types + import pairs
   • Rule is complete for new modules: any addition to the exports map requires both fields
   • NOTE: Definition of "new module" remains ambiguous (Challenger flagged this; I agree). Is it a new top-level export key? A new barrel file? This should be clarified in Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 gate implementation to avoid false positives on internal reorganization

2. PACKAGE-LOCK.JSON REVIEW PRACTICALITY: YES, reviewable but not automatable

   • Checking for unexpected dependency additions is realistic for human review (look at what changed in the dependency tree)
   • Current squad-ci.yml does NOT automate this check -- it's manual-only
   • Practical concern: most PR additions are expected (testing deps, build tools). False-positive risk is real unless reviewers have a whitelist of "expected" addition patterns
   • Recommend: during Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 implementation, add a note in the gate that squad-lock.json changes are "approval required" before merge, not an automated block
   • For accelerated PRs (security fixes), maintainer can approve lock file changes with a waiver (covered by Condition 1 in FIDO's exception process)

3. EXPORT MAP CHECK AS CI GATE FOR Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104: FEASIBLE WITH CAVEATS

   • Gate logic could check: "Did package.json exports map change? If yes, verify all new exports have both types and import entries"
   • This is automatable: parse JSON, verify structure, fail if missing field
   • Implementation approach: Read packages/squad-sdk/package.json, iterate exports map, confirm each export has both types and import properties
   • Risk: gate will trigger on ANY exports change, including refactoring existing exports. Gate should verify "no regressions" (no removed exports without migration path). This is harder to automate but critical
   • Recommendation for Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104: build two checks: (a) new exports are complete (types+import), (b) no exports removed without prior deprecation notice
   • Effort estimate: 30 lines of Node.js, feasible to implement in squad-ci.yml

4. SDK'S 38+ SUBPATH EXPORTS: MANAGEABLE BUT REQUIRES DISCIPLINE

   • Audit of squad-sdk/package.json: confirmed 38+ subpath exports, all properly structured
   • Current export pattern (barrel files + explicit subpath entries) scales well
   • Risk: export map can silently get out of sync with actual dist/ files. If a file is deleted but export remains, imports will fail at runtime with confusing errors
   • Recommendation: add a CI gate that validates exports actually exist in dist/ after build (parse exports map, stat each .d.ts and .js file, error if missing). This prevents stale exports
   • Effort: another 20 lines of Node.js

CONDITION 1: Before Phase 2, define "new module" precisely

• Example rule: "A new module is a new key in the exports map that did not exist before. Refactoring existing exports (changing the target path) is not a new module."
• This prevents false positives in Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 gates

CONDITION 2: Automate export map completeness check in #104

• Gate should verify new exports have both types and import entries
• Also verify no exports removed without migration notes

CONDITION 3: Add optional "export existence check" gate in Phase 3

• Validate that all exports point to existing files in dist/
• Prevents stale/broken exports from shipping

---

=== CATEGORY (D): SAMPLES ===

VERDICT: APPROVE WITH RESERVATIONS

Assessment Summary:
The "new user-facing features include at least one sample" requirement is philosophically sound but economically unrealistic for a 5-person team under deadline. Issue #103 (Samples CI coverage) provides the foundation this requirement needs. I recommend making samples STRONGLY RECOMMENDED (not REQUIRED) for Phase 1, with clear escalation to REQUIRED once #103 CI is in place.

FINDINGS:

1. "NEW USER-FACING FEATURES INCLUDE AT LEAST ONE SAMPLE": IDEALISTIC BUT RISKY AS REQUIRED

   • The intent is correct: samples teach users, reduce support burden, provide integration examples
   • Reality check against recent PRs: PR feat(sdk): StorageProvider abstraction — complete migration + example providers bradygaster/squad#640 (StorageProvider) ships two new samples -- but only because storage is a new subsystem. Smaller feature PRs rarely ship samples in practice
   • Risk: if samples are REQUIRED and Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 CI is not yet active, samples won't build in CI before merge. Authors will ship broken samples. Once one broken sample merges, precedent is set and subsequent samples quality drops
   • My recommendation: keep as REQUIRED in the spec (intent is right) but in Phase 2 template, gate it with: "New samples added? They must build:
     pm run build --if-present passes in the sample's directory. If no sample is added, justify in the PR description why this feature doesn't warrant a sample."
   • Action item for Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104: samples-specific CI gate should run AFTER Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 is complete, not in parallel

2. SAMPLE BUILD CHECKS: CI NOT MANUAL

   • Manual sample build checks are not scalable. PR reviewers cannot be expected to cd samples/{name} && npm install && npm run build for each of 11 samples on every PR
   • Build checks should be fully automated in CI (gate in Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104, implementable with Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103)
   • Effort: Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 workflow already designed to do this; just needs implementation
   • Recommendation: Phase 2 template should say "Sample build validation is CI-enforced (see Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103). Reviewers do not manually validate samples."

3. SAMPLE README REQUIREMENTS: INSUFFICIENT WITHOUT FORMAT STANDARD

   • The issue says "Sample README.md explains what the sample does and how to run it" (category d)
   • This is vague. FIDO flagged this; Challenger echoed it. I agree: "explains" is subjective
   • Audit of existing samples: most follow SAMPLE-README-TEMPLATE.md, which provides structure (prerequisites, quickstart, what-you-learn, how-it-works, expected-output, key-files, next-steps)
   • Recommendation: update category (d) to reference the template by name: "Sample README.md must follow SAMPLE-README-TEMPLATE.md and include: prerequisites, quick start steps, learning objectives, architecture explanation, expected output example, key files table"
   • This makes the requirement testable (template conformance) rather than subjective (quality judgment)
   • Effort: editorial, no implementation cost

4. INTERACTION WITH Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 (SAMPLES CI COVERAGE): TIGHT COUPLING, CLEAR OWNERSHIP

   • Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 provides the CI infrastructure that makes category (d) requirements enforceable
   • Current state: Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 is OPEN but not implemented. Therefore, category (d) sample build validation cannot be enforced
   • Dependency: Phase 2 cannot complete until Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 implementation is in progress. Otherwise, authors will add samples but CI won't validate them
   • Recommendation: add explicit dependency in Define PR requirements for healthy repo ecosystem (definition of done) #106: "Category (d) Samples requirements are Phase 2-aligned with Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 CI coverage implementation. Phase 3 gates (Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104) will reference Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 CI workflow for sample validation."

5. SAMPLES AS A DISTRIBUTION MECHANISM, NOT JUST DOCUMENTATION

   • Samples serve dual purpose: (1) documentation/learning for users, (2) integration test for SDK changes
   • Category (d) only addresses (1). The real value is (2), which is owned by Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103
   • Recommendation: clarify in Define PR requirements for healthy repo ecosystem (definition of done) #106 that "new user-facing features include at least one sample" serves the learning purpose, while Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 CI serves the integration-test purpose. Both are needed

CONDITION 1: Rephrase category (d) to reference SAMPLE-README-TEMPLATE.md explicitly

• Change "Sample README explains what the sample does and how to run it" to "Sample README.md follows SAMPLE-README-TEMPLATE.md structure and includes prerequisites, quick start, learning objectives, architecture, expected output, and key files"
• Makes requirement testable, not subjective

CONDITION 2: Clarify sample buildability requirement source

• Say: "Samples must build in CI (enforced by Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 workflow). Build failure blocks PR merge."
• This avoids ambiguity about manual vs automated checks

CONDITION 3: Add explicit dependency on #103 being in-flight or complete

• Reference Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 in Phase 2 acceptance criteria
• Clarify: Phase 2 template can describe sample requirements, but Phase 3 CI enforcement depends on Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103

CONDITION 4: Mark samples as STRONGLY RECOMMENDED until #103 CI is active

• Keep as REQUIRED in spec intent (right direction)
• But acknowledge in implementation notes: "Sample requirements become fully enforceable once Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 CI workflow is deployed. Until then, samples are reviewed but not CI-blocked."

---

=== CROSS-CATEGORY OBSERVATIONS ===

1. CATEGORIES (C) AND (D) ARE TIGHTLY COUPLED TO SDK-SPECIFIC WORK

   • Not all PRs touch exports or samples. Internal refactors, bug fixes, and CI changes are exempt
   • This is correctly noted in Condition 5 (Edge Case Exemptions) from FIDO's review
   • Recommendation: reference that exemption explicitly in Define PR requirements for healthy repo ecosystem (definition of done) #106 body or Phase 2 template

2. WAIVER PROCESS IS CRITICAL FOR BOTH CATEGORIES

   • Security fix needs to ship now but no sample available? Need a waiver (Condition 1 from FIDO review)
   • Export map refactoring should not be blocked for missing exports check? Waiver applies
   • Current Define PR requirements for healthy repo ecosystem (definition of done) #106 text does not explain waiver process. FIDO added a process; it should be in Define PR requirements for healthy repo ecosystem (definition of done) #106 before Phase 2

3. AUTOMATION READINESS ASSESSMENT FOR Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104

   • Category (c) export map check: ~50 lines of code, very feasible
   • Category (d) sample build check: already designed in Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103, feasible once Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 ships
   • Both are implementable as non-blocking initial checks (Phase 3) with escalation to blocking once mature

---

FINAL VERDICT: APPROVE WITH CONDITIONS

The export and samples requirements are directionally correct and address real gaps (as proven by bradygaster#640). However:

1. "Module" needs definition to prevent false positives in export gate
2. Sample README standard needs to reference template to be testable
3. Both categories need explicit connection to Squad product: samples/ directory has zero CI coverage -- SDK changes silently break samples #103 for CI enforcement
4. Exception/waiver process must be documented before Phase 2
5. Edge case exemptions (internal refactors, docs-only, CI-only) must be called out in Phase 2

Once those five conditions are addressed, categories (c) and (d) are ready for Phase 2 template and Phase 3 CI implementation.

-- EECOM (Backend/SDK Dev)

[diberry]

Final Consolidation Complete

This revision consolidates all inputs into a single, complete PRD:

What Changed

1. Added FIDO's Condition 4: Exception/Waiver Process (Section 6) -- defines who can approve waivers, how to document them, and clear examples. No self-waiving allowed.

2. Added FIDO's Condition 5: Edge Case Exemptions (Section 7) -- categorical exemptions for refactors, test-only PRs, CI changes, docs-only PRs. No approval needed for true exemptions.

3. Reordered Categories: Git Hygiene now first (most common failure mode). Order matches review flow: git -> build -> code -> docs -> exports -> samples.

4. Added Breaking Change Policy (Section 5): Explicit requirements for breaking changes (migration guide, docs note, sample updates).

5. Added Known Limitations (Section 8): Honest assessment of enforceability gaps, theater risks, team size constraints, and drift risks. Challenger's core findings incorporated.

6. Clarified "User-Facing Change" (Section 2): Precise definition to prevent half of all PR disputes.

7. Phase 1 Updated: Now includes .github/PR_REQUIREMENTS.md as a committed, versioned source of truth (addressing Challenger's FATAL finding).

8. Updated Acceptance Criteria: Now includes all consolidation items.

Key Decisions Made

• Breaking changes: Now REQUIRED to include migration guide (not optional)
• Waivers: Manual approval with audit trail (never silent)
• Exemptions: Structural (no approval needed when genuinely inapplicable)
• Spec versioning: Committed file + GitHub issue (issue is discussion, file is spec)

What's NOT in This PRD (Out of Scope)

• Automation decisions (Phase 3 in Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104)
• Review process decisions (Phase 4 in Squad product: Expand team review to include doc coverage and CI improvements #100)
• Team escalation paths (covered in waiver process)
• Quarterly review cadence (defer to squad/decisions.md)

Next Steps

1. Team review this consolidated version
2. Flag any gaps or disagreements in comments (not edits -- edits will be reverted)
3. Once agreed, Phase 1 closes with commitment of .github/PR_REQUIREMENTS.md
4. Squad product: PR completeness gates -- prevent merging without docs, CHANGELOG, exports, and samples CI #104 and Squad product: Expand team review to include doc coverage and CI improvements #100 reference this issue + that file for their implementations

---

Status: Ready for team agreement. This is the final canonical version.

[diberry]

Evidence from PR bradygaster#640: Why Test Gates Matter

What This Reveals

PR bradygaster#640 (StorageProvider abstraction) demonstrates why the PR #106 requirements matter. The PR migrated ~90 files but failed CI 3 times on bugs that 89 seconds of local testing would have caught:

CI Run	Bug	Why It Slipped
Run 1	Template filename references changed (.template suffix issue)	Code review focused on ecosystem: CHANGELOG? Exports? Docs? Not: do tests pass?
Run 2	Async/await signature mismatches (sync -> async conversion, tests not updated)	Migration reviewed like feature PR; nobody ran npm test locally before approval
Run 3	EECOM fixed all failures	Shows that test coverage exists; human process failed, not the suite

Total: 3 CI round-trips (~45 min pipeline time) for mechanical bugs.

How This Feeds Into PR #106 Requirements

PR bradygaster#640 validates the need for:

1. Mandatory local test gate before review approval -- HIGHEST IMPACT for preventing rebase churn
2. Separate ecosystem vs correctness review paths -- ecosystem is important but not a substitute for functional verification
3. Migration PR review checklist (>20 files get mandatory test attestation)
4. npm run preflight script -- build + test, gated before push

The Gap

Reviews checked packaging (are exports wired? CHANGELOG updated?). They did not check plumbing (do tests pass?). PR #106 is about making that distinction explicit in the review process.

Key Data Point

The test suite runs in 89 seconds. That is the cost of preventing this entire class of failure.

[diberry]

Phase 2 delivered: .github/PR_REQUIREMENTS.md + .github/PULL_REQUEST_TEMPLATE.md committed to dev and pushed.