diff --git a/.changeset/audit-onboarding-skill-guards.md b/.changeset/audit-onboarding-skill-guards.md new file mode 100644 index 000000000..a9bff2a99 --- /dev/null +++ b/.changeset/audit-onboarding-skill-guards.md @@ -0,0 +1,5 @@ +--- +'@bradygaster/squad-cli': patch +--- + +Add defensive guards from architecture audit: (1) verify roster is populated after team creation before dispatching to coordinator, preventing empty-roster dispatch loops; (2) warn when `squad upgrade` overwrites customized built-in skills diff --git a/packages/squad-cli/src/cli/core/upgrade.ts b/packages/squad-cli/src/cli/core/upgrade.ts index 59e64c615..975fb8f98 100644 --- a/packages/squad-cli/src/cli/core/upgrade.ts +++ b/packages/squad-cli/src/cli/core/upgrade.ts @@ -431,6 +431,20 @@ export function ensureCastingDefaults(dest: string, templatesDir?: string): stri return created; } +/** + * Warn when a built-in skill has been customized and is about to be overwritten. + * Normalizes line endings before comparison to avoid false positives from CRLF differences. + */ +function warnIfSkillCustomized(srcPath: string, destPath: string, sourceName: string): void { + if (!storage.existsSync(destPath)) return; + const existing = (storage.readSync(destPath) ?? '').replace(/\r\n/g, '\n'); + const template = (storage.readSync(srcPath) ?? '').replace(/\r\n/g, '\n'); + if (existing && existing !== template) { + const skillName = sourceName.split('/')[1] ?? sourceName; + warn(`Skill '${skillName}' has been customized — overwriting with built-in version`); + } +} + /** * Sync manifest-declared skills to .copilot/skills/, respecting overwriteOnUpgrade. * Only skills listed in TEMPLATE_MANIFEST are installed — not the entire templates/skills/ dir. @@ -448,6 +462,7 @@ function syncAllSkills(dest: string, templatesDir: string): number { if (!storage.existsSync(srcPath)) continue; if (!entry.overwriteOnUpgrade && storage.existsSync(destPath)) continue; + warnIfSkillCustomized(srcPath, destPath, entry.source); storage.mkdirSync(path.dirname(destPath), { recursive: true }); storage.copySync(srcPath, destPath); synced++; @@ -614,6 +629,9 @@ export async function runUpgrade(dest: string, options: UpgradeOptions = {}): Pr if (!storage.existsSync(srcPath)) continue; + if (file.source.startsWith('skills/')) { + warnIfSkillCustomized(srcPath, destPath, file.source); + } storage.mkdirSync(path.dirname(destPath), { recursive: true }); storage.copySync(srcPath, destPath); diff --git a/packages/squad-cli/src/cli/shell/index.ts b/packages/squad-cli/src/cli/shell/index.ts index ddff37d91..d45e878aa 100644 --- a/packages/squad-cli/src/cli/shell/index.ts +++ b/packages/squad-cli/src/cli/shell/index.ts @@ -986,6 +986,26 @@ export async function runShell(): Promise { files: result.filesCreated.length, }); + // Production guard: verify roster was actually populated before proceeding + const verifyTeamPath = join(teamRoot, '.squad', 'team.md'); + const verifyContent = storage.readSync(verifyTeamPath) ?? ''; + if (!hasRosterEntries(verifyContent)) { + debugLog('finalizeCast: roster empty after createTeam — aborting dispatch'); + // Clean up .init-prompt to prevent auto-retry loop on next startup + const initPromptCleanup = join(teamRoot, '.squad', '.init-prompt'); + if (storage.existsSync(initPromptCleanup)) { + try { await storage.delete(initPromptCleanup); } catch { /* ignore */ } + } + shellApi?.addMessage({ + role: 'system', + content: '⚠ Team creation completed but roster is empty — skipping dispatch. Check .squad/team.md.', + timestamp: new Date(), + }); + shellApi?.setActivityHint(undefined); + shellApi?.setProcessing(false); + return; + } + shellApi?.addMessage({ role: 'system', content: `✅ Team hired! ${result.membersCreated.length} members created.`, diff --git a/test/cli/upgrade.test.ts b/test/cli/upgrade.test.ts index 4a040e162..a5282f258 100644 --- a/test/cli/upgrade.test.ts +++ b/test/cli/upgrade.test.ts @@ -417,4 +417,103 @@ describe('CLI: upgrade command', () => { expect(existsSync(join(castingDir, 'policy.json'))).toBe(true); expect(existsSync(join(castingDir, 'history.json'))).toBe(true); }); + + /* ── warnIfSkillCustomized ──────────────────────────────────── */ + + it('warnIfSkillCustomized warns when a skill has been modified', async () => { + const agentPath = join(TEST_ROOT, '.github', 'agents', 'squad.agent.md'); + const skillPath = join(TEST_ROOT, '.copilot', 'skills', 'squad-conventions', 'SKILL.md'); + expect(existsSync(skillPath)).toBe(true); + + // Simulate old version so upgrade goes through the full manifest path + let agentContent = readFileSync(agentPath, 'utf8'); + agentContent = agentContent.replace(//m, ''); + writeFileSync(agentPath, agentContent); + + // Modify the skill to simulate user customization + const original = readFileSync(skillPath, 'utf8'); + writeFileSync(skillPath, original + '\n## My Custom Section\nCustom content here.\n'); + + const spy = vi.spyOn(console, 'log').mockImplementation(() => {}); + try { + await runUpgrade(TEST_ROOT); + const calls = spy.mock.calls.map(c => String(c[0])); + expect(calls.some(c => c.includes('has been customized'))).toBe(true); + } finally { + spy.mockRestore(); + } + }); + + it('warnIfSkillCustomized does NOT warn for CRLF-only differences', async () => { + const agentPath = join(TEST_ROOT, '.github', 'agents', 'squad.agent.md'); + const skillPath = join(TEST_ROOT, '.copilot', 'skills', 'squad-conventions', 'SKILL.md'); + if (!existsSync(skillPath)) { + await runUpgrade(TEST_ROOT); + } + + // Simulate old version to go through full manifest path + let agentContent = readFileSync(agentPath, 'utf8'); + agentContent = agentContent.replace(//m, ''); + writeFileSync(agentPath, agentContent); + + // Re-write content with CRLF line endings (no semantic change) + const original = readFileSync(skillPath, 'utf8'); + const crlf = original.replace(/\r?\n/g, '\r\n'); + writeFileSync(skillPath, crlf); + + const spy = vi.spyOn(console, 'log').mockImplementation(() => {}); + try { + await runUpgrade(TEST_ROOT); + const calls = spy.mock.calls.map(c => String(c[0])); + expect(calls.some(c => c.includes('has been customized'))).toBe(false); + } finally { + spy.mockRestore(); + } + }); + + it('warnIfSkillCustomized does NOT warn for identical content', async () => { + const agentPath = join(TEST_ROOT, '.github', 'agents', 'squad.agent.md'); + + // Simulate old version to go through full manifest path + let agentContent = readFileSync(agentPath, 'utf8'); + agentContent = agentContent.replace(//m, ''); + writeFileSync(agentPath, agentContent); + + // After init, skill content should match the template exactly — no customization + const spy = vi.spyOn(console, 'log').mockImplementation(() => {}); + try { + await runUpgrade(TEST_ROOT); + const calls = spy.mock.calls.map(c => String(c[0])); + expect(calls.some(c => c.includes('has been customized'))).toBe(false); + } finally { + spy.mockRestore(); + } + }); + + it('warnIfSkillCustomized warns during full version upgrade path', async () => { + const agentPath = join(TEST_ROOT, '.github', 'agents', 'squad.agent.md'); + const skillPath = join(TEST_ROOT, '.copilot', 'skills', 'squad-conventions', 'SKILL.md'); + if (!existsSync(skillPath)) { + await runUpgrade(TEST_ROOT); + } + + // Simulate old version to force full upgrade path + let content = readFileSync(agentPath, 'utf8'); + content = content.replace(//m, ''); + writeFileSync(agentPath, content); + + // Modify skill to simulate customization + const original = readFileSync(skillPath, 'utf8'); + writeFileSync(skillPath, original + '\n## Custom Addition\n'); + + const spy = vi.spyOn(console, 'log').mockImplementation(() => {}); + try { + await runUpgrade(TEST_ROOT, { force: true }); + const calls = spy.mock.calls.map(c => String(c[0])); + const customizedWarnings = calls.filter(c => c.includes('has been customized')); + expect(customizedWarnings.length).toBeGreaterThanOrEqual(1); + } finally { + spy.mockRestore(); + } + }); }); diff --git a/test/init-autocast.test.ts b/test/init-autocast.test.ts index e46e1547b..ac6174dad 100644 --- a/test/init-autocast.test.ts +++ b/test/init-autocast.test.ts @@ -276,6 +276,79 @@ describe('orphan .init-prompt cleanup', () => { }); }); +// =========================================================================== +// 3b. finalizeCast empty-roster guard (PR #867) +// =========================================================================== + +describe('finalizeCast: empty-roster guard prevents dispatch loop', () => { + let tmpDir: string; + let squadDir: string; + + beforeEach(() => { + tmpDir = makeTempDir('finalize-guard-'); + squadDir = path.join(tmpDir, '.squad'); + fs.mkdirSync(squadDir, { recursive: true }); + }); + + afterEach(() => { + cleanDir(tmpDir); + }); + + it('empty roster after createTeam cleans up .init-prompt and aborts', () => { + // Simulate: createTeam wrote a team.md with empty roster + .init-prompt exists + fs.writeFileSync(path.join(squadDir, 'team.md'), makeEmptyRosterTeamMd()); + fs.writeFileSync(path.join(squadDir, '.init-prompt'), 'Build a snake game'); + + const teamContent = fs.readFileSync(path.join(squadDir, 'team.md'), 'utf-8'); + + // Replicate the finalizeCast empty-roster guard from shell/index.ts + if (!hasRosterEntries(teamContent)) { + const initPromptPath = path.join(squadDir, '.init-prompt'); + if (fs.existsSync(initPromptPath)) { + try { fs.unlinkSync(initPromptPath); } catch { /* ignore */ } + } + // Guard fires: early return, no dispatch + } + + // .init-prompt must be cleaned up to prevent auto-retry loop + expect(fs.existsSync(path.join(squadDir, '.init-prompt'))).toBe(false); + // Roster is still empty — dispatch should NOT have happened + expect(hasRosterEntries(teamContent)).toBe(false); + }); + + it('populated roster after createTeam does NOT trigger guard', () => { + fs.writeFileSync( + path.join(squadDir, 'team.md'), + makePopulatedTeamMd([{ name: 'Fenster', role: 'Developer' }]), + ); + fs.writeFileSync(path.join(squadDir, '.init-prompt'), 'Build a snake game'); + + const teamContent = fs.readFileSync(path.join(squadDir, 'team.md'), 'utf-8'); + let guardFired = false; + + if (!hasRosterEntries(teamContent)) { + guardFired = true; + } + + // Guard should NOT fire — roster has entries, dispatch proceeds + expect(guardFired).toBe(false); + // .init-prompt should still exist (normal cleanup happens later in the flow) + expect(fs.existsSync(path.join(squadDir, '.init-prompt'))).toBe(true); + }); + + it('empty roster guard fires when team.md is missing entirely', () => { + // team.md doesn't exist → readSync returns '' → hasRosterEntries returns false + const teamContent = ''; + let guardFired = false; + + if (!hasRosterEntries(teamContent)) { + guardFired = true; + } + + expect(guardFired).toBe(true); + }); +}); + // =========================================================================== // 4. /init command — executeCommand('init', ...) // ===========================================================================