From ddc4f6a51d6bff123ce76f2b0ffc8127d5889a52 Mon Sep 17 00:00:00 2001 From: williamhallatt Date: Thu, 5 Mar 2026 19:12:25 +1000 Subject: [PATCH 1/7] fix(sdk): only install Squad-framework workflows during init Squad's own CI/CD workflows (squad-ci, squad-release, squad-docs, etc.) hardcode Squad's branch strategy and Node.js toolchain. They have no business in a user's repository. Add FRAMEWORK_WORKFLOWS constant and filter the workflow copy loop to only install the four workflows that power Squad features: - squad-heartbeat.yml - squad-issue-assign.yml - squad-triage.yml - sync-squad-labels.yml All other workflows in templates/workflows/ are Squad product CI/CD and are no longer installed into user repos. Closes bradygaster/squad#201 --- packages/squad-sdk/src/config/init.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/squad-sdk/src/config/init.ts b/packages/squad-sdk/src/config/init.ts index d5cc5531d..9cc3f7391 100644 --- a/packages/squad-sdk/src/config/init.ts +++ b/packages/squad-sdk/src/config/init.ts @@ -438,6 +438,18 @@ function stampVersionInContent(content: string, version: string): string { * @param options - Initialization options * @returns Result with created file paths */ + +/** + * Workflow files that are part of the Squad framework and should always be installed. + * Other workflows in templates/workflows/ are generic CI/CD scaffolding and are opt-in. + */ +const FRAMEWORK_WORKFLOWS = [ + 'squad-heartbeat.yml', + 'squad-issue-assign.yml', + 'squad-triage.yml', + 'sync-squad-labels.yml', +]; + export async function initSquad(options: InitOptions): Promise { const { teamRoot, @@ -802,7 +814,8 @@ ${projectDescription ? `- **Description:** ${projectDescription}\n` : ''}- **Cre const workflowsDest = join(teamRoot, '.github', 'workflows'); if (statSync(workflowsSrc).isDirectory()) { - const workflowFiles = readdirSync(workflowsSrc).filter(f => f.endsWith('.yml')); + const allWorkflowFiles = readdirSync(workflowsSrc).filter(f => f.endsWith('.yml')); + const workflowFiles = allWorkflowFiles.filter(f => FRAMEWORK_WORKFLOWS.includes(f)); await mkdir(workflowsDest, { recursive: true }); for (const file of workflowFiles) { From da84b90a288171508c53a81b24e0a624cd89dc11 Mon Sep 17 00:00:00 2001 From: williamhallatt Date: Thu, 5 Mar 2026 19:24:05 +1000 Subject: [PATCH 2/7] test(workflows): update expectations to match framework-only install --- test/workflows.test.js | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/test/workflows.test.js b/test/workflows.test.js index ba18c4f60..b37feff22 100644 --- a/test/workflows.test.js +++ b/test/workflows.test.js @@ -15,6 +15,14 @@ const CI_CD_WORKFLOWS = [ 'squad-release.yml', ]; +// Squad-framework workflows that ARE installed during init +const FRAMEWORK_WORKFLOWS = [ + 'squad-heartbeat.yml', + 'squad-triage.yml', + 'squad-issue-assign.yml', + 'sync-squad-labels.yml', +]; + function runSquad(args, cwd) { try { const result = execFileSync(process.execPath, [CLI, ...args], { @@ -111,8 +119,8 @@ describe('CI/CD workflow templates (squad-ci, squad-preview, squad-release)', () } }); - describe('fresh init copies CI/CD workflows to .github/workflows/', () => { - for (const file of CI_CD_WORKFLOWS) { + describe('fresh init copies framework workflows to .github/workflows/', () => { + for (const file of FRAMEWORK_WORKFLOWS) { it(`${file} is present after init`, (t) => { if (!fs.existsSync(path.join(TEMPLATES_DIR, file))) { t.skip(`${file} template not yet created`); @@ -125,6 +133,20 @@ describe('CI/CD workflow templates (squad-ci, squad-preview, squad-release)', () } }); + describe('fresh init does NOT copy CI/CD workflows to .github/workflows/', () => { + for (const file of CI_CD_WORKFLOWS) { + it(`${file} is absent after init`, (t) => { + if (!fs.existsSync(path.join(TEMPLATES_DIR, file))) { + t.skip(`${file} template not yet created`); + return; + } + initSquad(tmpDir); + const dest = path.join(tmpDir, '.github', 'workflows', file); + assert.ok(!fs.existsSync(dest), `${file} should NOT be installed by init`); + }); + } + }); + describe('upgrade copies/updates CI/CD workflows', () => { for (const file of CI_CD_WORKFLOWS) { it(`${file} is present after upgrade`, (t) => { @@ -173,7 +195,7 @@ describe('CI/CD workflow templates (squad-ci, squad-preview, squad-release)', () }); describe('workflow YAML validity after init', () => { - for (const file of CI_CD_WORKFLOWS) { + for (const file of FRAMEWORK_WORKFLOWS) { it(`${file} in .github/workflows/ is valid workflow YAML`, (t) => { if (!fs.existsSync(path.join(TEMPLATES_DIR, file))) { t.skip(`${file} template not yet created`); @@ -243,16 +265,18 @@ describe('CI/CD workflow templates (squad-ci, squad-preview, squad-release)', () }); }); - describe('all workflow templates have matching copies after init', () => { - it('every .yml in templates/workflows/ is copied to .github/workflows/', () => { - const allTemplates = getAllTemplateWorkflows(); - if (allTemplates.length === 0) { + describe('all framework workflow templates have matching copies after init', () => { + it('every framework workflow is copied to .github/workflows/', () => { + const presentTemplates = FRAMEWORK_WORKFLOWS.filter(f => + fs.existsSync(path.join(TEMPLATES_DIR, f)) + ); + if (presentTemplates.length === 0) { return; // nothing to test } initSquad(tmpDir); - for (const file of allTemplates) { + for (const file of presentTemplates) { const dest = path.join(tmpDir, '.github', 'workflows', file); assert.ok( fs.existsSync(dest), From 080d6b4eec50224e38b5477863b70ac550377fec Mon Sep 17 00:00:00 2001 From: williamhallatt Date: Thu, 5 Mar 2026 20:36:44 +1000 Subject: [PATCH 3/7] docs(ai-team): Merge PR #201 workflow filter reviews Session: 2026-03-05T10-35-50Z-validation-review Requested by: Scribe (session handler) Changes: - Orchestration logs: 4 agent reviews (Keaton, Fenster, Hockney, Edie) - Session log: Validation review summary, ready to merge - Decisions merged: Framework/scaffolding pattern, implementation, test coverage gaps, TypeScript validation - Cross-agent updates: Appended to Keaton, Fenster, Hockney, Edie history.md All agents APPROVED. Merge ready. --- .squad/.first-run | 1 + .squad/agents/edie/history.md | 13 + .squad/agents/fenster/history.md | 38 +++ .squad/agents/hockney/history.md | 49 ++++ .squad/agents/keaton/history.md | 28 ++ .squad/config.json | 4 + .squad/decisions.md | 267 ++++++++++++++++++ .../copilot-docs-migration-retrospective.md | 137 +++++++++ .../inbox/hockney-migrate-test-gaps.md | 164 +++++++++++ .../inbox/keaton-migration-arch-review.md | 111 ++++++++ .../waingro-destructive-qa-discipline.md | 108 +++++++ .../2026-03-05T10-35-50Z-validation-review.md | 40 +++ .../2026-03-05T10-35-50Z-edie.md | 44 +++ .../2026-03-05T10-35-50Z-fenster.md | 34 +++ .../2026-03-05T10-35-50Z-hockney.md | 44 +++ .../2026-03-05T10-35-50Z-keaton.md | 20 ++ 16 files changed, 1102 insertions(+) create mode 100644 .squad/.first-run create mode 100644 .squad/config.json create mode 100644 .squad/decisions/inbox/copilot-docs-migration-retrospective.md create mode 100644 .squad/decisions/inbox/hockney-migrate-test-gaps.md create mode 100644 .squad/decisions/inbox/keaton-migration-arch-review.md create mode 100644 .squad/decisions/inbox/waingro-destructive-qa-discipline.md create mode 100644 .squad/log/2026-03-05T10-35-50Z-validation-review.md create mode 100644 .squad/orchestration-log/2026-03-05T10-35-50Z-edie.md create mode 100644 .squad/orchestration-log/2026-03-05T10-35-50Z-fenster.md create mode 100644 .squad/orchestration-log/2026-03-05T10-35-50Z-hockney.md create mode 100644 .squad/orchestration-log/2026-03-05T10-35-50Z-keaton.md diff --git a/.squad/.first-run b/.squad/.first-run new file mode 100644 index 000000000..8dc2ae718 --- /dev/null +++ b/.squad/.first-run @@ -0,0 +1 @@ +2026-03-05T09:10:14.302Z diff --git a/.squad/agents/edie/history.md b/.squad/agents/edie/history.md index 8c7ae4719..0c4137820 100644 --- a/.squad/agents/edie/history.md +++ b/.squad/agents/edie/history.md @@ -175,3 +175,16 @@ All four agents shipped Phase 2 in parallel: Fortier wired TTFT/duration/through - No intermediate states recorded as final (except the above version ref). - All decisions match .squad/decisions.md consensus. - Confidence: High. History now reflects ground truth for future spawns. + +### Workflow filter type validation β€” #201 investigation +- Validated PR `williamhallatt/201-investigate-actions-install` for TypeScript correctness +- Change: `FRAMEWORK_WORKFLOWS` array filters workflows to only Squad framework files (4 entries) +- **Type inference:** `const FRAMEWORK_WORKFLOWS = [...]` correctly infers as `string[]`. `Array.prototype.includes(value: string)` accepts `string` from `readdirSync().filter()` with zero issues +- **Build:** `npm run build` passes cleanly with zero errors. All `.d.ts` files emit correctly +- **Lint:** `npm run lint` (noEmit check) passes cleanly +- **Strict mode compliance:** Root tsconfig has `strict: true` + `noUncheckedIndexedAccess: true`. The constant is module-scoped (not exported), correctly typed, and `.includes()` has no indexed access concern +- **ESM:** Package uses `"type": "module"`. Constant placement is correct for ESM β€” no side effects, no hoisting issues +- **Alternative considered:** `as const` would narrow to tuple literals `readonly ['squad-heartbeat.yml', ...]`, making `.includes()` require literal types (not suitable here since `readdirSync()` returns `string[]`) +- **Testability:** Constant is module-scoped, not exported. For testing, prefer integration tests that verify workflow installation behavior rather than unit-testing the constant +- **Verdict:** APPROVED. Type system is correct, build is clean, no `noUncheckedIndexedAccess` violations +πŸ“Œ Team update (2026-03-05T10-35-50Z): PR #201 workflow filter approved by all reviewers β€” framework/scaffolding distinction, implementation pattern validated, test coverage noted β€” decided by Keaton, Fenster, Hockney, Edie diff --git a/.squad/agents/fenster/history.md b/.squad/agents/fenster/history.md index c9e83907b..30e6261e9 100644 --- a/.squad/agents/fenster/history.md +++ b/.squad/agents/fenster/history.md @@ -578,3 +578,41 @@ The CLI couldn't run because `packages/squad-sdk/src/index.ts` was missing re-ex - Updated team.md with @copilot git workflow instructions - Key: Skill file is the single source of truth β€” coordinator loads it and injects into spawn prompts - Decision: `release` branch dropped per Keaton's recommendation (YAGNI pre-1.0) + +--- + +### 2026-03-05: Workflow Filter Implementation Review (PR williamhallatt/201) + +**Requested by:** williamhallatt (validation review of completed PR) + +**Context:** Branch `williamhallatt/201-investigate-actions-install` changed `packages/squad-sdk/src/config/init.ts` to filter workflow installation to only Squad-framework workflows (4 files) instead of copying all workflows from templates/. + +**Changes reviewed:** +- Added `FRAMEWORK_WORKFLOWS` constant (4 filenames: squad-heartbeat.yml, squad-issue-assign.yml, squad-triage.yml, sync-squad-labels.yml) +- Renamed `workflowFiles` β†’ `allWorkflowFiles` in read step +- Filtered with `allWorkflowFiles.filter(f => FRAMEWORK_WORKFLOWS.includes(f))` +- Tests updated in `test/workflows.test.js` to validate framework workflows installed, CI/CD workflows excluded + +**Implementation assessment:** +βœ… Core logic sound β€” read all `.yml` β†’ filter to framework β†’ copy filtered list +βœ… Variable rename clean and semantically correct throughout loop +βœ… `Array.includes()` appropriate for 4-item array (no perf concern, matches existing patterns at lines 744, 768) +βœ… Edge cases handled gracefully: + - Missing template files: silently skipped (no error, operates on disk-present files) + - `skipExisting: true` applies correctly (filter happens before copy loop) + - CLI layer has no bypass mechanism (filtering is SDK-internal, correct separation) +βœ… Constant placement discoverable (module-scope, well-commented, before `initSquad()`) +βœ… No other callers to update (workflow logic self-contained in `includeWorkflows` block) +βœ… Tests updated correctly (validates framework installed, CI/CD excluded) + +**Verdict:** APPROVED + +**Minor observation:** Missing template file handling is acceptable but could be improved β€” if a file in `FRAMEWORK_WORKFLOWS` doesn't exist in templates/, it's silently skipped with no warning. Not a blocker, but future enhancement could log warning. + +## Learnings + +- For small constant arrays (≀5 items), `Array.includes()` is idiomatic and performs equivalently to `Set.has()` β€” prefer readability over premature optimization +- When filtering file lists before copy loops, operate on the disk-present files first (`readdirSync` β†’ filter extensions β†’ filter whitelist) β€” this makes missing template files self-healing (no error thrown) +- Workflow installation in init.ts is self-contained in the `includeWorkflows` block β€” SDK layer controls filtering, CLI layer only gates the feature on/off + +πŸ“Œ Team update (2026-03-05T10-35-50Z): PR #201 workflow filter approved by all reviewers β€” framework/scaffolding distinction, implementation pattern validated, test coverage noted β€” decided by Keaton, Fenster, Hockney, Edie diff --git a/.squad/agents/hockney/history.md b/.squad/agents/hockney/history.md index dc7a6f667..95e209a63 100644 --- a/.squad/agents/hockney/history.md +++ b/.squad/agents/hockney/history.md @@ -803,3 +803,52 @@ All labeled squad:hockney for routing. Each issue includes: what's missing, why - **Key finding:** The SDK dist was stale (still had old `.squad-templates` path). Source was already updated but `npm run build` hadn't been run. Rebuilt SDK to verify test passes. - **Pre-existing failure:** Line 94 gitattributes content mismatch (unrelated, not introduced by this change). - **Lesson:** On Windows, use `join(root, '.squad', 'templates')` not `join(root, '.squad/templates')` β€” forward-slash segments in `join` args work on Node but it's better practice to use separate args. + +### Workflow filtering test coverage review β€” Issue #201 (2026-03-04) +**Status:** Complete β€” validated williamhallatt/201-investigate-actions-install branch. +**Context:** Only 4 FRAMEWORK_WORKFLOWS now installed by `squad init` (heartbeat, triage, issue-assign, sync-labels). 8 CI/CD workflows (ci, preview, release, docs, insider-release, label-enforce, main-guard, promote) NOT installed until `squad upgrade`. +**Test files reviewed:** +1. `test/workflows.test.js` (CJS, NOT run by vitest β€” only used for manual testing) +2. `test/cli/init.test.ts` (TypeScript, vitest runs this) +3. `test/cli/upgrade.test.ts` (TypeScript, vitest runs this) + +**Findings:** + +**workflows.test.js (CJS, NOT executed by vitest):** +- βœ… Correctly defines `FRAMEWORK_WORKFLOWS` (4 files) and `CI_CD_WORKFLOWS` (3 files: ci, preview, release) +- βœ… Tests split: init copies FRAMEWORK_WORKFLOWS, NOT CI_CD_WORKFLOWS +- βœ… Upgrade still copies CI/CD workflows (via templates.ts TEMPLATE_MANIFEST) +- ⚠️ Line 165 test "upgrade overwrites stale workflow content" β€” calls `initSquad(tmpDir)` (which no longer copies CI/CD), manually writes stale file to first available CI/CD workflow path, then runs upgrade. **This test still passes** because upgrade DOES copy CI/CD workflows. +- ⚠️ `runSquad([], dir)` in this test calls the built CLI (`index.js`) β€” runs actual init logic. Test structure is sound. + +**init.test.ts (vitest, line 129):** +- Line 136: `expect(ymlFiles.length).toBeGreaterThan(0)` β€” WEAK. Passes with 4 files OR 12 files OR 1 file. Would NOT catch regression where 0 files installed. +- ❌ **Gap:** No assertion that exactly 4 framework workflows are installed. +- ❌ **Gap:** No assertion that CI/CD workflows are NOT installed. +- βœ… Test does verify workflow directory exists and contains .yml files. + +**upgrade.test.ts (vitest, line 94):** +- Line 102: `expect(result.filesUpdated.some(f => f.includes('workflows'))).toBe(true)` β€” WEAK. This passes because upgrade touches workflows (CI/CD via templates.ts). +- ⚠️ Test doesn't verify WHICH workflows are updated. Could pass if only 1 workflow copied. +- βœ… Test does verify upgrade returns workflow updates in filesUpdated array. + +**Coverage gaps identified:** +1. **init.test.ts needs specific assertions:** + - Assert exactly 4 FRAMEWORK_WORKFLOWS present after init + - Assert CI/CD workflows (ci, preview, release, docs, etc.) are ABSENT after init + - Assert workflow content is valid YAML +2. **upgrade.test.ts needs specific assertions:** + - Assert CI/CD workflows are present AFTER upgrade + - Assert count of upgraded workflow files matches TEMPLATE_MANIFEST entries +3. **No regression protection:** If init.ts accidentally clears FRAMEWORK_WORKFLOWS array, init.test.ts line 136 still passes (empty array > 0 = false, test would fail β€” OK). But if it installs 1 wrong file, test passes. + +**Risk assessment:** +- **workflows.test.js (CJS):** Thorough, but NOT executed by vitest. Only runs via manual `node --test test/workflows.test.js`. +- **vitest suite (init + upgrade):** Weak assertions. Would NOT catch: + - Wrong workflows installed + - Extra CI/CD files leaked into init + - Missing framework workflows +- **Net coverage:** ⚠️ Adequate for smoke testing, inadequate for regression protection. + +**Recommendation:** APPROVED WITH NOTES β€” change is correct, but test coverage should be strengthened in a follow-up. Vitest suite needs explicit workflow name assertions. +πŸ“Œ Team update (2026-03-05T10-35-50Z): PR #201 workflow filter approved by all reviewers β€” framework/scaffolding distinction, implementation pattern validated, test coverage noted β€” decided by Keaton, Fenster, Hockney, Edie diff --git a/.squad/agents/keaton/history.md b/.squad/agents/keaton/history.md index 46fa4c676..61efc464f 100644 --- a/.squad/agents/keaton/history.md +++ b/.squad/agents/keaton/history.md @@ -702,3 +702,31 @@ This history accurately documents Keaton's work and decisions. Future spawns can πŸ“Œ Team update (2026-03-04T17:52:00Z): Migration docs file-safety guidance added β€” doctor command now live in CLI (fixes #188) β€” decided by Keaton, implemented by McManus + +### 2026-03-05T20:50:00Z: Issue #201 Fix Review β€” Workflow Install Filter Architecture + +- **Task:** Architectural review of PR fixing issue #201 (`squad init` was copying ALL 12 workflows including Squad's own CI/CD pipeline). PR introduces `FRAMEWORK_WORKFLOWS` constant to filter init to 4 framework workflows only. Two files changed: init.ts (filter logic), workflows.test.js (test expectations). +- **Context:** William Hallatt implemented fix on branch `williamhallatt/201-investigate-actions-install`. Tests pass. Issue reported that users were getting squad-ci.yml, squad-release.yml, squad-preview.yml, etc. β€” workflows meant for Squad's own repository, not user repos. Fix distinguishes "framework workflows" (always installed) from "CI/CD scaffolding" (opt-in via upgrade). +- **Review Findings:** + 1. **βœ… FRAMEWORK_WORKFLOWS classification is correct.** Four files in the constant: + - `squad-heartbeat.yml` (Ralph's automated status updates) + - `squad-issue-assign.yml` (automatic issue assignment to squad members) + - `squad-triage.yml` (triage workflow on `squad` label) + - `sync-squad-labels.yml` (label sync from team.md) + All four are Squad framework infrastructure, not user CI/CD. Verified by inspecting workflow triggers: they operate on issues/labels, not PR builds or releases. + 2. **βœ… Exclusion list is correct.** Eight workflows excluded: + - `squad-ci.yml`, `squad-release.yml`, `squad-preview.yml`, `squad-insider-release.yml`, `squad-docs.yml` (all build/deploy workflows) + - `squad-main-guard.yml`, `squad-promote.yml`, `squad-label-enforce.yml` (governance workflows) + Classification verified against templates.ts TEMPLATE_MANIFEST lines 176–245. All workflows have `overwriteOnUpgrade: true` (meaning upgrade.ts will copy them), so the gap isn't an exclusion error β€” it's by design. + 3. **βœ… Hard exclusion is the right architectural call.** No flag, no opt-in needed for init. Reasoning: CI/CD workflows are project-specific (npm vs. Python vs. Go vs. .NET). Templates exist as generic scaffolding, not production-ready defaults. Users can manually copy from `.squad/templates/workflows/` if desired. upgrade.ts already copies ALL workflows (lines 409–422), so existing users get updates. Init should be minimal; upgrade should be comprehensive. This is a clean separation of concerns. + 4. **⚠️ The upgrade gap is MEANINGFUL and should be documented.** Current architecture: + - `squad init` (init.ts:817): Filters to 4 framework workflows only. + - `squad upgrade` (upgrade.ts:409–422): Copies ALL 12 workflows (no filter). + This means: (A) New users get 4 workflows. (B) Existing users who ran init before this fix already have all 12 workflows, and `squad upgrade` will keep updating them. (C) Users who manually delete unwanted workflows will see them restored on every upgrade. + **Impact:** Users who want to remove squad-ci.yml must manually delete it after EVERY upgrade. This is a paper cut but not a blocker. The "right" architectural fix would be: upgrade.ts reads a user preference file (e.g., `.squad/config.json` field `workflows: ["squad-heartbeat", "squad-issue-assign"]`) and only updates opted-in workflows. But that's future scope, not this PR's responsibility. + 5. **βœ… Constant placement is appropriate.** Defined at module scope (line 446), immediately before initSquad() (line 453), after stampVersionInContent() (line 438). Follows existing pattern (other module-level constants defined near function signatures). JSDoc explains classification clearly. No issues. + 6. **βœ… PR is ready to submit.** Tests updated correctly (test/workflows.test.js lines 18–26, 122–133). New test block verifies CI/CD workflows are NOT installed (lines 136–147). YAML validity tests updated to check framework workflows only (line 198). All changes are minimal, surgical, correct. +- **Recommended PR note:** Suggest adding one sentence to PR description: "Note: `squad upgrade` will continue to copy all workflows (including CI/CD templates) for existing users. Future enhancement: let users opt out of specific workflows in upgrade via config file." +- **Verdict:** **βœ… APPROVED.** Classification correct, architecture sound, tests comprehensive. Hard exclusion is the right call for init. Upgrade gap is known and acceptable for this iteration. PR is production-ready. +- **Pattern learned:** When filtering lists in initialization, distinguish "framework infrastructure" (always needed) from "user scaffolding" (opt-in/customizable). Clear constant naming + JSDoc makes the policy visible to future maintainers. For workflows specifically: framework = issue/label automation, scaffolding = build/release/deploy. +πŸ“Œ Team update (2026-03-05T10-35-50Z): PR #201 workflow filter approved by all reviewers β€” framework/scaffolding distinction, implementation pattern validated, test coverage noted β€” decided by Keaton, Fenster, Hockney, Edie diff --git a/.squad/config.json b/.squad/config.json new file mode 100644 index 000000000..a1f09ba0a --- /dev/null +++ b/.squad/config.json @@ -0,0 +1,4 @@ +{ + "version": 1, + "teamRoot": "/home/williamh/code/squad" +} \ No newline at end of file diff --git a/.squad/decisions.md b/.squad/decisions.md index ccb7a2d61..dd472b693 100644 --- a/.squad/decisions.md +++ b/.squad/decisions.md @@ -2002,3 +2002,270 @@ This is low-effort, high-value β€” a broken link on the GitHub Pages site erodes **Action Required:** Brady to decide: keep origin/insiders (matches decision) + delete origin/insider, or keep origin/insider and delete origin/insiders. **Status:** Infrastructure ready. Awaiting final branch naming confirmation. +# Decision β€” Workflow Install Filter Architecture (#201) + +**Date:** 2026-03-05 +**Author:** Keaton (Lead) +**Context:** PR review for issue #201 fix (workflow install filter) + +## Summary + +Approved PR that filters `squad init` to install only 4 framework workflows (squad-heartbeat, squad-issue-assign, squad-triage, sync-squad-labels), excluding 8 generic CI/CD scaffolding workflows (squad-ci, squad-release, etc.). Classification is correct. Hard exclusion (no flag) is the right architectural call. + +## Decision + +**Adopt the "framework vs. scaffolding" distinction for workflow installation:** + +- **Framework workflows** (always installed by init): Issue/label automation that Squad needs to function (heartbeat, triage, assignment, label sync) +- **Scaffolding workflows** (opt-in via manual copy or upgrade): Build/release/deploy templates that are project-specific (CI, release, preview, docs, etc.) + +**Rationale:** +1. CI/CD workflows are project-specific (npm vs. Python vs. Go). Generic templates aren't production-ready for most users. +2. Users upgrading from versions before this fix already have all 12 workflows β€” `squad upgrade` continues to update them. This maintains backward compatibility. +3. New users get minimal working Squad infrastructure. They can copy scaffolding from `.squad/templates/workflows/` if needed. + +## Known Trade-Off + +**`squad upgrade` behavior is NOT aligned with this filter.** Current architecture: +- `squad init` (init.ts): Installs 4 framework workflows only +- `squad upgrade` (upgrade.ts lines 409–422): Copies ALL 12 workflows + +**Implication:** Users who manually delete unwanted workflows (e.g., squad-ci.yml) will see them restored on every upgrade. + +**Why this is acceptable for now:** +- The "right" fix (user preference in `.squad/config.json` to opt out of specific workflows) is future scope, not blocking. +- Existing users are already on this path (they have all 12). New users won't be surprised (they start with 4). +- Upgrade is Squad-owned territory (overwriteOnUpgrade: true) β€” users expect Squad to refresh its own workflows. + +**Future enhancement:** Let users opt out of specific workflows in `squad upgrade` via config field (e.g., `workflows: { exclude: ["squad-ci.yml", "squad-release.yml"] }`). + +## Pattern + +When filtering installation lists, distinguish **framework infrastructure** (always needed for the system to function) from **user scaffolding** (customizable, project-specific). Make the policy visible via: +1. Named constant with JSDoc explanation +2. Test coverage that verifies both inclusion AND exclusion +3. Documentation note for users about manual copy path + +For workflows specifically: +- **Framework** = issue/label automation (Squad's operational infrastructure) +- **Scaffolding** = build/release/deploy (user's project-specific CI/CD) +# Decision: FRAMEWORK_WORKFLOWS type pattern for filtered installation + +**Context:** PR #201 adds `FRAMEWORK_WORKFLOWS` constant to filter which workflow files get installed during `squad init`. Only Squad framework workflows are installed by default; generic CI/CD scaffolding is opt-in. + +**Type pattern validated:** +```typescript +const FRAMEWORK_WORKFLOWS = [ + 'squad-heartbeat.yml', + 'squad-issue-assign.yml', + 'squad-triage.yml', + 'sync-squad-labels.yml', +]; + +// Usage: +const allWorkflowFiles = readdirSync(workflowsSrc).filter(f => f.endsWith('.yml')); +const workflowFiles = allWorkflowFiles.filter(f => FRAMEWORK_WORKFLOWS.includes(f)); +``` + +**Type inference:** `string[]` (correct). `Array.prototype.includes(value: string)` accepts `string` from `readdirSync()`. + +**Alternatives considered:** +- `as const` β†’ Would narrow to `readonly ['squad-heartbeat.yml', ...]`, making `.includes()` require literal types instead of `string`. Not suitable when filtering runtime values from `readdirSync()`. +- `readonly string[]` β†’ No benefit over inferred `string[]` for module-scoped constant. Array is never mutated. + +**Recommendation:** Keep inferred `string[]` for constants used with `.includes()` on runtime `string` values. Use `as const` only when you need literal type narrowing (e.g., discriminated unions, enum-like behavior). + +**Testability:** Constant is module-scoped (not exported). Integration tests should verify correct workflow installation behavior, not unit-test the constant itself. + +**Build verification:** `npm run build` and `npm run lint` pass cleanly with zero errors. + +**Decided by:** Edie +**Date:** 2026-03-03 +**Status:** APPROVED β€” type pattern is correct, no changes needed +# Test Coverage Gaps: Workflow Filtering (Issue #201) + +**Date:** 2026-03-04 +**Reviewer:** Hockney (Tester) +**Branch:** williamhallatt/201-investigate-actions-install +**Status:** Change is correct, tests need strengthening + +## Background + +Squad init now installs only 4 FRAMEWORK_WORKFLOWS (heartbeat, triage, issue-assign, sync-labels). The 8 CI/CD workflows (ci, preview, release, docs, insider-release, label-enforce, main-guard, promote) are NOT installed until `squad upgrade`. + +## Test Coverage Issues + +### 1. `test/workflows.test.js` (CJS) β€” NOT executed by vitest + +- **Status:** Comprehensive tests exist but are never run by `npm test` +- **Issue:** vitest.config.ts includes only `test/**/*.test.ts`, excludes `.js` files +- **Tests present:** + - βœ… Init copies FRAMEWORK_WORKFLOWS + - βœ… Init does NOT copy CI/CD workflows + - βœ… Upgrade copies CI/CD workflows + - βœ… Workflow YAML validity checks +- **Risk:** If this file goes stale, no one will notice + +### 2. `test/cli/init.test.ts` (vitest, line 129-138) β€” Weak assertions + +**Current test:** +```typescript +it('should copy workflow files to .github/workflows/', async () => { + await runInit(TEST_ROOT); + + const workflowsPath = join(TEST_ROOT, '.github', 'workflows'); + if (existsSync(workflowsPath)) { + const files = await readdir(workflowsPath); + const ymlFiles = files.filter(f => f.endsWith('.yml')); + expect(ymlFiles.length).toBeGreaterThan(0); // ⚠️ WEAK + } +}); +``` + +**Problems:** +- Passes with 1 file OR 4 files OR 12 files +- Does NOT verify which workflows are installed +- Does NOT verify CI/CD workflows are absent + +**Should be:** +```typescript +it('should copy only FRAMEWORK_WORKFLOWS to .github/workflows/', async () => { + await runInit(TEST_ROOT); + + const workflowsPath = join(TEST_ROOT, '.github', 'workflows'); + const files = await readdir(workflowsPath); + const ymlFiles = files.filter(f => f.endsWith('.yml')); + + // Assert exactly 4 framework workflows + expect(ymlFiles).toHaveLength(4); + expect(ymlFiles).toContain('squad-heartbeat.yml'); + expect(ymlFiles).toContain('squad-triage.yml'); + expect(ymlFiles).toContain('squad-issue-assign.yml'); + expect(ymlFiles).toContain('sync-squad-labels.yml'); + + // Assert CI/CD workflows are NOT installed + expect(ymlFiles).not.toContain('squad-ci.yml'); + expect(ymlFiles).not.toContain('squad-preview.yml'); + expect(ymlFiles).not.toContain('squad-release.yml'); +}); +``` + +### 3. `test/cli/upgrade.test.ts` (vitest, line 94-104) β€” No workflow verification + +**Current test:** +```typescript +it('should upgrade workflows', async () => { + const workflowsDir = join(TEST_ROOT, '.github', 'workflows'); + + if (existsSync(workflowsDir)) { + const result = await runUpgrade(TEST_ROOT); + expect(result.filesUpdated.some(f => f.includes('workflows'))).toBe(true); // ⚠️ WEAK + } +}); +``` + +**Problems:** +- Does NOT verify which workflows are upgraded +- Passes if only 1 workflow touched +- Does NOT verify CI/CD workflows are present after upgrade + +**Should be:** +```typescript +it('should install CI/CD workflows during upgrade', async () => { + await runInit(TEST_ROOT); + + // Verify framework workflows installed + const workflowsDir = join(TEST_ROOT, '.github', 'workflows'); + let files = await readdir(workflowsDir); + expect(files).toHaveLength(4); // Only framework workflows + + // Run upgrade + const result = await runUpgrade(TEST_ROOT); + + // Verify CI/CD workflows now present + files = await readdir(workflowsDir); + expect(files.length).toBeGreaterThan(4); // Framework + CI/CD + expect(files).toContain('squad-ci.yml'); + expect(files).toContain('squad-preview.yml'); + expect(files).toContain('squad-release.yml'); + + // Verify upgrade report includes workflows + expect(result.filesUpdated.some(f => f.includes('workflows'))).toBe(true); +}); +``` + +## Regressions NOT Caught by Current Tests + +1. If `FRAMEWORK_WORKFLOWS` array is accidentally cleared β†’ init.test.ts line 136 would fail (good), but error message would be vague +2. If init accidentally installs 1 extra CI/CD workflow β†’ init.test.ts still passes +3. If upgrade skips a CI/CD workflow β†’ upgrade.test.ts still passes +4. If workflows are invalid YAML β†’ no vitest test catches this (workflows.test.js does, but isn't run) + +## Recommendations + +### Immediate (for this PR): +**APPROVED WITH NOTES** β€” Change is correct, merge it. Tests are adequate for smoke testing. + +### Follow-up (separate issue/PR): +1. **Make workflows.test.js executable by vitest:** + - Convert to TypeScript OR + - Update vitest.config.ts to include `test/**/*.test.js` +2. **Strengthen init.test.ts:** + - Replace line 136 with explicit workflow name assertions + - Add negative assertions for CI/CD workflows +3. **Strengthen upgrade.test.ts:** + - Add workflow count verification + - Add explicit CI/CD workflow presence checks +4. **Add YAML validity test to vitest suite:** + - Parse workflow files with a YAML library + - Assert required fields (name, on, jobs) + +## Impact + +- **Current risk:** Medium β€” Manual testing catches issues, but CI doesn't +- **With changes:** Low β€” Automated tests would catch workflow installation regressions +- **Effort:** 2-4 hours for a focused test improvement task + +--- + +**Decision needed:** +Should workflows.test.js be converted to TypeScript and integrated into vitest, or should we duplicate its assertions in init.test.ts/upgrade.test.ts? +# Decision: Workflow Filter Implementation Pattern + +**Context:** PR williamhallatt/201 implemented filtering of workflow files during `squad init` to only install Squad-framework workflows (4 files) instead of copying all workflows from templates/. + +**Implementation Pattern:** +```typescript +// 1. Define framework workflows as module-scope constant +const FRAMEWORK_WORKFLOWS = [ + 'squad-heartbeat.yml', + 'squad-issue-assign.yml', + 'squad-triage.yml', + 'sync-squad-labels.yml', +]; + +// 2. Read disk, filter extensions, then filter to whitelist +const allWorkflowFiles = readdirSync(workflowsSrc).filter(f => f.endsWith('.yml')); +const workflowFiles = allWorkflowFiles.filter(f => FRAMEWORK_WORKFLOWS.includes(f)); + +// 3. Copy loop operates on filtered list +for (const file of workflowFiles) { + // copy logic with skipExisting check +} +``` + +**Why This Pattern:** +- βœ… **Graceful handling of missing templates**: Filtering happens on disk-present files, so if a framework workflow is missing from templates/, no error is thrown +- βœ… **Separation of concerns**: SDK layer controls filtering, CLI layer only gates feature on/off (`includeWorkflows: true`) +- βœ… **Discoverable**: Module-scope constant with clear comment explaining framework vs. opt-in distinction +- βœ… **Consistent with codebase**: Matches existing `Array.includes()` patterns in init.ts (lines 744, 768) +- βœ… **Self-documenting**: Variable rename (`workflowFiles` β†’ `allWorkflowFiles` + new filtered `workflowFiles`) makes intent clear + +**Alternative Considered:** +- Using `Set.has()` for filtering β€” rejected as premature optimization for 4-item array (< 1ms difference) + +**Future Enhancement (optional, not blocking):** +- Log warning if a file in `FRAMEWORK_WORKFLOWS` doesn't exist in templates/ β€” helps catch template drift during development + +**Decided by:** Fenster (implementation review) +**Date:** 2026-03-05 diff --git a/.squad/decisions/inbox/copilot-docs-migration-retrospective.md b/.squad/decisions/inbox/copilot-docs-migration-retrospective.md new file mode 100644 index 000000000..d2cb7726d --- /dev/null +++ b/.squad/decisions/inbox/copilot-docs-migration-retrospective.md @@ -0,0 +1,137 @@ +# Docs Retrospective: v0.5.4 β†’ v0.8.x Migration Failure + +**Author:** McManus (DevRel) +**Context:** User migration failed due to stale, fragmented, and version-specific documentation +**Outcome:** Complete rewrite of 8 docs + 1 blog; identified structural failures in how docs are maintained + +--- + +## What Failed + +### 1. **Version Pinning Without Automatic Updates** +- `migration.md` pinned `@0.8.18` in 2024. By 2025, `v0.8.21` shippedβ€”docs not updated. +- Users followed 6-month-old instructions and hit version mismatches. +- `troubleshooting.md` had Node.js "22.0.0" hard-coded with no way to detect drift. +- **Root cause:** No process ties docs to `package.json` version; no CI check for stale references. + +### 2. **Multiple Doc Sources for One Topic** +- Migration info scattered across: + - `migration.md` (global install, v0.5.4 focus) + - `upgrading.md` (existing v0.8.x users) + - `scenarios/keep-my-squad.md` (partial overlap) + - `blog/021-the-migration.md` (historical, v0.8.18 launch) + - `disaster-recovery.md` (backup scenarios) +- No single source of truth β†’ users got conflicting information. +- **Root cause:** Docs grew organically without a clear domain ownership model. + +### 3. **Removed Distributions Not Documented** +- `npx github:bradygaster/squad` was removed but `docs/reference/cli.md` still recommended it. +- `docs/scenarios/upgrading.md` referenced the removed distribution. +- Users who followed old links or guides hit 404s. +- **Root cause:** No "what changed" audit when distributions sunset. + +### 4. **Internal Commands Listed as User-Facing** +- `squad migrate` didn't exist in the CLI initially (added in v0.8.20). +- `squad init` is internal, not user-facing, but no docs clarified this. +- Users got confused about which commands were stable/public. +- **Root cause:** No docs-as-contract for CLI; changes made in code without docs sync. + +### 5. **No "What's New" for Patch/Minor Releases** +- `whatsnew.md` was missing v0.8.21 section entirely. +- v0.8.18 β†’ v0.8.21 is 3 patch releasesβ€”users didn't know what was added. +- Migration guide didn't call out `squad migrate` as NEW (v0.8.20). +- **Root cause:** Release checklist didn't require "what's new" doc updates. + +--- + +## What We Do Now + +### Lesson 1: **Pin Docs to Code; Version Everything in CI** +**What we do now:** +- Every version reference in docs (Node.js, package versions, feature availability) gets a `` HTML comment. +- CI job (on each release) finds all version comments and compares against `package.json`. +- Template in `docs/template.html` defines how version comments are written. +- Release checklist requires: "Run version audit CI before merge." +- If a version reference is >2 releases old, CI fails the build. + +### Lesson 2: **Single Source of Truth per Task; Domain Owners** +**What we do now:** +- Docs map to CLI commands/features, not arbitrary topics: + - `docs/get-started/migration.md` = "getting from old squad to new squad" (owns ALL paths: global, local, CI/CD) + - `docs/scenarios/upgrading.md` = "already installed, want latest" (owns upgrade/rollback) + - `docs/scenarios/troubleshooting.md` = "something broke" (owns diagnosis) + - Blog posts = "announcements and stories" (reference docs, don't duplicate) +- `.squad/decisions.md` now tracks: "migration guide owner: [person]"; quarterly sync review. +- Incoming PRs that touch "migration" route to domain owner for approval. + +### Lesson 3: **Breaking Changes & Distribution Sunsets Require Docs Audit** +**What we do now:** +- Release checklist adds: + ``` + - [ ] List all distributions/commands removed/deprecated in this release + - [ ] For each: find all docs that mention it; update or flag as breaking change + - [ ] For each: check GH issues/discussions for user questions on old method; link new method + ``` +- `docs/reference/cli.md` now has a **Deprecated** section at the top with migration paths. +- When `npx github:bradygaster/squad` was removed, a callout was added to `migration.md` with the new command. +- Blog post required on major distribution changes; link from all affected docs. + +### Lesson 4: **CLI Commands are Public Contracts; Docs-First Design** +**What we do now:** +- New CLI command = PR flow: + 1. Docs PR adds command to `docs/reference/cli.md` (marking as internal/experimental if needed). + 2. Code PR references docs PR: "See docs/reference/cli.md for contract." + 3. Both reviewed together; code review confirms docs accuracy. + 4. If a command changes after initial PR, docs updated first, then code. +- `squad init` marked **Internal Command** in CLI ref with explanation. +- `squad migrate` has a "Since v0.8.20" tag in docs and CLI help. + +### Lesson 5: **Changelog Drives "What's New"; Release Docs Template** +**What we do now:** +- `CHANGELOG.md` is the single source for version changes (required by release checklist). +- `docs/whatsnew.md` auto-generated from CHANGELOG for each release. +- Template in release-checklist.md includes: + ``` + ## Release Documentation + - [ ] Update CHANGELOG.md with all changes + - [ ] Generate docs/whatsnew.md from CHANGELOG (template in docs/template.html) + - [ ] Verify migration guide links to whatsnew if breaking changes exist + - [ ] Blog post required for minor+; optional for patches + ``` +- Users always see what changed; `whatsnew.md` is the central hub. + +--- + +## McManus Practices (On Every New Feature/Release) + +1. **Read the code PR first.** What's the contract?β€”What's new, what broke, what's internal? +2. **Check: does a docs PR exist?** If not, ask the author to open one before merge. +3. **Version all references** (code versions, Node versions, URLs) with `` comments. +4. **Test all links, commands, code examples.** Run them locally or in CI. +5. **Update CHANGELOG before release.** Severity: blocker. (Release checklist already enforces.) +6. **After release: Generate whatsnew section.** Link from migration guide if breaking. +7. **Quarterly: Domain owner review.** Docs ownership shouldn't drift; revisit every release cycle. + +--- + +## Structural Changes Made + +- **`.squad/decisions.md`** now has "docs ownership" section +- **`release-checklist.md`** now includes docs sync and version audit steps +- **`docs/reference/cli.md`** has "Internal Command" and "Since vX.Y.Z" labels +- **`docs/whatsnew.md`** template added; generated from CHANGELOG +- **`docs/get-started/migration.md`** is now the canonical source (consolidated 8 docs + updates) +- **CI workflow** (new job) validates version references on every PR to main + +--- + +## Bottom Line + +**The failure wasn't about bad docs; it was about docs that couldn't stay in sync with code.** We fixed it by: +- Tying docs to code contracts (CLI as the API). +- Making version references machine-detectable. +- Eliminating duplication (one source per topic). +- Automating what we can (whatsnew generation, version checks). +- Adding docs to the release process, not an afterthought. + +Every team member now knows: docs aren't complete until the code is merged AND docs are synced. diff --git a/.squad/decisions/inbox/hockney-migrate-test-gaps.md b/.squad/decisions/inbox/hockney-migrate-test-gaps.md new file mode 100644 index 000000000..02d5ca4f7 --- /dev/null +++ b/.squad/decisions/inbox/hockney-migrate-test-gaps.md @@ -0,0 +1,164 @@ +# Testing Lessons from Migrate Command (57 tests, 5 critical bugs caught) + +**By:** Hockney (Tester) +**Date:** 2026-03-03 +**Context:** Code review of migrate-command.test.ts, cast-guard.test.ts, migrate-e2e.test.ts revealed gaps that allowed 5 bugs to slip through or only surface during peer review. + +--- + +## Executive Summary + +We wrote 57 tests for the migrate command and caught 5 significant bugs: +1. **Shell reinit after migrate** β€” `.first-run` not cleaned after reinit (lifecycle bug) +2. **Casting registry missing** β€” SDK wipe didn't recreate registry (state loss) +3. **config.json survival** β€” Not in USER_OWNED, nearly lost during wipe (data loss) +4. **Path traversal** β€” `--restore` and `--backup-dir` lacked containment checks (security) +5. **Partial backup rollback** β€” No `backupComplete` flag prevented destructive recovery (data corruption) + +**Root cause pattern:** Tests existed but were category-gapped. We tested the happy path and feature branches but didn't test: +- Filesystem state machine transitions (what happens *between* steps?) +- Security boundaries (path containment, symlink traversal) +- Recovery paths under partial failure (backup completeness + rollback) +- Integration points (shell lifecycle through migrate, SDK state recreation) + +--- + +## 3 Lessons + Action Items + +### 1. **Lifecycle Tests Must Cross CLI Boundaries** + +**Gap:** Tests for migrate existed. Tests for shell first-run gating existed. But NO test covered the contract between them: migrate should NOT trigger re-init when a user runs `squad shell` afterward. + +**What happened:** +- Migrate reinit via `sdkInitSquad()` created `.first-run` marker +- Shell checks `.first-run` to decide if init UI should show +- User sees "Welcome to Squad!" a second time ❌ + +**What we do now:** +- Every CLI command that modifies `.squad/` must test the downstream lifecycle impact +- For migrate: add test that runs `squad shell` (or at least checks shell entry condition) post-migration +- For any new `--reinit` or `--reset` flag: test that `.first-run`, `.init-prompt`, session markers are in the correct state for the *next* command + +**Pattern for new commands:** +```typescript +describe('Downstream shell interaction', () => { + it('leaves .squad/ in a state that does NOT retrigger init', () => { + // Run command that reinits + // Verify firstRunGating conditions are false + // Verify no leftover init markers exist + }); +}); +``` + +--- + +### 2. **SQUAD_OWNED vs USER_OWNED Is a Security+Correctness Boundary** + +**Gap:** Tests covered individual files in USER_OWNED (team.md, agents/) but didn't test the full set as a category. The list is canonical and easy to get wrong: +```typescript +const USER_OWNED = [ + 'team.md', 'agents/', 'identity/', + 'config.json', // <-- This was missed initially + 'orchestration-log.md', // etc +]; +``` + +**What happened:** +- Migrate wipes SQUAD_OWNED (templates, casting, etc.) +- Tests verified agents/ survived βœ“ +- Tests verified team.md survived βœ“ +- Tests did NOT verify config.json survived ❌ +- User lost squad.config.ts-derived config on migrate + +**What we do now:** +- Create a `test/fixtures/full-squad-dir.ts` that populates BOTH USER_OWNED and SQUAD_OWNED +- Test that after wipe+reinit, USER_OWNED files are **byte-identical** to before +- Test that SQUAD_OWNED is **recreated fresh** (not preserved) +- Any new file added to USER_OWNED requires: + 1. Update migrate.ts constants + 2. Add to both migrate-command.test.ts and migrate-e2e.test.ts fixture + 3. Add assertion `expect(fs.readFileSync(...)).toBe(originalContent)` + +**For all CLI commands that modify `.squad/`:** +- Maintain a shared `USER_OWNED_FILES` constant in a util +- Use it in every test that checks preservation/deletion + +--- + +### 3. **Security Boundaries Need Dedicated Test Category + Fuzzing** + +**Gap:** Migrate added `--restore` and `--backup-dir` flags with path containment checks: +```typescript +if (!path.resolve(backupRoot).startsWith(path.resolve(cwd))) { + error('path must be within the current directory'); +} +``` + +Tests for this existed (line 79 in migrate.ts comment), but only at the unit level. No E2E test. + +**What happened:** +- Tests used valid relative/absolute paths within cwd βœ“ +- Tests did NOT try: + - `--restore ../../../etc/passwd` (outside cwd) + - `--restore /tmp/evil` (absolute path outside cwd) + - `--restore ~/.squad-backup-evil` (symlink traversal) + - `--backup-dir=.` (current dir, ambiguous) + +**What we do now:** +- Every CLI flag that accepts a path gets a dedicated security test block: + ```typescript + describe('Security: path containment', () => { + it('rejects paths outside cwd', () => { + expect(() => runMigrate(cwd, { restore: '../../../etc/passwd' })) + .toThrow(/must be within/); + }); + it('rejects absolute paths outside cwd', () => { + expect(() => runMigrate(cwd, { restore: '/tmp/evil' })) + .toThrow(/must be within/); + }); + it('rejects symlinks that escape cwd', () => { + // Create a symlink: cwd/.squad-backup-evil -> /tmp + // Verify it's rejected + }); + }); + ``` +- Assign this test writing to Baer (Security) for review before merge + +--- + +## 4 Patterns for New CLI Commands + +When adding a new command (e.g., `squad export`, `squad import`), enforce: + +| Pattern | Why | Example Test | +|---------|-----|--------------| +| **Filesystem State Machine** | Commands have pre/post conditions; tests must verify *both* | Before: `.squad/` is X. Command runs. After: `.squad/` is Y. No in-between corruption. | +| **Boundary Categorization** | Files are USER_OWNED or SQUAD_OWNED; tests verify the right category is touched | Add to USER_OWNED? Update fixture + add preservation test. Delete? Add deletion test. | +| **Path Security** | Any `--path`, `--dir`, `--restore` flag must contain-check | `path.resolve(input).startsWith(path.resolve(cwd))` | +| **Recovery Paths** | If command has `--backup`, test that a partial backup doesn't corrupt on rollback | Only restore if `backupComplete` flag is true | +| **Downstream Lifecycle** | Commands that modify `.squad/` must test the next shell/init interaction | After migrate: verify `.first-run` doesn't exist (no re-init) | + +--- + +## Update to Hockney's Charter + +Add to `.squad/agents/hockney/charter.md` under "What I Own": + +> - **Lifecycle contracts:** CLI commands that modify `.squad/` must test their impact on downstream commands (shell init, casting state, etc.) +> - **Boundary testing:** USER_OWNED vs SQUAD_OWNED file preservation is tested as a category, not piecemeal +> - **Security test blocks:** Any path-accepting flag triggers a dedicated security category with containment, traversal, and symlink tests +> - **Recovery tests:** Backup/restore/rollback paths must test partial failure scenarios (e.g., incomplete backup + forced rollback) + +--- + +## Conclusion + +**80% coverage is the floor.** But coverage β‰  completeness. Tests must organize around: +1. **Boundaries** (what changes, what doesn't) +2. **Contracts** (CLI command β†’ downstream lifecycle) +3. **Security** (paths, escapes, boundaries) +4. **Recovery** (partial failure is a feature; test it) + +The 5 bugs caught here would have shipped without peer review. Two of them (shell reinit, config.json loss) would hit production users. The migration test gaps are now obvious in hindsightβ€”but only because we wrote 57 tests and forced the team to think hard about what wasn't tested. + +Next command: organize tests by these categories from the start. diff --git a/.squad/decisions/inbox/keaton-migration-arch-review.md b/.squad/decisions/inbox/keaton-migration-arch-review.md new file mode 100644 index 000000000..9961c88ec --- /dev/null +++ b/.squad/decisions/inbox/keaton-migration-arch-review.md @@ -0,0 +1,111 @@ +# Lessons from Migration Feature Work (Issue #197, PR #199) + +**By:** Keaton (Lead) +**Date:** 2026-03-06 +**Context:** Completed `squad migrate` command (57 tests, 8 pre-PR blockers caught by team review) + +--- + +## Lesson 1: File Ownership Model Must Be Explicit at Design Time + +**Problem Found:** +- `config.json` was not in `USER_OWNED`, so migration destroyed user customization +- `casting/registry.json` was marked `SQUAD_OWNED` (wiped), but `sdkInitSquad()` didn't recreate itβ€”stale state +- `.first-run` marker created by `sdkInitSquad()` was left behind post-migrate, causing shell to re-enter Init Mode + +**Root Cause:** +The migration command was designed without explicit categorization of what `.squad/` files belong to *system* vs. *user*. This was only discovered when testing the real upgrade path. + +**What We Do Now:** +- **Before implementation:** Any feature that touches state files (migrations, exports, imports, resets) must document file ownership explicitly in the proposal (`SQUAD_OWNED` vs. `USER_OWNED`). +- **Add to `.squad/` charter:** "File ownership is non-negotiable architectural context. If unclear, ask Keaton or Baer before coding." +- **Add to checklists:** Code review must include "verify all state files are categorized (SQUAD_OWNED/USER_OWNED)" before approving CLI state mutations. + +--- + +## Lesson 2: Security Boundaries Need Static Enforcement, Not Runtime Checks + +**Problem Found:** +- `--restore` and `--backup-dir` had no cwd containment guardβ€”path traversal was possible +- `copyRecursive()` restore targets weren't validatedβ€”could write outside `.squad/` +- These blockers came from Baer (Security) in pre-PR team review, not from code review + +**Root Cause:** +The migrate command implemented runtime path validation *after* path resolution, which could pass invalid paths through at critical moments. No guard to prevent `--restore ../../../etc`. + +**What We Do Now:** +- **New pattern for file operations:** All path flags (`--restore`, `--backup-dir`, etc.) must be validated *before* use: `if (!path.startsWith(cwd)) { throw SquadError }`. +- **Add to security checklist:** CLI commands touching the filesystem must be reviewed by Baer (or designated security owner) before merge. +- **Add to Baer's charter:** "All file I/O in CLI is security-relevant. Path validation non-negotiable." + +--- + +## Lesson 3: Correctness Invariants Must Be Tracked Explicitly + +**Problem Found:** +- `runMigrate()` could attempt rollback even if backup failedβ€”risking data loss +- The only guard was "hope the backup completed"β€”no `backupComplete` flag +- This blocker came from Fenster (Core Dev) in pre-PR review, not from tests + +**Root Cause:** +The code had an implicit assumption ("if we reach rollback, backup succeeded") but no enforcement. Test coverage was strong (57 tests) but didn't catch this logic error because happy paths always succeeded. + +**What We Do Now:** +- **New pattern for multi-step operations:** If a later step depends on an earlier step succeeding, use an explicit boolean flag (e.g., `backupComplete`). Document it in the code comment. +- **Add to code review checklist:** "Identify preconditions for rollback/cleanup/error paths. Verify they're enforced with explicit state, not implicit assumptions." +- **For next migration-like command:** Fenster reviews the error recovery paths before tests are written. + +--- + +## Lesson 4: Pre-PR Team Review Caught 8 Blockers That Code Review Missed + +**Problem Found:** +- Security (path traversal) β€” caught by Baer +- Correctness (backup incomplete rollback) β€” caught by Fenster +- Feature scope (config.json ownership) β€” caught by Keaton +- UX (sdkInitSquad internal naming, error styling) β€” caught by Marquez +- Docs (install command incorrect) β€” caught by McManus +- Plus 3 other fixes + +**Root Cause:** +Code review focused on *implementation correctness* (does the code do what it says?). Team review focused on *system correctness* (does this feature fit the architecture and user expectations?). + +**What We Do Now:** +- **Mandatory team review gate before PR for high-risk changes:** Features touching: + - State file mutations (migrations, exports, resets, backups) + - Security boundaries (path I/O, PII handling) + - Initialization/lifecycle (init, cleanup, re-entry guards) + - Should route to relevant agents *before* PR goes up, in a comment: "Ready for pre-PR team review" β†’ Baer, Fenster, Keaton, Marquez +- **Add to routing.md:** High-risk features get async pre-PR feedback from domain owners (security β†’ Baer, core runtime β†’ Fenster, arch β†’ Keaton, UX β†’ Marquez). +- **Document this in CONTRIBUTING.md:** "For features marked as 'high-risk' (migrations, security, lifecycle), request pre-PR feedback in an issue comment before opening the PR." + +--- + +## Lesson 5: PR Target Branch Is a Process Failure Waiting to Happen + +**Problem Found:** +- PR #199 was initially opened against `main` instead of `dev` (per CONTRIBUTING.md, features go to `dev`) +- Caught during PR creation, not during code review +- Small mistake, but revealed no automated enforcement + +**Root Cause:** +CONTRIBUTING.md documents the rule but GitHub/Git don't enforce it at push/PR-create time. + +**What We Do Now:** +- **Add branch protection rule:** Base branch for squad feature PRs must be `dev`, not `main`. Enforce this at GitHub level with branch protection rules. +- **If GitHub enforcement isn't available:** Add a pre-push hook that rejects direct pushes to `main` unless the commit includes "Closes #X" (release workflow exemption). +- **Document in CONTRIBUTING.md:** "Feature branches target `dev`. Release branches target `main`. If you see a PR targeting wrong branch, request change before review." + +--- + +## Summary: Architecture β†’ Process β†’ Codification + +| Gap | Addressed By | Now Codified In | +|-----|--------------|-----------------| +| File ownership implicit | Lesson 1 | `.squad/` charter + code review checklist | +| Path validation ad-hoc | Lesson 2 | Security checklist + Baer charter | +| Correctness assumptions unmeasured | Lesson 3 | Code review checklist + Fenster review pattern | +| Team review happened too late | Lesson 4 | Routing.md + CONTRIBUTING.md (pre-PR gate) | +| Branch target not enforced | Lesson 5 | GitHub branch protection rules | + +**Captured for:** All future migration-like features, high-risk CLI work, and state-mutation commands. diff --git a/.squad/decisions/inbox/waingro-destructive-qa-discipline.md b/.squad/decisions/inbox/waingro-destructive-qa-discipline.md new file mode 100644 index 000000000..fa5690ca6 --- /dev/null +++ b/.squad/decisions/inbox/waingro-destructive-qa-discipline.md @@ -0,0 +1,108 @@ +# QA Discipline: Destructive Commands Need Adversarial Testing at Design Time + +**Author:** Waingro (Product Dogfooder) +**Date:** 2026-03-04 +**Status:** Proposal (awaiting Scribe merge) +**Triggered by:** `squad migrate` post-user-testing bugs + +--- + +## Context + +The `squad migrate` command had a pre-PR team review (9 agents) that caught **2/4 critical bugs**. Post-user testing found **2 additional bugs**: + +| Bug | Severity | Review | User | Root Cause | +|-----|----------|--------|------|-----------| +| `casting/registry.json` wiped, not recreated | P0 | ❌ | βœ… | SDK skips registry.json; logic gap in migrate | +| `.first-run` marker β†’ re-entry to Init Mode | P1 | ❌ | βœ… | Incomplete marker cleanup | +| `config.json` destroyed (SQUAD_OWNED not USER_OWNED) | P0 | βœ… | N/A | File ownership violation | +| `--restore` path traversal (no cwd containment) | P0 | βœ… | N/A | Missing validation | + +This reflects a **QA discipline gap**: code review catches logical and security issues, but destructive operations need adversarial testing for state contamination, version compatibility, and edge cases. + +--- + +## Decision + +**For any CLI command that deletes, moves, or overwrites files, the implementation must include:** + +1. **File Ownership Matrix** (explicit, non-overlapping) + - SQUAD_OWNED files (regenerated fresh) vs USER_OWNED files (preserved) + - No overlap; documented at code level + +2. **Gherkin Scenario Matrix** (written at design time) + - File ownership enforcement scenarios + - Version-specific regression scenarios (β‰₯2 prior major versions) + - State contamination scenarios (.first-run, shell markers) + - Dangerous input scenarios (path traversal, symlinks) + - Partial failure & rollback scenarios + - Idempotence scenarios + +3. **Pre-Flight Checklist** (in PR template for destructive commands) + - [ ] File ownership matrix explicit & non-overlapping + - [ ] Rollback path: error during mutation β†’ restore from backup + - [ ] Marker cleanup: .first-run, .init-prompt, shell state removed + - [ ] Path validation: containment checks (startsWith cwd) + - [ ] Version compat: test β‰₯2 prior major versions + - [ ] Idempotence: safe to run twice + - [ ] Dry-run: pixel-perfect, no mutation + - [ ] Partial failure: can roll back mid-operation + - [ ] User communication: next steps clear + - [ ] Adversarial inputs: path traversal, symlinks, collisions + +4. **Shell State Tests** (dedicated test category) + - Any command touching `.squad/.first-run` or markers + - Verify shell would NOT re-enter Init Mode post-migration + - Verify markers are not created during reinit + +5. **Version-Specific Fixtures** (test against known cohorts) + - v0.5.x: no casting/, minimal agents + - v0.8.x: casting/ present, full registry + - Current: up-to-date schema + - Test each variant in isolation + +--- + +## Examples + +### Gherkin Scenarios for `squad migrate` + +```gherkin +Scenario: Migrate handles repos with no casting state (v0.5.x) + Given a .squad/ from v0.5.x (no casting/ directory) + When running 'squad migrate' + Then registry.json is created from agents/ + And casting/policy.json and casting/history.json are initialized + +Scenario: Migrate removes init-mode markers + Given a migrated repo with agent roster + When 'squad migrate' completes + Then .squad/.first-run does NOT exist + And shell does not re-enter Init Mode + +Scenario: --restore rejects path traversal attempts + When running 'squad migrate --restore ../../etc/passwd' + Then exit code is 1 + And no files outside cwd are accessed +``` + +### Pre-Flight Checklist in PR + +When reviewing a destructive command, the reviewer should verify all 10 items before approval. Include this checklist in the PR template. + +--- + +## Impact + +- **Code review focus:** syntax, logic, security (what reviewers are good at) +- **Adversarial QA focus:** state contamination, version compat, edge cases (what QA is good at) +- **Result:** destructive commands get both disciplines instead of one alone + +--- + +## Implementation + +1. Scribe: merge this decision +2. Keaton/Fenster: update PR template to include destructive command checklist +3. Waingro: add "destructive command" tag to issue templates +4. Squad: on next destructive command PR, enforce the checklist diff --git a/.squad/log/2026-03-05T10-35-50Z-validation-review.md b/.squad/log/2026-03-05T10-35-50Z-validation-review.md new file mode 100644 index 000000000..efb79c85a --- /dev/null +++ b/.squad/log/2026-03-05T10-35-50Z-validation-review.md @@ -0,0 +1,40 @@ +# Session Log: Workflow Filter PR #201 Validation + +**Timestamp:** 2026-03-05T10-35-50Z +**Topic:** PR #201 validation review β€” architectural, SDK, test, TypeScript + +## Agents Engaged + +| Agent | Role | Status | +|-------|------|--------| +| Keaton | Lead (Architecture) | APPROVED | +| Fenster | Core Dev (SDK) | APPROVED | +| Hockney | Tester (CI/CD) | APPROVED WITH NOTES | +| Edie | TypeScript (Build) | APPROVED | + +## Key Decisions + +1. **Framework vs. Scaffolding Distinction** (Keaton) + - Framework workflows (4): always installed by init + - Scaffolding workflows (8): opt-in via upgrade/manual copy + - Known trade-off: upgrade still copies all 12 (future scope) + +2. **Implementation Pattern** (Fenster) + - Module-scoped `FRAMEWORK_WORKFLOWS` constant + - Filter on disk-present files + whitelist + - Graceful missing-template handling + +3. **Test Coverage Gaps** (Hockney) + - Current tests are adequate (smoke testing) + - Weak assertions in init.test.ts and upgrade.test.ts + - workflows.test.js not executed by vitest + - Recommendation: Test improvement follow-up task + +4. **Type Validation** (Edie) + - `string[]` type inference correct + - Build clean (npm run build, npm run lint) + - Pattern ready for production + +## Outcome + +βœ… **Ready to merge.** All reviews complete. Test improvements tracked as separate work. diff --git a/.squad/orchestration-log/2026-03-05T10-35-50Z-edie.md b/.squad/orchestration-log/2026-03-05T10-35-50Z-edie.md new file mode 100644 index 000000000..9a9fb3646 --- /dev/null +++ b/.squad/orchestration-log/2026-03-05T10-35-50Z-edie.md @@ -0,0 +1,44 @@ +# Orchestration: Edie (TypeScript) + +**Timestamp:** 2026-03-05T10-35-50Z +**Agent:** Edie (TypeScript & Build) +**Status:** APPROVED +**Task:** TypeScript and build validation for workflow filtering + +## Summary + +Edie validated the TypeScript implementation for PR #201. The `FRAMEWORK_WORKFLOWS` constant uses correct type inference (`string[]`). Alternatives considered (as const, readonly string[]) but rejected. Build validation clean. + +## Type Pattern Validated + +```typescript +const FRAMEWORK_WORKFLOWS = [ + 'squad-heartbeat.yml', + 'squad-issue-assign.yml', + 'squad-triage.yml', + 'sync-squad-labels.yml', +]; +``` + +**Type inference:** `string[]` (correct for runtime filtering with `.includes()`) + +## Alternatives Rejected + +- `as const` β†’ Would narrow to literal types, not suitable for runtime string values from `readdirSync()` +- `readonly string[]` β†’ No benefit over inferred type; constant is never mutated + +## Build Status + +βœ… `npm run build` β€” zero errors +βœ… `npm run lint` β€” zero errors +βœ… Type inference correct for `.includes()` usage + +## Outcome + +βœ… **APPROVED** β€” Type pattern correct, no changes needed. Implementation is production-ready. + +## Decision Logged + +- **Decision ID:** edie-workflow-filter-review.md +- **Pattern:** Inferred `string[]` for runtime filtering +- **Build:** Verified clean diff --git a/.squad/orchestration-log/2026-03-05T10-35-50Z-fenster.md b/.squad/orchestration-log/2026-03-05T10-35-50Z-fenster.md new file mode 100644 index 000000000..acf7ef578 --- /dev/null +++ b/.squad/orchestration-log/2026-03-05T10-35-50Z-fenster.md @@ -0,0 +1,34 @@ +# Orchestration: Fenster (Core Dev) + +**Timestamp:** 2026-03-05T10-35-50Z +**Agent:** Fenster (Core Dev) +**Status:** APPROVED +**Task:** SDK implementation review of init.ts workflow filtering + +## Summary + +Fenster reviewed the implementation pattern for workflow filtering in PR #201. The pattern is clean, graceful, and consistent with existing codebase patterns. Uses module-scoped constant `FRAMEWORK_WORKFLOWS` with clear JSDoc explanation. + +## Implementation Pattern Approved + +```typescript +const FRAMEWORK_WORKFLOWS = [ + 'squad-heartbeat.yml', + 'squad-issue-assign.yml', + 'squad-triage.yml', + 'sync-squad-labels.yml', +]; + +const allWorkflowFiles = readdirSync(workflowsSrc).filter(f => f.endsWith('.yml')); +const workflowFiles = allWorkflowFiles.filter(f => FRAMEWORK_WORKFLOWS.includes(f)); +``` + +## Outcome + +βœ… **APPROVED** β€” Implementation is production-ready. Alternative of `Set.has()` considered but rejected (premature optimization for 4-item array). + +## Decision Logged + +- **Decision ID:** fenster-workflow-filter-review.md +- **Pattern:** Module-scoped constant + filtering loop +- **CLS:** Matches existing Array.includes() patterns diff --git a/.squad/orchestration-log/2026-03-05T10-35-50Z-hockney.md b/.squad/orchestration-log/2026-03-05T10-35-50Z-hockney.md new file mode 100644 index 000000000..627e10125 --- /dev/null +++ b/.squad/orchestration-log/2026-03-05T10-35-50Z-hockney.md @@ -0,0 +1,44 @@ +# Orchestration: Hockney (Tester) + +**Timestamp:** 2026-03-05T10-35-50Z +**Agent:** Hockney (CI/CD & Testing) +**Status:** APPROVED WITH NOTES +**Task:** Test coverage review for workflow filtering + +## Summary + +Hockney audited test coverage for PR #201. Change is correct and production-ready. Tests are adequate for smoke testing but have gaps: + +- `test/workflows.test.js` (CJS) is comprehensive but not executed by vitest +- `test/cli/init.test.ts` has weak assertions (pass with 1, 4, or 12 files) +- `test/cli/upgrade.test.ts` has no workflow verification + +## Test Coverage Status + +| Component | Coverage | Issue | +|-----------|----------|-------| +| workflows.test.js | Complete | Not run by vitest (CJS excluded) | +| init.test.ts (line 136) | Weak | Only checks count > 0 | +| upgrade.test.ts (line 78) | Weak | Only checks any workflow touched | + +## Regressions NOT Caught + +1. If `FRAMEWORK_WORKFLOWS` array cleared β†’ vague error +2. If init installs 1 extra CI/CD workflow β†’ test still passes +3. If upgrade skips CI/CD workflow β†’ test still passes +4. Invalid YAML β†’ no vitest test catches + +## Outcome + +βœ… **APPROVED WITH NOTES** β€” Merge PR. Test improvements as follow-up (separate issue). + +## Recommendations + +**Immediate:** Convert workflows.test.js to TS or update vitest.config to include .js files. +**Follow-up:** Strengthen init/upgrade assertions with explicit workflow name checks. + +## Decision Logged + +- **Decision ID:** hockney-workflow-filter-review.md +- **Risk Level:** Medium (manual testing catches issues, CI doesn't) +- **Effort:** 2-4 hours for focused test improvement task diff --git a/.squad/orchestration-log/2026-03-05T10-35-50Z-keaton.md b/.squad/orchestration-log/2026-03-05T10-35-50Z-keaton.md new file mode 100644 index 000000000..8bb0af7fb --- /dev/null +++ b/.squad/orchestration-log/2026-03-05T10-35-50Z-keaton.md @@ -0,0 +1,20 @@ +# Orchestration: Keaton (Lead) + +**Timestamp:** 2026-03-05T10-35-50Z +**Agent:** Keaton (Lead) +**Status:** APPROVED +**Task:** Architectural review of workflow filter PR #201 + +## Summary + +Keaton reviewed PR #201 and approved the "framework vs. scaffolding" distinction for workflow installation. The decision establishes a clear pattern: Squad framework workflows (heartbeat, triage, issue-assign, label-sync) are always installed by `squad init`, while generic CI/CD scaffolding workflows are opt-in (via `squad upgrade` or manual copy from templates). + +## Outcome + +βœ… **APPROVED** β€” Architectural pattern is sound. Known trade-off (upgrade still copies all 12 workflows) is documented and acceptable for current scope. + +## Decision Logged + +- **Decision ID:** keaton-workflow-filter-review.md +- **Policy:** Framework vs. scaffolding distinction +- **Impact:** init.ts filtering, future enhancement path From 0512cc4a8f933e91ddd0e6065cfe6a96e43cd0c8 Mon Sep 17 00:00:00 2001 From: williamhallatt Date: Thu, 5 Mar 2026 20:38:50 +1000 Subject: [PATCH 4/7] test(cli): assert exact workflow names installed/excluded by init --- test/cli/init.test.ts | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/test/cli/init.test.ts b/test/cli/init.test.ts index 0487d6f5e..b0b85e353 100644 --- a/test/cli/init.test.ts +++ b/test/cli/init.test.ts @@ -126,14 +126,42 @@ describe('CLI: init command', () => { expect(skills.length).toBeGreaterThan(0); }); - it('should copy workflow files to .github/workflows/', async () => { + it('should install exactly the 4 framework workflows', async () => { await runInit(TEST_ROOT); const workflowsPath = join(TEST_ROOT, '.github', 'workflows'); - if (existsSync(workflowsPath)) { - const files = await readdir(workflowsPath); - const ymlFiles = files.filter(f => f.endsWith('.yml')); - expect(ymlFiles.length).toBeGreaterThan(0); + expect(existsSync(workflowsPath)).toBe(true); + + const frameworkWorkflows = [ + 'squad-heartbeat.yml', + 'squad-triage.yml', + 'squad-issue-assign.yml', + 'sync-squad-labels.yml' + ]; + + for (const workflow of frameworkWorkflows) { + expect(existsSync(join(workflowsPath, workflow))).toBe(true); + } + }); + + it('should NOT install CI/CD workflows', async () => { + await runInit(TEST_ROOT); + + const workflowsPath = join(TEST_ROOT, '.github', 'workflows'); + + const cicdWorkflows = [ + 'squad-ci.yml', + 'squad-release.yml', + 'squad-docs.yml', + 'squad-insider-release.yml', + 'squad-preview.yml', + 'squad-promote.yml', + 'squad-main-guard.yml', + 'squad-label-enforce.yml' + ]; + + for (const workflow of cicdWorkflows) { + expect(existsSync(join(workflowsPath, workflow))).toBe(false); } }); From abdec4b3d6c8f48650e912d8ab65fc78639bf8ce Mon Sep 17 00:00:00 2001 From: williamhallatt Date: Thu, 5 Mar 2026 20:40:59 +1000 Subject: [PATCH 5/7] docs(ai-team): merged testing & QA discipline decisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Session: 2026-03-05T10-39-25Z-test-coverage-fix Requested by: Scribe Changes: - Merged decision: Testing Lessons from Migrate Command (Hockney) β€” lifecycle tests, boundary testing, security test blocks, recovery tests - Merged decision: QA Discipline for Destructive Commands (Waingro) β€” file ownership matrix, gherkin scenarios, pre-flight checklist, shell state tests, version-specific fixtures - Deleted inbox files: hockney-migrate-test-gaps.md, waingro-destructive-qa-discipline.md - Logged orchestration: Hockney (test verification), Kobayashi (issue documentation) - Updated Hockney's history with team decision Both decisions establish mandatory test patterns for CLI commands that modify .squad/ or perform destructive operations. --- .squad/agents/hockney/history.md | 11 + .squad/decisions.md | 278 ++++++++++++++++++ .../copilot-docs-migration-retrospective.md | 137 --------- .../inbox/hockney-migrate-test-gaps.md | 164 ----------- .../inbox/keaton-migration-arch-review.md | 111 ------- .../waingro-destructive-qa-discipline.md | 108 ------- 6 files changed, 289 insertions(+), 520 deletions(-) delete mode 100644 .squad/decisions/inbox/copilot-docs-migration-retrospective.md delete mode 100644 .squad/decisions/inbox/hockney-migrate-test-gaps.md delete mode 100644 .squad/decisions/inbox/keaton-migration-arch-review.md delete mode 100644 .squad/decisions/inbox/waingro-destructive-qa-discipline.md diff --git a/.squad/agents/hockney/history.md b/.squad/agents/hockney/history.md index 95e209a63..cea2756e8 100644 --- a/.squad/agents/hockney/history.md +++ b/.squad/agents/hockney/history.md @@ -852,3 +852,14 @@ All labeled squad:hockney for routing. Each issue includes: what's missing, why **Recommendation:** APPROVED WITH NOTES β€” change is correct, but test coverage should be strengthened in a follow-up. Vitest suite needs explicit workflow name assertions. πŸ“Œ Team update (2026-03-05T10-35-50Z): PR #201 workflow filter approved by all reviewers β€” framework/scaffolding distinction, implementation pattern validated, test coverage noted β€” decided by Keaton, Fenster, Hockney, Edie + +## Learnings + +**2026-03-05: Strengthened init workflow test for issue #201** +- Replaced weak test `expect(ymlFiles.length).toBeGreaterThan(0)` with two focused tests +- Test 1: Verifies exactly 4 framework workflows ARE installed (squad-heartbeat.yml, squad-triage.yml, squad-issue-assign.yml, sync-squad-labels.yml) +- Test 2: Verifies 8 CI/CD workflows are NOT installed (squad-ci.yml, squad-release.yml, squad-docs.yml, squad-insider-release.yml, squad-preview.yml, squad-promote.yml, squad-main-guard.yml, squad-label-enforce.yml) +- Both tests now use explicit `existsSync()` checks per file instead of counting +- Removed conditional guard (`if existsSync`) β€” directory should always exist post-init +- Tests pass with clear βœ“ indicators for each assertion +- **Key lesson:** Positive AND negative assertions catch regressions. Testing "what should be there" AND "what should NOT be there" provides full coverage against accidental leakage or omission. diff --git a/.squad/decisions.md b/.squad/decisions.md index dd472b693..198e9be34 100644 --- a/.squad/decisions.md +++ b/.squad/decisions.md @@ -2269,3 +2269,281 @@ for (const file of workflowFiles) { **Decided by:** Fenster (implementation review) **Date:** 2026-03-05 + +--- + +# Testing Lessons from Migrate Command (57 tests, 5 critical bugs caught) + +**By:** Hockney (Tester) +**Date:** 2026-03-03 +**Context:** Code review of migrate-command.test.ts, cast-guard.test.ts, migrate-e2e.test.ts revealed gaps that allowed 5 bugs to slip through or only surface during peer review. + +--- + +## Executive Summary + +We wrote 57 tests for the migrate command and caught 5 significant bugs: +1. **Shell reinit after migrate** β€” `.first-run` not cleaned after reinit (lifecycle bug) +2. **Casting registry missing** β€” SDK wipe didn't recreate registry (state loss) +3. **config.json survival** β€” Not in USER_OWNED, nearly lost during wipe (data loss) +4. **Path traversal** β€” `--restore` and `--backup-dir` lacked containment checks (security) +5. **Partial backup rollback** β€” No `backupComplete` flag prevented destructive recovery (data corruption) + +**Root cause pattern:** Tests existed but were category-gapped. We tested the happy path and feature branches but didn't test: +- Filesystem state machine transitions (what happens *between* steps?) +- Security boundaries (path containment, symlink traversal) +- Recovery paths under partial failure (backup completeness + rollback) +- Integration points (shell lifecycle through migrate, SDK state recreation) + +--- + +## 3 Lessons + Action Items + +### 1. **Lifecycle Tests Must Cross CLI Boundaries** + +**Gap:** Tests for migrate existed. Tests for shell first-run gating existed. But NO test covered the contract between them: migrate should NOT trigger re-init when a user runs `squad shell` afterward. + +**What happened:** +- Migrate reinit via `sdkInitSquad()` created `.first-run` marker +- Shell checks `.first-run` to decide if init UI should show +- User sees "Welcome to Squad!" a second time ❌ + +**What we do now:** +- Every CLI command that modifies `.squad/` must test the downstream lifecycle impact +- For migrate: add test that runs `squad shell` (or at least checks shell entry condition) post-migration +- For any new `--reinit` or `--reset` flag: test that `.first-run`, `.init-prompt`, session markers are in the correct state for the *next* command + +**Pattern for new commands:** +```typescript +describe('Downstream shell interaction', () => { + it('leaves .squad/ in a state that does NOT retrigger init', () => { + // Run command that reinits + // Verify firstRunGating conditions are false + // Verify no leftover init markers exist + }); +}); +``` + +--- + +### 2. **SQUAD_OWNED vs USER_OWNED Is a Security+Correctness Boundary** + +**Gap:** Tests covered individual files in USER_OWNED (team.md, agents/) but didn't test the full set as a category. The list is canonical and easy to get wrong: +```typescript +const USER_OWNED = [ + 'team.md', 'agents/', 'identity/', + 'config.json', // <-- This was missed initially + 'orchestration-log.md', // etc +]; +``` + +**What happened:** +- Migrate wipes SQUAD_OWNED (templates, casting, etc.) +- Tests verified agents/ survived βœ“ +- Tests verified team.md survived βœ“ +- Tests did NOT verify config.json survived ❌ +- User lost squad.config.ts-derived config on migrate + +**What we do now:** +- Create a `test/fixtures/full-squad-dir.ts` that populates BOTH USER_OWNED and SQUAD_OWNED +- Test that after wipe+reinit, USER_OWNED files are **byte-identical** to before +- Test that SQUAD_OWNED is **recreated fresh** (not preserved) +- Any new file added to USER_OWNED requires: + 1. Update migrate.ts constants + 2. Add to both migrate-command.test.ts and migrate-e2e.test.ts fixture + 3. Add assertion `expect(fs.readFileSync(...)).toBe(originalContent)` + +**For all CLI commands that modify `.squad/`:** +- Maintain a shared `USER_OWNED_FILES` constant in a util +- Use it in every test that checks preservation/deletion + +--- + +### 3. **Security Boundaries Need Dedicated Test Category + Fuzzing** + +**Gap:** Migrate added `--restore` and `--backup-dir` flags with path containment checks: +```typescript +if (!path.resolve(backupRoot).startsWith(path.resolve(cwd))) { + error('path must be within the current directory'); +} +``` + +Tests for this existed (line 79 in migrate.ts comment), but only at the unit level. No E2E test. + +**What happened:** +- Tests used valid relative/absolute paths within cwd βœ“ +- Tests did NOT try: + - `--restore ../../../etc/passwd` (outside cwd) + - `--restore /tmp/evil` (absolute path outside cwd) + - `--restore ~/.squad-backup-evil` (symlink traversal) + - `--backup-dir=.` (current dir, ambiguous) + +**What we do now:** +- Every CLI flag that accepts a path gets a dedicated security test block: + ```typescript + describe('Security: path containment', () => { + it('rejects paths outside cwd', () => { + expect(() => runMigrate(cwd, { restore: '../../../etc/passwd' })) + .toThrow(/must be within/); + }); + it('rejects absolute paths outside cwd', () => { + expect(() => runMigrate(cwd, { restore: '/tmp/evil' })) + .toThrow(/must be within/); + }); + it('rejects symlinks that escape cwd', () => { + // Create a symlink: cwd/.squad-backup-evil -> /tmp + // Verify it's rejected + }); + }); + ``` +- Assign this test writing to Baer (Security) for review before merge + +--- + +## 4 Patterns for New CLI Commands + +When adding a new command (e.g., `squad export`, `squad import`), enforce: + +| Pattern | Why | Example Test | +|---------|-----|--------------| +| **Filesystem State Machine** | Commands have pre/post conditions; tests must verify *both* | Before: `.squad/` is X. Command runs. After: `.squad/` is Y. No in-between corruption. | +| **Boundary Categorization** | Files are USER_OWNED or SQUAD_OWNED; tests verify the right category is touched | Add to USER_OWNED? Update fixture + add preservation test. Delete? Add deletion test. | +| **Path Security** | Any `--path`, `--dir`, `--restore` flag must contain-check | `path.resolve(input).startsWith(path.resolve(cwd))` | +| **Recovery Paths** | If command has `--backup`, test that a partial backup doesn't corrupt on rollback | Only restore if `backupComplete` flag is true | +| **Downstream Lifecycle** | Commands that modify `.squad/` must test the next shell/init interaction | After migrate: verify `.first-run` doesn't exist (no re-init) | + +--- + +## Update to Hockney's Charter + +Add to `.squad/agents/hockney/charter.md` under "What I Own": + +> - **Lifecycle contracts:** CLI commands that modify `.squad/` must test their impact on downstream commands (shell init, casting state, etc.) +> - **Boundary testing:** USER_OWNED vs SQUAD_OWNED file preservation is tested as a category, not piecemeal +> - **Security test blocks:** Any path-accepting flag triggers a dedicated security category with containment, traversal, and symlink tests +> - **Recovery tests:** Backup/restore/rollback paths must test partial failure scenarios (e.g., incomplete backup + forced rollback) + +--- + +## Conclusion + +**80% coverage is the floor.** But coverage β‰  completeness. Tests must organize around: +1. **Boundaries** (what changes, what doesn't) +2. **Contracts** (CLI command β†’ downstream lifecycle) +3. **Security** (paths, escapes, boundaries) +4. **Recovery** (partial failure is a feature; test it) + +The 5 bugs caught here would have shipped without peer review. Two of them (shell reinit, config.json loss) would hit production users. The migration test gaps are now obvious in hindsightβ€”but only because we wrote 57 tests and forced the team to think hard about what wasn't tested. + +Next command: organize tests by these categories from the start. + +--- + +# QA Discipline: Destructive Commands Need Adversarial Testing at Design Time + +**Author:** Waingro (Product Dogfooder) +**Date:** 2026-03-04 +**Status:** Proposal (merged by Scribe) +**Triggered by:** `squad migrate` post-user-testing bugs + +--- + +## Context + +The `squad migrate` command had a pre-PR team review (9 agents) that caught **2/4 critical bugs**. Post-user testing found **2 additional bugs**: + +| Bug | Severity | Review | User | Root Cause | +|-----|----------|--------|------|-----------| +| `casting/registry.json` wiped, not recreated | P0 | ❌ | βœ… | SDK skips registry.json; logic gap in migrate | +| `.first-run` marker β†’ re-entry to Init Mode | P1 | ❌ | βœ… | Incomplete marker cleanup | +| `config.json` destroyed (SQUAD_OWNED not USER_OWNED) | P0 | βœ… | N/A | File ownership violation | +| `--restore` path traversal (no cwd containment) | P0 | βœ… | N/A | Missing validation | + +This reflects a **QA discipline gap**: code review catches logical and security issues, but destructive operations need adversarial testing for state contamination, version compatibility, and edge cases. + +--- + +## Decision + +**For any CLI command that deletes, moves, or overwrites files, the implementation must include:** + +1. **File Ownership Matrix** (explicit, non-overlapping) + - SQUAD_OWNED files (regenerated fresh) vs USER_OWNED files (preserved) + - No overlap; documented at code level + +2. **Gherkin Scenario Matrix** (written at design time) + - File ownership enforcement scenarios + - Version-specific regression scenarios (β‰₯2 prior major versions) + - State contamination scenarios (.first-run, shell markers) + - Dangerous input scenarios (path traversal, symlinks) + - Partial failure & rollback scenarios + - Idempotence scenarios + +3. **Pre-Flight Checklist** (in PR template for destructive commands) + - [ ] File ownership matrix explicit & non-overlapping + - [ ] Rollback path: error during mutation β†’ restore from backup + - [ ] Marker cleanup: .first-run, .init-prompt, shell state removed + - [ ] Path validation: containment checks (startsWith cwd) + - [ ] Version compat: test β‰₯2 prior major versions + - [ ] Idempotence: safe to run twice + - [ ] Dry-run: pixel-perfect, no mutation + - [ ] Partial failure: can roll back mid-operation + - [ ] User communication: next steps clear + - [ ] Adversarial inputs: path traversal, symlinks, collisions + +4. **Shell State Tests** (dedicated test category) + - Any command touching `.squad/.first-run` or markers + - Verify shell would NOT re-enter Init Mode post-migration + - Verify markers are not created during reinit + +5. **Version-Specific Fixtures** (test against known cohorts) + - v0.5.x: no casting/, minimal agents + - v0.8.x: casting/ present, full registry + - Current: up-to-date schema + - Test each variant in isolation + +--- + +## Examples + +### Gherkin Scenarios for `squad migrate` + +```gherkin +Scenario: Migrate handles repos with no casting state (v0.5.x) + Given a .squad/ from v0.5.x (no casting/ directory) + When running 'squad migrate' + Then registry.json is created from agents/ + And casting/policy.json and casting/history.json are initialized + +Scenario: Migrate removes init-mode markers + Given a migrated repo with agent roster + When 'squad migrate' completes + Then .squad/.first-run does NOT exist + And shell does not re-enter Init Mode + +Scenario: --restore rejects path traversal attempts + When running 'squad migrate --restore ../../etc/passwd' + Then exit code is 1 + And no files outside cwd are accessed +``` + +### Pre-Flight Checklist in PR + +When reviewing a destructive command, the reviewer should verify all 10 items before approval. Include this checklist in the PR template. + +--- + +## Impact + +- **Code review focus:** syntax, logic, security (what reviewers are good at) +- **Adversarial QA focus:** state contamination, version compat, edge cases (what QA is good at) +- **Result:** destructive commands get both disciplines instead of one alone + +--- + +## Implementation + +1. Scribe: merge this decision +2. Keaton/Fenster: update PR template to include destructive command checklist +3. Waingro: add "destructive command" tag to issue templates +4. Squad: on next destructive command PR, enforce the checklist diff --git a/.squad/decisions/inbox/copilot-docs-migration-retrospective.md b/.squad/decisions/inbox/copilot-docs-migration-retrospective.md deleted file mode 100644 index d2cb7726d..000000000 --- a/.squad/decisions/inbox/copilot-docs-migration-retrospective.md +++ /dev/null @@ -1,137 +0,0 @@ -# Docs Retrospective: v0.5.4 β†’ v0.8.x Migration Failure - -**Author:** McManus (DevRel) -**Context:** User migration failed due to stale, fragmented, and version-specific documentation -**Outcome:** Complete rewrite of 8 docs + 1 blog; identified structural failures in how docs are maintained - ---- - -## What Failed - -### 1. **Version Pinning Without Automatic Updates** -- `migration.md` pinned `@0.8.18` in 2024. By 2025, `v0.8.21` shippedβ€”docs not updated. -- Users followed 6-month-old instructions and hit version mismatches. -- `troubleshooting.md` had Node.js "22.0.0" hard-coded with no way to detect drift. -- **Root cause:** No process ties docs to `package.json` version; no CI check for stale references. - -### 2. **Multiple Doc Sources for One Topic** -- Migration info scattered across: - - `migration.md` (global install, v0.5.4 focus) - - `upgrading.md` (existing v0.8.x users) - - `scenarios/keep-my-squad.md` (partial overlap) - - `blog/021-the-migration.md` (historical, v0.8.18 launch) - - `disaster-recovery.md` (backup scenarios) -- No single source of truth β†’ users got conflicting information. -- **Root cause:** Docs grew organically without a clear domain ownership model. - -### 3. **Removed Distributions Not Documented** -- `npx github:bradygaster/squad` was removed but `docs/reference/cli.md` still recommended it. -- `docs/scenarios/upgrading.md` referenced the removed distribution. -- Users who followed old links or guides hit 404s. -- **Root cause:** No "what changed" audit when distributions sunset. - -### 4. **Internal Commands Listed as User-Facing** -- `squad migrate` didn't exist in the CLI initially (added in v0.8.20). -- `squad init` is internal, not user-facing, but no docs clarified this. -- Users got confused about which commands were stable/public. -- **Root cause:** No docs-as-contract for CLI; changes made in code without docs sync. - -### 5. **No "What's New" for Patch/Minor Releases** -- `whatsnew.md` was missing v0.8.21 section entirely. -- v0.8.18 β†’ v0.8.21 is 3 patch releasesβ€”users didn't know what was added. -- Migration guide didn't call out `squad migrate` as NEW (v0.8.20). -- **Root cause:** Release checklist didn't require "what's new" doc updates. - ---- - -## What We Do Now - -### Lesson 1: **Pin Docs to Code; Version Everything in CI** -**What we do now:** -- Every version reference in docs (Node.js, package versions, feature availability) gets a `` HTML comment. -- CI job (on each release) finds all version comments and compares against `package.json`. -- Template in `docs/template.html` defines how version comments are written. -- Release checklist requires: "Run version audit CI before merge." -- If a version reference is >2 releases old, CI fails the build. - -### Lesson 2: **Single Source of Truth per Task; Domain Owners** -**What we do now:** -- Docs map to CLI commands/features, not arbitrary topics: - - `docs/get-started/migration.md` = "getting from old squad to new squad" (owns ALL paths: global, local, CI/CD) - - `docs/scenarios/upgrading.md` = "already installed, want latest" (owns upgrade/rollback) - - `docs/scenarios/troubleshooting.md` = "something broke" (owns diagnosis) - - Blog posts = "announcements and stories" (reference docs, don't duplicate) -- `.squad/decisions.md` now tracks: "migration guide owner: [person]"; quarterly sync review. -- Incoming PRs that touch "migration" route to domain owner for approval. - -### Lesson 3: **Breaking Changes & Distribution Sunsets Require Docs Audit** -**What we do now:** -- Release checklist adds: - ``` - - [ ] List all distributions/commands removed/deprecated in this release - - [ ] For each: find all docs that mention it; update or flag as breaking change - - [ ] For each: check GH issues/discussions for user questions on old method; link new method - ``` -- `docs/reference/cli.md` now has a **Deprecated** section at the top with migration paths. -- When `npx github:bradygaster/squad` was removed, a callout was added to `migration.md` with the new command. -- Blog post required on major distribution changes; link from all affected docs. - -### Lesson 4: **CLI Commands are Public Contracts; Docs-First Design** -**What we do now:** -- New CLI command = PR flow: - 1. Docs PR adds command to `docs/reference/cli.md` (marking as internal/experimental if needed). - 2. Code PR references docs PR: "See docs/reference/cli.md for contract." - 3. Both reviewed together; code review confirms docs accuracy. - 4. If a command changes after initial PR, docs updated first, then code. -- `squad init` marked **Internal Command** in CLI ref with explanation. -- `squad migrate` has a "Since v0.8.20" tag in docs and CLI help. - -### Lesson 5: **Changelog Drives "What's New"; Release Docs Template** -**What we do now:** -- `CHANGELOG.md` is the single source for version changes (required by release checklist). -- `docs/whatsnew.md` auto-generated from CHANGELOG for each release. -- Template in release-checklist.md includes: - ``` - ## Release Documentation - - [ ] Update CHANGELOG.md with all changes - - [ ] Generate docs/whatsnew.md from CHANGELOG (template in docs/template.html) - - [ ] Verify migration guide links to whatsnew if breaking changes exist - - [ ] Blog post required for minor+; optional for patches - ``` -- Users always see what changed; `whatsnew.md` is the central hub. - ---- - -## McManus Practices (On Every New Feature/Release) - -1. **Read the code PR first.** What's the contract?β€”What's new, what broke, what's internal? -2. **Check: does a docs PR exist?** If not, ask the author to open one before merge. -3. **Version all references** (code versions, Node versions, URLs) with `` comments. -4. **Test all links, commands, code examples.** Run them locally or in CI. -5. **Update CHANGELOG before release.** Severity: blocker. (Release checklist already enforces.) -6. **After release: Generate whatsnew section.** Link from migration guide if breaking. -7. **Quarterly: Domain owner review.** Docs ownership shouldn't drift; revisit every release cycle. - ---- - -## Structural Changes Made - -- **`.squad/decisions.md`** now has "docs ownership" section -- **`release-checklist.md`** now includes docs sync and version audit steps -- **`docs/reference/cli.md`** has "Internal Command" and "Since vX.Y.Z" labels -- **`docs/whatsnew.md`** template added; generated from CHANGELOG -- **`docs/get-started/migration.md`** is now the canonical source (consolidated 8 docs + updates) -- **CI workflow** (new job) validates version references on every PR to main - ---- - -## Bottom Line - -**The failure wasn't about bad docs; it was about docs that couldn't stay in sync with code.** We fixed it by: -- Tying docs to code contracts (CLI as the API). -- Making version references machine-detectable. -- Eliminating duplication (one source per topic). -- Automating what we can (whatsnew generation, version checks). -- Adding docs to the release process, not an afterthought. - -Every team member now knows: docs aren't complete until the code is merged AND docs are synced. diff --git a/.squad/decisions/inbox/hockney-migrate-test-gaps.md b/.squad/decisions/inbox/hockney-migrate-test-gaps.md deleted file mode 100644 index 02d5ca4f7..000000000 --- a/.squad/decisions/inbox/hockney-migrate-test-gaps.md +++ /dev/null @@ -1,164 +0,0 @@ -# Testing Lessons from Migrate Command (57 tests, 5 critical bugs caught) - -**By:** Hockney (Tester) -**Date:** 2026-03-03 -**Context:** Code review of migrate-command.test.ts, cast-guard.test.ts, migrate-e2e.test.ts revealed gaps that allowed 5 bugs to slip through or only surface during peer review. - ---- - -## Executive Summary - -We wrote 57 tests for the migrate command and caught 5 significant bugs: -1. **Shell reinit after migrate** β€” `.first-run` not cleaned after reinit (lifecycle bug) -2. **Casting registry missing** β€” SDK wipe didn't recreate registry (state loss) -3. **config.json survival** β€” Not in USER_OWNED, nearly lost during wipe (data loss) -4. **Path traversal** β€” `--restore` and `--backup-dir` lacked containment checks (security) -5. **Partial backup rollback** β€” No `backupComplete` flag prevented destructive recovery (data corruption) - -**Root cause pattern:** Tests existed but were category-gapped. We tested the happy path and feature branches but didn't test: -- Filesystem state machine transitions (what happens *between* steps?) -- Security boundaries (path containment, symlink traversal) -- Recovery paths under partial failure (backup completeness + rollback) -- Integration points (shell lifecycle through migrate, SDK state recreation) - ---- - -## 3 Lessons + Action Items - -### 1. **Lifecycle Tests Must Cross CLI Boundaries** - -**Gap:** Tests for migrate existed. Tests for shell first-run gating existed. But NO test covered the contract between them: migrate should NOT trigger re-init when a user runs `squad shell` afterward. - -**What happened:** -- Migrate reinit via `sdkInitSquad()` created `.first-run` marker -- Shell checks `.first-run` to decide if init UI should show -- User sees "Welcome to Squad!" a second time ❌ - -**What we do now:** -- Every CLI command that modifies `.squad/` must test the downstream lifecycle impact -- For migrate: add test that runs `squad shell` (or at least checks shell entry condition) post-migration -- For any new `--reinit` or `--reset` flag: test that `.first-run`, `.init-prompt`, session markers are in the correct state for the *next* command - -**Pattern for new commands:** -```typescript -describe('Downstream shell interaction', () => { - it('leaves .squad/ in a state that does NOT retrigger init', () => { - // Run command that reinits - // Verify firstRunGating conditions are false - // Verify no leftover init markers exist - }); -}); -``` - ---- - -### 2. **SQUAD_OWNED vs USER_OWNED Is a Security+Correctness Boundary** - -**Gap:** Tests covered individual files in USER_OWNED (team.md, agents/) but didn't test the full set as a category. The list is canonical and easy to get wrong: -```typescript -const USER_OWNED = [ - 'team.md', 'agents/', 'identity/', - 'config.json', // <-- This was missed initially - 'orchestration-log.md', // etc -]; -``` - -**What happened:** -- Migrate wipes SQUAD_OWNED (templates, casting, etc.) -- Tests verified agents/ survived βœ“ -- Tests verified team.md survived βœ“ -- Tests did NOT verify config.json survived ❌ -- User lost squad.config.ts-derived config on migrate - -**What we do now:** -- Create a `test/fixtures/full-squad-dir.ts` that populates BOTH USER_OWNED and SQUAD_OWNED -- Test that after wipe+reinit, USER_OWNED files are **byte-identical** to before -- Test that SQUAD_OWNED is **recreated fresh** (not preserved) -- Any new file added to USER_OWNED requires: - 1. Update migrate.ts constants - 2. Add to both migrate-command.test.ts and migrate-e2e.test.ts fixture - 3. Add assertion `expect(fs.readFileSync(...)).toBe(originalContent)` - -**For all CLI commands that modify `.squad/`:** -- Maintain a shared `USER_OWNED_FILES` constant in a util -- Use it in every test that checks preservation/deletion - ---- - -### 3. **Security Boundaries Need Dedicated Test Category + Fuzzing** - -**Gap:** Migrate added `--restore` and `--backup-dir` flags with path containment checks: -```typescript -if (!path.resolve(backupRoot).startsWith(path.resolve(cwd))) { - error('path must be within the current directory'); -} -``` - -Tests for this existed (line 79 in migrate.ts comment), but only at the unit level. No E2E test. - -**What happened:** -- Tests used valid relative/absolute paths within cwd βœ“ -- Tests did NOT try: - - `--restore ../../../etc/passwd` (outside cwd) - - `--restore /tmp/evil` (absolute path outside cwd) - - `--restore ~/.squad-backup-evil` (symlink traversal) - - `--backup-dir=.` (current dir, ambiguous) - -**What we do now:** -- Every CLI flag that accepts a path gets a dedicated security test block: - ```typescript - describe('Security: path containment', () => { - it('rejects paths outside cwd', () => { - expect(() => runMigrate(cwd, { restore: '../../../etc/passwd' })) - .toThrow(/must be within/); - }); - it('rejects absolute paths outside cwd', () => { - expect(() => runMigrate(cwd, { restore: '/tmp/evil' })) - .toThrow(/must be within/); - }); - it('rejects symlinks that escape cwd', () => { - // Create a symlink: cwd/.squad-backup-evil -> /tmp - // Verify it's rejected - }); - }); - ``` -- Assign this test writing to Baer (Security) for review before merge - ---- - -## 4 Patterns for New CLI Commands - -When adding a new command (e.g., `squad export`, `squad import`), enforce: - -| Pattern | Why | Example Test | -|---------|-----|--------------| -| **Filesystem State Machine** | Commands have pre/post conditions; tests must verify *both* | Before: `.squad/` is X. Command runs. After: `.squad/` is Y. No in-between corruption. | -| **Boundary Categorization** | Files are USER_OWNED or SQUAD_OWNED; tests verify the right category is touched | Add to USER_OWNED? Update fixture + add preservation test. Delete? Add deletion test. | -| **Path Security** | Any `--path`, `--dir`, `--restore` flag must contain-check | `path.resolve(input).startsWith(path.resolve(cwd))` | -| **Recovery Paths** | If command has `--backup`, test that a partial backup doesn't corrupt on rollback | Only restore if `backupComplete` flag is true | -| **Downstream Lifecycle** | Commands that modify `.squad/` must test the next shell/init interaction | After migrate: verify `.first-run` doesn't exist (no re-init) | - ---- - -## Update to Hockney's Charter - -Add to `.squad/agents/hockney/charter.md` under "What I Own": - -> - **Lifecycle contracts:** CLI commands that modify `.squad/` must test their impact on downstream commands (shell init, casting state, etc.) -> - **Boundary testing:** USER_OWNED vs SQUAD_OWNED file preservation is tested as a category, not piecemeal -> - **Security test blocks:** Any path-accepting flag triggers a dedicated security category with containment, traversal, and symlink tests -> - **Recovery tests:** Backup/restore/rollback paths must test partial failure scenarios (e.g., incomplete backup + forced rollback) - ---- - -## Conclusion - -**80% coverage is the floor.** But coverage β‰  completeness. Tests must organize around: -1. **Boundaries** (what changes, what doesn't) -2. **Contracts** (CLI command β†’ downstream lifecycle) -3. **Security** (paths, escapes, boundaries) -4. **Recovery** (partial failure is a feature; test it) - -The 5 bugs caught here would have shipped without peer review. Two of them (shell reinit, config.json loss) would hit production users. The migration test gaps are now obvious in hindsightβ€”but only because we wrote 57 tests and forced the team to think hard about what wasn't tested. - -Next command: organize tests by these categories from the start. diff --git a/.squad/decisions/inbox/keaton-migration-arch-review.md b/.squad/decisions/inbox/keaton-migration-arch-review.md deleted file mode 100644 index 9961c88ec..000000000 --- a/.squad/decisions/inbox/keaton-migration-arch-review.md +++ /dev/null @@ -1,111 +0,0 @@ -# Lessons from Migration Feature Work (Issue #197, PR #199) - -**By:** Keaton (Lead) -**Date:** 2026-03-06 -**Context:** Completed `squad migrate` command (57 tests, 8 pre-PR blockers caught by team review) - ---- - -## Lesson 1: File Ownership Model Must Be Explicit at Design Time - -**Problem Found:** -- `config.json` was not in `USER_OWNED`, so migration destroyed user customization -- `casting/registry.json` was marked `SQUAD_OWNED` (wiped), but `sdkInitSquad()` didn't recreate itβ€”stale state -- `.first-run` marker created by `sdkInitSquad()` was left behind post-migrate, causing shell to re-enter Init Mode - -**Root Cause:** -The migration command was designed without explicit categorization of what `.squad/` files belong to *system* vs. *user*. This was only discovered when testing the real upgrade path. - -**What We Do Now:** -- **Before implementation:** Any feature that touches state files (migrations, exports, imports, resets) must document file ownership explicitly in the proposal (`SQUAD_OWNED` vs. `USER_OWNED`). -- **Add to `.squad/` charter:** "File ownership is non-negotiable architectural context. If unclear, ask Keaton or Baer before coding." -- **Add to checklists:** Code review must include "verify all state files are categorized (SQUAD_OWNED/USER_OWNED)" before approving CLI state mutations. - ---- - -## Lesson 2: Security Boundaries Need Static Enforcement, Not Runtime Checks - -**Problem Found:** -- `--restore` and `--backup-dir` had no cwd containment guardβ€”path traversal was possible -- `copyRecursive()` restore targets weren't validatedβ€”could write outside `.squad/` -- These blockers came from Baer (Security) in pre-PR team review, not from code review - -**Root Cause:** -The migrate command implemented runtime path validation *after* path resolution, which could pass invalid paths through at critical moments. No guard to prevent `--restore ../../../etc`. - -**What We Do Now:** -- **New pattern for file operations:** All path flags (`--restore`, `--backup-dir`, etc.) must be validated *before* use: `if (!path.startsWith(cwd)) { throw SquadError }`. -- **Add to security checklist:** CLI commands touching the filesystem must be reviewed by Baer (or designated security owner) before merge. -- **Add to Baer's charter:** "All file I/O in CLI is security-relevant. Path validation non-negotiable." - ---- - -## Lesson 3: Correctness Invariants Must Be Tracked Explicitly - -**Problem Found:** -- `runMigrate()` could attempt rollback even if backup failedβ€”risking data loss -- The only guard was "hope the backup completed"β€”no `backupComplete` flag -- This blocker came from Fenster (Core Dev) in pre-PR review, not from tests - -**Root Cause:** -The code had an implicit assumption ("if we reach rollback, backup succeeded") but no enforcement. Test coverage was strong (57 tests) but didn't catch this logic error because happy paths always succeeded. - -**What We Do Now:** -- **New pattern for multi-step operations:** If a later step depends on an earlier step succeeding, use an explicit boolean flag (e.g., `backupComplete`). Document it in the code comment. -- **Add to code review checklist:** "Identify preconditions for rollback/cleanup/error paths. Verify they're enforced with explicit state, not implicit assumptions." -- **For next migration-like command:** Fenster reviews the error recovery paths before tests are written. - ---- - -## Lesson 4: Pre-PR Team Review Caught 8 Blockers That Code Review Missed - -**Problem Found:** -- Security (path traversal) β€” caught by Baer -- Correctness (backup incomplete rollback) β€” caught by Fenster -- Feature scope (config.json ownership) β€” caught by Keaton -- UX (sdkInitSquad internal naming, error styling) β€” caught by Marquez -- Docs (install command incorrect) β€” caught by McManus -- Plus 3 other fixes - -**Root Cause:** -Code review focused on *implementation correctness* (does the code do what it says?). Team review focused on *system correctness* (does this feature fit the architecture and user expectations?). - -**What We Do Now:** -- **Mandatory team review gate before PR for high-risk changes:** Features touching: - - State file mutations (migrations, exports, resets, backups) - - Security boundaries (path I/O, PII handling) - - Initialization/lifecycle (init, cleanup, re-entry guards) - - Should route to relevant agents *before* PR goes up, in a comment: "Ready for pre-PR team review" β†’ Baer, Fenster, Keaton, Marquez -- **Add to routing.md:** High-risk features get async pre-PR feedback from domain owners (security β†’ Baer, core runtime β†’ Fenster, arch β†’ Keaton, UX β†’ Marquez). -- **Document this in CONTRIBUTING.md:** "For features marked as 'high-risk' (migrations, security, lifecycle), request pre-PR feedback in an issue comment before opening the PR." - ---- - -## Lesson 5: PR Target Branch Is a Process Failure Waiting to Happen - -**Problem Found:** -- PR #199 was initially opened against `main` instead of `dev` (per CONTRIBUTING.md, features go to `dev`) -- Caught during PR creation, not during code review -- Small mistake, but revealed no automated enforcement - -**Root Cause:** -CONTRIBUTING.md documents the rule but GitHub/Git don't enforce it at push/PR-create time. - -**What We Do Now:** -- **Add branch protection rule:** Base branch for squad feature PRs must be `dev`, not `main`. Enforce this at GitHub level with branch protection rules. -- **If GitHub enforcement isn't available:** Add a pre-push hook that rejects direct pushes to `main` unless the commit includes "Closes #X" (release workflow exemption). -- **Document in CONTRIBUTING.md:** "Feature branches target `dev`. Release branches target `main`. If you see a PR targeting wrong branch, request change before review." - ---- - -## Summary: Architecture β†’ Process β†’ Codification - -| Gap | Addressed By | Now Codified In | -|-----|--------------|-----------------| -| File ownership implicit | Lesson 1 | `.squad/` charter + code review checklist | -| Path validation ad-hoc | Lesson 2 | Security checklist + Baer charter | -| Correctness assumptions unmeasured | Lesson 3 | Code review checklist + Fenster review pattern | -| Team review happened too late | Lesson 4 | Routing.md + CONTRIBUTING.md (pre-PR gate) | -| Branch target not enforced | Lesson 5 | GitHub branch protection rules | - -**Captured for:** All future migration-like features, high-risk CLI work, and state-mutation commands. diff --git a/.squad/decisions/inbox/waingro-destructive-qa-discipline.md b/.squad/decisions/inbox/waingro-destructive-qa-discipline.md deleted file mode 100644 index fa5690ca6..000000000 --- a/.squad/decisions/inbox/waingro-destructive-qa-discipline.md +++ /dev/null @@ -1,108 +0,0 @@ -# QA Discipline: Destructive Commands Need Adversarial Testing at Design Time - -**Author:** Waingro (Product Dogfooder) -**Date:** 2026-03-04 -**Status:** Proposal (awaiting Scribe merge) -**Triggered by:** `squad migrate` post-user-testing bugs - ---- - -## Context - -The `squad migrate` command had a pre-PR team review (9 agents) that caught **2/4 critical bugs**. Post-user testing found **2 additional bugs**: - -| Bug | Severity | Review | User | Root Cause | -|-----|----------|--------|------|-----------| -| `casting/registry.json` wiped, not recreated | P0 | ❌ | βœ… | SDK skips registry.json; logic gap in migrate | -| `.first-run` marker β†’ re-entry to Init Mode | P1 | ❌ | βœ… | Incomplete marker cleanup | -| `config.json` destroyed (SQUAD_OWNED not USER_OWNED) | P0 | βœ… | N/A | File ownership violation | -| `--restore` path traversal (no cwd containment) | P0 | βœ… | N/A | Missing validation | - -This reflects a **QA discipline gap**: code review catches logical and security issues, but destructive operations need adversarial testing for state contamination, version compatibility, and edge cases. - ---- - -## Decision - -**For any CLI command that deletes, moves, or overwrites files, the implementation must include:** - -1. **File Ownership Matrix** (explicit, non-overlapping) - - SQUAD_OWNED files (regenerated fresh) vs USER_OWNED files (preserved) - - No overlap; documented at code level - -2. **Gherkin Scenario Matrix** (written at design time) - - File ownership enforcement scenarios - - Version-specific regression scenarios (β‰₯2 prior major versions) - - State contamination scenarios (.first-run, shell markers) - - Dangerous input scenarios (path traversal, symlinks) - - Partial failure & rollback scenarios - - Idempotence scenarios - -3. **Pre-Flight Checklist** (in PR template for destructive commands) - - [ ] File ownership matrix explicit & non-overlapping - - [ ] Rollback path: error during mutation β†’ restore from backup - - [ ] Marker cleanup: .first-run, .init-prompt, shell state removed - - [ ] Path validation: containment checks (startsWith cwd) - - [ ] Version compat: test β‰₯2 prior major versions - - [ ] Idempotence: safe to run twice - - [ ] Dry-run: pixel-perfect, no mutation - - [ ] Partial failure: can roll back mid-operation - - [ ] User communication: next steps clear - - [ ] Adversarial inputs: path traversal, symlinks, collisions - -4. **Shell State Tests** (dedicated test category) - - Any command touching `.squad/.first-run` or markers - - Verify shell would NOT re-enter Init Mode post-migration - - Verify markers are not created during reinit - -5. **Version-Specific Fixtures** (test against known cohorts) - - v0.5.x: no casting/, minimal agents - - v0.8.x: casting/ present, full registry - - Current: up-to-date schema - - Test each variant in isolation - ---- - -## Examples - -### Gherkin Scenarios for `squad migrate` - -```gherkin -Scenario: Migrate handles repos with no casting state (v0.5.x) - Given a .squad/ from v0.5.x (no casting/ directory) - When running 'squad migrate' - Then registry.json is created from agents/ - And casting/policy.json and casting/history.json are initialized - -Scenario: Migrate removes init-mode markers - Given a migrated repo with agent roster - When 'squad migrate' completes - Then .squad/.first-run does NOT exist - And shell does not re-enter Init Mode - -Scenario: --restore rejects path traversal attempts - When running 'squad migrate --restore ../../etc/passwd' - Then exit code is 1 - And no files outside cwd are accessed -``` - -### Pre-Flight Checklist in PR - -When reviewing a destructive command, the reviewer should verify all 10 items before approval. Include this checklist in the PR template. - ---- - -## Impact - -- **Code review focus:** syntax, logic, security (what reviewers are good at) -- **Adversarial QA focus:** state contamination, version compat, edge cases (what QA is good at) -- **Result:** destructive commands get both disciplines instead of one alone - ---- - -## Implementation - -1. Scribe: merge this decision -2. Keaton/Fenster: update PR template to include destructive command checklist -3. Waingro: add "destructive command" tag to issue templates -4. Squad: on next destructive command PR, enforce the checklist From 8f6054ce2f3199bdb83654e350aaaa6398cae733 Mon Sep 17 00:00:00 2001 From: williamhallatt Date: Thu, 5 Mar 2026 20:58:57 +1000 Subject: [PATCH 6/7] chore(squad): clean session metadata from history files Remove ephemeral session attribution (requester usernames, branch names) from agent history.md files. History should capture reusable knowledge, not session metadata. Also clarifies history.md writing guidance in squad.agent.md spawn template to prevent recurrence. --- .github/agents/squad.agent.md | 4 +- .squad/agents/fenster/history.md | 38 +++++++++++-- .squad/agents/hockney/history.md | 2 +- .squad/agents/keaton/history.md | 48 +++++++++++++++- .squad/agents/kobayashi/history.md | 91 ++++++++++++++++++++++++++++++ 5 files changed, 176 insertions(+), 7 deletions(-) diff --git a/.github/agents/squad.agent.md b/.github/agents/squad.agent.md index 2c416dec4..e5aaac4ef 100644 --- a/.github/agents/squad.agent.md +++ b/.github/agents/squad.agent.md @@ -662,7 +662,9 @@ prompt: | AFTER work: 1. APPEND to .squad/agents/{name}/history.md under "## Learnings": - architecture decisions, patterns, user preferences, key file paths. + architecture decisions, reusable patterns, key file paths, API behaviors, team conventions. + ⚠️ DO NOT record: requester names, branch names, session metadata, or one-time task context. + History is for knowledge that will be useful in FUTURE sessions, not session attribution. 2. If you made a team-relevant decision, write to: .squad/decisions/inbox/{name}-{brief-slug}.md 3. SKILL EXTRACTION: If you found a reusable pattern, write/update diff --git a/.squad/agents/fenster/history.md b/.squad/agents/fenster/history.md index 30e6261e9..923f4ae1c 100644 --- a/.squad/agents/fenster/history.md +++ b/.squad/agents/fenster/history.md @@ -581,11 +581,9 @@ The CLI couldn't run because `packages/squad-sdk/src/index.ts` was missing re-ex --- -### 2026-03-05: Workflow Filter Implementation Review (PR williamhallatt/201) +### 2026-03-05: Workflow Filter Implementation Review (PR #201) -**Requested by:** williamhallatt (validation review of completed PR) - -**Context:** Branch `williamhallatt/201-investigate-actions-install` changed `packages/squad-sdk/src/config/init.ts` to filter workflow installation to only Squad-framework workflows (4 files) instead of copying all workflows from templates/. +**Context:** Issue #201 changed `packages/squad-sdk/src/config/init.ts` to filter workflow installation to only Squad-framework workflows (4 files) instead of copying all workflows from templates/. **Changes reviewed:** - Added `FRAMEWORK_WORKFLOWS` constant (4 filenames: squad-heartbeat.yml, squad-issue-assign.yml, squad-triage.yml, sync-squad-labels.yml) @@ -616,3 +614,35 @@ The CLI couldn't run because `packages/squad-sdk/src/index.ts` was missing re-ex - Workflow installation in init.ts is self-contained in the `includeWorkflows` block β€” SDK layer controls filtering, CLI layer only gates the feature on/off πŸ“Œ Team update (2026-03-05T10-35-50Z): PR #201 workflow filter approved by all reviewers β€” framework/scaffolding distinction, implementation pattern validated, test coverage noted β€” decided by Keaton, Fenster, Hockney, Edie + +## 2026-03-05: PR #201 Implementation Review + +**Task:** PR readiness review for issue #201 workflow filtering implementation. + +**Implementation verified:** + +βœ… **FRAMEWORK_WORKFLOWS constant** (lines 446-451 in init.ts): + - Correctly placed at module scope, well-commented + - Contains exactly 4 framework workflows: heartbeat, issue-assign, triage, sync-labels + - Declaration before `initSquad()` function β€” discoverable and maintainable + +βœ… **Filter logic** (lines 817-818): + - Two-stage filter: `allWorkflowFiles` (all .yml) β†’ `workflowFiles` (framework only) + - Variable naming is clear and intentional + - Pattern: `allWorkflowFiles.filter(f => FRAMEWORK_WORKFLOWS.includes(f))` + +βœ… **Edge case handling**: + - If FRAMEWORK_WORKFLOWS file doesn't exist in templates/: silently skipped (operates on intersection) + - This is acceptable β€” missing templates won't crash init, just fewer workflows installed + - Could log warning in future enhancement, but not a blocker + +βœ… **includeWorkflows: true** confirmed in CLI init.ts (line 114) β€” no bypass paths + +βœ… **Upgrade gap acknowledged**: + - templates.ts shows all 12 workflows in manifest + - Upgrade command copies all 12 (lines 413-420 in upgrade.ts) + - Acceptable to leave for follow-up β€” existing projects already have workflows + +βœ… **No other copy sites** β€” grep confirms workflow copying only in init.ts (SDK) and upgrade.ts (CLI) + +**Verdict:** Implementation is solid. No concerns. diff --git a/.squad/agents/hockney/history.md b/.squad/agents/hockney/history.md index cea2756e8..7a8c1c4f1 100644 --- a/.squad/agents/hockney/history.md +++ b/.squad/agents/hockney/history.md @@ -805,7 +805,7 @@ All labeled squad:hockney for routing. Each issue includes: what's missing, why - **Lesson:** On Windows, use `join(root, '.squad', 'templates')` not `join(root, '.squad/templates')` β€” forward-slash segments in `join` args work on Node but it's better practice to use separate args. ### Workflow filtering test coverage review β€” Issue #201 (2026-03-04) -**Status:** Complete β€” validated williamhallatt/201-investigate-actions-install branch. +**Status:** Complete β€” validated issue #201 workflow filter branch. **Context:** Only 4 FRAMEWORK_WORKFLOWS now installed by `squad init` (heartbeat, triage, issue-assign, sync-labels). 8 CI/CD workflows (ci, preview, release, docs, insider-release, label-enforce, main-guard, promote) NOT installed until `squad upgrade`. **Test files reviewed:** 1. `test/workflows.test.js` (CJS, NOT run by vitest β€” only used for manual testing) diff --git a/.squad/agents/keaton/history.md b/.squad/agents/keaton/history.md index 61efc464f..a0d1be324 100644 --- a/.squad/agents/keaton/history.md +++ b/.squad/agents/keaton/history.md @@ -28,6 +28,52 @@ ## Learnings +### 2026-03-05T[NOW]: PR #201 Readiness Review β€” Workflow Filter Architecture +- **Task:** PR readiness review for issue #201 (Squad CI/CD workflows leaking into user repos). Core fix: `FRAMEWORK_WORKFLOWS` constant at module scope in `packages/squad-sdk/src/config/init.ts`, filtering workflow copy loop to only 4 framework files. +- **Commits reviewed:** + - ddc4f6a: fix(sdk): only install Squad-framework workflows during init + - da84b90: test(workflows): update expectations to match framework-only install + - 080d6b4: docs(ai-team): Merge PR #201 workflow filter reviews + - 0512cc4: test(cli): assert exact workflow names installed/excluded by init + - abdec4b: docs(ai-team): merged testing & QA discipline decisions (LOCAL ONLY) +- **Architecture verdict:** βœ… **SOUND.** Constant placement at module scope (before `initSquad()`) is correct β€” no runtime overhead, clear scope, self-documenting. Filter placement (line 818) is the right choke point: early read + filter pattern (`allWorkflowFiles.filter(f => FRAMEWORK_WORKFLOWS.includes(f))`), no mutation, no side effects. Alternative considered: external config file (overengineered for 4 static names). `FRAMEWORK_WORKFLOWS` is semantic, not mechanical β€” choosing correct names for user repos is a business rule, belongs in code. +- **Code quality verdict:** βœ… **MERGE-READY.** Comment is excellent ("Other workflows in templates/workflows/ are generic CI/CD scaffolding and are opt-in"). Test coverage is complete: `test/cli/init.test.ts` has both positive list (4 framework workflows asserted to exist) and negative list (8 CI/CD workflows asserted NOT to exist). No edge cases missed (all 12 templates accounted for). Commit message quality high (explains why, not just what). No blocker-level issues. +- **Test verdict:** βœ… **COMPREHENSIVE.** Test changes span 2 commits: + - da84b90: Updated existing test expectations from "all 12 workflows" to "4 framework workflows" + - 0512cc4: Added explicit assertions for excluded workflows (hardened negative case) + - Lines 129–145 (init.test.ts): exact filenames checked, no magic numbers + - Lines 147–166 (init.test.ts): exclusion list hardened (would catch accidental leakage) + - Test philosophy correct: tests verify THE INTENT (only framework workflows), not "whatever is currently installed". +- **Concerns:** ⚠️ **ONE UPGRADE GAP (non-blocking).** Users on Squad v0.5.4–v0.8.20 already have all 12 workflows installed. `squad init` fixes this for NEW repos, but `squad upgrade` doesn't retroactively remove CI/CD workflows from existing repos. Not a blocker (old workflows are inert if not triggered), but worth noting in PR description: "Existing repos with Squad CI/CD workflows must manually remove them β€” `squad upgrade` does NOT clean old workflows." +- **PR title:** `fix(sdk): only install Squad framework workflows during init (4 of 12)` β€” Scope correct (SDK owns init.ts), type correct (fix for unwanted behavior), suffix adds context. +- **PR description template:** + ``` + Closes #201 + + Squad's own CI/CD workflows (squad-ci, squad-release, squad-docs, etc.) hardcode Squad's branch strategy and Node.js toolchain. They were being copied to user repos on `squad init`, where they have no business running. + + ### Changes + - Added `FRAMEWORK_WORKFLOWS` constant listing 4 files that power Squad features + - Filtered workflow copy loop to only install framework workflows + - Updated tests to assert exact workflow list (both installed and excluded) + + ### Framework workflows (installed): + - squad-heartbeat.yml + - squad-issue-assign.yml + - squad-triage.yml + - sync-squad-labels.yml + + ### CI/CD workflows (no longer installed): + - squad-ci, squad-release, squad-docs, squad-insider-release, squad-preview, squad-promote, squad-main-guard, squad-label-enforce + + ### Known limitation + Existing repos that already have Squad CI/CD workflows will NOT have them removed by `squad upgrade`. Manual cleanup required if users want to remove them. + ``` +- **Local commit (abdec4b) verdict:** ⚠️ **PUSH SEPARATELY, DO NOT INCLUDE IN PR #201.** Commit merges Hockney + Waingro testing decisions into `.squad/decisions.md` (278 lines added). This is UNRELATED to workflow filtering β€” it's Scribe orchestration work from a parallel session. Including it in PR #201 dilutes the PR scope (one fix, one concern). Recommendation: cherry-pick to a separate branch (`squad/scribe-decisions-merge-2026-03-05`) and open separate PR, or push to `dev` directly if team convention allows docs-only commits without PR. +- **Unstaged package.json changes verdict:** ❌ **DO NOT COMMIT IN THIS PR.** Version bumps (0.8.21-preview.1 β†’ 0.8.21-preview.2) are unrelated to workflow filtering. Likely William's local dev environment preparing for a preview release. Should either (1) be committed in a separate "chore: bump preview version" commit, or (2) be stashed/reset before opening PR #201. Including them pollutes PR diff and creates merge conflicts when Brady bumps versions independently. +- **Final verdict:** βœ… **APPROVED FOR MERGE TO DEV.** No blockers. Architecture is clean, tests are comprehensive, commit quality is high. Two actions before opening PR: (1) Handle local commit separately (push to different branch or drop), (2) Reset unstaged package.json changes (`git checkout HEAD -- package.json packages/*/package.json`). +- **Pattern learned:** PR readiness reviews need THREE dimensions: (1) Architecture correctness (is the solution structurally sound?), (2) Scope hygiene (are unrelated commits included?), (3) Upgrade impact (does the fix leave old users in a broken state?). For architectural fixes, "does it solve the problem?" is necessary but not sufficient β€” also ask "does it leave the next developer a clear path?" The `FRAMEWORK_WORKFLOWS` constant is a gift to future maintainers: when Squad adds a 5th framework workflow, they'll know exactly where to update. + ### 2026-03-05T[NOW]: Read-Only PR Assessment β€” 4 Open PRs (#182, #185, #178, #131) - **Task:** Brady requested read-only assessment of 4 open PRs (all except #189 which Tamir is still working on). Scope: Summary, Quality, Compatibility with recent main, Recommended action. - **Context:** Recent main (HEAD: 1dc3bb6) shows major TS replatform: migration from `index.js` (deprecated) to `packages/squad-cli/dist/cli-entry.ts` (new architecture). doctor command already wired, nap command restored, migration docs clarified. This creates compatibility assessment challenge: PRs based on old JS architecture may conflict. @@ -706,7 +752,7 @@ This history accurately documents Keaton's work and decisions. Future spawns can ### 2026-03-05T20:50:00Z: Issue #201 Fix Review β€” Workflow Install Filter Architecture - **Task:** Architectural review of PR fixing issue #201 (`squad init` was copying ALL 12 workflows including Squad's own CI/CD pipeline). PR introduces `FRAMEWORK_WORKFLOWS` constant to filter init to 4 framework workflows only. Two files changed: init.ts (filter logic), workflows.test.js (test expectations). -- **Context:** William Hallatt implemented fix on branch `williamhallatt/201-investigate-actions-install`. Tests pass. Issue reported that users were getting squad-ci.yml, squad-release.yml, squad-preview.yml, etc. β€” workflows meant for Squad's own repository, not user repos. Fix distinguishes "framework workflows" (always installed) from "CI/CD scaffolding" (opt-in via upgrade). +- **Context:** Fix for issue #201 addresses the problem that users were getting squad-ci.yml, squad-release.yml, squad-preview.yml, etc. β€” workflows meant for Squad's own repository, not user repos. Fix distinguishes "framework workflows" (always installed) from "CI/CD scaffolding" (opt-in via upgrade). - **Review Findings:** 1. **βœ… FRAMEWORK_WORKFLOWS classification is correct.** Four files in the constant: - `squad-heartbeat.yml` (Ralph's automated status updates) diff --git a/.squad/agents/kobayashi/history.md b/.squad/agents/kobayashi/history.md index 2e754e532..2d9e1f220 100644 --- a/.squad/agents/kobayashi/history.md +++ b/.squad/agents/kobayashi/history.md @@ -928,3 +928,94 @@ Branch infrastructure now matches 3-branch model: **State is clean, reversible, and ready for Brady's return.** + +### 2026-03-05: PR Readiness Review β€” Issue #201 Workflow Filter + +**Task:** Full pre-PR checklist (CONTRIBUTING.md compliance, branch naming, commit state, version bumps, PR target, PR template) + +#### Checklist Results + +**1. CONTRIBUTING.md Compliance βœ…** +- PR process (push branch, create PR, link issue, wait CI): All steps applicable +- Co-authored-by trailer required: This is a human contributor (williamhallatt), not a squad agent β€” trailer only required if Copilot participates in commits (see CONTRIBUTING.md lines 110-113: "The Co-authored-by trailer is **required** for all commits (added by Copilot CLI)"). Since Copilot didn't author these commits, trailer NOT required +- Branch naming (CONTRIBUTING.md lines 78-91): For "user-facing work, use user_name/issue-number-slug format" β€” `williamhallatt/201-investigate-actions-install` βœ… compliant +- Changeset requirement (line 226): `npm run changeset:check` must pass β€” reviewer will verify +- Base branch: CONTRIBUTING.md specifies main/dev/insiders (lines 212-216). PR target is `bradygaster/dev` (Brady's integration branch) β€” ⚠️ unconventional but acceptable for integration work per team practice + +**2. Branch Naming Convention βœ…** +- CONTRIBUTING.md line 83: Correct format for human contributor user_name/issue-number-slug for issue #201 +- Previous convention `squad/{issue}` (line 89) applies to agents only + +**3. Local-Only Commit Assessment βœ… SHOULD PUSH** +- Commit: `abdec4b docs(ai-team): merged testing & QA discipline decisions` +- Status: Testing & QA discipline decisions merged by Scribe into .squad/decisions.md +- Classification: .squad/ state file (merge-driver protected per team decisions, append-only) +- Recommendation: **PUSH this commit before opening PR** + - Reason 1: It's related to test discipline patterns (fits issue context β€” workflow filtering tests) + - Reason 2: .squad/decisions.md changes are non-conflicting (merge=union driver) + - Reason 3: Keeps PR description accurate (only code + state changes, no artificial splitting) + +**4. Unstaged Package.json Version Bumps βœ… STASH/REVERT** +- Files: package.json, packages/squad-cli/package.json, packages/squad-sdk/package.json +- Change: 0.8.21-preview.1 β†’ 0.8.21-preview.2 (NOT part of issue #201 fix) +- Action: **STASH or REVERT these changes** + - Reason: Version bumps are release engineering decisions, not part of the #201 fix + - Guideline: Per CONTRIBUTING.md line 166-167, versioning is controlled by changesets β€” manual bumps are out of scope + - Cleanup: After PR, these can be re-applied if needed for local dev + +**5. PR Target Branch βœ… ACCEPTABLE** +- Current: `bradygaster/dev` (Brady's integration branch) +- CONTRIBUTING.md baseline: main/dev/insiders +- Status: `bradygaster/dev` is Brady's personal integration branch, used for testing before merging to main branch +- Assessment: Not the formal `dev` branch, but acceptable for feature validation. Brady will merge to dev/main when ready +- Recommendation: **Use `bradygaster/dev` as target** (matches Brady's workflow) + +#### Recommended Pre-Push Sequence +1. Stash version bumps: `git stash push -m "version preview.2 bump (not part of PR)"` (or `git checkout -- package.json packages/*/package.json`) +3. Verify clean state: `git status` (should show "nothing to commit") +4. Open PR with command (see below) + +#### PR Creation Command + +```bash +gh pr create \ + --base bradygaster/dev \ + --title "fix: only install Squad-framework workflows during init" \ + --body "Closes #201 + +## Summary +Restricts workflow installation in \`squad init\` to only Squad-framework workflows (4 files: squad-heartbeat.yml, squad-issue-assign.yml, squad-triage.yml, sync-squad-labels.yml) instead of copying all workflows from the templates directory. + +## Changes +- Added \`FRAMEWORK_WORKFLOWS\` constant to filter workflows +- Updated \`initSquad()\` to filter workflow installation +- Tests updated to validate framework workflows installed and CI/CD workflows excluded + +## Notes +- All agents approved PR #201 (Keaton, Fenster, Hockney, Edie) +- CI/CD workflows now excluded from init (users must configure separately if needed) +- Addresses user experience gap: prevents workflow installation bloat on init +" \ + --label "area/init,type/fix,squad:williamhallatt" +``` + +#### Final State Assessment +- **Commits:** 5 pushed + 1 local (abdec4b will be 6 pushed after `git push origin`) +- **Test status:** Workflow tests updated and passing +- **State integrity:** .squad/ changes merge-safe (append-only, union driver), no corruption risk +- **Build readiness:** `npm run build`, `npm test`, `npm run lint` expected to pass (validated by agents) + +**Outcome:** Branch is PR-ready pending: +1. Push local commit (abdec4b) +2. Stash/revert version bumps +3. Run `gh pr create` with command above + +**Zero state corruption risk.** All PR process steps align with CONTRIBUTING.md and team conventions. + +## Learnings + +- Human contributor branch naming: user_name/issue-slug (not squad/{issue}) +- .squad/ state merges are safe with merge=union driver β€” push early, no conflicts +- Version bumps not part of feature PR β€” revert if accidental, changesets handle release versioning +- PR target can be Brady's integration branch (bradygaster/dev) not just main/dev/insiders β€” confirms Brady's workflow practice +- Co-authored-by trailer only required when Copilot actually authors commits (not for human contributors) From e62d18f8da8410e0081253e0741becea3ea0db8e Mon Sep 17 00:00:00 2001 From: williamhallatt Date: Thu, 5 Mar 2026 21:03:01 +1000 Subject: [PATCH 7/7] chore: ignore .squad/config.json (contains machine-local teamRoot path) .squad/config.json is written by 'squad link' and 'squad init --remote' for remote team mode. The teamRoot field is an absolute local path and must not be committed. Add to .gitignore to prevent accidental inclusion. --- .gitignore | 1 + .squad/config.json | 4 ---- 2 files changed, 1 insertion(+), 4 deletions(-) delete mode 100644 .squad/config.json diff --git a/.gitignore b/.gitignore index 862a7cfa5..0856ac704 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,7 @@ coverage/ # Squad: ignore generated logs .squad/orchestration-log/ .squad/log/ +.squad/config.json .test-cli-* # Docs site generated files docs/dist/ diff --git a/.squad/config.json b/.squad/config.json deleted file mode 100644 index a1f09ba0a..000000000 --- a/.squad/config.json +++ /dev/null @@ -1,4 +0,0 @@ -{ - "version": 1, - "teamRoot": "/home/williamh/code/squad" -} \ No newline at end of file