Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
46 changes: 38 additions & 8 deletions src/commands/cli/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ import {
import { CommandOutput } from "../../lib/formatters/output.js";
import { logger } from "../../lib/logger.js";
import {
addToFpath,
addToGitHubPath,
addToPath,
detectShell,
getFpathCommand,
getPathCommand,
isBashAvailable,
isInPath,
Expand Down Expand Up @@ -180,6 +182,35 @@ async function tryBashCompletionFallback(
return await installCompletions("bash", homeDir, xdgDataHome);
}

/**
* Ensure the zsh completion directory is in fpath.
*
* Runs even on updates so existing installs get fpath configured (one-time migration).
* Returns status messages for the user.
*/
async function handleZshFpath(
shell: ShellInfo,
completionDir: string,
isNewInstall: boolean
): Promise<string[]> {
const lines: string[] = [];

if (shell.configFile) {
const result = await addToFpath(shell.configFile, completionDir);
if (result.modified) {
lines.push(`Completions: ${result.message}`);
lines.push(` Restart your shell or run: source ${shell.configFile}`);
} else if (result.manualCommand) {
lines.push(`Completions: ${result.message}`);
lines.push(` Add manually to .zshrc: ${result.manualCommand}`);
}
} else if (isNewInstall) {
lines.push(` Add to your .zshrc: ${getFpathCommand(completionDir)}`);
}

return lines;
}

/**
* Handle shell completion installation.
*
Expand All @@ -200,20 +231,19 @@ async function handleCompletions(
const location = await installCompletions(shell.type, homeDir, xdgDataHome);

if (location) {
// Silently updated — no need to tell the user on every upgrade
if (!location.created) {
return [];
}

const lines = [`Completions: Installed to ${location.path}`];
const lines: string[] = [];

// Zsh may need fpath hint
if (shell.type === "zsh") {
const completionDir = dirname(location.path);
lines.push(
` You may need to add to .zshrc: fpath=(${completionDir} $fpath)`
...(await handleZshFpath(shell, completionDir, location.created))
);
}

Comment on lines 242 to +244
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: On upgrade, handleZshFpath silently fails to provide setup instructions if a zsh config file is not found, because it incorrectly checks the isNewInstall flag.
Severity: MEDIUM

Suggested Fix

Modify handleZshFpath to provide manual setup instructions whenever shell.configFile is null, regardless of the isNewInstall flag's value. This will align its behavior with handlePathModification and ensure users always receive guidance.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/commands/cli/setup.ts#L242-L244

Potential issue: The `handleZshFpath` function fails to provide setup instructions
during an upgrade if the user's zsh configuration file was not found on the initial
install. The logic checks for a config file, and if not found, it only provides guidance
if `isNewInstall` is true. On an upgrade, `isNewInstall` is false, causing the function
to silently do nothing. This leaves the user with non-functional shell completions and
no information on how to fix it. This behavior is inconsistent with the
`handlePathModification` function, which always provides guidance when a configuration
file is missing, regardless of whether it's a new install or an upgrade.

if (location.created) {
lines.unshift(`Completions: Installed to ${location.path}`);
}

return lines;
}

Expand Down
78 changes: 61 additions & 17 deletions src/lib/shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,72 +178,116 @@ export function isInPath(
* @param shellType - The shell type (for correct syntax)
* @returns Result of the modification attempt
*/
export async function addToPath(
/**
* Append a shell config line to a config file with idempotency.
*
* Shared implementation for `addToPath` and `addToFpath`. Handles file
* creation, duplicate detection, newline-aware appending, and error fallback.
*
* @param configFile - Path to the shell config file
* @param directory - Directory being configured (used for duplicate check)
* @param command - The full shell command to append (e.g. `export PATH="..."`)
* @param label - Human-readable label for messages (e.g. "PATH", "fpath")
*/
async function addToShellConfig(
configFile: string,
directory: string,
shellType: ShellType
command: string,
label: string
): Promise<PathModificationResult> {
const pathCommand = getPathCommand(shellType, directory);

// Read current content
const file = Bun.file(configFile);
const exists = await file.exists();

if (!exists) {
// Create the file with the PATH command
try {
await Bun.write(configFile, `# sentry\n${pathCommand}\n`);
await Bun.write(configFile, `# sentry\n${command}\n`);
return {
modified: true,
configFile,
message: `Created ${configFile} with PATH configuration`,
message: `Created ${configFile} with ${label} configuration`,
manualCommand: null,
};
} catch {
return {
modified: false,
configFile: null,
message: `Could not create ${configFile}`,
manualCommand: pathCommand,
manualCommand: command,
};
}
}

const content = await file.text();

// Check if already configured
if (content.includes(pathCommand) || content.includes(`"${directory}"`)) {
if (content.includes(command) || content.includes(`"${directory}"`)) {
return {
modified: false,
configFile,
message: `PATH already configured in ${configFile}`,
message: `${label} already configured in ${configFile}`,
manualCommand: null,
};
}

// Append to file
try {
const newContent = content.endsWith("\n")
? `${content}\n# sentry\n${pathCommand}\n`
: `${content}\n\n# sentry\n${pathCommand}\n`;
? `${content}\n# sentry\n${command}\n`
: `${content}\n\n# sentry\n${command}\n`;

await Bun.write(configFile, newContent);
return {
modified: true,
configFile,
message: `Added sentry to PATH in ${configFile}`,
message: `Added sentry ${label} in ${configFile}`,
manualCommand: null,
};
} catch {
return {
modified: false,
configFile: null,
message: `Could not write to ${configFile}`,
manualCommand: pathCommand,
manualCommand: command,
};
}
}

export function addToPath(
configFile: string,
directory: string,
shellType: ShellType
): Promise<PathModificationResult> {
return addToShellConfig(
configFile,
directory,
getPathCommand(shellType, directory),
"PATH"
);
}

/**
* Generate the fpath command for zsh completion directory.
*/
export function getFpathCommand(directory: string): string {
return `fpath=("${directory}" $fpath)`;
}

/**
* Add a directory to zsh's fpath in a shell config file.
*
* @param configFile - Path to the zsh config file (e.g. ~/.zshrc)
* @param directory - Directory to add to fpath
*/
export function addToFpath(
configFile: string,
directory: string
): Promise<PathModificationResult> {
return addToShellConfig(
configFile,
directory,
getFpathCommand(directory),
"fpath"
);
}

/**
* Add to GitHub Actions PATH if running in CI.
*/
Expand Down
46 changes: 44 additions & 2 deletions test/commands/cli/setup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,46 @@ describe("sentry cli setup", () => {
expect(getOutput()).toContain("Completions:");
});

test("shows zsh fpath hint for zsh completions", async () => {
test("adds fpath to .zshrc for zsh completions", async () => {
const zshrc = join(testDir, ".zshrc");
writeFileSync(zshrc, "# existing zshrc\n");

const { context, getOutput, restore } = createMockContext({
homeDir: testDir,
execPath: join(testDir, "bin", "sentry"),
env: {
PATH: `/usr/bin:${join(testDir, "bin")}:/bin`,
SHELL: "/bin/zsh",
},
});
restoreStderr = restore;

await run(
app,
["cli", "setup", "--no-modify-path", "--no-agent-skills"],
context
);

expect(getOutput()).toContain("fpath");
expect(getOutput()).toContain("Completions:");

// Verify .zshrc was actually modified
const content = await Bun.file(zshrc).text();
expect(content).toContain("fpath=");
expect(content).toContain("site-functions");
});

test("skips fpath modification when already configured in .zshrc", async () => {
const zshrc = join(testDir, ".zshrc");
const completionDir = join(
testDir,
".local",
"share",
"zsh",
"site-functions"
);
writeFileSync(zshrc, `# existing\nfpath=("${completionDir}" $fpath)\n`);

const { context, getOutput, restore } = createMockContext({
homeDir: testDir,
execPath: join(testDir, "bin", "sentry"),
Expand All @@ -308,7 +347,10 @@ describe("sentry cli setup", () => {
context
);

expect(getOutput()).toContain("fpath=");
// Should still show "Installed to" but not "Added ... to fpath"
const output = getOutput();
expect(output).toContain("Completions: Installed to");
expect(output).not.toContain("Added sentry completions to fpath");
});

test("handles GitHub Actions PATH when GITHUB_ACTIONS is set", async () => {
Expand Down
98 changes: 98 additions & 0 deletions test/lib/shell.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test";
import { mkdirSync, rmSync, writeFileSync } from "node:fs";
import { dirname, join } from "node:path";
import {
addToFpath,
addToGitHubPath,
addToPath,
detectShell,
Expand Down Expand Up @@ -193,6 +194,103 @@ describe("shell utilities", () => {
});
});

describe("addToFpath", () => {
let testDir: string;

beforeEach(() => {
testDir = join(
"/tmp",
`shell-test-${Date.now()}-${Math.random().toString(36).slice(2)}`
);
mkdirSync(testDir, { recursive: true });
});

afterEach(() => {
rmSync(testDir, { recursive: true, force: true });
});

test("creates config file if it doesn't exist", async () => {
const configFile = join(testDir, ".zshrc");
const result = await addToFpath(
configFile,
"/home/user/.local/share/zsh/site-functions"
);

expect(result.modified).toBe(true);
expect(result.configFile).toBe(configFile);

const content = await Bun.file(configFile).text();
expect(content).toContain(
'fpath=("/home/user/.local/share/zsh/site-functions" $fpath)'
);
});

test("appends to existing config file", async () => {
const configFile = join(testDir, ".zshrc");
writeFileSync(configFile, "# existing content\n");

const result = await addToFpath(
configFile,
"/home/user/.local/share/zsh/site-functions"
);

expect(result.modified).toBe(true);

const content = await Bun.file(configFile).text();
expect(content).toContain("# existing content");
expect(content).toContain("# sentry");
expect(content).toContain(
'fpath=("/home/user/.local/share/zsh/site-functions" $fpath)'
);
});

test("skips if already configured", async () => {
const configFile = join(testDir, ".zshrc");
writeFileSync(
configFile,
'# sentry\nfpath=("/home/user/.local/share/zsh/site-functions" $fpath)\n'
);

const result = await addToFpath(
configFile,
"/home/user/.local/share/zsh/site-functions"
);

expect(result.modified).toBe(false);
expect(result.message).toContain("already configured");
});

test("appends newline separator when file doesn't end with newline", async () => {
const configFile = join(testDir, ".zshrc");
writeFileSync(configFile, "# existing content without newline");

const result = await addToFpath(
configFile,
"/home/user/.local/share/zsh/site-functions"
);

expect(result.modified).toBe(true);

const content = await Bun.file(configFile).text();
expect(content).toContain(
"# existing content without newline\n\n# sentry\n"
);
});

test("returns manualCommand when config file cannot be created", async () => {
const configFile = "/dev/null/impossible/path/.zshrc";
const result = await addToFpath(
configFile,
"/home/user/.local/share/zsh/site-functions"
);

expect(result.modified).toBe(false);
expect(result.manualCommand).toBe(
'fpath=("/home/user/.local/share/zsh/site-functions" $fpath)'
);
});
});

describe("addToGitHubPath", () => {
let testDir: string;

Expand Down
Loading