Uh oh!
There was an error while loading. Please reload this page.
fix(import): guard interactive TUI with requireTTY() so non-TTY exits 1 cleanly (#982) - #1170
Closed
aidandaly24 wants to merge 3 commits into
Closed
fix(import): guard interactive TUI with requireTTY() so non-TTY exits 1 cleanly (#982)#1170aidandaly24 wants to merge 3 commits into
aidandaly24 wants to merge 3 commits into
Conversation
agentcore-cli-automation
approved these changes
May 7, 2026
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Reviewed the diff end-to-end against the codebase. This is a small, focused fix that addresses the stated bug:
- The
requireTTY()call is added in exactly the right place — the no---source/ no-subcommand branch ofagentcore import, which is the only branch that callsrender(<ImportFlow>).ImportFlowincludesArnInputScreenwhich usesTextInput, so Ink's raw mode is needed and the guard is warranted. - Ordering (
requireProject()thenrequireTTY()) matches the established pattern inadd,remove,deploy, andinvoke.requireProject()'srender(<FatalError>)path is a static component with nouseInput, so running it before the TTY check is safe in non-TTY environments. - The YAML-path / subcommand branches (
--source ...,import runtime,import memory, etc.) are correctly untouched — they don't launch the TUI.
Tests look good:
tty.test.tscovers all four TTY permutations (stdin-only, stdout-only, both, neither) and asserts exit code 1 + the user-facing message. UsingObject.definePropertywithconfigurable: trueto restoreisTTYis correct given Node's TTY descriptors.- The regression test in
command.test.tsexercises the real bug scenario viarunCLI(which spawns withstdio: ['ignore', 'pipe', 'pipe']— non-TTY stdin and stdout, matching the original repro) and asserts both the positive signal (interactive-terminal message, exit 1) and the negative signal (no "Raw mode is not supported" text). The minimalagentcore/agentcore.jsonis sufficient to satisfyfindConfigRoot/requireProject.
No serious issues. LGTM.
Contributor
Coverage Report
|
added 3 commits
May 7, 2026 19:54
- 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.
- 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.
| import { requireTTY } from '../tty.js'; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| describe('requireTTY', () => { |
Contributor
There was a problem hiding this comment.
what does this verify that the integ tests don't cover?
| // 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(); |
Contributor
There was a problem hiding this comment.
if I understand correctly, we need this logic before all commands. Is there a way we can bake this into a shared middleware to avoid regressions or missing it in other places?
| import { afterAll, beforeAll, describe, expect, it } from 'vitest'; | ||
| /** | ||
| * Regression test for issue #982 (the command named in the bug title): |
Contributor
There was a problem hiding this comment.
nit: the comments feel verbose and unnecessary.
aidandaly24
commented
Jun 25, 2026
ContributorAuthor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix issue #982: non-TTY invocations of TUI-based commands could crash with the Ink "Raw mode is not supported on the current process.stdin" error and silently exit with code 0, hiding the failure from CI/CD pipelines and scripted usage.
Root cause
The
requireTTY()guard insrc/cli/tui/guards/tty.tsalready exists and is applied indeploy,create,dev,add,remove, andinvoke. However, theimportcommand's interactive TUI branch (agentcore importwith no--sourceand no subcommand) calledrender(<ImportFlow>)without first callingrequireTTY(). Verified reproducer:Changes
src/cli/commands/import/command.ts— AddrequireTTY()(called beforerequireProject(), so the non-TTY exit path never goes through any Ink render) in the no-source branch. Hoisted to a static barrel import (import { requireProject, requireTTY } from '../../tui/guards') for consistency with the other TUI commands.src/cli/tui/guards/__tests__/tty.test.ts(new) — Unit tests locking in the exit-1 + clear-message contract forrequireTTY()across the four TTY combinations.src/cli/commands/import/__tests__/command.test.ts(new) — Integration test runningagentcore importviarunCLI(non-TTY stdin/stdout) asserting exit 1 with the interactive-terminal message and no Ink raw-mode text. Also pins guard placement:import --source <bogus>must take the source-file-validation path, not the TTY guard.src/cli/commands/deploy/__tests__/deploy-non-tty.test.ts(new) — Regression test for the command named in the bug title:agentcore deploy(no flags) andagentcore deploy --diffin non-TTY exit 1 cleanly without surfacing the Ink raw-mode error.Out of scope
deploy,create,dev,add,remove,invokealready haverequireTTY()(since PR fix: add TTY detection before TUI fallbacks to prevent agent/CI hangs #949) — verified working under the reproducer; no behavior change beyond adding the missing test coverage above.render(<Text>)usages instatus,fetch,package,traces,config-bundledon't use Ink raw mode and don't trigger the bug.uncaughtExceptionhandler insrc/cli/index.tsalready exits 1; no change needed.Related Issue
Closes#982
Documentation PR
N/A — bug fix, no user-facing surface area changes.
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integ(targeted: 8/8 tests acrosstty.test.ts,import/__tests__/command.test.ts,deploy/__tests__/deploy-non-tty.test.tspass; full unit suite green in CI)npm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshots (N/A — no asset changes)Manual verification of the reproducer:
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.