From f6d0eb8ab93783b98675708ba93553cebcfd91a5 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 29 May 2026 01:32:12 +0200 Subject: [PATCH] refactor: replace mock.module test-isolation workaround with spyOn Bun's mock.module() is process-global and retroactive: once a test file mocks a first-party module path, that mock replaces the module for every test file in the same process and is not undone by mock.restore(). To get the real implementations, the suite had split production code into auth-poll.ts and credential-store-impl.ts, with auth.ts/credential-store.ts reduced to thin delegating wrappers, purely so the "real" tests could import an un-mocked path. The isolation depended on test-file execution order, which is the actual source of the flakiness. Fix the root cause instead of contorting the module layout: - Merge auth-poll.ts back into auth.ts and credential-store-impl.ts back into credential-store.ts; delete the wrapper files. - Convert the three leaky mockers (login-flow, init-project, plugin/install) from mock.module() to `import * as mod` + spyOn in beforeEach, restored via mock.restore() in afterEach. spyOn is per-test and auto-restored, and it reaches both static named-import and dynamic `await import()` consumers because it mutates the single shared module instance (same pattern already used for pathsMod.findProjectRoot). Third-party mocks (inquirer, node:readline) keep using mock.module. - Consolidate tests: fold auth-poll.test.ts into auth.test.ts; drop the stub credential-store re-export test and move the real impl tests into credential-store.test.ts. - Document the rule in ARCH-005 (DO/DON'T, good/bad example, compliance item) so the workaround does not reappear. Signed-off-by: Rhuan Barreto --- .archgate/adrs/ARCH-005-testing-standards.md | 19 ++ src/helpers/auth-poll.ts | 120 --------- src/helpers/auth.ts | 103 ++++++-- src/helpers/credential-store-impl.ts | 248 ------------------- src/helpers/credential-store.ts | 242 ++++++++++++++++-- tests/commands/plugin/install.test.ts | 30 +-- tests/helpers/auth-poll.test.ts | 150 ----------- tests/helpers/auth.test.ts | 146 +++++++++++ tests/helpers/credential-store-impl.test.ts | 235 ------------------ tests/helpers/credential-store.test.ts | 234 +++++++++++++++-- tests/helpers/init-project.test.ts | 22 +- tests/helpers/login-flow.test.ts | 120 ++++----- 12 files changed, 757 insertions(+), 912 deletions(-) delete mode 100644 src/helpers/auth-poll.ts delete mode 100644 src/helpers/credential-store-impl.ts delete mode 100644 tests/helpers/auth-poll.test.ts delete mode 100644 tests/helpers/credential-store-impl.test.ts 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; });