From 0d251d97073253daae8a17f60f3869042df060ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?T=C3=82CHES?= Date: Wed, 11 Mar 2026 10:52:45 -0600 Subject: [PATCH] fix: bootstrap managed tools and gh auth Preserve the original #39 fix while adding the missing hardening and regression coverage. Credit to @LuxVTZ for the original fix incorporated here. --- src/cli.ts | 8 ++- src/resources/extensions/github/gh-api.ts | 76 ++++++++++++-------- src/tests/gh-api.test.ts | 52 ++++++++++++++ src/tests/tool-bootstrap.test.ts | 73 +++++++++++++++++++ src/tool-bootstrap.ts | 85 +++++++++++++++++++++++ 5 files changed, 263 insertions(+), 31 deletions(-) create mode 100644 src/tests/gh-api.test.ts create mode 100644 src/tests/tool-bootstrap.test.ts create mode 100644 src/tool-bootstrap.ts diff --git a/src/cli.ts b/src/cli.ts index 59cc8ec01..17cf193d0 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -6,11 +6,17 @@ import { createAgentSession, InteractiveMode, } from '@mariozechner/pi-coding-agent' -import { join } from 'path' +import { join } from 'node:path' import { agentDir, sessionsDir, authFilePath } from './app-paths.js' import { buildResourceLoader, initResources } from './resource-loader.js' +import { ensureManagedTools } from './tool-bootstrap.js' import { loadStoredEnvKeys, runWizardIfNeeded } from './wizard.js' +// Pi's tool bootstrap can mis-detect already-installed fd/rg on some systems +// because spawnSync(..., ["--version"]) returns EPERM despite a zero exit code. +// Provision local managed binaries first so Pi sees them without probing PATH. +ensureManagedTools(join(agentDir, 'bin')) + const authStorage = AuthStorage.create(authFilePath) loadStoredEnvKeys(authStorage) await runWizardIfNeeded(authStorage) diff --git a/src/resources/extensions/github/gh-api.ts b/src/resources/extensions/github/gh-api.ts index 6edc8b174..ccdeba8da 100644 --- a/src/resources/extensions/github/gh-api.ts +++ b/src/resources/extensions/github/gh-api.ts @@ -6,20 +6,44 @@ * Falls back to raw REST API with GITHUB_TOKEN env var. */ -import { execSync } from "node:child_process"; +import { execSync, spawnSync, type SpawnSyncReturns } from "node:child_process"; // ─── Auth detection ─────────────────────────────────────────────────────────── let _useGhCli: boolean | null = null; -function hasGhCli(): boolean { +let ghSpawnImpl = (args: string[], input?: string, cwd?: string): SpawnSyncReturns => + spawnSync("gh", args, { + cwd, + encoding: "utf8", + stdio: ["pipe", "pipe", "pipe"], + input, + }); + +function ghSpawn(args: string[], input?: string, cwd?: string): SpawnSyncReturns { + return ghSpawnImpl(args, input, cwd); +} + +export function resetGhCliDetectionForTests(): void { + _useGhCli = null; + ghSpawnImpl = (args: string[], input?: string, cwd?: string): SpawnSyncReturns => + spawnSync("gh", args, { + cwd, + encoding: "utf8", + stdio: ["pipe", "pipe", "pipe"], + input, + }); +} + +export function setGhSpawnForTests(fn: (args: string[], input?: string, cwd?: string) => SpawnSyncReturns): void { + ghSpawnImpl = fn; + _useGhCli = null; +} + +export function hasGhCli(): boolean { if (_useGhCli !== null) return _useGhCli; - try { - execSync("gh auth status", { encoding: "utf8", stdio: ["pipe", "pipe", "pipe"] }); - _useGhCli = true; - } catch { - _useGhCli = false; - } + const result = ghSpawn(["auth", "token"]); + _useGhCli = result.status === 0 && !result.error && !!result.stdout?.trim(); return _useGhCli; } @@ -120,11 +144,6 @@ export async function ghApi( return fetchApi(endpoint, method, options.params, options.body, token); } -function shellEscape(s: string): string { - // Single-quote wrapping, escaping any existing single quotes - return "'" + s.replace(/'/g, "'\\''") + "'"; -} - function ghCliApi( endpoint: string, method: string, @@ -132,39 +151,36 @@ function ghCliApi( body?: Record, cwd?: string, ): T { - const parts = ["gh", "api", shellEscape(endpoint), "--method", method]; + const args = ["api", endpoint, "--method", method]; if (params) { for (const [key, val] of Object.entries(params)) { if (val === undefined) continue; if (Array.isArray(val)) { for (const v of val) { - parts.push("-f", shellEscape(`${key}[]=${v}`)); + args.push("-f", `${key}[]=${v}`); } } else { - parts.push("-f", shellEscape(`${key}=${String(val)}`)); + args.push("-f", `${key}=${String(val)}`); } } } if (body) { - parts.push("--input", "-"); + args.push("--input", "-"); } - try { - const result = execSync(parts.join(" "), { - cwd: cwd ?? process.cwd(), - encoding: "utf8", - stdio: ["pipe", "pipe", "pipe"], - input: body ? JSON.stringify(body) : undefined, - }); - if (!result.trim()) return {} as T; - return JSON.parse(result) as T; - } catch (e: unknown) { - const err = e as { stderr?: string; stdout?: string; message?: string }; - const msg = err.stderr?.trim() || err.stdout?.trim() || err.message || String(e); - throw new Error(`gh api error: ${msg}`); + const result = ghSpawn(args, body ? JSON.stringify(body) : undefined, cwd ?? process.cwd()); + + const stdout = result.stdout?.trim() ?? ""; + const stderr = result.stderr?.trim() ?? ""; + + if (result.status !== 0) { + throw new Error(`gh api error: ${stderr || stdout || result.error?.message || `exit code ${result.status}`}`); } + + if (!stdout) return {} as T; + return JSON.parse(stdout) as T; } async function fetchApi( diff --git a/src/tests/gh-api.test.ts b/src/tests/gh-api.test.ts new file mode 100644 index 000000000..3d2ef5b77 --- /dev/null +++ b/src/tests/gh-api.test.ts @@ -0,0 +1,52 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { spawnSync as realSpawnSync } from "node:child_process"; + +import * as ghApiModule from "../resources/extensions/github/gh-api.ts"; + +function makeSpawnResult(overrides: Partial>): ReturnType { + return { + status: 0, + stdout: "", + stderr: "", + output: [null, "", ""], + pid: 1, + signal: null, + ...overrides, + } as ReturnType; +} + +test("hasGhCli treats zero-exit token output as authenticated", () => { + ghApiModule.setGhSpawnForTests(() => makeSpawnResult({ stdout: "gho_test\n" })); + + try { + assert.equal(ghApiModule.hasGhCli(), true); + assert.equal(ghApiModule.authMethod(), "gh CLI"); + } finally { + ghApiModule.resetGhCliDetectionForTests(); + } +}); + +test("hasGhCli rejects zero-exit responses with empty stdout", () => { + ghApiModule.setGhSpawnForTests(() => makeSpawnResult({ stdout: "" })); + + try { + assert.equal(ghApiModule.hasGhCli(), false); + } finally { + ghApiModule.resetGhCliDetectionForTests(); + } +}); + +test("hasGhCli rejects spawnSync error even with zero exit", () => { + ghApiModule.setGhSpawnForTests(() => makeSpawnResult({ + stdout: "gho_test\n", + stderr: "EPERM", + error: new Error("spawnSync gh EPERM"), + })); + + try { + assert.equal(ghApiModule.hasGhCli(), false); + } finally { + ghApiModule.resetGhCliDetectionForTests(); + } +}); diff --git a/src/tests/tool-bootstrap.test.ts b/src/tests/tool-bootstrap.test.ts new file mode 100644 index 000000000..b19b6740b --- /dev/null +++ b/src/tests/tool-bootstrap.test.ts @@ -0,0 +1,73 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { chmodSync, existsSync, lstatSync, mkdtempSync, mkdirSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +import { ensureManagedTools, resolveToolFromPath } from "../tool-bootstrap.js"; + +function makeExecutable(dir: string, name: string, content = "#!/bin/sh\nexit 0\n"): string { + const file = join(dir, name); + writeFileSync(file, content); + chmodSync(file, 0o755); + return file; +} + +test("resolveToolFromPath finds fd via fdfind fallback", () => { + const tmp = mkdtempSync(join(tmpdir(), "gsd-tool-bootstrap-resolve-")); + try { + makeExecutable(tmp, "fdfind"); + const resolved = resolveToolFromPath("fd", tmp); + assert.equal(resolved, join(tmp, "fdfind")); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } +}); + +test("ensureManagedTools provisions fd and rg into managed bin dir", () => { + const tmp = mkdtempSync(join(tmpdir(), "gsd-tool-bootstrap-provision-")); + const sourceBin = join(tmp, "source-bin"); + const targetBin = join(tmp, "target-bin"); + + mkdirSync(sourceBin, { recursive: true }); + mkdirSync(targetBin, { recursive: true }); + + try { + makeExecutable(sourceBin, "fdfind"); + makeExecutable(sourceBin, "rg"); + + const provisioned = ensureManagedTools(targetBin, sourceBin); + + assert.equal(provisioned.length, 2); + assert.ok(existsSync(join(targetBin, "fd"))); + assert.ok(existsSync(join(targetBin, "rg"))); + assert.ok(lstatSync(join(targetBin, "fd")).isSymbolicLink() || lstatSync(join(targetBin, "fd")).isFile()); + assert.ok(lstatSync(join(targetBin, "rg")).isSymbolicLink() || lstatSync(join(targetBin, "rg")).isFile()); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } +}); + +test("ensureManagedTools copies executable when symlink target already exists as a broken link", () => { + const tmp = mkdtempSync(join(tmpdir(), "gsd-tool-bootstrap-copy-")); + const sourceBin = join(tmp, "source-bin"); + const targetBin = join(tmp, "target-bin"); + const targetFd = join(targetBin, "fd"); + + mkdirSync(sourceBin, { recursive: true }); + mkdirSync(targetBin, { recursive: true }); + + try { + makeExecutable(sourceBin, "fdfind", "#!/bin/sh\necho fd\n"); + makeExecutable(sourceBin, "rg", "#!/bin/sh\necho rg\n"); + symlinkSync(join(tmp, "missing-target"), targetFd); + + const provisioned = ensureManagedTools(targetBin, sourceBin); + + assert.equal(provisioned.length, 2); + assert.ok(lstatSync(targetFd).isFile(), "fd fallback should replace broken symlink with a copied file"); + assert.match(readFileSync(targetFd, "utf8"), /echo fd/); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } +}); diff --git a/src/tool-bootstrap.ts b/src/tool-bootstrap.ts new file mode 100644 index 000000000..349133250 --- /dev/null +++ b/src/tool-bootstrap.ts @@ -0,0 +1,85 @@ +import { chmodSync, copyFileSync, existsSync, lstatSync, mkdirSync, rmSync, symlinkSync } from "node:fs"; +import { delimiter, join } from "node:path"; + +type ManagedTool = "fd" | "rg"; + +interface ToolSpec { + targetName: string; + candidates: string[]; +} + +const TOOL_SPECS: Record = { + fd: { + targetName: process.platform === "win32" ? "fd.exe" : "fd", + candidates: process.platform === "win32" ? ["fd.exe", "fd", "fdfind.exe", "fdfind"] : ["fd", "fdfind"], + }, + rg: { + targetName: process.platform === "win32" ? "rg.exe" : "rg", + candidates: process.platform === "win32" ? ["rg.exe", "rg"] : ["rg"], + }, +}; + +function splitPath(pathValue: string | undefined): string[] { + if (!pathValue) return []; + return pathValue.split(delimiter).map((segment) => segment.trim()).filter(Boolean); +} + +function getCandidateNames(name: string): string[] { + if (process.platform !== "win32") return [name]; + const lower = name.toLowerCase(); + if (lower.endsWith(".exe") || lower.endsWith(".cmd") || lower.endsWith(".bat")) return [name]; + return [name, `${name}.exe`, `${name}.cmd`, `${name}.bat`]; +} + +function isRegularFile(path: string): boolean { + try { + return lstatSync(path).isFile() || lstatSync(path).isSymbolicLink(); + } catch { + return false; + } +} + +export function resolveToolFromPath(tool: ManagedTool, pathValue: string | undefined = process.env.PATH): string | null { + const spec = TOOL_SPECS[tool]; + for (const dir of splitPath(pathValue)) { + for (const candidate of spec.candidates) { + for (const name of getCandidateNames(candidate)) { + const fullPath = join(dir, name); + if (existsSync(fullPath) && isRegularFile(fullPath)) { + return fullPath; + } + } + } + } + return null; +} + +function provisionTool(targetDir: string, tool: ManagedTool, sourcePath: string): string { + const targetPath = join(targetDir, TOOL_SPECS[tool].targetName); + if (existsSync(targetPath)) return targetPath; + + mkdirSync(targetDir, { recursive: true }); + + try { + symlinkSync(sourcePath, targetPath); + } catch { + rmSync(targetPath, { force: true }); + copyFileSync(sourcePath, targetPath); + chmodSync(targetPath, 0o755); + } + + return targetPath; +} + +export function ensureManagedTools(targetDir: string, pathValue: string | undefined = process.env.PATH): string[] { + const provisioned: string[] = []; + + for (const tool of Object.keys(TOOL_SPECS) as ManagedTool[]) { + if (existsSync(join(targetDir, TOOL_SPECS[tool].targetName))) continue; + const sourcePath = resolveToolFromPath(tool, pathValue); + if (!sourcePath) continue; + provisioned.push(provisionTool(targetDir, tool, sourcePath)); + } + + return provisioned; +}