refactor: deduplicate shared helpers and consolidate app startup wiring#384
Merged
Conversation
Five packages (aliases, authkeys, guardrails, workflows, modelselectors) each defined an identical ValidationError type. Replace them with type aliases to a single internal/validation.Error, keeping every package's public API (ValidationError, IsValidationError) unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
escapeLikeWildcards and buildWhereClause were byte-for-byte duplicates in usage and auditlog; clampLimitOffset differed only in pagination bounds. Move them to internal/storage/sqlutil, with each package keeping a one-line clampLimitOffset wrapper expressing its policy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
authkeys and budget stores each carried their own copies of the unix-timestamp and nullable-string conversion helpers (with pg-prefixed duplicates in the PostgreSQL store). Move them to internal/storage/sqlutil under provider-agnostic names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
New() repeated hand-maintained errors.Join chains at every init failure point (11 chains, each re-listing every prior component, with order already drifting between blocks) and called firstSharedStorage with a growing argument list at each step. Replace both with a LIFO closers slice unwound by a single fail() helper, and a running sharedStorage claimed after each component initializes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 25-entry known-field list for responses requests was inlined twice in responses_json.go, differing only by the streaming controls. Define responsesUtilityRequestFields once with responsesRequestFields layering stream/stream_options on top, making that relationship explicit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Six providers repeated the same 3-line fallback that copies the requested model into responses that omit it. Replace with core.EnsureModel so the OpenAI-compatibility rule is stated once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pers native_batch_support.go re-exported five gateway functions (and a selection struct) without adding logic; most were only called from tests. Call gateway directly and keep only the two cleanup methods that carry real handler logic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four providers carried identical Enrich() mechanics differing only in their endpoint-to-semantics mapping. Move the mechanics to a shared providers.SemanticEnricher; each provider now declares just its endpoint table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Six providers repeated the same url.Values limit/cursor block in ListBatches. Replace with providers.PaginatedEndpoint, parameterized by cursor name to cover Anthropic's before_id paging. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every aggregation pipeline repeated the $ifNull cost sum and $cond presence-counter BSON blocks (14 of each), and every decoder repeated the has-count-to-pointer conversion (13 sites). Extract mongoCostSum, mongoCostPresenceCount, and costPtr. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The len-or-null-literal check on trimmed JSON appeared 12 times across core, guardrails, anthropicapi, and the anthropic provider. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
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 (1)
📝 WalkthroughWalkthroughThis PR adds shared utilities (validation, sqlutil, JSON-null, provider enrichment), migrates many modules to use them (stores, readers, providers, server tests), and refactors app startup/shutdown to centralize cleanup and shared storage claiming. ChangesCentralized Utilities and Cross-Module Consolidation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
|
Greptile SummaryThis PR deduplicates helper code and startup wiring across the gateway. The main changes are:
Confidence Score: 4/5This is close, but the validation error behavior should be fixed before merging.
Important Files Changed
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+34
to
+36
| func IsError(err error) bool { | ||
| _, ok := errors.AsType[*Error](err) | ||
| return ok |
There was a problem hiding this comment.
IsError now matches the single shared *validation.Error type, so every package aliasing this type accepts validation errors from every other package. For example, aliases.IsValidationError(err) now returns true for an auth key, guardrail, workflow, or model selector validation error, whereas those were distinct public error types before. Callers that use a package-specific predicate to choose response handling can now classify an unrelated subsystem error as this package's validation failure.
Contributor
Author
There was a problem hiding this comment.
Acknowledged but intended: the widening is the point of the consolidation. In practice each admin handler applies its predicate only to errors returned by its own subsystem service, and the one place validation errors already crossed package lines before this PR (model/pricing overrides aliasing modelselectors.ValidationError) wanted exactly this classification. If a foreign validation error ever did reach another package's predicate, classifying it as a 400 invalid-input rather than a 500 is the more accurate outcome. No caller relies on package-specific discrimination (verified: the only external consumers are the per-feature validationWriter wrappers in internal/admin/errors.go).
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/app/app.go`:
- Around line 117-147: The providers.Init error path bypasses the unwinder so
app.live can leak; before calling providers.Init you must register a closer that
will shut down app.live into the closers slice (e.g., append a func() error that
calls app.live.Shutdown(...) or app.live.Close() as appropriate) and then
replace the direct return on providers.Init failure with return fail("failed to
initialize providers", err) so the unified fail(...) unwinds app.live and other
closers; ensure you reference the existing closers slice and fail function and
add the app.live closer prior to calling providers.Init.
- Around line 472-477: The ResponseCacheMiddleware (rcm) is never closed on
normal shutdown because App.Shutdown doesn't call Server.Shutdown; update the
shutdown flow so rcm.Close is always executed: either (preferred) modify
App.Shutdown to call a.server.Shutdown(ctx) (which will invoke
s.responseCacheMiddleware.Close()), or ensure the App-level closers pipeline
explicitly includes rcm.Close on successful start and is executed during normal
shutdown; reference rcm / responseCacheMiddleware, App.Shutdown and
internal/server.Server.Shutdown to locate where to add the call or to add
rcm.Close to the App's closers so the cache is drained on normal shutdown.
In `@internal/providers/passthrough.go`:
- Around line 90-95: The lookup misses endpoints with trailing slashes because
Enrich() only strips leading slashes; modify the normalization used before the
endpoints map lookup (the normalizedEndpoint computed from
PassthroughEndpointPath(&enriched) and used in the lookup against e.endpoints)
to also trim trailing slashes (e.g., strings.TrimRight(..., "/")) so that
equivalents like "responses/" and "/messages/" resolve to the same key and
populate enriched.SemanticOperation and enriched.AuditPath; keep the existing
fallback that sets enriched.AuditPath =
"/p/"+e.providerType+"/"+normalizedEndpoint when no semantic exists. Also add a
regression test in the provider table tests that calls Enrich()/Passthrough path
logic with a known endpoint including a trailing slash and asserts the same
SemanticOperation/AuditPath as the non-trailing-slash variant to lock this
behavior.
🪄 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: c31fbc7e-fa20-4d9b-8af5-3c23bc523183
📒 Files selected for processing (55)
internal/aliases/store.gointernal/anthropicapi/request.gointernal/app/app.gointernal/auditlog/reader_helpers.gointernal/auditlog/reader_postgresql.gointernal/auditlog/reader_sqlite.gointernal/auditlog/user_path_filter.gointernal/auditlog/user_path_filter_test.gointernal/authkeys/store.gointernal/authkeys/store_postgresql.gointernal/authkeys/store_sqlite.gointernal/budget/store_postgresql.gointernal/budget/store_sqlite.gointernal/core/chat_content.gointernal/core/json_fields.gointernal/core/responses_json.gointernal/core/types.gointernal/guardrails/store.gointernal/guardrails/store_mongodb.gointernal/modelselectors/selectors.gointernal/providers/anthropic/anthropic.gointernal/providers/anthropic/passthrough_semantics.gointernal/providers/anthropic/passthrough_semantics_test.gointernal/providers/anthropic/request_translation.gointernal/providers/azure/azure.gointernal/providers/deepseek/deepseek.gointernal/providers/deepseek/passthrough_semantics.gointernal/providers/deepseek/passthrough_semantics_test.gointernal/providers/endpoints.gointernal/providers/gemini/gemini.gointernal/providers/groq/groq.gointernal/providers/ollama/ollama.gointernal/providers/openai/compatible_provider.gointernal/providers/openai/openai.gointernal/providers/openai/passthrough_semantics.gointernal/providers/openai/passthrough_semantics_test.gointernal/providers/passthrough.gointernal/providers/vllm/passthrough_semantics.gointernal/providers/vllm/vllm.gointernal/providers/xai/xai.gointernal/server/handlers_test.gointernal/server/native_batch_support.gointernal/server/native_batch_support_test.gointernal/server/native_response_service.gointernal/server/workflow_policy_test.gointernal/storage/sqlutil/sqlutil.gointernal/usage/reader_helpers.gointernal/usage/reader_mongodb.gointernal/usage/reader_postgresql.gointernal/usage/reader_sqlite.gointernal/usage/recalculate_pricing_postgresql.gointernal/usage/recalculate_pricing_sqlite.gointernal/usage/user_path_filter.gointernal/validation/error.gointernal/workflows/store.go
💤 Files with no reviewable changes (1)
- internal/server/native_batch_support.go
Comment on lines
+90
to
+95
| normalizedEndpoint := strings.TrimLeft(strings.TrimSpace(PassthroughEndpointPath(&enriched)), "/") | ||
| if semantics, ok := e.endpoints["/"+normalizedEndpoint]; ok { | ||
| enriched.SemanticOperation = semantics.Operation | ||
| enriched.AuditPath = semantics.AuditPath | ||
| } else if strings.TrimSpace(enriched.AuditPath) == "" && normalizedEndpoint != "" { | ||
| enriched.AuditPath = "/p/" + e.providerType + "/" + normalizedEndpoint |
Contributor
There was a problem hiding this comment.
Trim trailing slashes before the endpoint-table lookup.
Enrich() only removes leading /, so equivalent inputs like "responses/" or "/messages/" miss the endpoints map and fall through to the generic /p/{provider}/... audit path. That silently changes SemanticOperation/AuditPath classification for a user-supplied endpoint variant this shared normalizer should accept.
Suggested fix
- normalizedEndpoint := strings.TrimLeft(strings.TrimSpace(PassthroughEndpointPath(&enriched)), "/")
+ normalizedEndpoint := strings.Trim(strings.TrimSpace(PassthroughEndpointPath(&enriched)), "/")
if semantics, ok := e.endpoints["/"+normalizedEndpoint]; ok {Please add one regression case in the existing provider table tests for a known endpoint with a trailing slash so this stays locked down.
As per coding guidelines, **/*.go should accept user requests generously and preserve the OpenAI-compatible public API; the PR objective also says audit/usage semantics should not change.
📝 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.
Suggested change
| normalizedEndpoint := strings.TrimLeft(strings.TrimSpace(PassthroughEndpointPath(&enriched)), "/") | |
| if semantics, ok := e.endpoints["/"+normalizedEndpoint]; ok { | |
| enriched.SemanticOperation = semantics.Operation | |
| enriched.AuditPath = semantics.AuditPath | |
| } else if strings.TrimSpace(enriched.AuditPath) == "" && normalizedEndpoint != "" { | |
| enriched.AuditPath = "/p/" + e.providerType + "/" + normalizedEndpoint | |
| normalizedEndpoint := strings.Trim(strings.TrimSpace(PassthroughEndpointPath(&enriched)), "/") | |
| if semantics, ok := e.endpoints["/"+normalizedEndpoint]; ok { | |
| enriched.SemanticOperation = semantics.Operation | |
| enriched.AuditPath = semantics.AuditPath | |
| } else if strings.TrimSpace(enriched.AuditPath) == "" && normalizedEndpoint != "" { | |
| enriched.AuditPath = "/p/" + e.providerType + "/" + normalizedEndpoint |
🤖 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/providers/passthrough.go` around lines 90 - 95, The lookup misses
endpoints with trailing slashes because Enrich() only strips leading slashes;
modify the normalization used before the endpoints map lookup (the
normalizedEndpoint computed from PassthroughEndpointPath(&enriched) and used in
the lookup against e.endpoints) to also trim trailing slashes (e.g.,
strings.TrimRight(..., "/")) so that equivalents like "responses/" and
"/messages/" resolve to the same key and populate enriched.SemanticOperation and
enriched.AuditPath; keep the existing fallback that sets enriched.AuditPath =
"/p/"+e.providerType+"/"+normalizedEndpoint when no semantic exists. Also add a
regression test in the provider table tests that calls Enrich()/Passthrough path
logic with a known endpoint including a trailing slash and asserts the same
SemanticOperation/AuditPath as the non-trailing-slash variant to lock this
behavior.
Source: Coding guidelines
Contributor
Author
There was a problem hiding this comment.
Not fixing in this PR: the claim that this "silently changes" classification is incorrect — the original per-provider implementations used the identical strings.TrimLeft(strings.TrimSpace(...), "/") + switch, so a trailing-slash endpoint missed the match table before this refactor too (verified against the pre-refactor files). PassthroughEndpointPath doesn't strip trailing slashes either. Accepting trailing slashes would be a deliberate behavior change to audit/semantic classification, which this refactor explicitly avoids; happy to do it as a small follow-up PR with regression tests if desired.
Contributor
There was a problem hiding this comment.
@SantiagoDePolonia, thanks for the detailed correction — that's a fair point. The refactor faithfully preserves the pre-existing normalization behavior, and framing it as "silently changes" classification was wrong on my part. The strings.TrimLeft(..., "/") pattern being identical to the original per-provider switch-based implementations confirms this is a straight lift, not a behavioral delta.
If you'd like, I can open a follow-up GitHub issue to track the trailing-slash normalization improvement (with the regression test you mentioned). Just say the word.
✏️ Learnings added
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GoModel PR: 384
File: internal/providers/passthrough.go:90-95
Timestamp: 2026-06-10T14:37:15.288Z
Learning: In `internal/providers/passthrough.go`, `SemanticEnricher.Enrich()` uses `strings.TrimLeft(strings.TrimSpace(PassthroughEndpointPath(&enriched)), "/")` to normalize the endpoint before the map lookup. This is intentionally identical to the pre-refactor per-provider implementations (e.g., anthropic, openai, deepseek, vllm passthrough semantics), which used the same pattern. `PassthroughEndpointPath` does not strip trailing slashes, so endpoints like "responses/" will miss the lookup table — but this is pre-existing behavior preserved deliberately. Any change to accept trailing slashes would be a deliberate behavior change to audit/semantic classification and should be done in a separate PR with explicit regression tests.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/auditlog_test.go:272-279
Timestamp: 2026-01-15T23:07:26.646Z
Learning: Go code guideline: Avoid using interface{} and map[string]interface{} for API request/response payload types. This should be restricted to public API surfaces and data payloads. It is acceptable to use interface{} in internal infrastructure packages (e.g., storage abstractions) only to break import cycles, and only when type assertions occur at initialization (not on every request) to minimize runtime type checks and improve readability. Apply this rule broadly to files under internal/ that deal with payload-like types, but not to internal components that are pure infrastructure or have runtime-per-request type assertions.
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/factory.go:112-143
Timestamp: 2026-01-15T23:07:37.652Z
Learning: Guideline: Do not use interface{} or map[string]interface{} for API request/response payload types. Prefer strongly-typed structs for API payload definitions to improve type safety, serialization, and documentation. Allow interface{} only in internal infrastructure code paths where pragmatic flexibility is necessary (e.g., to avoid import cycles or to handle highly dynamic internal contracts). In internal/auditlog/factory.go and similar non-API implementation files, applying this restriction is optional and should be evaluated on a case-by-case basis based on whether the type remains internal and does not define API boundary shapes.
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 99
File: internal/providers/responses_adapter.go:128-131
Timestamp: 2026-02-25T16:55:22.071Z
Learning: In internal/providers/responses_adapter.go (and related files), Do and DoStream should already normalize provider errors using core.ParseProviderError(). Call sites (e.g., ChatCompletion, StreamChatCompletion) receive normalized GatewayError types and must not call ParseProviderError again. When reviewing code, ensure no downstream calls duplicate Normalize error handling and that all provider-error surfaces rely on the GatewayError type as the canonical representation. If you find code that re-parses errors with ParseProviderError, suggest removing that duplication and relying on the existing normalization path.
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GoModel PR: 294
File: internal/admin/handler_usage.go:243-244
Timestamp: 2026-04-30T07:12:57.910Z
Learning: In Go code targeting Go 1.26+ (e.g., this repo uses Go 1.26.x), do not flag `errors.AsType[T](err)` as a missing/non-existent API. `errors.AsType[E error](err error)` returns `(E, bool)` as a type-safe, generic alternative to `errors.As`. For example, code like `if val, ok := errors.AsType[*SomeErrorType](err); ok { ... }` is valid standard-library usage and should be treated as such during review.
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GoModel PR: 294
File: internal/admin/handler_usage.go:243-244
Timestamp: 2026-04-30T07:12:57.910Z
Learning: In Go 1.26+ code, `errors.AsType[T](err)` is a standard-library, generic alternative to `errors.As`. Do not flag `errors.AsType[...]` as a non-existent function or compilation error when the repository’s Go version is 1.26 or newer. The expected usage pattern is `if val, ok := errors.AsType[*SomeErrorType](err); ok { ... }`.
Two lifecycle gaps surfaced by PR review: - Startup failures returned before anything closed the live log broker; it is now the first entry on the closers stack, and the providers.Init failure routes through the same unwinder as every later step. - Server.Shutdown (which drains in-flight response cache writes and closes the response/conversation stores) was never invoked: graceful shutdown only cancelled the HTTP server context. App.Shutdown now calls it once the server has stopped. Both gaps predate the refactor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
2 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
Cleanup pass across the codebase: removes duplicated code and dead indirection in 14 focused commits, −327 net lines with no behavior changes. Full test suite (including
make test-raceand lint via pre-commit) passed on every commit.Cross-package deduplication
ValidationErrorwas defined identically in 5 packages (aliases, authkeys, guardrails, workflows, modelselectors) → singleinternal/validation.Errorbehind type aliases; every package's public API (ValidationError,IsValidationError) is unchangedinternal/storage/sqlutilpackage collects helpers that had drifted into copies: LIKE-wildcard escaping, WHERE-clause building, pagination clamping (parameterized — usage keeps 50/200, auditlog keeps 25/100), unix-timestamp/nullable-string column converters, and SQLite timestamp parsingApp startup wiring (
internal/app/app.go, −94 lines)New()repeated hand-maintainederrors.Joinclose chains at 11 failure points, each re-listing every prior component — and the close order had already drifted between blocks. Replaced with a LIFOclosersslice unwound by a singlefail()helper, so shutdown order on startup failure has one source of truthfirstSharedStorage(a, b, c, ...)calls became a single runningsharedStorageclaimed after each component initializesProvider layer
Enrich()mechanics → sharedproviders.SemanticEnricher; each provider now declares only its endpoint tableListBatchespagination query building deduplicated across 6 providers (providers.PaginatedEndpoint, cursor param name covers Anthropic'sbefore_id)core.EnsureModelnames the empty-model response fallback repeated at 12 sites; ollama's five unsupported-batch stubs share one error constructorCore / JSON / readers
stream/stream_options) → defined once, with the full-request variant layered explicitly on topcore.IsJSONNullreplaces 12 copies of the trimmed-null check;slices.Containsreplaces a hand-rolled searchmongoCostSum/mongoCostPresenceCount/costPtrhelpers for the repeated cost-aggregation BSON and decode blocksserver/native_batch_support.go: removed five pass-through wrappers aroundgatewayfunctions (most were test-only); callers usegatewaydirectlyUser-visible impact
None intended. Public APIs, error types, wire formats, and audit/usage semantics are unchanged. The only message-level change: a few startup failure error strings now use the uniform
failed to <step>: <err> (also: close error: <err>)format.Notes for reviewers
app.gorewrite is the most behavior-sensitive commit (73c1dc4): cleanup-on-failure now always closes in exact reverse-init order, where the old hand-written chains had minor order inconsistencies between blocks (e.g. the response-cache failure path closed workflows before authKeys). Closing order among these independent stores is not semantically significant, which is why this is safe — but worth a look.main(Go 1.26.4 bump, TTS audio-duration costing) is merged in.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Bug Fixes