From 6896792bedd93c6cc6df5a5186bb2394e9af9fdc Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Wed, 28 Oct 2026 09:49:29 -0400 Subject: [PATCH 1/2] feat: an operation names the tool it is offered as MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A marked read was offered to a model under its operationId, which this runtime builds as OperationPrefix_Verb_Path. Wiki's first marked artifact (obin-ai/core-solutions#457) therefore published `Wiki_Get_V1_Wiki_Upload-configuration` as a tool name: it identifies the route and describes nothing, and every solution's tools would have read that way. Operation.Tool is the name instead. It is the one part of the marker the owner says rather than core derives, because it is the one part a model reads. Empty keeps today's behaviour exactly — the name falls back to the operationId — so no existing artifact changes until its owner names something. The name is validated, not sanitised: snake_case within 64 characters, refused at Operation.validate. Core's own toolName keeps whatever it is handed (it admits capitals and hyphens by design, being a sanitiser for method names), so a name typed with either would reach a model as typed, and this is the last place that can still say no. The operationId does not move. Generated clients are named from it, so making one tool read better would rename every caller's method. README and CHANGELOG carry the field, the fallback and why the operationId stays put. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 10 ++++++ README.md | 12 +++++++- operations.go | 31 ++++++++++++++++++- runnable_marker_test.go | 68 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 119 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f86a029..398ddfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changelog +## Unreleased — an operation names the tool it is offered as + +- **`Operation.Tool`** is the model-facing name of a marked read. Empty keeps + the previous behaviour exactly: the name falls back to the operationId. It is + validated as snake_case within 64 characters and refused rather than + sanitised, because core's `toolName` keeps what it is handed, so a capital or + a hyphen would reach a model as typed. +- The operationId does not move. Generated clients are named from it; a nicer + tool name is not worth renaming every caller's method. + ## Unreleased — every read a solution serves is a Runnable operation, and so a tool - **The artifact marks each read (`x-codefly-operation`).** An operation whose diff --git a/README.md b/README.md index 1854c7c..3d919aa 100644 --- a/README.md +++ b/README.md @@ -662,7 +662,17 @@ core's `x-codefly-operation` marker — the proto3 JSON of (`runnable.RouteMarker`): recomputed on an uncertain outcome, core's default bounds, invoked under exactly the scopes the policy requires, for the audience this runtime's hop checks (`Manifest.ID`), presented as a read-only tool named -by the operationId and described by the `Summary`. `codefly generate runnables` +by `Operation.Tool` — or, left empty, by the operationId — and described by the +`Summary`. + +`Operation.Tool` is the one part of the marker the owner says rather than core +derives, because it is the one part a model reads: snake_case, at most 64 +characters, refused at validation rather than sanitised, since core's own +`toolName` keeps whatever it is given. The fallback identifies the route and +describes nothing — `Wiki_Get_V1_Wiki_Upload-configuration` says where a route +is, not what calling it does. The operationId is deliberately not changed to +read better: generated clients are named from it, so moving it would rename +every caller's method to improve one tool's name. `codefly generate runnables` then derives a Runnable package from it, as it does from every method a module serves behind the hop; a `GET`'s query parameters are its input. A mutation is not marked: it is resolved by an effect receipt, read through a receipt route diff --git a/operations.go b/operations.go index e767f26..3c8fb12 100644 --- a/operations.go +++ b/operations.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "reflect" + "regexp" "slices" "strings" "unicode" @@ -74,6 +75,18 @@ type Operation struct { // route and every relayed route carry it, being a protocol and another // module's call rather than this backend's own operation. Exclude bool + // Tool is the name this operation is offered to a model under, when it is + // marked as a Runnable operation at all. It is the one part of the marker + // the owner says rather than core derives, because it is the one part a + // model reads: snake_case, 1..64 characters, starting with a letter. + // + // Left empty the name falls back to the operationId, which is this + // document's own identity for the route and a fine key but a poor word to + // put in front of a model — `Wiki_Get_V1_Wiki_Upload-configuration` says + // where the route is, not what calling it does. The operationId is not + // changed to improve it: generated clients are named from it, so moving it + // would rename every caller's method to make one tool read better. + Tool string // Exactly one of Handler and RequestHandler answers the route. Handler Handler RequestHandler RequestHandler @@ -376,7 +389,7 @@ func swaggerDocument(info InterfaceInfo, ops []Operation, audience string) (map[ if op.Policy != nil { endpoint[PolicyExtension] = op.Policy } - marker, err := operationMarker(op, method, route, id, audience) + marker, err := operationMarker(op, method, route, cmp.Or(op.Tool, id), audience) if err != nil { return nil, err } @@ -456,6 +469,17 @@ func operationResponses(op Operation, definitions map[string]any) map[string]any } } +// toolName is what a model-facing name may be: lowercase letters, digits and +// underscores, starting with a letter. It is core's admitted set narrowed to +// one spelling, so two owners' tools read the same way. +var toolName = regexp.MustCompile(`^[a-z][a-z0-9]*(_[a-z0-9]+)*$`) + +// maxToolName is the longest name a model-facing tool carries, as core bounds +// one (corerunnable.BoundedName over its own maxToolName). +const maxToolName = 64 + +const toolNameSpelling = "lowercase letters and digits in underscore-separated words, starting with a letter" + // validate reports an operation that cannot be rendered because it is // undocumented or unserved. func (o Operation) validate() error { @@ -476,6 +500,11 @@ func (o Operation) validate() error { return fmt.Errorf("operation %s declares two handlers", o.Path) case !slices.Contains(servedMethods, o.verb()): return fmt.Errorf("operation %s declares the method %q: an operation is one of %s", o.Path, o.Method, strings.Join(servedMethods, ", ")) + case o.Tool != "" && (len(o.Tool) > maxToolName || !toolName.MatchString(o.Tool)): + // Held here rather than sanitised: core's toolName keeps whatever it + // is given, so a name typed with a capital or a hyphen would reach a + // model as typed. An owner naming its own tool is told what a name is. + return fmt.Errorf("operation %s names the tool %q: a tool name is 1..64 characters of %s", o.Path, o.Tool, toolNameSpelling) } if err := o.Policy.validate(); err != nil { return fmt.Errorf("operation %s %s %w", o.verb(), o.Path, err) diff --git a/runnable_marker_test.go b/runnable_marker_test.go index aa06e5c..fe1ca8d 100644 --- a/runnable_marker_test.go +++ b/runnable_marker_test.go @@ -39,6 +39,7 @@ func releasesServer() *Server { return New(Manifest{ID: "releases"}).Operations( Operation{Path: "/v1/releases/recorded", Summary: "Read the board as records holds it, without asking GitHub", Callers: "Any member who may read records", Behavior: "The newest recorded pin of every component", + Tool: "read_recorded_board", Response: recordedBoard{}, Policy: reads, Handler: answer}, Operation{Path: "/v1/releases/history", Summary: "Read the recorded tags of one repository and the recorded pins of one component", Callers: "Any member who may read records", Behavior: "Newest first", @@ -225,3 +226,70 @@ func TestAnExcludedReadIsNoOperation(t *testing.T) { t.Fatalf("the same read, not excluded, was not marked (%v): %s", err, doc) } } + +// TestAnOperationNamesTheToolItIsOfferedAs: the one part of the marker a model +// actually reads is the one part the owner says. Left empty it falls back to +// the operationId, which identifies the route and describes nothing, and the +// operationId itself never moves — generated clients are named from it. +func TestAnOperationNamesTheToolItIsOfferedAs(t *testing.T) { + doc := releasesArtifact(t) + var parsed struct { + Paths map[string]map[string]struct { + OperationID string `json:"operationId"` + Marker *struct { + // The marker is proto3 JSON; decoded here as the plain + // document a reader sees, not through the proto type, whose + // enum does not round-trip encoding/json. + Tool struct { + Name string `json:"name"` + Effect string `json:"effect"` + } `json:"tool"` + } `json:"x-codefly-operation"` + } `json:"paths"` + } + if err := json.Unmarshal(doc, &parsed); err != nil { + t.Fatal(err) + } + named := parsed.Paths["/v1/releases/recorded"]["get"] + if named.Marker == nil { + t.Fatal("the named read is marked as a Runnable operation") + } + if got := named.Marker.Tool.Name; got != "read_recorded_board" { + t.Errorf("the tool is offered as %q, not the name its owner gave it", got) + } + if named.OperationID != "Releases_Get_V1_Releases_Recorded" { + t.Errorf("naming the tool moved the operationId to %q, which renames every generated client", named.OperationID) + } + + fallback := parsed.Paths["/v1/releases/history"]["get"] + if fallback.Marker == nil { + t.Fatal("the unnamed read is still marked") + } + if got := fallback.Marker.Tool.Name; got != fallback.OperationID { + t.Errorf("an operation that names no tool falls back to its operationId, got %q", got) + } +} + +// TestAToolNameIsOneSpelling: core's own sanitiser keeps whatever it is given, +// so a name typed with a capital or a hyphen would reach a model as typed. An +// owner naming its own tool is told what a name is, at the one place that can +// still refuse it. +func TestAToolNameIsOneSpelling(t *testing.T) { + for _, name := range []string{"Read_Board", "read-board", "9read", "", "read board", "read_board_"} { + op := Operation{ + Path: "/v1/releases/recorded", Summary: "Read the board", Callers: "Anyone", Behavior: "One object", + Tool: name, Response: recordedBoard{}, Policy: Requires("records.records", "read"), + Handler: func(context.Context, *Gateway) (any, error) { return nil, nil }, + } + err := op.validate() + if name == "" { + if err != nil { + t.Errorf("an operation that names no tool is valid, got %v", err) + } + continue + } + if err == nil { + t.Errorf("the tool name %q was accepted; a model-facing name is one spelling", name) + } + } +} From 40b93cc0f053add305b79f2d11aeea1421bae006 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Wed, 28 Oct 2026 09:58:47 -0400 Subject: [PATCH 2/2] fix: one tool name names one operation, and only a read is named MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the field found two ways a declaration could be accepted and then mean nothing — both the kind of quiet wrong this marker exists to avoid. **Two operations could claim one tool name.** Verified by rendering one: both routes published `tool.name: "same_name"` and nothing refused. A tool name is the identity a MODEL addresses, so two of them cannot share one — and nothing downstream collapses them, because a derived package is keyed by the operationId and Robin's catalogue dedupes on the operation, the contract digest and the policy, never on the word. Refused now where a duplicate operationId already was, naming the tool and both routes. **A name on what is never marked was silently dropped.** `Tool` on a mutation validated fine, the marker came back nil, and the name went nowhere — while reading, to whoever wrote it, as an exposure. Only a read is marked, so only a read may be named: a mutation, a stream, an excluded route and an any-checked-caller route are each refused by name, and each says why it is never a tool. The same operation without a name stays valid, which the test pins: this refuses the name, not the operation. Also verified rather than assumed: core's own maxToolName is 64, the same bound this validates, and BoundedName returns a name of exactly 64 unchanged — so a name that passes here is the name a model sees, which was the whole point of validating instead of sanitising. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 6 ++++ README.md | 7 +++- operations.go | 24 ++++++++++++- runnable_marker_test.go | 74 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 109 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 398ddfe..d928457 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ a hyphen would reach a model as typed. - The operationId does not move. Generated clients are named from it; a nicer tool name is not worth renaming every caller's method. +- **One name names one operation.** Two reads offering one tool name are + refused where a duplicate operationId already is: a tool name is the identity + a model addresses, and nothing downstream dedupes on the word. +- **Only a read is named.** A name on a mutation, a stream, an excluded route + or an any-checked-caller route is refused, because none of them is ever + marked: the name would be silently dropped while reading as an exposure. ## Unreleased — every read a solution serves is a Runnable operation, and so a tool diff --git a/README.md b/README.md index 3d919aa..ca3fc83 100644 --- a/README.md +++ b/README.md @@ -668,7 +668,12 @@ by `Operation.Tool` — or, left empty, by the operationId — and described by `Operation.Tool` is the one part of the marker the owner says rather than core derives, because it is the one part a model reads: snake_case, at most 64 characters, refused at validation rather than sanitised, since core's own -`toolName` keeps whatever it is given. The fallback identifies the route and +`toolName` keeps whatever it is given. One name names one operation, as an +operationId names one route — two reads claiming one name publish two tools a +model cannot tell apart, and nothing downstream collapses them. And only a read +is named, because only a read is marked: a name on a mutation, a stream, an +excluded route or an any-checked-caller route is a declaration that does +nothing and reads as the opposite, so it is refused. The fallback identifies the route and describes nothing — `Wiki_Get_V1_Wiki_Upload-configuration` says where a route is, not what calling it does. The operationId is deliberately not changed to read better: generated clients are named from it, so moving it would rename diff --git a/operations.go b/operations.go index 3c8fb12..fb9ccf4 100644 --- a/operations.go +++ b/operations.go @@ -355,6 +355,7 @@ func swaggerDocument(info InterfaceInfo, ops []Operation, audience string) (map[ } paths := map[string]map[string]any{} ids := map[string]string{} + tools := map[string]string{} for _, op := range ops { if err := op.validate(); err != nil { return nil, err @@ -375,6 +376,18 @@ func swaggerDocument(info InterfaceInfo, ops []Operation, audience string) (map[ return nil, fmt.Errorf("operations %s and %s %s share the operationId %q", other, strings.ToUpper(method), route, id) } ids[id] = strings.ToUpper(method) + " " + route + // A tool name is the identity a MODEL addresses, so it names exactly + // one operation, as an operationId names exactly one route. Two reads + // claiming one name publish two tools a model cannot tell apart, and + // nothing downstream collapses them: a derived package is keyed by the + // operationId, and Robin's catalogue dedupes on the operation, the + // contract digest and the policy — never on the word. + if op.Tool != "" { + if other, taken := tools[op.Tool]; taken { + return nil, fmt.Errorf("operations %s and %s %s both offer the tool %q", other, strings.ToUpper(method), route, op.Tool) + } + tools[op.Tool] = strings.ToUpper(method) + " " + route + } endpoint := map[string]any{ "operationId": id, "summary": op.Summary, @@ -504,7 +517,16 @@ func (o Operation) validate() error { // Held here rather than sanitised: core's toolName keeps whatever it // is given, so a name typed with a capital or a hyphen would reach a // model as typed. An owner naming its own tool is told what a name is. - return fmt.Errorf("operation %s names the tool %q: a tool name is 1..64 characters of %s", o.Path, o.Tool, toolNameSpelling) + return fmt.Errorf("operation %s names the tool %q: a tool name is 1..%d characters of %s", o.Path, o.Tool, maxToolName, toolNameSpelling) + case o.Tool != "" && o.Exclude: + return fmt.Errorf("operation %s names the tool %q and excludes itself from every agent: one of the two", o.Path, o.Tool) + case o.Tool != "" && o.Streaming: + return fmt.Errorf("operation %s names the tool %q and streams: a stream is not one finite call, so it is never offered as a tool", o.Path, o.Tool) + case o.Tool != "" && !o.Policy.readOnly(): + // A name on something that can never be marked is a declaration that + // does nothing, and it reads as the opposite: the owner believes they + // exposed a tool. Only a read is marked, so only a read is named. + return fmt.Errorf("operation %s names the tool %q, and only a read is offered as one: its policy requires more than %q, so the operation is resolved by its effect receipt and stays a route its own page calls", o.Path, o.Tool, corerunnable.ReadOnlyScopeAction) } if err := o.Policy.validate(); err != nil { return fmt.Errorf("operation %s %s %w", o.verb(), o.Path, err) diff --git a/runnable_marker_test.go b/runnable_marker_test.go index fe1ca8d..53ca033 100644 --- a/runnable_marker_test.go +++ b/runnable_marker_test.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "testing" basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" @@ -293,3 +294,76 @@ func TestAToolNameIsOneSpelling(t *testing.T) { } } } + +// TestOneToolNameNamesOneOperation: a tool name is the identity a model +// addresses, so it names exactly one operation, as an operationId names +// exactly one route. Two reads claiming one name would publish two tools a +// model cannot tell apart, and nothing downstream collapses them — a derived +// package is keyed by the operationId, and Robin's catalogue dedupes on the +// operation, the contract digest and the policy, never on the word. +func TestOneToolNameNamesOneOperation(t *testing.T) { + answer := func(context.Context, *Gateway) (any, error) { return nil, nil } + reads := Requires("records.records", "read") + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "service.codefly.yaml"), []byte("kind: service\nname: backend\nversion: 0.3.0\n"), 0o644); err != nil { + t.Fatal(err) + } + server := New(Manifest{ID: "releases"}).Operations( + Operation{Path: "/v1/releases/one", Summary: "One", Callers: "Any member", Behavior: "One object", + Tool: "read_board", Response: recordResult{}, Policy: reads, Handler: answer}, + Operation{Path: "/v1/releases/two", Summary: "Two", Callers: "Any member", Behavior: "One object", + Tool: "read_board", Response: recordResult{}, Policy: reads, Handler: answer}, + ) + _, err := server.InterfaceArtifact(dir, InterfaceInfo{Title: "releases", OperationPrefix: "Releases", Tag: "releases"}) + if err == nil { + t.Fatal("two operations offered one tool name and the artifact rendered") + } + if !strings.Contains(err.Error(), "both offer the tool") || !strings.Contains(err.Error(), "read_board") { + t.Errorf("the refusal must name the tool and both routes, got %v", err) + } +} + +// TestANameOnWhatIsNeverATool: only a read is marked, so only a read is named. +// A name on anything else is a declaration that does nothing and reads as the +// opposite — the owner believes they exposed a tool. +func TestANameOnWhatIsNeverATool(t *testing.T) { + answer := func(context.Context, *Gateway) (any, error) { return nil, nil } + base := func() Operation { + return Operation{Path: "/v1/releases/thing", Summary: "Thing", Callers: "Any member", + Behavior: "One object", Tool: "read_thing", Response: recordResult{}, + Policy: Requires("records.records", "read"), Handler: answer} + } + for _, c := range []struct { + why string + of func(Operation) Operation + names string + }{ + {"a mutation", func(o Operation) Operation { + o.Method, o.Policy = "post", Requires("records.records", "read", "write") + return o + }, "only a read"}, + {"an excluded route", func(o Operation) Operation { o.Exclude = true; return o }, "excludes itself"}, + {"a stream", func(o Operation) Operation { o.Streaming = true; return o }, "streams"}, + {"any checked caller", func(o Operation) Operation { o.Policy = AnyCheckedCaller(); return o }, "only a read"}, + } { + err := c.of(base()).validate() + if err == nil { + t.Errorf("%s named a tool and was accepted", c.why) + continue + } + if !strings.Contains(err.Error(), c.names) { + t.Errorf("%s: the refusal must say why it is never a tool, got %v", c.why, err) + } + } + // The same operation without a name is valid: this refuses the name, not + // the operation. + for _, of := range []func(Operation) Operation{ + func(o Operation) Operation { o.Tool, o.Exclude = "", true; return o }, + func(o Operation) Operation { o.Tool, o.Streaming = "", true; return o }, + func(o Operation) Operation { o.Tool, o.Policy = "", AnyCheckedCaller(); return o }, + } { + if err := of(base()).validate(); err != nil { + t.Errorf("an unnamed operation must stay valid, got %v", err) + } + } +}