Basic project scaffold with: chat completion, streaming and models endpoints from OpenAI#1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR establishes the foundational scaffold for HeavyModel, a high-performance LLM gateway written in Go. It provides OpenAI-compatible endpoints for chat completions (both streaming and non-streaming) and model listing, with a clean provider abstraction to support multiple LLM backends.
Key Changes:
- Implements core provider interface with OpenAI integration supporting GPT and O1 model families
- Sets up HTTP server with Echo framework providing
/v1/chat/completions,/v1/models, and/healthendpoints - Adds configuration management via Viper with support for environment variables and YAML config files
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod, go.sum | Defines Go 1.21 module with dependencies for Echo web framework and Viper configuration |
| Makefile | Provides build, run, test, and clean targets for development workflow |
| .gitignore | Basic ignore rules for compiled binary (minimal coverage) |
| README.md | Quick start documentation with Docker support and endpoint descriptions |
| config/config.yaml | Server configuration with port 8088 default and OpenAI API key placeholder |
| config/config.go | Configuration loader with Viper integration and environment variable override support |
| internal/core/types.go | Defines request/response types for chat completions and model listings |
| internal/core/interfaces.go | Provider interface defining model support checks, completions, and streaming |
| internal/providers/openai/openai.go | OpenAI provider implementation with HTTP client for API communication |
| internal/server/http.go | HTTP server setup with Echo, middleware, and route registration |
| internal/server/handlers.go | Request handlers for chat completions, streaming, health checks, and model listing |
| internal/server/handlers_test.go | Test suite covering successful request flows and basic error cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if req.Stream { | ||
| stream, err := h.provider.StreamChatCompletion(c.Request().Context(), &req) | ||
| if err != nil { | ||
| return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) |
There was a problem hiding this comment.
Exposing internal error details directly to API clients can leak sensitive information about the system's implementation. The error from h.provider.StreamChatCompletion could contain internal details, API keys, or stack traces. Consider using a generic error message for the client while logging the detailed error internally:
return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to process streaming chat completion"})| func (h *Handler) ListModels(c echo.Context) error { | ||
| resp, err := h.provider.ListModels(c.Request().Context()) | ||
| if err != nil { | ||
| return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) |
There was a problem hiding this comment.
Exposing internal error details directly to API clients can leak sensitive information. The error from h.provider.ListModels could contain internal details or API keys. Consider using a generic error message for the client while logging the detailed error internally:
return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to retrieve models"})| func (s *Server) Start(addr string) error { | ||
| return s.echo.Start(addr) | ||
| } |
There was a problem hiding this comment.
The HTTP server lacks timeout configurations, which could lead to resource exhaustion from slow or hanging connections. Consider configuring timeouts when starting the server:
func (s *Server) Start(addr string) error {
server := &http.Server{
Addr: addr,
Handler: s.echo,
ReadTimeout: 15 * time.Second,
WriteTimeout: 15 * time.Second,
IdleTimeout: 60 * time.Second,
}
return server.ListenAndServe()
}Note: You'll need to import "net/http" and "time".
| func (h *Handler) ChatCompletion(c echo.Context) error { | ||
| var req core.ChatRequest | ||
| if err := c.Bind(&req); err != nil { | ||
| return c.JSON(http.StatusBadRequest, map[string]string{ | ||
| "error": "invalid request body: " + err.Error(), | ||
| }) | ||
| } | ||
|
|
||
| if !h.provider.Supports(req.Model) { | ||
| return c.JSON(http.StatusBadRequest, map[string]string{ | ||
| "error": "unsupported model: " + req.Model, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing input validation for the chat request. The handler should validate that:
- The
Messagesarray is not empty - The
Modelfield is not an empty string - Message roles and content are not empty
Consider adding validation before processing the request:
if req.Model == "" {
return c.JSON(http.StatusBadRequest, map[string]string{
"error": "model is required",
})
}
if len(req.Messages) == 0 {
return c.JSON(http.StatusBadRequest, map[string]string{
"error": "messages array cannot be empty",
})
}| c.Response().Header().Set("Connection", "keep-alive") | ||
| c.Response().WriteHeader(http.StatusOK) | ||
|
|
||
| io.Copy(c.Response().Writer, stream) |
There was a problem hiding this comment.
The io.Copy call does not check for errors. If copying the stream fails, the error is silently ignored. Consider checking and logging the error:
if _, err := io.Copy(c.Response().Writer, stream); err != nil {
// Log error or handle appropriately
}|
|
||
| // Supports returns true if this provider can handle the given model | ||
| func (p *Provider) Supports(model string) bool { | ||
| return strings.HasPrefix(model, "gpt-") || strings.HasPrefix(model, "o1") |
There was a problem hiding this comment.
The model prefix "o1" should include a hyphen to properly match OpenAI's o1 model naming (e.g., "o1-preview", "o1-mini"). The current check would incorrectly match models starting with "o1" but not hyphenated, like "o123model". Consider using:
return strings.HasPrefix(model, "gpt-") || strings.HasPrefix(model, "o1-")| return strings.HasPrefix(model, "gpt-") || strings.HasPrefix(model, "o1") | |
| return strings.HasPrefix(model, "gpt-") || strings.HasPrefix(model, "o1-") |
| // Non-streaming | ||
| resp, err := h.provider.ChatCompletion(c.Request().Context(), &req) | ||
| if err != nil { | ||
| return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) |
There was a problem hiding this comment.
Exposing internal error details directly to API clients can leak sensitive information about the system's implementation. The error from h.provider.ChatCompletion could contain internal details, API keys, or stack traces. Consider using a generic error message for the client while logging the detailed error internally:
return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to process chat completion"})| func TestChatCompletion(t *testing.T) { | ||
| mock := &mockProvider{ | ||
| supportedModels: []string{"gpt-4o-mini"}, | ||
| response: &core.ChatResponse{ | ||
| ID: "chatcmpl-123", | ||
| Object: "chat.completion", | ||
| Created: 1234567890, | ||
| Model: "gpt-4o-mini", | ||
| Choices: []core.Choice{ | ||
| { | ||
| Index: 0, | ||
| Message: core.Message{Role: "assistant", Content: "Hello!"}, | ||
| FinishReason: "stop", | ||
| }, | ||
| }, | ||
| Usage: core.Usage{ | ||
| PromptTokens: 10, | ||
| CompletionTokens: 5, | ||
| TotalTokens: 15, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| e := echo.New() | ||
| handler := NewHandler(mock) | ||
|
|
||
| reqBody := `{"model": "gpt-4o-mini", "messages": [{"role": "user", "content": "Hi"}]}` | ||
| req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(reqBody)) | ||
| req.Header.Set("Content-Type", "application/json") | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| err := handler.ChatCompletion(c) | ||
| if err != nil { | ||
| t.Fatalf("handler returned error: %v", err) | ||
| } | ||
|
|
||
| if rec.Code != http.StatusOK { | ||
| t.Errorf("expected status 200, got %d", rec.Code) | ||
| } | ||
|
|
||
| body := rec.Body.String() | ||
| if !strings.Contains(body, "chatcmpl-123") { | ||
| t.Errorf("response missing expected ID, got: %s", body) | ||
| } | ||
| if !strings.Contains(body, "Hello!") { | ||
| t.Errorf("response missing expected content, got: %s", body) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test coverage for error scenarios in the ChatCompletion handler. While TestListModelsError demonstrates error handling for the ListModels endpoint, there are no tests for error cases in ChatCompletion, such as:
- Provider returning an error during non-streaming completion
- Provider returning an error during streaming completion
- Invalid request body (malformed JSON)
Consider adding test cases to cover these error paths.
| github.com/valyala/fasttemplate v1.2.2 // indirect | ||
| go.uber.org/atomic v1.9.0 // indirect | ||
| go.uber.org/multierr v1.9.0 // indirect | ||
| golang.org/x/crypto v0.17.0 // indirect |
There was a problem hiding this comment.
The golang.org/x/crypto version v0.17.0 has known vulnerabilities. Consider upgrading to at least v0.31.0 or later to address security issues. You can update by running:
go get golang.org/x/crypto@latest| golang.org/x/crypto v0.17.0 // indirect | |
| golang.org/x/crypto v0.31.0 // indirect |
| } | ||
|
|
||
| // Validate API key | ||
| if cfg.OpenAI.APIKey == "" { |
There was a problem hiding this comment.
The validation should be moved to config loader
…view quick wins (#472) * fix(anthropicapi): carry top_p and user in typed chat request fields /v1/messages ingress wrote top_p and metadata.user_id into ExtraFields even though core.ChatRequest has typed TopP and User fields. Wire output was identical, but internal consumers of the typed fields (Responses lowering, compatible-provider user copy) saw zero values for Anthropic-dialect traffic, and the split armed the duplicate-JSON-key hazard in the extras merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor: share client request-ID validator and remove dead code - providers.IsValidClientRequestID replaces three verbatim copies in the openai, openrouter, and azure packages; its table test moves next to it. - Remove dead responsecache.CacheTypeBoth (possible-refactoring #1) and the unreferenced circuitBreaker.Allow(). - Rename gateway/refactor_findings_test.go to edge_cases_test.go so the file is named for its content, not the review session that produced it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: add architecture review, correct guardrails claim, mark generated files - docs/dev/2026-07-04_architecture-review.md: full findings with file:line evidence, blast-radius analysis, and a sequenced refactoring roadmap. - CLAUDE.md: guardrail definitions are persisted in the guardrail_definitions store with admin CRUD; config.yaml only seeds it (was described as config-only). - .gitattributes: mark checked-in swagger artifacts linguist-generated so the 8k-line docs.go collapses in review diffs. - possible-refactoring.md: mark item 1 (CacheTypeBoth) done. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(providers): reject control characters in client request-ID validation IsValidClientRequestID documented a printable-ASCII contract but accepted any byte <= 127, letting control characters and DEL through; a control byte in a header value makes Go's transport reject the whole outbound request. Restrict to 0x20-0x7E and cover control/NUL/tab/DEL cases in the table test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
No description provided.