From a906cd459b2bc294ab341e7cec38333d4a2cbe34 Mon Sep 17 00:00:00 2001 From: Yathi <511386+yuth@users.noreply.github.com> Date: Tue, 15 Jul 2025 10:14:16 -0700 Subject: [PATCH] Strip comments before macro expansion in config (#193) A bug fix that ensures comments don't interfere with macro expansion by removing them first. This prevents unwanted comment text from appearing in the final expanded command. Co-authored-by: Yathiraj Bollimbala G --- proxy/config.go | 17 +++++++ proxy/config_test.go | 115 ++++++++++++++++++++++++++++++++++++++++++ proxy/process.go | 6 +-- proxy/process_test.go | 2 +- 4 files changed, 136 insertions(+), 4 deletions(-) diff --git a/proxy/config.go b/proxy/config.go index 9a6dba9..e019422 100644 --- a/proxy/config.go +++ b/proxy/config.go @@ -255,6 +255,10 @@ func LoadConfigFromReader(r io.Reader) (Config, error) { for _, modelId := range modelIds { modelConfig := config.Models[modelId] + // Strip comments from command fields before macro expansion + modelConfig.Cmd = StripComments(modelConfig.Cmd) + modelConfig.CmdStop = StripComments(modelConfig.CmdStop) + // go through model config fields: cmd, cmdStop, proxy, checkEndPoint and replace macros with macro values for macroName, macroValue := range config.Macros { macroSlug := fmt.Sprintf("${%s}", macroName) @@ -406,3 +410,16 @@ func SanitizeCommand(cmdStr string) ([]string, error) { return args, nil } + +func StripComments(cmdStr string) string { + var cleanedLines []string + for _, line := range strings.Split(cmdStr, "\n") { + trimmed := strings.TrimSpace(line) + // Skip comment lines + if strings.HasPrefix(trimmed, "#") { + continue + } + cleanedLines = append(cleanedLines, line) + } + return strings.Join(cleanedLines, "\n") +} diff --git a/proxy/config_test.go b/proxy/config_test.go index 70a1d2d..a5b4611 100644 --- a/proxy/config_test.go +++ b/proxy/config_test.go @@ -1,6 +1,7 @@ package proxy import ( + "slices" "strings" "testing" @@ -325,3 +326,117 @@ models: assert.Equal(t, []string{"temperature", "top_k", "top_p"}, sanitized) } } + +func TestStripComments(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "no comments", + input: "echo hello\necho world", + expected: "echo hello\necho world", + }, + { + name: "single comment line", + input: "# this is a comment\necho hello", + expected: "echo hello", + }, + { + name: "multiple comment lines", + input: "# comment 1\necho hello\n# comment 2\necho world", + expected: "echo hello\necho world", + }, + { + name: "comment with spaces", + input: " # indented comment\necho hello", + expected: "echo hello", + }, + { + name: "empty lines preserved", + input: "echo hello\n\necho world", + expected: "echo hello\n\necho world", + }, + { + name: "only comments", + input: "# comment 1\n# comment 2", + expected: "", + }, + { + name: "empty string", + input: "", + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := StripComments(tt.input) + if result != tt.expected { + t.Errorf("StripComments() = %q, expected %q", result, tt.expected) + } + }) + } +} + +func TestConfig_MacroInCommentStrippedBeforeExpansion(t *testing.T) { + // Test case that reproduces the original bug where a macro in a comment + // would get expanded and cause the comment text to be included in the command + content := ` +startPort: 9990 +macros: + "latest-llama": > + /user/llama.cpp/build/bin/llama-server + --port ${PORT} + +models: + "test-model": + cmd: | + # ${latest-llama} is a macro that is defined above + ${latest-llama} + --model /path/to/model.gguf + -ngl 99 +` + + config, err := LoadConfigFromReader(strings.NewReader(content)) + assert.NoError(t, err) + + // Get the sanitized command + sanitizedCmd, err := SanitizeCommand(config.Models["test-model"].Cmd) + assert.NoError(t, err) + + // Join the command for easier inspection + cmdStr := strings.Join(sanitizedCmd, " ") + + // Verify that comment text is NOT present in the final command as separate arguments + commentWords := []string{"is", "macro", "that", "defined", "above"} + for _, word := range commentWords { + found := slices.Contains(sanitizedCmd, word) + assert.False(t, found, "Comment text '%s' should not be present as a separate argument in final command", word) + } + + // Verify that the actual command components ARE present + expectedParts := []string{ + "/user/llama.cpp/build/bin/llama-server", + "--port", + "9990", + "--model", + "/path/to/model.gguf", + "-ngl", + "99", + } + + for _, part := range expectedParts { + assert.Contains(t, cmdStr, part, "Expected command part '%s' not found in final command", part) + } + + // Verify the server path appears exactly once (not duplicated due to macro expansion) + serverPath := "/user/llama.cpp/build/bin/llama-server" + count := strings.Count(cmdStr, serverPath) + assert.Equal(t, 1, count, "Expected exactly 1 occurrence of server path, found %d", count) + + // Verify the expected final command structure + expectedCmd := "/user/llama.cpp/build/bin/llama-server --port 9990 --model /path/to/model.gguf -ngl 99" + assert.Equal(t, expectedCmd, cmdStr, "Final command does not match expected structure") +} diff --git a/proxy/process.go b/proxy/process.go index c8e84ed..e42524a 100644 --- a/proxy/process.go +++ b/proxy/process.go @@ -212,11 +212,11 @@ func (p *Process) start() error { if curState, swapErr := p.swapState(StateStarting, StateStopped); swapErr != nil { p.state = StateStopped // force it into a stopped state return fmt.Errorf( - "failed to start command and state swap failed. command error: %v, current state: %v, state swap error: %v", - err, curState, swapErr, + "failed to start command '%s' and state swap failed. command error: %v, current state: %v, state swap error: %v", + strings.Join(args, " "), err, curState, swapErr, ) } - return fmt.Errorf("start() failed: %v", err) + return fmt.Errorf("start() failed for command '%s': %v", strings.Join(args, " "), err) } // Capture the exit error for later signalling diff --git a/proxy/process_test.go b/proxy/process_test.go index 50a8c15..da4a380 100644 --- a/proxy/process_test.go +++ b/proxy/process_test.go @@ -107,7 +107,7 @@ func TestProcess_BrokenModelConfig(t *testing.T) { w = httptest.NewRecorder() process.ProxyRequest(w, req) assert.Equal(t, http.StatusBadGateway, w.Code) - assert.Contains(t, w.Body.String(), "start() failed: ") + assert.Contains(t, w.Body.String(), "start() failed for command 'nonexistent-command':") } func TestProcess_UnloadAfterTTL(t *testing.T) {