feat(core): re-apply a recipe when its script changed - #30
Merged
Conversation
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.
WalkthroughRecipe application state now stores a SHA-256 script hash. ChangesRecipe hash tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Apply
participant ScriptHash
participant AppliedRecipe
participant VM
Apply->>ScriptHash: compute executed recipe script hash
ScriptHash-->>Apply: return SHA-256 hash
Apply->>AppliedRecipe: record version, timestamp, and hash
Apply->>VM: persist applied recipe state
VM->>AppliedRecipe: reload stored hash
Apply->>ScriptHash: compute current script hash
ScriptHash-->>Apply: return current hash
Apply-->>AppliedRecipe: skip once recipe when hashes match
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/apply_test.go`:
- Around line 399-474: Update TestApplyRecordsHashAndSkipsOnRerun to exercise
Apply directly instead of manually assigning v.Applied and calling
filterByRunMode. Add a provisioning seam or test double that allows Apply to
complete successfully, then assert the first Apply persists the executed script
hash and the second Apply performs no work while preserving the existing recipe
setup.
In `@internal/core/apply.go`:
- Around line 149-153: Update the apply flow around sshx.Provision and
recipes.ScriptHash so each recipe script is resolved once into an immutable
snapshot before provisioning; pass that snapshot to provisioning, then persist
its hash in v.Applied[name] after success. Do not reread the mutable recipe
source afterward, ensuring the recorded hash matches the script that executed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76fe8849-ffba-4688-b06d-15e8d682ee35
📒 Files selected for processing (6)
internal/config/config.gointernal/core/apply.gointernal/core/apply_test.gointernal/core/vm.gointernal/recipes/recipes.gointernal/recipes/recipes_test.go
Comment on lines
+399
to
+474
| // 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Test the Apply persistence path.
Lines 439-454 calculate ScriptHash and assign v.Applied directly. Lines 467-473 only call filterByRunMode. This test never calls Apply, so it cannot detect a regression in Apply lines 149-159.
Add a provisioning seam that lets this test complete Apply successfully. Then assert that Apply saves the executed hash and that a second Apply is a no-op.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/apply_test.go` around lines 399 - 474, Update
TestApplyRecordsHashAndSkipsOnRerun to exercise Apply directly instead of
manually assigning v.Applied and calling filterByRunMode. Add a provisioning
seam or test double that allows Apply to complete successfully, then assert the
first Apply persists the executed script hash and the second Apply performs no
work while preserving the existing recipe setup.
Comment on lines
+149
to
+153
| 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()} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Persist the hash of the executed script snapshot.
Line 149 reads the script after sshx.Provision completes. If the recipe changes during provisioning, provisioning can execute script A while this code saves script B's hash. The next once apply then skips script B although it did not run.
Resolve each script into an immutable snapshot before provisioning. Make provisioning execute that snapshot. Persist the same snapshot hash after success.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/apply.go` around lines 149 - 153, Update the apply flow around
sshx.Provision and recipes.ScriptHash so each recipe script is resolved once
into an immutable snapshot before provisioning; pass that snapshot to
provisioning, then persist its hash in v.Applied[name] after success. Do not
reread the mutable recipe source afterward, ensuring the recorded hash matches
the script that executed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part A of the auto-provision design (spec: 2026-08-10-auto-provision).
A "once" recipe was skipped when
Applied[name].Versionmatched the manifest version. A recipe author who fixes a script without bumping the version could not reach a VM that already ran the old one — which is exactly why the udev fix never reached already-provisioned VMs.AppliedRecipegains aHashfield: the hex SHA-256 of the script the VM's OS runs (recipes.ScriptHashoverScriptBody).filterByRunModeskips a "once" recipe only when the stored hash matches the current script's hash; a changed script re-runs at the same version.alwaysandmanualare untouched.An
Appliedentry written beforeHashexisted decodes as empty, which never matches a real hash, so an existing VM re-runs its "once" recipes exactly once and then carries a real hash. That is the self-heal: v4 and v5 pick up the udev recipe fix on their next provision with no manual step.Tests:
filterByRunModeacross matching/changed/empty hash plus always/manual;Applyrecords the hash and a second apply with no change runs nothing;ScriptHashtracksScriptBody.just checkandjust testpass.Summary by CodeRabbit