chore: address valid robustness issues in last Copilot review

jqnatividad · jqnatividad · commit 55a5a300b024 · 2026-01-07T12:15:10.000-05:00
diff --git a/.claude/skills/src/update-checker.ts b/.claude/skills/src/update-checker.ts
@@ -79,9 +79,11 @@ export class UpdateChecker {
           return;
         }
 
-        // Parse version from output like "qsv 0.132.0"
-        const match = output.match(/qsv\s+(\d+\.\d+\.\d+)/);
+        // Parse version from output like "qsv 0.132.0" or "qsv 0.132.0-beta"
+        // Handle multiple spaces, extra text, and pre-release tags
+        const match = output.match(/qsv\s+(\d+\.\d+\.\d+)(?:-[\w.]+)?/);
         if (match) {
+          // Only use the main version number (ignore pre-release tags for now)
           resolve(match[1]);
         } else {
           reject(new Error(`Could not parse qsv version from: ${output}`));
@@ -98,19 +100,33 @@ export class UpdateChecker {
    * Get version that skills were generated with (from skill JSON files)
    */
   getSkillsVersion(): string {
-    try {
-      // Read version from any skill file (they all have the same version)
-      const sampleSkillPath = join(this.skillsDir, 'qsv-stats.json');
-      if (!existsSync(sampleSkillPath)) {
-        return 'unknown';
-      }
+    // Try multiple skill files as fallbacks for resilience
+    const skillFilesToTry = [
+      'qsv-stats.json',
+      'qsv-select.json',
+      'qsv-count.json',
+      'qsv-search.json'
+    ];
+
+    for (const skillFile of skillFilesToTry) {
+      try {
+        const skillPath = join(this.skillsDir, skillFile);
+        if (!existsSync(skillPath)) {
+          continue;
+        }
 
-      const skill = JSON.parse(readFileSync(sampleSkillPath, 'utf-8'));
-      return skill.version || 'unknown';
-    } catch (error) {
-      console.error('[UpdateChecker] Failed to read skills version:', error);
-      return 'unknown';
+        const skill = JSON.parse(readFileSync(skillPath, 'utf-8'));
+        if (skill.version) {
+          return skill.version;
+        }
+      } catch (error) {
+        // Try next file
+        continue;
+      }
     }
+
+    console.warn('[UpdateChecker] Could not determine skills version from any skill file');
+    return 'unknown';
   }
 
   /**
@@ -153,6 +169,11 @@ export class UpdateChecker {
       writeFileSync(this.versionFilePath, JSON.stringify(info, null, 2), 'utf-8');
     } catch (error) {
       console.error('[UpdateChecker] Failed to save version info:', error);
+      console.warn(
+        '[UpdateChecker] WARNING: Version info could not be persisted at',
+        this.versionFilePath,
+        '- update checks may be repeated or version tracking may be inaccurate.'
+      );
     }
   }
 
@@ -177,9 +198,19 @@ export class UpdateChecker {
         return null;
       }
 
-      const data = await response.json() as { tag_name: string };
+      const data: unknown = await response.json();
+      if (
+        !data ||
+        typeof data !== 'object' ||
+        data === null ||
+        typeof (data as { tag_name?: unknown }).tag_name !== 'string'
+      ) {
+        console.error('[UpdateChecker] GitHub API response missing valid tag_name field');
+        return null;
+      }
+      const tagName = (data as { tag_name: string }).tag_name;
       // Tag format is typically "v0.132.0" or "0.132.0"
-      const version = data.tag_name.replace(/^v/, '');
+      const version = tagName.replace(/^v/, '');
       return version;
     } catch (error) {
       console.error('[UpdateChecker] Failed to check GitHub releases:', error);
@@ -189,11 +220,20 @@ export class UpdateChecker {
 
   /**
    * Compare semantic versions (simple implementation)
+   * Handles only numeric versions like "1.2.3" (pre-release tags are ignored)
    */
   private compareVersions(v1: string, v2: string): number {
     const parts1 = v1.split('.').map(Number);
     const parts2 = v2.split('.').map(Number);
 
+    // Validate that all parts are valid numbers
+    if (parts1.some(isNaN) || parts2.some(isNaN)) {
+      console.warn(
+        `[UpdateChecker] Invalid version format: "${v1}" or "${v2}" - comparison may be incorrect`
+      );
+      return 0; // Treat as equal if we can't compare
+    }
+
     for (let i = 0; i < Math.max(parts1.length, parts2.length); i++) {
       const part1 = parts1[i] || 0;
       const part2 = parts2[i] || 0;
diff --git a/src/main.rs b/src/main.rs
@@ -268,10 +268,13 @@ fn main() -> QsvExitCode {
     #[cfg(feature = "mcp")]
     {
         if args.flag_update_mcp_skills {
-            util::log_end(qsv_args, now);
             match mcp_skills_gen::generate_mcp_skills() {
-                Ok(()) => return QsvExitCode::Good,
+                Ok(()) => {
+                    util::log_end(qsv_args, now);
+                    return QsvExitCode::Good;
+                },
                 Err(e) => {
+                    util::log_end(qsv_args, now);
                     werr!("MCP skills generation error: {e}");
                     return QsvExitCode::Bad;
                 },
diff --git a/src/mcp_skills_gen.rs b/src/mcp_skills_gen.rs
@@ -601,21 +601,44 @@ pub fn generate_mcp_skills() -> CliResult<()> {
     // Determine repository root - look for Cargo.toml with src/cmd
     // This command must be run from within the qsv repository directory
     let mut repo_root = std::env::current_dir()?;
+    let original_dir = repo_root.clone();
+
+    let mut iterations = 0;
+    const MAX_ITERATIONS: usize = 100; // Prevent infinite loops
+
     loop {
         if repo_root.join("Cargo.toml").exists() && repo_root.join("src/cmd").exists() {
             break;
         }
+
+        iterations += 1;
+        if iterations >= MAX_ITERATIONS {
+            return fail_clierror!(
+                "Could not find qsv repository root after checking {} parent directories. \
+                 This command must be run from within the qsv repository directory \
+                 (where Cargo.toml and src/cmd exist).\n\
+                 Original directory: {}\n\
+                 \n\
+                 If you're using a package-installed qsv binary, you need to:\n\
+                 1. Clone the qsv repository: git clone https://github.com/dathere/qsv.git\n\
+                 2. cd into the repository: cd qsv\n\
+                 3. Run: qsv --update-mcp-skills",
+                MAX_ITERATIONS,
+                original_dir.display()
+            );
+        }
+
         if !repo_root.pop() {
             return fail_clierror!(
                 "Could not find qsv repository root. This command must be run from within \
                  the qsv repository directory (where Cargo.toml and src/cmd exist).\n\
-                 Current directory: {}\n\
+                 Original directory: {}\n\
                  \n\
                  If you're using a package-installed qsv binary, you need to:\n\
                  1. Clone the qsv repository: git clone https://github.com/dathere/qsv.git\n\
                  2. cd into the repository: cd qsv\n\
                  3. Run: qsv --update-mcp-skills",
-                std::env::current_dir()?.display()
+                original_dir.display()
             );
         }
     }
