Conversation
fix: obtain BUILD_VERSION by reading package.json with readFileSync inside a try/catch to avoid top-level await and return unknown on error. Prefix mock.module calls with void in tests to silence unused-promise lint warnings.
|
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. WalkthroughThis PR introduces LetMe MFA credential integration to the CLI, refactoring the command architecture to support multiple subcommand paths (init, config, setup-totp) with shared SDK provisioning. It adds LetMe profile activation via OTP generation, updates the SDK credential provider chain, bumps versions across platform-specific packages, and expands documentation with guides and troubleshooting. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (app.ts)
participant LetMe as LetMe Module
participant SDK as SDK Service
participant AWS as AWS Profile
User->>CLI: cloudops-tools init --use-letme --account myaccount
CLI->>CLI: Detect --use-letme flag
CLI->>LetMe: activateLetmeProfile("myaccount")
LetMe->>LetMe: ensureLetmeAvailable()
LetMe->>LetMe: getTOTPSecret()
LetMe->>LetMe: generateTOTPToken()
LetMe->>LetMe: runLetmeObtain(profile, otp)
LetMe->>AWS: Set AWS_PROFILE env var
LetMe-->>CLI: Success
CLI->>SDK: Effect.provide(SdkLive)
CLI->>SDK: Generate inventory with credentials
SDK->>AWS: List/Describe resources
AWS-->>SDK: Resource data
SDK-->>CLI: Inventory
CLI-->>User: Report output
sequenceDiagram
participant App as app.ts
participant Commands as Command Handler
participant Effect as Effect Runtime
participant Config as ConfigService
participant Context as BunContext
App->>App: Parse arguments
App->>App: Detect CLI path (init/config/setup-totp)
App->>Commands: Strip tokens from args
Commands->>Effect: Build Effect.gen pipeline
Effect->>Config: Provide ConfigServiceLive
Effect->>Context: Provide BunContext
Effect->>Commands: Execute with provided services
Commands->>Effect: Run inner workflow
Effect-->>Commands: Result
Commands-->>App: Success/Error
App->>App: Handle error with debug mode override
App-->>App: Return exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
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)
49-53:⚠️ Potential issue | 🔴 CriticaloptionalDependencies still pin
0.1.3while the package itself is0.1.4.Look, this is the kind of thing that drives me absolutely nuts. You bumped the version to
0.1.4on line 3, the AI summary confirms the platform packages are also bumped to0.1.4, and yet right here these optional deps are still pointing at0.1.3. This will cause resolution mismatches — users pulling the new CLI version get stale platform binaries. This is not a theoretical problem; this is a "you ship broken artifacts" problem.Fix it. Now.
🐛 Proposed fix
"optionalDependencies": { - "@cloudops-tools/cli-darwin-arm64": "0.1.3", - "@cloudops-tools/cli-linux-x64": "0.1.3", - "@cloudops-tools/cli-win32-x64": "0.1.3" + "@cloudops-tools/cli-darwin-arm64": "0.1.4", + "@cloudops-tools/cli-linux-x64": "0.1.4", + "@cloudops-tools/cli-win32-x64": "0.1.4" }
🤖 Fix all issues with AI agents
In `@cli/src/app.ts`:
- Around line 47-56: The main-help and help-examples branches currently print
the top-level help whenever wantsHelp/wantsHelpExamples is true and neither
forceInit nor forceSetupTotp are set, which ignores subcommand help requests;
update both if conditions to also check that wantsConfig is false (e.g., if
(wantsHelp && !forceInit && !forceSetupTotp && !wantsConfig) and similarly for
wantsHelpExamples) so that when wantsConfig is true the config subcommand can
render its own help/ examples instead of the main command's.
- Around line 58-65: The normalization handles forceSetupTotp/forceInit but not
a corresponding forceConfig, causing inconsistent flag support for "config"; add
a forceConfig boolean (derived from argv or parsed flags) and update the
normalizedArgs construction (the ternary branches where normalizedArgs is built)
to inject "config" when forceConfig is true (mirroring forceSetupTotp/forceInit
behavior), and change wantsConfig to: forceConfig ||
normalizedArgsForDetection.includes("config"); alternatively, if you intend
"config" to be subcommand-only, add a brief comment next to normalizedArgs/
wantsConfig explaining that decision and why a --config flag is omitted
(reference variables: normalizedArgs, forceSetupTotp, forceInit, wantsConfig).
In `@cli/src/commands/main.ts`:
- Around line 150-152: The code is suppressing a real readonly vs mutable array
type mismatch by using items as unknown[] when calling
reporting.generateMarkdownReport(handler.title, ...); fix by aligning types
rather than asserting: either change generateMarkdownReport to accept
ReadonlyArray<Record<string, unknown>> (or a generic ReadonlyArray<T>), or make
the producer (the handler that returns ReadonlyArray<Record<string, unknown>>)
return a mutable Record<string, unknown>[]; alternately, if you must keep the
current runtime shape, cast items to ReadonlyArray<Record<string, unknown>> (or
to ReadonlyArray<unknown>) before passing it in and add a brief comment
explaining why the cast is safe; update the call site around
generateMarkdownReport, handler.title, and items accordingly.
In `@cli/src/lib/letme.ts`:
- Around line 7-40: The ensureLetmeAvailable function (and runLetmeObtain)
currently spawns a child process without a timeout causing potential hangs; add
a configurable timeout (e.g., 30_000 ms) to the Promise that, when reached,
kills the spawned child process (child.kill()), removes listeners/clears any
buffers, and rejects with a clear timeout Error like "letme preflight check
timed out"; ensure you clear the timeout timer on normal resolve/reject to avoid
leaks and guard against multiple settle by using a settled flag or once-only
resolve/reject calls so the child error/close handlers don’t try to settle after
the timeout.
- Line 83: Replace the deprecated adapter-style generator used with Effect.gen
by removing the adapter parameter and using the adapter-less yield* form: change
the Effect.gen(function* (_){ ... yield* _("...") ... }) call to
Effect.gen(function* () { ... yield* someEffect ... }), i.e. remove the "_"
parameter and update all occurrences inside that generator to directly yield*
the Effect values (reference: the Effect.gen call in cli/src/lib/letme.ts that
currently uses function* (_)).
In `@documentation/content/docs/commands/index.mdx`:
- Around line 23-29: The docs table incorrectly lists `use-letme` as a
subcommand; remove the `use-letme` row from the Subcommands table so it matches
the CLI wiring where Command.withSubcommands([setupTotpCommand, initCommand,
describeCommand, configCommand]) defines subcommands, and treat `--use-letme` as
an option (already documented under Root Scan Options). Update the Subcommands
table to only include `init`, `describe`, `setup-totp`, and `config`, and add or
link a note under the Root Scan Options table pointing to the [`use-letme`
guide](/docs/commands/use-letme) for the flag workflow.
In `@documentation/content/docs/troubleshooting.mdx`:
- Around line 1-85: The Troubleshooting page is missing guidance for the new
--use-letme flow: add a "LetMe / --use-letme" subsection to the Troubleshooting
document that documents the three failure cases from use-letme.mdx (letme binary
not found, missing TOTP secret, missing --account) and shows the corrective
steps/commands (how to ensure letme is on PATH, run setup-totp to configure
TOTP, and re-run the command with --account or explain required flag usage and
error messaging). Reference the flag name (--use-letme / --account), the helper
command (setup-totp), and the binary name (letme) in the text so users can
locate relevant code/docs and reproduce the fixes.
In `@sdk/package.json`:
- Around line 24-26: The npm scripts in package.json are inconsistent: "test" is
scoped to "./test/unit/lib" while "test:coverage" and "test:watch" run unscoped
tests; update the "test:coverage" and "test:watch" entries so they call the same
scoped command as "test" (i.e., use bun test ./test/unit/lib) and preserve their
flags (--coverage for test:coverage, --watch for test:watch) so coverage and
watch modes run the same subset of tests; modify the "test:coverage" and
"test:watch" script values accordingly to match the "test" script behavior.
🧹 Nitpick comments (15)
documentation/content/docs/choose-command.mdx (1)
12-21: Potential confusion betweendescribesubcommand and--describeflag.Lines 16 and 17 show two different invocation styles for describe functionality:
cloudops-tools describe(subcommand)cloudops-tools --describe <type> --region <region>(root flag)If both are real and intentional, fine — but this will confuse users. Consider adding a brief note explaining why there are two forms and when to prefer one over the other, the same way you did in the Notes section for
rootvsinit.documentation/content/docs/scan-profiles.mdx (1)
1-64: Documentation is fine, but clarify the two command entry points.Some profiles use
cloudops-tools init ...while others use barecloudops-tools ...(Lines 13, 61 vs. Lines 21, 33, 41, 53). If these reflect genuinely different subcommand paths, that's fine — but a reader unfamiliar with the CLI will wonder "when do I useinitand when don't I?" A one-liner at the top explaining the distinction would save everyone a round-trip to the help text.cli/src/lib/letme.ts (1)
89-94: Error messages get double-wrapped into gibberish.Lines 92 and 113:
catch: (error) => new Error(String(error)). Iferroris already anError,String(error)produces"Error: letme preflight check failed: ...", and then you wrap that innew Error(...), giving the user:Error: Error: letme preflight check failed: .... Nobody wants to read that.♻️ Preserve the original error or extract the message
yield* _( Effect.tryPromise({ try: () => ensureLetmeAvailable(), - catch: (error) => new Error(String(error)), + catch: (error) => (error instanceof Error ? error : new Error(String(error))), }), );yield* _( Effect.tryPromise({ try: () => runLetmeObtain(normalizedProfileName, otp), - catch: (error) => new Error(String(error)), + catch: (error) => (error instanceof Error ? error : new Error(String(error))), }), );Also applies to: 110-115
cli/test/unit/lib/letme.test.ts (2)
86-95: One happy-path test for an MFA credential flow? Really?I get it — you wrote a test, and that's better than nothing. But this is your MFA credential activation pipeline. This is the code path where, if something goes wrong, users either can't authenticate or — worse — silently get the wrong credentials. And you're testing it with one scenario?
Where are the tests for:
- Empty/whitespace-only profile name (should fail with a clear error)
letmebinary not found (ENOENT path)- TOTP secret retrieval failure
- OTP generation failure
letme obtainreturning non-zero exit codeYour fake spawn always emits
closewith code0. You literally cannot test failure paths with this mock as written. The error handling code inletme.tsis completely untested.I'm not going to block the merge on this, but you need to come back and fill these in. Mark it as a follow-up if you must.
Do you want me to generate a more comprehensive test suite covering these failure paths, or open an issue to track this?
8-8: Relative path for SDK import instead of package name.Line 8 uses
../../../../sdk/src/index.tsrather than@cloudops-tools/sdk. This might be a Bun test resolution necessity, but it couples the test to the repo's directory structure. If you can make@cloudops-tools/sdkresolve correctly in the test context, prefer that.documentation/src/routes/docs/$.tsx (1)
11-50: The new routes are fine, but this manual registry is a ticking time bomb.The four new entries (Lines 17-19, 25) follow the established pattern — no complaints there. But look at this thing: 50 hand-maintained entries that must stay in lockstep with actual MDX files. Miss one and you get a silent 404. Add one and forget to register it here and nobody sees the page.
This isn't something you need to fix right now in this PR, but somebody should consider generating this map from the filesystem (glob the
.mdxfiles at build time) before it becomes truly unmanageable.cli/src/commands/describe.ts (1)
17-33: The outerEffect.genwrapper is pointless — flatten this.The outer
Effect.genat Line 17 does nothing exceptyield*a single inner effect. That's like writing a function whose sole purpose is to call another function. The command handler can directly return the inner effect piped withprovide:♻️ Flatten the unnecessary nesting
({ type, region, id, debug }) => - Effect.gen(function* (_) { - const runWithSdk = Effect.gen(function* (_) { - const reporting = yield* _(ReportingService); - const report = yield* _(reporting.describeResourceHarder(type, region, id, debug)); - if (report.startsWith("Unsupported resource type")) { - yield* _(Console.log(ui.error(report))); - return; - } - if (report.startsWith("Resource not found")) { - yield* _(Console.log(ui.warn(report))); - return; - } - yield* _(Console.log(report)); - }); - - yield* _(runWithSdk.pipe(Effect.provide(SdkLive))); - }), + Effect.gen(function* (_) { + const reporting = yield* _(ReportingService); + const report = yield* _(reporting.describeResourceHarder(type, region, id, debug)); + if (report.startsWith("Unsupported resource type")) { + yield* _(Console.log(ui.error(report))); + return; + } + if (report.startsWith("Resource not found")) { + yield* _(Console.log(ui.warn(report))); + return; + } + yield* _(Console.log(report)); + }).pipe(Effect.provide(SdkLive)),cli/src/app.ts (2)
30-45: Version fallback is fine, butString(version)is redundant.The
readFileSyncfallback forBUILD_VERSIONis a reasonable approach for dev environments. The try/catch is correct. Minor nit: Line 43'sString(version)is unnecessary — every code path already returns astring. Not going to lose sleep over it.
67-89:stripFirstTokenandstripTokens— functional, but the naming obscures the semantic difference.
stripFirstTokenremoves the first occurrence.stripTokensremoves all occurrences of multiple tokens. These are used together on Lines 115/117. The compositionstripTokens(stripFirstToken(normalizedArgs, "setup-totp"), ["--debug"])is correct but the reader has to mentally diff the two functions to understand why both exist.A brief comment above each would save everyone 30 seconds of "wait, why are there two of these?"
cli/src/commands/init.ts (3)
47-60: Duplicated--use-letmevalidation block — extract a shared helper.This exact same pre-flight check (require
--account, fail with the same message, callactivateLetmeProfile) is copy-pasted inmain.ts. I don't care how many subcommands you add — you do NOT copy-paste validation logic and pray that future-you remembers to update both. Extract this into a shared function in@/lib/letmeor a common helper.♻️ Example shared helper
Create a reusable helper, e.g. in
cli/src/lib/letme.ts:export const requireLetmeActivation = (account: Option.Option<string>) => Effect.gen(function* (_) { const letmeProfile = Option.getOrUndefined(account); if (!letmeProfile) { return yield* _( Effect.fail( new Error( "Missing required --account <profile> with --use-letme. Example: cloudops-tools --use-letme --account <profile>", ), ), ); } yield* _(activateLetmeProfile(letmeProfile)); });Then in both
init.tsandmain.ts:if (useLetme) { - const letmeProfile = Option.getOrUndefined(account); - if (!letmeProfile) { - return yield* _( - Effect.fail( - new Error( - "Missing required --account <profile> with --use-letme. Example: cloudops-tools init --use-letme --account engineering-prod", - ), - ), - ); - } - - yield* _(activateLetmeProfile(letmeProfile)); + yield* _(requireLetmeActivation(account)); }
62-64: Inconsistentyield*style — pick one and stick with it.Line 63 uses
yield* Effect.sync(...)(no adapter), but Line 64 usesyield* _(UtilService)(with adapter). This isn't a bug, but inconsistency in a codebase is how things rot. The rest of this file andmain.tsuse the adapter pattern. Be consistent.♻️ Proposed fix
- const progress = yield* Effect.sync(() => startProgressRenderer({ debug })); + const progress = yield* _(Effect.sync(() => startProgressRenderer({ debug })));
73-75:Option<string[] | undefined>defeats the purpose of Option.
Option.mapreturningundefinedinside producesOption<string[] | undefined>— wrappingundefinedinside anOptionis semantically confused.Optionalready represents the absence of a value. UseOption.flatMaporOption.filter+Option.mapso that"all"maps toOption.none().♻️ Proposed fix
- const serviceList = Option.map(services, (s: string) => - s.toLowerCase() === "all" ? undefined : s.split(",").map((svc) => svc.trim()), - ); + const serviceList = Option.flatMap(services, (s: string) => + s.toLowerCase() === "all" + ? Option.none() + : Option.some(s.split(",").map((svc) => svc.trim())), + );cli/src/commands/main.ts (3)
119-178: Inconsistentyield*adapter usage within the same generator.Lines 120-126 use
yield* _(...)(adapter pattern). Lines 172-177 use bareyield*(no adapter). Both work, but pick one style and use it everywhere. Mixing them in the same function is how you get developers arguing in PRs about which one is "right" instead of shipping code.
166-170:main.tshandlesservicescorrectly, butinit.tsdoesn't — reconcile them.Here you use
Option.matchreturningstring[] | undefineddirectly — correct. Ininit.tslines 73-75, you useOption.mapreturningOption<string[] | undefined>which wrapsundefinedinside an Option. These are the same logical operation implemented two different ways. Themain.tsversion is the right one. Makeinit.tsmatch.
160-162:Effect.promiseswallows the rejection reason — preferEffect.tryPromise.
mkdirandwritecan fail (permissions, disk full, etc.).Effect.promiseexpects the promise to always resolve. If it rejects, the fiber dies with a defect (unchecked).Effect.tryPromiselets you map the error into a typed failure so the caller can handle it gracefully instead of getting a cryptic fiber dump.♻️ Proposed fix
- yield* _(Effect.promise(() => mkdir(outputDir, { recursive: true }))); - yield* _(Effect.promise(() => write(outputPath, report))); + yield* _( + Effect.tryPromise({ + try: () => mkdir(outputDir, { recursive: true }), + catch: (err) => new Error(`Failed to create output directory: ${String(err)}`), + }), + ); + yield* _( + Effect.tryPromise({ + try: () => write(outputPath, report), + catch: (err) => new Error(`Failed to write report: ${String(err)}`), + }), + );
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
cloudops-tools/cli/src/commands/setup-totp.ts
Lines 5 to 7 in 10191f9
Convert setup-totp rejections into failures
Using Effect.promise treats rejected promises as defects, which bypasses the CLI’s withErrorHandling (it only catches failures), so common input errors in setupTOTP() (e.g., empty/invalid secret) will surface as uncaught defects with stack traces even when --debug isn’t set. Wrapping setupTOTP() in Effect.tryPromise (or mapping the error) keeps the error on the failure channel and preserves the intended user-friendly error handling path.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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
Release Notes
New Features
--use-letmeflag to streamline MFA credential activation in scan commands.configandsetup-totpsubcommands for advanced workflows.Documentation
Chores