From d2f786ce61a2707ada1a1d66189b69d2d651dab4 Mon Sep 17 00:00:00 2001 From: Snider Date: Tue, 17 Mar 2026 08:26:07 +0000 Subject: [PATCH] fix(dx): audit CLAUDE.md, fix cmd errors, add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CLAUDE.md: fix module count (174→41+3), KnownModules count (68→80), update error handling convention to reflect coreerr.E() migration - cmd/ansible: replace fmt.Errorf with coreerr.E(), os.Stat with coreio.Local.Exists()/IsDir(), os.Getwd with filepath.Abs() - Add 38 tests for untested critical paths: non-SSH module handlers (debug, fail, assert, set_fact, include_vars, meta), handleLookup, SetInventory, iterator functions, resolveExpr with registered vars/ facts/task vars/host vars/filters Coverage: 23.4% → 27.7% Co-Authored-By: Virgil --- CLAUDE.md | 6 +- cmd/ansible/ansible.go | 31 ++- executor_extra_test.go | 464 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 481 insertions(+), 20 deletions(-) create mode 100644 executor_extra_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 5882fde..b94a857 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,7 +4,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ## Project Overview -`core/go-ansible` is a pure Go Ansible playbook engine. It parses YAML playbooks, inventories, and roles, then executes tasks on remote hosts via SSH. 174 module implementations, Jinja2-compatible templating, privilege escalation (become), and event-driven callbacks. This is a library — there is no standalone binary. The CLI integration lives in `cmd/ansible/` and is compiled as part of the `core` CLI binary. +`core/go-ansible` is a pure Go Ansible playbook engine. It parses YAML playbooks, inventories, and roles, then executes tasks on remote hosts via SSH. 41 module handler implementations (plus 3 community modules), Jinja2-compatible templating, privilege escalation (become), and event-driven callbacks. This is a library — there is no standalone binary. The CLI integration lives in `cmd/ansible/` and is compiled as part of the `core` CLI binary. ## Build & Test @@ -29,7 +29,7 @@ Playbook YAML ──► Parser ──► []Play ──► Executor ──► Mod Inventory YAML ──► Parser ──► Inventory Callbacks (OnPlayStart, OnTaskEnd, ...) ``` -- **`types.go`** — Core structs (`Playbook`, `Play`, `Task`, `TaskResult`, `Inventory`, `Host`, `Facts`) and `KnownModules` registry (68 entries: both FQCN `ansible.builtin.*` and short forms). +- **`types.go`** — Core structs (`Playbook`, `Play`, `Task`, `TaskResult`, `Inventory`, `Host`, `Facts`) and `KnownModules` registry (80 entries: both FQCN `ansible.builtin.*` and short forms). - **`parser.go`** — YAML parsing for playbooks, inventories, tasks, and roles. Custom `Task.UnmarshalYAML` scans map keys against `KnownModules` to extract the module name and args (since Ansible embeds the module name as a YAML key, not a fixed field). Free-form syntax (`shell: echo hello`) is stored as `Args["_raw_params"]`. Iterator variants (`ParsePlaybookIter`, `ParseTasksIter`, etc.) return `iter.Seq` values. - **`executor.go`** — Orchestration engine: host resolution from inventory, play execution order (gather facts → pre_tasks → roles → tasks → post_tasks → notified handlers), `when:` condition evaluation, `{{ }}` Jinja2-style templating with filter support, loop execution, block/rescue/always, handler notification. - **`modules.go`** — 41 module handler implementations dispatched via a `switch` on the normalised module name. Each handler extracts args via `getStringArg`/`getBoolArg`, constructs shell commands, runs them via SSH, and returns a `TaskResult`. @@ -63,6 +63,6 @@ If adding new YAML keys to `Task`, update the `knownKeys` map in `Task.Unmarshal - **UK English** in comments and documentation (colour, organisation, centre) - Test naming: `_Good` (happy path), `_Bad` (expected errors), `_Ugly` (edge cases/panics) -- Use `log.E(scope, message, err)` from `go-log` for errors in SSH/parser code; `fmt.Errorf` with `%w` in executor code +- Use `coreerr.E(scope, message, err)` from `go-log` for all errors in production code (never `fmt.Errorf`) - Tests use `testify/assert` (soft) and `testify/require` (hard) - Licence: EUPL-1.2 diff --git a/cmd/ansible/ansible.go b/cmd/ansible/ansible.go index ef501bf..c561329 100644 --- a/cmd/ansible/ansible.go +++ b/cmd/ansible/ansible.go @@ -3,13 +3,14 @@ package anscmd import ( "context" "fmt" - "os" "path/filepath" "strings" "time" ansible "forge.lthn.ai/core/go-ansible" "forge.lthn.ai/core/cli/pkg/cli" + coreio "forge.lthn.ai/core/go-io" + coreerr "forge.lthn.ai/core/go-log" ) var ( @@ -91,12 +92,11 @@ func runAnsible(cmd *cli.Command, args []string) error { // Resolve playbook path if !filepath.IsAbs(playbookPath) { - cwd, _ := os.Getwd() - playbookPath = filepath.Join(cwd, playbookPath) + playbookPath, _ = filepath.Abs(playbookPath) } - if _, err := os.Stat(playbookPath); os.IsNotExist(err) { - return fmt.Errorf("playbook not found: %s", playbookPath) + if !coreio.Local.Exists(playbookPath) { + return coreerr.E("runAnsible", fmt.Sprintf("playbook not found: %s", playbookPath), nil) } // Create executor @@ -128,21 +128,18 @@ func runAnsible(cmd *cli.Command, args []string) error { if ansibleInventory != "" { invPath := ansibleInventory if !filepath.IsAbs(invPath) { - cwd, _ := os.Getwd() - invPath = filepath.Join(cwd, invPath) + invPath, _ = filepath.Abs(invPath) } - // Check if it's a directory - info, err := os.Stat(invPath) - if err != nil { - return fmt.Errorf("inventory not found: %s", invPath) + if !coreio.Local.Exists(invPath) { + return coreerr.E("runAnsible", fmt.Sprintf("inventory not found: %s", invPath), nil) } - if info.IsDir() { + if coreio.Local.IsDir(invPath) { // Look for inventory.yml or hosts.yml for _, name := range []string{"inventory.yml", "hosts.yml", "inventory.yaml", "hosts.yaml"} { p := filepath.Join(invPath, name) - if _, err := os.Stat(p); err == nil { + if coreio.Local.Exists(p) { invPath = p break } @@ -150,7 +147,7 @@ func runAnsible(cmd *cli.Command, args []string) error { } if err := executor.SetInventory(invPath); err != nil { - return fmt.Errorf("load inventory: %w", err) + return coreerr.E("runAnsible", "load inventory", err) } } @@ -217,7 +214,7 @@ func runAnsible(cmd *cli.Command, args []string) error { fmt.Printf("%s Running playbook: %s\n", cli.BoldStyle.Render("▶"), playbookPath) if err := executor.Run(ctx, playbookPath); err != nil { - return fmt.Errorf("playbook failed: %w", err) + return coreerr.E("runAnsible", "playbook failed", err) } fmt.Printf("\n%s Playbook completed in %s\n", @@ -243,7 +240,7 @@ func runAnsibleTest(cmd *cli.Command, args []string) error { client, err := ansible.NewSSHClient(cfg) if err != nil { - return fmt.Errorf("create client: %w", err) + return coreerr.E("runAnsibleTest", "create client", err) } defer func() { _ = client.Close() }() @@ -253,7 +250,7 @@ func runAnsibleTest(cmd *cli.Command, args []string) error { // Test connection start := time.Now() if err := client.Connect(ctx); err != nil { - return fmt.Errorf("connect failed: %w", err) + return coreerr.E("runAnsibleTest", "connect failed", err) } connectTime := time.Since(start) diff --git a/executor_extra_test.go b/executor_extra_test.go new file mode 100644 index 0000000..c3617d3 --- /dev/null +++ b/executor_extra_test.go @@ -0,0 +1,464 @@ +package ansible + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ============================================================ +// Tests for non-SSH module handlers (0% coverage) +// ============================================================ + +// --- moduleDebug --- + +func TestModuleDebug_Good_Message(t *testing.T) { + e := NewExecutor("/tmp") + result, err := e.moduleDebug(map[string]any{"msg": "Hello world"}) + + require.NoError(t, err) + assert.False(t, result.Changed) + assert.Equal(t, "Hello world", result.Msg) +} + +func TestModuleDebug_Good_Var(t *testing.T) { + e := NewExecutor("/tmp") + e.vars["my_version"] = "1.2.3" + + result, err := e.moduleDebug(map[string]any{"var": "my_version"}) + + require.NoError(t, err) + assert.Contains(t, result.Msg, "1.2.3") +} + +func TestModuleDebug_Good_EmptyArgs(t *testing.T) { + e := NewExecutor("/tmp") + result, err := e.moduleDebug(map[string]any{}) + + require.NoError(t, err) + assert.Equal(t, "", result.Msg) +} + +// --- moduleFail --- + +func TestModuleFail_Good_DefaultMessage(t *testing.T) { + e := NewExecutor("/tmp") + result, err := e.moduleFail(map[string]any{}) + + require.NoError(t, err) + assert.True(t, result.Failed) + assert.Equal(t, "Failed as requested", result.Msg) +} + +func TestModuleFail_Good_CustomMessage(t *testing.T) { + e := NewExecutor("/tmp") + result, err := e.moduleFail(map[string]any{"msg": "deployment blocked"}) + + require.NoError(t, err) + assert.True(t, result.Failed) + assert.Equal(t, "deployment blocked", result.Msg) +} + +// --- moduleAssert --- + +func TestModuleAssert_Good_PassingAssertion(t *testing.T) { + e := NewExecutor("/tmp") + e.vars["enabled"] = true + + result, err := e.moduleAssert(map[string]any{"that": "enabled"}, "host1") + + require.NoError(t, err) + assert.False(t, result.Failed) + assert.Equal(t, "All assertions passed", result.Msg) +} + +func TestModuleAssert_Bad_FailingAssertion(t *testing.T) { + e := NewExecutor("/tmp") + e.vars["enabled"] = false + + result, err := e.moduleAssert(map[string]any{"that": "enabled"}, "host1") + + require.NoError(t, err) + assert.True(t, result.Failed) + assert.Contains(t, result.Msg, "Assertion failed") +} + +func TestModuleAssert_Bad_MissingThat(t *testing.T) { + e := NewExecutor("/tmp") + + _, err := e.moduleAssert(map[string]any{}, "host1") + assert.Error(t, err) +} + +func TestModuleAssert_Good_CustomFailMsg(t *testing.T) { + e := NewExecutor("/tmp") + e.vars["ready"] = false + + result, err := e.moduleAssert(map[string]any{ + "that": "ready", + "fail_msg": "Service not ready", + }, "host1") + + require.NoError(t, err) + assert.True(t, result.Failed) + assert.Equal(t, "Service not ready", result.Msg) +} + +func TestModuleAssert_Good_MultipleConditions(t *testing.T) { + e := NewExecutor("/tmp") + e.vars["enabled"] = true + e.vars["count"] = 5 + + result, err := e.moduleAssert(map[string]any{ + "that": []any{"enabled", "count"}, + }, "host1") + + require.NoError(t, err) + assert.False(t, result.Failed) +} + +// --- moduleSetFact --- + +func TestModuleSetFact_Good(t *testing.T) { + e := NewExecutor("/tmp") + + result, err := e.moduleSetFact(map[string]any{ + "app_version": "2.0.0", + "deploy_env": "production", + }) + + require.NoError(t, err) + assert.True(t, result.Changed) + assert.Equal(t, "2.0.0", e.vars["app_version"]) + assert.Equal(t, "production", e.vars["deploy_env"]) +} + +func TestModuleSetFact_Good_SkipsCacheable(t *testing.T) { + e := NewExecutor("/tmp") + + e.moduleSetFact(map[string]any{ + "my_fact": "value", + "cacheable": true, + }) + + assert.Equal(t, "value", e.vars["my_fact"]) + _, hasCacheable := e.vars["cacheable"] + assert.False(t, hasCacheable) +} + +// --- moduleIncludeVars --- + +func TestModuleIncludeVars_Good_WithFile(t *testing.T) { + e := NewExecutor("/tmp") + result, err := e.moduleIncludeVars(map[string]any{"file": "vars/main.yml"}) + + require.NoError(t, err) + assert.Contains(t, result.Msg, "vars/main.yml") +} + +func TestModuleIncludeVars_Good_WithRawParams(t *testing.T) { + e := NewExecutor("/tmp") + result, err := e.moduleIncludeVars(map[string]any{"_raw_params": "defaults.yml"}) + + require.NoError(t, err) + assert.Contains(t, result.Msg, "defaults.yml") +} + +func TestModuleIncludeVars_Good_Empty(t *testing.T) { + e := NewExecutor("/tmp") + result, err := e.moduleIncludeVars(map[string]any{}) + + require.NoError(t, err) + assert.False(t, result.Changed) +} + +// --- moduleMeta --- + +func TestModuleMeta_Good(t *testing.T) { + e := NewExecutor("/tmp") + result, err := e.moduleMeta(map[string]any{"_raw_params": "flush_handlers"}) + + require.NoError(t, err) + assert.False(t, result.Changed) +} + +// ============================================================ +// Tests for handleLookup (0% coverage) +// ============================================================ + +func TestHandleLookup_Good_EnvVar(t *testing.T) { + e := NewExecutor("/tmp") + os.Setenv("TEST_ANSIBLE_LOOKUP", "found_it") + defer os.Unsetenv("TEST_ANSIBLE_LOOKUP") + + result := e.handleLookup("lookup('env', 'TEST_ANSIBLE_LOOKUP')") + assert.Equal(t, "found_it", result) +} + +func TestHandleLookup_Good_EnvVarMissing(t *testing.T) { + e := NewExecutor("/tmp") + result := e.handleLookup("lookup('env', 'NONEXISTENT_VAR_12345')") + assert.Equal(t, "", result) +} + +func TestHandleLookup_Bad_InvalidSyntax(t *testing.T) { + e := NewExecutor("/tmp") + result := e.handleLookup("lookup(invalid)") + assert.Equal(t, "", result) +} + +// ============================================================ +// Tests for SetInventory (0% coverage) +// ============================================================ + +func TestSetInventory_Good(t *testing.T) { + dir := t.TempDir() + invPath := filepath.Join(dir, "inventory.yml") + yaml := `all: + hosts: + web1: + ansible_host: 10.0.0.1 + web2: + ansible_host: 10.0.0.2 +` + require.NoError(t, os.WriteFile(invPath, []byte(yaml), 0644)) + + e := NewExecutor(dir) + err := e.SetInventory(invPath) + + require.NoError(t, err) + assert.NotNil(t, e.inventory) + assert.Len(t, e.inventory.All.Hosts, 2) +} + +func TestSetInventory_Bad_FileNotFound(t *testing.T) { + e := NewExecutor("/tmp") + err := e.SetInventory("/nonexistent/inventory.yml") + assert.Error(t, err) +} + +// ============================================================ +// Tests for iterator functions (0% coverage) +// ============================================================ + +func TestParsePlaybookIter_Good(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "playbook.yml") + yaml := `- name: First play + hosts: all + tasks: + - debug: + msg: hello + +- name: Second play + hosts: localhost + tasks: + - debug: + msg: world +` + require.NoError(t, os.WriteFile(path, []byte(yaml), 0644)) + + p := NewParser(dir) + iter, err := p.ParsePlaybookIter(path) + require.NoError(t, err) + + var plays []Play + for play := range iter { + plays = append(plays, play) + } + assert.Len(t, plays, 2) + assert.Equal(t, "First play", plays[0].Name) + assert.Equal(t, "Second play", plays[1].Name) +} + +func TestParsePlaybookIter_Bad_InvalidFile(t *testing.T) { + _, err := NewParser("/tmp").ParsePlaybookIter("/nonexistent.yml") + assert.Error(t, err) +} + +func TestParseTasksIter_Good(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "tasks.yml") + yaml := `- name: Task one + debug: + msg: first + +- name: Task two + debug: + msg: second +` + require.NoError(t, os.WriteFile(path, []byte(yaml), 0644)) + + p := NewParser(dir) + iter, err := p.ParseTasksIter(path) + require.NoError(t, err) + + var tasks []Task + for task := range iter { + tasks = append(tasks, task) + } + assert.Len(t, tasks, 2) + assert.Equal(t, "Task one", tasks[0].Name) +} + +func TestParseTasksIter_Bad_InvalidFile(t *testing.T) { + _, err := NewParser("/tmp").ParseTasksIter("/nonexistent.yml") + assert.Error(t, err) +} + +func TestGetHostsIter_Good(t *testing.T) { + inv := &Inventory{ + All: &InventoryGroup{ + Hosts: map[string]*Host{ + "web1": {}, + "web2": {}, + "db1": {}, + }, + }, + } + + var hosts []string + for host := range GetHostsIter(inv, "all") { + hosts = append(hosts, host) + } + assert.Len(t, hosts, 3) +} + +func TestAllHostsIter_Good(t *testing.T) { + group := &InventoryGroup{ + Hosts: map[string]*Host{ + "alpha": {}, + "beta": {}, + }, + Children: map[string]*InventoryGroup{ + "db": { + Hosts: map[string]*Host{ + "gamma": {}, + }, + }, + }, + } + + var hosts []string + for host := range AllHostsIter(group) { + hosts = append(hosts, host) + } + assert.Len(t, hosts, 3) + // AllHostsIter sorts keys + assert.Equal(t, "alpha", hosts[0]) + assert.Equal(t, "beta", hosts[1]) + assert.Equal(t, "gamma", hosts[2]) +} + +func TestAllHostsIter_Good_NilGroup(t *testing.T) { + var count int + for range AllHostsIter(nil) { + count++ + } + assert.Equal(t, 0, count) +} + +// ============================================================ +// Tests for resolveExpr with registered vars (additional coverage) +// ============================================================ + +func TestResolveExpr_Good_RegisteredVarFields(t *testing.T) { + e := NewExecutor("/tmp") + e.results["host1"] = map[string]*TaskResult{ + "cmd_result": { + Stdout: "output text", + Stderr: "error text", + RC: 0, + Changed: true, + Failed: false, + }, + } + + assert.Equal(t, "output text", e.resolveExpr("cmd_result.stdout", "host1", nil)) + assert.Equal(t, "error text", e.resolveExpr("cmd_result.stderr", "host1", nil)) + assert.Equal(t, "0", e.resolveExpr("cmd_result.rc", "host1", nil)) + assert.Equal(t, "true", e.resolveExpr("cmd_result.changed", "host1", nil)) + assert.Equal(t, "false", e.resolveExpr("cmd_result.failed", "host1", nil)) +} + +func TestResolveExpr_Good_TaskVars(t *testing.T) { + e := NewExecutor("/tmp") + task := &Task{ + Vars: map[string]any{"local_var": "local_value"}, + } + + result := e.resolveExpr("local_var", "host1", task) + assert.Equal(t, "local_value", result) +} + +func TestResolveExpr_Good_HostVars(t *testing.T) { + e := NewExecutor("/tmp") + e.SetInventoryDirect(&Inventory{ + All: &InventoryGroup{ + Hosts: map[string]*Host{ + "host1": {AnsibleHost: "10.0.0.1"}, + }, + }, + }) + + result := e.resolveExpr("ansible_host", "host1", nil) + assert.Equal(t, "10.0.0.1", result) +} + +func TestResolveExpr_Good_Facts(t *testing.T) { + e := NewExecutor("/tmp") + e.facts["host1"] = &Facts{ + Hostname: "web01", + FQDN: "web01.example.com", + Distribution: "ubuntu", + Version: "22.04", + Architecture: "x86_64", + Kernel: "5.15.0", + } + + assert.Equal(t, "web01", e.resolveExpr("ansible_hostname", "host1", nil)) + assert.Equal(t, "web01.example.com", e.resolveExpr("ansible_fqdn", "host1", nil)) + assert.Equal(t, "ubuntu", e.resolveExpr("ansible_distribution", "host1", nil)) + assert.Equal(t, "22.04", e.resolveExpr("ansible_distribution_version", "host1", nil)) + assert.Equal(t, "x86_64", e.resolveExpr("ansible_architecture", "host1", nil)) + assert.Equal(t, "5.15.0", e.resolveExpr("ansible_kernel", "host1", nil)) +} + +// --- applyFilter additional coverage --- + +func TestApplyFilter_Good_B64Decode(t *testing.T) { + e := NewExecutor("/tmp") + // b64decode is a no-op stub currently + assert.Equal(t, "hello", e.applyFilter("hello", "b64decode")) +} + +func TestApplyFilter_Good_UnknownFilter(t *testing.T) { + e := NewExecutor("/tmp") + assert.Equal(t, "value", e.applyFilter("value", "unknown_filter")) +} + +// --- evalCondition with default filter --- + +func TestEvalCondition_Good_DefaultFilter(t *testing.T) { + e := NewExecutor("/tmp") + assert.True(t, e.evalCondition("myvar | default('fallback')", "host1")) +} + +func TestEvalCondition_Good_UndefinedCheck(t *testing.T) { + e := NewExecutor("/tmp") + assert.True(t, e.evalCondition("missing_var is not defined", "host1")) + assert.True(t, e.evalCondition("missing_var is undefined", "host1")) +} + +// --- resolveExpr with filter pipe --- + +func TestResolveExpr_Good_WithFilter(t *testing.T) { + e := NewExecutor("/tmp") + e.vars["raw_value"] = " trimmed " + + result := e.resolveExpr("raw_value | trim", "host1", nil) + assert.Equal(t, "trimmed", result) +}