chore: linter added + failover placeholder#3
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces golangci-lint for code quality enforcement and addresses linter warnings throughout the codebase. It also adds a placeholder failover configuration file.
- Adds golangci-lint configuration with essential linters (errcheck, gosimple, govet, ineffassign, staticcheck, unused)
- Updates code to satisfy linter requirements by improving error handling and adding package documentation
- Includes Makefile targets for running the linter and documentation in README.md
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| .golangci.yml | New linter configuration file with timeout and enabled linters |
| Makefile | Adds lint and lint-fix targets with updated PHONY declaration |
| README.md | Documents linter installation and usage instructions |
| internal/server/http.go | Removes trailing whitespace |
| internal/server/handlers.go | Adds package comment and improves error handling with proper defer patterns |
| internal/server/handlers_test.go | Reorders struct fields alphabetically |
| internal/providers/openai/openai.go | Adds package comment, reorders fields, and improves error handling with nolint directives |
| internal/core/types.go | Reorders struct fields alphabetically |
| internal/core/interfaces.go | Adds package documentation comment |
| config/config.go | Adds package comment and nolint directive for intentionally ignored error |
| cmd/gomodel/main.go | Adds package documentation comment |
| config/failover.yaml | Adds commented placeholder for future failover configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $(shell go env GOPATH)/bin/golangci-lint run ./... | ||
|
|
||
| # Run linter with auto-fix | ||
| lint-fix: | ||
| $(shell go env GOPATH)/bin/golangci-lint run --fix ./... |
There was a problem hiding this comment.
Using $(shell go env GOPATH) in the recipe itself may not work as expected. Consider using golangci-lint directly (assuming it's in PATH) or using a variable:
GOPATH ?= $(shell go env GOPATH)
lint-fix:
$(GOPATH)/bin/golangci-lint run --fix ./...Alternatively, just use golangci-lint run --fix ./... if it's expected to be in PATH after installation.
| $(shell go env GOPATH)/bin/golangci-lint run ./... | |
| # Run linter with auto-fix | |
| lint-fix: | |
| $(shell go env GOPATH)/bin/golangci-lint run --fix ./... | |
| golangci-lint run ./... | |
| # Run linter with auto-fix | |
| lint-fix: | |
| golangci-lint run --fix ./... |
|
|
||
| io.Copy(c.Response().Writer, stream) | ||
| if _, err := io.Copy(c.Response().Writer, stream); err != nil { | ||
| // Can't return error after headers are sent, log it |
There was a problem hiding this comment.
The comment indicates that the error should be logged, but the code doesn't actually log it. Consider adding actual logging:
if _, err := io.Copy(c.Response().Writer, stream); err != nil {
// Can't return error after headers are sent, log it
// TODO: Add proper logging (e.g., log.Error(err))
return nil
}Or if logging isn't implemented yet, the comment should reflect the current behavior.
| // Can't return error after headers are sent, log it | |
| // Can't return error after headers are sent, so log it | |
| c.Logger().Error(err) |
| @@ -0,0 +1,2 @@ | |||
| #failover: | |||
| # "gpt-5": [gemini3-pro, gemini2.5-pro] | |||
There was a problem hiding this comment.
The model name "gpt-5" doesn't exist. As of now, GPT-4 is the latest major version. Consider using an actual model name in the placeholder example (e.g., "gpt-4o-mini" or "gpt-4").
| # "gpt-5": [gemini3-pro, gemini2.5-pro] | |
| # "gpt-4": [gemini3-pro, gemini2.5-pro] |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ite mode The provider wrapper and the server-side batch preparer each re-implemented the per-type (chat/responses/embeddings) batch-item rewrite over core.RewriteBatchSource — one redirect-only, one redirect + access validation. Unify them behind a single rewriteBatchSource/rewriteBatchItem with an optional per-item access-validator hook: the server preparer passes ValidateModelAccess, the provider wrapper passes nil (behavior unchanged — the wrapper never validated access in its batch path). One per-type switch now, not two. With batch rewriting separated out, the requestRewriteMode/rewriteForUpstream machinery is dead (the translated path only ever routes), so the three translated-request rewrite helpers drop their unused mode/expectedProviderType params. No behavior change; DRY/clean-code follow-up to the virtual-models unification (review item #3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.