Feat: Inline plugin-config templates in abctl edit#461
Merged
Conversation
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) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
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>-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) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
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) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
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] <description> judge_endpoint: "" # [optional, default=5000] <description> 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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
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: <newline> 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
<other>) 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) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
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) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds reflection-based plugin config schema extraction (SchemaProvider, FieldSchema, SchemaOf), updates plugins to publish schemas, includes schemas in the registry and /v1/plugins wire model, and renders/removes fenced template blocks in abctl edit; tests cover schemas, templates, and round-trips. ChangesPlugin Configuration Schema Introspection and Edit Template Rendering
Sequence DiagramsequenceDiagram
participant TUI as TUI App
participant FetchCmd as edit.FetchCmd
participant APIClient as API Client
participant Catalog as /v1/plugins
participant RenderT as RenderTemplates
participant TempFile as Temp YAML
participant Editor as User Editor
participant StripT as StripTemplates
TUI->>FetchCmd: Call with client & namespace/pod
FetchCmd->>APIClient: Fetch plugin catalog (if needed)
APIClient->>Catalog: GET /v1/plugins
Catalog-->>APIClient: CatalogEntry[].Fields
APIClient-->>FetchCmd: PluginCatalog
FetchCmd->>RenderT: Render templates from plugins
RenderT-->>FetchCmd: Fenced YAML blocks
FetchCmd->>TempFile: Write pipeline subtree + templates
TempFile-->>TUI: Return FetchedMsg with catalog
TUI->>Editor: Open tempfile
Editor->>Editor: User edits
Editor-->>TUI: Edited buffer
TUI->>StripT: Remove templates & fence
StripT-->>TUI: Clean pipeline YAML
TUI->>TUI: Parse & validate clean subtree
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
authbridge/authlib/sessionapi/server.go (1)
68-80: ⚡ Quick winWire JSON tags for FieldSchemaEntry match apiclient.PluginFieldEntry (server-side contract is aligned).
FieldSchemaEntryandPluginFieldEntryuse identicaljsontags forname,type,required,description,default,enum, andfields(includingomitempty), so schema fields should decode correctly. (Optional: extend the inline-fetch test to assert schema fields to guard against future tag drift.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@authbridge/authlib/sessionapi/server.go` around lines 68 - 80, FieldSchemaEntry's JSON tags must exactly match the server-side contract represented by apiclient.PluginFieldEntry so schema fields decode correctly; verify that the struct FieldSchemaEntry (fields Name, Type, Required, Description, Default, Enum, Fields) uses the same json tags and omitempty behavior as PluginFieldEntry and, if missing, update the tags on FieldSchemaEntry to match PluginFieldEntry exactly; optionally add/extend the inline-fetch test to assert decoded schema fields for FieldSchemaEntry to detect future tag drift.authbridge/cmd/abctl/edit/edit_test.go (1)
17-18: ⚡ Quick winAdd coverage for the new catalog-backed fetch paths.
These tests still only hit the legacy
client == nilbranch, so regressions in inline/v1/pluginsfetch, template append, andFetchedMsg.Catalogpropagation will slip through. Please add one case with a cached catalog and one case with anhttptest.Server, then assert the tempfile containsFenceMarkerand the fresh catalog is returned when fetched inline.Also applies to: 34-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@authbridge/cmd/abctl/edit/edit_test.go` around lines 17 - 18, Add test cases exercising the catalog-backed fetch paths: create one case where FetchCmd is invoked with a cached catalog (populate a temporary catalog file/structure and ensure FetchCmd uses that cache) and one case using an httptest.Server that serves the /v1/plugins template/catalog response; call FetchCmd(context.Background(), stub, <clientOrURL>, "team1", "email-agent", nil) and cast the result to FetchedMsg, then assert that the tempfile produced contains the FenceMarker string and that FetchedMsg.Catalog contains the fresh catalog returned by the server (or cached catalog in the cached case). Ensure tests hit the branch where client != nil so template append and inline /v1/plugins handling in FetchCmd and the FetchedMsg.Catalog propagation are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@authbridge/cmd/abctl/edit/templates.go`:
- Around line 336-342: The isBlankLine function currently only treats spaces and
tabs as blank, so CRLF-terminated blank lines (containing '\r') aren't
recognized; update isBlankLine to also consider '\r' as whitespace (i.e., treat
byte value '\r' as blank alongside ' ' and '\t') so trimTrailingBlankLines will
remove CRLF blank lines; locate and change the isBlankLine function accordingly.
- Around line 299-308: The current truncation logic allows any line that starts
with the fence (variable target) to match, stripping lines like "FenceMarker ...
extra"; change the condition in the function containing variables edited, start,
end, and target so it only truncates when the fence is an exact line (allowing
only optional trailing spaces/tabs). Concretely, replace the end-start >=
len(target) check with logic that (1) ensures edited[start:start+len(target)] ==
target and (2) that either start+len(target) == end or every character in
edited[start+len(target):end] is a space or tab; only then perform the
trimTrailingBlankLines(edited[:i]) return.
---
Nitpick comments:
In `@authbridge/authlib/sessionapi/server.go`:
- Around line 68-80: FieldSchemaEntry's JSON tags must exactly match the
server-side contract represented by apiclient.PluginFieldEntry so schema fields
decode correctly; verify that the struct FieldSchemaEntry (fields Name, Type,
Required, Description, Default, Enum, Fields) uses the same json tags and
omitempty behavior as PluginFieldEntry and, if missing, update the tags on
FieldSchemaEntry to match PluginFieldEntry exactly; optionally add/extend the
inline-fetch test to assert decoded schema fields for FieldSchemaEntry to detect
future tag drift.
In `@authbridge/cmd/abctl/edit/edit_test.go`:
- Around line 17-18: Add test cases exercising the catalog-backed fetch paths:
create one case where FetchCmd is invoked with a cached catalog (populate a
temporary catalog file/structure and ensure FetchCmd uses that cache) and one
case using an httptest.Server that serves the /v1/plugins template/catalog
response; call FetchCmd(context.Background(), stub, <clientOrURL>, "team1",
"email-agent", nil) and cast the result to FetchedMsg, then assert that the
tempfile produced contains the FenceMarker string and that FetchedMsg.Catalog
contains the fresh catalog returned by the server (or cached catalog in the
cached case). Ensure tests hit the branch where client != nil so template append
and inline /v1/plugins handling in FetchCmd and the FetchedMsg.Catalog
propagation are validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e41edc41-2c2d-456d-bf93-16cf25061b41
📒 Files selected for processing (17)
authbridge/authlib/pipeline/schema.goauthbridge/authlib/pipeline/schema_test.goauthbridge/authlib/plugins/ibac/plugin.goauthbridge/authlib/plugins/jwtvalidation/plugin.goauthbridge/authlib/plugins/registry.goauthbridge/authlib/plugins/tokenbroker/plugin.goauthbridge/authlib/plugins/tokenexchange/plugin.goauthbridge/authlib/sessionapi/catalog_adapter.goauthbridge/authlib/sessionapi/server.goauthbridge/authlib/sessionapi/server_test.goauthbridge/cmd/abctl/apiclient/client.goauthbridge/cmd/abctl/edit/edit.goauthbridge/cmd/abctl/edit/edit_test.goauthbridge/cmd/abctl/edit/templates.goauthbridge/cmd/abctl/edit/templates_test.goauthbridge/cmd/abctl/tui/app.goauthbridge/cmd/abctl/tui/keys.go
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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
cwiklik
approved these changes
Jun 1, 2026
Collaborator
cwiklik
left a comment
cwiklik
left a comment
There was a problem hiding this comment.
Well-structured PR — the layered design (reflection → plugin annotations → wire format → template renderer → strip-on-save) is clean and the test coverage is thorough (~583 lines of tests for ~1,128 added). All 13 commits signed-off, CI green.
Two minor observations inline (suggestion + nit), neither blocking.
Areas reviewed: Go (schema infra, plugin annotations, sessionapi wire, abctl edit/templates, TUI integration), Tests
Commits: 13 commits, all signed-off
CI status: all passing
| return out | ||
| } | ||
|
|
||
| // kindOf returns the coarse-grained Type tag for a field. |
Collaborator
There was a problem hiding this comment.
suggestion: The unbounded recursion note is well-documented, but consider adding a simple depth counter (e.g., max 10) as a safety net. A future plugin config struct with an inadvertent self-referential pointer field would stack overflow silently. Low risk today but cheap insurance.
func schemaOfType(t reflect.Type) []FieldSchema {
return schemaOfTypeDepth(t, 0)
}
func schemaOfTypeDepth(t reflect.Type, depth int) []FieldSchema {
if depth > 10 {
return nil
}
// ... existing logic, recurse with depth+1
}| 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') |
Collaborator
There was a problem hiding this comment.
nit: The fence-match check end-start >= len(target) means a line like # === ABCTL TEMPLATES BELOW (stripped on save) ===EXTRA would match. TestStripTemplates_NotFooledBySimilarLine covers inline mentions but not trailing garbage after the marker. Consider tightening to start+len(target) == end for an exact match (after stripping leading whitespace).
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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Contributor
Author
|
Pushed
All packages green: |
github-project-automation
Bot
moved this from New /:ToDo
to Done
in Kagenti Issue Prioritization
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an inline plugin-config template reference to
abctl edit. Whenoperators open the pipeline editor, they now see — below a fence
marker in the same buffer — a commented YAML template for every
plugin in the catalog, annotated with required/optional field
markers, defaults, and prose descriptions. Templates are stripped on
save.
The structurally-foundational piece is per-field plugin schemas,
reflected from struct tags on each plugin's config struct. This
unlocks the templates feature here and any future config-aware
tooling (kagenti-UI forms, JSON-Schema publishing, schema-driven
linters).
Layered design
authlib/pipeline/schema.goauthlib/plugins/{ibac,jwtvalidation,mcpparser,tokenexchange,tokenbroker}fieldsauthlib/sessionapi/catalog_adapter.goauthlib/sessionapi/server.gocmd/abctl/edit/templates.gocmd/abctl/edit/templates.go(StripTemplates)What the operator sees
Round-trip safety
The save path (StripTemplates) truncates at the fence marker and
trims trailing blank lines so opening + saving without edits produces
the original ConfigMap bytes byte-for-byte. No spurious
+diff.If the operator deletes the fence marker by accident, the whole
buffer applies — templates are comment-only YAML, so applying them
is a YAML-parser no-op.
Effective-required propagation
An object field whose own tag isn't
required:"true"but whichcontains a required descendant — e.g.
tokenexchange.identitywitha required
identity.typeinside — is treated as "effectivelyrequired": the operator can't legally omit the parent block. The
renderer annotates such parents as
[REQUIRED]and promotes them tothe top of the parent's config block. The
Required:header listsevery required leaf path (e.g.
identity.type).Tag vocabulary (additive)
The struct-tag schema can't express OR-groups or conditional
requirements directly (one bool per field), so descriptions carry
those rules in prose — e.g. token-exchange.token_url's description
notes "Required unless keycloak_url + keycloak_realm are both set".
Commits
Testing
End-to-end smoke against
team1/email-agent:go test ./...clean across authlib and cmd/abctlAssisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
New Features
Tests