Harden CI command execution and runtime cleanup paths

.github/actions/generate-embeddings/action.yml

@@ -91,45 +91,44 @@ runs:
       run: |
         echo "🧠 Generating embeddings for codebase..."
 
-        # Build command arguments based on actual CLI options
-        ARGS="embeddings:generate"
-        ARGS="${ARGS} --directory \"$GITHUB_WORKSPACE\""
+        # Build command arguments safely - avoid eval/string interpolation
+        ARGS=("embeddings:generate" "--directory" "$GITHUB_WORKSPACE")
 
         # Add concurrency only if provided
         if [ -n "${{ inputs.concurrency }}" ]; then
-          ARGS="${ARGS} --concurrency ${{ inputs.concurrency }}"
+          ARGS+=("--concurrency" "${{ inputs.concurrency }}")
         fi
 
         # Add specific files if provided
         if [ -n "${{ inputs.files }}" ]; then
           for file_pattern in ${{ inputs.files }}; do
-            ARGS="${ARGS} --files '${file_pattern}'"
+            ARGS+=("--files" "${file_pattern}")
           done
         fi
 
         # Always exclude action-related directories and files
-        ARGS="${ARGS} --exclude 'codecritique-tool/**'"
-        ARGS="${ARGS} --exclude '.codecritique-artifacts/**'"
+        ARGS+=("--exclude" "codecritique-tool/**")
+        ARGS+=("--exclude" ".codecritique-artifacts/**")
 
         # Add user-specified exclude patterns
         if [ -n "${{ inputs.exclude }}" ]; then
           for pattern in ${{ inputs.exclude }}; do
-            ARGS="${ARGS} --exclude '${pattern}'"
+            ARGS+=("--exclude" "${pattern}")
           done
         fi
 
         # Add exclude file if specified
         if [ -n "${{ inputs.exclude-file }}" ] && [ -f "$GITHUB_WORKSPACE/${{ inputs.exclude-file }}" ]; then
-          ARGS="${ARGS} --exclude-file \"$GITHUB_WORKSPACE/${{ inputs.exclude-file }}\""
+          ARGS+=("--exclude-file" "$GITHUB_WORKSPACE/${{ inputs.exclude-file }}")
         fi
 
         # Add verbose flag
         if [ "${{ inputs.verbose }}" = "true" ]; then
-          ARGS="${ARGS} --verbose"
+          ARGS+=("--verbose")
         fi
 
         # Execute with error handling
-        if eval "node src/index.js ${ARGS}"; then
+        if node src/index.js "${ARGS[@]}"; then
           echo "✅ Embedding generation completed successfully"
           echo "success=true" >> $GITHUB_OUTPUT
         else

.github/actions/pr-review/action.yml

@@ -156,56 +156,55 @@ runs:
         DEBUG: ${{ inputs.verbose }}
         VERBOSE: ${{ inputs.verbose }}
       run: |
-        # Build CLI command arguments - analyze workspace directly
-        CLI_ARGS="analyze -b ${{ github.head_ref }} --directory \"$GITHUB_WORKSPACE\""
+        # Build CLI command arguments safely - avoid eval/string interpolation.
+        CLI_ARGS=("analyze" "-b" "${{ github.head_ref }}" "--directory" "$GITHUB_WORKSPACE")
 
         # Add output format and output file (always JSON)
-        CLI_ARGS="$CLI_ARGS --output json --output-file review_output.json"
+        CLI_ARGS+=("--output" "json" "--output-file" "review_output.json")
 
         # Add verbose flag if enabled
         if [ "${{ inputs.verbose }}" = "true" ]; then
-          CLI_ARGS="$CLI_ARGS --verbose"
+          CLI_ARGS+=("--verbose")
         fi
 
         # Add model configuration
         if [ -n "${{ inputs.model }}" ]; then
-          CLI_ARGS="$CLI_ARGS --model ${{ inputs.model }}"
+          CLI_ARGS+=("--model" "${{ inputs.model }}")
         fi
 
         # Only add optional parameters if they have values
         if [ -n "${{ inputs.max-tokens }}" ]; then
-          CLI_ARGS="$CLI_ARGS --max-tokens ${{ inputs.max-tokens }}"
+          CLI_ARGS+=("--max-tokens" "${{ inputs.max-tokens }}")
         fi
 
         if [ -n "${{ inputs.cache-ttl }}" ]; then
-          CLI_ARGS="$CLI_ARGS --cache-ttl ${{ inputs.cache-ttl }}"
+          CLI_ARGS+=("--cache-ttl" "${{ inputs.cache-ttl }}")
         fi
 
         if [ -n "${{ inputs.concurrency }}" ]; then
-          CLI_ARGS="$CLI_ARGS --concurrency ${{ inputs.concurrency }}"
+          CLI_ARGS+=("--concurrency" "${{ inputs.concurrency }}")
         fi
 
         # Add custom documents if specified
         if [ -n "${{ inputs.custom-docs }}" ]; then
           IFS=',' read -ra DOCS <<< "${{ inputs.custom-docs }}"
           for doc in "${DOCS[@]}"; do
             if [ -n "$doc" ]; then
-              CLI_ARGS="$CLI_ARGS --doc \"$doc\""
+              CLI_ARGS+=("--doc" "$doc")
             fi
           done
         fi
 
         # Add feedback parameters
-        CLI_ARGS="$CLI_ARGS --track-feedback"
-        CLI_ARGS="$CLI_ARGS --feedback-path \"$GITHUB_WORKSPACE/.ai-feedback\""
+        CLI_ARGS+=("--track-feedback" "--feedback-path" "$GITHUB_WORKSPACE/.ai-feedback")
 
         # Run the AI code review
         echo "🤖 Running AI Code Review..."
-        echo "Command: node src/index.js $CLI_ARGS"
+        echo "Command: node src/index.js ${CLI_ARGS[*]}"
 
         # Execute the command and capture output
         # Run the CLI - JSON output goes to file, logs to console
-        eval "node src/index.js $CLI_ARGS" || {
+        node src/index.js "${CLI_ARGS[@]}" || {
           echo "❌ AI Code Review failed"
           exit 1
         }

.github/actions/pr-review/post-comments.js

@@ -257,6 +257,11 @@ export default async ({ github, context, core }) => {
     const currentFeedback = {};
 
     // Check if review output exists
+    if (!reviewOutputPath) {
+      console.log('❌ REVIEW_OUTPUT_PATH is not set');
+      return;
+    }
+
     if (!fs.existsSync(reviewOutputPath)) {
       console.log('❌ Review output file not found');
       return;

src/custom-documents.js

@@ -350,7 +350,7 @@ export class CustomDocumentProcessor {
       }
 
       // Try cache manager
-      const cachedChunks = await this.cacheManager.getCustomDocuments(resolvedProjectPath);
+      const cachedChunks = this.cacheManager.getCustomDocumentChunks(resolvedProjectPath);
       if (cachedChunks && cachedChunks.length > 0) {
         // Restore to memory
         this.customDocumentChunks.set(resolvedProjectPath, cachedChunks);
@@ -520,7 +520,7 @@ export class CustomDocumentProcessor {
     try {
       const resolvedProjectPath = path.resolve(projectPath);
       this.customDocumentChunks.delete(resolvedProjectPath);
-      await this.cacheManager.clearCustomDocuments(resolvedProjectPath);
+      this.cacheManager.customDocumentChunks.delete(resolvedProjectPath);
       console.log(chalk.green(`Cleared custom document chunks for project: ${resolvedProjectPath}`));
     } catch (error) {
       console.error(chalk.red(`Error clearing project chunks: ${error.message}`));

src/custom-documents.test.js

@@ -11,8 +11,7 @@ vi.mock('./embeddings/model-manager.js', () => ({
 vi.mock('./embeddings/cache-manager.js', () => ({
   CacheManager: class {
     storeCustomDocuments = vi.fn().mockResolvedValue(undefined);
-    getCustomDocuments = vi.fn().mockResolvedValue([]);
-    clearCustomDocuments = vi.fn().mockResolvedValue(undefined);
+    getCustomDocumentChunks = vi.fn().mockReturnValue([]);
   },
 }));
 
@@ -32,8 +31,7 @@ describe('CustomDocumentProcessor', () => {
 
     mockCacheManager = {
       storeCustomDocuments: vi.fn().mockResolvedValue(undefined),
-      getCustomDocuments: vi.fn().mockResolvedValue([]),
-      clearCustomDocuments: vi.fn().mockResolvedValue(undefined),
+      getCustomDocumentChunks: vi.fn().mockReturnValue([]),
     };
 
     processor = new CustomDocumentProcessor({
@@ -319,7 +317,7 @@ describe('CustomDocumentProcessor', () => {
 
     it('should return chunks from cache if not in memory', async () => {
       const cachedChunks = [{ id: 'cached', content: 'from cache' }];
-      mockCacheManager.getCustomDocuments.mockResolvedValue(cachedChunks);
+      mockCacheManager.getCustomDocumentChunks.mockReturnValue(cachedChunks);
 
       const result = await processor.getExistingChunks('/project');
 
@@ -328,15 +326,15 @@ describe('CustomDocumentProcessor', () => {
 
     it('should restore cached chunks to memory', async () => {
       const cachedChunks = [{ id: 'cached', content: 'from cache' }];
-      mockCacheManager.getCustomDocuments.mockResolvedValue(cachedChunks);
+      mockCacheManager.getCustomDocumentChunks.mockReturnValue(cachedChunks);
 
       await processor.getExistingChunks('/project');
 
       expect(processor.customDocumentChunks.get('/project')).toEqual(cachedChunks);
     });
 
     it('should return empty array when no chunks exist', async () => {
-      mockCacheManager.getCustomDocuments.mockResolvedValue([]);
+      mockCacheManager.getCustomDocumentChunks.mockReturnValue([]);
 
       const result = await processor.getExistingChunks('/project');
 
@@ -353,10 +351,14 @@ describe('CustomDocumentProcessor', () => {
       expect(processor.customDocumentChunks.has('/project')).toBe(false);
     });
 
-    it('should clear chunks from cache', async () => {
+    it('should clear chunks for only the selected project', async () => {
+      processor.customDocumentChunks.set('/project', [{ id: 'chunk-a' }]);
+      processor.customDocumentChunks.set('/other', [{ id: 'chunk-b' }]);
+
       await processor.clearProjectChunks('/project');
 
-      expect(mockCacheManager.clearCustomDocuments).toHaveBeenCalled();
+      expect(processor.customDocumentChunks.has('/project')).toBe(false);
+      expect(processor.customDocumentChunks.has('/other')).toBe(true);
     });
   });
 

src/embeddings/database.js

@@ -445,6 +445,14 @@ export class DatabaseManager {
       this.dbConnection = null;
       this.tablesInitialized = false;
       throw error;
+    } finally {
+      if (db) {
+        try {
+          await db.close();
+        } catch (closeError) {
+          console.warn(chalk.yellow(`Warning: Failed to close temporary database connection: ${closeError.message}`));
+        }
+      }
     }
   }
 
@@ -524,6 +532,14 @@ export class DatabaseManager {
     } catch (error) {
       console.error(chalk.red(`Error clearing project embeddings: ${error.message}`), error);
       throw error;
+    } finally {
+      if (db) {
+        try {
+          await db.close();
+        } catch (closeError) {
+          console.warn(chalk.yellow(`Warning: Failed to close temporary database connection: ${closeError.message}`));
+        }
+      }
     }
   }
 

src/index.js

@@ -576,35 +576,37 @@ async function generateEmbeddings(options) {
 
     // Start the progress update interval
     const progressInterval = setInterval(updateSpinner, 100);
+    let results;
+    try {
+      results = await embeddingsSystem.processBatchEmbeddings(filesToProcess, {
+        concurrency,
+        verbose: options.verbose,
+        excludePatterns,
+        respectGitignore: options.gitignore !== false,
+        baseDir: baseDir,
+        batchSize: 100, // Set a reasonable batch size
+        maxLines: parseInt(options.maxLines || '1000', 10),
+        onProgress: (status) => {
+          // Update counters based on status
+          if (status === 'processed') {
+            processedCount++;
+          } else if (status === 'skipped') {
+            skippedCount++;
+          } else if (status === 'failed') {
+            failedCount++;
+          } else if (status === 'excluded') {
+            excludedCount++;
+          }
 
-    const results = await embeddingsSystem.processBatchEmbeddings(filesToProcess, {
-      concurrency,
-      verbose: options.verbose,
-      excludePatterns,
-      respectGitignore: options.gitignore !== false,
-      baseDir: baseDir,
-      batchSize: 100, // Set a reasonable batch size
-      maxLines: parseInt(options.maxLines || '1000', 10),
-      onProgress: (status) => {
-        // Update counters based on status
-        if (status === 'processed') {
-          processedCount++;
-        } else if (status === 'skipped') {
-          skippedCount++;
-        } else if (status === 'failed') {
-          failedCount++;
-        } else if (status === 'excluded') {
-          excludedCount++;
-        }
-
-        // Update the spinner with new progress information
-        updateSpinner();
-      },
-    });
-
-    // Clean up the progress display
-    clearInterval(progressInterval);
-    spinner.stop(true);
+          // Update the spinner with new progress information
+          updateSpinner();
+        },
+      });
+    } finally {
+      // Clean up the progress display even if embedding generation fails.
+      clearInterval(progressInterval);
+      spinner.stop(true);
+    }
 
     console.log(chalk.green(`\nEmbedding generation complete!`));
     console.log(chalk.cyan(`Processed: ${results.processed} files`));

src/pr-history/database.js

@@ -421,11 +421,11 @@ export async function getLastAnalysisTimestamp(repository, projectPath) {
     const filters = [`repository = '${repository.replace(/'/g, "''")}'`, `project_path = '${resolvedProjectPath.replace(/'/g, "''")}'`];
 
     const results = await table
-      .search()
+      .query()
       .where(filters.join(' AND '))
-      .limit(1)
       .select(['created_at'])
       .orderBy([{ column: 'created_at', order: 'desc' }])
+      .limit(1)
       .toArray();
 
     if (results.length > 0) {

src/pr-history/database.test.js

@@ -392,11 +392,11 @@ describe('PR History Database', () => {
 
   describe('getLastAnalysisTimestamp', () => {
     const setupMockLastAnalysis = (results) => {
-      mockTable.search.mockReturnValue({
+      mockTable.query.mockReturnValue({
         where: vi.fn().mockReturnThis(),
-        limit: vi.fn().mockReturnThis(),
         select: vi.fn().mockReturnThis(),
         orderBy: vi.fn().mockReturnThis(),
+        limit: vi.fn().mockReturnThis(),
         toArray: vi.fn().mockResolvedValue(results),
       });
     };
@@ -407,11 +407,11 @@ describe('PR History Database', () => {
       [
         'null on errors',
         () =>
-          mockTable.search.mockReturnValue({
+          mockTable.query.mockReturnValue({
             where: vi.fn().mockReturnThis(),
-            limit: vi.fn().mockReturnThis(),
             select: vi.fn().mockReturnThis(),
             orderBy: vi.fn().mockReturnThis(),
+            limit: vi.fn().mockReturnThis(),
             toArray: vi.fn().mockRejectedValue(new Error('Search error')),
           }),
         null,

src/rag-analyzer.js

@@ -774,7 +774,7 @@ async function callLLMForAnalysis(context, options = {}) {
       return {
         summary: analysisResponse.summary || 'Analysis completed with parsing issues',
         issues: Array.isArray(analysisResponse.issues) ? analysisResponse.issues : [],
-        rawResponse: analysisResponse.rawResponse || llmResponse.substring(0, 500),
+        rawResponse: analysisResponse.rawResponse || JSON.stringify(llmResponse).substring(0, 500),
         parseWarning: 'Response structure was reconstructed due to parsing issues',
       };
     }

src/rag-analyzer.test.js

@@ -385,6 +385,17 @@ describe('rag-analyzer', () => {
       expect(result.results).toBeDefined();
     });
 
+    it('should reconstruct malformed responses without crashing', async () => {
+      llm.sendPromptToClaude.mockResolvedValue({
+        content: 'non-json fallback text',
+        json: { foo: 'bar' },
+      });
+      const result = await runAnalysis('/test/file.js');
+      expect(result.success).toBe(true);
+      expect(result.results.summary).toBeDefined();
+      expect(Array.isArray(result.results.issues)).toBe(true);
+    });
+
     it('should handle LLM response with null json', async () => {
       llm.sendPromptToClaude.mockResolvedValue({ json: null });
       const result = await runAnalysis('/test/file.js');