From d0e77928bea2aab16a988b10e0c637784e0bd88d Mon Sep 17 00:00:00 2001 From: Travis Wu Date: Sat, 12 Sep 2026 14:19:49 +0800 Subject: [PATCH] toolplane: say what a create would make, so a person can approve it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A create takes its flavour, image and network from a file on this node, and the SaaS had no way to learn them. The sentence a person approved therefore said those values were "the cluster's own settings" — true, and not something anyone can consent to. Validating the write path end to end on the 1cc lab R630 showed a VM created and reaching ACTIVE with the approver never told what it would be. describe_instance_profile reports them. It is a control operation: this process answers it from state its own operator wrote, running no command and calling no API, so it is a read that creates nothing and changes nothing. Three properties are worth stating because each was a choice. It reports values and not a sentence. The wording a person reads is product copy that should improve without an agent release on every cluster; the values are facts only this node holds. Each side owns the half it is authoritative for. It is Unlisted — served to the SaaS, never offered to the model. The model already cannot choose a flavour or an image, which is the profile's entire purpose, so advertising a tool that reports them would widen what the model sees without widening what it can do. The flag is published through toolcatalog rather than left for the SaaS to infer, because an exception typed out on that side is the hand-kept copy that package exists to abolish. It reports Configured false for a half-configured profile, not merely for an absent one. A profile missing any of the three refuses every create, so reporting it as configured would put values in front of a person for a call that cannot run. callControl now answers this operation before it looks for a probe runner. The guard it bypasses was written for exactly this case — its comment anticipated "a future control tool that is not scratch-class" — and an agent built without a probe plane still has to be able to say what a create would make. InstanceProfile lives in tunnelproto because both repositories read it: a wire shape only one side can import gets restated on the other, which is how cube-ai-advisor#121 and cube-advisor-agent#24 came to be green while disagreeing about a frame. Signed-off-by: Travis Wu --- internal/toolplane/allowlist.go | 55 ++++++++++-- internal/toolplane/describe_test.go | 129 ++++++++++++++++++++++++++++ internal/toolplane/registry.go | 56 ++++++++++-- pkg/toolcatalog/toolcatalog.go | 15 +++- pkg/toolcatalog/toolcatalog_test.go | 8 ++ pkg/tunnelproto/toolcall.go | 29 +++++++ 6 files changed, 279 insertions(+), 13 deletions(-) create mode 100644 internal/toolplane/describe_test.go diff --git a/internal/toolplane/allowlist.go b/internal/toolplane/allowlist.go index a4d3f5a..c95bbda 100644 --- a/internal/toolplane/allowlist.go +++ b/internal/toolplane/allowlist.go @@ -38,6 +38,8 @@ import ( "fmt" "strings" "time" + + "github.com/bigstack-oss/cube-advisor-agent/pkg/tunnelproto" ) // Impact is what a tool does to the cluster it runs against. It mirrors the @@ -208,8 +210,10 @@ func (b Backend) String() string { return fmt.Sprintf("backend(%d)", int(b)) } -// ControlOp identifies a built-in probe-plane operation. Like Impact it starts -// at 1, so a tool that sets nothing is not accidentally a control tool. +// ControlOp identifies a built-in operation — one this process answers from +// its own state rather than by running a command or calling an API. Like +// Impact it starts at 1, so a tool that sets nothing is not accidentally a +// control tool. type ControlOp int const ( @@ -217,6 +221,9 @@ const ( ControlProbeStart ControlOp = iota + 1 // ControlProbeStatus reports a run's state and, when finished, its metrics. ControlProbeStatus + // ControlInstanceProfile reports what a create would make, for the + // sentence a person approves. + ControlInstanceProfile ) func (c ControlOp) String() string { @@ -225,6 +232,8 @@ func (c ControlOp) String() string { return "probe_start" case ControlProbeStatus: return "probe_status" + case ControlInstanceProfile: + return "describe_instance_profile" } return fmt.Sprintf("control(%d)", int(c)) } @@ -237,6 +246,18 @@ type Tool struct { // Description is for the operator reading the allowlist, not the model. Description string + // Unlisted marks a tool the SaaS calls on its own account and never + // offers to the model. + // + // It is not a security boundary — the executor serves the name to whoever + // holds the tunnel, listed or not, and the level and impact checks are + // what decide whether a call runs. It is an honesty flag for the + // catalogue: without it, a SaaS that compares its model-facing tool list + // against this allowlist must either advertise a tool the model has no + // use for, or carry an exception typed out on that side — which is the + // hand-kept copy toolcatalog exists to abolish. + Unlisted bool + // Argv is the exact command to run. Elements equal to a parameter // placeholder (see Params) are replaced; everything else is literal. // The first element is the executable — resolved from PATH, never a shell. @@ -599,6 +620,23 @@ var Allowlist = []Tool{ // boots afterwards. This bounds the acceptance, not the boot. Timeout: 60 * time.Second, }, + // The create above takes its flavour, image and network from a file on + // this node, and until now the SaaS had no way to learn them. So the + // sentence a person approved said the values were "the cluster's own + // settings" — true, and not something anyone can consent to. This reports + // them, so the sentence can name what will exist. + // + // Read class: it discloses configuration this node's own operator wrote, + // creates nothing and changes nothing. Unlisted, because the model cannot + // choose these values and has no use for them; the SaaS calls it while + // composing an approval prompt. + { + Name: tunnelproto.DescribeInstanceProfile, + Description: "Report the flavour, image and network a create would use, for the approval prompt.", + Control: ControlInstanceProfile, + Impact: ImpactRead, + Unlisted: true, + }, } // ProbeControls is the probe plane's half of the allowlist, kept separate @@ -785,14 +823,17 @@ func (t Tool) validateControl() error { if len(t.Params) > 0 { return fmt.Errorf("tool %q is a control tool and must declare no parameters; its argument is checked by the probe runner, not substituted", t.Name) } - if t.Control != ControlProbeStart && t.Control != ControlProbeStatus { + switch t.Control { + case ControlProbeStart, ControlProbeStatus, ControlInstanceProfile: + default: return fmt.Errorf("tool %q declares an unknown control operation", t.Name) } - // The probe plane creates and destroys its own scratch resources and - // changes no configuration; a control tool claiming a configuring class is - // claiming to be something the probe runner cannot do. + // A control tool answers from this process's own state — a probe run it + // started, a setting its operator wrote. None of that reaches a cluster's + // configuration, so a control tool claiming a configuring class is + // claiming to be something no control op can do. if t.Impact != ImpactRead && t.Impact != ImpactScratch { - return fmt.Errorf("tool %q is a probe-plane control but declares impact %s; the probe plane changes no configuration", t.Name, t.Impact) + return fmt.Errorf("tool %q is a control tool but declares impact %s; a control operation changes no configuration", t.Name, t.Impact) } return nil } diff --git a/internal/toolplane/describe_test.go b/internal/toolplane/describe_test.go new file mode 100644 index 0000000..f0255a6 --- /dev/null +++ b/internal/toolplane/describe_test.go @@ -0,0 +1,129 @@ +package toolplane + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/bigstack-oss/cube-advisor-agent/pkg/tunnelproto" +) + +func describe(t *testing.T, r *Registry) tunnelproto.InstanceProfile { + t.Helper() + out, err := r.Call(context.Background(), tunnelproto.DescribeInstanceProfile, nil) + if err != nil { + t.Fatalf("describe: %v", err) + } + var got tunnelproto.InstanceProfile + if err := json.Unmarshal(out, &got); err != nil { + t.Fatalf("describe returned %s: %v", out, err) + } + return got +} + +// The values a person is asked to approve come from this call, so it has to +// report the ones a create would actually use — not a summary, not a subset. +func TestDescribeReportsTheProfileACreateWouldUse(t *testing.T) { + r, _, _ := newTestRegistry(t) + r.ConfigureInstanceProfile(InstanceProfile{ + Flavor: "m1.large", Image: "ubuntu-24.04", Network: "tenant-net", Project: "acme-prod", + }) + + got := describe(t, r) + want := tunnelproto.InstanceProfile{ + Configured: true, + Flavor: "m1.large", Image: "ubuntu-24.04", Network: "tenant-net", Project: "acme-prod", + } + if got != want { + t.Errorf("describe = %+v, want %+v", got, want) + } +} + +// An agent nobody has configured says so and invents nothing. The caller is +// composing a sentence for a person: empty strings rendered into it would +// promise a machine with no image. +func TestAnUnconfiguredAgentDescribesNoProfile(t *testing.T) { + r, _, _ := newTestRegistry(t) + + got := describe(t, r) + if got.Configured { + t.Errorf("an unconfigured agent reports a profile: %+v", got) + } + if got.Flavor != "" || got.Image != "" || got.Network != "" { + t.Errorf("an unconfigured agent named values: %+v", got) + } +} + +// A half-configured profile refuses every create, so reporting it as +// configured would put values in front of a person for a call that cannot run. +func TestAPartialProfileIsNotReportedAsConfigured(t *testing.T) { + for _, p := range []InstanceProfile{ + {Image: "ubuntu-24.04", Network: "tenant-net"}, + {Flavor: "m1.large", Network: "tenant-net"}, + {Flavor: "m1.large", Image: "ubuntu-24.04"}, + } { + r, _, _ := newTestRegistry(t) + r.ConfigureInstanceProfile(p) + if got := describe(t, r); got.Configured { + t.Errorf("profile %+v reported as configured", p) + } + } +} + +// It answers one question and takes nothing. A caller passing an argument has +// misunderstood what this reports, and ignoring it would let that +// misunderstanding reach an approval prompt. +func TestDescribeRefusesArguments(t *testing.T) { + r, rec, _ := newTestRegistry(t) + r.ConfigureInstanceProfile(InstanceProfile{Flavor: "f", Image: "i", Network: "n"}) + + _, err := r.Call(context.Background(), tunnelproto.DescribeInstanceProfile, + map[string]string{"cluster": "other"}) + if !errors.Is(err, ErrBadArgument) { + t.Fatalf("describe with an argument = %v, want ErrBadArgument", err) + } + if len(rec.calls) == 0 || rec.calls[len(rec.calls)-1].Allowed { + t.Error("the refusal was not audited") + } +} + +// The describing tool reaches no probe runner, and an agent built without one +// still has to say what a create would make. This is the guard on the +// restructure that let a non-probe control op past the runner check. +func TestDescribeWorksWithoutAProbeRunner(t *testing.T) { + r, err := New(Allowlist, &recorder{}) + if err != nil { + t.Fatalf("New: %v", err) + } + if r.probes != nil { + t.Fatal("a registry built without WithProbes has a runner") + } + r.ConfigureInstanceProfile(InstanceProfile{Flavor: "f", Image: "i", Network: "n"}) + + if got := describe(t, r); !got.Configured { + t.Errorf("describe without a probe runner = %+v, want the profile", got) + } +} + +// The model cannot choose a flavour or an image, so a tool reporting them has +// no business in its tool list. Unlisted is what lets the SaaS know that +// absence is deliberate rather than a list gone stale. +func TestDescribeIsNotOfferedToTheModel(t *testing.T) { + var found bool + for _, tool := range Allowlist { + if tool.Name != tunnelproto.DescribeInstanceProfile { + continue + } + found = true + if !tool.Unlisted { + t.Error("describe_instance_profile is listed to the model") + } + if tool.Impact != ImpactRead { + t.Errorf("describe_instance_profile impact = %s, want read", tool.Impact) + } + } + if !found { + t.Fatal("the allowlist has no describe_instance_profile") + } +} diff --git a/internal/toolplane/registry.go b/internal/toolplane/registry.go index c5d77a9..fa5967d 100644 --- a/internal/toolplane/registry.go +++ b/internal/toolplane/registry.go @@ -448,13 +448,20 @@ func (r *Registry) Call(ctx context.Context, name string, args map[string]string return out, runErr } -// callControl dispatches a probe-plane control call. +// callControl dispatches a control call — one this process answers from its +// own state rather than by running a command or calling an API. // -// Both operations are short by construction — starting a probe returns once -// the goroutine is launched, polling one reads a map — so neither needs the -// timeout ladder stretched to fit the measurement it controls. Results go back -// as JSON so the SaaS forwards numbers rather than prose. +// Every operation is short by construction: starting a probe returns once the +// goroutine is launched, polling one reads a map, describing the profile reads +// a struct. None needs the timeout ladder stretched to fit what it reports on. +// Results go back as JSON so the SaaS forwards values rather than prose. func (r *Registry) callControl(ctx context.Context, tool Tool, args map[string]string) ([]byte, error) { + // The profile description is answered before the probe runner is + // considered, because it needs none: an agent with no probe plane still + // has to be able to say what a create would make. + if tool.Control == ControlInstanceProfile { + return r.callDescribeProfile(tool, args) + } if r.probes == nil { // Unreachable: the scratch class is refused at registration without a // runner. Kept so a future control tool that is not scratch-class @@ -509,6 +516,45 @@ func (r *Registry) callControl(ctx context.Context, tool Tool, args map[string]s return nil, fmt.Errorf("%w: %q", ErrUnknownTool, tool.Name) } +// callDescribeProfile reports what a create would make. +// +// It takes no arguments at all, and says so rather than ignoring them: a caller +// passing one has misunderstood what this answers, and silence would let that +// misunderstanding reach an approval prompt. +// +// An unconfigured agent reports Configured false rather than failing. The +// caller is composing a sentence for a person, and "this cluster has not been +// told what to create" is something worth saying — a failure here would leave +// it with nothing and no reason. +func (r *Registry) callDescribeProfile(tool Tool, args map[string]string) ([]byte, error) { + for k := range args { + r.audit.RecordToolCall(ToolCall{ + Tool: tool.Name, Args: args, Allowed: false, + Reason: fmt.Sprintf("unexpected argument %q", k), At: time.Now().UTC(), + }) + return nil, fmt.Errorf("%w: unexpected argument %q", ErrBadArgument, k) + } + p := r.profile + out, err := json.Marshal(tunnelproto.InstanceProfile{ + // A profile is usable only with all three; a half-configured one + // refuses creates, and reporting it as configured would put values in + // an approval prompt for a call that cannot run. + Configured: p.Flavor != "" && p.Image != "" && p.Network != "", + Flavor: p.Flavor, + Image: p.Image, + Network: p.Network, + Project: p.Project, + }) + if err != nil { + return nil, err + } + r.audit.RecordToolCall(ToolCall{ + Tool: tool.Name, Args: args, Allowed: true, + At: time.Now().UTC(), Bytes: len(out), + }) + return out, nil +} + // callGet resolves a Get tool's path and fetches it from cube-cos-api. // // The path is the whole request: the method is GET (the getter offers nothing diff --git a/pkg/toolcatalog/toolcatalog.go b/pkg/toolcatalog/toolcatalog.go index f9176fc..8d10af6 100644 --- a/pkg/toolcatalog/toolcatalog.go +++ b/pkg/toolcatalog/toolcatalog.go @@ -38,6 +38,16 @@ type Entry struct { // comparison that ignored the distinction would demand the two lists // match when they legitimately do not. Probe bool + // Unlisted reports whether this entry is one the SaaS calls on its own + // account rather than offering to the model — describing the instance + // profile for an approval prompt, for instance. + // + // Published for the same reason Probe is. A SaaS comparing its tool list + // against this catalogue has to know which absences are deliberate, and + // the alternative is a list of exceptions typed out over there: a + // hand-kept copy of a fact this side already knows, which is the shape of + // failure this package exists to remove. + Unlisted bool // Reads is the sorted set of catalogue keys this entry accepts, empty for // a tool that is not a catalogue read. @@ -59,7 +69,10 @@ type Entry struct { func Entries() []Entry { out := make([]Entry, 0, len(toolplane.Allowlist)+len(toolplane.ProbeControls)) for _, t := range toolplane.Allowlist { - out = append(out, Entry{Name: t.Name, Impact: t.Impact.String(), Reads: readsOf(t)}) + out = append(out, Entry{ + Name: t.Name, Impact: t.Impact.String(), + Unlisted: t.Unlisted, Reads: readsOf(t), + }) } for _, t := range toolplane.ProbeControls { out = append(out, Entry{Name: t.Name, Impact: t.Impact.String(), Probe: true}) diff --git a/pkg/toolcatalog/toolcatalog_test.go b/pkg/toolcatalog/toolcatalog_test.go index 716c575..5536e00 100644 --- a/pkg/toolcatalog/toolcatalog_test.go +++ b/pkg/toolcatalog/toolcatalog_test.go @@ -29,6 +29,14 @@ func TestEveryAllowlistEntryIsPublished(t *testing.T) { if e.Probe { t.Errorf("%s is not a probe control but is published as one", tool.Name) } + // Whether a tool is offered to the model is the SaaS's only reason to + // tolerate an absence from its own list, so a mismatch here would + // leave that side unable to tell a deliberate omission from a stale + // copy — which is the whole failure this package removes. + if e.Unlisted != tool.Unlisted { + t.Errorf("%s: catalogue says unlisted=%v, allowlist says %v", + tool.Name, e.Unlisted, tool.Unlisted) + } } for _, tool := range toolplane.ProbeControls { if e := got[tool.Name]; !e.Probe { diff --git a/pkg/tunnelproto/toolcall.go b/pkg/tunnelproto/toolcall.go index 24923f5..4cceedb 100644 --- a/pkg/tunnelproto/toolcall.go +++ b/pkg/tunnelproto/toolcall.go @@ -133,3 +133,32 @@ func ReadToolResult(r io.Reader) (ToolResult, error) { // MaxToolOutputBytes bounds a result frame. Generous next to `cluster check` // output, small next to memory exhaustion from a hostile or broken peer. const MaxToolOutputBytes = 1 << 20 + +// DescribeInstanceProfile is the tool the SaaS calls to learn what a create +// would make, so the sentence a person approves can name it. +// +// It is not offered to the model. The model already cannot choose a flavour or +// an image — that is what the profile is for — so advertising a tool that +// reports them would widen what the model sees without widening what it can do. +const DescribeInstanceProfile = "describe_instance_profile" + +// InstanceProfile is what a cluster creates, as the SaaS needs to state it. +// +// It crosses the tool channel as that tool's result. Defined here because both +// repositories read it: the executor renders it from the file its operator +// wrote, the SaaS renders a sentence from it. A shape only one side could +// import gets restated on the other, which is how two definitions of one +// protocol come to disagree — cube-ai-advisor#121 and cube-advisor-agent#24 +// were both green while doing exactly that. +// +// Configured separates "this cluster has no profile" from "its fields are +// empty". Those are different things to tell a person: the first is an operator +// who has not opted in, the second would be a bug. A caller reading only the +// fields could not tell them apart. +type InstanceProfile struct { + Configured bool `json:"configured"` + Flavor string `json:"flavor,omitempty"` + Image string `json:"image,omitempty"` + Network string `json:"network,omitempty"` + Project string `json:"project,omitempty"` +}