diff --git a/.copilot/skills/bleed-check/SKILL.md b/.copilot/skills/bleed-check/SKILL.md new file mode 100644 index 000000000..f8f64e4fe --- /dev/null +++ b/.copilot/skills/bleed-check/SKILL.md @@ -0,0 +1,145 @@ +# 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. + +## 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) +- **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 | + +### 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 + +\\\ +| 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/.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 + diff --git a/.github/workflows/squad-ci.yml b/.github/workflows/squad-ci.yml index 8cd153634..f486be621 100644 --- a/.github/workflows/squad-ci.yml +++ b/.github/workflows/squad-ci.yml @@ -109,6 +109,271 @@ 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 + + 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: 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/flight-versioning-policy.md b/.squad/decisions/inbox/flight-versioning-policy.md new file mode 100644 index 000000000..9911fc318 --- /dev/null +++ b/.squad/decisions/inbox/flight-versioning-policy.md @@ -0,0 +1,32 @@ +# Decision: Versioning Policy — No Prerelease Versions on dev/main + +**By:** Flight (Lead) +**Date:** 2026-03-29 +**Requested by:** Dina +**Status:** DECIDED +**Confidence:** Medium (confirmed by PR #640 incident, PR #116 prerelease leak, CI gate implementation) + +## Decision + +1. **All packages use strict semver** (`MAJOR.MINOR.PATCH`). No prerelease suffixes on `dev` or `main`. +2. **Prerelease versions are ephemeral.** `bump-build.mjs` creates `-build.N` for local testing only — never committed. +3. **SDK and CLI versions must stay in sync.** Divergence silently breaks npm workspace resolution. +4. **Surgeon owns version bumps.** Other agents must not modify `version` fields in `package.json` unless fixing a prerelease leak. +5. **CI enforcement via `prerelease-version-guard`** blocks PRs with prerelease versions. `skip-version-check` label is Surgeon-only. + +## Why + +The repo had no documented versioning policy. This caused two incidents: + +- **PR #640:** Prerelease version `0.9.1-build.4` silently broke workspace resolution. The semver range `>=0.9.0` does not match prerelease versions, causing npm to install a stale registry package instead of the local workspace link. Four PRs (#637–#640) patched symptoms before the root cause was found. +- **PR #116:** Surgeon set versions to `0.9.1-build.1` instead of `0.9.1` on a release branch because there was no guidance on what constitutes a clean release version. + +## Skill Reference + +Full policy documented in `.squad/skills/versioning-policy/SKILL.md`. + +## Impact + +- All agents must follow the versioning policy when touching `package.json` +- Surgeon charter should reference this skill for release procedures +- CI pipeline enforces the policy via automated gate 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 new file mode 100644 index 000000000..bfe5becfe --- /dev/null +++ b/.squad/skills/fork-first-pipeline/SKILL.md @@ -0,0 +1,348 @@ +# 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. + +## 9-Step Pipeline + +\\\ +BRANCH → FORK PR → PREFLIGHT → 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 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. 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: +- \.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 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 + +#### 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` +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 +- 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 +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. + +## 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 | +| 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 | +| 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 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 + +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 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). + +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. + +### 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: +``` +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: + +- **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: +- **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. + +## 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. + diff --git a/.squad/skills/versioning-policy/SKILL.md b/.squad/skills/versioning-policy/SKILL.md new file mode 100644 index 000000000..997d4c4aa --- /dev/null +++ b/.squad/skills/versioning-policy/SKILL.md @@ -0,0 +1,119 @@ +--- +name: "versioning-policy" +description: "Semver versioning rules for Squad SDK and CLI — prevents prerelease version incidents" +domain: "release, versioning, npm, CI" +confidence: "medium" +source: "earned (PR #640 workspace resolution incident, PR #116 prerelease leak, CI gate implementation)" +--- + +## Context + +Squad is a monorepo with two publishable npm packages (`@bradygaster/squad-sdk` and `@bradygaster/squad-cli`) managed via npm workspaces. Version mismatches and prerelease leaks have caused production incidents — most notably PR #640, where a `-build.N` prerelease version silently broke workspace dependency resolution. + +This skill codifies the versioning rules every agent must follow. + +## 1. Version Format + +All packages use **strict semver**: `MAJOR.MINOR.PATCH` + +- ✅ `0.9.1`, `1.0.0`, `0.10.0` +- ❌ `0.9.1-build.4`, `0.9.1-preview.1`, `0.8.6.1-preview` + +No prerelease suffixes on `dev` or `main` branches — ever. + +## 2. Prerelease Versions Are Ephemeral + +The `scripts/bump-build.mjs` script creates `-build.N` versions (e.g., `0.9.1-build.4`) for **local development testing only**. + +Rules: +- `-build.N` versions are created automatically during local `npm run build` +- They are **never committed** to `dev` or `main` +- The script skips itself in CI (`CI=true` or `SKIP_BUILD_BUMP=1`) +- If you see a `-build.N` version in a PR diff, it is a bug — reject the PR + +## 3. SDK and CLI Version Sync + +Both `@bradygaster/squad-sdk` and `@bradygaster/squad-cli` **MUST have the same version** at all times. The root `package.json` version must also match. + +`bump-build.mjs` enforces this by updating all three `package.json` files in lockstep (root + `packages/squad-sdk` + `packages/squad-cli`). + +If versions diverge, workspace resolution silently breaks (see §4). + +## 4. npm Workspace Semver Footgun + +The CLI depends on the SDK via a workspace dependency with a semver range: + +```json +"@bradygaster/squad-sdk": ">=0.9.0" +``` + +**Critical:** Per the semver specification, `>=0.9.0` does **NOT** match `0.9.1-build.4`. + +Semver prerelease versions (anything with a `-` suffix) are only matched by ranges that explicitly reference the same `MAJOR.MINOR.PATCH` base with a prerelease comparator. A bare `>=0.9.0` range skips all prerelease versions. + +**What happens:** When the local SDK has version `0.9.1-build.4`, npm's workspace resolution fails to match the `>=0.9.0` range. npm then **silently installs a stale published version** from the npm registry instead of using the local workspace link. The build succeeds but runs against old SDK code. + +This is the root cause of the **PR #640 incident**, where workspace packages appeared linked but were actually running against stale registry versions. + +## 5. Who Bumps Versions + +**Surgeon (Release Manager) owns all version bumps.** + +| Agent | May modify `version` in package.json? | +|-------|---------------------------------------| +| Surgeon | ✅ Yes — sole owner of version bumps | +| Any other agent | ❌ No — unless explicitly fixing a prerelease leak | + +If you discover a prerelease version committed to `dev` or `main`, you may fix it (revert to the clean release version) without Surgeon's approval. This is a safety escape hatch, not a license to manage versions. + +## 6. Version Bump Lifecycle + +``` +┌─────────────────────────────────────────────────────────┐ +│ Development phase │ +│ Versions stay at current release: 0.9.1 │ +│ bump-build.mjs creates -build.N locally (not committed)│ +├─────────────────────────────────────────────────────────┤ +│ Pre-release testing │ +│ bump-build.mjs → 0.9.1-build.1, -build.2, ... │ +│ Local only. Never committed. Never pushed. │ +├─────────────────────────────────────────────────────────┤ +│ Release │ +│ Surgeon bumps to next version (e.g., 0.9.2 or 0.10.0) │ +│ Tags, publishes to npm registry │ +├─────────────────────────────────────────────────────────┤ +│ Post-release │ +│ Versions stay at the new release version (e.g., 0.9.2) │ +│ Development continues on clean version │ +└─────────────────────────────────────────────────────────┘ +``` + +## 7. CI Enforcement + +The **`prerelease-version-guard`** CI gate blocks any PR to `dev` or `main` that contains prerelease version strings in `package.json` files. + +- The gate scans all three `package.json` files for `-` in the version field +- PRs with prerelease versions **cannot merge** until the version is cleaned +- The `skip-version-check` label bypasses the gate — use **only** for the bump-build script's own PR (if applicable), and only with Surgeon's approval + +## 8. Incident Reference — PR #640 + +**PR #640** is the cautionary tale for this entire policy. + +**What happened:** Prerelease versions (`0.9.1-build.4`) were committed to a branch. The workspace dependency `>=0.9.0` failed to match the prerelease version per semver spec. npm silently installed a stale published SDK from the registry instead of linking the local workspace copy. Four PRs (#637–#640) attempted iterative patches before the root cause was identified. + +**Root cause:** No versioning policy existed. Agents didn't know that prerelease versions break workspace resolution, or that only Surgeon should modify versions. + +**Resolution:** This skill, the `prerelease-version-guard` CI gate, and the team decision to centralize version ownership under Surgeon. + +## Quick Reference + +| Rule | Summary | +|------|---------| +| Format | `MAJOR.MINOR.PATCH` — no prerelease on dev/main | +| Prerelease | `-build.N` is local-only, never committed | +| Sync | SDK + CLI + root must have identical versions | +| Ownership | Surgeon bumps versions; others don't touch them | +| CI gate | `prerelease-version-guard` blocks prerelease PRs | +| Escape hatch | Any agent may revert a prerelease leak to clean version | +| Footgun | `>=0.9.0` does NOT match `0.9.1-build.4` per semver | 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. --- 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 + }); +}); 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'); + }); +});