feat: runtime hardening, delivery seam, i18n and logout fixes (rebased) - #6286
feat: runtime hardening, delivery seam, i18n and logout fixes (rebased)#6286xvyimu wants to merge 40 commits into
Conversation
- decreaseTokenQuota skips remain floor for unlimited_quota tokens - batch quota debit guards user quota>=0 and negative token delta - billing session refresh and relay origin coverage - remove obsolete pre_consume_quota.go (functions unreferenced)
- session refresh reloads role/status/group from DB with nil guards - TokenAuthReadOnly rejects disabled/expired tokens - request trace middleware and coverage
- adaptive metrics record with concurrency guard and half-open recovery - channel circuit breaker, metrics and score modules - affinity usage settings and empty-group guards
- perf metric model with model_name equality index - /api/perf-metrics/* behind UserAuth() (401 for unauthenticated) - normalize utility with test coverage
- consolidate env var reads into common/env.go - init sequence cleanup in common/init.go - context key constants and API router alignment - SESSION_SECURE default true (overridden by start script)
- Markdown sanitize with allowlist (no svg/data/javascript/iframe-same-origin) - Auth redirect safeRedirect to prevent open redirect - OTP flow component hydration and route fixes - lib/api.ts custom fetch with session verification - About page iframe sandbox removal
- Pricing columns memoization and per-page perf badge map - Filters<->URL debounced sync - Bulk actions skip-confirm dialog - Channel mutate drawer split (skip-auto-test field extraction) - model-name/mock-badge helpers and filters test
- Creem/Waffo payment hooks with proper type safety - Payment lib extraction from components - All locale files (en/zh/fr/ru/ja/vi) populated for pricing, wallet, billing - Clean up dead locale keys
- log model cleanup - xAI relay text coverage and behavioral test - theme setting default fallback to 'default'
Rebase onto upstream renamed filterChannelsByRequestPath to filterChannelsByRequestPathAndModel; keep adaptive candidate selection aligned with Advanced Custom route matching.
Add restrictive CORS, bounded outbound HTTP clients, immutable asset caching, cursor log pagination, trace indexing, runtime plane separation, Prometheus/RUM observability, reproducible deployment defaults, and CI quality gates. Includes regression tests, bundle budget enforcement, operational runbooks, and the mobile setup layout fix discovered during browser validation.
Keep the monorepo and embedded dual-theme binary as the default path,
while adding an opt-in pure backend build (-tags frontend_external) and
FRONTEND_MODE={auto,embedded,redirect,disabled}.
Ship Dockerfile.backend plus a same-origin Nginx frontend image under
deploy/separated, extend the quality gate, and document the decision in
ADR 0001. Local Go router tests and both build modes passed; Docker image
builds were not run on this host.
nginx -t failed in CI because a static upstream block resolved backend:3000 at config load time inside a single-container job. Use variable proxy_pass with a configurable resolver (Docker DNS by default) and point the CI syntax check at 127.0.0.1:3000.
Add FRONTEND_MODE docs to .env.example, Makefile targets for pure backend and separated images, and portable smoke scripts. Resolve the frontend nginx base digest at quality-image build time via NGINX_IMAGE build-arg. Cover disabled 404 and master auto-mode asset requirements with tests.
Switch Dockerfile.dev to -tags frontend_external and FRONTEND_MODE=disabled so docker-compose.dev no longer needs placeholder web dist. Default the frontend Nginx base image ARG to the CI-pinned digest, and add scripts/local-ops.ps1 for Windows check/backend/inventory helpers.
Use zhCN/zh-CN as the interface and backend i18n fallback so local and China-facing deployments open in Chinese when no stored preference or Accept-Language match is available. Classic theme already defaulted to zh-CN.
fallbackLng alone never wins when navigator or a cached i18nextLng=en is present. Detect only localStorage, one-time migrate historical en caches to zhCN/zh-CN, and set default HTML lang to zh-CN. Classic theme gets the same migration.
Locale JSON stores UI strings under a translation object plus a few top-level keys. The custom backend passed the whole file into the defaultNS, so t(Home) never resolved and Chinese looked identical to English keys. Flatten translation before handing resources to i18next.
Unregistered /metrics and other relay/ops prefixes were served as the embedded index.html with HTTP 200, which breaks monitoring semantics and hides missing routes. Treat known backend prefixes as non-SPA and return API 404 JSON instead, with regression tests.
Record the readonly production cookie/HTTPS checklist, GitHub OAuth callback domain audit (including localhost server_address/passkey drift), and a no-code Cloudflare HSTS enablement runbook after API automation was blocked by token scope and dashboard challenges.
Add SecurityHeaders middleware that sets Strict-Transport-Security when TLS is present or X-Forwarded-Proto is https (Cloudflare Tunnel). Local HTTP keeps no HSTS. Also sets nosniff, Referrer-Policy, and SAMEORIGIN frame options.
Note that options were updated in the live SQLite database and HSTS is served by the app; GitHub OAuth callback still needs a manual Developer Settings change.
User confirmed Authorization callback URL is https://incc.qzz.io/oauth/github. Homepage URL still points at localhost and should be updated separately.
Homepage URL for the newapi OAuth App was changed from localhost to https://incc.qzz.io/ via the GitHub settings UI; callback remains correct.
Logout now expires the session cookie with MaxAge=-1 instead of leaving an empty Secure cookie. The UI clears auth/session-verified state and navigates to /sign-in instead of reloading the authenticated route, which previously caused getSelf 401 / Session expired loops.
Add a signing-out guard so concurrent 401s from in-flight queries do not toast Session expired or race navigation. Clear GET dedupe cache, expire the session cookie server-side, and hard-replace to /sign-in after local auth reset.
Capture the 2026-07-18 production health/logout regression results and disk cleanup notes, and refresh the operator-facing project overview.
Upstream Ollama helpers now require context.Context; restore compilation of channel pull/delete/version/fetch call sites on the rebased branch.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis large PR introduces a frontend/backend delivery seam (runtime modes, plane-aware routing, separated Docker deployment), adaptive channel balancing with a circuit breaker, session/CORS/trace security hardening, HTTP client consolidation with configurable timeouts, log trace-id/cursor pagination, Prometheus observability, quota overdraft guards, and numerous web app features (auth redirect safety, HTML sanitization, channel skip-auto-test, pricing live metrics, usage-log pagination, zh-CN default locale, bundle-size budgets). ChangesPlatform Runtime, Delivery Seam & Deployment
Security & Session Middleware
Adaptive Channel Balancing
HTTP Client & Relay Context Consolidation
Logging, Observability & Perf Metrics
Quota & Billing Integrity
Frontend Web App Features
Estimated code review effort: 5 (Critical) | ~180 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
service/billing_session.go (1)
67-78: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReturning here leaves a half-settled charge without a production retry path.
Funding is committed, but the supplied caller immediately returns this error and does not retry the session. Persist/reconcile the pending token adjustment or perform a bounded recovery before returning; an in-memory retry flag alone does not prevent permanent funding/token divergence. Also update the stale comment claiming the session is marked settled.
🤖 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/billing_session.go` around lines 67 - 78, Update the token-adjustment failure path in the billing session settlement flow around tokenErr and s.settled so a committed funding charge persists a pending adjustment for reconciliation or performs bounded recovery before returning. Ensure the durable mechanism prevents permanent funding/token divergence rather than relying on an in-memory retry flag, and revise the stale comment to reflect that the session is not marked settled at that point.relay/channel/ali/image.go (1)
241-267: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGoroutine leak on context cancellation.
The polling loop uses
time.Sleep, which blocks unconditionally and ignores context cancellation. If the client disconnects,c.Request.Context()is canceled andupdateTaskwill return an error immediately, but the loop will blindly continue to sleep for 10 seconds up to 20 times. This leaks goroutines for up to 200 seconds per canceled request.Replace
time.Sleepwith aselectblock that listens forc.Request.Context().Done()to break out of the loop early.🐛 Proposed fix to handle context cancellation
rsp, err, body := updateTask(c.Request.Context(), info, taskID) responseBody = body if err != nil { logger.LogWarn(c, "asyncTaskWait UpdateTask err: "+err.Error()) - time.Sleep(time.Duration(waitSeconds) * time.Second) + select { + case <-c.Request.Context().Done(): + return nil, nil, c.Request.Context().Err() + case <-time.After(time.Duration(waitSeconds) * time.Second): + } continue } if rsp.Output.TaskStatus == "" { return &taskResponse, responseBody, nil } switch rsp.Output.TaskStatus { case "FAILED": fallthrough case "CANCELED": fallthrough case "SUCCEEDED": fallthrough case "UNKNOWN": return rsp, responseBody, nil } if step >= maxStep { break } - time.Sleep(time.Duration(waitSeconds) * time.Second) + select { + case <-c.Request.Context().Done(): + return nil, nil, c.Request.Context().Err() + case <-time.After(time.Duration(waitSeconds) * time.Second): + } }🤖 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/ali/image.go` around lines 241 - 267, Replace both time.Sleep calls in the asyncTaskWait polling loop with context-aware select blocks using c.Request.Context().Done() and a timer, returning or breaking immediately when the request context is canceled. Preserve the existing wait duration and retry behavior for active contexts, including the error path after updateTask fails.docker-compose.dev.yml (1)
42-46: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRestore the
depends_onkey.
depends_on:is currently swallowed by the trusted-proxy comment, leavingredisandpostgresstructurally invalid.- # - TRUSTED_PROXY_CIDRS=127.0.0.1/32,::1/128 depends_on: + # - TRUSTED_PROXY_CIDRS=127.0.0.1/32,::1/128 + depends_on: redis: condition: service_started postgres:🤖 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 `@docker-compose.dev.yml` around lines 42 - 46, Restore the `depends_on:` key as an active YAML key in the service configuration, separate from the commented TRUSTED_PROXY_CIDRS line. Keep the existing redis and postgres dependency conditions nested under depends_on.
🟠 Major comments (40)
model/utils.go-87-88 (1)
87-88: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard signed batch deltas before negating them.
For the minimum
int, unary negation remains negative. This bypasses the intended floor comparison and can create a negative balance, database overflow, or quota credit.
model/utils.go#L87-L88: checked-convertvalueto a positive debit before callingdecreaseTokenQuota.model/user.go#L1256-L1263: reject or clamp an unrepresentable debit before evaluating-quotaand issuing the update.As per coding guidelines, “Billing and quota code must never produce negative charges or overflow-derived credits.”
🤖 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/utils.go` around lines 87 - 88, Guard signed quota debits against the minimum int before negation. In model/utils.go lines 87-88, checked-convert negative value to a representable positive debit before calling decreaseTokenQuota; in model/user.go lines 1256-1263, reject or clamp an unrepresentable quota debit before evaluating -quota and issuing the update. Ensure billing and quota paths never produce negative charges or overflow-derived credits.Source: Coding guidelines
middleware/auth.go-188-198 (1)
188-198: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire the refreshed user status to be explicitly enabled.
This branch rejects only
UserStatusDisabled, so an invalid or future non-enabled status can still passAdminAuth/UserAuth. Match the fail-closed check already used byTokenOrUserAuth.Proposed fix
- if statusInt == common.UserStatusDisabled { + if statusInt != common.UserStatusEnabled {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware/auth.go` around lines 188 - 198, Update the status validation in the AdminAuth/UserAuth flow around statusInt so it rejects every value except common.UserStatusEnabled, matching the fail-closed behavior in TokenOrUserAuth; preserve the existing banned-user response and abort/return handling for rejected statuses.web/default/src/features/usage-logs/lib/query-params.test.ts-19-20 (1)
19-20: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse Vitest for unit tests. As per coding guidelines, unit tests for utility functions and pure logic in
web/default/**/*.test.tsmust use Vitest instead of Node's built-in test runner.
web/default/src/features/usage-logs/lib/query-params.test.ts#L19-L20: Replace thenode:testandnode:assert/strictimports withvitestimports (describe, it, expect).web/default/src/features/usage-logs/lib/query-params.test.ts#L24-L48: Update assertions inbuildLogCursorScopetests to use Vitest'sexpect(second).toBe(first)andexpect(second).not.toBe(first).web/default/src/features/usage-logs/lib/query-params.test.ts#L50-L62: Update assertions inestimateCursorTotalCounttests to use Vitest'sexpect(...).toBe(...).🤖 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/usage-logs/lib/query-params.test.ts` around lines 19 - 20, Replace the Node test-runner imports in web/default/src/features/usage-logs/lib/query-params.test.ts:19-20 with Vitest’s describe, it, and expect imports. In web/default/src/features/usage-logs/lib/query-params.test.ts:24-48, update buildLogCursorScope assertions to use expect(...).toBe and expect(...).not.toBe; in :50-62, update estimateCursorTotalCount assertions to use expect(...).toBe(...).Source: Coding guidelines
web/default/src/features/pricing/components/pricing-toolbar.tsx-219-235 (1)
219-235: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExpose the metrics-source filter on mobile.
This control is inside
hidden sm:flex, and the mobile sheet has no equivalent. Mobile users therefore cannot switch between estimated and live-only metrics.🤖 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/components/pricing-toolbar.tsx` around lines 219 - 235, Expose the metrics-source filter for mobile by adding the existing SegmentedControl behavior to the mobile sheet, reusing its options, value derived from props.liveMetricsOnly, handleLiveMetricsOnlyChange handler, and accessibility label. Keep the current desktop control unchanged and ensure mobile users can switch between “all” and “live” metrics.web/default/src/features/pricing/components/pricing-columns.tsx-153-153 (1)
153-153: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the selected-group ratio throughout pricing rendering.
The group selection remains part of the public props, but this change drops it from every pricing calculation. Selecting a group can now filter models while displaying prices calculated with the default ratio.
web/default/src/features/pricing/components/pricing-columns.tsx#L153-L153: pass the selected group into dynamic input/output pricing.web/default/src/features/pricing/components/pricing-columns.tsx#L215-L225: restore it for token input/output formatting.web/default/src/features/pricing/components/pricing-columns.tsx#L248-L248: restore it for request pricing.web/default/src/features/pricing/components/pricing-columns.tsx#L276-L276: restore it for dynamic cached pricing.web/default/src/features/pricing/components/pricing-columns.tsx#L320-L320: restore it for cached-token formatting.web/default/src/features/pricing/components/pricing-table.tsx#L80-L85: passprops.selectedGroupintousePricingColumns.web/default/src/features/pricing/components/model-card-grid.tsx#L80-L80: continue passingprops.selectedGroupintoModelCard.🤖 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/components/pricing-columns.tsx` at line 153, Preserve the selected-group ratio across all pricing calculations and formatting. In web/default/src/features/pricing/components/pricing-columns.tsx at lines 153, 215-225, 248, 276, and 320, pass the selected group through dynamic input/output, token, request, cached, and cached-token pricing paths; in web/default/src/features/pricing/components/pricing-table.tsx lines 80-85, pass props.selectedGroup into usePricingColumns. Keep web/default/src/features/pricing/components/model-card-grid.tsx line 80 passing props.selectedGroup into ModelCard.web/default/src/features/pricing/lib/filters.ts-139-181 (1)
139-181: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter models before discarding deduplicated variants.
Deduplicating first loses vendor/group/quota/endpoint/tag metadata. For example, a base model in one group suppresses a
:freevariant in another group, so selecting the latter group returns no model. Apply the filters first, then deduplicate the remaining display rows, and add a regression test with duplicates belonging to different groups.Proposed ordering fix
- let result = dedupePricingModels(models) - result = filterBySearch(result, filters.search) + let result = filterBySearch(models, filters.search) result = filterByVendor(result, filters.vendor) result = filterByGroup(result, filters.group) result = filterByQuotaType(result, filters.quotaType) result = filterByEndpointType(result, filters.endpointType) result = filterByTag(result, filters.tag) + result = dedupePricingModels(result) result = sortModels(result, filters.sortBy)Also applies to: 218-224
🤖 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/filters.ts` around lines 139 - 181, Update the pricing model filtering flow around dedupePricingModels so vendor, group, quota, endpoint, and tag filters are applied before variants are removed or casefold duplicates are collapsed. Ensure deduplication only operates on the already-filtered display rows, preserving a matching variant when its base exists only in another group, and add a regression test covering duplicates across different groups.web/default/src/features/pricing/hooks/use-filters.ts-77-154 (1)
77-154: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSynchronize filter state when the route search changes.
This effect only writes local state to the URL. Browser back/forward navigation changes
searchwithout updatingfilterState, and the stale state can subsequently overwrite the restored URL. Make route search the source of truth or add a guarded reverse-sync effect.🤖 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/hooks/use-filters.ts` around lines 77 - 154, The filter synchronization around the existing URL-writing effect must also react to route search changes. Add a guarded reverse-sync path that parses the current route search and updates filterState, or otherwise make route search authoritative, while preventing the write effect from immediately overwriting restored browser history values. Anchor the change to the syncTimer effect and preserve the existing debounce and default-value serialization behavior.web/default/src/lib/sanitize-html.ts-247-253 (1)
247-253: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when DOM sanitization is unavailable.
The regex fallback is not an HTML sanitizer: entity-encoded protocols and dangerous elements such as
objectcan survive and become executable when rendered. Return an empty string, or use an equivalent parser-backed sanitizer for SSR.Proposed fail-closed fix
if (typeof window === 'undefined' || typeof DOMParser === 'undefined') { - return dirty - .replaceAll(/<script[\s\S]*?>[\s\S]*?<\/script>/gi, '') - .replaceAll(/<svg[\s\S]*?>[\s\S]*?<\/svg>/gi, '') - .replaceAll(/on\w+\s*=\s*("[^"]*"|'[^']*'|[^\s>]+)/gi, '') - .replaceAll(/javascript\s*:/gi, '') + 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 `@web/default/src/lib/sanitize-html.ts` around lines 247 - 253, Update the SSR fallback in the sanitizeHtml flow to fail closed when window or DOMParser is unavailable: return an empty string instead of applying the incomplete regex-based sanitization. Preserve the existing DOMParser-backed sanitization path when browser parsing is available.web/default/src/features/channels/channel-layout.test.ts-19-30 (1)
19-30: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse Vitest for unit tests.
As per coding guidelines, unit tests in the frontend must use Vitest. The current implementation uses
node:testandnode:assert/strict, which bypasses the project's standard test runner and assertions.♻️ Proposed refactor
-import assert from 'node:assert/strict' -import { describe, it } from 'node:test' +import { describe, it, expect } from 'vitest' import { CHANNEL_PAGE_SIZE_OPTIONS, DEFAULT_PAGE_SIZE } from './constants' describe('channel card pagination', () => { it('fills the three-column desktop grid without a trailing empty slot', () => { - assert.equal(DEFAULT_PAGE_SIZE % 3, 0) - assert.ok(CHANNEL_PAGE_SIZE_OPTIONS.includes(DEFAULT_PAGE_SIZE)) - assert.ok(CHANNEL_PAGE_SIZE_OPTIONS.every((pageSize) => pageSize % 3 === 0)) + expect(DEFAULT_PAGE_SIZE % 3).toBe(0) + expect(CHANNEL_PAGE_SIZE_OPTIONS).toContain(DEFAULT_PAGE_SIZE) + expect(CHANNEL_PAGE_SIZE_OPTIONS.every((pageSize) => pageSize % 3 === 0)).toBe(true) }) })🤖 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/channels/channel-layout.test.ts` around lines 19 - 30, Update the “channel card pagination” test to use Vitest imports and APIs instead of node:test and node:assert/strict: replace describe/it and assert.equal/assert.ok with the project’s Vitest equivalents, while preserving the existing test coverage and assertions.Source: Coding guidelines
web/default/src/features/channels/components/drawers/channel-mutate-drawer.tsx-4643-4645 (1)
4643-4645: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFix the placement of
ChannelSkipAutoTestField.The
ChannelSkipAutoTestFieldcomponent is currently placed outside the mainisChannelDetailLoadingternary expression and outside the drawer's layout grid. This means it will render incorrectly at the very bottom of the form, outside the intended "Channel Extra Settings" section, and it will even appear while the channel details are still loading.Move this component inside the
ADVANCED_SETTINGS_SECTION_IDS.extraSettingssection, specifically into thefieldsetcontaining the other boolean toggles.🐛 Proposed fix
Remove the component from here:
</div> )} - <ChannelSkipAutoTestField control={form.control} /> </form> </Form>And place it inside the extra settings
fieldset(around line 4173):<FormField control={form.control} name='disable_task_polling_sleep' render={({ field }) => ( ... )} /> <ChannelSkipAutoTestField control={form.control} /> </div> <FormField control={form.control} name='proxy'🤖 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/channels/components/drawers/channel-mutate-drawer.tsx` around lines 4643 - 4645, Move ChannelSkipAutoTestField from its current location after the loading ternary and before </form> into the ADVANCED_SETTINGS_SECTION_IDS.extraSettings fieldset alongside the other boolean toggles, immediately after the disable_task_polling_sleep FormField. Remove the original occurrence so it renders only within the drawer layout and after channel details finish loading.service/system_task.go-397-401 (1)
397-401: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop renewing the lease when the parent context is canceled.
The derived context is passed to the handler, but the heartbeat loop does not observe it. A non-responsive handler therefore keeps renewing its lock after shutdown cancellation, preventing timely task recovery.
Proposed fix
go func() { for { select { case <-done: return + case <-ctx.Done(): + return case <-ticker.C: if err := model.RenewSystemTaskLock(task.TaskID, runnerID, systemTaskLockUntil()); err != nil { cancel() 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 `@service/system_task.go` around lines 397 - 401, Update runWithLeaseHeartbeat so the lease-renewal heartbeat observes the derived context created with context.WithCancel and stops when that context is canceled, including cancellation inherited from parent. Ensure a non-responsive handler cannot continue renewing the lease after shutdown..github/workflows/quality.yml-21-22 (1)
21-22: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable persisted checkout credentials throughout the quality workflow. Each job subsequently runs repository-controlled build or test code.
.github/workflows/quality.yml#L21-L22: addpersist-credentials: falseto the Go quality checkout..github/workflows/quality.yml#L77-L78: addpersist-credentials: falseto the web quality checkout..github/workflows/quality.yml#L131-L132: addpersist-credentials: falseto the image build checkout.🤖 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 @.github/workflows/quality.yml around lines 21 - 22, Disable persisted checkout credentials by adding persist-credentials: false to the checkout steps at .github/workflows/quality.yml lines 21-22, 77-78, and 131-132, covering the Go quality, web quality, and image build jobs.Source: Linters/SAST tools
.env.example-89-89 (1)
89-89: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse the exact reverse-proxy subnet instead of
172.16.0.0/12. The broad range grants forwarding-header trust to unrelated private-network clients.
.env.example#L89-L89: replace the/12example with instructions for discovering and using the specific Compose subnet.docker-compose.yml#L40-L40: use a narrow placeholder or the explicitly configured proxy network subnet.🤖 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 @.env.example at line 89, Replace the broad reverse-proxy subnet example in .env.example at lines 89-89 with instructions to discover and configure the exact Docker Compose proxy subnet. Update the trust-subnet placeholder in docker-compose.yml at lines 40-40 to use a narrow value or the explicitly configured proxy network subnet, preserving forwarding-header trust only for that network.deploy/prometheus/new-api-alerts.yml-6-9 (1)
6-9: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExclude 4xx responses from the availability denominator.
The SLO in
docs/operations/slo.mdis defined over non-4xx requests, but this denominator includes 4xx traffic and can mask Relay failures.- clamp_min(sum(rate(newapi_http_requests_total{route_class="relay"}[5m])), 0.001) + clamp_min(sum(rate(newapi_http_requests_total{route_class="relay",status!~"4.."}[5m])), 0.001)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/prometheus/new-api-alerts.yml` around lines 6 - 9, Update the denominator in the relay availability expression to exclude 4xx responses, matching the non-4xx SLO definition in the surrounding newapi_http_requests_total rate calculation. Keep the existing 5-minute window, route_class filter, and clamp_min protection unchanged.docker-compose.yml-27-32 (1)
27-32: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not insert raw passwords into connection URIs.
Passwords containing
@,:,/,#, or%can produce invalid PostgreSQL or Redis URLs. Require separately URL-encoded DSN values, or construct these URLs in an entrypoint that performs encoding.- - SQL_DSN=postgresql://${POSTGRES_USER:-newapi}:${POSTGRES_PASSWORD:?Set POSTGRES_PASSWORD}`@postgres`:5432/${POSTGRES_DB:-new-api} + - SQL_DSN=postgresql://${POSTGRES_USER:-newapi}:${POSTGRES_PASSWORD_URLENCODED:?Set POSTGRES_PASSWORD_URLENCODED}`@postgres`:5432/${POSTGRES_DB:-new-api} ... - - REDIS_CONN_STRING=redis://:${REDIS_PASSWORD:?Set REDIS_PASSWORD}`@redis`:6379 + - REDIS_CONN_STRING=redis://:${REDIS_PASSWORD_URLENCODED:?Set REDIS_PASSWORD_URLENCODED}`@redis`:6379🤖 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 `@docker-compose.yml` around lines 27 - 32, Update the SQL_DSN and REDIS_CONN_STRING definitions to avoid interpolating raw passwords directly into connection URIs. Require URL-encoded DSN values or generate the connection strings through an entrypoint that encodes credentials, and apply the same protection to the commented MySQL and LOG_SQL_DSN examples.docs/operations/cookie-https-readonly-checklist.md-14-31 (1)
14-31: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMark superseded audit findings as historical and update current-state summaries.
These documents mix pre-remediation observations with later production evidence, so operators can misread fixed issues as still active.
docs/operations/cookie-https-readonly-checklist.md#L14-L31: update the executive summary to reflect the later application-layer HSTS deployment, or label this section with its snapshot timestamp.docs/operations/oauth-callback-domain-checklist.md#L114-L127: mark the localhost values as pre-fix and surface the correctedServerAddressand Passkey values in the current summary.docs/operations/optimization-audit-2026-07-18.md#L110-L129: mark P0-1 as resolved and link the deployment evidence recorded at Lines 237-250.🤖 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/operations/cookie-https-readonly-checklist.md` around lines 14 - 31, Update the current-state summaries to distinguish historical pre-remediation findings from verified production status: in docs/operations/cookie-https-readonly-checklist.md lines 14-31, reflect the later application-layer HSTS deployment or label the summary with its snapshot timestamp; in docs/operations/oauth-callback-domain-checklist.md lines 114-127, mark localhost values as pre-fix and include the corrected ServerAddress and Passkey values; in docs/operations/optimization-audit-2026-07-18.md lines 110-129, mark P0-1 resolved and link the deployment evidence at lines 237-250.docs/operations/oauth-callback-domain-checklist.md-145-154 (1)
145-154: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the OAuth cookie guidance.
SameSite=Strict不适合这个 GitHub 回调：/api/oauth/state写入的 session cookie 在跨站回跳时通常不会带回，state校验会失败。这里应改成Lax，或改为同站回调/中间跳转方案，不要写成 Strict 可能正常工作。🤖 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/operations/oauth-callback-domain-checklist.md` around lines 145 - 154, 更新 OAuth cookie 指引，移除“SameSite=Strict 可能正常工作”的表述；明确 GitHub 回调场景应使用 SameSite=Lax，或采用同站回调/中间跳转方案，并保持 /api/oauth/state 的 state 校验要求一致。main.go-44-49 (1)
44-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLoad
.envbefore parsing runtime configuration.
InitResources()loads.env, butRUN_MODE,APP_PLANE, andNODE_TYPEare read before that call. Values defined only in.envare therefore ignored, potentially startingall/allinstead of the configured worker, scheduler, migration, or plane role.Move dotenv loading ahead of
parseRuntimeConfig, preferably into a small initialization step that runs only once.🤖 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 `@main.go` around lines 44 - 49, Update the initialization flow in main so dotenv loading occurs before the os.Getenv calls passed to parseRuntimeConfig, ensuring .env values are available during runtime configuration parsing. Extract or reuse a one-time initialization step for loading dotenv, then keep InitResources and the existing error handling behavior intact.runtime_mode.go-59-64 (1)
59-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not let
RUN_MODE=allbypass slave-role validation.
runModeAllruns both workers and schedulers, but this check rejects only the exactworkerandschedulermodes. Consequently,RUN_MODE=all NODE_TYPE=slavestarts the same restricted capabilities.Proposed fix
- if (mode == runModeWorker || mode == runModeScheduler) && strings.EqualFold(strings.TrimSpace(nodeType), "slave") { + if (mode.runsWorker() || mode.runsScheduler()) && + strings.EqualFold(strings.TrimSpace(nodeType), "slave") { return "", "", fmt.Errorf("RUN_MODE=%s requires NODE_TYPE to be master or unset", mode) }Add an explicit
all/all/slaverejection test.🤖 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 `@runtime_mode.go` around lines 59 - 64, Update the validation around the runModeWorker/runModeScheduler check in runtime mode parsing to include runModeAll, so RUN_MODE=all with NODE_TYPE=slave returns the same rejection error. Add an explicit test covering the all/all/slave combination and preserve existing behavior for master or unset node types.router/main.go-133-141 (1)
133-141: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep backend paths out of the frontend redirect fallback.
Unlike embedded mode, this
NoRoutehandler redirects unmatched/api, relay, metrics, and health paths. API typos can therefore return a cached 301—and non-GET requests may be rewritten to GET—instead of the expected backend 404.Proposed fix
router.NoRoute(func(c *gin.Context) { c.Set(middleware.RouteTagKey, "web") + if isNonSPARequestPath(c.Request.RequestURI) { + controller.RelayNotFound(c) + return + } c.Redirect(http.StatusMovedPermanently, fmt.Sprintf("%s%s", frontendBaseURL, c.Request.RequestURI)) })Add a redirect-mode regression test for an unknown
/api/...path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/main.go` around lines 133 - 141, Update registerFrontendRedirect so its NoRoute fallback does not redirect unmatched backend paths, including /api, relay, metrics, and health endpoints; let those requests continue to the backend 404 behavior instead. Preserve the frontend redirect for non-backend paths and add a redirect-mode regression test covering an unknown /api/... request.main.go-299-305 (1)
299-305: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a ReadTimeout for request bodies.
ReadHeaderTimeoutonly covers headers; slow uploads can still occupy handlers indefinitely, even with the existing body-size limits. Add a configurableReadTimeouthere and leaveWriteTimeoutunset for SSE.🤖 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 `@main.go` around lines 299 - 305, Add a configurable ReadTimeout field in newHTTPServer alongside ReadHeaderTimeout, using the existing environment-based duration pattern and a suitable default for request-body reads. Do not set WriteTimeout, preserving support for SSE connections.Source: Linters/SAST tools
web/default/src/i18n/locales/ja.json-5176-5188 (1)
5176-5188: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the new entries inside
translation.Line 5176 closes the
translationobject before the newly added Japanese keys. These root-level keys will not be resolved byt()through the locale’stranslationnamespace, so the new UI strings will fall back or display untranslated keys.Move the closing brace after
"Metrics source filter".Proposed fix
- }, - "Skip Auto Test": "自動テストをスキップ", + "Skip Auto Test": "自動テストをスキップ", ... - "Metrics source filter": "指標ソースフィルタ" + "Metrics source filter": "指標ソースフィルタ", + } }🤖 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/i18n/locales/ja.json` around lines 5176 - 5188, Move the closing brace that currently precedes "Skip Auto Test" to after "Metrics source filter", keeping all newly added Japanese entries inside the locale’s translation object so they resolve through t().web/default/src/i18n/locales/zh.json-5176-5188 (1)
5176-5188: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the new entries inside the
translationobject.Line 5176 closes the translation namespace before the newly added keys, so
t('Skip Auto Test'),t('All metrics'), and the other new lookups will miss these Chinese translations and fall back to English.Proposed fix
- }, - "Skip Auto Test": "跳过自动测试", - "Exclude this channel from automatic batch tests; manual test still works": "从自动批量测试中排除该渠道；手动测试仍可用", - "Skip auto test for selected": "选中渠道跳过自动测试", - "Join auto test for selected": "选中渠道参与自动测试", - "{{count}} channel(s) will skip auto test": "已设置 {{count}} 个渠道跳过自动测试", - "{{count}} channel(s) will join auto test": "已设置 {{count}} 个渠道参与自动测试", - "Failed to update skip auto test": "更新跳过自动测试失败", - "All metrics": "全部指标", - "Live only": "仅实测", - "Show estimated badges when no live traffic": "无实测时显示示意徽章", - "Only show real probe / relay metrics": "仅显示真实探测/转发指标", - "Metrics source filter": "指标来源筛选" + "Skip Auto Test": "跳过自动测试", + "Exclude this channel from automatic batch tests; manual test still works": "从自动批量测试中排除该渠道；手动测试仍可用", + "Skip auto test for selected": "选中渠道跳过自动测试", + "Join auto test for selected": "选中渠道参与自动测试", + "{{count}} channel(s) will skip auto test": "已设置 {{count}} 个渠道跳过自动测试", + "{{count}} channel(s) will join auto test": "已设置 {{count}} 个渠道参与自动测试", + "Failed to update skip auto test": "更新跳过自动测试失败", + "All metrics": "全部指标", + "Live only": "仅实测", + "Show estimated badges when no live traffic": "无实测时显示估算徽章", + "Only show real probe / relay metrics": "仅显示真实探测/转发指标", + "Metrics source filter": "指标来源筛选" + },🤖 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/i18n/locales/zh.json` around lines 5176 - 5188, Move the newly added keys from the outer object into the existing translation object in the locale structure. Ensure “Skip Auto Test” through “Metrics source filter” are siblings of the other translation entries so lookups such as t('Skip Auto Test') resolve to the Chinese values.web/default/src/features/auth/lib/safe-redirect.test.ts-19-39 (1)
19-39: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse Vitest for unit tests.
As per coding guidelines, unit tests for utility functions and pure logic in
web/default/**/*.test.tsmust be written with Vitest. Please replace thenode:testandnode:assertimports with Vitest'sdescribe,it, andexpect.♻️ Proposed fix
-import assert from 'node:assert/strict' -import { describe, it } from 'node:test' +import { describe, expect, it } from 'vitest' import { safeRedirect } from './safe-redirect' describe('safeRedirect', () => { it('rejects browser-normalized network-path redirects', () => { - assert.equal(safeRedirect('/\\\\evil.example', '/dashboard'), '/dashboard') - assert.equal(safeRedirect('/\t/evil.example', '/dashboard'), '/dashboard') - assert.equal( - safeRedirect('/\u0000evil.example', '/dashboard'), - '/dashboard' - ) + expect(safeRedirect('/\\\\evil.example', '/dashboard')).toBe('/dashboard') + expect(safeRedirect('/\t/evil.example', '/dashboard')).toBe('/dashboard') + expect(safeRedirect('/\u0000evil.example', '/dashboard')).toBe('/dashboard') }) it('keeps normal same-app paths', () => { - assert.equal( - safeRedirect('/dashboard?tab=models#health', '/fallback'), - '/dashboard?tab=models#health' - ) + expect(safeRedirect('/dashboard?tab=models#health', '/fallback')).toBe('/dashboard?tab=models#health') }) })🤖 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/auth/lib/safe-redirect.test.ts` around lines 19 - 39, Update the safeRedirect tests to use Vitest: replace the node:test and node:assert/strict imports with Vitest’s describe, it, and expect, then convert the existing assert.equal checks to equivalent expect assertions while preserving all test cases and expected results.Source: Coding guidelines
service/channel_circuit.go-245-249 (1)
245-249: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not discard the acquired circuit permit.
A successful
ProbeHalfOpenincrementsHalfOpenInFlight, but the boolean-only API gives callers no permit to record or release. Remove this wrapper if unused, or return(CircuitPermit, bool).🤖 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_circuit.go` around lines 245 - 249, Update ProbeHalfOpen so it does not discard a successful circuit permit: remove the wrapper if no callers use it, otherwise change it to return the acquired CircuitPermit alongside the success boolean and preserve the AcquireCircuitPermit result for callers to record or release.service/channel_metrics.go-42-58 (1)
42-58: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound or expire the per-model metrics registry.
Every distinct
(channelID, group, model)creates a permanent entry. High-cardinality model/group values can grow process memory indefinitely and make each Redis snapshot progressively larger. Add a capacity/TTL policy or normalize entries into bounded buckets.🤖 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_metrics.go` around lines 42 - 58, Bound the registry managed by LocalMetricsSnapshot.ensureKey so distinct metricsKey values cannot accumulate indefinitely. Add an explicit capacity and/or TTL eviction policy, or normalize keys into a fixed set of buckets, while preserving creation of default ChannelMetrics values for retained entries and ensuring Redis snapshots exclude expired or evicted entries.service/channel_circuit.go-213-224 (1)
213-224: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInvalidate timed-out half-open permits before issuing replacements.
Resetting
HalfOpenInFlightwithout incrementingGenerationleaves the expired permit valid. Its late success can close the circuit while the replacement probe is still running.Proposed fix
if cb.HalfOpenInFlight > 0 && !cb.HalfOpenSince.IsZero() && time.Since(cb.HalfOpenSince) > halfOpenProbeTimeout { cb.HalfOpenInFlight = 0 cb.HalfOpenSince = time.Time{} + cb.Generation++ }🤖 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_circuit.go` around lines 213 - 224, The half-open timeout reset in the circuit permit flow must invalidate the expired probe before issuing a replacement. In the timeout branch of the half-open permit logic, increment cb.Generation along with clearing HalfOpenInFlight and HalfOpenSince, so late results from the old permit cannot close the circuit while the replacement probe runs.service/channel_adaptive.go-49-62 (1)
49-62: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate the resolved auto group back into adaptive scoring.
getCandidateChannelsresolves"auto"only in its localgroup. The caller therefore scores and reads affinity under"auto", while relay result metrics are recorded under the concrete group. Adaptive learning for auto groups never consumes its own observations.Proposed fix
channels, err := getCandidateChannels(group, modelName, param) if err != nil { return nil, group, err } +if param.TokenGroup == "auto" { + if resolvedGroup := common.GetContextKeyString(ctx, constant.ContextKeyAutoGroup); resolvedGroup != "" { + group = resolvedGroup + } +}Also applies to: 181-211
🤖 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_adaptive.go` around lines 49 - 62, Update the adaptive channel flow around getCandidateChannels so the resolved concrete group is returned or otherwise propagated to the caller before affinity lookup and scoring. Use that resolved group for getPreferredChannelID, ScoreCandidates, and downstream relay result metrics, while preserving the existing legacy fallback for empty channels.model/channel_cache.go-114-126 (1)
114-126: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftReturn all eligible DB candidates when memory caching is disabled.
The fallback returns one random channel rather than the candidate set. If that channel is circuit-open or already used, adaptive filtering returns no channel and terminates the request even when other database channels are available. Query all eligible channels through portable GORM logic.
🤖 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_cache.go` around lines 114 - 126, Update GetSatisfiedChannels so the memory-cache-disabled fallback queries and returns all eligible channels for the group, model, and requestPath using portable GORM logic, ordered by highest priority first. Do not call GetChannel or select a single random channel; preserve the empty-result behavior when no candidates exist.service/channel_select.go-131-131 (1)
131-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep adaptive exclusions out of disabled and shadow routing.
These legacy calls always consume
adaptiveUsedChannelSet, which is populated for every attempt. Consequently, adaptive-disabled and shadow-mode retries exclude the previous channel; single-channel deployments stop retrying, and shadow mode changes the routing it is meant only to observe.Pass exclusions only when adaptive routing is active and not in shadow mode.
Also applies to: 169-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/channel_select.go` at line 131, Update the legacy channel-selection calls around GetRandomSatisfiedChannelExcluding at both referenced sites so adaptiveUsedChannelSet is passed only when adaptive routing is enabled and shadow mode is inactive; otherwise provide no exclusions, preserving normal adaptive exclusions when active.service/channel_affinity.go-800-837 (1)
800-837: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the HybridCache memory tier coherent with Redis.
The Redis branch bypasses
cache.SetWithTTL, and Lua evictions delete only Redis keys. Existing local entries can therefore keep returning an old channel—or an evicted affinity—until their local TTL expires, defeating updates and the configured capacity limit.Update/invalidate the local tier after the script and return evicted keys so they can also be removed locally.
🤖 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_affinity.go` around lines 800 - 837, Update channelAffinityRedisSetScript and setChannelAffinityWithLimit so the Redis script returns the keys it evicts, then remove those entries from the local HybridCache tier after a successful Eval. Also update the local entry for the requested key with the same channelID and TTL, preserving Redis errors and ensuring local evictions match the configured capacity limit.service/channel_adaptive_test.go-28-39 (1)
28-39: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not enable adaptive routing globally for every service test.
This
initpermanently changes shared constants and metrics for unrelated tests in the package, making their behavior depend on this file. Move setup into a helper that snapshots state and restores it witht.Cleanup.As per coding guidelines, backend tests must initialize settings and cache state explicitly in fixtures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/channel_adaptive_test.go` around lines 28 - 39, Remove the package-wide initialization from init and introduce a test fixture helper that accepts *testing.T, snapshots the relevant global constants and globalSnapshot.metrics state, applies the adaptive-routing settings for the test, and registers t.Cleanup to restore all captured values. Update adaptive-routing tests to call this helper explicitly so unrelated service tests retain their own configuration.Source: Coding guidelines
controller/relay.go-393-400 (1)
393-400: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor explicit skip-retry before channel and quota classification.
IsChannelErrorand the quota classifier currently returntruebeforeIsSkipRetryErroris checked. Errors explicitly marked non-retryable can therefore trigger duplicate upstream attempts. Move the skip check ahead of both classifiers.Proposed fix
+if types.IsSkipRetryError(openaiErr) { + return false +} if isUpstreamChannelQuotaError(openaiErr) { return true } if types.IsChannelError(openaiErr) { return true } -if types.IsSkipRetryError(openaiErr) { - return false -}🤖 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 - 400, Update the error classification flow around isUpstreamChannelQuotaError and types.IsChannelError to evaluate types.IsSkipRetryError(openaiErr) first and immediately return false for explicitly non-retryable errors; only run the channel and quota classifiers when the skip-retry check does not match.common/env.go-40-49 (1)
40-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject non-finite EWMA values
strconv.ParseFloatacceptsNaNand±Inf, andcommon/init.gostores the result directly inconstant.EwmaAlpha.EwmaUpdateonly falls back onalpha <= 0 || alpha > 1, soNaNslips through and can poison the EWMA state. Reject non-finite values here and enforce0 < EWMA_ALPHA <= 1before assignment.🤖 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/env.go` around lines 40 - 49, Update GetEnvOrDefaultFloat to reject non-finite parsed values and enforce the EWMA_ALPHA range of 0 < value <= 1 before common/init.go assigns constant.EwmaAlpha. Treat invalid, NaN, infinite, or out-of-range values like parse failures, log the existing default-value error, and return defaultValue.controller/relay.go-215-230 (1)
215-230: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the circuit permit when an attempt panics.
The deferred cleanup only decrements concurrency. If a helper panics,
RecordAdaptiveResultis skipped and a half-open permit can remain occupied after recovery, preventing future probes.Proposed fix
+attemptCompleted := false func() { - defer service.DecChannelConcurrency(channel.Id) + defer func() { + service.DecChannelConcurrency(channel.Id) + if !attemptCompleted { + service.ReleaseAdaptiveCircuitPermit(c, channel.Id) + } + }() switch relayFormat { // ... } + attemptCompleted = true }()🤖 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 215 - 230, Update the attempt wrapper around the relay helper switch to ensure a panic also releases the circuit permit before recovery continues. Add the required cleanup alongside the existing service.DecChannelConcurrency defer, while preserving normal RecordAdaptiveResult behavior and the existing helper dispatch in the relay format switch.model/log.go-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the project JSON wrappers for cursor serialization.
These direct
json.Marshalandjson.Unmarshalcalls are application-level serialization and must usecommon.Marshalandcommon.Unmarshal.Proposed fix
- "encoding/json" ... - payload, err := json.Marshal(logCursor{ + payload, err := common.Marshal(logCursor{ ... - if err := json.Unmarshal(payload, &cursor); err != nil || cursor.CreatedAt <= 0 { + if err := common.Unmarshal(payload, &cursor); err != nil || cursor.CreatedAt <= 0 {As per coding guidelines, all JSON marshal and unmarshal operations in business code must use the wrappers in
common/json.go.Also applies to: 132-159
🤖 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/log.go` at line 6, Replace the direct encoding/json usage in the cursor serialization and deserialization paths with the project wrappers common.Marshal and common.Unmarshal, including the operations around the referenced lines. Remove the direct JSON dependency if no longer used, while preserving the existing cursor behavior and error handling.Source: Coding guidelines
controller/trace.go-23-26 (1)
23-26: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not expose database errors in the trace API response.
Returning
err.Error()can disclose SQL, schema, or infrastructure details. Log the error server-side and return a stable generic message.Proposed fix
logs, err := model.GetLogsByTraceId(traceId, 200) if err != nil { - c.JSON(http.StatusInternalServerError, gin.H{"success": false, "message": err.Error()}) + common.SysError("failed to query trace logs: " + err.Error()) + c.JSON(http.StatusInternalServerError, gin.H{ + "success": false, + "message": "failed to query trace logs", + }) 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/trace.go` around lines 23 - 26, Update the error branch in the trace handler around GetLogsByTraceId to log the database error server-side, then return a stable generic message in the JSON response instead of err.Error(). Preserve the existing HTTP 500 status and early return behavior.controller/channel.go-1349-1377 (1)
1349-1377: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not report partial batch failures as full success.
Missing channels and failed updates are silently ignored, yet the response always returns
success: true. Return failed IDs/counts—or fail the request when none succeed—so callers do not assume every selected channel was updated.🤖 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 1349 - 1377, The batch handler around the channel update loop must track missing channels and Update failures instead of always returning success. Record failed IDs or a failure count, include that information in the response, and set success to false when no requested channels are updated; preserve the existing successful update count and audit behavior.controller/channel_auto_test_helpers_test.go-12-93 (1)
12-93: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse Testify assertions in these new backend tests.
Replace
t.Fatal/t.Fatalfwithrequirefor fatal checks andassertfor table-case value checks.As per coding guidelines, “New or substantially rewritten Go backend tests must use
testify/requirefor setup and fatal assertions andtestify/assertfor 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 `@controller/channel_auto_test_helpers_test.go` around lines 12 - 93, Update the new tests TestDetectProbeModelKind, TestIsChatCapableProbeModel, TestPickAutoTestModel, TestShouldSkipAutoChannelTest, and TestNormalizeChannelTestEndpointInfersImage to use testify assertions: replace table-case and value comparisons with assert, and replace fatal/setup checks with require. Add the needed testify imports while preserving all existing test expectations.Source: Coding guidelines
controller/channel-test.go-1006-1009 (1)
1006-1009: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep all synthetic channel probes in the
probegroup.Successful probes currently use the test user’s real group, while failures without
relayInforemain inprobe. This contaminates production-group latency data and biases success rates. Always record channel-test samples underprobe.Proposed fix
probeGroup := "probe" - if result.relayInfo != nil && result.relayInfo.UsingGroup != "" { - probeGroup = result.relayInfo.UsingGroup - }🤖 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-test.go` around lines 1006 - 1009, Update the probe-group assignment near probe result recording to always use the literal "probe" group; remove the conditional reuse of result.relayInfo.UsingGroup while preserving the existing channel-test sample recording flow.
CodeRabbit #6286: applying RELAY_TIMEOUT to http.Client.Timeout aborts long-lived AI streams because Client.Timeout covers the full response body read. Stall protection already uses ResponseHeaderTimeout on the shared transport; non-streaming callers keep GetHttpClientWithTimeout.
nonSPAPathPrefixes previously treated the entire /dashboard tree as an API surface, so /dashboard/overview returned JSON 404 instead of the embedded console HTML. Only the OpenAI-compatible billing APIs under /dashboard/billing should stay non-SPA.
|
Withdrawing this PR for now — the change set has grown too broad for a single reliable review. I'll re-audit the full diff locally and follow up with smaller, independently verifiable PRs. Thanks for CodeRabbit's feedback (both actionable items already addressed in 6f18328/e0ede6b6). |
2 participants
Summary
QuantumNous/new-apimain (rebase/upstream-pr-6284).X-Forwarded-Proto, SPA no longer masks/metricsand other ops paths as HTML 200.frontend_external,FRONTEND_MODE,Dockerfile.backend, separated Nginx frontend (embedded dual-theme remains default).Conflict resolution during rebase
FetchModelsand channel upstream model discovery helpers (upstream already had the more complete Advanced Custom / Codex path).context.Contextas required by current upstream Ollama helpers.Test plan
go test ./controller ./router ./middlewarego test -tags frontend_external .go test ./pkg/observability ./serviceNotes
Summary by CodeRabbit