diff --git a/CHANGELOG.md b/CHANGELOG.md index 38a85a1d..19867ea6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,24 @@ # Changelog +## Unreleased + +### Added + +- The curated recipe index has its first entry, `caddy`, which installs the + Caddy web server and serves one directory over HTTP on Alpine, Ubuntu, + Debian, Fedora and Arch. `stoat recipe add caddy` now resolves a name + instead of needing a repository URL. `docs/recipes/sharing.md` describes how + to publish an entry. + +### Fixes + +- `stoat update --agent-access exec` set the level without updating the + legacy `allow_exec` field beside it, so the VM reported `allow_exec: false` + in every JSON and MCP payload while exec worked. The two stay in step now. +- Restoring or deleting a snapshot tag a VM does not have answered with qemu's + own wording under the error code `internal`, which reads as a fault in stoat. + It answers `not_found` now, and names the tags the VM does have. + ## v0.6.1 Fixes found by driving the MCP tools against a live VM. The JSON contract diff --git a/docs/guides/project-workflow.md b/docs/guides/project-workflow.md index 978d101c..a448cfa9 100644 --- a/docs/guides/project-workflow.md +++ b/docs/guides/project-workflow.md @@ -49,15 +49,20 @@ fields and validation rules. Declare a recipe in `[recipes]`, then resolve it to a commit: ```sh -stoat recipe add https://github.com/OWNER/REPOSITORY.git@TAG +stoat recipe add caddy stoat recipe lock stoat recipe sync ``` -Replace the URL and tag with a repository that contains a valid `recipe.toml`. -The repository's current `index.toml` has no published entries, so an index -name cannot be resolved from this checkout. Bundled recipes such as `docker` -are already available and do not need `recipe add`. +`caddy` is an index name. Pass a Git URL with an optional `@tag` instead for a +repository the index does not carry: + +```sh +stoat recipe add https://github.com/OWNER/REPOSITORY.git@TAG +``` + +Bundled recipes such as `docker` are already available and do not need +`recipe add`. An index name can be used when the configured index contains it. A Git URL can be passed instead, with an optional `@tag` or branch. `recipe add` writes the diff --git a/docs/recipes/sharing.md b/docs/recipes/sharing.md index 93968d92..73613f55 100644 --- a/docs/recipes/sharing.md +++ b/docs/recipes/sharing.md @@ -33,6 +33,27 @@ stoat recipe add https://github.com/example/stoat-my-tools@main -y name collision with a bundled, local, or remote recipe. Use it only when the replacement is intentional. +## Publish a recipe to the index + +Put the recipe in its own repository, with `recipe.toml` at the root and the +scripts beside it, then open a pull request against `index.toml` in the Stoat +repository: + +```toml +[recipes.caddy] +source = "https://github.com/NovusEdge/stoat-recipe-caddy" +description = "Caddy web server, serving a local directory over HTTP" +os = ["alpine", "ubuntu", "debian", "fedora", "arch"] +``` + +Pick a name no bundled recipe uses. `stoat recipe add` refuses a bundled name +without `--force`, and the MCP `add_recipe` tool passes no force, so an agent +can never install an entry named after a bundled recipe. + +Tag the recipe repository so users can pin a ref. An entry changes what a new +`recipe add` resolves; it never moves a recipe an existing VM already pinned, +because the commit lives in the user's `stoat.lock`. + List installed recipes, their scope, and the short commit pin: ```sh diff --git a/index.toml b/index.toml index 4d067f81..cae581e0 100644 --- a/index.toml +++ b/index.toml @@ -3,6 +3,15 @@ # name to the git repository that holds that recipe: recipe.toml at its root, # scripts beside it, one recipe per repository. The user's stoat.lock pins the # commit, so a change here never moves an existing VM's recipe. +# +# A name here must not repeat a bundled recipe name. CheckCollision refuses +# that name, and the MCP add_recipe tool passes no --force, so an agent could +# never install the entry. schema = 1 [recipes] + +[recipes.caddy] +source = "https://github.com/NovusEdge/stoat-recipe-caddy" +description = "Caddy web server, serving a local directory over HTTP" +os = ["alpine", "ubuntu", "debian", "fedora", "arch"] diff --git a/internal/core/remote_recipes_test.go b/internal/core/remote_recipes_test.go index 842d6649..0ae681ef 100644 --- a/internal/core/remote_recipes_test.go +++ b/internal/core/remote_recipes_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "testing" "time" @@ -23,6 +24,25 @@ func coreRemoteRoot(t *testing.T) string { return home } +// coreIndexRoot is coreRemoteRoot with a populated index, for the paths an +// agent reaches through add_recipe: a name, never a URL. +func coreIndexRoot(t *testing.T, sources map[string]string) string { + t.Helper() + home := root(t) + body := "schema = 1\n\n[recipes]\n" + names := make([]string, 0, len(sources)) + for name := range sources { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + body += fmt.Sprintf("\n[recipes.%s]\nsource = %q\ndescription = %q\n", name, sources[name], name+" test recipe") + } + index := testutil.GitRepo(t, map[string]string{"index.toml": body}) + t.Setenv("STOAT_INDEX", index) + return home +} + func coreRecipeRepo(t *testing.T, repoName, manifestName string) string { t.Helper() src := testutil.GitRepo(t, map[string]string{ @@ -108,6 +128,156 @@ func TestRemoveCheckedWithRecipeUsersDoesNotDeadlock(t *testing.T) { } } +// add_recipe refuses a URL, so an index name is the only source an agent can +// name. The declaration it writes carries the ref alone: the index owns the +// source, and a later index edit moves every project that names the recipe. +func TestAddRecipeResolvesAnIndexName(t *testing.T) { + src := coreRecipeRepo(t, "demo", "demo") + coreIndexRoot(t, map[string]string{"demo": src}) + project := t.TempDir() + t.Chdir(project) + if err := os.WriteFile(filepath.Join(project, "stoat.toml"), []byte("[recipes]\n"), 0o644); err != nil { + t.Fatal(err) + } + + if err := AddRecipe("demo", AddOpts{Yes: true}); err != nil { + t.Fatalf("AddRecipe(demo) = %v", err) + } + scope, err := recipes.ScopeFor(false) + if err != nil { + t.Fatal(err) + } + lock, err := scope.Lock() + if err != nil { + t.Fatal(err) + } + entry, ok := lock.Recipes["demo"] + if !ok { + t.Fatalf("lock = %+v, want a demo entry", lock.Recipes) + } + if entry.Source != src { + t.Fatalf("lock source = %q, want the index source %q", entry.Source, src) + } + if len(entry.Commit) != 40 { + t.Fatalf("lock commit = %q, want a full commit", entry.Commit) + } + decls, err := scope.Decls() + if err != nil { + t.Fatal(err) + } + if decl, ok := decls["demo"]; !ok || decl.Source != "" { + t.Fatalf("declaration = %+v, want an index name with no source", decl) + } +} + +// An index name that repeats a bundled name is unreachable from add_recipe: +// the tool passes no force, and CheckCollision refuses without it. Index +// entries have to avoid the bundled names. +func TestAddRecipeRefusesAnIndexNameThatShadowsABundledRecipe(t *testing.T) { + src := coreRecipeRepo(t, "xfce", "xfce") + coreIndexRoot(t, map[string]string{"xfce": src}) + if err := recipes.Install(); err != nil { + t.Fatal(err) + } + project := t.TempDir() + t.Chdir(project) + if err := os.WriteFile(filepath.Join(project, "stoat.toml"), []byte("[recipes]\n"), 0o644); err != nil { + t.Fatal(err) + } + + err := AddRecipe("xfce", AddOpts{Yes: true}) + if err == nil { + t.Fatal("AddRecipe(xfce) = nil, want a collision refusal") + } + if !strings.Contains(err.Error(), "bundled") { + t.Fatalf("AddRecipe(xfce) = %v, want the bundled collision named", err) + } +} + +func TestUpdateRecipeRepinsTheDeclaredRef(t *testing.T) { + src := coreRecipeRepo(t, "demo", "demo") + coreIndexRoot(t, map[string]string{"demo": src}) + project := t.TempDir() + t.Chdir(project) + if err := os.WriteFile(filepath.Join(project, "stoat.toml"), []byte("[recipes]\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := AddRecipe("demo", AddOpts{Yes: true}); err != nil { + t.Fatal(err) + } + moved := testutil.GitCommit(t, src, map[string]string{"install.sh": "#!/bin/sh\necho v2\n"}, "") + + if err := UpdateRecipe("demo"); err != nil { + t.Fatalf("UpdateRecipe(demo) = %v", err) + } + scope, err := recipes.ScopeFor(false) + if err != nil { + t.Fatal(err) + } + lock, err := scope.Lock() + if err != nil { + t.Fatal(err) + } + if got := lock.Recipes["demo"].Commit; got != moved { + t.Fatalf("lock commit = %q, want the new branch head %q", got, moved) + } + body, err := os.ReadFile(filepath.Join(scope.CachePath, "demo", "install.sh")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(body), "v2") { + t.Fatalf("cache install.sh = %q, want the updated script", body) + } +} + +// remove_recipe has no force, so this refusal is the whole guard on that tool. +func TestRemoveRecipeRefusesWhileAVMListsIt(t *testing.T) { + src := coreRecipeRepo(t, "demo", "demo") + coreIndexRoot(t, map[string]string{"demo": src}) + project := t.TempDir() + t.Chdir(project) + if err := os.WriteFile(filepath.Join(project, "stoat.toml"), []byte("[recipes]\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := AddRecipe("demo", AddOpts{Yes: true}); err != nil { + t.Fatal(err) + } + v := &config.VM{Name: "work", Mode: "live", OS: "alpine", RAM: 1024, CPUs: 1, SSHPort: 2200, Recipes: []string{"demo"}} + if err := v.Save(); err != nil { + t.Fatal(err) + } + + err := RemoveRecipe("demo", false) + if !errors.Is(err, ErrInUse) || !strings.Contains(err.Error(), "work") { + t.Fatalf("RemoveRecipe(demo) = %v, want ErrInUse naming work", err) + } + scope, err := recipes.ScopeFor(false) + if err != nil { + t.Fatal(err) + } + lock, err := scope.Lock() + if err != nil { + t.Fatal(err) + } + if _, ok := lock.Recipes["demo"]; !ok { + t.Fatal("the refused removal dropped the lock entry") + } + + if err := RemoveRecipe("demo", true); err != nil { + t.Fatalf("RemoveRecipe(demo, force) = %v", err) + } + lock, err = scope.Lock() + if err != nil { + t.Fatal(err) + } + if _, ok := lock.Recipes["demo"]; ok { + t.Fatal("the forced removal left the lock entry") + } + if _, err := os.Stat(filepath.Join(scope.CachePath, "demo")); !os.IsNotExist(err) { + t.Fatalf("cache stat = %v, want the checkout gone", err) + } +} + func TestPlanApplyAndNeedsProvisionUseAFreshProjectCacheWithoutSync(t *testing.T) { home := coreRemoteRoot(t) project := t.TempDir() diff --git a/internal/core/snapshot.go b/internal/core/snapshot.go index a1132cd3..eb2f072a 100644 --- a/internal/core/snapshot.go +++ b/internal/core/snapshot.go @@ -66,12 +66,38 @@ func Restore(name, tag string) error { if err != nil { return err } + if err := requireSnapshot(name, tag); err != nil { + return err + } if qemu.Running(v) { return qemu.SnapshotLoad(v, tag) } return qemuImgSnapshot(v, "-a", tag) } +// requireSnapshot rejects a tag the VM does not have, before qemu does. Both +// backends answer an unknown tag with their own prose, which reached a caller +// as an unmapped internal failure rather than the caller's own mistake. The +// known tags come back in the message because a wrong tag is usually a typo +// of a right one. +func requireSnapshot(name, tag string) error { + snaps, err := Snapshots(name) + if err != nil { + return err + } + tags := make([]string, 0, len(snaps)) + for _, s := range snaps { + if s.Tag == tag { + return nil + } + tags = append(tags, s.Tag) + } + if len(tags) == 0 { + return fmt.Errorf("%w: %s has no snapshot %q, and none at all", ErrNotFound, name, tag) + } + return fmt.Errorf("%w: %s has no snapshot %q; it has %s", ErrNotFound, name, tag, strings.Join(tags, ", ")) +} + // DeleteSnapshot removes one snapshot and frees its space in the qcow2. It // is not part of Destroy's naming family: deleting a VM and deleting a // snapshot are very different sizes of mistake. @@ -80,6 +106,9 @@ func DeleteSnapshot(name, tag string) error { if err != nil { return err } + if err := requireSnapshot(name, tag); err != nil { + return err + } if qemu.Running(v) { return qemu.SnapshotDelete(v, tag) } diff --git a/internal/core/snapshot_test.go b/internal/core/snapshot_test.go index 76b2e55b..54665016 100644 --- a/internal/core/snapshot_test.go +++ b/internal/core/snapshot_test.go @@ -2,6 +2,7 @@ package core import ( "errors" + "strings" "testing" "github.com/novusedge/stoat/internal/config" @@ -125,6 +126,37 @@ func TestSnapshotRejectsBadTags(t *testing.T) { } } +// An unknown tag used to reach qemu, which answered in its own prose and +// mapped to the internal code. It is the caller's mistake, so it is not_found. +func TestRestoreAndDeleteRejectAnUnknownTag(t *testing.T) { + // The stopped-VM path reads the tag list with qemu-img. Without it the + // lookup fails before the tag is ever compared. + haveQemuImg(t) + root(t) + // A cloud VM that never started has no qcow2, so Snapshots reports none + // rather than failing. That is the empty case the message distinguishes. + if err := (&config.VM{Name: "d", Mode: "cloud", RAM: 512, CPUs: 1, SSHPort: 2200, Disk: "8G"}).Save(); err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + name string + run func() error + }{ + {"Restore", func() error { return Restore("d", "nope") }}, + {"DeleteSnapshot", func() error { return DeleteSnapshot("d", "nope") }}, + } { + t.Run(tc.name, func(t *testing.T) { + err := tc.run() + if !errors.Is(err, ErrNotFound) { + t.Fatalf("%s = %v, want ErrNotFound", tc.name, err) + } + if !strings.Contains(err.Error(), "nope") { + t.Fatalf("%s = %v, want the tag named", tc.name, err) + } + }) + } +} + func TestSnapshotUnknownVM(t *testing.T) { root(t) if err := TakeSnapshot("nope", "x"); !errors.Is(err, ErrNotFound) { diff --git a/internal/core/update.go b/internal/core/update.go index c394b145..63da9311 100644 --- a/internal/core/update.go +++ b/internal/core/update.go @@ -198,6 +198,11 @@ func Update(name string, p Patch) (VM, error) { } if p.AgentAccess != nil { work.AgentAccess = *p.AgentAccess + // Create derives allow_exec from the level and the two never disagree + // on disk. Setting the level alone here left the legacy field behind, + // so a VM raised to exec reported allow_exec false to every wire + // reader while exec itself worked. + work.AllowExec = work.AgentAccess == "exec" } if p.SSHPort != nil && *p.SSHPort != work.SSHPort { diff --git a/internal/core/update_test.go b/internal/core/update_test.go index 5d1a42a7..1a480f5f 100644 --- a/internal/core/update_test.go +++ b/internal/core/update_test.go @@ -43,6 +43,45 @@ func TestUpdateImmutableFields(t *testing.T) { } } +// Create derives allow_exec from agent_access, and every wire payload still +// carries the legacy field. An update that changed only the level left a VM +// reporting allow_exec false while exec worked. +func TestUpdateAgentAccessKeepsAllowExecInStep(t *testing.T) { + root(t) + v := &config.VM{Name: "work", Mode: "live", OS: "alpine", Backend: "apkovl", RAM: 1024, CPUs: 1, SSHPort: 2200, AgentAccess: "manage"} + if err := v.Save(); err != nil { + t.Fatal(err) + } + + for _, tc := range []struct { + level string + want bool + }{ + {"exec", true}, + {"observe", false}, + } { + t.Run(tc.level, func(t *testing.T) { + got, err := Update("work", Patch{AgentAccess: ptr(tc.level)}) + if err != nil { + t.Fatal(err) + } + if got.AgentAccess != tc.level { + t.Fatalf("agent_access = %q, want %q", got.AgentAccess, tc.level) + } + if got.AllowExec != tc.want { + t.Fatalf("allow_exec = %v for level %q, want %v", got.AllowExec, tc.level, tc.want) + } + stored, err := config.Load("work") + if err != nil { + t.Fatal(err) + } + if stored.AllowExec != tc.want { + t.Fatalf("vm.toml allow_exec = %v, want %v", stored.AllowExec, tc.want) + } + }) + } +} + // Setting an immutable field to its current value is not a change // request. An MCP tool or CLI flag that round-trips a full VM through // Patch must not be refused for fields it never meant to touch.