Feat/cli/init #2049
Feat/cli/init #2049
Conversation
- Implement custom error classes for CLI operations in `errors.ts` - Create a logger utility in `logger.ts` that redacts sensitive information - Add unified JSON output helpers for CLI commands in `output.ts` - Configure TypeScript settings in `tsconfig.json` - Set up build configuration with `tsup` in `tsup.config.ts` feat(i18n): add CLI authorization translations - Add English and Chinese translations for CLI authorization messages in `ui.ts` feat(utils): extend ID generation utilities - Add new ID prefixes for API keys and device sessions in `id.ts` feat(web-core): implement CLI authorization page - Create CLI authorization page with device information display and authorization flow - Add styles for the CLI authorization page in `index.css` - Integrate the CLI authorization page into the main application
…matting - Add AI-powered workflow generation command (workflow generate) - Add workflow status command with watch mode for monitoring runs - Add node result command to fetch execution results - Add file/drive commands for listing, getting, and downloading files - Add tool calls command for inspecting tool executions - Add action result command for action execution results - Implement multi-format output support (json, pretty, compact, plain) - Add streaming download support in API client - Create new CLI controllers for action, drive, and tool-call modules - Update SKILL.md documentation with new commands - Add formatter, spinner, and UI utilities for better CLI experience Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* feat(cli-auth): enhance CLI authorization page functionality - Add user settings initialization to fetch profile and update login status. - Refactor login status check to improve clarity and logic. - Update effect dependencies to include user profile for better state management. * refactor(auth-cli): streamline handleCallback method signature for clarity * fix(formatter): correct AsciiSymbol references in output formatting methods
📝 WalkthroughWalkthroughIntroduces a comprehensive CLI implementation with API key management, device-based authorization flow, and complete workflow builder mode. Adds Prisma models for API key storage and device sessions, multiple API controllers for CLI endpoints, and a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Application
participant Server as Backend API
participant OAuthProvider as OAuth Provider<br/>(Google/GitHub)
participant LocalCallback as Local Callback<br/>Server
rect rgb(200, 220, 255)
note over CLI,LocalCallback: OAuth Flow (refly login --oauth)
CLI->>Server: POST /v1/auth/cli/oauth/init<br/>(provider, port)
Server-->>CLI: authUrl, state
CLI->>LocalCallback: Start local server on port
CLI->>CLI: Open browser to authUrl
OAuthProvider->>LocalCallback: Redirect with code
LocalCallback-->>CLI: code, state
CLI->>Server: POST /v1/auth/cli/oauth/callback<br/>(code, state, provider)
Server->>OAuthProvider: Exchange code for tokens
OAuthProvider-->>Server: accessToken, refreshToken, profile
Server->>Server: Create/link user via oauthValidate
Server-->>CLI: accessToken, refreshToken, user
CLI->>CLI: Store tokens in ~/.refly/config.json
end
rect rgb(200, 255, 220)
note over CLI,Server: Device Authorization Flow (headless)
CLI->>Server: POST /v1/auth/cli/device/init<br/>(cliVersion, host)
Server-->>CLI: deviceId, sessionId
CLI->>CLI: Display device code & auth URL
CLI->>Server: GET /v1/auth/cli/device/status?deviceId<br/>(poll)
Server-->>CLI: status=pending
User->>Server: POST /v1/auth/cli/device/authorize<br/>with JWT (via web)
Server->>Server: Link user to device session
CLI->>Server: GET /v1/auth/cli/device/status?deviceId
Server-->>CLI: status=authorized,<br/>accessToken, refreshToken
CLI->>CLI: Store tokens, exit
end
rect rgb(255, 220, 200)
note over CLI,Server: API Key Authentication
CLI->>Server: POST /v1/auth/cli/api-key<br/>(name, expiresInDays)
Server-->>CLI: keyId, apiKey, keyPrefix
CLI->>CLI: Store apiKey in ~/.refly/config.json
CLI->>Server: GET /v1/cli/workflow?api-key=rf_xxx<br/>(in X-API-Key header)
Server->>Server: Validate API key via ApiKeyService
Server-->>CLI: workflow list
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/api/src/modules/auth/guard/jwt-auth.guard.ts (2)
34-37: Avoid settingrequest.user.uidtoundefinedin desktop mode.
this.configService.get('local.uid')can be missing; downstream auth assumptions may break.Proposed fix
if (isDesktop()) { - request.user = { uid: this.configService.get('local.uid') }; + const uid = this.configService.get<string>('local.uid'); + if (!uid) { + throw new UnauthorizedException('Missing local.uid in desktop mode'); + } + request.user = { uid }; return true; }
69-72: Harden Authorization parsing and don't log raw verification errorsThe current code has two confirmed issues:
Error logging vulnerability (lines 69-72): Using template literal interpolation with error objects (
\jwt verify not valid: ${error}``) exposes error details in logs, which can leak sensitive stack traces and implementation details.Brittle header parsing (lines 87-94, 99-107): The
authHeader.split(' ')pattern fails gracefully with optional chaining, but can behave unexpectedly with malformed headers (e.g., "Bearer" without a token, or extra whitespace). For example, at line 104, ifauthHeaderis "Bearer" (no token), the condition!token?.startsWith('rf_')evaluates to true and returnsundefined, which is semantically incorrect.Recommended fix
- } catch (error) { - this.logger.warn(`jwt verify not valid: ${error}`); + } catch (error) { + this.logger.warn('jwt verify not valid'); throw new UnauthorizedException(); }For header parsing, validate the split result:
- const [type, token] = authHeader.split(' '); - if (type === 'Bearer' && token?.startsWith('rf_')) { + const parts = authHeader.split(' '); + if (parts.length === 2 && parts[0] === 'Bearer' && parts[1]?.startsWith('rf_')) { + const token = parts[1]; return token; }apps/api/src/modules/copilot-autogen/copilot-autogen.service.ts (2)
81-106: Critical:canvasIdownership isn’t enforced; Prisma update can let users overwrite others’ canvases (IDOR).
getCanvasRawData(...checkOwnership: false)+prisma.canvas.update({ where: { canvasId } })means a caller who can hit this flow with an arbitrarycanvasIdmay update someone else’s canvas.Suggested direction (high-level)
- const rawCanvas = await this.canvasService.getCanvasRawData(user, canvasId, { - checkOwnership: false, - }); + const rawCanvas = await this.canvasService.getCanvasRawData(user, canvasId, { + checkOwnership: true, + });Also strongly prefer routing persistence through a service method that enforces ownership (or include ownerId in Prisma
whereclause if available).Also applies to: 193-205, 326-375
212-242: Remove unsafe error property access in catch block at lines 371-374.The
catchblock accesseserror.messagewithout type checking. Sinceerroris implicitlyunknown, use a type guard to safely handle the error:Safe pattern
} catch (error) { - this.logger.error(`[Autogen] Failed to update canvas state: ${error.message}`); + const err = error instanceof Error ? error : new Error(String(error)); + this.logger.error(`[Autogen] Failed to update canvas state: ${err.message}`, err.stack); throw error; }
🤖 Fix all issues with AI agents
In @apps/api/src/modules/auth/auth-cli.controller.ts:
- Around line 54-94: Validate the incoming port in initOAuth before using it to
build redirectUri: ensure the @Query('port') value is a numeric integer between
1 and 65535 (reject otherwise with BadRequestException) and reject non-numeric
or out-of-range inputs to prevent malformed URIs or injection; apply the same
validation logic to any other CLI OAuth entrypoints that build a localhost
redirect (e.g., the other method referenced in lines 103-164). Locate initOAuth
and related helpers (generateOAuthStateToken, generateGoogleCliOAuthUrl,
generateGithubCliOAuthUrl) and perform the check immediately after reading the
port, returning a clear BadRequestException if invalid, and only then call
generateOAuthStateToken and build the redirectUri.
In @apps/api/src/modules/auth/device-auth.service.ts:
- Around line 291-309: The cleanupExpiredSessions method is never scheduled;
annotate it with a scheduling decorator (e.g., add @Cron('0 * * * *') or
@Interval(60_000) from @nestjs/schedule) on the cleanupExpiredSessions method in
DeviceAuthService so it runs periodically, import the corresponding decorator at
the top of apps/api/src/modules/auth/device-auth.service.ts, and ensure the
module that provides DeviceAuthService imports ScheduleModule.forRoot() (or
ScheduleModule) so the scheduler is active; also keep the method as async and
optionally add try/catch logging around the body to surface scheduling errors.
In @packages/cli/src/builder/store.ts:
- Around line 43-60: saveSession currently does an atomic rename with
fs.renameSync(tempPath, sessionPath) which can fail on Windows if sessionPath
already exists; update saveSession to detect and remove an existing sessionPath
before renaming (e.g., if fs.existsSync(sessionPath) then try
fs.unlinkSync(sessionPath) inside a try/catch), then perform
fs.renameSync(tempPath, sessionPath); ensure you still write to tempPath, keep
the BuilderSessionSchema.parse validation, and handle/unwind errors around
unlink+rename so the temp file is not left behind.
In @packages/cli/src/utils/spinner.ts:
- Around line 169-199: The update method can throw RangeError when this.total <=
0 or when current overshoots; clamp and guard numeric math: first normalize
this.current = Math.min(Math.max(current, 0), Math.total) (or use this.total if
total <= 0), compute percent using a guarded expression (percent = this.total >
0 ? Math.round((this.current / this.total) * 100) : 0), and compute filled as a
bounded integer (filled = this.total > 0 ? Math.max(0, Math.min(this.width,
Math.round((this.current / this.total) * this.width))) : (this.current > 0 ?
this.width : 0)); then set empty = this.width - filled; also use the same
guarded percent in the non-TTY branch so repeat() never receives
negative/Infinity. Apply these changes inside update in spinner.ts to prevent
divide-by-zero and negative repeat arguments.
🟠 Major comments (24)
packages/cli/src/commands/workflow/edit.ts-41-49 (1)
41-49: Address the unreachable code pattern in error handling.The error handling in
edit.tsis consistent with all other workflow commands, but all of them share a logic issue: the secondfail()call will always execute after theCLIErrorcheck. Use anelseblock orreturnstatement to prevent the fallback error from being triggered after handling aCLIError.This pattern exists consistently across all workflow commands (abort.ts, create.ts, delete.ts, get.ts, generate.ts, index.ts, list.ts, run.ts, run-get.ts, status.ts) and should be fixed across the codebase.
packages/cli/src/utils/spinner.ts-43-130 (1)
43-130: Spinner hides the cursor; ensure it’s restored on abnormal exits.If the process exits while the spinner is running (exception, SIGINT,
process.exit()elsewhere), the cursor can remain hidden in the user’s terminal.packages/cli/src/config/paths.ts-66-70 (1)
66-70: HardenensureDir: enforce permissions and handle TOCTOU.If
~/.reflyalready exists with permissive mode (e.g.0755), credentials stored under it may be readable by other local users. AlsoexistsSync→mkdirSynccan race.Proposed fix
export function ensureDir(dir: string): void { - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); - } + try { + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); + } catch { + // If another process created it concurrently, proceed to permission check below. + } + + try { + const stat = fs.statSync(dir); + const mode = stat.mode & 0o777; + if (mode !== 0o700) { + fs.chmodSync(dir, 0o700); + } + } catch { + // Best-effort; callers will fail later if the directory is unusable. + } }packages/cli/src/api/client.ts-245-360 (1)
245-360:apiRequestStream()buffers the entire download to memory; consider implementing true streaming to avoid OOM issues.The function buffers the full response into a
Bufferobject despite its name suggesting streaming behavior. For large files, this causes unnecessary memory overhead. Additionally, the CLI download commands make this worse by using synchronousfs.writeFileSync()after buffering the entire file, blocking the event loop during writes.Consider either:
- Returning a true stream (Node.js
Readable) so callers can pipe directly to disk- Enforcing a maximum file size cap before buffering
- Implementing chunked reading and writing to keep memory usage bounded
packages/cli/src/api/client.ts-148-197 (1)
148-197: Backend does not return token expiry in refresh response; CLI hardcodes 1-hour TTL despite backend having configurable JWT expiry.The refresh token endpoint returns only
{ accessToken, refreshToken }without expiry metadata. Meanwhile, the backend configures JWT TTL viaauth.jwt.expiresIn, creating a mismatch: the CLI always assumes 1 hour while the server may issue tokens with a different TTL. This can cause premature refreshes or invalid token usage if the backend's JWT expiry differs from 1 hour.The backend should return the token's actual expiration time (or TTL) so the CLI can sync correctly.
Proposed fix (backend must return expiry information)
- ): Promise<{ success: boolean; data: { accessToken: string; refreshToken: string } }> { + ): Promise<{ success: boolean; data: { accessToken: string; refreshToken: string; expiresInSeconds?: number } }> { const { refreshToken } = body; this.logger.log('[CLI_OAUTH_REFRESH]'); if (!refreshToken) { throw new AuthError('No refresh token provided'); } const tokens = await this.authService.refreshAccessToken(refreshToken); return buildSuccessResponse({ accessToken: tokens.accessToken, refreshToken: tokens.refreshToken, + expiresInSeconds: this.jwtConfigService.get('auth.jwt.expiresIn') || 3600, }); }Then update the CLI to use the server-provided TTL:
const data = (await response.json()) as APIResponse<{ accessToken: string; refreshToken: string; + expiresInSeconds?: number; }>; if (!data.success || !data.data) { throw new AuthError('Failed to refresh token'); } // Update stored tokens + const expiresInSeconds = data.data.expiresInSeconds ?? 3600; setOAuthTokens({ accessToken: data.data.accessToken, refreshToken: data.data.refreshToken, - expiresAt: new Date(Date.now() + 3600000).toISOString(), + expiresAt: new Date(Date.now() + expiresInSeconds * 1000).toISOString(), provider, user, });packages/cli/skill/references/api-errors.md-33-38 (1)
33-38: RemoveVERSION_MISMATCHfrom the error table—this code doesn't exist in the CLI codebase.The error codes documented at lines 33–38 include
VERSION_MISMATCH, but it is not defined anywhere in the CLI source code (onlyCLI_NOT_FOUNDandCONFIG_ERRORexist). This will mislead users. Replace or remove this entry before merging.Additionally, the bash example at lines 100–109 should quote the
$resultvariable to safely handle values with spaces or special characters:Proposed bash quoting fix
-if [ "$(echo $result | jq -r '.ok')" = "false" ]; then - code=$(echo $result | jq -r '.error.code') - hint=$(echo $result | jq -r '.error.hint') +if [ "$(echo "$result" | jq -r '.ok')" = "false" ]; then + code=$(echo "$result" | jq -r '.error.code') + hint=$(echo "$result" | jq -r '.error.hint') echo "Error: $code. Try: $hint" exit 1 fipackages/cli/src/commands/workflow/generate.ts-49-65 (1)
49-65: Validate--timeoutand--variablesshape before sending to API.Today
timeoutcan becomeNaN, and--variablescan parse to a non-array while being treated asunknown[].Proposed fix
- let variables: unknown[] | undefined; - if (options.variables) { + let variables: unknown[] | undefined; + if (options?.variables) { try { - variables = JSON.parse(options.variables); + const parsed = JSON.parse(options.variables); + if (!Array.isArray(parsed)) { + fail(ErrorCodes.INVALID_INPUT, '--variables must be a JSON array', { + hint: 'Example: --variables \'[{"name":"foo","value":"bar"}]\'', + }); + } + variables = parsed; } catch { fail(ErrorCodes.INVALID_INPUT, 'Invalid JSON in --variables', { hint: 'Ensure the variables parameter is valid JSON', }); } } + const timeoutMs = Number(options?.timeout ?? 300000); + if (!Number.isFinite(timeoutMs) || timeoutMs <= 0) { + fail(ErrorCodes.INVALID_INPUT, 'Invalid --timeout value', { + hint: 'Provide a positive number of milliseconds, e.g. --timeout 300000', + }); + } + // Build request body const body: Record<string, unknown> = { - query: options.query, - timeout: Number.parseInt(options.timeout, 10), + query: options?.query, + timeout: timeoutMs, }; ... const result = await apiRequest<GenerateWorkflowResponse>('/v1/cli/workflow/generate', { method: 'POST', body, - timeout: Number.parseInt(options.timeout, 10) + 30000, // Add buffer for API processing + timeout: timeoutMs + 30000, // Add buffer for API processing });Also applies to: 74-79
packages/cli/src/commands/drive/get.ts-24-30 (1)
24-30: EncodefileIdbefore interpolating into the URL path.A
fileIdcontaining/,?, or#will break routing/query parsing.Proposed fix
- const result = await apiRequest<DriveFile>( - `/v1/cli/drive/files/${fileId}?includeContent=${includeContent}`, - ); + const safeFileId = encodeURIComponent(fileId); + const result = await apiRequest<DriveFile>( + `/v1/cli/drive/files/${safeFileId}?includeContent=${includeContent}`, + );packages/cli/src/commands/file/list.ts-32-63 (1)
32-63:--include-contentflag is unused in output (option/behavior mismatch).The
--include-contentoption is passed to the API but thecontentfield is neither defined in theFileInfointerface nor included in the command output. This creates a confusing UX where setting the flag has no visible effect.Add
content?: stringto theFileInfointerface and conditionally include it in the mapped response when the flag is set, following the same pattern used in thefile getcommand.Proposed fix
interface FileInfo { fileId: string; name: string; type: string; size?: number; + content?: string; createdAt: string; updatedAt: string; } ... - files: result.files?.map((f) => ({ + files: (result.files ?? []).map((f) => ({ fileId: f.fileId, name: f.name, type: f.type, size: f.size, + content: options.includeContent ? f.content : undefined, createdAt: f.createdAt, updatedAt: f.updatedAt, })),apps/api/src/modules/drive/drive-cli.controller.ts-14-16 (1)
14-16: Add OpenAPI decorators for CLI endpoints.This controller has no Swagger/OpenAPI annotations (tags, auth, operations, query params, responses). As per controller guidelines, these endpoints should be documented.
Based on learnings, document APIs with OpenAPI specifications.
Also applies to: 19-64
apps/api/src/modules/drive/drive-cli.controller.ts-22-45 (1)
22-45:total: result.lengthdoes not reflect actual total count—returns only current page size, breaking pagination.
DriveService.listDriveFiles()returnsPromise<DriveFile[]>without total count metadata. Since the service usesskip/takepagination,result.lengthis at most the page size (20), not the total across all pages. Clients cannot determine total pages or know if more data exists.Suggested fix:
- Modify
listDriveFilesto return{ items: DriveFile[]; total: number }and forward that to the response, or- Add a separate
countDriveFiles()call and combine results.Additionally, this controller is missing OpenAPI decorators (
@ApiTags,@ApiOperation,@ApiResponse) required by repository guidelines for API documentation.apps/api/src/modules/drive/drive-cli.controller.ts-65-80 (1)
65-80: UseStreamableFileinstead of buffering entire file in memory.The method
getDriveFileStream()loads the complete file as a Buffer (data.length+res.send(data)), which consumes memory proportional to file size and defeats streaming. In NestJS v10, returnStreamableFilewith the actual Node stream and set headers via@Res({ passthrough: true }). This preserves Nest's lifecycle (interceptors, exception filters), works cross-platform, and handles backpressure automatically.Additionally, set
Content-Dispositionheaders to support RFC5987 for Unicode filenames: include bothfilename=(ASCII fallback) andfilename*=UTF-8''...(percent-encoded).packages/cli/src/bin/refly.ts-42-65 (1)
42-65: UseparseAsync()instead ofparse()to properly handle errors from async command actions.Errors thrown in async command actions won't be caught by
exitOverridewhen using synchronousparse(). Command.js requiresparseAsync()to properly await action callbacks and surface their errors to the error handler. Since your commands are async and use try-catch to callfail(), you needparseAsync()at line 109 to ensure proper error propagation.Additionally, avoid using
require()inside thepreActionhook (line 61) in an ESM module. Use static imports at the top of the file instead:import { Command } from 'commander'; import { fail, ErrorCodes, configureOutput, type OutputFormat } from '../utils/output.js'; +import { logger } from '../utils/logger.js'; // ... commands ... program.hook('preAction', (thisCommand) => { // ... - if (opts.debug) { - const { logger } = require('../utils/logger.js'); + if (opts.debug) { logger.setLevel('debug'); logger.enableFileLogging(); } }); -program.parse(); +await program.parseAsync();packages/cli/src/commands/login.ts-224-225 (1)
224-225: Potential XSS vulnerability in error message rendering.The
erroranderrorDescriptionvalues from the OAuth callback are inserted directly into HTML without escaping. A malicious OAuth provider or MITM attack could inject script content.🔒 Proposed fix with HTML escaping
+const escapeHtml = (str: string): string => { + return str + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); +}; + // In the error handling section: - <p><strong>Error:</strong> ${error}</p> - ${errorDescription ? `<p>${errorDescription}</p>` : ''} + <p><strong>Error:</strong> ${escapeHtml(error)}</p> + ${errorDescription ? `<p>${escapeHtml(errorDescription)}</p>` : ''}apps/api/src/modules/workflow/workflow-cli.controller.ts-478-509 (1)
478-509:limit/offsetquery params are strings at runtime; parse/validate them.apps/api/src/modules/workflow/workflow-cli.controller.ts-60-81 (1)
60-81: Don’t silently emitentityId: ''inbuildConnectToFilters(); validate or skip/throw.
Empty entityId is likely to create incorrect connection filters downstream.packages/cli/src/config/config.ts-83-102 (1)
83-102:renameSync(temp, config)may fail on Windows when destination exists; add a safe replace fallback.apps/api/src/modules/auth/auth-cli.controller.ts-104-114 (1)
104-114: Avoid destructuring request bodies before validating they exist (per TS guideline); prefer DTOs + validation pipe.Also applies to: 429-453, 559-574
apps/api/src/modules/workflow/workflow-cli.controller.ts-334-815 (1)
334-815: Add OpenAPI decorators for CLI endpoints (controllers guideline).
As per retrieved learnings / coding guidelines forapps/api/src/**/*.controller.ts.Also applies to: 821-944
packages/cli/src/config/config.ts-53-78 (1)
53-78: Bug:loadConfig()returnsDEFAULT_CONFIGby reference; callers mutate it (e.g.,updateSkillInfo).Proposed fix
export function loadConfig(): Config { const configPath = getConfigPath(); try { if (!fs.existsSync(configPath)) { - return DEFAULT_CONFIG; + return structuredClone(DEFAULT_CONFIG); } ... } catch { // Return default config if parsing fails - return DEFAULT_CONFIG; + return structuredClone(DEFAULT_CONFIG); } }If
structuredCloneisn’t available in your Node target, use a JSON clone or a small deep-clone helper.apps/api/src/modules/workflow/workflow-plan-cli.controller.ts-126-406 (1)
126-406: Add OpenAPI decorators for CLI endpoints (controllers guideline).
As per retrieved learnings / coding guidelines forapps/api/src/**/*.controller.ts.apps/api/src/modules/auth/auth-cli.controller.ts-34-700 (1)
34-700: Add OpenAPI decorators for CLI endpoints (controllers guideline).
As per retrieved learnings / coding guidelines forapps/api/src/**/*.controller.ts.apps/api/src/modules/workflow/workflow-plan-cli.controller.ts-210-247 (1)
210-247:@Query('version') version?: numberwill actually be a string; useParseIntPipe(or validation pipe).Suggested fix
import { Controller, Post, Get, Body, Param, Query, UseGuards, Logger, HttpException, HttpStatus, + ParseIntPipe, } from '@nestjs/common'; ... async get( @LoginedUser() user: User, @Param('planId') planId: string, - @Query('version') version?: number, + @Query('version', new ParseIntPipe({ optional: true })) version?: number, ): Promise<{ success: boolean; data: WorkflowPlanRecord }> {apps/api/src/modules/auth/auth-cli.controller.ts-170-224 (1)
170-224: Add AbortController with timeouts to fetch calls for Google and GitHub OAuth endpoints to prevent hung requests.Both
exchangeGoogleCode()(2 fetch calls) andexchangeGitHubCode()(3 fetch calls) make external API requests without timeout protection. These could hang indefinitely if the OAuth provider becomes unresponsive. Use the existing pattern fromapps/api/src/modules/knowledge/parsers/cheerio.parser.ts#fetchWithTimeout()as a reference.
🟡 Minor comments (26)
packages/cli/README.md-104-108 (1)
104-108: Add language specification to fenced code block.The state machine diagram code block is missing a language identifier, which is flagged by markdownlint (MD040).
📝 Proposed fix
-``` +```text IDLE → DRAFT → VALIDATED → COMMITTED ↑ ↓ ↓ └──────┴──────────┘ (abort)</details> </blockquote></details> <details> <summary>packages/cli/package.json-21-25 (1)</summary><blockquote> `21-25`: **Update dependencies to their latest stable versions.** The selected packages are well-suited for CLI parsing, input validation, and OAuth flows. However, the versions specified are outdated: - `commander`: ^12.1.0 → latest is 14.0.2 - `zod`: ^3.23.8 → latest is 4.3.5 - `open`: ^10.1.0 → latest is 11.0.0 Update to the latest versions to benefit from recent features, improvements, and security patches. </blockquote></details> <details> <summary>packages/cli/src/commands/workflow/edit.ts-14-24 (1)</summary><blockquote> `14-24`: **Unreachable code after `fail()` call in catch block.** The `fail()` function has return type `never` and calls `process.exit()`, but TypeScript doesn't understand that the code after the inner `catch` block is unreachable. While this works at runtime (because `fail()` exits the process), it's cleaner to add a `return` statement or restructure the code for clarity. <details> <summary>🔧 Proposed fix</summary> ```diff let ops: unknown; try { ops = JSON.parse(options.ops); } catch { fail(ErrorCodes.INVALID_INPUT, 'Invalid JSON in --ops', { hint: 'Ensure the operations are valid JSON', }); + return; // TypeScript flow analysis helper (never reached) }Alternatively, restructure to make the flow clearer:
- let ops: unknown; - try { - ops = JSON.parse(options.ops); - } catch { - fail(ErrorCodes.INVALID_INPUT, 'Invalid JSON in --ops', { - hint: 'Ensure the operations are valid JSON', - }); - } + let ops: unknown; + try { + ops = JSON.parse(options.ops); + } catch { + return fail(ErrorCodes.INVALID_INPUT, 'Invalid JSON in --ops', { + hint: 'Ensure the operations are valid JSON', + }); + }packages/cli/src/commands/tool/calls.ts-56-64 (1)
56-64: Missing return statement after firstfail()call causes unreachable code.When a
CLIErroris caught,fail()is called but execution continues to the secondfail()call. Add areturnstatement after the firstfail()to prevent this.🐛 Proposed fix
} catch (error) { if (error instanceof CLIError) { - fail(error.code, error.message, { details: error.details, hint: error.hint }); + return fail(error.code, error.message, { details: error.details, hint: error.hint }); } - fail( + return fail( ErrorCodes.INTERNAL_ERROR, error instanceof Error ? error.message : 'Failed to get tool calls', ); }packages/cli/src/commands/node/types.ts-29-71 (1)
29-71: Category list is inconsistent when--categoryis used (and guideline tweaks).
categoriesis computed fromdata.nodeTypes(unfiltered), whilenodeTypes/totalare filtered; also prefer?./??per repo guidelines.Proposed fix
export const nodeTypesCommand = new Command('types') .description('List available node types') .option('--refresh', 'Force refresh from API') .option('--category <category>', 'Filter by category') .action(async (options) => { try { let data: NodeTypesResponse; // Try cache first - if (!options.refresh) { + if (!options?.refresh) { const cached = loadFromCache(); if (cached) { data = cached; } else { data = await fetchAndCache(); } } else { data = await fetchAndCache(); } // Filter by category if specified - let nodeTypes = data.nodeTypes || []; - if (options.category) { + let nodeTypes = data.nodeTypes ?? []; + if (options?.category) { + const category = String(options.category).toLowerCase(); nodeTypes = nodeTypes.filter( - (t: NodeType) => t.category.toLowerCase() === options.category.toLowerCase(), + (t: NodeType) => t.category.toLowerCase() === category, ); } ok('node.types', { nodeTypes, total: nodeTypes.length, - categories: [...new Set(data.nodeTypes?.map((t: NodeType) => t.category) || [])], + categories: [...new Set(nodeTypes.map((t: NodeType) => t.category))], }); } catch (error) { if (error instanceof CLIError) { fail(error.code, error.message, { details: error.details, hint: error.hint }); } fail( ErrorCodes.INTERNAL_ERROR, error instanceof Error ? error.message : 'Failed to get node types', ); } });packages/cli/src/api/client.ts-202-229 (1)
202-229: Avoid casting arbitrary backenderrCodeintoErrorCode.If the backend sends unknown codes,
getExitCode(code)may behave unexpectedly. Prefer mapping unknown codes toAPI_ERROR/INTERNAL_ERRORand attach the original code indetails.Proposed fix
function mapAPIError(status: number, response: APIResponse): CLIError { const errCode = response.errCode ?? 'UNKNOWN'; const errMsg = response.errMsg ?? 'Unknown error'; @@ if (status >= 500) { return new CLIError(ErrorCodes.API_ERROR, errMsg, undefined, 'Try again later'); } - // Map API error codes to ErrorCode type - return new CLIError(errCode as ErrorCode, errMsg); + // Unknown/non-standard API codes: preserve in details and map to a stable CLI code. + return new CLIError(ErrorCodes.API_ERROR, errMsg, { apiErrCode: errCode }); }packages/cli/src/utils/formatter.ts-156-223 (1)
156-223: Comment/behavior mismatch:diffis described as nested output but is filtered out.Line 186 mentions “diff” as a nested object example, but
extractNestedObjects()excludeskey !== 'diff', so it will never render. Either remove “diff” from the comment or allow it through the filter.Also applies to: 627-636
packages/cli/commands/refly-login.md-12-16 (1)
12-16: Clarify credential storage security: credentials are stored in plaintext JSON, not encrypted.The docs claim "Store credentials securely" but the implementation stores API keys and OAuth tokens as plaintext JSON in
~/.refly/config.json. The only security measure is file permissions set to0o600(owner read/write only) on Unix-like systems; this permission enforcement does not apply on Windows. Readers should know the credentials are unencrypted and understand the actual security guarantees.Suggested doc fix
-3. Store credentials securely in `~/.refly/config.json` +3. Store credentials in `~/.refly/config.json` (plaintext JSON with file permissions set to 0600 on Unix-like systems; not encrypted)packages/cli/src/commands/init.ts-14-17 (1)
14-17: Avoid destructuringoptionswithout validation (per repo TS guidelines).Proposed fix
.option('--force', 'Force reinstall even if already installed') .action(async (options) => { try { - const { force } = options; + const force = options?.force ?? false;packages/cli/src/commands/workflow/status.ts-24-35 (1)
24-35: ImportWorkflowRunStatusandNodeExecutionStatusfrom the API DTO instead of redefining locally.These interfaces are already exported from
apps/api/src/modules/workflow/workflow-cli.dto.tsand designed specifically for CLI usage. Remove the local definitions and import them directly to eliminate duplication:import { WorkflowRunStatus, NodeExecutionStatus } from '@refly/api-types'; // or appropriate pathNote: Update the local
NodeStatusto useNodeExecutionStatusfrom the DTO, or ifNodeStatusrequires CLI-specific fields, clearly document the intentional separation.packages/cli/src/commands/builder/remove-node.ts-45-53 (1)
45-53: Missing return after firstfail()causes unreachable code to be type-checked.After handling
BuilderError, the code falls through to the generic error handler. Whilefail()terminates viaprocess.exit(), the secondfail()call is syntactically reachable. Add areturnfor explicit control flow.Suggested fix
} catch (error) { if (error instanceof BuilderError) { fail(error.code, error.message, { details: error.details, hint: error.hint }); + return; } fail( ErrorCodes.INTERNAL_ERROR, error instanceof Error ? error.message : 'Failed to remove node', ); }packages/cli/src/commands/action/result.ts-65-73 (1)
65-73: Add return after CLIError handling.Suggested fix
} catch (error) { if (error instanceof CLIError) { fail(error.code, error.message, { details: error.details, hint: error.hint }); + return; } fail( ErrorCodes.INTERNAL_ERROR, error instanceof Error ? error.message : 'Failed to get action result', ); }packages/cli/src/commands/workflow/abort.ts-23-31 (1)
23-31: Add return after CLIError handling.Suggested fix
} catch (error) { if (error instanceof CLIError) { fail(error.code, error.message, { details: error.details, hint: error.hint }); + return; } fail( ErrorCodes.INTERNAL_ERROR, error instanceof Error ? error.message : 'Failed to abort workflow', ); }packages/cli/src/commands/builder/remove-node.ts-19-27 (1)
19-27: Add explicit return afterfail()to prevent potential control flow issues.While
fail()returnsneverand callsprocess.exit(), TypeScript may not always infer this correctly in all contexts. Adding an explicitreturnafterfail()makes the control flow explicit and preventssessionfrom being potentiallynullon line 26.Suggested fix
if (!session) { fail(ErrorCodes.BUILDER_NOT_STARTED, 'No active builder session', { hint: 'refly builder start --name "your-workflow"', }); + return; }packages/cli/src/commands/workflow/run-get.ts-41-49 (1)
41-49: Add return after CLIError handling.Suggested fix
} catch (error) { if (error instanceof CLIError) { fail(error.code, error.message, { details: error.details, hint: error.hint }); + return; } fail( ErrorCodes.INTERNAL_ERROR, error instanceof Error ? error.message : 'Failed to get run status', ); }packages/cli/src/commands/workflow/create.ts-47-55 (1)
47-55: Add return after CLIError handling for explicit control flow.Same pattern issue as noted in other files - add explicit return after
fail().Suggested fix
} catch (error) { if (error instanceof CLIError) { fail(error.code, error.message, { details: error.details, hint: error.hint }); + return; } fail( ErrorCodes.INTERNAL_ERROR, error instanceof Error ? error.message : 'Failed to create workflow', ); }packages/cli/src/commands/workflow/create.ts-19-25 (1)
19-25: Add return afterfail()in JSON parse catch block.Without an explicit return, the code after the catch block is syntactically reachable, and
specwould beundefinedwhen used in the API call.Suggested fix
try { spec = JSON.parse(options.spec); } catch { fail(ErrorCodes.INVALID_INPUT, 'Invalid JSON in --spec', { hint: 'Ensure the spec is valid JSON', }); + return; }packages/cli/src/commands/workflow/list.ts-10-17 (1)
10-17: Removedescriptionfield fromWorkflowSummaryinterface.The API DTO's
WorkflowSummaryinterface (apps/api/src/modules/workflow/workflow-cli.dto.ts, lines 46-51) does not include adescriptionfield. The CLI should match the API contract to avoid confusion and ensure the interface accurately reflects what the API returns.packages/cli/src/commands/workflow/run.ts-22-36 (1)
22-36: Control flow issue:inputmay be used before assignment afterfail().The
fail()function returnsneverand exits the process, but TypeScript's control flow analysis doesn't track this across the try-catch. After the inner catch callsfail(), execution would never continue, but if it did,inputwould beundefined. Adding areturnafterfail()makes the intent clearer and satisfies stricter linting.Suggested fix
try { input = JSON.parse(options.input); } catch { fail(ErrorCodes.INVALID_INPUT, 'Invalid JSON in --input', { hint: 'Ensure the input is valid JSON', }); + return; // Unreachable but clarifies control flow }apps/api/src/modules/action/action-cli.controller.ts-1-30 (1)
1-30: Add OpenAPI documentation decorators to comply with API documentation requirements.The controller properly delegates to
ActionServicefor business logic and user-scoping—the service correctly filters results byuid: user.uidin the Prisma query. However, add OpenAPI decorators (@ApiTags,@ApiOperation,@ApiResponse,@ApiQuery) to document the endpoint per coding guidelines for API controllers.packages/cli/src/commands/builder/disconnect.ts-18-25 (1)
18-25: Stop control flow afterfail(...)to avoid relying onneverat runtime.If
fail(...)is ever mocked/non-exiting (tests),sessioncan beundefinedanddisconnectNodes(...)will crash.Proposed fix
const session = getCurrentSession(); if (!session) { fail(ErrorCodes.BUILDER_NOT_STARTED, 'No active builder session', { hint: 'refly builder start --name "your-workflow"', }); + return; }packages/cli/src/bin/refly.ts-29-40 (1)
29-40: Use Commander's.choices()to validate--formatat parse time.The current code casts
opts.format as OutputFormatwithout validating user input. Commander v12 supports.choices()for idiomatic validation: add.choices(['json', 'pretty', 'compact', 'plain'])to the--formatoption definition. This prevents invalid formats from reachingconfigureOutput()and aligns with Commander's recommended approach for restricting options to fixed values.packages/cli/src/commands/login.ts-372-378 (1)
372-378: Device flow incorrectly hardcodes provider as 'google'.The device flow doesn't involve an OAuth provider selection, yet it stores
provider: 'google'which is misleading. Consider using a dedicated value like'device'or omitting it.📝 Suggested fix
setOAuthTokens({ accessToken: statusResponse.accessToken, refreshToken: statusResponse.refreshToken, expiresAt: new Date(Date.now() + 3600000).toISOString(), // 1 hour - provider: 'google', // Device flow doesn't specify provider, default to google + provider: 'device', // Device-based authentication flow user: userInfo, });apps/api/src/modules/auth/device-auth.service.ts-271-286 (1)
271-286: Potential race condition in token delivery.Between checking
session.status === 'authorized'and clearing tokens, another poll request could retrieve the same tokens. Consider using a transaction or atomic update with a conditional check.🔒 Suggested atomic update
// Only include tokens if authorized if (session.status === 'authorized' && session.accessToken && session.refreshToken) { - result.accessToken = session.accessToken; - result.refreshToken = session.refreshToken; - - // Clear tokens from database after successful retrieval (one-time use) - await this.prisma.cliDeviceSession.update({ - where: { deviceId }, + // Atomically clear tokens and return them (one-time use) + const updated = await this.prisma.cliDeviceSession.updateMany({ + where: { + deviceId, + accessToken: { not: null }, + }, data: { accessToken: null, refreshToken: null, }, }); - this.logger.log(`[DEVICE_POLL] deviceId=${deviceId} tokens delivered`); + // Only return tokens if we were the one to clear them + if (updated.count > 0) { + result.accessToken = session.accessToken; + result.refreshToken = session.refreshToken; + this.logger.log(`[DEVICE_POLL] deviceId=${deviceId} tokens delivered`); + } }packages/cli/src/commands/login.ts-165-170 (1)
165-170: Backend doesn't provide token expiry in OAuth callback response; consider requesting this from backend.The
/v1/auth/cli/oauth/callbackendpoint doesn't return anexpiresInorexpiresAtfield (onlyaccessToken,refreshToken, anduser), so the 1-hour expiry is hardcoded. However, the device flow auth endpoint demonstrates that the backend can provide expiration info, and it would be more reliable to use backend-provided values rather than assuming a fixed lifetime. If token lifetimes differ from 1 hour, this could cause premature refresh attempts or stale tokens.apps/api/src/modules/workflow/workflow-cli.controller.ts-474-510 (1)
474-510: Fix paginationtotalto reflect all matching items, not just the current page.The
totalfield should return the count of all matching workflows after filtering, notworkflows.lengthwhich only contains the current page items. SincelistCanvases()applies filters (keyword, schedule status) before pagination, the total count can be significantly larger than the paginated result set.Either modify
listCanvases()to return both the paginated results and total count, or fetch the total separately before pagination.
| async initOAuth( | ||
| @Query('provider') provider: string, | ||
| @Query('port') port: string, | ||
| ): Promise<{ success: boolean; data: { authUrl: string; state: string } }> { | ||
| this.logger.log(`[CLI_OAUTH_INIT] provider=${provider}, port=${port}`); | ||
|
|
||
| if (!provider || !port) { | ||
| throw new BadRequestException('Provider and port are required'); | ||
| } | ||
|
|
||
| if (!['google', 'github'].includes(provider)) { | ||
| throw new BadRequestException('Provider must be google or github'); | ||
| } | ||
|
|
||
| try { | ||
| // Generate encrypted state token with port, timestamp, and nonce | ||
| const state = await this.authService.generateOAuthStateToken(port); | ||
|
|
||
| // Build redirect URL to localhost callback server | ||
| const redirectUri = `http://localhost:${port}/callback`; | ||
|
|
||
| // Generate OAuth URL based on provider | ||
| let authUrl: string; | ||
|
|
||
| if (provider === 'google') { | ||
| authUrl = await this.generateGoogleCliOAuthUrl(redirectUri, state); | ||
| } else if (provider === 'github') { | ||
| authUrl = await this.generateGithubCliOAuthUrl(redirectUri, state); | ||
| } else { | ||
| throw new BadRequestException('Unsupported provider'); | ||
| } | ||
|
|
||
| return buildSuccessResponse({ | ||
| authUrl, | ||
| state, | ||
| }); | ||
| } catch (error) { | ||
| this.logger.error(`[CLI_OAUTH_INIT] Failed: ${error.message}`, error.stack); | ||
| throw new OAuthError(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate port as numeric (1–65535) before building redirectUri (avoid malformed URI / injection).
Also applies to: 103-164
🤖 Prompt for AI Agents
In @apps/api/src/modules/auth/auth-cli.controller.ts around lines 54 - 94,
Validate the incoming port in initOAuth before using it to build redirectUri:
ensure the @Query('port') value is a numeric integer between 1 and 65535 (reject
otherwise with BadRequestException) and reject non-numeric or out-of-range
inputs to prevent malformed URIs or injection; apply the same validation logic
to any other CLI OAuth entrypoints that build a localhost redirect (e.g., the
other method referenced in lines 103-164). Locate initOAuth and related helpers
(generateOAuthStateToken, generateGoogleCliOAuthUrl, generateGithubCliOAuthUrl)
and perform the check immediately after reading the port, returning a clear
BadRequestException if invalid, and only then call generateOAuthStateToken and
build the redirectUri.
| /** | ||
| * Clean up expired device sessions | ||
| * Should be called periodically (e.g., by a cron job) | ||
| */ | ||
| async cleanupExpiredSessions(): Promise<number> { | ||
| const result = await this.prisma.cliDeviceSession.updateMany({ | ||
| where: { | ||
| status: 'pending', | ||
| expiresAt: { lt: new Date() }, | ||
| }, | ||
| data: { status: 'expired' }, | ||
| }); | ||
|
|
||
| if (result.count > 0) { | ||
| this.logger.log(`[DEVICE_CLEANUP] marked ${result.count} sessions as expired`); | ||
| } | ||
|
|
||
| return result.count; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for where cleanupExpiredSessions is called or scheduled
rg -n "cleanupExpiredSessions" --type=tsRepository: refly-ai/refly
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Search for cron jobs, intervals, or scheduled tasks in the codebase
rg -n "@Cron|@Interval|scheduleJob|setInterval|setTimeout" --type=ts -A 3 -B 1 | head -100Repository: refly-ai/refly
Length of output: 6852
🏁 Script executed:
#!/bin/bash
# Look for any TypeORM/NestJS scheduled decorators and tasks
rg -n "OnModuleInit|OnApplicationBootstrap|@Injectable.*Cron" --type=ts -A 5 | head -80Repository: refly-ai/refly
Length of output: 6755
Schedule the cleanupExpiredSessions method with a periodic task.
The cleanupExpiredSessions() method is defined but is never invoked anywhere in the codebase. Implement scheduling (e.g., using @Cron decorator or @Interval from @nestjs/schedule) to ensure expired device sessions are actually cleaned up periodically.
🤖 Prompt for AI Agents
In @apps/api/src/modules/auth/device-auth.service.ts around lines 291 - 309, The
cleanupExpiredSessions method is never scheduled; annotate it with a scheduling
decorator (e.g., add @Cron('0 * * * *') or @Interval(60_000) from
@nestjs/schedule) on the cleanupExpiredSessions method in DeviceAuthService so
it runs periodically, import the corresponding decorator at the top of
apps/api/src/modules/auth/device-auth.service.ts, and ensure the module that
provides DeviceAuthService imports ScheduleModule.forRoot() (or ScheduleModule)
so the scheduler is active; also keep the method as async and optionally add
try/catch logging around the body to surface scheduling errors.
| // 2. Create or get canvas | ||
| // Use skipDefaultNodes to avoid creating an extra version with default nodes | ||
| let canvasId = body.canvasId; | ||
| if (!canvasId) { | ||
| this.logger.log('[Apply] Creating new canvas with skipDefaultNodes'); | ||
| const canvas = await this.canvasService.createCanvas( | ||
| user, | ||
| { | ||
| canvasId: genCanvasID(), | ||
| title: body.title || workflowPlan.title || 'Workflow from Plan', | ||
| projectId: body.projectId, | ||
| }, | ||
| { skipDefaultNodes: true }, | ||
| ); | ||
| canvasId = canvas.canvasId; | ||
| this.logger.log(`[Apply] Created canvas: ${canvasId}`); | ||
| } else { | ||
| this.logger.log(`[Apply] Using existing canvas: ${canvasId}`); | ||
| } | ||
|
|
||
| // 3. Get tools and default model for node generation | ||
| const toolsData = await this.toolService.listTools(user, { enabled: true }); | ||
| const defaultModel = await this.providerService.findDefaultProviderItem( | ||
| user, | ||
| 'agent' as ModelScene, | ||
| ); | ||
| this.logger.log(`[Apply] Using ${toolsData?.length ?? 0} available tools`); | ||
|
|
||
| // 4. Convert WorkflowPlan to canvas nodes/edges | ||
| this.logger.log('[Apply] Generating canvas nodes and edges from plan'); | ||
| const { | ||
| nodes: generatedNodes, | ||
| edges, | ||
| variables, | ||
| } = generateCanvasDataFromWorkflowPlan(workflowPlan, toolsData ?? [], { | ||
| autoLayout: true, | ||
| defaultModel: defaultModel ? providerItem2ModelInfo(defaultModel as any) : undefined, | ||
| }); | ||
|
|
||
| // 4.1 Ensure start node exists (workflow entry point is required) | ||
| const nodes = ensureStartNode(generatedNodes as CanvasNode[]); | ||
| this.logger.log(`[Apply] Generated ${nodes.length} nodes and ${edges.length} edges`); | ||
|
|
||
| // 5. Update canvas state (full override) | ||
| // Use empty state base to avoid including default nodes from initEmptyCanvasState | ||
| const newState = { | ||
| ...initEmptyCanvasState(), | ||
| nodes, | ||
| edges, | ||
| }; | ||
|
|
||
| const stateStorageKey = await this.canvasSyncService.saveState(canvasId, newState); | ||
| this.logger.log(`[Apply] Canvas state saved with storage key: ${stateStorageKey}`); | ||
|
|
||
| // 6. Update canvas metadata | ||
| await this.prisma.canvas.update({ | ||
| where: { canvasId }, | ||
| data: { | ||
| version: newState.version, | ||
| workflow: JSON.stringify({ variables }), | ||
| }, | ||
| }); | ||
|
|
||
| // 7. Create canvas version record | ||
| await this.prisma.canvasVersion.create({ | ||
| data: { | ||
| canvasId, | ||
| version: newState.version, | ||
| hash: '', | ||
| stateStorageKey, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Critical: apply() can update arbitrary canvases if body.canvasId isn’t ownership-checked (IDOR) + Prisma bypass.
| export function saveSession(session: BuilderSession): void { | ||
| const sessionPath = getSessionPath(session.id); | ||
| const tempPath = path.join(getBuilderDir(), `.session-${session.id}-${Date.now()}.tmp`); | ||
|
|
||
| // Update timestamp | ||
| session.updatedAt = new Date().toISOString(); | ||
|
|
||
| // Validate before saving | ||
| const validated = BuilderSessionSchema.parse(session); | ||
|
|
||
| // Write to temp file first | ||
| fs.writeFileSync(tempPath, JSON.stringify(validated, null, 2), { | ||
| mode: 0o600, | ||
| }); | ||
|
|
||
| // Atomic rename | ||
| fs.renameSync(tempPath, sessionPath); | ||
| } |
There was a problem hiding this comment.
saveSession() likely breaks on Windows when overwriting an existing session file.
fs.renameSync(temp, sessionPath) can fail if sessionPath already exists (common on Windows), so subsequent saveSession() calls may throw.
Proposed fix (best-effort cross-platform overwrite)
export function saveSession(session: BuilderSession): void {
const sessionPath = getSessionPath(session.id);
const tempPath = path.join(getBuilderDir(), `.session-${session.id}-${Date.now()}.tmp`);
@@
// Write to temp file first
fs.writeFileSync(tempPath, JSON.stringify(validated, null, 2), {
mode: 0o600,
});
// Atomic rename
- fs.renameSync(tempPath, sessionPath);
+ try {
+ // Windows may not allow renaming over an existing file.
+ if (fs.existsSync(sessionPath)) {
+ fs.unlinkSync(sessionPath);
+ }
+ fs.renameSync(tempPath, sessionPath);
+ } catch (e) {
+ // Ensure temp file doesn't accumulate on failures.
+ try {
+ if (fs.existsSync(tempPath)) fs.unlinkSync(tempPath);
+ } catch {
+ // ignore
+ }
+ throw e;
+ }
}🤖 Prompt for AI Agents
In @packages/cli/src/builder/store.ts around lines 43 - 60, saveSession
currently does an atomic rename with fs.renameSync(tempPath, sessionPath) which
can fail on Windows if sessionPath already exists; update saveSession to detect
and remove an existing sessionPath before renaming (e.g., if
fs.existsSync(sessionPath) then try fs.unlinkSync(sessionPath) inside a
try/catch), then perform fs.renameSync(tempPath, sessionPath); ensure you still
write to tempPath, keep the BuilderSessionSchema.parse validation, and
handle/unwind errors around unlink+rename so the temp file is not left behind.
| update(current: number, message?: string): void { | ||
| this.current = current; | ||
| if (message) this.message = message; | ||
|
|
||
| if (!isTTY()) { | ||
| // Non-TTY: print only on completion or every 25% | ||
| const percent = Math.round((current / this.total) * 100); | ||
| if (percent % 25 === 0 || current === this.total) { | ||
| console.log( | ||
| `Progress: ${percent}% (${current}/${this.total})${message ? ` - ${message}` : ''}`, | ||
| ); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const percent = Math.round((this.current / this.total) * 100); | ||
| const filled = Math.round((this.current / this.total) * this.width); | ||
| const empty = this.width - filled; | ||
|
|
||
| const filledChar = this.useColor | ||
| ? styled('█'.repeat(filled), Style.TEXT_SUCCESS) | ||
| : '#'.repeat(filled); | ||
| const emptyChar = this.useColor ? styled('░'.repeat(empty), Style.TEXT_DIM) : '-'.repeat(empty); | ||
|
|
||
| let status = ''; | ||
| if (this.showPercentage) status += ` ${percent}%`; | ||
| if (this.showCount) status += ` (${this.current}/${this.total})`; | ||
| if (this.message) status += ` ${this.message}`; | ||
|
|
||
| process.stdout.write(`\r[${filledChar}${emptyChar}]${status}`); | ||
| } |
There was a problem hiding this comment.
ProgressBar can throw (divide-by-zero / negative repeat) when total <= 0 or current overshoots.
This can become a runtime RangeError (repeat with negative/Infinity) and break CLI output.
Proposed fix
update(current: number, message?: string): void {
- this.current = current;
+ const total = this.total;
+ const clampedCurrent =
+ total > 0 ? Math.min(Math.max(current, 0), total) : 0;
+ this.current = clampedCurrent;
if (message) this.message = message;
if (!isTTY()) {
// Non-TTY: print only on completion or every 25%
- const percent = Math.round((current / this.total) * 100);
- if (percent % 25 === 0 || current === this.total) {
+ const percent = total > 0 ? Math.round((clampedCurrent / total) * 100) : 0;
+ if (percent % 25 === 0 || (total > 0 && clampedCurrent === total)) {
console.log(
- `Progress: ${percent}% (${current}/${this.total})${message ? ` - ${message}` : ''}`,
+ `Progress: ${percent}% (${clampedCurrent}/${total})${
+ message ? ` - ${message}` : ''
+ }`,
);
}
return;
}
- const percent = Math.round((this.current / this.total) * 100);
- const filled = Math.round((this.current / this.total) * this.width);
- const empty = this.width - filled;
+ const percent = total > 0 ? Math.round((this.current / total) * 100) : 0;
+ const filledRaw = total > 0 ? Math.round((this.current / total) * this.width) : 0;
+ const filled = Math.min(Math.max(filledRaw, 0), this.width);
+ const empty = Math.max(0, this.width - filled);🤖 Prompt for AI Agents
In @packages/cli/src/utils/spinner.ts around lines 169 - 199, The update method
can throw RangeError when this.total <= 0 or when current overshoots; clamp and
guard numeric math: first normalize this.current = Math.min(Math.max(current,
0), Math.total) (or use this.total if total <= 0), compute percent using a
guarded expression (percent = this.total > 0 ? Math.round((this.current /
this.total) * 100) : 0), and compute filled as a bounded integer (filled =
this.total > 0 ? Math.max(0, Math.min(this.width, Math.round((this.current /
this.total) * this.width))) : (this.current > 0 ? this.width : 0)); then set
empty = this.width - filled; also use the same guarded percent in the non-TTY
branch so repeat() never receives negative/Infinity. Apply these changes inside
update in spinner.ts to prevent divide-by-zero and negative repeat arguments.
This pull request introduces significant enhancements to support CLI authentication and API key management, as well as new endpoints and supporting infrastructure for secure CLI access. The changes add new Prisma models for API keys and device authorization sessions, implement a robust API key service, and expose new controllers and endpoints for CLI clients. These updates lay the foundation for secure, auditable, and user-friendly CLI authentication and API key workflows.
Key changes include:
CLI Authentication & Device Authorization
CliDeviceSessionPrisma model to store pending device authorization requests for CLI login flows, supporting secure device-based authentication with status tracking and short-lived sessions.AuthServiceto ensure secure, replay-resistant CLI OAuth flows.API Key Management
UserApiKeyPrisma model for storing user API keys, including support for key rotation, expiration, revocation, and last-used tracking.ApiKeyServiceproviding methods to create, validate, list, and revoke API keys, with secure key hashing and base62 encoding.ApiKeyServiceas a global provider inAuthModulefor easy access across the application. [1] [2]CLI-Specific Endpoints
ActionCliControllerexposing a protected endpoint to fetch detailed action results for CLI tooling, and registered it in the module. [1] [2] [3]AuthCliController(implementation not shown here) to handle CLI authentication flows. [1] [2]These changes collectively enable secure, auditable, and user-centric CLI authentication and API key management, paving the way for robust CLI integrations.
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.