Skip to content

Commit e145b9d

Browse files
author
AgentCore Bot
committed
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 <bogus> 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.
1 parent fe038d8 commit e145b9d

3 files changed

Lines changed: 95 additions & 2 deletions

File tree

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
import { runCLI } from '../../../../test-utils/index.js';
2+
import { randomUUID } from 'node:crypto';
3+
import { mkdir, rm, writeFile } from 'node:fs/promises';
4+
import { tmpdir } from 'node:os';
5+
import { join } from 'node:path';
6+
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
7+
8+
/**
9+
* Regression test for issue #982 (the command named in the bug title):
10+
* `agentcore deploy` (and any TUI-mode invocation) in a non-TTY context
11+
* must NOT crash with the Ink "Raw mode is not supported" error and must
12+
* NOT exit 0. It must exit 1 with a clear interactive-terminal message so
13+
* CI pipelines can detect the failure.
14+
*
15+
* The TUI path is reached when no flags are provided (and also for --diff).
16+
* Flag-bearing modes (--yes, --json, --dry-run, --target, --verbose) route
17+
* to the non-interactive CLI path which does not require a TTY and is not
18+
* the subject of this regression.
19+
*/
20+
describe('deploy command non-TTY behavior (issue #982)', () => {
21+
let testDir: string;
22+
let projectDir: string;
23+
24+
beforeAll(async () => {
25+
testDir = join(tmpdir(), `agentcore-deploy-tty-${randomUUID()}`);
26+
projectDir = join(testDir, 'project');
27+
const configDir = join(projectDir, 'agentcore');
28+
await mkdir(configDir, { recursive: true });
29+
// Minimal valid project marker so requireProject() passes — guard
30+
// ordering means requireTTY() will fire first, but we keep a real
31+
// project dir so an accidental ordering regression surfaces clearly.
32+
await writeFile(join(configDir, 'agentcore.json'), JSON.stringify({ name: 'test', version: 1 }, null, 2), 'utf-8');
33+
});
34+
35+
afterAll(async () => {
36+
await rm(testDir, { recursive: true, force: true });
37+
});
38+
39+
it('exits 1 with a clear message when run without flags in non-TTY (interactive path)', async () => {
40+
// runCLI uses stdio: ['ignore', 'pipe', 'pipe'] — both stdin and stdout
41+
// are non-TTY, exactly the scenario from the bug report.
42+
const result = await runCLI(['deploy'], projectDir);
43+
44+
expect(result.exitCode).toBe(1);
45+
46+
const combined = `${result.stdout}\n${result.stderr}`;
47+
expect(combined.toLowerCase()).toContain('requires an interactive terminal');
48+
expect(combined).not.toContain('Raw mode is not supported');
49+
});
50+
51+
it('exits 1 cleanly with --diff in non-TTY (TUI diff path)', async () => {
52+
const result = await runCLI(['deploy', '--diff'], projectDir);
53+
54+
expect(result.exitCode).toBe(1);
55+
56+
const combined = `${result.stdout}\n${result.stderr}`;
57+
expect(combined).not.toContain('Raw mode is not supported');
58+
});
59+
});

src/cli/commands/import/__tests__/command.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,4 +40,19 @@ describe('import command non-TTY behavior (issue #982)', () => {
4040
expect(combined.toLowerCase()).toContain('requires an interactive terminal');
4141
expect(combined).not.toContain('Raw mode is not supported');
4242
});
43+
44+
it('does not apply the TTY guard to --source path (pins guard placement)', async () => {
45+
// The requireTTY() guard must remain inside the no-source branch only.
46+
// If it gets accidentally hoisted to the top of the action, this test
47+
// will start failing with the interactive-terminal message instead of
48+
// the source-file-validation message.
49+
const bogusSource = join(projectDir, 'definitely-does-not-exist.yaml');
50+
const result = await runCLI(['import', '--source', bogusSource], projectDir);
51+
52+
expect(result.exitCode).toBe(1);
53+
54+
const combined = `${result.stdout}\n${result.stderr}`;
55+
expect(combined).toContain('Source file not found');
56+
expect(combined.toLowerCase()).not.toContain('requires an interactive terminal');
57+
});
4358
});

src/cli/tui/guards/__tests__/tty.test.ts

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,31 @@ import { requireTTY } from '../tty.js';
22
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
33

44
describe('requireTTY', () => {
5-
const originalStdinIsTTY = process.stdin.isTTY;
6-
const originalStdoutIsTTY = process.stdout.isTTY;
5+
let originalStdinIsTTY: boolean | undefined;
6+
let originalStdoutIsTTY: boolean | undefined;
77
let exitSpy: ReturnType<typeof vi.spyOn>;
88
let errorSpy: ReturnType<typeof vi.spyOn>;
99

1010
beforeEach(() => {
11+
// Snapshot the live values inside beforeEach so a previous test file in
12+
// the same worker that mutated process.stdin/stdout.isTTY without
13+
// restoring cannot poison our restoration. Also re-define with
14+
// configurable: true to make subsequent restoration deterministic
15+
// across Node versions where the original descriptor for piped streams
16+
// may not be configurable.
17+
originalStdinIsTTY = process.stdin.isTTY;
18+
originalStdoutIsTTY = process.stdout.isTTY;
19+
Object.defineProperty(process.stdin, 'isTTY', {
20+
value: originalStdinIsTTY,
21+
configurable: true,
22+
writable: true,
23+
});
24+
Object.defineProperty(process.stdout, 'isTTY', {
25+
value: originalStdoutIsTTY,
26+
configurable: true,
27+
writable: true,
28+
});
29+
1130
// Throw from process.exit so we can assert without actually exiting the test runner.
1231
exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => {
1332
throw new Error(`__exit__:${code}`);

0 commit comments

Comments
 (0)