diff --git a/cmd/pilotctl/appstore.go b/cmd/pilotctl/appstore.go index eff24879..32c37079 100644 --- a/cmd/pilotctl/appstore.go +++ b/cmd/pilotctl/appstore.go @@ -1274,6 +1274,13 @@ func cmdAppStoreInstall(args []string) { // not fatal — the install itself already succeeded on disk. writeInstallAudit(finalDir, m.ID, m.AppVersion, m.Binary.SHA256, bundleDir, force) + // Cache the app's next-steps graph out of the sha-verified catalogue + // metadata, so every later `appstore call` can render its hints from a + // local file instead of reaching for the network on the hot path. Wholly + // best-effort: a sideload, an app with no graph, or an unreachable + // catalogue simply means no hints — never a failed install. + cacheNextSteps(finalDir, fetchNextStepsForInstall(m.ID)) + // Mirror to the install-root-level pilotctl-audit log too, so // the install+uninstall lifecycle pair stays reconstructable // after the app dir (and its per-app supervisor.log) is gone. @@ -2198,6 +2205,13 @@ func cmdAppStoreCall(args []string) { hint = fmt.Sprintf("app %q exposes: %v", appID, methods) } } + // A FAILED call is the highest-value moment to say what to do next: + // this is where an agent otherwise gives up on the app for good. The + // graph turns the app's own error into the fix — 402 into "top up", + // 401 into "signup first", a missing param into the schema. Handed to + // fatalHint (rather than printed here) so the steps land AFTER the + // error text they resolve; fatalHint owns the exit. + exitNextSteps = renderNextSteps(appID, method, false, err.Error()) fatalHint("ipc_error", hint, "%v", err) } @@ -2212,6 +2226,22 @@ func cmdAppStoreCall(args []string) { _ = client.Send(telemetry.Event{Kind: "app_usage", Payload: json.RawMessage(payload)}) } + // A SUCCESSFUL call is where an app is won or lost: the agent has a result + // and no idea the flow continues. Resolve the graph now and print it via + // defer so it lands after the result on every return path below — and, + // crucially, on stderr, so stdout stays the pure JSON that agents pipe to jq. + // + // The RESULT BODY is the match payload, not just the outcome: a soft-failed + // gateway ({"needs_signup":true, exit 0}) is indistinguishable from a real + // result without reading it, and that case — "sign up before anything + // works" — is the one an agent most needs told. + // + // Resolved BEFORE maybeInterceptOutput deliberately: the review prompt + // replaces the result wholesale, and matching a graph against + // "consider leaving a review for ..." would silently drop the real hint on + // whatever fraction of calls the prompt rolls. + edge := renderNextSteps(appID, method, true, string(result)) + // Maybe replace the real result with a review prompt (gated by // appstore.review_prompt feature flag + random roll). replaced, intercepted := maybeInterceptOutput(result, appID) @@ -2219,6 +2249,22 @@ func cmdAppStoreCall(args []string) { result = replaced } + if edge != nil { + defer func() { + if jsonOutput { + // In --json mode the caller is parsing, not reading: give it + // structured steps on stderr rather than prose to re-parse. + if env := nextStepsEnvelope(edge); env != nil { + if b, err := json.Marshal(env); err == nil { + fmt.Fprintln(os.Stderr, string(b)) + } + } + return + } + printNextSteps(edge) + }() + } + if jsonOutput { _, _ = os.Stdout.Write(result) _, _ = os.Stdout.Write([]byte("\n")) diff --git a/cmd/pilotctl/appstore_metadata.go b/cmd/pilotctl/appstore_metadata.go index 43737a56..ad732412 100644 --- a/cmd/pilotctl/appstore_metadata.go +++ b/cmd/pilotctl/appstore_metadata.go @@ -62,6 +62,13 @@ type appMetadata struct { // doc, so it adds no new trust surface. ProductDemo *ProductDemo `json:"product_demo,omitempty"` + // NextSteps is the app's next-steps graph: the dynamic context pilotctl + // renders after every `appstore call`. Install caches it to + // $APP/next-steps.json so the call path stays a local read (see + // appstore_nextsteps.go). Optional — absent means no hints, which is how + // every app that predates the feature degrades. + NextSteps *nextStepsGraph `json:"next_steps,omitempty"` + // Reviews is RESERVED for the future community-reviews service. It is // parsed if present but pilotctl never writes it today — reviews are a // separate, signed, dynamic service (see catalogue/README.md), not diff --git a/cmd/pilotctl/appstore_nextsteps.go b/cmd/pilotctl/appstore_nextsteps.go new file mode 100644 index 00000000..6f609348 --- /dev/null +++ b/cmd/pilotctl/appstore_nextsteps.go @@ -0,0 +1,397 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// Call-time "next steps" rendering — the dynamic context an agent sees after +// EVERY `pilotctl appstore call`, on success and on failure. +// +// The problem: an autonomous agent installs an app and then stops caring. The +// product demo (appstore_demo.go) addresses the install→first-call step, once. +// This addresses every call after it: when a call lands, the agent is told the +// small set of RECOMMENDED commands for where it now stands — the next step in +// the flow on success, and the specific fix on failure (402 → top up, 401 → +// signup, missing param → the schema). +// +// # Where the data comes from, and why the call path never touches the network +// +// The graph is authored in submission.json beside product_demo and flows +// verbatim into the catalogue's metadata.json, whose bytes are pinned by +// metadata_sha256 under the catalogue signature. At install we cache the graph +// to $APP/next-steps.json (see cacheNextSteps). Every call then resolves against +// that local file: no fetch, no DNS, no added latency, and it works offline. A +// call is the hottest path in the app store — it must not grow a network +// dependency to print a hint. +// +// # Trust +// +// The cached bytes came out of the sha-verified metadata.json, so this adds no +// new trust surface. $APP is already the app's own confined directory holding +// its signed manifest; a graph is strictly less privileged than the manifest +// sitting next to it — it produces text, never an action. Nothing here ever +// executes a recommended command; it prints it for the agent to decide on. +// +// # Failure posture +// +// Everything is additive and best-effort, and every entry point is guarded: an +// absent, unreadable, malformed, future-schema, or simply unmatched graph means +// the call behaves EXACTLY as it does today. A hint is a nicety; a call is not. +// Under no circumstances may this file's code turn a working call into a failed +// one — hence the recover() in renderNextSteps and the silent error drops. +package main + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "regexp" + "strings" +) + +// nextStepsFileName is the per-app cache written at install, read at call. +const nextStepsFileName = "next-steps.json" + +// maxNextStepsBytes bounds the cached graph read. A graph is a flow summary of a +// few KB; anything near this ceiling is not a graph and is not worth parsing on +// the call path. +const maxNextStepsBytes = 256 << 10 + +// maxThenRendered hard-caps recommendations per edge at render time, even if a +// graph somehow shipped with more (validation is authoring-side and this is a +// different binary — never trust the file to have been gated). +const maxThenRendered = 3 + +// nextStepsGraph mirrors the FROZEN `next_steps` object authors write in +// submission.json and that flows verbatim into catalogue metadata.json. It is a +// deliberate duplicate of app-template's internal/nextsteps types — the same +// call the ProductDemo mirror makes in appstore_demo.go. pilotctl must not +// depend on the app-template module, and a frozen wire contract is exactly the +// thing it is safe to restate. +type nextStepsGraph struct { + Schema int `json:"schema"` + App string `json:"app"` + Edges []nextStepsEdge `json:"edges"` +} + +// nextStepsEdge is one rule: "the agent ran From and it went On — recommend Then". +type nextStepsEdge struct { + From string `json:"from"` + On string `json:"on"` + // Match is a regex over the call's OUTCOME PAYLOAD: the error text on + // failure, the JSON RESULT BODY on success. + // + // Matching the success body is load-bearing, not a nicety. The most + // important case in the feature — "you must call .signup first" — is not + // an error: the scaffold's requireKey wrapper soft-fails an unauthenticated + // call with exit 0 and + // {"ok":false,"needs_signup":true,"activate":"primitive.signup"} + // so an outcome-only matcher would see a plain success and stay silent, + // precisely for the cold agent who most needs the gateway named. + Match string `json:"match,omitempty"` + Code int `json:"code,omitempty"` + Why string `json:"why,omitempty"` + Then []nextStepsStep `json:"then"` +} + +// nextStepsStep is one recommended command and the reason to run it. +type nextStepsStep struct { + Cmd string `json:"cmd"` + Why string `json:"why"` + Kind string `json:"kind,omitempty"` +} + +const ( + nextStepsWildcard = "*" + nextStepsOutcomeOK = "ok" + nextStepsOutcomeErr = "err" + nextStepsKindGate = "gateway" + nextStepsKindRecov = "recovery" +) + +// nextStepsStatusRe reads the backend status out of an adapter error. The +// generated HTTP adapter formats every non-2xx as +// +// backend: POST /v1/run -> 402: {"error":"insufficient credit ..."} +// +// (internal/scaffold/templates/client_http.go.tmpl), so the status is present in +// the text today even though there is no structured error code on the wire yet. +// This regex is the bridge; when apps grow a machine-readable code, this is the +// only place that has to learn about it. +var nextStepsStatusRe = regexp.MustCompile(`->\s*(\d{3})\b`) + +// nextStepsDisabled reports whether the operator turned the feature off. +// PILOT_NEXT_STEPS=off is the escape hatch for anyone who finds the banner +// noisy in a tight loop — an unconditional per-call print with no off switch is +// how a helpful feature becomes one people route around. +func nextStepsDisabled() bool { + switch strings.ToLower(strings.TrimSpace(os.Getenv("PILOT_NEXT_STEPS"))) { + case "off", "0", "false", "no": + return true + } + return false +} + +// loadNextStepsGraph reads the cached graph for an app. Every failure returns +// nil: a missing cache is the NORMAL case (an app installed before this feature, +// or one that never published a graph), not an error worth a word to the user. +func loadNextStepsGraph(appID string) *nextStepsGraph { + // appID arrives straight from argv, so filepath.Join alone is not enough: + // `pilotctl appstore call ../../etc/x ...` would resolve outside the install + // root and we would happily read (and print) whatever we found. resolveUnder + // is the same containment guard install and the supervisor use. + dir, err := resolveUnder(appStoreRoot(), appID) + if err != nil { + return nil + } + path := filepath.Join(dir, nextStepsFileName) + f, err := os.Open(path) // #nosec G304 -- path is confined to appStoreRoot() by resolveUnder above + if err != nil { + return nil + } + defer f.Close() + st, err := f.Stat() + if err != nil || st.Size() > maxNextStepsBytes { + return nil + } + var g nextStepsGraph + if err := json.NewDecoder(f).Decode(&g); err != nil { + return nil + } + // Schema 1 is the only shape this binary understands. A newer graph must be + // IGNORED rather than half-read: printing a guessed-at hint is worse than + // printing none. + if g.Schema != 1 || g.App != appID { + return nil + } + return &g +} + +// resolveNextStepsEdge picks the single best edge for a completed call, or nil +// when the graph has nothing to say (the common, silent case). +// +// Specificity decides, not file order. The governing rule: +// +// AN EDGE THAT MATCHED THE ACTUAL SITUATION BEATS ONE THAT MERELY MATCHED +// THE METHOD NAME. +// +// So every discriminated edge outranks every undiscriminated one, however exact +// its From: +// +// from exact + code (7) +// * + code (6) +// from exact + match (5) +// * + match (4) +// from exact (1) "you called this method", nothing more +// * (0) the catch-all +// +// The gap between 4 and 1 is load-bearing. A signup app's gateway edge is +// `*` + match on {"needs_signup":true} (the soft-fail is exit 0, so it cannot key +// on the outcome). If a bare From-exact flow edge could outrank it, a COLD agent +// calling primitive.send_email would be told "now read your inbox" instead of +// "sign up first" — the exact failure this feature exists to prevent. +// +// This MUST stay in lockstep with nextsteps.Graph.Resolve in app-template, which +// validates the graphs this resolves; TestResolveMatchesAppTemplateSemantics +// pins the shared cases. +func resolveNextStepsEdge(g *nextStepsGraph, method string, ok bool, payload string) *nextStepsEdge { + if g == nil { + return nil + } + want := nextStepsOutcomeErr + if ok { + want = nextStepsOutcomeOK + } + best, bestScore := -1, -1 + for i := range g.Edges { + e := &g.Edges[i] + if e.On != want { + continue + } + score := 0 + switch e.From { + case method: + score++ // a named method is the WEAKEST signal — see the ordering above + case nextStepsWildcard: + default: + continue // names a different method + } + // An edge that declares a discriminator and does not match is not + // applicable at all — it must NOT fall back to winning on its From + // bonus, or an unrelated failure would print a confidently wrong fix. + if e.Code != 0 { + if !nextStepsMatchesCode(payload, e.Code) { + continue + } + score += 6 // matched the situation, and pinned an exact status + } else if e.Match != "" { + re, err := regexp.Compile("(?i)" + e.Match) + if err != nil || !re.MatchString(payload) { + continue + } + score += 4 // matched the situation + } + if score > bestScore { + bestScore, best = score, i + } + } + if best < 0 { + return nil + } + return &g.Edges[best] +} + +func nextStepsMatchesCode(payload string, code int) bool { + m := nextStepsStatusRe.FindStringSubmatch(payload) + return m != nil && m[1] == fmt.Sprintf("%d", code) +} + +// renderNextStepsText builds the block printed after a call. The budget is +// brutal because this lands after EVERY call: a few lines, or the agent learns +// to skip it and the feature has made things worse than silence. +// +// next: budget exhausted — top up before retrying +// 1. pilotctl appstore call io.pilot.wallet wallet.balance '{}' +// why: check remaining USDC (fixes the error above) +func renderNextStepsText(e *nextStepsEdge) string { + if e == nil || len(e.Then) == 0 { + return "" + } + var b strings.Builder + if w := oneLineNS(e.Why); w != "" { + fmt.Fprintf(&b, "next: %s\n", w) + } else { + b.WriteString("next:\n") + } + for i, s := range e.Then { + if i >= maxThenRendered { + break + } + cmd := strings.TrimSpace(s.Cmd) + if cmd == "" { + continue + } + fmt.Fprintf(&b, " %d. %s\n", i+1, cmd) + if w := oneLineNS(s.Why); w != "" { + fmt.Fprintf(&b, " why: %s%s\n", w, nextStepsKindSuffix(s.Kind)) + } + } + return b.String() +} + +// nextStepsKindSuffix marks the two kinds an agent must not read past: a gateway +// is non-optional, and a recovery step is the fix for the error just printed. +// The ordinary forward step is unmarked — "the next useful thing" needs no label. +func nextStepsKindSuffix(kind string) string { + switch kind { + case nextStepsKindGate: + return " (required first)" + case nextStepsKindRecov: + return " (fixes the error above)" + default: + return "" + } +} + +func oneLineNS(s string) string { return strings.Join(strings.Fields(s), " ") } + +// exitNextSteps is drained by fatalHint so a FAILED call prints its recovery +// steps after the error, not before it. fatalHint owns the process exit, so a +// caller cannot print anything after it — the same reason motd's +// importantUpdate is a package var consumed there. Set it immediately before +// fatalHint; it is inert everywhere else. +var exitNextSteps *nextStepsEdge + +// renderNextSteps resolves and returns the block for a completed call, and is +// the ONLY entry point the call path uses. It is recover()-guarded and +// fail-silent by construction: a hint is a nicety, a call is not, and no bug in +// graph handling may ever turn a working call into a failed one. +func renderNextSteps(appID, method string, ok bool, payload string) *nextStepsEdge { + if nextStepsDisabled() { + return nil + } + var edge *nextStepsEdge + func() { + defer func() { + if r := recover(); r != nil { + edge = nil + } + }() + edge = resolveNextStepsEdge(loadNextStepsGraph(appID), method, ok, payload) + }() + return edge +} + +// printNextSteps writes the block for a SUCCESSFUL call to stderr. +// +// stderr, never stdout — this is the load-bearing decision in the whole file. +// stdout carries the call's JSON result and agents pipe it straight into jq; +// one line of prose there corrupts the parse and breaks the very workflow this +// feature exists to encourage. stderr is still captured by every agent harness, +// so the hint is seen without ever touching the data contract. +func printNextSteps(e *nextStepsEdge) { + if txt := renderNextStepsText(e); txt != "" { + fmt.Fprint(os.Stderr, txt) + } +} + +// nextStepsJSON is the machine-readable form attached to --json output, so an +// agent parsing the envelope gets structured steps rather than prose to re-parse. +type nextStepsJSON struct { + Why string `json:"why,omitempty"` + Steps []nextStepsStep `json:"steps"` +} + +func nextStepsEnvelope(e *nextStepsEdge) *nextStepsJSON { + if e == nil || len(e.Then) == 0 { + return nil + } + steps := e.Then + if len(steps) > maxThenRendered { + steps = steps[:maxThenRendered] + } + return &nextStepsJSON{Why: oneLineNS(e.Why), Steps: steps} +} + +// cacheNextSteps writes the graph from the (already sha-verified) catalogue +// metadata into the installed app dir, so the call path is a local read. +// +// Best-effort by design: a failure here must never fail an install that has +// otherwise fully succeeded. The cost of no cache is no hints; the cost of a +// failed install is a broken app. +func cacheNextSteps(appDir string, g *nextStepsGraph) { + if g == nil || len(g.Edges) == 0 { + return + } + data, err := json.MarshalIndent(g, "", " ") + if err != nil { + return + } + // Confine the write to the install root for the same reason the read is + // confined: appDir is derived from an app id, and a graph is never worth + // writing a byte outside the tree the app store owns. + out, err := resolveUnder(appStoreRoot(), filepath.Base(appDir)) + if err != nil { + return + } + _ = os.WriteFile(filepath.Join(out, nextStepsFileName), data, 0o600) // #nosec G304 -- confined by resolveUnder +} + +// fetchNextStepsForInstall pulls the graph out of the catalogue metadata for an +// app id. Best-effort and quiet: an app with no catalogue entry (a sideload), no +// metadata_url, or an unreachable host simply gets no graph. It is called once +// per install, never on the call path. +func fetchNextStepsForInstall(appID string) *nextStepsGraph { + c, err := loadCatalogue() + if err != nil { + return nil + } + for i := range c.Apps { + if c.Apps[i].ID != appID { + continue + } + m, err := loadAppMetadata(c.Apps[i]) + if err != nil || m == nil { + return nil + } + return m.NextSteps + } + return nil +} diff --git a/cmd/pilotctl/appstore_nextsteps_test.go b/cmd/pilotctl/appstore_nextsteps_test.go new file mode 100644 index 00000000..66f6bd29 --- /dev/null +++ b/cmd/pilotctl/appstore_nextsteps_test.go @@ -0,0 +1,372 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package main + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// The error strings below are VERBATIM, captured from a live daemon on this +// host or taken from the generating template. They are the whole reason the +// matching works, so they are pinned here rather than paraphrased — if an +// adapter changes its wording, these tests fail and tell us the graphs need +// re-matching before agents get a wrong hint. +const ( + // Captured live: pilotctl appstore call io.pilot.sqlite sqlite.query '{"sql":"SELECT 42"}' + liveMissingParamErr = `ipc: server error: backend: missing required param(s): database` + // Captured live: an unknown method on a real app. + liveMethodNotFoundErr = `ipc: server error: method not found: sqlite.nosuch` + // Shape from internal/scaffold/templates/client_http.go.tmpl ("backend: %s %s -> %d: %s") + // with the body from internal/broker/broker.go's exhausted-budget branch. + liveX402Err = `ipc: server error: backend: POST /v1/run -> 402: {"error":"insufficient credit — per-user budget exhausted","credits_remaining":0}` + liveQuotaErr = `ipc: server error: backend: POST /v1/run -> 429: {"error":"per-caller quota exceeded"}` +) + +func writeGraph(t *testing.T, root, appID string, body string) { + t.Helper() + dir := filepath.Join(root, appID) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, nextStepsFileName), []byte(body), 0o600); err != nil { + t.Fatal(err) + } +} + +const testGraphJSON = `{ + "schema": 1, + "app": "io.pilot.testapp", + "edges": [ + {"from": "*", "on": "err", "code": 402, "why": "budget exhausted", + "then": [{"cmd": "pilotctl appstore call io.pilot.testapp testapp.balance '{}'", "why": "check balance", "kind": "recovery"}]}, + {"from": "*", "on": "err", "match": "no api key|401", "why": "not signed up", + "then": [{"cmd": "pilotctl appstore call io.pilot.testapp testapp.signup '{}'", "why": "mint your key", "kind": "gateway"}]}, + {"from": "testapp.signup", "on": "ok", "why": "signed up", + "then": [{"cmd": "pilotctl appstore call io.pilot.testapp testapp.send '{}'", "why": "send your first", "kind": "flow"}]}, + {"from": "*", "on": "ok", "why": "catch-all", + "then": [{"cmd": "pilotctl appstore call io.pilot.testapp testapp.help '{}'", "why": "see everything"}]} + ] +}` + +func withAppRoot(t *testing.T) string { + t.Helper() + root := t.TempDir() + t.Setenv("PILOT_APPSTORE_ROOT", root) + t.Setenv("PILOT_NEXT_STEPS", "") + return root +} + +func TestLoadNextStepsGraph(t *testing.T) { + root := withAppRoot(t) + writeGraph(t, root, "io.pilot.testapp", testGraphJSON) + g := loadNextStepsGraph("io.pilot.testapp") + if g == nil || len(g.Edges) != 4 { + t.Fatalf("want a 4-edge graph, got %+v", g) + } +} + +// Every one of these is a NORMAL condition, not an error: the overwhelming +// majority of installed apps have no graph at all. Each must be silent. +func TestLoadNextStepsGraphDegradesSilently(t *testing.T) { + root := withAppRoot(t) + if g := loadNextStepsGraph("io.pilot.absent"); g != nil { + t.Error("a missing graph must load as nil") + } + writeGraph(t, root, "io.pilot.bad", `{not json`) + if g := loadNextStepsGraph("io.pilot.bad"); g != nil { + t.Error("malformed JSON must load as nil, never panic") + } + // A FUTURE schema must be ignored rather than half-read: a guessed hint is + // worse than no hint. + writeGraph(t, root, "io.pilot.future", `{"schema":2,"app":"io.pilot.future","edges":[]}`) + if g := loadNextStepsGraph("io.pilot.future"); g != nil { + t.Error("an unknown schema must load as nil") + } + // A graph claiming to be another app must not apply to this one. + writeGraph(t, root, "io.pilot.mine", `{"schema":1,"app":"io.pilot.theirs","edges":[]}`) + if g := loadNextStepsGraph("io.pilot.mine"); g != nil { + t.Error("a graph whose app id does not match its dir must load as nil") + } +} + +func TestRenderNextStepsMatchesLiveX402(t *testing.T) { + root := withAppRoot(t) + writeGraph(t, root, "io.pilot.testapp", testGraphJSON) + e := renderNextSteps("io.pilot.testapp", "testapp.run", false, liveX402Err) + if e == nil || e.Why != "budget exhausted" { + t.Fatalf("a live 402 must select the 402 edge, got %+v", e) + } + out := renderNextStepsText(e) + if !strings.Contains(out, "testapp.balance") || !strings.Contains(out, "(fixes the error above)") { + t.Fatalf("render must name the fix and mark it as such:\n%s", out) + } +} + +// The regression that would make EVERY failure claim the budget was exhausted. +func TestRenderNextStepsQuotaIsNotBudget(t *testing.T) { + root := withAppRoot(t) + writeGraph(t, root, "io.pilot.testapp", testGraphJSON) + e := renderNextSteps("io.pilot.testapp", "testapp.run", false, liveQuotaErr) + if e != nil && e.Code == 402 { + t.Fatalf("a 429 must not select the 402 edge, got %+v", e) + } +} + +func TestRenderNextStepsGatewayOnLive401(t *testing.T) { + root := withAppRoot(t) + writeGraph(t, root, "io.pilot.testapp", testGraphJSON) + e := renderNextSteps("io.pilot.testapp", "testapp.send", false, "ipc: server error: backend: GET /v1/me -> 401: no api key") + if e == nil || e.Why != "not signed up" { + t.Fatalf("a 401 must route to the signup gateway, got %+v", e) + } + if !strings.Contains(renderNextStepsText(e), "(required first)") { + t.Fatal("a gateway step must be marked required first") + } +} + +func TestRenderNextStepsPrefersExactMethod(t *testing.T) { + root := withAppRoot(t) + writeGraph(t, root, "io.pilot.testapp", testGraphJSON) + e := renderNextSteps("io.pilot.testapp", "testapp.signup", true, "") + if e == nil || e.Why != "signed up" { + t.Fatalf("want the exact-method ok edge, got %+v", e) + } + e = renderNextSteps("io.pilot.testapp", "testapp.other", true, "") + if e == nil || e.Why != "catch-all" { + t.Fatalf("want the wildcard ok edge, got %+v", e) + } +} + +// An app with no graph — the state of every app today — must produce nothing. +func TestRenderNextStepsSilentWithoutGraph(t *testing.T) { + withAppRoot(t) + if e := renderNextSteps("io.pilot.nograph", "x.y", true, ""); e != nil { + t.Fatal("an app with no graph must render nothing") + } +} + +func TestRenderNextStepsOffSwitch(t *testing.T) { + root := withAppRoot(t) + writeGraph(t, root, "io.pilot.testapp", testGraphJSON) + for _, v := range []string{"off", "0", "false", "no", "OFF"} { + t.Setenv("PILOT_NEXT_STEPS", v) + if e := renderNextSteps("io.pilot.testapp", "testapp.signup", true, ""); e != nil { + t.Errorf("PILOT_NEXT_STEPS=%q must disable hints", v) + } + } +} + +// A live method-not-found already gets pilotctl's own `exposes` hint. The graph +// must not also fire a misleading edge on it. +func TestRenderNextStepsIgnoresMethodNotFound(t *testing.T) { + root := withAppRoot(t) + writeGraph(t, root, "io.pilot.testapp", testGraphJSON) + e := renderNextSteps("io.pilot.testapp", "testapp.nosuch", false, liveMethodNotFoundErr) + if e != nil { + t.Fatalf("no edge matches a method-not-found; want silence, got %+v", e) + } +} + +func TestRenderNextStepsTextCapsAtThree(t *testing.T) { + e := &nextStepsEdge{Why: "x", Then: []nextStepsStep{ + {Cmd: "pilotctl a", Why: "1"}, {Cmd: "pilotctl b", Why: "2"}, + {Cmd: "pilotctl c", Why: "3"}, {Cmd: "pilotctl d", Why: "4"}, + }} + // Validation is authoring-side and lives in a different binary; the renderer + // must never trust the file to have been gated. + if strings.Contains(renderNextStepsText(e), "pilotctl d") { + t.Fatal("render must hard-cap at three steps regardless of the file") + } +} + +func TestRenderNextStepsTextNilIsEmpty(t *testing.T) { + if renderNextStepsText(nil) != "" { + t.Fatal("a nil edge must render empty") + } + if nextStepsEnvelope(nil) != nil { + t.Fatal("a nil edge must have no envelope") + } +} + +func TestCacheNextStepsRoundTrips(t *testing.T) { + root := withAppRoot(t) + dir := filepath.Join(root, "io.pilot.testapp") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + var g nextStepsGraph + if err := json.Unmarshal([]byte(testGraphJSON), &g); err != nil { + t.Fatal(err) + } + cacheNextSteps(dir, &g) + got := loadNextStepsGraph("io.pilot.testapp") + if got == nil || len(got.Edges) != len(g.Edges) { + t.Fatalf("cached graph did not round-trip: %+v", got) + } +} + +// An install with no graph must not litter the app dir with an empty file that +// later reads as "this app has a graph". +func TestCacheNextStepsNilIsNoOp(t *testing.T) { + root := withAppRoot(t) + dir := filepath.Join(root, "io.pilot.testapp") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + cacheNextSteps(dir, nil) + cacheNextSteps(dir, &nextStepsGraph{Schema: 1, App: "io.pilot.testapp"}) + if _, err := os.Stat(filepath.Join(dir, nextStepsFileName)); !os.IsNotExist(err) { + t.Fatal("caching an absent or empty graph must write no file") + } +} + +func TestNextStepsEnvelopeIsStructured(t *testing.T) { + root := withAppRoot(t) + writeGraph(t, root, "io.pilot.testapp", testGraphJSON) + e := renderNextSteps("io.pilot.testapp", "testapp.run", false, liveX402Err) + b, err := json.Marshal(nextStepsEnvelope(e)) + if err != nil { + t.Fatal(err) + } + var back nextStepsJSON + if err := json.Unmarshal(b, &back); err != nil { + t.Fatal(err) + } + if len(back.Steps) != 1 || back.Steps[0].Kind != "recovery" { + t.Fatalf("envelope must carry structured steps, got %s", b) + } +} + +// TestResolveMatchesAppTemplateSemantics pins the resolution rules this file +// shares with app-template's internal/nextsteps.Graph.Resolve. +// +// The two are deliberate duplicates: pilotctl must not depend on the app-template +// module, so a frozen wire contract is restated here (the same call +// appstore_demo.go's ProductDemo mirror makes). The cost of duplication is DRIFT +// — and drift here is silent and nasty, because a graph would validate at submit +// time against one set of rules and resolve at call time against another, so an +// author would ship an edge that never fires. +// +// These cases are the contract. They are mirrored one-for-one by +// TestResolve* in app-template's internal/nextsteps package; if you change +// matching semantics on either side, BOTH suites must be updated together. +func TestResolveMatchesAppTemplateSemantics(t *testing.T) { + g := &nextStepsGraph{Schema: 1, App: "io.pilot.demoapp", Edges: []nextStepsEdge{ + {From: "*", On: "err", Match: `401|no api key`, Why: "wildcard-match"}, + {From: "*", On: "err", Code: 402, Why: "wildcard-code"}, + {From: "*", On: "ok", Why: "wildcard-ok"}, + {From: "*", On: "ok", Match: `"needs_signup"\s*:\s*true`, Why: "wildcard-ok-match"}, + {From: "demoapp.signup", On: "ok", Why: "exact-ok"}, + {From: "demoapp.send", On: "err", Match: `missing required param`, Why: "exact-err-match"}, + }} + for i := range g.Edges { + g.Edges[i].Then = []nextStepsStep{{Cmd: "pilotctl x", Why: "y"}} + } + + cases := []struct { + name string + method string + ok bool + payload string + want string // edge Why, or "" for no match + }{ + {"exact method beats wildcard on success", "demoapp.signup", true, `{"ok":true}`, "exact-ok"}, + {"unrelated method falls back to wildcard", "demoapp.other", true, `{"ok":true}`, "wildcard-ok"}, + // The signup soft-fail: exit 0, and the discriminator is in the BODY. + {"needs_signup body beats bare wildcard ok", "demoapp.get", true, `{"needs_signup":true}`, "wildcard-ok-match"}, + // THE case this suite originally missed, which let the two resolvers drift: + // a bare exact-from edge must NOT shadow a wildcard gateway edge that + // actually matched. `demoapp.signup` has an exact ok edge; a cold agent + // calling it with a needs_signup body must still be routed to the gateway. + {"discriminated wildcard beats bare exact-from", "demoapp.signup", true, `{"needs_signup":true}`, "wildcard-ok-match"}, + {"real 402 selects the code edge", "demoapp.run", false, liveX402Err, "wildcard-code"}, + {"429 does not select the 402 edge", "demoapp.run", false, liveQuotaErr, ""}, + {"exact method + match wins", "demoapp.send", false, liveMissingParamErr, "exact-err-match"}, + // A declared-but-unmatched discriminator is not a fallback. + {"unmatched discriminator yields silence", "demoapp.send", false, "connection refused", ""}, + {"case-insensitive match", "demoapp.x", false, "GET /v1/me -> 401: No API Key", "wildcard-match"}, + {"no match at all is silent", "demoapp.x", false, "novel error", ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + e := resolveNextStepsEdge(g, c.method, c.ok, c.payload) + got := "" + if e != nil { + got = e.Why + } + if got != c.want { + t.Errorf("resolve(%q, ok=%v) = %q, want %q", c.method, c.ok, got, c.want) + } + }) + } +} + +// The live missing-param error is the single most common way an agent's first +// real call fails, so a graph must be able to key off it exactly. +func TestResolveOnLiveMissingParamError(t *testing.T) { + g := &nextStepsGraph{Schema: 1, App: "io.pilot.sqlite", Edges: []nextStepsEdge{ + {From: "*", On: "err", Match: `missing required param\(s\): database`, Why: "needs a database", + Then: []nextStepsStep{{Cmd: "pilotctl appstore call io.pilot.sqlite sqlite.query '{\"database\":\":memory:\",\"sql\":\"SELECT 1\"}'", Why: "pass database", Kind: "recovery"}}}, + }} + if e := resolveNextStepsEdge(g, "sqlite.query", false, liveMissingParamErr); e == nil { + t.Fatal("the live missing-param error must match its edge") + } + if e := resolveNextStepsEdge(g, "sqlite.query", false, "some other failure"); e != nil { + t.Fatal("an unrelated error must not match") + } +} + +// TestLoadNextStepsGraphRefusesTraversal is a security regression test. appID +// comes straight from argv (`pilotctl appstore call ...`), so a +// filepath.Join on it is a path traversal: gosec flagged exactly this as G703. +// Without resolveUnder, a crafted id would make pilotctl read a file outside the +// install root and print pieces of it back as "next steps". +func TestLoadNextStepsGraphRefusesTraversal(t *testing.T) { + root := withAppRoot(t) + + // Plant a valid graph OUTSIDE the install root, and a traversal that would + // reach it if the id were joined naively. + outside := t.TempDir() + rel, err := filepath.Rel(root, outside) + if err != nil { + t.Skipf("no relative path between temp dirs: %v", err) + } + // The planted graph's `app` MUST equal the traversing id, or the + // app-id-mismatch check rejects the file first and this test passes for the + // wrong reason — masking the very traversal it exists to catch. (It did: + // re-introducing the naive filepath.Join left the first draft of this test + // green.) With `app` matching, the ONLY thing that can stop the load is the + // containment guard. + escaped := strings.Replace(testGraphJSON, "io.pilot.testapp", rel, 1) + if err := os.WriteFile(filepath.Join(outside, nextStepsFileName), []byte(escaped), 0o600); err != nil { + t.Fatal(err) + } + if g := loadNextStepsGraph(rel); g != nil { + t.Fatalf("a traversing app id (%q) must not load a graph from outside the install root", rel) + } + + for _, bad := range []string{"../evil", "../../etc", "/etc/passwd", ""} { + if g := loadNextStepsGraph(bad); g != nil { + t.Errorf("app id %q must not resolve to a graph", bad) + } + } +} + +// The write side is confined for the same reason: a graph is never worth writing +// a byte outside the tree the app store owns. +func TestCacheNextStepsRefusesTraversal(t *testing.T) { + root := withAppRoot(t) + outside := t.TempDir() + var g nextStepsGraph + if err := json.Unmarshal([]byte(testGraphJSON), &g); err != nil { + t.Fatal(err) + } + cacheNextSteps(filepath.Join(root, "..", filepath.Base(outside)), &g) + if _, err := os.Stat(filepath.Join(outside, nextStepsFileName)); err == nil { + t.Fatal("cacheNextSteps wrote outside the install root") + } +} diff --git a/cmd/pilotctl/main.go b/cmd/pilotctl/main.go index eddb96c3..29abb7e6 100644 --- a/cmd/pilotctl/main.go +++ b/cmd/pilotctl/main.go @@ -188,7 +188,7 @@ func classifyDaemonError(err error) string { func fatalHint(code, hint, format string, args ...interface{}) { msg := fmt.Sprintf(format, args...) if jsonOutput { - env := map[string]string{ + env := map[string]any{ "status": "error", "code": code, "message": msg, @@ -198,10 +198,19 @@ func fatalHint(code, hint, format string, args ...interface{}) { if importantUpdate != "" { env["important_update"] = importantUpdate } + // Structured recovery steps for a failed `appstore call` (nil for every + // other exit). Additive: existing keys keep their shape and value. + if ns := nextStepsEnvelope(exitNextSteps); ns != nil { + env["next_steps"] = ns + } b, _ := json.Marshal(env) fmt.Fprintln(os.Stderr, string(b)) } else { fmt.Fprintf(os.Stderr, "error: %s\nhint: %s\n", msg, hint) + // Printed last, after the error it resolves — the reason this is a + // package var drained here rather than a print at the call site: only + // fatalHint knows where the output ends, because only it exits. + printNextSteps(exitNextSteps) } os.Exit(1) }