From a9bd408b329ad1595fe387b8987a885c2931e9c6 Mon Sep 17 00:00:00 2001 From: AgentCore Bot Date: Thu, 7 May 2026 19:32:59 +0000 Subject: [PATCH 1/3] fix: fix issue #982 --- .../commands/import/__tests__/command.test.ts | 49 ++++++++++++++ src/cli/commands/import/command.ts | 2 + src/cli/tui/guards/__tests__/tty.test.ts | 65 +++++++++++++++++++ 3 files changed, 116 insertions(+) create mode 100644 src/cli/commands/import/__tests__/command.test.ts create mode 100644 src/cli/tui/guards/__tests__/tty.test.ts diff --git a/src/cli/commands/import/__tests__/command.test.ts b/src/cli/commands/import/__tests__/command.test.ts new file mode 100644 index 000000000..687ed8f56 --- /dev/null +++ b/src/cli/commands/import/__tests__/command.test.ts @@ -0,0 +1,49 @@ +import { runCLI } from '../../../../test-utils/index.js'; +import { randomUUID } from 'node:crypto'; +import { mkdir, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +/** + * Regression test for issue #982: + * Running `agentcore import` (interactive TUI path) with a non-TTY stdin/stdout + * must NOT crash with the Ink "Raw mode is not supported" error and must NOT + * exit 0. It must exit 1 with a clear interactive-terminal message so that + * CI pipelines can detect the failure. + */ +describe('import command non-TTY behavior (issue #982)', () => { + let testDir: string; + let projectDir: string; + + beforeAll(async () => { + testDir = join(tmpdir(), `agentcore-import-tty-${randomUUID()}`); + projectDir = join(testDir, 'project'); + const configDir = join(projectDir, 'agentcore'); + await mkdir(configDir, { recursive: true }); + // Minimal valid project marker so requireProject() passes. + await writeFile(join(configDir, 'agentcore.json'), JSON.stringify({ name: 'test', version: 1 }, null, 2), 'utf-8'); + }); + + afterAll(async () => { + await rm(testDir, { recursive: true, force: true }); + }); + + it('exits non-zero with a clear message when stdin is not a TTY', async () => { + // runCLI uses stdio: ['ignore', 'pipe', 'pipe'] — both stdin and stdout + // are non-TTY, exactly the scenario from the bug report. + const result = await runCLI(['import'], projectDir); + + expect(result.exitCode).toBe(1); + + const combined = `${result.stdout}\n${result.stderr}`; + expect( + combined.toLowerCase().includes('requires an interactive terminal'), + `Should print interactive-terminal message, got stdout=${result.stdout}, stderr=${result.stderr}` + ).toBeTruthy(); + expect( + !combined.includes('Raw mode is not supported'), + `Must not surface the Ink raw-mode error, got: ${combined}` + ).toBeTruthy(); + }); +}); diff --git a/src/cli/commands/import/command.ts b/src/cli/commands/import/command.ts index 381167aaa..1e539f72a 100644 --- a/src/cli/commands/import/command.ts +++ b/src/cli/commands/import/command.ts @@ -24,7 +24,9 @@ export const registerImport = (program: Command) => { if (!cliOptions.source) { // No --source and no subcommand — launch interactive TUI const { requireProject } = await import('../../tui/guards/project'); + const { requireTTY } = await import('../../tui/guards/tty'); requireProject(); + requireTTY(); const { render } = await import('ink'); const React = await import('react'); const { ImportFlow } = await import('../../tui/screens/import'); diff --git a/src/cli/tui/guards/__tests__/tty.test.ts b/src/cli/tui/guards/__tests__/tty.test.ts new file mode 100644 index 000000000..bfaaaff14 --- /dev/null +++ b/src/cli/tui/guards/__tests__/tty.test.ts @@ -0,0 +1,65 @@ +import { requireTTY } from '../tty.js'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +describe('requireTTY', () => { + const originalStdinIsTTY = process.stdin.isTTY; + const originalStdoutIsTTY = process.stdout.isTTY; + let exitSpy: ReturnType; + let errorSpy: ReturnType; + + beforeEach(() => { + // Throw from process.exit so we can assert without actually exiting the test runner. + exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + throw new Error(`__exit__:${code}`); + }) as never); + errorSpy = vi.spyOn(console, 'error').mockImplementation(() => { + /* swallow output during tests */ + }); + }); + + afterEach(() => { + Object.defineProperty(process.stdin, 'isTTY', { value: originalStdinIsTTY, configurable: true }); + Object.defineProperty(process.stdout, 'isTTY', { value: originalStdoutIsTTY, configurable: true }); + exitSpy.mockRestore(); + errorSpy.mockRestore(); + }); + + it('exits 1 with an interactive-terminal message when stdin is not a TTY', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: false, configurable: true }); + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true }); + + expect(() => requireTTY()).toThrow(/__exit__:1/); + expect(exitSpy).toHaveBeenCalledWith(1); + expect(errorSpy).toHaveBeenCalledTimes(1); + const msg = String(errorSpy.mock.calls[0]?.[0] ?? ''); + expect(msg).toMatch(/requires an interactive terminal/); + }); + + it('exits 1 with an interactive-terminal message when stdout is not a TTY', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: true, configurable: true }); + Object.defineProperty(process.stdout, 'isTTY', { value: false, configurable: true }); + + expect(() => requireTTY()).toThrow(/__exit__:1/); + expect(exitSpy).toHaveBeenCalledWith(1); + expect(errorSpy).toHaveBeenCalledTimes(1); + const msg = String(errorSpy.mock.calls[0]?.[0] ?? ''); + expect(msg).toMatch(/requires an interactive terminal/); + }); + + it('exits 1 when both stdin and stdout are non-TTY', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: false, configurable: true }); + Object.defineProperty(process.stdout, 'isTTY', { value: false, configurable: true }); + + expect(() => requireTTY()).toThrow(/__exit__:1/); + expect(exitSpy).toHaveBeenCalledWith(1); + }); + + it('is a no-op when both stdin and stdout are TTYs', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: true, configurable: true }); + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true }); + + expect(() => requireTTY()).not.toThrow(); + expect(exitSpy).not.toHaveBeenCalled(); + expect(errorSpy).not.toHaveBeenCalled(); + }); +}); From 51cfa9d190a977d2ce934d7495298b4aba076d16 Mon Sep 17 00:00:00 2001 From: AgentCore Bot Date: Thu, 7 May 2026 19:45:35 +0000 Subject: [PATCH 2/3] fix: address review findings round 1 - Swap requireTTY() before requireProject() in import command so non-TTY contexts exit cleanly without going through any Ink render path (FatalError) that could emit malformed output to a piped stream. - Hoist requireTTY/requireProject to a static barrel import for consistency with deploy/dev/add/remove/invoke/create. - Replace expect(boolean, msg).toBeTruthy() with toContain assertions in import command test for clearer Vitest failure diagnostics. Notes on findings not addressed: - secretlint version finding is a false positive: this PR does not modify package.json (verified with git diff origin/main...HEAD). - The deploy command already has requireTTY() (since PR #949) and was verified working via reproducer; no change needed there. - The runCLI/dist bundle dependency is the existing convention across the repo; tty.test.ts provides the primary regression guard. --- src/cli/commands/import/__tests__/command.test.ts | 10 ++-------- src/cli/commands/import/command.ts | 11 +++++++---- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/cli/commands/import/__tests__/command.test.ts b/src/cli/commands/import/__tests__/command.test.ts index 687ed8f56..1e8912296 100644 --- a/src/cli/commands/import/__tests__/command.test.ts +++ b/src/cli/commands/import/__tests__/command.test.ts @@ -37,13 +37,7 @@ describe('import command non-TTY behavior (issue #982)', () => { expect(result.exitCode).toBe(1); const combined = `${result.stdout}\n${result.stderr}`; - expect( - combined.toLowerCase().includes('requires an interactive terminal'), - `Should print interactive-terminal message, got stdout=${result.stdout}, stderr=${result.stderr}` - ).toBeTruthy(); - expect( - !combined.includes('Raw mode is not supported'), - `Must not surface the Ink raw-mode error, got: ${combined}` - ).toBeTruthy(); + expect(combined.toLowerCase()).toContain('requires an interactive terminal'); + expect(combined).not.toContain('Raw mode is not supported'); }); }); diff --git a/src/cli/commands/import/command.ts b/src/cli/commands/import/command.ts index 1e539f72a..10ffc4214 100644 --- a/src/cli/commands/import/command.ts +++ b/src/cli/commands/import/command.ts @@ -1,3 +1,4 @@ +import { requireProject, requireTTY } from '../../tui/guards'; import { handleImport } from './actions'; import { ANSI } from './constants'; import { registerImportEvaluator } from './import-evaluator'; @@ -22,11 +23,13 @@ export const registerImport = (program: Command) => { .option('-y, --yes', 'Auto-confirm prompts') .action(async (cliOptions: { source?: string; target?: string; yes?: boolean }) => { if (!cliOptions.source) { - // No --source and no subcommand — launch interactive TUI - const { requireProject } = await import('../../tui/guards/project'); - const { requireTTY } = await import('../../tui/guards/tty'); - requireProject(); + // No --source and no subcommand — launch interactive TUI. + // Call requireTTY() before requireProject() so non-TTY contexts always + // exit with the plain-text message rather than going through any Ink + // rendering path (FatalError uses Ink, which can produce malformed + // output to a piped stream). requireTTY(); + requireProject(); const { render } = await import('ink'); const React = await import('react'); const { ImportFlow } = await import('../../tui/screens/import'); From 36d6c75d7c78b560c9842358e4161f7ad8d162d2 Mon Sep 17 00:00:00 2001 From: AgentCore Bot Date: Thu, 7 May 2026 19:51:14 +0000 Subject: [PATCH 3/3] fix: address review findings round 2 - Add deploy non-TTY regression test (deploy-non-tty.test.ts) that pins exit 1 + clear message + no Ink raw-mode error for the no-flags and --diff TUI paths. Flag-bearing modes (--yes, --json, --dry-run, --target, --verbose) route to the non-interactive CLI path which does not require a TTY. - Add import --source negative test that pins the guard placement: a future regression that hoists requireTTY() to the top of the action will break this test (it would yield the interactive-terminal message instead of 'Source file not found'). - Move originalStdinIsTTY/originalStdoutIsTTY snapshots into beforeEach in tty.test.ts and re-define with configurable: true so prior test contamination cannot poison restoration. --- .../deploy/__tests__/deploy-non-tty.test.ts | 59 +++++++++++++++++++ .../commands/import/__tests__/command.test.ts | 15 +++++ src/cli/tui/guards/__tests__/tty.test.ts | 23 +++++++- 3 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 src/cli/commands/deploy/__tests__/deploy-non-tty.test.ts diff --git a/src/cli/commands/deploy/__tests__/deploy-non-tty.test.ts b/src/cli/commands/deploy/__tests__/deploy-non-tty.test.ts new file mode 100644 index 000000000..e42f47db3 --- /dev/null +++ b/src/cli/commands/deploy/__tests__/deploy-non-tty.test.ts @@ -0,0 +1,59 @@ +import { runCLI } from '../../../../test-utils/index.js'; +import { randomUUID } from 'node:crypto'; +import { mkdir, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +/** + * Regression test for issue #982 (the command named in the bug title): + * `agentcore deploy` (and any TUI-mode invocation) in a non-TTY context + * must NOT crash with the Ink "Raw mode is not supported" error and must + * NOT exit 0. It must exit 1 with a clear interactive-terminal message so + * CI pipelines can detect the failure. + * + * The TUI path is reached when no flags are provided (and also for --diff). + * Flag-bearing modes (--yes, --json, --dry-run, --target, --verbose) route + * to the non-interactive CLI path which does not require a TTY and is not + * the subject of this regression. + */ +describe('deploy command non-TTY behavior (issue #982)', () => { + let testDir: string; + let projectDir: string; + + beforeAll(async () => { + testDir = join(tmpdir(), `agentcore-deploy-tty-${randomUUID()}`); + projectDir = join(testDir, 'project'); + const configDir = join(projectDir, 'agentcore'); + await mkdir(configDir, { recursive: true }); + // Minimal valid project marker so requireProject() passes — guard + // ordering means requireTTY() will fire first, but we keep a real + // project dir so an accidental ordering regression surfaces clearly. + await writeFile(join(configDir, 'agentcore.json'), JSON.stringify({ name: 'test', version: 1 }, null, 2), 'utf-8'); + }); + + afterAll(async () => { + await rm(testDir, { recursive: true, force: true }); + }); + + it('exits 1 with a clear message when run without flags in non-TTY (interactive path)', async () => { + // runCLI uses stdio: ['ignore', 'pipe', 'pipe'] — both stdin and stdout + // are non-TTY, exactly the scenario from the bug report. + const result = await runCLI(['deploy'], projectDir); + + expect(result.exitCode).toBe(1); + + const combined = `${result.stdout}\n${result.stderr}`; + expect(combined.toLowerCase()).toContain('requires an interactive terminal'); + expect(combined).not.toContain('Raw mode is not supported'); + }); + + it('exits 1 cleanly with --diff in non-TTY (TUI diff path)', async () => { + const result = await runCLI(['deploy', '--diff'], projectDir); + + expect(result.exitCode).toBe(1); + + const combined = `${result.stdout}\n${result.stderr}`; + expect(combined).not.toContain('Raw mode is not supported'); + }); +}); diff --git a/src/cli/commands/import/__tests__/command.test.ts b/src/cli/commands/import/__tests__/command.test.ts index 1e8912296..6435cf851 100644 --- a/src/cli/commands/import/__tests__/command.test.ts +++ b/src/cli/commands/import/__tests__/command.test.ts @@ -40,4 +40,19 @@ describe('import command non-TTY behavior (issue #982)', () => { expect(combined.toLowerCase()).toContain('requires an interactive terminal'); expect(combined).not.toContain('Raw mode is not supported'); }); + + it('does not apply the TTY guard to --source path (pins guard placement)', async () => { + // The requireTTY() guard must remain inside the no-source branch only. + // If it gets accidentally hoisted to the top of the action, this test + // will start failing with the interactive-terminal message instead of + // the source-file-validation message. + const bogusSource = join(projectDir, 'definitely-does-not-exist.yaml'); + const result = await runCLI(['import', '--source', bogusSource], projectDir); + + expect(result.exitCode).toBe(1); + + const combined = `${result.stdout}\n${result.stderr}`; + expect(combined).toContain('Source file not found'); + expect(combined.toLowerCase()).not.toContain('requires an interactive terminal'); + }); }); diff --git a/src/cli/tui/guards/__tests__/tty.test.ts b/src/cli/tui/guards/__tests__/tty.test.ts index bfaaaff14..c6d693616 100644 --- a/src/cli/tui/guards/__tests__/tty.test.ts +++ b/src/cli/tui/guards/__tests__/tty.test.ts @@ -2,12 +2,31 @@ import { requireTTY } from '../tty.js'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; describe('requireTTY', () => { - const originalStdinIsTTY = process.stdin.isTTY; - const originalStdoutIsTTY = process.stdout.isTTY; + let originalStdinIsTTY: boolean | undefined; + let originalStdoutIsTTY: boolean | undefined; let exitSpy: ReturnType; let errorSpy: ReturnType; beforeEach(() => { + // Snapshot the live values inside beforeEach so a previous test file in + // the same worker that mutated process.stdin/stdout.isTTY without + // restoring cannot poison our restoration. Also re-define with + // configurable: true to make subsequent restoration deterministic + // across Node versions where the original descriptor for piped streams + // may not be configurable. + originalStdinIsTTY = process.stdin.isTTY; + originalStdoutIsTTY = process.stdout.isTTY; + Object.defineProperty(process.stdin, 'isTTY', { + value: originalStdinIsTTY, + configurable: true, + writable: true, + }); + Object.defineProperty(process.stdout, 'isTTY', { + value: originalStdoutIsTTY, + configurable: true, + writable: true, + }); + // Throw from process.exit so we can assert without actually exiting the test runner. exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { throw new Error(`__exit__:${code}`);