fix(cli): implement CLI robustness, error handling and path sandboxing

app/commands/review/entrypoint.py

@@ -16,6 +16,15 @@
 from .prepare import do_prepare
 
 
+class ReviewValidationError(ValueError):
+    """Raised when review command flag validation fails.
+
+    Caught by the top-level CLI handler to produce a clean error message
+    without a traceback. This replaces scattered sys.exit() calls to
+    keep exit handling consistent and the function testable.
+    """
+
+
 def _enable_live_review_output() -> None:
     """Best-effort: force line-buffered review output for non-TTY runners."""
     for stream_name in ("stdout", "stderr"):
@@ -29,21 +38,12 @@ def _enable_live_review_output() -> None:
             continue
 
 
-def cmd_review(args: argparse.Namespace) -> None:
-    """Prepare or import subjective code review findings."""
-    _enable_live_review_output()
-    runtime = command_runtime(args)
-    state_file = runtime.state_path
-    state = runtime.state
-    lang = resolve_lang(args)
-
-    if not lang:
-        print(
-            colorize("  Error: could not detect language. Use --lang.", "red"),
-            file=sys.stderr,
-        )
-        sys.exit(1)
+def _validate_review_flags(args) -> None:
+    """Validate mutually-exclusive review mode flags up front.
 
+    Raises ReviewValidationError on invalid flag combinations so that
+    all validation happens before any work is done.
+    """
     merge = bool(getattr(args, "merge", False))
     run_batches = bool(getattr(args, "run_batches", False))
     external_start = bool(getattr(args, "external_start", False))
@@ -61,35 +61,44 @@ def cmd_review(args: argparse.Namespace) -> None:
         bool(validate_import_file),
     ]
     if sum(1 for enabled in mode_flags if enabled) > 1:
-        print(
-            colorize(
-                "  Error: choose one review mode per command "
-                "(--merge | --run-batches | --external-start | --external-submit | --import | --validate-import).",
-                "red",
-            ),
-            file=sys.stderr,
+        raise ReviewValidationError(
+            "choose one review mode per command "
+            "(--merge | --run-batches | --external-start | --external-submit | --import | --validate-import)."
         )
-        sys.exit(1)
 
     if external_submit and not import_file:
-        print(
-            colorize(
-                "  Error: --external-submit requires --import FILE.",
-                "red",
-            ),
-            file=sys.stderr,
+        raise ReviewValidationError(
+            "--external-submit requires --import FILE."
         )
-        sys.exit(2)
 
     if external_submit and not getattr(args, "session_id", None):
-        print(
-            colorize(
-                "  Error: --external-submit requires --session-id.",
-                "red",
-            ),
-            file=sys.stderr,
+        raise ReviewValidationError(
+            "--external-submit requires --session-id."
         )
-        sys.exit(2)
+
+
+def cmd_review(args: argparse.Namespace) -> None:
+    """Prepare or import subjective code review findings."""
+    _enable_live_review_output()
+    runtime = command_runtime(args)
+    state_file = runtime.state_path
+    state = runtime.state
+    lang = resolve_lang(args)
+
+    if not lang:
+        raise ReviewValidationError(
+            "could not detect language. Use --lang."
+        )
+
+    # Validate all flags before doing any work.
+    _validate_review_flags(args)
+
+    merge = bool(getattr(args, "merge", False))
+    run_batches = bool(getattr(args, "run_batches", False))
+    external_start = bool(getattr(args, "external_start", False))
+    external_submit = bool(getattr(args, "external_submit", False))
+    import_file = getattr(args, "import_file", None)
+    validate_import_file = getattr(args, "validate_import_file", None)
 
     if merge:
         from app.commands.review.merge import do_merge

cli.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import json
 import logging
 import sys
 from pathlib import Path
@@ -83,8 +84,16 @@ def _resolve_default_path(args) -> None:
                 saved = load_state(state_file)
                 saved_path = saved.get("scan_path")
                 if saved_path:
-                    args.path = str((runtime_root / saved_path).resolve())
-                    return
+                    # Normalize and sandbox: resolve the path and verify it
+                    # stays within the project root to prevent traversal.
+                    candidate = (runtime_root / saved_path).resolve()
+                    if candidate == runtime_root or runtime_root in candidate.parents:
+                        args.path = str(candidate)
+                        return
+                    logger.warning(
+                        "Persisted scan_path %r resolves outside project root; ignoring.",
+                        saved_path,
+                    )
         except (OSError, KeyError, ValueError, TypeError, AttributeError) as exc:
             logger.debug("Failed to resolve default path from saved state: %s", exc)
     lang = resolve_lang(args)
@@ -105,10 +114,13 @@ def _load_shared_runtime(args) -> None:
     args.runtime = CommandRuntime(config=config, state=state, state_path=state_file)
 
 
-def _resolve_handler(command: str):
+def _resolve_handler(command: str | None):
+    """Resolve a CLI command to its handler, or return None for unknown commands."""
+    if not command:
+        return None
     from app.commands.registry import get_command_handlers
 
-    return get_command_handlers()[command]
+    return get_command_handlers().get(command)
 
 
 def _handle_help_command(args, parser) -> None:
@@ -144,13 +156,64 @@ def main() -> None:
             _load_shared_runtime(args)
 
             handler = _resolve_handler(args.command)
+            if handler is None:
+                available = ", ".join(sorted(
+                    __import__("app.commands.registry", fromlist=["get_command_handlers"])
+                    .get_command_handlers()
+                    .keys()
+                ))
+                print(
+                    colorize(
+                        f"  Unknown command: {args.command!r}. "
+                        f"Available commands: {available}",
+                        "red",
+                    ),
+                    file=sys.stderr,
+                )
+                sys.exit(1)
             handler(args)
     except LangResolutionError as exc:
         print(colorize(f"  {exc.message}", "red"), file=sys.stderr)
         sys.exit(1)
     except KeyboardInterrupt:
         print("\nInterrupted.")
         sys.exit(1)
+    except SystemExit:
+        raise
+    except ValueError as exc:
+        # Covers ReviewValidationError and other validation errors from
+        # command handlers — clean user-facing message without traceback.
+        print(colorize(f"  Error: {exc}", "red"), file=sys.stderr)
+        sys.exit(1)
+    except (OSError, json.JSONDecodeError) as exc:
+        # Config/state file I/O or parse errors.
+        print(
+            colorize(f"  Error loading project data: {exc}", "red"),
+            file=sys.stderr,
+        )
+        sys.exit(1)
+    except ImportError as exc:
+        # Command module or plugin failed to load.
+        print(
+            colorize(
+                f"  Error loading command module: {exc}. "
+                f"Try reinstalling structorium.",
+                "red",
+            ),
+            file=sys.stderr,
+        )
+        sys.exit(1)
+    except Exception as exc:
+        # Last-resort catch: prevents raw tracebacks reaching the user.
+        logger.debug("Unhandled error in CLI dispatch", exc_info=True)
+        print(
+            colorize(
+                f"  Unexpected error: {type(exc).__name__}: {exc}",
+                "red",
+            ),
+            file=sys.stderr,
+        )
+        sys.exit(1)
 
 
 if __name__ == "__main__":

core/config.py

@@ -221,9 +221,21 @@ def load_config(path: Path | None = None) -> dict[str, Any]:
     p = path or CONFIG_FILE
     if p.exists():
         try:
-            config = json.loads(p.read_text())
+            raw = json.loads(p.read_text())
         except (json.JSONDecodeError, UnicodeDecodeError, OSError):
+            raw = {}
+        # Guard: config must be a dict — malformed payloads (list, string, etc.)
+        # would cause TypeError downstream when callers use .get() / .items().
+        if not isinstance(raw, dict):
+            logger.warning(
+                "Config file %s contains a non-object top-level value (%s); "
+                "falling back to defaults.",
+                p,
+                type(raw).__name__,
+            )
             config = {}
+        else:
+            config = raw
     else:
         # First run — try migrating from state files
         config = _migrate_from_state_files(p)

tests/commands/test_transitive_modules.py

@@ -436,7 +436,7 @@ def test_module_all(self):
 
 # ── 5. review/entrypoint.py ─────────────────────────────────────────────────
 
-from app.commands.review.entrypoint import cmd_review  # noqa: E402
+from app.commands.review.entrypoint import ReviewValidationError, cmd_review  # noqa: E402
 
 
 class TestCmdReviewEntrypoint:
@@ -559,9 +559,11 @@ def test_run_batches_rejects_conflicting_import_modes(
             session_id=None,
         )
 
-        with pytest.raises(SystemExit) as exc_info:
+        with pytest.raises(
+            ReviewValidationError,
+            match=r"choose one review mode per command",
+        ):
             cmd_review(args)
-        assert exc_info.value.code == 1
 
     @patch("app.commands.review.entrypoint.resolve_lang")
     @patch("app.commands.review.entrypoint.command_runtime")
@@ -582,9 +584,11 @@ def test_import_and_validate_reject_together(self, mock_runtime, mock_resolve_la
             session_id=None,
         )
 
-        with pytest.raises(SystemExit) as exc_info:
+        with pytest.raises(
+            ReviewValidationError,
+            match=r"choose one review mode per command",
+        ):
             cmd_review(args)
-        assert exc_info.value.code == 1
 
     @patch("app.commands.review.entrypoint.do_external_start")
     @patch("app.commands.review.entrypoint.resolve_lang")
@@ -638,7 +642,7 @@ def test_external_submit_path(self, mock_runtime, mock_resolve_lang, mock_extern
 
     @patch("app.commands.review.entrypoint.resolve_lang")
     @patch("app.commands.review.entrypoint.command_runtime")
-    def test_external_submit_requires_import_and_session(
+    def test_external_submit_requires_import_file(
         self, mock_runtime, mock_resolve_lang
     ):
         rt = MagicMock()
@@ -656,25 +660,54 @@ def test_external_submit_requires_import_and_session(
             session_id=None,
         )
 
-        with pytest.raises(SystemExit) as exc_info:
+        with pytest.raises(
+            ReviewValidationError,
+            match=r"--external-submit requires --import FILE\.",
+        ):
+            cmd_review(args)
+
+    @patch("app.commands.review.entrypoint.resolve_lang")
+    @patch("app.commands.review.entrypoint.command_runtime")
+    def test_external_submit_requires_session_id(
+        self, mock_runtime, mock_resolve_lang
+    ):
+        rt = MagicMock()
+        rt.state = {"findings": {}}
+        rt.state_path = "/tmp/state.json"
+        rt.config = {}
+        mock_runtime.return_value = rt
+        mock_resolve_lang.return_value = MagicMock(name="python")
+        args = argparse.Namespace(
+            run_batches=False,
+            import_file="/tmp/review.json",
+            validate_import_file=None,
+            external_start=False,
+            external_submit=True,
+            session_id=None,
+        )
+
+        with pytest.raises(
+            ReviewValidationError,
+            match=r"--external-submit requires --session-id\.",
+        ):
             cmd_review(args)
-        assert exc_info.value.code == 2
 
     @patch("app.commands.review.entrypoint.resolve_lang", return_value=None)
     @patch("app.commands.review.entrypoint.command_runtime")
-    @patch("app.commands.review.entrypoint.colorize", side_effect=lambda t, _c: t)
-    def test_exits_when_no_lang(self, _mock_colorize, mock_runtime, mock_resolve_lang):
-        """When resolve_lang returns None, prints error and exits."""
+    def test_raises_when_no_lang(self, mock_runtime, _mock_resolve_lang):
+        """When resolve_lang returns None, raises a validation error."""
         rt = MagicMock()
         rt.state = {"findings": {}}
         rt.state_path = "/tmp/state.json"
         rt.config = {}
         mock_runtime.return_value = rt
         args = argparse.Namespace()
 
-        with pytest.raises(SystemExit) as exc_info:
+        with pytest.raises(
+            ReviewValidationError,
+            match=r"could not detect language\. Use --lang\.",
+        ):
             cmd_review(args)
-        assert exc_info.value.code == 1
 
 
 # ── 6. update_skill.py ──────────────────────────────────────────────────────