feat: prevent wrapper command collisions + add --prefix (non-breaking)#44
feat: prevent wrapper command collisions + add --prefix (non-breaking)#44starworld wants to merge 3 commits intonumman-ali:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds collision detection to prevent wrapper scripts from shadowing existing CLI commands, introduces an opt-in --prefix naming helper when --name is omitted, and provides an escape hatch via --allow-collision.
Changes:
- Implement command-collision detection (existing wrapper +
$PATHconflicts when binDir is on$PATH) and enforce it by default. - Add CLI support for
--prefix(default naming) and--allow-collision(bypass guard). - Add tests covering collision detection and new CLI args parsing.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/core/paths.test.ts | Adds unit tests for command resolution and collision detection behavior. |
| test/cli/args.test.ts | Adds coverage for parsing --prefix and --allow-collision. |
| src/tui/hooks/useVariantCreate.ts | Adds TUI fail-fast collision detection before creating a variant. |
| src/core/variant-builder/VariantBuilder.ts | Enforces collision blocking in core variant creation unless allowCollision is set. |
| src/core/types.ts | Introduces allowCollision?: boolean in CreateVariantParams. |
| src/core/paths.ts | Adds resolveCommandPath() + detectCommandCollision() utilities. |
| src/core/index.ts | Re-exports detectCommandCollision from core entrypoint. |
| src/cli/help.ts | Documents --prefix and --allow-collision in CLI help output. |
| src/cli/commands/create.ts | Wires up --prefix naming, collision checks, and --allow-collision passthrough. |
| src/cli/args.ts | Extends ParsedArgs typing to include prefix and allow-collision. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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}").` | ||
| ); | ||
| } |
There was a problem hiding this comment.
The TUI now performs collision detection before calling into core, but this also happens before core’s assertValidVariantName() validation. If the user enters an invalid name (e.g., starting with - or containing /), the error shown may be a collision/resolve error rather than the “invalid variant name” message. Consider validating the name before running detectCommandCollision() so the failure mode is deterministic and the error message is accurate.
| assert.equal(result.pathConflicts, true); | ||
| assert.equal(result.hasCollision, true); | ||
| } finally { | ||
| process.env.PATH = previousPath; |
There was a problem hiding this comment.
previousPath can be undefined in some environments; assigning it back via process.env.PATH = previousPath will coerce to the string "undefined" and can break subsequent tests. In the finally, restore with delete process.env.PATH when previousPath is undefined, otherwise set it back to the prior string value.
| process.env.PATH = previousPath; | |
| if (previousPath === undefined) { | |
| delete process.env.PATH; | |
| } else { | |
| process.env.PATH = previousPath; | |
| } |
| 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; |
There was a problem hiding this comment.
resolveCommandPath() will treat names starting with - as options to which/where (e.g., --name=-h), which can yield incorrect results or unexpected output. Consider guarding against option-like commands (e.g., early-return null when cmd starts with -) or passing an argument terminator where supported, so collision checks can’t be influenced by flag parsing.
| const resolvedCommandPath = resolveCommandPath(name); | ||
| const pathConflicts = Boolean( | ||
| binDirOnPath && resolvedCommandPath && normalizeFsPath(resolvedCommandPath) !== normalizeFsPath(wrapperPath) | ||
| ); |
There was a problem hiding this comment.
detectCommandCollision() always calls resolveCommandPath() (spawns which/where) even when binDir is not on $PATH, but the result is only used when binDirOnPath is true. To match the PR’s “only when binDir is on PATH” behavior and avoid extra process spawns, compute resolvedCommandPath lazily only when binDirOnPath is true.
| 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) | |
| ); | |
| } |
| const assertNoCommandCollision = ( | ||
| name: string, | ||
| binDir: string, | ||
| provider: ProviderTemplate, | ||
| providerKey: string, | ||
| allowCollision: boolean | ||
| ): void => { | ||
| const collision = core.detectCommandCollision(name, binDir); | ||
| if (!collision.hasCollision || allowCollision) return; | ||
|
|
||
| const suggested = provider.defaultVariantName || `cc${providerKey}`; | ||
| const reasons: string[] = []; | ||
| if (collision.wrapperExists) { | ||
| reasons.push(`wrapper already exists at ${collision.wrapperPath}`); | ||
| } | ||
| if (collision.pathConflicts && collision.resolvedCommandPath) { | ||
| reasons.push(`'${name}' already resolves to ${collision.resolvedCommandPath}`); | ||
| } | ||
|
|
||
| throw new Error( | ||
| `Command name collision for "${name}": ${reasons.join('; ')}. ` + | ||
| `Use --name <unique-name> (suggested: "${suggested}") or --allow-collision to bypass.` | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Collision checking runs before variant-name validation, so an invalid --name can surface a collision error (or trigger odd which/where behavior) instead of the intended “invalid variant name” error from the core builder. Consider validating name (via the same assertValidVariantName used in core) before calling assertNoCommandCollision(), so users get the correct error and collision checks only run on valid command names.
… compact fix - Update model IDs: Opus/Sonnet → claude-*-4-6, Haiku → 4.5-20251001 - Port PR numman-ali#44: --prefix and --allow-collision flags prevent wrapper shadowing - Port PR numman-ali#43: expandTilde ~\ fix, listVariants tilde expansion, windows-path.ts - Mitigate numman-ali#29: set ANTHROPIC_SMALL_FAST_MODEL for mirror provider compaction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Prevent accidental shadowing/overwriting of existing CLI commands by cc-mirror wrapper scripts.
This PR is non-breaking: no provider defaults are changed. It adds safe guards and an opt-in naming helper.
Changes
--prefix <value>(applies when--nameis omitted)--allow-collisionto bypass (unsafe)Why
Users can have existing tools named like providers (e.g.
kimi), and creating a variant with the same name can shadow or overwrite the original command on$PATH.Notes
Collision detection considers
$PATHconflicts only when the targetbinDiris actually on$PATH, plus the existing-wrapper-file case.