fix(install): widen /dev/tty open-probe to sibling gates (#16746)
The contributor's PR (#16750) scoped the fix to run_setup_wizard() and explicitly punted the two sibling sites. Both have the identical [ -e /dev/tty ] pattern followed by a < /dev/tty redirect and crash in Docker the same way: - scripts/install.sh:732 install_system_packages() -- apt sudo prompt fallback. sudo ... < /dev/tty dies with the same ENXIO. - scripts/install.sh:1395 maybe_start_gateway() -- gateway-install gate, same function path as the wizard reproducer. Fix both with the same (: </dev/tty) 2>/dev/null probe, and parametrize the regression test over all three gated functions so any future regression is caught regardless of which site breaks.
This commit is contained in:
@@ -729,9 +729,12 @@ install_system_packages() {
|
|||||||
return 0
|
return 0
|
||||||
fi
|
fi
|
||||||
fi
|
fi
|
||||||
elif [ -e /dev/tty ]; then
|
elif (: </dev/tty) 2>/dev/null; then
|
||||||
# Non-interactive (e.g. curl | bash) but a terminal is available.
|
# Non-interactive (e.g. curl | bash) but a terminal is available.
|
||||||
# Read the prompt from /dev/tty (same approach the setup wizard uses).
|
# Read the prompt from /dev/tty (same approach the setup wizard uses).
|
||||||
|
# Probe by actually opening /dev/tty: a bare existence test passes
|
||||||
|
# in Docker builds where the device node is in the mount namespace
|
||||||
|
# but opening fails with ENXIO. See #16746.
|
||||||
echo ""
|
echo ""
|
||||||
log_info "sudo is needed ONLY to install optional system packages (${pkgs[*]}) via your package manager."
|
log_info "sudo is needed ONLY to install optional system packages (${pkgs[*]}) via your package manager."
|
||||||
log_info "Hermes Agent itself does not require or retain root access."
|
log_info "Hermes Agent itself does not require or retain root access."
|
||||||
@@ -1397,7 +1400,10 @@ maybe_start_gateway() {
|
|||||||
fi
|
fi
|
||||||
fi
|
fi
|
||||||
|
|
||||||
if ! [ -e /dev/tty ]; then
|
# Probe by actually opening /dev/tty: a bare existence test passes
|
||||||
|
# in Docker builds where the device node is in the mount namespace
|
||||||
|
# but opening fails with ENXIO. See #16746.
|
||||||
|
if ! (: </dev/tty) 2>/dev/null; then
|
||||||
log_info "Gateway setup skipped (no terminal available). Run 'hermes gateway install' later."
|
log_info "Gateway setup skipped (no terminal available). Run 'hermes gateway install' later."
|
||||||
return 0
|
return 0
|
||||||
fi
|
fi
|
||||||
|
|||||||
@@ -1,11 +1,16 @@
|
|||||||
"""Regression for #16746: setup-wizard tty gate must actually open /dev/tty.
|
"""Regression for #16746: install.sh /dev/tty gates must actually open /dev/tty.
|
||||||
|
|
||||||
In a Docker build, ``/dev/tty`` exists as a device node (so a bare ``-e``
|
In a Docker build, ``/dev/tty`` exists as a device node (so a bare ``-e``
|
||||||
existence test returns true) but opening it fails with ``ENXIO: No such
|
existence test returns true) but opening it fails with ``ENXIO: No such
|
||||||
device or address``. Under the old gate the wizard proceeded past the "no
|
device or address``. Under the old gates the script proceeded past the "no
|
||||||
terminal available" skip and then crashed on the ``< /dev/tty`` redirect a
|
terminal available" skip and then crashed on the ``< /dev/tty`` redirect a
|
||||||
few lines later, aborting the entire image build. The fix replaces the
|
few lines later, aborting the entire image build. The fix replaces every
|
||||||
existence check with an open-based probe so the skip kicks in correctly.
|
existence-based check that guards a subsequent ``< /dev/tty`` redirect with
|
||||||
|
an open-based probe so the skip kicks in correctly.
|
||||||
|
|
||||||
|
This module covers all three affected functions: ``run_setup_wizard()``
|
||||||
|
(the reproducer in #16746), ``install_system_packages()`` (the apt sudo
|
||||||
|
prompt fallback), and ``maybe_start_gateway()`` (the gateway-install gate).
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
@@ -13,29 +18,36 @@ from __future__ import annotations
|
|||||||
import re
|
import re
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
REPO_ROOT = Path(__file__).resolve().parent.parent
|
REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||||
INSTALL_SH = REPO_ROOT / "scripts" / "install.sh"
|
INSTALL_SH = REPO_ROOT / "scripts" / "install.sh"
|
||||||
|
|
||||||
|
# Every function in scripts/install.sh that previously gated on a bare
|
||||||
|
# ``[ -e /dev/tty ]`` check before redirecting stdin from ``/dev/tty``.
|
||||||
|
GATED_FUNCTIONS = ("run_setup_wizard", "install_system_packages", "maybe_start_gateway")
|
||||||
|
|
||||||
def _extract_run_setup_wizard() -> str:
|
|
||||||
"""Return the body of ``run_setup_wizard()`` as a single string.
|
|
||||||
|
|
||||||
Anchored to ``run_setup_wizard()`` and a top-of-line ``}`` so the helper
|
def _extract_function_body(name: str) -> str:
|
||||||
keeps working if neighbouring functions are renamed.
|
"""Return the body of ``<name>()`` as a single string.
|
||||||
|
|
||||||
|
Anchored to ``<name>()`` and a top-of-line ``}`` so the helper keeps
|
||||||
|
working if neighbouring functions are renamed.
|
||||||
"""
|
"""
|
||||||
text = INSTALL_SH.read_text()
|
text = INSTALL_SH.read_text()
|
||||||
match = re.search(
|
match = re.search(
|
||||||
r"^run_setup_wizard\(\)\s*\{\s*\n(?P<body>.*?)^\}",
|
rf"^{re.escape(name)}\(\)\s*\{{\s*\n(?P<body>.*?)^\}}",
|
||||||
text,
|
text,
|
||||||
re.MULTILINE | re.DOTALL,
|
re.MULTILINE | re.DOTALL,
|
||||||
)
|
)
|
||||||
assert match is not None, "run_setup_wizard() not found in scripts/install.sh"
|
assert match is not None, f"{name}() not found in scripts/install.sh"
|
||||||
return match["body"]
|
return match["body"]
|
||||||
|
|
||||||
|
|
||||||
def test_run_setup_wizard_does_not_use_existence_only_tty_check() -> None:
|
@pytest.mark.parametrize("fn_name", GATED_FUNCTIONS)
|
||||||
|
def test_tty_gate_does_not_use_existence_only_check(fn_name: str) -> None:
|
||||||
"""The bare ``-e`` test is the bug — no spelling of it should remain."""
|
"""The bare ``-e`` test is the bug — no spelling of it should remain."""
|
||||||
body = _extract_run_setup_wizard()
|
body = _extract_function_body(fn_name)
|
||||||
# Cover ``[ -e /dev/tty ]``, ``[ -e "/dev/tty" ]``, ``test -e /dev/tty``
|
# Cover ``[ -e /dev/tty ]``, ``[ -e "/dev/tty" ]``, ``test -e /dev/tty``
|
||||||
# and friends, with arbitrary surrounding whitespace.
|
# and friends, with arbitrary surrounding whitespace.
|
||||||
pattern = re.compile(
|
pattern = re.compile(
|
||||||
@@ -48,28 +60,32 @@ def test_run_setup_wizard_does_not_use_existence_only_tty_check() -> None:
|
|||||||
)
|
)
|
||||||
match = pattern.search(body)
|
match = pattern.search(body)
|
||||||
assert match is None, (
|
assert match is None, (
|
||||||
"run_setup_wizard contains an existence-only check on /dev/tty "
|
f"{fn_name} contains an existence-only check on /dev/tty "
|
||||||
f"({match.group(0)!r}). Bare `-e` tests pass in Docker builds "
|
f"({match.group(0)!r}). Bare `-e` tests pass in Docker builds "
|
||||||
"where the device node is in the mount namespace but cannot be "
|
"where the device node is in the mount namespace but cannot be "
|
||||||
"opened (ENXIO). Use an open-based probe (e.g. "
|
"opened (ENXIO). Use an open-based probe (e.g. "
|
||||||
"`(: </dev/tty) 2>/dev/null` or `exec 3</dev/tty`) so the skip "
|
"`(: </dev/tty) 2>/dev/null` or `exec 3</dev/tty`) so the skip "
|
||||||
"kicks in before the wizard tries to read from /dev/tty. "
|
"kicks in before the function tries to read from /dev/tty. "
|
||||||
"See #16746."
|
"See #16746."
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_run_setup_wizard_gates_on_open_based_tty_probe() -> None:
|
@pytest.mark.parametrize("fn_name", GATED_FUNCTIONS)
|
||||||
|
def test_tty_gate_uses_open_based_probe(fn_name: str) -> None:
|
||||||
"""The gate must actually attempt to open ``/dev/tty``.
|
"""The gate must actually attempt to open ``/dev/tty``.
|
||||||
|
|
||||||
Any ``if !`` (or ``if``) whose condition opens ``/dev/tty`` for input
|
Any ``if``/``if !``/``elif`` whose condition opens ``/dev/tty`` for
|
||||||
counts: ``(: </dev/tty)``, ``exec 3</dev/tty``, ``{ exec 3</dev/tty; }``,
|
input counts: ``(: </dev/tty)``, ``exec 3</dev/tty``,
|
||||||
etc. Asserting the higher-level invariant rather than a specific spelling
|
``{ exec 3</dev/tty; }``, etc. Asserting the higher-level invariant
|
||||||
so equivalent refactors stay green.
|
rather than a specific spelling so equivalent refactors stay green.
|
||||||
"""
|
"""
|
||||||
body = _extract_run_setup_wizard()
|
body = _extract_function_body(fn_name)
|
||||||
gate = re.compile(r"^\s*if\s+!?\s+[^\n]*<\s*/dev/tty[^\n]*;\s*then", re.MULTILINE)
|
gate = re.compile(
|
||||||
assert gate.search(body), (
|
r"^\s*(?:if|elif)\s+!?\s*[^\n]*<\s*/dev/tty[^\n]*;\s*then",
|
||||||
"run_setup_wizard must gate on an open-based probe of /dev/tty "
|
re.MULTILINE,
|
||||||
"(an `if`/`if !` whose test redirects stdin from /dev/tty), not a "
|
)
|
||||||
"mere existence check. See #16746."
|
assert gate.search(body), (
|
||||||
|
f"{fn_name} must gate on an open-based probe of /dev/tty "
|
||||||
|
"(an `if`/`if !`/`elif` whose test redirects stdin from /dev/tty), "
|
||||||
|
"not a mere existence check. See #16746."
|
||||||
)
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user