feat: OriginFlow backend — region routing, Model Market & sales-leads APIs, originflow infra - #6738
feat: OriginFlow backend — region routing, Model Market & sales-leads APIs, originflow infra#6738peidunyue wants to merge 1 commit into
Conversation
… APIs, originflow infra - Region-aware channel routing (model/region_route.go, channel_cache, ability, service/channel_select). - Model Market backend: product table, admin CRUD, public read API, i18n/currency. - Sales-leads backend + analytics events model. - Originflow infra: common/redirect.go (IsSafeRedirect), model/analytics_event.go, deploy/nginx/originflow.conf, OriginFlow SystemName branding. - Build/Docker: build from source, configurable npm registry; nginx serves www/app/api hosts. - Note: upstream test files intentionally retained (not deleted) vs the merged branch.
WalkthroughThis PR expands OriginFlow with public-site, market, team, SLA, distributor, analytics, and regional-routing APIs. It removes token auto-group persistence and replayable relay bodies, adds explicit request-body sizing, updates AWS handling, and adds local Docker and Nginx deployment configuration. ChangesOriginFlow platform APIs
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.72.0)Trivy execution failed: 2026-08-10T00:12:34Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: ansible scan error: fs filter error: fs filter error: walk error range error: stat .coderabbit-opengrep-fallback.5ddbd265-e041-4fd9-abfd-e955b232b45c.yml: no such file or directory: range error: stat .coderabbit-opengrep-fallback.5ddbd265-e041-4fd9-abfd-e955b232b45c.yml: no such file or directory 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
service/text_quota_test.go (1)
287-302: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd metadata-only billing-path cases.
These cases use constructors with provider payloads. The changed contract classifies known
SourceorSemanticvalues even when the provider-specific payload is absent. Add deterministic table cases for metadata-only OpenAI, Anthropic, and Gemini usage, including trimmed or case-varied values and estimated usage.As per coding guidelines, “Backend tests must protect real behavior, API contracts, billing/accounting invariants, compatibility, or regression paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/text_quota_test.go` around lines 287 - 302, The usageBillingPathForLog test cases only cover provider payload constructors, not metadata-only usage. Add deterministic cases covering OpenAI, Anthropic, and Gemini classifications using known Source or Semantic metadata without provider payloads, including trimmed or case-varied values and estimated Gemini usage, while preserving the existing payload-based cases.Source: Coding guidelines
middleware/distributor.go (1)
105-129: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftApply regional allowlists to affinity-selected channels.
The affinity branch accepts
preferredbeforeCacheGetRandomSatisfiedChannelresolves and filters regional routing. An activeAllowedIdswhitelist can therefore be bypassed by an existing channel affinity.Resolve routing before affinity selection. Reject an affinity channel that is outside the active allowlist. An affinity can override score ordering, but it must not override channel eligibility.
This path bypasses the routing-aware selector in
service/channel_select.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware/distributor.go` around lines 105 - 129, Update the affinity-selection flow around GetPreferredChannelByAffinity and CacheGetChannel to apply the same regional allowlist eligibility used by CacheGetRandomSatisfiedChannel before accepting preferred. Reject affinity channels whose IDs are not in the active AllowedIds set, while preserving affinity’s ability to override score ordering for otherwise eligible channels.model/ability.go (1)
110-127: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake database-backed retries use regional score tiers.
getChannelQueryrestricts candidates by legacy ability priority before Line 127 applies regional routing. When memory cache is disabled, a higher regional score in a lower priority tier cannot be selected. Retries also remain based on legacy priority tiers.
model/channel_cache.goinstead ranks all candidates byregionStrategyScorewhen routing is active. Make the database selector load the same candidate set, apply regional filtering, and derive retry tiers from the regional score.This comparison uses the provided
model/channel_cache.goselector context.Also applies to: 176-216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/ability.go` around lines 110 - 127, Update GetChannel and the related database selection flow to avoid getChannelQuery’s legacy priority restriction when routing is active: load all eligible abilities, apply filterAbilitiesByRegion before selecting, and derive retry tiers from regionStrategyScore so regional ranking matches the model/channel_cache.go selector. Preserve legacy priority behavior when routing is inactive and update the retry handling in the additional affected flow consistently.
🟠 Major comments (21)
deploy/nginx/originflow.conf-25-29 (1)
25-29: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winKeep upstream keepalive connections reusable.
The map sends
Connection: closefor every non-upgrade request. Theoriginflow_backendkeepalivepool needs a clearedConnectionheader on repeated proxy requests. Use an empty map value, set the server HTTP version consistently (for example in eachserverblock), and leave WebSocket upgrades withdefault upgrade;soConnection: upgradestill allows the request to complete.Proposed fix
map $http_upgrade $connection_upgrade { default upgrade; - '' close; + '' ''; } @@ -34,7 +34,7 @@ upstream originflow_backend { keepalive 32; }Verify the deployed Nginx version before relying on upstream connection reuse; older versions may require explicit
proxy_http_version 1.1;as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/nginx/originflow.conf` around lines 25 - 29, Update the $connection_upgrade map in originflow.conf to use an empty value for non-upgrade requests while retaining default upgrade for WebSocket requests. Configure a consistent HTTP version in each server block, adding explicit proxy_http_version 1.1 where required by the supported Nginx version, so originflow_backend keepalive connections remain reusable.controller/market.go-4-4 (1)
4-4: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the
common/json.gowrappers instead ofencoding/json.Lines 92 and 252 call
json.Unmarshaldirectly in business logic. The repository requirescommon.Unmarshalorcommon.UnmarshalJsonStrfor application-level deserialization. Replace both call sites and drop theencoding/jsonimport.As per coding guidelines: "Use the wrappers in common/json.go for JSON marshal and unmarshal operations... Do not directly call encoding/json operations in business code".
♻️ Proposed change
- if err := json.Unmarshal([]byte(req.Metadata), &tmp); err != nil { + if err := common.UnmarshalJsonStr(req.Metadata, &tmp); err != nil { return false, "metadata must be valid JSON" }var data map[string]map[string]string - if err := json.Unmarshal([]byte(m.Metadata), &data); err != nil { + if err := common.UnmarshalJsonStr(m.Metadata, &data); err != nil { return nil }Also applies to: 90-95, 251-254
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/market.go` at line 4, Replace the direct json.Unmarshal calls in the market controller’s business logic with the appropriate common.Unmarshal or common.UnmarshalJsonStr wrapper, preserving each call’s existing input and destination behavior. Remove the now-unused encoding/json import.Source: Coding guidelines
controller/public_site.go-34-38 (1)
34-38: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not return raw database errors on public endpoints.
Lines 36 and 48 pass
err.Error()to the client. These routes are anonymous, so a database failure exposes driver text, table names, and column names. Usecommon.ApiError, which is the pattern used by the other handlers in this change, or return a fixed message and log the detail server-side.🔒 Proposed fix
if err := model.DB.Where("locale = ? AND enabled = ?", locale, true). Order("sort asc").Find(&items).Error; err != nil { - common.ApiErrorMsg(c, err.Error()) + common.ApiError(c, err) return }Also applies to: 46-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/public_site.go` around lines 34 - 38, Update the error handling in both database query branches of the public handlers, including the branch containing the shown Find call and the corresponding block around lines 46–50, so clients receive a fixed or standardized message via common.ApiError instead of err.Error(); preserve the existing early returns and ensure detailed database errors are only logged server-side if needed.controller/distributor.go-106-114 (1)
106-114: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate handlers drop the validation and defaulting that the create handlers apply. Every update handler in this change validates the enum fields only, while the matching model function writes all columns unconditionally with a
map[string]interface{}. An omitted field is therefore persisted as its zero value, and a field that the create path defaults is rejected as invalid.
controller/distributor.go#L106-L114: add thenamelength check and default an emptytierand a zerostatusasCreateDistributordoes.controller/distributor.go#L243-L254: add themodelrequired check and theinput_price/output_pricenon-negative check, and defaultcurrencyandunit.controller/sla.go#L98-L118: add thetitlelength check and default an emptyseveritytominor.controller/region_route.go#L112-L130: require a non-emptyregion, default an emptymodelto*, and default an emptystrategytoavailability.controller/team.go#L85-L93: rejectowner_id <= 0asCreateTeamdoes.Extract the shared validation into one function per resource and call it from both the create and the update handler.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/distributor.go` around lines 106 - 114, Update the shared validation used by both create and update handlers: in controller/distributor.go lines 106-114 and 243-254, controller/sla.go lines 98-118, controller/region_route.go lines 112-130, and controller/team.go lines 85-93, extract one validator per resource and invoke it from both handlers. Preserve each Create* contract: validate name/model/title/region requirements and price or owner bounds, and apply the specified defaults for distributor tier/status, currency/unit, SLA severity, and route model/strategy before persistence.common/redirect.go-16-18 (1)
16-18: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject backslashes in relative redirect targets.
Line 16 accepts any value that starts with
/and does not start with//. Major browsers normalize\to/in the URL path, so/\evil.comand/\/evil.comare treated as protocol-relative URLs and redirect off-site. The current check does not block them.Also reject control characters and whitespace inside the value, because
/\t/evil.comstyle inputs can be normalized by clients.🔒 Proposed fix
// 相对路径，但排除协议相对地址（//evil.com） if strings.HasPrefix(u, "/") { - return !strings.HasPrefix(u, "//") + if strings.ContainsAny(u, "\\\t\r\n") { + return false + } + return !strings.HasPrefix(u, "//") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/redirect.go` around lines 16 - 18, Update the relative-target validation around the strings.HasPrefix check to reject backslashes, control characters, and whitespace anywhere in the redirect value before accepting a slash-prefixed path. Preserve acceptance of ordinary single-slash relative targets while rejecting normalized protocol-relative forms such as /\\evil.com and whitespace/control-character variants.router/api-router.go-111-111 (1)
111-111: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestore
CriticalRateLimiton the access-token endpoint.
GenerateAccessTokencreates a new random key, runs a uniqueness query, and writes the user row on every call. WithoutCriticalRateLimit, one authenticated session can call it in a loop and generate unbounded write load, and it also invalidates the previous access token on each call. The PR objectives do not describe this change.🔒 Proposed fix
- selfRoute.GET("/token", middleware.DisableCache(), controller.GenerateAccessToken) + selfRoute.GET("/token", middleware.CriticalRateLimit(), middleware.DisableCache(), controller.GenerateAccessToken)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/api-router.go` at line 111, Update the /token route registration in selfRoute so it applies the existing CriticalRateLimit middleware alongside DisableCache before controller.GenerateAccessToken, preserving the endpoint’s current handler and cache behavior.model/sla.go-138-155 (1)
138-155: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winLoad only the required channel columns.
DB.Find(&channels)loads every channel row with every column, including credential fields, into memory on each call.GetSlaStatusSummarybacks the anonymous/api/sla/statusendpoint, so an unauthenticated caller can trigger a full table scan repeatedly.Restrict the query with
Select("id, name, status, response_time"), and consider aggregatingok_node_countwith aCOUNTquery instead of a Go loop.Separately, confirm that exposing channel
namevalues to anonymous callers is intended. Channel names often identify upstream providers and internal deployments.⚡ Proposed fix
var channels []Channel - if err := DB.Find(&channels).Error; err != nil { + if err := DB.Select("id, name, status, response_time").Find(&channels).Error; err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/sla.go` around lines 138 - 155, Update the channel query in GetSlaStatusSummary to select only id, name, status, and response_time, avoiding credential columns and unnecessary data loading. Preserve the existing SlaNodeStatus mapping, and verify whether exposing channel name values through the anonymous endpoint is intended before retaining that field in the response.controller/distributor.go-229-235 (1)
229-235: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope distributor price updates and deletes by distributor
router/p2-router.goregisters:id/prices/:price_id, butcontroller/distributor.go:229ignores:idand passes onlyIdintoUpdateDistributorPrice.UpdateDistributorPricethen updates whereid = ?, andDeleteDistributorPricedeletes whereid = ?. This lets/admin/distributors/OTHER_PRICES/:price_idupdate or delete price rows belonging to a different distributor. Pass the distributor:idinto both model functions and adddistributor_id = ?predicates to the update and delete queries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/distributor.go` around lines 229 - 235, Update UpdateDistributorPrice and DeleteDistributorPrice to parse and propagate the distributor :id from the route into their model calls, then modify the corresponding update and delete queries to require both the price ID and distributor_id. Preserve the existing validation and response behavior while ensuring operations cannot affect prices belonging to another distributor.model/public_site_seed.go-7-27 (1)
7-27: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate public-site seed failures.
InitPublicSiteDefaultsignores create errors and returns silently on query errors.migrateDBthen continues startup without checking the seed result. A transient database failure can leave public pricing or categories empty until manual intervention.
model/public_site_seed.go#L7-L27: return each database error instead of discarding it.model/main.go#L312-L313: handle the returned seed error and abort migration initialization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/public_site_seed.go` around lines 7 - 27, The InitPublicSiteDefaults function in model/public_site_seed.go must return an error: propagate query and Create failures instead of silently returning or discarding them, while returning success when seeding completes or is unnecessary; update model/main.go at lines 312-313 so migrateDB handles this returned error and aborts migration initialization.model/distributor.go-65-70 (1)
65-70: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
CommissionRatebefore persistence.The create controller passes
CommissionRatewithout bounds validation. These methods also accept any integer. Negative rates or rates above 100 can enter commission calculations.Reject values outside
0..100in both create and update paths.Also applies to: 109-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/distributor.go` around lines 65 - 70, Validate Distributor.CommissionRate in both CreateDistributor and the corresponding update method before database persistence, rejecting values below 0 or above 100 with an error; preserve normal persistence for values within 0..100.model/public_site_seed.go-3-3 (1)
3-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
common.Marshalfor the seed data JSON.
model/public_site_seed.godirectly callsjson.MarshalindefaultPublicPricings()anddefaultPublicModelCategories(). Usecommon.Marshalfor these serialization points instead of importing and usingencoding/jsondirectly; keepcommonas the centralized JSON marshal source.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/public_site_seed.go` at line 3, Replace the direct encoding/json usage in defaultPublicPricings() and defaultPublicModelCategories() with common.Marshal, remove the encoding/json import, and retain the existing serialization behavior and error handling.Sources: Coding guidelines, Learnings
model/team.go-21-25 (1)
21-25: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce one membership row per team and user.
CreateTeamMembercan insert duplicate(team_id, user_id)rows.ListTeamMembersthen returns duplicate members, andGetTeamBillinginflatesMemberCount.Add a composite unique index on
TeamIdandUserId.Proposed fix
- TeamId int64 `json:"team_id" gorm:"not null;index"` - UserId int64 `json:"user_id" gorm:"not null;index"` + TeamId int64 `json:"team_id" gorm:"not null;index;uniqueIndex:idx_team_member_team_user"` + UserId int64 `json:"user_id" gorm:"not null;index;uniqueIndex:idx_team_member_team_user"`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/team.go` around lines 21 - 25, Add a composite unique database index covering TeamMember.TeamId and TeamMember.UserId, using the model’s GORM tags or the project’s migration mechanism. Ensure CreateTeamMember cannot persist duplicate memberships while preserving the existing fields and behavior.model/main.go-295-308 (1)
295-308: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMigrate the analytics event table.
CreateAnalyticsEventwrites to theanalytics_eventstable withDB.Create(event).Error, butmigrateDBdoes not include&AnalyticsEvent{}inAutoMigrate. Add&AnalyticsEvent{}to this migration list so fresh databases can store analytics events.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/main.go` around lines 295 - 308, Update the AutoMigrate model list in migrateDB to include &AnalyticsEvent{} alongside the other models, ensuring fresh databases create the analytics_events table used by CreateAnalyticsEvent.model/market_model.go-75-85 (1)
75-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBind
statusas an integer.
MarketModel.Statusis an integer column, butSearchMarketModelsbinds thestatusquery string directly. PostgreSQL does not find aninteger = textoperator and rejects the filter; this also bypasses the whitelist inAllowedMarketModelStatuses. Parse and validatestatusas an integer before adding theWhereclause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/market_model.go` around lines 75 - 85, Update SearchMarketModels to parse status into an integer and validate it against AllowedMarketModelStatuses before applying the status filter; return the validation error for invalid values, and bind the parsed integer in the Where clause while preserving empty-status behavior.Source: Coding guidelines
relay/compatible_handler.go-107-107 (1)
107-107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSet the explicit size for every pass-through body.
common.ReaderOnlyhides the concrete reader type. HTTP request construction cannot infer its content length. These branches do not setinfo.UpstreamRequestBodySize, unlike the equivalent Claude branch. Set it fromstorage.Size()before assigningrequestBody.
relay/compatible_handler.go#L107-L107: Setinfo.UpstreamRequestBodySize = storage.Size().relay/gemini_handler.go#L144-L144: Setinfo.UpstreamRequestBodySize = storage.Size().relay/image_handler.go#L54-L54: Setinfo.UpstreamRequestBodySize = storage.Size().relay/rerank_handler.go#L50-L50: Setinfo.UpstreamRequestBodySize = storage.Size().relay/responses_handler.go#L85-L85: Setinfo.UpstreamRequestBodySize = storage.Size().Proposed fix
storage, err := common.GetBodyStorage(c) if err != nil { // existing error handling } +info.UpstreamRequestBodySize = storage.Size() requestBody = common.ReaderOnly(storage)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/compatible_handler.go` at line 107, Set info.UpstreamRequestBodySize to storage.Size() before assigning requestBody in the pass-through body handling at relay/compatible_handler.go:107-107, relay/gemini_handler.go:144-144, relay/image_handler.go:54-54, relay/rerank_handler.go:50-50, and relay/responses_handler.go:85-85; preserve the existing common.ReaderOnly(storage) assignments.relay/channel/task/sora/adaptor.go-219-219 (1)
219-219: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSet the raw Sora body size before returning
ReaderOnly.
DoTaskApiRequestcallsapplyUpstreamContentLengthbefore setting headers, so Sora raw-body requests withoutinfo.UpstreamRequestBodySizewill keepContent-Lengthunset or chunked. If the upstream requires an explicit length, the request can be rejected. Setinfo.UpstreamRequestBodySize = storage.Size()when returning the rawcommon.ReaderOnly(storage)path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/channel/task/sora/adaptor.go` at line 219, Update the raw-body return path in DoTaskApiRequest to assign info.UpstreamRequestBodySize from storage.Size() immediately before returning common.ReaderOnly(storage), ensuring applyUpstreamContentLength can set the explicit Content-Length.relay/channel/aws/relay-aws.go-43-47 (1)
43-47: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWire AWS invoke contexts through the request context.
newAwsInvokeContext()creates Bedrock contexts fromcontext.Background()for allawsHandler,awsStreamHandler, andhandleNovaRequestcallers, so client disconnects will no longer cancelInvokeModel/InvokeModelWithResponseStreamcalls. Whencommon.RelayTimeout <= 0, synchronous invocations also miss the local deadline. Derive the timeout/cancel fromc.Request.Context()and pass it throughnewAwsInvokeContextat each call site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/channel/aws/relay-aws.go` around lines 43 - 47, Update newAwsInvokeContext to accept a parent context and derive its timeout from that context instead of context.Background(), while preserving cancellation when RelayTimeout is non-positive by applying the local timeout behavior consistently. Pass c.Request.Context() through every awsHandler, awsStreamHandler, and handleNovaRequest call site so client disconnects cancel AWS invocations.relay/channel/api_request.go-551-554 (1)
551-554: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn a fresh body from
GetBody.
GetBodywraps the originalrequestBodyreader instead of a factory that provides a fresh replay after the first request body is written. A 307/308 redirect can therefore send an exhausted or partial request body.Remove this
GetBodyoverride, or use a replayable body factory for theio.Readercases passed intoDoTaskApiRequest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/channel/api_request.go` around lines 551 - 554, Update the request-body handling around DoTaskApiRequest and applyUpstreamContentLength so GetBody returns a fresh replayable reader for each invocation rather than wrapping the already-consumed requestBody; alternatively remove the override when the underlying request already supplies replay support. Ensure 307/308 redirects can resend the complete body for every io.Reader input.controller/model.go-279-283 (1)
279-283: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle empty Anthropic model lists.
If no model passes the token-limit and billing filters,
useranthropicModelsis empty. Lines 281 and 283 then panic on slice indexing.Return empty
first_idandlast_idwhen the list is empty. Restore an empty-list regression test.Proposed fix
+ firstID, lastID := "", "" + if len(useranthropicModels) > 0 { + firstID = useranthropicModels[0].ID + lastID = useranthropicModels[len(useranthropicModels)-1].ID + } c.JSON(200, gin.H{ "data": useranthropicModels, - "first_id": useranthropicModels[0].ID, + "first_id": firstID, "has_more": false, - "last_id": useranthropicModels[len(useranthropicModels)-1].ID, + "last_id": lastID, })The removed empty-response test covers this regression path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/model.go` around lines 279 - 283, Update the response construction in the Anthropic model-list handler to avoid indexing useranthropicModels when it is empty, returning empty first_id and last_id values in that case while preserving IDs for non-empty lists. Restore the removed empty-response regression test covering lists filtered to zero models.model/user.go-755-755 (1)
755-755: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAffiliate columns lost their write protection and their regression test in the same change. The
Omitlist inUpdateWithTxno longer excludesaff_count,aff_quota, andaff_history, and the test that detected stale overwrites of those columns was removed at the same time. A staleUservalue can now revert an affiliate increment with no test failure.
model/user.go#L755-L755: addaff_count,aff_quota, andaff_historyback to theOmitlist, and remove onlyaccess_tokenfrom the exclusions if the goal was to make the access token writable.model/user_update_test.go#L30-L63: add the affiliate fields to the fixture, mutate them in the simulated concurrent update, and assert thatstaleUser.Update(false)preserves them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/user.go` at line 755, Update model/user.go lines 755-755 in UpdateWithTx to restore aff_count, aff_quota, and aff_history in the Omit list while removing only access_token from the exclusions. In model/user_update_test.go lines 30-63, include the affiliate fields in the fixture, mutate them during the simulated concurrent update, and assert that staleUser.Update(false) preserves those values.controller/user.go-415-425 (1)
415-425: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a separate destination for the duplicate token check.
useralready hasId, so GORM adds an implicitid = ?predicate with the current user. This can miss collisions from other users and also rewritesuserwith the matched row before saving/responding. Use a zero-initializedUser{}destination and exclude current users explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/user.go` around lines 415 - 425, The duplicate-token check before user.Update must not query into the existing user object or rely on its implicit ID predicate. Use a zero-initialized User destination for the lookup, explicitly exclude the current user by ID, and preserve the existing duplicate-error response when another user already owns the token.
🟡 Minor comments (8)
Dockerfile-14-14 (1)
14-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve existing
.npmrcsettings.Line 14 replaces the copied
.npmrcwhenNPM_REGISTRYis set. This removes any scoped registry, authentication, or package-manager settings in that file. Update only theregistrysetting, or append the override instead.Proposed fix
-RUN if [ -n "$NPM_REGISTRY" ]; then echo "registry=$NPM_REGISTRY" > .npmrc; fi \ +RUN if [ -n "$NPM_REGISTRY" ]; then printf '\nregistry=%s\n' "$NPM_REGISTRY" >> .npmrc; fi \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 14, Update the Dockerfile RUN command that handles NPM_REGISTRY so it preserves the existing copied .npmrc contents while overriding or appending only the registry setting. Do not replace the file, ensuring scoped registries, authentication, and other package-manager settings remain intact.controller/region_route.go-40-43 (1)
40-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize region values when creating and updating region routes.
CreateRegionRoutewritesreq.Regiondirectly, whileUpdateRegionRoutealso writesreq.Regionthrough model updates.ResolveRegionRoutingnormalizes its input before lookup, so admins can add policies likeCNorcnthat never match.model.CreateRegionRoutedoes not callmodel.NormalizeRegion; store the normalized value in both create and update flows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/region_route.go` around lines 40 - 43, Normalize req.Region with model.NormalizeRegion before persisting it in both CreateRegionRoute and UpdateRegionRoute, while retaining the existing required-value validation. Ensure model.CreateRegionRoute and the model update path receive the normalized value so stored routes match ResolveRegionRouting lookups.model/sla.go-64-70 (1)
64-70: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winParse
statusto an integer before the comparison.
statusarrives as a raw query string and is compared to an integer column. On PostgreSQL a non-numeric value such as?status=openmakes the driver fail the type inference and the request returns a 500 error. On SQLite the comparison silently matches nothing.Convert the value in the handler or in this function, and ignore it when the conversion fails.
As per coding guidelines: "Database code must support SQLite, MySQL >= 5.7.8, and PostgreSQL >= 9.6."
🐛 Proposed fix
if status != "" { - q = q.Where("status = ?", status) + if s, err := strconv.Atoi(status); err == nil { + q = q.Where("status = ?", s) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/sla.go` around lines 64 - 70, Update SearchSlaIncidents to parse the raw status string as an integer before applying the status filter; only add the Where condition when conversion succeeds, and ignore non-numeric values without querying the integer column with the raw string. Use a standard integer parsing helper compatible with SQLite, MySQL, and PostgreSQL.Source: Coding guidelines
model/public_site_seed.go-31-52 (1)
31-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSeed locale-specific nested content.
The
enpricing records reuse feature arrays written in Chinese. Theencategory records also reuse model notes written in Chinese. English public responses therefore contain mixed-language content.Create separate English feature arrays and model metadata before assigning them to
Locale: "en"records.Also applies to: 57-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/public_site_seed.go` around lines 31 - 52, Create separate English feature arrays and English model metadata in the seed data, then assign them to all Locale: "en" pricing and category records instead of reusing the Chinese values; keep the existing Chinese arrays and metadata for Locale: "zh" records, covering the related records in the additional affected section.model/user.go-104-105 (1)
104-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA user cannot clear
RegionPreferenceor leave a team through the struct update path.
UpdateWithTxcallsUpdates(newUser)with a struct. GORM skips zero values there. A user who setsRegionPreferenceto"eu"cannot reset it to"", andTeamIdcannot return to0. Both operations are plausible admin actions.If clearing is required, write these two columns through an explicit map update or add them to a
Selectlist in the responsible handler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/user.go` around lines 104 - 105, Update the struct-based update flow in UpdateWithTx so TeamId and RegionPreference are explicitly included when writing user changes, allowing zero values to persist. Use an explicit map update or add both fields to the update Select list while preserving the existing handling for other fields.service/channel_select_test.go-24-42 (1)
24-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a case that sets the header and the request-region key together.
The doc comment states the precedence
X-Regionheader > request context region > user preference. Case 2 sets the header and the preference. Case 3 sets the request-region key and the preference. No case sets the header and the request-region key at the same time. The header-over-request-region rule is therefore not covered. The proposed table in the previous comment includes this case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/channel_select_test.go` around lines 24 - 42, Extend the detectRegion tests with a case that sets both constant.HeaderRegion and constant.ContextKeyRequestRegion to different values, optionally alongside the user preference, and assert that the header value is returned. Keep the existing cases unchanged and use the established test context setup around detectRegion.model/region_route_test.go-11-22 (1)
11-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPin
common.MemoryCacheEnabledin the fixture.
ResolveRegionRoutingbranches oncommon.MemoryCacheEnabled. The fixture does not set this global. The tests then exercise either the cached path or the direct path depending on the state left by other tests in the package. Set the value explicitly and restore it int.Cleanup, assetupUserUpdateTestStateinmodel/user_update_test.godoes forcommon.RedisEnabled.💚 Proposed fixture change
func setupRegionRouteTest(t *testing.T) { t.Helper() require.NoError(t, DB.AutoMigrate(&RegionRoute{})) require.NoError(t, DB.Exec("DELETE FROM region_routes").Error) require.NoError(t, DB.Exec("DELETE FROM channels").Error) InvalidateRegionRoutingCache() + oldMemoryCacheEnabled := common.MemoryCacheEnabled + common.MemoryCacheEnabled = true t.Cleanup(func() { + common.MemoryCacheEnabled = oldMemoryCacheEnabled DB.Exec("DELETE FROM region_routes") DB.Exec("DELETE FROM channels") InvalidateRegionRoutingCache() }) }As per coding guidelines: "Initialize database, request context, user group, settings, and cache state explicitly in test fixtures."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/region_route_test.go` around lines 11 - 22, Update setupRegionRouteTest to explicitly set common.MemoryCacheEnabled to the intended test value before exercising ResolveRegionRouting, then capture its prior value and restore it in the existing t.Cleanup alongside the database cleanup and InvalidateRegionRoutingCache call. Follow the restoration pattern used by setupUserUpdateTestState for common.RedisEnabled.Source: Coding guidelines
service/channel_select_test.go-14-60 (1)
14-60: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winConvert this new test to a testify table test.
The repository requires
testify/requirefor setup and fatal assertions andtestify/assertfor non-fatal checks in new tests. This file usest.Fatalfonly. The five scenarios also repeat the same three setup steps, so a table test with explicit expected outputs is a better fit.💚 Proposed rewrite
import ( "net/http" "net/http/httptest" "testing" "github.com/QuantumNous/new-api/constant" "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestDetectRegionFallbackToUserPreference(t *testing.T) { - // 仅设置用户区域偏好 - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Request = httptest.NewRequest(http.MethodGet, "/", nil) - c.Set(string(constant.ContextKeyUserRegionPreference), "eu") - if got := detectRegion(c); got != "eu" { - t.Fatalf("expected user preference 'eu', got %q", got) - } - // ... remaining inline cases +func TestDetectRegionFallbackToUserPreference(t *testing.T) { + cases := []struct { + name string + header string + requestRegion string + preference string + want string + }{ + {name: "preference only", preference: "eu", want: "eu"}, + {name: "header wins over preference", header: "us", preference: "eu", want: "us"}, + {name: "header wins over request region", header: "us", requestRegion: "cn", preference: "eu", want: "us"}, + {name: "request region wins over preference", requestRegion: "cn", preference: "eu", want: "cn"}, + {name: "all empty", want: ""}, + {name: "preference normalized", preference: " EU ", want: "eu"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + c.Request = httptest.NewRequest(http.MethodGet, "/", nil) + require.NotNil(t, c.Request) + if tc.header != "" { + c.Request.Header.Set(constant.HeaderRegion, tc.header) + } + if tc.requestRegion != "" { + c.Set(string(constant.ContextKeyRequestRegion), tc.requestRegion) + } + if tc.preference != "" { + c.Set(string(constant.ContextKeyUserRegionPreference), tc.preference) + } + assert.Equal(t, tc.want, detectRegion(c)) + }) + } }As per coding guidelines: "New or substantially rewritten tests must use testify/require for setup and fatal assertions and testify/assert for non-fatal checks" and "Prefer deterministic table tests with explicit expected outputs".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/channel_select_test.go` around lines 14 - 60, Convert TestDetectRegionFallbackToUserPreference into a deterministic table-driven test with explicit scenario names, contexts, and expected regions, reusing the common request/context setup for each case. Replace setup and fatal checks with testify/require, and use testify/assert for the region result assertions. Preserve all five existing precedence, empty, and normalization scenarios.Source: Coding guidelines
🧹 Nitpick comments (9)
common/redirect.go (1)
26-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the redirect allowlist configurable.
Line 27 hardcodes
91flow.com. Any deployment on another domain silently loses all absolute redirects. Read the allowed hosts from configuration (for example an option or environment variable) and keep the current value as the default. Also compare the host case-insensitively and strip the port before comparison.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/redirect.go` around lines 26 - 27, Update the redirect host validation around parsed.Host to load allowed domains from the existing configuration mechanism, defaulting to 91flow.com, while supporting subdomains. Normalize the parsed host case-insensitively and remove any port before comparing it against the configured allowlist.controller/public_site.go (1)
102-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
common.IsSafeRedirecthere.This PR adds
common.IsSafeRedirectincommon/redirect.go, but this handler re-implements the check inline. Two independent redirect validators will drift. Call the shared helper and keep one implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/public_site.go` around lines 102 - 108, Replace the inline redirect validation in the handler’s safeRedirect construction with the shared common.IsSafeRedirect helper. Preserve the existing trim-and-empty behavior and assign the trimmed redirect only when the helper accepts it, removing the duplicated strings.HasPrefix and strings.Contains checks.router/api-router.go (1)
34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister the collection routes without the trailing slash.
Lines 36 and 38 register
/api/admin/market-models/. A client that calls/api/admin/market-modelsrelies on Gin's trailing-slash redirect, which converts aPOSTinto a 307 and drops some client behaviours. The new routes inrouter/p2-router.gouse""for the collection path. Use the same form here.♻️ Proposed change
- marketModelRoute.GET("/", controller.ListMarketModels) + marketModelRoute.GET("", controller.ListMarketModels) marketModelRoute.GET("/:id", controller.GetMarketModel) - marketModelRoute.POST("/", controller.CreateMarketModel) + marketModelRoute.POST("", controller.CreateMarketModel)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/api-router.go` around lines 34 - 42, Update the collection route registrations in the marketModelRoute group, specifically ListMarketModels and CreateMarketModel, to use an empty relative path instead of a trailing slash. Keep the item routes and public market-model route unchanged, matching the collection-path convention used in router/p2-router.go.controller/team.go (1)
12-22: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound
page_size.
parsePageaccepts any positivepage_size. It is shared by the team, distributor, SLA, and region-route list endpoints. A request withpage_size=1000000makes the database return the full table and the process buffer every row. Clamp the value to a maximum, for example 100.♻️ Proposed change
if pageSize <= 0 { pageSize = 10 } + if pageSize > 100 { + pageSize = 100 + } return page, pageSize🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/team.go` around lines 12 - 22, Update parsePage so positive page_size values are capped at a maximum of 100, while retaining the existing default of 10 for non-positive or invalid values. Keep page handling and the returned pagination contract unchanged.router/p2-router.go (1)
41-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a rate limit to the anonymous SLA endpoints.
GetPublicSlaStatusruns an aggregate overPerfMetric, loads every channel row, and counts incidents on each request.GetPublicSlaIncidentsreads 50 rows. Both are anonymous and only the global API limit applies. Addmiddleware.CriticalRateLimit(), and consider caching the status summary for a short interval.♻️ Proposed change
- apiRouter.GET("/sla/incidents", controller.GetPublicSlaIncidents) - apiRouter.GET("/sla/status", controller.GetPublicSlaStatus) + apiRouter.GET("/sla/incidents", middleware.CriticalRateLimit(), controller.GetPublicSlaIncidents) + apiRouter.GET("/sla/status", middleware.CriticalRateLimit(), controller.GetPublicSlaStatus)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/p2-router.go` around lines 41 - 43, 为匿名 SLA 接口路由 /sla/incidents 和 /sla/status 添加 middleware.CriticalRateLimit()，确保两个处理函数 GetPublicSlaIncidents 与 GetPublicSlaStatus 都受限流保护；暂不扩展其他路由或行为。controller/market.go (1)
13-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the model-level allowlists instead of mirroring them.
The comment at Line 13 states that these maps mirror
model.AllowedMarketModelStatuses. Two copies of the same enum drift apart. Other controllers in this change (controller/distributor.go,controller/sla.go) reference themodelpackage allowlists directly. Do the same here and delete the local copies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/market.go` around lines 13 - 36, The controller-level allowlists in market.go duplicate model definitions and should be removed. Update the affected validation logic to reference the model package’s allowlists directly, including model.AllowedMarketModelStatuses and the corresponding unit and currency allowlists, while preserving existing validation behavior.model/region_route.go (3)
128-135: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTruncate the region by rune, not by byte.
region[:maxRegionLength]slices bytes. If the input contains multi-byte UTF-8, the result can end with a broken rune. The value is later used as a query parameter and as a cache key. Truncate on a rune boundary to keep the value well-formed.♻️ Proposed change
func NormalizeRegion(region string) string { region = strings.ToLower(strings.TrimSpace(region)) - if len(region) > maxRegionLength { - region = region[:maxRegionLength] + if runes := []rune(region); len(runes) > maxRegionLength { + region = string(runes[:maxRegionLength]) } return region }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/region_route.go` around lines 128 - 135, Update NormalizeRegion to truncate region by Unicode rune count rather than byte index, preserving valid UTF-8 when the normalized value exceeds maxRegionLength; keep the existing trimming and lowercasing behavior unchanged.
60-78: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueBoth paginated queries reuse one
*gorm.DBvalue acrossCountandFind. GORM v2 keeps statement state on the instance after a finisher method runs. Reusing it depends on GORM restoring theSELECTclause internally, and it mutates the shared value.
model/region_route.go#L60-L78: build the count query and the page query inSearchRegionRoutesfrom separateSessioninstances.model/user.go#L1427-L1434: apply the same split inGetUsersByInviterId.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/region_route.go` around lines 60 - 78, Split the count and pagination queries into separate GORM Session instances. In model/region_route.go lines 60-78, update SearchRegionRoutes so Count uses one session and the ordered Offset/Limit/Find query uses another; apply the same separation in model/user.go lines 1427-1434 within GetUsersByInviterId.
193-240: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify the tag-based channel query against the reserved-column and dialect rules.
Where("tag IN (?)", tags)uses raw SQL for thechannelstable.tagis not a reserved word, so quoting is not an issue here. However the surrounding repository convention is to usecommonGroupCol/commonKeyColhelpers for reserved columns and GORM methods where possible. Confirm thatChannel.Tagmaps to the column nametagon all three supported dialects.Also note the silent error handling: if the
Pluckquery fails, the code proceeds with the id list only and no log entry. Add a log entry so operators can detect a failing tag lookup.🔧 Proposed change for the silent error
if len(tags) > 0 { var tagged []int64 - if err := DB.Model(&Channel{}).Where("tag IN (?)", tags).Pluck("id", &tagged).Error; err == nil { + if err := DB.Model(&Channel{}).Where("tag IN (?)", tags).Pluck("id", &tagged).Error; err != nil { + common.SysLog("failed to resolve region route tags: " + err.Error()) + } else { for _, id := range tagged { if !seen[id] { seen[id] = true ids = append(ids, id) } } } }As per coding guidelines: "When raw SQL is unavoidable, account for dialect-specific quoting and values".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/region_route.go` around lines 193 - 240, Update the tag lookup in resolveRegionRouting to use the Channel.Tag field or the repository’s commonGroupCol/commonKeyCol helpers instead of unverified raw SQL, preserving correct column mapping across all supported dialects. Handle a Pluck failure by logging the query error with sufficient context before continuing with the collected ID list.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c1ab93dd-d331-4f0e-a8d4-d9e1de1bf62b
📒 Files selected for processing (70)
.dockerignoreDockerfilecommon/body_storage.gocommon/constants.gocommon/redirect.goconstant/context_key.gocontroller/distributor.gocontroller/market.gocontroller/model.gocontroller/model_list_test.gocontroller/model_owned_by_test.gocontroller/public_site.gocontroller/region_route.gocontroller/sla.gocontroller/team.gocontroller/token.gocontroller/token_test.gocontroller/user.godeploy/nginx/originflow.confdocker-compose.ymli18n/keys.goi18n/locales/en.yamli18n/locales/zh-CN.yamli18n/locales/zh-TW.yamlmiddleware/auth.gomiddleware/distributor.gomodel/ability.gomodel/analytics_event.gomodel/channel_cache.gomodel/distributor.gomodel/log.gomodel/main.gomodel/market_model.gomodel/option.gomodel/public_model_category.gomodel/public_pricing.gomodel/public_site_seed.gomodel/region_route.gomodel/region_route_test.gomodel/sales_lead.gomodel/sla.gomodel/team.gomodel/token.gomodel/user.gomodel/user_cache.gomodel/user_update_test.gorelay/alpha_search_handler.gorelay/channel/api_request.gorelay/channel/aws/relay-aws.gorelay/channel/aws/relay_aws_test.gorelay/channel/jimeng/adaptor.gorelay/channel/task/sora/adaptor.gorelay/chat_completions_via_responses.gorelay/claude_handler.gorelay/common/outbound_body.gorelay/common/relay_info.gorelay/compatible_handler.gorelay/embedding_handler.gorelay/gemini_handler.gorelay/image_handler.gorelay/rerank_handler.gorelay/responses_handler.gorouter/api-router.gorouter/p2-router.goservice/billing_usage.goservice/channel_select.goservice/channel_select_test.goservice/group.goservice/text_quota_test.gosetting/auto_group.go
💤 Files with no reviewable changes (11)
- relay/channel/jimeng/adaptor.go
- i18n/locales/zh-CN.yaml
- i18n/locales/en.yaml
- i18n/keys.go
- i18n/locales/zh-TW.yaml
- controller/model_owned_by_test.go
- middleware/auth.go
- controller/token_test.go
- setting/auto_group.go
- relay/channel/aws/relay_aws_test.go
- model/option.go
| m := &model.MarketModel{ | ||
| Model: req.Model, | ||
| Provider: req.Provider, | ||
| Category: req.Category, | ||
| Tags: req.Tags, | ||
| InputPrice: req.InputPrice, | ||
| OutputPrice: req.OutputPrice, | ||
| Unit: orDefault(req.Unit, "token"), | ||
| TrialQuota: req.TrialQuota, | ||
| Status: req.Status, | ||
| Featured: req.Featured, | ||
| Sort: req.Sort, | ||
| } | ||
| if m.Status == 0 { | ||
| m.Status = MarketModelStatusAvailable | ||
| } | ||
| if m.Currency == "" { | ||
| m.Currency = "CNY" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
CreateMarketModel drops currency and metadata.
The struct literal at Line 121 omits Currency and Metadata. Two defects follow:
- Line 137 tests
m.Currency == "", which is always true, so every created record storesCNY. A request with"currency":"USD"is validated and then discarded. req.Metadatais validated as JSON at Line 90 and then never persisted. The locale overrides thatresolveMarketModelI18nreads are lost on create and can only be added by a later update.
Line 134 is also unreachable, because validateMarketModelRequest rejects Status == 0 at Line 96.
🐛 Proposed fix
m := &model.MarketModel{
Model: req.Model,
Provider: req.Provider,
Category: req.Category,
Tags: req.Tags,
InputPrice: req.InputPrice,
OutputPrice: req.OutputPrice,
+ Currency: orDefault(req.Currency, "CNY"),
Unit: orDefault(req.Unit, "token"),
+ Metadata: req.Metadata,
TrialQuota: req.TrialQuota,
Status: req.Status,
Featured: req.Featured,
Sort: req.Sort,
}
- if m.Status == 0 {
- m.Status = MarketModelStatusAvailable
- }
- if m.Currency == "" {
- m.Currency = "CNY"
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| m := &model.MarketModel{ | |
| Model: req.Model, | |
| Provider: req.Provider, | |
| Category: req.Category, | |
| Tags: req.Tags, | |
| InputPrice: req.InputPrice, | |
| OutputPrice: req.OutputPrice, | |
| Unit: orDefault(req.Unit, "token"), | |
| TrialQuota: req.TrialQuota, | |
| Status: req.Status, | |
| Featured: req.Featured, | |
| Sort: req.Sort, | |
| } | |
| if m.Status == 0 { | |
| m.Status = MarketModelStatusAvailable | |
| } | |
| if m.Currency == "" { | |
| m.Currency = "CNY" | |
| } | |
| m := &model.MarketModel{ | |
| Model: req.Model, | |
| Provider: req.Provider, | |
| Category: req.Category, | |
| Tags: req.Tags, | |
| InputPrice: req.InputPrice, | |
| OutputPrice: req.OutputPrice, | |
| Currency: orDefault(req.Currency, "CNY"), | |
| Unit: orDefault(req.Unit, "token"), | |
| Metadata: req.Metadata, | |
| TrialQuota: req.TrialQuota, | |
| Status: req.Status, | |
| Featured: req.Featured, | |
| Sort: req.Sort, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/market.go` around lines 121 - 139, Update CreateMarketModel’s
model.MarketModel literal to copy req.Currency and req.Metadata so validated
request values are persisted and the existing currency default only applies when
no currency is provided. Remove the unreachable Status == 0 defaulting block,
since validateMarketModelRequest rejects that value; preserve the remaining
field mappings and defaults.
| func inviteUser(inviterId int) (err error) { | ||
| user, err := GetUserById(inviterId, true) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| user.AffCount++ | ||
| user.AffQuota += common.QuotaForInviter | ||
| user.AffHistoryQuota += common.QuotaForInviter | ||
| return DB.Save(user).Error |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
inviteUser now loses concurrent updates and overwrites unrelated columns.
The function reads the inviter, increments three counters in memory, and calls DB.Save(user).
Two failures follow.
-
Lost update. Two concurrent invitations for the same inviter both read the same
AffCount. The secondSaveoverwrites the first increment. The counters drift below the true value. -
Column clobbering.
DB.Savewrites every column of the loaded record, includingquota,used_quota,request_count, andauth_version. Any concurrent quota deduction or usage accounting that lands between the read and theSaveis reverted. This is an accounting-correctness failure, not only a counter drift.
Restore the atomic column expression form.
🐛 Proposed fix
func inviteUser(inviterId int) (err error) {
- user, err := GetUserById(inviterId, true)
- if err != nil {
- return err
- }
- user.AffCount++
- user.AffQuota += common.QuotaForInviter
- user.AffHistoryQuota += common.QuotaForInviter
- return DB.Save(user).Error
+ result := DB.Model(&User{}).Where("id = ?", inviterId).Updates(map[string]interface{}{
+ "aff_count": gorm.Expr("aff_count + ?", 1),
+ "aff_quota": gorm.Expr("aff_quota + ?", common.QuotaForInviter),
+ "aff_history": gorm.Expr("aff_history + ?", common.QuotaForInviter),
+ })
+ if result.Error != nil {
+ return result.Error
+ }
+ if result.RowsAffected == 0 {
+ return errors.New("inviter not found")
+ }
+ return nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@model/user.go` around lines 496 - 504, Replace the read-modify-save logic in
inviteUser with an atomic database update that increments AffCount, AffQuota,
and AffHistoryQuota using column expressions, without calling DB.Save(user).
Preserve error propagation from the update operation so concurrent invitations
and unrelated column changes are not overwritten.
|
|
||
| // 加锁查询用户以确保数据一致性 | ||
| err := lockForUpdate(tx).First(user, user.Id).Error | ||
| err := lockForUpdate(tx).First(&user, user.Id).Error |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the receiver type of TransferAffQuotaToQuota and other First(&user, ...) call sites.
ast-grep run --pattern 'func (user *User) TransferAffQuotaToQuota($_) error { $$$ }' --lang go model/user.go
rg -nP --type=go -C2 'First\(&user,'Repository: QuantumNous/new-api
Length of output: 1483
🌐 Web query:
GORM v2 First with pointer to pointer destination ErrUnsupportedDataType
💡 Result:
The ErrUnsupportedDataType error in GORM v2 typically occurs when GORM cannot map the provided destination object to a valid database schema or structure [1][2]. This often happens if the input is not a pointer to a struct or a slice, or if the pointer is nil and GORM cannot determine the underlying type [1][3]. Regarding the use of a pointer to a pointer destination (e.g., user), GORM generally expects a single-level pointer to a struct when using methods like First [4]. Using a pointer-to-pointer often confuses the reflection logic that GORM uses to parse models and destination types [1][5]. Key technical reasons and patterns to observe: 1. Requirement for Pointers: GORM requires a pointer to a struct or slice to successfully perform reflection and map database columns to fields [3][2]. If you pass a pointer to a pointer, the reflection logic (often found in schema.Parse) may fail to identify the target struct type, resulting in the unsupported data type error [1][2]. 2. Initialization: Ensure that the destination variable is properly initialized as a pointer to the type you intend to store. For example, use var user *User followed by db.First(&user) [4]. 3. Alternatives: - If you need to scan into a pointer (e.g., to handle nullable results), ensure the target type itself is a pointer to the struct rather than a pointer to a pointer variable [6]. - If you are attempting to use a nil pointer, GORM sometimes struggles to initialize it unless it is a pointer to a defined struct [4][2]. - If you are dealing with complex types or specific database column mappings, wrap your destination in a struct to provide GORM with a clear schema [7]. If you are encountering this error, verify that you are passing &user where user is defined as the struct type itself (e.g., var user User), as this is the standard pattern for GORM operations [4]. If you must use a pointer, ensure it is initialized, such as user:= &User{} [4]. Avoid passing double pointers like User, as this falls outside the standard supported input types for GORM's reflection-based API [3].
Citations:
- 1: ErrUnsupportedDataType go-gorm/gorm#3000
- 2: https://github.com/go-gorm/gorm/blob/40f4afe8c21d96db63174bd501fb61d6e73c5587/schema/schema.go
- 3: invalid value, should be pointer to struct or slice go-gorm/gorm#6696
- 4: Retrieving gorm object support pointer not only for struct go-gorm/gorm#4003
- 5: https://www.programmersought.com/article/739510593832/
- 6: https://github.com/go-gorm/gorm/blob/master/tests/scan_test.go
- 7:
Scanpanics when passed a pointer to pointer to array go-gorm/gorm#5621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Go/GORM availability =="
go version || true
go list -m github.com/go-gorm/gorm || true
echo
echo "== model user receiver and First call =="
sed -n '1,80p' model/user.go
sed -n '480,545p' model/user.go
echo
echo "== lockForUpdate definition/usages =="
rg -n -C2 'func lockForUpdate|lockForUpdate\(tx\)' --type=go .
echo
echo "== read-only verifier for pointer type/argument expression =="
python3 - <<'PY'
from pathlib import Path
text = Path('model/user.go').read_text()
target = 'err := lockForUpdate(tx).First(&user, user.Id).Error'
method = 'func (user *User) TransferAffQuotaToQuota'
if method in text and target in text:
print('receiver_is_user_pointer_to_Receiver_User=True')
print('calls_First_with_address_of_receiver=True')
else:
print('matcher_failed')
PYRepository: QuantumNous/new-api
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== short repository excerpts =="
wc -l model/user.go
sed -n '1,25p' model/user.go
sed -n '500,525p' model/user.go
echo
echo "== lockForUpdate definition/usages =="
rg -n -C2 'func lockForUpdate|lockForUpdate\(tx\)' --type=go . || true
echo
echo "== target expression literal =="
python3 - <<'PY'
from pathlib import Path
text = Path('model/user.go').read_text()
print(text.count('func (user *User) TransferAffQuotaToQuota'))
print(text.count('lockForUpdate(tx).First(&user, user.Id).Error'))
PYRepository: QuantumNous/new-api
Length of output: 11954
Pass the User receiver directly to First.
TransferAffQuotaToQuota has receiver user *User, so &user is a **User; First expects a pointer to a struct, which makes this locked read reject the query before the balance check runs.
🐛 Proposed fix
- err := lockForUpdate(tx).First(&user, user.Id).Error
+ err := lockForUpdate(tx).First(user, user.Id).Error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err := lockForUpdate(tx).First(&user, user.Id).Error | |
| err := lockForUpdate(tx).First(user, user.Id).Error |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@model/user.go` at line 521, Update the locked read in TransferAffQuotaToQuota
to pass the User receiver directly to lockForUpdate(tx).First, avoiding the
current pointer-to-pointer argument while preserving the existing user ID lookup
and balance-check flow.
| if err := q.Order("id DESC").Offset(offset).Limit(pageSize). | ||
| Select("id, username, email, quota, used_quota, status, group, created_at, inviter_id, team_id"). | ||
| Find(&items).Error; err != nil { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the helper name and how other call sites quote the reserved group column.
rg -nP --type=go -C2 'commonGroupCol'Repository: QuantumNous/new-api
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -u
echo "== files =="
git ls-files | rg '(^model/user\.go$|^model/common.*\.go$|common\.go$|distributor\.go$)' || true
echo "== commonGroupCol/groupCol references =="
rg -n 'GroupCol|groupCol|common.*Group|`group`|"\bgroup\b"|\[.*group.*\]|SELECT\s+.*group' --type=go . || true
echo "== relevant model/user.go section =="
sed -n '1400,1470p' model/user.go 2>/dev/null || true
echo "== relevant distributor call sites =="
rg -n -C 3 'GetUsersByInviterId|ListDistributorSubUsers' --type=go . || true
echo "== gorm imports in user.go =="
sed -n '1,120p' model/user.go 2>/dev/null || trueRepository: QuantumNous/new-api
Length of output: 29922
Quote group in GetUsersByInviterId.
GetUsersByInviterId passes group as a bare identifier and is reached from ListDistributorSubUsers. Other model queries use commonGroupCol for the users.group column because GROUP is reserved on MySQL and PostgreSQL, and Select(...) does not quote these column names automatically. Replace the group identifier with commonGroupCol before calling Find.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@model/user.go` around lines 1435 - 1437, Update GetUsersByInviterId’s Select
column list to use commonGroupCol instead of the bare group identifier before
Find, preserving the existing selected fields and query behavior.
Source: Coding guidelines
51fdfc5 to
2b6f1df
Compare
1 participant
Summary
Backend / infrastructure portion of the OriginFlow (元点流商) secondary-development work, split out from the combined originflow merge for easier review. The companion frontend PR carries
web/src.Region-aware channel routing
model/region_route.go,model/channel_cache.go,model/ability.go,service/channel_select.go: resolve channels by normalized request region, cache results, score by strategy, filter candidates, preserve fallback.Model Market (Backend)
Sales leads + analytics
model/analytics_event.go: analytics event model + helpers.Originflow infra
common/redirect.go(IsSafeRedirect),deploy/nginx/originflow.conf,OriginFlowSystemNamebranding incommon/constants.go.www/app/apihosts.Notes
go build(CGO_ENABLED=0) passes. Frontend build output (web/dist) is generated during CI and embedded into the binary.Summary by CodeRabbit