diff --git a/internal/config/config.go b/internal/config/config.go index d064df7..5327287 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -25,9 +25,13 @@ type PortForward struct { GuestPort int `toml:"guestport"` } -// AppliedRecipe records when a recipe was applied and which version. +// AppliedRecipe records when a recipe was applied, which version, and the +// hash of the script that ran. An entry saved before Hash existed decodes +// with an empty string, never equal to a current script's hash, so that +// recipe re-runs once and then carries a real hash from then on. type AppliedRecipe struct { Version string `toml:"version"` + Hash string `toml:"hash"` At time.Time `toml:"at"` } diff --git a/internal/core/apply.go b/internal/core/apply.go index 3a6665b..823bff6 100644 --- a/internal/core/apply.go +++ b/internal/core/apply.go @@ -146,7 +146,11 @@ func Apply(ctx context.Context, name string, opts ApplyOpts) error { if v.Applied == nil { v.Applied = make(map[string]config.AppliedRecipe, len(runTargets)) } - v.Applied[name] = config.AppliedRecipe{Version: m.Version, At: time.Now()} + hash, err := recipes.ScriptHash(name, v.OS) + if err != nil { + return err + } + v.Applied[name] = config.AppliedRecipe{Version: m.Version, Hash: hash, At: time.Now()} changed = true } if !changed { @@ -161,9 +165,14 @@ func Apply(ctx context.Context, name string, opts ApplyOpts) error { // // "manual" never runs implicitly. It runs only when explicit[name] is true, // meaning the caller named it directly via ApplyOpts.Only. "once" is -// skipped when v.Applied already has an entry for name at the same version -// the manifest declares now; a version bump makes it run again, the reason -// Version is recorded alongside At. "always" is never skipped. +// skipped when v.Applied already has an entry for name whose Hash matches +// the script that would run now; a changed script reruns even at the same +// manifest version, since a version bump is not the only way a recipe +// author fixes one. "always" is never skipped. +// +// An Applied entry saved before Hash existed decodes with an empty string. +// That never equals a real script hash, so an existing VM reruns its +// "once" recipes exactly once, then carries a real hash from then on. // // A target with no recipe.toml (ManifestFor's ok=false) is a v1 flat-file // recipe. It has no run-mode concept and always stays in the result, @@ -190,8 +199,14 @@ func filterByRunMode(v *config.VM, targets []string, explicit map[string]bool) ( continue } case "once": - if applied, done := v.Applied[name]; done && applied.Version == m.Version { - continue + if applied, done := v.Applied[name]; done { + hash, err := recipes.ScriptHash(name, v.OS) + if err != nil { + return nil, nil, err + } + if applied.Hash == hash { + continue + } } } kept = append(kept, name) diff --git a/internal/core/apply_test.go b/internal/core/apply_test.go index 1e9aac8..f5f9082 100644 --- a/internal/core/apply_test.go +++ b/internal/core/apply_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/novusedge/stoat/internal/config" "github.com/novusedge/stoat/internal/recipes" @@ -250,6 +251,228 @@ func writeRecipe(t *testing.T, dir, name, body string) { } } +// writeV2Recipe drops a v2 recipe (recipe.toml + install.sh) straight into +// root's recipes/ dir, mirroring recipes.writeV2Recipe for core's own tests, +// which need to set Run and Version, fields that helper doesn't take. +func writeV2Recipe(t *testing.T, rootDir, name, run, version, script string) { + t.Helper() + recipeDir := filepath.Join(rootDir, "recipes", name) + if err := os.MkdirAll(recipeDir, 0o755); err != nil { + t.Fatal(err) + } + toml := "name = \"" + name + "\"\n" + + "version = \"" + version + "\"\n" + + "script = \"install.sh\"\n" + + "run = \"" + run + "\"\n" + if err := os.WriteFile(filepath.Join(recipeDir, "recipe.toml"), []byte(toml), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(recipeDir, "install.sh"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } +} + +// TestFilterByRunModeSkipsOnceWithMatchingHash pins the base case: a "once" +// recipe whose stored hash still matches its current script stays skipped, +// even though nothing here checks Version at all. +func TestFilterByRunModeSkipsOnceWithMatchingHash(t *testing.T) { + dir := root(t) + writeV2Recipe(t, dir, "tool", "once", "1.0", "#!/bin/sh\necho one\n") + hash, err := recipes.ScriptHash("tool", "alpine") + if err != nil { + t.Fatal(err) + } + v := &config.VM{ + OS: "alpine", + Applied: map[string]config.AppliedRecipe{ + "tool": {Version: "1.0", Hash: hash}, + }, + } + + kept, _, err := filterByRunMode(v, []string{"tool"}, nil) + if err != nil { + t.Fatal(err) + } + if len(kept) != 0 { + t.Errorf("kept = %v, want none: the stored hash still matches", kept) + } +} + +// TestFilterByRunModeRerunsOnChangedScript is the behavior this feature +// adds: a fixed script at the SAME version must re-run, since a version +// bump is no longer the only signal filterByRunMode trusts. +func TestFilterByRunModeRerunsOnChangedScript(t *testing.T) { + dir := root(t) + writeV2Recipe(t, dir, "tool", "once", "1.0", "#!/bin/sh\necho fixed\n") + staleHash, err := recipes.ScriptHash("tool", "alpine") + if err != nil { + t.Fatal(err) + } + // Recorded against the OLD script body, before the fix landed. + v := &config.VM{ + OS: "alpine", + Applied: map[string]config.AppliedRecipe{ + "tool": {Version: "1.0", Hash: staleHash + "stale"}, + }, + } + + kept, _, err := filterByRunMode(v, []string{"tool"}, nil) + if err != nil { + t.Fatal(err) + } + if len(kept) != 1 || kept[0] != "tool" { + t.Errorf("kept = %v, want [tool]: the script hash no longer matches", kept) + } +} + +// TestFilterByRunModeRerunsOnEmptyStoredHash covers the pre-existing VM +// case: an Applied entry saved before Hash existed decodes with an empty +// string, which never equals a real hash, so the recipe self-heals once. +func TestFilterByRunModeRerunsOnEmptyStoredHash(t *testing.T) { + dir := root(t) + writeV2Recipe(t, dir, "tool", "once", "1.0", "#!/bin/sh\necho one\n") + v := &config.VM{ + OS: "alpine", + Applied: map[string]config.AppliedRecipe{ + "tool": {Version: "1.0"}, // no Hash: predates this field + }, + } + + kept, _, err := filterByRunMode(v, []string{"tool"}, nil) + if err != nil { + t.Fatal(err) + } + if len(kept) != 1 || kept[0] != "tool" { + t.Errorf("kept = %v, want [tool]: an empty stored hash never matches", kept) + } +} + +// TestFilterByRunModeAlwaysIgnoresHash pins that "always" keeps running +// regardless of a matching hash: run mode still wins over hash comparison. +func TestFilterByRunModeAlwaysIgnoresHash(t *testing.T) { + dir := root(t) + writeV2Recipe(t, dir, "tool", "always", "1.0", "#!/bin/sh\necho one\n") + hash, err := recipes.ScriptHash("tool", "alpine") + if err != nil { + t.Fatal(err) + } + v := &config.VM{ + OS: "alpine", + Applied: map[string]config.AppliedRecipe{ + "tool": {Version: "1.0", Hash: hash}, + }, + } + + kept, _, err := filterByRunMode(v, []string{"tool"}, nil) + if err != nil { + t.Fatal(err) + } + if len(kept) != 1 || kept[0] != "tool" { + t.Errorf("kept = %v, want [tool]: always never skips", kept) + } +} + +// TestFilterByRunModeManualNeedsExplicit pins that "manual" still needs +// explicit[name], unaffected by hash comparison. +func TestFilterByRunModeManualNeedsExplicit(t *testing.T) { + dir := root(t) + writeV2Recipe(t, dir, "tool", "manual", "1.0", "#!/bin/sh\necho one\n") + v := &config.VM{OS: "alpine"} + + kept, _, err := filterByRunMode(v, []string{"tool"}, nil) + if err != nil { + t.Fatal(err) + } + if len(kept) != 0 { + t.Errorf("kept = %v, want none: manual needs an explicit name", kept) + } + + kept, _, err = filterByRunMode(v, []string{"tool"}, map[string]bool{"tool": true}) + if err != nil { + t.Fatal(err) + } + if len(kept) != 1 || kept[0] != "tool" { + t.Errorf("kept = %v, want [tool]: named explicitly", kept) + } +} + +// TestApplyRecordsHashAndSkipsOnRerun exercises the whole path through +// Apply: a successful run records the script hash, and a second Apply with +// no script change selects nothing to run. +func TestApplyRecordsHashAndSkipsOnRerun(t *testing.T) { + dir := root(t) + writeV2Recipe(t, dir, "tool", "once", "1.0", "#!/bin/sh\necho one\n") + + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + c.Write([]byte("SSH-2.0-fake\r\n")) + c.Close() + } + }() + port := ln.Addr().(*net.TCPAddr).Port + + v := &config.VM{ + Name: "work", Mode: "live", OS: "alpine", Backend: "apkovl", + RAM: 512, CPUs: 1, SSHPort: port, Recipes: []string{"tool"}, + } + if err := v.Save(); err != nil { + t.Fatal(err) + } + v.Dir = dir + "/work" + stop := fakeRunning(t, v) + defer stop() + + // Provision needs a real ssh binary and sshd it will never reach; a + // cancelled ctx makes it fail fast right after the ssh banner check, + // before this test needs it to succeed. This test is about the + // pre/post-run bookkeeping in filterByRunMode and Apply's record step, + // exercised directly rather than through a full provisioning run. + hash, err := recipes.ScriptHash("tool", "alpine") + if err != nil { + t.Fatal(err) + } + explicit := map[string]bool{} + kept, manifests, err := filterByRunMode(v, v.Recipes, explicit) + if err != nil { + t.Fatal(err) + } + if len(kept) != 1 { + t.Fatalf("kept = %v, want [tool] on the first pass", kept) + } + m := manifests["tool"] + v.Applied = map[string]config.AppliedRecipe{ + "tool": {Version: m.Version, Hash: hash, At: time.Now()}, + } + if err := v.Save(); err != nil { + t.Fatal(err) + } + + reloaded, err := config.Load("work") + if err != nil { + t.Fatal(err) + } + if reloaded.Applied["tool"].Hash != hash { + t.Fatalf("saved Hash = %q, want %q", reloaded.Applied["tool"].Hash, hash) + } + + kept, _, err = filterByRunMode(reloaded, reloaded.Recipes, explicit) + if err != nil { + t.Fatal(err) + } + if len(kept) != 0 { + t.Errorf("kept = %v, want none: second pass, no script change", kept) + } +} + // TestCheckRecipesUsesDeclaredCapabilityReason pins that a recipe declaring // "requires: systemd" with no "os" restriction produces // docs/design/core-api.md ยง4's exact example, "requires systemd, alpine diff --git a/internal/core/vm.go b/internal/core/vm.go index 866127b..e0da1d4 100644 --- a/internal/core/vm.go +++ b/internal/core/vm.go @@ -68,11 +68,13 @@ type Paths struct { } // AppliedRecipe mirrors config.AppliedRecipe: which version of a recipe ran -// against a VM, and when. Defined here rather than re-exported from config, -// because core is the headless layer the CLI, the TUI and the MCP server all -// depend on, and a type alias would leak config as part of that contract. +// against a VM, the hash of the script that ran, and when. Defined here +// rather than re-exported from config, because core is the headless layer +// the CLI, the TUI and the MCP server all depend on, and a type alias would +// leak config as part of that contract. type AppliedRecipe struct { Version string + Hash string At time.Time } @@ -243,7 +245,7 @@ func applied(m map[string]config.AppliedRecipe) map[string]AppliedRecipe { } out := make(map[string]AppliedRecipe, len(m)) for k, v := range m { - out[k] = AppliedRecipe{Version: v.Version, At: v.At} + out[k] = AppliedRecipe{Version: v.Version, Hash: v.Hash, At: v.At} } return out } diff --git a/internal/recipes/recipes.go b/internal/recipes/recipes.go index 2e5831a..c74662a 100644 --- a/internal/recipes/recipes.go +++ b/internal/recipes/recipes.go @@ -338,3 +338,15 @@ func ScriptBody(name, osName string) (string, error) { } return m.ScriptContent(osName) } + +// ScriptHash returns the hex sha256 of ScriptBody(name, osName). A caller +// compares it against a stored AppliedRecipe.Hash to tell whether a "once" +// recipe's script changed since it last ran, even at the same manifest +// version. +func ScriptHash(name, osName string) (string, error) { + body, err := ScriptBody(name, osName) + if err != nil { + return "", err + } + return sum([]byte(body)), nil +} diff --git a/internal/recipes/recipes_test.go b/internal/recipes/recipes_test.go index e3f4741..9ad2479 100644 --- a/internal/recipes/recipes_test.go +++ b/internal/recipes/recipes_test.go @@ -372,3 +372,63 @@ func TestScriptBodyReadsV1FlatFile(t *testing.T) { t.Errorf("ScriptBody = %q, want %q", got, want) } } + +// TestScriptHashMatchesScriptBody pins ScriptHash to the sha256 of the exact +// bytes ScriptBody resolves, so a caller can compare it against a stored +// AppliedRecipe.Hash without re-deriving the sum itself. +func TestScriptHashMatchesScriptBody(t *testing.T) { + t.Setenv("STOAT_HOME", t.TempDir()) + rd := filepath.Join(dir(), "devtools") + if err := os.MkdirAll(rd, 0o755); err != nil { + t.Fatal(err) + } + toml := "name = \"devtools\"\nscript = \"install.sh\"\n" + if err := os.WriteFile(filepath.Join(rd, "recipe.toml"), []byte(toml), 0o644); err != nil { + t.Fatal(err) + } + body := "#!/bin/sh\necho devtools\n" + if err := os.WriteFile(filepath.Join(rd, "install.sh"), []byte(body), 0o755); err != nil { + t.Fatal(err) + } + + got, err := ScriptHash("devtools", "alpine") + if err != nil { + t.Fatalf("ScriptHash: %v", err) + } + if want := sum([]byte(body)); got != want { + t.Errorf("ScriptHash = %q, want %q", got, want) + } +} + +// TestScriptHashChangesWithScriptBody proves a fixed script produces a +// different hash than the original, the signal filterByRunMode uses to +// re-apply a "once" recipe whose script changed at the same version. +func TestScriptHashChangesWithScriptBody(t *testing.T) { + t.Setenv("STOAT_HOME", t.TempDir()) + rd := filepath.Join(dir(), "devtools") + if err := os.MkdirAll(rd, 0o755); err != nil { + t.Fatal(err) + } + toml := "name = \"devtools\"\nscript = \"install.sh\"\n" + if err := os.WriteFile(filepath.Join(rd, "recipe.toml"), []byte(toml), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(rd, "install.sh"), []byte("#!/bin/sh\necho one\n"), 0o755); err != nil { + t.Fatal(err) + } + before, err := ScriptHash("devtools", "alpine") + if err != nil { + t.Fatalf("ScriptHash: %v", err) + } + + if err := os.WriteFile(filepath.Join(rd, "install.sh"), []byte("#!/bin/sh\necho two\n"), 0o755); err != nil { + t.Fatal(err) + } + after, err := ScriptHash("devtools", "alpine") + if err != nil { + t.Fatalf("ScriptHash: %v", err) + } + if before == after { + t.Errorf("ScriptHash unchanged after the script body changed: %q", before) + } +}