Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/agents/squad.agent.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -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/
1 change: 1 addition & 0 deletions .squad/.first-run
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
2026-03-05T09:10:14.302Z
13 changes: 13 additions & 0 deletions .squad/agents/edie/history.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
68 changes: 68 additions & 0 deletions .squad/agents/fenster/history.md
Original file line number Diff line number Diff line change
Expand Up @@ -578,3 +578,71 @@ 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 #201)

**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)
- 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

## 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.
60 changes: 60 additions & 0 deletions .squad/agents/hockney/history.md
Original file line number Diff line number Diff line change
Expand Up @@ -803,3 +803,63 @@ 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 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)
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

## 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.
Loading
Loading