From 89e8c87354524d4b9a4bc60cf150aa50c5679684 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Mon, 27 Apr 2026 18:18:00 -0700 Subject: [PATCH] test(install): regex-based gate assertions per copilot review on #16750 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the three Copilot inline findings on the regression test: - Switch _extract_run_setup_wizard() from str.index() with hard-coded markers (which raises ValueError if `maybe_start_gateway()` is renamed or the marker leaks into a comment) to an anchored regex on the function-definition + closing-brace boundaries. - Match `[ -e /dev/tty ]` with surrounding whitespace, optional quoting, and the `test -e /dev/tty` form so the regression guard catches every spelling of the existence-only check, not just the exact substring. - Replace the literal `(: --- .../test_install_sh_setup_wizard_tty_probe.py | 76 ++++++++++++++----- 1 file changed, 55 insertions(+), 21 deletions(-) diff --git a/tests/test_install_sh_setup_wizard_tty_probe.py b/tests/test_install_sh_setup_wizard_tty_probe.py index dbe705380..0f2a15fc5 100644 --- a/tests/test_install_sh_setup_wizard_tty_probe.py +++ b/tests/test_install_sh_setup_wizard_tty_probe.py @@ -1,13 +1,16 @@ """Regression for #16746: setup-wizard tty gate must actually open /dev/tty. -In a Docker build, ``/dev/tty`` exists as a device node (so ``[ -e /dev/tty ]`` -returns true) but opening it fails with ``ENXIO: No such device or address``. -Under the old gate the wizard proceeded past the "no terminal available" skip -and then crashed on the ``< /dev/tty`` redirect a few lines later, aborting -the entire image build. The fix replaces the bare existence check with an -open-based probe so the skip kicks in correctly. +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 +device or address``. Under the old gate the wizard proceeded past the "no +terminal available" skip and then crashed on the ``< /dev/tty`` redirect a +few lines later, aborting the entire image build. The fix replaces the +existence check with an open-based probe so the skip kicks in correctly. """ +from __future__ import annotations + +import re from pathlib import Path REPO_ROOT = Path(__file__).resolve().parent.parent @@ -15,27 +18,58 @@ INSTALL_SH = REPO_ROOT / "scripts" / "install.sh" def _extract_run_setup_wizard() -> str: - """Return the body of run_setup_wizard() as a single string.""" + """Return the body of ``run_setup_wizard()`` as a single string. + + Anchored to ``run_setup_wizard()`` and a top-of-line ``}`` so the helper + keeps working if neighbouring functions are renamed. + """ text = INSTALL_SH.read_text() - start = text.index("run_setup_wizard()") - # The next top-level function follows immediately; use it as the end marker. - end = text.index("\nmaybe_start_gateway()", start) - return text[start:end] + match = re.search( + r"^run_setup_wizard\(\)\s*\{\s*\n(?P.*?)^\}", + text, + re.MULTILINE | re.DOTALL, + ) + assert match is not None, "run_setup_wizard() not found in scripts/install.sh" + return match["body"] -def test_run_setup_wizard_does_not_use_bare_existence_check() -> None: +def test_run_setup_wizard_does_not_use_existence_only_tty_check() -> None: + """The bare ``-e`` test is the bug — no spelling of it should remain.""" body = _extract_run_setup_wizard() - assert "[ -e /dev/tty ]" not in body, ( - "run_setup_wizard guards on `[ -e /dev/tty ]`, which is true in Docker " - "builds where the device node exists but cannot be opened (ENXIO). " - "Use an open-based probe such as `(: /dev/null` so the " - "skip kicks in before the wizard tries to read from /dev/tty. See #16746." + # Cover ``[ -e /dev/tty ]``, ``[ -e "/dev/tty" ]``, ``test -e /dev/tty`` + # and friends, with arbitrary surrounding whitespace. + pattern = re.compile( + r"""( + \[\s*-e\s+["']?/dev/tty["']?\s*\] + | + \btest\s+-e\s+["']?/dev/tty["']? + )""", + re.VERBOSE, + ) + match = pattern.search(body) + assert match is None, ( + "run_setup_wizard contains an existence-only check on /dev/tty " + f"({match.group(0)!r}). Bare `-e` tests pass in Docker builds " + "where the device node is in the mount namespace but cannot be " + "opened (ENXIO). Use an open-based probe (e.g. " + "`(: /dev/null` or `exec 3 None: +def test_run_setup_wizard_gates_on_open_based_tty_probe() -> None: + """The gate must actually attempt to open ``/dev/tty``. + + Any ``if !`` (or ``if``) whose condition opens ``/dev/tty`` for input + counts: ``(: