fix(doctor): survive a PATH entry the user can't execute
The install itself now works end to end on a fresh macOS account — venv built,
package installed from git, entry point linked — and `doctor` then died with an
unhandled traceback:
PermissionError: [Errno 13] Permission denied: 'ip'
netpool's probe helpers caught only FileNotFoundError. That is not the only way
a probe binary can be unavailable: when a name on PATH exists but this user
cannot execute it, exec fails with EACCES, and CPython reports that in
preference to the ENOENT from the other PATH entries. So `except
FileNotFoundError` misses it and the crash propagates all the way out of
`doctor`. Reproduced directly: a mode-000 file named `ip` on PATH yields
exactly the error above.
_run_ok's docstring already stated the intent — treat an unavailable binary as
failure rather than crashing — so this widens the catch to OSError to match
what it says. Any OSError means the probe could not run, which for a
fail-closed check is indistinguishable from "not present". The same narrow
catch is fixed in overlapping_routes and in the two docker probes
(compose ls, docker ps), which are the same shape and equally reachable.
Deliberately not touched: the FileNotFoundError catches around file I/O in
bottle_state and orchestrator/service, where the narrow exception is correct.
The harness also required `bot-bottle` on PATH before running doctor, which
could never be true: install.sh prints the PATH line rather than editing a
shell profile, by design, so on a fresh account the entry point is installed
and working but not on PATH. It now looks where the installer actually puts
it (~/.local/bin, then the venv) and notes when it's running by absolute path.
That was the harness failing a run for a reason the installer intends.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WEfZZhakx13bxTfXcZCoS5
This commit is contained in:
@@ -74,10 +74,13 @@ def list_compose_projects(
|
|||||||
result = subprocess.run(
|
result = subprocess.run(
|
||||||
argv, capture_output=True, text=True, check=False,
|
argv, capture_output=True, text=True, check=False,
|
||||||
)
|
)
|
||||||
except FileNotFoundError as exc:
|
except OSError as exc:
|
||||||
|
# Not only "not found": docker on PATH but not executable by this
|
||||||
|
# user raises PermissionError. Either way the query never ran, so an
|
||||||
|
# enumeration caller must not read the empty result as authoritative.
|
||||||
if raise_on_error:
|
if raise_on_error:
|
||||||
raise EnumerationError(
|
raise EnumerationError(
|
||||||
"docker compose ls failed: docker not found"
|
f"docker compose ls failed: docker unavailable ({exc})"
|
||||||
) from exc
|
) from exc
|
||||||
return []
|
return []
|
||||||
if result.returncode != 0:
|
if result.returncode != 0:
|
||||||
|
|||||||
@@ -74,8 +74,9 @@ def _query_services_by_project() -> dict[str, set[str]]:
|
|||||||
],
|
],
|
||||||
capture_output=True, text=True, check=False,
|
capture_output=True, text=True, check=False,
|
||||||
)
|
)
|
||||||
except FileNotFoundError as exc:
|
except OSError as exc:
|
||||||
raise EnumerationError("docker ps failed: docker not found") from exc
|
# Missing, or on PATH but not executable by this user (PermissionError).
|
||||||
|
raise EnumerationError(f"docker ps failed: docker unavailable ({exc})") from exc
|
||||||
if r.returncode != 0:
|
if r.returncode != 0:
|
||||||
raise EnumerationError(f"docker ps failed: {r.stderr.strip()}")
|
raise EnumerationError(f"docker ps failed: {r.stderr.strip()}")
|
||||||
return _parse_services_by_project(r.stdout or "")
|
return _parse_services_by_project(r.stdout or "")
|
||||||
|
|||||||
@@ -177,14 +177,20 @@ def gw_slot() -> Slot:
|
|||||||
# --- fail-closed verification ---------------------------------------
|
# --- fail-closed verification ---------------------------------------
|
||||||
|
|
||||||
def _run_ok(argv: list[str]) -> bool:
|
def _run_ok(argv: list[str]) -> bool:
|
||||||
"""Run a probe command, treating a missing binary as failure
|
"""Run a probe command, treating an unavailable binary as failure
|
||||||
(rather than crashing) so callers can stay fail-closed."""
|
(rather than crashing) so callers can stay fail-closed."""
|
||||||
try:
|
try:
|
||||||
return subprocess.run(
|
return subprocess.run(
|
||||||
argv, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
|
argv, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
|
||||||
check=False,
|
check=False,
|
||||||
).returncode == 0
|
).returncode == 0
|
||||||
except FileNotFoundError:
|
except OSError:
|
||||||
|
# Not only "missing". A name on PATH that isn't executable by this
|
||||||
|
# user raises PermissionError, and CPython reports that EACCES in
|
||||||
|
# preference to the ENOENT from the other PATH entries — which is
|
||||||
|
# how `doctor` came to die with a traceback on a fresh macOS
|
||||||
|
# account. Any OSError means the probe couldn't run, which for a
|
||||||
|
# fail-closed check is indistinguishable from "not present".
|
||||||
return False
|
return False
|
||||||
|
|
||||||
|
|
||||||
@@ -244,7 +250,9 @@ def overlapping_routes() -> list[RouteConflict]:
|
|||||||
["ip", "-json", "route", "show", "table", "all"],
|
["ip", "-json", "route", "show", "table", "all"],
|
||||||
capture_output=True, text=True, check=False,
|
capture_output=True, text=True, check=False,
|
||||||
)
|
)
|
||||||
except FileNotFoundError:
|
except OSError:
|
||||||
|
# Missing, or present-but-not-executable for this user; either way
|
||||||
|
# there are no routes we can enumerate. See _run_ok.
|
||||||
return []
|
return []
|
||||||
if proc.returncode != 0 or not proc.stdout.strip():
|
if proc.returncode != 0 or not proc.stdout.strip():
|
||||||
return []
|
return []
|
||||||
|
|||||||
@@ -89,14 +89,28 @@ user_exists() { dscl . -read "/Users/$USER_NAME" >/dev/null 2>&1; }
|
|||||||
# Run a shell snippet as the throwaway user in a fresh login shell.
|
# Run a shell snippet as the throwaway user in a fresh login shell.
|
||||||
run_as_user() { sudo -u "$USER_NAME" -i sh -c "$1"; }
|
run_as_user() { sudo -u "$USER_NAME" -i sh -c "$1"; }
|
||||||
|
|
||||||
# `bot-bottle doctor` as the throwaway user. Non-zero when the entry point
|
# `bot-bottle doctor` as the throwaway user. Non-zero when no entry point was
|
||||||
# never made it onto that user's PATH, or when doctor itself is unhappy.
|
# installed at all, or when doctor itself is unhappy.
|
||||||
|
#
|
||||||
|
# Deliberately does NOT require `bot-bottle` on PATH: install.sh prints the
|
||||||
|
# PATH line rather than editing a shell profile, so on a fresh account the
|
||||||
|
# entry point is installed and working but not on PATH. Demanding PATH here
|
||||||
|
# would fail every run for a reason the installer intends.
|
||||||
doctor_as_user() {
|
doctor_as_user() {
|
||||||
if ! run_as_user 'command -v bot-bottle >/dev/null 2>&1'; then
|
# shellcheck disable=SC2016 # $HOME/$bb must expand in the *target* user's
|
||||||
echo " bot-bottle is not on PATH for $USER_NAME"
|
# shell, not in this one — that's the whole point of the single quotes.
|
||||||
return 1
|
run_as_user '
|
||||||
fi
|
for bb in "$HOME/.local/bin/bot-bottle" "$HOME/.bot-bottle/venv/bin/bot-bottle"; do
|
||||||
run_as_user 'bot-bottle doctor'
|
if [ -x "$bb" ]; then
|
||||||
|
command -v bot-bottle >/dev/null 2>&1 \
|
||||||
|
|| echo " (not on PATH — running $bb directly, as install.sh advises)"
|
||||||
|
exec "$bb" doctor
|
||||||
|
fi
|
||||||
|
done
|
||||||
|
command -v bot-bottle >/dev/null 2>&1 && exec bot-bottle doctor
|
||||||
|
echo " no bot-bottle entry point found for this user" >&2
|
||||||
|
exit 1
|
||||||
|
'
|
||||||
}
|
}
|
||||||
|
|
||||||
# --- commands --------------------------------------------------------
|
# --- commands --------------------------------------------------------
|
||||||
|
|||||||
@@ -44,6 +44,17 @@ class TestProjectNaming(unittest.TestCase):
|
|||||||
|
|
||||||
|
|
||||||
class TestComposeProjectListing(unittest.TestCase):
|
class TestComposeProjectListing(unittest.TestCase):
|
||||||
|
def test_compose_ls_empty_when_docker_unusable(self):
|
||||||
|
# Missing is the obvious case; present-but-not-executable raises
|
||||||
|
# PermissionError instead, which must not escape as a crash.
|
||||||
|
for exc in (FileNotFoundError, PermissionError(13, "Permission denied", "docker")):
|
||||||
|
with self.subTest(exc=type(exc).__name__):
|
||||||
|
with mock.patch(
|
||||||
|
"bot_bottle.backend.docker.compose.subprocess.run",
|
||||||
|
side_effect=exc,
|
||||||
|
):
|
||||||
|
self.assertEqual([], list_compose_projects())
|
||||||
|
|
||||||
def test_compose_ls_error_warns_by_default(self):
|
def test_compose_ls_error_warns_by_default(self):
|
||||||
with (
|
with (
|
||||||
mock.patch(
|
mock.patch(
|
||||||
|
|||||||
@@ -56,6 +56,21 @@ class TestNetpoolProbes(unittest.TestCase):
|
|||||||
with patch.object(netpool.subprocess, "run", side_effect=FileNotFoundError):
|
with patch.object(netpool.subprocess, "run", side_effect=FileNotFoundError):
|
||||||
self.assertFalse(netpool._run_ok(["nft"]))
|
self.assertFalse(netpool._run_ok(["nft"]))
|
||||||
|
|
||||||
|
def test_run_ok_false_on_unexecutable_binary(self):
|
||||||
|
# A name on PATH that this user can't execute raises PermissionError,
|
||||||
|
# not FileNotFoundError — CPython reports that EACCES in preference to
|
||||||
|
# the ENOENT from the other PATH entries. Catching only the latter made
|
||||||
|
# `doctor` die with a traceback on a fresh macOS account.
|
||||||
|
with patch.object(netpool.subprocess, "run",
|
||||||
|
side_effect=PermissionError(13, "Permission denied", "ip")):
|
||||||
|
self.assertFalse(netpool._run_ok(["ip", "link", "show", "bbfc0"]))
|
||||||
|
|
||||||
|
def test_overlapping_routes_empty_when_ip_unusable(self):
|
||||||
|
for exc in (FileNotFoundError, PermissionError(13, "Permission denied", "ip")):
|
||||||
|
with self.subTest(exc=type(exc).__name__), \
|
||||||
|
patch.object(netpool.subprocess, "run", side_effect=exc):
|
||||||
|
self.assertEqual([], netpool.overlapping_routes())
|
||||||
|
|
||||||
def test_tap_and_nft_probes(self):
|
def test_tap_and_nft_probes(self):
|
||||||
with patch.object(netpool, "_run_ok", return_value=True) as ok:
|
with patch.object(netpool, "_run_ok", return_value=True) as ok:
|
||||||
self.assertTrue(netpool.tap_present("bbfc0"))
|
self.assertTrue(netpool.tap_present("bbfc0"))
|
||||||
|
|||||||
Reference in New Issue
Block a user