fix(cli): address remaining review items -- URL params, exit codes, tests

- Use requests params dict for api_key instead of embedding in URLs (image.py)
- Replace min(code, 3) with explicit exit code mapping in deployment.py
- Add unit tests for _reorder_argv edge cases (12 tests)
- Add backward-compat alias tests including download regression (11 tests)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: yeldarby

roboflow/cli/handlers/deployment.py

@@ -32,9 +32,11 @@ def _wrapped(args: argparse.Namespace) -> None:
         except SystemExit as exc:
             sys.stdout = orig_stdout
             code = exc.code if isinstance(exc.code, int) else 1
+            # Map legacy exit codes to CLI conventions: 1=general, 2=auth, 3=not-found
+            exit_code = {0: 1, 1: 1, 2: 2, 3: 3}.get(code, 1) if code else 1
             text = captured.getvalue().strip()
             if text:
-                output_error(args, text, exit_code=min(code, 3) if code else 1)
+                output_error(args, text, exit_code=exit_code)
             else:
                 output_error(args, "Deployment command failed.", exit_code=1)
             return

roboflow/cli/handlers/image.py

@@ -174,8 +174,8 @@ def _handle_get(args: argparse.Namespace) -> None:
         output_error(args, "No workspace specified", hint="Use --workspace or run 'roboflow auth login'")
         return
 
-    url = f"{API_URL}/{workspace_url}/{args.project}/images/{args.image_id}?api_key={api_key}"
-    response = requests.get(url)
+    url = f"{API_URL}/{workspace_url}/{args.project}/images/{args.image_id}"
+    response = requests.get(url, params={"api_key": api_key})
     if response.status_code != 200:
         output_error(args, f"Failed to get image: {response.text}", exit_code=3)
         return
@@ -259,7 +259,7 @@ def _handle_tag(args: argparse.Namespace) -> None:
             tag = tag.strip()
             if not tag:
                 continue
-            resp = requests.post(f"{base}?api_key={api_key}", json={"tag": tag})
+            resp = requests.post(base, params={"api_key": api_key}, json={"tag": tag})
             if resp.status_code == 200:
                 added.append(tag)
 
@@ -268,7 +268,7 @@ def _handle_tag(args: argparse.Namespace) -> None:
             tag = tag.strip()
             if not tag:
                 continue
-            resp = requests.delete(f"{base}/{tag}?api_key={api_key}")
+            resp = requests.delete(f"{base}/{tag}", params={"api_key": api_key})
             if resp.status_code == 200:
                 removed.append(tag)
 

tests/cli/test_aliases.py

@@ -0,0 +1,68 @@
+"""Tests for backward-compatibility aliases in _aliases.py."""
+
+import unittest
+
+
+class TestAliases(unittest.TestCase):
+    """Verify top-level aliases parse correctly and delegate to the right handler."""
+
+    def _parse(self, argv: list[str]):
+        from roboflow.cli import build_parser
+
+        parser = build_parser()
+        return parser.parse_args(argv)
+
+    def test_login_alias_exists(self) -> None:
+        args = self._parse(["login"])
+        self.assertIsNotNone(args.func)
+
+    def test_login_alias_with_api_key(self) -> None:
+        args = self._parse(["login", "--api-key", "test-key"])
+        self.assertEqual(args.api_key_flag, "test-key")
+
+    def test_whoami_alias_exists(self) -> None:
+        args = self._parse(["whoami"])
+        self.assertIsNotNone(args.func)
+
+    def test_upload_alias_exists(self) -> None:
+        args = self._parse(["upload", "img.jpg", "-p", "my-project"])
+        self.assertIsNotNone(args.func)
+        self.assertEqual(args.path, "img.jpg")
+        self.assertEqual(args.project, "my-project")
+
+    def test_import_alias_exists(self) -> None:
+        args = self._parse(["import", "/data/images", "-p", "my-project"])
+        self.assertIsNotNone(args.func)
+        self.assertEqual(args.path, "/data/images")
+        self.assertEqual(args.project, "my-project")
+
+    def test_download_alias_parses_url(self) -> None:
+        """Regression: download alias must use url_or_id as dest, not datasetUrl."""
+        args = self._parse(["download", "my-ws/my-proj/3"])
+        self.assertIsNotNone(args.func)
+        # The critical check: args.url_or_id must exist (not args.datasetUrl)
+        self.assertEqual(args.url_or_id, "my-ws/my-proj/3")
+
+    def test_download_alias_with_format(self) -> None:
+        args = self._parse(["download", "my-ws/my-proj/3", "-f", "yolov8"])
+        self.assertEqual(args.format, "yolov8")
+
+    def test_download_alias_with_location(self) -> None:
+        args = self._parse(["download", "my-ws/my-proj/3", "-l", "/tmp/out"])
+        self.assertEqual(args.location, "/tmp/out")
+
+    def test_download_alias_delegates_to_version_download(self) -> None:
+        """The download alias should use the same handler as 'version download'."""
+        from roboflow.cli.handlers.version import _download
+
+        args = self._parse(["download", "my-ws/my-proj/3"])
+        self.assertIs(args.func, _download)
+
+    def test_upload_model_alias_hidden(self) -> None:
+        """upload_model is a hidden alias — it should still parse."""
+        args = self._parse(["upload_model", "-p", "my-proj", "-t", "yolov8", "-m", "/weights"])
+        self.assertIsNotNone(args.func)
+
+
+if __name__ == "__main__":
+    unittest.main()

tests/cli/test_reorder_argv.py

@@ -0,0 +1,68 @@
+"""Tests for _reorder_argv — global flag reordering."""
+
+import unittest
+
+
+class TestReorderArgv(unittest.TestCase):
+    """Verify _reorder_argv moves global flags before subcommands."""
+
+    def _reorder(self, argv: list[str]) -> list[str]:
+        from roboflow.cli import _reorder_argv
+
+        return _reorder_argv(argv)
+
+    def test_no_flags(self) -> None:
+        self.assertEqual(self._reorder(["project", "list"]), ["project", "list"])
+
+    def test_empty(self) -> None:
+        self.assertEqual(self._reorder([]), [])
+
+    def test_bool_flag_after_subcommand(self) -> None:
+        result = self._reorder(["project", "list", "--json"])
+        self.assertEqual(result, ["--json", "project", "list"])
+
+    def test_bool_flag_already_first(self) -> None:
+        result = self._reorder(["--json", "project", "list"])
+        self.assertEqual(result, ["--json", "project", "list"])
+
+    def test_short_bool_flag(self) -> None:
+        result = self._reorder(["project", "list", "-j"])
+        self.assertEqual(result, ["-j", "project", "list"])
+
+    def test_value_flag_after_subcommand(self) -> None:
+        result = self._reorder(["project", "list", "--api-key", "abc123"])
+        self.assertEqual(result, ["--api-key", "abc123", "project", "list"])
+
+    def test_short_value_flag(self) -> None:
+        result = self._reorder(["project", "list", "-k", "abc123"])
+        self.assertEqual(result, ["-k", "abc123", "project", "list"])
+
+    def test_multiple_flags_mixed(self) -> None:
+        result = self._reorder(["project", "list", "--json", "-w", "my-ws"])
+        self.assertEqual(result, ["--json", "-w", "my-ws", "project", "list"])
+
+    def test_value_flag_at_end_without_value(self) -> None:
+        """A value flag at the very end with no following arg should still be moved."""
+        result = self._reorder(["project", "list", "--api-key"])
+        self.assertEqual(result, ["--api-key", "project", "list"])
+
+    def test_non_global_flags_preserved(self) -> None:
+        """Flags not in the global set stay in place."""
+        result = self._reorder(["image", "upload", "--project", "my-proj", "--json"])
+        self.assertEqual(result, ["--json", "image", "upload", "--project", "my-proj"])
+
+    def test_quiet_and_version_flags(self) -> None:
+        result = self._reorder(["project", "list", "--quiet", "--version"])
+        self.assertEqual(result, ["--quiet", "--version", "project", "list"])
+
+    def test_workspace_flag(self) -> None:
+        result = self._reorder(["project", "list", "--workspace", "ws-1"])
+        self.assertEqual(result, ["--workspace", "ws-1", "project", "list"])
+
+    def test_preserves_subcommand_positional_args(self) -> None:
+        result = self._reorder(["version", "download", "ws/proj/3", "--json", "-f", "yolov8"])
+        self.assertEqual(result, ["--json", "version", "download", "ws/proj/3", "-f", "yolov8"])
+
+
+if __name__ == "__main__":
+    unittest.main()