Respect explicit -o when stdout is piped; rename events --effective-exposures#36
Conversation
…valid-exposures Replace the single boolean --effective-exposures flag on `events list` with a mutually exclusive --valid-exposures / --invalid-exposures pair, so users can explicitly request the exposures that were filtered OUT as ineffective — not just the ones that were kept. The tri-state (true / false / undefined) is plumbed through ListEvents to the `effective_exposures` filter so the API can distinguish "only valid", "only invalid", and "unfiltered".
List commands fell back to ids-only output whenever stdout was piped, even when the user explicitly passed `-o json`. The piping guard read `options.output` on the subcommand, but `-o, --output` is a global option, so the local value is always undefined — making the "explicit?" check always false. Detect the explicit flag once in `getGlobalOptions` via commander's option-value source, expose it as `outputExplicit`, and route the piping rule through a shared `shouldOutputIdsOnly` helper so the behavior stays consistent across every list command. Also exclude `.worktrees/**` from vitest discovery so sibling worktree checkouts don't appear as duplicate (stale) test files.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR introduces two parallel improvements: a refactoring of the events command's exposure filtering strategy, and an abstraction of piped-output behaviour common to list commands. The events command replaces its single Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/events/events.ts (1)
38-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
effectiveExposuresas a backward-compatible alias in the core params.Renaming the exported
ListEventsParamsfield is a breaking API change for@absmartly/cli/core/eventsconsumers. Existing callers usingeffectiveExposureswill lose this filter silently in JS.Suggested patch
export interface ListEventsParams { from?: number | undefined; to?: number | undefined; applications?: number[] | undefined; unitTypes?: number[] | undefined; eventTypes?: string[] | undefined; eventNames?: string[] | undefined; unitUids?: string[] | undefined; environmentTypes?: string[] | undefined; validExposures?: boolean | undefined; + /** `@deprecated` Use validExposures */ + effectiveExposures?: boolean | undefined; take?: number | undefined; skip?: number | undefined; } @@ - if (params.validExposures !== undefined) filters.effective_exposures = params.validExposures; + const effectiveExposures = params.validExposures ?? params.effectiveExposures; + if (effectiveExposures !== undefined) filters.effective_exposures = effectiveExposures;Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/events/events.ts` around lines 38 - 48, ListEventsParams removed the previously exported effectiveExposures field causing a breaking change; restore backwards compatibility by adding effectiveExposures?: boolean | undefined to the ListEventsParams interface (alongside the new validExposures?: boolean | undefined) and update any code that reads the filter (places that accept ListEventsParams) to treat effectiveExposures as an alias for validExposures (e.g., coalesce params.effectiveExposures ?? params.validExposures) so callers using either name behave identically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/events/index.ts`:
- Around line 60-65: The mutually-exclusive flag check runs after initialising
the API client; move the validation so it runs immediately after computing
globalOptions and before calling getAPIClientFromOptions. Specifically, in the
handler that currently calls getGlobalOptions(listCommand) and then
getAPIClientFromOptions(globalOptions), perform the if (options.validExposures
&& options.invalidExposures) throw new Error(...) check right after
getGlobalOptions(listCommand) (or even before that if options are available),
and only call getAPIClientFromOptions(globalOptions) once the flags validation
passes to avoid unnecessary client initialisation.
---
Outside diff comments:
In `@src/core/events/events.ts`:
- Around line 38-48: ListEventsParams removed the previously exported
effectiveExposures field causing a breaking change; restore backwards
compatibility by adding effectiveExposures?: boolean | undefined to the
ListEventsParams interface (alongside the new validExposures?: boolean |
undefined) and update any code that reads the filter (places that accept
ListEventsParams) to treat effectiveExposures as an alias for validExposures
(e.g., coalesce params.effectiveExposures ?? params.validExposures) so callers
using either name behave identically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71b7048d-7d15-4f0d-a09c-cc057cf40c43
📒 Files selected for processing (11)
README.mdsrc/commands/events/events.test.tssrc/commands/events/index.tssrc/commands/experiments/list.tssrc/core/events/events.test.tssrc/core/events/events.tssrc/lib/utils/api-helper.test.tssrc/lib/utils/api-helper.tssrc/lib/utils/list-command.test.tssrc/lib/utils/list-command.tsvitest.config.ts
| const globalOptions = getGlobalOptions(listCommand); | ||
| const client = await getAPIClientFromOptions(globalOptions); | ||
|
|
||
| if (options.validExposures && options.invalidExposures) { | ||
| throw new Error('--valid-exposures and --invalid-exposures are mutually exclusive'); | ||
| } |
There was a problem hiding this comment.
Validate mutually exclusive flags before client initialisation.
With invalid combined flags, the handler still initialises the API client first. That can surface unrelated auth/config errors instead of the intended usage error.
Suggested patch
.action(
withErrorHandling(async (options) => {
const globalOptions = getGlobalOptions(listCommand);
- const client = await getAPIClientFromOptions(globalOptions);
if (options.validExposures && options.invalidExposures) {
throw new Error('--valid-exposures and --invalid-exposures are mutually exclusive');
}
+ const client = await getAPIClientFromOptions(globalOptions);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const globalOptions = getGlobalOptions(listCommand); | |
| const client = await getAPIClientFromOptions(globalOptions); | |
| if (options.validExposures && options.invalidExposures) { | |
| throw new Error('--valid-exposures and --invalid-exposures are mutually exclusive'); | |
| } | |
| const globalOptions = getGlobalOptions(listCommand); | |
| if (options.validExposures && options.invalidExposures) { | |
| throw new Error('--valid-exposures and --invalid-exposures are mutually exclusive'); | |
| } | |
| const client = await getAPIClientFromOptions(globalOptions); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/events/index.ts` around lines 60 - 65, The mutually-exclusive
flag check runs after initialising the API client; move the validation so it
runs immediately after computing globalOptions and before calling
getAPIClientFromOptions. Specifically, in the handler that currently calls
getGlobalOptions(listCommand) and then getAPIClientFromOptions(globalOptions),
perform the if (options.validExposures && options.invalidExposures) throw new
Error(...) check right after getGlobalOptions(listCommand) (or even before that
if options are available), and only call getAPIClientFromOptions(globalOptions)
once the flags validation passes to avoid unnecessary client initialisation.
1 participant
Summary
Two unrelated changes bundled (per request):
-o json. The piping guard read the per-subcommandoptions.output, but-o, --outputis a global option, so that local value is alwaysundefined— making the "explicit?" check always false. Detect the explicit flag once ingetGlobalOptionsvia commander'sgetOptionValueSourceWithGlobals, expose it asoutputExplicit, and route the rule through a sharedshouldOutputIdsOnlyhelper so every list command behaves the same way. Also excludes.worktrees/**from vitest discovery so sibling worktree checkouts don't appear as duplicate (stale) test files.events list --effective-exposuresinto a mutually-exclusive--valid-exposures/--invalid-exposurespair so users can explicitly request the exposures that were filtered OUT, not just the ones that were kept. The tri-state is plumbed through to the API'seffective_exposuresfilter.Test plan
abs --profile demo-2 metrics list --items 200 -o json | catreturns JSON (was: ids only) — covered by new regression test inlist-command.test.ts-ostill emits ids only-o idsstill emits ids on a TTYevents list --valid-exposuressendseffective_exposures: trueevents list --invalid-exposuressendseffective_exposures: falseevents list --valid-exposures --invalid-exposureserrors with mutual-exclusion messagenpm run buildsucceedsnpm test— all 2377 passing (worktree noise excluded by vitest.config.ts change)Summary by CodeRabbit
Documentation
New Features
Tests
Chores