fix(keys): preserve last-enabled-key error code - #1393
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthrough密钥禁用相关流程将通用失败码替换为 Changes密钥禁用错误处理
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| throw new BatchUpdateError( | ||
| tError("CANNOT_DISABLE_LAST_KEY"), | ||
| ERROR_CODES.OPERATION_FAILED | ||
| ERROR_CODES.CANNOT_DISABLE_LAST_KEY |
There was a problem hiding this comment.
The two changed batch-disable guards now return CANNOT_DISABLE_LAST_KEY, but the added regressions exercise only toggleKeyEnabled. Add coverage for both batch guards so a future change cannot collapse their codes back to OPERATION_FAILED while the suite continues to pass.
Knowledge Base Used: Management API (/api/v1)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/keys.ts
Line: 1421
Comment:
**Cover batch error-code branches**
The two changed batch-disable guards now return `CANNOT_DISABLE_LAST_KEY`, but the added regressions exercise only `toggleKeyEnabled`. Add coverage for both batch guards so a future change cannot collapse their codes back to `OPERATION_FAILED` while the suite continues to pass.
**Knowledge Base Used:** [Management API (/api/v1)](https://app.greptile.com/ygxz/-/custom-context/knowledge-base/ding113/claude-code-hub/-/docs/management-api.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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: 6058231a1d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ok: false, | ||
| error: tError("CANNOT_DISABLE_LAST_KEY"), | ||
| errorCode: ERROR_CODES.OPERATION_FAILED, | ||
| errorCode: ERROR_CODES.CANNOT_DISABLE_LAST_KEY, |
There was a problem hiding this comment.
When a dashboard user tries to disable their last enabled key, the REST handler replaces the translated action message with the generic publicActionErrorDetail(400), while key-row-item.tsx:277 displays res.error instead of translating res.errorCode. Consequently, preserving this code still produces a generic “Bad Request” toast rather than the intended five-language message. The batch path has the same issue in batch-edit-dialog.tsx:363-372, which also ignores the newly preserved code.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/keys.ts (1)
1260-1266: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win仅在目标 Key 当前启用时返回该错误码。
当
enabled === false时，分支没有检查key.isEnabled。对已禁用 Key 重复发送禁用请求时，也可能返回CANNOT_DISABLE_LAST_KEY，即使请求不会减少启用 Key 数量。请与editKey的检查保持一致。建议修复
- if (!enabled) { + if (!enabled && key.isEnabled) {🤖 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 1260 - 1266, 在处理禁用请求的分支中，更新 keys 操作（围绕 enabled 与 countActiveKeysByUser 的逻辑），仅当目标 key.isEnabled 当前为 true 时才执行最后一个启用 Key 的校验并返回 CANNOT_DISABLE_LAST_KEY；已禁用 Key 的重复禁用请求应跳过该错误码检查，并与 editKey 的行为保持一致。
🧹 Nitpick comments (1)
tests/unit/actions/keys-self-service-authz.test.ts (1)
196-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win补充 PATCH 和批量禁用路径的回归测试。
当前新增用例只覆盖
toggleKeyEnabled。本 PR 同时修改了editKey的 PATCH 保护，以及batchUpdateKeys的事务前置和后置保护。请为这两类路径增加测试，至少断言CANNOT_DISABLE_LAST_KEY，并确认失败后没有持久化更新。依据 PR 目标和当前提供的测试片段。
🤖 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 196 - 209, 在现有最后一个启用密钥保护测试附近，为 editKey 的 PATCH 禁用路径和 batchUpdateKeys 的批量禁用路径补充回归测试：模拟当前仅剩一个启用密钥，断言操作失败并返回 CANNOT_DISABLE_LAST_KEY，同时确认 updateKeyMock 或对应批量持久化更新未被调用；覆盖批量路径的事务前置和后置保护行为。
🤖 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.
Outside diff comments:
In `@src/actions/keys.ts`:
- Around line 1260-1266: 在处理禁用请求的分支中，更新 keys 操作（围绕 enabled 与
countActiveKeysByUser 的逻辑），仅当目标 key.isEnabled 当前为 true 时才执行最后一个启用 Key 的校验并返回
CANNOT_DISABLE_LAST_KEY；已禁用 Key 的重复禁用请求应跳过该错误码检查，并与 editKey 的行为保持一致。
---
Nitpick comments:
In `@tests/unit/actions/keys-self-service-authz.test.ts`:
- Around line 196-209: 在现有最后一个启用密钥保护测试附近，为 editKey 的 PATCH 禁用路径和 batchUpdateKeys
的批量禁用路径补充回归测试：模拟当前仅剩一个启用密钥，断言操作失败并返回 CANNOT_DISABLE_LAST_KEY，同时确认 updateKeyMock
或对应批量持久化更新未被调用；覆盖批量路径的事务前置和后置保护行为。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6be90f5c-0d8e-4c8d-9e62-c2770d1c8197
📒 Files selected for processing (4)
src/actions/keys.tssrc/lib/utils/error-messages.tstests/unit/actions/keys-self-service-authz.test.tstests/unit/api/v1/api-client-actions.test.ts
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ok: false, | ||
| error: tError("CANNOT_DISABLE_LAST_KEY"), | ||
| errorCode: ERROR_CODES.OPERATION_FAILED, | ||
| errorCode: ERROR_CODES.CANNOT_DISABLE_LAST_KEY, |
There was a problem hiding this comment.
[High] [TEST-MISSING-CRITICAL] The PATCH disable path still has no regression asserting the new machine-readable code
Why this is a problem: This line changed from ERROR_CODES.OPERATION_FAILED to ERROR_CODES.CANNOT_DISABLE_LAST_KEY, but the existing PATCH coverage still only checks expect(result.error).toBe("CANNOT_DISABLE_LAST_KEY");. If errorCode regresses to OPERATION_FAILED, the suite stays green while the client falls back to the generic toast again. That misses the CLAUDE.md rule: All new features must have unit test coverage of at least 80%.
Suggested fix:
const result = await editKey(42, { name: "own-key", isEnabled: false });
expect(result.ok).toBe(false);
if (!result.ok) {
expect(result.errorCode).toBe("CANNOT_DISABLE_LAST_KEY");
}
| throw new BatchUpdateError( | ||
| tError("CANNOT_DISABLE_LAST_KEY"), | ||
| ERROR_CODES.OPERATION_FAILED | ||
| ERROR_CODES.CANNOT_DISABLE_LAST_KEY |
There was a problem hiding this comment.
[High] [TEST-MISSING-CRITICAL] The batch-disable error-code changes are still untested
Why this is a problem: Both batch guards in this PR were switched from ERROR_CODES.OPERATION_FAILED to ERROR_CODES.CANNOT_DISABLE_LAST_KEY, but there is no test exercising either the pre-update per-user count check or the post-update race-condition recheck. A future regression on these lines would silently ship the generic code again with the current suite still passing. That misses the CLAUDE.md rule: All new features must have unit test coverage of at least 80%.
Suggested fix:
const result = await batchUpdateKeys({
keyIds: [42],
updates: { isEnabled: false },
});
expect(result).toMatchObject({
ok: false,
errorCode: "CANNOT_DISABLE_LAST_KEY",
});Add one test that makes currentEnabledCount - disableCount < 1, and a second that drives the post-update remainingEnabled.count recheck to 0.
There was a problem hiding this comment.
Code Review Summary
I found two high-priority test coverage gaps in newly changed error-code branches. The runtime change itself is small and targeted, but the current suite does not fully lock in the dedicated CANNOT_DISABLE_LAST_KEY contract across every modified path.
PR Size: XS
- Lines changed: 43
- Files changed: 4
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 | 2 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
- None.
High Priority Issues (Should Fix)
src/actions/keys.ts:508–editKey()now returnsCANNOT_DISABLE_LAST_KEY, but the existing PATCH regression still only assertsresult.error, so this contract can regress without a failing test.src/actions/keys.ts:1421– the batch-disable code change is untested in both the pre-update guard and the post-update recheck, so the genericOPERATION_FAILEDfallback could return again unnoticed.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
Review Posted
- Posted a PR review on
#1393, added 2 inline comments, and applied thesize/XSlabel. - Summary outcome: 2 high-priority issues, both in test coverage for newly changed error-code paths.
Findings
src/actions/keys.ts:508—editKey()now returnsCANNOT_DISABLE_LAST_KEY, but the existing PATCH test still only assertsresult.error, so this machine-readable contract can regress without a failing test.src/actions/keys.ts:1421— the batch-disable error-code change is untested; both the pre-update guard here and the post-update recheck atsrc/actions/keys.ts:1481can fall back toOPERATION_FAILEDagain without the suite catching it.
Note
- I attempted to run the focused Vitest checks, but
bunxis not installed in this GitHub Actions environment, so the review is based on full diff/context inspection plus repository-wide search.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Testing
User-visible behavior
当用户仅剩最后一个启用中的 key 时，禁用操作仍会返回 400，但前端现在显示对应语言的专用文案，而不是通用“操作失败”。
Greptile Summary
This PR preserves
CANNOT_DISABLE_LAST_KEYas a dedicated business error code instead of collapsing it toOPERATION_FAILED.Confidence Score: 4/5
The PR appears safe to merge, with only a non-blocking regression-test gap for the changed batch-disable paths.
The centralized code definition and direct REST error propagation preserve the intended machine-readable value, while the two batch guards remain untested for that specific contract.
Files Needing Attention: src/actions/keys.ts
Important Files Changed
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(keys): preserve last-enabled-key err..." | Re-trigger Greptile
Context used: