diff --git a/CHANGELOG.md b/CHANGELOG.md index f86a029..d928457 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,21 @@ # 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. +- **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 - **The artifact marks each read (`x-codefly-operation`).** An operation whose diff --git a/README.md b/README.md index 1854c7c..ca3fc83 100644 --- a/README.md +++ b/README.md @@ -662,7 +662,22 @@ 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. 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 +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..fb9ccf4 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 @@ -342,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 @@ -362,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, @@ -376,7 +402,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 +482,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 +513,20 @@ 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..%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 aa06e5c..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" @@ -39,6 +40,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 +227,143 @@ 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) + } + } +} + +// 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) + } + } +}