Skip to content

proxy: fix path bug in /logs/stream/{model_id}#431

Merged
mostlygeek merged 1 commit intomainfrom
path-bug-issue-421
Dec 22, 2025
Merged

proxy: fix path bug in /logs/stream/{model_id}#431
mostlygeek merged 1 commit intomainfrom
path-bug-issue-421

Conversation

@mostlygeek
Copy link
Copy Markdown
Owner

@mostlygeek mostlygeek commented Dec 22, 2025

A {model_id} containing a forward slash trips up gin's path param parsing. This updates /logs/stream to work like /upstream where the model_id is built up in parts and searched for in the configuration.

Updates #421

Summary by CodeRabbit

Release Notes

  • New Features

    • Model identifiers can now contain slashes, enabling hierarchical naming conventions (e.g., "author/model")
    • Automatic redirects now supported for upstream paths that map directly to models
  • Bug Fixes

    • Enhanced log stream endpoint path parameter handling
    • Clarified error messages when invalid loggers are specified

✏️ Tip: You can customize this high-level summary in your review settings.

A {model_id} containing a forward slash trips up gin's path param
parsing. This updates /logs/stream to work like /upstream where the
model_id is built up in parts and searched for in the configuration.

Updates #421
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Dec 22, 2025

Walkthrough

The changes introduce a findModelInPath helper function to parse model names from slash-delimited paths and refactor request routing to use this helper. Additionally, redirect behavior is added for requests mapping directly to models, and support is extended for model IDs containing slashes.

Changes

Cohort / File(s) Summary
Core proxy routing refactoring
proxy/proxymanager.go
Introduces findModelInPath helper to iteratively locate model names in paths; refactors proxyToUpstream to use this helper; adds 308/301 redirect logic when upstream path directly maps to a model without extra components; updates route parameter from /logs/stream/:logMonitorID to /logs/stream/*logMonitorID (wildcard).
Log handler integration
proxy/proxymanager_loghandlers.go
Normalizes logMonitorID by stripping leading slash; updates getLogger to use findModelInPath instead of RealModelName lookup, enabling support for model IDs with slashes; updates error message to reference model ID option.
Test updates
proxy/proxymanager_test.go
Adds "author/model" test model to default configuration; extends expected streaming endpoints list to include /logs/stream/author/model path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Path parsing logic in findModelInPath: Verify iterative parsing correctly handles models with slashes and edge cases
  • Redirect behavior: Confirm 308/301 status code selection by HTTP method is correct; validate URL reconstruction and query string preservation
  • Log handler model resolution: Ensure findModelInPath integration correctly resolves slash-containing model IDs without breaking existing lookup patterns

Possibly related PRs

Suggested labels

bug

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main bug fix: updating path parsing in /logs/stream/{model_id} to handle model IDs with forward slashes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch path-bug-issue-421

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3f329f and be4f1d8.

📒 Files selected for processing (3)
  • proxy/proxymanager.go
  • proxy/proxymanager_loghandlers.go
  • proxy/proxymanager_test.go
🧰 Additional context used
📓 Path-based instructions (1)
proxy/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

Run make test-dev when making iterative changes to code under the proxy/ directory - this runs go test and staticcheck, and all static checking errors must be fixed

Files:

  • proxy/proxymanager_test.go
  • proxy/proxymanager_loghandlers.go
  • proxy/proxymanager.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 371
File: proxy/process.go:0-0
Timestamp: 2025-10-29T05:26:34.964Z
Learning: In proxy/process.go, the loading message "llama-swap loading model: {name}" intentionally uses p.ID (Process.ID) rather than the realModelName from the request context. This is the correct design choice.
📚 Learning: 2025-11-25T05:40:06.444Z
Learnt from: CR
Repo: mostlygeek/llama-swap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T05:40:06.444Z
Learning: Applies to proxy/**/*.go : Run `make test-dev` when making iterative changes to code under the `proxy/` directory - this runs `go test` and `staticcheck`, and all static checking errors must be fixed

Applied to files:

  • proxy/proxymanager_test.go
  • proxy/proxymanager.go
📚 Learning: 2025-10-29T05:26:34.964Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 371
File: proxy/process.go:0-0
Timestamp: 2025-10-29T05:26:34.964Z
Learning: In proxy/process.go, the loading message "llama-swap loading model: {name}" intentionally uses p.ID (Process.ID) rather than the realModelName from the request context. This is the correct design choice.

Applied to files:

  • proxy/proxymanager_loghandlers.go
  • proxy/proxymanager.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: run-tests
  • GitHub Check: run-tests
🔇 Additional comments (7)
proxy/proxymanager_loghandlers.go (2)

34-34: LGTM - Correct handling of wildcard parameter.

The TrimPrefix is necessary because the wildcard route parameter *logMonitorID includes the leading slash.


95-105: LGTM - Proper integration of path parsing helper.

The findModelInPath integration correctly handles model IDs containing slashes, and the updated error message accurately reflects the new capability.

proxy/proxymanager.go (4)

269-269: LGTM - Route pattern supports multi-segment model IDs.

Changing from :logMonitorID to *logMonitorID correctly enables wildcard matching for model IDs containing slashes.


469-494: LGTM - Robust path parsing implementation.

The findModelInPath helper correctly iterates through path segments to find model IDs containing slashes. The logic properly handles edge cases including exact matches (returning "/" as remainingPath) and no matches (returning empty values with found=false).


496-522: LGTM - Proper path parsing and redirect behavior.

The refactored function correctly uses findModelInPath to handle model IDs with slashes. The redirect logic (lines 506-522) ensures that requests to exact model paths without a trailing slash are redirected to the trailing-slash version, using appropriate HTTP status codes (308 for non-GET/HEAD to preserve method, 301 for GET/HEAD).


524-548: LGTM - Proxy logic preserved with enhanced path handling.

The existing swap and proxy logic is correctly integrated with the new path parsing, using the resolved modelName (real model name) for group swapping and remainingPath for upstream request rewriting.

proxy/proxymanager_test.go (1)

1081-1082: LGTM - Appropriate test coverage for multi-segment model IDs.

The test correctly adds a model with a slash in its ID ("author/model") and verifies that the corresponding log streaming endpoint returns the expected no-buffering header, ensuring the new path parsing logic works correctly.

Also applies to: 1095-1095


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

Comment @coderabbitai help to get the list of available commands and usage tips.

@mostlygeek mostlygeek merged commit e6a9e21 into main Dec 22, 2025
3 checks passed
@mostlygeek mostlygeek deleted the path-bug-issue-421 branch December 22, 2025 05:47
rohitpaul pushed a commit to rohitpaul/llama-swap that referenced this pull request Mar 29, 2026
A {model_id} containing a forward slash trips up gin's path param
parsing. This updates /logs/stream to work like /upstream where the
model_id is built up in parts and searched for in the configuration.

Updates mostlygeek#421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant