Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough引入 provider 范围代理路由：新增 ProviderProxyHandler 并在启动/组件初始化中注入；项目/提供商范围路径解析与路由分发更新；流上下文与 Router.Match 中传递 provider id；静态/根路由注册与多项单元/端到端测试扩展以覆盖 provider 范围行为。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Root as Root\ Handler
participant ProviderPH as Provider\Proxy\Handler
participant ProviderRepo as Provider\ Repository
participant Proxy as Proxy\ Handler
participant Models as Models\ Handler
participant Router as Router
participant Backend as Backend\ Service
Client->>Root: HTTP /provider/123/v1/messages
Root->>ProviderPH: delegate (isProviderProxyPath)
ProviderPH->>ProviderPH: parseScopedPath -> id="123", apiPath="/v1/messages"
ProviderPH->>ProviderRepo: GetByID(tenantID, 123)
ProviderRepo-->>ProviderPH: provider
ProviderPH->>ProviderPH: set header X-Maxx-Provider-ID, rewrite path
alt apiPath == "/v1/models"
ProviderPH->>Models: ServeHTTP(rewritten)
else
ProviderPH->>Proxy: ServeHTTP(rewritten)
Proxy->>Proxy: ingress reads X-Maxx-Provider-ID -> flow.KeyProviderID
Proxy->>Router: Match(ctx with ProviderID)
Router-->>Proxy: matched provider-scoped route
Proxy->>Backend: forward proxied request
Backend-->>Proxy: response
end
Proxy-->>Client: proxied response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
|
Test results for provider-scoped proxy routing:
Playwright evidence (requests UI after hitting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/handler/project_proxy.go`:
- Around line 110-129: The isValidAPIPath whitelist is too permissive because it
uses strings.HasPrefix and will accept names like "/v1/messages-debug" or
"/v1/responsesXYZ"; update the logic in isValidAPIPath to accept either exact
matches (e.g., "/v1/models") or a prefix followed by "/" for subpaths (e.g.,
"/v1/messages" OR "/v1/messages/") so only intended endpoints pass; ensure this
aligns with how ServeHTTP dispatches to modelsHandler and proxyHandler so that
exact "/v1/models" routes to modelsHandler while "/v1/models/..." is allowed
only when intended.
In `@internal/handler/proxy.go`:
- Around line 186-193: The code silently falls back to providerID=0 when
X-Maxx-Provider-ID is present but invalid; update the handler around the
r.Header.Get("X-Maxx-Provider-ID")/strconv.ParseUint logic so that if the header
exists but ParseUint returns an error or the parsed pid == 0 you immediately
write a 400 response and return (stop processing) instead of continuing with
providerID=0; reference the providerID variable, the X-Maxx-Provider-ID header
access, and the strconv.ParseUint call to locate and change the behavior (apply
same check at the other occurrence noted).
In `@internal/router/router.go`:
- Line 117: providerID is read into providerID (ctx.ProviderID) but only applied
in the global-route branch; when useProjectRoutes is true and ctx.ProjectID != 0
the project-specific branch currently ignores provider scope and can match other
providers and block the global provider filter. Fix by adding providerID-aware
filtering in the project-route branch (the code that builds/matches project
routes when useProjectRoutes is true): if providerID != 0 ensure the route’s
provider parameter matches providerID (or skip that route) instead of returning
a match that belongs to a different provider; also adjust control flow so a
provider-mismatch in the project branch does not short-circuit the global
fallback (i.e., skip/continue rather than return), and apply the same providerID
check logic you use in the global branch to the project-route matching code
paths referenced by providerID, useProjectRoutes, and ctx.ProjectID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26afddb3-c7ac-48ca-872f-e70e804e793f
📒 Files selected for processing (15)
cmd/maxx/main.gointernal/core/database.gointernal/executor/flow_state.gointernal/executor/middleware_ingress.gointernal/executor/middleware_route_match.gointernal/flow/keys.gointernal/handler/project_proxy.gointernal/handler/project_proxy_test.gointernal/handler/proxy.gointernal/handler/static.gointernal/router/router.gotests/e2e/playwright/package.jsontests/e2e/playwright/provider-proxy-route.spec.tstests/e2e/provider_proxy_test.gotests/e2e/proxy_setup_test.go
📜 Review details
⏰ 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: e2e
- GitHub Check: playwright
🔇 Additional comments (7)
internal/executor/flow_state.go (1)
22-22: 变更合理，作用域信息承载清晰。为执行态补充
providerID字段是必要且一致的扩展，便于后续路由匹配传递上下文。internal/flow/keys.go (1)
13-13: 上下文键新增正确。
KeyProviderID的引入与现有 flow key 体系保持一致，便于作用域信息在链路中传递。internal/executor/middleware_route_match.go (1)
27-27: 路由匹配上下文透传完整。在匹配上下文中补充
ProviderID是本次功能的关键连接点，实现上简洁且位置正确。internal/executor/middleware_ingress.go (1)
39-43: Ingress 状态注入实现正确。
flow -> execState的 provider 透传逻辑清晰，和现有projectID注入方式一致。tests/e2e/proxy_setup_test.go (1)
197-197: E2E 路由装配到位。测试环境已正确接入作用域代理入口，能有效覆盖
/provider/{id}/...与/project/{slug}/...场景。Also applies to: 220-221
tests/e2e/playwright/package.json (1)
14-14: 脚本补充准确。新增命令可直接触发 provider 作用域的 Playwright 用例，便于 CI/本地定向回归。
internal/handler/project_proxy_test.go (1)
5-20: 单测覆盖点选择很好。已覆盖 project/provider 两类 scoped path 的解析与判定主路径，能有效防止路由入口回归。
Also applies to: 22-37, 39-49
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/handler/project_proxy.go`:
- Around line 96-108: The function parseScopePath currently trims parts[0] only
for the emptiness check but then returns the untrimmed parts[0]; change it to
trim once and use the trimmed value everywhere: compute trimmed :=
strings.TrimSpace(parts[0]), use trimmed for the empty check and assign
scopeValue = trimmed before returning; keep apiPath logic the same and validate
with isValidAPIPath(apiPath) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b251572f-9c02-43e3-8da9-89599e23cc99
📒 Files selected for processing (6)
internal/handler/project_proxy.gointernal/handler/project_proxy_test.gointernal/handler/proxy.gointernal/router/router.gointernal/router/router_test.gotests/e2e/playwright/provider-proxy-route.spec.ts
✅ Files skipped from review due to trivial changes (1)
- internal/handler/project_proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/playwright/provider-proxy-route.spec.ts
📜 Review details
⏰ 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: playwright
- GitHub Check: e2e
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ymkiux
Repo: awsl-project/maxx PR: 0
File: :0-0
Timestamp: 2026-03-11T07:29:12.071Z
Learning: Reviewer ymkiux (repo: awsl-project/maxx) requires strict attention to breaking changes in all PRs. Always thoroughly audit public API, interface, and behavioral contract changes — especially in sidebar/auth/layout components — before raising review comments. Do not flag superficial style issues; focus on correctness, regressions, and breaking changes.
🔇 Additional comments (8)
internal/handler/proxy.go (2)
186-197: 修复已实现：现在正确拒绝无效的 X-Maxx-Provider-ID。代码现在在 header 存在但解析失败或值为 0 时返回 400 错误，避免了静默降级到未作用域路由的问题。这解决了之前 review 中提出的安全隐患。
224-225: LGTM!Provider ID 正确地在验证后存入 flow context，与 project ID 的处理方式一致。
internal/router/router_test.go (2)
9-21: 测试覆盖完善。
TestRouteMatchesProviderScope正确覆盖了三种关键场景：providerID=0的通配匹配、精确 ID 匹配、以及 ID 不匹配的拒绝情况。
23-44: LGTM!
TestRouteMatchesBaseScope完整测试了 tenant、client type 匹配及IsEnabled禁用场景，覆盖了基础作用域过滤的核心逻辑。internal/router/router.go (2)
144-161: 修复已实现：项目路由分支现在正确应用 provider 作用域过滤。Line 155 的
routeMatchesProviderScope(route, providerID)检查确保了/provider/{id}/...请求在项目路由分支中也会按 provider 过滤，不再会命中其他 provider 的路由。这解决了之前 review 中提出的跨 provider 匹配问题。
246-261: LGTM! 辅助函数设计清晰。
routeMatchesBaseScope和routeMatchesProviderScope的抽取提高了代码可读性和可维护性，逻辑正确且边界条件处理得当（如tenantID > 0时才做租户匹配，providerID == 0时通配所有 provider）。internal/handler/project_proxy.go (2)
110-122: 修复已实现：白名单校验现在更严格。
matchesEndpointPath使用path == endpoint || strings.HasPrefix(path, endpoint+"/")的逻辑，正确防止了/v1/messages-debug等相邻路径绕过白名单。这解决了之前 review 中提出的安全隐患。
57-70: LGTM! Provider 作用域路由实现正确。Provider 分支正确验证了 ID 格式（非法或为 0 返回 400），通过
providerRepo.GetByID确认 provider 存在性，并设置X-Maxx-Provider-IDheader 传递给下游 proxy handler。
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
3483fc1 to
c7bb9a7
Compare
|
Refactored the current PR to keep provider scope independent from project scope. What changed:
Validation:
Force-pushed updated branch head: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/handler/provider_proxy_test.go (1)
5-29: LGTM! 单元测试覆盖了基本场景。测试验证了 provider-scoped 路径解析和检测的核心功能。建议后续可考虑补充边界情况测试，如空 provider ID（
/provider//v1/messages）、无效 API 路径等。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handler/provider_proxy_test.go` around lines 5 - 29, Add unit tests covering edge cases for provider-scoped path parsing and detection: extend or add tests that call ProviderProxyHandler.parseScopedPath (same scope as TestParseProviderScopedPath) with an empty provider ID (e.g., "/provider//v1/messages") and with invalid/malformed API paths to assert parse failure (ok == false) and expected zero-values; likewise add cases to isProviderProxyPath (same scope as TestIsProviderProxyPath) verifying it returns false for empty provider IDs and malformed paths. Ensure tests reference ProviderProxyHandler.parseScopedPath and isProviderProxyPath and assert both boolean results and returned providerID/apiPath values where applicable.internal/handler/provider_proxy.go (1)
56-57: 请求热路径日志建议降噪。Line 56 每次请求都
log.Printf可能导致高流量下日志量过大；建议改为 debug 级、采样，或仅在异常路径记录。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handler/provider_proxy.go` around lines 56 - 57, 当前在 provider_proxy.go 中每次请求都会调用 log.Printf("[ProviderProxy] Routing request through provider: %s (ID: %d)", provider.Name, provider.ID) 导致热路径日志噪声过大；请将该调用改为不在每个请求打印，例如：使用现有的结构化 logger 的 Debug/Trace 级别（如 logger.Debugf）或添加采样/速率限制封装，或者只在异常分支（provider 为 nil 或错误发生）记录 info/error 日志；定位并修改包含 log.Printf、provider.Name、provider.ID 的代码行，确保保留 r.Header.Set("X-Maxx-Provider-ID", ...) 行不变。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/handler/provider_proxy.go`:
- Around line 50-53: The current block conflates storage errors and a missing
provider by returning 404 for both; change the logic in ProviderProxy where
GetByID is called so that if err != nil you log the error (include err and
providerValue) and return a 500 (internal server error) via writeError, while
only when provider == nil you log “Provider not found” and return 404; update
the log.Printf and writeError calls accordingly to reference providerValue, err,
provider, and keep behavior for successful retrieval unchanged.
---
Nitpick comments:
In `@internal/handler/provider_proxy_test.go`:
- Around line 5-29: Add unit tests covering edge cases for provider-scoped path
parsing and detection: extend or add tests that call
ProviderProxyHandler.parseScopedPath (same scope as TestParseProviderScopedPath)
with an empty provider ID (e.g., "/provider//v1/messages") and with
invalid/malformed API paths to assert parse failure (ok == false) and expected
zero-values; likewise add cases to isProviderProxyPath (same scope as
TestIsProviderProxyPath) verifying it returns false for empty provider IDs and
malformed paths. Ensure tests reference ProviderProxyHandler.parseScopedPath and
isProviderProxyPath and assert both boolean results and returned
providerID/apiPath values where applicable.
In `@internal/handler/provider_proxy.go`:
- Around line 56-57: 当前在 provider_proxy.go 中每次请求都会调用 log.Printf("[ProviderProxy]
Routing request through provider: %s (ID: %d)", provider.Name, provider.ID)
导致热路径日志噪声过大；请将该调用改为不在每个请求打印，例如：使用现有的结构化 logger 的 Debug/Trace 级别（如
logger.Debugf）或添加采样/速率限制封装，或者只在异常分支（provider 为 nil 或错误发生）记录 info/error
日志；定位并修改包含 log.Printf、provider.Name、provider.ID 的代码行，确保保留
r.Header.Set("X-Maxx-Provider-ID", ...) 行不变。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d2a3640-4e0c-4077-8ab1-ebdd37a7ac72
📒 Files selected for processing (13)
cmd/maxx/main.gointernal/core/database.gointernal/core/server.gointernal/handler/project_proxy.gointernal/handler/project_proxy_test.gointernal/handler/provider_proxy.gointernal/handler/provider_proxy_test.gointernal/handler/proxy.gointernal/handler/static.gointernal/router/router.gointernal/router/router_test.gotests/e2e/playwright/provider-proxy-route.spec.tstests/e2e/proxy_setup_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/handler/project_proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/playwright/provider-proxy-route.spec.ts
- internal/handler/static.go
📜 Review details
⏰ 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: e2e
- GitHub Check: playwright
🔇 Additional comments (16)
internal/handler/proxy.go (2)
186-197: LGTM! Provider ID 验证逻辑正确实现。代码已按之前的审查建议修复：当
X-Maxx-Provider-IDheader 存在但解析失败或值为 0 时，正确返回 400 错误并中止请求，避免了静默降级到未作用域路由的问题。
224-225: LGTM! Provider ID 正确传递到 flow context。
flow.KeyProviderID在flow.KeyProjectID之后设置，与下游中间件（middleware_ingress.go、middleware_route_match.go）的读取逻辑一致。tests/e2e/proxy_setup_test.go (1)
197-198: LGTM! 测试环境正确配置了 provider-scoped 路由。测试环境的路由配置与生产环境（
cmd/maxx/main.go、internal/core/server.go）保持一致，确保 E2E 测试能够覆盖新增的 provider-scoped 代理功能。Also applies to: 221-222
internal/core/server.go (1)
107-116: LGTM! 路由配置正确支持 provider-scoped 代理。两种模式的路由配置逻辑清晰：
- 静态服务启用时：
CombinedHandler根据路径前缀分发到对应处理器- 静态服务禁用时：显式注册
/project/和/provider/前缀路由cmd/maxx/main.go (2)
379-379: LGTM! Provider proxy handler 正确集成到主程序。
providerProxyHandler的创建和注册与internal/core/database.go保持一致，CombinedHandler正确接收三个参数以支持 project 和 provider 两种作用域的代理。Also applies to: 424-427
534-535: LGTM! 启动日志清晰记录了新增的端点。日志格式与现有端点保持一致，便于用户了解 provider-scoped 代理的访问方式。
internal/core/database.go (1)
92-92: LGTM! ServerComponents 正确扩展以支持 ProviderProxyHandler。
ProviderProxyHandler字段添加到ServerComponents结构体，并在InitializeServerComponents中使用cachedProviderRepo正确初始化，与ProjectProxyHandler使用cachedProjectRepo的模式一致。Also applies to: 413-413, 436-436
internal/handler/project_proxy.go (3)
60-68: LGTM! 路径解析逻辑重构为共享函数。
parseScopedPath现在委托给parseScopePath，使得 project 和 provider 处理器可以共享相同的解析逻辑，减少代码重复。
70-86: LGTM! 空白字符处理已修复。
parseScopePath现在使用strings.TrimSpace处理scopeValue，确保带空格的 scope value（如" my-project "）被正确清理，与之前的审查建议一致。
88-100: LGTM! API 路径白名单验证更加严格。
matchesEndpointPath实现了精确匹配或前缀加/的子路径匹配，避免了strings.HasPrefix可能误匹配/v1/messages-debug等非法路径的问题。测试文件project_proxy_test.go中的TestIsValidAPIPath_AllowsExactAndSubpathsOnly验证了所有预期场景。internal/router/router.go (4)
27-27: LGTM! MatchContext 正确扩展以支持 ProviderID。
ProviderID字段添加到MatchContext，并在Match方法中正确读取，与调用方（middleware_route_match.go）的传参一致。Also applies to: 117-117
122-139: LGTM! Provider-scoped 请求正确绕过项目路由。当
providerID != 0时，useProjectRoutes为false，确保 provider-scoped 请求直接使用全局路由，不会错误地匹配到其他项目的路由。这解决了之前审查中指出的问题。
145-161: LGTM! 项目路由过滤现在包含 provider scope 检查。
routeMatchesProviderScope确保在查找项目路由时，如果请求带有providerID，只有匹配该 provider 的路由才会被选中。
247-262: LGTM! 辅助函数提高了代码可读性。
routeMatchesBaseScope和routeMatchesProviderScope将重复的过滤逻辑抽取为独立函数，使主逻辑更清晰，也便于单元测试验证。internal/router/router_test.go (2)
22-57: 基础匹配测试覆盖完整，回归保护到位。这两组用例把 provider/base scope 的成功与失败路径都覆盖到了，粒度和断言都比较扎实。
59-99: Provider scope 跳过 project route 的行为验证很关键，写得很好。该用例精准锁定本次变更目标，能有效防止后续路由回归。
|
Adding the missing Playwright evidence screenshot via a free image host. Playwright UI evidence (requests page after hitting This screenshot comes from the provider-scoped Playwright test artifact for the current PR branch. |
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
c7bb9a7 to
0a97489
Compare
|
Updated the PR again with a narrower rewrite. What changed in this revision:
Validation:
Current branch head: |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/handler/provider_proxy.go (1)
49-54:⚠️ Potential issue | 🟠 Major存储层错误与"未找到"不应同一处理。
当前代码将
GetByID返回的真实错误（如数据库连接失败）与provider == nil的情况都返回 404，这会误导客户端重试策略并增加排障难度。建议分开处理：
err != nil时返回 500 并记录错误详情，provider == nil时返回 404。建议修复
tenantID := maxxctx.GetTenantID(r.Context()) provider, err := h.providerRepo.GetByID(tenantID, providerID) - if err != nil || provider == nil { - log.Printf("[ProviderProxy] Provider not found for id: %s", providerValue) - writeError(w, http.StatusNotFound, "provider not found") - return - } + if err != nil { + log.Printf("[ProviderProxy] failed to load provider tenant=%d id=%d: %v", tenantID, providerID, err) + writeError(w, http.StatusInternalServerError, "internal server error") + return + } + if provider == nil { + log.Printf("[ProviderProxy] provider not found tenant=%d id=%d", tenantID, providerID) + writeError(w, http.StatusNotFound, "provider not found") + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handler/provider_proxy.go` around lines 49 - 54, 当前逻辑把 h.providerRepo.GetByID 返回的真实错误与 provider==nil 混为一谈并统一返回 404；请分开处理：如果 err != nil，则记录具体错误（使用 log.Printf 或更合适的记录器并包含 err）并通过 writeError 返回 http.StatusInternalServerError，只有在 err == nil 且 provider == nil 时才返回 http.StatusNotFound 并返回 "provider not found"；调整处理顺序并保留 provider、providerID/tenantID 和 h.providerRepo.GetByID 的上下文以便定位修改点。
🧹 Nitpick comments (2)
internal/handler/project_proxy.go (1)
13-20: 把/provider/分支收回到ProviderProxyHandler即可。
internal/core/server.go在 Line 113-114 已经把/provider/直接注册到ProviderProxyHandler，internal/handler/static.go在 Line 318-320 也会先把这类请求分流过去，所以这里的 provider 解析和查库分支不会经过生产流量。继续把它留在ProjectProxyHandler里，会让NewProjectProxyHandler平白多背一个providerRepo依赖，并且让internal/handler/project_proxy_test.go继续覆盖一条不可达路径。建议把这个类型重新收窄到/project/{slug}/...，provider 逻辑和对应测试只保留在ProviderProxyHandler。Also applies to: 22-34, 37-72, 84-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handler/project_proxy.go` around lines 13 - 20, The ProjectProxyHandler currently contains provider-specific routing and a providerRepo dependency; remove the provider branch and any provider lookup logic from ProjectProxyHandler (and its constructor NewProjectProxyHandler) so the struct only holds proxyHandler, modelsHandler and projectRepo and only handles /project/{slug}/... routes; move the provider-specific code and tests into ProviderProxyHandler (and its tests like project_proxy_test.go should be split/moved so provider paths are covered only by ProviderProxyHandler tests) and remove providerRepo usage/references from ProjectProxyHandler and its tests to avoid the unreachable branch.internal/handler/provider_proxy.go (1)
57-57:TrimSpace对整数转换结果是多余的。
itoa(provider.ID)将整数转为字符串，结果不会包含空白字符，TrimSpace调用可移除。可选简化
- r.Header.Set("X-Maxx-Provider-ID", strings.TrimSpace(itoa(provider.ID))) + r.Header.Set("X-Maxx-Provider-ID", itoa(provider.ID))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handler/provider_proxy.go` at line 57, The code calls strings.TrimSpace on the result of itoa(provider.ID), which is unnecessary because integer-to-string conversion yields no surrounding whitespace; remove the strings.TrimSpace wrapper and set the header directly using the integer-to-string result (i.e., replace strings.TrimSpace(itoa(provider.ID)) with the direct itoa(provider.ID) / strconv.Itoa(provider.ID) value) in the r.Header.Set call in provider_proxy.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/router/router_test.go`:
- Around line 59-70: 测试使用了全局单例 cooldown.Default(), 导致与包内其它测试互相污染（provider 7 /
"claude" 可能被标记为 cooldown）；请改为为该用例注入一个独立的 cooldown 管理器到 Router（不要使用
cooldown.Default()），或者在测试前后显式清理全局管理器中 provider 7 和 "claude" 的 cooldown 状态
以保证隔离；参考符号：Router.Match、cooldown.Default()、provider id 7、"claude"，将构造中的
cooldown: cooldown.Default() 替换为新建的 manager 实例（或在 setup/teardown 调用清理方法）以修复偶发的
ErrNoRoutes。
In `@internal/router/router.go`:
- Around line 122-126: When handling provider-scoped requests, don't let project
routing rules influence selection: update the logic around
useProjectRoutes/getRoutingStrategy so that when providerID (ctx.ProviderID) !=
0 you force the project argument passed into getRoutingStrategy to 0 (global)
instead of the original projectID; e.g., compute a projectIDForStrategy variable
(0 for provider scope, original projectID otherwise) and use that in the calls
to getRoutingStrategy (also adjust the same behavior at the other occurrence
around the second getRoutingStrategy call referenced in the comment).
In `@tests/e2e/playwright/provider-proxy-route.spec.ts`:
- Around line 15-65: The startMockClaudeServer Promise executor only uses
resolve and throws inside the server.listen callback which won't reject the
Promise; modify the executor to accept both (resolve, reject), add
server.on('error', (err) => reject(err)) to propagate listen errors, and replace
the throw in the server.listen callback (inside startMockClaudeServer) with
reject(new Error(...)) so the caller receives a proper rejected Promise when the
mock server fails to bind.
---
Duplicate comments:
In `@internal/handler/provider_proxy.go`:
- Around line 49-54: 当前逻辑把 h.providerRepo.GetByID 返回的真实错误与 provider==nil
混为一谈并统一返回 404；请分开处理：如果 err != nil，则记录具体错误（使用 log.Printf 或更合适的记录器并包含 err）并通过
writeError 返回 http.StatusInternalServerError，只有在 err == nil 且 provider == nil
时才返回 http.StatusNotFound 并返回 "provider not found"；调整处理顺序并保留
provider、providerID/tenantID 和 h.providerRepo.GetByID 的上下文以便定位修改点。
---
Nitpick comments:
In `@internal/handler/project_proxy.go`:
- Around line 13-20: The ProjectProxyHandler currently contains
provider-specific routing and a providerRepo dependency; remove the provider
branch and any provider lookup logic from ProjectProxyHandler (and its
constructor NewProjectProxyHandler) so the struct only holds proxyHandler,
modelsHandler and projectRepo and only handles /project/{slug}/... routes; move
the provider-specific code and tests into ProviderProxyHandler (and its tests
like project_proxy_test.go should be split/moved so provider paths are covered
only by ProviderProxyHandler tests) and remove providerRepo usage/references
from ProjectProxyHandler and its tests to avoid the unreachable branch.
In `@internal/handler/provider_proxy.go`:
- Line 57: The code calls strings.TrimSpace on the result of itoa(provider.ID),
which is unnecessary because integer-to-string conversion yields no surrounding
whitespace; remove the strings.TrimSpace wrapper and set the header directly
using the integer-to-string result (i.e., replace
strings.TrimSpace(itoa(provider.ID)) with the direct itoa(provider.ID) /
strconv.Itoa(provider.ID) value) in the r.Header.Set call in provider_proxy.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcbe2f24-43f0-4560-841b-ddffc114d880
📒 Files selected for processing (13)
cmd/maxx/main.gointernal/core/database.gointernal/core/server.gointernal/handler/project_proxy.gointernal/handler/project_proxy_test.gointernal/handler/provider_proxy.gointernal/handler/provider_proxy_test.gointernal/handler/proxy.gointernal/handler/static.gointernal/router/router.gointernal/router/router_test.gotests/e2e/playwright/provider-proxy-route.spec.tstests/e2e/proxy_setup_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/handler/provider_proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/handler/proxy.go
- internal/core/database.go
- cmd/maxx/main.go
📜 Review details
⏰ 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: e2e
- GitHub Check: playwright
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ymkiux
Repo: awsl-project/maxx PR: 0
File: :0-0
Timestamp: 2026-03-11T07:29:12.071Z
Learning: Reviewer ymkiux (repo: awsl-project/maxx) requires strict attention to breaking changes in all PRs. Always thoroughly audit public API, interface, and behavioral contract changes — especially in sidebar/auth/layout components — before raising review comments. Do not flag superficial style issues; focus on correctness, regressions, and breaking changes.
🔇 Additional comments (4)
internal/handler/provider_proxy.go (4)
1-11: LGTM!包声明和导入正确，所有导入在代码中均有使用。
13-32: LGTM!结构体定义和构造函数遵循标准的 Go 依赖注入模式，无线程安全问题。
34-46: LGTM!路径解析和 provider ID 验证逻辑正确，对无效路径返回 404、无效 ID 返回 400 的状态码选择合理。
67-75: LGTM!
parseScopedPath实现正确，复用了project_proxy.go中的共享parseScopePath辅助函数，保持了与项目范围解析一致的行为。
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
0a97489 to
4d739ff
Compare
|
Updated the branch again to narrow the rewrite further. This revision keeps What remains in the diff:
Project-side churn was reduced to the routing detection expectation in Validation:
Current branch head: |
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
4d739ff to
0a02e95
Compare
|
Pushed another narrowed update. This revision keeps the original Validation:
Current branch head: |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
internal/handler/provider_proxy.go (1)
48-53:⚠️ Potential issue | 🟠 Major把仓储异常和“未找到”分开处理。
Line 49-53 现在把所有
GetByID错误都映射成 404。当前仓储实现会用domain.ErrNotFound表示不存在，但其它数据库/缓存异常也会落到这里并被伪装成provider not found，这会误导客户端和排障。这里应该只把 not found 返回 404，其它错误返回 500。🛠️ 建议修复
import ( + "errors" "log" "net/http" "strconv" "strings" maxxctx "github.com/awsl-project/maxx/internal/context" + "github.com/awsl-project/maxx/internal/domain" "github.com/awsl-project/maxx/internal/repository" ) @@ tenantID := maxxctx.GetTenantID(r.Context()) provider, err := h.providerRepo.GetByID(tenantID, providerID) - if err != nil || provider == nil { - log.Printf("[ProviderProxy] Provider not found for id: %s", providerValue) - writeError(w, http.StatusNotFound, "provider not found") - return - } + if err != nil { + if errors.Is(err, domain.ErrNotFound) { + log.Printf("[ProviderProxy] Provider not found tenant=%d id=%d", tenantID, providerID) + writeError(w, http.StatusNotFound, "provider not found") + return + } + log.Printf("[ProviderProxy] Failed to load provider tenant=%d id=%d: %v", tenantID, providerID, err) + writeError(w, http.StatusInternalServerError, "internal server error") + return + } + if provider == nil { + log.Printf("[ProviderProxy] Provider not found tenant=%d id=%d", tenantID, providerID) + writeError(w, http.StatusNotFound, "provider not found") + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handler/provider_proxy.go` around lines 48 - 53, Separate repository "not found" from other errors when calling providerRepo.GetByID: after calling h.providerRepo.GetByID(tenantID, providerID) check if err != nil and use errors.Is(err, domain.ErrNotFound) (or compare domain.ErrNotFound) to return writeError(w, http.StatusNotFound, "provider not found"); for any other non-nil err log the error and return writeError(w, http.StatusInternalServerError, "internal server error"); keep the existing provider == nil check only for the not-found path if you prefer, but do not map all errors to 404.internal/router/router_test.go (1)
59-70:⚠️ Potential issue | 🟡 Minor不要在这个用例里复用全局 cooldown 单例。
Line 69 把
cooldown.Default()注入给Router后，这个测试会和同包其它测试共享 cooldown 状态；只要别的用例给 provider7/claude留下标记，这里就会偶发返回ErrNoRoutes。建议改成独立 manager，或者至少在用例前后显式清理对应 cooldown。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/router/router_test.go` around lines 59 - 70, Test is injecting the global cooldown singleton via cooldown.Default() into Router which causes test flakiness due to shared state (provider id 7 / provider "claude" cooldown marks) and sporadic ErrNoRoutes; replace the global singleton with a fresh cooldown manager instance (or instantiate a new cooldown manager and inject that) for this test's Router (the Router struct created in TestMatch_ProviderScopeSkipsProjectRoutes) or explicitly clear the provider 7/“claude” cooldown before/after the test so the test does not share state with other tests.tests/e2e/playwright/provider-proxy-route.spec.ts (1)
15-65:⚠️ Potential issue | 🟡 Minor把 mock server 的启动失败显式 reject 给调用方。
Line 16 这里只有
resolve，而 Line 61 的throw发生在server.listen()的回调里，不会把外层 Promise 变成 rejected；同时这里也没监听error事件。端口绑定失败时，这个 helper 很容易变成未捕获异常或悬空 Promise。🛠️ 建议修复
function startMockClaudeServer(): Promise<{ server: http.Server; port: number }> { - return new Promise((resolve) => { + return new Promise((resolve, reject) => { const server = http.createServer((req, res) => { if (req.method === 'POST' && req.url?.startsWith('/v1/messages')) { let body = ''; req.on('data', (chunk) => { body += chunk; @@ res.writeHead(404, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ error: 'not found' })); }); + server.once('error', reject); server.listen(0, '127.0.0.1', () => { const address = server.address(); if (!address || typeof address === 'string') { - throw new Error('Failed to determine mock server port'); + reject(new Error('Failed to determine mock server port')); + return; } resolve({ server, port: address.port }); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/playwright/provider-proxy-route.spec.ts` around lines 15 - 65, The startMockClaudeServer Promise currently only calls resolve and throws inside the server.listen callback, which doesn't reject the outer Promise or handle listen errors; change the Promise executor to (resolve, reject), attach server.on('error', reject) before calling server.listen, and replace the throw in the server.listen callback with reject(new Error(...)) so port binding failures are propagated to callers; ensure any event listeners are cleaned up on resolve/reject if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/handler/provider_proxy.go`:
- Around line 48-53: Separate repository "not found" from other errors when
calling providerRepo.GetByID: after calling h.providerRepo.GetByID(tenantID,
providerID) check if err != nil and use errors.Is(err, domain.ErrNotFound) (or
compare domain.ErrNotFound) to return writeError(w, http.StatusNotFound,
"provider not found"); for any other non-nil err log the error and return
writeError(w, http.StatusInternalServerError, "internal server error"); keep the
existing provider == nil check only for the not-found path if you prefer, but do
not map all errors to 404.
In `@internal/router/router_test.go`:
- Around line 59-70: Test is injecting the global cooldown singleton via
cooldown.Default() into Router which causes test flakiness due to shared state
(provider id 7 / provider "claude" cooldown marks) and sporadic ErrNoRoutes;
replace the global singleton with a fresh cooldown manager instance (or
instantiate a new cooldown manager and inject that) for this test's Router (the
Router struct created in TestMatch_ProviderScopeSkipsProjectRoutes) or
explicitly clear the provider 7/“claude” cooldown before/after the test so the
test does not share state with other tests.
In `@tests/e2e/playwright/provider-proxy-route.spec.ts`:
- Around line 15-65: The startMockClaudeServer Promise currently only calls
resolve and throws inside the server.listen callback, which doesn't reject the
outer Promise or handle listen errors; change the Promise executor to (resolve,
reject), attach server.on('error', reject) before calling server.listen, and
replace the throw in the server.listen callback with reject(new Error(...)) so
port binding failures are propagated to callers; ensure any event listeners are
cleaned up on resolve/reject if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e7ac052-bf9e-4caf-8a82-3f38023a5ae2
📒 Files selected for processing (12)
cmd/maxx/main.gointernal/core/database.gointernal/core/server.gointernal/handler/project_proxy_test.gointernal/handler/provider_proxy.gointernal/handler/provider_proxy_test.gointernal/handler/proxy.gointernal/handler/static.gointernal/router/router.gointernal/router/router_test.gotests/e2e/playwright/provider-proxy-route.spec.tstests/e2e/proxy_setup_test.go
✅ Files skipped from review due to trivial changes (4)
- internal/core/server.go
- internal/handler/project_proxy_test.go
- internal/router/router.go
- tests/e2e/proxy_setup_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/handler/proxy.go
- internal/core/database.go
- internal/handler/provider_proxy_test.go
- internal/handler/static.go
📜 Review details
⏰ 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: e2e
- GitHub Check: playwright
🔇 Additional comments (3)
cmd/maxx/main.go (1)
378-379: Provider 作用域入口的接线是完整的。
ProviderProxyHandler的构造、CombinedHandler注入和启动日志三处都同步补齐了，和/provider/...的分流设计保持一致。Also applies to: 426-427, 535-535
internal/router/router_test.go (1)
22-57: 基础匹配规则的回归点补得很到位。
routeMatchesProviderScope和routeMatchesBaseScope的正反例都固定下来了，后续再改匹配逻辑时回归会很直接。internal/handler/provider_proxy.go (1)
67-101: 路径解析和白名单收口得比较稳。这里先裁剪 scope，再只放行明确支持的 API 前缀，能避免
/provider/{id}/...意外落到任意代理路径。
|
Pushed an update that keeps the original Validation:
Current branch head: |
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
2 times, most recently
from
c811709 to
0117639
Compare
|
Updated the provider handler to follow the existing project handler style more closely. What changed in this revision:
Validation:
Current branch head: |
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
0117639 to
e0415d3
Compare
cmd/maxx/main.go
Outdated
| combinedHandler := handler.NewCombinedHandler(projectProxyHandler, staticHandler) | ||
| combinedHandler := handler.NewCombinedHandler( | ||
| projectProxyHandler, | ||
| handler.NewProviderProxyHandler(proxyHandler, modelsHandler, cachedProviderRepo), |
There was a problem hiding this comment.
why this not same as project handler
internal/core/server.go
Outdated
| log.Printf("[Server] Static file serving enabled") | ||
| } else { | ||
| mux.Handle("/", components.ProjectProxyHandler) | ||
| mux.Handle("/project/", components.ProjectProxyHandler) |
There was a problem hiding this comment.
why change / to /projject
|
Addressed the latest review points. Updates in this revision:
Validation:
Current branch head: |
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
e0415d3 to
a5a5451
Compare
cmd/maxx/main.go
Outdated
| // Serve static files (Web UI) with project proxy support - must be last (default route) | ||
| staticHandler := handler.NewStaticHandler() | ||
| combinedHandler := handler.NewCombinedHandler(projectProxyHandler, staticHandler) | ||
| providerProxyHandler := handler.NewProviderProxyHandler(proxyHandler, modelsHandler, cachedProviderRepo) |
There was a problem hiding this comment.
移动到和 project 相同的初始化位置
internal/handler/proxy.go
Outdated
| } | ||
|
|
||
| var providerID uint64 | ||
| if providerIDStr := r.Header.Get("X-Maxx-Provider-ID"); providerIDStr != "" { |
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
2 times, most recently
from
9487b55 to
c0ab7fd
Compare
|
Updated the PR with only the two required follow-up changes from the latest review. What changed:
Validation:
Current branch head: |
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
c0ab7fd to
704a401
Compare
|
Updated the PR again to keep only the required provider-scope changes. What changed in this revision:
Validation:
Current branch head: |
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
704a401 to
4515f09
Compare
|
Updated the branch again to minimize changes and keep the new behavior parallel to the existing project path. What changed in this revision:
Validation:
Current branch head: |
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
4515f09 to
2240e3d
Compare
|
Pushed another narrowed revision focused on parallel provider logic with minimal churn to existing paths. What changed in this revision:
Validation:
Current branch head: |
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
2240e3d to
4532ced
Compare
|
Updated the branch again with a narrower provider-scoped pass. This revision keeps provider forwarding as a fixed one-to-one path:
Validation:
Current branch head: |
What: - rewrite provider-scoped proxy requests to use a dedicated direct-forward path instead of the generic router/executor route selection flow - wire provider proxy construction to the route and proxy-request repositories needed for one-to-one forwarding and request recording - drop the previous provider-scoped context/router/executor changes so the diff only keeps the required handler and test wiring Why: - provider-scoped requests should forward to the specified provider one-to-one without retry or generic route reuse - keeping the logic parallel to the existing project proxy entrypoint minimizes unrelated surface area in this PR Tests: - go test ./internal/handler ./internal/router ./tests/e2e/... (pass) - go build -o maxx ./cmd/maxx (pass) - MAXX_ADMIN_PASSWORD=test123 ./maxx -addr :9880 + MAXX_E2E_BASE_URL=http://127.0.0.1:9880 MAXX_E2E_USERNAME=admin MAXX_E2E_PASSWORD=test123 pnpm --dir tests/e2e/playwright test:provider-proxy-route (pass)
awsl233777
force-pushed
the
feat/provider-proxy-scope
branch
from
4532ced to
0577dd6
Compare
|
已按 review 意见把 provider-scoped proxy 收敛成 provider 一对一直转发，不再复用通用 router/executor 选路重试链路，相关的 context/router/executor 改动也已从这条 PR 里移除。 本轮保留的核心改动：
本地验证：
都已通过。 |
2 participants
Summary
/provider/{id}/...directlyChanges
/provider/{id}/...alongside/project/{slug}/...providerIDthrough proxy ingress and router matching so only routes for that provider are consideredTests
go test ./...MAXX_E2E_BASE_URL=http://127.0.0.1:9980 MAXX_E2E_USERNAME=admin MAXX_E2E_PASSWORD=test123 pnpm --dir tests/e2e/playwright test:provider-proxy-routeRisks
Follow-ups
Summary by CodeRabbit
发布说明
新功能
测试