From b459465aa82feed5592d567201aa2140261fb4ed Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 14:12:21 +0300 Subject: [PATCH 01/48] feat(zsh): detect/disable redundant legacy tooling, fix compinit dup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a legacy_shell module that finds pre-existing zinit, asdf, or nvm setups duplicating what devboost's znap/mise already manage, and disables the redundant lines in place (commented out with a devboost:disabled marker, never deleted) so they stay reviewable and reversible by hand. A new `devboost clean` command permanently removes marked lines once trusted, idempotently and independent of when apply last ran. Also fixes a real bug in the zsh module's own template: it called compinit itself before sourcing znap, which redefines compinit as a no-op and runs its own deferred completion init — so devboost's call did a full, wasted rebuild every shell start. Removing it, together with the legacy tooling fixes, cut a real machine's measured login-shell startup from ~1.44s to ~285-305ms. Closes the startup-lag root cause tracked in #5, #6, #7. --- .devboost.yaml.example | 9 + CHANGELOG.md | 12 ++ README.md | 16 ++ build.sh | 12 +- core/core_legacy_shell.sh | 202 +++++++++++++++++ core/core_main.sh | 10 +- devboost.sh | 380 +++++++++++++++++++++++++++++++- modules/module_legacy_shell.sh | 158 ++++++++++++++ modules/module_zsh.sh | 7 +- tests/test-legacy-shell.sh | 381 +++++++++++++++++++++++++++++++++ 10 files changed, 1175 insertions(+), 12 deletions(-) create mode 100644 core/core_legacy_shell.sh create mode 100644 modules/module_legacy_shell.sh create mode 100755 tests/test-legacy-shell.sh diff --git a/.devboost.yaml.example b/.devboost.yaml.example index 0e1cc29..07485b7 100644 --- a/.devboost.yaml.example +++ b/.devboost.yaml.example @@ -74,6 +74,15 @@ zsh: aliases: enable: true +# Detects and disables shell tooling that duplicates what devboost already +# manages when found in your pre-existing ~/.zshrc (a leftover zinit setup +# duplicating znap's plugins, or asdf alongside mise). Redundant lines are +# commented out (never deleted) so you can review/undo by hand; run +# `devboost clean` separately to actually remove the disabled lines. +legacy_shell: + enable: true + zshrc: ~/.zshrc + prompt: enable_starship: true starship_config: ~/.config/starship.toml diff --git a/CHANGELOG.md b/CHANGELOG.md index be0844a..bdaaa81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ with OS/tooling-specific adjustments. ## [Unreleased] +## [1.4.0] - 2026-08-08 + +### Added +- `legacy_shell` module: detects shell tooling in a pre-existing `~/.zshrc`/`~/.zprofile` that duplicates what devboost already manages — a leftover `zinit` setup loading the same plugins as devboost's `znap` (`zsh-autosuggestions`, a syntax-highlighting fork), `asdf` sourced alongside devboost's `mise`, or `nvm`'s shell hook (measured at ~850-900ms per login shell) also alongside `mise` — surfaced via `doctor`, and disabled by `apply`/`plan`. Redundant lines are commented out in place with a `# devboost:disabled:` marker rather than deleted, so they can be reviewed or restored by hand at any time; if a user removes the marker themselves, later runs respect that as an explicit override and leave the line alone. Full pre/post snapshots of every edited file are kept in `~/.devboost/backups/` as an audit trail independent of the marker. +- `devboost clean` command: permanently removes lines previously marked `# devboost:disabled:...`. Idempotent and order-independent — it re-derives what to remove by scanning the live file each run, so it works correctly regardless of when or whether `apply` last ran. Respects `--dry-run`. + +### Fixed +- `zsh` module no longer calls `compinit` itself in the generated `.zshrc.devboost`. It ran *before* znap was sourced, but znap redefines `compinit`/`compdef` as no-ops and runs its own deferred, `precmd`-hook-based compinit into a separate dumpfile the moment it loads — so devboost's own call was a wasted full completion rebuild every shell start, immediately superseded by znap's. Removing it, combined with the `legacy_shell` fixes above, took a real-machine's measured login-shell startup from ~1.44s to ~285-305ms (~80% reduction). + +### Motivation +Investigating real-world zsh startup lag surfaced this exact conflict on a live machine: a pre-existing, non-devboost `zinit` setup, `asdf`, and `nvm` were all running fully redundant plugin/version-manager initialization on every shell start, before devboost's own managed config even loaded — compounded by devboost's own generated config doing a second, wasted completion rebuild on top. + ## [1.3.0] - 2026-08-04 ### Added diff --git a/README.md b/README.md index b0348b8..03f0409 100644 --- a/README.md +++ b/README.md @@ -183,6 +183,7 @@ devboost [COMMAND] [OPTIONS] - **`plan`** - Preview what would change (dry-run) - **`doctor`** - Check system health and prerequisites - **`uninstall`** - Remove devboost-managed files +- **`clean`** - Remove devboost-disabled legacy-tooling lines and archived directories - **`migrate-from-oh-my-zsh`** - Remove oh-my-zsh and recover `.zshrc` customizations (destructive — needs `--yes`) ### Options @@ -212,8 +213,23 @@ devboost apply --config ~/my-config.yaml # Remove oh-my-zsh and recover your customizations devboost migrate-from-oh-my-zsh --dry-run # preview first devboost migrate-from-oh-my-zsh --yes # then actually run it + +# Permanently remove lines devboost previously disabled (see below) +devboost clean --dry-run # preview first +devboost clean # then actually run it ``` +### Legacy shell tooling cleanup + +If devboost finds a pre-existing `~/.zshrc` with tooling that duplicates what it +already manages — a leftover `zinit` setup loading the same plugins as devboost's +`znap`, or `asdf` alongside devboost's `mise` — `apply` comments out the redundant +lines in place (prefixed with `# devboost:disabled:...`) rather than deleting them, +so you can review or restore them by hand at any time. Run `devboost clean` whenever +you're ready to permanently remove those disabled lines; it's idempotent and safe to +run repeatedly. Disable this behavior entirely with `legacy_shell.enable: false` in +your config. + --- ## ⚙️ Configuration diff --git a/build.sh b/build.sh index dea8e2e..f80fab5 100755 --- a/build.sh +++ b/build.sh @@ -8,7 +8,7 @@ OUT="devboost.sh" { # Entry point cat devboost.sh.in - + # Core framework (order matters) echo "" echo "# === Core Framework ===" @@ -22,10 +22,12 @@ OUT="devboost.sh" echo "" cat core/core_omz.sh echo "" + cat core/core_legacy_shell.sh + echo "" cat core/core_modules.sh echo "" cat core/core_main.sh - + # Modules (order matters - dependencies first) echo "" echo "# === Modules ===" @@ -35,6 +37,8 @@ OUT="devboost.sh" echo "" cat modules/module_zsh.sh echo "" + cat modules/module_legacy_shell.sh + echo "" cat modules/module_starship.sh echo "" cat modules/module_tmux.sh @@ -60,6 +64,7 @@ db_load_modules() { db_module_pkg_register db_module_znap_register db_module_zsh_register + db_module_legacy_shell_register db_module_starship_register db_module_tmux_register db_module_mise_register @@ -72,7 +77,7 @@ db_load_modules() { db_log_verbose "Loaded ${#DB_MODULE_NAMES[@]} modules" } REGEOF - + # Main execution echo "" echo "# === Main Execution ===" @@ -94,4 +99,3 @@ chmod +x "$OUT" echo "Built: $OUT" echo "Size: $(wc -l < "$OUT") lines" - diff --git a/core/core_legacy_shell.sh b/core/core_legacy_shell.sh new file mode 100644 index 0000000..106cf8a --- /dev/null +++ b/core/core_legacy_shell.sh @@ -0,0 +1,202 @@ +# Generic helpers for detecting and safely retiring legacy shell tooling +# that devboost's own modules have superseded (e.g. a pre-existing zinit +# setup duplicating znap's plugins, or asdf duplicating mise). +# +# Design (see AGENTS.md's non-destructive principle, applied to files +# devboost does not own): +# - Redundant lines in a user-owned file (like ~/.zshrc) are never +# deleted. They're commented out in place with a marker that both a +# human and this tooling can recognize: +# # devboost:disabled: +# A human can undo this by hand (delete the marker prefix). If they +# do, later runs must treat that as an explicit override and leave +# the line alone. +# - Redundant directories (like a pre-existing ~/.oh-my-zsh-style +# install root) are moved aside into ~/.devboost/backups/, never +# rm -rf'd. +# - Every edit also gets a full pre/post file snapshot in +# ~/.devboost/backups/ as an audit trail, independent of the marker. +# - `devboost clean` (db_run_clean) is the opt-in, separate step that +# actually deletes marked lines. It re-derives what to clean by +# grepping the live file each run — no reliance on in-process state +# from a prior apply — so it's idempotent and order-independent. + +_DB_LEGACY_MARKER_PREFIX="# devboost:disabled:" + +_db_legacy_marker_for() { + local migration_id="$1" + echo "${_DB_LEGACY_MARKER_PREFIX}${migration_id} " +} + +# Hand-rolled snapshot with a caller-chosen suffix (db_backup_file's +# signature is fixed at one arg with no suffix hook). +_db_legacy_snapshot() { + local file="$1" migration_id="$2" phase="$3" + [[ -f "$file" ]] || return 0 + + local backup_dir="${DB_BACKUP_DIR:-$HOME/.devboost/backups}" + local timestamp + timestamp=$(date +%Y%m%d_%H%M%S) + local dest="${backup_dir}/$(basename "$file").${phase}-${migration_id}-${timestamp}" + + if [[ "${DB_DRY_RUN:-false}" == "true" ]]; then + db_log_info "Would snapshot: $file -> $dest" + return 0 + fi + + db_ensure_dir "$backup_dir" + cp "$file" "$dest" + db_log_verbose "Snapshotted ($phase): $file -> $dest" +} + +# True (0) if `file` currently contains a line matching `grep_pattern` +# that is NOT marked disabled for `migration_id`, AND that same line was +# previously marked (i.e. present in marked form in an earlier post-* +# snapshot for this migration_id). This is the "user peeled the marker +# off by hand" signal — later applies must not re-disable it. +_db_legacy_line_restored() { + local file="$1" grep_pattern="$2" migration_id="$3" + [[ -f "$file" ]] || return 1 + + local marker + marker=$(_db_legacy_marker_for "$migration_id") + + # Is there a live, unmarked line matching the pattern? + grep -Eq "$grep_pattern" "$file" 2>/dev/null || return 1 + grep -E "$grep_pattern" "$file" 2>/dev/null | grep -qv "^${marker}" || { + # every matching live line is already marked -> nothing "restored" + return 1 + } + + # Was it ever marked before, per our own snapshots? Check the most + # recent post- snapshot of this file for the marked form. + local backup_dir="${DB_BACKUP_DIR:-$HOME/.devboost/backups}" + local base + base=$(basename "$file") + local latest_snapshot + latest_snapshot=$(ls -t "${backup_dir}/${base}.post-${migration_id}-"* 2>/dev/null | head -1) || true + [[ -n "$latest_snapshot" ]] || return 1 + + grep -Fq "${marker}" "$latest_snapshot" 2>/dev/null +} + +# Comments out every live, unmarked line in `file` matching +# `grep_pattern` (extended regex) with the devboost:disabled marker for +# `migration_id`. Idempotent: already-marked lines are left untouched. +# No-op if the user has manually restored a previously-marked line +# (see _db_legacy_line_restored) — respects their override. +_db_legacy_disable_lines() { + local file="$1" grep_pattern="$2" migration_id="$3" + [[ -f "$file" ]] || return 0 + + local marker + marker=$(_db_legacy_marker_for "$migration_id") + + # Anything to do at all? Only unmarked lines matching the pattern. + local to_disable + to_disable=$(grep -E "$grep_pattern" "$file" 2>/dev/null | grep -v "^${marker}") || true + if [[ -z "$to_disable" ]]; then + db_log_verbose "No redundant lines to disable for $migration_id in $file" + return 0 + fi + + if _db_legacy_line_restored "$file" "$grep_pattern" "$migration_id"; then + db_log_verbose "Skipping $migration_id in $file — user restored a previously-disabled line" + return 0 + fi + + if [[ "${DB_DRY_RUN:-false}" == "true" ]]; then + while IFS= read -r line; do + db_log_info "Would disable ($migration_id) in $(basename "$file"): $line" + done <<< "$to_disable" + return 0 + fi + + _db_legacy_snapshot "$file" "$migration_id" "pre" + + local temp_file + temp_file=$(mktemp) + awk -v pattern="$grep_pattern" -v marker="$marker" ' + $0 ~ pattern && index($0, marker) != 1 { print marker $0; next } + { print } + ' "$file" > "$temp_file" + + # grep -E and awk's ERE dialect can disagree on backslash-escaped + # literals in a pattern passed through a shell variable (confirmed + # with nvm's source-line pattern). If grep found lines to disable + # but awk's rewrite is identical to the original, the pattern is + # dialect-mismatched — fail loudly instead of silently claiming + # success while leaving the file untouched. + if diff -q "$file" "$temp_file" >/dev/null 2>&1; then + rm -f "$temp_file" + db_log_error "legacy_shell ($migration_id): grep matched lines in $file but awk's rewrite changed nothing — pattern is grep/awk-dialect-mismatched, not applied" + return 1 + fi + + mv "$temp_file" "$file" + + db_log_success "Disabled redundant lines ($migration_id) in: $file" + + _db_legacy_snapshot "$file" "$migration_id" "post" +} + +# Moves `dir` aside into ~/.devboost/backups/ instead of deleting it. +# Idempotent: no-op if `dir` no longer exists (already archived). +_db_legacy_archive_dir() { + local dir="$1" migration_id="$2" + [[ -d "$dir" ]] || return 0 + + local backup_dir="${DB_BACKUP_DIR:-$HOME/.devboost/backups}" + local timestamp + timestamp=$(date +%Y%m%d_%H%M%S) + local dest="${backup_dir}/$(basename "$dir")-${migration_id}-${timestamp}" + + if [[ "${DB_DRY_RUN:-false}" == "true" ]]; then + db_log_info "Would archive: $dir -> $dest" + return 0 + fi + + db_ensure_dir "$backup_dir" + mv "$dir" "$dest" + db_log_success "Archived: $dir -> $dest" +} + +# Strips every devboost:disabled-marked line (any migration_id) from +# `file`, restoring nothing — this is real deletion, the opt-in step. +# Idempotent: no-op if no marked lines are present. +_db_legacy_clean_file() { + local file="$1" + [[ -f "$file" ]] || return 0 + + grep -qF "$_DB_LEGACY_MARKER_PREFIX" "$file" 2>/dev/null || return 0 + + if [[ "${DB_DRY_RUN:-false}" == "true" ]]; then + local count + count=$(grep -cF "$_DB_LEGACY_MARKER_PREFIX" "$file" 2>/dev/null) || count=0 + db_log_info "Would remove $count devboost-disabled line(s) from: $file" + return 0 + fi + + db_backup_file "$file" + local temp_file + temp_file=$(mktemp) + grep -vF "$_DB_LEGACY_MARKER_PREFIX" "$file" > "$temp_file" || true + mv "$temp_file" "$file" + db_log_success "Removed devboost-disabled lines from: $file" +} + +# List of files any legacy-shell migration might mark. Extend here if a +# future migration_id targets a different file. +_db_legacy_managed_files() { + echo "$HOME/.zshrc" + echo "$HOME/.zprofile" +} + +db_run_clean() { + db_log_info "Cleaning devboost-disabled lines..." + local f + while IFS= read -r f; do + _db_legacy_clean_file "$f" + done < <(_db_legacy_managed_files) + db_log_info "Archived directories remain under: ${DB_BACKUP_DIR:-$HOME/.devboost/backups} (remove that folder to purge everything)" +} diff --git a/core/core_main.sh b/core/core_main.sh index 0f5c32d..b4e21bb 100644 --- a/core/core_main.sh +++ b/core/core_main.sh @@ -1,6 +1,6 @@ # Main entry point and CLI -DB_VERSION="1.3.0" +DB_VERSION="1.4.0" DB_SUBCOMMAND="apply" DB_DRY_RUN=false DB_VERBOSE=false @@ -11,7 +11,7 @@ DB_STATE_FILE="${HOME}/.devboost.state.json" db_parse_flags() { while [[ $# -gt 0 ]]; do case $1 in - apply|plan|doctor|uninstall|migrate-from-oh-my-zsh) + apply|plan|doctor|uninstall|clean|migrate-from-oh-my-zsh) DB_SUBCOMMAND="$1" shift ;; @@ -59,6 +59,7 @@ Commands: plan Show actions without changing anything doctor Check prerequisites, PATHs, shells, conflicting files uninstall Remove managed files/blocks (leaves user custom files untouched) + clean Remove devboost-disabled legacy-tooling lines and archived dirs migrate-from-oh-my-zsh Remove oh-my-zsh and recover .zshrc customizations (needs --yes) Options: @@ -75,7 +76,7 @@ EOF coreMain() { # Parse command first (before flags) local cmd="apply" - if [[ $# -gt 0 ]] && [[ "$1" =~ ^(apply|plan|doctor|uninstall|migrate-from-oh-my-zsh)$ ]]; then + if [[ $# -gt 0 ]] && [[ "$1" =~ ^(apply|plan|doctor|uninstall|clean|migrate-from-oh-my-zsh)$ ]]; then cmd="$1" shift fi @@ -130,6 +131,9 @@ coreMain() { uninstall) db_run_uninstall ;; + clean) + db_run_clean + ;; migrate-from-oh-my-zsh) db_run_migrate_from_oh_my_zsh ;; diff --git a/devboost.sh b/devboost.sh index 6ae650f..d7ab386 100755 --- a/devboost.sh +++ b/devboost.sh @@ -559,6 +559,209 @@ db_run_migrate_from_oh_my_zsh() { return 0 } +# Generic helpers for detecting and safely retiring legacy shell tooling +# that devboost's own modules have superseded (e.g. a pre-existing zinit +# setup duplicating znap's plugins, or asdf duplicating mise). +# +# Design (see AGENTS.md's non-destructive principle, applied to files +# devboost does not own): +# - Redundant lines in a user-owned file (like ~/.zshrc) are never +# deleted. They're commented out in place with a marker that both a +# human and this tooling can recognize: +# # devboost:disabled: +# A human can undo this by hand (delete the marker prefix). If they +# do, later runs must treat that as an explicit override and leave +# the line alone. +# - Redundant directories (like a pre-existing ~/.oh-my-zsh-style +# install root) are moved aside into ~/.devboost/backups/, never +# rm -rf'd. +# - Every edit also gets a full pre/post file snapshot in +# ~/.devboost/backups/ as an audit trail, independent of the marker. +# - `devboost clean` (db_run_clean) is the opt-in, separate step that +# actually deletes marked lines. It re-derives what to clean by +# grepping the live file each run — no reliance on in-process state +# from a prior apply — so it's idempotent and order-independent. + +_DB_LEGACY_MARKER_PREFIX="# devboost:disabled:" + +_db_legacy_marker_for() { + local migration_id="$1" + echo "${_DB_LEGACY_MARKER_PREFIX}${migration_id} " +} + +# Hand-rolled snapshot with a caller-chosen suffix (db_backup_file's +# signature is fixed at one arg with no suffix hook). +_db_legacy_snapshot() { + local file="$1" migration_id="$2" phase="$3" + [[ -f "$file" ]] || return 0 + + local backup_dir="${DB_BACKUP_DIR:-$HOME/.devboost/backups}" + local timestamp + timestamp=$(date +%Y%m%d_%H%M%S) + local dest="${backup_dir}/$(basename "$file").${phase}-${migration_id}-${timestamp}" + + if [[ "${DB_DRY_RUN:-false}" == "true" ]]; then + db_log_info "Would snapshot: $file -> $dest" + return 0 + fi + + db_ensure_dir "$backup_dir" + cp "$file" "$dest" + db_log_verbose "Snapshotted ($phase): $file -> $dest" +} + +# True (0) if `file` currently contains a line matching `grep_pattern` +# that is NOT marked disabled for `migration_id`, AND that same line was +# previously marked (i.e. present in marked form in an earlier post-* +# snapshot for this migration_id). This is the "user peeled the marker +# off by hand" signal — later applies must not re-disable it. +_db_legacy_line_restored() { + local file="$1" grep_pattern="$2" migration_id="$3" + [[ -f "$file" ]] || return 1 + + local marker + marker=$(_db_legacy_marker_for "$migration_id") + + # Is there a live, unmarked line matching the pattern? + grep -Eq "$grep_pattern" "$file" 2>/dev/null || return 1 + grep -E "$grep_pattern" "$file" 2>/dev/null | grep -qv "^${marker}" || { + # every matching live line is already marked -> nothing "restored" + return 1 + } + + # Was it ever marked before, per our own snapshots? Check the most + # recent post- snapshot of this file for the marked form. + local backup_dir="${DB_BACKUP_DIR:-$HOME/.devboost/backups}" + local base + base=$(basename "$file") + local latest_snapshot + latest_snapshot=$(ls -t "${backup_dir}/${base}.post-${migration_id}-"* 2>/dev/null | head -1) || true + [[ -n "$latest_snapshot" ]] || return 1 + + grep -Fq "${marker}" "$latest_snapshot" 2>/dev/null +} + +# Comments out every live, unmarked line in `file` matching +# `grep_pattern` (extended regex) with the devboost:disabled marker for +# `migration_id`. Idempotent: already-marked lines are left untouched. +# No-op if the user has manually restored a previously-marked line +# (see _db_legacy_line_restored) — respects their override. +_db_legacy_disable_lines() { + local file="$1" grep_pattern="$2" migration_id="$3" + [[ -f "$file" ]] || return 0 + + local marker + marker=$(_db_legacy_marker_for "$migration_id") + + # Anything to do at all? Only unmarked lines matching the pattern. + local to_disable + to_disable=$(grep -E "$grep_pattern" "$file" 2>/dev/null | grep -v "^${marker}") || true + if [[ -z "$to_disable" ]]; then + db_log_verbose "No redundant lines to disable for $migration_id in $file" + return 0 + fi + + if _db_legacy_line_restored "$file" "$grep_pattern" "$migration_id"; then + db_log_verbose "Skipping $migration_id in $file — user restored a previously-disabled line" + return 0 + fi + + if [[ "${DB_DRY_RUN:-false}" == "true" ]]; then + while IFS= read -r line; do + db_log_info "Would disable ($migration_id) in $(basename "$file"): $line" + done <<< "$to_disable" + return 0 + fi + + _db_legacy_snapshot "$file" "$migration_id" "pre" + + local temp_file + temp_file=$(mktemp) + awk -v pattern="$grep_pattern" -v marker="$marker" ' + $0 ~ pattern && index($0, marker) != 1 { print marker $0; next } + { print } + ' "$file" > "$temp_file" + + # grep -E and awk's ERE dialect can disagree on backslash-escaped + # literals in a pattern passed through a shell variable (confirmed + # with nvm's source-line pattern). If grep found lines to disable + # but awk's rewrite is identical to the original, the pattern is + # dialect-mismatched — fail loudly instead of silently claiming + # success while leaving the file untouched. + if diff -q "$file" "$temp_file" >/dev/null 2>&1; then + rm -f "$temp_file" + db_log_error "legacy_shell ($migration_id): grep matched lines in $file but awk's rewrite changed nothing — pattern is grep/awk-dialect-mismatched, not applied" + return 1 + fi + + mv "$temp_file" "$file" + + db_log_success "Disabled redundant lines ($migration_id) in: $file" + + _db_legacy_snapshot "$file" "$migration_id" "post" +} + +# Moves `dir` aside into ~/.devboost/backups/ instead of deleting it. +# Idempotent: no-op if `dir` no longer exists (already archived). +_db_legacy_archive_dir() { + local dir="$1" migration_id="$2" + [[ -d "$dir" ]] || return 0 + + local backup_dir="${DB_BACKUP_DIR:-$HOME/.devboost/backups}" + local timestamp + timestamp=$(date +%Y%m%d_%H%M%S) + local dest="${backup_dir}/$(basename "$dir")-${migration_id}-${timestamp}" + + if [[ "${DB_DRY_RUN:-false}" == "true" ]]; then + db_log_info "Would archive: $dir -> $dest" + return 0 + fi + + db_ensure_dir "$backup_dir" + mv "$dir" "$dest" + db_log_success "Archived: $dir -> $dest" +} + +# Strips every devboost:disabled-marked line (any migration_id) from +# `file`, restoring nothing — this is real deletion, the opt-in step. +# Idempotent: no-op if no marked lines are present. +_db_legacy_clean_file() { + local file="$1" + [[ -f "$file" ]] || return 0 + + grep -qF "$_DB_LEGACY_MARKER_PREFIX" "$file" 2>/dev/null || return 0 + + if [[ "${DB_DRY_RUN:-false}" == "true" ]]; then + local count + count=$(grep -cF "$_DB_LEGACY_MARKER_PREFIX" "$file" 2>/dev/null) || count=0 + db_log_info "Would remove $count devboost-disabled line(s) from: $file" + return 0 + fi + + db_backup_file "$file" + local temp_file + temp_file=$(mktemp) + grep -vF "$_DB_LEGACY_MARKER_PREFIX" "$file" > "$temp_file" || true + mv "$temp_file" "$file" + db_log_success "Removed devboost-disabled lines from: $file" +} + +# List of files any legacy-shell migration might mark. Extend here if a +# future migration_id targets a different file. +_db_legacy_managed_files() { + echo "$HOME/.zshrc" + echo "$HOME/.zprofile" +} + +db_run_clean() { + db_log_info "Cleaning devboost-disabled lines..." + local f + while IFS= read -r f; do + _db_legacy_clean_file "$f" + done < <(_db_legacy_managed_files) + db_log_info "Archived directories remain under: ${DB_BACKUP_DIR:-$HOME/.devboost/backups} (remove that folder to purge everything)" +} + # Module registry system # Uses bash 3.x compatible approach (no associative arrays) @@ -624,7 +827,7 @@ db_run_doctor() { # Main entry point and CLI -DB_VERSION="1.3.0" +DB_VERSION="1.4.0" DB_SUBCOMMAND="apply" DB_DRY_RUN=false DB_VERBOSE=false @@ -635,7 +838,7 @@ DB_STATE_FILE="${HOME}/.devboost.state.json" db_parse_flags() { while [[ $# -gt 0 ]]; do case $1 in - apply|plan|doctor|uninstall|migrate-from-oh-my-zsh) + apply|plan|doctor|uninstall|clean|migrate-from-oh-my-zsh) DB_SUBCOMMAND="$1" shift ;; @@ -683,6 +886,7 @@ Commands: plan Show actions without changing anything doctor Check prerequisites, PATHs, shells, conflicting files uninstall Remove managed files/blocks (leaves user custom files untouched) + clean Remove devboost-disabled legacy-tooling lines and archived dirs migrate-from-oh-my-zsh Remove oh-my-zsh and recover .zshrc customizations (needs --yes) Options: @@ -699,7 +903,7 @@ EOF coreMain() { # Parse command first (before flags) local cmd="apply" - if [[ $# -gt 0 ]] && [[ "$1" =~ ^(apply|plan|doctor|uninstall|migrate-from-oh-my-zsh)$ ]]; then + if [[ $# -gt 0 ]] && [[ "$1" =~ ^(apply|plan|doctor|uninstall|clean|migrate-from-oh-my-zsh)$ ]]; then cmd="$1" shift fi @@ -754,6 +958,9 @@ coreMain() { uninstall) db_run_uninstall ;; + clean) + db_run_clean + ;; migrate-from-oh-my-zsh) db_run_migrate_from_oh_my_zsh ;; @@ -1074,9 +1281,14 @@ export EDITOR="nvim" export LANG="en_US.UTF-8" setopt HIST_IGNORE_ALL_DUPS HIST_REDUCE_BLANKS SHARE_HISTORY INC_APPEND_HISTORY -autoload -Uz compinit && compinit -u setopt AUTO_CD NO_BEEP +# znap owns completion init: it redefines compinit/compdef as no-ops and +# runs its own deferred, precmd-hook-based compinit after loading (see +# ~/.zsh-snap/scripts/init.zsh). Calling compinit here ourselves, before +# znap is sourced, would run a second full completion pass into a +# different dumpfile — pure redundant cost with no effect (znap's +# no-op override discards any completions we'd have registered anyway). # znap source "${znap_path}/znap.zsh" @@ -1216,6 +1428,165 @@ db_module_zsh_apply() { } +# Legacy shell tooling module +# +# Detects and safely retires shell tooling that duplicates what devboost's +# own modules already provide, when found in a pre-existing (non-devboost +# managed) ~/.zshrc or ~/.zprofile: +# - zinit loading the same plugins znap (module_znap.sh) already loads +# (zsh-autosuggestions, and a fast-syntax-highlighting/syntax-highlighting +# fork of the same feature) +# - asdf, alongside devboost's own mise (module_mise.sh) +# - nvm's shell hook (in ~/.zprofile, login-shell-only — measured at +# ~850-900ms per login shell via zprof, the dominant real-world startup +# cost found on the investigation machine), alongside devboost's own mise +# +# Redundant lines are commented out in place (see core/core_legacy_shell.sh) +# rather than deleted, so the user can review/undo by hand. Actual deletion +# of marked lines happens only via the separate `devboost clean` command. + +DB_LEGACY_ZINIT_ZNAP_ID="zinit-znap-dup" +DB_LEGACY_ASDF_MISE_ID="asdf-mise-dup" +DB_LEGACY_NVM_MISE_ID="nvm-mise-dup" + +# Matches zinit lines loading plugins znap's default config already loads. +DB_LEGACY_ZINIT_DUP_PATTERN='^[[:space:]]*zinit (light|load)[^#]*(zsh-users/zsh-autosuggestions|zdharma-continuum/fast-syntax-highlighting|zsh-users/zsh-syntax-highlighting)' + +# Matches the line sourcing asdf's shell integration. +DB_LEGACY_ASDF_SOURCE_PATTERN='(^|[[:space:]])\. .*/asdf\.sh([[:space:]]|$)' + +# Matches lines sourcing nvm's shell hook or its bash-completion shim +# (e.g. `[ -s ".../nvm.sh" ] && \. ".../nvm.sh"`). Deliberately does not +# match a plain `export NVM_DIR=...` line — that's a harmless variable, +# not the expensive part. Matches on the quoted path alone (not the +# leading `\.`/`source` token) since backslash-escaped literals in this +# pattern are interpreted differently by grep -E vs awk's ERE dialect +# when passed through a shell variable — see core_legacy_shell.sh's +# _db_legacy_disable_lines, which runs the same pattern string through +# both. Keep future patterns here grep/awk-dialect-safe for that reason. +DB_LEGACY_NVM_SOURCE_PATTERN='"[^"]*/nvm(\.sh|/etc/bash_completion\.d/nvm)"' + +db_module_legacy_shell_register() { + db_register_module "legacy_shell" \ + "db_module_legacy_shell_plan" \ + "db_module_legacy_shell_apply" \ + "db_module_legacy_shell_doctor" +} + +_db_legacy_shell_zshrc() { + db_yaml_get '.legacy_shell.zshrc' "$HOME/.zshrc" +} + +_db_legacy_shell_zprofile() { + db_yaml_get '.legacy_shell.zprofile' "$HOME/.zprofile" +} + +# True if `file` contains a live (unmarked) line matching `pattern` — +# i.e. one devboost hasn't already disabled for `migration_id`. A marker +# is prepended to, not a replacement of, the original line, so the +# pattern still matches inside an already-disabled line; excluding lines +# that start with the marker is what keeps doctor/plan from re-reporting +# something already fixed as still needing action. +_db_legacy_shell_unmarked_match_present() { + local file="$1" pattern="$2" migration_id="$3" + [[ -f "$file" ]] || return 1 + local marker="# devboost:disabled:${migration_id} " + grep -E "$pattern" "$file" 2>/dev/null | grep -qv "^${marker}" +} + +# True if zinit is loading a plugin that duplicates one of znap's. +_db_legacy_shell_zinit_dup_present() { + _db_legacy_shell_unmarked_match_present "$1" "$DB_LEGACY_ZINIT_DUP_PATTERN" "$DB_LEGACY_ZINIT_ZNAP_ID" +} + +# True if asdf is sourced (redundant with mise). +_db_legacy_shell_asdf_present() { + _db_legacy_shell_unmarked_match_present "$1" "$DB_LEGACY_ASDF_SOURCE_PATTERN" "$DB_LEGACY_ASDF_MISE_ID" +} + +# True if nvm's shell hook is sourced (redundant with mise). +_db_legacy_shell_nvm_present() { + _db_legacy_shell_unmarked_match_present "$1" "$DB_LEGACY_NVM_SOURCE_PATTERN" "$DB_LEGACY_NVM_MISE_ID" +} + +db_module_legacy_shell_doctor() { + local enable + enable=$(db_yaml_get '.legacy_shell.enable' 'true') + [[ "$enable" == "true" ]] || return 0 + + local zshrc + zshrc=$(_db_legacy_shell_zshrc) + + if _db_legacy_shell_zinit_dup_present "$zshrc"; then + db_log_warn "legacy_shell: zinit is loading plugin(s) that duplicate znap's — run 'devboost apply' to disable them" + else + db_log_success "legacy_shell: no zinit/znap plugin duplication found" + fi + + if _db_legacy_shell_asdf_present "$zshrc"; then + db_log_warn "legacy_shell: asdf is active alongside mise — run 'devboost apply' to disable it" + else + db_log_success "legacy_shell: no asdf/mise duplication found" + fi + + local zprofile + zprofile=$(_db_legacy_shell_zprofile) + + if _db_legacy_shell_nvm_present "$zprofile"; then + db_log_warn "legacy_shell: nvm's shell hook is active alongside mise (login-shell startup cost) — run 'devboost apply' to disable it" + else + db_log_success "legacy_shell: no nvm/mise duplication found" + fi +} + +db_module_legacy_shell_plan() { + local enable + enable=$(db_yaml_get '.legacy_shell.enable' 'true') + [[ "$enable" == "true" ]] || return 0 + + local zshrc + zshrc=$(_db_legacy_shell_zshrc) + + if _db_legacy_shell_zinit_dup_present "$zshrc"; then + db_log_info "Would disable redundant zinit plugin line(s) in: $zshrc" + fi + if _db_legacy_shell_asdf_present "$zshrc"; then + db_log_info "Would disable redundant asdf source line in: $zshrc" + fi + + local zprofile + zprofile=$(_db_legacy_shell_zprofile) + if _db_legacy_shell_nvm_present "$zprofile"; then + db_log_info "Would disable redundant nvm source line(s) in: $zprofile" + fi +} + +db_module_legacy_shell_apply() { + local enable + enable=$(db_yaml_get '.legacy_shell.enable' 'true') + if [[ "$enable" != "true" ]]; then + return 0 + fi + + local zshrc + zshrc=$(_db_legacy_shell_zshrc) + [[ -f "$zshrc" ]] || return 0 + + if _db_legacy_shell_zinit_dup_present "$zshrc"; then + _db_legacy_disable_lines "$zshrc" "$DB_LEGACY_ZINIT_DUP_PATTERN" "$DB_LEGACY_ZINIT_ZNAP_ID" + fi + + if _db_legacy_shell_asdf_present "$zshrc"; then + _db_legacy_disable_lines "$zshrc" "$DB_LEGACY_ASDF_SOURCE_PATTERN" "$DB_LEGACY_ASDF_MISE_ID" + fi + + local zprofile + zprofile=$(_db_legacy_shell_zprofile) + if [[ -f "$zprofile" ]] && _db_legacy_shell_nvm_present "$zprofile"; then + _db_legacy_disable_lines "$zprofile" "$DB_LEGACY_NVM_SOURCE_PATTERN" "$DB_LEGACY_NVM_MISE_ID" + fi +} + # Starship prompt module db_module_starship_register() { @@ -1835,6 +2206,7 @@ db_load_modules() { db_module_pkg_register db_module_znap_register db_module_zsh_register + db_module_legacy_shell_register db_module_starship_register db_module_tmux_register db_module_mise_register diff --git a/modules/module_legacy_shell.sh b/modules/module_legacy_shell.sh new file mode 100644 index 0000000..fc224e8 --- /dev/null +++ b/modules/module_legacy_shell.sh @@ -0,0 +1,158 @@ +# Legacy shell tooling module +# +# Detects and safely retires shell tooling that duplicates what devboost's +# own modules already provide, when found in a pre-existing (non-devboost +# managed) ~/.zshrc or ~/.zprofile: +# - zinit loading the same plugins znap (module_znap.sh) already loads +# (zsh-autosuggestions, and a fast-syntax-highlighting/syntax-highlighting +# fork of the same feature) +# - asdf, alongside devboost's own mise (module_mise.sh) +# - nvm's shell hook (in ~/.zprofile, login-shell-only — measured at +# ~850-900ms per login shell via zprof, the dominant real-world startup +# cost found on the investigation machine), alongside devboost's own mise +# +# Redundant lines are commented out in place (see core/core_legacy_shell.sh) +# rather than deleted, so the user can review/undo by hand. Actual deletion +# of marked lines happens only via the separate `devboost clean` command. + +DB_LEGACY_ZINIT_ZNAP_ID="zinit-znap-dup" +DB_LEGACY_ASDF_MISE_ID="asdf-mise-dup" +DB_LEGACY_NVM_MISE_ID="nvm-mise-dup" + +# Matches zinit lines loading plugins znap's default config already loads. +DB_LEGACY_ZINIT_DUP_PATTERN='^[[:space:]]*zinit (light|load)[^#]*(zsh-users/zsh-autosuggestions|zdharma-continuum/fast-syntax-highlighting|zsh-users/zsh-syntax-highlighting)' + +# Matches the line sourcing asdf's shell integration. +DB_LEGACY_ASDF_SOURCE_PATTERN='(^|[[:space:]])\. .*/asdf\.sh([[:space:]]|$)' + +# Matches lines sourcing nvm's shell hook or its bash-completion shim +# (e.g. `[ -s ".../nvm.sh" ] && \. ".../nvm.sh"`). Deliberately does not +# match a plain `export NVM_DIR=...` line — that's a harmless variable, +# not the expensive part. Matches on the quoted path alone (not the +# leading `\.`/`source` token) since backslash-escaped literals in this +# pattern are interpreted differently by grep -E vs awk's ERE dialect +# when passed through a shell variable — see core_legacy_shell.sh's +# _db_legacy_disable_lines, which runs the same pattern string through +# both. Keep future patterns here grep/awk-dialect-safe for that reason. +DB_LEGACY_NVM_SOURCE_PATTERN='"[^"]*/nvm(\.sh|/etc/bash_completion\.d/nvm)"' + +db_module_legacy_shell_register() { + db_register_module "legacy_shell" \ + "db_module_legacy_shell_plan" \ + "db_module_legacy_shell_apply" \ + "db_module_legacy_shell_doctor" +} + +_db_legacy_shell_zshrc() { + db_yaml_get '.legacy_shell.zshrc' "$HOME/.zshrc" +} + +_db_legacy_shell_zprofile() { + db_yaml_get '.legacy_shell.zprofile' "$HOME/.zprofile" +} + +# True if `file` contains a live (unmarked) line matching `pattern` — +# i.e. one devboost hasn't already disabled for `migration_id`. A marker +# is prepended to, not a replacement of, the original line, so the +# pattern still matches inside an already-disabled line; excluding lines +# that start with the marker is what keeps doctor/plan from re-reporting +# something already fixed as still needing action. +_db_legacy_shell_unmarked_match_present() { + local file="$1" pattern="$2" migration_id="$3" + [[ -f "$file" ]] || return 1 + local marker="# devboost:disabled:${migration_id} " + grep -E "$pattern" "$file" 2>/dev/null | grep -qv "^${marker}" +} + +# True if zinit is loading a plugin that duplicates one of znap's. +_db_legacy_shell_zinit_dup_present() { + _db_legacy_shell_unmarked_match_present "$1" "$DB_LEGACY_ZINIT_DUP_PATTERN" "$DB_LEGACY_ZINIT_ZNAP_ID" +} + +# True if asdf is sourced (redundant with mise). +_db_legacy_shell_asdf_present() { + _db_legacy_shell_unmarked_match_present "$1" "$DB_LEGACY_ASDF_SOURCE_PATTERN" "$DB_LEGACY_ASDF_MISE_ID" +} + +# True if nvm's shell hook is sourced (redundant with mise). +_db_legacy_shell_nvm_present() { + _db_legacy_shell_unmarked_match_present "$1" "$DB_LEGACY_NVM_SOURCE_PATTERN" "$DB_LEGACY_NVM_MISE_ID" +} + +db_module_legacy_shell_doctor() { + local enable + enable=$(db_yaml_get '.legacy_shell.enable' 'true') + [[ "$enable" == "true" ]] || return 0 + + local zshrc + zshrc=$(_db_legacy_shell_zshrc) + + if _db_legacy_shell_zinit_dup_present "$zshrc"; then + db_log_warn "legacy_shell: zinit is loading plugin(s) that duplicate znap's — run 'devboost apply' to disable them" + else + db_log_success "legacy_shell: no zinit/znap plugin duplication found" + fi + + if _db_legacy_shell_asdf_present "$zshrc"; then + db_log_warn "legacy_shell: asdf is active alongside mise — run 'devboost apply' to disable it" + else + db_log_success "legacy_shell: no asdf/mise duplication found" + fi + + local zprofile + zprofile=$(_db_legacy_shell_zprofile) + + if _db_legacy_shell_nvm_present "$zprofile"; then + db_log_warn "legacy_shell: nvm's shell hook is active alongside mise (login-shell startup cost) — run 'devboost apply' to disable it" + else + db_log_success "legacy_shell: no nvm/mise duplication found" + fi +} + +db_module_legacy_shell_plan() { + local enable + enable=$(db_yaml_get '.legacy_shell.enable' 'true') + [[ "$enable" == "true" ]] || return 0 + + local zshrc + zshrc=$(_db_legacy_shell_zshrc) + + if _db_legacy_shell_zinit_dup_present "$zshrc"; then + db_log_info "Would disable redundant zinit plugin line(s) in: $zshrc" + fi + if _db_legacy_shell_asdf_present "$zshrc"; then + db_log_info "Would disable redundant asdf source line in: $zshrc" + fi + + local zprofile + zprofile=$(_db_legacy_shell_zprofile) + if _db_legacy_shell_nvm_present "$zprofile"; then + db_log_info "Would disable redundant nvm source line(s) in: $zprofile" + fi +} + +db_module_legacy_shell_apply() { + local enable + enable=$(db_yaml_get '.legacy_shell.enable' 'true') + if [[ "$enable" != "true" ]]; then + return 0 + fi + + local zshrc + zshrc=$(_db_legacy_shell_zshrc) + [[ -f "$zshrc" ]] || return 0 + + if _db_legacy_shell_zinit_dup_present "$zshrc"; then + _db_legacy_disable_lines "$zshrc" "$DB_LEGACY_ZINIT_DUP_PATTERN" "$DB_LEGACY_ZINIT_ZNAP_ID" + fi + + if _db_legacy_shell_asdf_present "$zshrc"; then + _db_legacy_disable_lines "$zshrc" "$DB_LEGACY_ASDF_SOURCE_PATTERN" "$DB_LEGACY_ASDF_MISE_ID" + fi + + local zprofile + zprofile=$(_db_legacy_shell_zprofile) + if [[ -f "$zprofile" ]] && _db_legacy_shell_nvm_present "$zprofile"; then + _db_legacy_disable_lines "$zprofile" "$DB_LEGACY_NVM_SOURCE_PATTERN" "$DB_LEGACY_NVM_MISE_ID" + fi +} diff --git a/modules/module_zsh.sh b/modules/module_zsh.sh index 091a0e6..27a9b3a 100644 --- a/modules/module_zsh.sh +++ b/modules/module_zsh.sh @@ -74,9 +74,14 @@ export EDITOR="nvim" export LANG="en_US.UTF-8" setopt HIST_IGNORE_ALL_DUPS HIST_REDUCE_BLANKS SHARE_HISTORY INC_APPEND_HISTORY -autoload -Uz compinit && compinit -u setopt AUTO_CD NO_BEEP +# znap owns completion init: it redefines compinit/compdef as no-ops and +# runs its own deferred, precmd-hook-based compinit after loading (see +# ~/.zsh-snap/scripts/init.zsh). Calling compinit here ourselves, before +# znap is sourced, would run a second full completion pass into a +# different dumpfile — pure redundant cost with no effect (znap's +# no-op override discards any completions we'd have registered anyway). # znap source "${znap_path}/znap.zsh" diff --git a/tests/test-legacy-shell.sh b/tests/test-legacy-shell.sh new file mode 100755 index 0000000..b44bbad --- /dev/null +++ b/tests/test-legacy-shell.sh @@ -0,0 +1,381 @@ +#!/usr/bin/env bash +# Test devboost's legacy_shell module (zinit/znap, asdf/mise dedup) and the +# `clean` command, in a sandboxed environment. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" + +source "$SCRIPT_DIR/test_common.sh" + +cleanup() { + if [[ -n "${TEST_HOME:-}" ]] && [[ -d "$TEST_HOME" ]]; then + rm -rf "$TEST_HOME" + fi +} +trap cleanup EXIT + +test_suite_start "Legacy Shell Tooling + Clean Tests" + +if [[ ! -f "$PROJECT_ROOT/devboost.sh" ]]; then + echo -e "${RED}Error:${NC} devboost.sh not found. Run ./build.sh first." + exit 1 +fi + +# --- Test 1: clean .zshrc (no zinit, no asdf) -> apply is a no-op --- +TEST_HOME=$(mktemp -d) +cat > "$TEST_HOME/.zshrc" << 'EOF' +export EDITOR="nvim" +eval "$(mise activate zsh)" +EOF + +before=$(cat "$TEST_HOME/.zshrc") +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply >/dev/null 2>&1 || true +after=$(cat "$TEST_HOME/.zshrc") + +test_assert_contains \ + "Clean zshrc: no zinit/znap line disabled" \ + "$after" \ + 'export EDITOR="nvim"' + +test_assert_not_contains \ + "Clean zshrc: no devboost:disabled marker introduced" \ + "$after" \ + "devboost:disabled" + +rm -rf "$TEST_HOME" + +# --- Test 2: zinit+znap duplicate -> apply disables redundant zinit lines, idempotent --- +TEST_HOME=$(mktemp -d) +cat > "$TEST_HOME/.zshrc" << 'EOF' +source "$HOME/.local/share/zinit/zinit.git/zinit.zsh" +zinit light zdharma-continuum/fast-syntax-highlighting +zinit light zsh-users/zsh-autosuggestions +zinit light zsh-users/zsh-completions +EOF + +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply >/dev/null 2>&1 +result=$(cat "$TEST_HOME/.zshrc") + +test_assert_contains \ + "zinit dup: disables fast-syntax-highlighting line" \ + "$result" \ + "# devboost:disabled:zinit-znap-dup zinit light zdharma-continuum/fast-syntax-highlighting" + +test_assert_contains \ + "zinit dup: disables zsh-autosuggestions line" \ + "$result" \ + "# devboost:disabled:zinit-znap-dup zinit light zsh-users/zsh-autosuggestions" + +test_assert_contains \ + "zinit dup: leaves non-duplicate zsh-completions line active" \ + "$result" \ + "zinit light zsh-users/zsh-completions" + +test_assert_not_contains \ + "zinit dup: zsh-completions line itself not marked" \ + "$result" \ + "devboost:disabled:zinit-znap-dup zinit light zsh-users/zsh-completions" + +result_after_first=$result +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply >/dev/null 2>&1 +result_second=$(cat "$TEST_HOME/.zshrc") + +test_assert_eq \ + "zinit dup: second apply is idempotent (no double-marking)" \ + "$result_after_first" \ + "$result_second" + +rm -rf "$TEST_HOME" + +# --- Test 3: asdf+mise duplicate -> apply disables asdf source line, idempotent --- +TEST_HOME=$(mktemp -d) +cat > "$TEST_HOME/.zshrc" << 'EOF' +. /opt/homebrew/opt/asdf/libexec/asdf.sh +eval "$(mise activate zsh)" +EOF + +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply >/dev/null 2>&1 +result=$(cat "$TEST_HOME/.zshrc") + +test_assert_contains \ + "asdf dup: disables asdf source line" \ + "$result" \ + "# devboost:disabled:asdf-mise-dup . /opt/homebrew/opt/asdf/libexec/asdf.sh" + +test_assert_contains \ + "asdf dup: leaves mise line active" \ + "$result" \ + 'eval "$(mise activate zsh)"' + +result_after_first=$result +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply >/dev/null 2>&1 +result_second=$(cat "$TEST_HOME/.zshrc") + +test_assert_eq \ + "asdf dup: second apply is idempotent" \ + "$result_after_first" \ + "$result_second" + +rm -rf "$TEST_HOME" + +# --- Test 3b: nvm+mise duplicate (in ~/.zprofile) -> apply disables nvm source lines, idempotent --- +TEST_HOME=$(mktemp -d) +cat > "$TEST_HOME/.zshrc" << 'EOF' +eval "$(mise activate zsh)" +EOF +cat > "$TEST_HOME/.zprofile" << 'EOF' +eval "$(/opt/homebrew/bin/brew shellenv)" +export NVM_DIR="$HOME/.nvm" +[ -s "/opt/homebrew/opt/nvm/nvm.sh" ] && \. "/opt/homebrew/opt/nvm/nvm.sh" +[ -s "/opt/homebrew/opt/nvm/etc/bash_completion.d/nvm" ] && \. "/opt/homebrew/opt/nvm/etc/bash_completion.d/nvm" +EOF + +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply >/dev/null 2>&1 +result=$(cat "$TEST_HOME/.zprofile") + +test_assert_contains \ + "nvm dup: disables nvm.sh source line" \ + "$result" \ + '# devboost:disabled:nvm-mise-dup [ -s "/opt/homebrew/opt/nvm/nvm.sh" ]' + +test_assert_contains \ + "nvm dup: disables nvm bash_completion source line" \ + "$result" \ + '# devboost:disabled:nvm-mise-dup [ -s "/opt/homebrew/opt/nvm/etc/bash_completion.d/nvm" ]' + +test_assert_contains \ + "nvm dup: leaves NVM_DIR export untouched (harmless, not the expensive part)" \ + "$result" \ + 'export NVM_DIR="$HOME/.nvm"' + +test_assert_not_contains \ + "nvm dup: NVM_DIR line itself not marked" \ + "$result" \ + 'devboost:disabled:nvm-mise-dup export NVM_DIR' + +result_after_first=$result +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply >/dev/null 2>&1 +result_second=$(cat "$TEST_HOME/.zprofile") + +test_assert_eq \ + "nvm dup: second apply is idempotent" \ + "$result_after_first" \ + "$result_second" + +rm -rf "$TEST_HOME" + +# --- Test 4: user manually restores a disabled line -> next apply respects it --- +TEST_HOME=$(mktemp -d) +cat > "$TEST_HOME/.zshrc" << 'EOF' +zinit light zsh-users/zsh-autosuggestions +. /opt/homebrew/opt/asdf/libexec/asdf.sh +eval "$(mise activate zsh)" +EOF + +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply >/dev/null 2>&1 + +# User removes the marker prefix by hand, restoring the zinit line. +sed_inplace() { + if sed --version >/dev/null 2>&1; then + sed -i "$@" + else + sed -i '' "$@" + fi +} +sed_inplace 's/^# devboost:disabled:zinit-znap-dup //' "$TEST_HOME/.zshrc" + +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply >/dev/null 2>&1 +result=$(cat "$TEST_HOME/.zshrc") + +test_assert_contains \ + "Manual restore: zinit line stays restored (not re-disabled)" \ + "$result" \ + "zinit light zsh-users/zsh-autosuggestions" + +test_assert_not_contains \ + "Manual restore: no re-applied zinit marker" \ + "$result" \ + "devboost:disabled:zinit-znap-dup zinit light zsh-users/zsh-autosuggestions" + +test_assert_contains \ + "Manual restore: unrelated asdf line still disabled" \ + "$result" \ + "# devboost:disabled:asdf-mise-dup . /opt/homebrew/opt/asdf/libexec/asdf.sh" + +rm -rf "$TEST_HOME" + +# --- Test 5: clean strips marked lines; idempotent; works without prior apply in-process --- +TEST_HOME=$(mktemp -d) +cat > "$TEST_HOME/.zshrc" << 'EOF' +# devboost:disabled:zinit-znap-dup zinit light zsh-users/zsh-autosuggestions +# devboost:disabled:asdf-mise-dup . /opt/homebrew/opt/asdf/libexec/asdf.sh +eval "$(mise activate zsh)" +EOF +cat > "$TEST_HOME/.zprofile" << 'EOF' +export NVM_DIR="$HOME/.nvm" +# devboost:disabled:nvm-mise-dup [ -s "/opt/homebrew/opt/nvm/nvm.sh" ] && \. "/opt/homebrew/opt/nvm/nvm.sh" +EOF + +output=$(HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" clean 2>&1) && exit_code=0 || exit_code=$? +result=$(cat "$TEST_HOME/.zshrc") +result_zprofile=$(cat "$TEST_HOME/.zprofile") + +test_assert_eq \ + "clean: exits zero" \ + "0" \ + "$exit_code" + +test_assert_not_contains \ + "clean: removes zinit marker line entirely" \ + "$result" \ + "devboost:disabled:zinit-znap-dup" + +test_assert_not_contains \ + "clean: removes asdf marker line entirely" \ + "$result" \ + "devboost:disabled:asdf-mise-dup" + +test_assert_contains \ + "clean: leaves unrelated content untouched" \ + "$result" \ + 'eval "$(mise activate zsh)"' + +test_assert_not_contains \ + "clean: removes nvm marker line from .zprofile too" \ + "$result_zprofile" \ + "devboost:disabled:nvm-mise-dup" + +test_assert_contains \ + "clean: leaves .zprofile's NVM_DIR untouched" \ + "$result_zprofile" \ + 'export NVM_DIR="$HOME/.nvm"' + +result_after_first=$result +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" clean >/dev/null 2>&1 +result_second=$(cat "$TEST_HOME/.zshrc") + +test_assert_eq \ + "clean: idempotent on second run" \ + "$result_after_first" \ + "$result_second" + +rm -rf "$TEST_HOME" + +# --- Test 6: --dry-run makes no changes for apply and clean --- +TEST_HOME=$(mktemp -d) +cat > "$TEST_HOME/.zshrc" << 'EOF' +zinit light zsh-users/zsh-autosuggestions +. /opt/homebrew/opt/asdf/libexec/asdf.sh +EOF + +before=$(cat "$TEST_HOME/.zshrc") +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply --dry-run >/dev/null 2>&1 || true +after_apply=$(cat "$TEST_HOME/.zshrc") + +test_assert_eq \ + "Dry-run apply: .zshrc left untouched" \ + "$before" \ + "$after_apply" + +# Now actually mark it, then verify dry-run clean doesn't touch it. +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" apply >/dev/null 2>&1 +marked=$(cat "$TEST_HOME/.zshrc") +HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" clean --dry-run >/dev/null 2>&1 || true +after_clean=$(cat "$TEST_HOME/.zshrc") + +test_assert_eq \ + "Dry-run clean: .zshrc left untouched" \ + "$marked" \ + "$after_clean" + +rm -rf "$TEST_HOME" + +# --- Test 7: doctor reports conflicts without modifying any file --- +TEST_HOME=$(mktemp -d) +cat > "$TEST_HOME/.zshrc" << 'EOF' +zinit light zsh-users/zsh-autosuggestions +. /opt/homebrew/opt/asdf/libexec/asdf.sh +EOF +cat > "$TEST_HOME/.zprofile" << 'EOF' +[ -s "/opt/homebrew/opt/nvm/nvm.sh" ] && \. "/opt/homebrew/opt/nvm/nvm.sh" +EOF + +before=$(cat "$TEST_HOME/.zshrc") +before_zprofile=$(cat "$TEST_HOME/.zprofile") +output=$(HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" doctor 2>&1) || true +after=$(cat "$TEST_HOME/.zshrc") +after_zprofile=$(cat "$TEST_HOME/.zprofile") + +test_assert_contains \ + "doctor: reports zinit/znap duplication" \ + "$output" \ + "legacy_shell: zinit is loading plugin(s) that duplicate znap's" + +test_assert_contains \ + "doctor: reports asdf/mise duplication" \ + "$output" \ + "legacy_shell: asdf is active alongside mise" + +test_assert_contains \ + "doctor: reports nvm/mise duplication" \ + "$output" \ + "legacy_shell: nvm's shell hook is active alongside mise" + +test_assert_eq \ + "doctor: .zshrc left untouched" \ + "$before" \ + "$after" + +test_assert_eq \ + "doctor: .zprofile left untouched" \ + "$before_zprofile" \ + "$after_zprofile" + +rm -rf "$TEST_HOME" + +# --- Test 8: already-disabled lines must not be re-reported as still needing action --- +# Regression test: the marker prepends to, not replaces, the original line, so a +# naive "does this pattern appear anywhere in the file" check still matches inside +# an already-disabled line. doctor/plan must exclude marked lines specifically, not +# just detect the pattern's presence. (Found for real: after `apply` disabled the +# asdf line on the investigation machine, `doctor` kept warning "asdf is active" +# forever afterward.) +TEST_HOME=$(mktemp -d) +cat > "$TEST_HOME/.zshrc" << 'EOF' +# devboost:disabled:zinit-znap-dup zinit light zsh-users/zsh-autosuggestions +# devboost:disabled:asdf-mise-dup . /opt/homebrew/opt/asdf/libexec/asdf.sh +eval "$(mise activate zsh)" +EOF +cat > "$TEST_HOME/.zprofile" << 'EOF' +export NVM_DIR="$HOME/.nvm" +# devboost:disabled:nvm-mise-dup [ -s "/opt/homebrew/opt/nvm/nvm.sh" ] && \. "/opt/homebrew/opt/nvm/nvm.sh" +EOF + +doctor_output=$(HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" doctor 2>&1) || true +plan_output=$(HOME="$TEST_HOME" "$PROJECT_ROOT/devboost.sh" plan 2>&1) || true + +test_assert_contains \ + "Already-disabled: doctor reports zinit/znap clean, not still-warning" \ + "$doctor_output" \ + "legacy_shell: no zinit/znap plugin duplication found" + +test_assert_contains \ + "Already-disabled: doctor reports asdf/mise clean, not still-warning" \ + "$doctor_output" \ + "legacy_shell: no asdf/mise duplication found" + +test_assert_contains \ + "Already-disabled: doctor reports nvm/mise clean, not still-warning" \ + "$doctor_output" \ + "legacy_shell: no nvm/mise duplication found" + +test_assert_not_contains \ + "Already-disabled: plan says nothing more needs disabling" \ + "$plan_output" \ + "Would disable" + +rm -rf "$TEST_HOME" + +test_suite_end From 19847fa1a8b87055ef6e90196e0afa546251e96e Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 16:53:30 +0300 Subject: [PATCH 02/48] feat(engine): spike Go typed-resource engine against znap module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Proves the v2 architecture (typed resources, one diff function shared by plan/apply, Go struct literals instead of YAML) against the smallest real module: znap. plan and apply both call the same ComputeDiff, with dry-run being nothing more than "the caller that doesn't invoke the resulting ops" — no branching inside the diff logic itself, which is what actually closes the plan/apply-drift bug class the current bash module system has already hit in production. Introduces the first two resource kinds: DirExists (a parametrized, reusable diff-kind shorthand) and GitClone (composes it, shells out to git for the actual clone since git's own implementation is the correct primitive to use, not something to reimplement). Verified: real clone + idempotent second run, cross-compiles cleanly to macOS/Linux/Windows across amd64/arm64, and the existing bash tool and its full test suite are completely untouched. Existing bash tree stays authoritative until the full migration lands; this is the first proof of the mechanism, not a cutover. --- cmd/devboost-v2/main.go | 37 +++++++++++++ config/config.go | 110 ++++++++++++++++++++++++++++++++++++++ engine/apply.go | 28 ++++++++++ engine/kinds/directory.go | 35 ++++++++++++ engine/kinds/gitclone.go | 39 ++++++++++++++ engine/modules/znap.go | 27 ++++++++++ engine/plan.go | 19 +++++++ engine/resource.go | 53 ++++++++++++++++++ go.mod | 5 ++ go.sum | 2 + 10 files changed, 355 insertions(+) create mode 100644 cmd/devboost-v2/main.go create mode 100644 config/config.go create mode 100644 engine/apply.go create mode 100644 engine/kinds/directory.go create mode 100644 engine/kinds/gitclone.go create mode 100644 engine/modules/znap.go create mode 100644 engine/plan.go create mode 100644 engine/resource.go create mode 100644 go.mod create mode 100644 go.sum diff --git a/cmd/devboost-v2/main.go b/cmd/devboost-v2/main.go new file mode 100644 index 0000000..aef7338 --- /dev/null +++ b/cmd/devboost-v2/main.go @@ -0,0 +1,37 @@ +// Command devboost-v2 is the spike CLI proving the Go engine's plan/apply +// mechanism end to end against one ported module (znap). Not a full CLI +// replacement — see the v2 architecture proposal for the migration plan. +package main + +import ( + "fmt" + "os" + + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/modules" +) + +func main() { + if len(os.Args) < 2 { + fmt.Fprintln(os.Stderr, "usage: devboost-v2 plan|apply") + os.Exit(1) + } + + resources := modules.Znap() + + var err error + switch os.Args[1] { + case "plan": + err = engine.Plan(resources) + case "apply": + err = engine.Apply(resources) + default: + fmt.Fprintf(os.Stderr, "unknown command %q\n", os.Args[1]) + os.Exit(1) + } + + if err != nil { + fmt.Fprintln(os.Stderr, "error:", err) + os.Exit(1) + } +} diff --git a/config/config.go b/config/config.go new file mode 100644 index 0000000..62f1467 --- /dev/null +++ b/config/config.go @@ -0,0 +1,110 @@ +// Package config reads devboost's user-facing ~/.devboost.yaml. This stays +// YAML deliberately — it's a pre-existing, user-authored file, unrelated to +// the "module resource declarations are Go struct literals, not YAML" +// decision, which only concerns how devboost's own modules declare +// resources internally. +package config + +import ( + "os" + "path/filepath" + "strings" + + "go.yaml.in/yaml/v3" +) + +var loaded map[string]any +var loadErr error +var didLoad bool + +func path() string { + home, err := os.UserHomeDir() + if err != nil { + return ".devboost.yaml" + } + return filepath.Join(home, ".devboost.yaml") +} + +func load() { + didLoad = true + data, err := os.ReadFile(path()) + if err != nil { + if os.IsNotExist(err) { + loaded = map[string]any{} + return + } + loadErr = err + return + } + var m map[string]any + if err := yaml.Unmarshal(data, &m); err != nil { + loadErr = err + return + } + loaded = m +} + +// Get reads a dotted key path (e.g. "zsh.znap_path") from +// ~/.devboost.yaml, returning def if the file, key, or any intermediate +// segment is absent. It never errors on a missing file or key — only a +// malformed file is surfaced, via GetErr. +func Get(dottedKey string, def string) string { + return expandHome(get(dottedKey, def)) +} + +// get returns the raw (un-expanded) value, so expansion happens exactly +// once regardless of which return path was taken — including the default, +// since defaults like "~/.zsh-snap" need expansion too. +func get(dottedKey string, def string) string { + if !didLoad { + load() + } + if loadErr != nil { + return def + } + cur := any(loaded) + for _, part := range strings.Split(strings.Trim(dottedKey, "."), ".") { + m, ok := cur.(map[string]any) + if !ok { + return def + } + v, ok := m[part] + if !ok { + return def + } + cur = v + } + s, ok := cur.(string) + if !ok { + return def + } + return s +} + +// GetErr reports a parse error from the last Get call, if any (e.g. a +// malformed ~/.devboost.yaml). Callers that want to distinguish "using the +// default because nothing was configured" from "using the default because +// the config file is broken" should check this after calling Get. +func GetErr() error { + if !didLoad { + load() + } + return loadErr +} + +func expandHome(s string) string { + if s == "~" { + home, err := os.UserHomeDir() + if err == nil { + return home + } + return s + } + if len(s) >= 2 && s[0] == '~' && s[1] == '/' { + home, err := os.UserHomeDir() + if err == nil { + return filepath.Join(home, s[2:]) + } + } + return s +} diff --git a/engine/apply.go b/engine/apply.go new file mode 100644 index 0000000..6b579a0 --- /dev/null +++ b/engine/apply.go @@ -0,0 +1,28 @@ +package engine + +import "fmt" + +// Apply computes the same diff Plan does, then executes each pending +// operation. It calls the identical ComputeDiff — there is no separate +// "what apply would do" computation. +func Apply(resources []Resource) error { + ops, err := ComputeDiff(resources) + if err != nil { + return err + } + if len(ops) == 0 { + fmt.Println("No changes.") + return nil + } + for _, op := range ops { + fmt.Printf("%s...\n", op.Description) + if op.Execute == nil { + return fmt.Errorf("resource %s: pending op has no Execute", op.ResourceID) + } + if err := op.Execute(); err != nil { + return fmt.Errorf("resource %s: %w", op.ResourceID, err) + } + fmt.Printf("Done: %s\n", op.Description) + } + return nil +} diff --git a/engine/kinds/directory.go b/engine/kinds/directory.go new file mode 100644 index 0000000..bd993ec --- /dev/null +++ b/engine/kinds/directory.go @@ -0,0 +1,35 @@ +// Package kinds implements devboost's built-in resource kinds — the native +// Go diff/apply logic behind what modules declare as struct literals. +package kinds + +import ( + "fmt" + "os" + + "github.com/rolfsormo/devboost/engine" +) + +// DirExists is a built-in, parametrized diff-kind: "this directory should +// exist." It is the first candidate for the diff-kind shorthand devboost's +// architecture decided it wants — most modules needing "ensure a directory +// exists" declare this kind directly rather than writing a bespoke +// ResourceKind implementation. +type DirExists struct { + Path string +} + +func (d DirExists) Diff() (*engine.PendingOp, error) { + info, err := os.Stat(d.Path) + if err == nil && info.IsDir() { + return nil, nil + } + if err != nil && !os.IsNotExist(err) { + return nil, fmt.Errorf("stat %s: %w", d.Path, err) + } + return &engine.PendingOp{ + Description: fmt.Sprintf("create directory %s", d.Path), + Execute: func() error { + return os.MkdirAll(d.Path, 0o755) + }, + }, nil +} diff --git a/engine/kinds/gitclone.go b/engine/kinds/gitclone.go new file mode 100644 index 0000000..b26f659 --- /dev/null +++ b/engine/kinds/gitclone.go @@ -0,0 +1,39 @@ +package kinds + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + + "github.com/rolfsormo/devboost/engine" +) + +// GitClone declares "this URL should be cloned to this path." Its diff +// reuses DirExists (does Dest exist) rather than reimplementing that check — +// its Apply shells out to git clone itself, since git's own clone +// implementation is the correct primitive here, not something to +// reimplement in Go. +type GitClone struct { + URL string + Dest string +} + +func (g GitClone) Diff() (*engine.PendingOp, error) { + op, err := DirExists{Path: g.Dest}.Diff() + if err != nil || op == nil { + return op, err + } + return &engine.PendingOp{ + Description: fmt.Sprintf("git clone %s to %s", g.URL, g.Dest), + Execute: func() error { + if err := os.MkdirAll(filepath.Dir(g.Dest), 0o755); err != nil { + return err + } + cmd := exec.Command("git", "clone", "--depth", "1", g.URL, g.Dest) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + return cmd.Run() + }, + }, nil +} diff --git a/engine/modules/znap.go b/engine/modules/znap.go new file mode 100644 index 0000000..88c4e7f --- /dev/null +++ b/engine/modules/znap.go @@ -0,0 +1,27 @@ +// Package modules holds the Go-engine ports of devboost's bash modules. +// This package coexists with the original bash modules/*.sh tree during +// the v2 migration; it does not replace or modify any bash file. +package modules + +import ( + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +// Znap ports modules/module_znap.sh: ensure znap (the zsh plugin manager) +// is cloned to the configured path. Config is read lazily via a +// constructor, not a package-level var, since reading ~/.devboost.yaml at +// import time would make every consumer of this package touch the +// filesystem just by importing it. +func Znap() []engine.Resource { + return []engine.Resource{ + { + ID: "znap_install", + Kind: kinds.GitClone{ + URL: config.Get("zsh.znap_git", "https://github.com/marlonrichert/zsh-snap.git"), + Dest: config.Get("zsh.znap_path", "~/.zsh-snap"), + }, + }, + } +} diff --git a/engine/plan.go b/engine/plan.go new file mode 100644 index 0000000..3a4f179 --- /dev/null +++ b/engine/plan.go @@ -0,0 +1,19 @@ +package engine + +import "fmt" + +// Plan computes the diff and prints it without executing anything. +func Plan(resources []Resource) error { + ops, err := ComputeDiff(resources) + if err != nil { + return err + } + if len(ops) == 0 { + fmt.Println("No changes.") + return nil + } + for _, op := range ops { + fmt.Printf("Would: %s\n", op.Description) + } + return nil +} diff --git a/engine/resource.go b/engine/resource.go new file mode 100644 index 0000000..21d90a8 --- /dev/null +++ b/engine/resource.go @@ -0,0 +1,53 @@ +// Package engine implements devboost's typed-resource diff/apply model. +// +// A module declares desired state as a list of Resources. ComputeDiff is the +// single function that compares desired state to live system state and +// returns the pending operations needed to reconcile them. Both plan and +// apply call ComputeDiff identically — plan stops after printing the result, +// apply goes on to execute it. There is no dry-run flag threaded through the +// diffing logic anywhere: dry-run is purely "the caller that doesn't invoke +// PendingOp.Execute." +package engine + +import "fmt" + +// ResourceKind is implemented once per typed resource kind, in this package +// or a subpackage — never duplicated per module. A kind's own Diff is the +// only place its idempotency logic lives. +type ResourceKind interface { + // Diff reports whether current system state already matches desired + // state. A nil PendingOp means nothing needs to change. + Diff() (*PendingOp, error) +} + +// PendingOp is the output of a diff: the delta between desired and live +// state. It is never authored directly by a module — only ComputeDiff +// produces it. +type PendingOp struct { + ResourceID string + Description string + Execute func() error +} + +// Resource is what a module declares: an ID plus a kind carrying its own +// typed parameters. +type Resource struct { + ID string + Kind ResourceKind +} + +// ComputeDiff is the single function both plan and apply call. +func ComputeDiff(resources []Resource) ([]PendingOp, error) { + var ops []PendingOp + for _, r := range resources { + op, err := r.Kind.Diff() + if err != nil { + return nil, fmt.Errorf("resource %s: %w", r.ID, err) + } + if op != nil { + op.ResourceID = r.ID + ops = append(ops, *op) + } + } + return ops, nil +} diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..0ade321 --- /dev/null +++ b/go.mod @@ -0,0 +1,5 @@ +module github.com/rolfsormo/devboost + +go 1.23.12 + +require go.yaml.in/yaml/v3 v3.0.5 diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..8675e7f --- /dev/null +++ b/go.sum @@ -0,0 +1,2 @@ +go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= +go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= From ea40bdea40277af729d55e3319dabd1b3ad71a08 Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:01:08 +0300 Subject: [PATCH 03/48] feat(engine): dependency-ordered resources, testable config, real tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resources can now declare DependsOn, replacing bash's implicit, hand-maintained module ordering with an explicit graph (topoSort). Apply and plan traverse differently on purpose: plan diffs everything once, up front, since nothing has actually converged; apply diffs and executes one resource at a time in topological order, so a resource that depends on another sees its real post-execution effect, not a stale pre-execution snapshot — verified with a test that fails loudly if this regresses to a batch diff. Also replaces config's package-level global state (untestable, and the exact shape that produced the earlier ~-expansion bug — a default value skipped expansion because it took a different code path than a real config value) with a plain Config type callers load once and pass around. Added a permanent regression test for that bug. Every new resource kind and the config package now have real unit tests instead of relying solely on end-to-end manual runs. --- cmd/devboost-v2/main.go | 10 ++- config/config.go | 89 +++++++++------------- config/config_test.go | 129 +++++++++++++++++++++++++++++++ engine/apply.go | 19 ++--- engine/kinds/directory_test.go | 57 ++++++++++++++ engine/kinds/gitclone_test.go | 73 ++++++++++++++++++ engine/modules/znap.go | 15 ++-- engine/resource.go | 134 +++++++++++++++++++++++++++++---- engine/resource_test.go | 123 ++++++++++++++++++++++++++++++ 9 files changed, 564 insertions(+), 85 deletions(-) create mode 100644 config/config_test.go create mode 100644 engine/kinds/directory_test.go create mode 100644 engine/kinds/gitclone_test.go create mode 100644 engine/resource_test.go diff --git a/cmd/devboost-v2/main.go b/cmd/devboost-v2/main.go index aef7338..d8ab667 100644 --- a/cmd/devboost-v2/main.go +++ b/cmd/devboost-v2/main.go @@ -7,6 +7,7 @@ import ( "fmt" "os" + "github.com/rolfsormo/devboost/config" "github.com/rolfsormo/devboost/engine" "github.com/rolfsormo/devboost/engine/modules" ) @@ -17,9 +18,14 @@ func main() { os.Exit(1) } - resources := modules.Znap() + cfg, err := config.Load(config.DefaultPath()) + if err != nil { + fmt.Fprintln(os.Stderr, "error loading config:", err) + os.Exit(1) + } + + resources := modules.Znap(cfg) - var err error switch os.Args[1] { case "plan": err = engine.Plan(resources) diff --git a/config/config.go b/config/config.go index 62f1467..a3fd9e2 100644 --- a/config/config.go +++ b/config/config.go @@ -13,56 +13,54 @@ import ( "go.yaml.in/yaml/v3" ) -var loaded map[string]any -var loadErr error -var didLoad bool - -func path() string { - home, err := os.UserHomeDir() - if err != nil { - return ".devboost.yaml" - } - return filepath.Join(home, ".devboost.yaml") +// Config is a loaded ~/.devboost.yaml. Load it once at CLI startup and +// pass it to whatever needs it — no package-level global state, so tests +// can load an arbitrary fixture path without env-var tricks. +type Config struct { + data map[string]any } -func load() { - didLoad = true - data, err := os.ReadFile(path()) +// Load reads and parses the YAML file at path. A missing file is not an +// error — it's treated as an empty config, so every Get call falls back +// to its default. Only a malformed file (present but unparsable) errors. +func Load(path string) (*Config, error) { + data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { - loaded = map[string]any{} - return + return &Config{data: map[string]any{}}, nil } - loadErr = err - return + return nil, err } var m map[string]any if err := yaml.Unmarshal(data, &m); err != nil { - loadErr = err - return + return nil, err + } + if m == nil { + m = map[string]any{} + } + return &Config{data: m}, nil +} + +// DefaultPath returns ~/.devboost.yaml for the current user. +func DefaultPath() string { + home, err := os.UserHomeDir() + if err != nil { + return ".devboost.yaml" } - loaded = m + return filepath.Join(home, ".devboost.yaml") } -// Get reads a dotted key path (e.g. "zsh.znap_path") from -// ~/.devboost.yaml, returning def if the file, key, or any intermediate -// segment is absent. It never errors on a missing file or key — only a -// malformed file is surfaced, via GetErr. -func Get(dottedKey string, def string) string { - return expandHome(get(dottedKey, def)) +// Get reads a dotted key path (e.g. "zsh.znap_path"), returning def if +// the key or any intermediate segment is absent, or isn't a string. +// String values (both real ones and def itself) are expanded for a +// leading ~, since defaults like "~/.zsh-snap" need that too — expansion +// happens exactly once, regardless of which path returned the value. +func (c *Config) Get(dottedKey string, def string) string { + return expandHome(c.get(dottedKey, def)) } -// get returns the raw (un-expanded) value, so expansion happens exactly -// once regardless of which return path was taken — including the default, -// since defaults like "~/.zsh-snap" need expansion too. -func get(dottedKey string, def string) string { - if !didLoad { - load() - } - if loadErr != nil { - return def - } - cur := any(loaded) +func (c *Config) get(dottedKey string, def string) string { + cur := any(c.data) for _, part := range strings.Split(strings.Trim(dottedKey, "."), ".") { m, ok := cur.(map[string]any) if !ok { @@ -81,28 +79,15 @@ func get(dottedKey string, def string) string { return s } -// GetErr reports a parse error from the last Get call, if any (e.g. a -// malformed ~/.devboost.yaml). Callers that want to distinguish "using the -// default because nothing was configured" from "using the default because -// the config file is broken" should check this after calling Get. -func GetErr() error { - if !didLoad { - load() - } - return loadErr -} - func expandHome(s string) string { if s == "~" { - home, err := os.UserHomeDir() - if err == nil { + if home, err := os.UserHomeDir(); err == nil { return home } return s } if len(s) >= 2 && s[0] == '~' && s[1] == '/' { - home, err := os.UserHomeDir() - if err == nil { + if home, err := os.UserHomeDir(); err == nil { return filepath.Join(home, s[2:]) } } diff --git a/config/config_test.go b/config/config_test.go new file mode 100644 index 0000000..557ba57 --- /dev/null +++ b/config/config_test.go @@ -0,0 +1,129 @@ +package config + +import ( + "os" + "path/filepath" + "testing" +) + +func writeFixture(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), ".devboost.yaml") + if content != "" { + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + return path +} + +func TestLoadMissingFileYieldsEmptyConfig(t *testing.T) { + cfg, err := Load(filepath.Join(t.TempDir(), "does-not-exist.yaml")) + if err != nil { + t.Fatalf("missing file should not error: %v", err) + } + if got := cfg.Get("zsh.znap_path", "default"); got != "default" { + t.Fatalf("got %q, want default", got) + } +} + +func TestLoadMalformedFileErrors(t *testing.T) { + path := writeFixture(t, "zsh:\n - this is not valid: [") + if _, err := Load(path); err == nil { + t.Fatal("expected an error for malformed YAML") + } +} + +func TestGetReadsNestedKey(t *testing.T) { + path := writeFixture(t, "zsh:\n znap_path: /custom/path\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + if got := cfg.Get("zsh.znap_path", "default"); got != "/custom/path" { + t.Fatalf("got %q, want /custom/path", got) + } +} + +func TestGetFallsBackToDefaultWhenKeyAbsent(t *testing.T) { + path := writeFixture(t, "zsh:\n other_key: value\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + if got := cfg.Get("zsh.znap_path", "default"); got != "default" { + t.Fatalf("got %q, want default", got) + } +} + +func TestGetFallsBackToDefaultWhenIntermediateSegmentAbsent(t *testing.T) { + path := writeFixture(t, "other:\n key: value\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + if got := cfg.Get("zsh.znap_path", "default"); got != "default" { + t.Fatalf("got %q, want default", got) + } +} + +func TestGetFallsBackToDefaultWhenValueIsNotAString(t *testing.T) { + path := writeFixture(t, "zsh:\n znap_path:\n nested: true\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + if got := cfg.Get("zsh.znap_path", "default"); got != "default" { + t.Fatalf("got %q, want default", got) + } +} + +// TestGetExpandsHomeInDefault is a regression test for a real bug found +// during the znap spike: expansion only ran on values actually read from +// the file, not on the default — so a default like "~/.zsh-snap" with no +// config file present was passed through to git clone literally, +// including the tilde, which git happily "cloned into" as a real +// directory named "~". +func TestGetExpandsHomeInDefault(t *testing.T) { + cfg, err := Load(filepath.Join(t.TempDir(), "missing.yaml")) + if err != nil { + t.Fatal(err) + } + home, err := os.UserHomeDir() + if err != nil { + t.Skip("no home directory available in this environment") + } + got := cfg.Get("zsh.znap_path", "~/.zsh-snap") + want := filepath.Join(home, ".zsh-snap") + if got != want { + t.Fatalf("got %q, want %q (default was not tilde-expanded)", got, want) + } +} + +func TestGetExpandsHomeInConfiguredValue(t *testing.T) { + path := writeFixture(t, "zsh:\n znap_path: \"~/custom-znap\"\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + home, err := os.UserHomeDir() + if err != nil { + t.Skip("no home directory available in this environment") + } + got := cfg.Get("zsh.znap_path", "default") + want := filepath.Join(home, "custom-znap") + if got != want { + t.Fatalf("got %q, want %q", got, want) + } +} + +func TestGetLeavesNonTildeValuesUntouched(t *testing.T) { + path := writeFixture(t, "zsh:\n znap_path: /absolute/path\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + if got := cfg.Get("zsh.znap_path", "default"); got != "/absolute/path" { + t.Fatalf("got %q, want /absolute/path", got) + } +} diff --git a/engine/apply.go b/engine/apply.go index 6b579a0..d2189c4 100644 --- a/engine/apply.go +++ b/engine/apply.go @@ -2,11 +2,15 @@ package engine import "fmt" -// Apply computes the same diff Plan does, then executes each pending -// operation. It calls the identical ComputeDiff — there is no separate -// "what apply would do" computation. +// Apply diffs and executes resources one at a time, in dependency order — +// see the package doc for why this can't be "compute the same diff Plan +// computes, then execute it": a resource that depends on another must be +// diffed after that dependency has actually executed, not against a +// stale, pre-execution view of the system. func Apply(resources []Resource) error { - ops, err := ComputeDiff(resources) + ops, err := DiffAndExecute(resources, func(op PendingOp) { + fmt.Printf("%s...\n", op.Description) + }) if err != nil { return err } @@ -15,13 +19,6 @@ func Apply(resources []Resource) error { return nil } for _, op := range ops { - fmt.Printf("%s...\n", op.Description) - if op.Execute == nil { - return fmt.Errorf("resource %s: pending op has no Execute", op.ResourceID) - } - if err := op.Execute(); err != nil { - return fmt.Errorf("resource %s: %w", op.ResourceID, err) - } fmt.Printf("Done: %s\n", op.Description) } return nil diff --git a/engine/kinds/directory_test.go b/engine/kinds/directory_test.go new file mode 100644 index 0000000..d7dba48 --- /dev/null +++ b/engine/kinds/directory_test.go @@ -0,0 +1,57 @@ +package kinds + +import ( + "os" + "path/filepath" + "testing" +) + +func TestDirExistsDiffPendingWhenAbsent(t *testing.T) { + dir := filepath.Join(t.TempDir(), "sub", "target") + op, err := DirExists{Path: dir}.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op for an absent directory") + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + info, err := os.Stat(dir) + if err != nil || !info.IsDir() { + t.Fatalf("expected %s to be a directory after execute", dir) + } +} + +func TestDirExistsDiffNilWhenPresent(t *testing.T) { + dir := t.TempDir() + op, err := DirExists{Path: dir}.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected no pending op for an existing directory, got %+v", op) + } +} + +func TestDirExistsErrorsWhenPathIsAFile(t *testing.T) { + file := filepath.Join(t.TempDir(), "not-a-dir") + if err := os.WriteFile(file, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + // A file exists at the path but isn't a directory: Diff should report + // it as pending (create-directory would fail at Execute, which is the + // correct place for that failure to surface — Diff itself shouldn't + // silently treat "a file is here" as "the directory exists"). + op, err := DirExists{Path: file}.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op when a non-directory occupies the path") + } + if err := op.Execute(); err == nil { + t.Fatal("expected Execute to fail when a file occupies the target path") + } +} diff --git a/engine/kinds/gitclone_test.go b/engine/kinds/gitclone_test.go new file mode 100644 index 0000000..a999b8c --- /dev/null +++ b/engine/kinds/gitclone_test.go @@ -0,0 +1,73 @@ +package kinds + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +// newLocalRepo creates a tiny local git repo to clone from, so the test +// doesn't depend on network access. +func newLocalRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + run := func(args ...string) { + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@example.com", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + run("init", "-q") + if err := os.WriteFile(filepath.Join(dir, "f"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + run("add", "f") + run("commit", "-q", "-m", "init") + return dir +} + +func TestGitCloneDiffPendingWhenAbsent(t *testing.T) { + src := newLocalRepo(t) + dest := filepath.Join(t.TempDir(), "dest") + + op, err := GitClone{URL: src, Dest: dest}.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op when dest doesn't exist") + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + if _, err := os.Stat(filepath.Join(dest, "f")); err != nil { + t.Fatalf("expected cloned file to exist: %v", err) + } +} + +func TestGitCloneDiffNilWhenAlreadyCloned(t *testing.T) { + src := newLocalRepo(t) + dest := filepath.Join(t.TempDir(), "dest") + + op, err := GitClone{URL: src, Dest: dest}.Diff() + if err != nil || op == nil { + t.Fatalf("setup: expected a pending op, got op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("setup execute: %v", err) + } + + op, err = GitClone{URL: src, Dest: dest}.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected no pending op once already cloned, got %+v", op) + } +} diff --git a/engine/modules/znap.go b/engine/modules/znap.go index 88c4e7f..6d050db 100644 --- a/engine/modules/znap.go +++ b/engine/modules/znap.go @@ -10,17 +10,18 @@ import ( ) // Znap ports modules/module_znap.sh: ensure znap (the zsh plugin manager) -// is cloned to the configured path. Config is read lazily via a -// constructor, not a package-level var, since reading ~/.devboost.yaml at -// import time would make every consumer of this package touch the -// filesystem just by importing it. -func Znap() []engine.Resource { +// is cloned to the configured path. Every module constructor in this +// package takes the already-loaded *config.Config rather than reading +// ~/.devboost.yaml itself — the CLI entry point loads it once, and +// passing it explicitly keeps modules testable against an arbitrary +// fixture config instead of real files/env vars. +func Znap(cfg *config.Config) []engine.Resource { return []engine.Resource{ { ID: "znap_install", Kind: kinds.GitClone{ - URL: config.Get("zsh.znap_git", "https://github.com/marlonrichert/zsh-snap.git"), - Dest: config.Get("zsh.znap_path", "~/.zsh-snap"), + URL: cfg.Get("zsh.znap_git", "https://github.com/marlonrichert/zsh-snap.git"), + Dest: cfg.Get("zsh.znap_path", "~/.zsh-snap"), }, }, } diff --git a/engine/resource.go b/engine/resource.go index 21d90a8..818a044 100644 --- a/engine/resource.go +++ b/engine/resource.go @@ -1,12 +1,21 @@ // Package engine implements devboost's typed-resource diff/apply model. // -// A module declares desired state as a list of Resources. ComputeDiff is the -// single function that compares desired state to live system state and -// returns the pending operations needed to reconcile them. Both plan and -// apply call ComputeDiff identically — plan stops after printing the result, -// apply goes on to execute it. There is no dry-run flag threaded through the -// diffing logic anywhere: dry-run is purely "the caller that doesn't invoke -// PendingOp.Execute." +// A module declares desired state as a list of Resources. Resources may +// declare dependencies on other resources (DependsOn), replacing what used +// to be an implicit, hand-maintained ordering in build.sh's registration +// list with an explicit, checkable graph — e.g. starship declares it +// depends on the pkg resource that installs the starship binary, tmux's +// plugin-install step depends on TPM's clone, zsh's rendered config +// depends on znap being installed. +// +// Plan and apply both traverse resources in topological order, but they +// diff differently: Plan diffs every resource once, up front, since +// nothing is actually converging as it goes — the printed result describes +// what a real run would do. Apply cannot do that in one batch: if resource +// B depends on resource A, B's diff must run *after* A's Execute, or B +// would diff against pre-A state (e.g. checking whether a binary exists +// before the resource that installs it has actually run). So Apply +// diffs-then-executes one resource at a time, in dependency order. package engine import "fmt" @@ -21,7 +30,7 @@ type ResourceKind interface { } // PendingOp is the output of a diff: the delta between desired and live -// state. It is never authored directly by a module — only ComputeDiff +// state. It is never authored directly by a module — only a Diff call // produces it. type PendingOp struct { ResourceID string @@ -30,16 +39,25 @@ type PendingOp struct { } // Resource is what a module declares: an ID plus a kind carrying its own -// typed parameters. +// typed parameters, plus any other resources (by ID) that must converge +// before this one is diffed. type Resource struct { - ID string - Kind ResourceKind + ID string + Kind ResourceKind + DependsOn []string } -// ComputeDiff is the single function both plan and apply call. +// ComputeDiff diffs every resource once, in topological order, without +// executing anything — this is what Plan uses, since nothing needs to +// have actually converged for a description of "what would happen" to be +// accurate. func ComputeDiff(resources []Resource) ([]PendingOp, error) { + ordered, err := topoSort(resources) + if err != nil { + return nil, err + } var ops []PendingOp - for _, r := range resources { + for _, r := range ordered { op, err := r.Kind.Diff() if err != nil { return nil, fmt.Errorf("resource %s: %w", r.ID, err) @@ -51,3 +69,93 @@ func ComputeDiff(resources []Resource) ([]PendingOp, error) { } return ops, nil } + +// DiffAndExecute walks resources in topological order, diffing and (if +// there's a pending change) immediately executing each one before moving +// on — so a later resource's diff always sees the real effect of an +// earlier resource it depends on, not a stale pre-execution view. before +// is called on each PendingOp right before it's executed, letting the +// caller report progress; pass nil to skip reporting. Returns every +// PendingOp that was executed. +func DiffAndExecute(resources []Resource, before func(PendingOp)) ([]PendingOp, error) { + ordered, err := topoSort(resources) + if err != nil { + return nil, err + } + var ops []PendingOp + for _, r := range ordered { + op, err := r.Kind.Diff() + if err != nil { + return nil, fmt.Errorf("resource %s: %w", r.ID, err) + } + if op == nil { + continue + } + op.ResourceID = r.ID + if op.Execute == nil { + return nil, fmt.Errorf("resource %s: pending op has no Execute", r.ID) + } + if before != nil { + before(*op) + } + if err := op.Execute(); err != nil { + return nil, fmt.Errorf("resource %s: %w", r.ID, err) + } + ops = append(ops, *op) + } + return ops, nil +} + +// topoSort orders resources so that every resource comes after everything +// it DependsOn. Returns an error on an unknown dependency ID or a cycle. +func topoSort(resources []Resource) ([]Resource, error) { + byID := make(map[string]Resource, len(resources)) + for _, r := range resources { + if _, dup := byID[r.ID]; dup { + return nil, fmt.Errorf("duplicate resource ID %q", r.ID) + } + byID[r.ID] = r + } + for _, r := range resources { + for _, dep := range r.DependsOn { + if _, ok := byID[dep]; !ok { + return nil, fmt.Errorf("resource %s: depends on unknown resource %q", r.ID, dep) + } + } + } + + // state[id] is absent (zero value, unvisited) / visiting / visited. + const ( + visiting = 1 + visited = 2 + ) + state := make(map[string]int, len(resources)) + var ordered []Resource + + var visit func(id string) error + visit = func(id string) error { + switch state[id] { + case visited: + return nil + case visiting: + return fmt.Errorf("dependency cycle involving resource %q", id) + } + state[id] = visiting + r := byID[id] + for _, dep := range r.DependsOn { + if err := visit(dep); err != nil { + return err + } + } + state[id] = visited + ordered = append(ordered, r) + return nil + } + + for _, r := range resources { + if err := visit(r.ID); err != nil { + return nil, err + } + } + return ordered, nil +} diff --git a/engine/resource_test.go b/engine/resource_test.go new file mode 100644 index 0000000..5a1f535 --- /dev/null +++ b/engine/resource_test.go @@ -0,0 +1,123 @@ +package engine + +import ( + "strings" + "testing" +) + +type fakeKind struct { + pending bool + desc string + ran *[]string + id string +} + +func (f fakeKind) Diff() (*PendingOp, error) { + if !f.pending { + return nil, nil + } + ran := f.ran + id := f.id + return &PendingOp{ + Description: f.desc, + Execute: func() error { + *ran = append(*ran, id) + return nil + }, + }, nil +} + +func TestTopoSortOrdersDependenciesFirst(t *testing.T) { + resources := []Resource{ + {ID: "c", Kind: fakeKind{}, DependsOn: []string{"b"}}, + {ID: "a", Kind: fakeKind{}}, + {ID: "b", Kind: fakeKind{}, DependsOn: []string{"a"}}, + } + ordered, err := topoSort(resources) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + var ids []string + for _, r := range ordered { + ids = append(ids, r.ID) + } + want := "a b c" + if got := strings.Join(ids, " "); got != want { + t.Fatalf("order = %q, want %q", got, want) + } +} + +func TestTopoSortDetectsCycle(t *testing.T) { + resources := []Resource{ + {ID: "a", Kind: fakeKind{}, DependsOn: []string{"b"}}, + {ID: "b", Kind: fakeKind{}, DependsOn: []string{"a"}}, + } + if _, err := topoSort(resources); err == nil { + t.Fatal("expected a cycle error, got nil") + } +} + +func TestTopoSortDetectsUnknownDependency(t *testing.T) { + resources := []Resource{ + {ID: "a", Kind: fakeKind{}, DependsOn: []string{"missing"}}, + } + if _, err := topoSort(resources); err == nil { + t.Fatal("expected an unknown-dependency error, got nil") + } +} + +func TestTopoSortDetectsDuplicateID(t *testing.T) { + resources := []Resource{ + {ID: "a", Kind: fakeKind{}}, + {ID: "a", Kind: fakeKind{}}, + } + if _, err := topoSort(resources); err == nil { + t.Fatal("expected a duplicate-ID error, got nil") + } +} + +// TestDiffAndExecuteSeesDependencyEffects is the load-bearing test for the +// whole reason Apply can't reuse ComputeDiff's batch diff: resource b's +// Diff must observe the real effect of resource a's Execute, not a +// pre-execution snapshot. +func TestDiffAndExecuteSeesDependencyEffects(t *testing.T) { + var ran []string + + a := Resource{ + ID: "a", + Kind: fakeKind{ + pending: true, + desc: "converge a", + ran: &ran, + id: "a", + }, + } + // b's Diff asserts that a has already executed by the time it's + // called — simulating "b depends on state a's Execute establishes". + b := Resource{ + ID: "b", + DependsOn: []string{"a"}, + Kind: dynamicKind{ + diff: func() (*PendingOp, error) { + if len(ran) == 0 { + t.Fatal("b was diffed before a executed") + } + return nil, nil + }, + }, + } + + _, err := DiffAndExecute([]Resource{b, a}, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(ran) != 1 || ran[0] != "a" { + t.Fatalf("expected a to have executed, ran = %v", ran) + } +} + +type dynamicKind struct { + diff func() (*PendingOp, error) +} + +func (d dynamicKind) Diff() (*PendingOp, error) { return d.diff() } From 2d80613e8676156b13070b9f0b9a6d3c4627302c Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:12:15 +0300 Subject: [PATCH 04/48] feat(kinds): File, BlockInFile, GitConfig, Package, LineInFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five more typed resource kinds, ported faithfully from the bash tool's core helpers and per-module logic: - File: byte-for-byte content diff, backs up before overwrite by default (ports db_write_file/db_backup_file). - BlockInFile: create/append/replace-between-markers (ports db_upsert_block's awk logic). - GitConfig: shells to git config --get/--set, since git's own config parsing is the correct primitive, not something to reimplement. - Package: per-OS provider backends (brew/apt/dnf/pacman) behind one resource kind, matching the architecture doc's "providers are swappable behind one resource type" model. Ports db_install_packages and _db_pkg_map's name-mapping table, including Homebrew self-bootstrap and apt-get update-once-per-run. - LineInFile: the mechanism behind devboost's redundant-tooling dedup (zinit/znap, asdf/mise, nvm/mise) — marks matching lines disabled in place rather than deleting them, and respects a user manually restoring one by hand as an explicit override. Ports core_legacy_shell.sh's marker/snapshot/restore-detection logic. Unlike the bash version, there's no grep/awk dialect-mismatch risk to guard against — Go's regexp is the only engine used for both detection and rewriting. All five have real unit tests, including a regression test for the exact manual-restore behavior the dedup mechanism depends on. --- engine/kinds/backup.go | 64 ++++++++++ engine/kinds/blockinfile.go | 104 +++++++++++++++ engine/kinds/blockinfile_test.go | 119 ++++++++++++++++++ engine/kinds/file.go | 57 +++++++++ engine/kinds/file_test.go | 90 +++++++++++++ engine/kinds/gitconfig.go | 56 +++++++++ engine/kinds/gitconfig_test.go | 122 ++++++++++++++++++ engine/kinds/lineinfile.go | 196 +++++++++++++++++++++++++++++ engine/kinds/lineinfile_test.go | 145 +++++++++++++++++++++ engine/kinds/os.go | 41 ++++++ engine/kinds/package.go | 209 +++++++++++++++++++++++++++++++ 11 files changed, 1203 insertions(+) create mode 100644 engine/kinds/backup.go create mode 100644 engine/kinds/blockinfile.go create mode 100644 engine/kinds/blockinfile_test.go create mode 100644 engine/kinds/file.go create mode 100644 engine/kinds/file_test.go create mode 100644 engine/kinds/gitconfig.go create mode 100644 engine/kinds/gitconfig_test.go create mode 100644 engine/kinds/lineinfile.go create mode 100644 engine/kinds/lineinfile_test.go create mode 100644 engine/kinds/os.go create mode 100644 engine/kinds/package.go diff --git a/engine/kinds/backup.go b/engine/kinds/backup.go new file mode 100644 index 0000000..c389124 --- /dev/null +++ b/engine/kinds/backup.go @@ -0,0 +1,64 @@ +package kinds + +import ( + "fmt" + "io" + "os" + "path/filepath" + "time" +) + +// backupDir returns ~/.devboost/backups, matching the bash tool's default +// (DB_BACKUP_DIR). No env-var override yet — the bash version's +// --config-adjacent DB_BACKUP_DIR override isn't wired through here since +// nothing in the Go CLI surface sets it yet; add one if/when that's needed. +func backupDir() (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + return filepath.Join(home, ".devboost", "backups"), nil +} + +// backupFile copies path into a fresh timestamped subdirectory of the +// backup dir before it's about to be overwritten, mirroring the bash +// tool's db_backup_file. A no-op if path doesn't exist yet (nothing to +// back up). +func backupFile(path string) error { + if _, err := os.Stat(path); os.IsNotExist(err) { + return nil + } else if err != nil { + return err + } + + dir, err := backupDir() + if err != nil { + return err + } + dest := filepath.Join(dir, time.Now().Format("20060102_150405")) + if err := os.MkdirAll(dest, 0o755); err != nil { + return err + } + + src, err := os.Open(path) + if err != nil { + return err + } + defer src.Close() + + info, err := src.Stat() + if err != nil { + return err + } + + out, err := os.OpenFile(filepath.Join(dest, filepath.Base(path)), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, info.Mode()) + if err != nil { + return err + } + defer out.Close() + + if _, err := io.Copy(out, src); err != nil { + return fmt.Errorf("backup %s: %w", path, err) + } + return nil +} diff --git a/engine/kinds/blockinfile.go b/engine/kinds/blockinfile.go new file mode 100644 index 0000000..f0df3a9 --- /dev/null +++ b/engine/kinds/blockinfile.go @@ -0,0 +1,104 @@ +package kinds + +import ( + "bufio" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/rolfsormo/devboost/engine" +) + +// BlockInFile declares "this marked block should be present in this file +// with this content," porting the bash tool's db_upsert_block. Three +// cases: the file doesn't exist yet (create it with just the block); the +// start marker is already present (replace everything between the +// markers); neither (append the block to the end of the file). +type BlockInFile struct { + Path string + StartMarker string + EndMarker string + Content string +} + +func (b BlockInFile) block() string { + return b.StartMarker + "\n" + b.Content + "\n" + b.EndMarker + "\n" +} + +func (b BlockInFile) Diff() (*engine.PendingOp, error) { + data, err := os.ReadFile(b.Path) + if os.IsNotExist(err) { + return &engine.PendingOp{ + Description: fmt.Sprintf("create %s with managed block", b.Path), + Execute: func() error { + if err := os.MkdirAll(filepath.Dir(b.Path), 0o755); err != nil { + return err + } + return os.WriteFile(b.Path, []byte(b.block()), 0o644) + }, + }, nil + } + if err != nil { + return nil, fmt.Errorf("read %s: %w", b.Path, err) + } + + current := string(data) + desired := replaceOrAppendBlock(current, b.StartMarker, b.EndMarker, b.block()) + if desired == current { + return nil, nil + } + + verb := "update block in" + if !strings.Contains(current, b.StartMarker) { + verb = "append block to" + } + return &engine.PendingOp{ + Description: fmt.Sprintf("%s %s", verb, b.Path), + Execute: func() error { + if err := backupFile(b.Path); err != nil { + return err + } + return os.WriteFile(b.Path, []byte(desired), 0o644) + }, + }, nil +} + +// replaceOrAppendBlock ports db_upsert_block's awk logic line-by-line: if +// startMarker is found, everything from that line through the endMarker +// line (inclusive) is replaced with block; otherwise block is appended +// (preceded by a blank line, matching the bash version). +func replaceOrAppendBlock(content, startMarker, endMarker, block string) string { + if !strings.Contains(content, startMarker) { + if content == "" { + return block + } + sep := "\n" + if strings.HasSuffix(content, "\n") { + sep = "" + } + return content + sep + "\n" + block + } + + var out strings.Builder + inBlock := false + scanner := bufio.NewScanner(strings.NewReader(content)) + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) + for scanner.Scan() { + line := scanner.Text() + if !inBlock && strings.Contains(line, startMarker) { + inBlock = true + out.WriteString(block) + continue + } + if inBlock && strings.Contains(line, endMarker) { + inBlock = false + continue + } + if !inBlock { + out.WriteString(line) + out.WriteString("\n") + } + } + return out.String() +} diff --git a/engine/kinds/blockinfile_test.go b/engine/kinds/blockinfile_test.go new file mode 100644 index 0000000..5434dcd --- /dev/null +++ b/engine/kinds/blockinfile_test.go @@ -0,0 +1,119 @@ +package kinds + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +const start = "# >>> devboost test start" +const end = "# <<< devboost test end" + +func TestBlockInFileDiffCreatesFileWhenAbsent(t *testing.T) { + path := filepath.Join(t.TempDir(), "sub", "f") + b := BlockInFile{Path: path, StartMarker: start, EndMarker: end, Content: "hello"} + op, err := b.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op for an absent file") + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(data), "hello") { + t.Fatalf("expected content to contain block content, got %q", data) + } +} + +func TestBlockInFileDiffAppendsWhenMarkerAbsent(t *testing.T) { + path := filepath.Join(t.TempDir(), "f") + if err := os.WriteFile(path, []byte("existing content\n"), 0o644); err != nil { + t.Fatal(err) + } + b := BlockInFile{Path: path, StartMarker: start, EndMarker: end, Content: "new"} + op, err := b.Diff() + if err != nil || op == nil { + t.Fatalf("op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + data, _ := os.ReadFile(path) + if !strings.Contains(string(data), "existing content") || !strings.Contains(string(data), "new") { + t.Fatalf("expected both existing content and new block, got %q", data) + } +} + +func TestBlockInFileDiffReplacesExistingBlock(t *testing.T) { + path := filepath.Join(t.TempDir(), "f") + initial := "before\n" + start + "\nold content\n" + end + "\nafter\n" + if err := os.WriteFile(path, []byte(initial), 0o644); err != nil { + t.Fatal(err) + } + b := BlockInFile{Path: path, StartMarker: start, EndMarker: end, Content: "new content"} + op, err := b.Diff() + if err != nil || op == nil { + t.Fatalf("op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + data, _ := os.ReadFile(path) + got := string(data) + if strings.Contains(got, "old content") { + t.Fatalf("expected old content to be replaced, got %q", got) + } + if !strings.Contains(got, "new content") || !strings.Contains(got, "before") || !strings.Contains(got, "after") { + t.Fatalf("expected new content preserved alongside before/after, got %q", got) + } +} + +func TestBlockInFileDiffNilWhenAlreadyCorrect(t *testing.T) { + path := filepath.Join(t.TempDir(), "f") + b := BlockInFile{Path: path, StartMarker: start, EndMarker: end, Content: "hello"} + op, err := b.Diff() + if err != nil || op == nil { + t.Fatalf("setup: op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("setup execute: %v", err) + } + + op, err = b.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected no pending op once already correct, got %+v", op) + } +} + +func TestBlockInFileDiffIdempotentAfterAppend(t *testing.T) { + path := filepath.Join(t.TempDir(), "f") + if err := os.WriteFile(path, []byte("existing\n"), 0o644); err != nil { + t.Fatal(err) + } + b := BlockInFile{Path: path, StartMarker: start, EndMarker: end, Content: "block"} + op, err := b.Diff() + if err != nil || op == nil { + t.Fatalf("setup: op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("setup execute: %v", err) + } + + op, err = b.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected idempotent no-op on second diff, got %+v", op) + } +} diff --git a/engine/kinds/file.go b/engine/kinds/file.go new file mode 100644 index 0000000..8993432 --- /dev/null +++ b/engine/kinds/file.go @@ -0,0 +1,57 @@ +package kinds + +import ( + "fmt" + "os" + "path/filepath" + + "github.com/rolfsormo/devboost/engine" +) + +// File declares "this file should exist with this exact content." Its +// diff is byte-for-byte content comparison — any difference (including a +// missing file) is pending. Mode defaults to 0644 if unset. Backup +// defaults to true (matching the bash tool's db_write_file, which always +// backs up before overwriting). +type File struct { + Path string + Content string + Mode os.FileMode + NoBackup bool // set true to skip the pre-write backup +} + +func (f File) mode() os.FileMode { + if f.Mode == 0 { + return 0o644 + } + return f.Mode +} + +func (f File) Diff() (*engine.PendingOp, error) { + current, err := os.ReadFile(f.Path) + if err == nil && string(current) == f.Content { + return nil, nil + } + if err != nil && !os.IsNotExist(err) { + return nil, fmt.Errorf("read %s: %w", f.Path, err) + } + + verb := "write" + if err == nil { + verb = "update" + } + return &engine.PendingOp{ + Description: fmt.Sprintf("%s %s", verb, f.Path), + Execute: func() error { + if !f.NoBackup { + if err := backupFile(f.Path); err != nil { + return err + } + } + if err := os.MkdirAll(filepath.Dir(f.Path), 0o755); err != nil { + return err + } + return os.WriteFile(f.Path, []byte(f.Content), f.mode()) + }, + }, nil +} diff --git a/engine/kinds/file_test.go b/engine/kinds/file_test.go new file mode 100644 index 0000000..628e238 --- /dev/null +++ b/engine/kinds/file_test.go @@ -0,0 +1,90 @@ +package kinds + +import ( + "os" + "path/filepath" + "testing" +) + +func TestFileDiffPendingWhenAbsent(t *testing.T) { + path := filepath.Join(t.TempDir(), "sub", "f") + f := File{Path: path, Content: "hello\n", NoBackup: true} + op, err := f.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op for an absent file") + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + data, err := os.ReadFile(path) + if err != nil || string(data) != "hello\n" { + t.Fatalf("got %q, err %v; want %q", data, err, "hello\n") + } +} + +func TestFileDiffNilWhenContentMatches(t *testing.T) { + path := filepath.Join(t.TempDir(), "f") + if err := os.WriteFile(path, []byte("same\n"), 0o644); err != nil { + t.Fatal(err) + } + f := File{Path: path, Content: "same\n"} + op, err := f.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected no pending op when content matches, got %+v", op) + } +} + +func TestFileDiffPendingWhenContentDiffers(t *testing.T) { + path := filepath.Join(t.TempDir(), "f") + if err := os.WriteFile(path, []byte("old\n"), 0o644); err != nil { + t.Fatal(err) + } + f := File{Path: path, Content: "new\n", NoBackup: true} + op, err := f.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op when content differs") + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + data, _ := os.ReadFile(path) + if string(data) != "new\n" { + t.Fatalf("got %q, want %q", data, "new\n") + } +} + +func TestFileExecuteBacksUpByDefault(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + path := filepath.Join(home, "target") + if err := os.WriteFile(path, []byte("old\n"), 0o644); err != nil { + t.Fatal(err) + } + f := File{Path: path, Content: "new\n"} // NoBackup left false + op, err := f.Diff() + if err != nil || op == nil { + t.Fatalf("setup: op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + + dir, err := backupDir() + if err != nil { + t.Fatal(err) + } + entries, err := os.ReadDir(dir) + if err != nil || len(entries) == 0 { + t.Fatalf("expected at least one backup entry under %s, err=%v", dir, err) + } +} diff --git a/engine/kinds/gitconfig.go b/engine/kinds/gitconfig.go new file mode 100644 index 0000000..0d98a79 --- /dev/null +++ b/engine/kinds/gitconfig.go @@ -0,0 +1,56 @@ +package kinds + +import ( + "fmt" + "os/exec" + "strings" + + "github.com/rolfsormo/devboost/engine" +) + +// GitConfig declares "this git config key should have this value" at the +// given scope. Both the read (diff) and write (apply) shell out to git +// config itself — git's own config format/parsing is the correct +// primitive here, not something to reimplement. +type GitConfig struct { + Key string + Value string + // Scope is a git config scope flag, e.g. "--global" (the bash tool's + // only current use) or "--local". Defaults to "--global" if empty. + Scope string +} + +func (g GitConfig) scope() string { + if g.Scope == "" { + return "--global" + } + return g.Scope +} + +func (g GitConfig) current() (string, bool, error) { + out, err := exec.Command("git", "config", g.scope(), "--get", g.Key).Output() + if err != nil { + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { + // git config --get exits 1 when the key isn't set — not an error. + return "", false, nil + } + return "", false, fmt.Errorf("git config --get %s: %w", g.Key, err) + } + return strings.TrimSuffix(string(out), "\n"), true, nil +} + +func (g GitConfig) Diff() (*engine.PendingOp, error) { + current, set, err := g.current() + if err != nil { + return nil, err + } + if set && current == g.Value { + return nil, nil + } + return &engine.PendingOp{ + Description: fmt.Sprintf("git config %s %s %s", g.scope(), g.Key, g.Value), + Execute: func() error { + return exec.Command("git", "config", g.scope(), g.Key, g.Value).Run() + }, + }, nil +} diff --git a/engine/kinds/gitconfig_test.go b/engine/kinds/gitconfig_test.go new file mode 100644 index 0000000..99ba9d3 --- /dev/null +++ b/engine/kinds/gitconfig_test.go @@ -0,0 +1,122 @@ +package kinds + +import ( + "os" + "os/exec" + "testing" +) + +// newTestRepo creates a bare local git repo dir and returns a function +// that runs a command with its cwd set there — tests use --local scope +// against this repo, never --global, so they can't touch the real +// developer's git config. +func newTestRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + cmd := exec.Command("git", "init", "-q") + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git init: %v\n%s", err, out) + } + return dir +} + +// runIn shells to git config with --local against dir, matching what +// GitConfig itself does but from a fixed working directory (GitConfig has +// no explicit dir/cwd field — see the note on TestGitConfigDiff below for +// how the tests work around that). +func gitConfigLocal(t *testing.T, dir string, args ...string) (string, error) { + t.Helper() + cmd := exec.Command("git", append([]string{"config"}, args...)...) + cmd.Dir = dir + out, err := cmd.Output() + return string(out), err +} + +func TestGitConfigDiffPendingWhenUnset(t *testing.T) { + dir := newTestRepo(t) + oldwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer os.Chdir(oldwd) + if err := os.Chdir(dir); err != nil { + t.Fatal(err) + } + + g := GitConfig{Key: "core.pager", Value: "delta", Scope: "--local"} + op, err := g.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op for an unset key") + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + + got, err := gitConfigLocal(t, dir, "--local", "--get", "core.pager") + if err != nil { + t.Fatalf("expected core.pager to be set: %v", err) + } + if got != "delta\n" { + t.Fatalf("got %q, want %q", got, "delta\n") + } +} + +func TestGitConfigDiffNilWhenAlreadyCorrect(t *testing.T) { + dir := newTestRepo(t) + oldwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer os.Chdir(oldwd) + if err := os.Chdir(dir); err != nil { + t.Fatal(err) + } + + g := GitConfig{Key: "core.pager", Value: "delta", Scope: "--local"} + op, err := g.Diff() + if err != nil || op == nil { + t.Fatalf("setup: op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("setup execute: %v", err) + } + + op, err = g.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected no pending op once already correct, got %+v", op) + } +} + +func TestGitConfigDiffPendingWhenValueDiffers(t *testing.T) { + dir := newTestRepo(t) + oldwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer os.Chdir(oldwd) + if err := os.Chdir(dir); err != nil { + t.Fatal(err) + } + + first := GitConfig{Key: "core.pager", Value: "less", Scope: "--local"} + op, _ := first.Diff() + if op != nil { + _ = op.Execute() + } + + changed := GitConfig{Key: "core.pager", Value: "delta", Scope: "--local"} + op, err = changed.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op when the configured value differs") + } +} diff --git a/engine/kinds/lineinfile.go b/engine/kinds/lineinfile.go new file mode 100644 index 0000000..6594147 --- /dev/null +++ b/engine/kinds/lineinfile.go @@ -0,0 +1,196 @@ +package kinds + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "strings" + "time" + + "github.com/rolfsormo/devboost/engine" +) + +// markerPrefix and markerFor mirror the bash tool's +// _DB_LEGACY_MARKER_PREFIX/_db_legacy_marker_for exactly, so a file marked +// by the bash version and later processed by this Go version (or vice +// versa, during the migration period) is read identically by both. +const markerPrefix = "# devboost:disabled:" + +func markerFor(migrationID string) string { + return markerPrefix + migrationID + " " +} + +// LineInFile declares "lines matching Pattern in this file should be +// disabled (commented out with a devboost:disabled marker), unless the +// user has manually restored one by hand." This is the mechanism behind +// devboost's redundant-tooling dedup modules (zinit/znap, asdf/mise, +// nvm/mise) — never deletes a line, always leaves it reviewable and +// reversible, and respects a user peeling the marker off by hand as an +// explicit override not to be undone by a later run. +// +// Unlike the bash version (core_legacy_shell.sh), there is no grep/awk +// dialect-mismatch risk here: Go's regexp package is the only engine used +// for both detecting matches and rewriting them, so the bash version's +// explicit dialect-mismatch detection/failure path has no equivalent +// failure mode to guard against. +type LineInFile struct { + Path string + Pattern string // an unanchored regexp, matched against each line + MigrationID string +} + +func (l LineInFile) re() (*regexp.Regexp, error) { + return regexp.Compile(l.Pattern) +} + +// wasEverMarked reports whether the most recent post- +// snapshot of Path contains this pattern's marker — i.e. whether a line +// was ever actually disabled here before, which combined with an +// unmarked live match means the user peeled the marker off by hand. +func (l LineInFile) wasEverMarked() (bool, error) { + dir, err := backupDir() + if err != nil { + return false, err + } + matches, err := latestSnapshotsMatching(dir, filepath.Base(l.Path)+".post-"+l.MigrationID+"-") + if err != nil || matches == "" { + return false, err + } + data, err := os.ReadFile(matches) + if err != nil { + return false, err + } + return strings.Contains(string(data), markerFor(l.MigrationID)), nil +} + +func (l LineInFile) Diff() (*engine.PendingOp, error) { + re, err := l.re() + if err != nil { + return nil, fmt.Errorf("compile pattern %q: %w", l.Pattern, err) + } + + data, err := os.ReadFile(l.Path) + if os.IsNotExist(err) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("read %s: %w", l.Path, err) + } + + marker := markerFor(l.MigrationID) + lines := splitLines(string(data)) + var toDisable []int + for i, line := range lines { + if re.MatchString(line) && !strings.HasPrefix(line, marker) { + toDisable = append(toDisable, i) + } + } + if len(toDisable) == 0 { + return nil, nil + } + + restored, err := l.wasEverMarked() + if err != nil { + return nil, err + } + if restored { + // A previously-marked line is now live and unmarked: the user + // restored it by hand. Respect that — don't re-disable. + return nil, nil + } + + return &engine.PendingOp{ + Description: fmt.Sprintf("disable %d redundant line(s) (%s) in %s", len(toDisable), l.MigrationID, l.Path), + Execute: func() error { + if err := snapshot(l.Path, l.MigrationID, "pre"); err != nil { + return err + } + disabled := make(map[int]bool, len(toDisable)) + for _, i := range toDisable { + disabled[i] = true + } + out := make([]string, len(lines)) + for i, line := range lines { + if disabled[i] { + out[i] = marker + line + } else { + out[i] = line + } + } + if err := os.WriteFile(l.Path, []byte(strings.Join(out, "\n")+trailingNewline(string(data))), 0o644); err != nil { + return err + } + return snapshot(l.Path, l.MigrationID, "post") + }, + }, nil +} + +func splitLines(s string) []string { + s = strings.TrimSuffix(s, "\n") + if s == "" { + return nil + } + return strings.Split(s, "\n") +} + +func trailingNewline(s string) string { + if strings.HasSuffix(s, "\n") { + return "\n" + } + return "" +} + +// latestSnapshotsMatching returns the newest file in dir whose name has +// the given prefix, or "" if none exist. +func latestSnapshotsMatching(dir, prefix string) (string, error) { + entries, err := os.ReadDir(dir) + if os.IsNotExist(err) { + return "", nil + } + if err != nil { + return "", err + } + var latest string + var latestMod int64 + for _, e := range entries { + if e.IsDir() || !strings.HasPrefix(e.Name(), prefix) { + continue + } + info, err := e.Info() + if err != nil { + continue + } + if mod := info.ModTime().UnixNano(); mod > latestMod { + latestMod = mod + latest = filepath.Join(dir, e.Name()) + } + } + return latest, nil +} + +// snapshot mirrors _db_legacy_snapshot: a hand-named backup copy with a +// caller-chosen phase suffix, distinct from the plain backupFile helper +// (which db_backup_file's fixed one-arg signature required in bash but +// isn't a constraint here — kept as a separate function since it names +// its backups differently, matching the bash source it ports). +func snapshot(path, migrationID, phase string) error { + if _, err := os.Stat(path); os.IsNotExist(err) { + return nil + } else if err != nil { + return err + } + dir, err := backupDir() + if err != nil { + return err + } + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + name := fmt.Sprintf("%s.%s-%s-%s", filepath.Base(path), phase, migrationID, time.Now().Format("20060102_150405")) + data, err := os.ReadFile(path) + if err != nil { + return err + } + return os.WriteFile(filepath.Join(dir, name), data, 0o644) +} diff --git a/engine/kinds/lineinfile_test.go b/engine/kinds/lineinfile_test.go new file mode 100644 index 0000000..5267687 --- /dev/null +++ b/engine/kinds/lineinfile_test.go @@ -0,0 +1,145 @@ +package kinds + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func writeLines(t *testing.T, path string, lines ...string) { + t.Helper() + if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0o644); err != nil { + t.Fatal(err) + } +} + +func TestLineInFileDiffNilWhenAbsent(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, "missing") + l := LineInFile{Path: path, Pattern: "zinit", MigrationID: "test"} + op, err := l.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected no pending op for a missing file, got %+v", op) + } +} + +func TestLineInFileDiffPendingWhenUnmarkedMatchExists(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".zshrc") + writeLines(t, path, "zinit light zsh-users/zsh-autosuggestions", "eval \"$(mise activate zsh)\"") + + l := LineInFile{Path: path, Pattern: "zinit light", MigrationID: "zinit-znap-dup"} + op, err := l.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op for an unmarked matching line") + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + + data, _ := os.ReadFile(path) + got := string(data) + if !strings.Contains(got, markerFor("zinit-znap-dup")+"zinit light") { + t.Fatalf("expected disabled line to carry the marker, got %q", got) + } + if !strings.Contains(got, "mise activate zsh") { + t.Fatalf("expected unrelated line to remain untouched, got %q", got) + } +} + +func TestLineInFileDiffNilOnceMarked(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".zshrc") + writeLines(t, path, "zinit light zsh-users/zsh-autosuggestions") + + l := LineInFile{Path: path, Pattern: "zinit light", MigrationID: "zinit-znap-dup"} + op, err := l.Diff() + if err != nil || op == nil { + t.Fatalf("setup: op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("setup execute: %v", err) + } + + op, err = l.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected no pending op once marked, got %+v", op) + } +} + +// TestLineInFileRespectsManualRestore is the load-bearing test for the +// mechanism's whole point: a user peeling the marker off a previously +// disabled line by hand is an explicit override that later runs must not +// undo. +func TestLineInFileRespectsManualRestore(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".zshrc") + writeLines(t, path, "zinit light zsh-users/zsh-autosuggestions") + + l := LineInFile{Path: path, Pattern: "zinit light", MigrationID: "zinit-znap-dup"} + op, err := l.Diff() + if err != nil || op == nil { + t.Fatalf("setup: op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("setup execute: %v", err) + } + + // User manually removes the marker prefix, restoring the line. + data, _ := os.ReadFile(path) + restored := strings.ReplaceAll(string(data), markerFor("zinit-znap-dup"), "") + if err := os.WriteFile(path, []byte(restored), 0o644); err != nil { + t.Fatal(err) + } + + op, err = l.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected the manual restore to be respected (no re-disable), got %+v", op) + } +} + +func TestLineInFileLeavesNonMatchingLinesAlone(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".zshrc") + writeLines(t, path, + "zinit light zsh-users/zsh-autosuggestions", + "zinit light zsh-users/zsh-completions", + ) + + // Only the autosuggestions line duplicates znap; completions doesn't. + l := LineInFile{Path: path, Pattern: "zsh-autosuggestions", MigrationID: "zinit-znap-dup"} + op, err := l.Diff() + if err != nil || op == nil { + t.Fatalf("op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + + data, _ := os.ReadFile(path) + got := string(data) + if strings.Contains(got, markerFor("zinit-znap-dup")+"zinit light zsh-users/zsh-completions") { + t.Fatalf("expected the non-matching completions line to stay unmarked, got %q", got) + } + if !strings.Contains(got, "\nzinit light zsh-users/zsh-completions") && !strings.HasPrefix(got, "zinit light zsh-users/zsh-completions") { + t.Fatalf("expected the non-matching line to remain present, got %q", got) + } +} diff --git a/engine/kinds/os.go b/engine/kinds/os.go new file mode 100644 index 0000000..926329a --- /dev/null +++ b/engine/kinds/os.go @@ -0,0 +1,41 @@ +package kinds + +import ( + "os" + "runtime" +) + +// OS identifies the target platform, mirroring the bash tool's DB_OS +// values exactly (darwin, linux-ubuntu, linux-fedora, linux-arch, other). +type OS string + +const ( + OSDarwin OS = "darwin" + OSLinuxUbuntu OS = "linux-ubuntu" + OSLinuxFedora OS = "linux-fedora" + OSLinuxArch OS = "linux-arch" + OSOther OS = "other" +) + +// DetectOS ports db_detect_os: darwin via GOOS, Linux distro via the same +// marker files the bash version checks. +func DetectOS() OS { + if runtime.GOOS == "darwin" { + return OSDarwin + } + if fileExists("/etc/debian_version") { + return OSLinuxUbuntu + } + if fileExists("/etc/fedora-release") { + return OSLinuxFedora + } + if fileExists("/etc/arch-release") { + return OSLinuxArch + } + return OSOther +} + +func fileExists(path string) bool { + _, err := os.Stat(path) + return err == nil +} diff --git a/engine/kinds/package.go b/engine/kinds/package.go new file mode 100644 index 0000000..056f8d4 --- /dev/null +++ b/engine/kinds/package.go @@ -0,0 +1,209 @@ +package kinds + +import ( + "fmt" + "os/exec" + "strings" + "sync" + + "github.com/rolfsormo/devboost/engine" +) + +// packageNameOverrides ports _db_pkg_map: almost every package name is +// identical across package managers, so only the exceptions are listed +// (fd is packaged as fd-find on Debian/Ubuntu and Fedora) rather than the +// bash version's full per-OS case statement repeating every identity +// mapping. Anything not listed here maps to itself. +var packageNameOverrides = map[OS]map[string]string{ + OSLinuxUbuntu: {"fd": "fd-find"}, + OSLinuxFedora: {"fd": "fd-find"}, +} + +func mapPackageName(os OS, name string) string { + if overrides, ok := packageNameOverrides[os]; ok { + if mapped, ok := overrides[name]; ok { + return mapped + } + } + return name +} + +// packageProvider is the per-OS backend behind Package: how to check +// whether a package is already installed, and how to install it. This is +// the "provider" abstraction from the architecture doc — brew, apt, dnf, +// pacman are swappable backends behind one resource kind, the same way +// Terraform's AWS/GCP providers are swappable behind one resource type. +type packageProvider interface { + installed(name string) (bool, error) + install(name string) error +} + +func providerFor(os OS) (packageProvider, error) { + switch os { + case OSDarwin: + return brewProvider{}, nil + case OSLinuxUbuntu: + return aptProvider{}, nil + case OSLinuxFedora: + return dnfProvider{}, nil + case OSLinuxArch: + return pacmanProvider{}, nil + default: + return nil, fmt.Errorf("unsupported OS: %s", os) + } +} + +type brewProvider struct{} + +func (brewProvider) installed(name string) (bool, error) { + if err := ensureBrewInstalled(); err != nil { + return false, err + } + err := exec.Command("brew", "list", name).Run() + return err == nil, nil +} + +func (brewProvider) install(name string) error { + out, err := exec.Command("brew", "install", name).CombinedOutput() + if err != nil { + return fmt.Errorf("brew install %s: %w\n%s", name, err, out) + } + return nil +} + +// ensureBrewInstalled ports db_install_packages' darwin branch: if brew +// itself isn't present, bootstrap it via Homebrew's own official +// installer before anything else can proceed. +func ensureBrewInstalled() error { + if err := exec.Command("brew", "--version").Run(); err == nil { + return nil + } + cmd := exec.Command("/bin/bash", "-c", + `$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)`) + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("install Homebrew: %w\n%s", err, out) + } + return nil +} + +type aptProvider struct{} + +func (aptProvider) installed(name string) (bool, error) { + out, err := exec.Command("dpkg", "-l").Output() + if err != nil { + return false, fmt.Errorf("dpkg -l: %w", err) + } + for _, line := range strings.Split(string(out), "\n") { + fields := strings.Fields(line) + if len(fields) >= 2 && fields[0] == "ii" && fields[1] == name { + return true, nil + } + } + return false, nil +} + +var aptUpdateOnce sync.Once +var aptUpdateErr error + +func (aptProvider) install(name string) error { + // apt-get update runs once per process, immediately before the first + // real install — not on every installed() check — matching the bash + // tool's behavior of updating the index once per apply run. + aptUpdateOnce.Do(func() { + out, err := exec.Command("sudo", "apt-get", "update", "-qq").CombinedOutput() + if err != nil { + aptUpdateErr = fmt.Errorf("apt-get update: %w\n%s", err, out) + } + }) + if aptUpdateErr != nil { + return aptUpdateErr + } + + out, err := exec.Command("sudo", "apt-get", "install", "-y", name).CombinedOutput() + if err != nil { + return fmt.Errorf("apt-get install %s: %w\n%s", name, err, out) + } + return nil +} + +type dnfProvider struct{} + +func (dnfProvider) installed(name string) (bool, error) { + err := exec.Command("rpm", "-q", name).Run() + return err == nil, nil +} + +func (dnfProvider) install(name string) error { + out, err := exec.Command("sudo", "dnf", "install", "-y", name).CombinedOutput() + if err != nil { + return fmt.Errorf("dnf install %s: %w\n%s", name, err, out) + } + return nil +} + +type pacmanProvider struct{} + +func (pacmanProvider) installed(name string) (bool, error) { + err := exec.Command("pacman", "-Qi", name).Run() + return err == nil, nil +} + +func (pacmanProvider) install(name string) error { + out, err := exec.Command("sudo", "pacman", "-S", "--noconfirm", name).CombinedOutput() + if err != nil { + return fmt.Errorf("pacman -S %s: %w\n%s", name, err, out) + } + return nil +} + +// Package declares "these packages should be installed," mapped through +// the OS's own package-name conventions (fd -> fd-find on Debian/Fedora, +// etc.) and installed via the OS's own package manager. OS defaults to +// DetectOS() if unset — set it explicitly only in tests. +type Package struct { + Names []string + OS OS +} + +func (p Package) targetOS() OS { + if p.OS == "" { + return DetectOS() + } + return p.OS +} + +func (p Package) Diff() (*engine.PendingOp, error) { + os := p.targetOS() + provider, err := providerFor(os) + if err != nil { + return nil, err + } + + var missing []string + for _, name := range p.Names { + mapped := mapPackageName(os, name) + ok, err := provider.installed(mapped) + if err != nil { + return nil, fmt.Errorf("check %s installed: %w", mapped, err) + } + if !ok { + missing = append(missing, mapped) + } + } + if len(missing) == 0 { + return nil, nil + } + + return &engine.PendingOp{ + Description: fmt.Sprintf("install packages: %s", strings.Join(missing, " ")), + Execute: func() error { + for _, name := range missing { + if err := provider.install(name); err != nil { + return err + } + } + return nil + }, + }, nil +} From a1ea9f1729d709bee29a325be3acca282f8fafc0 Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:13:16 +0300 Subject: [PATCH 05/48] feat(kinds): CommandGuarded escape hatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The architecture's one deliberate escape hatch for state that doesn't fit File/Package/GitConfig/etc. A module still only ever declares data (ID, Wants) — never imperative logic at the declaration site. What keeps this from becoming a loophole: CommandGuarded{ID: "x"} does nothing by itself. ID must match a real Go implementation hand-registered in core via RegisterCommand; an unregistered ID errors loudly rather than silently no-opping. There is no generic "run a script, check the exit code" shortcut — adding a new use costs the same real Go work as adding a proper typed kind, which is the point: this must never be the easy path when a real kind is achievable. --- engine/kinds/commandguarded.go | 67 +++++++++++++++++++++++++++++ engine/kinds/commandguarded_test.go | 54 +++++++++++++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 engine/kinds/commandguarded.go create mode 100644 engine/kinds/commandguarded_test.go diff --git a/engine/kinds/commandguarded.go b/engine/kinds/commandguarded.go new file mode 100644 index 0000000..1950b32 --- /dev/null +++ b/engine/kinds/commandguarded.go @@ -0,0 +1,67 @@ +package kinds + +import ( + "fmt" + + "github.com/rolfsormo/devboost/engine" +) + +// CommandGuarded is the architecture's one deliberate escape hatch for +// state that doesn't fit any of the other typed kinds. A module still +// only ever declares data — {ID, Wants} — never imperative logic at the +// declaration site. What makes this a real escape hatch, not a loophole, +// is that CommandGuarded{ID: "x", ...} does nothing on its own: ID must +// match an entry hand-registered in this package via RegisterCommand, +// with real Go diff/apply logic behind it. There is no generic "run a +// script and check the exit code" shortcut — adding a new use requires +// writing an implementation in core, the same amount of real work as +// adding a proper new kind, which is the whole point: this must never be +// the easy path when a real typed kind (File, Package, GitConfig, ...) is +// achievable instead. +// +// An unregistered ID is a startup-time error (Diff returns an error), not +// a silent no-op — declaring one without an implementation should fail +// loudly, the same way a struct literal referencing an undefined type +// wouldn't compile. +type CommandGuarded struct { + ID string + Wants string +} + +// GuardedCommand is what RegisterCommand takes: the real diff/apply logic +// behind one CommandGuarded ID. +type GuardedCommand struct { + // Satisfied reports whether the desired state already holds. + Satisfied func() (bool, error) + // Converge brings the system to the desired state. Only called when + // Satisfied returned false. + Converge func() error +} + +var guardedCommands = map[string]GuardedCommand{} + +// RegisterCommand registers the real implementation behind a +// CommandGuarded ID. Intended to be called from an init() in the file +// that owns the concrete use case (e.g. mise's npm-globals-migrated +// check), not from module declaration sites. +func RegisterCommand(id string, cmd GuardedCommand) { + guardedCommands[id] = cmd +} + +func (c CommandGuarded) Diff() (*engine.PendingOp, error) { + cmd, ok := guardedCommands[c.ID] + if !ok { + return nil, fmt.Errorf("CommandGuarded %q has no registered implementation — see kinds.RegisterCommand", c.ID) + } + ok, err := cmd.Satisfied() + if err != nil { + return nil, fmt.Errorf("CommandGuarded %q: %w", c.ID, err) + } + if ok { + return nil, nil + } + return &engine.PendingOp{ + Description: c.Wants, + Execute: cmd.Converge, + }, nil +} diff --git a/engine/kinds/commandguarded_test.go b/engine/kinds/commandguarded_test.go new file mode 100644 index 0000000..f5a964f --- /dev/null +++ b/engine/kinds/commandguarded_test.go @@ -0,0 +1,54 @@ +package kinds + +import "testing" + +func TestCommandGuardedErrorsWhenUnregistered(t *testing.T) { + c := CommandGuarded{ID: "definitely-not-registered", Wants: "something"} + _, err := c.Diff() + if err == nil { + t.Fatal("expected an error for an unregistered CommandGuarded ID, got nil — this must never silently no-op") + } +} + +func TestCommandGuardedDiffNilWhenSatisfied(t *testing.T) { + RegisterCommand("test-satisfied", GuardedCommand{ + Satisfied: func() (bool, error) { return true, nil }, + Converge: func() error { t.Fatal("Converge should not be called when Satisfied"); return nil }, + }) + c := CommandGuarded{ID: "test-satisfied", Wants: "should already be true"} + op, err := c.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected no pending op when already satisfied, got %+v", op) + } +} + +func TestCommandGuardedDiffPendingWhenUnsatisfied(t *testing.T) { + converged := false + RegisterCommand("test-unsatisfied", GuardedCommand{ + Satisfied: func() (bool, error) { return converged, nil }, + Converge: func() error { + converged = true + return nil + }, + }) + c := CommandGuarded{ID: "test-unsatisfied", Wants: "convergence needed"} + op, err := c.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op when unsatisfied") + } + if op.Description != "convergence needed" { + t.Fatalf("got description %q, want %q", op.Description, "convergence needed") + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + if !converged { + t.Fatal("expected Execute to call Converge") + } +} From 997a57ffbafa70cc9fe520f40ffd90631125a003 Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:16:54 +0300 Subject: [PATCH 06/48] feat(modules): starship, direnv, git, corepack + config bool fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four straightforward module ports: - starship: static File resource for the prompt config. - direnv: File resource for .direnvrc, configurable content. - git: four GitConfig resources for delta integration. - corepack: first real CommandGuarded use — corepack enable has no direct "already enabled" query, and the bash version doesn't attempt one either, so this is ported faithfully rather than inventing new idempotency logic beyond what existed. Also fixes a real bug these modules' own tests caught immediately: config.Get only recognized string-typed YAML values, so a real boolean (enable: false, unquoted) was silently ignored and the default used instead — meaning every .enable flag in a user's real config would have been useless the moment they wrote it the natural YAML way instead of quoting it as "false". Get now stringifies bool/int/float scalars the same way the bash tool's yq-based reader renders them in plain text, matching real config semantics instead of just what the earlier tests happened to exercise (which only used already-quoted string values). --- config/config.go | 26 +++++++++---- config/config_test.go | 31 +++++++++++++++- engine/modules/corepack.go | 43 ++++++++++++++++++++++ engine/modules/corepack_test.go | 18 +++++++++ engine/modules/direnv.go | 26 +++++++++++++ engine/modules/direnv_test.go | 42 +++++++++++++++++++++ engine/modules/git.go | 22 +++++++++++ engine/modules/git_test.go | 18 +++++++++ engine/modules/starship.go | 59 ++++++++++++++++++++++++++++++ engine/modules/starship_test.go | 36 ++++++++++++++++++ engine/modules/testhelpers_test.go | 13 +++++++ 11 files changed, 326 insertions(+), 8 deletions(-) create mode 100644 engine/modules/corepack.go create mode 100644 engine/modules/corepack_test.go create mode 100644 engine/modules/direnv.go create mode 100644 engine/modules/direnv_test.go create mode 100644 engine/modules/git.go create mode 100644 engine/modules/git_test.go create mode 100644 engine/modules/starship.go create mode 100644 engine/modules/starship_test.go create mode 100644 engine/modules/testhelpers_test.go diff --git a/config/config.go b/config/config.go index a3fd9e2..70c2b3f 100644 --- a/config/config.go +++ b/config/config.go @@ -6,6 +6,7 @@ package config import ( + "fmt" "os" "path/filepath" "strings" @@ -51,10 +52,18 @@ func DefaultPath() string { } // Get reads a dotted key path (e.g. "zsh.znap_path"), returning def if -// the key or any intermediate segment is absent, or isn't a string. -// String values (both real ones and def itself) are expanded for a -// leading ~, since defaults like "~/.zsh-snap" need that too — expansion -// happens exactly once, regardless of which path returned the value. +// the key or any intermediate segment is absent. String values (both +// real ones and def itself) are expanded for a leading ~, since defaults +// like "~/.zsh-snap" need that too — expansion happens exactly once, +// regardless of which path returned the value. +// +// A non-string scalar (bool, int, float — e.g. "enable: false" written +// as a real YAML boolean, not a quoted string) is stringified the same +// way the bash tool's yq-based reader renders it in plain output ("true"/ +// "false", plain decimal), so config authors can write natural YAML +// without needing to know devboost's Get treats everything as text +// underneath. A map or list value (wrong shape for a scalar key) falls +// back to def, same as a missing key. func (c *Config) Get(dottedKey string, def string) string { return expandHome(c.get(dottedKey, def)) } @@ -72,11 +81,14 @@ func (c *Config) get(dottedKey string, def string) string { } cur = v } - s, ok := cur.(string) - if !ok { + switch v := cur.(type) { + case string: + return v + case bool, int, int64, float64: + return fmt.Sprintf("%v", v) + default: return def } - return s } func expandHome(s string) string { diff --git a/config/config_test.go b/config/config_test.go index 557ba57..91fc0b0 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -67,7 +67,7 @@ func TestGetFallsBackToDefaultWhenIntermediateSegmentAbsent(t *testing.T) { } } -func TestGetFallsBackToDefaultWhenValueIsNotAString(t *testing.T) { +func TestGetFallsBackToDefaultWhenValueIsAMap(t *testing.T) { path := writeFixture(t, "zsh:\n znap_path:\n nested: true\n") cfg, err := Load(path) if err != nil { @@ -78,6 +78,35 @@ func TestGetFallsBackToDefaultWhenValueIsNotAString(t *testing.T) { } } +// TestGetStringifiesBoolean is a regression test: a real YAML boolean +// (enable: false, not "enable: \"false\"") was silently ignored by an +// earlier version of Get — it only recognized string-typed values, so a +// module reading .git.delta.enable would see the default ("true") even +// though the user explicitly wrote false. yq (the bash tool's reader) +// renders YAML booleans as plain "true"/"false" text, so Get must match +// that, not require users to quote their booleans. +func TestGetStringifiesBoolean(t *testing.T) { + path := writeFixture(t, "git:\n delta:\n enable: false\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + if got := cfg.Get("git.delta.enable", "true"); got != "false" { + t.Fatalf("got %q, want %q", got, "false") + } +} + +func TestGetStringifiesInteger(t *testing.T) { + path := writeFixture(t, "tmux:\n settings:\n base_index: 1\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + if got := cfg.Get("tmux.settings.base_index", "0"); got != "1" { + t.Fatalf("got %q, want %q", got, "1") + } +} + // TestGetExpandsHomeInDefault is a regression test for a real bug found // during the znap spike: expansion only ran on values actually read from // the file, not on the default — so a default like "~/.zsh-snap" with no diff --git a/engine/modules/corepack.go b/engine/modules/corepack.go new file mode 100644 index 0000000..1b4a84a --- /dev/null +++ b/engine/modules/corepack.go @@ -0,0 +1,43 @@ +package modules + +import ( + "os/exec" + + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +func init() { + kinds.RegisterCommand("corepack_enabled", kinds.GuardedCommand{ + // corepack itself has no direct "is enable already run" query, and + // the bash version doesn't attempt one either — it just runs + // `corepack enable` every apply, relying on corepack's own command + // being idempotent. Faithfully ported: Satisfied is only about + // whether corepack exists at all (if not, there's nothing to + // converge — matches the bash version's "skip if missing" path, + // not an error). + Satisfied: func() (bool, error) { + _, err := exec.LookPath("corepack") + return err != nil, nil // corepack absent -> "satisfied" (nothing to do) + }, + Converge: func() error { + return exec.Command("corepack", "enable").Run() + }, + }) +} + +// Corepack ports modules/module_corepack.sh: enable pnpm/yarn shims via +// corepack, gated on toolchains.enable_mise (corepack ships bundled with +// Node.js, which mise manages). +func Corepack(cfg *config.Config) []engine.Resource { + if cfg.Get("toolchains.enable_mise", "true") != "true" { + return nil + } + return []engine.Resource{ + { + ID: "corepack", + Kind: kinds.CommandGuarded{ID: "corepack_enabled", Wants: "corepack pnpm/yarn shims enabled"}, + }, + } +} diff --git a/engine/modules/corepack_test.go b/engine/modules/corepack_test.go new file mode 100644 index 0000000..bd7c8f2 --- /dev/null +++ b/engine/modules/corepack_test.go @@ -0,0 +1,18 @@ +package modules + +import "testing" + +func TestCorepackDisabledWhenMiseDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "toolchains:\n enable_mise: false\n") + if got := Corepack(cfg); len(got) != 0 { + t.Fatalf("expected no resources when mise is disabled, got %v", got) + } +} + +func TestCorepackEnabledByDefault(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := Corepack(cfg) + if len(got) != 1 { + t.Fatalf("expected exactly one resource, got %d", len(got)) + } +} diff --git a/engine/modules/direnv.go b/engine/modules/direnv.go new file mode 100644 index 0000000..4e27c12 --- /dev/null +++ b/engine/modules/direnv.go @@ -0,0 +1,26 @@ +package modules + +import ( + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +const direnvDefaultContent = `use_mise() { eval "$(mise activate direnv)"; }` + +// Direnv ports modules/module_direnv.sh: write the devboost-managed +// .direnvrc, gated on direnv.enable. Content is configurable via +// direnv.content, defaulting to a use_mise helper. +func Direnv(cfg *config.Config) []engine.Resource { + if cfg.Get("direnv.enable", "true") != "true" { + return nil + } + path := cfg.Get("direnv.rc_path", "~/.direnvrc") + content := cfg.Get("direnv.content", direnvDefaultContent) + return []engine.Resource{ + { + ID: "direnvrc", + Kind: kinds.File{Path: path, Content: content}, + }, + } +} diff --git a/engine/modules/direnv_test.go b/engine/modules/direnv_test.go new file mode 100644 index 0000000..0bab218 --- /dev/null +++ b/engine/modules/direnv_test.go @@ -0,0 +1,42 @@ +package modules + +import ( + "strings" + "testing" + + "github.com/rolfsormo/devboost/engine/kinds" +) + +func TestDirenvDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "direnv:\n enable: false\n") + if got := Direnv(cfg); len(got) != 0 { + t.Fatalf("expected no resources when disabled, got %v", got) + } +} + +func TestDirenvDefaultContent(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := Direnv(cfg) + if len(got) != 1 { + t.Fatalf("expected exactly one resource, got %d", len(got)) + } + f, ok := got[0].Kind.(kinds.File) + if !ok { + t.Fatalf("expected a kinds.File resource, got %T", got[0].Kind) + } + if !strings.Contains(f.Content, "use_mise") { + t.Fatalf("expected default content to define use_mise, got %q", f.Content) + } +} + +func TestDirenvCustomContent(t *testing.T) { + cfg := loadFixtureConfig(t, "direnv:\n content: \"custom content\"\n") + got := Direnv(cfg) + f, ok := got[0].Kind.(kinds.File) + if !ok { + t.Fatalf("expected a kinds.File resource, got %T", got[0].Kind) + } + if f.Content != "custom content" { + t.Fatalf("got %q, want %q", f.Content, "custom content") + } +} diff --git a/engine/modules/git.go b/engine/modules/git.go new file mode 100644 index 0000000..43240ec --- /dev/null +++ b/engine/modules/git.go @@ -0,0 +1,22 @@ +package modules + +import ( + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +// Git ports modules/module_git.sh: configure delta as git's pager/diff +// filter, gated on git.delta.enable. All four settings use --global +// scope, matching the bash version (this module never touches --local). +func Git(cfg *config.Config) []engine.Resource { + if cfg.Get("git.delta.enable", "true") != "true" { + return nil + } + return []engine.Resource{ + {ID: "git_delta_pager", Kind: kinds.GitConfig{Key: "core.pager", Value: "delta"}}, + {ID: "git_delta_diff_filter", Kind: kinds.GitConfig{Key: "interactive.diffFilter", Value: "delta --color-only"}}, + {ID: "git_delta_navigate", Kind: kinds.GitConfig{Key: "delta.navigate", Value: cfg.Get("git.delta.navigate", "true")}}, + {ID: "git_delta_line_numbers", Kind: kinds.GitConfig{Key: "delta.line-numbers", Value: cfg.Get("git.delta.line_numbers", "true")}}, + } +} diff --git a/engine/modules/git_test.go b/engine/modules/git_test.go new file mode 100644 index 0000000..82cd3d6 --- /dev/null +++ b/engine/modules/git_test.go @@ -0,0 +1,18 @@ +package modules + +import "testing" + +func TestGitDeltaDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "git:\n delta:\n enable: false\n") + if got := Git(cfg); len(got) != 0 { + t.Fatalf("expected no resources when disabled, got %v", got) + } +} + +func TestGitDeltaEnabledByDefault(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := Git(cfg) + if len(got) != 4 { + t.Fatalf("expected 4 git config resources, got %d", len(got)) + } +} diff --git a/engine/modules/starship.go b/engine/modules/starship.go new file mode 100644 index 0000000..136e8e8 --- /dev/null +++ b/engine/modules/starship.go @@ -0,0 +1,59 @@ +package modules + +import ( + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +const starshipConfigContent = `add_newline = false +command_timeout = 700 + +[character] +success_symbol = "[❯](bold green)" +error_symbol = "[❯](bold red)" + +[directory] +truncation_length = 3 +style = "bold blue" + +[git_branch] +symbol = "" +style = "bold yellow" + +[git_status] +style = "bold red" +format = '([\[$all_status\]]($style))' + +[nodejs] +symbol = "" +style = "green" + +[python] +symbol = "" +style = "yellow" + +[rust] +symbol = "" +style = "red" + +[package] +disabled = true +` + +// Starship ports modules/module_starship.sh: write the devboost-managed +// starship prompt config, gated on prompt.enable_starship. The content is +// entirely static in the bash version too (db_render_starship_config +// takes no config-driven branches), so this is a plain File resource. +func Starship(cfg *config.Config) []engine.Resource { + if cfg.Get("prompt.enable_starship", "true") != "true" { + return nil + } + path := cfg.Get("prompt.starship_config", "~/.config/starship.toml") + return []engine.Resource{ + { + ID: "starship_config", + Kind: kinds.File{Path: path, Content: starshipConfigContent}, + }, + } +} diff --git a/engine/modules/starship_test.go b/engine/modules/starship_test.go new file mode 100644 index 0000000..ba62b37 --- /dev/null +++ b/engine/modules/starship_test.go @@ -0,0 +1,36 @@ +package modules + +import ( + "path/filepath" + "testing" + + "github.com/rolfsormo/devboost/config" +) + +func loadFixtureConfig(t *testing.T, yaml string) *config.Config { + t.Helper() + path := filepath.Join(t.TempDir(), ".devboost.yaml") + if yaml != "" { + writeFile(t, path, yaml) + } + cfg, err := config.Load(path) + if err != nil { + t.Fatal(err) + } + return cfg +} + +func TestStarshipDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "prompt:\n enable_starship: false\n") + if got := Starship(cfg); len(got) != 0 { + t.Fatalf("expected no resources when disabled, got %v", got) + } +} + +func TestStarshipEnabledByDefault(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := Starship(cfg) + if len(got) != 1 { + t.Fatalf("expected exactly one resource, got %d", len(got)) + } +} diff --git a/engine/modules/testhelpers_test.go b/engine/modules/testhelpers_test.go new file mode 100644 index 0000000..70bf7ed --- /dev/null +++ b/engine/modules/testhelpers_test.go @@ -0,0 +1,13 @@ +package modules + +import ( + "os" + "testing" +) + +func writeFile(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} From 5fd9be0e98d966ccd57ef414c9f1631cdadc3f9e Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:18:14 +0300 Subject: [PATCH 07/48] feat(modules): pkg module, config.GetList MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds Config.GetList for reading a YAML list of strings (packages.base and friends), refactored out of Get's traversal logic via a shared lookup helper rather than duplicating the dotted-path walk. Ports modules/module_pkg.sh: installs the configured (or default) base package list via the Package resource kind built earlier — the per-OS name mapping and install logic already lived there, so this module is just supplying the desired package list. --- config/config.go | 58 +++++++++++++++++++++++++++++------- config/config_test.go | 40 +++++++++++++++++++++++++ engine/kinds/package_test.go | 23 ++++++++++++++ engine/modules/pkg.go | 28 +++++++++++++++++ engine/modules/pkg_test.go | 34 +++++++++++++++++++++ 5 files changed, 173 insertions(+), 10 deletions(-) create mode 100644 engine/kinds/package_test.go create mode 100644 engine/modules/pkg.go create mode 100644 engine/modules/pkg_test.go diff --git a/config/config.go b/config/config.go index 70c2b3f..85e1d7f 100644 --- a/config/config.go +++ b/config/config.go @@ -69,26 +69,64 @@ func (c *Config) Get(dottedKey string, def string) string { } func (c *Config) get(dottedKey string, def string) string { + cur, ok := c.lookup(dottedKey) + if !ok { + return def + } + switch v := cur.(type) { + case string: + return v + case bool, int, int64, float64: + return fmt.Sprintf("%v", v) + default: + return def + } +} + +// GetList reads a dotted key path expected to hold a YAML list of +// strings (e.g. "packages.base"), returning nil if the key is absent or +// isn't a list. Non-string list items are stringified the same way Get +// stringifies scalars; items that are neither a string nor a plain +// scalar are skipped rather than erroring, so one malformed entry +// doesn't take down reading the whole list. +func (c *Config) GetList(dottedKey string) []string { + cur, ok := c.lookup(dottedKey) + if !ok { + return nil + } + items, ok := cur.([]any) + if !ok { + return nil + } + var out []string + for _, item := range items { + switch v := item.(type) { + case string: + out = append(out, v) + case bool, int, int64, float64: + out = append(out, fmt.Sprintf("%v", v)) + } + } + return out +} + +// lookup walks a dotted key path through the loaded config, returning the +// raw value at that path (whatever type it happens to be) and whether it +// was found at all. +func (c *Config) lookup(dottedKey string) (any, bool) { cur := any(c.data) for _, part := range strings.Split(strings.Trim(dottedKey, "."), ".") { m, ok := cur.(map[string]any) if !ok { - return def + return nil, false } v, ok := m[part] if !ok { - return def + return nil, false } cur = v } - switch v := cur.(type) { - case string: - return v - case bool, int, int64, float64: - return fmt.Sprintf("%v", v) - default: - return def - } + return cur, true } func expandHome(s string) string { diff --git a/config/config_test.go b/config/config_test.go index 91fc0b0..1e07852 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -156,3 +156,43 @@ func TestGetLeavesNonTildeValuesUntouched(t *testing.T) { t.Fatalf("got %q, want /absolute/path", got) } } + +func TestGetListReadsStringItems(t *testing.T) { + path := writeFixture(t, "packages:\n base:\n - zsh\n - tmux\n - fzf\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + got := cfg.GetList("packages.base") + want := []string{"zsh", "tmux", "fzf"} + if len(got) != len(want) { + t.Fatalf("got %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("got %v, want %v", got, want) + } + } +} + +func TestGetListNilWhenAbsent(t *testing.T) { + path := writeFixture(t, "packages:\n other: value\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + if got := cfg.GetList("packages.base"); got != nil { + t.Fatalf("got %v, want nil", got) + } +} + +func TestGetListNilWhenNotAList(t *testing.T) { + path := writeFixture(t, "packages:\n base: not-a-list\n") + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + if got := cfg.GetList("packages.base"); got != nil { + t.Fatalf("got %v, want nil", got) + } +} diff --git a/engine/kinds/package_test.go b/engine/kinds/package_test.go new file mode 100644 index 0000000..5a8a42e --- /dev/null +++ b/engine/kinds/package_test.go @@ -0,0 +1,23 @@ +package kinds + +import "testing" + +func TestMapPackageNameAppliesOverride(t *testing.T) { + if got := mapPackageName(OSLinuxUbuntu, "fd"); got != "fd-find" { + t.Fatalf("got %q, want fd-find", got) + } + if got := mapPackageName(OSLinuxFedora, "fd"); got != "fd-find" { + t.Fatalf("got %q, want fd-find", got) + } +} + +func TestMapPackageNameIdentityByDefault(t *testing.T) { + for _, os := range []OS{OSDarwin, OSLinuxArch} { + if got := mapPackageName(os, "fd"); got != "fd" { + t.Fatalf("%s: got %q, want fd (no override on this OS)", os, got) + } + } + if got := mapPackageName(OSLinuxUbuntu, "zsh"); got != "zsh" { + t.Fatalf("got %q, want zsh (no override for this package)", got) + } +} diff --git a/engine/modules/pkg.go b/engine/modules/pkg.go new file mode 100644 index 0000000..a976df2 --- /dev/null +++ b/engine/modules/pkg.go @@ -0,0 +1,28 @@ +package modules + +import ( + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +var defaultBasePackages = []string{ + "zsh", "zoxide", "fzf", "ripgrep", "fd", "bat", "eza", "jq", "yq", + "git-delta", "lazygit", "direnv", "mise", "atuin", "starship", "tmux", + "dust", "duf", "procs", +} + +// Pkg ports modules/module_pkg.sh: install the configured base package +// list (packages.base in config, falling back to devboost's own default +// set). Per-OS package name mapping (fd -> fd-find, etc.) and per-OS +// install/already-installed logic live in the Package resource kind +// itself — this module just supplies the desired package list. +func Pkg(cfg *config.Config) []engine.Resource { + names := cfg.GetList("packages.base") + if len(names) == 0 { + names = defaultBasePackages + } + return []engine.Resource{ + {ID: "base_packages", Kind: kinds.Package{Names: names}}, + } +} diff --git a/engine/modules/pkg_test.go b/engine/modules/pkg_test.go new file mode 100644 index 0000000..bc46f4e --- /dev/null +++ b/engine/modules/pkg_test.go @@ -0,0 +1,34 @@ +package modules + +import ( + "testing" + + "github.com/rolfsormo/devboost/engine/kinds" +) + +func TestPkgUsesDefaultsWhenUnconfigured(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := Pkg(cfg) + if len(got) != 1 { + t.Fatalf("expected exactly one resource, got %d", len(got)) + } + p, ok := got[0].Kind.(kinds.Package) + if !ok { + t.Fatalf("expected a kinds.Package resource, got %T", got[0].Kind) + } + if len(p.Names) == 0 { + t.Fatal("expected default package list to be non-empty") + } +} + +func TestPkgUsesConfiguredList(t *testing.T) { + cfg := loadFixtureConfig(t, "packages:\n base:\n - zsh\n - tmux\n") + got := Pkg(cfg) + p, ok := got[0].Kind.(kinds.Package) + if !ok { + t.Fatalf("expected a kinds.Package resource, got %T", got[0].Kind) + } + if len(p.Names) != 2 || p.Names[0] != "zsh" || p.Names[1] != "tmux" { + t.Fatalf("got %v, want [zsh tmux]", p.Names) + } +} From 776344e5887743e3b9297e818676589e9dfd98e2 Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:19:08 +0300 Subject: [PATCH 08/48] feat(modules): services (atuin daemon, darwin only) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports modules/module_services.sh's darwin branch as a CommandGuarded resource (checks brew services list for atuin already started, converges via brew services start). The bash version's Linux branch is purely informational — it never actually converges anything, just logs a suggestion to check systemd — so it isn't forced into a resource with nothing to do; that note belongs with whatever doctor-only informational mechanism lands alongside the security module (task #14). --- engine/modules/services.go | 60 +++++++++++++++++++++++++++++++++ engine/modules/services_test.go | 21 ++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 engine/modules/services.go create mode 100644 engine/modules/services_test.go diff --git a/engine/modules/services.go b/engine/modules/services.go new file mode 100644 index 0000000..f29de23 --- /dev/null +++ b/engine/modules/services.go @@ -0,0 +1,60 @@ +package modules + +import ( + "os/exec" + "strings" + + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +func init() { + kinds.RegisterCommand("atuin_brew_service_running", kinds.GuardedCommand{ + Satisfied: func() (bool, error) { + out, err := exec.Command("brew", "services", "list").Output() + if err != nil { + // brew not present/working — nothing this resource can + // converge; matches the bash version's "skip if atuin + // binary missing" behavior at the module-gating level. + return true, nil + } + for _, line := range strings.Split(string(out), "\n") { + fields := strings.Fields(line) + if len(fields) >= 2 && fields[0] == "atuin" && fields[1] == "started" { + return true, nil + } + } + return false, nil + }, + Converge: func() error { + return exec.Command("brew", "services", "start", "atuin").Run() + }, + }) +} + +// Services ports modules/module_services.sh, darwin only: start the +// atuin brew service if it isn't already running, gated on +// zsh.history.use_atuin. The bash version's Linux branch is purely +// informational (no actual action — it just logs a suggestion to check +// systemd) with nothing to converge, so it declares no resource here; +// that informational note belongs to whatever doctor/info-only +// diagnostic mechanism lands with the security module (task #14), not +// forced into a resource with nothing to do. +func Services(cfg *config.Config, os kinds.OS) []engine.Resource { + if cfg.Get("zsh.history.use_atuin", "true") != "true" { + return nil + } + if os != kinds.OSDarwin { + return nil + } + if _, err := exec.LookPath("atuin"); err != nil { + return nil + } + return []engine.Resource{ + { + ID: "atuin_service", + Kind: kinds.CommandGuarded{ID: "atuin_brew_service_running", Wants: "atuin service started via brew"}, + }, + } +} diff --git a/engine/modules/services_test.go b/engine/modules/services_test.go new file mode 100644 index 0000000..d47ad1a --- /dev/null +++ b/engine/modules/services_test.go @@ -0,0 +1,21 @@ +package modules + +import ( + "testing" + + "github.com/rolfsormo/devboost/engine/kinds" +) + +func TestServicesDisabledWhenAtuinDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "zsh:\n history:\n use_atuin: false\n") + if got := Services(cfg, kinds.OSDarwin); len(got) != 0 { + t.Fatalf("expected no resources when atuin disabled, got %v", got) + } +} + +func TestServicesNoResourceOnLinux(t *testing.T) { + cfg := loadFixtureConfig(t, "") + if got := Services(cfg, kinds.OSLinuxUbuntu); len(got) != 0 { + t.Fatalf("expected no resources on Linux (informational only), got %v", got) + } +} From fd436dbd31e1ec267279ef36594f94e76bbeb50e Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:21:54 +0300 Subject: [PATCH 09/48] feat(kinds,modules): CommandGuarded gains Params, tmux module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CommandGuarded now carries a Params any field, threaded through to its registered Satisfied/Converge functions. This was a real gap, not scope creep: tmux's plugin-install step genuinely needs the configured TPM path at Converge time, and the registry (ID -> GuardedCommand) has no other way to receive per-declaration data while keeping the "module only ever declares data, never logic" rule intact — Params is still just data the module supplies, the registered implementation for ID still owns all the actual logic. Ports modules/module_tmux.sh: GitClone for TPM, a dependency-ordered BlockInFile for the tmux.conf block (depends on tmux_tpm — the render needs the resolved TPM path), and a CommandGuarded plugin install/update step (depends on both, ported faithfully as always-pending since TPM's own install script is what's actually idempotent, matching the bash version's fire-and-forget behavior). --- engine/kinds/commandguarded.go | 43 +++++++----- engine/kinds/commandguarded_test.go | 39 +++++++++-- engine/modules/corepack.go | 4 +- engine/modules/services.go | 4 +- engine/modules/tmux.go | 105 ++++++++++++++++++++++++++++ engine/modules/tmux_test.go | 91 ++++++++++++++++++++++++ 6 files changed, 260 insertions(+), 26 deletions(-) create mode 100644 engine/modules/tmux.go create mode 100644 engine/modules/tmux_test.go diff --git a/engine/kinds/commandguarded.go b/engine/kinds/commandguarded.go index 1950b32..0add23d 100644 --- a/engine/kinds/commandguarded.go +++ b/engine/kinds/commandguarded.go @@ -8,34 +8,41 @@ import ( // CommandGuarded is the architecture's one deliberate escape hatch for // state that doesn't fit any of the other typed kinds. A module still -// only ever declares data — {ID, Wants} — never imperative logic at the -// declaration site. What makes this a real escape hatch, not a loophole, -// is that CommandGuarded{ID: "x", ...} does nothing on its own: ID must -// match an entry hand-registered in this package via RegisterCommand, -// with real Go diff/apply logic behind it. There is no generic "run a -// script and check the exit code" shortcut — adding a new use requires -// writing an implementation in core, the same amount of real work as -// adding a proper new kind, which is the whole point: this must never be -// the easy path when a real typed kind (File, Package, GitConfig, ...) is -// achievable instead. +// only ever declares data — {ID, Params, Wants} — never imperative logic +// at the declaration site. What makes this a real escape hatch, not a +// loophole, is that CommandGuarded{ID: "x", ...} does nothing on its own: +// ID must match an entry hand-registered in this package via +// RegisterCommand, with real Go diff/apply logic behind it. There is no +// generic "run a script and check the exit code" shortcut — adding a new +// use requires writing an implementation in core, the same amount of real +// work as adding a proper new kind, which is the whole point: this must +// never be the easy path when a real typed kind (File, Package, +// GitConfig, ...) is achievable instead. +// +// Params carries whatever plain data the registered implementation needs +// (e.g. a configured path) — still just data the module declares, not +// logic; the implementation registered for ID decides what shape it +// expects and type-asserts accordingly. // // An unregistered ID is a startup-time error (Diff returns an error), not // a silent no-op — declaring one without an implementation should fail // loudly, the same way a struct literal referencing an undefined type // wouldn't compile. type CommandGuarded struct { - ID string - Wants string + ID string + Params any + Wants string } // GuardedCommand is what RegisterCommand takes: the real diff/apply logic -// behind one CommandGuarded ID. +// behind one CommandGuarded ID. Both functions receive the Params value +// from the CommandGuarded that triggered them. type GuardedCommand struct { // Satisfied reports whether the desired state already holds. - Satisfied func() (bool, error) + Satisfied func(params any) (bool, error) // Converge brings the system to the desired state. Only called when // Satisfied returned false. - Converge func() error + Converge func(params any) error } var guardedCommands = map[string]GuardedCommand{} @@ -53,15 +60,15 @@ func (c CommandGuarded) Diff() (*engine.PendingOp, error) { if !ok { return nil, fmt.Errorf("CommandGuarded %q has no registered implementation — see kinds.RegisterCommand", c.ID) } - ok, err := cmd.Satisfied() + satisfied, err := cmd.Satisfied(c.Params) if err != nil { return nil, fmt.Errorf("CommandGuarded %q: %w", c.ID, err) } - if ok { + if satisfied { return nil, nil } return &engine.PendingOp{ Description: c.Wants, - Execute: cmd.Converge, + Execute: func() error { return cmd.Converge(c.Params) }, }, nil } diff --git a/engine/kinds/commandguarded_test.go b/engine/kinds/commandguarded_test.go index f5a964f..f6e4f26 100644 --- a/engine/kinds/commandguarded_test.go +++ b/engine/kinds/commandguarded_test.go @@ -12,8 +12,8 @@ func TestCommandGuardedErrorsWhenUnregistered(t *testing.T) { func TestCommandGuardedDiffNilWhenSatisfied(t *testing.T) { RegisterCommand("test-satisfied", GuardedCommand{ - Satisfied: func() (bool, error) { return true, nil }, - Converge: func() error { t.Fatal("Converge should not be called when Satisfied"); return nil }, + Satisfied: func(any) (bool, error) { return true, nil }, + Converge: func(any) error { t.Fatal("Converge should not be called when Satisfied"); return nil }, }) c := CommandGuarded{ID: "test-satisfied", Wants: "should already be true"} op, err := c.Diff() @@ -28,8 +28,8 @@ func TestCommandGuardedDiffNilWhenSatisfied(t *testing.T) { func TestCommandGuardedDiffPendingWhenUnsatisfied(t *testing.T) { converged := false RegisterCommand("test-unsatisfied", GuardedCommand{ - Satisfied: func() (bool, error) { return converged, nil }, - Converge: func() error { + Satisfied: func(any) (bool, error) { return converged, nil }, + Converge: func(any) error { converged = true return nil }, @@ -52,3 +52,34 @@ func TestCommandGuardedDiffPendingWhenUnsatisfied(t *testing.T) { t.Fatal("expected Execute to call Converge") } } + +// TestCommandGuardedPassesParams confirms Params flows through to both +// Satisfied and Converge unmodified — the mechanism tmux's plugin-install +// use (a configured TPM path) and similar parameterized uses depend on. +func TestCommandGuardedPassesParams(t *testing.T) { + var seenBySatisfied, seenByConverge string + RegisterCommand("test-params", GuardedCommand{ + Satisfied: func(p any) (bool, error) { + seenBySatisfied = p.(string) + return false, nil + }, + Converge: func(p any) error { + seenByConverge = p.(string) + return nil + }, + }) + c := CommandGuarded{ID: "test-params", Params: "hello", Wants: "x"} + op, err := c.Diff() + if err != nil || op == nil { + t.Fatalf("op=%+v err=%v", op, err) + } + if seenBySatisfied != "hello" { + t.Fatalf("Satisfied saw %q, want %q", seenBySatisfied, "hello") + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + if seenByConverge != "hello" { + t.Fatalf("Converge saw %q, want %q", seenByConverge, "hello") + } +} diff --git a/engine/modules/corepack.go b/engine/modules/corepack.go index 1b4a84a..43315be 100644 --- a/engine/modules/corepack.go +++ b/engine/modules/corepack.go @@ -17,11 +17,11 @@ func init() { // whether corepack exists at all (if not, there's nothing to // converge — matches the bash version's "skip if missing" path, // not an error). - Satisfied: func() (bool, error) { + Satisfied: func(any) (bool, error) { _, err := exec.LookPath("corepack") return err != nil, nil // corepack absent -> "satisfied" (nothing to do) }, - Converge: func() error { + Converge: func(any) error { return exec.Command("corepack", "enable").Run() }, }) diff --git a/engine/modules/services.go b/engine/modules/services.go index f29de23..66e64dd 100644 --- a/engine/modules/services.go +++ b/engine/modules/services.go @@ -11,7 +11,7 @@ import ( func init() { kinds.RegisterCommand("atuin_brew_service_running", kinds.GuardedCommand{ - Satisfied: func() (bool, error) { + Satisfied: func(any) (bool, error) { out, err := exec.Command("brew", "services", "list").Output() if err != nil { // brew not present/working — nothing this resource can @@ -27,7 +27,7 @@ func init() { } return false, nil }, - Converge: func() error { + Converge: func(any) error { return exec.Command("brew", "services", "start", "atuin").Run() }, }) diff --git a/engine/modules/tmux.go b/engine/modules/tmux.go new file mode 100644 index 0000000..7b47d36 --- /dev/null +++ b/engine/modules/tmux.go @@ -0,0 +1,105 @@ +package modules + +import ( + "fmt" + "os/exec" + + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +const ( + tmuxStartMarker = "# >>> devboost tmux start" + tmuxEndMarker = "# <<< devboost tmux end" + tpmGitURL = "https://github.com/tmux-plugins/tpm" +) + +func onOff(cfg *config.Config, key, def string) string { + if cfg.Get(key, def) == "true" { + return "on" + } + return "off" +} + +func renderTmuxBlock(cfg *config.Config, tpmPath string) string { + return fmt.Sprintf(`set -g base-index %s +setw -g pane-base-index %s +set -g mouse %s +set -g history-limit %s +set -s escape-time %s +set -g focus-events %s +set -g @plugin 'tmux-plugins/tpm' +set -g @plugin 'tmux-plugins/tmux-resurrect' +set -g @plugin 'tmux-plugins/tmux-continuum' +set -g @plugin 'tmux-plugins/tmux-yank' +set -g @plugin 'tmux-plugins/tmux-logging' +set -g @continuum-restore '%s' +set -g @resurrect-capture-pane-contents '%s' +run '%s/tpm'`, + cfg.Get("tmux.settings.base_index", "1"), + cfg.Get("tmux.settings.pane_base_index", "1"), + onOff(cfg, "tmux.settings.mouse", "true"), + cfg.Get("tmux.settings.history_limit", "50000"), + cfg.Get("tmux.settings.escape_time", "0"), + onOff(cfg, "tmux.settings.focus_events", "true"), + onOff(cfg, "tmux.settings.continuum_restore", "true"), + onOff(cfg, "tmux.settings.resurrect_capture_pane_contents", "true"), + tpmPath, + ) +} + +func init() { + kinds.RegisterCommand("tmux_plugins_installed", kinds.GuardedCommand{ + // Matches the bash version: plugin install/update is fire-and-forget + // (both commands' own errors are swallowed there too), and it only + // runs when system.auto_install_plugins is true — module-level + // gating decides whether this resource exists at all, so once it + // does exist there's no cheap way to know "are plugins already + // installed" short of re-running install, which TPM's own script + // already makes idempotent. Always pending is the faithful port. + Satisfied: func(any) (bool, error) { return false, nil }, + Converge: func(params any) error { + tpmPath := params.(string) + _ = exec.Command(tpmPath + "/bindings/install_plugins").Run() + _ = exec.Command(tpmPath+"/bindings/update_plugins", "all").Run() + return nil + }, + }) +} + +// Tmux ports modules/module_tmux.sh: clone TPM, upsert the devboost tmux +// config block, and install/update plugins (gated on +// system.auto_install_plugins), gated overall on tmux.enable. +func Tmux(cfg *config.Config) []engine.Resource { + if cfg.Get("tmux.enable", "true") != "true" { + return nil + } + + tpmPath := cfg.Get("tmux.tpm_path", "~/.tmux/plugins/tpm") + confFile := cfg.Get("tmux.conf_file", "~/.tmux.conf") + + resources := []engine.Resource{ + {ID: "tmux_tpm", Kind: kinds.GitClone{URL: tpmGitURL, Dest: tpmPath}}, + { + ID: "tmux_config_block", + Kind: kinds.BlockInFile{ + Path: confFile, + StartMarker: tmuxStartMarker, + EndMarker: tmuxEndMarker, + Content: renderTmuxBlock(cfg, tpmPath), + }, + DependsOn: []string{"tmux_tpm"}, + }, + } + + if cfg.Get("system.auto_install_plugins", "true") == "true" { + resources = append(resources, engine.Resource{ + ID: "tmux_plugins", + Kind: kinds.CommandGuarded{ID: "tmux_plugins_installed", Params: tpmPath, Wants: "install/update tmux plugins via TPM"}, + DependsOn: []string{"tmux_tpm", "tmux_config_block"}, + }) + } + + return resources +} diff --git a/engine/modules/tmux_test.go b/engine/modules/tmux_test.go new file mode 100644 index 0000000..b1db10e --- /dev/null +++ b/engine/modules/tmux_test.go @@ -0,0 +1,91 @@ +package modules + +import ( + "strings" + "testing" + + "github.com/rolfsormo/devboost/engine/kinds" +) + +func TestTmuxDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "tmux:\n enable: false\n") + if got := Tmux(cfg); len(got) != 0 { + t.Fatalf("expected no resources when disabled, got %v", got) + } +} + +func TestTmuxEnabledByDefaultProducesThreeResources(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := Tmux(cfg) + // tpm clone, config block, plugin install (auto_install_plugins defaults true) + if len(got) != 3 { + t.Fatalf("expected 3 resources, got %d: %v", len(got), got) + } +} + +func TestTmuxOmitsPluginInstallWhenAutoInstallDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "system:\n auto_install_plugins: false\n") + got := Tmux(cfg) + if len(got) != 2 { + t.Fatalf("expected 2 resources (no plugin install step), got %d: %v", len(got), got) + } + for _, r := range got { + if r.ID == "tmux_plugins" { + t.Fatal("expected no tmux_plugins resource when auto_install_plugins is false") + } + } +} + +func TestTmuxConfigBlockDependsOnTPM(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := Tmux(cfg) + for _, r := range got { + if r.ID == "tmux_config_block" { + if len(r.DependsOn) != 1 || r.DependsOn[0] != "tmux_tpm" { + t.Fatalf("expected tmux_config_block to depend on tmux_tpm, got %v", r.DependsOn) + } + return + } + } + t.Fatal("expected a tmux_config_block resource") +} + +func TestRenderTmuxBlockUsesConfiguredSettings(t *testing.T) { + cfg := loadFixtureConfig(t, "tmux:\n settings:\n base_index: 0\n mouse: false\n") + block := renderTmuxBlock(cfg, "/tpm/path") + if !strings.Contains(block, "base-index 0") { + t.Fatalf("expected configured base_index in block, got %q", block) + } + if !strings.Contains(block, "mouse off") { + t.Fatalf("expected mouse off in block, got %q", block) + } + if !strings.Contains(block, "run '/tpm/path/tpm'") { + t.Fatalf("expected tpm path wired into run line, got %q", block) + } +} + +func TestRenderTmuxBlockDefaults(t *testing.T) { + cfg := loadFixtureConfig(t, "") + block := renderTmuxBlock(cfg, "/tpm/path") + if !strings.Contains(block, "base-index 1") || !strings.Contains(block, "mouse on") { + t.Fatalf("expected default settings, got %q", block) + } +} + +func TestTmuxPluginsResourceCarriesTPMPathAsParams(t *testing.T) { + cfg := loadFixtureConfig(t, "tmux:\n tpm_path: /custom/tpm\n") + got := Tmux(cfg) + for _, r := range got { + if r.ID == "tmux_plugins" { + c, ok := r.Kind.(kinds.CommandGuarded) + if !ok { + t.Fatalf("expected kinds.CommandGuarded, got %T", r.Kind) + } + if c.Params != "/custom/tpm" { + t.Fatalf("got Params %v, want /custom/tpm", c.Params) + } + return + } + } + t.Fatal("expected a tmux_plugins resource") +} From 172d3923e63d0370a1a51554483de892ba6f5ea0 Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:24:40 +0300 Subject: [PATCH 10/48] feat(modules): mise (toolchains + npm-globals migration) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports modules/module_mise.sh: converges mise-managed global toolchain versions via CommandGuarded (mise use/install are both idempotent no-ops already, and the bash version never checked beforehand either — faithfully always-pending), then — the escape hatch's first real, substantial use — offers to reinstall previously-global npm packages into a new node version when mise's own convergence changes it, including the interactive confirm prompt (Converge just reads stdin directly, same as the bash version reads the terminal directly; no new engine plumbing needed for this). Also fixes a real bug the port's own tests caught: the bash version's npm-globals parser let the literal string "node_modules" (npm's own global module directory, always the first line of `npm list -g --parseable`) leak through as a false-positive package name — its awk filter only checked "does this line have a slash," which that line also satisfies. Confirmed by running the actual awk command against real npm-shaped output before deciding this wasn't specific to the port. Fixed by filtering on name, not position, so it doesn't depend on npm's output ordering. Tracked to backport to the bash tool as task #24 (low priority, low severity: worst case is an npm install -g node_modules that just fails). --- engine/modules/mise.go | 182 ++++++++++++++++++++++++++++++++++++ engine/modules/mise_test.go | 48 ++++++++++ 2 files changed, 230 insertions(+) create mode 100644 engine/modules/mise.go create mode 100644 engine/modules/mise_test.go diff --git a/engine/modules/mise.go b/engine/modules/mise.go new file mode 100644 index 0000000..1fa4e00 --- /dev/null +++ b/engine/modules/mise.go @@ -0,0 +1,182 @@ +package modules + +import ( + "bufio" + "fmt" + "os" + "os/exec" + "strings" + + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +// miseToolchains carries the desired toolchain versions through +// CommandGuarded's Params — plain data, same rule as tmux's TPM path. +type miseToolchains struct { + node, python, goVersion, rust, deno string +} + +func (m miseToolchains) args() []string { + return []string{ + "node@" + m.node, + "python@" + m.python, + "go@" + m.goVersion, + "rust@" + m.rust, + "deno@" + m.deno, + } +} + +func currentMiseNodeVersion() string { + out, err := exec.Command("mise", "current", "node").Output() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + +// npmGlobals lists globally installed npm packages, excluding npm/corepack +// themselves — ports _db_mise_npm_globals. +func npmGlobals() []string { + nodeBin, err := exec.LookPath("node") + if err != nil { + return nil + } + npmBin := nodeBin[:strings.LastIndex(nodeBin, "/")+1] + "npm" + if _, err := os.Stat(npmBin); err != nil { + return nil + } + out, err := exec.Command(npmBin, "list", "-g", "--depth=0", "--parseable").Output() + if err != nil { + return nil + } + return parseNpmGlobalsOutput(string(out)) +} + +// parseNpmGlobalsOutput extracts package names from `npm list -g --depth=0 +// --parseable` output, excluding npm/corepack themselves — split out from +// npmGlobals so the parsing logic is testable without a real npm/node on +// the test machine. +// +// npm always prints the global node_modules directory itself as the +// first line, with no package name after it — its last path segment is +// literally "node_modules". The bash tool's original awk filter (NF>1 && +// ...) didn't exclude this by name, only by "has a slash," which the +// node_modules directory line also satisfies — so it leaked through as a +// false-positive "package" (confirmed by testing the actual awk command +// against real npm-shaped output). Harmless in practice (worst case: +// offering to `npm install -g node_modules`, which just fails) but a +// real bug nonetheless — fixed here by name rather than ported +// faithfully, since a bug found while porting doesn't need to survive +// the port, and filtering by name (not position) doesn't depend on +// npm's output ordering staying stable. +func parseNpmGlobalsOutput(out string) []string { + var pkgs []string + for _, line := range strings.Split(out, "\n") { + parts := strings.Split(line, "/") + if len(parts) < 2 { + continue + } + name := parts[len(parts)-1] + if name != "" && name != "npm" && name != "corepack" && name != "node_modules" { + pkgs = append(pkgs, name) + } + } + return pkgs +} + +func confirm(prompt string) bool { + fmt.Printf("? %s [y/N] ", prompt) + reader := bufio.NewReader(os.Stdin) + line, _ := reader.ReadString('\n') + return strings.ToLower(strings.TrimSpace(line)) == "y" +} + +func init() { + kinds.RegisterCommand("mise_toolchains_converged", kinds.GuardedCommand{ + // mise use/install are both idempotent no-ops when already at the + // desired versions, and the bash version never checked beforehand + // either — it always ran both commands. Faithful port: always + // pending when this resource exists at all. + Satisfied: func(any) (bool, error) { return false, nil }, + Converge: func(params any) error { + t := params.(miseToolchains) + + prevNode := currentMiseNodeVersion() + var globals []string + if prevNode != "" { + globals = npmGlobals() + } + + useArgs := append([]string{"use", "-g"}, t.args()...) + _ = exec.Command("mise", useArgs...).Run() + if err := exec.Command("mise", "install").Run(); err != nil { + fmt.Fprintln(os.Stderr, "warning: some toolchains may not be available") + } + + if len(globals) == 0 { + return nil + } + newNode := currentMiseNodeVersion() + if newNode == prevNode { + return nil + } + + fmt.Printf("Node upgraded: %s -> %s\n", prevNode, newNode) + fmt.Println("The following global npm packages were present in the old version:") + for _, pkg := range globals { + fmt.Println(" -", pkg) + } + fmt.Println("Note: any version pins or custom configuration for these packages will NOT be migrated.") + if !confirm(fmt.Sprintf("Reinstall these packages into node@%s?", newNode)) { + return nil + } + + nodeBin, err := exec.LookPath("node") + if err != nil { + return nil + } + npmBin := nodeBin[:strings.LastIndex(nodeBin, "/")+1] + "npm" + for _, pkg := range globals { + fmt.Println("Installing", pkg, "...") + if err := exec.Command(npmBin, "install", "-g", pkg).Run(); err != nil { + fmt.Println(" x", pkg, "(failed — install manually if needed)") + } else { + fmt.Println(" ✓", pkg) + } + } + return nil + }, + }) +} + +// Mise ports modules/module_mise.sh: converge mise-managed toolchain +// versions, gated on toolchains.enable_mise, then (if node's version +// actually changed as a result) offer to reinstall previously-global npm +// packages into the new node version — the CommandGuarded escape hatch's +// first real, substantial use, since neither "converge toolchains" nor +// "conditionally offer an interactive migration" maps onto any of the +// typed kinds. +func Mise(cfg *config.Config) []engine.Resource { + if cfg.Get("toolchains.enable_mise", "true") != "true" { + return nil + } + if _, err := exec.LookPath("mise"); err != nil { + return nil + } + + t := miseToolchains{ + node: cfg.Get("toolchains.globals.node", "lts"), + python: cfg.Get("toolchains.globals.python", "3.14"), + goVersion: cfg.Get("toolchains.globals.go", "1.26"), + rust: cfg.Get("toolchains.globals.rust", "stable"), + deno: cfg.Get("toolchains.globals.deno", "lts"), + } + return []engine.Resource{ + { + ID: "mise_toolchains", + Kind: kinds.CommandGuarded{ID: "mise_toolchains_converged", Params: t, Wants: "configure mise toolchains"}, + }, + } +} diff --git a/engine/modules/mise_test.go b/engine/modules/mise_test.go new file mode 100644 index 0000000..8c3b67f --- /dev/null +++ b/engine/modules/mise_test.go @@ -0,0 +1,48 @@ +package modules + +import "testing" + +func TestMiseDisabledWhenMiseConfigDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "toolchains:\n enable_mise: false\n") + if got := Mise(cfg); len(got) != 0 { + t.Fatalf("expected no resources when disabled, got %v", got) + } +} + +func TestMiseToolchainsArgs(t *testing.T) { + m := miseToolchains{node: "lts", python: "3.14", goVersion: "1.26", rust: "stable", deno: "lts"} + args := m.args() + want := []string{"node@lts", "python@3.14", "go@1.26", "rust@stable", "deno@lts"} + if len(args) != len(want) { + t.Fatalf("got %v, want %v", args, want) + } + for i := range want { + if args[i] != want[i] { + t.Fatalf("got %v, want %v", args, want) + } + } +} + +func TestParseNpmGlobalsOutputExcludesNpmAndCorepack(t *testing.T) { + out := "/opt/node/lib/node_modules\n" + + "/opt/node/lib/node_modules/npm\n" + + "/opt/node/lib/node_modules/corepack\n" + + "/opt/node/lib/node_modules/typescript\n" + + "/opt/node/lib/node_modules/eslint\n" + got := parseNpmGlobalsOutput(out) + want := []string{"typescript", "eslint"} + if len(got) != len(want) { + t.Fatalf("got %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("got %v, want %v", got, want) + } + } +} + +func TestParseNpmGlobalsOutputEmpty(t *testing.T) { + if got := parseNpmGlobalsOutput(""); got != nil { + t.Fatalf("got %v, want nil", got) + } +} From 5e45e7b1f6faf82f3e54a35e219a2d236fe0e31d Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:28:57 +0300 Subject: [PATCH 11/48] =?UTF-8?q?feat(modules):=20zsh=20=E2=80=94=20the=20?= =?UTF-8?q?big=20one=20(.zshrc.devboost,=20include=20block,=20atuin)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports modules/module_zsh.sh, the largest module: renderZshDevboost composes the whole .zshrc.devboost file exactly like the bash version's sequential heredoc/echo composition, section by section, each gated on the same config keys (starship, atuin, fzf, mise, direnv, aesthetics, aliases). Includes a regression test asserting the module never reintroduces the direct compinit call removed earlier this session (that bug — devboost calling compinit itself before znap redefines it as a no-op — was the dominant real-world startup-lag finding from the zsh investigation). The include-block injection into ~/.zshrc needed a bespoke, module-local resource kind (zshIncludeBlock) rather than reusing BlockInFile: it has a fourth case BlockInFile's contract has no room for — an unmarked pre-existing line already sourcing .zshrc.devboost must cause a warn-and-skip, not a normal inject, or shell startup would silently pay for everything in .zshrc.devboost twice. This is exactly the double-sourcing bug fixed for real earlier this session; the port has a dedicated regression test for it. Also documents (with tests, not a silent fix) a real weakness inherited from the bash regex: it only excludes '#' immediately adjacent to the match, not general end-of-line comments — logged as task #25 for a deliberate decision rather than changed unilaterally during the port. Exports kinds.BackupFile (was unexported) so this module-local kind can reuse the same backup behavior instead of duplicating it. --- engine/kinds/backup.go | 9 +- engine/kinds/blockinfile.go | 2 +- engine/kinds/file.go | 2 +- engine/modules/zsh.go | 57 ++++++++++ engine/modules/zsh_test.go | 46 ++++++++ engine/modules/zshdevboost.go | 104 ++++++++++++++++++ engine/modules/zshdevboost_test.go | 94 ++++++++++++++++ engine/modules/zshinclude.go | 87 +++++++++++++++ engine/modules/zshinclude_test.go | 168 +++++++++++++++++++++++++++++ 9 files changed, 564 insertions(+), 5 deletions(-) create mode 100644 engine/modules/zsh.go create mode 100644 engine/modules/zsh_test.go create mode 100644 engine/modules/zshdevboost.go create mode 100644 engine/modules/zshdevboost_test.go create mode 100644 engine/modules/zshinclude.go create mode 100644 engine/modules/zshinclude_test.go diff --git a/engine/kinds/backup.go b/engine/kinds/backup.go index c389124..2ad54b0 100644 --- a/engine/kinds/backup.go +++ b/engine/kinds/backup.go @@ -20,11 +20,14 @@ func backupDir() (string, error) { return filepath.Join(home, ".devboost", "backups"), nil } -// backupFile copies path into a fresh timestamped subdirectory of the +// BackupFile copies path into a fresh timestamped subdirectory of the // backup dir before it's about to be overwritten, mirroring the bash // tool's db_backup_file. A no-op if path doesn't exist yet (nothing to -// back up). -func backupFile(path string) error { +// back up). Exported so module-local resource kinds outside this package +// (e.g. zsh's include-block handling, which has its own custom diff logic +// not shaped like any generic kind) can reuse the same backup behavior +// instead of duplicating it. +func BackupFile(path string) error { if _, err := os.Stat(path); os.IsNotExist(err) { return nil } else if err != nil { diff --git a/engine/kinds/blockinfile.go b/engine/kinds/blockinfile.go index f0df3a9..e61d9f1 100644 --- a/engine/kinds/blockinfile.go +++ b/engine/kinds/blockinfile.go @@ -56,7 +56,7 @@ func (b BlockInFile) Diff() (*engine.PendingOp, error) { return &engine.PendingOp{ Description: fmt.Sprintf("%s %s", verb, b.Path), Execute: func() error { - if err := backupFile(b.Path); err != nil { + if err := BackupFile(b.Path); err != nil { return err } return os.WriteFile(b.Path, []byte(desired), 0o644) diff --git a/engine/kinds/file.go b/engine/kinds/file.go index 8993432..0157d12 100644 --- a/engine/kinds/file.go +++ b/engine/kinds/file.go @@ -44,7 +44,7 @@ func (f File) Diff() (*engine.PendingOp, error) { Description: fmt.Sprintf("%s %s", verb, f.Path), Execute: func() error { if !f.NoBackup { - if err := backupFile(f.Path); err != nil { + if err := BackupFile(f.Path); err != nil { return err } } diff --git a/engine/modules/zsh.go b/engine/modules/zsh.go new file mode 100644 index 0000000..4ab2f72 --- /dev/null +++ b/engine/modules/zsh.go @@ -0,0 +1,57 @@ +package modules + +import ( + "os" + "path/filepath" + + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +// Zsh ports modules/module_zsh.sh: renders .zshrc.devboost from config, +// injects the devboost include block into ~/.zshrc (skipping injection +// with a warning if an unmarked line already sources .zshrc.devboost — +// see zshIncludeBlock), and writes atuin's config.toml when atuin history +// is enabled. Gated overall on zsh.enable. +func Zsh(cfg *config.Config) []engine.Resource { + if cfg.Get("zsh.enable", "true") != "true" { + return nil + } + + includeFile := cfg.Get("zsh.include_file", "~/.zshrc.devboost") + // ~/.zshrc's path is hardcoded in the bash version too (never + // config-driven, unlike include_file) — it's the one file a zsh + // shell always sources by name, not something devboost lets a user + // relocate. + home, err := os.UserHomeDir() + if err != nil { + home = "~" + } + zshrc := filepath.Join(home, ".zshrc") + + resources := []engine.Resource{ + { + ID: "zshrc_devboost", + Kind: kinds.File{Path: includeFile, Content: renderZshDevboost(cfg)}, + }, + { + ID: "zshrc_include_block", + Kind: zshIncludeBlock{zshrcPath: zshrc}, + DependsOn: []string{"zshrc_devboost"}, + }, + } + + if cfg.Get("zsh.history.use_atuin", "true") == "true" { + // Hardcoded in the bash version too — not a config.Get default + // standing in for a real knob, atuin's config path was never + // user-configurable there either. + atuinConfig := filepath.Join(home, ".config", "atuin", "config.toml") + resources = append(resources, engine.Resource{ + ID: "atuin_config", + Kind: kinds.File{Path: atuinConfig, Content: renderAtuinConfig(cfg)}, + }) + } + + return resources +} diff --git a/engine/modules/zsh_test.go b/engine/modules/zsh_test.go new file mode 100644 index 0000000..5ab44db --- /dev/null +++ b/engine/modules/zsh_test.go @@ -0,0 +1,46 @@ +package modules + +import "testing" + +func TestZshDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "zsh:\n enable: false\n") + if got := Zsh(cfg); len(got) != 0 { + t.Fatalf("expected no resources when disabled, got %v", got) + } +} + +func TestZshEnabledByDefaultProducesThreeResources(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := Zsh(cfg) + // zshrc.devboost, include block, atuin config (use_atuin defaults true) + if len(got) != 3 { + t.Fatalf("expected 3 resources, got %d: %v", len(got), got) + } +} + +func TestZshOmitsAtuinConfigWhenDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "zsh:\n history:\n use_atuin: false\n") + got := Zsh(cfg) + if len(got) != 2 { + t.Fatalf("expected 2 resources (no atuin config), got %d: %v", len(got), got) + } + for _, r := range got { + if r.ID == "atuin_config" { + t.Fatal("expected no atuin_config resource when use_atuin is false") + } + } +} + +func TestZshIncludeBlockDependsOnDevboostFile(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := Zsh(cfg) + for _, r := range got { + if r.ID == "zshrc_include_block" { + if len(r.DependsOn) != 1 || r.DependsOn[0] != "zshrc_devboost" { + t.Fatalf("expected dependency on zshrc_devboost, got %v", r.DependsOn) + } + return + } + } + t.Fatal("expected a zshrc_include_block resource") +} diff --git a/engine/modules/zshdevboost.go b/engine/modules/zshdevboost.go new file mode 100644 index 0000000..9999c3b --- /dev/null +++ b/engine/modules/zshdevboost.go @@ -0,0 +1,104 @@ +package modules + +import ( + "fmt" + "strings" + + "github.com/rolfsormo/devboost/config" +) + +// renderAtuinConfig ports db_render_atuin_config. +func renderAtuinConfig(cfg *config.Config) string { + filterMode := cfg.Get("zsh.history.atuin.filter_mode", "directory") + return "# Generated by devboost - DO NOT EDIT MANUALLY\n" + + "# Atuin filter mode: controls how command history is shared between shells\n" + + "# Options: global, host, session, directory, workspace\n" + + "# Default: directory (context-specific history for developers)\n" + + fmt.Sprintf("filter_mode = %q\n", filterMode) +} + +// renderZshDevboost ports db_render_zsh_devboost: builds the entire +// .zshrc.devboost content as a sequence of config-gated sections, in the +// same order as the bash version's cat/echo composition. +func renderZshDevboost(cfg *config.Config) string { + znapPath := cfg.Get("zsh.znap_path", "~/.zsh-snap") + enableStarship := cfg.Get("prompt.enable_starship", "true") == "true" + starshipConfig := cfg.Get("prompt.starship_config", "~/.config/starship.toml") + useAtuin := cfg.Get("zsh.history.use_atuin", "true") == "true" + fzfEnable := cfg.Get("zsh.fzf.enable", "true") == "true" + fzfFiles := cfg.Get("zsh.fzf.default_command_files", "fd --type f --hidden --follow --exclude .git") + fzfDirs := cfg.Get("zsh.fzf.default_command_dirs", "fd --type d --hidden --follow --exclude .git") + enableMise := cfg.Get("toolchains.enable_mise", "true") == "true" + enableDirenv := cfg.Get("direnv.enable", "true") == "true" + clicolor := cfg.Get("aesthetics.clicolor", "true") == "true" + lscColours := cfg.Get("aesthetics.lsc_colours", "ExFxCxDxBxegedabagacad") + aliasesEnable := cfg.Get("zsh.aliases.enable", "true") == "true" + + var b strings.Builder + + b.WriteString("# Generated by devboost - DO NOT EDIT MANUALLY\n") + b.WriteString("export EDITOR=\"nvim\"\n") + b.WriteString("export LANG=\"en_US.UTF-8\"\n\n") + b.WriteString("setopt HIST_IGNORE_ALL_DUPS HIST_REDUCE_BLANKS SHARE_HISTORY INC_APPEND_HISTORY\n") + b.WriteString("setopt AUTO_CD NO_BEEP\n\n") + b.WriteString("# znap owns completion init: it redefines compinit/compdef as no-ops and\n") + b.WriteString("# runs its own deferred, precmd-hook-based compinit after loading (see\n") + b.WriteString("# ~/.zsh-snap/scripts/init.zsh). Calling compinit here ourselves, before\n") + b.WriteString("# znap is sourced, would run a second full completion pass into a\n") + b.WriteString("# different dumpfile — pure redundant cost with no effect (znap's\n") + b.WriteString("# no-op override discards any completions we'd have registered anyway).\n") + b.WriteString("# znap\n") + fmt.Fprintf(&b, "source %q\n\n", znapPath+"/znap.zsh") + + b.WriteString("# prompt\n") + if enableStarship { + fmt.Fprintf(&b, "export STARSHIP_CONFIG=%q\n", starshipConfig) + b.WriteString(`eval "$(starship init zsh)"` + "\n") + } + + b.WriteString("\n# plugins\n") + b.WriteString("znap source zsh-users/zsh-autosuggestions\n") + b.WriteString("znap source zsh-users/zsh-syntax-highlighting\n\n") + b.WriteString("# nav/search/history\n") + b.WriteString(`eval "$(zoxide init zsh)"` + "\n") + + if useAtuin { + b.WriteString(`eval "$(atuin init zsh)"` + "\n") + } + if fzfEnable { + b.WriteString(`eval "$(fzf --zsh 2>/dev/null || /opt/homebrew/bin/fzf --zsh 2>/dev/null || true)"` + "\n") + fmt.Fprintf(&b, "export FZF_DEFAULT_COMMAND=%q\n", fzfFiles) + b.WriteString(`export FZF_CTRL_T_COMMAND="$FZF_DEFAULT_COMMAND"` + "\n") + fmt.Fprintf(&b, "export FZF_ALT_C_COMMAND=%q\n", fzfDirs) + } + + b.WriteString("\n# toolchains & per-project env\n") + if enableMise { + b.WriteString(`eval "$(mise activate zsh)"` + "\n") + } + if enableDirenv { + b.WriteString(`eval "$(direnv hook zsh)"` + "\n") + } + + b.WriteString("\n# aesthetics\n") + if clicolor { + b.WriteString("export CLICOLOR=1\n") + } + fmt.Fprintf(&b, "export LSCOLORS=%q\n", lscColours) + + b.WriteString("\n# aliases\n") + if aliasesEnable { + b.WriteString(`alias ls='eza -alg --git --group --time-style=relative'` + "\n") + b.WriteString(`alias cat='bat -pp'` + "\n") + b.WriteString(`alias grep='rg'` + "\n") + b.WriteString(`alias find='fd'` + "\n") + b.WriteString(`alias du='dust'` + "\n") + b.WriteString(`alias df='duf'` + "\n") + b.WriteString(`alias ps='procs'` + "\n") + b.WriteString(`alias lg='lazygit'` + "\n") + b.WriteString(`alias tm='tmux attach -t main || tmux new -s main'` + "\n") + b.WriteString(`alias please='sudo $(fc -ln -1)'` + "\n") + } + + return b.String() +} diff --git a/engine/modules/zshdevboost_test.go b/engine/modules/zshdevboost_test.go new file mode 100644 index 0000000..76519b8 --- /dev/null +++ b/engine/modules/zshdevboost_test.go @@ -0,0 +1,94 @@ +package modules + +import ( + "strings" + "testing" +) + +func TestRenderZshDevboostDefaults(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := renderZshDevboost(cfg) + for _, want := range []string{ + "znap.zsh", + "starship init zsh", + "zsh-autosuggestions", + "atuin init zsh", + "fzf --zsh", + "mise activate zsh", + "direnv hook zsh", + "CLICOLOR=1", + "alias ls=", + } { + if !strings.Contains(got, want) { + t.Fatalf("expected default-enabled output to contain %q, got:\n%s", want, got) + } + } +} + +func TestRenderZshDevboostDisablesEachSection(t *testing.T) { + cfg := loadFixtureConfig(t, ` +prompt: + enable_starship: false +zsh: + history: + use_atuin: false + fzf: + enable: false + aliases: + enable: false +toolchains: + enable_mise: false +direnv: + enable: false +aesthetics: + clicolor: false +`) + got := renderZshDevboost(cfg) + for _, unwanted := range []string{ + "starship init zsh", + "atuin init zsh", + "fzf --zsh", + "mise activate zsh", + "direnv hook zsh", + "CLICOLOR=1", + "alias ls=", + } { + if strings.Contains(got, unwanted) { + t.Fatalf("expected disabled section to be absent, found %q in:\n%s", unwanted, got) + } + } + // znap/zoxide/plugins are unconditional in the bash version — always present. + for _, want := range []string{"znap.zsh", "zoxide init zsh", "zsh-autosuggestions"} { + if !strings.Contains(got, want) { + t.Fatalf("expected unconditional section present, missing %q", want) + } + } +} + +func TestRenderZshDevboostNeverCallsCompinitDirectly(t *testing.T) { + // Regression guard for the real compinit-duplication bug fixed + // earlier this session (module_zsh.sh used to call compinit itself + // before sourcing znap, which redefines it as a no-op — pure wasted + // work). The Go port must not reintroduce it. + cfg := loadFixtureConfig(t, "") + got := renderZshDevboost(cfg) + if strings.Contains(got, "compinit -u") || strings.Contains(got, "autoload -Uz compinit") { + t.Fatalf("expected no direct compinit call (znap owns it), got:\n%s", got) + } +} + +func TestRenderAtuinConfigDefault(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := renderAtuinConfig(cfg) + if !strings.Contains(got, `filter_mode = "directory"`) { + t.Fatalf("expected directory as default filter_mode, got %q", got) + } +} + +func TestRenderAtuinConfigCustomFilterMode(t *testing.T) { + cfg := loadFixtureConfig(t, "zsh:\n history:\n atuin:\n filter_mode: global\n") + got := renderAtuinConfig(cfg) + if !strings.Contains(got, `filter_mode = "global"`) { + t.Fatalf("expected configured filter_mode, got %q", got) + } +} diff --git a/engine/modules/zshinclude.go b/engine/modules/zshinclude.go new file mode 100644 index 0000000..64a6a6c --- /dev/null +++ b/engine/modules/zshinclude.go @@ -0,0 +1,87 @@ +package modules + +import ( + "fmt" + "os" + "regexp" + "strings" + + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +const ( + zshIncludeStart = "# >>> devboost include start" + zshIncludeEnd = "# <<< devboost include end" +) + +// zshUnmarkedSourceRe matches a live (non-comment) line sourcing +// .zshrc.devboost — ports the bash version's +// grep -Eq '(^|[^#].*)\.zshrc\.devboost'. +var zshUnmarkedSourceRe = regexp.MustCompile(`(^|[^#].*)\.zshrc\.devboost`) + +// zshIncludeBlock is module-local, not a general resource kind: its +// behavior (detect a pre-existing unmarked line sourcing the same file +// and refuse to inject rather than double-source it) is specific to this +// one situation, not a shape any other module needs. Still fully typed +// and diff-based like any resource kind — "module-local" is about where +// it's registered, not about being any less rigorous than a promoted +// kind. +type zshIncludeBlock struct { + zshrcPath string +} + +func (z zshIncludeBlock) Diff() (*engine.PendingOp, error) { + block := zshIncludeStart + "\n" + + `[ -f "$HOME/.zshrc.devboost" ] && source "$HOME/.zshrc.devboost"` + "\n" + + zshIncludeEnd + "\n" + + data, err := os.ReadFile(z.zshrcPath) + if os.IsNotExist(err) { + return &engine.PendingOp{ + Description: fmt.Sprintf("create %s with devboost include block", z.zshrcPath), + Execute: func() error { + return os.WriteFile(z.zshrcPath, []byte(block), 0o644) + }, + }, nil + } + if err != nil { + return nil, fmt.Errorf("read %s: %w", z.zshrcPath, err) + } + content := string(data) + + if strings.Contains(content, zshIncludeStart) { + return nil, nil // already present, nothing to do + } + + if zshUnmarkedSourceRe.MatchString(content) { + // An unmarked line already sources .zshrc.devboost — likely left + // over from a prior manual edit or migrate-from-oh-my-zsh + // recovery. Appending our own marked block on top would source + // it twice on every shell start. This resource has nothing safe + // to converge to here — surface it as a warning-shaped pending op + // with no Execute, so plan/apply can show it without silently + // double-sourcing (mirrors the bash version's db_log_warn + skip). + return &engine.PendingOp{ + Description: fmt.Sprintf( + "WARNING: %s already has an unmarked line sourcing .zshrc.devboost — "+ + "skipping include-block injection to avoid double-sourcing it. "+ + "Remove that line and re-run apply.", z.zshrcPath), + Execute: func() error { return nil }, // acknowledge, do nothing + }, nil + } + + return &engine.PendingOp{ + Description: fmt.Sprintf("append devboost include block to %s", z.zshrcPath), + Execute: func() error { + if err := kinds.BackupFile(z.zshrcPath); err != nil { + return err + } + sep := "\n" + if strings.HasSuffix(content, "\n") { + sep = "" + } + return os.WriteFile(z.zshrcPath, []byte(content+sep+"\n"+block), 0o644) + }, + }, nil +} diff --git a/engine/modules/zshinclude_test.go b/engine/modules/zshinclude_test.go new file mode 100644 index 0000000..76a4d7d --- /dev/null +++ b/engine/modules/zshinclude_test.go @@ -0,0 +1,168 @@ +package modules + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestZshIncludeBlockCreatesFileWhenAbsent(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".zshrc") + + z := zshIncludeBlock{zshrcPath: path} + op, err := z.Diff() + if err != nil || op == nil { + t.Fatalf("op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + data, _ := os.ReadFile(path) + if !strings.Contains(string(data), zshIncludeStart) { + t.Fatalf("expected include block in created file, got %q", data) + } +} + +func TestZshIncludeBlockAppendsWhenAbsentFromExistingFile(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".zshrc") + if err := os.WriteFile(path, []byte("existing stuff\n"), 0o644); err != nil { + t.Fatal(err) + } + + z := zshIncludeBlock{zshrcPath: path} + op, err := z.Diff() + if err != nil || op == nil { + t.Fatalf("op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + data, _ := os.ReadFile(path) + got := string(data) + if !strings.Contains(got, "existing stuff") || !strings.Contains(got, zshIncludeStart) { + t.Fatalf("expected both existing content and include block, got %q", got) + } +} + +func TestZshIncludeBlockNilWhenAlreadyPresent(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".zshrc") + + z := zshIncludeBlock{zshrcPath: path} + op, err := z.Diff() + if err != nil || op == nil { + t.Fatalf("setup: op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("setup execute: %v", err) + } + + op, err = z.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op != nil { + t.Fatalf("expected no pending op once present, got %+v", op) + } +} + +// TestZshIncludeBlockWarnsInsteadOfDoubleSourcing is the load-bearing +// test for this module's whole reason to exist as a bespoke kind: an +// unmarked pre-existing line already sourcing .zshrc.devboost (e.g. left +// over from a manual edit or oh-my-zsh migration recovery) must not get +// a second, marked block appended on top — that would source +// .zshrc.devboost twice on every shell start, doubling the cost of +// everything in it. This was a real production bug fixed earlier in the +// bash tool; the Go port must not regress it. +func TestZshIncludeBlockWarnsInsteadOfDoubleSourcing(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".zshrc") + unmarkedLine := `[ -f "$HOME/.zshrc.devboost" ] && source "$HOME/.zshrc.devboost"` + "\n" + if err := os.WriteFile(path, []byte(unmarkedLine), 0o644); err != nil { + t.Fatal(err) + } + + z := zshIncludeBlock{zshrcPath: path} + op, err := z.Diff() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op == nil { + t.Fatal("expected a pending op describing the conflict, not nil") + } + if !strings.Contains(op.Description, "WARNING") { + t.Fatalf("expected a warning description, got %q", op.Description) + } + if err := op.Execute(); err != nil { + t.Fatalf("execute (acknowledge-only) should not error: %v", err) + } + + data, _ := os.ReadFile(path) + got := string(data) + if got != unmarkedLine { + t.Fatalf("expected the file to be left byte-for-byte unchanged, got %q", got) + } + if strings.Contains(got, zshIncludeStart) { + t.Fatalf("expected no marked block to be injected (would double-source), got %q", got) + } +} + +// TestZshIncludeBlockRegexMatchesEvenInLooseComments documents an actual +// weakness inherited from the bash version's own detection regex +// ((^|[^#].*)\.zshrc\.devboost): [^#] only requires the character +// immediately before the match to not be '#', so a comment like +// "# see .zshrc.devboost for details" (space before the match, not '#') +// still matches and triggers the same warn-and-skip path, even though +// it's just a comment, not a real unmarked source line. This is existing +// bash behavior (confirmed by running the actual grep -Eq command), not +// something introduced by the port — ported faithfully rather than +// silently "fixed," since unlike the unambiguous node_modules bug, this +// one is a real product-behavior question (how strict should comment +// detection be?) worth a deliberate decision, not a silent unilateral +// change during a port. +func TestZshIncludeBlockRegexMatchesEvenInLooseComments(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".zshrc") + commentOnly := "# see .zshrc.devboost for details\n" + if err := os.WriteFile(path, []byte(commentOnly), 0o644); err != nil { + t.Fatal(err) + } + + z := zshIncludeBlock{zshrcPath: path} + op, err := z.Diff() + if err != nil || op == nil { + t.Fatalf("op=%+v err=%v", op, err) + } + if !strings.Contains(op.Description, "WARNING") { + t.Fatalf("expected the (imperfect, faithfully-ported) regex to still flag this as a false positive, got %q", op.Description) + } +} + +func TestZshIncludeBlockDoesNotFlagDirectlyCommentedOutLine(t *testing.T) { + // The one comment shape the bash regex DOES correctly exclude: '#' + // immediately adjacent to the match, no characters in between. + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".zshrc") + commentOnly := "#.zshrc.devboost\n" + if err := os.WriteFile(path, []byte(commentOnly), 0o644); err != nil { + t.Fatal(err) + } + + z := zshIncludeBlock{zshrcPath: path} + op, err := z.Diff() + if err != nil || op == nil { + t.Fatalf("op=%+v err=%v", op, err) + } + if strings.Contains(op.Description, "WARNING") { + t.Fatalf("expected normal append for a directly-adjacent-# comment, got %q", op.Description) + } +} From 91c024feed128c7bbdb8662a8e46104d5eebcb3e Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:32:19 +0300 Subject: [PATCH 12/48] feat(modules): zinit_znap, asdf_mise, nvm_mise dedup (split from legacy_shell) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports modules/module_legacy_shell.sh as three separate modules, per the architecture doc's tool-first grouping decision (v1's flat redundancy-pair split would read as an unscannable list once dozens of dedup checks accumulate across many tools). Each module owns one pattern, targeting the file the bash version actually used per migration_id (zinit/asdf against ~/.zshrc, nvm against ~/.zprofile — covered by a dedicated test asserting the nvm module doesn't accidentally look at .zshrc). All three reuse the same LineInFile kind built earlier; the shared marker/backup mechanism from core_legacy_shell.sh already lives there. Verified all three regex patterns (zinit dup, asdf source, nvm source) compile and match correctly under Go's RE2 engine before committing to them — confirms the architecture doc's claim that the grep/awk dialect-mismatch risk from the bash version genuinely has no equivalent here, not just asserted. Exports kinds.MarkerFor (was unexported) so callers outside the package — these tests, and eventually clean/doctor — can construct the exact marker string without duplicating the format. --- engine/kinds/lineinfile.go | 11 ++- engine/kinds/lineinfile_test.go | 6 +- engine/modules/asdf_mise_dedup.go | 34 +++++++ engine/modules/dedup_test.go | 138 +++++++++++++++++++++++++++++ engine/modules/nvm_mise_dedup.go | 42 +++++++++ engine/modules/zinit_znap_dedup.go | 80 +++++++++++++++++ 6 files changed, 304 insertions(+), 7 deletions(-) create mode 100644 engine/modules/asdf_mise_dedup.go create mode 100644 engine/modules/dedup_test.go create mode 100644 engine/modules/nvm_mise_dedup.go create mode 100644 engine/modules/zinit_znap_dedup.go diff --git a/engine/kinds/lineinfile.go b/engine/kinds/lineinfile.go index 6594147..b8b12fa 100644 --- a/engine/kinds/lineinfile.go +++ b/engine/kinds/lineinfile.go @@ -11,13 +11,16 @@ import ( "github.com/rolfsormo/devboost/engine" ) -// markerPrefix and markerFor mirror the bash tool's +// markerPrefix and MarkerFor mirror the bash tool's // _DB_LEGACY_MARKER_PREFIX/_db_legacy_marker_for exactly, so a file marked // by the bash version and later processed by this Go version (or vice // versa, during the migration period) is read identically by both. +// Exported so callers outside this package (tests, a future doctor/clean +// implementation) can construct the exact marker string for a migration +// ID without duplicating the format. const markerPrefix = "# devboost:disabled:" -func markerFor(migrationID string) string { +func MarkerFor(migrationID string) string { return markerPrefix + migrationID + " " } @@ -61,7 +64,7 @@ func (l LineInFile) wasEverMarked() (bool, error) { if err != nil { return false, err } - return strings.Contains(string(data), markerFor(l.MigrationID)), nil + return strings.Contains(string(data), MarkerFor(l.MigrationID)), nil } func (l LineInFile) Diff() (*engine.PendingOp, error) { @@ -78,7 +81,7 @@ func (l LineInFile) Diff() (*engine.PendingOp, error) { return nil, fmt.Errorf("read %s: %w", l.Path, err) } - marker := markerFor(l.MigrationID) + marker := MarkerFor(l.MigrationID) lines := splitLines(string(data)) var toDisable []int for i, line := range lines { diff --git a/engine/kinds/lineinfile_test.go b/engine/kinds/lineinfile_test.go index 5267687..3962d8d 100644 --- a/engine/kinds/lineinfile_test.go +++ b/engine/kinds/lineinfile_test.go @@ -48,7 +48,7 @@ func TestLineInFileDiffPendingWhenUnmarkedMatchExists(t *testing.T) { data, _ := os.ReadFile(path) got := string(data) - if !strings.Contains(got, markerFor("zinit-znap-dup")+"zinit light") { + if !strings.Contains(got, MarkerFor("zinit-znap-dup")+"zinit light") { t.Fatalf("expected disabled line to carry the marker, got %q", got) } if !strings.Contains(got, "mise activate zsh") { @@ -101,7 +101,7 @@ func TestLineInFileRespectsManualRestore(t *testing.T) { // User manually removes the marker prefix, restoring the line. data, _ := os.ReadFile(path) - restored := strings.ReplaceAll(string(data), markerFor("zinit-znap-dup"), "") + restored := strings.ReplaceAll(string(data), MarkerFor("zinit-znap-dup"), "") if err := os.WriteFile(path, []byte(restored), 0o644); err != nil { t.Fatal(err) } @@ -136,7 +136,7 @@ func TestLineInFileLeavesNonMatchingLinesAlone(t *testing.T) { data, _ := os.ReadFile(path) got := string(data) - if strings.Contains(got, markerFor("zinit-znap-dup")+"zinit light zsh-users/zsh-completions") { + if strings.Contains(got, MarkerFor("zinit-znap-dup")+"zinit light zsh-users/zsh-completions") { t.Fatalf("expected the non-matching completions line to stay unmarked, got %q", got) } if !strings.Contains(got, "\nzinit light zsh-users/zsh-completions") && !strings.HasPrefix(got, "zinit light zsh-users/zsh-completions") { diff --git a/engine/modules/asdf_mise_dedup.go b/engine/modules/asdf_mise_dedup.go new file mode 100644 index 0000000..d018c03 --- /dev/null +++ b/engine/modules/asdf_mise_dedup.go @@ -0,0 +1,34 @@ +package modules + +import ( + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +// asdfSourcePattern matches the line sourcing asdf's shell integration — +// ports DB_LEGACY_ASDF_SOURCE_PATTERN exactly. +const asdfSourcePattern = `(^|[[:space:]])\. .*/asdf\.sh([[:space:]]|$)` + +// AsdfMiseDedup ports the asdf-mise-dup half of +// modules/module_legacy_shell.sh: asdf sourced alongside devboost's own +// mise, disabled in place, never deleted. +func AsdfMiseDedup(cfg *config.Config) []engine.Resource { + if cfg.Get("legacy_shell.enable", "true") != "true" { + return nil + } + zshrc := legacyShellZshrc(cfg) + if !fileHasMatch(zshrc, asdfSourcePattern) { + return nil + } + return []engine.Resource{ + { + ID: "asdf_mise_dedup", + Kind: kinds.LineInFile{ + Path: zshrc, + Pattern: asdfSourcePattern, + MigrationID: "asdf-mise-dup", + }, + }, + } +} diff --git a/engine/modules/dedup_test.go b/engine/modules/dedup_test.go new file mode 100644 index 0000000..ed37756 --- /dev/null +++ b/engine/modules/dedup_test.go @@ -0,0 +1,138 @@ +package modules + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/rolfsormo/devboost/engine/kinds" +) + +func TestZinitZnapDedupNoResourceWhenNoMatch(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + writeFile(t, filepath.Join(home, ".zshrc"), "eval \"$(mise activate zsh)\"\n") + + cfg := loadFixtureConfig(t, "") + if got := ZinitZnapDedup(cfg); len(got) != 0 { + t.Fatalf("expected no resources when no zinit duplication present, got %v", got) + } +} + +func TestZinitZnapDedupDetectsDuplicate(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + writeFile(t, filepath.Join(home, ".zshrc"), + "zinit light zdharma-continuum/fast-syntax-highlighting\n"+ + "zinit light zsh-users/zsh-autosuggestions\n"+ + "zinit light zsh-users/zsh-completions\n") + + cfg := loadFixtureConfig(t, "") + got := ZinitZnapDedup(cfg) + if len(got) != 1 { + t.Fatalf("expected exactly one resource, got %d", len(got)) + } + + op, err := got[0].Kind.Diff() + if err != nil || op == nil { + t.Fatalf("op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + + data, _ := os.ReadFile(filepath.Join(home, ".zshrc")) + got2 := string(data) + if !containsAll(got2, + kinds.MarkerFor("zinit-znap-dup")+"zinit light zdharma-continuum/fast-syntax-highlighting", + kinds.MarkerFor("zinit-znap-dup")+"zinit light zsh-users/zsh-autosuggestions", + ) { + t.Fatalf("expected both duplicate lines disabled, got %q", got2) + } + if containsAll(got2, kinds.MarkerFor("zinit-znap-dup")+"zinit light zsh-users/zsh-completions") { + t.Fatalf("expected the non-duplicate completions line to stay untouched, got %q", got2) + } +} + +func TestAsdfMiseDedupDetectsDuplicate(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + writeFile(t, filepath.Join(home, ".zshrc"), + ". /opt/homebrew/opt/asdf/libexec/asdf.sh\n"+ + "eval \"$(mise activate zsh)\"\n") + + cfg := loadFixtureConfig(t, "") + got := AsdfMiseDedup(cfg) + if len(got) != 1 { + t.Fatalf("expected exactly one resource, got %d", len(got)) + } +} + +func TestAsdfMiseDedupNoResourceWhenNoMatch(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + writeFile(t, filepath.Join(home, ".zshrc"), "eval \"$(mise activate zsh)\"\n") + + cfg := loadFixtureConfig(t, "") + if got := AsdfMiseDedup(cfg); len(got) != 0 { + t.Fatalf("expected no resources, got %v", got) + } +} + +func TestNvmMiseDedupTargetsZprofileNotZshrc(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + // nvm's dedup targets ~/.zprofile — put the redundant line ONLY there, + // confirming this module doesn't (incorrectly) look at .zshrc instead. + writeFile(t, filepath.Join(home, ".zprofile"), + `export NVM_DIR="$HOME/.nvm"`+"\n"+ + `[ -s "/opt/homebrew/opt/nvm/nvm.sh" ] && \. "/opt/homebrew/opt/nvm/nvm.sh"`+"\n") + + cfg := loadFixtureConfig(t, "") + got := NvmMiseDedup(cfg) + if len(got) != 1 { + t.Fatalf("expected exactly one resource, got %d", len(got)) + } + + op, err := got[0].Kind.Diff() + if err != nil || op == nil { + t.Fatalf("op=%+v err=%v", op, err) + } + if err := op.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + + data, _ := os.ReadFile(filepath.Join(home, ".zprofile")) + got2 := string(data) + if !containsAll(got2, "NVM_DIR") { + t.Fatalf("expected NVM_DIR export to remain (harmless, not the expensive part), got %q", got2) + } + if !containsAll(got2, kinds.MarkerFor("nvm-mise-dup")) { + t.Fatalf("expected the nvm source line to be disabled, got %q", got2) + } +} + +func TestDedupModulesDisabledByLegacyShellEnable(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + writeFile(t, filepath.Join(home, ".zshrc"), "zinit light zsh-users/zsh-autosuggestions\n") + writeFile(t, filepath.Join(home, ".zprofile"), `[ -s "/x/nvm.sh" ] && \. "/x/nvm.sh"`+"\n") + + cfg := loadFixtureConfig(t, "legacy_shell:\n enable: false\n") + if got := ZinitZnapDedup(cfg); len(got) != 0 { + t.Fatalf("expected no resources when legacy_shell disabled, got %v", got) + } + if got := NvmMiseDedup(cfg); len(got) != 0 { + t.Fatalf("expected no resources when legacy_shell disabled, got %v", got) + } +} + +func containsAll(haystack string, needles ...string) bool { + for _, n := range needles { + if !strings.Contains(haystack, n) { + return false + } + } + return true +} diff --git a/engine/modules/nvm_mise_dedup.go b/engine/modules/nvm_mise_dedup.go new file mode 100644 index 0000000..2fb8b22 --- /dev/null +++ b/engine/modules/nvm_mise_dedup.go @@ -0,0 +1,42 @@ +package modules + +import ( + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +// nvmSourcePattern matches lines sourcing nvm's shell hook or its +// bash-completion shim — ports DB_LEGACY_NVM_SOURCE_PATTERN exactly, +// deliberately matching on the quoted path alone (not a leading +// backslash/source token) since that's what kept the original bash +// pattern grep/awk-dialect-safe; no such constraint applies in Go (RE2 is +// the only engine here), but there's no reason to diverge from a pattern +// that already works correctly. +const nvmSourcePattern = `"[^"]*/nvm(\.sh|/etc/bash_completion\.d/nvm)"` + +// NvmMiseDedup ports the nvm-mise-dup half of +// modules/module_legacy_shell.sh: nvm's shell hook (measured at +// ~850-900ms per login shell — the dominant real-world contributor to +// the startup-lag investigation this whole dedup mechanism came from) +// sourced in ~/.zprofile alongside devboost's own mise, disabled in +// place. +func NvmMiseDedup(cfg *config.Config) []engine.Resource { + if cfg.Get("legacy_shell.enable", "true") != "true" { + return nil + } + zprofile := cfg.Get("legacy_shell.zprofile", "~/.zprofile") + if !fileHasMatch(zprofile, nvmSourcePattern) { + return nil + } + return []engine.Resource{ + { + ID: "nvm_mise_dedup", + Kind: kinds.LineInFile{ + Path: zprofile, + Pattern: nvmSourcePattern, + MigrationID: "nvm-mise-dup", + }, + }, + } +} diff --git a/engine/modules/zinit_znap_dedup.go b/engine/modules/zinit_znap_dedup.go new file mode 100644 index 0000000..29851bf --- /dev/null +++ b/engine/modules/zinit_znap_dedup.go @@ -0,0 +1,80 @@ +package modules + +import ( + "os" + "path/filepath" + "regexp" + + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +// zinitDupPattern matches zinit lines loading plugins znap's default +// config already loads — ports module_legacy_shell.sh's +// DB_LEGACY_ZINIT_DUP_PATTERN exactly. +const zinitDupPattern = `^[[:space:]]*zinit (light|load)[^#]*(zsh-users/zsh-autosuggestions|zdharma-continuum/fast-syntax-highlighting|zsh-users/zsh-syntax-highlighting)` + +// ZinitZnapDedup ports the zinit-znap-dup half of modules/module_legacy_shell.sh: +// detects a pre-existing zinit setup loading plugins znap already +// provides, and disables the redundant lines in place (never deletes — +// see LineInFile). This is one of the three per-tool dedup modules split +// out of the original single legacy_shell module, per the architecture +// doc's tool-first grouping decision. +func ZinitZnapDedup(cfg *config.Config) []engine.Resource { + if cfg.Get("legacy_shell.enable", "true") != "true" { + return nil + } + zshrc := legacyShellZshrc(cfg) + if !fileHasMatch(zshrc, zinitDupPattern) { + return nil + } + return []engine.Resource{ + { + ID: "zinit_znap_dedup", + Kind: kinds.LineInFile{ + Path: zshrc, + Pattern: zinitDupPattern, + MigrationID: "zinit-znap-dup", + }, + }, + } +} + +// legacyShellZshrc ports _db_legacy_shell_zshrc's default — hardcoded to +// ~/.zshrc in the bash version, never actually config-driven despite the +// db_yaml_get call (no module ever set .legacy_shell.zshrc). +func legacyShellZshrc(cfg *config.Config) string { + return cfg.Get("legacy_shell.zshrc", "~/.zshrc") +} + +// fileHasMatch is a cheap presence pre-check so a module can decide +// whether to declare a LineInFile resource at all — mirrors the bash +// module's own _db_legacy_shell_*_present gating before calling +// _db_legacy_disable_lines. Not strictly required (LineInFile.Diff +// already returns nil if nothing matches), but keeps doctor/plan from +// declaring a resource whose kind will just immediately no-op, which +// matters once doctor groups output by module (task #15) — a module +// with zero declared resources reads as "nothing to check here," not +// "checked and found nothing," a real distinction once there's tool-first +// grouping to report against. +func fileHasMatch(path, pattern string) bool { + data, err := os.ReadFile(expandHomeForLegacyCheck(path)) + if err != nil { + return false + } + return regexp.MustCompile(pattern).Match(data) +} + +func expandHomeForLegacyCheck(path string) string { + // LineInFile/config.Get already expand ~ before this is called in + // practice (legacyShellZshrc goes through cfg.Get), but guard anyway + // since fileHasMatch could be called with a raw path in tests. + if len(path) >= 2 && path[0] == '~' && path[1] == '/' { + home, err := os.UserHomeDir() + if err == nil { + return filepath.Join(home, path[2:]) + } + } + return path +} From 66fdc8138619527abbe9392e45a4d49c36f3bcc8 Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:34:15 +0300 Subject: [PATCH 13/48] feat(engine,modules): Diagnostic mechanism, security module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds engine.Diagnostic/DiagnosticFunc — a deliberately separate concept from Resource/PendingOp for read-only findings with nothing to converge (a toolchain pinned to 'latest', oh-my-zsh present, a double-sourced .zshrc). Earlier modules (zsh's include-block conflict) used a PendingOp with a no-op Execute for a similar warning-shaped case, but that doesn't generalize: a pure diagnostic was never a pending *change*, so folding it into PendingOp would make apply's "N changes made" accounting lie, and a module with only diagnostics (nothing to install/fix) is a genuinely different shape from a module with nothing to report at all. Diagnostics lets that distinction be real. Ports modules/module_security.sh: the devboost-check alias block (apply-side, a normal BlockInFile resource) and five doctor-only checks (apply-side has nothing to do with these — latest-pinned toolchains, oh-my-zsh presence, double-sourced .zshrc, TPM over HTTP, stale Homebrew index), now expressed as Diagnostics rather than fake resources. --- engine/diagnostic.go | 28 +++++ engine/modules/security.go | 175 ++++++++++++++++++++++++++++++++ engine/modules/security_test.go | 125 +++++++++++++++++++++++ 3 files changed, 328 insertions(+) create mode 100644 engine/diagnostic.go create mode 100644 engine/modules/security.go create mode 100644 engine/modules/security_test.go diff --git a/engine/diagnostic.go b/engine/diagnostic.go new file mode 100644 index 0000000..70f2fa3 --- /dev/null +++ b/engine/diagnostic.go @@ -0,0 +1,28 @@ +package engine + +// Diagnostic is a read-only finding with nothing to converge — e.g. "this +// toolchain is pinned to 'latest', which pulls unreviewed releases," or +// "oh-my-zsh is installed alongside devboost, which is redundant." This +// is deliberately a separate concept from Resource/PendingOp, not another +// use of PendingOp with a no-op Execute: a Diagnostic was never a pending +// *change* in the first place, so it shouldn't appear in apply's "N +// changes made" accounting the way a real converged resource does, and a +// module offering only diagnostics (nothing to install/fix) is a +// different, legitimate shape from a module with nothing to report at +// all — Diagnostics lets that distinction be real instead of implied by +// an empty PendingOp. +// +// A DiagnosticFunc returning nil means nothing to report. +type Diagnostic struct { + Module string // which module this finding belongs to, for grouped doctor output + Message string + Warn bool // true for a warning-level finding, false for informational/success +} + +// DiagnosticFunc is what a module supplies for doctor: a function that +// inspects live system state and returns zero or more findings. Unlike +// ResourceKind.Diff, there's no "desired vs. actual" comparison implied — +// a diagnostic can report on anything worth surfacing, including things +// with no notion of convergence at all (e.g. "an unrelated tool your +// devboost setup didn't install is present"). +type DiagnosticFunc func() ([]Diagnostic, error) diff --git a/engine/modules/security.go b/engine/modules/security.go new file mode 100644 index 0000000..5baa031 --- /dev/null +++ b/engine/modules/security.go @@ -0,0 +1,175 @@ +package modules + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "time" + + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +const ( + securityStartMarker = "# >>> devboost security start" + securityEndMarker = "# <<< devboost security end" +) + +const devboostCheckAlias = `# Run 'devboost-check' at any time to see a summary of outdated tools. +devboost-check() { + echo "=== devboost security check ===" + local issues=0 + + # Homebrew outdated + if command -v brew &>/dev/null; then + local outdated + outdated=$(brew outdated --quiet 2>/dev/null | head -20) + if [[ -n "$outdated" ]]; then + echo "[brew] Outdated packages (run: brew upgrade):" + echo "$outdated" | sed 's/^/ /' + issues=$((issues + 1)) + else + echo "[brew] All packages up to date" + fi + fi + + # mise outdated toolchains + if command -v mise &>/dev/null; then + local mise_outdated + mise_outdated=$(mise outdated 2>/dev/null | grep -v "^Tool" | grep -v "^$" | head -10) + if [[ -n "$mise_outdated" ]]; then + echo "[mise] Outdated toolchains (run: mise upgrade):" + echo "$mise_outdated" | sed 's/^/ /' + issues=$((issues + 1)) + else + echo "[mise] All toolchains up to date" + fi + fi + + # npm audit (only if a package.json is in the current directory) + if command -v npm &>/dev/null && [[ -f "$(pwd)/package.json" ]]; then + echo "[npm] Running audit in $(pwd)..." + npm audit --audit-level=high 2>/dev/null || true + fi + + if [[ $issues -eq 0 ]]; then + echo "All checks passed." + else + echo "" + echo "Tip: keep tools at stable LTS, not bleeding edge — run update checks weekly." + fi +}` + +// Security ports modules/module_security.sh's apply half: injects the +// devboost-check alias block into .zshrc.devboost, gated on +// security.enable. +func Security(cfg *config.Config) []engine.Resource { + if cfg.Get("security.enable", "true") != "true" { + return nil + } + includeFile := cfg.Get("zsh.include_file", "~/.zshrc.devboost") + return []engine.Resource{ + { + ID: "security_check_alias", + Kind: kinds.BlockInFile{ + Path: includeFile, + StartMarker: securityStartMarker, + EndMarker: securityEndMarker, + Content: devboostCheckAlias, + }, + }, + } +} + +// SecurityDiagnostics ports modules/module_security.sh's doctor half: +// five read-only findings with nothing to converge — see engine.Diagnostic +// for why these are a separate mechanism from Resource/PendingOp rather +// than resources with a no-op Execute. +func SecurityDiagnostics(cfg *config.Config) engine.DiagnosticFunc { + return func() ([]engine.Diagnostic, error) { + if cfg.Get("security.enable", "true") != "true" { + return nil, nil + } + var diags []engine.Diagnostic + + for _, key := range []string{"node", "python", "go", "rust", "deno"} { + ver := cfg.Get("toolchains.globals."+key, "") + if ver == "latest" { + diags = append(diags, engine.Diagnostic{ + Module: "security", + Warn: true, + Message: fmt.Sprintf( + "toolchain '%s' is pinned to 'latest' — prefer a specific version or 'lts'/'stable' to avoid pulling unreviewed releases", + key), + }) + } + } + + home, err := os.UserHomeDir() + if err == nil { + if _, err := os.Stat(filepath.Join(home, ".oh-my-zsh")); err == nil { + diags = append(diags, engine.Diagnostic{ + Module: "security", + Warn: true, + Message: "~/.oh-my-zsh detected — this is redundant with devboost's znap+starship setup and can " + + "slow shell startup or conflict with it. Consider removing it (see README).", + }) + } + + zshrc := filepath.Join(home, ".zshrc") + if data, err := os.ReadFile(zshrc); err == nil { + count := strings.Count(string(data), ".zshrc.devboost") + if count > 1 { + diags = append(diags, engine.Diagnostic{ + Module: "security", + Warn: true, + Message: fmt.Sprintf( + "%s sources .zshrc.devboost %d times — likely double-sourced, which doubles shell startup cost. Run: grep -n zshrc.devboost %s", + zshrc, count, zshrc), + }) + } + } + } + + tpmPath := cfg.Get("tmux.tpm_path", "~/.tmux/plugins/tpm") + if _, err := os.Stat(filepath.Join(tpmPath, ".git")); err == nil { + out, err := exec.Command("git", "-C", tpmPath, "remote", "get-url", "origin").Output() + if err == nil && strings.HasPrefix(strings.TrimSpace(string(out)), "http://") { + diags = append(diags, engine.Diagnostic{ + Module: "security", + Warn: true, + Message: "TPM remote uses plain HTTP — re-clone over HTTPS", + }) + } + } + + if brewRepo, err := exec.Command("brew", "--repository").Output(); err == nil { + repoPath := filepath.Join(strings.TrimSpace(string(brewRepo)), "Library", "Taps", "homebrew", "homebrew-core") + if _, err := os.Stat(filepath.Join(repoPath, ".git")); err == nil { + out, err := exec.Command("git", "-C", repoPath, "log", "-1", "--format=%ct").Output() + if err == nil { + if lastFetch, err := strconv.ParseInt(strings.TrimSpace(string(out)), 10, 64); err == nil { + ageDays := int(time.Since(time.Unix(lastFetch, 0)).Hours() / 24) + if ageDays > 7 { + diags = append(diags, engine.Diagnostic{ + Module: "security", + Warn: true, + Message: fmt.Sprintf( + "Homebrew index is %d days old — run 'brew update' to get security patches", ageDays), + }) + } + } + } + } + } + + if len(diags) == 0 { + diags = append(diags, engine.Diagnostic{Module: "security", Message: "No obvious issues found"}) + } + return diags, nil + } +} diff --git a/engine/modules/security_test.go b/engine/modules/security_test.go new file mode 100644 index 0000000..1b4d5ec --- /dev/null +++ b/engine/modules/security_test.go @@ -0,0 +1,125 @@ +package modules + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/rolfsormo/devboost/engine/kinds" +) + +func TestSecurityDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "security:\n enable: false\n") + if got := Security(cfg); len(got) != 0 { + t.Fatalf("expected no resources when disabled, got %v", got) + } +} + +func TestSecurityEnabledByDefaultInjectsAliasBlock(t *testing.T) { + cfg := loadFixtureConfig(t, "") + got := Security(cfg) + if len(got) != 1 { + t.Fatalf("expected exactly one resource, got %d", len(got)) + } + b, ok := got[0].Kind.(kinds.BlockInFile) + if !ok { + t.Fatalf("expected kinds.BlockInFile, got %T", got[0].Kind) + } + if !strings.Contains(b.Content, "devboost-check()") { + t.Fatalf("expected devboost-check function in block content, got %q", b.Content) + } +} + +func TestSecurityDiagnosticsNoneWhenClean(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + cfg := loadFixtureConfig(t, "") + + diags, err := SecurityDiagnostics(cfg)() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(diags) != 1 || diags[0].Warn { + t.Fatalf("expected a single non-warning 'no issues' diagnostic, got %+v", diags) + } +} + +func TestSecurityDiagnosticsWarnsOnLatestPin(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + cfg := loadFixtureConfig(t, "toolchains:\n globals:\n node: latest\n") + + diags, err := SecurityDiagnostics(cfg)() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + found := false + for _, d := range diags { + if d.Warn && strings.Contains(d.Message, "'node'") && strings.Contains(d.Message, "latest") { + found = true + } + } + if !found { + t.Fatalf("expected a warning about node pinned to latest, got %+v", diags) + } +} + +func TestSecurityDiagnosticsWarnsOnOhMyZshPresent(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + if err := os.MkdirAll(filepath.Join(home, ".oh-my-zsh"), 0o755); err != nil { + t.Fatal(err) + } + cfg := loadFixtureConfig(t, "") + + diags, err := SecurityDiagnostics(cfg)() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + found := false + for _, d := range diags { + if d.Warn && strings.Contains(d.Message, "oh-my-zsh") { + found = true + } + } + if !found { + t.Fatalf("expected an oh-my-zsh warning, got %+v", diags) + } +} + +func TestSecurityDiagnosticsWarnsOnDoubleSourcedZshrc(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + writeFile(t, filepath.Join(home, ".zshrc"), + "source ~/.zshrc.devboost\nsource ~/.zshrc.devboost\n") + cfg := loadFixtureConfig(t, "") + + diags, err := SecurityDiagnostics(cfg)() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + found := false + for _, d := range diags { + if d.Warn && strings.Contains(d.Message, "double-sourced") { + found = true + } + } + if !found { + t.Fatalf("expected a double-sourced warning, got %+v", diags) + } +} + +func TestSecurityDiagnosticsDisabledReturnsNil(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + cfg := loadFixtureConfig(t, "security:\n enable: false\n") + + diags, err := SecurityDiagnostics(cfg)() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if diags != nil { + t.Fatalf("expected nil diagnostics when disabled, got %+v", diags) + } +} From 78f5c5bb58213c24f85d717ca893ad0f2b161f7e Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:42:19 +0300 Subject: [PATCH 14/48] feat(engine,cli): Doctor with tool-first grouping, module registry, full CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the module registry (engine/modules/registry.go): every module presents a uniform {Name, Resources, Diagnostics} shape regardless of its internal signature differences (Services/Pkg need OS, Security has both Resources and Diagnostics, most just need config). AllResources combines every module's desired state into the one list Plan/Apply already operate on. Doctor computes ONE combined diff across all modules together (same graph Plan/Apply use), then groups the results back by module for readable output — NOT by diffing each module in isolation, which was the first implementation and broke immediately: security's alias-block resource depends on zsh's zshrc_devboost resource (see below), and diffing security alone can never resolve a dependency on a resource that isn't in its own list. Caught by actually running the CLI's doctor command against a fresh fake HOME, not just unit tests — added a regression test for the cross-module case specifically. Real bug found and fixed while wiring the full registry together: zsh's .zshrc.devboost resource is a File (full-content overwrite); security's devboost-check alias resource is a BlockInFile (append/replace a marked block) targeting the SAME path. With no explicit ordering, running security before zsh let zsh's File silently overwrite and destroy security's already-written block — confirmed with a test exercising both orderings before adding security_check_alias's DependsOn on zshrc_devboost, which also required security to skip cleanly (not error) when zsh is disabled, since that dependency target then wouldn't exist. Rewrites cmd/devboost-v2/main.go into the real CLI: plan/apply/doctor subcommands, --config/--help/--version flags, using the registry instead of the spike's single hardcoded znap module. --dry-run, --verbose, --yes, and the uninstall/migrate-from-oh-my-zsh subcommands are intentionally not yet wired — tasks #16/#17. --- cmd/devboost-v2/main.go | 126 ++++++++++++++++-- engine/doctor.go | 67 ++++++++++ engine/doctor_test.go | 94 +++++++++++++ engine/modules/registry.go | 66 +++++++++ engine/modules/registry_test.go | 48 +++++++ engine/modules/security.go | 21 +++ engine/modules/security_test.go | 14 ++ engine/modules/znap.go | 3 - .../modules/zsh_security_interaction_test.go | 75 +++++++++++ 9 files changed, 498 insertions(+), 16 deletions(-) create mode 100644 engine/doctor.go create mode 100644 engine/doctor_test.go create mode 100644 engine/modules/registry.go create mode 100644 engine/modules/registry_test.go create mode 100644 engine/modules/zsh_security_interaction_test.go diff --git a/cmd/devboost-v2/main.go b/cmd/devboost-v2/main.go index d8ab667..5382251 100644 --- a/cmd/devboost-v2/main.go +++ b/cmd/devboost-v2/main.go @@ -1,6 +1,7 @@ -// Command devboost-v2 is the spike CLI proving the Go engine's plan/apply -// mechanism end to end against one ported module (znap). Not a full CLI -// replacement — see the v2 architecture proposal for the migration plan. +// Command devboost-v2 is the Go-engine CLI, coexisting with the bash +// tool (devboost.sh) during the v2 migration. Not yet wired into a +// bootstrap/release pipeline — see the v2 architecture proposal and +// issue #4 for the migration plan and cutover criteria. package main import ( @@ -9,30 +10,47 @@ import ( "github.com/rolfsormo/devboost/config" "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" "github.com/rolfsormo/devboost/engine/modules" ) +const usage = `devboost - Bootstrap a modern dev environment + +Usage: devboost-v2 [COMMAND] [OPTIONS] + +Commands: + apply Converge machine to config (default) + plan Show actions without changing anything + doctor Check prerequisites and report per-module findings + +Options: + --config FILE Config file path (default: ~/.devboost.yaml) + --help, -h Show this help message + --version Show version +` + +const version = "2.0.0-dev" + func main() { - if len(os.Args) < 2 { - fmt.Fprintln(os.Stderr, "usage: devboost-v2 plan|apply") - os.Exit(1) - } + cmd, configPath := parseArgs(os.Args[1:]) - cfg, err := config.Load(config.DefaultPath()) + cfg, err := config.Load(configPath) if err != nil { fmt.Fprintln(os.Stderr, "error loading config:", err) os.Exit(1) } - resources := modules.Znap(cfg) + detectedOS := kinds.DetectOS() - switch os.Args[1] { + switch cmd { case "plan": - err = engine.Plan(resources) + err = engine.Plan(modules.AllResources(cfg, detectedOS)) case "apply": - err = engine.Apply(resources) + err = engine.Apply(modules.AllResources(cfg, detectedOS)) + case "doctor": + err = runDoctor(cfg, detectedOS) default: - fmt.Fprintf(os.Stderr, "unknown command %q\n", os.Args[1]) + fmt.Fprintf(os.Stderr, "unknown command %q\n\n%s", cmd, usage) os.Exit(1) } @@ -41,3 +59,85 @@ func main() { os.Exit(1) } } + +// parseArgs is intentionally small: a subcommand token plus --config, +// mirroring the bash tool's db_parse_flags for the subset of flags this +// CLI currently supports. --dry-run/--verbose/--yes and the +// uninstall/migrate-from-oh-my-zsh subcommands are not yet wired here — +// see tasks #16/#17. +func parseArgs(args []string) (cmd string, configPath string) { + cmd = "apply" + configPath = config.DefaultPath() + + i := 0 + if len(args) > 0 { + switch args[0] { + case "apply", "plan", "doctor": + cmd = args[0] + i = 1 + case "--help", "-h": + fmt.Print(usage) + os.Exit(0) + case "--version": + fmt.Println("devboost", version) + os.Exit(0) + } + } + + for ; i < len(args); i++ { + switch args[i] { + case "--config": + if i+1 < len(args) { + configPath = args[i+1] + i++ + } + case "--help", "-h": + fmt.Print(usage) + os.Exit(0) + case "--version": + fmt.Println("devboost", version) + os.Exit(0) + } + } + return cmd, configPath +} + +// runDoctor computes and prints per-module findings, grouped by module +// name — the tool-first grouping from the architecture doc, so this +// stays readable regardless of how many resources/dedup-checks +// accumulate inside any one module over time. +func runDoctor(cfg *config.Config, os kinds.OS) error { + names := make([]string, len(modules.All)) + resourcesByModule := make([][]engine.Resource, len(modules.All)) + diagnosticsByModule := make([]engine.DiagnosticFunc, len(modules.All)) + for i, m := range modules.All { + names[i] = m.Name + resourcesByModule[i] = m.Resources(cfg, os) + if m.Diagnostics != nil { + diagnosticsByModule[i] = m.Diagnostics(cfg, os) + } + } + + reports, err := engine.Doctor(names, resourcesByModule, diagnosticsByModule) + if err != nil { + return err + } + if len(reports) == 0 { + fmt.Println("Everything looks good.") + return nil + } + for _, r := range reports { + fmt.Printf("%s:\n", r.Name) + for _, op := range r.Pending { + fmt.Printf(" ⚠ %s\n", op.Description) + } + for _, d := range r.Diagnostics { + mark := "✓" + if d.Warn { + mark = "⚠" + } + fmt.Printf(" %s %s\n", mark, d.Message) + } + } + return nil +} diff --git a/engine/doctor.go b/engine/doctor.go new file mode 100644 index 0000000..dac55ef --- /dev/null +++ b/engine/doctor.go @@ -0,0 +1,67 @@ +package engine + +import "fmt" + +// ModuleReport is one module's worth of doctor output: its pending +// resource changes (same shape plan/apply already compute, just grouped +// by module) plus any read-only Diagnostics. +type ModuleReport struct { + Name string + Pending []PendingOp + Diagnostics []Diagnostic +} + +// Doctor computes ONE combined diff across every module's resources +// together — same as Plan — then groups the resulting PendingOps back by +// which module owns each resource ID for readable, tool-first output. +// +// This must diff the combined graph, not each module in isolation: a +// resource can legitimately DependsOn another module's resource (e.g. +// security's alias-block injection depends on zsh having already +// written .zshrc.devboost, since both target the same file with +// incompatible write semantics — diffing security's resources alone +// would make that dependency unresolvable, since the resource it depends +// on wouldn't even be in the list). Grouping happens after the diff, as +// a pure reporting step, not by fragmenting the diff itself. +func Doctor(modules []string, resourcesByModule [][]Resource, diagnosticsByModule []DiagnosticFunc) ([]ModuleReport, error) { + if len(modules) != len(resourcesByModule) || len(modules) != len(diagnosticsByModule) { + return nil, fmt.Errorf("doctor: modules/resources/diagnostics length mismatch") + } + + moduleOf := make(map[string]string) // resource ID -> owning module name + var combined []Resource + for i, name := range modules { + for _, r := range resourcesByModule[i] { + moduleOf[r.ID] = name + combined = append(combined, r) + } + } + + pending, err := ComputeDiff(combined) + if err != nil { + return nil, err + } + + pendingByModule := make(map[string][]PendingOp) + for _, op := range pending { + name := moduleOf[op.ResourceID] + pendingByModule[name] = append(pendingByModule[name], op) + } + + reports := make([]ModuleReport, 0, len(modules)) + for i, name := range modules { + var diags []Diagnostic + if fn := diagnosticsByModule[i]; fn != nil { + diags, err = fn() + if err != nil { + return nil, fmt.Errorf("module %s diagnostics: %w", name, err) + } + } + modulePending := pendingByModule[name] + if len(modulePending) == 0 && len(diags) == 0 { + continue // nothing to report for this module at all + } + reports = append(reports, ModuleReport{Name: name, Pending: modulePending, Diagnostics: diags}) + } + return reports, nil +} diff --git a/engine/doctor_test.go b/engine/doctor_test.go new file mode 100644 index 0000000..bc6ea49 --- /dev/null +++ b/engine/doctor_test.go @@ -0,0 +1,94 @@ +package engine + +import "testing" + +func TestDoctorGroupsByModuleAndOmitsClean(t *testing.T) { + pendingKind := fakeKind{pending: true, desc: "fix me", ran: &[]string{}, id: "a"} + cleanKind := fakeKind{pending: false} + + modules := []string{"dirty_module", "clean_module"} + resources := [][]Resource{ + {{ID: "a", Kind: pendingKind}}, + {{ID: "b", Kind: cleanKind}}, + } + diagnostics := []DiagnosticFunc{nil, nil} + + reports, err := Doctor(modules, resources, diagnostics) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reports) != 1 { + t.Fatalf("expected only the dirty module to be reported, got %d reports: %+v", len(reports), reports) + } + if reports[0].Name != "dirty_module" { + t.Fatalf("got %q, want dirty_module", reports[0].Name) + } + if len(reports[0].Pending) != 1 { + t.Fatalf("expected one pending op, got %v", reports[0].Pending) + } +} + +func TestDoctorIncludesDiagnosticsOnlyModules(t *testing.T) { + cleanKind := fakeKind{pending: false} + modules := []string{"diag_only"} + resources := [][]Resource{{{ID: "a", Kind: cleanKind}}} + diagnostics := []DiagnosticFunc{ + func() ([]Diagnostic, error) { + return []Diagnostic{{Module: "diag_only", Message: "something worth knowing"}}, nil + }, + } + + reports, err := Doctor(modules, resources, diagnostics) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reports) != 1 { + t.Fatalf("expected the diagnostics-only module to be reported, got %d", len(reports)) + } + if len(reports[0].Pending) != 0 { + t.Fatalf("expected no pending ops, got %v", reports[0].Pending) + } + if len(reports[0].Diagnostics) != 1 { + t.Fatalf("expected one diagnostic, got %v", reports[0].Diagnostics) + } +} + +func TestDoctorErrorsOnMismatchedLengths(t *testing.T) { + _, err := Doctor([]string{"a", "b"}, [][]Resource{{}}, []DiagnosticFunc{nil}) + if err == nil { + t.Fatal("expected an error for mismatched slice lengths") + } +} + +// TestDoctorResolvesCrossModuleDependencies is a regression test for a +// real bug: an earlier version of Doctor diffed each module's resources +// in isolation, one at a time. A resource legitimately depending on +// ANOTHER module's resource (like security's alias-block injection +// depending on zsh having already written .zshrc.devboost — both target +// the same file with incompatible write semantics) then failed with +// "depends on unknown resource", because the dependency target wasn't in +// that module's own resource list. Doctor must diff the combined graph +// across all modules together, then group results by module afterward — +// exactly what Plan already does, just with an extra grouping step. +// (Doctor, like Plan, never executes anything — so this only asserts +// dependency *resolution* succeeds, not that execution effects +// propagate, which DiffAndExecute's own test already covers.) +func TestDoctorResolvesCrossModuleDependencies(t *testing.T) { + moduleA := fakeKind{pending: true, desc: "converge a"} + moduleBDependsOnA := fakeKind{pending: false} + + modules := []string{"module_a", "module_b"} + resources := [][]Resource{ + {{ID: "a", Kind: moduleA}}, + {{ID: "b", Kind: moduleBDependsOnA, DependsOn: []string{"a"}}}, + } + diagnostics := []DiagnosticFunc{nil, nil} + + reports, err := Doctor(modules, resources, diagnostics) + if err != nil { + t.Fatalf("expected the cross-module dependency to resolve, got error: %v", err) + } + if len(reports) != 1 || reports[0].Name != "module_a" { + t.Fatalf("expected only module_a to report a pending change, got %+v", reports) + } +} diff --git a/engine/modules/registry.go b/engine/modules/registry.go new file mode 100644 index 0000000..45b8938 --- /dev/null +++ b/engine/modules/registry.go @@ -0,0 +1,66 @@ +// Package modules holds the Go-engine ports of devboost's bash modules. +// This package coexists with the original bash modules/*.sh tree during +// the v2 migration; it does not replace or modify any bash file. +package modules + +import ( + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +// Module is what every module in this package presents to the CLI: +// a display Name (used to group doctor output — the tool-first grouping +// the architecture doc settled on, so 50 dedup checks across 10 tools +// still reads as 10 lines, not 50), a Resources function computing +// desired state from config and the detected OS, and an optional +// Diagnostics function for read-only findings (nil if the module has +// none). +type Module struct { + Name string + Resources func(cfg *config.Config, os kinds.OS) []engine.Resource + Diagnostics func(cfg *config.Config, os kinds.OS) engine.DiagnosticFunc +} + +// All is every module, in the same dependency-respecting order the bash +// tool's build.sh registration list encoded by hand (pkg first, since +// other modules assume tools are already installed; zsh after znap, +// since zsh's rendered config sources znap). Under the new engine this +// ordering is advisory/documentation only — the real ordering guarantee +// is each Resource's own DependsOn, not registration order — but keeping +// registration in a sensible order still matters for doctor's grouped +// output to read naturally top to bottom. +var All = []Module{ + {Name: "pkg", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return Pkg(cfg) }}, + {Name: "zsh", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return Zsh(cfg) }}, + {Name: "znap", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return Znap(cfg) }}, + {Name: "starship", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return Starship(cfg) }}, + {Name: "tmux", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return Tmux(cfg) }}, + {Name: "mise", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return Mise(cfg) }}, + {Name: "corepack", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return Corepack(cfg) }}, + {Name: "direnv", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return Direnv(cfg) }}, + {Name: "git", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return Git(cfg) }}, + {Name: "services", Resources: Services}, + { + Name: "security", + Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return Security(cfg) }, + Diagnostics: func(cfg *config.Config, os kinds.OS) engine.DiagnosticFunc { return SecurityDiagnostics(cfg) }, + }, + {Name: "zinit", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return ZinitZnapDedup(cfg) }}, + {Name: "asdf", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return AsdfMiseDedup(cfg) }}, + {Name: "nvm", Resources: func(cfg *config.Config, os kinds.OS) []engine.Resource { return NvmMiseDedup(cfg) }}, +} + +// AllResources computes every module's desired-state resources for the +// given config/OS, in registration order — the list ComputeDiff/Apply +// operate on. Resource IDs must be unique across the whole set (topoSort +// already enforces this), so module authors need to keep their IDs +// distinctly namespaced, same discipline the bash tool's flat function +// names already required. +func AllResources(cfg *config.Config, os kinds.OS) []engine.Resource { + var all []engine.Resource + for _, m := range All { + all = append(all, m.Resources(cfg, os)...) + } + return all +} diff --git a/engine/modules/registry_test.go b/engine/modules/registry_test.go new file mode 100644 index 0000000..1ce5fb0 --- /dev/null +++ b/engine/modules/registry_test.go @@ -0,0 +1,48 @@ +package modules + +import ( + "testing" + + "github.com/rolfsormo/devboost/engine" + "github.com/rolfsormo/devboost/engine/kinds" +) + +// TestAllResourcesResolveWithDefaultConfig is the real integration check +// for the whole registry: every module's DependsOn references must +// resolve when all modules run together (the actual apply/plan +// scenario), not just in isolated pairwise tests. This is exactly the +// kind of cross-module coupling (zsh/security's file-write race) that +// only surfaces when the full set runs together — catches a bad +// DependsOn ID, a missing module in the registry, or a real dependency +// cycle before it reaches a real machine. +func TestAllResourcesResolveWithDefaultConfig(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + cfg := loadFixtureConfig(t, "") + + resources := AllResources(cfg, kinds.OSDarwin) + if len(resources) == 0 { + t.Fatal("expected at least some resources with default config") + } + + // ComputeDiff runs topoSort internally — an unresolvable DependsOn or + // a cycle surfaces here as an error, not a panic or silent wrong order. + if _, err := engine.ComputeDiff(resources); err != nil { + t.Fatalf("registry produced resources that don't resolve: %v", err) + } +} + +func TestAllResourcesNoDuplicateIDs(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + cfg := loadFixtureConfig(t, "") + + resources := AllResources(cfg, kinds.OSDarwin) + seen := make(map[string]bool, len(resources)) + for _, r := range resources { + if seen[r.ID] { + t.Fatalf("duplicate resource ID %q across modules — topoSort would reject this at runtime", r.ID) + } + seen[r.ID] = true + } +} diff --git a/engine/modules/security.go b/engine/modules/security.go index 5baa031..88eaa50 100644 --- a/engine/modules/security.go +++ b/engine/modules/security.go @@ -67,10 +67,30 @@ devboost-check() { // Security ports modules/module_security.sh's apply half: injects the // devboost-check alias block into .zshrc.devboost, gated on // security.enable. +// +// This resource explicitly DependsOn zsh's zshrc_devboost resource, +// because both target the same file and have incompatible write +// semantics: zsh's is a File (full-content overwrite), security's is a +// BlockInFile (append/replace a marked block). Confirmed by test: run in +// the wrong order (security's block written first, zsh's File overwrite +// second), zsh silently destroys security's block — File has no +// awareness that BlockInFile already wrote something worth preserving. +// The dependency guarantees security always runs after zsh has already +// written the file, so BlockInFile's append-or-replace-between-markers +// logic is always operating on the real, already-current content. func Security(cfg *config.Config) []engine.Resource { if cfg.Get("security.enable", "true") != "true" { return nil } + if cfg.Get("zsh.enable", "true") != "true" { + // security's alias block targets .zshrc.devboost, which only + // exists if the zsh module is enabled — the DependsOn on + // zshrc_devboost below assumes that resource exists in the same + // combined resource list. Skip cleanly here rather than let + // topoSort surface a cryptic "depends on unknown resource" error + // for what's actually a sensible, valid config combination. + return nil + } includeFile := cfg.Get("zsh.include_file", "~/.zshrc.devboost") return []engine.Resource{ { @@ -81,6 +101,7 @@ func Security(cfg *config.Config) []engine.Resource { EndMarker: securityEndMarker, Content: devboostCheckAlias, }, + DependsOn: []string{"zshrc_devboost"}, }, } } diff --git a/engine/modules/security_test.go b/engine/modules/security_test.go index 1b4d5ec..8209c90 100644 --- a/engine/modules/security_test.go +++ b/engine/modules/security_test.go @@ -16,6 +16,20 @@ func TestSecurityDisabled(t *testing.T) { } } +// TestSecurityDegradesGracefullyWhenZshDisabled is a regression test for +// the security_check_alias resource's DependsOn on zsh's +// zshrc_devboost — that dependency only resolves if both modules' resources +// are combined in the same list AND zshrc_devboost actually exists in it. +// zsh.enable: false is a valid, sensible config combination (security +// alone doesn't need zsh's plugin manager), so Security must not declare +// a resource whose dependency can never be satisfied. +func TestSecurityDegradesGracefullyWhenZshDisabled(t *testing.T) { + cfg := loadFixtureConfig(t, "zsh:\n enable: false\n") + if got := Security(cfg); len(got) != 0 { + t.Fatalf("expected no resources when zsh is disabled (its DependsOn target wouldn't exist), got %v", got) + } +} + func TestSecurityEnabledByDefaultInjectsAliasBlock(t *testing.T) { cfg := loadFixtureConfig(t, "") got := Security(cfg) diff --git a/engine/modules/znap.go b/engine/modules/znap.go index 6d050db..53fc2b7 100644 --- a/engine/modules/znap.go +++ b/engine/modules/znap.go @@ -1,6 +1,3 @@ -// Package modules holds the Go-engine ports of devboost's bash modules. -// This package coexists with the original bash modules/*.sh tree during -// the v2 migration; it does not replace or modify any bash file. package modules import ( diff --git a/engine/modules/zsh_security_interaction_test.go b/engine/modules/zsh_security_interaction_test.go new file mode 100644 index 0000000..550e750 --- /dev/null +++ b/engine/modules/zsh_security_interaction_test.go @@ -0,0 +1,75 @@ +package modules + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/rolfsormo/devboost/engine" +) + +// TestZshAndSecurityBothTargetZshrcDevboost is a load-bearing test for a +// real risk found while wiring the full registry: zsh's zshrc_devboost +// resource is a File (full-content overwrite) and security's +// security_check_alias resource is a BlockInFile (append/replace a +// marked block) — both targeting the SAME path +// (~/.zshrc.devboost/zsh.include_file). If security's block gets applied +// before zsh's File resource runs, zsh's Execute would silently +// overwrite the whole file and wipe out security's block. This test +// applies both resources together, in registry order, and asserts the +// final file has both zsh's rendered content AND security's block — +// catching the corruption if dependency ordering (or ordering by +// coincidence) ever breaks it. +func TestZshAndSecurityBothTargetZshrcDevboost(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + cfg := loadFixtureConfig(t, "") + + // Registry order: zsh registers before security (see registry.go), + // so exercise exactly that order here. + resources := append(Zsh(cfg), Security(cfg)...) + + if _, err := engine.DiffAndExecute(resources, nil); err != nil { + t.Fatalf("apply: %v", err) + } + + data, err := os.ReadFile(filepath.Join(home, ".zshrc.devboost")) + if err != nil { + t.Fatal(err) + } + got := string(data) + if !strings.Contains(got, "znap.zsh") { + t.Fatalf("expected zsh's rendered content to survive, got %q", got) + } + if !strings.Contains(got, "devboost-check()") { + t.Fatalf("expected security's block to survive alongside zsh's content — "+ + "if this fails, zsh's File resource overwrote security's BlockInFile "+ + "(or vice versa) rather than the two composing safely. Got: %q", got) + } +} + +// TestSecurityThenZshDoesNotLoseSecurityBlock is the reverse order — +// confirms the outcome doesn't depend on which of the two happens to run +// first, since nothing currently declares an explicit DependsOn between +// them. +func TestSecurityThenZshDoesNotLoseSecurityBlock(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + cfg := loadFixtureConfig(t, "") + + resources := append(Security(cfg), Zsh(cfg)...) + + if _, err := engine.DiffAndExecute(resources, nil); err != nil { + t.Fatalf("apply: %v", err) + } + + data, err := os.ReadFile(filepath.Join(home, ".zshrc.devboost")) + if err != nil { + t.Fatal(err) + } + got := string(data) + if !strings.Contains(got, "znap.zsh") || !strings.Contains(got, "devboost-check()") { + t.Fatalf("expected both zsh's content and security's block to survive regardless of order, got %q", got) + } +} From 9654e203b5f43222badc3840fb0d6d4e88fec9c4 Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:45:23 +0300 Subject: [PATCH 15/48] feat(kinds,modules,cli): uninstall command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds kinds.RemoveBlock (ports db_remove_block), separate from BlockInFile.Diff/Execute which only ever adds/updates a block — removal is a deliberate, distinct action, same split the bash tool already had. Ports core_main.sh's db_run_uninstall as modules.Uninstall: removes .zshrc.devboost, the devboost block from ~/.zshrc (leaving the user's own content in that file untouched — verified with a test), the devboost block from ~/.tmux.conf, .direnvrc, and the state file. Deliberately does not remove packages/znap/TPM/mise toolchains, same scope the bash version documented. Wired into the CLI as the uninstall subcommand. --- cmd/devboost-v2/main.go | 14 +++-- engine/kinds/blockinfile.go | 25 +++++++++ engine/kinds/removeblock_test.go | 53 ++++++++++++++++++ engine/modules/uninstall.go | 69 +++++++++++++++++++++++ engine/modules/uninstall_test.go | 95 ++++++++++++++++++++++++++++++++ 5 files changed, 250 insertions(+), 6 deletions(-) create mode 100644 engine/kinds/removeblock_test.go create mode 100644 engine/modules/uninstall.go create mode 100644 engine/modules/uninstall_test.go diff --git a/cmd/devboost-v2/main.go b/cmd/devboost-v2/main.go index 5382251..f07c11a 100644 --- a/cmd/devboost-v2/main.go +++ b/cmd/devboost-v2/main.go @@ -19,9 +19,10 @@ const usage = `devboost - Bootstrap a modern dev environment Usage: devboost-v2 [COMMAND] [OPTIONS] Commands: - apply Converge machine to config (default) - plan Show actions without changing anything - doctor Check prerequisites and report per-module findings + apply Converge machine to config (default) + plan Show actions without changing anything + doctor Check prerequisites and report per-module findings + uninstall Remove managed files/blocks (leaves user custom files untouched) Options: --config FILE Config file path (default: ~/.devboost.yaml) @@ -49,6 +50,8 @@ func main() { err = engine.Apply(modules.AllResources(cfg, detectedOS)) case "doctor": err = runDoctor(cfg, detectedOS) + case "uninstall": + err = modules.Uninstall(cfg) default: fmt.Fprintf(os.Stderr, "unknown command %q\n\n%s", cmd, usage) os.Exit(1) @@ -63,8 +66,7 @@ func main() { // parseArgs is intentionally small: a subcommand token plus --config, // mirroring the bash tool's db_parse_flags for the subset of flags this // CLI currently supports. --dry-run/--verbose/--yes and the -// uninstall/migrate-from-oh-my-zsh subcommands are not yet wired here — -// see tasks #16/#17. +// migrate-from-oh-my-zsh subcommand are not yet wired here — see task #17. func parseArgs(args []string) (cmd string, configPath string) { cmd = "apply" configPath = config.DefaultPath() @@ -72,7 +74,7 @@ func parseArgs(args []string) (cmd string, configPath string) { i := 0 if len(args) > 0 { switch args[0] { - case "apply", "plan", "doctor": + case "apply", "plan", "doctor", "uninstall": cmd = args[0] i = 1 case "--help", "-h": diff --git a/engine/kinds/blockinfile.go b/engine/kinds/blockinfile.go index e61d9f1..04ee176 100644 --- a/engine/kinds/blockinfile.go +++ b/engine/kinds/blockinfile.go @@ -64,6 +64,31 @@ func (b BlockInFile) Diff() (*engine.PendingOp, error) { }, nil } +// RemoveBlock strips a marked block (start/end markers inclusive) from +// path, ports the bash tool's db_remove_block. A no-op if the file +// doesn't exist or the start marker isn't present. Used by uninstall, +// not by BlockInFile.Diff/Execute (which only ever adds/updates a +// block) — removal is a deliberate, separate action, same split as the +// bash tool's db_upsert_block vs. db_remove_block. +func RemoveBlock(path, startMarker, endMarker string) error { + data, err := os.ReadFile(path) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return fmt.Errorf("read %s: %w", path, err) + } + current := string(data) + if !strings.Contains(current, startMarker) { + return nil + } + if err := BackupFile(path); err != nil { + return err + } + desired := replaceOrAppendBlock(current, startMarker, endMarker, "") + return os.WriteFile(path, []byte(desired), 0o644) +} + // replaceOrAppendBlock ports db_upsert_block's awk logic line-by-line: if // startMarker is found, everything from that line through the endMarker // line (inclusive) is replaced with block; otherwise block is appended diff --git a/engine/kinds/removeblock_test.go b/engine/kinds/removeblock_test.go new file mode 100644 index 0000000..9a8991f --- /dev/null +++ b/engine/kinds/removeblock_test.go @@ -0,0 +1,53 @@ +package kinds + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestRemoveBlockNoOpWhenFileAbsent(t *testing.T) { + path := filepath.Join(t.TempDir(), "missing") + if err := RemoveBlock(path, start, end); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestRemoveBlockNoOpWhenMarkerAbsent(t *testing.T) { + path := filepath.Join(t.TempDir(), "f") + original := "just some content\n" + if err := os.WriteFile(path, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + if err := RemoveBlock(path, start, end); err != nil { + t.Fatalf("unexpected error: %v", err) + } + data, _ := os.ReadFile(path) + if string(data) != original { + t.Fatalf("expected file untouched, got %q", data) + } +} + +func TestRemoveBlockStripsMarkedSection(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, "f") + content := "before\n" + start + "\nmanaged content\n" + end + "\nafter\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + if err := RemoveBlock(path, start, end); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + data, _ := os.ReadFile(path) + got := string(data) + if strings.Contains(got, start) || strings.Contains(got, "managed content") { + t.Fatalf("expected marked block removed, got %q", got) + } + if !strings.Contains(got, "before") || !strings.Contains(got, "after") { + t.Fatalf("expected surrounding content preserved, got %q", got) + } +} diff --git a/engine/modules/uninstall.go b/engine/modules/uninstall.go new file mode 100644 index 0000000..cea0838 --- /dev/null +++ b/engine/modules/uninstall.go @@ -0,0 +1,69 @@ +package modules + +import ( + "fmt" + "os" + "path/filepath" + + "github.com/rolfsormo/devboost/config" + "github.com/rolfsormo/devboost/engine/kinds" +) + +// Uninstall ports core_main.sh's db_run_uninstall: removes +// devboost-managed files and the marked blocks it injected into files it +// doesn't own, leaving the user's own content in those files untouched. +// Deliberately does NOT remove packages, znap, TPM, or mise toolchains — +// same scope the bash version documented (uninstalling devboost's own +// config surface, not undoing everything it ever installed on the +// system). +func Uninstall(cfg *config.Config) error { + includeFile := cfg.Get("zsh.include_file", "~/.zshrc.devboost") + if err := removeFile(includeFile); err != nil { + return err + } + + home, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("resolve home directory: %w", err) + } + zshrc := filepath.Join(home, ".zshrc") + if err := kinds.RemoveBlock(zshrc, zshIncludeStart, zshIncludeEnd); err != nil { + return fmt.Errorf("remove devboost block from %s: %w", zshrc, err) + } + + tmuxConf := cfg.Get("tmux.conf_file", "~/.tmux.conf") + if err := kinds.RemoveBlock(tmuxConf, tmuxStartMarker, tmuxEndMarker); err != nil { + return fmt.Errorf("remove devboost block from %s: %w", tmuxConf, err) + } + + direnvrc := cfg.Get("direnv.rc_path", "~/.direnvrc") + if err := removeFile(direnvrc); err != nil { + return err + } + + stateFile := filepath.Join(home, ".devboost.state.json") + if err := removeFile(stateFile); err != nil { + return err + } + + return nil +} + +// removeFile deletes path if it exists, backing it up first — matches +// the bash version's db_backup_file-then-rm pattern for uninstall's +// direct file removals (as opposed to the block removals above, which +// RemoveBlock already backs up internally). +func removeFile(path string) error { + if _, err := os.Stat(path); os.IsNotExist(err) { + return nil + } else if err != nil { + return err + } + if err := kinds.BackupFile(path); err != nil { + return err + } + if err := os.Remove(path); err != nil { + return fmt.Errorf("remove %s: %w", path, err) + } + return nil +} diff --git a/engine/modules/uninstall_test.go b/engine/modules/uninstall_test.go new file mode 100644 index 0000000..a2eb631 --- /dev/null +++ b/engine/modules/uninstall_test.go @@ -0,0 +1,95 @@ +package modules + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestUninstallRemovesZshrcDevboost(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + includeFile := filepath.Join(home, ".zshrc.devboost") + writeFile(t, includeFile, "generated content\n") + + cfg := loadFixtureConfig(t, "") + if err := Uninstall(cfg); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, err := os.Stat(includeFile); !os.IsNotExist(err) { + t.Fatalf("expected %s to be removed, stat err=%v", includeFile, err) + } +} + +func TestUninstallRemovesDevboostBlockFromZshrcKeepingUserContent(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + zshrc := filepath.Join(home, ".zshrc") + writeFile(t, zshrc, + "my custom alias\n"+ + zshIncludeStart+"\n"+ + `[ -f "$HOME/.zshrc.devboost" ] && source "$HOME/.zshrc.devboost"`+"\n"+ + zshIncludeEnd+"\n"+ + "my other custom line\n") + + cfg := loadFixtureConfig(t, "") + if err := Uninstall(cfg); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + data, err := os.ReadFile(zshrc) + if err != nil { + t.Fatalf("expected .zshrc to still exist (user content, not devboost's), got: %v", err) + } + got := string(data) + if strings.Contains(got, zshIncludeStart) { + t.Fatalf("expected devboost's block removed, got %q", got) + } + if !strings.Contains(got, "my custom alias") || !strings.Contains(got, "my other custom line") { + t.Fatalf("expected user's own content preserved, got %q", got) + } +} + +func TestUninstallRemovesDirenvrc(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + direnvrc := filepath.Join(home, ".direnvrc") + writeFile(t, direnvrc, "use_mise() { eval \"$(mise activate direnv)\"; }\n") + + cfg := loadFixtureConfig(t, "") + if err := Uninstall(cfg); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, err := os.Stat(direnvrc); !os.IsNotExist(err) { + t.Fatalf("expected %s to be removed", direnvrc) + } +} + +func TestUninstallIsNoOpOnAlreadyCleanHome(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + cfg := loadFixtureConfig(t, "") + // Nothing devboost-managed exists at all — should not error. + if err := Uninstall(cfg); err != nil { + t.Fatalf("expected uninstall on a clean home to be a no-op, got: %v", err) + } +} + +func TestUninstallBacksUpBeforeRemoving(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + includeFile := filepath.Join(home, ".zshrc.devboost") + writeFile(t, includeFile, "generated content\n") + + cfg := loadFixtureConfig(t, "") + if err := Uninstall(cfg); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + backupRoot := filepath.Join(home, ".devboost", "backups") + entries, err := os.ReadDir(backupRoot) + if err != nil || len(entries) == 0 { + t.Fatalf("expected at least one backup under %s, err=%v", backupRoot, err) + } +} From 31abba09da8b4fbec4fc0c486b6f329b4d9c169a Mon Sep 17 00:00:00 2001 From: Rolf Sormo Date: Sat, 8 Aug 2026 17:50:01 +0300 Subject: [PATCH 16/48] feat(kinds,modules,cli): migrate-from-oh-my-zsh, full CLI flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds kinds.ArchiveDir (moves a directory into the backup root instead of deleting it, named -