router: match longest entrypoint path prefix#389
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @avinxshKD! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
![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 refactors the upstream URL determination logic to match the longest path prefix among sandbox entrypoints and introduces a helper function for path boundary matching, along with unit tests. The review feedback suggests improving the robustness of the path matching logic by normalizing leading and trailing slashes, and adding a defensive nil check for the sandbox parameter to prevent potential nil pointer dereferences.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func determineUpstreamURL(sandbox *types.SandboxInfo, path string) (*url.URL, error) { | ||
| // prefer matched entrypoint by path | ||
| var matched types.SandboxEntryPoint | ||
| found := false |
There was a problem hiding this comment.
To prevent potential nil pointer dereferences, it is recommended to add a defensive nil check for the sandbox parameter at the beginning of determineUpstreamURL.
func determineUpstreamURL(sandbox *types.SandboxInfo, path string) (*url.URL, error) {
if sandbox == nil {
return nil, fmt.Errorf("sandbox info is nil")
}
var matched types.SandboxEntryPoint
found := falseThere was a problem hiding this comment.
Leaving this out for now. determineUpstreamURL is only called after session lookup returns a sandbox, so a nil guard here feels unrelated to the path matching fix.
|
Hey @LiZhenCheng9527 @hzxuzhonghu checked this while looking at the router path selection logic. Current code picks the first HasPrefix match, so /can steal /api/foo depending on spec order. Also /api2 matches /api. Pls take a look when free. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
===========================================
+ Coverage 47.57% 58.12% +10.55%
===========================================
Files 30 34 +4
Lines 2819 3193 +374
===========================================
+ Hits 1341 1856 +515
+ Misses 1338 1153 -185
- Partials 140 184 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
avinxshKD
force-pushed
the
fix/router-longest-path-prefix
branch
from
acb3961 to
9b99ab4
Compare
3 participants
/kind bug
What this PR does / why we need it:
Fixes router entrypoint selection when multiple
pathPrefixvalues overlap.The router was picking the first prefix match, so a runtime with
/before/apicould route/api/footo the/entrypoint. It also treated/api2as matching/api.This changes the selection to use the longest valid prefix match and keeps path boundaries sane.
Which issue(s) this PR fixes:
Fixes #388
Special notes for your reviewer:
Added focused tests for overlapping prefixes and
/api2boundary behavior.Does this PR introduce a user-facing change?: