Conversation
为登录后用户补充使用文档入口 [未完成]
The removeKey action previously returned hardcoded Chinese strings or generic messages, causing the UI to display unhelpful toasts via the REST bridge. It now returns specific error codes that the UI translates through the errors namespace. Additionally, the key creation dialogs incorrectly claimed the full key is only shown once. The copy now accurately reflects that owners and admins can reveal and copy the full key from the key list at any time. Includes regression tests for the action error codes, API error mapping, UI toast translation, and i18n copy guards.
Non-admin dashboard users previously could not create API keys because the per-user key creation route was admin-only. This adds a new read-tier POST /api/v1/users:self/keys endpoint that derives the target user from the authenticated session, rejecting read-only sessions to prevent privilege escalation. The API client now falls back to this self-service endpoint when the admin route returns 403 Forbidden. Additionally, raw REST error codes are now mapped to translation keys via getApiErrorMessageKey so frontend forms display localized messages instead of literal error code strings.
When Anthropic-compatible providers like DeepSeek or MiMo receive a request with thinking disabled but reasoning_effort or output_config.effort set, they return a 400 error. This rectifier detects the specific error message, strips the conflicting effort fields while preserving the disabled thinking state, and retries the request once against the same provider. Adds a new system setting enableThinkingEffortConflictRectifier (enabled by default) with full UI, API, and database migration support to control this behavior.
Add missing enableThinkingEffortConflictRectifier field to UpdateSystemSettingsInput to resolve a typecheck failure. Strip only the effort key from output_config in the thinking effort conflict rectifier instead of deleting the entire object, preserving any sibling configuration fields. Extend the system_settings database degradation fallback chain to handle missing enableThinkingEffortConflictRectifier columns in un-migrated databases for both read and update operations.
Deny-by-default canLoginWebUi check (only explicit true proceeds) to prevent read-tier sessions from minting Web-UI-capable keys. Spread session userId last in addKey payload to prevent the request body from overriding the target user. Add tests covering toVoidActionResult delete-code passthrough and double-403 self-fallback permission denied mapping.
The previous assertion only checked the transformer default value for the new column, which did not guarantee the fallback query actually stripped the correct column. The test now inspects the second select call to ensure the newly added column is removed while older fallback columns remain, catching regressions in the degradation chain order.
A preceding test left a persistent implementation on postMock, which interfered with the mockRejectedValueOnce calls in the double-403 self-fallback case. Adding mockReset ensures only the intended rejections are in play.
…reated-copy fix(keys): removeKey errorCode passthrough + key-creation dialog copy aligned with real reveal behavior
fix(keys): non-admin self-service key creation (#1259) + errorCode display mapping
…ectifier feat(proxy): thinking effort conflict rectifier for DeepSeek/MiMo subagent thinking-disabled errors (#1257)
Provider create and edit dialogs contain long forms that were previously dismissed by accidental outside clicks, window focus loss, or the Escape key, leading to potential data loss. Introduce explicitCloseOnlyDialogProps to neutralize implicit close triggers and apply it across all provider and vendor key dialogs. The dialogs now only close via the explicit close button, cancel, or successful submission. Includes unit tests for the new dialog utility.
The previous explicitCloseOnlyDialogProps helper blocked the Escape key along with outside clicks and window blur, which was too restrictive for users trying to dismiss long forms. Rename the helper to preventCloseOnOutsideInteraction and drop the onEscapeKeyDown interceptor. This keeps the protection against accidental click-aways and focus loss while letting Escape close the dialog as expected.
The file dialog-explicit-close.test.ts is renamed to dialog-prevent-close-on-outside-interaction.test.ts to align with the preventCloseOnOutsideInteraction helper it actually covers.
Consolidate the preventCloseOnOutsideInteraction guard and standard layout classes into a shared ProviderFormDialogContent component. This ensures all provider create, edit, and clone dialogs consistently prevent accidental dismissal via outside clicks or window blur without requiring manual prop spreading. Remove the standalone unit test for the interaction guard since the behavior is now encapsulated within the component.
…-blur fix(providers): provider create/edit dialogs close only on explicit action, not on outside click or window blur
…1251) * fix(proxy): finalize complete responses after client abort * fix(proxy): sanitize inert chat chunks in responses streams
* fix(proxy): preserve hedge loser Codex priority billing * fix(proxy): track hedge loser Redis cost with billing snapshot
📝 WalkthroughWalkthrough新增 thinking-effort 冲突整流器并将其作为系统设置（默认开启）贯通至 proxy forwarder；收紧 key 写入权限、增加自助建 key API 与错误码映射；添加前端使用文档入口与对话框防外部关闭工具；并补充大量单元/集成测试与数据库迁移。 Changes主要变更（汇总）
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
github-actions
Bot
added
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Thinking Effort Conflict Rectifier' to handle payload conflicts on Anthropic-compatible providers (such as DeepSeek), adds a self-service API key creation endpoint for non-admin users, and improves error handling and translation mapping for key deletions. It also updates the ResponseFixer to filter out inert chat completion chunks and introduces a dialog guard to prevent accidental dismissals of long forms. Feedback on these changes highlights a potential runtime crash in the dashboard header due to unsafe access to session.key, as well as a risk of UTF-8 character corruption in the streaming response fixer when decoding chunks without a stateful, stream-aware TextDecoder.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export async function DashboardHeader({ session, locale }: DashboardHeaderProps) { | ||
| const t = await getTranslations({ locale, namespace: "dashboard.nav" }); | ||
| const isAdmin = session?.user.role === "admin"; | ||
| const canUseDashboard = !!session && (isAdmin || session.key.canLoginWebUi); |
Contributor
There was a problem hiding this comment.
Potential runtime crash: session.key can be undefined or null (for example, in sessions authenticated via password or OAuth rather than an API key). Accessing session.key.canLoginWebUi directly will throw a TypeError.
Use optional chaining (session.key?.canLoginWebUi) to safely handle cases where the session does not have an associated key.
Suggested change
| const canUseDashboard = !!session && (isAdmin || session.key.canLoginWebUi); | |
| const canUseDashboard = !!session && (isAdmin || session.key?.canLoginWebUi); |
References
- When suggesting optional chaining (
?.), verify that a guard clause checking for the object's nullability doesn't already exist earlier in the function.
Comment on lines
+589
to
+591
| if (session.originalFormat !== "response") { | ||
| return { data, applied: false }; | ||
| } |
Contributor
There was a problem hiding this comment.
Decoding and re-encoding arbitrary stream chunks using a global TextDecoder without { stream: true } can lead to UTF-8 multi-byte character corruption if a character is split across chunk boundaries.
Since filterInertResponsesChatCompletionChunks is called on raw stream chunks, any split multi-byte character at the boundary of a matching chunk will be decoded as a replacement character (``) and permanently corrupted upon re-encoding. Consider using a per-session stateful TextDecoder with `{ stream: true }` to safely handle split UTF-8 characters.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 553a310568
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+589
to
+590
| const { usageMetrics } = parseUsageFromResponseText(allContent, provider?.providerType); | ||
| return hasPositiveBillableTokens(usageMetrics); |
There was a problem hiding this comment.
When a client disconnects from an Anthropic/Claude SSE stream after message_start but before the terminal message_delta/completion, parseUsageFromResponseText() can still return positive input tokens from the early message_start event. This new check therefore classifies that truncated client-aborted stream as a completed 2xx success, which updates the request as completed and records provider/session success instead of a 499/client abort. Please require evidence of a terminal/completed stream event rather than any positive usage before taking the success path.
Useful? React with 👍 / 👎.
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
This is a well-structured release PR (v0.8.6) bundling 8 individually reviewed PRs. The proxy-layer changes (thinking-effort rectifier, client-abort drain, hedge-loser billing, response-fixer SSE filtering) follow established patterns from prior rectifiers and are thoroughly tested. The self-service key-creation endpoint has proper authorization fencing (session-derived userId, canLoginWebUi gate, Zod strict schema rejecting extra fields). Error transparency fixes and provider dialog UX are clean, minimal, and i18n-complete across all 5 locales. No significant issues identified.
PR Size: XL
- Lines changed: 8,524 (8,410 additions + 114 deletions)
- Files changed: 84
Suggestion for future releases: Consider splitting proxy-layer (rectifier + billing + stream) and UI-layer (dialogs + i18n + key management) into separate release PRs to reduce review surface. The 19+ test files and auto-generated migration snapshot inflate the file count significantly.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean (self-service endpoint properly fenced)
- Error handling - Clean (all error paths logged, no silent catches)
- Type safety - Clean (follows existing
as never[]action bridge pattern) - Documentation accuracy - Clean (rectifier JSDoc matches behavior)
- Test coverage - Comprehensive (19+ test files, edge cases covered)
- Code clarity - Clean (new
ProviderFormDialogContentandpreventCloseOnOutsideInteractionreduce duplication)
Automated review by Claude AI
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+18
to
+24
| export type ThinkingEffortConflictRectifierResult = { | ||
| applied: boolean; | ||
| removedOutputConfig: boolean; | ||
| removedReasoningEffort: boolean; | ||
| thinkingType: string | null; | ||
| effort: string | null; | ||
| }; |
There was a problem hiding this comment.
removedOutputConfig is set to true even when only the effort key is stripped from output_config (and the object itself survives). An admin reading the audit record who interprets this as "the entire output_config block was removed" will draw the wrong conclusion in the partial-strip case. A name like removedEffortFromOutputConfig (or a dedicated removedOutputConfigEffort boolean) would remove the ambiguity.
Suggested change
| export type ThinkingEffortConflictRectifierResult = { | |
| applied: boolean; | |
| removedOutputConfig: boolean; | |
| removedReasoningEffort: boolean; | |
| thinkingType: string | null; | |
| effort: string | null; | |
| }; | |
| export type ThinkingEffortConflictRectifierResult = { | |
| applied: boolean; | |
| /** True when the `effort` field inside `output_config` was stripped (the key itself may still exist if it had other fields). */ | |
| removedOutputConfigEffort: boolean; | |
| removedReasoningEffort: boolean; | |
| thinkingType: string | null; | |
| effort: string | null; | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/thinking-effort-conflict-rectifier.ts
Line: 18-24
Comment:
The audit field `removedOutputConfig` is set to `true` even when only the `effort` key is stripped from `output_config` (and the object itself survives). An admin reading the audit record who interprets this as "the entire `output_config` block was removed" will draw the wrong conclusion in the partial-strip case. A name like `removedEffortFromOutputConfig` (or a dedicated `removedOutputConfigEffort` boolean) would remove the ambiguity.
```suggestion
export type ThinkingEffortConflictRectifierResult = {
applied: boolean;
/** True when the `effort` field inside `output_config` was stripped (the key itself may still exist if it had other fields). */
removedOutputConfigEffort: boolean;
removedReasoningEffort: boolean;
thinkingType: string | null;
effort: string | null;
};
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
2278
to
+2310
| // 使用 AsyncTaskManager 管理后台处理任务 | ||
| const taskId = `stream-${messageContext?.id || `unknown-${Date.now()}`}`; | ||
| const abortController = new AbortController(); | ||
| const idleTimeoutMs = | ||
| provider.streamingIdleTimeoutMs > 0 ? provider.streamingIdleTimeoutMs : Infinity; | ||
| const clientAbortDrainTimeoutMs = idleTimeoutMs === Infinity ? 60_000 : idleTimeoutMs; | ||
|
|
||
| // ⭐ 提升 idleTimeoutId 到外部作用域，以便客户端断开时能清除 | ||
| let idleTimeoutId: NodeJS.Timeout | null = null; | ||
| let clientAbortDrainTimeoutId: NodeJS.Timeout | null = null; | ||
| const clearClientAbortDrainTimer = () => { | ||
| if (clientAbortDrainTimeoutId) { | ||
| clearTimeout(clientAbortDrainTimeoutId); | ||
| clientAbortDrainTimeoutId = null; | ||
| } | ||
| }; | ||
| const cleanupClientAbortListener = bindClientAbortListener(session.clientAbortSignal, () => { | ||
| logger.debug("ResponseHandler: Client disconnected, cleaning up", { | ||
| taskId, | ||
| providerId: provider.id, | ||
| messageId: messageContext.id, | ||
| }); | ||
|
|
||
| // 客户端断开时清除 idle timeout，避免任务已取消后仍误触发。 | ||
| if (idleTimeoutId) { | ||
| clearTimeout(idleTimeoutId); | ||
| idleTimeoutId = null; | ||
| logger.debug("ResponseHandler: Idle timeout cleared due to client disconnect", { | ||
| // Do not cancel internal accounting on pure client disconnect. If the | ||
| // upstream stream has already completed, the tee'd internal branch can | ||
| // still drain buffered final usage and record the request as successful. | ||
| // Idle/response timeout paths still abort via abortController. | ||
| clearClientAbortDrainTimer(); | ||
| clientAbortDrainTimeoutId = setTimeout(() => { | ||
| logger.info("ResponseHandler: Client abort drain window exceeded", { | ||
| taskId, | ||
| providerId: provider.id, | ||
| messageId: messageContext.id, | ||
| clientAbortDrainTimeoutMs, |
There was a problem hiding this comment.
When provider.streamingIdleTimeoutMs is 0 (disabled), idleTimeoutMs is set to Infinity, and clientAbortDrainTimeoutMs consequently falls back to 60_000 ms. This means a provider configured with no idle timeout (intentionally unlimited streaming duration) will now silently cap the post-disconnect drain window at 60 seconds. A very long or slow upstream response that hasn't completed by the time the client disconnects will be abandoned after 60 s, and the request billed as failed rather than success.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 2278-2310
Comment:
**Drain timeout falls back to 60 s when idle timeout is disabled**
When `provider.streamingIdleTimeoutMs` is 0 (disabled), `idleTimeoutMs` is set to `Infinity`, and `clientAbortDrainTimeoutMs` consequently falls back to `60_000` ms. This means a provider configured with no idle timeout (intentionally unlimited streaming duration) will now silently cap the post-disconnect drain window at 60 seconds. A very long or slow upstream response that hasn't completed by the time the client disconnects will be abandoned after 60 s, and the request billed as failed rather than success.
How can I resolve this? If you propose a fix, please make it concise.| return { data, applied: false }; | ||
| } | ||
|
|
||
| const lines = text.split("\n"); |
There was a problem hiding this comment.
" " leaves a trailing on each line when the upstream emits line endings (valid per the SSE spec). isInertChatCompletionDataLine would then fail the payloadText.startsWith("{") check on "{ ...", so inert chunks would not be filtered. isBlankSseSeparatorLine already handles " " explicitly, which hints at this risk.
Suggested change
| const lines = text.split("\n"); | |
| const lines = text.split(/\r?\n/); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-fixer/index.ts
Line: 598
Comment:
Splitting on `"
"` leaves a trailing `
` on each line when the upstream emits `
` line endings (valid per the SSE spec). `isInertChatCompletionDataLine` would then fail the `payloadText.startsWith("{")` check on `"{
..."`, so inert chunks would not be filtered. `isBlankSseSeparatorLine` already handles `"
"` explicitly, which hints at this risk.
```suggestion
const lines = text.split(/\r?\n/);
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/request-filters.ts (1)
357-381:⚠️ Potential issue | 🟠 Major | ⚡ Quick win校验触发条件仍有漏网路径，能写入“simple + 空 target”的无效配置
Line 357 当前只在绑定字段变更时才走
validatePayload。如果仅把ruleMode从advanced改为simple（且历史记录target=""），更新会绕过 simple 模式的target必填约束并成功落库。建议把validatePayload的触发条件扩展到所有影响有效配置的字段，并统一使用“更新后 effective 值”组装 payload。建议修复（示例）
- if ( - updates.bindingType !== undefined || - updates.providerIds !== undefined || - updates.groupTags !== undefined - ) { + if ( + updates.ruleMode !== undefined || + updates.operations !== undefined || + updates.target !== undefined || + updates.action !== undefined || + updates.matchType !== undefined || + updates.bindingType !== undefined || + updates.providerIds !== undefined || + updates.groupTags !== undefined + ) { const effectiveBindingType = updates.bindingType ?? existing!.bindingType; const effectiveProviderIds = updates.providerIds !== undefined ? updates.providerIds : existing!.providerIds; const effectiveGroupTags = updates.groupTags !== undefined ? updates.groupTags : existing!.groupTags; const effectiveRuleMode = updates.ruleMode ?? existing!.ruleMode; const effectiveOperations = updates.operations !== undefined ? updates.operations : existing!.operations; + const effectiveTarget = updates.target ?? existing!.target; + const effectiveAction = updates.action ?? existing!.action; + const effectiveMatchType = updates.matchType ?? existing!.matchType; const validationError = validatePayload({ name: existing!.name, scope: existing!.scope, - action: existing!.action, - target: existing!.target, + action: effectiveAction, + target: effectiveTarget, + matchType: effectiveMatchType ?? undefined, bindingType: effectiveBindingType, providerIds: effectiveProviderIds, groupTags: effectiveGroupTags, ruleMode: effectiveRuleMode, operations: effectiveOperations, });🤖 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 `@src/actions/request-filters.ts` around lines 357 - 381, The current conditional only runs validatePayload when binding-related fields change, letting a change like ruleMode from "advanced" to "simple" bypass target-required checks; update the if condition to include all fields that affect validity (at least ruleMode, target, name, scope, action, operations, providerIds, groupTags, bindingType) so any of those in updates triggers validation, and keep using the assembled effective* values (effectiveBindingType, effectiveProviderIds, effectiveGroupTags, effectiveRuleMode, effectiveOperations, plus effectiveTarget/effectiveName/effectiveScope/effectiveAction derived from existing/updates) when calling validatePayload with existing!.name/scope/action/target replaced by those effective values.
🤖 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 `@messages/zh-TW/myUsage.json`:
- Line 5: The translation for the JSON key "documentation" is semantically off;
update the value for "documentation" in messages/zh-TW/myUsage.json from "使用文件"
to a clearer phrase such as "使用說明" (or "使用文件說明") so it reads as a
documentation/usage entry rather than a file; locate the "documentation" key in
that file and replace the value accordingly.
In `@src/app/`[locale]/dashboard/_components/dashboard-header.test.tsx:
- Line 1: The test file dashboard-header.test.tsx is placed under src/... which
violates the repo test-location conventions; move or rename it so Vitest picks
it up correctly: either relocate the file into tests/unit/ (or
tests/integration/ as appropriate) or convert it to a source-adjacent test by
renaming/moving it to match src/**/*.test.ts (keeping the same test content and
imports for the DashboardHeader component), and update any import paths if they
change.
In `@src/app/`[locale]/settings/providers/_components/add-provider-dialog.tsx:
- Line 10: Replace the relative import for the local component with the project
path-alias: change the import of ProviderFormDialogContent (currently
"./provider-form-dialog-content") to use
"`@/app/`[locale]/settings/providers/_components/provider-form-dialog-content" so
it follows the codebase rule of using `@/` to map to ./src/, ensuring consistency
across imports and preventing future path-drift during refactors.
In `@src/lib/api-client/v1/actions/keys.ts`:
- Around line 17-29: The addKey function currently falls back to creating a key
for the session user on any admin-forbidden error, which can silently ignore the
caller's userId; change addKey to accept an explicit opt-in flag (e.g.,
allowSelfFallback: boolean, default false) and only attempt the
apiPost("/api/v1/users:self/keys", ...) retry when allowSelfFallback is true and
isAdminForbidden(error) is true; update the call sites that intend self-service
to pass allowSelfFallback=true and keep all other usages unchanged; reference
addKey, isAdminForbidden, and the two apiPost routes when making the change.
---
Outside diff comments:
In `@src/actions/request-filters.ts`:
- Around line 357-381: The current conditional only runs validatePayload when
binding-related fields change, letting a change like ruleMode from "advanced" to
"simple" bypass target-required checks; update the if condition to include all
fields that affect validity (at least ruleMode, target, name, scope, action,
operations, providerIds, groupTags, bindingType) so any of those in updates
triggers validation, and keep using the assembled effective* values
(effectiveBindingType, effectiveProviderIds, effectiveGroupTags,
effectiveRuleMode, effectiveOperations, plus
effectiveTarget/effectiveName/effectiveScope/effectiveAction derived from
existing/updates) when calling validatePayload with
existing!.name/scope/action/target replaced by those effective values.
🪄 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: 1876670c-428a-4e4a-968f-e99cb4b956ae
📒 Files selected for processing (84)
drizzle/0105_chief_rocket_racer.sqldrizzle/meta/0105_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/dashboard.jsonmessages/en/errors.jsonmessages/en/myUsage.jsonmessages/en/settings/config.jsonmessages/ja/dashboard.jsonmessages/ja/errors.jsonmessages/ja/myUsage.jsonmessages/ja/settings/config.jsonmessages/ru/dashboard.jsonmessages/ru/errors.jsonmessages/ru/myUsage.jsonmessages/ru/settings/config.jsonmessages/zh-CN/dashboard.jsonmessages/zh-CN/errors.jsonmessages/zh-CN/myUsage.jsonmessages/zh-CN/settings/config.jsonmessages/zh-TW/dashboard.jsonmessages/zh-TW/errors.jsonmessages/zh-TW/myUsage.jsonmessages/zh-TW/settings/config.jsonpackage.jsonsrc/actions/keys.tssrc/actions/request-filters.tssrc/actions/system-config.tssrc/app/[locale]/dashboard/_components/dashboard-header.test.tsxsrc/app/[locale]/dashboard/_components/dashboard-header.tsxsrc/app/[locale]/dashboard/_components/user-menu.test.tsxsrc/app/[locale]/dashboard/_components/user-menu.tsxsrc/app/[locale]/dashboard/_components/user/forms/delete-key-confirm.tsxsrc/app/[locale]/dashboard/_components/user/user-key-table-row.tsxsrc/app/[locale]/my-usage/_components/my-usage-header.test.tsxsrc/app/[locale]/my-usage/_components/my-usage-header.tsxsrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/[locale]/settings/config/page.tsxsrc/app/[locale]/settings/providers/_components/add-provider-dialog.tsxsrc/app/[locale]/settings/providers/_components/provider-form-dialog-content.tsxsrc/app/[locale]/settings/providers/_components/provider-manager.tsxsrc/app/[locale]/settings/providers/_components/provider-rich-list-item.tsxsrc/app/[locale]/settings/providers/_components/vendor-keys-compact-list.tsxsrc/app/[locale]/usage-doc/layout.tsxsrc/app/api/v1/resources/keys/handlers.tssrc/app/api/v1/resources/keys/router.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/response-fixer/index.tssrc/app/v1/_lib/proxy/response-fixer/response-fixer.test.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/thinking-effort-conflict-rectifier.test.tssrc/app/v1/_lib/proxy/thinking-effort-conflict-rectifier.tssrc/drizzle/schema.tssrc/lib/api-client/v1/actions/_compat.tssrc/lib/api-client/v1/actions/keys.tssrc/lib/api-client/v1/actions/users.tssrc/lib/api-client/v1/errors.tssrc/lib/api-client/v1/openapi-types.gen.tssrc/lib/api/v1/schemas/system-config.tssrc/lib/config/system-settings-cache.tssrc/lib/utils/dialog.tssrc/lib/utils/error-messages.tssrc/lib/utils/special-settings.tssrc/lib/validation/schemas.tssrc/repository/_shared/transformers.tssrc/repository/system-config.tssrc/types/special-settings.tssrc/types/system-config.tstests/api/v1/keys/keys.self.test.tstests/configs/thinking-effort-conflict-rectifier.config.tstests/unit/actions/keys-remove-error-codes.test.tstests/unit/actions/request-filters-cache-reload.test.tstests/unit/actions/system-config-thinking-effort-conflict-setting.test.tstests/unit/api/v1/api-client-actions.test.tstests/unit/api/v1/keys-delete-error-mapping.test.tstests/unit/delete-key-error-toast-ui.test.tsxtests/unit/dialog-prevent-close-on-outside-interaction.test.tstests/unit/frontend/api-error-i18n.test.tstests/unit/i18n/key-created-copy.test.tstests/unit/i18n/my-usage-keys.test.tstests/unit/proxy/proxy-forwarder-thinking-effort-conflict-rectifier.test.tstests/unit/proxy/response-handler-client-abort-drain.test.tstests/unit/proxy/response-handler-hedge-loser-priority.test.tstests/unit/repository/system-config-update-missing-columns.test.tstests/unit/usage-doc/usage-doc-auth-state.test.tsx
👮 Files not reviewed due to content moderation or server errors (5)
- src/app/v1/_lib/proxy/response-fixer/index.ts
- src/app/v1/_lib/proxy/response-fixer/response-fixer.test.ts
- src/app/v1/_lib/proxy/response-handler.ts
- tests/unit/proxy/response-handler-client-abort-drain.test.ts
- tests/unit/proxy/response-handler-hedge-loser-priority.test.ts
| @@ -0,0 +1,111 @@ | |||
| import type { ReactNode } from "react"; | |||
There was a problem hiding this comment.
测试文件路径/命名不符合仓库约定，建议迁移到 tests/unit/ 或调整为 src/**/*.test.ts。
Line 1 新增文件位于 src/.../dashboard-header.test.tsx，与当前约定不一致，可能导致测试组织和规则检查出现偏差。
As per coding guidelines: {tests/**/*.test.{ts,tsx},src/**/*.test.ts} 要求 Vitest 测试放在 tests/unit/、tests/integration/，或 source-adjacent 的 src/**/*.test.ts。
🤖 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 `@src/app/`[locale]/dashboard/_components/dashboard-header.test.tsx at line 1,
The test file dashboard-header.test.tsx is placed under src/... which violates
the repo test-location conventions; move or rename it so Vitest picks it up
correctly: either relocate the file into tests/unit/ (or tests/integration/ as
appropriate) or convert it to a source-adjacent test by renaming/moving it to
match src/**/*.test.ts (keeping the same test content and imports for the
DashboardHeader component), and update any import paths if they change.
Source: Coding guidelines
| import { Dialog, DialogContent, DialogTitle, DialogTrigger } from "@/components/ui/dialog"; | ||
| import { Dialog, DialogTitle, DialogTrigger } from "@/components/ui/dialog"; | ||
| import { ProviderForm } from "./forms/provider-form"; | ||
| import { ProviderFormDialogContent } from "./provider-form-dialog-content"; |
There was a problem hiding this comment.
跨文件导入规范不一致：新增本地组件导入都使用了相对路径。
四处新增导入都使用了 ./provider-form-dialog-content，建议统一替换为 @/app/[locale]/settings/providers/_components/provider-form-dialog-content，避免后续路径迁移和重构时出现风格漂移。
As per coding guidelines, **/*.{ts,tsx,js,jsx}: Use path alias @/ to map to ./src/ for imports.
🤖 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 `@src/app/`[locale]/settings/providers/_components/add-provider-dialog.tsx at
line 10, Replace the relative import for the local component with the project
path-alias: change the import of ProviderFormDialogContent (currently
"./provider-form-dialog-content") to use
"`@/app/`[locale]/settings/providers/_components/provider-form-dialog-content" so
it follows the codebase rule of using `@/` to map to ./src/, ensuring consistency
across imports and preventing future path-drift during refactors.
Source: Coding guidelines
… success Prior behaviour reclassified truncated streams as 200 success if they carried positive usage tokens, but providers like Anthropic emit usage in the first event. Now checks for a format-appropriate completion marker before billing.
…ilures Surfaces specific error codes and parameters when self-service key creation fails due to duplicate names or limit violations, replacing generic failures.
…ates Resolves a regression where converting an advanced filter to simple mode with a new target was rejected because validation checked the stale empty target.
Read-only sessions can view docs but not the dashboard, so the back link dead-ended at login. Gates the dashboard access predicate on canLoginWebUi.
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/actions/request-filters.ts (1)
374-374: 💤 Low value建议统一有效值计算的代码风格。
第374行使用
??运算符计算effectiveName，而第375行对effectiveTarget使用!== undefined检查。两者在功能上等价（因为name和target都是非空字段，Partial<{name: string}>只会引入undefined而不会引入null），但风格不一致。为了与第375行以及同一代码块中第368-369行（
effectiveOperations）保持一致，建议将第374行也改为显式的!== undefined检查。♻️ 建议的风格统一
- const effectiveName = updates.name ?? existing!.name; + const effectiveName = updates.name !== undefined ? updates.name : existing!.name; const effectiveTarget = updates.target !== undefined ? updates.target : existing!.target;🤖 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 `@src/actions/request-filters.ts` at line 374, Change the effectiveName assignment to use an explicit !== undefined check like the other computed values: when computing effectiveName (currently using updates.name ?? existing!.name) replace the nullish-coalescing with a check for updates.name !== undefined and fall back to existing!.name so it matches the style used for effectiveTarget and effectiveOperations in this block.src/actions/keys.ts (1)
213-323: ⚡ Quick win统一 errorCode 的使用方式。
Line 194 使用了
ERROR_CODES.DUPLICATE_NAME常量，但 Lines 213, 234, 255, 276, 297, 318 直接使用字符串字面量（如"KEY_LIMIT_5H_EXCEEDS_USER_LIMIT"）。为保持代码一致性与可维护性，建议将所有 errorCode 统一为 ERROR_CODES 常量引用，或在@/lib/utils/error-messages中补充缺失的常量定义。♻️ 建议统一使用 ERROR_CODES 常量
在
@/lib/utils/error-messages中补充定义（如果尚未存在）：export const ERROR_CODES = { UNAUTHORIZED: "UNAUTHORIZED", PERMISSION_DENIED: "PERMISSION_DENIED", DUPLICATE_NAME: "DUPLICATE_NAME", + KEY_LIMIT_5H_EXCEEDS_USER_LIMIT: "KEY_LIMIT_5H_EXCEEDS_USER_LIMIT", + KEY_LIMIT_DAILY_EXCEEDS_USER_LIMIT: "KEY_LIMIT_DAILY_EXCEEDS_USER_LIMIT", + KEY_LIMIT_WEEKLY_EXCEEDS_USER_LIMIT: "KEY_LIMIT_WEEKLY_EXCEEDS_USER_LIMIT", + KEY_LIMIT_MONTHLY_EXCEEDS_USER_LIMIT: "KEY_LIMIT_MONTHLY_EXCEEDS_USER_LIMIT", + KEY_LIMIT_TOTAL_EXCEEDS_USER_LIMIT: "KEY_LIMIT_TOTAL_EXCEEDS_USER_LIMIT", + KEY_LIMIT_CONCURRENT_EXCEEDS_USER_LIMIT: "KEY_LIMIT_CONCURRENT_EXCEEDS_USER_LIMIT", // ... } as const;然后在本文件中统一使用常量：
- errorCode: "KEY_LIMIT_5H_EXCEEDS_USER_LIMIT", + errorCode: ERROR_CODES.KEY_LIMIT_5H_EXCEEDS_USER_LIMIT,对其余 Lines 234, 255, 276, 297, 318 进行相同替换。
🤖 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 `@src/actions/keys.ts` around lines 213 - 323, The errorCode strings in the validation branch returns (e.g., "KEY_LIMIT_5H_EXCEEDS_USER_LIMIT", "KEY_LIMIT_DAILY_EXCEEDS_USER_LIMIT", etc.) should be replaced with the corresponding ERROR_CODES constants to match the earlier usage of ERROR_CODES.DUPLICATE_NAME; update each return block that sets errorCode and errorParams to reference the constant from ERROR_CODES (ensure the constants exist in the error-messages module and add them there if missing), keeping the tError calls unchanged (symbols to locate: ERROR_CODES, tError, validatedData.limit5hUsd/limitDailyUsd/limitWeeklyUsd/limitMonthlyUsd/limitTotalUsd/limitConcurrentSessions and user.limit5hUsd/user.dailyQuota/user.limitWeeklyUsd/user.limitMonthlyUsd/user.limitTotalUsd/user.limitConcurrentSessions).
🤖 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.
Nitpick comments:
In `@src/actions/keys.ts`:
- Around line 213-323: The errorCode strings in the validation branch returns
(e.g., "KEY_LIMIT_5H_EXCEEDS_USER_LIMIT", "KEY_LIMIT_DAILY_EXCEEDS_USER_LIMIT",
etc.) should be replaced with the corresponding ERROR_CODES constants to match
the earlier usage of ERROR_CODES.DUPLICATE_NAME; update each return block that
sets errorCode and errorParams to reference the constant from ERROR_CODES
(ensure the constants exist in the error-messages module and add them there if
missing), keeping the tError calls unchanged (symbols to locate: ERROR_CODES,
tError,
validatedData.limit5hUsd/limitDailyUsd/limitWeeklyUsd/limitMonthlyUsd/limitTotalUsd/limitConcurrentSessions
and
user.limit5hUsd/user.dailyQuota/user.limitWeeklyUsd/user.limitMonthlyUsd/user.limitTotalUsd/user.limitConcurrentSessions).
In `@src/actions/request-filters.ts`:
- Line 374: Change the effectiveName assignment to use an explicit !== undefined
check like the other computed values: when computing effectiveName (currently
using updates.name ?? existing!.name) replace the nullish-coalescing with a
check for updates.name !== undefined and fall back to existing!.name so it
matches the style used for effectiveTarget and effectiveOperations in this
block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb11be88-959a-4b20-90b8-ff523a80a0a3
📒 Files selected for processing (8)
src/actions/keys.tssrc/actions/request-filters.tssrc/app/[locale]/usage-doc/layout.tsxsrc/app/v1/_lib/proxy/response-handler.tstests/unit/actions/add-key-error-codes.test.tstests/unit/actions/request-filters-cache-reload.test.tstests/unit/proxy/response-handler-client-abort-drain.test.tstests/unit/usage-doc/usage-doc-auth-state.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/usage-doc/usage-doc-auth-state.test.tsx
- src/app/v1/_lib/proxy/response-handler.ts
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/dashboard/add-key-form-expiry-clear-ui.test.tsx (1)
114-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win这里把回归条件放宽过头了，测试抓不住原问题。
这条用例的目标是确保“清除到期时间时显式传空字符串”，但断言同时接受
undefined。如果后面又退回成undefined被序列化丢弃，这个测试仍然会绿，等于把这次修复的核心回归点放掉了。建议修改
expect(Object.hasOwn(payload, "expiresAt")).toBe(true); - // 空字符串或 undefined 都是有效的清除值，但根据修复，应该是空字符串 - expect(payload.expiresAt === "" || payload.expiresAt === undefined).toBe(true); + expect(payload.expiresAt).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 `@tests/unit/dashboard/add-key-form-expiry-clear-ui.test.tsx` around lines 114 - 118, The test currently allows expiresAt to be either "" or undefined which weakens the regression check; update the assertion on the payload to require an explicit empty string for payload.expiresAt (not undefined) so the test enforces the "clear by sending empty string" behavior — keep the existing Object.hasOwn(payload, "expiresAt") check and replace the loose truthy check with an assertion that payload.expiresAt equals the empty string ("") to catch regressions where the field would be omitted/serialized away.src/app/[locale]/dashboard/_components/user/forms/add-key-form.tsx (1)
73-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick win自助建 Key 路径仍被
userId硬性拦住了。非管理员现在走的是会话定向的
addOwnKey(body)，但这里仍然在提交前throw userIdMissing，并且把提交按钮绑在!!userId上。这样一来，只要调用方没有传userId，自助建 Key 就会被前端直接拦死，和这次新增的 self endpoint 契约相冲突。建议修改
onSubmit: async (data) => { - if (!userId) { + if (isAdmin && !userId) { throw new Error(t("errors.userIdMissing")); } @@ - const result = isAdmin ? await addKey({ userId: userId!, ...body }) : await addOwnKey(body); + const result = isAdmin ? await addKey({ userId: userId!, ...body }) : await addOwnKey(body); @@ - canSubmit={form.canSubmit && !!userId} + canSubmit={form.canSubmit && (!isAdmin || !!userId)}Also applies to: 98-98, 153-153
🤖 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 `@src/app/`[locale]/dashboard/_components/user/forms/add-key-form.tsx around lines 73 - 76, The onSubmit handler in add-key-form.tsx currently throws when userId is missing and the submit button is gated by !!userId; change this to support the new self endpoint by removing the throw and switching the API call based on presence of userId: inside onSubmit decide between calling addOwnKey(body) when userId is falsy and the existing addKey/addUserKey call when userId is present, and update the submit button so it is not disabled solely by !!userId (keep other validations intact).
🧹 Nitpick comments (1)
tests/unit/api/v1/keys-write-handlers-authz.test.ts (1)
68-131: 💤 Low value建议补充
createSelfKeyhandler 的鉴权测试当前测试覆盖了
deleteKey、updateKey、enableKey、renewKey的会话守卫，但未覆盖同样有内联鉴权逻辑的createSelfKey。建议补充以下场景：
- 只读会话（
canLoginWebUi=false）返回 403- 无 session 返回 401
- Web-UI 会话正常创建
🤖 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 `@tests/unit/api/v1/keys-write-handlers-authz.test.ts` around lines 68 - 131, Add unit tests for the createSelfKey handler mirroring existing patterns: import createSelfKey from "`@/app/api/v1/resources/keys/handlers`" and call it with makeContext(readOnlyAuth, <body>) asserting a 403 and that the create action mock (e.g., createSelfKeyMock or the existing key-creation mock used elsewhere) was not called; call createSelfKey with makeContext(null, <body>) asserting a 401 and no action call; call createSelfKey with makeContext(webAuth, <body>) asserting a successful creation response and that the create action mock was invoked with the expected args. Ensure you reuse makeContext, readOnlyAuth, webAuth and the same mock names/patterns used in this file so the new tests follow the existing style.
🤖 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 `@tests/unit/actions/keys-self-service-authz.test.ts`:
- Around line 154-157: The test currently asserts the error string
(expect(result.error) === "CANNOT_DISABLE_LAST_KEY") which doesn't verify the
structured error contract; change the assertion to check the structured error
code instead (expect(result.errorCode).toBe("CANNOT_DISABLE_LAST_KEY")) while
keeping the existing expect(result.ok).toBe(false) check so the test ensures
both failure and the correct errorCode from the operation (refer to the result
object used in this test).
---
Outside diff comments:
In `@src/app/`[locale]/dashboard/_components/user/forms/add-key-form.tsx:
- Around line 73-76: The onSubmit handler in add-key-form.tsx currently throws
when userId is missing and the submit button is gated by !!userId; change this
to support the new self endpoint by removing the throw and switching the API
call based on presence of userId: inside onSubmit decide between calling
addOwnKey(body) when userId is falsy and the existing addKey/addUserKey call
when userId is present, and update the submit button so it is not disabled
solely by !!userId (keep other validations intact).
In `@tests/unit/dashboard/add-key-form-expiry-clear-ui.test.tsx`:
- Around line 114-118: The test currently allows expiresAt to be either "" or
undefined which weakens the regression check; update the assertion on the
payload to require an explicit empty string for payload.expiresAt (not
undefined) so the test enforces the "clear by sending empty string" behavior —
keep the existing Object.hasOwn(payload, "expiresAt") check and replace the
loose truthy check with an assertion that payload.expiresAt equals the empty
string ("") to catch regressions where the field would be omitted/serialized
away.
---
Nitpick comments:
In `@tests/unit/api/v1/keys-write-handlers-authz.test.ts`:
- Around line 68-131: Add unit tests for the createSelfKey handler mirroring
existing patterns: import createSelfKey from
"`@/app/api/v1/resources/keys/handlers`" and call it with
makeContext(readOnlyAuth, <body>) asserting a 403 and that the create action
mock (e.g., createSelfKeyMock or the existing key-creation mock used elsewhere)
was not called; call createSelfKey with makeContext(null, <body>) asserting a
401 and no action call; call createSelfKey with makeContext(webAuth, <body>)
asserting a successful creation response and that the create action mock was
invoked with the expected args. Ensure you reuse makeContext, readOnlyAuth,
webAuth and the same mock names/patterns used in this file so the new tests
follow the existing style.
🪄 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: 07f4822d-c64a-4f67-9f43-0b355612be3d
📒 Files selected for processing (27)
messages/zh-TW/myUsage.jsonsrc/actions/keys.tssrc/app/[locale]/dashboard/_components/user/forms/add-key-form.tsxsrc/app/api/v1/resources/keys/handlers.tssrc/app/api/v1/resources/keys/router.tssrc/app/api/v1/resources/me/handlers.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/response-fixer/index.tssrc/app/v1/_lib/proxy/response-fixer/response-fixer.test.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/thinking-effort-conflict-rectifier.test.tssrc/app/v1/_lib/proxy/thinking-effort-conflict-rectifier.tssrc/lib/api-client/v1/actions/keys.tssrc/lib/api-client/v1/openapi-types.gen.tssrc/lib/utils/error-messages.tssrc/lib/utils/special-settings.tssrc/repository/system-config.tssrc/types/special-settings.tstests/unit/actions/keys-remove-error-codes.test.tstests/unit/actions/keys-self-service-authz.test.tstests/unit/api/v1/api-client-actions.test.tstests/unit/api/v1/keys-delete-error-mapping.test.tstests/unit/api/v1/keys-write-handlers-authz.test.tstests/unit/dashboard/add-key-form-balance-page-toggle.test.tsxtests/unit/dashboard/add-key-form-expiry-clear-ui.test.tsxtests/unit/dashboard/add-key-form-self-service.test.tsxtests/unit/repository/system-config-degradation-ladder.test.ts
✅ Files skipped from review due to trivial changes (2)
- messages/zh-TW/myUsage.json
- src/lib/api-client/v1/openapi-types.gen.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/lib/utils/special-settings.ts
- src/types/special-settings.ts
- src/lib/utils/error-messages.ts
- tests/unit/api/v1/keys-delete-error-mapping.test.ts
- src/app/v1/_lib/proxy/thinking-effort-conflict-rectifier.ts
- tests/unit/actions/keys-remove-error-codes.test.ts
- src/app/v1/_lib/proxy/thinking-effort-conflict-rectifier.test.ts
- src/app/v1/_lib/proxy/forwarder.ts
- src/app/v1/_lib/proxy/response-fixer/index.ts
Comment on lines
+154
to
+157
| expect(result.ok).toBe(false); | ||
| if (!result.ok) { | ||
| expect(result.error).toBe("CANNOT_DISABLE_LAST_KEY"); | ||
| } |
There was a problem hiding this comment.
请改为断言 errorCode，不要只看 error 字符串。
这条用例覆盖的是这次 PR 的“结构化错误码”契约，但现在断言的是 result.error === "CANNOT_DISABLE_LAST_KEY"。如果后续实现退化成只返回同名文本而丢失 errorCode，这个测试仍然会通过，前端按错误码翻译的链路却已经坏了。
建议修改
expect(result.ok).toBe(false);
if (!result.ok) {
- expect(result.error).toBe("CANNOT_DISABLE_LAST_KEY");
+ expect(result.errorCode).toBe("CANNOT_DISABLE_LAST_KEY");
}📝 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
| expect(result.ok).toBe(false); | |
| if (!result.ok) { | |
| expect(result.error).toBe("CANNOT_DISABLE_LAST_KEY"); | |
| } | |
| expect(result.ok).toBe(false); | |
| if (!result.ok) { | |
| expect(result.errorCode).toBe("CANNOT_DISABLE_LAST_KEY"); | |
| } |
🤖 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 `@tests/unit/actions/keys-self-service-authz.test.ts` around lines 154 - 157,
The test currently asserts the error string (expect(result.error) ===
"CANNOT_DISABLE_LAST_KEY") which doesn't verify the structured error contract;
change the assertion to check the structured error code instead
(expect(result.errorCode).toBe("CANNOT_DISABLE_LAST_KEY")) while keeping the
existing expect(result.ok).toBe(false) check so the test ensures both failure
and the correct errorCode from the operation (refer to the result object used in
this test).
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
3 participants
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
Release v0.8.6 — bundles 7 merged PRs since v0.8.5 plus a post-review hardening round (8 direct commits on
dev). Highlights: a new thinking-effort conflict rectifier for DeepSeek/MiMo compatibility, full self-service key management for non-admin users (create / edit / delete / toggle / renew), a terminal-marker billing gate for client-aborted streams, key error transparency fixes, provider dialog UX improvements, hedge-loser Codex priority billing fix, request-filter advanced mode regression fix, and three behavior-preserving maintainability refactors.Included Changes
Features
thinking: {type: "disabled"}for sub-agent tasks without strippingoutput_config.effort, which causes 400 errors on DeepSeek/MiMo's strict Anthropic-compatible endpoints. Rectifies by stripping conflicting fields and retrying once on the same provider. Controlled by system settingenableThinkingEffortConflictRectifier(default: enabled). Includes DB migration0105_chief_rocket_racer.sql. The audit field is namedremovedOutputConfigEffort(renamed fromremovedOutputConfig, which misread as "whole block removed" in the partial-strip case).POST /api/v1/users:self/keysat read auth tier so non-admin users withcanLoginWebUikeys can create keys for themselves. The hardening round extends this to full lifecycle management:PATCH/DELETE /keys/{id},:enable,:renew, andGET /keys/{id}/limit-usageare relaxed from admin to read tier behind a three-layer guard (route tier →requireKeyWriteSessionhandler guard →denyKeyWriteForReadOnlySessionaction guard). Web-UI sessions manage only the keys they own; read-only sessions (canLoginWebUi=false) are rejected on every write path, closing the create-then-editcanLoginWebUiself-escalation;PATCH isEnabled=falsehonors the last-enabled-key protection for non-admins. On the client,addOwnKey()posts straight to the session-scoped endpoint andaddKey()no longer silently retargets to the session user on 403.Bug Fixes
499 CLIENT_ABORTEDwhen the client disconnects: internal accounting drains briefly after disconnect to capture the real 2xx status and usage. The hardening round closed the inverse hole: billing-as-success now also requires a format-appropriate terminal completion marker (message_stop/response.completed/[DONE]/finish_reason) — Anthropic emits usage in the firstmessage_startevent, so positive tokens alone would have billed truncated aborts as 200.removeKeyreturns propererrorCodefor all 6 failure paths (new codesCANNOT_DELETE_LAST_KEY,CANNOT_DELETE_LAST_GROUP_KEY), and the delete UI shows i18n-translated messages. The hardening round extends machine-readable codes to key creation (DUPLICATE_NAME+ 6KEY_LIMIT_*codes, so the self-service route surfaces specific errors instead of a generic toast) and introducesKEY_NOT_FOUNDacross all 6 key not-found paths. Also fixes key creation dialog copy that incorrectly claimed "key shown only once".ruleMode/operations/name/target, which also unblocks advanced→simple conversion.ProviderFormDialogContentapplied to all 6 provider form dialogs.priorityservice tier but the winner session sync overwrote it; the billing snapshot is captured before sync.canLoginWebUipredicate as the dashboard header.output_configsibling fields were lost during certain fallback chains.myUsage.documentationcorrected to 使用說明.Maintainability (behavior-preserving)
forwarder.ts's three copy-pasted rectifier branches folded into a typed descriptor table with one generic executor; detection precedence, audit payload shapes, hit-then-terminate and retry-once semantics are pinned by the existing suites.system-config.ts's hand-written 11-rung read / 10-rung update fallback chains are now generated from a single column table (net −306 lines); adding a futuresystem_settingscolumn touches exactly one entry. A 470-line lock-test pins the exact attempt sequences of the old implementation.response-fixer's duplicated transform/flush blocks deduplicated intoapplyStreamFixerswith the inertchat.completion.chunkfilter as an integral step plus a byte-level marker pre-scan (non-matching chunks are returned by reference, no decode); billing parameters consolidated into a singleBillingComputeInputsbag acrossupdateRequestCostFromUsage/trackCostToRedis(hedge-loser snapshot semantics unchanged); the client-abort drain path no longer parses the same stream text twice.Other
AddKeyFormtest files whose mocks pointed at a stale module path (and never intercepted anything) were repaired.Review Hardening
Two independent multi-agent deep reviews (25 finders + adversarial verification) were run against this PR; every confirmed finding was fixed in place with TDD and the full local matrix re-verified (build / typecheck / lint / full unit suite /
test:v1/ OpenAPI check+lint / migrations / i18n audit — all green, coverage failures at baseline parity):Database Migration
0105_chief_rocket_racer.sql: Addsenable_thinking_effort_conflict_rectifiercolumn (boolean, defaulttrue) tosystem_settingstable.Included PRs
Plus 8 direct commits on
devfrom the review hardening rounds: 04834e9, 7b6ae61, 7a03462, c4e1ff4, 1addcc7, 632e590, ea1a3b2, 36b61fe.Description enhanced by Claude AI
Greptile Summary
This release bundles 7 merged PRs since v0.8.5, covering proxy reliability, key management access control, and UI fixes. The changes are well-structured and backed by 19+ new test files.
ResponseFixerfilter for inert chat-completion chunks, and fixes hedge-loser Codex priority billing by snapshottingrequestedServiceTierbefore winner sync.POST /api/v1/users:self/keysself-service endpoint, relaxes several key routes from admin to read-tier auth with consistentrequireKeyWriteSession/denyKeyWriteForReadOnlySessionguards, and surfaces structured error codes for all key deletion failure paths.Confidence Score: 4/5
Safe to merge with one targeted fix: the client-abort completion detector needs anchored matching to avoid false positives on model text output.
The bulk of the changes — the rectifier registry, key access-control guards, billing snapshot, request-filter fix, and dialog UX — are clean and well-tested. The concern is in hasStreamCompletionMarker: the three plain includes() calls search allContent (the full raw SSE text) for "message_stop", "[DONE]", and "response.completed". Because allContent contains SSE data payloads verbatim, model text output that contains one of those literal strings would satisfy the check on a genuinely truncated stream, reclassifying it as a successful completion and keeping session binding.
src/app/v1/_lib/proxy/response-handler.ts — specifically the hasStreamCompletionMarker function and its three plain-string includes() checks.
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as Client participant PH as ProxyResponseHandler participant US as Upstream participant FIN as finalizeDeferredStreaming C->>PH: Request PH->>US: Forward US-->>PH: Stream chunks (tee'd) C->>PH: Client disconnects Note over PH: Start drain timer (clientAbortDrainTimeoutMs) Note over PH: Do NOT cancel internal read loop US-->>PH: Continue streaming chunks alt Stream completes before drain timeout US-->>PH: "done=true → streamEndedNormally=true" PH->>FIN: "finalize(streamEndedNormally=true)" FIN-->>PH: Normal 2xx success path else Drain timeout fires PH->>FIN: "finalize(streamEndedNormally=false, clientAborted=true)" FIN->>FIN: hasStreamCompletionMarker(allContent)? alt Marker found + positive tokens FIN-->>PH: "clientAbortCompleteSuccess=true → bill as success" else No marker FIN-->>PH: 499 CLIENT_ABORTED (unbilled) end endReviews (3): Last reviewed commit: "feat(keys): enable self-service key mana..." | Re-trigger Greptile