Merged
Conversation
- Add packages/cli with Commander, API key auth, config persistence - Exclude auth endpoints; mirror core API nesting - Generate commands from OpenAPI (scripts/generate-cli.mjs) - Config: API_KEY/BASILIC_API_KEY env or ~/.config/basilic/config.json - Docs: README, development/cli.mdx, agentic integrations note
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
WalkthroughA new CLI package, Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Program
participant Config as Config Manager
participant Gen as Generated Metadata
participant Client as API Client
participant Server as Basilic API Server
User->>CLI: invoke command (args/flags)
CLI->>Config: resolve API key (env → config → prompt)
Config-->>CLI: apiKey, baseUrl
CLI->>Gen: lookup commandSpec & operationMeta
Gen-->>CLI: command metadata
CLI->>Client: construct client (baseUrl, apiKey) and call runCommand
Client->>Server: HTTP request
Server-->>Client: response
Client-->>CLI: result
CLI->>User: output JSON / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
biome.json (1)
149-157: Narrow the CLI-wide lint carve-out.This disables
noConsoleandnoNonNullAssertionfor every file inpackages/cli, including secret-handling code likesrc/config.ts. Please scope the exception to the specific command/output files that actually need raw console access or non-null assertions.♻️ Suggested scope reduction
- { - "includes": ["**/packages/cli/**"], + { + "includes": [ + "**/packages/cli/src/cli.ts", + "**/packages/cli/src/run-command.ts", + "**/packages/cli/scripts/**" + ], "linter": { "rules": { "suspicious": { "noConsole": "off" }, "style": { "noNonNullAssertion": "off" } } } },As per coding guidelines "SECURITY: Verify no secrets logged (use logger redaction features)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@biome.json` around lines 149 - 157, The current biome.json broad rule carve-out disables suspicious.noConsole and style.noNonNullAssertion for every file under the packages/cli include; narrow this by changing the include/glob so the exceptions only apply to the actual CLI command/output files (e.g., CLI command modules and entrypoint scripts) instead of the entire package, and keep the rest of the package governed by the default linter rules; update the linter rules block so suspicious.noConsole and style.noNonNullAssertion are scoped to that narrower include (leave config files like src/config.ts outside the exception).packages/cli/scripts/generate-cli.mjs (1)
65-76: Generate richer parameter metadata instead of only names.This script currently emits
pathParamsandbodyParamsas{ name }only. That is whypackages/cli/src/run-command.ts, Lines 48-68, has to special-case fields likemessages,tools,stream, andtemperature, and why there is nowhere to represent query params or schema-driven boolean/number/object parsing. Emittingin,required, and schema/type info here would keep the runtime generic as the OpenAPI spec evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/scripts/generate-cli.mjs` around lines 65 - 76, The getParams function currently returns only names for pathParams and bodyParams; update it to emit richer metadata including the parameter location (in), required flag, and schema/type information so the runtime can generically handle query params and typed parsing. Specifically, extend getParams to (1) iterate operation.parameters and push objects like { name: p.name, in: p.in, required: !!p.required, schema: p.schema } into separate arrays for pathParams and queryParams (create queryParams alongside pathParams), and (2) when reading operation.requestBody?.content?.['application/json']?.schema, emit bodyParams as { name, required: (body.required?.includes(name) || false), schema: body.properties[name] } instead of just { name }; keep the same function name getParams and return shape { pathParams, queryParams, bodyParams } so callers (e.g., run-command.ts) can remove ad-hoc special-casing and perform schema-driven parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docu/content/docs/development/cli.mdx`:
- Line 34: The markdown link currently uses the placeholder "your-org" which
creates a dead link; update the link target in the CLI docs line that reads "See
[packages/cli/README.md](https://github.com/your-org/basilic/blob/main/packages/cli/README.md)"
to the real repository/org or an internal docs route (e.g., replace "your-org"
with the actual GitHub org name or point to the internal docs path) so the URL
resolves correctly; ensure the visible link text remains accurate and that the
change appears in the CLI docs MDX file where that link is defined.
In `@packages/cli/README.md`:
- Around line 18-23: Update the README line that lists config file locations to
use the environment variable notation for XDG_CONFIG_HOME: replace the literal
path string "XDG_CONFIG_HOME/basilic/config.json" with
"$XDG_CONFIG_HOME/basilic/config.json" so it clearly indicates an environment
variable rather than a literal directory; ensure the other listed path
(`~/.config/basilic/config.json`) remains unchanged.
- Around line 7-14: Remove the misleading global install instruction that
suggests running "pnpm add -g `@repo/cli`" because the package is marked private;
update the README code block to only show the monorepo-local workflow (the pnpm
--filter `@repo/cli` build and node packages/cli/dist/cli.js --help commands) and
delete or replace any reference to installing `@repo/cli` globally so readers are
not instructed to publish/Install a private package.
In `@packages/cli/src/cli.ts`:
- Around line 47-52: The code is coercing every top-level value of cmdOpts.body
into strings (using String(v)) which loses arrays/objects/booleans; instead,
parse rawBody into an object and assign the parsed value(s) without
stringifying: e.g. keep the parsed object and set opts.body = parsed (or assign
opts[k] = v without String(v)) so types are preserved for downstream code (see
rawBody, cmdOpts.body, parsed, opts and the consumer in run-command.ts); update
the branch to return opts with the original parsed values rather than their
String() representations.
- Around line 10-17: The promptApiKey function currently uses readline.question
which echoes the API key; change promptApiKey to suppress echo by either
(preferred) replacing readline usage with a secure prompt library like prompts
or inquirer that supports hidden input, or implement manual echo suppression by
setting process.stdin.setRawMode(true), reading character-by-character, writing
a mask (or nothing) to stdout, and restoring raw mode before resolving; ensure
the function name promptApiKey and any callers (e.g., config set-api-key flow)
use the updated implementation and that stdin state is always restored on
errors.
- Around line 128-130: Replace the current use of this.parent?.opts() when
computing baseUrl inside the cmd.action handler so global/root options are
visible for nested commands: call this.optsWithGlobals() and read baseUrl from
that merged options object (fall back to resolveBaseUrl() only if the merged
optsWithGlobals().baseUrl is undefined); similarly replace this.opts?.() used to
populate optsFromCmd with this.optsWithGlobals() to ensure global options are
included. Ensure you update references in the cmd.action anonymous function (the
code that assigns baseUrl and optsFromCmd) and keep resolveBaseUrl() as the
final fallback.
In `@packages/cli/src/config.ts`:
- Around line 38-48: The saveConfig function writes secrets to disk with default
permissions; change directory creation and file write to use restrictive modes
so the config (apiKey) is not world-readable: when calling mkdirSync for the
config directory (in saveConfig using getConfigPath / dir) pass a mode of 0o700
(or equivalent) and when writing the file (writeFileSync) ensure the file is
created with mode 0o600 (or call chmodSync after write to set 0o600); also
consider detecting existing files and tightening their permissions if needed
(use loadConfig, getConfigPath, mkdirSync, writeFileSync, chmodSync as the
referenced symbols).
In `@packages/cli/src/run-command.ts`:
- Around line 58-66: The CLI currently silently mutates/omits flags on parse
errors; update the parsing in run-command so that when p.name === 'stream' you
explicitly accept only 'true'/'1' -> true and 'false'/'0' -> false and otherwise
throw/exit with a clear error about an invalid stream value, and when p.name ===
'tools' and typeof v === 'string' attempt JSON.parse(v) but on parse failure do
not set bodyParams[p.name] = undefined — instead throw/exit with a clear error
indicating malformed JSON for --tools; keep the existing handling for
temperature/model (using Number.parseFloat for 'temperature') but ensure invalid
numeric input also results in a validation error rather than silently skipping.
---
Nitpick comments:
In `@biome.json`:
- Around line 149-157: The current biome.json broad rule carve-out disables
suspicious.noConsole and style.noNonNullAssertion for every file under the
packages/cli include; narrow this by changing the include/glob so the exceptions
only apply to the actual CLI command/output files (e.g., CLI command modules and
entrypoint scripts) instead of the entire package, and keep the rest of the
package governed by the default linter rules; update the linter rules block so
suspicious.noConsole and style.noNonNullAssertion are scoped to that narrower
include (leave config files like src/config.ts outside the exception).
In `@packages/cli/scripts/generate-cli.mjs`:
- Around line 65-76: The getParams function currently returns only names for
pathParams and bodyParams; update it to emit richer metadata including the
parameter location (in), required flag, and schema/type information so the
runtime can generically handle query params and typed parsing. Specifically,
extend getParams to (1) iterate operation.parameters and push objects like {
name: p.name, in: p.in, required: !!p.required, schema: p.schema } into separate
arrays for pathParams and queryParams (create queryParams alongside pathParams),
and (2) when reading
operation.requestBody?.content?.['application/json']?.schema, emit bodyParams as
{ name, required: (body.required?.includes(name) || false), schema:
body.properties[name] } instead of just { name }; keep the same function name
getParams and return shape { pathParams, queryParams, bodyParams } so callers
(e.g., run-command.ts) can remove ad-hoc special-casing and perform
schema-driven parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1834fe99-2edf-401a-a889-54c86a78a06b
⛔ Files ignored due to path filters (3)
packages/cli/src/gen/commands.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.tspackages/cli/tsconfig.jsonis excluded by!**/tsconfig*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (13)
README.mdapps/docu/content/docs/development/cli.mdxapps/docu/content/docs/development/packages.mdxbiome.jsonpackages/cli/README.mdpackages/cli/eslint.config.jspackages/cli/package.jsonpackages/cli/scripts/generate-cli.mjspackages/cli/src/cli.tspackages/cli/src/config.tspackages/cli/src/run-command.tspackages/cli/tsup.config.tsturbo.json
Comment on lines
+47
to
+52
| const rawBody = cmdOpts.body as string | undefined | ||
| if (rawBody) | ||
| try { | ||
| const parsed = JSON.parse(rawBody) as Record<string, unknown> | ||
| for (const [k, v] of Object.entries(parsed)) if (v !== undefined) opts[k] = String(v) | ||
| return opts |
There was a problem hiding this comment.
--body is lossy here, not raw JSON.
After parsing, every top-level value is coerced with String(v). Arrays/objects become "[object Object]", booleans become strings, and nested payloads stop matching the API contract before they even reach packages/cli/src/run-command.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/cli.ts` around lines 47 - 52, The code is coercing every
top-level value of cmdOpts.body into strings (using String(v)) which loses
arrays/objects/booleans; instead, parse rawBody into an object and assign the
parsed value(s) without stringifying: e.g. keep the parsed object and set
opts.body = parsed (or assign opts[k] = v without String(v)) so types are
preserved for downstream code (see rawBody, cmdOpts.body, parsed, opts and the
consumer in run-command.ts); update the branch to return opts with the original
parsed values rather than their String() representations.
- fix cli.mdx dead link (your-org -> blockmatic) - fix README: use $XDG_CONFIG_HOME notation, remove global install - add restrictive permissions for config (0o700 dir, 0o600 file) - suppress API key echo in promptApiKey - use optsWithGlobals for nested command options - validate stream/tools/temperature in run-command - narrow biome packages/cli carve-out to cli.ts and gen/
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/cli/src/cli.ts (1)
55-73:⚠️ Potential issue | 🟠 Major
--bodystill breaks mixed path+body commands.This branch returns before the
meta.pathParamsloop, so any command that needs both a path param and a body dropsopts.pathentirely when--bodyis used.String(v)also flattens arrays/objects into"[object Object]", so structured JSON is corrupted before it reachespackages/cli/src/run-command.ts. Keep the parsed body as-is, validate that it is an object, and still merge the path params.💡 Direction for a fix
-function buildOpts( +function buildOpts( meta: (typeof operationMeta)[keyof typeof operationMeta], cmdOpts: Record<string, unknown>, -): Record<string, string | undefined> { - const opts: Record<string, string | undefined> = {} +): Record<string, unknown> { + const opts: Record<string, unknown> = {} const rawBody = cmdOpts.body as string | undefined - if (rawBody) + if (rawBody) try { - const parsed = JSON.parse(rawBody) as Record<string, unknown> - for (const [k, v] of Object.entries(parsed)) if (v !== undefined) opts[k] = String(v) - return opts + const parsed = JSON.parse(rawBody) as Record<string, unknown> + if (parsed === null || Array.isArray(parsed) || typeof parsed !== 'object') { + console.error('Invalid --body JSON: expected an object') + process.exit(1) + } + Object.assign(opts, parsed) } catch { console.error('Invalid --body JSON') process.exit(1) } for (const p of meta.pathParams) opts[p.name] = String(cmdOpts[p.name] ?? '') - for (const p of meta.bodyParams) opts[p.name] = String(cmdOpts[p.name] ?? '') + if (!rawBody) for (const p of meta.bodyParams) opts[p.name] = String(cmdOpts[p.name] ?? '') return opts }You’ll also need to widen the
optstype inpackages/cli/src/run-command.tsso raw JSON values can flow through unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli.ts` around lines 55 - 73, The buildOpts function returns early when --body is present and JSON-parses it, which drops meta.pathParams and coerces structured values to strings; change buildOpts so that after JSON.parse it validates parsed is a plain object, merges its entries into opts without String(...) coercion (preserving arrays/objects), and then continues to set meta.pathParams and meta.bodyParams defaults as needed; update the consumer type in run-command.ts to accept non-string raw JSON values so the uncoerced values flow through unchanged.packages/cli/src/config.ts (1)
41-48:⚠️ Potential issue | 🟠 MajorTighten permissions on existing config paths too.
modehere only applies when the directory/file is created. Ifbasilic/orconfig.jsonalready exists with broader permissions, this keeps the old mode and the storedapiKeystays readable to other local users. Follow the write withchmodSync(dir, 0o700)andchmodSync(path, 0o600). As per coding guidelines, "SECURITY: Verify no secrets exposed in code (API keys, tokens, passwords)".🔒 Proposed hardening
-import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs' @@ const path = getConfigPath() const dir = join(path, '..') if (!existsSync(dir)) mkdirSync(dir, { recursive: true, mode: 0o700 }) + chmodSync(dir, 0o700) @@ } writeFileSync(path, JSON.stringify(next, null, 2), { encoding: 'utf-8', mode: 0o600 }) + chmodSync(path, 0o600) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/config.ts` around lines 41 - 48, The config write only sets modes on creation; ensure existing paths are hardened by calling chmodSync(dir, 0o700) after ensuring the directory exists (regardless of whether mkdirSync created it) and chmodSync(path, 0o600) immediately after writeFileSync; update the function that builds and writes the Config (references: dir, path, loadConfig, writeFileSync, mkdirSync) to always apply these chmodSync calls so existing basilic/ and config.json get tightened too.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/cli.ts`:
- Around line 12-24: The prompt is being suppressed because mutedStdout.muted is
set before rl.question, so the prompt text doesn't appear; fix by emitting the
prompt to the real stdout before enabling mute or by special-casing the prompt
in the custom Writable: either call process.stdout.write('API key: ') (or
rl.output.write) before setting mutedStdout.muted = true and then call
rl.question with the muted output, or change mutedStdout.write to detect the
prompt string (from rl.question) and forward it to process.stdout even when
muted; update references in this block (mutedStdout, createInterface,
rl.question) accordingly and leave mutedStdout.muted toggled back to false after
rl.close().
- Line 181: The CLI currently calls program.parse(), which does not await
promise-returning .action handlers (e.g., the async action callbacks registered
on the Command instance), so switch the entrypoint to await program.parseAsync()
instead of program.parse() to ensure async handlers are awaited and errors are
handled correctly; locate the call to program.parse() and replace it with an
awaited call to program.parseAsync() (or return the Promise from main) so
Commander will wait for the async .action(...) callbacks to complete.
---
Duplicate comments:
In `@packages/cli/src/cli.ts`:
- Around line 55-73: The buildOpts function returns early when --body is present
and JSON-parses it, which drops meta.pathParams and coerces structured values to
strings; change buildOpts so that after JSON.parse it validates parsed is a
plain object, merges its entries into opts without String(...) coercion
(preserving arrays/objects), and then continues to set meta.pathParams and
meta.bodyParams defaults as needed; update the consumer type in run-command.ts
to accept non-string raw JSON values so the uncoerced values flow through
unchanged.
In `@packages/cli/src/config.ts`:
- Around line 41-48: The config write only sets modes on creation; ensure
existing paths are hardened by calling chmodSync(dir, 0o700) after ensuring the
directory exists (regardless of whether mkdirSync created it) and
chmodSync(path, 0o600) immediately after writeFileSync; update the function that
builds and writes the Config (references: dir, path, loadConfig, writeFileSync,
mkdirSync) to always apply these chmodSync calls so existing basilic/ and
config.json get tightened too.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 545d5773-97a7-4a12-8578-409af9420ae6
📒 Files selected for processing (6)
apps/docu/content/docs/development/cli.mdxbiome.jsonpackages/cli/README.mdpackages/cli/src/cli.tspackages/cli/src/config.tspackages/cli/src/run-command.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- biome.json
- apps/docu/content/docs/development/cli.mdx
Comment on lines
+12
to
+24
| const mutedStdout = new Writable({ | ||
| write(chunk, _encoding, callback) { | ||
| if (!(mutedStdout as Writable & { muted?: boolean }).muted) process.stdout.write(chunk) | ||
| callback() | ||
| }, | ||
| }) as Writable & { muted?: boolean } | ||
| mutedStdout.muted = true | ||
| const rl = createInterface({ input: process.stdin, output: mutedStdout, terminal: true }) | ||
| return new Promise((resolve, reject) => { | ||
| rl.question('API key: ', answer => { | ||
| rl.close() | ||
| mutedStdout.muted = false | ||
| resolve(answer.trim()) |
There was a problem hiding this comment.
The API key prompt is currently invisible.
mutedStdout.muted is enabled before rl.question(...), so the prompt string is suppressed together with the typed characters. Users end up typing blind. Print the label before muting, or special-case the prompt text in the custom Writable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/cli.ts` around lines 12 - 24, The prompt is being suppressed
because mutedStdout.muted is set before rl.question, so the prompt text doesn't
appear; fix by emitting the prompt to the real stdout before enabling mute or by
special-casing the prompt in the custom Writable: either call
process.stdout.write('API key: ') (or rl.output.write) before setting
mutedStdout.muted = true and then call rl.question with the muted output, or
change mutedStdout.write to detect the prompt string (from rl.question) and
forward it to process.stdout even when muted; update references in this block
(mutedStdout, createInterface, rl.question) accordingly and leave
mutedStdout.muted toggled back to false after rl.close().
| } | ||
| } | ||
|
|
||
| program.parse() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/cli/src/cli.ts | head -200Repository: blockmatic/basilic
Length of output: 7340
🏁 Script executed:
rg "\.action\(" packages/cli/src/cli.ts -A 5 -B 2Repository: blockmatic/basilic
Length of output: 1091
🌐 Web query:
Commander.js parseAsync async action handlers promises
💡 Result:
In Commander.js, if any of your command .action(...) handlers (or lifecycle hooks like preAction/postAction) are async / return a Promise, you should use parseAsync and await it, so Commander waits for those Promises to settle before your CLI process continues/exits. [1]
import { Command } from 'commander';
const program = new Command();
program
.command('run')
.action(async () => {
await doWork(); // can await inside
});
await program.parseAsync(process.argv); // waits for async action/hook(s)parseAsync(...) itself is async and returns a Promise that resolves to the command (this), so await program.parseAsync(...) is the intended usage. [2]
Key point: program.parse(...) does not await async actions; program.parseAsync(...) does. [1]
Sources: [1] Commander README (async action handlers + parseAsync guidance), [2] Commander Command.parseAsync JSDoc/implementation.
Use await program.parseAsync() instead of program.parse().
This file registers async .action(...) callbacks (lines 92, 141), but the entrypoint uses program.parse(). Commander only awaits promise-returning handlers when you use parseAsync(), so rejected async work can escape the intended CLI error flow and the process may exit before operations complete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/cli.ts` at line 181, The CLI currently calls
program.parse(), which does not await promise-returning .action handlers (e.g.,
the async action callbacks registered on the Command instance), so switch the
entrypoint to await program.parseAsync() instead of program.parse() to ensure
async handlers are awaited and errors are handled correctly; locate the call to
program.parse() and replace it with an awaited call to program.parseAsync() (or
return the Promise from main) so Commander will wait for the async .action(...)
callbacks to complete.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit