diff --git a/CLAUDE.md b/CLAUDE.md index 36dc7cb..7f08e14 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,44 +8,43 @@ mirrors these commands. On any command / flag change, run the protocol below β€” the concrete website-regen steps live in "Keeping the Website Docs in Sync" near the end of this file. This repo is one of five in the product mesh. -### Repo map - -| repo | path | role | public? | changes that ripple across the mesh | -|---|---|---|---|---| -| **fc** | `../fc` | control-plane β€” **source of truth** | πŸ”’ private | HTTP API, wire/JSON fields, error shapes, lifecycle/state, limits/quotas, behavior | -| **fc-sdk** | `../fc-sdk` | TypeScript SDK **+ `examples/`** | 🌐 public | public SDK methods, wire types, example apps | -| **createos-cli** | `../createos-cli` | Go CLI | 🌐 public | commands, flags, help/UX text | -| **website-04** | `../website-04` (`content/docs/Sandbox`) | public docs | 🌐 public | REST / SDK / CLI reference + concept pages | -| **createos-plugin** | `../createos-plugin` | Claude Code plugin over the `createos` CLI | 🌐 public | skills, slash commands, hooks | - -### What counts as a shared surface - -HTTP endpoint or method Β· wire or JSON field Β· error shape Β· sandbox -lifecycle/state Β· limit or quota Β· CLI command or flag Β· public SDK method Β· -documented behavior. A change confined to internals β€” refactor, comment, -private helper, test-only β€” is **not** a shared surface, so skip the mesh for it. - -### Protocol β€” run before finalizing a shared-surface change - -1. **Classify origin.** `fc` is the source of truth; SDK / CLI / docs / plugin - are downstream consumers. A downstream change that implies a backend change - (new field, new endpoint) β†’ surface it to the user; never invent server - behavior inside a client. -2. **Search every sibling** for the touched symbol / endpoint / flag β€” - `semble search` first, then `rg`. -3. **Build a status matrix** per sibling: `already-present` Β· - `missing-needs-update` Β· `n/a`. **Flag the already-present ones to the user** - ("already exists in fc-sdk + docs"). Never silently duplicate a change that is - already there β€” that is the whole point of this check. -4. **`fc` β†’ any public repo is a leak-guard boundary (security).** Strip private - implementation, security internals, infra, threat-model notes, and - internal-only tooling (`fcctl`, host filesystem paths, mTLS/CA internals) - before anything lands in a public repo. Respect each public repo's own wording - rules (e.g. `fc-sdk/AGENTS.md` forbids the word "VM"). Report the proposed diff - and **ask for approval before landing any public edit** β€” never auto-write - across the boundary. -5. **Use the `sync-docs` skill** to execute SDK / CLI / website reconciliation - against upstream `fc` where it applies. +| repo | role | public? | +|---|---|---| +| **fc** | control-plane β€” source of truth | πŸ”’ private | +| **fc-sdk** | TypeScript SDK + `examples/` | 🌐 public | +| **createos-cli** | Go CLI | 🌐 public | +| **website-04** (`content/docs/Sandbox`) | public docs | 🌐 public | +| **createos-plugin** | Claude Code plugin over the `createos` CLI | 🌐 public | + +**Shared surface** = HTTP endpoint or method Β· wire or JSON field Β· error shape Β· +sandbox lifecycle/state Β· limit or quota Β· CLI command or flag Β· public SDK +method Β· documented behavior. Internals β€” refactor, private helper, test-only β€” +are not, so skip the mesh for them. + +On a shared-surface change: search every sibling for the touched symbol +(`semble search` first, then `rg`), report a per-sibling matrix +(`already-present` / `missing-needs-update` / `n/a`) before finalizing, and never +silently duplicate what is already there. Origin matters β€” `fc` is upstream; a +downstream change implying new server behavior goes to the user, never invented +inside a client. **`fc` β†’ any public repo is a leak-guard boundary:** strip +private implementation, security internals, infra, threat-model notes, and +internal-only tooling (`fcctl`, host filesystem paths, mTLS/CA internals), +respect each public repo's own wording rules (e.g. `fc-sdk/AGENTS.md` forbids +the word "VM"), and get approval before landing any public edit. The `sync-docs` +skill executes SDK / CLI / website reconciliation against upstream `fc`. + +### Downstream reference implementations (not mesh-protocol members) + +Two public repos shell out to this CLI and are worth checking before a +command/flag/help-text change ships, even though neither owns shared +surface and both sit outside the formal 5-repo protocol above: + +- **createos-plugin** (`../createos-plugin/createos-sandbox`) β€” the primary + CLI wrapper; most exposed, since it parses `createos` stdout. +- **createos-sandbox-ghar** (`../createos-sandbox-ghar`) β€” its + `.github/workflows/bump-runner.yml` daily job shells out to this CLI to + rebuild the `ghar-runner` rootfs template; a flag/output change here can + break that job silently. ## Project Structure @@ -91,6 +90,68 @@ The API has two response shapes β€” use the right one: 2. Match field names exactly to the JSON response β€” use nullable pointers (`*string`) for fields that can be `null` 3. For errors, return `ParseAPIError(resp.StatusCode(), resp.Body())` β€” never `fmt.Errorf("API error %d: %s", ...)` +## Machine-readable Output + +Decisions and rejected alternatives: `docs/decisions.md`. + +### Every mutation must emit JSON + +A `sandbox` command that creates, changes, or deletes something calls +`renderResult` (`cmd/sandbox/jsonout.go`), never a bare `pterm.Success`: + +```go +renderResult(c, "created", map[string]any{ + "id": resp.ID, + "name": str(resp.Name), +}, func() { + pterm.Success.Printfln("Created %s", resp.ID) +}) +``` + +The human renderer goes in the closure; it runs only in table mode. Rules: + +- `action` names what happened (past tense: `created`, `paused`, `disk_attached`). +- Field names match the read commands β€” a caller diffs `create` against `get`. +- Nullable API pointers go through `str()` so a key is always present. +- Wrap the fields in `withResponse(resp, …)` when the API returns a struct, so + callers also get everything the server sent. Curated keys win on collision. +- Never branch the API call on output format. One call path serves both; the + spinner already writes to stderr. Two paths drift β€” that is how a JSON-mode + `fork` once skipped its status check and reported a failed fork as success. +- Error strings in results go through `api.UserMessageVerbose(err)`, never + `err.Error()` β€” raw Go errors leak syscall detail and local paths into JSON + just as readily as into the terminal. +- Batch commands return a `results` array with one entry per ref, plus + `deleted` / `failed` counts. An exit code cannot express a partial batch. +- Interactive streams (`shell`, `sync`, `editor`, `exec --stream`, + `template logs`) stay text-only. Blocking commands that have a result + (`tunnel`, `vpn up`) emit it *before* they block. + +### Two JSON checks β€” pick the right one + +| Helper | Use for | +|---|---| +| `output.IsJSON` / `output.Render` | Normal commands. True when `--output json` **or** stdout is not a TTY. | +| `output.IsJSONExplicit` | Commands whose stdout **is** the payload (`exec`). Only true when the user typed `--output json`, so `exec … > file` still writes raw bytes. | + +### Streams + +- **stdout** β€” data only. In JSON mode it holds exactly one document; the + root `Before` hook redirects all pterm output to stderr to guarantee it. +- **stderr** β€” errors, progress, hints, spinners. +- Errors are formatted in `main.go`: a JSON envelope on stdout in JSON mode, + otherwise plain text on stderr. Add new status codes to + `api.APIError.Code()`, which produces the envelope's `code` slug. +- Colour is disabled automatically for non-TTY stdout and for `NO_COLOR`. + +### Global flags + +`internal/cliargs.Hoist` rewrites argv before `app.Run` so global flags work +after the subcommand. **When adding or renaming a global flag in +`root.go`, add it to `globalStringFlags` / `globalBoolFlags` too** β€” a +missing entry silently reintroduces the "flag provided but not defined" +error. Tokens after a bare `--` are never hoisted. + ## Error Handling ### API errors diff --git a/README.md b/README.md index 25445e5..de12c3f 100644 --- a/README.md +++ b/README.md @@ -491,6 +491,8 @@ createos sandbox list --status paused --quiet | xargs createos sandbox rm --forc createos sandbox get createos sandbox exec my-box -- uname -a createos sandbox exec my-box --stream -- pip install requests +echo "hello" | createos sandbox exec my-box -- cat +createos sandbox exec my-box --stdin ./setup.sh -- bash createos sandbox shell my-box createos sandbox shell my-box --ssh createos sandbox push my-box ./script.py /root/script.py @@ -595,6 +597,90 @@ createos projects get --project --output json createos environments list --project -o json ``` +### Global flags work anywhere on the line + +`--output`, `--debug`, `--api-url`, `--api-key`, `--sandbox-api-url`, and +`--sandbox-gateway` can go before or after the subcommand β€” both of these +are the same command: + +```bash +createos --output json sandbox create +createos sandbox create --output json +``` + +Anything after a bare `--` is passed through untouched, so +`createos sandbox exec my-box -- ./ci.sh --debug` still sends `--debug` to +your script rather than to the CLI. + +### Sandbox commands that change something + +Every `sandbox` subcommand that creates, changes, or deletes something also +reports its result as JSON. Each object carries an `action` naming what +happened, plus the ids needed to chain the next command: + +```bash +$ createos sandbox create --output json +{ + "action": "created", + "id": "sb-01k...", + "name": "quiet-lake-4821", + "shape": "s-2vcpu-4gb", + "rootfs": "devbox", + "ip": "10.0.4.19", + "ingress_url": "https://-sb-01k....sb.createos.sh", + "shell_command": "createos sandbox shell sb-01k..." +} + +# chain it +ID=$(createos sandbox create -o json | jq -r .id) +createos sandbox exec "$ID" -- uname -a +createos sandbox rm "$ID" --force +``` + +Batch commands (`rm`, `disk rm`, `network rm`, `template rm`) return one +entry per reference you passed, so you can tell which ones actually went: + +```json +{ + "action": "deleted", + "results": [ + { "ref": "my-box", "id": "sb-01k...", "deleted": true }, + { "ref": "typo-box", "deleted": false, "error": "no sandbox named typo-box" } + ], + "deleted": 1, + "failed": 1 +} +``` + +`sandbox shell`, `sandbox sync`, `sandbox editor`, and `sandbox exec --stream` +stay human-readable β€” they are interactive streams with no single result to +report. `sandbox tunnel` and `sandbox vpn up` print their JSON result when the +connection comes up, before they block. + +### Errors + +Errors go to **stderr**, so `2>/dev/null` and pipes behave. In JSON mode the +error is a machine-readable envelope on stdout: + +```bash +$ createos sandbox get nope --output json +{ + "error": { + "code": "not_found", + "message": "no sandbox named nope" + } +} +``` + +Codes: `bad_request`, `unauthorized`, `forbidden`, `not_found`, `conflict`, +`rate_limited`, `server_error`, `api_error`, `error`. The exit code is 1 for +any failure. + +### Colour + +ANSI colour is switched off automatically when stdout is not a terminal, and +whenever `NO_COLOR` is set. CI logs stay greppable without any flag. + ## Options | Flag | Description | diff --git a/cmd/root/root.go b/cmd/root/root.go index c640814..1f9831c 100644 --- a/cmd/root/root.go +++ b/cmd/root/root.go @@ -4,8 +4,10 @@ package root import ( "fmt" "net/url" + "os" "time" + "github.com/pterm/pterm" "github.com/urfave/cli/v2" "github.com/NodeOps-app/createos-cli/cmd/ask" @@ -36,6 +38,7 @@ import ( internaloauth "github.com/NodeOps-app/createos-cli/internal/oauth" "github.com/NodeOps-app/createos-cli/internal/output" "github.com/NodeOps-app/createos-cli/internal/pkg/version" + "github.com/NodeOps-app/createos-cli/internal/terminal" ) // NewApp creates and configures the root CLI application. @@ -89,6 +92,23 @@ func NewApp() *cli.App { Before: func(c *cli.Context) error { // Store the output format in metadata c.App.Metadata[output.FormatKey] = output.DetectFormat(c) + c.App.Metadata[output.FormatExplicitKey] = c.String("output") != "" + + // One choke point for colour: a pipe, a CI log, or NO_COLOR + // must never receive ANSI escapes. pterm styles every helper + // through this global, so disabling it here covers every + // command at once. + if os.Getenv("NO_COLOR") != "" || !terminal.IsInteractive() { + pterm.DisableStyling() + } + + // In JSON mode stdout carries one machine-readable document and + // nothing else. Sending every pterm print to stderr keeps that + // true even for commands that still narrate progress, so a + // consumer never has to strip prose out of the stream. + if output.IsJSON(c) { + pterm.SetDefaultOutput(os.Stderr) + } // Skip auth for --help / -h on any command for _, a := range c.Args().Slice() { diff --git a/cmd/sandbox/create.go b/cmd/sandbox/create.go index 5d2e022..3b0e2ba 100644 --- a/cmd/sandbox/create.go +++ b/cmd/sandbox/create.go @@ -10,7 +10,6 @@ import ( "github.com/urfave/cli/v2" "github.com/NodeOps-app/createos-cli/internal/api" - "github.com/NodeOps-app/createos-cli/internal/output" ) func newCreateCommand() *cli.Command { @@ -196,15 +195,8 @@ func runCreate(c *cli.Context) error { req.AutoPauseAfterSeconds = autoPauseSecs } - if output.IsJSON(c) { - jsonResp, jsonErr := client.CreateSandbox(c.Context, req) - if jsonErr != nil { - return jsonErr - } - output.Render(c, jsonResp, func() {}) - return nil - } - + // One call path for both formats: the spinner writes to stderr, so it + // cannot corrupt a JSON document on stdout. spinner, _ := pterm.DefaultSpinner.Start("Creating sandbox…") //nolint:errcheck resp, err := client.CreateSandbox(c.Context, req) if err != nil { @@ -213,7 +205,15 @@ func runCreate(c *cli.Context) error { } spinner.Success("Sandbox is ready") - printCreateResult(resp) + renderResult(c, "created", withResponse(resp, map[string]any{ + "id": resp.ID, + "name": str(resp.Name), + "shape": resp.Shape, + "rootfs": str(resp.Rootfs), + "ip": resp.IP, + "ingress_url": resp.IngressURLTemplate, + "shell_command": fmt.Sprintf("createos sandbox shell %s", resp.ID), + }), func() { printCreateResult(resp) }) return nil } diff --git a/cmd/sandbox/devices.go b/cmd/sandbox/devices.go index d0b2e3b..fac5de3 100644 --- a/cmd/sandbox/devices.go +++ b/cmd/sandbox/devices.go @@ -16,7 +16,6 @@ import ( "github.com/urfave/cli/v2" "github.com/NodeOps-app/createos-cli/internal/api" - "github.com/NodeOps-app/createos-cli/internal/output" ) // deviceState is what we persist locally after `devices register`. The @@ -204,9 +203,15 @@ func runDeviceRegister(c *cli.Context) error { if err := saveDeviceState(st); err != nil { return fmt.Errorf("could not save device state: %w", err) } - pterm.Success.Printfln("Registered %q (%s)", view.Name, view.ClientIP) - pterm.Println(pterm.Gray(" Attach this device to a network in the UI, then:")) - pterm.Println(pterm.Gray(" createos sb vpn up")) + renderResult(c, "device_registered", map[string]any{ + "id": view.ID, + "name": view.Name, + "client_ip": view.ClientIP, + }, func() { + pterm.Success.Printfln("Registered %q (%s)", view.Name, view.ClientIP) + pterm.Println(pterm.Gray(" Attach this device to a network in the UI, then:")) + pterm.Println(pterm.Gray(" createos sb vpn up")) + }) return nil } @@ -237,7 +242,12 @@ func runDeviceUnregister(c *cli.Context) error { if err := clearDeviceState(); err != nil { return fmt.Errorf("clear local state: %w", err) } - pterm.Success.Printfln("Unregistered %q.", st.Name) + renderResult(c, "device_unregistered", map[string]any{ + "id": st.DeviceID, + "name": st.Name, + }, func() { + pterm.Success.Printfln("Unregistered %q.", st.Name) + }) return nil } @@ -364,15 +374,24 @@ func runDeviceRemove(c *cli.Context) error { if err := client.DeleteDevice(ctx, target.ID); err != nil { return err } - pterm.Success.Printfln("Removed %q (%s).", target.Name, target.ClientIP) - + clearedLocal := false // If we just deleted the row backing this machine's device.json, // scrub local state too so `vpn up` doesn't loop against a ghost id. if local, _ := loadDeviceState(); local != nil && local.DeviceID == target.ID { //nolint:errcheck _ = clearDeviceState() //nolint:errcheck - pterm.Println(pterm.Gray(" (also cleared local device.json for this machine)")) + clearedLocal = true } + + renderResult(c, "device_removed", map[string]any{ + "id": target.ID, + "name": target.Name, + "client_ip": target.ClientIP, + "cleared_local_state": clearedLocal, + }, func() { + pterm.Success.Printfln("Removed %q (%s).", target.Name, target.ClientIP) + if clearedLocal { + pterm.Println(pterm.Gray(" (also cleared local device.json for this machine)")) + } + }) return nil } - -var _ = output.Render diff --git a/cmd/sandbox/disk.go b/cmd/sandbox/disk.go index 3650cbe..2dd497c 100644 --- a/cmd/sandbox/disk.go +++ b/cmd/sandbox/disk.go @@ -156,8 +156,13 @@ func runDiskCreate(c *cli.Context) error { return err } spinner.Success(fmt.Sprintf("Registered disk %s (%s)", d.Name, d.ID)) - pterm.Println(pterm.Gray(" Attach it at create time: createos sandbox create --disk " + d.Name + ":/mnt/data")) - pterm.Println(pterm.Gray(" Or live-attach it later: createos sandbox disk attach " + d.Name + " /mnt/data")) + renderResult(c, "disk_created", map[string]any{ + "id": d.ID, + "name": d.Name, + }, func() { + pterm.Println(pterm.Gray(" Attach it at create time: createos sandbox create --disk " + d.Name + ":/mnt/data")) + pterm.Println(pterm.Gray(" Or live-attach it later: createos sandbox disk attach " + d.Name + " /mnt/data")) + }) return nil } @@ -315,14 +320,25 @@ func runDiskRm(c *cli.Context) error { } } failed := 0 + results := make([]map[string]any, 0, len(refs)) for _, ref := range refs { if err := deleteDiskCascade(c, client, ref); err != nil { - pterm.Error.Printfln("%s: %s", ref, api.UserMessageVerbose(err)) + // Sanitized in the JSON result too β€” a raw Go error leaks + // syscall detail and local paths whichever stream it lands on. + msg := api.UserMessageVerbose(err) + pterm.Error.Printfln("%s: %s", ref, msg) + results = append(results, map[string]any{"ref": ref, "deleted": false, "error": msg}) failed++ continue } + results = append(results, map[string]any{"ref": ref, "deleted": true}) pterm.Success.Printfln("Deleted disk %s", ref) } + renderResult(c, "disk_deleted", map[string]any{ + "results": results, + "deleted": len(results) - failed, + "failed": failed, + }, func() {}) if failed > 0 { return fmt.Errorf("%d of %d deletes failed", failed, len(refs)) } @@ -497,7 +513,13 @@ func runDiskAttach(c *cli.Context) error { return err } spinner.Success(fmt.Sprintf("Attached %s β†’ %s:%s", diskRef, refLabel(sandboxRef, sandboxID), mountPath)) - pterm.Println(pterm.Gray(" The mount appears inside the sandbox within a few seconds.")) + renderResult(c, "disk_attached", map[string]any{ + "disk": diskRef, + "sandbox_id": sandboxID, + "mount_path": mountPath, + }, func() { + pterm.Println(pterm.Gray(" The mount appears inside the sandbox within a few seconds.")) + }) return nil } @@ -609,7 +631,13 @@ func runDiskDetach(c *cli.Context) error { if err := client.DetachDisk(c.Context, sandboxID, diskRef, mountPath); err != nil { return err } - pterm.Success.Printfln("Detached %s from %s at %s", diskRef, refLabel(sandboxRef, sandboxID), mountPath) + renderResult(c, "disk_detached", map[string]any{ + "disk": diskRef, + "sandbox_id": sandboxID, + "mount_path": mountPath, + }, func() { + pterm.Success.Printfln("Detached %s from %s at %s", diskRef, refLabel(sandboxRef, sandboxID), mountPath) + }) return nil } diff --git a/cmd/sandbox/edit.go b/cmd/sandbox/edit.go index 203aac9..7c435bb 100644 --- a/cmd/sandbox/edit.go +++ b/cmd/sandbox/edit.go @@ -322,15 +322,21 @@ func applyIngressFlag(c *cli.Context, client *api.SandboxClient, label, id, valu if err != nil { return err } - if target { - pterm.Success.Printfln("Public URL is on for %s", refLabel(label, id)) - if updated.IngressURLTemplate != "" { - fmt.Printf(" %s\n", updated.IngressURLTemplate) - pterm.Println(pterm.Gray(" Replace with the port your service is listening on.")) + renderResult(c, "ingress_updated", map[string]any{ + "id": id, + "ingress": target, + "ingress_url": updated.IngressURLTemplate, + }, func() { + if target { + pterm.Success.Printfln("Public URL is on for %s", refLabel(label, id)) + if updated.IngressURLTemplate != "" { + fmt.Printf(" %s\n", updated.IngressURLTemplate) + pterm.Println(pterm.Gray(" Replace with the port your service is listening on.")) + } + } else { + pterm.Success.Printfln("Public URL is off for %s", refLabel(label, id)) } - } else { - pterm.Success.Printfln("Public URL is off for %s", refLabel(label, id)) - } + }) return nil } @@ -347,7 +353,13 @@ func applyAddSSHKeys(c *cli.Context, client *api.SandboxClient, label, id string if err != nil { return err } - pterm.Success.Printfln("Added %d SSH key(s) to %s β€” total now %d", len(keys), refLabel(label, id), count) + renderResult(c, "ssh_keys_added", map[string]any{ + "id": id, + "keys_added": len(keys), + "total_keys": count, + }, func() { + pterm.Success.Printfln("Added %d SSH key(s) to %s β€” total now %d", len(keys), refLabel(label, id), count) + }) return nil } @@ -368,12 +380,17 @@ func applyAutoPauseFlag(c *cli.Context, client *api.SandboxClient, label, id, va if err != nil { return err } - if updated.AutoPauseAfterSeconds != nil { - d := time.Duration(*updated.AutoPauseAfterSeconds) * time.Second - pterm.Success.Printfln("Auto-pause set to %s for %s", formatDuration(d), refLabel(label, id)) - } else { - pterm.Success.Printfln("Auto-pause turned off for %s", refLabel(label, id)) - } + renderResult(c, "auto_pause_updated", map[string]any{ + "id": id, + "auto_pause_after_seconds": updated.AutoPauseAfterSeconds, + }, func() { + if updated.AutoPauseAfterSeconds != nil { + d := time.Duration(*updated.AutoPauseAfterSeconds) * time.Second + pterm.Success.Printfln("Auto-pause set to %s for %s", formatDuration(d), refLabel(label, id)) + } else { + pterm.Success.Printfln("Auto-pause turned off for %s", refLabel(label, id)) + } + }) return nil } diff --git a/cmd/sandbox/exec.go b/cmd/sandbox/exec.go index 6adfbb9..78ea5d7 100644 --- a/cmd/sandbox/exec.go +++ b/cmd/sandbox/exec.go @@ -10,6 +10,8 @@ import ( "github.com/urfave/cli/v2" "github.com/NodeOps-app/createos-cli/internal/api" + "github.com/NodeOps-app/createos-cli/internal/cliargs" + "github.com/NodeOps-app/createos-cli/internal/output" "github.com/NodeOps-app/createos-cli/internal/terminal" ) @@ -23,11 +25,17 @@ func newExecCommand() *cli.Command { arrives all at once when the command finishes. Pass --stream to see stdout/stderr live as it happens. +Anything piped in is forwarded to the command's standard input, so +'cat script.sh | createos sandbox exec my-box -- bash' works. Use +--stdin to read from a file instead. + Examples: createos sandbox exec my-box -- uname -a createos sandbox exec my-box -- python3 -c 'print("hi")' createos sandbox exec my-box --stream -- pip install requests createos sandbox exec my-box -- bash -c "echo $USER && date" + echo "hello" | createos sandbox exec my-box -- cat + createos sandbox exec my-box --stdin ./setup.sh -- bash The command's exit code is preserved β€” if the program inside the sandbox exits with 1, this CLI also exits with 1.`, @@ -37,6 +45,10 @@ sandbox exits with 1, this CLI also exits with 1.`, Aliases: []string{"s"}, Usage: "Show output live as the command runs", }, + &cli.StringFlag{ + Name: "stdin", + Usage: "Read the command's standard input from `FILE` ('-' for piped input)", + }, &cli.StringSliceFlag{ Name: "env", Usage: "Override an environment variable for this exec (repeatable): KEY=VALUE. " + @@ -104,27 +116,134 @@ func runExec(c *cli.Context) error { } } - envs, err := parseEnvFlags(c.StringSlice("env")) + stdinPath, envFlags, stream := parseExecFlags(c) + + envs, err := parseEnvFlags(envFlags) + if err != nil { + return err + } + stdin, err := readExecStdin(stdinPath) if err != nil { return err } req := api.SandboxExecReq{ - Cmd: cmd, - Args: args, - Env: envs, + Cmd: cmd, + Args: args, + Env: envs, + Stdin: stdin, } - if c.Bool("stream") { + if stream { return runExecStream(c, client, id, req) } return runExecBuffered(c, client, id, req) } +// parseExecFlags recovers exec's own flags (--stdin, --env, --stream) from +// the raw tokens between the sandbox ref and the `--` command delimiter. +// urfave/cli v2 stops flag parsing at the first positional (the ref), so +// `exec --stdin file -- cmd` never reaches c.String("stdin") β€” the +// CLI's own --help examples show exactly that ordering. Same class of bug, +// same fix shape, as parseSyncArgs. Seeds from whatever urfave DID parse +// (covers flags placed before the ref) and overrides with anything found in +// the ref..`--` window, so either ordering works. +func parseExecFlags(c *cli.Context) (stdin string, envs []string, stream bool) { + stdin = c.String("stdin") + envs = append([]string{}, c.StringSlice("env")...) + stream = c.Bool("stream") + + all := c.Args().Slice() + sep := -1 + for i, a := range all { + if a == "--" { + sep = i + break + } + } + if sep <= 1 { + // No ref..`--` window to scan (no ref, or `--` is the first token). + return stdin, envs, stream + } + + own := all[1:sep] + for i := 0; i < len(own); i++ { + a := strings.TrimSpace(own[i]) + if !strings.HasPrefix(a, "-") { + continue + } + raw := strings.TrimLeft(a, "-") + key, inline, hasInline := raw, "", false + if eq := strings.IndexByte(raw, '='); eq >= 0 { + key, inline, hasInline = raw[:eq], raw[eq+1:], true + } + switch key { + case "stream", "s": + stream = true + case "stdin": + val := inline + if !hasInline && i+1 < len(own) { + val = own[i+1] + i++ + } + stdin = strings.TrimSpace(val) + case "env": + val := inline + if !hasInline && i+1 < len(own) { + val = own[i+1] + i++ + } + if val != "" { + envs = append(envs, val) + } + } + } + return stdin, envs, stream +} + +// readExecStdin collects the payload for the command's standard input: +// an explicit --stdin FILE, or whatever was piped in. On a TTY with no +// --stdin there is nothing to read, so the command gets empty stdin +// rather than blocking on the keyboard. +func readExecStdin(path string) (string, error) { + switch { + case path != "" && path != "-": + data, err := os.ReadFile(path) // #nosec G304 -- the user names the file to send + if err != nil { + return "", fmt.Errorf("could not read %s\n\n Check the path is correct and the file exists", path) + } + return string(data), nil + case path == "-" || terminal.HasPipedStdin(): + data, err := io.ReadAll(os.Stdin) + if err != nil { + return "", fmt.Errorf("could not read the piped input: %w", err) + } + return string(data), nil + } + return "", nil +} + func runExecBuffered(c *cli.Context, client *api.SandboxClient, id string, req api.SandboxExecReq) error { resp, err := client.ExecSandbox(c.Context, id, req) if err != nil { return err } + + // JSON mode keeps stdout a single parseable document: the command's + // own output becomes fields rather than raw bytes on the stream. + if output.IsJSONExplicit(c) { + output.Render(c, map[string]any{ + "sandbox_id": id, + "exit_code": resp.Result.ExitCode, + "stdout": resp.Result.Stdout, + "stderr": resp.Result.Stderr, + "error": resp.Result.Error, + }, func() {}) + if resp.Result.ExitCode != 0 { + os.Exit(resp.Result.ExitCode) + } + return nil + } + if resp.Result.Stdout != "" { fmt.Print(resp.Result.Stdout) if !strings.HasSuffix(resp.Result.Stdout, "\n") { @@ -190,10 +309,13 @@ func parseExecArgs(c *cli.Context) (ref, cmd string, args []string) { return "", "", nil } - // First: did the user write `... exec -- …`? Scan os.Args. + // First: did the user write `... exec -- …`? Scan os.Args, hoisted the + // same way main.go hoists it, so a global flag typed after the + // subcommand ("exec --output json -- ls") doesn't hide the separator. + argv := cliargs.Hoist(os.Args) leadingDoubleDash := false - for i, a := range os.Args { - if a == "exec" && i+1 < len(os.Args) && os.Args[i+1] == "--" { + for i, a := range argv { + if a == "exec" && i+1 < len(argv) && argv[i+1] == "--" { leadingDoubleDash = true break } diff --git a/cmd/sandbox/firewall.go b/cmd/sandbox/firewall.go index d542842..880212a 100644 --- a/cmd/sandbox/firewall.go +++ b/cmd/sandbox/firewall.go @@ -173,7 +173,12 @@ func runFirewallClear(c *cli.Context) error { if _, err := client.SetEgress(c.Context, id, []string{}); err != nil { return err } - pterm.Success.Printfln("Firewall cleared on %s β€” all outbound traffic allowed.", refLabel(ref, id)) + renderResult(c, "firewall_cleared", map[string]any{ + "id": id, + "rules": []string{}, + }, func() { + pterm.Success.Printfln("Firewall cleared on %s β€” all outbound traffic allowed.", refLabel(ref, id)) + }) return nil } @@ -183,10 +188,15 @@ func applyFirewall(c *cli.Context, client *api.SandboxClient, id, ref string, ru if err != nil { return err } - pterm.Success.Printfln("Firewall updated on %s β€” %d rule(s) active.", refLabel(ref, id), len(stored)) - for _, r := range stored { - pterm.Println(pterm.Gray(" β€’ " + r)) - } + renderResult(c, "firewall_updated", map[string]any{ + "id": id, + "rules": stored, + }, func() { + pterm.Success.Printfln("Firewall updated on %s β€” %d rule(s) active.", refLabel(ref, id), len(stored)) + for _, r := range stored { + pterm.Println(pterm.Gray(" β€’ " + r)) + } + }) return nil } diff --git a/cmd/sandbox/fork.go b/cmd/sandbox/fork.go index bd55de9..1044ec7 100644 --- a/cmd/sandbox/fork.go +++ b/cmd/sandbox/fork.go @@ -8,7 +8,6 @@ import ( "github.com/urfave/cli/v2" "github.com/NodeOps-app/createos-cli/internal/api" - "github.com/NodeOps-app/createos-cli/internal/output" "github.com/NodeOps-app/createos-cli/internal/terminal" ) @@ -81,23 +80,9 @@ func runForkByID(c *cli.Context, client *api.SandboxClient, ref, srcID string) e req.Egress = egress } - if output.IsJSON(c) { - view, err := client.ForkSandbox(c.Context, srcID, req) - if err != nil { - return err - } - target := "running" - if req.StartPaused { - target = "paused" - } - sb, err := waitForStatus(c.Context, client, view.ID, target) - if err != nil { - return err - } - output.Render(c, sb, func() {}) - return nil - } - + // One call path for both formats β€” the spinner writes to stderr. A + // separate JSON branch here used to skip the status check below, so a + // fork that landed in the wrong state reported success. spinner, _ := pterm.DefaultSpinner.Start(fmt.Sprintf("Forking %s…", refLabel(ref, srcID))) //nolint:errcheck view, err := client.ForkSandbox(c.Context, srcID, req) if err != nil { @@ -119,16 +104,24 @@ func runForkByID(c *cli.Context, client *api.SandboxClient, ref, srcID string) e return fmt.Errorf("sandbox %s is %s β€” see `createos sandbox get %s` for details", sb.ID, sb.Status, sb.ID) } - name := "" - if sb.Name != nil { - name = *sb.Name - } + name := str(sb.Name) spinner.Success(fmt.Sprintf("Forked into %s", refLabel(name, sb.ID))) - if sb.IP != nil && *sb.IP != "" { - fmt.Printf(" IP: %s\n", *sb.IP) - } - if sb.IngressURLTemplate != "" { - fmt.Printf(" URL: %s\n", sb.IngressURLTemplate) - } + + renderResult(c, "forked", withResponse(sb, map[string]any{ + "id": sb.ID, + "name": name, + "status": sb.Status, + "ip": str(sb.IP), + "ingress_url": sb.IngressURLTemplate, + "source_id": srcID, + "shell_command": fmt.Sprintf("createos sandbox shell %s", sb.ID), + }), func() { + if sb.IP != nil && *sb.IP != "" { + fmt.Printf(" IP: %s\n", *sb.IP) + } + if sb.IngressURLTemplate != "" { + fmt.Printf(" URL: %s\n", sb.IngressURLTemplate) + } + }) return nil } diff --git a/cmd/sandbox/jsonout.go b/cmd/sandbox/jsonout.go new file mode 100644 index 0000000..22f9e6e --- /dev/null +++ b/cmd/sandbox/jsonout.go @@ -0,0 +1,56 @@ +package sandbox + +import ( + "encoding/json" + "maps" + + "github.com/urfave/cli/v2" + + "github.com/NodeOps-app/createos-cli/internal/output" +) + +// renderResult is how every sandbox mutation reports what it did. +// +// In JSON mode it writes one object to stdout; otherwise it runs the +// human renderer untouched. The "action" field names the operation that +// completed, so a caller reading a stream of results can tell a pause from +// a resume without tracking which command it invoked. +// +// Keys are shared with the read commands on purpose β€” a caller can diff the +// object `create` returned against the one `get` returns later. +func renderResult(c *cli.Context, action string, fields map[string]any, human func()) { + obj := make(map[string]any, len(fields)+1) + obj["action"] = action + maps.Copy(obj, fields) + output.Render(c, obj, human) +} + +// withResponse folds an API response's own JSON fields into a result object, +// so a caller gets everything the server returned (vcpu, mem_mib, egress, +// quotas…) as well as the action and the convenience keys. Curated keys win +// on collision β€” those are the documented, stable ones. +// +// This keeps the whole-payload behaviour of the earlier per-command JSON +// branches while routing every command through one code path. +func withResponse(resp any, fields map[string]any) map[string]any { + raw, err := json.Marshal(resp) + if err != nil { + return fields + } + var flat map[string]any + if err := json.Unmarshal(raw, &flat); err != nil { + return fields + } + maps.Copy(flat, fields) + return flat +} + +// str dereferences an optional API string field. The sandbox API returns +// null for anything not yet assigned (a name, an IP on a still-creating +// box), and JSON consumers are better served by "" than by a missing key. +func str(p *string) string { + if p == nil { + return "" + } + return *p +} diff --git a/cmd/sandbox/network.go b/cmd/sandbox/network.go index 5b82cd5..58d7e55 100644 --- a/cmd/sandbox/network.go +++ b/cmd/sandbox/network.go @@ -68,7 +68,10 @@ func runNetworkCreate(c *cli.Context) error { if err != nil { return err } - output.Render(c, n, func() { + renderResult(c, "network_created", withResponse(n, map[string]any{ + "id": n.ID, + "name": n.Name, + }), func() { pterm.Success.Printfln("Created network %s (%s)", n.Name, n.ID) pterm.Println(pterm.Gray(" Attach at create time: createos sandbox create --network " + n.Name)) pterm.Println(pterm.Gray(" Or live-attach later: createos sandbox network attach " + n.Name + " ")) @@ -242,14 +245,25 @@ func runNetworkRm(c *cli.Context) error { } } failed := 0 + results := make([]map[string]any, 0, len(refs)) for _, ref := range refs { if err := deleteNetworkCascade(c, client, ref); err != nil { - pterm.Error.Printfln("%s: %s", ref, api.UserMessageVerbose(err)) + // Sanitized in the JSON result too β€” a raw Go error leaks + // syscall detail and local paths whichever stream it lands on. + msg := api.UserMessageVerbose(err) + pterm.Error.Printfln("%s: %s", ref, msg) + results = append(results, map[string]any{"ref": ref, "deleted": false, "error": msg}) failed++ continue } + results = append(results, map[string]any{"ref": ref, "deleted": true}) pterm.Success.Printfln("Deleted network %s", ref) } + renderResult(c, "network_deleted", map[string]any{ + "results": results, + "deleted": len(results) - failed, + "failed": failed, + }, func() {}) if failed > 0 { return fmt.Errorf("%d of %d deletes failed", failed, len(refs)) } @@ -374,8 +388,14 @@ func runNetworkAttach(c *cli.Context) error { if err := client.AttachDeviceToNetwork(c.Context, ref, netRef); err != nil { return err } - pterm.Success.Printfln("Attached device %s β†’ network %s", ref, netRef) - pterm.Println(pterm.Gray(" The device can now reach VMs on this network once it brings up the tunnel.")) + renderResult(c, "network_attached", map[string]any{ + "network": netRef, + "endpoint": ref, + "type": "device", + }, func() { + pterm.Success.Printfln("Attached device %s β†’ network %s", ref, netRef) + pterm.Println(pterm.Gray(" The device can now reach VMs on this network once it brings up the tunnel.")) + }) return nil } sandboxID, err := resolveSandboxRef(c.Context, client, ref) @@ -385,8 +405,14 @@ func runNetworkAttach(c *cli.Context) error { if err := client.AttachNetwork(c.Context, sandboxID, netRef); err != nil { return err } - pterm.Success.Printfln("Attached %s β†’ network %s", refLabel(ref, sandboxID), netRef) - pterm.Println(pterm.Gray(" Other sandboxes on this network can now reach this one by name.")) + renderResult(c, "network_attached", map[string]any{ + "network": netRef, + "endpoint": sandboxID, + "type": "sandbox", + }, func() { + pterm.Success.Printfln("Attached %s β†’ network %s", refLabel(ref, sandboxID), netRef) + pterm.Println(pterm.Gray(" Other sandboxes on this network can now reach this one by name.")) + }) return nil } @@ -476,7 +502,13 @@ func runNetworkDetach(c *cli.Context) error { if err := client.DetachDeviceFromNetwork(c.Context, ref, netRef); err != nil { return err } - pterm.Success.Printfln("Detached device %s from network %s", ref, netRef) + renderResult(c, "network_detached", map[string]any{ + "network": netRef, + "endpoint": ref, + "type": "device", + }, func() { + pterm.Success.Printfln("Detached device %s from network %s", ref, netRef) + }) return nil } sandboxID, err := resolveSandboxRef(c.Context, client, ref) @@ -486,7 +518,13 @@ func runNetworkDetach(c *cli.Context) error { if err := client.DetachNetwork(c.Context, sandboxID, netRef); err != nil { return err } - pterm.Success.Printfln("Detached %s from network %s", refLabel(ref, sandboxID), netRef) + renderResult(c, "network_detached", map[string]any{ + "network": netRef, + "endpoint": sandboxID, + "type": "sandbox", + }, func() { + pterm.Success.Printfln("Detached %s from network %s", refLabel(ref, sandboxID), netRef) + }) return nil } diff --git a/cmd/sandbox/pause.go b/cmd/sandbox/pause.go index 3c8f120..0a0e532 100644 --- a/cmd/sandbox/pause.go +++ b/cmd/sandbox/pause.go @@ -67,5 +67,10 @@ func runPauseByID(c *cli.Context, client *api.SandboxClient, ref, id string) err return fmt.Errorf("sandbox %s is %s β€” see `createos sandbox get %s` for details", refLabel(ref, id), sb.Status, id) } spinner.Success(fmt.Sprintf("Paused %s", refLabel(ref, id))) + renderResult(c, "paused", map[string]any{ + "id": sb.ID, + "name": str(sb.Name), + "status": sb.Status, + }, func() {}) return nil } diff --git a/cmd/sandbox/pull.go b/cmd/sandbox/pull.go index 28ca621..910b923 100644 --- a/cmd/sandbox/pull.go +++ b/cmd/sandbox/pull.go @@ -69,5 +69,11 @@ func runPull(c *cli.Context) error { return err } spinner.Success(fmt.Sprintf("Downloaded %s:%s β†’ %s (%s)", refLabel(ref, id), remote, local, humanBytes(n))) + renderResult(c, "pulled", map[string]any{ + "id": id, + "remote_path": remote, + "local_path": local, + "bytes": n, + }, func() {}) return nil } diff --git a/cmd/sandbox/push.go b/cmd/sandbox/push.go index a8a6d1b..8be1a57 100644 --- a/cmd/sandbox/push.go +++ b/cmd/sandbox/push.go @@ -117,6 +117,12 @@ func runPush(c *cli.Context) error { pterm.Println(pterm.Gray(fmt.Sprintf(" %s in %s (%s)", humanBytes(sent), formatElapsed(elapsed), throughput(sent, elapsed)))) } + renderResult(c, "pushed", map[string]any{ + "id": id, + "remote_path": remote, + "bytes": sent, + "elapsed_ms": elapsed.Milliseconds(), + }, func() {}) return nil } diff --git a/cmd/sandbox/resolve.go b/cmd/sandbox/resolve.go index 355424c..cb8a20e 100644 --- a/cmd/sandbox/resolve.go +++ b/cmd/sandbox/resolve.go @@ -3,6 +3,7 @@ package sandbox import ( "context" "fmt" + "net/http" "sort" "strings" @@ -89,7 +90,17 @@ func resolveSandboxRef(ctx context.Context, client *api.SandboxClient, ref strin } } if len(matches) == 0 { - return "", fmt.Errorf("no sandbox named %q\n\n To see your sandboxes, run:\n createos sandbox list", ref) + // Shaped as an *api.APIError, not a bare fmt.Errorf, even though + // this never touched the network: it's the only client-side "not + // found" in the whole CLI, and without this every consumer of the + // error β€” main.go's JSON envelope code/hint, batch-loop results in + // rm/disk/network/template β€” silently lost both the "not_found" + // code and, through api.UserMessageVerbose's raw-error fallback, + // the message itself down to a generic "something went wrong". + return "", &api.APIError{ + StatusCode: http.StatusNotFound, + Message: fmt.Sprintf("no sandbox named %q\n\n To see your sandboxes, run:\n createos sandbox list", ref), + } } // Most-recent wins. Stable sort so deterministic when timestamps tie. sort.SliceStable(matches, func(i, j int) bool { diff --git a/cmd/sandbox/resume.go b/cmd/sandbox/resume.go index b9cc866..d384827 100644 --- a/cmd/sandbox/resume.go +++ b/cmd/sandbox/resume.go @@ -66,5 +66,13 @@ func runResumeByID(c *cli.Context, client *api.SandboxClient, ref, id string) er return fmt.Errorf("sandbox %s is %s β€” see `createos sandbox get %s` for details", refLabel(ref, id), sb.Status, id) } spinner.Success(fmt.Sprintf("Resumed %s", refLabel(ref, id))) + renderResult(c, "resumed", map[string]any{ + "id": sb.ID, + "name": str(sb.Name), + "status": sb.Status, + "ip": str(sb.IP), + "ingress_url": sb.IngressURLTemplate, + "shell_command": fmt.Sprintf("createos sandbox shell %s", sb.ID), + }, func() {}) return nil } diff --git a/cmd/sandbox/rm.go b/cmd/sandbox/rm.go index 866d237..4c14ce8 100644 --- a/cmd/sandbox/rm.go +++ b/cmd/sandbox/rm.go @@ -105,18 +105,28 @@ func runRm(c *cli.Context) error { // are reported per-ref so a typo in one name doesn't kill the rest // of the batch. failed := 0 + // One entry per ref, deleted or not, so a batch caller can tell which + // of the refs it passed actually went away. + results := make([]map[string]any, 0, len(ids)) for _, ref := range ids { id, err := resolveSandboxRef(c.Context, client, ref) if err != nil { - pterm.Error.Printfln("%s: %s", ref, api.UserMessageVerbose(err)) + // Sanitized in the JSON result too β€” a raw Go error leaks + // syscall detail and local paths whichever stream it lands on. + msg := api.UserMessageVerbose(err) + pterm.Error.Printfln("%s: %s", ref, msg) + results = append(results, map[string]any{"ref": ref, "deleted": false, "error": msg}) failed++ continue } if err := client.DestroySandbox(c.Context, id); err != nil { - pterm.Error.Printfln("%s: %s", ref, api.UserMessageVerbose(err)) + msg := api.UserMessageVerbose(err) + pterm.Error.Printfln("%s: %s", ref, msg) + results = append(results, map[string]any{"ref": ref, "id": id, "deleted": false, "error": msg}) failed++ continue } + results = append(results, map[string]any{"ref": ref, "id": id, "deleted": true}) // Echo the friendly ref the user typed; if it was already an // id this reads the same, if it was a name they see what was // actually removed. @@ -126,6 +136,13 @@ func runRm(c *cli.Context) error { pterm.Success.Printfln("Deleted %s", id) } } + + renderResult(c, "deleted", map[string]any{ + "results": results, + "deleted": len(results) - failed, + "failed": failed, + }, func() {}) + if failed > 0 { // Non-zero exit so scripts can tell something went wrong. os.Exit(1) diff --git a/cmd/sandbox/template.go b/cmd/sandbox/template.go index 1fad088..3dc8fa6 100644 --- a/cmd/sandbox/template.go +++ b/cmd/sandbox/template.go @@ -79,7 +79,13 @@ func runTemplateSubmit(c *cli.Context) error { if err != nil { return err } - pterm.Success.Printfln("Submitted template %s (status: %s)", view.Name, view.Status) + renderResult(c, "template_submitted", map[string]any{ + "id": view.ID, + "name": view.Name, + "status": view.Status, + }, func() { + pterm.Success.Printfln("Submitted template %s (status: %s)", view.Name, view.Status) + }) if !follow { pterm.Println(pterm.Gray(fmt.Sprintf(" Watch progress with: createos sandbox template logs %s --follow", view.Name))) return nil @@ -378,14 +384,25 @@ func runTemplateRm(c *cli.Context) error { } } failed := 0 + results := make([]map[string]any, 0, len(refs)) for _, ref := range refs { if err := client.DeleteTemplate(c.Context, ref); err != nil { - pterm.Error.Printfln("%s: %s", ref, api.UserMessageVerbose(err)) + // Sanitized in the JSON result too β€” a raw Go error leaks + // syscall detail and local paths whichever stream it lands on. + msg := api.UserMessageVerbose(err) + pterm.Error.Printfln("%s: %s", ref, msg) + results = append(results, map[string]any{"ref": ref, "deleted": false, "error": msg}) failed++ continue } + results = append(results, map[string]any{"ref": ref, "deleted": true}) pterm.Success.Printfln("Deleted template %s", ref) } + renderResult(c, "template_deleted", map[string]any{ + "results": results, + "deleted": len(results) - failed, + "failed": failed, + }, func() {}) if failed > 0 { return fmt.Errorf("%d of %d deletes failed", failed, len(refs)) } diff --git a/cmd/sandbox/tunnel.go b/cmd/sandbox/tunnel.go index a11e211..e986f7e 100644 --- a/cmd/sandbox/tunnel.go +++ b/cmd/sandbox/tunnel.go @@ -169,8 +169,16 @@ func runTunnel(c *cli.Context) error { return err } - pterm.Success.Printfln("Forwarding %s β†’ %s:%d", listenAddr, refLabel(ref, id), remote) - pterm.Println(pterm.Gray(" Press Ctrl+C to stop.")) + // Emitted before the blocking accept loop: a caller in JSON mode needs + // the bound address now, not when the tunnel is torn down. + renderResult(c, "tunnel_started", map[string]any{ + "id": id, + "listen_addr": listenAddr, + "remote_port": remote, + }, func() { + pterm.Success.Printfln("Forwarding %s β†’ %s:%d", listenAddr, refLabel(ref, id), remote) + pterm.Println(pterm.Gray(" Press Ctrl+C to stop.")) + }) // Trap Ctrl+C so we can close cleanly and not leave half-open conns. sigCh := make(chan os.Signal, 1) diff --git a/cmd/sandbox/vpn.go b/cmd/sandbox/vpn.go index 889bc59..60a3839 100644 --- a/cmd/sandbox/vpn.go +++ b/cmd/sandbox/vpn.go @@ -183,10 +183,19 @@ func runVPNUp(c *cli.Context) error { } ifaceName := strings.TrimSuffix(filepath.Base(confPath), ".conf") - pterm.Success.Printfln("VPN connected as %s (%s).", st.Name, st.ClientIP) - pterm.Println(pterm.Gray(fmt.Sprintf(" device: %s", st.Name))) - pterm.Println(pterm.Gray(fmt.Sprintf(" iface: %s", ifaceName))) - pterm.Println(pterm.Gray("Press Ctrl-C to disconnect.")) + // Emitted before the block on Ctrl-C, for the same reason as the + // tunnel: the caller needs the interface and address up front. + renderResult(c, "vpn_connected", map[string]any{ + "device_id": st.DeviceID, + "device": st.Name, + "client_ip": st.ClientIP, + "interface": ifaceName, + }, func() { + pterm.Success.Printfln("VPN connected as %s (%s).", st.Name, st.ClientIP) + pterm.Println(pterm.Gray(fmt.Sprintf(" device: %s", st.Name))) + pterm.Println(pterm.Gray(fmt.Sprintf(" iface: %s", ifaceName))) + pterm.Println(pterm.Gray("Press Ctrl-C to disconnect.")) + }) // Block until Ctrl-C / SIGTERM (user disconnect) or until the // renewal goroutine signals that the server-side session is gone. diff --git a/docs/decisions.md b/docs/decisions.md new file mode 100644 index 0000000..026411a --- /dev/null +++ b/docs/decisions.md @@ -0,0 +1,198 @@ +# Decisions + +Running log of design decisions in this repo, with the options that were on +the table and why one won. Written for whoever β€” human or agent β€” picks this +up next. + +## 2026-08-11 β€” Machine-readable output for CI and agents + +An external review produced ~40 UX/DX/CI/agent findings. We shipped the five +that were blocking automation, plus the scope the user widened. + +### D1. Errors go to stderr; JSON mode gets an error envelope + +**Problem.** `main.go` printed errors with `pterm.Error`, whose default writer +is stdout. `createos … > out.json` captured the error text as data, and +`2>/dev/null` hid nothing. + +Options considered: + +- **(a) Print errors to stderr always.** Simple, conventional. In JSON mode a + consumer reading only stdout gets an empty stream on failure and has to + infer the error from the exit code. +- **(b) Error envelope on stdout in JSON mode, stderr otherwise.** ← chosen. + A JSON consumer reading one stream always parses a valid document, success + or failure. Human users get the Unix convention. +- (c) Envelope on stderr in JSON mode. Rejected: forces every consumer to + merge two streams to find out what happened. + +The envelope is `{"error":{"code","message"}}`. `code` is a stable slug +derived from the HTTP status (`api.APIError.Code()`), so callers branch on +the failure class instead of matching on message text. + +### D2. `--output json` implies "stdout is only JSON" + +`pterm.SetDefaultOutput(os.Stderr)` is set in the root `Before` hook whenever +JSON mode is active. Every pterm print in the codebase β€” including commands +not yet converted β€” lands on stderr, so no narration can corrupt the +document on stdout. Spinners already wrote to stderr (pterm's default). + +### D3. `sandbox exec` uses `IsJSONExplicit`, not `IsJSON` + +`output.DetectFormat` auto-selects JSON when stdout is not a TTY. For most +commands that is right. For `exec` it is actively wrong: stdout **is** the +payload, so `createos sandbox exec box -- cat data.csv > out` would have +written a JSON envelope instead of the file the caller expected. + +So `exec` only wraps its result when the user typed `--output json`. Every +other command keeps the auto-detect behaviour. `output.IsJSONExplicit` marks +this distinction; use it for any future command whose stdout is a payload +rather than a report. + +### D4. Global flags are hoisted in argv, not mirrored onto commands + +**Problem.** urfave/cli v2 only parses app-level flags before the first +subcommand token, so `createos sandbox create --output json` died with +"flag provided but not defined: -output". + +Options considered: + +- **(a) Declare hidden copies of each global flag on every top-level command.** + Rejected: the flags would *parse* but their values would never be read β€” + the `App.Before` hook that consumes them runs before subcommand parsing, so + `sandbox create --api-key X` would silently ignore the key. Silently-wrong + is worse than the error it replaces. +- **(b) Re-detect the format in a per-command `Before` hook.** Fixes + `--output` only; leaves `--api-key`, `--api-url`, `--debug` broken. +- **(c) Rewrite argv before `app.Run`.** ← chosen. `internal/cliargs.Hoist` + moves known global flags (and their values) in front of the subcommand. + One change, every flag and every command fixed, no per-command upkeep. + +Tokens after a bare `--` are never touched, so `exec box -- ./ci.sh --debug` +keeps passing `--debug` to the user's script. `cmd/sandbox/exec.go`'s own +argv scan runs `Hoist` first, so it sees the same normalized line. + +Verified safe: no subcommand declares any of the hoisted names or uses `-o` +/ `-d` as an alias. + +### D5. Which sandbox commands emit JSON + +The user's call: **all** sandbox subcommands, not just `create`/`fork`/`edit`. + +Every command that creates, changes, or deletes now calls `renderResult` +(`cmd/sandbox/jsonout.go`) with an `action` field plus the ids needed to +chain the next call. Batch deletes (`rm`, `disk rm`, `network rm`, +`template rm`) return a per-ref `results` array so a caller can tell which +references succeeded β€” a single exit code cannot express a partial batch. + +Left as human-readable, deliberately: + +| Command | Why | +|---|---| +| `shell`, `editor`, `sync` | Interactive sessions. No single result to report. | +| `exec --stream` | The stream *is* the output; framing it would break live piping. | +| `template logs` | Same β€” a log stream. | +| `edit` interactive menu | TTY-only path; JSON mode never reaches it. | + +`tunnel` and `vpn up` block until Ctrl-C, so they emit their JSON result +when the connection comes up rather than on exit β€” a caller needs the bound +address while the tunnel is alive, not after it dies. + +### D6. `exec` stdin + +`api.SandboxExecReq.Stdin` existed and was never populated. Now: piped stdin +is forwarded automatically, `--stdin FILE` reads from a file, `--stdin -` +forces reading the pipe. On a TTY with no `--stdin` nothing is read, so the +command does not block waiting on a keyboard. + +Known limitation: consuming piped stdin means it is not available for +interactive prompts. In practice a caller that pipes data also passes the +sandbox ref and command explicitly, so no prompt is reached. + +### D7. Reconciling with the JSON work that landed on main first + +While this branch was open, `33c6f8a` ("fix: json output for create") added JSON +to `create`, `fork`, `disk create`, and `network create` by a different route: +an `if output.IsJSON(c)` early-return that repeats the API call and renders the +raw response struct. `57d2ebf` separately added `api.UserMessageVerbose()` to +stop raw Go errors leaking to users. + +Resolved as follows: + +- **Structure β€” kept the single path.** The early-return branch calls + `client.CreateSandbox` twice in two places, so a change to one silently + misses the other. It had already drifted: the `fork` branch skipped the + `sb.Status != target` check, so a fork that ended in the wrong state + reported success in JSON mode. The spinner writes to stderr, so one path + serves both formats safely. +- **Payload β€” kept the whole response.** Rendering the raw struct returned + more than the curated key set (`vcpu`, `mem_mib`, `disk_mib`, `egress`, + quotas). Dropping those would regress anyone on today's `main`, so + `withResponse` folds the response's own JSON fields in underneath the + curated keys, which win on collision. +- **Sanitization β€” extended to JSON.** Batch results now carry + `api.UserMessageVerbose(err)` rather than `err.Error()`. A raw Go error + leaks syscall detail and local paths whichever stream it lands on, so the + machine-readable field gets the same treatment as the human one. +- **`Hint()` β€” added to the envelope.** `main.go` on main had started showing + `APIError.Hint()` to humans. The JSON envelope carries the same string as an + optional `hint`, so an agent relaying a failure can pass on the same advice. +- **Arg order β€” theirs wins.** `05f45b6` swapped `network attach|detach` to + ` `; the result objects were adapted to it. + +### Not done (and why) + +- **`--wait` on create/get** β€” real CI value, but a caller can poll `get`. + Next tier, not blocking. +- **Cancel exit code 130** β€” correct convention; needs the ~52 scattered + cancel strings unified in the same pass. Deferred as one coherent change. +- **`--quiet` on mutations** β€” superseded for machine callers by JSON output. + Still worth it for shell users. +- **Shape price hints in the picker** β€” `api.Shape` carries no pricing. That + is an `fc` change, not a CLI change. + +## 2026-08-11 β€” Six-agent fleet test against a compiled binary, real account + +Before treating the JSON-output work as done, six agents drove a locally +compiled binary against a real authenticated account in parallel β€” each one +briefed only on the feature surface, not told what to expect β€” covering the +full createβ†’execβ†’rm lifecycle, exec's stdin contract, flag placement and the +error envelope, `NO_COLOR`/non-TTY hygiene, the network mutation lifecycle, +and a cold-start agent with zero prior knowledge of the CLI working from +`--help` alone. Full transcripts aren't kept here; the two real bugs they +surfaced and their fixes are. + +**Bug 1 β€” `exec`'s own flags silently no-op after the ref.** `exec +--stdin file -- cmd` and `exec --env K=V -- cmd` both sent nothing, +with no error β€” exactly the ordering shown in the command's own `--help` +examples, so the documented usage didn't work as documented. Root cause: +`parseExecArgs` (added when `--stdin` shipped) only recovered `ref`/`cmd` +from the raw tokens around `--`, never `--stdin`/`--env`/`--stream` +themselves β€” those still went through `c.String("stdin")` etc., which is +empty because urfave/cli v2 stops flag parsing at the first positional (the +ref). This is the identical bug class `parseSyncArgs` already exists to work +around in `sync.go`; `--env` had silently had this bug since before this +work started, `--stdin` inherited it on arrival. Fixed with a +`parseExecFlags` following `parseSyncArgs`'s exact shape: seed from whatever +urfave did parse, override with anything found in the ref..`--` window. + +**Bug 2 β€” batch-delete error messages collapsed to a generic placeholder.** +Traced to the `57d2ebf` merge conflict resolution: `resolveSandboxRef`'s +"no sandbox named X" error was a bare `fmt.Errorf`, not an `*api.APIError`. +Passing it through `api.UserMessageVerbose` β€” correct for actually-raw Go +errors, which is what that function exists to sanitize β€” flattened it to +"something went wrong β€” please try again or contact support" in every batch +result (`rm`, and by the same path `disk rm`/`network rm`/`template rm` +share). It also meant every direct not-found (`get`, `pause`, …) reported +`"code": "not_found"`'s intended slug as the placeholder `"code": "error"` +instead. Fixed by shaping `resolveSandboxRef`'s not-found error as a real +`*api.APIError` (404) β€” the one client-side "not found" in the CLI, so this +was a one-function fix, not a policy change to `UserMessageVerbose` itself. + +**Found, not fixed β€” flagged for follow-up:** `network attach|detach --yes` +silently fails to parse when `--yes` trails the two positionals (same class +of bug as above, but on a command with no re-scanner of its own yet β€” would +need a `parseNetworkArgs` mirroring `parseSyncArgs`/`parseExecFlags`). +Everything else the fleet touched β€” JSON shape, batch results, `NO_COLOR`, +stdout/stderr separation, flag hoisting, the redirect-safety boundary on +`exec`'s stdout β€” held up clean on live account against real and bogus refs. diff --git a/internal/api/types.go b/internal/api/types.go index 27f5eeb..a392fef 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -74,6 +74,30 @@ func (e *APIError) Hint() string { } } +// Code returns a stable, machine-readable slug for the status code. It is +// the "code" field of the JSON error envelope, so callers (CI, agents) can +// branch on the failure class without string-matching Message. +func (e *APIError) Code() string { + switch e.StatusCode { + case http.StatusBadRequest: + return "bad_request" + case http.StatusUnauthorized: + return "unauthorized" + case http.StatusForbidden: + return "forbidden" + case http.StatusNotFound: + return "not_found" + case http.StatusConflict: + return "conflict" + case http.StatusTooManyRequests: + return "rate_limited" + } + if e.StatusCode >= 500 { + return "server_error" + } + return "api_error" +} + // ParseAPIError extracts a human-readable message from an API error response body. // // JSend "fail" bodies can shape `data` three different ways: diff --git a/internal/cliargs/hoist.go b/internal/cliargs/hoist.go new file mode 100644 index 0000000..8d0da6e --- /dev/null +++ b/internal/cliargs/hoist.go @@ -0,0 +1,80 @@ +// Package cliargs normalizes the raw process argv before urfave/cli sees it. +package cliargs + +import "strings" + +// globalStringFlags are the app-level flags that take a value. Their value +// may arrive as either "--api-url X" (two tokens) or "--api-url=X" (one). +var globalStringFlags = map[string]bool{ + "--output": true, + "-o": true, + "--api-url": true, + "--api-key": true, + "--sandbox-api-url": true, + "--sandbox-gateway": true, +} + +// globalBoolFlags are the app-level flags that take no value. +var globalBoolFlags = map[string]bool{ + "--debug": true, + "-d": true, +} + +// Hoist moves app-level flags in front of the subcommand so that +// "createos sandbox create --output json" behaves like +// "createos --output json sandbox create". +// +// urfave/cli v2 only parses app-level flags before the first subcommand +// token; anything after it is handed to the subcommand's own flag set and +// fails with "flag provided but not defined". Rewriting argv is the one +// change that fixes every command at once β€” mirroring hidden flags onto each +// command would let them parse but leave their values unread, because the +// App.Before hook that consumes them runs before subcommand parsing. +// +// Everything after a bare "--" is left untouched: those tokens belong to the +// user's own command line (e.g. "sandbox exec box -- ./ci.sh --debug"). +func Hoist(args []string) []string { + if len(args) < 2 { + return args + } + + hoisted := []string{} + rest := []string{} + + for i := 1; i < len(args); i++ { + arg := args[i] + + if arg == "--" { + rest = append(rest, args[i:]...) + break + } + + name, _, hasInlineValue := strings.Cut(arg, "=") + + switch { + case globalBoolFlags[arg]: + hoisted = append(hoisted, arg) + case globalStringFlags[name] && hasInlineValue: + hoisted = append(hoisted, arg) + case globalStringFlags[arg]: + // Value is the next token; take it along, if present. + hoisted = append(hoisted, arg) + if i+1 < len(args) { + i++ + hoisted = append(hoisted, args[i]) + } + default: + rest = append(rest, arg) + } + } + + if len(hoisted) == 0 { + return args + } + + out := make([]string, 0, len(args)) + out = append(out, args[0]) + out = append(out, hoisted...) + out = append(out, rest...) + return out +} diff --git a/internal/cliargs/hoist_test.go b/internal/cliargs/hoist_test.go new file mode 100644 index 0000000..e8cb7a3 --- /dev/null +++ b/internal/cliargs/hoist_test.go @@ -0,0 +1,74 @@ +package cliargs + +import ( + "reflect" + "testing" +) + +func TestHoistGlobalFlags(t *testing.T) { + tests := []struct { + name string + in []string + want []string + }{ + { + name: "flag after subcommand is hoisted", + in: []string{"createos", "sandbox", "create", "--output", "json"}, + want: []string{"createos", "--output", "json", "sandbox", "create"}, + }, + { + name: "inline value is hoisted as one token", + in: []string{"createos", "sandbox", "list", "--output=json"}, + want: []string{"createos", "--output=json", "sandbox", "list"}, + }, + { + name: "bool flag and alias", + in: []string{"createos", "sandbox", "get", "box", "-d", "-o", "json"}, + want: []string{"createos", "-d", "-o", "json", "sandbox", "get", "box"}, + }, + { + name: "already-global order is preserved", + in: []string{"createos", "--output", "json", "sandbox", "list"}, + want: []string{"createos", "--output", "json", "sandbox", "list"}, + }, + { + name: "nothing to hoist returns input untouched", + in: []string{"createos", "sandbox", "list"}, + want: []string{"createos", "sandbox", "list"}, + }, + { + name: "tokens after -- are never hoisted", + in: []string{"createos", "sandbox", "exec", "box", "--", "./ci.sh", "--debug", "--output", "json"}, + want: []string{"createos", "sandbox", "exec", "box", "--", "./ci.sh", "--debug", "--output", "json"}, + }, + { + name: "global before -- is hoisted, passthrough after is not", + in: []string{"createos", "sandbox", "exec", "box", "--output", "json", "--", "env", "-d"}, + want: []string{"createos", "--output", "json", "sandbox", "exec", "box", "--", "env", "-d"}, + }, + { + name: "subcommand flags keep their own values", + in: []string{"createos", "sandbox", "sync", "box", "--exclude", "*.log", "--output", "json"}, + want: []string{"createos", "--output", "json", "sandbox", "sync", "box", "--exclude", "*.log"}, + }, + { + name: "trailing value-less string flag does not panic", + in: []string{"createos", "sandbox", "list", "--output"}, + want: []string{"createos", "--output", "sandbox", "list"}, + }, + { + name: "bare program name", + in: []string{"createos"}, + want: []string{"createos"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := Hoist(tt.in) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("Hoist(%q)\n got: %q\nwant: %q", tt.in, got, tt.want) + } + }) + } +} diff --git a/internal/output/render.go b/internal/output/render.go index 4c7316f..e3ce4cd 100644 --- a/internal/output/render.go +++ b/internal/output/render.go @@ -14,9 +14,33 @@ import ( // FormatKey is the metadata key for the output format. const FormatKey = "output_format" +// FormatExplicitKey records whether the format came from --output rather +// than from TTY detection. +const FormatExplicitKey = "output_format_explicit" + +// IsJSONExplicit reports whether the user actually asked for JSON with +// --output json. Commands whose stdout IS the payload β€” `sandbox exec` +// forwarding a program's own output β€” must use this instead of IsJSON, or +// a plain `… exec box -- cat data.csv > out` would silently write a JSON +// envelope instead of the file the caller expected. +func IsJSONExplicit(c *cli.Context) bool { + explicit, ok := c.App.Metadata[FormatExplicitKey].(bool) + return ok && explicit && IsJSON(c) +} + // IsJSON returns true if the output format is JSON. func IsJSON(c *cli.Context) bool { - if f, ok := c.App.Metadata[FormatKey].(string); ok { + return AppIsJSON(c.App) +} + +// AppIsJSON reports whether the app is in JSON mode. It reads the same +// metadata IsJSON does, but works from the *cli.App alone β€” main.go handles +// errors after Run returns, where no *cli.Context is available. +func AppIsJSON(app *cli.App) bool { + if app == nil { + return false + } + if f, ok := app.Metadata[FormatKey].(string); ok { return f == "json" } return false @@ -36,18 +60,32 @@ func Render(c *cli.Context, data any, tableRenderer func()) { } // RenderError outputs an error as JSON if --output json is set, otherwise returns false. -func RenderError(c *cli.Context, code string, message string) bool { - if !IsJSON(c) { +func RenderError(c *cli.Context, code string, message string, hint string) bool { + return AppRenderError(c.App, code, message, hint) +} + +// AppRenderError is RenderError for callers that only hold the *cli.App. +// The envelope goes to stdout so a JSON consumer reading one stream still +// gets valid JSON on failure; the human-readable path in main.go writes to +// stderr instead. +// +// hint carries the same next-step suggestion humans get (APIError.Hint) and +// is omitted when empty β€” an agent relaying a failure to a user should be +// able to pass on the same advice. +func AppRenderError(app *cli.App, code string, message string, hint string) bool { + if !AppIsJSON(app) { return false } + body := map[string]string{ + "code": code, + "message": message, + } + if hint != "" { + body["hint"] = hint + } enc := json.NewEncoder(os.Stdout) enc.SetIndent("", " ") - if err := enc.Encode(map[string]any{ - "error": map[string]string{ - "code": code, - "message": message, - }, - }); err != nil { + if err := enc.Encode(map[string]any{"error": body}); err != nil { fmt.Fprintln(os.Stderr, err) } return true diff --git a/internal/terminal/tty.go b/internal/terminal/tty.go index c6abafc..a400d72 100644 --- a/internal/terminal/tty.go +++ b/internal/terminal/tty.go @@ -12,3 +12,11 @@ import ( func IsInteractive() bool { return term.IsTerminal(int(os.Stdout.Fd())) // #nosec G115 -- uintptr->int safe on all supported platforms for fd values } + +// HasPipedStdin reports whether stdin is a pipe or a file rather than a +// keyboard, i.e. `echo hi | createos …` or `createos … < file`. Checked +// separately from IsInteractive because the two streams are redirected +// independently β€” a human on a TTY can still pipe data in. +func HasPipedStdin() bool { + return !term.IsTerminal(int(os.Stdin.Fd())) // #nosec G115 -- uintptr->int safe on all supported platforms for fd values +} diff --git a/main.go b/main.go index a31b350..61036d2 100644 --- a/main.go +++ b/main.go @@ -10,6 +10,8 @@ import ( "github.com/NodeOps-app/createos-cli/cmd/root" "github.com/NodeOps-app/createos-cli/internal/api" + "github.com/NodeOps-app/createos-cli/internal/cliargs" + "github.com/NodeOps-app/createos-cli/internal/output" ) func main() { @@ -20,17 +22,27 @@ func main() { app := root.NewApp() - if err := app.Run(os.Args); err != nil { + if err := app.Run(cliargs.Hoist(os.Args)); err != nil { + code, message, hint := "error", err.Error(), "" var apiErr *api.APIError if errors.As(err, &apiErr) { - pterm.Error.Println(apiErr.Message) - if hint := apiErr.Hint(); hint != "" { - pterm.Println(pterm.Gray(" Hint: " + hint)) + code, message, hint = apiErr.Code(), apiErr.Message, apiErr.Hint() + } + + // JSON mode emits a machine-readable envelope on stdout so a + // consumer reading a single stream still parses valid JSON. + // Otherwise the human-readable error goes to stderr, keeping + // stdout clean for data in pipes and CI logs. + if !output.AppRenderError(app, code, message, hint) { + errOut := pterm.Error.WithWriter(os.Stderr) + errOut.Println(message) + if hint != "" { + pterm.Fprintln(os.Stderr, pterm.Gray(" Hint: "+hint)) } - } else { - pterm.Error.Println(err.Error()) - if api.DebugEnabled() { - pterm.Println(pterm.Gray(" debug: " + err.Error())) + // Raw error text is withheld unless the user asked for it: + // Go errors can carry syscall details and local paths. + if apiErr == nil && api.DebugEnabled() { + pterm.Fprintln(os.Stderr, pterm.Gray(" debug: "+err.Error())) } } os.Exit(1)