-
Notifications
You must be signed in to change notification settings - Fork 124
feat(cli): modularize CLI with noun-verb commands, --json output, and auto-discovery #445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 40 commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
fb0dca6
feat(cli): add modular CLI foundation (Wave 0)
yeldarby ffb0d9a
Add auth and workspace CLI handler modules
yeldarby 68fc439
fix(cli): respect --json flag for --version output
yeldarby 88ef877
Add project and version CLI handler modules
yeldarby 2fbfe60
fix(cli): gracefully handle broken handler modules during auto-discovery
yeldarby fd60a4e
Add image and annotation CLI handler modules
yeldarby e4e189e
Add remaining CLI handlers: deployment, search, workflow, video, univ…
yeldarby ab7774d
Add model, train, and infer CLI handler modules
yeldarby fa921b4
Fix deployment handler to avoid fragile argparse internals
yeldarby 5820b1c
Fix project list stdout pollution breaking --json output
yeldarby 01eeaac
Fix 3 bugs and 1 rough-edge in auth and workspace handlers
yeldarby 39c63b3
feat(cli): wire up backwards-compat aliases and integrate all handlers
yeldarby 5266e4f
Fix QA feedback on image and annotation handlers
yeldarby b9315bb
Fix auth status env var fallback and workspace get member count
yeldarby 2960cf8
fix(cli): address QA findings from Wave 3
yeldarby f30356e
Fix deployment help display, structured errors, and search stdout leak
yeldarby d5d1b48
Fix error handling in model and train handlers
yeldarby 65f67f7
feat(cli): add suppress_sdk_output helper for --json/--quiet mode
yeldarby c45fd9b
docs: add DEVIATIONS.md documenting plan changes during implementation
yeldarby 7ca9721
fix(cli): custom HelpFormatter hides legacy aliases, fix annotation s…
yeldarby 406a19d
fix(cli): default annotation group and improve project get display
yeldarby edacf90
fix(cli): format epoch timestamps in project get output
yeldarby 4649508
fix(cli): parse JSON error strings to avoid double-encoding in --json…
yeldarby 142c25d
refactor(cli): centralize JSON error parsing in output_error
yeldarby a8203a1
fix(cli): add suppress_sdk_output and error handling to image/version…
yeldarby cca8809
fix(cli): extend suppress_sdk_output scope for image upload and versi…
yeldarby 59ec057
fix(cli): always suppress SDK init noise in image upload and version …
yeldarby 7b877cc
fix(cli): polish remaining rough edges from QA round 2
yeldarby c9bb74b
fix(cli): allow --json in any position, fix type choices, unwrap doub…
yeldarby 2558170
feat(cli): add descriptive args to batch command stubs
yeldarby 9ee05f0
fix(cli): whoami honors --api-key, normalize error JSON schema
yeldarby 4b600f0
fix(cli): rewrite deployment handler with clean names, legacy shims h…
yeldarby 1756e4e
fix(cli): workspace list falls back to API when no local config exists
yeldarby 54f055f
fix(cli): project list/get auto-detect workspace from API key
yeldarby f213c87
fix(cli): add hint for non-image upload errors
yeldarby 1d8ac14
fix(cli): address review feedback -- alias bug, stubs, output consist…
yeldarby 9e93fcf
fix(cli): address remaining review items -- URL params, exit codes, t…
yeldarby a06a059
refactor(tests): consolidate alias and reorder_argv tests into test_d…
yeldarby 11a2de2
security(cli): fix config file permissions and login alias api_key dest
yeldarby adfa656
fix(cli): add backwards-compat re-exports to roboflowpy.py shim
yeldarby b894bdf
fix(pre_commit): 🎨 auto format pre-commit hooks
pre-commit-ci[bot] 7b4a4b1
chore: remove DEVIATIONS.md (internal planning doc, not needed in PR)
yeldarby 71c530d
fix(cli): address Codex review - flag collision, missing args, compat
yeldarby edef1f3
docs: update CLAUDE.md, CLI-COMMANDS.md, CONTRIBUTING.md for modular CLI
yeldarby File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| # CLI Modernization: Plan Deviations | ||
|
|
||
| This document records deviations from the original plan made during implementation, per the orchestration guidelines. | ||
|
|
||
| ## Deviations | ||
|
|
||
| ### 1. Graceful handler error handling in auto-discovery | ||
| **Plan**: Auto-discovery loads all handlers without error handling. | ||
| **Change**: Added try/except around each handler's `register()` call so a broken handler doesn't crash the entire CLI. | ||
| **Reason**: During Wave 1, engineer-5's in-progress deployment handler had a bug that crashed every CLI command. This was a QA blocker. | ||
| **Assessment**: Good permanent change. A broken handler should never take down the CLI. | ||
|
|
||
| ### 2. SDK stdout suppression via context manager | ||
| **Plan**: Not explicitly planned. | ||
| **Change**: Added `suppress_sdk_output(args)` context manager in `_output.py` that redirects stdout when `--json` or `--quiet` is active. Used by search and model handlers. | ||
| **Reason**: The SDK's `Roboflow()` and `rf.workspace()` print "loading Roboflow workspace..." to stdout, which corrupts `--json` output for piping. QA flagged this as a bug. | ||
| **Assessment**: Correct fix. The SDK's chatty output is a design debt that should eventually be addressed at the SDK level, but suppressing at the CLI layer is the right short-term approach. | ||
|
|
||
| ### 3. Error message extraction from JSON-encoded exceptions | ||
| **Plan**: Not explicitly planned. | ||
| **Change**: Added `_extract_error_message()` helper in model.py and train.py that parses JSON error strings from `RoboflowError` exceptions into clean messages. | ||
| **Reason**: QA found that API errors were double-encoded in `--json` output (JSON string inside JSON). The API returns error bodies as exception message strings. | ||
| **Assessment**: Good fix. Should eventually be centralized into `_output.py` rather than duplicated. | ||
|
|
||
| ### 4. Legacy aliases show ==SUPPRESS== in help | ||
| **Plan**: Legacy aliases would be completely hidden from help. | ||
| **Change**: Used `argparse.SUPPRESS` for help text, which hides the description but still shows the command name in the choices list with `==SUPPRESS==` text. | ||
| **Known limitation**: argparse doesn't support fully hiding subparser choices. Would need a custom HelpFormatter to fix completely. | ||
| **Assessment**: Cosmetic issue. The commands work correctly. Can be addressed in a follow-up. | ||
|
|
||
| ### 5. No separate worktree branches to merge | ||
| **Plan**: Engineers work in isolated worktrees, lead merges branches. | ||
| **Actual**: Engineers' worktrees shared the filesystem with the main branch (worktree isolation cleaned up but files persisted). Changes were committed directly to the working directory. | ||
| **Assessment**: Worked fine in practice — no merge conflicts since each engineer owned distinct files. The worktree isolation still prevented engineers from interfering with each other's running processes. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| # PYTHON_ARGCOMPLETE_OK | ||
| """Roboflow CLI — computer vision at your fingertips. | ||
|
|
||
| This package implements the modular CLI for the Roboflow Python SDK. | ||
| Commands are auto-discovered from the ``handlers`` sub-package: any module | ||
| that exposes a ``register(subparsers)`` callable is loaded automatically. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import argparse | ||
| import importlib | ||
| import pkgutil | ||
| import sys | ||
| from typing import Any | ||
|
|
||
| import roboflow | ||
| from roboflow.cli import handlers as _handlers_pkg | ||
|
|
||
|
|
||
| class _CleanHelpFormatter(argparse.HelpFormatter): | ||
| """Custom formatter that hides SUPPRESS-ed subparser choices. | ||
|
|
||
| The default argparse formatter includes *all* subparser names in the | ||
| ``{a,b,c,...}`` usage line and shows ``==SUPPRESS==`` in the command | ||
| list. This formatter filters both so that hidden legacy aliases are | ||
| truly invisible. | ||
| """ | ||
|
|
||
| def _format_action(self, action: argparse.Action) -> str: | ||
| # Hide subparser entries whose help is SUPPRESS | ||
| if action.help == argparse.SUPPRESS: | ||
| return "" | ||
| return super()._format_action(action) | ||
|
|
||
| def _metavar_formatter( | ||
| self, | ||
| action: argparse.Action, | ||
| default_metavar: str, | ||
| ) -> Any: | ||
| if isinstance(action, argparse._SubParsersAction): | ||
| # Filter choices to only those with visible help | ||
| visible = [ | ||
| name | ||
| for name, parser in action.choices.items() | ||
| if not any(ca.dest == name and ca.help == argparse.SUPPRESS for ca in action._choices_actions) | ||
| and name in [ca.dest for ca in action._choices_actions if ca.help != argparse.SUPPRESS] | ||
| ] | ||
| if visible: | ||
|
|
||
| def _fmt(tuple_size: int) -> tuple[str, ...]: | ||
| result = "{" + ",".join(visible) + "}" | ||
| return (result,) * tuple_size if tuple_size > 1 else (result,) | ||
|
|
||
| return _fmt | ||
| return super()._metavar_formatter(action, default_metavar) | ||
|
|
||
|
|
||
| def build_parser() -> argparse.ArgumentParser: | ||
| """Build the root argument parser with global flags and auto-discovered handlers.""" | ||
| parser = argparse.ArgumentParser( | ||
| prog="roboflow", | ||
| description="Roboflow CLI: computer vision at your fingertips", | ||
| formatter_class=_CleanHelpFormatter, | ||
| ) | ||
|
|
||
| # --- global flags --- | ||
| parser.add_argument( | ||
| "--json", | ||
| "-j", | ||
| dest="json", | ||
| action="store_true", | ||
| default=False, | ||
| help="Output results as JSON (stable schema, for agents and piping)", | ||
| ) | ||
| parser.add_argument( | ||
| "--api-key", | ||
| "-k", | ||
| dest="api_key", | ||
| default=None, | ||
| help="API key override (default: $ROBOFLOW_API_KEY or config file)", | ||
| ) | ||
| parser.add_argument( | ||
| "--workspace", | ||
| "-w", | ||
| dest="workspace", | ||
| default=None, | ||
| help="Workspace URL or ID override (default: configured default)", | ||
| ) | ||
| parser.add_argument( | ||
| "--quiet", | ||
| "-q", | ||
| dest="quiet", | ||
| action="store_true", | ||
| default=False, | ||
| help="Suppress non-essential output (progress bars, status messages)", | ||
| ) | ||
| parser.add_argument( | ||
| "--version", | ||
| action="store_true", | ||
| default=False, | ||
| help="Show package version and exit", | ||
| ) | ||
|
|
||
| # --- subcommands --- | ||
| subparsers = parser.add_subparsers(title="commands", dest="command") | ||
|
|
||
| # Auto-discover handler modules (skip private modules starting with _) | ||
| for _importer, modname, _ispkg in pkgutil.iter_modules(_handlers_pkg.__path__): | ||
| if modname.startswith("_"): | ||
| continue | ||
| try: | ||
| mod = importlib.import_module(f"roboflow.cli.handlers.{modname}") | ||
| if hasattr(mod, "register"): | ||
| mod.register(subparsers) | ||
| except Exception as exc: # noqa: BLE001 | ||
| # A broken handler must not take down the entire CLI | ||
| import logging | ||
|
|
||
| logging.getLogger("roboflow.cli").debug("Failed to load handler %s: %s", modname, exc) | ||
|
|
||
| # Load aliases last so they can reference handler functions | ||
| from roboflow.cli.handlers import _aliases | ||
|
|
||
| _aliases.register(subparsers) | ||
|
|
||
| parser.set_defaults(func=None) | ||
| return parser | ||
|
|
||
|
|
||
| def _show_version(args: argparse.Namespace) -> None: | ||
| if getattr(args, "json", False): | ||
| import json | ||
|
|
||
| print(json.dumps({"version": roboflow.__version__})) | ||
| else: | ||
| print(roboflow.__version__) | ||
|
|
||
|
|
||
| def _reorder_argv(argv: list[str]) -> list[str]: | ||
| """Move known global flags that appear after the subcommand to the front. | ||
|
|
||
| argparse only recognises global flags when they appear *before* the | ||
| subcommand. Many users (and AI agents) naturally write them at the end, | ||
| e.g. ``roboflow project list --json``. This helper transparently | ||
| re-orders the argv so those flags are consumed by the root parser. | ||
| """ | ||
| global_flags_with_value = {"--api-key", "-k", "--workspace", "-w"} | ||
| global_flags_bool = {"--json", "-j", "--quiet", "-q", "--version"} | ||
|
|
||
| reordered: list[str] = [] | ||
| rest: list[str] = [] | ||
| i = 0 | ||
| while i < len(argv): | ||
| arg = argv[i] | ||
| if arg in global_flags_bool: | ||
| reordered.append(arg) | ||
| elif arg in global_flags_with_value: | ||
| reordered.append(arg) | ||
| if i + 1 < len(argv): | ||
| i += 1 | ||
| reordered.append(argv[i]) | ||
| else: | ||
| rest.append(arg) | ||
| i += 1 | ||
| return reordered + rest | ||
|
|
||
|
|
||
| def main() -> None: | ||
| """CLI entry point.""" | ||
| parser = build_parser() | ||
| args = parser.parse_args(_reorder_argv(sys.argv[1:])) | ||
|
|
||
| if args.version: | ||
| _show_version(args) | ||
| sys.exit(0) | ||
|
|
||
| if args.func is not None: | ||
| args.func(args) | ||
| else: | ||
| parser.print_help() | ||
| sys.exit(0) | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| """Structured output helpers for the Roboflow CLI. | ||
|
|
||
| Every command should use ``output()`` for its result and ``output_error()`` | ||
| for failures so that ``--json`` mode works uniformly. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import contextlib | ||
| import io | ||
| import json | ||
| import sys | ||
| from typing import Any, Iterator, Optional | ||
|
|
||
|
|
||
| def output(args: Any, data: Any, text: Optional[str] = None) -> None: | ||
| """Print a command result in JSON or human-readable format. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| args: | ||
| The parsed argparse namespace (must have a ``json`` attribute). | ||
| data: | ||
| Structured data to emit when ``--json`` is active. Also used as | ||
| fallback when *text* is ``None``. | ||
| text: | ||
| Human-readable string printed in normal (non-JSON) mode. When | ||
| ``None``, *data* is pretty-printed as JSON regardless of mode. | ||
| """ | ||
| if getattr(args, "json", False): | ||
| print(json.dumps(data, indent=2, default=str)) | ||
|
|
||
| elif text is not None: | ||
| print(text) | ||
|
|
||
| else: | ||
| # Fallback: pretty-print data even in non-JSON mode | ||
| print(json.dumps(data, indent=2, default=str)) | ||
|
|
||
|
|
||
|
|
||
| def _parse_error_message(raw: str) -> tuple[Optional[dict[str, Any]], str]: | ||
| """Try to parse a raw error string that may contain embedded JSON. | ||
|
|
||
| Returns ``(parsed_dict_or_None, human_readable_message)``. | ||
| The *parsed_dict* is the deserialized JSON when the string is JSON, | ||
| otherwise ``None``. The *human_readable_message* drills into nested | ||
| ``error.message`` structures so the text-mode output is clean. | ||
| """ | ||
| text = raw.strip() | ||
| # Strip status-code prefix like "404: {...}" | ||
| colon_idx = text.find(": {") | ||
| if 0 < colon_idx < 5: | ||
| text = text[colon_idx + 2 :] | ||
| try: | ||
| parsed = json.loads(text) | ||
| if isinstance(parsed, dict): | ||
| err = parsed.get("error", parsed) | ||
| if isinstance(err, dict): | ||
| human = str(err.get("message") or err.get("hint") or err) | ||
| else: | ||
| human = str(err) | ||
| return parsed, human | ||
| except (json.JSONDecodeError, TypeError, ValueError): | ||
| pass | ||
| return None, raw | ||
|
|
||
|
|
||
| def output_error( | ||
| args: Any, | ||
| message: str, | ||
| hint: Optional[str] = None, | ||
| exit_code: int = 1, | ||
| ) -> None: | ||
| """Print an error and exit. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| args: | ||
| The parsed argparse namespace. | ||
| message: | ||
| What went wrong. | ||
| hint: | ||
| Actionable suggestion for the user / agent. | ||
| exit_code: | ||
| Process exit code. Convention: 1 = general, 2 = auth, 3 = not found. | ||
| """ | ||
| parsed, human_message = _parse_error_message(message) | ||
|
|
||
| if getattr(args, "json", False): | ||
| # Normalise error to always be {"error": {"message": "..."}} so | ||
| # consumers see a consistent schema regardless of error source. | ||
| if parsed is not None and "error" in parsed: | ||
| inner: Any = parsed["error"] | ||
| elif parsed is not None: | ||
| inner = parsed | ||
| else: | ||
| inner = None | ||
|
|
||
| if isinstance(inner, dict): | ||
| error_obj: dict[str, Any] = dict(inner) | ||
| error_obj.setdefault("message", human_message) | ||
| else: | ||
| error_obj = {"message": human_message} | ||
|
|
||
| if hint: | ||
| error_obj.setdefault("hint", hint) | ||
| payload: dict[str, Any] = {"error": error_obj} | ||
| print(json.dumps(payload), file=sys.stderr) | ||
|
|
||
| else: | ||
| msg = f"Error: {human_message}" | ||
| if hint: | ||
| msg += f"\n Hint: {hint}" | ||
| print(msg, file=sys.stderr) | ||
|
|
||
| sys.exit(exit_code) | ||
|
|
||
|
|
||
| def stub(args: Any) -> None: | ||
| """Placeholder handler for not-yet-implemented commands.""" | ||
| output_error(args, "This command is not yet implemented.", hint="Coming soon.", exit_code=1) | ||
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def suppress_sdk_output(args: Any = None) -> Iterator[None]: | ||
| """Suppress SDK stdout noise (e.g. 'loading Roboflow workspace...'). | ||
|
|
||
| Always active — the SDK's "loading Roboflow workspace..." messages | ||
| are not useful CLI output in any mode. The CLI controls its own | ||
| output via ``output()`` and ``output_error()``. | ||
| """ | ||
| with contextlib.redirect_stdout(io.StringIO()): | ||
| yield | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.