fix: allow minimal scope in CLI commands#24
Conversation
|
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 as they are similar to previous changes (1)
WalkthroughAdded a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ocbs/cli.py (2)
26-33:⚠️ Potential issue | 🟠 Major
backup --source native --scope minimalcan still execute with non-minimal native flags.Line 26 now allows
minimal, but the native path (core._scope_to_native_args) has no explicitMINIMALmapping, so it falls back to default flags. That’s like labeling a button “eco mode” while the engine still runs in sport mode. Please guard or explicitly map this combination before callingcore.backup(...).Suggested guard in CLI (prevents silent scope mismatch)
def backup(ctx, scope, source, reason): """Create a backup.""" core = ctx.obj['core'] scope_enum = BackupScope(scope) source_enum = BackupSource(source) if source else None + effective_source = source_enum or core.get_default_source() + + if scope_enum == BackupScope.MINIMAL and effective_source == BackupSource.NATIVE: + raise click.BadParameter( + "'minimal' scope is not supported for native backups yet. " + "Use '--source direct' or a broader scope." + ) try: manifest = core.backup(scope_enum, reason, source=source_enum)Also applies to: 39-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/cli.py` around lines 26 - 33, The CLI allows --scope minimal but when --source is 'native' core._scope_to_native_args lacks a MINIMAL mapping so native backups run with wrong flags; update the CLI handler that constructs native args (before calling core.backup) to either explicitly map the 'minimal' scope to the correct native flag set in core._scope_to_native_args or add a guard that rejects/overrides the combination (e.g., error out or translate 'minimal' into the supported native flags) so core.backup is never called with a silent scope mismatch; reference the code paths around core._scope_to_native_args and the call site core.backup to find where to implement the mapping/guard.
164-179:⚠️ Potential issue | 🟠 Major
native_backup --scope minimalcurrently behaves as full scope.At Line 164,
minimalis accepted, but Lines 175-179 only special-caseconfigandconfig+session;minimalfalls through to “no flags” (full backup semantics). This violates user intent and can back up much more than requested.Suggested fix (fail fast until native minimal semantics are defined)
def native_backup(scope, verify, output): @@ - if scope == "config": + if scope == "minimal": + click.echo( + "Error: --scope minimal is not supported for native backup yet.", + err=True, + ) + sys.exit(2) + elif scope == "config": args.append("--only-config") elif scope == "config+session": args.append("--no-include-workspace")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/cli.py` around lines 164 - 179, The native_backup command incorrectly treats scope='minimal' as full scope; update the native_backup function to explicitly handle the 'minimal' choice (the scope parameter and args list in native_backup) and fail fast until minimal semantics are implemented—e.g., detect if scope == 'minimal' and raise a click.ClickException or click.BadParameter with a clear message so users cannot accidentally run a full backup when they asked for minimal. Ensure you modify the conditional block that currently checks scope == "config" and "config+session" to include the 'minimal' branch and return/abort accordingly.
🧹 Nitpick comments (1)
src/ocbs/cli.py (1)
26-27: Centralize scope choices to avoid future drift.The same scope literal list is repeated 4 times. A shared constant (e.g.,
SCOPE_CHOICES) will keep command behavior in sync and prevent this exact mismatch from resurfacing.Also applies to: 107-108, 127-128, 164-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/cli.py` around lines 26 - 27, Create a single shared constant (e.g., SCOPE_CHOICES = ['minimal', 'config', 'config+session', 'config+session+workspace']) and replace each repeated literal list in the click.option('--scope', ...) declarations with type=click.Choice(SCOPE_CHOICES) so all scope options (the various click.option('--scope', ...) occurrences in src/ocbs/cli.py) reference the same list; update any imports/ordering as needed so SCOPE_CHOICES is defined near the top of the module and used by each option to prevent drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ocbs/cli.py`:
- Around line 26-33: The CLI allows --scope minimal but when --source is
'native' core._scope_to_native_args lacks a MINIMAL mapping so native backups
run with wrong flags; update the CLI handler that constructs native args (before
calling core.backup) to either explicitly map the 'minimal' scope to the correct
native flag set in core._scope_to_native_args or add a guard that
rejects/overrides the combination (e.g., error out or translate 'minimal' into
the supported native flags) so core.backup is never called with a silent scope
mismatch; reference the code paths around core._scope_to_native_args and the
call site core.backup to find where to implement the mapping/guard.
- Around line 164-179: The native_backup command incorrectly treats
scope='minimal' as full scope; update the native_backup function to explicitly
handle the 'minimal' choice (the scope parameter and args list in native_backup)
and fail fast until minimal semantics are implemented—e.g., detect if scope ==
'minimal' and raise a click.ClickException or click.BadParameter with a clear
message so users cannot accidentally run a full backup when they asked for
minimal. Ensure you modify the conditional block that currently checks scope ==
"config" and "config+session" to include the 'minimal' branch and return/abort
accordingly.
---
Nitpick comments:
In `@src/ocbs/cli.py`:
- Around line 26-27: Create a single shared constant (e.g., SCOPE_CHOICES =
['minimal', 'config', 'config+session', 'config+session+workspace']) and replace
each repeated literal list in the click.option('--scope', ...) declarations with
type=click.Choice(SCOPE_CHOICES) so all scope options (the various
click.option('--scope', ...) occurrences in src/ocbs/cli.py) reference the same
list; update any imports/ordering as needed so SCOPE_CHOICES is defined near the
top of the module and used by each option to prevent drift.
1 participant
Fixes an oversight where
--scope minimalwas defined in the core and docs but not permitted by the CLI'sclick.Choicearray.Summary by CodeRabbit
New Features
Bug Fixes / Validation