Feat: Declare plugin pipeline directions and complete ConfigSchema coverage - #847
Feat: Declare plugin pipeline directions and complete ConfigSchema coverage#847esnible wants to merge 1 commit into
Conversation
…verage Make plugin placement and config metadata machine-readable, so config generators no longer have to infer either from source or from a hand-maintained table. Directions (new PluginCapabilities field): Every plugin has an intended chain -- jwt-validation is inbound, token-exchange outbound, opa both -- but that lived only in the Direction column of docs/plugin-catalog.md. All 14 in-tree plugins now declare it as Capabilities().Directions, published on /v1/plugins and /v1/pipeline as `directions`, and asserted by a test so the docs table has a source of truth. Advisory, never fatal: no plugin enforces direction at runtime (opa, the one that cares, merely branches on pctx.Direction), so a misplaced plugin is a probable misconfiguration rather than a guaranteed one, and failing the boot would break configs that work today. A mismatch logs a startup WARN (plugins.WarnPluginDirections, called beside the existing WarnEmptyPipelines) and shows an advisory in abctl. Nil Directions means unconstrained, so out-of-tree plugins are unaffected. The wire field is plural and string-typed on purpose. `direction` (singular) already means "the chain this configured instance sits in" and stays untouched; `directions` is the type-level set of chains a plugin supports. Strings rather than []Direction because Direction.UnmarshalJSON decodes any unknown value to Inbound without erroring, which on a slice would turn a future third value into a false "inbound" claim. ConfigSchema: mcp-parser, opa, session-budget and litellm-budget-track were Configurable but not SchemaProvider, so /v1/plugins reported no field metadata and abctl rendered them as bare names. All four now implement it; mcp-parser and opa also gained the field annotations (defaults and required flags taken from their applyDefaults/Configure, not guessed). a2a-parser and inference-parser are deliberately excluded -- they have no config at all, and pipeline/schema.go names them as legitimate omissions. Also adds a "number" schema type for floats. litellm-budget-track is the first plugin to expose float config; without it max_budget and the five per-token rates would publish as "unknown" and render as quoted empty strings in templates. abctl: Templates carry a `# chain:` line per plugin, and the pre-apply validator flags a plugin pasted into a chain it doesn't declare. ValidationError gains a Severity so advisories render under their own banner -- folding them into the existing one would make its "framework reload will reject" claim false. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (39)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughPlugins now declare supported pipeline directions and configuration schemas. Session APIs and abctl expose this metadata. Runtime builds log placement warnings, while abctl reports direction mismatches as advisories. ChangesPipeline capability and schema contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR adds machine-readable plugin directions and configuration schemas while preserving existing execution behavior; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant abctl
participant SessionAPI
participant PluginCatalog
participant PipelineValidator
participant PipelineBuild
abctl->>SessionAPI: request plugin and pipeline metadata
SessionAPI->>PluginCatalog: read normalized directions and schemas
SessionAPI-->>abctl: return directions and field schemas
abctl->>PipelineValidator: validate configured chain
PipelineValidator-->>abctl: return errors and direction advisories
PipelineBuild->>PluginCatalog: read plugin capabilities
PipelineBuild-->>PipelineBuild: log mismatches without blocking build
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 81.13% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 53 functions across 36 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)level=error msg="[linters_context] typechecking error: build constraints exclude all Go files in /authbridge/cmd/authbridge-cpex" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
The design here is careful and the parts most likely to be wrong hold up — I checked them rather than taking the body's word for it. The block is not about the feature; it is that the branch cannot compile against main, and the conflict sits in exactly the two functions this PR modifies, so the rebase needs a deliberate resolution rather than a mechanical one.
State: 1 commit ahead, 34 behind, mergeable: false, rebaseable: false. And CI never ran on this SHA — only DCO and add-to-project executed (plus CodeRabbit). There is no Go build or test job in the check list, so none of the new tests have been run by CI, and the body's "go build, go vet, go test clean across all five modules" is a local claim only. That is a consequence of the conflict and should clear itself on rebase, but it means the three must-fixes below were all found by reading rather than by a red build.
Verified rather than assumed:
litellm-budget-track: Inboundis correct, which surprised me — it parses an LLM response, andinference-parsernext to it is Outbound.docs/litellm-budgettrack-plugin.mdsettles it: the design doc says "inbound pipeline" throughout and its example config ispipeline:\n inbound:, because AuthBridge fronts the LiteLLM gateway there rather than sitting beside the caller. Worth a half-sentence in the catalog row, since the pairing reads like a mistake to anyone who does not know that topology.- All 14 declarations match
docs/plugin-catalog.md's Direction column exactly, includingopa→Both→{Inbound, Outbound}. - Every
ConfigSchemaannotation matches the code. I checked each annotated field against the realapplyDefaults/validate:opa'sbundle_urlrequired is genuinely enforced (validate()errors, called fromConfigure),agent_id_file's default really is/shared/client-id.txt,polling_min_delay/polling_max_delayreally are 10/120, andmcp-parser'spathsdefault really is["/mcp"]. The field counts the tests assert (1 / 6 / 6) are right, and all four delegate topipeline.SchemaOf, so drift is structurally impossible. The "taken fromapplyDefaults/Configurerather than guessed" claim is accurate. - The WARN is wired into all three binaries that build pipelines (
-proxy,-envoy,-cpex), each placed besideWarnEmptyPipelinesand beforeBuild, as its godoc specifies.authbridge-praxisis correctly excluded — it never callsplugins.Buildand does not callWarnEmptyPipelineseither. - Both endpoints normalize.
describePipelineusespl.Capabilities().Normalize()andPluginsCataloglikewise, so/v1/pipelineand/v1/pluginscannot disagree on ordering — which is the thingcanonicalDirectionsexists to guarantee. Severity's zero value is safe.SeverityError = iotameans all six pre-existingValidationErrorsites keep blocking semantics; only the new direction check setsSeverityWarning. The banner split is complete and the y/N prompt correctly stays "apply this change?" when only advisories are present.- The
pipelinetests are the good kind.TestNormalizeDoesNotMutateInputguards a real aliasing hazard, and thet.Skipguards inplugins/do not actually fire —plugins_test.goblank-imports seven real plugins into the same test binary, soTestRegisteredPluginsDeclareDirectionsandTestCatalogCloneIsolatesDirectionsrun against real data. The mutation-testing story in the body checks out: the clone test nowt.Fatals instead of skipping.
Suggestions below are accuracy and coverage, not design. Nothing there blocks.
Author: esnible (MEMBER — maintainer)
Areas reviewed: Go (pipeline, plugins, sessionapi, abctl), tests, docs, security. 1 commit, signed off, no Co-Authored-By. CI: only DCO + add-to-project ran; no build/test job. No .claude/ or .vscode/ changes.
Assisted-By: Claude Code
| if c.WritesBody { | ||
| c.ReadsBody = true | ||
| } | ||
| c.Directions = canonicalDirections(c.Directions) |
There was a problem hiding this comment.
must-fix — this function will not compile against main. WritesBody no longer exists there: 6ca3fbb ("Rename WritesBody to WritesRequestBody") and 6440927 ("Split body-write capability by direction") both landed 2026-09-02, the day this PR was opened, and grep -c WritesBody on main's plugin.go is now 0.
main's version reads:
func (c PluginCapabilities) Normalize() PluginCapabilities {
if c.WritesRequestBody || c.WritesResponseBody {
c.ReadsBody = true
}
return c
}So the resolution is to keep main's condition and add your one line to it:
if c.WritesRequestBody || c.WritesResponseBody {
c.ReadsBody = true
}
c.Directions = canonicalDirections(c.Directions)
return cThe Directions field itself, canonicalDirections, and Supports all apply cleanly — the collision is only with the WritesBody line and the doc comment above it (which also names WritesBody, at :96). Flagging it explicitly because a rebase that resolves this hunk by taking your side compiles nowhere, and one that takes main's side silently drops the canonicalDirections call — losing the de-duplication and sort that the rest of the change depends on.
| Description: caps.Description, | ||
| Requires: append([]string(nil), caps.Requires...), | ||
| RequiresAny: append([]string(nil), caps.RequiresAny...), | ||
| Directions: append([]pipeline.Direction(nil), caps.Directions...), |
There was a problem hiding this comment.
must-fix — this is the more dangerous half of the conflict, because the naive resolution compiles and is wrong.
main has already replaced this hand-enumerated form with a struct copy, for precisely the reason your TestCatalogCloneIsolatesDirections docstring gives:
// Struct copy, then reallocate the slices. Copying field-by-field
// silently drops any capability added later; this picks them up.
caps := in[i].Capabilities
caps.Requires = append([]string(nil), in[i].Capabilities.Requires...)
caps.RequiresAny = append([]string(nil), in[i].Capabilities.RequiresAny...)Taking this PR's side of that conflict reverts that fix and drops two fields that now exist: WritesRequestBody and WritesResponseBody would be absent from the literal, so every Catalog() clone — and therefore every /v1/plugins response — would report them as false. tool-prune sets WritesRequestBody: true, so this would be observable immediately, and silently: no test on main covers those two fields surviving a clone.
Correct resolution — keep main's struct copy, add one line:
caps.Directions = append([]pipeline.Direction(nil), in[i].Capabilities.Directions...)Worth noting your clone test stays valuable after that change rather than becoming redundant: a struct copy duplicates the slice header but shares the backing array, so the explicit realloc is still what makes the mutation in TestCatalogCloneIsolatesDirections fail to reach the cache. The test guards the line above, not the struct copy.
| t.Fatal("catalog is empty; plugin registration is broken") | ||
| } | ||
| for _, e := range cat { | ||
| if len(e.Directions) == 0 { |
There was a problem hiding this comment.
must-fix — this test will fail after the rebase, on a plugin that does not exist on your branch yet.
tool-prune landed on main after the branch point. It is default-on (cmd/authbridge-proxy/plugins_toolprune.go is //go:build !exclude_plugin_toolprune), so it is in PluginsCatalog() for the default build, and it declares no Directions — meaning this loop hits t.Errorf("plugin %q publishes no directions"). TestRegisteredPluginsDeclareDirections in authlib/plugins would fail on it too if it were linked there.
That is your test working as designed, and it is the right failure to have. The fix is one line in toolprune/plugin.go's Capabilities():
Directions: []pipeline.Direction{pipeline.Outbound},Outbound is well-supported: docs/plugin-catalog.md on main already documents the row as Outbound, the package doc says "outbound inference requests", and its RequiresAny: []string{"inference-parser"} points at an outbound-only plugin, so any other value would make the dependency unsatisfiable in practice.
That is the only new plugin to account for — I diffed the catalog tables and main has 15 rows to this branch's 14, differing by tool-prune alone.
| callee); "both" means the plugin evaluates on both pipelines. This column | ||
| is **declared in code** as `Capabilities().Directions` (see | ||
| `authlib/pipeline/plugin.go`), published on `/v1/plugins` as | ||
| `directions`, and asserted by a test — so the table below has a source of |
There was a problem hiding this comment.
suggestion — "asserted by a test — so the table below has a source of truth rather than drifting" claims more than the tests deliver. No Go test reads this file; the only reference to it anywhere in the test code is inside an error message string.
What the new tests actually assert is that every registered plugin declares something non-empty (TestRegisteredPluginsDeclareDirections, TestShippedCatalogPublishesDirections) and that the published values are "inbound"/"outbound". Neither compares against this column. So flipping a plugin's Directions to the opposite chain leaves every test green while this table says the reverse — which is the exact drift the sentence promises is now impossible.
Two honest options: parse the table in a test and compare (it is a stable pipe-delimited grid, and Both → {Inbound, Outbound} is the only mapping needed), or drop "and asserted by a test" and "rather than drifting" and keep the accurate half — declared in code, published on /v1/plugins. The first is what makes the Direction column genuinely authoritative; the second at least stops over-promising.
| } | ||
| } | ||
|
|
||
| // litellm-budget-track is the first plugin to expose float config, which |
There was a problem hiding this comment.
suggestion — "litellm-budget-track is the first plugin to expose float config" is not right, and the same claim is in the PR body ("litellm-budget-track is the first plugin with float config").
sparc got there first: sparc/plugin.go:94 is DenyScoreThreshold float64, and sparc has implemented ConfigSchema() since well before this PR (:220, untouched here). So sparc has been publishing a float field all along — as "unknown", via kindOf's default branch.
The consequence is worth a line in the PR description rather than a code change: this PR silently changes an existing plugin's published schema, flipping deny_score_threshold from "unknown" to "number" on /v1/plugins. That is the improvement working as intended, and I confirmed it is harmless downstream — placeholderFor is the only consumer that switches on Type, this PR teaches it "number", and deny_score_threshold's default:"0" short-circuits the placeholder path anyway. But it is an unremarked behaviour change to a plugin the PR never mentions, and no test pins it. A sparc case in TestSchemaOf_Floats' orbit — or just one sentence in the description — would cover it.
| }{ | ||
| "mcp-parser": {1, nil}, | ||
| "opa": {6, []string{"bundle_url"}}, | ||
| "session-budget": {18, []string{"redis_url"}}, |
There was a problem hiding this comment.
suggestion — this expectation can never be exercised, and the comment in catalog_test.go defers to it, so session-budget's new ConfigSchema() ends up with zero coverage between them.
session-budget is registered only from cmd/authbridge-{proxy,envoy}/plugins_sessionbudget.go, under //go:build include_plugin_sessionbudget. No build of the authlib/plugins test binary reaches that registration, so Catalog() here never contains session-budget and this row is simply never matched — the test counts what it finds and passes. Meanwhile catalog_test.go:50-53 excuses its own omission with "covered by authlib/plugins' TestConfigSchemaShapes when linked." Each points at the other; neither runs it.
Cheapest fix is to test ConfigSchema() directly rather than through the registry — pipeline.SchemaOf(sessionBudgetConfig{}) from inside the sessionbudget package, where the build tag is not in the way. Failing that, drop the row and say plainly that it is uncovered, so the next reader does not trust a guarantee that is not there.
Related, :55: the docstring says these plugins "decode with DisallowUnknownFields", which is true for opa and mcp-parser but not for session-budget or litellm-budget-track — both use plain json.Unmarshal. For those two, TestConfigSchemaFieldNamesDecode would still pass with a wrong json name in the schema, since nothing rejects the unknown key. Worth narrowing the claim to the two plugins it holds for.
| // and pick value placeholders. One of: | ||
| // "string", "int", "bool", "[]string", "object", "unknown". | ||
| // "string", "int", "number", "bool", "[]string", "object", "unknown". | ||
| // "number" is a float (per-token costs, budgets); "int" stays |
There was a problem hiding this comment.
suggestion — the stated rationale is contradicted by the only consumer, inside this same PR. "int" is said to stay separate "so a template can emit \"0\" vs \"0.0\" appropriately", but templates.go's placeholderFor returns "0" for both, and its new case "number" explains why that is the right call: "0 is valid YAML for a float, and avoids implying a fixed precision."
I agree with the code, not the comment — the two case bodies are now byte-identical, and no template emits 0.0. Keeping "number" distinct is still worth doing, just for a different reason: it is honest type metadata for consumers that are not this template renderer (a JSON-Schema emitter, a form generator, a validator that needs to know a value may be fractional). Restating it that way would leave the comment true, and would stop the next person collapsing the two cases on the grounds that the documented distinction is unused.
| if e.Name == name { | ||
| p := apiclient.PipelinePlugin{ | ||
| Name: e.Name, | ||
| Directions: e.Directions, |
There was a problem hiding this comment.
suggestion — this is currently a dead write, and the comment above promises otherwise ("it is exactly the metadata a catalog browser wants").
showPluginDetail's renderer reads only the singular field — plugin_detail_pane.go:30-31 is if p.Direction != "" { ... "Direction:" ... } — and nothing anywhere reads p.Directions. So the value is copied onto the synthetic PipelinePlugin and then never displayed, which means the catalog pane still cannot tell an operator which chain a plugin belongs in, even though the data is now sitting right there.
Given the feature's whole premise is making placement visible, rendering it is the better resolution — a Chains: line beside Direction:, elided when empty, mirrors how the existing field degrades. Two lines in the detail pane. Otherwise drop the copy and the sentence, so the next reader is not misled into thinking the pane already surfaces it.
| } | ||
| if len(vWarns) > 0 { | ||
| b.WriteString(styleWarn.Render(fmt.Sprintf( | ||
| "%d advisory — reload will accept, but check:", |
There was a problem hiding this comment.
nit — missing plural(), so two advisories render as "2 advisory". The error branch four lines up gets this right ("%d validation issue%s" with plural(len(vErrs))), and the helper is already imported in this file, so it is "%d advisor%s" / plural — or "%d advisories"-style wording if you prefer, since plural returns "s" and "advisorys" is not a word.
README.md:165 shows the singular case, which is why it reads fine there.
While in this function: splitBySeverity and writeValidationLines have no test coverage at all — tui/edit_overlay_test.go is not in this PR and has never had a validationErrs case. The severity classification is well covered in edit/validate_test.go (SeveritiesCoexist is a good test), but the rendering split — the part that keeps the "reload will reject" banner truthful — is untested, and a mixed error+advisory set is exactly the case worth pinning.
3 participants
Makes plugin placement and config field metadata machine-readable, so config generators don't have to infer either from source.
DirectionsNew
PluginCapabilities.Directionsfield. All 14 in-tree plugins declare their intended chain, published on/v1/pluginsand/v1/pipelineasdirections, and asserted by a test — so the Direction column indocs/plugin-catalog.mdnow has a source of truth instead of drifting.Advisory, never fatal. No plugin enforces direction at runtime (
opa, the one that cares, only branches onpctx.Direction), so a misplaced plugin is a probable misconfiguration rather than a guaranteed one, and failing the boot would break configs that work today. A mismatch logs a startup WARN (plugins.WarnPluginDirections, called beside the existingWarnEmptyPipelines) and shows an advisory in abctl. NilDirectionsmeans unconstrained, so out-of-tree plugins are unaffected.Two naming/typing decisions:
direction(singular) already means "the chain this configured instance sits in" and is untouched;directionsis the type-level set a plugin supports.[]string, not[]Direction, becauseDirection.UnmarshalJSONdecodes any unknown value toInboundwithout erroring — fine for a single value, but on a slice that turns a future third direction into a false "inbound" claim.ConfigSchemamcp-parser,opa,session-budgetandlitellm-budget-trackwereConfigurablebut notSchemaProvider, so/v1/pluginsreported no field metadata and abctl rendered them as bare names. All four now implement it;mcp-parserandopaalso gained field annotations, with defaults and required flags taken from theirapplyDefaults/Configurerather than guessed.a2a-parserandinference-parserare excluded — they have no config at all, andpipeline/schema.gonames them as legitimate omissions. (The original ask said six plugins; four is the real count, and two of those were already annotated.)Also adds a
"number"schema type for floats:litellm-budget-trackis the first plugin with float config, so without itmax_budgetand five per-token rates would publish as"unknown"and render as quoted empty strings in templates.abctl
Templates carry a
# chain:line per plugin, and the pre-apply validator flags a plugin pasted into a chain it doesn't declare.ValidationErrorgains aSeverityso advisories render under their own banner — folding them into the existing one would make its "framework reload will reject" claim false.Testing
go build,go vet,go testclean across all five modules (cpex type-checked with-tags cpex).I verified the two highest-risk new tests actually fail when the code is broken. The clone-isolation test initially passed with
Directionsremoved fromcloneCatalog— it hit its ownt.Skip, which is exactly the silent-drop bug it exists to catch; it now fails and names the culprit.End-to-end: the WARN fires with plugin, both directions and position, and the pipeline still builds;
/v1/pluginsreportsdirectionsfor all 11 default-build plugins andnumberfor the six float fields.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
New Features
0for numeric fields.Bug Fixes
Documentation