fix: address post-merge review findings (#876, #900, #875) — type safety, YAML escaping

.changeset/review-findings-fix.md

@@ -0,0 +1,5 @@
+---
+'@bradygaster/squad-cli': patch
+---
+
+fix: address post-merge review findings — YAML escaping, type safety, deprecation messages

packages/squad-cli/src/cli-entry.ts

@@ -719,7 +719,7 @@ async function main(): Promise<void> {
 
   if (cmd === 'start') {
     console.log(`\n${YELLOW}⚠ DEPRECATED:${RESET} "squad start" is deprecated and will be removed in a future release.`);
-    console.log(`  Use the GitHub Copilot CLI directly: ${BOLD}copilot${RESET} or ${BOLD}gh copilot${RESET}\n`);
+    console.log(`  Use the GitHub Copilot CLI directly: ${BOLD}gh copilot${RESET}\n`);
     const { runStart } = await import('./cli/commands/start.js');
     const hasTunnel = args.includes('--tunnel');
     if (hasTunnel) {
@@ -805,7 +805,7 @@ async function main(): Promise<void> {
 
   if (cmd === 'rc' || cmd === 'remote-control') {
     console.log(`\n${YELLOW}⚠ DEPRECATED:${RESET} "squad rc" is deprecated and will be removed in a future release.`);
-    console.log(`  Use the GitHub Copilot CLI directly: ${BOLD}copilot${RESET} or ${BOLD}gh copilot${RESET}\n`);
+    console.log(`  Use the GitHub Copilot CLI directly: ${BOLD}gh copilot${RESET}\n`);
     const { runRC } = await import('./cli/commands/rc.js');
     const hasTunnel = args.includes('--tunnel');
     const portIdx = args.indexOf('--port');

packages/squad-cli/src/cli/commands/plugin.ts

@@ -156,8 +156,9 @@ export async function runPlugin(dest: string, args: string[]): Promise<void> {
         { timeout: TIMEOUTS.PLUGIN_FETCH_MS }
       );
       entries = JSON.parse(stdout.trim());
-    } catch (err: any) {
-      fatal(`Could not browse ${marketplace.source} — ${err.message}`);
+    } catch (err: unknown) {
+      const message = err instanceof Error ? err.message : String(err);
+      fatal(`Could not browse ${marketplace.source} — ${message}`);
     }
 
     if (!entries || entries.length === 0) {

packages/squad-cli/src/cli/commands/skill.ts

@@ -42,6 +42,26 @@ export interface ApmManifest {
 
 // ── Helpers ───────────────────────────────────────────────────────────────────
 
+/** Wrap a YAML scalar in single quotes if it contains special characters. */
+export function escapeYamlValue(val: string): string {
+  if (/[:#'"\n\r]/.test(val)) {
+    return `'${val.replace(/'/g, "''")}'`;
+  }
+  return val;
+}
+
+/** Type guard for objects with string `name` and `path` properties. */
+function isNamedPath(obj: unknown): obj is { name: string; path: string } {
+  return (
+    typeof obj === 'object' &&
+    obj !== null &&
+    'name' in obj &&
+    'path' in obj &&
+    typeof (obj as Record<string, unknown>).name === 'string' &&
+    typeof (obj as Record<string, unknown>).path === 'string'
+  );
+}
+
 /** Parse `---\nkey: value\n---` front-matter from a Markdown file. */
 function parseFrontMatter(content: string): Record<string, string> {
   const match = content.match(/^---\r?\n([\s\S]*?)\r?\n---/);
@@ -144,14 +164,14 @@ async function publish(dest: string, skillName?: string): Promise<void> {
     // Build the skill's own apm.yml inside its directory
     const apmSkillPath = join(skillsDir, skillName, 'apm.yml');
     const skillApm = [
-      `name: ${fm['name'] ?? skillName}`,
-      `version: ${fm['version'] ?? '1.0.0'}`,
-      fm['description'] ? `description: "${fm['description']}"` : null,
+      `name: ${escapeYamlValue(fm['name'] ?? skillName)}`,
+      `version: ${escapeYamlValue(fm['version'] ?? '1.0.0')}`,
+      fm['description'] ? `description: ${escapeYamlValue(fm['description'])}` : null,
       ``,
       `skills:`,
-      `  - name: ${fm['name'] ?? skillName}`,
+      `  - name: ${escapeYamlValue(fm['name'] ?? skillName)}`,
       `    path: skill.md`,
-      fm['description'] ? `    description: "${fm['description']}"` : null,
+      fm['description'] ? `    description: ${escapeYamlValue(fm['description'])}` : null,
     ]
       .filter(l => l !== null)
       .join('\n');
@@ -186,17 +206,17 @@ async function publish(dest: string, skillName?: string): Promise<void> {
     `# apm.yml — Agent Package Manager manifest`,
     `# See: https://github.com/microsoft/apm`,
     ``,
-    `name: ${existing.name ?? projectName}`,
+    `name: ${escapeYamlValue(existing.name ?? projectName)}`,
     `version: 1.0.0`,
     ``,
     `# Skills exported from ${relPrefix}/`,
     `skills:`,
     ...skills.map(s =>
       [
-        `  - name: ${s.name}`,
-        s.description ? `    description: "${s.description}"` : null,
+        `  - name: ${escapeYamlValue(s.name)}`,
+        s.description ? `    description: ${escapeYamlValue(s.description)}` : null,
         `    path: ${s.path}`,
-        s.version ? `    version: ${s.version}` : null,
+        s.version ? `    version: ${escapeYamlValue(s.version)}` : null,
       ]
         .filter(l => l !== null)
         .join('\n')
@@ -281,8 +301,9 @@ async function installFromUrl(url: string, skillsDir: string, relPrefix: string)
       fatal(`Failed to fetch ${url}: ${res.status} ${res.statusText}`);
     }
     content = await res.text();
-  } catch (err: any) {
-    fatal(`Failed to fetch ${url}: ${err.message}`);
+  } catch (err: unknown) {
+    const message = err instanceof Error ? err.message : String(err);
+    fatal(`Failed to fetch ${url}: ${message}`);
     return;
   }
 
@@ -341,21 +362,21 @@ async function installFromGitHub(
     }
     if (inSkills && line.match(/^[a-z]/i) && !line.startsWith('  ')) {
       // New top-level key — exit skills section
-      if (currentSkill.name && currentSkill.path) skillPaths.push(currentSkill as { name: string; path: string });
+      if (isNamedPath(currentSkill)) skillPaths.push(currentSkill);
       currentSkill = {};
       inSkills = false;
     }
     if (inSkills) {
       const nameMatch = line.match(/^\s+- name:\s*(.+)$/);
       const pathMatch = line.match(/^\s+path:\s*(.+)$/);
       if (nameMatch) {
-        if (currentSkill.name && currentSkill.path) skillPaths.push(currentSkill as { name: string; path: string });
+        if (isNamedPath(currentSkill)) skillPaths.push(currentSkill);
         currentSkill = { name: nameMatch[1]!.trim() };
       }
       if (pathMatch) currentSkill.path = pathMatch[1]!.trim();
     }
   }
-  if (currentSkill.name && currentSkill.path) skillPaths.push(currentSkill as { name: string; path: string });
+  if (isNamedPath(currentSkill)) skillPaths.push(currentSkill);
 
   // Filter by skill name if specified
   const toInstall = skillFilter
@@ -395,8 +416,9 @@ async function installFromGitHub(
       success(`Installed skill '${skill.name}'`);
       info(`  ${DIM}Source: ${owner}/${repo}${skill.path}${RESET}`);
       installed++;
-    } catch (err: any) {
-      warn(`Failed to install '${skill.name}': ${err.message}`);
+    } catch (err: unknown) {
+      const message = err instanceof Error ? err.message : String(err);
+      warn(`Failed to install '${skill.name}': ${message}`);
     }
   }
 

packages/squad-cli/src/cli/core/init.ts

@@ -13,6 +13,7 @@ import { fatal } from './errors.js';
 import { detectProjectType } from './project-type.js';
 import { getPackageVersion, stampVersion } from './version.js';
 import { initSquad as sdkInitSquad, cleanupOrphanInitPrompt, ensurePersonalSquadDir, resolvePersonalSquadDir, type InitOptions } from '@bradygaster/squad-sdk';
+import { escapeYamlValue } from '../commands/skill.js';
 
 const storage = new FSStorageProvider();
 
@@ -40,7 +41,7 @@ export function generateApmYml(dest: string, projectName: string): void {
     `# This file makes your Squad skills versioned, portable, and community-shareable.`,
     `# Run 'squad skill publish' to populate the skills section after adding skills.`,
     ``,
-    `name: ${projectName}`,
+    `name: ${escapeYamlValue(projectName)}`,
     `version: 1.0.0`,
     ``,
     `# Skills — add entries here or run 'squad skill publish' to auto-populate`,
@@ -292,9 +293,10 @@ export async function runInit(dest: string, options: RunInitOptions = {}): Promi
   let result;
   try {
     result = await sdkInitSquad(initOptions);
-  } catch (err: any) {
+  } catch (err: unknown) {
     process.off('SIGINT', sigintHandler);
-    fatal(`Failed to initialize squad: ${err.message}`);
+    const message = err instanceof Error ? err.message : String(err);
+    fatal(`Failed to initialize squad: ${message}`);
     return; // Unreachable but makes TS happy
   }
 

test/cross-package-exports.test.ts

@@ -15,6 +15,8 @@
  *   add a corresponding assertion here.  The grep one-liner in the test
  *   description shows how to audit.
  *
+ *     grep -rn "from '@bradygaster/squad-sdk" packages/squad-cli/src/
+ *
  * Related incident: v0.9.3-insider.1 shipped with FSStorageProvider missing
  * from the SDK barrel — broke users at runtime while tests passed locally.
  */
@@ -264,7 +266,13 @@ describe('cross-package exports — CLI → SDK', () => {
       const pkg = JSON.parse(
         fs.readFileSync(resolve(sdkRoot!, 'package.json'), 'utf8'),
       );
-      const exportsMap = pkg.exports as Record<string, Record<string, string>>;
+      expect(pkg.exports, 'SDK package.json should have an exports field').toBeDefined();
+      expect(
+        typeof pkg.exports === 'object' && pkg.exports !== null,
+        'exports should be an object',
+      ).toBe(true);
+
+      const exportsMap = pkg.exports as Record<string, unknown>;
       const missing: string[] = [];
 
       for (const [subpath, targets] of Object.entries(exportsMap)) {
@@ -274,9 +282,12 @@ describe('cross-package exports — CLI → SDK', () => {
           }
           continue;
         }
-        for (const [condition, file] of Object.entries(targets)) {
-          if (!existsSync(resolve(sdkRoot!, file))) {
-            missing.push(`${subpath}[${condition}] → ${file}`);
+        if (typeof targets === 'object' && targets !== null) {
+          for (const [condition, file] of Object.entries(targets as Record<string, unknown>)) {
+            if (typeof file !== 'string') continue;
+            if (!existsSync(resolve(sdkRoot!, file))) {
+              missing.push(`${subpath}[${condition}] → ${file}`);
+            }
           }
         }
       }