Skip to content

test(tui): isolate terminalSetup tests from SSH env - #18929

Closed
sauerdaniel wants to merge 0 commit into
NousResearch:mainfrom
sauerdaniel:fix/tui-terminalSetup-test-isolation
Closed

test(tui): isolate terminalSetup tests from SSH env#18929
sauerdaniel wants to merge 0 commit into
NousResearch:mainfrom
sauerdaniel:fix/tui-terminalSetup-test-isolation

Conversation

@sauerdaniel

@sauerdaniel sauerdaniel commented May 2, 2026

Copy link
Copy Markdown

What does this PR do?

Current main now isolates the terminalSetup.test.ts keybinding tests from ambient SSH environment variables with the same fix this PR proposed. The production SSH guard remains covered explicitly, while non-SSH keybinding tests no longer inherit SSH_CONNECTION, SSH_TTY, or SSH_CLIENT from the developer or CI shell. Stubbed environment variables are restored after every test so this suite cannot leak state into later tests in the same Vitest worker.

Related Issue

Fixes #18939

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

How to Test

From ui-tui/:

  1. npm test -- src/__tests__/terminalSetup.test.ts
  2. Repeat the command with non-empty SSH_CONNECTION, SSH_TTY, and SSH_CLIENT values in the process environment.
  3. npm run typecheck
  4. npm run build

From the repository root:

Latest local results after rebasing onto current main:

  • Focused Vitest run on current main: 20/20 passed.
  • Focused Vitest run with non-empty SSH_CONNECTION, SSH_TTY, and SSH_CLIENT: 20/20 passed; TypeScript typecheck passed.
  • Full TUI Vitest suite after npm run build:ink, with non-empty SSH_CONNECTION, SSH_TTY, and SSH_CLIENT: 107/107 files passed; 1,125 tests passed and 1 skipped.
  • TUI TypeScript typecheck: passed.
  • TUI production build: passed.
  • Full TUI ESLint: passed.

Tested on Fedora Linux.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • Python suite — N/A: this PR changes one Vitest file; the full TUI suite and focused SSH-environment regression pass.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Fedora Linux

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — test-only environment isolation; no runtime change.
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

npm test -- src/__tests__/terminalSetup.test.ts
Test Files  1 passed (1)
Tests       20 passed (20)

npm run build:ink && npm test
Test Files  107 passed (107)
Tests       1125 passed | 1 skipped (1126)

npm run typecheck
> tsc --noEmit -p tsconfig.json

npm run build
built ui-tui/dist/entry.js

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have comp/tui Terminal UI (ui-tui/ + tui_gateway/) labels May 2, 2026
@sauerdaniel
sauerdaniel force-pushed the fix/tui-terminalSetup-test-isolation branch from f16ab58 to f30bcb2 Compare May 19, 2026 13:58

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating this environment-sensitive test path. The underlying premise remains valid on current main: configureTerminalKeybindings() falls back to process.env at ui-tui/src/lib/terminalSetup.ts:284 and short-circuits SSH sessions at :290-295; the affected tests omit options.env (for example ui-tui/src/__tests__/terminalSetup.test.ts:71-75).

Problems

  • The added vi.stubEnv calls are not restored. ui-tui/vitest.config.ts:3-7 does not enable unstubEnvs, whose installed Vitest default is false, so later tests in the same worker can inherit blank SSH variables. The established local pattern restores them in ui-tui/src/__tests__/theme.test.ts:37-40.

Suggested changes

  • Import afterEach and call vi.unstubAllEnvs() after each test in this suite.

This is an automated hermes-sweeper review.

Comment thread ui-tui/src/__tests__/terminalSetup.test.ts Outdated
@teknium1 teknium1 added sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 12, 2026
@sauerdaniel
sauerdaniel force-pushed the fix/tui-terminalSetup-test-isolation branch 4 times, most recently from 0b8a025 to f4c3e2b Compare July 16, 2026 11:09
@sauerdaniel

Copy link
Copy Markdown
Author

Rebased onto current main in f4c3e2b21. The afterEach(() => vi.unstubAllEnvs()) cleanup remains, and the branch still changes only terminalSetup.test.ts. Verification with non-empty SSH_CONNECTION, SSH_TTY, and SSH_CLIENT: full TUI Vitest passed 107 files / 1,125 tests with 1 skipped; build, typecheck, and ESLint passed.

@sauerdaniel
sauerdaniel force-pushed the fix/tui-terminalSetup-test-isolation branch from f4c3e2b to 9ecacd6 Compare July 22, 2026 08:29
@sauerdaniel

Copy link
Copy Markdown
Author

Closing record: the original branch commit was not merged directly, but merged PR #66373 independently landed the same fix on main: global beforeEach stubs for SSH_CONNECTION, SSH_TTY, and SSH_CLIENT, plus afterEach(() => vi.unstubAllEnvs()), while retaining explicit SSH-path coverage. The merged placement is slightly broader because it isolates the whole test file. No related change remains to salvage from this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: terminalSetup tests fail on SSH-connected machines due to env leakage

3 participants