From abacce5807e5adff6c46db7f4b2021d39dd370ed Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 13:40:09 -0700 Subject: [PATCH 01/16] chore: add fork-first pipeline and bleed-check skills Fork-specific workflow skills for PR hygiene: - fork-first-pipeline: review on fork before opening upstream PRs - bleed-check: twice-daily cross-branch audit for stowaway files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .copilot/skills/bleed-check/SKILL.md | 108 +++++++++++++++++++++ .squad/skills/fork-first-pipeline/SKILL.md | 94 ++++++++++++++++++ 2 files changed, 202 insertions(+) create mode 100644 .copilot/skills/bleed-check/SKILL.md create mode 100644 .squad/skills/fork-first-pipeline/SKILL.md diff --git a/.copilot/skills/bleed-check/SKILL.md b/.copilot/skills/bleed-check/SKILL.md new file mode 100644 index 000000000..ff8064531 --- /dev/null +++ b/.copilot/skills/bleed-check/SKILL.md @@ -0,0 +1,108 @@ +# Bleed-Check: Cross-Branch Audit + +## Confidence +High + +## Domain +Periodic cross-branch bleed audits, stowaway file detection + +## Problem Statement +Features developed on forks can accidentally include files intended for \.squad/\, docs, or unrelated purposes. These "stowaway" files pollute the upstream repository if not caught before PR merge. A periodic audit across all open PRs by a contributor identifies and flags these stragglers before they reach main. + +## Trigger + +- **Scheduled**: Twice per session day (morning and afternoon sweeps) +- **Proactive**: If >4 hours since last check, coordinator offers proactive bleed audit reminder + +## Scope + +All open PRs by **diberry** targeting **bradygaster/squad** + +Query: +\\\ash +gh pr list --author diberry --repo bradygaster/squad --base dev --state open +\\\ + +## Process + +### 1. List PRs +Fetch all open PRs from diberry targeting bradygaster/squad. + +### 2. For Each PR: Check File List +Retrieve the file list: +\\\ash +gh pr view {pr-number} --repo bradygaster/squad --json files +\\\ + +### 3. Flag Stowaways +Check each file against the PR's stated purpose (from title/description). Red flags: + +| Red Flag | Example | Why It's Bad | +|---|---|---| +| \.squad/\ files | \.squad/decisions/...\, \.squad/agents/...\ | Should not ship in app PRs | +| Navigation entries | \.squad/routing.md\ changes | Wrong PR can cause nav breakage | +| Test expectations | \.squad/agents/*/expected-output\ | Unmaintainable across PRs | +| Full-file rewrites | Accidental large refactors | Out of scope, causes merge debt | +| Build artifacts | \dist/\, \uild/\, \.next/\ | Should be in \.gitignore\ | +| Root-level strays | Unexpected \.env.local\, \secrets.json\ | Likely accidental commits | + +### 4. Output Format: Emoji-Based Table + +\\\ +| PR # | Title | Status | Bleed? | Details | +|---|---|---|---|---| +| #42 | Add auth feature | 🟒 CLEAN | No | All files in scope | +| #43 | Refactor parser | πŸ”΄ BLEED | Yes | \.squad/routing.md\ found (stowaway) | +| #44 | Update docs | 🟒 CLEAN | No | Docs changes as intended | +\\\ + +Status indicators: +- 🟒 CLEAN: All files align with PR purpose +- πŸ”΄ BLEED: Stowaway files detected, PR needs cleanup + +## Coordinator Behavior + +The Copilot acting as coordinator: + +1. **Track Last Check Time**: Record timestamp of last bleed audit +2. **Proactive Reminders**: After 4+ hours, suggest: "Hey, time for a bleed check? Want me to audit your open PRs?" +3. **Detailed Reports**: Show emoji table with file-by-file breakdown +4. **Actionable Guidance**: If bleed detected, suggest: "Pull request #43 has \.squad/routing.md\ - should this be removed before merging?" + +## Session Integration + +- Check at start of session for any overnight bleeds +- Offer mid-session reminder if threshold exceeded +- Report findings before upstream PR opens +- Track frequency for team metrics + +## Example Session Flow + +\\\ +βœ“ Session starts + "Last bleed check was 6 hours ago. Want me to run an audit?" + [User: Yes] + +β†’ Auditing 4 open PRs by diberry... + | #42 | auth feature | 🟒 CLEAN | + | #43 | parser refactor | πŸ”΄ BLEED | .squad/routing.md detected | + | #44 | docs update | 🟒 CLEAN | + | #45 | ui polish | 🟒 CLEAN | + +⚠️ Found 1 bleed in PR #43. Recommend removing .squad/routing.md before merging. +\\\ + +## Files to Inspect + +- PR title and description (stated purpose) +- \gh pr view --json files\ output (file manifest) +- Commit diff per file (spot-check suspect files) +- PR body for merge strategy notes + +## Non-Bleed Scenarios + +NOT considered bleed: +- Legitimate test files for the PR feature +- README or CONTRIBUTING updates (if PR documents the change) +- New src/ files (app code is in scope) +- Configuration files directly related to the feature (tsconfig.json tweaks, etc.) diff --git a/.squad/skills/fork-first-pipeline/SKILL.md b/.squad/skills/fork-first-pipeline/SKILL.md new file mode 100644 index 000000000..758d244d4 --- /dev/null +++ b/.squad/skills/fork-first-pipeline/SKILL.md @@ -0,0 +1,94 @@ +# Fork-First PR Pipeline + +## Confidence +High + +## Domain +PR workflow, cross-fork collaboration + +## Problem Statement +PRs opened directly on the upstream repository get messy iteration in public. Code review feedback creates visible churn. Force-push history is exposed. This workflow keeps development clean by staging changes on the fork first, then opening a single clean upstream PR after review is complete. + +## 8-Step Pipeline + +\\\ +BRANCH β†’ FORK PR β†’ REVIEW β†’ FIX β†’ BLEED CHECK β†’ CLEAN β†’ UPSTREAM β†’ DONE +\\\ + +### Step 1: BRANCH +Create a feature branch locally: +\\\ash +git checkout -b squad/{issue-number}-{slug} +\\\ + +### Step 2: FORK PR +Push to fork and open PR **against your fork's dev branch**: +\\\ash +git push origin {branch-name} +gh pr create --base dev --draft # Opens on fork/dev, not upstream +\\\ + +### Step 3: REVIEW +Iterate on the fork PR with teammates. Collect feedback via review comments. This happens in your fork, not upstream. + +### Step 4: FIX +Address review comments. Commit changes directly to the feature branch (don't squash yet). + +### Step 5: BLEED CHECK +Run a bleed audit to verify no stowaway files are committed. Check for: +- \.squad/\ files (should not be in app PRs) +- Navigation entries for wrong PR +- Test expectations for wrong PR +- Full-file rewrites +- Build artifacts +- Root-level strays + +If bleed detected, fix on the feature branch. + +### Step 6: CLEAN +Prepare for upstream PR: +- Squash commits into logical units +- Clean up commit messages +- Remove any \.squad/\ files if present +- Verify no \/docs/\ prefix in titles +- Remove double blank lines from description + +### Step 7: UPSTREAM +Open PR on upstream repository against \radygaster/squad:dev\: +\\\ash +gh pr create --repo bradygaster/squad --base dev --fill +\\\ + +### Step 8: DONE +Upstream PR is merged. Close or keep fork PR for reference. + +## Anti-Patterns + +| Anti-Pattern | Why It Fails | Better Way | +|---|---|---| +| Open upstream PR before fork review complete | Public iteration, messy history | Complete review cycle on fork first | +| Force-push to upstream branch | Breaks links, confuses reviewers | Squash locally, push once | +| Skip bleed check | Stowaway files merge upstream | Always audit before upstream PR | +| Commit \.squad/\ files in app PRs | Repo pollution, merge conflicts | Exclude from staging, bleed check catches this | +| Open multiple PRs per feature | Fragmented review, merge chaos | One upstream PR per feature | + +## Pre-Upstream Gate Checklist + +Before opening the upstream PR, verify: + +- [ ] **Flight approval**: Fork PR merged into fork/dev +- [ ] **FIDO approval**: Code quality, tests pass, no security issues +- [ ] **Bleed check pass**: Zero stowaway files, no \.squad/\ commits +- [ ] **Squash commits**: 1-3 logical commits, not N from iteration +- [ ] **Clean description**: No double blank lines, clear problem/solution +- [ ] **No \.squad/\ files**: Excluded from commit entirely +- [ ] **No \/docs/\ prefix**: If docs changes, they go elsewhere +- [ ] **No double blank lines**: Markdown/description formatting clean + +## Workflow Summary + +This pipeline separates concerns: +- **Fork PR**: Messy iteration, team feedback, bleed capture +- **Upstream PR**: Clean, single-commit, ready-to-merge + +Result: Upstream PRs are lean, reviewed, and production-ready. From 46d25c99d5f32592917f91750c14185ed98ef701 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 13:42:25 -0700 Subject: [PATCH 02/16] chore: add rebase guidance to fork-first pipeline skill Adds Step 5.5 REBASE with guidance on rebasing feature branches against origin/dev to avoid full-file rewrites on shared files. Includes Shared File Strategy for surgical additions and When Rebase Fails recovery steps. Updates anti-patterns table to flag 'Skip rebase before upstream'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/skills/fork-first-pipeline/SKILL.md | 26 ++++++++++ .../content/docs/concepts/nap-vs-compact.md | 52 +++++++++++++++++++ .../content/docs/features/context-hygiene.md | 4 +- 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 docs/src/content/docs/concepts/nap-vs-compact.md diff --git a/.squad/skills/fork-first-pipeline/SKILL.md b/.squad/skills/fork-first-pipeline/SKILL.md index 758d244d4..1bdfff754 100644 --- a/.squad/skills/fork-first-pipeline/SKILL.md +++ b/.squad/skills/fork-first-pipeline/SKILL.md @@ -45,6 +45,31 @@ Run a bleed audit to verify no stowaway files are committed. Check for: If bleed detected, fix on the feature branch. +### Step 5.5: REBASE +Before squashing for upstream, rebase the feature branch against `origin/dev` to avoid full-file rewrites: +\\\ash +git fetch origin dev +git rebase origin/dev +\\\ + +#### Shared File Strategy +For files shared across PRs (navigation.ts, test files, CI workflows): +- **Never** make full-file changes on feature branches +- **Always** reset to dev first, then make surgical additions: + \\\ash + git checkout origin/dev -- docs/src/navigation.ts + # Then manually add ONLY the entries for this PR's content + \\\ +- This prevents diffs that rewrite the entire file, which cause merge conflicts with every other PR + +#### When Rebase Fails +If rebase has conflicts on shared files: +1. `git rebase --abort` +2. Reset the shared files to dev: `git checkout origin/dev -- {file}` +3. Re-add only this PR's surgical changes +4. `git commit --amend --no-edit` +5. Continue with step 6 CLEAN + ### Step 6: CLEAN Prepare for upstream PR: - Squash commits into logical units @@ -71,6 +96,7 @@ Upstream PR is merged. Close or keep fork PR for reference. | Skip bleed check | Stowaway files merge upstream | Always audit before upstream PR | | Commit \.squad/\ files in app PRs | Repo pollution, merge conflicts | Exclude from staging, bleed check catches this | | Open multiple PRs per feature | Fragmented review, merge chaos | One upstream PR per feature | +| Skip rebase before upstream | Diverged branch creates full-file diffs | Always rebase against origin/dev before step 6 | ## Pre-Upstream Gate Checklist diff --git a/docs/src/content/docs/concepts/nap-vs-compact.md b/docs/src/content/docs/concepts/nap-vs-compact.md new file mode 100644 index 000000000..cd4b54f73 --- /dev/null +++ b/docs/src/content/docs/concepts/nap-vs-compact.md @@ -0,0 +1,52 @@ +--- +title: Nap vs /compact +description: How Squad nap and Copilot CLI /compact solve different memory problems at different layers. +--- + +# Nap vs /compact + +Users hit memory limits in two ways: the chat gets too big, or agent memory piles up over time. + +The pain shows up for users as slow agents, lost context, or token pressure β€” even though the fixes happen behind the scenes. + +## How each one works + +**Squad Nap** fixes a long-term team problem by archiving shared memory to disk. Nothing is lost, and future agents stay lean. + +**/compact** fixes a right-now chat problem by summarizing and freeing tokens. Original messages are gone. + +## Comparison + +| | Squad Nap | /compact | +|---|---|---| +| Purpose | Archive squad memory | Shrink current chat | +| Scope | Persistent, shared | Ephemeral, session-only | +| Method | Move full data to disk | Replace with summary | +| Logic | Size + age rules | One-pass LLM | +| Trigger | Thresholds hit | User or token pressure | +| Benefit | Future agents | Current conversation | +| Recovery | Full (archives exist) | None | +| Model | Filing cabinet | Whiteboard erase | + +## When to use each + +**Use `squad nap` when:** +- Your `.squad/` files are growing and agents feel slower +- You've completed a phase and want to archive old work +- You're preparing to spawn new agents (smaller state = cheaper spawns) +- You want to keep a permanent archive of past decisions (they go to `-archive.md`) + +**Use `/compact` when:** +- Your current conversation is getting long and token-heavy +- You want to continue the session but free up context space for new questions +- You're running out of conversation tokens within a single session +- You want a summary of your chat history before moving on + +## Using both together + +They're **not** mutually exclusive β€” use both in sequence for optimal team hygiene: + +1. **In your Squad session:** Tell the team to `take a nap`. This cleans up persistent state files, making future agent spawns lighter and faster. +2. **After the session:** If your conversation was long, use `/compact` in the CLI to summarize the transcript. This frees tokens for the next session. + +**Bottom line: /compact helps this conversation. Squad Nap helps every conversation after this.** diff --git a/docs/src/content/docs/features/context-hygiene.md b/docs/src/content/docs/features/context-hygiene.md index bef518244..2a3317834 100644 --- a/docs/src/content/docs/features/context-hygiene.md +++ b/docs/src/content/docs/features/context-hygiene.md @@ -24,7 +24,7 @@ Over multiple sessions, Squad's `.squad/` files grow β€” agent histories, decisi ## Nap -**What it does:** Summarizes accumulated work into smaller, more efficient memory files. This is the same as running `/compact` in the CLI or `squad nap` from the command line. +**What it does:** Summarizes accumulated work in `.squad/` team state files into smaller, more efficient memory files. This compresses your persistent team knowledge β€” agent histories, decisions, and logs across multiple sessions. When you tell the team to "take a nap," each agent: @@ -52,7 +52,7 @@ squad nap --deep # Thorough cleanup with recursive descent squad nap --dry-run # Preview what would be cleaned up ``` -In the interactive shell, use `/compact` for the same effect. +> **Note:** For session-level conversation compression, use `/compact` in the Copilot CLI interactive shell. See [Nap vs /compact](/concepts/nap-vs-compact) for the difference between team state cleanup and session transcript compression. --- From 44f992f158d3a50bfeaed034345269bb4578ac1e Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 14:03:20 -0700 Subject: [PATCH 03/16] chore: update skills with session learnings (serial ops, shared files, conventions) - Fork-first-pipeline: Added serial branch operations warning, force-add note for gitignored skills, stale comments check - Bleed-check: Added high-risk shared files callout, convention gate checks, CI path debugging pattern Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .copilot/skills/bleed-check/SKILL.md | 37 ++++++++++++++++++++++ .squad/skills/fork-first-pipeline/SKILL.md | 17 ++++++++++ 2 files changed, 54 insertions(+) diff --git a/.copilot/skills/bleed-check/SKILL.md b/.copilot/skills/bleed-check/SKILL.md index ff8064531..f8f64e4fe 100644 --- a/.copilot/skills/bleed-check/SKILL.md +++ b/.copilot/skills/bleed-check/SKILL.md @@ -9,6 +9,16 @@ Periodic cross-branch bleed audits, stowaway file detection ## Problem Statement Features developed on forks can accidentally include files intended for \.squad/\, docs, or unrelated purposes. These "stowaway" files pollute the upstream repository if not caught before PR merge. A periodic audit across all open PRs by a contributor identifies and flags these stragglers before they reach main. +## High-Risk Shared Files + +**These files are the #1 bleed vectors** β€” they appear in almost every docs PR and are where cross-branch contamination happens most: + +- **`navigation.ts`** β€” contains site structure and nav entries for all features +- **`test/docs-build.test.ts`** β€” build verification tests that reference multiple PRs' output paths +- **`docs/` directories** β€” shared documentation structure + +**When checking these files, verify entries are ONLY for this PR's content β€” not entries from other concurrent PRs or stale previous runs. Flag full-file rewrites of these shared files β€” surgical additions only.** + ## Trigger - **Scheduled**: Twice per session day (morning and afternoon sweeps) @@ -46,6 +56,33 @@ Check each file against the PR's stated purpose (from title/description). Red fl | Build artifacts | \dist/\, \uild/\, \.next/\ | Should be in \.gitignore\ | | Root-level strays | Unexpected \.env.local\, \secrets.json\ | Likely accidental commits | +### 3.5: Convention Gate Checks + +While auditing files, also check for house-style violations. **These are blockers, not nits β€” per team directive.** + +| Convention | Rule | Blocker? | +|-----------|------|----------| +| Internal link format | Use bare paths like `/features/memory`, not `/docs/features/memory` | βœ… Yes | +| Blank lines | Single blank line between sections (not double) | βœ… Yes | +| Entry duplication | Each nav entry appears exactly once | βœ… Yes | +| Stale TDD comments | Clean up "RED PHASE", "TODO: implement", "WIP" markers before merge | βœ… Yes | + +### 3.6: CI Path Debugging Pattern + +When CI reports a step as successful but tests fail on a missing file, path mismatches often indicate cross-branch contamination or stale config: + +Example: CI says "generator succeeded β€” output at docs/public/" but the test looks for docs/dist/ and fails. + +**Check actual path**: +\\\ash +ls -la docs/public/ +ls -la docs/dist/ +grep "outDir" build.config.ts +grep "docs/dist" test/docs-build.test.ts +\\\ + +**Pattern**: Add `ls -la {expected-path}` verification steps when debugging CI file issues. This reveals if the build wrote to the wrong directory (often from stale config or entries from another PR). + ### 4. Output Format: Emoji-Based Table \\\ diff --git a/.squad/skills/fork-first-pipeline/SKILL.md b/.squad/skills/fork-first-pipeline/SKILL.md index 1bdfff754..e4caacd51 100644 --- a/.squad/skills/fork-first-pipeline/SKILL.md +++ b/.squad/skills/fork-first-pipeline/SKILL.md @@ -62,6 +62,17 @@ For files shared across PRs (navigation.ts, test files, CI workflows): \\\ - This prevents diffs that rewrite the entire file, which cause merge conflicts with every other PR +#### Serial Branch Operations β€” Critical Rule + +⚠️ **When multiple agents fix different PRs in parallel on the same physical repository (even if different branches), they all run `git checkout` in the same working tree.** This causes: +- Working tree conflicts (both agents try to update `.git/index` simultaneously) +- Commits landing on wrong branches (Agent A's commit lands on Agent B's branch) +- Race conditions (`.git/HEAD` and `.git/index` become inconsistent) + +**RULE: Bleed fixes across multiple PRs must be done SERIALLY (one at a time) unless using git worktrees.** Never launch parallel agents that checkout different branches in the same repo without worktrees. + +**Exception**: Use `git worktree add ../squad-195 -b squad/195-fix-issue origin/dev` to give each agent an isolated working directory. + #### When Rebase Fails If rebase has conflicts on shared files: 1. `git rebase --abort` @@ -75,9 +86,12 @@ Prepare for upstream PR: - Squash commits into logical units - Clean up commit messages - Remove any \.squad/\ files if present +- Check for stale TDD comments ("RED PHASE", "TODO: implement", "WIP") that no longer apply - Verify no \/docs/\ prefix in titles - Remove double blank lines from description +**Note on `.squad/skills/` files**: The `.squad/skills/` directory may be gitignored in some configurations. If you need to commit skill files to the fork, use `git add -f` to force them through, even if they match `.gitignore` patterns. + ### Step 7: UPSTREAM Open PR on upstream repository against \radygaster/squad:dev\: \\\ash @@ -97,6 +111,9 @@ Upstream PR is merged. Close or keep fork PR for reference. | Commit \.squad/\ files in app PRs | Repo pollution, merge conflicts | Exclude from staging, bleed check catches this | | Open multiple PRs per feature | Fragmented review, merge chaos | One upstream PR per feature | | Skip rebase before upstream | Diverged branch creates full-file diffs | Always rebase against origin/dev before step 6 | +| Parallel branch checkouts in same repo | Multiple agents switch branches simultaneously, causing working tree conflicts and commits landing on wrong branches | Use git worktrees or fix PRs serially | +| Forgetting to stash skill file updates | `.squad/skills/` files are gitignored, so they don't commit to the fork | Use `git add -f` when committing skill files | +| Leaving stale TDD comments in code | "RED PHASE", "TODO: implement" markers confuse reviewers after merge | Clean up all TDD markers before approval | ## Pre-Upstream Gate Checklist From 4f1c7326e5054f9c5aac9d1c36d05127b79b9d73 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 14:05:07 -0700 Subject: [PATCH 04/16] chore: add team review protocol to fork-first pipeline skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Dual Reviewer Gate: both Flight and FIDO must approve - Convention Issues Are Blockers: /docs/ prefix, double blanks, table order, whitespace - Reviewβ†’Fixβ†’Re-review Loop: both reviewers must re-review after fixes - Reviewer Lockout: locked authors cannot self-fix after rejection - Known PAO Quirks: watch for .squad/ files, /docs/ prefix, verification before push - Review Comments: formal PR comment format and verdict options - Tamir Reviewer Rule: only on upstream PRs with source material - Commit Hygiene: one commit, amend fixups, force-push safely, verify before push Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/skills/fork-first-pipeline/SKILL.md | 81 ++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/.squad/skills/fork-first-pipeline/SKILL.md b/.squad/skills/fork-first-pipeline/SKILL.md index e4caacd51..ac2c92642 100644 --- a/.squad/skills/fork-first-pipeline/SKILL.md +++ b/.squad/skills/fork-first-pipeline/SKILL.md @@ -128,6 +128,87 @@ Before opening the upstream PR, verify: - [ ] **No \/docs/\ prefix**: If docs changes, they go elsewhere - [ ] **No double blank lines**: Markdown/description formatting clean +## Team Review Protocol + +The fork PR review phase enforces team-wide quality standards. These patterns are mandatory. + +### Dual Reviewer Gate +Every fork PR must be reviewed by **BOTH Flight (architecture)** and **FIDO (quality)** before approval. The rule: +- **Both APPROVE**: Ready to proceed +- **One APPROVE + One REJECT**: Not ready β€” re-review cycle required +- **No single-reviewer approvals**: A partial approval does not unblock + +Both reviewers must sign off in the same review round or in sequential re-review rounds (see Reviewβ†’Fixβ†’Re-review Loop below). + +### Convention Issues Are Blockers +The following are **NOT nits** β€” they are blockers per team directive. Flight and FIDO must flag these as `NEEDS FIXES`, never as "non-blocking notes": + +- **`/docs/` prefix in internal links**: Links like `/docs/features/memory` should be bare: `/features/memory`. The /docs/ prefix is stripped at build time; links must not include it. +- **Double blank lines**: Single blank line is house style. Two or more consecutive blank lines must be reduced to one. +- **Table ordering discontinuities**: If tables reference order (e.g., step 1, 2, 3), they must be sequential without gaps. +- **Whitespace violations**: Trailing spaces, tabs instead of spaces, or inconsistent indentation. + +When a reviewer finds any of these, the review verdict is **NEEDS FIXES** with findings documented. The author must correct all flagged conventions. + +### Reviewβ†’Fixβ†’Re-review Loop +After reviewers identify blockers: + +1. **Fix**: Author addresses all blocker comments on the feature branch (use `git commit --amend --no-edit` for fast-forward fixes, or new commits if refactoring is complex). +2. **Re-run reviews**: **Both Flight AND FIDO must re-review**, even if only one reviewer found issues. This ensures convention fixes don't introduce new problems. +3. **Loop until both APPROVE**: Repeat until both reviewers vote APPROVE. +4. **Bleed check**: After both APPROVE, proceed to Step 5 (bleed check). + +Do not skip to bleed check after a single reviewer fix. + +### Reviewer Lockout +If Flight rejects a fork PR, the original author is locked out from self-revising. A **different agent** must make the fix and commit it. This is enforced mechanically: + +- **Original author**: Locked from further commits on the feature branch after rejection. +- **Different agent** (e.g., Booster stepping in for PAO): Makes the fixes, commits, and force-pushes with `--force-with-lease`. +- **Re-review**: Both Flight and FIDO re-review the updated PR. + +This rule prevents single-agent fixation loops and ensures peer review diversity. + +### Known PAO Quirks (Watch for These) +If PAO is the author, Flight and FIDO should watch for: + +- **`.squad/` file creep**: PAO often includes `.squad/agents/pao/history.md` or other `.squad/` files in commits. These must be explicitly excluded via bleed check or pre-commit rules. +- **`/docs/` prefix overuse**: PAO tends to add `/docs/` prefix to all internal links. Every reviewer must instruct "use bare paths β€” no /docs/ prefix." +- **Force-push caution**: Before PAO pushes, verify `git diff --cached --stat` matches intent and `git diff --cached --diff-filter=D` shows zero unintended deletions. + +This is not a critique β€” it is a known pattern. Treat it as a checklist, not a personal issue. + +### Review Comments Posted on PR +Formal reviews are posted to the fork PR as: +``` +gh pr comment {pr-number} --body "REVIEW: {verdict} + +## Findings +{findings} + +## Summary +{summary}" +``` + +Verdict options: `APPROVE`, `NEEDS FIXES`, `REQUEST CHANGES`. + +Findings should reference specific files, line numbers, and convention violations. Summary should restate the path forward (re-review required, bleed check next, etc.). + +### Tamir Reviewer Rule +Only assign Tamir as a reviewer on **upstream PRs** where his original code PR is the source material for the content being documented. Do not add him to every upstream PR by default. On fork PRs, Flight and FIDO are the standard reviewers. + +### Commit Hygiene +To ensure clean histories: + +- **One commit per fork PR**: Squash all review iterations into a single logical commit before opening upstream PR. +- **Amend, don't create new commits**: Use `git commit --amend --no-edit` to fix blockers without adding commit count. +- **Force-push safely**: Use `--force-with-lease` to prevent accidental overwrites if teammates push simultaneously (rare in single-agent fork setup, but good practice). +- **Always verify before push**: + ``` + git diff --cached --stat # File count must match intent + git diff --cached --diff-filter=D # Should show zero unintended deletions + ``` + ## Workflow Summary This pipeline separates concerns: From eb6ba5ae35e72df78d5c7fdab2384266a3c64847 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 14:08:16 -0700 Subject: [PATCH 05/16] chore: add undraft rule to fork-first pipeline skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a PR completes the full fork pipeline (Flight+FIDO approved, bleed check passed, squashed, rebased, moved to upstream targeting Brady's dev), the upstream PR should be undrafted immediately. The upstream PR is the final presentation β€” draft PRs signal incomplete work, but at this stage all iteration is finished on the fork. Add gh pr ready command to Step 7 (UPSTREAM) as the final action after opening the upstream PR. Add anti-pattern entry for leaving upstream PRs in draft. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/skills/fork-first-pipeline/SKILL.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/.squad/skills/fork-first-pipeline/SKILL.md b/.squad/skills/fork-first-pipeline/SKILL.md index ac2c92642..b66273b06 100644 --- a/.squad/skills/fork-first-pipeline/SKILL.md +++ b/.squad/skills/fork-first-pipeline/SKILL.md @@ -93,11 +93,17 @@ Prepare for upstream PR: **Note on `.squad/skills/` files**: The `.squad/skills/` directory may be gitignored in some configurations. If you need to commit skill files to the fork, use `git add -f` to force them through, even if they match `.gitignore` patterns. ### Step 7: UPSTREAM -Open PR on upstream repository against \radygaster/squad:dev\: -\\\ash +Open PR on upstream repository against radygaster/squad:dev: +\\\ash gh pr create --repo bradygaster/squad --base dev --fill \\\ +After the PR is created, undraft it to mark it ready for review. The upstream PR is the **final presentation** β€” it should not remain in draft: +\\\ash +gh pr ready {pr-number} --repo bradygaster/squad +\\\ + +The upstream PR signals completion of the fork pipeline. Leaving it in draft confuses reviewers β€” they may assume work is still in progress when iteration is already complete. ### Step 8: DONE Upstream PR is merged. Close or keep fork PR for reference. @@ -114,6 +120,7 @@ Upstream PR is merged. Close or keep fork PR for reference. | Parallel branch checkouts in same repo | Multiple agents switch branches simultaneously, causing working tree conflicts and commits landing on wrong branches | Use git worktrees or fix PRs serially | | Forgetting to stash skill file updates | `.squad/skills/` files are gitignored, so they don't commit to the fork | Use `git add -f` when committing skill files | | Leaving stale TDD comments in code | "RED PHASE", "TODO: implement" markers confuse reviewers after merge | Clean up all TDD markers before approval | +| Leave upstream PR in draft | Signals incomplete work when pipeline is done | Undraft upstream PR after opening β€” it's presentation-ready | ## Pre-Upstream Gate Checklist @@ -216,3 +223,5 @@ This pipeline separates concerns: - **Upstream PR**: Clean, single-commit, ready-to-merge Result: Upstream PRs are lean, reviewed, and production-ready. + + From ff55992cbd33d5c05ba4020f5e99b34180d8c555 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 10:41:22 -0700 Subject: [PATCH 06/16] test: add guard removal migration tests (closes #99) Validates that the squad-main-guard.yml workflow was properly removed: - v0.5.4 migration in index.js correctly targets and deletes the guard - TEMPLATE_MANIFEST has no reference to squad-main-guard.yml - SDK init and CLI init/upgrade never create the guard workflow - No remaining workflow templates block .squad/ paths on push to main 10 tests covering guard removal, template manifest, init/upgrade safety, and workflow template scanning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/guard-removal.test.ts | 211 +++++++++++++++++++++++++++++++++++++ 1 file changed, 211 insertions(+) create mode 100644 test/guard-removal.test.ts diff --git a/test/guard-removal.test.ts b/test/guard-removal.test.ts new file mode 100644 index 000000000..d0749e5a7 --- /dev/null +++ b/test/guard-removal.test.ts @@ -0,0 +1,211 @@ +/** + * Guard removal & Scribe commit safety tests (#99) + * + * Validates that: + * 1. The v0.5.4 migration in index.js correctly deletes squad-main-guard.yml + * 2. TEMPLATE_MANIFEST does not include squad-main-guard.yml + * 3. Init and upgrade never (re-)create the guard workflow + * 4. No remaining workflow templates block .squad/ on push to main + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { + mkdirSync, writeFileSync, readFileSync, rmSync, + existsSync, readdirSync, +} from 'node:fs'; +import { join, resolve, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { randomBytes } from 'node:crypto'; +import { initSquad } from '@bradygaster/squad-sdk'; +import type { InitOptions } from '@bradygaster/squad-sdk'; +import { runInit } from '@bradygaster/squad-cli/core/init'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const ROOT = resolve(__dirname, '..'); +const TEMPLATES_DIR = join(ROOT, 'templates'); +const WORKFLOWS_DIR = join(TEMPLATES_DIR, 'workflows'); + +// ============================================================================ +// Helpers +// ============================================================================ + +function makeTempDir(): string { + const dir = join( + process.cwd(), + `.test-guard-removal-${randomBytes(4).toString('hex')}`, + ); + mkdirSync(dir, { recursive: true }); + return dir; +} + +function sdkInitOptions(teamRoot: string): InitOptions { + return { + teamRoot, + projectName: 'guard-test', + agents: [{ name: 'test-agent', role: 'Dev' }], + configFormat: 'markdown', + includeWorkflows: true, + }; +} + +// ============================================================================ +// Test 1: v0.5.4 migration in index.js removes squad-main-guard.yml +// ============================================================================ + +describe('v0.5.4 migration removes squad-main-guard.yml', () => { + it('index.js contains a v0.5.4 migration that deletes the guard file', () => { + // Source-level verification: the bundled CLI has the migration + const indexSrc = readFileSync(join(ROOT, 'index.js'), 'utf8'); + + // Must have version 0.5.4 migration + expect(indexSrc).toContain("version: '0.5.4'"); + expect(indexSrc).toContain("'Remove squad-main-guard.yml workflow'"); + + // Must use fs.unlinkSync to delete the file + expect(indexSrc).toContain('squad-main-guard.yml'); + expect(indexSrc).toContain('unlinkSync(guardPath)'); + }); + + it('migration logic correctly targets .github/workflows/squad-main-guard.yml', () => { + const indexSrc = readFileSync(join(ROOT, 'index.js'), 'utf8'); + + // Extract the v0.5.4 migration block + const migrationMatch = indexSrc.match( + /\{\s*version:\s*'0\.5\.4'[\s\S]*?(?=\{[\s\n]*version:\s*')/, + ); + expect(migrationMatch).not.toBeNull(); + const block = migrationMatch![0]; + + // Guard path is constructed from dest + .github/workflows/ + expect(block).toContain('.github'); + expect(block).toContain('workflows'); + expect(block).toContain('squad-main-guard.yml'); + + // Conditional delete (existsSync + unlinkSync) + expect(block).toContain('existsSync'); + expect(block).toContain('unlinkSync'); + }); + + it('SDK migrations module does NOT create or reference the guard file', async () => { + const { runMigrations } = await import( + '../packages/squad-cli/dist/cli/core/migrations.js' + ); + + // Run all migrations in a temp dir β€” none should create the guard + const tmpDir = makeTempDir(); + try { + mkdirSync(join(tmpDir, '.github', 'workflows'), { recursive: true }); + await runMigrations(tmpDir, '0.0.0', '99.0.0'); + expect( + existsSync(join(tmpDir, '.github', 'workflows', 'squad-main-guard.yml')), + ).toBe(false); + } finally { + rmSync(tmpDir, { recursive: true, force: true }); + } + }); +}); + +// ============================================================================ +// Test 2: TEMPLATE_MANIFEST does not include squad-main-guard.yml +// ============================================================================ + +describe('TEMPLATE_MANIFEST excludes squad-main-guard', () => { + it('no entry references squad-main-guard in source or destination', async () => { + const { TEMPLATE_MANIFEST } = await import( + '../packages/squad-cli/dist/cli/core/templates.js' + ); + + for (const entry of TEMPLATE_MANIFEST) { + expect(entry.source).not.toContain('main-guard'); + expect(entry.destination).not.toContain('main-guard'); + } + }); + + it('no workflow template file named squad-main-guard.yml exists', () => { + const files = readdirSync(WORKFLOWS_DIR); + expect(files).not.toContain('squad-main-guard.yml'); + }); +}); + +// ============================================================================ +// Test 3: Init and upgrade do not create squad-main-guard.yml +// ============================================================================ + +describe('squad init/upgrade never creates squad-main-guard.yml', () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = makeTempDir(); + }); + + afterEach(() => { + rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('SDK initSquad() does not create the guard workflow', async () => { + await initSquad(sdkInitOptions(tmpDir)); + + const guardPath = join(tmpDir, '.github', 'workflows', 'squad-main-guard.yml'); + expect(existsSync(guardPath)).toBe(false); + }); + + it('CLI runInit() does not create the guard workflow', async () => { + await runInit(tmpDir); + + const guardPath = join(tmpDir, '.github', 'workflows', 'squad-main-guard.yml'); + expect(existsSync(guardPath)).toBe(false); + }); + + it('upgrade of existing project does not create the guard workflow', async () => { + // First init + await runInit(tmpDir); + + // Verify guard absent after init + const guardPath = join(tmpDir, '.github', 'workflows', 'squad-main-guard.yml'); + expect(existsSync(guardPath)).toBe(false); + + // Import and run upgrade + const { runUpgrade } = await import('@bradygaster/squad-cli/core/upgrade'); + await runUpgrade(tmpDir).catch(() => { + // Upgrade may fail in test env (missing git, etc.) β€” that's OK + }); + + // Guard must still be absent + expect(existsSync(guardPath)).toBe(false); + }); +}); + +// ============================================================================ +// Test 4: No workflow blocks .squad/ files on push to main +// ============================================================================ + +describe('no workflow template blocks .squad/ on push to main', () => { + it('no template workflow has guard logic blocking .squad/ paths', () => { + const workflowFiles = readdirSync(WORKFLOWS_DIR).filter(f => + f.endsWith('.yml'), + ); + expect(workflowFiles.length).toBeGreaterThan(0); + + for (const file of workflowFiles) { + const content = readFileSync(join(WORKFLOWS_DIR, file), 'utf8'); + + // A blocking guard would: push on main + .squad/ paths filter + exit 1 + const hasPushMain = + /push:[\s\S]*?branches:[\s\S]*?(?:\[.*main.*\]|main)/m.test(content); + const hasSquadPathBlock = + /paths:[\s\S]*?['"]?\.squad\/\*\*['"]?/m.test(content); + const hasExitFailure = /exit\s+1/.test(content); + + const isBlockingGuard = hasPushMain && hasSquadPathBlock && hasExitFailure; + expect( + isBlockingGuard, + `${file} should not block .squad/ commits on push to main`, + ).toBe(false); + } + }); + + it('squad-main-guard.yml is not among template workflow files', () => { + const workflowFiles = readdirSync(WORKFLOWS_DIR); + expect(workflowFiles).not.toContain('squad-main-guard.yml'); + }); +}); From 8d4edf6833fa99fa0edf6c2af71cd90371e1fd73 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 13:52:24 -0700 Subject: [PATCH 07/16] chore: push local .squad/ state and fork-first-pipeline skill update - Updated fork-first-pipeline skill with PR #640 retrospective learnings - .squad/ decisions and state synced - Merged inbox decisions into decisions.md and decisions-archive.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/decisions-archive.md | 106 +++++++++ .squad/decisions.md | 201 +++++++++--------- .../inbox/booster-ci-deletion-guard.md | 4 - .../inbox/retro-copilot-git-safety.md | 4 - .squad/skills/fork-first-pipeline/SKILL.md | 102 ++++++++- 5 files changed, 295 insertions(+), 122 deletions(-) delete mode 100644 .squad/decisions/inbox/booster-ci-deletion-guard.md delete mode 100644 .squad/decisions/inbox/retro-copilot-git-safety.md diff --git a/.squad/decisions-archive.md b/.squad/decisions-archive.md index 3d75e459f..caa844e1d 100644 --- a/.squad/decisions-archive.md +++ b/.squad/decisions-archive.md @@ -4288,3 +4288,109 @@ Migration is **non-destructive and registration-only**. When esolveSquadPath() - All future squad path resolution should go through esolveSquadPath() from multi-squad.ts - Existing esolveSquad() and esolveSquadPaths() in esolution.ts remain unchanged (project-local .squad/ walk-up) - Phase 2 CLI commands will consume these SDK functions directly +### 2026-02-21: User directive β€” no temp/memory files in repo root +**By:** Brady (via Copilot) +**What:** NEVER write temp files, issue files, or memory files to the repo root. All squad state/scratch files belong in .squad/ and ONLY .squad/. Root tree of a user's repo is sacred. +**Why:** User request β€” hard rule. Captured for all agents. + +### 2026-02-21: npm workspace protocol for monorepo +**By:** Edie (TypeScript Engineer) +**What:** Use npm-native workspace resolution (version-string references) instead of `workspace:*` protocol for cross-package dependencies. +**Why:** The `workspace:*` protocol is pnpm/Yarn-specific. npm workspaces resolve workspace packages automatically. +**Impact:** All inter-package dependencies in `packages/*/package.json` should use the actual version string, not `workspace:*`. + +### 2026-02-21: Distribution is npm-only (GitHub-native removed) +**By:** Rabin (Distribution) + Fenster (Core Dev) +**What:** Squad packages (`@bradygaster/squad-sdk` and `@bradygaster/squad-cli`) are distributed exclusively via npmjs.com. The GitHub-native `npx github:bradygaster/squad` path has been removed. +**Why:** npm is the standard distribution channel. One distribution path reduces confusion and maintenance burden. Root `cli.js` prints deprecation warning if anyone still hits the old path. + +### 2026-02-21: Coordinator prompt structure β€” three routing modes +**By:** Verbal (Prompt Engineer) +**What:** Coordinator uses structured response format: `DIRECT:` (answer inline), `ROUTE:` + `TASK:` + `CONTEXT:` (single agent), `MULTI:` (fan-out). Unrecognized formats fall back to `DIRECT`. +**Why:** Keyword prefixes are cheap to parse and reliable. Fallback-to-direct prevents silent failures. +### 2026-02-21: CLI entry point split β€” src/index.ts is a pure barrel +**By:** Edie (TypeScript Engineer) +**What:** `src/index.ts` is a pure re-export barrel with ZERO side effects. `src/cli-entry.ts` contains `main()` and all CLI routing. +**Why:** Library consumers importing `@bradygaster/squad` were triggering CLI argument parsing and `process.exit()` on import. + +### 2026-02-21: Process.exit() refactor β€” library-safe CLI functions +**By:** Kujan (SDK Expert) +**What:** `fatal()` throws `SquadError` instead of `process.exit(1)`. Only `cli-entry.ts` may call `process.exit()`. +**Pattern:** Library functions throw `SquadError`. CLI entry catches and exits. Library consumers catch for structured error handling. + +### 2026-02-21: User directive β€” docs as you go +**By:** bradygaster (via Copilot) +**What:** Doc and blog as you go during SquadUI integration work. Doesn't have to be perfect β€” keep docs updated incrementally. + +### 2026-02-22: Runtime EventBus as canonical bus +**By:** Fortier +**What:** `runtime/event-bus.ts` (colon-notation: `session:created`, `subscribe()` API) is the canonical EventBus for all orchestration classes. The `client/event-bus.ts` (dot-notation) remains for backward-compat but should not be used in new code. +**Why:** Runtime EventBus has proper error isolation β€” one handler failure doesn't crash others. + +### 2026-02-22: Subpath exports in @bradygaster/squad-sdk +**By:** Edie (TypeScript Engineer) +**What:** SDK declares subpath exports (`.`, `./parsers`, `./types`, and module paths). Each uses types-first condition ordering. +**Constraints:** Every subpath needs a source barrel. `"types"` before `"import"`. ESM-only: no `"require"` condition. + +### 2026-02-22: User directive β€” Aspire testing requirements +**By:** Brady (via Copilot) +**What:** Integration tests must launch the Aspire dashboard and validate OTel telemetry shows up. Use Playwright. Use latest Aspire bits. Reference aspire.dev (NOT learn.microsoft.com). It's "Aspire" not ".NET Aspire". + +### 2026-02-23: User directive β€” code fences +**By:** Brady (via Copilot) +**What:** Never use / or \ as code fences in GitHub issues, PRs, or comments. Only use backticks to format code. + +### 2026-02-23: User Directive β€” Docs Overhaul & Publication Pause +**By:** Brady (via Copilot) +**What:** Pause docs publication until Brady explicitly gives go-ahead. Tone: lighthearted, welcoming, fun (NOT stuffy). First doc should be "first experience" with squad CLI. All docs: brief, prompt-first, action-oriented, fun. Human tone throughout. + +### 2026-02-23: Use sendAndWait for streaming dispatch +**By:** Kovash (REPL Expert) +**What:** `dispatchToAgent()` and `dispatchToCoordinator()` use `sendAndWait()` instead of `sendMessage()`. Fallback listens for `turn_end`/`idle` if unavailable. +**Why:** `sendMessage()` is fire-and-forget β€” resolves before streaming deltas arrive. +**Impact:** Never parse `accumulated` after a bare `sendMessage()`. Always use `awaitStreamedResponse`. + +### 2026-02-23: extractDelta field priority β€” deltaContent first +**By:** Kovash (REPL Expert) +**What:** `extractDelta` priority: `deltaContent` > `delta` > `content`. Matches SDK actual format. +**Impact:** Use `deltaContent` as the canonical field name for streamed text chunks. + +### 2026-02-24: Per-command --help/-h: intercept-before-dispatch pattern +**By:** Fenster (Core Dev) +**What:** All CLI subcommands support `--help` and `-h`. Help intercepted before command routing prevents destructive commands from executing. +**Convention:** New CLI commands MUST have a `getCommandHelp()` entry with usage, description, options, and 2+ examples. + +### 2026-02-25: REPL cancellation and configurable timeout +**By:** Kovash (REPL Expert) +**What:** Ctrl+C immediately resets `processing` state. Timeout: `SQUAD_REPL_TIMEOUT` (seconds) > `SQUAD_SESSION_TIMEOUT_MS` (ms) > 600000ms default. CLI `--timeout` flag sets env var. + +### 2026-02-24: Shell Observability Metrics +**By:** Saul (Aspire & Observability) +**What:** Four metrics under `squad.shell.*` namespace, gated behind `SQUAD_TELEMETRY=1`. +**Convention:** Shell metrics require explicit consent via `SQUAD_TELEMETRY=1`, separate from OTLP endpoint activation. + +### 2026-02-23: Telemetry in both CLI and agent modes +**By:** Brady (via Copilot) +**What:** Squad should pump telemetry during BOTH modes: (1) standalone Squad CLI, and (2) running as an agent inside GitHub Copilot CLI. + +### 2026-02-24: Version format β€” bare semver canonical +**By:** Fenster +**What:** Bare semver (e.g., `0.8.5.1`) for version commands. Display contexts use `squad v{VERSION}`. + +### 2026-02-25: Help text β€” progressive disclosure +**By:** Fenster +**What:** Default `/help` shows 4 essential lines. `/help full` shows complete reference. + +### 2026-02-24: Unified status vocabulary +**By:** Marquez (CLI UX Designer) +**What:** Use `[WORK]` / `[STREAM]` / `[ERR]` / `[IDLE]` across ALL status surfaces. +**Why:** Most granular, NO_COLOR compatible, text-over-emoji, consistent across contexts. + +### 2026-02-24: Pick one tagline +**By:** Marquez (CLI UX Designer) +**What:** Use "Team of AI agents at your fingertips" everywhere. + +### 2026-02-24: User directive β€” experimental messaging +**By:** Brady (via Copilot) +**What:** CLI docs should note the project is experimental and ask users to file issues. + diff --git a/.squad/decisions.md b/.squad/decisions.md index 2f5b93a9a..38014841c 100644 --- a/.squad/decisions.md +++ b/.squad/decisions.md @@ -52,26 +52,6 @@ **What:** Squad becomes its own interactive CLI shell. `squad` with no args enters a REPL. **Why:** Squad needs to own the full interactive experience. -### 2026-02-21: User directive β€” no temp/memory files in repo root -**By:** Brady (via Copilot) -**What:** NEVER write temp files, issue files, or memory files to the repo root. All squad state/scratch files belong in .squad/ and ONLY .squad/. Root tree of a user's repo is sacred. -**Why:** User request β€” hard rule. Captured for all agents. - -### 2026-02-21: npm workspace protocol for monorepo -**By:** Edie (TypeScript Engineer) -**What:** Use npm-native workspace resolution (version-string references) instead of `workspace:*` protocol for cross-package dependencies. -**Why:** The `workspace:*` protocol is pnpm/Yarn-specific. npm workspaces resolve workspace packages automatically. -**Impact:** All inter-package dependencies in `packages/*/package.json` should use the actual version string, not `workspace:*`. - -### 2026-02-21: Distribution is npm-only (GitHub-native removed) -**By:** Rabin (Distribution) + Fenster (Core Dev) -**What:** Squad packages (`@bradygaster/squad-sdk` and `@bradygaster/squad-cli`) are distributed exclusively via npmjs.com. The GitHub-native `npx github:bradygaster/squad` path has been removed. -**Why:** npm is the standard distribution channel. One distribution path reduces confusion and maintenance burden. Root `cli.js` prints deprecation warning if anyone still hits the old path. - -### 2026-02-21: Coordinator prompt structure β€” three routing modes -**By:** Verbal (Prompt Engineer) -**What:** Coordinator uses structured response format: `DIRECT:` (answer inline), `ROUTE:` + `TASK:` + `CONTEXT:` (single agent), `MULTI:` (fan-out). Unrecognized formats fall back to `DIRECT`. -**Why:** Keyword prefixes are cheap to parse and reliable. Fallback-to-direct prevents silent failures. ### `.squad/` Directory Scope β€” Owner Directive **By:** Brady (project owner, PR #326 review) **Date:** 2026-03-10 @@ -98,97 +78,11 @@ **By:** Flight (implementing Brady's directives above) **Date:** 2026-03-09 -### 2026-02-21: CLI entry point split β€” src/index.ts is a pure barrel -**By:** Edie (TypeScript Engineer) -**What:** `src/index.ts` is a pure re-export barrel with ZERO side effects. `src/cli-entry.ts` contains `main()` and all CLI routing. -**Why:** Library consumers importing `@bradygaster/squad` were triggering CLI argument parsing and `process.exit()` on import. - -### 2026-02-21: Process.exit() refactor β€” library-safe CLI functions -**By:** Kujan (SDK Expert) -**What:** `fatal()` throws `SquadError` instead of `process.exit(1)`. Only `cli-entry.ts` may call `process.exit()`. -**Pattern:** Library functions throw `SquadError`. CLI entry catches and exits. Library consumers catch for structured error handling. - -### 2026-02-21: User directive β€” docs as you go -**By:** bradygaster (via Copilot) -**What:** Doc and blog as you go during SquadUI integration work. Doesn't have to be perfect β€” keep docs updated incrementally. - -### 2026-02-22: Runtime EventBus as canonical bus -**By:** Fortier -**What:** `runtime/event-bus.ts` (colon-notation: `session:created`, `subscribe()` API) is the canonical EventBus for all orchestration classes. The `client/event-bus.ts` (dot-notation) remains for backward-compat but should not be used in new code. -**Why:** Runtime EventBus has proper error isolation β€” one handler failure doesn't crash others. - -### 2026-02-22: Subpath exports in @bradygaster/squad-sdk -**By:** Edie (TypeScript Engineer) -**What:** SDK declares subpath exports (`.`, `./parsers`, `./types`, and module paths). Each uses types-first condition ordering. -**Constraints:** Every subpath needs a source barrel. `"types"` before `"import"`. ESM-only: no `"require"` condition. - -### 2026-02-22: User directive β€” Aspire testing requirements -**By:** Brady (via Copilot) -**What:** Integration tests must launch the Aspire dashboard and validate OTel telemetry shows up. Use Playwright. Use latest Aspire bits. Reference aspire.dev (NOT learn.microsoft.com). It's "Aspire" not ".NET Aspire". - -### 2026-02-23: User directive β€” code fences -**By:** Brady (via Copilot) -**What:** Never use / or \ as code fences in GitHub issues, PRs, or comments. Only use backticks to format code. - -### 2026-02-23: User Directive β€” Docs Overhaul & Publication Pause -**By:** Brady (via Copilot) -**What:** Pause docs publication until Brady explicitly gives go-ahead. Tone: lighthearted, welcoming, fun (NOT stuffy). First doc should be "first experience" with squad CLI. All docs: brief, prompt-first, action-oriented, fun. Human tone throughout. - -### 2026-02-23: Use sendAndWait for streaming dispatch -**By:** Kovash (REPL Expert) -**What:** `dispatchToAgent()` and `dispatchToCoordinator()` use `sendAndWait()` instead of `sendMessage()`. Fallback listens for `turn_end`/`idle` if unavailable. -**Why:** `sendMessage()` is fire-and-forget β€” resolves before streaming deltas arrive. -**Impact:** Never parse `accumulated` after a bare `sendMessage()`. Always use `awaitStreamedResponse`. - -### 2026-02-23: extractDelta field priority β€” deltaContent first -**By:** Kovash (REPL Expert) -**What:** `extractDelta` priority: `deltaContent` > `delta` > `content`. Matches SDK actual format. -**Impact:** Use `deltaContent` as the canonical field name for streamed text chunks. - -### 2026-02-24: Per-command --help/-h: intercept-before-dispatch pattern -**By:** Fenster (Core Dev) -**What:** All CLI subcommands support `--help` and `-h`. Help intercepted before command routing prevents destructive commands from executing. -**Convention:** New CLI commands MUST have a `getCommandHelp()` entry with usage, description, options, and 2+ examples. - -### 2026-02-25: REPL cancellation and configurable timeout -**By:** Kovash (REPL Expert) -**What:** Ctrl+C immediately resets `processing` state. Timeout: `SQUAD_REPL_TIMEOUT` (seconds) > `SQUAD_SESSION_TIMEOUT_MS` (ms) > 600000ms default. CLI `--timeout` flag sets env var. - -### 2026-02-24: Shell Observability Metrics -**By:** Saul (Aspire & Observability) -**What:** Four metrics under `squad.shell.*` namespace, gated behind `SQUAD_TELEMETRY=1`. -**Convention:** Shell metrics require explicit consent via `SQUAD_TELEMETRY=1`, separate from OTLP endpoint activation. - -### 2026-02-23: Telemetry in both CLI and agent modes -**By:** Brady (via Copilot) -**What:** Squad should pump telemetry during BOTH modes: (1) standalone Squad CLI, and (2) running as an agent inside GitHub Copilot CLI. - ### 2026-02-27: ASCII-only separators and NO_COLOR **By:** Cheritto (TUI Engineer) **What:** All separators use ASCII hyphens. Text-over-emoji principle: text status is primary, emoji is supplementary. **Convention:** Use ASCII hyphens for separators. Keep emoji out of status/system messages. -### 2026-02-24: Version format β€” bare semver canonical -**By:** Fenster -**What:** Bare semver (e.g., `0.8.5.1`) for version commands. Display contexts use `squad v{VERSION}`. - -### 2026-02-25: Help text β€” progressive disclosure -**By:** Fenster -**What:** Default `/help` shows 4 essential lines. `/help full` shows complete reference. - -### 2026-02-24: Unified status vocabulary -**By:** Marquez (CLI UX Designer) -**What:** Use `[WORK]` / `[STREAM]` / `[ERR]` / `[IDLE]` across ALL status surfaces. -**Why:** Most granular, NO_COLOR compatible, text-over-emoji, consistent across contexts. - -### 2026-02-24: Pick one tagline -**By:** Marquez (CLI UX Designer) -**What:** Use "Team of AI agents at your fingertips" everywhere. - -### 2026-02-24: User directive β€” experimental messaging -**By:** Brady (via Copilot) -**What:** CLI docs should note the project is experimental and ask users to file issues. - ### 2026-02-28: User directive β€” DO NOT merge PR #547 **By:** Brady (via Copilot) **What:** DO NOT merge PR #547 (Squad Remote Control). Do not touch #547 at all. @@ -8087,3 +7981,98 @@ Triaged 14 untriaged issues (3 docs, 6 community features, 3 bugs, 2 questions). - #357, #336, #335, #334, #333, #332, #316 (A2A) β€” stays shelved per existing decision - #581 (ADO PRD) β€” P2, blocked until #341 (SDK-first parity) ships + +### 2026-03-26: CI deletion guard and source tree canary +**By:** Booster (CI/CD) +**What:** Added two safety checks to squad-ci.yml: (1) source tree canary verifying critical files exist, (2) large deletion guard failing PRs that delete >50 files without 'large-deletion-approved' label. Branch protection on dev requested (may need manual setup). +**Why:** Incident #631 β€” @copilot deleted 361 files on dev with no CI gate catching it. + +### 2026-03-28T12:58: User directive β€” retarget PRs, don't create new ones +**By:** Dina Berry (via Copilot) +**What:** When promoting a PR from diberry/squad to bradygaster/squad, RETARGET the existing PR (change base repo/branch) instead of opening a new PR. This keeps the commit history, review comments, and CI status intact. +**Why:** Creating a new PR loses context, duplicates work, and clutters Brady's PR queue. Retargeting is cleaner. + +## How to retarget +```bash +# Change the base of an existing PR from diberry/squad to bradygaster/squad +gh pr edit {PR_NUMBER} --repo diberry/squad --base bradygaster:dev +``` + +If `gh pr edit` doesn't support cross-repo base changes, the alternative is: +```bash +# Close the fork PR, open upstream PR from the same branch +gh pr close {FORK_PR_NUMBER} --repo diberry/squad --comment "Retargeting to upstream" +gh pr create --repo bradygaster/squad --base dev --head diberry:{branch} --title "{title}" --body "{body}" +``` + +But prefer retarget over new PR whenever GitHub supports it. + +### 2026-03-28T14:09: User directive +**By:** Dina Berry (via Copilot) +**What:** PR management workflow must include Copilot review gates: +1. On diberry/squad: request Copilot review, iterate until all feedback resolved +2. On retarget to bradygaster/squad: if Copilot raises new comments, iterate until clean +3. Keep upstream PR in draft until: Copilot clean, 1 squashed commit, rebased from latest dev, CI passes +4. Only then mark ready for review and @mention Brady +**Why:** User request -- ensures PRs are polished before Brady sees them + +### 2026-03-28T14:24: User directive +**By:** Dina Berry (via Copilot) +**What:** When addressing Copilot review comments, ALWAYS reply to each comment on the PR with "Fixed in {commit}" (accept) or explanation (dismiss), then resolve the thread. Never silently fix code without responding to the comment. +**Why:** User request -- ensures PR comment threads are clean and resolved, not orphaned + +# Decision: Skills Discovery Reads Both Directories + +**Date:** 2025-07-24 +**Author:** EECOM +**Issue:** #77 +**PR:** #78 + +## Decision + +Skill discovery now reads BOTH `.copilot/skills/` and `.squad/skills/`, merging results. On name conflicts, `.copilot/skills/` wins (first-dir-wins priority). + +## Rationale + +- `.copilot/skills/` is the canonical write location (tools/index.ts, fresh init) +- `.squad/skills/` has historical content and upstream compatibility +- Silently dropping either directory loses team knowledge +- The merge is additive β€” no breaking change for existing users + +## Impact + +- All skill writes should target `.copilot/skills/` (squad.agent.md updated) +- `.squad/skills/` is effectively read-only legacy +- Over time, skills naturally consolidate into `.copilot/skills/` +- No migration needed β€” the runtime merge handles both dirs transparently + +# Decision: Skill Discovery Must Merge Both Directories + +**By:** Flight (Lead) +**Date:** 2025-07-24 +**Issue:** #77 +**Status:** Proposed + +## Context + +`LocalSkillSource.skillsDir` uses either/or logic β€” if `.copilot/skills/` exists, `.squad/skills/` is never read. The coordinator instructions correctly tell agents to "check BOTH skill directories," but the runtime only checks one. The spawn template (line 825) tells agents to write SKILL EXTRACTION output to `.squad/skills/`, but the skill tool writes to `.copilot/skills/`. This means agent-discovered skills are silently lost whenever `.copilot/skills/` exists (which is always, since `squad init` creates it). + +## Decision + +1. **Merge at read time.** `skill-source.ts` and `upstream/resolver.ts` will scan BOTH `.copilot/skills/` and `.squad/skills/`, deduplicating by skill ID. `.copilot/skills/` wins on name conflicts. +2. **Canonical write location is `.copilot/skills/`.** All write paths (tools/index.ts skill tool, squad.agent.md SKILL EXTRACTION, plugin install) must target `.copilot/skills/`. +3. **`.squad/skills/` becomes read-only legacy.** We do NOT delete it β€” existing skills there remain discoverable. Over time, all new skills accumulate in `.copilot/skills/`. +4. **No manual migration required.** The runtime fix is additive. Existing customers get both directories merged automatically on upgrade. + +## Impact + +- `skill-source.ts`: Replace `skillsDir` getter with multi-dir scan + dedup +- `resolver.ts`: Replace `.find()` with loop over all candidate dirs + dedup +- `squad.agent.md`: Fix SKILL EXTRACTION path (line 825), plugin install path (lines 913, 936) +- Docs: Document "where skills live" in features/skills.md and troubleshooting.md + +### 2026-03-26: Copilot git safety rules +**By:** RETRO (Security) +**What:** Added mandatory Git Safety section to copilot-instructions.md: prohibits `git add .`, requires feature branches and PRs, adds pre-push checklist, defines red-flag stop conditions. +**Why:** Incident #631 β€” @copilot used destructive staging on an incomplete working tree, deleting 361 files. + diff --git a/.squad/decisions/inbox/booster-ci-deletion-guard.md b/.squad/decisions/inbox/booster-ci-deletion-guard.md deleted file mode 100644 index 581b1f419..000000000 --- a/.squad/decisions/inbox/booster-ci-deletion-guard.md +++ /dev/null @@ -1,4 +0,0 @@ -### 2026-03-26: CI deletion guard and source tree canary -**By:** Booster (CI/CD) -**What:** Added two safety checks to squad-ci.yml: (1) source tree canary verifying critical files exist, (2) large deletion guard failing PRs that delete >50 files without 'large-deletion-approved' label. Branch protection on dev requested (may need manual setup). -**Why:** Incident #631 β€” @copilot deleted 361 files on dev with no CI gate catching it. diff --git a/.squad/decisions/inbox/retro-copilot-git-safety.md b/.squad/decisions/inbox/retro-copilot-git-safety.md deleted file mode 100644 index ed7dfc985..000000000 --- a/.squad/decisions/inbox/retro-copilot-git-safety.md +++ /dev/null @@ -1,4 +0,0 @@ -### 2026-03-26: Copilot git safety rules -**By:** RETRO (Security) -**What:** Added mandatory Git Safety section to copilot-instructions.md: prohibits `git add .`, requires feature branches and PRs, adds pre-push checklist, defines red-flag stop conditions. -**Why:** Incident #631 β€” @copilot used destructive staging on an incomplete working tree, deleting 361 files. diff --git a/.squad/skills/fork-first-pipeline/SKILL.md b/.squad/skills/fork-first-pipeline/SKILL.md index b66273b06..7e65c6a0c 100644 --- a/.squad/skills/fork-first-pipeline/SKILL.md +++ b/.squad/skills/fork-first-pipeline/SKILL.md @@ -9,10 +9,10 @@ PR workflow, cross-fork collaboration ## Problem Statement PRs opened directly on the upstream repository get messy iteration in public. Code review feedback creates visible churn. Force-push history is exposed. This workflow keeps development clean by staging changes on the fork first, then opening a single clean upstream PR after review is complete. -## 8-Step Pipeline +## 9-Step Pipeline \\\ -BRANCH β†’ FORK PR β†’ REVIEW β†’ FIX β†’ BLEED CHECK β†’ CLEAN β†’ UPSTREAM β†’ DONE +BRANCH β†’ FORK PR β†’ PREFLIGHT β†’ REVIEW β†’ FIX β†’ BLEED CHECK β†’ CLEAN β†’ UPSTREAM β†’ DONE \\\ ### Step 1: BRANCH @@ -28,11 +28,53 @@ git push origin {branch-name} gh pr create --base dev --draft # Opens on fork/dev, not upstream \\\ +### Step 2.5: PREFLIGHT TEST GATE + +Before requesting review, the author **MUST** run the full build and test suite locally: + +```bash +npm run build && npm test +``` + +This is not optional for any PR. If tests fail, fix them before requesting review. + +**PR description attestation**: Add a "Preflight" section to the PR description confirming tests pass: + +```markdown +## Preflight +- [x] `npm run build` β€” passes +- [x] `npm test` β€” passes (N suites, N tests, Ns runtime) +``` + +**Migration PRs (>20 files changed)**: Include the full test output summary (suite count, pass/fail counts, total runtime) in the PR description. This prevents the "approved but broken" pattern where reviewers check ecosystem completeness without verifying functional correctness. + +The 89-second test run is cheaper than one CI round-trip. Always run it locally. + ### Step 3: REVIEW Iterate on the fork PR with teammates. Collect feedback via review comments. This happens in your fork, not upstream. +#### Correctness Before Completeness + +Reviews **MUST** follow this order: + +1. **Functional correctness first** β€” Do tests pass? Does `npm run build && npm test` succeed? If not, stop here. Do not review ecosystem items on broken code. +2. **Mechanical correctness** β€” Are all callsites updated? Are string literals consistent? Are async/await signatures correct throughout? +3. **Ecosystem completeness last** β€” Docs, changelog, exports, navigation entries. + +If tests fail at step 1, the review verdict is **NEEDS FIXES** with a single finding: "Tests must pass before review can proceed." Do not spend reviewer time evaluating docs or changelog on code that doesn't build. + ### Step 4: FIX -Address review comments. Commit changes directly to the feature branch (don't squash yet). +Address review comments. Use **additive commits** so reviewers can see what changed between iterations. Do not squash-amend-force-push during active review β€” this hides fix iteration history and makes re-review impossible. + +```bash +# Good: additive commit during review +git commit -m "fix: await async storage calls per review feedback" + +# Bad: squash during active review +git commit --amend --no-edit && git push --force-with-lease # ← hides iteration +``` + +Squash happens later, at Step 6 (CLEAN), after CI is green and review is complete. ### Step 5: BLEED CHECK Run a bleed audit to verify no stowaway files are committed. Check for: @@ -121,19 +163,26 @@ Upstream PR is merged. Close or keep fork PR for reference. | Forgetting to stash skill file updates | `.squad/skills/` files are gitignored, so they don't commit to the fork | Use `git add -f` when committing skill files | | Leaving stale TDD comments in code | "RED PHASE", "TODO: implement" markers confuse reviewers after merge | Clean up all TDD markers before approval | | Leave upstream PR in draft | Signals incomplete work when pipeline is done | Undraft upstream PR after opening β€” it's presentation-ready | +| Approving without running tests | Reviewers check ecosystem completeness (docs, changelog, exports) but miss functional breakage | Require test attestation before review; verify correctness before completeness | +| Squash-amend-force-push during review | Hides fix iteration history, reviewers can't see what changed between rounds | Use additive commits during review, squash only at merge time | +| Reviewing a 90-file migration like a 5-file PR | Mechanical errors (missed callsites, unwired async) survive review because reviewer skims | Use Migration PR Protocol: sample files, verify mechanical patterns, require test attestation | +| Skipping preflight test run | A sub-2-minute local test run would catch failures before CI round-trips | Always run `npm run build && npm test` before requesting review | ## Pre-Upstream Gate Checklist Before opening the upstream PR, verify: +- [ ] **Preflight tests pass**: `npm run build && npm test` run locally with passing results +- [ ] **Test attestation in PR description**: Preflight section with suite count, pass/fail, runtime - [ ] **Flight approval**: Fork PR merged into fork/dev - [ ] **FIDO approval**: Code quality, tests pass, no security issues - [ ] **Bleed check pass**: Zero stowaway files, no \.squad/\ commits -- [ ] **Squash commits**: 1-3 logical commits, not N from iteration +- [ ] **Squash commits**: 1-3 logical commits, not N from iteration (squash at merge time, not during review) - [ ] **Clean description**: No double blank lines, clear problem/solution - [ ] **No \.squad/\ files**: Excluded from commit entirely - [ ] **No \/docs/\ prefix**: If docs changes, they go elsewhere - [ ] **No double blank lines**: Markdown/description formatting clean +- [ ] **Migration PR extras (if >20 files)**: Full test output summary, structured change description, additive commit history preserved until squash ## Team Review Protocol @@ -160,7 +209,7 @@ When a reviewer finds any of these, the review verdict is **NEEDS FIXES** with f ### Reviewβ†’Fixβ†’Re-review Loop After reviewers identify blockers: -1. **Fix**: Author addresses all blocker comments on the feature branch (use `git commit --amend --no-edit` for fast-forward fixes, or new commits if refactoring is complex). +1. **Fix**: Author addresses all blocker comments on the feature branch. Use additive commits with descriptive messages (e.g., `fix: await async storage calls per review feedback`). Do not squash-amend during active review β€” reviewers need to see what changed. 2. **Re-run reviews**: **Both Flight AND FIDO must re-review**, even if only one reviewer found issues. This ensures convention fixes don't introduce new problems. 3. **Loop until both APPROVE**: Repeat until both reviewers vote APPROVE. 4. **Bleed check**: After both APPROVE, proceed to Step 5 (bleed check). @@ -185,6 +234,23 @@ If PAO is the author, Flight and FIDO should watch for: This is not a critique β€” it is a known pattern. Treat it as a checklist, not a personal issue. +### Migration PR Protocol (>20 Files) + +PRs touching more than 20 files (bulk renames, API migrations, provider abstractions) follow a stricter protocol. These cannot be reviewed like a 5-file feature PR. + +#### Author obligations +1. **Mandatory local test attestation** β€” Run `npm run build && npm test` and include the full test output summary in the PR description (suite count, pass/fail, runtime). +2. **Structured change description** β€” Break the PR description into: (a) what changed mechanically, (b) what changed semantically, (c) what callsites were updated. +3. **Additive commits during review** β€” Never squash during active review on a migration PR. Reviewers need to see exactly which files changed between iterations. + +#### Reviewer obligations +1. **Mechanical correctness focus** β€” Are all callsites updated? Are string literals consistent across the migration? Are sync-to-async signature changes properly awaited everywhere? +2. **Sampling strategy** β€” Don't read all 80 files line-by-line. Sample 5-10 representative files, then verify the mechanical pattern holds across the rest. +3. **Test verification** β€” Before approving, confirm the author's test attestation. If the attestation is missing, request it before reviewing anything else. + +#### Squash timing +Squash to a single commit **only at merge time** (Step 6: CLEAN), after CI is green and both reviewers approve. During review, the full commit history must be preserved. + ### Review Comments Posted on PR Formal reviews are posted to the fork PR as: ``` @@ -207,15 +273,35 @@ Only assign Tamir as a reviewer on **upstream PRs** where his original code PR i ### Commit Hygiene To ensure clean histories: -- **One commit per fork PR**: Squash all review iterations into a single logical commit before opening upstream PR. -- **Amend, don't create new commits**: Use `git commit --amend --no-edit` to fix blockers without adding commit count. -- **Force-push safely**: Use `--force-with-lease` to prevent accidental overwrites if teammates push simultaneously (rare in single-agent fork setup, but good practice). +- **Additive commits during review**: Use descriptive commit messages for each fix iteration so reviewers can track what changed. Do NOT squash-amend-force-push during active review β€” this hides the fix history and makes re-review harder. +- **Squash at merge time only**: After CI is green and both reviewers approve, squash all review iterations into a single logical commit before opening the upstream PR (Step 6: CLEAN). +- **One commit per upstream PR**: The upstream PR should contain 1-3 logical commits, not N from iteration. +- **Force-push safely**: Use `--force-with-lease` to prevent accidental overwrites if teammates push simultaneously (rare in single-agent fork setup, but good practice). Only force-push after review is complete and you're preparing for upstream. - **Always verify before push**: ``` git diff --cached --stat # File count must match intent git diff --cached --diff-filter=D # Should show zero unintended deletions ``` +## Lessons Learned β€” PR #640 Retrospective + +These anti-patterns were observed during the StorageProvider abstraction PR (#640) and directly informed the Preflight Test Gate, Correctness Before Completeness, and Migration PR Protocol additions to this skill. + +### "Approved but broken" +Flight and FIDO both approved the PR β€” twice β€” while CI was failing. Reviews checked ecosystem completeness (docs present? changelog updated? exports wired?) but nobody ran `npm test` locally or verified CI status before approving. **Root cause**: the review checklist evaluated packaging, not correctness. + +### Template filename drift during bulk migration +An 89-file migration renamed storage provider references across the codebase. Some template filenames were updated inconsistently β€” the provider class was renamed but string literals referencing template paths were not. This is a mechanical correctness issue that sampling-based review should catch. + +### Sync-to-async signature changes breaking tests +The migration changed synchronous method signatures to async, but not all test callsites added `await`. Tests that called async methods without awaiting them passed silently (the promise was truthy) until the test runner caught unhandled rejections. **Lesson**: when a migration changes function signatures, reviewers must verify all callsites match the new signature. + +### The 89-second test run +The full test suite runs in ~89 seconds. Running it once locally before requesting review would have caught all three CI failures and prevented 3 consecutive CI round-trips (each taking 5-10 minutes). The preflight test gate exists because this single check has the highest ROI of any quality step in the pipeline. + +### Squash-amend-force-push hiding iteration +During review, the author used `git commit --amend --no-edit && git push --force-with-lease` to address feedback. This replaced the previous commit entirely, making it impossible for reviewers to see what changed between review rounds. The fix: use additive commits during review, squash only at merge time. + ## Workflow Summary This pipeline separates concerns: From ac1576b7dd15961624e97494c433dd998a725ec8 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 13:59:36 -0700 Subject: [PATCH 08/16] skill: add branch segregation rule to fork-first-pipeline Prevents .squad/ stowaways on feature branches. Learned from 3 incidents on PR #640. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/skills/fork-first-pipeline/SKILL.md | 39 ++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/.squad/skills/fork-first-pipeline/SKILL.md b/.squad/skills/fork-first-pipeline/SKILL.md index 7e65c6a0c..bfe5becfe 100644 --- a/.squad/skills/fork-first-pipeline/SKILL.md +++ b/.squad/skills/fork-first-pipeline/SKILL.md @@ -308,6 +308,41 @@ This pipeline separates concerns: - **Fork PR**: Messy iteration, team feedback, bleed capture - **Upstream PR**: Clean, single-commit, ready-to-merge -Result: Upstream PRs are lean, reviewed, and production-ready. - +Result: Upstream PRs are lean, reviewed, and production-ready. + +## Branch Segregation Rule + +**Confidence:** high (validated by 3 stowaway incidents on PR #640) + +### What goes where + +| Content | Target Branch | +|---------|--------------| +| Code changes for the feature/issue | Feature branch | +| .squad/ state (decisions, logs, history) | dev | +| Skill updates (.squad/skills/) | dev | +| Fork-sync results | dev | +| Anything not part of a specific PR's scope | dev | + +### Mechanic (when on a feature branch) + +If you need to commit infrastructure work while on a feature branch: + +1. `git stash` -- save feature work +2. `git checkout dev && git pull` +3. Commit and push the infrastructure change to dev +4. `git checkout -` -- return to feature branch +5. `git stash pop` -- restore feature work + +### Coordinator responsibility + +When dispatching agents for non-feature work while on a feature branch, include in the spawn prompt: + +> "You are on a feature branch. Infrastructure work (.squad/, skills, decisions) must go to dev -- switch before committing." + +### Anti-pattern + +NEVER commit .squad/, skills, decisions, or fork-sync to a feature branch. This creates "stowaways" that contaminate PRs and require manual cleanup during squash. + +**Evidence:** PR #640 had 3 stowaway incidents in one session -- fork-sync skill, fork-first-pipeline update, and .squad/ state all landed on the StorageProvider PR branch and had to be stripped. From 8097c9609ea8b667a93efbf06d3604ff7f43ad88 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 14:16:37 -0700 Subject: [PATCH 09/16] feat: add PR requirements spec and PR template (#106 Phase 2) - .github/PR_REQUIREMENTS.md: versioned canonical spec (from #106 PRD) - .github/PULL_REQUEST_TEMPLATE.md: author-facing checklist Addresses Challenger FATAL finding: spec must be versioned, not mutable issue. Part of #106. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/PR_REQUIREMENTS.md | 213 +++++++++++++++++++++++++++++++ .github/PULL_REQUEST_TEMPLATE.md | 30 +++++ 2 files changed, 243 insertions(+) create mode 100644 .github/PR_REQUIREMENTS.md create mode 100644 .github/PULL_REQUEST_TEMPLATE.md diff --git a/.github/PR_REQUIREMENTS.md b/.github/PR_REQUIREMENTS.md new file mode 100644 index 000000000..ab2850003 --- /dev/null +++ b/.github/PR_REQUIREMENTS.md @@ -0,0 +1,213 @@ +# PR Requirements β€” Squad Repository + +> **This file is the canonical source of truth for PR requirements.** +> Issue #106 tracks the PRD; this file is the versioned spec. +> Changes to this file must go through PR review (audit trail). + +--- + +## 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 requirement categories below apply conditionally to user-facing changes. Knowing what counts as user-facing prevents disputes about "does this need a CHANGELOG entry?" + +--- + +## PR Requirements by Category + +Requirements are ordered by review flow. 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: `npm run build` completes successfully +- [ ] All existing tests pass: `npm test` or `npm 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: `eslint` passes +- [ ] No type errors: `tsc --noEmit` passes +- [ ] Tests written for all new functionality +- [ ] All tests pass: `vitest` exits 0 + +**Enforced by**: eslint 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 types (`.d.ts`) and import (`.js`) entries required per export + - Example: the `/storage` export was missing from #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: `npm 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 + +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: (d) 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 | + +### 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. | + +For migration PRs (> 20 files): include test output summary in the PR description. + +--- + +## Known Limitations + +### 1. Enforceability Gap + +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. + +**Mitigation**: Phase 4 (Flight/FIDO checklist) codifies consistent judgment. Regular enforcement audits catch drift. + +### 2. Theater Risk + +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. + +### 3. Silent Drift Risk + +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. + +### 4. Ambiguities + +- **"Module" definition**: new directory under `packages/squad-sdk/src/` or new export namespace. Refactoring existing module does not trigger export requirement. +- **"User-facing" detection for CHANGELOG**: Currently manual. #104 will implement export-diff detection. + +--- + +## Relationship to Other Issues + +| Issue | Role | +|-------|------| +| #100 (Review completeness) | Defines the review *process* β€” this file defines *what reviewers check against* | +| #103 (Samples CI coverage) | Implements the "samples" requirement category (f) | +| #104 (PR completeness gates) | Implements automated enforcement of REQUIRED items as CI gates | + +This file is the source of truth. #100 and #104 derive their checklists from it. diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 000000000..9cb9aa6f5 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,30 @@ +### What + + +### Why + + +### How + + +### Testing + +- [ ] `npm run build` passes +- [ ] `npm test` passes (all tests green) +- [ ] For migration PRs (>20 files): test output summary included below + +### Docs + +- [ ] CHANGELOG.md entry (if packages/squad-sdk/src/ or packages/squad-cli/src/ changed) +- [ ] README section updated (if new feature/module) +- [ ] Docs feature page (if new user-facing capability) + +### Exports + +- [ ] package.json subpath exports updated (if new module) + +### Breaking Changes + + +### Waivers + From accd5c2f508dd0627c3a350c552be2ff43ca5def Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 14:45:42 -0700 Subject: [PATCH 10/16] Revert "feat: add PR requirements spec and PR template (#106 Phase 2)" This reverts commit 8097c9609ea8b667a93efbf06d3604ff7f43ad88. --- .github/PR_REQUIREMENTS.md | 213 ------------------------------- .github/PULL_REQUEST_TEMPLATE.md | 30 ----- 2 files changed, 243 deletions(-) delete mode 100644 .github/PR_REQUIREMENTS.md delete mode 100644 .github/PULL_REQUEST_TEMPLATE.md diff --git a/.github/PR_REQUIREMENTS.md b/.github/PR_REQUIREMENTS.md deleted file mode 100644 index ab2850003..000000000 --- a/.github/PR_REQUIREMENTS.md +++ /dev/null @@ -1,213 +0,0 @@ -# PR Requirements β€” Squad Repository - -> **This file is the canonical source of truth for PR requirements.** -> Issue #106 tracks the PRD; this file is the versioned spec. -> Changes to this file must go through PR review (audit trail). - ---- - -## 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 requirement categories below apply conditionally to user-facing changes. Knowing what counts as user-facing prevents disputes about "does this need a CHANGELOG entry?" - ---- - -## PR Requirements by Category - -Requirements are ordered by review flow. 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: `npm run build` completes successfully -- [ ] All existing tests pass: `npm test` or `npm 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: `eslint` passes -- [ ] No type errors: `tsc --noEmit` passes -- [ ] Tests written for all new functionality -- [ ] All tests pass: `vitest` exits 0 - -**Enforced by**: eslint 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 types (`.d.ts`) and import (`.js`) entries required per export - - Example: the `/storage` export was missing from #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: `npm 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 - -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: (d) 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 | - -### 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. | - -For migration PRs (> 20 files): include test output summary in the PR description. - ---- - -## Known Limitations - -### 1. Enforceability Gap - -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. - -**Mitigation**: Phase 4 (Flight/FIDO checklist) codifies consistent judgment. Regular enforcement audits catch drift. - -### 2. Theater Risk - -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. - -### 3. Silent Drift Risk - -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. - -### 4. Ambiguities - -- **"Module" definition**: new directory under `packages/squad-sdk/src/` or new export namespace. Refactoring existing module does not trigger export requirement. -- **"User-facing" detection for CHANGELOG**: Currently manual. #104 will implement export-diff detection. - ---- - -## Relationship to Other Issues - -| Issue | Role | -|-------|------| -| #100 (Review completeness) | Defines the review *process* β€” this file defines *what reviewers check against* | -| #103 (Samples CI coverage) | Implements the "samples" requirement category (f) | -| #104 (PR completeness gates) | Implements automated enforcement of REQUIRED items as CI gates | - -This file is the source of truth. #100 and #104 derive their checklists from it. diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md deleted file mode 100644 index 9cb9aa6f5..000000000 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ /dev/null @@ -1,30 +0,0 @@ -### What - - -### Why - - -### How - - -### Testing - -- [ ] `npm run build` passes -- [ ] `npm test` passes (all tests green) -- [ ] For migration PRs (>20 files): test output summary included below - -### Docs - -- [ ] CHANGELOG.md entry (if packages/squad-sdk/src/ or packages/squad-cli/src/ changed) -- [ ] README section updated (if new feature/module) -- [ ] Docs feature page (if new user-facing capability) - -### Exports - -- [ ] package.json subpath exports updated (if new module) - -### Breaking Changes - - -### Waivers - From 736e81be616b29b3044887e6f2655c05bf6719be Mon Sep 17 00:00:00 2001 From: "Dina Berry (MSFT)" Date: Sat, 28 Mar 2026 15:21:56 -0700 Subject: [PATCH 11/16] feat: add PR requirements spec and PR template (#106 Phase 2) Adds .github/PR_REQUIREMENTS.md (versioned spec with 6 categories, CRUD-on-CLI/SDK user-facing definition, waiver process, exemptions) and .github/PULL_REQUEST_TEMPLATE.md (author-facing checklist). Part 1 of 2 for repo health -- #104 will automate enforcement. Closes Phase 2 of #106 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/PR_REQUIREMENTS.md | 242 +++++++++++++++++++++++++++++++ .github/PULL_REQUEST_TEMPLATE.md | 30 ++++ 2 files changed, 272 insertions(+) create mode 100644 .github/PR_REQUIREMENTS.md create mode 100644 .github/PULL_REQUEST_TEMPLATE.md diff --git a/.github/PR_REQUIREMENTS.md b/.github/PR_REQUIREMENTS.md new file mode 100644 index 000000000..cf9584424 --- /dev/null +++ b/.github/PR_REQUIREMENTS.md @@ -0,0 +1,242 @@ +# PR Requirements β€” Squad Repository + +> **This file is the canonical source of truth for PR requirements.** +> Issue #106 tracks the PRD; this file is the versioned spec. +> Changes to this file must go through PR review (audit trail). + +--- + +## Definition of "User-Facing Change" + +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. + +"User-facing" is anchored to two concrete package boundaries: +- **SDK layer**: `packages/squad-sdk/src/` exports exposed in `package.json` subpath exports +- **CLI layer**: `packages/squad-cli/src/cli/` commands and subcommands + +### CRUD Operations β€” What Counts as User-Facing + +| Operation | SDK Layer | CLI Layer | +|-----------|-----------|-----------| +| **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/` | +| **Read** | N/A β€” reading doesn't change the surface | N/A β€” reading doesn't change the surface | +| **Update** | Change to existing public API signature (parameters, return types, behavior) | Change to CLI command flags, behavior, or subcommand structure | +| **Delete** | Remove a previously public export from `package.json` | Remove a CLI command or subcommand | + +### What Is NOT User-Facing + +- `.squad/` state files (decisions, history, skills, templates) +- Internal refactors that don't change the public API surface +- Infrastructure (CI, GitHub Actions, workflows) +- Documentation-only changes +- Dependency updates that don't change API surface + +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?" + +### Examples of "new module" + +- Adding a new directory under packages/squad-sdk/src/ (e.g., src/storage/, src/casting/) +- Adding a new subpath export to package.json (e.g., ./storage, ./casting) +- Adding a new CLI command file under packages/squad-cli/src/cli/commands/ + +### NOT a new module + +- Adding a new function to an existing module +- Refactoring internals without changing the public API surface +- Adding test files + +--- + +## PR Requirements by Category + +Requirements are ordered by review flow. 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: `npm run build` completes successfully +- [ ] All existing tests pass: `npm test` or `npm 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: `eslint` passes +- [ ] No type errors: `tsc --noEmit` passes +- [ ] Tests written for all new functionality +- [ ] All tests pass: `vitest` exits 0 + +**Enforced by**: eslint 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 types (`.d.ts`) and import (`.js`) entries required per export + - Example: the `/storage` export was missing from #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: `npm 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 + +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: (d) 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 | + +### 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. | + +For migration PRs (> 20 files): include test output summary in the PR description. + +--- + +## Known Limitations + +### 1. Enforceability Gap + +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. + +**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. + +**Mitigation**: Phase 4 (Flight/FIDO checklist) codifies consistent judgment. Regular enforcement audits catch drift. + +### 2. Theater Risk + +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. + +### 3. Silent Drift Risk + +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. + +### 4. Ambiguities + +- **"Module" definition**: new directory under `packages/squad-sdk/src/` or new export namespace. Refactoring existing module does not trigger export requirement. +- **"User-facing" detection for CHANGELOG**: Currently manual. #104 will implement export-diff detection. + +--- + +## Relationship to Other Issues + +| Issue | Role | +|-------|------| +| #100 (Review completeness) | Defines the review *process* β€” this file defines *what reviewers check against* | +| #103 (Samples CI coverage) | Implements the "samples" requirement category (f) | +| #104 (PR completeness gates) | Implements automated enforcement of REQUIRED items as CI gates | + +This file is the source of truth. #100 and #104 derive their checklists from it. diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 000000000..d6203f561 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,30 @@ +### What + + +### Why + + +### How + + +### Testing + +- [ ] `npm run build` passes +- [ ] `npm test` passes (all tests green) +- [ ] For migration PRs (>20 files): test output summary included below + +### Docs + +- [ ] CHANGELOG.md entry (if packages/squad-sdk/src/ or packages/squad-cli/src/ changed) +- [ ] README section updated (if new feature/module) +- [ ] Docs feature page (if new user-facing capability) + +### Exports + +- [ ] package.json subpath exports updated (if new module) + +### Breaking Changes + + +### Waivers + From 9156f7f52d5d7f88fc368ebec61264692a8e3c03 Mon Sep 17 00:00:00 2001 From: "Dina Berry (MSFT)" Date: Sat, 28 Mar 2026 16:38:45 -0700 Subject: [PATCH 12/16] feat(ci): add CHANGELOG and exports map completeness gates (#104) Part 2 of 2 for repo health. Adds two automated CI enforcement gates to squad-ci.yml: 1. CHANGELOG gate -- requires CHANGELOG.md update when SDK/CLI source changes 2. Exports map check -- verifies package.json exports match barrel files Both feature-flagged (vars.SQUAD_CHANGELOG_CHECK, vars.SQUAD_EXPORTS_CHECK) with skip labels. Includes test coverage for check-exports-map.mjs. Refs #104 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/squad-ci.yml | 132 +++++++++++++++++++++++++++++++++ scripts/check-exports-map.mjs | 45 +++++++++++ test/check-exports-map.test.ts | 53 +++++++++++++ 3 files changed, 230 insertions(+) create mode 100644 scripts/check-exports-map.mjs create mode 100644 test/check-exports-map.test.ts diff --git a/.github/workflows/squad-ci.yml b/.github/workflows/squad-ci.yml index 8cd153634..9ae8a2f9f 100644 --- a/.github/workflows/squad-ci.yml +++ b/.github/workflows/squad-ci.yml @@ -109,6 +109,138 @@ jobs: - name: Run tests run: npm test + changelog-gate: + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Check feature flag + id: flag + # Default: gate is ENABLED. When vars.SQUAD_CHANGELOG_CHECK is + # undefined (not set in repo/org variables), the bash comparison + # [ "" = "false" ] evaluates to false, so skip stays "false" and + # the gate runs. Set vars.SQUAD_CHANGELOG_CHECK to "false" to + # explicitly disable. + run: | + if [ "${{ vars.SQUAD_CHANGELOG_CHECK }}" = "false" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "CHANGELOG gate disabled via vars.SQUAD_CHANGELOG_CHECK" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: Check skip label + if: steps.flag.outputs.skip == 'false' + id: label + run: | + LABELS=$(gh pr view ${{ github.event.pull_request.number }} --json labels --jq '.labels[].name' 2>/dev/null || echo "") + if echo "$LABELS" | grep -q "skip-changelog"; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "Skipping CHANGELOG gate (skip-changelog label present)" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + env: + GH_TOKEN: ${{ github.token }} + + - name: Require CHANGELOG update for SDK/CLI source changes + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' + run: | + BASE="${{ github.event.pull_request.base.sha }}" + HEAD="${{ github.event.pull_request.head.sha }}" + # Three-dot diff (base...head) finds the merge-base automatically, + # so it works correctly even when the PR branch contains merge + # commits from syncing with the base branch. It compares against + # the common ancestor, not the literal base SHA. + CHANGED=$(git diff --name-only "$BASE"..."$HEAD") + + SDK_CLI_CHANGED=$(echo "$CHANGED" | grep -E '^packages/squad-(sdk|cli)/src/' || true) + if [ -z "$SDK_CLI_CHANGED" ]; then + echo "No SDK/CLI source changes detected -- CHANGELOG gate not applicable" + exit 0 + fi + + echo "SDK/CLI source files changed:" + echo "$SDK_CLI_CHANGED" + + CHANGELOG_CHANGED=$(echo "$CHANGED" | grep -E '^CHANGELOG\.md$' || true) + if [ -z "$CHANGELOG_CHANGED" ]; then + echo "" + echo "::error::CHANGELOG.md was not updated but SDK/CLI source files were changed." + echo "::error::Please add a CHANGELOG.md entry describing your changes." + echo "::error::If this is intentional, add the 'skip-changelog' label to your PR." + exit 1 + fi + + echo "CHANGELOG.md updated -- gate passed" + + exports-map-check: + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - uses: actions/setup-node@v4 + with: + node-version: 22 + + - name: Check feature flag + id: flag + # Default: gate is ENABLED. When vars.SQUAD_EXPORTS_CHECK is + # undefined (not set in repo/org variables), the bash comparison + # [ "" = "false" ] evaluates to false, so skip stays "false" and + # the gate runs. Set vars.SQUAD_EXPORTS_CHECK to "false" to + # explicitly disable. + run: | + if [ "${{ vars.SQUAD_EXPORTS_CHECK }}" = "false" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "Exports map check disabled via vars.SQUAD_EXPORTS_CHECK" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: Check skip label + if: steps.flag.outputs.skip == 'false' + id: label + run: | + LABELS=$(gh pr view ${{ github.event.pull_request.number }} --json labels --jq '.labels[].name' 2>/dev/null || echo "") + if echo "$LABELS" | grep -q "skip-exports-check"; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "Skipping exports map check (skip-exports-check label present)" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + env: + GH_TOKEN: ${{ github.token }} + + - name: Check for SDK source changes + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' + id: changes + run: | + BASE="${{ github.event.pull_request.base.sha }}" + HEAD="${{ github.event.pull_request.head.sha }}" + # Three-dot diff (base...head) finds the merge-base automatically, + # so it works correctly even when the PR branch contains merge + # commits from syncing with the base branch. + SDK_CHANGED=$(git diff --name-only "$BASE"..."$HEAD" | grep -E '^packages/squad-sdk/src/' || true) + if [ -z "$SDK_CHANGED" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "No SDK source changes detected -- exports check not applicable" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + echo "SDK source files changed:" + echo "$SDK_CHANGED" + fi + + - name: Verify exports map matches barrel files + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' && steps.changes.outputs.skip != 'true' + run: node scripts/check-exports-map.mjs + publish-policy: runs-on: ubuntu-latest steps: diff --git a/scripts/check-exports-map.mjs b/scripts/check-exports-map.mjs new file mode 100644 index 000000000..0ce25470d --- /dev/null +++ b/scripts/check-exports-map.mjs @@ -0,0 +1,45 @@ +#!/usr/bin/env node +// check-exports-map.mjs -- Verify package.json exports match barrel files. +// Exit 0 if all barrels are mapped, exit 1 with details if any are missing. +// Uses only Node.js built-ins (fs, path). + +import { readFileSync, readdirSync, existsSync } from 'node:fs'; +import { resolve, join, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const SDK_ROOT = resolve(__dirname, '..', 'packages', 'squad-sdk'); +const SRC_DIR = join(SDK_ROOT, 'src'); +const PKG_PATH = join(SDK_ROOT, 'package.json'); + +const pkg = JSON.parse(readFileSync(PKG_PATH, 'utf8')); +const exportsMap = pkg.exports || {}; + +const srcEntries = readdirSync(SRC_DIR, { withFileTypes: true }); +const barrelDirs = srcEntries + .filter((entry) => entry.isDirectory()) + .filter((entry) => existsSync(join(SRC_DIR, entry.name, 'index.ts'))) + .map((entry) => entry.name); + +const missing = []; + +for (const dir of barrelDirs) { + const exportKey = `./${dir}`; + if (!exportsMap[exportKey]) { + missing.push({ dir, expectedKey: exportKey }); + } +} + +if (missing.length === 0) { + console.log(`Exports map check passed: all ${barrelDirs.length} barrel directories have export entries.`); + process.exit(0); +} else { + console.error(`Exports map check FAILED: ${missing.length} barrel(s) missing from package.json exports.`); + console.error(`This is by design -- new barrel directories must have matching export entries.\n`); + for (const { dir, expectedKey } of missing) { + console.error(` MISSING: "${expectedKey}" (has src/${dir}/index.ts but no export entry)`); + } + console.error(`\nTo fix: add export entries to packages/squad-sdk/package.json "exports" for each missing barrel.`); + console.error('To skip: add the "skip-exports-check" label to your PR to bypass this gate.'); + process.exit(1); +} \ No newline at end of file diff --git a/test/check-exports-map.test.ts b/test/check-exports-map.test.ts new file mode 100644 index 000000000..94cb71242 --- /dev/null +++ b/test/check-exports-map.test.ts @@ -0,0 +1,53 @@ +/** + * check-exports-map.mjs β€” Script execution test (#104) + * + * Validates that the exports map checker script: + * 1. Executes without crashing (exits 0 or 1, not a runtime error) + * 2. Produces structured output on stdout or stderr + * + * This does NOT test that exports are complete β€” the script itself + * catches real gaps (e.g., platform, remote, roles, streams, upstream). + * Those missing exports are expected; they are tracked separately. + */ + +import { describe, it, expect } from 'vitest'; +import { execFile } from 'node:child_process'; +import { resolve } from 'node:path'; + +const SCRIPT_PATH = resolve(process.cwd(), 'scripts', 'check-exports-map.mjs'); + +function runScript(): Promise<{ code: number | null; stdout: string; stderr: string }> { + return new Promise((res) => { + execFile('node', [SCRIPT_PATH], { cwd: process.cwd() }, (error, stdout, stderr) => { + const code = error ? error.code ?? (error as NodeJS.ErrnoException & { status?: number }).status ?? 1 : 0; + res({ code: typeof code === 'number' ? code : 1, stdout, stderr }); + }); + }); +} + +describe('check-exports-map.mjs', () => { + it('executes without crashing (exits 0 or 1)', async () => { + const { code } = await runScript(); + // Exit 0 = all barrels mapped, exit 1 = some missing. + // Both are valid outcomes. A crash would be a non-0/1 code or thrown error. + expect([0, 1]).toContain(code); + }); + + it('produces output describing the check result', async () => { + const { stdout, stderr } = await runScript(); + const combined = stdout + stderr; + // The script always prints either "passed" or "FAILED" in its output + expect(combined).toMatch(/Exports map check (passed|FAILED)/); + }); + + it('reports MISSING entries with expected format when barrels are unmapped', async () => { + const { code, stderr } = await runScript(); + if (code === 1) { + // When the check fails, each missing barrel is reported with a MISSING: prefix + expect(stderr).toContain('MISSING:'); + // The error message should mention the skip label escape hatch + expect(stderr).toContain('skip-exports-check'); + } + // If code === 0, all barrels are mapped and there is nothing to assert here + }); +}); From d81b338098f88b93cc0cde174a4529083150a221 Mon Sep 17 00:00:00 2001 From: "Dina Berry (MSFT)" Date: Sat, 28 Mar 2026 18:49:50 -0700 Subject: [PATCH 13/16] feat(ci): add samples build validation gate (#111) Closes #103 Adds samples-build CI job that validates all 11 samples compile when SDK changes. - Loops samples/ directories with npm install/build/test - Feature-flagged via skip-samples-build label - 15-minute timeout, --ignore-scripts, npm cache - Accepted Copilot suggestions: cache-dependency-path, workspace root install Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/squad-ci.yml | 133 +++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/.github/workflows/squad-ci.yml b/.github/workflows/squad-ci.yml index 9ae8a2f9f..f486be621 100644 --- a/.github/workflows/squad-ci.yml +++ b/.github/workflows/squad-ci.yml @@ -241,6 +241,139 @@ jobs: if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' && steps.changes.outputs.skip != 'true' run: node scripts/check-exports-map.mjs + samples-build: + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - uses: actions/setup-node@v4 + with: + node-version: 22 + cache: 'npm' + cache-dependency-path: | + package-lock.json + samples/**/package-lock.json + cache-dependency-path: | + package-lock.json + samples/**/package-lock.json + + - name: Check feature flag + id: flag + # Default: gate is ENABLED. Set vars.SQUAD_SAMPLES_CI to "false" + # to explicitly disable. + run: | + if [ "${{ vars.SQUAD_SAMPLES_CI }}" = "false" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "Samples build gate disabled via vars.SQUAD_SAMPLES_CI" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: Check skip label + if: steps.flag.outputs.skip == 'false' + id: label + run: | + SKIP="false" + LABELS='${{ toJSON(github.event.pull_request.labels.*.name) }}' + if echo "$LABELS" | grep -q "skip-samples-ci"; then + SKIP="true" + echo "Skipping samples build gate (skip-samples-ci label present)" + fi + echo "skip=$SKIP" >> "$GITHUB_OUTPUT" + + - name: Check for SDK source changes + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' + id: changes + run: | + BASE="${{ github.event.pull_request.base.sha }}" + HEAD="${{ github.event.pull_request.head.sha }}" + SDK_CHANGED=$(git diff --name-only "$BASE"..."$HEAD" | grep -E '^packages/squad-sdk/src/' || true) + if [ -z "$SDK_CHANGED" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "No SDK source changes detected -- samples build not applicable" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + echo "SDK source files changed:" + echo "$SDK_CHANGED" + fi + + - name: Install root dependencies and build SDK + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' && steps.changes.outputs.skip != 'true' + run: | + npm ci --ignore-scripts + npm run build -w packages/squad-sdk + + - name: Build and test samples + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' && steps.changes.outputs.skip != 'true' + run: | + FAILED=0 + PASSED=0 + SKIPPED=0 + + for sample_dir in samples/*/; do + sample=$(basename "$sample_dir") + + if [ ! -f "$sample_dir/package.json" ]; then + echo "::notice::[$sample] No package.json -- skipping" + SKIPPED=$((SKIPPED + 1)) + continue + fi + + HAS_BUILD=$(node -e "const p=require('./$sample_dir/package.json'); process.exit(p.scripts?.build ? 0 : 1)" 2>/dev/null && echo "true" || echo "false") + HAS_TEST=$(node -e "const p=require('./$sample_dir/package.json'); process.exit(p.scripts?.test ? 0 : 1)" 2>/dev/null && echo "true" || echo "false") + + if [ "$HAS_BUILD" = "false" ] && [ "$HAS_TEST" = "false" ]; then + echo "::notice::[$sample] No build or test scripts -- skipping" + SKIPPED=$((SKIPPED + 1)) + continue + fi + + echo "" + echo "=========================================" + echo "[$sample] Installing dependencies..." + echo "=========================================" + if ! (cd "$sample_dir" && npm install --ignore-scripts 2>&1); then + echo "::error::[$sample] npm install failed" + FAILED=$((FAILED + 1)) + continue + fi + + if [ "$HAS_BUILD" = "true" ]; then + echo "[$sample] Running build..." + if ! (cd "$sample_dir" && npm run build 2>&1); then + echo "::error::[$sample] npm run build failed" + FAILED=$((FAILED + 1)) + continue + fi + fi + + if [ "$HAS_TEST" = "true" ]; then + echo "[$sample] Running tests..." + if ! (cd "$sample_dir" && npm test 2>&1); then + echo "::error::[$sample] npm test failed" + FAILED=$((FAILED + 1)) + continue + fi + fi + + echo "::notice::[$sample] Passed" + PASSED=$((PASSED + 1)) + done + + echo "" + echo "=========================================" + echo "Samples build summary: $PASSED passed, $FAILED failed, $SKIPPED skipped" + echo "=========================================" + + if [ "$FAILED" -gt 0 ]; then + echo "::error::$FAILED sample(s) failed build/test validation" + exit 1 + fi + publish-policy: runs-on: ubuntu-latest steps: From 53d2258a1e88833539156433bd6a2898e1f624e0 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sun, 29 Mar 2026 08:16:47 -0700 Subject: [PATCH 14/16] feat(ci): add workspace integrity, prerelease guard, and export smoke gates Adds 3 new CI validation gates motivated by the PR #640 prerelease version incident where npm silently resolved a stale registry SDK. - workspace-integrity: verifies lockfile has no stale registry entries for workspace packages (zero-install, reads lockfile only) - prerelease-version-guard: blocks prerelease version suffixes from merging to dev/main (zero-install, reads package.json only) - export-smoke-test: verifies all subpath exports resolve to built artifacts after SDK build (lightweight install+build) All gates follow existing patterns: feature flags (vars.SQUAD_*), skip labels, three-dot diff for change detection, ::error:: annotations. Closes diberry/squad#114 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/squad-ci.yml | 290 +++++++++++++++++++++++++++++++++ 1 file changed, 290 insertions(+) diff --git a/.github/workflows/squad-ci.yml b/.github/workflows/squad-ci.yml index f486be621..c58224c86 100644 --- a/.github/workflows/squad-ci.yml +++ b/.github/workflows/squad-ci.yml @@ -399,3 +399,293 @@ jobs: exit 1 fi echo "βœ… All npm publish commands are workspace-scoped" + + workspace-integrity: + # ────────────────────────────────────────────────────────────────────── + # Workspace Integrity Check + # Purpose: Verify workspace packages resolve to local file: links, + # not stale registry versions in the lockfile. + # Catches: npm silently resolving a published registry copy instead + # of the local workspace symlink due to version mismatches. + # Why: Added after PR #640 prerelease version incident where + # >=0.9.0 didn't match 0.9.1-build.4, so npm pulled the + # stale published SDK from the registry. + # Cost: Zero-install β€” reads package-lock.json only. + # ────────────────────────────────────────────────────────────────────── + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Check feature flag + id: flag + # Default: gate is ENABLED. When vars.SQUAD_WORKSPACE_CHECK is + # undefined (not set in repo/org variables), the bash comparison + # [ "" = "false" ] evaluates to false, so skip stays "false" and + # the gate runs. Set vars.SQUAD_WORKSPACE_CHECK to "false" to + # explicitly disable. + run: | + if [ "${{ vars.SQUAD_WORKSPACE_CHECK }}" = "false" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "Workspace integrity check disabled via vars.SQUAD_WORKSPACE_CHECK" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: Check skip label + if: steps.flag.outputs.skip == 'false' + id: label + run: | + LABELS='${{ toJSON(github.event.pull_request.labels.*.name) }}' + if echo "$LABELS" | grep -q "skip-workspace-check"; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "Skipping workspace integrity check (skip-workspace-check label present)" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: Verify workspace packages resolve locally + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' + run: | + node -e " + const fs = require('fs'); + const lock = JSON.parse(fs.readFileSync('package-lock.json', 'utf8')); + const pkgs = lock.packages || {}; + const problems = []; + + for (const [key, val] of Object.entries(pkgs)) { + if (!key.includes('node_modules/@bradygaster/squad-')) continue; + if (val.resolved && val.resolved.startsWith('https://')) { + problems.push({ path: key, resolved: val.resolved }); + } else if (val.version && !val.link) { + problems.push({ path: key, version: val.version, link: false }); + } + } + + if (problems.length > 0) { + console.error('::error::WORKSPACE INTEGRITY FAILURE β€” npm resolved registry packages instead of local workspace copies.'); + console.error('::error::This likely means a version mismatch between workspace packages (see PR #640).'); + console.error(''); + problems.forEach(p => { + console.error(' STALE: ' + p.path + (p.resolved ? ' β†’ ' + p.resolved : ' (version: ' + p.version + ', not a workspace link)')); + }); + console.error(''); + console.error('To fix: ensure all workspace package version ranges match local versions,'); + console.error('then run npm install at the repo root to regenerate the lockfile.'); + process.exit(1); + } + + console.log('βœ… All workspace packages resolve to local file: links'); + " + + prerelease-version-guard: + # ────────────────────────────────────────────────────────────────────── + # Prerelease Version Guard + # Purpose: Prevent prerelease version strings (-build, -alpha, -beta, + # -rc) from being committed to dev or main. + # Catches: Forgotten prerelease suffixes that break semver range + # resolution in workspace dependencies. + # Why: Added after PR #640 prerelease version incident where a + # -build.N suffix caused npm to skip the local workspace + # copy during dependency resolution. + # Cost: Zero-install β€” reads package.json files only. + # ────────────────────────────────────────────────────────────────────── + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Check feature flag + id: flag + # Default: gate is ENABLED. When vars.SQUAD_VERSION_CHECK is + # undefined (not set in repo/org variables), the bash comparison + # [ "" = "false" ] evaluates to false, so skip stays "false" and + # the gate runs. Set vars.SQUAD_VERSION_CHECK to "false" to + # explicitly disable. + run: | + if [ "${{ vars.SQUAD_VERSION_CHECK }}" = "false" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "Prerelease version guard disabled via vars.SQUAD_VERSION_CHECK" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: Check skip label + if: steps.flag.outputs.skip == 'false' + id: label + run: | + LABELS='${{ toJSON(github.event.pull_request.labels.*.name) }}' + if echo "$LABELS" | grep -q "skip-version-check"; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "Skipping prerelease version guard (skip-version-check label present)" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: Scan packages for prerelease versions + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' + run: | + node -e " + const fs = require('fs'); + const path = require('path'); + const pkgDirs = fs.readdirSync('packages', { withFileTypes: true }) + .filter(d => d.isDirectory()) + .map(d => d.name); + + const violations = []; + for (const dir of pkgDirs) { + const pkgPath = path.join('packages', dir, 'package.json'); + if (!fs.existsSync(pkgPath)) continue; + const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); + if (pkg.version && /-/.test(pkg.version)) { + violations.push({ name: pkg.name, version: pkg.version, path: pkgPath }); + } + } + + if (violations.length > 0) { + console.error('::error::PRERELEASE VERSION DETECTED β€” packages with prerelease versions cannot merge to dev/main.'); + console.error(''); + violations.forEach(v => { + console.error(' ' + v.name + '@' + v.version + ' (' + v.path + ')'); + }); + console.error(''); + console.error('Prerelease suffixes (-build, -alpha, -beta, -rc) must be removed before merging.'); + console.error('To fix: update the version field in each listed package.json to a release version.'); + console.error('To skip: add the \"skip-version-check\" label to your PR.'); + process.exit(1); + } + + console.log('βœ… All package versions are release versions (no prerelease suffixes)'); + pkgDirs.forEach(dir => { + const pkgPath = path.join('packages', dir, 'package.json'); + if (fs.existsSync(pkgPath)) { + const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); + if (pkg.version) console.log(' ' + pkg.name + '@' + pkg.version); + } + }); + " + + export-smoke-test: + # ────────────────────────────────────────────────────────────────────── + # Export Smoke Test + # Purpose: Verify that subpath exports actually resolve after build. + # The exports-map-check validates config (barrel files match + # export entries); this gate validates built artifacts exist + # and are importable. + # Catches: Missing dist/ files for declared subpath exports β€” e.g. a + # new export added to package.json but the build doesn't + # produce the referenced .js file. + # Why: Added after PR #640 prerelease version incident to + # strengthen build artifact validation. + # Cost: Requires install + SDK build (~30s). + # ────────────────────────────────────────────────────────────────────── + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - uses: actions/setup-node@v4 + with: + node-version: 22 + + - name: Check feature flag + id: flag + # Default: gate is ENABLED. When vars.SQUAD_EXPORT_SMOKE is + # undefined (not set in repo/org variables), the bash comparison + # [ "" = "false" ] evaluates to false, so skip stays "false" and + # the gate runs. Set vars.SQUAD_EXPORT_SMOKE to "false" to + # explicitly disable. + run: | + if [ "${{ vars.SQUAD_EXPORT_SMOKE }}" = "false" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "Export smoke test disabled via vars.SQUAD_EXPORT_SMOKE" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: Check skip label + if: steps.flag.outputs.skip == 'false' + id: label + run: | + LABELS='${{ toJSON(github.event.pull_request.labels.*.name) }}' + if echo "$LABELS" | grep -q "skip-export-smoke"; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "Skipping export smoke test (skip-export-smoke label present)" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: Check for SDK source changes + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' + id: changes + run: | + BASE="${{ github.event.pull_request.base.sha }}" + HEAD="${{ github.event.pull_request.head.sha }}" + # Three-dot diff (base...head) finds the merge-base automatically, + # so it works correctly even when the PR branch contains merge + # commits from syncing with the base branch. + SDK_CHANGED=$(git diff --name-only "$BASE"..."$HEAD" | grep -E '^packages/squad-sdk/(src/|package\.json)' || true) + if [ -z "$SDK_CHANGED" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "No SDK source/config changes detected -- export smoke test not applicable" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + echo "SDK files changed:" + echo "$SDK_CHANGED" + fi + + - name: Install and build SDK + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' && steps.changes.outputs.skip != 'true' + run: | + npm ci --ignore-scripts + npm run build -w packages/squad-sdk + + - name: Smoke test all subpath exports + if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' && steps.changes.outputs.skip != 'true' + run: | + node -e " + const fs = require('fs'); + const path = require('path'); + const pkg = JSON.parse(fs.readFileSync('packages/squad-sdk/package.json', 'utf8')); + const exportsMap = pkg.exports || {}; + const failures = []; + let passed = 0; + + for (const [subpath, targets] of Object.entries(exportsMap)) { + const importPath = subpath === '.' + ? '@bradygaster/squad-sdk' + : '@bradygaster/squad-sdk/' + subpath.slice(2); + const filePath = typeof targets === 'string' + ? targets + : (targets.import || targets.default); + if (!filePath) { + failures.push({ subpath, importPath, error: 'No import target defined' }); + continue; + } + const resolvedPath = path.resolve('packages/squad-sdk', filePath); + if (fs.existsSync(resolvedPath)) { + passed++; + console.log(' βœ… ' + importPath + ' β†’ ' + filePath); + } else { + failures.push({ subpath, importPath, filePath, error: 'File not found: ' + resolvedPath }); + } + } + + console.log(''); + if (failures.length > 0) { + console.error('::error::EXPORT SMOKE TEST FAILED β€” ' + failures.length + ' subpath export(s) do not resolve to built artifacts.'); + console.error(''); + failures.forEach(f => { + console.error(' ❌ ' + (f.importPath || f.subpath) + ': ' + f.error); + }); + console.error(''); + console.error('This means consumers importing these subpaths will get runtime errors.'); + console.error('To fix: ensure the build produces all files referenced in package.json exports.'); + console.error('To skip: add the \"skip-export-smoke\" label to your PR.'); + process.exit(1); + } + + console.log('βœ… All ' + passed + ' subpath exports resolve to built artifacts'); + " From c39f5837a26c04f275c80bdbb5af0536990cd7a0 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sun, 29 Mar 2026 08:43:40 -0700 Subject: [PATCH 15/16] fix(ci): address team review suggestions on health gates - Add Skip Labels Reference comment block listing all available skip labels (PAO, Flight) - Add local testing instructions to each health gate (Flight, FIDO) - Document change-detection regex patterns for future maintainers (FIDO) - Enhance export smoke test with dynamic import() validation (EECOM) - Add test PR creation hints to gate comments (FIDO) Closes #115 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/squad-ci.yml | 115 ++++++++++++++++++++++++++++++--- 1 file changed, 106 insertions(+), 9 deletions(-) diff --git a/.github/workflows/squad-ci.yml b/.github/workflows/squad-ci.yml index c58224c86..5daa850a8 100644 --- a/.github/workflows/squad-ci.yml +++ b/.github/workflows/squad-ci.yml @@ -109,7 +109,33 @@ jobs: - name: Run tests run: npm test + # ════════════════════════════════════════════════════════════════════════ + # Skip Labels Reference + # ──────────────────────────────────────────────────────────────────────── + # The following PR labels can be used to bypass specific health gates. + # Add them via the GitHub UI or `gh pr edit --add-label