Skip to content

Tiny Fix: Handle the non-tty case for sysconf#1356

Open
jeff-hykin wants to merge 2 commits intodevfrom
jeff/fix/sysconf
Open

Tiny Fix: Handle the non-tty case for sysconf#1356
jeff-hykin wants to merge 2 commits intodevfrom
jeff/fix/sysconf

Conversation

@jeff-hykin
Copy link
Member

Problem

Can break inside docker and other automated envs

Solution

5 line change to check if stdin is tty

Breaking Changes

None

How to Test

Reboot or change multicast, then run any dimos blueprint with lcm

Contributor License Agreement

  • I have read and approved the CLA.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 24, 2026

Greptile Summary

Adds sys.stdin.isatty() check to handle non-interactive environments in the configure_system function. When stdin is not a TTY (e.g., in Docker containers or CI/CD pipelines), the function now automatically assumes "yes" to apply system configuration changes instead of attempting to prompt for user input, which would fail with EOFError.

  • Imports sys module to access stdin.isatty()
  • Wraps existing input() prompt in TTY check
  • Prints informative message when auto-accepting in non-interactive mode
  • Maintains existing error handling for EOFError/KeyboardInterrupt in interactive mode

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • Simple, well-scoped change that solves a real problem. Uses standard Python sys.stdin.isatty() for TTY detection, which is the idiomatic approach. Follows existing code patterns and error handling. The auto-acceptance behavior in non-interactive environments is appropriate since the function already checks for CI environments earlier, and critical checks will still cause SystemExit if they fail.
  • No files require special attention

Important Files Changed

Filename Overview
dimos/protocol/service/system_configurator.py Adds TTY detection to automatically approve system changes in non-interactive environments like Docker

Last reviewed commit: e0d2258

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

paul-nechifor
paul-nechifor previously approved these changes Feb 24, 2026
Comment on lines 191 to +198
def test_handles_eof_error_on_input(self) -> None:
with patch.dict(os.environ, {"CI": ""}, clear=False):
mock_check = MockConfigurator(passes=False)
with patch("builtins.input", side_effect=EOFError):
configure_system([mock_check])
assert not mock_check.fix_called
with patch("sys.stdin") as mock_stdin:
mock_stdin.isatty.return_value = True
with patch("builtins.input", side_effect=EOFError):
configure_system([mock_check])
assert not mock_check.fix_called
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the mocker fixture to not need to indent so much. It automatically cleans up when it's done.

Suggested change
def test_handles_eof_error_on_input(self) -> None:
with patch.dict(os.environ, {"CI": ""}, clear=False):
mock_check = MockConfigurator(passes=False)
with patch("builtins.input", side_effect=EOFError):
configure_system([mock_check])
assert not mock_check.fix_called
with patch("sys.stdin") as mock_stdin:
mock_stdin.isatty.return_value = True
with patch("builtins.input", side_effect=EOFError):
configure_system([mock_check])
assert not mock_check.fix_called
def test_handles_eof_error_on_input(self, mocker) -> None:
mocker.patch.dict(os.environ, {"CI": ""}, clear=False)
mock_check = MockConfigurator(passes=False)
mock_stdin = mocker.patch("sys.stdin")
mock_stdin.isatty.return_value = True
mocker.patch("builtins.input", side_effect=EOFError)
configure_system([mock_check])
assert not mock_check.fix_called

Also, all of them use mocker.patch.dict(os.environ, {"CI": ""}, clear=False) so you can put that in a common mock so you don't have to add it in each test.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants