diff --git a/.claude/agents/plan-ci-reviewer.md b/.claude/agents/plan-ci-reviewer.md index 7154656..3bde113 100644 --- a/.claude/agents/plan-ci-reviewer.md +++ b/.claude/agents/plan-ci-reviewer.md @@ -1,7 +1,8 @@ --- name: plan-ci-reviewer description: "Use this agent to review sub-plans that involve GitHub Actions CI/CD workflows. Evaluates proposed workflow structure, job design, matrix builds, permissions, caching, and security against project conventions.\n\n\nContext: A sub-plan covers adding a new CI workflow or modifying an existing one.\nuser: \"Review sub-plan 03-add-lint-workflow.md for CI correctness.\"\nassistant: \"I'll review the sub-plan for CI issues using the plan-ci-reviewer.\"\n\nSub-plan involves GitHub Actions workflow changes. Launch the CI domain reviewer.\n\n\n\n\nContext: A sub-plan covers adding E2E tests to the CI pipeline.\nuser: \"Review sub-plan 04-e2e-test-matrix.md for CI correctness.\"\nassistant: \"I'll review the sub-plan for workflow and testing patterns using the plan-ci-reviewer.\"\n\nSub-plan involves CI pipeline changes with container-based E2E tests. Launch the CI domain reviewer.\n\n" -tools: Read, Write, Glob, Grep +tools: Read, Glob, Grep +memory: project skills: - configuring-github-actions --- @@ -14,6 +15,18 @@ container-based testing, and security. You are NOT here to praise, summarize, or restate the plan. You are here to find what's wrong with it from a CI/CD perspective. +## Memory + +Consult your agent memory before starting work — it contains knowledge about +this project's workflow structure, job naming conventions, caching strategies, +and CI patterns from previous reviews. This saves you from re-exploring the +codebase. + +After completing your review, update your agent memory with workflow patterns, +job structures, action versions, caching strategies, and CI conventions you +discovered. Write concise notes about what you found and where. Keep memory +focused on facts that help future reviews start faster. + ## What You Review You will be given a path to a specific sub-plan file (e.g., @@ -23,22 +36,24 @@ codebase to verify claims and check existing patterns. ## How You Review 1. **Read the sub-plan** completely. -2. **Read project documentation** — `AGENTS.md` (root), and any project - documentation (`docs/`, `doc/`, etc.). Documentation is dramatically - cheaper than code exploration. +2. **Read ALL project documentation first** — `AGENTS.md` (root), and any + project documentation (`docs/`, `doc/`, etc.). Documentation is orders of + magnitude cheaper than code exploration. Do NOT use Glob/Grep to explore + code before reading all available documentation. 3. **Read existing workflows** — check `.github/workflows/` to understand current patterns, job structure, and conventions already in use. 4. **Apply your skills** to evaluate the plan against project conventions. Your preloaded skills encode the conventions for GitHub Actions workflows. Use them as your review criteria. -5. **Verify claims against the codebase** — if the plan references existing - workflows, jobs, or actions, use Glob and Grep to confirm they exist and - the plan's approach is compatible. +5. **Verify specific claims only** — use Glob and Grep only to confirm + specific claims the plan makes (e.g., a workflow exists, a job name is + correct). Do not broadly explore the codebase. ## Output Format -Write your findings to `reviews/.ci.md` inside the plan directory. -Use the exact format below. +Return your findings as your response using the format below. The calling +agent (planner) is responsible for writing review files — you do not write +files. ```markdown # CI Review: diff --git a/.claude/agents/plan-installer-reviewer.md b/.claude/agents/plan-installer-reviewer.md index fc5c38b..4ef1c3f 100644 --- a/.claude/agents/plan-installer-reviewer.md +++ b/.claude/agents/plan-installer-reviewer.md @@ -1,7 +1,8 @@ --- name: plan-installer-reviewer description: "Use this agent to review sub-plans that involve the Go installer application. Evaluates proposed CLI command structure, Go code patterns, interactive UI design, and cross-platform concerns against project conventions.\n\n\nContext: A sub-plan covers adding a new Cobra command to the installer.\nuser: \"Review sub-plan 02-add-uninstall-command.md for installer correctness.\"\nassistant: \"I'll review the sub-plan for installer issues using the plan-installer-reviewer.\"\n\nSub-plan involves installer CLI work. Launch the installer domain reviewer.\n\n\n\n\nContext: A sub-plan covers adding a new package manager implementation.\nuser: \"Review sub-plan 03-pacman-package-manager.md for installer correctness.\"\nassistant: \"I'll review the sub-plan for Go and CLI patterns using the plan-installer-reviewer.\"\n\nSub-plan involves new Go code in the installer's lib/ layer. Launch the installer domain reviewer.\n\n" -tools: Read, Write, Glob, Grep +tools: Read, Glob, Grep +memory: project skills: - writing-go-code - applying-effective-go @@ -16,6 +17,18 @@ cross-platform behavior. You are NOT here to praise, summarize, or restate the plan. You are here to find what's wrong with it from an installer development perspective. +## Memory + +Consult your agent memory before starting work — it contains knowledge about +this project's Go package structure, interfaces, CLI command layout, DI +patterns, and code conventions from previous reviews. This saves you from +re-exploring the codebase. + +After completing your review, update your agent memory with package locations, +interface definitions, CLI patterns, DI wiring, and code conventions you +discovered. Write concise notes about what you found and where. Keep memory +focused on facts that help future reviews start faster. + ## What You Review You will be given a path to a specific sub-plan file (e.g., @@ -25,20 +38,23 @@ codebase to verify claims and check existing patterns. ## How You Review 1. **Read the sub-plan** completely. -2. **Read project documentation** — `AGENTS.md` (root), `installer/AGENTS.md`, - and any project documentation (`docs/`, `doc/`, etc.). Documentation - is dramatically cheaper than code exploration. +2. **Read ALL project documentation first** — `AGENTS.md` (root), + `installer/AGENTS.md`, and any project documentation (`docs/`, `doc/`, + etc.). Documentation is orders of magnitude cheaper than code exploration. + Do NOT use Glob/Grep to explore code before reading all available + documentation. 3. **Apply your skills** to evaluate the plan against project conventions. Your preloaded skills encode the conventions for Go code and CLI patterns. Use them as your review criteria. -4. **Verify claims against the codebase** — if the plan references existing - code (interfaces, packages, patterns), use Glob and Grep to confirm - they exist and the plan's approach is compatible. +4. **Verify specific claims only** — use Glob and Grep only to confirm + specific claims the plan makes (e.g., an interface exists, a package + structure is correct). Do not broadly explore the codebase. ## Output Format -Write your findings to `reviews/.installer.md` inside the plan -directory. Use the exact format below. +Return your findings as your response using the format below. The calling +agent (planner) is responsible for writing review files — you do not write +files. ```markdown # Installer Review: diff --git a/.claude/agents/plan-zsh-reviewer.md b/.claude/agents/plan-zsh-reviewer.md index d791a87..0a237a9 100644 --- a/.claude/agents/plan-zsh-reviewer.md +++ b/.claude/agents/plan-zsh-reviewer.md @@ -1,7 +1,8 @@ --- name: plan-zsh-reviewer description: "Use this agent to review sub-plans that involve Zsh shell configuration. Evaluates proposed changes to startup files, environment variables, plugin setup, completion configuration, and performance against project conventions.\n\n\nContext: A sub-plan covers restructuring .zshrc or changing plugin load order.\nuser: \"Review sub-plan 02-restructure-zshrc.md for Zsh correctness.\"\nassistant: \"I'll review the sub-plan for Zsh issues using the plan-zsh-reviewer.\"\n\nSub-plan involves Zsh configuration changes. Launch the Zsh domain reviewer.\n\n\n\n\nContext: A sub-plan covers adding environment variables or fixing PATH setup.\nuser: \"Review sub-plan 03-fix-path-ordering.md for Zsh correctness.\"\nassistant: \"I'll review the sub-plan for startup file and PATH issues using the plan-zsh-reviewer.\"\n\nSub-plan involves Zsh environment and PATH changes. Launch the Zsh domain reviewer.\n\n" -tools: Read, Write, Glob, Grep +tools: Read, Glob, Grep +memory: project skills: - configuring-zsh - managing-chezmoi @@ -15,6 +16,18 @@ completions, performance, and chezmoi integration. You are NOT here to praise, summarize, or restate the plan. You are here to find what's wrong with it from a Zsh configuration perspective. +## Memory + +Consult your agent memory before starting work — it contains knowledge about +this project's shell config structure, plugin setup, startup file ordering, +template variables, and Zsh conventions from previous reviews. This saves you +from re-exploring the codebase. + +After completing your review, update your agent memory with shell config +locations, plugin configurations, startup file ordering, template variables, +and Zsh conventions you discovered. Write concise notes about what you found +and where. Keep memory focused on facts that help future reviews start faster. + ## What You Review You will be given a path to a specific sub-plan file (e.g., @@ -24,23 +37,25 @@ codebase to verify claims and check existing patterns. ## How You Review 1. **Read the sub-plan** completely. -2. **Read project documentation** — `AGENTS.md` (root), and any project - documentation (`docs/`, `doc/`, etc.). Documentation is dramatically - cheaper than code exploration. +2. **Read ALL project documentation first** — `AGENTS.md` (root), and any + project documentation (`docs/`, `doc/`, etc.). Documentation is orders of + magnitude cheaper than code exploration. Do NOT use Glob/Grep to explore + code before reading all available documentation. 3. **Read existing shell configs** — check `dot_zshrc`, `dot_zshenv`, `dot_zprofile`, and `dot_config/sheldon/` to understand current patterns and conventions already in use. 4. **Apply your skills** to evaluate the plan against project conventions. Your preloaded skills encode the conventions for Zsh configuration and chezmoi management. Use them as your review criteria. -5. **Verify claims against the codebase** — if the plan references existing - config blocks, plugins, or template variables, use Glob and Grep to - confirm they exist and the plan's approach is compatible. +5. **Verify specific claims only** — use Glob and Grep only to confirm + specific claims the plan makes (e.g., a plugin exists, a template variable + is defined). Do not broadly explore the codebase. ## Output Format -Write your findings to `reviews/.zsh.md` inside the plan directory. -Use the exact format below. +Return your findings as your response using the format below. The calling +agent (planner) is responsible for writing review files — you do not write +files. ```markdown # Zsh Review: diff --git a/.claude/skills/testing-go-code/SKILL.md b/.claude/skills/testing-go-code/SKILL.md index 61678fa..18c8423 100644 --- a/.claude/skills/testing-go-code/SKILL.md +++ b/.claude/skills/testing-go-code/SKILL.md @@ -15,7 +15,7 @@ task test -- -run TestName # Run specific test(s) task test -- -short # Skip integration tests ``` -For test conventions (naming, assertions, table-driven patterns, mock usage), see the `writing-go-code` skill. +For test conventions (naming, assertions, table-driven patterns, mock usage), see the `writing-go-tests` skill. ## Coverage diff --git a/.claude/skills/writing-go-code/SKILL.md b/.claude/skills/writing-go-code/SKILL.md index fae4b08..089e506 100644 --- a/.claude/skills/writing-go-code/SKILL.md +++ b/.claude/skills/writing-go-code/SKILL.md @@ -1,20 +1,16 @@ --- name: writing-go-code -description: Apply Go coding standards when writing, reviewing, or modifying Go code. Use when implementing functions, writing tests with testify, generating mocks with mockery, using dependency injection, handling errors idiomatically, or working with interfaces. Use this skill for any Go file editing task. +description: Apply Go coding standards when writing or modifying Go code. Use when implementing functions, using dependency injection, handling errors idiomatically, or working with interfaces. For test conventions, use the `writing-go-tests` skill instead. --- # Go Development Standards Project-specific Go coding standards for this codebase. -## Companion Skill +## Companion Skills -This skill covers **project-specific** Go patterns (testing conventions, mock generation, dependency injection style). For **general Go idioms** from the official Effective Go documentation (naming, control flow, error handling philosophy, concurrency patterns), also load the `applying-effective-go` skill. Both skills are complementary — this one tells you how *this project* writes Go, the other tells you how *Go itself* should be written. - -## Quick Reference - -**Coding patterns:** See [Code Style Reference](references/code-style.md) -**Testing patterns:** See [Test Style Reference](references/test-style.md) +- **`applying-effective-go`** — General Go idioms from the official Effective Go documentation (naming, control flow, error handling philosophy, concurrency patterns). Complementary to this skill. +- **`writing-go-tests`** — Test conventions, mock usage, assertions, naming. Always load when writing test files. ## Code Organization @@ -34,53 +30,29 @@ func NewMyService(logger Logger, fs FileSystem) *MyService { } ``` -## Testing Patterns +## Dependency Injection -Use `testify/require` for all assertions. Name tests descriptively: +Always inject dependencies via constructors. Never create dependencies internally. ```go -func Test_ServiceReturnsErrorWhenFileNotFound(t *testing.T) { - // Arrange: create mock - fs := &MoqFileSystem{ - ReadFileFunc: func(path string) ([]byte, error) { - return nil, os.ErrNotExist - }, +// Good: dependencies injected +func NewHandler( + logger Logger, + service Service, + validator Validator, +) *Handler { + return &Handler{ + logger: logger, + service: service, + validator: validator, } - svc := NewMyService(logger, fs) - - // Act - err := svc.LoadConfig("missing.yaml") - - // Assert: check error by keyword, not full message - require.Error(t, err) - require.Contains(t, err.Error(), "not found") } -``` - -## Table-Driven Tests -```go -func Test_ParseConfig(t *testing.T) { - tests := []struct { - name string - input string - want Config - wantErr bool - }{ - {"valid config", "key: value", Config{Key: "value"}, false}, - {"empty input", "", Config{}, true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := ParseConfig(tt.input) - if tt.wantErr { - require.Error(t, err) - return - } - require.NoError(t, err) - require.Equal(t, tt.want, got) - }) +// Bad: dependencies created internally +func NewHandler() *Handler { + return &Handler{ + logger: NewDefaultLogger(), // Don't do this + service: NewService(), // Don't do this } } ``` @@ -93,7 +65,7 @@ Mocks use `mockery` with moq template. To regenerate all mocks: mockery ``` -Mock types are prefixed with `Moq` (e.g., `MoqLogger`, `MoqFileSystem`). +Mock types are prefixed with `Moq` (e.g., `MoqLogger`, `MoqFileSystem`). For mock usage conventions in tests, see the `writing-go-tests` skill. ## Optional Types @@ -111,10 +83,43 @@ if shell, ok := config.Shell.Get(); ok { } ``` +## Code Formatting + +- Line length: 120 characters max. +- Vertically align function arguments when there are multiple arguments. +- Insert blank lines between logical sections of code. +- Do not separate error unwrapping from related code with a blank line; treat it as part of the same section. + +```go +// Good: error handling is part of the same section +result, err := doSomething() +if err != nil { + return fmt.Errorf("failed to do something: %w", err) +} + +// Next logical section starts after blank line +processResult(result) +``` + +## Documentation + +End all type and function comments with a period, following Go conventions. + +```go +// MyService handles business logic for the application. +type MyService struct { + // ... +} + +// Process executes the main workflow and returns the result. +func (s *MyService) Process(ctx context.Context) error { + // ... +} +``` + ## Key Rules -- Line length: 120 characters max -- Pre-allocate slices/maps when size is known -- Wrap OS operations in interfaces for mockability -- End doc comments with a period -- Never edit mock files manually +- Use the Go standard library whenever possible. Only use third-party libraries when necessary. +- Pre-allocate slices/maps when size is known. +- Wrap OS operations in interfaces for mockability. +- Never edit mock files manually. diff --git a/.claude/skills/writing-go-code/references/code-style.md b/.claude/skills/writing-go-code/references/code-style.md deleted file mode 100644 index 2d95798..0000000 --- a/.claude/skills/writing-go-code/references/code-style.md +++ /dev/null @@ -1,151 +0,0 @@ -# Go Code Style Guidelines - -## Table of Contents - -- [Dependency Injection](#dependency-injection) -- [Struct Definitions](#struct-definitions) -- [Constructors](#constructors) -- [Mocks](#mocks) -- [General Principles](#general-principles) -- [Code Formatting](#code-formatting) -- [Documentation](#documentation) -- [Performance](#performance) - ---- - -## Dependency Injection - -Always prefer to use dependency injection to pass dependencies into constructors. - -```go -// Good: dependencies injected -func NewHandler( - logger Logger, - service Service, - validator Validator, -) *Handler { - return &Handler{ - logger: logger, - service: service, - validator: validator, - } -} - -// Bad: dependencies created internally -func NewHandler() *Handler { - return &Handler{ - logger: NewDefaultLogger(), // Don't do this - service: NewService(), // Don't do this - } -} -``` - ---- - -## Struct Definitions - -After each struct definition, verify interface implementation by adding: - -```go -var _ InterfaceName = (*StructName)(nil) -``` - ---- - -## Constructors - -Provide a constructor function for each struct, named `NewStructName`. - -Place this function immediately after the struct definition and the interface assertion line (if present). - -```go -type MyService struct { - logger Logger - fs FileSystem -} - -var _ Service = (*MyService)(nil) - -func NewMyService(logger Logger, fs FileSystem) *MyService { - return &MyService{ - logger: logger, - fs: fs, - } -} -``` - ---- - -## Mocks - -Never edit mock files directly. Instead, regenerate them by running the `mockery` command in the Go module root directory: - -```bash -mockery -``` - -Run without any arguments to regenerate all mocks. - ---- - -## General Principles - -- Use the Go standard library whenever possible. Only use third-party libraries when necessary. -- Limit line length to 120 characters. -- Write code that is easy to test: - - Use interfaces to decouple components and improve testability. - - Use dependency injection to pass dependencies into functions and methods. - - Wrap even basic operations (such as OS functions and file operations) in interfaces to make them easier to mock and test. - ---- - -## Code Formatting - -- Vertically align function arguments when there are multiple arguments. -- Insert blank lines between logical sections of code. -- Do not separate error unwrapping from related code with a blank line; treat it as part of the same section. - -```go -// Good: error handling is part of the same section -result, err := doSomething() -if err != nil { - return fmt.Errorf("failed to do something: %w", err) -} - -// Next logical section starts after blank line -processResult(result) -``` - -## Documentation - -End all type and function comments with a period, following Go conventions. - -```go -// MyService handles business logic for the application. -type MyService struct { - // ... -} - -// Process executes the main workflow and returns the result. -func (s *MyService) Process(ctx context.Context) error { - // ... -} -``` - -## Performance - -Pre-allocate collections (such as slices and maps) to their expected size when possible to reduce memory allocations and improve performance. - -```go -// Good: pre-allocated slice -items := make([]Item, 0, len(input)) -for _, v := range input { - items = append(items, transform(v)) -} - -// Good: pre-allocated map -lookup := make(map[string]int, len(keys)) -for i, k := range keys { - lookup[k] = i -} -``` diff --git a/.claude/skills/writing-go-code/references/test-style.md b/.claude/skills/writing-go-code/references/test-style.md deleted file mode 100644 index fdfa24c..0000000 --- a/.claude/skills/writing-go-code/references/test-style.md +++ /dev/null @@ -1,165 +0,0 @@ -# Go Test Style Guidelines - -## Table of Contents - -- [General Principles](#general-principles) -- [Test Naming Conventions](#test-naming-conventions) -- [Table-Driven Tests](#table-driven-tests) -- [Error Assertions](#error-assertions) -- [Unit Tests](#unit-tests) -- [Integration Tests](#integration-tests) -- [Using Mocks](#using-mocks) -- [Tech Stack](#tech-stack) - ---- - -## General Principles - -- Use the `testify` library for all testing in Go. -- Always use the `require` package from `testify` when checking the `error` type. -- Each test should verify a single behavior or property. Do not test multiple behaviors in a single test. - ---- - -## Test Naming Conventions - -Name tests using descriptive and natural language (literate test names). - -**Format:** `Test_` - -**Guidelines:** - -- Test names should describe what the test does and what it verifies, not implementation details. -- Use the `Test_` prefix for test functions. -- If a condition is crucial to the test, include it in the test name. -- Prefer `Should` statements when they fit naturally. - -**Examples:** - -```go -// Good: describes behavior -func Test_CompatibilityConfigCanBeLoadedFromFile(t *testing.T) -func Test_CompatibilityConfigCanBeLoadedFromFileWhenFileExists(t *testing.T) -func Test_CreatingClientShouldLoadCompatibilityMapFromFile(t *testing.T) - -// Bad: describes implementation -func Test_LoadConfig(t *testing.T) -func Test_ConfigLoader_Success(t *testing.T) -``` - ---- - -## Table-Driven Tests - -Use table-driven tests when testing multiple scenarios. This pattern makes it easy to add new test cases and keeps the code clean. - -```go -func Test_VerbosityLevelDetermination(t *testing.T) { - tests := []struct { - name string - verbose bool - extra bool - expected VerbosityLevel - }{ - {"default returns normal", false, false, VerbosityNormal}, - {"verbose flag returns verbose", true, false, VerbosityVerbose}, - {"both flags returns extra verbose", true, true, VerbosityExtraVerbose}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := determineVerbosity(tt.verbose, tt.extra) - require.Equal(t, tt.expected, result) - }) - } -} -``` - ---- - -## Error Assertions - -When expecting an error, don't match the error message directly. Instead, use specific keywords that are pivotal to the error. - -```go -// Good: checks for key error indicator -err := loadConfig("nonexistent.yaml") -require.Error(t, err) -require.Contains(t, err.Error(), "not found") - -// Bad: matches entire error message -require.EqualError(t, err, "file could not be found in the path /config/nonexistent.yaml") -``` - ---- - -## Unit Tests - -Unit tests verify a single function or method in isolation. - -**Characteristics:** - -- Use mocks to isolate the function or method being tested. -- Place unit tests in the same package as the code being tested. - -```go -// File: lib/brew/brew_test.go -package brew // Same package as implementation - -func Test_BrewPackageManagerInstallsPackageSuccessfully(t *testing.T) { - // Arrange - commander := &MoqCommander{ - RunCommandFunc: func(ctx context.Context, name string, args []string, opts ...Option) (Result, error) { - return Result{ExitCode: 0}, nil - }, - } - pm := NewBrewPackageManager(logger, commander, osManager, "/opt/homebrew/bin/brew") - - // Act - err := pm.InstallPackage(ctx, RequestedPackageInfo{Name: "git"}) - - // Assert - require.NoError(t, err) -} -``` - ---- - -## Integration Tests - -Integration tests verify the interaction between multiple functions or methods, including OS-dependent interactions. - -**Characteristics:** - -- Cover OS-dependent interactions (anything beyond CPU and memory). -- Allow opting out using `testing.Short()`. -- Place in the test package (e.g., `lib_test` for `lib` package). -- Write in BDD-style "given-when-then". - -**Naming Format:** `Test__Should__When_` - -```go -// File: lib/brew/integration_test.go -package brew_test // Test package (external) - -func Test_InstallingPackage_Should_SucceedWithoutError_When_BrewIsAvailable(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test in short mode") - } - - // Given - pm := NewBrewPackageManager(/* real dependencies */) - - // When - err := pm.InstallPackage(ctx, RequestedPackageInfo{Name: "tree"}) - - // Then - require.NoError(t, err) -} -``` - ---- - -## Using Mocks in Tests - -Mocks are generated by mockery. For regeneration commands, see the `testing-go-code` skill. For mock naming, file placement, and struct conventions, see `.mockery.yml` — these are configuration-driven, not hardcoded conventions. diff --git a/.claude/skills/writing-go-tests/SKILL.md b/.claude/skills/writing-go-tests/SKILL.md new file mode 100644 index 0000000..4872c44 --- /dev/null +++ b/.claude/skills/writing-go-tests/SKILL.md @@ -0,0 +1,140 @@ +--- +name: writing-go-tests +description: Write Go tests following project conventions. Use when creating test files, writing unit or integration tests, choosing mocks, or setting up test fixtures. Covers test naming, assertions, mock usage, table-driven patterns, and common pitfalls. +--- + +# Writing Go Tests + +Project-specific test conventions for this codebase. + +## Critical Rules + +- **Always use mockery-generated `Moq*` mocks** when one exists for the interface. Never hand-roll a mock struct for an interface that has a `*_mock.go` file. Run `mockery` (no args) from the module root to regenerate mocks after interface changes. +- **Use `logger.NoopLogger{}` when tests don't need to assert on log output.** It already implements the full `Logger` interface. Never create a custom mock logger just to satisfy the interface — that duplicates `NoopLogger` for no reason. +- **Use `logger.MoqLogger{}` only when the test needs to verify specific log calls** (e.g., asserting that `Warning` was called with a specific message). + +## Mock Selection Guide + +| Situation | Use | Import | +|-----------|-----|--------| +| Interface has a `*_mock.go` file | `Moq*` struct from that file | Same package (in-package tests) | +| Logger needed but output doesn't matter | `logger.NoopLogger{}` | `utils/logger` | +| Logger needed and must assert on calls | `logger.MoqLogger{}` | `utils/logger` | +| Interface has NO mock file (e.g., `MultiSelectSelector[T]`) | Inline mock struct in test file | N/A | + +Before creating an inline mock, check if a `*_mock.go` file exists in the interface's package: + +```bash +ls installer//*_mock.go +``` + +## Test Naming + +**Format:** `Test_` + +Test names describe behavior, not implementation: + +```go +// Good: describes behavior +func Test_CompatibilityConfigCanBeLoadedFromFile(t *testing.T) +func Test_CreatingClientShouldLoadCompatibilityMapFromFile(t *testing.T) + +// Bad: describes implementation +func Test_LoadConfig(t *testing.T) +func Test_ConfigLoader_Success(t *testing.T) +``` + +## Assertions + +Use `testify/require` for all assertions. When expecting errors, match by keyword, not full message: + +```go +// Good: checks for key error indicator +require.Error(t, err) +require.Contains(t, err.Error(), "not found") + +// Bad: matches entire error message +require.EqualError(t, err, "file could not be found in the path /config/nonexistent.yaml") +``` + +## Unit Tests + +Unit tests verify a single function or method in isolation. + +- Use mocks to isolate the function being tested. +- Place unit tests in the same package as the code being tested. +- Each test verifies a single behavior. + +```go +package brew + +func Test_BrewPackageManagerInstallsPackageSuccessfully(t *testing.T) { + // Arrange + commander := &MoqCommander{ + RunCommandFunc: func(ctx context.Context, name string, args []string, opts ...Option) (Result, error) { + return Result{ExitCode: 0}, nil + }, + } + pm := NewBrewPackageManager(&logger.NoopLogger{}, commander, osManager, "/opt/homebrew/bin/brew") + + // Act + err := pm.InstallPackage(ctx, RequestedPackageInfo{Name: "git"}) + + // Assert + require.NoError(t, err) +} +``` + +## Table-Driven Tests + +Use when testing multiple scenarios of the same function: + +```go +func Test_VerbosityLevelDetermination(t *testing.T) { + tests := []struct { + name string + verbose bool + extra bool + expected VerbosityLevel + }{ + {"default returns normal", false, false, VerbosityNormal}, + {"verbose flag returns verbose", true, false, VerbosityVerbose}, + {"both flags returns extra verbose", true, true, VerbosityExtraVerbose}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := determineVerbosity(tt.verbose, tt.extra) + require.Equal(t, tt.expected, result) + }) + } +} +``` + +## Integration Tests + +Integration tests verify interaction between components, including OS-dependent interactions. + +- Allow opting out with `testing.Short()`. +- Place in the test package (e.g., `brew_test` for `brew` package). +- Use BDD-style naming: `Test__Should__When_`. + +```go +package brew_test + +func Test_InstallingPackage_Should_SucceedWithoutError_When_BrewIsAvailable(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + + pm := NewBrewPackageManager(/* real dependencies */) + err := pm.InstallPackage(ctx, RequestedPackageInfo{Name: "tree"}) + require.NoError(t, err) +} +``` + +## Tech Stack + +- `testify/require` — assertions (never `assert` for error checks) +- `mockery` with moq template — mock generation (see `.mockery.yml`) +- `logger.NoopLogger{}` — silent logger for tests diff --git a/.github/workflows/installer-ci.yml b/.github/workflows/installer-ci.yml index 7476e98..7320431 100644 --- a/.github/workflows/installer-ci.yml +++ b/.github/workflows/installer-ci.yml @@ -254,7 +254,7 @@ jobs: fi fi - - name: Test Interactive GPG Installation + - name: Test Interactive Installer run: | echo "Testing installer in interactive mode with expect automation..." diff --git a/AGENTS.md b/AGENTS.md index 1b034fe..b958a4b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -6,7 +6,8 @@ Personal dotfiles managed with [chezmoi]. This repo is the **chezmoi source dire | Skill | Use When | | ---------------------------- | --------------------------------------------------------------------------- | -| `writing-go-code` | Writing/editing Go code, tests, mocks, interfaces | +| `writing-go-code` | Writing/editing Go code, interfaces, dependency injection | +| `writing-go-tests` | Writing test files, assertions, mock selection, test naming | | `developing-cli-apps` | CLI commands, flags, Cobra patterns, Huh interactive UI, signal handling | | `building-go-binaries` | Compiling the project, troubleshooting build failures | | `linting-go-code` | Running linters, fixing lint errors, formatting code | diff --git a/docs/architecture-installer.md b/docs/architecture-installer.md index bc1c00e..b928a7a 100644 --- a/docs/architecture-installer.md +++ b/docs/architecture-installer.md @@ -37,6 +37,7 @@ Each package under `lib/` owns one domain area: | [`shell`][shell] | Shell installation and default-setting | `ShellInstaller`, `ShellResolver`, `ShellChanger` | | [`gpg`][gpg] | GPG client installation and key management | `GpgInstaller`, `GpgClient` | | [`dotfilesmanager`][dotfilesmanager] | Chezmoi integration | `DotfilesManager` (composed of `DotfilesInstaller`, `DotfilesDataInitializer`, `DotfilesApplier`) | +| [`toolsinstaller`][toolsinstaller] | Optional tool installation | `ToolsInstaller` — resolve and install user-selected CLI tools | **Dependency direction within lib**: Packages depend on `pkgmanager` (the interface), never on each other's concrete implementations. For example, `shell` accepts a `PackageManager` — it doesn't know whether it's brew or apt. @@ -48,6 +49,7 @@ Each package under `lib/` owns one domain area: - `selector.go` — Generic single-select wrapper around Huh - `multiselect_selector.go` — Generic multi-select wrapper - `prerequisite_selector.go` — Prerequisite selection form + - `tool_selector.go` — Optional tool selection form - `gpg_selector.go` — GPG key selection form ### utils — Infrastructure Layer @@ -69,8 +71,8 @@ Shared utilities that all other layers depend on: ### internal/config — Embedded Configuration -- **Responsibility**: Store static YAML config files ([`compatibility.yaml`][compatibility-yaml], [`packagemap.yaml`][packagemap-yaml]) embedded into the binary via `go:embed` -- **Boundaries**: Read-only. Loaded by `lib/compatibility` and `lib/packageresolver` through viper. +- **Responsibility**: Store static YAML config files ([`compatibility.yaml`][compatibility-yaml], [`packagemap.yaml`][packagemap-yaml], [`tools.yaml`][tools-yaml]) embedded into the binary via `go:embed` +- **Boundaries**: Read-only. Loaded by `lib/compatibility`, `lib/packageresolver`, and `lib/toolsinstaller` through viper. ## Communication Patterns @@ -134,6 +136,7 @@ flowchart TD shell_pkg["shell"] gpg_pkg["gpg"] dotfiles["dotfilesmanager\n+ chezmoi impl"] + toolsinst["toolsinstaller"] brew -.->|implements| pkgmgr apt_pkg -.->|implements| pkgmgr @@ -152,6 +155,7 @@ flowchart TD subgraph internal ["internal/config"] compat_yaml["compatibility.yaml"] pkgmap_yaml["packagemap.yaml"] + tools_yaml["tools.yaml"] end cmd --> cli @@ -188,3 +192,5 @@ flowchart TD [shell]: ../installer/lib/shell [gpg]: ../installer/lib/gpg [dotfilesmanager]: ../installer/lib/dotfilesmanager +[toolsinstaller]: ../installer/lib/toolsinstaller +[tools-yaml]: ../installer/internal/config/tools.yaml diff --git a/docs/architecture.md b/docs/architecture.md index 38c0d33..f30220f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -33,7 +33,7 @@ The project has three major parts that interact through a clear data contract: a ### Embedded Configuration (`installer/internal/config/`) -- **Responsibility**: Store static configuration that the installer needs — supported platforms ([`compatibility.yaml`][compatibility-yaml]) and package name mappings ([`packagemap.yaml`][packagemap-yaml]) +- **Responsibility**: Store static configuration that the installer needs — supported platforms ([`compatibility.yaml`][compatibility-yaml]), package name mappings ([`packagemap.yaml`][packagemap-yaml]), and optional tool definitions ([`tools.yaml`][tools-yaml]) - **Boundaries**: Embedded into the Go binary at compile time via `go:embed`. Read-only at runtime. Overridable via CLI flags for testing. - **Dependencies**: None — these are self-contained data files @@ -74,6 +74,7 @@ flowchart LR I1[Collect user input] I2[Install prerequisites] I3[Write chezmoi.toml] + I4[Install optional tools] end subgraph chezmoi ["Chezmoi"] @@ -94,6 +95,7 @@ flowchart LR I3 -- "chezmoi.toml\n(data contract)" --> C1 C1 --> C2 --> C3 C3 -- "generated files" --> R1 + C3 --> I4 R1 --> R2 R2 --> R3 ``` @@ -104,4 +106,5 @@ flowchart LR [arch-installer]: architecture-installer.md [compatibility-yaml]: ../installer/internal/config/compatibility.yaml [packagemap-yaml]: ../installer/internal/config/packagemap.yaml +[tools-yaml]: ../installer/internal/config/tools.yaml [shell-startup]: processes/shell-startup.md diff --git a/docs/domain.md b/docs/domain.md index ca20765..a57862f 100644 --- a/docs/domain.md +++ b/docs/domain.md @@ -86,6 +86,17 @@ build-essential: See the [package resolution process][pkg-resolution] for the resolution flow from abstract key to installable package. +### Optional Tools + +Daily-use CLI tools (fzf, bat, eza, ripgrep, fd, difftastic, sheldon) that the installer can install at the user's request. Defined in [`tools.yaml`][tools-yaml], separate from prerequisites. + +- **Tool definition**: An entry in `tools.yaml` with a `name` ([abstract package key](#package-resolution)) and a human-readable `description`. +- **Not required**: Unlike prerequisites, optional tools are not needed for correct dotfiles setup. They enhance the shell experience but the system works without them. +- **Not persisted**: Tool selections are not saved to chezmoi data — tools are not part of the data contract. +- **Platform-dependent availability**: Not all tools have package mappings for every manager. Some (e.g., `sheldon`, `eza`, `difftastic`) are brew-only. + +See the [optional tools installation process][tools-install] for selection, filtering, and installation details. + ### Shell Source Strategy When installing the user's shell, the installer supports three strategies for locating and installing it: @@ -148,10 +159,14 @@ See the [shell startup process][shell-startup] for the full shell startup flow i | Package type | Classification of a package: regular (default), group (DNF group install), or pattern | | Shell source strategy | How the installer finds/installs the shell: auto, brew, or system | | Specific work profile | Employer-specific configuration at `~/.work/{work_name}/profile` | +| Optional tool | A daily-use CLI tool (e.g., fzf, bat) defined in `tools.yaml`, installable after dotfiles setup via the installer's optional tools step | +| Tool definition | An entry in `tools.yaml` with a name (abstract package key) and description | | Work name | Short employer identifier (e.g., `sedg`) used in paths and environment variable prefixes | [installation]: processes/installation.md +[tools-install]: processes/tools-installation.md [pkg-resolution]: processes/package-resolution.md [shell-startup]: processes/shell-startup.md [work-env-loading]: processes/work-environment-loading.md [packagemap-yaml]: ../installer/internal/config/packagemap.yaml +[tools-yaml]: ../installer/internal/config/tools.yaml diff --git a/docs/processes/installation.md b/docs/processes/installation.md index d591aab..1bfbdcf 100644 --- a/docs/processes/installation.md +++ b/docs/processes/installation.md @@ -42,7 +42,10 @@ flowchart TD M --> N[Install chezmoi] N --> O[Initialize chezmoi data] O --> P[Apply dotfiles] - P --> Q([Installation complete]) + P --> R{Tools available &
selected/auto?} + R -- Yes --> S[Install optional tools] + R -- No --> Q + S --> Q([Installation complete]) style Q fill:#2d6,stroke:#183,color:#fff ``` @@ -60,14 +63,16 @@ flowchart TD 7. **[Install and configure shell][shell-setup]** — Install the target shell (default: zsh) using the [shell source strategy][domain-shell-source], then set it as the user's default shell 8. **[Set up GPG keys][gpg-setup]** — Check for existing GPG keys. If none exist, create a new key pair interactively. If keys exist, let the user select one. Skipped in non-interactive mode. 9. **[Set up dotfiles manager][dotfiles-setup]** — Install chezmoi if needed, initialize [chezmoi data][domain-data-schema] from collected input, then apply dotfiles +10. **[Install optional tools][tools-install]** — Load [tool definitions][domain-optional-tools] from `tools.yaml`, pre-filter against the active package manager, then either auto-install all (if `--install-tools`) or present an interactive multi-select. Individual failures are logged but never abort the install. -Result: Machine is fully configured with the user's dotfiles, shell, and GPG setup. +Result: Machine is fully configured with the user's dotfiles, shell, GPG setup, and selected optional tools. ### Failure Scenarios Each sub-process has its own detailed failure scenarios. At the orchestration level: -- Any step failure causes the installer to exit non-zero — there is no rollback mechanism +- Any step failure in steps 1–9 causes the installer to exit non-zero — there is no rollback mechanism +- Step 10 (optional tools) is non-fatal: individual tool failures are logged but do not affect the exit code - On macOS, Homebrew failure at step 2 blocks the entire flow (prerequisites depend on it) - Steps are sequential: each depends on state set by previous steps (e.g., `selectedGpgKey` from step 8 feeds into step 9) @@ -81,6 +86,7 @@ See the individual process docs for detailed failure scenarios and handling. - **GPG keyring**: New key pair created or existing key selected (see [GPG setup][gpg-setup]) - **Chezmoi config**: `~/.config/chezmoi/chezmoi.toml` written with all data namespaces (see [dotfiles setup][dotfiles-setup]) - **Home directory**: Dotfiles applied — shell configs, git config, work profiles, etc. +- **Optional tools**: Selected CLI tools installed via the active package manager (if any were chosen) ## Sub-Processes @@ -92,6 +98,7 @@ See the individual process docs for detailed failure scenarios and handling. | [Shell Setup][shell-setup] | Install shell and set as default | | [GPG Setup][gpg-setup] | Install GPG client, create or select signing key | | [Dotfiles Setup][dotfiles-setup] | Install chezmoi, write config, clone repo, apply dotfiles | +| [Optional Tools Installation][tools-install] | Load tool definitions, pre-filter by platform, select and install optional CLI tools | ## Dependencies @@ -107,6 +114,8 @@ See the individual process docs for detailed failure scenarios and handling. [shell-setup]: shell-setup.md [gpg-setup]: gpg-setup.md [dotfiles-setup]: dotfiles-setup.md +[tools-install]: tools-installation.md [domain-shell-source]: ../domain.md#shell-source-strategy [domain-data-schema]: ../domain.md#chezmoi-data-schema +[domain-optional-tools]: ../domain.md#optional-tools [domain-pkg-resolution]: ../domain.md#package-resolution diff --git a/docs/processes/tools-installation.md b/docs/processes/tools-installation.md new file mode 100644 index 0000000..2bd468c --- /dev/null +++ b/docs/processes/tools-installation.md @@ -0,0 +1,120 @@ +# Optional Tools Installation + +## Overview + +Installs optional CLI tools after dotfiles have been applied. Loads [tool definitions][domain-optional-tools] from [`tools.yaml`][tools-yaml], pre-filters them against the active package manager, and installs the user's selection. This step is entirely non-fatal — failures are logged but never abort the install. + +## Trigger + +The [dotfiles setup][dotfiles-setup] step completes successfully during the [installation process][installation]. + +## Actors + +- **User**: Selects which tools to install (interactive mode only) +- **Tools loader**: Reads tool definitions from `tools.yaml` (embedded or overridden via `--tools-config`) +- **Package resolver**: Pre-filters tools and resolves abstract keys to concrete package names (see [package resolution][pkg-resolution]) +- **Package manager**: Installs the resolved packages (apt, dnf, or brew) +- **Tool selector**: Presents the multi-select UI for tool selection (cli layer) + +## Diagram + +```mermaid +flowchart TD + A[Load tools config] --> B{Config loaded?} + B -- No --> B1([Warning: skip tools]) + B -- Yes --> C{Any tools
defined?} + C -- No --> C1([No tools available]) + C -- Yes --> D[Create package manager & resolver] + D --> E{Manager/resolver
available?} + E -- No --> E1([Warning: skip tools]) + E -- Yes --> F[Pre-filter: resolve each tool] + F --> G{Any tools
resolvable?} + G -- No --> G1([No tools for this system]) + G -- Yes --> H{--install-tools?} + H -- Yes --> K[Install all available] + H -- No --> I{Interactive?} + I -- No --> I1([Skip: non-interactive]) + I -- Yes --> J[Show multi-select UI] + J --> J1{Any selected?} + J1 -- No --> J2([No tools selected]) + J1 -- Yes --> K + K --> L[For each tool: resolve & install] + L --> M[Report summary] + M --> N([Tools setup completed]) + + style B1 fill:#e90,stroke:#b60,color:#fff + style C1 fill:#36a,stroke:#248,color:#fff + style E1 fill:#e90,stroke:#b60,color:#fff + style G1 fill:#36a,stroke:#248,color:#fff + style I1 fill:#36a,stroke:#248,color:#fff + style J2 fill:#36a,stroke:#248,color:#fff + style N fill:#2d6,stroke:#183,color:#fff +``` + +## Flow + +### Happy Path + +1. **Load tools configuration** — Load from `--tools-config` file if provided, otherwise from the embedded `tools.yaml`. Uses a fresh viper instance to avoid state pollution from other config loaders. +2. **Guard: tools defined** — If no tools are defined in the config, finish early. +3. **Create package manager and resolver** — Reuse the system's active package manager and create a resolver for the current platform. +4. **Pre-filter tools** — For each tool in the config, attempt to resolve it against the active package manager. Only tools with valid mappings in [`packagemap.yaml`][packagemap-yaml] are kept. Tools without mappings are silently dropped (e.g., `sheldon`, `eza`, `difftastic` are brew-only and won't appear on apt/dnf systems). +5. **Guard: resolvable tools** — If no tools survived pre-filtering, finish early. +6. **Determine which tools to install**: + - **`--install-tools` flag**: Auto-install all available tools without prompting + - **Interactive**: Show a multi-select UI (all tools unselected by default). User selects with space, confirms with enter. + - **Non-interactive without `--install-tools`**: Skip entirely +7. **Install each tool** — For each selected tool, resolve and install via the package manager. Failures are logged per-tool but do not stop remaining installations. +8. **Report summary** — Log the count of successes and failures. + +Result: Selected tools installed. Failures logged but install continues. + +### Failure Scenarios + +#### Config load failure + +- **Trigger**: `--tools-config` file doesn't exist or embedded config is corrupted +- **At step**: 1 +- **Handling**: Logs a warning and skips the entire tools step +- **User impact**: No tools installed; main installation unaffected + +#### No package manager or resolver available + +- **Trigger**: System has no supported package manager +- **At step**: 3 +- **Handling**: Logs a warning and skips tools +- **User impact**: Must install tools manually + +#### Tool selection cancelled + +- **Trigger**: User cancels the multi-select UI (e.g., Ctrl+C in Huh) +- **At step**: 6 +- **Handling**: Logs a warning and skips tools +- **User impact**: No tools installed; main installation unaffected + +#### Individual tool installation fails + +- **Trigger**: Package manager returns an error for a specific tool +- **At step**: 7 +- **Handling**: Logs the failure, continues with remaining tools, reports in summary +- **User impact**: Failed tools must be installed manually; other tools are unaffected + +## State Changes + +- System packages installed via apt/dnf/brew for each selected tool +- No installer-specific state files written — tool selections are not persisted + +## Dependencies + +- A supported package manager (apt, dnf, or brew) +- [`tools.yaml`][tools-yaml] for tool definitions +- [`packagemap.yaml`][packagemap-yaml] for name resolution +- Privilege escalation (sudo/doas) for apt/dnf installations +- Terminal/TTY for interactive tool selection + +[installation]: installation.md +[dotfiles-setup]: dotfiles-setup.md +[pkg-resolution]: package-resolution.md +[packagemap-yaml]: ../../installer/internal/config/packagemap.yaml +[tools-yaml]: ../../installer/internal/config/tools.yaml +[domain-optional-tools]: ../domain.md#optional-tools diff --git a/dot_claude/CLAUDE.md b/dot_claude/CLAUDE.md index 66796c1..9b87d23 100644 --- a/dot_claude/CLAUDE.md +++ b/dot_claude/CLAUDE.md @@ -12,6 +12,13 @@ - Always assume the current file state is correct - never revert based on cached/stale versions. - Clean up any temporary files, scripts, or helpers created during the task. +## Plan Execution + +- When a plan specifies model assignments for sub-plans, respect them exactly. +- When a plan qualifies for Agent Teams (per the planning skill criteria), use Agent Teams (TeamCreate) — never ad-hoc Task sub-agents. Agent Team teammates have their own context windows and can write files; Task sub-agents cannot write files regardless of permission mode and consume the main context window. +- If a sub-agent fails, diagnose the failure and retry with a fix — do NOT silently take over the work yourself. +- If sub-agent execution cannot be made to work after a reasonable attempt, STOP and ask before proceeding. Never fall back to a more expensive model without explicit approval. + ## Session Summaries - Summarize work at the end of edit sessions. diff --git a/dot_claude/agents/component-docs-reviewer.md b/dot_claude/agents/component-docs-reviewer.md new file mode 100644 index 0000000..a355d33 --- /dev/null +++ b/dot_claude/agents/component-docs-reviewer.md @@ -0,0 +1,113 @@ +--- +name: component-docs-reviewer +description: "Use this agent to review component-level documentation after implementation completes. Identifies implementation-vs-plan drift in docs that describe code internals — interfaces, behavior, patterns. Returns findings so the calling agent can make fixes. Domain, architecture, and process docs are updated by a dedicated documentation sub-plan during planning; this agent handles only component docs.\n\n\nContext: A feature plan has been fully executed and the project has component-level docs.\nuser: \"Run component-docs-reviewer to check if component docs still match the implementation.\"\nassistant: \"I'll diff the implementation against component docs and report any drift.\"\n\nPost-execution component doc review. Launch component-docs-reviewer.\n\n\n\n\nContext: A refactoring session changed internal interfaces and the project has component docs.\nuser: \"Check if the refactoring broke any component documentation.\"\nassistant: \"I'll review the changes and report which component docs reference old interfaces.\"\n\nComponent docs may reference old signatures. Launch component-docs-reviewer.\n\n" +tools: Read, Bash, Glob, Grep +memory: project +--- + +You are a component documentation reviewer. Your job is to verify that component-level documentation (module internals, interfaces, behavior, code patterns) still matches the actual implementation after a coding session, and report any drift you find. + +You do NOT write or edit files. Return your findings as your response — the calling agent is responsible for making fixes based on your report. + +**Your scope is strictly component docs.** Domain docs, architecture docs, and process docs are handled by a documentation sub-plan during planning — those are planned upfront and human-reviewed. You handle only the implementation-detail docs where the code may have drifted from the plan. + +## Memory + +Consult your agent memory before starting work — it contains knowledge about this project's component doc locations, interface documentation patterns, and conventions from previous reviews. This saves you from re-discovering the doc layout. + +After completing your review, update your agent memory with component doc locations, documentation patterns, and conventions you discovered. Write concise notes about what you found and where. Keep memory focused on facts that help future reviews start faster. + +## What You Review + +Component documentation describes code internals: +- Interface signatures and method contracts +- Internal behavior and data flow within a module +- Code patterns and conventions specific to a component +- Dependencies between internal modules + +You do NOT review: +- Domain docs (entity definitions, terminology) — planned upfront +- Architecture docs (system structure, layer boundaries) — planned upfront +- Process docs (end-to-end workflows) — planned upfront + +## Workflow + +### Step 1: Identify What Changed + +1. Run `git diff` to understand what was modified in the session +2. Focus on implementation changes that affect documented interfaces, behavior, or patterns: + - Modified function/method signatures + - Changed internal behavior or data flow + - Renamed or moved packages/modules + - Deleted or added interfaces + - Changed error handling patterns + +### Step 2: Find Affected Component Documentation + +1. Read AGENTS.md for documentation pointers +2. Search for component docs that reference the changed code (file paths, function names, type names) +3. Check for docs co-located with the changed code (e.g., `docs/components/`, module-level READMEs) + +**If no component documentation exists**, return early — there's nothing to review. + +### Step 3: Identify Drift + +For each affected component doc: + +1. **Read the current doc** to understand what it describes +2. **Read the actual implementation** (the code) to see what really exists +3. **Compare** — identify where the doc no longer matches the code +4. **Categorize** each finding by type + +Types of drift: + +| Drift Type | What's Wrong | +|---|---| +| Interface change | Doc shows old signatures, params, or return types | +| Behavioral change | Doc describes old internal logic or data flow | +| Dependency change | Doc references old internal dependencies | +| Stale reference | Doc references removed code, renamed entities, or deleted files | +| Missing coverage | New interfaces or patterns exist in code but not in docs | + +### Step 4: Verify Cross-References + +Check that cross-references between component docs are still valid — one doc may reference interfaces documented in another. + +## Response Format + +Return your findings as your response using this structure: + +```markdown +# Component Documentation Review + +## Summary + + +## Findings + +### + +#### : +- **Doc says**: +- **Code says**: +- **Fix**: + +#### : +... + +### +... + +## No Issues Found + +``` + +Be specific in the "Fix" field — provide enough detail that the calling agent can make the edit without re-reading the code. Include the exact section name, the incorrect text, and what it should say. + +## Rules + +- **Component docs only** — never report on domain, architecture, or process docs. Those are managed by the documentation sub-plan. +- **Return findings, never write files** — the calling agent makes fixes based on your report. +- **Always compare against actual code, not the plan** — the whole point is catching plan-vs-implementation drift. +- **Be specific and actionable** — every finding must include exactly what's wrong and how to fix it. +- **Read docs first, then code** — documentation is cheaper than code exploration. Only read code files that docs reference. diff --git a/dot_claude/agents/plan-architect-reviewer.md b/dot_claude/agents/plan-architect-reviewer.md index bbf534e..6a3f3e6 100644 --- a/dot_claude/agents/plan-architect-reviewer.md +++ b/dot_claude/agents/plan-architect-reviewer.md @@ -1,13 +1,20 @@ --- name: plan-architect-reviewer description: "Use this agent to review master plans and their sub-plan decompositions for architectural soundness. Evaluates whether the decomposition boundaries are in the right places, dependencies between sub-plans are minimal and correctly captured, the pieces will fit together when assembled, and the overall approach is feasible.\n\n\nContext: A master plan has been created for adding a new authentication system with 5 sub-plans.\nuser: \"Review the master plan and sub-plans in .claude/plans/auth-system/ for architectural soundness.\"\nassistant: \"I'll review the plan decomposition for boundary correctness, dependency completeness, and integration feasibility.\"\n\nInvoke plan-architect-reviewer after initial plan creation (Phase 5, Step 1 of project-feature-planning) to catch decomposition issues before sub-plans are reviewed individually.\n\n\n\n\nContext: A sub-plan review found that two sub-plans have a hidden circular dependency. The master plan was updated and needs re-review.\nuser: \"The master plan was updated after sub-plan review feedback. Re-review the affected parts.\"\nassistant: \"I'll re-evaluate the changed boundaries and dependency graph to confirm the circular dependency is resolved.\"\n\nInvoke plan-architect-reviewer during the convergence loop when master plan changes need re-validation.\n\n" -tools: Read, Write, Glob, Grep +tools: Read, Glob, Grep +memory: project --- You are an architecture reviewer. Your job is to review feature plans — specifically, a master plan and its sub-plan decomposition — and find problems before an executing agent attempts implementation. You are NOT here to praise, summarize, or restate the plan. You are here to find what's wrong with it. +## Memory + +Consult your agent memory before starting work — it contains knowledge about this project's architecture, module boundaries, key abstractions, and file locations from previous reviews. This saves you from re-exploring the codebase. + +After completing your review, update your agent memory with architectural patterns, module boundaries, key abstractions, and file locations you discovered. Write concise notes about what you found and where. Keep memory focused on facts that help future reviews start faster. + ## What You Review You will be given a path to a plan directory (e.g., `.claude/plans//`) containing: @@ -46,6 +53,8 @@ Read the master plan and every sub-plan. Understand the full picture before maki ### 5. Evaluate Against the Codebase +**Read all available project documentation first** — `AGENTS.md`, `docs/`, `doc/`, component-level docs. Documentation is orders of magnitude cheaper than code exploration. Do NOT use Glob/Grep to explore code before reading available documentation. Only use Glob/Grep to verify specific claims the plan makes about the codebase. + - Do the proposed changes conflict with existing architecture or patterns? - Are there existing abstractions the plan should use but doesn't? - Does the plan introduce unnecessary complexity where simpler approaches exist in the codebase? @@ -59,7 +68,7 @@ Read the master plan and every sub-plan. Understand the full picture before maki ## Output Format -Write your findings to the `reviews/` subdirectory within the plan directory you were given. Use the naming pattern `.architect.md` (e.g., `reviews/00-master.architect.md`). +Return your findings as your response using the format below. The calling agent (planner) is responsible for writing review files — you do not write files. Be direct and specific — every finding must reference the exact plan file and section it relates to. diff --git a/dot_claude/agents/plan-risk-reviewer.md b/dot_claude/agents/plan-risk-reviewer.md index 6ea3e92..f317b45 100644 --- a/dot_claude/agents/plan-risk-reviewer.md +++ b/dot_claude/agents/plan-risk-reviewer.md @@ -1,13 +1,20 @@ --- name: plan-risk-reviewer description: "Use this agent to review master plans and their sub-plan decompositions for technical risks and feasibility issues. Identifies migration pitfalls, backward-compatibility landmines, missing rollback strategies, and sub-plans that may be significantly harder or more complex than they appear.\n\n\nContext: A master plan has been created for migrating a database schema with 4 sub-plans.\nuser: \"Review the plan in .claude/plans/db-migration/ for risks and feasibility.\"\nassistant: \"I'll review the plan for technical risks, hidden complexity, and feasibility issues.\"\n\nInvoke plan-risk-reviewer after initial plan creation (Phase 5, Step 1 of project-feature-planning) alongside plan-architect-reviewer to catch risks before sub-plans are reviewed individually.\n\n\n\n\nContext: The architecture reviewer flagged a decomposition change. The master plan was updated and needs risk re-assessment.\nuser: \"The master plan was restructured after architecture review. Re-assess risks for the affected parts.\"\nassistant: \"I'll re-evaluate the changed plan for new risks introduced by the restructuring.\"\n\nInvoke plan-risk-reviewer during the convergence loop when master plan changes may have introduced new risks.\n\n" -tools: Read, Write, Glob, Grep +tools: Read, Glob, Grep +memory: project --- You are a risk and feasibility reviewer. Your job is to review feature plans — specifically, a master plan and its sub-plan decomposition — and find risks, hidden complexity, and feasibility problems before an executing agent attempts implementation. You are NOT here to praise, summarize, or restate the plan. You are here to find what could go wrong. +## Memory + +Consult your agent memory before starting work — it contains knowledge about this project's tech stack, known risk areas, past migration patterns, and complexity hotspots from previous reviews. This saves you from re-exploring the codebase. + +After completing your review, update your agent memory with risk patterns, complexity hotspots, tech stack details, and areas that proved harder than expected. Write concise notes about what you found and where. Keep memory focused on facts that help future risk assessments start faster. + ## What You Review You will be given a path to a plan directory (e.g., `.claude/plans//`) containing: @@ -18,9 +25,9 @@ You also have access to the full codebase to verify claims and assess feasibilit ## How You Review -### 1. Read All Plan Files +### 1. Read All Plan Files and Project Documentation -Read the master plan and every sub-plan. Understand the full picture before making any judgments. +Read the master plan and every sub-plan. Then **read all available project documentation** — `AGENTS.md`, `docs/`, `doc/`, component-level docs. Documentation is orders of magnitude cheaper than code exploration. Do NOT use Glob/Grep to explore code before reading available documentation. Only use Glob/Grep to verify specific claims the plan makes about the codebase. ### 2. Evaluate Feasibility @@ -58,7 +65,7 @@ Read the master plan and every sub-plan. Understand the full picture before maki ## Output Format -Write your findings to the `reviews/` subdirectory within the plan directory you were given. Use the naming pattern `.risk.md` (e.g., `reviews/00-master.risk.md`). +Return your findings as your response using the format below. The calling agent (planner) is responsible for writing review files — you do not write files. Be direct and specific — every finding must reference the exact plan file and section it relates to. diff --git a/dot_claude/skills/developing-plan-reviewers/SKILL.md b/dot_claude/skills/developing-plan-reviewers/SKILL.md index 19decfb..61f614c 100644 --- a/dot_claude/skills/developing-plan-reviewers/SKILL.md +++ b/dot_claude/skills/developing-plan-reviewers/SKILL.md @@ -34,7 +34,8 @@ It does **not** define how to review. Domain knowledge comes from the preloaded --- name: plan--reviewer description: "Use this agent to review sub-plans that involve . Evaluates against project conventions.\n\n\nContext: A sub-plan covers implementation within a feature plan.\nuser: \"Review sub-plan 02-.md for correctness.\"\nassistant: \"I'll review the sub-plan for issues using the plan--reviewer agent.\"\n\nSub-plan involves work. Launch the domain-specific reviewer.\n\n" -tools: Read, Write, Glob, Grep +tools: Read, Glob, Grep +memory: project skills: - - @@ -47,6 +48,17 @@ conventions, avoids known pitfalls, and will produce correct results. You are NOT here to praise, summarize, or restate the plan. You are here to find what's wrong with it from a perspective. +## Memory + +Consult your agent memory before starting work — it contains knowledge about +this project's from +previous reviews. This saves you from re-exploring the codebase. + +After completing your review, update your agent memory with +patterns, file locations, and conventions you discovered. Write concise notes +about what you found and where. Keep memory focused on facts that help future +reviews start faster. + ## What You Review You will be given a path to a specific sub-plan file (e.g., @@ -56,21 +68,23 @@ codebase to verify claims. ## How You Review 1. **Read the sub-plan** completely. -2. **Read project documentation** — AGENTS.md, component-level AGENTS.md - files, and any project documentation (`docs/`, `doc/`, etc.). - Documentation is dramatically - cheaper than code exploration. +2. **Read ALL project documentation first** — AGENTS.md, component-level + AGENTS.md files, and any project documentation (`docs/`, `doc/`, etc.). + Documentation is orders of magnitude cheaper than code exploration. Do + NOT use Glob/Grep to explore code before reading all available + documentation. 3. **Apply your skills** to evaluate the plan against project conventions. Your preloaded skills encode the conventions for this domain. Use them as your review criteria. -4. **Verify claims against the codebase** — if the plan references existing - code (interfaces, packages, patterns), use Glob and Grep to confirm - they exist and the plan's approach is compatible. +4. **Verify specific claims only** — use Glob and Grep only to confirm + specific claims the plan makes (e.g., an interface exists, a file path + is correct). Do not broadly explore the codebase. ## Output Format -Write your findings to `reviews/..md` inside the -plan directory. Use the exact format below. +Return your findings as your response using the format below. The calling +agent (planner) is responsible for writing review files — you do not write +files. [Insert the standard output format template from this skill] @@ -92,7 +106,8 @@ plan directory. Use the exact format below. ### Key Choices -- **`tools: Read, Write, Glob, Grep`** — Write is only for the review output file. Match the global reviewer pattern. +- **`tools: Read, Glob, Grep`** — No Write. Reviewers return findings as their Task response; the planner writes review files. This avoids the problem where Task sub-agents cannot write files regardless of permission mode. +- **`memory: project`** — Persistent memory scoped to the project. Reviewers build up knowledge about codebase patterns, file locations, and conventions across reviews. This dramatically reduces redundant codebase exploration — the reviewer knows where to look instead of rediscovering the same modules each time. Memory auto-enables Read/Write/Edit for the memory directory only, so it doesn't conflict with the no-Write-to-codebase design. - **`skills`** — the differentiator. Preloads domain knowledge so the reviewer doesn't need to discover conventions at runtime. Skills are the review criteria — the agent should not hardcode evaluation checklists that duplicate what skills already teach. See [Choosing Skills to Preload](#choosing-skills-to-preload). - **No `model` field** — inherits from parent, matching the global reviewers. - **Description** — must be clear enough for the planner to match it to sub-plans. Include what domain it covers and what it evaluates. Use `\n` escapes for multi-line content (not literal newlines) to ensure valid YAML frontmatter. @@ -158,7 +173,7 @@ Complete, copy-and-adapt reviewer examples: ## Rules - **Always follow the standard output format** — the planner depends on the Verdict/Critical Findings/Concerns/Observations structure -- **Write output to `reviews/..md`** — never elsewhere +- **Return findings as response, never write files** — the planner writes review output to `reviews/..md` - **Review the plan, not the code** — evaluate whether the plan's strategy is sound for the domain; code-level review happens during execution - **Be specific and actionable** — every finding must reference the exact plan section and provide a recommendation - **Don't duplicate architecture or risk review** — focus only on domain expertise diff --git a/dot_claude/skills/developing-plan-reviewers/examples/plan-api-reviewer.md b/dot_claude/skills/developing-plan-reviewers/examples/plan-api-reviewer.md index 58ff225..9ed5240 100644 --- a/dot_claude/skills/developing-plan-reviewers/examples/plan-api-reviewer.md +++ b/dot_claude/skills/developing-plan-reviewers/examples/plan-api-reviewer.md @@ -6,7 +6,8 @@ For a project with HTTP APIs. Skills provide the review criteria — the agent d --- name: plan-api-reviewer description: "Use this agent to review sub-plans that involve API endpoints or HTTP layer changes. Evaluates endpoint design, request/response contracts, error responses, and backward compatibility.\n\n\nContext: A sub-plan covers adding new REST endpoints.\nuser: \"Review sub-plan 03-api-endpoints.md for API correctness.\"\nassistant: \"I'll review the sub-plan for API issues using the plan-api-reviewer.\"\n\nSub-plan involves API/HTTP work. Launch the API domain reviewer.\n\n" -tools: Read, Write, Glob, Grep +tools: Read, Glob, Grep +memory: project skills: - writing-go-code --- @@ -18,6 +19,18 @@ and changes don't break existing consumers. You are NOT here to praise, summarize, or restate the plan. You are here to find what's wrong with it from an API perspective. +## Memory + +Consult your agent memory before starting work — it contains knowledge about +this project's endpoint patterns, request/response types, middleware, and API +conventions from previous reviews. This saves you from re-exploring the +codebase. + +After completing your review, update your agent memory with endpoint patterns, +type definitions, middleware chains, and API conventions you discovered. Write +concise notes about what you found and where. Keep memory focused on facts +that help future reviews start faster. + ## What You Review You will be given a path to a specific sub-plan file. You also have access to @@ -26,19 +39,20 @@ the full codebase to verify claims and check existing API patterns. ## How You Review 1. **Read the sub-plan** completely. -2. **Read project documentation** — AGENTS.md, API docs, and any - project documentation (`docs/`, `doc/`, etc.). Documentation is - dramatically cheaper than code exploration. +2. **Read ALL project documentation first** — AGENTS.md, API docs, and any + project documentation (`docs/`, `doc/`, etc.). Documentation is orders of + magnitude cheaper than code exploration. Do NOT use Glob/Grep to explore + code before reading all available documentation. 3. **Apply your skills** to evaluate the plan against project conventions. Your preloaded skills encode the coding standards. Use them plus existing API patterns in the codebase as your review criteria. -4. **Verify claims against the codebase** — if the plan references existing - endpoints, middleware, or request/response types, use Glob and Grep to - confirm they exist and the plan's approach is compatible. +4. **Verify specific claims only** — use Glob and Grep only to confirm + specific claims the plan makes (e.g., an endpoint exists, a type is + defined). Do not broadly explore the codebase. ## Output Format -Write your findings to `reviews/.api.md` inside the plan directory. +Return your findings as your response. The planner writes review files. # API Review: diff --git a/dot_claude/skills/developing-plan-reviewers/examples/plan-go-reviewer.md b/dot_claude/skills/developing-plan-reviewers/examples/plan-go-reviewer.md index d5df1e7..1a3f7da 100644 --- a/dot_claude/skills/developing-plan-reviewers/examples/plan-go-reviewer.md +++ b/dot_claude/skills/developing-plan-reviewers/examples/plan-go-reviewer.md @@ -6,7 +6,8 @@ For a project with Go code. Skills provide the review criteria — the agent doe --- name: plan-go-reviewer description: "Use this agent to review sub-plans that involve Go implementation. Evaluates proposed Go code structure, error handling, interface design, and test strategy against project conventions.\n\n\nContext: A sub-plan covers implementing a new Go service.\nuser: \"Review sub-plan 02-user-service.md for Go correctness.\"\nassistant: \"I'll review the sub-plan for Go issues using the plan-go-reviewer.\"\n\nSub-plan involves Go implementation. Launch the Go domain reviewer.\n\n" -tools: Read, Write, Glob, Grep +tools: Read, Glob, Grep +memory: project skills: - writing-go-code - applying-effective-go @@ -19,6 +20,18 @@ conventions, and will produce maintainable, testable code. You are NOT here to praise, summarize, or restate the plan. You are here to find what's wrong with it from a Go perspective. +## Memory + +Consult your agent memory before starting work — it contains knowledge about +this project's Go package structure, interfaces, error handling patterns, and +code conventions from previous reviews. This saves you from re-exploring the +codebase. + +After completing your review, update your agent memory with package locations, +interface definitions, error handling patterns, and Go conventions you +discovered. Write concise notes about what you found and where. Keep memory +focused on facts that help future reviews start faster. + ## What You Review You will be given a path to a specific sub-plan file. You also have access to @@ -27,19 +40,21 @@ the full codebase to verify claims and check existing patterns. ## How You Review 1. **Read the sub-plan** completely. -2. **Read project documentation** — AGENTS.md, component-level AGENTS.md - files, and any project documentation (`docs/`, `doc/`, etc.). - Documentation is dramatically cheaper than code exploration. +2. **Read ALL project documentation first** — AGENTS.md, component-level + AGENTS.md files, and any project documentation (`docs/`, `doc/`, etc.). + Documentation is orders of magnitude cheaper than code exploration. Do + NOT use Glob/Grep to explore code before reading all available + documentation. 3. **Apply your skills** to evaluate the plan against project conventions. Your preloaded skills encode Go idioms, coding standards, and test patterns. Use them as your review criteria. -4. **Verify claims against the codebase** — if the plan references existing - code (interfaces, packages, patterns), use Glob and Grep to confirm - they exist and the plan's approach is compatible. +4. **Verify specific claims only** — use Glob and Grep only to confirm + specific claims the plan makes (e.g., an interface exists, a package + structure is correct). Do not broadly explore the codebase. ## Output Format -Write your findings to `reviews/.go.md` inside the plan directory. +Return your findings as your response. The planner writes review files. # Go Review: diff --git a/dot_claude/skills/documenting-business-processes/SKILL.md b/dot_claude/skills/documenting-business-processes/SKILL.md index 6bbf2cb..42016a5 100644 --- a/dot_claude/skills/documenting-business-processes/SKILL.md +++ b/dot_claude/skills/documenting-business-processes/SKILL.md @@ -113,6 +113,31 @@ Guidelines: - Mark error terminals distinctly (e.g., red styling or stop symbols) - Use subgraphs to separate phases when a process has distinct stages +#### Sub-Process Decomposition + +When tracing a process, some steps may be complex enough to warrant their own dedicated process doc. Decompose a step into a sub-process when: + +- The step has its own **decision branches, failure modes, or actors** beyond the parent flow +- The step involves **multiple sequential actions** that would bloat the parent doc +- The step is **reusable** — referenced by multiple parent processes +- Documenting it inline would make the parent doc's flow section harder to scan + +**When you identify a sub-process:** + +1. **In the parent doc**: Replace the inline description with a link to the sub-process doc. Keep only a one-line summary in the flow step — the detail lives in the sub-process doc. + ```markdown + 3. **[Check system compatibility][compat-check]** — Verify the system meets minimum requirements + ``` +2. **Add a Sub-Processes table** to the parent doc (if one doesn't exist) listing all sub-processes with links and brief descriptions. +3. **Create the sub-process doc** using the same template as the parent — Overview, Trigger, Actors, Diagram, Flow, Failure Scenarios, State Changes, Dependencies. The trigger should reference the parent process (e.g., "Called during step 3 of [installation][installation]"). +4. **Match existing patterns** — if the parent doc already has sub-process links, follow the exact same structure (link style, summary length, Sub-Processes table format). Consistency across sub-processes matters. + +**When NOT to decompose:** + +- The step is a single action with no branching or failure modes +- The step's behavior is fully captured in one sentence +- Breaking it out would create a trivially short doc that adds navigation overhead without value + ### Step 4: Update AGENTS.md Pointers If new documentation files were created, propose adding a pointer in AGENTS.md: @@ -139,4 +164,6 @@ See `docs/processes/.md` for . - **Always include failure paths** — happy-path-only docs are incomplete and misleading - **Business language first** — describe what happens from the business perspective, then note which components are involved - **Use reference-style links** — when linking to other docs or source files, use reference links (`[text][ref]` with `[ref]: path` at the bottom of the file) rather than inline links. They read better in source and are easier to maintain. +- **Decompose complex steps into sub-processes** — if a step has its own decision branches, failure modes, or multiple sequential actions, it needs its own doc. Don't inline what should be a sub-process. +- **Match existing sub-process patterns** — if a parent doc already has sub-process links, new sub-processes must follow the exact same structure and link style - **Propose structure first** — if no process docs exist yet, propose a directory structure and format before creating files diff --git a/dot_claude/skills/documenting-domain/SKILL.md b/dot_claude/skills/documenting-domain/SKILL.md index 0659327..bceea9f 100644 --- a/dot_claude/skills/documenting-domain/SKILL.md +++ b/dot_claude/skills/documenting-domain/SKILL.md @@ -110,5 +110,6 @@ See `docs/domain/.md` for . - **No code snippets** — domain docs describe the business model; component docs show the implementation - **One definition per concept** — if a term is already defined in domain docs, other layers must reference it, not redefine it - **Defer process details to process docs** — if a concept involves a multi-step flow (loading chain, resolution sequence, initialization steps), define the concept here and link to the process doc for the "how" +- **No behavioral conditionals** — if you're writing "when X happens", "in Y mode", or "if Z flag is set", that's a process description. Domain docs define static properties of concepts (what they are, what they contain, how they relate). Conditional behavior, modes, flags, and runtime decisions belong in process docs. - **Use reference-style links** — when linking to other docs or source files, use reference links (`[text][ref]` with `[ref]: path` at the bottom of the file) rather than inline links. They read better in source and are easier to maintain. - **Propose structure first** — if no domain docs exist yet, propose a directory structure and format to the user before creating files diff --git a/dot_claude/skills/planning-project-features/SKILL.md b/dot_claude/skills/planning-project-features/SKILL.md index fa09c47..11c63ca 100644 --- a/dot_claude/skills/planning-project-features/SKILL.md +++ b/dot_claude/skills/planning-project-features/SKILL.md @@ -64,6 +64,7 @@ This is the most critical phase. Break the feature into sub-plans: 3. **Embed all necessary context**: Each sub-plan must include the interfaces, data shapes, conventions, and file contents an executing agent needs. Don't assume the agent has read the master plan or any other sub-plan. 4. **Define clear inputs and outputs**: If sub-plan B depends on sub-plan A, sub-plan B must specify exactly what it expects to exist (e.g., "a `UserService` interface in `internal/service/user.go` with methods `Create(ctx, user) error` and `GetByID(ctx, id) (User, error)`"). 5. **Keep sub-plans small**: A good sub-plan should be completable in a single focused session. If it feels too big, split it further. +6. **Plan documentation updates as a sub-plan**: If the feature affects documented domain concepts, architecture, or business processes, add a final sub-plan that updates those docs. This sub-plan is planned upfront — the planner already knows what's changing and can specify exactly which docs to update, which new docs to create, and which existing docs to use as structural patterns. This makes documentation updates human-reviewable alongside the rest of the plan. See [Documentation Sub-Plan](#documentation-sub-plan) for guidance on what belongs here vs. post-execution review. Present the decomposition to the user for review before writing the actual plan files. @@ -128,28 +129,30 @@ The review loop uses two types of reviewer agents: #### Review Output Location -All review output is written to `reviews/` within the plan directory, named `..md`: +Review output is saved to `reviews/` within the plan directory, named `..md`: ``` .claude/plans//reviews/ ├── 00-master.architect.md # Architecture review of master plan ├── 00-master.risk.md # Risk review of master plan -├── 01-data-model.codebase.md # Codebase review of sub-plan 01 -├── 02-api-layer.codebase.md # Codebase review of sub-plan 02 +├── 01-data-model.installer.md # Installer review of sub-plan 01 +├── 02-api-layer.ci.md # CI review of sub-plan 02 └── ... ``` +**Important**: Reviewer agents return their findings as their Task response — they do not write files. The planner is responsible for writing each reviewer's output to the appropriate `reviews/` file. + This directory is ephemeral — already covered by the `.claude/plans/` ignore rule — but persists locally across sessions for reference. #### Step 1: Master Plan Review -Launch `plan-architect-reviewer` and `plan-risk-reviewer` against the master plan (in parallel — they are independent). Pass the plan directory path so they can read all plan files and cross-reference against the codebase. Instruct each reviewer to write its output to `reviews/00-master..md`. +Launch `plan-architect-reviewer` and `plan-risk-reviewer` against the master plan (in parallel — they are independent). Pass the plan directory path so they can read all plan files and cross-reference against the codebase. Each reviewer returns its findings as a response — write them to `reviews/00-master.architect.md` and `reviews/00-master.risk.md` respectively. Incorporate findings into both the master plan and affected sub-plans. #### Step 2: Sub-Plan Review -After the master plan review is resolved, launch each sub-plan's assigned reviewer (from the `## Reviewer` field) against it. Sub-plan reviews can run in parallel — even when different sub-plans use different reviewers. Each reviewer writes its output to `reviews/..md`. +After the master plan review is resolved, launch each sub-plan's assigned reviewer (from the `## Reviewer` field) against it. Sub-plan reviews can run in parallel — even when different sub-plans use different reviewers. Each reviewer returns its findings as a response — write them to `reviews/..md`. **Output normalization**: If a local reviewer's output doesn't follow the standard format (Verdict, Critical Findings, Concerns, Observations), normalize it before incorporating. The planner interprets the reviewer's findings and translates them into actionable changes to the plan. @@ -165,17 +168,18 @@ The user may also request additional specialized reviewers (e.g., security, perf Present the fully reviewed plan (master + sub-plans) along with a summary of review findings and how they were addressed. Only mark as ready when the user explicitly approves. -### Post-Execution: Documentation Updates +### Post-Execution: Component Documentation Review + +Domain, architecture, and process documentation updates are handled by the documentation sub-plan (Phase 3, step 6) — planned upfront and human-reviewed. -After sub-plans have been executed, the `updating-documentation` skill should be run to keep project documentation in sync with the changes. This is not part of the planning workflow itself, but should be noted in the master plan as a final step: +**Component docs** are the exception: they describe implementation details (interfaces, internal behavior, code patterns) that may deviate from the plan during execution. For projects that have component documentation, run the `component-docs-reviewer` agent after all sub-plans complete to catch implementation-vs-plan drift in component docs. ```markdown ## Post-Execution -After all sub-plans are complete, run the `updating-documentation` skill to update affected docs. +If this project has component-level documentation, run the `component-docs-reviewer` agent to verify +component docs still match the actual implementation. ``` -This ensures that the documentation investment compounds — each feature execution improves docs for the next planning session. - ## Master Plan Structure The master plan is the orchestration document. It does NOT contain implementation details — those live in sub-plans. @@ -210,16 +214,11 @@ The master plan is the orchestration document. It does NOT contain implementatio ## Team Execution (Agent Teams) -**Use Agent Teams when**: -- ✅ Plan has 2+ sub-plans with meaningful scope -- ✅ Sub-plans are self-contained (minimal cross-dependencies) -- ✅ Sub-plans touch different files (avoid conflicts) -- ✅ Parallelization offers significant time savings +**Agent Teams are REQUIRED for plans with 2+ sub-plans.** Do not use Task sub-agents — they cannot write files and consume the main context window. -**Skip Agent Teams when**: +**The only exception** — skip Agent Teams when: - ❌ Single sub-plan (just execute directly) -- ❌ Tiny sub-plans (overhead > benefit, e.g., "add one import") -- ❌ Highly coupled sub-plans (too much coordination needed) +- ❌ All sub-plans are trivially small (e.g., "add one import") **Setup**: 1. Enable Agent Teams: `export CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1` @@ -264,7 +263,8 @@ Create a team with teammates to execute .claude/plans//00-mast | ... | ... | ## Post-Execution -After all sub-plans are complete, run the `updating-documentation` skill to update affected project documentation. +If this project has component-level documentation, run the `component-docs-reviewer` agent to verify +component docs still match the actual implementation. ``` ## Sub-Plan Structure @@ -317,8 +317,40 @@ Examples: ... ``` +## Documentation Sub-Plan + +When a feature affects documented domain concepts, architecture, or business processes, the planner adds a **documentation sub-plan** as the final sub-plan in the execution order. This makes doc updates part of the plan — visible, reviewable, and deliberate. + +### What Goes in the Documentation Sub-Plan + +| Doc Level | Planned Upfront? | Rationale | +|---|---|---| +| Domain docs | Yes | The planner knows what domain concepts are changing | +| Architecture docs | Yes | The planner knows what structural changes are happening | +| Process docs | Yes | The planner knows what flows are being added/modified | +| Component docs | **No** — post-execution | Component docs describe implementation details that may drift from the plan | + +### How to Write It + +The documentation sub-plan follows the standard sub-plan template but its implementation steps are doc edits, not code changes. Be specific: + +- **Which existing docs to update** — file paths, which sections, what to change +- **Which new docs to create** — file paths, which existing doc to use as a structural pattern, what the new doc should cover +- **Structural pattern matching** — if existing docs follow a pattern (e.g., process steps link to sub-process docs), new additions must follow it. Specify the pattern explicitly. +- **Required skills**: List the documenting skills the executing agent needs (e.g., `documenting-business-processes` for new process docs, `documenting-domain` for new domain entries) + +### When to Skip It + +Skip the documentation sub-plan when: +- The feature doesn't affect any documented concepts, flows, or architecture +- The only doc impact is component-level (handled by `component-docs-reviewer` post-execution) +- No project documentation exists yet (recommend creating initial docs as a separate effort) + ## Rules (Non-Negotiable) +- **Always respect model assignments during execution** — Sub-plan model assignments (Haiku, Sonnet, Opus) are deliberate cost-optimization decisions. When executing a plan, the assigned model MUST be used. If a sub-agent fails at the assigned model, diagnose and fix the failure (e.g., permission mode, tool access). Never silently fall back to executing the work on a more expensive model. If the issue cannot be resolved, stop and ask the user how to proceed. +- **Use Agent Teams for multi-plan execution — ALWAYS** — When a plan has 2+ sub-plans, ALWAYS use Agent Teams (TeamCreate) to spawn teammates for execution. This is not optional. Agent Team teammates have their own independent context windows (preserving the lead's context budget) and have full tool access including file writes. Task sub-agents (spawned via the Task tool) cannot write files regardless of permission mode and consume the main context window. Never use Task sub-agents for plan execution. If Agent Teams are unavailable or fail, STOP and ask the user — do not silently execute sub-plans on the main agent. +- **Reviewers return findings, planner writes files** — Reviewer agents (both global and local) return their findings as their Task response. They do not write files. The planner is responsible for writing review output to `reviews/..md`. - **Never write a plan based on incomplete information** - **Never invent requirements the user didn't specify** - **Always decompose into sub-plans** — a single monolithic plan is a failure mode diff --git a/dot_claude/skills/updating-documentation/SKILL.md b/dot_claude/skills/updating-documentation/SKILL.md deleted file mode 100644 index 465dfe0..0000000 --- a/dot_claude/skills/updating-documentation/SKILL.md +++ /dev/null @@ -1,125 +0,0 @@ ---- -name: updating-documentation -description: Update existing project documentation to reflect changes made during a session. Analyzes what changed in code and surgically updates only affected documentation. Use when (1) a coding session has modified component behavior or interfaces, (2) implementation work is complete and docs need to stay in sync, (3) refactoring changed patterns or conventions, or (4) code was deleted and docs may reference removed things. Run at the end of a session — "when it's all said and done." ---- - -# Updating Documentation - -Surgically update existing documentation to reflect changes made during a coding session. Run this **after implementation work is complete** — when it's all said and done. - -## Core Principles - -1. **Surgical Updates**: Only touch documentation affected by the session's changes. Never rewrite unrelated docs. -2. **Change-Driven**: Start from what changed (git diff, modified files), then trace to affected docs. Never explore the codebase looking for things to update. -3. **Preserve Style**: Match the existing documentation's tone, format, and level of detail. -4. **No Fabrication**: Only document changes you can verify. Don't speculate about intent or future plans. -5. **Minimal Disruption**: Small, precise edits over wholesale rewrites. -6. **Respect the Hierarchy**: Changes may affect docs at multiple levels. A renamed domain entity needs updates in domain docs, which may cascade to architecture, process, and component docs that reference it. Always update from the top down. - -## Documentation Hierarchy - -This skill updates documentation across all levels. When tracing affected docs, check every level: - -``` -Domain ← Entity definitions, terminology, domain rules - ↓ -Architecture ← System structure, design decisions, layer boundaries - ↓ -Business Processes ← End-to-end workflows, failure scenarios - ↓ -Components ← Module internals, interfaces, behavior -``` - -A single code change can affect multiple levels. For example, renaming a domain entity touches domain docs (definition), architecture docs (references), process docs (flow descriptions), and component docs (interface signatures). - -## Workflow - -### Step 1: Identify What Changed - -1. Review the session's changes (`git diff`, modified/created/deleted files) -2. Categorize changes by documentation impact: - - **New components**: May need new documentation (flag for docs-writer) - - **Modified interfaces**: Existing docs may describe old signatures - - **Behavioral changes**: Existing docs may describe old behavior - - **Deleted code**: Docs may reference things that no longer exist - - **New patterns or conventions**: May need documenting if they deviate from existing norms - -### Step 2: Find Affected Documentation - -1. Read AGENTS.md for documentation pointers -2. Search existing docs for references to changed components, functions, types, or files -3. Check if changed files have associated documentation (e.g., `docs/auth.md` for `src/auth/`) -4. Identify docs that describe interfaces, behaviors, or patterns that were modified - -**If no documentation exists for the changed areas**, skip to Step 4 — there's nothing to update. - -### Step 3: Make Targeted Updates - -For each affected doc: - -1. **Read the current doc** to understand what it says -2. **Compare against the changes** to identify what's now wrong or incomplete -3. **Make surgical edits** — update specific sections, don't rewrite the whole file -4. **Add new sections** only if the changes introduced genuinely new concepts within the existing component's scope - -Types of updates: - -| Change Type | Doc Update | -|---|---| -| Interface change (signatures, params, returns) | Update the Key Interfaces section | -| Behavioral change (logic, flow, error handling) | Update descriptions of how things work | -| New dependency added | Update the Dependencies section | -| Dependency removed | Remove stale references | -| Pattern change (new convention adopted) | Update the Patterns & Conventions section | -| Code deleted | Remove references to removed components | - -### Step 4: Handle Documentation Gaps - -If the session's changes introduced undocumented areas: - -1. **Do not write full docs** — that's the job of the appropriate documenting skill -2. **Flag the gap with a specific recommendation**: - - New domain concepts introduced → recommend `documenting-domain` - - Architectural changes (new layers, patterns) → recommend `documenting-architecture` - - New business workflows → recommend `documenting-business-processes` - - New modules or components → recommend `documenting-components` -3. **Update AGENTS.md pointers** only if new doc files were actually created in this session - -### Step 4.5: Check for Duplication - -While updating, watch for concepts that are duplicated across documentation levels: - -1. If a domain term is redefined in component docs, consolidate to domain docs and replace with a reference -2. If an architectural pattern is re-explained in process docs, consolidate to architecture docs and replace with a reference -3. Flag any duplication found to the user — don't silently reorganize large sections - -### Step 5: Verify Consistency - -After updates: - -1. Check that AGENTS.md pointers still point to valid files -2. Ensure no docs reference removed code or renamed interfaces -3. Confirm updated docs accurately reflect the new state - -## Integration with Other Skills - -This skill is designed to run **after other work completes**: - -- **After plan execution**: When an agent finishes implementing a sub-plan, run this to keep documentation in sync -- **After refactoring**: When code structure changes, docs referencing old structure need updates -- **After bug fixes**: If the fix changed documented behavior or revealed incorrect documentation - -This skill does **not** create documentation from scratch — it only updates what exists. For undocumented areas, recommend the appropriate documenting skill: -- `documenting-domain` — for business concepts and terminology -- `documenting-architecture` — for system design and structure -- `documenting-business-processes` — for end-to-end business workflows -- `documenting-components` — for specific modules and interfaces - -## Rules - -- **Never rewrite docs that weren't affected by the session's changes** -- **Never create comprehensive documentation from scratch** — that's the docs-writer's job; only make targeted updates here -- **Always start from the diff** — changes drive updates, not exploration -- **Preserve the original author's voice** — edit surgically, don't rewrite -- **Flag gaps, don't fill them** — if something needs full documentation, recommend docs-writer instead -- **No meta-commentary** — don't add "updated by AI" or session timestamps to docs diff --git a/installer/AGENTS.md b/installer/AGENTS.md index 93ed0eb..4ba7f20 100644 --- a/installer/AGENTS.md +++ b/installer/AGENTS.md @@ -23,8 +23,9 @@ installer/ │ ├── gpg/ # GPG key management │ ├── shell/ # Shell installation │ ├── dotfilesmanager/ # Chezmoi integration -│ └── packageresolver/ # Package name resolution -├── utils/ # Shared utilities +│ ├── packageresolver/ # Package name resolution +│ └── toolsinstaller/ # Optional tools installation +├── utils/ # Shared utilities │ ├── logger/ # Logging with progress display │ ├── osmanager/ # OS operations interface │ ├── privilege/ # Sudo/doas escalation @@ -144,6 +145,18 @@ supported_os: prerequisites: [git, curl] ``` +### tools.yaml (internal/config/) + +Defines optional CLI tools available for installation after dotfiles setup: + +```yaml +tools: + - name: fzf + description: "Fuzzy finder for the command line" + - name: bat + description: "Cat clone with syntax highlighting" +``` + ### packagemap.yaml (internal/config/) Maps generic package codes to manager-specific names: diff --git a/installer/cli/gpg_selector.go b/installer/cli/gpg_selector.go index ef6aee7..48923ab 100644 --- a/installer/cli/gpg_selector.go +++ b/installer/cli/gpg_selector.go @@ -17,9 +17,9 @@ func NewGpgKeySelector(selector Selector[string]) *GpgKeySelector { } // NewDefaultGpgKeySelector constructs a GpgKeySelector with the default HuhSelector. -func NewDefaultGpgKeySelector() *GpgKeySelector { +func NewDefaultGpgKeySelector(accessible bool) *GpgKeySelector { return &GpgKeySelector{ - selector: NewHuhSelector[string](), + selector: NewHuhSelector[string](accessible), } } diff --git a/installer/cli/multiselect_selector.go b/installer/cli/multiselect_selector.go index 6b8ae51..2841b11 100644 --- a/installer/cli/multiselect_selector.go +++ b/installer/cli/multiselect_selector.go @@ -22,11 +22,13 @@ type MultiSelectSelector[T comparable] interface { var _ MultiSelectSelector[string] = (*HuhMultiSelectSelector[string])(nil) // HuhMultiSelectSelector implements MultiSelectSelector using the huh library. -type HuhMultiSelectSelector[T comparable] struct{} +type HuhMultiSelectSelector[T comparable] struct { + accessible bool +} // NewHuhMultiSelectSelector constructs a HuhMultiSelectSelector. -func NewHuhMultiSelectSelector[T comparable]() *HuhMultiSelectSelector[T] { - return &HuhMultiSelectSelector[T]{} +func NewHuhMultiSelectSelector[T comparable](accessible bool) *HuhMultiSelectSelector[T] { + return &HuhMultiSelectSelector[T]{accessible: accessible} } // SelectMultiple implements MultiSelectSelector. @@ -55,7 +57,7 @@ func (s *HuhMultiSelectSelector[T]) SelectMultiple(title string, items []MultiSe Options(options...). Value(&selectedItems), ), - ) + ).WithAccessible(s.accessible) err := form.Run() if err != nil { diff --git a/installer/cli/prerequisite_selector.go b/installer/cli/prerequisite_selector.go index 1f0126a..126f5c3 100644 --- a/installer/cli/prerequisite_selector.go +++ b/installer/cli/prerequisite_selector.go @@ -17,9 +17,9 @@ func NewPrerequisiteSelector(selector MultiSelectSelector[string]) *Prerequisite } // NewDefaultPrerequisiteSelector constructs a PrerequisiteSelector with the default HuhMultiSelectSelector. -func NewDefaultPrerequisiteSelector() *PrerequisiteSelector { +func NewDefaultPrerequisiteSelector(accessible bool) *PrerequisiteSelector { return &PrerequisiteSelector{ - selector: NewHuhMultiSelectSelector[string](), + selector: NewHuhMultiSelectSelector[string](accessible), } } diff --git a/installer/cli/selector.go b/installer/cli/selector.go index dc6b0a2..379fa37 100644 --- a/installer/cli/selector.go +++ b/installer/cli/selector.go @@ -17,11 +17,13 @@ type Selector[T comparable] interface { var _ Selector[string] = (*HuhSelector[string])(nil) // HuhSelector implements Selector using the huh library. -type HuhSelector[T comparable] struct{} +type HuhSelector[T comparable] struct { + accessible bool +} // NewHuhSelector constructs a HuhSelector. -func NewHuhSelector[T comparable]() *HuhSelector[T] { - return &HuhSelector[T]{} +func NewHuhSelector[T comparable](accessible bool) *HuhSelector[T] { + return &HuhSelector[T]{accessible: accessible} } // Select implements Selector. @@ -43,7 +45,7 @@ func (s *HuhSelector[T]) Select(title string, items []T) (T, error) { Options(huh.NewOptions(items...)...). Value(&selectedItem), ), - ) + ).WithAccessible(s.accessible) err := form.Run() if err != nil { @@ -81,7 +83,7 @@ func (s *HuhSelector[T]) SelectWithLabels(title string, items []T, labels []stri Options(options...). Value(&selectedItem), ), - ) + ).WithAccessible(s.accessible) err := form.Run() if err != nil { diff --git a/installer/cli/tool_selector.go b/installer/cli/tool_selector.go new file mode 100644 index 0000000..796b395 --- /dev/null +++ b/installer/cli/tool_selector.go @@ -0,0 +1,56 @@ +package cli + +import ( + "errors" +) + +// ToolSelector provides tool-specific selection functionality. +type ToolSelector struct { + selector MultiSelectSelector[string] +} + +// NewToolSelector constructs a ToolSelector with the given multi-select selector. +func NewToolSelector(selector MultiSelectSelector[string]) *ToolSelector { + return &ToolSelector{ + selector: selector, + } +} + +// NewDefaultToolSelector constructs a ToolSelector with the default HuhMultiSelectSelector. +func NewDefaultToolSelector(accessible bool) *ToolSelector { + return &ToolSelector{ + selector: NewHuhMultiSelectSelector[string](accessible), + } +} + +// ToolDetail represents the details of a tool. +type ToolDetail struct { + Name string // Name of the tool. + Description string // Human-readable description. +} + +// SelectTools prompts the user to select tools to install from the available ones. +// All tools start unselected. Returns the list of selected tool names (generic package codes). +func (s *ToolSelector) SelectTools(availableTools []string, + toolDetails map[string]ToolDetail, +) ([]string, error) { + if len(availableTools) == 0 { + return nil, errors.New("no tools available for selection") + } + + items := make([]MultiSelectItem[string], len(availableTools)) + for i, tool := range availableTools { + item := MultiSelectItem[string]{ + Value: tool, + Label: tool, + } + + if detail, exists := toolDetails[tool]; exists { + item.Description = detail.Description + } + + items[i] = item + } + + return s.selector.SelectMultiple("Select optional tools to install:", items) +} diff --git a/installer/cli/tool_selector_test.go b/installer/cli/tool_selector_test.go new file mode 100644 index 0000000..e928b35 --- /dev/null +++ b/installer/cli/tool_selector_test.go @@ -0,0 +1,161 @@ +package cli + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" +) + +// MockMultiSelectSelector is an inline mock for MultiSelectSelector[string]. +type MockMultiSelectSelector struct { + SelectMultipleFunc func(title string, items []MultiSelectItem[string]) ([]string, error) +} + +var _ MultiSelectSelector[string] = (*MockMultiSelectSelector)(nil) + +func (m *MockMultiSelectSelector) SelectMultiple(title string, items []MultiSelectItem[string]) ([]string, error) { + if m.SelectMultipleFunc != nil { + return m.SelectMultipleFunc(title, items) + } + return nil, nil +} + +func Test_ItemsCreatedCorrectlyFromToolDetails(t *testing.T) { + availableTools := []string{"tool1", "tool2", "tool3"} + toolDetails := map[string]ToolDetail{ + "tool1": {Name: "tool1", Description: "First tool"}, + "tool2": {Name: "tool2", Description: "Second tool"}, + "tool3": {Name: "tool3", Description: "Third tool"}, + } + + var capturedItems []MultiSelectItem[string] + mockSelector := &MockMultiSelectSelector{ + SelectMultipleFunc: func(title string, items []MultiSelectItem[string]) ([]string, error) { + capturedItems = items + return []string{}, nil + }, + } + + selector := NewToolSelector(mockSelector) + _, err := selector.SelectTools(availableTools, toolDetails) + + require.NoError(t, err) + require.Len(t, capturedItems, 3) + + require.Equal(t, "tool1", capturedItems[0].Value) + require.Equal(t, "tool1", capturedItems[0].Label) + require.Equal(t, "First tool", capturedItems[0].Description) + + require.Equal(t, "tool2", capturedItems[1].Value) + require.Equal(t, "tool2", capturedItems[1].Label) + require.Equal(t, "Second tool", capturedItems[1].Description) + + require.Equal(t, "tool3", capturedItems[2].Value) + require.Equal(t, "tool3", capturedItems[2].Label) + require.Equal(t, "Third tool", capturedItems[2].Description) +} + +func Test_EmptyAvailableToolsReturnsError(t *testing.T) { + mockSelector := &MockMultiSelectSelector{ + SelectMultipleFunc: func(title string, items []MultiSelectItem[string]) ([]string, error) { + return nil, nil + }, + } + + selector := NewToolSelector(mockSelector) + _, err := selector.SelectTools([]string{}, map[string]ToolDetail{}) + + require.Error(t, err) + require.Equal(t, "no tools available for selection", err.Error()) +} + +func Test_EmptySelectionReturnsEmptySlice(t *testing.T) { + availableTools := []string{"tool1", "tool2"} + toolDetails := map[string]ToolDetail{ + "tool1": {Name: "tool1", Description: "First tool"}, + "tool2": {Name: "tool2", Description: "Second tool"}, + } + + mockSelector := &MockMultiSelectSelector{ + SelectMultipleFunc: func(title string, items []MultiSelectItem[string]) ([]string, error) { + return []string{}, nil + }, + } + + selector := NewToolSelector(mockSelector) + result, err := selector.SelectTools(availableTools, toolDetails) + + require.NoError(t, err) + require.Empty(t, result) + require.Equal(t, []string{}, result) +} + +func Test_SelectorErrorReturnsError(t *testing.T) { + availableTools := []string{"tool1"} + toolDetails := map[string]ToolDetail{ + "tool1": {Name: "tool1", Description: "First tool"}, + } + + expectedErr := errors.New("user cancelled selection") + mockSelector := &MockMultiSelectSelector{ + SelectMultipleFunc: func(title string, items []MultiSelectItem[string]) ([]string, error) { + return nil, expectedErr + }, + } + + selector := NewToolSelector(mockSelector) + _, err := selector.SelectTools(availableTools, toolDetails) + + require.Error(t, err) + require.Equal(t, expectedErr, err) +} + +func Test_SelectionReturnsCorrectToolNames(t *testing.T) { + availableTools := []string{"tool1", "tool2", "tool3"} + toolDetails := map[string]ToolDetail{ + "tool1": {Name: "tool1", Description: "First tool"}, + "tool2": {Name: "tool2", Description: "Second tool"}, + "tool3": {Name: "tool3", Description: "Third tool"}, + } + + expectedSelection := []string{"tool1", "tool3"} + mockSelector := &MockMultiSelectSelector{ + SelectMultipleFunc: func(title string, items []MultiSelectItem[string]) ([]string, error) { + return expectedSelection, nil + }, + } + + selector := NewToolSelector(mockSelector) + result, err := selector.SelectTools(availableTools, toolDetails) + + require.NoError(t, err) + require.Equal(t, expectedSelection, result) +} + +func Test_ItemsWithoutDetailsHaveEmptyDescription(t *testing.T) { + availableTools := []string{"tool1", "tool2"} + toolDetails := map[string]ToolDetail{ + "tool1": {Name: "tool1", Description: "First tool"}, + } + + var capturedItems []MultiSelectItem[string] + mockSelector := &MockMultiSelectSelector{ + SelectMultipleFunc: func(title string, items []MultiSelectItem[string]) ([]string, error) { + capturedItems = items + return []string{}, nil + }, + } + + selector := NewToolSelector(mockSelector) + _, err := selector.SelectTools(availableTools, toolDetails) + + require.NoError(t, err) + require.Len(t, capturedItems, 2) + + // tool1 has description + require.Equal(t, "First tool", capturedItems[0].Description) + + // tool2 has no description + require.Equal(t, "", capturedItems[1].Description) +} diff --git a/installer/cmd/install.go b/installer/cmd/install.go index e0e56f2..2718cac 100644 --- a/installer/cmd/install.go +++ b/installer/cmd/install.go @@ -18,6 +18,7 @@ import ( "github.com/MrPointer/dotfiles/installer/lib/packageresolver" "github.com/MrPointer/dotfiles/installer/lib/pkgmanager" "github.com/MrPointer/dotfiles/installer/lib/shell" + "github.com/MrPointer/dotfiles/installer/lib/toolsinstaller" "github.com/MrPointer/dotfiles/installer/utils/logger" "github.com/MrPointer/dotfiles/installer/utils/privilege" "github.com/samber/mo" @@ -38,6 +39,7 @@ var ( gitBranch string verbose bool installPrerequisites bool + installTools bool ) // global variables for the command execution context. @@ -125,6 +127,10 @@ var installCmd = &cobra.Command{ os.Exit(1) } + if err := installOptionalTools(installLogger); err != nil { + installLogger.Warning("Failed to install optional tools: %v", err) + } + installLogger.Success("Installation completed successfully") }, } @@ -222,7 +228,7 @@ func handlePrerequisiteInstallation(sysInfo compatibility.SystemInfo, log logger log.StartProgress("Installing missing prerequisites automatically") } else { // In interactive mode, let user select which prerequisites to install - prerequisiteSelector := cli.NewDefaultPrerequisiteSelector() + prerequisiteSelector := cli.NewDefaultPrerequisiteSelector(plainFlag) // Convert compatibility.PrerequisiteDetail to cli.PrerequisiteDetail cliDetails := make(map[string]cli.PrerequisiteDetail) @@ -526,7 +532,7 @@ func setupGpgKeys(log logger.Logger) error { log.FinishInteractiveProgress("GPG key pair created successfully") } else { log.StartInteractiveProgress("Selecting GPG key from existing keys") - gpgSelector := cli.NewDefaultGpgKeySelector() + gpgSelector := cli.NewDefaultGpgKeySelector(plainFlag) selectedKey, err := gpgSelector.SelectKey(existingKeys) if err != nil { log.FailInteractiveProgress("Failed to select GPG key", err) @@ -649,6 +655,113 @@ func initDotfilesManagerData(dm dotfilesmanager.DotfilesManager) error { return dm.Initialize(dotfiles_data) } +// installOptionalTools installs optional CLI tools after dotfiles setup completes. +// This function is non-fatal - it logs warnings for failures but never aborts the overall install. +func installOptionalTools(log logger.Logger) error { + // Load tools configuration with a fresh viper instance + log.StartProgress("Loading optional tools configuration") + toolsCfg, err := toolsinstaller.LoadToolsConfig(viper.New(), toolsConfigFile) + if err != nil { + log.FailProgress("Failed to load tools configuration", err) + return err + } + + if len(toolsCfg.Tools) == 0 { + log.FinishProgress("No optional tools configured") + return nil + } + + // Create package manager and resolver + pm := createPackageManagerForSystem(&globalSysInfo) + if pm == nil { + log.FailProgress("No package manager available for optional tools", nil) + return fmt.Errorf("no package manager available") + } + + resolver := createPackageResolverForSystem(pm, &globalSysInfo) + if resolver == nil { + log.FailProgress("Cannot resolve package information for optional tools", nil) + return fmt.Errorf("cannot resolve package information") + } + + // Pre-filter tools: only keep tools that can be resolved + var availableTools []string + toolDetails := make(map[string]cli.ToolDetail) + + for _, tool := range toolsCfg.Tools { + _, err := resolver.Resolve(tool.Name, "") + if err == nil { + availableTools = append(availableTools, tool.Name) + toolDetails[tool.Name] = cli.ToolDetail{ + Name: tool.Name, + Description: tool.Description, + } + } else { + log.Debug("Tool %s not available: %v", tool.Name, err) + } + } + log.FinishProgress(fmt.Sprintf("Found %d available optional tools", len(availableTools))) + + if len(availableTools) == 0 { + return nil + } + + var toolsToInstall []string + + // Determine which tools to install based on mode + if installTools { + // Auto-install all available tools + toolsToInstall = availableTools + } else if !IsNonInteractive() { + // Interactive mode: show selector + log.StartInteractiveProgress("Selecting optional tools to install") + + selector := cli.NewDefaultToolSelector(plainFlag) + selected, err := selector.SelectTools(availableTools, toolDetails) + if err != nil { + log.FinishInteractiveProgress("Tool selection cancelled or failed") + return nil + } + + log.FinishInteractiveProgress("Tool selection completed") + + if len(selected) == 0 { + return nil + } + + toolsToInstall = selected + } else { + // Non-interactive without --install-tools flag: skip entirely + log.Info("Skipping optional tools in non-interactive mode (use --install-tools to enable)") + return nil + } + + // Install selected tools + log.StartPersistentProgress(fmt.Sprintf("Installing %d optional tools", len(toolsToInstall))) + + installer := toolsinstaller.NewToolsInstaller(resolver, pm, log) + results := installer.InstallTools(toolsToInstall) + + // Report results + successCount := 0 + failureCount := 0 + for _, result := range results { + if result.Success { + successCount++ + } else { + failureCount++ + } + } + + if failureCount > 0 { + log.FinishPersistentProgress(fmt.Sprintf("Optional tools installed (%d succeeded, %d failed)", successCount, failureCount)) + } else { + log.FinishPersistentProgress(fmt.Sprintf("All %d optional tools installed successfully", successCount)) + } + + return nil +} + //nolint:gochecknoinits // Cobra requires an init function to set up the command structure. func init() { rootCmd.AddCommand(installCmd) @@ -672,6 +785,8 @@ func init() { "Useful for testing changes in feature branches or when running in CI/CD pipelines.") installCmd.Flags().BoolVar(&installPrerequisites, "install-prerequisites", false, "Automatically install missing prerequisites") + installCmd.Flags().BoolVar(&installTools, "install-tools", false, + "Automatically install all optional tools") viper.BindPFlag("work-env", installCmd.Flags().Lookup("work-env")) viper.BindPFlag("work-name", installCmd.Flags().Lookup("work-name")) @@ -683,4 +798,5 @@ func init() { viper.BindPFlag("git-branch", installCmd.Flags().Lookup("git-branch")) viper.BindPFlag("install-prerequisites", installCmd.Flags().Lookup("install-prerequisites")) + viper.BindPFlag("install-tools", installCmd.Flags().Lookup("install-tools")) } diff --git a/installer/cmd/root.go b/installer/cmd/root.go index 6087bd8..a174fb9 100644 --- a/installer/cmd/root.go +++ b/installer/cmd/root.go @@ -20,6 +20,7 @@ import ( var ( cfgFile string compatibilityConfigFile string + toolsConfigFile string globalCompatibilityConfig *compatibility.CompatibilityConfig globalVerbosity logger.VerbosityLevel verboseCount int @@ -120,6 +121,9 @@ func init() { rootCmd.PersistentFlags().StringVar(&compatibilityConfigFile, "compat-config", "", "compatibility configuration file (uses embedded config by default)") + rootCmd.PersistentFlags().StringVar(&toolsConfigFile, "tools-config", "", + "tools configuration file (uses embedded config by default)") + // Verbosity flags: supports multiple levels // - No flags: Normal verbosity with progress indicators (default) // - -v or --verbose: Verbose level (adds Debug messages, progress disabled by default) diff --git a/installer/internal/config/embed.go b/installer/internal/config/embed.go index 1308d85..45acf86 100644 --- a/installer/internal/config/embed.go +++ b/installer/internal/config/embed.go @@ -5,7 +5,7 @@ import ( "fmt" ) -//go:embed compatibility.yaml packagemap.yaml +//go:embed compatibility.yaml packagemap.yaml tools.yaml var configFS embed.FS // GetRawEmbeddedCompatibilityConfig returns the raw content of the embedded compatibility configuration file. @@ -27,3 +27,12 @@ func GetRawEmbeddedPackageMapConfig() ([]byte, error) { } return data, nil } + +// GetRawEmbeddedToolsConfig returns the raw content of the embedded tools configuration file. +func GetRawEmbeddedToolsConfig() ([]byte, error) { + data, err := configFS.ReadFile("tools.yaml") + if err != nil { + return nil, fmt.Errorf("failed to read embedded tools config: %w", err) + } + return data, nil +} diff --git a/installer/internal/config/embed_test.go b/installer/internal/config/embed_test.go index ec84638..4f59327 100644 --- a/installer/internal/config/embed_test.go +++ b/installer/internal/config/embed_test.go @@ -29,3 +29,15 @@ func TestEmbeddedPackageMapConfigCanBeLoaded(t *testing.T) { t.Fatal("Expected package map config to be non-nil, got nil") } } + +func TestEmbeddedToolsConfigCanBeLoaded(t *testing.T) { + // Test basic loading functionality + config, err := config.GetRawEmbeddedToolsConfig() + if err != nil { + t.Fatalf("Expected no error when loading embedded tools config, got: %v", err) + } + + if config == nil { + t.Fatal("Expected tools config to be non-nil, got nil") + } +} diff --git a/installer/internal/config/packagemap.yaml b/installer/internal/config/packagemap.yaml index 7d5e0d1..340f509 100644 --- a/installer/internal/config/packagemap.yaml +++ b/installer/internal/config/packagemap.yaml @@ -57,3 +57,40 @@ packages: name: procps dnf: name: procps-ng + fzf: + apt: + name: fzf + brew: + name: fzf + dnf: + name: fzf + bat: + apt: + name: bat + brew: + name: bat + dnf: + name: bat + ripgrep: + apt: + name: ripgrep + brew: + name: ripgrep + dnf: + name: ripgrep + fd: + apt: + name: fd-find + brew: + name: fd + dnf: + name: fd-find + sheldon: + brew: + name: sheldon + eza: + brew: + name: eza + difftastic: + brew: + name: difftastic diff --git a/installer/internal/config/tools.yaml b/installer/internal/config/tools.yaml new file mode 100644 index 0000000..1a30924 --- /dev/null +++ b/installer/internal/config/tools.yaml @@ -0,0 +1,15 @@ +tools: + - name: fzf + description: "Fuzzy finder for the command line" + - name: bat + description: "Cat clone with syntax highlighting" + - name: sheldon + description: "Shell plugin manager" + - name: eza + description: "Modern replacement for ls" + - name: ripgrep + description: "Fast recursive search tool" + - name: fd + description: "Fast alternative to find" + - name: difftastic + description: "Structural diff tool" diff --git a/installer/lib/toolsinstaller/installer.go b/installer/lib/toolsinstaller/installer.go new file mode 100644 index 0000000..feb07e1 --- /dev/null +++ b/installer/lib/toolsinstaller/installer.go @@ -0,0 +1,75 @@ +package toolsinstaller + +import ( + "fmt" + + "github.com/MrPointer/dotfiles/installer/lib/packageresolver" + "github.com/MrPointer/dotfiles/installer/lib/pkgmanager" + "github.com/MrPointer/dotfiles/installer/utils/logger" +) + +// ToolsInstaller manages the installation of optional tools. +type ToolsInstaller struct { + resolver packageresolver.PackageManagerResolver + packageManager pkgmanager.PackageManager + logger logger.Logger +} + +// InstallResult represents the outcome of installing a single tool. +type InstallResult struct { + Name string + Success bool + Error error +} + +var _ interface{} = (*ToolsInstaller)(nil) + +// NewToolsInstaller creates a new ToolsInstaller with the provided dependencies. +func NewToolsInstaller( + resolver packageresolver.PackageManagerResolver, + pm pkgmanager.PackageManager, + log logger.Logger, +) *ToolsInstaller { + return &ToolsInstaller{ + resolver: resolver, + packageManager: pm, + logger: log, + } +} + +// InstallTools installs the provided list of tools and returns results for each. +// If any tool installation fails, it logs a warning and continues with the next tool. +// It never aborts on individual failures. +func (ti *ToolsInstaller) InstallTools(tools []string) []InstallResult { + results := make([]InstallResult, len(tools)) + + for i, tool := range tools { + result := InstallResult{Name: tool} + + ti.logger.UpdateProgress(fmt.Sprintf("Installing %s (%d/%d)", tool, i+1, len(tools))) + + // Resolve the tool name using the resolver + resolvedInfo, err := ti.resolver.Resolve(tool, "") + if err != nil { + result.Success = false + result.Error = fmt.Errorf("failed to resolve tool '%s': %w", tool, err) + results[i] = result + continue + } + + // Install the tool using the package manager + err = ti.packageManager.InstallPackage(resolvedInfo) + if err != nil { + result.Success = false + result.Error = fmt.Errorf("failed to install tool '%s': %w", tool, err) + results[i] = result + continue + } + + ti.logger.LogAccomplishment(fmt.Sprintf("Installed %s", tool)) + result.Success = true + results[i] = result + } + + return results +} diff --git a/installer/lib/toolsinstaller/installer_test.go b/installer/lib/toolsinstaller/installer_test.go new file mode 100644 index 0000000..c64bf06 --- /dev/null +++ b/installer/lib/toolsinstaller/installer_test.go @@ -0,0 +1,143 @@ +package toolsinstaller + +import ( + "errors" + "testing" + + "github.com/MrPointer/dotfiles/installer/lib/packageresolver" + "github.com/MrPointer/dotfiles/installer/lib/pkgmanager" + "github.com/MrPointer/dotfiles/installer/utils/logger" + "github.com/stretchr/testify/require" +) + +func Test_InstallToolsSucceedsForAllTools(t *testing.T) { + resolver := &packageresolver.MoqPackageManagerResolver{ + ResolveFunc: func(genericPackageCode, versionConstraintString string) (pkgmanager.RequestedPackageInfo, error) { + return pkgmanager.RequestedPackageInfo{Name: genericPackageCode + "-resolved"}, nil + }, + } + + pm := &pkgmanager.MoqPackageManager{ + InstallPackageFunc: func(requestedPackageInfo pkgmanager.RequestedPackageInfo) error { + return nil + }, + } + + installer := NewToolsInstaller(resolver, pm, &logger.NoopLogger{}) + + results := installer.InstallTools([]string{"git", "fzf"}) + + require.Len(t, results, 2) + require.True(t, results[0].Success) + require.Equal(t, "git", results[0].Name) + require.NoError(t, results[0].Error) + require.True(t, results[1].Success) + require.Equal(t, "fzf", results[1].Name) + require.NoError(t, results[1].Error) +} + +func Test_InstallToolsContinuesWhenResolverFails(t *testing.T) { + resolver := &packageresolver.MoqPackageManagerResolver{ + ResolveFunc: func(genericPackageCode, versionConstraintString string) (pkgmanager.RequestedPackageInfo, error) { + if genericPackageCode == "bad-tool" { + return pkgmanager.RequestedPackageInfo{}, errors.New("unknown tool") + } + + return pkgmanager.RequestedPackageInfo{Name: genericPackageCode + "-resolved"}, nil + }, + } + + pm := &pkgmanager.MoqPackageManager{ + InstallPackageFunc: func(requestedPackageInfo pkgmanager.RequestedPackageInfo) error { + return nil + }, + } + + installer := NewToolsInstaller(resolver, pm, &logger.NoopLogger{}) + + results := installer.InstallTools([]string{"git", "bad-tool", "fzf"}) + + require.Len(t, results, 3) + require.True(t, results[0].Success) + require.False(t, results[1].Success) + require.Contains(t, results[1].Error.Error(), "unknown tool") + require.True(t, results[2].Success) +} + +func Test_InstallToolsContinuesWhenInstallFails(t *testing.T) { + callCount := 0 + resolver := &packageresolver.MoqPackageManagerResolver{ + ResolveFunc: func(genericPackageCode, versionConstraintString string) (pkgmanager.RequestedPackageInfo, error) { + return pkgmanager.RequestedPackageInfo{Name: genericPackageCode + "-resolved"}, nil + }, + } + + pm := &pkgmanager.MoqPackageManager{ + InstallPackageFunc: func(requestedPackageInfo pkgmanager.RequestedPackageInfo) error { + callCount++ + if callCount == 2 { + return errors.New("installation failed") + } + + return nil + }, + } + + installer := NewToolsInstaller(resolver, pm, &logger.NoopLogger{}) + + results := installer.InstallTools([]string{"git", "bad-pkg", "fzf"}) + + require.Len(t, results, 3) + require.True(t, results[0].Success) + require.False(t, results[1].Success) + require.Contains(t, results[1].Error.Error(), "installation failed") + require.True(t, results[2].Success) +} + +func Test_InstallToolsReturnsEmptyResultsForEmptyList(t *testing.T) { + resolver := &packageresolver.MoqPackageManagerResolver{} + pm := &pkgmanager.MoqPackageManager{} + installer := NewToolsInstaller(resolver, pm, &logger.NoopLogger{}) + + results := installer.InstallTools([]string{}) + + require.Len(t, results, 0) +} + +func Test_InstallToolsTracksSuccessAndFailureAccurately(t *testing.T) { + toolResults := map[string]bool{ + "git": true, + "fzf": false, + "bat": true, + "neovim": false, + "ripgrep": true, + } + + resolver := &packageresolver.MoqPackageManagerResolver{ + ResolveFunc: func(genericPackageCode, versionConstraintString string) (pkgmanager.RequestedPackageInfo, error) { + if !toolResults[genericPackageCode] { + return pkgmanager.RequestedPackageInfo{}, errors.New("failed to resolve") + } + + return pkgmanager.RequestedPackageInfo{Name: genericPackageCode + "-resolved"}, nil + }, + } + + pm := &pkgmanager.MoqPackageManager{ + InstallPackageFunc: func(requestedPackageInfo pkgmanager.RequestedPackageInfo) error { + return nil + }, + } + + installer := NewToolsInstaller(resolver, pm, &logger.NoopLogger{}) + + tools := []string{"git", "fzf", "bat", "neovim", "ripgrep"} + results := installer.InstallTools(tools) + + require.Len(t, results, 5) + require.True(t, results[0].Success) + require.False(t, results[1].Success) + require.True(t, results[2].Success) + require.False(t, results[3].Success) + require.True(t, results[4].Success) +} diff --git a/installer/lib/toolsinstaller/loader.go b/installer/lib/toolsinstaller/loader.go new file mode 100644 index 0000000..525c29f --- /dev/null +++ b/installer/lib/toolsinstaller/loader.go @@ -0,0 +1,39 @@ +package toolsinstaller + +import ( + "bytes" + "fmt" + + "github.com/MrPointer/dotfiles/installer/internal/config" + "github.com/spf13/viper" +) + +// LoadToolsConfig loads the tools configuration. +// It first tries to load from the specified toolsConfigFile. If toolsConfigFile is empty, +// it loads from the embedded default configuration. +// Callers must pass viper.New() to avoid state pollution from other config loaders. +func LoadToolsConfig(v *viper.Viper, toolsConfigFile string) (*ToolsConfig, error) { + if toolsConfigFile != "" { + v.SetConfigFile(toolsConfigFile) + if err := v.ReadInConfig(); err != nil { + return nil, fmt.Errorf("error reading tools config file '%s': %w", toolsConfigFile, err) + } + } else { + v.SetConfigType("yaml") + + embeddedData, err := config.GetRawEmbeddedToolsConfig() + if err != nil { + return nil, fmt.Errorf("error loading embedded tools config: %w", err) + } + if err := v.ReadConfig(bytes.NewBuffer(embeddedData)); err != nil { + return nil, fmt.Errorf("error reading embedded tools config: %w", err) + } + } + + var toolsCfg ToolsConfig + if err := v.Unmarshal(&toolsCfg); err != nil { + return nil, fmt.Errorf("error parsing tools configuration: %w", err) + } + + return &toolsCfg, nil +} diff --git a/installer/lib/toolsinstaller/loader_test.go b/installer/lib/toolsinstaller/loader_test.go new file mode 100644 index 0000000..c010951 --- /dev/null +++ b/installer/lib/toolsinstaller/loader_test.go @@ -0,0 +1,65 @@ +package toolsinstaller + +import ( + "os" + "path/filepath" + "testing" + + "github.com/spf13/viper" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLoadToolsConfig_Embedded(t *testing.T) { + cfg, err := LoadToolsConfig(viper.New(), "") + require.NoError(t, err) + require.NotNil(t, cfg) + require.NotEmpty(t, cfg.Tools) + + for _, tool := range cfg.Tools { + assert.NotEmpty(t, tool.Name, "tool name should not be empty") + assert.NotEmpty(t, tool.Description, "tool description should not be empty") + } +} + +func TestLoadToolsConfig_CustomFile(t *testing.T) { + content := `tools: + - name: mytool + description: "A custom tool" + - name: anothertool + description: "Another custom tool" +` + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "custom-tools.yaml") + err := os.WriteFile(tmpFile, []byte(content), 0o644) + require.NoError(t, err) + + cfg, err := LoadToolsConfig(viper.New(), tmpFile) + require.NoError(t, err) + require.NotNil(t, cfg) + require.Len(t, cfg.Tools, 2) + assert.Equal(t, "mytool", cfg.Tools[0].Name) + assert.Equal(t, "A custom tool", cfg.Tools[0].Description) + assert.Equal(t, "anothertool", cfg.Tools[1].Name) + assert.Equal(t, "Another custom tool", cfg.Tools[1].Description) +} + +func TestLoadToolsConfig_NonExistentFile(t *testing.T) { + cfg, err := LoadToolsConfig(viper.New(), "/nonexistent/path/tools.yaml") + require.Error(t, err) + assert.Nil(t, cfg) +} + +func TestLoadToolsConfig_EmptyConfig(t *testing.T) { + content := `tools: [] +` + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "empty-tools.yaml") + err := os.WriteFile(tmpFile, []byte(content), 0o644) + require.NoError(t, err) + + cfg, err := LoadToolsConfig(viper.New(), tmpFile) + require.NoError(t, err) + require.NotNil(t, cfg) + assert.Empty(t, cfg.Tools) +} diff --git a/installer/lib/toolsinstaller/types.go b/installer/lib/toolsinstaller/types.go new file mode 100644 index 0000000..f000978 --- /dev/null +++ b/installer/lib/toolsinstaller/types.go @@ -0,0 +1,12 @@ +package toolsinstaller + +// ToolDefinition represents a single tool entry from tools.yaml. +type ToolDefinition struct { + Name string `mapstructure:"name"` + Description string `mapstructure:"description"` +} + +// ToolsConfig is the top-level structure for tools.yaml. +type ToolsConfig struct { + Tools []ToolDefinition `mapstructure:"tools"` +} diff --git a/installer/test-interactive-gpg.exp b/installer/test-interactive-gpg.exp index b750f52..4688ce5 100755 --- a/installer/test-interactive-gpg.exp +++ b/installer/test-interactive-gpg.exp @@ -64,7 +64,10 @@ if {$verbosity ne ""} { spawn $installer_path {*}$cmd_args -# Main interaction loop - Only handle GPG-specific prompts +# State counter for tool multi-select (each prompt gets the next action) +set tool_select_step 0 + +# Main interaction loop expect { # GPG email prompts (various possible formats) -re "(?i).*(email|e-?mail address)" { @@ -136,6 +139,32 @@ expect { exp_continue } + # Optional tools multi-select (accessible mode: number-based prompts) + # Each "Input a number" prompt is handled one at a time via the main loop. + # Select items 1, 2, 3 then confirm with 0. + -re "Input a number between 0 and" { + incr tool_select_step + switch $tool_select_step { + 1 { + puts "🔧 Tool selection: selecting item 1..." + send "1\r" + } + 2 { + puts "🔧 Tool selection: selecting item 2..." + send "2\r" + } + 3 { + puts "🔧 Tool selection: selecting item 3..." + send "3\r" + } + 4 { + puts "🔧 Tool selection: confirming with 0..." + send "0\r" + } + } + exp_continue + } + # Error patterns -re "(?i).*(error|fail|abort)" { puts "❌ Error detected in output"