[DX-1123] fix(auth): add --force flag to revoke-token, standardize keys revoke prompt#364
[DX-1123] fix(auth): add --force flag to revoke-token, standardize keys revoke prompt#364
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR closes a safety gap in Changes
Review Notes
|
![claude-code-ably-assistant[bot]](https://avatars.githubusercontent.com/in/2109912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The approach is correct - revoke-token was the only hard-delete command missing the --force / confirmation safety pattern, and the fix is consistent with how the rest of the codebase handles this.
Two things worth addressing before merge:
- Incomplete standardization in keys/revoke.ts
The PR description says it replaced interactiveHelper.confirm() with promptForConfirmation() to match all other destructive commands, but keys/revoke.ts line 113 still uses the old helper for the remove-from-config prompt. This is now the only remaining interactiveHelper.confirm() across all destructive commands. If the intent was full standardization, migrate this one too. If it was intentionally left, the PR description should clarify why.
- Test cleanup not in try-finally - stdin can leak between tests
The 'should cancel when user declines confirmation' test (line 193) manipulates process.stdin but the cleanup is not in a try-finally block. If either expect() assertion throws an AssertionError, process.stdin stays replaced for every subsequent test in the same worker, causing silent test pollution. Move the Object.defineProperty restore and nock.cleanAll() into a finally block wrapping runCommand and the assertions.
There was a problem hiding this comment.
Pull request overview
Adds the standard “destructive action safety” pattern to auth revoke-token and aligns auth keys revoke with the shared confirmation-prompt helper, with corresponding unit test updates.
Changes:
- Added
--forceflag + interactive confirmation prompt toauth revoke-token, including a JSON-mode guard that requires--force. - Switched
auth keys revokeconfirmation frominteractiveHelper.confirm()topromptForConfirmation()for consistency. - Updated/extended unit tests to pass
--forcewhere required and to cover JSON guard + user-cancel flows.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/commands/auth/revoke-token.ts |
Adds --force, JSON guard, and interactive confirmation before revoking tokens. |
src/commands/auth/keys/revoke.ts |
Standardizes the destructive confirmation prompt helper. |
test/unit/commands/auth/revoke-token.test.ts |
Updates existing tests for new --force requirement and adds new coverage for guard/cancel behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sacOO7
force-pushed
the
fix/revoke-token-with-confirmation-prompt
branch
from
0ab3a37 to
736ff38
Compare
…dize keys revoke prompt - Add `--force` flag and interactive confirmation prompt to `auth revoke-token` (the only hard-delete command missing the safety pattern) - Require `--force` in JSON mode to prevent accidental scripted revocations - Replace `interactiveHelper.confirm()` with `promptForConfirmation()` in `auth keys revoke` for consistency with all other destructive commands - Update tests with new --force flag and add coverage for JSON guard and cancellation flow
sacOO7
force-pushed
the
fix/revoke-token-with-confirmation-prompt
branch
from
736ff38 to
e4c0028
Compare
|
@claude review |
There was a problem hiding this comment.
Review: fix(auth): add --force flag to revoke-token, standardize keys revoke prompt
The core change is correct and necessary. Adding --force to auth revoke-token was the right fix — it was the only hard-delete command missing this safety pattern. The JSON-mode guard and interactive confirmation flow are implemented correctly and mirror what other destructive commands do.
Two issues worth addressing:
1. Incomplete standardization in keys/revoke.ts — second interactiveHelper.confirm() left unchanged
The PR standardizes the primary revocation prompt (line ~76) to promptForConfirmation(), but the secondary prompt at line ~112 still uses this.interactiveHelper.confirm() (inquirer):
// Line ~112 — unchanged in this PR
const shouldRemove = await this.interactiveHelper.confirm(
"The revoked key was your current key for this app. Remove it from configuration?",
);interactiveHelper.confirm uses inquirer, while promptForConfirmation uses readline. These behave differently in non-TTY environments — inquirer may throw or hang, while readline degrades gracefully. In a single command run, the user now hits two different prompt UX implementations back-to-back. If the stated goal is to standardize on promptForConfirmation, this second call should be updated too.
2. Cancellation test uses fragile stdin monkey-patching
it("should cancel when user declines confirmation", async () => {
// replaces process.stdin with a Readable and pushes "n\n" via queueMicrotask
mockStdinAnswer("n");
...
const { stderr } = await runCommand([...]);
expect(stderr).toContain("Revocation cancelled");
expect(revokeNock.isDone()).toBe(false);
});This works due to Node.js Readable stream buffering (data is pushed before readline attaches), but it is fragile: any refactor of promptForConfirmation (e.g., switching to inquirer or a different input mechanism) would silently break this test without a compile error. The idiomatic approach is to mock the function itself:
import * as promptModule from "../../../utils/prompt-confirmation.js";
vi.spyOn(promptModule, "promptForConfirmation").mockResolvedValueOnce(false);This makes the test's intent explicit and decouples it from the implementation details of how stdin is read.
Both issues are fixable before merge. The feature itself is correct.
|
Looks like there's a couple of Claude comments so will hold off the review til those are addressed :) |
sacOO7
changed the title
|
@claude review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.log(`\nYou are about to revoke tokens matching:`); | ||
| if (flags["client-id"]) { | ||
| this.log( | ||
| `${formatLabel("Client ID")} ${formatResource(flags["client-id"])}`, | ||
| ); | ||
| } else { | ||
| const truncatedToken = | ||
| token.length > 15 ? token.slice(0, 15) + "..." : token; | ||
| this.log(`${formatLabel("Token")} ${formatResource(truncatedToken)}`); | ||
| } | ||
|
|
||
| const confirmed = await promptForConfirmation( | ||
| "\nThis will permanently revoke this token and any applications using it need to be re-issued a new token. Are you sure?", |
There was a problem hiding this comment.
The confirmation prompt text implies a single token is being revoked ("revoke this token"), but the request always revokes by clientId (targets: ["clientId:<value>"]). This is especially misleading when --client-id is provided (revokes all tokens for that client ID) and when it’s omitted (the positional token is treated as a client ID, with the warning currently logged later). Adjust the confirmation/warning flow so the user is told exactly what will be revoked before confirming.
| this.log(`\nYou are about to revoke tokens matching:`); | |
| if (flags["client-id"]) { | |
| this.log( | |
| `${formatLabel("Client ID")} ${formatResource(flags["client-id"])}`, | |
| ); | |
| } else { | |
| const truncatedToken = | |
| token.length > 15 ? token.slice(0, 15) + "..." : token; | |
| this.log(`${formatLabel("Token")} ${formatResource(truncatedToken)}`); | |
| } | |
| const confirmed = await promptForConfirmation( | |
| "\nThis will permanently revoke this token and any applications using it need to be re-issued a new token. Are you sure?", | |
| const revocationClientId = flags["client-id"] ?? token; | |
| this.log(`\nYou are about to revoke all tokens for this Client ID:`); | |
| this.log( | |
| `${formatLabel("Client ID")} ${formatResource(revocationClientId)}`, | |
| ); | |
| if (!flags["client-id"]) { | |
| this.logWarning( | |
| "No --client-id was provided. The positional token argument will be treated as a Client ID revocation target, and all tokens for that Client ID will be revoked.", | |
| flags, | |
| ); | |
| } | |
| const confirmed = await promptForConfirmation( | |
| "\nThis will permanently revoke all tokens for this Client ID, and any applications using those tokens will need to be issued new tokens. Are you sure?", |
| this.fail( | ||
| "The --force flag is required when using --json to confirm revocation", | ||
| flags, | ||
| "revokeToken", |
There was a problem hiding this comment.
This JSON-mode --force guard uses component name "revokeToken", but later fail() calls in this command use "tokenRevoke". Using different component strings will fragment CLI event/error grouping; make this consistent with the rest of the command.
| "revokeToken", | |
| "tokenRevoke", |
There was a problem hiding this comment.
Review: [DX-1123] fix(auth): add --force flag to revoke-token, standardize keys revoke prompt
The approach is solid — adding the --force guard pattern to revoke-token and migrating all interactiveHelper.confirm() calls to promptForConfirmation() are both correct moves. Two issues below.
1. Component string inconsistency in revoke-token.ts
File: src/commands/auth/revoke-token.ts
The new JSON-mode guard uses "revokeToken" as the component string, but every other this.fail() call in the same file uses "tokenRevoke":
// New guard (PR adds this)
this.fail(
"The --force flag is required when using --json to confirm revocation",
flags,
"revokeToken", // ← doesn't match the rest of this file
);
// Existing calls (all use "tokenRevoke")
this.fail("Invalid API key format...", flags, "tokenRevoke");
this.fail("Token not found or already revoked", flags, "tokenRevoke");
this.fail(error, flags, "tokenRevoke");Component strings appear in verbose log output and JSON error envelopes, so having two different component names for the same command makes debugging harder. Should be "tokenRevoke" to match.
2. Fragile stdin mock in the cancellation test
File: test/unit/commands/auth/revoke-token.test.ts
The "should cancel when user declines confirmation" test replaces process.stdin via Object.defineProperty and uses queueMicrotask to inject "n\n":
Object.defineProperty(process, "stdin", {
value: readable,
writable: true,
configurable: true,
});
queueMicrotask(() => {
for (const chunk of [`${answer}\n`, null]) readable.push(chunk);
});Two risks:
- Replaceability:
process.stdinis a getter-defined property in Node.js. In some environments (Node.js version, Vitest worker config) it may not be configurable, causingObject.definePropertyto throw silently or loudly. If it fails, the real stdin stays active andreadline.questionwaits indefinitely, hanging the test. - Timing: Pushing both
"n\n"andnull(EOF) via a single microtask means the stream is ended before readline's interface is created. If readline sees an already-ended stream, the callback might receive an empty string rather than"n"— cancellation would still happen, but for the wrong reason, masking a broken test.
A more reliable approach is vi.mock:
import { vi } from "vitest";
// At top of the file, before the describe block:
vi.mock("../../utils/prompt-confirmation.js", () => ({
promptForConfirmation: vi.fn(),
}));
// In the test:
import { promptForConfirmation } from "../../utils/prompt-confirmation.js";
(promptForConfirmation as ReturnType<typeof vi.fn>).mockResolvedValueOnce(false);This is immune to TTY/stdin environment differences and doesn't touch global process state.
Everything else looks correct: the --force flag is wired up properly, the JSON guard fires before the prompt block, examples are updated, promptForConfirmation is imported in all affected files (base-command.ts, keys/revoke.ts, bench/publisher.ts), and all existing tests correctly receive --force to bypass the new prompt.
Summary
auth revoke-token: Added--forceflag and interactive confirmation prompt — this was the only hard-delete command missing the safety pattern. In JSON mode,--forceis required to prevent accidental scripted revocations.auth keys revoke: ReplacedinteractiveHelper.confirm()withpromptForConfirmation()to match the pattern used by all other destructive commands.--force, added coverage for JSON mode guard and user cancellation flow.