Fix audit IP fallback and CLI TypeScript regressions#66
Merged
weroperking merged 1 commit intomainfrom Apr 26, 2026
Merged
Conversation
WalkthroughThis PR modifies three independent modules: simplifies the init command's directory cleanup from individual file enumeration to recursive removal, switches CLI login password prompting from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/init.ts (1)
1341-1347:⚠️ Potential issue | 🟡 MinorSwallowed cleanup error proceeds with stale directory.
If
rmfails, the catch only logs and falls through tocopyIaCTemplate(projectPath). SincecopyIaCTemplatecallsmkdir(targetDir, { recursive: true }), the EEXIST guard there won't trip, and template files get written on top of whatever survived the failed cleanup — producing a corrupt project. Either rethrow or abort here.Proposed fix
// Clean up existing directory - try { - await rm(projectPath, { recursive: true, force: true }); - await mkdir(projectPath, { recursive: true }); - } catch (err) { - logger.error(`Failed to clean directory: ${err}`); - } + try { + await rm(projectPath, { recursive: true, force: true }); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error(`Failed to clean directory \`${projectName}\`: ${message}`); + }Note: the subsequent
mkdiris redundant —copyIaCTemplatealready doesmkdir(targetDir, { recursive: true }).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/init.ts` around lines 1341 - 1347, The cleanup catch currently logs rm(projectPath...) failures and continues, which can leave a stale directory and corrupt later copyIaCTemplate(projectPath) runs; update the try/catch around rm(projectPath, { recursive: true, force: true }) so that on error you abort (rethrow or throw a new Error) instead of falling through to copyIaCTemplate, and remove the redundant await mkdir(projectPath, { recursive: true }) since copyIaCTemplate already creates the directory; ensure references to projectPath, rm, mkdir and copyIaCTemplate are used so the change is applied in the same block.
🤖 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/src/index.ts`:
- Around line 569-583: The code should not silently coerce password to "" before
calling runApiKeyLogin; instead, after the inquirer prompt or env check ensure
password is a non-empty string and surface a local error if it's missing.
Replace the `password ?? ""` usage by explicitly validating the `password`
variable (the value read from process.env.ADMIN_PASSWORD or from the inquirer
prompt) and if it is undefined or an empty string throw or exit with a clear
message like "Admin password is required" so the issue is caught locally, then
pass the validated password into runApiKeyLogin (use the validated variable
directly, e.g., password or password! after the explicit check).
---
Outside diff comments:
In `@packages/cli/src/commands/init.ts`:
- Around line 1341-1347: The cleanup catch currently logs rm(projectPath...)
failures and continues, which can leave a stale directory and corrupt later
copyIaCTemplate(projectPath) runs; update the try/catch around rm(projectPath, {
recursive: true, force: true }) so that on error you abort (rethrow or throw a
new Error) instead of falling through to copyIaCTemplate, and remove the
redundant await mkdir(projectPath, { recursive: true }) since copyIaCTemplate
already creates the directory; ensure references to projectPath, rm, mkdir and
copyIaCTemplate are used so the change is applied in the same block.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2b58f17-3e65-478e-98d0-c733e27c687f
📒 Files selected for processing (3)
packages/cli/src/commands/init.tspackages/cli/src/index.tspackages/server/src/lib/audit.ts
Comment on lines
569
to
+583
| let password = process.env.ADMIN_PASSWORD; | ||
| if (!password) { | ||
| const { default: prompts } = await import("prompts"); | ||
| const result = await prompts({ | ||
| type: "password", | ||
| name: "password", | ||
| message: "Admin password:", | ||
| validate: (p: string) => p.length >= 1, | ||
| }); | ||
| const { default: inquirer } = await import("inquirer"); | ||
| const result = await inquirer.prompt<{ password: string }>([ | ||
| { | ||
| type: "password", | ||
| name: "password", | ||
| message: "Admin password:", | ||
| mask: "*", | ||
| validate: (value: string) => value.length >= 1 || "Password is required", | ||
| }, | ||
| ]); | ||
| password = result.password; | ||
| } | ||
| await runApiKeyLogin({ serverUrl: opts.url, email: opts.email, password }); | ||
| await runApiKeyLogin({ serverUrl: opts.url, email: opts.email, password: password ?? "" }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten the password type narrowing instead of ?? "".
password ?? "" is only there to satisfy TS narrowing (process.env.ADMIN_PASSWORD is string | undefined). With the inquirer validate enforcing length ≥ 1, an empty string is logically unreachable — but if the env-var branch ever sets ADMIN_PASSWORD="", the truthy check on Line 570 sends control into the prompt block, so that's also covered. The ?? "" therefore silently swallows any future bug where password legitimately becomes undefined and forwards "" to the server, which will fail z.string().min(1) with an opaque 400.
Prefer narrowing explicitly so a regression surfaces locally rather than as a server-side validation error:
Proposed narrowing
- if (opts.email) {
- const { runApiKeyLogin } = await import("./commands/login");
- let password = process.env.ADMIN_PASSWORD;
- if (!password) {
+ if (opts.email) {
+ const { runApiKeyLogin } = await import("./commands/login");
+ let password: string | undefined = process.env.ADMIN_PASSWORD;
+ if (!password) {
const { default: inquirer } = await import("inquirer");
const result = await inquirer.prompt<{ password: string }>([
{
type: "password",
name: "password",
message: "Admin password:",
mask: "*",
validate: (value: string) => value.length >= 1 || "Password is required",
},
]);
password = result.password;
}
- await runApiKeyLogin({ serverUrl: opts.url, email: opts.email, password: password ?? "" });
+ if (!password) {
+ logger.error("Password is required");
+ process.exit(1);
+ }
+ await runApiKeyLogin({ serverUrl: opts.url, email: opts.email, password });
} else {📝 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 password = process.env.ADMIN_PASSWORD; | |
| if (!password) { | |
| const { default: prompts } = await import("prompts"); | |
| const result = await prompts({ | |
| type: "password", | |
| name: "password", | |
| message: "Admin password:", | |
| validate: (p: string) => p.length >= 1, | |
| }); | |
| const { default: inquirer } = await import("inquirer"); | |
| const result = await inquirer.prompt<{ password: string }>([ | |
| { | |
| type: "password", | |
| name: "password", | |
| message: "Admin password:", | |
| mask: "*", | |
| validate: (value: string) => value.length >= 1 || "Password is required", | |
| }, | |
| ]); | |
| password = result.password; | |
| } | |
| await runApiKeyLogin({ serverUrl: opts.url, email: opts.email, password }); | |
| await runApiKeyLogin({ serverUrl: opts.url, email: opts.email, password: password ?? "" }); | |
| let password: string | undefined = process.env.ADMIN_PASSWORD; | |
| if (!password) { | |
| const { default: inquirer } = await import("inquirer"); | |
| const result = await inquirer.prompt<{ password: string }>([ | |
| { | |
| type: "password", | |
| name: "password", | |
| message: "Admin password:", | |
| mask: "*", | |
| validate: (value: string) => value.length >= 1 || "Password is required", | |
| }, | |
| ]); | |
| password = result.password; | |
| } | |
| if (!password) { | |
| logger.error("Password is required"); | |
| process.exit(1); | |
| } | |
| await runApiKeyLogin({ serverUrl: opts.url, email: opts.email, password }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/index.ts` around lines 569 - 583, The code should not
silently coerce password to "" before calling runApiKeyLogin; instead, after the
inquirer prompt or env check ensure password is a non-empty string and surface a
local error if it's missing. Replace the `password ?? ""` usage by explicitly
validating the `password` variable (the value read from
process.env.ADMIN_PASSWORD or from the inquirer prompt) and if it is undefined
or an empty string throw or exit with a clear message like "Admin password is
required" so the issue is caught locally, then pass the validated password into
runApiKeyLogin (use the validated variable directly, e.g., password or password!
after the explicit check).
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.
Motivation
getClientIpproduced a UA fingerprint instead of returning"unknown"when no IP/user-agent headers are present.promptsimport in the CLIinit/login flows.Description
getClientIpinpackages/server/src/lib/audit.tsto preferx-forwarded-for, thenx-real-ip, and return"unknown"when no proxy or user-agent data exists instead of always generating a UA fingerprint.packages/cli/src/commands/init.tswithnode:fs/promisesrm(..., { recursive: true, force: true })andmkdir(..., { recursive: true })to avoid Bun-specific APIs that broke typechecking.promptsin the CLI login flow with the existinginquirerprompt inpackages/cli/src/index.tsand ensured thepasswordpassed torunApiKeyLoginis always a string.Testing
bun test packages/server/test/audit.test.tsand all audit tests passed.bun run --cwd packages/server testand observed no failures for the server package.bun run --cwd packages/cli typecheck(tsc -p tsconfig.json --noEmit) and the typecheck passed.Codex Task
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes