Fix: Strip comments before macro expansion to prevent command parsing errors

[yuth]

Problem

Comments containing macros in YAML configuration caused process startup failures with errors like:

unable to start process: start() failed: exec: "is": executable file not found in $PATH

Example Configuration That Failed

macros:
  "latest-llama": >
    /user/llama.cpp/build/bin/llama-server
    --port ${PORT}

models:
  "qwen3-30b-a3b":
    cmd: |
      # ${latest-llama} is a macro that is defined above
      ${latest-llama}
      --model /path/to/model.gguf
      -ngl 99

Root Cause

The issue was caused by order of operations:

1. Macro expansion happened first: ${latest-llama} in the comment was expanded, which could make the comment broken into multiple lines after expansion. The comment start character was present only on the first line. The effective command would look as below

models:
  "qwen3-30b-a3b":
    cmd: |
      #  /user/llama.cpp/build/bin/llama-server
       --port ${PORT}
       is a macro that is defined above
      /user/llama.cpp/build/bin/llama-server
       --port ${PORT}
      --model /path/to/model.gguf
      -ngl 99

3. Comment removal happened second: SanitizeCommand() only removes lines that start with #, so the expanded comment text remained
4. Command execution failed: Comment words like "is", "a", "macro" became command arguments, causing the shell to try executing "is" as a command

Solution

Reversed the order: Strip comments before macro expansion.

• Strip comments from cmd and cmdStop fields before macro processing

Result

Same configuration now produces clean command:

/user/llama.cpp/build/bin/llama-server --port 5800 --model /path/to/model.gguf -ngl 99

Testing

• Added test TestConfig_MacroInCommentStrippedBeforeExpansion that reproduces the original bug scenario
• Verifies comment text doesn't appear as command arguments
• All existing tests pass

Impact

• Fixes critical bug: Users can safely use comments that reference macros
• Backward compatible: All existing configurations continue to work
• No breaking changes: Pure bug fix

---

fixes #190

Summary by CodeRabbit

• Bug Fixes

  • Improved error messages to include the full command string when a process fails to start, providing clearer context in error outputs.

• Tests

  • Added tests to verify that comment lines are correctly stripped from command configurations and that macros in comments are not expanded.
  • Refined existing tests to check for more specific error messages, ensuring better validation of failure scenarios.

[coderabbitai]

Walkthrough

A new function, StripComments, is introduced to remove comment lines (lines starting with #) from model command strings before macro expansion in configuration loading. This ensures that comments do not interfere with command parsing or macro expansion. Error messages in process startup now include the full command string, and related tests are updated and expanded.

Changes

File(s)	Change Summary
proxy/config.go	Added StripComments function; applied it to Cmd and CmdStop before macro expansion.
proxy/config_test.go	Added tests for StripComments and for macro expansion behavior with comments in commands.
proxy/process.go	Enhanced error messages in start() to include the full command string.
proxy/process_test.go	Updated error message assertion to expect command string in failure output.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant ConfigLoader
    participant MacroExpander

    User->>ConfigLoader: Load config with command string
    ConfigLoader->>ConfigLoader: StripComments(command string)
    ConfigLoader->>MacroExpander: Expand macros in stripped command
    MacroExpander-->>ConfigLoader: Expanded command
    ConfigLoader-->>User: Config object with sanitized command

Loading

Assessment against linked issues

Objective	Addressed	Explanation
Error message for failed process start should include the command that failed (#190)	✅
Comments in command strings should not be interpreted as commands or macros (#190)	✅
Macro expansion should not occur within comment lines in command strings (#190)	✅
Model should not fail to start due to comment lines in config (#190)	✅

Possibly related PRs

• Add macros to Configuration schema #149: Implements macro substitution mechanisms; this PR builds on that by ensuring comments are stripped before macro expansion.
• Fix #133 SanitizeCommand removes comments #134: Modifies command sanitization to strip comment lines; related as both address comment removal but at different stages.

Suggested labels

bug, configuration

Suggested reviewers

• mostlygeek

---

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 78b2bc3 and f826930.

📒 Files selected for processing (4)

• proxy/config.go (2 hunks)
• proxy/config_test.go (2 hunks)
• proxy/process.go (1 hunks)
• proxy/process_test.go (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

proxy/process_test.go (1)

Learnt from: mostlygeek
PR: mostlygeek/llama-swap#155
File: proxy/process.go:359-372
Timestamp: 2025-06-05T22:57:53.793Z
Learning: In Go, cmdWaitChan in proxy/process.go is a signal channel (chan struct{}) that follows the pattern where waitForCmd() closes the channel when cmd.Wait() returns, and stopCommand() waits on it. Reading from a closed channel returns immediately, so <-cmdWaitChan won't block indefinitely.

🔇 Additional comments (7)

proxy/config.go (2)

258-261: LGTM! Correct fix for the macro expansion bug.

The fix appropriately strips comments from command fields before macro expansion, which prevents the reported issue where expanded macros inside comments would be treated as executable commands.

---

414-425: LGTM! Well-implemented comment stripping function.

The StripComments function correctly:

• Handles line-by-line processing
• Properly detects comment lines (including those with leading whitespace)
• Preserves non-comment lines including empty lines
• Returns a clean multiline string

proxy/process.go (1)

214-220: LGTM! Enhanced error messages improve debugging.

The inclusion of the full command string in error messages will significantly help users identify which specific command failed during process startup. This is a valuable improvement for troubleshooting.

proxy/process_test.go (1)

110-110: LGTM! Test assertion correctly updated for enhanced error messages.

The updated assertion properly validates that the error message includes the specific command that failed, which aligns with the enhanced error reporting in the process module.

proxy/config_test.go (3)

4-4: LGTM! Import added for new test functionality.

The slices import is appropriately added to support the slices.Contains function used in the new test.

---

330-381: LGTM! Comprehensive test coverage for StripComments function.

The test cases cover all important scenarios:

• No comments
• Single and multiple comment lines
• Indented comments
• Empty lines preservation
• Edge cases (only comments, empty string)

The test implementation is thorough and validates the function's behavior correctly.

---

383-442: LGTM! Excellent integration test that reproduces the original bug scenario.

This test effectively:

• Reproduces the exact scenario that caused the original bug
• Validates that comment text is NOT present as separate arguments
• Verifies that actual command components ARE present
• Ensures no duplication of macro-expanded content
• Confirms the final command structure is correct

The test provides strong confidence that the fix works as intended and prevents regression.

✨ Finishing Touches

• 📝 Generate Docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[mostlygeek]

Thanks for submitting this, looks good!