From be8704f1f593695a41310998c60ef80301903830 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 25 Aug 2026 13:51:05 +0200 Subject: [PATCH 1/5] fix(sync): one canonical script[] shape + statement-boundary markers Two functions held contradictory definitions of parameters.blocks[].codes[].script: config_format._normalize_scripts (pull + the remote side of diff) collapsed each code into exactly one joined string, while code_extraction._lines_to_script (push + the local side of diff) split SQL on statement boundaries. They can never agree for a multi-statement SQL transformation, which is the permanent "~ REMOTE MODIFIED ... codes changed" phantom drift of issue #686. The runtime already picked the canonical shape -- one array element is one executable statement (#119/#120/#274) -- so _normalize_scripts is now component-aware and splits per element via the shared canonical_sql_script() producer; non-SQL components keep the join. transform.sql also carries an explicit /* ===== STATEMENT ===== */ marker when (and only when) semicolons alone cannot recover the canonical array. Without it, API elements with no trailing semicolon lost their boundaries and push silently rewrote production into the MULTI_STATEMENT_COUNT=1 crash shape. Emission is conditional, so ;-terminated trees stay byte-identical; markers are guaranteed boundaries but split_statements still runs within each segment. --- src/keboola_agent_cli/sync/code_extraction.py | 119 +++++++++++++++-- src/keboola_agent_cli/sync/config_format.py | 120 +++++++++++++----- tests/test_sync_code_extraction.py | 99 +++++++++++++++ tests/test_sync_config_format.py | 94 ++++++++++++-- 4 files changed, 383 insertions(+), 49 deletions(-) diff --git a/src/keboola_agent_cli/sync/code_extraction.py b/src/keboola_agent_cli/sync/code_extraction.py index 33742a1a..f333e92a 100644 --- a/src/keboola_agent_cli/sync/code_extraction.py +++ b/src/keboola_agent_cli/sync/code_extraction.py @@ -6,11 +6,29 @@ from __future__ import annotations +import logging from pathlib import Path from typing import Any, cast from keboola_agent_cli.sync.sql_split import split_statements +logger = logging.getLogger(__name__) + +# Explicit statement boundary written into ``transform.sql`` when semicolons +# alone cannot recover the canonical ``script[]`` array -- i.e. when the API's +# elements carry no trailing ``;`` (issue #686 part 3). Without it, +# ``merge_code_files`` collapses several statements into one element and +# ``sync push`` silently rewrites production into the +# ``MULTI_STATEMENT_COUNT=1`` crash shape of issues #119/#120/#274. +# +# Emission is CONDITIONAL: a ``;``-terminated script round-trips on its own, so +# existing trees stay byte-identical. Recognition is an exact full-line match +# after stripping. Auto-appending the missing ``;`` was rejected as the +# alternative: it changes content, and Oracle (ODBC) rejects a trailing +# semicolon outright (ORA-00911). The marker never changes content and is +# backend-neutral. +SQL_STATEMENT_MARKER = "/* ===== STATEMENT ===== */" + def _strip_trailing_empty(lines: list[str]) -> list[str]: """Remove trailing empty lines but preserve leading whitespace.""" @@ -20,16 +38,59 @@ def _strip_trailing_empty(lines: list[str]) -> list[str]: return result +def canonical_sql_script(script: list[Any]) -> list[str]: + """Return the canonical ``script[]`` array for a SQL transformation code. + + ONE array element = ONE executable statement (the Keboola runtime's own + semantics -- the premise of issues #119/#120/#274). Each existing element + is split independently and the results are flattened; the array is NEVER + joined first, so two elements without trailing semicolons stay two + statements instead of silently merging into one (issue #686). + + This is the single producer of that shape: the API side + (``config_format._normalize_scripts``) and the file side + (:func:`_lines_to_script`) both agree with it, which is what makes the + stored ``pull_config_hash`` comparable across pull, push and diff. + """ + canonical: list[str] = [] + for element in script: + if isinstance(element, str): + canonical.extend(split_statements(element)) + elif element is not None: + canonical.append(element) + return canonical + + +def _split_on_statement_markers(lines: list[str]) -> list[list[str]]: + """Split collected code lines into segments on marker lines.""" + segments: list[list[str]] = [[]] + for line in lines: + if line.strip() == SQL_STATEMENT_MARKER: + segments.append([]) + continue + segments[-1].append(line) + return segments + + def _lines_to_script(lines: list[str], *, is_sql: bool = False) -> list[str]: """Convert collected lines back into the ``script[]`` array. For SQL transformations: splits on semicolons using a state machine, producing one element per statement (matching Keboola runtime semantics). + :data:`SQL_STATEMENT_MARKER` lines, when present, are GUARANTEED + boundaries -- but :func:`split_statements` still runs *within* each + segment, so a user who types ``; SELECT ...`` inside a marked segment + still gets it split correctly. For Python/other: joins all lines into a single element. """ stripped = _strip_trailing_empty(lines) if not stripped: return [] + if is_sql and any(line.strip() == SQL_STATEMENT_MARKER for line in stripped): + script: list[str] = [] + for segment in _split_on_statement_markers(stripped): + script.extend(split_statements("\n".join(segment))) + return script content = "\n".join(stripped) if is_sql: return split_statements(content) @@ -311,6 +372,54 @@ def merge_code_files( # ---- SQL Transformations ---- +def _render_sql_script_lines(scripts: list[Any], *, with_markers: bool) -> list[str]: + """Render one code's ``script[]`` as ``transform.sql`` lines.""" + lines: list[str] = [] + for si, script in enumerate(scripts): + if si > 0: + lines.append("") # blank line between statements + if with_markers: + lines.append(SQL_STATEMENT_MARKER) + lines.append("") + if isinstance(script, str) and "\n" in script: + lines.extend(script.split("\n")) + else: + lines.append(script) + return lines + + +def _render_sql_code(scripts: list[Any], code_name: str) -> list[str]: + """Render one code block, adding statement markers only when needed. + + Markers are emitted only when the plain rendering cannot be parsed back + into the canonical statement array (:func:`canonical_sql_script`) -- i.e. + when the elements carry no trailing semicolons. The ``;``-terminated case + (the overwhelming majority) renders exactly as before, so existing trees + are byte-stable and produce no spurious diff. + + Collision guard: if a statement's own text already contains a + marker-identical line, emitting boundaries would make the file ambiguous. + Markers are then suppressed for that code and a warning is logged -- the + round-trip degrades to the pre-#686 semicolon-only behaviour for it. + """ + plain = _render_sql_script_lines(scripts, with_markers=False) + canonical = canonical_sql_script(scripts) + if _lines_to_script(plain, is_sql=True) == canonical: + return plain + if any( + isinstance(s, str) and any(line.strip() == SQL_STATEMENT_MARKER for line in s.split("\n")) + for s in scripts + ): + logger.warning( + "Code %r contains a line identical to the statement-boundary marker; " + "writing transform.sql without boundary markers. Statements without a " + "trailing semicolon may be merged on the next push.", + code_name, + ) + return plain + return _render_sql_script_lines(scripts, with_markers=True) + + def _extract_sql_transformation(config_data: dict[str, Any], config_dir: Path) -> dict[str, Any]: """Extract SQL blocks from parameters.blocks into transform.sql.""" parameters = config_data.get("parameters") or {} @@ -330,15 +439,7 @@ def _extract_sql_transformation(config_data: dict[str, Any], config_dir: Path) - for code in block.get("codes", []): code_name = code.get("name", "unnamed") lines.append(SQL_CODE_MARKER.format(name=code_name)) - - scripts = code.get("script") or [] - for si, script in enumerate(scripts): - if si > 0: - lines.append("") # blank line between statements - if isinstance(script, str) and "\n" in script: - lines.extend(script.split("\n")) - else: - lines.append(script) + lines.extend(_render_sql_code(code.get("script") or [], code_name)) lines.append("") sql_content = "\n".join(lines).rstrip() + "\n" diff --git a/src/keboola_agent_cli/sync/config_format.py b/src/keboola_agent_cli/sync/config_format.py index e71c36fc..c44c95a3 100644 --- a/src/keboola_agent_cli/sync/config_format.py +++ b/src/keboola_agent_cli/sync/config_format.py @@ -13,46 +13,91 @@ import yaml from ..constants import CONFIG_YML_VERSION +from .code_extraction import canonical_sql_script, is_sql_transformation_component -def _normalize_scripts(parameters: Any) -> Any: +def _iter_codes(parameters: dict[str, Any]) -> Any: + """Yield every ``blocks[].codes[]`` dict in a transformation's parameters.""" + for block in parameters.get("blocks", []): + if not isinstance(block, dict): + continue + for code in block.get("codes", []): + if isinstance(code, dict): + yield code + + +def _join_script(scripts: list[Any]) -> list[str]: + """Collapse a code's ``script[]`` into a single joined string. + + The non-SQL shape: Python / R / custom-app components carry ONE code + body per code block, and the push side (``_lines_to_script`` without + ``is_sql``) produces exactly one element, so both sides agree. + """ + all_lines: list[str] = [] + for s in scripts: + if isinstance(s, str) and "\n" in s: + all_lines.extend(s.split("\n")) + else: + all_lines.append(s) + # Strip trailing whitespace per line (YAML roundtrip strips it) and + # remove trailing empty lines. + all_lines = [line.rstrip() for line in all_lines] + while all_lines and all_lines[-1] == "": + all_lines.pop() + return ["\n".join(all_lines)] if all_lines else [] + + +def _normalize_scripts(parameters: Any, component_id: str) -> Any: """Normalize script arrays in transformation parameters. The Keboola API inconsistently returns code scripts as either: - ``["line1", "line2", ...]`` (per-line array) - ``["full\\ncode\\nwith\\nnewlines"]`` (single multiline string) - This normalizes to single-string-per-code-block format so that local - merge (which produces single-string via ``_lines_to_script``) and - remote data compare identically. The Keboola transformation runner - treats each array element as a separate executable statement, so each - CODE block must be a single joined string. + The normalization is COMPONENT-AWARE (issue #686). Both sides of every + hash comparison must agree on what ``script[]`` looks like, and the push + side (``code_extraction._lines_to_script``) has split SQL on statement + boundaries since PR #120: + + - **SQL transformations** (:func:`is_sql_transformation_component`): + one element per executable statement, via + :func:`canonical_sql_script` -- each existing element split + independently and flattened, never joined first. + - **Everything else**: joined into a single string per code block, + matching ``_lines_to_script``'s non-SQL branch. + + Until #686 this function always collapsed to one element, which no SQL + transformation with two or more statements could ever match -- the + permanent ``~ REMOTE MODIFIED`` phantom drift after every ``sync push``. """ if not isinstance(parameters, dict): return parameters params = copy.deepcopy(parameters) - for block in params.get("blocks", []): - if not isinstance(block, dict): - continue - for code in block.get("codes", []): - if not isinstance(code, dict): - continue - scripts = code.get("script") - if isinstance(scripts, list) and scripts: - # Flatten everything into individual lines first. - all_lines: list[str] = [] - for s in scripts: - if isinstance(s, str) and "\n" in s: - all_lines.extend(s.split("\n")) - else: - all_lines.append(s) - # Strip trailing whitespace per line (YAML roundtrip - # strips it) and remove trailing empty lines. - all_lines = [line.rstrip() for line in all_lines] - while all_lines and all_lines[-1] == "": - all_lines.pop() - # Join into a single string per code block. - code["script"] = ["\n".join(all_lines)] if all_lines else [] + is_sql = is_sql_transformation_component(component_id) + for code in _iter_codes(params): + scripts = code.get("script") + if isinstance(scripts, list) and scripts: + code["script"] = canonical_sql_script(scripts) if is_sql else _join_script(scripts) + return params + + +def _normalize_scripts_legacy(parameters: Any) -> Any: + """Pre-#686 normalization: collapse every code's ``script[]`` into one. + + Kept for ONE purpose: recomputing the hash a pre-#686 kbagent would have + stored, so a manifest entry without ``config_hash_version`` can be + recognised as "in sync apart from the script shape" instead of being + re-classified as drift on the first run after the upgrade. It is never + used to produce data that is written anywhere -- only to compare against + an already-stored hash, and always from the RAW API config. + """ + if not isinstance(parameters, dict): + return parameters + params = copy.deepcopy(parameters) + for code in _iter_codes(params): + scripts = code.get("script") + if isinstance(scripts, list) and scripts: + code["script"] = _join_script(scripts) return params @@ -109,7 +154,11 @@ def classify_component_type(api_type: str) -> str: def api_config_to_local( - component_id: str, config_data: dict[str, Any], config_id: str + component_id: str, + config_data: dict[str, Any], + config_id: str, + *, + legacy_scripts: bool = False, ) -> dict[str, Any]: """Convert an API configuration response to the local ``_config.yml`` structure. @@ -128,6 +177,13 @@ def api_config_to_local( Any remaining keys inside ``configuration`` that are not explicitly promoted are preserved under a ``_configuration_extra`` key so that round-tripping does not lose data. + + Args: + legacy_scripts: Migration compatibility only (issue #686). When True, + ``parameters`` is normalized with the pre-#686 collapse + (:func:`_normalize_scripts_legacy`) so the caller can recompute + the hash an older kbagent would have stored for this same remote + config. Never pass it on a path that WRITES the result. """ configuration: dict[str, Any] = config_data.get("configuration") or {} @@ -141,7 +197,11 @@ def api_config_to_local( # Promote well-known nested keys if "parameters" in configuration: - local["parameters"] = _normalize_scripts(configuration["parameters"]) + local["parameters"] = ( + _normalize_scripts_legacy(configuration["parameters"]) + if legacy_scripts + else _normalize_scripts(configuration["parameters"], component_id) + ) storage: dict[str, Any] = configuration.get("storage") or {} if "input" in storage: diff --git a/tests/test_sync_code_extraction.py b/tests/test_sync_code_extraction.py index e127aa87..d6be4622 100644 --- a/tests/test_sync_code_extraction.py +++ b/tests/test_sync_code_extraction.py @@ -7,6 +7,8 @@ import pytest from keboola_agent_cli.sync.code_extraction import ( + SQL_STATEMENT_MARKER, + canonical_sql_script, extract_code_files, merge_code_files, ) @@ -693,3 +695,100 @@ def test_all_sql_components_recognized(self, component_id: str, tmp_path: Path) extract_code_files(component_id, config_data, config_dir) assert (config_dir / "transform.sql").exists() + + +# =================================================================== +# Statement-boundary markers (issue #686, part 3) +# =================================================================== + + +SQL_COMPONENT = "keboola.snowflake-transformation" + + +def _sql_config(script: list[str]) -> dict[str, Any]: + """Build a minimal SQL-transformation config carrying one code block.""" + return { + "parameters": { + "blocks": [{"name": "Block 1", "codes": [{"name": "Code 1", "script": list(script)}]}] + } + } + + +def _script_of(config_data: dict[str, Any]) -> list[str]: + """Return the first code's script array.""" + return config_data["parameters"]["blocks"][0]["codes"][0]["script"] + + +class TestStatementMarkers: + """``transform.sql`` carries explicit statement boundaries when needed.""" + + def test_marker_written_when_semicolons_cannot_recover_boundaries(self, tmp_path: Path) -> None: + """No trailing semicolons -> an explicit marker separates the statements.""" + config_data = _sql_config(["SELECT 1", "SELECT 2"]) + config_dir = tmp_path / "no-semicolons" + + extract_code_files(SQL_COMPONENT, config_data, config_dir) + + content = (config_dir / "transform.sql").read_text(encoding="utf-8") + assert SQL_STATEMENT_MARKER in content + + def test_marker_round_trip_preserves_statement_count(self, tmp_path: Path) -> None: + """The marked file merges back to the exact original array.""" + original = ["SELECT 1", "SELECT 2", "SELECT 3"] + config_data = _sql_config(original) + config_dir = tmp_path / "no-semicolons-rt" + + extract_code_files(SQL_COMPONENT, config_data, config_dir) + merge_code_files(SQL_COMPONENT, config_data, config_dir) + + assert _script_of(config_data) == original + + def test_no_marker_for_semicolon_terminated_scripts(self, tmp_path: Path) -> None: + """The common ``;``-terminated case stays byte-identical to before.""" + config_data = _sql_config(["SELECT 1;", "SELECT 2;"]) + config_dir = tmp_path / "semicolons" + + extract_code_files(SQL_COMPONENT, config_data, config_dir) + + content = (config_dir / "transform.sql").read_text(encoding="utf-8") + assert SQL_STATEMENT_MARKER not in content + assert "STATEMENT" not in content + + def test_marker_segment_is_still_split_on_semicolons(self, tmp_path: Path) -> None: + """A user adding ``; SELECT ...`` inside a marked segment gets split (R3).""" + config_data = _sql_config(["SELECT 1", "SELECT 2"]) + config_dir = tmp_path / "resplit-segment" + extract_code_files(SQL_COMPONENT, config_data, config_dir) + + sql_file = config_dir / "transform.sql" + content = sql_file.read_text(encoding="utf-8") + sql_file.write_text(content.replace("SELECT 1", "SELECT 1; SELECT 9;"), encoding="utf-8") + + merged: dict[str, Any] = {"parameters": {}} + merge_code_files(SQL_COMPONENT, merged, config_dir) + + assert _script_of(merged) == ["SELECT 1;", "SELECT 9;", "SELECT 2"] + + def test_marker_collision_falls_back_to_no_markers(self, tmp_path: Path) -> None: + """A statement whose own text holds a marker line disables marker emission.""" + config_data = _sql_config([f"SELECT 1\n{SQL_STATEMENT_MARKER}", "SELECT 2"]) + config_dir = tmp_path / "collision" + + extract_code_files(SQL_COMPONENT, config_data, config_dir) + + content = (config_dir / "transform.sql").read_text(encoding="utf-8") + # The marker text appears only as part of the statement itself -- exactly + # once -- never as an emitted boundary. + assert content.count(SQL_STATEMENT_MARKER) == 1 + + def test_canonical_sql_script_splits_each_element(self) -> None: + """Each element is split independently and the results flattened.""" + assert canonical_sql_script(["SELECT 1; SELECT 2;", "SELECT 3"]) == [ + "SELECT 1;", + "SELECT 2;", + "SELECT 3", + ] + + def test_canonical_sql_script_drops_blank_elements(self) -> None: + """Whitespace-only elements vanish, matching the file round-trip.""" + assert canonical_sql_script(["SELECT 1;", " ", ""]) == ["SELECT 1;"] diff --git a/tests/test_sync_config_format.py b/tests/test_sync_config_format.py index 58d7a1a1..c90191e0 100644 --- a/tests/test_sync_config_format.py +++ b/tests/test_sync_config_format.py @@ -6,6 +6,7 @@ from keboola_agent_cli.sync.config_format import ( _normalize_scripts, + _normalize_scripts_legacy, api_config_to_local, api_row_to_local, classify_component_type, @@ -39,6 +40,11 @@ SAMPLE_COMPONENT_ID = "keboola.ex-http" SAMPLE_CONFIG_ID = "cfg-123" +# Script normalization is component-aware (issue #686): SQL transformations +# split one element per statement, everything else joins into one string. +SQL_COMPONENT = "keboola.snowflake-transformation" +NON_SQL_COMPONENT = "keboola.python-transformation-v2" + class TestClassifyComponentType: """Tests for classify_component_type().""" @@ -171,16 +177,22 @@ def test_local_config_to_api_extras_merged_back(self) -> None: class TestNormalizeScripts: - """Tests for _normalize_scripts() -- script array normalization.""" + """Tests for _normalize_scripts() -- script array normalization. + + The normalization is component-aware since issue #686: SQL + transformations split into one element per statement (the runtime's + own semantics, matching ``_lines_to_script`` on the push side), every + other component keeps the historical join-into-one-string behaviour. + """ def test_per_line_array_joined_to_single_string(self) -> None: - """Per-line script array is joined into a single string.""" + """Per-line script array is joined into a single string (non-SQL).""" params = { "blocks": [ {"codes": [{"script": ["CREATE TABLE foo AS", " SELECT col1", " FROM bar;"]}]} ] } - result = _normalize_scripts(params) + result = _normalize_scripts(params, NON_SQL_COMPONENT) script = result["blocks"][0]["codes"][0]["script"] assert len(script) == 1 assert script[0] == "CREATE TABLE foo AS\n SELECT col1\n FROM bar;" @@ -192,7 +204,7 @@ def test_single_multiline_string_preserved(self) -> None: {"codes": [{"script": ["CREATE TABLE foo AS\n SELECT col1\n FROM bar;"]}]} ] } - result = _normalize_scripts(params) + result = _normalize_scripts(params, NON_SQL_COMPONENT) script = result["blocks"][0]["codes"][0]["script"] assert len(script) == 1 assert script[0] == "CREATE TABLE foo AS\n SELECT col1\n FROM bar;" @@ -200,7 +212,7 @@ def test_single_multiline_string_preserved(self) -> None: def test_trailing_whitespace_stripped(self) -> None: """Trailing whitespace per line is stripped during normalization.""" params = {"blocks": [{"codes": [{"script": ["SELECT 1 ", "FROM bar "]}]}]} - result = _normalize_scripts(params) + result = _normalize_scripts(params, NON_SQL_COMPONENT) script = result["blocks"][0]["codes"][0]["script"] assert len(script) == 1 assert script[0] == "SELECT 1\nFROM bar" @@ -208,19 +220,19 @@ def test_trailing_whitespace_stripped(self) -> None: def test_empty_script_preserved(self) -> None: """Empty script array stays empty.""" params = {"blocks": [{"codes": [{"script": []}]}]} - result = _normalize_scripts(params) + result = _normalize_scripts(params, NON_SQL_COMPONENT) assert result["blocks"][0]["codes"][0]["script"] == [] def test_no_blocks_passthrough(self) -> None: """Parameters without blocks are returned unchanged.""" params = {"key": "value"} - result = _normalize_scripts(params) + result = _normalize_scripts(params, NON_SQL_COMPONENT) assert result == {"key": "value"} def test_non_dict_passthrough(self) -> None: """Non-dict input is returned as-is.""" - assert _normalize_scripts("not a dict") == "not a dict" - assert _normalize_scripts(42) == 42 + assert _normalize_scripts("not a dict", NON_SQL_COMPONENT) == "not a dict" + assert _normalize_scripts(42, NON_SQL_COMPONENT) == 42 def test_does_not_mutate_input(self) -> None: """Original parameters are not mutated.""" @@ -228,9 +240,71 @@ def test_does_not_mutate_input(self) -> None: import copy original = copy.deepcopy(params) - _normalize_scripts(params) + _normalize_scripts(params, NON_SQL_COMPONENT) assert params == original + # -- SQL transformations: one element per statement (issue #686) -------- + + def test_sql_splits_each_element_into_statements(self) -> None: + """Each element is split on statement boundaries and flattened.""" + params = {"blocks": [{"codes": [{"script": ["SELECT 1;\nSELECT 2;", "SELECT 3;"]}]}]} + result = _normalize_scripts(params, SQL_COMPONENT) + assert result["blocks"][0]["codes"][0]["script"] == [ + "SELECT 1;", + "SELECT 2;", + "SELECT 3;", + ] + + def test_sql_never_joins_elements_before_splitting(self) -> None: + """Elements without trailing semicolons stay separate statements (R1).""" + params = {"blocks": [{"codes": [{"script": ["SELECT 1", "SELECT 2"]}]}]} + result = _normalize_scripts(params, SQL_COMPONENT) + assert result["blocks"][0]["codes"][0]["script"] == ["SELECT 1", "SELECT 2"] + + def test_sql_multiline_single_statement_stays_one_element(self) -> None: + """A multi-line single statement is not exploded per line.""" + params = { + "blocks": [ + {"codes": [{"script": ["CREATE TABLE foo AS", " SELECT col1", " FROM bar;"]}]} + ] + } + result = _normalize_scripts(params, SQL_COMPONENT) + assert result["blocks"][0]["codes"][0]["script"] == [ + "CREATE TABLE foo AS", + "SELECT col1", + "FROM bar;", + ] + + def test_legacy_normalization_still_collapses(self) -> None: + """The legacy producer (migration compat only) keeps collapsing to one.""" + params = {"blocks": [{"codes": [{"script": ["SELECT 1;", "SELECT 2;"]}]}]} + result = _normalize_scripts_legacy(params) + assert result["blocks"][0]["codes"][0]["script"] == ["SELECT 1;\nSELECT 2;"] + + def test_api_config_to_local_splits_for_sql_component(self) -> None: + """The pull-side converter emits the split shape for SQL components.""" + api_config = { + "id": "1", + "name": "t", + "description": "", + "configuration": { + "parameters": { + "blocks": [{"name": "B", "codes": [{"name": "C", "script": ["S1;", "S2;"]}]}] + } + }, + } + local = api_config_to_local(SQL_COMPONENT, api_config, "1") + assert local["parameters"]["blocks"][0]["codes"][0]["script"] == ["S1;", "S2;"] + + legacy = api_config_to_local(SQL_COMPONENT, api_config, "1", legacy_scripts=True) + assert legacy["parameters"]["blocks"][0]["codes"][0]["script"] == ["S1;\nS2;"] + + def test_broad_predicate_sql_variant_also_splits(self) -> None: + """A SQL backend matched only by fragment (exasol) splits too (R1).""" + params = {"blocks": [{"codes": [{"script": ["SELECT 1;", "SELECT 2;"]}]}]} + result = _normalize_scripts(params, "keboola.exasol-transformation") + assert result["blocks"][0]["codes"][0]["script"] == ["SELECT 1;", "SELECT 2;"] + class TestRowConversion: """Tests for api_row_to_local() and local_row_to_api().""" From 1c6af698f9e11124c0f42bfc5e4fc6d9381267b9 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 25 Aug 2026 14:10:12 +0200 Subject: [PATCH 2/5] fix(sync): push stamps the manifest baseline from the API response `pull_config_hash` is the 3-way diff's base and is defined as the hash of the config as the API returns it -- that is how `sync pull` and the remote side of `sync diff` compute it. `sync push` recomputed it from the files on disk instead, so any config whose local<->API round-trip is not hash-stable was reported `~ REMOTE MODIFIED` by every later diff, forever, with the tree byte-identical to the remote (issue #686). All six stamp sites now derive the hash from the API's own view of what was just written: config create + update, row create + update, and the Phase C / Phase D link backfills. The mutation response is used when it carries a `configuration`; otherwise the config/row is re-read. When neither works, `pull_config_hash` is left UNTOUCHED and a warning is surfaced in the push envelope -- a disk-derived fallback is exactly the asymmetry this fixes, and a partial response missing `isDisabled` must never be read as "enabled". `pull_hash` / `pull_extra_hashes` stay disk-derived: they describe local files, which is correct. This also closes the non-script instance of the same class: a config disabled in the UI whose local YAML has no `is_disabled` key (issue #467 semantics) previously drifted permanently after every push. Migration for manifests already in the wild, since the canonical script[] shape changed the hash of multi-statement SQL configs: * every stamp records `metadata.config_hash_version` next to the hash it computed -- never next to a preserved legacy one; * an entry WITHOUT that key is compared leniently: a stored hash equal to the pre-change hash of the SAME remote config counts as in sync, which pins every other field, so the leniency cannot mask real drift. Applied to the diff base, the local override and the pull idempotency check; * one `sync pull` re-runs extraction (writing boundary markers) and re-stamps, ending the leniency for that entry. The migration pull also honours edited companion files, which the ordinary overwrite-guard never checked; * `sync push` refuses one specific legacy change: a pre-markers tree whose only difference from the remote is the lost statement boundaries would silently collapse statements, so it aborts that change with SYNC_LEGACY_BOUNDARY and tells the user to pull first. Genuine edits push normally. --- docs/error-codes.md | 1 + src/keboola_agent_cli/commands/sync.py | 2 + src/keboola_agent_cli/constants.py | 7 + src/keboola_agent_cli/errors.py | 2 + .../services/_sync_baseline.py | 406 +++++++++++ .../services/_sync_bindings.py | 69 +- .../services/_sync_models.py | 6 +- .../services/_sync_push_ops.py | 62 +- .../services/_sync_writeback.py | 93 ++- .../services/sync_service.py | 152 ++-- src/keboola_agent_cli/sync/code_extraction.py | 11 + tests/test_sync_baseline_stamping.py | 680 ++++++++++++++++++ tests/test_sync_reconcile.py | 19 + tests/test_sync_service.py | 26 + 14 files changed, 1461 insertions(+), 75 deletions(-) create mode 100644 src/keboola_agent_cli/services/_sync_baseline.py create mode 100644 tests/test_sync_baseline_stamping.py diff --git a/docs/error-codes.md b/docs/error-codes.md index d13decaf..721dd956 100644 --- a/docs/error-codes.md +++ b/docs/error-codes.md @@ -129,6 +129,7 @@ of `ErrorCode` in `src/keboola_agent_cli/errors.py`. | `PARENT_CONFIG_NOT_TRACKED` | Row operation references a parent config not in the manifest | | `VARIABLE_LINK_UNRESOLVED` | `sync push` could not resolve a transformation's variables link to a tracked config | | `SYNC_CONFLICT` | `sync pull --force` aborted: local and remote both changed since the last pull (`details.conflicts` lists them) | +| `SYNC_LEGACY_BOUNDARY` | `sync push` refused one config: the working tree predates statement-boundary tracking, so pushing it would merge separate SQL statements into one. Run `sync pull` first | ### Encryption diff --git a/src/keboola_agent_cli/commands/sync.py b/src/keboola_agent_cli/commands/sync.py index 722ff7e6..70cc483f 100644 --- a/src/keboola_agent_cli/commands/sync.py +++ b/src/keboola_agent_cli/commands/sync.py @@ -1098,6 +1098,8 @@ def sync_push( f" Error: {err['change_type']} {err['component_id']}/{err['config_id']}: " f"{err['message']}" ) + for warn in result.get("warnings", []): + formatter.warning(f" {warn['message']}") @sync_app.command("clone") diff --git a/src/keboola_agent_cli/constants.py b/src/keboola_agent_cli/constants.py index e549b16f..8415ec64 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -629,6 +629,13 @@ def _resolve_app_name() -> str: # Manifest v3 introduces ManifestConfigRow.metadata (row-level pull hashes) but does # not change the on-disk YAML shape, so CONFIG_YML_VERSION stays at 2. CONFIG_YML_VERSION: int = 2 +# Shape version of the manifest's ``pull_config_hash``, stored per entry under +# ``metadata.config_hash_version`` (issue #686). Version 2 = SQL transformation +# ``script[]`` normalized to one element per statement. An entry WITHOUT the key +# predates the fix: its stored hash may be the legacy collapsed shape, which +# `sync diff` / `sync pull` accept leniently until the next pull re-stamps it. +CONFIG_HASH_VERSION: int = 2 +CONFIG_HASH_VERSION_KEY: str = "config_hash_version" SANITIZE_NAME_MAX_LENGTH: int = 100 # How many `orphaned` rows `sync diff` / `sync push` print in human mode before # collapsing the rest into a count. A manifest re-targeted by diff --git a/src/keboola_agent_cli/errors.py b/src/keboola_agent_cli/errors.py index 93518334..bba83ffb 100644 --- a/src/keboola_agent_cli/errors.py +++ b/src/keboola_agent_cli/errors.py @@ -93,6 +93,7 @@ class ErrorCode(StrEnum): PARENT_CONFIG_NOT_TRACKED = "PARENT_CONFIG_NOT_TRACKED" VARIABLE_LINK_UNRESOLVED = "VARIABLE_LINK_UNRESOLVED" SYNC_CONFLICT = "SYNC_CONFLICT" + SYNC_LEGACY_BOUNDARY = "SYNC_LEGACY_BOUNDARY" # Encryption ENCRYPTION_FAILED = "ENCRYPTION_FAILED" @@ -304,6 +305,7 @@ def __init__(self, feature: str, *, remedy: str = "") -> None: ErrorCode.CONFIG_ERROR: "configuration", ErrorCode.VALIDATION_ERROR: "validation", ErrorCode.SYNC_CONFLICT: "conflict", + ErrorCode.SYNC_LEGACY_BOUNDARY: "conflict", ErrorCode.PERMISSION_DENIED: "authorization", ErrorCode.DP_LOGIN_FAILED: "authentication", ErrorCode.DP_MFA_REQUIRED: "authentication", diff --git a/src/keboola_agent_cli/services/_sync_baseline.py b/src/keboola_agent_cli/services/_sync_baseline.py new file mode 100644 index 00000000..3f0a8629 --- /dev/null +++ b/src/keboola_agent_cli/services/_sync_baseline.py @@ -0,0 +1,406 @@ +"""API-derived manifest baselines and legacy-hash compatibility (issue #686). + +``pull_config_hash`` is the 3-way diff's base: the normalized hash of the +config *as the API returns it*. ``sync pull`` and the remote side of +``sync diff`` always computed it that way; ``sync push`` instead recomputed it +from the files on disk, so any config whose local<->API round-trip is not +hash-stable was reported ``~ REMOTE MODIFIED`` by every subsequent diff -- +forever, with the working tree byte-identical to the remote. + +This module holds the one producer push now uses (:func:`config_baseline` / +:func:`row_baseline`) plus the migration helpers that keep manifests written by +a pre-#686 kbagent readable: + +- :func:`effective_stored_hash` -- lenient comparison for entries without + ``metadata.config_hash_version``. +- :func:`raise_on_legacy_boundary` -- refuses to push a legacy tree whose + ``transform.sql`` cannot represent the remote's statement boundaries, which + would silently collapse several statements into one. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from pathlib import Path +from typing import TYPE_CHECKING, Any + +from ..constants import CONFIG_HASH_VERSION, CONFIG_HASH_VERSION_KEY +from ..errors import ErrorCode, KeboolaApiError +from ..sync.code_extraction import ( + is_sql_transformation_component, + marker_less_roundtrip, + merge_code_files, +) +from ..sync.config_format import api_config_to_local, api_row_to_local +from ..sync.diff_engine import config_hash +from ..sync.manifest import Manifest + +if TYPE_CHECKING: + from .sync_service import SyncService + +logger = logging.getLogger(__name__) + + +@dataclass +class BaselineStamp: + """The ``pull_config_hash`` a push should record, or the reason it cannot. + + ``stamped=False`` means the API state could not be established (the + mutation response was partial AND the follow-up detail fetch failed). The + caller must then leave the previous ``pull_config_hash`` untouched -- never + fall back to a hash computed from disk, which is the very asymmetry #686 + is about, and which would read a response missing ``isDisabled`` as + "enabled". + """ + + cfg_hash: str = "" + stamped: bool = False + warning: dict[str, str] | None = None + + @property + def version(self) -> int | None: + """The hash-shape version to store, or ``None`` when nothing is stamped. + + The version is only ever written next to a hash actually computed with + the current producer -- never next to a preserved legacy hash. + """ + return CONFIG_HASH_VERSION if self.stamped else None + + +def _usable_payload(response: Any) -> dict[str, Any] | None: + """Return *response* when it is a full configuration/row object. + + The Storage API makes no ``include`` promise on PUT/POST, and mocked + clients in tests routinely answer ``{"id": "..."}``. A payload counts as + full only when it carries a ``configuration`` key -- an id-only response + would hash as an empty, enabled config and stamp a baseline that never + matches. The value may be a list: PHP serializes an empty configuration as + ``[]`` and ``api_config_to_local`` already normalizes that to ``{}``. + """ + if not isinstance(response, dict): + return None + if not isinstance(response.get("configuration"), (dict, list)): + return None + return response + + +def _fetch_warning(component_id: str, config_id: str, exc: Exception) -> dict[str, str]: + """Build the push-envelope warning for an unstampable baseline.""" + message = ( + f"Could not read back {component_id}/{config_id} after the write, so the " + f"manifest baseline was left unchanged; run 'kbagent sync pull' to refresh " + f"it. Cause: {exc}" + ) + logger.warning("%s", message) + return { + "change_type": "baseline_stamp", + "component_id": component_id, + "config_id": config_id, + "message": message, + } + + +def config_baseline( + client: Any, + *, + component_id: str, + config_id: str, + branch_id: int | None, + response: Any, +) -> BaselineStamp: + """Compute the post-write baseline hash for a configuration. + + Prefers the mutation response; falls back to ``get_config_detail`` when it + is partial. A failed fetch yields an unstamped result plus a warning. + """ + payload = _usable_payload(response) + if payload is None: + try: + payload = _usable_payload( + client.get_config_detail( + component_id=component_id, + config_id=config_id, + branch_id=branch_id, + ) + ) + except Exception as exc: + return BaselineStamp(warning=_fetch_warning(component_id, config_id, exc)) + if payload is None: + return BaselineStamp( + warning=_fetch_warning( + component_id, config_id, ValueError("configuration missing from the response") + ) + ) + return BaselineStamp( + cfg_hash=config_hash(api_config_to_local(component_id, payload, config_id)), + stamped=True, + ) + + +def row_baseline( + client: Any, + *, + component_id: str, + config_id: str, + row_id: str, + branch_id: int | None, + response: Any, +) -> BaselineStamp: + """Compute the post-write baseline hash for a configuration row. + + Same contract as :func:`config_baseline`, via ``get_config_row``. Row + hashes are NOT script-normalized (``api_row_to_local`` never was), so no + shape migration applies to them -- but a partial response would still drop + ``isDisabled`` and strand a permanent phantom diff, which is why rows take + the identical read-back path. + """ + payload = _usable_payload(response) + if payload is None: + try: + payload = _usable_payload( + client.get_config_row( + component_id=component_id, + config_id=config_id, + row_id=row_id, + branch_id=branch_id, + ) + ) + except Exception as exc: + return BaselineStamp(warning=_fetch_warning(component_id, f"{config_id}/{row_id}", exc)) + if payload is None: + return BaselineStamp( + warning=_fetch_warning( + component_id, + f"{config_id}/{row_id}", + ValueError("configuration missing from the response"), + ) + ) + return BaselineStamp( + cfg_hash=config_hash(api_row_to_local(payload, component_id)), + stamped=True, + ) + + +def apply_stamp(metadata: dict[str, Any], stamp: BaselineStamp) -> None: + """Record a baseline on a manifest entry's metadata dict. + + A no-op when the stamp failed: the previous ``pull_config_hash`` (and its + version marker, if any) survives untouched, so the state stays *visibly* + stale rather than confidently wrong. + """ + if not stamp.stamped: + return + metadata["pull_config_hash"] = stamp.cfg_hash + metadata[CONFIG_HASH_VERSION_KEY] = CONFIG_HASH_VERSION + + +# --------------------------------------------------------------------------- +# Migration: manifests written before the shape change +# --------------------------------------------------------------------------- + + +def is_legacy_hash( + stored: str, + *, + component_id: str, + config_id: str, + raw_remote: dict[str, Any], +) -> bool: + """True iff *stored* is the pre-#686 hash of this very remote config. + + Computed from the RAW API config through the old collapse normalization -- + never by re-collapsing already-split data, which would not reproduce the + same bytes. A match proves the entry differs from the current producer in + the script shape ALONE: every other field is pinned by the same hash, so + the leniency cannot mask real drift. + """ + if not stored: + return False + legacy = api_config_to_local(component_id, raw_remote, config_id, legacy_scripts=True) + return stored == config_hash(legacy) + + +def effective_stored_hash( + metadata: dict[str, Any], + *, + component_id: str, + config_id: str, + raw_remote: dict[str, Any] | None, + remote_local: dict[str, Any] | None, +) -> str: + """Return the entry's baseline hash, upgraded when it is legacy-shaped. + + Versioned entries (``config_hash_version`` present) are compared strictly. + An unversioned entry whose stored hash is the legacy-shape hash of the + CURRENT remote is treated as if it held the new-shape hash -- that is the + whole migration: one ``sync pull`` re-stamps it and the leniency stops + applying. Anything else (a real remote edit, a real local edit) is + returned untouched, so the leniency cannot mask drift. + """ + stored = str(metadata.get("pull_config_hash", "") or "") + if not stored or metadata.get(CONFIG_HASH_VERSION_KEY): + return stored + if raw_remote is None or remote_local is None: + return stored + remote_hash = config_hash(remote_local) + if stored == remote_hash: + return stored + if is_legacy_hash( + stored, component_id=component_id, config_id=config_id, raw_remote=raw_remote + ): + return remote_hash + return stored + + +def needs_shape_migration( + metadata: dict[str, Any], + *, + component_id: str, + config_id: str, + raw_remote: dict[str, Any], + api_cfg_hash: str, +) -> bool: + """True iff this entry's baseline is a pre-#686 hash of the same remote. + + The pull-side counterpart of :func:`effective_stored_hash`: the remote is + unchanged, only the recorded shape is old, so the pull must re-run + extraction (to write the boundary markers) and re-stamp -- unless the + local files were edited, in which case they are preserved untouched. + """ + stored = str(metadata.get("pull_config_hash", "") or "") + if not stored or metadata.get(CONFIG_HASH_VERSION_KEY) or stored == api_cfg_hash: + return False + return is_legacy_hash( + stored, component_id=component_id, config_id=config_id, raw_remote=raw_remote + ) + + +def extras_modified(service: SyncService, config_dir: Path, extra_hashes: dict[str, str]) -> bool: + """True iff any companion file recorded at pull time changed on disk. + + The pull overwrite-guard has only ever compared ``_config.yml``, so an + edited ``transform.sql`` beside an untouched ``_config.yml`` was + overwritten. That is pre-existing behaviour everywhere EXCEPT the shape + migration, which rewrites code files for a remote that did not change -- + there, silently discarding a local edit would be new damage, so the + migration checks the companions too. + """ + for fname, stored_hash in (extra_hashes or {}).items(): + fpath = config_dir / fname + if not fpath.exists() or service._file_hash(fpath) != stored_hash: + return True + return False + + +def _scripts_by_code(config_data: dict[str, Any]) -> list[list[Any]] | None: + """Collect every ``blocks[].codes[].script`` array, in document order.""" + parameters = config_data.get("parameters") + if not isinstance(parameters, dict): + return None + blocks = parameters.get("blocks") + if not isinstance(blocks, list): + return None + scripts: list[list[Any]] = [] + for block in blocks: + if not isinstance(block, dict): + return None + for code in block.get("codes") or []: + if not isinstance(code, dict): + return None + script = code.get("script") + scripts.append(list(script) if isinstance(script, list) else []) + return scripts + + +def _boundary_only_difference(local: dict[str, Any], remote: dict[str, Any]) -> bool: + """True iff local is exactly the marker-less rendering of the remote. + + Any other difference -- a real SQL edit, a reindent, a different block + layout -- is a genuine local change and must push normally. + """ + local_scripts = _scripts_by_code(local) + remote_scripts = _scripts_by_code(remote) + if local_scripts is None or remote_scripts is None: + return False + if len(local_scripts) != len(remote_scripts): + return False + found = False + for local_script, remote_script in zip(local_scripts, remote_scripts, strict=True): + if local_script == remote_script: + continue + if marker_less_roundtrip(remote_script) != local_script: + return False # a genuine edit -- let it push + found = True + return found + + +def raise_on_legacy_boundary( + service: SyncService, + client: Any, + *, + component_id: str, + config_id: str, + config_dir: Path, + manifest: Manifest, + branch_id: int | None, +) -> None: + """Refuse a push that would collapse the remote's statement boundaries. + + A tree pulled before #686 has a ``transform.sql`` with no boundary markers. + When the remote's ``script[]`` elements carry no trailing semicolons, that + file cannot express where one statement ends -- so ``merge_code_files`` + returns ONE element holding everything and the push silently rewrites + production into the ``MULTI_STATEMENT_COUNT=1`` crash shape of issues + #119/#120/#274, while ``sync diff`` reports "in sync". + + Scope is deliberately narrow: SQL transformations only, only entries with + no ``config_hash_version`` (a pulled-since-#686 tree carries markers and + cannot hit this), and only when the statement TEXT is identical while the + element counts differ. A genuine edit proceeds. A failed remote read + proceeds too -- this is a safety net, not a gate. + + Raises: + KeboolaApiError: with :data:`ErrorCode.SYNC_LEGACY_BOUNDARY`, caught by + the push loop and accumulated as a per-change error. + """ + if not is_sql_transformation_component(component_id): + return + entry = next( + ( + c + for c in manifest.configurations + if c.component_id == component_id and c.id == config_id + ), + None, + ) + if entry is None or entry.metadata.get(CONFIG_HASH_VERSION_KEY): + return + local_data = service._read_config_file(config_dir) + if local_data is None: + return + try: + remote_raw = client.get_config_detail( + component_id=component_id, config_id=config_id, branch_id=branch_id + ) + except Exception: + logger.debug("Legacy boundary guard skipped: %s/%s unreadable", component_id, config_id) + return + if _usable_payload(remote_raw) is None: + return + merge_code_files(component_id, local_data, config_dir) + if not _boundary_only_difference( + local_data, api_config_to_local(component_id, remote_raw, config_id) + ): + return + raise KeboolaApiError( + message=( + f"Refusing to push {component_id}/{config_id}: this working tree predates " + f"statement-boundary tracking, so pushing it would merge separate SQL " + f"statements into one (the MULTI_STATEMENT_COUNT=1 failure). The content " + f"is otherwise identical to the remote. Run 'kbagent sync pull' for this " + f"project first, then push again." + ), + status_code=0, + error_code=ErrorCode.SYNC_LEGACY_BOUNDARY, + ) diff --git a/src/keboola_agent_cli/services/_sync_bindings.py b/src/keboola_agent_cli/services/_sync_bindings.py index bcb34139..e5417972 100644 --- a/src/keboola_agent_cli/services/_sync_bindings.py +++ b/src/keboola_agent_cli/services/_sync_bindings.py @@ -26,6 +26,7 @@ from ..sync.code_extraction import merge_code_files from ..sync.config_format import local_config_to_api from ..sync.manifest import Manifest +from ._sync_baseline import apply_stamp, config_baseline from ._sync_models import ( FLOW_COMPONENT_ID, VARIABLES_COMPONENT_ID, @@ -128,6 +129,7 @@ def resolve_variable_bindings( row_ulid=row_ulid, manifest=manifest, branch_id=branch_id, + warnings=result.warnings, ) except KeboolaApiError as exc: result.errors.append( @@ -255,6 +257,7 @@ def _apply_variable_binding( row_ulid: str | None, manifest: Manifest, branch_id: int | None, + warnings: list[dict[str, str]], ) -> None: """PUT the resolved variables link, rewrite local, refresh manifest hashes. @@ -270,7 +273,7 @@ def _apply_variable_binding( if row_ulid: configuration["variables_values_id"] = row_ulid - client.update_config( + response = client.update_config( component_id=created.component_id, config_id=created.config_id, configuration=configuration, @@ -295,7 +298,44 @@ def _apply_variable_binding( # config_hash includes _configuration_extra, so refresh the stored # hashes from the post-rewrite disk state or sync diff sees a conflict. + _refresh_binding_hashes( + service, + client, + created=created, + manifest=manifest, + branch_id=branch_id, + response=response, + warnings=warnings, + ) + + +def _refresh_binding_hashes( + service: SyncService, + client: Any, + *, + created: CreatedConfig, + manifest: Manifest, + branch_id: int | None, + response: Any, + warnings: list[dict[str, str]], +) -> None: + """Re-stamp a rebound config's manifest bookkeeping after the backfill PUT. + + ``config_hash`` includes ``_configuration_extra``, which both backfills + rewrite, so the stored hashes must be refreshed or ``sync diff`` reports a + conflict. The config hash comes from the API's view of the config it just + wrote (issue #686); the file hashes describe the local files. + """ hashes = service._compute_config_hashes(created.config_dir, created.component_id) + stamp = config_baseline( + client, + component_id=created.component_id, + config_id=created.config_id, + branch_id=branch_id, + response=response, + ) + if stamp.warning is not None: + warnings.append(stamp.warning) target_branch = branch_id or 0 for cfg in manifest.configurations: if ( @@ -304,8 +344,8 @@ def _apply_variable_binding( and cfg.id == created.config_id ): cfg.metadata["pull_hash"] = hashes.file_hash - cfg.metadata["pull_config_hash"] = hashes.cfg_hash cfg.metadata["pull_extra_hashes"] = hashes.extra_hashes + apply_stamp(cfg.metadata, stamp) break @@ -365,6 +405,7 @@ def resolve_flow_task_bindings( local_data=local_data, manifest=manifest, branch_id=branch_id, + warnings=result.warnings, ) except KeboolaApiError as exc: result.errors.append( @@ -416,6 +457,7 @@ def _apply_flow_task_binding( local_data: dict[str, Any], manifest: Manifest, branch_id: int | None, + warnings: list[dict[str, str]], ) -> None: """PUT a remapped flow, rewrite local ``_config.yml``, refresh hashes. @@ -428,7 +470,7 @@ def _apply_flow_task_binding( merge_code_files(created.component_id, merged, created.config_dir) _name, _description, configuration = local_config_to_api(merged) - client.update_config( + response = client.update_config( component_id=created.component_id, config_id=created.config_id, configuration=configuration, @@ -442,15 +484,12 @@ def _apply_flow_task_binding( ) service._write_config_file(created.config_dir, local_data) - hashes = service._compute_config_hashes(created.config_dir, created.component_id) - target_branch = branch_id or 0 - for cfg in manifest.configurations: - if ( - cfg.branch_id == target_branch - and cfg.component_id == created.component_id - and cfg.id == created.config_id - ): - cfg.metadata["pull_hash"] = hashes.file_hash - cfg.metadata["pull_config_hash"] = hashes.cfg_hash - cfg.metadata["pull_extra_hashes"] = hashes.extra_hashes - break + _refresh_binding_hashes( + service, + client, + created=created, + manifest=manifest, + branch_id=branch_id, + response=response, + warnings=warnings, + ) diff --git a/src/keboola_agent_cli/services/_sync_models.py b/src/keboola_agent_cli/services/_sync_models.py index aae3c66a..2307d322 100644 --- a/src/keboola_agent_cli/services/_sync_models.py +++ b/src/keboola_agent_cli/services/_sync_models.py @@ -61,9 +61,11 @@ class VariableBindingResult: local ``_configuration_extra`` were rebound to ULIDs (drives the manifest-dirty flag). ``errors`` accumulates unresolved links so the push envelope surfaces them instead of leaving a broken link silently. + ``warnings`` carries non-fatal baseline-stamping notices (issue #686). """ errors: list[dict[str, str]] = field(default_factory=list) + warnings: list[dict[str, str]] = field(default_factory=list) configs_rewritten: int = 0 @@ -74,10 +76,12 @@ class FlowBindingResult: ``configs_rewritten`` counts flows whose task ``configId``s were remapped to ULIDs (drives the manifest-dirty flag); ``tasks_remapped`` is the total task references rewritten; ``errors`` accumulates PUT failures so the push - envelope surfaces them. + envelope surfaces them. ``warnings`` carries non-fatal baseline-stamping + notices (issue #686). """ errors: list[dict[str, str]] = field(default_factory=list) + warnings: list[dict[str, str]] = field(default_factory=list) configs_rewritten: int = 0 tasks_remapped: int = 0 diff --git a/src/keboola_agent_cli/services/_sync_push_ops.py b/src/keboola_agent_cli/services/_sync_push_ops.py index 4bf104fd..4ffa8d9e 100644 --- a/src/keboola_agent_cli/services/_sync_push_ops.py +++ b/src/keboola_agent_cli/services/_sync_push_ops.py @@ -20,9 +20,9 @@ from ..errors import ErrorCode, KeboolaApiError from ..sync.code_extraction import merge_code_files from ..sync.config_format import local_config_to_api, local_row_to_api -from ..sync.diff_engine import config_hash from ..sync.manifest import Manifest, ManifestConfiguration from ._encryption import encrypt_secrets_in_config +from ._sync_baseline import apply_stamp, row_baseline from ._sync_writeback import writeback_after_push, writeback_create_row_in_manifest if TYPE_CHECKING: @@ -44,6 +44,7 @@ def push_row_change( manifest: Manifest, branch_id: int | None, allow_plaintext_fallback: bool = False, + warnings: list[dict[str, str]] | None = None, ) -> str | None: """Dispatch a single row-level change (added/modified/deleted) to the API. @@ -57,6 +58,11 @@ def push_row_change( before dispatch, so both the manifest parent lookup and ``create_config_row(config_id=...)`` hit the real config (KFR-05). + ``warnings`` accumulates non-fatal baseline-stamping warnings (issue #686) + for the push envelope; when the API state cannot be read back after the + write, the row's ``pull_config_hash`` is left untouched rather than + recomputed from disk. + Returns the API-assigned row id on ``added`` (so the caller can map placeholder -> ULID for variable-link backfill), else ``None``. """ @@ -108,6 +114,7 @@ def push_row_change( branch_id=branch_id, project_id=project_id, allow_plaintext_fallback=allow_plaintext_fallback, + warnings=warnings, ) if change_type == "modified": @@ -122,6 +129,7 @@ def push_row_change( branch_id=branch_id, project_id=project_id, allow_plaintext_fallback=allow_plaintext_fallback, + warnings=warnings, ) return None @@ -140,6 +148,7 @@ def _push_create_row( branch_id: int | None, project_id: int | None, allow_plaintext_fallback: bool, + warnings: list[dict[str, str]] | None = None, ) -> str: """POST a new row; record API-assigned id + hashes in the parent's row list. @@ -178,14 +187,24 @@ def _push_create_row( row_file = row_dir / CONFIG_FILENAME new_file_hash = service._file_hash(row_file) if row_file.exists() else "" - cfg_hash_value = config_hash(pristine_data) - writeback_create_row_in_manifest( + stamp = row_baseline( + client, + component_id=component_id, + config_id=parent_config_id, + row_id=new_row_id, + branch_id=branch_id, + response=result, + ) + row_entry = writeback_create_row_in_manifest( parent=parent, row_path_str=row_path_str, new_row_id=new_row_id, file_hash=new_file_hash, - cfg_hash=cfg_hash_value, + cfg_hash=stamp.cfg_hash, ) + apply_stamp(row_entry.metadata, stamp) + if stamp.warning is not None and warnings is not None: + warnings.append(stamp.warning) return new_row_id @@ -201,8 +220,14 @@ def push_update_row( branch_id: int | None, project_id: int | None, allow_plaintext_fallback: bool, + warnings: list[dict[str, str]] | None = None, ) -> None: - """PUT an existing row; refresh its hashes in the parent's row list.""" + """PUT an existing row; refresh its hashes in the parent's row list. + + The baseline comes from the API's own view of the row (issue #686), so a + row disabled remotely whose local file carries no ``is_disabled`` key does + not leave a permanent phantom diff behind. + """ local_data = service._read_config_file(row_dir) if local_data is None: raise FileNotFoundError(f"Row file not found: {row_dir / CONFIG_FILENAME}") @@ -217,7 +242,7 @@ def push_update_row( allow_plaintext_fallback=allow_plaintext_fallback, ) - client.update_config_row( + result = client.update_config_row( component_id=component_id, config_id=parent_config_id, row_id=row_id, @@ -236,11 +261,20 @@ def push_update_row( row_file = row_dir / CONFIG_FILENAME new_file_hash = service._file_hash(row_file) if row_file.exists() else "" - cfg_hash_value = config_hash(pristine_data) + stamp = row_baseline( + client, + component_id=component_id, + config_id=parent_config_id, + row_id=row_id, + branch_id=branch_id, + response=result, + ) + if stamp.warning is not None and warnings is not None: + warnings.append(stamp.warning) for r in parent.rows: if r.id == row_id: r.metadata["pull_hash"] = new_file_hash - r.metadata["pull_config_hash"] = cfg_hash_value + apply_stamp(r.metadata, stamp) break @@ -331,8 +365,12 @@ def push_update( branch_id: int | None, *, allow_plaintext_fallback: bool = False, -) -> None: - """Update an existing config from a local _config.yml file.""" +) -> dict[str, Any]: + """Update an existing config from a local _config.yml file. + + Returns the API response so the caller can stamp the manifest baseline + from the remote's own view of the config (issue #686). + """ branch_path = service._resolve_source_branch_path(manifest, project_root, branch_id) config_dir = project_root / branch_path / config_path_str local_data = service._read_config_file(config_dir) @@ -358,7 +396,7 @@ def push_update( allow_plaintext_fallback=allow_plaintext_fallback, ) - client.update_config( + result = client.update_config( component_id=component_id, config_id=config_id, name=name, @@ -375,3 +413,5 @@ def push_update( # Write back: update local file with encrypted secrets. Use pristine_data so # blocks/code stay only in their code files. writeback_after_push(service, pristine_data, config_dir, config_id, configuration) + + return result if isinstance(result, dict) else {} diff --git a/src/keboola_agent_cli/services/_sync_writeback.py b/src/keboola_agent_cli/services/_sync_writeback.py index 2524fc3b..d3e737a3 100644 --- a/src/keboola_agent_cli/services/_sync_writeback.py +++ b/src/keboola_agent_cli/services/_sync_writeback.py @@ -15,7 +15,8 @@ from ..errors import KeboolaApiError from ..sync.manifest import ManifestConfigRow, ManifestConfiguration from ._encryption import apply_encrypted_to_local -from ._sync_models import WritebackResult +from ._sync_baseline import apply_stamp, config_baseline +from ._sync_models import LocalConfigHashes, WritebackResult if TYPE_CHECKING: from ..sync.manifest import Manifest @@ -24,6 +25,96 @@ logger = logging.getLogger(__name__) +def stamp_created_config( + client: Any, + *, + manifest: Manifest, + component_id: str, + branch_id: int | None, + config_path_str: str, + new_id: str, + hashes: LocalConfigHashes, + response: Any, + warnings: list[dict[str, str]], +) -> WritebackResult: + """Record a created config with an API-derived ``pull_config_hash`` (#686). + + ``pull_hash`` describes the local file and stays disk-derived; the config + hash is the API's own view of what was just written, so the next + ``sync diff`` compares like with like. + """ + stamp = config_baseline( + client, + component_id=component_id, + config_id=new_id, + branch_id=branch_id, + response=response, + ) + if stamp.warning is not None: + warnings.append(stamp.warning) + writeback = writeback_create_config_in_manifest( + manifest=manifest, + component_id=component_id, + branch_id=branch_id, + config_path_str=config_path_str, + new_id=new_id, + file_hash=hashes.file_hash, + cfg_hash=stamp.cfg_hash, + ) + apply_stamp(writeback.entry.metadata, stamp) + return writeback + + +def stamp_updated_config( + client: Any, + *, + manifest: Manifest, + component_id: str, + config_id: str, + branch_id: int | None, + config_path_str: str, + hashes: LocalConfigHashes, + response: Any, + warnings: list[dict[str, str]], +) -> None: + """Refresh a pushed config's manifest bookkeeping from the API state (#686). + + When the API state cannot be established (partial response AND a failed + read-back), ``pull_config_hash`` is left exactly as it was: visibly stale + beats confidently wrong, and a disk-derived value is what created the + phantom drift in the first place. + + An update that finds no manifest entry is an adopted-by-id config (issue + #497) -- an untracked local file whose ``_keboola.config_id`` resolved on + the branch. It is registered here so later diffs read a stable entry. + """ + stamp = config_baseline( + client, + component_id=component_id, + config_id=config_id, + branch_id=branch_id, + response=response, + ) + if stamp.warning is not None: + warnings.append(stamp.warning) + for cfg in manifest.configurations: + if cfg.component_id == component_id and cfg.id == config_id: + cfg.metadata["pull_hash"] = hashes.file_hash + cfg.metadata["pull_extra_hashes"] = hashes.extra_hashes + apply_stamp(cfg.metadata, stamp) + return + entry = writeback_create_config_in_manifest( + manifest=manifest, + component_id=component_id, + branch_id=branch_id, + config_path_str=config_path_str, + new_id=config_id, + file_hash=hashes.file_hash, + cfg_hash=stamp.cfg_hash, + ).entry + apply_stamp(entry.metadata, stamp) + + def writeback_create_config_in_manifest( *, manifest: Manifest, diff --git a/src/keboola_agent_cli/services/sync_service.py b/src/keboola_agent_cli/services/sync_service.py index e94b97c8..9ca51fca 100644 --- a/src/keboola_agent_cli/services/sync_service.py +++ b/src/keboola_agent_cli/services/sync_service.py @@ -18,6 +18,8 @@ ALWAYS_IGNORED_COMPONENTS, BRANCH_MAPPING_FILENAME, CONFIG_FILENAME, + CONFIG_HASH_VERSION, + CONFIG_HASH_VERSION_KEY, DEFAULT_JOBS_PER_CONFIG, DEFAULT_MAX_SAMPLES, DEFAULT_SAMPLE_LIMIT, @@ -68,6 +70,12 @@ encrypt_secrets_in_config, find_plaintext_secret_keys, ) +from ._sync_baseline import ( + effective_stored_hash, + extras_modified, + needs_shape_migration, + raise_on_legacy_boundary, +) from ._sync_bindings import resolve_flow_task_bindings, resolve_variable_bindings from ._sync_branch import ( branch_link as _branch_link, @@ -98,7 +106,8 @@ ) from ._sync_writeback import ( propagate_kbc_metadata, - writeback_create_config_in_manifest, + stamp_created_config, + stamp_updated_config, ) from .base import BaseService @@ -657,6 +666,11 @@ def pull( f"{c.component_id}/{c.id}": c.path for c in manifest.configurations } existing_keys: set[str] = set(existing_paths.keys()) + # Full metadata per entry -- the shape-migration checks need the + # ``config_hash_version`` marker alongside the hashes (issue #686). + existing_metadata: dict[str, dict[str, Any]] = { + f"{c.component_id}/{c.id}": c.metadata for c in manifest.configurations + } existing_config_hashes: dict[str, str] = { f"{c.component_id}/{c.id}": c.metadata.get("pull_config_hash", "") for c in manifest.configurations @@ -809,6 +823,22 @@ def pull( if config_file.exists(): current_file_hash = self._file_hash(config_file) locally_modified = current_file_hash != old_file_hash + # Shape migration (issue #686): the remote is unchanged, only + # the recorded hash shape is old, so this pull re-extracts + # (writing the boundary markers) and re-stamps. Because the + # rewrite is not driven by a remote change, an edited + # companion file must be preserved too -- the ordinary + # overwrite-guard above only ever looks at ``_config.yml``. + if not locally_modified and needs_shape_migration( + existing_metadata.get(lookup_key, {}), + component_id=component_id, + config_id=config_id, + raw_remote=cfg, + api_cfg_hash=api_cfg_hash, + ): + locally_modified = extras_modified( + self, config_dir, existing_extra_hashes.get(lookup_key, {}) + ) remote_unchanged = False # set in else branch; default for locally_modified path if locally_modified and not dry_run: @@ -1008,6 +1038,12 @@ def pull( "pull_hash": old_pull_hash, "pull_config_hash": old_cfg_hash, } + # The preserved hash was NOT produced by the current + # producer, so its version marker is carried over verbatim + # (absent stays absent) -- never stamped onto a legacy hash. + old_version = existing_metadata.get(lookup_key, {}).get(CONFIG_HASH_VERSION_KEY) + if old_version: + cfg_metadata[CONFIG_HASH_VERSION_KEY] = old_version else: # Compute hashes for all extracted files extra_hashes: dict[str, str] = {} @@ -1026,6 +1062,9 @@ def pull( "pull_hash": file_hash, "pull_config_hash": pull_cfg_hash, "pull_extra_hashes": extra_hashes, + # Freshly computed with the current producer, so the + # shape version is stamped alongside it (issue #686). + CONFIG_HASH_VERSION_KEY: CONFIG_HASH_VERSION, } new_configurations.append( ManifestConfiguration( @@ -1241,6 +1280,10 @@ def diff( # remote_rows: "{component_id}/{parent_config_id}/rows/{row_id}" -> row data remote_configs: dict[str, dict[str, Any]] = {} remote_rows: dict[str, dict[str, Any]] = {} + # Raw (unconverted) API configs, kept so a manifest entry without + # ``config_hash_version`` can be checked against the pre-#686 hash of + # this very config -- the migration leniency in ``effective_stored_hash``. + remote_raw: dict[str, dict[str, Any]] = {} for component in components: component_id = component.get("id", "") if component_id in ALWAYS_IGNORED_COMPONENTS: @@ -1249,6 +1292,7 @@ def diff( config_id = str(cfg.get("id", "")) key = f"{component_id}/{config_id}" remote_configs[key] = api_config_to_local(component_id, cfg, config_id) + remote_raw[key] = cfg for row in cfg.get("rows", []): row_id = str(row.get("id", "")) row_key = f"{component_id}/{config_id}/rows/{row_id}" @@ -1279,6 +1323,7 @@ def diff( local_configs: list[dict[str, Any]] = [] file_unchanged: dict[str, bool] = {} local_override_hashes: dict[str, str] = {} + stored_hashes: dict[str, str] = {} for cfg in scope.in_tree: config_dir = project_root / source_branch_path / cfg.path local_data = self._read_config_file(config_dir) @@ -1309,11 +1354,20 @@ def diff( is_unchanged = config_unchanged and extras_unchanged file_unchanged[key] = is_unchanged - if is_unchanged: + # Baseline hash, leniently upgraded for entries written before the + # script-shape change (issue #686). Strict for versioned entries. + stored_cfg_hash = effective_stored_hash( + cfg.metadata, + component_id=cfg.component_id, + config_id=cfg.id, + raw_remote=remote_raw.get(key), + remote_local=remote_configs.get(key), + ) + stored_hashes[key] = stored_cfg_hash + + if is_unchanged and stored_cfg_hash: # All files match pull state -- use stored API hash - stored_cfg_hash = cfg.metadata.get("pull_config_hash", "") - if stored_cfg_hash: - local_override_hashes[key] = stored_cfg_hash + local_override_hashes[key] = stored_cfg_hash # Always merge for local_data (needed for deep_diff details) merge_code_files(cfg.component_id, local_data, config_dir) @@ -1384,7 +1438,7 @@ def diff( base_hashes: dict[str, str] = {} for cfg in scope.in_tree: key = f"{cfg.component_id}/{cfg.id}" - pch = cfg.metadata.get("pull_config_hash") + pch = stored_hashes.get(key) if pch: base_hashes[key] = pch elif file_unchanged.get(key): @@ -1606,6 +1660,9 @@ def push( updated = 0 deleted = 0 errors: list[dict[str, str]] = [] + # Non-fatal push warnings: today only unstampable manifest baselines + # (issue #686), i.e. the API state could not be read back after a write. + warnings: list[dict[str, str]] = [] pushed_details: list[dict[str, str]] = [] manifest_dirty = False @@ -1649,15 +1706,16 @@ def push( if result: new_id = str(result.get("id", "")) config_dir = project_root / branch_path / config_path_str - hashes = self._compute_config_hashes(config_dir, component_id) - writeback = writeback_create_config_in_manifest( + writeback = stamp_created_config( + client, manifest=manifest, component_id=component_id, branch_id=branch_id, config_path_str=config_path_str, new_id=new_id, - file_hash=hashes.file_hash, - cfg_hash=hashes.cfg_hash, + hashes=self._compute_config_hashes(config_dir, component_id), + response=result, + warnings=warnings, ) # Record placeholder -> ULID so child rows and # transformation variable links can be remapped. @@ -1692,7 +1750,17 @@ def push( pushed_details.append(change) elif change_type == "modified": - push_update( + config_dir = project_root / branch_path / config_path_str + raise_on_legacy_boundary( + self, + client, + component_id=component_id, + config_id=config_id, + config_dir=config_dir, + manifest=manifest, + branch_id=branch_id, + ) + response = push_update( self, client, component_id, @@ -1704,34 +1772,18 @@ def push( allow_plaintext_fallback=allow_plaintext_fallback, ) # Update hashes so pull knows local == remote - config_dir = project_root / branch_path / config_path_str - config_file = config_dir / CONFIG_FILENAME - if config_file.exists(): - hashes = self._compute_config_hashes(config_dir, component_id) - entry_found = False - for cfg in manifest.configurations: - if cfg.component_id == component_id and cfg.id == config_id: - cfg.metadata["pull_hash"] = hashes.file_hash - cfg.metadata["pull_config_hash"] = hashes.cfg_hash - cfg.metadata["pull_extra_hashes"] = hashes.extra_hashes - entry_found = True - break - if not entry_found: - # Adopted-by-id config (issue #497): the update - # targeted an existing remote config that had no - # manifest entry (untracked local file whose - # _keboola.config_id resolved on the branch). - # Register it now so subsequent diffs read a - # stable entry and a local deletion is detected. - writeback_create_config_in_manifest( - manifest=manifest, - component_id=component_id, - branch_id=branch_id, - config_path_str=config_path_str, - new_id=config_id, - file_hash=hashes.file_hash, - cfg_hash=hashes.cfg_hash, - ) + if (config_dir / CONFIG_FILENAME).exists(): + stamp_updated_config( + client, + manifest=manifest, + component_id=component_id, + config_id=config_id, + branch_id=branch_id, + config_path_str=config_path_str, + hashes=self._compute_config_hashes(config_dir, component_id), + response=response, + warnings=warnings, + ) manifest_dirty = True updated += 1 pushed_details.append(change) @@ -1796,6 +1848,7 @@ def push( manifest=manifest, branch_id=branch_id, allow_plaintext_fallback=allow_plaintext_fallback, + warnings=warnings, ) manifest_dirty = True if change_type == "added": @@ -1832,6 +1885,7 @@ def push( branch_id=branch_id, ) errors.extend(binding.errors) + warnings.extend(binding.warnings) if binding.configs_rewritten: manifest_dirty = True @@ -1848,6 +1902,7 @@ def push( branch_id=branch_id, ) errors.extend(flow_binding.errors) + warnings.extend(flow_binding.warnings) if flow_binding.configs_rewritten: manifest_dirty = True @@ -1863,6 +1918,8 @@ def push( "errors": errors, "pushed_details": pushed_details, } + if warnings: + result_data["warnings"] = warnings if flow_binding.tasks_remapped: result_data["flow_task_remaps"] = flow_binding.tasks_remapped if name_drift_warnings and not no_name_drift_warnings: @@ -1949,14 +2006,15 @@ def _record_push_error( config_id, exc, ) - errors.append( - { - "change_type": change_type, - "component_id": component_id, - "config_id": config_id, - "message": str(exc), - } - ) + record: dict[str, str] = { + "change_type": change_type, + "component_id": component_id, + "config_id": config_id, + "message": str(exc), + } + if isinstance(exc, KeboolaApiError) and exc.error_code: + record["error_code"] = str(exc.error_code) + errors.append(record) # ------------------------------------------------------------------ # bulk operations (all projects) diff --git a/src/keboola_agent_cli/sync/code_extraction.py b/src/keboola_agent_cli/sync/code_extraction.py index f333e92a..c0d630cd 100644 --- a/src/keboola_agent_cli/sync/code_extraction.py +++ b/src/keboola_agent_cli/sync/code_extraction.py @@ -388,6 +388,17 @@ def _render_sql_script_lines(scripts: list[Any], *, with_markers: bool) -> list[ return lines +def marker_less_roundtrip(script: list[Any]) -> list[str]: + """What a ``transform.sql`` written WITHOUT markers merges back into. + + The pre-#686 rendering, kept as a predicate: a working tree pulled before + boundary markers existed holds exactly this array, so comparing against it + identifies a config whose only difference from the remote is the lost + statement boundaries (see ``_sync_baseline.raise_on_legacy_boundary``). + """ + return _lines_to_script(_render_sql_script_lines(script, with_markers=False), is_sql=True) + + def _render_sql_code(scripts: list[Any], code_name: str) -> list[str]: """Render one code block, adding statement markers only when needed. diff --git a/tests/test_sync_baseline_stamping.py b/tests/test_sync_baseline_stamping.py new file mode 100644 index 00000000..43fb06c8 --- /dev/null +++ b/tests/test_sync_baseline_stamping.py @@ -0,0 +1,680 @@ +"""Regression tests for issue #686 -- push stamps API-derived baselines. + +``pull_config_hash`` is the 3-way diff's base and is defined as the hash of +the config *as the API returns it*. ``sync push`` used to recompute it from +the files on disk instead, so any config whose local<->API round-trip is not +hash-stable was reported ``~ REMOTE MODIFIED`` by every later ``sync diff``, +forever, with the tree byte-identical to the remote (18 configs across a +21-project production repo in the field report). + +Covered here: + +* push -> diff is in sync for a multi-statement SQL transformation; +* push sends the same statement COUNT it pulled, semicolons or not; +* the ``is_disabled`` instance of the same class (remote disabled, local file + without the key); +* create / row create / row update / Phase C all stamp API-derived hashes; +* a partial (id-only) mutation response triggers a detail fetch, and a failed + fetch leaves the baseline unstamped with a warning -- never a disk hash; +* the ``config_hash_version`` migration: unversioned entries match leniently, + versioned ones strictly; +* the legacy push guard that refuses a boundary-only rewrite. +""" + +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock + +import yaml + +from helpers import setup_single_project +from keboola_agent_cli.constants import ( + CONFIG_FILENAME, + CONFIG_HASH_VERSION, + CONFIG_HASH_VERSION_KEY, +) +from keboola_agent_cli.models import TokenVerifyResponse +from keboola_agent_cli.services.sync_service import SyncService +from keboola_agent_cli.sync.manifest import load_manifest + +SQL_COMPONENT = "keboola.snowflake-transformation" + +SAMPLE_VERIFY_TOKEN = TokenVerifyResponse( + token_id="tok-001", + token_description="kbagent-cli", + project_id=258, + project_name="Production", + owner_name="My Org", +) + +SAMPLE_BRANCHES = [{"id": 12345, "name": "Main", "isDefault": True}] + + +# --------------------------------------------------------------------------- +# Remote fixtures +# --------------------------------------------------------------------------- + + +def _sql_config(script: list[str], config_id: str = "cfg-sql") -> dict[str, Any]: + """One SQL transformation config whose single code holds *script*.""" + return { + "id": config_id, + "name": "Raw data processing", + "description": "", + "configuration": { + "parameters": { + "blocks": [{"name": "Block 1", "codes": [{"name": "Code 1", "script": script}]}] + } + }, + "rows": [], + } + + +def _sql_components(script: list[str]) -> list[dict[str, Any]]: + return [ + { + "id": SQL_COMPONENT, + "type": "transformation", + "configurations": [_sql_config(script)], + } + ] + + +def _http_components( + *, is_disabled: bool = False, rows: list | None = None +) -> list[dict[str, Any]]: + config: dict[str, Any] = { + "id": "cfg-001", + "name": "My HTTP Extractor", + "description": "Fetches data", + "configuration": {"parameters": {"baseUrl": "https://api.example.com"}}, + "rows": rows or [], + } + if is_disabled: + config["isDisabled"] = True + return [{"id": "keboola.ex-http", "type": "extractor", "configurations": [config]}] + + +# --------------------------------------------------------------------------- +# A mock client that behaves like the real API: writes update the state that +# subsequent reads (list / detail) return. +# --------------------------------------------------------------------------- + + +class FakeApi: + """Minimal stateful Storage API double for config + row writes.""" + + def __init__(self, components: list[dict[str, Any]]): + self.components = components + self.update_calls: list[dict[str, Any]] = [] + self.row_update_calls: list[dict[str, Any]] = [] + self.detail_calls = 0 + self.detail_fails = False + self.partial_responses = False + + # -- lookup helpers ------------------------------------------------- + def _find(self, component_id: str, config_id: str) -> dict[str, Any]: + for component in self.components: + if component["id"] != component_id: + continue + for config in component["configurations"]: + if str(config["id"]) == str(config_id): + return config + raise KeyError(f"{component_id}/{config_id}") + + def _response(self, config: dict[str, Any]) -> dict[str, Any]: + return {"id": config["id"]} if self.partial_responses else dict(config) + + # -- API surface ---------------------------------------------------- + def list_components_with_configs(self, branch_id: int | None = None) -> list[dict[str, Any]]: + return self.components + + def list_dev_branches(self) -> list[dict[str, Any]]: + return SAMPLE_BRANCHES + + def verify_token(self) -> TokenVerifyResponse: + return SAMPLE_VERIFY_TOKEN + + def get_config_detail( + self, component_id: str, config_id: str, branch_id: int | None = None + ) -> dict[str, Any]: + self.detail_calls += 1 + if self.detail_fails: + raise RuntimeError("boom") + return dict(self._find(component_id, config_id)) + + def get_config_row( + self, + component_id: str, + config_id: str, + row_id: str, + branch_id: int | None = None, + ) -> dict[str, Any]: + parent = self._find(component_id, config_id) + for row in parent.get("rows", []): + if str(row["id"]) == str(row_id): + return dict(row) + raise KeyError(row_id) + + def update_config( + self, + component_id: str, + config_id: str, + name: str | None = None, + configuration: dict[str, Any] | None = None, + description: str | None = None, + change_description: str = "", + branch_id: int | None = None, + is_disabled: bool | None = None, + ) -> dict[str, Any]: + config = self._find(component_id, config_id) + self.update_calls.append({"component_id": component_id, "configuration": configuration}) + if configuration is not None: + config["configuration"] = configuration + if name is not None: + config["name"] = name + if description is not None: + config["description"] = description + if is_disabled is not None: + config["isDisabled"] = is_disabled + return self._response(config) + + def create_config( + self, + component_id: str, + name: str, + configuration: dict[str, Any], + description: str = "", + branch_id: int | None = None, + is_disabled: bool = False, + ) -> dict[str, Any]: + config: dict[str, Any] = { + "id": "cfg-new", + "name": name, + "description": description, + "configuration": configuration, + "rows": [], + } + if is_disabled: + config["isDisabled"] = True + for component in self.components: + if component["id"] == component_id: + component["configurations"].append(config) + break + else: + self.components.append( + {"id": component_id, "type": "transformation", "configurations": [config]} + ) + return self._response(config) + + def create_config_row( + self, + component_id: str, + config_id: str, + name: str, + configuration: dict[str, Any], + description: str = "", + is_disabled: bool = False, + branch_id: int | None = None, + ) -> dict[str, Any]: + parent = self._find(component_id, config_id) + row: dict[str, Any] = { + "id": "row-new", + "name": name, + "description": description, + "configuration": configuration, + } + if is_disabled: + row["isDisabled"] = True + parent.setdefault("rows", []).append(row) + return {"id": row["id"]} if self.partial_responses else dict(row) + + def update_config_row( + self, + component_id: str, + config_id: str, + row_id: str, + name: str | None = None, + configuration: dict[str, Any] | None = None, + description: str | None = None, + is_disabled: bool | None = None, + change_description: str = "", + branch_id: int | None = None, + ) -> dict[str, Any]: + parent = self._find(component_id, config_id) + for row in parent.get("rows", []): + if str(row["id"]) != str(row_id): + continue + self.row_update_calls.append({"row_id": row_id, "configuration": configuration}) + if configuration is not None: + row["configuration"] = configuration + if name is not None: + row["name"] = name + if is_disabled is not None: + row["isDisabled"] = is_disabled + return {"id": row["id"]} if self.partial_responses else dict(row) + raise KeyError(row_id) + + +def _client_for(api: FakeApi) -> MagicMock: + """Wrap a :class:`FakeApi` in a context-manager mock client.""" + client = MagicMock(wraps=api) + client.__enter__ = MagicMock(return_value=client) + client.__exit__ = MagicMock(return_value=False) + client.encrypt_values = MagicMock(side_effect=lambda *a, **k: {}) + return client + + +def _service(store: Any, api: FakeApi) -> SyncService: + return SyncService(config_store=store, client_factory=lambda url, token: _client_for(api)) + + +def _init_and_pull(tmp_config_dir: Path, project_root: Path, api: FakeApi) -> Any: + project_root.mkdir(exist_ok=True) + store = setup_single_project(tmp_config_dir) + _service(store, api).init_sync(alias="prod", project_root=project_root) + _service(store, api).pull( + alias="prod", project_root=project_root, no_storage=True, no_jobs=True + ) + return store + + +def _sql_file(project_root: Path) -> Path: + matches = list(project_root.rglob("transform.sql")) + assert len(matches) == 1 + return matches[0] + + +def _config_file(project_root: Path, under_rows: bool = False) -> Path: + matches = [f for f in project_root.rglob(CONFIG_FILENAME) if ("rows" in f.parts) == under_rows] + assert len(matches) == 1 + return matches[0] + + +def _entry(project_root: Path, config_id: str = "cfg-sql") -> Any: + manifest = load_manifest(project_root) + return next(c for c in manifest.configurations if c.id == config_id) + + +def _remote_script(api: FakeApi) -> list[str]: + config = api._find(SQL_COMPONENT, "cfg-sql") + return config["configuration"]["parameters"]["blocks"][0]["codes"][0]["script"] + + +# =================================================================== +# The reporter's regression test: push -> diff must be in sync +# =================================================================== + + +def test_push_then_diff_in_sync_for_multi_statement_sql( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """The #686 headline: no phantom drift after pushing a multi-statement SQL.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;", "SELECT 3;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 3;", "SELECT 4;"), encoding="utf-8" + ) + + push_result = _service(store, api).push(alias="prod", project_root=project_root) + assert push_result["status"] == "pushed" + assert push_result["errors"] == [] + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 0 + assert diff_result["summary"]["modified"] == 0 + assert diff_result["summary"]["conflict"] == 0 + + +def test_push_preserves_statement_count_with_semicolons( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """Mirror test: push sends the same number of statements it pulled.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2;", "SELECT 22;"), encoding="utf-8" + ) + _service(store, api).push(alias="prod", project_root=project_root) + + assert _remote_script(api) == ["SELECT 1;", "SELECT 22;"] + + +def test_push_preserves_statement_count_without_semicolons( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """The silent-rewrite half: no trailing ``;`` must not collapse to one.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1", "SELECT 2"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2", "SELECT 22"), encoding="utf-8" + ) + _service(store, api).push(alias="prod", project_root=project_root) + + assert _remote_script(api) == ["SELECT 1", "SELECT 22"] + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 0 + + +def test_push_of_disabled_config_without_local_key_leaves_no_drift( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """is_disabled instance of the same class (issue #467 semantics).""" + project_root = tmp_path / "project" + api = FakeApi(_http_components(is_disabled=True)) + store = _init_and_pull(tmp_config_dir, project_root, api) + + # Drop the sparse is_disabled key, as a hand-written/legacy tree has it, + # and change something so the config is pushable. + config_file = _config_file(project_root) + data = yaml.safe_load(config_file.read_text(encoding="utf-8")) + data.pop("is_disabled", None) + data["parameters"]["baseUrl"] = "https://changed.example.com" + config_file.write_text(yaml.dump(data, default_flow_style=False), encoding="utf-8") + + _service(store, api).push(alias="prod", project_root=project_root) + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 0 + assert diff_result["summary"]["conflict"] == 0 + + +# =================================================================== +# Stamping mechanics (create / rows / partial responses) +# =================================================================== + + +def test_update_stamps_api_derived_hash_and_version(tmp_config_dir: Path, tmp_path: Path) -> None: + """A pushed config records the API's hash plus the shape version.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + before = _entry(project_root).metadata["pull_config_hash"] + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2;", "SELECT 9;"), encoding="utf-8" + ) + _service(store, api).push(alias="prod", project_root=project_root) + + entry = _entry(project_root) + assert entry.metadata["pull_config_hash"] != before + assert entry.metadata[CONFIG_HASH_VERSION_KEY] == CONFIG_HASH_VERSION + + +def test_create_stamps_api_derived_hash(tmp_config_dir: Path, tmp_path: Path) -> None: + """A freshly created config gets an API-derived baseline, not a disk one.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + new_dir = project_root / "main" / "transformation" / SQL_COMPONENT / "new-transformation" + new_dir.mkdir(parents=True) + (new_dir / CONFIG_FILENAME).write_text( + yaml.dump({"version": 2, "name": "New", "description": "", "parameters": {}}), + encoding="utf-8", + ) + (new_dir / "transform.sql").write_text("SELECT 100;\n\nSELECT 200;\n", encoding="utf-8") + + result = _service(store, api).push(alias="prod", project_root=project_root) + assert result["created"] == 1 + + entry = _entry(project_root, "cfg-new") + assert entry.metadata["pull_config_hash"] + assert entry.metadata[CONFIG_HASH_VERSION_KEY] == CONFIG_HASH_VERSION + assert ( + _service(store, api).diff(alias="prod", project_root=project_root)["summary"][ + "remote_modified" + ] + == 0 + ) + + +def test_row_push_stamps_api_derived_hash(tmp_config_dir: Path, tmp_path: Path) -> None: + """A disabled remote row whose local file lacks the key leaves no drift.""" + project_root = tmp_path / "project" + row = { + "id": "row-001", + "name": "Users", + "description": "", + "configuration": {"parameters": {"path": "/users"}}, + "isDisabled": True, + } + api = FakeApi(_http_components(rows=[row])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + row_file = _config_file(project_root, under_rows=True) + data = yaml.safe_load(row_file.read_text(encoding="utf-8")) + data.pop("is_disabled", None) + data["parameters"]["path"] = "/people" + row_file.write_text(yaml.dump(data, default_flow_style=False), encoding="utf-8") + + _service(store, api).push(alias="prod", project_root=project_root) + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 0 + assert diff_result["summary"]["conflict"] == 0 + + +def test_partial_mutation_response_triggers_detail_fetch( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """An id-only PUT response is not trusted -- the config is re-read.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + api.partial_responses = True + api.detail_calls = 0 + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2;", "SELECT 9;"), encoding="utf-8" + ) + result = _service(store, api).push(alias="prod", project_root=project_root) + + assert api.detail_calls >= 1 + assert result["errors"] == [] + assert not result.get("warnings") + assert _entry(project_root).metadata[CONFIG_HASH_VERSION_KEY] == CONFIG_HASH_VERSION + + +def test_detail_fetch_failure_leaves_baseline_unstamped( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """No API state, no stamp: the old baseline survives and a warning is raised.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + before = _entry(project_root).metadata["pull_config_hash"] + api.partial_responses = True + api.detail_fails = True + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2;", "SELECT 9;"), encoding="utf-8" + ) + result = _service(store, api).push(alias="prod", project_root=project_root) + + assert result["errors"] == [] + assert len(result["warnings"]) == 1 + assert "sync pull" in result["warnings"][0]["message"] + # The baseline is left exactly as the pull wrote it -- never recomputed + # from disk, which is the asymmetry #686 is about. + assert _entry(project_root).metadata["pull_config_hash"] == before + + +# =================================================================== +# Migration: manifests written before the shape change +# =================================================================== + + +SQL_HEADER = "/* ===== BLOCK: Block 1 ===== */\n\n/* ===== CODE: Code 1 ===== */\n" + + +def _downgrade_to_legacy( + project_root: Path, api: FakeApi, marker_less_body: str | None = None +) -> None: + """Rewrite the tree the way a pre-#686 kbagent would have left it. + + The manifest entry loses its ``config_hash_version`` and carries the old + collapsed-shape hash. When *marker_less_body* is given, ``transform.sql`` is + rewritten without boundary markers (the pre-#686 rendering) and its recorded + companion hash is refreshed, so the file still counts as untouched. + """ + import hashlib + + from keboola_agent_cli.sync.config_format import api_config_to_local + from keboola_agent_cli.sync.diff_engine import config_hash + from keboola_agent_cli.sync.manifest import save_manifest + + sql_hash = "" + if marker_less_body is not None: + sql_file = _sql_file(project_root) + sql_file.write_text(SQL_HEADER + marker_less_body, encoding="utf-8") + sql_hash = hashlib.sha256(sql_file.read_bytes()).hexdigest() + + manifest = load_manifest(project_root) + raw = api._find(SQL_COMPONENT, "cfg-sql") + legacy = config_hash(api_config_to_local(SQL_COMPONENT, raw, "cfg-sql", legacy_scripts=True)) + for cfg in manifest.configurations: + if cfg.id == "cfg-sql": + cfg.metadata["pull_config_hash"] = legacy + cfg.metadata.pop(CONFIG_HASH_VERSION_KEY, None) + if sql_hash: + cfg.metadata.setdefault("pull_extra_hashes", {})["transform.sql"] = sql_hash + save_manifest(project_root, manifest) + + +def test_unversioned_legacy_hash_diffs_as_in_sync(tmp_config_dir: Path, tmp_path: Path) -> None: + """A pre-#686 baseline is accepted leniently instead of showing drift.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api) + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 0 + assert diff_result["summary"]["conflict"] == 0 + + +def test_versioned_entry_is_compared_strictly(tmp_config_dir: Path, tmp_path: Path) -> None: + """With the version key present, a legacy-shaped hash is real drift.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api) + + from keboola_agent_cli.sync.manifest import save_manifest + + manifest = load_manifest(project_root) + for cfg in manifest.configurations: + cfg.metadata[CONFIG_HASH_VERSION_KEY] = CONFIG_HASH_VERSION + save_manifest(project_root, manifest) + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 1 + + +def test_real_remote_drift_is_still_reported_on_unversioned_entry( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """Leniency covers the script shape only -- genuine remote edits still show.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api) + + api._find(SQL_COMPONENT, "cfg-sql")["name"] = "Renamed in the UI" + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 1 + + +def test_pull_migrates_unversioned_entry(tmp_config_dir: Path, tmp_path: Path) -> None: + """One ``sync pull`` re-stamps the entry with the new shape + version.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1", "SELECT 2"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + # A tree pulled before boundary markers existed. + _downgrade_to_legacy(project_root, api, "SELECT 1\n\nSELECT 2\n") + + _service(store, api).pull( + alias="prod", project_root=project_root, no_storage=True, no_jobs=True + ) + + entry = _entry(project_root) + assert entry.metadata[CONFIG_HASH_VERSION_KEY] == CONFIG_HASH_VERSION + # Extraction re-ran, so the boundary markers are now on disk. + assert "STATEMENT" in _sql_file(project_root).read_text(encoding="utf-8") + + +def test_pull_migration_preserves_locally_edited_code_file( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """A migration pull must not clobber an edited transform.sql (R4).""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1", "SELECT 2"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api, "SELECT 1\n\nSELECT 2\n") + + sql_file = _sql_file(project_root) + edited = SQL_HEADER + "SELECT 1\n\nSELECT 999\n" + sql_file.write_text(edited, encoding="utf-8") + + _service(store, api).pull( + alias="prod", project_root=project_root, no_storage=True, no_jobs=True + ) + + assert sql_file.read_text(encoding="utf-8") == edited + assert CONFIG_HASH_VERSION_KEY not in _entry(project_root).metadata + + +def test_legacy_boundary_push_is_refused(tmp_config_dir: Path, tmp_path: Path) -> None: + """A pre-markers tree that would collapse statements is aborted, not pushed.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1", "SELECT 2"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + # Pre-markers rendering: the boundary between the two statements is lost. + _downgrade_to_legacy(project_root, api, "SELECT 1\n\nSELECT 2\n") + # Touch _config.yml so the config is classified as locally modified. + config_file = _config_file(project_root) + data = yaml.safe_load(config_file.read_text(encoding="utf-8")) + data["description"] = "edited" + config_file.write_text(yaml.dump(data, default_flow_style=False), encoding="utf-8") + + result = _service(store, api).push(alias="prod", project_root=project_root) + + assert result["updated"] == 0 + assert len(result["errors"]) == 1 + assert result["errors"][0]["error_code"] == "SYNC_LEGACY_BOUNDARY" + assert "sync pull" in result["errors"][0]["message"] + # The remote statement array is untouched. + assert _remote_script(api) == ["SELECT 1", "SELECT 2"] + + +def test_genuine_edit_on_legacy_tree_still_pushes(tmp_config_dir: Path, tmp_path: Path) -> None: + """The guard is boundary-specific: a real SQL edit is not blocked.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api) + + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2;", "SELECT 42;"), encoding="utf-8" + ) + result = _service(store, api).push(alias="prod", project_root=project_root) + + assert result["errors"] == [] + assert result["updated"] == 1 + assert _remote_script(api) == ["SELECT 1;", "SELECT 42;"] diff --git a/tests/test_sync_reconcile.py b/tests/test_sync_reconcile.py index c28fc223..dd4ed284 100644 --- a/tests/test_sync_reconcile.py +++ b/tests/test_sync_reconcile.py @@ -18,6 +18,7 @@ import shutil from pathlib import Path +from typing import Any from unittest.mock import MagicMock import yaml @@ -87,11 +88,29 @@ def _make_mock_client( client.verify_token.return_value = verify_token_response if components_response is not None: client.list_components_with_configs.return_value = components_response + # ``sync push`` reads the config back to stamp an API-derived manifest + # baseline (issue #686); serve it from the same remote fixture. + client.get_config_detail.side_effect = _detail_from(components_response) if branches_response is not None: client.list_dev_branches.return_value = branches_response return client +def _detail_from(components: list) -> Any: + """Build a ``get_config_detail`` side effect backed by *components*.""" + + def _detail(component_id: str, config_id: str, branch_id: int | None = None) -> dict: + for component in components: + if component.get("id") != component_id: + continue + for config in component.get("configurations", []): + if str(config.get("id")) == str(config_id): + return dict(config) + raise KeyError(f"{component_id}/{config_id}") + + return _detail + + def _svc(store: ConfigStore, components: list | None = None) -> SyncService: """SyncService whose factory mints a fresh mock client per call.""" return SyncService( diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index 7c70f924..a46066a1 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -3540,6 +3540,32 @@ def fake_create_config(**kwargs: Any) -> dict[str, str]: client.create_config.side_effect = fake_create_config client.create_config_row.return_value = {"id": "VALS-9"} client.update_config.return_value = {"id": "TX-9"} + + # ``sync push`` reads each written config/row back to stamp an + # API-derived manifest baseline (issue #686). The mutation responses + # above are id-only, so serve the read-back from the post-push remote. + remote = self._remote_after_create() + + def fake_detail(component_id: str, config_id: str, branch_id: Any = None) -> dict[str, Any]: + for component in remote: + if component["id"] != component_id: + continue + for config in component["configurations"]: + if config["id"] == config_id: + return dict(config) + raise KeyError(config_id) + + def fake_row( + component_id: str, config_id: str, row_id: str, branch_id: Any = None + ) -> dict[str, Any]: + parent = fake_detail(component_id, config_id) + for row in parent.get("rows", []): + if row["id"] == row_id: + return dict(row) + raise KeyError(row_id) + + client.get_config_detail.side_effect = fake_detail + client.get_config_row.side_effect = fake_row return client def _remote_after_create(self) -> list[dict[str, Any]]: From 8490b2dad661ace7f2847b89df9323a76a534de6 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 25 Aug 2026 14:13:29 +0200 Subject: [PATCH 3/5] docs(sync): record the #686 baseline fix and its one-pull migration Covers every silent-drift doc surface for the change: the gotchas log (new vNEXT-tagged section), sync-workflow.md (push/diff behaviour plus a "migrating a tree pulled before vNEXT" section), the CLAUDE.md sync command block, commands-reference.md (sync push / sync diff), and the keboola-expert tool-selection note that a `codes changed` diff on an untouched config was phantom on <= 0.90.1. --- CLAUDE.md | 12 ++++ plugins/kbagent/agents/keboola-expert.md | 6 +- .../kbagent/references/commands-reference.md | 4 +- .../skills/kbagent/references/gotchas.md | 69 +++++++++++++++++++ .../kbagent/references/sync-workflow.md | 38 ++++++++++ 5 files changed, 126 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7240e196..fbb7e242 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -807,6 +807,18 @@ kbagent sync pull --project ALIAS [--all-projects] [--force] [--theirs] [--dry-r 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] +# sync push (since vNEXT, #686): the manifest baseline `pull_config_hash` is stamped from the API +# response (or a read-back), never from disk -- push-deployed multi-statement SQL transformations +# (and anything disabled in the UI whose local YAML lacks `is_disabled`) no longer show permanent +# phantom `~ REMOTE MODIFIED` drift in `sync diff`. Unreadable-after-write leaves the baseline +# UNTOUCHED + a `warnings[]` entry (never a disk-derived fallback). One canonical script[] shape now +# (one element = one statement) and `transform.sql` gains `/* ===== STATEMENT ===== */` markers when +# semicolons cannot recover the boundaries -- which also closes a SILENT pre-vNEXT rewrite that +# collapsed such scripts to one statement (MULTI_STATEMENT_COUNT=1) while diff said "in sync". +# Migration: entries carry `metadata.config_hash_version`; unversioned ones match leniently (pre-vNEXT +# hash of the SAME remote counts as in sync, nothing else), and ONE `sync pull` per project migrates. +# A pre-markers tree whose only difference from the remote is the lost boundaries is REFUSED per-change +# with SYNC_LEGACY_BOUNDARY telling you to pull first; genuine edits push normally. # sync diff/push (0.89.0+, #649): local side read from exactly ONE tree (target branch subtree, else main/); entries tracked on another branch's tree are excluded from the changeset and reported under orphaned[] + summary.orphaned (reasons + reconcile hints); fix with sync pull. Adopt-by-id is branch-aware. 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). 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. diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index fb99e212..0fac9e53 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -256,7 +256,11 @@ its absence is NOT a promise the entry is version-independent (see ยง1 Rule 6). `never_fetched` warning on diff/push = run `sync pull` first; a non-zero `summary.orphaned` (0.89.0+, #649) = the manifest is targeted at another branch's tree -- `sync pull` to re-target, never push. `sync status` - is local-only -- audit real drift with `sync diff`. + is local-only -- audit real drift with `sync diff`. On <= 0.90.1 a + `~ REMOTE MODIFIED ... codes changed` on a config nobody touched is usually + PHANTOM (issue #686: push stamped the baseline from disk); fixed in vNEXT -- + one `sync pull` per project migrates a tree pulled by an older version, and + a `SYNC_LEGACY_BOUNDARY` push error means exactly that: pull first. - **Native types**: `--column amount:NUMBER(18,2)` passes through; `BOOLEAN` defaults must be lowercase; `INTEGER(10)` is invalid (use `NUMBER(3,0)`); `--not-null` / `--default` must name a defined `--column`. In a dev branch diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index 9cdfc5f2..21cce442 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -323,9 +323,9 @@ Requires the project to be added with its **master ('owner') Storage API token** ## Sync (GitOps) - `sync init --project ALIAS [--directory DIR] [--git-branching] [--adopt-existing]` -- initialize sync working directory; `--adopt-existing` (since v0.22.0) adopts a `.keboola/manifest.json` already written by the kbc Go CLI without overwriting (idempotent; validates `project_id` against the alias token) - `sync pull --project ALIAS [--all-projects] [--force] [--theirs] [--dry-run] [--with-samples] [--no-storage] [--no-jobs] [--job-limit N] [--branch ID]` -- download configs to local files. For large projects (>100 configs), automatically fetches jobs per-config when the grouped API limit is insufficient. `--force` is conflict-aware (since 0.53.0): a locally-modified config whose remote is unchanged is **preserved** (pending delta stays pushable, never silently re-stamped); a true merge conflict (local AND remote both changed since last pull) **aborts** the pull (exit 1, `SYNC_CONFLICT`; `--json` lists `details.conflicts`); local-untouched + remote-changed takes remote. `--theirs` (since v0.72.0) is the supported "discard local, take production" reconcile path: overwrites locally-modified configs/rows, restores deleted/missing files, resolves conflicts by taking remote (no abort, no manifest surgery). Since v0.72.0 plain pull also re-materializes a tracked config whose local dir was deleted (manifest<->disk invariant), so delete-dir-then-pull refetches. Config-level `isDisabled` round-trips (since v0.72.0) as sparse `is_disabled: true` in `_config.yml` -- absent key = enabled. `--branch` (0.47.0+) per-invocation dev-branch override, beats every other branch source. -- `sync push --project ALIAS [--all-projects] [--dry-run] [--force] [--allow-plaintext-on-encrypt-failure] [--branch ID] [--no-name-drift-warnings]` -- push local changes (auto-encrypts secrets, fails if encryption fails). Fresh-CREATE writeback updates placeholder manifest entries in place (since 0.47.0) and propagates any `KBC.configuration.*` metadata via `set_config_metadata`. Fresh-CREATE variable binding (since 0.47.2): when a `keboola.variables` config + its values row are created alongside a transformation in the same push, the transformation's `variables_id` / `variables_values_id` placeholders are rebound to the assigned ULIDs and the row's `values` are hoisted even without a `_keboola` block, so `job run` succeeds with no post-push `config variables-set` step (unresolvable/ambiguous links surface a `variable_link` entry in `errors[]`, never a broken link). Never-fetched guard (since v0.72.0): a manifest entry with an empty `pull_hash` and no local files (pre-0.72 name-collision phantom) is **never** planned as a remote DELETE -- diff/push exclude it and report it under `never_fetched` with a warning (run `sync pull` to materialize); local deletion of a properly-pulled config still deletes on push. Adopted-by-id writeback (since v0.72.0): pushing an untracked file whose `_keboola.config_id` resolves on the branch also writes the manifest entry, so follow-up diffs are stable. `--branch` (0.47.0+) per-invocation override; when no `/` subtree exists on disk (since 0.47.2) the local default tree (`main/`) is promoted to the target branch (API writes still target the branch id); `--no-name-drift-warnings` (0.47.0+) drops the cosmetic warnings array. Branch-scoped since v0.89.0 (issue #649): push consumes the diff's changeset, so configs tracked on another branch's tree are never planned as creates -- they ride along on the result envelope under `orphaned` instead (see `sync diff`). +- `sync push --project ALIAS [--all-projects] [--dry-run] [--force] [--allow-plaintext-on-encrypt-failure] [--branch ID] [--no-name-drift-warnings]` -- push local changes (auto-encrypts secrets, fails if encryption fails). Fresh-CREATE writeback updates placeholder manifest entries in place (since 0.47.0) and propagates any `KBC.configuration.*` metadata via `set_config_metadata`. Fresh-CREATE variable binding (since 0.47.2): when a `keboola.variables` config + its values row are created alongside a transformation in the same push, the transformation's `variables_id` / `variables_values_id` placeholders are rebound to the assigned ULIDs and the row's `values` are hoisted even without a `_keboola` block, so `job run` succeeds with no post-push `config variables-set` step (unresolvable/ambiguous links surface a `variable_link` entry in `errors[]`, never a broken link). Never-fetched guard (since v0.72.0): a manifest entry with an empty `pull_hash` and no local files (pre-0.72 name-collision phantom) is **never** planned as a remote DELETE -- diff/push exclude it and report it under `never_fetched` with a warning (run `sync pull` to materialize); local deletion of a properly-pulled config still deletes on push. Adopted-by-id writeback (since v0.72.0): pushing an untracked file whose `_keboola.config_id` resolves on the branch also writes the manifest entry, so follow-up diffs are stable. `--branch` (0.47.0+) per-invocation override; when no `/` subtree exists on disk (since 0.47.2) the local default tree (`main/`) is promoted to the target branch (API writes still target the branch id); `--no-name-drift-warnings` (0.47.0+) drops the cosmetic warnings array. Branch-scoped since v0.89.0 (issue #649): push consumes the diff's changeset, so configs tracked on another branch's tree are never planned as creates -- they ride along on the result envelope under `orphaned` instead (see `sync diff`). **Since vNEXT (#686)** the manifest baseline `pull_config_hash` is stamped from the API response (or a read-back), not from the files on disk, so a pushed multi-statement SQL transformation -- or anything disabled in the UI whose local YAML lacks `is_disabled` -- no longer shows permanent phantom `REMOTE MODIFIED` drift; if the config cannot be read back after the write the baseline is left UNTOUCHED and a `warnings[]` entry says to run `sync pull` (never a disk-derived fallback). One legacy change is refused per-change with `SYNC_LEGACY_BOUNDARY`: a tree pulled before statement-boundary markers existed whose only difference from the remote is the lost boundaries (pushing it would collapse separate SQL statements into one) -- run `sync pull` for that project first. - `sync clone --source DIR --target ALIAS --target-dir DIR [--bucket-map FILE] [--variable-values FILE] [--instance-rename FILE] [--dry-run] [--branch ID]` -- clone a reference synced project into a **fresh** target project and parameterize it (since v0.63.0). Copies the reference tree at `--source` into `--target-dir`, applies declarative overrides from JSON/YAML files (`--bucket-map` `{old_bucket_id: new_bucket_id}` rewrites storage input/output table refs; `--variable-values` `{var_name: value}` overrides `keboola.variables` rows; `--instance-rename` `{old_path_prefix: new_path_prefix}` renames config dirs + manifest paths), re-points the manifest at the target project, and pushes. Because the reference's config ids do not exist in the fresh target, every config is CREATEd fresh and **keboola.flow task `configId`s + transformation variable links are remapped reference->ULID** by push Phase C/D (the push result carries `flow_task_remaps`). **Idempotent**: re-running with an existing `--target-dir` skips copy/overrides and just pushes, reporting `no_changes` / `created: 0`. Fails fast (`CONFIG_ERROR`) if the target already contains the reference's configs -- clone requires a fresh/empty target. `SyncService.clone_project(...)` returns a typed `CloneResult` for in-process SDK callers. Override files must be flat `{id: scalar}` mappings *(since v0.89.0)* -- a nested mapping, list, or null value is rejected with `CONFIG_ERROR` (exit 5) naming the key and its actual type. -- `sync diff --project ALIAS [--all-projects] [--branch ID]` -- 3-way diff (local vs base vs remote), detects conflicts. `--branch` (0.47.0+) per-invocation dev-branch override. Branch-scoped since v0.89.0 (issue #649): the local side is read from exactly ONE tree (the target branch's subtree, or `main/` when the target has none). Manifest entries belonging to another branch's tree -- what `sync pull --branch ` leaves behind when it re-targets the manifest -- are excluded from the changeset and reported under `orphaned` (`summary.orphaned` + details with `component_id`, `config_id`, `path`, `branch_id`, `branch_path`, `exists_on_target`, `reason`, `hint`); human mode previews the first 10. An orphaned FILE whose `_keboola.config_id` still resolves on the target is adopted (diffed as `unchanged`/`modified`), never re-created; same-tree id claims keep the #482/#497 fork-by-copy CREATE. Fix a non-zero `summary.orphaned` with `sync pull`. +- `sync diff --project ALIAS [--all-projects] [--branch ID]` -- 3-way diff (local vs base vs remote), detects conflicts. `--branch` (0.47.0+) per-invocation dev-branch override. Branch-scoped since v0.89.0 (issue #649): the local side is read from exactly ONE tree (the target branch's subtree, or `main/` when the target has none). Manifest entries belonging to another branch's tree -- what `sync pull --branch ` leaves behind when it re-targets the manifest -- are excluded from the changeset and reported under `orphaned` (`summary.orphaned` + details with `component_id`, `config_id`, `path`, `branch_id`, `branch_path`, `exists_on_target`, `reason`, `hint`); human mode previews the first 10. An orphaned FILE whose `_keboola.config_id` still resolves on the target is adopted (diffed as `unchanged`/`modified`), never re-created; same-tree id claims keep the #482/#497 fork-by-copy CREATE. Fix a non-zero `summary.orphaned` with `sync pull`. **Since vNEXT (#686)** a manifest entry without `metadata.config_hash_version` (written by a pre-vNEXT kbagent) is compared leniently: a stored hash equal to the pre-vNEXT hash of the SAME remote config counts as in sync, so the phantom `codes changed` entries disappear immediately; every other field is still pinned by that hash, so real remote drift is unaffected. One `sync pull` per project stamps the version and ends the leniency. - `sync status [--directory DIR]` -- show locally modified/added/deleted configs. Also surfaces `plaintext_secret_warnings` (since 0.55.0): in-sync configs/rows whose `#`-secrets are still plaintext on the remote (a leftover from pre-0.54.0 writes; #378). Pending (un-pushed) edits are not flagged. Fix = re-push on >=0.54.0 + rotate (version history keeps the plaintext). - `sync branch-link --project ALIAS [--branch-id ID] [--branch-name NAME]` -- link git branch to Keboola dev branch - `sync branch-unlink [--directory DIR]` -- remove git-to-Keboola branch mapping diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index a4e9e0df..d0369cdd 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -458,6 +458,75 @@ Versioning convention: `--allow-plaintext-on-encrypt-failure`, which would write the PAT in plaintext into Storage. +## `sync push` no longer leaves phantom `REMOTE MODIFIED` drift; `transform.sql` carries statement boundaries (since vNEXT, #686) + +`pull_config_hash` in `.keboola/manifest.json` is the 3-way diff's base, and it +means "the hash of this config **as the API returns it**". `sync pull` and the +remote side of `sync diff` always computed it that way. `sync push` computed it +from the **files on disk** instead -- and the two producers disagreed about +`parameters.blocks[].codes[].script`, so every multi-statement SQL +transformation deployed through `sync push` was reported +`~ REMOTE MODIFIED ... codes changed` by every later `sync diff`, forever, with +the working tree byte-identical to the remote. Only a real `sync pull` cleared +it, and the next deploy re-created it (field report: 18 phantom configs hiding +2 real UI changes across 21 projects). + +- **What changed.** Push now stamps the baseline from the API's own view of + what it just wrote -- at all six sites (config create/update, row + create/update, and the Phase C/D link backfills). `pull_hash` and + `pull_extra_hashes` stay disk-derived; they describe local files. +- **The same fix closes the `is_disabled` phantom.** A config (or row) disabled + in the UI whose local YAML has no `is_disabled` key drifted permanently after + every push for the same reason. No workaround needed any more. +- **If the config cannot be read back after the write** (a partial mutation + response AND a failed re-read), `pull_config_hash` is left EXACTLY as it was + and the push envelope carries a `warnings[]` entry telling you to run + `sync pull`. Visibly stale beats confidently wrong -- push never falls back + to a disk-derived baseline. +- **One `script[]` shape now, everywhere: one element = one executable + statement.** That is what the Keboola runtime means by the array + (#119/#120/#274), and `sync push` has produced it since 0.30.x. The API side + used to collapse each code into ONE joined string, which no multi-statement + SQL config could ever match. Non-SQL components (Python, R, custom apps) are + unchanged -- there both sides always agreed on the single joined string. +- **`transform.sql` can now contain `/* ===== STATEMENT ===== */` marker + lines.** They are written only when semicolons alone cannot recover the + statement array -- i.e. when the API's elements carry no trailing `;`. A + `;`-terminated file is byte-identical to what earlier versions wrote, so + existing trees produce no spurious diff. This closes a SILENT failure: before + vNEXT, a script like `["SELECT 1", "SELECT 2"]` lost its boundary on pull and + push rewrote production as ONE statement (the `MULTI_STATEMENT_COUNT=1` crash + shape) while `sync diff` reported "in sync". + - Markers are guaranteed boundaries, but `;` splitting still runs INSIDE each + marked segment -- typing `; SELECT ...` in a marked segment splits + correctly. + - Do not hand-write a line identical to the marker inside a statement: when + extraction sees one it suppresses markers for that code entirely (logging a + warning) rather than writing an ambiguous file. + +**Migration -- one `sync pull` per project, then the noise is gone:** + +- Each manifest entry now carries `metadata.config_hash_version` (currently + `2`) next to a hash the current producer computed. +- An entry WITHOUT that key predates the fix, so it is compared **leniently**: + a stored hash equal to the pre-fix hash of the SAME remote config counts as + in sync. Every other field is pinned by that same hash, so the leniency + cannot hide real drift -- a renamed or edited remote still reports + `REMOTE MODIFIED`. +- `sync pull` re-runs extraction (writing the markers where needed) and stamps + the version, which ends the leniency for that entry. `sync diff` stays + read-only and migrates nothing. +- A migration pull will NOT overwrite an edited `transform.sql` / + `_description.md`: unlike the ordinary overwrite-guard (which only ever + checked `_config.yml`), the migration path checks the companion files too, + and preserves the entry unstamped when any of them changed. +- **`sync push` refuses ONE specific legacy change** with + `SYNC_LEGACY_BOUNDARY` (per-change error, exit 1, the rest of the push + proceeds): a tree pulled before markers existed whose only difference from + the remote is the lost statement boundaries. Pushing it would merge separate + statements into one. Run `sync pull` for that project, then push again. + Genuine SQL edits are never blocked by this guard. + ## `sync push` fresh-CREATE writeback now updates placeholders in place (since v0.47.0) Before v0.47.0, `kbagent sync push` always **appended** new `ManifestConfiguration` diff --git a/plugins/kbagent/skills/kbagent/references/sync-workflow.md b/plugins/kbagent/skills/kbagent/references/sync-workflow.md index b0668116..156eccd9 100644 --- a/plugins/kbagent/skills/kbagent/references/sync-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/sync-workflow.md @@ -416,6 +416,11 @@ Stored in `.keboola/branch-mapping.json`: - **Pull protects local edits**: locally-modified files are skipped by default - **`--force` is conflict-aware (since 0.53.0)**: see below -- it no longer blindly overwrites - **Push only sends local changes**: remote_modified and conflict changes are skipped +- **Push records the API's own view of what it wrote (since vNEXT, #686)**: the + manifest baseline (`pull_config_hash`) comes from the API response (or a + read-back), never from the files on disk. Before vNEXT the two producers + disagreed and every pushed multi-statement SQL transformation showed + permanent phantom `REMOTE MODIFIED` drift - **Encrypted values**: nonce differences are ignored in diff (no false positives) - **New configs**: push auto-assigns IDs from the API, updates manifest - **Storage metadata is read-only**: not tracked in manifest, excluded from diff/push @@ -443,6 +448,39 @@ the 3-way diff state per config (and per row): > stops you loudly instead of losing work. To intentionally drop a local edit, > delete the file (or the config directory) and pull. +## Migrating a tree pulled before vNEXT (#686) + +`sync push` used to stamp the manifest baseline from the files on disk while +`sync pull` / `sync diff` computed it from the API. Any config the two +producers disagreed about -- in practice every SQL transformation with two or +more statements, plus anything disabled in the UI whose local YAML has no +`is_disabled` key -- came back as `~ REMOTE MODIFIED` after every deploy, with +the tree byte-identical to the remote. + +**What to do once per project: `kbagent sync pull --project `.** + +That single pull re-extracts the code files (writing +`/* ===== STATEMENT ===== */` boundary markers where semicolons cannot recover +the statement array) and stamps `metadata.config_hash_version` on each manifest +entry. Commit the resulting `manifest.json` (plus any `transform.sql` that +gained markers) and the phantom entries are gone for good -- no more +content-free "refresh the baseline" PR after each deploy. + +Until that pull happens, kbagent is lenient with unversioned entries: a stored +hash that matches the pre-vNEXT hash of the same remote config is treated as in +sync. The leniency covers ONLY that difference -- a genuinely changed remote +still reports `REMOTE MODIFIED`. + +One case is refused rather than pushed: if a pre-markers tree's only difference +from the remote is the lost statement boundaries, `sync push` aborts THAT change +with `SYNC_LEGACY_BOUNDARY` (other changes in the same push still go through) +because pushing it would merge separate SQL statements into one -- the +`MULTI_STATEMENT_COUNT=1` runtime failure. Pull the project, then push. + +If a config cannot be read back after a successful write, push leaves its +baseline untouched and reports it under `warnings[]` in the result envelope; +run `sync pull` to refresh it. + ## Cloning a reference project (`sync clone`, since v0.63.0) `sync clone` builds a new customer/instance project by **copying a golden From 82c99c866c042915d4aef94961c6eb804f1a9494 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 25 Aug 2026 14:16:40 +0200 Subject: [PATCH 4/5] test(sync): pin the #686 repro table and the broad-SQL-predicate parity Drives the real pull -> push -> diff producers over the four script shapes from the issue and asserts both the sent array and hash parity; the single-element-two-statements row is CHANGED by design (one element = one statement is what the runtime wants, #274), no longer phantom. Also pins the case R1 called out: a SQL backend matched only by the fragment predicate (keboola.exasol-transformation) is not in the extraction set, so its blocks round-trip through the YAML unchanged and both sides of the hash still agree. --- tests/test_sync_code_extraction.py | 103 +++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/tests/test_sync_code_extraction.py b/tests/test_sync_code_extraction.py index d6be4622..128f82b0 100644 --- a/tests/test_sync_code_extraction.py +++ b/tests/test_sync_code_extraction.py @@ -792,3 +792,106 @@ def test_canonical_sql_script_splits_each_element(self) -> None: def test_canonical_sql_script_drops_blank_elements(self) -> None: """Whitespace-only elements vanish, matching the file round-trip.""" assert canonical_sql_script(["SELECT 1;", " ", ""]) == ["SELECT 1;"] + + +class TestPullPushDiffShapeParity: + """The issue #686 repro table, at the hash level. + + Drives the real pull -> push -> diff producers over one code block and + asserts the two sides agree (no phantom drift) and that the statement + array survives the file round-trip. + """ + + @staticmethod + def _api(script: list[str]) -> dict[str, Any]: + return { + "id": "1", + "name": "c", + "description": "", + "configuration": { + "parameters": { + "blocks": [{"name": "B", "codes": [{"name": "C", "script": list(script)}]}] + } + }, + } + + @pytest.mark.parametrize( + ("api_script", "expected_sent"), + [ + # ;-terminated, several elements: unchanged, was PHANTOM before. + (["SELECT 1;", "SELECT 2;"], ["SELECT 1;", "SELECT 2;"]), + # No semicolons: boundaries survive via the marker. Before the fix + # push silently sent ONE element (MULTI_STATEMENT_COUNT=1). + (["SELECT 1", "SELECT 2"], ["SELECT 1", "SELECT 2"]), + # One element packing two statements is NOT a canonical array: the + # runtime wants one statement per element, so it is split. That is + # the #274 normalization, deliberate -- and now no longer phantom. + (["SELECT 1;\nSELECT 2;"], ["SELECT 1;", "SELECT 2;"]), + (["SELECT 1"], ["SELECT 1"]), + ], + ) + def test_push_matches_remote_and_diff_is_in_sync( + self, api_script: list[str], expected_sent: list[str], tmp_path: Path + ) -> None: + from keboola_agent_cli.sync.config_format import api_config_to_local + from keboola_agent_cli.sync.diff_engine import config_hash + + component = "keboola.snowflake-transformation" + config_dir = tmp_path / "cfg" + + local = api_config_to_local(component, self._api(api_script), "1") # pull + extract_code_files(component, local, config_dir) + pushed = copy.deepcopy(local) + merge_code_files(component, pushed, config_dir) # push + sent = pushed["parameters"]["blocks"][0]["codes"][0]["script"] + + assert sent == expected_sent + # diff: remote side of what push just wrote vs the pushed baseline + remote_hash = config_hash(api_config_to_local(component, self._api(sent), "1")) + assert remote_hash == config_hash(pushed) + + +class TestBroadPredicateSqlComponent: + """SQL backends matched only by fragment keep both sides consistent (R1). + + ``keboola.exasol-transformation`` is SQL for normalization purposes but is + not in the exact extraction set, so its blocks stay inside ``_config.yml`` + and the merge is an identity. The split shape must therefore survive + untouched -- the local data IS the normalized data. + """ + + COMPONENT = "keboola.exasol-transformation" + + def test_yaml_identity_round_trip_preserves_split_shape(self, tmp_path: Path) -> None: + from keboola_agent_cli.sync.config_format import api_config_to_local + from keboola_agent_cli.sync.diff_engine import config_hash + + api_config = { + "id": "1", + "name": "c", + "description": "", + "configuration": { + "parameters": { + "blocks": [ + { + "name": "B", + "codes": [{"name": "C", "script": ["SELECT 1;", "SELECT 2;"]}], + } + ] + } + }, + } + config_dir = tmp_path / "exasol" + + local = api_config_to_local(self.COMPONENT, api_config, "1") + extract_code_files(self.COMPONENT, local, config_dir) + # No code file is written -- the blocks stay in the YAML body. + assert not (config_dir / "transform.sql").exists() + assert local["parameters"]["blocks"][0]["codes"][0]["script"] == [ + "SELECT 1;", + "SELECT 2;", + ] + + pushed = copy.deepcopy(local) + merge_code_files(self.COMPONENT, pushed, config_dir) + assert config_hash(pushed) == config_hash(local) From 5115f04bce16e7528df83ea2d98bc2323fc33d49 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 25 Aug 2026 14:20:20 +0200 Subject: [PATCH 5/5] fix(sync): apply the baseline leniency to force-pull conflict detection `sync pull --force` aborts with SYNC_CONFLICT when a config is both locally modified and changed on the remote. It compared the stored baseline strictly, so an entry written before the script-shape change (issue #686) read as "remote changed" and a force-pull aborted on a config nobody had touched remotely -- the third place a stored hash meets a fresh API hash, after the diff base and the pull idempotency check. `_is_conflict` and `_detect_force_pull_conflicts` move to `_sync_baseline` as free functions, joining the other two comparison sites, and the config-level check now runs the stored hash through `effective_stored_hash`. Rows stay strict: their hash producer never changed. sync_service.py shrinks by ~78 code lines in the process. --- .../services/_sync_baseline.py | 138 +++++++++++++++++- .../services/sync_service.py | 111 +------------- tests/test_sync_baseline_stamping.py | 25 ++++ 3 files changed, 166 insertions(+), 108 deletions(-) diff --git a/src/keboola_agent_cli/services/_sync_baseline.py b/src/keboola_agent_cli/services/_sync_baseline.py index 3f0a8629..80463848 100644 --- a/src/keboola_agent_cli/services/_sync_baseline.py +++ b/src/keboola_agent_cli/services/_sync_baseline.py @@ -16,6 +16,11 @@ - :func:`raise_on_legacy_boundary` -- refuses to push a legacy tree whose ``transform.sql`` cannot represent the remote's statement boundaries, which would silently collapse several statements into one. + +:func:`detect_force_pull_conflicts` lives here for the same reason: it is the +third place a stored baseline is weighed against a fresh API hash, and it must +apply the same leniency or a ``--force`` pull would abort on a shape-only +difference. """ from __future__ import annotations @@ -25,7 +30,12 @@ from pathlib import Path from typing import TYPE_CHECKING, Any -from ..constants import CONFIG_HASH_VERSION, CONFIG_HASH_VERSION_KEY +from ..constants import ( + ALWAYS_IGNORED_COMPONENTS, + CONFIG_FILENAME, + CONFIG_HASH_VERSION, + CONFIG_HASH_VERSION_KEY, +) from ..errors import ErrorCode, KeboolaApiError from ..sync.code_extraction import ( is_sql_transformation_component, @@ -404,3 +414,129 @@ def raise_on_legacy_boundary( status_code=0, error_code=ErrorCode.SYNC_LEGACY_BOUNDARY, ) + + +# --------------------------------------------------------------------------- +# Force-pull conflict detection +# --------------------------------------------------------------------------- + + +def _is_conflict( + service: SyncService, + config_file: Path, + old_pull_hash: str, + old_cfg_hash: str, + api_cfg_hash: str, +) -> bool: + """True iff the file is locally modified AND the remote also changed. + + A 3-way conflict needs both a stored ``pull_hash`` (the synced file + state) and a stored ``pull_config_hash`` (the synced remote state); + without either we cannot prove a conflict, so return False -- be + conservative, ``--force`` must not abort on incomplete bookkeeping. + A missing local file is not a content conflict (nothing to lose). + """ + if not old_pull_hash or not old_cfg_hash: + return False + if not config_file.exists(): + return False + locally_modified = service._file_hash(config_file) != old_pull_hash + remote_changed = api_cfg_hash != old_cfg_hash + return locally_modified and remote_changed + + +def detect_force_pull_conflicts( + service: SyncService, + components: list[dict[str, Any]], + branch_dir: Path, + *, + existing_keys: set[str], + existing_paths: dict[str, str], + existing_file_hashes: dict[str, str], + existing_metadata: dict[str, dict[str, Any]], + existing_rows: dict[str, dict[str, str]], +) -> list[dict[str, str]]: + """Return configs/rows a ``--force`` pull would clobber as conflicts. + + A *conflict* is a config (or row) that is BOTH locally modified (its + on-disk ``_config.yml`` hash differs from the manifest ``pull_hash``) + AND changed on the remote since the last pull (the freshly fetched + config hash differs from ``pull_config_hash``). That is the only case + where ``--force`` must stop: local and remote have diverged, so neither + "take remote" nor "keep local" is safe without the user deciding. + + Configs only locally modified (remote unchanged) are NOT conflicts -- + ``--force`` preserves them so their pending delta stays pushable. + Brand-new remote configs and configs whose local file is missing are + skipped (nothing local to lose). Read-only: hashes but writes nothing. + + The stored config hash goes through :func:`effective_stored_hash`, so a + baseline written before the script-shape change (issue #686) is not + mistaken for a remote edit and does not abort an otherwise clean + ``--force`` pull. Rows are compared strictly -- their hash producer never + changed. + """ + conflicts: list[dict[str, str]] = [] + for component in components: + component_id = component.get("id", "") + if component_id in ALWAYS_IGNORED_COMPONENTS: + continue + for cfg in component.get("configurations", []): + config_id = str(cfg.get("id", "")) + lookup_key = f"{component_id}/{config_id}" + if lookup_key not in existing_keys: + continue # brand-new remote config -- nothing local to lose + + rel_path = existing_paths.get(lookup_key, "") + remote_local = api_config_to_local(component_id, cfg, config_id) + if _is_conflict( + service, + branch_dir / rel_path / CONFIG_FILENAME, + existing_file_hashes.get(lookup_key, ""), + effective_stored_hash( + existing_metadata.get(lookup_key, {}), + component_id=component_id, + config_id=config_id, + raw_remote=cfg, + remote_local=remote_local, + ), + config_hash(remote_local), + ): + conflicts.append( + { + "scope": "config", + "component_id": component_id, + "config_id": config_id, + "config_name": str(cfg.get("name", "untitled")), + "path": rel_path, + } + ) + + # Row-level conflicts (same 3-way rule, per row). + config_dir = branch_dir / rel_path + for row in cfg.get("rows", []): + row_id = str(row.get("id", "")) + existing_row = existing_rows.get(f"{component_id}/{config_id}/{row_id}") + if not existing_row: + continue + row_rel_path = existing_row.get("path", "") + if _is_conflict( + service, + config_dir / row_rel_path / CONFIG_FILENAME, + existing_row.get("pull_hash", ""), + existing_row.get("pull_config_hash", ""), + config_hash(api_row_to_local(row, component_id)), + ): + conflicts.append( + { + "scope": "row", + "component_id": component_id, + "config_id": config_id, + "config_name": ( + f"{cfg.get('name', 'untitled')}/{row.get('name', 'untitled')}" + ), + "path": f"{rel_path}/{row_rel_path}", + "row_id": row_id, + } + ) + return conflicts diff --git a/src/keboola_agent_cli/services/sync_service.py b/src/keboola_agent_cli/services/sync_service.py index 9ca51fca..632c9d1e 100644 --- a/src/keboola_agent_cli/services/sync_service.py +++ b/src/keboola_agent_cli/services/sync_service.py @@ -71,6 +71,7 @@ find_plaintext_secret_keys, ) from ._sync_baseline import ( + detect_force_pull_conflicts, effective_stored_hash, extras_modified, needs_shape_migration, @@ -447,111 +448,6 @@ def _local_files_match_pull_state( return False return True - def _is_conflict( - self, - config_file: Path, - old_pull_hash: str, - old_cfg_hash: str, - api_cfg_hash: str, - ) -> bool: - """True iff the file is locally modified AND the remote also changed. - - A 3-way conflict needs both a stored ``pull_hash`` (the synced file - state) and a stored ``pull_config_hash`` (the synced remote state); - without either we cannot prove a conflict, so return False -- be - conservative, ``--force`` must not abort on incomplete bookkeeping. - A missing local file is not a content conflict (nothing to lose). - """ - if not old_pull_hash or not old_cfg_hash: - return False - if not config_file.exists(): - return False - locally_modified = self._file_hash(config_file) != old_pull_hash - remote_changed = api_cfg_hash != old_cfg_hash - return locally_modified and remote_changed - - def _detect_force_pull_conflicts( - self, - components: list[dict[str, Any]], - branch_dir: Path, - *, - existing_keys: set[str], - existing_paths: dict[str, str], - existing_file_hashes: dict[str, str], - existing_config_hashes: dict[str, str], - existing_rows: dict[str, dict[str, str]], - ) -> list[dict[str, str]]: - """Return configs/rows a ``--force`` pull would clobber as conflicts. - - A *conflict* is a config (or row) that is BOTH locally modified (its - on-disk ``_config.yml`` hash differs from the manifest ``pull_hash``) - AND changed on the remote since the last pull (the freshly fetched - config hash differs from ``pull_config_hash``). That is the only case - where ``--force`` must stop: local and remote have diverged, so neither - "take remote" nor "keep local" is safe without the user deciding. - - Configs only locally modified (remote unchanged) are NOT conflicts -- - ``--force`` preserves them so their pending delta stays pushable. - Brand-new remote configs and configs whose local file is missing are - skipped (nothing local to lose). Read-only: hashes but writes nothing. - """ - conflicts: list[dict[str, str]] = [] - for component in components: - component_id = component.get("id", "") - if component_id in ALWAYS_IGNORED_COMPONENTS: - continue - for cfg in component.get("configurations", []): - config_id = str(cfg.get("id", "")) - lookup_key = f"{component_id}/{config_id}" - if lookup_key not in existing_keys: - continue # brand-new remote config -- nothing local to lose - - rel_path = existing_paths.get(lookup_key, "") - api_cfg_hash = config_hash(api_config_to_local(component_id, cfg, config_id)) - if self._is_conflict( - branch_dir / rel_path / CONFIG_FILENAME, - existing_file_hashes.get(lookup_key, ""), - existing_config_hashes.get(lookup_key, ""), - api_cfg_hash, - ): - conflicts.append( - { - "scope": "config", - "component_id": component_id, - "config_id": config_id, - "config_name": str(cfg.get("name", "untitled")), - "path": rel_path, - } - ) - - # Row-level conflicts (same 3-way rule, per row). - config_dir = branch_dir / rel_path - for row in cfg.get("rows", []): - row_id = str(row.get("id", "")) - existing_row = existing_rows.get(f"{component_id}/{config_id}/{row_id}") - if not existing_row: - continue - row_rel_path = existing_row.get("path", "") - if self._is_conflict( - config_dir / row_rel_path / CONFIG_FILENAME, - existing_row.get("pull_hash", ""), - existing_row.get("pull_config_hash", ""), - config_hash(api_row_to_local(row, component_id)), - ): - conflicts.append( - { - "scope": "row", - "component_id": component_id, - "config_id": config_id, - "config_name": ( - f"{cfg.get('name', 'untitled')}/{row.get('name', 'untitled')}" - ), - "path": f"{rel_path}/{row_rel_path}", - "row_id": row_id, - } - ) - return conflicts - def pull( self, alias: str, @@ -715,13 +611,14 @@ def pull( # ``--theirs`` skips the guard entirely: the user explicitly asked for # remote to win, so conflicts are resolved by overwriting, not aborting. if force and not theirs: - conflicts = self._detect_force_pull_conflicts( + conflicts = detect_force_pull_conflicts( + self, components, branch_dir, existing_keys=existing_keys, existing_paths=existing_paths, existing_file_hashes=existing_file_hashes, - existing_config_hashes=existing_config_hashes, + existing_metadata=existing_metadata, existing_rows=existing_rows, ) if conflicts: diff --git a/tests/test_sync_baseline_stamping.py b/tests/test_sync_baseline_stamping.py index 43fb06c8..92884946 100644 --- a/tests/test_sync_baseline_stamping.py +++ b/tests/test_sync_baseline_stamping.py @@ -678,3 +678,28 @@ def test_genuine_edit_on_legacy_tree_still_pushes(tmp_config_dir: Path, tmp_path assert result["errors"] == [] assert result["updated"] == 1 assert _remote_script(api) == ["SELECT 1;", "SELECT 42;"] + + +def test_force_pull_does_not_abort_on_legacy_shape(tmp_config_dir: Path, tmp_path: Path) -> None: + """A shape-only baseline is not a remote change, so --force must not abort.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api) + + # Local edit + a legacy baseline: strict comparison would read the remote + # as "changed" and raise SyncConflictError. + config_file = _config_file(project_root) + data = yaml.safe_load(config_file.read_text(encoding="utf-8")) + data["description"] = "edited locally" + config_file.write_text(yaml.dump(data, default_flow_style=False), encoding="utf-8") + + _service(store, api).pull( + alias="prod", project_root=project_root, force=True, no_storage=True, no_jobs=True + ) + + # The un-pushed edit survives (force preserves a locally-modified config + # whose remote did not move). + assert ( + yaml.safe_load(config_file.read_text(encoding="utf-8"))["description"] == "edited locally" + )