fix: address CodeRabbit review suggestions for PR #64

- doctor.ts: use execFileSync, typed AgentType, remove as-any casts
- activity.ts: validate parseInt with radix and NaN guard
- issue.ts: wrap JSON.parse in try/catch for gh CLI output
- issue-planner.ts: guard JSON.parse, fix includeTests no-op ternary,
  remove duplicate IssuePlanMetadata (import from types.ts)
- status.ts: typed StatusOverview interface, remove as-any casts
- snapshot-manager.ts: align observations param with SessionSnapshot type
- session.ts: proper type mapping for observations (no double cast)
- activity-log.ts: min-length guard for SHA prefix match
- e2e tests: use top-level imports instead of inline require()
- commands.test.ts: add assertions for DoctorCommand, IssuePlanCommand,
  IssueListCommand

Author: rohitg00

packages/cli/src/__tests__/commands.test.ts

@@ -22,6 +22,9 @@ describe('CLI Commands', () => {
       expect(commands.RecommendCommand).toBeDefined();
 
       expect(commands.ActivityCommand).toBeDefined();
+      expect(commands.DoctorCommand).toBeDefined();
+      expect(commands.IssuePlanCommand).toBeDefined();
+      expect(commands.IssueListCommand).toBeDefined();
       expect(commands.SessionSnapshotSaveCommand).toBeDefined();
       expect(commands.SessionSnapshotRestoreCommand).toBeDefined();
       expect(commands.SessionSnapshotListCommand).toBeDefined();

packages/cli/src/__tests__/e2e-session-features.test.ts

@@ -1,8 +1,9 @@
 import { describe, it, expect, beforeEach, afterEach } from 'vitest';
 import { execSync } from 'node:child_process';
-import { mkdirSync, rmSync, existsSync, readFileSync } from 'node:fs';
+import { mkdirSync, rmSync, existsSync, readFileSync, writeFileSync } from 'node:fs';
 import { join } from 'node:path';
 import { tmpdir } from 'node:os';
+import { stringify } from 'yaml';
 
 const CLI = join(__dirname, '../../../../apps/skillkit/dist/cli.js');
 
@@ -82,8 +83,6 @@ describe('E2E: Session Features', () => {
     });
 
     it('should save and restore snapshot with active session', () => {
-      const { stringify } = require('yaml');
-
       const skillkitDir = join(testDir, '.skillkit');
       mkdirSync(skillkitDir, { recursive: true });
 
@@ -111,7 +110,6 @@ describe('E2E: Session Features', () => {
         decisions: [{ key: 'test-key', value: 'test-value', madeAt: new Date().toISOString() }],
       };
 
-      const { writeFileSync } = require('node:fs');
       writeFileSync(
         join(skillkitDir, 'session.yaml'),
         stringify(sessionState)
@@ -158,9 +156,6 @@ describe('E2E: Session Features', () => {
     });
 
     it('should explain session with active execution', () => {
-      const { stringify } = require('yaml');
-      const { writeFileSync } = require('node:fs');
-
       const skillkitDir = join(testDir, '.skillkit');
       mkdirSync(skillkitDir, { recursive: true });
 

packages/cli/src/commands/activity.ts

@@ -29,7 +29,8 @@ export class ActivityCommand extends Command {
   async execute(): Promise<number> {
     const projectPath = process.cwd();
     const log = new ActivityLog(projectPath);
-    const limit = this.limit ? parseInt(this.limit) : 10;
+    const parsed = this.limit ? parseInt(this.limit, 10) : 10;
+    const limit = Number.isFinite(parsed) && parsed > 0 ? parsed : 10;
 
     const activities = this.skill
       ? log.getBySkill(this.skill)

packages/cli/src/commands/doctor.ts

@@ -1,9 +1,10 @@
 import { Command, Option } from 'clipanion';
-import { execSync } from 'node:child_process';
+import { execFileSync } from 'node:child_process';
 import { existsSync, lstatSync, readlinkSync, readdirSync, mkdirSync, unlinkSync } from 'node:fs';
 import { resolve, join } from 'node:path';
 import { homedir } from 'node:os';
 import { detectAgent, getAllAdapters, getAdapter } from '@skillkit/agents';
+import type { AgentType } from '@skillkit/core';
 import { findAllSkills, validateSkill, loadConfig, getProjectConfigPath } from '@skillkit/core';
 import { getSearchDirs } from '../helpers.js';
 import { colors, symbols } from '../onboarding/index.js';
@@ -81,7 +82,7 @@ export class DoctorCommand extends Command {
     }
 
     // --- Agent Detection ---
-    let detectedAgent = 'universal';
+    let detectedAgent: AgentType = 'universal';
     try {
       detectedAgent = await detectAgent();
       results.push({ name: 'Detected agent', status: 'pass', message: `Detected: ${detectedAgent}` });
@@ -110,7 +111,7 @@ export class DoctorCommand extends Command {
     }
 
     // --- Skills ---
-    const adapter = getAdapter(detectedAgent as any);
+    const adapter = getAdapter(detectedAgent);
     const skillsDir = resolve(adapter.skillsDir);
     const skillsDirExists = existsSync(skillsDir);
 
@@ -142,7 +143,7 @@ export class DoctorCommand extends Command {
     // Skill validation
     let searchDirs: string[] = [];
     try {
-      searchDirs = getSearchDirs(detectedAgent as any);
+      searchDirs = getSearchDirs(detectedAgent);
     } catch {
       searchDirs = skillsDirExists ? [skillsDir] : [];
     }
@@ -285,7 +286,7 @@ export class DoctorCommand extends Command {
 
   private whichVersion(cmd: string): string | null {
     try {
-      return execSync(`${cmd} --version`, { encoding: 'utf-8', timeout: 5000 })
+      return execFileSync(cmd, ['--version'], { encoding: 'utf-8', timeout: 5000 })
         .trim()
         .replace(/^v/, '');
     } catch {

packages/cli/src/commands/issue.ts

@@ -245,12 +245,18 @@ export class IssueListCommand extends Command {
       return 1;
     }
 
-    const issues = JSON.parse(result) as Array<{
+    let issues: Array<{
       number: number;
       title: string;
       labels: Array<{ name: string }>;
       assignees: Array<{ login: string }>;
     }>;
+    try {
+      issues = JSON.parse(result);
+    } catch {
+      this.context.stderr.write('Error: Failed to parse GitHub CLI response.\n');
+      return 1;
+    }
 
     if (this.json) {
       this.context.stdout.write(JSON.stringify(issues, null, 2) + '\n');

packages/cli/src/commands/session.ts

@@ -12,6 +12,7 @@ import {
   SessionExplainer,
   ObservationStore,
   type SessionFile,
+  type SessionSnapshot,
 } from '@skillkit/core';
 
 export class SessionCommand extends Command {
@@ -357,9 +358,18 @@ export class SessionSnapshotSaveCommand extends Command {
       return 1;
     }
 
-    let observations: Array<Record<string, unknown>> = [];
+    let observations: SessionSnapshot['observations'] = [];
     try {
-      observations = new ObservationStore(projectPath).getAll() as unknown as Array<Record<string, unknown>>;
+      const raw = new ObservationStore(projectPath).getAll();
+      observations = raw.map((o) => ({
+        id: o.id,
+        timestamp: o.timestamp,
+        sessionId: o.sessionId,
+        agent: o.agent as string,
+        type: o.type as string,
+        content: { ...o.content } as Record<string, unknown>,
+        relevance: o.relevance,
+      }));
     } catch {
       // No observations available
     }

packages/cli/src/commands/status.ts

@@ -2,10 +2,23 @@ import { Command, Option } from 'clipanion';
 import { resolve } from 'node:path';
 import { existsSync } from 'node:fs';
 import chalk from 'chalk';
+import type { AgentType } from '@skillkit/core';
 import { SessionManager, findAllSkills, loadConfig, getProjectConfigPath } from '@skillkit/core';
 import { detectAgent, getAdapter } from '@skillkit/agents';
 import { getSearchDirs } from '../helpers.js';
 
+interface StatusOverview {
+  agent: string;
+  config: string | null;
+  configAgent: string;
+  version: string;
+  totalSkills: number;
+  projectSkills: number;
+  globalSkills: number;
+  skillsDir: string;
+  recentHistory: Array<{ status: string; skillName: string; completedAt: string }>;
+}
+
 /**
  * Status command - show current session state
  */
@@ -137,8 +150,8 @@ export class StatusCommand extends Command {
     return 0;
   }
 
-  private async buildOverview(manager: SessionManager): Promise<Record<string, unknown>> {
-    let agent = 'universal';
+  private async buildOverview(manager: SessionManager): Promise<StatusOverview> {
+    let agent: AgentType = 'universal';
     try {
       agent = await detectAgent();
     } catch {
@@ -160,15 +173,15 @@ export class StatusCommand extends Command {
 
     let searchDirs: string[] = [];
     try {
-      searchDirs = getSearchDirs(agent as any);
+      searchDirs = getSearchDirs(agent);
     } catch {
       // fallback
     }
 
     const allSkills = findAllSkills(searchDirs);
     const projectSkills = allSkills.filter(s => s.location === 'project');
     const globalSkills = allSkills.filter(s => s.location === 'global');
-    const adapter = getAdapter(agent as any);
+    const adapter = getAdapter(agent);
     const recentHistory = manager.getHistory(3);
 
     return {
@@ -184,26 +197,24 @@ export class StatusCommand extends Command {
     };
   }
 
-  private showOverview(overview: Record<string, unknown>): void {
+  private showOverview(overview: StatusOverview): void {
     console.log('');
     console.log(chalk.cyan('  Project Overview'));
-    console.log(`    Agent:    ${chalk.bold(String(overview.agent))}`);
+    console.log(`    Agent:    ${chalk.bold(overview.agent)}`);
     console.log(`    Config:   ${overview.config ? chalk.green('skillkit.yaml') : chalk.dim('none (defaults)')}`);
-    console.log(`    Version:  ${chalk.bold(String(overview.version))}`);
+    console.log(`    Version:  ${chalk.bold(overview.version)}`);
     console.log('');
 
-    const total = overview.totalSkills as number;
-    console.log(chalk.cyan(`  Skills (${total} installed)`));
+    console.log(chalk.cyan(`  Skills (${overview.totalSkills} installed)`));
     console.log(`    Project:  ${overview.projectSkills} skills in ${overview.skillsDir}`);
     console.log(`    Global:   ${overview.globalSkills} skills`);
     console.log('');
 
     console.log(chalk.cyan('  Recent Activity'));
-    const history = overview.recentHistory as Array<{ status: string; skillName: string; completedAt: string }>;
-    if (history.length === 0) {
+    if (overview.recentHistory.length === 0) {
       console.log(chalk.dim('    No recent executions.'));
     } else {
-      for (const entry of history) {
+      for (const entry of overview.recentHistory) {
         const statusColor = entry.status === 'completed' ? chalk.green : chalk.red;
         console.log(`    ${statusColor('\u25cf')} ${entry.skillName} - ${new Date(entry.completedAt).toLocaleDateString()}`);
       }

packages/core/src/plan/issue-planner.ts

@@ -1,6 +1,8 @@
 import { execFileSync } from 'node:child_process';
 import { PlanGenerator } from './generator.js';
-import type { StructuredPlan, PlanTaskFiles } from './types.js';
+import type { StructuredPlan, PlanTaskFiles, IssuePlanMetadata } from './types.js';
+
+export type { IssuePlanMetadata };
 
 export interface GitHubIssue {
   number: number;
@@ -19,14 +21,6 @@ export interface IssuePlanOptions {
   includeTests?: boolean;
 }
 
-export interface IssuePlanMetadata {
-  issueNumber: number;
-  issueUrl: string;
-  issueLabels: string[];
-  agent: string;
-  generatedAt: string;
-}
-
 interface ParsedRef {
   owner?: string;
   repo?: string;
@@ -110,17 +104,26 @@ export class IssuePlanner {
       timeout: 15_000,
     });
 
-    const data = JSON.parse(result);
+    let data: {
+      number: number;
+      title: string;
+      body?: string;
+      labels?: Array<{ name: string }>;
+      assignees?: Array<{ login: string }>;
+      url: string;
+      state: string;
+    };
+    try {
+      data = JSON.parse(result);
+    } catch {
+      throw new Error(`Failed to parse GitHub CLI response for issue ${parsed.number}`);
+    }
     return {
       number: data.number,
       title: data.title,
       body: data.body || '',
-      labels: (data.labels || []).map(
-        (l: { name: string }) => l.name
-      ),
-      assignees: (data.assignees || []).map(
-        (a: { login: string }) => a.login
-      ),
+      labels: (data.labels || []).map((l) => l.name),
+      assignees: (data.assignees || []).map((a) => a.login),
       url: data.url,
       state: data.state,
     };
@@ -205,7 +208,7 @@ export class IssuePlanner {
           steps: [
             { type: 'implement', description: item.name },
             ...(includeTests
-              ? []
+              ? [{ type: 'test' as const, description: `Write tests for ${item.name}` }]
               : []),
           ],
         });

packages/core/src/session/activity-log.ts

@@ -73,6 +73,7 @@ export class ActivityLog {
   }
 
   getByCommit(sha: string): SkillActivity | undefined {
+    if (sha.length < 4) return undefined;
     const data = this.load();
     return data.activities.find(
       (a) => a.commitSha === sha || a.commitSha.startsWith(sha)

packages/core/src/session/snapshot-manager.ts

@@ -40,7 +40,7 @@ export class SnapshotManager {
   save(
     name: string,
     sessionState: SessionState,
-    observations: Array<Record<string, unknown>>,
+    observations: SessionSnapshot['observations'],
     description?: string
   ): void {
     this.ensureDir();
@@ -51,7 +51,7 @@ export class SnapshotManager {
       createdAt: new Date().toISOString(),
       description,
       sessionState,
-      observations: observations as SessionSnapshot['observations'],
+      observations,
     };
 
     writeFileSync(this.getPath(name), stringify(snapshot));