Skip to content

fix(keys): preserve last-enabled-key error code #1393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ding113 merged 1 commit into from
Aug 7, 2026
Merged

fix(keys): preserve last-enabled-key error code #1393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/actions/keys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ export async function editKey(
return {
ok: false,
error: tError("CANNOT_DISABLE_LAST_KEY"),
errorCode: ERROR_CODES.OPERATION_FAILED,
errorCode: ERROR_CODES.CANNOT_DISABLE_LAST_KEY,

@github-actions github-actions Bot Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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");
}

};
}
}
Expand Down Expand Up @@ -1263,7 +1263,7 @@ export async function toggleKeyEnabled(keyId: number, enabled: boolean): Promise
return {
ok: false,
error: tError("CANNOT_DISABLE_LAST_KEY"),
errorCode: ERROR_CODES.OPERATION_FAILED,
errorCode: ERROR_CODES.CANNOT_DISABLE_LAST_KEY,

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Consume the dedicated code in dashboard error toasts

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 👍 / 👎.

};
}
}
Expand Down Expand Up @@ -1418,7 +1418,7 @@ export async function batchUpdateKeys(
if (currentEnabledCount - disableCount < 1) {
throw new BatchUpdateError(
tError("CANNOT_DISABLE_LAST_KEY"),
ERROR_CODES.OPERATION_FAILED
ERROR_CODES.CANNOT_DISABLE_LAST_KEY

@greptile-apps greptile-apps Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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)

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!

@github-actions github-actions Bot Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

);
}
}
Expand Down Expand Up @@ -1478,7 +1478,7 @@ export async function batchUpdateKeys(
if (Number(remainingEnabled?.count ?? 0) < 1) {
throw new BatchUpdateError(
tError("CANNOT_DISABLE_LAST_KEY"),
ERROR_CODES.OPERATION_FAILED
ERROR_CODES.CANNOT_DISABLE_LAST_KEY
);
}
}
Expand Down
1 change: 1 addition & 0 deletions src/lib/utils/error-messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ export const BUSINESS_ERRORS = {
USER_LIMITS_RESET_PARTIAL_FAILURE: "USER_LIMITS_RESET_PARTIAL_FAILURE",
USER_STATS_RESET_PARTIAL_FAILURE: "USER_STATS_RESET_PARTIAL_FAILURE",
CANNOT_DELETE_LAST_KEY: "CANNOT_DELETE_LAST_KEY",
CANNOT_DISABLE_LAST_KEY: "CANNOT_DISABLE_LAST_KEY",
CANNOT_DELETE_LAST_GROUP_KEY: "CANNOT_DELETE_LAST_GROUP_KEY",
KEY_NOT_FOUND: "KEY_NOT_FOUND",
} as const;
Expand Down
14 changes: 14 additions & 0 deletions tests/unit/actions/keys-self-service-authz.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,20 @@ describe("toggleKeyEnabled self-service authorization", () => {
expect(result.ok).toBe(true);
expect(updateKeyMock).toHaveBeenCalledWith(42, { is_enabled: false });
});

it("returns the dedicated business code when disabling the last enabled key", async () => {
getSessionMock.mockResolvedValue(webSession);
countActiveKeysByUserMock.mockResolvedValue(1);

const { toggleKeyEnabled } = await import("@/actions/keys");
const result = await toggleKeyEnabled(42, false);

expect(result.ok).toBe(false);
if (!result.ok) {
expect(result.errorCode).toBe("CANNOT_DISABLE_LAST_KEY");
}
expect(updateKeyMock).not.toHaveBeenCalled();
});
});

describe("renewKeyExpiresAt self-service authorization", () => {
Expand Down
20 changes: 20 additions & 0 deletions tests/unit/api/v1/api-client-actions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,26 @@ describe("v1 action compatibility client", () => {
});
});

test("preserves the last-enabled-key business code through toggleKeyEnabled", async () => {
postMock.mockRejectedValueOnce(
new ApiError({
status: 400,
errorCode: "CANNOT_DISABLE_LAST_KEY",
detail: "Bad request",
})
);

const result = await keys.toggleKeyEnabled(7, false);

expect(postMock).toHaveBeenCalledWith("/api/v1/keys/7:enable", { enabled: false }, undefined);
expect(result).toEqual({
ok: false,
error: "Bad request",
errorCode: "CANNOT_DISABLE_LAST_KEY",
errorParams: undefined,
});
});

test("maps key.action_failed through toVoidActionResult to OPERATION_FAILED", async () => {
deleteMock.mockRejectedValueOnce(
new ApiError({ status: 400, errorCode: "key.action_failed", detail: "Bad request" })
Expand Down
Loading