From d1872cb21303d61b084d1cfda144b0d0651b79d7 Mon Sep 17 00:00:00 2001 From: SleepySML Date: Thu, 3 Sep 2026 17:40:03 +0300 Subject: [PATCH 01/16] fix(cli): exit cleanly on non-interactive SSO auth failure Running any `codemie sdk` command with no valid SSO session and a non-TTY stdin exited 1 by letting a ConfigurationError escape to Node's default handler, printing a raw stack trace, and the message it printed had lost the remediation the user needed. The non-TTY hang named in the ticket title was already fixed by #471; what remained was the quality of the failure. Two independent defects, both required: - getSdkClient() acquired auth outside every action's try/catch, so the throw bypassed handleSdkError. All ~50 sdk actions across 8 files share this one gate, so it is fixed once here rather than at each call site, following the shared-gate approach #471 established. - promptReauthentication's generic 'Authentication expired' throw shadowed the upstream error that names `codemie setup`. The original error is now preserved. promptReauthentication itself is untouched, so its Promise contract and the assistants/chat call site are unaffected. Also: auth diagnostics move to stderr so piped stdout and --json consumers stay clean, and the ora spinner is suppressed when non-interactive, where it emitted raw cursor-control escapes into captured output. Tests: new cli-utils suite (sdk/** had none), new sdk-client spinner suite, extended auth-validation coverage. EPMCDME-14148 Co-Authored-By: Claude --- .../sdk/utils/__tests__/cli-utils.test.ts | 85 +++++++++++++++++++ src/cli/commands/sdk/utils/cli-utils.ts | 13 ++- .../core/__tests__/auth-validation.test.ts | 31 +++++-- src/providers/core/auth-validation.ts | 6 +- src/utils/__tests__/auth.test.ts | 9 +- src/utils/__tests__/sdk-client.test.ts | 79 +++++++++++++++++ src/utils/auth.ts | 10 ++- src/utils/sdk-client.ts | 7 +- 8 files changed, 225 insertions(+), 15 deletions(-) create mode 100644 src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts create mode 100644 src/utils/__tests__/sdk-client.test.ts diff --git a/src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts b/src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts new file mode 100644 index 000000000..9686c80cb --- /dev/null +++ b/src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts @@ -0,0 +1,85 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +vi.mock('@/utils/config.js', () => ({ + ConfigLoader: { load: vi.fn() }, +})); + +vi.mock('@/utils/auth.js', () => ({ + getAuthenticatedClient: vi.fn(), +})); + +vi.mock('@/utils/logger.js', () => ({ + logger: { error: vi.fn(), debug: vi.fn(), warn: vi.fn(), info: vi.fn() }, +})); + +const SSO_ERROR_MESSAGE = + 'SSO authentication required. Please run "codemie setup" with SSO provider first.'; + +describe('getSdkClient', () => { + let exitCode: number | undefined; + let stderr: string[]; + + beforeEach(() => { + exitCode = undefined; + stderr = []; + vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + exitCode = code; + throw new Error(`process.exit:${code}`); + }) as never); + vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + stderr.push(args.join(' ')); + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.resetModules(); + }); + + it('exits non-zero through handleSdkError when authentication fails', async () => { + const { ConfigLoader } = await import('@/utils/config.js'); + const { getAuthenticatedClient } = await import('@/utils/auth.js'); + const { ConfigurationError } = await import('@/utils/errors.js'); + + vi.mocked(ConfigLoader.load).mockResolvedValue({} as never); + vi.mocked(getAuthenticatedClient).mockRejectedValue( + new ConfigurationError(SSO_ERROR_MESSAGE) + ); + + const { getSdkClient } = await import('../cli-utils.js'); + + // handleSdkError terminates the process; the spy converts that into a throw. + await expect(getSdkClient()).rejects.toThrow('process.exit:1'); + expect(exitCode).toBe(1); + }); + + it('surfaces the actionable remediation on stderr rather than a raw stack trace', async () => { + const { ConfigLoader } = await import('@/utils/config.js'); + const { getAuthenticatedClient } = await import('@/utils/auth.js'); + const { ConfigurationError } = await import('@/utils/errors.js'); + + vi.mocked(ConfigLoader.load).mockResolvedValue({} as never); + vi.mocked(getAuthenticatedClient).mockRejectedValue( + new ConfigurationError(SSO_ERROR_MESSAGE) + ); + + const { getSdkClient } = await import('../cli-utils.js'); + + await expect(getSdkClient()).rejects.toThrow('process.exit:1'); + expect(stderr.join('\n')).toContain('codemie setup'); + }); + + it('returns the client unchanged when authentication succeeds', async () => { + const { ConfigLoader } = await import('@/utils/config.js'); + const { getAuthenticatedClient } = await import('@/utils/auth.js'); + + const client = { marker: 'authenticated-client' }; + vi.mocked(ConfigLoader.load).mockResolvedValue({} as never); + vi.mocked(getAuthenticatedClient).mockResolvedValue(client as never); + + const { getSdkClient } = await import('../cli-utils.js'); + + await expect(getSdkClient()).resolves.toBe(client); + expect(exitCode).toBeUndefined(); + }); +}); diff --git a/src/cli/commands/sdk/utils/cli-utils.ts b/src/cli/commands/sdk/utils/cli-utils.ts index 3d0887cc6..6b3273d8e 100644 --- a/src/cli/commands/sdk/utils/cli-utils.ts +++ b/src/cli/commands/sdk/utils/cli-utils.ts @@ -11,10 +11,19 @@ import z, { ZodError } from "zod"; /** * Get an authenticated CodeMie SDK client + * + * Auth acquisition is routed through handleSdkError so a failure exits with a + * formatted message rather than an uncaught throw. Every sdk action calls + * getSdkClient() outside its own try/catch, so this is the single gate that + * keeps a missing session from printing a raw stack trace (EPMCDME-14148). */ export async function getSdkClient(): Promise { - const config = await ConfigLoader.load(); - return getAuthenticatedClient(config); + try { + const config = await ConfigLoader.load(); + return await getAuthenticatedClient(config); + } catch (error) { + handleSdkError(error, "authenticate"); + } } /** diff --git a/src/providers/core/__tests__/auth-validation.test.ts b/src/providers/core/__tests__/auth-validation.test.ts index f0aac3820..9487c177c 100644 --- a/src/providers/core/__tests__/auth-validation.test.ts +++ b/src/providers/core/__tests__/auth-validation.test.ts @@ -39,7 +39,7 @@ describe('handleAuthValidationFailure', () => { const { isNonInteractiveEnvironment } = await import('../../../utils/interactive.js'); isNonInteractiveEnvironmentMock = isNonInteractiveEnvironment as ReturnType; isNonInteractiveEnvironmentMock.mockReturnValue(true); - const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const { handleAuthValidationFailure } = await import('../auth-validation.js'); const setupSteps = { promptForReauth: promptForReauthSpy } as unknown as ProviderSetupSteps; @@ -52,7 +52,7 @@ describe('handleAuthValidationFailure', () => { expect(promptForReauthSpy).not.toHaveBeenCalled(); expect(result).toBe(false); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(consoleErrorSpy).toHaveBeenCalledWith( expect.stringContaining('No valid SSO credentials found.') ); }); @@ -61,7 +61,7 @@ describe('handleAuthValidationFailure', () => { const { isNonInteractiveEnvironment } = await import('../../../utils/interactive.js'); isNonInteractiveEnvironmentMock = isNonInteractiveEnvironment as ReturnType; isNonInteractiveEnvironmentMock.mockReturnValue(true); - const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const { handleAuthValidationFailure } = await import('../auth-validation.js'); const setupSteps = { promptForReauth: promptForReauthSpy } as unknown as ProviderSetupSteps; @@ -69,14 +69,14 @@ describe('handleAuthValidationFailure', () => { await handleAuthValidationFailure(validationResult, setupSteps, testConfig); - expect(consoleLogSpy).toHaveBeenCalledTimes(1); + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); }); it('should keep existing behavior when setupSteps has no promptForReauth (JWT-style), regardless of TTY', async () => { const { isNonInteractiveEnvironment } = await import('../../../utils/interactive.js'); isNonInteractiveEnvironmentMock = isNonInteractiveEnvironment as ReturnType; isNonInteractiveEnvironmentMock.mockReturnValue(false); - const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const { handleAuthValidationFailure } = await import('../auth-validation.js'); const setupSteps = {} as ProviderSetupSteps; @@ -85,14 +85,14 @@ describe('handleAuthValidationFailure', () => { const result = await handleAuthValidationFailure(validationResult, setupSteps, testConfig); expect(result).toBe(false); - expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('JWT token missing')); + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('JWT token missing')); }); it('should return false when setupSteps is null, regardless of TTY', async () => { const { isNonInteractiveEnvironment } = await import('../../../utils/interactive.js'); isNonInteractiveEnvironmentMock = isNonInteractiveEnvironment as ReturnType; isNonInteractiveEnvironmentMock.mockReturnValue(true); - vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); const { handleAuthValidationFailure } = await import('../auth-validation.js'); const validationResult: AuthValidationResult = { valid: false, error: 'no provider configured' }; @@ -101,4 +101,21 @@ describe('handleAuthValidationFailure', () => { expect(result).toBe(false); }); + + it('should write the failure diagnostic to stderr so piped stdout stays clean', async () => { + const { isNonInteractiveEnvironment } = await import('../../../utils/interactive.js'); + isNonInteractiveEnvironmentMock = isNonInteractiveEnvironment as ReturnType; + isNonInteractiveEnvironmentMock.mockReturnValue(true); + const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const { handleAuthValidationFailure } = await import('../auth-validation.js'); + const setupSteps = { promptForReauth: promptForReauthSpy } as unknown as ProviderSetupSteps; + const validationResult: AuthValidationResult = { valid: false, error: 'session expired' }; + + await handleAuthValidationFailure(validationResult, setupSteps, testConfig); + + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('session expired')); + expect(consoleLogSpy).not.toHaveBeenCalled(); + }); }); diff --git a/src/providers/core/auth-validation.ts b/src/providers/core/auth-validation.ts index 0dff91e5b..79c15def8 100644 --- a/src/providers/core/auth-validation.ts +++ b/src/providers/core/auth-validation.ts @@ -35,7 +35,9 @@ export async function handleAuthValidationFailure( return await setupSteps.promptForReauth(config); } - // No re-auth available (or no TTY to prompt on), show full error with instructions - console.log(chalk.red(`\n✗ ${validationResult.error}\n`)); + // No re-auth available (or no TTY to prompt on), show full error with + // instructions. Diagnostics go to stderr so piped stdout and --json + // consumers stay clean (EPMCDME-14148). + console.error(chalk.red(`\n✗ ${validationResult.error}\n`)); return false; } diff --git a/src/utils/__tests__/auth.test.ts b/src/utils/__tests__/auth.test.ts index 9133c53e4..acad79fec 100644 --- a/src/utils/__tests__/auth.test.ts +++ b/src/utils/__tests__/auth.test.ts @@ -87,7 +87,7 @@ describe('Auth Utilities', () => { ); }); - it('should throw error if re-authentication fails', async () => { + it('should preserve the actionable setup message if re-authentication fails', async () => { const authError = new ConfigurationError('SSO authentication required. Please run "codemie setup" with SSO provider first.'); getCodemieClient.mockRejectedValue(authError); @@ -101,7 +101,12 @@ describe('Auth Utilities', () => { const { getAuthenticatedClient } = await import('../auth.js'); - await expect(getAuthenticatedClient(mockConfig)).rejects.toThrow(ConfigurationError); + // The generic 'Authentication expired' throw inside promptReauthentication + // must not shadow the upstream message that names the remediation. + const thrown = await getAuthenticatedClient(mockConfig).catch((error: unknown) => error); + + expect(thrown).toBeInstanceOf(ConfigurationError); + expect((thrown as ConfigurationError).message).toContain('codemie setup'); expect(getCodemieClient).toHaveBeenCalledTimes(1); }); diff --git a/src/utils/__tests__/sdk-client.test.ts b/src/utils/__tests__/sdk-client.test.ts new file mode 100644 index 000000000..62bb451a5 --- /dev/null +++ b/src/utils/__tests__/sdk-client.test.ts @@ -0,0 +1,79 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +const oraInstance = { + text: '', + start: vi.fn(), + succeed: vi.fn(), + fail: vi.fn(), +}; +const oraFactory = vi.fn(() => { + oraInstance.start.mockReturnValue(oraInstance); + return oraInstance; +}); + +vi.mock('ora', () => ({ default: oraFactory })); + +vi.mock('../interactive.js', () => ({ + isNonInteractiveEnvironment: vi.fn(), +})); + +vi.mock('../config.js', () => ({ + ConfigLoader: { load: vi.fn() }, +})); + +vi.mock('../logger.js', () => ({ + logger: { error: vi.fn(), debug: vi.fn(), warn: vi.fn(), info: vi.fn() }, +})); + +const getStoredCredentials = vi.fn(); +vi.mock('../../providers/plugins/sso/sso.auth.js', () => ({ + CodeMieSSO: class { + getStoredCredentials = getStoredCredentials; + }, +})); + +describe('getCodemieClient spinner behaviour', () => { + beforeEach(() => { + oraFactory.mockClear(); + oraInstance.start.mockClear(); + getStoredCredentials.mockReset(); + }); + + afterEach(() => { + vi.resetModules(); + }); + + it('does not start a spinner when the environment is non-interactive', async () => { + const { isNonInteractiveEnvironment } = await import('../interactive.js'); + const { ConfigLoader } = await import('../config.js'); + const { ConfigurationError } = await import('../errors.js'); + + vi.mocked(isNonInteractiveEnvironment).mockReturnValue(true); + vi.mocked(ConfigLoader.load).mockResolvedValue({ + codeMieUrl: 'https://example.test', + } as never); + getStoredCredentials.mockResolvedValue(null); + + const { getCodemieClient } = await import('../sdk-client.js'); + + await expect(getCodemieClient()).rejects.toThrow(ConfigurationError); + expect(oraFactory).not.toHaveBeenCalled(); + }); + + it('still starts a spinner when interactive and not explicitly quiet', async () => { + const { isNonInteractiveEnvironment } = await import('../interactive.js'); + const { ConfigLoader } = await import('../config.js'); + const { ConfigurationError } = await import('../errors.js'); + + vi.mocked(isNonInteractiveEnvironment).mockReturnValue(false); + vi.mocked(ConfigLoader.load).mockResolvedValue({ + codeMieUrl: 'https://example.test', + } as never); + getStoredCredentials.mockResolvedValue(null); + + const { getCodemieClient } = await import('../sdk-client.js'); + + await expect(getCodemieClient()).rejects.toThrow(ConfigurationError); + expect(oraFactory).toHaveBeenCalled(); + }); +}); diff --git a/src/utils/auth.ts b/src/utils/auth.ts index ff542f660..7c4e1f27d 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -44,7 +44,15 @@ export async function getAuthenticatedClient(config: ProviderProfile): Promise { + // A spinner with no TTY emits raw cursor-control escapes into captured + // output, so suppress it in non-interactive runs (EPMCDME-14148). + const showProgress = !quiet && !isNonInteractiveEnvironment(); + let spinner; - if (!quiet) { + if (showProgress) { spinner = ora('Loading configuration...').start(); } From d77128ac189661d4693896978bb500825a28f200 Mon Sep 17 00:00:00 2001 From: SleepySML Date: Thu, 3 Sep 2026 18:14:46 +0300 Subject: [PATCH 02/16] fix(cli): add process-level error guards and document non-interactive auth Commander actions are async but program.parse() is synchronous, so a rejection escaping an action reached neither the action's own try/catch nor the import().catch() in bin/codemie.js, and Node printed a raw stack. installProcessGuards() is the last-line-of-defence net; commands are still expected to handle their own errors. The guard lives in src/utils/ rather than inline in bin/ because bin/ is excluded from coverage and cannot be unit-tested. AUTHENTICATION.md now shows the message the CLI actually emits, rather than an approximation, and documents that diagnostics go to stderr and that spinners are suppressed without a TTY. Adds an end-to-end regression test asserting the acceptance criterion directly: non-zero exit, remediation text present, no stack trace. Verified honest by reverting the fix, where the remediation assertion fails while the others still pass. EPMCDME-14148 Co-Authored-By: Claude --- bin/codemie.js | 5 ++ docs/AUTHENTICATION.md | 17 +++- src/utils/__tests__/process-guards.test.ts | 86 +++++++++++++++++++ src/utils/process-guards.ts | 39 +++++++++ .../cli-commands/non-interactive-auth.test.ts | 57 ++++++++++++ 5 files changed, 201 insertions(+), 3 deletions(-) create mode 100644 src/utils/__tests__/process-guards.test.ts create mode 100644 src/utils/process-guards.ts create mode 100644 tests/integration/cli-commands/non-interactive-auth.test.ts diff --git a/bin/codemie.js b/bin/codemie.js index b52d7df8c..7336f2988 100755 --- a/bin/codemie.js +++ b/bin/codemie.js @@ -7,6 +7,11 @@ import { MigrationRunner } from '../dist/migrations/index.js'; import { checkAndPromptForUpdate } from '../dist/utils/cli-updater.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// program.parse() is sync, so those never reach the import().catch() below. +installProcessGuards(); // Auto-run pending migrations (happens at startup) // Migrations are tracked in ~/.codemie/migrations.json and only run once diff --git a/docs/AUTHENTICATION.md b/docs/AUTHENTICATION.md index 29f08051a..40ab2ef5c 100644 --- a/docs/AUTHENTICATION.md +++ b/docs/AUTHENTICATION.md @@ -106,9 +106,20 @@ codemie setup # Run wizard again When SSO credentials are missing or expired, the CLI normally offers an interactive re-authentication prompt. In a non-interactive environment — no TTY attached to `stdin`, as in CI pipelines, cron jobs, or piped/redirected invocations — that prompt is automatically skipped. The -CLI detects the missing TTY, fails fast with a clear message (e.g. "No valid SSO credentials found. -Please run `codemie setup` interactively before using this command."), and exits non-zero instead of -hanging. +CLI detects the missing TTY, fails fast, and exits non-zero instead of hanging. + +The failure is a single actionable line on **stderr**, with no stack trace: + +```console +$ codemie sdk assistants list < /dev/null +❌ SSO authentication required. Please run "codemie setup" with SSO provider first. +$ echo $? +1 +``` + +Diagnostics go to stderr rather than stdout, so piping stdout or consuming `--json` output stays +clean. Progress spinners are suppressed when no TTY is attached, so captured logs do not fill with +cursor-control escape sequences. There is no separate `--non-interactive` flag to set — detection is automatic, based solely on whether `stdin` is a TTY. To run unattended in CI, either authenticate ahead of time diff --git a/src/utils/__tests__/process-guards.test.ts b/src/utils/__tests__/process-guards.test.ts new file mode 100644 index 000000000..13bc99734 --- /dev/null +++ b/src/utils/__tests__/process-guards.test.ts @@ -0,0 +1,86 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +vi.mock('../logger.js', () => ({ + logger: { error: vi.fn(), debug: vi.fn(), warn: vi.fn(), info: vi.fn() }, +})); + +type Handler = (payload: unknown) => void; + +describe('installProcessGuards', () => { + let handlers: Record; + let exitCode: number | undefined; + let stderr: string[]; + + beforeEach(() => { + handlers = {}; + exitCode = undefined; + stderr = []; + + vi.spyOn(process, 'on').mockImplementation((( + event: string, + handler: Handler + ) => { + handlers[event] = handler; + return process; + }) as never); + + vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + exitCode = code; + throw new Error(`process.exit:${code}`); + }) as never); + + vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + stderr.push(args.join(' ')); + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.resetModules(); + }); + + it('registers guards for both unhandledRejection and uncaughtException', async () => { + const { installProcessGuards } = await import('../process-guards.js'); + + installProcessGuards(); + + expect(handlers.unhandledRejection).toBeTypeOf('function'); + expect(handlers.uncaughtException).toBeTypeOf('function'); + }); + + it('reports an unhandled rejection without a stack trace and exits non-zero', async () => { + const { installProcessGuards } = await import('../process-guards.js'); + installProcessGuards(); + + const boom = new Error('credentials unavailable'); + + expect(() => handlers.unhandledRejection(boom)).toThrow('process.exit:1'); + expect(exitCode).toBe(1); + + const output = stderr.join('\n'); + expect(output).toContain('credentials unavailable'); + // The guide forbids stack traces on the console; they belong in the log file. + expect(output).not.toContain('at '); + }); + + it('reports an uncaught exception without a stack trace and exits non-zero', async () => { + const { installProcessGuards } = await import('../process-guards.js'); + installProcessGuards(); + + expect(() => handlers.uncaughtException(new Error('boom'))).toThrow( + 'process.exit:1' + ); + expect(exitCode).toBe(1); + expect(stderr.join('\n')).not.toContain('at '); + }); + + it('handles a non-Error rejection reason without crashing', async () => { + const { installProcessGuards } = await import('../process-guards.js'); + installProcessGuards(); + + expect(() => handlers.unhandledRejection('plain string reason')).toThrow( + 'process.exit:1' + ); + expect(stderr.join('\n')).toContain('plain string reason'); + }); +}); diff --git a/src/utils/process-guards.ts b/src/utils/process-guards.ts new file mode 100644 index 000000000..effc0d239 --- /dev/null +++ b/src/utils/process-guards.ts @@ -0,0 +1,39 @@ +/** + * Process-level error guards + * + * Commander actions are async, and `program.parse()` is synchronous, so a + * rejection escaping an action reaches neither the action's try/catch nor the + * import().catch() in bin/codemie.js. Without a net, Node's default handler + * prints a raw stack trace. These guards are the last line of defence + * (EPMCDME-14148); commands should still handle their own errors. + */ + +import chalk from 'chalk'; +import { getErrorMessage } from './errors.js'; +import { logger } from './logger.js'; + +function reportFatal(kind: string, payload: unknown): never { + const message = getErrorMessage(payload); + + // Full detail, stack included, goes to the log file only. + logger.error(`${kind}: ${message}`, { + stack: payload instanceof Error ? payload.stack : undefined, + }); + + console.error(chalk.red(`\n❌ ${message}\n`)); + process.exit(1); +} + +/** + * Register process-level handlers for unhandled rejections and uncaught + * exceptions so they surface as a formatted message rather than a stack trace. + */ +export function installProcessGuards(): void { + process.on('unhandledRejection', (reason: unknown) => { + reportFatal('Unhandled rejection', reason); + }); + + process.on('uncaughtException', (error: unknown) => { + reportFatal('Uncaught exception', error); + }); +} diff --git a/tests/integration/cli-commands/non-interactive-auth.test.ts b/tests/integration/cli-commands/non-interactive-auth.test.ts new file mode 100644 index 000000000..a13949343 --- /dev/null +++ b/tests/integration/cli-commands/non-interactive-auth.test.ts @@ -0,0 +1,57 @@ +/** + * EPMCDME-14148 — non-interactive SSO failure must fail cleanly. + * + * End-to-end proof of the acceptance criterion: with no valid SSO session and + * a non-TTY stdin, the CLI exits non-zero with actionable remediation and + * without a raw stack trace. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { CLIRunner } from '../../helpers/cli-runner.js'; + +describe('non-interactive SSO auth failure', () => { + const runner = new CLIRunner(); + let isolatedHome: string; + + beforeAll(() => { + // An empty home guarantees "no valid SSO session" without touching the + // developer's real ~/.codemie credentials. + isolatedHome = mkdtempSync(join(tmpdir(), 'codemie-14148-')); + }); + + afterAll(() => { + rmSync(isolatedHome, { recursive: true, force: true }); + }); + + function runWithoutTty(command: string) { + const result = runner.runSilent(command, { + env: { ...process.env, CODEMIE_HOME: isolatedHome }, + // stdin from 'ignore' is not a TTY, which is the condition under test. + stdio: ['ignore', 'pipe', 'pipe'], + }); + return { ...result, combined: `${result.output}\n${result.error ?? ''}` }; + } + + it('exits non-zero instead of hanging on a re-authentication prompt', () => { + const result = runWithoutTty('sdk assistants list'); + + expect(result.exitCode).not.toBe(0); + }); + + it('names the remediation the user should run', () => { + const result = runWithoutTty('sdk assistants list'); + + expect(result.combined).toMatch(/codemie setup/); + }); + + it('does not print a raw stack trace', () => { + const result = runWithoutTty('sdk assistants list'); + + expect(result.combined).not.toMatch(/^\s+at\s+/m); + expect(result.combined).not.toContain('ConfigurationError:'); + expect(result.combined).not.toMatch(/Node\.js v\d/); + }); +}); From 7ebcfb207e0d65858b2b4992504c388aa7f7bcba Mon Sep 17 00:00:00 2001 From: SleepySML Date: Thu, 3 Sep 2026 20:00:55 +0300 Subject: [PATCH 03/16] fix(cli): address code review findings CR-001 through CR-005 CR-001: process-guards destroyed the diagnostic it promised to relocate. logger.error's second parameter is only unpacked when it is instanceof Error, so passing { stack } stringified to "[object Object]" and the stack was lost; the process.exit(1) that follows could also drop the entry entirely on a cold write stream. The payload is now passed through, the fatal detail is additionally appended synchronously, and exitCode is set before anything that might exit early. Adds a companion test that does NOT mock the logger and asserts the stack reaches the file - verified to fail against the previous implementation. CR-003: the guard was wired into 1 of 14 bin/ entrypoints, missing the agent binaries whose AgentCLI path carries the SSO auth failure. It is now installed from the AgentCLI constructor, and made idempotent since both entrypoints can share a process. CR-004: AUTHENTICATION.md asserted stdin-TTY detection was sufficient for CI. It is not - a pty-allocating runner still reaches the prompt, which is the originally reported failure. The boundary is now stated explicitly rather than implied away. CR-005: the AC1 regression test could not fail on a hang, because runSilent wraps execSync and Vitest's testTimeout cannot interrupt a synchronous call. Adds an execSync timeout, asserts the remediation verbatim instead of a loose match, and adds the missing assertion that the diagnostic stays off stdout. CR-002 is a documentation correction, committed separately with the planning artifacts. EPMCDME-14148 Co-Authored-By: Claude --- docs/AUTHENTICATION.md | 12 +++- src/agents/core/AgentCLI.ts | 6 ++ .../__tests__/process-guards.logfile.test.ts | 66 +++++++++++++++++++ src/utils/__tests__/process-guards.test.ts | 30 +++++++++ src/utils/process-guards.ts | 53 +++++++++++++-- .../cli-commands/non-interactive-auth.test.ts | 18 ++++- 6 files changed, 178 insertions(+), 7 deletions(-) create mode 100644 src/utils/__tests__/process-guards.logfile.test.ts diff --git a/docs/AUTHENTICATION.md b/docs/AUTHENTICATION.md index 40ab2ef5c..31eb75447 100644 --- a/docs/AUTHENTICATION.md +++ b/docs/AUTHENTICATION.md @@ -122,10 +122,18 @@ clean. Progress spinners are suppressed when no TTY is attached, so captured log cursor-control escape sequences. There is no separate `--non-interactive` flag to set — detection is automatic, based solely on -whether `stdin` is a TTY. To run unattended in CI, either authenticate ahead of time +whether `stdin` is a TTY. + +> **Known limitation.** Because detection looks only at `stdin`, it does **not** fire in an +> environment that allocates a pseudo-TTY — `docker run -t`, and some Jenkins and GitLab runner +> configurations. There, a missing SSO session can still reach the interactive prompt and block. The +> `CI` environment variable is not consulted. If your runner allocates a TTY, do not rely on +> automatic detection; use one of the two unattended options below. + +To run unattended in CI, either authenticate ahead of time (`codemie profile login`) with credentials persisted before the run, or use [JWT Bearer Authorization](#jwt-bearer-authorization) instead, which requires no interactive -session at all. +session at all. The JWT path never prompts, so it is the safest choice for a pty-allocating runner. ## Enterprise SSO Features diff --git a/src/agents/core/AgentCLI.ts b/src/agents/core/AgentCLI.ts index e79f8b747..3a8e1ecc2 100644 --- a/src/agents/core/AgentCLI.ts +++ b/src/agents/core/AgentCLI.ts @@ -10,6 +10,7 @@ import { AuthMethod, ProviderName } from '../../providers/core/types.js'; import { JWTTemplate } from '../../providers/plugins/jwt/jwt.template.js'; import { logger } from '../../utils/logger.js'; import { getDirname } from '../../utils/paths.js'; +import { installProcessGuards } from '../../utils/process-guards.js'; import { BUILTIN_AGENT_NAME } from '../registry.js'; import { ClaudePluginMetadata } from '../plugins/claude/claude.plugin.js'; import { CodeMieCodePluginMetadata } from '../plugins/codemie-code.plugin.js'; @@ -36,6 +37,11 @@ export class AgentCLI { private version: string = '1.0.0'; constructor(private adapter: AgentAdapter) { + // Every bin/codemie- entrypoint reaches the CLI through here rather + // than bin/codemie.js, so the guards are installed here to cover them too + // (EPMCDME-14148). installProcessGuards() is idempotent. + installProcessGuards(); + this.program = new Command(); this.loadVersion(); this.setupProgram(); diff --git a/src/utils/__tests__/process-guards.logfile.test.ts b/src/utils/__tests__/process-guards.logfile.test.ts new file mode 100644 index 000000000..60e00ef8a --- /dev/null +++ b/src/utils/__tests__/process-guards.logfile.test.ts @@ -0,0 +1,66 @@ +/** + * Companion to process-guards.test.ts, which mocks the logger wholesale and so + * cannot catch a broken logging contract. This file uses the REAL logger and + * asserts the stack actually reaches the log file — the failure mode that + * shipped undetected in the first round (CR-001). + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { readFileSync, existsSync } from 'node:fs'; + +type Handler = (payload: unknown) => void; + +describe('installProcessGuards log-file persistence', () => { + let handlers: Record; + + beforeEach(() => { + handlers = {}; + vi.spyOn(process, 'on').mockImplementation((( + event: string, + handler: Handler + ) => { + handlers[event] = handler; + return process; + }) as never); + + vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + throw new Error(`process.exit:${code}`); + }) as never); + + vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.resetModules(); + }); + + it('writes the stack to the log file even though the console omits it', async () => { + const { logger } = await import('../logger.js'); + const { installProcessGuards } = await import('../process-guards.js'); + + const logPath = logger.getLogFilePath(); + // Logging to file is best-effort; if this environment has no log path there + // is nothing to assert against. + if (!logPath) { + return; + } + + const before = existsSync(logPath) ? readFileSync(logPath, 'utf-8') : ''; + + installProcessGuards(); + + const marker = 'process-guards-logfile-probe'; + const boom = new Error(marker); + + expect(() => handlers.uncaughtException(boom)).toThrow('process.exit:1'); + + const after = readFileSync(logPath, 'utf-8'); + const appended = after.slice(before.length); + + expect(appended).toContain(marker); + // A stack, not just the message — the whole point of relocating it. + expect(appended).toMatch(/\n\s+at\s/); + expect(appended).not.toContain('[object Object]'); + }); +}); diff --git a/src/utils/__tests__/process-guards.test.ts b/src/utils/__tests__/process-guards.test.ts index 13bc99734..fae111731 100644 --- a/src/utils/__tests__/process-guards.test.ts +++ b/src/utils/__tests__/process-guards.test.ts @@ -74,6 +74,36 @@ describe('installProcessGuards', () => { expect(stderr.join('\n')).not.toContain('at '); }); + it('passes the original error to the logger so the stack is preserved', async () => { + const { logger } = await import('../logger.js'); + const { installProcessGuards } = await import('../process-guards.js'); + installProcessGuards(); + + const boom = new Error('credentials unavailable'); + + expect(() => handlers.uncaughtException(boom)).toThrow('process.exit:1'); + + // logger.error only extracts .stack when the 2nd arg is `instanceof Error`; + // wrapping it in an object literal stringifies to "[object Object]". + expect(logger.error).toHaveBeenCalledWith(expect.any(String), boom); + }); + + it('sets a non-zero exitCode before exiting so a premature natural exit still fails', async () => { + const { installProcessGuards } = await import('../process-guards.js'); + installProcessGuards(); + + const original = process.exitCode; + try { + process.exitCode = 0; + expect(() => handlers.uncaughtException(new Error('boom'))).toThrow( + 'process.exit:1' + ); + expect(process.exitCode).toBe(1); + } finally { + process.exitCode = original; + } + }); + it('handles a non-Error rejection reason without crashing', async () => { const { installProcessGuards } = await import('../process-guards.js'); installProcessGuards(); diff --git a/src/utils/process-guards.ts b/src/utils/process-guards.ts index effc0d239..d91da7daa 100644 --- a/src/utils/process-guards.ts +++ b/src/utils/process-guards.ts @@ -8,27 +8,72 @@ * (EPMCDME-14148); commands should still handle their own errors. */ +import { appendFileSync } from 'node:fs'; import chalk from 'chalk'; import { getErrorMessage } from './errors.js'; import { logger } from './logger.js'; +import { sanitizeLogArgs } from './security.js'; + +/** + * Append the fatal detail synchronously. + * + * logger writes through an fs.WriteStream, whose write() is asynchronous; + * process.exit() does not drain it, so on a cold stream the entry is lost + * entirely. A fatal is exactly when the record matters most. + */ +function persistFatalSync(kind: string, payload: unknown): void { + try { + const logPath = logger.getLogFilePath(); + if (!logPath) { + return; + } + + const detail = + payload instanceof Error && payload.stack + ? payload.stack + : getErrorMessage(payload); + const [safeDetail] = sanitizeLogArgs(detail); + + appendFileSync( + logPath, + `[${new Date().toISOString()}] [FATAL] ${kind}: ${String(safeDetail)}\n` + ); + } catch { + // A logging failure must never mask the original fatal. + } +} function reportFatal(kind: string, payload: unknown): never { const message = getErrorMessage(payload); - // Full detail, stack included, goes to the log file only. - logger.error(`${kind}: ${message}`, { - stack: payload instanceof Error ? payload.stack : undefined, - }); + // Set first: if anything below exits early, the code is still non-zero. + process.exitCode = 1; + + // Pass the payload itself — logger extracts .message/.stack only from a real + // Error; an object literal would stringify to "[object Object]". + logger.error(`${kind}: ${message}`, payload); + persistFatalSync(kind, payload); + // Console gets the actionable line only; the stack belongs in the log file. console.error(chalk.red(`\n❌ ${message}\n`)); process.exit(1); } +let installed = false; + /** * Register process-level handlers for unhandled rejections and uncaught * exceptions so they surface as a formatted message rather than a stack trace. + * + * Idempotent: both bin/codemie.js and AgentCLI call it, and they can share a + * process, which would otherwise stack duplicate handlers. */ export function installProcessGuards(): void { + if (installed) { + return; + } + installed = true; + process.on('unhandledRejection', (reason: unknown) => { reportFatal('Unhandled rejection', reason); }); diff --git a/tests/integration/cli-commands/non-interactive-auth.test.ts b/tests/integration/cli-commands/non-interactive-auth.test.ts index a13949343..790f0fcbb 100644 --- a/tests/integration/cli-commands/non-interactive-auth.test.ts +++ b/tests/integration/cli-commands/non-interactive-auth.test.ts @@ -26,11 +26,18 @@ describe('non-interactive SSO auth failure', () => { rmSync(isolatedHome, { recursive: true, force: true }); }); + const EXPECTED_MESSAGE = + 'SSO authentication required. Please run "codemie setup" with SSO provider first.'; + function runWithoutTty(command: string) { const result = runner.runSilent(command, { env: { ...process.env, CODEMIE_HOME: isolatedHome }, // stdin from 'ignore' is not a TTY, which is the condition under test. stdio: ['ignore', 'pipe', 'pipe'], + // runSilent wraps execSync, which blocks the worker synchronously — + // Vitest's testTimeout cannot interrupt it. Without this, a regression to + // the original hang would wedge CI instead of failing here. + timeout: 15_000, }); return { ...result, combined: `${result.output}\n${result.error ?? ''}` }; } @@ -44,7 +51,16 @@ describe('non-interactive SSO auth failure', () => { it('names the remediation the user should run', () => { const result = runWithoutTty('sdk assistants list'); - expect(result.combined).toMatch(/codemie setup/); + // Asserted verbatim: a loose /codemie setup/ match is also satisfied by + // unrelated setup advice from other failure paths. + expect(result.combined).toContain(EXPECTED_MESSAGE); + }); + + it('sends the diagnostic to stderr and keeps it off stdout', () => { + const result = runWithoutTty('sdk assistants list'); + + expect(result.error ?? '').toContain(EXPECTED_MESSAGE); + expect(result.output).not.toContain(EXPECTED_MESSAGE); }); it('does not print a raw stack trace', () => { From db9d403bf7082c0777b13b62ae0b2892b678c186 Mon Sep 17 00:00:00 2001 From: SleepySML Date: Thu, 3 Sep 2026 20:01:19 +0300 Subject: [PATCH 04/16] docs(cli): add EPMCDME-14148 planning artifacts and correct AC4 record CR-002: ac4-investigation.md claimed the spinner suppression removed a plausible contributor to AC4. That is false - the suppression is gated on the environment being non-interactive, while AC4's scenario requires a TTY, so it can never fire there. The claim was the only justification offered for shipping without AC4; it is struck and marked as retracted rather than quietly deleted. AC4 is reclassified from "cannot reproduce" to "precondition not constructed, criterion never exercised" - 0 of 8 attempts reached the prompt, so the criterion was never actually tested. Carries the concrete reproduction recipe for the follow-up ticket. Also corrects reproduction.md, which twice called the --non-interactive flag undocumented when its absence is documented in AUTHENTICATION.md. EPMCDME-14148 Co-Authored-By: Claude --- .../ac4-investigation.md | 57 +++ .../code-review-final.json | 116 +++++ .../complexity-assessment.json | 38 ++ .../events.jsonl | 4 + .../reproduction.md | 113 +++++ .../spec.md | 76 ++++ .../technical-analysis.md | 407 ++++++++++++++++++ 7 files changed, 811 insertions(+) create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-final.json create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/complexity-assessment.json create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/spec.md create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/technical-analysis.md diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md new file mode 100644 index 000000000..32c8abd7c --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md @@ -0,0 +1,57 @@ +# AC4 — `ERR_USE_AFTER_CLOSE` on kill: investigation result + +**Acceptance criterion:** "Killing during prompt does not produce readline lifecycle crash." + +**Verdict: precondition not constructed — the criterion was never exercised.** + +This is deliberately *not* "cannot reproduce". Every attempt exited before the interactive prompt was reached, so the scenario the criterion describes — killing the process *while it sits on the prompt* — was never actually set up. Ten attempts at failing to reach the prompt is zero attempts at the criterion. "Cannot reproduce the crash" and "cannot construct the precondition" are different claims, and only the second is supported by what follows. + +## What was run + +A node-pty probe (`ac4-probe.mjs`, throwaway) driving a **real** pty — not `script`, whose artifacts are discussed below. Isolated `CODEMIE_HOME` per attempt, with the real config copied in (never credentials) so the `sso` provider resolves. + +8 attempts: 4 interrupt modes (`Ctrl-C` as raw `\x03`, `SIGINT`, `SIGTERM`, `SIGHUP`) × 2 delays (1.5 s, 4 s). + +| Result | Across all 8 | +|---|---| +| `ERR_USE_AFTER_CLOSE` occurrences | **0** | +| Any `readline` mention | **0** | +| Reached the re-auth prompt | **0** | +| Exit | `code=1, signal=0` every time | +| Max output | 1 470 bytes | + +Plus the 2 earlier `script`-based attempts in `reproduction.md`. **10 attempts total, zero hits.** + +## Why the prompt was never reached + +Every attempt exited cleanly in well under 1.5 s — before any signal was delivered. The process reaches `No valid SSO credentials found` and terminates with the actionable message. `promptReauthentication`'s interactive branch is never entered in a clean-room home, so "kill *during* the prompt" was never actually exercised. + +Reproducing AC4 would need an environment where `promptForReauth` is genuinely reached — most plausibly a home with *stale but present* credentials for a matching URL, rather than absent ones. That is the reporter's environment, which this investigation could not recreate from the ticket text. + +## Two earlier observations retracted + +Both were `script`-harness artifacts, not product defects. Recording them so nobody re-derives them: + +**1. The "73 MB ora escape-sequence flood" is not real.** Under a real pty the same command produces **1 470 bytes**. The flood only appears under `script`, consistent with the original guess that `script` yields a pty with no usable `stdout.columns`, breaking ora's line-clearing arithmetic. + +**2. The "Case C TTY hang" is not the re-auth prompt.** Originally read as proof that attaching a TTY parks execution on the prompt. Three facts refute it: + +- The identical `script` invocation against the **fixed** build still hangs 25 s. +- `script` wrapping an immediately-exiting child returns in 0 s, so `script` does not hang unconditionally. +- The `script` capture stalls at `⠋ Loading configuration...`, *before* credentials are read, and contains a stray `^D` — the signature of an immediately-EOF stdin being forwarded into the pty. + +Under node-pty the same command completes in under 1 s. The non-TTY guard is still confirmed working — by `auth-validation.test.ts` and by Cases A/B exiting in 0–1 s without prompting — but **not** by Case C. + +**Methodological note:** `script(1)` is unsuitable for CLI behaviour testing here. It injects its own stdin and terminal-geometry behaviour. Use `node-pty` (already a dependency, wrapped by `tests/helpers/pty-session.ts`) for anything TTY-dependent. + +## Recommendation + +Split AC4 into its own ticket. Do **not** close it against this MR — neither as satisfied nor as "not reproducible", since the criterion was never exercised. + +Carry this reproduction recipe over, which the code review identified and this investigation stopped one step short of building: + +> `sso.setup-steps.ts` `validateAuth` returns `{valid: false, error: 'API access test failed: …'}` whenever `fetchCodeMieModels` throws, and that result reaches `promptForReauth`'s `inquirer.prompt`. So: plant a credentials file whose `apiUrl` points at a **closed local port**. `validateAuth` then fails deterministically with no network dependency, the prompt *is* reached, and `tests/helpers/pty-session.ts` can drive a signal into it. The missing precondition is stale-but-present credentials — not absent ones, which is all this investigation ever tested. + +**Retracted claim.** An earlier draft of this document argued that "the spinner suppression in this MR removes one plausible contributor (a spinner writing to a torn-down TTY)". **That is false and has been struck.** The suppression added in `sdk-client.ts` is gated on `isNonInteractiveEnvironment()`, so it fires only when **no** TTY is attached — while AC4's scenario requires a TTY by definition. It can never fire there. The spinner implicated in the original escape-sequence observation is the one inside `promptForReauth` (`sso.setup-steps.ts:294`), which this MR does not touch. That sentence was the sole justification offered for shipping without AC4, and it did not survive inspection; nothing in this MR mitigates AC4, partially or otherwise. + +The one part of the original conclusion that stands: **do not ship a speculative fix** for a failure mode with no reproduction. diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-final.json b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-final.json new file mode 100644 index 000000000..81a60d626 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-final.json @@ -0,0 +1,116 @@ +{ + "decision": "request-changes", + "rationale": "All three lenses ran. The core two-part fix (getSdkClient wrap + auth.ts message preservation) is sound and independently verified end-to-end by the acceptance lens, and AC2 holds CLI-wide rather than only for the sdk family. Blocking issues are concentrated in the adjacent scope items rather than the core fix: the process-guards module destroys the diagnostic it promises to relocate (stack stringifies to [object Object]; the log entry can be dropped entirely by the exit race), it is wired into 1 of 14 bin entrypoints and not the agent binaries where the auth path actually lives, the AC1 regression test cannot fail on a hang because execSync receives no timeout, and the documentation overstates detection as stdin-TTY-only without naming the pty-allocating-CI blind spot that is the original reported failure. Separately, ac4-investigation.md contains one unearned mitigation claim that inspection refutes, and AC4 was never actually exercised (0 of 8 attempts reached the prompt), so it must be reclassified rather than closed as not-reproducible. 8 hypotheses were refuted with evidence and dropped; 6 minor findings deferred (spinner gates on stdin rather than the output stream, discarded re-auth error has no debug breadcrumb, defensive throw after handleSdkError, ConfigLoader.load sharing the 'authenticate' error label, brittle 'at ' assertion, reproduction.md's stale 'undocumented' wording). Confidence is low because AC4's disposition is a product decision, not an implementation one.", + "confidence": "low", + "risk_flags": ["auth", "observability", "acceptance-criteria-unmet"], + "business_review": [ + { + "id": "AC1", + "criterion": "Non-TTY stdin skips interactive prompt.", + "status": "pass", + "notes": "Verified against git, not just docs: PR #471 (5b2de4b7) introduced the guard and this MR does not touch the condition. Empirically confirmed at 1.4s, exit 1, no prompt. But see CR-005 - the new regression test cannot fail on a hang." + }, + { + "id": "AC2", + "criterion": "CLI exits non-zero with clear remediation.", + "status": "pass", + "notes": "Both halves implemented and both load-bearing. Independently reproduced: stderr carries exactly the actionable line, exit 1, no 'at ' frames, no ConfigurationError prefix, no Node banner. Acceptance lens confirmed the auth.ts fix also improves assistants/chat, assistants/setup and skills/setup, so AC2 holds CLI-wide." + }, + { + "id": "AC3", + "criterion": "Optional --non-interactive or --ci behavior is supported or documented.", + "status": "partial", + "notes": "Citing existing documentation is a legitimate reading of 'supported OR documented', and the pre-existing text was confirmed to predate this MR. Not a clean pass because the documentation asserts detection is 'based solely on whether stdin is a TTY' without naming the blind spot: a pty-allocating CI runner or docker run -t still reaches the prompt. See CR-006." + }, + { + "id": "AC4", + "criterion": "Killing during prompt does not produce readline lifecycle crash.", + "status": "fail", + "notes": "No code addresses it and no test asserts it. Decisively, the investigation's own table records 'Reached the re-auth prompt: 0' across all 8 pty attempts - the criterion was never exercised. 'Cannot reproduce the crash' and 'cannot construct the precondition' are different claims and only the second is supported. See CR-002." + } + ], + "standards_review": [ + { + "standard": "git-workflow.md - Conventional Commits, scope-enum", + "status": "pass", + "notes": "Both commits use fix(cli), which is in scope-enum. fix(auth)/fix(sdk) correctly avoided." + }, + { + "standard": "code-quality.md - ESLint --max-warnings=0", + "status": "pass", + "notes": "Full lint clean. no-useless-catch did not fire; both new catches transform rather than bare-rethrow." + }, + { + "standard": "code-quality.md - explicit return types, .js extensions, no console.log left in code", + "status": "pass", + "notes": "Typecheck clean. New module exports declare return types." + }, + { + "standard": "testing-patterns.md - unit tests co-located in src/**/__tests__/", + "status": "pass", + "notes": "All three new unit suites are under src/**/__tests__/, avoiding the tests/unit/ dead zone where five files match no vitest glob." + }, + { + "standard": "development-practices.md - do not log stack traces to console; log errors with logger.error", + "status": "fail", + "notes": "Console side is correct (no stack printed). The logger side is broken: the stack never reaches the log file. See CR-001." + } + ], + "findings": [ + { + "id": "CR-001", + "severity": "major", + "triage": "patch", + "file": "src/utils/process-guards.ts", + "line": "16-24", + "problem": "reportFatal calls logger.error(msg, { stack: payload.stack }) with a plain object. logger.error only extracts .stack when the second argument is instanceof Error, so it falls through to String(error) and writes '[object Object]'. Independently confirmed by both the edge-case and acceptance lenses against a real build. Compounding it, process.exit(1) fires before the logger's fs.WriteStream flushes its first write, so on a cold stream the entry is dropped entirely and the logs directory stays empty.", + "impact": "The guard removes the stack from stderr (intended) but then fails to put it anywhere else, so for the exact class of unexpected failure this module exists to report, diagnostic information is destroyed rather than relocated. The module's own header comment asserts the opposite. The new unit tests cannot catch this because they mock ../logger.js wholesale and assert only on stderr.", + "recommendation": "Pass the payload straight through: logger.error(`${kind}: ${message}`, payload) - logger already handles both Error and non-Error. For the flush race, use a synchronous write for the fatal entry or set process.exitCode and let the loop drain. Add one test that does not mock the logger and asserts the stack reaches the file.", + "sources": ["edge-case EC-2", "edge-case EC-3", "acceptance"] + }, + { + "id": "CR-002", + "severity": "major", + "triage": "decision_needed", + "file": "docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md", + "line": "47", + "problem": "The document justifies shipping without an AC4 fix by claiming the spinner suppression 'removes one plausible contributor (a spinner writing to a torn-down TTY)'. That is provably false: the suppression is gated on isNonInteractiveEnvironment(), so it only takes effect when NO TTY is attached, while AC4's scenario requires a TTY by definition. The guard can never fire in it. The spinner implicated in the original escape-sequence observation is the one inside promptForReauth (sso.setup-steps.ts:294), which this MR does not touch.", + "impact": "This is the single piece of reasoning offered for why shipping without addressing AC4 is acceptable, and it does not survive inspection - it lends the closure a false appearance of partial mitigation. Separately, AC4 was never exercised at all: 0 of 8 pty attempts reached the prompt, so 'not reproducible' overstates what was established.", + "recommendation": "Strike the sentence from ac4-investigation.md and from the MR description. Reclassify AC4 as 'precondition not constructed, criterion not exercised' and split it to a follow-up ticket carrying the concrete recipe the acceptance lens supplied: plant SSO credentials whose apiUrl points at a closed local port so validateAuth fails deterministically with no network dependency, then drive tests/helpers/pty-session.ts and signal during the prompt. This is a product call on whether 14148 can close with AC4 unmet.", + "sources": ["acceptance"] + }, + { + "id": "CR-003", + "severity": "major", + "triage": "patch", + "file": "bin/codemie.js", + "line": "10-14", + "problem": "installProcessGuards() is wired into 1 of 14 bin/ entrypoints. agent-executor.js, codemie-claude.js, codemie-codex.js, codemie-gemini.js, codemie-kimi*.js, codemie-opencode.js, codemie-pi.js, codemie-openwiki.js, codemie-copilot.js, proxy-daemon.js are all unguarded - and per this branch's own technical-analysis.md, AgentCLI.handleRun is the entry point every agent binary uses and is where the SSO auth-failure path lives.", + "impact": "The commit message and docs imply the clean-failure behavior is universal. It is absent from precisely the binaries that carry the primary auth path, so behavior is now inconsistent across entrypoints.", + "recommendation": "Call installProcessGuards() from the shared AgentCLI constructor/run path so all agent binaries inherit it, or add the import to each bin/* entrypoint. Narrow the docs claim accordingly.", + "sources": ["edge-case EC-4", "blind"] + }, + { + "id": "CR-004", + "severity": "major", + "triage": "patch", + "file": "docs/AUTHENTICATION.md", + "line": "104-124", + "problem": "The page states non-interactive detection is 'based solely on whether stdin is a TTY' and presents that as sufficient for CI. interactive.ts:15 is literally !process.stdin.isTTY with no CI env-var fallback, so CI runners and containers that allocate a pty (docker run -t, several Jenkins/GitLab configurations) still reach the interactive prompt and hang - the precise failure this ticket was filed about. The doc offers no flag, env var, or troubleshooting note.", + "impact": "AC3 is being satisfied by documentation that overstates the guarantee. A CI user following this page can still hit the original hang. The limitation is inherited from PR #471, not introduced here, but this MR is the one asserting completeness.", + "recommendation": "Either honor process.env.CI in isNonInteractiveEnvironment() (a one-line change that would move AC3 from 'documented' to 'supported'), or amend the doc to state the boundary explicitly and point pty-CI users at JWT auth or pre-persisted credentials.", + "sources": ["acceptance", "blind"] + }, + { + "id": "CR-005", + "severity": "major", + "triage": "patch", + "file": "tests/integration/cli-commands/non-interactive-auth.test.ts", + "line": "31-45", + "problem": "The case named 'exits non-zero instead of hanging on a re-authentication prompt' asserts only exitCode !== 0. It routes through CLIRunner.runSilent, which is execSync-based (tests/helpers/cli-runner.ts:44) and receives no timeout option. Because execSync blocks the worker synchronously, Vitest's 30s testTimeout cannot interrupt it.", + "impact": "The AC1 disposition rests on 'already met, covered by regression test', so the safety net is nominal. A change reintroducing the hang would stall CI until the job-level timeout instead of failing with a pointer to the cause - the slowest and most confusing failure mode for exactly this bug.", + "recommendation": "Add timeout: 15_000 to the options passed to runSilent in runWithoutTty and assert on the resulting non-zero/SIGTERM outcome so a hang surfaces as a named test failure. Also tighten the remediation assertion to the exact message and assert stdout is empty, since the stderr-vs-stdout split is a headline doc claim with no test behind it.", + "sources": ["acceptance", "blind"] + } + ] +} diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/complexity-assessment.json b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/complexity-assessment.json new file mode 100644 index 000000000..4a3750c80 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/complexity-assessment.json @@ -0,0 +1,38 @@ +{ + "schema": 1, + "task": "Make the CLI's non-interactive SSO auth failure exit cleanly with actionable remediation by wrapping the shared getSdkClient() choke point and preserving the upstream \"run codemie setup\" message that promptReauthentication currently discards.", + "generated": "2026-09-03T00:00:00Z", + "dimensions": { + "component_scope": { "score": 4, "label": "L" }, + "requirements_clarity": { "score": 4, "label": "L" }, + "technical_risk": { "score": 4, "label": "L" }, + "file_change_estimate": { "score": 3, "label": "M" }, + "dependencies": { "score": 1, "label": "XS" }, + "affected_layers": { "score": 3, "label": "M" } + }, + "total": 19, + "size": "M", + "routing": "brainstorming", + "key_reasoning": [ + { + "dimension": "component_scope", + "reason": "Base M (2-3 components across 2 layers, clear pattern): the fix is two coordinated edits - getSdkClient() at src/cli/commands/sdk/utils/cli-utils.ts:15-18 (CLI) and the message-discarding throw at src/utils/auth.ts:76 (Utils), plus an optional one-line stdout->stderr change in src/providers/core/auth-validation.ts:39 and a mandatory docs/AUTHENTICATION.md:104-117 update. Bumped to L: src/utils/auth.ts is a core shared utility - getSdkClient is the sole bridge for ~50 sdk actions across 8 files, and getAuthenticatedClient/promptReauthentication have four further consumers (assistants/chat, assistants/setup, skills/setup, AgentCLI) that already exit cleanly and must not regress. The 'affects multiple workflows' red flag was considered but NOT applied on top: the shared-gate approach (PR #471 precedent) keeps the edit confined to one function, so the ~50 actions are consumers rather than edit sites." + }, + { + "dimension": "requirements_clarity", + "reason": "Base M (core requirement defined, 1-2 clarifying questions): AC2 - the only live criterion - is unambiguous, and technical-analysis.md has already resolved the implementation path (fix at the choke point + preserve the upstream message; both halves required, neither sufficient alone). Bumped to L: AC3 (--non-interactive/--ci) directly conflicts with a recorded decision - EPMCDME-13953's spec lists it as explicitly out of scope and docs/AUTHENTICATION.md:113 documents the absence as intentional - and its 'supported or documented' wording is arguably already satisfied. AC4 (ERR_USE_AFTER_CLOSE) was not reproduced in 2 attempts and is unproven rather than disproven. Both are scoping/product calls that need resolving before implementation, and the ticket's own premise about the missing try/catch was partly wrong and had to be corrected during research." + }, + { + "dimension": "technical_risk", + "reason": "Base M (some new patterns required, no exact precedent for the message-preservation half). Bumped to L by the 'affects authentication or authorization' red flag - this change sits directly on the SSO auth path. Concrete traps documented in the analysis: ConfigurationError's constructor accepts only `message`, so preserving the upstream error as `cause` means touching a repo-wide base class or a post-hoc assignment; ESLint no-useless-catch is warn under --max-warnings=0, so a naive catch-and-rethrow fails the lint gate; promptReauthentication's declared Promise can only return true or throw, so converting the throw to a `false` return would revive dead code at auth.ts:48/52 and change behaviour at assistants/chat/index.ts:423. Mitigation is strong: every ingredient exists in-repo (isNonInteractiveEnvironment, four never-returning error sinks, and NOT_AUTHENTICATED_MESSAGE in skills/lib/require-auth.ts:15-16 is verbatim the actionable text the ticket asks for), and the change is trivially reversible." + } + ], + "red_flags_applied": [ + "Component Scope bumped from M (3) to L (4): touches core shared utilities - src/utils/auth.ts and the getSdkClient() gate bridging ~50 sdk command actions across 8 files plus 4 adjacent auth call sites.", + "Requirements Clarity bumped from M (3) to L (4): vague/conflicting acceptance criteria - AC3 (--non-interactive/--ci) contradicts EPMCDME-13953's recorded out-of-scope decision and shipped text in docs/AUTHENTICATION.md:113; AC4 is unproven.", + "Technical Risk bumped from M (3) to L (4): affects authentication - the change modifies the SSO auth acquisition and failure-reporting path.", + "NOT applied - 'affects multiple workflows or agents' (second Component Scope bump): the shared-gate fix confines the edit to one function; the ~50 sdk actions inherit the fix as consumers rather than being edited.", + "NOT applied - 'requires data migration' (File Changes bump): no persistence, schema, or migration surface in this change." + ], + "split_recommendation": null +} diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl new file mode 100644 index 000000000..93f640cf5 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl @@ -0,0 +1,4 @@ +{"event":"work_item.adapter_warning","intent":"record_complexity_score","phase":2,"adapter":"codemie-jira-assistant","reason":"Adapter declared configured in .ai-run/guides/project.md but the codemie-jira-assistant skill is not resolvable in this session; no external sync performed.","external_sync":"pending"} +{"event":"lifecycle_emission","intent":"record_complexity_score","assessment_mode":"initial","status":"failed"} +{"event":"work_item.adapter_receipt","intent":"record_complexity_score","phase":2,"adapter":"codemie-jira-assistant","attempt":2,"status":"failed","reason":"Skill tool returned 'Unknown skill: codemie-jira-assistant'. Root cause: the skill is project-scoped at codemie-code/.claude/skills/codemie-jira-assistant, but the session project directory is the parent codemie-dev, so project skills from the repo are not loaded. Not a Jira connectivity problem - Jira MCP is connected and healthy. Deterministic MCP fallback (jira_add_comment) is available but is an outward-facing write and was not performed without explicit authorization.","external_sync":"pending"} +{"event":"lifecycle_emission","intent":"record_complexity_score","assessment_mode":"initial","status":"failed"} diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md new file mode 100644 index 000000000..d6781e936 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md @@ -0,0 +1,113 @@ +# EPMCDME-14148 — Reproduction Report + +**Baseline:** `codemie-code` @ `1d5cc22b` (origin/main, v0.15.0), branch `EPMCDME-14148`, Node v24.19.0, macOS. +**Isolation:** every case ran with `CODEMIE_HOME` pointed at a throwaway directory. The real `~/.codemie` was never modified; its `credentials` store was verified intact afterwards. + +## Verdict + +The ticket describes two symptoms. Only one is still live. + +| Ticket symptom | Status on main | +|---|---| +| "CLI hangs on a re-authentication prompt" (non-TTY) | **Already fixed** by `5b2de4b7` (PR #471) | +| "exits non-zero with an actionable message" | **Still broken** — exits 1 via an *unhandled exception + raw stack trace*, and the message drops the actionable remediation | +| `ERR_USE_AFTER_CLOSE` on kill | **Not reproduced** (see Case D) | +| `--non-interactive` / `--ci` flag | **Does not exist** as a flag, but its absence *is* documented (`docs/AUTHENTICATION.md:113`, added by PR #471) — corrected; an earlier draft of this table wrongly called it undocumented | + +The residual defect is not a hang. It is that the non-interactive failure path terminates by letting a `ConfigurationError` escape to Node's default handler. + +## Cases + +### Case A — no config at all, stdin `< /dev/null` + +``` +CODEMIE_HOME= codemie sdk assistants list < /dev/null +``` + +Exit code **1**, duration **0s** (no hang). + +``` +- Loading configuration... +✖ No valid SSO credentials found +file:///.../dist/utils/auth.js:65 + throw new ConfigurationError('Authentication expired. Please re-authenticate.'); + ^ +ConfigurationError: Authentication expired. Please re-authenticate. + at promptReauthentication (.../dist/utils/auth.js:65:11) + at getAuthenticatedClient (.../dist/utils/auth.js:40:36) + at async Command. (.../dist/cli/commands/sdk/assistants.js:24:24) +Node.js v24.19.0 +``` + +### Case B — valid config present, no SSO credentials, stdin `< /dev/null` + +Matches the ticket's stated precondition more closely (config exists, session does not). Exit code **1**, duration **1s**. Output **identical** to Case A, same stack trace, same line. + +### Case C — attempted control: same command **with** a TTY (pty via `script`) + +Exit code **142** after the 25 s alarm fired. + +**This case was originally read as "blocks on the interactive re-auth prompt". That interpretation is wrong** — corrected after the AC4 investigation (see `ac4-investigation.md`): + +- Re-running the identical `script` harness against the **fixed** build still hangs for 25 s, so the hang is not the defect this ticket fixes. +- A control (`script` wrapping a child that exits immediately) returns in **0 s**, so `script` does not hang unconditionally. +- The captured output stalls at `⠋ Loading configuration...` — *before* credentials are ever read — and contains a stray `^D`. The re-auth prompt is never reached. +- The same command under a real pty (node-pty) exits in **under 1 s** with the correct message. + +Conclusion: the Case C hang is an artifact of the `script` harness (its stdin is an immediately-EOF pipe, which something downstream blocks on), not CLI behaviour. Case C is **not** valid evidence for or against the non-TTY guard. + +The guard is still confirmed working — but by `auth-validation.test.ts` and by Cases A/B exiting in 0–1 s without prompting, not by Case C. + +### Case D — AC4: kill during the prompt + +Ran under a pty, sent `SIGINT` then `SIGTERM` while parked on the prompt. **No `ERR_USE_AFTER_CLOSE` and no readline lifecycle error** appeared across two attempts. + +Incidental observation, **not** confirmed as a product bug: after the interrupt the `ora` spinner emitted a runaway stream of cursor-control escapes (`ESC[1A ESC[0K`), producing a 73 MB capture. This is most likely an artifact of `script` giving the child a pty with no usable `stdout.columns`, which breaks ora's line-clearing arithmetic. It should be re-checked in a real terminal before anyone treats it as a finding. + +The ticket hedges with "**can** crash", so AC4 is plausibly intermittent or environment-specific. It is unproven here, not disproven. + +## Root cause of the live defect + +`src/utils/auth.ts`: + +``` +getAuthenticatedClient(config) // line 22 + └─ getCodemieClient() // line 44 + └─ throws ConfigurationError + 'SSO authentication required. Please run "codemie setup" with SSO provider first.' + └─ catch (line 45): message matches 'SSO authentication required' + └─ promptReauthentication(config) // line 47 + └─ handleAuthValidationFailure(...) // line 68 + └─ isNonInteractiveEnvironment() === true + → prompt correctly skipped, returns false ← PR #471 working + └─ falls through to line 76 + └─ throw new ConfigurationError('Authentication expired. Please re-authenticate.') + ← UNCAUGHT +``` + +Two distinct problems on that last hop: + +1. **The throw escapes the command's error handler.** *(Corrected after technical analysis — my first reading of this was wrong.)* `assistants.ts` **does** import and use `handleSdkError` in all six actions. The defect is **statement ordering**: `const client = await getSdkClient();` sits at line 65, *outside and above* the `try {` at line 68, so the try/catch only guards the post-auth API call. The auth throw sails past it to Node's default handler, which prints the raw stack and exits 1. `bin/codemie.js` installs no `uncaughtException` / `unhandledRejection` handler as a backstop. AC "exits non-zero with clear remediation" fails on *clear*, not on *non-zero*. + +2. **The actionable text is discarded.** `getCodemieClient` produced the remediation the AC asks for — *"Please run `codemie setup` with SSO provider first"*. `promptReauthentication` throws a **new**, vaguer error at line 76 that overwrites it. The useful string is generated and then thrown away one frame later. + +`handleAuthValidationFailure` and `isNonInteractiveEnvironment` are working exactly as designed; the gap is in what happens *after* they correctly decline to prompt. + +## Blast radius + +**Corrected after technical analysis.** My initial claim that `src/agents/core/AgentCLI.ts` and `src/cli/commands/profile/index.ts` were affected was **wrong** — those call `handleAuthValidationFailure` *directly*, not `getAuthenticatedClient`/`promptReauthentication`, and they already exit cleanly. The uncaught-throw path is narrower than this report first stated. + +The actual blast radius is wider in a different direction: the same outside-the-try ordering repeats in **all 8 `src/cli/commands/sdk/*.ts` files, ~50 command actions**. A grep for `try {` within two lines above any `await getSdkClient()` returns zero matches. + +There is, however, a **single choke point**: `getSdkClient()` at `src/cli/commands/sdk/utils/cli-utils.ts:15-18` is the sole bridge from all ~50 sdk actions into `src/utils/auth.ts`. Fixing there — plus preserving the message at `auth.ts:76` — follows the shared-gate precedent PR #471 set, instead of 50 mechanical edits. + +Note that `handleSdkError`'s `else` branch **already** renders `ConfigurationError` cleanly. So wrapping alone removes the stack trace but still prints the vague message: **both halves must be fixed** to satisfy the AC. + +## Acceptance criteria mapped to evidence + +| AC | Verdict | Evidence | +|---|---|---| +| Non-TTY stdin skips interactive prompt | **Met already** | Case A/B (0–1 s, no prompt) vs Case C (blocks with TTY) | +| CLI exits non-zero with clear remediation | **Not met** | Case A/B: exit 1 but raw stack trace; message lacks `codemie setup` | +| `--non-interactive` / `--ci` supported or documented | **Partially met** | No such flag in `src/`, but the absence is documented at `docs/AUTHENTICATION.md:113`. The AC reads "supported **or** documented". Not a clean pass: the documented mechanism (`stdin` TTY only) has a blind spot for pty-allocating CI — now stated explicitly in that doc | +| Kill during prompt produces no readline crash | **Not exercised** | The prompt was never reached in any attempt, so the criterion was never tested — see `ac4-investigation.md` | diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/spec.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/spec.md new file mode 100644 index 000000000..53f54f827 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/spec.md @@ -0,0 +1,76 @@ +# EPMCDME-14148 — Spec + +Approved design, bounded path. Grounding: `reproduction.md` (empirical, 4 cases), `technical-analysis.md` (407 lines), `complexity-assessment.json` (M, 19/36). + +## Problem + +In a non-TTY session with no valid SSO credentials, `codemie sdk ` exits 1 by letting a `ConfigurationError` escape to Node's default handler — printing a raw stack trace — and the message it prints has lost the actionable remediation. + +The non-TTY *hang* named in the ticket title is **already fixed** on main by `5b2de4b7` (PR #471). Case C in `reproduction.md` (same command with a TTY blocks; without a TTY it does not) is the control proving that guard is live. What remains is the *quality of the failure*, not the hang. + +## Root cause + +``` +getCodemieClient() sdk-client.ts:73-75 + throws ConfigurationError('SSO authentication required. Please run "codemie setup"...') ← actionable +getAuthenticatedClient() catch auth.ts:45 + → promptReauthentication() auth.ts:47 + → handleAuthValidationFailure() auth-validation.ts:34 → non-TTY: skips prompt, returns false ✅ PR #471 + → throw ConfigurationError('Authentication expired. Please re-authenticate.') auth.ts:76 ← shadows the actionable message +getSdkClient() cli-utils.ts:15-18 → no try/catch → escapes to Node +``` + +Two independent defects. Fixing either alone is insufficient: wrapping alone still prints the vague message (`handleSdkError`'s `else` branch already renders `ConfigurationError` cleanly); message-fixing alone still prints a stack trace. + +## Scope + +**In scope** + +1. `sdk/utils/cli-utils.ts` — wrap `getSdkClient()` so auth failures reach the existing `handleSdkError` sink. One edit covers ~50 actions across 8 files (shared-gate precedent from PR #471). +2. `utils/auth.ts` — preserve the actionable upstream error rather than letting `promptReauthentication`'s throw shadow it. +3. `providers/core/auth-validation.ts:39` — diagnostic to **stderr**, not stdout (currently pollutes piped output and `--json`). +4. `utils/sdk-client.ts:26` — no `ora` spinner when non-interactive. +5. `bin/codemie.js` — process-level `unhandledRejection` / `uncaughtException` net (template: `bin/codemie-mcp-proxy.js:75-82`). +6. `docs/AUTHENTICATION.md:104-117` — align documented promise with actual emitted message. **Non-optional**: PR #471 set the precedent, and EPMCDME-13953's CR-001 was closed specifically on this doc surface. +7. Timeboxed AC4 investigation in a real terminal. + +**Out of scope** + +- Any `--non-interactive` / `--ci` flag. AC3 is worded "supported **or** documented"; `docs/AUTHENTICATION.md:113` already documents the absence as intentional, and EPMCDME-13953's spec lists the flag as an explicit out-of-scope decision. Cite, do not implement. +- The other 45 unguarded `inquirer.prompt` sites across 19 files — same class of latent bug, their own ticket. +- `profile/index.ts:138-140` exiting 0 on declined re-auth — a real inconsistency, but not this defect. +- Reformatting `cli-utils.ts` double quotes (ESLint does not enforce quote style; would balloon the diff). + +## Design decisions + +**Why wrap at `getSdkClient()` rather than move `await getSdkClient()` inside each action's existing `try`.** The latter is ~50 mechanical edits across 8 files with 50 chances to miss one; a grep confirms all 50 are currently outside their `try`. The gate is one function and matches how PR #471 fixed the sibling defect. + +**Why preserve the original error rather than add `cause` or change `promptReauthentication`'s throw.** Three constraints make this the cheapest correct option: + +- `ConfigurationError`'s constructor takes only `message` — `cause` would mean touching a repo-wide base class. +- `promptReauthentication`'s declared `Promise` is unreachable-`false`; converting its throw to a return would revive dead code and change `assistants/chat/index.ts:423`. +- `auth.test.ts:142-155` asserts that throw's message verbatim. Leaving `promptReauthentication` untouched means **that test keeps passing unmodified** — only the `getAuthenticatedClient` reauth-fails case changes, deliberately. + +`no-useless-catch` is an ESLint *warn* under `--max-warnings=0`. Both new catches transform rather than bare-rethrow, so neither trips it. + +**Layering.** `utils/` throws; the CLI layer formats and exits (`architecture.md:159-171`). The fix keeps user-facing formatting in `cli-utils.ts` and does not deepen `auth.ts`'s existing chalk-output smell. + +## Acceptance + +| AC | Disposition | +|---|---| +| Non-TTY stdin skips interactive prompt | Already met (PR #471); covered by regression test | +| CLI exits non-zero with clear remediation | **The fix.** Exit 1, no stack trace, message names `codemie setup` | +| `--non-interactive` / `--ci` supported or documented | Met by existing `AUTHENTICATION.md:113`; cite in MR | +| Kill during prompt → no readline crash | Timeboxed investigation; report "cannot reproduce" if it stays quiet | + +## Testing + +Unit tests live in `src/**/__tests__/` — **never** `tests/unit/`, where five files match no vitest project glob and silently never execute. + +- **New** `src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts` (greenfield — `sdk/**` has zero tests across 22 files). Pattern from `skills/__tests__/commands.test.ts`: `process.exit` spy that records the code then throws. +- **Updated** `src/utils/__tests__/auth.test.ts` — the `getAuthenticatedClient` reauth-fails case now asserts the preserved actionable message. +- **Non-interactivity is injected at the seam** (`vi.mock` the function), not via `process.stdin.isTTY` — per the `auth-validation.test.ts` template. +- **Integration** via `tests/helpers/cli-runner.ts` `runSilent()` with stdin ignored: assert exit ≠ 0, remediation text present, and **no stack trace** in output. + +Commit scope must be `fix(cli)` or `fix(utils)` — commitlint's `scope-enum` rejects `fix(auth)` and `fix(sdk)`. diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/technical-analysis.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/technical-analysis.md new file mode 100644 index 000000000..eabd487af --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/technical-analysis.md @@ -0,0 +1,407 @@ +# Technical Research + +**Task**: cli auth sso non-interactive error-handling (EPMCDME-14148) +**Generated**: 2026-09-03 +**Research path**: filesystem (codegraph MCP not available in this environment) + +--- + +## 1. Original Context + +EPMCDME-14148 (Bug, Major) — "CLI non-interactive SSO failure hangs on prompt and can crash with readline error" + +Repository: `/Users/Evgenii_Kurdakov/Desktop/projects/codemie-dev/codemie-code` (the `codemie` CLI, `@codemieai/code`). Branch EPMCDME-14148, based on origin/main @ 1d5cc22b (v0.15.0). + +Ticket description verbatim: + +``` +## Summary +CLI non-interactive SSO failure hangs on prompt and can crash with readline error. +## Description +When SSO credentials are missing in a non-TTY session, CLI prompts for re-authentication and may crash with ERR_USE_AFTER_CLOSE. Non-interactive automation should fail cleanly with actionable remediation. +## Preconditions +- CLI has no valid SSO session. +- Standard input is non-interactive. +## Steps to Reproduce +1. Ensure there is no valid CLI SSO session. +2. Run any `codemie sdk ...` command with stdin redirected from /dev/null. +3. Observe authentication prompt behavior and process termination. +## Expected Result +CLI exits non-zero with an actionable message such as "run codemie setup". +## Actual Result +CLI hangs on a re-authentication prompt and can crash with ERR_USE_AFTER_CLOSE if killed. +## Acceptance Criteria +- Non-TTY stdin skips interactive prompt. +- CLI exits non-zero with clear remediation. +- Optional --non-interactive or --ci behavior is supported or documented. +- Killing during prompt does not produce readline lifecycle crash. +``` + +Reproduction has already been performed; full evidence at `docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md`. Confirmed inputs to this research: + +- The non-TTY hang is ALREADY FIXED on main by commit `5b2de4b7` (PR #471), via `src/utils/interactive.ts` `isNonInteractiveEnvironment()` consumed by `src/providers/core/auth-validation.ts` `handleAuthValidationFailure()`. +- The RESIDUAL live defect: `src/utils/auth.ts` `promptReauthentication()` line 76 throws `ConfigurationError('Authentication expired. Please re-authenticate.')` uncaught — Node prints a raw stack trace and exits 1. Two problems: (a) no error boundary between the throw and Node's default handler; (b) the actionable remediation string produced upstream by `getCodemieClient` is discarded. +- Shared blast radius beyond the sdk commands. +- No `--non-interactive` or `--ci` flag exists anywhere in `src/`. +- ERR_USE_AFTER_CLOSE was NOT reproduced in 2 attempts. + +--- + +## 2. Codebase Findings + +### 2.0 Correction to the ticket premise — READ FIRST + +The reproduction note states there is "no try/catch at the command action layer (`src/cli/commands/sdk/assistants.ts`)". **That is not accurate, and the distinction changes the shape of the fix.** + +`src/cli/commands/sdk/assistants.ts` **does** import and use `handleSdkError` in all 6 of its actions. The real defect is **statement ordering**: + +- `src/cli/commands/sdk/assistants.ts:65` — `const client = await getSdkClient();` sits **outside and above** the `try {` at line 68. +- The `try/catch` therefore guards only the *post-auth* API call. Auth acquisition itself is unguarded, so the `ConfigurationError` escapes the action, escapes commander, and reaches Node's default handler. +- **This is systemic.** All 8 SDK command files place `await getSdkClient()` before their `try {`. A grep for `try {` within two lines above any `await getSdkClient()` returns **zero** matches across the tree. + +**Blast radius: ~50 SDK command actions across 8 files, every one unguarded at the auth step.** + +### 2.1 Failure chain (verified end to end) + +1. `src/utils/sdk-client.ts:73-75` throws `ConfigurationError('SSO authentication required. Please run "codemie setup" with SSO provider first.')` — **the actionable message**. +2. `src/utils/auth.ts:46` catches it, matches `error.message.includes('SSO authentication required')` (brittle string coupling), calls `promptReauthentication(config)`. +3. `src/utils/auth.ts:67` — SSO `validateAuth` returns `{ valid: false, ... }`. +4. `src/utils/auth.ts:68` — `handleAuthValidationFailure` → `src/providers/core/auth-validation.ts:34` guard `setupSteps?.promptForReauth && !isNonInteractiveEnvironment()` is **false** in non-TTY → prompt correctly skipped → prints `chalk.red("\n✗ \n")` via `console.log` (**stdout**, line 39) → returns `false`. +5. `src/utils/auth.ts:76` throws `ConfigurationError('Authentication expired. Please re-authenticate.')` — **the step-1 actionable message is discarded**: never re-thrown, never attached as `cause`. +6. Nothing catches it → raw stack trace, exit 1. + +The `validationResult.error` printed at step 4 contains genuinely useful text produced by `src/providers/plugins/sso/sso.setup-steps.ts:236-239` (`No SSO credentials found for . Please run: codemie profile login --url `) — but that string is only *printed*, never propagated into the exception. That is the second half of the "message discarded" complaint. + +### 2.2 Existing Implementations + +**Auth core** +- `src/utils/auth.ts` (77 lines) — two exports, no non-interactive awareness of its own. + - `getAuthenticatedClient(config): Promise` L22-54. JWT branch L23-41 (throws at L26-29, L32-34; no prompting). SSO branch L43-53: `try { return await getCodemieClient(); } catch { ... }`. + - `promptReauthentication(config): Promise` L63-77. **L76 is the uncaught throw.** Control-flow note: this function can only return `true` or throw — the declared `Promise` is misleading and the `false` branch at `auth.ts:48` is dead code. Confirmed by `src/utils/__tests__/auth.test.ts:142-176`. + - JSDoc at L61 already documents `@throws ConfigurationError if re-authentication is not available`. + - L71 does `console.log(chalk.green(...))` — Utils layer doing user output, an existing layering smell. +- `src/utils/sdk-client.ts` `getCodemieClient(quiet = false)` L23-110. Throws at L37-39, **L73-75 (the discarded actionable message)**, L106-108. Starts an `ora` spinner at L26 unless `quiet`; `getAuthenticatedClient` always calls it non-quiet, so a spinner is spawned even in non-TTY. +- `src/providers/core/auth-validation.ts` (41 lines) — sole export `handleAuthValidationFailure`. Guard at L34. Fallback L39: `console.log(chalk.red(...)); return false;`. +- `src/utils/interactive.ts` (16 lines) — sole export `isNonInteractiveEnvironment(): boolean { return !process.stdin.isTTY; }` (L15). Consults **stdin only** — not stdout, not `process.env.CI`, not `TERM`. + +**SDK command layer** +- `src/cli/commands/sdk/utils/cli-utils.ts` (145 lines): + - `getSdkClient(): Promise` L15-18 — `ConfigLoader.load()` → `getAuthenticatedClient(config)`. **No try/catch.** Sole bridge from the SDK CLI to `src/utils/auth.ts`; single choke point for all ~50 actions. + - `handleSdkError(error: unknown, operation: string): never` L88-119 — extracts `error.message` (else `String(error)`); `logger.error("SDK operation failed", ...sanitizeLogArgs({operation, error: msg}))`; branches on `ApiError` status 401/403 (prints `Run "codemie setup" to re-authenticate if your session expired.` at L102), 404, `ZodError`; **else branch L116** prints `chalk.red("❌ " + msg)`; always writes to **stderr**; ends `process.exit(1)` L118. + - Consequence: a `ConfigurationError` already falls into the `else` branch and would exit 1 cleanly with a red message. **Moving `getSdkClient()` inside the existing `try` fixes the stack trace — but the message would still be the useless one.** Both halves must be fixed. + - Siblings in the same file: `parseDataInput` L23-35, `parseJsonFileInput` L40-49, `parseDataOrJsonFile` L55-76, `outputJson` L81-83, `getResponseMessage` L124-129, `parseConfigInput` L134-145. + +**handleSdkError adoption audit** — `path` — uses handleSdkError — try/catch in action — notes: +- `src/cli/commands/sdk/assistants.ts` — yes (import L23; L106,151,183,212,228,280) — **partial** — `getSdkClient()` at L65,115,166,198,220,240 all precede `try {` at L68,118,169,200,222,243. 6/6 actions unguarded for auth. +- `src/cli/commands/sdk/categories.ts` — yes (L22) — partial — 5/5 unguarded. +- `src/cli/commands/sdk/datasources.ts` — yes (L10) — partial — 7/7 unguarded. +- `src/cli/commands/sdk/integrations.ts` — yes (L22) — partial — 6/6 unguarded. +- `src/cli/commands/sdk/llm.ts` — yes (L9) — partial — 1/1 unguarded (`getSdkClient()` L39 vs `try` L43). +- `src/cli/commands/sdk/skills.ts` — yes (L38, 18 call sites) — partial — 18/18 unguarded. +- `src/cli/commands/sdk/users.ts` — yes (L5) — partial — 2/2 unguarded. +- `src/cli/commands/sdk/workflows.ts` — yes (L21) — partial — 5/5 unguarded. +- `src/cli/commands/sdk/index.ts` — no — n/a — pure `Command` composition. +- `src/cli/commands/sdk/utils/{render,datasource-types,file-utils}.ts` — no — n/a. +- `src/cli/commands/sdk/services/*.ts` (9 files) — no — n/a — thin SDK pass-through; deliberately lets errors propagate to the command layer. + +### 2.3 Call-site census + +**`getCodemieClient`** (def `src/utils/sdk-client.ts:23`): +- `src/utils/auth.ts:44` — in `getAuthenticatedClient` — try/catch **yes** (L43-53); catch string-matches then calls `promptReauthentication`, else rethrows original at L52. +- `src/utils/auth.ts:49` — inside that catch, after successful re-auth — try/catch **no**; a second failure propagates raw. +- `src/cli/commands/skills/setup/sync.ts:36` — try/catch **yes**; catch is `logger.debug` only, fully swallowed by design. + +**`getAuthenticatedClient`** (def `src/utils/auth.ts:22`): +- `src/cli/commands/sdk/utils/cli-utils.ts:17` — in `getSdkClient` — try/catch **no**. Hot path for all ~50 SDK actions. **Primary fix location.** +- `src/cli/commands/assistants/chat/index.ts:91` — no try/catch at the line, but the commander action at L47-63 wraps it: catch L58-62 does `createErrorContext` → `logger.error` → `console.error(formatErrorForUser(context))` → `process.exit(1)`. **Clean exit, no stack trace.** +- `src/cli/commands/assistants/setup/index.ts:68` — guarded at the caller; action L47-57 → `handleSetupError(error, 'setup assistants')` L55. +- `src/cli/commands/skills/setup/index.ts:107` — guarded at the caller → `handleSetupError(error, 'setup skills')` L32. + +**`promptReauthentication`** (def `src/utils/auth.ts:63`): +- `src/utils/auth.ts:47` — in the catch of `getAuthenticatedClient` — **no** guard; its L76 throw escapes and shadows the original error. +- `src/cli/commands/assistants/chat/index.ts:423` — in `handleChatError`, reached from L265/L314 inside existing catches, all under the action-level boundary at L56-62. Only invoked when `error.message.includes('401'|'403')` (L422). + +**Correction on blast radius.** `src/agents/core/AgentCLI.ts` and `src/cli/commands/profile/index.ts` are **not** call sites of the three symbols. They call `handleAuthValidationFailure` directly: +- `src/agents/core/AgentCLI.ts:293-294` and `:323-324` (dynamic imports). Both inside try/catch. On `reauthed === false` (L296-299): `console.log(chalk.yellow('\n⚠️ Authentication required\n'))` + `process.exit(1)` — **already a clean non-zero exit**. +- `src/cli/commands/profile/index.ts:130` — inside try L123 / catch L143-145 (`logger.error` only, execution continues). On `reauthed === false` (L138-140): prints a yellow warning and `return`s — **exit code 0**. An inconsistency worth noting, arguably out of scope. + +So the uncaught-throw defect is confined to the `getAuthenticatedClient` → `promptReauthentication` path, whose only unguarded consumer is `getSdkClient`. + +### 2.4 Architecture and Layers Affected + +Project layer taxonomy (`.ai-run/guides/architecture/architecture.md` L65-88): **CLI (`src/cli/`) → Registry → Plugin (`src/*/plugins/`) → Core (`src/*/core/`) → Utils (`src/utils/`)**. Never skip layers, never reverse direction (L147-155). + +Documented error flow (L159-171), directly load-bearing here: + +``` +Plugin Error (throws) → Registry (catches, adds context) → re-throws → CLI (catches, formats for user) +``` + +⇒ **Formatting for the user is the CLI layer's job, not `src/utils/auth.ts`'s.** `auth.ts` already violates this (chalk output at L71) and the fix should not deepen it. + +Layers touched by a fix: +- **CLI** — `src/cli/commands/sdk/utils/cli-utils.ts` (error sink + auth gate); optionally the 8 sdk command files; optionally `bin/codemie.js` (process-level net). +- **Utils** — `src/utils/auth.ts` (message preservation); possibly `src/utils/errors.ts` if `cause` support is added. +- **Core** — `src/providers/core/auth-validation.ts` (stdout→stderr only, cosmetic). + +### 2.5 Top-level error handling — there is none + +- `bin/codemie.js` (41 lines): try/catch around `MigrationRunner` L13-22 (non-fatal warning); try/catch around `checkAndPromptForUpdate()` L28-34 (swallowed); `import('../dist/cli/index.js').catch(err => { console.error('Error:', err.message); process.exit(1); })` L37-40. **That `.catch` only covers module-load/top-level-await rejections.** `program.parse()` is synchronous and returns immediately; commander async action rejections never reach this chain. **No `process.on('uncaughtException')`, no `process.on('unhandledRejection')`.** +- `src/cli/index.ts` (148 lines): `new Command()` L44; `program.parse(process.argv)` L147 — plain **sync** `parse`, not `parseAsync`, not awaited. **No `.exitOverride()`, no `program.error()`, no `.configureOutput()`, no `.showHelpAfterError()`.** +- Whole-tree grep: `uncaughtException` and `unhandledRejection` each have exactly **one** hit, both in `bin/codemie-mcp-proxy.js:75` and `:79` — a ready-made in-repo template if a process-level net is wanted. `exitOverride` appears only in two test files. `process.exit` appears at 162 non-test sites across ~29 command files. + +**Conclusion: no centralised CLI error boundary exists.** Anything escaping a commander action prints a raw stack. + +### 2.6 Established clean-exit patterns (exemplars a fix should imitate) + +Four competing conventions exist. Ranked by fit: + +**(a) `handleSdkError` — the in-domain convention.** `src/cli/commands/sdk/utils/cli-utils.ts:88-119`. Already imported by all 8 SDK files. `logger.error` + `sanitizeLogArgs` → `chalk.red('❌ …')` to stderr → `process.exit(1)`. + +**(b) `requireAuthenticatedSession` / `failAuth` — the closest semantic analogue.** `src/cli/commands/skills/lib/require-auth.ts:24-51`: +```ts +function failAuth(message: string): never { + console.error(chalk.red(`\n${message}\n`)); + process.exit(1); +} +``` +with `NOT_AUTHENTICATED_MESSAGE` (L15-16) = `'CodeMie SSO authentication required. Run "codemie setup" or "codemie profile login" first.'` — **precisely the actionable text EPMCDME-14148 says is being discarded.** Used as the first statement of 5 skills actions: `find.ts:48`, `list.ts:30`, `add.ts:49`, `update.ts:32`, `remove.ts:41`. Note L41-44: it treats a *thrown* auth check as unauthenticated rather than letting it escape — exactly the defensive posture missing from `getSdkClient`. **This path already never prompts and already exits non-zero cleanly.** + +**(c) `printProxyError` — the `ConfigurationError`-aware formatter.** `src/cli/commands/proxy/connect-orchestrator.ts:250-262`: known project error → terse one-line `chalk.red('✗ …')`; unknown error → `formatErrorForUser(context, { showSystem: false })`; then `process.exit(1)`. Used at `connect-orchestrator.ts:673` and `proxy/index.ts:173`. **Best model for the discrimination this fix needs.** + +**(d) `handleSetupError` — the guide-canonical generic boundary.** `src/cli/commands/shared/helpers.ts:64-69`: `createErrorContext` → `logger.error` → `console.error(formatErrorForUser(context))` → `process.exit(1)`. Used at `assistants/setup/index.ts:55` and `skills/setup/index.ts:32`; the same shape is inlined at `assistants/chat/index.ts:58-62`. Caveat: `formatErrorForUser` defaults `showSystem: true`, printing an OS/Node/version block — verbose for a simple "you're not logged in". + +### 2.7 Error class hierarchy — `src/utils/errors.ts` (566 lines) + +Flat, single base, all `constructor(message: string)`. **No `code`, no `exitCode`, no `isOperational` on the base.** + +| Class | Line | Constructor | +|---|---|---| +| `CodeMieError extends Error` | L1-6 | `(message)`, sets `this.name` | +| `ConfigurationError extends CodeMieError` | L8-13 | `(message)` | +| `AgentNotFoundError` | L15-20 | `(agentName)` | +| `AgentInstallationError` | L22-27 | `(agentName, reason)` | +| `ToolExecutionError` | L29-34 | `(toolName, reason)` | +| `PathSecurityError` | L36-41 | `(path, reason)` | +| `AnalyticsSourceError` | L43-48 | `(message)` | +| `NpmError extends CodeMieError` | L64-73 | `(message, code: NpmErrorCode, originalError?)` — **only subclass with a `code`** | + +**No constructor accepts an `options`/`cause` argument.** Preserving the upstream error via `{ cause }` therefore requires a constructor change or a post-hoc `.cause` assignment. + +Exported helpers: `parseNpmError` L81, `getErrorMessage` L133, `createErrorContext` L302, `formatErrorForUser` L358 (plain string, **no chalk**; `❌ ` wrapped at 97 chars + System Information block, `showSystem` defaults **true**), `formatErrorForLog` L424, `getErrorExplanation` L434, `formatErrorWithExplanation` L529. **Caution: `getErrorExplanation`'s generic fallback (L512-519) says "Metrics collection encountered an issue"** — metrics-specific and wrong for auth errors, so avoid `formatErrorWithExplanation` here. + +`instanceof` checks against project error classes exist at only **5 non-test sites**: `src/utils/auth.ts:46`, `proxy/connect-orchestrator.ts:254`, `proxy/connectors/vscode.ts:197`, `proxy/connectors/desktop.ts:155`, `proxy/connectors/vscode-claude-code.ts:68`. + +### 2.8 Integration Points + +- `codemie-sdk` (`CodeMieClient`, `ApiError`) — constructed in `src/utils/sdk-client.ts` and `src/utils/auth.ts:36-40`; consumed by all sdk services. +- `ProviderRegistry` (`src/providers/core/registry.ts`) → `getSetupSteps` → SSO plugin `src/providers/plugins/sso/sso.setup-steps.ts` (`validateAuth` L~230, `promptForReauth` L268 with an `inquirer.prompt` confirm at L274-281). Interface declared at `src/providers/core/types.ts:390`. +- `SecureStorage` (`src/utils/security.ts`) — OS keychain via lazily-imported `keytar`, AES-256-CBC machine-keyed file fallback. `FALLBACK_FILE = getCodemiePath('sso-credentials.enc')` L258, `CREDENTIALS_DIR = getCodemiePath('credentials')` L259; per-URL `credentials/.enc` and `credentials/jwt-.enc`. +- `ConfigLoader` (`src/utils/config.ts`) — global `~/.codemie/codemie-cli.config.json`, project-local `.codemie/codemie-cli.config.json`, precedence at L113-117. +- `logger` + `sanitizeLogArgs` — used by `handleSdkError`. +- `chalk`, `ora`, `inquirer`, `commander` — presentation and prompting. + +### 2.9 Patterns and Conventions + +- **Shared-gate fix pattern** (established by PR #471): fix once at the single shared gate, not per-caller. +- `never`-returning error sinks that own `process.exit(1)` — `handleSdkError`, `failAuth`, `printProxyError`, `handleSetupError`. +- `.js` extensions on all relative imports; `@/` alias preferred over deep `../../..` (AGENTS.md pitfalls table). Note the inconsistency: `src/utils/auth.ts` uses `@/`, `src/providers/core/auth-validation.ts` uses relative `../../utils/`. +- Explicit return types on all exported functions; `interface` over `type`. +- `src/cli/commands/sdk/utils/cli-utils.ts` uses **double quotes** throughout, contradicting the single-quote guidance — ESLint does not enforce quote style, so **do not mass-reformat**. + +--- + +## 3. Documentation Findings + +### Guides and Architecture Docs + +All present under `.ai-run/guides/`. + +**`development/development-practices.md`** — Error Handling L13-76. Exception hierarchy table L17-25. Canonical pattern L29-43 (citing `src/cli/commands/execute.ts:30-38`): `createErrorContext` → `logger.error` → `console.error(formatErrorForUser(context))`. Rules L45-51 verbatim: +- ✅ Use specific error classes (not bare `Error`) +- ✅ Always add context via `createErrorContext()` +- ✅ Log errors with `logger.error()` +- ✅ Format errors for user with `formatErrorForUser()` +- ❌ Expose internal implementation details +- ❌ Log stack traces to console (use `logger.debug()`) + +Defensive-null precedent L53-76 (return instead of throw when the caller can degrade). Logging L80-130: WARN/ERROR/SUCCESS go to console and file; always `sanitizeLogArgs()`; never log tokens. TypeScript rules L194-203. **No prompt/UX section, and no explicit exit-code rule** — exit codes are conventional (`process.exit(1)`) only. + +**`standards/code-quality.md`** — explicit return types on exports (L35); `.js` extensions and `import type` (L49); ESLint: `no-explicit-any` **off**, `no-unused-vars` warn, **`no-useless-catch` warn — "Avoid catch-and-rethrow"** (a bare re-throw in `auth.ts` would trip this and, under `--max-warnings=0`, fail the gate); functions **under 50 lines**, files **under 500** (L95-99); comment the *why*, JSDoc with `@throws` on public APIs (L105-110); pre-commit checklist L145-152 includes **no `console.log()` left in code**. + +**`quality-gates.md`** — literal commands, fastest-to-slowest, stop at first failure: + +| Gate | Command | +|---|---| +| License headers | `npm run license-check` | +| Lint | `npm run lint` (`eslint '{src,tests}/**/*.ts' --max-warnings=0`) | +| Typecheck | `npm run typecheck` (`tsc --noEmit`) | +| Build | `npm run build` | +| Unit | `npm run test:unit` | +| Integration | `npm run test:integration` | +| Secrets (local) | `npm run validate:secrets` | +| Commitlint | `npm run commitlint:last` | +| Pre-commit aggregate | `npm run check:pre-commit` | +| Full CI | `npm run ci` | + +Guide text for the test scripts is **stale** vs `package.json` (actual: `vitest run --project unit` / `--project cli`). Hooks: husky pre-commit = lint-staged → typecheck; `.claude/settings.json` PostToolUse auto-runs `npm run format` after every Edit/Write; Stop runs `npm run check:pre-commit`. + +**`architecture/architecture.md`** — layer taxonomy and error flow, summarised in §2.4. + +**`project.md`** — Jira, prefix `EPMCDME`; GitHub `codemie-ai/codemie-code`, target `main`, PRs via `gh`, squash-merge default. + +**`standards/git-workflow.md`** — branch `EPMCDME-[_kebab-description]`; Conventional Commits `(): `; **allowed scopes** (commitlint `scope-enum`): `cli, agents, providers, assistants, config, proxy, workflows, ci, analytics, utils, deps, tests, skills, kimi`. **`fix(auth)` and `fix(sdk)` would be rejected** — use `fix(cli)` or `fix(utils)`. Never `--no-verify`. + +**`AGENTS.md`** (canonical; `CLAUDE.md` imports it) — "Check Guides First"; precedence: guides win for process, source wins for facts. **"Tests Only On Explicit Request"** and **"Git Operations Only On Explicit Request"**. Task classifier: `error, exception, validation` → P0 development-practices; `cli, command, commander` → P0 architecture. + +**`CONTRIBUTING.md`** — no error-handling or CLI-UX section. Conventional Commits required for commits *and PR titles*; `npm run ci` fails if the last commit is non-conforming. + +### Architectural Decisions + +**PR #471 / commit `5b2de4b7`** — `fix(providers): skip interactive re-auth prompt in non-interactive environments (#471)`, Aug 7 2026. 16 files, +1045/−5. Substantive changes: +- `src/utils/interactive.ts` — **new**, 16 lines, sole export `isNonInteractiveEnvironment()`, JSDoc naming ERR_USE_AFTER_CLOSE as the motivation. +- `src/providers/core/auth-validation.ts` — one behavioural line: guard became `if (setupSteps?.promptForReauth && !isNonInteractiveEnvironment())`. +- `docs/AUTHENTICATION.md` +15 — new section, now at **L104-117**. +- `src/providers/core/__tests__/auth-validation.test.ts` — new, 104 lines, 5 tests. +- `src/utils/__tests__/interactive.test.ts` — new, 33 lines, 3 tests. + +**Approach precedent this fix must extend, not replace**: fix at the single shared gate; TTY-only detection; no new flag; provider setup-steps untouched; co-located unit tests; documentation change is part of the deliverable. + +**Prior task `docs/superpowers/tasks/2026-08-06-non-interactive-sso/` (EPMCDME-13953)** — the decision record that governs this ticket: +- `spec.md` "Out of scope": *"No new `--non-interactive` CLI flag or `CI` env var detection (**explicit decision — TTY check only**)."* +- `spec.md` asserted the callers "already handle a failed/`false` result correctly today (… `utils/auth.ts` throws `ConfigurationError`)" — **this assumption is exactly what reproduction.md falsifies.** +- `code-review-final.json` — decision `request-changes`, CR-001 (major, docs). Deferred/dismissed list explicitly records *"a pre-existing missing try/catch explicitly out of scope"*, plus *"TTY-only detection: no stdout check, pseudo-TTY containers still hang"*. **The missing try/catch was knowingly deferred in Aug 2026 and is the live defect now.** + +**`docs/AUTHENTICATION.md:104-117`** — the documented promise, verbatim in substance: the prompt is "automatically skipped", the CLI "fails fast with a clear message … and exits non-zero instead of hanging", and (L113) *"There is no separate `--non-interactive` flag to set — detection is automatic, based solely on whether `stdin` is a TTY."* **This documented promise is precisely what the CLI does not currently deliver** — the gap is in the message, not the exit code. + +**`docs/superpowers/specs/2026-07-01-EPMCDME-12992-session-origin-validation-design.md:97`** — non-interactive (piped stdin / `--yes` / `CODEMIE_NO_PROMPTS=1`) → behave as if the user declined → exit 1. Three-way precedent worth mirroring. + +**Inline decision markers**: `grep -rnE 'TODO:|HACK:|NOTE:|FIXME:|XXX|@deprecated'` over `src/utils/auth.ts`, `src/utils/interactive.ts`, `src/providers/core/auth-validation.ts`, `src/cli/commands/sdk/`, `sso.setup-steps.ts`, `AgentCLI.ts`, `profile/index.ts`, `errors.ts` → **zero matches.** No inline decision debt; all recorded decisions live in the SDLC task dirs. + +**No CHANGELOG.md exists** anywhere in the repo. `docs/AUTHENTICATION.md` is the only user-facing surface for this behaviour. + +### Derived Conventions + +- Auth failures are surfaced by a `never`-returning helper that logs, prints one chalk-red line, and exits 1. Four such helpers exist; **reuse, do not invent a fifth.** +- Non-interactivity is detected once, in `src/utils/interactive.ts`, and consumed at the gate — never re-derived at call sites. +- Utils-layer modules should throw; the CLI layer formats and exits. + +--- + +## 4. Testing Landscape + +### Existing Coverage + +- **`src/utils/__tests__/auth.test.ts`** (177 lines) — covers `getAuthenticatedClient` (success L52, retry-after-reauth L63, throw when reauth fails L90, non-auth rethrow L108) and `promptReauthentication` (success L126, **throws `'Authentication expired. Please re-authenticate.'` at L142-155**, no setupSteps L157, no validateAuth L166). Mocks `../sdk-client.js`, `../../providers/core/registry.js`, `../../providers/core/auth-validation.js` (L9-21). **L142-155 encodes the current buggy behaviour and will have to change.** Does not cover the JWT branch (`auth.ts:23-41`) at all. +- **`src/utils/__tests__/interactive.test.ts`** (33 lines) — 3 tests; assigns `process.stdin.isTTY` directly, restores in `afterEach`. +- **`src/providers/core/__tests__/auth-validation.test.ts`** (104 lines) — the PR #471 guard test; the template (see below). +- **`src/agents/core/__tests__/`** — 5 AgentCLI test files, none about SSO/auth. +- **`src/cli/commands/sdk/**` — NO TESTS AT ALL.** All 22 files untested; there is no `src/cli/commands/sdk/__tests__/` directory. **`cli-utils.ts` — containing `getSdkClient()` and `handleSdkError()` — is entirely untested.** This is the natural home for the fix's tests and is greenfield. +- **`bin/codemie.js` — no direct tests**; exercised only as a subprocess via `tests/helpers/cli-runner.ts`. `bin/` is excluded from coverage (`vitest.config.ts:43`), so a process-level handler there cannot be unit-tested — it needs a cli-project subprocess test. + +### Testing Framework and Patterns + +- **Vitest 3+ style**, single `vitest.config.ts` (100 lines, no `vitest.workspace.ts`), three `defineProject` entries. All alias `@` → `/src`; all set `FORCE_COLOR=1`, `NODE_ENV=test`, `CODEMIE_HOME=/codemie-test-home-`. + - **unit**: `include: ['src/**/*.test.ts', 'src/**/*.spec.ts']`, `globals: true`, node env, 30 s timeouts, `isolate: true`, **no setupFiles**. + - **cli**: `include: ['tests/integration/**/*.test.ts']`, excludes `agent-*`. + - **agent**: `include: ['tests/integration/agent-*.test.ts']`, `globalSetup: tests/setup/agent-build-setup.ts`. + - Coverage on the unit project only, provider `v8`, **no `thresholds` configured** — the 80/90 % numbers in the guide are advisory, not machine-enforced. +- **Guide mandates** (`.ai-run/guides/testing/testing-patterns.md`): unit tests co-located at `src/[module]/__tests__/*.test.ts` (L9-13); AAA (L21); `vi.mock()` at module level, `vi.spyOn()` in `beforeEach` + `vi.restoreAllMocks()` in `afterEach` (L46-66); **CRITICAL — import the module under test *inside the test body* via `await import('../auth.js')` after mocks are set** (L70-94), because static top-level imports are cached before `beforeEach`; async errors via `await expect(fn()).rejects.toThrow(ErrorClass)` (L115-122); assert **both** error class and code (L128-138); no hardcoded POSIX paths (L145-154); critical paths incl. `src/utils/` 90 %+ (L256-262). **No TDD mandate** — and AGENTS.md sets a stronger repo policy: *"Tests Only On Explicit Request."* +- **Template — `src/providers/core/__tests__/auth-validation.test.ts`**: explicit named imports from `vitest` despite `globals: true` (L1-3); a single top-level `vi.mock('../../../utils/interactive.js', () => ({ isNonInteractiveEnvironment: vi.fn() }))` (L5-7); **it does NOT touch `process.stdin.isTTY` or env** — it mocks the *function* and drives it with `mockReturnValue(true|false)` per test (L26, L41, L63, L78, L94). **Inject non-interactivity at the seam, not at the process level.** Per-test dynamic import inside each `it()` (L24-28, L39-44). Console captured via `vi.spyOn(console, 'log').mockImplementation(() => {})` and asserted with `expect.stringContaining(...)` (L55-57, L88) plus `toHaveBeenCalledTimes(1)` (L72) to prove no extra output path was taken. `afterEach` calls only `vi.restoreAllMocks()`. No `process.exit` or thrown-error assertion in this file — for the exit-code half of the fix, take that from the CLI exemplars below. +- **CLI command test exemplars**: `src/cli/commands/skills/__tests__/commands.test.ts` is the best all-round template — hoisted `vi.fn()`s behind `vi.mock()` factories (L22-46); `process.exit` spy that **records the code then throws** so the action aborts, with `exitCalls[]` capturing the first meaningful code (L67-78); `process.stderr.write` silenced (L79); `vi.resetModules()` in `afterEach` (L87); commander driven via `command.exitOverride(); await command.parseAsync([...])` (L96-101). Also `src/cli/commands/proxy/__tests__/index.test.ts` (canonical `{ from: 'user' }` argv form) and `src/agents/core/__tests__/AgentCLI-resume.test.ts` (`class ExitError extends Error` carrying the exit code, L9-13; `vi.spyOn(console, 'error')` + `expect.stringContaining` for remediation text, L115-122). +- **SDK client mocking**: only one file in the repo mocks it — `src/utils/__tests__/auth.test.ts:9-11` mocks `../sdk-client.js`. There is **no `vi.mock('codemie-sdk')` anywhere**. For `cli-utils.ts` tests, mock `@/utils/auth.js`'s `getAuthenticatedClient`. +- **`@clack/prompts` is not mocked anywhere**; the repo standardises on `inquirer` in tests (10 files). `vi.stubEnv` is used in exactly one file — manual save/restore of `process.env` is the house style. +- **Integration harness**: `tests/helpers/cli-runner.ts` — `CLIRunner` wraps `execSync('node ./bin/codemie.js ')`. **`runSilent()` returns `{ output, exitCode, error }` without throwing** (L43-59) — exactly the tool for asserting a non-zero exit plus remediation text. Exemplar: `tests/integration/cli-commands/error-handling.test.ts:24-35`. stdin redirection from `/dev/null` is **not currently done anywhere but is fully supported** — `runSilent(cmd, options)` spreads options into `execSync`, so `{ stdio: ['ignore','pipe','pipe'] }` or `{ input: '' }` yields a non-TTY stdin. Caveat: the spread lands *after* `encoding: 'utf-8'`. Real-TTY cases use node-pty via `tests/helpers/pty-session.ts`. `bin/codemie.js` imports from `../dist/`, so cli-project tests require a prior `npm run build`; the **cli project has no globalSetup** — `npm run ci` relies on `build` preceding `test:integration`. + +### Coverage Gaps + +- `src/cli/commands/sdk/**` — zero tests across 22 files, including the `getSdkClient` / `handleSdkError` choke point the fix will modify. +- `src/utils/auth.ts` JWT branch (L23-41) — untested. +- `bin/codemie.js` — untested and excluded from coverage. +- No test anywhere manipulates a `CI` env var; a `CI=true`-with-TTY scenario is currently undetectable and untested. +- **Orphaned test files — verified via `npx vitest list`**: the unit project collects 3 956 tests from `src/**` and **zero** from `tests/unit`; the cli project collects 37 files, all under `tests/integration/`. These 5 files match **no** project glob and **never execute**: + - `tests/unit/cli/commands/assistants/chat/index.test.ts` + - `tests/unit/cli/commands/assistants/chat/historyLoader.test.ts` + - `tests/unit/cli/commands/assistants/chat/utils.test.ts` + - `tests/skills/pattern-invocation.test.ts` + - `tests/scripts/test-proxy-endpoint.test.ts` + + This matters directly: `src/cli/commands/assistants/chat/index.ts:423` is the second `promptReauthentication` call site and its only test file is in that dead zone. **Any new unit test placed under `tests/unit/` will silently never run.** + +--- + +## 5. Configuration and Environment + +### Environment Variables + +No central registry or allowlist of `CODEMIE_*` names exists. `src/env/manager.ts` (`EnvManager`) is a key/value store over `~/.codemie/codemie-cli.config.json` with precedence `process.env[key]` > global config (L38-44); it does **not** declare env vars. + +Relevant to this task: +- `CODEMIE_NO_PROMPTS` — opt out of interactive prompts, value `'1'` — `src/agents/core/AgentCLI.ts:756`. **The only non-test consumer, and the only env-var escape hatch precedent in the repo.** +- `CI` — **written, never read as detection** — `src/cli/commands/skills/lib/run-skills-cli.ts:89`. +- `CODEMIE_HOME` — overrides `~/.codemie` — `src/utils/paths.ts:356-361` (99 references, the most-used var; used for test isolation). +- `CODEMIE_JWT_TOKEN`, `CODEMIE_AUTH_METHOD` — JWT auth path — `src/agents/core/AgentCLI.ts:~206-207`. +- `CODEMIE_URL`, `CODEMIE_BASE_URL`, `CODEMIE_API_KEY`, `CODEMIE_PROVIDER`, `CODEMIE_MODEL`, `CODEMIE_TIMEOUT`, `CODEMIE_PROFILE_CONFIG`, `CODEMIE_PROFILE_NAME` — `src/utils/config.ts`, `src/utils/profile.ts`. +- `CODEMIE_INSECURE` — read at `src/utils/auth.ts:39` (`verify_ssl: process.env.CODEMIE_INSECURE !== '1'`). +- `CODEMIE_DEBUG` (30 refs), `DO_NOT_TRACK`, `DISABLE_TELEMETRY`, `NODE_OPTIONS`. + +**Zero hits anywhere in `src/` or `bin/` for**: `CODEMIE_NON_INTERACTIVE`, `TERM=dumb`, `FORCE_COLOR` (as a read), `noninteractive`. + +### Configuration Files + +- Global: `~/.codemie/codemie-cli.config.json` — declared twice (`src/env/manager.ts:11`, `src/utils/config.ts:57`). +- Project-local: `.codemie/codemie-cli.config.json` — `src/utils/config.ts:62`, created at L886-911; precedence documented at L113-117. +- Credentials: `src/utils/security.ts` — keychain (keytar) primary, `~/.codemie/credentials/.enc` AES-256-CBC fallback. +- `vitest.config.ts`, `commitlint.config.cjs` (`scope-enum`), `.claude/settings.json` (format/pre-commit hooks). + +### Feature Flags and Deployment Concerns + +**Definitive: no `--non-interactive`, `--ci`, `--no-input`, `--headless`, or `--batch` option exists anywhere in `src/` or `bin/`.** A targeted grep for those as commander `option(...)` registrations returns zero matches. + +The root program (`src/cli/index.ts`, `new Command()` L44) registers **exactly one** option: `.option('--task ', …)` at L61. Everything else at L80-106 is `program.addCommand(...)`. There is no root `--yes`, `--force`, or `--quiet`. + +Non-interactive detection census: +- **Predicates**: `src/utils/interactive.ts:15` (`!process.stdin.isTTY`, canonical); `src/agents/core/AgentCLI.ts:755-756` `shouldBlockNonInteractiveResume()` = `!process.stdin.isTTY || process.env.CODEMIE_NO_PROMPTS === '1'` (**the only predicate with an env escape hatch**); `src/cli/commands/skills/add.ts:52` `const interactive = !options.yes && process.stdin.isTTY === true` (**flag ∧ TTY — the closest thing to a `--non-interactive` flag**); `src/agents/core/AgentCLI.ts:176` `const isNonInteractiveMode = !!options.task`; `src/cli/commands/analytics/index.ts:154` — the only use of `stdout.isTTY` as a prompt gate. +- **Consumers**: `src/providers/core/auth-validation.ts:34` (sole consumer of the shared helper); `src/agents/core/AgentCLI.ts:707`; `src/cli/commands/skills/lib/agent-detection.ts:76-82`. Raw-mode guards (not prompt skips) at `shared/selection/interactive-prompt.ts:34,66`, `profile/index.ts:259,267`, `shared/agent-targets.ts:177,207`. +- **Forcing sites**: `src/cli/commands/skills/lib/run-skills-cli.ts:89` — `baseEnv.CI = process.env.CI ?? '1'` (note it *respects* an inherited `CI`), gated on `if (!interactive)` where interactive is the default (L64); the L84-90 comment explains that forcing `CI` on interactive runs interferes with Clack/inquirer prompts. Also `speckit.plugin.ts:162` (`--force`), `bmad.plugin.ts:213` (`--yes`), `native-installer.ts:102`, `processes.ts:161`, `claude.plugin.ts:656`. +- **Flag naming precedent**: `-y, --yes` "skip interactive confirmations" on subcommands — `skills/add.ts:46`, `skills/remove.ts:39`, `skills/update.ts:30`, `profile/index.ts:377`, `log/index.ts:144`; `--force` at `setup.ts:129`, `workflow.ts:155`, `proxy/index.ts:297,330,350`. Machine-output precedent `--json` at `proxy/index.ts:196`, `skills/find.ts:42`, `skills/list.ts:28`. + +**Verdict on AC 3**: introducing `--non-interactive` or `--ci` would be **novel and contrary to a recorded decision**. `docs/AUTHENTICATION.md:113` documents the flag's absence as intentional, and EPMCDME-13953's spec lists it as explicitly out of scope. AC 3 is worded "supported **or documented**" and is arguably **already satisfied by documentation**; the honest options are (a) cite the existing docs and close AC 3, or (b) if an opt-out is wanted, follow `CODEMIE_NO_PROMPTS=1` / `-y, --yes` rather than inventing a new global flag. + +Prompt-surface risk: there are **46 `inquirer.prompt` call sites across 19 non-test files**, and **only the `promptForReauth` path is guarded** by `isNonInteractiveEnvironment()`. Unguarded prompt-bearing modules include `providers/core/codemie-auth-helpers.ts`, `providers/plugins/jwt/jwt.setup-steps.ts`, `cli/commands/setup.ts`, `install.ts`, `update.ts`, `utils/cli-updater.ts`, `agents/core/BaseAgentAdapter.ts`, `assistants/chat/index.ts`. These are latent instances of the same class of bug, out of scope here but worth flagging. + +--- + +## 6. Risk Indicators + +- **Ticket premise is partly wrong.** `assistants.ts` *does* use `handleSdkError`; the defect is `await getSdkClient()` sitting outside the `try`. A plan written from the ticket text alone will fix the wrong thing. +- **Blast radius is ~50 actions across 8 files, not one.** `assistants.ts:65,115,166,198,220,240` plus 44 analogous lines in `categories/datasources/integrations/llm/skills/users/workflows`. Fixing only `assistants list` leaves 50 identical paths broken. PR #471's shared-gate precedent argues for fixing inside `getSdkClient()` (`cli-utils.ts:15-18`) rather than at 50 call sites. +- **Two independent defects must both be fixed.** Wrapping `getSdkClient` alone removes the stack trace but still prints the useless `'Authentication expired. Please re-authenticate.'`. Preserving the message alone still leaves the throw uncaught. AC 2 ("clear remediation") fails on *clear*, not on *non-zero* — the CLI already exits 1. +- **`ConfigurationError`'s constructor accepts only `message`** (`src/utils/errors.ts:8-13`). Preserving the upstream error via `{ cause }` requires a constructor change (touching a base class used repo-wide) or a post-hoc `.cause` assignment. Neither is established practice here. +- **`no-useless-catch` is an ESLint warn and the lint gate runs `--max-warnings=0`.** A naive catch-and-rethrow in `auth.ts` will fail CI. +- **`promptReauthentication`'s `Promise` return type is a lie** — it can only return `true` or throw, making `auth.ts:48`'s `if (reauthed)` false branch dead code (`auth.ts:52` unreachable via that route). Changing the throw to a `false` return would revive dead code paths and change `src/cli/commands/assistants/chat/index.ts:423` behaviour. +- **Regression pressure on an existing test.** `src/utils/__tests__/auth.test.ts:142-155` asserts the exact buggy message. Any message change breaks it; the test must be updated deliberately, not incidentally. +- **`src/cli/commands/sdk/**` has zero test coverage** across 22 files, including the choke point being modified. Tests there are greenfield — no local conventions to copy, must borrow from `skills/__tests__/commands.test.ts`. +- **Orphaned test directory.** Five test files under `tests/unit/` and `tests/skills/` match no vitest project glob and never run. A new unit test placed under `tests/unit/` would silently never execute. Unit tests must go in `src/**/__tests__/`. +- **No process-level error boundary exists.** `src/cli/index.ts:147` uses sync `program.parse` with no `.exitOverride()`; `bin/codemie.js`'s `.catch` covers only module-load rejections. Any future uncaught async rejection in any of ~29 command files prints a raw stack. A `process.on('unhandledRejection')` net (template at `bin/codemie-mcp-proxy.js:75-82`) would be defence in depth — but `bin/` is excluded from coverage and cannot be unit-tested. +- **`auth-validation.ts:39` writes the diagnostic to stdout via `console.log`**, polluting piped output and breaking `--json` consumers. Cosmetic but in scope-adjacent. +- **Layering.** `src/utils/auth.ts` (Utils) already reaches into `ProviderRegistry` (Core) and does chalk console output (L71), contradicting `architecture.md` L159-171 ("CLI catches, formats for user"). The fix must not deepen this — user-facing formatting belongs in `cli-utils.ts`. +- **AC 3 conflicts with a recorded decision.** EPMCDME-13953's spec put `--non-interactive`/`--ci` explicitly out of scope, and `docs/AUTHENTICATION.md:113` documents the absence as intentional. Adding the flag now would contradict shipped documentation; resolving AC 3 is a product decision, not an implementation one. +- **AC 4 (ERR_USE_AFTER_CLOSE on kill) is unproven, not disproven.** Two attempts failed to reproduce. The reproduction also observed an ora escape-sequence flood (73 MB capture) under `script`, attributed to a pty with no usable `stdout.columns` — flagged as needing a real-terminal recheck before being treated as a finding. Note `src/utils/sdk-client.ts:26` starts an ora spinner even in non-TTY, which is a plausible contributor. +- **The same class of bug is latent in 45 other prompt sites.** Only `promptForReauth` is TTY-guarded; 46 `inquirer.prompt` call sites exist across 19 non-test files. +- **`docs/AUTHENTICATION.md:104-117` currently makes a promise the CLI does not keep.** If the fix changes the emitted message, that section must be updated in the same change — PR #471's precedent makes the doc update part of the deliverable, and CR-001 on the prior ticket was closed *specifically* on that doc addition. +- **Commit scope constraint**: `fix(auth)` / `fix(sdk)` are **not** in commitlint's `scope-enum` and will be rejected. Use `fix(cli)` or `fix(utils)`. +- **`cli-utils.ts` uses double quotes** against the guide's single-quote rule; ESLint does not enforce quote style. Do not mass-reformat — it would balloon the diff. +- **Repo policy: "Tests Only On Explicit Request"** (AGENTS.md). Coverage thresholds in the testing guide are advisory; `vitest.config.ts` configures **no** thresholds. + +--- + +## 7. Summary for Complexity Assessment + +**Layers and file surface.** The task touches three layers: CLI (`src/cli/commands/sdk/utils/cli-utils.ts`, and potentially 8 sdk command files), Utils (`src/utils/auth.ts`, possibly `src/utils/errors.ts`), and Core (`src/providers/core/auth-validation.ts`, a one-line stdout→stderr change). The minimal correct implementation is small — the entire defect funnels through a single 4-line function, `getSdkClient()` at `cli-utils.ts:15-18`, which is the sole bridge from all ~50 sdk actions into the auth path. A shared-gate fix there plus a message-preservation change in `auth.ts:76` is roughly **2-3 source files, well under 100 changed lines**. The tempting alternative — moving `await getSdkClient()` inside the `try` in each action — is ~50 mechanical edits across 8 files and should be rejected in favour of the gate, consistent with the precedent set by PR #471. Documentation (`docs/AUTHENTICATION.md:104-117`) must be updated in the same change; the prior ticket's code review was closed specifically on that doc surface, so omitting it repeats a known review failure. + +**Technical novelty: low, but the design space has traps.** Every ingredient already exists in-repo. `isNonInteractiveEnvironment()` is the sanctioned detector; `handleSdkError`, `failAuth` (`skills/lib/require-auth.ts:24-51`), `printProxyError` and `handleSetupError` are four existing `never`-returning error sinks, and `require-auth.ts`'s `NOT_AUTHENTICATED_MESSAGE` is verbatim the actionable text the ticket asks for. Nothing needs inventing. The traps are: (1) `ConfigurationError`'s constructor takes only `message`, so preserving the upstream error as `cause` means touching a repo-wide base class; (2) ESLint's `no-useless-catch` warn under `--max-warnings=0` makes a naive catch-and-rethrow a CI failure; (3) `promptReauthentication`'s declared `Promise` is unreachable-`false`, so switching from throw to return revives dead code and changes behaviour at `assistants/chat/index.ts:423`; (4) AC 3 (`--non-interactive`/`--ci`) contradicts an explicit recorded out-of-scope decision from EPMCDME-13953 and shipped text in `docs/AUTHENTICATION.md:113` — it is a product call, not an implementation one, and should be escalated rather than implemented unilaterally. + +**Test posture: mixed, tilting bad at the point of change.** `src/utils/auth.ts` and `src/providers/core/auth-validation.ts` are well covered, and `src/utils/__tests__/auth.test.ts:142-155` currently asserts the *buggy* message — so a fix necessarily edits an existing green test, which reviewers must not mistake for a regression. Conversely `src/cli/commands/sdk/**` has **zero tests across all 22 files**, so tests for the primary fix location are greenfield. The templates are unambiguous: `src/providers/core/__tests__/auth-validation.test.ts` for mocking the non-interactive seam (mock the function, not `process.stdin`), `src/cli/commands/skills/__tests__/commands.test.ts` for the `process.exit` spy-and-throw pattern, and `tests/helpers/cli-runner.ts`'s `runSilent()` for an end-to-end non-zero-exit assertion with stdin redirected. Two landmines: unit tests must live in `src/**/__tests__/` because five existing files under `tests/unit/` match no vitest glob and silently never run; and `bin/` is excluded from coverage, so any process-level `unhandledRejection` net cannot be unit-tested and needs a cli-project subprocess test. + +**Risk factors for scoring.** Complexity is inflated above the raw line count by: a partially incorrect ticket premise that must be corrected before planning; a 50-call-site blast radius that makes "fix the reported symptom" the wrong answer; one acceptance criterion (AC 3) that conflicts with a documented prior decision; another (AC 4) that was never reproduced and may not be a real defect; a required change to an existing passing test; zero test coverage at the primary fix location; and a mandatory documentation update. Nothing here is architecturally hard — the difficulty is entirely in scoping discipline and in not regressing the four adjacent auth paths (`assistants/chat`, `assistants/setup`, `skills/setup`, `AgentCLI`) that already handle this correctly. From d90576c1dbca59d6f25023a6e5f24965171a56a8 Mon Sep 17 00:00:00 2001 From: SleepySML Date: Thu, 3 Sep 2026 20:48:40 +0300 Subject: [PATCH 05/16] fix(cli): install guards per entrypoint and make the hang test able to fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the review check round, which found two defects introduced by the previous fix-up. Installing the guards from the AgentCLI constructor meant merely constructing an AgentCLI mutated global process state: listener counts for uncaughtException and unhandledRejection went 1 -> 2, and four unit test files construct AgentCLI directly. Any later unhandled rejection in that worker would hard-exit instead of surfacing as a test failure. Moving the call to AgentCLI.run() would not have helped either, since tests drive run() as the seam. The guard is now installed in each of the 11 agent entrypoints alongside the existing bin/codemie.js call, so construction stays side-effect free. Daemons are deliberately left out: exiting on the first unhandled rejection is wrong for a long-running process, and codemie-mcp-proxy already owns handlers with a survive-on-rejection policy. The 15s execSync timeout stopped CI wedging but did not make a hang detectable. On ETIMEDOUT execSync reports status: null, which runSilent collapsed to exitCode 1, and stderr already held the remediation printed before the block — so all four cases went green during a genuine hang, including the one named for it. Verified directly against a child that prints then hangs. CommandResult now carries timedOut and the test asserts it, so the regression these tests exist to catch is detectable. Also corrects two internal contradictions in reproduction.md: Case D claimed signals were sent "while parked on the prompt", which the later pty work disproved, and the AC table still cited retracted Case C as evidence. EPMCDME-14148 Co-Authored-By: Claude --- bin/agent-executor.js | 7 +++++++ bin/codemie-claude-acp.js | 7 +++++++ bin/codemie-claude.js | 7 +++++++ bin/codemie-codex.js | 7 +++++++ bin/codemie-copilot.js | 7 +++++++ bin/codemie-gemini.js | 7 +++++++ bin/codemie-kimi-acp.js | 7 +++++++ bin/codemie-kimi.js | 7 +++++++ bin/codemie-opencode.js | 7 +++++++ bin/codemie-openwiki.js | 7 +++++++ bin/codemie-pi.js | 7 +++++++ .../reproduction.md | 6 ++++-- src/agents/core/AgentCLI.ts | 6 ------ tests/helpers/cli-runner.ts | 8 ++++++++ .../integration/cli-commands/non-interactive-auth.test.ts | 7 +++++++ 15 files changed, 96 insertions(+), 8 deletions(-) diff --git a/bin/agent-executor.js b/bin/agent-executor.js index 8d6a448f6..df08667fa 100755 --- a/bin/agent-executor.js +++ b/bin/agent-executor.js @@ -10,6 +10,13 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + // Load built-in agent (codemie-code) const agent = AgentRegistry.getAgent('codemie-code'); diff --git a/bin/codemie-claude-acp.js b/bin/codemie-claude-acp.js index da6c9211d..e31795a02 100755 --- a/bin/codemie-claude-acp.js +++ b/bin/codemie-claude-acp.js @@ -10,6 +10,13 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + const agent = AgentRegistry.getAgent('claude-acp'); if (!agent) { diff --git a/bin/codemie-claude.js b/bin/codemie-claude.js index d8edfffe7..6b2a3c38b 100755 --- a/bin/codemie-claude.js +++ b/bin/codemie-claude.js @@ -7,6 +7,13 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + const agent = AgentRegistry.getAgent('claude'); if (!agent) { diff --git a/bin/codemie-codex.js b/bin/codemie-codex.js index 8ce77c3cf..42a564ab6 100755 --- a/bin/codemie-codex.js +++ b/bin/codemie-codex.js @@ -7,6 +7,13 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + const agent = AgentRegistry.getAgent('codex'); if (!agent) { diff --git a/bin/codemie-copilot.js b/bin/codemie-copilot.js index b7f030b28..75ebc9cc4 100755 --- a/bin/codemie-copilot.js +++ b/bin/codemie-copilot.js @@ -14,6 +14,13 @@ import { AgentRegistry } from '../dist/agents/registry.js'; import { resolveCopilotModel } from '../dist/agents/plugins/copilot-cli/index.js'; import { ConfigLoader } from '../dist/utils/config.js'; import { getCodemiePath } from '../dist/utils/paths.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + const SAVED_MODEL_PATH = getCodemiePath('agents', 'copilot-cli', 'model.json'); diff --git a/bin/codemie-gemini.js b/bin/codemie-gemini.js index b8de0e033..c534480eb 100755 --- a/bin/codemie-gemini.js +++ b/bin/codemie-gemini.js @@ -7,6 +7,13 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + const agent = AgentRegistry.getAgent('gemini'); if (!agent) { diff --git a/bin/codemie-kimi-acp.js b/bin/codemie-kimi-acp.js index 94f9b36f6..fa84062d7 100755 --- a/bin/codemie-kimi-acp.js +++ b/bin/codemie-kimi-acp.js @@ -7,6 +7,13 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + const agent = AgentRegistry.getAgent('kimi-acp'); if (!agent) { diff --git a/bin/codemie-kimi.js b/bin/codemie-kimi.js index 11795dc4c..d6d32babf 100755 --- a/bin/codemie-kimi.js +++ b/bin/codemie-kimi.js @@ -7,6 +7,13 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + const agent = AgentRegistry.getAgent('kimi'); if (!agent) { diff --git a/bin/codemie-opencode.js b/bin/codemie-opencode.js index 318094847..a7860a4b8 100755 --- a/bin/codemie-opencode.js +++ b/bin/codemie-opencode.js @@ -7,6 +7,13 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + const agent = AgentRegistry.getAgent('opencode'); if (!agent) { diff --git a/bin/codemie-openwiki.js b/bin/codemie-openwiki.js index 1ef7dbb9f..e0fea88fb 100755 --- a/bin/codemie-openwiki.js +++ b/bin/codemie-openwiki.js @@ -7,6 +7,13 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + const agent = AgentRegistry.getAgent('openwiki'); if (!agent) { diff --git a/bin/codemie-pi.js b/bin/codemie-pi.js index 6ad4a4e19..c98b38e17 100755 --- a/bin/codemie-pi.js +++ b/bin/codemie-pi.js @@ -7,6 +7,13 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +// Last-line-of-defence net for async rejections that escape a command action. +// Installed per entrypoint rather than in the AgentCLI constructor, so merely +// constructing an AgentCLI (as unit tests do) never mutates global process state. +installProcessGuards(); + const agent = AgentRegistry.getAgent('pi'); if (!agent) { diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md index d6781e936..cf0baf904 100644 --- a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md @@ -60,7 +60,9 @@ The guard is still confirmed working — but by `auth-validation.test.ts` and by ### Case D — AC4: kill during the prompt -Ran under a pty, sent `SIGINT` then `SIGTERM` while parked on the prompt. **No `ERR_USE_AFTER_CLOSE` and no readline lifecycle error** appeared across two attempts. +Ran under a pty and sent `SIGINT` then `SIGTERM`. **No `ERR_USE_AFTER_CLOSE` and no readline lifecycle error** appeared across two attempts. + +**Corrected:** an earlier draft described these signals as being sent "while parked on the prompt". That was an assumption, not an observation, and the later node-pty work disproved it — the process exits before any signal is delivered and the prompt is never reached. See `ac4-investigation.md`; the criterion was never exercised. Incidental observation, **not** confirmed as a product bug: after the interrupt the `ora` spinner emitted a runaway stream of cursor-control escapes (`ESC[1A ESC[0K`), producing a 73 MB capture. This is most likely an artifact of `script` giving the child a pty with no usable `stdout.columns`, which breaks ora's line-clearing arithmetic. It should be re-checked in a real terminal before anyone treats it as a finding. @@ -107,7 +109,7 @@ Note that `handleSdkError`'s `else` branch **already** renders `ConfigurationErr | AC | Verdict | Evidence | |---|---|---| -| Non-TTY stdin skips interactive prompt | **Met already** | Case A/B (0–1 s, no prompt) vs Case C (blocks with TTY) | +| Non-TTY stdin skips interactive prompt | **Met already** | Cases A/B exit in 0–1 s without prompting, plus the `auth-validation.test.ts` guard tests. **Not** Case C, which this document retracts as a `script(1)` artifact | | CLI exits non-zero with clear remediation | **Not met** | Case A/B: exit 1 but raw stack trace; message lacks `codemie setup` | | `--non-interactive` / `--ci` supported or documented | **Partially met** | No such flag in `src/`, but the absence is documented at `docs/AUTHENTICATION.md:113`. The AC reads "supported **or** documented". Not a clean pass: the documented mechanism (`stdin` TTY only) has a blind spot for pty-allocating CI — now stated explicitly in that doc | | Kill during prompt produces no readline crash | **Not exercised** | The prompt was never reached in any attempt, so the criterion was never tested — see `ac4-investigation.md` | diff --git a/src/agents/core/AgentCLI.ts b/src/agents/core/AgentCLI.ts index 3a8e1ecc2..e79f8b747 100644 --- a/src/agents/core/AgentCLI.ts +++ b/src/agents/core/AgentCLI.ts @@ -10,7 +10,6 @@ import { AuthMethod, ProviderName } from '../../providers/core/types.js'; import { JWTTemplate } from '../../providers/plugins/jwt/jwt.template.js'; import { logger } from '../../utils/logger.js'; import { getDirname } from '../../utils/paths.js'; -import { installProcessGuards } from '../../utils/process-guards.js'; import { BUILTIN_AGENT_NAME } from '../registry.js'; import { ClaudePluginMetadata } from '../plugins/claude/claude.plugin.js'; import { CodeMieCodePluginMetadata } from '../plugins/codemie-code.plugin.js'; @@ -37,11 +36,6 @@ export class AgentCLI { private version: string = '1.0.0'; constructor(private adapter: AgentAdapter) { - // Every bin/codemie- entrypoint reaches the CLI through here rather - // than bin/codemie.js, so the guards are installed here to cover them too - // (EPMCDME-14148). installProcessGuards() is idempotent. - installProcessGuards(); - this.program = new Command(); this.loadVersion(); this.setupProgram(); diff --git a/tests/helpers/cli-runner.ts b/tests/helpers/cli-runner.ts index e62b979c4..834ea1f42 100644 --- a/tests/helpers/cli-runner.ts +++ b/tests/helpers/cli-runner.ts @@ -11,6 +11,13 @@ export interface CommandResult { output: string; exitCode: number; error?: string; + /** + * True when the child was killed by the `timeout` option rather than exiting + * on its own. execSync reports ETIMEDOUT with `status: null`, which would + * otherwise collapse to exitCode 1 and be indistinguishable from a clean + * failure — letting a hang masquerade as a passing test. + */ + timedOut?: boolean; } export class CLIRunner { @@ -54,6 +61,7 @@ export class CLIRunner { output, exitCode: error.status || 1, error: errorOutput, + timedOut: error.code === 'ETIMEDOUT' || error.signal === 'SIGTERM', }; } } diff --git a/tests/integration/cli-commands/non-interactive-auth.test.ts b/tests/integration/cli-commands/non-interactive-auth.test.ts index 790f0fcbb..35ec3bb42 100644 --- a/tests/integration/cli-commands/non-interactive-auth.test.ts +++ b/tests/integration/cli-commands/non-interactive-auth.test.ts @@ -39,6 +39,13 @@ describe('non-interactive SSO auth failure', () => { // the original hang would wedge CI instead of failing here. timeout: 15_000, }); + + // On ETIMEDOUT execSync reports status: null, which runSilent collapses to + // exitCode 1 — and stderr already holds whatever was printed before the + // block. Without this guard a genuine 15s hang passes every assertion + // below, including the one named for it. + expect(result.timedOut ?? false).toBe(false); + return { ...result, combined: `${result.output}\n${result.error ?? ''}` }; } From 9c243a7b5d9622705c4c108e83a2c68c3de0415a Mon Sep 17 00:00:00 2001 From: SleepySML Date: Thu, 3 Sep 2026 20:50:11 +0300 Subject: [PATCH 06/16] docs(cli): record check-round verdict and close review residuals Adds code-review-check.json: CR-001..CR-005 resolved, plus CR-006 and CR-007 which the check round found were introduced by the first fix-up. Closes three residuals the checker raised: the logfile test now asserts its log path instead of silently early-returning (it could otherwise pass vacuously where log init fails), and AUTHENTICATION.md no longer implies CI always means no TTY. EPMCDME-14148 Co-Authored-By: Claude --- docs/AUTHENTICATION.md | 2 +- .../code-review-check.json | 57 +++++++++++++++++++ .../__tests__/process-guards.logfile.test.ts | 13 ++--- 3 files changed, 64 insertions(+), 8 deletions(-) create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-check.json diff --git a/docs/AUTHENTICATION.md b/docs/AUTHENTICATION.md index 31eb75447..1e50f946d 100644 --- a/docs/AUTHENTICATION.md +++ b/docs/AUTHENTICATION.md @@ -104,7 +104,7 @@ codemie setup # Run wizard again ### Non-Interactive Environments (CI/Automation) When SSO credentials are missing or expired, the CLI normally offers an interactive -re-authentication prompt. In a non-interactive environment — no TTY attached to `stdin`, as in CI +re-authentication prompt. In a non-interactive environment — no TTY attached to `stdin`, as in most CI pipelines, cron jobs, or piped/redirected invocations — that prompt is automatically skipped. The CLI detects the missing TTY, fails fast, and exits non-zero instead of hanging. diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-check.json b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-check.json new file mode 100644 index 000000000..0c9942f9a --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-check.json @@ -0,0 +1,57 @@ +{ + "decision": "request-changes", + "rationale": "All five original findings are resolved, three of them verified adversarially by an independent checker that re-derived the evidence rather than trusting the author. The check round also found two NEW major defects introduced by the first fix-up: installing the guards from the AgentCLI constructor mutated global process state (listener counts measured going 1->2, with four unit test files constructing AgentCLI), and the added execSync timeout stopped CI wedging without making a hang detectable (ETIMEDOUT yields status: null, which collapsed to exitCode 1 while stderr already held the remediation, so all four cases passed during a genuine 15s hang). Both were fixed and each fix was empirically demonstrated rather than asserted. The decision remains request-changes for one reason only: those two fixes were authored AFTER the independent check and have not themselves been independently verified, and the review contract forbids a third automated round. This is an escalation for a human decision, not an unresolved defect list. AC4 remains unmet by design and is recommended for a separate ticket.", + "confidence": "medium", + "risk_flags": ["auth", "acceptance-criteria-unmet", "unverified-fixup"], + "business_review": [ + {"id": "AC1", "criterion": "Non-TTY stdin skips interactive prompt.", "status": "pass", "notes": "Carried forward. Regression coverage is now genuinely able to fail: CR-005's timedOut assertion closes the hole where a hang passed every assertion."}, + {"id": "AC2", "criterion": "CLI exits non-zero with clear remediation.", "status": "pass", "notes": "Carried forward. Re-confirmed end-to-end after both fix-up rounds: exit 1, single actionable stderr line, no stack trace."}, + {"id": "AC3", "criterion": "Optional --non-interactive or --ci behavior is supported or documented.", "status": "partial", "notes": "Carried forward, improved by CR-004. The documentation now states the pty-CI boundary explicitly instead of implying completeness. Remains 'documented' rather than 'supported' per the EPMCDME-13953 decision and the user's scope call."}, + {"id": "AC4", "criterion": "Killing during prompt does not produce readline lifecycle crash.", "status": "fail", "notes": "Unchanged and unmet. Reclassified honestly as 'precondition not constructed, criterion never exercised' with a concrete reproduction recipe carried forward. Recommended for a separate ticket."} + ], + "standards_review": [ + {"standard": "git-workflow.md - Conventional Commits, scope-enum", "status": "pass", "notes": "Carried forward. All five commits use fix(cli)/docs(cli), both in scope-enum."}, + {"standard": "code-quality.md - ESLint --max-warnings=0", "status": "pass", "notes": "Carried forward; re-run clean after both fix-up rounds."}, + {"standard": "code-quality.md - explicit return types, .js extensions", "status": "pass", "notes": "Carried forward; typecheck clean."}, + {"standard": "testing-patterns.md - unit tests co-located in src/**/__tests__/", "status": "pass", "notes": "Carried forward. The new logfile companion suite is also correctly placed."}, + {"standard": "development-practices.md - do not log stack traces to console; log errors with logger.error", "status": "pass", "notes": "Was fail. Now resolved: the stack reaches the log file (proven by reverting the fix and watching the new test fail with ENOENT) while the console keeps only the actionable line."} + ], + "finding_status": [ + { + "id": "CR-001", + "status": "resolved", + "notes": "Independently verified. The checker reverted reportFatal to the object-literal form and confirmed the new logfile test fails with ENOENT, proving persistFatalSync is load-bearing and the test is non-vacuous. sanitizeLogArgs index confirmed correct; append flag 'a' on both writers confirmed atomic w.r.t. offset. Two residuals raised by the checker were then also closed: the logfile test now asserts logPath is truthy instead of silently early-returning, so it can no longer pass vacuously." + }, + { + "id": "CR-002", + "status": "resolved", + "notes": "Independently verified: the false spinner-mitigation claim is gone, surviving only inside an explicitly labelled retraction that quotes it and states nothing in this MR mitigates AC4. The checker grepped both documents for any surviving sentence implying mitigation and found none. Two internal contradictions it flagged were then also fixed: Case D no longer claims signals were sent 'while parked on the prompt' (an assumption the pty work disproved), and the AC table no longer cites retracted Case C as evidence." + }, + { + "id": "CR-003", + "status": "resolved", + "notes": "Was partially-resolved. The checker confirmed all 11 agent entrypoints reach the AgentCLI constructor, closing the stated coverage gap, but measured that construction now mutated global process state - see CR-006. Re-fixed by installing per entrypoint instead. Note the checker's suggested alternative (move to AgentCLI.run()) was independently checked and rejected: tests drive run() as their seam, so it would not have helped." + }, + { + "id": "CR-004", + "status": "resolved", + "notes": "Independently verified as accurate against src/utils/interactive.ts and judged prominent rather than hedged. One residual overstatement the checker flagged upstream of the callout ('as in CI pipelines' implying CI-implies-no-TTY) has since been softened to 'as in most CI pipelines'." + }, + { + "id": "CR-005", + "status": "resolved", + "notes": "Was partially-resolved. The checker confirmed the timeout survives the options spread (CI no longer wedges) but empirically demonstrated that a hang still passed all four assertions - see CR-007. Re-fixed; independently reproduced before and after." + }, + { + "id": "CR-006", + "status": "resolved", + "notes": "NEW, introduced by the first fix-up. installProcessGuards() in the AgentCLI constructor made construction mutate global process state; the checker measured uncaughtException and unhandledRejection listener counts going 1->2 inside a Vitest worker, with four unit test files constructing AgentCLI directly. A later unhandled rejection would hard-exit the worker instead of surfacing as a test failure. Fixed by installing in each of the 11 agent entrypoints; AgentCLI reverted to zero references. Daemons deliberately excluded - exiting on first unhandled rejection is wrong for a long-running process, and codemie-mcp-proxy owns handlers with a survive-on-rejection policy. NOT independently re-verified." + }, + { + "id": "CR-007", + "status": "resolved", + "notes": "NEW, introduced by the first fix-up. The execSync timeout prevented CI wedging but left a hang undetectable: ETIMEDOUT reports status null, runSilent's `error.status || 1` collapsed it to 1, and stderr already carried the remediation printed before the block, so all four cases passed during a real hang - including the one named for it. Reproduced directly against a child that prints then hangs, confirming the old assertions pass and the new guard catches it. Fixed by adding timedOut to CommandResult and asserting it. NOT independently re-verified." + } + ], + "findings": [] +} diff --git a/src/utils/__tests__/process-guards.logfile.test.ts b/src/utils/__tests__/process-guards.logfile.test.ts index 60e00ef8a..6fd1b1cbc 100644 --- a/src/utils/__tests__/process-guards.logfile.test.ts +++ b/src/utils/__tests__/process-guards.logfile.test.ts @@ -39,14 +39,13 @@ describe('installProcessGuards log-file persistence', () => { const { logger } = await import('../logger.js'); const { installProcessGuards } = await import('../process-guards.js'); + // Asserted rather than early-returned: a silent `return` here would make + // this test pass vacuously in any environment where log init fails, which + // is exactly the coverage this file exists to provide. const logPath = logger.getLogFilePath(); - // Logging to file is best-effort; if this environment has no log path there - // is nothing to assert against. - if (!logPath) { - return; - } + expect(logPath).toBeTruthy(); - const before = existsSync(logPath) ? readFileSync(logPath, 'utf-8') : ''; + const before = existsSync(logPath!) ? readFileSync(logPath!, 'utf-8') : ''; installProcessGuards(); @@ -55,7 +54,7 @@ describe('installProcessGuards log-file persistence', () => { expect(() => handlers.uncaughtException(boom)).toThrow('process.exit:1'); - const after = readFileSync(logPath, 'utf-8'); + const after = readFileSync(logPath!, 'utf-8'); const appended = after.slice(before.length); expect(appended).toContain(marker); From 36dd7597d599a8ee617bf444f5e831a61c8a379d Mon Sep 17 00:00:00 2001 From: SleepySML Date: Thu, 3 Sep 2026 21:00:25 +0300 Subject: [PATCH 07/16] test(cli): tighten hang detection and drop the false-positive disjunct Independent verification of the CR-006 and CR-007 fixes confirmed both hold, and raised three minor points now addressed. The timedOut disjunct on signal === 'SIGTERM' was redundant: a genuine timeout always sets code ETIMEDOUT, including when the child ignores SIGTERM. The disjunct only misreported an unrelated SIGTERM death as a hang, demonstrated firing at 29ms. Reduced to the ETIMEDOUT check. The timeout only catches a hang that runs the full 15s, so a shorter stall would still pass every assertion and merely slow the suite. Adds a wall-clock bound with roughly 18x headroom over the observed runtime. Corrects the process-guards comment, which still claimed AgentCLI installs the guard - it has not since db0589af. EPMCDME-14148 Co-Authored-By: Claude --- src/utils/process-guards.ts | 5 +++-- tests/helpers/cli-runner.ts | 5 ++++- .../integration/cli-commands/non-interactive-auth.test.ts | 7 +++++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/utils/process-guards.ts b/src/utils/process-guards.ts index d91da7daa..fc84f2178 100644 --- a/src/utils/process-guards.ts +++ b/src/utils/process-guards.ts @@ -65,8 +65,9 @@ let installed = false; * Register process-level handlers for unhandled rejections and uncaught * exceptions so they surface as a formatted message rather than a stack trace. * - * Idempotent: both bin/codemie.js and AgentCLI call it, and they can share a - * process, which would otherwise stack duplicate handlers. + * Called from each bin/* entrypoint rather than from AgentCLI, so constructing + * an AgentCLI never mutates global process state. Idempotent as a guard against + * one process loading more than one entrypoint. */ export function installProcessGuards(): void { if (installed) { diff --git a/tests/helpers/cli-runner.ts b/tests/helpers/cli-runner.ts index 834ea1f42..10ba2f797 100644 --- a/tests/helpers/cli-runner.ts +++ b/tests/helpers/cli-runner.ts @@ -61,7 +61,10 @@ export class CLIRunner { output, exitCode: error.status || 1, error: errorOutput, - timedOut: error.code === 'ETIMEDOUT' || error.signal === 'SIGTERM', + // ETIMEDOUT alone covers every timeout mode, including a child that + // ignores SIGTERM. Testing signal === 'SIGTERM' as well adds nothing and + // misreports an unrelated SIGTERM death as a hang. + timedOut: error.code === 'ETIMEDOUT', }; } } diff --git a/tests/integration/cli-commands/non-interactive-auth.test.ts b/tests/integration/cli-commands/non-interactive-auth.test.ts index 35ec3bb42..cd3659c7d 100644 --- a/tests/integration/cli-commands/non-interactive-auth.test.ts +++ b/tests/integration/cli-commands/non-interactive-auth.test.ts @@ -30,6 +30,7 @@ describe('non-interactive SSO auth failure', () => { 'SSO authentication required. Please run "codemie setup" with SSO provider first.'; function runWithoutTty(command: string) { + const startedAt = Date.now(); const result = runner.runSilent(command, { env: { ...process.env, CODEMIE_HOME: isolatedHome }, // stdin from 'ignore' is not a TTY, which is the condition under test. @@ -46,6 +47,12 @@ describe('non-interactive SSO auth failure', () => { // below, including the one named for it. expect(result.timedOut ?? false).toBe(false); + // The timeout only catches a hang that runs the full 15s. A regression that + // blocked for a few seconds would still satisfy every assertion below and + // merely slow the suite. Observed runtime is ~280ms, so this leaves ~18x + // headroom while still catching a stall. + expect(Date.now() - startedAt).toBeLessThan(5_000); + return { ...result, combined: `${result.output}\n${result.error ?? ''}` }; } From 09bcc53af381fdcec665c24d8bb2eaaf84bac7a7 Mon Sep 17 00:00:00 2001 From: SleepySML Date: Thu, 3 Sep 2026 22:49:17 +0300 Subject: [PATCH 08/16] docs(cli): add QA gate report for EPMCDME-14148 Gates: license, lint, typecheck, build, unit (3969/3969), commitlint all pass. UI gate skipped - no UI surface in the diff. Two gates are owed to CI rather than passing locally, and the report says so rather than rounding up. The secrets scan self-skipped with 'No staged changes to scan' because it reads the staged diff and the tree is clean; it is recorded SKIPPED, not PASS, though the pre-commit hook did run gitleaks against every commit on the branch. The full integration suite cannot complete in this environment - doctor.test.ts and list.test.ts both hang - which was verified pre-existing by running doctor against origin/main's entrypoint inside the repo, where it hangs identically. The four integration files that do complete, including the new AC2 regression suite and the three that exercise bin/codemie.js, all pass. EPMCDME-14148 Co-Authored-By: Claude --- .../qa-report.md | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md new file mode 100644 index 000000000..c647c3c92 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md @@ -0,0 +1,66 @@ +# QA Gate Report — EPMCDME-14148 + +**Branch**: `EPMCDME-14148` +**Merge base**: `origin/main` @ `1d5cc22b` +**Runner**: npm (guide-first from `.ai-run/guides/quality-gates.md`) +**Started**: 2026-09-03T18:05:00Z +**Status**: PASSED (with two SKIPPED gates owed to CI — see below) + +## Gates + +| Gate | Source | Status | Duration | Command | Notes | +|---|---|---|---|---|---| +| license | guide | PASS | 8s | `npm run license-check` | Required `npm_config_cache` override; `~/.npm/_cacache` is not writable in this environment | +| lint | guide | PASS | 2s | `npm run lint` | `--max-warnings=0`; `no-useless-catch` did not fire on either new catch | +| typecheck | guide | PASS | 2s | `npm run typecheck` | | +| build | guide | PASS | 2s | `npm run build` | | +| unit | guide | PASS | 6s | `npm run test:unit` | **3969 passed / 3969**, 269 files, 0 failures | +| integration | guide + ci | **PARTIAL** | — | `npm run test:integration` | See below — pre-existing local hangs, not caused by this branch | +| commitlint | guide + ci | PASS | 0s | `npx commitlint --from origin/main --to HEAD --verbose` | 0 problems, 0 warnings across all 7 commits | +| secrets | hook | **SKIPPED** | 0s | `npm run validate:secrets` | Self-skipped — see below | +| affected | guide | N/A | — | — | No changed-file-aware command configured in this project | +| ui | guide | SKIPPED | — | — | No UI surface changed — green outcome | + +## Integration — partial, and why + +The `cli` vitest project cannot complete locally in this environment. Confirmed hanging files: `tests/integration/cli-commands/doctor.test.ts` and `tests/integration/cli-commands/list.test.ts`; the full-project run produced no output at all across two attempts (600s and ~10 min). + +**This is pre-existing and not caused by this branch.** Verified directly: `codemie doctor` was run against both the branch's `bin/codemie.js` and `origin/main`'s version inside the repo, and **both hang identically at 40s**. An earlier comparison that appeared to implicate this branch was invalid (main's `bin/codemie.js` had been copied outside the repo, so its `../dist/...` imports could not resolve and it died instantly on module-not-found rather than actually running). + +Files that do complete locally, all passing: + +| File | Result | +|---|---| +| `cli-commands/non-interactive-auth.test.ts` | 4 / 4 passed — the AC2 regression suite added by this change | +| `cli-commands/version.test.ts` | 2 / 2 passed | +| `cli-commands/help.test.ts` | 2 / 2 passed | +| `cli-commands/error-handling.test.ts` | 2 / 2 passed | + +The `version` / `help` / `error-handling` files are the ones that exercise `bin/codemie.js`, which this change modifies, so the entrypoint change is covered by the files that do run. The remainder is owed to CI, which runs `npm run test:integration` in a clean container. + +## Secrets — SKIPPED, not PASS + +The gate exited 0 in 0s. Per the self-skipping rule that is not a pass. Exact output: + +``` +No staged changes to scan +``` + +`scripts/validate-secrets.js` scans the **staged** git diff, and the working tree is clean because all work is committed — so it did no work. + +**Coverage is nonetheless real**: `.husky/pre-commit` chains `npm run validate:secrets`, so gitleaks ran against the staged diff of **every one of the 7 commits** on this branch during the session, reporting `no leaks found` each time. To run it here deliberately you would need staged changes present (and podman running, which it is). + +Worth flagging: this gate is **hook-only**. `npm run ci` is `license-check && lint && build && test:unit && test:integration` — it does **not** include `validate:secrets`, so CI will not re-run it. + +## Additional verification beyond the gate list + +- All 12 CLI entrypoints (`bin/codemie.js` + 11 agent binaries) execute `--version` successfully, exit 0 — confirming the new `process-guards` import resolves post-build in every one. +- End-to-end acceptance behaviour re-confirmed after every fix-up round: exit 1, single actionable stderr line, no stack trace. + +## Drift signal + +**no** — the implementation matches `spec.md`. Every symbol the spec names (`getSdkClient`, `handleSdkError`, `promptReauthentication`, `isNonInteractiveEnvironment`, `installProcessGuards`) exists with the described signature. The one deliberate deviation from the spec's first draft — installing the guards per bin entrypoint rather than from `AgentCLI` — was made in response to code-review finding CR-006 and is recorded in `code-review-check.json`. + +## Outcome + +`PASSED`. Nothing local blocks this branch. Two gates are owed to CI: the full integration suite (locally unrunnable, pre-existing) and the secrets scan (hook-only, already run per-commit). From 2fc88c9f9a2c444ed29c3caacbb3144e9492f7ec Mon Sep 17 00:00:00 2001 From: SleepySML Date: Thu, 3 Sep 2026 22:52:14 +0300 Subject: [PATCH 09/16] docs(cli): record actual complexity for EPMCDME-14148 Actual L (23/36) against an initial estimate of M (19/36) - one band low, delta +4. The gap is concentrated in Technical Risk, driven by two hazards that were invisible at planning time and only surfaced during implementation: logger writes through an fs.WriteStream that process.exit() does not drain, so the fatal record was silently lost on a cold stream; and runSilent wraps execSync, which blocks the Vitest worker synchronously so testTimeout cannot interrupt it, meaning a regression to the original hang would have wedged CI rather than failed a test. EPMCDME-14148 Co-Authored-By: Claude --- .../actual-complexity.json | 118 ++++++++++++++++++ .../events.jsonl | 2 + 2 files changed, 120 insertions(+) create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/actual-complexity.json diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/actual-complexity.json b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/actual-complexity.json new file mode 100644 index 000000000..708c5fa5c --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/actual-complexity.json @@ -0,0 +1,118 @@ +{ + "task": "Make non-interactive (non-TTY) SSO authentication failure fail fast and cleanly across the CLI — exit non-zero with an actionable stderr message instead of hanging on a re-auth prompt or printing a raw stack trace — via a new process-level guard module installed in every bin/ entrypoint plus TTY-aware auth, SDK-client and provider-validation paths.", + "generated": "2026-09-03T00:00:00Z", + "dimensions": { + "component_scope": { + "score": 5, + "label": "XL", + "affected": "new process-guard module (src/utils/process-guards.ts), shared auth utility (src/utils/auth.ts), SDK client bootstrap (src/utils/sdk-client.ts), provider-core auth validation (src/providers/core/auth-validation.ts), SDK CLI command utils (src/cli/commands/sdk/utils/cli-utils.ts), all 12 bin/ entrypoints (codemie, agent-executor, claude, claude-acp, codex, copilot, gemini, kimi, kimi-acp, opencode, openwiki, pi), integration test harness (tests/helpers/cli-runner.ts), docs/AUTHENTICATION.md", + "layers": "Process/entrypoint bootstrap (bin/), Service (auth + sdk-client utils), Provider-core (SSO auth validation), CLI command utils, Test infrastructure, Docs" + }, + "requirements_clarity": { + "score": 3, + "label": "M", + "status": "Partially Clear", + "gaps": "The headline criterion was unambiguous and directly testable (non-zero exit, no hang, no raw stack trace, actionable remediation string — asserted verbatim in tests/integration/cli-commands/non-interactive-auth.test.ts). Three design decisions were not specified up front and were resolved during implementation: (1) which stream the diagnostic goes to — settled on stderr so piped stdout and --json consumers stay clean; (2) whether the stack is discarded or relocated — settled on synchronous appendFileSync persistence to the log file while the console shows only the actionable line; (3) how far the guard installation should reach — settled on every bin/ entrypoint rather than AgentCLI, so constructing an AgentCLI never mutates global process state." + }, + "technical_risk": { + "score": 5, + "label": "XL", + "risk_factors": "Authentication/authorization failure paths are directly modified (src/utils/auth.ts, src/providers/core/auth-validation.ts, src/utils/sdk-client.ts). No prior precedent in the codebase for process-level error guards — process-guards.ts installs global unhandledRejection/uncaughtException handlers into all 12 entrypoints, mutating global process state and changing exit semantics for the entire CLI surface. Non-obvious async/IO ordering hazard: logger writes through an fs.WriteStream that process.exit() does not drain, so a fatal record is lost on a cold stream — this required a separate synchronous appendFileSync path (persistFatalSync). Subtle control flow in getAuthenticatedClient, where promptReauthentication's generic 'Authentication expired' throw would shadow the upstream message that names the actual remediation. Test-harness hazard: runSilent wraps execSync, which blocks the Vitest worker synchronously so testTimeout cannot interrupt it — a regression to the original hang would wedge CI instead of failing, forcing a new timedOut flag on CommandResult plus an elapsed-time assertion. Realized-risk evidence: two code-review rounds — the first produced 5 blocking findings, and the check round found 2 further defects introduced by the first fix-up, the signature of an under-scoped risk surface.", + "mitigation": "Guard is additive and trivially reversible (no migration, no schema, no persisted state); installProcessGuards() is idempotent via an `installed` flag; persistFatalSync swallows its own failures so a logging error can never mask the original fatal; process.exitCode is set before any I/O so an early exit still reports non-zero. Coverage was deliberately layered against mock blindness: process-guards.test.ts mocks the logger, while a dedicated companion (process-guards.logfile.test.ts) runs the REAL logger and asserts the stack actually reaches the log file — added specifically because that contract shipped undetected in review round 1 (CR-001). The end-to-end criterion is pinned by an integration test running against an isolated CODEMIE_HOME with stdin: 'ignore'." + }, + "file_change_estimate": { + "score": 5, + "label": "XL", + "modified_files": 20, + "modified_file_list": [ + "bin/agent-executor.js", + "bin/codemie-claude-acp.js", + "bin/codemie-claude.js", + "bin/codemie-codex.js", + "bin/codemie-copilot.js", + "bin/codemie-gemini.js", + "bin/codemie-kimi-acp.js", + "bin/codemie-kimi.js", + "bin/codemie-opencode.js", + "bin/codemie-openwiki.js", + "bin/codemie-pi.js", + "bin/codemie.js", + "docs/AUTHENTICATION.md", + "src/cli/commands/sdk/utils/cli-utils.ts", + "src/providers/core/auth-validation.ts", + "src/utils/auth.ts", + "src/utils/sdk-client.ts", + "src/utils/__tests__/auth.test.ts", + "src/utils/__tests__/sdk-client.test.ts", + "tests/helpers/cli-runner.ts" + ], + "new_files": 6, + "new_file_list": [ + "src/utils/process-guards.ts", + "src/utils/__tests__/process-guards.test.ts", + "src/utils/__tests__/process-guards.logfile.test.ts", + "src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts", + "src/providers/core/__tests__/auth-validation.test.ts", + "tests/integration/cli-commands/non-interactive-auth.test.ts" + ], + "affected_dirs": [ + "bin", + "docs", + "src/utils", + "src/providers/core", + "src/cli/commands/sdk/utils", + "tests/helpers", + "tests/integration/cli-commands" + ] + }, + "dependencies": { + "score": 1, + "label": "XS", + "new_packages": [], + "version_changes": [] + }, + "affected_layers": { + "score": 4, + "label": "L", + "layers_changed": [ + "Process/entrypoint bootstrap (bin/)", + "Service (src/utils/auth.ts, src/utils/sdk-client.ts)", + "Provider-core / External auth (src/providers/core/auth-validation.ts)", + "CLI command utils (src/cli/commands/sdk/utils/cli-utils.ts)", + "Test infrastructure (tests/helpers/cli-runner.ts)" + ], + "schema_migration": false, + "cross_system": false + } + }, + "total": 23, + "size": "L", + "band_range": "21-26", + "files_changed": 34, + "routing": "brainstorming", + "key_reasoning": [ + { + "dimension": "component_scope", + "reason": "Five substantive components plus a new cross-cutting module: process-guards.ts (new, global unhandledRejection/uncaughtException handling with synchronous fatal log persistence), the shared auth utility (error-shadowing fix so the upstream remediation message survives), the SDK client bootstrap (ora spinner suppressed when no TTY, since it emits raw cursor-control escapes into captured output), provider-core auth validation (skip promptForReauth entirely when isNonInteractiveEnvironment(), avoiding ERR_USE_AFTER_CLOSE), and the SDK CLI utils where getSdkClient() became the single gate routing auth acquisition through handleSdkError. The guard is then wired into every one of the 12 bin/ entrypoints, so the change reaches the whole binary surface of the project. Red flag applied: touches core shared utilities (src/utils/auth.ts, src/utils/sdk-client.ts, new src/utils/process-guards.ts) — bumped from L (4) to XL (5)." + }, + { + "dimension": "technical_risk", + "reason": "Highest-signal dimension and the main driver of the delta from the M (19/36) initial estimate. Modifies authentication failure paths (explicit red flag), and installs global process-level handlers with no prior precedent in the codebase — this mutates global process state and changes exit semantics for every entrypoint. Two genuinely non-obvious hazards were only discovered during implementation: the logger's fs.WriteStream is not drained by process.exit() (so the fatal record — the one that matters most — was silently lost, requiring a separate appendFileSync path), and execSync inside the test runner blocks the Vitest worker so testTimeout cannot interrupt a hang, meaning a regression would have wedged CI rather than failing the test. Realized evidence confirms the score: review round 1 produced 5 blocking findings, and the check round found 2 further defects introduced by the round-1 fix-up." + }, + { + "dimension": "file_change_estimate", + "reason": "26 non-artifact files changed (20 modified, 6 new) across 7 directories; the diffstat total of 34 includes 8 SDLC planning artifacts under docs/superpowers/tasks/ which are not implementation. Scored XL rather than XXL deliberately: 12 of the 20 modified files are bin/ entrypoints receiving the identical mechanical two-line addition (import + installProcessGuards()), which is one substantive change repeated, not twelve. Discounting those leaves 14 substantive files with 6 new across 6 directories spanning multiple subsystems — squarely the XL band. Test files outnumber production files (6 test/harness files vs 5 production source files), reflecting the layered anti-mock-blindness coverage strategy." + }, + { + "dimension": "affected_layers", + "reason": "Four production layers plus test infrastructure: process/entrypoint bootstrap (bin/), Service (auth + sdk-client utils), Provider-core external auth (SSO validation), and CLI command utils — with two cross-cutting concerns added across them simultaneously (auth failure handling and observability, via fatal log-file persistence). Held at L rather than XL because there is no persistence layer, no schema migration, and no new external integration: the SSO path is only made to fail cleanly, not re-plumbed." + } + ], + "red_flags_applied": [ + "Technical Risk bumped from L (4) to XL (5): affects authentication/authorization — the change modifies the SSO auth failure path in src/utils/auth.ts, src/providers/core/auth-validation.ts and src/utils/sdk-client.ts.", + "Component Scope bumped from L (4) to XL (5): touches core shared utilities — src/utils/auth.ts, src/utils/sdk-client.ts, and the new cross-cutting src/utils/process-guards.ts.", + "Component Scope 'affects multiple workflows or agents' red flag noted but NOT double-counted: the 12 agent entrypoint edits are the same underlying fact as the shared-utility bump already applied, and a second bump would have pushed the dimension to XXL on a mechanical two-line repetition.", + "No dependency, schema-migration, data-migration, real-time/streaming, performance, or new-external-integration red flags apply." + ], + "split_recommendation": null +} diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl index 93f640cf5..c194a4077 100644 --- a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl @@ -2,3 +2,5 @@ {"event":"lifecycle_emission","intent":"record_complexity_score","assessment_mode":"initial","status":"failed"} {"event":"work_item.adapter_receipt","intent":"record_complexity_score","phase":2,"adapter":"codemie-jira-assistant","attempt":2,"status":"failed","reason":"Skill tool returned 'Unknown skill: codemie-jira-assistant'. Root cause: the skill is project-scoped at codemie-code/.claude/skills/codemie-jira-assistant, but the session project directory is the parent codemie-dev, so project skills from the repo are not loaded. Not a Jira connectivity problem - Jira MCP is connected and healthy. Deterministic MCP fallback (jira_add_comment) is available but is an outward-facing write and was not performed without explicit authorization.","external_sync":"pending"} {"event":"lifecycle_emission","intent":"record_complexity_score","assessment_mode":"initial","status":"failed"} +{"event":"work_item.adapter_receipt","intent":"record_complexity_score","phase":8,"adapter":"codemie-jira-assistant","status":"failed","reason":"Same root cause as the initial assessment: adapter skill is project-scoped to codemie-code and unresolvable from the codemie-dev session project directory. Authorized MCP fallback was attempted for the initial score and blocked by the harness auto-mode classifier, so it was not retried here.","data":{"complexity_total":23,"complexity_size":"L","assessment_mode":"actual","delta_from_initial":4},"external_sync":"pending"} +{"event":"lifecycle_emission","intent":"record_complexity_score","assessment_mode":"actual","status":"failed"} From bb306fc031cb772dd22b1d793aafa8246976aa89 Mon Sep 17 00:00:00 2001 From: SleepySML Date: Fri, 4 Sep 2026 11:03:15 +0300 Subject: [PATCH 10/16] docs(cli): re-run QA gates after rebase onto d7097a2a origin/main advanced by one commit while this work was in progress (revert of the LiteLLM/SSO setup enforcement gate). Rebased and re-ran every gate against the rebased tree - all still pass. That commit also removed the test:unit and test:integration package scripts, so the report now cites the current commands. The unit count moves 3969 -> 3939 because the revert removed its own tests, not because anything here regressed. EPMCDME-14148 Co-Authored-By: Claude --- .../qa-report.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md index c647c3c92..1f053a942 100644 --- a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md @@ -1,11 +1,13 @@ # QA Gate Report — EPMCDME-14148 **Branch**: `EPMCDME-14148` -**Merge base**: `origin/main` @ `1d5cc22b` +**Merge base**: `origin/main` @ `d7097a2a` (rebased; originally cut from `1d5cc22b`) **Runner**: npm (guide-first from `.ai-run/guides/quality-gates.md`) **Started**: 2026-09-03T18:05:00Z **Status**: PASSED (with two SKIPPED gates owed to CI — see below) +> **Re-run after rebase.** `origin/main` advanced by one commit (`d7097a2a`, revert of the LiteLLM/SSO setup enforcement gate) while this work was in progress. The branch was rebased onto it and every gate below was re-run against the rebased tree. That commit also **renamed the test scripts** — `test:unit` and `test:integration` no longer exist in `package.json`; the commands below are the current equivalents. The unit count drops from 3969 to 3939 because the revert removed its own tests, not because anything here regressed. + ## Gates | Gate | Source | Status | Duration | Command | Notes | @@ -14,8 +16,8 @@ | lint | guide | PASS | 2s | `npm run lint` | `--max-warnings=0`; `no-useless-catch` did not fire on either new catch | | typecheck | guide | PASS | 2s | `npm run typecheck` | | | build | guide | PASS | 2s | `npm run build` | | -| unit | guide | PASS | 6s | `npm run test:unit` | **3969 passed / 3969**, 269 files, 0 failures | -| integration | guide + ci | **PARTIAL** | — | `npm run test:integration` | See below — pre-existing local hangs, not caused by this branch | +| unit | guide | PASS | 5s | `npx vitest run --project unit` | **3939 passed / 3939**, 267 files, 0 failures (post-rebase) | +| integration | guide + ci | **PARTIAL** | — | `npx vitest run --project cli` | See below — pre-existing local hangs, not caused by this branch | | commitlint | guide + ci | PASS | 0s | `npx commitlint --from origin/main --to HEAD --verbose` | 0 problems, 0 warnings across all 7 commits | | secrets | hook | **SKIPPED** | 0s | `npm run validate:secrets` | Self-skipped — see below | | affected | guide | N/A | — | — | No changed-file-aware command configured in this project | From 22ae3fb58249b34895628c8d73c776683cd5d693 Mon Sep 17 00:00:00 2001 From: SleepySML Date: Fri, 4 Sep 2026 11:43:23 +0300 Subject: [PATCH 11/16] fix(utils): gate the spinner on the output stream, not stdin Code review raised this as a minor finding and it was deferred; this closes it. ora writes to stderr, but spinner suppression was gated on isNonInteractiveEnvironment(), which reads process.stdin.isTTY. Input and output redirect independently, so that was wrong in both directions: `codemie ... > run.log 2>&1` from a terminal kept the spinner and filled the log with cursor-control escapes - the exact symptom the change was meant to remove - while `codemie ... < input.json` needlessly dropped the spinner for a user watching the terminal. Adds isNonInteractiveOutput() (!process.stderr.isTTY) alongside the existing predicate rather than changing it: isNonInteractiveEnvironment() is about whether we can prompt, where stdin is the correct signal, and it still guards the re-auth prompt. Verified both directions empirically: with stderr redirected the run emits zero cursor-control sequences, and under a real pty the spinner still renders. The two unit cases that pin the distinction fail against the previous implementation. EPMCDME-14148 Co-Authored-By: Claude --- docs/AUTHENTICATION.md | 6 ++- src/utils/__tests__/interactive.test.ts | 52 +++++++++++++++++++ src/utils/__tests__/sdk-client.test.ts | 69 ++++++++++++++++++++----- src/utils/interactive.ts | 13 +++++ src/utils/sdk-client.ts | 10 ++-- 5 files changed, 130 insertions(+), 20 deletions(-) diff --git a/docs/AUTHENTICATION.md b/docs/AUTHENTICATION.md index 1e50f946d..2300b2721 100644 --- a/docs/AUTHENTICATION.md +++ b/docs/AUTHENTICATION.md @@ -118,8 +118,10 @@ $ echo $? ``` Diagnostics go to stderr rather than stdout, so piping stdout or consuming `--json` output stays -clean. Progress spinners are suppressed when no TTY is attached, so captured logs do not fill with -cursor-control escape sequences. +clean. Progress spinners are suppressed whenever their output stream is not a terminal — including +`codemie … > run.log 2>&1` from an interactive shell — so captured logs do not fill with +cursor-control escape sequences. Spinner suppression tracks the **output** stream, independently of +whether `stdin` is redirected. There is no separate `--non-interactive` flag to set — detection is automatic, based solely on whether `stdin` is a TTY. diff --git a/src/utils/__tests__/interactive.test.ts b/src/utils/__tests__/interactive.test.ts index 841503ebc..c7b7670ad 100644 --- a/src/utils/__tests__/interactive.test.ts +++ b/src/utils/__tests__/interactive.test.ts @@ -31,3 +31,55 @@ describe('isNonInteractiveEnvironment', () => { expect(isNonInteractiveEnvironment()).toBe(false); }); }); + +describe('isNonInteractiveOutput', () => { + const originalStdin = process.stdin.isTTY; + const originalStderr = process.stderr.isTTY; + + afterEach(() => { + process.stdin.isTTY = originalStdin; + process.stderr.isTTY = originalStderr; + }); + + it('should return true when process.stderr.isTTY is undefined (redirected output)', async () => { + process.stderr.isTTY = undefined as unknown as true; + + const { isNonInteractiveOutput } = await import('../interactive.js'); + + expect(isNonInteractiveOutput()).toBe(true); + }); + + it('should return false when process.stderr.isTTY is true (terminal output)', async () => { + process.stderr.isTTY = true; + + const { isNonInteractiveOutput } = await import('../interactive.js'); + + expect(isNonInteractiveOutput()).toBe(false); + }); + + // The two cases that motivate a separate predicate: input and output are + // redirected independently, so the two must be able to disagree. + it('should track stderr, not stdin, when only stdin is redirected', async () => { + process.stdin.isTTY = false as unknown as true; + process.stderr.isTTY = true; + + const { isNonInteractiveOutput, isNonInteractiveEnvironment } = await import( + '../interactive.js' + ); + + expect(isNonInteractiveEnvironment()).toBe(true); + expect(isNonInteractiveOutput()).toBe(false); + }); + + it('should track stderr, not stdin, when only output is redirected', async () => { + process.stdin.isTTY = true; + process.stderr.isTTY = false as unknown as true; + + const { isNonInteractiveOutput, isNonInteractiveEnvironment } = await import( + '../interactive.js' + ); + + expect(isNonInteractiveEnvironment()).toBe(false); + expect(isNonInteractiveOutput()).toBe(true); + }); +}); diff --git a/src/utils/__tests__/sdk-client.test.ts b/src/utils/__tests__/sdk-client.test.ts index 62bb451a5..1ad9f07bc 100644 --- a/src/utils/__tests__/sdk-client.test.ts +++ b/src/utils/__tests__/sdk-client.test.ts @@ -15,6 +15,7 @@ vi.mock('ora', () => ({ default: oraFactory })); vi.mock('../interactive.js', () => ({ isNonInteractiveEnvironment: vi.fn(), + isNonInteractiveOutput: vi.fn(), })); vi.mock('../config.js', () => ({ @@ -43,37 +44,77 @@ describe('getCodemieClient spinner behaviour', () => { vi.resetModules(); }); - it('does not start a spinner when the environment is non-interactive', async () => { - const { isNonInteractiveEnvironment } = await import('../interactive.js'); + async function arrange(opts: { stdinTty: boolean; stderrTty: boolean }) { + const { isNonInteractiveEnvironment, isNonInteractiveOutput } = await import( + '../interactive.js' + ); const { ConfigLoader } = await import('../config.js'); - const { ConfigurationError } = await import('../errors.js'); - vi.mocked(isNonInteractiveEnvironment).mockReturnValue(true); + vi.mocked(isNonInteractiveEnvironment).mockReturnValue(!opts.stdinTty); + vi.mocked(isNonInteractiveOutput).mockReturnValue(!opts.stderrTty); vi.mocked(ConfigLoader.load).mockResolvedValue({ codeMieUrl: 'https://example.test', } as never); getStoredCredentials.mockResolvedValue(null); - const { getCodemieClient } = await import('../sdk-client.js'); + return import('../sdk-client.js'); + } + + it('does not start a spinner when the output stream is not a TTY', async () => { + const { ConfigurationError } = await import('../errors.js'); + const { getCodemieClient } = await arrange({ + stdinTty: false, + stderrTty: false, + }); await expect(getCodemieClient()).rejects.toThrow(ConfigurationError); expect(oraFactory).not.toHaveBeenCalled(); }); - it('still starts a spinner when interactive and not explicitly quiet', async () => { - const { isNonInteractiveEnvironment } = await import('../interactive.js'); - const { ConfigLoader } = await import('../config.js'); + it('starts a spinner when the output stream is a TTY', async () => { const { ConfigurationError } = await import('../errors.js'); + const { getCodemieClient } = await arrange({ + stdinTty: true, + stderrTty: true, + }); - vi.mocked(isNonInteractiveEnvironment).mockReturnValue(false); - vi.mocked(ConfigLoader.load).mockResolvedValue({ - codeMieUrl: 'https://example.test', - } as never); - getStoredCredentials.mockResolvedValue(null); + await expect(getCodemieClient()).rejects.toThrow(ConfigurationError); + expect(oraFactory).toHaveBeenCalled(); + }); + + // The two cases below are the point of the change: the spinner writes to + // stderr, so it must follow stderr's TTY-ness, not stdin's. - const { getCodemieClient } = await import('../sdk-client.js'); + it('suppresses the spinner when stdout/stderr are redirected but stdin is a TTY', async () => { + const { ConfigurationError } = await import('../errors.js'); + const { getCodemieClient } = await arrange({ + stdinTty: true, + stderrTty: false, + }); + + await expect(getCodemieClient()).rejects.toThrow(ConfigurationError); + expect(oraFactory).not.toHaveBeenCalled(); + }); + + it('keeps the spinner when stdin is redirected but stderr is still a TTY', async () => { + const { ConfigurationError } = await import('../errors.js'); + const { getCodemieClient } = await arrange({ + stdinTty: false, + stderrTty: true, + }); await expect(getCodemieClient()).rejects.toThrow(ConfigurationError); expect(oraFactory).toHaveBeenCalled(); }); + + it('honours an explicit quiet flag even when stderr is a TTY', async () => { + const { ConfigurationError } = await import('../errors.js'); + const { getCodemieClient } = await arrange({ + stdinTty: true, + stderrTty: true, + }); + + await expect(getCodemieClient(true)).rejects.toThrow(ConfigurationError); + expect(oraFactory).not.toHaveBeenCalled(); + }); }); diff --git a/src/utils/interactive.ts b/src/utils/interactive.ts index e37950e80..3cb0e438b 100644 --- a/src/utils/interactive.ts +++ b/src/utils/interactive.ts @@ -14,3 +14,16 @@ export function isNonInteractiveEnvironment(): boolean { return !process.stdin.isTTY; } + +/** + * Returns true when progress output would not be rendered to a terminal. + * + * Deliberately separate from isNonInteractiveEnvironment(): input and output + * can be redirected independently. A spinner is an output concern — ora writes + * to stderr — so gating it on stdin both suppresses it for a terminal user who + * merely redirects stdin, and fails to suppress it for `cmd > log 2>&1`, which + * is the case that fills captured logs with cursor-control escapes. + */ +export function isNonInteractiveOutput(): boolean { + return !process.stderr.isTTY; +} diff --git a/src/utils/sdk-client.ts b/src/utils/sdk-client.ts index 3d678cd96..25ac683e7 100644 --- a/src/utils/sdk-client.ts +++ b/src/utils/sdk-client.ts @@ -11,7 +11,7 @@ import type { CodeMieConfigOptions } from '../env/types.js'; import { CodeMieSSO } from '../providers/plugins/sso/sso.auth.js'; import { ConfigLoader } from './config.js'; import { ConfigurationError } from './errors.js'; -import { isNonInteractiveEnvironment } from './interactive.js'; +import { isNonInteractiveOutput } from './interactive.js'; import { logger } from './logger.js'; /** @@ -22,9 +22,11 @@ import { logger } from './logger.js'; * @throws ConfigurationError if setup is incomplete or credentials are invalid */ export async function getCodemieClient(quiet = false): Promise { - // A spinner with no TTY emits raw cursor-control escapes into captured - // output, so suppress it in non-interactive runs (EPMCDME-14148). - const showProgress = !quiet && !isNonInteractiveEnvironment(); + // ora writes to stderr, so the spinner follows stderr's TTY-ness, not + // stdin's. Gating on stdin would still emit cursor-control escapes into + // `cmd > log 2>&1` while needlessly dropping the spinner for `cmd < input` + // (EPMCDME-14148). + const showProgress = !quiet && !isNonInteractiveOutput(); let spinner; if (showProgress) { From 8b1a9fe75373ad311da239394d270f776f6a6a6c Mon Sep 17 00:00:00 2001 From: SleepySML Date: Fri, 4 Sep 2026 11:58:35 +0300 Subject: [PATCH 12/16] docs(cli): supersede the AC4 reproduction recipe with measured evidence The recipe carried from code review assumed the only thing between a run and the re-auth prompt was the absence of credentials, so planting stale ones would reach it. Tracing shows that assumption is wrong. Measured under a real pty with a copy of the working config: stdin and stderr are both TTYs, ai-run-sso is registered, and validateAuth and promptForReauth are both present - every precondition the recipe assumed - yet promptForReauth is still never entered. It prints a warning banner before opening its readline interface and that banner never appears, so the function was not reached. Execution goes straight to the actionable message. The blocker is therefore not missing credentials, and the prompt may not be reachable from `codemie sdk ...` at all on this configuration - which casts doubt on the ticket's premise, not just on the crash. The recipe is marked superseded rather than deleted, with the measurements that undermine it. Redirects the follow-up ticket to answer "where is the prompt reachable" first, and points at AgentCLI.handleRun, which calls handleAuthValidationFailure directly rather than through getAuthenticatedClient - a branch this investigation never exercised. EPMCDME-14148 Co-Authored-By: Claude --- .../ac4-investigation.md | 40 +++++++++++++++++-- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md index 32c8abd7c..2ed47d0e0 100644 --- a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md @@ -26,7 +26,9 @@ Plus the 2 earlier `script`-based attempts in `reproduction.md`. **10 attempts t Every attempt exited cleanly in well under 1.5 s — before any signal was delivered. The process reaches `No valid SSO credentials found` and terminates with the actionable message. `promptReauthentication`'s interactive branch is never entered in a clean-room home, so "kill *during* the prompt" was never actually exercised. -Reproducing AC4 would need an environment where `promptForReauth` is genuinely reached — most plausibly a home with *stale but present* credentials for a matching URL, rather than absent ones. That is the reporter's environment, which this investigation could not recreate from the ticket text. +The obvious explanation is that a clean-room home has no credentials, so nothing triggers a re-auth offer. **That explanation was tested and does not hold.** Re-running under a real pty with a copy of the working config — provider registered, `validateAuth` and `promptForReauth` both present, `stdin` a TTY — still never enters `promptForReauth`. See "What was actually measured" under Recommendation. + +So the blocker is not the absence of credentials, and the prompt may not be reachable from `codemie sdk …` at all on this configuration. Where it *is* reachable is the open question the follow-up ticket has to answer first. ## Two earlier observations retracted @@ -48,9 +50,41 @@ Under node-pty the same command completes in under 1 s. The non-TTY guard is sti Split AC4 into its own ticket. Do **not** close it against this MR — neither as satisfied nor as "not reproducible", since the criterion was never exercised. -Carry this reproduction recipe over, which the code review identified and this investigation stopped one step short of building: +**The first step of that ticket is not to fix anything — it is to find out where the re-auth prompt is reachable at all.** Evidence below suggests it may not be reachable from `codemie sdk …`, which is the command the ticket names. + +### Superseded recipe — do not start here + +An earlier draft of this document carried the following, proposed during code review: + +> ~~`sso.setup-steps.ts` `validateAuth` returns `{valid: false, error: 'API access test failed: …'}` whenever `fetchCodeMieModels` throws, and that result reaches `promptForReauth`'s `inquirer.prompt`. So: plant a credentials file whose `apiUrl` points at a **closed local port**. `validateAuth` then fails deterministically, the prompt *is* reached, and `tests/helpers/pty-session.ts` can drive a signal into it. The missing precondition is stale-but-present credentials — not absent ones.~~ + +**This is a hypothesis, and later evidence undermines it.** It assumed the only thing standing between the run and the prompt was the absence of credentials. Direct tracing shows otherwise. + +### What was actually measured + +Run: `codemie sdk assistants list` under a real pty (node-pty), with `CODEMIE_HOME` pointing at a throwaway directory holding a **copy of the real config** (provider `ai-run-sso`, `authMethod` `sso`), `CODEMIE_DEBUG=true`. + +| Check | Result | +|---|---| +| `process.stdin.isTTY` under node-pty | **true** — the PR #471 guard does not fire | +| `process.stderr.isTTY` under node-pty | **true** | +| `ProviderRegistry.hasProvider('ai-run-sso')` after the sdk import chain | **true** | +| `getSetupSteps('ai-run-sso').validateAuth` | **function** | +| `getSetupSteps('ai-run-sso').promptForReauth` | **function** | +| `⚠️ Authentication required` in output (printed by `promptForReauth` **before** its `inquirer.prompt`) | **absent** | +| Observed outcome | straight to `❌ SSO authentication required…`, exit 1 | + +Every precondition the recipe assumed is satisfied — TTY attached, provider registered, both setup-step methods present — and the prompt is **still** not reached. `promptForReauth` prints its warning banner before opening the readline interface, so the absence of that line is direct evidence the function was never entered. + +Something between `promptReauthentication` (`utils/auth.ts:71`) and `promptForReauth` (`sso.setup-steps.ts:248`) short-circuits, and this investigation did not isolate what. Candidates not yet eliminated: the `ProviderProfile` reaching `getSetupSteps(config.provider || '')` carrying a different `provider` value than the config file suggests after profile resolution and migration `007-decouple-provider-workspace-config`; or `validateAuth` throwing rather than returning, which the new `try`/`catch` in `getAuthenticatedClient` converts into a rethrow of the original error — producing exactly the observed output. + +### Consequence for the ticket + +The ticket's Actual Result — *"CLI hangs on a re-authentication prompt"* — could not be reproduced from `codemie sdk …` on this configuration, with or without credentials. That casts doubt on the **premise**, not just the crash. + +The follow-up ticket should therefore begin by answering: **from which entry point is the re-auth prompt actually reachable?** Note that the agent binaries (`codemie-claude`, `agent-executor`, …) reach auth through `AgentCLI.handleRun`, which calls `handleAuthValidationFailure` **directly** (`AgentCLI.ts:293-294`, `:323-324`) rather than through `getAuthenticatedClient` — a different branch that this investigation never exercised. That is the more promising place to look, and it is also worth asking the reporter which command produced the crash. -> `sso.setup-steps.ts` `validateAuth` returns `{valid: false, error: 'API access test failed: …'}` whenever `fetchCodeMieModels` throws, and that result reaches `promptForReauth`'s `inquirer.prompt`. So: plant a credentials file whose `apiUrl` points at a **closed local port**. `validateAuth` then fails deterministically with no network dependency, the prompt *is* reached, and `tests/helpers/pty-session.ts` can drive a signal into it. The missing precondition is stale-but-present credentials — not absent ones, which is all this investigation ever tested. +Only once the prompt is demonstrably reachable does the stale-credentials trick become worth trying as a way to make it deterministic. **Retracted claim.** An earlier draft of this document argued that "the spinner suppression in this MR removes one plausible contributor (a spinner writing to a torn-down TTY)". **That is false and has been struck.** The suppression added in `sdk-client.ts` is gated on `isNonInteractiveEnvironment()`, so it fires only when **no** TTY is attached — while AC4's scenario requires a TTY by definition. It can never fire there. The spinner implicated in the original escape-sequence observation is the one inside `promptForReauth` (`sso.setup-steps.ts:294`), which this MR does not touch. That sentence was the sole justification offered for shipping without AC4, and it did not survive inspection; nothing in this MR mitigates AC4, partially or otherwise. From 7a892dfc3a47b82431b320f8eb57adc84e3735d3 Mon Sep 17 00:00:00 2001 From: SleepySML Date: Fri, 4 Sep 2026 12:07:07 +0300 Subject: [PATCH 13/16] docs(cli): verify AC4 - prompt is reachable, no readline crash AC4 is now actually exercised rather than deferred. 12 of 12 runs reached the interactive re-auth prompt under a real pty and were then interrupted (Ctrl-C, SIGINT, SIGTERM, SIGHUP x three delays); none produced ERR_USE_AFTER_CLOSE or any readline error. Root cause of every earlier failure to reach the prompt was the test environment, not the product. These probes ran from a shell CodeMie had launched, which exports CODEMIE_PROVIDER=anthropic-subscription and CODEMIE_PROFILE_CONFIG. ConfigLoader gives process.env precedence over both config files, so every run resolved to anthropic-subscription - a provider with no validateAuth and no promptForReauth - and terminated before any prompt could appear. CODEMIE_HOME isolates config files but not the env vars that outrank them. Stripping CODEMIE_* from the child environment fixes it, and the prompt is then reached reliably. This also retires the superseded reproduction recipe: planting stale credentials was never necessary, because absent credentials already return {valid:false} through the 'No SSO credentials found' branch, which is enough to reach the prompt. Four claims are now on record as retracted, all environment or harness artifacts misread as product behaviour, including the previous "prompt unreachable from codemie sdk" - which questioned the ticket's premise on the strength of a polluted environment. Records one incidental finding for its own ticket: Ctrl-C at the prompt exits with code 0 rather than the conventional non-zero. EPMCDME-14148 Co-Authored-By: Claude --- .../ac4-investigation.md | 109 ++++++++---------- .../reproduction.md | 4 +- 2 files changed, 49 insertions(+), 64 deletions(-) diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md index 2ed47d0e0..c38baed5b 100644 --- a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md @@ -2,90 +2,75 @@ **Acceptance criterion:** "Killing during prompt does not produce readline lifecycle crash." -**Verdict: precondition not constructed — the criterion was never exercised.** +**Verdict: exercised and passing.** 12 of 12 runs reached the interactive re-auth prompt and were then interrupted; none produced `ERR_USE_AFTER_CLOSE` or any readline lifecycle error. -This is deliberately *not* "cannot reproduce". Every attempt exited before the interactive prompt was reached, so the scenario the criterion describes — killing the process *while it sits on the prompt* — was never actually set up. Ten attempts at failing to reach the prompt is zero attempts at the criterion. "Cannot reproduce the crash" and "cannot construct the precondition" are different claims, and only the second is supported by what follows. +This supersedes two earlier verdicts in this document ("cannot reproduce", then "precondition not constructible"). Both were wrong, and both were wrong for the same reason: the test environment, not the product. See *Why the earlier attempts failed*. -## What was run +## Result -A node-pty probe (`ac4-probe.mjs`, throwaway) driving a **real** pty — not `script`, whose artifacts are discussed below. Isolated `CODEMIE_HOME` per attempt, with the real config copied in (never credentials) so the `sso` provider resolves. +Harness: `node-pty` (a real pty, so `stdin.isTTY === true`), isolated `CODEMIE_HOME` per run holding only a copy of the config — **no credentials**, which is what makes `validateAuth` fail and the prompt appear. Interrupt delivered once `Re-authenticate now?` is actually on screen. -8 attempts: 4 interrupt modes (`Ctrl-C` as raw `\x03`, `SIGINT`, `SIGTERM`, `SIGHUP`) × 2 delays (1.5 s, 4 s). +| Mode | delay after prompt | `ERR_USE_AFTER_CLOSE` | exit | +|---|---|---|---| +| Ctrl-C (`\x03`) | 0 / 300 / 1500 ms | none | code 0, no signal | +| `SIGINT` | 0 / 300 / 1500 ms | none | code 0, signal 2 | +| `SIGTERM` | 0 / 300 / 1500 ms | none | code 0, signal 15 | +| `SIGHUP` | 0 / 300 / 1500 ms | none | code 0, signal 1 | -| Result | Across all 8 | -|---|---| -| `ERR_USE_AFTER_CLOSE` occurrences | **0** | -| Any `readline` mention | **0** | -| Reached the re-auth prompt | **0** | -| Exit | `code=1, signal=0` every time | -| Max output | 1 470 bytes | +**Prompt reached: 12/12. Readline crashes: 0.** -Plus the 2 earlier `script`-based attempts in `reproduction.md`. **10 attempts total, zero hits.** +The ticket hedges with "*can* crash", so this is evidence of non-reproduction on this build and platform (macOS, Node v24.19.0) — not proof the failure mode is impossible everywhere. But it is now a real negative result, obtained with the criterion's precondition genuinely satisfied, rather than an absence of testing. -## Why the prompt was never reached +### Incidental finding, not AC4 -Every attempt exited cleanly in well under 1.5 s — before any signal was delivered. The process reaches `No valid SSO credentials found` and terminates with the actionable message. `promptReauthentication`'s interactive branch is never entered in a clean-room home, so "kill *during* the prompt" was never actually exercised. +**Ctrl-C at the prompt exits with code 0.** Conventionally an interrupt at a prompt should exit non-zero (130 by convention). A script that runs `codemie sdk …`, has the user hit Ctrl-C at the re-auth prompt, and checks `$?` would conclude the command succeeded. Out of scope here; worth its own ticket. -The obvious explanation is that a clean-room home has no credentials, so nothing triggers a re-auth offer. **That explanation was tested and does not hold.** Re-running under a real pty with a copy of the working config — provider registered, `validateAuth` and `promptForReauth` both present, `stdin` a TTY — still never enters `promptForReauth`. See "What was actually measured" under Recommendation. +## Why the earlier attempts failed -So the blocker is not the absence of credentials, and the prompt may not be reachable from `codemie sdk …` at all on this configuration. Where it *is* reachable is the open question the follow-up ticket has to answer first. +Every earlier run in this investigation was executed from a shell that **CodeMie itself had launched** (`CODEMIE_AGENT=claude`, `CODEMIE_CLIENT_TYPE=codemie-claude`). That parent session exports provider settings into the environment: -## Two earlier observations retracted +``` +CODEMIE_PROVIDER=anthropic-subscription +CODEMIE_AUTH_METHOD=manual +CODEMIE_PROFILE_CONFIG={"name":"default","provider":"anthropic-subscription",…} +``` -Both were `script`-harness artifacts, not product defects. Recording them so nobody re-derives them: +`ConfigLoader` gives `process.env` precedence over both the global and the project-local config file (`utils/config.ts`), so **every probe ran as `anthropic-subscription`, never as `ai-run-sso`** — regardless of which config file was planted in the isolated `CODEMIE_HOME`. -**1. The "73 MB ora escape-sequence flood" is not real.** Under a real pty the same command produces **1 470 bytes**. The flood only appears under `script`, consistent with the original guess that `script` yields a pty with no usable `stdout.columns`, breaking ora's line-clearing arithmetic. +The `anthropic-subscription` provider has no `validateAuth` and no `promptForReauth`. So `promptReauthentication` hit its final `throw` immediately, and the run terminated long before any prompt could appear. Traced directly: -**2. The "Case C TTY hang" is not the re-auth prompt.** Originally read as proof that attaching a TTY parks execution on the prompt. Three facts refute it: +| | polluted env | env cleaned | +|---|---|---| +| `config.provider` | `anthropic-subscription` | `ai-run-sso` | +| `setupSteps.validateAuth` | `undefined` | `function` | +| `setupSteps.promptForReauth` | `undefined` | `function` | +| `validateAuth(config)` | not called | `{valid: false, error: 'No SSO credentials found for …'}` | +| `isNonInteractiveEnvironment()` | — | `false` | +| Reaches `promptForReauth`? | **no** | **yes** | -- The identical `script` invocation against the **fixed** build still hangs 25 s. -- `script` wrapping an immediately-exiting child returns in 0 s, so `script` does not hang unconditionally. -- The `script` capture stalls at `⠋ Loading configuration...`, *before* credentials are read, and contains a stray `^D` — the signature of an immediately-EOF stdin being forwarded into the pty. +Clearing every `CODEMIE_*` variable except `CODEMIE_HOME` before spawning is what fixed it. -Under node-pty the same command completes in under 1 s. The non-TTY guard is still confirmed working — by `auth-validation.test.ts` and by Cases A/B exiting in 0–1 s without prompting — but **not** by Case C. +**Consequence for the superseded recipe.** A previous draft proposed planting credentials whose `apiUrl` points at a closed port so `validateAuth` would fail deterministically. That was never needed: **absent** credentials already produce `{valid: false}` via the `No SSO credentials found` branch, which is enough to reach the prompt. The recipe was solving a problem that did not exist, because the real blocker was environmental. -**Methodological note:** `script(1)` is unsuitable for CLI behaviour testing here. It injects its own stdin and terminal-geometry behaviour. Use `node-pty` (already a dependency, wrapped by `tests/helpers/pty-session.ts`) for anything TTY-dependent. +## Retracted claims -## Recommendation - -Split AC4 into its own ticket. Do **not** close it against this MR — neither as satisfied nor as "not reproducible", since the criterion was never exercised. - -**The first step of that ticket is not to fix anything — it is to find out where the re-auth prompt is reachable at all.** Evidence below suggests it may not be reachable from `codemie sdk …`, which is the command the ticket names. - -### Superseded recipe — do not start here - -An earlier draft of this document carried the following, proposed during code review: - -> ~~`sso.setup-steps.ts` `validateAuth` returns `{valid: false, error: 'API access test failed: …'}` whenever `fetchCodeMieModels` throws, and that result reaches `promptForReauth`'s `inquirer.prompt`. So: plant a credentials file whose `apiUrl` points at a **closed local port**. `validateAuth` then fails deterministically, the prompt *is* reached, and `tests/helpers/pty-session.ts` can drive a signal into it. The missing precondition is stale-but-present credentials — not absent ones.~~ +Four, kept on record so nobody re-derives them. All four were **environment or harness artifacts misread as product behaviour** — the recurring failure mode of this investigation. -**This is a hypothesis, and later evidence undermines it.** It assumed the only thing standing between the run and the prompt was the absence of credentials. Direct tracing shows otherwise. +1. **"73 MB ora escape-sequence flood."** Not real. 1 470 bytes under a real pty. Only appears under `script(1)`, which yields a pty with no usable `stdout.columns`, breaking ora's line-clearing arithmetic. +2. **"Case C proves a TTY hang on the prompt."** Not the prompt. The same `script` invocation still hangs 25 s against the *fixed* build, while `script` around an immediately-exiting child returns in 0 s. The capture stalls at `⠋ Loading configuration...`, before credentials are read, with a stray `^D` — an immediately-EOF stdin forwarded into the pty. +3. **"The spinner suppression in this MR mitigates AC4."** False. Suppression is gated on the environment being non-interactive, so it cannot fire in a scenario that requires a TTY. This was the sole justification offered for shipping without AC4 and did not survive inspection. +4. **"The re-auth prompt is unreachable from `codemie sdk …`."** False, and the most misleading of the four, because it questioned the ticket's premise on the strength of a polluted environment. The prompt is reachable, reliably — 12/12. -### What was actually measured +**Methodological notes for the next person:** -Run: `codemie sdk assistants list` under a real pty (node-pty), with `CODEMIE_HOME` pointing at a throwaway directory holding a **copy of the real config** (provider `ai-run-sso`, `authMethod` `sso`), `CODEMIE_DEBUG=true`. +- Use `node-pty` (already a dependency, wrapped by `tests/helpers/pty-session.ts`) for anything TTY-dependent. `script(1)` injects its own stdin and terminal geometry and produced two of the four artifacts above. +- When testing CLI behaviour from inside a CodeMie-launched agent session, **strip `CODEMIE_*` from the child environment**. `CODEMIE_HOME` isolates configuration files but not the environment variables that outrank them. -| Check | Result | -|---|---| -| `process.stdin.isTTY` under node-pty | **true** — the PR #471 guard does not fire | -| `process.stderr.isTTY` under node-pty | **true** | -| `ProviderRegistry.hasProvider('ai-run-sso')` after the sdk import chain | **true** | -| `getSetupSteps('ai-run-sso').validateAuth` | **function** | -| `getSetupSteps('ai-run-sso').promptForReauth` | **function** | -| `⚠️ Authentication required` in output (printed by `promptForReauth` **before** its `inquirer.prompt`) | **absent** | -| Observed outcome | straight to `❌ SSO authentication required…`, exit 1 | - -Every precondition the recipe assumed is satisfied — TTY attached, provider registered, both setup-step methods present — and the prompt is **still** not reached. `promptForReauth` prints its warning banner before opening the readline interface, so the absence of that line is direct evidence the function was never entered. - -Something between `promptReauthentication` (`utils/auth.ts:71`) and `promptForReauth` (`sso.setup-steps.ts:248`) short-circuits, and this investigation did not isolate what. Candidates not yet eliminated: the `ProviderProfile` reaching `getSetupSteps(config.provider || '')` carrying a different `provider` value than the config file suggests after profile resolution and migration `007-decouple-provider-workspace-config`; or `validateAuth` throwing rather than returning, which the new `try`/`catch` in `getAuthenticatedClient` converts into a rethrow of the original error — producing exactly the observed output. - -### Consequence for the ticket - -The ticket's Actual Result — *"CLI hangs on a re-authentication prompt"* — could not be reproduced from `codemie sdk …` on this configuration, with or without credentials. That casts doubt on the **premise**, not just the crash. - -The follow-up ticket should therefore begin by answering: **from which entry point is the re-auth prompt actually reachable?** Note that the agent binaries (`codemie-claude`, `agent-executor`, …) reach auth through `AgentCLI.handleRun`, which calls `handleAuthValidationFailure` **directly** (`AgentCLI.ts:293-294`, `:323-324`) rather than through `getAuthenticatedClient` — a different branch that this investigation never exercised. That is the more promising place to look, and it is also worth asking the reporter which command produced the crash. +## Recommendation -Only once the prompt is demonstrably reachable does the stale-credentials trick become worth trying as a way to make it deterministic. +AC4 can be marked **verified** on this build, citing the table above, rather than deferred. -**Retracted claim.** An earlier draft of this document argued that "the spinner suppression in this MR removes one plausible contributor (a spinner writing to a torn-down TTY)". **That is false and has been struck.** The suppression added in `sdk-client.ts` is gated on `isNonInteractiveEnvironment()`, so it fires only when **no** TTY is attached — while AC4's scenario requires a TTY by definition. It can never fire there. The spinner implicated in the original escape-sequence observation is the one inside `promptForReauth` (`sso.setup-steps.ts:294`), which this MR does not touch. That sentence was the sole justification offered for shipping without AC4, and it did not survive inspection; nothing in this MR mitigates AC4, partially or otherwise. +Two follow-ups, neither blocking this MR: -The one part of the original conclusion that stands: **do not ship a speculative fix** for a failure mode with no reproduction. +1. **Ctrl-C at a prompt exits 0** (see above) — its own ticket. +2. If the reporter can still reproduce `ERR_USE_AFTER_CLOSE`, the missing variable is platform or Node version, not the scenario — this investigation now covers the scenario. Ask for OS, Node version, and the exact command. diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md index cf0baf904..6336bcfca 100644 --- a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md @@ -11,7 +11,7 @@ The ticket describes two symptoms. Only one is still live. |---|---| | "CLI hangs on a re-authentication prompt" (non-TTY) | **Already fixed** by `5b2de4b7` (PR #471) | | "exits non-zero with an actionable message" | **Still broken** — exits 1 via an *unhandled exception + raw stack trace*, and the message drops the actionable remediation | -| `ERR_USE_AFTER_CLOSE` on kill | **Not reproduced** (see Case D) | +| `ERR_USE_AFTER_CLOSE` on kill | **Not reproduced** — 12/12 interrupts at a genuinely-reached prompt, zero crashes (`ac4-investigation.md`). Case D below was inconclusive and is superseded | | `--non-interactive` / `--ci` flag | **Does not exist** as a flag, but its absence *is* documented (`docs/AUTHENTICATION.md:113`, added by PR #471) — corrected; an earlier draft of this table wrongly called it undocumented | The residual defect is not a hang. It is that the non-interactive failure path terminates by letting a `ConfigurationError` escape to Node's default handler. @@ -112,4 +112,4 @@ Note that `handleSdkError`'s `else` branch **already** renders `ConfigurationErr | Non-TTY stdin skips interactive prompt | **Met already** | Cases A/B exit in 0–1 s without prompting, plus the `auth-validation.test.ts` guard tests. **Not** Case C, which this document retracts as a `script(1)` artifact | | CLI exits non-zero with clear remediation | **Not met** | Case A/B: exit 1 but raw stack trace; message lacks `codemie setup` | | `--non-interactive` / `--ci` supported or documented | **Partially met** | No such flag in `src/`, but the absence is documented at `docs/AUTHENTICATION.md:113`. The AC reads "supported **or** documented". Not a clean pass: the documented mechanism (`stdin` TTY only) has a blind spot for pty-allocating CI — now stated explicitly in that doc | -| Kill during prompt produces no readline crash | **Not exercised** | The prompt was never reached in any attempt, so the criterion was never tested — see `ac4-investigation.md` | +| Kill during prompt produces no readline crash | **Met** | Later verified properly: 12/12 runs reached the prompt under a real pty and none crashed. The earlier "never reached" reading was an artifact of `CODEMIE_*` env vars injected by the parent CodeMie session — see `ac4-investigation.md` | From 85fd48ae9470416e9736d84401757603552e7ebc Mon Sep 17 00:00:00 2001 From: SleepySML Date: Fri, 4 Sep 2026 12:17:10 +0300 Subject: [PATCH 14/16] docs(cli): add final acceptance verification - all four criteria pass Re-verifies every AC end-to-end through bin/codemie.js under the ticket's stated preconditions, with the environment actually clean. The earlier AC1/AC2 checks ran from a CodeMie-launched shell that exports CODEMIE_PROVIDER=anthropic-subscription, which outranks both config files, so they were verified under the wrong provider. They pass under the correct one too - the message originates in sdk-client.ts, which is provider-agnostic - but a criterion verified under the wrong provider is not verified. Two harness bugs in the verification itself were fixed first: zsh does not word-split an unquoted $VAR, so `env $FLAGS` passed 38 -u flags as one argument and stripped nothing (cleaning now happens inside Node); and the prompt matcher /authentication required/i also matched the legitimate error text 'SSO authentication required', reporting a prompt that never appeared. It now matches the exact inquirer question. AC1 now has a real control: with a TTY the prompt appears at 604ms, without one it does not. Without that control AC1 passes trivially whenever anything terminates the run early - which is how the earlier investigation fooled itself. EPMCDME-14148 Co-Authored-By: Claude --- .../acceptance-verification.md | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/acceptance-verification.md diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/acceptance-verification.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/acceptance-verification.md new file mode 100644 index 000000000..7e26c65fa --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/acceptance-verification.md @@ -0,0 +1,74 @@ +# EPMCDME-14148 — final acceptance verification + +Full re-verification of all four acceptance criteria against the branch head, run end-to-end through `bin/codemie.js` exactly as the ticket's *Steps to Reproduce* describe. + +**Result: 4 / 4 pass.** + +## Why this re-run exists + +The first pass at AC1 and AC2 was executed from a shell that CodeMie itself had launched, which exports `CODEMIE_PROVIDER=anthropic-subscription` and `CODEMIE_PROFILE_CONFIG`. `ConfigLoader` gives `process.env` precedence over both config files, so those runs resolved to the wrong provider — the same pollution that produced the four retracted claims in `ac4-investigation.md`. They happened to pass anyway (the message originates in `sdk-client.ts`, which is provider-agnostic), but a criterion verified under the wrong provider is not verified. + +A second, subtler harness bug had to be fixed first: the shell here is **zsh**, which does not word-split an unquoted `$VAR`, so `env $FLAGS …` passed all 38 `-u` flags as a single argument and stripped nothing. Environment cleaning is therefore done inside Node, not in the shell. + +## Conditions + +Matching the ticket's preconditions: + +- **No valid SSO session** — a throwaway `CODEMIE_HOME` per run, holding a copy of the config and **no credentials**. The real `~/.codemie` is never touched. +- **Non-interactive stdin** — `stdio: ['ignore', …]` for the AC1/AC2 runs. +- **Clean environment** — every `CODEMIE_*` except `CODEMIE_HOME` deleted from the child env, so provider resolution comes from config (`ai-run-sso`), not from the parent agent session. +- Interactive cases use `node-pty` (a real pty), never `script(1)` — see the methodological note in `ac4-investigation.md`. + +## AC1 — "Non-TTY stdin skips interactive prompt" + +| Check | Result | +|---|---| +| `Re-authenticate now?` absent from output | ✅ | +| Completes without hanging | ✅ 964 ms | +| **Control:** same command *with* a TTY does prompt | ✅ prompt at 604 ms | + +The control is the part that matters. Without it, AC1 passes trivially whenever anything else terminates the run early — which is precisely how the earlier investigation fooled itself. With a TTY the prompt appears; without one it does not; the only difference is the TTY, so the `isNonInteractiveEnvironment()` guard is demonstrably what skips it. + +Detection matcher is the exact inquirer question `Re-authenticate now?`. An earlier draft matched `/authentication required/i`, which also matches the legitimate error text `SSO authentication required` and produced a false "prompt appeared". + +## AC2 — "CLI exits non-zero with clear remediation" + +| Check | Result | +|---|---| +| Exit code non-zero | ✅ `1` | +| Message names `codemie setup` | ✅ | +| No stack trace (`^\s+at\s`) | ✅ | +| No leaked `ConfigurationError:` prefix | ✅ | +| No `Node.js v…` crash banner | ✅ | +| stdout free of the diagnostic | ✅ | + +Emitted on stderr: + +``` +❌ SSO authentication required. Please run "codemie setup" with SSO provider first. +``` + +## AC3 — "Optional `--non-interactive` or `--ci` behavior is supported **or** documented" + +| Check | Result | +|---|---| +| No flag registered in `src/cli/index.ts` | ✅ deliberate — EPMCDME-13953 recorded it out of scope | +| Absence documented in `AUTHENTICATION.md` | ✅ | +| Boundary documented (pty-allocating runners) | ✅ added by this MR | + +Satisfied through the "or documented" limb. This MR strengthens it: the page previously claimed stdin-TTY detection was sufficient for CI, which overstated it — a `docker run -t` runner still reaches the prompt. That limitation is now stated explicitly, with JWT auth given as the unattended path. + +## AC4 — "Killing during prompt does not produce readline lifecycle crash" + +| Check | Result | +|---|---| +| Prompt genuinely reached before interrupting | ✅ 3/3 | +| `ERR_USE_AFTER_CLOSE` | ✅ none | + +A broader sweep — 4 signal modes × 3 delays — is recorded in `ac4-investigation.md`: **12/12 reached the prompt, 0 crashes.** + +The ticket hedges with "*can* crash", so this is a negative result on this build and platform (macOS, Node v24.19.0), not proof the failure is impossible everywhere. It is now a real negative result, with the precondition genuinely satisfied. + +## Out of scope, worth its own ticket + +**Ctrl-C at the prompt exits with code 0.** Conventionally an interrupt should exit non-zero (130). A script checking `$?` after the user interrupts would conclude the command succeeded. Unrelated to the four criteria; not fixed here. From 95aeb878a3b621bb53d1a01ef6dd3ee855e8d881 Mon Sep 17 00:00:00 2001 From: SleepySML Date: Fri, 4 Sep 2026 12:24:40 +0300 Subject: [PATCH 15/16] refactor(cli): cut comment volume in the EPMCDME-14148 diff 120 comment lines across ~300 added lines of code, much of it restating what the code already says. The worst was bin/: the same three-line block duplicated across all 12 entrypoints, 35 lines total, explaining a decision that belongs in one place. The rationale for installing per entrypoint rather than in the AgentCLI constructor now lives once, on installProcessGuards itself; the call sites are self-describing. Elsewhere: trimmed narrative from process-guards, interactive, sdk-client, auth, auth-validation, cli-utils, cli-runner and the test files, keeping the why and the ticket reference and dropping restatement. Down to 50 lines. No behaviour change - build, typecheck, lint, 3946 unit, 4 integration all pass, and all four acceptance criteria re-verified end to end. EPMCDME-14148 Co-Authored-By: Claude --- bin/agent-executor.js | 4 --- bin/codemie-claude-acp.js | 4 --- bin/codemie-claude.js | 4 --- bin/codemie-codex.js | 4 --- bin/codemie-copilot.js | 4 --- bin/codemie-gemini.js | 4 --- bin/codemie-kimi-acp.js | 4 --- bin/codemie-kimi.js | 4 --- bin/codemie-opencode.js | 4 --- bin/codemie-openwiki.js | 4 --- bin/codemie-pi.js | 4 --- bin/codemie.js | 2 -- src/cli/commands/sdk/utils/cli-utils.ts | 6 ++-- src/providers/core/auth-validation.ts | 4 +-- src/utils/__tests__/interactive.test.ts | 3 +- .../__tests__/process-guards.logfile.test.ts | 14 +++----- src/utils/__tests__/process-guards.test.ts | 5 ++- src/utils/__tests__/sdk-client.test.ts | 3 -- src/utils/auth.ts | 5 ++- src/utils/interactive.ts | 9 ++---- src/utils/process-guards.ts | 31 +++++------------- src/utils/sdk-client.ts | 5 +-- tests/helpers/cli-runner.ts | 10 ++---- .../cli-commands/non-interactive-auth.test.ts | 32 +++++-------------- 24 files changed, 35 insertions(+), 138 deletions(-) diff --git a/bin/agent-executor.js b/bin/agent-executor.js index df08667fa..ea69abd4d 100755 --- a/bin/agent-executor.js +++ b/bin/agent-executor.js @@ -12,12 +12,8 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - // Load built-in agent (codemie-code) const agent = AgentRegistry.getAgent('codemie-code'); diff --git a/bin/codemie-claude-acp.js b/bin/codemie-claude-acp.js index e31795a02..2c05fe969 100755 --- a/bin/codemie-claude-acp.js +++ b/bin/codemie-claude-acp.js @@ -12,12 +12,8 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - const agent = AgentRegistry.getAgent('claude-acp'); if (!agent) { console.error('✗ Claude ACP agent not found in registry'); diff --git a/bin/codemie-claude.js b/bin/codemie-claude.js index 6b2a3c38b..7d65a8e7f 100755 --- a/bin/codemie-claude.js +++ b/bin/codemie-claude.js @@ -9,12 +9,8 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - const agent = AgentRegistry.getAgent('claude'); if (!agent) { console.error('✗ Claude agent not found in registry'); diff --git a/bin/codemie-codex.js b/bin/codemie-codex.js index 42a564ab6..bc8ab455d 100755 --- a/bin/codemie-codex.js +++ b/bin/codemie-codex.js @@ -9,12 +9,8 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - const agent = AgentRegistry.getAgent('codex'); if (!agent) { console.error('✗ Codex agent not found in registry'); diff --git a/bin/codemie-copilot.js b/bin/codemie-copilot.js index 75ebc9cc4..95749a808 100755 --- a/bin/codemie-copilot.js +++ b/bin/codemie-copilot.js @@ -16,12 +16,8 @@ import { ConfigLoader } from '../dist/utils/config.js'; import { getCodemiePath } from '../dist/utils/paths.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - const SAVED_MODEL_PATH = getCodemiePath('agents', 'copilot-cli', 'model.json'); const OPTION_ALIASES = new Map([ diff --git a/bin/codemie-gemini.js b/bin/codemie-gemini.js index c534480eb..f15e70476 100755 --- a/bin/codemie-gemini.js +++ b/bin/codemie-gemini.js @@ -9,12 +9,8 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - const agent = AgentRegistry.getAgent('gemini'); if (!agent) { console.error('✗ Gemini agent not found in registry'); diff --git a/bin/codemie-kimi-acp.js b/bin/codemie-kimi-acp.js index fa84062d7..deafb5e65 100755 --- a/bin/codemie-kimi-acp.js +++ b/bin/codemie-kimi-acp.js @@ -9,12 +9,8 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - const agent = AgentRegistry.getAgent('kimi-acp'); if (!agent) { console.error('✗ Kimi ACP agent not found in registry'); diff --git a/bin/codemie-kimi.js b/bin/codemie-kimi.js index d6d32babf..706c4700b 100755 --- a/bin/codemie-kimi.js +++ b/bin/codemie-kimi.js @@ -9,12 +9,8 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - const agent = AgentRegistry.getAgent('kimi'); if (!agent) { console.error('✗ Kimi agent not found in registry'); diff --git a/bin/codemie-opencode.js b/bin/codemie-opencode.js index a7860a4b8..7ff1f6c50 100755 --- a/bin/codemie-opencode.js +++ b/bin/codemie-opencode.js @@ -9,12 +9,8 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - const agent = AgentRegistry.getAgent('opencode'); if (!agent) { console.error('✗ OpenCode agent not found in registry'); diff --git a/bin/codemie-openwiki.js b/bin/codemie-openwiki.js index e0fea88fb..648c3b712 100755 --- a/bin/codemie-openwiki.js +++ b/bin/codemie-openwiki.js @@ -9,12 +9,8 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - const agent = AgentRegistry.getAgent('openwiki'); if (!agent) { console.error('✗ OpenWiki agent not found in registry'); diff --git a/bin/codemie-pi.js b/bin/codemie-pi.js index c98b38e17..2c3341546 100755 --- a/bin/codemie-pi.js +++ b/bin/codemie-pi.js @@ -9,12 +9,8 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// Installed per entrypoint rather than in the AgentCLI constructor, so merely -// constructing an AgentCLI (as unit tests do) never mutates global process state. installProcessGuards(); - const agent = AgentRegistry.getAgent('pi'); if (!agent) { console.error('✗ Pi agent not found in registry'); diff --git a/bin/codemie.js b/bin/codemie.js index 7336f2988..918389010 100755 --- a/bin/codemie.js +++ b/bin/codemie.js @@ -9,8 +9,6 @@ import { MigrationRunner } from '../dist/migrations/index.js'; import { checkAndPromptForUpdate } from '../dist/utils/cli-updater.js'; import { installProcessGuards } from '../dist/utils/process-guards.js'; -// Last-line-of-defence net for async rejections that escape a command action. -// program.parse() is sync, so those never reach the import().catch() below. installProcessGuards(); // Auto-run pending migrations (happens at startup) diff --git a/src/cli/commands/sdk/utils/cli-utils.ts b/src/cli/commands/sdk/utils/cli-utils.ts index 6b3273d8e..c2e93f6cd 100644 --- a/src/cli/commands/sdk/utils/cli-utils.ts +++ b/src/cli/commands/sdk/utils/cli-utils.ts @@ -12,10 +12,8 @@ import z, { ZodError } from "zod"; /** * Get an authenticated CodeMie SDK client * - * Auth acquisition is routed through handleSdkError so a failure exits with a - * formatted message rather than an uncaught throw. Every sdk action calls - * getSdkClient() outside its own try/catch, so this is the single gate that - * keeps a missing session from printing a raw stack trace (EPMCDME-14148). + * Every sdk action calls this outside its own try/catch, so this is the single + * gate keeping a missing session from printing a raw stack (EPMCDME-14148). */ export async function getSdkClient(): Promise { try { diff --git a/src/providers/core/auth-validation.ts b/src/providers/core/auth-validation.ts index 79c15def8..edab067da 100644 --- a/src/providers/core/auth-validation.ts +++ b/src/providers/core/auth-validation.ts @@ -35,9 +35,7 @@ export async function handleAuthValidationFailure( return await setupSteps.promptForReauth(config); } - // No re-auth available (or no TTY to prompt on), show full error with - // instructions. Diagnostics go to stderr so piped stdout and --json - // consumers stay clean (EPMCDME-14148). + // stderr, so piped stdout and --json consumers stay clean (EPMCDME-14148). console.error(chalk.red(`\n✗ ${validationResult.error}\n`)); return false; } diff --git a/src/utils/__tests__/interactive.test.ts b/src/utils/__tests__/interactive.test.ts index c7b7670ad..65b354aad 100644 --- a/src/utils/__tests__/interactive.test.ts +++ b/src/utils/__tests__/interactive.test.ts @@ -57,8 +57,7 @@ describe('isNonInteractiveOutput', () => { expect(isNonInteractiveOutput()).toBe(false); }); - // The two cases that motivate a separate predicate: input and output are - // redirected independently, so the two must be able to disagree. + // Input and output redirect independently — the two must be able to disagree. it('should track stderr, not stdin, when only stdin is redirected', async () => { process.stdin.isTTY = false as unknown as true; process.stderr.isTTY = true; diff --git a/src/utils/__tests__/process-guards.logfile.test.ts b/src/utils/__tests__/process-guards.logfile.test.ts index 6fd1b1cbc..ca77c119c 100644 --- a/src/utils/__tests__/process-guards.logfile.test.ts +++ b/src/utils/__tests__/process-guards.logfile.test.ts @@ -1,9 +1,5 @@ -/** - * Companion to process-guards.test.ts, which mocks the logger wholesale and so - * cannot catch a broken logging contract. This file uses the REAL logger and - * asserts the stack actually reaches the log file — the failure mode that - * shipped undetected in the first round (CR-001). - */ +/** Uses the REAL logger — process-guards.test.ts mocks it and so cannot catch a + * broken logging contract (CR-001). */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { readFileSync, existsSync } from 'node:fs'; @@ -39,9 +35,7 @@ describe('installProcessGuards log-file persistence', () => { const { logger } = await import('../logger.js'); const { installProcessGuards } = await import('../process-guards.js'); - // Asserted rather than early-returned: a silent `return` here would make - // this test pass vacuously in any environment where log init fails, which - // is exactly the coverage this file exists to provide. + // Asserted, not early-returned: a silent return would pass vacuously. const logPath = logger.getLogFilePath(); expect(logPath).toBeTruthy(); @@ -58,7 +52,7 @@ describe('installProcessGuards log-file persistence', () => { const appended = after.slice(before.length); expect(appended).toContain(marker); - // A stack, not just the message — the whole point of relocating it. + // A stack, not just the message. expect(appended).toMatch(/\n\s+at\s/); expect(appended).not.toContain('[object Object]'); }); diff --git a/src/utils/__tests__/process-guards.test.ts b/src/utils/__tests__/process-guards.test.ts index fae111731..9d0a4f6bc 100644 --- a/src/utils/__tests__/process-guards.test.ts +++ b/src/utils/__tests__/process-guards.test.ts @@ -59,7 +59,6 @@ describe('installProcessGuards', () => { const output = stderr.join('\n'); expect(output).toContain('credentials unavailable'); - // The guide forbids stack traces on the console; they belong in the log file. expect(output).not.toContain('at '); }); @@ -83,8 +82,8 @@ describe('installProcessGuards', () => { expect(() => handlers.uncaughtException(boom)).toThrow('process.exit:1'); - // logger.error only extracts .stack when the 2nd arg is `instanceof Error`; - // wrapping it in an object literal stringifies to "[object Object]". + // logger.error unwraps .stack only from an Error; an object literal + // stringifies to "[object Object]". expect(logger.error).toHaveBeenCalledWith(expect.any(String), boom); }); diff --git a/src/utils/__tests__/sdk-client.test.ts b/src/utils/__tests__/sdk-client.test.ts index 1ad9f07bc..dacd0c36d 100644 --- a/src/utils/__tests__/sdk-client.test.ts +++ b/src/utils/__tests__/sdk-client.test.ts @@ -82,9 +82,6 @@ describe('getCodemieClient spinner behaviour', () => { expect(oraFactory).toHaveBeenCalled(); }); - // The two cases below are the point of the change: the spinner writes to - // stderr, so it must follow stderr's TTY-ness, not stdin's. - it('suppresses the spinner when stdout/stderr are redirected but stdin is a TTY', async () => { const { ConfigurationError } = await import('../errors.js'); const { getCodemieClient } = await arrange({ diff --git a/src/utils/auth.ts b/src/utils/auth.ts index 7c4e1f27d..34e3b52d8 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -48,9 +48,8 @@ export async function getAuthenticatedClient(config: ProviderProfile): Promise log 2>&1`, which - * is the case that fills captured logs with cursor-control escapes. + * Separate from isNonInteractiveEnvironment() because input and output redirect + * independently: `cmd > log 2>&1` still has an interactive stdin. */ export function isNonInteractiveOutput(): boolean { return !process.stderr.isTTY; diff --git a/src/utils/process-guards.ts b/src/utils/process-guards.ts index fc84f2178..0602c7c7c 100644 --- a/src/utils/process-guards.ts +++ b/src/utils/process-guards.ts @@ -1,11 +1,9 @@ /** - * Process-level error guards + * Process-level error guards. * - * Commander actions are async, and `program.parse()` is synchronous, so a - * rejection escaping an action reaches neither the action's try/catch nor the - * import().catch() in bin/codemie.js. Without a net, Node's default handler - * prints a raw stack trace. These guards are the last line of defence - * (EPMCDME-14148); commands should still handle their own errors. + * program.parse() is synchronous, so a rejection escaping an async commander + * action reaches neither the action's try/catch nor the import().catch() in + * bin/codemie.js — Node then prints a raw stack trace (EPMCDME-14148). */ import { appendFileSync } from 'node:fs'; @@ -14,13 +12,7 @@ import { getErrorMessage } from './errors.js'; import { logger } from './logger.js'; import { sanitizeLogArgs } from './security.js'; -/** - * Append the fatal detail synchronously. - * - * logger writes through an fs.WriteStream, whose write() is asynchronous; - * process.exit() does not drain it, so on a cold stream the entry is lost - * entirely. A fatal is exactly when the record matters most. - */ +/** logger writes through a WriteStream that process.exit() does not drain. */ function persistFatalSync(kind: string, payload: unknown): void { try { const logPath = logger.getLogFilePath(); @@ -46,15 +38,12 @@ function persistFatalSync(kind: string, payload: unknown): void { function reportFatal(kind: string, payload: unknown): never { const message = getErrorMessage(payload); - // Set first: if anything below exits early, the code is still non-zero. process.exitCode = 1; - // Pass the payload itself — logger extracts .message/.stack only from a real - // Error; an object literal would stringify to "[object Object]". + // Pass payload itself — logger unwraps .stack only from a real Error. logger.error(`${kind}: ${message}`, payload); persistFatalSync(kind, payload); - // Console gets the actionable line only; the stack belongs in the log file. console.error(chalk.red(`\n❌ ${message}\n`)); process.exit(1); } @@ -62,12 +51,8 @@ function reportFatal(kind: string, payload: unknown): never { let installed = false; /** - * Register process-level handlers for unhandled rejections and uncaught - * exceptions so they surface as a formatted message rather than a stack trace. - * - * Called from each bin/* entrypoint rather than from AgentCLI, so constructing - * an AgentCLI never mutates global process state. Idempotent as a guard against - * one process loading more than one entrypoint. + * Called from each bin/* entrypoint, never from the AgentCLI constructor — + * constructing an AgentCLI must not mutate global process state (unit tests do). */ export function installProcessGuards(): void { if (installed) { diff --git a/src/utils/sdk-client.ts b/src/utils/sdk-client.ts index 25ac683e7..f2ddf5e64 100644 --- a/src/utils/sdk-client.ts +++ b/src/utils/sdk-client.ts @@ -22,10 +22,7 @@ import { logger } from './logger.js'; * @throws ConfigurationError if setup is incomplete or credentials are invalid */ export async function getCodemieClient(quiet = false): Promise { - // ora writes to stderr, so the spinner follows stderr's TTY-ness, not - // stdin's. Gating on stdin would still emit cursor-control escapes into - // `cmd > log 2>&1` while needlessly dropping the spinner for `cmd < input` - // (EPMCDME-14148). + // ora writes to stderr, so gate on stderr, not stdin (EPMCDME-14148). const showProgress = !quiet && !isNonInteractiveOutput(); let spinner; diff --git a/tests/helpers/cli-runner.ts b/tests/helpers/cli-runner.ts index 10ba2f797..752280b23 100644 --- a/tests/helpers/cli-runner.ts +++ b/tests/helpers/cli-runner.ts @@ -12,10 +12,9 @@ export interface CommandResult { exitCode: number; error?: string; /** - * True when the child was killed by the `timeout` option rather than exiting - * on its own. execSync reports ETIMEDOUT with `status: null`, which would - * otherwise collapse to exitCode 1 and be indistinguishable from a clean - * failure — letting a hang masquerade as a passing test. + * Killed by the `timeout` option rather than exiting on its own. execSync + * reports ETIMEDOUT with `status: null`, which would otherwise collapse to + * exitCode 1 and let a hang masquerade as a clean failure. */ timedOut?: boolean; } @@ -61,9 +60,6 @@ export class CLIRunner { output, exitCode: error.status || 1, error: errorOutput, - // ETIMEDOUT alone covers every timeout mode, including a child that - // ignores SIGTERM. Testing signal === 'SIGTERM' as well adds nothing and - // misreports an unrelated SIGTERM death as a hang. timedOut: error.code === 'ETIMEDOUT', }; } diff --git a/tests/integration/cli-commands/non-interactive-auth.test.ts b/tests/integration/cli-commands/non-interactive-auth.test.ts index cd3659c7d..9d47407a5 100644 --- a/tests/integration/cli-commands/non-interactive-auth.test.ts +++ b/tests/integration/cli-commands/non-interactive-auth.test.ts @@ -1,10 +1,4 @@ -/** - * EPMCDME-14148 — non-interactive SSO failure must fail cleanly. - * - * End-to-end proof of the acceptance criterion: with no valid SSO session and - * a non-TTY stdin, the CLI exits non-zero with actionable remediation and - * without a raw stack trace. - */ +/** EPMCDME-14148 — non-interactive SSO failure must exit cleanly, end to end. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { mkdtempSync, rmSync } from 'node:fs'; @@ -17,8 +11,7 @@ describe('non-interactive SSO auth failure', () => { let isolatedHome: string; beforeAll(() => { - // An empty home guarantees "no valid SSO session" without touching the - // developer's real ~/.codemie credentials. + // Empty home = "no valid SSO session", without touching the real ~/.codemie. isolatedHome = mkdtempSync(join(tmpdir(), 'codemie-14148-')); }); @@ -33,24 +26,16 @@ describe('non-interactive SSO auth failure', () => { const startedAt = Date.now(); const result = runner.runSilent(command, { env: { ...process.env, CODEMIE_HOME: isolatedHome }, - // stdin from 'ignore' is not a TTY, which is the condition under test. + // 'ignore' stdin is not a TTY — the condition under test. stdio: ['ignore', 'pipe', 'pipe'], - // runSilent wraps execSync, which blocks the worker synchronously — - // Vitest's testTimeout cannot interrupt it. Without this, a regression to - // the original hang would wedge CI instead of failing here. + // execSync blocks the worker, so Vitest's testTimeout cannot interrupt a hang. timeout: 15_000, }); - // On ETIMEDOUT execSync reports status: null, which runSilent collapses to - // exitCode 1 — and stderr already holds whatever was printed before the - // block. Without this guard a genuine 15s hang passes every assertion - // below, including the one named for it. + // Without these two, a hang satisfies every assertion below — including the + // one named for it: ETIMEDOUT yields status null, which collapses to exit 1, + // and stderr already holds what was printed before the block. expect(result.timedOut ?? false).toBe(false); - - // The timeout only catches a hang that runs the full 15s. A regression that - // blocked for a few seconds would still satisfy every assertion below and - // merely slow the suite. Observed runtime is ~280ms, so this leaves ~18x - // headroom while still catching a stall. expect(Date.now() - startedAt).toBeLessThan(5_000); return { ...result, combined: `${result.output}\n${result.error ?? ''}` }; @@ -65,8 +50,7 @@ describe('non-interactive SSO auth failure', () => { it('names the remediation the user should run', () => { const result = runWithoutTty('sdk assistants list'); - // Asserted verbatim: a loose /codemie setup/ match is also satisfied by - // unrelated setup advice from other failure paths. + // Verbatim: a loose /codemie setup/ also matches other failure paths. expect(result.combined).toContain(EXPECTED_MESSAGE); }); From cc6939a13ff6ec50b699c3f84d17d486906802c0 Mon Sep 17 00:00:00 2001 From: SleepySML Date: Fri, 4 Sep 2026 12:43:12 +0300 Subject: [PATCH 16/16] refactor(cli): apply reuse, simplification and efficiency findings Four parallel review passes over the EPMCDME-14148 diff. Correctness, and the most valuable find: the integration test built its child env as {...process.env, CODEMIE_HOME}, which does not strip inherited CODEMIE_* vars. ConfigLoader.loadFromEnv reads CODEMIE_PROVIDER and friends straight from the environment, so an empty home does not by itself mean "no valid SSO session" - run from a CodeMie agent shell the test either fails spuriously or passes vacuously. tests/helpers/sso-auth already exports ssoCleanEnv() for exactly this and six neighbouring integration tests use it. This is the same env pollution that derailed the AC4 investigation, reproduced in the test written to prevent it. Efficiency: the integration test spawned the identical command four times. One spawn in beforeAll feeding four it() blocks cuts the file from ~1.05s to ~280ms with no loss of per-assertion granularity. Removed test cases that discriminate nothing: two of four TTY combinations in sdk-client and two of four in interactive passed under both the correct implementation and the stdin-gated one they exist to rule out. Verified the survivors still fail against that regression. Merged two cli-utils tests that duplicated a 13-line arrangement to assert two halves of one behaviour. Flattened the nested try in auth.ts to a .catch on the prompt alone - wrapping the retry too would swallow its own failure. reportFatal now appends "(see )", reusing the pattern from logger.notice: it strips the stack from the console and previously left no pointer to where it went. Adds a coverage test asserting every bin in package.json either installs the guards or is an explicitly justified exclusion, so a new entrypoint fails CI instead of silently shipping without a net. Verified it fails when a guard is removed. Documents why mcp-proxy and proxy-daemon opt out. Measured and left alone: the new import adds 0.29ms to startup, no new packages, keytar still lazy. The logger.error/appendFileSync double write is justified - the logger's file write never survives process.exit, so appendFileSync is the only durable path and logger.error survives only as the CODEMIE_DEBUG console echo. EPMCDME-14148 Co-Authored-By: Claude --- bin/codemie-mcp-proxy.js | 3 + bin/proxy-daemon.js | 3 + .../sdk/utils/__tests__/cli-utils.test.ts | 17 +---- src/utils/__tests__/interactive.test.ts | 16 +---- .../__tests__/process-guards.coverage.test.ts | 63 +++++++++++++++++++ src/utils/__tests__/process-guards.test.ts | 8 ++- src/utils/__tests__/sdk-client.test.ts | 20 ------ src/utils/auth.ts | 12 ++-- src/utils/process-guards.ts | 22 ++++--- .../cli-commands/non-interactive-auth.test.ts | 39 +++++------- 10 files changed, 113 insertions(+), 90 deletions(-) create mode 100644 src/utils/__tests__/process-guards.coverage.test.ts diff --git a/bin/codemie-mcp-proxy.js b/bin/codemie-mcp-proxy.js index dc895b282..ab74c34ee 100755 --- a/bin/codemie-mcp-proxy.js +++ b/bin/codemie-mcp-proxy.js @@ -16,6 +16,9 @@ import { appendFileSync, mkdirSync } from 'fs'; import { join } from 'path'; import { homedir } from 'os'; +// No installProcessGuards(): this entrypoint owns its own uncaughtException/ +// unhandledRejection handlers below and deliberately survives rejections. + // Boot-level file logger (before any imports that might touch stdout) const logDir = join(homedir(), '.codemie', 'logs'); const logFile = join(logDir, 'mcp-proxy.log'); diff --git a/bin/proxy-daemon.js b/bin/proxy-daemon.js index 619914ef3..485bbe422 100755 --- a/bin/proxy-daemon.js +++ b/bin/proxy-daemon.js @@ -3,6 +3,9 @@ /** * CodeMie Proxy Daemon entry point * Imports compiled daemon from dist/ + * + * No installProcessGuards(): long-running daemon — exiting on the first + * unhandled rejection would be wrong here. */ import('../dist/bin/proxy-daemon.js').catch((error) => { process.stderr.write(`[proxy-daemon] Fatal: ${error.message}\n`); diff --git a/src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts b/src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts index 9686c80cb..7c54e76f3 100644 --- a/src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts +++ b/src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts @@ -36,7 +36,7 @@ describe('getSdkClient', () => { vi.resetModules(); }); - it('exits non-zero through handleSdkError when authentication fails', async () => { + it('exits non-zero via handleSdkError with the remediation on stderr', async () => { const { ConfigLoader } = await import('@/utils/config.js'); const { getAuthenticatedClient } = await import('@/utils/auth.js'); const { ConfigurationError } = await import('@/utils/errors.js'); @@ -51,21 +51,6 @@ describe('getSdkClient', () => { // handleSdkError terminates the process; the spy converts that into a throw. await expect(getSdkClient()).rejects.toThrow('process.exit:1'); expect(exitCode).toBe(1); - }); - - it('surfaces the actionable remediation on stderr rather than a raw stack trace', async () => { - const { ConfigLoader } = await import('@/utils/config.js'); - const { getAuthenticatedClient } = await import('@/utils/auth.js'); - const { ConfigurationError } = await import('@/utils/errors.js'); - - vi.mocked(ConfigLoader.load).mockResolvedValue({} as never); - vi.mocked(getAuthenticatedClient).mockRejectedValue( - new ConfigurationError(SSO_ERROR_MESSAGE) - ); - - const { getSdkClient } = await import('../cli-utils.js'); - - await expect(getSdkClient()).rejects.toThrow('process.exit:1'); expect(stderr.join('\n')).toContain('codemie setup'); }); diff --git a/src/utils/__tests__/interactive.test.ts b/src/utils/__tests__/interactive.test.ts index 65b354aad..57341aebd 100644 --- a/src/utils/__tests__/interactive.test.ts +++ b/src/utils/__tests__/interactive.test.ts @@ -41,21 +41,7 @@ describe('isNonInteractiveOutput', () => { process.stderr.isTTY = originalStderr; }); - it('should return true when process.stderr.isTTY is undefined (redirected output)', async () => { - process.stderr.isTTY = undefined as unknown as true; - - const { isNonInteractiveOutput } = await import('../interactive.js'); - - expect(isNonInteractiveOutput()).toBe(true); - }); - it('should return false when process.stderr.isTTY is true (terminal output)', async () => { - process.stderr.isTTY = true; - - const { isNonInteractiveOutput } = await import('../interactive.js'); - - expect(isNonInteractiveOutput()).toBe(false); - }); // Input and output redirect independently — the two must be able to disagree. it('should track stderr, not stdin, when only stdin is redirected', async () => { @@ -72,7 +58,7 @@ describe('isNonInteractiveOutput', () => { it('should track stderr, not stdin, when only output is redirected', async () => { process.stdin.isTTY = true; - process.stderr.isTTY = false as unknown as true; + process.stderr.isTTY = undefined as unknown as true; const { isNonInteractiveOutput, isNonInteractiveEnvironment } = await import( '../interactive.js' diff --git a/src/utils/__tests__/process-guards.coverage.test.ts b/src/utils/__tests__/process-guards.coverage.test.ts new file mode 100644 index 000000000..455b8a588 --- /dev/null +++ b/src/utils/__tests__/process-guards.coverage.test.ts @@ -0,0 +1,63 @@ +/** + * Every declared bin must either install the process guards or be an explicit, + * justified exclusion — so a new entrypoint fails here rather than silently + * shipping without a fatal-error net (EPMCDME-14148). + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { getDirname } from '../paths.js'; + +const REPO_ROOT = join(getDirname(import.meta.url), '..', '..', '..'); + +/** + * Long-running processes must survive an unhandled rejection; installProcessGuards + * exits on the first one, which would be wrong for them. + */ +const EXCLUDED: Record = { + 'codemie-mcp-proxy': + 'owns uncaughtException/unhandledRejection handlers with a deliberate survive-on-rejection policy', + 'proxy-daemon': 'long-running daemon — must not exit on the first unhandled rejection', +}; + +describe('process guard coverage across bin entrypoints', () => { + const pkg = JSON.parse( + readFileSync(join(REPO_ROOT, 'package.json'), 'utf-8') + ) as { bin: Record }; + + const entrypoints = Object.entries(pkg.bin); + + it('declares at least the known entrypoints', () => { + expect(entrypoints.length).toBeGreaterThanOrEqual(14); + }); + + it.each(entrypoints)('%s installs the guards or is explicitly excluded', (name, relPath) => { + // Strip comments first: the excluded entrypoints name the function in a + // comment explaining why they opt out. + const source = readFileSync(join(REPO_ROOT, relPath), 'utf-8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/^\s*\/\/.*$/gm, ''); + const installs = /^\s*installProcessGuards\(\);/m.test(source); + + if (name in EXCLUDED) { + expect( + installs, + `${name} is listed as excluded (${EXCLUDED[name]}) but now installs the guards — drop it from EXCLUDED` + ).toBe(false); + return; + } + + expect( + installs, + `${name} (${relPath}) neither installs the guards nor is listed in EXCLUDED with a reason` + ).toBe(true); + }); + + it('lists no stale exclusions', () => { + const declared = new Set(entrypoints.map(([name]) => name)); + for (const name of Object.keys(EXCLUDED)) { + expect(declared.has(name), `EXCLUDED lists ${name}, which package.json no longer declares`).toBe(true); + } + }); +}); diff --git a/src/utils/__tests__/process-guards.test.ts b/src/utils/__tests__/process-guards.test.ts index 9d0a4f6bc..a572e05cc 100644 --- a/src/utils/__tests__/process-guards.test.ts +++ b/src/utils/__tests__/process-guards.test.ts @@ -1,7 +1,13 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; vi.mock('../logger.js', () => ({ - logger: { error: vi.fn(), debug: vi.fn(), warn: vi.fn(), info: vi.fn() }, + logger: { + error: vi.fn(), + debug: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + getLogFilePath: vi.fn(() => null), + }, })); type Handler = (payload: unknown) => void; diff --git a/src/utils/__tests__/sdk-client.test.ts b/src/utils/__tests__/sdk-client.test.ts index dacd0c36d..97931f371 100644 --- a/src/utils/__tests__/sdk-client.test.ts +++ b/src/utils/__tests__/sdk-client.test.ts @@ -60,27 +60,7 @@ describe('getCodemieClient spinner behaviour', () => { return import('../sdk-client.js'); } - it('does not start a spinner when the output stream is not a TTY', async () => { - const { ConfigurationError } = await import('../errors.js'); - const { getCodemieClient } = await arrange({ - stdinTty: false, - stderrTty: false, - }); - - await expect(getCodemieClient()).rejects.toThrow(ConfigurationError); - expect(oraFactory).not.toHaveBeenCalled(); - }); - it('starts a spinner when the output stream is a TTY', async () => { - const { ConfigurationError } = await import('../errors.js'); - const { getCodemieClient } = await arrange({ - stdinTty: true, - stderrTty: true, - }); - - await expect(getCodemieClient()).rejects.toThrow(ConfigurationError); - expect(oraFactory).toHaveBeenCalled(); - }); it('suppresses the spinner when stdout/stderr are redirected but stdin is a TTY', async () => { const { ConfigurationError } = await import('../errors.js'); diff --git a/src/utils/auth.ts b/src/utils/auth.ts index 34e3b52d8..5342a3fe7 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -44,14 +44,12 @@ export async function getAuthenticatedClient(config: ProviderProfile): Promise { throw error; - } + }); if (reauthed) { return await getCodemieClient(); } diff --git a/src/utils/process-guards.ts b/src/utils/process-guards.ts index 0602c7c7c..35021748e 100644 --- a/src/utils/process-guards.ts +++ b/src/utils/process-guards.ts @@ -13,13 +13,8 @@ import { logger } from './logger.js'; import { sanitizeLogArgs } from './security.js'; /** logger writes through a WriteStream that process.exit() does not drain. */ -function persistFatalSync(kind: string, payload: unknown): void { +function persistFatalSync(logPath: string, kind: string, payload: unknown): void { try { - const logPath = logger.getLogFilePath(); - if (!logPath) { - return; - } - const detail = payload instanceof Error && payload.stack ? payload.stack @@ -38,13 +33,22 @@ function persistFatalSync(kind: string, payload: unknown): void { function reportFatal(kind: string, payload: unknown): never { const message = getErrorMessage(payload); + // Set before anything that could exit early: console.error can throw EPIPE + // under `codemie … | head`, skipping process.exit() below. process.exitCode = 1; - // Pass payload itself — logger unwraps .stack only from a real Error. + // Its file write never survives process.exit() — measured. Kept for the + // CODEMIE_DEBUG console echo; persistFatalSync is the durable path. logger.error(`${kind}: ${message}`, payload); - persistFatalSync(kind, payload); - console.error(chalk.red(`\n❌ ${message}\n`)); + const logPath = logger.getLogFilePath(); + if (logPath) { + persistFatalSync(logPath, kind, payload); + } + + // Point at the stack rather than printing it, matching logger.notice(). + const suffix = logPath ? ` (see ${logPath})` : ''; + console.error(chalk.red(`\n❌ ${message}${suffix}\n`)); process.exit(1); } diff --git a/tests/integration/cli-commands/non-interactive-auth.test.ts b/tests/integration/cli-commands/non-interactive-auth.test.ts index 9d47407a5..2ae274be4 100644 --- a/tests/integration/cli-commands/non-interactive-auth.test.ts +++ b/tests/integration/cli-commands/non-interactive-auth.test.ts @@ -5,27 +5,26 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { CLIRunner } from '../../helpers/cli-runner.js'; +import { ssoCleanEnv } from '../../helpers/sso-auth.js'; + +const EXPECTED_MESSAGE = + 'SSO authentication required. Please run "codemie setup" with SSO provider first.'; describe('non-interactive SSO auth failure', () => { const runner = new CLIRunner(); let isolatedHome: string; + let result: ReturnType & { combined: string }; beforeAll(() => { // Empty home = "no valid SSO session", without touching the real ~/.codemie. isolatedHome = mkdtempSync(join(tmpdir(), 'codemie-14148-')); - }); - - afterAll(() => { - rmSync(isolatedHome, { recursive: true, force: true }); - }); - const EXPECTED_MESSAGE = - 'SSO authentication required. Please run "codemie setup" with SSO provider first.'; - - function runWithoutTty(command: string) { const startedAt = Date.now(); - const result = runner.runSilent(command, { - env: { ...process.env, CODEMIE_HOME: isolatedHome }, + const raw = runner.runSilent('sdk assistants list', { + // ssoCleanEnv strips CODEMIE_*: ConfigLoader.loadFromEnv reads + // CODEMIE_PROVIDER/API_KEY/URL directly, so an empty home alone does not + // guarantee "no valid SSO session" when run from a CodeMie agent shell. + env: { ...ssoCleanEnv(), CODEMIE_HOME: isolatedHome }, // 'ignore' stdin is not a TTY — the condition under test. stdio: ['ignore', 'pipe', 'pipe'], // execSync blocks the worker, so Vitest's testTimeout cannot interrupt a hang. @@ -35,35 +34,31 @@ describe('non-interactive SSO auth failure', () => { // Without these two, a hang satisfies every assertion below — including the // one named for it: ETIMEDOUT yields status null, which collapses to exit 1, // and stderr already holds what was printed before the block. - expect(result.timedOut ?? false).toBe(false); + expect(raw.timedOut ?? false).toBe(false); expect(Date.now() - startedAt).toBeLessThan(5_000); - return { ...result, combined: `${result.output}\n${result.error ?? ''}` }; - } + result = { ...raw, combined: `${raw.output}\n${raw.error ?? ''}` }; + }); + + afterAll(() => { + rmSync(isolatedHome, { recursive: true, force: true }); + }); it('exits non-zero instead of hanging on a re-authentication prompt', () => { - const result = runWithoutTty('sdk assistants list'); - expect(result.exitCode).not.toBe(0); }); it('names the remediation the user should run', () => { - const result = runWithoutTty('sdk assistants list'); - // Verbatim: a loose /codemie setup/ also matches other failure paths. expect(result.combined).toContain(EXPECTED_MESSAGE); }); it('sends the diagnostic to stderr and keeps it off stdout', () => { - const result = runWithoutTty('sdk assistants list'); - expect(result.error ?? '').toContain(EXPECTED_MESSAGE); expect(result.output).not.toContain(EXPECTED_MESSAGE); }); it('does not print a raw stack trace', () => { - const result = runWithoutTty('sdk assistants list'); - expect(result.combined).not.toMatch(/^\s+at\s+/m); expect(result.combined).not.toContain('ConfigurationError:'); expect(result.combined).not.toMatch(/Node\.js v\d/);