From 78d4064853f75dd5ec61606c4c30d74e237e6a4e Mon Sep 17 00:00:00 2001 From: Tomas Mirchev Date: Mon, 16 Mar 2026 06:27:31 +0200 Subject: [PATCH] fix: address all code review issues 1. _merge_config: track explicit fields instead of comparing to defaults 2. plan_install: let asset resolution errors propagate (fail loudly) 3. _install_binary/_install_appimage: use argv lists instead of shell strings 4. _find_module: narrow exception to OSError/YAMLError, raise ConfigError 5. _install_binary: use pkg.extract_dir to scope binary search 6. plan_install: raise FlowError when pkg type needs PM but none found 7. Frozen dataclasses: change mutable list fields to tuples throughout 8. Remove dead stream_shell method and unused Console import 9. Guard os.getuid() with hasattr for cross-platform safety 10. _parse_targets: raise ConfigError on malformed entries 11. Bootstrap modules: use shlex.quote on all interpolated values Co-Authored-By: Claude Opus 4.6 (1M context) --- src/flow/cli.py | 24 ++++++++++++------- src/flow/core/config.py | 26 ++++++++++++++++---- src/flow/core/runtime.py | 32 ------------------------- src/flow/domain/bootstrap/models.py | 14 +++++------ src/flow/domain/bootstrap/modules.py | 19 ++++++++------- src/flow/domain/bootstrap/planning.py | 30 +++++++++++------------ src/flow/domain/dotfiles/models.py | 4 ++-- src/flow/domain/dotfiles/planning.py | 8 +++---- src/flow/domain/packages/models.py | 6 ++--- src/flow/domain/packages/planning.py | 24 ++++++++++--------- src/flow/services/dotfiles.py | 7 +++--- src/flow/services/packages.py | 16 ++++++------- tests/test_domain_bootstrap_planning.py | 2 +- 13 files changed, 105 insertions(+), 107 deletions(-) diff --git a/src/flow/cli.py b/src/flow/cli.py index 05bef91..2f7ef01 100644 --- a/src/flow/cli.py +++ b/src/flow/cli.py @@ -18,7 +18,7 @@ from flow.core.runtime import SystemRuntime def main(argv: Optional[list[str]] = None) -> None: """Main entry point.""" - if os.getuid() == 0: + if hasattr(os, "getuid") and os.getuid() == 0: print("Error: flow must not run as root", file=sys.stderr) sys.exit(1) @@ -72,15 +72,21 @@ def main(argv: Optional[list[str]] = None) -> None: def _merge_config(base: AppConfig, overlay: AppConfig) -> AppConfig: - """Merge two configs: overlay values override base when non-default.""" + """Merge two configs: overlay's explicitly-set fields override base.""" + + def _pick(field: str) -> str: + if field in overlay._explicit: + return getattr(overlay, field) + return getattr(base, field) + return AppConfig( - dotfiles_url=overlay.dotfiles_url or base.dotfiles_url, - dotfiles_branch=overlay.dotfiles_branch if overlay.dotfiles_branch != "main" else base.dotfiles_branch, - projects_dir=overlay.projects_dir if overlay.projects_dir != "~/projects" else base.projects_dir, - container_registry=overlay.container_registry if overlay.container_registry != "registry.tomastm.com" else base.container_registry, - container_tag=overlay.container_tag if overlay.container_tag != "latest" else base.container_tag, - tmux_session=overlay.tmux_session if overlay.tmux_session != "default" else base.tmux_session, - targets=overlay.targets or base.targets, + dotfiles_url=_pick("dotfiles_url"), + dotfiles_branch=_pick("dotfiles_branch"), + projects_dir=_pick("projects_dir"), + container_registry=_pick("container_registry"), + container_tag=_pick("container_tag"), + tmux_session=_pick("tmux_session"), + targets=overlay.targets if "targets" in overlay._explicit else base.targets, ) diff --git a/src/flow/core/config.py b/src/flow/core/config.py index 6e454c7..095314c 100644 --- a/src/flow/core/config.py +++ b/src/flow/core/config.py @@ -30,6 +30,8 @@ class AppConfig: container_tag: str = "latest" tmux_session: str = "default" targets: list[TargetConfig] = field(default_factory=list) + # Tracks which fields were explicitly set in config (not defaults) + _explicit: set[str] = field(default_factory=set, repr=False, compare=False) @dataclass @@ -58,16 +60,18 @@ def _load_yaml_file(path: Path) -> dict[str, Any]: def _parse_targets(raw: Any) -> list[TargetConfig]: + from flow.core.errors import ConfigError + if not isinstance(raw, dict): return [] targets: list[TargetConfig] = [] for key, value in raw.items(): if "@" not in key: - continue + raise ConfigError(f"Invalid target key '{key}': expected 'namespace@platform'") namespace, platform = key.split("@", 1) if not namespace or not platform: - continue + raise ConfigError(f"Invalid target key '{key}': both namespace and platform required") if isinstance(value, str): targets.append(TargetConfig( @@ -76,9 +80,9 @@ def _parse_targets(raw: Any) -> list[TargetConfig]: host=value, )) elif isinstance(value, dict): - host = value.get("host", "") + host = value.get("host") if not host: - continue + raise ConfigError(f"Target '{key}': 'host' is required") identity = value.get("identity") targets.append(TargetConfig( namespace=namespace, @@ -86,6 +90,8 @@ def _parse_targets(raw: Any) -> list[TargetConfig]: host=str(host), identity=str(identity) if identity is not None else None, )) + else: + raise ConfigError(f"Target '{key}': value must be a string or mapping") return targets @@ -99,35 +105,47 @@ def load_config(config_dir: Path) -> AppConfig: data = _load_yaml_file(config_file) cfg = AppConfig() + explicit: set[str] = set() repository = data.get("repository") if isinstance(repository, dict): url = repository.get("url") if url is not None: cfg.dotfiles_url = str(url) + explicit.add("dotfiles_url") branch = repository.get("branch") if branch is not None: cfg.dotfiles_branch = str(branch) + explicit.add("dotfiles_branch") paths_section = data.get("paths") if isinstance(paths_section, dict): projects = paths_section.get("projects") if projects is not None: cfg.projects_dir = str(projects) + explicit.add("projects_dir") defaults = data.get("defaults") if isinstance(defaults, dict): registry = defaults.get("container-registry") if registry is not None: cfg.container_registry = str(registry) + explicit.add("container_registry") + tag = defaults.get("container-tag") + if tag is not None: + cfg.container_tag = str(tag) + explicit.add("container_tag") tmux = defaults.get("tmux-session") if tmux is not None: cfg.tmux_session = str(tmux) + explicit.add("tmux_session") raw_targets = data.get("targets") if raw_targets is not None: cfg.targets = _parse_targets(raw_targets) + explicit.add("targets") + cfg._explicit = explicit return cfg diff --git a/src/flow/core/runtime.py b/src/flow/core/runtime.py index 77de6cd..adf6c02 100644 --- a/src/flow/core/runtime.py +++ b/src/flow/core/runtime.py @@ -9,7 +9,6 @@ from dataclasses import dataclass, field from pathlib import Path from typing import Any, Iterable, Mapping, Optional, Sequence -from flow.core.console import Console from flow.core.errors import FlowError @@ -70,37 +69,6 @@ class CommandRunner: raise FlowError(msg) return completed - def stream_shell( - self, - command: str, - console: Console, - *, - check: bool = True, - ) -> subprocess.CompletedProcess[str]: - process = subprocess.Popen( - command, - shell=True, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - text=True, - bufsize=1, - ) - lines: list[str] = [] - assert process.stdout is not None - try: - for line in process.stdout: - stripped = line.rstrip() - if stripped: - lines.append(stripped) - finally: - process.stdout.close() - process.wait() - - if check and process.returncode != 0: - raise FlowError(f"Command failed (exit {process.returncode}): {command}") - - return subprocess.CompletedProcess(command, process.returncode, stdout="\n".join(lines), stderr="") - def require_binary(self, name: str) -> str: path = shutil.which(name) if path is None: diff --git a/src/flow/domain/bootstrap/models.py b/src/flow/domain/bootstrap/models.py index d17b3f1..51ba086 100644 --- a/src/flow/domain/bootstrap/models.py +++ b/src/flow/domain/bootstrap/models.py @@ -13,10 +13,10 @@ class Profile: hostname: Optional[str] locale: Optional[str] shell: Optional[str] - ssh_keys: list[dict[str, str]] - runcmd: list[str] - packages: list[Any] # Raw entries, resolved later - env_required: list[str] + ssh_keys: tuple[dict[str, str], ...] + runcmd: tuple[str, ...] + packages: tuple[Any, ...] # Raw entries, resolved later + env_required: tuple[str, ...] @dataclass(frozen=True) @@ -24,7 +24,7 @@ class BootstrapAction: """A single action in a bootstrap plan.""" phase: str # "validate" | "setup" | "packages" | "shell" | "dotfiles" description: str - commands: list[str] + commands: tuple[str, ...] needs_sudo: bool = False def __str__(self) -> str: @@ -36,8 +36,8 @@ class BootstrapAction: class BootstrapPlan: """Complete bootstrap plan.""" profile: str - actions: list[BootstrapAction] - packages_to_install: list[Any] # PackageDef list + actions: tuple[BootstrapAction, ...] + packages_to_install: tuple[Any, ...] # PackageDef tuple @property def total_steps(self) -> int: diff --git a/src/flow/domain/bootstrap/modules.py b/src/flow/domain/bootstrap/modules.py index dd36052..13e6b60 100644 --- a/src/flow/domain/bootstrap/modules.py +++ b/src/flow/domain/bootstrap/modules.py @@ -1,5 +1,6 @@ """Bootstrap setup modules -- each produces shell commands.""" +import shlex from dataclasses import dataclass from typing import Optional @@ -21,9 +22,10 @@ class HostnameModule(SetupModule): return f"Set hostname to {self.hostname}" def plan(self) -> list[str]: + h = shlex.quote(self.hostname) return [ - f"sudo hostnamectl set-hostname {self.hostname}", - f"echo '127.0.1.1 {self.hostname}' | sudo tee -a /etc/hosts", + f"sudo hostnamectl set-hostname {h}", + f"echo '127.0.1.1 {h}' | sudo tee -a /etc/hosts", ] @@ -35,9 +37,10 @@ class LocaleModule(SetupModule): return f"Set locale to {self.locale}" def plan(self) -> list[str]: + loc = shlex.quote(self.locale) return [ - f"sudo locale-gen {self.locale}", - f"sudo update-locale LANG={self.locale}", + f"sudo locale-gen {loc}", + f"sudo update-locale LANG={loc}", ] @@ -49,7 +52,7 @@ class ShellModule(SetupModule): return f"Install and configure shell: {self.shell}" def plan(self) -> list[str]: - shell_path = f"/usr/bin/{self.shell}" + shell_path = shlex.quote(f"/usr/bin/{self.shell}") return [ f"sudo chsh -s {shell_path} $USER", ] @@ -65,12 +68,12 @@ class SSHKeygenModule(SetupModule): def plan(self) -> list[str]: commands: list[str] = [] for key in self.keys: - path = key.get("path", "~/.ssh/id_ed25519") - key_type = key.get("type", "ed25519") + path = shlex.quote(key.get("path", "~/.ssh/id_ed25519")) + key_type = shlex.quote(key.get("type", "ed25519")) comment = key.get("comment", "") cmd = f'ssh-keygen -t {key_type} -f {path} -N ""' if comment: - cmd += f' -C "{comment}"' + cmd += f" -C {shlex.quote(comment)}" commands.append(cmd) return commands diff --git a/src/flow/domain/bootstrap/planning.py b/src/flow/domain/bootstrap/planning.py index 8c99884..d4d6213 100644 --- a/src/flow/domain/bootstrap/planning.py +++ b/src/flow/domain/bootstrap/planning.py @@ -26,10 +26,10 @@ def parse_profile(name: str, raw: dict[str, Any]) -> Profile: hostname=raw.get("hostname"), locale=raw.get("locale"), shell=raw.get("shell"), - ssh_keys=raw.get("ssh-keys") or raw.get("ssh_keys") or [], - runcmd=raw.get("runcmd") or [], - packages=raw.get("packages") or [], - env_required=raw.get("env-required") or raw.get("env_required") or [], + ssh_keys=tuple(raw.get("ssh-keys") or raw.get("ssh_keys") or []), + runcmd=tuple(raw.get("runcmd") or []), + packages=tuple(raw.get("packages") or []), + env_required=tuple(raw.get("env-required") or raw.get("env_required") or []), ) @@ -53,21 +53,21 @@ def plan_bootstrap( m = HostnameModule(hostname=profile.hostname) actions.append(BootstrapAction( phase="setup", description=m.describe(), - commands=m.plan(), needs_sudo=True, + commands=tuple(m.plan()), needs_sudo=True, )) if profile.locale: m = LocaleModule(locale=profile.locale) actions.append(BootstrapAction( phase="setup", description=m.describe(), - commands=m.plan(), needs_sudo=True, + commands=tuple(m.plan()), needs_sudo=True, )) if profile.ssh_keys: - m = SSHKeygenModule(keys=profile.ssh_keys) + m = SSHKeygenModule(keys=list(profile.ssh_keys)) actions.append(BootstrapAction( phase="setup", description=m.describe(), - commands=m.plan(), + commands=tuple(m.plan()), )) # Phase 3: Packages @@ -83,7 +83,7 @@ def plan_bootstrap( actions.append(BootstrapAction( phase="packages", description=f"Install {len(packages_to_install)} package(s): {', '.join(pkg_names[:5])}{'...' if len(pkg_names) > 5 else ''}", - commands=[], # Executed by PackageService + commands=(), # Executed by PackageService )) # Phase 4: Shell @@ -91,26 +91,26 @@ def plan_bootstrap( m = ShellModule(shell=profile.shell) actions.append(BootstrapAction( phase="shell", description=m.describe(), - commands=m.plan(), needs_sudo=True, + commands=tuple(m.plan()), needs_sudo=True, )) # Phase 5: Custom commands if profile.runcmd: - m = RuncmdModule(commands=profile.runcmd) + m = RuncmdModule(commands=list(profile.runcmd)) actions.append(BootstrapAction( phase="setup", description=m.describe(), - commands=m.plan(), + commands=tuple(m.plan()), )) # Phase 6: Dotfiles link actions.append(BootstrapAction( phase="dotfiles", description="Link dotfiles", - commands=[], # Executed by DotfilesService + commands=(), # Executed by DotfilesService )) return BootstrapPlan( profile=profile.name, - actions=actions, - packages_to_install=packages_to_install, + actions=tuple(actions), + packages_to_install=tuple(packages_to_install), ) diff --git a/src/flow/domain/dotfiles/models.py b/src/flow/domain/dotfiles/models.py index 91b1537..01d612e 100644 --- a/src/flow/domain/dotfiles/models.py +++ b/src/flow/domain/dotfiles/models.py @@ -68,8 +68,8 @@ class PlanSummary: @dataclass(frozen=True) class LinkPlan: """Complete reconciliation plan.""" - operations: list[LinkOp] - conflicts: list[str] + operations: tuple[LinkOp, ...] + conflicts: tuple[str, ...] summary: PlanSummary diff --git a/src/flow/domain/dotfiles/planning.py b/src/flow/domain/dotfiles/planning.py index 16c1d35..0713bf5 100644 --- a/src/flow/domain/dotfiles/planning.py +++ b/src/flow/domain/dotfiles/planning.py @@ -83,8 +83,8 @@ def plan_link( from_modules += 1 return LinkPlan( - operations=ops, - conflicts=conflicts, + operations=tuple(ops), + conflicts=tuple(conflicts), summary=PlanSummary( added=added, removed=removed, unchanged=unchanged, from_modules=from_modules, @@ -113,7 +113,7 @@ def plan_unlink( )) return LinkPlan( - operations=ops, - conflicts=[], + operations=tuple(ops), + conflicts=(), summary=PlanSummary(added=0, removed=len(ops), unchanged=0, from_modules=0), ) diff --git a/src/flow/domain/packages/models.py b/src/flow/domain/packages/models.py index 25c9612..e718219 100644 --- a/src/flow/domain/packages/models.py +++ b/src/flow/domain/packages/models.py @@ -50,7 +50,7 @@ class PkgRemoveOp: """A single package remove operation.""" name: str type: str - files: list[Path] + files: tuple[Path, ...] def __str__(self) -> str: return f"REMOVE: {self.name} ({len(self.files)} file(s))" @@ -59,8 +59,8 @@ class PkgRemoveOp: @dataclass(frozen=True) class PackagePlan: """Complete package install/remove plan.""" - install_ops: list[PkgInstallOp] - remove_ops: list[PkgRemoveOp] + install_ops: tuple[PkgInstallOp, ...] + remove_ops: tuple[PkgRemoveOp, ...] pm_update_needed: bool pm_command: Optional[str] diff --git a/src/flow/domain/packages/planning.py b/src/flow/domain/packages/planning.py index 15ef948..6a2adbc 100644 --- a/src/flow/domain/packages/planning.py +++ b/src/flow/domain/packages/planning.py @@ -2,6 +2,7 @@ from typing import Optional +from flow.core.errors import FlowError from flow.domain.packages.models import ( InstalledState, PackageDef, @@ -36,6 +37,11 @@ def plan_install( continue # Already installed if pkg.type == "pkg": + if pm is None: + raise FlowError( + f"No supported package manager found to install '{pkg.name}'. " + "Install apt, dnf, or brew." + ) source_name = resolve_source_name(pkg, pm) pm_packages.append(source_name) install_ops.append(PkgInstallOp( @@ -43,12 +49,8 @@ def plan_install( source_name=source_name, download_url=None, )) elif pkg.type in ("binary", "appimage"): - try: - asset = resolve_binary_asset(pkg, platform_str) - url = resolve_download_url(pkg, asset) - except Exception: - asset = pkg.name - url = None + asset = resolve_binary_asset(pkg, platform_str) + url = resolve_download_url(pkg, asset) install_ops.append(PkgInstallOp( package=pkg, method=pkg.type, source_name=asset, download_url=url, @@ -63,8 +65,8 @@ def plan_install( pm_cmd = pm_install_command(pm, pm_packages) if pm and pm_packages else None return PackagePlan( - install_ops=install_ops, - remove_ops=[], + install_ops=tuple(install_ops), + remove_ops=(), pm_update_needed=bool(pm_packages), pm_command=pm_cmd, ) @@ -81,12 +83,12 @@ def plan_remove( if name in installed.packages: pkg = installed.packages[name] remove_ops.append(PkgRemoveOp( - name=name, type=pkg.type, files=pkg.files, + name=name, type=pkg.type, files=tuple(pkg.files), )) return PackagePlan( - install_ops=[], - remove_ops=remove_ops, + install_ops=(), + remove_ops=tuple(remove_ops), pm_update_needed=False, pm_command=None, ) diff --git a/src/flow/services/dotfiles.py b/src/flow/services/dotfiles.py index 84aa40d..60095b7 100644 --- a/src/flow/services/dotfiles.py +++ b/src/flow/services/dotfiles.py @@ -269,10 +269,11 @@ class DotfilesService: """Find and parse _module.yaml in a package directory.""" for module_yaml in pkg_dir.rglob(MODULE_FILE): try: - with open(module_yaml) as f: + with open(module_yaml, encoding="utf-8") as f: raw = yaml.safe_load(f) or {} - except Exception: - continue + except (OSError, yaml.YAMLError) as e: + from flow.core.errors import ConfigError + raise ConfigError(f"Failed to read {module_yaml}: {e}") from e mount_path = compute_mount_path(module_yaml, pkg_dir) diff --git a/src/flow/services/packages.py b/src/flow/services/packages.py index 1d9d969..8eddbf4 100644 --- a/src/flow/services/packages.py +++ b/src/flow/services/packages.py @@ -117,8 +117,8 @@ class PackageService: self.ctx.runtime.fs.ensure_dir(tmp_dir) archive = tmp_dir / asset - self.ctx.runtime.runner.run_shell( - f"curl -fSL -o {archive} '{url}'", check=True, + self.ctx.runtime.runner.run( + ["curl", "-fSL", "-o", str(archive), url], check=True, ) bin_dir = Path.home() / ".local" / "bin" @@ -128,15 +128,15 @@ class PackageService: if asset.endswith((".tar.gz", ".tar.xz", ".tar.bz2", ".tgz")): extract_dir = tmp_dir / f"{pkg.name}-extract" self.ctx.runtime.fs.ensure_dir(extract_dir) - self.ctx.runtime.runner.run_shell( - f"tar -xf {archive} -C {extract_dir}", check=True, + self.ctx.runtime.runner.run( + ["tar", "-xf", str(archive), "-C", str(extract_dir)], check=True, ) # Find and install binaries install_cfg = pkg.install or {} binary_name = install_cfg.get("binary", pkg.name) - src_dir = pkg.extract_dir or "" + search_root = extract_dir / pkg.extract_dir if pkg.extract_dir else extract_dir - for candidate in extract_dir.rglob(binary_name): + for candidate in search_root.rglob(binary_name): if candidate.is_file(): target = bin_dir / binary_name self.ctx.runtime.fs.copy_file(candidate, target) @@ -169,8 +169,8 @@ class PackageService: target = bin_dir / pkg.name self.ctx.console.info(f"Downloading {pkg.name} AppImage...") - self.ctx.runtime.runner.run_shell( - f"curl -fSL -o {target} '{url}'", check=True, + self.ctx.runtime.runner.run( + ["curl", "-fSL", "-o", str(target), url], check=True, ) target.chmod(0o755) diff --git a/tests/test_domain_bootstrap_planning.py b/tests/test_domain_bootstrap_planning.py index 201fb49..7b30359 100644 --- a/tests/test_domain_bootstrap_planning.py +++ b/tests/test_domain_bootstrap_planning.py @@ -26,7 +26,7 @@ class TestParseProfile: profile = parse_profile("minimal", {}) assert profile.os == "linux" assert profile.hostname is None - assert profile.packages == [] + assert profile.packages == () def test_ssh_keys(self): raw = {"ssh-keys": [{"path": "~/.ssh/id_ed25519", "type": "ed25519"}]}