CLI fixes#8
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughAdds a Node-based binary launcher with download/verify/cache/spawn flow; migrates Bun I/O and spawn to Node streams and child_process; implements email auth commands and setup flow; updates publish workflow and package scripts; refactors CLI commands, local-desktop/profile utilities, logo rendering, and tests. ChangesRelease & package publishing
Binary distribution & Node runtime migration
Email-based authentication and setup
CLI simplifications, local-desktop, profiles, and logo
Tests and E2E harness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 6
🧹 Nitpick comments (1)
.github/workflows/publish-release.yml (1)
29-30: ⚡ Quick winAvoid building
beeper-clitwice in the release job.
binaryalready runsbun run build, so the explicit build step before it is redundant and increases publish time.♻️ Proposed simplification
- - name: Build package - run: bun run --filter beeper-cli build - name: Publish GitHub release env: GH_TOKEN: ${{ github.token }}Also applies to: 41-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish-release.yml around lines 29 - 30, The workflow is building beeper-cli twice: remove the redundant "Build package" step(s) that run "bun run --filter beeper-cli build" (the one named "Build package" and the duplicate at lines 41-42) because the "binary" step already runs "bun run build"; delete those explicit build steps so the release job only builds once and ensure the "binary" step remains to perform the actual build and packaging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli-plugin-cloudflare/src/lib/cloudflared.ts`:
- Line 196: The current download materializes the whole response via
arrayBuffer() and writeFile(to, ...) which can spike memory; replace that call
with a streaming pipeline: import pipeline from "stream/promises" and
createWriteStream from "fs" and then await pipeline(response.body,
createWriteStream(to)) so the binary is streamed to disk and memory usage is
bounded — update the spot where writeFile(to, Buffer.from(await
response.arrayBuffer())) is used, referencing the existing response and to
variables.
In `@packages/cli/bin/run.js`:
- Line 20: The default releaseBaseURL currently hardcodes the wrong GitHub repo
("beeper/desktop-api-cli") so installs 404; update the default to point at this
repository's releases by replacing that hardcoded path with the repository that
publishes the assets, e.g. construct the URL using the GITHUB_REPOSITORY env or
the correct repo name and releaseTag (use process.env.GITHUB_REPOSITORY ||
'owner/repo' in the template) while preserving the existing
BEEPER_CLI_RELEASE_BASE_URL override and the trailing-slash trim logic; update
the releaseBaseURL declaration and keep the variable/releaseTag names unchanged.
- Around line 95-103: The download() function can recurse indefinitely on cyclic
redirects; add a redirect depth cap by introducing a counter (e.g.,
options._redirectCount or options.maxRedirects defaulting to 10) and check it at
the start of download(), rejecting the Promise if the count exceeds the limit;
when following a redirect inside download() call the same function but pass an
incremented redirect count so each recursive call tracks depth and stops after
the configured maxRedirects.
In `@packages/cli/src/commands/accounts/add.ts`:
- Around line 101-102: The selection logic currently uses
Number.parseInt(answer, 10) which allows inputs like "1abc" to be treated as 1;
update the validation so the answer is accepted only if it is entirely numeric
before parsing. Specifically, in the block that sets selected from answer and
validates against available (the variables answer, selected, and available),
first verify answer matches a full-digit pattern (e.g., /^\d+$/) or use a strict
Number conversion that rejects non-numeric characters, then parse to an integer
and apply the existing bounds check (selected >= 1 && selected <=
available.length) before returning available[selected - 1]!.id.
In `@packages/cli/src/lib/installations.ts`:
- Around line 363-365: The writeResponseToFile helper currently buffers the
entire response via response.arrayBuffer(); change writeResponseToFile to stream
the response to disk using a writable stream (e.g., createWriteStream) and the
streaming reader from response.body.getReader(), mirroring the pattern used in
export/index.ts (use the reader loop to read chunks and write them to the file
until done); ensure you handle reader read() results, close/finish the write
stream on completion, and propagate/throw any errors instead of buffering the
whole payload in memory.
In `@scripts/publish-packages.ts`:
- Line 4: Replace any fragile string-based path stripping (e.g., code that
removes "/package.json" or uses replace to derive a package directory) with
proper path dirname usage: import dirname from "node:path" and call
dirname(packageJsonPath) (or, if you are starting from an ESM URL, first convert
with fileURLToPath and then dirname) to compute the package directory; update
both the occurrence near the import of join and the similar occurrence around
line 71 so all package directory resolutions use dirname instead of manual
string replacement.
---
Nitpick comments:
In @.github/workflows/publish-release.yml:
- Around line 29-30: The workflow is building beeper-cli twice: remove the
redundant "Build package" step(s) that run "bun run --filter beeper-cli build"
(the one named "Build package" and the duplicate at lines 41-42) because the
"binary" step already runs "bun run build"; delete those explicit build steps so
the release job only builds once and ensure the "binary" step remains to perform
the actual build and packaging.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bad7dfb-e8fd-4f61-90d1-60df6f7079e2
📒 Files selected for processing (22)
.github/workflows/publish-release.ymlpackage.jsonpackages/cli-plugin-cloudflare/package.jsonpackages/cli-plugin-cloudflare/src/lib/cloudflared.tspackages/cli/bin/binary-bootstrap.jspackages/cli/bin/cli.jspackages/cli/bin/run.jspackages/cli/package.jsonpackages/cli/scripts/build-binaries.tspackages/cli/src/commands.generated.tspackages/cli/src/commands/accounts/add.tspackages/cli/src/commands/auth/email/response.tspackages/cli/src/commands/auth/email/start.tspackages/cli/src/commands/contacts/list.tspackages/cli/src/commands/setup.tspackages/cli/src/commands/update.tspackages/cli/src/lib/installations.tspackages/cli/src/lib/matrix-direct.tspackages/cli/src/lib/runner.tspackages/cli/src/lib/setup-login.tspackages/cli/test/e2e-staging.tsscripts/publish-packages.ts
📜 Review details
🔇 Additional comments (22)
packages/cli/src/lib/setup-login.ts (1)
1-79: LGTM!packages/cli/src/commands/auth/email/response.ts (1)
1-29: LGTM!packages/cli/src/commands/auth/email/start.ts (1)
1-19: LGTM!packages/cli/src/commands/setup.ts (1)
10-10: LGTM!Also applies to: 36-37, 51-53, 77-80, 148-151, 209-209, 214-214, 328-334
packages/cli/src/commands.generated.ts (1)
9-101: LGTM!Also applies to: 112-204
packages/cli/src/lib/matrix-direct.ts (1)
1-1: LGTM!Also applies to: 15-19
packages/cli/src/commands/contacts/list.ts (1)
6-6: LGTM!Also applies to: 24-24
packages/cli/src/commands/accounts/add.ts (1)
41-46: LGTM!Also applies to: 87-100, 103-110
packages/cli/test/e2e-staging.ts (6)
12-12: LGTM!
268-272: LGTM!Also applies to: 278-278, 296-296
320-321: LGTM!Also applies to: 771-772
415-416: LGTM!
660-661: LGTM!
669-704: LGTM!packages/cli/src/lib/runner.ts (1)
1-2: LGTM!Also applies to: 11-19, 26-39
packages/cli/src/commands/update.ts (1)
131-131: LGTM!Also applies to: 135-135
package.json (1)
2-2: LGTM!Also applies to: 19-19
packages/cli-plugin-cloudflare/package.json (1)
3-3: LGTM!Also applies to: 37-37
packages/cli/package.json (1)
8-8: LGTM!Also applies to: 30-30, 57-57
packages/cli/bin/binary-bootstrap.js (1)
14-14: LGTM!packages/cli/bin/cli.js (1)
1-11: LGTM!packages/cli/scripts/build-binaries.ts (1)
4-4: LGTM!Also applies to: 61-61, 83-83
|
@indent review |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/README.md`:
- Line 434: The markdown table cell containing the flag string
`--server-env=<production|staging>` is breaking the table because the pipe
character is unescaped; update that cell to escape or wrap the pipe (for example
replace `--server-env=<production|staging>` with an escaped form like
`--server-env=<production\|staging>` or wrap it in backticks/code span that
preserves the pipe) so the table columns remain valid and MD056 is resolved.
In `@packages/cli/src/commands/chats/start.ts`:
- Line 20: Remove the "as any" cast on the payload passed to client.chats.start
and replace it with the proper request type imported from "`@beeper/desktop-api`"
(use the start request/interface name used by other chat methods), construct an
object matching that typed shape with accountID, user, and title (ensuring field
names and types match the imported request type), and pass that typed object to
client.chats.start so TypeScript can validate the call.
In `@packages/cli/src/commands/setup.ts`:
- Around line 591-596: installWithCopy currently ignores the user-provided
channel and server environment by hardcoding 'stable' and 'production'; update
the call sites inside installWithCopy to pass the user flags (use flags.channel
for channel and flags['server-env'] for serverEnv) when calling installDesktop
and installServer, with sensible fallbacks if those flags are undefined so
behavior remains unchanged for existing callers; reference the installWithCopy
function and the installDesktop/installServer calls so you modify the correct
calls.
In `@packages/cli/src/commands/verify/reset-recovery-key.ts`:
- Around line 15-19: The code violates statement-padding: add a blank line after
the if-block that writes the recovery key and prompts (the block that references
flags.yes and promptYesNoDefaultYes) and add another blank line before the
subsequent call to client.app.login.verification.recoveryKey.reset.confirm so
there is one blank line separating the if statement from the next statement;
modify the area around process.stderr.write / promptYesNoDefaultYes and the line
with const confirmed = await
client.app.login.verification.recoveryKey.reset.confirm(...) to insert those
blank lines.
In `@packages/cli/src/lib/profiles.ts`:
- Around line 85-98: Add the required blank-line separations to satisfy the
statement-padding lint rule: insert a blank line before the "if
(process.platform === 'win32')" block (the Windows detection block), add a blank
line after that block's closing brace (i.e. between the Windows for-loop that
checks candidates and the subsequent Unix for-loop over
['/usr/bin/beeper','/usr/local/bin/beeper']), and insert a blank line before the
final "return undefined" statement; these changes touch the block containing
process.platform === 'win32', the candidate path for-loop, the Unix paths
for-loop, and the final return.
- Around line 95-97: The code that probes for Linux beeper binaries (the loop
over ['/usr/bin/beeper', '/usr/local/bin/beeper'] and the pathExists checks)
runs on all OSes and can wrongly pick up a macOS binary; guard this probe by
checking process.platform === 'linux' (or invert with if (process.platform !==
'linux') skip) before iterating those paths so only Linux attempts return those
paths; update the logic around the pathExists loop in profiles.ts (the for (...)
{ if (await pathExists(path)) return path }) accordingly to avoid returning
Linux-specific locations on non-Linux platforms.
In `@packages/cli/test/e2e-staging.ts`:
- Around line 520-531: The current check (if (target.kind !== 'remote')) lets
desktop targets hit the server-only "targets restart" path; change the condition
to only run the restart block for server targets (e.g., if (target.kind ===
'server') or the appropriate server-identifying property on target) and skip or
record a no-op for others; update usages in this block (runCli(args...),
recordCommand, recordCoverage, waitForInfo, and recordFailure) to only execute
when the target is a server so desktop targets don't trigger control-surface
failures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8966edb0-2b66-4297-9547-cfc2f1154a82
📒 Files selected for processing (14)
packages/cli/README.mdpackages/cli/src/commands/chats/show.tspackages/cli/src/commands/chats/start.tspackages/cli/src/commands/messages/list.tspackages/cli/src/commands/send/text.tspackages/cli/src/commands/setup.tspackages/cli/src/commands/verify/reset-recovery-key.tspackages/cli/src/lib/logo.tspackages/cli/src/lib/manifest.tspackages/cli/src/lib/matrix-direct.tspackages/cli/src/lib/profiles.tspackages/cli/test/cli-smoke.tspackages/cli/test/e2e-staging.tspackages/cli/test/messages-search-validation.test.ts
💤 Files with no reviewable changes (1)
- packages/cli/src/lib/matrix-direct.ts
✅ Files skipped from review due to trivial changes (2)
- packages/cli/src/lib/manifest.ts
- packages/cli/test/messages-search-validation.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (macos-latest / bun 1.3.10)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
- GitHub Check: test (macos-latest / bun 1.3.10)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
🧰 Additional context used
🪛 ESLint
packages/cli/src/commands/verify/reset-recovery-key.ts
[error] 15-15: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 19-19: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
packages/cli/src/lib/profiles.ts
[error] 85-85: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 95-95: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 98-98: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
packages/cli/src/commands/chats/start.ts
[error] 20-20: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
packages/cli/test/e2e-staging.ts
[error] 153-153: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 260-260: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 291-291: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 443-443: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 444-444: Do not call Array#push() multiple times.
(unicorn/no-array-push-push)
[error] 488-488: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 520-520: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 520-520: Unexpected negated condition.
(unicorn/no-negated-condition)
[error] 553-553: Prefer .findLast(…) over .filter(…).at(-1).
(unicorn/prefer-array-find)
[error] 648-648: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 709-709: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 821-821: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 841-841: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 861-861: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 918-918: Do not use useless undefined.
(unicorn/no-useless-undefined)
[error] 920-920: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
packages/cli/src/commands/setup.ts
[error] 42-48: Unexpected blank line between class members.
(@stylistic/lines-between-class-members)
[error] 84-84: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 99-99: Use object destructuring.
(prefer-destructuring)
[error] 104-104: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 108-108: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 113-113: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 119-149: Unnecessary 'else' after 'return'.
(no-else-return)
[error] 158-158: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 160-160: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 218-219: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 232-232: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 266-266: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 272-272: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 292-292: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 298-298: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 408-408: Do not use useless undefined.
(unicorn/no-useless-undefined)
[error] 474-474: Use object destructuring.
(prefer-destructuring)
[error] 499-499: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 573-573: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 576-576: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 594-595: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 601-601: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 602-602: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 626-626: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 639-639: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 653-653: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 724-724: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
🪛 markdownlint-cli2 (0.22.1)
packages/cli/README.md
[warning] 434-434: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🔇 Additional comments (5)
packages/cli/src/lib/logo.ts (1)
1-97: LGTM!packages/cli/src/commands/send/text.ts (1)
31-31: LGTM!packages/cli/src/commands/chats/show.ts (1)
14-14: LGTM!packages/cli/test/cli-smoke.ts (1)
12-12: LGTM!Also applies to: 50-52, 175-176, 221-245
packages/cli/src/commands/messages/list.ts (1)
26-26: LGTM!
|
@indent review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 209-212: The code currently persists the remote target
(writeTarget and updateConfig) before performing authentication, which may leave
an unauthenticated/invalid target saved if setupEmailTarget or setupOAuthTarget
fails or is cancelled; move the calls to writeTarget and updateConfig to after a
successful auth flow: call setupEmailTarget or setupOAuthTarget first (using the
same parameters and keeping 'remote-oauth' tag), verify the returned result
indicates success, then persist the target with writeTarget(target) and
updateConfig to set defaultTarget only if flags.target is not set; still call
this.printSetupResult(result, flags) at the end. Ensure references: writeTarget,
updateConfig, setupEmailTarget, setupOAuthTarget, printSetupResult, and
flags.target are updated accordingly.
In `@packages/cli/src/lib/local-desktop.ts`:
- Around line 46-75: localDesktopReadiness currently treats any case besides
firstSyncDone === false as ready; change it to compute a single readiness
boolean that requires firstSyncDone === true and all critical parsed E2EE flags
to be true (use the same helpers booleanValue(...) and recordValue(...) on
session.state to derive initialized, secret_storage, cross_signing, verified,
key_backup, has_backed_up_code and the secrets
master_key/self_signing_key/user_signing_key/megolm_backup_key/recovery_code),
then set state = allGood ? 'ready' : 'needs-first-sync' (or another non-ready
state) and keep actions/message consistent so incomplete local states won't be
surfaced as ready in localDesktopReadiness.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c200bd9d-aa47-481f-949f-191a55539a09
📒 Files selected for processing (2)
packages/cli/src/commands/setup.tspackages/cli/src/lib/local-desktop.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (macos-latest / bun 1.3.10)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
🧰 Additional context used
🪛 ESLint
packages/cli/src/lib/local-desktop.ts
[error] 92-92: Do not use useless undefined.
(unicorn/no-useless-undefined)
[error] 96-96: arrow function is equivalent to Boolean. Use Boolean directly.
(unicorn/prefer-native-coercion-functions)
packages/cli/src/commands/setup.ts
[error] 42-48: Unexpected blank line between class members.
(@stylistic/lines-between-class-members)
[error] 84-84: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 99-99: Use object destructuring.
(prefer-destructuring)
[error] 104-104: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 108-108: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 113-113: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 119-149: Unnecessary 'else' after 'return'.
(no-else-return)
[error] 158-158: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 160-160: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 218-219: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 232-232: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 266-266: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 272-272: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 292-292: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 298-298: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 406-406: Do not use useless undefined.
(unicorn/no-useless-undefined)
[error] 472-472: Use object destructuring.
(prefer-destructuring)
[error] 497-497: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 571-571: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 574-574: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 592-593: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 599-599: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 600-600: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 624-624: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 637-637: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 651-651: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 722-722: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
🔇 Additional comments (1)
packages/cli/src/commands/setup.ts (1)
589-594:installWithCopystill ignores the caller's install flags.This still hardcodes
stable/production, so--channeland--server-envare dropped duringsetup --install.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/cli/src/commands/setup.ts (1)
209-212:⚠️ Potential issue | 🟡 Minor | ⚖️ Poor tradeoffRemote target is persisted before authentication completes.
writeTarget(target)andupdateConfig()are called immediately aftersetupEmailTarget/setupOAuthTargetreturns on line 209, but if the auth flow inside those functions throws (e.g., user cancels OAuth, email verification fails), the target is still saved. This can leave an unauthenticated remote as the default target.Consider moving
writeTargetandupdateConfigcalls inside the auth success path, or restructure so persistence only happens after confirmed success.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/setup.ts` around lines 209 - 212, The target is being persisted before authentication completes: move the persistence calls so writeTarget(target) and the updateConfig(...) that sets defaultTarget only run after setupEmailTarget or setupOAuthTarget has confirmed success; specifically, have setupEmailTarget/setupOAuthTarget return a success indicator or the fully authenticated target (used by printSetupResult) and call writeTarget and updateConfig only when that success result is truthy (e.g., inside the success path returned from setupEmailTarget/setupOAuthTarget) rather than immediately after their invocation to avoid saving unauthenticated/partial targets.
🧹 Nitpick comments (1)
packages/cli/src/commands/accounts/add.ts (1)
185-205: 💤 Low valueApply the same strict numeric validation to
chooseLoginFlow.Line 196 uses
Number.parseInt(answer, 10)without the same/^\d+$/.test(answer)guard that was added tochooseAccountType. Input like1abcwill be interpreted as1.Proposed fix
- const selected = Number.parseInt(answer, 10) + const selected = /^\d+$/.test(answer) ? Number.parseInt(answer, 10) : Number.NaN if (Number.isInteger(selected) && selected >= 1 && selected <= flows.length) return flows[selected - 1]!.id🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/accounts/add.ts` around lines 185 - 205, The numeric parsing in chooseLoginFlow is too permissive: before using Number.parseInt(answer, 10) validate the input with /^\d+$/.test(answer) (or equivalent) so strings like "1abc" are rejected; if the regex passes, parse and check range against flows.length and return flows[selected - 1].id, otherwise fall back to the byID lookup and the existing error message. Ensure you update the answer handling inside chooseLoginFlow (the answer variable and the numeric selection branch) to use this strict numeric guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/test/e2e-staging.ts`:
- Around line 283-304: The group chat is only created with distinctReceivers[0]
but later you assert visibility for two receivers (the for-loop over [sender,
...distinctReceivers.slice(0, 2)]), causing a false negative for the second
receiver; update the setup so both receivers are added before assertions—either
include distinctReceivers[1].matrix.userID when building startArgs (the variable
startArgs used to run runCli and produce chatID) or explicitly run the
invite/join CLI for distinctReceivers[1] (using the same runCli pattern) after
chatID is available and before sending the message and running the messages list
checks. Ensure chatID is used consistently when adding the second receiver so
the subsequent loop can validate both receivers.
In `@scripts/publish-packages.ts`:
- Around line 9-10: The help flag never matches because flags only collects args
starting with "--"; change the filters so single-dash flags are captured: build
flags from args.filter(arg => arg.startsWith("-")) (or normalize by stripping
leading dashes) and build positional from args.filter(arg =>
!arg.startsWith("-")), so that flags.has("-h") (and any other single-dash
checks) will be true; update the declarations for the flags and positional
variables to use this broader startsWith("-") rule so the existing help check
(flags.has("-h")) works as intended.
- Around line 126-140: The final summary always uses ordered.length which
overcounts when skipExisting causes continues; fix by adding a publishedCount
variable (e.g., let publishedCount = 0) before the for loop and incrementing it
only when you actually call run(command, { cwd: pkg.dir }) to publish (or when
you simulate a publish in dryRun), then change the final console.log to print
publishedCount (e.g., dryRun ? "Dry run complete." : `Published
${publishedCount} package(s) at ${version}.`) so the summary reflects actual
published packages; touch the loop that references ordered, skipExisting, pkg,
run, version, and dryRun.
---
Duplicate comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 209-212: The target is being persisted before authentication
completes: move the persistence calls so writeTarget(target) and the
updateConfig(...) that sets defaultTarget only run after setupEmailTarget or
setupOAuthTarget has confirmed success; specifically, have
setupEmailTarget/setupOAuthTarget return a success indicator or the fully
authenticated target (used by printSetupResult) and call writeTarget and
updateConfig only when that success result is truthy (e.g., inside the success
path returned from setupEmailTarget/setupOAuthTarget) rather than immediately
after their invocation to avoid saving unauthenticated/partial targets.
---
Nitpick comments:
In `@packages/cli/src/commands/accounts/add.ts`:
- Around line 185-205: The numeric parsing in chooseLoginFlow is too permissive:
before using Number.parseInt(answer, 10) validate the input with
/^\d+$/.test(answer) (or equivalent) so strings like "1abc" are rejected; if the
regex passes, parse and check range against flows.length and return
flows[selected - 1].id, otherwise fall back to the byID lookup and the existing
error message. Ensure you update the answer handling inside chooseLoginFlow (the
answer variable and the numeric selection branch) to use this strict numeric
guard.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48ef1475-225d-4962-9866-98b1b3051dfa
📒 Files selected for processing (15)
.github/workflows/publish-release.ymlpackages/cli-plugin-cloudflare/src/lib/cloudflared.tspackages/cli/README.mdpackages/cli/bin/run.jspackages/cli/scripts/generate-readme.tspackages/cli/src/commands/accounts/add.tspackages/cli/src/commands/chats/start.tspackages/cli/src/commands/setup.tspackages/cli/src/commands/verify/reset-recovery-key.tspackages/cli/src/lib/installations.tspackages/cli/src/lib/local-desktop.tspackages/cli/src/lib/profiles.tspackages/cli/test/cli-smoke.tspackages/cli/test/e2e-staging.tsscripts/publish-packages.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/scripts/generate-readme.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/cli/src/lib/profiles.ts
- .github/workflows/publish-release.yml
- packages/cli/src/commands/chats/start.ts
- packages/cli/test/cli-smoke.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
- GitHub Check: test (macos-latest / bun 1.3.10)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
🧰 Additional context used
🪛 ESLint
packages/cli/src/commands/accounts/add.ts
[error] 41-41: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 89-89: Use .length === 0 when checking length is zero.
(unicorn/explicit-length-check)
[error] 92-92: Use for…of instead of .forEach(…).
(unicorn/no-array-for-each)
packages/cli/bin/run.js
[error] 14-14: Use object destructuring.
(prefer-destructuring)
[error] 34-34: Don't use process.exit(); throw an error instead.
(n/no-process-exit)
[error] 38-38: Don't use process.exit(); throw an error instead.
(n/no-process-exit)
[error] 42-42: Don't use process.exit(); throw an error instead.
(n/no-process-exit)
[error] 54-54: Do not use useless undefined.
(unicorn/no-useless-undefined)
[error] 106-106: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 111-111: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
packages/cli/src/commands/setup.ts
[error] 42-48: Unexpected blank line between class members.
(@stylistic/lines-between-class-members)
[error] 84-84: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 99-99: Use object destructuring.
(prefer-destructuring)
[error] 104-104: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 108-108: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 113-113: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 119-149: Unnecessary 'else' after 'return'.
(no-else-return)
[error] 158-158: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 160-160: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 202-202: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 218-219: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 232-232: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 266-266: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 272-272: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 292-292: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 298-298: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 406-406: Do not use useless undefined.
(unicorn/no-useless-undefined)
[error] 472-472: Use object destructuring.
(prefer-destructuring)
[error] 497-497: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 571-571: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 574-574: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 594-595: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 601-601: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 602-602: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 626-626: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 639-639: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 653-653: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 724-724: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
packages/cli/src/lib/local-desktop.ts
[error] 107-107: Do not use useless undefined.
(unicorn/no-useless-undefined)
[error] 111-111: arrow function is equivalent to Boolean. Use Boolean directly.
(unicorn/prefer-native-coercion-functions)
packages/cli/test/e2e-staging.ts
[error] 153-153: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 260-260: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 291-291: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 443-443: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 444-444: Do not call Array#push() multiple times.
(unicorn/no-array-push-push)
[error] 488-488: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 520-520: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 553-553: Prefer .findLast(…) over .filter(…).at(-1).
(unicorn/prefer-array-find)
[error] 648-648: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 709-709: Use .length > 0 when checking length is not zero.
(unicorn/explicit-length-check)
[error] 821-821: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 841-841: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 861-861: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 918-918: Do not use useless undefined.
(unicorn/no-useless-undefined)
[error] 920-920: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
scripts/publish-packages.ts
[error] 10-10: Prefer .find(…) over .filter(…).
(unicorn/prefer-array-find)
[error] 27-27: Don't use process.exit(); throw an error instead.
(n/no-process-exit)
[error] 32-32: Unexpected prompt.
(no-alert)
[error] 32-32: 'prompt' is not defined.
(no-undef)
[error] 38-38: Don't use process.exit(); throw an error instead.
(n/no-process-exit)
[error] 58-58: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
[error] 115-115: Unexpected negated condition.
(unicorn/no-negated-condition)
🔇 Additional comments (12)
packages/cli-plugin-cloudflare/src/lib/cloudflared.ts (1)
1-7: LGTM!Also applies to: 197-201
packages/cli/src/lib/installations.ts (1)
1-7: LGTM!Also applies to: 213-223, 367-371
packages/cli/src/commands/accounts/add.ts (1)
87-110: LGTM!packages/cli/src/commands/verify/reset-recovery-key.ts (1)
4-27: LGTM!packages/cli/README.md (1)
333-334: LGTM!Also applies to: 426-435, 909-951
packages/cli/bin/run.js (1)
1-121: LGTM!packages/cli/src/commands/setup.ts (3)
38-39: LGTM!Also applies to: 84-87, 190-193, 470-476
589-597: LGTM!
338-423: LGTM!packages/cli/src/lib/local-desktop.ts (3)
12-44: LGTM!Also applies to: 46-92
106-113: LGTM!
140-151: LGTM!Also applies to: 180-182
| const startArgs = ['chats', 'start', distinctReceivers[0].matrix.userID, '--target', sender.name, '--account', 'matrix', '--title', `CLI E2E ${runID}`, '--json'] | ||
| const start = runCli(startArgs, { env: { BEEPER_ACCESS_TOKEN: sender.accessToken }, allowFailure: true }) | ||
| recordCommand('messaging-group', startArgs, start) | ||
| const chatID = parseEnvelope(start.stdout)?.data?.chat?.id ?? parseEnvelope(start.stdout)?.data?.id ?? parseEnvelope(start.stdout)?.data?.chatID | ||
| if (!chatID) { | ||
| recordFailure('messaging-group', sender, 'Could not create Matrix group room through the raw API fallback.') | ||
| recordBlock('messaging-group', sender, 'Could not create a group chat through the Desktop API chat surface. Raw Matrix createRoom/join is intentionally not used.') | ||
| return | ||
| } | ||
| report.coverage.groupChatID = chatID | ||
|
|
||
| const text = `group staging e2e ${runID}` | ||
| const sendArgs = ['send', 'text', '--to', chatID, '--message', text, '--target', sender.name, '--json'] | ||
| const send = runCli(sendArgs, { env: { BEEPER_ACCESS_TOKEN: sender.accessToken }, allowFailure: true }) | ||
| recordCommand('messaging-group', sendArgs, send) | ||
| await sleep(1000) | ||
|
|
||
| for (const target of [sender, ...receivers.slice(0, 2)]) { | ||
| for (const target of [sender, ...distinctReceivers.slice(0, 2)]) { | ||
| const listArgs = ['messages', 'list', '--chat', chatID, '--target', target.name, '--limit', '10', '--json'] | ||
| const list = runCli(listArgs, { env: { BEEPER_ACCESS_TOKEN: target.accessToken }, allowFailure: true }) | ||
| recordCommand('messaging-group', listArgs, list) | ||
| if (list.status !== 0) recordFailure('messaging-group', target, `group message list failed for ${target.name}`) | ||
| if (list.status !== 0 && /not in room|M_FORBIDDEN/i.test(`${list.stderr}${list.stdout}`)) { | ||
| recordBlock('messaging-group', target, 'Group room invite was created, but this target has not joined the room yet.') |
There was a problem hiding this comment.
Include the second receiver before asserting on it.
Line 283 only creates the chat with distinctReceivers[0], but Line 299 verifies message visibility for the first two receivers. That makes the second receiver a guaranteed false negative unless they were added elsewhere, so this coverage path can fail even when messaging is healthy.
🧰 Tools
🪛 ESLint
[error] 291-291: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/test/e2e-staging.ts` around lines 283 - 304, The group chat is
only created with distinctReceivers[0] but later you assert visibility for two
receivers (the for-loop over [sender, ...distinctReceivers.slice(0, 2)]),
causing a false negative for the second receiver; update the setup so both
receivers are added before assertions—either include
distinctReceivers[1].matrix.userID when building startArgs (the variable
startArgs used to run runCli and produce chatID) or explicitly run the
invite/join CLI for distinctReceivers[1] (using the same runCli pattern) after
chatID is available and before sending the message and running the messages list
checks. Ensure chatID is used consistently when adding the second receiver so
the subsequent loop can validate both receivers.
| const flags = new Set(args.filter((arg) => arg.startsWith("--"))); | ||
| const positional = args.filter((arg) => !arg.startsWith("--")); |
There was a problem hiding this comment.
-h never reaches the help path.
flags only includes arguments starting with --, so flags.has("-h") on Line 25 is always false. Right now bun run publish:packages -h falls through and gets treated as the version argument instead of showing usage.
Suggested fix
-const flags = new Set(args.filter((arg) => arg.startsWith("--")));
-const positional = args.filter((arg) => !arg.startsWith("--"));
+const flags = new Set(args.filter((arg) => arg.startsWith("--") || arg === "-h"));
+const positional = args.filter((arg) => !(arg.startsWith("--") || arg === "-h"));Also applies to: 25-27
🧰 Tools
🪛 ESLint
[error] 10-10: Prefer .find(…) over .filter(…).
(unicorn/prefer-array-find)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/publish-packages.ts` around lines 9 - 10, The help flag never matches
because flags only collects args starting with "--"; change the filters so
single-dash flags are captured: build flags from args.filter(arg =>
arg.startsWith("-")) (or normalize by stripping leading dashes) and build
positional from args.filter(arg => !arg.startsWith("-")), so that
flags.has("-h") (and any other single-dash checks) will be true; update the
declarations for the flags and positional variables to use this broader
startsWith("-") rule so the existing help check (flags.has("-h")) works as
intended.
| for (const pkg of ordered) { | ||
| if (skipExisting) { | ||
| const code = await run(["npm", "view", `${pkg.json.name}@${version}`, "version"], { allowFailure: true }); | ||
| if (code === 0) { | ||
| console.log(`Skipping already-published ${pkg.json.name}@${version}`); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| const command = ["npm", "publish", "--access", "public"]; | ||
| if (dryRun) command.push("--dry-run"); | ||
| await run(command, { cwd: pkg.dir }); | ||
| } | ||
|
|
||
| console.log(dryRun ? "Dry run complete." : `Published ${ordered.length} package(s) at ${version}.`); |
There was a problem hiding this comment.
Success output overcounts publishes when --skip-existing is used.
The loop can continue for already-published packages, but the final message still prints ordered.length. That makes the release summary inaccurate whenever some packages were skipped.
Suggested fix
+let publishedCount = 0;
+
for (const pkg of ordered) {
if (skipExisting) {
const code = await run(["npm", "view", `${pkg.json.name}@${version}`, "version"], { allowFailure: true });
if (code === 0) {
console.log(`Skipping already-published ${pkg.json.name}@${version}`);
continue;
}
}
const command = ["npm", "publish", "--access", "public"];
if (dryRun) command.push("--dry-run");
await run(command, { cwd: pkg.dir });
+ publishedCount += 1;
}
-console.log(dryRun ? "Dry run complete." : `Published ${ordered.length} package(s) at ${version}.`);
+console.log(dryRun ? "Dry run complete." : `Published ${publishedCount} package(s) at ${version}.`);📝 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.
| for (const pkg of ordered) { | |
| if (skipExisting) { | |
| const code = await run(["npm", "view", `${pkg.json.name}@${version}`, "version"], { allowFailure: true }); | |
| if (code === 0) { | |
| console.log(`Skipping already-published ${pkg.json.name}@${version}`); | |
| continue; | |
| } | |
| } | |
| const command = ["npm", "publish", "--access", "public"]; | |
| if (dryRun) command.push("--dry-run"); | |
| await run(command, { cwd: pkg.dir }); | |
| } | |
| console.log(dryRun ? "Dry run complete." : `Published ${ordered.length} package(s) at ${version}.`); | |
| let publishedCount = 0; | |
| for (const pkg of ordered) { | |
| if (skipExisting) { | |
| const code = await run(["npm", "view", `${pkg.json.name}@${version}`, "version"], { allowFailure: true }); | |
| if (code === 0) { | |
| console.log(`Skipping already-published ${pkg.json.name}@${version}`); | |
| continue; | |
| } | |
| } | |
| const command = ["npm", "publish", "--access", "public"]; | |
| if (dryRun) command.push("--dry-run"); | |
| await run(command, { cwd: pkg.dir }); | |
| publishedCount += 1; | |
| } | |
| console.log(dryRun ? "Dry run complete." : `Published ${publishedCount} package(s) at ${version}.`); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/publish-packages.ts` around lines 126 - 140, The final summary always
uses ordered.length which overcounts when skipExisting causes continues; fix by
adding a publishedCount variable (e.g., let publishedCount = 0) before the for
loop and incrementing it only when you actually call run(command, { cwd: pkg.dir
}) to publish (or when you simulate a publish in dryRun), then change the final
console.log to print publishedCount (e.g., dryRun ? "Dry run complete." :
`Published ${publishedCount} package(s) at ${version}.`) so the summary reflects
actual published packages; touch the loop that references ordered, skipExisting,
pkg, run, version, and dryRun.
Add a small alias map to the command generator and expand detected command entries to include aliases. The generator now flatMaps files into command+alias entries, deduplicates import paths and reuses a single import identifier per module. Update the generated commands file to include new aliases (e.g. accounts, chats, bridges, targets, contacts) and adjust imports order, and update smoke tests to assert alias behavior.
|
@indent are you there |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/scripts/generate-command-map.ts (1)
25-33: ⚡ Quick winAdd duplicate-command detection before writing the generated map.
Line 25–33 currently allows silent key overwrites if aliases collide with canonical commands (now or in future). Please fail generation when duplicate command keys are detected.
Proposed fix
const entries = canonicalEntries .flatMap(entry => [entry, ...(listAliases[entry.command] ?? []).map(command => ({ command, importPath: entry.importPath }))]) .sort((a, b) => a.command.localeCompare(b.command)) +const seenCommands = new Set<string>() +for (const entry of entries) { + if (seenCommands.has(entry.command)) { + throw new Error(`Duplicate command key generated: ${entry.command}`) + } + seenCommands.add(entry.command) +} + const importPaths = canonicalEntries.map(entry => entry.importPath) const commandImports = new Map(importPaths.map((importPath, index) => [importPath, `Command${index}`])) const imports = importPaths.map((importPath, index) => `import Command${index} from '${importPath}'`).join('\n')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/scripts/generate-command-map.ts` around lines 25 - 33, Before generating the map, detect duplicate command keys created by entries (from canonicalEntries and listAliases) and fail generation if any duplicates exist: compute a frequency map of entries.map(e => e.command), find any commands with count > 1, and throw or exit with a clear error listing those duplicate command strings; do this check right after entries is computed (before building importPaths, commandImports, imports, and mapEntries) so collisions between aliases and canonical commands are caught and generation is aborted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/cli/scripts/generate-command-map.ts`:
- Around line 25-33: Before generating the map, detect duplicate command keys
created by entries (from canonicalEntries and listAliases) and fail generation
if any duplicates exist: compute a frequency map of entries.map(e => e.command),
find any commands with count > 1, and throw or exit with a clear error listing
those duplicate command strings; do this check right after entries is computed
(before building importPaths, commandImports, imports, and mapEntries) so
collisions between aliases and canonical commands are caught and generation is
aborted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41552a70-a3ad-495c-8342-6ce8d766a212
📒 Files selected for processing (3)
packages/cli/scripts/generate-command-map.tspackages/cli/src/commands.generated.tspackages/cli/test/cli-smoke.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/test/cli-smoke.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
- GitHub Check: test (macos-latest / bun 1.3.10)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
🔇 Additional comments (1)
packages/cli/src/commands.generated.ts (1)
9-210: LGTM!
* trip * Update e2e-staging.ts * wip * wip * Fix CLI binary release repository * Cap CLI binary download redirects * Stream cloudflared downloads to disk * Stream installer downloads to disk * Resolve publish package directories portably * Avoid redundant release package build * Require numeric bridge selection input * Escape README flag option pipes * Type chats start request payload * Fix recovery key reset statement spacing * Persist remote setup after auth succeeds * Honor setup install channel flags * Guard desktop Linux binary probing * Require complete local desktop readiness * Restrict restart coverage to server targets * Add command alias support to CLI generator Add a small alias map to the command generator and expand detected command entries to include aliases. The generator now flatMaps files into command+alias entries, deduplicates import paths and reuses a single import identifier per module. Update the generated commands file to include new aliases (e.g. accounts, chats, bridges, targets, contacts) and adjust imports order, and update smoke tests to assert alias behavior.
1 participant
No description provided.