CLI v2#7
Open
batuhan wants to merge 53 commits into
Open
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughMonorepo migrates from Go to a Node/TypeScript oclif CLI. Adds CLI package, commands, UI, auth/export utilities, tests, CI and release workflows, Homebrew packaging, and documentation. Removes Go code, custom encoders, autocomplete, and related tests/config. ChangesNode CLI migration and decommission Go
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as beeper (oclif)
participant SDK as BeeperDesktop SDK
participant API as Desktop API
User->>CLI: beeper chats search --json
CLI->>SDK: createClient(baseURL, token)
SDK->>API: GET /v1/chats/search
API-->>SDK: 200 JSON
SDK-->>CLI: results
CLI-->>User: JSON list
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
.agents/skills/opentui/docs/components/ascii-font.mdx-81-84 (1)
81-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypo: "Pallet" should be "Palette".
The font style name appears to be "Palette" (a range of colors/styles) rather than "Pallet" (a wooden shipping platform).
📝 Proposed fix
-// Pallet style font +// Palette style font { - font: "pallet" + font: "palette" }🤖 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 @.agents/skills/opentui/docs/components/ascii-font.mdx around lines 81 - 84, The font style name is misspelled as "pallet" in the font config object; update the value for the font key (the object containing font: "pallet") to "palette" so the style name reads font: "palette" (ensure you update any identical occurrences of "pallet" in the same component to maintain consistency)..agents/skills/opentui/docs/keymap/hosts.mdx-55-55 (1)
55-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse correct "macOS" branding.
Apple's operating system should be capitalized as "macOS" per official branding guidelines.
📝 Proposed fix
-| `platform` | `macos`, `windows`, `linux` or `unknown` | +| `platform` | `macOS`, `windows`, `linux` or `unknown` |🤖 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 @.agents/skills/opentui/docs/keymap/hosts.mdx at line 55, Update the platform table entry so Apple’s OS uses the correct branding: replace the literal `macos` string with `macOS` in the table row that documents the `platform` field (the cell currently showing `macos`, `windows`, `linux` or `unknown`), preserving the rest of the format and punctuation..agents/skills/opentui/docs/components/tab-select.mdx-158-158 (1)
158-158:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the exported event constant instead of a string literal.
Line 158 uses the string literal
"itemSelected"while the rest of the documentation consistently demonstratesTabSelectRenderableEvents.ITEM_SELECTED. Use the constant to maintain consistency with the earlier examples and prevent silent drift if event names change.Proposed doc fix
-tabs.on("itemSelected", (index, option) => { +tabs.on(TabSelectRenderableEvents.ITEM_SELECTED, (index, option) => {🤖 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 @.agents/skills/opentui/docs/components/tab-select.mdx at line 158, Replace the hard-coded event string in the event listener with the exported enum/constant to keep docs consistent: change the tabs.on("itemSelected", (index, option) => { ...) usage to use TabSelectRenderableEvents.ITEM_SELECTED so the listener references the exported event constant (TabSelectRenderableEvents.ITEM_SELECTED) instead of the string literal..agents/skills/opentui/docs/components/select.mdx-79-82 (1)
79-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
SelectOptiontype import to event handler snippets.The code examples at lines 81 and 91 use
SelectOptionas a type annotation in function parameters but don't import it. Copy-pasting these snippets into a TypeScript file will fail to compile with "SelectOption is not defined."Fix
-import { SelectRenderableEvents } from "@opentui/core" +import { SelectRenderableEvents, type SelectOption } from "@opentui/core"Also applies to line 91.
🤖 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 @.agents/skills/opentui/docs/components/select.mdx around lines 79 - 82, The examples register a handler using menu.on(SelectRenderableEvents.ITEM_SELECTED, ...) and annotate the callback parameter with SelectOption but never import that type, causing TypeScript compile errors; fix by adding an import for SelectOption from "@opentui/core" alongside the existing SelectRenderableEvents import so the two snippets (the handler at the menu.on call shown and the similar one on line 91) reference a defined type and compile correctly..agents/skills/opentui/docs/keymap/react.mdx-115-117 (1)
115-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd React and OpenTUI imports to the
reactiveMatcherFromStore()code example.The snippet on lines 115–127 uses
useMemo,reactiveMatcherFromStore, anduseBindingswithout showing their imports. While the "Basic usage" example earlier includes complete imports, this pattern example omits them, making it fail if copied into a standalone file.+import { useMemo } from "react" +import { reactiveMatcherFromStore, useBindings } from "@opentui/keymap/react" + const matcher = useMemo( () => reactiveMatcherFromStore(store.subscribe, store.getSnapshot, (mode) => mode === "normal"), [store], ) useBindings(🤖 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 @.agents/skills/opentui/docs/keymap/react.mdx around lines 115 - 117, The example is missing imports for React hooks and OpenTUI helpers; add imports for useMemo (from 'react' or 'preact' depending on the project), and import reactiveMatcherFromStore and useBindings from the OpenTUI package so the snippet using useMemo, reactiveMatcherFromStore, and useBindings is complete and copy-pasteable; update the top of the example to include these imports and ensure the named symbols (useMemo, reactiveMatcherFromStore, useBindings) match the existing exported names used elsewhere in the docs.src/commands/unarchive.ts-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse unarchive-specific help summary.
Line 7 points to
apiCopy.chats.archive, so help text for this command is likely misleading. Use an unarchive-specific summary key/string.Suggested change
- static override summary = apiCopy.chats.archive + static override summary = apiCopy.chats.unarchive🤖 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 `@src/commands/unarchive.ts` at line 7, The command's help summary wrongly reuses apiCopy.chats.archive; update the static override summary in the unarchive command (the static override summary field) to reference the unarchive-specific copy key/string (e.g., apiCopy.chats.unarchive or the appropriate unarchive summary key) so the CLI help shows an accurate description for the unarchive command.src/commands/message-expiry.ts-24-24 (1)
24-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign validation error text with actual rule.
Line 24 allows
0but the message says “positive integer”. Use “non-negative integer” (or change the check to<= 0).Proposed fix
- if (expiry !== null && (!Number.isInteger(expiry) || expiry < 0)) throw new Error('SECONDS must be a positive integer or "off"') + if (expiry !== null && (!Number.isInteger(expiry) || expiry < 0)) throw new Error('SECONDS must be a non-negative integer or "off"')🤖 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 `@src/commands/message-expiry.ts` at line 24, The validation message says "positive integer" but the code permits 0; update the check and message to be consistent: either change the condition in the message/throw in the message to say "non-negative integer" (so keep the current condition using expiry !== null && (!Number.isInteger(expiry) || expiry < 0)) or tighten the validation to forbid 0 by using expiry <= 0 and keep "positive integer" in the Error; locate the validation around the expiry variable in the message-expiry command (the if that throws Error('SECONDS must be a positive integer or "off"')) and make the corresponding change so the thrown error text matches the numeric check.scripts/check-api-copy.mjs-21-22 (1)
21-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd drift coverage for
contacts.list.
apiCopy.contacts.listis defined but not included inchecks, so this script won’t detect drift for that entry.Proposed fix
['chats.start', 'resources/chats/chats.d.ts', 'start'], + ['contacts.list', 'resources/accounts/contacts.d.ts', 'list'], ['contacts.search', 'resources/accounts/contacts.d.ts', 'search'],🤖 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/check-api-copy.mjs` around lines 21 - 22, The checks array in scripts/check-api-copy.mjs is missing coverage for apiCopy.contacts.list, so add an entry for it (e.g., ['contacts.list', 'resources/contacts.d.ts', 'list']) to the list of checks so the script will detect drift for contacts.list; update the checks array near the existing ['contacts.search', ...] and ['messages.delete', ...] entries to include that tuple referencing the apiCopy key (contacts.list), the d.ts resource (resources/contacts.d.ts), and the method name ('list').src/commands/config/set.ts-8-14 (1)
8-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
baseURLbefore writing config.Line 13 persists raw input, so malformed URLs can poison config and cause later command failures.
Suggested fix
async run(): Promise<void> { const { args } = await this.parse(ConfigSet) + if (args.key === 'baseURL') { + try { + new URL(args.value) + } catch { + this.error(`Invalid baseURL: ${args.value}`) + } + } await updateConfig(config => ({ ...config, [args.key]: args.value })) this.log(`${args.key}=${args.value}`) }🤖 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 `@src/commands/config/set.ts` around lines 8 - 14, When handling the ConfigSet.run() flow, validate args.value when args.key === 'baseURL' before calling updateConfig to avoid persisting malformed URLs; in run(), check if args.key === 'baseURL' and attempt to parse args.value with the URL constructor (or otherwise validate scheme/host), and if parsing fails call this.error(...) or throw a descriptive error instead of writing to updateConfig, otherwise proceed with updateConfig(config => ({ ...config, [args.key]: args.value })) and this.log.src/commands/send-file.ts-51-55 (1)
51-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an explicit guard for missing
pendingMessageIDwhen--waitis enabled.The wait path currently assumes
pendingMessageIDexists; if it does not, this fails deeper inwaitForMessagewith a less clear error.Suggested fix
if (flags.wait) { + if (!result.pendingMessageID) { + this.error('Cannot wait for resolution: send did not return a pendingMessageID.') + } const resolved = await waitForMessage(client, chatID, result.pendingMessageID, { intervalMs: flags['wait-interval'], timeoutMs: flags['wait-timeout'], })🤖 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 `@src/commands/send-file.ts` around lines 51 - 55, When flags.wait is true, the code calls waitForMessage(client, chatID, result.pendingMessageID, ...) without verifying result.pendingMessageID; add an explicit guard that checks if result.pendingMessageID is present before calling waitForMessage and if missing throw or return a clear error (e.g., log and exit or reject) indicating that pendingMessageID is required for the --wait path. Update the block around flags.wait to validate result.pendingMessageID and only call waitForMessage when it exists, otherwise provide a descriptive error mentioning pendingMessageID and the --wait flag.src/commands/create-chat.ts-24-30 (1)
24-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce single-chat participant cardinality before sending the request.
--type singlecurrently allows multiple--participantvalues, which can produce an ambiguous/invalid create payload and fail downstream.Suggested fix
async run(): Promise<void> { const { flags } = await this.parse(CreateChat) const client = await createClient(flags) const accountID = await resolveAccountID(client, flags.account) + if (flags.type === 'single' && flags.participant.length !== 1) { + this.error('`--type single` requires exactly one `--participant`.') + } + const result = await client.chats.create({ accountID, messageText: flags.message, participantIDs: flags.participant, title: flags.title, type: flags.type as 'single' | 'group', })🤖 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 `@src/commands/create-chat.ts` around lines 24 - 30, Before calling client.chats.create, validate the participant cardinality when flags.type === 'single': check flags.participant (or the parsed participant array) and if type is 'single' ensure exactly one participant is provided; if not, throw or print a clear error and exit (or return) to avoid sending an invalid payload to client.chats.create. Locate the create call (client.chats.create) and the CLI flags (flags.participant, flags.type, flags.message, etc.) and add this guard so only a single participant is allowed for 'single' chats.src/commands/api/post.ts-20-21 (1)
20-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle invalid
--bodyJSON with a CLI-friendly error.Line 20 parses untrusted CLI input directly; malformed JSON will throw an unhandled exception. Wrap in try-catch and rethrow with a clear message (other commands like
watch.tsandrpc.tsalready follow this pattern).Suggested patch
- const body = JSON.parse(flags.body) as Record<string, unknown> + let body: Record<string, unknown> + try { + const parsed = JSON.parse(flags.body) + if (parsed === null || Array.isArray(parsed) || typeof parsed !== 'object') { + throw new Error('Body must be a JSON object') + } + body = parsed as Record<string, unknown> + } catch (error) { + throw new Error(`Invalid --body JSON: ${(error as Error).message}`) + }🤖 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 `@src/commands/api/post.ts` around lines 20 - 21, The code parses flags.body directly which will throw on malformed JSON; wrap the JSON.parse(flags.body) call in a try/catch (same pattern used in watch.ts and rpc.ts) and when JSON.parse fails, throw a CLI-friendly error (e.g., new Error("Invalid JSON for --body: " + err.message)) so the user sees a clear message; ensure you still call client.post(args.path, { body }) and printData(await client.post(...), flags.json ? 'json' : 'human') after successful parsing, referencing flags.body, client.post, and printData.src/commands/messages/search.ts-20-23 (1)
20-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit defaults to negatable boolean flags to match help text.
Lines 20 and 22 declare negatable flags without explicit defaults. The help text describes specific intended behaviors ("Exclude low-priority chats" and "Include muted chats"), but when these flags are omitted, they resolve to
undefinedbefore being passed to the API (lines 42–43). This creates a mismatch: the help text implies these flags have defaults, but the implementation passesundefinedinstead.Align the flag definitions with their documented behavior:
Proposed fix
- 'exclude-low-priority': Flags.boolean({ allowNo: true, description: 'Exclude low-priority chats. Use --no-exclude-low-priority to include all.' }), + 'exclude-low-priority': Flags.boolean({ default: true, allowNo: true, description: 'Exclude low-priority chats. Use --no-exclude-low-priority to include all.' }), @@ - 'include-muted': Flags.boolean({ allowNo: true, description: 'Include muted chats. Use --no-include-muted for a tighter search.' }), + 'include-muted': Flags.boolean({ default: true, allowNo: true, description: 'Include muted chats. Use --no-include-muted for a tighter search.' }),🤖 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 `@src/commands/messages/search.ts` around lines 20 - 23, The negatable flags 'exclude-low-priority' and 'include-muted' are missing explicit defaults so they can be undefined when passed to the API; update their Flags.boolean definitions (the declarations for 'exclude-low-priority' and 'include-muted') to include explicit default: true (to match the help text semantics: exclude low-priority by default and include muted by default) so the values read in run()/where you build the search params and pass them to the API are always booleans rather than undefined.src/lib/manifest.ts-66-66 (1)
66-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
unarchivedescription to avoid archive/unarchive ambiguity.Line 66 currently reuses
apiCopy.chats.archive, sounarchiveis surfaced with archive wording in help/manifest output.Suggested patch
- { command: 'unarchive', description: apiCopy.chats.archive }, + { command: 'unarchive', description: 'Unarchive a chat' },🤖 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 `@src/lib/manifest.ts` at line 66, The manifest entry for the 'unarchive' command is using the wrong description (it references apiCopy.chats.archive), causing help text to show archive wording for unarchive; update the manifest object for the 'unarchive' command (the entry with command: 'unarchive') to use the correct description key (e.g., apiCopy.chats.unarchive) or add a new unarchive string to apiCopy.chats and reference that so the help/manifest displays the proper unarchive wording.
🧹 Nitpick comments (3)
src/commands/contacts/search.ts (1)
28-30: ⚡ Quick winSurface per-account lookup failures when debugging.
The catch block currently suppresses all errors silently, which can hide partial failures. Emit a warning in
--debugmode so skipped accounts are visible.Suggested fix
- } catch { + } catch (error) { // Some networks reject exact lookups for some identifiers; keep trying the rest. + if (flags.debug) { + this.warn( + `contacts.search failed for account ${accountID}: ${error instanceof Error ? error.message : String(error)}`, + ) + } }🤖 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 `@src/commands/contacts/search.ts` around lines 28 - 30, The empty catch is hiding per-account lookup failures; change it to catch (err) and, when the CLI debug flag is enabled (e.g., options.debug / flags.debug / program.opts().debug or the module's debug variable used in this command), emit a warning including the account identifier being queried and the error (err.message or err.stack) so skipped accounts are visible during debugging; keep the existing behavior of continuing to try other accounts after logging. Reference the catch block in the contacts search flow (the per-account lookup inside the search command) and the surrounding variables for the account id and debug flag to implement this.src/lib/oauth.ts (1)
110-115: ⚖️ Poor tradeoffConsider using safer URL opening without shell interpretation.
While the URL is constructed via the URL API (which encodes special characters), spawning a shell command with the URL string could theoretically be vulnerable to command injection in edge cases, especially on Windows. Consider using a library like
open(npm package) that handles cross-platform URL opening more safely.🤖 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 `@src/lib/oauth.ts` around lines 110 - 115, The openExternal function currently builds a platform-specific shell command and calls spawn (spawn(...) in openExternal), which can allow shell interpretation risks; replace this implementation to use a cross-platform opener library (e.g., the `open` npm package) instead of manually spawning shell commands. Import and call the library (e.g., open(url)) from openExternal and remove the platform branching and spawn/unref logic so URLs are opened safely across macOS/Windows/Linux without constructing cmd/open/xdg-open arguments.src/commands/watch.ts (1)
24-24: ⚡ Quick winType assertion bypasses WebSocket API type safety.
The cast
as unknown as string[]at line 24 forces an options object with headers into a parameter position that doesn't support it. TypeScript's default DOM WebSocket type (no explicit lib configuration in tsconfig) expects the second parameter to bestring | string[]for subprotocols, not an options object. While this works at runtime with Node.js's built-in WebSocket (v21+), the type assertion hides a real type mismatch.Either configure
tsconfig.jsonto include the correct library types for your Node.js environment, or avoid the type cast by using proper WebSocket initialization patterns.🤖 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 `@src/commands/watch.ts` at line 24, The code is bypassing TypeScript types by casting the options object to string[] for the WebSocket constructor; remove the unsafe cast and use the Node.js/ws types so headers are correctly typed: import the WebSocket implementation/type from 'ws' (or adjust tsconfig to provide Node WebSocket libs), then replace the instantiation with new WebSocket(url, { headers: { Authorization: `Bearer ${token}` } }) using the imported WebSocket type (referencing WebSocket, url, token) so the headers option is accepted by the constructor without type assertions.
🤖 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 `@scripts/check-api-copy.mjs`:
- Line 5: The code uses new URL('..', import.meta.url).pathname to build root
which yields incorrect Windows paths and doesn't decode percent-encodings;
replace the .pathname usage with Node's fileURLToPath to convert the
import.meta.url-derived URL to a proper filesystem path (import fileURLToPath
from 'node:url'), then call fileURLToPath(new URL('..', import.meta.url)) and
pass that into resolve to set root (refer to the root variable, resolve call,
and new URL(import.meta.url) in the file).
In `@src/commands/auth/login.ts`:
- Around line 38-48: The OAuth branch returns after loginWithPKCE without
persisting the token; update the code in the oauth branch (the block using
loginWithPKCE and the local token variable) to call the same persistence routine
used for other flows—e.g., invoke finishLogin(token) or call
updateConfig/saveAuth with the token fields (clientID, access token, expires_in,
refresh token, scope, etc.) before logging and returning—so the OAuth
credentials are written to the CLI config/state for subsequent commands.
In `@src/commands/auth/logout.ts`:
- Around line 10-17: The logout flow awaits the remote revoke call (fetch to
URL('/oauth/revoke', config.baseURL)) which can hang and prevent clearing local
auth; change the logic in logout.ts so the revoke request is fire-and-forget or
bounded by a timeout and does not block updateConfig. Specifically, call the
revoke endpoint without awaiting it (or wrap the fetch in a Promise.race with a
short timeout) and ensure updateConfig(current => ({ ...current, auth: undefined
})) always runs (use try/finally or simply trigger revoke asynchronously),
referencing the existing token variable, the fetch(...) call, and updateConfig
to locate and modify the code.
In `@src/commands/chats/search.ts`:
- Line 18: The 'include-muted' flag is inverted because allowNo: true without an
explicit default yields false; update the Flags.boolean call for 'include-muted'
to include default: true (e.g., Flags.boolean({ allowNo: true, default: true,
description: 'Include muted chats. Use --no-include-muted for a tighter search.'
})) so muted chats are included by default while still supporting the
--no-include-muted override; ensure the change is applied to the 'include-muted'
flag definition.
In `@src/commands/config/get.ts`:
- Around line 16-18: The current code prints raw auth data which can leak
credentials; before calling printData, redact sensitive fields in the
config.auth object (e.g., accessToken, refreshToken, secret) by replacing them
with a placeholder like "[REDACTED]" when printing the whole config or the auth
key by default. Locate the usage of readConfig, args.key and printData in this
file and, if args.key is undefined or args.key === 'auth', create a shallow copy
of config (or config.auth) and replace known secret properties with "[REDACTED]"
then pass that sanitized value to printData (leave JSON output behavior intact
or respect flags.json if you want to allow machine-readable full dumps).
In `@src/commands/contacts/list.ts`:
- Around line 32-34: The --ids branch calls printIDs(items) but printIDs expects
id/chatID/messageID while contact rows usually carry userID; update the --ids
handling in the list command to pass the correct identifier by mapping items to
userID when present (e.g. items.map(i => i.userID ?? i.id)) before calling
printIDs, or modify printIDs to prefer userID for contact objects; ensure you
reference the flags.ids check and the printIDs call so returned IDs are contact
userIDs, falling back to existing id/chatID/messageID values if userID is
missing.
- Around line 27-31: The loop currently requests up to flags.limit per account,
allowing the final items array to exceed the global limit; fix by computing
remaining = flags.limit - items.length before each fetch, break early if
remaining <= 0, and pass remaining into collectPage instead of flags.limit when
calling client.accounts.contacts.list(accountID, { query: flags.query }); keep
pushing accountID into each item and break the outer loop once items.length >=
flags.limit.
In `@src/commands/description.ts`:
- Around line 24-27: The command handler currently accepts both args.description
and the --clear flag and silently treats description as cleared; change the
validation to reject this conflicting input by checking if flags.clear and
args.description are both truthy and throwing a clear error (e.g., throw new
Error('Provide either DESCRIPTION or --clear, not both')) before calling
createClient/resolveChatID/client.chats.update/printData so the conflict is
detected early and accidental data loss is prevented.
In `@src/commands/doctor.ts`:
- Line 21: The fetch call that probes the server (the request using new
URL('/v1/info', baseURL)) lacks a timeout and can hang; wrap it with an
AbortController, pass controller.signal into fetch, start a timer (e.g., const
timeoutId = setTimeout(() => controller.abort(), <timeoutMs>)) before calling
fetch, and clear the timer (clearTimeout(timeoutId)) after the response or on
error so the request is aborted on timeout and resources are cleaned up;
reference the fetch call, baseURL, and the AbortController usage when making the
change.
In `@src/commands/messages/index.ts`:
- Around line 27-28: The code computes cursor and direction with inconsistent
precedence (cursor = flags.before ?? flags.after but direction = flags.after ?
'after' : flags.before ? 'before' : undefined), which can pair a before cursor
with an after direction; update the pagination handling in the command (use the
flags object) to enforce mutual exclusion or a unified precedence: either (A)
validate and throw/print an error if both flags.before and flags.after are
provided, or (B) pick a single precedence rule and apply it consistently to both
cursor and direction (e.g., if flags.after takes precedence, set cursor =
flags.after and direction = 'after' when both present). Ensure changes touch the
variables cursor and direction so the built pagination params are always
consistent.
In `@src/commands/rpc.ts`:
- Around line 38-42: The error response currently hardcodes id: null in the
process.stdout.write JSON, breaking request correlation; update the error path
that writes the response (the process.stdout.write({...}) block) to preserve the
incoming request id instead of null—e.g. use the existing request id variable in
scope (such as id or request.id, or req.id) with a fallback to null (id: id ??
(request?.id ?? null)) so error responses carry the original request identifier.
In `@src/commands/start-chat.ts`:
- Around line 30-40: The command currently constructs an empty user object and
calls tryStartChat even when no recipient selector is provided; add a pre-check
before calling tryStartChat (around the construction of user) to validate that
either args.query is set or at least one identity flag (flags.email, flags.name,
flags.id, flags.phone, flags.username) is present, and if none are provided fail
fast by returning an error / printing a clear message and exiting (or throwing)
instead of calling tryStartChat; update the code paths around
userQueryFromInput, the local variable user, and the tryStartChat invocation to
enforce this validation.
In `@src/commands/status.ts`:
- Around line 17-19: The fetch in the status probe (the call creating response
with fetch(new URL('/v1/info', baseURL))) has no timeout and can hang; update it
to use AbortSignal.timeout(timeoutMs) (Node v16.14+), pass the resulting signal
to fetch (e.g., fetch(url, { signal })), and handle the abort by catching the
thrown DOMException/Error and converting it into a clear timeout error before
proceeding with the existing response.ok check; keep references to the same URL
'/v1/info' and baseURL and choose a sensible timeout value (e.g., 5s) as
timeoutMs.
In `@src/lib/argv.ts`:
- Around line 24-27: The tokenizer drops empty quoted arguments because token
emission is gated by truthiness (if (current)) — change emission logic in the
loop that handles whitespace and quote boundaries so that you push current even
when it's an empty string if that empty string was produced by closing a quote.
Concretely: introduce a small flag (e.g., closedQuote) that you set when you
close a quote (when quote is toggled off), and then in both emission sites (the
whitespace handler and the end-of-input flush) replace the truthy check with a
condition that pushes current when current !== '' OR closedQuote is true; reset
closedQuote after pushing. This preserves ""/'' as empty tokens while still
preventing spurious empty tokens from normal consecutive spaces.
In `@src/lib/config.ts`:
- Around line 24-33: The readConfig function currently swallows all errors in
its try/catch and silently falls back to default baseURL which hides malformed
JSON or permission errors; update readConfig to distinguish errors: attempt
readFile/configPath and JSON.parse as before, but if readFile fails with a "file
not found" (ENOENT) return the safe fallback, whereas if JSON.parse throws or
any other I/O error occurs, rethrow or surface the error (or log it and then
throw) so callers can detect malformed config; keep the same returned shape
(baseURL and auth when parse succeeds) and reference the existing symbols
readConfig, configPath(), defaultBaseURL, parsed.auth when making the changes.
In `@src/lib/oauth.ts`:
- Around line 94-108: The token exchange omits redirect_uri and the
exchangeToken signature lacks a redirectURI parameter, which violates RFC 6749;
update the exchangeToken function signature (exchangeToken) to accept a
redirectURI argument and include redirect_uri: redirectURI in the
URLSearchParams body used for the POST so the token request matches the original
authorization request.
In `@src/lib/output.ts`:
- Around line 241-246: In collectPage, the current logic treats limit=0 as "no
limit" and pushes one item before breaking; change the loop to check the
explicit presence of a limit before pushing by using if (limit !== undefined &&
items.length >= limit) break at the top of the for-await loop (use strict !==
undefined rather than truthiness) so collectPage(..., 0) returns an empty array
and any explicit numeric limit is honored; update the function collectPage and
its local items/limit handling accordingly.
In `@src/lib/resolve.ts`:
- Around line 98-103: The retrieveChat function currently swallows all errors
from client.chats.retrieve which hides auth/network/server issues; change its
error handling to only return undefined for a genuine “not found” case and
rethrow (or log and rethrow) other errors: inspect the caught error inside
retrieveChat (from client.chats.retrieve) for a not-found indicator (e.g.,
status/code/message) and return undefined only then, otherwise throw the error
(or call processLogger.error before throwing) so upstream can handle
auth/network/server failures appropriately.
---
Minor comments:
In @.agents/skills/opentui/docs/components/ascii-font.mdx:
- Around line 81-84: The font style name is misspelled as "pallet" in the font
config object; update the value for the font key (the object containing font:
"pallet") to "palette" so the style name reads font: "palette" (ensure you
update any identical occurrences of "pallet" in the same component to maintain
consistency).
In @.agents/skills/opentui/docs/components/select.mdx:
- Around line 79-82: The examples register a handler using
menu.on(SelectRenderableEvents.ITEM_SELECTED, ...) and annotate the callback
parameter with SelectOption but never import that type, causing TypeScript
compile errors; fix by adding an import for SelectOption from "@opentui/core"
alongside the existing SelectRenderableEvents import so the two snippets (the
handler at the menu.on call shown and the similar one on line 91) reference a
defined type and compile correctly.
In @.agents/skills/opentui/docs/components/tab-select.mdx:
- Line 158: Replace the hard-coded event string in the event listener with the
exported enum/constant to keep docs consistent: change the
tabs.on("itemSelected", (index, option) => { ...) usage to use
TabSelectRenderableEvents.ITEM_SELECTED so the listener references the exported
event constant (TabSelectRenderableEvents.ITEM_SELECTED) instead of the string
literal.
In @.agents/skills/opentui/docs/keymap/hosts.mdx:
- Line 55: Update the platform table entry so Apple’s OS uses the correct
branding: replace the literal `macos` string with `macOS` in the table row that
documents the `platform` field (the cell currently showing `macos`, `windows`,
`linux` or `unknown`), preserving the rest of the format and punctuation.
In @.agents/skills/opentui/docs/keymap/react.mdx:
- Around line 115-117: The example is missing imports for React hooks and
OpenTUI helpers; add imports for useMemo (from 'react' or 'preact' depending on
the project), and import reactiveMatcherFromStore and useBindings from the
OpenTUI package so the snippet using useMemo, reactiveMatcherFromStore, and
useBindings is complete and copy-pasteable; update the top of the example to
include these imports and ensure the named symbols (useMemo,
reactiveMatcherFromStore, useBindings) match the existing exported names used
elsewhere in the docs.
In `@scripts/check-api-copy.mjs`:
- Around line 21-22: The checks array in scripts/check-api-copy.mjs is missing
coverage for apiCopy.contacts.list, so add an entry for it (e.g.,
['contacts.list', 'resources/contacts.d.ts', 'list']) to the list of checks so
the script will detect drift for contacts.list; update the checks array near the
existing ['contacts.search', ...] and ['messages.delete', ...] entries to
include that tuple referencing the apiCopy key (contacts.list), the d.ts
resource (resources/contacts.d.ts), and the method name ('list').
In `@src/commands/api/post.ts`:
- Around line 20-21: The code parses flags.body directly which will throw on
malformed JSON; wrap the JSON.parse(flags.body) call in a try/catch (same
pattern used in watch.ts and rpc.ts) and when JSON.parse fails, throw a
CLI-friendly error (e.g., new Error("Invalid JSON for --body: " + err.message))
so the user sees a clear message; ensure you still call client.post(args.path, {
body }) and printData(await client.post(...), flags.json ? 'json' : 'human')
after successful parsing, referencing flags.body, client.post, and printData.
In `@src/commands/config/set.ts`:
- Around line 8-14: When handling the ConfigSet.run() flow, validate args.value
when args.key === 'baseURL' before calling updateConfig to avoid persisting
malformed URLs; in run(), check if args.key === 'baseURL' and attempt to parse
args.value with the URL constructor (or otherwise validate scheme/host), and if
parsing fails call this.error(...) or throw a descriptive error instead of
writing to updateConfig, otherwise proceed with updateConfig(config => ({
...config, [args.key]: args.value })) and this.log.
In `@src/commands/create-chat.ts`:
- Around line 24-30: Before calling client.chats.create, validate the
participant cardinality when flags.type === 'single': check flags.participant
(or the parsed participant array) and if type is 'single' ensure exactly one
participant is provided; if not, throw or print a clear error and exit (or
return) to avoid sending an invalid payload to client.chats.create. Locate the
create call (client.chats.create) and the CLI flags (flags.participant,
flags.type, flags.message, etc.) and add this guard so only a single participant
is allowed for 'single' chats.
In `@src/commands/message-expiry.ts`:
- Line 24: The validation message says "positive integer" but the code permits
0; update the check and message to be consistent: either change the condition in
the message/throw in the message to say "non-negative integer" (so keep the
current condition using expiry !== null && (!Number.isInteger(expiry) || expiry
< 0)) or tighten the validation to forbid 0 by using expiry <= 0 and keep
"positive integer" in the Error; locate the validation around the expiry
variable in the message-expiry command (the if that throws Error('SECONDS must
be a positive integer or "off"')) and make the corresponding change so the
thrown error text matches the numeric check.
In `@src/commands/messages/search.ts`:
- Around line 20-23: The negatable flags 'exclude-low-priority' and
'include-muted' are missing explicit defaults so they can be undefined when
passed to the API; update their Flags.boolean definitions (the declarations for
'exclude-low-priority' and 'include-muted') to include explicit default: true
(to match the help text semantics: exclude low-priority by default and include
muted by default) so the values read in run()/where you build the search params
and pass them to the API are always booleans rather than undefined.
In `@src/commands/send-file.ts`:
- Around line 51-55: When flags.wait is true, the code calls
waitForMessage(client, chatID, result.pendingMessageID, ...) without verifying
result.pendingMessageID; add an explicit guard that checks if
result.pendingMessageID is present before calling waitForMessage and if missing
throw or return a clear error (e.g., log and exit or reject) indicating that
pendingMessageID is required for the --wait path. Update the block around
flags.wait to validate result.pendingMessageID and only call waitForMessage when
it exists, otherwise provide a descriptive error mentioning pendingMessageID and
the --wait flag.
In `@src/commands/unarchive.ts`:
- Line 7: The command's help summary wrongly reuses apiCopy.chats.archive;
update the static override summary in the unarchive command (the static override
summary field) to reference the unarchive-specific copy key/string (e.g.,
apiCopy.chats.unarchive or the appropriate unarchive summary key) so the CLI
help shows an accurate description for the unarchive command.
In `@src/lib/manifest.ts`:
- Line 66: The manifest entry for the 'unarchive' command is using the wrong
description (it references apiCopy.chats.archive), causing help text to show
archive wording for unarchive; update the manifest object for the 'unarchive'
command (the entry with command: 'unarchive') to use the correct description key
(e.g., apiCopy.chats.unarchive) or add a new unarchive string to apiCopy.chats
and reference that so the help/manifest displays the proper unarchive wording.
---
Nitpick comments:
In `@src/commands/contacts/search.ts`:
- Around line 28-30: The empty catch is hiding per-account lookup failures;
change it to catch (err) and, when the CLI debug flag is enabled (e.g.,
options.debug / flags.debug / program.opts().debug or the module's debug
variable used in this command), emit a warning including the account identifier
being queried and the error (err.message or err.stack) so skipped accounts are
visible during debugging; keep the existing behavior of continuing to try other
accounts after logging. Reference the catch block in the contacts search flow
(the per-account lookup inside the search command) and the surrounding variables
for the account id and debug flag to implement this.
In `@src/commands/watch.ts`:
- Line 24: The code is bypassing TypeScript types by casting the options object
to string[] for the WebSocket constructor; remove the unsafe cast and use the
Node.js/ws types so headers are correctly typed: import the WebSocket
implementation/type from 'ws' (or adjust tsconfig to provide Node WebSocket
libs), then replace the instantiation with new WebSocket(url, { headers: {
Authorization: `Bearer ${token}` } }) using the imported WebSocket type
(referencing WebSocket, url, token) so the headers option is accepted by the
constructor without type assertions.
In `@src/lib/oauth.ts`:
- Around line 110-115: The openExternal function currently builds a
platform-specific shell command and calls spawn (spawn(...) in openExternal),
which can allow shell interpretation risks; replace this implementation to use a
cross-platform opener library (e.g., the `open` npm package) instead of manually
spawning shell commands. Import and call the library (e.g., open(url)) from
openExternal and remove the platform branching and spawn/unref logic so URLs are
opened safely across macOS/Windows/Linux without constructing cmd/open/xdg-open
arguments.
🪄 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: 7cda4489-7b12-4994-9f64-9eecfa712ee6
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (151)
.agents/skills/opentui/SKILL.md.agents/skills/opentui/config.ts.agents/skills/opentui/docs/bindings/react.mdx.agents/skills/opentui/docs/bindings/solid.mdx.agents/skills/opentui/docs/components/ascii-font.mdx.agents/skills/opentui/docs/components/box.mdx.agents/skills/opentui/docs/components/code.mdx.agents/skills/opentui/docs/components/diff.mdx.agents/skills/opentui/docs/components/frame-buffer.mdx.agents/skills/opentui/docs/components/input.mdx.agents/skills/opentui/docs/components/line-number.mdx.agents/skills/opentui/docs/components/markdown.mdx.agents/skills/opentui/docs/components/scrollbar.mdx.agents/skills/opentui/docs/components/scrollbox.mdx.agents/skills/opentui/docs/components/select.mdx.agents/skills/opentui/docs/components/slider.mdx.agents/skills/opentui/docs/components/tab-select.mdx.agents/skills/opentui/docs/components/text.mdx.agents/skills/opentui/docs/components/textarea.mdx.agents/skills/opentui/docs/core-concepts/audio.mdx.agents/skills/opentui/docs/core-concepts/colors.mdx.agents/skills/opentui/docs/core-concepts/console.mdx.agents/skills/opentui/docs/core-concepts/constructs.mdx.agents/skills/opentui/docs/core-concepts/keyboard.mdx.agents/skills/opentui/docs/core-concepts/layout.mdx.agents/skills/opentui/docs/core-concepts/lifecycle.mdx.agents/skills/opentui/docs/core-concepts/notifications.mdx.agents/skills/opentui/docs/core-concepts/renderables-vs-constructs.mdx.agents/skills/opentui/docs/core-concepts/renderables.mdx.agents/skills/opentui/docs/core-concepts/renderer.mdx.agents/skills/opentui/docs/getting-started.mdx.agents/skills/opentui/docs/keymap/addons.mdx.agents/skills/opentui/docs/keymap/core.mdx.agents/skills/opentui/docs/keymap/custom-addons.mdx.agents/skills/opentui/docs/keymap/hosts.mdx.agents/skills/opentui/docs/keymap/overview.mdx.agents/skills/opentui/docs/keymap/react.mdx.agents/skills/opentui/docs/keymap/solid.mdx.agents/skills/opentui/docs/plugins/core.mdx.agents/skills/opentui/docs/plugins/react.mdx.agents/skills/opentui/docs/plugins/slots.mdx.agents/skills/opentui/docs/plugins/solid.mdx.agents/skills/opentui/docs/reference/color-matrix.mdx.agents/skills/opentui/docs/reference/env-vars.mdx.agents/skills/opentui/docs/reference/tree-sitter.mdx.github/workflows/ci.yml.github/workflows/publish-release.yml.gitignore.goreleaser.ymlREADME.mdTYPESCRIPT_CLI.mdbin/check-release-environmentbin/dev.jsbin/run.jspackage.jsonrelease-please-config.jsonscripts/build-homebrew-archive.mjsscripts/check-api-copy.mjsscripts/publish-homebrew-formula.mjsskills-lock.jsonsrc/commands/accounts.tssrc/commands/api/get.tssrc/commands/api/post.tssrc/commands/app/e2ee/recovery-code/mark-backed-up.tssrc/commands/app/e2ee/recovery-code/reset/begin.tssrc/commands/app/e2ee/recovery-code/reset/confirm.tssrc/commands/app/e2ee/recovery-code/verify.tssrc/commands/app/status.tssrc/commands/archive.tssrc/commands/assets/download.tssrc/commands/assets/upload.tssrc/commands/auth/login.tssrc/commands/auth/logout.tssrc/commands/auth/status.tssrc/commands/avatar.tssrc/commands/chat.tssrc/commands/chat/open.tssrc/commands/chats/index.tssrc/commands/chats/search.tssrc/commands/clear-draft.tssrc/commands/commands.tssrc/commands/config/get.tssrc/commands/config/path.tssrc/commands/config/reset.tssrc/commands/config/set.tssrc/commands/contacts/list.tssrc/commands/contacts/search.tssrc/commands/create-chat.tssrc/commands/current-user.tssrc/commands/delete-message.tssrc/commands/description.tssrc/commands/doctor.tssrc/commands/draft.tssrc/commands/edit.tssrc/commands/export.tssrc/commands/focus.tssrc/commands/inbox.tssrc/commands/interactive.tssrc/commands/llm.tssrc/commands/login.tssrc/commands/logout.tssrc/commands/low-priority.tssrc/commands/mark-read.tssrc/commands/mark-unread.tssrc/commands/message-expiry.tssrc/commands/message.tssrc/commands/messages/index.tssrc/commands/messages/search.tssrc/commands/mute.tssrc/commands/notify-anyway.tssrc/commands/pin.tssrc/commands/react.tssrc/commands/read.tssrc/commands/remind.tssrc/commands/reply-file.tssrc/commands/reply.tssrc/commands/rpc.tssrc/commands/search.tssrc/commands/send-file.tssrc/commands/send.tssrc/commands/shell.tssrc/commands/start-chat.tssrc/commands/status.tssrc/commands/tail.tssrc/commands/thread.tssrc/commands/threads.tssrc/commands/title.tssrc/commands/unarchive.tssrc/commands/unmute.tssrc/commands/unpin.tssrc/commands/unreact.tssrc/commands/unread.tssrc/commands/unremind.tssrc/commands/watch.tssrc/commands/whoami.tssrc/lib/app-api.tssrc/lib/argv.tssrc/lib/client.tssrc/lib/config.tssrc/lib/copy.tssrc/lib/export/index.tssrc/lib/interactive.tssrc/lib/manifest.tssrc/lib/oauth.tssrc/lib/output.tssrc/lib/pkce.tssrc/lib/resolve.tssrc/lib/runner.tssrc/lib/wait.tstest/cli-smoke.mjstsconfig.json
💤 Files with no reviewable changes (1)
- .goreleaser.yml
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/bridge/login.ts (1)
36-37: ⚡ Quick winAlways emit final human-readable output in guided mode.
In non-JSON mode, guided execution currently skips
printBridgeLoginStep(...)entirely in this command. Consider printing the finalresultfor consistent CLI output across modes.Proposed change
- if (flags.json) printData(result, 'json') - else if (!flags.guided) printBridgeLoginStep(result) + if (flags.json) printData(result, 'json') + else printBridgeLoginStep(result)🤖 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 `@src/commands/bridge/login.ts` around lines 36 - 37, The CLI currently skips emitting the final human-readable result when running in guided mode because the conditional only calls printBridgeLoginStep(result) when !flags.guided; change the branching so that if flags.json you call printData(result, 'json'), otherwise always call printBridgeLoginStep(result) (i.e., remove the !flags.guided guard), ensuring printBridgeLoginStep(result) runs for both guided and non-guided non-JSON flows.
🤖 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 `@src/commands/bridge/login/submit-cookies.ts`:
- Line 23: Wrap the JSON.parse(flags.body) call in a try/catch and validate the
resulting value before casting: catch JSON parse errors and emit a clear CLI
error referencing the --body flag (rather than letting it throw), then ensure
the parsed value is an object and every property value is a string (i.e. the
intended Record<string,string> shape); if validation fails emit a descriptive
CLI error and exit/return. Use the existing flags.body variable and the const
body assignment location in submit-cookies to implement this guard and
validation.
In `@src/commands/bridge/login/submit-user-input.ts`:
- Line 23: Wrap the JSON.parse of flags.body in a guarded parse (try/catch) and
return a clear CLI error on parse failure; after parsing, validate that the
result is an object (not null/array) and that each property value is a string
(or coerce/validate accordingly) before proceeding to API submission — update
the parsing around const body = JSON.parse(flags.body) so malformed JSON or
invalid shapes produce actionable error messages (e.g., via the same CLI error
path) instead of throwing.
---
Nitpick comments:
In `@src/commands/bridge/login.ts`:
- Around line 36-37: The CLI currently skips emitting the final human-readable
result when running in guided mode because the conditional only calls
printBridgeLoginStep(result) when !flags.guided; change the branching so that if
flags.json you call printData(result, 'json'), otherwise always call
printBridgeLoginStep(result) (i.e., remove the !flags.guided guard), ensuring
printBridgeLoginStep(result) runs for both guided and non-guided non-JSON flows.
🪄 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: 0b7ad9c4-fe51-481c-8f53-5cab32253393
📒 Files selected for processing (19)
package.jsonsrc/commands/app/e2ee/verification/accept.tssrc/commands/app/e2ee/verification/cancel.tssrc/commands/app/e2ee/verification/qr/confirm-scanned.tssrc/commands/app/e2ee/verification/qr/scan.tssrc/commands/app/e2ee/verification/sas/confirm.tssrc/commands/app/e2ee/verification/sas/start.tssrc/commands/app/e2ee/verification/start.tssrc/commands/bridge/flows.tssrc/commands/bridge/login.tssrc/commands/bridge/login/start.tssrc/commands/bridge/login/submit-cookies.tssrc/commands/bridge/login/submit-user-input.tssrc/commands/bridge/login/wait.tssrc/commands/bridge/logins.tssrc/commands/bridge/logout.tssrc/commands/bridge/whoami.tssrc/lib/bridge-login.tssrc/lib/manifest.ts
✅ Files skipped from review due to trivial changes (3)
- src/commands/app/e2ee/verification/cancel.ts
- src/lib/bridge-login.ts
- src/commands/app/e2ee/verification/sas/confirm.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/manifest.ts
- package.json
📜 Review details
🔇 Additional comments (12)
src/commands/app/e2ee/verification/start.ts (1)
1-23: LGTM!src/commands/app/e2ee/verification/qr/confirm-scanned.ts (1)
1-24: LGTM!src/commands/app/e2ee/verification/sas/start.ts (1)
1-24: LGTM!src/commands/app/e2ee/verification/accept.ts (1)
1-24: LGTM!src/commands/app/e2ee/verification/qr/scan.ts (1)
1-24: LGTM!src/commands/bridge/logins.ts (1)
1-21: LGTM!src/commands/bridge/logout.ts (1)
1-22: LGTM!src/commands/bridge/whoami.ts (1)
1-21: LGTM!src/commands/bridge/flows.ts (1)
1-21: LGTM!src/commands/bridge/login/start.ts (1)
1-29: LGTM!src/commands/bridge/login/wait.ts (1)
1-29: LGTM!src/commands/bridge/login.ts (1)
1-35: LGTM!Also applies to: 38-39
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/commands/accounts/add.ts`:
- Line 22: The code currently falls back to loginFlows[0] when the --flow flag
is omitted and the chooser is skipped (e.g., flags.json or flags.guided ===
false); change this so we do not silently pick the first backend flow: when
flags.flow is undefined and the chooser is skipped (check the same
chooserSkipped/flags.json || flags.guided === false condition used near the flow
selection), throw a user-facing error asking the caller to provide --flow rather
than assigning loginFlows[0]. Update the selection logic that references
loginFlows and the flow flag (the flow: Flags.string flag parsing and the code
that sets selectedFlow = flags.flow ?? loginFlows[0]) to validate and require
flags.flow in chooser-skipped paths and only use a default when the interactive
chooser runs.
In `@src/lib/account-login.ts`:
- Around line 101-110: The cookie prompt currently uses promptText and therefore
echoes sensitive cookie values to the terminal; update the 'cookies' branch in
the handler (the block gated by if ('cookies' in step)) to use the
masked/no-echo prompt function instead of promptText (e.g., promptSecret /
promptPassword / promptNoEcho) when collecting body[field.name], preserving the
existing checks that use options.cookies and options.nonInteractive and the same
error message on non-interactive; apply the same replacement for the second
cookie collection occurrence referenced around lines 120-123 so all cookie
inputs are captured without terminal echo.
- Around line 30-58: printAccountLoginStep currently writes human-readable
progress to process.stdout which corrupts machine-readable output; change it to
write to process.stderr (or accept an optional writable stream parameter
defaulting to process.stderr) and use that stream for all writes in
printAccountLoginStep; apply the same change to the other helper functions in
this file that emit interactive output so callers wanting JSON can still use
stdout unmodified.
🪄 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: 78f2f52a-be95-4973-8447-befb9ad53f55
📒 Files selected for processing (4)
package.jsonsrc/commands/accounts/add.tssrc/lib/account-login.tssrc/lib/manifest.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/lib/manifest.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e-staging.mjs (2)
13-14: ⚡ Quick winHardcoded developer-specific path reduces test portability.
The default path
/Users/batuhan/.codex/worktrees/43bc/beeper/desktopis specific to a particular developer's machine and won't work for other developers or CI environments.Consider either requiring
BEEPER_DESKTOP_ROOTto be set (throwing an error if missing) or using a more generic default like a relative path from the repo root. This makes the test more portable and prevents confusing failures when other developers run it.🤖 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 `@test/e2e-staging.mjs` around lines 13 - 14, defaultDesktopRoot is set to a developer-specific absolute path and makes desktopRoot unreliable for other environments; change the logic around defaultDesktopRoot/desktopRoot so that either (a) if process.env.BEEPER_DESKTOP_ROOT is not set the script throws a clear error asking the caller to set BEEPER_DESKTOP_ROOT, or (b) compute a portable default (e.g., a repo-relative path using __dirname or process.cwd()) and fall back to that; update the code that reads defaultDesktopRoot/desktopRoot to implement one of these approaches and ensure error messaging or the relative-path computation mentions BEEPER_DESKTOP_ROOT, defaultDesktopRoot, and desktopRoot for clarity.
982-982: 💤 Low valueInconsistent SIGKILL timeout.
Line 982 waits 1000ms between SIGTERM and SIGKILL, while line 858 in
runProcesswaits 2000ms for the same pattern. Consider using a consistent timeout duration for graceful shutdown attempts.🤖 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 `@test/e2e-staging.mjs` at line 982, The sleep call waiting 1000ms between SIGTERM and SIGKILL is inconsistent with the 2000ms used in runProcess; replace the magic number with a shared constant (e.g., GRACEFUL_SHUTDOWN_TIMEOUT) or update the sleep(1000) to sleep(2000) so both the standalone wait and the wait inside runProcess use the same duration; locate the sleep call and the runProcess implementation and use the same identifier (GRACEFUL_SHUTDOWN_TIMEOUT) or matching literal to ensure consistent graceful shutdown timing.
🤖 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 `@src/commands/app/e2ee/recovery-code/reset/begin.ts`:
- Line 10: The current CLI defines a sensitive 'recovery-code' flag using
Flags.string which can expose E2EE recovery codes in shell history and process
listings; change the implementation in begin.ts so that if the 'recovery-code'
flag is omitted the command prompts interactively with hidden input (or reads
from stdin/file) instead of relying on a visible flag, add a note in the command
help/documentation recommending environment variables or secure input methods
for automation, and if you must keep the flag for automation, clearly document
the security risks and consider marking the flag as deprecated or providing an
alternative secure input flow.
---
Nitpick comments:
In `@test/e2e-staging.mjs`:
- Around line 13-14: defaultDesktopRoot is set to a developer-specific absolute
path and makes desktopRoot unreliable for other environments; change the logic
around defaultDesktopRoot/desktopRoot so that either (a) if
process.env.BEEPER_DESKTOP_ROOT is not set the script throws a clear error
asking the caller to set BEEPER_DESKTOP_ROOT, or (b) compute a portable default
(e.g., a repo-relative path using __dirname or process.cwd()) and fall back to
that; update the code that reads defaultDesktopRoot/desktopRoot to implement one
of these approaches and ensure error messaging or the relative-path computation
mentions BEEPER_DESKTOP_ROOT, defaultDesktopRoot, and desktopRoot for clarity.
- Line 982: The sleep call waiting 1000ms between SIGTERM and SIGKILL is
inconsistent with the 2000ms used in runProcess; replace the magic number with a
shared constant (e.g., GRACEFUL_SHUTDOWN_TIMEOUT) or update the sleep(1000) to
sleep(2000) so both the standalone wait and the wait inside runProcess use the
same duration; locate the sleep call and the runProcess implementation and use
the same identifier (GRACEFUL_SHUTDOWN_TIMEOUT) or matching literal to ensure
consistent graceful shutdown timing.
🪄 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: b91035be-31dd-4c91-897b-d7d9626707a2
📒 Files selected for processing (8)
README.mdTYPESCRIPT_CLI.mdpackage.jsonsrc/commands/app/e2ee/recovery-code/reset/begin.tssrc/commands/app/e2ee/verification/start.tssrc/commands/auth/login.tssrc/lib/oauth.tstest/e2e-staging.mjs
✅ Files skipped from review due to trivial changes (2)
- package.json
- TYPESCRIPT_CLI.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/commands/app/e2ee/verification/start.ts
- src/lib/oauth.ts
- src/commands/auth/login.ts
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~251-~251: The official name of this software platform is spelled with a capital “H”.
Context: ...pm and Homebrew. Push a v* tag to run .github/workflows/publish-release.yml. The re...
(GITHUB)
🔇 Additional comments (2)
src/commands/app/e2ee/recovery-code/reset/begin.ts (1)
14-21: LGTM!test/e2e-staging.mjs (1)
753-785: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/publish-release.yml:
- Around line 3-5: Remove the unused OIDC permission by deleting the id-token:
write entry from the workflow permissions block so only contents: write remains;
update the permissions mapping (the permissions key and its children) to no
longer include the id-token field, leaving the workflow with just contents:
write for GitHub release operations.
In `@README.md`:
- Line 129: Update the example command line that currently reads beeper remind
"Family" 2026-05-13T12:00:00Z to use a future or placeholder ISO-8601 timestamp
(e.g., 2026-12-01T12:00:00Z or <FUTURE_TIMESTAMP>) so copy/paste users won't hit
an already-passed time; change the literal in the README example accordingly
wherever the remind example appears.
- Around line 210-211: The README's examples show `commands --json` and `llm` as
standalone binaries; update the documentation to show them as subcommands by
prefixing each with the CLI root `beeper` (e.g., `beeper commands --json` and
`beeper llm`) so copy/paste yields correct invocation; locate the lines
referencing `commands --json` and `llm` and replace them with the prefixed forms
and update any surrounding explanatory text to reflect they are subcommands of
`beeper`.
🪄 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: 170a2bb2-2c9d-4c5c-a5fb-8d8753b5e903
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (68)
.github/actions/setup-go/action.yml.github/workflows/publish-release.ymlREADME.mdcmd/beeper-desktop-cli/main.gogo.modinternal/apiform/encoder.gointernal/apiform/form.gointernal/apiform/form_test.gointernal/apiquery/encoder.gointernal/apiquery/query.gointernal/apiquery/query_test.gointernal/autocomplete/autocomplete.gointernal/autocomplete/autocomplete_test.gointernal/autocomplete/shellscripts/bash_autocomplete.bashinternal/autocomplete/shellscripts/fish_autocomplete.fishinternal/autocomplete/shellscripts/pwsh_autocomplete.ps1internal/autocomplete/shellscripts/zsh_autocomplete.zshinternal/binaryparam/binary_param.gointernal/binaryparam/binary_param_test.gointernal/debugmiddleware/debug_middleware.gointernal/debugmiddleware/debug_middleware_test.gointernal/jsonview/explorer.gointernal/jsonview/staticdisplay.gointernal/mocktest/mocktest.gointernal/requestflag/innerflag.gointernal/requestflag/innerflag_test.gointernal/requestflag/requestflag.gointernal/requestflag/requestflag_test.gopackage.jsonpkg/cmd/account.gopkg/cmd/account_test.gopkg/cmd/accountcontact.gopkg/cmd/accountcontact_test.gopkg/cmd/asset.gopkg/cmd/asset_test.gopkg/cmd/beeperdesktopapi.gopkg/cmd/beeperdesktopapi_test.gopkg/cmd/chat.gopkg/cmd/chat_test.gopkg/cmd/chatmessagereaction.gopkg/cmd/chatmessagereaction_test.gopkg/cmd/chatreminder.gopkg/cmd/chatreminder_test.gopkg/cmd/cmd.gopkg/cmd/cmdutil.gopkg/cmd/cmdutil_test.gopkg/cmd/cmdutil_unix.gopkg/cmd/cmdutil_windows.gopkg/cmd/flagoptions.gopkg/cmd/flagoptions_test.gopkg/cmd/info.gopkg/cmd/info_test.gopkg/cmd/message.gopkg/cmd/message_test.gopkg/cmd/suggest.gopkg/cmd/version.gorelease-please-config.jsonscripts/bootstrapscripts/buildscripts/build-homebrew-archive.mjsscripts/linkscripts/lintscripts/publish-homebrew-formula.mjsscripts/runscripts/testscripts/unlinkscripts/utils/upload-artifact.shsrc/commands/llm.ts
💤 Files with no reviewable changes (41)
- internal/autocomplete/shellscripts/bash_autocomplete.bash
- pkg/cmd/chatmessagereaction.go
- internal/apiform/encoder.go
- .github/actions/setup-go/action.yml
- pkg/cmd/asset_test.go
- pkg/cmd/account_test.go
- pkg/cmd/chat_test.go
- pkg/cmd/beeperdesktopapi.go
- pkg/cmd/chatreminder_test.go
- internal/autocomplete/shellscripts/zsh_autocomplete.zsh
- go.mod
- pkg/cmd/accountcontact_test.go
- pkg/cmd/chat.go
- pkg/cmd/chatmessagereaction_test.go
- pkg/cmd/accountcontact.go
- internal/jsonview/staticdisplay.go
- pkg/cmd/beeperdesktopapi_test.go
- internal/autocomplete/shellscripts/fish_autocomplete.fish
- pkg/cmd/account.go
- internal/debugmiddleware/debug_middleware_test.go
- internal/apiform/form.go
- internal/autocomplete/autocomplete.go
- internal/apiquery/query.go
- internal/binaryparam/binary_param_test.go
- internal/requestflag/innerflag.go
- pkg/cmd/cmd.go
- internal/debugmiddleware/debug_middleware.go
- internal/binaryparam/binary_param.go
- pkg/cmd/chatreminder.go
- internal/apiquery/encoder.go
- internal/jsonview/explorer.go
- pkg/cmd/asset.go
- internal/requestflag/innerflag_test.go
- internal/autocomplete/shellscripts/pwsh_autocomplete.ps1
- cmd/beeper-desktop-cli/main.go
- internal/requestflag/requestflag_test.go
- internal/requestflag/requestflag.go
- internal/apiquery/query_test.go
- internal/autocomplete/autocomplete_test.go
- internal/mocktest/mocktest.go
- internal/apiform/form_test.go
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~248-~248: The official name of this software platform is spelled with a capital “H”.
Context: ...GitHub Releases. Push a v* tag to run .github/workflows/publish-release.yml. The re...
(GITHUB)
🔇 Additional comments (3)
package.json (1)
1-95: LGTM!.github/workflows/publish-release.yml (1)
15-47: LGTM!README.md (1)
1-128: LGTM!Also applies to: 130-209, 212-260
Multiple refactors and improvements across the CLI: - commands/current-user: use appRequest helper instead of manual fetch and token handling. - lib/client: prefer BEEPER_ACCESS_TOKEN env or config auth token before falling back to desktop auth; requireToken reads env/config first. - lib/desktop-auth: prefer probing the explicit baseURL first and concurrently probe remaining candidates via Promise.any for faster discovery. - lib/export: major rewrite to stream message and attachment data to disk using file handles, append to partial files, emit an attachments manifest as JSONL, and avoid accumulating large in-memory arrays; optimize chat collection for provided chat IDs with Promise.all; change downloadMessageAttachments to return counts and accept a write-entry callback; remove appendJSONL helper and add a MIME extension map helper. - lib/output: lazy-load Ink renderers to reduce startup cost, centralize JSON/JSONL writing, and make startStream async; rendering functions now dynamically import from ink/render. - lib/ink/render and lib/ui: small cleanup (removed comment and kept exports). Purpose: reduce memory pressure for large exports, speed up desktop discovery, support environment-provided tokens, and defer heavy Ink module loads to improve CLI startup performance.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
100-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrefix CLI subcommands with
beeperfor copy/paste correctness.Line 100 and Line 101 currently read like standalone binaries; they should be documented as
beepersubcommands.Suggested fix
-`commands --json` prints a compact command manifest for tools and agents. -`llm` prints a concise human-readable command guide. +`beeper commands --json` prints a compact command manifest for tools and agents. +`beeper llm` prints a concise human-readable command guide.🤖 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 `@README.md` around lines 100 - 101, Update the README lines that describe the CLI so the two commands are shown as beeper subcommands; change the text referencing `commands --json` to `beeper commands --json` and the text referencing `llm` to `beeper llm` so examples are copy/paste accurate and clearly indicate these are subcommands of the `beeper` binary.
🤖 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 `@README.md`:
- Around line 674-680: Several table cells contain unescaped pipe characters
that break Markdown table parsing (MD056); update each option cell value to
escape the internal '|' characters (e.g., in the entries for
`--inbox=<primary|low-priority|archive>`, `--scope=<titles|participants>`, and
`--type=<single|group|any>`) so the pipes are rendered literally (use `\|`) and
ensure the same change is applied to the other occurrences referenced in the
comment (the cells at the other listed locations).
---
Duplicate comments:
In `@README.md`:
- Around line 100-101: Update the README lines that describe the CLI so the two
commands are shown as beeper subcommands; change the text referencing `commands
--json` to `beeper commands --json` and the text referencing `llm` to `beeper
llm` so examples are copy/paste accurate and clearly indicate these are
subcommands of the `beeper` binary.
🪄 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: a5cb5fdc-7740-4e22-9396-b6fd19069ad0
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (153)
.github/actions/setup-go/action.yml.github/workflows/publish-release.ymlREADME.mdcmd/beeper-desktop-cli/main.gogo.modinternal/apiform/encoder.gointernal/apiform/form.gointernal/apiform/form_test.gointernal/apiquery/encoder.gointernal/apiquery/query.gointernal/apiquery/query_test.gointernal/autocomplete/autocomplete.gointernal/autocomplete/autocomplete_test.gointernal/autocomplete/shellscripts/bash_autocomplete.bashinternal/autocomplete/shellscripts/fish_autocomplete.fishinternal/autocomplete/shellscripts/pwsh_autocomplete.ps1internal/autocomplete/shellscripts/zsh_autocomplete.zshinternal/binaryparam/binary_param.gointernal/binaryparam/binary_param_test.gointernal/debugmiddleware/debug_middleware.gointernal/debugmiddleware/debug_middleware_test.gointernal/jsonview/explorer.gointernal/jsonview/staticdisplay.gointernal/mocktest/mocktest.gointernal/requestflag/innerflag.gointernal/requestflag/innerflag_test.gointernal/requestflag/requestflag.gointernal/requestflag/requestflag_test.gopackage.jsonpkg/cmd/account.gopkg/cmd/account_test.gopkg/cmd/accountcontact.gopkg/cmd/accountcontact_test.gopkg/cmd/asset.gopkg/cmd/asset_test.gopkg/cmd/beeperdesktopapi.gopkg/cmd/beeperdesktopapi_test.gopkg/cmd/chat.gopkg/cmd/chat_test.gopkg/cmd/chatmessagereaction.gopkg/cmd/chatmessagereaction_test.gopkg/cmd/chatreminder.gopkg/cmd/chatreminder_test.gopkg/cmd/cmd.gopkg/cmd/cmdutil.gopkg/cmd/cmdutil_test.gopkg/cmd/cmdutil_unix.gopkg/cmd/cmdutil_windows.gopkg/cmd/flagoptions.gopkg/cmd/flagoptions_test.gopkg/cmd/info.gopkg/cmd/info_test.gopkg/cmd/message.gopkg/cmd/message_test.gopkg/cmd/suggest.gopkg/cmd/version.gorelease-please-config.jsonscripts/bootstrapscripts/buildscripts/build-homebrew-archive.mjsscripts/generate-readme.mjsscripts/linkscripts/lintscripts/publish-homebrew-formula.mjsscripts/runscripts/testscripts/unlinkscripts/utils/upload-artifact.shsrc/commands/accounts.tssrc/commands/accounts/add.tssrc/commands/api/get.tssrc/commands/api/post.tssrc/commands/app/e2ee/recovery-code/mark-backed-up.tssrc/commands/app/e2ee/recovery-code/reset/begin.tssrc/commands/app/e2ee/recovery-code/reset/confirm.tssrc/commands/app/e2ee/recovery-code/verify.tssrc/commands/app/e2ee/verification/accept.tssrc/commands/app/e2ee/verification/cancel.tssrc/commands/app/e2ee/verification/qr/confirm-scanned.tssrc/commands/app/e2ee/verification/qr/scan.tssrc/commands/app/e2ee/verification/sas/confirm.tssrc/commands/app/e2ee/verification/sas/start.tssrc/commands/app/e2ee/verification/start.tssrc/commands/app/status.tssrc/commands/archive.tssrc/commands/assets/download.tssrc/commands/assets/upload.tssrc/commands/auth/login.tsxsrc/commands/auth/logout.tssrc/commands/auth/status.tssrc/commands/avatar.tssrc/commands/chat.tssrc/commands/chats/index.tssrc/commands/chats/search.tssrc/commands/clear-draft.tssrc/commands/commands.tssrc/commands/config/get.tssrc/commands/config/path.tssrc/commands/config/reset.tssrc/commands/config/set.tssrc/commands/contacts/list.tssrc/commands/contacts/search.tssrc/commands/create-chat.tssrc/commands/current-user.tssrc/commands/delete-message.tssrc/commands/description.tssrc/commands/doctor.tssrc/commands/draft.tssrc/commands/edit.tssrc/commands/focus.tssrc/commands/inbox.tssrc/commands/llm.tssrc/commands/low-priority.tssrc/commands/message-expiry.tssrc/commands/message.tssrc/commands/messages/index.tssrc/commands/messages/search.tssrc/commands/mute.tssrc/commands/notify-anyway.tssrc/commands/pin.tssrc/commands/react.tssrc/commands/read.tssrc/commands/remind.tssrc/commands/reply-file.tssrc/commands/reply.tssrc/commands/search.tssrc/commands/send-file.tssrc/commands/send.tssrc/commands/start-chat.tssrc/commands/status.tssrc/commands/title.tssrc/commands/unarchive.tssrc/commands/unmute.tssrc/commands/unpin.tssrc/commands/unreact.tssrc/commands/unread.tssrc/commands/unremind.tssrc/commands/watch.tssrc/lib/client.tssrc/lib/config.tssrc/lib/desktop-auth.tssrc/lib/export/index.tssrc/lib/ink/components.tsxsrc/lib/ink/format.tssrc/lib/ink/render.tsxsrc/lib/ink/spinner.tsxsrc/lib/ink/theme.tssrc/lib/manifest.tssrc/lib/output.tssrc/lib/ui.tstest/cli-smoke.mjstest/e2e-staging.mjstsconfig.json
💤 Files with no reviewable changes (28)
- internal/binaryparam/binary_param.go
- internal/apiform/form_test.go
- internal/apiquery/query.go
- internal/autocomplete/autocomplete_test.go
- internal/autocomplete/shellscripts/bash_autocomplete.bash
- pkg/cmd/account_test.go
- internal/mocktest/mocktest.go
- internal/requestflag/innerflag.go
- go.mod
- internal/debugmiddleware/debug_middleware_test.go
- internal/autocomplete/shellscripts/pwsh_autocomplete.ps1
- cmd/beeper-desktop-cli/main.go
- internal/requestflag/innerflag_test.go
- internal/autocomplete/shellscripts/fish_autocomplete.fish
- internal/autocomplete/shellscripts/zsh_autocomplete.zsh
- internal/jsonview/staticdisplay.go
- .github/actions/setup-go/action.yml
- internal/apiquery/query_test.go
- internal/requestflag/requestflag.go
- internal/requestflag/requestflag_test.go
- internal/jsonview/explorer.go
- pkg/cmd/account.go
- internal/apiform/form.go
- internal/apiform/encoder.go
- internal/autocomplete/autocomplete.go
- internal/apiquery/encoder.go
- internal/binaryparam/binary_param_test.go
- internal/debugmiddleware/debug_middleware.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/publish-release.yml
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~1781-~1781: The official name of this software platform is spelled with a capital “H”.
Context: ...GitHub Releases. Push a v* tag to run .github/workflows/publish-release.yml. The re...
(GITHUB)
🪛 markdownlint-cli2 (0.22.1)
README.md
[warning] 674-674: Table column count
Expected: 3; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 679-679: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 680-680: Table column count
Expected: 3; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 823-823: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 1223-1223: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 1230-1230: Table column count
Expected: 3; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
🔇 Additional comments (1)
package.json (1)
1-100: LGTM!
Promote login/logout and send subcommands to canonical commands, remove compatibility aliases, and add shared base command flags for JSON, events, and read-only handling. Regenerate README and update smoke/e2e coverage for the new command names.
Member
Author
|
@indent review |
|
Member
Author
|
@coderabbitai Review-body items handled:
|
|
Tip For best results, initiate chat on the files or code changes.
Would you like me to resolve the corresponding open review comments for any of these items? |
Delete the vendored OpenTUI skill and documentation from .agents/skills so this repository no longer carries that agent-specific reference material.
|
Review posted to the banner above. Headline findings: Critical \u2014 release/CI pipeline is non-functional as-shipped:
Functional: Latent: Nits called out inline (unused import in Tag me with |
![indent[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
Introduce a stable plugin SDK entrypoint for third-party oclif plugins by adding src/plugin-sdk.ts and exporting it from package.json. Document plugin usage and author guidance in the CLI README, and enable plugin management by adding @oclif/plugin-plugins to the oclif plugins list and dependencies.
Member
Author
|
@indent Addressed the headline findings in the pushed commits: workflows now use pnpm/Corepack and correct Homebrew package/upload paths (6065dc5), archive packaging uses pnpm instead of package-lock/npm ci (60c4e8b), watch uses ws with Authorization headers (fe4a861), human list renderers are awaited (bf5f1db), resolve/export normalize account list shapes and use maxParticipantCount while making attachment resume safer (5a3bf3d), and the two nits are fixed (cc2281d, 88f2b3e). Validation was attempted; typecheck/build still fail on pre-existing Desktop SDK typing errors for bridges/matrix in accounts add/account-login.\n\n- Batuhan's agent |
|
Verified all 11 fixes against the diffs in Two follow-ups worth pulling into this PR before the release pipeline goes green:
Tag me with |
| MACOS_NOTARY_ISSUER_ID: ${{ secrets.MACOS_NOTARY_ISSUER_ID }} | ||
| MACOS_NOTARY_KEY_ID: ${{ secrets.MACOS_NOTARY_KEY_ID }} | ||
| MACOS_NOTARY_KEY: ${{ secrets.MACOS_NOTARY_KEY }} No newline at end of file | ||
| run: node scripts/publish-homebrew-formula.mjs |
There was a problem hiding this comment.
Same scripts/ path bug as the upload step had — only this one wasn't fixed. After the monorepo move there's no scripts/ at the repo root (ls scripts/ returns No such file or directory); the publish script lives at packages/cli/scripts/publish-homebrew-formula.mjs. node scripts/publish-homebrew-formula.mjs from the repo root will fail with Cannot find module, so the formula publish step never runs even after the release assets upload succeeds. Fix with node packages/cli/scripts/publish-homebrew-formula.mjs or set working-directory: packages/cli for this step.
Suggested change
| run: node scripts/publish-homebrew-formula.mjs | |
| run: node packages/cli/scripts/publish-homebrew-formula.mjs |
| const client = await createClient(flags) | ||
|
|
||
| if (!args.account) { | ||
| const bridges = await client.bridges.list() |
There was a problem hiding this comment.
Typecheck regression you flagged. As you noted in the PR comments, pnpm build (and therefore pnpm pack:homebrew) fails with TS2339 on client.bridges (lines 33, 43) and client.matrix (lines 52, 64), and the same client.matrix errors hit lib/account-login.ts (lines 73, 98, 113). The pinned @beeper/desktop-api@^5.0.0 doesn't expose these resources, so CI's Typecheck → Test → Package Homebrew archive steps will all fail even after the pnpm migration. Bump the SDK to a version that exports bridges and matrix.bridges.auth, or fall back to client.get/client.post calls until it's published.
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
packages/cli/src/commands/api/post.ts-20-21 (1)
20-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle invalid
--bodyJSON with a CLI-friendly error.On Line 20, malformed JSON throws a raw parse error. Wrap parsing so users get a clear actionable message instead of an uncaught-style failure.
Proposed fix
+import { Args, Flags, Errors } from '`@oclif/core`' import { BeeperCommand, ensureWritable } from '../../lib/command.js' import { createClient } from '../../lib/client.js' import { printData } from '../../lib/output.js' @@ async run(): Promise<void> { const { args, flags } = await this.parse(ApiPost) ensureWritable(flags) const client = await createClient(flags) - const body = JSON.parse(flags.body) as Record<string, unknown> + let body: Record<string, unknown> + try { + body = JSON.parse(flags.body) as Record<string, unknown> + } catch { + throw new Errors.CLIError('Invalid JSON for --body') + } await printData(await client.post(args.path, { body }), flags.json ? 'json' : 'human') } }🤖 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/api/post.ts` around lines 20 - 21, Wrap the JSON.parse(flags.body) call in a try/catch to detect malformed JSON and produce a CLI-friendly error: catch the parse error thrown by JSON.parse(flags.body), log a clear message like "Invalid JSON in --body: <parse error message>" (or throw a CLI error), and exit with a non-zero status instead of letting the raw exception bubble up; then only call client.post(args.path, { body }) and printData(...) when parsing succeeds.packages/cli/src/commands/avatar.ts-23-27 (1)
23-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject conflicting
PATHand--cleartogether.Line 23 validates missing input, but not mutually exclusive input. Passing both currently clears the avatar and ignores
PATH, which is easy to do by mistake.Suggested fix
- if (!flags.clear && !args.path) throw new Error('Provide PATH or pass --clear') + if (flags.clear && args.path) throw new Error('Pass PATH or --clear, not both') + if (!flags.clear && !args.path) throw new Error('Provide PATH or pass --clear')🤖 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/avatar.ts` around lines 23 - 27, Add a mutual-exclusion check so callers cannot pass both PATH and --clear: in the avatar command handler (where flags.clear and args.path are currently checked before calling createClient, resolveChatID, client.chats.update, and printData) validate that exactly one of flags.clear or args.path is provided; if both are present throw a clear error (e.g., "Cannot provide both PATH and --clear") and if neither is present keep the existing "Provide PATH or pass --clear" behavior.packages/cli/src/lib/wait.ts-14-20 (1)
14-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTimeout can exceed requested
timeoutMsby one poll interval.After a failed attempt, the unconditional sleep can push total wait beyond the configured timeout. Bound sleep to remaining time before timeout.
Proposed fix
while (Date.now() - started < timeoutMs) { try { return await client.messages.retrieve(pendingMessageID, { chatID }) } catch (error) { lastError = error - await sleep(intervalMs) + const elapsedMs = Date.now() - started + const remainingMs = timeoutMs - elapsedMs + if (remainingMs <= 0) break + await sleep(Math.min(intervalMs, remainingMs)) } }🤖 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/lib/wait.ts` around lines 14 - 20, The loop in wait.ts can exceed timeoutMs because it always sleeps intervalMs after a failed retrieve; update the catch block in the loop that calls client.messages.retrieve(pendingMessageID, { chatID }) so that before sleeping you compute remainingMs = timeoutMs - (Date.now() - started) and if remainingMs <= 0 break/throw, otherwise await sleep(Math.min(intervalMs, remainingMs)); this ensures sleep is bounded to the remaining time and prevents the total wait from exceeding timeoutMs while preserving lastError behavior.packages/cli/src/commands/message-expiry.ts-23-23 (1)
23-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix validation message to match accepted values.
Line 23 allows
0but says “positive integer,” which excludes0. Please align message with actual validation rule.Suggested patch
- if (expiry !== null && (!Number.isInteger(expiry) || expiry < 0)) throw new Error('SECONDS must be a positive integer or "off"') + if (expiry !== null && (!Number.isInteger(expiry) || expiry < 0)) throw new Error('SECONDS must be a non-negative integer or "off"')🤖 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/message-expiry.ts` at line 23, The validation message is incorrect: the check at the expiry validation (the if that tests expiry !== null && (!Number.isInteger(expiry) || expiry < 0)) accepts 0 but the error text says "positive integer"; update the thrown message to state "non-negative integer or \"off\"" (or similar wording like "a non-negative integer or \"off\"") so it matches the implemented rule in the expiry validation.packages/cli/src/commands/accounts/add.ts-21-21 (1)
21-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
--flowflag help to match actual behavior.Line 21 says omitted
--flowuses a default flow, but Line 55–57 requires explicit--flowwhen multiple methods exist in non-interactive/JSON paths. This makes--helpmisleading.Suggested patch
- flow: Flags.string({ description: 'Login flow ID. If omitted, Desktop chooses the default flow.' }), + flow: Flags.string({ description: 'Login flow ID. If omitted, you can choose interactively; in non-interactive/JSON mode with multiple methods, pass --flow.' }),🤖 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` at line 21, Update the help text for the flow flag so it reflects actual behavior: modify the description passed to Flags.string for the flow flag (symbol "flow") in the Accounts add command to state that the CLI will pick a default flow in interactive mode but that when using non-interactive or JSON output paths (see the command's run method logic) you must supply --flow explicitly if multiple login methods exist; ensure the wording mentions both interactive default selection and the requirement for explicit --flow in non-interactive/JSON scenarios.packages/cli/src/lib/resolve.ts-64-68 (1)
64-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
pickexplicitly instead of relying on truthiness.Line 64 skips validation when
--pick 0is provided, which falls through to ambiguity handling. Validatepick >= 1whenever it is present.Suggested patch
- if (options.pick) { + if (options.pick !== undefined) { + if (!Number.isInteger(options.pick) || options.pick < 1) { + throw new Error('--pick must be a positive integer') + } const selected = matches[options.pick - 1] if (!selected) throw new Error(`--pick ${options.pick} is outside the ${matches.length} matching chats`) return String(selected.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/lib/resolve.ts` around lines 64 - 68, The current branch uses if (options.pick) so a value of 0 is treated as falsy and skips validation; change the check to explicitly detect presence (e.g., options.pick !== undefined) and validate the numeric range (ensure options.pick is >= 1 and <= matches.length) before indexing matches; if out of range throw the existing error message (`--pick ${options.pick} is outside the ${matches.length} matching chats`), then compute selected = matches[options.pick - 1] and return String(selected.id).packages/cli/test/cli-smoke.mjs-14-21 (1)
14-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse per-run config directories to avoid smoke-test cross-contamination.
These hard-coded
/tmp/...config directories are shared across runs, which can leak state and cause flaky results in parallel CI/local reruns. Generate unique temp dirs per run and clean them up at the end.💡 Suggested fix
-const run = (...args) => spawnSync(process.execPath, ['./bin/run.js', ...args], { +const smokeConfigDir = mkdtempSync(join(tmpdir(), 'beeper-cli-smoke-config-')) +const run = (...args) => spawnSync(process.execPath, ['./bin/run.js', ...args], { cwd: root, encoding: 'utf8', env: { ...process.env, - BEEPER_CLI_CONFIG_DIR: '/tmp/beeper-cli-test', + BEEPER_CLI_CONFIG_DIR: smokeConfigDir, }, }) @@ -const configDir = '/tmp/beeper-cli-test-config' +const configDir = mkdtempSync(join(tmpdir(), 'beeper-cli-test-config-')) @@ - BEEPER_CLI_CONFIG_DIR: '/tmp/beeper-cli-accounts-test', + BEEPER_CLI_CONFIG_DIR: mkdtempSync(join(tmpdir(), 'beeper-cli-accounts-test-')), @@ rmSync(exportRoot, { recursive: true, force: true }) +rmSync(smokeConfigDir, { recursive: true, force: true }) +rmSync(configDir, { recursive: true, force: true })Also applies to: 86-87, 146-147
🤖 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/cli-smoke.mjs` around lines 14 - 21, The smoke-test helper 'run' currently sets a hard-coded BEEPER_CLI_CONFIG_DIR ('/tmp/beeper-cli-test'), which can leak state across runs; change the test to create a unique temp directory per invocation (e.g., using fs.mkdtempSync or tmp-promise) and set BEEPER_CLI_CONFIG_DIR to that path in the env passed to run (also update the two other similar usages at the locations referenced around lines 86-87 and 146-147), and ensure tests remove the temp dirs after the run (or create/cleanup in before/after hooks) so each invocation uses an isolated config directory.packages/cli/src/lib/export/index.ts-168-168 (1)
168-168:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHonor explicit zero limits for chats/messages.
Line 168, Line 223, and Line 234 use truthy checks, so
0is treated as “no limit”. That makes--limit-chats 0/--limit-messages 0export data instead of none.Suggested fix
- if (options.limitChats && chats.length >= options.limitChats) break + if (options.limitChats !== undefined && chats.length >= options.limitChats) break ... - if (options.limitMessages && messagesWritten >= options.limitMessages) break + if (options.limitMessages !== undefined && messagesWritten >= options.limitMessages) break ... - if (options.limitMessages && messagesWritten >= options.limitMessages) break + if (options.limitMessages !== undefined && messagesWritten >= options.limitMessages) breakAlso applies to: 223-223, 234-234
🤖 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/lib/export/index.ts` at line 168, The checks use truthy checks so an explicit 0 is treated as “no limit”; change them to explicit null/undefined checks so 0 is honored. Replace occurrences like "if (options.limitChats && chats.length >= options.limitChats) break" with a guard that checks the option is defined (e.g. "if (options.limitChats !== undefined && options.limitChats !== null && chats.length >= options.limitChats) break"), and do the same for options.limitMessages checks (the other two occurrences referencing messages.length and options.limitMessages). Ensure you update all three occurrences (limitChats and both limitMessages checks) to use explicit defined checks rather than truthiness.packages/cli/test/e2e-staging.mjs-13-14 (1)
13-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the machine-specific default Desktop root.
Line 13 bakes in a user-local absolute path, which makes this script non-portable unless env override is always set.
Suggested fix
-const defaultDesktopRoot = '/Users/batuhan/.codex/worktrees/43bc/beeper/desktop' -const desktopRoot = process.env.BEEPER_DESKTOP_ROOT || defaultDesktopRoot +const desktopRoot = process.env.BEEPER_DESKTOP_ROOT +if (!desktopRoot) { + throw new Error('Set BEEPER_DESKTOP_ROOT to your Beeper Desktop checkout path') +}🤖 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.mjs` around lines 13 - 14, Remove the hardcoded user-specific path in the defaultDesktopRoot variable and make desktopRoot portable by deriving a default from the current user's home directory or requiring the environment variable; specifically, replace the literal '/Users/batuhan/…' assigned to defaultDesktopRoot with a computed path using the platform-safe home directory (e.g., path.join(os.homedir(), '.codex', 'worktrees', '43bc', 'beeper', 'desktop')) or make desktopRoot fall back to throwing a clear error if process.env.BEEPER_DESKTOP_ROOT is not set, updating the references to defaultDesktopRoot and desktopRoot accordingly.
🧹 Nitpick comments (1)
packages/cli/scripts/lint (1)
8-8: ⚡ Quick winSwitch lint script to repo's declared package manager (pnpm).
The workspace declares
pnpm@10.25.0at the root butpackages/cli/scripts/lintinvokesnpm run typecheck. This mismatch can cause inconsistent behavior between local environments and CI. Other scripts in the same directory (run, build, generate-readme.mjs) have the same issue. Update all script runners to usepnpm runfor consistency.🤖 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/lint` at line 8, Replace usage of the npm CLI with pnpm in the scripts to match the repository package manager: change the "npm run typecheck" call in packages/cli/scripts/lint to "pnpm run typecheck" and make the same substitution in the other script files in the same directory (packages/cli/scripts/run, packages/cli/scripts/build, packages/cli/scripts/generate-readme.mjs) so all script runners use "pnpm run" consistently.
🤖 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`:
- Around line 154-155: The generated CLI command reference is including
non-user-facing declaration artifacts like "accounts.d", "accounts add.d", and
"api get.d"; update the command-manifest generation logic (the routine that
builds the command reference/manifest) to filter out entries whose command names
end with ".d" or that are marked as declaration/generated before writing docs.
Locate the function that collects commands for the README (e.g., the manifest or
reference builder) and add a predicate to exclude names matching /\.d$/ or a
generated/declaration flag so only real user commands appear in the docs.
In `@packages/cli/scripts/publish-homebrew-formula.mjs`:
- Around line 2-8: The root path conversion is using .pathname which breaks on
Windows and with URL-encoded chars; import fileURLToPath from 'node:url' and
replace the current root assignment (root = new URL('..',
import.meta.url).pathname) with a call that uses fileURLToPath(new URL('..',
import.meta.url)) so the variable root is a correct cross-platform filesystem
path.
In `@packages/cli/src/commands/app/e2ee/recovery-code/reset/begin.ts`:
- Around line 31-36: The stty invocations (execFileSync('stty', ['-echo'], ...)
and execFileSync('stty', ['echo'], ...)) can throw on platforms without stty;
wrap both calls in try/catch and silently ignore errors so input still proceeds
(i.e., attempt to disable echo before calling rl.question and re-enable in the
finally block, but if execFileSync throws just continue), ensuring rl.question,
rl.close and output.write behavior is unchanged; reference execFileSync,
rl.question, rl.close and output.write when making the change.
In `@packages/cli/src/commands/contacts/search.ts`:
- Around line 25-30: The current catch block around
client.accounts.contacts.search(accountID, ...) silently swallows failures which
can hide operational issues; modify the loop to record per-account errors (e.g.,
push error + accountID into an errors array or set a failedAccounts list) and
only swallow individual errors while continuing, then after the loop check if
collected.length === 0 and errors.length > 0 and surface an
aggregated/meaningful error (or rethrow) so callers see an operational failure
instead of an empty “no matches”; use the existing symbols
client.accounts.contacts.search, collected, result.items and accountID to locate
and implement the change.
In `@packages/cli/src/commands/export.ts`:
- Line 2: The export command must respect read-only mode: call and await
ensureWritable(flags) at the start of the command handler (before instantiating
the client and before any export/write actions) so the write-guard is enforced;
add the ensureWritable import if missing and insert await ensureWritable(flags)
in the BeeperCommand-derived command's run/execute method immediately prior to
creating the client/exporting files.
In `@packages/cli/src/lib/account-login.ts`:
- Around line 130-137: promptSecret currently calls execFileSync('stty', ...)
without handling failures or tracking whether echo was actually disabled; wrap
the disabling call in a try/catch and set a boolean (e.g., echoDisabled) only if
execFileSync succeeds, fall back to calling promptText(label) when execFileSync
throws (or when input.isTTY is false), and in the finally block only call
execFileSync('stty', ['echo'], ...) and output.write('\n') if echoDisabled is
true; reference promptSecret, promptText, execFileSync, input.isTTY, and
output.write when applying the change.
In `@packages/cli/src/lib/app-api.ts`:
- Around line 22-43: The appRequest function can return undefined for HTTP 204
but is declared as Promise<T>, so update the contract: change the function
signature of appRequest<T> to return Promise<T | undefined>, adjust the 204
branch to return undefined as T | undefined, and update any callers (or their
typings) expecting T to handle undefined responses; locate the function
appRequest in app-api.ts and update its return type and the return expressions
accordingly.
In `@packages/cli/src/lib/export/index.ts`:
- Around line 291-297: The current code in the export flow accepts
attacker-controlled attachment URLs and will copy local files when sourceURL
startsWith 'file://' or '/' (see sourceURL, copyFile, fileURLToPath,
absoluteTarget, target), which allows exfiltration of sensitive local files;
restrict this by rejecting local-file schemes by default, only allowing them
when an explicit opt-in flag is present and the resolved path is inside a
configured allowlist of trusted roots (normalize/resolve paths and verify
against allowed roots before calling fileURLToPath/copyFile), and return a clear
error when local sources are not permitted.
In `@packages/cli/src/lib/ink/components.tsx`:
- Around line 624-626: Top-level masking uses a regex but nested objects only
match exact keys, so update the rendering in components.tsx to reuse a shared
secret-key regex and mask nested secret-like keys too: introduce a single
secretKeyRegex (e.g. /(token|secret|key)/i), use that instead of the current
/^(accesstoken|token|secret|key)$/i test, and when rendering object values
(where the code currently uses typeof v === 'object' ? JSON.stringify(v) :
String(v)) iterate the object's entries and apply secretKeyRegex.test(propName)
to decide whether to call maskToken(value) for each nested property so keys like
refreshToken or clientSecret are masked consistently.
- Around line 9-15: The OSC-8 sequences are malformed: update the constants so
the ESC introducer is explicit and the closing OSC-8 segment is emitted; set
OSC8_START to '\x1b]8;;', BEL to '\x07', and OSC8_END to '\x1b]8;;\x07' and
ensure the Hyperlink component (and its use of supportsHyperlinks and Text)
concatenates OSC8_START + url + BEL + (children ?? url) + OSC8_END so terminals
can parse the hyperlink correctly.
In `@packages/cli/src/lib/ink/render.tsx`:
- Around line 289-291: The push method currently appends events unboundedly;
modify push (in the same object where push(event) calls setState with current
and events) to cap the events array to a fixed max (e.g., MAX_EVENTS = 1000) by
creating a new array that appends the incoming event and then takes only the
last MAX_EVENTS entries (use array slice or similar) before calling setState;
ensure you reference the existing symbols push, setState, current, and events so
the change preserves order while preventing unbounded memory growth.
- Around line 266-282: The interruptHandlers array is never populated so the
onInterrupt callback ends up no-op; fix this by exposing a way to register
handlers into that array when binding the StreamView: after setting api = hooks
in the bind block, add a registration method (e.g., hooks.registerInterrupt or
api.registerInterrupt) that pushes a provided callback into interruptHandlers
(interruptHandlers.push(fn)), and ensure callers use that registerInterrupt to
add their Ctrl+C handlers; keep the existing hooks.onInterrupt invocation that
iterates interruptHandlers to invoke them.
In `@packages/cli/test/e2e-staging.mjs`:
- Around line 96-106: The failure arises because pseudo-commands (the `.d`
entries in manifest) are counted as real commands; update the missing
calculation to ignore such pseudo-commands by first filtering manifest items
whose command endsWith('.d') (or otherwise identifies them as non-user) before
mapping and checking against coveredCommands so that report.missingCommands and
the subsequent throw only consider real CLI commands (adjust the expression that
defines missing and ensure writeReport()/error logic uses this filtered
missing).
---
Minor comments:
In `@packages/cli/src/commands/accounts/add.ts`:
- Line 21: Update the help text for the flow flag so it reflects actual
behavior: modify the description passed to Flags.string for the flow flag
(symbol "flow") in the Accounts add command to state that the CLI will pick a
default flow in interactive mode but that when using non-interactive or JSON
output paths (see the command's run method logic) you must supply --flow
explicitly if multiple login methods exist; ensure the wording mentions both
interactive default selection and the requirement for explicit --flow in
non-interactive/JSON scenarios.
In `@packages/cli/src/commands/api/post.ts`:
- Around line 20-21: Wrap the JSON.parse(flags.body) call in a try/catch to
detect malformed JSON and produce a CLI-friendly error: catch the parse error
thrown by JSON.parse(flags.body), log a clear message like "Invalid JSON in
--body: <parse error message>" (or throw a CLI error), and exit with a non-zero
status instead of letting the raw exception bubble up; then only call
client.post(args.path, { body }) and printData(...) when parsing succeeds.
In `@packages/cli/src/commands/avatar.ts`:
- Around line 23-27: Add a mutual-exclusion check so callers cannot pass both
PATH and --clear: in the avatar command handler (where flags.clear and args.path
are currently checked before calling createClient, resolveChatID,
client.chats.update, and printData) validate that exactly one of flags.clear or
args.path is provided; if both are present throw a clear error (e.g., "Cannot
provide both PATH and --clear") and if neither is present keep the existing
"Provide PATH or pass --clear" behavior.
In `@packages/cli/src/commands/message-expiry.ts`:
- Line 23: The validation message is incorrect: the check at the expiry
validation (the if that tests expiry !== null && (!Number.isInteger(expiry) ||
expiry < 0)) accepts 0 but the error text says "positive integer"; update the
thrown message to state "non-negative integer or \"off\"" (or similar wording
like "a non-negative integer or \"off\"") so it matches the implemented rule in
the expiry validation.
In `@packages/cli/src/lib/export/index.ts`:
- Line 168: The checks use truthy checks so an explicit 0 is treated as “no
limit”; change them to explicit null/undefined checks so 0 is honored. Replace
occurrences like "if (options.limitChats && chats.length >= options.limitChats)
break" with a guard that checks the option is defined (e.g. "if
(options.limitChats !== undefined && options.limitChats !== null && chats.length
>= options.limitChats) break"), and do the same for options.limitMessages checks
(the other two occurrences referencing messages.length and
options.limitMessages). Ensure you update all three occurrences (limitChats and
both limitMessages checks) to use explicit defined checks rather than
truthiness.
In `@packages/cli/src/lib/resolve.ts`:
- Around line 64-68: The current branch uses if (options.pick) so a value of 0
is treated as falsy and skips validation; change the check to explicitly detect
presence (e.g., options.pick !== undefined) and validate the numeric range
(ensure options.pick is >= 1 and <= matches.length) before indexing matches; if
out of range throw the existing error message (`--pick ${options.pick} is
outside the ${matches.length} matching chats`), then compute selected =
matches[options.pick - 1] and return String(selected.id).
In `@packages/cli/src/lib/wait.ts`:
- Around line 14-20: The loop in wait.ts can exceed timeoutMs because it always
sleeps intervalMs after a failed retrieve; update the catch block in the loop
that calls client.messages.retrieve(pendingMessageID, { chatID }) so that before
sleeping you compute remainingMs = timeoutMs - (Date.now() - started) and if
remainingMs <= 0 break/throw, otherwise await sleep(Math.min(intervalMs,
remainingMs)); this ensures sleep is bounded to the remaining time and prevents
the total wait from exceeding timeoutMs while preserving lastError behavior.
In `@packages/cli/test/cli-smoke.mjs`:
- Around line 14-21: The smoke-test helper 'run' currently sets a hard-coded
BEEPER_CLI_CONFIG_DIR ('/tmp/beeper-cli-test'), which can leak state across
runs; change the test to create a unique temp directory per invocation (e.g.,
using fs.mkdtempSync or tmp-promise) and set BEEPER_CLI_CONFIG_DIR to that path
in the env passed to run (also update the two other similar usages at the
locations referenced around lines 86-87 and 146-147), and ensure tests remove
the temp dirs after the run (or create/cleanup in before/after hooks) so each
invocation uses an isolated config directory.
In `@packages/cli/test/e2e-staging.mjs`:
- Around line 13-14: Remove the hardcoded user-specific path in the
defaultDesktopRoot variable and make desktopRoot portable by deriving a default
from the current user's home directory or requiring the environment variable;
specifically, replace the literal '/Users/batuhan/…' assigned to
defaultDesktopRoot with a computed path using the platform-safe home directory
(e.g., path.join(os.homedir(), '.codex', 'worktrees', '43bc', 'beeper',
'desktop')) or make desktopRoot fall back to throwing a clear error if
process.env.BEEPER_DESKTOP_ROOT is not set, updating the references to
defaultDesktopRoot and desktopRoot accordingly.
---
Nitpick comments:
In `@packages/cli/scripts/lint`:
- Line 8: Replace usage of the npm CLI with pnpm in the scripts to match the
repository package manager: change the "npm run typecheck" call in
packages/cli/scripts/lint to "pnpm run typecheck" and make the same substitution
in the other script files in the same directory (packages/cli/scripts/run,
packages/cli/scripts/build, packages/cli/scripts/generate-readme.mjs) so all
script runners use "pnpm run" consistently.
🪄 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: 66560138-a4b4-41ef-bf4b-0dedb2881227
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (186)
.changeset/README.md.changeset/config.json.github/actions/setup-go/action.yml.github/workflows/ci.yml.github/workflows/publish-release.ymlREADME.mdcmd/beeper-desktop-cli/main.gogo.modinternal/apiform/encoder.gointernal/apiform/form.gointernal/apiform/form_test.gointernal/apiquery/encoder.gointernal/apiquery/query.gointernal/apiquery/query_test.gointernal/autocomplete/autocomplete.gointernal/autocomplete/autocomplete_test.gointernal/autocomplete/shellscripts/bash_autocomplete.bashinternal/autocomplete/shellscripts/fish_autocomplete.fishinternal/autocomplete/shellscripts/pwsh_autocomplete.ps1internal/autocomplete/shellscripts/zsh_autocomplete.zshinternal/binaryparam/binary_param.gointernal/binaryparam/binary_param_test.gointernal/debugmiddleware/debug_middleware.gointernal/debugmiddleware/debug_middleware_test.gointernal/jsonview/explorer.gointernal/jsonview/staticdisplay.gointernal/mocktest/mocktest.gointernal/requestflag/innerflag.gointernal/requestflag/innerflag_test.gointernal/requestflag/requestflag.gointernal/requestflag/requestflag_test.gopackage.jsonpackages/cli/CHANGELOG.mdpackages/cli/LICENSEpackages/cli/README.mdpackages/cli/SECURITY.mdpackages/cli/bin/check-release-environmentpackages/cli/bin/dev.jspackages/cli/bin/run.jspackages/cli/package.jsonpackages/cli/release-please-config.jsonpackages/cli/scripts/bootstrappackages/cli/scripts/buildpackages/cli/scripts/build-homebrew-archive.mjspackages/cli/scripts/check-api-copy.mjspackages/cli/scripts/formatpackages/cli/scripts/generate-readme.mjspackages/cli/scripts/linkpackages/cli/scripts/lintpackages/cli/scripts/mockpackages/cli/scripts/publish-homebrew-formula.mjspackages/cli/scripts/runpackages/cli/scripts/testpackages/cli/scripts/unlinkpackages/cli/src/commands/accounts.tspackages/cli/src/commands/accounts/add.tspackages/cli/src/commands/api/get.tspackages/cli/src/commands/api/post.tspackages/cli/src/commands/app/e2ee/recovery-code/mark-backed-up.tspackages/cli/src/commands/app/e2ee/recovery-code/reset/begin.tspackages/cli/src/commands/app/e2ee/recovery-code/reset/confirm.tspackages/cli/src/commands/app/e2ee/recovery-code/verify.tspackages/cli/src/commands/app/e2ee/verification/accept.tspackages/cli/src/commands/app/e2ee/verification/cancel.tspackages/cli/src/commands/app/e2ee/verification/qr/confirm-scanned.tspackages/cli/src/commands/app/e2ee/verification/qr/scan.tspackages/cli/src/commands/app/e2ee/verification/sas/confirm.tspackages/cli/src/commands/app/e2ee/verification/sas/start.tspackages/cli/src/commands/app/e2ee/verification/start.tspackages/cli/src/commands/app/status.tspackages/cli/src/commands/archive.tspackages/cli/src/commands/assets/download.tspackages/cli/src/commands/assets/upload.tspackages/cli/src/commands/auth/status.tspackages/cli/src/commands/avatar.tspackages/cli/src/commands/chat.tspackages/cli/src/commands/chats/index.tspackages/cli/src/commands/chats/search.tspackages/cli/src/commands/clear-draft.tspackages/cli/src/commands/commands.tspackages/cli/src/commands/config/get.tspackages/cli/src/commands/config/path.tspackages/cli/src/commands/config/reset.tspackages/cli/src/commands/config/set.tspackages/cli/src/commands/contacts/list.tspackages/cli/src/commands/contacts/search.tspackages/cli/src/commands/create-chat.tspackages/cli/src/commands/current-user.tspackages/cli/src/commands/delete-message.tspackages/cli/src/commands/description.tspackages/cli/src/commands/doctor.tspackages/cli/src/commands/draft.tspackages/cli/src/commands/edit.tspackages/cli/src/commands/export.tspackages/cli/src/commands/focus.tspackages/cli/src/commands/inbox.tspackages/cli/src/commands/llm.tspackages/cli/src/commands/login.tsxpackages/cli/src/commands/logout.tspackages/cli/src/commands/low-priority.tspackages/cli/src/commands/message-expiry.tspackages/cli/src/commands/message.tspackages/cli/src/commands/messages/index.tspackages/cli/src/commands/messages/search.tspackages/cli/src/commands/mute.tspackages/cli/src/commands/notify-anyway.tspackages/cli/src/commands/pin.tspackages/cli/src/commands/react.tspackages/cli/src/commands/read.tspackages/cli/src/commands/remind.tspackages/cli/src/commands/rpc.tspackages/cli/src/commands/search.tspackages/cli/src/commands/send/file.tspackages/cli/src/commands/send/text.tspackages/cli/src/commands/shell.tspackages/cli/src/commands/start-chat.tspackages/cli/src/commands/status.tspackages/cli/src/commands/title.tspackages/cli/src/commands/unarchive.tspackages/cli/src/commands/unmute.tspackages/cli/src/commands/unpin.tspackages/cli/src/commands/unreact.tspackages/cli/src/commands/unread.tspackages/cli/src/commands/unremind.tspackages/cli/src/commands/watch.tspackages/cli/src/lib/account-login.tspackages/cli/src/lib/app-api.tspackages/cli/src/lib/argv.tspackages/cli/src/lib/client.tspackages/cli/src/lib/command.tspackages/cli/src/lib/config.tspackages/cli/src/lib/copy.tspackages/cli/src/lib/desktop-auth.tspackages/cli/src/lib/export/index.tspackages/cli/src/lib/ink/components.tsxpackages/cli/src/lib/ink/format.tspackages/cli/src/lib/ink/render.tsxpackages/cli/src/lib/ink/spinner.tsxpackages/cli/src/lib/ink/theme.tspackages/cli/src/lib/manifest.tspackages/cli/src/lib/oauth.tspackages/cli/src/lib/output.tspackages/cli/src/lib/pkce.tspackages/cli/src/lib/resolve.tspackages/cli/src/lib/runner.tspackages/cli/src/lib/send-message.tspackages/cli/src/lib/wait.tspackages/cli/src/plugin-sdk.tspackages/cli/test/cli-smoke.mjspackages/cli/test/e2e-staging.mjspackages/cli/tsconfig.jsonpkg/cmd/account.gopkg/cmd/account_test.gopkg/cmd/accountcontact.gopkg/cmd/accountcontact_test.gopkg/cmd/asset.gopkg/cmd/asset_test.gopkg/cmd/beeperdesktopapi.gopkg/cmd/beeperdesktopapi_test.gopkg/cmd/chat.gopkg/cmd/chat_test.gopkg/cmd/chatmessagereaction.gopkg/cmd/chatmessagereaction_test.gopkg/cmd/chatreminder.gopkg/cmd/chatreminder_test.gopkg/cmd/cmd.gopkg/cmd/cmdutil.gopkg/cmd/cmdutil_test.gopkg/cmd/cmdutil_unix.gopkg/cmd/cmdutil_windows.gopkg/cmd/flagoptions.gopkg/cmd/flagoptions_test.gopkg/cmd/info.gopkg/cmd/info_test.gopkg/cmd/message.gopkg/cmd/message_test.gopkg/cmd/suggest.gopkg/cmd/version.gopnpm-workspace.yamlscripts/buildscripts/linkscripts/lintscripts/runscripts/unlinkscripts/utils/upload-artifact.shtsconfig.base.json
💤 Files with no reviewable changes (51)
- internal/binaryparam/binary_param_test.go
- pkg/cmd/info_test.go
- internal/debugmiddleware/debug_middleware.go
- internal/apiform/encoder.go
- .github/actions/setup-go/action.yml
- pkg/cmd/flagoptions.go
- go.mod
- cmd/beeper-desktop-cli/main.go
- internal/autocomplete/shellscripts/bash_autocomplete.bash
- internal/autocomplete/shellscripts/fish_autocomplete.fish
- pkg/cmd/cmdutil_test.go
- internal/apiquery/query.go
- pkg/cmd/accountcontact.go
- pkg/cmd/account_test.go
- internal/debugmiddleware/debug_middleware_test.go
- pkg/cmd/chat_test.go
- internal/apiform/form_test.go
- internal/autocomplete/autocomplete_test.go
- pkg/cmd/flagoptions_test.go
- internal/autocomplete/shellscripts/pwsh_autocomplete.ps1
- pkg/cmd/cmdutil_unix.go
- internal/apiquery/query_test.go
- internal/mocktest/mocktest.go
- pkg/cmd/asset_test.go
- internal/requestflag/requestflag_test.go
- pkg/cmd/chatmessagereaction.go
- internal/binaryparam/binary_param.go
- pkg/cmd/beeperdesktopapi_test.go
- pkg/cmd/account.go
- pkg/cmd/chatreminder_test.go
- internal/apiquery/encoder.go
- pkg/cmd/cmdutil_windows.go
- pkg/cmd/chatreminder.go
- pkg/cmd/chatmessagereaction_test.go
- pkg/cmd/info.go
- internal/jsonview/staticdisplay.go
- pkg/cmd/beeperdesktopapi.go
- pkg/cmd/cmdutil.go
- internal/requestflag/innerflag_test.go
- pkg/cmd/message.go
- pkg/cmd/cmd.go
- internal/autocomplete/shellscripts/zsh_autocomplete.zsh
- README.md
- internal/autocomplete/autocomplete.go
- pkg/cmd/accountcontact_test.go
- pkg/cmd/asset.go
- internal/jsonview/explorer.go
- internal/requestflag/requestflag.go
- internal/apiform/form.go
- internal/requestflag/innerflag.go
- pkg/cmd/chat.go
✅ Files skipped from review due to trivial changes (8)
- .changeset/README.md
- packages/cli/scripts/run
- packages/cli/scripts/link
- .changeset/config.json
- packages/cli/tsconfig.json
- packages/cli/src/commands/config/reset.ts
- packages/cli/bin/check-release-environment
- packages/cli/src/lib/ink/theme.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.42.2)
packages/cli/src/lib/export/index.ts
[warning] 532-535: Manual HTML sanitization detected using string replacement methods. Manual sanitization is error-prone and can be bypassed. Use dedicated HTML sanitization libraries like 'sanitize-html' or 'DOMPurify' instead.
Context: value
.replaceAll('&', '&')
.replaceAll('<', '<')
.replaceAll('>', '>')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html
- https://cwe.mitre.org/data/definitions/79.html
(manual-html-sanitization)
[warning] 532-534: Manual HTML sanitization detected using string replacement methods. Manual sanitization is error-prone and can be bypassed. Use dedicated HTML sanitization libraries like 'sanitize-html' or 'DOMPurify' instead.
Context: value
.replaceAll('&', '&')
.replaceAll('<', '<')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html
- https://cwe.mitre.org/data/definitions/79.html
(manual-html-sanitization)
[warning] 532-535: Manual HTML entity encoding detected. Avoid manually sanitizing HTML input as it can be error-prone and lead to XSS vulnerabilities. Use dedicated libraries like sanitize-html or DOMPurify for proper HTML sanitization.
Context: value
.replaceAll('&', '&')
.replaceAll('<', '<')
.replaceAll('>', '>')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://cwe.mitre.org/data/definitions/79.html
- https://owasp.org/www-project-cheat-sheets/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html
(manual-html-entity-encoding)
[warning] 532-534: Manual HTML entity encoding detected. Avoid manually sanitizing HTML input as it can be error-prone and lead to XSS vulnerabilities. Use dedicated libraries like sanitize-html or DOMPurify for proper HTML sanitization.
Context: value
.replaceAll('&', '&')
.replaceAll('<', '<')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://cwe.mitre.org/data/definitions/79.html
- https://owasp.org/www-project-cheat-sheets/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html
(manual-html-entity-encoding)
🪛 LanguageTool
packages/cli/README.md
[uncategorized] ~49-~49: The official name of this software platform is spelled with a capital “H”.
Context: ... should use namespaced commands such as beeper github issue create and must treat installed ...
(GITHUB)
[uncategorized] ~2226-~2226: The official name of this software platform is spelled with a capital “H”.
Context: ...GitHub Releases. Push a v* tag to run .github/workflows/publish-release.yml. The re...
(GITHUB)
🔇 Additional comments (94)
packages/cli/src/commands/config/path.ts (1)
1-17: LGTM!packages/cli/src/commands/app/status.ts (1)
1-14: LGTM!packages/cli/src/commands/remind.ts (1)
1-36: LGTM!packages/cli/src/commands/config/set.ts (1)
1-23: LGTM!packages/cli/src/commands/app/e2ee/verification/start.ts (1)
1-22: LGTM!packages/cli/src/commands/app/e2ee/recovery-code/verify.ts (1)
1-22: LGTM!packages/cli/src/commands/edit.ts (1)
1-30: LGTM!packages/cli/src/commands/chats/search.ts (1)
1-62: LGTM!packages/cli/bin/dev.js (1)
1-4: LGTM!packages/cli/src/commands/config/get.ts (1)
6-25: LGTM!packages/cli/src/commands/app/e2ee/verification/cancel.ts (1)
7-25: LGTM!packages/cli/src/commands/app/e2ee/recovery-code/mark-backed-up.ts (1)
6-16: LGTM!packages/cli/src/commands/delete-message.ts (1)
8-33: LGTM!packages/cli/src/commands/app/e2ee/recovery-code/reset/confirm.ts (1)
7-21: LGTM!packages/cli/src/commands/current-user.ts (1)
1-13: LGTM!packages/cli/src/commands/status.ts (1)
1-25: LGTM!packages/cli/src/commands/send/file.ts (1)
1-44: LGTM!packages/cli/src/commands/chats/index.ts (1)
1-41: LGTM!packages/cli/src/commands/draft.ts (1)
1-51: LGTM!packages/cli/scripts/unlink (1)
1-8: LGTM!packages/cli/src/commands/app/e2ee/verification/sas/start.ts (1)
1-22: LGTM!packages/cli/src/commands/app/e2ee/verification/sas/confirm.ts (1)
1-22: LGTM!packages/cli/src/commands/app/e2ee/verification/qr/scan.ts (1)
1-22: LGTM!packages/cli/src/commands/logout.ts (1)
1-30: LGTM!packages/cli/src/commands/chat.ts (1)
1-27: LGTM!packages/cli/src/lib/manifest.ts (1)
1-75: LGTM!packages/cli/src/commands/focus.ts (1)
1-32: LGTM!packages/cli/src/commands/message.ts (1)
1-25: LGTM!packages/cli/src/commands/read.ts (1)
1-26: LGTM!packages/cli/scripts/build (1)
1-8: LGTM!packages/cli/src/commands/unarchive.ts (1)
1-25: LGTM!packages/cli/src/lib/copy.ts (1)
1-66: LGTM!packages/cli/src/commands/inbox.ts (1)
1-25: LGTM!packages/cli/src/commands/assets/download.ts (1)
1-20: LGTM!packages/cli/src/commands/accounts.ts (1)
1-42: LGTM!packages/cli/src/commands/llm.ts (1)
1-18: LGTM!packages/cli/package.json (1)
1-115: LGTM!packages/cli/src/commands/react.ts (1)
1-32: LGTM!packages/cli/src/lib/argv.ts (1)
1-47: LGTM!packages/cli/src/lib/pkce.ts (1)
1-16: LGTM!packages/cli/src/commands/notify-anyway.ts (1)
1-25: LGTM!packages/cli/src/commands/archive.ts (1)
1-25: LGTM!packages/cli/src/commands/auth/status.ts (1)
1-22: LGTM!packages/cli/src/lib/runner.ts (1)
1-38: LGTM!packages/cli/src/commands/unremind.ts (1)
8-24: LGTM!packages/cli/src/commands/pin.ts (1)
8-24: LGTM!packages/cli/bin/run.js (1)
1-4: LGTM!packages/cli/src/lib/command.ts (1)
3-38: LGTM!packages/cli/src/commands/commands.ts (1)
5-12: LGTM!packages/cli/src/commands/description.ts (1)
20-28: LGTM!packages/cli/src/commands/search.ts (1)
1-28: LGTM!packages/cli/src/commands/clear-draft.ts (1)
1-24: LGTM!packages/cli/src/lib/ink/spinner.tsx (1)
1-121: LGTM!packages/cli/src/commands/shell.ts (2)
1-30: LGTM!Also applies to: 35-40
31-33: ⚡ Quick winThe concern about
this.error()with{ exit: false }terminating the session is incorrect.According to oclif documentation,
this.error(message, { exit: false })does not throw or cause process termination—it displays the error message and continues execution. The process-terminating logic is explicitly bypassed whenexit: falseis set. The current code in the loop is already safe and will not break the interactive session.Additionally, the proposed fix references
this.logToStderr(), which does not exist in this codebase.> Likely an incorrect or invalid review comment.packages/cli/src/plugin-sdk.ts (1)
1-38: LGTM!packages/cli/src/commands/messages/search.ts (1)
1-67: LGTM!packages/cli/src/commands/send/text.ts (1)
1-44: LGTM!packages/cli/release-please-config.json (1)
64-65: LGTM!packages/cli/src/commands/unread.ts (1)
1-26: LGTM!packages/cli/src/commands/app/e2ee/verification/qr/confirm-scanned.ts (1)
1-22: LGTM!packages/cli/src/commands/title.ts (1)
1-26: LGTM!packages/cli/src/commands/rpc.ts (1)
1-54: LGTM!packages/cli/src/commands/unpin.ts (1)
1-25: LGTM!packages/cli/src/lib/send-message.ts (1)
1-44: LGTM!packages/cli/src/commands/messages/index.ts (1)
1-48: LGTM!packages/cli/src/commands/low-priority.ts (1)
8-25: LGTM!packages/cli/scripts/test (1)
56-56: LGTM!packages/cli/scripts/bootstrap (1)
21-25: LGTM!packages/cli/src/commands/app/e2ee/verification/accept.ts (1)
7-22: LGTM!packages/cli/src/commands/unmute.ts (1)
7-24: LGTM!packages/cli/src/commands/create-chat.ts (1)
8-32: LGTM!packages/cli/src/commands/contacts/list.ts (1)
9-62: LGTM!packages/cli/src/commands/login.tsx (1)
40-173: LGTM!packages/cli/src/lib/desktop-auth.ts (1)
16-96: LGTM!packages/cli/src/commands/doctor.ts (1)
9-58: LGTM!packages/cli/scripts/check-api-copy.mjs (1)
1-77: LGTM!packages/cli/src/commands/assets/upload.ts (1)
8-29: LGTM!packages/cli/src/lib/app-api.ts (1)
45-62: LGTM!.github/workflows/publish-release.yml (1)
14-47: LGTM!.github/workflows/ci.yml (1)
5-43: LGTM!packages/cli/src/lib/client.ts (1)
5-23: LGTM!packages/cli/src/lib/config.ts (1)
5-60: LGTM!packages/cli/src/commands/api/get.ts (1)
6-20: LGTM!packages/cli/src/lib/oauth.ts (1)
29-168: LGTM!packages/cli/scripts/generate-readme.mjs (1)
1-253: LGTM!packages/cli/src/commands/mute.ts (1)
1-24: LGTM!packages/cli/src/commands/unreact.ts (1)
1-30: LGTM!packages/cli/scripts/build-homebrew-archive.mjs (1)
1-116: LGTM!packages/cli/src/commands/start-chat.ts (1)
1-58: LGTM!packages/cli/src/commands/watch.ts (1)
1-106: LGTM!packages/cli/src/lib/ink/format.ts (1)
1-121: LGTM!package.json (1)
1-25: LGTM!packages/cli/src/lib/output.ts (1)
1-121: LGTM!
Comment on lines
+154
to
+155
| | `accounts add.d` | | | ||
| | `accounts.d` | | |
There was a problem hiding this comment.
Generated command reference includes non-user .d commands.
Entries like accounts.d, api get.d, etc. should not be in user-facing docs; this indicates command-manifest generation is pulling declaration artifacts/noise.
Also applies to: 334-349
🤖 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/README.md` around lines 154 - 155, The generated CLI command
reference is including non-user-facing declaration artifacts like "accounts.d",
"accounts add.d", and "api get.d"; update the command-manifest generation logic
(the routine that builds the command reference/manifest) to filter out entries
whose command names end with ".d" or that are marked as declaration/generated
before writing docs. Locate the function that collects commands for the README
(e.g., the manifest or reference builder) and add a predicate to exclude names
matching /\.d$/ or a generated/declaration flag so only real user commands
appear in the docs.
Comment on lines
+2
to
+8
| import {existsSync} from 'node:fs'; | ||
| import {mkdir, mkdtemp, readFile, rm, writeFile} from 'node:fs/promises'; | ||
| import {tmpdir} from 'node:os'; | ||
| import {join} from 'node:path'; | ||
| import {spawn} from 'node:child_process'; | ||
|
|
||
| const root = new URL('..', import.meta.url).pathname; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/cli/scripts/publish-homebrew-formula.mjs | head -20Repository: beeper/desktop-api-cli
Length of output: 1287
🌐 Web query:
fileURLToPath vs pathname Node.js filesystem paths Windows
💡 Result:
In Node.js, you should use url.fileURLToPath instead of the URL.pathname property when converting a file URL to a filesystem path [1][2]. Using pathname directly is unreliable and often produces incorrect results, especially on Windows [3][2]. Key differences and reasons to use fileURLToPath: 1. Platform-Specific Formatting: On Windows, fileURLToPath correctly converts a file URL (e.g., file:///C:/path/) into a valid Windows-style path with backslashes (C:\path) [3][2]. In contrast, accessing.pathname on a Windows file URL typically returns a string with a leading slash and forward slashes (e.g., /C:/path/), which is not a valid path for most Node.js filesystem APIs [3][4]. 2. Percent-Encoding Handling: fileURLToPath automatically decodes percent-encoded characters (such as %20 for spaces or %E4%BD%A0 for non-ASCII characters) [3][2]. The.pathname property does not perform this decoding, leaving the string in its URL-encoded format [3][1]. 3. UNC Path Support: fileURLToPath correctly handles Windows UNC paths (e.g., file://nas/foo.txt), converting them to the appropriate format (\nas\foo.txt) [3][2]. The.pathname property would incorrectly return /foo.txt [3][2]. In summary, while.pathname is a raw component of the URL object, fileURLToPath is a utility specifically designed to resolve these URLs into platform-appropriate, usable filesystem paths [3][2]. Always use fileURLToPath to ensure cross-platform compatibility and correct path formatting [3][1].
Citations:
- 1: https://exploringjs.com/nodejs-shell-scripting/ch_nodejs-path.html
- 2: https://nodejs.org/api/url.html
- 3: https://docs.deno.com/api/node/url/~/fileURLToPath
- 4: https://stackoverflow.com/questions/66081774/how-to-specify-in-nodejs-file-url-a-absolute-path-under-windows
🏁 Script executed:
cat -n packages/cli/scripts/publish-homebrew-formula.mjs | grep -E "new URL|\.pathname"Repository: beeper/desktop-api-cli
Length of output: 354
🏁 Script executed:
rg "new URL.*import.meta.url" --type js --type mjs -A 1 -B 1Repository: beeper/desktop-api-cli
Length of output: 94
🏁 Script executed:
rg "new URL.*import.meta.url" --type js -A 1 -B 1Repository: beeper/desktop-api-cli
Length of output: 48
Use fileURLToPath for filesystem path conversion.
Line 8 uses .pathname, which produces incorrect filesystem paths on Windows (returns /C:/path/ instead of C:\path\) and fails to decode URL-encoded characters (spaces as %20, non-ASCII characters, etc.). Node.js documentation recommends using fileURLToPath for cross-platform compatibility.
Proposed fix
import {existsSync} from 'node:fs';
import {mkdir, mkdtemp, readFile, rm, writeFile} from 'node:fs/promises';
import {tmpdir} from 'node:os';
import {join} from 'node:path';
+import {fileURLToPath} from 'node:url';
import {spawn} from 'node:child_process';
-const root = new URL('..', import.meta.url).pathname;
+const root = fileURLToPath(new URL('..', import.meta.url));📝 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
| import {existsSync} from 'node:fs'; | |
| import {mkdir, mkdtemp, readFile, rm, writeFile} from 'node:fs/promises'; | |
| import {tmpdir} from 'node:os'; | |
| import {join} from 'node:path'; | |
| import {spawn} from 'node:child_process'; | |
| const root = new URL('..', import.meta.url).pathname; | |
| import {existsSync} from 'node:fs'; | |
| import {mkdir, mkdtemp, readFile, rm, writeFile} from 'node:fs/promises'; | |
| import {tmpdir} from 'node:os'; | |
| import {join} from 'node:path'; | |
| import {fileURLToPath} from 'node:url'; | |
| import {spawn} from 'node:child_process'; | |
| const root = fileURLToPath(new URL('..', import.meta.url)); |
🤖 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/publish-homebrew-formula.mjs` around lines 2 - 8, The
root path conversion is using .pathname which breaks on Windows and with
URL-encoded chars; import fileURLToPath from 'node:url' and replace the current
root assignment (root = new URL('..', import.meta.url).pathname) with a call
that uses fileURLToPath(new URL('..', import.meta.url)) so the variable root is
a correct cross-platform filesystem path.
Comment on lines
+31
to
+36
| execFileSync('stty', ['-echo'], { stdio: ['inherit', 'ignore', 'ignore'] }) | ||
| return (await rl.question(label)).trim() | ||
| } finally { | ||
| rl.close() | ||
| execFileSync('stty', ['echo'], { stdio: ['inherit', 'ignore', 'ignore'] }) | ||
| output.write('\n') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/cli/src/commands/app/e2ee/recovery-code/reset/begin.tsRepository: beeper/desktop-api-cli
Length of output: 2010
🏁 Script executed:
rg 'stty' --type ts --type jsRepository: beeper/desktop-api-cli
Length of output: 707
🏁 Script executed:
fd package.json | head -5 | xargs catRepository: beeper/desktop-api-cli
Length of output: 3932
🏁 Script executed:
rg -B 5 -A 5 "stty" packages/cli/src/lib/account-login.tsRepository: beeper/desktop-api-cli
Length of output: 421
🏁 Script executed:
rg -i 'windows|win32|platform|darwin' packages/cli/src --type ts --type js | head -20Repository: beeper/desktop-api-cli
Length of output: 637
🏁 Script executed:
rg -B 3 -A 8 'promptSecret' packages/cli/src/lib/account-login.ts | head -30Repository: beeper/desktop-api-cli
Length of output: 886
Wrap stty calls in error handling for platform portability.
Lines 31 and 35 call stty without catching errors. On platforms where stty is unavailable (notably Windows with modern TTY support), this causes a hard crash before input is captured. The function should gracefully fall back to standard input without echo suppression when stty is unavailable.
Suggested fix
async function promptSecret(label: string): Promise<string> {
const rl = createInterface({ input, output })
+ let echoDisabled = false
try {
- execFileSync('stty', ['-echo'], { stdio: ['inherit', 'ignore', 'ignore'] })
+ try {
+ execFileSync('stty', ['-echo'], { stdio: ['inherit', 'ignore', 'ignore'] })
+ echoDisabled = true
+ } catch {
+ // Fallback: continue without echo suppression when stty is unavailable.
+ }
return (await rl.question(label)).trim()
} finally {
rl.close()
- execFileSync('stty', ['echo'], { stdio: ['inherit', 'ignore', 'ignore'] })
- output.write('\n')
+ if (echoDisabled) {
+ execFileSync('stty', ['echo'], { stdio: ['inherit', 'ignore', 'ignore'] })
+ output.write('\n')
+ }
}
}📝 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
| execFileSync('stty', ['-echo'], { stdio: ['inherit', 'ignore', 'ignore'] }) | |
| return (await rl.question(label)).trim() | |
| } finally { | |
| rl.close() | |
| execFileSync('stty', ['echo'], { stdio: ['inherit', 'ignore', 'ignore'] }) | |
| output.write('\n') | |
| async function promptSecret(label: string): Promise<string> { | |
| const rl = createInterface({ input, output }) | |
| let echoDisabled = false | |
| try { | |
| try { | |
| execFileSync('stty', ['-echo'], { stdio: ['inherit', 'ignore', 'ignore'] }) | |
| echoDisabled = true | |
| } catch { | |
| // Fallback: continue without echo suppression when stty is unavailable. | |
| } | |
| return (await rl.question(label)).trim() | |
| } finally { | |
| rl.close() | |
| if (echoDisabled) { | |
| execFileSync('stty', ['echo'], { stdio: ['inherit', 'ignore', 'ignore'] }) | |
| output.write('\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/src/commands/app/e2ee/recovery-code/reset/begin.ts` around lines
31 - 36, The stty invocations (execFileSync('stty', ['-echo'], ...) and
execFileSync('stty', ['echo'], ...)) can throw on platforms without stty; wrap
both calls in try/catch and silently ignore errors so input still proceeds
(i.e., attempt to disable echo before calling rl.question and re-enable in the
finally block, but if execFileSync throws just continue), ensuring rl.question,
rl.close and output.write behavior is unchanged; reference execFileSync,
rl.question, rl.close and output.write when making the change.
Comment on lines
+25
to
+30
| try { | ||
| const result = await client.accounts.contacts.search(accountID, { query: args.query }) | ||
| collected.push(...result.items.map((item: unknown) => ({ ...(item as Record<string, unknown>), accountID }))) | ||
| } catch { | ||
| // Some networks reject exact lookups for some identifiers; keep trying the rest. | ||
| } |
There was a problem hiding this comment.
Don’t silently swallow all account-level failures.
If every account request fails, this currently returns an empty result and looks like “no matches” instead of an operational failure.
Suggested patch
const load = async (): Promise<Array<Record<string, unknown>>> => {
const collected: Array<Record<string, unknown>> = []
+ let successCount = 0
for (const accountID of accountIDs) {
try {
const result = await client.accounts.contacts.search(accountID, { query: args.query })
+ successCount += 1
collected.push(...result.items.map((item: unknown) => ({ ...(item as Record<string, unknown>), accountID })))
} catch {
// Some networks reject exact lookups for some identifiers; keep trying the rest.
}
}
+ if (accountIDs.length > 0 && successCount === 0) {
+ throw new Error('Contact search failed for all selected accounts')
+ }
return collected
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| const result = await client.accounts.contacts.search(accountID, { query: args.query }) | |
| collected.push(...result.items.map((item: unknown) => ({ ...(item as Record<string, unknown>), accountID }))) | |
| } catch { | |
| // Some networks reject exact lookups for some identifiers; keep trying the rest. | |
| } | |
| const load = async (): Promise<Array<Record<string, unknown>>> => { | |
| const collected: Array<Record<string, unknown>> = [] | |
| let successCount = 0 | |
| for (const accountID of accountIDs) { | |
| try { | |
| const result = await client.accounts.contacts.search(accountID, { query: args.query }) | |
| successCount += 1 | |
| collected.push(...result.items.map((item: unknown) => ({ ...(item as Record<string, unknown>), accountID }))) | |
| } catch { | |
| // Some networks reject exact lookups for some identifiers; keep trying the rest. | |
| } | |
| } | |
| if (accountIDs.length > 0 && successCount === 0) { | |
| throw new Error('Contact search failed for all selected accounts') | |
| } | |
| return collected | |
| } |
🤖 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/contacts/search.ts` around lines 25 - 30, The
current catch block around client.accounts.contacts.search(accountID, ...)
silently swallows failures which can hide operational issues; modify the loop to
record per-account errors (e.g., push error + accountID into an errors array or
set a failedAccounts list) and only swallow individual errors while continuing,
then after the loop check if collected.length === 0 and errors.length > 0 and
surface an aggregated/meaningful error (or rethrow) so callers see an
operational failure instead of an empty “no matches”; use the existing symbols
client.accounts.contacts.search, collected, result.items and accountID to locate
and implement the change.
| @@ -0,0 +1,50 @@ | |||
| import { Flags } from '@oclif/core' | |||
| import { BeeperCommand } from '../lib/command.js' | |||
There was a problem hiding this comment.
Respect read-only mode before starting export.
This command writes local state (export files) but skips the write guard. Add ensureWritable(flags) before creating the client/exporting.
Proposed fix
-import { BeeperCommand } from '../lib/command.js'
+import { BeeperCommand, ensureWritable } from '../lib/command.js'
@@
async run(): Promise<void> {
const { flags } = await this.parse(Export)
+ ensureWritable(flags)
const client = await createClient(flags)Also applies to: 27-29
🤖 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/export.ts` at line 2, The export command must
respect read-only mode: call and await ensureWritable(flags) at the start of the
command handler (before instantiating the client and before any export/write
actions) so the write-guard is enforced; add the ensureWritable import if
missing and insert await ensureWritable(flags) in the BeeperCommand-derived
command's run/execute method immediately prior to creating the client/exporting
files.
Comment on lines
+9
to
+15
| const OSC8_START = ']8;;' | ||
| const OSC8_END = ']8;;' | ||
| const BEL = '' | ||
| const Hyperlink: React.FC<{ url: string; children?: React.ReactNode }> = ({ url, children }) => { | ||
| if (!supportsHyperlinks) return <>{children ?? url}</> | ||
| return <Text>{OSC8_START}{url}{BEL}{children ?? url}{OSC8_END}</Text> | ||
| } |
There was a problem hiding this comment.
Fix OSC-8 hyperlink escape sequence generation.
The current sequence omits the ESC introducer and closing OSC-8 segment, so terminals won’t parse it as a hyperlink.
Suggested fix
-const OSC8_START = ';;'
-const OSC8_END = ''
+const ESC = '\u001B'
+const OSC8_START = `${ESC}]8;;`
+const OSC8_END = `${ESC}]8;;`
const BEL = '�'
const Hyperlink: React.FC<{ url: string; children?: React.ReactNode }> = ({ url, children }) => {
if (!supportsHyperlinks) return <>{children ?? url}</>
- return <Text>{OSC8_START}{url}{BEL}{children ?? url}{OSC8_END}</Text>
+ return <Text>{OSC8_START}{url}{BEL}{children ?? url}{OSC8_END}{BEL}</Text>
}🤖 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/lib/ink/components.tsx` around lines 9 - 15, The OSC-8
sequences are malformed: update the constants so the ESC introducer is explicit
and the closing OSC-8 segment is emitted; set OSC8_START to '\x1b]8;;', BEL to
'\x07', and OSC8_END to '\x1b]8;;\x07' and ensure the Hyperlink component (and
its use of supportsHyperlinks and Text) concatenates OSC8_START + url + BEL +
(children ?? url) + OSC8_END so terminals can parse the hyperlink correctly.
Comment on lines
+624
to
+626
| {/^(accesstoken|token|secret|key)$/i.test(k) && typeof v === 'string' | ||
| ? <Text color={theme.subtle}>{maskToken(v)}</Text> | ||
| : <Text color={theme.text}>{typeof v === 'object' ? JSON.stringify(v) : String(v)}</Text>} |
There was a problem hiding this comment.
Mask nested secret-like keys consistently.
Top-level masking uses /token|secret|key/i, but nested masking only matches exact keys (token|secret|key|accesstoken). Keys like refreshToken or clientSecret in nested objects can leak.
Suggested fix
- {/^(accesstoken|token|secret|key)$/i.test(k) && typeof v === 'string'
+ {/token|secret|key/i.test(k) && typeof v === 'string'
? <Text color={theme.subtle}>{maskToken(v)}</Text>
: <Text color={theme.text}>{typeof v === 'object' ? JSON.stringify(v) : String(v)}</Text>}📝 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
| {/^(accesstoken|token|secret|key)$/i.test(k) && typeof v === 'string' | |
| ? <Text color={theme.subtle}>{maskToken(v)}</Text> | |
| : <Text color={theme.text}>{typeof v === 'object' ? JSON.stringify(v) : String(v)}</Text>} | |
| {/token|secret|key/i.test(k) && typeof v === 'string' | |
| ? <Text color={theme.subtle}>{maskToken(v)}</Text> | |
| : <Text color={theme.text}>{typeof v === 'object' ? JSON.stringify(v) : String(v)}</Text>} |
🤖 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/lib/ink/components.tsx` around lines 624 - 626, Top-level
masking uses a regex but nested objects only match exact keys, so update the
rendering in components.tsx to reuse a shared secret-key regex and mask nested
secret-like keys too: introduce a single secretKeyRegex (e.g.
/(token|secret|key)/i), use that instead of the current
/^(accesstoken|token|secret|key)$/i test, and when rendering object values
(where the code currently uses typeof v === 'object' ? JSON.stringify(v) :
String(v)) iterate the object's entries and apply secretKeyRegex.test(propName)
to decide whether to call maskToken(value) for each nested property so keys like
refreshToken or clientSecret are masked consistently.
Comment on lines
+266
to
+282
| const interruptHandlers: Array<() => void> = [] | ||
|
|
||
| const setState = (next: StreamState): void => { | ||
| current = next | ||
| api?.update(next) | ||
| } | ||
|
|
||
| const instance = inkRender( | ||
| <StreamView | ||
| initialState={initial} | ||
| baseURL={opts.baseURL} | ||
| subscribed={opts.subscribed} | ||
| bind={hooks => { | ||
| api = hooks | ||
| hooks.onInterrupt(() => { | ||
| for (const fn of interruptHandlers) fn() | ||
| }) |
There was a problem hiding this comment.
Ctrl+C interrupt callback currently does nothing.
interruptHandlers is never populated, so the registered interrupt callback can no-op and leave stream mode running.
Suggested fix
bind={hooks => {
api = hooks
hooks.onInterrupt(() => {
- for (const fn of interruptHandlers) fn()
+ if (interruptHandlers.length === 0) {
+ hooks.exit()
+ return
+ }
+ for (const fn of interruptHandlers) fn()
})
}}📝 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
| const interruptHandlers: Array<() => void> = [] | |
| const setState = (next: StreamState): void => { | |
| current = next | |
| api?.update(next) | |
| } | |
| const instance = inkRender( | |
| <StreamView | |
| initialState={initial} | |
| baseURL={opts.baseURL} | |
| subscribed={opts.subscribed} | |
| bind={hooks => { | |
| api = hooks | |
| hooks.onInterrupt(() => { | |
| for (const fn of interruptHandlers) fn() | |
| }) | |
| const interruptHandlers: Array<() => void> = [] | |
| const setState = (next: StreamState): void => { | |
| current = next | |
| api?.update(next) | |
| } | |
| const instance = inkRender( | |
| <StreamView | |
| initialState={initial} | |
| baseURL={opts.baseURL} | |
| subscribed={opts.subscribed} | |
| bind={hooks => { | |
| api = hooks | |
| hooks.onInterrupt(() => { | |
| if (interruptHandlers.length === 0) { | |
| hooks.exit() | |
| return | |
| } | |
| for (const fn of interruptHandlers) fn() | |
| }) |
🤖 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/lib/ink/render.tsx` around lines 266 - 282, The
interruptHandlers array is never populated so the onInterrupt callback ends up
no-op; fix this by exposing a way to register handlers into that array when
binding the StreamView: after setting api = hooks in the bind block, add a
registration method (e.g., hooks.registerInterrupt or api.registerInterrupt)
that pushes a provided callback into interruptHandlers
(interruptHandlers.push(fn)), and ensure callers use that registerInterrupt to
add their Ctrl+C handlers; keep the existing hooks.onInterrupt invocation that
iterates interruptHandlers to invoke them.
Comment on lines
+289
to
+291
| push(event) { | ||
| setState({ ...current, events: [...current.events, event] }) | ||
| }, |
There was a problem hiding this comment.
Cap in-memory stream history to avoid unbounded growth.
push() appends forever, so long-running watch sessions can grow memory without bound.
Suggested fix
+ const MAX_EVENTS = 2000
...
push(event) {
- setState({ ...current, events: [...current.events, event] })
+ const nextEvents = [...current.events, event]
+ setState({ ...current, events: nextEvents.slice(-MAX_EVENTS) })
},🤖 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/lib/ink/render.tsx` around lines 289 - 291, The push method
currently appends events unboundedly; modify push (in the same object where
push(event) calls setState with current and events) to cap the events array to a
fixed max (e.g., MAX_EVENTS = 1000) by creating a new array that appends the
incoming event and then takes only the last MAX_EVENTS entries (use array slice
or similar) before calling setState; ensure you reference the existing symbols
push, setState, current, and events so the change preserves order while
preventing unbounded memory growth.
Comment on lines
+96
to
+106
| const missing = manifest.map(item => item.command).filter(command => !coveredCommands.has(command)) | ||
| report.missingCommands = missing | ||
| if (report.missingOpenAPIOperations?.length) { | ||
| throw new Error(`E2E did not cover ${report.missingOpenAPIOperations.length} OpenAPI operation(s): ${report.missingOpenAPIOperations.join(', ')}`) | ||
| } | ||
| report.finishedAt = new Date().toISOString() | ||
| await writeReport() | ||
|
|
||
| if (missing.length) { | ||
| throw new Error(`E2E did not exercise ${missing.length} command(s): ${missing.join(', ')}`) | ||
| } |
There was a problem hiding this comment.
Filter pseudo-commands before enforcing full command coverage.
The manifest currently includes non-user .d entries (also visible in generated docs), so this strict check can fail even when real CLI commands are covered.
Suggested fix
- const missing = manifest.map(item => item.command).filter(command => !coveredCommands.has(command))
+ const manifestCommands = manifest
+ .map(item => item.command)
+ .filter(command => !command.endsWith('.d'))
+ const missing = manifestCommands.filter(command => !coveredCommands.has(command))📝 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
| const missing = manifest.map(item => item.command).filter(command => !coveredCommands.has(command)) | |
| report.missingCommands = missing | |
| if (report.missingOpenAPIOperations?.length) { | |
| throw new Error(`E2E did not cover ${report.missingOpenAPIOperations.length} OpenAPI operation(s): ${report.missingOpenAPIOperations.join(', ')}`) | |
| } | |
| report.finishedAt = new Date().toISOString() | |
| await writeReport() | |
| if (missing.length) { | |
| throw new Error(`E2E did not exercise ${missing.length} command(s): ${missing.join(', ')}`) | |
| } | |
| const manifestCommands = manifest | |
| .map(item => item.command) | |
| .filter(command => !command.endsWith('.d')) | |
| const missing = manifestCommands.filter(command => !coveredCommands.has(command)) | |
| report.missingCommands = missing | |
| if (report.missingOpenAPIOperations?.length) { | |
| throw new Error(`E2E did not cover ${report.missingOpenAPIOperations.length} OpenAPI operation(s): ${report.missingOpenAPIOperations.join(', ')}`) | |
| } | |
| report.finishedAt = new Date().toISOString() | |
| await writeReport() | |
| if (missing.length) { | |
| throw new Error(`E2E did not exercise ${missing.length} command(s): ${missing.join(', ')}`) | |
| } |
🤖 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.mjs` around lines 96 - 106, The failure arises
because pseudo-commands (the `.d` entries in manifest) are counted as real
commands; update the missing calculation to ignore such pseudo-commands by first
filtering manifest items whose command endsWith('.d') (or otherwise identifies
them as non-user) before mapping and checking against coveredCommands so that
report.missingCommands and the subsequent throw only consider real CLI commands
(adjust the expression that defines missing and ensure writeReport()/error logic
uses this filtered missing).
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.
No description provided.