fix: layer 2 — unify internal and pkg/llmproxy config types#408
fix: layer 2 — unify internal and pkg/llmproxy config types#408KooshaPari merged 1 commit intoclean-mainfrom
Conversation
- Add SDK types to pkg/llmproxy/config - Remove sdk_config.go (was causing type conflicts) - Add air.toml for hot reload development - Build succeeds with unified types
Summary of ChangesHello @KooshaPari, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the configuration type definitions within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to refactor configuration types to resolve an import cycle by moving SDKConfig and StreamingConfig to the pkg layer. This is a good architectural improvement. However, the implementation has introduced code duplication by not removing the original struct definitions from the internal package. This creates a maintainability issue. Additionally, the new air.toml configuration file for hot-reloading contains a minor redundancy. I've provided specific comments and suggestions to address these points.
| type SDKConfig struct { | ||
| // ProxyURL is the URL of an optional proxy server to use for outbound requests. | ||
| ProxyURL string `yaml:"proxy-url" json:"proxy-url"` | ||
|
|
||
| // ForceModelPrefix requires explicit model prefixes (e.g., "teamA/gemini-3-pro-preview") | ||
| // to target prefixed credentials. When false, unprefixed model requests may use prefixed | ||
| // credentials as well. | ||
| ForceModelPrefix bool `yaml:"force-model-prefix" json:"force-model-prefix"` | ||
|
|
||
| // RequestLog enables or disables detailed request logging functionality. | ||
| RequestLog bool `yaml:"request-log" json:"request-log"` | ||
|
|
||
| // APIKeys is a list of keys for authenticating clients to this proxy server. | ||
| APIKeys []string `yaml:"api-keys" json:"api-keys"` | ||
|
|
||
| // PassthroughHeaders controls whether upstream response headers are forwarded to downstream clients. | ||
| // Default is false (disabled). | ||
| PassthroughHeaders bool `yaml:"passthrough-headers" json:"passthrough-headers"` | ||
|
|
||
| // Streaming configures server-side streaming behavior (keep-alives and safe bootstrap retries). | ||
| Streaming StreamingConfig `yaml:"streaming" json:"streaming"` | ||
|
|
||
| // NonStreamKeepAliveInterval controls how often blank lines are emitted for non-streaming responses. | ||
| // <= 0 disables keep-alives. Value is in seconds. | ||
| NonStreamKeepAliveInterval int `yaml:"nonstream-keepalive-interval,omitempty" json:"nonstream-keepalive-interval,omitempty"` | ||
| } |
There was a problem hiding this comment.
While moving these struct definitions here from the internal package is the right direction to fix the import cycle, the original definitions in internal/config/sdk_config.go have not been removed. This has introduced code duplication, as SDKConfig and StreamingConfig are now defined in both pkg/llmproxy/config/sdk_types.go and internal/config/sdk_config.go.
To complete this refactoring correctly and avoid future maintenance issues, the definitions in internal/config/sdk_config.go should be replaced with type aliases that point to these new definitions in the pkg layer. This will create a single source of truth.
Here is a suggested implementation for internal/config/sdk_config.go:
// in internal/config/sdk_config.go
package config
import (
pkgconfig "github.com/router-for-me/CLIProxyAPI/v6/pkg/llmproxy/config"
)
// SDKConfig is an alias to pkg/llmproxy/config.SDKConfig.
type SDKConfig = pkgconfig.SDKConfig
// StreamingConfig is an alias to pkg/llmproxy/config.StreamingConfig.
type StreamingConfig = pkgconfig.StreamingConfig|
|
||
| [watcher] | ||
| watch_exts = ["go", "yaml", "yml", "json", "toml"] | ||
| ignore_paths = [".git", "node_modules", "vendor", "tmp", ".air"] |
There was a problem hiding this comment.
The paths listed in ignore_paths are already included in exclude_dir on line 10. According to the air tool's documentation, exclude_dir is sufficient for excluding directories from being watched, which makes this ignore_paths entry redundant. You can safely remove this line to simplify the configuration.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
feat: dynamic model fetching for GitHub Copilot
1 participant
Summary
Eliminates the import-cycle-prone alias pattern in
pkg/llmproxy/configby movingSDKConfigandStreamingConfigdefinitions directly into thepkglayer. Addsair.tomlfor hot-reload dev tooling.Changes
pkg/llmproxy/config/sdk_config.go(deleted): Removed type alias file that re-exportedinternal/config.SDKConfigpkg/llmproxy/config/sdk_types.go(new): Inline definitions ofSDKConfigandStreamingConfig— no longer depends oninternal/configfor these typesair.toml(new): Hot-reload config forairdev server targetingcmd/serverMotivation
The alias pattern (
type SDKConfig = internalconfig.SDKConfig) creates a direct import dependency frompkg/tointernal/. Moving the types topkg/makes the dependency graph cleaner and avoids future circular import issues.Stack Position
Layer 2 of stacked merge (base:
clean-main, depends on Layer 1)Test plan
go build ./...passesgo vet ./...passesconfig.SDKConfigstill compile (type is structurally identical)