tm0/flow
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address all code review issues

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Tomas Mirchev
2026-03-16 06:27:31 +02:00
parent bc420114bf
commit 78d4064853
13 changed files with 105 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
src/flow/cli.py
View File

@@ -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,
)

26
src/flow/core/config.py
View File

@@ -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

32
src/flow/core/runtime.py
View File

@@ -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:

14
src/flow/domain/bootstrap/models.py
View File

@@ -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:

19
src/flow/domain/bootstrap/modules.py
View File

@@ -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

30
src/flow/domain/bootstrap/planning.py
View File

@@ -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),
)

4
src/flow/domain/dotfiles/models.py
View File

@@ -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

8
src/flow/domain/dotfiles/planning.py
View File

@@ -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),
)

6
src/flow/domain/packages/models.py
View File

@@ -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]

24
src/flow/domain/packages/planning.py
View File

@@ -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,
)

7
src/flow/services/dotfiles.py
View File

@@ -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)

16
src/flow/services/packages.py
View File

@@ -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)

2
tests/test_domain_bootstrap_planning.py
View File

@@ -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"}]}