feat: Plugin catalog + abctl visibility & validation#449
Merged
Conversation
Operators currently have no way to know what a plugin does without reading its source or hunting through CLAUDE.md. Add a one-line Description field to PluginCapabilities and populate it on every registered plugin. This is the data that abctl's plugin-detail and catalog panes will surface in subsequent commits. The godoc on Description also documents the static-metadata constraint: Capabilities() must return the same value for any instance produced by a given factory, because the catalog endpoint walks factory()→Capabilities() to introspect plugins without holding live instances. No behavioral change: Description is purely additive metadata. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Surfaces the registered-plugin metadata that abctl needs to render its catalog browser pane and pre-apply validation. Calls each factory once with no config — Capabilities() on a freshly-built instance is the static type-level metadata. The framework's existing constructors are all cheap (allocate-only) so this is fine; the godoc on PluginCapabilities documents the constraint going forward. No new behavior in /v1/* yet — that wiring lands in the next commit. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Two complementary changes: 1. /v1/pipeline now carries Requires/RequiresAny/After/Claims/ Description on each plugin (omitempty). abctl will use these to render the plugin-detail pane and a "deps satisfied" indicator on the Pipeline pane in subsequent commits. 2. New GET /v1/plugins lists every registered plugin's metadata, not just the ones in the active pipeline. abctl will render this in a catalog browser pane so operators can see what's available before adding one. The endpoint is gated on a CatalogProvider (WithCatalog option); without it the endpoint 404s. The three binaries (proxy, envoy, lite) all wire sessionapi.PluginsCatalog which adapts plugins.Catalog(). The /v1/pipeline change also fixes a pre-existing wire issue: BodyAccess now reflects ReadsBody (post-Normalize) so plugins that declare ReadsBody-only correctly surface as having body access. Backward-compatible because every plugin that previously set BodyAccess still does. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Decode the Requires/RequiresAny/After/Claims/Description fields the server now emits on /v1/pipeline. Add PluginCatalog/PluginCatalogEntry types and a GetPluginCatalog method for the new /v1/plugins endpoint. Wire-format only — abctl's UI changes follow. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Two related abctl-side changes consuming the new wire fields:
Plugin-detail pane (showPluginDetail):
- Render Description as a hint line under the plugin name when
populated.
- New Requires / Requires any of / After (soft) / Claims sections.
When the plugin is part of an active chain, each Requires entry
gets a ✓/✗ indicator with the satisfying upstream's position;
✗ entries are flagged "NOT in this chain". After entries report
✓/✗ for misorder, marking absent ones as soft-OK. RequiresAny
shows per-alternative satisfaction so operators see which option
is currently doing the work.
Pipeline pane (rebuildPipelineTable):
- New DEPS column. ✓ when all declared dependencies are met, ✗ when
any fail, blank when the plugin declares no deps. Prevents a
"looks fine" misread on plugins with nothing to verify.
- Footer hint counts plugins with unmet deps so a single ✗ in a
long list doesn't get lost ("· 1 plugin with unmet deps").
The dependency-checking helpers (deps.go) are shared between the
two surfaces; deps_test.go covers Requires/RequiresAny/After
satisfaction logic plus the unmet-count walker.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
New paneCatalog renders /v1/plugins as a table with NAME / REQUIRES / DESCRIPTION columns. Pressing `P` from any session-view pane opens the catalog (lazy-loaded on first open, cached for the session; `r` refreshes from inside the pane). `Enter` on a row drills into the existing plugin-detail pane, reusing showPluginDetail with a synthetic PipelinePlugin built from the catalog entry — Direction and Position are blank, so showPluginDetail elides them gracefully. `Esc` from the catalog returns to whichever pane invoked it (previousPane is captured at P-press time). `Esc` from plugin-detail also routes back to the catalog when the detail was opened from there, so the back-out chain is symmetric. Operators can browse the registered plugin set without reading source — descriptions and dependency declarations come straight from the framework. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
After the user saves their pipeline edit, abctl now runs the same
Requires/RequiresAny/After/Claims/unknown-name checks the framework
runs at reload-time, BUT against the locally-cached plugin catalog —
result lands in ~50ms instead of waiting through kubelet sync (~60s)
to discover the issue.
ValidatePipeline parses the proposed YAML, walks each direction,
and surfaces structured errors. The diff overlay renders them as a
red banner above the diff, with one bullet per issue:
⚠ 1 validation issue — framework reload will reject:
• [outbound] ibac pos 1: Requires "mcp-parser", but it is not in the outbound chain
The y/N prompt becomes "apply anyway? (y/N)" so it's still possible
to push past the warning — abctl is the fast-feedback layer; the
framework's validateRelationships is the source of truth and will
fire again at reload regardless.
Validation is silently skipped when the catalog isn't loaded
(operator hasn't pressed P yet); a single visit to the Catalog pane
populates m.catalog for the rest of the session.
Tests: validate_test.go covers happy path, missing Requires,
misordered Requires, After misorder, Unknown plugin, Claims
conflict, and nil-catalog skip.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Cover the new abctl-side surfaces in cmd/abctl/README.md: - Add P/r keybind rows. - Update the Panes section to list Pipeline, Plugin detail, and Catalog (the new browser). - New "Pre-apply validation" subsection under "Editing the pipeline" documents the catalog-backed warnings that land before kubectl apply. - New top-level "Plugin dependencies" section names Requires/RequiresAny/After/Claims and lists the three places abctl surfaces them. Cover /v1/pipeline and /v1/plugins in authbridge/CLAUDE.md's session-API endpoint table — the previous version only listed the session-related endpoints. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
When a parser's OnResponse runs on a request it had previously parsed (Extensions.X populated) but the response body is empty — the typical case for JSON-RPC notifications that ack with HTTP 202 — the parser was returning Continue without recording any Invocation. abctl's events table then displayed the response row with `—` for plugin and action, and the events_pane.go pairing logic (which keys on plugin+method+direction) couldn't pair the request and response, leaving both rows orphaned and missing the ┌/└ tree glyphs. Fix: split the early-return guard. Stay silent only when the request side never participated (Extensions.X == nil). When the parser DID process the request but the response is empty, record a Skip with reason "no_response_body" so abctl can pair the rows. Applied to all three parsers (mcp, a2a, inference) — same pattern, same gap. Each gets a strengthened OnResponse_EmptyBody test asserting the new Skip Invocation contract. Triggers visibly on `notifications/initialized` (MCP) and `message/cancel` style fire-and-forget A2A calls. The skip rows are hidden by default in abctl (action=skip is muted unless `s` is toggled), but pairing now works regardless. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Pre-apply validation parsed only the user's edited subtree, so an indentation mismatch between the edit and the splice site produced a subtree that parses standalone yet breaks the combined inner YAML. The error wouldn't surface until the framework's hot-reload tried the spliced doc 60s later, then auto-rollback fired with a stale "mapping values are not allowed" error pointing at a line operators can't trace back to their edit. Splice the edit into the inner YAML and yaml.Unmarshal the result in the editorExitedMsg handler, before transitioning to the diff phase. Failures route to editPhaseError immediately with a hint that the pipeline: subtree must start at column 0. Regression test: TestEditFlow_PostSpliceYAMLError supplies an edit with leading whitespace on `pipeline:` — passes the standalone YAML check, fails the post-splice check. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Two events_pane rendering fixes the empty-body parser-skip work exposed: 1. Skip rows that pair with a non-skip request were getting hidden under the default skip-filter, leaving the request's ┌ glyph orphaned. The classic case: mcp-parser/observe on a notifications/initialized request paired with mcp-parser/skip/ no_response_body on the response — the response would vanish from the timeline, making a successful exchange read as a missing response. New rule: a skip stays visible when its pair partner is non-skip; only fully-skip pairs (both ends skip) get hidden. 2. Continuation rows (multiple invocations on one event) were rendering only the tree glyph in the PHASE column, blanking "req"/"resp". Operators scanning the column couldn't tell which side of the lifecycle each plugin's row belonged to. Render prefix + shortPhase always — the 6-char budget (outer + inner + "resp") still fits. Together: notifications/initialized exchanges now show both rows with ┌req/└resp glyphs, and a2a-parser on a multi-plugin inbound event correctly displays "req" alongside its tree glyph. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Inbound A2A request bucketing was: A2A.SessionID || ActiveSession() || DefaultSessionID. The middle fallback was a cross-conversation contamination vector — a new chat's first turn (empty contextId) silently inherited the previous chat's session bucket as long as that bucket was still inside its TTL. Symptom: every event of a new chat (inbound request + all outbound MCP/inference work the agent fired) landed in the prior chat's bucket. Only the response, by then carrying the agent-assigned contextId, created a new bucket — leaving the old bucket polluted and the new bucket as a 1-event orphan with just the response. Extract inboundSessionID() helper mirroring extproc's inboundSessionID(): trust the A2A-stated contextId when non-empty, otherwise route to DefaultSessionID. The rekey-on-response path already migrates Default → contextId once the agent reveals it, so the new chat lands cleanly in its own bucket. Applied to all three reverseproxy bucketing sites: inbound request recording, inbound response recording, recordInboundReject. New regression tests: - TestInboundSessionID_NoActiveSessionFallback (table-driven first turn vs subsequent turn) - TestInboundSessionID_NoA2AExtension (auth-only path) - TestRecordInbound_NewChatDoesntLeakIntoPriorBucket (integration) Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Five findings from the pre-PR review: 1. plugins.Catalog() now memoizes the result on first call. Bounds the constructor side-effect surface: even a misbehaving plugin that leaks a goroutine in its constructor leaks one process-wide rather than per /v1/plugins request. Tightens the constructor contract from "should be cheap" to "MUST NOT allocate goroutines / connections / file handles" with explicit godoc. Cache is reset by resetCatalogCache() for tests. 2. New TestCatalog_FactoryInvariant runs across every registered plugin: calls factory twice, asserts Capabilities() is byte-equal between the two instances. Locks the load-bearing claim that underlies the catalog cache (one snapshot describes every instance). 3. Validator's After semantics now match the framework's validateRelationships exactly (rp >= pos, not rp > pos). Previously abctl could pass YAML the framework would reject on duplicate plugin names. 4. CatalogEntry wire shape uses readsBody (modern) instead of bodyAccess (deprecated alias). New endpoint, no migration cost; the parallel pipelinePluginView keeps bodyAccess for compat with abctl versions shipped pre-catalog. 5. previousPane uses an explicit paneNone = -1 sentinel instead of relying on paneNamespaces being the zero value. Future-proofs against P opening the catalog from new entry points. token-broker description tweaked: was "Inbound token broker" but the plugin can be configured outbound too. Operator-facing labels should be direction-agnostic. Tests: full authlib + abctl suites green with -race. 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 (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a memoized plugin catalog and session API surface (/v1/plugins), expands plugin capability metadata, centralizes inbound session bucketing, implements pipeline pre-apply validation, and extends abctl with a Catalog pane, dependency evaluation, and related client/server tests and wiring. ChangesPlugin Dependency System & Catalog Surface
🎯 4 (Complex) | ⏱️ ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SessionAPI
participant PluginRegistry
Client->>SessionAPI: GET /v1/plugins
alt CatalogProvider set
SessionAPI->>PluginRegistry: Catalog()
PluginRegistry-->>SessionAPI: []CatalogEntry
SessionAPI-->>Client: 200 {plugins: [...]}
else
SessionAPI-->>Client: 404
end
Possibly Related PRs
Suggested reviewers
🚥 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: 5
🧹 Nitpick comments (2)
authbridge/cmd/abctl/edit/validate_test.go (1)
10-150: ⚡ Quick winAdd a direction-mismatch validation test case.
Current tests don’t exercise placing a cataloged inbound-only plugin in the outbound chain (or inverse), which is a useful guard for pre-apply validation behavior.
Proposed test sketch
func validateFixtureCatalog() []apiclient.PluginCatalogEntry { return []apiclient.PluginCatalogEntry{ - {Name: "jwt-validation", Description: "Inbound JWT"}, + {Name: "jwt-validation", Direction: "inbound", Description: "Inbound JWT"}, {Name: "a2a-parser", Description: "Parser"}, {Name: "mcp-parser", Description: "MCP parser"}, {Name: "ibac", Description: "IBAC", Requires: []string{"mcp-parser"}, After: []string{"a2a-parser"}}, {Name: "claim-a", Claims: []string{"authorization-header"}}, {Name: "claim-b", Claims: []string{"authorization-header"}}, } } + +func TestValidatePipeline_DirectionMismatch(t *testing.T) { + yaml := `pipeline: + outbound: + plugins: + - name: jwt-validation +` + errs := ValidatePipeline([]byte(yaml), validateFixtureCatalog()) + found := false + for _, e := range errs { + if strings.Contains(e.Message, "cannot run in outbound chain") { + found = true + } + } + if !found { + t.Fatalf("expected direction mismatch error; got %+v", errs) + } +}🤖 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/validate_test.go` around lines 10 - 150, Add a test that verifies ValidatePipeline rejects a plugin placed in the wrong direction: extend validateFixtureCatalog to include a plugin with an explicit direction (e.g., add a PluginCatalogEntry like {Name: "inbound-only", Description: "Inbound only", Direction: "inbound"}) and add a new test TestValidatePipeline_DirectionMismatch that places "inbound-only" under outbound in the YAML, calls ValidatePipeline([]byte(yaml), validateFixtureCatalog()), asserts an error is returned, and checks the error message mentions the direction mismatch (e.g., contains "inbound" or "Inbound-only") and references the PluginName "inbound-only". Ensure you use the existing ValidatePipeline and validateFixtureCatalog identifiers so the test lives alongside the other TestValidatePipeline_* cases.authbridge/cmd/abctl/apiclient/client_test.go (1)
150-180: ⚡ Quick winAdd an explicit 404 contract test for
GetPluginCatalog.
GetPluginCatalogdocuments gracefulErrNotFoundbehavior, but this path is currently untested.Proposed test
+func TestGetPluginCatalog_404(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/v1/plugins" { + t.Fatalf("wrong path: %q", r.URL.Path) + } + http.NotFound(w, r) + })) + defer ts.Close() + + c := New(ts.URL) + _, err := c.GetPluginCatalog(context.Background()) + if !errors.Is(err, ErrNotFound) { + t.Fatalf("expected ErrNotFound, got %v", err) + } +}🤖 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/apiclient/client_test.go` around lines 150 - 180, Add a test that verifies GetPluginCatalog returns the documented not-found error: create a new httptest.Server (or a sub-case in TestGetPluginCatalog) whose handler returns a 404 for "/v1/plugins" (with optional empty body), call c := New(ts.URL) and then c.GetPluginCatalog(context.Background()), and assert the returned error matches ErrNotFound (use errors.Is(err, ErrNotFound) if the code wraps errors). Name the test to indicate the 404 contract (e.g., TestGetPluginCatalog_NotFound) and keep the existing happy-path test unchanged.
🤖 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/authlib/plugins/registry.go`:
- Around line 124-129: Catalog() currently returns the shared cached slice
(catalogCacheVal) directly which allows callers to mutate the cache; change
Catalog() to return a defensive deep copy: after taking catalogCacheVal into a
local variable, allocate a new slice and copy each element, and for elements
that contain nested slices or mutable fields (e.g., the capabilities/capability
slices inside the Plugin metadata structs) copy those nested slices/structures
as well so the returned value is fully independent of catalogCacheVal; keep
using catalogCacheMu for locking but always return the deep-copied slice instead
of the original catalogCacheVal.
In `@authbridge/cmd/abctl/edit/validate.go`:
- Around line 96-106: The current lookup using byName[p.Name] only checks
existence and can accept plugins whose catalog direction doesn't match the
target; after locating entry := byName[p.Name], verify the catalog's allowed
direction(s) on entry (e.g. compare entry.Direction or entry.Directions) against
the current direction variable and, if they conflict, append a ValidationError
(same fields: Direction: direction, PluginName: p.Name, Position: pos) with a
message saying the plugin is not valid for that direction; keep the existing
continue behavior when reporting the error.
In `@authbridge/cmd/abctl/README.md`:
- Around line 147-150: The fenced code block in README.md (the block showing the
validation warning) lacks a language tag, causing markdownlint MD040 failures;
update the block surrounding the lines that start with "⚠ 1 validation issue —
framework reload will reject:" to use a language tag (e.g., replace ``` with
```text) so the fenced example is explicitly marked as plain text.
In `@authbridge/cmd/abctl/tui/app.go`:
- Around line 233-240: The session-scoped plugin catalog cache (m.catalog)
persists across pod switches and must be cleared when the connection context
changes; update the reconnection/teardown path (e.g., inside backToPodsPane() or
wherever the app resets connection state) to set m.catalog = nil so the next
time the catalog pane opens (catalog pane code that checks m.catalog) it will
fetch /v1/plugins anew for the current pod.
In `@authbridge/cmd/abctl/tui/deps.go`:
- Around line 55-71: The current requiresAnyOK implementation returns true if
any named dependency appears earlier, but we must instead ensure that every name
listed in p.RequiresAny that is present in the chain is positioned earlier than
p; change requiresAnyOK to iterate each name and: if the name is found in chain
and its q.Position >= p.Position, return false; after checking all names return
true (and still return true when none are present). Apply the same corrected
logic to the other occurrence referenced (the similar block at lines 116-134 /
the other helper used by pluginDepsAllSatisfied) so DEPS/footer and
pluginDepsAllSatisfied use the stricter validation semantics.
---
Nitpick comments:
In `@authbridge/cmd/abctl/apiclient/client_test.go`:
- Around line 150-180: Add a test that verifies GetPluginCatalog returns the
documented not-found error: create a new httptest.Server (or a sub-case in
TestGetPluginCatalog) whose handler returns a 404 for "/v1/plugins" (with
optional empty body), call c := New(ts.URL) and then
c.GetPluginCatalog(context.Background()), and assert the returned error matches
ErrNotFound (use errors.Is(err, ErrNotFound) if the code wraps errors). Name the
test to indicate the 404 contract (e.g., TestGetPluginCatalog_NotFound) and keep
the existing happy-path test unchanged.
In `@authbridge/cmd/abctl/edit/validate_test.go`:
- Around line 10-150: Add a test that verifies ValidatePipeline rejects a plugin
placed in the wrong direction: extend validateFixtureCatalog to include a plugin
with an explicit direction (e.g., add a PluginCatalogEntry like {Name:
"inbound-only", Description: "Inbound only", Direction: "inbound"}) and add a
new test TestValidatePipeline_DirectionMismatch that places "inbound-only" under
outbound in the YAML, calls ValidatePipeline([]byte(yaml),
validateFixtureCatalog()), asserts an error is returned, and checks the error
message mentions the direction mismatch (e.g., contains "inbound" or
"Inbound-only") and references the PluginName "inbound-only". Ensure you use the
existing ValidatePipeline and validateFixtureCatalog identifiers so the test
lives alongside the other TestValidatePipeline_* cases.
🪄 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: 640d0208-0abc-484e-9040-02797357d8ef
📒 Files selected for processing (40)
authbridge/CLAUDE.mdauthbridge/authlib/listener/reverseproxy/server.goauthbridge/authlib/listener/reverseproxy/server_test.goauthbridge/authlib/pipeline/plugin.goauthbridge/authlib/plugins/a2aparser/plugin.goauthbridge/authlib/plugins/a2aparser/plugin_test.goauthbridge/authlib/plugins/ibac/plugin.goauthbridge/authlib/plugins/inferenceparser/plugin.goauthbridge/authlib/plugins/inferenceparser/plugin_test.goauthbridge/authlib/plugins/jwtvalidation/plugin.goauthbridge/authlib/plugins/mcpparser/plugin.goauthbridge/authlib/plugins/mcpparser/plugin_test.goauthbridge/authlib/plugins/registry.goauthbridge/authlib/plugins/registry_test.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/README.mdauthbridge/cmd/abctl/apiclient/client.goauthbridge/cmd/abctl/apiclient/client_test.goauthbridge/cmd/abctl/edit/validate.goauthbridge/cmd/abctl/edit/validate_test.goauthbridge/cmd/abctl/tui/app.goauthbridge/cmd/abctl/tui/catalog_pane.goauthbridge/cmd/abctl/tui/catalog_pane_test.goauthbridge/cmd/abctl/tui/deps.goauthbridge/cmd/abctl/tui/deps_test.goauthbridge/cmd/abctl/tui/edit_e2e_test.goauthbridge/cmd/abctl/tui/edit_overlay.goauthbridge/cmd/abctl/tui/events_pane.goauthbridge/cmd/abctl/tui/keys.goauthbridge/cmd/abctl/tui/namespaces_pane.goauthbridge/cmd/abctl/tui/pipeline_pane.goauthbridge/cmd/abctl/tui/plugin_detail_pane.goauthbridge/cmd/abctl/tui/plugin_detail_pane_test.goauthbridge/cmd/authbridge-envoy/main.goauthbridge/cmd/authbridge-lite/main.goauthbridge/cmd/authbridge-proxy/main.go
Comment on lines
+96
to
+106
| entry, known := byName[p.Name] | ||
| if !known { | ||
| errs = append(errs, ValidationError{ | ||
| Direction: direction, | ||
| PluginName: p.Name, | ||
| Position: pos, | ||
| Message: fmt.Sprintf("Unknown plugin %q (not in /v1/plugins catalog)", | ||
| p.Name), | ||
| }) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Validate plugin direction against the target chain.
At Line 96, name-only catalog lookup can treat an inbound-only plugin as valid in outbound (and vice versa), so pre-apply validation can miss a real misconfiguration.
Proposed fix
entry, known := byName[p.Name]
if !known {
errs = append(errs, ValidationError{
Direction: direction,
PluginName: p.Name,
Position: pos,
Message: fmt.Sprintf("Unknown plugin %q (not in /v1/plugins catalog)",
p.Name),
})
continue
}
+ if entry.Direction != "" && entry.Direction != direction {
+ errs = append(errs, ValidationError{
+ Direction: direction,
+ PluginName: p.Name,
+ Position: pos,
+ Message: fmt.Sprintf("Plugin %q is %s-only, cannot run in %s chain",
+ p.Name, entry.Direction, direction),
+ })
+ continue
+ }🤖 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/validate.go` around lines 96 - 106, The current
lookup using byName[p.Name] only checks existence and can accept plugins whose
catalog direction doesn't match the target; after locating entry :=
byName[p.Name], verify the catalog's allowed direction(s) on entry (e.g. compare
entry.Direction or entry.Directions) against the current direction variable and,
if they conflict, append a ValidationError (same fields: Direction: direction,
PluginName: p.Name, Position: pos) with a message saying the plugin is not valid
for that direction; keep the existing continue behavior when reporting the
error.
Three suggestions worth addressing:
1. Eager Catalog() at boot. New plugins.WarmCatalog() helper is
called in each binary's main() right before sessionapi.New, so any
factory that violates the constructor contract (panics, allocates
goroutines/connections/file handles) fails fast at startup rather
than silently caching a faulty throwaway instance on the first
/v1/plugins request. Cheap insurance — the result is already
memoized so this just moves "first call" out of the request hot
path.
2. Stale-catalog hint on the editor's "Unknown plugin" validation
error. abctl caches /v1/plugins for the session; a freshly-
installed plugin in-cluster won't appear until refresh. The
message now hints at the staleness path:
Unknown plugin "foo" (not in cached /v1/plugins; catalog may
be stale, press P then r to refresh)
The prompt is already non-blocking ("apply anyway? (y/N)"), so
operators can still proceed; this just saves one round of
confusion.
3. Document the auth-posture decision on /v1/plugins. The handler
inherits the no-auth posture of the rest of /v1/*; the catalog
reveals plugin metadata (names, deps, descriptions) but never
user content or secrets, so it's fine. Doc-only addition so the
next maintainer doesn't re-derive the conclusion.
Reviewer's rossoctl#4 was a compliment on the inboundSessionID comment
block — no action needed.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Four findings worth addressing: 1. Catalog() returns a defensive deep copy, not the cached slice. Callers can mutate the returned []CatalogEntry (sort/filter) and the nested capability []string fields without tainting the cache or polluting future /v1/plugins responses. New TestCatalog_ReturnsDefensiveCopy locks the contract: mutate first call's result, second call still sees originals. 2. RequiresAny semantics in deps.go now match the framework's validateRelationships exactly. Was: at-least-one-upstream wins. Now: at-least-one-upstream AND every present alternative must be earlier (downstream presence is a misorder, even if another alternative is upstream). Earlier the Pipeline pane DEPS column could show ✓ for chains the framework would reject. New TestPluginDepsAllSatisfied_RequiresAnyMisorderOnAlternate locks it. 3. backToPodsPane() now drops m.catalog (and the catalogTbl rows). Different pod = different framework instance = potentially different plugin versions registered. Without this the next P would show the prior pod's catalog with no refresh prompt. 4. README.md fenced code block gets a `text` language tag (markdownlint MD040). Skipped CodeRabbit's "validate plugin direction against the target chain" suggestion: the catalog deliberately doesn't populate Direction (see catalog_adapter.go) — plugins are direction-agnostic at the type level, with positional placement decided by config. The framework's own validateRelationships doesn't enforce a per- plugin direction either, so the suggestion would diverge from the source-of-truth validation it's supposed to mirror. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
cwiklik
approved these changes
May 29, 2026
cwiklik
left a comment
cwiklik
left a comment
Collaborator
There was a problem hiding this comment.
Large but well-structured PR: adds plugin catalog endpoint (GET /v1/plugins) with proper gating via WithCatalog option, fixes a cross-chat session leak (all 3 call sites updated + test locks the invariant), and extends the abctl TUI with catalog browsing and pre-apply validation. Strong test coverage across all new features. No dependency changes, no security concerns.
Areas reviewed: Go (framework, API, CLI, tests), Docs, Security
Commits: 15 commits, all signed-off: yes
CI status: all checks passing
Two minor suggestions:
-
reverseproxy/server.go—inboundSessionID: The comment says it "Mirrors extproc's inboundSessionID." Consider extracting to a shared package (e.g.,session.InboundID(pctx)) to prevent future drift between the two implementations. -
sessionapi/server.go—handlePluginCatalog: JSON encoding errors are logged atslog.Debuglevel. Since this is an API endpoint and the client receives a truncated/empty response on failure,slog.Warnmight be more appropriate for operational visibility.
github-project-automation
Bot
moved this from New /:ToDo
to Done
in Kagenti Issue Prioritization
This was referenced Jun 1, 2026
9 tasks
Merged
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 a plugin catalog to authbridge + abctl, giving operators visibility into the registered-plugin set without reading source. Plus several follow-on fixes that surfaced during testing of the new editor in #448.
Backend (framework + wire)
pipeline.PluginCapabilitiesgainsDescription— a one-line operator-facing label, populated on every registered plugin (jwt-validation, token-exchange, a2a-parser, mcp-parser, inference-parser, ibac, token-broker).plugins.Catalog()— new accessor returning[{Name, Capabilities}]for every registered plugin. Memoized on first call; constructor contract tightened to "MUST NOT allocate goroutines/connections" with a factory-invariant test that locks the load-bearing claim./v1/pipelinecarries the full metadata —Requires/RequiresAny/After/Claims/Description(allomitempty).GET /v1/plugins— registered-plugin catalog endpoint; gated onWithCatalogOption. Three binaries (proxy, envoy, lite) wiresessionapi.PluginsCatalogidentically.abctl UI
Pkey opens the plugin catalog browser pane — sortable table of every registered plugin with description + dependency declarations. Lazy-fetched, cached for the session,rrefreshes.DEPScolumn — ✓/✗/blank per plugin row based on whetherRequires/RequiresAny/Afterare satisfied. Footer hint counts plugins with unmet deps.Requires/RequiresAny/After/Claims/unknown-namechecks against the cached catalog before showing the diff. Issues render as a red banner above the diff in ~50ms instead of waiting through kubelet sync (~60s) for the framework's reload error. Non-blocking — the prompt becomes "apply anyway? (y/N)".Follow-on fixes that surfaced during testing
notifications/initialized) that ack with HTTP 202: parsers now recordSkip("no_response_body")so abctl's pair-by-(plugin, method, direction) keying works.┌glyphs); continuation rows always render the phase text alongside the tree glyph.reverseproxycross-conversation session leak — inbound A2A request bucketing wasA2A.SessionID || ActiveSession() || Default. The middle fallback silently routed a new chat's first turn into a previous chat's still-alive bucket. Fix mirrors whatextprocalready does: drop theActiveSession()fallback; route toDefaultSessionIDand rely on the existing rekey-on-response.Test Plan
go test -race ./...acrossauthlib(all packages) andcmd/abctl(all packages) — all greenTestCatalog_FactoryInvariantruns across all 6 registered plugins — locks theCapabilities()consistency contractTestInboundSessionID_NoActiveSessionFallback+TestRecordInbound_NewChatDoesntLeakIntoPriorBucket— regression coverage for the session-leak fixTestEditFlow_PostSpliceYAMLError— locks the splice-context YAML validationTestEditFlow_PairedSkipVisibility— pair-aware skip filtering covered via existing tests intui/OnResponse_EmptyBodynow assert theSkip("no_response_body")invocationTestHandlePluginCatalog_*— new endpoint covered (lists + 404 when no provider)TestValidatePipeline_*— happy path, missing Requires, misordered Requires, After misorder, Unknown plugin, Claims conflict, nil-catalog skip/v1/pluginsreachable, DEPS column shows ✓ for ibac (mcp-parser upstream), Catalog pane renders 6 plugins, pre-apply validation catchesbogus-plugintypo, two consecutive chats no longer cross-contaminate session buckets, paired req/resp visible fornotifications/initializedPre-PR review
A whole-branch reviewer (Opus) flagged 10 findings; 3 IMPORTANT and 3 NITs addressed in commit
2266bbe:TestCatalog_FactoryInvariantadded to lock the consistency claimAftersemantics now match framework'svalidateRelationshipsexactlyreadsBody(modern) notbodyAccess(deprecated)previousPaneuses explicitpaneNonesentineltokenbrokerdescription rewritten to be direction-agnosticAssisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
New Features
Improvements