From 3b3d3ce044339e9d403bc2f895090b568d2dabee Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 08:51:23 -0400 Subject: [PATCH 01/14] feat(pipeline): Add FieldSchema reflection from struct tags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plugins decode their config from JSON via Configurable.Configure but the framework has only known JSON field names — no per-field metadata. Tools that want to render config UIs, generate templates, or publish JSON Schema have had to read source. Add a thin reflection layer: type FieldSchema struct { Name string // json key Type string // string|int|bool|[]string|object|unknown Required bool // from `required:"true"` tag Description string // from `description:"..."` tag Default string // from `default:"..."` tag (cosmetic) Enum []string // from `enum:"a,b,c"` tag Fields []FieldSchema // populated when Type == "object" } func SchemaOf(configType any) []FieldSchema SchemaOf accepts a zero-value struct (or *struct), walks the type via reflect, and emits one FieldSchema per JSON-tagged exported field. Tag vocabulary is additive — absence of any tag means "no metadata," and the field still appears in the schema. The four new tags (description, required, default, enum) are hints for tooling. They do not change runtime behavior; the plugin's existing applyDefaults / validate pipeline remains the authoritative source of truth for runtime config semantics. Type categorization is deliberately coarse: - Primitive scalars (string/int/bool/pointers thereof) - []string (the most common slice in plugin configs) - Nested struct → recurse one level into its fields - Anything else (slice-of-struct, maps, etc.) → "unknown" Slice-of-struct shapes (e.g. tokenexchange.RoutesConfig.Rules) don't fit the per-line YAML scalar template model abctl will use, so they're surfaced as "unknown" rather than recursed. If a future consumer needs deeper recursion the helper can be extended; today no plugin's flat YAML template benefits from it. Tests cover primitives, []string, nested structs, pointer indirection, untagged-field skipping, dash-tag (`json:"-"`) skipping, non-struct inputs, slice-of-struct → "unknown", and enum tag whitespace/empty trimming. This is foundational only — no consumer wires up to it yet. The catalog adapter, abctl renderer, and per-plugin annotations land in subsequent commits. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/authlib/pipeline/schema.go | 165 +++++++++++++++++++++ authbridge/authlib/pipeline/schema_test.go | 142 ++++++++++++++++++ 2 files changed, 307 insertions(+) create mode 100644 authbridge/authlib/pipeline/schema.go create mode 100644 authbridge/authlib/pipeline/schema_test.go diff --git a/authbridge/authlib/pipeline/schema.go b/authbridge/authlib/pipeline/schema.go new file mode 100644 index 000000000..218d605a6 --- /dev/null +++ b/authbridge/authlib/pipeline/schema.go @@ -0,0 +1,165 @@ +// Schema introspection for plugin config structs. +// +// Plugins decode their config from JSON via Configurable.Configure +// (see configurable.go). The framework has long known JSON field +// names (`json:"foo_bar"` tags) but nothing else about each field. +// Tools that want to render config UIs, generate templates, or +// publish JSON Schema have had to read source. +// +// SchemaOf walks a config struct via reflection and emits one +// FieldSchema per JSON-tagged field. Plugin authors annotate fields +// with three additional tags: +// +// required:"true" // boot fails if empty/zero +// description:"prose, one line" // shown in templates / hover +// default:"5000" // documented default (cosmetic) +// enum:"a,b,c" // allowed values +// +// All four tags are optional. Absence of any tag means "no +// metadata" — the field appears in the schema but with empty fields. +// +// This package owns the shape; presentation (template YAML, hover +// formatting, JSON Schema) lives in the consumer (abctl, kagenti UI, +// future generators). + +package pipeline + +import ( + "reflect" + "strings" +) + +// FieldSchema describes one config field's metadata for tooling. +type FieldSchema struct { + // Name is the JSON key (snake_case) — what operators type in YAML. + Name string `json:"name"` + + // Type is a coarse-grained category sufficient to render templates + // and pick value placeholders. One of: + // "string", "int", "bool", "[]string", "object", "unknown". + // "object" indicates a nested struct whose fields populate Fields. + // "unknown" covers shapes the helper hasn't been taught (maps, + // slice-of-struct, etc.); the field still renders but without a + // type-specific placeholder. + Type string `json:"type"` + + // Required reports the `required:"true"` tag. Boot semantics are + // the plugin's own concern — this field is just metadata. + Required bool `json:"required,omitempty"` + + // Description is the `description:"..."` tag verbatim. Single-line. + Description string `json:"description,omitempty"` + + // Default is the `default:"..."` tag verbatim. Cosmetic — the + // authoritative default is whatever applyDefaults sets at runtime. + Default string `json:"default,omitempty"` + + // Enum is the `enum:"a,b,c"` tag split on commas. Empty when the + // field is not enum-shaped. + Enum []string `json:"enum,omitempty"` + + // Fields is populated when Type is "object" (nested struct). The + // outer field's Description applies to the struct as a whole; the + // nested fields each carry their own metadata. + Fields []FieldSchema `json:"fields,omitempty"` +} + +// SchemaOf walks the given struct value and returns its field schemas. +// Pass a zero value (e.g. `SchemaOf(ibacConfig{})`) — the value is +// inspected for type only, not for runtime field values. +// +// Returns nil if the argument isn't a struct (or pointer to struct). +// Anonymous fields are flattened. Fields without a `json:` tag are +// skipped — explicit JSON tagging is the existing wire convention, +// and untagged fields don't appear in the operator-facing YAML. +func SchemaOf(configType any) []FieldSchema { + t := reflect.TypeOf(configType) + if t == nil { + return nil + } + if t.Kind() == reflect.Ptr { + t = t.Elem() + } + if t.Kind() != reflect.Struct { + return nil + } + return schemaOfType(t) +} + +func schemaOfType(t reflect.Type) []FieldSchema { + var out []FieldSchema + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if !f.IsExported() { + continue + } + jsonTag := f.Tag.Get("json") + if jsonTag == "" || jsonTag == "-" { + continue + } + name := strings.SplitN(jsonTag, ",", 2)[0] + if name == "" || name == "-" { + continue + } + schema := FieldSchema{ + Name: name, + Type: kindOf(f.Type), + Required: f.Tag.Get("required") == "true", + Description: f.Tag.Get("description"), + Default: f.Tag.Get("default"), + } + if enumTag := f.Tag.Get("enum"); enumTag != "" { + parts := strings.Split(enumTag, ",") + schema.Enum = make([]string, 0, len(parts)) + for _, p := range parts { + if v := strings.TrimSpace(p); v != "" { + schema.Enum = append(schema.Enum, v) + } + } + } + if schema.Type == "object" { + // Recurse one level. Operators rarely have deeply-nested + // configs; if the need arises later we can lift this guard. + schema.Fields = schemaOfType(unwrap(f.Type)) + } + out = append(out, schema) + } + return out +} + +// kindOf returns the coarse-grained Type tag for a field. +func kindOf(t reflect.Type) string { + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + switch t.Kind() { + case reflect.String: + return "string" + case reflect.Bool: + return "bool" + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return "int" + case reflect.Slice: + // Only []string gets a typed tag; other slices are "unknown" + // (slice-of-struct, slice-of-map, etc. are rare in plugin + // configs and don't render to a single YAML scalar template). + if t.Elem().Kind() == reflect.String { + return "[]string" + } + return "unknown" + case reflect.Struct: + return "object" + default: + return "unknown" + } +} + +// unwrap returns the underlying struct type for a struct or *struct. +// Returns t unchanged if it's not a struct shape. +func unwrap(t reflect.Type) reflect.Type { + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + return t +} diff --git a/authbridge/authlib/pipeline/schema_test.go b/authbridge/authlib/pipeline/schema_test.go new file mode 100644 index 000000000..f7e0b9d27 --- /dev/null +++ b/authbridge/authlib/pipeline/schema_test.go @@ -0,0 +1,142 @@ +package pipeline + +import ( + "reflect" + "testing" +) + +// Tests below use representative shapes covering every Type the +// helper recognizes plus the no-tag and pointer-to-struct cases. +// They lock in the contract that consumers (abctl template +// renderer, JSON-Schema generator) build on. + +type primitives struct { + Hostname string `json:"hostname" required:"true" description:"Target hostname."` + Port int `json:"port" description:"TCP port." default:"8080"` + Verbose bool `json:"verbose"` + Mode string `json:"mode" enum:"strict, lenient, off"` +} + +func TestSchemaOf_Primitives(t *testing.T) { + got := SchemaOf(primitives{}) + want := []FieldSchema{ + {Name: "hostname", Type: "string", Required: true, Description: "Target hostname."}, + {Name: "port", Type: "int", Description: "TCP port.", Default: "8080"}, + {Name: "verbose", Type: "bool"}, + {Name: "mode", Type: "string", Enum: []string{"strict", "lenient", "off"}}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("SchemaOf primitives:\n got: %+v\nwant: %+v", got, want) + } +} + +type withList struct { + Allowed []string `json:"allowed" description:"Allowlisted hosts."` +} + +func TestSchemaOf_StringSlice(t *testing.T) { + got := SchemaOf(withList{}) + if len(got) != 1 || got[0].Type != "[]string" { + t.Errorf("expected one []string field, got %+v", got) + } +} + +type nestedInner struct { + Path string `json:"path" required:"true" description:"Routes file path."` + Inner bool `json:"inline"` +} + +type nestedOuter struct { + Name string `json:"name" required:"true"` + Inner nestedInner `json:"inner" description:"Nested config block."` +} + +func TestSchemaOf_NestedStruct(t *testing.T) { + got := SchemaOf(nestedOuter{}) + if len(got) != 2 { + t.Fatalf("expected 2 top-level fields, got %d (%+v)", len(got), got) + } + if got[1].Name != "inner" || got[1].Type != "object" { + t.Errorf("nested field shape wrong: %+v", got[1]) + } + if len(got[1].Fields) != 2 { + t.Fatalf("nested fields not populated: %+v", got[1].Fields) + } + if got[1].Fields[0].Name != "path" || !got[1].Fields[0].Required { + t.Errorf("nested[0] shape wrong: %+v", got[1].Fields[0]) + } +} + +type withSkippable struct { + JSONOmitted string `json:"-"` + NoTag string + Counted int `json:"counted"` +} + +func TestSchemaOf_SkipsUntaggedAndDashed(t *testing.T) { + // Skipping unexported fields is also part of the contract; + // we don't unit-test that case here because adding an + // unexported field with a json tag triggers `go vet`'s + // structtag check and pollutes the test file. The + // IsExported() guard in schemaOfType is straightforward. + got := SchemaOf(withSkippable{}) + if len(got) != 1 || got[0].Name != "counted" { + t.Errorf("expected only the `counted` field, got %+v", got) + } +} + +type withPointer struct { + Optional *int `json:"optional" description:"Optional override."` +} + +func TestSchemaOf_PointerField(t *testing.T) { + got := SchemaOf(withPointer{}) + if len(got) != 1 || got[0].Type != "int" { + t.Errorf("expected pointer-to-int to render as int, got %+v", got) + } +} + +func TestSchemaOf_PointerToStruct(t *testing.T) { + got := SchemaOf(&primitives{}) + if len(got) != 4 { + t.Errorf("expected SchemaOf to handle *struct, got %d fields", len(got)) + } +} + +func TestSchemaOf_NotAStruct(t *testing.T) { + if got := SchemaOf("not-a-struct"); got != nil { + t.Errorf("expected nil for non-struct, got %+v", got) + } + if got := SchemaOf(42); got != nil { + t.Errorf("expected nil for non-struct, got %+v", got) + } + if got := SchemaOf(nil); got != nil { + t.Errorf("expected nil for nil, got %+v", got) + } +} + +type withSliceOfStruct struct { + Routes []nestedInner `json:"routes" description:"List of routes."` +} + +func TestSchemaOf_SliceOfStructIsUnknown(t *testing.T) { + // We deliberately don't recurse into slice-of-struct — those + // shapes don't fit the per-line YAML-scalar template model + // abctl uses. Documented in kindOf comment. + got := SchemaOf(withSliceOfStruct{}) + if len(got) != 1 || got[0].Type != "unknown" { + t.Errorf("slice-of-struct should be \"unknown\", got %+v", got) + } +} + +// Empty-enum tag should produce nil, not [""]: the parser trims and +// drops empty parts. +func TestSchemaOf_EnumWhitespaceAndEmpty(t *testing.T) { + type withMessyEnum struct { + Mode string `json:"mode" enum:" on , , off "` + } + got := SchemaOf(withMessyEnum{}) + if len(got) != 1 || !reflect.DeepEqual(got[0].Enum, []string{"on", "off"}) { + t.Errorf("expected trimmed [on, off], got %+v", got) + } +} From 1b1580e6697be366a6759444089c132064aeb46f Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 09:00:58 -0400 Subject: [PATCH 02/14] feat(plugins): Annotate config structs + ConfigSchema methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires four plugin configs (ibac, jwt-validation, token-exchange, token-broker) up to pipeline.SchemaOf. Each plugin's config struct gains description / required / default / enum tags on its JSON- tagged fields, and each plugin gains a one-line ConfigSchema() method that returns SchemaOf of its config type. Adds the SchemaProvider interface in pipeline/schema.go (one-method contract) so the upcoming catalog adapter can type-assert against it without needing to import every plugin package. Tag conventions applied uniformly: - description: single-line summary suitable for inline comments in YAML templates. Long-form rationale stays in the field's Go doc comment and in docs/-plugin.md. - required: "true" only on fields whose absence boot-fails. Most fields are optional with sensible defaults. - default: cosmetic mirror of what applyDefaults produces. Lets operator-facing tooling render the default visibly without invoking applyDefaults. - enum: comma-separated allowed values for enum-shaped fields. Whitespace around commas is trimmed by the schema parser. Plugins without configs (a2a-parser, inference-parser, mcp-parser on this branch) deliberately do NOT implement SchemaProvider — the catalog adapter will surface them as "no fields" without any boilerplate on those plugins. The struct tag values must be on a single line per field; Go's reflect.StructTag.Get parser requires "key:\"value\"" pairs to be space-separated, not newline-separated. Tag lines in this commit are intentionally long for that reason. No behavior change at runtime — these tags are pure metadata for tooling. Existing config decode (DisallowUnknownFields) keeps its authoritative role for accepted/rejected fields; plugins' applyDefaults / validate paths are untouched. Tests: covered by pipeline/schema_test.go (commit 1). A smoke test that ibac.ConfigSchema() returns the expected field list confirms end-to-end wiring; deeper per-plugin assertions land in sessionapi tests in the next commit. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/authlib/pipeline/schema.go | 18 +++++ authbridge/authlib/plugins/ibac/plugin.go | 74 ++++++++----------- .../authlib/plugins/jwtvalidation/plugin.go | 29 +++++--- .../authlib/plugins/tokenbroker/plugin.go | 28 ++++--- .../authlib/plugins/tokenexchange/plugin.go | 42 +++++++---- 5 files changed, 115 insertions(+), 76 deletions(-) diff --git a/authbridge/authlib/pipeline/schema.go b/authbridge/authlib/pipeline/schema.go index 218d605a6..397602174 100644 --- a/authbridge/authlib/pipeline/schema.go +++ b/authbridge/authlib/pipeline/schema.go @@ -29,6 +29,24 @@ import ( "strings" ) +// SchemaProvider is implemented by plugins whose config field +// metadata should appear in the catalog and downstream tooling +// (abctl edit templates, future kagenti-UI forms, JSON Schema +// generators). Plugins without configs (a2a-parser, +// inference-parser today) can omit this interface. +// +// The convention is a one-line method that delegates to SchemaOf: +// +// func (p *MyPlugin) ConfigSchema() []FieldSchema { +// return SchemaOf(myPluginConfig{}) +// } +// +// The framework's catalog adapter type-asserts against this +// interface; absence is silently treated as "no field metadata." +type SchemaProvider interface { + ConfigSchema() []FieldSchema +} + // FieldSchema describes one config field's metadata for tooling. type FieldSchema struct { // Name is the JSON key (snake_case) — what operators type in YAML. diff --git a/authbridge/authlib/plugins/ibac/plugin.go b/authbridge/authlib/plugins/ibac/plugin.go index b93109427..2a011620f 100644 --- a/authbridge/authlib/plugins/ibac/plugin.go +++ b/authbridge/authlib/plugins/ibac/plugin.go @@ -44,47 +44,29 @@ import ( // ibacConfig is the plugin's local config schema. See // authbridge/docs/plugin-reference.md for the decode → applyDefaults → // validate convention shared with jwt-validation and token-exchange. +// +// Field tags drive both runtime decoding (json) and operator-facing +// schema introspection (description / required / default / enum). +// See pipeline/schema.go for the consumer contract; descriptions are +// kept single-line — full prose lives in docs/ibac-plugin.md. type ibacConfig struct { - // JudgeEndpoint is the base URL of the LLM-judge service. The - // plugin POSTs OpenAI-compatible chat-completion requests to - // JudgeEndpoint+"/v1/chat/completions". - JudgeEndpoint string `json:"judge_endpoint"` - - // JudgeModel names the model to use for verdicts (e.g. - // "llama3.2:3b" for ollama, "gpt-4o-mini" for OpenAI). - JudgeModel string `json:"judge_model"` - - // JudgeBearer is an optional bearer token for the judge endpoint. - // Leave empty for unauthenticated local LLMs (ollama). - JudgeBearer string `json:"judge_bearer"` - - // SystemPrompt overrides the default judge system prompt. Empty - // means "use the default" (see judge.go:defaultSystemPrompt). - SystemPrompt string `json:"system_prompt"` - - // TimeoutMs bounds each judge call. Defaults to 5000. - TimeoutMs int `json:"timeout_ms"` - - // JudgeInference, when true, also judges outbound traffic where - // pctx.Extensions.Inference is populated (the agent's own LLM - // reasoning loop). Default false — judging the agent's prompts - // is high-cost low-value for typical deployments. - JudgeInference bool `json:"judge_inference"` - - // AgentLLMHost is a convenience: the host of the agent's own LLM - // endpoint. When set, it's added to the bypass-host list so the - // agent's reasoning traffic is never judged regardless of - // JudgeInference. - AgentLLMHost string `json:"agent_llm_host"` - - // BypassHosts are host globs (path.Match syntax) skipped without - // judging. Defaults include common infrastructure hostnames so - // the judge isn't called on Keycloak / OTel / agent-card hops. - BypassHosts []string `json:"bypass_hosts"` - - // BypassPaths are URL path globs skipped without judging. - // Defaults to bypass.DefaultPatterns (.well-known, healthz, etc). - BypassPaths []string `json:"bypass_paths"` + JudgeEndpoint string `json:"judge_endpoint" required:"true" description:"Base URL of the LLM-judge service. The plugin POSTs to {endpoint}/v1/chat/completions."` + + JudgeModel string `json:"judge_model" required:"true" description:"Model name passed to the judge, e.g. \"llama3.2:3b\" or \"gpt-4o-mini\"."` + + JudgeBearer string `json:"judge_bearer" description:"Optional bearer token for the judge endpoint. Empty for unauthenticated local LLMs."` + + SystemPrompt string `json:"system_prompt" description:"Override the default judge system prompt. Empty means use the built-in default."` + + TimeoutMs int `json:"timeout_ms" description:"Per-call judge timeout. Validation rejects values below 100." default:"5000"` + + JudgeInference bool `json:"judge_inference" description:"When true, also judge outbound LLM-reasoning traffic. High-cost / low-value default off." default:"false"` + + AgentLLMHost string `json:"agent_llm_host" description:"Convenience: agent's own LLM host. Added to bypass_hosts so reasoning traffic is never judged."` + + BypassHosts []string `json:"bypass_hosts" description:"Host globs (path.Match) skipped without judging. Defaults include keycloak / spire / otel."` + + BypassPaths []string `json:"bypass_paths" description:"URL path globs skipped without judging. Defaults: /.well-known/* /healthz /readyz /livez."` // NoIntentPolicy controls behavior when a request reaches step 6 // without a recorded user intent — either because Session is nil @@ -110,7 +92,7 @@ type ibacConfig struct { // Deployments that mix user-driven and self-driven traffic get // the right behavior automatically; deployments that want hard // fail-closed semantics opt in via "deny". - NoIntentPolicy string `json:"no_intent_policy"` + NoIntentPolicy string `json:"no_intent_policy" description:"Behavior when an action lacks recorded user intent. allow=skip; deny=403." default:"allow" enum:"allow,deny"` // UnclassifiedPolicy controls behavior at step 4 (the // classification gate) when no protocol parser populated any @@ -142,7 +124,15 @@ type ibacConfig struct { // arbitrary HTTP traffic isn't paid for by most operators. // The IBAC demo opts into "judge" to keep its plain-HTTP exfil // scenario operational. - UnclassifiedPolicy string `json:"unclassified_policy"` + UnclassifiedPolicy string `json:"unclassified_policy" description:"Behavior when no parser claimed the request. passthrough=skip; judge=fall through to judge." default:"passthrough" enum:"passthrough,judge"` +} + +// ConfigSchema exposes the ibacConfig fields for schema-aware tooling +// (abctl edit templates, future kagenti-UI forms, etc.). Implements +// pipeline.SchemaProvider; absence would simply make IBAC opaque to +// such tooling without affecting runtime. +func (p *IBAC) ConfigSchema() []pipeline.FieldSchema { + return pipeline.SchemaOf(ibacConfig{}) } // no_intent_policy values. diff --git a/authbridge/authlib/plugins/jwtvalidation/plugin.go b/authbridge/authlib/plugins/jwtvalidation/plugin.go index f3f263de7..72c147879 100644 --- a/authbridge/authlib/plugins/jwtvalidation/plugin.go +++ b/authbridge/authlib/plugins/jwtvalidation/plugin.go @@ -22,12 +22,17 @@ import ( // jwtValidationConfig is the plugin's local config schema. See // authbridge/docs/plugin-reference.md for the decode → applyDefaults → // validate pattern. +// Field tags drive both runtime decoding (json) and operator-facing +// schema introspection (description / required / default / enum). +// Inline doc comments retain the long-form rationale; struct tags +// carry single-line summaries for templating. See pipeline/schema.go +// for the consumer contract. type jwtValidationConfig struct { // Issuer is the JWT `iss` claim expected on inbound tokens. // In split-horizon deployments this is the PUBLIC Keycloak URL // (whatever Keycloak stamps into the `iss` claim) — it only needs // to match bit-for-bit, not be reachable from inside the pod. - Issuer string `json:"issuer"` + Issuer string `json:"issuer" required:"true" description:"Expected JWT iss claim. Public Keycloak URL in split-horizon deployments."` // JWKSURL points at the JWKS endpoint used to verify signatures. // The sidecar actually GETs this URL from inside the cluster, so @@ -40,7 +45,7 @@ type jwtValidationConfig struct { // internal URL, when the operator supplies it) // 2. Issuer (fallback for single-horizon deployments where the // issuer hostname is reachable from inside the cluster) - JWKSURL string `json:"jwks_url"` + JWKSURL string `json:"jwks_url" description:"JWKS endpoint URL (internal). Derived from KeycloakURL+Realm or Issuer when empty."` // KeycloakURL and KeycloakRealm are a convenience for deriving // JWKSURL from the internal Keycloak service URL, symmetric with @@ -52,13 +57,13 @@ type jwtValidationConfig struct { // via a cross-plugin pass; per-plugin configs don't share state, // so each plugin now carries its own copy of the "where is // Keycloak internally" hint. - KeycloakURL string `json:"keycloak_url"` - KeycloakRealm string `json:"keycloak_realm"` + KeycloakURL string `json:"keycloak_url" description:"Internal Keycloak base URL. Used to derive jwks_url when omitted."` + KeycloakRealm string `json:"keycloak_realm" description:"Keycloak realm name. Pairs with keycloak_url for jwks_url derivation."` // Audience is the literal audience value expected on inbound // tokens. One of {Audience, AudienceFile, AudienceMode:"per-host"} // is required. - Audience string `json:"audience"` + Audience string `json:"audience" description:"Expected aud claim value. One of audience / audience_file / audience_mode=per-host is required."` // AudienceFile reads the expected audience from a file. Used // together with client-registration's /shared/client-id.txt. The @@ -69,12 +74,12 @@ type jwtValidationConfig struct { // will fill in /shared/client-id.txt. To opt out of any file poll, // supply an explicit Audience instead; the file default only kicks // in when both Audience and AudienceFile are empty. - AudienceFile string `json:"audience_file"` + AudienceFile string `json:"audience_file" description:"Read expected audience from this file. Default: /shared/client-id.txt when both audience fields are empty."` // AudienceMode chooses how the expected audience is resolved: // "static" (default) uses Audience/AudienceFile; "per-host" derives // it from pctx.Host via routing.ServiceNameFromHost (waypoint mode). - AudienceMode string `json:"audience_mode"` + AudienceMode string `json:"audience_mode" description:"How to resolve expected audience: static or per-host." default:"static" enum:"static,per-host"` // AllowedAudiences lists extra audience strings the sidecar accepts // for inbound JWT validation (OR semantics: the token's aud claim @@ -89,11 +94,11 @@ type jwtValidationConfig struct { // multiple audiences (RFC 7519 string or array) and the agent must // accept more than the workload client ID alone — prefer aligning // IdP audience policy long-term. See plugin-reference.md. - AllowedAudiences []string `json:"allowed_audiences"` + AllowedAudiences []string `json:"allowed_audiences" description:"Extra audience strings accepted (OR semantics)."` // BypassPaths are URL path globs (see authlib/bypass) that skip // validation entirely. - BypassPaths []string `json:"bypass_paths"` + BypassPaths []string `json:"bypass_paths" description:"Path globs (path.Match) skipped without JWT validation. Defaults: /healthz /readyz /livez /.well-known/*."` } func (c *jwtValidationConfig) applyDefaults() { @@ -215,6 +220,12 @@ func (p *JWTValidation) Capabilities() pipeline.PluginCapabilities { } } +// ConfigSchema implements pipeline.SchemaProvider; surfaces field +// metadata to abctl edit templates and other config-aware tooling. +func (p *JWTValidation) ConfigSchema() []pipeline.FieldSchema { + return pipeline.SchemaOf(jwtValidationConfig{}) +} + // Configure decodes the plugin's config subtree, applies defaults, // validates, and constructs the internal auth handler. If AudienceFile // is set but the file isn't yet readable (client-registration still diff --git a/authbridge/authlib/plugins/tokenbroker/plugin.go b/authbridge/authlib/plugins/tokenbroker/plugin.go index 345092b88..9a613151d 100644 --- a/authbridge/authlib/plugins/tokenbroker/plugin.go +++ b/authbridge/authlib/plugins/tokenbroker/plugin.go @@ -20,33 +20,37 @@ import ( ) // tokenBrokerConfig is the plugin's local config schema. +// +// Field tags drive both runtime decoding (json) and operator-facing +// schema introspection (description / required / default / enum). +// See pipeline/schema.go for the consumer contract. type tokenBrokerConfig struct { // BrokerURL is the base URL of the token broker service. - BrokerURL string `json:"broker_url"` + BrokerURL string `json:"broker_url" required:"true" description:"Base URL of the token broker service."` // DefaultPolicy is applied when a request's host matches no route: // "passthrough" (default) forwards the request unchanged; // "broker" attempts to acquire a token from the broker. - DefaultPolicy string `json:"default_policy"` + DefaultPolicy string `json:"default_policy" description:"Behavior when host matches no route." default:"passthrough" enum:"passthrough,broker"` // Routes drives host-to-broker matching. A host that matches no // route falls through to DefaultPolicy. - Routes tokenBrokerRoutes `json:"routes"` + Routes tokenBrokerRoutes `json:"routes" description:"Host-to-broker routing rules; non-matching hosts fall through to default_policy."` } type tokenBrokerRoutes struct { // File is an optional path to a routes.yaml file. - File string `json:"file"` + File string `json:"file" description:"Path to a routes.yaml file. Inline rules below are merged with file-loaded rules."` // Rules are inline route entries; combined with routes loaded from File. - Rules []tokenBrokerRoute `json:"rules"` + Rules []tokenBrokerRoute `json:"rules" description:"Inline route entries. Combined with rules loaded from file."` } type tokenBrokerRoute struct { - Host string `json:"host"` - Action string `json:"action"` // "broker" or "passthrough"; defaults to "broker" - AuthorizationEndpoint string `json:"authorization_endpoint,omitempty"` - TokenEndpoint string `json:"token_endpoint,omitempty"` + Host string `json:"host" description:"Host glob pattern."` + Action string `json:"action" description:"broker or passthrough." default:"broker" enum:"broker,passthrough"` + AuthorizationEndpoint string `json:"authorization_endpoint,omitempty" description:"Per-route OAuth authorization endpoint override."` + TokenEndpoint string `json:"token_endpoint,omitempty" description:"Per-route OAuth token endpoint override."` } func (c *tokenBrokerConfig) applyDefaults() { @@ -159,6 +163,12 @@ func (p *TokenBroker) Capabilities() pipeline.PluginCapabilities { } } +// ConfigSchema implements pipeline.SchemaProvider; surfaces field +// metadata to abctl edit templates and other config-aware tooling. +func (p *TokenBroker) ConfigSchema() []pipeline.FieldSchema { + return pipeline.SchemaOf(tokenBrokerConfig{}) +} + func (p *TokenBroker) Configure(raw json.RawMessage) error { var c tokenBrokerConfig diff --git a/authbridge/authlib/plugins/tokenexchange/plugin.go b/authbridge/authlib/plugins/tokenexchange/plugin.go index 6183554df..547302bf2 100644 --- a/authbridge/authlib/plugins/tokenexchange/plugin.go +++ b/authbridge/authlib/plugins/tokenexchange/plugin.go @@ -23,24 +23,28 @@ import ( // tokenExchangeConfig is the plugin's local config schema. See // authbridge/docs/plugin-reference.md for the pattern. +// Field tags drive both runtime decoding (json) and operator-facing +// schema introspection (description / required / default / enum). +// Inline doc comments retain long-form rationale; struct tags carry +// single-line summaries for templating. See pipeline/schema.go. type tokenExchangeConfig struct { // TokenURL is the OAuth token endpoint. Explicit value wins; else // derived from KeycloakURL + KeycloakRealm using Keycloak's // convention. - TokenURL string `json:"token_url"` + TokenURL string `json:"token_url" description:"OAuth token endpoint URL. Derived from keycloak_url+realm when empty."` // KeycloakURL and KeycloakRealm are a convenience for deriving // TokenURL when the operator prefers to supply Keycloak base + realm // rather than the full token endpoint. - KeycloakURL string `json:"keycloak_url"` - KeycloakRealm string `json:"keycloak_realm"` + KeycloakURL string `json:"keycloak_url" description:"Internal Keycloak base URL. Used to derive token_url when omitted."` + KeycloakRealm string `json:"keycloak_realm" description:"Keycloak realm name. Pairs with keycloak_url for token_url derivation."` // DefaultPolicy is applied when a request's host matches no route: // "passthrough" (default) forwards the request unchanged; // "exchange" attempts a client-credentials exchange with an empty // audience (usually fails — kept for rare use cases where the IdP // allows it). - DefaultPolicy string `json:"default_policy"` + DefaultPolicy string `json:"default_policy" description:"Behavior when host matches no route: passthrough forwards unchanged, exchange attempts empty-audience client-credentials." default:"passthrough" enum:"passthrough,exchange"` // NoTokenPolicy controls how the plugin handles outbound requests // that arrive without a bearer token: "client-credentials" does an @@ -48,41 +52,41 @@ type tokenExchangeConfig struct { // unchanged; "deny" rejects. Default: "deny" in all modes. // Operators who need a different behavior (e.g. waypoint's historic // "allow" default) must set it explicitly per plugin entry. - NoTokenPolicy string `json:"no_token_policy"` + NoTokenPolicy string `json:"no_token_policy" description:"Behavior when outbound has no bearer token: client-credentials, allow, or deny." default:"deny" enum:"client-credentials,allow,deny"` // Identity carries client credentials used for token exchange. - Identity tokenExchangeIdentity `json:"identity"` + Identity tokenExchangeIdentity `json:"identity" description:"Client credentials used for token exchange (spiffe or client-secret)."` // Routes drives host-to-audience matching. A host that matches no // route falls through to DefaultPolicy. - Routes tokenExchangeRoutes `json:"routes"` + Routes tokenExchangeRoutes `json:"routes" description:"Host-to-audience routing rules; non-matching hosts fall through to default_policy."` // AudienceFromHost — when true, requests with no matching route use // routing.ServiceNameFromHost(host) as the target audience. Used in // waypoint mode. - AudienceFromHost bool `json:"audience_from_host"` + AudienceFromHost bool `json:"audience_from_host" description:"When true, derive audience from host for unrouted requests (waypoint mode)." default:"false"` } type tokenExchangeIdentity struct { // Type is one of "spiffe" or "client-secret". - Type string `json:"type"` + Type string `json:"type" description:"Identity scheme: spiffe (JWT-SVID assertion) or client-secret." enum:"spiffe,client-secret"` // ClientID identifies the client in Keycloak. Explicit value wins; // else read from ClientIDFile at Configure time (or by Init if the // file isn't yet available). - ClientID string `json:"client_id"` - ClientIDFile string `json:"client_id_file"` + ClientID string `json:"client_id" description:"Inline Keycloak client ID. One of client_id or client_id_file is required."` + ClientIDFile string `json:"client_id_file" description:"Read client ID from this file. Default: /shared/client-id.txt."` // ClientSecret / ClientSecretFile are the client-secret credentials // (type=client-secret). - ClientSecret string `json:"client_secret"` - ClientSecretFile string `json:"client_secret_file"` + ClientSecret string `json:"client_secret" description:"Inline Keycloak client secret (type=client-secret)."` + ClientSecretFile string `json:"client_secret_file" description:"Read client secret from file. Default: /shared/client-secret.txt."` // JWTAudience is the audience claim minted on the JWT-SVID used as // the RFC 8693 client assertion. Required when Type=="spiffe"; // ignored otherwise. Lives on the plugin (not the framework spiffe // block) because only the spiffe identity path consumes it. - JWTAudience string `json:"jwt_audience"` + JWTAudience string `json:"jwt_audience" description:"Audience claim minted on the JWT-SVID assertion. Required when type=spiffe."` // jwt_svid_path was historically a per-plugin path to the JWT-SVID // file written by spiffe-helper. Removed in favor of injection via @@ -94,11 +98,11 @@ type tokenExchangeIdentity struct { type tokenExchangeRoutes struct { // File is an optional path to a routes.yaml file (see // authlib/routing.LoadRoutes). - File string `json:"file"` + File string `json:"file" description:"Path to a routes.yaml file. Default: /etc/authproxy/routes.yaml."` // Rules are inline route entries; combined with routes loaded from // File. - Rules []tokenExchangeRoute `json:"rules"` + Rules []tokenExchangeRoute `json:"rules" description:"Inline route entries. Combined with rules loaded from file."` } type tokenExchangeRoute struct { @@ -278,6 +282,12 @@ func (p *TokenExchange) Capabilities() pipeline.PluginCapabilities { } } +// ConfigSchema implements pipeline.SchemaProvider; surfaces field +// metadata to abctl edit templates and other config-aware tooling. +func (p *TokenExchange) ConfigSchema() []pipeline.FieldSchema { + return pipeline.SchemaOf(tokenExchangeConfig{}) +} + func (p *TokenExchange) Configure(raw json.RawMessage) error { var c tokenExchangeConfig if len(raw) > 0 { From eb137c4fc989f7835b5b3b54f5ee9a24e2783148 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 09:29:05 -0400 Subject: [PATCH 03/14] feat(sessionapi): Surface per-plugin field schemas in /v1/plugins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires the schema layer end-to-end: framework Catalog() captures each plugin's ConfigSchema (when implemented), the sessionapi adapter converts to a wire-shape FieldSchemaEntry slice, and the abctl apiclient mirrors the same shape. plugins.CatalogEntry gains a Fields slice populated by type- asserting against pipeline.SchemaProvider during Catalog()'s single-shot factory walk. Constraint already documented for Capabilities() — the throwaway instance must be state-independent and side-effect free — extends naturally to ConfigSchema(): in practice it's `return SchemaOf(myConfig{})`, satisfying both. cloneCatalog deep-copies the new Fields slice (including the Enum sub-slices and recursively into nested-struct Fields) so callers mutating the returned snapshot can't taint the cached copy that seeds future /v1/plugins responses. sessionapi gains FieldSchemaEntry as the wire-level mirror of pipeline.FieldSchema, kept package-local so /v1/plugins consumers (abctl, future kagenti-UI clients) don't transitively import authlib/pipeline. The adapter's convertFieldSchemas helper recurses for nested-struct schemas. apiclient.PluginCatalogEntry gains Fields []PluginFieldEntry to match. Existing consumers that only read the original fields continue to work — JSON decoding ignores unknown wire fields, and PluginFieldEntry uses omitempty throughout, so older catalog responses without Fields decode cleanly. Tests: - TestHandlePluginCatalog_IncludesFieldSchemas confirms the new Fields field round-trips on the wire including required, default, and enum sub-fields. - The existing TestHandlePluginCatalog_ListsRegisteredPlugins stays green (it doesn't reference Fields, and the omitempty wire format doesn't change its output). No abctl-visible behavior change yet — the renderer arrives in the next commit. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/authlib/plugins/registry.go | 49 ++++++++++++++-- .../authlib/sessionapi/catalog_adapter.go | 34 ++++++++++- authbridge/authlib/sessionapi/server.go | 35 ++++++++---- authbridge/authlib/sessionapi/server_test.go | 57 +++++++++++++++++++ authbridge/cmd/abctl/apiclient/client.go | 34 +++++++---- 5 files changed, 182 insertions(+), 27 deletions(-) diff --git a/authbridge/authlib/plugins/registry.go b/authbridge/authlib/plugins/registry.go index 61532fdd0..d9421aad6 100644 --- a/authbridge/authlib/plugins/registry.go +++ b/authbridge/authlib/plugins/registry.go @@ -81,12 +81,20 @@ func RegisteredPlugins() []string { } // CatalogEntry pairs a registered plugin's name with the capabilities -// it advertises. Surfaces in `abctl`'s catalog pane and in the -// /v1/plugins endpoint so operators can see what plugins exist and -// what each one needs without reading source. +// it advertises and the field-level schema of its config (if it +// implements pipeline.SchemaProvider). Surfaces in `abctl`'s catalog +// pane, in the /v1/plugins endpoint, and in `abctl edit`'s template +// renderer so operators can see what plugins exist, what each one +// needs, and what each field means without reading source. +// +// Fields is nil for plugins without a config (or that don't +// implement SchemaProvider) — the wire format omits the field via +// `omitempty`, so existing consumers that don't know about field +// schemas keep working. type CatalogEntry struct { Name string Capabilities pipeline.PluginCapabilities + Fields []pipeline.FieldSchema } // catalogCache memoizes the Catalog() result on first call. Constructors @@ -139,10 +147,21 @@ func Catalog() []CatalogEntry { defer registryMu.RUnlock() out := make([]CatalogEntry, 0, len(registry)) for name, factory := range registry { - out = append(out, CatalogEntry{ + instance := factory() + entry := CatalogEntry{ Name: name, - Capabilities: factory().Capabilities().Normalize(), - }) + Capabilities: instance.Capabilities().Normalize(), + } + // Plugins that implement SchemaProvider expose per-field + // metadata for templating. Constraints carry over from the + // constructor contract above: the throwaway instance is never + // Shutdown'd, so SchemaProvider implementations must be + // instance-state-independent and side-effect free (typically + // just `return pipeline.SchemaOf(myConfig{})`). + if sp, ok := instance.(pipeline.SchemaProvider); ok { + entry.Fields = sp.ConfigSchema() + } + out = append(out, entry) } sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) catalogCacheVal = out @@ -175,11 +194,29 @@ func cloneCatalog(in []CatalogEntry) []CatalogEntry { After: append([]string(nil), caps.After...), Claims: append([]string(nil), caps.Claims...), }, + Fields: cloneFieldSchemas(in[i].Fields), } } return out } +// cloneFieldSchemas deep-copies the per-field schema slice. Fields +// itself is a slice, but each FieldSchema also contains slices +// (Enum, Fields for nested structs) that need their own backing +// arrays to avoid aliasing the cached snapshot. +func cloneFieldSchemas(in []pipeline.FieldSchema) []pipeline.FieldSchema { + if len(in) == 0 { + return nil + } + out := make([]pipeline.FieldSchema, len(in)) + for i, f := range in { + out[i] = f + out[i].Enum = append([]string(nil), f.Enum...) + out[i].Fields = cloneFieldSchemas(f.Fields) // recurse for nested struct fields + } + return out +} + // resetCatalogCache clears the memoized Catalog result. Intended for // tests that register/unregister plugins and need a fresh view. func resetCatalogCache() { diff --git a/authbridge/authlib/sessionapi/catalog_adapter.go b/authbridge/authlib/sessionapi/catalog_adapter.go index dfb678e65..17f4d6845 100644 --- a/authbridge/authlib/sessionapi/catalog_adapter.go +++ b/authbridge/authlib/sessionapi/catalog_adapter.go @@ -1,6 +1,9 @@ package sessionapi -import "github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins" +import ( + "github.com/kagenti/kagenti-extensions/authbridge/authlib/pipeline" + "github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins" +) // PluginsCatalog adapts plugins.Catalog() into the wire-shaped // CatalogEntry slice WithCatalog expects. Three binaries (authbridge- @@ -11,6 +14,11 @@ import "github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins" // most plugins can be configured into either chain (parsers especially). // abctl renders direction only for the active pipeline, where the // answer is positional, not type-level. +// +// Fields is populated for plugins that implement +// pipeline.SchemaProvider (most config-bearing plugins). Plugins +// without configs emit a nil Fields slice and the wire format +// elides it via `omitempty`. func PluginsCatalog() []CatalogEntry { src := plugins.Catalog() out := make([]CatalogEntry, len(src)) @@ -26,6 +34,30 @@ func PluginsCatalog() []CatalogEntry { After: n.After, Claims: n.Claims, Description: n.Description, + Fields: convertFieldSchemas(e.Fields), + } + } + return out +} + +// convertFieldSchemas maps the framework-level FieldSchema slice to +// the wire-level FieldSchemaEntry slice. Recurses into nested struct +// schemas. Returns nil for empty input so the JSON marshaller can +// elide the field via `omitempty`. +func convertFieldSchemas(in []pipeline.FieldSchema) []FieldSchemaEntry { + if len(in) == 0 { + return nil + } + out := make([]FieldSchemaEntry, len(in)) + for i, f := range in { + out[i] = FieldSchemaEntry{ + Name: f.Name, + Type: f.Type, + Required: f.Required, + Description: f.Description, + Default: f.Default, + Enum: append([]string(nil), f.Enum...), + Fields: convertFieldSchemas(f.Fields), } } return out diff --git a/authbridge/authlib/sessionapi/server.go b/authbridge/authlib/sessionapi/server.go index db6a2fe8f..6d18844a0 100644 --- a/authbridge/authlib/sessionapi/server.go +++ b/authbridge/authlib/sessionapi/server.go @@ -52,16 +52,31 @@ type Server struct { // that documents bodyAccess as deprecated, so there's no compat cost to // emit the right name from day one. type CatalogEntry struct { - Name string `json:"name"` - Direction string `json:"direction,omitempty"` - ReadsBody bool `json:"readsBody,omitempty"` - Writes []string `json:"writes,omitempty"` - Reads []string `json:"reads,omitempty"` - Requires []string `json:"requires,omitempty"` - RequiresAny []string `json:"requiresAny,omitempty"` - After []string `json:"after,omitempty"` - Claims []string `json:"claims,omitempty"` - Description string `json:"description,omitempty"` + Name string `json:"name"` + Direction string `json:"direction,omitempty"` + ReadsBody bool `json:"readsBody,omitempty"` + Writes []string `json:"writes,omitempty"` + Reads []string `json:"reads,omitempty"` + Requires []string `json:"requires,omitempty"` + RequiresAny []string `json:"requiresAny,omitempty"` + After []string `json:"after,omitempty"` + Claims []string `json:"claims,omitempty"` + Description string `json:"description,omitempty"` + Fields []FieldSchemaEntry `json:"fields,omitempty"` +} + +// FieldSchemaEntry is the wire shape for one config field's schema +// metadata. Mirrors pipeline.FieldSchema; lives in the sessionapi +// package so consumers (abctl apiclient, future kagenti-UI clients) +// don't have to import authlib/pipeline transitively. +type FieldSchemaEntry struct { + Name string `json:"name"` + Type string `json:"type"` + Required bool `json:"required,omitempty"` + Description string `json:"description,omitempty"` + Default string `json:"default,omitempty"` + Enum []string `json:"enum,omitempty"` + Fields []FieldSchemaEntry `json:"fields,omitempty"` } // CatalogProvider is the function the binary supplies to expose diff --git a/authbridge/authlib/sessionapi/server_test.go b/authbridge/authlib/sessionapi/server_test.go index f2ca67985..35ba20b83 100644 --- a/authbridge/authlib/sessionapi/server_test.go +++ b/authbridge/authlib/sessionapi/server_test.go @@ -718,6 +718,63 @@ func TestHandlePluginCatalog_ListsRegisteredPlugins(t *testing.T) { } } +// TestHandlePluginCatalog_IncludesFieldSchemas confirms field-level +// schemas attached by the catalog provider make it onto the wire +// (and that omitempty hides them when absent). +func TestHandlePluginCatalog_IncludesFieldSchemas(t *testing.T) { + stub := func() []CatalogEntry { + return []CatalogEntry{ + { + Name: "alpha", + Description: "Has fields", + Fields: []FieldSchemaEntry{ + {Name: "endpoint", Type: "string", Required: true, Description: "API URL."}, + {Name: "policy", Type: "string", Default: "allow", Enum: []string{"allow", "deny"}}, + }, + }, + {Name: "beta", Description: "No fields"}, + } + } + store := session.New(5*time.Minute, 100, 0) + defer store.Close() + srv := New(":0", store, WithCatalog(stub)) + ts := httptest.NewServer(srv.server.Handler) + defer ts.Close() + + resp, err := http.Get(ts.URL + "/v1/plugins") + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + + var body struct { + Plugins []CatalogEntry `json:"plugins"` + } + if err := json.NewDecoder(resp.Body).Decode(&body); err != nil { + t.Fatal(err) + } + if len(body.Plugins) != 2 { + t.Fatalf("plugins = %d, want 2", len(body.Plugins)) + } + // alpha: fields populated, including required/enum/default. + if got := len(body.Plugins[0].Fields); got != 2 { + t.Fatalf("alpha.Fields = %d, want 2", got) + } + if !body.Plugins[0].Fields[0].Required { + t.Errorf("alpha.endpoint should be required") + } + if body.Plugins[0].Fields[1].Default != "allow" { + t.Errorf("alpha.policy.Default = %q", body.Plugins[0].Fields[1].Default) + } + if got := len(body.Plugins[0].Fields[1].Enum); got != 2 { + t.Errorf("alpha.policy.Enum length = %d", got) + } + // beta: no fields → wire format omits the field; decoded slice is nil/empty. + if len(body.Plugins[1].Fields) != 0 { + t.Errorf("beta.Fields should be empty, got %+v", body.Plugins[1].Fields) + } +} + func TestHandlePluginCatalog_NoProvider404(t *testing.T) { store := session.New(5*time.Minute, 100, 0) defer store.Close() diff --git a/authbridge/cmd/abctl/apiclient/client.go b/authbridge/cmd/abctl/apiclient/client.go index 1fe4ac168..06b8eab0c 100644 --- a/authbridge/cmd/abctl/apiclient/client.go +++ b/authbridge/cmd/abctl/apiclient/client.go @@ -119,16 +119,30 @@ type PluginCatalog struct { // post-Normalize); the older bodyAccess alias is intentionally NOT // emitted on this new wire shape. type PluginCatalogEntry struct { - Name string `json:"name"` - Direction string `json:"direction,omitempty"` - ReadsBody bool `json:"readsBody,omitempty"` - Writes []string `json:"writes,omitempty"` - Reads []string `json:"reads,omitempty"` - Requires []string `json:"requires,omitempty"` - RequiresAny []string `json:"requiresAny,omitempty"` - After []string `json:"after,omitempty"` - Claims []string `json:"claims,omitempty"` - Description string `json:"description,omitempty"` + Name string `json:"name"` + Direction string `json:"direction,omitempty"` + ReadsBody bool `json:"readsBody,omitempty"` + Writes []string `json:"writes,omitempty"` + Reads []string `json:"reads,omitempty"` + Requires []string `json:"requires,omitempty"` + RequiresAny []string `json:"requiresAny,omitempty"` + After []string `json:"after,omitempty"` + Claims []string `json:"claims,omitempty"` + Description string `json:"description,omitempty"` + Fields []PluginFieldEntry `json:"fields,omitempty"` +} + +// PluginFieldEntry mirrors sessionapi.FieldSchemaEntry — per-field +// schema metadata for a plugin's config. Used by abctl edit's +// templates renderer; nil for plugins without configs. +type PluginFieldEntry struct { + Name string `json:"name"` + Type string `json:"type"` + Required bool `json:"required,omitempty"` + Description string `json:"description,omitempty"` + Default string `json:"default,omitempty"` + Enum []string `json:"enum,omitempty"` + Fields []PluginFieldEntry `json:"fields,omitempty"` } // GetPluginCatalog fetches /v1/plugins. Returns ErrNotFound when the From 2e2e5c370536be9e4aca3186551b06e3b4835bd0 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 09:41:56 -0400 Subject: [PATCH 04/14] feat(abctl): Render plugin templates below pipeline subtree on edit Builds on the schema layer wired through in the previous commit: when abctl opens $EDITOR on the pipeline subtree, the temp file now also carries a commented templates section listing every registered plugin and its config fields. Operators copy a block, strip the leading "# " prefix, and adjust indentation to fit their inbound: or outbound: chain - no more bouncing to docs to remember a plugin's required fields. Mechanism: - New edit/templates.go renders a fence-marked YAML comment block per plugin in the catalog. Required fields render first inside each block, with placeholders typed to match the field (string -> "", int -> 0, bool -> false, []string -> []) plus an inline comment carrying required/default/enum/description. The documented Default substitutes for the typed placeholder when present, so operators see the actual fallback inline. - FetchCmd gains a catalog parameter. nil produces today's behavior (no templates appended) - used by tests, --endpoint mode without a /v1/plugins fetch, and any future degraded-server path. tui/ passes m.catalog through a small catalogPlugins helper that unwraps the *PluginCatalog pointer. - The fence marker is exported as edit.FenceMarker so the save-side splice logic (commit 5) can match it without a duplicated string. The templates section is pure comments - every emitted line starts with "#". If the operator deletes the fence marker by accident, applying the buffer is still a YAML no-op (comments are discarded by the parser), preserving the safe-fallback the plan committed to. Tests: - TestRenderTemplates_EmptyCatalog (nil and empty cases) - TestRenderTemplates_FenceMarkerPresent - TestRenderTemplates_PluginWithFields covers required-first ordering, Required: header line, default/enum/description inline notes - TestRenderTemplates_PluginNoFields confirms no spurious config: line for parsers without configurable fields - TestRenderTemplates_PlaceholderTypes covers all five typed placeholders plus the documented-default substitution path - TestFetchCmd_AppendsTemplatesWhenCatalogProvided is the end-to-end integration: real ConfigMap fixture + catalog -> tempfile contains both the pipeline subtree and the templates - TestFetchCmd_NoTemplatesWhenCatalogNil pins the nil-catalog fallback so future refactors can't silently emit templates in --endpoint mode Save-side stripping ships in the next commit; until then a preserved fence marker would land in the ConfigMap as comments. This commit is independently safe because the splice path already preserves comments verbatim - nothing breaks if a user saves with the templates section left in place. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/edit/edit.go | 15 +- authbridge/cmd/abctl/edit/edit_test.go | 4 +- authbridge/cmd/abctl/edit/templates.go | 148 +++++++++++++++++++ authbridge/cmd/abctl/edit/templates_test.go | 156 ++++++++++++++++++++ authbridge/cmd/abctl/tui/keys.go | 17 ++- 5 files changed, 335 insertions(+), 5 deletions(-) create mode 100644 authbridge/cmd/abctl/edit/templates.go create mode 100644 authbridge/cmd/abctl/edit/templates_test.go diff --git a/authbridge/cmd/abctl/edit/edit.go b/authbridge/cmd/abctl/edit/edit.go index 794d6b536..ad31a676f 100644 --- a/authbridge/cmd/abctl/edit/edit.go +++ b/authbridge/cmd/abctl/edit/edit.go @@ -7,6 +7,8 @@ import ( "time" tea "github.com/charmbracelet/bubbletea" + + "github.com/kagenti/kagenti-extensions/authbridge/cmd/abctl/apiclient" ) // tempFileMaxAge is how long a stale edit tempfile is allowed to sit @@ -54,7 +56,12 @@ type FetchedMsg struct { // $EDITOR), and emits FetchedMsg. The tempfile lives in $TMPDIR; abctl // leaves it in place on every exit path (success, error, abort) so users // can recover an in-progress edit. -func FetchCmd(ctx context.Context, run Runner, namespace, pod string) tea.Cmd { +// +// catalog is the plugin catalog used to render the templates reference +// section below the pipeline subtree. Pass nil to skip templates (older +// servers without /v1/plugins, --endpoint mode without a catalog fetch, +// or tests). The save path strips the templates section automatically. +func FetchCmd(ctx context.Context, run Runner, namespace, pod string, catalog []apiclient.PluginCatalogEntry) tea.Cmd { return func() tea.Msg { agent, err := ResolveAgentName(ctx, run, namespace, pod) if err != nil { @@ -73,6 +80,12 @@ func FetchCmd(ctx context.Context, run Runner, namespace, pod string) tea.Cmd { tmp.Close() return FetchedMsg{Err: err} } + if templates := RenderTemplates(catalog); len(templates) > 0 { + if _, err := tmp.Write(templates); err != nil { + tmp.Close() + return FetchedMsg{Err: err} + } + } path := tmp.Name() if err := tmp.Close(); err != nil { return FetchedMsg{Err: err} diff --git a/authbridge/cmd/abctl/edit/edit_test.go b/authbridge/cmd/abctl/edit/edit_test.go index 173a9c996..42488c09e 100644 --- a/authbridge/cmd/abctl/edit/edit_test.go +++ b/authbridge/cmd/abctl/edit/edit_test.go @@ -14,7 +14,7 @@ func TestFetchCmd_Success(t *testing.T) { stub := func(ctx context.Context, args ...string) ([]byte, error) { return []byte(fixtureCMYAML), nil } - cmd := FetchCmd(context.Background(), stub, "team1", "email-agent") + cmd := FetchCmd(context.Background(), stub, "team1", "email-agent", nil) msg := cmd().(FetchedMsg) if msg.Err != nil { t.Fatalf("FetchedMsg.Err = %v", msg.Err) @@ -31,7 +31,7 @@ func TestFetchCmd_Error(t *testing.T) { stub := func(ctx context.Context, args ...string) ([]byte, error) { return nil, fmt.Errorf("forbidden") } - cmd := FetchCmd(context.Background(), stub, "team1", "email-agent") + cmd := FetchCmd(context.Background(), stub, "team1", "email-agent", nil) msg := cmd().(FetchedMsg) if msg.Err == nil { t.Fatal("expected error") diff --git a/authbridge/cmd/abctl/edit/templates.go b/authbridge/cmd/abctl/edit/templates.go new file mode 100644 index 000000000..56cdf41ff --- /dev/null +++ b/authbridge/cmd/abctl/edit/templates.go @@ -0,0 +1,148 @@ +package edit + +import ( + "fmt" + "strings" + + "github.com/kagenti/kagenti-extensions/authbridge/cmd/abctl/apiclient" +) + +// FenceMarker delimits the active pipeline subtree (above) from the +// commented templates reference (below) inside the abctl edit tempfile. +// The save path strips everything from this line onward before applying. +// +// The exact bytes matter: detection is a literal line match. Keep them +// in sync with templates_test.go and configmap.go's fence-stripping. +const FenceMarker = "# === ABCTL TEMPLATES BELOW (stripped on save) ===" + +// templatesBanner is the prose shown immediately below FenceMarker, +// telling the operator how to use the reference. +const templatesBanner = `# Reference: every plugin in the catalog. Copy a block above the fence, +# strip the leading "# " from each line, then adjust indentation to fit +# your inbound: or outbound: chain (typically 6-space "- name:" indent). +# This entire section is removed before kubectl apply.` + +// RenderTemplates returns a fence marker followed by a commented YAML +// template block per plugin in the catalog. Returns nil for an empty +// catalog so callers can append unconditionally. +// +// Every emitted line starts with "#" — the templates section is pure +// comments. If the operator deletes the fence marker by accident, the +// templates still parse as comment-only YAML (no semantic effect), +// which is the safe-fallback the plan committed to. +func RenderTemplates(plugins []apiclient.PluginCatalogEntry) []byte { + if len(plugins) == 0 { + return nil + } + var b strings.Builder + b.WriteString("\n") + b.WriteString(FenceMarker) + b.WriteString("\n") + b.WriteString(templatesBanner) + b.WriteString("\n") + for _, p := range plugins { + renderPluginTemplate(&b, p) + } + return []byte(b.String()) +} + +func renderPluginTemplate(b *strings.Builder, p apiclient.PluginCatalogEntry) { + b.WriteString("\n# --- ") + b.WriteString(p.Name) + b.WriteString(" ---\n") + if p.Description != "" { + // Description is single-line (Go struct tags can't carry newlines) + // so a one-line "# " comment is sufficient. + b.WriteString("# ") + b.WriteString(p.Description) + b.WriteString("\n") + } + + // Split fields into required vs optional to render required first — + // required-on-top makes the operator's eye land on the must-fill slots. + var required, optional []apiclient.PluginFieldEntry + for _, f := range p.Fields { + if f.Required { + required = append(required, f) + } else { + optional = append(optional, f) + } + } + if len(required) > 0 { + names := make([]string, len(required)) + for i, f := range required { + names[i] = f.Name + } + b.WriteString("# Required: ") + b.WriteString(strings.Join(names, ", ")) + b.WriteString("\n") + } + + b.WriteString("# - name: ") + b.WriteString(p.Name) + b.WriteString("\n") + if len(p.Fields) == 0 { + b.WriteString("# # (no configurable fields)\n") + return + } + b.WriteString("# config:\n") + for _, f := range required { + renderField(b, f) + } + for _, f := range optional { + renderField(b, f) + } +} + +func renderField(b *strings.Builder, f apiclient.PluginFieldEntry) { + b.WriteString("# ") + b.WriteString(f.Name) + b.WriteString(": ") + b.WriteString(placeholderFor(f)) + + var notes []string + if f.Required { + notes = append(notes, "required") + } + if f.Default != "" { + notes = append(notes, fmt.Sprintf("default=%s", f.Default)) + } + if len(f.Enum) > 0 { + notes = append(notes, "enum="+strings.Join(f.Enum, "|")) + } + if f.Description != "" { + notes = append(notes, f.Description) + } + if len(notes) > 0 { + b.WriteString(" # ") + b.WriteString(strings.Join(notes, "; ")) + } + b.WriteString("\n") +} + +// placeholderFor picks the YAML placeholder that goes after the field +// name. Documented defaults take priority for primitive types so the +// operator sees the actual fallback inline; otherwise an empty value +// matching the field's type. +func placeholderFor(f apiclient.PluginFieldEntry) string { + if f.Default != "" && f.Type != "object" { + // Quote strings so the line is valid YAML when uncommented. + if f.Type == "string" { + return fmt.Sprintf("%q", f.Default) + } + return f.Default + } + switch f.Type { + case "string": + return `""` + case "int": + return "0" + case "bool": + return "false" + case "[]string": + return "[]" + case "object": + return "{}" + } + return `""` +} diff --git a/authbridge/cmd/abctl/edit/templates_test.go b/authbridge/cmd/abctl/edit/templates_test.go new file mode 100644 index 000000000..2fa0921c8 --- /dev/null +++ b/authbridge/cmd/abctl/edit/templates_test.go @@ -0,0 +1,156 @@ +package edit + +import ( + "context" + "os" + "strings" + "testing" + + "github.com/kagenti/kagenti-extensions/authbridge/cmd/abctl/apiclient" +) + +func TestRenderTemplates_EmptyCatalog(t *testing.T) { + if got := RenderTemplates(nil); got != nil { + t.Fatalf("nil catalog should produce nil output, got %q", string(got)) + } + if got := RenderTemplates([]apiclient.PluginCatalogEntry{}); got != nil { + t.Fatalf("empty catalog should produce nil output, got %q", string(got)) + } +} + +func TestRenderTemplates_FenceMarkerPresent(t *testing.T) { + out := RenderTemplates([]apiclient.PluginCatalogEntry{{Name: "noop"}}) + if !strings.Contains(string(out), FenceMarker) { + t.Fatalf("output missing fence marker:\n%s", string(out)) + } +} + +func TestRenderTemplates_PluginWithFields(t *testing.T) { + cat := []apiclient.PluginCatalogEntry{ + { + Name: "ibac", + Description: "Intent-based access control via LLM judge.", + Fields: []apiclient.PluginFieldEntry{ + {Name: "judge_endpoint", Type: "string", Required: true, + Description: "Base URL of the LLM judge."}, + {Name: "judge_model", Type: "string", Required: true, + Description: "Model name."}, + {Name: "timeout_ms", Type: "int", Default: "5000", + Description: "Per-call timeout."}, + {Name: "unclassified_policy", Type: "string", Default: "passthrough", + Enum: []string{"passthrough", "judge"}, + Description: "Behavior when no parser claimed the request."}, + }, + }, + } + out := string(RenderTemplates(cat)) + + for _, want := range []string{ + "# --- ibac ---", + "Intent-based access control via LLM judge.", + "# Required: judge_endpoint, judge_model", + "# - name: ibac", + "# config:", + `# judge_endpoint: "" # required; Base URL of the LLM judge.`, + "# timeout_ms: 5000 # default=5000; Per-call timeout.", + "enum=passthrough|judge", + } { + if !strings.Contains(out, want) { + t.Errorf("output missing %q\n----\n%s", want, out) + } + } + + // Required fields should appear before optional ones. + reqIdx := strings.Index(out, "judge_endpoint:") + optIdx := strings.Index(out, "timeout_ms:") + if reqIdx < 0 || optIdx < 0 { + t.Fatalf("could not locate fields in output:\n%s", out) + } + if reqIdx > optIdx { + t.Errorf("required fields should render before optional ones; got required at %d, optional at %d", reqIdx, optIdx) + } +} + +func TestRenderTemplates_PluginNoFields(t *testing.T) { + cat := []apiclient.PluginCatalogEntry{ + {Name: "a2a-parser", Description: "A2A protocol parser."}, + } + out := string(RenderTemplates(cat)) + if !strings.Contains(out, "# (no configurable fields)") { + t.Fatalf("expected no-fields hint:\n%s", out) + } + if strings.Contains(out, "config:") { + t.Errorf("plugin without fields shouldn't emit config: line:\n%s", out) + } +} + +func TestRenderTemplates_PlaceholderTypes(t *testing.T) { + cat := []apiclient.PluginCatalogEntry{ + { + Name: "broker", + Fields: []apiclient.PluginFieldEntry{ + {Name: "name", Type: "string"}, + {Name: "count", Type: "int"}, + {Name: "flag", Type: "bool"}, + {Name: "items", Type: "[]string"}, + {Name: "nested", Type: "object"}, + {Name: "with_default", Type: "string", Default: "abc"}, + }, + }, + } + out := string(RenderTemplates(cat)) + for _, want := range []string{ + `name: ""`, + "count: 0", + "flag: false", + "items: []", + "nested: {}", + `with_default: "abc"`, // string default should be quoted + } { + if !strings.Contains(out, want) { + t.Errorf("output missing %q\n----\n%s", want, out) + } + } +} + +func TestFetchCmd_AppendsTemplatesWhenCatalogProvided(t *testing.T) { + r := func(_ context.Context, _ ...string) ([]byte, error) { + return []byte(fixtureCMYAML), nil + } + cat := []apiclient.PluginCatalogEntry{ + {Name: "ibac", Description: "test plugin"}, + } + cmd := FetchCmd(context.Background(), r, "team1", "email-agent", cat) + msg := cmd().(FetchedMsg) + if msg.Err != nil { + t.Fatalf("FetchCmd err: %v", msg.Err) + } + body, err := os.ReadFile(msg.TempPath) + if err != nil { + t.Fatalf("read tempfile: %v", err) + } + if !strings.Contains(string(body), FenceMarker) { + t.Fatalf("tempfile missing fence marker; templates not appended:\n%s", string(body)) + } + if !strings.Contains(string(body), "# --- ibac ---") { + t.Fatalf("tempfile missing plugin block:\n%s", string(body)) + } +} + +func TestFetchCmd_NoTemplatesWhenCatalogNil(t *testing.T) { + r := func(_ context.Context, _ ...string) ([]byte, error) { + return []byte(fixtureCMYAML), nil + } + cmd := FetchCmd(context.Background(), r, "team1", "email-agent", nil) + msg := cmd().(FetchedMsg) + if msg.Err != nil { + t.Fatalf("FetchCmd err: %v", msg.Err) + } + body, err := os.ReadFile(msg.TempPath) + if err != nil { + t.Fatalf("read tempfile: %v", err) + } + if strings.Contains(string(body), FenceMarker) { + t.Fatalf("tempfile should not contain fence marker when catalog is nil:\n%s", string(body)) + } +} diff --git a/authbridge/cmd/abctl/tui/keys.go b/authbridge/cmd/abctl/tui/keys.go index 798eb0c9e..75f0b9660 100644 --- a/authbridge/cmd/abctl/tui/keys.go +++ b/authbridge/cmd/abctl/tui/keys.go @@ -6,9 +6,22 @@ import ( tea "github.com/charmbracelet/bubbletea" + "github.com/kagenti/kagenti-extensions/authbridge/cmd/abctl/apiclient" "github.com/kagenti/kagenti-extensions/authbridge/cmd/abctl/edit" ) +// catalogPlugins extracts the plugin slice from a (possibly nil) +// catalog snapshot so FetchCmd can render templates inline. Nil-safe: +// if the catalog hasn't loaded yet (--endpoint mode without +// /v1/plugins, or first-edit-before-poll), the edit just opens +// without templates rather than blocking. +func catalogPlugins(c *apiclient.PluginCatalog) []apiclient.PluginCatalogEntry { + if c == nil { + return nil + } + return c.Plugins +} + // handleKey processes every key press. The filter-input overlay takes // precedence; otherwise keys are dispatched based on the active pane. func (m *model) handleKey(msg tea.KeyMsg) tea.Cmd { @@ -247,7 +260,7 @@ func (m *model) handleKey(msg tea.KeyMsg) tea.Cmd { } gen := m.editState.generation + 1 m.editState = editState{phase: editPhaseFetching, generation: gen} - return withGen(gen, edit.FetchCmd(m.ctx, m.editRunner, m.selectedNamespace, m.selectedPod)) + return withGen(gen, edit.FetchCmd(m.ctx, m.editRunner, m.selectedNamespace, m.selectedPod, catalogPlugins(m.catalog))) case "g": m.goTop() @@ -504,7 +517,7 @@ func (m *model) handleEditKey(msg tea.KeyMsg) tea.Cmd { if m.editState.tempPath == "" { gen := m.editState.generation + 1 m.editState = editState{phase: editPhaseFetching, generation: gen} - return withGen(gen, edit.FetchCmd(m.ctx, m.editRunner, m.selectedNamespace, m.selectedPod)) + return withGen(gen, edit.FetchCmd(m.ctx, m.editRunner, m.selectedNamespace, m.selectedPod, catalogPlugins(m.catalog))) } m.editState.phase = editPhaseEditing return openEditorCmd(m.editState.generation, m.editState.tempPath) From 74cda4022c8c578ef4cacf80ba16279b854e9d71 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 09:49:44 -0400 Subject: [PATCH 05/14] feat(abctl): Strip templates section on save Closes the loop on the templates-in-buffer feature: the read path appends the catalog reference below a fence marker, and the save path now strips the fence and everything after it before any downstream processing. StripTemplates is a strict line-match: a line whose first non- whitespace content is exactly FenceMarker. Whitespace before the marker is tolerated (some editors auto-indent comment lines), and truncation lands at the start of that line so the previous line's trailing newline survives. Inline mentions of the marker text (e.g. inside a comment that happens to discuss the marker) do not trigger truncation. The strip is invoked in tui/app.go immediately after the tempfile read, before the empty-check, YAML parse, splice preview, validation, and diff. Every downstream step sees only the active pipeline subtree - the templates section never makes it to the kubectl apply call. Safe-fallback contract preserved: when the fence marker is absent (catalog wasn't fetched at edit time, --endpoint mode without /v1/plugins, or operator deleted the marker by hand) StripTemplates returns the input unchanged, and applying the buffer remains a YAML no-op for the comment-only templates section even if it somehow survives. Tests: - TestStripTemplates_RemovesFenceAndBelow - happy path: fence followed by template comments is truncated cleanly - TestStripTemplates_NoFenceReturnsUnchanged - input without fence passes through byte-identical - TestStripTemplates_ToleratesLeadingWhitespace - auto-indented fence still matches - TestStripTemplates_FenceAtEOF - fence with no trailing newline still strips correctly - TestStripTemplates_NotFooledBySimilarLine - inline mention of the marker text inside prose does not trigger truncation - TestStripTemplates_IntegratesWithRender - round-trip: render templates after a real subtree, strip them, the subtree's structural content survives byte-for-byte (a renderer-emitted blank line separator may remain as harmless trailing whitespace, which the YAML parser tolerates and the existing trailing-newline normalization in the save path absorbs) End-to-end behavior is now: operator presses 'e' on a pipeline pod, sees the active config plus a commented reference for every registered plugin, copies/edits/saves; abctl strips the reference section, validates, splices, applies, and polls /reload/status. The ConfigMap on disk only ever carries the active config - no templates pollution. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/edit/templates.go | 40 ++++++++++++ authbridge/cmd/abctl/edit/templates_test.go | 69 +++++++++++++++++++++ authbridge/cmd/abctl/tui/app.go | 6 ++ 3 files changed, 115 insertions(+) diff --git a/authbridge/cmd/abctl/edit/templates.go b/authbridge/cmd/abctl/edit/templates.go index 56cdf41ff..d1160e79b 100644 --- a/authbridge/cmd/abctl/edit/templates.go +++ b/authbridge/cmd/abctl/edit/templates.go @@ -120,6 +120,46 @@ func renderField(b *strings.Builder, f apiclient.PluginFieldEntry) { b.WriteString("\n") } +// StripTemplates returns edited with the fence marker and everything +// after it removed. If the fence marker is absent, edited is returned +// unchanged — the safe-fallback contract from the plan: a missing +// fence marker means the operator either deleted it deliberately or +// the catalog wasn't available at fetch time, and either way the +// remaining buffer is what gets applied. +// +// Detection is a strict line match: a line whose first non-whitespace +// content is exactly FenceMarker. Whitespace before the marker is +// tolerated (some editors auto-indent comment lines), but the marker +// itself must be intact. Truncation occurs at the start of that line +// so the trailing newline of the previous line — the active pipeline +// subtree's terminator — survives. +func StripTemplates(edited []byte) []byte { + target := FenceMarker + // Walk lines manually rather than splitting; preserves the byte + // position needed for the truncation cut. + i := 0 + for i < len(edited) { + // Find the next line's end. + end := i + for end < len(edited) && edited[end] != '\n' { + end++ + } + // Strip leading whitespace for the comparison. + start := i + for start < end && (edited[start] == ' ' || edited[start] == '\t') { + start++ + } + if end-start >= len(target) && string(edited[start:start+len(target)]) == target { + // Truncate at i — the start of this line — discarding the + // fence and everything after. + return edited[:i] + } + // Advance past the newline. + i = end + 1 + } + return edited +} + // placeholderFor picks the YAML placeholder that goes after the field // name. Documented defaults take priority for primitive types so the // operator sees the actual fallback inline; otherwise an empty value diff --git a/authbridge/cmd/abctl/edit/templates_test.go b/authbridge/cmd/abctl/edit/templates_test.go index 2fa0921c8..f7a3d040a 100644 --- a/authbridge/cmd/abctl/edit/templates_test.go +++ b/authbridge/cmd/abctl/edit/templates_test.go @@ -137,6 +137,75 @@ func TestFetchCmd_AppendsTemplatesWhenCatalogProvided(t *testing.T) { } } +func TestStripTemplates_RemovesFenceAndBelow(t *testing.T) { + original := "pipeline:\n inbound:\n plugins: []\n" + edited := []byte(original + "\n" + FenceMarker + "\n# --- ibac ---\n# stuff\n") + got := string(StripTemplates(edited)) + if got != original+"\n" { + t.Fatalf("StripTemplates mismatch\nwant: %q\ngot: %q", original+"\n", got) + } +} + +func TestStripTemplates_NoFenceReturnsUnchanged(t *testing.T) { + in := []byte("pipeline:\n inbound:\n plugins: []\n") + got := StripTemplates(in) + if string(got) != string(in) { + t.Fatalf("input without fence should be unchanged\nwant: %q\ngot: %q", string(in), string(got)) + } +} + +func TestStripTemplates_ToleratesLeadingWhitespace(t *testing.T) { + original := "pipeline: {}\n" + edited := []byte(original + " " + FenceMarker + "\n# stuff\n") + got := string(StripTemplates(edited)) + if got != original { + t.Fatalf("leading-whitespace fence not stripped\nwant: %q\ngot: %q", original, got) + } +} + +func TestStripTemplates_FenceAtEOF(t *testing.T) { + original := "pipeline: {}\n" + edited := []byte(original + FenceMarker) + got := string(StripTemplates(edited)) + if got != original { + t.Fatalf("fence-at-eof case\nwant: %q\ngot: %q", original, got) + } +} + +func TestStripTemplates_NotFooledBySimilarLine(t *testing.T) { + // A YAML comment that mentions the marker as part of prose, not on its own line. + in := []byte("pipeline:\n # see " + FenceMarker + " for details\n inbound: {}\n") + got := StripTemplates(in) + if string(got) != string(in) { + t.Fatalf("inline mention should not match; input should be unchanged") + } +} + +func TestStripTemplates_IntegratesWithRender(t *testing.T) { + // Round-trip: render templates after a real subtree, strip them. + // The renderer prefixes a blank-line separator before the fence + // for readability, so the strip preserves that separator. The + // subtree's structural content survives byte-for-byte; the trailing + // blank line is harmless (YAML parser tolerates it). + subtree := []byte("pipeline:\n inbound:\n plugins:\n - name: ibac\n") + templates := RenderTemplates([]apiclient.PluginCatalogEntry{ + {Name: "ibac", Description: "test"}, + }) + combined := append([]byte{}, subtree...) + combined = append(combined, templates...) + stripped := StripTemplates(combined) + if !strings.HasPrefix(string(stripped), string(subtree)) { + t.Fatalf("subtree not preserved at head of stripped output\nwant prefix: %q\ngot: %q", + string(subtree), string(stripped)) + } + // Whatever survives after the subtree should be whitespace only — + // no leaked fence content. + tail := strings.TrimSpace(string(stripped[len(subtree):])) + if tail != "" { + t.Fatalf("strip leaked non-whitespace tail: %q", tail) + } +} + func TestFetchCmd_NoTemplatesWhenCatalogNil(t *testing.T) { r := func(_ context.Context, _ ...string) ([]byte, error) { return []byte(fixtureCMYAML), nil diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 6c4fe6534..125fed065 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -650,6 +650,12 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.editState.err = "read edited file: " + err.Error() return m, nil } + // Strip the templates reference section before any downstream + // processing — the empty-check, YAML parse, splice preview, and + // validation all expect to see only the active pipeline subtree. + // No-op when the catalog wasn't included at fetch time (no fence + // marker present in the buffer). + edited = edit.StripTemplates(edited) // Fix 2: reject empty input before it can silently wipe the pipeline. if len(bytes.TrimSpace(edited)) == 0 { m.editState.phase = editPhaseError From 1992bcd041c79af5f75cb0fe0616d2be9fdb3adc Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 12:55:41 -0400 Subject: [PATCH 06/14] fix(abctl): Fetch catalog on first edit when not yet cached The previous commit assumed the operator had already opened the catalog pane (P) before pressing 'e' to edit. In practice that ordering is the exception, not the rule - operators going straight to edit saw the active pipeline subtree but no templates section, because m.catalog was nil and FetchCmd skipped template rendering. FetchCmd now takes the *apiclient.Client directly. When the supplied cachedCatalog is nil and client is non-nil, it issues GetPluginCatalog inline before composing the tempfile - the catalog is small, the endpoint is in-cluster, and one extra round-trip on the edit hot path is invisible compared to the kubectl ConfigMap fetch already happening. The fetched catalog is returned in FetchedMsg.Catalog so the TUI caches it into m.catalog. Subsequent edits and the catalog pane both reuse the same snapshot - no duplicate fetches. Catalog-fetch failure is non-fatal: the edit still opens without templates, mirroring the older "no catalog" path. An older server (no /v1/plugins) or a transient network blip degrades gracefully instead of blocking the edit. Tests: - TestFetchCmd_FetchesCatalogInlineWhenCacheNil uses httptest to serve /v1/plugins and confirms the rendered tempfile carries both the pipeline subtree and the inline-fetched catalog, plus FetchedMsg.Catalog is populated for TUI caching. - TestFetchCmd_FetcherErrorIsNonFatal confirms a 500-returning catalog endpoint doesn't break the edit flow - tempfile lacks the fence marker, but msg.Err is nil and the path is set. The function signature ended up taking *apiclient.Client concretely rather than a CatalogFetcher interface. Go's nil-interface gotcha (a typed nil pointer assigned to an interface compares != nil) made the interface form silently panic in TUI tests that pass a nil client; the concrete-pointer form lets `client != nil` work as expected without a separate IsZero helper. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/edit/edit.go | 38 ++++++++++--- authbridge/cmd/abctl/edit/edit_test.go | 4 +- authbridge/cmd/abctl/edit/templates_test.go | 60 ++++++++++++++++++++- authbridge/cmd/abctl/tui/app.go | 7 +++ authbridge/cmd/abctl/tui/keys.go | 4 +- 5 files changed, 101 insertions(+), 12 deletions(-) diff --git a/authbridge/cmd/abctl/edit/edit.go b/authbridge/cmd/abctl/edit/edit.go index ad31a676f..db26fc502 100644 --- a/authbridge/cmd/abctl/edit/edit.go +++ b/authbridge/cmd/abctl/edit/edit.go @@ -44,9 +44,14 @@ func SweepStaleTempfiles() int { // FetchedMsg is the result of FetchCmd. On success: Fetched and TempPath // are both set, Err is nil. On failure: Err is populated, others are zero. +// +// Catalog carries the catalog used to render templates, so the TUI can +// cache it into m.catalog after a fresh fetch. Nil when the caller +// supplied a cached catalog or no client. type FetchedMsg struct { Fetched *FetchedPipeline TempPath string // path to the tempfile holding just the pipeline subtree + Catalog *apiclient.PluginCatalog Err error } @@ -57,11 +62,19 @@ type FetchedMsg struct { // leaves it in place on every exit path (success, error, abort) so users // can recover an in-progress edit. // -// catalog is the plugin catalog used to render the templates reference -// section below the pipeline subtree. Pass nil to skip templates (older -// servers without /v1/plugins, --endpoint mode without a catalog fetch, -// or tests). The save path strips the templates section automatically. -func FetchCmd(ctx context.Context, run Runner, namespace, pod string, catalog []apiclient.PluginCatalogEntry) tea.Cmd { +// cachedCatalog is the catalog the TUI has already fetched (e.g. via +// the catalog pane). When non-nil it's used as-is to render templates. +// When nil and client is non-nil, FetchCmd fetches it inline so the +// edit experience works on first 'e' press without requiring the +// operator to open the catalog pane first. Both nil → no templates +// (used by tests and degraded server paths). +func FetchCmd( + ctx context.Context, + run Runner, + client *apiclient.Client, + namespace, pod string, + cachedCatalog []apiclient.PluginCatalogEntry, +) tea.Cmd { return func() tea.Msg { agent, err := ResolveAgentName(ctx, run, namespace, pod) if err != nil { @@ -71,6 +84,19 @@ func FetchCmd(ctx context.Context, run Runner, namespace, pod string, catalog [] if err != nil { return FetchedMsg{Err: err} } + + // Resolve the catalog: prefer cached, otherwise fetch inline. + // Catalog-fetch failure is non-fatal — the edit still opens + // without templates, mirroring the older "no catalog" path. + catalog := cachedCatalog + var freshCatalog *apiclient.PluginCatalog + if catalog == nil && client != nil { + if c, err := client.GetPluginCatalog(ctx); err == nil && c != nil { + freshCatalog = c + catalog = c.Plugins + } + } + tmp, err := os.CreateTemp("", "abctl-pipeline-*.yaml") if err != nil { return FetchedMsg{Err: err} @@ -90,7 +116,7 @@ func FetchCmd(ctx context.Context, run Runner, namespace, pod string, catalog [] if err := tmp.Close(); err != nil { return FetchedMsg{Err: err} } - return FetchedMsg{Fetched: fp, TempPath: path} + return FetchedMsg{Fetched: fp, TempPath: path, Catalog: freshCatalog} } } diff --git a/authbridge/cmd/abctl/edit/edit_test.go b/authbridge/cmd/abctl/edit/edit_test.go index 42488c09e..c21901581 100644 --- a/authbridge/cmd/abctl/edit/edit_test.go +++ b/authbridge/cmd/abctl/edit/edit_test.go @@ -14,7 +14,7 @@ func TestFetchCmd_Success(t *testing.T) { stub := func(ctx context.Context, args ...string) ([]byte, error) { return []byte(fixtureCMYAML), nil } - cmd := FetchCmd(context.Background(), stub, "team1", "email-agent", nil) + cmd := FetchCmd(context.Background(), stub, nil, "team1", "email-agent", nil) msg := cmd().(FetchedMsg) if msg.Err != nil { t.Fatalf("FetchedMsg.Err = %v", msg.Err) @@ -31,7 +31,7 @@ func TestFetchCmd_Error(t *testing.T) { stub := func(ctx context.Context, args ...string) ([]byte, error) { return nil, fmt.Errorf("forbidden") } - cmd := FetchCmd(context.Background(), stub, "team1", "email-agent", nil) + cmd := FetchCmd(context.Background(), stub, nil, "team1", "email-agent", nil) msg := cmd().(FetchedMsg) if msg.Err == nil { t.Fatal("expected error") diff --git a/authbridge/cmd/abctl/edit/templates_test.go b/authbridge/cmd/abctl/edit/templates_test.go index f7a3d040a..7f3de9da6 100644 --- a/authbridge/cmd/abctl/edit/templates_test.go +++ b/authbridge/cmd/abctl/edit/templates_test.go @@ -2,6 +2,8 @@ package edit import ( "context" + "net/http" + "net/http/httptest" "os" "strings" "testing" @@ -120,7 +122,7 @@ func TestFetchCmd_AppendsTemplatesWhenCatalogProvided(t *testing.T) { cat := []apiclient.PluginCatalogEntry{ {Name: "ibac", Description: "test plugin"}, } - cmd := FetchCmd(context.Background(), r, "team1", "email-agent", cat) + cmd := FetchCmd(context.Background(), r, nil, "team1", "email-agent", cat) msg := cmd().(FetchedMsg) if msg.Err != nil { t.Fatalf("FetchCmd err: %v", msg.Err) @@ -206,11 +208,65 @@ func TestStripTemplates_IntegratesWithRender(t *testing.T) { } } +func TestFetchCmd_FetchesCatalogInlineWhenCacheNil(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/v1/plugins" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"plugins":[{"name":"fetched-from-stub"}]}`)) + })) + defer srv.Close() + client := apiclient.New(srv.URL) + r := func(_ context.Context, _ ...string) ([]byte, error) { + return []byte(fixtureCMYAML), nil + } + cmd := FetchCmd(context.Background(), r, client, "team1", "email-agent", nil) + msg := cmd().(FetchedMsg) + if msg.Err != nil { + t.Fatalf("FetchCmd err: %v", msg.Err) + } + body, err := os.ReadFile(msg.TempPath) + if err != nil { + t.Fatalf("read tempfile: %v", err) + } + if !strings.Contains(string(body), "fetched-from-stub") { + t.Fatalf("inline-fetched catalog not rendered:\n%s", string(body)) + } + if msg.Catalog == nil { + t.Fatal("FetchedMsg.Catalog should be set when fetched inline (so the TUI can cache it)") + } +} + +func TestFetchCmd_FetcherErrorIsNonFatal(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + client := apiclient.New(srv.URL) + r := func(_ context.Context, _ ...string) ([]byte, error) { + return []byte(fixtureCMYAML), nil + } + cmd := FetchCmd(context.Background(), r, client, "team1", "email-agent", nil) + msg := cmd().(FetchedMsg) + if msg.Err != nil { + t.Fatalf("catalog-fetch failure should not break edit: %v", msg.Err) + } + body, err := os.ReadFile(msg.TempPath) + if err != nil { + t.Fatalf("read tempfile: %v", err) + } + if strings.Contains(string(body), FenceMarker) { + t.Fatalf("templates should be absent when fetcher errored:\n%s", string(body)) + } +} + func TestFetchCmd_NoTemplatesWhenCatalogNil(t *testing.T) { r := func(_ context.Context, _ ...string) ([]byte, error) { return []byte(fixtureCMYAML), nil } - cmd := FetchCmd(context.Background(), r, "team1", "email-agent", nil) + cmd := FetchCmd(context.Background(), r, nil, "team1", "email-agent", nil) msg := cmd().(FetchedMsg) if msg.Err != nil { t.Fatalf("FetchCmd err: %v", msg.Err) diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 125fed065..928e8f255 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -633,6 +633,13 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.editState.fetched = msg.Fetched m.editState.tempPath = msg.TempPath m.editState.phase = editPhaseEditing + // FetchCmd may have fetched the catalog inline so it could + // render the templates section. Cache it so the catalog pane + // (P) and the post-save validator both reuse it without a + // second round-trip. + if msg.Catalog != nil { + m.catalog = msg.Catalog + } return m, openEditorCmd(m.editState.generation, msg.TempPath) case editorExitedMsg: diff --git a/authbridge/cmd/abctl/tui/keys.go b/authbridge/cmd/abctl/tui/keys.go index 75f0b9660..b55f6a13f 100644 --- a/authbridge/cmd/abctl/tui/keys.go +++ b/authbridge/cmd/abctl/tui/keys.go @@ -260,7 +260,7 @@ func (m *model) handleKey(msg tea.KeyMsg) tea.Cmd { } gen := m.editState.generation + 1 m.editState = editState{phase: editPhaseFetching, generation: gen} - return withGen(gen, edit.FetchCmd(m.ctx, m.editRunner, m.selectedNamespace, m.selectedPod, catalogPlugins(m.catalog))) + return withGen(gen, edit.FetchCmd(m.ctx, m.editRunner, m.client, m.selectedNamespace, m.selectedPod, catalogPlugins(m.catalog))) case "g": m.goTop() @@ -517,7 +517,7 @@ func (m *model) handleEditKey(msg tea.KeyMsg) tea.Cmd { if m.editState.tempPath == "" { gen := m.editState.generation + 1 m.editState = editState{phase: editPhaseFetching, generation: gen} - return withGen(gen, edit.FetchCmd(m.ctx, m.editRunner, m.selectedNamespace, m.selectedPod, catalogPlugins(m.catalog))) + return withGen(gen, edit.FetchCmd(m.ctx, m.editRunner, m.client, m.selectedNamespace, m.selectedPod, catalogPlugins(m.catalog))) } m.editState.phase = editPhaseEditing return openEditorCmd(m.editState.generation, m.editState.tempPath) From 2118a08f1be8815e7c5eedebc280f4d2499d2418 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 13:03:01 -0400 Subject: [PATCH 07/14] fix(abctl): No spurious diff when saving without changes The renderer prefixed a blank line before the fence marker for visual separation. StripTemplates truncated at the fence start, preserving that blank line - so opening edit and saving without changes produced subtree + "\n" instead of subtree, surfacing a spurious "+" line in the diff prompt. Drop the leading newline from the renderer. The fence sits directly against the last line of the active subtree; visual separation is provided by the fence text itself plus the templatesBanner immediately below. Round-trip is now byte-exact: StripTemplates(subtree + RenderTemplates(catalog)) == subtree. Tightened TestStripTemplates_IntegratesWithRender to assert byte-exact equality instead of the loose "subtree is a prefix" check that masked this regression. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/edit/templates.go | 8 +++++++- authbridge/cmd/abctl/edit/templates_test.go | 20 +++++++------------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/authbridge/cmd/abctl/edit/templates.go b/authbridge/cmd/abctl/edit/templates.go index d1160e79b..8f2ae975c 100644 --- a/authbridge/cmd/abctl/edit/templates.go +++ b/authbridge/cmd/abctl/edit/templates.go @@ -30,12 +30,18 @@ const templatesBanner = `# Reference: every plugin in the catalog. Copy a block // comments. If the operator deletes the fence marker by accident, the // templates still parse as comment-only YAML (no semantic effect), // which is the safe-fallback the plan committed to. +// +// Output starts directly with the fence marker (no leading blank +// separator). This matters at save time: the active pipeline subtree +// already ends with its own newline, so a stripped buffer should equal +// the original subtree byte-for-byte. A blank-line separator here +// would survive the strip and surface as a spurious `+` line in the +// no-changes diff. func RenderTemplates(plugins []apiclient.PluginCatalogEntry) []byte { if len(plugins) == 0 { return nil } var b strings.Builder - b.WriteString("\n") b.WriteString(FenceMarker) b.WriteString("\n") b.WriteString(templatesBanner) diff --git a/authbridge/cmd/abctl/edit/templates_test.go b/authbridge/cmd/abctl/edit/templates_test.go index 7f3de9da6..a39182433 100644 --- a/authbridge/cmd/abctl/edit/templates_test.go +++ b/authbridge/cmd/abctl/edit/templates_test.go @@ -184,11 +184,11 @@ func TestStripTemplates_NotFooledBySimilarLine(t *testing.T) { } func TestStripTemplates_IntegratesWithRender(t *testing.T) { - // Round-trip: render templates after a real subtree, strip them. - // The renderer prefixes a blank-line separator before the fence - // for readability, so the strip preserves that separator. The - // subtree's structural content survives byte-for-byte; the trailing - // blank line is harmless (YAML parser tolerates it). + // Round-trip: render templates after a real subtree, strip them, + // and get the subtree back byte-for-byte. This is the no-changes- + // diff case: opening + saving without edits must produce the same + // bytes the operator started with — otherwise the diff prompt + // misleadingly shows a `+` line and asks to apply. subtree := []byte("pipeline:\n inbound:\n plugins:\n - name: ibac\n") templates := RenderTemplates([]apiclient.PluginCatalogEntry{ {Name: "ibac", Description: "test"}, @@ -196,16 +196,10 @@ func TestStripTemplates_IntegratesWithRender(t *testing.T) { combined := append([]byte{}, subtree...) combined = append(combined, templates...) stripped := StripTemplates(combined) - if !strings.HasPrefix(string(stripped), string(subtree)) { - t.Fatalf("subtree not preserved at head of stripped output\nwant prefix: %q\ngot: %q", + if string(stripped) != string(subtree) { + t.Fatalf("round-trip mismatch (would surface as spurious diff)\nwant: %q\ngot: %q", string(subtree), string(stripped)) } - // Whatever survives after the subtree should be whitespace only — - // no leaked fence content. - tail := strings.TrimSpace(string(stripped[len(subtree):])) - if tail != "" { - t.Fatalf("strip leaked non-whitespace tail: %q", tail) - } } func TestFetchCmd_FetchesCatalogInlineWhenCacheNil(t *testing.T) { From c18dcbd79b6f15fab6d9404a5230c44905eaf75a Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 13:25:40 -0400 Subject: [PATCH 08/14] fix(abctl): Make required vs optional scan at the left margin Previous template format hid required-ness in inline "# required;" notes mixed with description text. Operators reading a token-exchange block - which has no required fields - couldn't tell from the inline format alone. New per-field layout: # [REQUIRED] judge_endpoint: "" # [optional, default=5000] timeout_ms: 5000 The bracket prefix sits at the start of the comment so the operator scans down a column to find required fields. Optional fields list their default and enum constraints in the same bracket. Required fields with enums get an extra "choices: a | b | c" line so the constraint is still visible. Per-plugin "Required: ..." header is now always emitted (even when empty) - "Required: (none - every field is optional)" makes the answer affirmative instead of inferred from absence. Tests: - Updated TestRenderTemplates_PluginWithFields for new line format - TestRenderTemplates_AllOptionalShowsRequiredNoneHeader pins the "(none - every field is optional)" path - TestRenderTemplates_RequiredEnumShowsChoices pins the choices line for required+enum fields Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/edit/templates.go | 82 ++++++++++++++------- authbridge/cmd/abctl/edit/templates_test.go | 42 ++++++++++- 2 files changed, 96 insertions(+), 28 deletions(-) diff --git a/authbridge/cmd/abctl/edit/templates.go b/authbridge/cmd/abctl/edit/templates.go index 8f2ae975c..0deb25f40 100644 --- a/authbridge/cmd/abctl/edit/templates.go +++ b/authbridge/cmd/abctl/edit/templates.go @@ -64,8 +64,10 @@ func renderPluginTemplate(b *strings.Builder, p apiclient.PluginCatalogEntry) { b.WriteString("\n") } - // Split fields into required vs optional to render required first — - // required-on-top makes the operator's eye land on the must-fill slots. + // Split fields into required vs optional. Required render first so + // the operator's eye lands on must-fill slots; the "Required:" header + // is always emitted (even when none) so the answer is explicit + // rather than inferred from absence. var required, optional []apiclient.PluginFieldEntry for _, f := range p.Fields { if f.Required { @@ -74,13 +76,17 @@ func renderPluginTemplate(b *strings.Builder, p apiclient.PluginCatalogEntry) { optional = append(optional, f) } } - if len(required) > 0 { - names := make([]string, len(required)) - for i, f := range required { - names[i] = f.Name - } + if len(p.Fields) > 0 { b.WriteString("# Required: ") - b.WriteString(strings.Join(names, ", ")) + if len(required) == 0 { + b.WriteString("(none — every field is optional)") + } else { + names := make([]string, len(required)) + for i, f := range required { + names[i] = f.Name + } + b.WriteString(strings.Join(names, ", ")) + } b.WriteString("\n") } @@ -100,29 +106,55 @@ func renderPluginTemplate(b *strings.Builder, p apiclient.PluginCatalogEntry) { } } +// renderField emits two lines per field for readability: +// +// # # [REQUIRED] +// # : +// +// or, for optional fields: +// +// # # [optional, default=X, enum=a|b] +// # : +// +// The bracket prefix scans at the left margin so the operator can tell +// required from optional without parsing inline notes. func renderField(b *strings.Builder, f apiclient.PluginFieldEntry) { - b.WriteString("# ") - b.WriteString(f.Name) - b.WriteString(": ") - b.WriteString(placeholderFor(f)) - - var notes []string + // Annotation line. + b.WriteString("# # ") if f.Required { - notes = append(notes, "required") - } - if f.Default != "" { - notes = append(notes, fmt.Sprintf("default=%s", f.Default)) - } - if len(f.Enum) > 0 { - notes = append(notes, "enum="+strings.Join(f.Enum, "|")) + b.WriteString("[REQUIRED]") + } else { + var attrs []string + attrs = append(attrs, "optional") + if f.Default != "" { + attrs = append(attrs, fmt.Sprintf("default=%s", f.Default)) + } + if len(f.Enum) > 0 { + attrs = append(attrs, "enum="+strings.Join(f.Enum, "|")) + } + b.WriteString("[") + b.WriteString(strings.Join(attrs, ", ")) + b.WriteString("]") } if f.Description != "" { - notes = append(notes, f.Description) + b.WriteString(" ") + b.WriteString(f.Description) } - if len(notes) > 0 { - b.WriteString(" # ") - b.WriteString(strings.Join(notes, "; ")) + b.WriteString("\n") + + // Required field with an enum: still surface the choices, since + // they're constraint information, not a default note. + if f.Required && len(f.Enum) > 0 { + b.WriteString("# # choices: ") + b.WriteString(strings.Join(f.Enum, " | ")) + b.WriteString("\n") } + + // Value line. + b.WriteString("# ") + b.WriteString(f.Name) + b.WriteString(": ") + b.WriteString(placeholderFor(f)) b.WriteString("\n") } diff --git a/authbridge/cmd/abctl/edit/templates_test.go b/authbridge/cmd/abctl/edit/templates_test.go index a39182433..f3a7a0a39 100644 --- a/authbridge/cmd/abctl/edit/templates_test.go +++ b/authbridge/cmd/abctl/edit/templates_test.go @@ -53,9 +53,11 @@ func TestRenderTemplates_PluginWithFields(t *testing.T) { "# Required: judge_endpoint, judge_model", "# - name: ibac", "# config:", - `# judge_endpoint: "" # required; Base URL of the LLM judge.`, - "# timeout_ms: 5000 # default=5000; Per-call timeout.", - "enum=passthrough|judge", + "# [REQUIRED] Base URL of the LLM judge.", + `# judge_endpoint: ""`, + "# [optional, default=5000] Per-call timeout.", + "# timeout_ms: 5000", + "[optional, default=passthrough, enum=passthrough|judge]", } { if !strings.Contains(out, want) { t.Errorf("output missing %q\n----\n%s", want, out) @@ -86,6 +88,40 @@ func TestRenderTemplates_PluginNoFields(t *testing.T) { } } +func TestRenderTemplates_AllOptionalShowsRequiredNoneHeader(t *testing.T) { + cat := []apiclient.PluginCatalogEntry{ + { + Name: "all-opt", + Fields: []apiclient.PluginFieldEntry{ + {Name: "x", Type: "string"}, + }, + }, + } + out := string(RenderTemplates(cat)) + if !strings.Contains(out, "# Required: (none — every field is optional)") { + t.Fatalf("plugin with no required fields should explicitly say so:\n%s", out) + } +} + +func TestRenderTemplates_RequiredEnumShowsChoices(t *testing.T) { + cat := []apiclient.PluginCatalogEntry{ + { + Name: "p", + Fields: []apiclient.PluginFieldEntry{ + {Name: "mode", Type: "string", Required: true, + Enum: []string{"a", "b", "c"}, Description: "Pick one."}, + }, + }, + } + out := string(RenderTemplates(cat)) + if !strings.Contains(out, "# [REQUIRED] Pick one.") { + t.Errorf("required field annotation missing:\n%s", out) + } + if !strings.Contains(out, "# choices: a | b | c") { + t.Errorf("required+enum field should surface choices line:\n%s", out) + } +} + func TestRenderTemplates_PlaceholderTypes(t *testing.T) { cat := []apiclient.PluginCatalogEntry{ { From a62e703501af2eacc8149aa0ec5185ef63ccdf0a Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 13:49:44 -0400 Subject: [PATCH 09/14] fix(abctl): Surface nested + OR-group requirements in templates Two correctness gaps in the templates reference were hiding real constraints from operators. 1. Nested object fields collapsed to "{}". token-exchange's identity sub-object includes a required `type` field, but the renderer emitted just `identity: {}` with no hint that anything inside was required. Fix: render object-typed fields with sub-fields as a recursive block (parent: sub-fields indented +2). Required and optional sub-fields are reordered the same way the top-level block does, so `[REQUIRED] type:` lands first. 2. The per-plugin "Required:" header only scanned top-level fields. token-exchange showed "(none)" even though identity.type is required. Fix: collectRequiredPaths recurses the whole schema and emits dotted paths (e.g. `identity.type`). Operators see the complete must-fill set in one place at the top of each plugin block. 3. tokenexchange struct tags now reflect what validate() enforces: - identity.type marked required:"true" - token_url description loudly notes the OR-group: "Required unless keycloak_url + keycloak_realm are both set" - keycloak_url / keycloak_realm descriptions: "Required (with ) when token_url is empty" - identity.jwt_audience description: "REQUIRED when type=spiffe" The struct-tag schema can't express OR-groups or conditional requirements directly (one bool per field), so the description text carries the rule. The header captures the unconditional case (identity.type), and the prose in [optional] entries carries the conditional ones. Tests: - TestRenderTemplates_NestedObjectRecurses pins the parent:\n sub-block layout and the +2 indentation contract - TestRenderTemplates_HeaderListsNestedRequiredPaths confirms the dotted-path output (identity.type, not just type) Image rebuild required for the schema-tag change to take effect in /v1/plugins; abctl alone does not. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- .../authlib/plugins/tokenexchange/plugin.go | 10 +- authbridge/cmd/abctl/edit/templates.go | 92 +++++++++++++++---- authbridge/cmd/abctl/edit/templates_test.go | 59 ++++++++++++ 3 files changed, 140 insertions(+), 21 deletions(-) diff --git a/authbridge/authlib/plugins/tokenexchange/plugin.go b/authbridge/authlib/plugins/tokenexchange/plugin.go index 547302bf2..f992ce615 100644 --- a/authbridge/authlib/plugins/tokenexchange/plugin.go +++ b/authbridge/authlib/plugins/tokenexchange/plugin.go @@ -31,13 +31,13 @@ type tokenExchangeConfig struct { // TokenURL is the OAuth token endpoint. Explicit value wins; else // derived from KeycloakURL + KeycloakRealm using Keycloak's // convention. - TokenURL string `json:"token_url" description:"OAuth token endpoint URL. Derived from keycloak_url+realm when empty."` + TokenURL string `json:"token_url" description:"OAuth token endpoint URL. Required unless keycloak_url + keycloak_realm are both set (the plugin derives token_url from the pair)."` // KeycloakURL and KeycloakRealm are a convenience for deriving // TokenURL when the operator prefers to supply Keycloak base + realm // rather than the full token endpoint. - KeycloakURL string `json:"keycloak_url" description:"Internal Keycloak base URL. Used to derive token_url when omitted."` - KeycloakRealm string `json:"keycloak_realm" description:"Keycloak realm name. Pairs with keycloak_url for token_url derivation."` + KeycloakURL string `json:"keycloak_url" description:"Internal Keycloak base URL. Required (with keycloak_realm) when token_url is empty."` + KeycloakRealm string `json:"keycloak_realm" description:"Keycloak realm name. Required (with keycloak_url) when token_url is empty."` // DefaultPolicy is applied when a request's host matches no route: // "passthrough" (default) forwards the request unchanged; @@ -69,7 +69,7 @@ type tokenExchangeConfig struct { type tokenExchangeIdentity struct { // Type is one of "spiffe" or "client-secret". - Type string `json:"type" description:"Identity scheme: spiffe (JWT-SVID assertion) or client-secret." enum:"spiffe,client-secret"` + Type string `json:"type" required:"true" description:"Identity scheme: spiffe (JWT-SVID assertion) or client-secret." enum:"spiffe,client-secret"` // ClientID identifies the client in Keycloak. Explicit value wins; // else read from ClientIDFile at Configure time (or by Init if the @@ -86,7 +86,7 @@ type tokenExchangeIdentity struct { // the RFC 8693 client assertion. Required when Type=="spiffe"; // ignored otherwise. Lives on the plugin (not the framework spiffe // block) because only the spiffe identity path consumes it. - JWTAudience string `json:"jwt_audience" description:"Audience claim minted on the JWT-SVID assertion. Required when type=spiffe."` + JWTAudience string `json:"jwt_audience" description:"Audience claim minted on the JWT-SVID assertion. REQUIRED when type=spiffe; ignored otherwise."` // jwt_svid_path was historically a per-plugin path to the JWT-SVID // file written by spiffe-helper. Removed in favor of injection via diff --git a/authbridge/cmd/abctl/edit/templates.go b/authbridge/cmd/abctl/edit/templates.go index 0deb25f40..312c95920 100644 --- a/authbridge/cmd/abctl/edit/templates.go +++ b/authbridge/cmd/abctl/edit/templates.go @@ -64,10 +64,8 @@ func renderPluginTemplate(b *strings.Builder, p apiclient.PluginCatalogEntry) { b.WriteString("\n") } - // Split fields into required vs optional. Required render first so - // the operator's eye lands on must-fill slots; the "Required:" header - // is always emitted (even when none) so the answer is explicit - // rather than inferred from absence. + // Split top-level fields into required vs optional for ordering + // (required render first inside the config: block). var required, optional []apiclient.PluginFieldEntry for _, f := range p.Fields { if f.Required { @@ -76,16 +74,16 @@ func renderPluginTemplate(b *strings.Builder, p apiclient.PluginCatalogEntry) { optional = append(optional, f) } } + // Header lists every required field anywhere in the schema — + // including nested sub-fields like identity.type — so operators + // see the full set of must-fill slots without having to skim + // the body looking for [REQUIRED] markers inside object blocks. if len(p.Fields) > 0 { b.WriteString("# Required: ") - if len(required) == 0 { + if reqPaths := collectRequiredPaths("", p.Fields); len(reqPaths) == 0 { b.WriteString("(none — every field is optional)") } else { - names := make([]string, len(required)) - for i, f := range required { - names[i] = f.Name - } - b.WriteString(strings.Join(names, ", ")) + b.WriteString(strings.Join(reqPaths, ", ")) } b.WriteString("\n") } @@ -99,13 +97,35 @@ func renderPluginTemplate(b *strings.Builder, p apiclient.PluginCatalogEntry) { } b.WriteString("# config:\n") for _, f := range required { - renderField(b, f) + renderField(b, f, " ") } for _, f := range optional { - renderField(b, f) + renderField(b, f, " ") } } +// collectRequiredPaths walks the schema tree and returns dotted paths +// for every required field. Top-level required fields appear as +// `name`; nested ones as `parent.child` (e.g. `identity.type`). Used +// by the per-plugin "Required:" header so operators see the full +// must-fill set including ones buried inside object sub-trees. +func collectRequiredPaths(prefix string, fields []apiclient.PluginFieldEntry) []string { + var out []string + for _, f := range fields { + path := f.Name + if prefix != "" { + path = prefix + "." + f.Name + } + if f.Required { + out = append(out, path) + } + if f.Type == "object" && len(f.Fields) > 0 { + out = append(out, collectRequiredPaths(path, f.Fields)...) + } + } + return out +} + // renderField emits two lines per field for readability: // // # # [REQUIRED] @@ -116,11 +136,21 @@ func renderPluginTemplate(b *strings.Builder, p apiclient.PluginCatalogEntry) { // # # [optional, default=X, enum=a|b] // # : // +// For nested struct fields with their own sub-fields, the placeholder +// is replaced by a recursive block so operators see required nested +// fields (e.g. identity.type) instead of a misleading "identity: {}". +// // The bracket prefix scans at the left margin so the operator can tell // required from optional without parsing inline notes. -func renderField(b *strings.Builder, f apiclient.PluginFieldEntry) { +// +// indent is the column-prefix for the value line (counted after the +// leading "#"). The default value column is " " (matching +// " config:"); recursive calls deepen by two spaces per level. +func renderField(b *strings.Builder, f apiclient.PluginFieldEntry, indent string) { // Annotation line. - b.WriteString("# # ") + b.WriteString("#") + b.WriteString(indent) + b.WriteString("# ") if f.Required { b.WriteString("[REQUIRED]") } else { @@ -145,13 +175,43 @@ func renderField(b *strings.Builder, f apiclient.PluginFieldEntry) { // Required field with an enum: still surface the choices, since // they're constraint information, not a default note. if f.Required && len(f.Enum) > 0 { - b.WriteString("# # choices: ") + b.WriteString("#") + b.WriteString(indent) + b.WriteString("# choices: ") b.WriteString(strings.Join(f.Enum, " | ")) b.WriteString("\n") } + // Object field with sub-fields: render the sub-tree inline so + // nested required fields (e.g. identity.type) are visible. + if f.Type == "object" && len(f.Fields) > 0 { + b.WriteString("#") + b.WriteString(indent) + b.WriteString(f.Name) + b.WriteString(":\n") + // Reorder sub-fields so required render first within the + // nested block, matching top-level convention. + var req, opt []apiclient.PluginFieldEntry + for _, sf := range f.Fields { + if sf.Required { + req = append(req, sf) + } else { + opt = append(opt, sf) + } + } + nestedIndent := indent + " " + for _, sf := range req { + renderField(b, sf, nestedIndent) + } + for _, sf := range opt { + renderField(b, sf, nestedIndent) + } + return + } + // Value line. - b.WriteString("# ") + b.WriteString("#") + b.WriteString(indent) b.WriteString(f.Name) b.WriteString(": ") b.WriteString(placeholderFor(f)) diff --git a/authbridge/cmd/abctl/edit/templates_test.go b/authbridge/cmd/abctl/edit/templates_test.go index f3a7a0a39..5be52deac 100644 --- a/authbridge/cmd/abctl/edit/templates_test.go +++ b/authbridge/cmd/abctl/edit/templates_test.go @@ -103,6 +103,65 @@ func TestRenderTemplates_AllOptionalShowsRequiredNoneHeader(t *testing.T) { } } +func TestRenderTemplates_HeaderListsNestedRequiredPaths(t *testing.T) { + cat := []apiclient.PluginCatalogEntry{ + { + Name: "te", + Fields: []apiclient.PluginFieldEntry{ + {Name: "token_url", Type: "string"}, + { + Name: "identity", Type: "object", + Fields: []apiclient.PluginFieldEntry{ + {Name: "type", Type: "string", Required: true}, + }, + }, + }, + }, + } + out := string(RenderTemplates(cat)) + if !strings.Contains(out, "# Required: identity.type") { + t.Fatalf("nested required field should appear in header as identity.type:\n%s", out) + } +} + +func TestRenderTemplates_NestedObjectRecurses(t *testing.T) { + cat := []apiclient.PluginCatalogEntry{ + { + Name: "te", + Fields: []apiclient.PluginFieldEntry{ + { + Name: "identity", + Type: "object", + Description: "Client credentials.", + Fields: []apiclient.PluginFieldEntry{ + {Name: "type", Type: "string", Required: true, + Enum: []string{"spiffe", "client-secret"}, + Description: "Identity scheme."}, + {Name: "client_id", Type: "string", + Description: "Inline client id."}, + }, + }, + }, + }, + } + out := string(RenderTemplates(cat)) + if strings.Contains(out, "identity: {}") { + t.Errorf("nested object should NOT collapse to {}; got:\n%s", out) + } + if !strings.Contains(out, "identity:\n") { + t.Errorf("nested object should render `identity:` then sub-fields, got:\n%s", out) + } + if !strings.Contains(out, "[REQUIRED] Identity scheme.") { + t.Errorf("nested required field annotation missing:\n%s", out) + } + // Nested fields should be indented further than the top-level + // config fields. Top-level uses "# " (11 spaces); + // nested adds 2 more. + if !strings.Contains(out, "# type:") { + t.Errorf("nested field should indent by 2 spaces beyond parent:\n%s", out) + } +} + func TestRenderTemplates_RequiredEnumShowsChoices(t *testing.T) { cat := []apiclient.PluginCatalogEntry{ { From e17695adc9c2c4adb5642f2021051217d17e4cf8 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 14:40:05 -0400 Subject: [PATCH 10/14] fix(abctl): Mark parents of required leaves as effectively required MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Templates showed `[optional] Client credentials...` for token-exchange's identity block even though identity.type below it is required. An operator can't legally omit the parent block (validate() would reject the empty Identity.Type), so [optional] was misleading. Add hasRequiredDescendant: an object field whose own tag is not required:"true" but which contains any required field anywhere beneath it is "effectively required" — the operator can't validly leave it out. The renderer: - Annotates effectively-required fields as [REQUIRED] - Promotes them to the top of the parent's config: block (same ordering required leaves get) The "Required:" header keeps emitting leaf paths only (identity.type, not the redundant identity, identity.type) so the operator sees the actionable name without double-counting. Tests: TestRenderTemplates_NestedObjectRecurses now also asserts the parent block carries [REQUIRED] (not [optional]) when its sub-tree contains a required leaf. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/edit/templates.go | 30 ++++++++++++++++++--- authbridge/cmd/abctl/edit/templates_test.go | 6 +++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/authbridge/cmd/abctl/edit/templates.go b/authbridge/cmd/abctl/edit/templates.go index 312c95920..e71d0d067 100644 --- a/authbridge/cmd/abctl/edit/templates.go +++ b/authbridge/cmd/abctl/edit/templates.go @@ -65,10 +65,14 @@ func renderPluginTemplate(b *strings.Builder, p apiclient.PluginCatalogEntry) { } // Split top-level fields into required vs optional for ordering - // (required render first inside the config: block). + // (required render first inside the config: block). Object fields + // with required descendants (e.g. identity wrapping identity.type) + // are treated as effectively required so the parent renders at the + // top of the block, matching the [REQUIRED] annotation it'll get + // from renderField. var required, optional []apiclient.PluginFieldEntry for _, f := range p.Fields { - if f.Required { + if f.Required || hasRequiredDescendant(f) { required = append(required, f) } else { optional = append(optional, f) @@ -104,6 +108,21 @@ func renderPluginTemplate(b *strings.Builder, p apiclient.PluginCatalogEntry) { } } +// hasRequiredDescendant reports whether f or any field beneath it +// carries Required=true. An object field whose own Required tag is +// false but which contains a required leaf (e.g. tokenexchange's +// identity wrapping a required identity.type) is "effectively +// required" — the operator can't legally omit the block, so the +// renderer should show it as [REQUIRED] rather than [optional]. +func hasRequiredDescendant(f apiclient.PluginFieldEntry) bool { + for _, sf := range f.Fields { + if sf.Required || hasRequiredDescendant(sf) { + return true + } + } + return false +} + // collectRequiredPaths walks the schema tree and returns dotted paths // for every required field. Top-level required fields appear as // `name`; nested ones as `parent.child` (e.g. `identity.type`). Used @@ -147,11 +166,16 @@ func collectRequiredPaths(prefix string, fields []apiclient.PluginFieldEntry) [] // leading "#"). The default value column is " " (matching // " config:"); recursive calls deepen by two spaces per level. func renderField(b *strings.Builder, f apiclient.PluginFieldEntry, indent string) { + // Effective required-ness: a field is required either because its + // own tag says so, or because it's an object containing a required + // descendant (omitting the parent block makes the child unsettable). + effectivelyRequired := f.Required || hasRequiredDescendant(f) + // Annotation line. b.WriteString("#") b.WriteString(indent) b.WriteString("# ") - if f.Required { + if effectivelyRequired { b.WriteString("[REQUIRED]") } else { var attrs []string diff --git a/authbridge/cmd/abctl/edit/templates_test.go b/authbridge/cmd/abctl/edit/templates_test.go index 5be52deac..66757c034 100644 --- a/authbridge/cmd/abctl/edit/templates_test.go +++ b/authbridge/cmd/abctl/edit/templates_test.go @@ -154,6 +154,12 @@ func TestRenderTemplates_NestedObjectRecurses(t *testing.T) { if !strings.Contains(out, "[REQUIRED] Identity scheme.") { t.Errorf("nested required field annotation missing:\n%s", out) } + // Object parent of a required leaf should itself read [REQUIRED] + // even though its own tag isn't required:"true" — operators can't + // legally omit the block, so [optional] would mislead. + if !strings.Contains(out, "[REQUIRED] Client credentials.") { + t.Errorf("parent of required leaf should annotate as [REQUIRED]:\n%s", out) + } // Nested fields should be indented further than the top-level // config fields. Top-level uses "# " (11 spaces); // nested adds 2 more. From 003b4fa77c6d5a740e42095f1c206b6026084a3b Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 15:11:23 -0400 Subject: [PATCH 11/14] fix(abctl): Make the templates fence visually separate from config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operators reported the boundary between the active pipeline subtree and the templates reference was hard to see — the fence comment line sat directly under the last plugin's config, so the two blocks ran together visually and operators couldn't tell where their editable content ended. Render two blank lines + the fence + a bordered banner block: ...token-exchange config above... # === ABCTL TEMPLATES BELOW (stripped on save) === # ════════════════════════════════════════════════════════════════════ # # Reference: every plugin in the catalog. # # To use a template, copy a plugin block from below ... # # ════════════════════════════════════════════════════════════════════ The visual padding ABOVE the fence has to be whitespace-only — content lines there would survive strip and surface as a `+` diff on save-without-changes. So all decoration (the ═══ borders, the prose) lives BELOW the fence where it's stripped naturally. To make the leading blank lines safe, StripTemplates now also trims trailing whitespace-only lines after truncating at the fence. The previous real line's terminator stays put. trimTrailingBlankLines + isBlankLine are the small helpers that do this. Tests: - TestStripTemplates_RemovesFenceAndBelow updated: a blank-line separator no longer survives strip - TestStripTemplates_TrimsMultipleBlankLinesBeforeFence pins the multi-blank-line case - TestStripTemplates_TrimsWhitespaceOnlyLineBeforeFence covers spaces / tabs as "blank" - The existing TestStripTemplates_IntegratesWithRender (round-trip) keeps passing — render-then-strip is still byte-identical to the original subtree Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/edit/templates.go | 86 +++++++++++++++++---- authbridge/cmd/abctl/edit/templates_test.go | 26 ++++++- 2 files changed, 97 insertions(+), 15 deletions(-) diff --git a/authbridge/cmd/abctl/edit/templates.go b/authbridge/cmd/abctl/edit/templates.go index e71d0d067..b6d616511 100644 --- a/authbridge/cmd/abctl/edit/templates.go +++ b/authbridge/cmd/abctl/edit/templates.go @@ -1,6 +1,7 @@ package edit import ( + "bytes" "fmt" "strings" @@ -16,11 +17,31 @@ import ( const FenceMarker = "# === ABCTL TEMPLATES BELOW (stripped on save) ===" // templatesBanner is the prose shown immediately below FenceMarker, -// telling the operator how to use the reference. -const templatesBanner = `# Reference: every plugin in the catalog. Copy a block above the fence, -# strip the leading "# " from each line, then adjust indentation to fit -# your inbound: or outbound: chain (typically 6-space "- name:" indent). -# This entire section is removed before kubectl apply.` +// telling the operator how to use the reference. The leading and +// trailing rule lines bracket the fence visually so the boundary +// scans at-a-glance even after a long pipeline subtree. +// +// All lines are below the fence and get stripped on save. The visual +// padding ABOVE the fence is supplied by RenderTemplates as blank +// lines; StripTemplates trims trailing blank lines so the round-trip +// stays byte-identical. +// +// ASCII-only by design: a previous Unicode-bordered banner triggered +// editor-plugin failures on some operator setups. ASCII works +// everywhere without sacrificing visual hierarchy. +const templatesBanner = `# ==================================================================== +# +# Reference: every plugin in the catalog. +# +# To use a template, copy a plugin block from below, paste it above the +# fence into your inbound: or outbound: chain, strip the leading "# " +# from each line, then adjust indentation (the templates use a 6-space +# "- name:" indent -- match whatever your existing plugins use). +# +# This entire section -- from the fence line down -- is stripped before +# the edited buffer is written back to the ConfigMap. +# +# ====================================================================` // RenderTemplates returns a fence marker followed by a commented YAML // template block per plugin in the catalog. Returns nil for an empty @@ -31,17 +52,20 @@ const templatesBanner = `# Reference: every plugin in the catalog. Copy a block // templates still parse as comment-only YAML (no semantic effect), // which is the safe-fallback the plan committed to. // -// Output starts directly with the fence marker (no leading blank -// separator). This matters at save time: the active pipeline subtree -// already ends with its own newline, so a stripped buffer should equal -// the original subtree byte-for-byte. A blank-line separator here -// would survive the strip and surface as a spurious `+` line in the -// no-changes diff. +// Output starts with two blank lines for visual separation from the +// active pipeline subtree above, then the fence marker, then the +// banner block. The blank lines are safe because StripTemplates trims +// trailing whitespace-only lines after truncating at the fence — so a +// render-then-strip round-trip is byte-identical even with the +// padding. A render that emitted CONTENT before the fence would +// survive strip and surface as a spurious `+` line in the no-changes +// diff, which is why all decoration lives below the fence. func RenderTemplates(plugins []apiclient.PluginCatalogEntry) []byte { if len(plugins) == 0 { return nil } var b strings.Builder + b.WriteString("\n\n") b.WriteString(FenceMarker) b.WriteString("\n") b.WriteString(templatesBanner) @@ -255,6 +279,12 @@ func renderField(b *strings.Builder, f apiclient.PluginFieldEntry, indent string // itself must be intact. Truncation occurs at the start of that line // so the trailing newline of the previous line — the active pipeline // subtree's terminator — survives. +// +// After truncating, any trailing whitespace-only lines are also +// removed. RenderTemplates inserts blank lines BEFORE the fence as +// visual padding; without this trim those blank lines would survive +// strip and surface as spurious `+` diff lines on save-without-changes. +// The previous real line's terminating \n is preserved. func StripTemplates(edited []byte) []byte { target := FenceMarker // Walk lines manually rather than splitting; preserves the byte @@ -273,8 +303,9 @@ func StripTemplates(edited []byte) []byte { } if end-start >= len(target) && string(edited[start:start+len(target)]) == target { // Truncate at i — the start of this line — discarding the - // fence and everything after. - return edited[:i] + // fence and everything after, then trim trailing blank + // lines so the renderer can prepend visual padding. + return trimTrailingBlankLines(edited[:i]) } // Advance past the newline. i = end + 1 @@ -282,6 +313,35 @@ func StripTemplates(edited []byte) []byte { return edited } +// trimTrailingBlankLines drops trailing whitespace-only lines from +// out, preserving the terminating newline of the last non-blank line. +// "out" is expected to end with \n (the typical content shape) but +// the function is safe on any input. +func trimTrailingBlankLines(out []byte) []byte { + for len(out) > 0 && out[len(out)-1] == '\n' { + // Find the start of the line that ends at len(out)-1. + prevNL := bytes.LastIndexByte(out[:len(out)-1], '\n') + lineStart := prevNL + 1 // 0 when no earlier \n + line := out[lineStart : len(out)-1] + if !isBlankLine(line) { + break + } + // Drop the blank line; the previous real line's terminating + // \n (at position prevNL) is at out[lineStart-1] and stays. + out = out[:lineStart] + } + return out +} + +func isBlankLine(b []byte) bool { + for _, c := range b { + if c != ' ' && c != '\t' { + return false + } + } + return true +} + // placeholderFor picks the YAML placeholder that goes after the field // name. Documented defaults take priority for primitive types so the // operator sees the actual fallback inline; otherwise an empty value diff --git a/authbridge/cmd/abctl/edit/templates_test.go b/authbridge/cmd/abctl/edit/templates_test.go index 66757c034..be2095c5e 100644 --- a/authbridge/cmd/abctl/edit/templates_test.go +++ b/authbridge/cmd/abctl/edit/templates_test.go @@ -244,8 +244,30 @@ func TestStripTemplates_RemovesFenceAndBelow(t *testing.T) { original := "pipeline:\n inbound:\n plugins: []\n" edited := []byte(original + "\n" + FenceMarker + "\n# --- ibac ---\n# stuff\n") got := string(StripTemplates(edited)) - if got != original+"\n" { - t.Fatalf("StripTemplates mismatch\nwant: %q\ngot: %q", original+"\n", got) + // Trailing blank line(s) before the fence are also stripped — the + // renderer prepends them as visual padding, so preserving them + // would create drift on save-without-changes. + if got != original { + t.Fatalf("StripTemplates mismatch\nwant: %q\ngot: %q", original, got) + } +} + +func TestStripTemplates_TrimsMultipleBlankLinesBeforeFence(t *testing.T) { + original := "pipeline: {}\n" + edited := []byte(original + "\n\n\n" + FenceMarker + "\nbanner\n") + got := string(StripTemplates(edited)) + if got != original { + t.Fatalf("multiple blank lines before fence should all be stripped\nwant: %q\ngot: %q", original, got) + } +} + +func TestStripTemplates_TrimsWhitespaceOnlyLineBeforeFence(t *testing.T) { + // A line with just spaces / tabs is also "blank" for trim purposes. + original := "pipeline: {}\n" + edited := []byte(original + " \t \n" + FenceMarker + "\n") + got := string(StripTemplates(edited)) + if got != original { + t.Fatalf("whitespace-only line before fence should be stripped\nwant: %q\ngot: %q", original, got) } } From b9aced40249960e26ddf917a3923fd2a3bb52e8a Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 15:57:38 -0400 Subject: [PATCH 12/14] fix: Address review feedback on PR #461 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small follow-ups from the review: 1. isBlankLine now treats \r as horizontal whitespace. An editor that saves with CRLF would otherwise leave a stray "\r\n" line surviving trimTrailingBlankLines and break the byte-identical round-trip on save-without-changes. Adds TestStripTemplates_TrimsCRLFBlankLine- BeforeFence to pin the behavior. 2. schemaOfType comment updated to reflect actual unbounded recursion (not "one level"). Plugin configs in practice nest at most one or two levels; a self-referential config would loop, but none exist today. Comment now points future readers at where to add a depth guard if it ever becomes necessary. 3. SchemaOf doc updated to clarify that untagged anonymous/embedded structs are skipped, NOT flattened — the opposite of encoding/json. No current plugin uses untagged embedding, so this is a doc correctness fix rather than a behavior change. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/authlib/pipeline/schema.go | 19 ++++++++++++++----- authbridge/cmd/abctl/edit/templates.go | 6 +++++- authbridge/cmd/abctl/edit/templates_test.go | 13 +++++++++++++ 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/authbridge/authlib/pipeline/schema.go b/authbridge/authlib/pipeline/schema.go index 397602174..e05644883 100644 --- a/authbridge/authlib/pipeline/schema.go +++ b/authbridge/authlib/pipeline/schema.go @@ -87,9 +87,13 @@ type FieldSchema struct { // inspected for type only, not for runtime field values. // // Returns nil if the argument isn't a struct (or pointer to struct). -// Anonymous fields are flattened. Fields without a `json:` tag are -// skipped — explicit JSON tagging is the existing wire convention, -// and untagged fields don't appear in the operator-facing YAML. +// Fields without a `json:` tag are skipped (including untagged +// anonymous/embedded structs — unlike encoding/json, which promotes +// them). Explicit JSON tagging is the existing wire convention, and +// untagged fields don't appear in the operator-facing YAML, so they +// have nothing to surface in the schema either. No current plugin +// uses untagged embedding; if one ever needs that, this helper would +// need to grow flattening support. func SchemaOf(configType any) []FieldSchema { t := reflect.TypeOf(configType) if t == nil { @@ -136,8 +140,13 @@ func schemaOfType(t reflect.Type) []FieldSchema { } } if schema.Type == "object" { - // Recurse one level. Operators rarely have deeply-nested - // configs; if the need arises later we can lift this guard. + // Recurse unboundedly. Plugin configs in practice nest at + // most one or two levels (e.g. tokenexchange's identity + + // routes blocks); pathological self-referential configs + // (a struct containing *Self) would loop, but no such + // shape exists today and the framework rejects exotic + // configs at decode time anyway. If a depth guard becomes + // necessary, this is the spot for it. schema.Fields = schemaOfType(unwrap(f.Type)) } out = append(out, schema) diff --git a/authbridge/cmd/abctl/edit/templates.go b/authbridge/cmd/abctl/edit/templates.go index b6d616511..e1bdae7c8 100644 --- a/authbridge/cmd/abctl/edit/templates.go +++ b/authbridge/cmd/abctl/edit/templates.go @@ -333,9 +333,13 @@ func trimTrailingBlankLines(out []byte) []byte { return out } +// isBlankLine reports whether b contains only horizontal whitespace. +// '\r' counts too: an editor that saves with CRLF line endings would +// otherwise leave a stray "\r\n" line surviving the trim and break +// the byte-identical round-trip on save-without-changes. func isBlankLine(b []byte) bool { for _, c := range b { - if c != ' ' && c != '\t' { + if c != ' ' && c != '\t' && c != '\r' { return false } } diff --git a/authbridge/cmd/abctl/edit/templates_test.go b/authbridge/cmd/abctl/edit/templates_test.go index be2095c5e..0e2a5e117 100644 --- a/authbridge/cmd/abctl/edit/templates_test.go +++ b/authbridge/cmd/abctl/edit/templates_test.go @@ -271,6 +271,19 @@ func TestStripTemplates_TrimsWhitespaceOnlyLineBeforeFence(t *testing.T) { } } +func TestStripTemplates_TrimsCRLFBlankLineBeforeFence(t *testing.T) { + // An editor that normalizes line endings to CRLF on save would + // leave "\r\n" blank lines above the fence. Without the \r-aware + // blank check, those would survive the trim and break the + // byte-identical round-trip the PR is built around. + original := "pipeline: {}\n" + edited := []byte(original + "\r\n\r\n" + FenceMarker + "\n") + got := string(StripTemplates(edited)) + if got != original { + t.Fatalf("CRLF blank line before fence should be stripped\nwant: %q\ngot: %q", original, got) + } +} + func TestStripTemplates_NoFenceReturnsUnchanged(t *testing.T) { in := []byte("pipeline:\n inbound:\n plugins: []\n") got := StripTemplates(in) From 17bc1f3ff4b571d142bbea2b1d9fc7357971d225 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 16:07:02 -0400 Subject: [PATCH 13/14] fix(abctl): Require exact fence-line match before stripping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit caught that the fence detection was a prefix match: a line starting with FenceMarker followed by extra content would still trigger truncation. The doc explicitly promised "marker must be intact", so the looser match was a contract violation — and an operator who happened to type past the marker on that line would silently lose their edits below. Replace the prefix compare with an exact length+content compare, tolerating only a trailing \r so CRLF line endings on the fence line itself still match. Anything else after the marker (a stray comment, a typo, an accidental keystroke) now fails the match and the strip is skipped — the operator gets the safe fallback path. Tests: - TestStripTemplates_RequiresExactFenceMatch — line with trailing content stays intact - TestStripTemplates_AcceptsCRLFFenceLine — CRLF on the marker line itself still matches (the tolerated suffix) Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/edit/templates.go | 11 +++++++++- authbridge/cmd/abctl/edit/templates_test.go | 23 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/authbridge/cmd/abctl/edit/templates.go b/authbridge/cmd/abctl/edit/templates.go index e1bdae7c8..32bca5c27 100644 --- a/authbridge/cmd/abctl/edit/templates.go +++ b/authbridge/cmd/abctl/edit/templates.go @@ -301,7 +301,16 @@ func StripTemplates(edited []byte) []byte { for start < end && (edited[start] == ' ' || edited[start] == '\t') { start++ } - if end-start >= len(target) && string(edited[start:start+len(target)]) == target { + // Exact line match: tolerate a trailing CR (CRLF endings) but + // reject any other extra content. A prefix match would let + // "# === ABCTL TEMPLATES BELOW (stripped on save) ===extra" + // trigger truncation, silently dropping operator edits made + // on a line that happens to begin with the marker. + lineEnd := end + if lineEnd > start && edited[lineEnd-1] == '\r' { + lineEnd-- + } + if lineEnd-start == len(target) && string(edited[start:lineEnd]) == target { // Truncate at i — the start of this line — discarding the // fence and everything after, then trim trailing blank // lines so the renderer can prepend visual padding. diff --git a/authbridge/cmd/abctl/edit/templates_test.go b/authbridge/cmd/abctl/edit/templates_test.go index 0e2a5e117..4fb435367 100644 --- a/authbridge/cmd/abctl/edit/templates_test.go +++ b/authbridge/cmd/abctl/edit/templates_test.go @@ -319,6 +319,29 @@ func TestStripTemplates_NotFooledBySimilarLine(t *testing.T) { } } +func TestStripTemplates_RequiresExactFenceMatch(t *testing.T) { + // A line that starts with the marker but has trailing extra content + // must NOT match — otherwise an operator's edits on that line could + // be silently truncated. + original := "pipeline: {}\n" + in := []byte(original + FenceMarker + " trailing comment\nstill active\n") + got := StripTemplates(in) + if string(got) != string(in) { + t.Fatalf("fence with trailing content should not trigger truncation\nwant: %q\ngot: %q", string(in), string(got)) + } +} + +func TestStripTemplates_AcceptsCRLFFenceLine(t *testing.T) { + // CRLF line endings on the fence line itself must still match — + // a trailing CR is the one tolerated suffix. + original := "pipeline: {}\n" + in := []byte(original + FenceMarker + "\r\nbanner\r\n") + got := StripTemplates(in) + if string(got) != original { + t.Fatalf("CRLF on fence line should still match\nwant: %q\ngot: %q", original, string(got)) + } +} + func TestStripTemplates_IntegratesWithRender(t *testing.T) { // Round-trip: render templates after a real subtree, strip them, // and get the subtree back byte-for-byte. This is the no-changes- From c051be06abbc4591a38e69c5cf40daa2c2c75ad6 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Mon, 1 Jun 2026 16:36:02 -0400 Subject: [PATCH 14/14] fix: Address review feedback from cwiklik + CodeRabbit on PR #461 Two follow-ups from the review pass: 1. Bound schemaOfType recursion at depth 10 (cwiklik's defensive suggestion). Plugin configs nest one or two levels in practice, so the cap is generous; its job is to keep an inadvertent self-referential field (e.g. *Self) from silently stack-overflowing. New TestSchemaOf_SelfReferentialIsBounded pins the behavior with a linked-list-style node{Next *node}. 2. New TestGetPluginCatalog_DecodesFieldSchemas in apiclient (CodeRabbit tag-drift-guard suggestion). Server-side FieldSchemaEntry and client-side PluginFieldEntry have aligned JSON tags today; this test would catch a future drift by decoding a realistic /v1/plugins payload (including a nested object field with required + enum + default) and asserting every metadata key round-trips. The existing TestGetPluginCatalog stays focused on basic plugin metadata. Skipped: CodeRabbit nit about edit_test.go lacking catalog-path coverage. Those paths ARE covered, just in templates_test.go: TestFetchCmd_AppendsTemplatesWhenCatalogProvided (cached catalog + FenceMarker assertion) and TestFetchCmd_FetchesCatalogInlineWhenCacheNil (httptest.Server + Catalog propagation assertion). The reviewer appears to have looked at edit_test.go in isolation. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/authlib/pipeline/schema.go | 27 ++++--- authbridge/authlib/pipeline/schema_test.go | 27 +++++++ authbridge/cmd/abctl/apiclient/client_test.go | 75 +++++++++++++++++++ 3 files changed, 119 insertions(+), 10 deletions(-) diff --git a/authbridge/authlib/pipeline/schema.go b/authbridge/authlib/pipeline/schema.go index e05644883..b00218970 100644 --- a/authbridge/authlib/pipeline/schema.go +++ b/authbridge/authlib/pipeline/schema.go @@ -105,10 +105,20 @@ func SchemaOf(configType any) []FieldSchema { if t.Kind() != reflect.Struct { return nil } - return schemaOfType(t) + return schemaOfType(t, 0) } -func schemaOfType(t reflect.Type) []FieldSchema { +// maxSchemaDepth caps recursion in schemaOfType. Plugin configs in +// practice nest one or two levels (tokenexchange's identity + routes +// blocks are the deepest today); the cap is defensive against a +// future config struct with an inadvertent self-referential field +// (e.g. a *Self pointer) which would otherwise stack-overflow. +const maxSchemaDepth = 10 + +func schemaOfType(t reflect.Type, depth int) []FieldSchema { + if depth > maxSchemaDepth { + return nil + } var out []FieldSchema for i := 0; i < t.NumField(); i++ { f := t.Field(i) @@ -140,14 +150,11 @@ func schemaOfType(t reflect.Type) []FieldSchema { } } if schema.Type == "object" { - // Recurse unboundedly. Plugin configs in practice nest at - // most one or two levels (e.g. tokenexchange's identity + - // routes blocks); pathological self-referential configs - // (a struct containing *Self) would loop, but no such - // shape exists today and the framework rejects exotic - // configs at decode time anyway. If a depth guard becomes - // necessary, this is the spot for it. - schema.Fields = schemaOfType(unwrap(f.Type)) + // Recurse with a depth bound (see maxSchemaDepth). Plugin + // configs nest one or two levels in practice; the cap + // guards against an inadvertent self-referential field + // (e.g. *Self) silently blowing the stack. + schema.Fields = schemaOfType(unwrap(f.Type), depth+1) } out = append(out, schema) } diff --git a/authbridge/authlib/pipeline/schema_test.go b/authbridge/authlib/pipeline/schema_test.go index f7e0b9d27..c58576672 100644 --- a/authbridge/authlib/pipeline/schema_test.go +++ b/authbridge/authlib/pipeline/schema_test.go @@ -140,3 +140,30 @@ func TestSchemaOf_EnumWhitespaceAndEmpty(t *testing.T) { t.Errorf("expected trimmed [on, off], got %+v", got) } } + +// Self-referential pointer fields should not stack-overflow — the +// depth bound in schemaOfType caps recursion. A linked-list-style +// node (Next *node) is the canonical case. +func TestSchemaOf_SelfReferentialIsBounded(t *testing.T) { + type node struct { + Name string `json:"name"` + Next *node `json:"next"` + } + // Should return without panicking and emit a finite schema. + got := SchemaOf(node{}) + if len(got) != 2 { + t.Fatalf("expected 2 top-level fields, got %d", len(got)) + } + // The recursion eventually returns nil at depth > maxSchemaDepth, + // so somewhere in the nested chain Fields is nil. Walk down and + // confirm we don't loop forever (the test itself would hang). + depth := 0 + cur := got[1] // "next" + for cur.Type == "object" && len(cur.Fields) >= 2 { + depth++ + if depth > maxSchemaDepth+5 { + t.Fatalf("recursion not bounded; reached depth %d", depth) + } + cur = cur.Fields[1] + } +} diff --git a/authbridge/cmd/abctl/apiclient/client_test.go b/authbridge/cmd/abctl/apiclient/client_test.go index 428e489e1..76d78dc70 100644 --- a/authbridge/cmd/abctl/apiclient/client_test.go +++ b/authbridge/cmd/abctl/apiclient/client_test.go @@ -179,6 +179,81 @@ func TestGetPluginCatalog(t *testing.T) { } } +// TestGetPluginCatalog_DecodesFieldSchemas guards against tag drift +// between server-side FieldSchemaEntry (authlib/sessionapi/server.go) +// and client-side PluginFieldEntry (here in apiclient). Every JSON +// key the server emits must decode into the matching Go field, or the +// abctl edit templates renderer silently loses metadata. Covers +// nested fields too — token-exchange's identity sub-schema is the +// real-world use case. +func TestGetPluginCatalog_DecodesFieldSchemas(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/v1/plugins" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "plugins": [ + { + "name": "te", + "description": "Token exchange.", + "fields": [ + {"name": "token_url", "type": "string", "description": "Endpoint."}, + { + "name": "identity", + "type": "object", + "description": "Client credentials.", + "fields": [ + {"name": "type", "type": "string", "required": true, + "description": "Scheme.", "enum": ["spiffe", "client-secret"]}, + {"name": "timeout_ms", "type": "int", "default": "5000"} + ] + } + ] + } + ] + }`)) + })) + defer ts.Close() + + c := New(ts.URL) + cat, err := c.GetPluginCatalog(context.Background()) + if err != nil { + t.Fatalf("GetPluginCatalog: %v", err) + } + if len(cat.Plugins) != 1 || len(cat.Plugins[0].Fields) != 2 { + t.Fatalf("unexpected catalog shape: %+v", cat) + } + te := cat.Plugins[0] + + tokenURL := te.Fields[0] + if tokenURL.Name != "token_url" || tokenURL.Type != "string" || tokenURL.Description != "Endpoint." { + t.Errorf("token_url decode mismatch: %+v", tokenURL) + } + + id := te.Fields[1] + if id.Name != "identity" || id.Type != "object" || id.Description != "Client credentials." { + t.Errorf("identity decode mismatch: %+v", id) + } + if len(id.Fields) != 2 { + t.Fatalf("identity.Fields = %d, want 2", len(id.Fields)) + } + + idType := id.Fields[0] + if idType.Name != "type" || !idType.Required || idType.Description != "Scheme." { + t.Errorf("identity.type decode mismatch: %+v", idType) + } + if len(idType.Enum) != 2 || idType.Enum[0] != "spiffe" || idType.Enum[1] != "client-secret" { + t.Errorf("identity.type.enum = %v", idType.Enum) + } + + timeout := id.Fields[1] + if timeout.Name != "timeout_ms" || timeout.Type != "int" || timeout.Default != "5000" { + t.Errorf("identity.timeout_ms decode mismatch: %+v", timeout) + } +} + // TestPipelinePluginDecodesCapabilityMetadata verifies the new // metadata fields decode correctly through the apiclient. func TestPipelinePluginDecodesCapabilityMetadata(t *testing.T) {