From c7a391d6b3b19c6428e869238176431f921c3919 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 12:17:03 +0300 Subject: [PATCH 01/15] wip: AIT-438 CLI should accept an API key from the environment From 2a14230da4c38407d189bb3d9858cbae8b3dac75 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 12:31:55 +0300 Subject: [PATCH 02/15] feat(auth): AIT-438 accept an org API key from HOOKMYAPP_API_KEY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the credential in one place — readCredentials() — so every authenticated path (apiClient, doctor, listen loops, rescope) picks up the environment key without its own branch. The key is shaped as a kind: 'agent' credential, which is exactly what `credentials create` persists, so refresh and rescope stay no-ops for it. Precedence: the environment beats the stored credential, matching gh (GH_TOKEN), aws, vercel and stripe. The reverse order silently ignores the key an agent or CI job was handed — the case this ticket exists for. Kept visible rather than silent: doctor names the source, logout warns that the variable still authenticates, and login refuses to run while it is set instead of storing a credential that would never be used. Both hmok_ and legacy ac_ prefixes are accepted, mirroring the backend's isAgentToken; a set-but-malformed value throws an AuthError naming the variable instead of falling through to "Not logged in". Also adds HOOKMYAPP_WORKSPACE_ID: a headless caller has no workspace config, and getDefaultWorkspaceId() dead-ends on a multi-workspace org with advice ("workspace use") that a spawned process cannot follow. --- src/auth/__tests__/env-api-key.test.ts | 73 ++++++++++++++++++++++++++ src/auth/login.ts | 11 ++++ src/auth/logout.ts | 16 ++++-- src/auth/store.ts | 50 +++++++++++++++++- src/commands/__tests__/doctor.test.ts | 37 ++++++++++++- src/commands/_helpers.ts | 20 ++++++- src/commands/doctor.ts | 24 +++++++-- src/config/env-vars.ts | 9 ++++ src/storage/secrets.ts | 3 ++ 9 files changed, 232 insertions(+), 11 deletions(-) create mode 100644 src/auth/__tests__/env-api-key.test.ts create mode 100644 src/config/env-vars.ts diff --git a/src/auth/__tests__/env-api-key.test.ts b/src/auth/__tests__/env-api-key.test.ts new file mode 100644 index 00000000..3f91c68d --- /dev/null +++ b/src/auth/__tests__/env-api-key.test.ts @@ -0,0 +1,73 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { readCredentials, readEnvCredential } from '../store.js'; +import { API_KEY_ENV_VAR } from '../../config/env-vars.js'; +import * as secrets from '../../storage/secrets.js'; +import { AuthError } from '../../output/error.js'; + +const STORED = { + accessToken: 'stored-token', + refreshToken: 'r', + expiresAt: 0, +} as const; + +describe('HOOKMYAPP_API_KEY credential (AIT-438)', () => { + const original = process.env[API_KEY_ENV_VAR]; + + beforeEach(() => { + delete process.env[API_KEY_ENV_VAR]; + vi.restoreAllMocks(); + }); + afterEach(() => { + if (original === undefined) delete process.env[API_KEY_ENV_VAR]; + else process.env[API_KEY_ENV_VAR] = original; + }); + + it('is used when no credential is stored', async () => { + vi.spyOn(secrets, 'readSecrets').mockResolvedValue(null); + process.env[API_KEY_ENV_VAR] = 'hmok_abc123'; + const creds = await readCredentials(); + expect(creds?.accessToken).toBe('hmok_abc123'); + // Shaped as an agent credential so refresh/rescope stay no-ops. + expect(creds?.kind).toBe('agent'); + expect(creds?.source).toBe('env'); + }); + + it('outranks a stored credential', async () => { + vi.spyOn(secrets, 'readSecrets').mockResolvedValue({ ...STORED }); + process.env[API_KEY_ENV_VAR] = 'hmok_abc123'; + expect((await readCredentials())?.accessToken).toBe('hmok_abc123'); + }); + + it('leaves the stored credential in charge when unset', async () => { + vi.spyOn(secrets, 'readSecrets').mockResolvedValue({ ...STORED }); + const creds = await readCredentials(); + expect(creds?.accessToken).toBe('stored-token'); + expect(creds?.source).toBeUndefined(); + }); + + it('accepts legacy ac_ keys, which the backend still resolves', () => { + process.env[API_KEY_ENV_VAR] = 'ac_legacy'; + expect(readEnvCredential()?.accessToken).toBe('ac_legacy'); + }); + + it('ignores an empty or whitespace-only value', () => { + process.env[API_KEY_ENV_VAR] = ' '; + expect(readEnvCredential()).toBeNull(); + }); + + it('rejects a malformed value, naming the variable', () => { + process.env[API_KEY_ENV_VAR] = 'not-a-key'; + expect(() => readEnvCredential()).toThrow(AuthError); + expect(() => readEnvCredential()).toThrow(/HOOKMYAPP_API_KEY/); + }); + + it('never echoes the key in the malformed-value error', () => { + process.env[API_KEY_ENV_VAR] = 'sk_live_supersecret'; + try { + readEnvCredential(); + throw new Error('expected a throw'); + } catch (err) { + expect((err as Error).message).not.toContain('supersecret'); + } + }); +}); diff --git a/src/auth/login.ts b/src/auth/login.ts index ae5807b0..ccd4b002 100644 --- a/src/auth/login.ts +++ b/src/auth/login.ts @@ -1,5 +1,6 @@ import { Command } from 'commander'; import { saveCredentials, peekIdentity } from './store.js'; +import { API_KEY_ENV_VAR } from '../config/env-vars.js'; import { AuthError, NetworkError, ValidationError } from '../output/error.js'; import { addExamples } from '../output/help.js'; import { c, icon } from '../output/color.js'; @@ -692,6 +693,16 @@ export function loginCommand(program: Command): void { registrationId?: string; scope?: string[]; }) => { + // AIT-438: HOOKMYAPP_API_KEY outranks the stored credential, so a + // login completed now would be authenticated over and have no effect. + // Refuse with the fix rather than let the user sign in for nothing + // (same contract as `gh auth login` under GH_TOKEN). + if (process.env[API_KEY_ENV_VAR]?.trim()) { + throw new ValidationError( + `${API_KEY_ENV_VAR} is set, and it takes precedence over a stored login — signing in now would have no effect. ` + + `Unset ${API_KEY_ENV_VAR} first, then run: ${cliCommandPrefix()} login`, + ); + } // Reject an invalid --next locally (exit 2) instead of silently // coercing it to undefined — `login --next bogus` previously ran the // default flow and exited 0, hiding the typo. diff --git a/src/auth/logout.ts b/src/auth/logout.ts index eeb8db42..976618ee 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -1,6 +1,7 @@ import { Command } from 'commander'; -import { readCredentials, deleteCredentials } from './store.js'; -import { isAgentCredential } from '../storage/secrets.js'; +import { deleteCredentials } from './store.js'; +import { API_KEY_ENV_VAR } from '../config/env-vars.js'; +import { isAgentCredential, readSecrets } from '../storage/secrets.js'; import { addExamples } from '../output/help.js'; import { removeClaudeMcp } from '../commands/mcp.js'; @@ -17,7 +18,11 @@ export function logoutCommand(program: Command): void { // credentials. WorkOS sessions carry no CLI-side revoke, so this only // fires for agent credentials. let revoked = false; - const creds = await readCredentials(); + // Stored credential only (AIT-438): logout manages credentials.json and + // must never revoke a key that came from HOOKMYAPP_API_KEY — the + // environment is not ours to clear, and revoking it server-side would + // break every other process sharing that key. + const creds = await readSecrets(); if (creds && isAgentCredential(creds) && creds.credentialPublicId) { try { const { apiClient } = await import('../api/client.js'); @@ -47,6 +52,11 @@ export function logoutCommand(program: Command): void { ? '\n✓ Logged out\n' : `\n✓ Logged out\n⚠ ${mcpCleanup.detail}\n`, ); + if (process.env[API_KEY_ENV_VAR]?.trim()) { + console.log( + `⚠ ${API_KEY_ENV_VAR} is still set — commands stay authenticated with it. Unset it to sign out fully.\n`, + ); + } } }); diff --git a/src/auth/store.ts b/src/auth/store.ts index 0c5c1e73..a59d1e17 100644 --- a/src/auth/store.ts +++ b/src/auth/store.ts @@ -1,5 +1,7 @@ import { readFileSync } from 'node:fs'; import { getConfigFile } from '../storage/path.js'; +import { AuthError } from '../output/error.js'; +import { API_KEY_ENV_VAR } from '../config/env-vars.js'; import { writeSecrets, readSecrets, @@ -17,8 +19,54 @@ export async function saveCredentials(creds: Credentials): Promise { await writeSecrets(creds); } +/** + * Build a credential from HOOKMYAPP_API_KEY, or null when the variable is + * unset/empty. + * + * Shaped as `kind: 'agent'` on purpose: an org API key is exactly what + * `hookmyapp credentials create` persists, so every existing agent-credential + * path (no refresh, no rescope, bearer sent verbatim) applies unchanged. + * + * Accepted prefixes mirror the backend's `isAgentToken` — `hmok_` is the + * current mint, `ac_` is legacy and still resolves server-side. Rejecting + * `ac_` here would fail keys the API accepts. + * + * Throws rather than returning null on a malformed value: a set-but-wrong + * variable is a configuration mistake, and silently falling through to + * "Not logged in. Run: hookmyapp login" hides where the bad key came from. + */ +export function readEnvCredential(): Credentials | null { + const raw = process.env[API_KEY_ENV_VAR]?.trim(); + if (!raw) return null; + if (!raw.startsWith('hmok_') && !raw.startsWith('ac_')) { + throw new AuthError( + `${API_KEY_ENV_VAR} is not a valid API key (expected it to start with "hmok_"). ` + + `Fix or unset the variable, then retry. Mint a key with: hookmyapp credentials create`, + ); + } + return { + accessToken: raw, + refreshToken: '', + expiresAt: 0, + kind: 'agent', + source: 'env', + }; +} + +/** + * Resolve the credential every authenticated code path uses. + * + * Precedence: HOOKMYAPP_API_KEY beats the stored credential, matching gh + * (GH_TOKEN), aws, vercel and stripe — an explicitly exported key is the more + * deliberate signal, and the reverse order silently ignores the key an agent + * or CI job was handed. The override is reported by `doctor`, and `login` + * refuses to run while the variable is set, so it is never invisible. + * + * `login`/`logout` deliberately bypass this and use readSecrets() directly: + * they manage the stored credential and must not act on the environment. + */ export async function readCredentials(): Promise { - return readSecrets(); + return readEnvCredential() ?? (await readSecrets()); } export async function deleteCredentials(): Promise { diff --git a/src/commands/__tests__/doctor.test.ts b/src/commands/__tests__/doctor.test.ts index edce84ad..72d4c36b 100644 --- a/src/commands/__tests__/doctor.test.ts +++ b/src/commands/__tests__/doctor.test.ts @@ -45,10 +45,41 @@ describe('doctor — auth probe uses the real authenticated request path', () => expect(apiClient).toHaveBeenCalledWith('/workspaces'); expect(report.loggedIn).toBe(true); expect(report.checks.find((c) => c.id === 'auth')!.detail).toBe( - 'credentials valid for this env', + 'credentials valid for this env — stored credentials', ); }); + // AIT-438: which credential is in play must be visible — an env key and a + // stored login produce identical output otherwise. + it('names HOOKMYAPP_API_KEY when the credential came from the environment', async () => { + vi.mocked(apiClient).mockResolvedValue([]); + vi.mocked(readCredentials).mockResolvedValue({ + accessToken: 'hmok_abc', + refreshToken: '', + expiresAt: 0, + kind: 'agent', + source: 'env', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + + const report = await collectDoctorReport({ checkTools: false }); + + expect(report.checks.find((c) => c.id === 'auth')!.detail).toBe( + 'credentials valid for this env — HOOKMYAPP_API_KEY (environment)', + ); + }); + + it('surfaces a malformed env key instead of "not logged in"', async () => { + vi.mocked(readCredentials).mockRejectedValue( + new AuthError('HOOKMYAPP_API_KEY is not a valid API key (expected it to start with "hmok_").'), + ); + + const report = await collectDoctorReport({ checkTools: false }); + + expect(report.loggedIn).toBe(false); + expect(report.checks.find((c) => c.id === 'auth')!.detail).toContain('HOOKMYAPP_API_KEY'); + }); + it('fails auth when apiClient throws AuthError (genuinely invalid credentials)', async () => { vi.mocked(apiClient).mockRejectedValue(new AuthError()); @@ -67,7 +98,9 @@ describe('doctor — auth probe uses the real authenticated request path', () => const report = await collectDoctorReport({ checkTools: false }); expect(report.loggedIn).toBe(true); - expect(report.checks.find((c) => c.id === 'auth')!.detail).toBe('credentials present'); + expect(report.checks.find((c) => c.id === 'auth')!.detail).toBe( + 'credentials present — stored credentials', + ); }); }); diff --git a/src/commands/_helpers.ts b/src/commands/_helpers.ts index e2bf597b..b733d090 100644 --- a/src/commands/_helpers.ts +++ b/src/commands/_helpers.ts @@ -2,6 +2,7 @@ import { apiClient, setWorkspaceContext } from '../api/client.js'; import { AuthError, CliError, NetworkError, ValidationError, exitCodeFor } from '../output/error.js'; import { readWorkspaceConfig, writeWorkspaceConfig } from './workspace.js'; import { isLikelyUuid, isValidPublicId } from '../lib/publicId.js'; +import { WORKSPACE_ENV_VAR } from '../config/env-vars.js'; import { emit, shouldEmitCommandInvoked } from '../observability/posthog.js'; import type { CliExitCode } from '../analytics/events.js'; import { getCliVersion } from '../observability/posthog.js'; @@ -89,6 +90,22 @@ export async function getDefaultWorkspaceId(): Promise { return match.id; } + // AIT-438: a headless caller has no workspace config to read — the same + // environment that carries HOOKMYAPP_API_KEY carries the workspace it is + // scoped to. Mirrors HOOKMYAPP_CHANNEL_ID (see resolveChannelRefOrDefault) + // and sits above the stored config for the same reason the API key does: + // an explicitly exported value beats a persisted default. + const envWorkspace = process.env[WORKSPACE_ENV_VAR]?.trim(); + if (envWorkspace) { + if (!isValidPublicId(envWorkspace, 'ws')) { + throw new ValidationError( + `${WORKSPACE_ENV_VAR} must be a workspace publicId (ws_<8-char>), got "${envWorkspace}".`, + ); + } + setWorkspaceContext({ workspaceId: envWorkspace }); + return envWorkspace; + } + const config = readWorkspaceConfig(); if (config.activeWorkspaceId) { setWorkspaceContext({ workspaceId: config.activeWorkspaceId }); @@ -110,7 +127,8 @@ export async function getDefaultWorkspaceId(): Promise { if (Array.isArray(workspaces) && workspaces.length > 1) { throw new ValidationError( - `You're a member of ${workspaces.length} workspaces. Pick one first:\n hookmyapp workspace use `, + `You're a member of ${workspaces.length} workspaces. Pick one first:\n hookmyapp workspace use \n` + + ' (non-interactive: pass --workspace or set HOOKMYAPP_WORKSPACE_ID)', ); } diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 0cd848fd..01a1f2db 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -1,6 +1,7 @@ import type { Command } from 'commander'; import { runTool } from '../lib/spawn-tool.js'; import { readCredentials } from '../auth/store.js'; +import { API_KEY_ENV_VAR } from '../config/env-vars.js'; import { apiClient } from '../api/client.js'; import { AuthError, ForbiddenError, PermissionError } from '../output/error.js'; import { isJsonMode } from '../output/format.js'; @@ -56,7 +57,17 @@ export async function collectDoctorReport( let loggedIn = false; let creds: Awaited> = null; - try { creds = await readCredentials(); loggedIn = creds !== null; } catch { loggedIn = false; } + // A malformed HOOKMYAPP_API_KEY throws here (AIT-438). Keep its message — + // "not logged in" would send the user to `login` when the real fix is to + // correct the variable. + let credsError: string | null = null; + try { + creds = await readCredentials(); + loggedIn = creds !== null; + } catch (err) { + loggedIn = false; + credsError = err instanceof Error ? err.message : String(err); + } // Credentials-present alone is a false positive when the stored token is // expired or belongs to another env (2026-07-07 audit: doctor said OK, the // next command 401'd). When network checks are on, prove the token works @@ -64,7 +75,10 @@ export async function collectDoctorReport( // command uses, including the token-refresh attempt. A raw fetch with the // stored accessToken false-FAILs on tokens that are merely expired but // refreshable (2026-07-08 audit). - let authDetail = loggedIn ? 'credentials present' : 'not logged in — run: hookmyapp login'; + const credSource = creds?.source === 'env' ? `${API_KEY_ENV_VAR} (environment)` : 'stored credentials'; + let authDetail = loggedIn + ? `credentials present — ${credSource}` + : credsError ?? 'not logged in — run: hookmyapp login'; // Kept from the auth probe so the workspace check below can validate the // persisted activeWorkspaceId against the backend instead of trusting the // cache (AIT-51: after a DB re-seed doctor said OK while every scoped @@ -74,13 +88,15 @@ export async function collectDoctorReport( try { const res = await apiClient('/workspaces'); if (Array.isArray(res)) workspaces = res; - authDetail = 'credentials valid for this env'; + authDetail = `credentials valid for this env — ${credSource}`; } catch (err) { // ForbiddenError is what coded 403s map to since AIT-151 — a denial is // still a rejected credential, not a network flake. if (err instanceof AuthError || err instanceof PermissionError || err instanceof ForbiddenError) { loggedIn = false; - authDetail = 'credentials present but rejected by this env — run: hookmyapp login'; + authDetail = creds?.source === 'env' + ? `credentials present but rejected by this env — the key in ${API_KEY_ENV_VAR} is invalid or revoked` + : 'credentials present but rejected by this env — run: hookmyapp login'; } // Anything else (network flake, 5xx) — leave the presence-based verdict. } diff --git a/src/config/env-vars.ts b/src/config/env-vars.ts new file mode 100644 index 00000000..6a813667 --- /dev/null +++ b/src/config/env-vars.ts @@ -0,0 +1,9 @@ +/** + * Names of the environment variables the CLI reads for credentials and + * workspace selection. Kept in a leaf module (no imports) so consumers can + * pull the name without dragging in the auth store — several test suites + * mock `auth/store.js` wholesale, and a constant exported from there would + * break every one of them. + */ +export const API_KEY_ENV_VAR = 'HOOKMYAPP_API_KEY'; +export const WORKSPACE_ENV_VAR = 'HOOKMYAPP_WORKSPACE_ID'; diff --git a/src/storage/secrets.ts b/src/storage/secrets.ts index da4ecb7a..c99f3604 100644 --- a/src/storage/secrets.ts +++ b/src/storage/secrets.ts @@ -33,6 +33,9 @@ export interface Secrets { /** Agent credentials only: login email — the hmok_ token is opaque, so this * is the only human identity available for crash attribution (AIT-278). */ email?: string; + /** Where the credential came from. Undefined = read from credentials.json. + * 'env' = synthesized from HOOKMYAPP_API_KEY and never persisted (AIT-438). */ + source?: 'env'; } /** True for an auth.md-issued org-scoped agent credential (no refresh token). */ From 0958ddf980496319bb77dde7ae259fd584beaad2 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 12:41:55 +0300 Subject: [PATCH 03/15] fix(auth): strip cmd.exe quotes from env credential values cmd.exe stores `set VAR="value"` with the quotes included, unlike PowerShell and POSIX shells, so a Windows user following the documented instructions would get "not a valid API key" for a key that plainly starts with hmok_. Same normalization dotenv applies, shared by both HOOKMYAPP_API_KEY and HOOKMYAPP_WORKSPACE_ID. An unbalanced quote is left alone so it still fails as malformed. --- src/auth/__tests__/env-api-key.test.ts | 15 +++++++++++++++ src/auth/store.ts | 4 ++-- src/commands/_helpers.ts | 5 +++-- src/config/env-vars.ts | 17 +++++++++++++++++ 4 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/auth/__tests__/env-api-key.test.ts b/src/auth/__tests__/env-api-key.test.ts index 3f91c68d..20e1147c 100644 --- a/src/auth/__tests__/env-api-key.test.ts +++ b/src/auth/__tests__/env-api-key.test.ts @@ -55,6 +55,21 @@ describe('HOOKMYAPP_API_KEY credential (AIT-438)', () => { expect(readEnvCredential()).toBeNull(); }); + // cmd.exe keeps the quotes in `set HOOKMYAPP_API_KEY="hmok_abc"`, unlike + // PowerShell and POSIX shells — without stripping, Windows users get + // "not a valid API key" for a key that plainly starts with hmok_. + it('accepts a value quoted the way cmd.exe stores it', () => { + process.env[API_KEY_ENV_VAR] = '"hmok_abc123"'; + expect(readEnvCredential()?.accessToken).toBe('hmok_abc123'); + process.env[API_KEY_ENV_VAR] = "'hmok_abc123'"; + expect(readEnvCredential()?.accessToken).toBe('hmok_abc123'); + }); + + it('leaves an unbalanced quote alone, so it still fails as malformed', () => { + process.env[API_KEY_ENV_VAR] = '"hmok_abc123'; + expect(() => readEnvCredential()).toThrow(AuthError); + }); + it('rejects a malformed value, naming the variable', () => { process.env[API_KEY_ENV_VAR] = 'not-a-key'; expect(() => readEnvCredential()).toThrow(AuthError); diff --git a/src/auth/store.ts b/src/auth/store.ts index a59d1e17..7e65df32 100644 --- a/src/auth/store.ts +++ b/src/auth/store.ts @@ -1,7 +1,7 @@ import { readFileSync } from 'node:fs'; import { getConfigFile } from '../storage/path.js'; import { AuthError } from '../output/error.js'; -import { API_KEY_ENV_VAR } from '../config/env-vars.js'; +import { API_KEY_ENV_VAR, stripEnvQuotes } from '../config/env-vars.js'; import { writeSecrets, readSecrets, @@ -36,7 +36,7 @@ export async function saveCredentials(creds: Credentials): Promise { * "Not logged in. Run: hookmyapp login" hides where the bad key came from. */ export function readEnvCredential(): Credentials | null { - const raw = process.env[API_KEY_ENV_VAR]?.trim(); + const raw = stripEnvQuotes(process.env[API_KEY_ENV_VAR]?.trim() ?? ''); if (!raw) return null; if (!raw.startsWith('hmok_') && !raw.startsWith('ac_')) { throw new AuthError( diff --git a/src/commands/_helpers.ts b/src/commands/_helpers.ts index b733d090..9c0957d0 100644 --- a/src/commands/_helpers.ts +++ b/src/commands/_helpers.ts @@ -2,7 +2,7 @@ import { apiClient, setWorkspaceContext } from '../api/client.js'; import { AuthError, CliError, NetworkError, ValidationError, exitCodeFor } from '../output/error.js'; import { readWorkspaceConfig, writeWorkspaceConfig } from './workspace.js'; import { isLikelyUuid, isValidPublicId } from '../lib/publicId.js'; -import { WORKSPACE_ENV_VAR } from '../config/env-vars.js'; +import { WORKSPACE_ENV_VAR, stripEnvQuotes } from '../config/env-vars.js'; import { emit, shouldEmitCommandInvoked } from '../observability/posthog.js'; import type { CliExitCode } from '../analytics/events.js'; import { getCliVersion } from '../observability/posthog.js'; @@ -95,7 +95,8 @@ export async function getDefaultWorkspaceId(): Promise { // scoped to. Mirrors HOOKMYAPP_CHANNEL_ID (see resolveChannelRefOrDefault) // and sits above the stored config for the same reason the API key does: // an explicitly exported value beats a persisted default. - const envWorkspace = process.env[WORKSPACE_ENV_VAR]?.trim(); + // stripQuotes: cmd.exe keeps the quotes in `set VAR="ws_abc12345"`. + const envWorkspace = stripEnvQuotes(process.env[WORKSPACE_ENV_VAR]?.trim() ?? ''); if (envWorkspace) { if (!isValidPublicId(envWorkspace, 'ws')) { throw new ValidationError( diff --git a/src/config/env-vars.ts b/src/config/env-vars.ts index 6a813667..a5ece71c 100644 --- a/src/config/env-vars.ts +++ b/src/config/env-vars.ts @@ -7,3 +7,20 @@ */ export const API_KEY_ENV_VAR = 'HOOKMYAPP_API_KEY'; export const WORKSPACE_ENV_VAR = 'HOOKMYAPP_WORKSPACE_ID'; + +/** + * Drop one layer of matching surrounding quotes from an env-var value. + * + * cmd.exe stores `set VAR="value"` with the quotes included, unlike PowerShell + * and POSIX shells, so a Windows user following the documented instructions + * ends up with a value no prefix/shape check would accept. + */ +export function stripEnvQuotes(value: string): string { + if (value.length >= 2) { + const first = value[0]; + if ((first === '"' || first === "'") && value[value.length - 1] === first) { + return value.slice(1, -1).trim(); + } + } + return value; +} From 1055c7a569b90c1f5e7bed44e3400810dd2cf5cb Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 12:48:23 +0300 Subject: [PATCH 04/15] fix(auth): drop the invented command from the malformed-key error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live-testing the message surfaced two problems. The error told users to run \`hookmyapp credentials create\`, which does not exist — the credentials command only lists and revokes; keys come from the auth.md login flow. It now says what to do without naming a command that isn't there. Also adds the missing wire-level assertion: the env key must reach fetch() as the Authorization bearer with no refresh attempt in front of it. That was the one behavior a fake key could not prove live. --- src/api/__tests__/env-key-bearer.test.ts | 40 ++++++++++++++++++++++++ src/auth/store.ts | 9 +++--- 2 files changed, 45 insertions(+), 4 deletions(-) create mode 100644 src/api/__tests__/env-key-bearer.test.ts diff --git a/src/api/__tests__/env-key-bearer.test.ts b/src/api/__tests__/env-key-bearer.test.ts new file mode 100644 index 00000000..019a146e --- /dev/null +++ b/src/api/__tests__/env-key-bearer.test.ts @@ -0,0 +1,40 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { apiClient } from '../client.js'; +import { API_KEY_ENV_VAR } from '../../config/env-vars.js'; +import * as secrets from '../../storage/secrets.js'; + +// The env key must reach the wire as the bearer token, exactly as a stored +// agent credential does — no refresh attempt, no rewriting (AIT-438). +describe('apiClient with HOOKMYAPP_API_KEY (AIT-438)', () => { + const original = process.env[API_KEY_ENV_VAR]; + + beforeEach(() => { + vi.restoreAllMocks(); + vi.spyOn(secrets, 'readSecrets').mockResolvedValue(null); + }); + afterEach(() => { + if (original === undefined) delete process.env[API_KEY_ENV_VAR]; + else process.env[API_KEY_ENV_VAR] = original; + vi.unstubAllGlobals(); + }); + + it('sends the env key as the Authorization bearer', async () => { + process.env[API_KEY_ENV_VAR] = 'hmok_livekey123'; + const fetchMock = vi.fn(async (_url: string, _init: RequestInit) => ({ + ok: true, + status: 200, + headers: new Headers(), + json: async () => [{ id: 'ws_abc12345' }], + })); + vi.stubGlobal('fetch', fetchMock); + + const res = await apiClient('/workspaces'); + + expect(res).toEqual([{ id: 'ws_abc12345' }]); + const headers = fetchMock.mock.calls[0]![1].headers as Record; + expect(headers.Authorization).toBe('Bearer hmok_livekey123'); + // One call only: an agent credential has no refresh token, so nothing + // should have tried to reach WorkOS first. + expect(fetchMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/auth/store.ts b/src/auth/store.ts index 7e65df32..2671b3ac 100644 --- a/src/auth/store.ts +++ b/src/auth/store.ts @@ -23,9 +23,10 @@ export async function saveCredentials(creds: Credentials): Promise { * Build a credential from HOOKMYAPP_API_KEY, or null when the variable is * unset/empty. * - * Shaped as `kind: 'agent'` on purpose: an org API key is exactly what - * `hookmyapp credentials create` persists, so every existing agent-credential - * path (no refresh, no rescope, bearer sent verbatim) applies unchanged. + * Shaped as `kind: 'agent'` on purpose: an org API key is exactly what the + * auth.md flow (`login --email … --scope …`) persists, so every existing + * agent-credential path (no refresh, no rescope, bearer sent verbatim) + * applies unchanged. * * Accepted prefixes mirror the backend's `isAgentToken` — `hmok_` is the * current mint, `ac_` is legacy and still resolves server-side. Rejecting @@ -41,7 +42,7 @@ export function readEnvCredential(): Credentials | null { if (!raw.startsWith('hmok_') && !raw.startsWith('ac_')) { throw new AuthError( `${API_KEY_ENV_VAR} is not a valid API key (expected it to start with "hmok_"). ` + - `Fix or unset the variable, then retry. Mint a key with: hookmyapp credentials create`, + `Fix or unset the variable, then retry.`, ); } return { From b39b2511a8299f6ae3f611b3883206ea977837e3 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 12:53:30 +0300 Subject: [PATCH 05/15] fix(api): name the variable when an env API key is rejected A revoked key 401s, and the generic AuthError told the user "Session expired. Run: hookmyapp login". Wrong twice over for an env credential: there is no session to expire, and login now refuses to run while the variable is set, so the guidance is a loop. Found by revoking a real staging key and watching what the CLI said. --- src/api/__tests__/env-key-bearer.test.ts | 18 ++++++++++++++++++ src/api/client.ts | 12 +++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/api/__tests__/env-key-bearer.test.ts b/src/api/__tests__/env-key-bearer.test.ts index 019a146e..96f21f12 100644 --- a/src/api/__tests__/env-key-bearer.test.ts +++ b/src/api/__tests__/env-key-bearer.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { apiClient } from '../client.js'; import { API_KEY_ENV_VAR } from '../../config/env-vars.js'; import * as secrets from '../../storage/secrets.js'; +import { AuthError } from '../../output/error.js'; // The env key must reach the wire as the bearer token, exactly as a stored // agent credential does — no refresh attempt, no rewriting (AIT-438). @@ -18,6 +19,23 @@ describe('apiClient with HOOKMYAPP_API_KEY (AIT-438)', () => { vi.unstubAllGlobals(); }); + // A revoked key 401s. "Session expired. Run: login" is wrong twice over — + // there is no session, and login refuses while the variable is set. + it('names the variable when the key is rejected, not "session expired"', async () => { + process.env[API_KEY_ENV_VAR] = 'hmok_revoked'; + vi.stubGlobal('fetch', vi.fn(async () => ({ + ok: false, + status: 401, + headers: new Headers(), + json: async () => ({ message: 'Unauthorized' }), + }))); + + await expect(apiClient('/workspaces')).rejects.toThrow(AuthError); + await expect(apiClient('/workspaces')).rejects.toThrow( + /HOOKMYAPP_API_KEY was rejected/, + ); + }); + it('sends the env key as the Authorization bearer', async () => { process.env[API_KEY_ENV_VAR] = 'hmok_livekey123'; const fetchMock = vi.fn(async (_url: string, _init: RequestInit) => ({ diff --git a/src/api/client.ts b/src/api/client.ts index 7820eba6..4c3886f4 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -20,6 +20,7 @@ import { getEffectiveWorkosClientId, } from '../config/env-profiles.js'; import { buildVersionHeaders } from './version-headers.js'; +import { API_KEY_ENV_VAR } from '../config/env-vars.js'; // Module-level workspace context populated by the top-level CLI entry after // parsing --workspace. Explicit options.workspaceId on a specific apiClient() @@ -383,7 +384,16 @@ export async function apiClient( } if (!res.ok) { - throw await mapApiError(res); + const err = await mapApiError(res); + // A 401 on an env credential must not say "Session expired. Run: login": + // there is no session, and `login` refuses to run while the variable is + // set, so that guidance is a loop (AIT-438). + if (err instanceof AuthError && creds.source === 'env') { + throw new AuthError( + `The API key in ${API_KEY_ENV_VAR} was rejected (invalid or revoked). Replace it or unset the variable.`, + ); + } + throw err; } // 204 No Content (and other empty-body 2xx responses) have no JSON to parse. From 46fec881cdbb293f03b2b0325ad7fe8ffc9fc6af Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 12:56:04 +0300 Subject: [PATCH 06/15] =?UTF-8?q?fix:=20address=20Codex=20review=20?= =?UTF-8?q?=E2=80=94=20doctor=20workspace=20override,=20logout=20JSON?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real gaps from the automated review: doctor derived the active workspace from readWorkspaceConfig() alone, so with HOOKMYAPP_WORKSPACE_ID set it reported "(none)" for an env-only agent, or flagged the ignored persisted workspace as stale. It now reports the same winner getDefaultWorkspaceId() picks, fails the check on a malformed value, and points at the variable rather than `workspace use` when the env workspace is unknown to the backend. logout --json reported status "logged_out" while an env key kept authenticating — humans saw the warning, automation saw nothing. The payload now carries envKeyActive (+ envKeyVar) and downgrades the status to logged_out_with_warning. The third finding (an error pointing at `credentials create`, which does not exist) was already fixed in 052cde7. --- src/auth/__tests__/logout.test.ts | 24 +++++++++++ src/auth/logout.ts | 13 +++++- src/commands/__tests__/doctor.test.ts | 59 ++++++++++++++++++++++++++- src/commands/doctor.ts | 44 ++++++++++++++------ 4 files changed, 125 insertions(+), 15 deletions(-) diff --git a/src/auth/__tests__/logout.test.ts b/src/auth/__tests__/logout.test.ts index 9320dc4d..195dabfd 100644 --- a/src/auth/__tests__/logout.test.ts +++ b/src/auth/__tests__/logout.test.ts @@ -69,12 +69,36 @@ describe('logout', () => { expect(JSON.parse(written.trim())).toEqual({ status: 'logged_out', revoked: false, + envKeyActive: false, mcpCleanup: { ok: true }, }); // The human check line must NOT be printed in --json mode. expect(logSpy.mock.calls.flat().join('')).not.toMatch(/Logged out/); }); + // AIT-438: an env key keeps authenticating after logout. Automation reads + // the payload, not the stderr warning, so the signal has to be in the JSON. + test('--json flags a still-active HOOKMYAPP_API_KEY', async () => { + const original = process.env.HOOKMYAPP_API_KEY; + process.env.HOOKMYAPP_API_KEY = 'hmok_stillhere'; + const credsPath = join(DIR, 'credentials.json'); + writeFileSync(credsPath, JSON.stringify({ accessToken: 'a', refreshToken: 'r', expiresAt: 1 })); + const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); + + try { + await runLogout(['--json']); + const written = stdoutSpy.mock.calls.map((c) => String(c[0])).join(''); + expect(JSON.parse(written.trim())).toMatchObject({ + status: 'logged_out_with_warning', + envKeyActive: true, + envKeyVar: 'HOOKMYAPP_API_KEY', + }); + } finally { + if (original === undefined) delete process.env.HOOKMYAPP_API_KEY; + else process.env.HOOKMYAPP_API_KEY = original; + } + }); + test('reports MCP cleanup failure after credentials are removed', async () => { removeClaudeMcpMock.mockReturnValue({ ok: false, detail: 'Claude MCP cleanup timed out' }); const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); diff --git a/src/auth/logout.ts b/src/auth/logout.ts index 976618ee..d48961dd 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -11,6 +11,10 @@ export function logoutCommand(program: Command): void { .description('Remove stored credentials') .action(async () => { const json = !!program.opts().json; + // AIT-438: an env key keeps authenticating after logout. Humans get the + // warning below; --json callers need the same signal in the payload, or + // automation reads status "logged_out" and assumes it is signed out. + const envKeyActive = Boolean(process.env[API_KEY_ENV_VAR]?.trim()); // AIT-153: for an agent credential (org API key), also revoke it // server-side so it can't keep being used after logout. Best-effort — an @@ -41,8 +45,13 @@ export function logoutCommand(program: Command): void { if (json) { process.stdout.write( JSON.stringify({ - status: mcpCleanup.ok ? 'logged_out' : 'logged_out_with_warning', + status: + !mcpCleanup.ok || envKeyActive + ? 'logged_out_with_warning' + : 'logged_out', revoked, + envKeyActive, + ...(envKeyActive ? { envKeyVar: API_KEY_ENV_VAR } : {}), mcpCleanup, }) + '\n', ); @@ -52,7 +61,7 @@ export function logoutCommand(program: Command): void { ? '\n✓ Logged out\n' : `\n✓ Logged out\n⚠ ${mcpCleanup.detail}\n`, ); - if (process.env[API_KEY_ENV_VAR]?.trim()) { + if (envKeyActive) { console.log( `⚠ ${API_KEY_ENV_VAR} is still set — commands stay authenticated with it. Unset it to sign out fully.\n`, ); diff --git a/src/commands/__tests__/doctor.test.ts b/src/commands/__tests__/doctor.test.ts index 72d4c36b..93d3c6ba 100644 --- a/src/commands/__tests__/doctor.test.ts +++ b/src/commands/__tests__/doctor.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; vi.mock('../../auth/store.js', () => ({ readCredentials: vi.fn(async () => null) })); vi.mock('../../api/client.js', () => ({ apiClient: vi.fn() })); vi.mock('../workspace.js', () => ({ readWorkspaceConfig: vi.fn(() => ({})) })); @@ -104,6 +104,63 @@ describe('doctor — auth probe uses the real authenticated request path', () => }); }); +describe('doctor — HOOKMYAPP_WORKSPACE_ID override (AIT-438)', () => { + const original = process.env.HOOKMYAPP_WORKSPACE_ID; + beforeEach(() => { + vi.clearAllMocks(); + vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, status: 200 }))); + vi.mocked(readCredentials).mockResolvedValue({ + accessToken: 'hmok_x', + refreshToken: '', + expiresAt: 0, + kind: 'agent', + source: 'env', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + vi.mocked(readWorkspaceConfig).mockReturnValue({}); + }); + afterEach(() => { + if (original === undefined) delete process.env.HOOKMYAPP_WORKSPACE_ID; + else process.env.HOOKMYAPP_WORKSPACE_ID = original; + }); + + it('reports the env workspace instead of "(none)"', async () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_abc12345'; + vi.mocked(apiClient).mockResolvedValue([{ id: 'ws_abc12345' }]); + + const report = await collectDoctorReport({ checkTools: false }); + const ws = report.checks.find((c) => c.id === 'workspace')!; + + expect(ws.ok).toBe(true); + expect(ws.detail).toBe('ws_abc12345 — HOOKMYAPP_WORKSPACE_ID (environment)'); + }); + + it('fails the check on a malformed value, the same way commands do', async () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'not-a-ws-id'; + vi.mocked(apiClient).mockResolvedValue([]); + + const ws = (await collectDoctorReport({ checkTools: false })).checks.find( + (c) => c.id === 'workspace', + )!; + + expect(ws.ok).toBe(false); + expect(ws.detail).toContain('HOOKMYAPP_WORKSPACE_ID'); + }); + + it('points at the variable, not `workspace use`, when the env workspace is unknown', async () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_gone1234'; + vi.mocked(apiClient).mockResolvedValue([{ id: 'ws_other123' }]); + + const ws = (await collectDoctorReport({ checkTools: false })).checks.find( + (c) => c.id === 'workspace', + )!; + + expect(ws.ok).toBe(false); + expect(ws.detail).toContain('unset HOOKMYAPP_WORKSPACE_ID'); + expect(ws.detail).not.toContain('workspace use'); + }); +}); + describe('doctor — active workspace is validated against the backend (AIT-51)', () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 01a1f2db..33ab3f5a 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -1,7 +1,8 @@ import type { Command } from 'commander'; import { runTool } from '../lib/spawn-tool.js'; import { readCredentials } from '../auth/store.js'; -import { API_KEY_ENV_VAR } from '../config/env-vars.js'; +import { API_KEY_ENV_VAR, WORKSPACE_ENV_VAR, stripEnvQuotes } from '../config/env-vars.js'; +import { isValidPublicId } from '../lib/publicId.js'; import { apiClient } from '../api/client.js'; import { AuthError, ForbiddenError, PermissionError } from '../output/error.js'; import { isJsonMode } from '../output/format.js'; @@ -106,19 +107,38 @@ export async function collectDoctorReport( let wsId: string | undefined; let wsSlug: string | undefined; - try { - const cfg = readWorkspaceConfig(); - wsId = cfg.activeWorkspaceId ?? undefined; - wsSlug = cfg.activeWorkspaceSlug ?? undefined; - } catch { /* ignore */ } + // AIT-438: HOOKMYAPP_WORKSPACE_ID outranks the persisted config in + // getDefaultWorkspaceId(), so doctor has to report the same winner — reading + // only the config would say "(none)" for an env-only agent, or flag the + // ignored persisted workspace as stale. + const envWs = stripEnvQuotes(process.env[WORKSPACE_ENV_VAR]?.trim() ?? ''); + let wsFromEnv = false; + if (envWs) { + wsFromEnv = true; + wsId = envWs; + } else { + try { + const cfg = readWorkspaceConfig(); + wsId = cfg.activeWorkspaceId ?? undefined; + wsSlug = cfg.activeWorkspaceSlug ?? undefined; + } catch { /* ignore */ } + } let wsOk = true; - let wsDetail = wsSlug ?? '(none — auto-resolves on first call)'; - // Only a definitive backend list can fail this check — on a network flake - // (workspaces === null) the cache-based verdict stands, matching the auth - // probe's fail-open posture above. - if (wsId && workspaces && !workspaces.some((w) => w?.id === wsId)) { + let wsDetail = wsFromEnv + ? `${envWs} — ${WORKSPACE_ENV_VAR} (environment)` + : wsSlug ?? '(none — auto-resolves on first call)'; + if (wsFromEnv && !isValidPublicId(envWs, 'ws')) { + // Commands throw a ValidationError on this; doctor must not call it OK. + wsOk = false; + wsDetail = `${WORKSPACE_ENV_VAR} must be a workspace publicId (ws_<8-char>), got "${envWs}"`; + } else if (wsId && workspaces && !workspaces.some((w) => w?.id === wsId)) { + // Only a definitive backend list can fail this check — on a network flake + // (workspaces === null) the cache-based verdict stands, matching the auth + // probe's fail-open posture above. wsOk = false; - wsDetail = `"${wsSlug ?? wsId}" not found on this env (stale selection) — run: hookmyapp workspace use `; + wsDetail = wsFromEnv + ? `"${envWs}" not found on this env — fix or unset ${WORKSPACE_ENV_VAR}` + : `"${wsSlug ?? wsId}" not found on this env (stale selection) — run: hookmyapp workspace use `; } checks.push({ id: 'workspace', label: 'Active workspace', ok: wsOk, hard: false, detail: wsDetail }); const envChannel = process.env.HOOKMYAPP_CHANNEL_ID; From 5c99270c672183711752cd2eb38745bb003b6910 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 13:06:50 +0300 Subject: [PATCH 07/15] =?UTF-8?q?fix:=20address=20second=20Codex=20review?= =?UTF-8?q?=20=E2=80=94=20two=20P1s=20and=20a=20cache=20collision?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit workspace use / customers use persisted a selection and reported success while HOOKMYAPP_WORKSPACE_ID kept overriding it, so the next mutating command ran against the workspace the user thought they had left. Both now refuse while the override is set, same contract as login under HOOKMYAPP_API_KEY. logout's self-revoke goes through apiClient, which authenticates with the env key while it is set. With the same key in both places that revoked the environment credential every other process was sharing; with different keys the backend rejects it as a non-self revoke. It now skips the call and prints the credential id plus the command to run after unsetting the variable. credentialFingerprint returned "unknown" for an opaque env key, so every key against one API origin shared a notification cache — one principal's unread state and 24h throttle shown for another. Env credentials now get a hashed, non-secret fingerprint. --- src/auth/__tests__/env-api-key.test.ts | 14 ++++++++ src/auth/__tests__/logout.test.ts | 32 +++++++++++++++++++ src/auth/logout.ts | 13 +++++++- .../__tests__/workspace-env-override.test.ts | 29 +++++++++++++++++ src/commands/workspace.ts | 12 +++++++ src/notifications-nudge.ts | 7 ++++ 6 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 src/commands/__tests__/workspace-env-override.test.ts diff --git a/src/auth/__tests__/env-api-key.test.ts b/src/auth/__tests__/env-api-key.test.ts index 20e1147c..7fc78005 100644 --- a/src/auth/__tests__/env-api-key.test.ts +++ b/src/auth/__tests__/env-api-key.test.ts @@ -70,6 +70,20 @@ describe('HOOKMYAPP_API_KEY credential (AIT-438)', () => { expect(() => readEnvCredential()).toThrow(AuthError); }); + it('gives each env key its own notification-cache fingerprint', async () => { + const { credentialFingerprint } = await import('../../notifications-nudge.js'); + const a = credentialFingerprint({ + accessToken: 'hmok_keyA', refreshToken: '', expiresAt: 0, kind: 'agent', source: 'env', + }); + const b = credentialFingerprint({ + accessToken: 'hmok_keyB', refreshToken: '', expiresAt: 0, kind: 'agent', source: 'env', + }); + expect(a).not.toBe(b); + expect(a).not.toBe('unknown'); + // Derived, never the secret itself. + expect(a).not.toContain('hmok_keyA'); + }); + it('rejects a malformed value, naming the variable', () => { process.env[API_KEY_ENV_VAR] = 'not-a-key'; expect(() => readEnvCredential()).toThrow(AuthError); diff --git a/src/auth/__tests__/logout.test.ts b/src/auth/__tests__/logout.test.ts index 195dabfd..e8d1edee 100644 --- a/src/auth/__tests__/logout.test.ts +++ b/src/auth/__tests__/logout.test.ts @@ -99,6 +99,38 @@ describe('logout', () => { } }); + // AIT-438: the revoke goes through apiClient, which authenticates with the + // env key while it is set — a "self-revoke" would kill the credential every + // other process is sharing. + test('skips the server-side revoke while HOOKMYAPP_API_KEY is set', async () => { + const original = process.env.HOOKMYAPP_API_KEY; + process.env.HOOKMYAPP_API_KEY = 'hmok_stillhere'; + writeFileSync( + join(DIR, 'credentials.json'), + JSON.stringify({ + accessToken: 'hmok_stillhere', + refreshToken: '', + expiresAt: 0, + kind: 'agent', + credentialPublicId: 'ac_self0001', + }), + ); + const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); + + try { + await runLogout(['--json']); + const payload = JSON.parse( + stdoutSpy.mock.calls.map((c) => String(c[0])).join('').trim(), + ); + expect(payload.revoked).toBe(false); + expect(payload.envKeyActive).toBe(true); + expect(existsSync(join(DIR, 'credentials.json'))).toBe(false); + } finally { + if (original === undefined) delete process.env.HOOKMYAPP_API_KEY; + else process.env.HOOKMYAPP_API_KEY = original; + } + }); + test('reports MCP cleanup failure after credentials are removed', async () => { removeClaudeMcpMock.mockReturnValue({ ok: false, detail: 'Claude MCP cleanup timed out' }); const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); diff --git a/src/auth/logout.ts b/src/auth/logout.ts index d48961dd..f8e39c10 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -27,7 +27,18 @@ export function logoutCommand(program: Command): void { // environment is not ours to clear, and revoking it server-side would // break every other process sharing that key. const creds = await readSecrets(); - if (creds && isAgentCredential(creds) && creds.credentialPublicId) { + // The revoke call goes through apiClient, which authenticates with the + // env key while it is set. If both hold the same key, the "self-revoke" + // would kill the environment credential every other process is using; + // if they differ, the backend rejects it as a non-self revoke anyway. + // Skip it and say so — the local credential is still cleared. + if (envKeyActive && creds && isAgentCredential(creds)) { + console.error( + `\n⚠ Stored key ${creds.credentialPublicId ?? ''} was not revoked server-side: ` + + `while ${API_KEY_ENV_VAR} is set, the request would authenticate as that key. ` + + `Unset it and run: hookmyapp credentials revoke ${creds.credentialPublicId ?? ''}\n`, + ); + } else if (creds && isAgentCredential(creds) && creds.credentialPublicId) { try { const { apiClient } = await import('../api/client.js'); await apiClient(`/agent/credentials/${creds.credentialPublicId}`, { diff --git a/src/commands/__tests__/workspace-env-override.test.ts b/src/commands/__tests__/workspace-env-override.test.ts new file mode 100644 index 00000000..86eef102 --- /dev/null +++ b/src/commands/__tests__/workspace-env-override.test.ts @@ -0,0 +1,29 @@ +import { describe, it, expect, afterEach } from 'vitest'; +import { switchActiveWorkspace } from '../workspace.js'; +import { ValidationError } from '../../output/error.js'; + +// AIT-438: HOOKMYAPP_WORKSPACE_ID outranks the persisted selection, so a +// `workspace use` that "succeeds" would send the next command at the old +// workspace. Refuse instead of lying. +describe('workspace use under HOOKMYAPP_WORKSPACE_ID', () => { + const original = process.env.HOOKMYAPP_WORKSPACE_ID; + afterEach(() => { + if (original === undefined) delete process.env.HOOKMYAPP_WORKSPACE_ID; + else process.env.HOOKMYAPP_WORKSPACE_ID = original; + }); + + it('refuses the switch while the override is set', async () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_abc12345'; + await expect(switchActiveWorkspace('some-other-ws')).rejects.toThrow(ValidationError); + await expect(switchActiveWorkspace('some-other-ws')).rejects.toThrow( + /HOOKMYAPP_WORKSPACE_ID is set to ws_abc12345/, + ); + }); + + it('refuses `customers use` the same way', async () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_abc12345'; + await expect( + switchActiveWorkspace('some-customer', { kind: 'customer' }), + ).rejects.toThrow(ValidationError); + }); +}); diff --git a/src/commands/workspace.ts b/src/commands/workspace.ts index 7e101ccd..82eded05 100644 --- a/src/commands/workspace.ts +++ b/src/commands/workspace.ts @@ -8,6 +8,7 @@ import { isLikelyUuid, isValidPublicId } from '../lib/publicId.js'; import fs from 'node:fs'; import { getConfigFile, safeWriteFileSync } from '../storage/path.js'; import { resolveEnv } from '../config/env-profiles.js'; +import { WORKSPACE_ENV_VAR, stripEnvQuotes } from '../config/env-vars.js'; export interface WorkspaceConfig { activeWorkspaceId?: string; @@ -132,6 +133,17 @@ export async function switchActiveWorkspace( opts: { kind?: 'team' | 'customer' } = {}, ): Promise { const noun = opts.kind === 'customer' ? 'customer' : 'workspace'; + // AIT-438: HOOKMYAPP_WORKSPACE_ID outranks the persisted selection, so + // writing one here would report a switch that never takes effect and send + // the next mutating command at the old workspace. Same contract as `login` + // under HOOKMYAPP_API_KEY: refuse, and say what to unset. + const envWs = stripEnvQuotes(process.env[WORKSPACE_ENV_VAR]?.trim() ?? ''); + if (envWs) { + throw new ValidationError( + `${WORKSPACE_ENV_VAR} is set to ${envWs} and takes precedence, so this switch would have no effect. ` + + `Unset ${WORKSPACE_ENV_VAR} first, or change its value.`, + ); + } let workspace: Workspace; if (nameOrId) { workspace = (await resolveWorkspace(nameOrId, opts.kind)) as unknown as Workspace; diff --git a/src/notifications-nudge.ts b/src/notifications-nudge.ts index 3b0a5b72..af1b914b 100644 --- a/src/notifications-nudge.ts +++ b/src/notifications-nudge.ts @@ -45,6 +45,13 @@ export interface NotificationsCache { /** Non-secret identity for cache namespacing. Never a raw token. */ export function credentialFingerprint(creds: Secrets): string { if (creds.credentialPublicId) return creds.credentialPublicId; + // An opaque env key (AIT-438) carries no publicId, no JWT claims and no + // email, so every such key would collapse to 'unknown' and share one cache + // file — one key's unread state and 24h throttle shown for another. Hash the + // token into a stable, non-secret id instead. Never logged or transmitted. + if (creds.source === 'env') { + return `env:${createHash('sha256').update(creds.accessToken).digest('hex').slice(0, 16)}`; + } try { // WorkOS sessions: `sub` (stable user id) + `org_id` (session org scope) // — both non-secret. org_id matters: the same user re-scoped to another From 6efdbb643942113b95dab7b84edf3dcac2c62f81 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 13:12:44 +0300 Subject: [PATCH 08/15] fix: address third Codex pass and CodeRabbit review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit workspace new persisted the new workspace and said "switched to it" while HOOKMYAPP_WORKSPACE_ID kept outranking it. It now creates the workspace, skips the ineffective switch, and says which variable is holding the active selection (Codex P1). customers list and customers current read the persisted id directly, so they starred and described a workspace no command was using. Both now go through effectiveActiveWorkspaceId() (Codex P2). Every read of HOOKMYAPP_API_KEY / HOOKMYAPP_WORKSPACE_ID now goes through one normalizing helper. login and logout tested the raw value, so a cmd.exe `set VAR=""` — two quote characters, which readEnvCredential treats as unset — made login refuse and logout warn while no credential existed (CodeRabbit). doctor called every rejection "invalid or revoked". A 403 is an authenticated principal without permission; that message sent users to replace a working key. Only a 401 says revoked now (CodeRabbit). Test fixture no longer uses a Stripe-shaped string that trips secret scanners (CodeRabbit). --- src/auth/__tests__/env-api-key.test.ts | 12 ++++++- src/auth/login.ts | 4 +-- src/auth/logout.ts | 4 +-- src/auth/store.ts | 4 +-- .../__tests__/workspace-env-override.test.ts | 22 ++++++++++++- src/commands/_helpers.ts | 5 ++- src/commands/customers.ts | 11 ++++--- src/commands/doctor.ts | 16 +++++++--- src/commands/workspace.ts | 32 +++++++++++++++++-- src/config/env-vars.ts | 15 +++++++++ 10 files changed, 104 insertions(+), 21 deletions(-) diff --git a/src/auth/__tests__/env-api-key.test.ts b/src/auth/__tests__/env-api-key.test.ts index 7fc78005..afe559e9 100644 --- a/src/auth/__tests__/env-api-key.test.ts +++ b/src/auth/__tests__/env-api-key.test.ts @@ -55,6 +55,16 @@ describe('HOOKMYAPP_API_KEY credential (AIT-438)', () => { expect(readEnvCredential()).toBeNull(); }); + // `set HOOKMYAPP_API_KEY=""` on cmd.exe stores the two quote characters. + // Every caller must agree that is "unset", or login refuses while no + // credential exists. + it.each(['""', "''", '" "'])('treats %s as unset everywhere', async (value) => { + process.env[API_KEY_ENV_VAR] = value; + expect(readEnvCredential()).toBeNull(); + const { envApiKey } = await import('../../config/env-vars.js'); + expect(envApiKey()).toBe(''); + }); + // cmd.exe keeps the quotes in `set HOOKMYAPP_API_KEY="hmok_abc"`, unlike // PowerShell and POSIX shells — without stripping, Windows users get // "not a valid API key" for a key that plainly starts with hmok_. @@ -91,7 +101,7 @@ describe('HOOKMYAPP_API_KEY credential (AIT-438)', () => { }); it('never echoes the key in the malformed-value error', () => { - process.env[API_KEY_ENV_VAR] = 'sk_live_supersecret'; + process.env[API_KEY_ENV_VAR] = 'not-a-key-supersecret'; try { readEnvCredential(); throw new Error('expected a throw'); diff --git a/src/auth/login.ts b/src/auth/login.ts index ccd4b002..46a9f9b1 100644 --- a/src/auth/login.ts +++ b/src/auth/login.ts @@ -1,6 +1,6 @@ import { Command } from 'commander'; import { saveCredentials, peekIdentity } from './store.js'; -import { API_KEY_ENV_VAR } from '../config/env-vars.js'; +import { API_KEY_ENV_VAR, envApiKey } from '../config/env-vars.js'; import { AuthError, NetworkError, ValidationError } from '../output/error.js'; import { addExamples } from '../output/help.js'; import { c, icon } from '../output/color.js'; @@ -697,7 +697,7 @@ export function loginCommand(program: Command): void { // login completed now would be authenticated over and have no effect. // Refuse with the fix rather than let the user sign in for nothing // (same contract as `gh auth login` under GH_TOKEN). - if (process.env[API_KEY_ENV_VAR]?.trim()) { + if (envApiKey()) { throw new ValidationError( `${API_KEY_ENV_VAR} is set, and it takes precedence over a stored login — signing in now would have no effect. ` + `Unset ${API_KEY_ENV_VAR} first, then run: ${cliCommandPrefix()} login`, diff --git a/src/auth/logout.ts b/src/auth/logout.ts index f8e39c10..b80749e5 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -1,6 +1,6 @@ import { Command } from 'commander'; import { deleteCredentials } from './store.js'; -import { API_KEY_ENV_VAR } from '../config/env-vars.js'; +import { API_KEY_ENV_VAR, envApiKey } from '../config/env-vars.js'; import { isAgentCredential, readSecrets } from '../storage/secrets.js'; import { addExamples } from '../output/help.js'; import { removeClaudeMcp } from '../commands/mcp.js'; @@ -14,7 +14,7 @@ export function logoutCommand(program: Command): void { // AIT-438: an env key keeps authenticating after logout. Humans get the // warning below; --json callers need the same signal in the payload, or // automation reads status "logged_out" and assumes it is signed out. - const envKeyActive = Boolean(process.env[API_KEY_ENV_VAR]?.trim()); + const envKeyActive = Boolean(envApiKey()); // AIT-153: for an agent credential (org API key), also revoke it // server-side so it can't keep being used after logout. Best-effort — an diff --git a/src/auth/store.ts b/src/auth/store.ts index 2671b3ac..ccef1a53 100644 --- a/src/auth/store.ts +++ b/src/auth/store.ts @@ -1,7 +1,7 @@ import { readFileSync } from 'node:fs'; import { getConfigFile } from '../storage/path.js'; import { AuthError } from '../output/error.js'; -import { API_KEY_ENV_VAR, stripEnvQuotes } from '../config/env-vars.js'; +import { API_KEY_ENV_VAR, envApiKey } from '../config/env-vars.js'; import { writeSecrets, readSecrets, @@ -37,7 +37,7 @@ export async function saveCredentials(creds: Credentials): Promise { * "Not logged in. Run: hookmyapp login" hides where the bad key came from. */ export function readEnvCredential(): Credentials | null { - const raw = stripEnvQuotes(process.env[API_KEY_ENV_VAR]?.trim() ?? ''); + const raw = envApiKey(); if (!raw) return null; if (!raw.startsWith('hmok_') && !raw.startsWith('ac_')) { throw new AuthError( diff --git a/src/commands/__tests__/workspace-env-override.test.ts b/src/commands/__tests__/workspace-env-override.test.ts index 86eef102..2f1c9e65 100644 --- a/src/commands/__tests__/workspace-env-override.test.ts +++ b/src/commands/__tests__/workspace-env-override.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, afterEach } from 'vitest'; -import { switchActiveWorkspace } from '../workspace.js'; +import { switchActiveWorkspace, effectiveActiveWorkspaceId } from '../workspace.js'; import { ValidationError } from '../../output/error.js'; // AIT-438: HOOKMYAPP_WORKSPACE_ID outranks the persisted selection, so a @@ -27,3 +27,23 @@ describe('workspace use under HOOKMYAPP_WORKSPACE_ID', () => { ).rejects.toThrow(ValidationError); }); }); + +// Status surfaces (customers list/current, doctor) must describe the workspace +// commands actually use, not the persisted one the override supersedes. +describe('effectiveActiveWorkspaceId', () => { + const original = process.env.HOOKMYAPP_WORKSPACE_ID; + afterEach(() => { + if (original === undefined) delete process.env.HOOKMYAPP_WORKSPACE_ID; + else process.env.HOOKMYAPP_WORKSPACE_ID = original; + }); + + it('prefers the environment override', () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_env12345'; + expect(effectiveActiveWorkspaceId()).toBe('ws_env12345'); + }); + + it('treats a quoted-empty value as unset', () => { + process.env.HOOKMYAPP_WORKSPACE_ID = '""'; + expect(effectiveActiveWorkspaceId()).not.toBe('""'); + }); +}); diff --git a/src/commands/_helpers.ts b/src/commands/_helpers.ts index 9c0957d0..172463e6 100644 --- a/src/commands/_helpers.ts +++ b/src/commands/_helpers.ts @@ -2,7 +2,7 @@ import { apiClient, setWorkspaceContext } from '../api/client.js'; import { AuthError, CliError, NetworkError, ValidationError, exitCodeFor } from '../output/error.js'; import { readWorkspaceConfig, writeWorkspaceConfig } from './workspace.js'; import { isLikelyUuid, isValidPublicId } from '../lib/publicId.js'; -import { WORKSPACE_ENV_VAR, stripEnvQuotes } from '../config/env-vars.js'; +import { WORKSPACE_ENV_VAR, envWorkspaceId } from '../config/env-vars.js'; import { emit, shouldEmitCommandInvoked } from '../observability/posthog.js'; import type { CliExitCode } from '../analytics/events.js'; import { getCliVersion } from '../observability/posthog.js'; @@ -95,8 +95,7 @@ export async function getDefaultWorkspaceId(): Promise { // scoped to. Mirrors HOOKMYAPP_CHANNEL_ID (see resolveChannelRefOrDefault) // and sits above the stored config for the same reason the API key does: // an explicitly exported value beats a persisted default. - // stripQuotes: cmd.exe keeps the quotes in `set VAR="ws_abc12345"`. - const envWorkspace = stripEnvQuotes(process.env[WORKSPACE_ENV_VAR]?.trim() ?? ''); + const envWorkspace = envWorkspaceId(); if (envWorkspace) { if (!isValidPublicId(envWorkspace, 'ws')) { throw new ValidationError( diff --git a/src/commands/customers.ts b/src/commands/customers.ts index 942133ae..8feadccf 100644 --- a/src/commands/customers.ts +++ b/src/commands/customers.ts @@ -4,7 +4,7 @@ import { output } from '../output/format.js'; import { ValidationError } from '../output/error.js'; import { addExamples } from '../output/help.js'; import { dropWorkosOrgId, type Workspace } from '../types/workspace.js'; -import { readWorkspaceConfig, switchActiveWorkspace } from './workspace.js'; +import { switchActiveWorkspace, effectiveActiveWorkspaceId } from './workspace.js'; import { getDefaultWorkspaceId, resolveOrgPublicIdForWorkspace } from './_helpers.js'; /** A /workspaces union row carries the org it belongs to; the base Workspace type does not. */ @@ -54,9 +54,12 @@ export function registerCustomersCommand(program: Command): void { console.log(JSON.stringify(customers.map(dropWorkosOrgId), null, 2)); return; } - const config = readWorkspaceConfig(); + // Effective id, not the raw config: HOOKMYAPP_WORKSPACE_ID outranks the + // persisted selection, so marking the config row would star a workspace + // no command is using (AIT-438). + const activeId = effectiveActiveWorkspaceId(); const rows = customers.map((w) => ({ - ACTIVE: w.id === config.activeWorkspaceId ? '*' : ' ', + ACTIVE: w.id === activeId ? '*' : ' ', NAME: w.name, ID: w.id, ROLE: w.role, @@ -109,7 +112,7 @@ export function registerCustomersCommand(program: Command): void { .option('--json', 'Output machine-readable JSON') .action(async (opts: { json?: boolean }) => { const all = (await apiClient('/workspaces')) as Workspace[]; - const active = readWorkspaceConfig().activeWorkspaceId; + const active = effectiveActiveWorkspaceId(); const cur = all.find((w) => w.id === active && w.kind === 'customer'); const json = !!(opts.json || program.opts().json); if (!cur) { diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 33ab3f5a..ad0c7d63 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -1,7 +1,7 @@ import type { Command } from 'commander'; import { runTool } from '../lib/spawn-tool.js'; import { readCredentials } from '../auth/store.js'; -import { API_KEY_ENV_VAR, WORKSPACE_ENV_VAR, stripEnvQuotes } from '../config/env-vars.js'; +import { API_KEY_ENV_VAR, WORKSPACE_ENV_VAR, envWorkspaceId } from '../config/env-vars.js'; import { isValidPublicId } from '../lib/publicId.js'; import { apiClient } from '../api/client.js'; import { AuthError, ForbiddenError, PermissionError } from '../output/error.js'; @@ -95,9 +95,17 @@ export async function collectDoctorReport( // still a rejected credential, not a network flake. if (err instanceof AuthError || err instanceof PermissionError || err instanceof ForbiddenError) { loggedIn = false; + // Only a 401 means the credential itself is bad. A 403 is an + // authenticated principal without permission — telling the user their + // key is revoked would send them to replace a perfectly good key. + const denied = !(err instanceof AuthError); authDetail = creds?.source === 'env' - ? `credentials present but rejected by this env — the key in ${API_KEY_ENV_VAR} is invalid or revoked` - : 'credentials present but rejected by this env — run: hookmyapp login'; + ? denied + ? `credentials present but denied on this env — the key in ${API_KEY_ENV_VAR} lacks permission for this workspace` + : `credentials present but rejected by this env — the key in ${API_KEY_ENV_VAR} is invalid or revoked` + : denied + ? 'credentials present but denied on this env — this account lacks permission for this workspace' + : 'credentials present but rejected by this env — run: hookmyapp login'; } // Anything else (network flake, 5xx) — leave the presence-based verdict. } @@ -111,7 +119,7 @@ export async function collectDoctorReport( // getDefaultWorkspaceId(), so doctor has to report the same winner — reading // only the config would say "(none)" for an env-only agent, or flag the // ignored persisted workspace as stale. - const envWs = stripEnvQuotes(process.env[WORKSPACE_ENV_VAR]?.trim() ?? ''); + const envWs = envWorkspaceId(); let wsFromEnv = false; if (envWs) { wsFromEnv = true; diff --git a/src/commands/workspace.ts b/src/commands/workspace.ts index 82eded05..0513e3bd 100644 --- a/src/commands/workspace.ts +++ b/src/commands/workspace.ts @@ -8,7 +8,7 @@ import { isLikelyUuid, isValidPublicId } from '../lib/publicId.js'; import fs from 'node:fs'; import { getConfigFile, safeWriteFileSync } from '../storage/path.js'; import { resolveEnv } from '../config/env-profiles.js'; -import { WORKSPACE_ENV_VAR, stripEnvQuotes } from '../config/env-vars.js'; +import { WORKSPACE_ENV_VAR, envWorkspaceId } from '../config/env-vars.js'; export interface WorkspaceConfig { activeWorkspaceId?: string; @@ -58,6 +58,15 @@ export function readWorkspaceConfig(): WorkspaceConfig { }; } +/** + * The workspace commands actually act on: the env override when set, else the + * persisted selection. Status surfaces must use this, not the raw config, or + * they describe a workspace no command is using (AIT-438). + */ +export function effectiveActiveWorkspaceId(): string | undefined { + return envWorkspaceId() || readWorkspaceConfig().activeWorkspaceId || undefined; +} + export function writeWorkspaceConfig(config: WorkspaceConfig): void { // Hard cutover: refuse to persist a UUID-shaped activeWorkspaceId. // Symmetric with the read-side drop in readWorkspaceConfig — invariant is @@ -137,7 +146,7 @@ export async function switchActiveWorkspace( // writing one here would report a switch that never takes effect and send // the next mutating command at the old workspace. Same contract as `login` // under HOOKMYAPP_API_KEY: refuse, and say what to unset. - const envWs = stripEnvQuotes(process.env[WORKSPACE_ENV_VAR]?.trim() ?? ''); + const envWs = envWorkspaceId(); if (envWs) { throw new ValidationError( `${WORKSPACE_ENV_VAR} is set to ${envWs} and takes precedence, so this switch would have no effect. ` + @@ -248,6 +257,25 @@ export function registerWorkspaceCommand(program: Command): void { method: 'POST', body: JSON.stringify({ name }), }); + // AIT-438: HOOKMYAPP_WORKSPACE_ID outranks the persisted selection, so + // the usual create-then-switch would report a switch that never takes + // effect. Create the workspace (that part works), skip the switch, say so. + const envWsNew = envWorkspaceId(); + if (envWsNew) { + if (!program.opts().json) { + console.log( + `Created workspace "${result.name}" (${result.id})\n` + + `Active workspace unchanged: ${WORKSPACE_ENV_VAR} is set to ${envWsNew}. ` + + `Unset it to switch, or point it at ${result.id}.`, + ); + } else { + output( + { id: result.id, name: result.name, switched: false, activeWorkspaceId: envWsNew }, + { human: false }, + ); + } + return; + } // Re-scope the JWT to the newly-created workspace's org (server-side, // AIT-182) BEFORE persisting the switch — a failed rescope must not // leave config pointing at a workspace the token isn't valid for. diff --git a/src/config/env-vars.ts b/src/config/env-vars.ts index a5ece71c..89c42ea7 100644 --- a/src/config/env-vars.ts +++ b/src/config/env-vars.ts @@ -24,3 +24,18 @@ export function stripEnvQuotes(value: string): string { } return value; } + +/** + * The active org API key from the environment, normalized ('' when unset). + * Every caller must go through this: `HOOKMYAPP_API_KEY='""'` strips to empty, + * so a raw `process.env[...]` truthiness test would refuse a login (or claim a + * key is active) when no credential exists at all. + */ +export function envApiKey(): string { + return stripEnvQuotes(process.env[API_KEY_ENV_VAR]?.trim() ?? ''); +} + +/** The workspace override from the environment, normalized ('' when unset). */ +export function envWorkspaceId(): string { + return stripEnvQuotes(process.env[WORKSPACE_ENV_VAR]?.trim() ?? ''); +} From af2826cb5509cdd7b172132a1fd8fbe3875e9d0d Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 13:18:19 +0300 Subject: [PATCH 09/15] =?UTF-8?q?fix:=20address=20fourth=20Codex=20pass=20?= =?UTF-8?q?=E2=80=94=20revoke=20path,=20list=20marker,=20telemetry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit's logout warning was not executable: it told the user to unset HOOKMYAPP_API_KEY and run `credentials revoke`, but logout had already deleted the credential that command needs to authenticate. Rather than print advice that cannot be followed, apiClient gained a `bearerToken` option that pins a request to one credential, and logout uses it to revoke the STORED key even while an env key would otherwise win. The skip now applies only when the env holds that same key, where revoking would break every other process sharing it. workspace list starred the persisted workspace while commands used the environment one — same fix already applied to customers list. PostHog attributed every cli_command_invoked / cli_error_shown to the persisted workspace, so env-driven invocations were tagged with a stale workspace (or none) while the request targeted another. Telemetry now reads the effective workspace. --- src/api/client.ts | 13 +++++++---- src/auth/__tests__/logout.test.ts | 38 +++++++++++++++++++++++++++++++ src/auth/logout.ts | 26 ++++++++++----------- src/commands/workspace.ts | 4 ++-- src/config/index.ts | 7 ++++++ 5 files changed, 68 insertions(+), 20 deletions(-) diff --git a/src/api/client.ts b/src/api/client.ts index 4c3886f4..5ad82724 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -313,9 +313,14 @@ export function describeFetchError(err: unknown): string { export async function apiClient( path: string, - options?: RequestInit & { workspaceId?: string }, + // `bearerToken` pins the request to one credential instead of the resolved + // one. Only logout needs it: it must authenticate as the STORED key to + // revoke it, which the env key would otherwise shadow (AIT-438). + options?: RequestInit & { workspaceId?: string; bearerToken?: string }, ): Promise { - const creds = await readCredentials(); + const creds = options?.bearerToken + ? ({ accessToken: options.bearerToken, refreshToken: '', expiresAt: 0, kind: 'agent' } as const) + : await readCredentials(); if (!creds) { throw new AuthError('Not logged in. Run: hookmyapp login'); } @@ -336,7 +341,7 @@ export async function apiClient( const baseUrl = getEffectiveApiUrl(); - const { workspaceId, ...fetchOptions } = options ?? {}; + const { workspaceId, bearerToken: _bearerToken, ...fetchOptions } = options ?? {}; const headers: Record = { Authorization: `Bearer ${accessToken}`, @@ -388,7 +393,7 @@ export async function apiClient( // A 401 on an env credential must not say "Session expired. Run: login": // there is no session, and `login` refuses to run while the variable is // set, so that guidance is a loop (AIT-438). - if (err instanceof AuthError && creds.source === 'env') { + if (err instanceof AuthError && 'source' in creds && creds.source === 'env') { throw new AuthError( `The API key in ${API_KEY_ENV_VAR} was rejected (invalid or revoked). Replace it or unset the variable.`, ); diff --git a/src/auth/__tests__/logout.test.ts b/src/auth/__tests__/logout.test.ts index e8d1edee..6c8e3d3a 100644 --- a/src/auth/__tests__/logout.test.ts +++ b/src/auth/__tests__/logout.test.ts @@ -124,6 +124,7 @@ describe('logout', () => { ); expect(payload.revoked).toBe(false); expect(payload.envKeyActive).toBe(true); + expect(payload.envKeyIsStoredKey).toBe(true); expect(existsSync(join(DIR, 'credentials.json'))).toBe(false); } finally { if (original === undefined) delete process.env.HOOKMYAPP_API_KEY; @@ -131,6 +132,43 @@ describe('logout', () => { } }); + // A different env key must not stop logout from revoking the stored one: + // the request is pinned to the stored token, so it is a real self-revoke. + test('still revokes a stored key when the env holds a DIFFERENT key', async () => { + const original = process.env.HOOKMYAPP_API_KEY; + process.env.HOOKMYAPP_API_KEY = 'hmok_otherkey'; + writeFileSync( + join(DIR, 'credentials.json'), + JSON.stringify({ + accessToken: 'hmok_storedkey', + refreshToken: '', + expiresAt: 0, + kind: 'agent', + credentialPublicId: 'ac_stored01', + }), + ); + const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); + const fetchMock = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); + vi.stubGlobal('fetch', fetchMock); + + try { + await runLogout(['--json']); + const payload = JSON.parse( + stdoutSpy.mock.calls.map((c) => String(c[0])).join('').trim(), + ); + expect(payload.envKeyIsStoredKey).toBe(false); + const call = fetchMock.mock.calls.find(([url]) => + String(url).includes('/agent/credentials/ac_stored01'), + ); + expect(call).toBeDefined(); + // Pinned to the stored key, NOT the env key that would otherwise win. + expect(call![1].headers.Authorization).toBe('Bearer hmok_storedkey'); + } finally { + if (original === undefined) delete process.env.HOOKMYAPP_API_KEY; + else process.env.HOOKMYAPP_API_KEY = original; + } + }); + test('reports MCP cleanup failure after credentials are removed', async () => { removeClaudeMcpMock.mockReturnValue({ ok: false, detail: 'Claude MCP cleanup timed out' }); const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); diff --git a/src/auth/logout.ts b/src/auth/logout.ts index b80749e5..b453dbe1 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -27,22 +27,18 @@ export function logoutCommand(program: Command): void { // environment is not ours to clear, and revoking it server-side would // break every other process sharing that key. const creds = await readSecrets(); - // The revoke call goes through apiClient, which authenticates with the - // env key while it is set. If both hold the same key, the "self-revoke" - // would kill the environment credential every other process is using; - // if they differ, the backend rejects it as a non-self revoke anyway. - // Skip it and say so — the local credential is still cleared. - if (envKeyActive && creds && isAgentCredential(creds)) { - console.error( - `\n⚠ Stored key ${creds.credentialPublicId ?? ''} was not revoked server-side: ` + - `while ${API_KEY_ENV_VAR} is set, the request would authenticate as that key. ` + - `Unset it and run: hookmyapp credentials revoke ${creds.credentialPublicId ?? ''}\n`, - ); - } else if (creds && isAgentCredential(creds) && creds.credentialPublicId) { + // The revoke goes through apiClient, which prefers the env key. Pin it + // to the stored token so logout revokes the credential it is actually + // clearing. The one case to skip: the env holds that SAME key — revoking + // it would break every other process sharing it, and the user did not + // ask to invalidate their environment (AIT-438). + const envIsSameKey = envKeyActive && creds?.accessToken === envApiKey(); + if (creds && isAgentCredential(creds) && creds.credentialPublicId && !envIsSameKey) { try { const { apiClient } = await import('../api/client.js'); await apiClient(`/agent/credentials/${creds.credentialPublicId}`, { method: 'DELETE', + bearerToken: creds.accessToken, }); revoked = true; } catch { @@ -62,7 +58,7 @@ export function logoutCommand(program: Command): void { : 'logged_out', revoked, envKeyActive, - ...(envKeyActive ? { envKeyVar: API_KEY_ENV_VAR } : {}), + ...(envKeyActive ? { envKeyVar: API_KEY_ENV_VAR, envKeyIsStoredKey: envIsSameKey } : {}), mcpCleanup, }) + '\n', ); @@ -74,7 +70,9 @@ export function logoutCommand(program: Command): void { ); if (envKeyActive) { console.log( - `⚠ ${API_KEY_ENV_VAR} is still set — commands stay authenticated with it. Unset it to sign out fully.\n`, + envIsSameKey + ? `⚠ ${API_KEY_ENV_VAR} holds this same key, so it was not revoked and commands stay authenticated with it. Unset it to sign out fully.\n` + : `⚠ ${API_KEY_ENV_VAR} is still set — commands stay authenticated with it. Unset it to sign out fully.\n`, ); } } diff --git a/src/commands/workspace.ts b/src/commands/workspace.ts index 0513e3bd..3d6d871c 100644 --- a/src/commands/workspace.ts +++ b/src/commands/workspace.ts @@ -230,14 +230,14 @@ export function registerWorkspaceCommand(program: Command): void { // the fail-safe: an unknown kind never renders as a team workspace. const all = (await apiClient('/workspaces')) as Workspace[]; const data = all.filter((w) => w.kind === 'team').map(dropWorkosOrgId); - const config = readWorkspaceConfig(); + const activeId = effectiveActiveWorkspaceId(); if (opts.json) { console.log(JSON.stringify(data, null, 2)); return; } if (!program.opts().json) { const rows = data.map((w) => ({ - ACTIVE: w.id === config.activeWorkspaceId ? '*' : ' ', + ACTIVE: w.id === activeId ? '*' : ' ', NAME: w.name, ID: w.id, ROLE: w.role, diff --git a/src/config/index.ts b/src/config/index.ts index 387c7df4..ff303a82 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -24,6 +24,7 @@ import { existsSync, } from 'node:fs'; import { getConfigFile, safeWriteFileSync } from '../storage/path.js'; +import { envWorkspaceId } from './env-vars.js'; /** * The full on-disk config shape — narrow type for the keys this module reads / @@ -124,6 +125,12 @@ export function writePosthogConfig(slice: Partial): void { * cycle (commands/workspace.ts → api/client.ts → … → posthog.ts → config). */ export function readActiveWorkspacePublicId(): string | undefined { + // HOOKMYAPP_WORKSPACE_ID outranks the persisted selection for every command + // (AIT-438), so telemetry has to attribute events to it too — otherwise each + // env-driven invocation is tagged with the stale persisted workspace, or no + // workspace at all, while the API request targets a different one. + const env = envWorkspaceId(); + if (env.startsWith('ws_')) return env; const cfg = readFullConfig(); const id = cfg.activeWorkspaceId; return typeof id === 'string' && id.startsWith('ws_') ? id : undefined; From 539ef1aa104396eb08af7a35801ce39f621761ad Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 14:08:00 +0300 Subject: [PATCH 10/15] =?UTF-8?q?fix:=20address=20fifth=20Codex=20pass=20?= =?UTF-8?q?=E2=80=94=20telemetry=20precedence,=20403=20guidance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous telemetry fix returned the environment workspace unconditionally, so `--workspace ws_B` with HOOKMYAPP_WORKSPACE_ID=ws_A sent the request to ws_B and tagged the event ws_A. The workspace context moved to a leaf module both the API client and telemetry can read, so events now carry the workspace the invocation actually resolved. A bare 403 maps to PermissionError, which the environment rewrite did not cover: it named the persisted workspace slug and told the user to run `hookmyapp login`, which refuses while the variable is set. Environment credentials now get permission guidance naming the effective workspace. --- src/api/__tests__/env-key-bearer.test.ts | 23 ++++++++++- src/api/client.ts | 40 ++++++++++++------- .../__tests__/workspace-env-override.test.ts | 27 +++++++++++++ src/commands/_helpers.ts | 3 +- src/config/index.ts | 11 +++-- src/config/workspace-context.ts | 19 +++++++++ 6 files changed, 103 insertions(+), 20 deletions(-) create mode 100644 src/config/workspace-context.ts diff --git a/src/api/__tests__/env-key-bearer.test.ts b/src/api/__tests__/env-key-bearer.test.ts index 96f21f12..3510aafd 100644 --- a/src/api/__tests__/env-key-bearer.test.ts +++ b/src/api/__tests__/env-key-bearer.test.ts @@ -2,7 +2,8 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { apiClient } from '../client.js'; import { API_KEY_ENV_VAR } from '../../config/env-vars.js'; import * as secrets from '../../storage/secrets.js'; -import { AuthError } from '../../output/error.js'; +import { AuthError, ForbiddenError } from '../../output/error.js'; +import { setWorkspaceContext } from '../../config/workspace-context.js'; // The env key must reach the wire as the bearer token, exactly as a stored // agent credential does — no refresh attempt, no rewriting (AIT-438). @@ -36,6 +37,26 @@ describe('apiClient with HOOKMYAPP_API_KEY (AIT-438)', () => { ); }); + // A bare 403 maps to PermissionError, whose stock message says "run: + // hookmyapp login" — which login now refuses to do while the key is set. + it('gives permission guidance, not login advice, on a bare 403', async () => { + process.env[API_KEY_ENV_VAR] = 'hmok_lowperm'; + setWorkspaceContext({ workspaceId: 'ws_abc12345' }); + vi.stubGlobal('fetch', vi.fn(async () => ({ + ok: false, + status: 403, + headers: new Headers(), + json: async () => ({ message: 'Forbidden' }), + }))); + + await expect(apiClient('/channels')).rejects.toThrow(ForbiddenError); + await expect(apiClient('/channels')).rejects.toThrow( + /HOOKMYAPP_API_KEY lacks permission for workspace ws_abc12345/, + ); + await expect(apiClient('/channels')).rejects.not.toThrow(/hookmyapp login/); + setWorkspaceContext({ workspaceId: null }); + }); + it('sends the env key as the Authorization bearer', async () => { process.env[API_KEY_ENV_VAR] = 'hmok_livekey123'; const fetchMock = vi.fn(async (_url: string, _init: RequestInit) => ({ diff --git a/src/api/client.ts b/src/api/client.ts index 5ad82724..9fd73b2b 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -21,16 +21,15 @@ import { } from '../config/env-profiles.js'; import { buildVersionHeaders } from './version-headers.js'; import { API_KEY_ENV_VAR } from '../config/env-vars.js'; +import { getWorkspaceContext } from '../config/workspace-context.js'; -// Module-level workspace context populated by the top-level CLI entry after -// parsing --workspace. Explicit options.workspaceId on a specific apiClient() -// call always wins; this is the fallback used by every other call site, so -// subcommands like `env` don't have to remember to thread the header through. -let workspaceCtx: { workspaceId: string | null } = { workspaceId: null }; - -export function setWorkspaceContext(ctx: { workspaceId: string | null }): void { - workspaceCtx = ctx; -} +// Workspace context populated by getDefaultWorkspaceId() after applying the +// --workspace / HOOKMYAPP_WORKSPACE_ID / config precedence. Explicit +// options.workspaceId on a specific apiClient() call always wins; this is the +// fallback used by every other call site, so subcommands like `env` don't have +// to remember to thread the header through. Lives in a leaf module because +// telemetry needs the same value and cannot import this file (AIT-438). +export { setWorkspaceContext } from '../config/workspace-context.js'; function decodeJwtExp(token: string): number { try { @@ -354,7 +353,7 @@ export async function apiClient( // global --workspace context. /workspaces is the discovery endpoint — we // intentionally never inject (chicken-and-egg: the user is fetching the // list precisely to pick a workspace). - const resolvedWsId = workspaceId !== undefined ? workspaceId : workspaceCtx.workspaceId; + const resolvedWsId = workspaceId !== undefined ? workspaceId : getWorkspaceContext(); if (resolvedWsId && !path.startsWith('/workspaces')) { headers['X-Workspace-Id'] = resolvedWsId; } @@ -393,10 +392,23 @@ export async function apiClient( // A 401 on an env credential must not say "Session expired. Run: login": // there is no session, and `login` refuses to run while the variable is // set, so that guidance is a loop (AIT-438). - if (err instanceof AuthError && 'source' in creds && creds.source === 'env') { - throw new AuthError( - `The API key in ${API_KEY_ENV_VAR} was rejected (invalid or revoked). Replace it or unset the variable.`, - ); + if ('source' in creds && creds.source === 'env') { + if (err instanceof AuthError) { + throw new AuthError( + `The API key in ${API_KEY_ENV_VAR} was rejected (invalid or revoked). Replace it or unset the variable.`, + ); + } + // A bare 403 maps to PermissionError, whose message names the persisted + // workspace slug and says "run: hookmyapp login" — wrong on both counts + // here: login refuses while the variable is set, and the workspace in + // play may have come from the environment too. + if (err instanceof PermissionError) { + const ws = getWorkspaceContext() ?? resolvedWsId ?? '(unresolved)'; + throw new ForbiddenError( + `The API key in ${API_KEY_ENV_VAR} lacks permission for workspace ${ws}. Use a key with the required role, or unset the variable to use your stored login.`, + 'AGENT_KEY_FORBIDDEN', + ); + } } throw err; } diff --git a/src/commands/__tests__/workspace-env-override.test.ts b/src/commands/__tests__/workspace-env-override.test.ts index 2f1c9e65..d149f9bb 100644 --- a/src/commands/__tests__/workspace-env-override.test.ts +++ b/src/commands/__tests__/workspace-env-override.test.ts @@ -47,3 +47,30 @@ describe('effectiveActiveWorkspaceId', () => { expect(effectiveActiveWorkspaceId()).not.toBe('""'); }); }); + +// Telemetry must tag events with the workspace the request targeted, which is +// the flag when one was passed — not the environment override it outranks. +describe('telemetry workspace attribution', () => { + const original = process.env.HOOKMYAPP_WORKSPACE_ID; + afterEach(async () => { + if (original === undefined) delete process.env.HOOKMYAPP_WORKSPACE_ID; + else process.env.HOOKMYAPP_WORKSPACE_ID = original; + const { setWorkspaceContext } = await import('../../config/workspace-context.js'); + setWorkspaceContext({ workspaceId: null }); + }); + + it('prefers the workspace resolved for the invocation over the env value', async () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_envAAAAA'; + const { setWorkspaceContext } = await import('../../config/workspace-context.js'); + const { readActiveWorkspacePublicId } = await import('../../config/index.js'); + + setWorkspaceContext({ workspaceId: 'ws_flagBBBB' }); + expect(readActiveWorkspacePublicId()).toBe('ws_flagBBBB'); + }); + + it('falls back to the env value when nothing was resolved', async () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_envAAAAA'; + const { readActiveWorkspacePublicId } = await import('../../config/index.js'); + expect(readActiveWorkspacePublicId()).toBe('ws_envAAAAA'); + }); +}); diff --git a/src/commands/_helpers.ts b/src/commands/_helpers.ts index 172463e6..7880f51c 100644 --- a/src/commands/_helpers.ts +++ b/src/commands/_helpers.ts @@ -1,4 +1,5 @@ -import { apiClient, setWorkspaceContext } from '../api/client.js'; +import { apiClient } from '../api/client.js'; +import { setWorkspaceContext } from '../config/workspace-context.js'; import { AuthError, CliError, NetworkError, ValidationError, exitCodeFor } from '../output/error.js'; import { readWorkspaceConfig, writeWorkspaceConfig } from './workspace.js'; import { isLikelyUuid, isValidPublicId } from '../lib/publicId.js'; diff --git a/src/config/index.ts b/src/config/index.ts index ff303a82..ce0f0438 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -25,6 +25,7 @@ import { } from 'node:fs'; import { getConfigFile, safeWriteFileSync } from '../storage/path.js'; import { envWorkspaceId } from './env-vars.js'; +import { getWorkspaceContext } from './workspace-context.js'; /** * The full on-disk config shape — narrow type for the keys this module reads / @@ -125,10 +126,12 @@ export function writePosthogConfig(slice: Partial): void { * cycle (commands/workspace.ts → api/client.ts → … → posthog.ts → config). */ export function readActiveWorkspacePublicId(): string | undefined { - // HOOKMYAPP_WORKSPACE_ID outranks the persisted selection for every command - // (AIT-438), so telemetry has to attribute events to it too — otherwise each - // env-driven invocation is tagged with the stale persisted workspace, or no - // workspace at all, while the API request targets a different one. + // Mirror getDefaultWorkspaceId()'s precedence exactly, or events get tagged + // with a workspace the request never touched (AIT-438). The resolved context + // is set once the flag/env/config decision is made, so it already accounts + // for `--workspace ws_B` beating HOOKMYAPP_WORKSPACE_ID=ws_A. + const resolved = getWorkspaceContext(); + if (resolved?.startsWith('ws_')) return resolved; const env = envWorkspaceId(); if (env.startsWith('ws_')) return env; const cfg = readFullConfig(); diff --git a/src/config/workspace-context.ts b/src/config/workspace-context.ts new file mode 100644 index 00000000..547688fd --- /dev/null +++ b/src/config/workspace-context.ts @@ -0,0 +1,19 @@ +/** + * The workspace resolved for THIS invocation, published by + * getDefaultWorkspaceId() once it has applied the precedence rules + * (--workspace > HOOKMYAPP_WORKSPACE_ID > persisted config). + * + * A leaf module with no imports: both the API client (which sends + * X-Workspace-Id) and telemetry (which tags events) need it, and neither can + * import the other without a cycle. Whoever reads this gets the workspace the + * request actually targeted, not a guess reconstructed from config. + */ +let resolvedWorkspaceId: string | null = null; + +export function setWorkspaceContext(ctx: { workspaceId: string | null }): void { + resolvedWorkspaceId = ctx.workspaceId; +} + +export function getWorkspaceContext(): string | null { + return resolvedWorkspaceId; +} From d5eeb6e6e8489219af3ef2161cbd05f8431b6371 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 14:14:44 +0300 Subject: [PATCH 11/15] =?UTF-8?q?fix:=20address=20CodeRabbit=20on=209b9ce6?= =?UTF-8?q?1=20=E2=80=94=20invocation=20workspace=20on=20status=20surfaces?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit effectiveActiveWorkspaceId() ignored the workspace resolved for the invocation, so `--workspace ws_B` reported ws_A everywhere. It now follows getDefaultWorkspaceId()'s own order: resolved context, environment, persisted. workspace list, customers list and customers current never resolved the `--workspace` flag at all. They now resolve it against the list they already fetched (no extra round trip) via markActiveWorkspaceId(). The 403 message used the shared context ahead of resolvedWsId, which already folds in a per-call options.workspaceId — so a call with an explicit workspace named the wrong one. Note the `??` trap this hit on the way: envWorkspaceId() returns '' when unset, so a `??` chain stopped there and never reached the persisted selection. Two existing tests caught it; the chain uses `||`. --- src/api/client.ts | 4 +- .../__tests__/workspace-env-override.test.ts | 32 +++++++++++++++- src/commands/customers.ts | 6 +-- src/commands/workspace.ts | 37 ++++++++++++++++--- 4 files changed, 69 insertions(+), 10 deletions(-) diff --git a/src/api/client.ts b/src/api/client.ts index 9fd73b2b..0574628f 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -403,7 +403,9 @@ export async function apiClient( // here: login refuses while the variable is set, and the workspace in // play may have come from the environment too. if (err instanceof PermissionError) { - const ws = getWorkspaceContext() ?? resolvedWsId ?? '(unresolved)'; + // resolvedWsId already folds in options.workspaceId, which beats the + // shared context for this specific call. + const ws = resolvedWsId ?? '(unresolved)'; throw new ForbiddenError( `The API key in ${API_KEY_ENV_VAR} lacks permission for workspace ${ws}. Use a key with the required role, or unset the variable to use your stored login.`, 'AGENT_KEY_FORBIDDEN', diff --git a/src/commands/__tests__/workspace-env-override.test.ts b/src/commands/__tests__/workspace-env-override.test.ts index d149f9bb..00036168 100644 --- a/src/commands/__tests__/workspace-env-override.test.ts +++ b/src/commands/__tests__/workspace-env-override.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, afterEach } from 'vitest'; -import { switchActiveWorkspace, effectiveActiveWorkspaceId } from '../workspace.js'; +import { switchActiveWorkspace, effectiveActiveWorkspaceId, markActiveWorkspaceId } from '../workspace.js'; import { ValidationError } from '../../output/error.js'; // AIT-438: HOOKMYAPP_WORKSPACE_ID outranks the persisted selection, so a @@ -74,3 +74,33 @@ describe('telemetry workspace attribution', () => { expect(readActiveWorkspacePublicId()).toBe('ws_envAAAAA'); }); }); + +// --workspace outranks the env override for the invocation, so a status +// surface that ignores it stars a workspace the command is not using. +describe('markActiveWorkspaceId', () => { + const original = process.env.HOOKMYAPP_WORKSPACE_ID; + const all = [ + { id: 'ws_env12345', name: 'EnvSpace' }, + { id: 'ws_flag1234', name: 'FlagSpace' }, + ]; + afterEach(() => { + if (original === undefined) delete process.env.HOOKMYAPP_WORKSPACE_ID; + else process.env.HOOKMYAPP_WORKSPACE_ID = original; + }); + + it('resolves the flag by name against the fetched list', () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_env12345'; + expect(markActiveWorkspaceId(all, 'FlagSpace')).toBe('ws_flag1234'); + expect(markActiveWorkspaceId(all, 'ws_flag1234')).toBe('ws_flag1234'); + }); + + it('falls back to the effective workspace with no flag', () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_env12345'; + expect(markActiveWorkspaceId(all)).toBe('ws_env12345'); + }); + + it('ignores a flag that matches nothing in the list', () => { + process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_env12345'; + expect(markActiveWorkspaceId(all, 'nope')).toBe('ws_env12345'); + }); +}); diff --git a/src/commands/customers.ts b/src/commands/customers.ts index 8feadccf..d13ffdba 100644 --- a/src/commands/customers.ts +++ b/src/commands/customers.ts @@ -4,7 +4,7 @@ import { output } from '../output/format.js'; import { ValidationError } from '../output/error.js'; import { addExamples } from '../output/help.js'; import { dropWorkosOrgId, type Workspace } from '../types/workspace.js'; -import { switchActiveWorkspace, effectiveActiveWorkspaceId } from './workspace.js'; +import { switchActiveWorkspace, markActiveWorkspaceId } from './workspace.js'; import { getDefaultWorkspaceId, resolveOrgPublicIdForWorkspace } from './_helpers.js'; /** A /workspaces union row carries the org it belongs to; the base Workspace type does not. */ @@ -57,7 +57,7 @@ export function registerCustomersCommand(program: Command): void { // Effective id, not the raw config: HOOKMYAPP_WORKSPACE_ID outranks the // persisted selection, so marking the config row would star a workspace // no command is using (AIT-438). - const activeId = effectiveActiveWorkspaceId(); + const activeId = markActiveWorkspaceId(all, program.opts().workspace as string | undefined); const rows = customers.map((w) => ({ ACTIVE: w.id === activeId ? '*' : ' ', NAME: w.name, @@ -112,7 +112,7 @@ export function registerCustomersCommand(program: Command): void { .option('--json', 'Output machine-readable JSON') .action(async (opts: { json?: boolean }) => { const all = (await apiClient('/workspaces')) as Workspace[]; - const active = effectiveActiveWorkspaceId(); + const active = markActiveWorkspaceId(all, program.opts().workspace as string | undefined); const cur = all.find((w) => w.id === active && w.kind === 'customer'); const json = !!(opts.json || program.opts().json); if (!cur) { diff --git a/src/commands/workspace.ts b/src/commands/workspace.ts index 3d6d871c..a8caf740 100644 --- a/src/commands/workspace.ts +++ b/src/commands/workspace.ts @@ -9,6 +9,7 @@ import fs from 'node:fs'; import { getConfigFile, safeWriteFileSync } from '../storage/path.js'; import { resolveEnv } from '../config/env-profiles.js'; import { WORKSPACE_ENV_VAR, envWorkspaceId } from '../config/env-vars.js'; +import { getWorkspaceContext } from '../config/workspace-context.js'; export interface WorkspaceConfig { activeWorkspaceId?: string; @@ -59,12 +60,38 @@ export function readWorkspaceConfig(): WorkspaceConfig { } /** - * The workspace commands actually act on: the env override when set, else the - * persisted selection. Status surfaces must use this, not the raw config, or - * they describe a workspace no command is using (AIT-438). + * The workspace commands actually act on, in getDefaultWorkspaceId()'s own + * precedence: the workspace resolved for this invocation (--workspace), then + * the env override, then the persisted selection. Status surfaces must use + * this, not the raw config, or they describe a workspace no command is + * using (AIT-438). */ export function effectiveActiveWorkspaceId(): string | undefined { - return envWorkspaceId() || readWorkspaceConfig().activeWorkspaceId || undefined; + // `||`, not `??`: envWorkspaceId() returns '' when unset, and `??` would + // stop there and never reach the persisted selection. + return ( + getWorkspaceContext() || envWorkspaceId() || readWorkspaceConfig().activeWorkspaceId || undefined + ); +} + +/** + * The workspace a status surface should mark as active, given the list it just + * fetched. `--workspace` outranks everything but is a name-or-id, so it is + * resolved against that list rather than with another round trip; surfaces + * that never resolve the flag would otherwise star the env/persisted + * workspace while the invocation targets another (AIT-438). + */ +export function markActiveWorkspaceId( + all: Array<{ id: string; name: string }>, + flag?: string, +): string | undefined { + if (flag) { + const match = all.find( + (w) => w.id === flag || w.name === flag || w.name.toLowerCase() === flag.toLowerCase(), + ); + if (match) return match.id; + } + return effectiveActiveWorkspaceId(); } export function writeWorkspaceConfig(config: WorkspaceConfig): void { @@ -230,7 +257,7 @@ export function registerWorkspaceCommand(program: Command): void { // the fail-safe: an unknown kind never renders as a team workspace. const all = (await apiClient('/workspaces')) as Workspace[]; const data = all.filter((w) => w.kind === 'team').map(dropWorkosOrgId); - const activeId = effectiveActiveWorkspaceId(); + const activeId = markActiveWorkspaceId(all, program.opts().workspace as string | undefined); if (opts.json) { console.log(JSON.stringify(data, null, 2)); return; From c24d73607b87578e19e72cd7e962f94c39c69faa Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 14:19:32 +0300 Subject: [PATCH 12/15] fix: credentials revoke must clean up the stored credential The post-revoke cleanup read the resolved credential, so with the same key also in HOOKMYAPP_API_KEY it saw the synthesized environment credential, which carries no credentialPublicId. The comparison never matched, credentials.json kept a revoked token, and the CLI started 401ing the moment the variable was unset. It reads the stored credential directly now, the same way logout does. --- src/commands/__tests__/credentials.test.ts | 21 +++++++++++++++++++++ src/commands/credentials.ts | 11 ++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/commands/__tests__/credentials.test.ts b/src/commands/__tests__/credentials.test.ts index 535b7477..e1fba548 100644 --- a/src/commands/__tests__/credentials.test.ts +++ b/src/commands/__tests__/credentials.test.ts @@ -83,6 +83,27 @@ test('revoking the currently stored credential clears it from disk', async () => expect(existsSync(join(DIR, 'credentials.json'))).toBe(false); }); +// AIT-438: with the same key also in HOOKMYAPP_API_KEY, reading the resolved +// credential returns the synthesized env one, which has no credentialPublicId +// — the match fails and a revoked token stays on disk, 401ing the moment the +// variable is unset. +test('clears the stored credential even when the env holds the same key', async () => { + const original = process.env.HOOKMYAPP_API_KEY; + process.env.HOOKMYAPP_API_KEY = 'hmok_same'; + writeFileSync( + join(DIR, 'credentials.json'), + JSON.stringify({ accessToken: 'hmok_same', refreshToken: '', expiresAt: 0, kind: 'agent', credentialPublicId: 'ac_pub1', scopes: [] }), + ); + apiClientMock.mockResolvedValue(undefined); + try { + await run(['credentials', 'revoke', 'ac_pub1', '-y', '--json']); + expect(existsSync(join(DIR, 'credentials.json'))).toBe(false); + } finally { + if (original === undefined) delete process.env.HOOKMYAPP_API_KEY; + else process.env.HOOKMYAPP_API_KEY = original; + } +}); + test('revoking a different credential leaves the stored one intact', async () => { writeFileSync( join(DIR, 'credentials.json'), diff --git a/src/commands/credentials.ts b/src/commands/credentials.ts index 1a898f80..615e1d42 100644 --- a/src/commands/credentials.ts +++ b/src/commands/credentials.ts @@ -1,7 +1,7 @@ import { Command } from 'commander'; import { apiClient } from '../api/client.js'; -import { readCredentials, deleteCredentials } from '../auth/store.js'; -import { isAgentCredential } from '../storage/secrets.js'; +import { deleteCredentials } from '../auth/store.js'; +import { isAgentCredential, readSecrets } from '../storage/secrets.js'; import { addExamples } from '../output/help.js'; interface AgentCredentialRow { @@ -60,7 +60,12 @@ export function registerCredentialsCommand(program: Command): void { }); // If this is the credential we're currently authenticated with, drop the // now-dead token from disk so the next command doesn't send a 401. - const creds = await readCredentials(); + // Read the STORED credential, not the resolved one: with the same key + // also in HOOKMYAPP_API_KEY, the synthesized env credential carries no + // credentialPublicId, the comparison never matches, and credentials.json + // keeps a revoked token that 401s as soon as the variable is unset + // (AIT-438). + const creds = await readSecrets(); if (creds && isAgentCredential(creds) && creds.credentialPublicId === publicId) { await deleteCredentials(); } From d1b30815aec319e5550c532a745832e94d3ebe18 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 14:40:27 +0300 Subject: [PATCH 13/15] refactor: split HOOKMYAPP_WORKSPACE_ID out to AIT-441 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workspace override was not in this ticket's acceptance criteria: the ticket said --workspace and X-Workspace-Id should cover it, and the template will pass --workspace explicitly. It doubled the diff and produced about half the review findings, because an override that outranks persisted config invalidates every surface that reads the config directly — workspace use/new, the list markers, customers current, doctor and PostHog attribution each needed their own fix. The work is preserved on branch ait-441-cli-workspace-env-override with those fixes intact; AIT-441 carries the context. This PR is back to the API key fallback the template actually needs. --- src/api/__tests__/env-key-bearer.test.ts | 2 +- src/api/client.ts | 19 ++-- src/commands/__tests__/doctor.test.ts | 59 +--------- .../__tests__/workspace-env-override.test.ts | 106 ------------------ src/commands/_helpers.ts | 22 +--- src/commands/customers.ts | 11 +- src/commands/doctor.ts | 44 ++------ src/commands/workspace.ts | 71 +----------- src/config/env-vars.ts | 9 +- src/config/index.ts | 10 -- src/config/workspace-context.ts | 19 ---- 11 files changed, 33 insertions(+), 339 deletions(-) delete mode 100644 src/commands/__tests__/workspace-env-override.test.ts delete mode 100644 src/config/workspace-context.ts diff --git a/src/api/__tests__/env-key-bearer.test.ts b/src/api/__tests__/env-key-bearer.test.ts index 3510aafd..34c304fe 100644 --- a/src/api/__tests__/env-key-bearer.test.ts +++ b/src/api/__tests__/env-key-bearer.test.ts @@ -3,7 +3,7 @@ import { apiClient } from '../client.js'; import { API_KEY_ENV_VAR } from '../../config/env-vars.js'; import * as secrets from '../../storage/secrets.js'; import { AuthError, ForbiddenError } from '../../output/error.js'; -import { setWorkspaceContext } from '../../config/workspace-context.js'; +import { setWorkspaceContext } from '../client.js'; // The env key must reach the wire as the bearer token, exactly as a stored // agent credential does — no refresh attempt, no rewriting (AIT-438). diff --git a/src/api/client.ts b/src/api/client.ts index 0574628f..461af485 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -21,15 +21,16 @@ import { } from '../config/env-profiles.js'; import { buildVersionHeaders } from './version-headers.js'; import { API_KEY_ENV_VAR } from '../config/env-vars.js'; -import { getWorkspaceContext } from '../config/workspace-context.js'; -// Workspace context populated by getDefaultWorkspaceId() after applying the -// --workspace / HOOKMYAPP_WORKSPACE_ID / config precedence. Explicit -// options.workspaceId on a specific apiClient() call always wins; this is the -// fallback used by every other call site, so subcommands like `env` don't have -// to remember to thread the header through. Lives in a leaf module because -// telemetry needs the same value and cannot import this file (AIT-438). -export { setWorkspaceContext } from '../config/workspace-context.js'; +// Module-level workspace context populated by the top-level CLI entry after +// parsing --workspace. Explicit options.workspaceId on a specific apiClient() +// call always wins; this is the fallback used by every other call site, so +// subcommands like `env` don't have to remember to thread the header through. +let workspaceCtx: { workspaceId: string | null } = { workspaceId: null }; + +export function setWorkspaceContext(ctx: { workspaceId: string | null }): void { + workspaceCtx = ctx; +} function decodeJwtExp(token: string): number { try { @@ -353,7 +354,7 @@ export async function apiClient( // global --workspace context. /workspaces is the discovery endpoint — we // intentionally never inject (chicken-and-egg: the user is fetching the // list precisely to pick a workspace). - const resolvedWsId = workspaceId !== undefined ? workspaceId : getWorkspaceContext(); + const resolvedWsId = workspaceId !== undefined ? workspaceId : workspaceCtx.workspaceId; if (resolvedWsId && !path.startsWith('/workspaces')) { headers['X-Workspace-Id'] = resolvedWsId; } diff --git a/src/commands/__tests__/doctor.test.ts b/src/commands/__tests__/doctor.test.ts index 93d3c6ba..72d4c36b 100644 --- a/src/commands/__tests__/doctor.test.ts +++ b/src/commands/__tests__/doctor.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; vi.mock('../../auth/store.js', () => ({ readCredentials: vi.fn(async () => null) })); vi.mock('../../api/client.js', () => ({ apiClient: vi.fn() })); vi.mock('../workspace.js', () => ({ readWorkspaceConfig: vi.fn(() => ({})) })); @@ -104,63 +104,6 @@ describe('doctor — auth probe uses the real authenticated request path', () => }); }); -describe('doctor — HOOKMYAPP_WORKSPACE_ID override (AIT-438)', () => { - const original = process.env.HOOKMYAPP_WORKSPACE_ID; - beforeEach(() => { - vi.clearAllMocks(); - vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, status: 200 }))); - vi.mocked(readCredentials).mockResolvedValue({ - accessToken: 'hmok_x', - refreshToken: '', - expiresAt: 0, - kind: 'agent', - source: 'env', - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - vi.mocked(readWorkspaceConfig).mockReturnValue({}); - }); - afterEach(() => { - if (original === undefined) delete process.env.HOOKMYAPP_WORKSPACE_ID; - else process.env.HOOKMYAPP_WORKSPACE_ID = original; - }); - - it('reports the env workspace instead of "(none)"', async () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_abc12345'; - vi.mocked(apiClient).mockResolvedValue([{ id: 'ws_abc12345' }]); - - const report = await collectDoctorReport({ checkTools: false }); - const ws = report.checks.find((c) => c.id === 'workspace')!; - - expect(ws.ok).toBe(true); - expect(ws.detail).toBe('ws_abc12345 — HOOKMYAPP_WORKSPACE_ID (environment)'); - }); - - it('fails the check on a malformed value, the same way commands do', async () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'not-a-ws-id'; - vi.mocked(apiClient).mockResolvedValue([]); - - const ws = (await collectDoctorReport({ checkTools: false })).checks.find( - (c) => c.id === 'workspace', - )!; - - expect(ws.ok).toBe(false); - expect(ws.detail).toContain('HOOKMYAPP_WORKSPACE_ID'); - }); - - it('points at the variable, not `workspace use`, when the env workspace is unknown', async () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_gone1234'; - vi.mocked(apiClient).mockResolvedValue([{ id: 'ws_other123' }]); - - const ws = (await collectDoctorReport({ checkTools: false })).checks.find( - (c) => c.id === 'workspace', - )!; - - expect(ws.ok).toBe(false); - expect(ws.detail).toContain('unset HOOKMYAPP_WORKSPACE_ID'); - expect(ws.detail).not.toContain('workspace use'); - }); -}); - describe('doctor — active workspace is validated against the backend (AIT-51)', () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/src/commands/__tests__/workspace-env-override.test.ts b/src/commands/__tests__/workspace-env-override.test.ts deleted file mode 100644 index 00036168..00000000 --- a/src/commands/__tests__/workspace-env-override.test.ts +++ /dev/null @@ -1,106 +0,0 @@ -import { describe, it, expect, afterEach } from 'vitest'; -import { switchActiveWorkspace, effectiveActiveWorkspaceId, markActiveWorkspaceId } from '../workspace.js'; -import { ValidationError } from '../../output/error.js'; - -// AIT-438: HOOKMYAPP_WORKSPACE_ID outranks the persisted selection, so a -// `workspace use` that "succeeds" would send the next command at the old -// workspace. Refuse instead of lying. -describe('workspace use under HOOKMYAPP_WORKSPACE_ID', () => { - const original = process.env.HOOKMYAPP_WORKSPACE_ID; - afterEach(() => { - if (original === undefined) delete process.env.HOOKMYAPP_WORKSPACE_ID; - else process.env.HOOKMYAPP_WORKSPACE_ID = original; - }); - - it('refuses the switch while the override is set', async () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_abc12345'; - await expect(switchActiveWorkspace('some-other-ws')).rejects.toThrow(ValidationError); - await expect(switchActiveWorkspace('some-other-ws')).rejects.toThrow( - /HOOKMYAPP_WORKSPACE_ID is set to ws_abc12345/, - ); - }); - - it('refuses `customers use` the same way', async () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_abc12345'; - await expect( - switchActiveWorkspace('some-customer', { kind: 'customer' }), - ).rejects.toThrow(ValidationError); - }); -}); - -// Status surfaces (customers list/current, doctor) must describe the workspace -// commands actually use, not the persisted one the override supersedes. -describe('effectiveActiveWorkspaceId', () => { - const original = process.env.HOOKMYAPP_WORKSPACE_ID; - afterEach(() => { - if (original === undefined) delete process.env.HOOKMYAPP_WORKSPACE_ID; - else process.env.HOOKMYAPP_WORKSPACE_ID = original; - }); - - it('prefers the environment override', () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_env12345'; - expect(effectiveActiveWorkspaceId()).toBe('ws_env12345'); - }); - - it('treats a quoted-empty value as unset', () => { - process.env.HOOKMYAPP_WORKSPACE_ID = '""'; - expect(effectiveActiveWorkspaceId()).not.toBe('""'); - }); -}); - -// Telemetry must tag events with the workspace the request targeted, which is -// the flag when one was passed — not the environment override it outranks. -describe('telemetry workspace attribution', () => { - const original = process.env.HOOKMYAPP_WORKSPACE_ID; - afterEach(async () => { - if (original === undefined) delete process.env.HOOKMYAPP_WORKSPACE_ID; - else process.env.HOOKMYAPP_WORKSPACE_ID = original; - const { setWorkspaceContext } = await import('../../config/workspace-context.js'); - setWorkspaceContext({ workspaceId: null }); - }); - - it('prefers the workspace resolved for the invocation over the env value', async () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_envAAAAA'; - const { setWorkspaceContext } = await import('../../config/workspace-context.js'); - const { readActiveWorkspacePublicId } = await import('../../config/index.js'); - - setWorkspaceContext({ workspaceId: 'ws_flagBBBB' }); - expect(readActiveWorkspacePublicId()).toBe('ws_flagBBBB'); - }); - - it('falls back to the env value when nothing was resolved', async () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_envAAAAA'; - const { readActiveWorkspacePublicId } = await import('../../config/index.js'); - expect(readActiveWorkspacePublicId()).toBe('ws_envAAAAA'); - }); -}); - -// --workspace outranks the env override for the invocation, so a status -// surface that ignores it stars a workspace the command is not using. -describe('markActiveWorkspaceId', () => { - const original = process.env.HOOKMYAPP_WORKSPACE_ID; - const all = [ - { id: 'ws_env12345', name: 'EnvSpace' }, - { id: 'ws_flag1234', name: 'FlagSpace' }, - ]; - afterEach(() => { - if (original === undefined) delete process.env.HOOKMYAPP_WORKSPACE_ID; - else process.env.HOOKMYAPP_WORKSPACE_ID = original; - }); - - it('resolves the flag by name against the fetched list', () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_env12345'; - expect(markActiveWorkspaceId(all, 'FlagSpace')).toBe('ws_flag1234'); - expect(markActiveWorkspaceId(all, 'ws_flag1234')).toBe('ws_flag1234'); - }); - - it('falls back to the effective workspace with no flag', () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_env12345'; - expect(markActiveWorkspaceId(all)).toBe('ws_env12345'); - }); - - it('ignores a flag that matches nothing in the list', () => { - process.env.HOOKMYAPP_WORKSPACE_ID = 'ws_env12345'; - expect(markActiveWorkspaceId(all, 'nope')).toBe('ws_env12345'); - }); -}); diff --git a/src/commands/_helpers.ts b/src/commands/_helpers.ts index 7880f51c..02cafa42 100644 --- a/src/commands/_helpers.ts +++ b/src/commands/_helpers.ts @@ -1,9 +1,7 @@ -import { apiClient } from '../api/client.js'; -import { setWorkspaceContext } from '../config/workspace-context.js'; +import { apiClient, setWorkspaceContext } from '../api/client.js'; import { AuthError, CliError, NetworkError, ValidationError, exitCodeFor } from '../output/error.js'; import { readWorkspaceConfig, writeWorkspaceConfig } from './workspace.js'; import { isLikelyUuid, isValidPublicId } from '../lib/publicId.js'; -import { WORKSPACE_ENV_VAR, envWorkspaceId } from '../config/env-vars.js'; import { emit, shouldEmitCommandInvoked } from '../observability/posthog.js'; import type { CliExitCode } from '../analytics/events.js'; import { getCliVersion } from '../observability/posthog.js'; @@ -91,22 +89,6 @@ export async function getDefaultWorkspaceId(): Promise { return match.id; } - // AIT-438: a headless caller has no workspace config to read — the same - // environment that carries HOOKMYAPP_API_KEY carries the workspace it is - // scoped to. Mirrors HOOKMYAPP_CHANNEL_ID (see resolveChannelRefOrDefault) - // and sits above the stored config for the same reason the API key does: - // an explicitly exported value beats a persisted default. - const envWorkspace = envWorkspaceId(); - if (envWorkspace) { - if (!isValidPublicId(envWorkspace, 'ws')) { - throw new ValidationError( - `${WORKSPACE_ENV_VAR} must be a workspace publicId (ws_<8-char>), got "${envWorkspace}".`, - ); - } - setWorkspaceContext({ workspaceId: envWorkspace }); - return envWorkspace; - } - const config = readWorkspaceConfig(); if (config.activeWorkspaceId) { setWorkspaceContext({ workspaceId: config.activeWorkspaceId }); @@ -129,7 +111,7 @@ export async function getDefaultWorkspaceId(): Promise { if (Array.isArray(workspaces) && workspaces.length > 1) { throw new ValidationError( `You're a member of ${workspaces.length} workspaces. Pick one first:\n hookmyapp workspace use \n` + - ' (non-interactive: pass --workspace or set HOOKMYAPP_WORKSPACE_ID)', + ' (non-interactive: pass --workspace )', ); } diff --git a/src/commands/customers.ts b/src/commands/customers.ts index d13ffdba..942133ae 100644 --- a/src/commands/customers.ts +++ b/src/commands/customers.ts @@ -4,7 +4,7 @@ import { output } from '../output/format.js'; import { ValidationError } from '../output/error.js'; import { addExamples } from '../output/help.js'; import { dropWorkosOrgId, type Workspace } from '../types/workspace.js'; -import { switchActiveWorkspace, markActiveWorkspaceId } from './workspace.js'; +import { readWorkspaceConfig, switchActiveWorkspace } from './workspace.js'; import { getDefaultWorkspaceId, resolveOrgPublicIdForWorkspace } from './_helpers.js'; /** A /workspaces union row carries the org it belongs to; the base Workspace type does not. */ @@ -54,12 +54,9 @@ export function registerCustomersCommand(program: Command): void { console.log(JSON.stringify(customers.map(dropWorkosOrgId), null, 2)); return; } - // Effective id, not the raw config: HOOKMYAPP_WORKSPACE_ID outranks the - // persisted selection, so marking the config row would star a workspace - // no command is using (AIT-438). - const activeId = markActiveWorkspaceId(all, program.opts().workspace as string | undefined); + const config = readWorkspaceConfig(); const rows = customers.map((w) => ({ - ACTIVE: w.id === activeId ? '*' : ' ', + ACTIVE: w.id === config.activeWorkspaceId ? '*' : ' ', NAME: w.name, ID: w.id, ROLE: w.role, @@ -112,7 +109,7 @@ export function registerCustomersCommand(program: Command): void { .option('--json', 'Output machine-readable JSON') .action(async (opts: { json?: boolean }) => { const all = (await apiClient('/workspaces')) as Workspace[]; - const active = markActiveWorkspaceId(all, program.opts().workspace as string | undefined); + const active = readWorkspaceConfig().activeWorkspaceId; const cur = all.find((w) => w.id === active && w.kind === 'customer'); const json = !!(opts.json || program.opts().json); if (!cur) { diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index ad0c7d63..1c6bde24 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -1,8 +1,7 @@ import type { Command } from 'commander'; import { runTool } from '../lib/spawn-tool.js'; import { readCredentials } from '../auth/store.js'; -import { API_KEY_ENV_VAR, WORKSPACE_ENV_VAR, envWorkspaceId } from '../config/env-vars.js'; -import { isValidPublicId } from '../lib/publicId.js'; +import { API_KEY_ENV_VAR } from '../config/env-vars.js'; import { apiClient } from '../api/client.js'; import { AuthError, ForbiddenError, PermissionError } from '../output/error.js'; import { isJsonMode } from '../output/format.js'; @@ -115,38 +114,19 @@ export async function collectDoctorReport( let wsId: string | undefined; let wsSlug: string | undefined; - // AIT-438: HOOKMYAPP_WORKSPACE_ID outranks the persisted config in - // getDefaultWorkspaceId(), so doctor has to report the same winner — reading - // only the config would say "(none)" for an env-only agent, or flag the - // ignored persisted workspace as stale. - const envWs = envWorkspaceId(); - let wsFromEnv = false; - if (envWs) { - wsFromEnv = true; - wsId = envWs; - } else { - try { - const cfg = readWorkspaceConfig(); - wsId = cfg.activeWorkspaceId ?? undefined; - wsSlug = cfg.activeWorkspaceSlug ?? undefined; - } catch { /* ignore */ } - } + try { + const cfg = readWorkspaceConfig(); + wsId = cfg.activeWorkspaceId ?? undefined; + wsSlug = cfg.activeWorkspaceSlug ?? undefined; + } catch { /* ignore */ } let wsOk = true; - let wsDetail = wsFromEnv - ? `${envWs} — ${WORKSPACE_ENV_VAR} (environment)` - : wsSlug ?? '(none — auto-resolves on first call)'; - if (wsFromEnv && !isValidPublicId(envWs, 'ws')) { - // Commands throw a ValidationError on this; doctor must not call it OK. - wsOk = false; - wsDetail = `${WORKSPACE_ENV_VAR} must be a workspace publicId (ws_<8-char>), got "${envWs}"`; - } else if (wsId && workspaces && !workspaces.some((w) => w?.id === wsId)) { - // Only a definitive backend list can fail this check — on a network flake - // (workspaces === null) the cache-based verdict stands, matching the auth - // probe's fail-open posture above. + let wsDetail = wsSlug ?? '(none — auto-resolves on first call)'; + // Only a definitive backend list can fail this check — on a network flake + // (workspaces === null) the cache-based verdict stands, matching the auth + // probe's fail-open posture above. + if (wsId && workspaces && !workspaces.some((w) => w?.id === wsId)) { wsOk = false; - wsDetail = wsFromEnv - ? `"${envWs}" not found on this env — fix or unset ${WORKSPACE_ENV_VAR}` - : `"${wsSlug ?? wsId}" not found on this env (stale selection) — run: hookmyapp workspace use `; + wsDetail = `"${wsSlug ?? wsId}" not found on this env (stale selection) — run: hookmyapp workspace use `; } checks.push({ id: 'workspace', label: 'Active workspace', ok: wsOk, hard: false, detail: wsDetail }); const envChannel = process.env.HOOKMYAPP_CHANNEL_ID; diff --git a/src/commands/workspace.ts b/src/commands/workspace.ts index a8caf740..7e101ccd 100644 --- a/src/commands/workspace.ts +++ b/src/commands/workspace.ts @@ -8,8 +8,6 @@ import { isLikelyUuid, isValidPublicId } from '../lib/publicId.js'; import fs from 'node:fs'; import { getConfigFile, safeWriteFileSync } from '../storage/path.js'; import { resolveEnv } from '../config/env-profiles.js'; -import { WORKSPACE_ENV_VAR, envWorkspaceId } from '../config/env-vars.js'; -import { getWorkspaceContext } from '../config/workspace-context.js'; export interface WorkspaceConfig { activeWorkspaceId?: string; @@ -59,41 +57,6 @@ export function readWorkspaceConfig(): WorkspaceConfig { }; } -/** - * The workspace commands actually act on, in getDefaultWorkspaceId()'s own - * precedence: the workspace resolved for this invocation (--workspace), then - * the env override, then the persisted selection. Status surfaces must use - * this, not the raw config, or they describe a workspace no command is - * using (AIT-438). - */ -export function effectiveActiveWorkspaceId(): string | undefined { - // `||`, not `??`: envWorkspaceId() returns '' when unset, and `??` would - // stop there and never reach the persisted selection. - return ( - getWorkspaceContext() || envWorkspaceId() || readWorkspaceConfig().activeWorkspaceId || undefined - ); -} - -/** - * The workspace a status surface should mark as active, given the list it just - * fetched. `--workspace` outranks everything but is a name-or-id, so it is - * resolved against that list rather than with another round trip; surfaces - * that never resolve the flag would otherwise star the env/persisted - * workspace while the invocation targets another (AIT-438). - */ -export function markActiveWorkspaceId( - all: Array<{ id: string; name: string }>, - flag?: string, -): string | undefined { - if (flag) { - const match = all.find( - (w) => w.id === flag || w.name === flag || w.name.toLowerCase() === flag.toLowerCase(), - ); - if (match) return match.id; - } - return effectiveActiveWorkspaceId(); -} - export function writeWorkspaceConfig(config: WorkspaceConfig): void { // Hard cutover: refuse to persist a UUID-shaped activeWorkspaceId. // Symmetric with the read-side drop in readWorkspaceConfig — invariant is @@ -169,17 +132,6 @@ export async function switchActiveWorkspace( opts: { kind?: 'team' | 'customer' } = {}, ): Promise { const noun = opts.kind === 'customer' ? 'customer' : 'workspace'; - // AIT-438: HOOKMYAPP_WORKSPACE_ID outranks the persisted selection, so - // writing one here would report a switch that never takes effect and send - // the next mutating command at the old workspace. Same contract as `login` - // under HOOKMYAPP_API_KEY: refuse, and say what to unset. - const envWs = envWorkspaceId(); - if (envWs) { - throw new ValidationError( - `${WORKSPACE_ENV_VAR} is set to ${envWs} and takes precedence, so this switch would have no effect. ` + - `Unset ${WORKSPACE_ENV_VAR} first, or change its value.`, - ); - } let workspace: Workspace; if (nameOrId) { workspace = (await resolveWorkspace(nameOrId, opts.kind)) as unknown as Workspace; @@ -257,14 +209,14 @@ export function registerWorkspaceCommand(program: Command): void { // the fail-safe: an unknown kind never renders as a team workspace. const all = (await apiClient('/workspaces')) as Workspace[]; const data = all.filter((w) => w.kind === 'team').map(dropWorkosOrgId); - const activeId = markActiveWorkspaceId(all, program.opts().workspace as string | undefined); + const config = readWorkspaceConfig(); if (opts.json) { console.log(JSON.stringify(data, null, 2)); return; } if (!program.opts().json) { const rows = data.map((w) => ({ - ACTIVE: w.id === activeId ? '*' : ' ', + ACTIVE: w.id === config.activeWorkspaceId ? '*' : ' ', NAME: w.name, ID: w.id, ROLE: w.role, @@ -284,25 +236,6 @@ export function registerWorkspaceCommand(program: Command): void { method: 'POST', body: JSON.stringify({ name }), }); - // AIT-438: HOOKMYAPP_WORKSPACE_ID outranks the persisted selection, so - // the usual create-then-switch would report a switch that never takes - // effect. Create the workspace (that part works), skip the switch, say so. - const envWsNew = envWorkspaceId(); - if (envWsNew) { - if (!program.opts().json) { - console.log( - `Created workspace "${result.name}" (${result.id})\n` + - `Active workspace unchanged: ${WORKSPACE_ENV_VAR} is set to ${envWsNew}. ` + - `Unset it to switch, or point it at ${result.id}.`, - ); - } else { - output( - { id: result.id, name: result.name, switched: false, activeWorkspaceId: envWsNew }, - { human: false }, - ); - } - return; - } // Re-scope the JWT to the newly-created workspace's org (server-side, // AIT-182) BEFORE persisting the switch — a failed rescope must not // leave config pointing at a workspace the token isn't valid for. diff --git a/src/config/env-vars.ts b/src/config/env-vars.ts index 89c42ea7..003add26 100644 --- a/src/config/env-vars.ts +++ b/src/config/env-vars.ts @@ -1,12 +1,10 @@ /** - * Names of the environment variables the CLI reads for credentials and - * workspace selection. Kept in a leaf module (no imports) so consumers can + * Name of the environment variable the CLI reads for credentials. Kept in a leaf module (no imports) so consumers can * pull the name without dragging in the auth store — several test suites * mock `auth/store.js` wholesale, and a constant exported from there would * break every one of them. */ export const API_KEY_ENV_VAR = 'HOOKMYAPP_API_KEY'; -export const WORKSPACE_ENV_VAR = 'HOOKMYAPP_WORKSPACE_ID'; /** * Drop one layer of matching surrounding quotes from an env-var value. @@ -34,8 +32,3 @@ export function stripEnvQuotes(value: string): string { export function envApiKey(): string { return stripEnvQuotes(process.env[API_KEY_ENV_VAR]?.trim() ?? ''); } - -/** The workspace override from the environment, normalized ('' when unset). */ -export function envWorkspaceId(): string { - return stripEnvQuotes(process.env[WORKSPACE_ENV_VAR]?.trim() ?? ''); -} diff --git a/src/config/index.ts b/src/config/index.ts index ce0f0438..387c7df4 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -24,8 +24,6 @@ import { existsSync, } from 'node:fs'; import { getConfigFile, safeWriteFileSync } from '../storage/path.js'; -import { envWorkspaceId } from './env-vars.js'; -import { getWorkspaceContext } from './workspace-context.js'; /** * The full on-disk config shape — narrow type for the keys this module reads / @@ -126,14 +124,6 @@ export function writePosthogConfig(slice: Partial): void { * cycle (commands/workspace.ts → api/client.ts → … → posthog.ts → config). */ export function readActiveWorkspacePublicId(): string | undefined { - // Mirror getDefaultWorkspaceId()'s precedence exactly, or events get tagged - // with a workspace the request never touched (AIT-438). The resolved context - // is set once the flag/env/config decision is made, so it already accounts - // for `--workspace ws_B` beating HOOKMYAPP_WORKSPACE_ID=ws_A. - const resolved = getWorkspaceContext(); - if (resolved?.startsWith('ws_')) return resolved; - const env = envWorkspaceId(); - if (env.startsWith('ws_')) return env; const cfg = readFullConfig(); const id = cfg.activeWorkspaceId; return typeof id === 'string' && id.startsWith('ws_') ? id : undefined; diff --git a/src/config/workspace-context.ts b/src/config/workspace-context.ts deleted file mode 100644 index 547688fd..00000000 --- a/src/config/workspace-context.ts +++ /dev/null @@ -1,19 +0,0 @@ -/** - * The workspace resolved for THIS invocation, published by - * getDefaultWorkspaceId() once it has applied the precedence rules - * (--workspace > HOOKMYAPP_WORKSPACE_ID > persisted config). - * - * A leaf module with no imports: both the API client (which sends - * X-Workspace-Id) and telemetry (which tags events) need it, and neither can - * import the other without a cycle. Whoever reads this gets the workspace the - * request actually targeted, not a guess reconstructed from config. - */ -let resolvedWorkspaceId: string | null = null; - -export function setWorkspaceContext(ctx: { workspaceId: string | null }): void { - resolvedWorkspaceId = ctx.workspaceId; -} - -export function getWorkspaceContext(): string | null { - return resolvedWorkspaceId; -} From 0c53db4a01f9777d7139e803f7335f1ac986bf48 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 14:46:50 +0300 Subject: [PATCH 14/15] test: isolate the suite from an exported HOOKMYAPP_API_KEY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged the logout suite; the exposure was wider. With the variable exported — which a developer or CI runner may legitimately have, now that it is a supported auth mechanism — 13 tests across 5 files failed, because the env credential outranks the stored one and silently flips every test that assumes "not logged in" or writes its own credentials.json. vitest.setup.ts already isolates HOOKMYAPP_CONFIG_DIR for the same reason; it now clears the key too, and the tests that exercise the variable set it themselves. Verified green both with and without it exported. --- src/auth/__tests__/logout.test.ts | 25 ++++++++++--------------- vitest.setup.ts | 7 +++++++ 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/auth/__tests__/logout.test.ts b/src/auth/__tests__/logout.test.ts index 6c8e3d3a..58d276d4 100644 --- a/src/auth/__tests__/logout.test.ts +++ b/src/auth/__tests__/logout.test.ts @@ -15,12 +15,17 @@ vi.mock('../../commands/mcp.js', () => ({ removeClaudeMcp: removeClaudeMcpMock } let DIR: string; const SAVED = process.env.HOOKMYAPP_CONFIG_DIR; +// HOOKMYAPP_API_KEY is a supported auth mechanism now, so a CI runner may well +// have one exported. Every baseline expectation here assumes no env +// credential; clear it per test and let the env-specific cases set their own. +const SAVED_KEY = process.env.HOOKMYAPP_API_KEY; let logSpy: ReturnType; beforeEach(() => { removeClaudeMcpMock.mockReset().mockReturnValue({ ok: true }); DIR = mkdtempSync(join(tmpdir(), 'hma-logout-')); process.env.HOOKMYAPP_CONFIG_DIR = DIR; + delete process.env.HOOKMYAPP_API_KEY; logSpy = vi.spyOn(console, 'log').mockReturnValue(undefined); }); @@ -28,6 +33,8 @@ afterEach(() => { rmSync(DIR, { recursive: true, force: true }); if (SAVED) process.env.HOOKMYAPP_CONFIG_DIR = SAVED; else delete process.env.HOOKMYAPP_CONFIG_DIR; + if (SAVED_KEY === undefined) delete process.env.HOOKMYAPP_API_KEY; + else process.env.HOOKMYAPP_API_KEY = SAVED_KEY; vi.restoreAllMocks(); }); @@ -79,13 +86,12 @@ describe('logout', () => { // AIT-438: an env key keeps authenticating after logout. Automation reads // the payload, not the stderr warning, so the signal has to be in the JSON. test('--json flags a still-active HOOKMYAPP_API_KEY', async () => { - const original = process.env.HOOKMYAPP_API_KEY; process.env.HOOKMYAPP_API_KEY = 'hmok_stillhere'; const credsPath = join(DIR, 'credentials.json'); writeFileSync(credsPath, JSON.stringify({ accessToken: 'a', refreshToken: 'r', expiresAt: 1 })); const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); - try { + { await runLogout(['--json']); const written = stdoutSpy.mock.calls.map((c) => String(c[0])).join(''); expect(JSON.parse(written.trim())).toMatchObject({ @@ -93,9 +99,6 @@ describe('logout', () => { envKeyActive: true, envKeyVar: 'HOOKMYAPP_API_KEY', }); - } finally { - if (original === undefined) delete process.env.HOOKMYAPP_API_KEY; - else process.env.HOOKMYAPP_API_KEY = original; } }); @@ -103,7 +106,6 @@ describe('logout', () => { // env key while it is set — a "self-revoke" would kill the credential every // other process is sharing. test('skips the server-side revoke while HOOKMYAPP_API_KEY is set', async () => { - const original = process.env.HOOKMYAPP_API_KEY; process.env.HOOKMYAPP_API_KEY = 'hmok_stillhere'; writeFileSync( join(DIR, 'credentials.json'), @@ -117,7 +119,7 @@ describe('logout', () => { ); const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); - try { + { await runLogout(['--json']); const payload = JSON.parse( stdoutSpy.mock.calls.map((c) => String(c[0])).join('').trim(), @@ -126,16 +128,12 @@ describe('logout', () => { expect(payload.envKeyActive).toBe(true); expect(payload.envKeyIsStoredKey).toBe(true); expect(existsSync(join(DIR, 'credentials.json'))).toBe(false); - } finally { - if (original === undefined) delete process.env.HOOKMYAPP_API_KEY; - else process.env.HOOKMYAPP_API_KEY = original; } }); // A different env key must not stop logout from revoking the stored one: // the request is pinned to the stored token, so it is a real self-revoke. test('still revokes a stored key when the env holds a DIFFERENT key', async () => { - const original = process.env.HOOKMYAPP_API_KEY; process.env.HOOKMYAPP_API_KEY = 'hmok_otherkey'; writeFileSync( join(DIR, 'credentials.json'), @@ -151,7 +149,7 @@ describe('logout', () => { const fetchMock = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); vi.stubGlobal('fetch', fetchMock); - try { + { await runLogout(['--json']); const payload = JSON.parse( stdoutSpy.mock.calls.map((c) => String(c[0])).join('').trim(), @@ -163,9 +161,6 @@ describe('logout', () => { expect(call).toBeDefined(); // Pinned to the stored key, NOT the env key that would otherwise win. expect(call![1].headers.Authorization).toBe('Bearer hmok_storedkey'); - } finally { - if (original === undefined) delete process.env.HOOKMYAPP_API_KEY; - else process.env.HOOKMYAPP_API_KEY = original; } }); diff --git a/vitest.setup.ts b/vitest.setup.ts index 22070a8a..6af57e40 100644 --- a/vitest.setup.ts +++ b/vitest.setup.ts @@ -10,6 +10,13 @@ if (!process.env.HOOKMYAPP_CONFIG_DIR) { process.env.HOOKMYAPP_CONFIG_DIR = mkdtempSync(join(tmpdir(), 'hookmyapp-cli-test-')); } +// Unset the credential env var for the whole suite. It is a supported auth +// mechanism (AIT-438), so a developer or CI runner may legitimately have one +// exported — and it outranks the stored credential, which silently flips every +// test that assumes "not logged in" or writes its own credentials.json. Tests +// that exercise the variable set it themselves. +delete process.env.HOOKMYAPP_API_KEY; + // Pin color output OFF for the whole suite. picocolors latches color support // from process.stdout.isTTY at first import; several tests toggle isTTY to // exercise interactive paths, which — combined with lazy dynamic imports — From 6f381402fd295cbaa70fb74a7da2c6b81fa49a00 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Thu, 20 Aug 2026 14:51:40 +0300 Subject: [PATCH 15/15] fix: give an environment key its own telemetry identity getDistinctId() prefers config.json's lastWorkosSub, which outlives the session that wrote it. On any machine where a human had logged in, every cli_command_invoked and cli_error_shown from a key-authenticated CI or agent run was merged into that person's PostHog profile. An environment key now gets a distinct id derived from the key itself, sharing the same non-secret fingerprint helper the notification cache uses. --- src/config/env-vars.ts | 12 +++++++ src/notifications-nudge.ts | 3 +- .../__tests__/env-key-identity.test.ts | 34 +++++++++++++++++++ src/observability/posthog.ts | 6 ++++ 4 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 src/observability/__tests__/env-key-identity.test.ts diff --git a/src/config/env-vars.ts b/src/config/env-vars.ts index 003add26..6a3dd903 100644 --- a/src/config/env-vars.ts +++ b/src/config/env-vars.ts @@ -1,3 +1,5 @@ +import { createHash } from 'node:crypto'; + /** * Name of the environment variable the CLI reads for credentials. Kept in a leaf module (no imports) so consumers can * pull the name without dragging in the auth store — several test suites @@ -32,3 +34,13 @@ export function stripEnvQuotes(value: string): string { export function envApiKey(): string { return stripEnvQuotes(process.env[API_KEY_ENV_VAR]?.trim() ?? ''); } + +/** + * Stable, non-secret id derived from an opaque API key. Used wherever a key + * needs an identity — telemetry distinct-id, notification cache key — because + * an hmok_ token carries no publicId, no JWT claims and no email. Never + * logged, transmitted, or reversible to the key. + */ +export function keyFingerprint(token: string): string { + return createHash('sha256').update(token).digest('hex').slice(0, 16); +} diff --git a/src/notifications-nudge.ts b/src/notifications-nudge.ts index af1b914b..64422c15 100644 --- a/src/notifications-nudge.ts +++ b/src/notifications-nudge.ts @@ -28,6 +28,7 @@ import { getValidAccessToken } from './api/client.js'; import { readCredentials } from './auth/store.js'; import type { Secrets } from './storage/secrets.js'; import { ENV_PROFILES, getEffectiveApiUrl, isValidEnv } from './config/env-profiles.js'; +import { keyFingerprint } from './config/env-vars.js'; import { getConfigDir } from './storage/path.js'; const DAY_MS = 24 * 60 * 60 * 1000; @@ -50,7 +51,7 @@ export function credentialFingerprint(creds: Secrets): string { // file — one key's unread state and 24h throttle shown for another. Hash the // token into a stable, non-secret id instead. Never logged or transmitted. if (creds.source === 'env') { - return `env:${createHash('sha256').update(creds.accessToken).digest('hex').slice(0, 16)}`; + return `env:${keyFingerprint(creds.accessToken)}`; } try { // WorkOS sessions: `sub` (stable user id) + `org_id` (session org scope) diff --git a/src/observability/__tests__/env-key-identity.test.ts b/src/observability/__tests__/env-key-identity.test.ts new file mode 100644 index 00000000..85d531c6 --- /dev/null +++ b/src/observability/__tests__/env-key-identity.test.ts @@ -0,0 +1,34 @@ +import { describe, it, expect, afterEach } from 'vitest'; +import { getDistinctId } from '../posthog.js'; +import { API_KEY_ENV_VAR } from '../../config/env-vars.js'; + +// AIT-438: lastWorkosSub outlives a human's session, so without a key-specific +// identity every CI/agent event authenticated by HOOKMYAPP_API_KEY is merged +// into whoever last logged in on that machine. +describe('telemetry identity under HOOKMYAPP_API_KEY', () => { + afterEach(() => { + delete process.env[API_KEY_ENV_VAR]; + }); + + it('uses a key-specific distinct id, not the persisted human sub', () => { + const withoutKey = getDistinctId(); + process.env[API_KEY_ENV_VAR] = 'hmok_agentkey'; + const withKey = getDistinctId(); + + expect(withKey).not.toBe(withoutKey); + expect(withKey).toMatch(/^key_[0-9a-f]{16}$/); + // Derived, never the secret itself. + expect(withKey).not.toContain('agentkey'); + }); + + it('is stable for one key and distinct across keys', () => { + process.env[API_KEY_ENV_VAR] = 'hmok_a'; + const a1 = getDistinctId(); + const a2 = getDistinctId(); + process.env[API_KEY_ENV_VAR] = 'hmok_b'; + const b = getDistinctId(); + + expect(a1).toBe(a2); + expect(a1).not.toBe(b); + }); +}); diff --git a/src/observability/posthog.ts b/src/observability/posthog.ts index c22df578..2448f25f 100644 --- a/src/observability/posthog.ts +++ b/src/observability/posthog.ts @@ -43,6 +43,7 @@ import { readActiveWorkspacePublicId, } from '../config/index.js'; import { resolveEnv } from '../config/env-profiles.js'; +import { envApiKey, keyFingerprint } from '../config/env-vars.js'; import { nanoid } from 'nanoid'; import type { EventName, EventProperties } from '../analytics/events.js'; @@ -149,6 +150,11 @@ export function getOrCreateMachineId(): string { * captures. */ export function getDistinctId(): string { + // An environment key is a different principal from whoever last logged in on + // this machine. Without this, CI and agent activity merges into that human's + // PostHog profile, because lastWorkosSub outlives their session (AIT-438). + const envKey = envApiKey(); + if (envKey) return `key_${keyFingerprint(envKey)}`; const cfg = readPosthogConfig(); return cfg.lastWorkosSub ?? getOrCreateMachineId(); }