diff --git a/.archgate/adrs/ARCH-005-testing-standards.md b/.archgate/adrs/ARCH-005-testing-standards.md index 053deb50..f9566e23 100644 --- a/.archgate/adrs/ARCH-005-testing-standards.md +++ b/.archgate/adrs/ARCH-005-testing-standards.md @@ -52,6 +52,7 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes - **Wrap `spyOn` / `mockImplementation` calls in `try/finally` to guarantee `mockRestore()` runs** — when `expect()` assertions fail, they throw immediately, skipping any `mockRestore()` that follows. The un-restored spy leaks into subsequent tests, causing false positives or false negatives. Pattern: `const spy = spyOn(...).mockImplementation(() => {}); try { /* assertions */ } finally { spy.mockRestore(); }`. Alternatively, create and restore spies in `beforeEach`/`afterEach` hooks instead of inline. - **Make large production thresholds injectable so tests can use a small value** — When production code only acts past a large numeric threshold (e.g., `SCOPE_FILE_WARN_THRESHOLD = 1000` in `src/engine/git-files.ts`), add an optional parameter that defaults to the module constant (e.g., `resolveScopedFiles(root, globs, { fileWarnThreshold })`). Tests inject a tiny value such as `5` and create only a handful of files to exercise the same code path. This keeps the test fast and deterministic on every platform instead of materializing thousands of fixture files. - **Only ever raise a per-test timeout override above the global, never below it** — the project global is `bun test --timeout 60000` (60s). A per-test `}, 30_000` override makes that test _more_ likely to time out than the default. Use a per-test override solely to grant a genuinely slow test more time than 60s; never set one shorter than the global. +- **Mock first-party modules with `import * as mod` + `spyOn(mod, "fn")`, not `mock.module()`** — declare `import * as authMod from "../../src/helpers/auth"`, then in `beforeEach` install `spyOn(authMod, "requestDeviceCode").mockResolvedValue(...)` and restore with `mock.restore()` in `afterEach`. `spyOn` is scoped per-test and auto-restored, and it correctly reaches BOTH static named-import consumers (Bun backs named imports with live bindings) AND dynamic `await import()` consumers, because it mutates the single shared module instance. This is the same pattern used for `spyOn(pathsMod, "findProjectRoot")`. ### Don't @@ -66,6 +67,7 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes - **Don't use a bare early `return` or an empty callback body to conditionally skip a test** — use `test.skipIf(condition)`, `test.skip`, or `test.todo` so the skip is explicit and the assertion rule can recognize it. Bare returns and empty bodies are exactly how assertion-less tests accumulated silently before the rule existed. - Don't import test utilities from `node:test` — use Bun's built-in `bun:test` module - **Don't use `mock.module("node:fetch", ...)` to intercept HTTP fetch calls** — in Bun, the runtime fetch is `globalThis.fetch` and `mock.module` targeting `node:fetch` does not intercept it. The mock silently has no effect: the real network is hit, making tests non-deterministic and dependent on external services. Assign `globalThis.fetch` directly instead (see Do's above). +- **Don't use `mock.module()` on a first-party module that any other test file imports** — Bun's `mock.module()` is process-global and retroactive: it replaces the module for ALL test files in the same Bun process and is NOT undone by `mock.restore()`. Symptom: a test that imports the real implementation intermittently receives a mocked one depending on file execution order — a flaky failure that passes in isolation. Do NOT work around this by splitting production code into a separate `-impl` file (or thin delegating wrapper) just so the "real" tests can import an un-mocked path — that contorts the production module layout to serve a test-tool quirk. Fix the test by switching to `spyOn` (see Do's above). `mock.module()` remains acceptable for third-party modules (e.g. `inquirer`, `node:readline`) that no first-party test needs the real implementation of. - **Don't place `mockRestore()` after assertions without `try/finally` protection** — if the assertion throws, the spy is never restored and contaminates subsequent tests. This is especially dangerous when spying on globals like `console.warn` or `globalThis.fetch`, where a leaked spy silently suppresses or redirects output for every test that follows. - **Don't materialize large filesystem fixtures (1000+ files, plus `git add .`) just to cross a production threshold** — on Windows CI runners that filesystem work is slow enough to exceed the per-test timeout, killing the staging subprocess mid-run (`git add . failed (exit 143)`, where 143 = 128 + SIGTERM) and producing a flaky failure that does not reproduce locally. Inject a small threshold instead (see Do's above). This was the root cause of a recurring Windows smoke-test flake on the `resolveScopedFiles` warning test. @@ -178,6 +180,22 @@ mock.module("node:fetch", () => ({ // GOOD: assign globalThis.fetch directly globalThis.fetch = (() => Promise.reject(new Error("network error"))) as unknown as typeof fetch; + +// BAD: mock.module on a first-party module is process-global and leaks into +// EVERY other test file in the process — auth.test.ts then receives this mock +// instead of the real implementation (flaky, order-dependent). +mock.module("../../src/helpers/auth", () => ({ + requestDeviceCode: mock(() => Promise.resolve({ device_code: "x" })), +})); + +// GOOD: spyOn the imported module namespace — per-test and auto-restored. +import * as authMod from "../../src/helpers/auth"; +beforeEach(() => { + spyOn(authMod, "requestDeviceCode").mockResolvedValue({ device_code: "x" }); +}); +afterEach(() => { + mock.restore(); +}); ``` ## Consequences @@ -225,6 +243,7 @@ Code reviewers MUST verify: 7. Tests that use `spyOn` or `mockImplementation` inline (not in `beforeEach`/`afterEach`) wrap the spy lifecycle in `try/finally` to guarantee `mockRestore()` runs even when assertions fail 8. Every test asserts something with `expect()` — no smoke tests that merely call a function; "does not throw" contracts are made explicit via `expect(() => fn()).not.toThrow()` or `await expect(promise).resolves.toBeUndefined()` 9. Tests that exercise a numeric production threshold inject a small threshold value rather than generating fixtures large enough to trip the production default, and no per-test timeout override is shorter than the global `--timeout 60000` +10. Tests mock first-party modules via `import * as mod` + `spyOn`, not `mock.module()` — and no production module has been split into a separate `-impl` file solely to dodge `mock.module` leakage ## References diff --git a/src/helpers/auth-poll.ts b/src/helpers/auth-poll.ts deleted file mode 100644 index 776cc894..00000000 --- a/src/helpers/auth-poll.ts +++ /dev/null @@ -1,120 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// Copyright 2026 Archgate -/** - * auth-poll.ts — RFC 8628 device code polling logic. - * - * Extracted from auth.ts so that test files can import `pollForAccessToken` - * from a module path that is NOT targeted by `mock.module()` in - * login-flow.test.ts. Bun's `mock.module` is process-global and retroactive - * — it replaces live ESM bindings even for static imports — so the only - * reliable isolation is a separate physical file. - */ - -import { logDebug } from "./log"; - -// --------------------------------------------------------------------------- -// Constants -// --------------------------------------------------------------------------- - -const GITHUB_DEVICE_TOKEN_URL = "https://github.com/login/oauth/access_token"; - -/** - * GitHub OAuth App client ID for the archgate CLI (public client — no secret). - * Device flow apps are public clients; the client_id is not confidential. - */ -const GITHUB_CLIENT_ID = "Ov23liZUI9Aiv2ZrSAgn"; - -// --------------------------------------------------------------------------- -// Types -// --------------------------------------------------------------------------- - -interface DeviceTokenSuccessResponse { - access_token: string; - token_type: string; - scope: string; -} - -interface DeviceTokenPendingResponse { - error: "authorization_pending" | "slow_down"; - error_description?: string; -} - -interface DeviceTokenErrorResponse { - error: "expired_token" | "access_denied" | string; - error_description?: string; -} - -type DeviceTokenResponse = - | DeviceTokenSuccessResponse - | DeviceTokenPendingResponse - | DeviceTokenErrorResponse; - -// --------------------------------------------------------------------------- -// Polling -// --------------------------------------------------------------------------- - -/** - * Poll GitHub until the user authorizes (or the code expires). - * Returns the GitHub access token on success. - */ -export async function pollForAccessToken( - deviceCode: string, - interval: number, - expiresIn: number -): Promise { - const deadline = Date.now() + expiresIn * 1000; - let pollInterval = interval; - logDebug( - "Starting device code polling, deadline:", - new Date(deadline).toISOString() - ); - - /* oxlint-disable no-await-in-loop -- sequential polling is required by RFC 8628 */ - while (Date.now() < deadline) { - await Bun.sleep(pollInterval * 1000); - - const response = await fetch(GITHUB_DEVICE_TOKEN_URL, { - method: "POST", - headers: { - Accept: "application/json", - "Content-Type": "application/json", - }, - body: JSON.stringify({ - client_id: GITHUB_CLIENT_ID, - device_code: deviceCode, - grant_type: "urn:ietf:params:oauth:grant-type:device_code", - }), - signal: AbortSignal.timeout(15_000), - }); - - if (!response.ok) { - throw new Error(`GitHub token poll failed (HTTP ${response.status})`); - } - - const data = (await response.json()) as DeviceTokenResponse; - - if ("access_token" in data) { - logDebug("Access token received"); - return data.access_token; - } - - if ("error" in data) { - if (data.error === "authorization_pending") { - logDebug("Authorization pending, retrying in", pollInterval, "seconds"); - continue; - } - if (data.error === "slow_down") { - pollInterval += 5; - logDebug("Slow down requested, new interval:", pollInterval, "seconds"); - continue; - } - // expired_token, access_denied, or other terminal error - throw new Error( - data.error_description ?? `GitHub authorization failed: ${data.error}` - ); - } - } - /* oxlint-enable no-await-in-loop */ - - throw new Error("Device code expired. Please try again."); -} diff --git a/src/helpers/auth.ts b/src/helpers/auth.ts index dcf88f73..7b1766f7 100644 --- a/src/helpers/auth.ts +++ b/src/helpers/auth.ts @@ -8,30 +8,16 @@ * credential helpers (macOS Keychain, Windows Credential Manager, libsecret). */ -// Re-export pollForAccessToken as a wrapper (NOT a live re-export) so that -// mock.module("auth") in login-flow.test.ts does NOT follow the binding chain -// into auth-poll.ts. A live `export { X } from "./Y"` creates a binding that -// Bun's mock.module replaces at the source, poisoning auth-poll.ts for other -// test files. A wrapper function is its own binding — mocking auth.ts replaces -// the wrapper, leaving auth-poll.ts's binding untouched. -import { pollForAccessToken as pollForAccessTokenImpl } from "./auth-poll"; import { logDebug } from "./log"; import { SignupRequiredError, isSignupRequiredError } from "./signup"; -export async function pollForAccessToken( - deviceCode: string, - interval: number, - expiresIn: number -): Promise { - return await pollForAccessTokenImpl(deviceCode, interval, expiresIn); -} - // --------------------------------------------------------------------------- // Constants // --------------------------------------------------------------------------- const PLUGINS_API = "https://plugins.archgate.dev"; const GITHUB_DEVICE_CODE_URL = "https://github.com/login/device/code"; +const GITHUB_DEVICE_TOKEN_URL = "https://github.com/login/oauth/access_token"; /** * GitHub OAuth App client ID for the archgate CLI (public client — no secret). @@ -51,6 +37,27 @@ interface DeviceCodeResponse { interval: number; } +interface DeviceTokenSuccessResponse { + access_token: string; + token_type: string; + scope: string; +} + +interface DeviceTokenPendingResponse { + error: "authorization_pending" | "slow_down"; + error_description?: string; +} + +interface DeviceTokenErrorResponse { + error: "expired_token" | "access_denied" | string; + error_description?: string; +} + +type DeviceTokenResponse = + | DeviceTokenSuccessResponse + | DeviceTokenPendingResponse + | DeviceTokenErrorResponse; + // --------------------------------------------------------------------------- // GitHub Device Flow // --------------------------------------------------------------------------- @@ -80,6 +87,72 @@ export async function requestDeviceCode(): Promise { return data; } +/** + * Step 2: Poll GitHub until the user authorizes (or the code expires). + * Returns the GitHub access token on success. + */ +export async function pollForAccessToken( + deviceCode: string, + interval: number, + expiresIn: number +): Promise { + const deadline = Date.now() + expiresIn * 1000; + let pollInterval = interval; + logDebug( + "Starting device code polling, deadline:", + new Date(deadline).toISOString() + ); + + /* oxlint-disable no-await-in-loop -- sequential polling is required by RFC 8628 */ + while (Date.now() < deadline) { + await Bun.sleep(pollInterval * 1000); + + const response = await fetch(GITHUB_DEVICE_TOKEN_URL, { + method: "POST", + headers: { + Accept: "application/json", + "Content-Type": "application/json", + }, + body: JSON.stringify({ + client_id: GITHUB_CLIENT_ID, + device_code: deviceCode, + grant_type: "urn:ietf:params:oauth:grant-type:device_code", + }), + signal: AbortSignal.timeout(15_000), + }); + + if (!response.ok) { + throw new Error(`GitHub token poll failed (HTTP ${response.status})`); + } + + const data = (await response.json()) as DeviceTokenResponse; + + if ("access_token" in data) { + logDebug("Access token received"); + return data.access_token; + } + + if ("error" in data) { + if (data.error === "authorization_pending") { + logDebug("Authorization pending, retrying in", pollInterval, "seconds"); + continue; + } + if (data.error === "slow_down") { + pollInterval += 5; + logDebug("Slow down requested, new interval:", pollInterval, "seconds"); + continue; + } + // expired_token, access_denied, or other terminal error + throw new Error( + data.error_description ?? `GitHub authorization failed: ${data.error}` + ); + } + } + /* oxlint-enable no-await-in-loop */ + + throw new Error("Device code expired. Please try again."); +} + interface GitHubUserInfo { login: string; email: string | null; diff --git a/src/helpers/credential-store-impl.ts b/src/helpers/credential-store-impl.ts deleted file mode 100644 index ea207434..00000000 --- a/src/helpers/credential-store-impl.ts +++ /dev/null @@ -1,248 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// Copyright 2026 Archgate -/** - * credential-store-impl.ts — Secure credential storage using git's native credential helpers. - * - * Tokens are stored exclusively in the OS credential manager (macOS Keychain, - * Windows Credential Manager, libsecret) via `git credential approve/fill/reject`. - * Nothing is written to disk — no metadata files, no plaintext tokens. - * - * The `username` field in the git credential protocol carries the GitHub username, - * and the `password` field carries the archgate plugin token. - * - * This implementation lives in a separate file from credential-store.ts so that - * test files can import from a module path that is NOT targeted by `mock.module()` - * in login-flow.test.ts. Bun's `mock.module` is process-global and retroactive - * — it replaces live ESM bindings even for static imports — so the only - * reliable isolation is a separate physical file. - * - * @see https://git-scm.com/docs/git-credential - */ - -import { unlinkSync } from "node:fs"; - -import { logDebug, logWarn } from "./log"; -import { internalPath } from "./paths"; - -const CREDENTIAL_HOST = "plugins.archgate.dev"; -const CREDENTIAL_TIMEOUT_MS = 3_000; - -/** - * Build env for git credential commands at call time (not import time). - * - * Suppresses ALL interactive prompts — terminal, GUI, and askpass — across - * platforms and Git Credential Manager (GCM) versions: - * - * - GIT_TERMINAL_PROMPT=0 — git's own terminal prompt - * - GCM_INTERACTIVE=never — GCM interactive mode (terminal + GUI) - * - GCM_GUI_PROMPT=false — GCM GUI-only prompt (Windows toast/dialog) - * - GIT_ASKPASS="" — external askpass program - * - SSH_ASKPASS="" — SSH askpass fallback (some helpers reuse it) - */ -function gitCredentialEnv(): Record { - return { - ...Bun.env, - GIT_TERMINAL_PROMPT: "0", - GCM_INTERACTIVE: "never", - GCM_GUI_PROMPT: "false", - GIT_ASKPASS: "", - SSH_ASKPASS: "", - }; -} - -export interface StoredCredentials { - token: string; - github_user: string; -} - -// --------------------------------------------------------------------------- -// Git credential protocol helpers -// --------------------------------------------------------------------------- - -function credentialInput(username?: string, password?: string): string { - const lines = ["protocol=https", `host=${CREDENTIAL_HOST}`]; - if (username) lines.push(`username=${username}`); - if (password) lines.push(`password=${password}`); - lines.push("", ""); - return lines.join("\n"); -} - -async function gitCredentialApprove( - username: string, - password: string -): Promise { - const proc = Bun.spawn(["git", "credential", "approve"], { - stdin: new Blob([credentialInput(username, password)]), - stdout: "pipe", - stderr: "pipe", - env: gitCredentialEnv(), - }); - return (await proc.exited) === 0; -} - -async function gitCredentialFill(): Promise<{ - username: string; - password: string; -} | null> { - try { - const proc = Bun.spawn(["git", "credential", "fill"], { - stdin: new Blob([credentialInput()]), - stdout: "pipe", - stderr: "pipe", - env: gitCredentialEnv(), - }); - - // The timeout MUST be cancelled when the spawn wins the race — - // `Bun.sleep` / `setTimeout` both keep the event loop alive for - // their full duration, which used to add 3s of latency to - // commands that call `loadCredentials()` (e.g. `archgate doctor`). - let timer: ReturnType | undefined; - const result = await Promise.race([ - (async () => { - const stdout = await new Response(proc.stdout).text(); - const exitCode = await proc.exited; - return { stdout, exitCode }; - })(), - new Promise((resolve) => { - timer = setTimeout(() => { - proc.kill(); - resolve(null); - }, CREDENTIAL_TIMEOUT_MS); - }), - ]).finally(() => { - if (timer) clearTimeout(timer); - }); - - if (!result || result.exitCode !== 0) return null; - - let username = ""; - let password = ""; - for (const line of result.stdout.split("\n")) { - if (line.startsWith("username=")) username = line.slice(9); - if (line.startsWith("password=")) password = line.slice(9); - } - return username && password ? { username, password } : null; - } catch { - return null; - } -} - -async function gitCredentialReject( - username: string, - password: string -): Promise { - const proc = Bun.spawn(["git", "credential", "reject"], { - stdin: new Blob([credentialInput(username, password)]), - stdout: "pipe", - stderr: "pipe", - env: gitCredentialEnv(), - }); - await proc.exited; -} - -// --------------------------------------------------------------------------- -// Legacy metadata file cleanup -// --------------------------------------------------------------------------- - -/** Path to the legacy metadata file (~/.archgate/credentials). */ -function legacyMetadataPath(): string { - return internalPath("credentials"); -} - -/** - * Delete the legacy ~/.archgate/credentials file if it exists. - * Returns true if a file was found and deleted. - */ -async function cleanupLegacyMetadata(): Promise { - const file = Bun.file(legacyMetadataPath()); - if (await file.exists()) { - unlinkSync(legacyMetadataPath()); - logDebug("Legacy credentials metadata file removed"); - return true; - } - return false; -} - -// --------------------------------------------------------------------------- -// Public API -// --------------------------------------------------------------------------- - -const CREDENTIAL_HELPER_HINT = - "Run `git config --global credential.helper` to check your configuration."; - -/** - * Persist archgate credentials in the OS credential manager. - * - * A verification round-trip (`git credential fill`) confirms the token was - * actually persisted — `git credential approve` exits 0 even without a - * configured helper, silently storing nothing. - */ -export async function saveCredentials( - credentials: StoredCredentials -): Promise { - // Clean up any legacy metadata file from previous versions. - await cleanupLegacyMetadata(); - - const stored = await gitCredentialApprove( - credentials.github_user, - credentials.token - ); - - if (stored) { - const verified = await gitCredentialFill(); - if (verified) { - logDebug("Token verified in git credential manager"); - } else { - logWarn( - "Token could not be verified in git credential manager.", - "Your credential helper may not persist credentials.", - CREDENTIAL_HELPER_HINT, - "Without a working credential helper, you will need to re-login after each session." - ); - } - } else { - logWarn( - "git credential approve failed.", - "Your git credential helper may not be configured.", - CREDENTIAL_HELPER_HINT - ); - } -} - -/** - * Load stored archgate credentials from the OS credential manager. - * Returns null if no credentials are stored. - * - * If a legacy ~/.archgate/credentials file exists, it is deleted and - * the user is asked to re-login. - */ -export async function loadCredentials(): Promise { - // Delete legacy metadata file — force re-login for a clean slate. - const hadLegacy = await cleanupLegacyMetadata(); - if (hadLegacy) { - logWarn( - "Legacy credentials file removed.", - "Run `archgate login` to re-authenticate." - ); - return null; - } - - const gitCreds = await gitCredentialFill(); - if (gitCreds) { - return { token: gitCreds.password, github_user: gitCreds.username }; - } - return null; -} - -/** - * Remove stored credentials (logout). - * Clears the OS credential manager and any legacy metadata file. - */ -export async function clearCredentials(): Promise { - const gitCreds = await gitCredentialFill(); - if (gitCreds) { - await gitCredentialReject(gitCreds.username, gitCreds.password); - logDebug("Token removed from git credential manager"); - } - await cleanupLegacyMetadata(); -} diff --git a/src/helpers/credential-store.ts b/src/helpers/credential-store.ts index e6e3db00..849b5dc1 100644 --- a/src/helpers/credential-store.ts +++ b/src/helpers/credential-store.ts @@ -1,36 +1,242 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * credential-store.ts — Public API for credential storage. + * credential-store.ts — Secure credential storage using git's native credential helpers. * - * Wraps credential-store-impl.ts with delegate functions (NOT live re-exports) - * so that mock.module("credential-store") in login-flow.test.ts does NOT follow - * the ESM binding chain into credential-store-impl.ts. A live - * `export { X } from "./Y"` creates a binding that Bun's mock.module replaces - * at the source, poisoning credential-store-impl.ts for other test files. - * Wrapper functions are their own bindings — mocking credential-store.ts - * replaces the wrappers, leaving credential-store-impl.ts untouched. + * Tokens are stored exclusively in the OS credential manager (macOS Keychain, + * Windows Credential Manager, libsecret) via `git credential approve/fill/reject`. + * Nothing is written to disk — no metadata files, no plaintext tokens. + * + * The `username` field in the git credential protocol carries the GitHub username, + * and the `password` field carries the archgate plugin token. + * + * @see https://git-scm.com/docs/git-credential + */ + +import { unlinkSync } from "node:fs"; + +import { logDebug, logWarn } from "./log"; +import { internalPath } from "./paths"; + +const CREDENTIAL_HOST = "plugins.archgate.dev"; +const CREDENTIAL_TIMEOUT_MS = 3_000; + +/** + * Build env for git credential commands at call time (not import time). + * + * Suppresses ALL interactive prompts — terminal, GUI, and askpass — across + * platforms and Git Credential Manager (GCM) versions: + * + * - GIT_TERMINAL_PROMPT=0 — git's own terminal prompt + * - GCM_INTERACTIVE=never — GCM interactive mode (terminal + GUI) + * - GCM_GUI_PROMPT=false — GCM GUI-only prompt (Windows toast/dialog) + * - GIT_ASKPASS="" — external askpass program + * - SSH_ASKPASS="" — SSH askpass fallback (some helpers reuse it) + */ +function gitCredentialEnv(): Record { + return { + ...Bun.env, + GIT_TERMINAL_PROMPT: "0", + GCM_INTERACTIVE: "never", + GCM_GUI_PROMPT: "false", + GIT_ASKPASS: "", + SSH_ASKPASS: "", + }; +} + +export interface StoredCredentials { + token: string; + github_user: string; +} + +// --------------------------------------------------------------------------- +// Git credential protocol helpers +// --------------------------------------------------------------------------- + +function credentialInput(username?: string, password?: string): string { + const lines = ["protocol=https", `host=${CREDENTIAL_HOST}`]; + if (username) lines.push(`username=${username}`); + if (password) lines.push(`password=${password}`); + lines.push("", ""); + return lines.join("\n"); +} + +async function gitCredentialApprove( + username: string, + password: string +): Promise { + const proc = Bun.spawn(["git", "credential", "approve"], { + stdin: new Blob([credentialInput(username, password)]), + stdout: "pipe", + stderr: "pipe", + env: gitCredentialEnv(), + }); + return (await proc.exited) === 0; +} + +async function gitCredentialFill(): Promise<{ + username: string; + password: string; +} | null> { + try { + const proc = Bun.spawn(["git", "credential", "fill"], { + stdin: new Blob([credentialInput()]), + stdout: "pipe", + stderr: "pipe", + env: gitCredentialEnv(), + }); + + // The timeout MUST be cancelled when the spawn wins the race — + // `Bun.sleep` / `setTimeout` both keep the event loop alive for + // their full duration, which used to add 3s of latency to + // commands that call `loadCredentials()` (e.g. `archgate doctor`). + let timer: ReturnType | undefined; + const result = await Promise.race([ + (async () => { + const stdout = await new Response(proc.stdout).text(); + const exitCode = await proc.exited; + return { stdout, exitCode }; + })(), + new Promise((resolve) => { + timer = setTimeout(() => { + proc.kill(); + resolve(null); + }, CREDENTIAL_TIMEOUT_MS); + }), + ]).finally(() => { + if (timer) clearTimeout(timer); + }); + + if (!result || result.exitCode !== 0) return null; + + let username = ""; + let password = ""; + for (const line of result.stdout.split("\n")) { + if (line.startsWith("username=")) username = line.slice(9); + if (line.startsWith("password=")) password = line.slice(9); + } + return username && password ? { username, password } : null; + } catch { + return null; + } +} + +async function gitCredentialReject( + username: string, + password: string +): Promise { + const proc = Bun.spawn(["git", "credential", "reject"], { + stdin: new Blob([credentialInput(username, password)]), + stdout: "pipe", + stderr: "pipe", + env: gitCredentialEnv(), + }); + await proc.exited; +} + +// --------------------------------------------------------------------------- +// Legacy metadata file cleanup +// --------------------------------------------------------------------------- + +/** Path to the legacy metadata file (~/.archgate/credentials). */ +function legacyMetadataPath(): string { + return internalPath("credentials"); +} + +/** + * Delete the legacy ~/.archgate/credentials file if it exists. + * Returns true if a file was found and deleted. */ +async function cleanupLegacyMetadata(): Promise { + const file = Bun.file(legacyMetadataPath()); + if (await file.exists()) { + unlinkSync(legacyMetadataPath()); + logDebug("Legacy credentials metadata file removed"); + return true; + } + return false; +} -import { - saveCredentials as saveCredentialsImpl, - loadCredentials as loadCredentialsImpl, - clearCredentials as clearCredentialsImpl, -} from "./credential-store-impl"; -import type { StoredCredentials } from "./credential-store-impl"; +// --------------------------------------------------------------------------- +// Public API +// --------------------------------------------------------------------------- -export type { StoredCredentials } from "./credential-store-impl"; +const CREDENTIAL_HELPER_HINT = + "Run `git config --global credential.helper` to check your configuration."; +/** + * Persist archgate credentials in the OS credential manager. + * + * A verification round-trip (`git credential fill`) confirms the token was + * actually persisted — `git credential approve` exits 0 even without a + * configured helper, silently storing nothing. + */ export async function saveCredentials( credentials: StoredCredentials ): Promise { - return await saveCredentialsImpl(credentials); + // Clean up any legacy metadata file from previous versions. + await cleanupLegacyMetadata(); + + const stored = await gitCredentialApprove( + credentials.github_user, + credentials.token + ); + + if (stored) { + const verified = await gitCredentialFill(); + if (verified) { + logDebug("Token verified in git credential manager"); + } else { + logWarn( + "Token could not be verified in git credential manager.", + "Your credential helper may not persist credentials.", + CREDENTIAL_HELPER_HINT, + "Without a working credential helper, you will need to re-login after each session." + ); + } + } else { + logWarn( + "git credential approve failed.", + "Your git credential helper may not be configured.", + CREDENTIAL_HELPER_HINT + ); + } } +/** + * Load stored archgate credentials from the OS credential manager. + * Returns null if no credentials are stored. + * + * If a legacy ~/.archgate/credentials file exists, it is deleted and + * the user is asked to re-login. + */ export async function loadCredentials(): Promise { - return await loadCredentialsImpl(); + // Delete legacy metadata file — force re-login for a clean slate. + const hadLegacy = await cleanupLegacyMetadata(); + if (hadLegacy) { + logWarn( + "Legacy credentials file removed.", + "Run `archgate login` to re-authenticate." + ); + return null; + } + + const gitCreds = await gitCredentialFill(); + if (gitCreds) { + return { token: gitCreds.password, github_user: gitCreds.username }; + } + return null; } +/** + * Remove stored credentials (logout). + * Clears the OS credential manager and any legacy metadata file. + */ export async function clearCredentials(): Promise { - return await clearCredentialsImpl(); + const gitCreds = await gitCredentialFill(); + if (gitCreds) { + await gitCredentialReject(gitCreds.username, gitCreds.password); + logDebug("Token removed from git credential manager"); + } + await cleanupLegacyMetadata(); } diff --git a/tests/commands/plugin/install.test.ts b/tests/commands/plugin/install.test.ts index 821ed1ec..e612d8f5 100644 --- a/tests/commands/plugin/install.test.ts +++ b/tests/commands/plugin/install.test.ts @@ -14,17 +14,10 @@ import { // Module mocks — declared before imports that use them. // --------------------------------------------------------------------------- -const mockLoadCredentials = mock< - () => Promise<{ token: string; github_user: string } | null> ->(() => Promise.resolve(null)); -// Provide ALL exports so other test files that resolve this module -// (via mock.module's process-global replacement) get callable functions -// instead of undefined. The impl tests import from credential-store-impl.ts. -mock.module("../../../src/helpers/credential-store", () => ({ - saveCredentials: mock(() => Promise.resolve()), - loadCredentials: mockLoadCredentials, - clearCredentials: mock(() => Promise.resolve()), -})); +// loadCredentials is stubbed per-test via spyOn (see beforeEach), NOT +// mock.module — mock.module is process-global and would leak the stub into +// credential-store.test.ts and other consumers. +let mockLoadCredentials: ReturnType; const mockInstallClaudePlugin = mock(() => Promise.resolve()); const mockInstallCopilotPlugin = mock(() => Promise.resolve()); @@ -67,10 +60,10 @@ mock.module("../../../src/helpers/vscode-settings", () => ({ configureVscodeSettings: mockConfigureVscodeSettings, })); -// NOTE: Do NOT mock.module paths or credential-store here — it leaks globally -// and breaks session-context tests. Instead, those are mocked above via -// mock.module (credential-store is test-scoped since only plugin/install uses it). -// For findProjectRoot we use spyOn in beforeEach below. +// NOTE: Do NOT mock.module paths or credential-store here — mock.module is +// process-global and leaks into other test files (e.g. session-context and +// credential-store tests). For first-party modules we use spyOn in beforeEach +// (findProjectRoot, loadCredentials), which is per-test and auto-restored. // --------------------------------------------------------------------------- // Imports under test — loaded AFTER mocks are registered. @@ -79,6 +72,7 @@ mock.module("../../../src/helpers/vscode-settings", () => ({ import { Command } from "@commander-js/extra-typings"; import { registerPluginInstallCommand } from "../../../src/commands/plugin/install"; +import * as credentialStore from "../../../src/helpers/credential-store"; import * as pathsMod from "../../../src/helpers/paths"; // --------------------------------------------------------------------------- @@ -115,9 +109,12 @@ beforeEach(() => { throw new Error("process.exit called"); }); spyOn(pathsMod, "findProjectRoot").mockReturnValue("/fake/project"); + mockLoadCredentials = spyOn( + credentialStore, + "loadCredentials" + ).mockResolvedValue(null); // Reset all mocks - mockLoadCredentials.mockReset(); mockInstallClaudePlugin.mockReset(); mockInstallCopilotPlugin.mockReset(); mockInstallVscodeExtension.mockReset(); @@ -131,7 +128,6 @@ beforeEach(() => { mockConfigureVscodeSettings.mockReset(); // Default implementations - mockLoadCredentials.mockImplementation(() => Promise.resolve(null)); mockInstallClaudePlugin.mockImplementation(() => Promise.resolve()); mockInstallCopilotPlugin.mockImplementation(() => Promise.resolve()); mockInstallVscodeExtension.mockImplementation((_token: string) => diff --git a/tests/helpers/auth-poll.test.ts b/tests/helpers/auth-poll.test.ts deleted file mode 100644 index 4a886570..00000000 --- a/tests/helpers/auth-poll.test.ts +++ /dev/null @@ -1,150 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// Copyright 2026 Archgate -import { describe, expect, test, mock } from "bun:test"; - -// Import from auth-poll.ts (not auth.ts) so that mock.module() calls in -// parallel test files (e.g. login-flow.test.ts which mocks "../../src/helpers/auth") -// do not replace this binding. Bun's mock.module is process-global and retroactive -// — only importing from a different physical file provides reliable isolation. -import { pollForAccessToken } from "../../src/helpers/auth-poll"; - -/** Type-safe fetch mock — Bun's fetch type includes `preconnect` which mock() doesn't provide. */ -function mockFetch(handler: () => Promise) { - globalThis.fetch = mock(handler) as unknown as typeof fetch; -} - -describe("pollForAccessToken", () => { - test("returns token after authorization_pending then success", async () => { - const originalFetch = globalThis.fetch; - const originalSleep = Bun.sleep; - Bun.sleep = mock(() => Promise.resolve()) as unknown as typeof Bun.sleep; - - let callCount = 0; - globalThis.fetch = mock(() => { - callCount++; - if (callCount === 1) { - return Promise.resolve( - Response.json({ error: "authorization_pending" }) - ); - } - return Promise.resolve( - Response.json({ - access_token: "gho_polled_token", - token_type: "bearer", - scope: "read:user", - }) - ); - }) as unknown as typeof fetch; - - try { - const token = await pollForAccessToken("dc_abc", 0, 60); - expect(token).toBe("gho_polled_token"); - expect(callCount).toBe(2); - } finally { - globalThis.fetch = originalFetch; - Bun.sleep = originalSleep; - } - }); - - test("handles slow_down by increasing poll interval", async () => { - const originalFetch = globalThis.fetch; - const originalSleep = Bun.sleep; - const sleepArgs: number[] = []; - Bun.sleep = mock((ms: number) => { - sleepArgs.push(ms); - return Promise.resolve(); - }) as unknown as typeof Bun.sleep; - - let callCount = 0; - globalThis.fetch = mock(() => { - callCount++; - if (callCount === 1) { - return Promise.resolve(Response.json({ error: "slow_down" })); - } - return Promise.resolve( - Response.json({ - access_token: "gho_after_slow_down", - token_type: "bearer", - scope: "read:user", - }) - ); - }) as unknown as typeof fetch; - - try { - const token = await pollForAccessToken("dc_abc", 0, 60); - expect(token).toBe("gho_after_slow_down"); - // After slow_down, interval increases by 5; second sleep should be 5*1000 - expect(sleepArgs[1]).toBe(5 * 1000); - } finally { - globalThis.fetch = originalFetch; - Bun.sleep = originalSleep; - } - }); - - test("throws on expired_token", async () => { - const originalFetch = globalThis.fetch; - const originalSleep = Bun.sleep; - Bun.sleep = mock(() => Promise.resolve()) as unknown as typeof Bun.sleep; - - mockFetch(() => - Promise.resolve( - Response.json({ - error: "expired_token", - error_description: "The device code has expired.", - }) - ) - ); - - try { - await expect(pollForAccessToken("dc_abc", 0, 60)).rejects.toThrow( - "The device code has expired." - ); - } finally { - globalThis.fetch = originalFetch; - Bun.sleep = originalSleep; - } - }); - - test("throws on access_denied", async () => { - const originalFetch = globalThis.fetch; - const originalSleep = Bun.sleep; - Bun.sleep = mock(() => Promise.resolve()) as unknown as typeof Bun.sleep; - - mockFetch(() => - Promise.resolve( - Response.json({ - error: "access_denied", - error_description: "The user denied your request.", - }) - ) - ); - - try { - await expect(pollForAccessToken("dc_abc", 0, 60)).rejects.toThrow( - "The user denied your request." - ); - } finally { - globalThis.fetch = originalFetch; - Bun.sleep = originalSleep; - } - }); - - test("throws when deadline expires before authorization", async () => { - const originalFetch = globalThis.fetch; - const originalSleep = Bun.sleep; - Bun.sleep = mock(() => Promise.resolve()) as unknown as typeof Bun.sleep; - - mockFetch(() => - Promise.resolve(Response.json({ error: "authorization_pending" })) - ); - - try { - await expect(pollForAccessToken("dc_abc", 0, 0)).rejects.toThrow( - "Device code expired. Please try again." - ); - } finally { - globalThis.fetch = originalFetch; - Bun.sleep = originalSleep; - } - }); -}); diff --git a/tests/helpers/auth.test.ts b/tests/helpers/auth.test.ts index cfce34c6..24bfab42 100644 --- a/tests/helpers/auth.test.ts +++ b/tests/helpers/auth.test.ts @@ -276,4 +276,150 @@ describe("auth", () => { } }); }); + + describe("pollForAccessToken", () => { + test("returns token after authorization_pending then success", async () => { + const { pollForAccessToken } = await import("../../src/helpers/auth"); + + const originalFetch = globalThis.fetch; + const originalSleep = Bun.sleep; + Bun.sleep = mock(() => Promise.resolve()) as unknown as typeof Bun.sleep; + + let callCount = 0; + globalThis.fetch = mock(() => { + callCount++; + if (callCount === 1) { + return Promise.resolve( + Response.json({ error: "authorization_pending" }) + ); + } + return Promise.resolve( + Response.json({ + access_token: "gho_polled_token", + token_type: "bearer", + scope: "read:user", + }) + ); + }) as unknown as typeof fetch; + + try { + const token = await pollForAccessToken("dc_abc", 0, 60); + expect(token).toBe("gho_polled_token"); + expect(callCount).toBe(2); + } finally { + globalThis.fetch = originalFetch; + Bun.sleep = originalSleep; + } + }); + + test("handles slow_down by increasing poll interval", async () => { + const { pollForAccessToken } = await import("../../src/helpers/auth"); + + const originalFetch = globalThis.fetch; + const originalSleep = Bun.sleep; + const sleepArgs: number[] = []; + Bun.sleep = mock((ms: number) => { + sleepArgs.push(ms); + return Promise.resolve(); + }) as unknown as typeof Bun.sleep; + + let callCount = 0; + globalThis.fetch = mock(() => { + callCount++; + if (callCount === 1) { + return Promise.resolve(Response.json({ error: "slow_down" })); + } + return Promise.resolve( + Response.json({ + access_token: "gho_after_slow_down", + token_type: "bearer", + scope: "read:user", + }) + ); + }) as unknown as typeof fetch; + + try { + const token = await pollForAccessToken("dc_abc", 0, 60); + expect(token).toBe("gho_after_slow_down"); + // After slow_down, interval increases by 5; second sleep should be 5*1000 + expect(sleepArgs[1]).toBe(5 * 1000); + } finally { + globalThis.fetch = originalFetch; + Bun.sleep = originalSleep; + } + }); + + test("throws on expired_token", async () => { + const { pollForAccessToken } = await import("../../src/helpers/auth"); + + const originalFetch = globalThis.fetch; + const originalSleep = Bun.sleep; + Bun.sleep = mock(() => Promise.resolve()) as unknown as typeof Bun.sleep; + + mockFetch(() => + Promise.resolve( + Response.json({ + error: "expired_token", + error_description: "The device code has expired.", + }) + ) + ); + + try { + await expect(pollForAccessToken("dc_abc", 0, 60)).rejects.toThrow( + "The device code has expired." + ); + } finally { + globalThis.fetch = originalFetch; + Bun.sleep = originalSleep; + } + }); + + test("throws on access_denied", async () => { + const { pollForAccessToken } = await import("../../src/helpers/auth"); + + const originalFetch = globalThis.fetch; + const originalSleep = Bun.sleep; + Bun.sleep = mock(() => Promise.resolve()) as unknown as typeof Bun.sleep; + + mockFetch(() => + Promise.resolve( + Response.json({ + error: "access_denied", + error_description: "The user denied your request.", + }) + ) + ); + + try { + await expect(pollForAccessToken("dc_abc", 0, 60)).rejects.toThrow( + "The user denied your request." + ); + } finally { + globalThis.fetch = originalFetch; + Bun.sleep = originalSleep; + } + }); + + test("throws when deadline expires before authorization", async () => { + const { pollForAccessToken } = await import("../../src/helpers/auth"); + + const originalFetch = globalThis.fetch; + const originalSleep = Bun.sleep; + Bun.sleep = mock(() => Promise.resolve()) as unknown as typeof Bun.sleep; + + mockFetch(() => + Promise.resolve(Response.json({ error: "authorization_pending" })) + ); + + try { + await expect(pollForAccessToken("dc_abc", 0, 0)).rejects.toThrow( + "Device code expired. Please try again." + ); + } finally { + globalThis.fetch = originalFetch; + Bun.sleep = originalSleep; + } + }); + }); }); diff --git a/tests/helpers/credential-store-impl.test.ts b/tests/helpers/credential-store-impl.test.ts deleted file mode 100644 index 0b24d034..00000000 --- a/tests/helpers/credential-store-impl.test.ts +++ /dev/null @@ -1,235 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// Copyright 2026 Archgate -import { describe, expect, test, beforeEach, afterEach, spyOn } from "bun:test"; -import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; - -// Import from credential-store-impl.ts (not credential-store.ts) so that -// mock.module() calls in parallel test files (e.g. login-flow.test.ts which -// mocks "../../src/helpers/credential-store") do not replace these bindings. -// Bun's mock.module is process-global and retroactive — only importing from -// a different physical file provides reliable isolation. -import { - saveCredentials, - loadCredentials, - clearCredentials, -} from "../../src/helpers/credential-store-impl"; - -describe("credential-store", () => { - let tempDir: string; - let originalHome: string | undefined; - let originalGitConfigNoSystem: string | undefined; - let originalGitConfigGlobal: string | undefined; - - beforeEach(() => { - tempDir = mkdtempSync(join(tmpdir(), "archgate-credstore-test-")); - originalHome = Bun.env.HOME; - originalGitConfigNoSystem = Bun.env.GIT_CONFIG_NOSYSTEM; - originalGitConfigGlobal = Bun.env.GIT_CONFIG_GLOBAL; - Bun.env.HOME = tempDir; - // Isolate git credential operations from the system credential store. - Bun.env.GIT_CONFIG_NOSYSTEM = "1"; - const emptyGitConfig = join(tempDir, ".gitconfig"); - writeFileSync(emptyGitConfig, ""); - Bun.env.GIT_CONFIG_GLOBAL = emptyGitConfig; - }); - - afterEach(() => { - Bun.env.HOME = originalHome; - Bun.env.GIT_CONFIG_NOSYSTEM = originalGitConfigNoSystem; - Bun.env.GIT_CONFIG_GLOBAL = originalGitConfigGlobal; - try { - rmSync(tempDir, { recursive: true, force: true }); - } catch { - /* temp dir cleanup best-effort */ - } - }); - - describe("saveCredentials", () => { - test("does not write any metadata file to disk", async () => { - await saveCredentials({ - token: "ag_beta_abc123", - github_user: "testuser", - }); - - // No credentials file should be written — everything is in git credential manager. - const credPath = join(tempDir, ".archgate", "credentials"); - expect(await Bun.file(credPath).exists()).toBe(false); - }); - - // This test depends on saveCredentials actually removing a legacy file, - // which requires a working git credential helper. On Linux CI without a - // configured helper, the credential flow does not behave the same way. - test.skipIf(process.platform !== "win32")( - "cleans up legacy metadata file on save", - async () => { - // Create a legacy metadata file - mkdirSync(join(tempDir, ".archgate"), { recursive: true }); - const credPath = join(tempDir, ".archgate", "credentials"); - await Bun.write( - credPath, - JSON.stringify({ github_user: "old", created_at: "2025-01-01" }) - ); - - await saveCredentials({ - token: "ag_beta_abc123", - github_user: "testuser", - }); - - // Legacy file should be removed. - expect(await Bun.file(credPath).exists()).toBe(false); - } - ); - - // This test relies on git credential approve + fill behavior which - // differs based on the configured credential helper. - test.skipIf(process.platform !== "win32")( - "warns when verification after approve fails", - async () => { - // With no credential helper configured, approve succeeds (exit 0) but - // fill returns nothing — triggers the verification warning path. - const warnSpy = spyOn(console, "warn").mockImplementation(() => {}); - try { - await saveCredentials({ - token: "ag_beta_test", - github_user: "testuser", - }); - - // The warning is printed because fill cannot verify the stored token. - // Either the "approve failed" or "could not be verified" warning fires. - expect(warnSpy).toHaveBeenCalled(); - const allArgs = warnSpy.mock.calls.flat().join(" "); - const hasVerifyWarning = - allArgs.includes("could not be verified") || - allArgs.includes("approve failed"); - expect(hasVerifyWarning).toBe(true); - } finally { - warnSpy.mockRestore(); - } - } - ); - }); - - describe("loadCredentials", () => { - test("returns null when no credentials exist anywhere", async () => { - const result = await loadCredentials(); - expect(result).toBeNull(); - }); - - test("returns null and deletes legacy metadata file", async () => { - const credPath = join(tempDir, ".archgate", "credentials"); - mkdirSync(join(tempDir, ".archgate"), { recursive: true }); - await Bun.write( - credPath, - JSON.stringify({ - token: "ag_beta_legacy", - github_user: "testuser", - created_at: "2026-01-15", - }) - ); - - // Legacy file triggers deletion and returns null (re-login required). - const result = await loadCredentials(); - expect(result).toBeNull(); - expect(await Bun.file(credPath).exists()).toBe(false); - }); - - test("returns null when no git creds and no legacy file", async () => { - // With isolated git config (no credential helper), returns null. - const result = await loadCredentials(); - expect(result).toBeNull(); - }); - }); - - describe("clearCredentials", () => { - test("does not throw when no credentials exist", async () => { - await expect(clearCredentials()).resolves.toBeUndefined(); - }); - - test("cleans up legacy metadata file", async () => { - mkdirSync(join(tempDir, ".archgate"), { recursive: true }); - const credPath = join(tempDir, ".archgate", "credentials"); - await Bun.write( - credPath, - JSON.stringify({ github_user: "testuser", created_at: "2026-01-15" }) - ); - - await clearCredentials(); - - expect(await Bun.file(credPath).exists()).toBe(false); - }); - - test("completes without error when git credential reject runs", async () => { - // clearCredentials calls gitCredentialFill first; with no helper - // configured, fill returns null so reject is skipped — but legacy - // cleanup still runs. This exercises the full clearCredentials path. - - mkdirSync(join(tempDir, ".archgate"), { recursive: true }); - const credPath = join(tempDir, ".archgate", "credentials"); - await Bun.write(credPath, "{}"); - - await clearCredentials(); - expect(await Bun.file(credPath).exists()).toBe(false); - }); - }); - - describe("credential fill with store helper", () => { - // This test depends on git credential store + fill round-tripping - // correctly with env var overrides. The credential-store module's - // gitCredentialEnv() spreads Bun.env at call time and the store helper - // interaction differs across platforms. Skipped until we can reliably - // isolate the credential helper in all CI environments. - test.skip("round-trips credentials through a file-based credential helper", async () => { - // Configure a simple store-based credential helper that persists - // credentials to a file. This lets us exercise the approve→fill→reject - // cycle end-to-end without touching the OS credential manager. - const storePath = join(tempDir, "git-credentials"); - const gitConfig = join(tempDir, ".gitconfig"); - writeFileSync( - gitConfig, - `[credential]\n helper = store --file=${storePath}\n` - ); - // Point git at our custom config so the store helper is used - Bun.env.GIT_CONFIG_GLOBAL = gitConfig; - - // Save should succeed and be verifiable - const warnSpy = spyOn(console, "warn").mockImplementation(() => {}); - try { - await saveCredentials({ - token: "ag_beta_roundtrip", - github_user: "rounduser", - }); - - // With a working helper, verification succeeds — no warning about - // "could not be verified". - const verifyWarning = warnSpy.mock.calls - .flat() - .join(" ") - .includes("could not be verified"); - expect(verifyWarning).toBe(false); - } finally { - warnSpy.mockRestore(); - } - - // Load should return the saved credentials - const loaded = await loadCredentials(); - expect(loaded).not.toBeNull(); - expect(loaded!.token).toBe("ag_beta_roundtrip"); - expect(loaded!.github_user).toBe("rounduser"); - - // Clear should remove them - await clearCredentials(); - const afterClear = await loadCredentials(); - expect(afterClear).toBeNull(); - }); - }); - - describe("StoredCredentials type", () => { - test("interface has expected shape", () => { - expect(typeof saveCredentials).toBe("function"); - expect(typeof loadCredentials).toBe("function"); - expect(typeof clearCredentials).toBe("function"); - }); - }); -}); diff --git a/tests/helpers/credential-store.test.ts b/tests/helpers/credential-store.test.ts index 437965ff..1f8a5d5e 100644 --- a/tests/helpers/credential-store.test.ts +++ b/tests/helpers/credential-store.test.ts @@ -1,32 +1,230 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -/** - * Verifies that credential-store.ts correctly re-exports from - * credential-store-impl.ts. The full implementation tests live in - * credential-store-impl.test.ts — this file only checks the re-export surface. - */ -import { describe, expect, test } from "bun:test"; - -// Import from the re-export wrapper (credential-store.ts). -// Note: login-flow.test.ts mocks this path via mock.module(), so in a shared -// test process these may be mock functions. The assertions below only check -// that the exports exist and are functions — they don't invoke real behavior. +import { describe, expect, test, beforeEach, afterEach, spyOn } from "bun:test"; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + import { saveCredentials, loadCredentials, clearCredentials, } from "../../src/helpers/credential-store"; -describe("credential-store (re-export surface)", () => { - test("saveCredentials is exported as a function", () => { - expect(typeof saveCredentials).toBe("function"); +describe("credential-store", () => { + let tempDir: string; + let originalHome: string | undefined; + let originalGitConfigNoSystem: string | undefined; + let originalGitConfigGlobal: string | undefined; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-credstore-test-")); + originalHome = Bun.env.HOME; + originalGitConfigNoSystem = Bun.env.GIT_CONFIG_NOSYSTEM; + originalGitConfigGlobal = Bun.env.GIT_CONFIG_GLOBAL; + Bun.env.HOME = tempDir; + // Isolate git credential operations from the system credential store. + Bun.env.GIT_CONFIG_NOSYSTEM = "1"; + const emptyGitConfig = join(tempDir, ".gitconfig"); + writeFileSync(emptyGitConfig, ""); + Bun.env.GIT_CONFIG_GLOBAL = emptyGitConfig; + }); + + afterEach(() => { + Bun.env.HOME = originalHome; + Bun.env.GIT_CONFIG_NOSYSTEM = originalGitConfigNoSystem; + Bun.env.GIT_CONFIG_GLOBAL = originalGitConfigGlobal; + try { + rmSync(tempDir, { recursive: true, force: true }); + } catch { + /* temp dir cleanup best-effort */ + } }); - test("loadCredentials is exported as a function", () => { - expect(typeof loadCredentials).toBe("function"); + describe("saveCredentials", () => { + test("does not write any metadata file to disk", async () => { + await saveCredentials({ + token: "ag_beta_abc123", + github_user: "testuser", + }); + + // No credentials file should be written — everything is in git credential manager. + const credPath = join(tempDir, ".archgate", "credentials"); + expect(await Bun.file(credPath).exists()).toBe(false); + }); + + // This test depends on saveCredentials actually removing a legacy file, + // which requires a working git credential helper. On Linux CI without a + // configured helper, the credential flow does not behave the same way. + test.skipIf(process.platform !== "win32")( + "cleans up legacy metadata file on save", + async () => { + // Create a legacy metadata file + mkdirSync(join(tempDir, ".archgate"), { recursive: true }); + const credPath = join(tempDir, ".archgate", "credentials"); + await Bun.write( + credPath, + JSON.stringify({ github_user: "old", created_at: "2025-01-01" }) + ); + + await saveCredentials({ + token: "ag_beta_abc123", + github_user: "testuser", + }); + + // Legacy file should be removed. + expect(await Bun.file(credPath).exists()).toBe(false); + } + ); + + // This test relies on git credential approve + fill behavior which + // differs based on the configured credential helper. + test.skipIf(process.platform !== "win32")( + "warns when verification after approve fails", + async () => { + // With no credential helper configured, approve succeeds (exit 0) but + // fill returns nothing — triggers the verification warning path. + const warnSpy = spyOn(console, "warn").mockImplementation(() => {}); + try { + await saveCredentials({ + token: "ag_beta_test", + github_user: "testuser", + }); + + // The warning is printed because fill cannot verify the stored token. + // Either the "approve failed" or "could not be verified" warning fires. + expect(warnSpy).toHaveBeenCalled(); + const allArgs = warnSpy.mock.calls.flat().join(" "); + const hasVerifyWarning = + allArgs.includes("could not be verified") || + allArgs.includes("approve failed"); + expect(hasVerifyWarning).toBe(true); + } finally { + warnSpy.mockRestore(); + } + } + ); + }); + + describe("loadCredentials", () => { + test("returns null when no credentials exist anywhere", async () => { + const result = await loadCredentials(); + expect(result).toBeNull(); + }); + + test("returns null and deletes legacy metadata file", async () => { + const credPath = join(tempDir, ".archgate", "credentials"); + mkdirSync(join(tempDir, ".archgate"), { recursive: true }); + await Bun.write( + credPath, + JSON.stringify({ + token: "ag_beta_legacy", + github_user: "testuser", + created_at: "2026-01-15", + }) + ); + + // Legacy file triggers deletion and returns null (re-login required). + const result = await loadCredentials(); + expect(result).toBeNull(); + expect(await Bun.file(credPath).exists()).toBe(false); + }); + + test("returns null when no git creds and no legacy file", async () => { + // With isolated git config (no credential helper), returns null. + const result = await loadCredentials(); + expect(result).toBeNull(); + }); + }); + + describe("clearCredentials", () => { + test("does not throw when no credentials exist", async () => { + await expect(clearCredentials()).resolves.toBeUndefined(); + }); + + test("cleans up legacy metadata file", async () => { + mkdirSync(join(tempDir, ".archgate"), { recursive: true }); + const credPath = join(tempDir, ".archgate", "credentials"); + await Bun.write( + credPath, + JSON.stringify({ github_user: "testuser", created_at: "2026-01-15" }) + ); + + await clearCredentials(); + + expect(await Bun.file(credPath).exists()).toBe(false); + }); + + test("completes without error when git credential reject runs", async () => { + // clearCredentials calls gitCredentialFill first; with no helper + // configured, fill returns null so reject is skipped — but legacy + // cleanup still runs. This exercises the full clearCredentials path. + + mkdirSync(join(tempDir, ".archgate"), { recursive: true }); + const credPath = join(tempDir, ".archgate", "credentials"); + await Bun.write(credPath, "{}"); + + await clearCredentials(); + expect(await Bun.file(credPath).exists()).toBe(false); + }); + }); + + describe("credential fill with store helper", () => { + // This test depends on git credential store + fill round-tripping + // correctly with env var overrides. The credential-store module's + // gitCredentialEnv() spreads Bun.env at call time and the store helper + // interaction differs across platforms. Skipped until we can reliably + // isolate the credential helper in all CI environments. + test.skip("round-trips credentials through a file-based credential helper", async () => { + // Configure a simple store-based credential helper that persists + // credentials to a file. This lets us exercise the approve→fill→reject + // cycle end-to-end without touching the OS credential manager. + const storePath = join(tempDir, "git-credentials"); + const gitConfig = join(tempDir, ".gitconfig"); + writeFileSync( + gitConfig, + `[credential]\n helper = store --file=${storePath}\n` + ); + // Point git at our custom config so the store helper is used + Bun.env.GIT_CONFIG_GLOBAL = gitConfig; + + // Save should succeed and be verifiable + const warnSpy = spyOn(console, "warn").mockImplementation(() => {}); + try { + await saveCredentials({ + token: "ag_beta_roundtrip", + github_user: "rounduser", + }); + + // With a working helper, verification succeeds — no warning about + // "could not be verified". + const verifyWarning = warnSpy.mock.calls + .flat() + .join(" ") + .includes("could not be verified"); + expect(verifyWarning).toBe(false); + } finally { + warnSpy.mockRestore(); + } + + // Load should return the saved credentials + const loaded = await loadCredentials(); + expect(loaded).not.toBeNull(); + expect(loaded!.token).toBe("ag_beta_roundtrip"); + expect(loaded!.github_user).toBe("rounduser"); + + // Clear should remove them + await clearCredentials(); + const afterClear = await loadCredentials(); + expect(afterClear).toBeNull(); + }); }); - test("clearCredentials is exported as a function", () => { - expect(typeof clearCredentials).toBe("function"); + describe("StoredCredentials type", () => { + test("interface has expected shape", () => { + expect(typeof saveCredentials).toBe("function"); + expect(typeof loadCredentials).toBe("function"); + expect(typeof clearCredentials).toBe("function"); + }); }); }); diff --git a/tests/helpers/init-project.test.ts b/tests/helpers/init-project.test.ts index 69506f1e..7d2762a9 100644 --- a/tests/helpers/init-project.test.ts +++ b/tests/helpers/init-project.test.ts @@ -1,14 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -import { - afterEach, - beforeEach, - describe, - expect, - mock, - spyOn, - test, -} from "bun:test"; +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; import { mkdtempSync, rmSync, existsSync, mkdirSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -226,10 +218,12 @@ describe("initProject", () => { test("configures VS Code settings when editor is vscode", async () => { // The vscode branch in configureEditorSettings dynamically imports - // credential-store. Mock it to avoid hitting the real credential store. - mock.module("../../src/helpers/credential-store", () => ({ - loadCredentials: () => Promise.resolve(null), - })); + // credential-store. Spy on it (not mock.module, which leaks globally) to + // avoid hitting the real credential store. spyOn reflects through the + // dynamic import because it targets the same module instance. + const credSpy = spyOn(credentialStore, "loadCredentials").mockResolvedValue( + null + ); try { const result = await initProject(tempDir, { editor: "vscode" }); @@ -241,7 +235,7 @@ describe("initProject", () => { false ); } finally { - mock.restore(); + credSpy.mockRestore(); } }); }); diff --git a/tests/helpers/login-flow.test.ts b/tests/helpers/login-flow.test.ts index aecfc317..b2d97f99 100644 --- a/tests/helpers/login-flow.test.ts +++ b/tests/helpers/login-flow.test.ts @@ -17,44 +17,15 @@ import { /** Mock cursorTo from node:readline (used by prompt.ts withPromptFix). */ mock.module("node:readline", () => ({ cursorTo: mock(() => true) })); -// Mock auth functions — auth-poll.test.ts imports from auth-poll.ts (not auth.ts), -// so this mock does not leak into its tests. -const mockRequestDeviceCode = mock(() => - Promise.resolve({ - device_code: "dc-test-123", - user_code: "ABCD-1234", - verification_uri: "https://github.com/login/device", - expires_in: 900, - interval: 5, - }) -); -const mockPollForAccessToken = mock(() => Promise.resolve("gh-token-test-456")); -const mockGetGitHubUser = mock(() => - Promise.resolve({ login: "octocat", email: "octocat@github.com" }) -); -const mockClaimArchgateToken = mock(() => - Promise.resolve("archgate-token-789") -); - -mock.module("../../src/helpers/auth", () => ({ - requestDeviceCode: mockRequestDeviceCode, - pollForAccessToken: mockPollForAccessToken, - getGitHubUser: mockGetGitHubUser, - claimArchgateToken: mockClaimArchgateToken, -})); - -// Mock credential-store — uses git subprocess, not fetch. -// credential-store.test.ts imports from credential-store-impl.ts (not -// credential-store.ts), so this mock does not leak into its tests. -const mockSaveCredentials = mock(() => Promise.resolve()); -// Provide ALL exports so other test files that resolve this module -// (via mock.module's process-global replacement) get callable functions -// instead of undefined. The impl tests import from credential-store-impl.ts. -mock.module("../../src/helpers/credential-store", () => ({ - saveCredentials: mockSaveCredentials, - loadCredentials: mock(() => Promise.resolve(null)), - clearCredentials: mock(() => Promise.resolve()), -})); +// Auth + credential-store stubs are installed per-test via spyOn (see +// beforeEach), NOT mock.module. spyOn is auto-restored and scoped to this +// file; mock.module is process-global and would leak mocked implementations +// into auth.test.ts and credential-store.test.ts. +let mockRequestDeviceCode: ReturnType; +let mockPollForAccessToken: ReturnType; +let mockGetGitHubUser: ReturnType; +let mockClaimArchgateToken: ReturnType; +let mockSaveCredentials: ReturnType; // Mock inquirer for the signup flow prompts (lazy-loaded via dynamic import). // Use Record as return type so mockImplementation can return @@ -72,6 +43,8 @@ mock.module("inquirer", () => ({ default: { prompt: mockInquirerPrompt } })); // signup.test.ts (which uses static imports). // --------------------------------------------------------------------------- +import * as authMod from "../../src/helpers/auth"; +import * as credMod from "../../src/helpers/credential-store"; import { runLoginFlow } from "../../src/helpers/login-flow"; // --------------------------------------------------------------------------- // Imports under test — loaded AFTER mocks are registered. @@ -89,63 +62,56 @@ import { SignupRequiredError } from "../../src/helpers/signup"; let originalFetch: typeof globalThis.fetch; -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - -let consoleLogSpy: ReturnType; -let consoleErrorSpy: ReturnType; - // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- describe("login-flow", () => { beforeEach(() => { - // Silence console output - consoleLogSpy = spyOn(console, "log").mockImplementation(() => {}); - consoleErrorSpy = spyOn(console, "error").mockImplementation(() => {}); + // Silence console output (restored via mock.restore() in afterEach). + spyOn(console, "log").mockImplementation(() => {}); + spyOn(console, "error").mockImplementation(() => {}); // Save original fetch — only needed for signup tests that go through // the real requestSignup function. originalFetch = globalThis.fetch; - // Clear mock call counts from previous tests - mockRequestDeviceCode.mockClear(); - mockPollForAccessToken.mockClear(); - mockGetGitHubUser.mockClear(); - mockClaimArchgateToken.mockClear(); - mockSaveCredentials.mockClear(); - mockInquirerPrompt.mockClear(); - - // Reset default implementations - mockRequestDeviceCode.mockImplementation(() => - Promise.resolve({ - device_code: "dc-test-123", - user_code: "ABCD-1234", - verification_uri: "https://github.com/login/device", - expires_in: 900, - interval: 5, - }) - ); - mockPollForAccessToken.mockImplementation(() => - Promise.resolve("gh-token-test-456") - ); - mockGetGitHubUser.mockImplementation(() => - Promise.resolve({ login: "octocat", email: "octocat@github.com" }) - ); - mockClaimArchgateToken.mockImplementation(() => - Promise.resolve("archgate-token-789") + // Install fresh per-test spies with default implementations. + mockRequestDeviceCode = spyOn( + authMod, + "requestDeviceCode" + ).mockResolvedValue({ + device_code: "dc-test-123", + user_code: "ABCD-1234", + verification_uri: "https://github.com/login/device", + expires_in: 900, + interval: 5, + }); + mockPollForAccessToken = spyOn( + authMod, + "pollForAccessToken" + ).mockResolvedValue("gh-token-test-456"); + mockGetGitHubUser = spyOn(authMod, "getGitHubUser").mockResolvedValue({ + login: "octocat", + email: "octocat@github.com", + }); + mockClaimArchgateToken = spyOn( + authMod, + "claimArchgateToken" + ).mockResolvedValue("archgate-token-789"); + mockSaveCredentials = spyOn(credMod, "saveCredentials").mockImplementation( + () => Promise.resolve() ); - mockSaveCredentials.mockImplementation(() => Promise.resolve()); + + mockInquirerPrompt.mockClear(); mockInquirerPrompt.mockImplementation(() => Promise.resolve({ email: "test@example.com" }) ); }); afterEach(() => { - consoleLogSpy.mockRestore(); - consoleErrorSpy.mockRestore(); + // Restore all spyOn spies (console + auth + credential-store). + mock.restore(); globalThis.fetch = originalFetch; });