From 46389d1b328cecebc68f709fd4135ab7eaab743b Mon Sep 17 00:00:00 2001 From: NovusEdge Date: Tue, 29 Sep 2026 21:16:24 +0300 Subject: [PATCH] fix(errors): one not-found error, usage tails, limit_reached CLI: a usage error prints the failing command's usage line and a pointer to its help instead of the root help. `stoat exec --help` and `stoat help ` show the command's help. A missing VM is one error, `no VM "x" (did you mean "y"?); see stoat ls`, with no on-disk path. An unknown image suggests catalog ids and points at `stoat images`. cp no longer doubles the scp prefix, and a refused confirmation says why. The invalid alpine-3.21 help example is fixed. MCP: core.ErrLimit maps to limit_reached with needed_mb and available_mb, and the start message drops -y. Input mistakes in the tool guards return usage, not internal. Errors name MCP tools, keep stderr on index-refresh failure, and list valid values. Closed-set inputs carry JSON-schema enums. Signed-off-by: NovusEdge --- docs/reference/json.md | 2 + docs/reference/mcp.md | 7 ++ docs/reference/project-file.md | 2 +- docs/troubleshooting.md | 10 +-- go.mod | 2 +- internal/capabilities/load.go | 2 +- internal/cli/cli.go | 51 +++++++++++++- internal/cli/cli_test.go | 52 ++++++++++++++ internal/cli/confirm.go | 15 ++-- internal/cli/grammar.go | 2 +- internal/cli/project.go | 4 +- internal/cli/project_test.go | 9 ++- internal/cli/run_recipe_remote.go | 2 +- internal/cli/wire/envelope.go | 3 + internal/cli/wire/errors.go | 11 ++- internal/cli/wire/errors_test.go | 11 +++ internal/config/config.go | 3 + internal/config/namespace.go | 21 ++++++ internal/core/copy.go | 2 +- internal/core/image.go | 13 +++- internal/core/images.go | 3 +- internal/core/limits.go | 33 +++++++-- internal/core/vm.go | 2 +- internal/core/wait.go | 10 ++- internal/coreerr/coreerr.go | 96 +++++++++++++++++++++++++- internal/coreerr/coreerr_test.go | 51 ++++++++++++++ internal/mcpsrv/access.go | 3 +- internal/mcpsrv/capabilities_test.go | 2 +- internal/mcpsrv/error_contract_test.go | 77 +++++++++++++++++++++ internal/mcpsrv/guards.go | 46 ++++++------ internal/mcpsrv/server.go | 51 ++++++++++++++ internal/mcpsrv/tools_exec.go | 10 +-- internal/mcpsrv/tools_guest.go | 14 ++-- internal/mcpsrv/tools_recipe.go | 5 +- 34 files changed, 555 insertions(+), 72 deletions(-) create mode 100644 internal/coreerr/coreerr_test.go diff --git a/docs/reference/json.md b/docs/reference/json.md index a174147e..4b696ba2 100644 --- a/docs/reference/json.md +++ b/docs/reference/json.md @@ -102,6 +102,7 @@ types can be added without a version bump. | `message` | string | always, human-readable, not machine-parseable | | `subject` | string | reserved, see below | | `kind` | string | reserved, see below | +| `needed_mb`, `available_mb` | number | `limit_reached` for a RAM refusal only | Branch on `code`. Never parse `message`. @@ -149,6 +150,7 @@ bump the contract version. Do not write code that requires them. | `confirmation_required` | a destructive command was run without `-y` | | `access_denied` | MCP guest access was refused because the VM's `agent_access` level is too low | | `rate_limited` | MCP refused a tool call because its per-tool or shared rate bucket was exhausted | +| `limit_reached` | a configured limit or the host's free memory refuses the operation; a RAM refusal adds `needed_mb` and `available_mb` | | `lock_out_of_date` | a project declaration is not pinned in `stoat.lock` | | `host_unsupported` | this host has no qualified native VM runtime yet | | `capability_unavailable` | the VM's provider does not support this operation | diff --git a/docs/reference/mcp.md b/docs/reference/mcp.md index 18d658c6..c75526cf 100644 --- a/docs/reference/mcp.md +++ b/docs/reference/mcp.md @@ -186,6 +186,13 @@ unavailable fork and continuation proposals. Discovery does not mutate a VM. | `snapshot`, `restore`, `forward`, `wait`, `prune` | Manage disk snapshots, port forwards, state waits, and stale files. `prune` is dry-run unless `apply=true`. | | `project_status`, `project_up`, `project_down`, `project_apply`, `project_wait` | Inspect or operate on every VM declared by the server working directory's `stoat.toml`, in declaration order. A failure stops the run and later VMs are marked skipped. | +`start` has no force option. When the VM needs more memory than the host has +free, or the RAM limit refuses it, the tool returns `limit_reached` with +`needed_mb` and `available_mb`. Lower `ram_mb` with `update`, or ask a person +to start the VM with `stoat up -y`. Inputs with a fixed set of values +(`until`, `mode`, `agent_access`, `stream`, `action`, `which`) list them as +`enum` in the tool schema. + `wait` accepts `reachable`, `applied`, or `stopped`; `healthy=true` waits for the applied recipes' health checks. Its `timeout_seconds` is a count of seconds capped at 600, not a duration string. diff --git a/docs/reference/project-file.md b/docs/reference/project-file.md index 2de86d01..46af8ebc 100644 --- a/docs/reference/project-file.md +++ b/docs/reference/project-file.md @@ -143,4 +143,4 @@ the stop request can return while QEMU is still exiting. | share outside project | `stoat.toml: vms.dev.shares: "../secrets" is outside the project` | | immutable change | `dev: image changed (ubuntu-24 → debian-12); run stoat rm dev and stoat up` | | new at project scope | `a stoat.toml is present; declare the VM there and run stoat up, or pass --global` | -| unknown key in a bare argument | `no VM "db" in stoat.toml or ~/.stoat/vms` | +| unknown key in a bare argument | `no VM "db"; see stoat ls` | diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index b8ab2956..3c5d88b1 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -239,15 +239,17 @@ deletes its directory and persistent disk. A project command without a VM name uses declaration order; use the key or global name when repairing one entry. -## `stoat up` says `no VM ... in stoat.toml or ~/.stoat/vms` +## `stoat up` says `no VM ""; see stoat ls` Project scope is active only when `stoat.toml` exists in the current directory. Stoat does not walk up to a parent directory. A bare VM argument must be a declaration key, a declared `name`, or a global VM that exists in -the data root. +the data root. When a similar name exists, the message adds +`(did you mean ""?)`. -**Fix:** change to the directory containing the intended `stoat.toml`, use the -declaration key, or create a global VM with `stoat create --global ...`. +**Fix:** run `stoat ls` to see the VM names. Change to the directory +containing the intended `stoat.toml`, use the declaration key, or create a +global VM with `stoat create --global ...`. ## `stoat recipe lock` says the lock is out of date diff --git a/go.mod b/go.mod index 5e22bbe5..c85f08d2 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,7 @@ require ( github.com/charmbracelet/x/ansi v0.11.8 github.com/charmbracelet/x/term v0.2.2 github.com/diskfs/go-diskfs v1.9.4 + github.com/google/jsonschema-go v0.4.3 github.com/googleapis/gax-go/v2 v2.26.2 github.com/modelcontextprotocol/go-sdk v1.8.0 github.com/pelletier/go-toml/v2 v2.4.3 @@ -47,7 +48,6 @@ require ( github.com/go-logfmt/logfmt v0.6.1 // indirect github.com/go-logr/logr v1.4.4 // indirect github.com/go-logr/stdr v1.2.2 // indirect - github.com/google/jsonschema-go v0.4.3 // indirect github.com/google/s2a-go v0.1.10 // indirect github.com/google/uuid v1.6.0 // indirect github.com/googleapis/enterprise-certificate-proxy v0.3.22 // indirect diff --git a/internal/capabilities/load.go b/internal/capabilities/load.go index f0e33c66..009a59c2 100644 --- a/internal/capabilities/load.go +++ b/internal/capabilities/load.go @@ -20,7 +20,7 @@ func LoadTarget(name string) (Target, error) { path := filepath.Join(config.Root(), name, "vm.toml") if _, err := os.Stat(path); err != nil { if os.IsNotExist(err) { - return Target{}, fmt.Errorf("%w: %s", coreerr.ErrNotFound, name) + return Target{}, config.NoVM(name) } return Target{}, fmt.Errorf("%w: %s: %v", coreerr.ErrBroken, name, err) } diff --git a/internal/cli/cli.go b/internal/cli/cli.go index eded228d..fc429b70 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -264,6 +264,11 @@ func commandPath(ctx *kong.Context) string { // as a pure function: kong.Parse populates the grammar and returns, and no // Run() method exists for it to call. func Parse(args []string) (*Args, error) { + if len(args) > 1 && args[0] == "help" { + // `help ` is ` --help`, which is also the only spelling kong + // and parseExec both understand. + args = append(append([]string{}, args[1:]...), "--help") + } if len(args) > 0 && args[0] == "exec" { return parseExec(args[1:]) } @@ -348,6 +353,9 @@ func preserveRecipeSearchTerm(args []string) []string { // requires to arrive untouched. wire.SplitJSONFlag stops its own argv scan // at exec's VM name for the same reason. func parseExec(rest []string) (*Args, error) { + if len(rest) > 0 && (rest[0] == "-h" || rest[0] == "--help") { + return &Args{Cmd: "help", Help: kongHelp("exec", "--help")}, nil + } if len(rest) == 0 { return nil, usageError("exec: missing vm name") } @@ -363,6 +371,47 @@ func parseExec(rest []string) (*Args, error) { return &Args{Cmd: "exec", VM: name, Command: cmd}, nil } +// kongHelp returns the text kong prints for argv, which must end in --help. +func kongHelp(argv ...string) string { + var g grammar + var help bytes.Buffer + p, err := newParser(&g, &help) + if err != nil { + return "cli grammar: " + err.Error() + } + _, _ = p.Parse(argv) + return help.String() +} + +// usageTail is what follows a usage error: the failing command's usage line +// and a pointer to its help. The whole root help is 60 lines and buries the +// error above it. +func usageTail(argv []string) string { + var g grammar + var buf bytes.Buffer + p, err := newParser(&g, &buf) + if err != nil { + return "" + } + if len(argv) > 0 && argv[0] == "exec" { + // parseExec bypasses kong, so trace only the command name. + argv = argv[:1] + } + ctx, err := kong.Trace(p, argv) + if err != nil { + return "" + } + _ = ctx.PrintUsage(true) + // The summary carries the description and its own hint after the usage + // line; only the line is wanted. + usage, _, _ := strings.Cut(buf.String(), "\n") + cmd := commandPath(ctx) + if cmd == "" { + return usage + "\nRun \"stoat --help\" for the command list.\n" + } + return usage + "\nRun \"stoat " + cmd + " --help\" for more information.\n" +} + // helpText renders the same text `stoat --help` prints, for the `help` // subcommand and for --json help. It is generated from the grammar, so unlike // the usage() string it replaces, it cannot drift from the actual flags. @@ -460,7 +509,7 @@ func Main(args []string, version string, stdin io.Reader, stdout, stderr io.Writ return ExitUsage } fmt.Fprintln(stderr, "stoat:", err) - fmt.Fprintln(stderr, helpText()) + fmt.Fprint(stderr, usageTail(argv)) return ExitUsage } a.JSON = jsonMode diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index e6fe1566..b1e57cb1 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -626,3 +626,55 @@ func TestParseCPResolvesLocalToAbsolutePath(t *testing.T) { t.Errorf("flag form: Local = %q, want %q", got.Local, want) } } + +func TestUsageErrorPrintsCommandUsageNotRootHelp(t *testing.T) { + var out, errOut bytes.Buffer + if code := Main([]string{"get"}, "test", nil, &out, &errOut); code != ExitUsage { + t.Fatalf("exit = %d, want %d", code, ExitUsage) + } + got := errOut.String() + for _, want := range []string{"Usage: stoat get ", `Run "stoat get --help"`} { + if !strings.Contains(got, want) { + t.Errorf("stderr missing %q:\n%s", want, got) + } + } + if strings.Contains(got, "Run local QEMU VMs") || strings.Count(got, "\n") > 4 { + t.Errorf("stderr carries the root help:\n%s", got) + } +} + +func TestHelpForExec(t *testing.T) { + for _, argv := range [][]string{{"exec", "--help"}, {"exec", "-h"}, {"help", "exec"}} { + var out, errOut bytes.Buffer + if code := Main(argv, "test", nil, &out, &errOut); code != ExitOK { + t.Fatalf("%v: exit = %d, stderr %q", argv, code, errOut.String()) + } + if !strings.Contains(out.String(), "Usage: stoat exec ") { + t.Errorf("%v: stdout is not exec's help:\n%s", argv, out.String()) + } + } +} + +func TestHelpForNestedCommand(t *testing.T) { + var out, errOut bytes.Buffer + if code := Main([]string{"help", "recipe", "new"}, "test", nil, &out, &errOut); code != ExitOK { + t.Fatalf("exit = %d, stderr %q", code, errOut.String()) + } + if !strings.Contains(out.String(), "Usage: stoat recipe new") { + t.Errorf("stdout is not recipe new's help:\n%s", out.String()) + } +} + +func TestMissingVMNamesTheFix(t *testing.T) { + cliRoot(t) + if err := (&config.VM{Name: "qol1", Mode: "live", RAM: 1024, CPUs: 1, SSHPort: 2200}).Save(); err != nil { + t.Fatal(err) + } + var out, errOut bytes.Buffer + if code := Main([]string{"ssh", "qol"}, "test", nil, &out, &errOut); code != ExitFail { + t.Fatalf("exit = %d, want %d", code, ExitFail) + } + if got, want := errOut.String(), "stoat: ssh: no VM \"qol\" (did you mean \"qol1\"?); see stoat ls\n"; got != want { + t.Errorf("stderr = %q, want %q", got, want) + } +} diff --git a/internal/cli/confirm.go b/internal/cli/confirm.go index f793b83f..d1075dc8 100644 --- a/internal/cli/confirm.go +++ b/internal/cli/confirm.go @@ -18,11 +18,18 @@ func confirm(a *Args, stdin io.Reader, stdout, stderr io.Writer, prompt string) if a.Yes { return true, ExitOK } - if a.JSON { - return false, a.failMsg(stdout, stderr, wire.ErrConfirmationRequired, prompt+"; pass -y to confirm") + why := "" + switch { + case a.JSON: + why = "--json never prompts" + case a.Quiet: + why = "--quiet never prompts" + case !terminal(stdin) || !terminal(stdout): + why = "no terminal" } - if a.Quiet || !terminal(stdin) || !terminal(stdout) { - return false, a.failMsg(stdout, stderr, wire.ErrConfirmationRequired, prompt+"; pass -y to confirm") + if why != "" { + msg := "refusing to " + strings.TrimSuffix(prompt, "?") + " without confirmation (" + why + "); pass -y" + return false, a.failMsg(stdout, stderr, wire.ErrConfirmationRequired, msg) } fmt.Fprintf(stdout, "%s [y/N] ", prompt) line, _ := bufio.NewReader(stdin).ReadString('\n') diff --git a/internal/cli/grammar.go b/internal/cli/grammar.go index 49cc9400..09a285a7 100644 --- a/internal/cli/grammar.go +++ b/internal/cli/grammar.go @@ -227,7 +227,7 @@ type createCmd struct { func (createCmd) Help() string { return `Examples: stoat create dev --image debian-13 --ram 2048 --cpus 2 - stoat create lab --image alpine-3.21 --mode live --agent-access observe + stoat create lab --image alpine-standard --mode live --agent-access observe stoat create web --image ubuntu-24.04 --recipes nginx --set nginx.port=8080` } diff --git a/internal/cli/project.go b/internal/cli/project.go index 5a03c2d2..331fc7cc 100644 --- a/internal/cli/project.go +++ b/internal/cli/project.go @@ -1,8 +1,6 @@ package cli import ( - "fmt" - "github.com/novusedge/stoat/internal/capabilities" "github.com/novusedge/stoat/internal/project" ) @@ -57,7 +55,7 @@ func resolveScope(a *Args) error { return nil } if _, err := coreGet(a.VM); err != nil { - return fmt.Errorf("no VM %q in %s or ~/.stoat/vms", a.VM, project.FileName) + return err } return nil } diff --git a/internal/cli/project_test.go b/internal/cli/project_test.go index 3cf7b514..7e79fa4f 100644 --- a/internal/cli/project_test.go +++ b/internal/cli/project_test.go @@ -3,7 +3,6 @@ package cli import ( "os" "path/filepath" - "strings" "testing" "github.com/novusedge/stoat/internal/config" @@ -51,9 +50,9 @@ func TestBareKeyResolvesToTheGlobalName(t *testing.T) { } } -// An argument that is neither a key nor an existing VM names both places it -// was looked for. -func TestUnknownBareArgumentNamesBothScopes(t *testing.T) { +// An argument that is neither a key nor an existing VM gets the same +// not-found error as everywhere else. +func TestUnknownBareArgumentIsNoVM(t *testing.T) { projectRoot(t, twoVMs) code, objs := runJSON(t, "get", "db") if code != ExitFail { @@ -61,7 +60,7 @@ func TestUnknownBareArgumentNamesBothScopes(t *testing.T) { } errObj, _ := result(t, objs)["error"].(map[string]any) msg, _ := errObj["message"].(string) - if !strings.Contains(msg, `no VM "db" in stoat.toml or ~/.stoat/vms`) { + if msg != `no VM "db"; see stoat ls` { t.Errorf("message = %q", msg) } } diff --git a/internal/cli/run_recipe_remote.go b/internal/cli/run_recipe_remote.go index 207d4ab4..f0013d4d 100644 --- a/internal/cli/run_recipe_remote.go +++ b/internal/cli/run_recipe_remote.go @@ -32,7 +32,7 @@ func runRecipeAdd(a *Args, stdin io.Reader, stdout, stderr io.Writer) int { source, gitRef, isURL := recipes.ParseRef(a.Ref) if isURL && !a.Yes { if a.JSON || !terminal(stdin) || !terminal(stdout) { - _, code := confirm(a, stdin, stdout, stderr, "install this recipe; pass -y to confirm") + _, code := confirm(a, stdin, stdout, stderr, "install this recipe") return code } m, tmp, previewErr := recipes.Preview(source, gitRef) diff --git a/internal/cli/wire/envelope.go b/internal/cli/wire/envelope.go index 902fd950..7bd3ba4d 100644 --- a/internal/cli/wire/envelope.go +++ b/internal/cli/wire/envelope.go @@ -57,6 +57,9 @@ type ErrorInfo struct { // constants, so an agent branches on a field instead of parsing prose // out of Message. Reason string `json:"reason,omitempty"` + // NeededMB and AvailableMB are present on limit_reached for a RAM refusal. + NeededMB int `json:"needed_mb,omitempty"` + AvailableMB int `json:"available_mb,omitempty"` } // WithSubject sets Subject and Kind and returns e, for a one-line call at diff --git a/internal/cli/wire/errors.go b/internal/cli/wire/errors.go index 645f46ba..709e7d3d 100644 --- a/internal/cli/wire/errors.go +++ b/internal/cli/wire/errors.go @@ -45,6 +45,7 @@ const ( CodeConfirmationRequired Code = "confirmation_required" CodeAccessDenied Code = "access_denied" CodeRateLimited Code = "rate_limited" + CodeLimitReached Code = "limit_reached" CodeInternal Code = "internal" CodeQemuMissing Code = "qemu_missing" @@ -83,7 +84,7 @@ func Codes() []Code { CodeAlreadyRunning, CodeNoDisk, CodeImmutableField, CodeDiskShrink, CodeCannotReach, CodeUnknownLog, CodeTimeout, CodeCanceled, CodeUsage, CodeConfirmationRequired, CodeInternal, - CodeAccessDenied, CodeRateLimited, + CodeAccessDenied, CodeRateLimited, CodeLimitReached, CodeQemuMissing, CodeKVMUnusable, CodeQemuStartFailed, CodeMonitorUnreachable, CodeMonitorRejected, CodeNoConsolePassword, CodeShareInvalid, CodeNoXattr, @@ -173,6 +174,7 @@ var codeTable = []struct { {CodeConfirmationRequired, ErrConfirmationRequired}, {CodeAccessDenied, ErrAccessDenied}, {CodeRateLimited, ErrRateLimited}, + {CodeLimitReached, core.ErrLimit}, {CodeUsage, ErrBadInput}, {CodeQemuMissing, qemu.ErrBinaryMissing}, {CodeKVMUnusable, qemu.ErrKVMUnusable}, @@ -219,7 +221,12 @@ func MapError(err error) *ErrorInfo { } for _, e := range codeTable { if errors.Is(err, e.err) { - return &ErrorInfo{Code: e.code, Message: err.Error()} + info := &ErrorInfo{Code: e.code, Message: err.Error()} + var limit *core.LimitError + if errors.As(err, &limit) { + info.NeededMB, info.AvailableMB = limit.NeededMB, limit.AvailableMB + } + return info } } return &ErrorInfo{Code: CodeInternal, Message: err.Error()} diff --git a/internal/cli/wire/errors_test.go b/internal/cli/wire/errors_test.go index 7f688bf1..22b7f869 100644 --- a/internal/cli/wire/errors_test.go +++ b/internal/cli/wire/errors_test.go @@ -300,3 +300,14 @@ func TestEveryISOSentinelHasOneRow(t *testing.T) { } } } + +func TestMapErrorLimitReachedCarriesTheNumbers(t *testing.T) { + err := fmt.Errorf("start: %w", &core.LimitError{NeededMB: 4096, AvailableMB: 1500}) + got := MapError(err) + if got.Code != CodeLimitReached || got.NeededMB != 4096 || got.AvailableMB != 1500 { + t.Errorf("MapError = %+v, want limit_reached with 4096 needed and 1500 available", got) + } + if plain := MapError(fmt.Errorf("%w: 3 vms exist", core.ErrLimit)); plain.Code != CodeLimitReached || plain.NeededMB != 0 { + t.Errorf("count limit = %+v, want limit_reached without MB fields", plain) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 0d1d9e92..5f819924 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -304,6 +304,9 @@ func (v *VM) Save() error { // Load reads one VM by name. func Load(name string) (*VM, error) { + if !Exists(name) { + return nil, NoVM(name) + } dir := DirFor(name) path := filepath.Join(dir, "vm.toml") // Absent allow_exec means true; the seed survives the decode, a written diff --git a/internal/config/namespace.go b/internal/config/namespace.go index 6fd760a7..270626ed 100644 --- a/internal/config/namespace.go +++ b/internal/config/namespace.go @@ -3,6 +3,8 @@ package config import ( "os" "path/filepath" + + "github.com/novusedge/stoat/internal/coreerr" ) // v2Dir is where a VM whose provider is not the local hypervisor lives. @@ -44,6 +46,25 @@ func Exists(name string) bool { return false } +// NoVM is the not-found error for name. Load returns it for a missing +// vm.toml so no caller prints the record's on-disk path. +func NoVM(name string) error { + return &coreerr.NoVMError{Name: name, Known: knownNames} +} + +func knownNames() []string { + var names []string + entries, _ := os.ReadDir(Root()) + for _, e := range entries { + if e.IsDir() && !reserved(e.Name()) && e.Name() != v2Dir { + if _, err := os.Stat(filepath.Join(Root(), e.Name(), "vm.toml")); err == nil { + names = append(names, e.Name()) + } + } + } + return append(names, v2Names()...) +} + // v2Names lists the VM directories under v2, or nothing when it is absent. func v2Names() []string { entries, err := os.ReadDir(filepath.Join(Root(), v2Dir)) diff --git a/internal/core/copy.go b/internal/core/copy.go index c999d48c..bfe39b59 100644 --- a/internal/core/copy.go +++ b/internal/core/copy.go @@ -89,7 +89,7 @@ func doCopy(ctx context.Context, name, localPath, remotePath string, toRemote bo if errors.As(err, &ee) && stderr.Len() > 0 { // scp ran and failed: its own stderr (no such file, permission // denied, ...) is a far better error than a bare exit status. - return fmt.Errorf("%s: scp: %s", name, strings.TrimSpace(stderr.String())) + return fmt.Errorf("%s: %s", name, strings.TrimPrefix(strings.TrimSpace(stderr.String()), "scp: ")) } // scp could not be started at all (not installed, not executable). return fmt.Errorf("%s: scp: %w", name, err) diff --git a/internal/core/image.go b/internal/core/image.go index 7cd487e6..357e3dd6 100644 --- a/internal/core/image.go +++ b/internal/core/image.go @@ -10,6 +10,7 @@ import ( "github.com/novusedge/stoat/internal/cloudinit" "github.com/novusedge/stoat/internal/config" + "github.com/novusedge/stoat/internal/coreerr" "github.com/novusedge/stoat/internal/iso" ) @@ -199,7 +200,17 @@ func resolveImage(spec string, remote bool) (image, error) { backend, osName := iso.Infer(f) return image{abs: abs, rel: f, backend: backend, osName: osName}, nil } - return image{}, fmt.Errorf("%w: no catalog entry or local image called %q", ErrNotFound, spec) + return image{}, noImage(spec, files) +} + +// noImage builds the not-found error, suggesting from catalog ids and any +// local image files. +func noImage(spec string, local []string) error { + known := append([]string{}, local...) + for _, e := range iso.Catalog() { + known = append(known, e.ID) + } + return &coreerr.NoImageError{Spec: spec, Known: known} } // apply folds a Spec's BYO overrides onto what the file itself declared, diff --git a/internal/core/images.go b/internal/core/images.go index 25a0d0f5..0ab4ad3d 100644 --- a/internal/core/images.go +++ b/internal/core/images.go @@ -2,7 +2,6 @@ package core import ( "context" - "fmt" "os" "path/filepath" @@ -165,7 +164,7 @@ func DownloadImage(ctx context.Context, id string, progress func(done, total int } } if !found { - return DownloadResult{}, fmt.Errorf("%w: %s", ErrNotFound, id) + return DownloadResult{}, noImage(id, nil) } // Checked up front so an already-cancelled ctx costs no network at all. diff --git a/internal/core/limits.go b/internal/core/limits.go index 1bbe253a..ddfeacb1 100644 --- a/internal/core/limits.go +++ b/internal/core/limits.go @@ -17,6 +17,23 @@ import ( // limit, or would ask the host for memory it does not have. var ErrLimit = errors.New("limit reached") +// LimitError is the RAM refusal from CheckStart. It carries the numbers so a +// machine caller reads them from fields instead of the message. +type LimitError struct { + NeededMB, AvailableMB int + msg string +} + +func (e *LimitError) Error() string { return e.msg } + +func (e *LimitError) Is(target error) bool { return target == ErrLimit } + +// MCPText is Error without the CLI's -y escape hatch, which an agent cannot +// use. +func (e *LimitError) MCPText() string { + return fmt.Sprintf("limit reached: needs %d MB and %d MB is available; lower ram_mb with update, or ask a person to start it", e.NeededMB, e.AvailableMB) +} + // limitsFor resolves the account limits, lowered by the limits of the project // that declared this VM. A VM created by `stoat new` has no project, so the // account limits stand alone. @@ -76,8 +93,12 @@ func CheckStart(v *config.VM, force bool) error { if len(running) > 0 { held = fmt.Sprintf("%d MB already runs (%s)", total, strings.Join(running, ", ")) } - return fmt.Errorf("%w: %s needs %d MB, %s, and limits.max_ram_mb is %d", - ErrLimit, v.Name, v.RAM, held, l.MaxRAMMB) + return &LimitError{ + NeededMB: v.RAM, + AvailableMB: max(l.MaxRAMMB-total, 0), + msg: fmt.Sprintf("%v: %s needs %d MB, %s, and limits.max_ram_mb is %d", + ErrLimit, v.Name, v.RAM, held, l.MaxRAMMB), + } } // The host check runs even with no limits configured. It is the floor that @@ -87,8 +108,12 @@ func CheckStart(v *config.VM, force bool) error { } avail := hostcheck.AvailableMB() if avail > 0 && v.RAM > avail { - return fmt.Errorf("%w: %s needs %d MB and the host has %d MB available; `-y` starts it anyway", - ErrLimit, v.Name, v.RAM, avail) + return &LimitError{ + NeededMB: v.RAM, + AvailableMB: avail, + msg: fmt.Sprintf("%v: %s needs %d MB and the host has %d MB available; `-y` starts it anyway", + ErrLimit, v.Name, v.RAM, avail), + } } return nil } diff --git a/internal/core/vm.go b/internal/core/vm.go index ceef5686..3ad18814 100644 --- a/internal/core/vm.go +++ b/internal/core/vm.go @@ -260,7 +260,7 @@ type VM struct { // broken VM a real error instead of a raw TOML parse message. func load(name string) (*config.VM, error) { if !config.Exists(name) { - return nil, fmt.Errorf("%w: %s", ErrNotFound, name) + return nil, config.NoVM(name) } v, err := config.Load(name) if err != nil { diff --git a/internal/core/wait.go b/internal/core/wait.go index cc16c9ef..ac2f66cd 100644 --- a/internal/core/wait.go +++ b/internal/core/wait.go @@ -42,6 +42,14 @@ func Untils() []Until { return []Until{UntilReachable, UntilApplied, UntilStopped, UntilHealthy} } +func joinUntils() string { + names := make([]string, 0, len(Untils())) + for _, u := range Untils() { + names = append(names, string(u)) + } + return strings.Join(names, ", ") +} + // Valid reports whether u is one of Untils(). Wait calls it before it loads // the VM, so a typo fails with the reason rather than with "not found". func (u Until) Valid() bool { return slices.Contains(Untils(), u) } @@ -79,7 +87,7 @@ const InstallTimeout = 15 * time.Minute // instead of re-polling Get in a loop. func Wait(ctx context.Context, name string, until Until) error { if !until.Valid() { - return fmt.Errorf("%w: unknown Until %q", ErrInvalidSpec, until) + return fmt.Errorf("%w: until must be one of %s, not %q", ErrInvalidSpec, joinUntils(), until) } v, err := load(name) if err != nil { diff --git a/internal/coreerr/coreerr.go b/internal/coreerr/coreerr.go index 4befddf7..e1b098c3 100644 --- a/internal/coreerr/coreerr.go +++ b/internal/coreerr/coreerr.go @@ -6,10 +6,104 @@ // core.ErrXxx caller compiles unchanged. package coreerr -import "errors" +import ( + "errors" + "fmt" + "io/fs" + "strings" +) var ( ErrNotFound = errors.New("not found") ErrInvalidSpec = errors.New("invalid spec") ErrBroken = errors.New("broken vm.toml") ) + +// NoVMError is the one "VM not found" error. Known is called only when the +// message is rendered, because a failed config.Load inside List's scan of +// non-VM directories would otherwise list the data root once per directory. +type NoVMError struct { + Name string + Known func() []string +} + +// Error renders the CLI wording. Text takes the tool that lists VMs, because +// an MCP client has no `stoat ls`. +func (e *NoVMError) Error() string { return e.Text("stoat ls") } + +func (e *NoVMError) Text(list string) string { + msg := fmt.Sprintf("no VM %q", e.Name) + if e.Known != nil { + if s := Suggest(e.Name, e.Known()); s != "" { + msg += fmt.Sprintf(" (did you mean %q?)", s) + } + } + return msg + "; see " + list +} + +// MCPText is Error with the MCP tool that lists VMs. +func (e *NoVMError) MCPText() string { return e.Text("list_vms") } + +// Is matches fs.ErrNotExist too: config.Load used to return the raw stat +// error, and callers that test for absence keep working. +func (e *NoVMError) Is(target error) bool { + return target == ErrNotFound || target == fs.ErrNotExist +} + +// NoImageError is ErrNotFound for an image id that is neither in the catalog +// nor a local file. +type NoImageError struct { + Spec string + Known []string +} + +func (e *NoImageError) Error() string { return e.Text("run stoat images") } + +func (e *NoImageError) Text(list string) string { + msg := fmt.Sprintf("no image %q", e.Spec) + if s := Suggest(e.Spec, e.Known); s != "" { + msg += fmt.Sprintf(" (did you mean %q?)", s) + } + return msg + "; " + list +} + +// MCPText is Error with the MCP tool that lists images. +func (e *NoImageError) MCPText() string { return e.Text("call list_images") } + +func (e *NoImageError) Is(target error) bool { return target == ErrNotFound } + +// Suggest returns the candidate closest to name, or "" when none is close +// enough to be a plausible typo. +func Suggest(name string, candidates []string) string { + best, bestDist := "", len(name)/3+2 + for _, c := range candidates { + d := editDistance(strings.ToLower(name), strings.ToLower(c)) + if strings.Contains(strings.ToLower(c), strings.ToLower(name)) && name != "" { + d = 1 + } + if d < bestDist { + best, bestDist = c, d + } + } + return best +} + +func editDistance(a, b string) int { + prev := make([]int, len(b)+1) + for j := range prev { + prev[j] = j + } + for i := 1; i <= len(a); i++ { + cur := make([]int, len(b)+1) + cur[0] = i + for j := 1; j <= len(b); j++ { + cost := 1 + if a[i-1] == b[j-1] { + cost = 0 + } + cur[j] = min(prev[j]+1, cur[j-1]+1, prev[j-1]+cost) + } + prev = cur + } + return prev[len(b)] +} diff --git a/internal/coreerr/coreerr_test.go b/internal/coreerr/coreerr_test.go new file mode 100644 index 00000000..2c999f5e --- /dev/null +++ b/internal/coreerr/coreerr_test.go @@ -0,0 +1,51 @@ +package coreerr + +import ( + "errors" + "io/fs" + "testing" +) + +func TestSuggest(t *testing.T) { + known := []string{"alpine-standard", "alpine-virt", "debian-13"} + tests := []struct{ name, want string }{ + {"alpine-standrd", "alpine-standard"}, + {"debian13", "debian-13"}, + {"alpine", "alpine-standard"}, + {"windows-11", ""}, + {"", ""}, + } + for _, tt := range tests { + if got := Suggest(tt.name, known); got != tt.want { + t.Errorf("Suggest(%q) = %q, want %q", tt.name, got, tt.want) + } + } +} + +func TestNoVMErrorText(t *testing.T) { + err := &NoVMError{Name: "qol", Known: func() []string { return []string{"qol1", "web"} }} + if got, want := err.Error(), `no VM "qol" (did you mean "qol1"?); see stoat ls`; got != want { + t.Errorf("Error() = %q, want %q", got, want) + } + if got, want := err.MCPText(), `no VM "qol" (did you mean "qol1"?); see list_vms`; got != want { + t.Errorf("MCPText() = %q, want %q", got, want) + } + if !errors.Is(err, ErrNotFound) || !errors.Is(err, fs.ErrNotExist) { + t.Errorf("NoVMError must match ErrNotFound and fs.ErrNotExist") + } + bare := &NoVMError{Name: "nope"} + if got, want := bare.Error(), `no VM "nope"; see stoat ls`; got != want { + t.Errorf("Error() = %q, want %q", got, want) + } +} + +func TestNoImageErrorText(t *testing.T) { + err := &NoImageError{Spec: "alpine-viirt", Known: []string{"alpine-standard", "alpine-virt"}} + want := `no image "alpine-viirt" (did you mean "alpine-virt"?); run stoat images` + if got := err.Error(); got != want { + t.Errorf("Error() = %q, want %q", got, want) + } + if !errors.Is(err, ErrNotFound) { + t.Errorf("NoImageError must match ErrNotFound") + } +} diff --git a/internal/mcpsrv/access.go b/internal/mcpsrv/access.go index fd2b32cf..a3bcf858 100644 --- a/internal/mcpsrv/access.go +++ b/internal/mcpsrv/access.go @@ -2,6 +2,7 @@ package mcpsrv import ( "fmt" + "strings" "github.com/novusedge/stoat/internal/cli/wire" "github.com/novusedge/stoat/internal/config" @@ -46,7 +47,7 @@ func ParseLevel(s string) (Level, error) { return Level(i), nil } } - return 0, fmt.Errorf("invalid agent_access %q: one of none, observe, manage, exec", s) + return 0, badInput("invalid agent_access %q: one of %s", s, strings.Join(levelNames[:], ", ")) } // currentLevel reads vm's agent_access straight off vm.toml. diff --git a/internal/mcpsrv/capabilities_test.go b/internal/mcpsrv/capabilities_test.go index 80b33851..7ec8637c 100644 --- a/internal/mcpsrv/capabilities_test.go +++ b/internal/mcpsrv/capabilities_test.go @@ -96,7 +96,7 @@ func TestCapabilitiesMCPMissingTargetIsNotFound(t *testing.T) { message.WriteString(text.Text) } } - if !strings.Contains(message.String(), "missing") || !strings.Contains(strings.ToLower(message.String()), "not found") { + if !strings.Contains(message.String(), `no VM "missing"`) { t.Errorf("missing target MCP error = %q, want existing not-found message", message.String()) } } diff --git a/internal/mcpsrv/error_contract_test.go b/internal/mcpsrv/error_contract_test.go index 03396eaa..9dffb781 100644 --- a/internal/mcpsrv/error_contract_test.go +++ b/internal/mcpsrv/error_contract_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/novusedge/stoat/internal/cli/wire" "github.com/novusedge/stoat/internal/core" ) @@ -286,3 +287,79 @@ func TestMCPErrorContractEscapedMessages(t *testing.T) { t.Fatalf("metadata message %q differs from redacted human message %q", metadata.Message, first.Message) } } + +func TestMCPErrorSaysMCPToolsNotCLICommands(t *testing.T) { + t.Setenv("STOAT_HOME", t.TempDir()) + res := callTool(t, "vm_status", map[string]any{"vm": "nope"}) + if !res.IsError { + t.Fatal("missing VM did not fail") + } + meta, first := decodeErrorContract(t, res) + if meta.Code != "not_found" || first.Message != `no VM "nope"; see list_vms` || meta.Message != first.Message { + t.Errorf("error = %+v, first = %+v", meta, first) + } + + limit := toolError(&core.LimitError{NeededMB: 4096, AvailableMB: 1500}) + text := limit.Content[0].(*mcp.TextContent).Text + if strings.Contains(text, "-y") || !strings.Contains(text, "lower ram_mb with update, or ask a person to start it") { + t.Errorf("limit message = %q", text) + } + if !strings.Contains(limit.Content[1].(*mcp.TextContent).Text, `"needed_mb":4096`) { + t.Errorf("envelope lacks needed_mb: %v", limit.Content[1]) + } +} + +func TestMCPInputErrorsAreUsage(t *testing.T) { + t.Setenv("STOAT_HOME", t.TempDir()) + // The schema enum rejects a bad level before the handler runs, so the + // handler's own check is exercised directly. + _, err := ParseLevel("root") + if info := wire.MapError(err); info.Code != wire.CodeUsage || !strings.Contains(info.Message, "none, observe, manage, exec") { + t.Errorf("ParseLevel error = %+v, want usage naming the valid levels", info) + } + res := callTool(t, "create", map[string]any{"name": "a", "image": "alpine-virt", "agent_access": "root"}) + if !res.IsError || !strings.Contains(fmt.Sprint(res.Content[0].(*mcp.TextContent).Text), "observe") { + t.Errorf("schema did not reject the level and list the valid ones: %+v", res.Content) + } +} + +func TestMCPToolSchemasListEnums(t *testing.T) { + ctx := context.Background() + srv := New(Options{Version: "test", Limits: DefaultLimits()}) + ct, st := mcp.NewInMemoryTransports() + if _, err := srv.Connect(ctx, st, nil); err != nil { + t.Fatal(err) + } + cs, err := mcp.NewClient(&mcp.Implementation{Name: "test", Version: "0"}, nil).Connect(ctx, ct, nil) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = cs.Close() }) + tools, err := cs.ListTools(ctx, nil) + if err != nil { + t.Fatal(err) + } + want := map[string]string{"wait": "until", "create": "agent_access", "job_output": "stream", "svc": "action", "logs": "which"} + for _, tool := range tools.Tools { + field, ok := want[tool.Name] + if !ok { + continue + } + raw, _ := json.Marshal(tool.InputSchema) + var schema struct { + Properties map[string]struct { + Enum []string `json:"enum"` + } `json:"properties"` + } + if err := json.Unmarshal(raw, &schema); err != nil { + t.Fatal(err) + } + if len(schema.Properties[field].Enum) == 0 { + t.Errorf("%s.%s has no enum in %s", tool.Name, field, raw) + } + delete(want, tool.Name) + } + if len(want) != 0 { + t.Errorf("tools not found: %v", want) + } +} diff --git a/internal/mcpsrv/guards.go b/internal/mcpsrv/guards.go index c4227cb4..9d6b9bde 100644 --- a/internal/mcpsrv/guards.go +++ b/internal/mcpsrv/guards.go @@ -32,6 +32,12 @@ var ( // grants an arbitrary host directory into a guest read-write. var forbiddenPatchKeys = []string{"share", "image", "base", "iso", "console_password"} +// badInput marks an error as the caller's mistake, so it reaches the client +// as "usage" and not "internal". +func badInput(format string, args ...any) error { + return wire.WithSentinel(fmt.Errorf(format, args...), wire.ErrBadInput) +} + // checkVMName keeps an operation inside the data root: the name becomes a // directory there. func checkVMName(name string) (string, error) { @@ -43,13 +49,13 @@ func checkVMName(name string) (string, error) { func checkImageID(image string) (string, error) { if strings.TrimSpace(image) == "" { - return "", fmt.Errorf("image id is required") + return "", badInput("image id is required") } if strings.ContainsAny(image, `/\`) || strings.HasPrefix(image, "~") { - return "", fmt.Errorf("image %q looks like a path; only catalog image ids are accepted, run list_images to see them", image) + return "", badInput("image %q looks like a path; only catalog image ids are accepted, run list_images to see them", image) } if !imageIDRE.MatchString(image) { - return "", fmt.Errorf("image id %q is not a valid catalog id", image) + return "", badInput("image id %q is not a valid catalog id", image) } return image, nil } @@ -71,10 +77,10 @@ func sharedDir(vm string) (string, error) { // "work". func checkHostPath(path, vm string) (string, error) { if strings.TrimSpace(path) == "" { - return "", fmt.Errorf("path is required") + return "", badInput("path is required") } if strings.ContainsRune(path, 0) { - return "", fmt.Errorf("path contains a null byte") + return "", badInput("path contains a null byte") } sandbox, err := sharedDir(vm) if err != nil { @@ -107,7 +113,7 @@ func resolveExisting(p string) (string, error) { } parent := filepath.Dir(cur) if parent == cur { - return "", fmt.Errorf("path %q cannot be resolved", p) + return "", badInput("path %q cannot be resolved", p) } rest = filepath.Join(filepath.Base(cur), rest) cur = parent @@ -121,10 +127,10 @@ func resolveExisting(p string) (string, error) { func checkFlagFree(values []string, what string) error { for _, v := range values { if strings.TrimSpace(v) == "" { - return fmt.Errorf("%s contains an empty value", what) + return badInput("%s contains an empty value", what) } if strings.HasPrefix(v, "-") { - return fmt.Errorf("%s value %q may not start with a dash", what, v) + return badInput("%s value %q may not start with a dash", what, v) } } return nil @@ -150,18 +156,18 @@ func stripForbidden(patch map[string]any) map[string]any { // "feature/topic". func checkIndexName(ref string) (string, string, error) { if strings.TrimSpace(ref) == "" { - return "", "", fmt.Errorf("recipe name is required") + return "", "", badInput("recipe name is required") } name, gitRef, hasRef := strings.Cut(ref, "@") if strings.Contains(gitRef, "@") { - return "", "", fmt.Errorf("invalid recipe name %q: at most one @ref", ref) + return "", "", badInput("invalid recipe name %q: at most one @ref", ref) } if !indexNameRE.MatchString(name) { - return "", "", fmt.Errorf("invalid recipe name %q: index names only, not a URL, and it must match %s", ref, indexNameRE) + return "", "", badInput("invalid recipe name %q: index names only, not a URL, and it must match %s", ref, indexNameRE) } if hasRef { if err := checkGitRef(gitRef); err != nil { - return "", "", fmt.Errorf("invalid ref %q for recipe %q: %w", gitRef, name, err) + return "", "", badInput("invalid ref %q for recipe %q: %w", gitRef, name, err) } } return name, gitRef, nil @@ -172,20 +178,20 @@ func checkIndexName(ref string) (string, string, error) { // git itself refuses. func checkGitRef(ref string) error { if !gitRefRE.MatchString(ref) { - return fmt.Errorf("must match %s", gitRefRE) + return badInput("must match %s", gitRefRE) } if strings.Contains(ref, "..") { - return fmt.Errorf("contains a traversal") + return badInput("contains a traversal") } for _, part := range strings.Split(ref, "/") { if part == "" { - return fmt.Errorf("has an empty path component") + return badInput("has an empty path component") } if strings.HasPrefix(part, ".") || strings.HasSuffix(part, ".") { - return fmt.Errorf("component %q starts or ends with a dot", part) + return badInput("component %q starts or ends with a dot", part) } if strings.HasSuffix(part, ".lock") { - return fmt.Errorf("component %q ends with .lock", part) + return badInput("component %q ends with .lock", part) } } return nil @@ -193,7 +199,7 @@ func checkGitRef(ref string) error { func checkParamName(name string) (string, error) { if !paramNameRE.MatchString(name) { - return "", fmt.Errorf("invalid param name %q: must match %s", name, paramNameRE) + return "", badInput("invalid param name %q: must match %s", name, paramNameRE) } return name, nil } @@ -219,7 +225,7 @@ func checkGuestPath(path string) (string, error) { // boundary. func checkSvcName(name string) (string, error) { if !svcNameRE.MatchString(name) { - return "", fmt.Errorf("invalid service name %q: must match %s", name, svcNameRE) + return "", badInput("invalid service name %q: must match %s", name, svcNameRE) } return name, nil } @@ -230,7 +236,7 @@ func checkSvcName(name string) (string, error) { // a variable name. func checkEnvName(name string) (string, error) { if !envNameRE.MatchString(name) { - return "", fmt.Errorf("invalid env name %q: must match %s", name, envNameRE) + return "", badInput("invalid env name %q: must match %s", name, envNameRE) } return name, nil } diff --git a/internal/mcpsrv/server.go b/internal/mcpsrv/server.go index bb76fe32..05ec8e95 100644 --- a/internal/mcpsrv/server.go +++ b/internal/mcpsrv/server.go @@ -10,9 +10,12 @@ import ( "net/http" "os" "reflect" + "strings" + "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/novusedge/stoat/internal/cli/wire" + "github.com/novusedge/stoat/internal/core" "github.com/novusedge/stoat/internal/logx" "github.com/novusedge/stoat/internal/project" ) @@ -129,6 +132,9 @@ func annotationsFor(c class) *mcp.ToolAnnotations { // import) assigns this tool's name; TestAnnotationsMatchTable checks that. func register[In, Out any](server *mcp.Server, name string, c class, description string, h func(context.Context, In) (Out, error)) { tool := &mcp.Tool{Name: name, Description: description, Annotations: annotationsFor(c)} + if s := inputSchema[In](); s != nil { + tool.InputSchema = s + } mcp.AddTool(server, tool, func(ctx context.Context, _ *mcp.CallToolRequest, in In) (*mcp.CallToolResult, Out, error) { out, err := h(ctx, in) if err != nil { @@ -138,6 +144,46 @@ func register[In, Out any](server *mcp.Server, name string, c class, description }) } +// inputEnums lists the accepted values of each closed-set string input, keyed +// by input type and JSON field. The jsonschema struct tag carries only a +// description, so the enum goes onto the schema after it is generated. +var inputEnums = map[string][]string{ + "createIn.mode": {"live", "disk"}, + "createIn.agent_access": levelNames[:], + "updateIn.agent_access": levelNames[:], + "waitIn.until": {string(core.UntilReachable), string(core.UntilApplied), string(core.UntilStopped)}, + "jobOutputIn.stream": {"stdout", "stderr"}, + "svcIn.action": svcActions, + "logsIn.which": {string(core.WhichConsole), string(core.WhichApply)}, +} + +// inputSchema is the schema the SDK would generate for In, plus inputEnums. +// It returns nil for an In with no enum, which leaves the SDK to generate it. +func inputSchema[In any]() *jsonschema.Schema { + typ := reflect.TypeFor[In]().Name() + var schema *jsonschema.Schema + for key, values := range inputEnums { + owner, field, _ := strings.Cut(key, ".") + if owner != typ { + continue + } + if schema == nil { + var err error + if schema, err = jsonschema.For[In](nil); err != nil { + panic(fmt.Sprintf("input schema for %s: %v", typ, err)) + } + } + prop := schema.Properties[field] + if prop == nil { + panic(fmt.Sprintf("inputEnums: %s has no field %q", typ, field)) + } + for _, v := range values { + prop.Enum = append(prop.Enum, v) + } + } + return schema +} + // zeroOut builds Out's zero value with every nil map and slice field // replaced by an empty one. The SDK validates a tool's output against its // generated schema even on the error path, and a field without an @@ -177,6 +223,11 @@ func fillEmpty(v reflect.Value) { // failure and a user reading the CLI see one message. func toolError(err error) *mcp.CallToolResult { info := wire.MapError(err) + // An error that names a CLI command for the fix has an MCP spelling. + var mcpText interface{ MCPText() string } + if errors.As(err, &mcpText) { + info.Message = mcpText.MCPText() + } // Keep the redaction that receiving middleware applies to the human block // before copying the message into metadata and the JSON fallback. info.Message = redactText(info.Message) diff --git a/internal/mcpsrv/tools_exec.go b/internal/mcpsrv/tools_exec.go index ed8838c4..c45bf1a5 100644 --- a/internal/mcpsrv/tools_exec.go +++ b/internal/mcpsrv/tools_exec.go @@ -50,7 +50,7 @@ type jobOutputIn struct { type jobKillIn struct { VM string `json:"vm" jsonschema:"name of the VM"` JobID string `json:"job_id" jsonschema:"job id returned by exec_bg"` - Signal string `json:"signal,omitempty" jsonschema:"signal name without the SIG prefix; TERM is the default"` + Signal string `json:"signal,omitempty" jsonschema:"signal name such as TERM, HUP or KILL, with or without the SIG prefix; TERM is the default"` } var signalRE = regexp.MustCompile(`^[A-Z]{2,10}[0-9]?$`) @@ -97,7 +97,7 @@ func (s *srv) registerExec(server *mcp.Server) { return wire.CommandResult{}, err } if len(in.Argv) == 0 { - return wire.CommandResult{}, fmt.Errorf("argv is required") + return wire.CommandResult{}, badInput("argv is required") } argv, err := envArgv(in.Env, in.CWD, in.Argv) if err != nil { @@ -129,7 +129,7 @@ func (s *srv) registerExec(server *mcp.Server) { return wire.JobStarted{}, err } if len(in.Argv) == 0 { - return wire.JobStarted{}, fmt.Errorf("argv is required") + return wire.JobStarted{}, badInput("argv is required") } argv, err := envArgv(in.Env, in.CWD, in.Argv) if err != nil { @@ -233,7 +233,7 @@ func (s *srv) registerExec(server *mcp.Server) { case "stderr": file = "err" default: - return wire.FileContent{}, fmt.Errorf("invalid stream %q: stdout or stderr", in.Stream) + return wire.FileContent{}, badInput("invalid stream %q: stdout or stderr", in.Stream) } return readGuestFile(ctx, v, path.Join(j.Dir, file), in.Offset, in.MaxBytes) }) @@ -254,7 +254,7 @@ func (s *srv) registerExec(server *mcp.Server) { sig = "TERM" } if !signalRE.MatchString(sig) { - return wire.CommandResult{}, fmt.Errorf("invalid signal %q: a name such as TERM or KILL, without the SIG prefix", sig) + return wire.CommandResult{}, badInput("invalid signal %q: a name such as TERM, HUP or KILL", sig) } pidOut, _, code, err := sshx.Run(ctx, v, false, []string{"cat", path.Join(j.Dir, "pid")}, nil) if err != nil { diff --git a/internal/mcpsrv/tools_guest.go b/internal/mcpsrv/tools_guest.go index 54925e02..7f2a91a8 100644 --- a/internal/mcpsrv/tools_guest.go +++ b/internal/mcpsrv/tools_guest.go @@ -330,7 +330,7 @@ func (s *srv) registerGuestWrite(server *mcp.Server) { mode = "0644" } if !modeRE.MatchString(mode) { - return wire.CommandResult{}, fmt.Errorf("invalid mode %q: three or four octal digits", mode) + return wire.CommandResult{}, badInput("invalid mode %q: three or four octal digits", mode) } // tee rather than a redirect: a redirect is shell syntax the // tool would have to build around the path. @@ -367,21 +367,21 @@ func (s *srv) registerGuestWrite(server *mcp.Server) { return wire.CommandResult{}, err } if len(in.Packages) == 0 { - return wire.CommandResult{}, fmt.Errorf("packages is required") + return wire.CommandResult{}, badInput("packages is required") } if err := checkFlagFree(in.Packages, "packages"); err != nil { return wire.CommandResult{}, err } os, ok := guest.Lookup(v.OS) if !ok { - return wire.CommandResult{}, fmt.Errorf("unknown guest %q; run stoat guest ls", v.OS) + return wire.CommandResult{}, fmt.Errorf("unknown guest %q; call list_guests", v.OS) } // pkg.setup is the distro's own index refresh and carries no // tool input, so running it as the guest file wrote it is safe. - if _, _, code, err := sshx.Run(ctx, v, true, []string{"sh", "-c", os.Pkg.Setup}, nil); err != nil { + if _, errb, code, err := sshx.Run(ctx, v, true, []string{"sh", "-c", os.Pkg.Setup}, nil); err != nil { return wire.CommandResult{}, err } else if code != 0 { - return wire.CommandResult{}, fmt.Errorf("%s: package index refresh exited %d", v.Name, code) + return wire.CommandResult{}, fmt.Errorf("%s: package index refresh exited %d: %s", v.Name, code, strings.TrimSpace(string(errb))) } return runToResult(ctx, v, true, append(append([]string{}, os.Pkg.Install...), in.Packages...)) }) @@ -394,7 +394,7 @@ func (s *srv) registerGuestWrite(server *mcp.Server) { return wire.CommandResult{}, err } if !slices.Contains(svcActions, in.Action) { - return wire.CommandResult{}, fmt.Errorf("invalid action %q: one of enable, start, stop, restart", in.Action) + return wire.CommandResult{}, badInput("invalid action %q: one of %s", in.Action, strings.Join(svcActions, ", ")) } name, err := checkSvcName(in.Name) if err != nil { @@ -433,7 +433,7 @@ func (s *srv) registerGuestWrite(server *mcp.Server) { func svcArgv(v *config.VM, action, name string) ([]string, error) { os, ok := guest.Lookup(v.OS) if !ok { - return nil, fmt.Errorf("unknown guest %q; run stoat guest ls", v.OS) + return nil, fmt.Errorf("unknown guest %q; call list_guests", v.OS) } tmpl := os.Svc.Get(action) if tmpl == "" { diff --git a/internal/mcpsrv/tools_recipe.go b/internal/mcpsrv/tools_recipe.go index 17257315..009c69e8 100644 --- a/internal/mcpsrv/tools_recipe.go +++ b/internal/mcpsrv/tools_recipe.go @@ -2,7 +2,6 @@ package mcpsrv import ( "context" - "fmt" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/novusedge/stoat/internal/cli/wire" @@ -53,7 +52,7 @@ func (s *srv) registerRecipe(server *mcp.Server) { return wire.RecipeCatalog{}, err } if ref != "" { - return wire.RecipeCatalog{}, fmt.Errorf("update_recipe takes a plain recipe name; pin a ref with add_recipe") + return wire.RecipeCatalog{}, badInput("update_recipe takes a plain recipe name; pin a ref with add_recipe") } name = n } @@ -71,7 +70,7 @@ func (s *srv) registerRecipe(server *mcp.Server) { return wire.RecipeCatalog{}, err } if ref != "" { - return wire.RecipeCatalog{}, fmt.Errorf("remove_recipe takes a plain recipe name, without @ref") + return wire.RecipeCatalog{}, badInput("remove_recipe takes a plain recipe name, without @ref") } // force is deliberately absent rather than false-by-default: a // parameter that exists is eventually reachable.