diff --git a/app/commands/review/entrypoint.py b/app/commands/review/entrypoint.py index 4f37b1c..7bc3315 100644 --- a/app/commands/review/entrypoint.py +++ b/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 diff --git a/cli.py b/cli.py index 3c62b9f..38278ba 100644 --- a/cli.py +++ b/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,6 +156,21 @@ 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) @@ -151,6 +178,42 @@ def main() -> None: 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__": diff --git a/core/config.py b/core/config.py index 7d7162b..35606eb 100644 --- a/core/config.py +++ b/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) diff --git a/tests/commands/test_transitive_modules.py b/tests/commands/test_transitive_modules.py index 999be35..33f091b 100644 --- a/tests/commands/test_transitive_modules.py +++ b/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,15 +660,42 @@ 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" @@ -672,9 +703,11 @@ def test_exits_when_no_lang(self, _mock_colorize, mock_runtime, mock_resolve_lan 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 ──────────────────────────────────────────────────────