fix: harden stream billing and affinity handling - #6185
Closed
lingozhi wants to merge 48 commits into
Closed
Conversation
added 30 commits
May 7, 2026 15:44
…clients (Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>)
…ploads The OpenAI image-edits adaptor previously only checked mf.File['image'], returning 'image is required' when clients sent image=<URL> or image=<data:URI> as multipart text fields. The downstream cf-worker already handles these formats via collectImageFields + normalizeImageInput, but new-api was rejecting them at the gateway. This change: - Forwards all mf.Value text fields (including image / image[] / image[N]) to the outgoing multipart body so worker can fetch/inline them. - Only rejects when no file uploads AND no http(s):// or data: text values are present. - Improves the error message to enumerate the three supported input modes.
- Gateway: in OpenaiHandlerWithUsage (image edits/generations only), flush headers + a single whitespace byte BEFORE io.ReadAll. The upstream model can hold the connection 60-120s; without an early byte, CF in front of api.opwan.ai returns 524 at the 100s mark. JSON parsers ignore leading whitespace so the eventual envelope still parses cleanly on the client. - Worker: extract buildImagesDataFromResponses helper shared by handleImagesEdits and handleImagesGenerations to avoid duplicating the image_generation_call iteration / R2 upload / envelope-shaping block. Thread ctx through the image handlers in case future fixes need waitUntil-style hooks.
Upstream gpt-image-2 accepts output_format=webp but silently returns PNG bytes. The previous inferExt(claimed, b64) trusted `claimed` first, producing R2 URLs ending in .webp whose body was actually PNG — breaking webp-aware clients that key off content-type or extension. - inferExt now sniffs the b64 magic bytes first (iVBOR/9j//UklG/R0lGOD) and only falls back to the claimed format if the bytes are unknown. - Envelope output_format now reflects the actual format detected, so callers see what they really got instead of what they asked for. - firstFormat is derived from the inferred ext, not item.output_format. Net effect: webp requests still return whatever upstream produces (PNG in practice), but URLs and metadata now match the real bytes.
- Add docs/xixiapi-gpt-image-2.md — the upstream spec we measured against (size constraints, format/quality matrix, edit mode behavior, perf data). - Update new-api.Apifox.json /v1/images/edits description: image field forms, multi-image syntax, webp output downgrade, retry guidance. - Add /tmp to .gitignore so local stress-test artifacts stay out.
…rations
Routes /v1/images/generations requests for any gpt-image-* model through
a new in-process handler that:
1. Re-shapes the OpenAI Images-API request into a /v1/responses payload
with stream:true
2. POSTs directly to the configured channel base_url (no worker hop)
3. Aggregates the SSE stream in Go, skipping huge partial_image events
4. Uploads the final image to R2 via the Cloudflare REST API (single
authenticated PUT; no S3 SDK / aws-sdk-go needed)
5. Builds the classic Images-API envelope and writes it to the client
6. Triggers the standard quota-consume billing path
An "early flush" emits response headers + a single whitespace byte
before the upstream call so the CF edge in front of api.opwan.ai sees
TTFB inside its 100s window even though the upstream model takes
60-150s. JSON parsers ignore leading whitespace so the eventual body
parses cleanly.
R2 access is configured via four env vars:
CLOUDFLARE_R2_API_TOKEN, _ACCOUNT_ID, _BUCKET, _PUBLIC_BASE
Gating is at the relay/image_handler.go layer and currently fires only
on RelayModeImagesGenerations + gpt-image-* model. /v1/images/edits
continues through the existing worker-relayed path until Phase 3.
Layout:
relay/channel/openai/image_stream/
handler.go — orchestration: build request, post, aggregate, upload, write
r2.go — R2 PUT via Cloudflare REST API + magic-byte ext sniffing
sse.go — SSE aggregator (skips partial_image, captures completed)
request.go — /v1/responses payload builder for generations
Adds multipart/form-data handling for /v1/images/edits when the model
matches the gpt-image-* family. The handler now dispatches by relay
mode and shares the upstream-call / SSE aggregation / R2 upload flow:
generations → buildGenerationsRequest (input is the prompt string)
edits → buildEditsRequest (input is a user message with
input_text + N input_image content parts)
input_normalizer.go centralizes image-source handling:
- multipart file uploads (image=@/path/file.png)
- http(s) URLs (image=https://...)
- data:URIs (image=data:image/png;base64,...)
- multi-image syntaxes: image / image[] / image[N]
- max 16 images per request, ≤25 MiB each, png/jpeg/webp only
- magic-byte mime sniffing wins over declared Content-Type so
upstream sees the right type when clients lie about it
Other fixes in this commit:
- Add writeError() calls on the early-error paths (NewRequestWithContext,
httpClient.Do failure) so clients see a JSON error envelope instead of
a hung connection that closes after just the early-flush whitespace.
- Drop unused constant import + dead `var _` placeholder from handler.go.
- Gating in image_handler.go now matches both ImagesGenerations and
ImagesEdits when the model is gpt-image-*.
Validated Phase 2 in production:
1024×1024 medium ✓ 18s 1.2 MB PNG
2560×1440 high ✓ 62s 7.5 MB PNG
2048×2048 high ✓ 215s 9.9 MB PNG (broke past CF 100s)
4K UHD high ✓ 67s, structured upstream error (retryable)
12-concurrent stress on generations: 7 ✓ + 5 503 (upstream overload),
zero CF 524 timeouts — the architectural goal of this rewrite.
The 4 Cloudflare Workers (image-relay, -lx, -luc, -md) have been deleted from the CF account. All gpt-image-* traffic now flows through the new in-process Go SSE aggregator (relay/channel/openai/image_stream). Other models (dall-e-*, etc.) continue through the standard adaptor path without any worker hop.
The upstream /v1/responses payload splits cost across two places: - response.usage LLM reasoning only (~40-200 tokens) - tool_usage.image_gen.* actual image cost (often thousands) Previously the handler forwarded only response.usage, so new-api's record_consume_log showed prompt_tokens=1 / completion_tokens=0 even when a real 4K image was rendered. mergeUsage() now sums both halves and mirrors the responses-API counters (input_tokens/output_tokens) into the legacy PromptTokens/CompletionTokens fields the billing path keys off, plus per-modality details (image_tokens / text_tokens) so logs can attribute cost correctly. Also surfaces the merged usage in the response envelope so clients see realistic numbers, not just the reasoning slice.
Upstream returns response.background as a bool (`false`) in some events
and as a string (`"opaque"`) in others. Declaring it as string failed
the whole response.completed unmarshal with:
json: cannot unmarshal bool into Go struct field
UpstreamResponse.response.background of type string
That single field's type mismatch was silently dropping the entire
response.completed event — leaving snapshot.Usage / snapshot.ToolUsage
both nil, so billing logged prompt_tokens=1 / completion_tokens=0
even on successful generations.
Removed the field from UpstreamResponse since the envelope doesn't
need it (the client controls background on input). Usage and
tool_usage now flow through correctly.
…s off
Codex CLI and openai-python helpers raise "stream disconnected before
completion: stream closed before response.completed" when the upstream
Responses stream ends without emitting response.{completed,failed,
incomplete}. Reasoning-heavy models (gpt-5.x) hit this regularly: long
silent reasoning windows trigger STREAMING_TIMEOUT, and upstream
providers occasionally drop the connection mid-stream.
OaiResponsesStreamHandler now tracks whether a terminal event arrived
and synthesizes one at scanner exit: response.completed (with local
usage estimate) for graceful EOF with any output, response.failed
(with EndReason as error.code) otherwise. Client-gone path is a no-op.
The synthesized payload uses map[string]any so we emit the exact wire
fields Codex's Rust parser reads (id + usage.{input,output,total}_tokens
plus details) without going through dto.IncompleteDetails (whose JSON
tag is misnamed "reasoning" instead of "reason").
Codex CLI is hitting POST /v1/responses and getting 400 "invalid JSON request body" but a manually-crafted request with the same key and a minimal Responses-API body succeeds. Log the first 512 bytes of the offending body plus Content-Type so we can see what the client actually sends. Will be reverted once diagnosed.
Codex CLI 0.133+ sends POST /v1/responses request bodies with Content-Encoding: zstd. DecompressRequestMiddleware only handled gzip and br, so the raw zstd bytes (magic 28 B5 2F FD) reached the JSON parser and every request 400'd with "invalid JSON request body". Add a zstd branch using klauspost/compress/zstd, already in the module graph as an indirect dependency. Also drop the temporary body-dump log in the distributor now that the cause is confirmed.
…rite new-api auto-rewrites /v1/responses/compact requests' model field to <base>-openai-compact so admins can route and bill compact traffic separately. That's overhead when the upstream relay treats compact and regular Responses calls identically (xtokenmirror, hostcentral, most codex-compatible proxies) — every channel then needs to declare the suffixed variant in its model list AND a *-openai-compact price entry. When COMPACT_USE_BASE_MODEL=true, skip the rewrite so compact traffic routes via the base model's channel and price config. Default behavior preserved.
Temporarily cool down balance-exhausted upstream channels and strip incompatible max_output_tokens per channel so retries avoid known-bad routes without permanently disabling channels. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Keep compact responses billing on the base upstream model when COMPACT_USE_BASE_MODEL is enabled, and add stream termination diagnostics for client_gone investigations. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Automatically cool a channel for one hour when repeated stream transport failures indicate intermittent upstream instability, while ignoring normal client cancellations. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Convert malformed OpenAI-compatible Claude terminal streams into upstream channel errors, patch Responses terminal output shape for Codex, and temporarily cool unstable upstream channels without penalizing client/auth errors. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When every candidate channel for a group/model is cooling down, the selector returned no channel, producing a distributor-stage 503 (无可用渠道) that is upstream of the relay retry loop and therefore never retried. Split candidates into available vs cooling and fall back to the cooling set when no non-cooling channel exists, so the request still attempts an upstream instead of hard-failing.
A 403 like 'Image generation is not enabled for this group' is a per-channel capability gap, not a client error, but the blanket 4xx gate in ShouldCooldownChannelForUpstreamError skipped cooldown. The channel kept being re-selected — retried 3x within one request (21s hangs) and across requests — thrashing the pool and spilling over onto unrelated Codex traffic in the same group. Add capabilityCooldownKeywords checked before the 4xx gate so these cool for 15m and get skipped on retry.
Channels temporarily cooled down (auto-cooldown for transient upstream errors) were indistinguishable from healthy ones in the admin UI. Expose the in-memory cooldown state (reason + expiry) via GetChannelCooldown and annotate channel list/search/detail responses with cooling_down, cooldown_reason, cooldown_expires (transient, gorm:"-"). Both themes render a warning badge next to the status with remaining minutes and a tooltip showing the reason and recovery time, mirroring the existing auto-disabled (status=3) reason tooltip.
Two additions so misbehaving channels leave the selection pool faster: - Any error that sends the request to retry another channel now cools the failing channel for the full ChannelCooldownDuration (30m). isRetryableChannelError mirrors shouldRetry's error classification (channel errors, retryable status codes, excluding skip-retry/pinned/client 4xx). This subsumes the previous 15m upstream-5xx / capability-4xx path for retryable cases; non-retryable cool-worthy errors (skip-retry malformed bodies) still use the 15m path. - A request that ultimately succeeds but whose first-response-time exceeds SlowChannelFRTThreshold (30s) cools the channel 30m. FRT (not total elapsed) is used so large prompts / high-reasoning requests that stream promptly are not punished; pinned-channel and non-streamed requests without a measured first response are skipped.
… gaps The blanket 30m cooldown on any retryable error sidelined channels that only briefly blipped (a single upstream 5xx) for the full duration, which hurts recovery when the whole pool is flapping. Split CooldownChannelForRetry: transient retryable failures (mostly 5xx) now cool for ShortChannelCooldownDuration (5m) so a recovered channel rejoins rotation fast, while structural per-channel capability gaps (e.g. image generation disabled) still cool 30m since a quick retry won't fix them. Balance/quota (30m) and slow-channel (30m) unchanged. Extracted isCapabilityError and reused it in ShouldCooldownChannelForUpstreamError.
Fix stream cancellation and empty-stream handling, bind upstream requests to client contexts, preserve zero-token fixed pricing and tool surcharges, normalize context-limit retries, secure model fetching, and support Claude file content.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Bound upstream response-header waits, allow transient affinity failures to use healthy alternatives, exclude attempted and cooling channels during retries, and make default CLI affinity rules soft.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Propagate downstream stream write failures, make quota conversions saturating and observable, preserve RWMap state on decode errors, and retain Claude affinity billing headers.
Contributor
WalkthroughThis PR adds quota clamping, channel cooldown and retry selection, streaming lifecycle safeguards, GPT image handling, Claude token counting and media conversion, Responses compatibility changes, API documentation, and frontend pricing, metadata, cooldown, and branding updates. ChangesCore relay behavior
Image and API compatibility
Frontend metadata and presentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Contributor
Author
|
Closing because the fork branch contains unrelated historical divergence from upstream/main. I will replace this with a clean, upstream-based PR containing only the focused runtime fixes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
controller/channel.go (1)
1134-1140: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
common.Unmarshalinstead ofencoding/jsondirectly.As per coding guidelines, avoid using
encoding/jsondirectly in business logic for parsing payloads. Please read the response body bytes and utilizecommon.Unmarshalto maintain consistency across the codebase.♻️ Proposed fix
- if err := json.NewDecoder(limitedBody).Decode(&result); err != nil { - c.JSON(http.StatusInternalServerError, gin.H{ - "success": false, - "message": err.Error(), - }) - return - } + bodyBytes, err := io.ReadAll(limitedBody) + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{ + "success": false, + "message": "Failed to read response body: " + err.Error(), + }) + return + } + if err := common.Unmarshal(bodyBytes, &result); err != nil { + c.JSON(http.StatusInternalServerError, gin.H{ + "success": false, + "message": "Failed to parse models: " + err.Error(), + }) + return + }🤖 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/channel.go` around lines 1134 - 1140, Update the response decoding flow around the JSON decoder to read the limited response body into bytes and parse it with common.Unmarshal instead of encoding/json directly. Preserve the existing error response and early return behavior when reading or unmarshalling fails.Sources: Coding guidelines, Learnings
relay/channel/openai/relay-openai.go (1)
572-608: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftEarly-flushed 200 response can be left malformed on read/unmarshal failure.
c.Writer.WriteHeader(resp.StatusCode)+Flush()commit the200status to the wire beforeio.ReadAllandcommon.Unmarshalrun. If either fails (plausible exactly for the 60-120s upstream calls this change targets), the function still returns a*types.NewAPIErrorwith a different status code — but the status line is already locked to200, so the caller's error-response path can only append body bytes after our leading whitespace, delivering a200 OKwith a broken/inconsistent body instead of a clean error to the client.Once
earlyFlushedis true, these two error returns need to degrade gracefully (e.g., write a best-effort error envelope in the body while keeping the already-committed200, or short-circuit so the caller does not attempt to also write its own error response) rather than trying to convey a different HTTP status.🩹 Illustrative direction (needs coordination with the caller's error-handling path)
responseBody, err := io.ReadAll(resp.Body) if err != nil { - return nil, types.NewOpenAIError(err, types.ErrorCodeReadResponseBodyFailed, http.StatusInternalServerError) + if earlyFlushed { + logger.LogError(c, "read response body failed after early flush: "+err.Error()) + // status is already committed to 200; do not attempt a status-changing error response. + return nil, types.NewOpenAIError(err, types.ErrorCodeReadResponseBodyFailed, resp.StatusCode) + } + return nil, types.NewOpenAIError(err, types.ErrorCodeReadResponseBodyFailed, http.StatusInternalServerError) } var usageResp dto.SimpleResponse err = common.Unmarshal(responseBody, &usageResp) if err != nil { - return nil, types.NewOpenAIError(err, types.ErrorCodeBadResponseBody, http.StatusInternalServerError) + if earlyFlushed { + logger.LogError(c, "unmarshal response body failed after early flush: "+err.Error()) + return nil, types.NewOpenAIError(err, types.ErrorCodeBadResponseBody, resp.StatusCode) + } + return nil, types.NewOpenAIError(err, types.ErrorCodeBadResponseBody, http.StatusInternalServerError) }Note: the caller of
OpenaiHandlerWithUsagealso needs to avoid re-writing a status-changing error response when the returned error's status already matches an early-flushed status, otherwise the double-write problem persists further up the call stack.🤖 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/openai/relay-openai.go` around lines 572 - 608, Update the early-flushed error paths in OpenaiHandlerWithUsage for io.ReadAll and common.Unmarshal failures: once earlyFlushed is true, do not return an error that causes the caller to write a conflicting status or duplicate response. Instead, write a best-effort error envelope after the committed leading whitespace or return a caller-recognizable result, and update the caller’s error-handling path to avoid rewriting a status-changing response when the early-flushed 200 is already committed.relay/common/stream_status.go (1)
161-195: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
defer s.mu.Unlock()inSummary()for consistency and panic-safety.Every other method in this file (
RecordError,RecordDataReceived,SetEndReasonWithSource,Snapshot,IsNormalEnd) unlocks viadefer.Summary()locks then unlocks manually at the end; if anyfmt.Fprintfcall between them were to panic, the mutex would stay locked, deadlocking every other call on thisStreamStatusfor the rest of the request.🔒️ Proposed fix
b := &strings.Builder{} s.mu.Lock() + defer s.mu.Unlock() fmt.Fprintf(b, "reason=%s", s.EndReason) @@ if s.ErrorCount > 0 { fmt.Fprintf(b, " soft_errors=%d", s.ErrorCount) } - s.mu.Unlock() return b.String() }🤖 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/common/stream_status.go` around lines 161 - 195, Update StreamStatus.Summary to defer s.mu.Unlock() immediately after acquiring s.mu, and remove the manual unlock at the end while preserving the existing summary formatting and return behavior.
🧹 Nitpick comments (13)
docs/xixiapi-gpt-image-2.md (3)
197-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Consider specifying
textorhttpas the language to resolve the markdown linter warning (MD040).🔨 Proposed fix
-``` +```text event: response.image_generation_call.partial_image data: { "type": "response.image_generation_call.partial_image",🤖 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 `@docs/xixiapi-gpt-image-2.md` around lines 197 - 200, Add a language identifier to the fenced code block containing the partial image event example, using text or http consistently with the snippet’s content to satisfy the markdown linter.Source: Linters/SAST tools
399-402: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Consider specifying
textas the language to resolve the markdown linter warning (MD040).🔨 Proposed fix
-``` +```text 客户端上传图 → 你的中转 ↓ ┌─ 客户端给 URL 且公网可达 → 直接透传 URL 给 xixiapi(最省带宽)🤖 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 `@docs/xixiapi-gpt-image-2.md` around lines 399 - 402, Specify the text language on the fenced code block containing the upload-flow diagram, changing its opening fence to use text while preserving the diagram content unchanged.Source: Linters/SAST tools
162-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Consider specifying
textas the language to resolve the markdown linter warning (MD040) and improve syntax highlighting behavior across different viewers.🔨 Proposed fix
-``` +```text id, object, model, status, created_at, completed_at, output, error, incomplete_details,🤖 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 `@docs/xixiapi-gpt-image-2.md` around lines 162 - 164, Update the fenced code block containing the response field list near id, object, and model to specify text as its language, preserving the existing content.Source: Linters/SAST tools
relay/helper/common.go (1)
41-43: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNil
cis treated as "not done," inconsistent withFlushWriter's nil guard.
FlushWriterin this same file explicitly treats a nil*gin.Contextas a safe no-op.requestContextDonereturnsfalsefor nilc(meaning callers proceed), which would then hitrenderCustomEvent(c, ...)→c.Writeron a nil receiver. Not currently reachable from real gin handlers, but worth aligning with the established nil-safety convention in this file.🛡️ Suggested fix
func requestContextDone(c *gin.Context) bool { - return c != nil && c.Request != nil && c.Request.Context().Err() != nil + return c == nil || c.Writer == nil || (c.Request != nil && c.Request.Context().Err() != nil) }Note this would also change the semantics for callers returning an error message referencing
c.Request.Context().Err(), so the error-construction lines would need a nil-safe guard too.🤖 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/helper/common.go` around lines 41 - 43, Update requestContextDone to treat a nil *gin.Context as done, matching FlushWriter’s nil-safety behavior and preventing subsequent rendering through a nil context. Also update any callers that construct errors from c.Request.Context().Err() to guard nil c and avoid dereferencing it.relay/channel/openai/image_stream/request.go (1)
52-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
buildEditsRequesthas 10 positional parameters, 6 of the same type.Same-typed string parameters (
size, quality, outputFormat, background, moderation) in a long positional signature are easy to transpose silently at the call site with no compiler help. Consider grouping into a small options struct.#!/bin/bash rg -n 'buildEditsRequest\(' relay/channel/openai/image_stream🤖 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/openai/image_stream/request.go` at line 52, Refactor buildEditsRequest to accept a dedicated options struct grouping the related edit settings (size, quality, outputFormat, background, moderation, and outputCompression) instead of the long positional parameter list. Update every buildEditsRequest call site in the image stream package to construct and pass this struct, preserving the existing values and request behavior.controller/claude_count_tokens_test.go (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
assertfor non-fatal value checks.As per coding guidelines, "New or substantially rewritten Go backend tests must use
requirefor setup and fatal assertions andassertfor non-fatal value checks." Please useassert(and add"github.com/stretchr/testify/assert"to your imports) for these final test assertions.
controller/claude_count_tokens_test.go#L41-L41: changerequire.Greatertoassert.Greater.relay/helper/common_disconnect_test.go#L24-L25: changerequire.ErrorIsandrequire.Emptytoassert.ErrorIsandassert.Empty.🤖 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/claude_count_tokens_test.go` at line 41, The final value checks in controller/claude_count_tokens_test.go lines 41-41 and relay/helper/common_disconnect_test.go lines 24-25 should be non-fatal: replace require.Greater with assert.Greater, and require.ErrorIs/require.Empty with assert.ErrorIs/assert.Empty, adding the testify/assert import in both affected test files as needed.Source: Coding guidelines
model/ability.go (1)
141-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated priority-sort/retry-clamp logic in both channel-selection paths; retry/exclusion interaction needs confirmation. Both
GetChannelWithOptionsandGetRandomSatisfiedChannelWithOptionsindependently build a unique-priority list from the post-exclusion-filtered candidate set and then clampretryas an index into it. Whether this is correct depends on the caller (service/channel_select.go→controller/relay.go, the latter not in this review batch) only advancingretryonce a priority tier is fully exhausted, while retrying within a tier solely via growingExcludedChannelIDs.
model/ability.go#L141-L159: confirm the retry/exclusion contract with the caller incontroller/relay.go, and extract the unique-priority build+sort+clamp block into a shared helper reused bymodel/channel_cache.go.model/channel_cache.go#L162-L178: same retry/exclusion contract applies here (Channel-based variant); consume the same extracted helper once available.🤖 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 141 - 159, The unique-priority construction, descending sort, and retry clamping duplicated in model/ability.go lines 141-159 and model/channel_cache.go lines 162-178 must be extracted into one shared helper and reused by both channel-selection paths. Confirm against service/channel_select.go and controller/relay.go that retry advances only after a priority tier is exhausted, while ExcludedChannelIDs retries remain within the current tier; preserve that contract in the helper’s behavior and both callers.service/channel_cooldown_test.go (1)
1-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse testify
require/assertinstead of rawt.Fatalf.Test logic itself is correct and matches
service/channel_cooldown.gobehavior, but every assertion here usest.Fatalfdirectly.♻️ Example fix pattern
- reason, expires, cooling := model.GetChannelCooldown(9001) - if !cooling { - t.Fatalf("expected retryable 5xx error to cool the channel") - } - if !strings.Contains(reason, "retryable_transient") { - t.Fatalf("expected retryable_transient reason, got %q", reason) - } + reason, expires, cooling := model.GetChannelCooldown(9001) + require.True(t, cooling, "expected retryable 5xx error to cool the channel") + assert.Contains(t, reason, "retryable_transient")As per path instructions: "New or substantially rewritten Go backend tests must use
requirefor setup and fatal assertions andassertfor non-fatal value checks."🤖 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_cooldown_test.go` around lines 1 - 108, Update all tests in the channel cooldown test file to use testify assertions: use require for setup or fatal conditions such as cooldown state, and assert for non-fatal reason and duration checks. Add the testify dependency import and replace each raw t.Fatalf while preserving the existing test expectations and logic.Source: Path instructions
relay/helper/price.go (1)
88-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated error-check boilerplate around strict quota conversion.
The
if err != nil { return types.PriceData{}, err }guard after each strict/rounding call is duplicated 5 times. Could be tightened with a small local helper, but it's cosmetic and low priority given the current one-caller-per-branch nature of each call site.Also applies to: 116-127, 204-208, 217-221, 283-286
🤖 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/helper/price.go` at line 88, Optionally consolidate the repeated strict quota conversion error checks in the surrounding price-building function using a small local helper, covering the call sites associated with quotaErr and the noted strict/rounding conversions. Preserve each existing conversion, branch behavior, and return of types.PriceData{} with the original error; avoid broader refactoring.web/default/src/stores/system-config-store.ts (1)
58-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeduplicate
normalizeLogoUrlhelper function.The
normalizeLogoUrlfunction introduced here is an exact duplicate of the one added inweb/default/src/hooks/use-system-config.ts. To uphold DRY principles, consider moving this function intoweb/default/src/lib/constants.tsnext toDEFAULT_LOGOand exporting it for use in both the store and the hook.Also applies to: 87-95, 107-127
🤖 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 `@web/default/src/stores/system-config-store.ts` around lines 58 - 63, Move the duplicated normalizeLogoUrl helper from system-config-store.ts and use-system-config.ts into lib/constants.ts alongside DEFAULT_LOGO, exporting it from there. Update both callers to import and reuse the shared helper, preserving its current behavior and eliminating the local definitions.web/default/src/features/pricing/lib/model-metadata.ts (1)
91-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated capabilities/modalities into a shared constant.
All three override entries repeat the exact same
input_modalities,output_modalities, and 9-itemcapabilitiesarray. A shared constant avoids the three copies drifting apart when Claude's capability list changes.♻️ Proposed fix
+const CLAUDE_VISION_CAPABILITIES: ModelCapability[] = [ + 'streaming', + 'system_prompt', + 'function_calling', + 'tools', + 'json_mode', + 'structured_output', + 'vision', + 'reasoning', + 'code_interpreter', +] +const CLAUDE_VISION_MODALITIES = { + input: ['text', 'image'] as Modality[], + output: ['text'] as Modality[], +} + const MODEL_METADATA_OVERRIDES: Record<...> = { 'claude-sonnet-4-6': { context_length: 1_000_000, max_output_tokens: 64_000, - input_modalities: ['text', 'image'], - output_modalities: ['text'], - capabilities: [ ... ], + input_modalities: CLAUDE_VISION_MODALITIES.input, + output_modalities: CLAUDE_VISION_MODALITIES.output, + capabilities: CLAUDE_VISION_CAPABILITIES, }, // ...repeat for the other two entries }🤖 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 `@web/default/src/features/pricing/lib/model-metadata.ts` around lines 91 - 157, Extract the shared input_modalities, output_modalities, and capabilities values from MODEL_METADATA_OVERRIDES into a reusable constant, then spread or reference that constant in each Claude override entry while preserving their model-specific limits.controller/relay.go (1)
393-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isRetryableChannelErrorduplicatesshouldRetry's classification logic almost verbatim.Both functions implement the same skip-retry/semantic-error/affinity/status-code cascade; only the
retryTimes <= 0gate differs. The doc comment already acknowledges this mirroring. Any future change to one classification rule (e.g. a new always-skip code, a new semantic phrase) risks being applied to only one of the two, silently diverging retry vs. cooldown behavior.♻️ Proposed fix: extract the shared classification
+// classifyChannelRetryError applies the shared error-classification rules used +// by both shouldRetry and isRetryableChannelError, excluding the +// remaining-retry-count gate. +func classifyChannelRetryError(c *gin.Context, openaiErr *types.NewAPIError) bool { + if openaiErr == nil { + return false + } + if types.IsSkipRetryError(openaiErr) || isSemanticClientError(openaiErr) { + return false + } + if service.ShouldSkipRetryAfterChannelAffinityFailure(c) && openaiErr.StatusCode < http.StatusInternalServerError { + return false + } + if types.IsChannelError(openaiErr) { + return true + } + if _, ok := c.Get("specific_channel_id"); ok { + return false + } + code := openaiErr.StatusCode + if code >= 200 && code < 300 { + return false + } + if code < 100 || code > 599 { + return true + } + if operation_setting.IsAlwaysSkipRetryCode(openaiErr.GetErrorCode()) { + return false + } + return operation_setting.ShouldRetryByStatusCode(code) +} + func shouldRetry(c *gin.Context, openaiErr *types.NewAPIError, retryTimes int) bool { - if openaiErr == nil { - return false - } - if types.IsSkipRetryError(openaiErr) || isSemanticClientError(openaiErr) { - return false - } - if service.ShouldSkipRetryAfterChannelAffinityFailure(c) && openaiErr.StatusCode < http.StatusInternalServerError { - return false - } - if types.IsChannelError(openaiErr) { - return true - } - if retryTimes <= 0 { - return false - } - if _, ok := c.Get("specific_channel_id"); ok { - return false - } - code := openaiErr.StatusCode - if code >= 200 && code < 300 { - return false - } - if code < 100 || code > 599 { - return true - } - if operation_setting.IsAlwaysSkipRetryCode(openaiErr.GetErrorCode()) { - return false - } - return operation_setting.ShouldRetryByStatusCode(code) + if openaiErr != nil && types.IsChannelError(openaiErr) { + return classifyChannelRetryError(c, openaiErr) + } + if retryTimes <= 0 { + return false + } + return classifyChannelRetryError(c, openaiErr) } func isRetryableChannelError(c *gin.Context, openaiErr *types.NewAPIError) bool { - ... (same body) ... + return classifyChannelRetryError(c, openaiErr) }🤖 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/relay.go` around lines 393 - 424, Extract the shared error-classification cascade from shouldRetry and isRetryableChannelError into a common helper, preserving the existing skip-retry, semantic-error, channel-affinity, channel-error, and status-code rules. Have shouldRetry retain only its retryTimes <= 0 gate, while isRetryableChannelError reuses the helper without that gate so retry and cooldown decisions remain consistent.new-api.Apifox.json (1)
8901-8912: 📐 Maintainability & Code Quality | 🔵 TrivialGenerated Apifox export committed in full; confirm this is the intended workflow.
This file is a large (~18k line) auto-generated Apifox project export containing hundreds of structurally identical security-scheme entries. This is expected tool output, not something requiring line-by-line review, but worth confirming the team intends to keep regenerating/committing the full export on every doc change rather than excluding it from version control or trimming duplicate scheme objects.
🤖 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 `@new-api.Apifox.json` around lines 8901 - 8912, Confirm the intended workflow for the generated Apifox export before retaining this full file: determine whether complete regeneration and committing all duplicate security-scheme entries is required for documentation changes. If not required, update the repository workflow to exclude the generated export or reduce redundant entries; otherwise document or preserve the established generation-and-commit process.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@common/custom-event.go`:
- Around line 66-72: Update the data-writing logic around
dataReplacer.WriteString to capture fmt.Sprint(data) in a string variable, reuse
that variable for writing and the strings.HasPrefix check, and remove the unsafe
data.(string) assertion so nil, []byte, and numeric values cannot panic.
In `@common/quota_math_test.go`:
- Around line 24-64: Update common/quota_math_test.go lines 24-64 to import
testify/assert and use assert.Equal, assert.Zero, and assert.Contains for
non-fatal checks while retaining require for setup or fatal assertions. In
service/channel_affinity_template_test.go lines 270-328, use assert.Equal for
channelID, meta.RuleName, and overriding-header output checks. In
model/channel_selection_test.go lines 30-193, replace manual error checks with
require.NoError and express channel expectations with assert.NotNil, assert.Nil,
and assert.Equal, including the selected channel ID.
In `@model/channel_cooldown_test.go`:
- Around line 13-18: Replace all manual conditional assertions in
model/channel_cooldown_test.go, including the checks around
IsChannelCoolingDown, with assert.True and assert.False from the test assertion
library. Also update all manual conditionals and t.Fatalf/t.Fatal assertions in
controller/relay_retryable_test.go to use assert.Equal, assert.True, or
assert.False as appropriate; use require only where setup or fatal assertions
are necessary.
In `@relay/channel/claude/relay-claude.go`:
- Around line 49-93: The buildClaudeFileMessage function silently drops
non-PDF/text attachments through its default case. Reuse the existing generic
file conversion path to represent other supported MIME types, or return a
descriptive error when conversion is unavailable, ensuring the caller cannot
silently omit the attachment.
In `@relay/channel/openai/adaptor.go`:
- Around line 503-530: Ensure the file opened in the imageFiles loop is closed
on every exit path, including CreatePart and io.Copy failures. Update the
cleanup around fileHeader.Open, writer.CreatePart, and io.Copy so errors close
file before returning while preserving the existing success-path cleanup.
- Around line 461-475: Update the image file collection around mf.File to
preserve image[N] ordering by extracting and sorting indexed field names before
appending their files, rather than ranging over the map directly. In the
multipart forwarding logic, ensure every successful fileHeader.Open() and
maskFiles[0].Open() is closed on all subsequent CreatePart and io.Copy error
paths, including deferred cleanup before returning.
In `@relay/channel/openai/chat_via_responses.go`:
- Around line 592-603: Update the common.UnmarshalJsonStr error branch in the
stream callback to use the sibling handler’s non-fatal stream error handling via
sr.Error(err) instead of stopping the scanner with sr.Stop(err). Preserve the
existing logging and return behavior, and ensure the function can continue to
its normal completion path without silently treating the malformed event as a
successful truncated response.
- Line 780: Update the response handling around IOCopyBytesGracefully so the
upstream SSE Content-Type is not forwarded to clients serving this JSON body.
Preserve the body-copy behavior while explicitly removing or overriding the
propagated text/event-stream header before or during the copy.
In `@relay/channel/openai/helper.go`:
- Around line 144-147: Update the marshal-error branch in the response-building
function around common.Marshal to report failures through common.SysError before
returning an empty string. Match the existing ClaudeData error-logging pattern
in this file and preserve the current return behavior.
In `@relay/channel/openai/image_stream/handler.go`:
- Line 158: Replace the bare HTTP client created in the image-stream handler
with the shared client from service.GetHttpClient(), preserving the existing
timeout if the shared-client API supports configuring it. Ensure the gpt-image-*
upstream request uses the shared proxy, redirect, and TLS policies defined by
the service client.
- Around line 215-240: The earlyFlushHeaders and writeError flow commits HTTP
200 before upstream failures are known, causing error responses to retain a
successful status. Remove or defer the early header flush until the request can
be completed successfully, while ensuring writeError can set the actual failure
status before writing the response body.
In `@relay/channel/openai/image_stream/input_normalizer.go`:
- Around line 26-30: Update the URL normalization loop in the input normalizer
to fetch remote images concurrently rather than sequentially, while preserving
maxImagesPerRequest enforcement, per-fetch timeout behavior, result ordering,
and error propagation. Use the existing normalization symbols and ensure all
launched fetches are awaited before returning.
- Around line 140-172: Update fetchAndNormalize to use the existing shared
SSRF-safe URL fetch helper instead of http.DefaultClient.Do, preserving its URL
validation and redirect protections. Ensure the helper supports the existing
size limit and request timeout, and use the established bounded/concurrent fetch
mechanism so multiple remote images do not serialize within one request.
In `@relay/channel/openai/image_stream/request.go`:
- Around line 34-44: Update rawString to use the project’s common.Unmarshal
wrapper instead of encoding/json.Unmarshal when decoding the json.RawMessage,
while preserving the existing fallback that returns the raw bytes on decode
failure.
In `@relay/channel/openai/image_stream/sse.go`:
- Around line 106-108: Gate the diagnostic SysLog call in the SSE handling path
behind common.DebugEnabled, so the payload head is logged only when debug
logging is enabled. Preserve the existing event-type filtering and diagnostic
fields for response.completed, response.failed, and error events.
In `@relay/relay_task.go`:
- Around line 197-207: Update the quota calculation in the non-TaskPricePatches
branch around info.PriceData.OtherRatios to multiply all ratios together in
float64 first, then call QuotaFromFloatStrict exactly once with the combined
result. Preserve the existing quota_out_of_range error wrapping and assignment
behavior, and follow the established recalcQuotaFromRatios pattern rather than
rounding after each map entry.
In `@service/channel_disable_cooldown_test.go`:
- Around line 12-72: Update all seven test functions in the channel cooldown
test file to use testify’s require assertions instead of raw if-condition and
t.Fatalf checks, preserving each existing condition and failure message. Add or
reuse the appropriate testify/require import and convert both expected-true and
expected-false assertions consistently.
In `@service/channel_stream_quality_test.go`:
- Around line 3-134: Update the assertions in the
TestObserveStreamChannelQuality* tests to use testify’s require package instead
of manual if checks with t.Fatalf: replace expected cooldown checks with
require.True and non-cooldown checks with require.False, passing t and the
existing failure message. Add the require import while preserving all setup,
cleanup, and test behavior.
In `@service/text_quota.go`:
- Around line 384-396: Move model.UpdateUserUsedQuotaAndRequestCount outside the
summary.Quota > 0 conditional so it runs for every request, including zero-quota
requests; keep model.UpdateChannelUsedQuota guarded by summary.Quota > 0.
In `@web/default/src/features/pricing/lib/model-metadata.test.ts`:
- Around line 1-3: Replace the node:assert and node:test imports in the
model-metadata tests with Vitest imports, using Vitest’s describe, test, and
expect APIs. Update existing assertions to use expect while preserving the
current test coverage and behavior around formatTokenCount and
inferModelMetadata.
---
Outside diff comments:
In `@controller/channel.go`:
- Around line 1134-1140: Update the response decoding flow around the JSON
decoder to read the limited response body into bytes and parse it with
common.Unmarshal instead of encoding/json directly. Preserve the existing error
response and early return behavior when reading or unmarshalling fails.
In `@relay/channel/openai/relay-openai.go`:
- Around line 572-608: Update the early-flushed error paths in
OpenaiHandlerWithUsage for io.ReadAll and common.Unmarshal failures: once
earlyFlushed is true, do not return an error that causes the caller to write a
conflicting status or duplicate response. Instead, write a best-effort error
envelope after the committed leading whitespace or return a caller-recognizable
result, and update the caller’s error-handling path to avoid rewriting a
status-changing response when the early-flushed 200 is already committed.
In `@relay/common/stream_status.go`:
- Around line 161-195: Update StreamStatus.Summary to defer s.mu.Unlock()
immediately after acquiring s.mu, and remove the manual unlock at the end while
preserving the existing summary formatting and return behavior.
---
Nitpick comments:
In `@controller/claude_count_tokens_test.go`:
- Line 41: The final value checks in controller/claude_count_tokens_test.go
lines 41-41 and relay/helper/common_disconnect_test.go lines 24-25 should be
non-fatal: replace require.Greater with assert.Greater, and
require.ErrorIs/require.Empty with assert.ErrorIs/assert.Empty, adding the
testify/assert import in both affected test files as needed.
In `@controller/relay.go`:
- Around line 393-424: Extract the shared error-classification cascade from
shouldRetry and isRetryableChannelError into a common helper, preserving the
existing skip-retry, semantic-error, channel-affinity, channel-error, and
status-code rules. Have shouldRetry retain only its retryTimes <= 0 gate, while
isRetryableChannelError reuses the helper without that gate so retry and
cooldown decisions remain consistent.
In `@docs/xixiapi-gpt-image-2.md`:
- Around line 197-200: Add a language identifier to the fenced code block
containing the partial image event example, using text or http consistently with
the snippet’s content to satisfy the markdown linter.
- Around line 399-402: Specify the text language on the fenced code block
containing the upload-flow diagram, changing its opening fence to use text while
preserving the diagram content unchanged.
- Around line 162-164: Update the fenced code block containing the response
field list near id, object, and model to specify text as its language,
preserving the existing content.
In `@model/ability.go`:
- Around line 141-159: The unique-priority construction, descending sort, and
retry clamping duplicated in model/ability.go lines 141-159 and
model/channel_cache.go lines 162-178 must be extracted into one shared helper
and reused by both channel-selection paths. Confirm against
service/channel_select.go and controller/relay.go that retry advances only after
a priority tier is exhausted, while ExcludedChannelIDs retries remain within the
current tier; preserve that contract in the helper’s behavior and both callers.
In `@new-api.Apifox.json`:
- Around line 8901-8912: Confirm the intended workflow for the generated Apifox
export before retaining this full file: determine whether complete regeneration
and committing all duplicate security-scheme entries is required for
documentation changes. If not required, update the repository workflow to
exclude the generated export or reduce redundant entries; otherwise document or
preserve the established generation-and-commit process.
In `@relay/channel/openai/image_stream/request.go`:
- Line 52: Refactor buildEditsRequest to accept a dedicated options struct
grouping the related edit settings (size, quality, outputFormat, background,
moderation, and outputCompression) instead of the long positional parameter
list. Update every buildEditsRequest call site in the image stream package to
construct and pass this struct, preserving the existing values and request
behavior.
In `@relay/helper/common.go`:
- Around line 41-43: Update requestContextDone to treat a nil *gin.Context as
done, matching FlushWriter’s nil-safety behavior and preventing subsequent
rendering through a nil context. Also update any callers that construct errors
from c.Request.Context().Err() to guard nil c and avoid dereferencing it.
In `@relay/helper/price.go`:
- Line 88: Optionally consolidate the repeated strict quota conversion error
checks in the surrounding price-building function using a small local helper,
covering the call sites associated with quotaErr and the noted strict/rounding
conversions. Preserve each existing conversion, branch behavior, and return of
types.PriceData{} with the original error; avoid broader refactoring.
In `@service/channel_cooldown_test.go`:
- Around line 1-108: Update all tests in the channel cooldown test file to use
testify assertions: use require for setup or fatal conditions such as cooldown
state, and assert for non-fatal reason and duration checks. Add the testify
dependency import and replace each raw t.Fatalf while preserving the existing
test expectations and logic.
In `@web/default/src/features/pricing/lib/model-metadata.ts`:
- Around line 91-157: Extract the shared input_modalities, output_modalities,
and capabilities values from MODEL_METADATA_OVERRIDES into a reusable constant,
then spread or reference that constant in each Claude override entry while
preserving their model-specific limits.
In `@web/default/src/stores/system-config-store.ts`:
- Around line 58-63: Move the duplicated normalizeLogoUrl helper from
system-config-store.ts and use-system-config.ts into lib/constants.ts alongside
DEFAULT_LOGO, exporting it from there. Update both callers to import and reuse
the shared helper, preserving its current behavior and eliminating the local
definitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6839f126-382b-4022-8ac7-927862e694bb
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumweb/classic/public/favicon.icois excluded by!**/*.icoweb/classic/public/logo.pngis excluded by!**/*.pngweb/default/public/favicon.icois excluded by!**/*.icoweb/default/public/logo.pngis excluded by!**/*.png
📒 Files selected for processing (114)
.gitignorecommon/constants.gocommon/custom-event.gocommon/init.gocommon/quota_math.gocommon/quota_math_test.gocontroller/channel-test.gocontroller/channel.gocontroller/claude_count_tokens.gocontroller/claude_count_tokens_test.gocontroller/model_list_test.gocontroller/relay.gocontroller/relay_retryable_test.godocs/superpowers/specs/2026-05-16-official-api-model-metadata-alignment-design.mddocs/xixiapi-gpt-image-2.mddto/channel_settings.gogo.modmiddleware/distributor.gomiddleware/distributor_cooldown_test.gomiddleware/gzip.gomodel/ability.gomodel/channel.gomodel/channel_cache.gomodel/channel_cooldown.gomodel/channel_cooldown_test.gomodel/channel_selection_test.gomodel/option.gonew-api.Apifox.jsonpkg/billingexpr/round.gopkg/billingexpr/settle.gopkg/billingexpr/types.gorelay/channel/api_request.gorelay/channel/claude/relay-claude.gorelay/channel/openai/adaptor.gorelay/channel/openai/chat_via_responses.gorelay/channel/openai/helper.gorelay/channel/openai/image_stream/handler.gorelay/channel/openai/image_stream/input_normalizer.gorelay/channel/openai/image_stream/r2.gorelay/channel/openai/image_stream/request.gorelay/channel/openai/image_stream/sse.gorelay/channel/openai/relay-openai.gorelay/channel/openai/relay_openai_stream_claude_test.gorelay/channel/openai/relay_responses.gorelay/channel/openai/responses_fallback.gorelay/channel/openai/responses_fallback_test.gorelay/chat_completions_via_responses.gorelay/claude_handler.gorelay/common/relay_info.gorelay/common/remove_disabled_fields_max_output_tokens_test.gorelay/common/stream_status.gorelay/common/stream_status_test.gorelay/compatible_handler.gorelay/helper/common.gorelay/helper/common_disconnect_test.gorelay/helper/model_mapped.gorelay/helper/model_mapped_test.gorelay/helper/price.gorelay/helper/stream_result.gorelay/helper/stream_scanner.gorelay/helper/stream_scanner_test.gorelay/image_handler.gorelay/relay_task.gorelay/responses_handler.gorouter/relay-router.goservice/channel.goservice/channel_affinity.goservice/channel_affinity_template_test.goservice/channel_cooldown.goservice/channel_cooldown_test.goservice/channel_disable_cooldown_test.goservice/channel_select.goservice/channel_stream_quality.goservice/channel_stream_quality_test.goservice/http_client.goservice/log_info_generate.goservice/text_quota.goservice/text_quota_test.goservice/tiered_settle.goservice/token_counter.gosetting/operation_setting/channel_affinity_setting.gosetting/ratio_setting/compact_suffix.gotypes/rw_map.gotypes/rw_map_test.goweb/classic/index.htmlweb/classic/src/components/table/channels/ChannelsColumnDefs.jsxweb/classic/src/helpers/data.jsweb/classic/src/helpers/utils.jsxweb/classic/src/i18n/locales/en.jsonweb/default/index.htmlweb/default/src/features/channels/components/channels-columns.tsxweb/default/src/features/channels/types.tsweb/default/src/features/pricing/components/model-card-grid.tsxweb/default/src/features/pricing/components/model-card.tsxweb/default/src/features/pricing/components/model-details-quick-stats.tsxweb/default/src/features/pricing/components/pricing-columns.tsxweb/default/src/features/pricing/components/pricing-table.tsxweb/default/src/features/pricing/hooks/use-pricing-data.tsweb/default/src/features/pricing/index.tsxweb/default/src/features/pricing/lib/model-metadata.test.tsweb/default/src/features/pricing/lib/model-metadata.tsweb/default/src/features/pricing/lib/price.test.tsweb/default/src/features/pricing/lib/price.tsweb/default/src/hooks/use-system-config.tsweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh.jsonweb/default/src/i18n/static-keys.tsweb/default/src/lib/constants.tsweb/default/src/main.tsxweb/default/src/stores/system-config-store.ts
Comment on lines
+66
to
+72
| if _, err := dataReplacer.WriteString(w, fmt.Sprint(data)); err != nil { | ||
| return err | ||
| } | ||
| if strings.HasPrefix(data.(string), "data") { | ||
| w.writeString("\n\n") | ||
| if _, err := w.writeString("\n\n"); err != nil { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent runtime panic on type assertion.
If data is not a string (e.g., nil, []byte, or numeric), the type assertion data.(string) will panic and crash the request. Since fmt.Sprint(data) gracefully converts any type into a string, you can capture its output in a variable to guarantee safety and avoid the redundant formatting step.
🐛 Proposed fix
func writeData(w stringWriter, data interface{}) error {
- if _, err := dataReplacer.WriteString(w, fmt.Sprint(data)); err != nil {
+ str := fmt.Sprint(data)
+ if _, err := dataReplacer.WriteString(w, str); err != nil {
return err
}
- if strings.HasPrefix(data.(string), "data") {
+ if strings.HasPrefix(str, "data") {
if _, err := w.writeString("\n\n"); err != nil {
return err
}📝 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.
Suggested change
| if _, err := dataReplacer.WriteString(w, fmt.Sprint(data)); err != nil { | |
| return err | |
| } | |
| if strings.HasPrefix(data.(string), "data") { | |
| w.writeString("\n\n") | |
| if _, err := w.writeString("\n\n"); err != nil { | |
| return err | |
| } | |
| str := fmt.Sprint(data) | |
| if _, err := dataReplacer.WriteString(w, str); err != nil { | |
| return err | |
| } | |
| if strings.HasPrefix(str, "data") { | |
| if _, err := w.writeString("\n\n"); err != nil { | |
| return 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 `@common/custom-event.go` around lines 66 - 72, Update the data-writing logic
around dataReplacer.WriteString to capture fmt.Sprint(data) in a string
variable, reuse that variable for writing and the strings.HasPrefix check, and
remove the unsafe data.(string) assertion so nil, []byte, and numeric values
cannot panic.
Comment on lines
+24
to
+64
| got := QuotaFromFloat(tt.in) | ||
| require.Equal(t, tt.want, got) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestQuotaRoundStrictRejectsSaturation(t *testing.T) { | ||
| quota, err := QuotaRoundStrict(float64(MaxQuota) + 1) | ||
|
|
||
| require.Error(t, err) | ||
| require.Zero(t, quota) | ||
| clamp, ok := err.(*QuotaClamp) | ||
| require.True(t, ok) | ||
| require.Equal(t, QuotaClampOverflow, clamp.Kind) | ||
| } | ||
|
|
||
| func TestQuotaRoundStrictAcceptsIntegerBounds(t *testing.T) { | ||
| maxQuota, maxErr := QuotaRoundStrict(float64(MaxQuota)) | ||
| minQuota, minErr := QuotaRoundStrict(float64(MinQuota)) | ||
|
|
||
| require.NoError(t, maxErr) | ||
| require.NoError(t, minErr) | ||
| require.Equal(t, MaxQuota, maxQuota) | ||
| require.Equal(t, MinQuota, minQuota) | ||
| } | ||
|
|
||
| func TestQuotaClampNaNIsJSONSafe(t *testing.T) { | ||
| _, clamp := QuotaFromFloatChecked(math.NaN()) | ||
| require.NotNil(t, clamp) | ||
|
|
||
| data, err := Marshal(clamp) | ||
|
|
||
| require.NoError(t, err) | ||
| require.Contains(t, string(data), `"original":"NaN"`) | ||
| } | ||
|
|
||
| func TestQuotaFromDecimalRoundsBeforeSaturating(t *testing.T) { | ||
| quota := QuotaFromDecimal(decimal.NewFromFloat(1.5)) | ||
|
|
||
| require.Equal(t, 2, quota) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use require for setup/fatal assertions and assert for value checks.
These test files violate the coding guideline that strictly requires using testify/require for setup or fatal conditions and testify/assert for non-fatal value checks. Please remember to import "github.com/stretchr/testify/assert" where applicable.
common/quota_math_test.go#L24-L64: replace non-fatal validations such asrequire.Equal,require.Zero, andrequire.Containswithassert.Equal,assert.Zero, andassert.Contains.service/channel_affinity_template_test.go#L270-L328: replacerequire.Equalwithassert.Equalfor output verifications (e.g., verifyingchannelID,meta.RuleName, and overriding headers).model/channel_selection_test.go#L30-L193: refactor the manualif err != nil { t.Fatalf(...) }statements to userequire.NoError(t, err)for operations like DB seeding/retrieval, and useassert.NotNil,assert.Nil, andassert.Equalfor channel assertions (instead of checkingselected == nil || selected.Id != 29).
📍 Affects 3 files
common/quota_math_test.go#L24-L64(this comment)service/channel_affinity_template_test.go#L270-L328model/channel_selection_test.go#L30-L193
🤖 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/quota_math_test.go` around lines 24 - 64, Update
common/quota_math_test.go lines 24-64 to import testify/assert and use
assert.Equal, assert.Zero, and assert.Contains for non-fatal checks while
retaining require for setup or fatal assertions. In
service/channel_affinity_template_test.go lines 270-328, use assert.Equal for
channelID, meta.RuleName, and overriding-header output checks. In
model/channel_selection_test.go lines 30-193, replace manual error checks with
require.NoError and express channel expectations with assert.NotNil, assert.Nil,
and assert.Equal, including the selected channel ID.
Source: Coding guidelines
Comment on lines
+13
to
+18
| if !IsChannelCoolingDown(17) { | ||
| t.Fatalf("expected channel 17 to be cooling down") | ||
| } | ||
| if IsChannelCoolingDown(29) { | ||
| t.Fatalf("expected channel 29 to remain available") | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use require and assert for test assertions.
As per coding guidelines, new Go backend tests must use require for setup and fatal assertions, and assert for non-fatal value checks, rather than manual if conditionals and t.Fatalf/t.Fatal.
model/channel_cooldown_test.go#L13-L18: Replace manual conditionals andt.Fatalfwithassert.Trueandassert.False(applies to all assertions in this file).controller/relay_retryable_test.go#L66-L68: Replace manual conditionals andt.Fatalf/t.Fatalwithassert.Equal,assert.True, orassert.Falseacross all tests in this file.
📍 Affects 2 files
model/channel_cooldown_test.go#L13-L18(this comment)controller/relay_retryable_test.go#L66-L68
🤖 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/channel_cooldown_test.go` around lines 13 - 18, Replace all manual
conditional assertions in model/channel_cooldown_test.go, including the checks
around IsChannelCoolingDown, with assert.True and assert.False from the test
assertion library. Also update all manual conditionals and t.Fatalf/t.Fatal
assertions in controller/relay_retryable_test.go to use assert.Equal,
assert.True, or assert.False as appropriate; use require only where setup or
fatal assertions are necessary.
Source: Coding guidelines
Comment on lines
+49
to
+93
| func createClaudeFileSource(file *dto.MessageFile) types.FileSource { | ||
| if file == nil || file.FileData == "" { | ||
| return nil | ||
| } | ||
|
|
||
| mimeType := service.GetMimeTypeByExtension(strings.TrimPrefix(strings.ToLower(filepath.Ext(file.FileName)), ".")) | ||
| if mimeType == "application/octet-stream" { | ||
| return nil | ||
| } | ||
| return types.NewFileSourceFromData(file.FileData, mimeType) | ||
| } | ||
|
|
||
| func buildClaudeFileMessage(c *gin.Context, file *dto.MessageFile) (*dto.ClaudeMediaMessage, error) { | ||
| source := createClaudeFileSource(file) | ||
| if source == nil { | ||
| return nil, nil | ||
| } | ||
| base64Data, mimeType, err := service.GetBase64Data(c, source, "formatting document for Claude") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("get file data failed: %w", err) | ||
| } | ||
|
|
||
| switch strings.ToLower(mimeType) { | ||
| case "application/pdf": | ||
| return &dto.ClaudeMediaMessage{ | ||
| Type: "document", | ||
| Source: &dto.ClaudeMessageSource{ | ||
| Type: "base64", | ||
| MediaType: mimeType, | ||
| Data: base64Data, | ||
| }, | ||
| }, nil | ||
| case "text/plain": | ||
| textData, err := base64.StdEncoding.DecodeString(base64Data) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode text file data failed: %w", err) | ||
| } | ||
| return &dto.ClaudeMediaMessage{ | ||
| Type: "text", | ||
| Text: common.GetPointer(string(textData)), | ||
| }, nil | ||
| default: | ||
| return nil, nil | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n 'ContentTypeFile|buildClaudeFileMessage|ToFileSource' relay/channel/claude/relay-claude.go dto/openai_request.goRepository: QuantumNous/new-api
Length of output: 1204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relay/channel/claude/relay-claude.go (around file handling) ==\n'
sed -n '40,110p' relay/channel/claude/relay-claude.go
printf '\n== relay/channel/claude/relay-claude.go (caller around ContentTypeFile) ==\n'
sed -n '410,465p' relay/channel/claude/relay-claude.go
printf '\n== dto/openai_request.go (MediaContent ToFileSource and ContentTypeFile handling) ==\n'
sed -n '340,410p' dto/openai_request.go
printf '\n== dto/openai_request.go (ContentTypeFile constants and builder paths) ==\n'
sed -n '420,820p' dto/openai_request.goRepository: QuantumNous/new-api
Length of output: 15317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('relay/channel/claude/relay-claude.go')
text = p.read_text()
for needle in ['buildClaudeFileMessage', 'ContentTypeFile', 'default:', 'application/pdf', 'text/plain']:
print(f'-- {needle} --')
idx = text.find(needle)
if idx != -1:
start = max(0, text.rfind('\n', 0, idx-200))
end = min(len(text), text.find('\n', idx+300))
print(text[idx-120:idx+400])
print()
PYRepository: QuantumNous/new-api
Length of output: 2872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- relay-claude.go: buildClaudeFileMessage and caller ---'
nl -ba relay/channel/claude/relay-claude.go | sed -n '45,105p;420,465p'
echo
echo '--- dto/openai_request.go: MediaContent.ToFileSource and ContentTypeFile ---'
nl -ba dto/openai_request.go | sed -n '360,410p;420,470p;590,820p'Repository: QuantumNous/new-api
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '420,460p' relay/channel/claude/relay-claude.go
echo '---'
sed -n '590,640p' dto/openai_request.go
echo '---'
sed -n '770,810p' dto/openai_request.goRepository: QuantumNous/new-api
Length of output: 4037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba relay/channel/claude/relay-claude.go | sed -n '420,452p'Repository: QuantumNous/new-api
Length of output: 197
Non-PDF/text file attachments are dropped.
buildClaudeFileMessage only returns a Claude media message for application/pdf and text/plain; every other MIME type falls through to nil, and the caller skips the item. Generic file content with other attachments is silently removed from the request instead of being converted or rejected.
Consider reusing the generic file conversion path here, or at least logging/returning an error when the attachment can’t be represented.
🤖 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/claude/relay-claude.go` around lines 49 - 93, The
buildClaudeFileMessage function silently drops non-PDF/text attachments through
its default case. Reuse the existing generic file conversion path to represent
other supported MIME types, or return a descriptive error when conversion is
unavailable, ensuring the caller cannot silently omit the attachment.
Comment on lines
+461
to
+475
| // 收集 image 二进制文件：image / image[] / image[N] | ||
| var imageFiles []*multipart.FileHeader | ||
| if mf.File != nil { | ||
| if files, ok := mf.File["image"]; ok && len(files) > 0 { | ||
| imageFiles = files | ||
| } else if files, ok := mf.File["image[]"]; ok && len(files) > 0 { | ||
| imageFiles = files | ||
| } else { | ||
| for fieldName, files := range mf.File { | ||
| if strings.HasPrefix(fieldName, "image[") && len(files) > 0 { | ||
| imageFiles = append(imageFiles, files...) | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm intended semantics for indexed image[] fields (ordering contract with downstream worker).
rg -n 'image\[' --type=go relay/channel/openai/image_streamRepository: QuantumNous/new-api
Length of output: 619
🏁 Script executed:
sed -n '430,560p' relay/channel/openai/adaptor.go
printf '\n---\n'
sed -n '1,220p' relay/channel/openai/image_stream/input_normalizer.go
printf '\n---\n'
rg -n 'imageFiles|CreatePart|Open\(' relay/channel/openai -g '*.go'Repository: QuantumNous/new-api
Length of output: 11208
🏁 Script executed:
rg -n 'CollectAndNormalizeImages|normalizeImageInput|image\[\d+\]|order encountered|order' relay/channel/openai -g '*.go'Repository: QuantumNous/new-api
Length of output: 3099
Preserve image[N] order and close files on every error path.
image[N]entries are still collected by ranging overmf.File, so their order can be shuffled before forwarding.fileHeader.Open()/maskFiles[0].Open()should be paired withdefer Close(); theCreatePartandio.Copyerror returns currently skip cleanup.
🤖 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/openai/adaptor.go` around lines 461 - 475, Update the image
file collection around mf.File to preserve image[N] ordering by extracting and
sorting indexed field names before appending their files, rather than ranging
over the map directly. In the multipart forwarding logic, ensure every
successful fileHeader.Open() and maskFiles[0].Open() is closed on all subsequent
CreatePart and io.Copy error paths, including deferred cleanup before returning.
Comment on lines
197
to
207
| if !common.StringsContains(constant.TaskPricePatches, modelName) { | ||
| for _, ra := range info.PriceData.OtherRatios { | ||
| if ra != 1.0 { | ||
| info.PriceData.Quota = int(float64(info.PriceData.Quota) * ra) | ||
| quota, quotaErr := common.QuotaFromFloatStrict(float64(info.PriceData.Quota) * ra) | ||
| if quotaErr != nil { | ||
| return nil, service.TaskErrorWrapper(quotaErr, "quota_out_of_range", http.StatusBadRequest) | ||
| } | ||
| info.PriceData.Quota = quota | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Quota depends on Go map iteration order due to per-ratio rounding.
Applying each OtherRatios entry with a fresh QuotaFromFloatStrict round-trip after every multiplication makes the final quota order-dependent, and Go map iteration order is randomized. Two runs of the identical request can settle to different pre-consumed quotas. recalcQuotaFromRatios below avoids this by combining all ratios in float64 first and rounding once — apply the same pattern here.
🐛 Proposed fix
if !common.StringsContains(constant.TaskPricePatches, modelName) {
- for _, ra := range info.PriceData.OtherRatios {
- if ra != 1.0 {
- quota, quotaErr := common.QuotaFromFloatStrict(float64(info.PriceData.Quota) * ra)
- if quotaErr != nil {
- return nil, service.TaskErrorWrapper(quotaErr, "quota_out_of_range", http.StatusBadRequest)
- }
- info.PriceData.Quota = quota
- }
- }
+ combinedRatio := 1.0
+ for _, ra := range info.PriceData.OtherRatios {
+ if ra != 1.0 {
+ combinedRatio *= ra
+ }
+ }
+ if combinedRatio != 1.0 {
+ quota, quotaErr := common.QuotaFromFloatStrict(float64(info.PriceData.Quota) * combinedRatio)
+ if quotaErr != nil {
+ return nil, service.TaskErrorWrapper(quotaErr, "quota_out_of_range", http.StatusBadRequest)
+ }
+ info.PriceData.Quota = quota
+ }
}📝 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.
Suggested change
| if !common.StringsContains(constant.TaskPricePatches, modelName) { | |
| for _, ra := range info.PriceData.OtherRatios { | |
| if ra != 1.0 { | |
| info.PriceData.Quota = int(float64(info.PriceData.Quota) * ra) | |
| quota, quotaErr := common.QuotaFromFloatStrict(float64(info.PriceData.Quota) * ra) | |
| if quotaErr != nil { | |
| return nil, service.TaskErrorWrapper(quotaErr, "quota_out_of_range", http.StatusBadRequest) | |
| } | |
| info.PriceData.Quota = quota | |
| } | |
| } | |
| } | |
| if !common.StringsContains(constant.TaskPricePatches, modelName) { | |
| combinedRatio := 1.0 | |
| for _, ra := range info.PriceData.OtherRatios { | |
| if ra != 1.0 { | |
| combinedRatio *= ra | |
| } | |
| } | |
| if combinedRatio != 1.0 { | |
| quota, quotaErr := common.QuotaFromFloatStrict(float64(info.PriceData.Quota) * combinedRatio) | |
| if quotaErr != nil { | |
| return nil, service.TaskErrorWrapper(quotaErr, "quota_out_of_range", http.StatusBadRequest) | |
| } | |
| info.PriceData.Quota = quota | |
| } | |
| } |
🤖 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/relay_task.go` around lines 197 - 207, Update the quota calculation in
the non-TaskPricePatches branch around info.PriceData.OtherRatios to multiply
all ratios together in float64 first, then call QuotaFromFloatStrict exactly
once with the combined result. Preserve the existing quota_out_of_range error
wrapping and assignment behavior, and follow the established
recalcQuotaFromRatios pattern rather than rounding after each map entry.
Comment on lines
+12
to
+72
| func TestShouldDisableChannelIgnoresCooldownBalanceError(t *testing.T) { | ||
| oldAutomaticDisableChannelEnabled := common.AutomaticDisableChannelEnabled | ||
| common.AutomaticDisableChannelEnabled = true | ||
| t.Cleanup(func() { | ||
| common.AutomaticDisableChannelEnabled = oldAutomaticDisableChannelEnabled | ||
| }) | ||
|
|
||
| err := types.NewErrorWithStatusCode(errors.New("Insufficient account balance"), types.ErrorCodeBadResponseStatusCode, http.StatusForbidden) | ||
|
|
||
| if ShouldDisableChannel(err) { | ||
| t.Fatalf("expected balance error to cooldown without permanent auto-disable") | ||
| } | ||
| } | ||
|
|
||
| func TestShouldCooldownChannelForUpstreamErrorCoolsMalformedResponses(t *testing.T) { | ||
| err := types.NewErrorWithStatusCode(errors.New("API returned an empty or malformed response (HTTP 200)"), types.ErrorCodeBadResponseBody, http.StatusInternalServerError) | ||
|
|
||
| if !ShouldCooldownChannelForUpstreamError(err) { | ||
| t.Fatalf("expected malformed upstream response to cooldown") | ||
| } | ||
| } | ||
|
|
||
| func TestShouldCooldownChannelForUpstreamErrorCoolsSkipRetryMalformedResponses(t *testing.T) { | ||
| err := types.NewErrorWithStatusCode(errors.New("API returned an empty or malformed response (HTTP 200)"), types.ErrorCodeBadResponseBody, http.StatusInternalServerError, types.ErrOptionWithSkipRetry()) | ||
|
|
||
| if !ShouldCooldownChannelForUpstreamError(err) { | ||
| t.Fatalf("expected malformed upstream response to cooldown even when retry is skipped") | ||
| } | ||
| } | ||
|
|
||
| func TestShouldCooldownChannelForUpstreamErrorCoolsBadGateway(t *testing.T) { | ||
| err := types.WithOpenAIError(types.OpenAIError{Message: "openai_error", Type: "openai_error", Code: "openai_error"}, http.StatusBadGateway) | ||
|
|
||
| if !ShouldCooldownChannelForUpstreamError(err) { | ||
| t.Fatalf("expected upstream 502 to cooldown") | ||
| } | ||
| } | ||
|
|
||
| func TestShouldCooldownChannelForUpstreamErrorCoolsImageGenerationCapabilityGap(t *testing.T) { | ||
| err := types.NewErrorWithStatusCode(errors.New("Image generation is not enabled for this group"), types.ErrorCodeBadResponseStatusCode, http.StatusForbidden) | ||
|
|
||
| if !ShouldCooldownChannelForUpstreamError(err) { | ||
| t.Fatalf("expected per-channel capability gap (image generation disabled) to cooldown despite being 4xx") | ||
| } | ||
| } | ||
|
|
||
| func TestShouldCooldownChannelForUpstreamErrorIgnoresClientErrors(t *testing.T) { | ||
| err := types.NewErrorWithStatusCode(errors.New("invalid request"), types.ErrorCodeInvalidRequest, http.StatusBadRequest, types.ErrOptionWithSkipRetry()) | ||
|
|
||
| if ShouldCooldownChannelForUpstreamError(err) { | ||
| t.Fatalf("expected client validation error to avoid cooldown") | ||
| } | ||
| } | ||
|
|
||
| func TestShouldCooldownChannelForUpstreamErrorIgnoresAuthErrors(t *testing.T) { | ||
| err := types.NewErrorWithStatusCode(errors.New("invalid token"), types.ErrorCodeAccessDenied, http.StatusUnauthorized) | ||
|
|
||
| if ShouldCooldownChannelForUpstreamError(err) { | ||
| t.Fatalf("expected auth error to avoid cooldown") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use testify require for these fatal assertions.
All 7 test functions use raw if cond { t.Fatalf(...) } instead of require. Per guideline, new Go backend tests must use require for fatal assertions.
♻️ Proposed fix (pattern, apply to all 7 functions)
+ "github.com/stretchr/testify/require"
)
func TestShouldDisableChannelIgnoresCooldownBalanceError(t *testing.T) {
...
- if ShouldDisableChannel(err) {
- t.Fatalf("expected balance error to cooldown without permanent auto-disable")
- }
+ require.False(t, ShouldDisableChannel(err), "expected balance error to cooldown without permanent auto-disable")
}As per coding guidelines: "New or substantially rewritten Go backend tests must use require for setup and fatal assertions and assert for non-fatal value checks."
🤖 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_disable_cooldown_test.go` around lines 12 - 72, Update all
seven test functions in the channel cooldown test file to use testify’s require
assertions instead of raw if-condition and t.Fatalf checks, preserving each
existing condition and failure message. Add or reuse the appropriate
testify/require import and convert both expected-true and expected-false
assertions consistently.
Source: Coding guidelines
Comment on lines
+3
to
+134
| import ( | ||
| "fmt" | ||
| "testing" | ||
|
|
||
| "github.com/QuantumNous/new-api/model" | ||
| relaycommon "github.com/QuantumNous/new-api/relay/common" | ||
| ) | ||
|
|
||
| func TestObserveStreamChannelQualityCoolsAfterRepeatedTimeouts(t *testing.T) { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| t.Cleanup(func() { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| }) | ||
|
|
||
| for i := 0; i < streamQualityFailureThreshold-1; i++ { | ||
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil)) | ||
| if model.IsChannelCoolingDown(12) { | ||
| t.Fatalf("channel cooled before threshold at failure %d", i+1) | ||
| } | ||
| } | ||
|
|
||
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil)) | ||
|
|
||
| if !model.IsChannelCoolingDown(12) { | ||
| t.Fatalf("expected channel to cool down after repeated stream timeouts") | ||
| } | ||
| } | ||
|
|
||
| func TestObserveStreamChannelQualityIgnoresNormalClientGoneAfterData(t *testing.T) { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| t.Cleanup(func() { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| }) | ||
|
|
||
| for i := 0; i < streamQualityFailureThreshold+1; i++ { | ||
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 10, nil)) | ||
| } | ||
|
|
||
| if model.IsChannelCoolingDown(12) { | ||
| t.Fatalf("expected normal client_gone after data to avoid channel cooldown") | ||
| } | ||
| } | ||
|
|
||
| func TestObserveStreamChannelQualityIgnoresClientGoneBeforeData(t *testing.T) { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| t.Cleanup(func() { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| }) | ||
|
|
||
| for i := 0; i < streamQualityFailureThreshold+1; i++ { | ||
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(17, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 0, nil)) | ||
| } | ||
|
|
||
| if model.IsChannelCoolingDown(17) { | ||
| t.Fatalf("expected client_gone before data without transport error to avoid channel cooldown") | ||
| } | ||
| } | ||
|
|
||
| func TestObserveStreamChannelQualityCoolsTransportErrors(t *testing.T) { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| t.Cleanup(func() { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| }) | ||
|
|
||
| for i := 0; i < streamQualityFailureThreshold; i++ { | ||
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(19, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 20, []string{"http2: response body closed"})) | ||
| } | ||
|
|
||
| if !model.IsChannelCoolingDown(19) { | ||
| t.Fatalf("expected repeated stream transport errors to cool channel") | ||
| } | ||
| } | ||
|
|
||
| func TestObserveStreamChannelQualityCoolsClientGoneTerminalTransportError(t *testing.T) { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| t.Cleanup(func() { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| }) | ||
|
|
||
| for i := 0; i < streamQualityFailureThreshold; i++ { | ||
| ObserveStreamChannelQuality(newStreamQualityRelayInfoWithEndError(21, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 20, "connection reset by peer", nil)) | ||
| } | ||
|
|
||
| if !model.IsChannelCoolingDown(21) { | ||
| t.Fatalf("expected repeated terminal transport errors to cool channel") | ||
| } | ||
| } | ||
|
|
||
| func TestObserveStreamChannelQualityCoolsSoftMalformedErrors(t *testing.T) { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| t.Cleanup(func() { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| }) | ||
|
|
||
| for i := 0; i < streamQualityFailureThreshold; i++ { | ||
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(22, "gpt-5.5", relaycommon.StreamEndReasonEOF, 20, []string{"invalid character '<' looking for beginning of value"})) | ||
| } | ||
|
|
||
| if !model.IsChannelCoolingDown(22) { | ||
| t.Fatalf("expected repeated malformed stream chunks to cool channel") | ||
| } | ||
| } | ||
|
|
||
| func TestObserveStreamChannelQualityTracksModelSeparately(t *testing.T) { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| t.Cleanup(func() { | ||
| model.ClearChannelCooldownsForTest() | ||
| clearStreamChannelQualityForTest() | ||
| }) | ||
|
|
||
| for i := 0; i < streamQualityFailureThreshold-1; i++ { | ||
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil)) | ||
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.4", relaycommon.StreamEndReasonTimeout, 0, nil)) | ||
| } | ||
|
|
||
| if model.IsChannelCoolingDown(12) { | ||
| t.Fatalf("expected per-model failures to stay below threshold") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use require for test assertions instead of manual t.Fatalf.
As per coding guidelines, new Go backend tests must use require for setup and fatal assertions. Please replace the manual if ... { t.Fatalf(...) } checks with require.True() or require.False().
♻️ Proposed refactor for test assertions
import (
"fmt"
"testing"
"github.com/QuantumNous/new-api/model"
relaycommon "github.com/QuantumNous/new-api/relay/common"
+ "github.com/stretchr/testify/require"
)
func TestObserveStreamChannelQualityCoolsAfterRepeatedTimeouts(t *testing.T) {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
t.Cleanup(func() {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
})
for i := 0; i < streamQualityFailureThreshold-1; i++ {
ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil))
- if model.IsChannelCoolingDown(12) {
- t.Fatalf("channel cooled before threshold at failure %d", i+1)
- }
+ require.False(t, model.IsChannelCoolingDown(12), "channel cooled before threshold at failure %d", i+1)
}
ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil))
- if !model.IsChannelCoolingDown(12) {
- t.Fatalf("expected channel to cool down after repeated stream timeouts")
- }
+ require.True(t, model.IsChannelCoolingDown(12), "expected channel to cool down after repeated stream timeouts")
}
func TestObserveStreamChannelQualityIgnoresNormalClientGoneAfterData(t *testing.T) {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
t.Cleanup(func() {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
})
for i := 0; i < streamQualityFailureThreshold+1; i++ {
ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 10, nil))
}
- if model.IsChannelCoolingDown(12) {
- t.Fatalf("expected normal client_gone after data to avoid channel cooldown")
- }
+ require.False(t, model.IsChannelCoolingDown(12), "expected normal client_gone after data to avoid channel cooldown")
}
func TestObserveStreamChannelQualityIgnoresClientGoneBeforeData(t *testing.T) {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
t.Cleanup(func() {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
})
for i := 0; i < streamQualityFailureThreshold+1; i++ {
ObserveStreamChannelQuality(newStreamQualityRelayInfo(17, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 0, nil))
}
- if model.IsChannelCoolingDown(17) {
- t.Fatalf("expected client_gone before data without transport error to avoid channel cooldown")
- }
+ require.False(t, model.IsChannelCoolingDown(17), "expected client_gone before data without transport error to avoid channel cooldown")
}
func TestObserveStreamChannelQualityCoolsTransportErrors(t *testing.T) {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
t.Cleanup(func() {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
})
for i := 0; i < streamQualityFailureThreshold; i++ {
ObserveStreamChannelQuality(newStreamQualityRelayInfo(19, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 20, []string{"http2: response body closed"}))
}
- if !model.IsChannelCoolingDown(19) {
- t.Fatalf("expected repeated stream transport errors to cool channel")
- }
+ require.True(t, model.IsChannelCoolingDown(19), "expected repeated stream transport errors to cool channel")
}
func TestObserveStreamChannelQualityCoolsClientGoneTerminalTransportError(t *testing.T) {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
t.Cleanup(func() {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
})
for i := 0; i < streamQualityFailureThreshold; i++ {
ObserveStreamChannelQuality(newStreamQualityRelayInfoWithEndError(21, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 20, "connection reset by peer", nil))
}
- if !model.IsChannelCoolingDown(21) {
- t.Fatalf("expected repeated terminal transport errors to cool channel")
- }
+ require.True(t, model.IsChannelCoolingDown(21), "expected repeated terminal transport errors to cool channel")
}
func TestObserveStreamChannelQualityCoolsSoftMalformedErrors(t *testing.T) {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
t.Cleanup(func() {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
})
for i := 0; i < streamQualityFailureThreshold; i++ {
ObserveStreamChannelQuality(newStreamQualityRelayInfo(22, "gpt-5.5", relaycommon.StreamEndReasonEOF, 20, []string{"invalid character '<' looking for beginning of value"}))
}
- if !model.IsChannelCoolingDown(22) {
- t.Fatalf("expected repeated malformed stream chunks to cool channel")
- }
+ require.True(t, model.IsChannelCoolingDown(22), "expected repeated malformed stream chunks to cool channel")
}
func TestObserveStreamChannelQualityTracksModelSeparately(t *testing.T) {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
t.Cleanup(func() {
model.ClearChannelCooldownsForTest()
clearStreamChannelQualityForTest()
})
for i := 0; i < streamQualityFailureThreshold-1; i++ {
ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil))
ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.4", relaycommon.StreamEndReasonTimeout, 0, nil))
}
- if model.IsChannelCoolingDown(12) {
- t.Fatalf("expected per-model failures to stay below threshold")
- }
+ require.False(t, model.IsChannelCoolingDown(12), "expected per-model failures to stay below threshold")
}📝 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.
Suggested change
| import ( | |
| "fmt" | |
| "testing" | |
| "github.com/QuantumNous/new-api/model" | |
| relaycommon "github.com/QuantumNous/new-api/relay/common" | |
| ) | |
| func TestObserveStreamChannelQualityCoolsAfterRepeatedTimeouts(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold-1; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil)) | |
| if model.IsChannelCoolingDown(12) { | |
| t.Fatalf("channel cooled before threshold at failure %d", i+1) | |
| } | |
| } | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil)) | |
| if !model.IsChannelCoolingDown(12) { | |
| t.Fatalf("expected channel to cool down after repeated stream timeouts") | |
| } | |
| } | |
| func TestObserveStreamChannelQualityIgnoresNormalClientGoneAfterData(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold+1; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 10, nil)) | |
| } | |
| if model.IsChannelCoolingDown(12) { | |
| t.Fatalf("expected normal client_gone after data to avoid channel cooldown") | |
| } | |
| } | |
| func TestObserveStreamChannelQualityIgnoresClientGoneBeforeData(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold+1; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(17, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 0, nil)) | |
| } | |
| if model.IsChannelCoolingDown(17) { | |
| t.Fatalf("expected client_gone before data without transport error to avoid channel cooldown") | |
| } | |
| } | |
| func TestObserveStreamChannelQualityCoolsTransportErrors(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(19, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 20, []string{"http2: response body closed"})) | |
| } | |
| if !model.IsChannelCoolingDown(19) { | |
| t.Fatalf("expected repeated stream transport errors to cool channel") | |
| } | |
| } | |
| func TestObserveStreamChannelQualityCoolsClientGoneTerminalTransportError(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfoWithEndError(21, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 20, "connection reset by peer", nil)) | |
| } | |
| if !model.IsChannelCoolingDown(21) { | |
| t.Fatalf("expected repeated terminal transport errors to cool channel") | |
| } | |
| } | |
| func TestObserveStreamChannelQualityCoolsSoftMalformedErrors(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(22, "gpt-5.5", relaycommon.StreamEndReasonEOF, 20, []string{"invalid character '<' looking for beginning of value"})) | |
| } | |
| if !model.IsChannelCoolingDown(22) { | |
| t.Fatalf("expected repeated malformed stream chunks to cool channel") | |
| } | |
| } | |
| func TestObserveStreamChannelQualityTracksModelSeparately(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold-1; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil)) | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.4", relaycommon.StreamEndReasonTimeout, 0, nil)) | |
| } | |
| if model.IsChannelCoolingDown(12) { | |
| t.Fatalf("expected per-model failures to stay below threshold") | |
| } | |
| } | |
| import ( | |
| "fmt" | |
| "testing" | |
| "github.com/QuantumNous/new-api/model" | |
| relaycommon "github.com/QuantumNous/new-api/relay/common" | |
| "github.com/stretchr/testify/require" | |
| ) | |
| func TestObserveStreamChannelQualityCoolsAfterRepeatedTimeouts(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold-1; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil)) | |
| require.False(t, model.IsChannelCoolingDown(12), "channel cooled before threshold at failure %d", i+1) | |
| } | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil)) | |
| require.True(t, model.IsChannelCoolingDown(12), "expected channel to cool down after repeated stream timeouts") | |
| } | |
| func TestObserveStreamChannelQualityIgnoresNormalClientGoneAfterData(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold+1; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 10, nil)) | |
| } | |
| require.False(t, model.IsChannelCoolingDown(12), "expected normal client_gone after data to avoid channel cooldown") | |
| } | |
| func TestObserveStreamChannelQualityIgnoresClientGoneBeforeData(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold+1; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(17, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 0, nil)) | |
| } | |
| require.False(t, model.IsChannelCoolingDown(17), "expected client_gone before data without transport error to avoid channel cooldown") | |
| } | |
| func TestObserveStreamChannelQualityCoolsTransportErrors(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(19, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 20, []string{"http2: response body closed"})) | |
| } | |
| require.True(t, model.IsChannelCoolingDown(19), "expected repeated stream transport errors to cool channel") | |
| } | |
| func TestObserveStreamChannelQualityCoolsClientGoneTerminalTransportError(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfoWithEndError(21, "gpt-5.5", relaycommon.StreamEndReasonClientGone, 20, "connection reset by peer", nil)) | |
| } | |
| require.True(t, model.IsChannelCoolingDown(21), "expected repeated terminal transport errors to cool channel") | |
| } | |
| func TestObserveStreamChannelQualityCoolsSoftMalformedErrors(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(22, "gpt-5.5", relaycommon.StreamEndReasonEOF, 20, []string{"invalid character '<' looking for beginning of value"})) | |
| } | |
| require.True(t, model.IsChannelCoolingDown(22), "expected repeated malformed stream chunks to cool channel") | |
| } | |
| func TestObserveStreamChannelQualityTracksModelSeparately(t *testing.T) { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| t.Cleanup(func() { | |
| model.ClearChannelCooldownsForTest() | |
| clearStreamChannelQualityForTest() | |
| }) | |
| for i := 0; i < streamQualityFailureThreshold-1; i++ { | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.5", relaycommon.StreamEndReasonTimeout, 0, nil)) | |
| ObserveStreamChannelQuality(newStreamQualityRelayInfo(12, "gpt-5.4", relaycommon.StreamEndReasonTimeout, 0, nil)) | |
| } | |
| require.False(t, model.IsChannelCoolingDown(12), "expected per-model failures to stay below threshold") | |
| } |
🤖 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_stream_quality_test.go` around lines 3 - 134, Update the
assertions in the TestObserveStreamChannelQuality* tests to use testify’s
require package instead of manual if checks with t.Fatalf: replace expected
cooldown checks with require.True and non-cooldown checks with require.False,
passing t and the existing failure message. Add the require import while
preserving all setup, cleanup, and test behavior.
Source: Coding guidelines
Comment on lines
384
to
396
| if summary.TotalTokens == 0 { | ||
| extraContent = append(extraContent, "上游没有返回计费信息，无法扣费（可能是上游超时）") | ||
| logger.LogError(ctx, fmt.Sprintf("total tokens is 0, cannot consume quota, userId %d, channelId %d, tokenId %d, model %s， pre-consumed quota %d", relayInfo.UserId, relayInfo.ChannelId, relayInfo.TokenId, summary.ModelName, relayInfo.FinalPreConsumedQuota)) | ||
| } else { | ||
| extraContent = append(extraContent, "上游没有返回 Token 计费信息（可能是上游超时）") | ||
| message := fmt.Sprintf("total tokens is 0, userId %d, channelId %d, tokenId %d, model %s, pre-consumed quota %d", relayInfo.UserId, relayInfo.ChannelId, relayInfo.TokenId, summary.ModelName, relayInfo.FinalPreConsumedQuota) | ||
| if relayInfo.StreamStatus != nil && relayInfo.StreamStatus.Snapshot().EndReason == relaycommon.StreamEndReasonClientGone { | ||
| logger.LogInfo(ctx, message) | ||
| } else { | ||
| logger.LogError(ctx, message) | ||
| } | ||
| } | ||
| if summary.Quota > 0 { | ||
| model.UpdateUserUsedQuotaAndRequestCount(relayInfo.UserId, summary.Quota) | ||
| model.UpdateChannelUsedQuota(relayInfo.ChannelId, summary.Quota) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep run --pattern 'func UpdateUserUsedQuotaAndRequestCount($$$) {
$$$
}' --lang go model/Repository: QuantumNous/new-api
Length of output: 561
🏁 Script executed:
sed -n '955,980p' model/user.go
printf '\n---\n'
sed -n '370,410p' service/text_quota.goRepository: QuantumNous/new-api
Length of output: 3703
Keep the request-count update outside the summary.Quota > 0 guard. UpdateUserUsedQuotaAndRequestCount increments request_count by 1 on every call, so zero-quota requests will stop contributing to usage stats even though they should still be counted.
🤖 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.go` around lines 384 - 396, Move
model.UpdateUserUsedQuotaAndRequestCount outside the summary.Quota > 0
conditional so it runs for every request, including zero-quota requests; keep
model.UpdateChannelUsedQuota guarded by summary.Quota > 0.
Comment on lines
+1
to
+3
| import assert from 'node:assert/strict' | ||
| import { describe, test } from 'node:test' | ||
| import { formatTokenCount, inferModelMetadata } from './model-metadata' |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Vitest instead of node:test.
As per coding guidelines, unit tests for utility functions and pure logic must be written with Vitest. Please replace node:test and node:assert with Vitest's describe, test, and expect.
♻️ Proposed fixes
-import assert from 'node:assert/strict'
-import { describe, test } from 'node:test'
+import { describe, test, expect } from 'vitest'
import { formatTokenCount, inferModelMetadata } from './model-metadata'📝 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.
Suggested change
| import assert from 'node:assert/strict' | |
| import { describe, test } from 'node:test' | |
| import { formatTokenCount, inferModelMetadata } from './model-metadata' | |
| import { describe, test, expect } from 'vitest' | |
| import { formatTokenCount, inferModelMetadata } from './model-metadata' |
🤖 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 `@web/default/src/features/pricing/lib/model-metadata.test.ts` around lines 1 -
3, Replace the node:assert and node:test imports in the model-metadata tests
with Vitest imports, using Vitest’s describe, test, and expect APIs. Update
existing assertions to use expect while preserving the current test coverage and
behavior around formatTokenCount and inferModelMetadata.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
X-Anthropic-Billing-HeaderValidation
go test ./common ./types ./controller ./relay ./relay/helper ./relay/channel/openai ./relay/channel/claude ./service ./pkg/billingexpr ./setting/operation_setting -count=1git diff --checkNotes
docs/superpowers/audits/was intentionally excluded.Summary by CodeRabbit
New Features
/v1/messages/count_tokens.Bug Fixes
Documentation