fix(sync): push stamps API-derived baselines; one canonical script[] shape; statement-boundary markers (#686) - #694
Merged
Conversation
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.
`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.
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.
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.
`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.
Member
Author
Live E2E verification (project 5946, e2e-snowflake)Ran an A/B harness ( Control = installed v0.90.1 (validates the instrument — both defects must reproduce): Fix = this branch: Result: 11 pass, 0 fail. The last check strips |
padak
marked this pull request as ready for review
August 25, 2026 13:01
Resolves the #691 (ignoredComponents) overlap: _is_conflict and _detect_force_pull_conflicts moved to _sync_baseline.py by this branch, so main's ignored_components parameter and filter are carried into the moved detect_force_pull_conflicts; _effective_ignored_components stays on SyncService. Doc conflicts combine both sides.
padak
added a commit
that referenced
this pull request
Aug 25, 2026
Follow-up to #694 / issue #686. sync push (push_create, push_update, Phase C variables backfill) now runs normalize_blocks_codes_script — the #274 runtime-safety guard already wired into config update and transformation edit/create — before sending, surfacing normalization records via the push envelope's warnings[] in both human and --json mode. Belt-and-braces: after #694 the guard is a no-op on canonical trees; it protects hand-authored _config.yml bodies that bypass transform.sql.
padak
added a commit
that referenced
this pull request
Aug 25, 2026
CONTRIBUTING.md release step 4 keeps version tags off headings: resolving the placeholder changes the generated anchor slug and breaks inbound links. PR #697 did this pass, but #694 and #696 landed their headings around the same time and fell outside its grep. - gotchas.md: two headings, tag moved to the section's first body line - sync-workflow.md: heading reworded to 'Migrating a legacy sync tree', with the version stated on a body line No inbound links reference the affected anchors (verified by grep).
This was referenced Aug 25, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #686.
What this fixes
Three defects sharing one root cause —
sync pushstamped the manifest baseline (pull_config_hash) from a hash computed from the files on disk, whilesync pullandsync diffcompute it from the API response, and the two sides held contradictory definitions of whatscript[]looks like:sync pushof a multi-statement SQL transformation, every subsequentsync diffreported~ REMOTE MODIFIED … codes changedforever, even though remote and working tree were byte-identical. Only a realsync pullcleared it — until the next deploy re-created it.script[]elements carry no trailing semicolons (["SELECT 1", "SELECT 2"]),transform.sqlhad no statement boundaries left to recover, so push collapsed N statements into ONE element — theMULTI_STATEMENT_COUNT=1crash shape of sync push collapses transformation script[] array → runtime fails #119/fix: preserve multi-element script[] arrays in sync roundtrip #120/config update: re-split script[] elements that contain multiple statements (gap beyond #245) #274 — anddiffthen said in sync.is_disabledphantom (same class, non-script instance): a config disabled in the UI whose local YAML has nois_disabledkey drifted permanently after any push, because the disk-derived baseline lacked the key the API-derived comparison emits.How
Part 1 — push stamps the baseline from the API response (all six sites: config create/update, row create/update, Phase C/D binding backfills). The mutation response is used only when it carries a complete
configuration; an id-only response triggers aget_config_detail()/get_config_row()read-back; if that fails, the baseline is left visibly stale (+ awarnings[]entry in the push envelope) — never silently stamped from disk.pull_hash/pull_extra_hashesstay disk-derived (they describe local files; that is correct).Part 2 — one canonical
script[]shape. Runtime semantics decide: one array element = one executable statement (the premise of #119/#120/#274)._normalize_scriptsis now component-aware — SQL transformations split per-element via the new single producercanonical_sql_script()(never joined first), everything else keeps the join. This is also why existing push-stamped baselines start matching diff without any user action: they were already split-shaped.Part 3 —
transform.sqlcarries statement boundaries when semicolons can't. Extraction verifies the round-trip; only whensplit_statements()cannot reproduce the canonical array does it insert/* ===== STATEMENT ===== */between statements —;-terminated trees stay byte-identical. Markers are guaranteed boundaries on merge, andsplit_statements()still runs within each segment (a hand-typed; SELECT …inside a marked segment splits correctly). A statement whose own text contains a marker-identical line suppresses markers with a warning (documented limitation). Auto-appending missing;was rejected: it changes content and Oracle/ODBC rejects trailing semicolons (ORA-00911).Migration — per-entry
config_hash_version. Part 2 changes the computed hash for pull-stamped baselines of multi-statement SQL configs, so a naive release would hand every existing tree a third flavour of phantom drift. Instead:sync pullmigrates: re-extracts (writing markers where needed) and stampsconfig_hash_version: 2— but only when_config.ymlAND all companion files are unchanged; a locally-modified tree keeps its files, old hash and no version stamp;SYNC_LEGACY_BOUNDARYerror ("runsync pullfirst") instead of silently rewriting production; genuine edits proceed.Verification
The issue's repro table now reads (was
PHANTOM/CHANGEDbefore):Row 3 is deliberate: one element packing two statements is not a canonical array (it is the #274 crash shape); push normalizes it. No file format can express "do not split here", and push has always sent 2 elements for it — only the phantom is gone.
tests/test_sync_baseline_stamping.py(17 tests): the reporter's regression (pull → edit → push → diff in sync), statement-count mirrors with/without semicolons, theis_disabledinstance, partial-response read-back, failed-fetch leaves baseline unstamped + warning, versioned-vs-unversioned migration incl. real-drift discrimination, pull migration preserving an editedtransform.sql, theSYNC_LEGACY_BOUNDARYabort vs genuine-edit passthrough, force-pull leniency (A/B-verified).tests/test_sync_code_extraction.py: marker round-trip, marker collision fallback, within-segment re-split, broad-predicate SQL variants, the full repro table pinned as a parametrized test.make checkgreen: 6189 passed, 12 skipped; ruff/ty/loc-check/error-codes/sentinel-guards all clean (sync_service.pynet −78 code lines).Deliberately updated legacy tests
tests/test_sync_config_format.py::TestNormalizeScripts(7 tests) —_normalize_scriptsnow takescomponent_id; they pass a non-SQL component and keep asserting the join semantics for that branch.tests/test_sync_reconcile.py::test_adopted_by_id_push_registers_manifest_entry— the bare mock now servesget_config_detailfrom the remote fixture (an id-only response no longer counts as a stampable baseline).tests/test_sync_service.py::TestFreshCreateVariableBinding::test_push_resolves_bindings_end_to_end— create mocks gainedget_config_detail/get_config_rowread-backs.Notes for the release PR
changelog.pyentry (per the release process). Doc surfaces (gotchas.md,sync-workflow.md,CLAUDE.md,commands-reference.md,keboola-expert.md) are tagged(since vNEXT).SYNC_LEGACY_BOUNDARY(typeconflict).sync pullonce per project after upgrading to migrate baselines toconfig_hash_version: 2; until then unversioned entries are matched leniently (no worse than the pre-upgrade blindness, and only for the one collapsed-shape difference).Follow-up
A separate PR wires
normalize_blocks_codes_script(the #274 runtime-safety guard) into the sync push path as a belt-and-braces backstop — after parts 2+3 it is a no-op on this path by construction.