diff --git a/CLAUDE.md b/CLAUDE.md index e4615ed5..5ac973e6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -690,7 +690,7 @@ kbagent sync status [--directory DIR] kbagent sync diff --project ALIAS [--all-projects] [--directory DIR] [--branch ID] kbagent sync push --project ALIAS [--all-projects] [--dry-run] [--force] [--allow-plaintext-on-encrypt-failure] [--branch ID] [--no-name-drift-warnings] kbagent sync clone --source DIR --target ALIAS --target-dir DIR [--bucket-map FILE] [--variable-values FILE] [--instance-rename FILE] [--dry-run] [--branch ID] -# `sync clone` (0.63.0+) copies a reference synced tree into a fresh target project + parameterizes it: applies bucket_map / variable_values / instance_rename overrides (JSON/YAML files), then pushes so every config CREATEs fresh -- keboola.flow task configIds and transformation variable links are remapped reference->ULID by push Phase C/D. Idempotent: re-run with an existing --target-dir reports no_changes. Fails fast if the target already contains the reference's configs (clone needs a fresh target). +# `sync clone` (0.63.0+) copies a reference synced tree into a fresh target project + parameterizes it: applies bucket_map / variable_values / instance_rename overrides (JSON/YAML files), then pushes so every config CREATEs fresh -- keboola.flow task configIds and transformation variable links are remapped reference->ULID by push Phase C/D. Idempotent: re-run with an existing --target-dir reports no_changes. Fails fast if the target already contains the reference's configs (clone needs a fresh target). Override files must be flat {id: scalar} mappings (0.89.0+): a nested mapping/list/null value is rejected with CONFIG_ERROR naming the key + actual type, instead of being silently stringified into a bogus ID. kbagent sync branch-link --project ALIAS (--branch-id ID | --branch-name NAME) [--directory DIR] kbagent sync branch-unlink [--directory DIR] kbagent sync branch-status [--directory DIR] diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index f69048a0..b9871005 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -3514,6 +3514,12 @@ things to internalise: new/empty target. Re-running clone with the same `--target-dir` is idempotent (`no_changes`), because after the first push the local manifest carries the new ULIDs that match the target remote. +- Override files (`--bucket-map` / `--variable-values` / `--instance-rename`) + must be **flat `{id: scalar}` mappings** (since v0.89.0). A nested value — + one fat-fingered colon away from valid YAML, e.g. `in.c-old:` followed by an + indented `new: in.c-new` — is rejected with `CONFIG_ERROR` (exit 5) naming + the offending key and its actual type. Older versions silently stringified + it (`str(dict)` → `"{'new': 'in.c-new'}"`) and pushed that as a "bucket ID". ### `search --regex` matches entity names only; `matched_columns` is textual-only (since v0.67.0) diff --git a/plugins/kbagent/skills/kbagent/references/sync-workflow.md b/plugins/kbagent/skills/kbagent/references/sync-workflow.md index 82520247..6b31385c 100644 --- a/plugins/kbagent/skills/kbagent/references/sync-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/sync-workflow.md @@ -425,6 +425,11 @@ Override files are JSON or YAML objects: - `--variable-values` → `{ "db_host": "prod-db", "api_base": "https://..." }` — overrides matching `keboola.variables` row values. - `--instance-rename` → `{ "extractor/keboola.ex-db/Acme": "extractor/keboola.ex-db/Globex" }` — renames config dirs + manifest paths. +All three files must be **flat `{id: scalar}` mappings** (since v0.89.0) — a +nested mapping, list, or empty (`null`) value is rejected with `CONFIG_ERROR` +(exit 5) naming the offending key and its actual type, instead of being +silently stringified into a bogus ID. + **Why it just works on a fresh target:** the reference's config ids do not exist in the target project, so the push diff classifies every config as `added` and assigns new ULIDs. Because the push's `created_id_map` is keyed by the reference diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index dda183fc..fc3a0ebc 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -25,6 +25,15 @@ # Ordered newest-first. Each value is a list of brief one-line descriptions. CHANGELOG: dict[str, list[str]] = { "0.89.0": [ + "Fix (#646): `sync clone` now rejects a malformed override file instead of " + "silently pushing garbage IDs. The `--bucket-map` / `--variable-values` / " + "`--instance-rename` loader used to coerce every YAML value with bare " + "`str(value)`, so a nested mapping -- one fat-fingered colon away from valid " + "input -- was used as the literal string `\"{'new': 'in.c-new'}\"` in the " + "target project. A mapping, list, or empty (`null`) value now fails fast with " + "`CONFIG_ERROR` (exit 5) naming the offending key and its actual type in YAML " + "vocabulary (mapping/list/null). Scalar values (string, number, boolean) keep " + "the existing string coercion.", "Fix: `config delete` can no longer permanently purge a configuration by being run " "twice. The Storage API overloads DELETE -- on a live configuration it soft-deletes " "into the trash, but on a configuration ALREADY in the trash the same call purges it " diff --git a/src/keboola_agent_cli/commands/sync.py b/src/keboola_agent_cli/commands/sync.py index a30ec3b6..da31c26d 100644 --- a/src/keboola_agent_cli/commands/sync.py +++ b/src/keboola_agent_cli/commands/sync.py @@ -52,21 +52,13 @@ def _resolve_project_root(directory: Path, alias: str | None = None) -> Path: def _load_override_file(path: Path) -> dict[str, str]: """Load a clone override map (JSON or YAML) as a flat ``{str: str}`` dict. - YAML's loader also parses JSON, so a single path handles both. Every value - is coerced to ``str`` (bucket ids, variable values, and path prefixes are - all strings). + Values must be scalars (bucket ids, variable values, and path prefixes are + all strings); a nested mapping or list raises instead of being silently + stringified into a bogus ID. """ - import yaml + from ..yaml_input import load_flat_scalar_mapping - if not path.exists(): - raise ConfigError(f"Override file not found: {path}") - try: - data = yaml.safe_load(path.read_text(encoding="utf-8")) - except yaml.YAMLError as exc: - raise ConfigError(f"Cannot parse override file {path}: {exc}") from exc - if not isinstance(data, dict): - raise ConfigError(f"Override file {path} must contain a JSON/YAML object (mapping).") - return {str(key): str(value) for key, value in data.items()} + return load_flat_scalar_mapping(path, label="override file") def _change_label(change: dict) -> str: diff --git a/src/keboola_agent_cli/services/_describe_batch_input.py b/src/keboola_agent_cli/services/_describe_batch_input.py index ab96f0e9..ee657526 100644 --- a/src/keboola_agent_cli/services/_describe_batch_input.py +++ b/src/keboola_agent_cli/services/_describe_batch_input.py @@ -39,17 +39,7 @@ from pathlib import Path from typing import Any, NamedTuple -# YAML-flavoured names for the types a section can wrongly be: the author is -# reading their own YAML, not Python, so "string"/"number" beat "str"/"int". -_TYPE_NAMES: dict[type, str] = { - bool: "boolean", - dict: "mapping", - float: "number", - int: "number", - list: "list", - str: "string", - type(None): "null", -} +from ..yaml_input import yaml_type_name class _Section(NamedTuple): @@ -102,15 +92,10 @@ def total(self) -> int: return len(self.buckets) + len(self.tables) + len(self.columns) -def _type_name(value: Any) -> str: - """Return ``value``'s type named in YAML vocabulary.""" - return _TYPE_NAMES.get(type(value), type(value).__name__) - - def _shape_error(key: str, expected: str, value: Any, example: str) -> ValueError: """Build the ValueError for one wrongly-shaped key.""" return ValueError( - f"'{key}' must be {expected}, got a {_type_name(value)}. Expected:\n{example}" + f"'{key}' must be {expected}, got a {yaml_type_name(value)}. Expected:\n{example}" ) diff --git a/src/keboola_agent_cli/yaml_input.py b/src/keboola_agent_cli/yaml_input.py new file mode 100644 index 00000000..1a914dd2 --- /dev/null +++ b/src/keboola_agent_cli/yaml_input.py @@ -0,0 +1,77 @@ +"""Shared shape validation for flat ``{id: scalar}`` YAML/JSON input files. + +Several commands accept a small user-authored mapping file -- ``sync clone``'s +``--bucket-map`` / ``--variable-values`` / ``--instance-rename`` are the +canonical case. Coercing every value with bare ``str(value)`` silently turns a +nested mapping (one fat-fingered colon away from valid input) into the literal +string ``"{'new': 'in.c-new'}"``, which then lands in the target project as a +"bucket ID". This module rejects non-scalar values (and ``None``) up front, +naming the offending key and its actual type -- same approach as the +``storage describe-batch --from-file`` validation in +``services/_describe_batch_input.py``. + +Type names come out in YAML vocabulary ("mapping", "list", "null"), not +Python's -- the author is reading their own YAML file, not a traceback. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +from .errors import ConfigError + +_TYPE_NAMES: dict[type, str] = { + bool: "boolean", + dict: "mapping", + float: "number", + int: "number", + list: "list", + str: "string", + type(None): "null", +} + + +def yaml_type_name(value: Any) -> str: + """Return ``value``'s type named in YAML vocabulary.""" + return _TYPE_NAMES.get(type(value), type(value).__name__) + + +def load_flat_scalar_mapping(path: Path, *, label: str = "input file") -> dict[str, str]: + """Load a JSON/YAML file as a flat ``{str: str}`` mapping, or raise. + + YAML's loader also parses JSON, so a single path handles both. Scalar + values (string, number, boolean) are coerced to ``str``; a container or + ``None`` value raises instead of being stringified. + + Args: + path: The file to load. + label: What the file is, for error messages (e.g. "override file"). + + Raises: + ConfigError: The file is missing, is not valid YAML/JSON, is not a + mapping at the top level, or any value is not a scalar. + """ + import yaml + + if not path.exists(): + raise ConfigError(f"{label.capitalize()} not found: {path}") + try: + data = yaml.safe_load(path.read_text(encoding="utf-8")) + except yaml.YAMLError as exc: + raise ConfigError(f"Cannot parse {label} {path}: {exc}") from exc + if not isinstance(data, dict): + raise ConfigError( + f"{label.capitalize()} {path} must contain a JSON/YAML object (mapping), " + f"got a {yaml_type_name(data)}." + ) + result: dict[str, str] = {} + for key, value in data.items(): + if value is None or isinstance(value, dict | list): + raise ConfigError( + f"'{key}' in {label} {path} must be a single scalar value " + f"(string, number or boolean), got a {yaml_type_name(value)}. " + f"Check the file for a stray colon or missing value." + ) + result[str(key)] = str(value) + return result diff --git a/tests/test_sync_clone.py b/tests/test_sync_clone.py index 217eb340..9cfa9b51 100644 --- a/tests/test_sync_clone.py +++ b/tests/test_sync_clone.py @@ -602,6 +602,43 @@ def test_forwards_args_and_overrides(self, tmp_path: Path) -> None: assert call["overrides"]["bucket_map"] == {"in.c-ref": "in.c-prod"} assert "Cloned into target" in result.output + def test_nested_override_value_errors(self, tmp_path: Path) -> None: + """A non-scalar override value (fat-fingered colon) is rejected, not stringified.""" + from unittest.mock import patch + + from typer.testing import CliRunner + + from keboola_agent_cli.cli import app + + runner = CliRunner() + source = tmp_path / "golden" + _golden_source(source) + bmap = tmp_path / "buckets.yaml" + bmap.write_text("in.c-ref:\n new: in.c-prod\n", encoding="utf-8") + + with patch("keboola_agent_cli.cli.SyncService") as MockSync: + svc = MagicMock() + MockSync.return_value = svc + result = runner.invoke( + app, + [ + "sync", + "clone", + "--source", + str(source), + "--target", + "target", + "--target-dir", + str(tmp_path / "clone"), + "--bucket-map", + str(bmap), + ], + ) + assert result.exit_code == 5 + assert "in.c-ref" in result.output + assert "mapping" in result.output + svc.clone_project.assert_not_called() + def test_missing_override_file_errors(self, tmp_path: Path) -> None: from unittest.mock import patch diff --git a/tests/test_yaml_input.py b/tests/test_yaml_input.py new file mode 100644 index 00000000..41b0f17b --- /dev/null +++ b/tests/test_yaml_input.py @@ -0,0 +1,93 @@ +"""Unit tests for the shared flat {id: scalar} YAML-mapping loader (yaml_input.py). + +The loader backs `sync clone --bucket-map/--variable-values/--instance-rename`. +Before it existed, `commands/sync.py` coerced every value with bare +``str(value)``, so a nested mapping (a fat-fingered colon in the YAML) was +silently used as the literal string ``"{'new': 'in.c-new'}"`` -- a wrong +bucket ID in the target project instead of a rejection. +""" + +from pathlib import Path + +import pytest + +from keboola_agent_cli.errors import ConfigError +from keboola_agent_cli.yaml_input import load_flat_scalar_mapping, yaml_type_name + + +class TestYamlTypeName: + """Type names come out in YAML vocabulary, not Python's.""" + + @pytest.mark.parametrize( + ("value", "expected"), + [ + ({"a": 1}, "mapping"), + (["a"], "list"), + (None, "null"), + ("x", "string"), + (42, "number"), + (4.2, "number"), + (True, "boolean"), + ], + ) + def test_known_types(self, value: object, expected: str) -> None: + assert yaml_type_name(value) == expected + + def test_unknown_type_falls_back_to_python_name(self) -> None: + class Weird: + pass + + assert yaml_type_name(Weird()) == "Weird" + + +class TestLoadFlatScalarMapping: + def _write(self, tmp_path: Path, content: str) -> Path: + path = tmp_path / "map.yaml" + path.write_text(content, encoding="utf-8") + return path + + def test_valid_json(self, tmp_path: Path) -> None: + path = self._write(tmp_path, '{"in.c-old": "in.c-new"}') + assert load_flat_scalar_mapping(path) == {"in.c-old": "in.c-new"} + + def test_valid_yaml_scalars_coerced_to_str(self, tmp_path: Path) -> None: + path = self._write(tmp_path, "retries: 3\nname: prod\n") + assert load_flat_scalar_mapping(path) == {"retries": "3", "name": "prod"} + + def test_nested_mapping_value_rejected(self, tmp_path: Path) -> None: + path = self._write(tmp_path, "in.c-old:\n new: in.c-new\n") + with pytest.raises(ConfigError, match=r"'in\.c-old'.*mapping"): + load_flat_scalar_mapping(path) + + def test_list_value_rejected(self, tmp_path: Path) -> None: + path = self._write(tmp_path, "in.c-old:\n - in.c-new\n") + with pytest.raises(ConfigError, match=r"'in\.c-old'.*list"): + load_flat_scalar_mapping(path) + + def test_null_value_rejected(self, tmp_path: Path) -> None: + path = self._write(tmp_path, "in.c-old:\n") + with pytest.raises(ConfigError, match=r"'in\.c-old'.*null"): + load_flat_scalar_mapping(path) + + def test_top_level_list_rejected(self, tmp_path: Path) -> None: + path = self._write(tmp_path, "- in.c-old\n- in.c-new\n") + with pytest.raises(ConfigError, match="mapping"): + load_flat_scalar_mapping(path) + + def test_empty_file_rejected(self, tmp_path: Path) -> None: + path = self._write(tmp_path, "") + with pytest.raises(ConfigError, match="mapping"): + load_flat_scalar_mapping(path) + + def test_missing_file(self, tmp_path: Path) -> None: + with pytest.raises(ConfigError, match="not found"): + load_flat_scalar_mapping(tmp_path / "nope.yaml") + + def test_invalid_yaml(self, tmp_path: Path) -> None: + path = self._write(tmp_path, "key: [unclosed\n") + with pytest.raises(ConfigError, match=r"[Cc]annot parse"): + load_flat_scalar_mapping(path) + + def test_label_used_in_messages(self, tmp_path: Path) -> None: + with pytest.raises(ConfigError, match=r"[Oo]verride file not found"): + load_flat_scalar_mapping(tmp_path / "nope.yaml", label="override file")