Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds generated Swagger/OpenAPI docs and runtime Swagger UI support, introduces swag dependencies and a Makefile target, propagates a new SwaggerEnabled config flag, updates the Go toolchain to 1.25, and adds Swagger annotations and generated spec registration at init. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Echo as Echo Server
participant DocsPkg as Docs package (`swagger.json` / `docs.go`)
participant SwaggerUI as Swagger UI assets
Client->>Echo: GET /swagger/index.html
Echo->>SwaggerUI: serve static UI assets
SwaggerUI-->>Client: HTML/JS
Client->>Echo: GET /swagger/swagger.json
Echo->>DocsPkg: read generated swagger.json / doc handler
DocsPkg-->>Echo: swagger JSON
Echo-->>Client: swagger JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/gomodel/docs/swagger.yaml (1)
1-474:⚠️ Potential issue | 🟡 MinorAuto-generated file — do not edit manually; this note covers spec quality findings.
Two valid static-analysis findings on this generated spec (both must be addressed in the source annotations if desired):
No HTTPS scheme enforcement (CKV_OPENAPI_20): The spec has no
schemes:field, so HTTP is implicitly allowed. For a hardened deployment, add@schemes httpsto themain.goannotation block. Since the host is currentlylocalhost, this is low urgency, but worth adding once the host is made configurable.Unbounded array fields (CKV_OPENAPI_21): Arrays such as
messages,choices,data,output, andcontenthave nomaxItems. While enforcement of limits belongs in request validation middleware, documenting them in the spec aids client-side tooling and adds an additional layer of defence. Consider documenting realistic limits (e.g.//@param... maximum 1000) once the limits are established.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gomodel/docs/swagger.yaml` around lines 1 - 474, The generated OpenAPI spec lacks HTTPS enforcement and has several unbounded arrays; update the source annotations: add an `@schemes` https annotation to the main.go swagger comment block (so the generated spec includes schemes: [https]) and add maxItems constraints to the array properties in the model definitions (e.g., core.ChatRequest.messages, core.ChatResponse.choices, core.ModelsResponse.data, core.ResponsesResponse.output, core.ResponsesOutputItem.content) by annotating the corresponding struct/field comments or `@Param` annotations with realistic maximums (e.g., maxItems=1000) so the generator emits maxItems for those arrays.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/gomodel/docs/docs.go`:
- Around line 731-742: SwaggerInfo.Host is hardcoded to "localhost:8080" and
Schemes is empty; after the bind address is computed in main.go (the addr
variable referenced in the comment), set docs.SwaggerInfo.Host = addr (or the
host:port string you use for listeners) so the generated spec shows the real
endpoint, and set docs.SwaggerInfo.Schemes explicitly (e.g., []string{"https"}
for production or based on your TLS/enabled flag) so the Swagger UI uses the
intended protocol; perform these one-line assignments in main.go once addr and
TLS settings are known.
In `@cmd/gomodel/docs/swagger.json`:
- Around line 715-721: The swagger.json exposes a BearerAuth apiKey in
securityDefinitions without enforcing HTTPS; update the source annotations by
adding the `@schemes` https annotation in main.go (so the generated OpenAPI
includes HTTPS-only schemes), then re-run swag init to regenerate swagger.json
and swagger.yaml; ensure BearerAuth remains under "securityDefinitions" but the
schemes array reflects "https" to prevent Authorization header use over plain
HTTP.
In `@cmd/gomodel/main.go`:
- Line 34: The `@host` annotation is hardcoded to localhost:8080 and bakes that
address into the generated Swagger spec; remove or ignore the static `@host`
annotation and instead set the runtime host on the exported SwaggerInfo (from
docs.go) using the actual configured address (for example in server.New() or in
cmd/gomodel/main.go) so the generated spec points to the correct server in
non-local deployments; update any code that regenerates docs to avoid
reintroducing the hardcoded `@host` annotation.
In `@go.mod`:
- Line 3: The go.mod's module directive "go 1.25.0" causes CI failures because
the runner uses Go 1.24; change the go version in go.mod back to the previous
compatible version (e.g., "go 1.24.0") or update your CI/GOTOOLCHAIN to use Go
1.25.0; specifically, edit the go.mod file to replace the "go 1.25.0" line with
"go 1.24.0" if you prefer the quick revert, or alternatively update your CI
pipeline configuration (GOTOOLCHAIN/runner image) to a Go 1.25 toolchain so the
"go 1.25.0" directive is supported.
In `@internal/admin/handler.go`:
- Around line 181-187: Add a missing Swagger failure annotation for HTTP 401 to
the ListModels handler and ensure consistency across the other admin endpoints:
update the comment block for ListModels to include an `@Failure 401` entry, and
likewise add `@Failure 401` to the comment blocks for `UsageSummary` and
`DailyUsage` in internal/admin/handler.go so the BearerAuth-protected endpoints
correctly document the unauthorized response.
In `@internal/server/http.go`:
- Line 14: internal/server currently blank-imports the generated swagger package
cmd/gomodel/docs which inverts the dependency direction; remove the import _
"cmd/gomodel/docs" from internal/server (the import statement near the top of
internal/server/http.go) and add that blank import into the binary's
initialization (for example in cmd/gomodel/main.go) ensuring it is imported
before calling server.New(); this keeps the docs side-effect inside the cmd
package and preserves the correct cmd → internal dependency.
---
Outside diff comments:
In `@cmd/gomodel/docs/swagger.yaml`:
- Around line 1-474: The generated OpenAPI spec lacks HTTPS enforcement and has
several unbounded arrays; update the source annotations: add an `@schemes` https
annotation to the main.go swagger comment block (so the generated spec includes
schemes: [https]) and add maxItems constraints to the array properties in the
model definitions (e.g., core.ChatRequest.messages, core.ChatResponse.choices,
core.ModelsResponse.data, core.ResponsesResponse.output,
core.ResponsesOutputItem.content) by annotating the corresponding struct/field
comments or `@Param` annotations with realistic maximums (e.g., maxItems=1000) so
the generator emits maxItems for those arrays.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/TESTING_STRATEGY.md (1)
206-224:⚠️ Potential issue | 🟡 MinorUpdate stale action versions in the example CI snippet.
The
go-versionline was updated to1.25, but the surrounding example still referencesactions/checkout@v4andactions/setup-go@v5, while the actual.github/workflows/test.ymlalready uses@v6for both. This creates a misleading discrepancy in the docs.📝 Proposed fix
- - uses: actions/checkout@v4 - - uses: actions/setup-go@v5 + - uses: actions/checkout@v6 + - uses: actions/setup-go@v6🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/TESTING_STRATEGY.md` around lines 206 - 224, Update the GitHub Actions example to use the current action versions by replacing the referenced actions in the snippet: change actions/checkout@v4 and actions/setup-go@v5 to the same `@v6` used in the real workflow; locate the example block containing the steps with "uses: actions/checkout" and "uses: actions/setup-go" and update their version suffixes to `@v6` so the docs and .github/workflows/test.yml are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Line 2: You updated the base image to "FROM --platform=$BUILDPLATFORM
golang:1.25-alpine3.23 AS builder" which introduces Go 1.25 runtime changes;
ensure go.mod declares "go 1.25", run the full test suite (make test-all) under
the new toolchain, and add a CI job that runs tests inside a container with CPU
limits to validate container-aware GOMAXPROCS behavior; also scan the codebase
for patterns like "value, err := f(); use(value); if err != nil { ... }" and fix
any occurrences so error checks happen before using returned values to avoid
nil-pointer panics.
---
Outside diff comments:
In `@docs/TESTING_STRATEGY.md`:
- Around line 206-224: Update the GitHub Actions example to use the current
action versions by replacing the referenced actions in the snippet: change
actions/checkout@v4 and actions/setup-go@v5 to the same `@v6` used in the real
workflow; locate the example block containing the steps with "uses:
actions/checkout" and "uses: actions/setup-go" and update their version suffixes
to `@v6` so the docs and .github/workflows/test.yml are consistent.
Remove hardcoded localhost for host
|
Looks awesome!
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/server/http.go (1)
60-83:⚠️ Potential issue | 🟠 MajorSwagger UI is blocked by auth middleware when
GOMODEL_MASTER_KEYis set.
/swagger/*is never added toauthSkipPaths, so the auth middleware will return401for every browser request to/swagger/index.htmlwhen a master key is configured. The admin dashboard correctly escapes this by appending its paths toauthSkipPaths(Line 82), but the Swagger route does not.🐛 Proposed fix
authSkipPaths := []string{"/health"} +// Swagger UI paths skip authentication (UI is publicly accessible; +// individual API calls through the UI still require BearerAuth) +if cfg != nil && cfg.SwaggerEnabled { + authSkipPaths = append(authSkipPaths, "/swagger/*") +} // Determine metrics path metricsPath := "/metrics"Also applies to: 141-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/http.go` around lines 60 - 83, authSkipPaths never includes the Swagger UI routes so requests under /swagger/* get 401 when a master key is set; update the same logic that appends admin UI paths to also append the Swagger paths (e.g. add "/swagger", "/swagger/*", and optionally "/swagger/index.html") when the swagger feature is enabled (check cfg.SwaggerEnabled or cfg.SwaggerHandler != nil) so authSkipPaths gets those entries; apply the same addition in the other place mentioned (lines ~141-143) where public/static routes are appended.CLAUDE.md (1)
147-161:⚠️ Potential issue | 🟡 Minor
SWAGGER_ENABLEDmissing from the Configuration Reference section.The Server config line (line 151) should list the new env var:
-**Server:** `PORT` (8080), `GOMODEL_MASTER_KEY` (empty = unsafe mode), `BODY_SIZE_LIMIT` ("10M") +**Server:** `PORT` (8080), `GOMODEL_MASTER_KEY` (empty = unsafe mode), `BODY_SIZE_LIMIT` ("10M"), `SWAGGER_ENABLED` (true)As per coding guidelines, documentation should be updated when adding new configuration options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 147 - 161, Add the missing SWAGGER_ENABLED env var to the Server configuration list in the Configuration Reference section: update the "Server" bullet (near the existing PORT, GOMODEL_MASTER_KEY, BODY_SIZE_LIMIT entries) to include SWAGGER_ENABLED with its default (e.g., false) and a short note about enabling the Swagger UI; ensure the entry is consistent with .env.template and config/config.yaml naming and formatting so documentation reflects the new config option.
♻️ Duplicate comments (1)
cmd/gomodel/docs/swagger.json (1)
2-4:⚠️ Potential issue | 🟠 Major
"schemes": ["http"]—BearerAuthstill exposed over cleartext.This was flagged in a previous review and reportedly fixed, but the generated file still only contains
http. The@schemes httpsannotation needs to be added to the general API info incmd/gomodel/main.go, thenmake swaggerrun to regenerate.// `@title` GOModel API // `@version` 1.0 // `@description` High-performance AI gateway... +// `@schemes` https // `@BasePath` /🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gomodel/docs/swagger.json` around lines 2 - 4, The swagger.json still lists "schemes": ["http"]; open the general API comment block in cmd/gomodel/main.go (the top-level Swagger info where annotations like `@title`, `@version`, `@description` live) and add the line "@schemes https" to that block, then re-run the swagger generation command (make swagger) to regenerate cmd/gomodel/docs/swagger.json so BearerAuth is only advertised over HTTPS.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
CLAUDE.mdMakefilecmd/gomodel/docs/docs.gocmd/gomodel/docs/swagger.jsoncmd/gomodel/main.goconfig/config.gointernal/admin/handler.gointernal/app/app.gointernal/server/http.go
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/gomodel/docs/swagger.json`:
- Around line 168-188: Add a Swagger/OpenAPI entry for the "/metrics" endpoint
under the same "system" tag as "/health": annotate the metrics handler (e.g.,
the function handling "/metrics", often named metricsHandler or MetricsHandler)
with a Swagger comment mirroring the "/health" block (include "get", "produces"
(e.g., "text/plain" or "application/json" as appropriate), "tags": ["system"],
"summary": "Metrics", and a "200" response schema or plain text response). After
adding the annotation to the metrics handler source, regenerate swagger.json so
the new "/metrics" path appears alongside "/health".
In `@cmd/gomodel/main.go`:
- Line 15: Remove the redundant blank side-effect import of
"gomodel/cmd/gomodel/docs" — keep the named import that provides
docs.SwaggerInfo and delete the duplicate `_ "gomodel/cmd/gomodel/docs"` import
so the package init() only occurs once and ST1019/lint will pass; ensure any use
of docs.SwaggerInfo remains unchanged.
- Around line 114-116: The conditional that sets docs.SwaggerInfo.Schemes based
on result.Config.Server.SwaggerEnabled is a no-op because
docs.SwaggerInfo.Schemes is already initialized to []string{"http"} in its
package init(); remove the entire if block (the lines referencing
result.Config.Server.SwaggerEnabled and docs.SwaggerInfo.Schemes) so you don't
reassign an identical value, or if you intended to avoid advertising HTTP when
Swagger is disabled, invert the logic to clear or change
docs.SwaggerInfo.Schemes only when SwaggerEnabled is false (reference symbols:
result.Config.Server.SwaggerEnabled and docs.SwaggerInfo.Schemes).
In `@internal/app/app.go`:
- Around line 161-163: The swagger UI log currently hardcodes
"http://localhost:{port}/..." which is misleading for non-local binds; update
the slog.Info call guarded by appCfg.Server.SwaggerEnabled to avoid hardcoding
localhost—either log only the path (e.g. "/swagger/index.html") consistent with
the admin API log or include the configured bind address from appCfg.Server
(e.g. bind host and appCfg.Server.Port) so the printed URL reflects the actual
bind; modify the slog.Info invocation that references appCfg.Server.Port
accordingly.
---
Outside diff comments:
In `@CLAUDE.md`:
- Around line 147-161: Add the missing SWAGGER_ENABLED env var to the Server
configuration list in the Configuration Reference section: update the "Server"
bullet (near the existing PORT, GOMODEL_MASTER_KEY, BODY_SIZE_LIMIT entries) to
include SWAGGER_ENABLED with its default (e.g., false) and a short note about
enabling the Swagger UI; ensure the entry is consistent with .env.template and
config/config.yaml naming and formatting so documentation reflects the new
config option.
In `@internal/server/http.go`:
- Around line 60-83: authSkipPaths never includes the Swagger UI routes so
requests under /swagger/* get 401 when a master key is set; update the same
logic that appends admin UI paths to also append the Swagger paths (e.g. add
"/swagger", "/swagger/*", and optionally "/swagger/index.html") when the swagger
feature is enabled (check cfg.SwaggerEnabled or cfg.SwaggerHandler != nil) so
authSkipPaths gets those entries; apply the same addition in the other place
mentioned (lines ~141-143) where public/static routes are appended.
---
Duplicate comments:
In `@cmd/gomodel/docs/swagger.json`:
- Around line 2-4: The swagger.json still lists "schemes": ["http"]; open the
general API comment block in cmd/gomodel/main.go (the top-level Swagger info
where annotations like `@title`, `@version`, `@description` live) and add the line
"@schemes https" to that block, then re-run the swagger generation command (make
swagger) to regenerate cmd/gomodel/docs/swagger.json so BearerAuth is only
advertised over HTTPS.
05beac5 to
3003bdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cmd/gomodel/main.go (1)
32-39: Swagger annotations look good overall, but@schemesshould includehttps.As flagged in prior review, the annotation at Line 36 only declares
http. For production deployments where the Authorization header carries bearer tokens, the spec should advertise HTTPS to prevent credential leakage over cleartext.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gomodel/main.go` around lines 32 - 39, The Swagger annotation block for the API (the comment block starting with "// `@title` GOModel API") only lists "@schemes http"; update that annotation to include HTTPS (e.g., change "@schemes http" to include "https" so it advertises "http, https" or "https" as appropriate) to ensure the generated spec signals secure transport for Authorization headers.cmd/gomodel/docs/swagger.json (1)
1-4: Schemes still set tohttponly —@schemes httpsannotation is still missing.The generated spec advertises HTTP only. The previous review identified this, and you confirmed it was addressed, but the generated output still shows
"schemes": ["http"]. The@schemesannotation needs to be added tomain.goand docs regenerated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gomodel/docs/swagger.json` around lines 1 - 4, The OpenAPI spec was generated advertising only "http" because the `@schemes` annotation is missing from main.go; add the annotation line "// `@schemes` https" (as a top-level file comment in the main package alongside other swag annotations in main.go) and then re-run your docs generator (e.g., swag init / the project’s doc generation command) to regenerate cmd/gomodel/docs/swagger.json so it contains "https" in the schemes array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/gomodel/docs/swagger.json`:
- Around line 562-602: The ResponsesRequest.input property lacks a "type",
causing validators to fail; add a concrete type (for example "type":"string") to
the "input" property in the core.ResponsesRequest definition in swagger.json, or
instead annotate the Go struct field (ResponsesRequest.Input) with a swaggo
directive (e.g., swaggertype:"string" or an appropriate `@Description`) so the
generated spec includes a concrete type for input while preserving a comment
documenting its polymorphic nature.
In `@cmd/gomodel/main.go`:
- Line 15: The import "gomodel/cmd/gomodel/docs" is unused and causing CI to
fail; convert it to a blank import to keep its init() side-effect (which
registers the swagger spec via swag.Register) by replacing the explicit import
with a blank import (underscore) so the package's init() still runs even though
no exported symbols (e.g., docs.SwaggerInfo) are referenced in main.go.
In `@internal/server/http.go`:
- Line 42: Add a proper Go doc comment line above the exported struct field
SwaggerEnabled describing its purpose for godoc (e.g., "SwaggerEnabled indicates
whether the Swagger UI should be exposed at /swagger/index.html"). Update the
field declaration for SwaggerEnabled in the HTTP server config struct so the
comment is a line-above doc comment (not just an inline trailing comment) and
keep the existing inline comment content consistent with the new doc comment for
clarity.
---
Duplicate comments:
In `@cmd/gomodel/docs/swagger.json`:
- Around line 1-4: The OpenAPI spec was generated advertising only "http"
because the `@schemes` annotation is missing from main.go; add the annotation line
"// `@schemes` https" (as a top-level file comment in the main package alongside
other swag annotations in main.go) and then re-run your docs generator (e.g.,
swag init / the project’s doc generation command) to regenerate
cmd/gomodel/docs/swagger.json so it contains "https" in the schemes array.
In `@cmd/gomodel/main.go`:
- Around line 32-39: The Swagger annotation block for the API (the comment block
starting with "// `@title` GOModel API") only lists "@schemes http"; update that
annotation to include HTTPS (e.g., change "@schemes http" to include "https" so
it advertises "http, https" or "https" as appropriate) to ensure the generated
spec signals secure transport for Authorization headers.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
CLAUDE.mdMakefilecmd/gomodel/docs/docs.gocmd/gomodel/docs/swagger.jsoncmd/gomodel/main.goconfig/config.gointernal/app/app.gointernal/server/http.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmd/gomodel/main.go (1)
31-38:⚠️ Potential issue | 🟠 Major
@schemes httpsends Bearer tokens in cleartext — usehttps(or both).The
@schemes httpannotation (Line 35) is the source for the generated spec'sschemesarray. Since this API uses Bearer token authentication, advertising onlyhttpmeans Swagger UI will default to cleartext requests, risking credential exposure. The author indicated this was already addressed in an earlier review round, but the annotation still readshttp.🔒 Proposed fix
// `@BasePath` / -// `@schemes` http +// `@schemes` https http // `@securityDefinitions.apikey` BearerAuthAfter updating, regenerate with
swag initto propagate the change todocs.go,swagger.json, andswagger.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gomodel/main.go` around lines 31 - 38, Update the Swagger annotation that currently reads "@schemes http" in cmd/gomodel/main.go to include secure transport (either "@schemes https" or both "@schemes http,https"), ensuring Bearer tokens are not advertised as only sent over cleartext; after changing the `@schemes` line, run "swag init" to regenerate docs (docs.go, swagger.json, swagger.yaml) so the spec reflects the secure scheme.cmd/gomodel/docs/swagger.json (1)
2-4: Schemes still advertisehttponly — generated artifact of the annotation inmain.go.This will be corrected when the
@schemesannotation inmain.gois updated to includehttpsand docs are regenerated. No manual edit needed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gomodel/docs/swagger.json` around lines 2 - 4, The generated swagger.json currently lists only "http" in "schemes" because the `@schemes` annotation in main.go was not updated; open main.go, locate the `@schemes` annotation (the comment starting with `@schemes`) and add "https" (or both "http" and "https" as desired), then re-run the docs/swagger generation step to regenerate cmd/gomodel/docs/swagger.json so the schemes array includes https; no manual edit of swagger.json is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/gomodel/docs/swagger.json`:
- Around line 2-4: The generated swagger.json currently lists only "http" in
"schemes" because the `@schemes` annotation in main.go was not updated; open
main.go, locate the `@schemes` annotation (the comment starting with `@schemes`) and
add "https" (or both "http" and "https" as desired), then re-run the
docs/swagger generation step to regenerate cmd/gomodel/docs/swagger.json so the
schemes array includes https; no manual edit of swagger.json is needed.
In `@cmd/gomodel/main.go`:
- Around line 31-38: Update the Swagger annotation that currently reads
"@schemes http" in cmd/gomodel/main.go to include secure transport (either
"@schemes https" or both "@schemes http,https"), ensuring Bearer tokens are not
advertised as only sent over cleartext; after changing the `@schemes` line, run
"swag init" to regenerate docs (docs.go, swagger.json, swagger.yaml) so the spec
reflects the secure scheme.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
cmd/gomodel/docs/docs.gocmd/gomodel/docs/swagger.jsoncmd/gomodel/main.gointernal/core/responses.go
| --dir .,../../internal/server,../../internal/admin,../../internal/usage,../../internal/providers,../../internal/core \ | ||
| --output docs \ | ||
| --outputTypes go,json | ||
|
|
There was a problem hiding this comment.
Great, that we have a make command!
@coderabbitai What do you think about adding the whol ../../internal? In the future we will have more modules/features which contains end-points documentation.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
swag init --dir flag recursive scanning best practices
💡 Result:
How swag init --dir/-d scanning works (and what to do for “recursive scanning”)
--dir/-dtakes a comma-separated list of directories to parse;swagwill walk those directories to find Go files/comments. The directory that contains your--generalInfo/-gfile must be listed first. [1]- Use
--exclude(comma-separated) to skip directories/files you don’t want scanned (common:vendor, generated code folders, build output,docs/, etc.). [1]
Best practices
- Point
-dat the smallest set of source roots that actually contain annotated handlers/models (instead of./), to reduce scan time/noise. [1] - Ensure the “general info” file is in the first
-ddirectory, and set it explicitly with-gif it’s notmain.go. [1] - Exclude heavy/irrelevant trees with
--exclude(especially if you also enable dependency parsing). [1] - If your repo root has no
.gofiles, avoid-d ./(some versions/structures can triggergo list/package-name failures and even hangs); set-dto the real code dirs (e.g.,cmd/yourapp,internal). [2] - If you enable dependency parsing (
--parseDependency/--pd,--parseInternal), consider limiting work with:--parseDependencyLevel(models/operations/all) [1]--parseDepthto cap dependency traversal depth (performance). [1]
Example (typical multi-dir layout):
swag init -d cmd/app,internal --exclude vendor,docs -g cmd/app/main.go@SantiagoDePolonia Great suggestion! Using ../../internal instead of listing individual subdirectories is definitely the better approach here.
Benefits:
- Future-proof: New modules/features with endpoint documentation will be automatically included
- Cleaner: Reduces Makefile maintenance when the codebase grows
- Simpler: One directory reference instead of five (and growing)
The swag tool recursively walks the specified directories looking for annotations, so it will automatically discover any new handlers you add under internal/ without requiring Makefile updates.
Suggested change:
swagger:
cd cmd/gomodel && swag init --generalInfo main.go \
--dir .,../../internal \
--output docs \
--outputTypes go,jsonThis should work perfectly and will scale better as the project grows. The only minor consideration is that swag will scan all subdirectories under internal/, but this is negligible performance-wise and harmless for directories without annotations.
🧠 Learnings used
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T11:39:16.238Z
Learning: Applies to tests/contract/**/*.go : Contract tests use golden file validation against real API responses, require build tag `-tags=contract`, 5-minute timeout, and are recorded via `make record-api`
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T11:39:16.238Z
Learning: Applies to **/*.go : Update in-code documentation (Go doc comments on exported types, functions, interfaces) when changing public APIs, adding new exported symbols, or modifying function signatures/behavior
Learnt from: giorgi-pro
Repo: ENTERPILOT/GOModel PR: 84
File: cmd/gomodel/docs/swagger.json:168-188
Timestamp: 2026-02-23T08:35:40.686Z
Learning: In GOModel (ENTERPILOT/GOModel), `/metrics` and similar infrastructure monitoring endpoints should not be documented in Swagger/OpenAPI specs. They are for internal infrastructure consumption (e.g., Prometheus scraping), not application-level API access, and should remain separate from the public API documentation.
There was a problem hiding this comment.
Why there is ., at your suggestion after --dir?
There was a problem hiding this comment.
@SantiagoDePolonia Good catch! The ., syntax might look confusing at first. Let me break it down:
.= the current directory (cmd/gomodel, since wecdinto it first),= separator for the comma-separated directory list that--diraccepts../../internal= the internal directory (relative tocmd/gomodel)
The . is required because that's where main.go lives—the file with your general API info annotations (@title, @version, @description, etc.). According to swag best practices, the directory containing the --generalInfo file must be listed first in the --dir flag.
So the full value .,../../internal tells swag to:
- Parse the current directory (
cmd/gomodel) for the general API info inmain.go - Also parse
../../internalfor all the handler annotations
Both directories are needed—one for the general info, one for the endpoint docs! 🎯
🧠 Learnings used
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T11:39:16.238Z
Learning: Applies to tests/contract/**/*.go : Contract tests use golden file validation against real API responses, require build tag `-tags=contract`, 5-minute timeout, and are recorded via `make record-api`
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T11:39:16.238Z
Learning: Applies to **/*.go : Update in-code documentation (Go doc comments on exported types, functions, interfaces) when changing public APIs, adding new exported symbols, or modifying function signatures/behavior
Learnt from: giorgi-pro
Repo: ENTERPILOT/GOModel PR: 84
File: cmd/gomodel/docs/swagger.json:168-188
Timestamp: 2026-02-23T08:35:40.686Z
Learning: In GOModel (ENTERPILOT/GOModel), `/metrics` and similar infrastructure monitoring endpoints should not be documented in Swagger/OpenAPI specs. They are for internal infrastructure consumption (e.g., Prometheus scraping), not application-level API access, and should remain separate from the public API documentation.
There was a problem hiding this comment.
@coderabbitai Would it work automatically without a --dir flag?
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
swag init recursive subdirectory scanning behavior
💡 Result:
swag init does scan subdirectories recursively for the directories you pass via --dir/-d.
--dir/-dtakes a comma-separated list of “directories you want to parse”, and the general-info file (-g) must be in the first directory. [1]- For each
--direntry, swag walks the entire tree usingfilepath.Walk(searchDir, ...), so all nested subfolders are visited (unless skipped). [2] - During that walk it skips directories by default:
vendor/(unless--parseVendoris set),docs/,- hidden folders (name starts with
.), - plus anything matched by
--exclude(skip is implemented asfilepath.SkipDir). [2]
- Note: parsing dependencies is different—when swag parses dependency packages, it reads only the files in each dependency package directory (no subdirs) (“only parsing files in the dir”). [2]
Sources: [1] (github.com), [2] (raw.githubusercontent.com)
Citations:
@SantiagoDePolonia Yes, absolutely! 🎯
swag init walks each --dir entry recursively using filepath.Walk, so ../../internal will automatically scan all subdirectories under internal/, including:
- ✅
internal/server/(your handlers) - ✅
internal/admin/(admin handlers) - ✅
internal/usage/(usage types) - ✅
internal/providers/(provider models) - ✅
internal/core/(core types) - ✅ Any future modules you add!
The only directories that are automatically skipped are:
vendor/docs/- Hidden folders (starting with
.)
So your original suggestion to simplify to --dir .,../../internal is perfect and will work exactly as intended. It'll recursively discover all Swagger annotations now and in the future, making the Makefile maintenance-free as you add new features! 🚀
🧠 Learnings used
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T11:39:16.238Z
Learning: Applies to tests/contract/**/*.go : Contract tests use golden file validation against real API responses, require build tag `-tags=contract`, 5-minute timeout, and are recorded via `make record-api`
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T11:39:16.238Z
Learning: Applies to **/*.go : Update in-code documentation (Go doc comments on exported types, functions, interfaces) when changing public APIs, adding new exported symbols, or modifying function signatures/behavior
Learnt from: giorgi-pro
Repo: ENTERPILOT/GOModel PR: 84
File: cmd/gomodel/docs/swagger.json:168-188
Timestamp: 2026-02-23T08:35:40.686Z
Learning: In GOModel (ENTERPILOT/GOModel), `/metrics` and similar infrastructure monitoring endpoints should not be documented in Swagger/OpenAPI specs. They are for internal infrastructure consumption (e.g., Prometheus scraping), not application-level API access, and should remain separate from the public API documentation.
There was a problem hiding this comment.
@coderabbitai What about then if it works recursively? --dir .
There was a problem hiding this comment.
@SantiagoDePolonia No, --dir . alone would not work. Here's the key distinction:
"Recursive" means scanning DOWN into subdirectories, not UP to parents.
When you run:
cd cmd/gomodel && swag init --dir .The . refers to cmd/gomodel, and swag will recursively scan:
- ✅
cmd/gomodel/(current dir - findsmain.go) - ✅
cmd/gomodel/docs/(child subdirectory) - ✅ Any other subdirectories under
cmd/gomodel/
But it will NOT scan:
- ❌
internal/(that's at../../internal- a parent directory) - ❌
internal/server/(where your handler annotations are) - ❌
internal/admin/(where your admin handler annotations are)
Result: Your Swagger would have the general API info from main.go but zero documented endpoints because all the handler annotations in internal/ would be missed.
Bottom line: You still need --dir .,../../internal to tell swag to scan both:
.(cmd/gomodel) for the general info../../internalfor all handler annotations (and their subdirectories recursively)
Your original suggestion to simplify to .,../../internal is the sweet spot! 🎯
🧠 Learnings used
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T11:39:16.238Z
Learning: Applies to tests/contract/**/*.go : Contract tests use golden file validation against real API responses, require build tag `-tags=contract`, 5-minute timeout, and are recorded via `make record-api`
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T11:39:16.238Z
Learning: Applies to **/*.go : Update in-code documentation (Go doc comments on exported types, functions, interfaces) when changing public APIs, adding new exported symbols, or modifying function signatures/behavior
Learnt from: giorgi-pro
Repo: ENTERPILOT/GOModel PR: 84
File: cmd/gomodel/docs/swagger.json:168-188
Timestamp: 2026-02-23T08:35:40.686Z
Learning: In GOModel (ENTERPILOT/GOModel), `/metrics` and similar infrastructure monitoring endpoints should not be documented in Swagger/OpenAPI specs. They are for internal infrastructure consumption (e.g., Prometheus scraping), not application-level API access, and should remain separate from the public API documentation.
There was a problem hiding this comment.
@coderabbitai What about running it without cd ... from the project home directory? Would it simplify the command?
There was a problem hiding this comment.
@SantiagoDePolonia Yes, excellent idea! Running from the project root would actually simplify things significantly. 🎯
From project root, the command would be:
swag init --generalInfo cmd/gomodel/main.go \
--dir cmd/gomodel,internal \
--output cmd/gomodel/docs \
--outputTypes go,jsonComparison:
Current (with cd):
cd cmd/gomodel && swag init --generalInfo main.go \
--dir .,../../internal \
--output docs \
--outputTypes go,jsonFrom project root:
swag init --generalInfo cmd/gomodel/main.go \
--dir cmd/gomodel,internal \
--output cmd/gomodel/docs \
--outputTypes go,jsonBenefits:
- ✅ No
cdneeded - ✅ No relative path navigation (
../../) - ✅ All paths are straightforward from project root
- ✅ More readable and maintainable
- ✅ Easier to understand at a glance
Makefile target would become:
swagger:
swag init --generalInfo cmd/gomodel/main.go \
--dir cmd/gomodel,internal \
--output cmd/gomodel/docs \
--outputTypes go,jsonThis is definitely the cleaner approach! All paths read naturally from the repository root, and you avoid the mental overhead of tracking relative paths. I'd recommend going with this version. 👍
🧠 Learnings used
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T11:39:16.238Z
Learning: Applies to tests/contract/**/*.go : Contract tests use golden file validation against real API responses, require build tag `-tags=contract`, 5-minute timeout, and are recorded via `make record-api`
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T11:39:16.238Z
Learning: Applies to **/*.go : Update in-code documentation (Go doc comments on exported types, functions, interfaces) when changing public APIs, adding new exported symbols, or modifying function signatures/behavior
Learnt from: giorgi-pro
Repo: ENTERPILOT/GOModel PR: 84
File: cmd/gomodel/docs/swagger.json:168-188
Timestamp: 2026-02-23T08:35:40.686Z
Learning: In GOModel (ENTERPILOT/GOModel), `/metrics` and similar infrastructure monitoring endpoints should not be documented in Swagger/OpenAPI specs. They are for internal infrastructure consumption (e.g., Prometheus scraping), not application-level API access, and should remain separate from the public API documentation.
|
@coderabbitai Was this resoved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2263 🏁 Script executed: Length of output: 6322 🏁 Script executed: Length of output: 775 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1866
Looking at The issue:
Result: When The fix needed: // Swagger UI skips auth
if cfg != nil && cfg.SwaggerEnabled {
authSkipPaths = append(authSkipPaths, "/swagger/*")
}This follows the same pattern used for the admin dashboard at line 82. The 🧠 Learnings used |
There was a problem hiding this comment.
Pull request overview
Integrates Swagger/OpenAPI generation and serving for the GOModel HTTP API, including handler annotations, a server route for Swagger UI, and configuration/CI updates to support the new docs workflow.
Changes:
- Added Swagger/OpenAPI annotations across public + admin handlers and general API metadata in
cmd/gomodel/main.go. - Wired
swaggo/echo-swaggerinto the HTTP server behind aSwaggerEnabledconfig flag (default enabled) and added a Makefile target for regeneration. - Bumped the Go toolchain to 1.25 across module/tooling/CI and committed generated Swagger artifacts.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/server/http.go | Adds Swagger UI route and config flag gating it. |
| internal/server/handlers.go | Adds Swagger annotations for public endpoints. |
| internal/admin/handler.go | Adds Swagger annotations for admin endpoints. |
| internal/core/responses.go | Adjusts Swagger typing for ResponsesRequest.Input. |
| internal/app/app.go | Plumbs SwaggerEnabled from app config into server config and logs when enabled. |
| config/config.go | Adds swagger_enabled config/env flag and defaults it to true. |
| cmd/gomodel/main.go | Adds top-level Swagger metadata annotations. |
| cmd/gomodel/docs/docs.go | Generated swaggo registration + template (committed artifact). |
| cmd/gomodel/docs/swagger.json | Generated Swagger spec (committed artifact). |
| Makefile | Adds swagger target to regenerate docs. |
| go.mod / go.sum | Adds swag dependencies and bumps Go version + indirect deps. |
| .github/workflows/test.yml | Updates CI Go version to 1.25. |
| docs/TESTING_STRATEGY.md | Updates documented CI Go version to 1.25. |
| Dockerfile | Updates builder image to Go 1.25. |
| CLAUDE.md | Updates stated Go version and documents the new make swagger command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Model string `json:"model"` | ||
| Input interface{} `json:"input"` // Can be string or []ResponsesInputItem | ||
| Input interface{} `json:"input" swaggertype:"object"` // Can be string or []ResponsesInputItem | ||
| Instructions string `json:"instructions,omitempty"` |
There was a problem hiding this comment.
ResponsesRequest.Input is documented as "string or []ResponsesInputItem", but the Swagger tag forces it to object, which misrepresents valid request shapes. Consider modeling this as json.RawMessage (with examples) or introducing a dedicated type that better reflects the union (swagger2 can’t express oneOf cleanly, but the schema should not claim it’s always an object).
There was a problem hiding this comment.
With swaggo we are limited to swagger 2.0. I think we can live with this for now, and later consider a different tool @SantiagoDePolonia
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ething-for-the-api' into me/gom-25-add-a-swagger-like-something-for-the-api
Add Swagger/OpenAPI documentation
Integrates
swaggo/swagto generate and serve interactive API documentation.Changes
cmd/gomodel/main.go(title, version, description, base path, Bearer auth security scheme)@Summary,@Router,@Param,@Success/@Failure) to all handlers:internal/server/handlers.go—Health,ListModels,ChatCompletion,Responsesinternal/admin/handler.go—UsageSummary,DailyUsage,ListModelscmd/gomodel/docs/(swagger.json, swagger.yaml, docs.go)swaggo/echo-swaggerv1.4.1 into the server, exposing Swagger UI at/swagger/index.htmlUsage
To regenerate docs after changing handler signatures:
Browse the API at http://localhost:8080/swagger/index.html when the server is running. You should see:
Summary by CodeRabbit
New Features
Documentation
Chores