feat(cli): agent-friendly CLI, docs, and Docus skill#751
Merged
Conversation
Introduce global CLI context (--json, --quiet, --yes, --non-interactive) with structured errors, non-interactive guards on all commands, and refreshed shelve.cloud documentation plus a publishable agent skill at /.well-known/skills/.
|
Deployment failed with the following error: |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR implements Shelve CLI v5 with agent-friendly infrastructure: adds global flags (--json, --quiet, --yes, --non-interactive) for structured/quiet/non-interactive operation, introduces CliError exception class with standardized error codes and JSON output helpers, refactors all commands to use new output system, introduces doctor validation command, enforces non-interactive guards blocking prompts in agent shells, and provides comprehensive documentation and published agent skills for CI/automation workflows. ChangesAgent-Friendly CLI Modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
Thank you for following the naming conventions! 🙏 |
Extend agent automation with shelve doctor, structured error codes, run spawn JSON events, GitHub Action, playground CI, troubleshooting docs, and shelve-app skill — without local Cursor skills or global --token flags.
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 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/actions/shelve-run/action.yml:
- Around line 24-27: The action currently defaults the inputs.cli-version to
"latest", making CI non-deterministic; update the default value in action.yml
for the input key cli-version from 'latest' to a pinned semver (e.g., '1.0.0' or
a major-only pin like '1') while keeping the input override, and ensure the
runtime invocation that uses npx --yes "`@shelve/cli`@${{ inputs.cli-version }}"
continues to reference inputs.cli-version so callers can still override the
version when needed.
In @.github/workflows/test.yml:
- Around line 41-49: Replace mutable tag refs with immutable commit SHAs for all
third-party actions in the workflow: change occurrences of
"actions/checkout@v6", "pnpm/action-setup@v6", and "actions/setup-node@v4" to
their corresponding full commit SHA pins; and for each "actions/checkout" step,
add with: persist-credentials: false so the checkout does not persist repository
credentials to the runner. Locate the action usages by the action identifiers
(actions/checkout, pnpm/action-setup, actions/setup-node) and update the ref
strings to SHAs and add the persist-credentials key to the checkout steps.
In `@apps/lp/content/docs/3.cli/10.agents-automation.md`:
- Around line 25-29: The ordered list restarts at "2. Run **`shelve init`**"
which breaks markdownlint sequence; locate the list item that begins with "2.
Run **`shelve init`**" and change its leading numeral to continue the previous
sequence (i.e., set it to the previous list's last number + 1) so the sequence
flows correctly through "Run **`shelve init`**", "Inject secrets with **`shelve
run -- pnpm dev`**", "Use **`--json`**…" and "Use **`--non-interactive`**…".
In `@apps/lp/content/docs/3.cli/3.run.md`:
- Line 6: The doc overstates security by claiming secrets "never touch disk";
update the sentence about `shelve run` so it accurately states that `shelve run`
does not write a plaintext `.env` file (e.g., change the phrase "No `.env` file
is written — secrets never touch disk." to something like "No plaintext `.env`
file is written by `shelve run` — encrypted cache files (e.g., ~/.shelve/cache/)
may still be created."), keeping reference to `shelve run` and the encrypted
cache location to avoid misleading readers.
In `@apps/lp/content/docs/3.cli/8.generate.md`:
- Line 43: The sentence describing optional installs is backwards: update the
sentence that currently reads "Optionally installs `eslint` and
`@hrcd/eslint-config` when run interactively (skipped with `--yes` /
non-interactive)" to clarify that `--yes` auto-accepts the optional installs
while non-interactive mode skips them; specifically edit the line containing
"Optionally installs `eslint` and `@hrcd/eslint-config`" to say something like
installs occur when interactive or when `--yes` is passed (which auto-confirms),
and that only non-interactive runs will skip the installs.
In `@apps/lp/skills/shelve/agent-workflows.md`:
- Line 15: Replace the typo in the user guidance sentence that currently reads
"a on-disk `.env` file" with the grammatically correct phrase "an on-disk `.env`
file" in the docs (look for the exact text "a on-disk `.env` file" in
agent-workflows.md and update it to "an on-disk `.env` file").
In `@docs/agents/cli.md`:
- Around line 24-27: The ordered list restarts at "2." causing markdownlint
MD029; renumber the sequence so it starts at 1 and increments (change "2. Run
**`shelve init`**" to "1. Run **`shelve init`**" and adjust the subsequent items
"3. Prefer **`shelve run -- <cmd>`**", "4. Use **`--json`**", "5. Use
**`--non-interactive`**" to "2.", "3.", "4." respectively so the items are a
continuous ordered list.
In `@packages/cli/README.md`:
- Around line 59-74: Add the missing "doctor" command to the CLI docs: update
the USAGE line to include "doctor" (e.g., "USAGE shelve
run|push|pull|login|logout|me|init|create|config|generate|upgrade|doctor") and
insert a new command entry under COMMANDS to match the existing style (e.g., "
doctor Run diagnostic checks for the Shelve CLI and environment"), ensuring
alignment and formatting consistent with the other command descriptions.
In `@packages/cli/src/commands/config.ts`:
- Line 16: The current console.log(config) call prints Node's default
inspection; replace it with a pretty JSON serialization for human-readable
output by calling JSON.stringify(config, null, 2) (use the same conditional that
checks the --json flag if present) so the object returned by redactConfig(...)
is displayed consistently and legibly; update the output in the command handler
where console.log(config) is used.
In `@packages/cli/src/commands/doctor.ts`:
- Around line 60-69: Early returns call finishDoctor(checks) before
pushAgentChecks(checks), causing agent/CI diagnostics to be skipped; move or add
calls to pushAgentChecks(checks) so they always run prior to any
finishDoctor(checks) return. Specifically, ensure that after
pushConfigChecks(config, checks) you invoke pushAgentChecks(checks)
unconditionally (and likewise before the other early-return branches that
currently call finishDoctor(checks)), so pushAgentChecks is executed regardless
of config.token presence or other failing conditions before calling
finishDoctor(checks).
- Around line 256-271: The JSON branch currently always calls cliSuccess(data,
...) even when healthy is false; change it so that when isJson() is true and
healthy is false you do not emit a success-shaped payload: either call the
corresponding failure emitter (cliError or similar) with the same data/context
(e.g., cliError(data, undefined, 'doctor')) or explicitly set data.ok = false
(or equivalent failure flag) before emitting, so machines receive a failing JSON
result when !healthy; update the isJson() branch around the cliSuccess(...) call
to handle both healthy and !healthy cases accordingly.
In `@packages/cli/src/commands/generate.ts`:
- Around line 80-81: The default branch in the switch (calling
cliCancel('Invalid option')) is dead code because the variable mapped is
validated earlier (lines validating mapped before runGenerate). Remove the
unreachable default case from the switch in generate.ts (the switch that
dispatches on mapped before calling runGenerate) so only the concrete cases
remain; ensure no other code relies on that default path and run tests/TS
compile to confirm no fallthrough errors.
In `@packages/cli/src/commands/me.ts`:
- Around line 13-20: The current me command assumes both config.username and
config.email exist when logging in; change the logic so that when either field
is present it still sets loggedIn: true, and build the success message from only
the present fields (e.g., create a displayParts array that adds config.username
and `<${config.email}>` only if they exist, then join with a space or
separator). Update the cliSuccess call (the payload passed to cliSuccess and its
message) to use these conditional values instead of interpolating potentially
undefined config.username/config.email so the printed message never contains
"undefined".
In `@packages/cli/src/commands/push.ts`:
- Around line 33-35: The current logic in push.ts treats a non-interactive
environment (shouldSkipConfirm()) as implicit approval by setting skipConfirm
and computing effectiveConfirmChanges; instead, change the behavior so that if
args.yes is not set and shouldSkipConfirm() returns true, the command throws a
structured error (e.g., "cannot prompt for confirmation in non-interactive mode;
pass --yes to confirm") rather than assuming consent; locate and update the code
that computes skipConfirm/effectiveConfirmChanges (references: args.yes,
shouldSkipConfirm, skipConfirm, effectiveConfirmChanges, confirmChanges) to
perform this check and throw the error before proceeding with any mutating
action.
In `@packages/cli/src/commands/upgrade.ts`:
- Around line 21-22: The JSON payload currently sets current: 'latest', which is
misleading; change the call to use the actual installed version returned by
installLatest() (e.g., let installed = await installLatest() and pass current:
installed) or, if installLatest() cannot return a version yet, remove the
current field from the cliSuccess call; update the code around installLatest()
and the cliSuccess invocation so the structured output contains a real resolved
version (or omits current) instead of the literal 'latest'.
In `@packages/cli/src/utils/cli-context.ts`:
- Around line 87-97: Both helpers mistakenly continue processing after an option
terminator ("--"); update getCommandFromArgv and stripGlobalFlags so they stop
parsing when encountering "--". In getCommandFromArgv (function name) iterate
argv.slice(2) and if you see arg === "--" then break and return undefined (or
leave the remainder alone) so subsequent args aren't treated as global flags or
commands; in stripGlobalFlags (function name) scan arguments and when you
encounter "--" stop filtering and append the rest of the argv slice (including
the "--" and following items) unchanged to the result so child process flags are
preserved.
In `@packages/cli/src/utils/output.ts`:
- Around line 135-141: The spinner error is being rendered twice on
unrecoverable failures because s.cancel(formatCliError(error, message)) prints
the error and then handleThrownError(error, message) prints/exits again;
instead, on the unrecoverable path remove the direct spinner render and only
call handleThrownError so the spinner is stopped/cancelled without duplicating
output—e.g., call s.cancel() or s.stop() to clear the spinner and then invoke
handleThrownError(error, message), preserving the existing recoverable branch
that calls s.stop(message) and rethrows.
- Around line 46-50: The JSON path in cliError currently concatenates hint into
message before calling writeJsonError, causing duplicate human text; change
cliError so when isJson() is true it calls writeJsonError with the original
input (or with separate message and hint fields) instead of the combined
message, while preserving the existing concatenated message only for the
non-JSON (human) branch; update the code in cliError to use the computed
combined message for the non-JSON branch but pass input.message and input.hint
(or input unchanged) to writeJsonError so JSON consumers get separate fields.
In `@packages/cli/src/utils/templates.ts`:
- Around line 33-38: The logic that computes shouldInstall is inverted:
shouldSkipConfirm() means “auto-confirm”, so when it returns true we should set
shouldInstall to true instead of false; update the expression that defines
shouldInstall (referenced by the variable name shouldInstall and functions
shouldSkipConfirm(), isNonInteractive(), askBoolean()) so that
shouldSkipConfirm() yields true (auto-accept), keep isNonInteractive()
short-circuiting to false, and otherwise call await askBoolean('Do you want to
install ESLint and `@hrcd/eslint-config`?') to prompt the user.
- Around line 23-24: The code fetches templates.eslint into response and
immediately reads text and writes it out; add a check for response.ok after the
fetch (before calling response.text()) and handle non-2xx responses by throwing
or returning an error so you don't write an HTML error page into
eslint.config.js. Specifically, after the fetch(templates.eslint) check
response.ok and if false, throw an Error including
response.status/response.statusText (or log and abort), otherwise await
response.text() and proceed with writing; update the code paths that currently
use response/text to rely on this check so generation fails cleanly on 404/500.
In `@packages/cli/test/cli-context.test.ts`:
- Around line 15-18: The afterEach teardown in the test sets
process.env.AI_AGENT and process.env.CI directly from
ORIGINAL_AI_AGENT/ORIGINAL_CI which can assign the literal string "undefined"
when those originals were not set; change the restore logic in the afterEach
(around the afterEach block that calls initCliContextFromArgv) to check each
saved value (ORIGINAL_AI_AGENT and ORIGINAL_CI) and if it is undefined use
delete process.env.AI_AGENT / delete process.env.CI, otherwise reassign the
saved string to process.env.AI_AGENT / process.env.CI so unset envs are truly
removed.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 68765c5d-4566-43c3-ad45-68d81d03dc31
📒 Files selected for processing (56)
.changeset/cli-agent-friendly.md.github/actions/shelve-run/README.md.github/actions/shelve-run/action.yml.github/workflows/test.ymlAGENTS.mdapps/lp/content/docs/3.cli/1.init.mdapps/lp/content/docs/3.cli/10.agents-automation.mdapps/lp/content/docs/3.cli/11.troubleshooting.mdapps/lp/content/docs/3.cli/2.index.mdapps/lp/content/docs/3.cli/3.run.mdapps/lp/content/docs/3.cli/4.login-logout.mdapps/lp/content/docs/3.cli/5.push-pull.mdapps/lp/content/docs/3.cli/6.create.mdapps/lp/content/docs/3.cli/7.config.mdapps/lp/content/docs/3.cli/8.generate.mdapps/lp/content/docs/3.cli/9.upgrade.mdapps/lp/nuxt.config.tsapps/lp/skills/shelve-app/SKILL.mdapps/lp/skills/shelve/SKILL.mdapps/lp/skills/shelve/agent-workflows.mdapps/lp/skills/shelve/cli-commands.mddocs/agents/cli.mddocs/agents/docs-links.mdpackage.jsonpackages/cli/README.mdpackages/cli/src/commands/config.tspackages/cli/src/commands/create.tspackages/cli/src/commands/doctor.tspackages/cli/src/commands/generate.tspackages/cli/src/commands/init.tspackages/cli/src/commands/login.tspackages/cli/src/commands/logout.tspackages/cli/src/commands/me.tspackages/cli/src/commands/pull.tspackages/cli/src/commands/push.tspackages/cli/src/commands/run.tspackages/cli/src/commands/upgrade.tspackages/cli/src/constants.tspackages/cli/src/index.tspackages/cli/src/services/api-error.tspackages/cli/src/services/base.tspackages/cli/src/services/env.tspackages/cli/src/services/environment.tspackages/cli/src/services/project.tspackages/cli/src/utils/cli-context.tspackages/cli/src/utils/config.tspackages/cli/src/utils/error-codes.tspackages/cli/src/utils/index.tspackages/cli/src/utils/output.tspackages/cli/src/utils/prompt.tspackages/cli/src/utils/templates.tspackages/cli/test/cli-context.test.tspackages/cli/test/error-codes.test.tspackages/cli/test/non-interactive.test.tspackages/cli/test/output.test.tsplayground/run/README.md
Pin CI action SHAs, harden doctor/push/json output, fix flag parsing after `--`, and correct docs for run security, generate installs, and list numbering.
The playground fail script intentionally exits with 7 to verify propagation through shelve run, not 1.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/cli/src/utils/cli-context.ts (1)
48-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop reading global flags after the
--separator during context init.
hasFlag()scans the full argv, soinitCliContextFromArgv()still treats child-process args as Shelve globals. For example,shelve run -- echo --jsonwill incorrectly flip JSON mode on Line 53 even though--jsonbelongs to the spawned command. Slice argv at the first--before computingjsonMode/quietMode/yesMode/nonInteractiveFlag.Proposed fix
export function initCliContextFromArgv(argv: string[] = process.argv): void { - jsonMode = hasFlag(argv, '--json') - quietMode = hasFlag(argv, '--quiet', '-q') - yesMode = hasFlag(argv, '--yes', '-y') - nonInteractiveFlag = hasFlag(argv, '--non-interactive') + const separatorIndex = argv.indexOf('--') + const cliArgv = separatorIndex === -1 ? argv : argv.slice(0, separatorIndex) + + jsonMode = hasFlag(cliArgv, '--json') + quietMode = hasFlag(cliArgv, '--quiet', '-q') + yesMode = hasFlag(cliArgv, '--yes', '-y') + nonInteractiveFlag = hasFlag(cliArgv, '--non-interactive') if (process.env.CI === 'true' || process.env.CI === '1') { nonInteractiveFlag = true } }🤖 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/utils/cli-context.ts` around lines 48 - 56, The CLI context initialization currently scans the entire argv (via hasFlag) which incorrectly picks up flags after a `--` separator; update initCliContextFromArgv to first locate the first `--` in the provided argv and slice argv to argv.slice(0, indexOfDoubleDash) (or use the full argv if no `--`), then call hasFlag against that sliced array when setting jsonMode, quietMode, yesMode and nonInteractiveFlag so flags intended for spawned commands are ignored; reference initCliContextFromArgv and hasFlag to make the change.packages/cli/src/utils/index.ts (1)
38-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlways clean up the spinner on failure paths.
Both flows start the shared spinner and then await network/package-manager calls without a
finally. If either call throws, the spinner is never stopped and the terminal can be left in a broken-looking state. Wrap the start/stop pair intry/finallyso cleanup happens on both success and error paths.🤖 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/utils/index.ts` around lines 38 - 59, Both isLatestVersion and installLatest start the shared spinner before awaiting network/package calls but don't stop it on exceptions; wrap the spinner lifecycle in a try/finally in each function so s.stop(...) always runs even if fetchLatestCliVersion() or addDependency(...) throws. Specifically, in isLatestVersion and installLatest, call s.start(...) as you currently do, then perform the awaited work inside a try block and call s.stop('...') in the finally block (guarding the stop with the same !isQuiet() && !isJson() check), ensuring the spinner is cleaned up on both success and error paths.packages/cli/src/utils/templates.ts (1)
22-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t report every failure here as a fetch error.
This
catchnow covers the download, file write, and bothaddDevDependency()calls, but it always emits “Failed to fetch ESLint config”. If installation fails after Line 29 writes the file, users get the wrong diagnosis and lose the partial-success context. Narrow thetry/catchto the fetch step or surface the original error message instead of replacing it unconditionally.Proposed fix
- } catch { - cliCancel('Failed to fetch ESLint config') + } catch (error) { + cliCancel(error instanceof Error ? error.message : 'Failed to generate ESLint config') }🤖 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/utils/templates.ts` around lines 22 - 52, The catch currently hides the real failure cause by always calling cliCancel('Failed to fetch ESLint config') for errors from fetch, FileService.write, askBoolean, or addDevDependency; either narrow the try/catch to only surround the fetch/response.text() (the fetch of templates.eslint and response.ok check) and let later steps throw normally, or change the catch to capture the real error (catch (err)) and call cliCancel with a message that includes err.message/context (e.g., mention whether the failure happened during fetch, file write via FileService.write, or dependency installation via addDevDependency) so callers can see partial successes and accurate failure reasons.
♻️ Duplicate comments (3)
packages/cli/src/commands/push.ts (1)
35-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not let
shouldSkipConfirm()become implicit consent forpush.
confirmed = args.yes || shouldSkipConfirm()still auto-approves pushes in agent/CI shells, so the guard on Line 36 never fires for the case it is meant to block. Keep auto-skip separate from explicit--yes, throw before mutating, and only disable confirmation when the caller actually passed--yes.Suggested fix
- const confirmed = args.yes || shouldSkipConfirm() - if (confirmChanges && !confirmed && isNonInteractive()) { + const autoSkipConfirm = shouldSkipConfirm() + if (confirmChanges && !args.yes && autoSkipConfirm && isNonInteractive()) { throw new CliError( 'Push confirmation is required.', 'CONFIRMATION_REQUIRED', undefined, 'Pass --yes to confirm pushing variables in non-interactive mode.', ) } - const effectiveConfirmChanges = confirmed ? false : confirmChanges + const effectiveConfirmChanges = args.yes ? false : confirmChanges🤖 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/push.ts` around lines 35 - 44, The current code treats shouldSkipConfirm() as explicit consent by setting confirmed = args.yes || shouldSkipConfirm(), which bypasses the non-interactive guard; change this so confirmed is only based on args.yes (e.g., const confirmed = !!args.yes), call shouldSkipConfirm() into a separate variable (e.g., const autoSkip = shouldSkipConfirm()) if you still need it for prompt-skipping, perform the throw for non-interactive when confirmChanges && !confirmed && isNonInteractive(), and compute effectiveConfirmChanges solely from confirmed (const effectiveConfirmChanges = confirmed ? false : confirmChanges) so autoSkip does not implicitly disable confirmation.apps/lp/content/docs/3.cli/10.agents-automation.md (1)
25-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis list still fails MD029 because the code fence above breaks the sequence.
Markdownlint expects these items to restart at
1.unless the fenced block under item 2 is indented as part of the same list item. Either indent the code block above or renumber this block as a new list.🤖 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 `@apps/lp/content/docs/3.cli/10.agents-automation.md` around lines 25 - 28, The markdown list triggers MD029 because the fenced code block under item 2 isn't indented as part of that list item; to fix, either indent the fenced block so it is nested under item 2 (so items 3-6 continue the same numbered list) or treat items 3-6 as a new list by renumbering them to start at 1; locate the list containing "Run `shelve init`", the preceding code fence under item 2, and the flags `--json` and `--non-interactive` and apply one of the two fixes to resolve the lint error.packages/cli/src/commands/doctor.ts (1)
257-262:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the standard JSON error envelope on failing doctor runs.
The unhealthy branch now emits
{ ok: false, command, data }, but every other JSON failure path is documented and implemented as anerrorobject with stablecode/messagefields. That makesdoctor --jsona special case that automation has to parse differently on failure.Suggested fix
if (isJson()) { if (healthy) { cliSuccess(data, undefined, 'doctor') } else { - console.error(JSON.stringify({ ok: false, command: 'doctor', data })) + console.error(JSON.stringify({ + ok: false, + command: 'doctor', + error: { + code: 'DOCTOR_FAILED', + message: 'Doctor checks failed', + }, + data, + })) } } else {🤖 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/doctor.ts` around lines 257 - 262, The failing JSON branch in the doctor command currently prints JSON.stringify({ ok: false, command: 'doctor', data }) which breaks the standard error envelope used elsewhere; change that branch to emit the documented error envelope with an error object containing stable code and message fields (and include the original data as nested details if needed), e.g. produce JSON.stringify({ ok: false, command: 'doctor', error: { code: '<SOME_CODE>', message: '<human-readable message>', details: data }}) instead of the current shape; update the unhealthy path in the isJson() block (where cliSuccess is used for healthy) to construct and console.error this standard error object so automated consumers receive a consistent {error: {code, message}} envelope.
🤖 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.
Outside diff comments:
In `@packages/cli/src/utils/cli-context.ts`:
- Around line 48-56: The CLI context initialization currently scans the entire
argv (via hasFlag) which incorrectly picks up flags after a `--` separator;
update initCliContextFromArgv to first locate the first `--` in the provided
argv and slice argv to argv.slice(0, indexOfDoubleDash) (or use the full argv if
no `--`), then call hasFlag against that sliced array when setting jsonMode,
quietMode, yesMode and nonInteractiveFlag so flags intended for spawned commands
are ignored; reference initCliContextFromArgv and hasFlag to make the change.
In `@packages/cli/src/utils/index.ts`:
- Around line 38-59: Both isLatestVersion and installLatest start the shared
spinner before awaiting network/package calls but don't stop it on exceptions;
wrap the spinner lifecycle in a try/finally in each function so s.stop(...)
always runs even if fetchLatestCliVersion() or addDependency(...) throws.
Specifically, in isLatestVersion and installLatest, call s.start(...) as you
currently do, then perform the awaited work inside a try block and call
s.stop('...') in the finally block (guarding the stop with the same !isQuiet()
&& !isJson() check), ensuring the spinner is cleaned up on both success and
error paths.
In `@packages/cli/src/utils/templates.ts`:
- Around line 22-52: The catch currently hides the real failure cause by always
calling cliCancel('Failed to fetch ESLint config') for errors from fetch,
FileService.write, askBoolean, or addDevDependency; either narrow the try/catch
to only surround the fetch/response.text() (the fetch of templates.eslint and
response.ok check) and let later steps throw normally, or change the catch to
capture the real error (catch (err)) and call cliCancel with a message that
includes err.message/context (e.g., mention whether the failure happened during
fetch, file write via FileService.write, or dependency installation via
addDevDependency) so callers can see partial successes and accurate failure
reasons.
---
Duplicate comments:
In `@apps/lp/content/docs/3.cli/10.agents-automation.md`:
- Around line 25-28: The markdown list triggers MD029 because the fenced code
block under item 2 isn't indented as part of that list item; to fix, either
indent the fenced block so it is nested under item 2 (so items 3-6 continue the
same numbered list) or treat items 3-6 as a new list by renumbering them to
start at 1; locate the list containing "Run `shelve init`", the preceding code
fence under item 2, and the flags `--json` and `--non-interactive` and apply one
of the two fixes to resolve the lint error.
In `@packages/cli/src/commands/doctor.ts`:
- Around line 257-262: The failing JSON branch in the doctor command currently
prints JSON.stringify({ ok: false, command: 'doctor', data }) which breaks the
standard error envelope used elsewhere; change that branch to emit the
documented error envelope with an error object containing stable code and
message fields (and include the original data as nested details if needed), e.g.
produce JSON.stringify({ ok: false, command: 'doctor', error: { code:
'<SOME_CODE>', message: '<human-readable message>', details: data }}) instead of
the current shape; update the unhealthy path in the isJson() block (where
cliSuccess is used for healthy) to construct and console.error this standard
error object so automated consumers receive a consistent {error: {code,
message}} envelope.
In `@packages/cli/src/commands/push.ts`:
- Around line 35-44: The current code treats shouldSkipConfirm() as explicit
consent by setting confirmed = args.yes || shouldSkipConfirm(), which bypasses
the non-interactive guard; change this so confirmed is only based on args.yes
(e.g., const confirmed = !!args.yes), call shouldSkipConfirm() into a separate
variable (e.g., const autoSkip = shouldSkipConfirm()) if you still need it for
prompt-skipping, perform the throw for non-interactive when confirmChanges &&
!confirmed && isNonInteractive(), and compute effectiveConfirmChanges solely
from confirmed (const effectiveConfirmChanges = confirmed ? false :
confirmChanges) so autoSkip does not implicitly disable confirmation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ae150301-220d-4457-a9df-8f98ebaa68be
📒 Files selected for processing (20)
.github/actions/shelve-run/README.md.github/actions/shelve-run/action.yml.github/workflows/test.ymlapps/lp/content/docs/3.cli/10.agents-automation.mdapps/lp/content/docs/3.cli/3.run.mdapps/lp/content/docs/3.cli/8.generate.mdapps/lp/skills/shelve/agent-workflows.mddocs/agents/cli.mdpackages/cli/README.mdpackages/cli/src/commands/config.tspackages/cli/src/commands/doctor.tspackages/cli/src/commands/generate.tspackages/cli/src/commands/me.tspackages/cli/src/commands/push.tspackages/cli/src/commands/upgrade.tspackages/cli/src/utils/cli-context.tspackages/cli/src/utils/index.tspackages/cli/src/utils/output.tspackages/cli/src/utils/templates.tspackages/cli/test/cli-context.test.ts
Ignore post-`--` flags in context init, harden spinner cleanup, narrow ESLint fetch errors, standardize doctor JSON errors, and require push --yes for non-interactive confirmation.
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
--json,--quiet,--yes,--non-interactive,--debug) with auto-detection in CI/agent shells, structured JSON errors on stderr, and non-interactive guards across all commands.login --token,generate --type,push --yes, and fix silent fetch failures inshelve run./.well-known/skills/(npx skills add https://shelve.cloud).Test plan
pnpm --filter @shelve/cli test(61 tests)pnpm play -- --json config— JSON output with redacted tokenpnpm play—shelve runend-to-end against fake APIcurl https://shelve.cloud/.well-known/skills/index.jsonlistsshelveskillshelve pullwithout--yesreturnsAGENT_BLOCKEDSHELVE_TOKEN=… shelve --non-interactive run -- echo okSummary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests