-
Notifications
You must be signed in to change notification settings - Fork 290
feat: prevent wrapper command collisions + add --prefix (non-breaking) #44
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||||||||||||||||||||||
| import fs from 'node:fs'; | ||||||||||||||||||||||||||
| import { spawnSync } from 'node:child_process'; | ||||||||||||||||||||||||||
| import os from 'node:os'; | ||||||||||||||||||||||||||
| import path from 'node:path'; | ||||||||||||||||||||||||||
|
|
@@ -42,3 +43,53 @@ export const commandExists = (cmd: string): boolean => { | |||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| return result.status === 0 && result.stdout.trim().length > 0; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export interface CommandCollisionCheck { | ||||||||||||||||||||||||||
| wrapperPath: string; | ||||||||||||||||||||||||||
| wrapperExists: boolean; | ||||||||||||||||||||||||||
| binDirOnPath: boolean; | ||||||||||||||||||||||||||
| resolvedCommandPath: string | null; | ||||||||||||||||||||||||||
| pathConflicts: boolean; | ||||||||||||||||||||||||||
| hasCollision: boolean; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const normalizeFsPath = (input: string): string => { | ||||||||||||||||||||||||||
| const normalized = path.normalize(input); | ||||||||||||||||||||||||||
| return isWindows ? normalized.toLowerCase() : normalized; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export const resolveCommandPath = (cmd: string): string | null => { | ||||||||||||||||||||||||||
| const result = spawnSync(process.platform === 'win32' ? 'where' : 'which', [cmd], { | ||||||||||||||||||||||||||
| encoding: 'utf8', | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| if (result.status !== 0 || !result.stdout) return null; | ||||||||||||||||||||||||||
| const firstLine = result.stdout | ||||||||||||||||||||||||||
| .split(/\r?\n/) | ||||||||||||||||||||||||||
| .map((line) => line.trim()) | ||||||||||||||||||||||||||
| .find(Boolean); | ||||||||||||||||||||||||||
| return firstLine || null; | ||||||||||||||||||||||||||
|
Comment on lines
+61
to
+70
|
||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export const detectCommandCollision = (name: string, binDir: string): CommandCollisionCheck => { | ||||||||||||||||||||||||||
| const resolvedBin = expandTilde(binDir) ?? binDir; | ||||||||||||||||||||||||||
| const wrapperPath = getWrapperPath(resolvedBin, name); | ||||||||||||||||||||||||||
| const wrapperExists = fs.existsSync(wrapperPath); | ||||||||||||||||||||||||||
| const pathEntries = (process.env.PATH || '') | ||||||||||||||||||||||||||
| .split(path.delimiter) | ||||||||||||||||||||||||||
| .map((entry) => entry.trim()) | ||||||||||||||||||||||||||
| .filter(Boolean); | ||||||||||||||||||||||||||
| const normalizedResolvedBin = normalizeFsPath(resolvedBin); | ||||||||||||||||||||||||||
| const binDirOnPath = pathEntries.some((entry) => normalizeFsPath(entry) === normalizedResolvedBin); | ||||||||||||||||||||||||||
| const resolvedCommandPath = resolveCommandPath(name); | ||||||||||||||||||||||||||
| const pathConflicts = Boolean( | ||||||||||||||||||||||||||
| binDirOnPath && resolvedCommandPath && normalizeFsPath(resolvedCommandPath) !== normalizeFsPath(wrapperPath) | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
|
Comment on lines
+83
to
+86
|
||||||||||||||||||||||||||
| const resolvedCommandPath = resolveCommandPath(name); | |
| const pathConflicts = Boolean( | |
| binDirOnPath && resolvedCommandPath && normalizeFsPath(resolvedCommandPath) !== normalizeFsPath(wrapperPath) | |
| ); | |
| let resolvedCommandPath: string | null = null; | |
| let pathConflicts = false; | |
| if (binDirOnPath) { | |
| resolvedCommandPath = resolveCommandPath(name); | |
| pathConflicts = Boolean( | |
| resolvedCommandPath && normalizeFsPath(resolvedCommandPath) !== normalizeFsPath(wrapperPath) | |
| ); | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import { useEffect, useRef } from 'react'; | |
| import path from 'node:path'; | ||
| import type { CoreModule } from '../app.js'; | ||
| import type { CreateVariantParams, CompletionResult, ModelOverrides } from './types.js'; | ||
| import { detectCommandCollision } from '../../core/paths.js'; | ||
|
|
||
| export interface UseVariantCreateOptions { | ||
| screen: string; | ||
|
|
@@ -91,6 +92,22 @@ export function useVariantCreate(options: UseVariantCreateOptions): void { | |
|
|
||
| const runCreate = async () => { | ||
| try { | ||
| const collision = detectCommandCollision(params.name, params.binDir); | ||
| if (collision.hasCollision) { | ||
| const suggested = params.provider?.defaultVariantName || `cc${params.providerKey}`; | ||
| const reasons: string[] = []; | ||
| if (collision.wrapperExists) { | ||
| reasons.push(`wrapper exists at ${collision.wrapperPath}`); | ||
| } | ||
| if (collision.pathConflicts && collision.resolvedCommandPath) { | ||
| reasons.push(`'${params.name}' resolves to ${collision.resolvedCommandPath}`); | ||
| } | ||
| throw new Error( | ||
| `Command name collision for "${params.name}": ${reasons.join('; ')}. ` + | ||
| `Choose another name (suggested: "${suggested}").` | ||
| ); | ||
| } | ||
|
Comment on lines
+95
to
+109
|
||
|
|
||
| setProgressLines(() => []); | ||
| const createParams = { | ||
| name: params.name, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,57 @@ | ||||||||||||||
| import test from 'node:test'; | ||||||||||||||
| import assert from 'node:assert/strict'; | ||||||||||||||
| import fs from 'node:fs'; | ||||||||||||||
|
|
||||||||||||||
| import { cleanup, makeTempDir } from '../helpers/index.js'; | ||||||||||||||
| import { detectCommandCollision, getWrapperPath, resolveCommandPath } from '../../src/core/paths.js'; | ||||||||||||||
|
|
||||||||||||||
| test('resolveCommandPath returns null for unknown command', () => { | ||||||||||||||
| const resolved = resolveCommandPath('cc_mirror_command_that_should_not_exist_12345'); | ||||||||||||||
| assert.equal(resolved, null); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| test('detectCommandCollision reports wrapper collisions', () => { | ||||||||||||||
| const binDir = makeTempDir(); | ||||||||||||||
| try { | ||||||||||||||
| const wrapperPath = getWrapperPath(binDir, 'samplecmd'); | ||||||||||||||
| fs.writeFileSync(wrapperPath, '#!/usr/bin/env bash\necho hello\n'); | ||||||||||||||
| const result = detectCommandCollision('samplecmd', binDir); | ||||||||||||||
| assert.equal(result.wrapperExists, true); | ||||||||||||||
| assert.equal(result.hasCollision, true); | ||||||||||||||
| } finally { | ||||||||||||||
| cleanup(binDir); | ||||||||||||||
| } | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| test('detectCommandCollision reports path conflicts for existing system commands', () => { | ||||||||||||||
| const resolvedNode = resolveCommandPath('node'); | ||||||||||||||
| if (!resolvedNode) return; | ||||||||||||||
|
|
||||||||||||||
| const binDir = makeTempDir(); | ||||||||||||||
| try { | ||||||||||||||
| const result = detectCommandCollision('node', binDir); | ||||||||||||||
| assert.equal(result.binDirOnPath, false); | ||||||||||||||
| assert.equal(result.pathConflicts, false); | ||||||||||||||
| assert.equal(result.hasCollision, false); | ||||||||||||||
| } finally { | ||||||||||||||
| cleanup(binDir); | ||||||||||||||
| } | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| test('detectCommandCollision marks path conflict when bin dir is on PATH', () => { | ||||||||||||||
| const resolvedNode = resolveCommandPath('node'); | ||||||||||||||
| if (!resolvedNode) return; | ||||||||||||||
|
|
||||||||||||||
| const binDir = makeTempDir(); | ||||||||||||||
| const previousPath = process.env.PATH; | ||||||||||||||
| try { | ||||||||||||||
| process.env.PATH = `${binDir}${process.platform === 'win32' ? ';' : ':'}${previousPath || ''}`; | ||||||||||||||
| const result = detectCommandCollision('node', binDir); | ||||||||||||||
| assert.equal(result.binDirOnPath, true); | ||||||||||||||
| assert.equal(result.pathConflicts, true); | ||||||||||||||
| assert.equal(result.hasCollision, true); | ||||||||||||||
| } finally { | ||||||||||||||
| process.env.PATH = previousPath; | ||||||||||||||
|
||||||||||||||
| process.env.PATH = previousPath; | |
| if (previousPath === undefined) { | |
| delete process.env.PATH; | |
| } else { | |
| process.env.PATH = previousPath; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collision checking runs before variant-name validation, so an invalid
--namecan surface a collision error (or trigger oddwhich/wherebehavior) instead of the intended “invalid variant name” error from the core builder. Consider validatingname(via the sameassertValidVariantNameused in core) before callingassertNoCommandCollision(), so users get the correct error and collision checks only run on valid command names.