Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughRestructures native binary distribution to platform-specific optional npm packages, removes the postinstall installer, updates the CLI entry resolver to load platform packages dynamically, adds platform packaging artifacts and matrix publish workflow steps, and introduces unit tests for CLI utilities and UI. Changes
Sequence Diagram(s)sequenceDiagram
participant WF as GitHub Workflow (publish-platform-packages)
participant Art as Release Run Artifacts
participant Repo as Checkout (git/release_sha)
participant Node as Node/npm environment
participant NPM as npm Registry
WF->>Art: request artifact (run-id = release_run_id, path per-platform)
Art-->>WF: return platform artifact
WF->>Repo: checkout head_sha or release_sha
WF->>Node: setup Node.js & npm
WF->>Node: verify package version not published
alt version not published
WF->>NPM: npm publish platform artifact
NPM-->>WF: publish success
else version exists
WF-->>WF: skip publish
end
WF->>Node: after matrix, run publish-main job (tag/version validation, dry-run)
Node->>NPM: npm publish main package
NPM-->>Node: publish result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/package.json (1)
14-16:⚠️ Potential issue | 🟡 MinorThe release workflow handles this correctly, but the npm script is wrong.
The CI/CD pipeline works fine—the workflow properly outputs to
cloudops-tools-macos-arm64and correctly renames it tocloudops-toolswhen copying to the npm package. The darwin-arm64 package will ship with the right binary name.However, the
build:macOS-armnpm script incli/package.json(line 15) is misleading. It says:--outfile dist/cloudops-tools.appBut it should be:
--outfile dist/cloudops-toolsThe
.appextension is wrong—binaries on macOS don't use that extension (that's for application bundles). If someone runsbun run build:macOS-armlocally, they'll get a file namedcloudops-tools.appinstead ofcloudops-tools, which breaks the consistency with the other platforms and confuses the naming scheme. Fix the npm script to match what the workflow does and what the npm package expects.
🤖 Fix all issues with AI agents
In @.github/workflows/cli-publish.yml:
- Around line 16-22: The job "publish-platform-packages" currently has a
condition using github.event.workflow_run.conclusion which is undefined for
manual runs (workflow_dispatch) causing the job to be skipped; either remove the
workflow_dispatch trigger if manual runs aren't intended, or change the
conditional logic around the if: ${{ github.event.workflow_run.conclusion ==
'success' }} to allow workflow_dispatch (e.g., check event name or allow missing
github.event.workflow_run), and if you keep workflow_dispatch also update
downstream artifact/checkout steps that rely on github.event.workflow_run.id and
github.event.workflow_run.head_sha by adding workflow_dispatch inputs (run
ID/SHA) or an alternate artifact retrieval strategy so manual runs have the
required values.
In @.github/workflows/cli-release.yml:
- Around line 116-121: The release job is downloading and publishing every
artifact (including the npm-${{ matrix.npm_dir }} uploads) causing npm platform
files to appear in the GitHub Release; fix by narrowing what gets downloaded or
what gets published: either change the actions/download-artifact@v4 step in the
release job to use the pattern parameter (e.g. only download artifacts matching
your zip/raw binary names like cloudops-tools-*) or change the
softprops/action-gh-release input glob from artifacts/**/* to a more specific
pattern that excludes npm-* (for example only include artifacts/**/*.zip or
artifacts/**/cloudops-tools-*), updating the steps that reference
actions/download-artifact@v4 and softprops/action-gh-release accordingly.
In `@cli/test/unit/lib/progress.test.ts`:
- Around line 248-291: The test file duplicates the helper functions
parseErrorPayload and formatParsedError multiple times (including inside the
prettifyError block); remove the duplicate declarations and declare a single
shared copy of parseErrorPayload and formatParsedError once at the top of the
describe("progress") block, then have prettifyError and all tests call those
shared helpers instead of redeclaring them.
- Around line 6-319: Your tests define local copies of formatDuration, makeBar,
parseErrorPayload, formatParsedError, and prettifyError so they don't exercise
the real implementations; either export the real helper functions from
progress.ts (e.g., export them as __internal exports: formatDuration, makeBar,
parseErrorPayload, formatParsedError, prettifyError) and import those into
progress.test.ts, or delete the local stubs and instead validate their behavior
by driving the public API startProgressRenderer (call startProgressRenderer and
assert rendered output/side-effects) so the tests exercise the actual code
paths.
In `@cli/test/unit/lib/ui.test.ts`:
- Around line 131-143: The test is relying on a fresh import but the ui module's
colors (the colors constant in ui.ts initialized via
pc.createColors(isColorEnabled())) is cached at first import, so setting
NO_COLOR before a dynamic import in the same process won't change behavior;
update the test to run in isolation: spawn a subprocess (Node/Bun) that sets
process.env.NO_COLOR and then imports "../../../src/ui" and calls ui.info(...)
and returns/prints the result, then assert the subprocess output equals "test".
Alternatively, if you prefer an in-process change, refactor ui.ts to defer color
checks (e.g., call isColorEnabled() inside ui.info or expose a getUi(colors?)
factory) so colors is resolved per-call and update tests to use that API;
reference ui.info, ui.ts, isColorEnabled, and the module-level colors constant
when locating code to change.
🧹 Nitpick comments (6)
cli/test/unit/lib/options.test.ts (2)
63-84: This entire"option types"block is redundant with the"exports"block above.You literally test the exact same thing twice. The individual tests on lines 7–60 already assert every single one of these keys is
toBeDefined(). This block adds zero additional coverage. It's not testing "types" — it's testing the sametoBeDefined()with extra steps.Remove it. Dead test code is still dead code, and it still wastes everyone's time when they read it.
🗑️ Proposed removal
- describe("option types", () => { - test("all options are defined as Effect CLI Options", async () => { - const options = await import("../../../src/options"); - const optionKeys = [ - "account", - "region", - "initRegions", - "exportFormat", - "limitRegions", - "debugOption", - "describeOption", - "skipGlobalOption", - "onlyGlobalOption", - "modeOption", - "servicesOption", - ]; - - for (const key of optionKeys) { - expect(options[key as keyof typeof options]).toBeDefined(); - } - }); - });
6-60: Consider a data-driven approach instead of 11 copy-paste tests.Every single test here is structurally identical: import the module, check
toBeDefined(). That's fine as an intent, but you don't need to repeat the same boilerplate 11 times. A loop does the job with less noise and is trivially extensible when you add new options.♻️ Suggested consolidation
describe("exports", () => { - test("exports account option", async () => { - const options = await import("../../../src/options"); - expect(options.account).toBeDefined(); - }); - - test("exports region option", async () => { - const options = await import("../../../src/options"); - expect(options.region).toBeDefined(); - }); - - ... (repeat x11) - + const expectedExports = [ + "account", + "region", + "initRegions", + "exportFormat", + "limitRegions", + "debugOption", + "describeOption", + "skipGlobalOption", + "onlyGlobalOption", + "modeOption", + "servicesOption", + ] as const; + + test.each(expectedExports)("exports %s", async (key) => { + const options = await import("../../../src/options"); + expect(options[key as keyof typeof options]).toBeDefined(); + }); });cli/test/unit/lib/ui.test.ts (1)
50-58: Thedelete process.envcalls beforeawait import(...)are no-ops after first import.Lines 52-53 delete
FORCE_COLORandNO_COLORbefore the dynamic import, presumably to ensure a clean color state. But the module is already cached from the"exports"tests above. These deletions affect nothing — picocolors already decided whether to use colors when it was first loaded.This isn't harmful (the tests still pass because
toContainis lenient enough to match through ANSI codes), but it's misleading. Either remove the env manipulation or add a comment acknowledging it's vestigial.cli/bin/cloudops-tools.js (1)
26-37: Thecatchswallows the original error — consider logging it for debuggability.Look, the resolver pattern is correct,
createRequire+resolveis exactly how you do this in ESM. But when something goes wrong — and things will go wrong because this is the real world — you're throwing away the actual error. A user files a bug report saying "it told me to install optional dependencies" and you have nothing to debug with. Was it ENOENT? A permissions issue? A corrupted node_modules? You don't know, because you threw the error in the trash.At minimum, log the original error in a debug/verbose mode, or just tack it on:
Proposed improvement
-} catch { +} catch (err) { console.error( `cloudops-tools: could not find package "${platform.pkg}".\n` + - "Make sure optional dependencies are installed (do not use --no-optional).", + "Make sure optional dependencies are installed (do not use --no-optional).\n" + + `Details: ${err.message}`, ); process.exit(1); }.github/workflows/cli-publish.yml (2)
20-22:fail-fast: truemeans a single platform failure leaves you in a partially-published state.Think about this scenario:
darwin-arm64publishes successfully, thenlinux-x64fails.fail-fast: truecancelswin32-x64. Now you have one platform package on npm and two missing.publish-mainnever runs, so the main CLI package isn't updated — but the darwin-arm64 package IS out there at the new version with no corresponding main package.You probably want
fail-fast: falsehere so all platforms get their best shot at publishing, and then letpublish-mainonly proceed if all succeeded (whichneedsalready enforces).Suggested change
strategy: - fail-fast: true + fail-fast: false matrix:
39-46:node-version: "latest"in a publish workflow is playing with fire.I don't care if it's "just" a publish step. You do NOT use unpinned, rolling versions in CI that touches your release pipeline. One day Node 24 drops, something breaks in npm's provenance flow, and your release is dead until you figure out why.
Pin to an LTS version. You can update it deliberately and on your schedule, not Node.js's.
Suggested change
- name: Setup Node.js uses: actions/setup-node@v6 with: - node-version: "latest" + node-version: "22" registry-url: https://registry.npmjs.orgThis applies to both jobs (lines 40-43 and 89-92).
Add workflow_dispatch inputs release_run_id and release_sha to allow manual publish runs to download artifacts and checkout the correct commit. Make publish job run on manual dispatch or successful workflow_run. Use inputs.release_sha and inputs.release_run_id as fallbacks for checkout ref and artifact download run-id. Add artifact pattern filter in the release workflow.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/cli-publish.yml (1)
84-128:⚠️ Potential issue | 🟠 Major
publish-mainhas no "already published" guard — unlike the platform jobs.You gave the platform packages a nice
npm viewcheck (lines 68-77) to skip re-publishing. The main package? It just yeetsnpm publishand hopes for the best. If this workflow gets re-triggered (manual dispatch, re-run), it'll fail with a 403 "cannot publish over existing version" and you'll be staring at a red CI wondering what went wrong.Be consistent. Apply the same guard here.
Proposed fix — add version check before publish
+ - name: Check if version already published + id: check + shell: bash + run: | + VERSION="$(node -p "require('./cli/package.json').version")" + PKG="$(node -p "require('./cli/package.json').name")" + if npm view "${PKG}@${VERSION}" version 2>/dev/null; then + echo "skip=true" >> "$GITHUB_OUTPUT" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + - name: Publish package + if: steps.check.outputs.skip != 'true' working-directory: cli run: npm publish --access public --provenance
🤖 Fix all issues with AI agents
In @.github/workflows/cli-publish.yml:
- Around line 29-30: The workflow's matrix strategy currently enables fail-fast
(strategy: fail-fast: true), which can cancel remaining publish jobs and cause
partial releases; change the matrix configuration by setting fail-fast to false
(i.e., update the strategy's fail-fast setting in the workflow containing the
matrix/publish jobs) so all matrix publish attempts (e.g., cli-darwin-arm64,
cli-linux-x64, cli-win32-x64) run to completion and let the downstream
publish-main job decide whether to proceed.
- Around line 48-55: The CI is using unpinned versions: change the Setup Node.js
step (uses: actions/setup-node@v6) to set a stable node-version (e.g., replace
node-version: "latest" with node-version: "22" or "lts/*") and stop installing
npm@latest; instead pin the npm install to a specific known version (replace npm
install -g npm@latest with npm install -g npm@<major-or-fixed-version> such as
npm@10) or remove the explicit npm install if your chosen Node image already
provides the required npm; update the "Setup Node.js" and "Ensure npm CLI
supports trusted publishing" steps accordingly.
🧹 Nitpick comments (1)
.github/workflows/cli-publish.yml (1)
106-120: Tag selection is fragile when multiple tags point at HEAD.
git tag --points-at HEAD | head -n 1picks whichever taggitfeels like listing first (alphabetical by default). If someone tags bothv1.2.3andv1.2.3-rc1on the same commit, you get non-deterministic behavior. That's the kind of thing that works 99 times and then breaks in the one case that matters.Filter for your expected tag pattern:
Proposed fix
- TAG="$(git tag --points-at HEAD | head -n 1)" + TAG="$(git tag --points-at HEAD | grep -E '^v?[0-9]+\.[0-9]+\.[0-9]+' | head -n 1)"
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary by CodeRabbit
New Features
Chores
Tests
Documentation