Skip to content

fix(upgrade): detectBunLink fails because bun resolves symlinks in argv[1] #704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MrAladdin wants to merge 1 commit into from
Closed

fix(upgrade): detectBunLink fails because bun resolves symlinks in argv[1] #704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 36 additions & 42 deletions src/commands/upgrade.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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...');
Expand Down Expand Up @@ -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`).
Expand Down Expand Up @@ -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;
Expand Down
28 changes: 25 additions & 3 deletions test/upgrade.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down