diff --git a/structkit/commands/generate.py b/structkit/commands/generate.py index 6f8c731..00729fe 100644 --- a/structkit/commands/generate.py +++ b/structkit/commands/generate.py @@ -10,6 +10,10 @@ import subprocess +class GenerateConfigError(ValueError): + """Expected generate config load/shape error shown without traceback.""" + + # Generate command class class GenerateCommand(Command): def __init__(self, parser): @@ -111,8 +115,7 @@ def _load_yaml_config(self, structure_definition, structures_path): structure_definition = f"file://{structure_definition}" if structure_definition.startswith("file://") and structure_definition.endswith(".yaml"): - with open(structure_definition[7:], 'r') as f: - return yaml.safe_load(f) + file_path = structure_definition[7:] else: this_file = os.path.dirname(os.path.realpath(__file__)) contribs_path = os.path.join(this_file, "..", "contribs") @@ -121,11 +124,23 @@ def _load_yaml_config(self, structure_definition, structures_path): file_path = os.path.join(structures_path, f"{structure_definition}.yaml") if not os.path.exists(file_path): file_path = os.path.join(contribs_path, f"{structure_definition}.yaml") - if not os.path.exists(file_path): - self.logger.error(f"❗ File not found: {file_path}") - return None + + try: with open(file_path, 'r') as f: return yaml.safe_load(f) + except FileNotFoundError: + raise GenerateConfigError(f"File not found: {file_path}") from None + except yaml.YAMLError as exc: + raise GenerateConfigError(f"Invalid YAML in {file_path}: {exc}") from None + except OSError as exc: + raise GenerateConfigError(f"Failed to read {file_path}: {exc}") from None + + def _validate_loaded_config(self, config): + if config is None: + return {} + if not isinstance(config, dict): + raise GenerateConfigError("Top-level YAML content must be a mapping.") + return config def execute(self, args): try: @@ -164,6 +179,16 @@ def execute(self, args): self.logger.error(f"Mappings file not found: {mappings_file_path}") return + # Load and validate config before creating output/backup paths, so config + # errors do not leave partial filesystem side effects behind. + try: + config = self._validate_loaded_config( + self._load_yaml_config(args.structure_definition, args.structures_path) + ) + except GenerateConfigError as exc: + self.logger.error(f"❗ {exc}") + raise SystemExit(1) from None + if args.backup and not os.path.exists(args.backup): os.makedirs(args.backup) @@ -171,12 +196,6 @@ def execute(self, args): self.logger.info(f"Creating base path: {args.base_path}") os.makedirs(args.base_path) - # Load config to check for hooks - config = None - config = self._load_yaml_config(args.structure_definition, args.structures_path) - if config is None: - return - pre_hooks = config.get('pre_hooks', []) post_hooks = config.get('post_hooks', []) @@ -188,7 +207,7 @@ def execute(self, args): # Actually generate structure try: self._create_structure(args, mappings) - except TemplateVariableError as exc: + except (GenerateConfigError, TemplateVariableError) as exc: self.logger.error(f"❗ {exc}") raise SystemExit(1) from None @@ -201,9 +220,9 @@ def _create_structure(self, args, mappings=None, summary=None, print_summary=Tru if isinstance(args, dict): args = argparse.Namespace(**args) - config = self._load_yaml_config(args.structure_definition, args.structures_path) - if config is None: - return summary if summary is not None else None + config = self._validate_loaded_config( + self._load_yaml_config(args.structure_definition, args.structures_path) + ) # Safely parse template variables template_vars = self._parse_template_vars(args.vars) if getattr(args, 'vars', None) else {} diff --git a/structkit/mcp_server.py b/structkit/mcp_server.py index 62e0240..4d1ab31 100644 --- a/structkit/mcp_server.py +++ b/structkit/mcp_server.py @@ -178,13 +178,19 @@ class Args: GenerateCommand(dummy_parser).execute(args) text = buf.getvalue() return text.strip() or "Structure generation completed successfully" + except SystemExit as exc: + text = buf.getvalue().strip() + return text or f"Error: structure generation failed with exit code {exc.code}" finally: sys.stdout = old else: # Create a dummy parser for GenerateCommand import argparse dummy_parser = argparse.ArgumentParser() - GenerateCommand(dummy_parser).execute(args) + try: + GenerateCommand(dummy_parser).execute(args) + except SystemExit as exc: + return f"Error: structure generation failed with exit code {exc.code}" if dry_run: return f"Dry run completed for structure '{structure_definition}' at '{base_path}'" return f"Structure '{structure_definition}' generated successfully at '{base_path}'" diff --git a/tests/test_commands_more.py b/tests/test_commands_more.py index 824926d..2109e0b 100644 --- a/tests/test_commands_more.py +++ b/tests/test_commands_more.py @@ -134,6 +134,53 @@ def test_generate_reports_template_variable_error_without_traceback(parser, tmp_ assert "Traceback" not in caplog.text +def test_generate_invalid_yaml_reports_clean_error_without_side_effect(parser, tmp_path, caplog): + command = GenerateCommand(parser) + invalid = tmp_path / 'invalid.yaml' + invalid.write_text('files: [\n') + out_dir = tmp_path / 'out-invalid' + args = parser.parse_args(['--non-interactive', str(invalid), str(out_dir)]) + + with pytest.raises(SystemExit) as excinfo: + command.execute(args) + + assert excinfo.value.code == 1 + assert f"Invalid YAML in {invalid}" in caplog.text + assert "Traceback" not in caplog.text + assert not out_dir.exists() + + +def test_generate_top_level_non_mapping_reports_clean_error_without_side_effect(parser, tmp_path, caplog): + command = GenerateCommand(parser) + non_mapping = tmp_path / 'list.yaml' + non_mapping.write_text('- item\n') + out_dir = tmp_path / 'out-list' + args = parser.parse_args(['--non-interactive', str(non_mapping), str(out_dir)]) + + with pytest.raises(SystemExit) as excinfo: + command.execute(args) + + assert excinfo.value.code == 1 + assert "Top-level YAML content must be a mapping." in caplog.text + assert "Traceback" not in caplog.text + assert not out_dir.exists() + + +def test_generate_missing_file_reports_clean_error_without_side_effect(parser, tmp_path, caplog): + command = GenerateCommand(parser) + missing = tmp_path / 'missing.yaml' + out_dir = tmp_path / 'out-missing' + args = parser.parse_args(['--non-interactive', str(missing), str(out_dir)]) + + with pytest.raises(SystemExit) as excinfo: + command.execute(args) + + assert excinfo.value.code == 1 + assert f"File not found: {missing}" in caplog.text + assert "Traceback" not in caplog.text + assert not out_dir.exists() + + def test_info_nonexistent_file_logs_error(parser): command = InfoCommand(parser) args = parser.parse_args(['does-not-exist'])