feat: Superset CLI + CLI framework + Better Auth 1.5.6#3194
Merged
Conversation
Command pseudocode for auth, tasks, workspaces, devices, and host. File-based routing structure with command.ts/meta.ts/middleware.ts convention. Includes CLI spec and TODO docs.
New CLI framework with file-based routing, typed option builders, single middleware with skip, lazy command loading, auto-generated help, signal handling, agent mode detection, and output formatting. ~1545 lines, zero dependencies, typechecks clean.
- CLI package imports from @superset/cli-framework - All commands typecheck cleanly - Help works at root, group, and command levels - Aliases work (ws → workspaces, t → tasks) - Typo suggestions work (taks → tasks) - --json output works - Middleware skip works for auth/host commands - Lazy-loads command descriptions for help
…ons in help - assigneeMe → --assignee-me in help and parsing - Command description now shows in command-level help - All option configs consistently kebab-cased
- Auth flow: login (browser callback), logout, whoami with real API calls - Tasks: list, get, create, delete with real tRPC mutations/queries - Workspaces/devices: stubbed with CLIError for device routing - Config management: ~/.superset/config.json for auth token storage - Middleware uses real readConfig + createApiClient
Breaking changes handled: - apiKey plugin moved to @better-auth/api-key (server + client imports) - apiKey.userId → apiKey.referenceId (schema + MCP auth flow) - Added configId column to apikeys table - Added device_codes table for OAuth device flow (RFC 8628) - Added deviceAuthorization() plugin to auth config Migration needed: apikeys table (userId → referenceId, add configId) + new device_codes table
CLI: superset auth login uses RFC 8628 device authorization flow — requests device code, opens browser with pre-filled code, polls for approval. Token stored in ~/.superset/config.json. Web: /device page for code entry and approve/deny flow.
- Device flow: fixed snake_case response fields, auto-registers OAuth client - /device page: redirects to sign-in if not authenticated, supports redirect param - Sign-in page: supports ?redirect= query param for post-auth redirect - Tasks list: flatten API response, display real data in table - Fixed migration SQL for apikeys rename - Added dev script to cli package.json with dotenv + NEXT_PUBLIC_API_URL
- org list: shows all orgs with active marker - org switch: interactive picker (clack select) when no arg, direct by name/slug/id - auth login: clack spinner during browser wait - /device page: org dropdown before authorize, redirects to sign-in if not authed - sign-in page: supports ?redirect= query param - tasks list: fixed field names to match API response shape - Added @clack/prompts dependency
- Table columns truncate at 60 chars with ellipsis - tRPC errors mapped to friendly messages (UNAUTHORIZED, NOT_FOUND, FETCH_ERROR) - --no-json flag overrides agent mode auto-detection - auth login: clack spinner, shows user/org after login, no duplicate output - tasks update command added - tasks get: fixed duplicate priority field - deviceAuth returns structured result for command to control display
- bin.ts auto-detects dev vs compiled mode - run-static.ts: static command tree from explicit imports (no fs scanning) - commands/index.ts: manifest of all commands, groups, middleware - buildStaticTree + resolveStaticMiddleware in framework - Build scripts: bun build --compile for darwin-arm64/x64 + linux-x64 - 55MB compiled binary, all features working
- Desktop: apiKeyClient import moved to @better-auth/api-key/client - Desktop: add @better-auth/api-key dependency - Desktop: type annotations for members.find(), token(), and stripe callbacks where Better Auth generic inference doesn't propagate through React hooks - Web: oauth consent response changed uri → url
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates better-auth dependencies to 1.5.6 across multiple applications, refactors API key verification to use Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Web as Web App<br/>(/device page)
participant API as Backend API
participant Session as Session<br/>Store
Browser->>Web: Load /device with user_code
Web->>Session: getSession()
alt Not authenticated
Web->>Browser: Redirect to /sign-in?redirect=/device
else Authenticated
Web->>API: GET /api/auth/device?user_code=XXXX
alt Valid code
API-->>Web: { userCode, expiresIn, ... }
Web->>Web: Load organizations & set activeOrg
Web->>Browser: Display approval form
Browser->>Web: User clicks approve
alt Has org context
Web->>API: POST /api/auth/organization/setActive
end
Web->>API: POST /api/auth/device/approve { userCode }
API-->>Web: Success
Web->>Browser: Show approved message
else Invalid/Expired code
Web->>Browser: Display error form
end
end
sequenceDiagram
participant User
participant CLI as CLI<br/>(bin.ts)
participant Framework as CLI Framework<br/>(router/parser)
participant Command as Command<br/>Handler
participant API as API Client
User->>CLI: superset tasks list --status=open
CLI->>Framework: route command path
Framework-->>CLI: { commandPath, args }
CLI->>Framework: parse argv
Framework-->>CLI: { options, positionals }
CLI->>Framework: resolve middleware
Framework->>API: Middleware creates API client
API-->>Framework: Client context
Framework->>Command: run({ options, ctx, signal })
Command->>API: api.task.all.query(filter)
API-->>Command: tasks[]
Command-->>CLI: { data: tasks[], message }
CLI->>Framework: formatOutput(result, flags)
Framework-->>CLI: formatted string
CLI->>User: Print table to stdout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
17 issues found across 71 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/lib/auth.ts">
<violation number="1" location="packages/cli/src/lib/auth.ts:66">
P1: **Command injection via shell-interpolated URL + silently swallowed error.** `verificationUrl` is server-controlled and passed directly into a shell via `exec`. A malicious API response can inject arbitrary shell commands. Use `execFile` (which does not invoke a shell) to pass the URL as an argument instead of interpolating it into a command string. This also lets you handle errors properly.
(Based on your team's feedback about handling async errors explicitly and not silently swallowing failures.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/cli/src/lib/auth.ts:69">
P2: RFC 8628 requires that `slow_down` permanently increases the polling interval by 5 seconds for all subsequent requests, not just the current iteration. Change `interval` to `let` and add `interval += 5000` in the `slow_down` branch.</violation>
</file>
<file name="packages/cli/src/commands/tasks/delete/command.ts">
<violation number="1" location="packages/cli/src/commands/tasks/delete/command.ts:13">
P1: The delete command claims to support task IDs, but it only resolves tasks by slug and never falls back to `task.byId`, causing valid UUID inputs to fail.</violation>
</file>
<file name="packages/cli-framework/src/cli.ts">
<violation number="1" location="packages/cli-framework/src/cli.ts:93">
P2: Help detection scans all raw args and ignores `--`, so positional values like `--help` can incorrectly trigger help instead of running the command.</violation>
</file>
<file name="packages/cli-framework/src/command.ts">
<violation number="1" location="packages/cli-framework/src/command.ts:3">
P2: Use a discriminated union for `CommandResult` instead of shape-based variants to make result handling explicit and safer to evolve.
(Based on your team's feedback about using discriminated unions for result typing.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/cli/src/bin.ts">
<violation number="1" location="packages/cli/src/bin.ts:6">
P2: Build `commandsDir` with a file-URL-to-path utility instead of `URL.pathname`; the current value can be invalid on Windows and break compiled/dev mode detection.
(Based on your team's feedback about using cross-platform path utilities.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/cli/src/commands/host/start/command.ts">
<violation number="1" location="packages/cli/src/commands/host/start/command.ts:12">
P1: Do not return a successful "Host service started" result for daemon mode before the host is actually forked.</violation>
</file>
<file name="packages/cli-framework/src/parser.ts">
<violation number="1" location="packages/cli-framework/src/parser.ts:126">
P2: Negative numeric values are incorrectly rejected for `--flag value` syntax because any `-`-prefixed value is treated as a missing argument.</violation>
</file>
<file name="packages/db/src/schema/auth.ts">
<violation number="1" location="packages/db/src/schema/auth.ts:283">
P2: The `deviceCodes` table is missing indexes on `device_code` and `user_code`. Both are lookup columns hit during the device flow — `device_code` on every poll request and `user_code` when the user approves. Without indexes, these will be sequential scans on every poll interval.</violation>
</file>
<file name="packages/cli/src/commands/auth/whoami/command.ts">
<violation number="1" location="packages/cli/src/commands/auth/whoami/command.ts:14">
P2: Wrap async API calls in explicit error handling and rethrow as `CLIError` so failures are surfaced predictably instead of bubbling as unhandled command errors.
(Based on your team's feedback about handling async failures explicitly and avoiding unhandled rejections.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/cli/src/commands/host/install/command.ts">
<violation number="1" location="packages/cli/src/commands/host/install/command.ts:7">
P2: This command currently exits successfully even though install is not implemented, which can mislead users and scripts into believing host service setup completed.</violation>
</file>
<file name="apps/web/src/app/device/page.tsx">
<violation number="1" location="apps/web/src/app/device/page.tsx:23">
P2: Unhandled promise rejection: `authClient.getSession().then(...)` and the nested `organization.list().then(...)` have no `.catch()` handlers. A network failure here will produce an unhandled rejection instead of showing an error state to the user.
(Based on your team's feedback about handling async errors explicitly.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/web/src/app/device/page.tsx:71">
P2: The `catch` block in `handleDeny` silently swallows the error. Capture the error and log a warning so failures during deny are observable.
(Based on your team's feedback about handling async errors explicitly and avoiding silent catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/cli-framework/src/router.ts">
<violation number="1" location="packages/cli-framework/src/router.ts:279">
P2: The `?? middlewares.get(segment)` fallback can match middleware registered under a bare segment name (e.g., `"login"`) even when the full path is `"auth/login"`. This could cause unrelated middleware to apply when two different command subtrees share a segment name. `resolveMiddleware` doesn't have this issue because it uses full directory paths. Drop the fallback to keep behavior consistent.</violation>
</file>
<file name="packages/cli/src/commands/auth/login/command.ts">
<violation number="1" location="packages/cli/src/commands/auth/login/command.ts:39">
P2: Avoid silently swallowing failures in the post-login whoami fetch; log a warning so this async failure is observable.
(Based on your team's feedback about handling async errors explicitly and avoiding empty catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/cli/src/commands/org/list/command.ts">
<violation number="1" location="packages/cli/src/commands/org/list/command.ts:16">
P2: Wrap API awaits in explicit error handling so request failures are surfaced with context instead of bubbling as unhandled rejections.
(Based on your team's feedback about handling async calls/errors explicitly and avoiding silent or unhandled failures.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/cli/src/lib/api-client.ts">
<violation number="1" location="packages/cli/src/lib/api-client.ts:14">
P2: Normalize `apiUrl` before appending `/api/trpc` to avoid double-slash request paths when config includes a trailing slash.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| : "xdg-open"; | ||
|
|
||
| const { exec } = await import("node:child_process"); | ||
| exec(`${openCmd} "${verificationUrl}"`); |
Contributor
There was a problem hiding this comment.
P1: Command injection via shell-interpolated URL + silently swallowed error. verificationUrl is server-controlled and passed directly into a shell via exec. A malicious API response can inject arbitrary shell commands. Use execFile (which does not invoke a shell) to pass the URL as an argument instead of interpolating it into a command string. This also lets you handle errors properly.
(Based on your team's feedback about handling async errors explicitly and not silently swallowing failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/lib/auth.ts, line 66:
<comment>**Command injection via shell-interpolated URL + silently swallowed error.** `verificationUrl` is server-controlled and passed directly into a shell via `exec`. A malicious API response can inject arbitrary shell commands. Use `execFile` (which does not invoke a shell) to pass the URL as an argument instead of interpolating it into a command string. This also lets you handle errors properly.
(Based on your team's feedback about handling async errors explicitly and not silently swallowing failures.) </comment>
<file context>
@@ -0,0 +1,163 @@
+ : "xdg-open";
+
+ const { exec } = await import("node:child_process");
+ exec(`${openCmd} "${verificationUrl}"`);
+
+ // Step 3: Poll for token
</file context>
|
|
||
| for (const idOrSlug of ids) { | ||
| // Try as slug first, then as UUID | ||
| const task = await api.task.bySlug.query(idOrSlug); |
Contributor
There was a problem hiding this comment.
P1: The delete command claims to support task IDs, but it only resolves tasks by slug and never falls back to task.byId, causing valid UUID inputs to fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/tasks/delete/command.ts, line 13:
<comment>The delete command claims to support task IDs, but it only resolves tasks by slug and never falls back to `task.byId`, causing valid UUID inputs to fail.</comment>
<file context>
@@ -0,0 +1,28 @@
+
+ for (const idOrSlug of ids) {
+ // Try as slug first, then as UUID
+ const task = await api.task.bySlug.query(idOrSlug);
+ if (!task) {
+ throw new CLIError(`Task not found: ${idOrSlug}`);
</file context>
| run: async (opts) => { | ||
| if (opts.options.daemon) { | ||
| // TODO: fork to background | ||
| return { data: { pid: 0, port: opts.options.port }, message: "Host service started" }; |
Contributor
There was a problem hiding this comment.
P1: Do not return a successful "Host service started" result for daemon mode before the host is actually forked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/host/start/command.ts, line 12:
<comment>Do not return a successful "Host service started" result for daemon mode before the host is actually forked.</comment>
<file context>
@@ -0,0 +1,17 @@
+ run: async (opts) => {
+ if (opts.options.daemon) {
+ // TODO: fork to background
+ return { data: { pid: 0, port: opts.options.port }, message: "Host service started" };
+ }
+ // TODO: foreground mode with opts.signal for cleanup
</file context>
| exec(`${openCmd} "${verificationUrl}"`); | ||
|
|
||
| // Step 3: Poll for token | ||
| const interval = (codeData.interval || 5) * 1000; |
Contributor
There was a problem hiding this comment.
P2: RFC 8628 requires that slow_down permanently increases the polling interval by 5 seconds for all subsequent requests, not just the current iteration. Change interval to let and add interval += 5000 in the slow_down branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/lib/auth.ts, line 69:
<comment>RFC 8628 requires that `slow_down` permanently increases the polling interval by 5 seconds for all subsequent requests, not just the current iteration. Change `interval` to `let` and add `interval += 5000` in the `slow_down` branch.</comment>
<file context>
@@ -0,0 +1,163 @@
+ exec(`${openCmd} "${verificationUrl}"`);
+
+ // Step 3: Poll for token
+ const interval = (codeData.interval || 5) * 1000;
+ const deadline = Date.now() + codeData.expires_in * 1000;
+
</file context>
| const { root, middlewares } = await scanCommands(commandsDir); | ||
|
|
||
| // Quick check for --help with no command | ||
| if (args.length === 0 || args.includes("--help") || args.includes("-h")) { |
Contributor
There was a problem hiding this comment.
P2: Help detection scans all raw args and ignores --, so positional values like --help can incorrectly trigger help instead of running the command.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli-framework/src/cli.ts, line 93:
<comment>Help detection scans all raw args and ignores `--`, so positional values like `--help` can incorrectly trigger help instead of running the command.</comment>
<file context>
@@ -0,0 +1,289 @@
+ const { root, middlewares } = await scanCommands(commandsDir);
+
+ // Quick check for --help with no command
+ if (args.length === 0 || args.includes("--help") || args.includes("-h")) {
+ // Check if there's a command path first
+ const routeResult = routeCommand(root, args.filter(a => a !== "--help" && a !== "-h"), commandsDir);
</file context>
| const [selectedOrgId, setSelectedOrgId] = useState<string>(""); | ||
|
|
||
| useEffect(() => { | ||
| authClient.getSession().then(({ data: session }) => { |
Contributor
There was a problem hiding this comment.
P2: Unhandled promise rejection: authClient.getSession().then(...) and the nested organization.list().then(...) have no .catch() handlers. A network failure here will produce an unhandled rejection instead of showing an error state to the user.
(Based on your team's feedback about handling async errors explicitly.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/app/device/page.tsx, line 23:
<comment>Unhandled promise rejection: `authClient.getSession().then(...)` and the nested `organization.list().then(...)` have no `.catch()` handlers. A network failure here will produce an unhandled rejection instead of showing an error state to the user.
(Based on your team's feedback about handling async errors explicitly.) </comment>
<file context>
@@ -0,0 +1,273 @@
+ const [selectedOrgId, setSelectedOrgId] = useState<string>("");
+
+ useEffect(() => {
+ authClient.getSession().then(({ data: session }) => {
+ if (!session) {
+ const returnUrl = `/device${userCodeParam ? `?user_code=${userCodeParam}` : ""}`;
</file context>
| let pathSoFar = ""; | ||
| for (const segment of commandPath) { | ||
| pathSoFar = pathSoFar ? `${pathSoFar}/${segment}` : segment; | ||
| const mw = middlewares.get(pathSoFar) ?? middlewares.get(segment); |
Contributor
There was a problem hiding this comment.
P2: The ?? middlewares.get(segment) fallback can match middleware registered under a bare segment name (e.g., "login") even when the full path is "auth/login". This could cause unrelated middleware to apply when two different command subtrees share a segment name. resolveMiddleware doesn't have this issue because it uses full directory paths. Drop the fallback to keep behavior consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli-framework/src/router.ts, line 279:
<comment>The `?? middlewares.get(segment)` fallback can match middleware registered under a bare segment name (e.g., `"login"`) even when the full path is `"auth/login"`. This could cause unrelated middleware to apply when two different command subtrees share a segment name. `resolveMiddleware` doesn't have this issue because it uses full directory paths. Drop the fallback to keep behavior consistent.</comment>
<file context>
@@ -0,0 +1,288 @@
+ let pathSoFar = "";
+ for (const segment of commandPath) {
+ pathSoFar = pathSoFar ? `${pathSoFar}/${segment}` : segment;
+ const mw = middlewares.get(pathSoFar) ?? middlewares.get(segment);
+ if (mw === skip) {
+ activeMiddleware = null;
</file context>
| const org = await api.user.myOrganization.query(); | ||
| p.log.info(`${user.name} (${user.email})`); | ||
| if (org) p.log.info(`Organization: ${org.name}`); | ||
| } catch { |
Contributor
There was a problem hiding this comment.
P2: Avoid silently swallowing failures in the post-login whoami fetch; log a warning so this async failure is observable.
(Based on your team's feedback about handling async errors explicitly and avoiding empty catch blocks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/auth/login/command.ts, line 39:
<comment>Avoid silently swallowing failures in the post-login whoami fetch; log a warning so this async failure is observable.
(Based on your team's feedback about handling async errors explicitly and avoiding empty catch blocks.) </comment>
<file context>
@@ -0,0 +1,47 @@
+ const org = await api.user.myOrganization.query();
+ p.log.info(`${user.name} (${user.email})`);
+ if (org) p.log.info(`Organization: ${org.name}`);
+ } catch {
+ // Non-fatal — login succeeded even if whoami fails
+ }
</file context>
|
|
||
| run: async (opts) => { | ||
| const api = opts.ctx.api as ApiClient; | ||
| const orgs = await api.user.myOrganizations.query(); |
Contributor
There was a problem hiding this comment.
P2: Wrap API awaits in explicit error handling so request failures are surfaced with context instead of bubbling as unhandled rejections.
(Based on your team's feedback about handling async calls/errors explicitly and avoiding silent or unhandled failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/org/list/command.ts, line 16:
<comment>Wrap API awaits in explicit error handling so request failures are surfaced with context instead of bubbling as unhandled rejections.
(Based on your team's feedback about handling async calls/errors explicitly and avoiding silent or unhandled failures.) </comment>
<file context>
@@ -0,0 +1,27 @@
+
+ run: async (opts) => {
+ const api = opts.ctx.api as ApiClient;
+ const orgs = await api.user.myOrganizations.query();
+ const me = await api.user.myOrganization.query();
+ const activeId = me?.id;
</file context>
| return createTRPCClient<AppRouter>({ | ||
| links: [ | ||
| httpBatchLink({ | ||
| url: `${getApiUrl(config)}/api/trpc`, |
Contributor
There was a problem hiding this comment.
P2: Normalize apiUrl before appending /api/trpc to avoid double-slash request paths when config includes a trailing slash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/lib/api-client.ts, line 14:
<comment>Normalize `apiUrl` before appending `/api/trpc` to avoid double-slash request paths when config includes a trailing slash.</comment>
<file context>
@@ -0,0 +1,22 @@
+ return createTRPCClient<AppRouter>({
+ links: [
+ httpBatchLink({
+ url: `${getApiUrl(config)}/api/trpc`,
+ transformer: SuperJSON,
+ headers() {
</file context>
Greptile SummaryThis PR introduces the Superset CLI ( The framework is well-architected: typed phantom-type option builders, file-based routing with lazy loading, single-middleware-per-tree, auto-generated help at every level, agent-mode auto-detection, and compiled-binary support via static registration. The Better Auth upgrade is handled cleanly across all five apps (web, desktop, mobile, admin, api), with the Issues found:
Confidence Score: 4/5Safe to merge after fixing the two P1 issues (filter pass-through and grant_type registration); the P2s are low-risk polish. The core auth flow, Better Auth upgrade, framework routing, and DB migration are all solid. Two P1 bugs (silent filter no-op and wrong OAuth grant_type) need to be addressed before the PR can be considered fully correct on its primary user paths, hence 4 rather than 5. packages/cli/src/commands/tasks/list/command.ts (filter options not forwarded) and packages/cli/src/lib/auth.ts (wrong grant_types in client registration + execFile hardening) Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as superset CLI
participant API as Superset API
participant Browser as User Browser
participant WebApp as Web App (/device)
CLI->>API: POST /api/auth/oauth2/register<br/>(client_name, grant_types)
API-->>CLI: { client_id }
CLI->>API: POST /api/auth/device/code<br/>{ client_id }
API-->>CLI: { device_code, user_code, verification_uri, expires_in, interval }
CLI->>Browser: open(verification_uri_complete)
Browser->>WebApp: GET /device?user_code=XXXX
WebApp->>API: GET /api/auth/device?user_code=XXXX
API-->>WebApp: 200 OK (code valid)
WebApp->>API: POST /api/auth/organization/set-active
WebApp->>API: POST /api/auth/device/approve
API-->>WebApp: 200 OK
loop Poll every interval seconds
CLI->>API: POST /api/auth/device/token<br/>{ device_code, grant_type }
API-->>CLI: authorization_pending OR { access_token }
end
CLI->>CLI: writeConfig({ auth: { accessToken } })
Reviews (1): Last reviewed commit: "fix: resolve all lint errors (biome form..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+22
to
+30
| limit: number().default(50).desc("Max results"), | ||
| offset: number().default(0).desc("Skip results"), | ||
| }, | ||
| display: (data) => | ||
| table( | ||
| data as Record<string, unknown>[], | ||
| ["slug", "title", "priority", "assignee"], | ||
| ["SLUG", "TITLE", "PRIORITY", "ASSIGNEE"], | ||
| ), |
There was a problem hiding this comment.
Seven options (status, priority, assigneeMe, creatorMe, search, limit, offset) are declared but the run function calls api.task.all.query() with no arguments. All filtering is a no-op regardless of what the user passes.
Suggested change
| limit: number().default(50).desc("Max results"), | |
| offset: number().default(0).desc("Skip results"), | |
| }, | |
| display: (data) => | |
| table( | |
| data as Record<string, unknown>[], | |
| ["slug", "title", "priority", "assignee"], | |
| ["SLUG", "TITLE", "PRIORITY", "ASSIGNEE"], | |
| ), | |
| run: async (opts) => { | |
| const api = opts.ctx.api as ApiClient; | |
| const result = await api.task.all.query({ | |
| status: opts.options.status as string | undefined, | |
| priority: opts.options.priority as string | undefined, | |
| assigneeMe: opts.options.assigneeMe, | |
| creatorMe: opts.options.creatorMe, | |
| search: opts.options.search, | |
| limit: opts.options.limit, | |
| offset: opts.options.offset, | |
| }); |
(adjust to match the actual tRPC input schema — the point is the options need to be forwarded)
Comment on lines
+116
to
+127
|
|
||
| throw new CLIError("Authorization timed out — please try again"); | ||
| } | ||
|
|
||
| /** | ||
| * Ensure we have a registered OAuth client ID for this API. | ||
| * Registers one via dynamic client registration on first use, | ||
| * then caches it in ~/.superset/config.json. | ||
| */ | ||
| async function ensureClientId(apiUrl: string): Promise<string> { | ||
| const config = readConfig(); | ||
| const cached = config.clientIds?.[apiUrl]; |
There was a problem hiding this comment.
grant_types in dynamic client registration
The client registers with grant_types: ["authorization_code"], but the device authorization flow uses the grant type urn:ietf:params:oauth:grant-type:device_code. Per RFC 8628 §3.4 and RFC 7591 §2, authorization servers that validate a client's registered grant types against actual usage will reject the device-code token request with unauthorized_client. This would silently break superset auth login in a stricter server configuration.
Suggested change
| throw new CLIError("Authorization timed out — please try again"); | |
| } | |
| /** | |
| * Ensure we have a registered OAuth client ID for this API. | |
| * Registers one via dynamic client registration on first use, | |
| * then caches it in ~/.superset/config.json. | |
| */ | |
| async function ensureClientId(apiUrl: string): Promise<string> { | |
| const config = readConfig(); | |
| const cached = config.clientIds?.[apiUrl]; | |
| body: JSON.stringify({ | |
| client_name: CLI_CLIENT_NAME, | |
| redirect_uris: ["http://localhost/callback"], | |
| grant_types: ["urn:ietf:params:oauth:grant-type:device_code"], | |
| response_types: ["code"], | |
| token_endpoint_auth_method: "none", | |
| }), |
Comment on lines
+62
to
+67
| ? "start" | ||
| : "xdg-open"; | ||
|
|
||
| const { exec } = await import("node:child_process"); | ||
| exec(`${openCmd} "${verificationUrl}"`); | ||
|
|
There was a problem hiding this comment.
execFile
exec(\${openCmd} "${verificationUrl}"`)interpolates a server-controlled value into a shell command string. If the API response ever contains double-quotes, backticks, or$()in theverification_uri_completefield (e.g., via a compromised API or MITM before the TLS handshake completes), this becomes a remote command injection vector on the developer's machine. UsingexecFile` avoids the shell entirely:
Suggested change
| ? "start" | |
| : "xdg-open"; | |
| const { exec } = await import("node:child_process"); | |
| exec(`${openCmd} "${verificationUrl}"`); | |
| const { execFile } = await import("node:child_process"); | |
| execFile(openCmd, [verificationUrl]); |
Comment on lines
+31
to
+38
| return JSON.parse(readFileSync(CONFIG_PATH, "utf-8")); | ||
| } | ||
|
|
||
| export function writeConfig(config: SupersetConfig): void { | ||
| ensureDir(); | ||
| writeFileSync(CONFIG_PATH, JSON.stringify(config, null, 2)); | ||
| } | ||
|
|
There was a problem hiding this comment.
JSON.parse(readFileSync(CONFIG_PATH, "utf-8")) throws a raw SyntaxError (and readDeviceConfig has the same problem) if the file was partially written or manually edited. The user would see an opaque stack trace instead of a helpful hint.
Consider wrapping in a try/catch:
export function readConfig(): SupersetConfig {
if (!existsSync(CONFIG_PATH)) return {};
try {
return JSON.parse(readFileSync(CONFIG_PATH, "utf-8"));
} catch {
throw new Error(
`Corrupt config file at ${CONFIG_PATH}. Delete it and run: superset auth login`
);
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
packages/cli-framework/src/help.ts (2)
31-39: Non-null assertion could be avoided with inline lookup.The
root.children.get(name)!assertion is safe sinceentriescomes from the same map, but restructuring avoids the assertion.♻️ Proposed refactor
const maxLen = Math.max( - ...entries.map(([name]) => { - const node = root.children.get(name)!; + ...entries.map(([name, node]) => { const aliasStr = node.aliases?.length ? ` (${node.aliases.join(", ")})` : ""; return name.length + aliasStr.length; }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-framework/src/help.ts` around lines 31 - 39, The code uses a non-null assertion on root.children.get(name)! when computing maxLen; avoid the assertion by destructuring the map entries or performing the lookup once inline: iterate over entries (the same Map entries used to build the array) and use the already-available node (or assign const node = root.children.get(name) with a guarded fallback) to compute aliasStr and length, updating the logic in the maxLen calculation (function/variable names: maxLen, entries, root.children, aliasStr) so no “!” is needed.
63-68: Unused_globalsparameters suggest incomplete help output.Both
generateGroupHelpandgenerateCommandHelpaccept_globalsbut don't use them. If global options should appear in subcommand/group help (which is common CLI behavior), this is incomplete. Otherwise, remove the parameters to clarify intent.Also applies to: 95-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-framework/src/help.ts` around lines 63 - 68, The functions generateGroupHelp and generateCommandHelp declare an unused _globals parameter, causing dead code or incomplete help output; either remove the _globals parameter from both function signatures (and any callers) to clarify intent, or update both functions to incorporate global options into the rendered help (e.g., merge ProcessedBuilderConfig entries into the options/flags section) so globals appear in subcommand/group help—locate the functions by name and adjust their signatures and implementation consistently across callers.packages/cli/src/commands/tasks/delete/command.ts (1)
11-18: Partial failure leaves system in inconsistent state.If deletion fails midway through the loop, some tasks will be deleted while others remain. Consider either wrapping in a transaction (if supported) or collecting all resolved tasks first, then deleting, to provide better error reporting.
Additionally, sequential API calls could be parallelized with
Promise.allfor better performance.♻️ Proposed refactor with parallel resolution and deletion
- for (const idOrSlug of ids) { - // Try as slug first, then as UUID - const task = await api.task.bySlug.query(idOrSlug); - if (!task) { - throw new CLIError(`Task not found: ${idOrSlug}`); - } - await api.task.delete.mutate(task.id); - } + // Resolve all tasks first to fail fast before any deletions + const tasks = await Promise.all( + ids.map(async (idOrSlug) => { + const task = await api.task.bySlug.query(idOrSlug); + if (!task) { + throw new CLIError(`Task not found: ${idOrSlug}`); + } + return task; + }), + ); + + // Delete all resolved tasks + await Promise.all(tasks.map((task) => api.task.delete.mutate(task.id)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/tasks/delete/command.ts` around lines 11 - 18, The loop deletes tasks one-by-one which can leave partial state on failure; first resolve all ids into tasks using api.task.bySlug.query in parallel (Promise.all) and validate that every idOrSlug produced a task (throw a single aggregated CLIError listing missing ids if any), then perform deletions in parallel via api.task.delete.mutate (or wrap in a transaction if your API supports a transactional endpoint) so either all delete promises are awaited together and failures reported collectively; refer to the ids array, api.task.bySlug.query, and api.task.delete.mutate to locate and change the logic.packages/cli/src/commands/workspaces/delete/command.ts (1)
1-1: Unused importstring.The
stringbuilder is imported but not used. Thedeviceoption on line 7 appears to define the option but the option value (opts.options.device) is never accessed—onlyopts.ctx.deviceIdis checked.♻️ Remove unused import
-import { CLIError, command, positional, string } from "@superset/cli-framework"; +import { CLIError, command, positional } from "@superset/cli-framework";If the
deviceoption is intended to populatectx.deviceIdvia middleware, the import removal is sufficient. Otherwise, verify the option wiring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/workspaces/delete/command.ts` at line 1, Remove the unused `string` import from the top-level import statement in command.ts (import { CLIError, command, positional, string } ...) since `string` is never referenced; then verify the `device` option wiring: either read the option value via `opts.options.device` where needed or ensure middleware correctly populates `opts.ctx.deviceId` and remove the unused `device` option if middleware is relied upon — adjust usage of `opts.options.device`/`opts.ctx.deviceId` accordingly.packages/cli/src/run-static.ts (1)
45-49: Avoidanyin the shared error path.A narrow error type or small type guard is enough for the
UNAUTHORIZEDcheck here. Usinganyin this hot path throws away type safety for no real gain.Suggested fix
+type UnauthorizedLikeError = Error & { + code?: string; + data?: { code?: string }; +}; + if (error instanceof Error) { + const authError = error as UnauthorizedLikeError; if ( - (error as any).data?.code === "UNAUTHORIZED" || - (error as any).code === "UNAUTHORIZED" + authError.data?.code === "UNAUTHORIZED" || + authError.code === "UNAUTHORIZED" ) {As per coding guidelines,
Avoid any type annotation in TypeScript code unless necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/run-static.ts` around lines 45 - 49, The current UNAUTHORIZED checks use (error as any) which loses type-safety; create a narrow type guard (e.g., isUnauthorizedError(err: unknown): err is { code?: string; data?: { code?: string } }) and use it inside the existing error instanceof Error branch to safely check (err.code === "UNAUTHORIZED" || err.data?.code === "UNAUTHORIZED") without any casts; update the UNAUTHORIZED check in run-static.ts to call this guard (referencing the error variable and the new isUnauthorizedError function) so you avoid using any while preserving the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-framework/src/errors.ts`:
- Around line 54-56: Transposition is using the variable cost (which can be 0
when chars match) instead of a fixed cost of 1, producing incorrect
Damerau-Levenshtein distances; in the block that checks transposition (the if
with i, j, a[i-1] === b[j-2] && a[i-2] === b[j-1]) update the update to use +1
(a fixed transposition cost) instead of +cost—i.e., replace the term (d[i -
2]?.[j - 2] ?? 0) + cost with (d[i - 2]?.[j - 2] ?? 0) + 1 so variables a, b, d,
val, cost remain otherwise unchanged.
In `@packages/cli-framework/src/parser.ts`:
- Around line 132-141: The parser currently rejects any nextArg that starts with
"-" when parsing option values (see variables args, i, nextArg and the CLIError
thrown inside parser.ts), which prevents negative numbers like `--offset -5`;
change the check so that instead of using nextArg.startsWith("-") you verify
whether nextArg is a known flag token (e.g., matches an entry in the parser's
flag/option map or a flag pattern) and only treat it as missing when it is a
recognized flag; update the validation around the entry handling (entry[1],
enumVals) to allow `-5` as a value while still throwing CLIError when the next
token is actually another defined flag.
- Around line 155-161: The linter flags the non-null assertion on
process.env[config.envVar] inside the parser logic; to fix, read the environment
value into a local const (e.g., const envVal = process.env[config.envVar]),
check it for undefined with the existing if (config.envVar && envVal !==
undefined) condition, and then pass envVal (not a non-null assertion) into
coerce when setting options[key]; update the block around the coerce(...) call
(where config, key, and coerce are used) to use the local variable instead.
- Around line 61-62: Replace the forbidden non-null assertion on args[i]! by
extracting with a guard or iterating safely: in parser.ts change the loop that
uses for (let i = 0; i < args.length; i++) { const arg = args[i]!; } to either
use a for (const arg of args) { ... } or keep the index loop but use const arg =
args[i]; if (arg === undefined) continue; so no non-null assertion is required
(refer to the args variable and the loop body where arg is used).
In `@packages/cli-framework/src/router.ts`:
- Around line 117-133: The router currently only throws a CLIError for an
invalid segment when suggestSimilar(...) returns a suggestion, otherwise it
falls through (break) and lets callers treat the command as valid; update the
logic in the block that checks if (!matched && currentNode.children.size > 0) to
always throw a CLIError for the unknown segment variable `segment` when no child
matches; if `suggestSimilar(segment, candidates)` returns a suggestion include
that suggestion text in the CLIError, otherwise throw a CLIError with a generic
"Unknown command: <segment>" message so invalid subcommands always fail (modify
the code around `matched`, `currentNode.children`, `suggestSimilar`, and the
`CLIError` construction accordingly).
- Around line 276-284: The loop resolving middleware incorrectly falls back to
middlewares.get(segment) which allows middleware defined for a segment name in
one branch to leak into other branches; change the resolution in the loop that
builds pathSoFar (the code using commandPath, segment, pathSoFar, middlewares,
skip, and activeMiddleware) to look up middleware only by the full path key
(middlewares.get(pathSoFar)) and remove the fallback to
middlewares.get(segment), preserving the existing handling for skip and setting
activeMiddleware accordingly.
- Around line 98-99: Remove unnecessary non-null assertions in router.ts by
replacing direct uses of asserted expressions with a local non-null-typed
variable where loop bounds or prior checks already guarantee existence: for the
segment loop replace uses of args[i]! with const segment = args[i]; and
similarly in the group and command node creation loops introduce a local const
child = node.children.get(key) (or equivalent) after the existence check (or
where i < args.length ensures presence) and use child instead of child! or other
non-null assertions on lines referenced (99, ~226, ~236, ~244); this removes the
"!" assertions and improves clarity while preserving runtime guarantees.
In `@packages/cli/src/commands/tasks/get/command.ts`:
- Around line 5-14: The command claims to accept "ID or slug" but only calls
api.task.bySlug.query(slug), so add a fallback to try ID lookup when slug lookup
returns no task: in the run handler after const task = await
api.task.bySlug.query(slug), if task is falsy attempt an ID lookup via the
task-by-id endpoint (e.g., api.task.byId.query(slug) or whichever method exists)
and use that result; if still not found throw the same CLIError(`Task not found:
${slug}`). Keep the original description and argument name, and ensure you
reference the same run function and api.task.bySlug.query/api.task.byId.query
symbols when making the change.
In `@packages/cli/src/commands/tasks/list/command.ts`:
- Around line 31-37: The CLI is passing filter flags to api.task.all.query but
the backend task.all is defined as protectedProcedure.query(...) with no
.input(), so passing params will fail; either remove the unused flags from the
CLI command (the run handler in command.ts around api.task.all.query and the
option definitions) or update the backend task router's task.all procedure to
accept an input schema (add .input(...) to the task.all protectedProcedure and
apply the filters inside the resolver) so api.task.all.query({...}) becomes
valid — pick one approach and apply changes consistently across the CLI (run in
command.ts) and the backend task.all procedure.
In `@packages/cli/src/run-static.ts`:
- Around line 214-229: Middleware short-circuiting is ignored because cmd.run is
always executed after mw is invoked; change the flow so mw's next() is
responsible for running the command and returning its result (or
short-circuiting), rather than unconditionally calling cmd.run afterwards.
Concretely, when mw exists pass a next function that executes cmd.run({ options:
parsed.options, args: argsResult, ctx, signal }) and returns that result,
capture and return the value returned by mw (or by next when mw calls it); if no
mw exists call cmd.run directly. Ensure the next closure uses the existing
symbols (mw, next, cmd.run, parsed.options, argsResult, ctx, signal) and
preserves ctx mutation from the middleware before running the command.
- Around line 177-203: The positional parsing loop in run-static.ts builds
argsResult from positionalConfigs but never rejects extra tokens in
parsed.positionals, so commands with fixed arity silently ignore leftovers;
after the for...of loop (and after handling a variadic posConfig) check if
parsed.positionals.length > posIdx and, if so, throw a CLIError (e.g.
`Unexpected extra argument(s): ${parsed.positionals.slice(posIdx).join(' ')}`)
to reject unexpected positional arguments; use the existing variables
parsed.positionals, posIdx, and positionalConfigs to implement this check.
---
Nitpick comments:
In `@packages/cli-framework/src/help.ts`:
- Around line 31-39: The code uses a non-null assertion on
root.children.get(name)! when computing maxLen; avoid the assertion by
destructuring the map entries or performing the lookup once inline: iterate over
entries (the same Map entries used to build the array) and use the
already-available node (or assign const node = root.children.get(name) with a
guarded fallback) to compute aliasStr and length, updating the logic in the
maxLen calculation (function/variable names: maxLen, entries, root.children,
aliasStr) so no “!” is needed.
- Around line 63-68: The functions generateGroupHelp and generateCommandHelp
declare an unused _globals parameter, causing dead code or incomplete help
output; either remove the _globals parameter from both function signatures (and
any callers) to clarify intent, or update both functions to incorporate global
options into the rendered help (e.g., merge ProcessedBuilderConfig entries into
the options/flags section) so globals appear in subcommand/group help—locate the
functions by name and adjust their signatures and implementation consistently
across callers.
In `@packages/cli/src/commands/tasks/delete/command.ts`:
- Around line 11-18: The loop deletes tasks one-by-one which can leave partial
state on failure; first resolve all ids into tasks using api.task.bySlug.query
in parallel (Promise.all) and validate that every idOrSlug produced a task
(throw a single aggregated CLIError listing missing ids if any), then perform
deletions in parallel via api.task.delete.mutate (or wrap in a transaction if
your API supports a transactional endpoint) so either all delete promises are
awaited together and failures reported collectively; refer to the ids array,
api.task.bySlug.query, and api.task.delete.mutate to locate and change the
logic.
In `@packages/cli/src/commands/workspaces/delete/command.ts`:
- Line 1: Remove the unused `string` import from the top-level import statement
in command.ts (import { CLIError, command, positional, string } ...) since
`string` is never referenced; then verify the `device` option wiring: either
read the option value via `opts.options.device` where needed or ensure
middleware correctly populates `opts.ctx.deviceId` and remove the unused
`device` option if middleware is relied upon — adjust usage of
`opts.options.device`/`opts.ctx.deviceId` accordingly.
In `@packages/cli/src/run-static.ts`:
- Around line 45-49: The current UNAUTHORIZED checks use (error as any) which
loses type-safety; create a narrow type guard (e.g., isUnauthorizedError(err:
unknown): err is { code?: string; data?: { code?: string } }) and use it inside
the existing error instanceof Error branch to safely check (err.code ===
"UNAUTHORIZED" || err.data?.code === "UNAUTHORIZED") without any casts; update
the UNAUTHORIZED check in run-static.ts to call this guard (referencing the
error variable and the new isUnauthorizedError function) so you avoid using any
while preserving the same behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30282d72-fab8-41a6-bc0c-2f87ec6053a4
📒 Files selected for processing (39)
apps/desktop/package.jsonapps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsxapps/web/src/app/device/page.tsxpackages/auth/src/client.tspackages/auth/src/server.tspackages/cli-framework/src/cli.tspackages/cli-framework/src/command.tspackages/cli-framework/src/errors.tspackages/cli-framework/src/help.tspackages/cli-framework/src/index.tspackages/cli-framework/src/option.tspackages/cli-framework/src/output.tspackages/cli-framework/src/parser.tspackages/cli-framework/src/router.tspackages/cli/src/bin.tspackages/cli/src/commands/auth/login/command.tspackages/cli/src/commands/auth/meta.tspackages/cli/src/commands/auth/whoami/command.tspackages/cli/src/commands/devices/list/command.tspackages/cli/src/commands/devices/meta.tspackages/cli/src/commands/host/meta.tspackages/cli/src/commands/host/start/command.tspackages/cli/src/commands/host/stop/command.tspackages/cli/src/commands/index.tspackages/cli/src/commands/middleware.tspackages/cli/src/commands/org/list/command.tspackages/cli/src/commands/org/switch/command.tspackages/cli/src/commands/tasks/delete/command.tspackages/cli/src/commands/tasks/get/command.tspackages/cli/src/commands/tasks/list/command.tspackages/cli/src/commands/tasks/meta.tspackages/cli/src/commands/tasks/update/command.tspackages/cli/src/commands/workspaces/create/command.tspackages/cli/src/commands/workspaces/delete/command.tspackages/cli/src/commands/workspaces/list/command.tspackages/cli/src/commands/workspaces/meta.tspackages/cli/src/lib/api-client.tspackages/cli/src/lib/config.tspackages/cli/src/run-static.ts
✅ Files skipped from review due to trivial changes (14)
- packages/cli/src/commands/host/meta.ts
- packages/cli/src/commands/auth/meta.ts
- packages/cli/src/commands/devices/meta.ts
- packages/cli/src/commands/workspaces/meta.ts
- packages/cli/src/commands/tasks/meta.ts
- packages/cli/src/bin.ts
- packages/cli/src/commands/middleware.ts
- packages/cli/src/lib/api-client.ts
- packages/cli/src/commands/org/list/command.ts
- packages/cli/src/commands/auth/whoami/command.ts
- packages/cli/src/lib/config.ts
- apps/web/src/app/device/page.tsx
- packages/cli-framework/src/index.ts
- packages/cli-framework/src/cli.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/desktop/package.json
- packages/auth/src/client.ts
- packages/cli/src/commands/host/stop/command.ts
- apps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsx
- packages/cli/src/commands/host/start/command.ts
- packages/cli/src/commands/workspaces/list/command.ts
- packages/cli/src/commands/tasks/update/command.ts
- packages/cli/src/commands/devices/list/command.ts
- packages/cli/src/commands/workspaces/create/command.ts
- packages/cli/src/commands/org/switch/command.ts
- packages/cli/src/commands/index.ts
- packages/cli/src/commands/auth/login/command.ts
- packages/cli-framework/src/output.ts
- packages/cli-framework/src/option.ts
Comment on lines
+54
to
+56
| if (i > 1 && j > 1 && a[i - 1] === b[j - 2] && a[i - 2] === b[j - 1]) { | ||
| val = Math.min(val, (d[i - 2]?.[j - 2] ?? 0) + cost); | ||
| } |
Contributor
There was a problem hiding this comment.
Incorrect transposition cost in Damerau-Levenshtein algorithm.
The transposition operation should always have a cost of 1, but here it uses cost which is 0 when the characters at positions i-1 and j-1 match. This can produce incorrect edit distances.
🐛 Proposed fix
if (i > 1 && j > 1 && a[i - 1] === b[j - 2] && a[i - 2] === b[j - 1]) {
- val = Math.min(val, (d[i - 2]?.[j - 2] ?? 0) + cost);
+ val = Math.min(val, (d[i - 2]?.[j - 2] ?? 0) + 1);
}📝 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
| if (i > 1 && j > 1 && a[i - 1] === b[j - 2] && a[i - 2] === b[j - 1]) { | |
| val = Math.min(val, (d[i - 2]?.[j - 2] ?? 0) + cost); | |
| } | |
| if (i > 1 && j > 1 && a[i - 1] === b[j - 2] && a[i - 2] === b[j - 1]) { | |
| val = Math.min(val, (d[i - 2]?.[j - 2] ?? 0) + 1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-framework/src/errors.ts` around lines 54 - 56, Transposition is
using the variable cost (which can be 0 when chars match) instead of a fixed
cost of 1, producing incorrect Damerau-Levenshtein distances; in the block that
checks transposition (the if with i, j, a[i-1] === b[j-2] && a[i-2] === b[j-1])
update the update to use +1 (a fixed transposition cost) instead of +cost—i.e.,
replace the term (d[i - 2]?.[j - 2] ?? 0) + cost with (d[i - 2]?.[j - 2] ?? 0) +
1 so variables a, b, d, val, cost remain otherwise unchanged.
Comment on lines
+61
to
+62
| for (let i = 0; i < args.length; i++) { | ||
| const arg = args[i]!; |
Contributor
There was a problem hiding this comment.
Address non-null assertion flagged by linter.
The pipeline flags args[i]! as a forbidden non-null assertion. While the loop bound guarantees args[i] exists, you can satisfy the linter by extracting the value with a guard.
🔧 Proposed fix
for (let i = 0; i < args.length; i++) {
- const arg = args[i]!;
+ const arg = args[i];
+ if (arg === undefined) continue;📝 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
| for (let i = 0; i < args.length; i++) { | |
| const arg = args[i]!; | |
| for (let i = 0; i < args.length; i++) { | |
| const arg = args[i]; | |
| if (arg === undefined) continue; |
🧰 Tools
🪛 GitHub Actions: CI
[warning] 62-62: lint/style/noNonNullAssertion: Forbidden non-null assertion. (line contains: "args[i]!")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-framework/src/parser.ts` around lines 61 - 62, Replace the
forbidden non-null assertion on args[i]! by extracting with a guard or iterating
safely: in parser.ts change the loop that uses for (let i = 0; i < args.length;
i++) { const arg = args[i]!; } to either use a for (const arg of args) { ... }
or keep the index loop but use const arg = args[i]; if (arg === undefined)
continue; so no non-null assertion is required (refer to the args variable and
the loop body where arg is used).
Comment on lines
+132
to
+141
| // String/number: consume next arg as value | ||
| const nextArg = args[i + 1]; | ||
| if (nextArg === undefined || nextArg.startsWith("-")) { | ||
| throw new CLIError( | ||
| `Option ${arg} requires a value`, | ||
| entry[1].enumVals | ||
| ? `Valid values: ${entry[1].enumVals.join(", ")}` | ||
| : undefined, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Negative numbers are rejected as option values.
When parsing --flag value, if nextArg starts with -, it's rejected. This prevents passing negative numbers like --offset -5. Consider checking if the next arg matches a known flag rather than rejecting all - prefixed values.
🔧 Proposed fix
// String/number: consume next arg as value
const nextArg = args[i + 1];
- if (nextArg === undefined || nextArg.startsWith("-")) {
+ if (nextArg === undefined || (nextArg.startsWith("-") && optionsByFlag.has(nextArg))) {
throw new CLIError(
`Option ${arg} requires a value`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-framework/src/parser.ts` around lines 132 - 141, The parser
currently rejects any nextArg that starts with "-" when parsing option values
(see variables args, i, nextArg and the CLIError thrown inside parser.ts), which
prevents negative numbers like `--offset -5`; change the check so that instead
of using nextArg.startsWith("-") you verify whether nextArg is a known flag
token (e.g., matches an entry in the parser's flag/option map or a flag pattern)
and only treat it as missing when it is a recognized flag; update the validation
around the entry handling (entry[1], enumVals) to allow `-5` as a value while
still throwing CLIError when the next token is actually another defined flag.
Comment on lines
+155
to
+161
| if (config.envVar && process.env[config.envVar] !== undefined) { | ||
| options[key] = coerce( | ||
| config, | ||
| process.env[config.envVar]!, | ||
| `$${config.envVar}`, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Address non-null assertion flagged by linter.
The pipeline flags process.env[config.envVar]! as a forbidden non-null assertion. The condition guarantees it's defined, but you can restructure to satisfy the linter.
🔧 Proposed fix
for (const [key, config] of Object.entries(allConfigs)) {
if (options[key] !== undefined) continue;
- if (config.envVar && process.env[config.envVar] !== undefined) {
+ const envValue = config.envVar ? process.env[config.envVar] : undefined;
+ if (envValue !== undefined) {
options[key] = coerce(
config,
- process.env[config.envVar]!,
+ envValue,
`$${config.envVar}`,
);
}
}📝 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
| if (config.envVar && process.env[config.envVar] !== undefined) { | |
| options[key] = coerce( | |
| config, | |
| process.env[config.envVar]!, | |
| `$${config.envVar}`, | |
| ); | |
| } | |
| const envValue = config.envVar ? process.env[config.envVar] : undefined; | |
| if (envValue !== undefined) { | |
| options[key] = coerce( | |
| config, | |
| envValue, | |
| `$${config.envVar}`, | |
| ); | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[warning] 158-158: lint/style/noNonNullAssertion: Forbidden non-null assertion. (line contains: "process.env[config.envVar]!")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-framework/src/parser.ts` around lines 155 - 161, The linter
flags the non-null assertion on process.env[config.envVar] inside the parser
logic; to fix, read the environment value into a local const (e.g., const envVal
= process.env[config.envVar]), check it for undefined with the existing if
(config.envVar && envVal !== undefined) condition, and then pass envVal (not a
non-null assertion) into coerce when setting options[key]; update the block
around the coerce(...) call (where config, key, and coerce are used) to use the
local variable instead.
Comment on lines
+98
to
+99
| for (; i < args.length; i++) { | ||
| const segment = args[i]!; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -path "**/packages/cli-framework/src/router.ts" | head -20Repository: superset-sh/superset
Length of output: 102
🏁 Script executed:
wc -l ./packages/cli-framework/src/router.tsRepository: superset-sh/superset
Length of output: 106
🏁 Script executed:
cat -n ./packages/cli-framework/src/router.ts | sed -n '90,110p'Repository: superset-sh/superset
Length of output: 753
🏁 Script executed:
cat -n ./packages/cli-framework/src/router.ts | sed -n '215,250p'Repository: superset-sh/superset
Length of output: 1189
🏁 Script executed:
cat -n ./packages/cli-framework/src/router.ts | sed -n '1,50p'Repository: superset-sh/superset
Length of output: 1827
🏁 Script executed:
find . -name "eslint*" -o -name ".eslintrc*" | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
cat -n ./packages/cli-framework/src/router.ts | sed -n '195,215p'Repository: superset-sh/superset
Length of output: 810
🏁 Script executed:
cat -n ./packages/cli-framework/src/router.ts | sed -n '98,130p'Repository: superset-sh/superset
Length of output: 1180
🏁 Script executed:
# Check if there's any linting configuration in the package
ls -la ./packages/cli-framework/ | grep -E "(eslint|tsconfig|package.json)"Repository: superset-sh/superset
Length of output: 188
🏁 Script executed:
cat -n ./packages/cli-framework/src/router.ts | sed -n '1,20p'Repository: superset-sh/superset
Length of output: 776
Remove unnecessary non-null assertions in router.ts
Line 99 is protected by the loop bounds (i < args.length), and lines 226/236/244 have preceding checks or loop bounds that guarantee the values exist. Refactor to use a local child variable reference in the group and command node creation loops to eliminate the assertions and improve clarity.
Suggested fix
- const segment = args[i]!;
+ const segment = args[i];
// Create group nodes
for (const group of groups) {
let node = root;
for (const segment of group.path) {
- if (!node.children.has(segment)) {
- node.children.set(segment, {
+ let child = node.children.get(segment);
+ if (!child) {
+ child = {
name: segment,
children: new Map(),
hasCommand: false,
- });
+ };
+ node.children.set(segment, child);
}
- node = node.children.get(segment)!;
+ node = child;
}
node.description = group.description;
node.aliases = group.aliases;
}
// Create command nodes
for (const entry of commands) {
let node = root;
- for (let i = 0; i < entry.path.length; i++) {
- const segment = entry.path[i]!;
- if (!node.children.has(segment)) {
- node.children.set(segment, {
+ for (const segment of entry.path) {
+ let child = node.children.get(segment);
+ if (!child) {
+ child = {
name: segment,
children: new Map(),
hasCommand: false,
- });
+ };
+ node.children.set(segment, child);
}
- node = node.children.get(segment)!;
+ node = child;
}🧰 Tools
🪛 GitHub Actions: CI
[warning] 99-99: lint/style/noNonNullAssertion: Forbidden non-null assertion. (line contains: "args[i]!")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-framework/src/router.ts` around lines 98 - 99, Remove
unnecessary non-null assertions in router.ts by replacing direct uses of
asserted expressions with a local non-null-typed variable where loop bounds or
prior checks already guarantee existence: for the segment loop replace uses of
args[i]! with const segment = args[i]; and similarly in the group and command
node creation loops introduce a local const child = node.children.get(key) (or
equivalent) after the existence check (or where i < args.length ensures
presence) and use child instead of child! or other non-null assertions on lines
referenced (99, ~226, ~236, ~244); this removes the "!" assertions and improves
clarity while preserving runtime guarantees.
Comment on lines
+276
to
+284
| let pathSoFar = ""; | ||
| for (const segment of commandPath) { | ||
| pathSoFar = pathSoFar ? `${pathSoFar}/${segment}` : segment; | ||
| const mw = middlewares.get(pathSoFar) ?? middlewares.get(segment); | ||
| if (mw === skip) { | ||
| activeMiddleware = null; | ||
| } else if (mw) { | ||
| activeMiddleware = mw as MiddlewareFn; | ||
| } |
Contributor
There was a problem hiding this comment.
Resolve static middleware by full path only.
middlewares.get(segment) lets middleware from one branch leak into any other branch that reuses the same segment name. This PR already has repeated leaves like list, so static routing can pick the wrong middleware as soon as one branch adds one.
Suggested fix
- const mw = middlewares.get(pathSoFar) ?? middlewares.get(segment);
+ const mw = middlewares.get(pathSoFar);📝 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
| let pathSoFar = ""; | |
| for (const segment of commandPath) { | |
| pathSoFar = pathSoFar ? `${pathSoFar}/${segment}` : segment; | |
| const mw = middlewares.get(pathSoFar) ?? middlewares.get(segment); | |
| if (mw === skip) { | |
| activeMiddleware = null; | |
| } else if (mw) { | |
| activeMiddleware = mw as MiddlewareFn; | |
| } | |
| let pathSoFar = ""; | |
| for (const segment of commandPath) { | |
| pathSoFar = pathSoFar ? `${pathSoFar}/${segment}` : segment; | |
| const mw = middlewares.get(pathSoFar); | |
| if (mw === skip) { | |
| activeMiddleware = null; | |
| } else if (mw) { | |
| activeMiddleware = mw as MiddlewareFn; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-framework/src/router.ts` around lines 276 - 284, The loop
resolving middleware incorrectly falls back to middlewares.get(segment) which
allows middleware defined for a segment name in one branch to leak into other
branches; change the resolution in the loop that builds pathSoFar (the code
using commandPath, segment, pathSoFar, middlewares, skip, and activeMiddleware)
to look up middleware only by the full path key (middlewares.get(pathSoFar)) and
remove the fallback to middlewares.get(segment), preserving the existing
handling for skip and setting activeMiddleware accordingly.
Comment on lines
+5
to
+14
| description: "Get a task by ID or slug", | ||
| args: [positional("idOrSlug").required().desc("Task ID or slug")], | ||
| run: async (opts) => { | ||
| const api = opts.ctx.api as ApiClient; | ||
| const slug = opts.args.idOrSlug as string; | ||
| const task = await api.task.bySlug.query(slug); | ||
|
|
||
| if (!task) { | ||
| throw new CLIError(`Task not found: ${slug}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Description promises ID support but only slug is queried.
The description says "Get a task by ID or slug" and the argument is named idOrSlug, but the implementation only calls api.task.bySlug.query(). If bySlug doesn't resolve UUIDs, users passing task IDs will receive "Task not found" errors.
Either update the description to "Get a task by slug", or add fallback logic to query by ID when slug lookup fails (similar to the pattern suggested in the delete command's comment).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/tasks/get/command.ts` around lines 5 - 14, The
command claims to accept "ID or slug" but only calls
api.task.bySlug.query(slug), so add a fallback to try ID lookup when slug lookup
returns no task: in the run handler after const task = await
api.task.bySlug.query(slug), if task is falsy attempt an ID lookup via the
task-by-id endpoint (e.g., api.task.byId.query(slug) or whichever method exists)
and use that result; if still not found throw the same CLIError(`Task not found:
${slug}`). Keep the original description and argument name, and ensure you
reference the same run function and api.task.bySlug.query/api.task.byId.query
symbols when making the change.
Comment on lines
+31
to
+37
| run: async (opts) => { | ||
| const api = opts.ctx.api as ApiClient; | ||
| const result = await api.task.all.query(); | ||
| return result.map((r) => ({ | ||
| ...r.task, | ||
| assignee: r.assignee?.name ?? "—", | ||
| })); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the api.task.all.query signature to confirm it accepts filter parameters
ast-grep --pattern $'all: $_($$$).query($$$)'
rg -n "task\.all" --type ts -C 5Repository: superset-sh/superset
Length of output: 924
🏁 Script executed:
cat -n packages/cli/src/commands/tasks/list/command.tsRepository: superset-sh/superset
Length of output: 1429
🏁 Script executed:
# Search for the api.task.all definition to understand the query signature
rg -n "\.all\s*=" --type ts -B 2 -A 10 packages/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's any usage of opts.options in this file
rg -n "opts\.options" packages/cli/src/commands/tasks/list/command.tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Find ApiClient type definition
rg -n "ApiClient" --type ts packages/cli/src/lib/api-client.ts -A 20Repository: superset-sh/superset
Length of output: 509
🏁 Script executed:
# Search for task.all definition
rg -n "task\s*:" --type ts packages/ -A 5 | head -50Repository: superset-sh/superset
Length of output: 3249
🏁 Script executed:
# Look for the full ApiClient interface/type
fd "api-client" --type ts packages/Repository: superset-sh/superset
Length of output: 233
🏁 Script executed:
# Search for AppRouter definition
rg -n "AppRouter" --type ts packages/ -A 3 | head -40Repository: superset-sh/superset
Length of output: 2636
🏁 Script executed:
# Find router definition files
fd "router" -type f packages/ | grep -E "\.(ts|tsx)$" | head -20Repository: superset-sh/superset
Length of output: 234
🏁 Script executed:
# Look for task router specifically
rg -n "task" packages/ -l --type ts | grep -i route | head -10Repository: superset-sh/superset
Length of output: 277
🏁 Script executed:
cat -n packages/trpc/src/router/task/task.tsRepository: superset-sh/superset
Length of output: 13401
🏁 Script executed:
cat -n packages/trpc/src/router/task/schema.tsRepository: superset-sh/superset
Length of output: 1760
The all procedure does not accept input parameters—the proposed fix will not work.
The CLI defines filter options (status, priority, assigneeMe, creatorMe, search, limit, offset) but they are never used. However, the backend task.all procedure is defined as protectedProcedure.query(...) without an .input() schema, so it does not accept any parameters. The proposed fix to pass these options to api.task.all.query({...}) will fail at runtime.
To resolve this issue, either:
- Remove the unused filter options from the CLI command, or
- Modify the backend
task.allprocedure to accept and apply these filters via an.input()schema.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/tasks/list/command.ts` around lines 31 - 37, The
CLI is passing filter flags to api.task.all.query but the backend task.all is
defined as protectedProcedure.query(...) with no .input(), so passing params
will fail; either remove the unused flags from the CLI command (the run handler
in command.ts around api.task.all.query and the option definitions) or update
the backend task router's task.all procedure to accept an input schema (add
.input(...) to the task.all protectedProcedure and apply the filters inside the
resolver) so api.task.all.query({...}) becomes valid — pick one approach and
apply changes consistently across the CLI (run in command.ts) and the backend
task.all procedure.
Comment on lines
+177
to
+203
| // Positional args | ||
| const argsResult: Record<string, unknown> = {}; | ||
| if (cmd.args) { | ||
| const positionalConfigs = (cmd.args as GenericBuilderInternals[]).map( | ||
| (b) => b._.config, | ||
| ); | ||
| let posIdx = 0; | ||
| for (const posConfig of positionalConfigs) { | ||
| const argName = posConfig.name ?? `arg${posIdx}`; | ||
| if (posConfig.isVariadic) { | ||
| argsResult[argName] = parsed.positionals.slice(posIdx); | ||
| if ( | ||
| posConfig.isRequired && | ||
| (argsResult[argName] as string[]).length === 0 | ||
| ) { | ||
| throw new CLIError(`Missing required argument: <${argName}...>`); | ||
| } | ||
| break; | ||
| } | ||
| const value = parsed.positionals[posIdx]; | ||
| if (posConfig.isRequired && value === undefined) { | ||
| throw new CLIError(`Missing required argument: <${argName}>`); | ||
| } | ||
| argsResult[argName] = value; | ||
| posIdx++; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Reject unexpected positional arguments.
This block validates missing required args, but it never errors on leftovers in parsed.positionals. Commands with fixed arity will silently ignore extra tokens instead of failing fast.
Suggested fix
- const argsResult: Record<string, unknown> = {};
- if (cmd.args) {
- const positionalConfigs = (cmd.args as GenericBuilderInternals[]).map(
- (b) => b._.config,
- );
+ const argsResult: Record<string, unknown> = {};
+ const positionalConfigs = cmd.args
+ ? (cmd.args as GenericBuilderInternals[]).map((b) => b._.config)
+ : [];
+
+ const lastPositional = positionalConfigs[positionalConfigs.length - 1];
+ if (
+ !lastPositional?.isVariadic &&
+ parsed.positionals.length > positionalConfigs.length
+ ) {
+ throw new CLIError(
+ `Unexpected argument: ${parsed.positionals[positionalConfigs.length]}`,
+ );
+ }
+
+ if (positionalConfigs.length > 0) {
let posIdx = 0;
for (const posConfig of positionalConfigs) {
const argName = posConfig.name ?? `arg${posIdx}`;📝 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
| // Positional args | |
| const argsResult: Record<string, unknown> = {}; | |
| if (cmd.args) { | |
| const positionalConfigs = (cmd.args as GenericBuilderInternals[]).map( | |
| (b) => b._.config, | |
| ); | |
| let posIdx = 0; | |
| for (const posConfig of positionalConfigs) { | |
| const argName = posConfig.name ?? `arg${posIdx}`; | |
| if (posConfig.isVariadic) { | |
| argsResult[argName] = parsed.positionals.slice(posIdx); | |
| if ( | |
| posConfig.isRequired && | |
| (argsResult[argName] as string[]).length === 0 | |
| ) { | |
| throw new CLIError(`Missing required argument: <${argName}...>`); | |
| } | |
| break; | |
| } | |
| const value = parsed.positionals[posIdx]; | |
| if (posConfig.isRequired && value === undefined) { | |
| throw new CLIError(`Missing required argument: <${argName}>`); | |
| } | |
| argsResult[argName] = value; | |
| posIdx++; | |
| } | |
| } | |
| // Positional args | |
| const argsResult: Record<string, unknown> = {}; | |
| const positionalConfigs = cmd.args | |
| ? (cmd.args as GenericBuilderInternals[]).map((b) => b._.config) | |
| : []; | |
| const lastPositional = positionalConfigs[positionalConfigs.length - 1]; | |
| if ( | |
| !lastPositional?.isVariadic && | |
| parsed.positionals.length > positionalConfigs.length | |
| ) { | |
| throw new CLIError( | |
| `Unexpected argument: ${parsed.positionals[positionalConfigs.length]}`, | |
| ); | |
| } | |
| if (positionalConfigs.length > 0) { | |
| let posIdx = 0; | |
| for (const posConfig of positionalConfigs) { | |
| const argName = posConfig.name ?? `arg${posIdx}`; | |
| if (posConfig.isVariadic) { | |
| argsResult[argName] = parsed.positionals.slice(posIdx); | |
| if ( | |
| posConfig.isRequired && | |
| (argsResult[argName] as string[]).length === 0 | |
| ) { | |
| throw new CLIError(`Missing required argument: <${argName}...>`); | |
| } | |
| break; | |
| } | |
| const value = parsed.positionals[posIdx]; | |
| if (posConfig.isRequired && value === undefined) { | |
| throw new CLIError(`Missing required argument: <${argName}>`); | |
| } | |
| argsResult[argName] = value; | |
| posIdx++; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/run-static.ts` around lines 177 - 203, The positional
parsing loop in run-static.ts builds argsResult from positionalConfigs but never
rejects extra tokens in parsed.positionals, so commands with fixed arity
silently ignore leftovers; after the for...of loop (and after handling a
variadic posConfig) check if parsed.positionals.length > posIdx and, if so,
throw a CLIError (e.g. `Unexpected extra argument(s):
${parsed.positionals.slice(posIdx).join(' ')}`) to reject unexpected positional
arguments; use the existing variables parsed.positionals, posIdx, and
positionalConfigs to implement this check.
Comment on lines
+214
to
+229
| if (mw) { | ||
| await mw({ | ||
| options: parsed.options, | ||
| next: async (params) => { | ||
| ctx = params.ctx; | ||
| return undefined; | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const result = await cmd.run({ | ||
| options: parsed.options as any, | ||
| args: argsResult as any, | ||
| ctx, | ||
| signal, | ||
| }); |
Contributor
There was a problem hiding this comment.
Middleware opt-out is ignored in static mode.
next() never runs the command here, and cmd.run() still executes unconditionally afterward. That makes middleware short-circuiting impossible and can run guarded commands after middleware tried to stop them.
Suggested fix
- if (mw) {
- await mw({
- options: parsed.options,
- next: async (params) => {
- ctx = params.ctx;
- return undefined;
- },
- });
- }
-
- const result = await cmd.run({
- options: parsed.options as any,
- args: argsResult as any,
- ctx,
- signal,
- });
+ let result: unknown;
+ if (mw) {
+ let proceeded = false;
+ result = await mw({
+ options: parsed.options,
+ next: async (params) => {
+ proceeded = true;
+ ctx = params.ctx;
+ return cmd.run({
+ options: parsed.options as any,
+ args: argsResult as any,
+ ctx,
+ signal,
+ });
+ },
+ });
+ if (!proceeded) return;
+ } else {
+ result = await cmd.run({
+ options: parsed.options as any,
+ args: argsResult as any,
+ ctx,
+ signal,
+ });
+ }📝 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
| if (mw) { | |
| await mw({ | |
| options: parsed.options, | |
| next: async (params) => { | |
| ctx = params.ctx; | |
| return undefined; | |
| }, | |
| }); | |
| } | |
| const result = await cmd.run({ | |
| options: parsed.options as any, | |
| args: argsResult as any, | |
| ctx, | |
| signal, | |
| }); | |
| let result: unknown; | |
| if (mw) { | |
| let proceeded = false; | |
| result = await mw({ | |
| options: parsed.options, | |
| next: async (params) => { | |
| proceeded = true; | |
| ctx = params.ctx; | |
| return cmd.run({ | |
| options: parsed.options as any, | |
| args: argsResult as any, | |
| ctx, | |
| signal, | |
| }); | |
| }, | |
| }); | |
| if (!proceeded) return; | |
| } else { | |
| result = await cmd.run({ | |
| options: parsed.options as any, | |
| args: argsResult as any, | |
| ctx, | |
| signal, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/run-static.ts` around lines 214 - 229, Middleware
short-circuiting is ignored because cmd.run is always executed after mw is
invoked; change the flow so mw's next() is responsible for running the command
and returning its result (or short-circuiting), rather than unconditionally
calling cmd.run afterwards. Concretely, when mw exists pass a next function that
executes cmd.run({ options: parsed.options, args: argsResult, ctx, signal }) and
returns that result, capture and return the value returned by mw (or by next
when mw calls it); if no mw exists call cmd.run directly. Ensure the next
closure uses the existing symbols (mw, next, cmd.run, parsed.options,
argsResult, ctx, signal) and preserves ctx mutation from the middleware before
running the command.
- Add biome overrides for cli-framework/cli packages (noExplicitAny, noNonNullAssertion) - Fix typecheck error in output.ts (number | undefined → fallback to 0) - Wrap /device page useSearchParams in Suspense boundary (fixes SSG build) - Replace `as any` casts in device page with proper types - Use biome-ignore comments for Better Auth inference gaps in desktop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
biome.jsonc (1)
32-44: Override for CLI packages is reasonable but consider documenting the rationale.Disabling
noExplicitAnyandnoNonNullAssertionfor CLI framework code is a common pattern—CLI frameworks inherently deal with dynamic command registration and option parsing where strict typing is impractical. Since this is scoped narrowly topackages/cli-framework/**andpackages/cli/**, it won't affect the rest of the codebase.Consider adding a comment above this override block explaining why these rules are relaxed, so future maintainers understand the intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@biome.jsonc` around lines 32 - 44, Add a short inline comment above the override block that targets "packages/cli-framework/**" and "packages/cli/**" explaining why the linter rules "noExplicitAny" and "noNonNullAssertion" are disabled (e.g., dynamic command registration and option parsing in CLI code makes strict typing impractical), so future maintainers understand the narrow scope and rationale for relaxing those rules.apps/web/src/app/device/page.tsx (1)
144-157: Consider distinguishing denial errors from successful denials.The catch block masks all errors and shows "Denied" regardless of whether the server call succeeded. If the deny request fails (network issue), the device code may remain valid server-side while the user believes it was denied.
This might be intentional UX to avoid confusing the user, but consider at minimum logging the error for debugging.
💡 Optional: surface denial errors
const handleDeny = async () => { const code = userCode.replace(/[-\s]/g, "").toUpperCase(); try { - await fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`, { + const res = await fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`, { method: "POST", headers: { "Content-Type": "application/json" }, credentials: "include", body: JSON.stringify({ userCode: code }), }); + if (!res.ok) { + console.warn("Deny request failed:", res.status); + } setStatus("denied"); } catch { + console.warn("Deny request failed"); setStatus("denied"); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/device/page.tsx` around lines 144 - 157, The handleDeny function currently catches all errors and always calls setStatus("denied"), which hides failures of the POST to /api/auth/device/deny; change it to differentiate success vs failure by checking the fetch response (e.g., response.ok) and only setStatus("denied") on success, otherwise set a distinct error state (e.g., setStatus("deny_error")) or rethrow; also log or capture the caught error (console.error or a logger) inside the catch so denial failures are visible for debugging while preserving the intended UX.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/device/page.tsx`:
- Around line 71-99: The authClient.organization.list() call inside the
useEffect has no error handling; wrap the promise with a .catch (or use
try/catch if converting to async) to handle failures: on error call
processLogger/error logging (or console.error), set an error status via
setStatus (e.g., "error" or revert to "idle"), ensure setOrgs([]) or keep prior
state and clear/setSelectedOrgId to a safe default (""), and optionally surface
a user-facing message; target the authClient.organization.list() call and the
handlers that call setOrgs and setSelectedOrgId within the useEffect so the
component doesn't hang on failed network/auth requests.
- Around line 106-142: The handleApprove function currently calls
authClient.organization.setActive without handling failures; change
handleApprove so that the call to authClient.organization.setActive({
organizationId: selectedOrgId }) is wrapped in a try/catch (or checks its
result) and if it throws or fails, setError with a descriptive message and
setStatus("error") and return early so you do not proceed to the API POST;
ensure the rest of the approval logic (the fetch to /api/auth/device/approve and
subsequent setStatus("approved")) only runs when setActive succeeds.
In `@packages/cli-framework/src/output.ts`:
- Around line 70-71: Validate the headers array length against the computed
columns before rendering: in the table-rendering function that accepts headers,
columns and maxColWidth (referencing the headers and columns variables), add a
check after columns is determined to ensure headers == null or headers.length
=== columns.length; if not, either throw a clear error or normalize by
padding/truncating the headers array to columns.length (and log a warning). This
prevents silent misalignment when headers length diverges from columns and keeps
downstream width calculations (which use headers, columns, and maxColWidth)
consistent.
- Around line 38-44: The type-narrowing helper is using unsafe `as any`
assertions; update isResultWithMessage to cast `result` to `Record<string,
unknown>` and use bracket access to check the message property (e.g., typeof
(result as Record<string, unknown>)['message'] === 'string'), and apply the same
change to the other occurrence mentioned (the other type-narrowing check around
line ~60) so no `as any` remains and property access stays type-safe.
- Around line 14-16: formatOutput and extractIds call JSON.stringify on unknown
values which can return undefined for top-level undefined/function/symbol,
violating the declared string return type; update both functions to capture
JSON.stringify's result and guarantee a string by returning a safe fallback
(e.g., 'null' or 'undefined' or an explicit diagnostic string) when
JSON.stringify returns undefined, or alternatively coerce the value before
stringify (e.g., replace unsupported top-level values with null) so the
functions always return a string; locate usages in formatOutput and extractIds
and apply the same pattern to any other JSON.stringify calls in this module to
ensure type safety.
---
Nitpick comments:
In `@apps/web/src/app/device/page.tsx`:
- Around line 144-157: The handleDeny function currently catches all errors and
always calls setStatus("denied"), which hides failures of the POST to
/api/auth/device/deny; change it to differentiate success vs failure by checking
the fetch response (e.g., response.ok) and only setStatus("denied") on success,
otherwise set a distinct error state (e.g., setStatus("deny_error")) or rethrow;
also log or capture the caught error (console.error or a logger) inside the
catch so denial failures are visible for debugging while preserving the intended
UX.
In `@biome.jsonc`:
- Around line 32-44: Add a short inline comment above the override block that
targets "packages/cli-framework/**" and "packages/cli/**" explaining why the
linter rules "noExplicitAny" and "noNonNullAssertion" are disabled (e.g.,
dynamic command registration and option parsing in CLI code makes strict typing
impractical), so future maintainers understand the narrow scope and rationale
for relaxing those rules.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26fba3a3-1286-4bda-b540-4124537107a9
📒 Files selected for processing (5)
apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/BillingOverview.tsxapps/desktop/src/renderer/routes/_authenticated/settings/organization/components/OrganizationSettings/OrganizationSettings.tsxapps/web/src/app/device/page.tsxbiome.jsoncpackages/cli-framework/src/output.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/routes/_authenticated/settings/organization/components/OrganizationSettings/OrganizationSettings.tsx
- apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/BillingOverview.tsx
Comment on lines
+106
to
+142
| const handleApprove = async () => { | ||
| const code = userCode.replace(/[-\s]/g, "").toUpperCase(); | ||
|
|
||
| // Set the active org before approving so the session token | ||
| // carries the right org context | ||
| if (selectedOrgId) { | ||
| await authClient.organization.setActive({ | ||
| organizationId: selectedOrgId, | ||
| }); | ||
| } | ||
|
|
||
| try { | ||
| const res = await fetch( | ||
| `${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/approve`, | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| credentials: "include", | ||
| body: JSON.stringify({ userCode: code }), | ||
| }, | ||
| ); | ||
|
|
||
| if (!res.ok) { | ||
| const data = await res.json().catch(() => ({})); | ||
| setError( | ||
| (data as Record<string, unknown>).message ?? "Failed to approve", | ||
| ); | ||
| setStatus("error"); | ||
| return; | ||
| } | ||
|
|
||
| setStatus("approved"); | ||
| } catch { | ||
| setError("Failed to approve"); | ||
| setStatus("error"); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Handle potential failure of setActive() before approval.
If authClient.organization.setActive() fails (network error, invalid org ID), the error goes unhandled and handleApprove proceeds to approve the device—potentially with the wrong or no org context.
🛡️ Proposed fix to handle setActive errors
const handleApprove = async () => {
const code = userCode.replace(/[-\s]/g, "").toUpperCase();
// Set the active org before approving so the session token
// carries the right org context
if (selectedOrgId) {
- await authClient.organization.setActive({
+ const { error: setActiveError } = await authClient.organization.setActive({
organizationId: selectedOrgId,
});
+ if (setActiveError) {
+ setError("Failed to set organization");
+ setStatus("error");
+ return;
+ }
}
try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/device/page.tsx` around lines 106 - 142, The handleApprove
function currently calls authClient.organization.setActive without handling
failures; change handleApprove so that the call to
authClient.organization.setActive({ organizationId: selectedOrgId }) is wrapped
in a try/catch (or checks its result) and if it throws or fails, setError with a
descriptive message and setStatus("error") and return early so you do not
proceed to the API POST; ensure the rest of the approval logic (the fetch to
/api/auth/device/approve and subsequent setStatus("approved")) only runs when
setActive succeeds.
Comment on lines
+14
to
+16
| if (flags.json) { | ||
| return JSON.stringify(data, null, 2); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, what does JSON.stringify return for top-level undefined, function, and symbol values?
💡 Result:
For top-level undefined, function, and symbol values, JSON.stringify returns undefined. This is per the official MDN documentation and ECMAScript specification. Examples: JSON.stringify(undefined) === undefined JSON.stringify(function{}) === undefined JSON.stringify(Symbol('')) === undefined Note that when these values appear in objects or arrays (not top-level), behavior differs: in objects they are omitted (property skipped), in arrays they become null. But for top-level primitives like these, the method returns undefined.
Citations:
🏁 Script executed:
find . -path "*/packages/cli-framework/src/output.ts" -type fRepository: superset-sh/superset
Length of output: 102
🏁 Script executed:
cat -n packages/cli-framework/src/output.tsRepository: superset-sh/superset
Length of output: 3532
JSON.stringify can return undefined for unsupported top-level values, violating the declared string return type.
Lines 15, 31, 54, and 63 use JSON.stringify with values typed as unknown. Per the ECMAScript specification, JSON.stringify returns undefined (not a string) when given top-level undefined, function, or symbol values. Since both formatOutput and extractIds declare a return type of string, this creates a type safety violation.
Proposed fix
+function stringifySafe(value: unknown, pretty = false): string {
+ const json = JSON.stringify(value, null, pretty ? 2 : 0);
+ return json ?? String(value);
+}
+
export function formatOutput(
@@
if (flags.json) {
- return JSON.stringify(data, null, 2);
+ return stringifySafe(data, true);
}
@@
// Fallback: JSON
- return JSON.stringify(data, null, 2);
+ return stringifySafe(data, true);
}
@@
if (typeof item === "object" && item !== null && "id" in item)
return String(item.id);
- return JSON.stringify(item);
+ return stringifySafe(item);
})
.join("\n");
@@
- return JSON.stringify(data);
+ return stringifySafe(data);
}📝 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
| if (flags.json) { | |
| return JSON.stringify(data, null, 2); | |
| } | |
| if (flags.json) { | |
| return stringifySafe(data, true); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-framework/src/output.ts` around lines 14 - 16, formatOutput and
extractIds call JSON.stringify on unknown values which can return undefined for
top-level undefined/function/symbol, violating the declared string return type;
update both functions to capture JSON.stringify's result and guarantee a string
by returning a safe fallback (e.g., 'null' or 'undefined' or an explicit
diagnostic string) when JSON.stringify returns undefined, or alternatively
coerce the value before stringify (e.g., replace unsupported top-level values
with null) so the functions always return a string; locate usages in
formatOutput and extractIds and apply the same pattern to any other
JSON.stringify calls in this module to ensure type safety.
Comment on lines
+38
to
+44
| function isResultWithMessage(result: unknown): result is { message: string } { | ||
| return ( | ||
| typeof result === "object" && | ||
| result !== null && | ||
| "message" in result && | ||
| typeof (result as any).message === "string" | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining `as any` usage in TS files
rg -nP '\bas\s+any\b' --type=ts --type=tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# First, check if the file exists
if [ -f "packages/cli-framework/src/output.ts" ]; then
echo "✓ File found"
# Read the file to check lines 38-44 and around line 60
echo "=== Lines 35-65 of output.ts ==="
sed -n '35,65p' packages/cli-framework/src/output.ts | cat -n
else
echo "✗ File not found"
find . -name "output.ts" -type f 2>/dev/null | head -10
fiRepository: superset-sh/superset
Length of output: 1071
🏁 Script executed:
#!/bin/bash
# Search for 'as any' in TypeScript files (correct syntax)
echo "=== Searching for 'as any' in .ts files ==="
rg -n 'as\s+any' -t ts .Repository: superset-sh/superset
Length of output: 3082
Remove as any assertions in type narrowing.
Lines 43 and 60 use as any, which bypasses type safety. Replace with Record<string, unknown> for safe property access.
Proposed fix
function isResultWithMessage(result: unknown): result is { message: string } {
+ const rec = typeof result === "object" && result !== null
+ ? (result as Record<string, unknown>)
+ : undefined;
return (
- typeof result === "object" &&
- result !== null &&
- "message" in result &&
- typeof (result as any).message === "string"
+ !!rec &&
+ "message" in rec &&
+ typeof rec.message === "string"
);
}
@@
if (typeof data === "object" && data !== null && "id" in data) {
- return String((data as any).id);
+ return String((data as Record<string, unknown>).id);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-framework/src/output.ts` around lines 38 - 44, The
type-narrowing helper is using unsafe `as any` assertions; update
isResultWithMessage to cast `result` to `Record<string, unknown>` and use
bracket access to check the message property (e.g., typeof (result as
Record<string, unknown>)['message'] === 'string'), and apply the same change to
the other occurrence mentioned (the other type-narrowing check around line ~60)
so no `as any` remains and property access stays type-safe.
Comment on lines
+70
to
+71
| headers?: string[], | ||
| maxColWidth = 60, |
Contributor
There was a problem hiding this comment.
Validate headers length against columns before rendering.
Line 75 accepts arbitrary header arrays, but Line 95 and Line 97 assume width alignment. If lengths diverge, table output becomes misaligned silently.
Proposed fix
export function table(
data: Record<string, unknown>[],
columns: string[],
headers?: string[],
maxColWidth = 60,
): string {
if (data.length === 0) return "No results.";
const hdrs = headers ?? columns.map((c) => c.toUpperCase());
+ if (hdrs.length !== columns.length) {
+ throw new Error("headers length must match columns length");
+ }
@@
- const headerLine = hdrs.map((h, i) => h.padEnd(widths[i]!)).join(" ");
+ const headerLine = hdrs.map((h, i) => h.padEnd(widths[i])).join(" ");
const bodyLines = rows.map((r) =>
- r.map((cell, i) => cell.padEnd(widths[i]!)).join(" "),
+ r.map((cell, i) => cell.padEnd(widths[i])).join(" "),
);Also applies to: 75-75, 95-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-framework/src/output.ts` around lines 70 - 71, Validate the
headers array length against the computed columns before rendering: in the
table-rendering function that accepts headers, columns and maxColWidth
(referencing the headers and columns variables), add a check after columns is
determined to ensure headers == null or headers.length === columns.length; if
not, either throw a clear error or normalize by padding/truncating the headers
array to columns.length (and log a warning). This prevents silent misalignment
when headers length diverges from columns and keeps downstream width
calculations (which use headers, columns, and maxColWidth) consistent.
…nse) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/app/device/page.tsx (1)
139-152: Consider distinguishing deny failure from success.Currently, if the deny request fails (network error), the user sees "Denied" but the device authorization may still succeed if the device polls before the code expires. Showing an error state instead would give the user a chance to retry.
💡 Optional: Show error on deny failure
const handleDeny = async () => { const code = userCode.replace(/[-\s]/g, "").toUpperCase(); try { - await fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`, { + const res = await fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`, { method: "POST", headers: { "Content-Type": "application/json" }, credentials: "include", body: JSON.stringify({ userCode: code }), }); + if (!res.ok) { + setError("Failed to deny authorization"); + setStatus("error"); + return; + } setStatus("denied"); } catch { - setStatus("denied"); + setError("Failed to deny authorization"); + setStatus("error"); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/device/page.tsx` around lines 139 - 152, The deny flow conflates network/response failures with success in handleDeny; change the implementation to check the fetch response (capture response = await fetch(...)) and only call setStatus("denied") when response.ok is true, otherwise parse or log the error and call setStatus("deny_error") (or another distinct error state) in the catch block or when !response.ok so the UI can show a retry/error message; update any UI that reads setStatus to handle the new "deny_error" state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/app/device/page.tsx`:
- Around line 139-152: The deny flow conflates network/response failures with
success in handleDeny; change the implementation to check the fetch response
(capture response = await fetch(...)) and only call setStatus("denied") when
response.ok is true, otherwise parse or log the error and call
setStatus("deny_error") (or another distinct error state) in the catch block or
when !response.ok so the UI can show a retry/error message; update any UI that
reads setStatus to handle the new "deny_error" state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d8fa361-91f0-4729-a567-fbcde40c5395
📒 Files selected for processing (1)
apps/web/src/app/device/page.tsx
Better Auth's generics infer these correctly — the manual annotations were masking proper inference rather than fixing real type gaps. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/web/src/app/device/page.tsx (2)
107-111:⚠️ Potential issue | 🟡 MinorGuard
setActivefailure before proceeding to approval POST.
setActiveis awaited outside thetryblock, so a thrown error skips your status/error handling and can leave the flow inconsistent.💡 Suggested fix
- if (selectedOrgId) { - await authClient.organization.setActive({ - organizationId: selectedOrgId, - }); - } - - try { + try { + if (selectedOrgId) { + await authClient.organization.setActive({ + organizationId: selectedOrgId, + }); + } + const res = await fetch( `${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/approve`, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/device/page.tsx` around lines 107 - 111, The await call to authClient.organization.setActive is outside the try/catch so its errors bypass your error handling and allow the subsequent approval POST to run in an inconsistent state; move the await authClient.organization.setActive({ organizationId: selectedOrgId }) inside the existing try block (or wrap it in its own try/catch) and on error set the component/error state and return early so the approval POST (the code that sends the approval POST) does not execute when setActive fails.
77-87:⚠️ Potential issue | 🟡 MinorAdd failure handling for organization loading before showing approval state.
authClient.organization.list()has no failure branch; if it fails, org context can remain unset while the flow continues.💡 Suggested fix
- authClient.organization.list().then(({ data }) => { + authClient.organization.list().then(({ data }) => { const orgList = (data ?? []).map((m) => ({ id: m.id, name: m.name, })); setOrgs(orgList); // Default to active org or first org const activeOrgId = session.session.activeOrganizationId; setSelectedOrgId(activeOrgId ?? orgList[0]?.id ?? ""); - }); + }).catch(() => { + setError("Failed to load organizations"); + setStatus("error"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/device/page.tsx` around lines 77 - 87, The org listing call (authClient.organization.list()) lacks error handling and may leave org context unset; wrap the promise with a .catch to handle failures: log the error (or forward to process logger), setOrgs([]) and setSelectedOrgId(session.session.activeOrganizationId ?? "") (or empty string) as a safe default, and set a new flag state (e.g., setOrgsLoaded or setOrgLoadError) so the approval flow/UI (where setSelectedOrgId and approval state are used) waits for org loading to complete before showing approval UI; update the component to gate the approval state rendering on that loaded flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/device/page.tsx`:
- Around line 140-150: The deny branch currently sets setStatus("denied")
unconditionally, including in the catch, which falsely reports success; modify
the device deny logic that calls
fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`) so you only
call setStatus("denied") when the response is successful (e.g., response.ok) and
in the catch or non-OK path call setStatus("error") or another appropriate
failure state instead; update the code around the try/catch and the response
handling that uses setStatus to reflect success vs failure instead of always
setting "denied".
- Around line 68-95: The getSession() promise can reject and currently has no
error handler, which leaves the UI stuck in "loading"; wrap or chain
authClient.getSession() with a .catch handler (or use try/catch if converted to
async) inside the existing useEffect to handle failures: on rejection call
setStatus("error") or setStatus("idle") (and optionally set an error state), log
the error (console.error or process logger), and ensure you do not proceed to
call authClient.organization.list(), setOrgs, setSelectedOrgId, or verifyCode
when session retrieval fails; update the block around authClient.getSession(),
setStatus, verifyCode, setOrgs, and setSelectedOrgId to include this error
handling.
---
Duplicate comments:
In `@apps/web/src/app/device/page.tsx`:
- Around line 107-111: The await call to authClient.organization.setActive is
outside the try/catch so its errors bypass your error handling and allow the
subsequent approval POST to run in an inconsistent state; move the await
authClient.organization.setActive({ organizationId: selectedOrgId }) inside the
existing try block (or wrap it in its own try/catch) and on error set the
component/error state and return early so the approval POST (the code that sends
the approval POST) does not execute when setActive fails.
- Around line 77-87: The org listing call (authClient.organization.list()) lacks
error handling and may leave org context unset; wrap the promise with a .catch
to handle failures: log the error (or forward to process logger), setOrgs([])
and setSelectedOrgId(session.session.activeOrganizationId ?? "") (or empty
string) as a safe default, and set a new flag state (e.g., setOrgsLoaded or
setOrgLoadError) so the approval flow/UI (where setSelectedOrgId and approval
state are used) waits for org loading to complete before showing approval UI;
update the component to gate the approval state rendering on that loaded flag.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a8efdb9-244f-450a-922e-c748d940e45c
📒 Files selected for processing (1)
apps/web/src/app/device/page.tsx
Comment on lines
+68
to
+95
| useEffect(() => { | ||
| authClient.getSession().then(({ data: session }) => { | ||
| if (!session) { | ||
| const returnUrl = `/device${userCodeParam ? `?user_code=${userCodeParam}` : ""}`; | ||
| router.push(`/sign-in?redirect=${encodeURIComponent(returnUrl)}`); | ||
| return; | ||
| } | ||
|
|
||
| // Fetch user's organizations | ||
| authClient.organization.list().then(({ data }) => { | ||
| const orgList = (data ?? []).map((m) => ({ | ||
| id: m.id, | ||
| name: m.name, | ||
| })); | ||
| setOrgs(orgList); | ||
|
|
||
| // Default to active org or first org | ||
| const activeOrgId = session.session.activeOrganizationId; | ||
| setSelectedOrgId(activeOrgId ?? orgList[0]?.id ?? ""); | ||
| }); | ||
|
|
||
| setStatus(userCodeParam ? "verifying" : "idle"); | ||
|
|
||
| if (userCodeParam) { | ||
| verifyCode(userCodeParam); | ||
| } | ||
| }); | ||
| }, [router, userCodeParam, verifyCode]); |
Contributor
There was a problem hiding this comment.
Handle getSession() rejection to avoid a stuck loading state.
If authClient.getSession() rejects, the effect has no fallback path, so users can remain on "loading" with no feedback.
💡 Suggested fix
useEffect(() => {
- authClient.getSession().then(({ data: session }) => {
+ authClient.getSession().then(({ data: session }) => {
if (!session) {
const returnUrl = `/device${userCodeParam ? `?user_code=${userCodeParam}` : ""}`;
router.push(`/sign-in?redirect=${encodeURIComponent(returnUrl)}`);
return;
}
@@
if (userCodeParam) {
verifyCode(userCodeParam);
}
- });
+ }).catch(() => {
+ setError("Failed to load session");
+ setStatus("error");
+ });
}, [router, userCodeParam, verifyCode]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/device/page.tsx` around lines 68 - 95, The getSession()
promise can reject and currently has no error handler, which leaves the UI stuck
in "loading"; wrap or chain authClient.getSession() with a .catch handler (or
use try/catch if converted to async) inside the existing useEffect to handle
failures: on rejection call setStatus("error") or setStatus("idle") (and
optionally set an error state), log the error (console.error or process logger),
and ensure you do not proceed to call authClient.organization.list(), setOrgs,
setSelectedOrgId, or verifyCode when session retrieval fails; update the block
around authClient.getSession(), setStatus, verifyCode, setOrgs, and
setSelectedOrgId to include this error handling.
Comment on lines
+140
to
+150
| try { | ||
| await fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| credentials: "include", | ||
| body: JSON.stringify({ userCode: code }), | ||
| }); | ||
| setStatus("denied"); | ||
| } catch { | ||
| setStatus("denied"); | ||
| } |
Contributor
There was a problem hiding this comment.
Do not report “Denied” when the deny request fails.
The current code sets "denied" in both success and failure paths, which can falsely confirm revocation to the user.
💡 Suggested fix
try {
- await fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`, {
+ const res = await fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`, {
method: "POST",
headers: { "Content-Type": "application/json" },
credentials: "include",
body: JSON.stringify({ userCode: code }),
});
- setStatus("denied");
+ if (!res.ok) {
+ setError("Failed to deny request");
+ setStatus("error");
+ return;
+ }
+ setStatus("denied");
} catch {
- setStatus("denied");
+ setError("Failed to deny request");
+ setStatus("error");
}📝 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
| try { | |
| await fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| credentials: "include", | |
| body: JSON.stringify({ userCode: code }), | |
| }); | |
| setStatus("denied"); | |
| } catch { | |
| setStatus("denied"); | |
| } | |
| try { | |
| const res = await fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| credentials: "include", | |
| body: JSON.stringify({ userCode: code }), | |
| }); | |
| if (!res.ok) { | |
| setError("Failed to deny request"); | |
| setStatus("error"); | |
| return; | |
| } | |
| setStatus("denied"); | |
| } catch { | |
| setError("Failed to deny request"); | |
| setStatus("error"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/device/page.tsx` around lines 140 - 150, The deny branch
currently sets setStatus("denied") unconditionally, including in the catch,
which falsely reports success; modify the device deny logic that calls
fetch(`${process.env.NEXT_PUBLIC_API_URL}/api/auth/device/deny`) so you only
call setStatus("denied") when the response is successful (e.g., response.ok) and
in the catch or non-OK path call setStatus("error") or another appropriate
failure state instead; update the code around the try/catch and the response
handling that uses setStatus to reflect success vs failure instead of always
setting "denied".
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 7, 2026
) * add CLI package scaffolding with pseudocode commands Command pseudocode for auth, tasks, workspaces, devices, and host. File-based routing structure with command.ts/meta.ts/middleware.ts convention. Includes CLI spec and TODO docs. * add @superset/cli-framework package New CLI framework with file-based routing, typed option builders, single middleware with skip, lazy command loading, auto-generated help, signal handling, agent mode detection, and output formatting. ~1545 lines, zero dependencies, typechecks clean. * wire CLI commands to framework, all typechecking and running - CLI package imports from @superset/cli-framework - All commands typecheck cleanly - Help works at root, group, and command levels - Aliases work (ws → workspaces, t → tasks) - Typo suggestions work (taks → tasks) - --json output works - Middleware skip works for auth/host commands - Lazy-loads command descriptions for help * fix: camelCase to kebab-case for option flags, show command descriptions in help - assigneeMe → --assignee-me in help and parsing - Command description now shows in command-level help - All option configs consistently kebab-cased * wire real tRPC client and API types into CLI commands - Auth flow: login (browser callback), logout, whoami with real API calls - Tasks: list, get, create, delete with real tRPC mutations/queries - Workspaces/devices: stubbed with CLIError for device routing - Config management: ~/.superset/config.json for auth token storage - Middleware uses real readConfig + createApiClient * bump better-auth 1.4.18 → 1.5.6, add device authorization plugin Breaking changes handled: - apiKey plugin moved to @better-auth/api-key (server + client imports) - apiKey.userId → apiKey.referenceId (schema + MCP auth flow) - Added configId column to apikeys table - Added device_codes table for OAuth device flow (RFC 8628) - Added deviceAuthorization() plugin to auth config Migration needed: apikeys table (userId → referenceId, add configId) + new device_codes table * add OAuth device flow for CLI auth + /device approval page CLI: superset auth login uses RFC 8628 device authorization flow — requests device code, opens browser with pre-filled code, polls for approval. Token stored in ~/.superset/config.json. Web: /device page for code entry and approve/deny flow. * CLI auth e2e working + tasks list with real data - Device flow: fixed snake_case response fields, auto-registers OAuth client - /device page: redirects to sign-in if not authenticated, supports redirect param - Sign-in page: supports ?redirect= query param for post-auth redirect - Tasks list: flatten API response, display real data in table - Fixed migration SQL for apikeys rename - Added dev script to cli package.json with dotenv + NEXT_PUBLIC_API_URL * fix: detect CLAUDECODE and CLAUDE_CODE_ENTRYPOINT for agent mode auto-JSON * add org list/switch commands, clack prompts, device page org selection - org list: shows all orgs with active marker - org switch: interactive picker (clack select) when no arg, direct by name/slug/id - auth login: clack spinner during browser wait - /device page: org dropdown before authorize, redirects to sign-in if not authed - sign-in page: supports ?redirect= query param - tasks list: fixed field names to match API response shape - Added @clack/prompts dependency * polish: table truncation, error handling, tasks update, auth login UX - Table columns truncate at 60 chars with ellipsis - tRPC errors mapped to friendly messages (UNAUTHORIZED, NOT_FOUND, FETCH_ERROR) - --no-json flag overrides agent mode auto-detection - auth login: clack spinner, shows user/org after login, no duplicate output - tasks update command added - tasks get: fixed duplicate priority field - deviceAuth returns structured result for command to control display * add compiled binary support with static command registration - bin.ts auto-detects dev vs compiled mode - run-static.ts: static command tree from explicit imports (no fs scanning) - commands/index.ts: manifest of all commands, groups, middleware - buildStaticTree + resolveStaticMiddleware in framework - Build scripts: bun build --compile for darwin-arm64/x64 + linux-x64 - 55MB compiled binary, all features working * fix: resolve all Better Auth 1.5.6 type breakages across monorepo - Desktop: apiKeyClient import moved to @better-auth/api-key/client - Desktop: add @better-auth/api-key dependency - Desktop: type annotations for members.find(), token(), and stripe callbacks where Better Auth generic inference doesn't propagate through React hooks - Web: oauth consent response changed uri → url * fix: resolve all lint errors (biome formatting + React hooks) * fix: resolve remaining CI lint, typecheck, and build failures - Add biome overrides for cli-framework/cli packages (noExplicitAny, noNonNullAssertion) - Fix typecheck error in output.ts (number | undefined → fallback to 0) - Wrap /device page useSearchParams in Suspense boundary (fixes SSG build) - Replace `as any` casts in device page with proper types - Use biome-ignore comments for Better Auth inference gaps in desktop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve device page typecheck error (unknown → typed error response) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: remove unnecessary type annotations from Better Auth 1.5.6 upgrade Better Auth's generics infer these correctly — the manual annotations were masking proper inference rather than fixing real type gaps. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 7, 2026
cherry-pick済み: - e728ebd feat(desktop): wire up missing hotkeys for v2 workspace (superset-sh#3190) - 1eddeb3 feat(desktop): git changes sidebar with resource-oriented API (superset-sh#3177) - 11ed4f8 V2 terminal env (superset-sh#3184) - 0c52ecc feat(desktop): pane context menus + binary tree layout (superset-sh#3196) - 5578746 fix(desktop): resolve file icons from origin instead of href (superset-sh#3199) - 5a1e5d1 feat(panes): prefer sibling pane when closing active pane (superset-sh#3198) - d670c4a V2 top bar: right sidebar toggle, org dropdown in sidebar, unified open-in button (superset-sh#3197) - 2573fa2 fix(desktop): remove macOS background-to-tray quit interception (superset-sh#3205) - 4a29342 feat: Superset CLI + CLI framework + Better Auth 1.5.6 (superset-sh#3194) - 700cd65 fix(desktop): revert broken file icon origin fix + bundle all icon sources (superset-sh#3218) フォーク独自対応: - cleanupMainWindowResources()をexit pathに移動維持 (#3205対応) - BROWSER_HARD_RELOAD/SEARCH_IN_FILESをv2 workspaceに配線 - BROWSER_RELOAD/HARD_RELOADのuseHotkey配線修正（リマップ対応） - ansi_up依存維持
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
1 participant
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
packages/cli/) — new CLI for managing Superset from the terminal. Auth, tasks, orgs, workspaces, devices, host commands.packages/cli-framework/) — generic, open-sourceable CLI framework with typed option builders, file-based routing, middleware, auto-generated help, agent mode detection, and compiled binary support.@better-auth/api-key,userId→referenceId, oauth consenturi→url, generic inference fixes)./deviceweb page for code approval with org selection.apikeystable schema update + newdevice_codestable.CLI commands working
superset auth loginsuperset auth logoutsuperset auth whoamisuperset org listsuperset org switchsuperset tasks listsuperset tasks get <slug>superset tasks createsuperset tasks update <slug>superset tasks delete <slug...>superset devices listsuperset workspaces *superset host *Framework features
commands/tasks/list/command.ts→superset tasks list)--helpat all levels--json/--quiet/--no-jsonoutput modesbun build --compile(55MB)Test plan
--help,--version,auth whoami)--no-jsonoverrides agent modeuserId→referenceIdmigrationSummary by cubic
Adds a Superset CLI and internal CLI framework, implements OAuth Device Flow with a
/deviceapproval page, and upgradesbetter-authto 1.5.6 with breaking changes handled. Also fixes CI lint/type/build issues, adds CLI lint overrides, resolves SSG on the/devicepage, and removes redundant Better Auth type annotations.New Features
packages/cli):auth login/logout/whoami,org list/switch,tasks list/get/create/update/delete; stubs fordevicesandworkspaces; supports--json,--quiet,--no-json(overrides agent mode), auto-JSON in agent mode, and a compiled binary with static command registration.packages/cli-framework): typed option builders, file-based routing, single middleware with skip, auto help, typo suggestions, signal handling, static build support, static command tree for compiled mode, and safe output formatting.deviceAuthorization()with browser approval at/device; sign-in supports?redirect=; consent response usesurl;/devicepage wrapped in Suspense and typed error handling for SSG.Migration
auth.device_codes; renameauth.apikeys.user_id→reference_id(text), addconfig_idand new indexes.deviceAuthorization({ verificationUri: "<WEB_URL>/device" }); update API key plugin to@better-auth/api-keyand usereferenceId.@better-auth/api-key/client; handle OAuth consent responseuri→url; remove unnecessary type annotations introduced during thebetter-auth1.5.6 upgrade.Written for commit 5b695f3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Refactor
Chores