feat: public extension API and embeddable bootstrap#479
Conversation
Adds the ext package — a registry where custom gateway builds register raw-JSON request rewriters, Echo middleware, extra routes, and auth-skip paths — and wires it into the server: rewriters run on POST /v1/chat/completions, /v1/messages, and /v1/responses after authentication and before workflow resolution, fail-closed, with audit logs keeping the original client body. Nothing is registered when no extensions exist, so the default build pays nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Moves CLI parsing, logging setup, health/ready probes, provider registration, and the start/shutdown lifecycle into the importable run package: run.Run(ctx, run.Options) with a Setup hook that fires only when the gateway actually serves (keeps --version and healthcheck probes silent) and run.ExitCode for exit-code mapping. cmd/gomodel becomes a thin wrapper with identical CLI behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR moves gateway startup into ChangesRun package extraction and CLI lifecycle
Extension system, request rewriting, and audit UI
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Main as cmd/gomodel/main.go
participant Run as run.Run
participant Config as Config Loader
participant Factory as defaultProviderFactory
participant App as Application
Main->>Run: Run(ctx, Options)
Run->>Run: parseCLI / handle --version / --help
Run->>Config: load config
Run->>Factory: defaultProviderFactory(cfg)
Run->>App: app.New(cfg, factory, Extensions)
Run->>App: Start()
Run->>App: Shutdown() on SIGINT / SIGTERM
Run-->>Main: error
Main->>Main: ExitCode(err)
sequenceDiagram
participant Client
participant AuthMW as Auth Middleware
participant RewriteMW as RequestRewriteMiddleware
participant Rewriter as ext.RequestRewriter
participant Audit as AuditLog
participant Provider
Client->>AuthMW: POST /v1/chat/completions
AuthMW->>RewriteMW: authenticated request
RewriteMW->>RewriteMW: check rewriteEndpoint
RewriteMW->>Rewriter: Rewrite(ctx, Input)
Rewriter-->>RewriteMW: Result{Body, ResponseHeader, Detail}
RewriteMW->>Audit: pinOriginalAuditRequestBody + recordRequestRevision
RewriteMW->>RewriteMW: applyRewrittenBody
RewriteMW->>Provider: forward rewritten request
Provider-->>Client: response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/extensions.md`:
- Around line 8-15: The markdown in extensions.md triggers MD028 because there
is a blank line inside a blockquote, causing two quoted sections to merge.
Update the blockquote around the GoModel module path note and the API stability
note so they are separated without an empty quoted line, keeping the content in
distinct quote blocks. Use the existing quoted paragraphs in the docs section as
the reference points.
In `@ext/ext.go`:
- Around line 27-55: Redact credential-bearing headers before rewriters see them
and before any revision details are persisted; the current flow passes the full
cloned header map through Input.Header and then stores Result.Detail via
recordRequestRevision without sanitization. Update the request rewrite path so
Input.Header excludes or masks sensitive values like Authorization, Cookie,
Proxy-Authorization, and X-Api-Key, and ensure any Detail produced by a rewriter
is filtered before it is recorded. Keep the fix centered around Input, Result,
and recordRequestRevision so the audit trail never captures secrets.
In `@internal/app/app.go`:
- Around line 439-444: Add app-level test coverage for the cfg.Extensions wiring
in app.New: create a test in internal/app/app_test.go that passes a non-nil
Extensions registry and verifies the resulting serverCfg gets RequestRewriters,
ExtraMiddleware, ExtraRoutes, and ExtraAuthSkipPaths populated from
cfg.Extensions. Use the app.New entrypoint and the Extensions-related methods
Rewriters, Middleware, Routes, and PublicPaths to exercise the translation path
that server.Config tests do not cover.
In `@internal/server/request_rewrite.go`:
- Around line 26-37: RequestRewriteMiddleware always buffers the request body
even when there are no rewriters, which adds unnecessary overhead on the ingest
path. Add an early exit in RequestRewriteMiddleware, before calling
requestBodyBytes(c), so that when the rewriters slice is empty the middleware
immediately delegates to next(c) after rewriteEndpoint(c.Request()) succeeds;
keep the existing rewrite/audit flow only for the non-empty rewriters case.
In `@run/providers.go`:
- Around line 30-58: Add test coverage for defaultProviderFactory to prevent
regressions when provider registrations change. Create tests around
defaultProviderFactory that verify the cfg.Metrics.Enabled branch sets
observability.NewPrometheusHooks on the ProviderFactory and that, with metrics
disabled, no hooks are added. Also assert that all expected provider
registrations are present after the calls to factory.Add in
defaultProviderFactory, using provider type names or factory inspection helpers
so the tests remain stable if line numbers change.
In `@run/run_test.go`:
- Around line 1-61: Add Run()-level test coverage for the --health and --ready
dispatch paths in Run, since the existing tests only cover --version, usage, and
help. Extend this test file to exercise the cliOpts.Health and cliOpts.Ready
branches in run.go, asserting the correct short-circuit behavior and that errors
from the underlying probe/ready handling are surfaced through Run. Use the Run
and ExitCode symbols to locate the dispatch logic and verify the flag routing
end-to-end.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d096a235-d027-4cef-8e1c-0b955f8baa39
📒 Files selected for processing (25)
cmd/gomodel/main.godocs/extensions.mdext/ext.goext/registry.goext/registry_test.gointernal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/audit-list.test.cjsinternal/admin/dashboard/templates/audit-pane.htmlinternal/app/app.gointernal/auditlog/auditlog.gointernal/auditlog/middleware.gointernal/server/http.gointernal/server/request_rewrite.gointernal/server/request_rewrite_test.gorun/flags.gorun/flags_test.gorun/health.gorun/health_test.gorun/lifecycle_test.gorun/logging.gorun/logging_test.gorun/providers.gorun/ready_test.gorun/run.gorun/run_test.go
| > **Consuming from another module:** GoModel's module path is `gomodel`, | ||
| > which Go cannot fetch remotely, so external builds currently use a local | ||
| > checkout with a `replace gomodel => ../GoModel` directive (or vendoring). | ||
| > Renaming the module to a canonical fetchable path is proposed separately | ||
| > in [#457](https://github.com/ENTERPILOT/GoModel/pull/457). | ||
|
|
||
| > **API stability:** the `ext` and `run` packages are experimental — no | ||
| > compatibility promise until they are declared stable. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix MD028 lint warning: blank line inside blockquote.
markdownlint flags the blank line between the two > blockquotes; some renderers will merge them into one quote.
📝 Proposed fix
> **Consuming from another module:** GoModel's module path is `gomodel`,
> which Go cannot fetch remotely, so external builds currently use a local
> checkout with a `replace gomodel => ../GoModel` directive (or vendoring).
> Renaming the module to a canonical fetchable path is proposed separately
> in [`#457`](https://github.com/ENTERPILOT/GoModel/pull/457).
+<!-- -->
+
> **API stability:** the `ext` and `run` packages are experimental — no
> compatibility promise until they are declared stable.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > **Consuming from another module:** GoModel's module path is `gomodel`, | |
| > which Go cannot fetch remotely, so external builds currently use a local | |
| > checkout with a `replace gomodel => ../GoModel` directive (or vendoring). | |
| > Renaming the module to a canonical fetchable path is proposed separately | |
| > in [#457](https://github.com/ENTERPILOT/GoModel/pull/457). | |
| > **API stability:** the `ext` and `run` packages are experimental — no | |
| > compatibility promise until they are declared stable. | |
| > **Consuming from another module:** GoModel's module path is `gomodel`, | |
| > which Go cannot fetch remotely, so external builds currently use a local | |
| > checkout with a `replace gomodel => ../GoModel` directive (or vendoring). | |
| > Renaming the module to a canonical fetchable path is proposed separately | |
| > in [`#457`](https://github.com/ENTERPILOT/GoModel/pull/457). | |
| <!-- --> | |
| > **API stability:** the `ext` and `run` packages are experimental — no | |
| > compatibility promise until they are declared stable. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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 `@docs/extensions.md` around lines 8 - 15, The markdown in extensions.md
triggers MD028 because there is a blank line inside a blockquote, causing two
quoted sections to merge. Update the blockquote around the GoModel module path
note and the API stability note so they are separated without an empty quoted
line, keeping the content in distinct quote blocks. Use the existing quoted
paragraphs in the docs section as the reference points.
Source: Linters/SAST tools
| if cfg.Extensions != nil { | ||
| serverCfg.RequestRewriters = cfg.Extensions.Rewriters() | ||
| serverCfg.ExtraMiddleware = cfg.Extensions.Middleware() | ||
| serverCfg.ExtraRoutes = cfg.Extensions.Routes() | ||
| serverCfg.ExtraAuthSkipPaths = cfg.Extensions.PublicPaths() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing app-level test coverage of Extensions wiring
rg -n 'Extensions' internal/app/app_test.goRepository: ENTERPILOT/GoModel
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app.go mapping =="
nl -ba internal/app/app.go | sed -n '430,455p'
echo
echo "== app_test.go references =="
rg -n 'app\.New|New\(|Extensions|RequestRewriters|ExtraMiddleware|ExtraRoutes|ExtraAuthSkipPaths' internal/app/app_test.go internal/app -g '*_test.go' -g '*.go'
echo
echo "== repo-wide test coverage for serverCfg mapping fields =="
rg -n 'RequestRewriters|ExtraMiddleware|ExtraRoutes|ExtraAuthSkipPaths|PublicPaths\(\)|Rewriters\(\)|Middleware\(\)|Routes\(\)' internal -g '*_test.go' -g '*.go'Repository: ENTERPILOT/GoModel
Length of output: 217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('internal/app/app.go')
start, end = 430, 455
for i, line in enumerate(path.read_text().splitlines(), 1):
if start <= i <= end:
print(f"{i:4d}: {line}")
PY
echo
echo "== app_test.go references =="
rg -n 'app\.New|New\(|Extensions|RequestRewriters|ExtraMiddleware|ExtraRoutes|ExtraAuthSkipPaths' internal/app/app_test.go internal/app -g '*_test.go' -g '*.go' || true
echo
echo "== repo-wide test coverage for mapping fields =="
rg -n 'RequestRewriters|ExtraMiddleware|ExtraRoutes|ExtraAuthSkipPaths|PublicPaths\(\)|Rewriters\(\)|Middleware\(\)|Routes\(\)' internal -g '*_test.go' -g '*.go' || trueRepository: ENTERPILOT/GoModel
Length of output: 6829
Add app-level coverage for cfg.Extensions wiring Add a test in internal/app/app_test.go that calls app.New() with a non-nil Extensions registry and asserts RequestRewriters, ExtraMiddleware, ExtraRoutes, and ExtraAuthSkipPaths are copied onto serverCfg. The server tests cover server.Config directly, but not this translation path.
🤖 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 `@internal/app/app.go` around lines 439 - 444, Add app-level test coverage for
the cfg.Extensions wiring in app.New: create a test in internal/app/app_test.go
that passes a non-nil Extensions registry and verifies the resulting serverCfg
gets RequestRewriters, ExtraMiddleware, ExtraRoutes, and ExtraAuthSkipPaths
populated from cfg.Extensions. Use the app.New entrypoint and the
Extensions-related methods Rewriters, Middleware, Routes, and PublicPaths to
exercise the translation path that server.Config tests do not cover.
Source: Path instructions
| func RequestRewriteMiddleware(rewriters []ext.RequestRewriter, auditLogger auditlog.LoggerInterface) echo.MiddlewareFunc { | ||
| return func(next echo.HandlerFunc) echo.HandlerFunc { | ||
| return func(c *echo.Context) error { | ||
| endpoint, ok := rewriteEndpoint(c.Request()) | ||
| if !ok { | ||
| return next(c) | ||
| } | ||
|
|
||
| body, err := requestBodyBytes(c) | ||
| if err != nil { | ||
| return handleError(c, core.NewInvalidRequestError("failed to read request body", err)) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Skip body buffering entirely when no rewriters are registered.
requestBodyBytes(c) is always invoked for any eligible POST endpoint, even when rewriters is empty (e.g. the default OSS binary with no custom ext build). This buffers the full request body into memory for no benefit on the hot ingest path.
⚡ Proposed early-exit
func RequestRewriteMiddleware(rewriters []ext.RequestRewriter, auditLogger auditlog.LoggerInterface) echo.MiddlewareFunc {
return func(next echo.HandlerFunc) echo.HandlerFunc {
return func(c *echo.Context) error {
+ if len(rewriters) == 0 {
+ return next(c)
+ }
endpoint, ok := rewriteEndpoint(c.Request())
if !ok {
return next(c)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func RequestRewriteMiddleware(rewriters []ext.RequestRewriter, auditLogger auditlog.LoggerInterface) echo.MiddlewareFunc { | |
| return func(next echo.HandlerFunc) echo.HandlerFunc { | |
| return func(c *echo.Context) error { | |
| endpoint, ok := rewriteEndpoint(c.Request()) | |
| if !ok { | |
| return next(c) | |
| } | |
| body, err := requestBodyBytes(c) | |
| if err != nil { | |
| return handleError(c, core.NewInvalidRequestError("failed to read request body", err)) | |
| } | |
| func RequestRewriteMiddleware(rewriters []ext.RequestRewriter, auditLogger auditlog.LoggerInterface) echo.MiddlewareFunc { | |
| return func(next echo.HandlerFunc) echo.HandlerFunc { | |
| return func(c *echo.Context) error { | |
| if len(rewriters) == 0 { | |
| return next(c) | |
| } | |
| endpoint, ok := rewriteEndpoint(c.Request()) | |
| if !ok { | |
| return next(c) | |
| } | |
| body, err := requestBodyBytes(c) | |
| if err != nil { | |
| return handleError(c, core.NewInvalidRequestError("failed to read request body", 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 `@internal/server/request_rewrite.go` around lines 26 - 37,
RequestRewriteMiddleware always buffers the request body even when there are no
rewriters, which adds unnecessary overhead on the ingest path. Add an early exit
in RequestRewriteMiddleware, before calling requestBodyBytes(c), so that when
the rewriters slice is empty the middleware immediately delegates to next(c)
after rewriteEndpoint(c.Request()) succeeds; keep the existing rewrite/audit
flow only for the non-empty rewriters case.
| func defaultProviderFactory(cfg *config.Config) *providers.ProviderFactory { | ||
| factory := providers.NewProviderFactory() | ||
|
|
||
| if cfg.Metrics.Enabled { | ||
| factory.SetHooks(observability.NewPrometheusHooks()) | ||
| } | ||
|
|
||
| factory.Add(openai.Registration) | ||
| factory.Add(openrouter.Registration) | ||
| factory.Add(azure.Registration) | ||
| factory.Add(bailian.Registration) | ||
| factory.Add(oracle.Registration) | ||
| factory.Add(anthropic.Registration) | ||
| factory.Add(bedrock.Registration) | ||
| factory.Add(deepseek.Registration) | ||
| factory.Add(fireworks.Registration) | ||
| factory.Add(gemini.Registration) | ||
| factory.Add(vertex.Registration) | ||
| factory.Add(groq.Registration) | ||
| factory.Add(minimax.Registration) | ||
| factory.Add(ollama.Registration) | ||
| factory.Add(opencodego.Registration) | ||
| factory.Add(vllm.Registration) | ||
| factory.Add(xai.Registration) | ||
| factory.Add(xiaomi.Registration) | ||
| factory.Add(zai.Registration) | ||
|
|
||
| return factory | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add test coverage for defaultProviderFactory.
No test file accompanies this new function. A small table/behavior test verifying the metrics-hook conditional (cfg.Metrics.Enabled) and that all expected provider types are registered would guard against silent regressions when providers are added/removed.
✅ Example test sketch
func TestDefaultProviderFactory_RegistersMetricsHooksWhenEnabled(t *testing.T) {
cfg := &config.Config{Metrics: config.MetricsConfig{Enabled: true}}
factory := defaultProviderFactory(cfg)
// assert factory has hooks set, e.g. via an exported accessor or behavior check
}
func TestDefaultProviderFactory_RegistersAllProviders(t *testing.T) {
cfg := &config.Config{}
factory := defaultProviderFactory(cfg)
for _, typ := range []string{"openai", "openrouter", "azure", "bailian", "oracle", "anthropic", "bedrock", "deepseek", "fireworks", "gemini", "vertex", "groq", "minimax", "ollama", "opencodego", "vllm", "xai", "xiaomi", "zai"} {
// assert factory has a registration for typ
}
}As per coding guidelines, **/*_test.go: "Add or update tests for behavior changes, with coverage for request translation, response normalization, error handling, default configuration, and provider-specific parameter mapping."
🤖 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 `@run/providers.go` around lines 30 - 58, Add test coverage for
defaultProviderFactory to prevent regressions when provider registrations
change. Create tests around defaultProviderFactory that verify the
cfg.Metrics.Enabled branch sets observability.NewPrometheusHooks on the
ProviderFactory and that, with metrics disabled, no hooks are added. Also assert
that all expected provider registrations are present after the calls to
factory.Add in defaultProviderFactory, using provider type names or factory
inspection helpers so the tests remain stable if line numbers change.
Source: Coding guidelines
Audit entries record one request_revisions entry per rewriter that changed the body (name, sizes, optional detail, rewritten body when body logging is on) while request_body stays the original client request. The dashboard audit detail renders revisions as 'Rewritten' tabs between Request and Response. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
b28fbe3 to
1b13d56
Compare
Rewriters run post-auth and get UserPath for identity, so they never need raw credentials; masking Authorization/cookie/API-key values in Input.Header keeps secrets out of anything a rewriter might echo into its audit-trail detail. The Detail contract now states it must never contain secrets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds coverage review flagged as missing: applyExtensions snapshotting the registry into server.Config (extracted as a helper), default provider factory registering all 19 provider types with metrics on and off, and Run's --health/--ready dispatch against a live and an unreachable gateway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the CodeRabbit review — disposition of each finding: Fixed:
Skipped:
Full suite re-verified: unit, e2e, dashboard tests, golangci-lint 0 issues. 🤖 Generated with Claude Code |
|
@coderabbitai continue |
|
I'll kick off a fresh review pass now to verify everything against the latest commits. ✅ Action performedReview finished.
|
What this does
Gives GoModel a small, feature-neutral extension surface so custom gateway binaries can be built on top of it — the open-source binary itself becomes a thin wrapper over the same API. Split out of #457 so the module-path rename can be decided independently; this PR does not touch the module name (434 rename-only files dropped from the original diff).
1. Public
extpackage + ingress wiringA registry where custom builds register:
POST /v1/chat/completions,/v1/messages,/v1/responses(subroutes excluded) after auth and before workflow resolution, so body rewrites (includingmodel) affect routing, failover, guardrails, budgets, and caching. Fail-closed, errors rendered in each endpoint's native dialect.An empty registry adds zero request overhead — the middleware isn't even registered, so the default build is byte-for-byte unchanged in behavior.
2. Public
runpackagerun.Run(ctx, run.Options)is the full importable gateway lifecycle (CLI, probes, dotenv, logging, config, providers, signals);Options.Setupfires only when the process actually serves, keeping--versionand Docker healthchecks silent.cmd/gomodelkeeps byte-identical CLI behavior.3. Audit request-revision chain
request_bodystays the original client request; a newrequest_revisionsarray records each ingress rewrite (rewriter, sizes, optional detail, rewritten body when body logging is on) — mirroring the provider-attempts pattern — and the dashboard renders them as "Rewritten" tabs.The extension/embedding guide (
docs/extensions.md) ships with #457, since the embedding recipe only becomes practical once the module is go-gettable. Until then theext/runpackages should be treated as experimental with no compatibility promise.Testing
Full unit suite, e2e, 402 dashboard JS tests, and golangci-lint all green. New table-driven tests cover registry semantics, per-endpoint gating, rewriter chaining, fail-closed error envelopes per dialect, >64KB/>1MB bodies, audit-keeps-original for streaming and buffered paths, revision recording with/without body logging, and run CLI behavior.
🤖 Generated with Claude Code
Summary by CodeRabbit