diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..d28aff4 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,42 @@ +# Development Standards + +## General + +- **Fix the root cause, not the symptom.** Do not add defaults, fallbacks, conditional wrappers, or + runtime workarounds to cope with a broken upstream flow. If a value is missing, fix the source + that provides it. If a dependency isn't ready, fix the ordering. + +- **No speculative abstraction.** Do not add overrides, toggles, or configurable escape hatches for + scenarios that do not exist yet. If a new requirement appears, add the abstraction then. + +- **Fail loudly at the boundary.** When a value reaches an unexpected state, throw or surface the + error immediately. A loud failure with a clear trace is more useful than a silent fallback that + hides the bug and lets it propagate further. + +- **Direct access over defensive wrapping.** Access values directly. Do not add null-coalescing + chains, fallback guards, or default-dict patterns for values that are always expected to be + present. + +- **One canonical form.** Keep a single representation for each concept -- one source of truth for + constants, one naming convention per domain, no conversion layers between equivalent forms. + +- **Explicit over implicit.** Write values and behaviour directly rather than generating or + inferring them when the intent is small and stable. Clarity at the call site matters more than + cleverness at the definition. + +- **Reuse existing mechanisms.** Do not introduce local duplicates of something already shared + without a clear reason. Prefer extending what exists. + +- **No empty files or placeholders.** Every file committed should serve a purpose. + +- **Exhaustive over defensive.** When branching on a finite set of known values, handle every case + explicitly. The compiler should enforce completeness -- not runtime guards or catch-all defaults. + +- **Infer intent from context.** Resolve ambiguity against existing conventions and adjacent code + first. If uncertainty remains, ask before changing behaviour. + +- **Refactors are collaborative.** Discuss direction first, implement once agreed. Prefer small + directed phases over broad rewrites. + +- **During implementation, audit the upstream flow.** Report any bug, mismatch, or unexpected + result found -- even if it is outside the immediate scope. diff --git a/docs/architecture.md b/docs/architecture.md deleted file mode 100644 index 65e3a73..0000000 --- a/docs/architecture.md +++ /dev/null @@ -1,52 +0,0 @@ -## Flow CLI Architecture - -### Layers - -`flow` now follows a stricter adapter/service/runtime split: - -- `flow.cli` - - global startup, platform/config loading, top-level error handling -- `flow.commands.*` - - argparse registration and compatibility wrappers only -- `flow.services.*` - - domain behavior for SSH entry, containers, dotfiles, bootstrap, packages, and project sync -- `flow.core.system` - - shared process, git, filesystem, and JSON state primitives -- `flow.core.*` - - config loading, platform detection, console output, variables - -### Runtime Safety - -Mutating operations are centralized behind `flow.core.system`: - -- `CommandRunner` - - subprocess execution and shell streaming -- `GitClient` - - repository-scoped git execution -- `FileSystem` - - directory creation, copy, symlink, removal, JSON/text writes -- `JsonStateStore` - - state persistence with explicit paths - -This keeps command handlers out of the business of directly creating, deleting, or overwriting filesystem state. - -### Domain Boundaries - -- `services.ssh` - - parses enter targets, resolves host templates, builds the SSH handoff -- `services.containers` - - owns container create/exec/connect/list/stop/remove/respawn logic -- `services.projects` - - owns git status/fetch/summary logic for project directories -- `services.package_defs` - - normalizes manifest package definitions and binary package install logic -- `services.packages` - - package state listing/install/remove behavior -- `services.bootstrap` - - provisioning orchestration, package-manager resolution, hooks, shell/locale/hostname setup -- `services.dotfiles` - - repo sync, module discovery, link planning, transactional undo, status, edit flow - -### Compatibility Strategy - -The current CLI surface is preserved. The command modules still expose a small set of legacy helper symbols because the existing tests use them directly, but the behavioral implementation now lives in the service layer. diff --git a/docs/code-review.md b/docs/code-review.md new file mode 100644 index 0000000..065da26 --- /dev/null +++ b/docs/code-review.md @@ -0,0 +1,271 @@ +# Flow CLI -- Code Review + +> Reviewed 2026-03-15 against commit `24d682a` (main). Source: ~6,900 LOC across 21 modules. Tests: +> ~2,600 LOC, 167 pass / 6 skip. + +--- + +## 1. Reason to Exist + +Flow solves a real problem: managing a personal dev environment across machines. It unifies dotfiles +linking, machine bootstrapping, container management, SSH entry, and binary package installs into +one CLI. The alternative is a pile of shell scripts or Ansible for what is essentially +personal-workstation setup. Flow is lighter, more opinionated, and self-hosted (config lives in your +dotfiles repo). This is a reasonable project to maintain. + +--- + +## 2. Features Supported + +| Area | Implemented | Notes | +| ------------ | --------------------------------------------------------------------------------------------- | ----------------------------------------- | +| `enter` | SSH handoff with tmux, terminfo warnings, target config | Works | +| `dev` | Container create/exec/connect/list/stop/remove/respawn | Docker & Podman | +| `dotfiles` | init/link/unlink/undo/status/sync/relink/clean/edit, modules, repo ops | Most complex subsystem | +| `bootstrap` | Profile-driven provisioning: packages, shell, locale, hostname, ssh-keygen, runcmd, post-link | Works | +| `package` | Binary package install/list/remove from manifest | Real install; "remove" only forgets state | +| `sync` | Git health check across `~/projects` | Works | +| `completion` | Dynamic zsh completion with context-aware candidates | Thorough | + +--- + +## 3. Mismatches Between Docs/README and Source Code + +### 3.1 Architecture doc describes layers that don't fully exist yet + +`docs/architecture.md` describes a clean adapter/service/runtime split: + +> `flow.commands.*` — argparse registration and compatibility wrappers only `flow.services.*` — +> domain behavior + +In practice: + +- **`commands/sync.py`** still contains ~170 lines of its own git logic (`_git`, `_check_repo`, + `run_check`, `run_fetch`, `run_summary`) that duplicate `services/projects.py` almost + line-for-line. The service exists but the command module **never imports or uses it**. +- **`commands/package.py`** does its own JSON state loading and binary install calls instead of + delegating to `services/packages.py`. The service class `PackageService` exists but is **never + used anywhere**. +- **`commands/dotfiles.py`** is a 193-line shim that re-exports ~20 private functions from + `services/dotfiles.py`, each wrapped in `_sync_service_module()`. The command module is not + "argparse registration only" -- it's a compatibility layer that monkeypatches module-level + constants into the service at runtime. + +**Verdict:** The architecture doc describes the target state, not the actual state. Two service +modules (`projects`, `packages`) are dead code. + +### 3.2 README says "`environments` is not supported" but code still checks for it + +`services/bootstrap.py:63` raises `RuntimeError` if `environments` key is found. This is actually +correct defensive code, but the README makes it sound like the key is simply ignored. Minor. + +### 3.3 README documents `flow dev create api -i tm0/node -p ~/projects/api` + +The code works, but the container is always `sleep infinity` + exec-in. README doesn't mention this +is the execution model (not a standard container entry point). + +### 3.4 `docs/flows.md` lists "Keep But Treat As Convenience Aliases" section + +These aliases aren't flagged in code or docs as aliases. `dotfiles sync` is described as +"`repo pull` + `modules sync`", but in code it's `_pull_dotfiles` + `_sync_modules` -- not actually +calling the repo/modules commands. Functionally equivalent but not aliased. + +--- + +## 4. Code Quality Assessment + +### 4.1 Bugs + +#### Duplicate method definition in `system.py` + +`FileSystem.write_bytes` is defined **twice** (lines 259-261 and 263-265). The second definition +silently shadows the first. They're identical, so no behavioral bug, but this is a clear copy-paste +error. + +#### `commands/container.py` has unused legacy functions + +`_container_exists`, `_container_running`, and `_tmux_fallback` are defined at module level (lines +61-122) but never called -- the `ContainerService` class has its own versions. Dead code. + +#### `commands/bootstrap.py` monkeypatches the service module + +```python +def _sync_service_module() -> None: + _service.shutil = shutil + _service.urllib = urllib + _service._copy_install_item = _copy_install_item +``` + +This replaces `shutil` and `urllib` on the service module object at runtime. The service already +imports these -- this patching has no real effect (the service's own imports win). +`_copy_install_item` patch is circular: it reassigns the service function to a wrapper that calls +the original. This is effectively a no-op. + +#### `dotfiles.py` command module monkeypatches module-level constants + +```python +def _sync_service_module() -> None: + _service.DOTFILES_DIR = DOTFILES_DIR + _service.MODULES_DIR = MODULES_DIR + _service.LINKED_STATE = LINKED_STATE + ... +``` + +The service already imports these from `flow.core.paths`. This override only matters if the command +module's own constants diverge from the service's -- but they're imported from the same source. The +whole pattern is a code smell from an incomplete refactoring. + +### 4.2 Massive Code Duplication + +This is the single biggest quality issue. The bootstrap service duplicates nearly every function +from `package_defs.py`: + +| `services/package_defs.py` | `services/bootstrap.py` | +| ------------------------------------- | ------------------------------------ | +| `linux_detect_package_manager()` | `_linux_detect_package_manager()` | +| `resolve_package_manager()` | `_resolve_package_manager()` | +| `get_package_catalog()` | `_get_package_catalog()` | +| `normalize_profile_package_entry()` | `_normalize_profile_package_entry()` | +| `resolve_package_spec()` | `_resolve_package_spec()` | +| `resolve_pkg_source_name()` | `_resolve_pkg_source_name()` | +| `platform_lookup_keys()` | `_platform_lookup_keys()` | +| `resolve_binary_platform_vars()` | `_resolve_binary_platform_vars()` | +| `resolve_binary_asset()` | `_resolve_binary_asset()` | +| `resolve_binary_download_url()` | `_resolve_binary_download_url()` | +| `validate_declared_install_path()` | `_validate_declared_install_path()` | +| `install_destination()` | `_install_destination()` | +| `install_strip_prefix()` | `_install_strip_prefix()` | +| `strip_prefix()` | `_strip_prefix()` | +| `BinaryInstaller.install()` | `_install_binary_package()` | +| `BinaryInstaller.copy_install_item()` | `_copy_install_item()` | +| `profile_template_context()` | `_profile_template_context()` | +| `render_template_value()` | `_render_template_value()` | + +That's **18 duplicate function pairs** with near-identical implementations. `package_defs.py` was +clearly extracted as the canonical version, but `bootstrap.py` was never updated to import from it. +Similarly, `services/projects.py` duplicates `commands/sync.py`. + +Total duplicate code: roughly 500-600 lines. + +### 4.3 Module-Level Singletons + +`services/dotfiles.py` and `services/bootstrap.py` use module-level singletons: + +```python +_RUNNER = CommandRunner() +_FS = FileSystem() +``` + +These are then patched by the command modules. This makes the service modules hard to test in +isolation and creates hidden mutable global state. The `FlowContext` already carries a +`SystemRuntime` with `runner`, `fs`, and `git` -- but the service code ignores it and uses the +module-level singletons instead. + +### 4.4 Tests Reach Into Private APIs + +Tests import private (underscore-prefixed) functions directly: + +```python +from flow.commands.bootstrap import ( + _ensure_required_variables, + _get_profiles, + _install_binary_package, + ... +) +``` + +This couples tests to internal implementation details. Combined with the command-module shimming +pattern, it means the test suite is testing the wrong layer (command shims instead of service logic +directly). + +### 4.5 Inconsistent Error Handling + +Two exception hierarchies coexist: + +- `FlowError(RuntimeError)` -- defined in `core/errors.py`, used by `system.py` and services +- Plain `RuntimeError` -- used throughout `bootstrap.py`, `dotfiles.py`, and command modules + +The CLI entry point catches `RuntimeError` as the generic error type, so both work. But `FlowError` +was clearly intended to be the project-wide error type and is underused. The `bootstrap.py` service +(1001 lines) never imports or uses `FlowError`. + +### 4.6 `stow.py` is Unused + +`core/stow.py` (358 lines) implements a GNU Stow-style tree folding/unfolding algorithm with +`LinkTree`, `TreeFolder`, `LinkOperation`. It has 310 lines of tests. But **nothing in the +application imports or uses it**. The dotfiles service has its own `LinkSpec`-based approach. This +is dead code (or a planned feature that never landed). + +### 4.7 `action.py` is Unused + +`core/action.py` (120 lines) defines `Action` and `ActionExecutor` for plan-then-execute workflows. +It has 115 lines of tests. But **no command or service uses it**. The bootstrap command does its own +sequential execution. Dead code. + +### 4.8 `process.py` is a Thin Wrapper With Odd Restrictions + +```python +def run_command(command, console, *, check=True, shell=True, capture=False): + if not shell: + raise RuntimeError("run_command only supports shell commands") +``` + +It rejects `shell=False` even though the parameter exists in the signature. This wrapper creates a +new `CommandRunner()` on every call instead of using the one from context. + +### 4.9 Minor Issues + +- **No type checking enforcement.** `pyproject.toml` has no mypy/pyright configuration. Type + annotations exist but are never validated. +- **`ConsoleLogger` hardcodes ANSI codes.** No TTY detection, no `--no-color` flag. Piping output to + a file produces escape sequences. +- **`flow package remove` only forgets state.** README and `docs/flows.md` both acknowledge this, + but the CLI help text says "Remove installed packages" without qualification. Users will expect + files to be deleted. +- **SSH keygen quoting.** `bootstrap.py:747` passes `shlex.quote` results into a string that is + later shell-executed. Double quoting could be an issue with values containing spaces, though + unlikely in practice for key filenames. + +--- + +## 5. Summary + +### What's Good + +- **Clear domain model.** The manifest format is well-designed: profiles, packages, dotfiles layout, + modules, templates. It's genuinely useful. +- **Transactional dotfiles linking.** The undo system with snapshots and backup directories is + well-thought-out and more robust than most dotfile managers. +- **Defensive input validation.** Config parsing handles multiple formats (dict keys, lists, string + shorthand), missing values, and type mismatches gracefully. +- **Test coverage exists.** 167 tests pass. The dotfiles folding and e2e container tests show real + investment in correctness. +- **Clean CLI surface.** Argparse registration is consistent, aliases work, help text is clear. +- **Zsh completion is thorough.** Context-aware dynamic completion is a nice touch. + +### What Needs Work + +| Priority | Issue | Effort | +| -------- | ------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------- | +| High | ~600 lines of duplicated code between bootstrap and package_defs | Delete duplicates, import from package_defs | +| High | Two dead modules (stow.py, action.py) + dead service modules (projects.py, packages.py) = ~900 lines of unused code | Delete or wire up | +| High | Command modules bypass their own service layer | Finish the refactoring described in architecture.md | +| Medium | Module-level singletons vs FlowContext.runtime | Use runtime from context consistently | +| Medium | `_sync_service_module()` monkeypatching pattern | Remove once services use their own imports | +| Medium | Inconsistent error types (RuntimeError vs FlowError) | Standardize on FlowError | +| Low | No color/TTY detection | Add `--no-color` or detect `isatty` | +| Low | `package remove` semantics | Either implement real uninstall or rename to `forget` | +| Low | Duplicate `write_bytes` method | Delete the duplicate | + +### Overall Assessment + +This is a **functional but mid-refactoring codebase**. The core functionality works -- dotfiles +linking, bootstrapping, container management, and SSH entry all do what they claim. The manifest +model and transactional undo are genuinely well-designed. + +The main debt is architectural: a service-layer extraction was started but left incomplete, +resulting in large amounts of duplicated code, dead modules, and a monkeypatching compatibility +layer. The tests pass but are coupled to the wrong abstraction layer. + +For a personal "vibe-coded" tool, this is solid. For production or team use, the duplication and +dead code need cleanup before adding more features. diff --git a/docs/flows.md b/docs/flows.md deleted file mode 100644 index 42ec5b7..0000000 --- a/docs/flows.md +++ /dev/null @@ -1,86 +0,0 @@ -## Feature Inventory - -### Core Features - -- `enter` - - SSH into a named environment target with optional tmux auto-attach -- `dev` - - create, exec into, attach to, list, stop, remove, and respawn development containers -- `dotfiles` - - clone the dotfiles repo, link/unlink/relink configs, undo link transactions, inspect status, sync modules, clean broken links, edit packages, and interact with repo state -- `bootstrap` - - run machine bootstrap profiles with packages, env validation, hostname/locale/shell setup, ssh-keygen, `runcmd`, config linking, and post-link hooks -- `package` - - install/list/remove binary packages defined in the manifest -- `sync` - - inspect git project health, fetch remotes, and summarize project state -- `completion` - - dynamic zsh completion generation and installation - -### Supported Flows - -#### Access a host - -1. Resolve `[user@]namespace@platform` -2. Expand platform host template or configured target override -3. Optionally warn about missing remote terminfo -4. Open SSH, optionally into tmux - -#### Start a dev container - -1. Resolve runtime (`docker` or `podman`) -2. Normalize image shorthand -3. Apply labels and common host mounts -4. Start the container -5. `flow dev connect` attaches through tmux or falls back to direct exec - -#### Manage dotfiles - -1. Clone dotfiles repo -2. Optionally sync external module repos -3. Resolve shared + profile packages -4. Validate target conflicts -5. Snapshot replaced targets -6. Apply links transactionally -7. Undo from persisted transaction state if needed - -#### Bootstrap a machine - -1. Load and validate a profile -2. Detect or select the package manager -3. Check required environment variables -4. Apply hostname/locale/shell prerequisites -5. Install profile packages -6. Run package hooks -7. Generate SSH keys -8. Run `runcmd` -9. Link dotfiles for the profile -10. Run post-link hooks - -### Command Surface Review - -### Keep - -- `enter` -- `dev` -- `dotfiles` -- `bootstrap` -- `package` -- `sync` -- `completion` - -### Keep But Treat As Convenience Aliases - -- `dotfiles sync` - - effectively `repo pull` + `modules sync` -- `dotfiles relink` - - effectively `unlink` + `link` -- `sync summary` - - effectively `sync check --no-fetch` - -### Commands That Need Follow-Up Product Decisions - -- `package remove` - - today it forgets install state but does not uninstall files; either rename it to `forget` or implement real uninstall semantics -- `dotfiles edit` - - current auto-commit/push behavior is powerful but risky; it may deserve an explicit confirm-or-dry-run mode before wider use