Closed
Conversation
WalkthroughPropagates config path and embedded example YAML to ProxyManager instances, exposes API endpoints to read/update current and example configs, embeds example YAML, adds RPC health checks and request timeouts, introduces a Svelte two-panel YAML editor with CodeMirror deps, and provides a sample config and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@proxy/proxymanager_api.go`:
- Around line 37-38: The /api/config GET/POST routes (registered as
apiGroup.GET("/config", pm.apiGetConfig) and apiGroup.POST("/config",
pm.apiUpdateConfig)) currently rely solely on apiKeyAuth; add an explicit opt-in
guard to prevent accidental public exposure by implementing a configuration flag
(e.g., EnableConfigAPI) or env var and/or a localhost-only middleware and apply
it to these two routes: if the flag is false (or the request is not from
localhost when using the localhost guard) return 403 before invoking
pm.apiGetConfig / pm.apiUpdateConfig; place the check in a small middleware
function or at the top of those handler methods so the decision is centralized
and easy to test.
- Around line 318-321: Current write uses os.WriteFile(absConfigPath,
[]byte(req.Content), 0644) which widens permissions; change to preserve existing
permissions or default to restrictive 0600: call os.Stat(absConfigPath) to see
if the file exists and capture its FileMode (mode.Perm()); if it exists, use
that permission when writing (or write then call os.Chmod(absConfigPath,
existingPerm)), otherwise use 0600 for new files; update the write flow around
absConfigPath/req.Content to use os.WriteFile with the chosen perm and ensure a
post-write os.Chmod when needed, referencing absConfigPath, req.Content,
os.Stat, os.WriteFile, and os.Chmod in the fix.
🧹 Nitpick comments (1)
ui/src/pages/Config.tsx (1)
25-28: MissingloadConfigin useEffect dependency array.The
loadConfigfunction is called inside the effect but not listed in the dependency array. While this pattern achieves "run once on mount" behavior, it violates React's exhaustive-deps rule and may cause linting warnings.Consider either adding the dependency and using a ref to track initialization, or adding an ESLint disable comment to document the intent:
// Load config on mount useEffect(() => { loadConfig(); + // eslint-disable-next-line react-hooks/exhaustive-deps }, []);
Fix parseEndpointList to handle single and double quotes that are treated as literal characters on Windows. - Strip surrounding quotes before parsing comma-separated endpoints - Fixes test failures on Windows CI Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ui-svelte/src/routes/Config.svelte`:
- Around line 331-351: The rendering loop in renderCliArgs() uses
line.indexOf(arg) which always returns the first match, breaking lines with
multiple args; fix by iterating matches with their actual positions (e.g., using
matchAll or regex.exec) and track a sliding cursor (lastIndex) per line to
compute beforeArg and afterArg from the match's index and length so each
argument button is sliced from the correct location; update the cursor after
rendering each arg so handleCopyArg, copiedArg and the surrounding text render
correctly for lines with multiple arguments.
🧹 Nitpick comments (4)
ui-svelte/src/stores/api.ts (1)
199-202: Handle non-JSON error responses gracefully.If the server returns a non-2xx response with a body that isn't valid JSON (e.g., plain text or HTML error page),
response.json()will throw a parsing error, masking the original HTTP status.♻️ Proposed fix
if (!response.ok) { - const data = await response.json(); - throw new Error(data.error || `Failed to save config: ${response.status}`); + let errorMsg = `Failed to save config: ${response.status}`; + try { + const data = await response.json(); + if (data.error) errorMsg = data.error; + } catch { + // Response body wasn't JSON, use default error message + } + throw new Error(errorMsg); }ui-svelte/src/routes/Config.svelte (3)
41-63: Incomplete YAML validation and unused variable.The
validateYAMLfunction only checks for tab indentation, but theindentStackvariable on line 45 is declared and never used. This suggests incomplete implementation. The current validation may give users a false sense of security since it doesn't catch common YAML errors like invalid indentation levels, missing colons, or unquoted special characters.Consider either removing the unused variable or implementing more comprehensive validation. Alternatively, use a proper YAML parsing library for validation.
♻️ Remove unused variable (minimal fix)
function validateYAML(content: string): string { try { // Basic YAML validation - check for common syntax errors const lines = content.split('\n'); - let indentStack: number[] = [0]; for (let i = 0; i < lines.length; i++) { const line = lines[i];
177-237: Consider movingloadConfig()outside the try block for better error isolation.Currently,
loadConfig()is called at line 236 after the Monaco setup, but if Monaco fails, the code still attempts to load config. This is fine, but the cleanup function (lines 226-230) is only returned if Monaco loads successfully. If Monaco fails but the component unmounts, theisDarkModesubscription won't be cleaned up (though in this case there's no subscription created).The structure is acceptable, but for clarity, consider restructuring:
♻️ Suggested restructure for clarity
onMount(async () => { + // Load config first - this doesn't depend on Monaco + await loadConfig(); + // Dynamically import Monaco editor try { const monaco = await import('monaco-editor'); // ... Monaco setup ... return () => { unsubscribe(); editor?.dispose(); exampleEditor?.dispose(); }; } catch (err) { console.error('Failed to load Monaco editor:', err); error = 'Failed to load code editor. Using fallback.'; } - - await loadConfig(); });
142-152: Redundant return statement inrenderCliArgs.The function checks for
'No matches found.'and returns it, then immediately returnstextToRenderanyway. This check is unnecessary since both paths return the same value.♻️ Simplify the function
function renderCliArgs(): string { - const textToRender = cliArgsSearch.trim() + return cliArgsSearch.trim() ? filterCliArgs(CLI_ARGS_REFERENCE, cliArgsSearch) : CLI_ARGS_REFERENCE; - - if (textToRender === 'No matches found.') { - return textToRender; - } - - return textToRender; }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ui-svelte/src/routes/Config.svelte`:
- Around line 55-58: The current check uses line.match(/^\t/) which only catches
a tab in column 0; update the indentation check to detect any tab character
inside the leading indentation by replacing that regex with one that matches
tabs anywhere in the leading whitespace (e.g., use /^[ \t]*\t/ or /^[ ]+\t/ via
if (/^[ \t]*\t/.test(line))). Keep the same error message and the same
loop/variable (line) so lines with spaces followed by a tab are flagged.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@proxy/process.go`:
- Around line 529-553: The timeout goroutine can call StopImmediately while the
process is in StateStarting, but StopImmediately currently only swaps from
StateReady so the kill fails; update the shutdown logic so the timeout path can
always force-stop a starting process: modify StopImmediately (or add a new
ForceStop/StopForce method) to accept and handle StateStarting in addition to
StateReady (perform the same kill/cleanup and state transition), and ensure the
timeout goroutine uses that method when the timer fires so long-starting models
are terminated correctly.
- Around line 412-413: The rpc health fields (rpcHealthCancel, rpcHealthTicker)
are accessed concurrently; add a dedicated mutex (e.g., rpcHealthMu) on the
Process struct and use it in startRPCHealthChecker and stopRPCHealthChecker to
serialize access: in startRPCHealthChecker lock rpcHealthMu while initializing
rpcHealthTicker and rpcHealthCancel and capture any values you need for the
spawned goroutine into locals (so the goroutine uses the local copy, not reading
p.rpcHealthTicker directly), and in stopRPCHealthChecker lock rpcHealthMu to
atomically nil out and retrieve rpcHealthCancel/rpcHealthTicker into local vars,
then unlock and call the cancel/stop on those locals; also ensure any deferred
cleanup inside the goroutine updates p.rpcHealthTicker/p.rpcHealthCancel under
rpcHealthMu when setting them to nil.
In `@ui-svelte/src/routes/Config.svelte`:
- Around line 183-249: The cleanup returned from the async IIFE isn't being
registered by $effect, so Monaco editors and the theme subscription never get
disposed; move the teardown to the outer $effect so it is returned
synchronously. Concretely, in the $effect surrounding editor initialization,
create variables (e.g., let disposeEditors = () => {}, let unsubscribe = null)
before starting the async IIFE; inside the IIFE assign disposeEditors to a
function that disposes editor and exampleEditor and unsubscribe (and set
editorsInitialized appropriately) or set unsubscribe from isDarkMode.subscribe;
then at the end of the outer $effect return a single cleanup function that calls
unsubscribe (if set) and disposeEditors(); ensure references to editor,
exampleEditor, isDarkMode.subscribe, and editorsInitialized are used so
disposals run when the effect re-runs or the component unmounts.
🧹 Nitpick comments (2)
proxy/config/config.go (1)
537-566: Multiple--rpcflags overwrite rather than accumulate endpoints.The current implementation overwrites
endpointseach time an--rpcflag is encountered. If a command contains multiple--rpcflags, only the last one's endpoints are kept. This may be intentional (last-wins semantics), but if accumulation is desired:♻️ Optional fix to accumulate endpoints from multiple --rpc flags
var endpoints []string for i, arg := range args { if arg == "--rpc" || arg == "-rpc" { if i+1 < len(args) { - endpoints = parseEndpointList(args[i+1]) + endpoints = append(endpoints, parseEndpointList(args[i+1])...) } } else if strings.HasPrefix(arg, "--rpc=") { - endpoints = parseEndpointList(strings.TrimPrefix(arg, "--rpc=")) + endpoints = append(endpoints, parseEndpointList(strings.TrimPrefix(arg, "--rpc="))...) } else if strings.HasPrefix(arg, "-rpc=") { - endpoints = parseEndpointList(strings.TrimPrefix(arg, "-rpc=")) + endpoints = append(endpoints, parseEndpointList(strings.TrimPrefix(arg, "-rpc="))...) } }docs/configuration.md (1)
75-85: Minor: Use hyphen in compound adjective."event driven" should be "event-driven" when used as a compound adjective modifying "functionality".
📝 Suggested fix
-| `hooks` | event driven functionality | +| `hooks` | event-driven functionality |
overcuriousity
force-pushed
the
feat--web-config
branch
2 times, most recently
from
48ca15e to
c8f2761
Compare
overcuriousity
force-pushed
the
feat--web-config
branch
2 times, most recently
from
ced6f52 to
c8f2761
Compare
RPC health checking now runs continuously from process creation until proxy shutdown, completely independent of whether the model is loaded, starting, stopped, or in any other state. - Start health checker in NewProcess when rpcHealthCheck is enabled - Remove stopRPCHealthChecker - only stops on proxy shutdown - Remove state checks from health checker goroutine - Health status always reflects current RPC endpoint availability Previously, the health checker only ran while a process was in StateReady, causing stale health data when processes stopped. Now /v1/models always shows accurate RPC health regardless of model state. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Work in progress on web configuration feature. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The requestTimeout feature was not working because the timeout context was not connected to the HTTP request. When the timeout fired, it attempted to kill the process but the reverse proxy continued waiting for a response indefinitely. - Use context.WithTimeout() to create a timeout context for the HTTP request - Clone the request with the timeout context before proxying - When timeout fires, the HTTP request is immediately cancelled - Fix StopImmediately() to handle timeouts during model loading (StateStarting) - Add unit test to verify timeout behavior Before: requests would run for 60+ seconds despite requestTimeout: 20 After: requests terminate in exactly 20 seconds as configured Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add brief mention of requestTimeout feature in the customizable features section of README. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Feat web config
Feat timeout
…thcheck Feat conditional rpc healthcheck
Fix editor cleanup and improve dark mode appearance with better colors, contrast, and styling. - Add proper editor disposal in $effect cleanup - Update theme colors for better dark mode visibility - Improve button styling with teal export button - Better text contrast and subtle borders - Refine error message styling Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ui-svelte/src/routes/Config.svelte`:
- Around line 157-160: The error handling block using res.json() can itself
throw when the response is non-JSON; update the failure path that currently
reads "if (!res.ok) { const errData = await res.json(); ... }" to robustly parse
errors by attempting res.json() in a try/catch and, on parse failure, falling
back to await res.text(); construct and throw an Error that includes
response.status (and statusText) plus either the parsed JSON message
(errData.error || JSON.stringify(errData)) or the raw text body so non-JSON
error pages produce a clear message.
- Around line 17-21: The bug is that a single Compartment instance
(themeCompartment) is shared between both editors causing theme/readOnly config
to collide; create a second Compartment (e.g., exampleThemeCompartment) and use
one compartment per editor (one for editorView, one for exampleView), update the
theme effect to dispatch separate reconfigurations to each compartment (apply
the readOnly:false theme to the editorView's compartment and readOnly:true to
the exampleView's compartment), and modify createEditor calls to accept and
attach the appropriate compartment so each editor can be configured
independently.
Fix theme compartment sharing bug and improve error response handling. - create separate Compartment instances for each CodeMirror editor - update createEditor to accept compartment parameter - improve saveConfig error handling to parse both JSON and non-JSON responses - include status code and statusText in error messages
Fix cursor jumping to top after typing by preventing reactive effect from re-running on content changes. Use untrack() to read config state without creating reactive dependency, ensuring editor is only created once and not destroyed/recreated on each keystroke. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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 (1)
proxy/config/model_config.go (1)
40-64:⚠️ Potential issue | 🟡 MinorValidate
requestTimeoutis non-negative during unmarshalling.Negative values currently slip through and can lead to immediate/undefined timeout behavior despite schema minimums. Add a guard after unmarshal.
🛠️ Suggested fix
if err := unmarshal(&defaults); err != nil { return err } + + if defaults.RequestTimeout < 0 { + return errors.New("requestTimeout must be >= 0") + } *m = ModelConfig(defaults) return nil }
🤖 Fix all issues with AI agents
In `@docs/configuration.md`:
- Around line 75-82: The documentation table entry for the "hooks" feature uses
"event driven" but should be hyphenated as the compound adjective
"event-driven"; update the Description cell for the `hooks` row in the table
(the line containing the `hooks` feature) to read "event-driven functionality".
In `@proxy/config/config_test.go`:
- Around line 1377-1480: Rename the tests to follow the repository's config test
naming convention by adding a TestConfig_ prefix to each test that targets
ParseRPCEndpoints: rename TestParseRPCEndpoints_ValidFormats to
TestConfig_ParseRPCEndpoints_ValidFormats, TestParseRPCEndpoints_NoRPCFlag to
TestConfig_ParseRPCEndpoints_NoRPCFlag, TestParseRPCEndpoints_InvalidFormats to
TestConfig_ParseRPCEndpoints_InvalidFormats,
TestParseRPCEndpoints_EmptyEndpointsFiltered to
TestConfig_ParseRPCEndpoints_EmptyEndpointsFiltered, and
TestParseRPCEndpoints_MultilineCommand to
TestConfig_ParseRPCEndpoints_MultilineCommand so the tests remain clearly
associated with the ParseRPCEndpoints function and follow the TestConfig_*
convention.
In `@proxy/config/config.go`:
- Around line 545-565: The loop that parses RPC flags currently overwrites
endpoints and silently accepts flags with no value; update the logic so that
encountering "--rpc" or "-rpc" without a following value returns an error, and
when parsing "--rpc=..." or "-rpc=..." or the next arg value you append parsed
endpoints to the existing endpoints slice instead of replacing it; modify the
block that calls parseEndpointList (referenced here as parseEndpointList and the
endpoints variable) to append results (e.g., endpoints = append(endpoints,
parseEndpointList(... )...)) and return an explicit error when a flag is present
but has no value.
In `@proxy/process_timeout_test.go`:
- Around line 3-12: The calls to NewProcess in the tests are missing its
required shutdownCtx parameter; import the context package in
process_timeout_test.go and create/pass a context (e.g. context.Background() or
a cancellable ctx) as the shutdownCtx argument to each NewProcess invocation
(referencing NewProcess) where CI flagged errors (including the other call at
the second occurrence), ensuring the function signature matches by adding the
context import and supplying shutdownCtx to both calls.
🧹 Nitpick comments (1)
proxy/process.go (1)
91-135: Guard against a nil shutdownCtx in NewProcess.
context.WithCancel(nil)will panic if any caller accidentally passes nil.Proposed defensive default
func NewProcess(ID string, healthCheckTimeout int, modelConfig config.ModelConfig, processLogger *LogMonitor, proxyLogger *LogMonitor, shutdownCtx context.Context) *Process { + if shutdownCtx == nil { + shutdownCtx = context.Background() + } concurrentLimit := 10 if modelConfig.ConcurrencyLimit > 0 { concurrentLimit = modelConfig.ConcurrencyLimit }
Comment on lines
+75
to
+82
| | Feature | Description | | ||
| | ----------------- | ------------------------------------------------------- | | ||
| | `ttl` | automatic unloading of models after a timeout | | ||
| | `macros` | reusable snippets to use in configurations | | ||
| | `groups` | run multiple models at a time | | ||
| | `hooks` | event driven functionality | | ||
| | `env` | define environment variables per model | | ||
| | `aliases` | serve a model with different names | |
There was a problem hiding this comment.
Hyphenate the compound adjective (“event-driven”).
Minor doc polish.
Proposed edit
-| `hooks` | event driven functionality |
+| `hooks` | event-driven functionality |📝 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
| | Feature | Description | | |
| | ----------------- | ------------------------------------------------------- | | |
| | `ttl` | automatic unloading of models after a timeout | | |
| | `macros` | reusable snippets to use in configurations | | |
| | `groups` | run multiple models at a time | | |
| | `hooks` | event driven functionality | | |
| | `env` | define environment variables per model | | |
| | `aliases` | serve a model with different names | | |
| | Feature | Description | | |
| | ----------------- | ------------------------------------------------------- | | |
| | `ttl` | automatic unloading of models after a timeout | | |
| | `macros` | reusable snippets to use in configurations | | |
| | `groups` | run multiple models at a time | | |
| | `hooks` | event-driven functionality | | |
| | `env` | define environment variables per model | | |
| | `aliases` | serve a model with different names | |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~80-~80: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... | | hooks | event driven functionality ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🤖 Prompt for AI Agents
In `@docs/configuration.md` around lines 75 - 82, The documentation table entry
for the "hooks" feature uses "event driven" but should be hyphenated as the
compound adjective "event-driven"; update the Description cell for the `hooks`
row in the table (the line containing the `hooks` feature) to read "event-driven
functionality".
Comment on lines
+1377
to
+1480
| func TestParseRPCEndpoints_ValidFormats(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| cmd string | ||
| expected []string | ||
| }{ | ||
| { | ||
| name: "single endpoint with --rpc", | ||
| cmd: "llama-server --rpc localhost:50051 -ngl 99", | ||
| expected: []string{"localhost:50051"}, | ||
| }, | ||
| { | ||
| name: "single endpoint with --rpc=", | ||
| cmd: "llama-server --rpc=192.168.1.100:50051 -ngl 99", | ||
| expected: []string{"192.168.1.100:50051"}, | ||
| }, | ||
| { | ||
| name: "single endpoint with -rpc", | ||
| cmd: "llama-server -rpc localhost:50051 -ngl 99", | ||
| expected: []string{"localhost:50051"}, | ||
| }, | ||
| { | ||
| name: "single endpoint with -rpc=", | ||
| cmd: "llama-server -rpc=localhost:50051 -ngl 99", | ||
| expected: []string{"localhost:50051"}, | ||
| }, | ||
| { | ||
| name: "multiple endpoints comma-separated", | ||
| cmd: "llama-server --rpc 192.168.1.10:50051,192.168.1.11:50051 -ngl 99", | ||
| expected: []string{"192.168.1.10:50051", "192.168.1.11:50051"}, | ||
| }, | ||
| { | ||
| name: "multiple endpoints with spaces trimmed", | ||
| cmd: "llama-server --rpc '192.168.1.10:50051, 192.168.1.11:50051' -ngl 99", | ||
| expected: []string{"192.168.1.10:50051", "192.168.1.11:50051"}, | ||
| }, | ||
| { | ||
| name: "IPv6 endpoint", | ||
| cmd: "llama-server --rpc [::1]:50051 -ngl 99", | ||
| expected: []string{"[::1]:50051"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| endpoints, err := ParseRPCEndpoints(tt.cmd) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, tt.expected, endpoints) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestParseRPCEndpoints_NoRPCFlag(t *testing.T) { | ||
| cmd := "llama-server -ngl 99 -m model.gguf" | ||
| endpoints, err := ParseRPCEndpoints(cmd) | ||
| assert.NoError(t, err) | ||
| assert.Empty(t, endpoints) | ||
| } | ||
|
|
||
| func TestParseRPCEndpoints_InvalidFormats(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| cmd string | ||
| wantErr string | ||
| }{ | ||
| { | ||
| name: "missing port", | ||
| cmd: "llama-server --rpc localhost -ngl 99", | ||
| wantErr: "invalid RPC endpoint", | ||
| }, | ||
| { | ||
| name: "invalid host:port format", | ||
| cmd: "llama-server --rpc not-a-valid-endpoint -ngl 99", | ||
| wantErr: "invalid RPC endpoint", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| _, err := ParseRPCEndpoints(tt.cmd) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), tt.wantErr) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestParseRPCEndpoints_EmptyEndpointsFiltered(t *testing.T) { | ||
| // Empty strings after commas are filtered out | ||
| cmd := "llama-server --rpc 'localhost:50051,,' -ngl 99" | ||
| endpoints, err := ParseRPCEndpoints(cmd) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, []string{"localhost:50051"}, endpoints) | ||
| } | ||
|
|
||
| func TestParseRPCEndpoints_MultilineCommand(t *testing.T) { | ||
| cmd := `llama-server \ | ||
| --rpc localhost:50051 \ | ||
| -ngl 99 \ | ||
| -m model.gguf` | ||
|
|
||
| endpoints, err := ParseRPCEndpoints(cmd) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, []string{"localhost:50051"}, endpoints) | ||
| } |
There was a problem hiding this comment.
Rename ParseRPCEndpoints tests to follow TestConfig_* convention.
The new test names don’t follow the repository’s test naming pattern for config tests.
🛠️ Suggested rename
-func TestParseRPCEndpoints_ValidFormats(t *testing.T) {
+func TestConfig_ParseRPCEndpoints_ValidFormats(t *testing.T) {
@@
-func TestParseRPCEndpoints_NoRPCFlag(t *testing.T) {
+func TestConfig_ParseRPCEndpoints_NoRPCFlag(t *testing.T) {
@@
-func TestParseRPCEndpoints_InvalidFormats(t *testing.T) {
+func TestConfig_ParseRPCEndpoints_InvalidFormats(t *testing.T) {
@@
-func TestParseRPCEndpoints_EmptyEndpointsFiltered(t *testing.T) {
+func TestConfig_ParseRPCEndpoints_EmptyEndpointsFiltered(t *testing.T) {
@@
-func TestParseRPCEndpoints_MultilineCommand(t *testing.T) {
+func TestConfig_ParseRPCEndpoints_MultilineCommand(t *testing.T) {📝 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
| func TestParseRPCEndpoints_ValidFormats(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| cmd string | |
| expected []string | |
| }{ | |
| { | |
| name: "single endpoint with --rpc", | |
| cmd: "llama-server --rpc localhost:50051 -ngl 99", | |
| expected: []string{"localhost:50051"}, | |
| }, | |
| { | |
| name: "single endpoint with --rpc=", | |
| cmd: "llama-server --rpc=192.168.1.100:50051 -ngl 99", | |
| expected: []string{"192.168.1.100:50051"}, | |
| }, | |
| { | |
| name: "single endpoint with -rpc", | |
| cmd: "llama-server -rpc localhost:50051 -ngl 99", | |
| expected: []string{"localhost:50051"}, | |
| }, | |
| { | |
| name: "single endpoint with -rpc=", | |
| cmd: "llama-server -rpc=localhost:50051 -ngl 99", | |
| expected: []string{"localhost:50051"}, | |
| }, | |
| { | |
| name: "multiple endpoints comma-separated", | |
| cmd: "llama-server --rpc 192.168.1.10:50051,192.168.1.11:50051 -ngl 99", | |
| expected: []string{"192.168.1.10:50051", "192.168.1.11:50051"}, | |
| }, | |
| { | |
| name: "multiple endpoints with spaces trimmed", | |
| cmd: "llama-server --rpc '192.168.1.10:50051, 192.168.1.11:50051' -ngl 99", | |
| expected: []string{"192.168.1.10:50051", "192.168.1.11:50051"}, | |
| }, | |
| { | |
| name: "IPv6 endpoint", | |
| cmd: "llama-server --rpc [::1]:50051 -ngl 99", | |
| expected: []string{"[::1]:50051"}, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| endpoints, err := ParseRPCEndpoints(tt.cmd) | |
| assert.NoError(t, err) | |
| assert.Equal(t, tt.expected, endpoints) | |
| }) | |
| } | |
| } | |
| func TestParseRPCEndpoints_NoRPCFlag(t *testing.T) { | |
| cmd := "llama-server -ngl 99 -m model.gguf" | |
| endpoints, err := ParseRPCEndpoints(cmd) | |
| assert.NoError(t, err) | |
| assert.Empty(t, endpoints) | |
| } | |
| func TestParseRPCEndpoints_InvalidFormats(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| cmd string | |
| wantErr string | |
| }{ | |
| { | |
| name: "missing port", | |
| cmd: "llama-server --rpc localhost -ngl 99", | |
| wantErr: "invalid RPC endpoint", | |
| }, | |
| { | |
| name: "invalid host:port format", | |
| cmd: "llama-server --rpc not-a-valid-endpoint -ngl 99", | |
| wantErr: "invalid RPC endpoint", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| _, err := ParseRPCEndpoints(tt.cmd) | |
| assert.Error(t, err) | |
| assert.Contains(t, err.Error(), tt.wantErr) | |
| }) | |
| } | |
| } | |
| func TestParseRPCEndpoints_EmptyEndpointsFiltered(t *testing.T) { | |
| // Empty strings after commas are filtered out | |
| cmd := "llama-server --rpc 'localhost:50051,,' -ngl 99" | |
| endpoints, err := ParseRPCEndpoints(cmd) | |
| assert.NoError(t, err) | |
| assert.Equal(t, []string{"localhost:50051"}, endpoints) | |
| } | |
| func TestParseRPCEndpoints_MultilineCommand(t *testing.T) { | |
| cmd := `llama-server \ | |
| --rpc localhost:50051 \ | |
| -ngl 99 \ | |
| -m model.gguf` | |
| endpoints, err := ParseRPCEndpoints(cmd) | |
| assert.NoError(t, err) | |
| assert.Equal(t, []string{"localhost:50051"}, endpoints) | |
| } | |
| func TestConfig_ParseRPCEndpoints_ValidFormats(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| cmd string | |
| expected []string | |
| }{ | |
| { | |
| name: "single endpoint with --rpc", | |
| cmd: "llama-server --rpc localhost:50051 -ngl 99", | |
| expected: []string{"localhost:50051"}, | |
| }, | |
| { | |
| name: "single endpoint with --rpc=", | |
| cmd: "llama-server --rpc=192.168.1.100:50051 -ngl 99", | |
| expected: []string{"192.168.1.100:50051"}, | |
| }, | |
| { | |
| name: "single endpoint with -rpc", | |
| cmd: "llama-server -rpc localhost:50051 -ngl 99", | |
| expected: []string{"localhost:50051"}, | |
| }, | |
| { | |
| name: "single endpoint with -rpc=", | |
| cmd: "llama-server -rpc=localhost:50051 -ngl 99", | |
| expected: []string{"localhost:50051"}, | |
| }, | |
| { | |
| name: "multiple endpoints comma-separated", | |
| cmd: "llama-server --rpc 192.168.1.10:50051,192.168.1.11:50051 -ngl 99", | |
| expected: []string{"192.168.1.10:50051", "192.168.1.11:50051"}, | |
| }, | |
| { | |
| name: "multiple endpoints with spaces trimmed", | |
| cmd: "llama-server --rpc '192.168.1.10:50051, 192.168.1.11:50051' -ngl 99", | |
| expected: []string{"192.168.1.10:50051", "192.168.1.11:50051"}, | |
| }, | |
| { | |
| name: "IPv6 endpoint", | |
| cmd: "llama-server --rpc [::1]:50051 -ngl 99", | |
| expected: []string{"[::1]:50051"}, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| endpoints, err := ParseRPCEndpoints(tt.cmd) | |
| assert.NoError(t, err) | |
| assert.Equal(t, tt.expected, endpoints) | |
| }) | |
| } | |
| } | |
| func TestConfig_ParseRPCEndpoints_NoRPCFlag(t *testing.T) { | |
| cmd := "llama-server -ngl 99 -m model.gguf" | |
| endpoints, err := ParseRPCEndpoints(cmd) | |
| assert.NoError(t, err) | |
| assert.Empty(t, endpoints) | |
| } | |
| func TestConfig_ParseRPCEndpoints_InvalidFormats(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| cmd string | |
| wantErr string | |
| }{ | |
| { | |
| name: "missing port", | |
| cmd: "llama-server --rpc localhost -ngl 99", | |
| wantErr: "invalid RPC endpoint", | |
| }, | |
| { | |
| name: "invalid host:port format", | |
| cmd: "llama-server --rpc not-a-valid-endpoint -ngl 99", | |
| wantErr: "invalid RPC endpoint", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| _, err := ParseRPCEndpoints(tt.cmd) | |
| assert.Error(t, err) | |
| assert.Contains(t, err.Error(), tt.wantErr) | |
| }) | |
| } | |
| } | |
| func TestConfig_ParseRPCEndpoints_EmptyEndpointsFiltered(t *testing.T) { | |
| // Empty strings after commas are filtered out | |
| cmd := "llama-server --rpc 'localhost:50051,,' -ngl 99" | |
| endpoints, err := ParseRPCEndpoints(cmd) | |
| assert.NoError(t, err) | |
| assert.Equal(t, []string{"localhost:50051"}, endpoints) | |
| } | |
| func TestConfig_ParseRPCEndpoints_MultilineCommand(t *testing.T) { | |
| cmd := `llama-server \ | |
| --rpc localhost:50051 \ | |
| -ngl 99 \ | |
| -m model.gguf` | |
| endpoints, err := ParseRPCEndpoints(cmd) | |
| assert.NoError(t, err) | |
| assert.Equal(t, []string{"localhost:50051"}, endpoints) | |
| } |
🤖 Prompt for AI Agents
In `@proxy/config/config_test.go` around lines 1377 - 1480, Rename the tests to
follow the repository's config test naming convention by adding a TestConfig_
prefix to each test that targets ParseRPCEndpoints: rename
TestParseRPCEndpoints_ValidFormats to TestConfig_ParseRPCEndpoints_ValidFormats,
TestParseRPCEndpoints_NoRPCFlag to TestConfig_ParseRPCEndpoints_NoRPCFlag,
TestParseRPCEndpoints_InvalidFormats to
TestConfig_ParseRPCEndpoints_InvalidFormats,
TestParseRPCEndpoints_EmptyEndpointsFiltered to
TestConfig_ParseRPCEndpoints_EmptyEndpointsFiltered, and
TestParseRPCEndpoints_MultilineCommand to
TestConfig_ParseRPCEndpoints_MultilineCommand so the tests remain clearly
associated with the ParseRPCEndpoints function and follow the TestConfig_*
convention.
Comment on lines
+545
to
+565
| var endpoints []string | ||
| for i, arg := range args { | ||
| if arg == "--rpc" || arg == "-rpc" { | ||
| if i+1 < len(args) { | ||
| endpoints = parseEndpointList(args[i+1]) | ||
| } | ||
| } else if strings.HasPrefix(arg, "--rpc=") { | ||
| endpoints = parseEndpointList(strings.TrimPrefix(arg, "--rpc=")) | ||
| } else if strings.HasPrefix(arg, "-rpc=") { | ||
| endpoints = parseEndpointList(strings.TrimPrefix(arg, "-rpc=")) | ||
| } | ||
| } | ||
|
|
||
| // Validate each endpoint | ||
| for _, ep := range endpoints { | ||
| if _, _, err := net.SplitHostPort(ep); err != nil { | ||
| return nil, fmt.Errorf("invalid RPC endpoint %q: %w", ep, err) | ||
| } | ||
| } | ||
|
|
||
| return endpoints, nil |
There was a problem hiding this comment.
Fail fast when --rpc/-rpc has no value (and avoid silent overwrite).
If a flag is present without a value, the function currently returns an empty list. Also, repeated flags overwrite earlier endpoints. Consider erroring on missing values and appending endpoints.
🛠️ Suggested fix
for i, arg := range args {
if arg == "--rpc" || arg == "-rpc" {
- if i+1 < len(args) {
- endpoints = parseEndpointList(args[i+1])
- }
+ if i+1 >= len(args) {
+ return nil, fmt.Errorf("rpc flag requires host:port value")
+ }
+ endpoints = append(endpoints, parseEndpointList(args[i+1])...)
} else if strings.HasPrefix(arg, "--rpc=") {
- endpoints = parseEndpointList(strings.TrimPrefix(arg, "--rpc="))
+ endpoints = append(endpoints, parseEndpointList(strings.TrimPrefix(arg, "--rpc="))...)
} else if strings.HasPrefix(arg, "-rpc=") {
- endpoints = parseEndpointList(strings.TrimPrefix(arg, "-rpc="))
+ endpoints = append(endpoints, parseEndpointList(strings.TrimPrefix(arg, "-rpc="))...)
}
}📝 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
| var endpoints []string | |
| for i, arg := range args { | |
| if arg == "--rpc" || arg == "-rpc" { | |
| if i+1 < len(args) { | |
| endpoints = parseEndpointList(args[i+1]) | |
| } | |
| } else if strings.HasPrefix(arg, "--rpc=") { | |
| endpoints = parseEndpointList(strings.TrimPrefix(arg, "--rpc=")) | |
| } else if strings.HasPrefix(arg, "-rpc=") { | |
| endpoints = parseEndpointList(strings.TrimPrefix(arg, "-rpc=")) | |
| } | |
| } | |
| // Validate each endpoint | |
| for _, ep := range endpoints { | |
| if _, _, err := net.SplitHostPort(ep); err != nil { | |
| return nil, fmt.Errorf("invalid RPC endpoint %q: %w", ep, err) | |
| } | |
| } | |
| return endpoints, nil | |
| var endpoints []string | |
| for i, arg := range args { | |
| if arg == "--rpc" || arg == "-rpc" { | |
| if i+1 >= len(args) { | |
| return nil, fmt.Errorf("rpc flag requires host:port value") | |
| } | |
| endpoints = append(endpoints, parseEndpointList(args[i+1])...) | |
| } else if strings.HasPrefix(arg, "--rpc=") { | |
| endpoints = append(endpoints, parseEndpointList(strings.TrimPrefix(arg, "--rpc="))...) | |
| } else if strings.HasPrefix(arg, "-rpc=") { | |
| endpoints = append(endpoints, parseEndpointList(strings.TrimPrefix(arg, "-rpc="))...) | |
| } | |
| } | |
| // Validate each endpoint | |
| for _, ep := range endpoints { | |
| if _, _, err := net.SplitHostPort(ep); err != nil { | |
| return nil, fmt.Errorf("invalid RPC endpoint %q: %w", ep, err) | |
| } | |
| } | |
| return endpoints, nil |
🤖 Prompt for AI Agents
In `@proxy/config/config.go` around lines 545 - 565, The loop that parses RPC
flags currently overwrites endpoints and silently accepts flags with no value;
update the logic so that encountering "--rpc" or "-rpc" without a following
value returns an error, and when parsing "--rpc=..." or "-rpc=..." or the next
arg value you append parsed endpoints to the existing endpoints slice instead of
replacing it; modify the block that calls parseEndpointList (referenced here as
parseEndpointList and the endpoints variable) to append results (e.g., endpoints
= append(endpoints, parseEndpointList(... )...)) and return an explicit error
when a flag is present but has no value.
Comment on lines
+3
to
+12
| import ( | ||
| "fmt" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "sync" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/mostlygeek/llama-swap/proxy/config" | ||
| ) |
There was a problem hiding this comment.
Fix compile error: missing shutdownCtx argument in NewProcess.
CI fails with “not enough arguments in call to NewProcess”.
Proposed fix
import (
+ "context"
"fmt"
"net/http"
"net/http/httptest"
"sync"
"testing"
"time"
"github.com/mostlygeek/llama-swap/proxy/config"
)
@@
- p := NewProcess("test-timeout", 30, cfg, processLogger, proxyLogger)
+ p := NewProcess("test-timeout", 30, cfg, processLogger, proxyLogger, context.Background())Also applies to: 56-57
🤖 Prompt for AI Agents
In `@proxy/process_timeout_test.go` around lines 3 - 12, The calls to NewProcess
in the tests are missing its required shutdownCtx parameter; import the context
package in process_timeout_test.go and create/pass a context (e.g.
context.Background() or a cancellable ctx) as the shutdownCtx argument to each
NewProcess invocation (referencing NewProcess) where CI flagged errors
(including the other call at the second occurrence), ensuring the function
signature matches by adding the context import and supplying shutdownCtx to both
calls.
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
1 participant
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.
Implemented a tab in the web ui which allows direct editing, import/export of the config.yaml. Some references and the config.yaml.example are provided at the side panel.
I am aware of the potential issues of command injection by pasting arbitrary file content. This is tackled by yaml validation and the recommendation this should not be run publicly anyways.
Love this project! Will replace my ollama instance which improved to be somewhat restrictive and dont utilize many features of llama.cpp.
Summary by CodeRabbit
New Features
Behavior
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.