fix: [#830] project_scan 32% failure rate — defensive spawn + degraded tracking

packages/opencode/src/altimate/tools/project-scan.ts

@@ -47,28 +47,64 @@ export interface ConfigFileInfo {
 
 // --- Detection functions (exported for testing) ---
 
+/**
+ * Run a subprocess that may or may not exist on the host.
+ *
+ * `Bun.spawnSync` throws when the binary is missing from $PATH (e.g.
+ * `Executable not found in $PATH: "git"`) — not just when it exits non-zero.
+ * Telemetry shows this fingerprint hitting ~437 distinct users on
+ * `project_scan` (the binary name `"git"` was masked to `?` downstream,
+ * which made it look like a generic shell error). Wrap every spawn so a
+ * missing binary degrades gracefully instead of crashing the whole tool.
+ *
+ * Return value semantics:
+ *   - `null`         → could not spawn (binary missing, permission denied,
+ *                      Bun internal failure). Distinct from "ran and failed".
+ *   - `{exitCode: 0, stdout}`         → ran successfully.
+ *   - `{exitCode: N>0, stdout}`       → ran and exited non-zero.
+ *   - `{exitCode: 1, stdout: ""}`     → also returned when `Bun.spawnSync`
+ *                                       gives back `exitCode: null` (signaled
+ *                                       child). Coalesced to `1` because no
+ *                                       current caller distinguishes "killed
+ *                                       by signal" from "exited with status 1";
+ *                                       expand this contract if that changes.
+ */
+export function safeSpawnSync(
+  args: string[],
+  opts?: { timeout?: number },
+): { exitCode: number; stdout: string } | null {
+  try {
+    const result = Bun.spawnSync(args, {
+      stdout: "pipe",
+      stderr: "pipe",
+      ...(opts?.timeout !== undefined && { timeout: opts.timeout }),
+    })
+    return {
+      exitCode: result.exitCode ?? 1,
+      stdout: result.stdout?.toString() ?? "",
+    }
+  } catch {
+    // Binary missing from PATH, permission denied, etc. — treat as
+    // "command failed" so the caller sees a deterministic result instead
+    // of an exception bubbling up.
+    return null
+  }
+}
+
 export async function detectGit(): Promise<GitInfo> {
-  const isRepoResult = Bun.spawnSync(["git", "rev-parse", "--is-inside-work-tree"], {
-    stdout: "pipe",
-    stderr: "pipe",
-  })
-  if (isRepoResult.exitCode !== 0) {
+  const isRepoResult = safeSpawnSync(["git", "rev-parse", "--is-inside-work-tree"])
+  if (!isRepoResult || isRepoResult.exitCode !== 0) {
     return { isRepo: false }
   }
 
-  const branchResult = Bun.spawnSync(["git", "branch", "--show-current"], {
-    stdout: "pipe",
-    stderr: "pipe",
-  })
-  const branch = branchResult.exitCode === 0 ? branchResult.stdout.toString().trim() || undefined : undefined
+  const branchResult = safeSpawnSync(["git", "branch", "--show-current"])
+  const branch =
+    branchResult && branchResult.exitCode === 0 ? branchResult.stdout.trim() || undefined : undefined
 
   let remoteUrl: string | undefined
-  const remoteResult = Bun.spawnSync(["git", "remote", "get-url", "origin"], {
-    stdout: "pipe",
-    stderr: "pipe",
-  })
-  if (remoteResult.exitCode === 0) {
-    remoteUrl = remoteResult.stdout.toString().trim()
+  const remoteResult = safeSpawnSync(["git", "remote", "get-url", "origin"])
+  if (remoteResult && remoteResult.exitCode === 0) {
+    remoteUrl = remoteResult.stdout.trim()
   }
 
   return { isRepo: true, branch, remoteUrl }
@@ -335,25 +371,23 @@ export function parseToolVersion(output: string): string | undefined {
 export async function detectDataTools(skip: boolean): Promise<DataToolInfo[]> {
   if (skip) return []
 
+  // Route through safeSpawnSync for consistency with detectGit. The previous
+  // implementation used a bare try/catch around Bun.spawnSync — same outcome
+  // for the caller, but the two functions solved the same problem two
+  // different ways. Using one helper keeps the contract uniform: `null`
+  // means "could not spawn" (binary missing), distinct from "ran and exited
+  // non-zero" (binary present but the version check failed).
   const results = await Promise.all(
     DATA_TOOL_NAMES.map(async (tool): Promise<DataToolInfo> => {
-      try {
-        const result = Bun.spawnSync([tool, "--version"], {
-          stdout: "pipe",
-          stderr: "pipe",
-          timeout: 5000,
-        })
-        if (result.exitCode === 0) {
-          return {
-            name: tool,
-            installed: true,
-            version: parseToolVersion(result.stdout.toString()),
-          }
+      const result = safeSpawnSync([tool, "--version"], { timeout: 5000 })
+      if (result && result.exitCode === 0) {
+        return {
+          name: tool,
+          installed: true,
+          version: parseToolVersion(result.stdout),
         }
-        return { name: tool, installed: false }
-      } catch {
-        return { name: tool, installed: false }
       }
+      return { name: tool, installed: false }
     }),
   )
 
@@ -454,40 +488,82 @@ export const ProjectScanTool = Tool.define("project_scan", {
   async execute(args, ctx) {
     const cwd = process.cwd()
 
-    // Run local detections in parallel
+    // Track which sub-detections failed so the LLM can see partial results
+    // instead of getting a single opaque "Executable not found in $PATH: ?"
+    // failure (the previous behavior — see safeSpawnSync above).
+    const degraded: string[] = []
+
+    // Run local detections in parallel. Every detection function is now
+    // expected to fail-safe (return a "not found" or empty result) rather
+    // than throw — see detectGit + detectDataTools.
     const [git, dbtProject, envVars, dataTools, configFiles] = await Promise.all([
-      detectGit(),
-      detectDbtProject(cwd),
-      detectEnvVars(),
-      detectDataTools(!!args.skip_tools),
-      detectConfigFiles(cwd),
+      detectGit().catch(() => {
+        degraded.push("git")
+        return { isRepo: false } as GitInfo
+      }),
+      detectDbtProject(cwd).catch(() => {
+        degraded.push("dbt-project")
+        return { found: false } as DbtProjectInfo
+      }),
+      detectEnvVars().catch(() => {
+        degraded.push("env-vars")
+        return [] as EnvVarConnection[]
+      }),
+      detectDataTools(!!args.skip_tools).catch(() => {
+        degraded.push("data-tools")
+        return [] as DataToolInfo[]
+      }),
+      detectConfigFiles(cwd).catch(() => {
+        degraded.push("config-files")
+        return { altimateConfig: false, sqlfluff: false, preCommit: false } as ConfigFileInfo
+      }),
     ])
 
-    // Run bridge-dependent detections with individual error handling
+    // Run bridge-dependent detections with individual error handling. A
+    // dispatcher failure on any one of these is expected (Python engine may
+    // not be running locally) — degrade silently and report in metadata.
     const engineHealth = await Dispatcher.call("ping", {} as any)
       .then((r) => ({ healthy: true, status: r.status }))
-      .catch(() => ({ healthy: false, status: undefined as string | undefined }))
+      .catch(() => {
+        degraded.push("python-engine")
+        return { healthy: false, status: undefined as string | undefined }
+      })
 
     const existingConnections = await Dispatcher.call("warehouse.list", {})
       .then((r) => r.warehouses)
-      .catch(() => [] as Array<{ name: string; type: string; database?: string }>)
+      .catch(() => {
+        degraded.push("warehouse.list")
+        return [] as Array<{ name: string; type: string; database?: string }>
+      })
 
     const dbtProfiles = await Dispatcher.call("dbt.profiles", {
       projectDir: dbtProject.found ? dbtProject.path : undefined,
     })
       .then((r) => r.connections ?? [])
-      .catch(() => [] as Array<{ name: string; type: string; config: Record<string, unknown> }>)
+      .catch(() => {
+        degraded.push("dbt.profiles")
+        return [] as Array<{ name: string; type: string; config: Record<string, unknown> }>
+      })
 
     const dockerContainers = args.skip_docker
       ? []
       : await Dispatcher.call("warehouse.discover", {} as any)
           .then((r) => r.containers ?? [])
-          .catch(() => [] as Array<{ name: string; db_type: string; host: string; port: number; database?: string }>)
-
-    const schemaCache = await Dispatcher.call("schema.cache_status", {}).catch(() => null)
+          .catch(() => {
+            degraded.push("warehouse.discover")
+            return [] as Array<{ name: string; db_type: string; host: string; port: number; database?: string }>
+          })
+
+    const schemaCache = await Dispatcher.call("schema.cache_status", {}).catch(() => {
+      degraded.push("schema.cache_status")
+      return null
+    })
 
     const dbtManifest = dbtProject.manifestPath
-      ? await Dispatcher.call("dbt.manifest", { path: dbtProject.manifestPath }).catch(() => null)
+      ? await Dispatcher.call("dbt.manifest", { path: dbtProject.manifestPath }).catch(() => {
+          degraded.push("dbt.manifest")
+          return null
+        })
       : null
 
     // Deduplicate connections
@@ -549,7 +625,10 @@ export const ProjectScanTool = Tool.define("project_scan", {
           suggestionsShown: ["dbt-develop", "dbt-troubleshoot", "dbt-analyze"],
         })
       } catch {
-        // Telemetry must never break scan output
+        // Telemetry must never break scan output, but record it in the
+        // degraded list so we can see post-deploy whether the dynamic
+        // import is silently failing for some users.
+        degraded.push("post-connect-suggestions")
       }
       // altimate_change end
     } else {
@@ -655,7 +734,12 @@ export const ProjectScanTool = Tool.define("project_scan", {
     if (connections.newFromDocker.length > 0) connectionSources.push("docker")
     if (connections.newFromEnv.length > 0) connectionSources.push("env-var")
 
-    const mcpConfig = (await Config.get()).mcp ?? {}
+    const mcpConfig = await Config.get()
+      .then((c) => c.mcp ?? {})
+      .catch(() => {
+        degraded.push("config")
+        return {}
+      })
     const mcpServerCount = Object.keys(mcpConfig).length
 
     const enabledFlags: string[] = []
@@ -669,7 +753,10 @@ export const ProjectScanTool = Tool.define("project_scan", {
 
     const skillCount = await Skill.all()
       .then((s) => s.length)
-      .catch(() => 0)
+      .catch(() => {
+        degraded.push("skills")
+        return 0
+      })
 
     Telemetry.track({
       type: "environment_census",
@@ -702,11 +789,26 @@ export const ProjectScanTool = Tool.define("project_scan", {
       feature_flags: enabledFlags,
     })
 
+    // Surface degraded detections in the output so the LLM can recommend
+    // installing the missing pieces without thinking the tool itself failed.
+    if (degraded.length > 0) {
+      lines.push("")
+      lines.push("## Degraded Detections")
+      lines.push(
+        `The following sub-detections failed and were skipped (the rest of the scan is still valid):`,
+      )
+      for (const d of degraded) {
+        lines.push(`- ${d}`)
+      }
+    }
+
     // Build metadata
     const toolsFound = dataTools.filter((t) => t.installed).map((t) => t.name)
 
     return {
-      title: `Scan: ${totalConnections} connection(s), ${dbtProject.found ? "dbt found" : "no dbt"}`,
+      title: `Scan: ${totalConnections} connection(s), ${dbtProject.found ? "dbt found" : "no dbt"}${
+        degraded.length > 0 ? ` (${degraded.length} degraded)` : ""
+      }`,
       metadata: {
         engine_healthy: engineHealth.healthy,
         git: { isRepo: git.isRepo, branch: git.branch },
@@ -729,6 +831,7 @@ export const ProjectScanTool = Tool.define("project_scan", {
             }
           : { warehouses: 0, tables: 0, columns: 0 },
         tools_found: toolsFound,
+        degraded: degraded.length > 0 ? degraded : undefined,
       },
       output: lines.join("\n"),
     }

packages/opencode/test/tool/project-scan.test.ts

@@ -11,6 +11,7 @@ import {
   detectDataTools,
   detectConfigFiles,
   parseToolVersion,
+  safeSpawnSync,
   DATA_TOOL_NAMES,
   type GitInfo,
   type DbtProjectInfo,
@@ -119,6 +120,34 @@ describe("detectGit", () => {
     }
   })
 
+  test("does not throw when git binary is missing from PATH", async () => {
+    // Telemetry-2026-05-21 showed `project_scan` failing 32% of the time with
+    // `Executable not found in $PATH: ?` — the binary was masked to `?` by the
+    // PII filter, but the underlying cause was Bun.spawnSync throwing on a
+    // missing git executable. This test pins the fix: detectGit must return a
+    // sentinel result rather than throw, so the rest of project_scan still
+    // runs.
+    const dir = nextTmpDir()
+    await fsp.mkdir(dir, { recursive: true })
+
+    const originalPath = process.env.PATH
+    const originalCwd = process.cwd()
+    // Point PATH at an empty directory so the shell can't find git (or any
+    // other binary). Bun.spawnSync will throw "Executable not found in $PATH"
+    // — which the safeSpawnSync wrapper should catch.
+    process.env.PATH = dir
+    process.chdir(dir)
+    try {
+      const result = await detectGit()
+      expect(result.isRepo).toBe(false)
+      expect(result.branch).toBeUndefined()
+      expect(result.remoteUrl).toBeUndefined()
+    } finally {
+      process.chdir(originalCwd)
+      process.env.PATH = originalPath
+    }
+  })
+
   afterAll(async () => {
     await fsp.rm(tmpRoot, { recursive: true, force: true }).catch(() => {})
   })
@@ -902,3 +931,41 @@ describe("return type contracts", () => {
     expect(typeof result.preCommit).toBe("boolean")
   })
 })
+
+// ---------------------------------------------------------------------------
+// safeSpawnSync — the new primitive both detectGit and detectDataTools route through
+// ---------------------------------------------------------------------------
+
+describe("safeSpawnSync", () => {
+  test("returns null when the binary is missing from PATH", () => {
+    const result = safeSpawnSync(["this-binary-does-not-exist-anywhere-on-the-system-xyz123"])
+    expect(result).toBeNull()
+  })
+
+  test("returns { exitCode, stdout } when the binary runs successfully", () => {
+    // Use `node --version` — present on any system that can run this test.
+    const result = safeSpawnSync(["node", "--version"])
+    expect(result).not.toBeNull()
+    if (result) {
+      expect(result.exitCode).toBe(0)
+      expect(result.stdout).toMatch(/^v?\d+\.\d+/)
+    }
+  })
+
+  test("returns { exitCode: nonzero } when the binary exits non-zero", () => {
+    // `node -e 'process.exit(7)'` runs but exits 7.
+    const result = safeSpawnSync(["node", "-e", "process.exit(7)"])
+    expect(result).not.toBeNull()
+    if (result) {
+      expect(result.exitCode).toBe(7)
+    }
+  })
+
+  test("forwards the timeout option to Bun.spawnSync", () => {
+    // Just verify the helper doesn't crash when given timeout — actually
+    // exercising the timeout path requires a long-running command, which
+    // is flaky in CI. Smoke-check the wiring instead.
+    const result = safeSpawnSync(["node", "--version"], { timeout: 5000 })
+    expect(result).not.toBeNull()
+  })
+})