fix: add onboarding roster guard and skill overwrite warning

.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

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

packages/squad-cli/src/cli/shell/index.ts

@@ -986,6 +986,26 @@ export async function runShell(): Promise<void> {
       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.`,

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(/<!-- version: [^>]+ -->/m, '<!-- version: 0.1.0 -->');
+    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(/<!-- version: [^>]+ -->/m, '<!-- version: 0.1.0 -->');
+    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(/<!-- version: [^>]+ -->/m, '<!-- version: 0.1.0 -->');
+    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(/<!-- version: [^>]+ -->/m, '<!-- version: 0.1.0 -->');
+    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();
+    }
+  });
 });

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', ...)
 // ===========================================================================