diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index 2787b966c..b736215be 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -1,6 +1,6 @@ -import { execSync } from 'child_process'; -import { existsSync, readFileSync, writeFileSync, mkdirSync, appendFileSync, realpathSync, lstatSync } from 'fs'; -import { join, dirname } from 'path'; +import { execSync, execFileSync } from 'child_process'; +import { existsSync, readFileSync, writeFileSync, mkdirSync, appendFileSync, realpathSync } from 'fs'; +import { join, dirname, resolve } from 'path'; import { VERSION } from '../version.ts'; const GBRAIN_GITHUB_REPO = 'garrytan/gbrain'; @@ -19,17 +19,23 @@ export async function runUpgrade(args: string[]) { let upgraded = false; switch (method) { - case 'bun-link': - // v0.28.5: bun-link installs are source clones. Pull + bun install - // is the upgrade path; npm/bun's update mechanism doesn't apply. - console.log('Upgrading via bun-link source clone...'); - console.log(' cd into your gbrain checkout, then run:'); - console.log(' git pull'); - console.log(' bun install'); - console.log(' bun link'); - console.log(''); - console.log(' (auto-detect can\'t do this for you because it doesn\'t know which checkout to update.)'); + case 'bun-link': { + const linkInfo = detectBunLink(); + if (!linkInfo) { + console.error('bun-link detected but could not resolve repo root.'); + break; + } + console.log(`Upgrading bun-link source clone at ${linkInfo.repoRoot}...`); + try { + execFileSync('git', ['-C', linkInfo.repoRoot, 'pull', '--ff-only'], { stdio: 'inherit', timeout: 120_000 }); + execFileSync('bun', ['install'], { cwd: linkInfo.repoRoot, stdio: 'inherit', timeout: 120_000 }); + upgraded = true; + } catch { + console.error('Auto-upgrade failed. Try manually:'); + console.error(` cd ${linkInfo.repoRoot} && git pull && bun install`); + } break; + } case 'bun': console.log('Upgrading via bun...'); @@ -298,7 +304,7 @@ export function detectInstallMethod(): 'bun' | 'bun-link' | 'binary' | 'clawhub' // resolves into a directory we can walk up from to find a .git/config // pointing at our repo. const bunLinkResult = detectBunLink(); - if (bunLinkResult === 'bun-link') return 'bun-link'; + if (bunLinkResult) return 'bun-link'; // Check if running from node_modules (bun/npm install). Could be canonical // (we publish under garrytan/gbrain) OR the squatter (npm `gbrain@1.3.x`). @@ -328,51 +334,39 @@ export function detectInstallMethod(): 'bun' | 'bun-link' | 'binary' | 'clawhub' } /** - * v0.28.5 cluster D, signal 1 — bun-link detection (closes #656). + * Detect bun-link source-clone installs (closes #656, fixes #368). * - * argv[1] is what `bun /path/to/cli.ts` was invoked with. When `bun link` - * is in play, that path is typically a symlink (~/.bun/bin/gbrain) to - * either the source repo's compiled binary or src/cli.ts directly. - * Walk up from the realpath looking for a `.git/config` whose remote - * url contains `garrytan/gbrain` (case-insensitive substring). + * Walk up from argv[1] looking for a `.git/config` whose remote url + * contains `garrytan/gbrain` (case-insensitive substring). * - * Returns 'bun-link' when we're confident; null otherwise (caller falls - * through to the existing detection chain). Best-effort: forks, tarball - * installs, detached source trees, and `.git`-less installs all fall - * through, which is acceptable per codex's plan-review feedback. + * v0.28.5 gated on lstatSync(argv1).isSymbolicLink(), but bun resolves + * the entire symlink chain before setting process.argv[1], so the check + * always returned false and short-circuited detection. Now we skip the + * symlink check and use argv[1] directly — it is already the real path + * inside the checkout, which is exactly what the git-config walk needs. + * + * Returns { repoRoot } when confident; null otherwise (caller falls + * through to the existing detection chain). */ -function detectBunLink(): 'bun-link' | null { +function detectBunLink(): { repoRoot: string } | null { try { const argv1 = process.argv[1]; if (!argv1) return null; - // Symlink check first: `bun link` always creates one. - let isSymlink = false; - try { - isSymlink = lstatSync(argv1).isSymbolicLink(); - } catch { - return null; - } - if (!isSymlink) return null; - - const resolved = realpathSync(argv1); - let dir = dirname(resolved); - // Walk up at most 6 levels looking for .git/config. + let dir = dirname(resolve(argv1)); for (let i = 0; i < 6; i++) { const gitConfigPath = join(dir, '.git', 'config'); if (existsSync(gitConfigPath)) { try { const cfg = readFileSync(gitConfigPath, 'utf-8'); - // Loose substring match: covers https://, git@, ssh://, fork URLs - // that mention upstream in [remote "upstream"], and case variants. if (cfg.toLowerCase().includes(GBRAIN_GITHUB_REPO.toLowerCase())) { - return 'bun-link'; + return { repoRoot: dir }; } } catch { /* unreadable config — not our case */ } - return null; // found .git/config but no match → not our repo + return null; } const parent = dirname(dir); - if (parent === dir) break; // reached filesystem root + if (parent === dir) break; dir = parent; } return null; diff --git a/test/upgrade.test.ts b/test/upgrade.test.ts index 2f30b3507..b19d51572 100644 --- a/test/upgrade.test.ts +++ b/test/upgrade.test.ts @@ -74,14 +74,36 @@ describe('detectInstallMethod heuristic (source analysis)', () => { // v0.28.5 cluster D: 3-signal layered detection. test('bun-link signal walks .git/config for garrytan/gbrain match', () => { - // detectBunLink reads .git/config and matches our repo name as a - // case-insensitive substring. Confirm both the function exists and - // that it does the loose substring check (not a strict URL parse). expect(source).toContain('function detectBunLink'); expect(source).toContain('GBRAIN_GITHUB_REPO'); expect(source).toContain('toLowerCase()'); }); + test('detectBunLink does not gate on isSymbolicLink (bun resolves argv[1])', () => { + // v0.28.5 gated on lstatSync(argv1).isSymbolicLink() which always + // returned false because bun resolves symlinks before setting argv[1]. + // The function body between "function detectBunLink" and the next + // top-level function must not contain isSymbolicLink. + const fnStart = source.indexOf('function detectBunLink'); + const fnEnd = source.indexOf('\nfunction ', fnStart + 1); + const fnBody = source.slice(fnStart, fnEnd > -1 ? fnEnd : undefined); + expect(fnBody).not.toContain('isSymbolicLink'); + expect(fnBody).not.toContain('lstatSync'); + }); + + test('detectBunLink returns repoRoot, not a string literal', () => { + expect(source).toContain("{ repoRoot: string } | null"); + expect(source).toContain('repoRoot: dir'); + }); + + test('bun-link upgrade uses execFileSync for shell-injection safety', () => { + // execFileSync with array args bypasses the shell (same pattern as + // dry-fix.ts:172). execSync with template strings is vulnerable to + // paths containing shell metacharacters. + expect(source).toContain("execFileSync('git', ['-C', linkInfo.repoRoot, 'pull', '--ff-only']"); + expect(source).toContain("execFileSync('bun', ['install']"); + }); + test('classifyBunInstall checks repository.url AND src/cli.ts marker', () => { // Codex feedback: repository.url alone is spoofable by future squatter // updates; the source-marker fallback (src/cli.ts presence) is