refactor: fail loud, tighten types, remove speculative abstraction
Fail loud at the boundary:
- substitute_template raises ConfigError on unresolved {{...}}; no more
silent literal placeholders in download URLs.
- parse_profile raises ConfigError when 'os' is missing -- no
raw.get("os", "linux") default that silently masks typos.
- urllib download failures wrapped to FlowError.
- bootstrap _execute_action dispatches phases explicitly and raises
on unhandled phase; no more "anything else runs as shell".
Direct access over defensive wrapping:
- plan_bootstrap requires env; plan_install requires pm. Drop the
dead `or os.environ` / `or detect_package_manager()` fallbacks.
- InstalledState.from_dict raises ConfigError on missing fields
rather than .get(..., default).
- Replace `x or {}` chains with explicit `x if x is not None else {}`
in package resolution; catalog validates type/platform-map/install
shapes at parse.
One canonical form / direct access:
- Path.home() replaced with paths.HOME in services/packages.py and
commands/completion.py. paths.HOME is the single source now.
- Use Path.is_relative_to for install-path containment instead of
str.startswith.
Domain purity:
- domain/containers/resolution.resolve_mounts takes a filesystem_check
predicate; service passes the probe in. Domain no longer touches
the filesystem directly.
No speculative abstraction:
- Drop the `allow_sudo` field entirely. The _script_uses_sudo check
it gated was bypassable (substring match) and gave false confidence;
the manifest is fully user-trusted anyway.
- Delete dead terminfo_fix_command + RemoteService.fix_terminfo
(no command surface exposes them).
- FileSystem.remove_tree no longer swallows errors via ignore_errors;
callers opt into missing_ok if needed.
Typed enums:
- PackageDef.type, AppConfig.container_runtime as Literal[...].
container_runtime values validated at config parse.
Completion bypasses runtime no longer:
- complete(ctx, ...) threads context; ContainerRuntime and state-file
reads go through ctx.runtime instead of constructing primitives.
Tests added for: template raise, missing os raise, env/pm required,
unknown phase raise, no allow_sudo gate, URL download failure, install
path escape, corrupt installed.json, container_runtime Literal,
filesystem_check controls mounts.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -2,6 +2,7 @@
|
||||
|
||||
import io
|
||||
import tarfile
|
||||
import urllib.error
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
@@ -12,7 +13,7 @@ from flow.core.errors import FlowError
|
||||
from flow.core.platform import PlatformInfo
|
||||
from flow.core.runtime import SystemRuntime
|
||||
from flow.core import paths
|
||||
from flow.domain.packages.models import InstalledPackage, InstalledState
|
||||
from flow.domain.packages.models import InstalledPackage, InstalledState, PackageDef
|
||||
from flow.services.packages import PackageService
|
||||
|
||||
|
||||
@@ -78,6 +79,7 @@ class TestPackageService:
|
||||
home = tmp_path / "home"
|
||||
home.mkdir()
|
||||
monkeypatch.setenv("HOME", str(home))
|
||||
monkeypatch.setattr(paths, "HOME", home)
|
||||
monkeypatch.setattr(paths, "DATA_DIR", tmp_path / "data")
|
||||
monkeypatch.setattr(paths, "INSTALLED_STATE", tmp_path / "installed.json")
|
||||
|
||||
@@ -130,3 +132,105 @@ class TestPackageService:
|
||||
assert (home / ".local" / "bin" / "nvim").exists()
|
||||
assert (home / ".local" / "share" / "nvim" / "runtime.txt").exists()
|
||||
assert (home / ".local" / "share" / "man" / "man1" / "nvim.1").exists()
|
||||
|
||||
def test_post_install_with_sudo_runs_unchecked(self, tmp_path, monkeypatch):
|
||||
"""No allow_sudo gate -- post-install scripts run as written."""
|
||||
home = tmp_path / "home"
|
||||
home.mkdir()
|
||||
monkeypatch.setattr(paths, "HOME", home)
|
||||
monkeypatch.setattr(paths, "INSTALLED_STATE", tmp_path / "installed.json")
|
||||
|
||||
calls: list[str] = []
|
||||
|
||||
class _Runner:
|
||||
def run_shell(self, command, **kwargs):
|
||||
calls.append(command)
|
||||
|
||||
class _Result:
|
||||
returncode = 0
|
||||
stdout = ""
|
||||
stderr = ""
|
||||
|
||||
return _Result()
|
||||
|
||||
ctx = _make_ctx(tmp_path)
|
||||
ctx.runtime.runner = _Runner()
|
||||
svc = PackageService(ctx)
|
||||
pkg = PackageDef(
|
||||
name="docker", type="pkg", sources={},
|
||||
source=None, version=None, asset_pattern=None,
|
||||
platform_map={}, extract_dir=None, install={},
|
||||
post_install="sudo groupadd docker || true",
|
||||
)
|
||||
svc._run_post_install(pkg)
|
||||
assert calls == ["sudo groupadd docker || true"]
|
||||
|
||||
def test_install_binary_url_failure_raises_flow_error(self, tmp_path, monkeypatch):
|
||||
home = tmp_path / "home"
|
||||
home.mkdir()
|
||||
monkeypatch.setattr(paths, "HOME", home)
|
||||
monkeypatch.setattr(paths, "DATA_DIR", tmp_path / "data")
|
||||
monkeypatch.setattr(paths, "INSTALLED_STATE", tmp_path / "installed.json")
|
||||
|
||||
def _raise(*args, **kwargs):
|
||||
raise urllib.error.URLError("Network unreachable")
|
||||
|
||||
monkeypatch.setattr(
|
||||
"flow.services.packages.urllib.request.urlopen", _raise,
|
||||
)
|
||||
|
||||
manifest = {
|
||||
"packages": [{
|
||||
"name": "neovim",
|
||||
"type": "binary",
|
||||
"source": "github:neovim/neovim",
|
||||
"version": "0.10.4",
|
||||
"platform-map": {"linux-x64": "nvim-linux-x64.tar.gz"},
|
||||
"install": {"bin": ["bin/nvim"]},
|
||||
}],
|
||||
}
|
||||
ctx = _make_ctx(tmp_path, manifest)
|
||||
svc = PackageService(ctx)
|
||||
packages = svc.resolve_install_packages(package_names=["neovim"])
|
||||
with pytest.raises(FlowError, match="Failed to download"):
|
||||
svc.install(packages)
|
||||
|
||||
def test_install_path_absolute_raises(self, tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(paths, "HOME", tmp_path / "home")
|
||||
ctx = _make_ctx(tmp_path)
|
||||
svc = PackageService(ctx)
|
||||
with pytest.raises(FlowError, match="must be relative"):
|
||||
svc._validate_install_path("pkg", Path("/etc/passwd"))
|
||||
|
||||
def test_install_path_parent_traversal_raises(self, tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(paths, "HOME", tmp_path / "home")
|
||||
ctx = _make_ctx(tmp_path)
|
||||
svc = PackageService(ctx)
|
||||
with pytest.raises(FlowError, match="parent traversal"):
|
||||
svc._validate_install_path("pkg", Path("../etc/passwd"))
|
||||
|
||||
def test_install_path_escapes_extract_dir_raises(self, tmp_path, monkeypatch):
|
||||
"""A relative path whose resolved location is outside the extract dir."""
|
||||
home = tmp_path / "home"
|
||||
home.mkdir()
|
||||
monkeypatch.setattr(paths, "HOME", home)
|
||||
ctx = _make_ctx(tmp_path)
|
||||
svc = PackageService(ctx)
|
||||
|
||||
extract_root = tmp_path / "extract"
|
||||
extract_root.mkdir()
|
||||
sibling = tmp_path / "sibling"
|
||||
sibling.mkdir()
|
||||
# Symlink inside the extract root pointing outside -- the resolved
|
||||
# source escapes the root.
|
||||
link = extract_root / "evil"
|
||||
link.symlink_to(sibling)
|
||||
|
||||
with pytest.raises(FlowError, match="escapes extract-dir"):
|
||||
svc._copy_install_item(
|
||||
"pkg",
|
||||
extract_root,
|
||||
extract_root.resolve(),
|
||||
"bin",
|
||||
"evil/escape",
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user