From ae63ace7728bf33e70938cd0957c74edf631f066 Mon Sep 17 00:00:00 2001 From: NovusEdge Date: Mon, 10 Aug 2026 00:09:41 +0300 Subject: [PATCH] feat(core): re-apply a recipe when its script changed A "once" recipe skipped only on a version match. A fixed script at the same version never reran on a VM that already applied the old one. filterByRunMode now compares the script's sha256 hash instead of the version alone; recipes.ScriptHash resolves and hashes the same body ScriptBody returns. Apply records the hash alongside the version after each successful run. An Applied entry saved before Hash existed decodes with an empty string, which never matches a real hash, so an existing VM reruns its "once" recipes once and then tracks hashes from then on. --- internal/config/config.go | 6 +- internal/core/apply.go | 27 +++- internal/core/apply_test.go | 223 +++++++++++++++++++++++++++++++ internal/core/vm.go | 10 +- internal/recipes/recipes.go | 12 ++ internal/recipes/recipes_test.go | 60 +++++++++ 6 files changed, 327 insertions(+), 11 deletions(-) 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) + } +}