diff --git a/src/cli.test.ts b/src/cli.test.ts index 5444552f..bae79960 100644 --- a/src/cli.test.ts +++ b/src/cli.test.ts @@ -234,7 +234,7 @@ describe("root CLI preAction", () => { errorSpy.mockRestore(); }); - it("triggers auto-login for init before setup runs", async () => { + it("leaves init auth handling to the init command", async () => { const errorSpy = spyOn(console, "error").mockImplementation(() => {}); const container = createLoginDeps({ hasValidToken: false }); const createContainer = mock(() => Promise.resolve(container)); @@ -257,11 +257,9 @@ describe("root CLI preAction", () => { await program.parseAsync(["node", "githits", "init"]); expect(ran).toBe(true); - expect(createContainer).toHaveBeenCalledTimes(1); - expect(loginFlow).toHaveBeenCalledWith({}, container); - expect(errorSpy.mock.calls.map((call) => call[0])).toEqual([ - "Authentication complete. Continuing setup...", - ]); + expect(createContainer).not.toHaveBeenCalled(); + expect(loginFlow).not.toHaveBeenCalled(); + expect(errorSpy).not.toHaveBeenCalled(); errorSpy.mockRestore(); }); diff --git a/src/commands/auth-status.ts b/src/commands/auth-status.ts index 949b6593..df3d9425 100644 --- a/src/commands/auth-status.ts +++ b/src/commands/auth-status.ts @@ -53,9 +53,8 @@ export async function authStatusAction( if (!auth) { console.log("Not authenticated.\n"); console.log(` Environment: ${mcpUrl}\n`); - console.log(""); - console.log("To authenticate:"); - console.log(" githits login"); + console.log(` Storage: ${authStorage.getStorageLocation()}\n`); + printAuthTroubleshooting(); return; } @@ -81,8 +80,8 @@ export async function authStatusAction( console.log( ` Expired: ${new Date(auth.expiresAt).toLocaleDateString()}\n`, ); - console.log(""); - console.log("Run `githits login` to re-authenticate."); + console.log(` Storage: ${authStorage.getStorageLocation()}\n`); + printAuthTroubleshooting("expired"); return; } @@ -92,6 +91,23 @@ export async function authStatusAction( console.log(`\n Storage: ${authStorage.getStorageLocation()}`); } +function printAuthTroubleshooting(reason: "missing" | "expired" = "missing") { + const loginCommand = + reason === "expired" ? "githits login --force" : "githits login"; + console.log("Recovery steps:"); + console.log(` ${loginCommand}`); + if (reason === "missing") { + console.log(" githits login --force # if a previous login is stale"); + } + console.log("For CI/automation, set GITHITS_API_TOKEN."); + console.log( + "If your system keychain is locked or unavailable, unlock it and retry.", + ); + console.log( + "As a last resort, set GITHITS_AUTH_STORAGE=file to use plaintext file storage.", + ); +} + const STATUS_DESCRIPTION = `Show current authentication status. Displays details about the stored token including environment diff --git a/src/commands/example.test.ts b/src/commands/example.test.ts index 1b2c066c..9d699284 100644 --- a/src/commands/example.test.ts +++ b/src/commands/example.test.ts @@ -81,4 +81,31 @@ describe("exampleAction", () => { ), ).rejects.toThrow(AuthRequiredError); }); + + it("prints JSON auth error for --json when auth is missing", async () => { + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + const exitSpy = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + + await expect( + exampleAction( + "test", + { json: true }, + createDeps({ hasValidToken: false }), + ), + ).rejects.toThrow("process.exit"); + + const output = errorSpy.mock.calls[0]?.[0] as string; + expect(JSON.parse(output)).toEqual({ + error: + "Authentication required. Run `githits login`, then retry this command.", + code: "AUTH_REQUIRED", + retryable: false, + }); + expect(exitSpy).toHaveBeenCalledWith(1); + + errorSpy.mockRestore(); + exitSpy.mockRestore(); + }); }); diff --git a/src/commands/example.ts b/src/commands/example.ts index 45a701c9..b6f8ba6f 100644 --- a/src/commands/example.ts +++ b/src/commands/example.ts @@ -1,5 +1,6 @@ import { type Command, Option } from "commander"; import type { GitHitsService } from "../services/githits-service.js"; +import { AuthenticationError } from "../services/githits-service.js"; import { extractSolutionId } from "../shared/extract-solution-id.js"; import { AuthRequiredError, requireAuth } from "../shared/require-auth.js"; @@ -21,6 +22,15 @@ export async function exampleAction( options: ExampleOptions, deps: ExampleDependencies, ): Promise { + if (!deps.hasValidToken && options.json) { + printExampleError( + "Authentication required. Run `githits login`, then retry this command.", + "AUTH_REQUIRED", + true, + ); + process.exit(1); + } + requireAuth(deps); try { @@ -41,13 +51,31 @@ export async function exampleAction( console.log(result); } } catch (error) { - console.error( + if (error instanceof AuthenticationError) { + printExampleError( + "Authentication required. Run `githits login`, then retry this command.", + "AUTH_REQUIRED", + options.json ?? false, + ); + process.exit(1); + } + printExampleError( `Failed to get example: ${error instanceof Error ? error.message : error}`, + "UNKNOWN", + options.json ?? false, ); process.exit(1); } } +function printExampleError(error: string, code: string, json: boolean): void { + if (json) { + console.error(JSON.stringify({ error, code, retryable: false })); + return; + } + console.error(error); +} + const EXAMPLE_DESCRIPTION = `Get verified, canonical code examples from global open source. For dependency, package, or repository source search, use \`githits search\` instead. diff --git a/src/commands/init/init.test.ts b/src/commands/init/init.test.ts index 050f23a6..a5366ada 100644 --- a/src/commands/init/init.test.ts +++ b/src/commands/init/init.test.ts @@ -841,9 +841,60 @@ describe("initAction", () => { const logCalls = getLogOutput(); expect(logCalls.some((msg) => msg.includes("Login failed"))).toBe(true); + expect( + logCalls.some((msg) => msg.includes("GitHits tools will require auth")), + ).toBe(true); expect(fs.atomicWriteFile).toHaveBeenCalled(); }); + it("does not claim GitHits is ready after continuing without auth", async () => { + const fs = createFsWithDetection(["/home/test/.cursor"], { + "/home/test/.cursor/mcp.json": JSON.stringify({ + mcpServers: { + GitHits: { + command: "npx", + args: ["-y", "githits@latest", "mcp", "start"], + }, + }, + }), + }); + const promptService = createMockPromptService({ + confirm3: mock(() => Promise.resolve("yes" as ConfirmChoice)), + }); + const createLoginDeps = mock(() => + Promise.resolve({ + authService: createMockAuthService({ + discoverEndpoints: mock(() => + Promise.reject(new Error("Network error")), + ), + }), + authStorage: createMockAuthStorage(), + browserService: createMockBrowserService(), + mcpUrl: "https://mcp.githits.com", + }), + ); + + await initAction( + {}, + { + fileSystemService: fs, + promptService, + execService: createMockExecService(), + createLoginDeps, + }, + ); + + const logCalls = getLogOutput(); + expect( + logCalls.some((msg) => + msg.includes("authentication is still required"), + ), + ).toBe(true); + expect( + logCalls.some((msg) => msg.includes("Done! GitHits is ready")), + ).toBe(false); + }); + it("cancels setup when login fails and user declines to continue", async () => { const fs = createFsWithDetection(["/home/test/.cursor"]); const promptService = createMockPromptService({ diff --git a/src/commands/init/init.ts b/src/commands/init/init.ts index 912540cc..01c38a14 100644 --- a/src/commands/init/init.ts +++ b/src/commands/init/init.ts @@ -83,6 +83,7 @@ export async function initAction( const useColors = shouldUseColors(); const { fileSystemService, promptService, execService, createLoginDeps } = deps; + let continuedWithoutAuth = false; // Header console.log( @@ -109,6 +110,7 @@ export async function initAction( console.log( ` ${warning(`Login failed: ${loginResult.message}`, useColors)}\n`, ); + printAuthRecoveryHint(); if (!options.yes) { try { const choice = await promptService.confirm3( @@ -128,6 +130,7 @@ export async function initAction( throw err; } } + continuedWithoutAuth = true; console.log(" Continuing without authentication...\n"); } } @@ -168,6 +171,13 @@ export async function initAction( // All detected agents already configured if (scan.needsSetup.length === 0) { + if (continuedWithoutAuth) { + console.log( + " MCP is already configured, but authentication is still required.", + ); + console.log(" Run `githits login` before using GitHits tools.\n"); + return; + } console.log( " All detected agents are already configured. Nothing to do.\n", ); @@ -267,6 +277,9 @@ export async function initAction( if (failed > 0) { console.log(" Setup completed with errors."); + } else if (continuedWithoutAuth && (configured > 0 || alreadyDone > 0)) { + console.log(" MCP is configured, but authentication is still required."); + console.log(" Run `githits login` before using GitHits tools."); } else if (configured > 0 || alreadyDone > 0) { console.log(" Done! GitHits is ready."); } else if (skipped > 0) { @@ -290,6 +303,19 @@ export async function initAction( console.log(); } +function printAuthRecoveryHint(): void { + console.log( + " You can still configure MCP, but GitHits tools will require auth.", + ); + console.log(" Recovery steps:"); + console.log(" githits auth status"); + console.log(" githits login --force"); + console.log(" For CI or locked-down machines, set GITHITS_API_TOKEN."); + console.log( + " If your system keychain is unavailable, set GITHITS_AUTH_STORAGE=file after accepting plaintext storage.\n", + ); +} + const INIT_DESCRIPTION = `Set up GitHits MCP server for your coding agents. Authenticates with your GitHits account, then scans for available agents diff --git a/src/commands/login.test.ts b/src/commands/login.test.ts index 11f24236..4b8aaca1 100644 --- a/src/commands/login.test.ts +++ b/src/commands/login.test.ts @@ -437,6 +437,86 @@ describe("loginFlow", () => { consoleSpy.mockRestore(); }); + it("prints manual URL when browser opening fails", async () => { + const writes: string[] = []; + const browserService = createMockBrowserService({ + open: mock(() => Promise.reject(new Error("no display"))), + }); + + const result = await loginFlow( + { port: 8080 }, + { + authService: createMockAuthService(), + authStorage: createMockAuthStorage(), + browserService, + mcpUrl, + }, + { write: (message: string) => writes.push(message) }, + ); + + expect(result.status).toBe("success"); + expect(writes).toContain( + "Could not open browser automatically: no display\n", + ); + expect(writes).toContain("Open this URL in your browser:\n"); + expect(writes).toContain(" http://example.com/auth\n"); + }); + + it("closes callback server when authentication wait fails", async () => { + const close = mock(() => Promise.resolve()); + const authService = createMockAuthService({ + startCallbackServer: mock(() => + Promise.resolve({ + result: Promise.reject(new Error("callback failed")), + close, + }), + ), + }); + + const result = await loginFlow( + { port: 8080 }, + { + authService, + authStorage: createMockAuthStorage(), + browserService: createMockBrowserService(), + mcpUrl, + }, + silentLoginOutput, + ); + + expect(result.status).toBe("failed"); + expect(result.message).toBe("callback failed."); + expect(close).toHaveBeenCalledTimes(1); + }); + + it("does not open browser when callback server cannot start", async () => { + const browserService = createMockBrowserService(); + const authService = createMockAuthService({ + startCallbackServer: mock(() => + Promise.reject( + new Error("Failed to start callback server: EADDRINUSE"), + ), + ), + }); + + const result = await loginFlow( + { port: 8080 }, + { + authService, + authStorage: createMockAuthStorage(), + browserService, + mcpUrl, + }, + silentLoginOutput, + ); + + expect(result).toEqual({ + status: "failed", + message: "Failed to start callback server: EADDRINUSE", + }); + expect(browserService.open).not.toHaveBeenCalled(); + }); + it("returns already_authenticated when valid tokens exist", async () => { const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); diff --git a/src/commands/login.ts b/src/commands/login.ts index 41d9a8b7..bac2c986 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -190,7 +190,15 @@ export async function loginFlow( }); // Step 5: Start callback server - const serverPromise = authService.startCallbackServer(port, state); + let callbackServer: Awaited< + ReturnType + >; + try { + callbackServer = await authService.startCallbackServer(port, state); + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + return { status: "failed", message: msg }; + } // Step 6: Open browser or show URL if (options.browser === false) { @@ -198,7 +206,14 @@ export async function loginFlow( output.write(` ${authUrl}\n`); } else { output.write("Opening browser..."); - await browserService.open(authUrl); + try { + await browserService.open(authUrl); + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + output.write(`Could not open browser automatically: ${msg}\n`); + output.write("Open this URL in your browser:\n"); + output.write(` ${authUrl}\n`); + } } output.write("Waiting for authentication...\n"); @@ -212,12 +227,13 @@ export async function loginFlow( ); }); - let callback: Awaited; + let callback: Awaited; try { - callback = await Promise.race([serverPromise, timeoutPromise]); + callback = await Promise.race([callbackServer.result, timeoutPromise]); if (timeoutId) clearTimeout(timeoutId); } catch (error) { if (timeoutId) clearTimeout(timeoutId); + await callbackServer.close().catch(() => {}); const msg = error instanceof Error ? error.message : "Authentication failed"; return { status: "failed", message: `${msg}.` }; @@ -298,7 +314,7 @@ export async function loginAction( if (result.status === "failed") { console.error(`${result.message}\n`); - console.log("Run `githits login` to try again."); + printLoginRecoveryHint(result.message); process.exit(1); } @@ -309,6 +325,21 @@ export async function loginAction( console.log("\nYou're ready to use githits with your AI assistant."); } +function printLoginRecoveryHint(message: string): void { + console.log("Recovery steps:"); + console.log(" githits auth status"); + console.log(" githits login --force"); + if (message.includes("Cannot persist OAuth credentials")) { + console.log( + "If your system keychain is locked or unavailable, unlock it and retry.", + ); + console.log("For CI/automation, set GITHITS_API_TOKEN."); + console.log( + "As a last resort, set GITHITS_AUTH_STORAGE=file to use plaintext file storage.", + ); + } +} + const LOGIN_DESCRIPTION = `Authenticate with your GitHits account via browser. Opens your browser to complete authentication securely using OAuth. diff --git a/src/services/auth-service.test.ts b/src/services/auth-service.test.ts index 8dd6d341..28bde96a 100644 --- a/src/services/auth-service.test.ts +++ b/src/services/auth-service.test.ts @@ -1,4 +1,5 @@ import { afterEach, describe, expect, it, mock } from "bun:test"; +import { createServer } from "node:http"; import { AuthServiceImpl, evaluateCallback } from "./auth-service.js"; describe("AuthServiceImpl", () => { @@ -58,6 +59,31 @@ describe("AuthServiceImpl", () => { }); }); + describe("startCallbackServer", () => { + it("rejects before returning a handle when the callback port is unavailable", async () => { + const occupiedServer = createServer(); + await new Promise((resolve) => { + occupiedServer.listen(0, "127.0.0.1", resolve); + }); + const address = occupiedServer.address(); + const port = + typeof address === "object" && address !== null ? address.port : 0; + + try { + await expect( + service.startCallbackServer(port, "state"), + ).rejects.toThrow("Failed to start callback server"); + } finally { + await new Promise((resolve, reject) => { + occupiedServer.close((error) => { + if (error) reject(error); + else resolve(); + }); + }); + } + }); + }); + describe("refreshAccessToken", () => { it("accepts refresh responses that omit refresh_token", async () => { const fetchMock = mock(() => diff --git a/src/services/auth-service.ts b/src/services/auth-service.ts index 6a7eb57a..e12f34b2 100644 --- a/src/services/auth-service.ts +++ b/src/services/auth-service.ts @@ -134,7 +134,7 @@ export interface AuthService { startCallbackServer( port: number, expectedState: string, - ): Promise; + ): Promise; /** Exchange authorization code for tokens */ exchangeCodeForTokens(params: ExchangeParams): Promise; @@ -143,6 +143,11 @@ export interface AuthService { refreshAccessToken(params: RefreshParams): Promise; } +export interface CallbackServerHandle { + result: Promise; + close(): Promise; +} + /** * Production implementation of AuthService using OAuth PKCE with DCR. */ @@ -227,14 +232,18 @@ export class AuthServiceImpl implements AuthService { startCallbackServer( port: number, expectedState: string, - ): Promise { - return new Promise((resolve, reject) => { - let callbackHandled = false; - let resolved = false; - let closeTimer: ReturnType | undefined; - - const server = createServer((req, res) => { - const url = new URL(req.url ?? "", `http://127.0.0.1:${port}`); + ): Promise { + let closeTimer: ReturnType | undefined; + const server = createServer(); + let callbackHandled = false; + let resolved = false; + + const result = new Promise((resolve) => { + server.on("request", (req, res) => { + const address = server.address(); + const actualPort = + typeof address === "object" && address !== null ? address.port : port; + const url = new URL(req.url ?? "", `http://127.0.0.1:${actualPort}`); // Browsers frequently request favicon right after loading callback page. // Keep this endpoint quiet to avoid noisy follow-up errors. @@ -285,11 +294,23 @@ export class AuthServiceImpl implements AuthService { if (closeTimer) clearTimeout(closeTimer); closeTimer = setTimeout(() => closeServer(server), 1500); }); + }); - // Bind to 127.0.0.1 only for security - server.listen(port, "127.0.0.1"); - server.on("error", (err) => { + return new Promise((resolve, reject) => { + const onError = (err: Error) => { reject(new Error(`Failed to start callback server: ${err.message}`)); + }; + server.once("error", onError); + server.listen(port, "127.0.0.1", () => { + server.off("error", onError); + server.on("error", () => {}); + resolve({ + result, + close: async () => { + if (closeTimer) clearTimeout(closeTimer); + await closeServer(server); + }, + }); }); }); } @@ -670,8 +691,17 @@ function sendHtmlResponse( } /** Close server gracefully */ -function closeServer(server: Server): void { - server.close(); +function closeServer(server: Server): Promise { + return new Promise((resolve, reject) => { + if (!server.listening) { + resolve(); + return; + } + server.close((error) => { + if (error) reject(error); + else resolve(); + }); + }); } /** Escape HTML to prevent XSS */ diff --git a/src/services/test-helpers.ts b/src/services/test-helpers.ts index 557d31af..4bedadb2 100644 --- a/src/services/test-helpers.ts +++ b/src/services/test-helpers.ts @@ -96,7 +96,12 @@ export function createMockAuthService( ), generatePkceParams: mock(() => defaultPkceParams), buildAuthUrl: mock(() => "http://example.com/auth"), - startCallbackServer: mock(() => Promise.resolve(defaultCallbackResult)), + startCallbackServer: mock(() => + Promise.resolve({ + result: Promise.resolve(defaultCallbackResult), + close: mock(() => Promise.resolve()), + }), + ), exchangeCodeForTokens: mock(() => Promise.resolve(defaultTokenResponse)), refreshAccessToken: mock(() => Promise.resolve(defaultTokenResponse)), ...impl, diff --git a/src/shared/auto-login.test.ts b/src/shared/auto-login.test.ts index 8b1e87d9..4093cfcc 100644 --- a/src/shared/auto-login.test.ts +++ b/src/shared/auto-login.test.ts @@ -68,16 +68,16 @@ describe("isAutoLoginEligibleCommand", () => { ).toBe(true); }); - it("allows interactive init invocations for first-run onboarding", () => { + it("leaves init auth handling to the init command", () => { expect( isAutoLoginEligibleCommand(createCommand(["init"]), { stdinIsTTY: true, stdoutIsTTY: true, }), - ).toBe(true); + ).toBe(false); }); - it("honors init --skip-login", () => { + it("does not special-case init --skip-login in root auto-login", () => { expect( isAutoLoginEligibleCommand(createCommand(["init"], { skipLogin: true }), { stdinIsTTY: true, diff --git a/src/shared/auto-login.ts b/src/shared/auto-login.ts index 5ee7119b..5c77aea9 100644 --- a/src/shared/auto-login.ts +++ b/src/shared/auto-login.ts @@ -5,7 +5,6 @@ import type { } from "../commands/login.js"; const AUTO_LOGIN_ELIGIBLE_COMMANDS = new Set([ - "init", "example", "languages", "feedback", @@ -73,10 +72,6 @@ export function isAutoLoginEligibleCommand( }, ): boolean { const commandPath = getCommandPath(command).join(" "); - if (commandPath === "init" && command.opts().skipLogin === true) { - return false; - } - if (!AUTO_LOGIN_ELIGIBLE_COMMANDS.has(commandPath)) { return false; } diff --git a/src/shared/root-cli-pre-action.ts b/src/shared/root-cli-pre-action.ts index c9a90983..07ab2a4b 100644 --- a/src/shared/root-cli-pre-action.ts +++ b/src/shared/root-cli-pre-action.ts @@ -52,8 +52,6 @@ export function createRootCliPreAction( function getPostLoginContinuationMessage(command: Command): string | undefined { switch (getCommandPath(command).join(" ")) { - case "init": - return "Authentication complete. Continuing setup..."; case "example": return "Authentication complete. Running example search..."; case "languages":