From 78a0043cfdd4521b933af5cfa25247dae3f54396 Mon Sep 17 00:00:00 2001 From: soustruh Date: Sat, 10 Oct 2026 03:27:16 +0200 Subject: [PATCH] fix(sync): keep UI edits of adopted configs and encrypt secrets before the first push write (#792 E, F) --- CLAUDE.md | 10 + formal/sync/README.md | 16 +- plugins/kbagent/agents/keboola-expert.md | 7 +- .../kbagent/references/commands-reference.md | 6 +- .../skills/kbagent/references/gotchas.md | 52 ++++ .../kbagent/references/scaffold-workflow.md | 3 + .../kbagent/references/sync-workflow.md | 38 ++- src/keboola_agent_cli/commands/config.py | 12 +- src/keboola_agent_cli/commands/context.py | 18 +- src/keboola_agent_cli/services/_encryption.py | 15 + .../services/_sync_baseline.py | 45 ++- .../services/_sync_push_ops.py | 114 +++++++- .../services/_sync_writeback.py | 6 +- .../services/component_service.py | 33 ++- .../services/sync_service.py | 63 ++-- src/keboola_agent_cli/sync/config_format.py | 7 + src/keboola_agent_cli/sync/diff_engine.py | 19 +- tests/test_component_service.py | 40 +++ tests/test_config_create_cli.py | 24 ++ tests/test_sync_adopted_files.py | 275 ++++++++++++++++++ tests/test_sync_diff_engine.py | 51 ++++ tests/test_sync_formal_counterexamples.py | 67 ++--- tests/test_sync_push_encrypt_first.py | 261 +++++++++++++++++ tests/test_sync_reconcile.py | 30 +- tests/test_sync_service.py | 19 +- 25 files changed, 1095 insertions(+), 136 deletions(-) create mode 100644 tests/test_sync_adopted_files.py create mode 100644 tests/test_sync_push_encrypt_first.py diff --git a/CLAUDE.md b/CLAUDE.md index af707b045..228c90c3f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1026,6 +1026,9 @@ kbagent config new --component-id ID [--name NAME] [--project NAME] [--output-di # the scaffold had no ID and always landed in the default branch tree, so the next # `sync push` created a DUPLICATE (34-config incident). With --configuration, the local file # mirrors the pushed encrypted body -- placeholders would overwrite the remote on next push. +# The scaffold also records _keboola.base_config_hash (#792 E), the created config as the API +# returned it, so `sync diff` compares it 3-way and push never overwrites an edit made in the UI. +# Version gate in gotchas.md. # sync: GitOps -- configs as local files. init/pull/push/diff are filesystem-local (no serve REST surface). kbagent sync init --project ALIAS [--directory DIR] [--git-branching] [--adopt-existing] [--with-workspaces] @@ -1041,6 +1044,13 @@ kbagent sync push --project ALIAS [--all-projects] [--dry-run] [--force] [--allo # under skipped_deletions (+ skipped_deletions_reason), also in --dry-run, whose summary.deleted counts only # what push would delete. A config/row deleted on the remote since the last pull diffs as remote_deleted and # is never re-created (it lands in skipped). Version gate in gotchas.md. +# sync diff/push/pull and adopted files (#792 E): an untracked _config.yml whose _keboola.config_id +# exists on the target has no baseline unless it records _keboola.base_config_hash; without one a +# difference is a conflict (push skips it), two files with one id are both conflicts, and pull keeps +# such a file with local changes (skipped) -- only --theirs overwrites it. Version gate in gotchas.md. +# sync push encryption (#792 F): every #-secret is encrypted before the first write, so an +# ENCRYPTION_FAILED stops push before anything reaches the remote (the message ends with "Nothing +# was pushed.") and a retry creates nothing twice. Version gate in gotchas.md. # sync push workspace delete (CLI-25): in a `syncWorkspaces` tree, a `push --force` that deletes a shared SQL workspace also deletes its SQL editor sessions (every user's, push branch) and their backend workspaces, which `config restore` does not bring back; `push --dry-run --force` lists them (warnings[] workspace_sessions), a plain push lists the workspace under skipped_deletions and touches no session. `--force` is destructive-class (FLAG_ESCALATIONS `sync.push --force`), so `--deny-destructive` / a cli:destructive deny blocks it while a plain push stays write-class. Version gate for this entry lives in gotchas.md. # Partial failures change the exit code (#745): commands that collect per-item failures # and keep going exit 1 (was 0) when at least one item failed: diff --git a/formal/sync/README.md b/formal/sync/README.md index 6ab19bbcd..da9c8ca15 100644 --- a/formal/sync/README.md +++ b/formal/sync/README.md @@ -81,8 +81,8 @@ explores traces of up to 5 actions. BFS returns the shortest counterexample. | I5 pull keeps local work | violated: a remote delete plus a local edit ends with plain or `--force` pull deleting the edited directory silently | | I6 push then diff is clean | violated on `--branch` promote (finding D, since fixed in the engine; the TLA model is unchanged). Holds on production only (30,334 states) | | I8 manifest matches disk | violated: the stale sweep, and a promote write-back that records a `devt/` entry for a file in `main/` | -| I9 an aborted push is atomic | violated (strong reading): the changes before the failing one reached the API and the manifest was never saved | -| I11 no lost remote update | violated: an adopted file with a config id is diffed 2-way, so push reverts a UI edit | +| I9 an aborted push is atomic | violated (strong reading): the changes before the failing one reached the API and the manifest was never saved. Fixed in the code (F: push encrypts before its first write); the model is unchanged | +| I11 no lost remote update | violated: an adopted file with a config id is diffed 2-way, so push reverts a UI edit. Fixed in the code (E); the model is unchanged | | I11b no silent resurrect | violated: a remote delete followed by any push re-creates the config, even with no local edit (S2). Fixed in the code (H); the model is unchanged | | I12 a pull resolves REMOTE MODIFIED | violated: after a cosmetic edit, plain pull skips the file forever and `--force` raises a conflict (S3) | @@ -103,8 +103,8 @@ the regression test for each in `tests/test_sync_formal_counterexamples.py`. | B | Pull compares only `_config.yml`: local edits in `transform.sql`/`code.py`/`_description.md` are silently overwritten when the remote changed, no SYNC_CONFLICT | Lean F6 | HIGH -- **FIXED** (pull's local-modification check now covers every `pull_extra_hashes` companion file) | `test_b_pull_never_overwrites_local_sql_edit` (unmarked regression guard) + `tests/test_sync_pull_companion_files.py` | | C | Remote delete + local edit: pull (plain/`--force`) deletes the locally-edited directory with no conflict | Lean F7, TLA I5 | HIGH -- **fixed** (#792 A/C PR) | `test_c_pull_never_deletes_locally_edited_dir_on_remote_delete` | | D | `sync push --branch dev` (promote) creates another dev copy of a prod-only config on every push. **Fixed** (fix/792-dev-promote-duplicates): on the promote path the target-branch entry shadows the production entry for the same `main/` dir (`sync/branch_scope.py::_promoted_paths`) | TLA I6/I1 | HIGH | `test_d_promote_push_is_idempotent`, `test_d_promote_push_diff_is_clean_and_edits_update_the_dev_copy` (regression guards) | -| E | An untracked file carrying a config id (`config new --push --output-dir` scaffold / adopted orphan) is diffed 2-way: push overwrites a UI edit made after the scaffold was written | TLA I11 | MED | `test_e_adopted_scaffold_push_does_not_overwrite_remote_edit` | -| F | A push aborted by `ENCRYPTION_FAILED` leaves the manifest unsaved; the retry duplicates the change(s) the aborted push already applied | TLA I1b (model trace only; replayed live for this pilot) | MED | `test_f_aborted_push_does_not_duplicate_already_created_config` | +| E | An untracked file carrying a config id (`config new --push --output-dir` scaffold / adopted orphan) is diffed 2-way: push overwrites a UI edit made after the scaffold was written | TLA I11, Lean F4/F5 | MED -- **fixed** (without a baseline a difference is a `conflict`; the scaffold records `_keboola.base_config_hash`; pull keeps such a file) | `test_e_adopted_scaffold_push_does_not_overwrite_remote_edit` + `tests/test_sync_adopted_files.py` | +| F | A push aborted by `ENCRYPTION_FAILED` leaves the manifest unsaved; the retry duplicates the change(s) the aborted push already applied | TLA I1b (model trace only; replayed live for this pilot) | MED -- **fixed** (push encrypts every secret before its first write, so the abort writes nothing) | `test_f_aborted_push_does_not_duplicate_already_created_config` + `tests/test_sync_push_encrypt_first.py` | | G | `sync push` deletes remote configs with no `--force`; the CLI help text says `--force` gates deletion (soft delete to trash since 0.89.0, restorable) | Spec S1, Lean F1, TLA I2b | MED -- **fixed** (push deletes only with `--force`) | `test_g_push_without_force_does_not_delete_remote_config` | | H | A config deleted remotely by another actor is silently re-created by the next push, no warning | Spec S2, Lean F2, TLA I11b | MED -- **fixed** (diff reports `remote_deleted`, push skips it) | `test_h_push_does_not_silently_resurrect_deleted_config` | | I | Moving a config's directory by hand (`mv`/`git mv`) is seen as remote DELETE + CREATE under a new id | Lean F3 | LOW-MED | `test_i_moving_config_dir_is_not_delete_plus_create` | @@ -120,11 +120,11 @@ and is deleted only by `--theirs`. That is what the I2 (sweep half), I5 and I8 TLC/Lean rerun still reports them until the model's `Pull` is updated to match. Their tests are ordinary regression guards now. -E, F and I reproduce on current code and are `xfail(strict=True)` (D, G and H +I reproduces on current code and is `xfail(strict=True)` (D, E, F, G and H are fixed) -- flipping to a hard failure the moment a fix lands is the point: -delete the `xfail` marker to adopt the fix. F's test now reaches the aborted +delete the `xfail` marker to adopt the fix. F's test reaches the aborted create through a promote push, because push no longer re-creates the -remote-deleted config its first version used. J reproduces and -is `xfail`. K is deliberate, documented behavior, so it is an ordinary +remote-deleted config its first version used; since the F fix the aborted +push creates nothing. J reproduces and is `xfail`. K is deliberate, documented behavior, so it is an ordinary (unmarked) regression guard instead. B is fixed: its marker was removed and the test is now an ordinary regression guard. diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index 58984aea1..641a24876 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -326,7 +326,12 @@ its absence is NOT a promise the entry is version-independent (see §1 Rule 6). branch's tree -- `sync pull` to re-target, never push. Since 0.96.1 (#792) `sync push` deletes only with `--force` (else `skipped_deletions`), and a `- REMOTE DELETED` diff line = deleted on the remote, push never re-creates - it: `sync pull`, or `config restore` to keep it. `sync status` + it: `sync pull`, or `config restore` to keep it. Since vNEXT (#792) a + `CONFLICT` on a file that carries a config id but has no manifest entry (a + lost manifest, a `config new --push --output-dir` scaffold of an older + version) means "no baseline": keep the edit aside, `sync pull --theirs`, + apply the edit again, push. An `ENCRYPTION_FAILED` push wrote nothing: fix + the cause and run the same push again. `sync status` 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 0.91.0 -- diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index 880753173..99029ab0c 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -146,7 +146,7 @@ Requires a **super-admin** Manage API token (same kind as `org setup`). Same def - `config delete --project NAME --component-id ID --config-id ID [--branch ID] [--dry-run]` -- SOFT-delete a configuration into the Storage trash (restorable). Since 0.89.0 it locates the config first and a config already in the trash is NOT deleted again -- the raw API purges permanently on a second DELETE (the retry-after-timeout trap); kbagent reports `already_in_trash` and exits 0 instead. `--dry-run` reports the located state without writing. Undo with `config restore`. Permission class `destructive` - `config restore --project NAME --component-id ID --config-id ID [--branch ID]` *(since v0.89.0)* -- restore a trashed configuration (versions, rows and metadata come back). Only works on a config currently in the trash. Permission class `write` - `config trash-list --project NAME [--component-id ID] [--branch ID]` *(since v0.89.0)* -- list configurations in the trash; each row carries `component_id`, `config_id`, `name`, `version` and `deleted_at`, which is exactly what `config restore` needs. Permission class `read` -- `config new --component-id ID [--project NAME] [--name NAME] [--output-dir DIR] [--push --no-files --description D --configuration JSON|@file|- --configuration-file PATH --no-validate --branch ID --dry-run --allow-plaintext-on-encrypt-failure]` -- **two modes**. **Default (no `--push`)**: scaffold new config from component schema; writes files to `--output-dir` or prints to stdout. **No Storage write calls** -- one AI Service read fetches the component schema/examples (that is what `--project` authenticates even without `--push`). **With `--push`** (0.33.0+, requires `--project` + non-empty `--name`): also POSTs to `/v2/storage/components/{cid}/configs` for a one-shot remote create. `#`-prefixed secrets in the pushed body auto-encrypt via the Encryption API first (fail-closed; since 0.54.0, #378; `--allow-plaintext-on-encrypt-failure` overrides). `--no-files` skips the filesystem step entirely (FIIA-style empty-shell pattern). `--configuration` / `--configuration-file` override the POSTed body (default is `{}`, with validation auto-skipped for the default empty shell). `--dry-run` previews the planned POST + validation result without creating. Since 0.89.0, `--push` + `--output-dir` writes the scaffold WITH `_keboola.config_id` into the subtree of the branch the config was created in (registering the branch in the manifest when missing), so the next `sync push` adopts it instead of duplicating (issue #644); with an explicit body the local file mirrors the pushed encrypted configuration. Schema validation runs by default when an explicit body is given (fail-closed: `ConfigError` exit 5 on mismatch) but skips silently if the AI Service has no schema for the component or returns an error; `--no-validate` opts out. Works for ALL component types including `keboola.snowflake-transformation`. **Since 0.90.0** the scaffold mode's optional `--project` really does fall back to a default project when omitted -- on <= 0.89.x it failed with `CONFIG_ERROR: Project 'None' not found` (see `gotchas.md`). **Since 0.93.0 (#684)** that fallback is the `project use` pin, not the first registered project. +- `config new --component-id ID [--project NAME] [--name NAME] [--output-dir DIR] [--push --no-files --description D --configuration JSON|@file|- --configuration-file PATH --no-validate --branch ID --dry-run --allow-plaintext-on-encrypt-failure]` -- **two modes**. **Default (no `--push`)**: scaffold new config from component schema; writes files to `--output-dir` or prints to stdout. **No Storage write calls** -- one AI Service read fetches the component schema/examples (that is what `--project` authenticates even without `--push`). **With `--push`** (0.33.0+, requires `--project` + non-empty `--name`): also POSTs to `/v2/storage/components/{cid}/configs` for a one-shot remote create. `#`-prefixed secrets in the pushed body auto-encrypt via the Encryption API first (fail-closed; since 0.54.0, #378; `--allow-plaintext-on-encrypt-failure` overrides). `--no-files` skips the filesystem step entirely (FIIA-style empty-shell pattern). `--configuration` / `--configuration-file` override the POSTed body (default is `{}`, with validation auto-skipped for the default empty shell). `--dry-run` previews the planned POST + validation result without creating. Since 0.89.0, `--push` + `--output-dir` writes the scaffold WITH `_keboola.config_id` into the subtree of the branch the config was created in (registering the branch in the manifest when missing), so the next `sync push` adopts it instead of duplicating (issue #644); with an explicit body the local file mirrors the pushed encrypted configuration. Since vNEXT (#792) the scaffold also records `_keboola.base_config_hash` (the created config as the API returned it), so `sync diff` compares it 3-way and push never overwrites an edit made in the UI (see `sync-workflow.md` > "Files with a config id and no manifest entry"). Schema validation runs by default when an explicit body is given (fail-closed: `ConfigError` exit 5 on mismatch) but skips silently if the AI Service has no schema for the component or returns an error; `--no-validate` opts out. Works for ALL component types including `keboola.snowflake-transformation`. **Since 0.90.0** the scaffold mode's optional `--project` really does fall back to a default project when omitted -- on <= 0.89.x it failed with `CONFIG_ERROR: Project 'None' not found` (see `gotchas.md`). **Since 0.93.0 (#684)** that fallback is the `project use` pin, not the first registered project. - `config clone --project P --component-id ID --config-id ID --name NAME [--target-project P2] [--description D] [--set PATH=VALUE ...] [--secret PATH=VALUE ...] [--branch ID] [--target-branch ID] [--dry-run] [--allow-plaintext-on-encrypt-failure]` (0.84.2+, #587) -- duplicate a configuration **whole**. Reach for this instead of reading `config detail` and rebuilding a body: copying only `configuration["parameters"]` silently drops its siblings (`runtime`, `storage`, `authorization`), and a lost `runtime.parallelism` makes Keboola fall back to `parallelism: 1` -- the reporter's 65-row writer went sequential, 140 min instead of ~60-90, with nothing in any output pointing at it. **Same project** (default): server-side copy via `POST .../configs/{id}/versions/{v}/create`; rows and `KBC::` encrypted values travel with it (verified live). `--set PATH=VALUE` is applied as a follow-up update on the copy, so an override can never be the reason a key went missing. **Cross project** (`--target-project`): reassembled client-side and rows recreated one by one, because encrypted values **cannot** travel -- a Keboola ciphertext is scoped to the project it was encrypted in. Any `KBC::` value makes the clone **fail with exit 5**, listing every path, until re-supplied via `--secret PATH=VALUE` (encrypted in the TARGET project on write). `--dry-run` reports those paths instead of refusing -- run it first to learn what to gather. Storage bucket/table IDs are copied **verbatim, never remapped**; `sync clone` is the command that remaps. - `config variables-set --project NAME --component-id ID --config-id ID --var KEY=VALUE [--var ...] [--replace] [--variables-id ID] [--values-id ID] [--branch ID] [--dry-run] [--allow-plaintext-on-encrypt-failure] [--yes]` -- attach variable values to a config. Auto-creates a sibling `keboola.variables` config + default row on first use and links it via the parent's `runtime.variables_id` / `variables_values_id`. Defaults to merge; `--replace` drops keys not in `--var`. `#`-prefixed values encrypt via the Encryption API (fail-closed; exit non-zero on `ENCRYPTION_FAILED`). See `variables-workflow.md` - `config variables-get --project NAME --component-id ID --config-id ID [--branch ID]` -- resolve `variables_id` + `values_id` from the parent config and fetch the current KEY=VALUE map. Returns `{linked: bool, variables_id, values_id, values}`; `linked=false` means the parent has no variables attached @@ -363,9 +363,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] [--with-workspaces]` -- initialize sync working directory; `--adopt-existing` adopts a `.keboola/manifest.json` already written by the kbc Go CLI without overwriting (idempotent; validates `project_id` against the alias token). `--with-workspaces` *(since 0.96.1)* sets `syncWorkspaces` in the manifest, so pull/diff/push/clone also sync shared SQL workspaces (`keboola.sandboxes` without `parameters.id`, `runtime.shared: true`), config only; a `push --force` delete removes the workspace's SQL editor sessions first (a plain push holds it back under `skipped_deletions`). With `--adopt-existing` it turns the key on in an existing manifest. See `sync-workflow.md` > "Shared SQL workspaces". - `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. **Auto-inits:** if the target directory has no `.keboola/manifest.json`, pull runs `init` first, so a separate `sync init` is not needed for a first checkout of a project. 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` is kept through pull and push (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. Ignored components (since 0.91.0): `keboola.sandboxes` + `keboola.mcp-server-tool` are always excluded (`keboola.sandboxes` shared SQL workspaces are synced when the manifest sets `syncWorkspaces`, *since 0.96.1*), unioned with the manifest's `ignoredComponents` list; a component newly ignored has its manifest entry dropped and local directory removed, reported with pull action `"ignored"` (distinct from `"removed"` = genuinely deleted on remote). Config-folder round-trip *(since 0.94.0)*: pull captures each config's UI folder (`KBC.configuration.folderName`, from the branch-only `search/component-configurations` endpoint) into the manifest, and reports `folder_lookup_failed` when that lookup fails, keeping the previously captured folder. **Since 0.98.0 (#745)** `--all-projects` exits **1** when `summary.failed > 0` (a project whose pull raised); `--dry-run` exits **1** on a failure too. -- `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). Workspace delete *(since 0.96.1)*: in a `syncWorkspaces` tree, `push --force` deleting a shared SQL workspace also deletes its SQL editor sessions (every user's, push branch) and their backend workspaces, which `config restore` does not bring back, so check `push --dry-run --force` (`warnings[]` `workspace_sessions`) first; `--force` is destructive-class (`sync.push --force`, blocked by `--deny-destructive`). 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 add a `variable_link` entry in `errors[]`, never a broken link). Since 0.96.1 push also remaps shared-code links, flow and orchestrator task `configId`s / `configRowIds` and schedule targets to the configs created in the same push. A link it cannot set is an `errors[]` entry with `change_type` `shared_code_link`, `flow_task_link` or `schedule_target_link` (error code `LINK_UNRESOLVED` for a row id without a new row, `API_ERROR` for a failed PUT). After a failed PUT the local files already hold the new ids and the manifest hash stays stale, so the next `sync push` sends them; this also applies to `variable_link`. The result carries `link_remaps` (`flow_tasks`, `orchestrator_tasks`, `schedule_targets`, `shared_code`, `config_row_ids`); `flow_task_remaps` counts flow tasks only. 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 deletes on `push --force`; since 0.96.1 (#792) a plain push deletes nothing and lists the deletion under `skipped_deletions` (also in `--dry-run`), and a config or row deleted on the remote since the last pull is `remote_deleted`, which push never re-creates. 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 0.91.0 (#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. Ignored components (since 0.91.0) are filtered out on both sides of the diff push builds on, so a stale local directory for an ignored component (e.g. `keboola.mcp-server-tool`) is never classified as `DELETED` and can never be pushed as a remote deletion. **Since 0.98.0 (#745)** a push with a non-empty `errors[]` exits **1** and the human headline reads `Failed: Pushed: N created, ..., M failed` instead of a green `Success:`. `--all-projects` exits 1 when `summary.failed > 0`, and `summary.failed` now also counts a project whose push returned a non-empty `errors[]` (before, it counted as `success` and its line read `OK`; now the line is marked `x` and states the failed count). A `--dry-run` exits 1 on a failure too. `--json` output is still emitted before the non-zero exit. +- `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). Workspace delete *(since 0.96.1)*: in a `syncWorkspaces` tree, `push --force` deleting a shared SQL workspace also deletes its SQL editor sessions (every user's, push branch) and their backend workspaces, which `config restore` does not bring back, so check `push --dry-run --force` (`warnings[]` `workspace_sessions`) first; `--force` is destructive-class (`sync.push --force`, blocked by `--deny-destructive`). 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 add a `variable_link` entry in `errors[]`, never a broken link). Since 0.96.1 push also remaps shared-code links, flow and orchestrator task `configId`s / `configRowIds` and schedule targets to the configs created in the same push. A link it cannot set is an `errors[]` entry with `change_type` `shared_code_link`, `flow_task_link` or `schedule_target_link` (error code `LINK_UNRESOLVED` for a row id without a new row, `API_ERROR` for a failed PUT). After a failed PUT the local files already hold the new ids and the manifest hash stays stale, so the next `sync push` sends them; this also applies to `variable_link`. The result carries `link_remaps` (`flow_tasks`, `orchestrator_tasks`, `schedule_targets`, `shared_code`, `config_row_ids`); `flow_task_remaps` counts flow tasks only. 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 deletes on `push --force`; since 0.96.1 (#792) a plain push deletes nothing and lists the deletion under `skipped_deletions` (also in `--dry-run`), and a config or row deleted on the remote since the last pull is `remote_deleted`, which push never re-creates. 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. Since vNEXT (#792) such a file without a baseline is a `conflict` when it differs from the remote, so push skips it. Since vNEXT (#792) push encrypts every secret before its first write: an `ENCRYPTION_FAILED` stops it before anything reaches the remote (message ends with `Nothing was pushed.`), so the retry creates nothing twice. `--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 0.91.0 (#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. Ignored components (since 0.91.0) are filtered out on both sides of the diff push builds on, so a stale local directory for an ignored component (e.g. `keboola.mcp-server-tool`) is never classified as `DELETED` and can never be pushed as a remote deletion. **Since 0.98.0 (#745)** a push with a non-empty `errors[]` exits **1** and the human headline reads `Failed: Pushed: N created, ..., M failed` instead of a green `Success:`. `--all-projects` exits 1 when `summary.failed > 0`, and `summary.failed` now also counts a project whose push returned a non-empty `errors[]` (before, it counted as `success` and its line read `OK`; now the line is marked `x` and states the failed count). A `--dry-run` exits 1 on a failure too. `--json` output is still emitted before the non-zero exit. - `sync clone --source DIR --target ALIAS --target-dir DIR [--bucket-map FILE] [--variable-values FILE] [--instance-rename FILE] [--no-create-buckets] [--dry-run] [--branch ID]` -- clone a reference synced project into a **fresh** target project and parameterize it. 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`). Since 0.96.1 push also remaps shared-code links (`shared_code_id`, `shared_code_row_ids`, `{{}}` script placeholders), legacy `keboola.orchestrator` task `configId`s, task `configRowIds` and a schedule's `target.configurationId`; the clone result carries `link_remaps` per kind (see `sync push`). **`warnings[]` (since 0.96.1)**: the clone result (also `--dry-run`; human mode prints them) lists `missing_task_target` (a flow or orchestrator task runs a config that is not in the tree), `encrypted_values_copied` (the `KBC::` paths the target cannot decrypt, as `_config.yml` paths: `secret_keys` get a plaintext + `sync push`, `unencryptable_keys` need `kbagent encrypt values`, `oauth_keys` a new authorization), `data_app_not_deployed` (run `data-app deploy`) and `schedule_not_active` (clone never activates a schedule; `flow schedule` does, and the hint is left out when several schedules run one flow), plus the push warnings. Only the run that creates the configs reports them: a re-run returns `warnings: []`, so keep them from the first run. **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. `--branch` is optional on a fresh clone *(since v0.93.1)*. It defaults to the target's production branch, resolved from the API the same way `sync init` does. Pass `--branch ` only to target a dev branch. The config folder (`KBC.configuration.folderName`) is recreated in the target *(since 0.94.0)* -- clone re-points its production configs onto the branch push resolves, so the create-path writeback carries the folder for a plain, `--branch`, and git-branching production clone. **Data-app runtime type (since 0.94.0)**: a `keboola.data-apps` config's type (`python-js` / `streamlit`) lives only on the Data Science `/apps` record, so `sync pull` records it in `_keboola.data_app_type` and clone sends it through `create_app`. A config with no recorded type is created as `python-js`, the default, with a `data_app_type_default` warning *(since 0.96.1)*. Re-pull a tree pulled by an older version before cloning a Streamlit app, or it is created as `python-js`. **Storage buckets (since 0.96.1)**: clone copies configs, not storage, so a cloned config's input/output mappings point at buckets a fresh target lacks. Clone reads the `storage/buckets.json` pull export and creates the missing buckets in the target **by default** (`--no-create-buckets` skips it) -- idempotent (an existing bucket is skipped, a per-bucket API failure is collected in `bucket_errors`), and the created id is `--bucket-map`-remapped so it matches the rewritten config refs. Each bucket is created on the backend the export recorded. A linked (shared) bucket is linked to the same source as in the reference, under the same id, and listed in `linked_buckets` with its source (an empty bucket in its place would stay empty). The source project's sharing settings decide whether the target may link it; a refused link lands in `bucket_errors`. A tree pulled by an older version does not record which buckets are linked, so clone creates no bucket from it and records one `bucket_errors` entry -- re-pull the reference, or pass `--no-create-buckets`. Only the buckets are created, never their tables or data (the export has no table data) -- populate tables by bucket sharing / `storage upload-table`, or by running the flows. **Since 0.98.0 (#745)** a clone with a non-empty `errors[]` or `bucket_errors[]` exits **1** and the human headline reads `Failed: ...` with the failed count -- a clone where every config failed used to print `Success ... 0 created` and exit 0. A re-run (`no_changes`) with a `bucket_errors[]` entry exits 1 too. -- `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 0.91.0 (#686)** a manifest entry without `metadata.config_hash_version` (written by a pre-0.91.0 kbagent) is compared leniently: a stored hash equal to the pre-0.91.0 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. Ignored components (since 0.91.0) -- `keboola.sandboxes`, `keboola.mcp-server-tool`, and anything listed in the manifest's `ignoredComponents` -- are excluded from BOTH sides of the comparison, so a stale local directory for one of them never shows up as `DELETED`. Since 0.96.1 (#792) a config or row the manifest fetched from the target branch and that is missing on the remote is `remote_deleted` (human: `- REMOTE DELETED`, `summary.remote_deleted`): run `sync pull`, push never re-creates it. `DELETED` changes are applied only by `sync push --force`. **Since 0.98.0 (#745)** `--all-projects` exits **1** when `summary.failed > 0` (a project whose diff failed), like `sync pull --all-projects`. +- `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 -- since vNEXT (#792) an adopted file without a baseline (`_keboola.base_config_hash`) is `conflict` when it differs, never `modified`; same-tree id claims keep the #482/#497 fork-by-copy CREATE. Fix a non-zero `summary.orphaned` with `sync pull`. **Since 0.91.0 (#686)** a manifest entry without `metadata.config_hash_version` (written by a pre-0.91.0 kbagent) is compared leniently: a stored hash equal to the pre-0.91.0 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. Ignored components (since 0.91.0) -- `keboola.sandboxes`, `keboola.mcp-server-tool`, and anything listed in the manifest's `ignoredComponents` -- are excluded from BOTH sides of the comparison, so a stale local directory for one of them never shows up as `DELETED`. Since 0.96.1 (#792) a config or row the manifest fetched from the target branch and that is missing on the remote is `remote_deleted` (human: `- REMOTE DELETED`, `summary.remote_deleted`): run `sync pull`, push never re-creates it. `DELETED` changes are applied only by `sync push --force`. **Since 0.98.0 (#745)** `--all-projects` exits **1** when `summary.failed > 0` (a project whose diff failed), like `sync pull --all-projects`. - `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 c9149ba70..389a73cbf 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -5711,6 +5711,58 @@ drops manifest entries whose config is gone from the remote) are closed: created as `remote_deleted`. Delete that target directory and run the clone again. +## `sync` and a file that carries a config id but has no manifest entry (#792) + +*(since vNEXT, #792)* A `_config.yml` whose `_keboola.config_id` exists on the +target branch, but that has no manifest entry, is *adopted*: `sync diff` +compares it with that remote config, and `sync push` never creates a +duplicate of it. Examples: a `config new --push --output-dir` scaffold, a tree +whose manifest was lost or written again, a `main/` file that +`sync pull --branch` left behind. Such a file has no sync baseline. + +- **No baseline: a difference is a `conflict`.** Before, diff read every + difference as a local edit (`modified`), so `sync push` overwrote an edit + made in the UI after the file was written. Now diff reports `conflict`, and + push skips it (`skipped`, `skipped_reason`). Two files that carry the same id + are both a `conflict`. Before, push sent both and the last one won. +- **A `config new --push --output-dir` scaffold records its baseline** in + `_keboola.base_config_hash`: the created config as the API returned it. Its + diff is 3-way: an edit of the scaffold is `modified` and push applies it, an + edit in the UI is `remote_modified`, an edit on both sides is a `conflict`. + After the first push the manifest holds the baseline, and push removes the + key from the file. A scaffold written by an older kbagent has no key, so a + difference from the remote is a `conflict`. +- **Pull keeps an adopted file with local changes.** Before, pull treated the + file as a new config and overwrote it with no message. Now plain pull and + `pull --force` keep the file, and report it as `skipped` with reason + `locally modified, not tracked in the manifest`. The file stays untracked. + "Local changes" means a difference from `_keboola.base_config_hash`, or, + when the file has no such key, any difference from the remote. A scaffold + with no local changes gets the remote version, as before. +- **To resolve such a `conflict`:** keep a copy of the local edit if you need + it, run `kbagent sync pull --theirs` (the remote wins and the file becomes + tracked), apply the edit again, and push. +- Pull protects only a file in the directory where pull writes that config. A + file with the same config id in another directory stays a separate copy + after the pull, and push creates it as a new config (fork by copy, #482). + +## `sync push` encrypts every secret before its first write (#792) + +*(since vNEXT, #792)* Before, push encrypted the `#` secrets of each config +just before its own write. An `ENCRYPTION_FAILED` on a later config stopped +the push after earlier configs were created or updated, and the manifest did +not record them. The retry then created those configs again. + +- Now push encrypts the secrets of every config and row that it writes before + the first write, one Encryption API call per component. An + `ENCRYPTION_FAILED` stops the push before anything reaches the remote, and + the error message ends with `Nothing was pushed.` The remote, the manifest + and the local files do not change. Fix the cause and run the same push + again. +- With `--allow-plaintext-on-encrypt-failure` nothing changes: push does not + encrypt in advance, and a failed encryption writes plaintext with a warning. +- `--dry-run` calls no Encryption API, as before. + ## `sync` can sync shared SQL workspaces, opt-in per tree (CLI-25) *(since 0.96.1)* `keboola.sandboxes` is no longer skipped when the manifest sets diff --git a/plugins/kbagent/skills/kbagent/references/scaffold-workflow.md b/plugins/kbagent/skills/kbagent/references/scaffold-workflow.md index dd21d6756..16ce488bb 100644 --- a/plugins/kbagent/skills/kbagent/references/scaffold-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/scaffold-workflow.md @@ -56,6 +56,9 @@ kbagent --json config new --component-id COMPONENT_ID --project ALIAS --name "Co # (_keboola.config_id) and lands in the subtree of the branch the config was # created in -- the next `sync push` ADOPTS the config (reported as # `modified` until you edit + push) instead of creating a duplicate. +# Since vNEXT (#792) it also records _keboola.base_config_hash, the created +# config as the API returned it: an edit made in the UI before your push is +# reported as `remote_modified` / `conflict` and push does not overwrite it. # On older versions this combo wrote an ID-less scaffold: the next # `sync push` DUPLICATED the config (issue #644). There, use the two-step # path (scaffold without --push, edit, `sync push`) instead. diff --git a/plugins/kbagent/skills/kbagent/references/sync-workflow.md b/plugins/kbagent/skills/kbagent/references/sync-workflow.md index 41a5775fc..d5732b95f 100644 --- a/plugins/kbagent/skills/kbagent/references/sync-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/sync-workflow.md @@ -178,7 +178,10 @@ kbagent sync diff --project prod # target: production - Configs whose id **does** exist on the target branch are still diffed, from the file in the target's own tree (`main/`) — so an ordinary production diff - after a dev pull reports `unchanged`, not a create. + after a dev pull reports `unchanged`, not a create. Such a file has no + baseline for the target, so a file that differs is a `conflict`, and push + does not send it *(since vNEXT, #792)*; see "Files with a config id and no + manifest entry" below. - Configs that exist **only** on the dev branch are excluded entirely. Promote them with `kbagent branch merge`, never by pushing them to production. - `--json` carries the full list under `orphaned` (`component_id`, `config_id`, @@ -416,7 +419,7 @@ Stored in `.keboola/branch-mapping.json`: |------------|---------|--------| | MODIFIED | Local changed, remote unchanged | Safe to push | | REMOTE MODIFIED | Remote changed, local unchanged | Run pull to fetch | -| CONFLICT | Both sides changed | Resolve manually, then push | +| CONFLICT | Both sides changed, or a file with a config id and no manifest entry differs from the remote *(since vNEXT, #792)* | Resolve manually, then push | | ADDED | New local config | Push creates it | | DELETED | Local file removed | `push --force` deletes from remote; a plain push lists it under `skipped_deletions` *(since 0.96.1)* | | REMOTE DELETED | Deleted on the remote since the last pull | Run pull; push never re-creates it *(since 0.96.1)* | @@ -429,10 +432,20 @@ Stored in `.keboola/branch-mapping.json`: an edit to a companion file (`transform.sql`, `code.py`, `_description.md`, ...) protects the config the same way *(since 0.96.1, #792)*. Before, such an edit was silently overwritten whenever the remote changed, plain or `--force` +- **Pull keeps an untracked file that carries the config id** and has local + changes (`skipped`, reason `locally modified, not tracked in the manifest`) + *(since vNEXT, #792)*; only `--theirs` overwrites it. Before, pull + overwrote it with no message. See "Files with a config id and no manifest + entry" below - **`--force` is conflict-aware**: see below -- it no longer blindly overwrites - **Push only sends local changes**: remote_modified, conflict and remote_deleted changes are skipped (`skipped` in the result), and local deletions are applied only with `--force` (`skipped_deletions` otherwise) *(since 0.96.1, #792)* +- **Push encrypts every secret before its first write** *(since vNEXT, #792)*: + an `ENCRYPTION_FAILED` stops the push before anything reaches the remote + (the message ends with `Nothing was pushed.`), so running the same push + again after the fix is safe. Before, the push stopped after earlier configs + were created, and the retry created them again - **Push records the API's own view of what it wrote (since 0.91.0, #686)**: the manifest baseline (`pull_config_hash`) comes from the API response (or a read-back), never from the files on disk. Before 0.91.0 the two producers @@ -452,6 +465,27 @@ Stored in `.keboola/branch-mapping.json`: - **Data samples auto-trim**: tables with >30 columns export only first 30 (API sync limit) - **Encrypted columns masked**: columns starting with `#` show `***ENCRYPTED***` in samples +## Files with a config id and no manifest entry + +*(since vNEXT, #792)* A `_config.yml` whose `_keboola.config_id` exists on the +target branch but has no manifest entry is adopted: diff compares it with that +remote config, and push never creates a duplicate. A `config new --push +--output-dir` scaffold is such a file, and so is a file in a tree whose +manifest was lost. + +| The file | `sync diff` / `sync push` | `sync pull` (plain, `--force`) | +|---|---|---| +| Records `_keboola.base_config_hash` (a scaffold) | 3-way against that hash: `modified` is pushed, `remote_modified` and `conflict` are skipped | Local changes: kept, `skipped`. No local changes: remote version written, file tracked | +| Has no baseline, same as the remote | `unchanged` | Remote version written, file tracked | +| Has no baseline, differs from the remote | `conflict`, skipped by push | Kept, `skipped` | +| Two files with the same id | Both `conflict` | The file in the directory pull writes to: as above. The other file: not changed | + +`sync pull --theirs` writes the remote version over the file in every case, +and the manifest then tracks it. After the first push of a scaffold, the +manifest holds the baseline and push removes `base_config_hash` from the +file. Older versions read every difference as `modified`, so push overwrote +an edit made in the UI, and pull overwrote a local edit. + ## Ignored components *(since 0.91.0)* Some components are excluded from every sync operation (`pull`, `diff`, diff --git a/src/keboola_agent_cli/commands/config.py b/src/keboola_agent_cli/commands/config.py index 0f9fb61db..a25076096 100644 --- a/src/keboola_agent_cli/commands/config.py +++ b/src/keboola_agent_cli/commands/config.py @@ -20,7 +20,11 @@ from ..effective_branch import resolve_branch from ..errors import ConfigError, ErrorCode, KeboolaApiError from ..output import format_config_detail, format_configs_table, format_search_results -from ..services.component_service import materialize_pushed_config, stamp_scaffold_config_id +from ..services.component_service import ( + materialize_pushed_config, + pushed_config_base_hash, + stamp_scaffold_config_id, +) from ..services.config_service import validate_set_paths from ..sync.branch_registry import default_branch_prefix from ._helpers import ( @@ -1427,7 +1431,11 @@ def config_new( written_dir = target_dir else: if created_id: - scaffold = stamp_scaffold_config_id(scaffold, created_id) + scaffold = stamp_scaffold_config_id( + scaffold, + created_id, + pushed_config_base_hash(component_id, created_id, push_result), + ) written_dir = _write_scaffold_to_disk( formatter, scaffold, diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index 17fbca623..9c5368eca 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -509,7 +509,9 @@ instead of creating a duplicate. With an explicit --configuration body the local directory mirrors the pushed (already encrypted) body exactly like sync pull would materialize it (real transform.sql/.py extracted from the body) instead of - placeholder scaffolding. + placeholder scaffolding. The scaffold also records _keboola.base_config_hash (the + created config as the API returned it; since vNEXT, #792 E), so sync diff compares + it 3-way and push never overwrites an edit made in the UI. kbagent config clone --project P --component-id ID --config-id ID --name NAME [--target-project P2] [--description D] [--set PATH=VALUE ...] @@ -1687,9 +1689,13 @@ (summary.orphaned + details: 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 (unchanged/modified), - never re-created; a same-tree id claim keeps the #482 fork-by-copy CREATE. Fix a - non-zero summary.orphaned with `sync pull`; promote dev-only configs with - `branch merge`, never by pushing them to production. + never re-created; a same-tree id claim keeps the #482 fork-by-copy CREATE. An adopted + file has no baseline unless it records _keboola.base_config_hash (a config new --push + scaffold): without one a difference is a CONFLICT, never modified, and two files with + one id are both conflicts (since vNEXT, #792 E). sync pull keeps such a file with + local changes (skipped, "locally modified, not tracked in the manifest"); --theirs + overwrites it. Fix a non-zero summary.orphaned with `sync pull`; promote dev-only + configs with `branch merge`, never by pushing them to production. Ignored components (since 0.91.0, #689) are excluded from BOTH sides of the comparison, so a stale manifest entry or leftover dir for one of them contributes nothing -- not added, not deleted, not orphaned. Push builds on this diff, so it plans nothing @@ -1698,6 +1704,10 @@ kbagent 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. Skips conflicts (pull first). Fails if encryption fails (plaintext secrets never pushed). Use escape hatch flag only if you know what you are doing. + Every secret is encrypted BEFORE the first write (since vNEXT, #792 F): an + ENCRYPTION_FAILED stops the push before anything reaches the remote (message ends + "Nothing was pushed."), so run the same push again after the fix -- it creates + nothing twice. Workspace delete (since 0.96.1, syncWorkspaces trees): a --force push that deletes a shared SQL workspace also deletes its SQL editor sessions (every user's, push branch) and their backend workspaces, which config restore does not bring back; check diff --git a/src/keboola_agent_cli/services/_encryption.py b/src/keboola_agent_cli/services/_encryption.py index 5b43231fc..a5d546909 100644 --- a/src/keboola_agent_cli/services/_encryption.py +++ b/src/keboola_agent_cli/services/_encryption.py @@ -18,6 +18,10 @@ _ENCRYPTED_PREFIX = "KBC::" +# ``(component_id, plaintext) -> ciphertext`` of values encrypted in advance, +# so a write can reuse them without a second Encryption API call. +SecretCache = dict[tuple[str, str], str] + def is_secret_key(key: object) -> TypeGuard[str]: """True if ``key`` is a Keboola secret key (a ``str`` starting with ``#``). @@ -141,6 +145,7 @@ def encrypt_secrets_in_config( configuration: dict[str, Any], *, allow_plaintext_fallback: bool = False, + known: SecretCache | None = None, ) -> dict[str, Any]: """Encrypt ``#``-prefixed secret values in ``configuration`` in place. @@ -163,12 +168,22 @@ def encrypt_secrets_in_config( allow_plaintext_fallback: When ``False`` (default), raise on encryption failure. When ``True``, log a warning and return the config with plaintext intact. + known: Values already encrypted for this project. They are applied + without an API call; only the other values are sent. """ if not project_id: return configuration secrets: dict[str, str] = {} collect_secrets(configuration, "", secrets) + if known: + cached = { + key: known[(component_id, value)] + for key, value in secrets.items() + if (component_id, value) in known + } + apply_encrypted(configuration, "", cached) + secrets = {key: value for key, value in secrets.items() if key not in cached} if not secrets: return configuration diff --git a/src/keboola_agent_cli/services/_sync_baseline.py b/src/keboola_agent_cli/services/_sync_baseline.py index 7c231900d..5b7dae353 100644 --- a/src/keboola_agent_cli/services/_sync_baseline.py +++ b/src/keboola_agent_cli/services/_sync_baseline.py @@ -41,7 +41,7 @@ marker_less_roundtrip, merge_code_files, ) -from ..sync.config_format import api_config_to_local, api_row_to_local +from ..sync.config_format import BASE_CONFIG_HASH_KEY, api_config_to_local, api_row_to_local from ..sync.diff_engine import config_hash from ..sync.manifest import Manifest @@ -304,6 +304,49 @@ def config_locally_modified( return extras_modified(service, config_dir, extra_hashes) +# Pull ``details[].reason`` for a kept file that only carries the config id. +ADOPTED_SKIP_REASON = "locally modified, not tracked in the manifest" + + +def adopted_skip_detail( + service: SyncService, + config_dir: Path, + component_id: str, + remote: dict[str, Any], + rel_path: str, +) -> dict[str, str] | None: + """Pull's ``skipped`` detail for an untracked local file of *remote* with local changes. + + Such a file carries the config's ``_keboola.config_id`` but has no manifest + entry: a ``config new --push --output-dir`` scaffold, or a tree whose + manifest was lost. Pull treated it as a new config and overwrote it, so a + local edit was lost without a message (issue #792 E). Its baseline is the + ``base_config_hash`` the scaffold records. Without one, pull cannot tell + which side changed, so any difference from the remote counts as a local + change. Returns ``None`` when pull may write the remote version. + """ + config_id = str(remote.get("id", "")) + local = service._read_config_file(config_dir) + meta = (local or {}).get("_keboola") or {} + if local is None or meta.get("component_id") != component_id: + return None + if str(meta.get("config_id", "")) != config_id: + return None + merge_code_files(component_id, local, config_dir) + base = meta.get(BASE_CONFIG_HASH_KEY) or config_hash( + api_config_to_local(component_id, remote, config_id) + ) + if config_hash(local) == base: + return None + return { + "action": "skipped", + "component_id": component_id, + "config_name": remote.get("name", "untitled"), + "path": rel_path, + "reason": ADOPTED_SKIP_REASON, + } + + 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") diff --git a/src/keboola_agent_cli/services/_sync_push_ops.py b/src/keboola_agent_cli/services/_sync_push_ops.py index 26cdbce8d..74634fdce 100644 --- a/src/keboola_agent_cli/services/_sync_push_ops.py +++ b/src/keboola_agent_cli/services/_sync_push_ops.py @@ -23,7 +23,13 @@ from ..sync.code_extraction import merge_code_files, normalize_blocks_codes_script from ..sync.config_format import local_config_to_api, local_row_to_api from ..sync.manifest import Manifest, ManifestConfiguration -from ._encryption import encrypt_secrets_in_config +from ..sync.naming import sanitize_name +from ._encryption import ( + SecretCache, + collect_secrets, + encrypt_secrets_in_config, + is_already_encrypted, +) from ._sync_baseline import apply_stamp, row_baseline from ._sync_data_app import create_synced_data_app from ._sync_workspace import SANDBOXES_COMPONENT_ID, warn_on_backend_size_change @@ -87,6 +93,60 @@ def report(self) -> dict[str, Any]: report["skipped_deletions_reason"] += SKIPPED_WORKSPACE_DELETIONS_NOTE return report + def encrypt_secrets( + self, + service: SyncService, + client: Any, + source_dir: Path, + manifest: Manifest, + allow_plaintext_fallback: bool, + ) -> SecretCache: + """Encrypt every ``#`` secret push sends, before the first write. + + Reads the local file of each ``added`` / ``modified`` config and row + in *source_dir* and encrypts its plaintext secrets, one Encryption API + call per component. The writes then take the ciphertext from the + returned cache. So an ``ENCRYPTION_FAILED`` stops the push before + anything reaches the remote: it used to stop in the middle, and the + retry created again the configs created before the failure, which the + manifest did not record (issue #792 F). With + *allow_plaintext_fallback* an encryption failure never stops the + push, so nothing is encrypted in advance. + """ + project_id = manifest.project.id if manifest.project else None + if not project_id or allow_plaintext_fallback: + return {} + parent_paths = {(c.component_id, c.id): c.path for c in manifest.configurations} + plaintexts: dict[str, set[str]] = {} + for change in self.changes: + if change["change_type"] not in ("added", "modified"): + continue + path = change.get("path", "") + if change.get("is_row"): + parent_path = parent_paths.get((change["component_id"], change["parent_config_id"])) + if parent_path is None: + continue # push reports PARENT_CONFIG_NOT_TRACKED for it + path = f"{parent_path}/{path}" + found: dict[str, str] = {} + collect_secrets(service._read_config_file(source_dir / path) or {}, "", found) + plaintexts.setdefault(change["component_id"], set()).update(found.values()) + cache: SecretCache = {} + for component_id, values in plaintexts.items(): + ordered = sorted(values) + payload = {f"#secret{index}": value for index, value in enumerate(ordered)} + try: + encrypt_secrets_in_config(client, project_id, component_id, payload) + except KeboolaApiError as exc: + raise KeboolaApiError( + message=f"{exc.message} Nothing was pushed.", error_code=exc.error_code + ) from exc + for index, value in enumerate(ordered): + # A value the API did not return encrypted is not cached: the + # write then asks for it again rather than send plaintext. + if is_already_encrypted(payload[f"#secret{index}"]): + cache[(component_id, value)] = payload[f"#secret{index}"] + return cache + def plan_push(all_changes: list[dict[str, Any]], *, force: bool) -> PushPlan: """Split a diff changeset into what push applies and what it holds back. @@ -106,6 +166,47 @@ def plan_push(all_changes: list[dict[str, Any]], *, force: bool) -> PushPlan: return plan +def detect_name_drift( + service: SyncService, manifest: Manifest, project_root: Path +) -> list[dict[str, str]]: + """Detect configs where local dir name doesn't match the config name. + + Reads each tracked config's _config.yml to get the current name, + then compares sanitize_name(name) against the directory basename. + + Returns a list of warning dicts with component_id, config_id, + local_dirname, and expected_dirname. + """ + warnings: list[dict[str, str]] = [] + for cfg in manifest.configurations: + path = cfg.path + dirname = path.rsplit("/", 1)[-1] if "/" in path else path + + # Find branch dir and read _config.yml + branch_path = service._find_branch_path(manifest, cfg.branch_id) + config_dir = project_root / branch_path / path + local_data = service._read_config_file(config_dir) + if local_data is None: + continue + + config_name = local_data.get("name", "") + if not config_name: + continue + + expected_dirname = sanitize_name(config_name) + if dirname != expected_dirname: + warnings.append( + { + "component_id": cfg.component_id, + "config_id": cfg.id, + "local_dirname": dirname, + "expected_dirname": expected_dirname, + "config_name": config_name, + } + ) + return warnings + + def guard_script_shape( component_id: str, configuration: dict[str, Any], @@ -214,6 +315,7 @@ def push_row_change( branch_id: int | None, allow_plaintext_fallback: bool = False, warnings: list[dict[str, Any]] | None = None, + known_secrets: SecretCache | None = None, ) -> str | None: """Dispatch a single row-level change (added/modified/deleted) to the API. @@ -284,6 +386,7 @@ def push_row_change( project_id=project_id, allow_plaintext_fallback=allow_plaintext_fallback, warnings=warnings, + known_secrets=known_secrets, ) if change_type == "modified": @@ -299,6 +402,7 @@ def push_row_change( project_id=project_id, allow_plaintext_fallback=allow_plaintext_fallback, warnings=warnings, + known_secrets=known_secrets, ) return None @@ -318,6 +422,7 @@ def _push_create_row( project_id: int | None, allow_plaintext_fallback: bool, warnings: list[dict[str, Any]] | None = None, + known_secrets: SecretCache | None = None, ) -> str: """POST a new row; record API-assigned id + hashes in the parent's row list. @@ -335,6 +440,7 @@ def _push_create_row( component_id, configuration, allow_plaintext_fallback=allow_plaintext_fallback, + known=known_secrets, ) result = client.create_config_row( @@ -390,6 +496,7 @@ def push_update_row( project_id: int | None, allow_plaintext_fallback: bool, warnings: list[dict[str, Any]] | None = None, + known_secrets: SecretCache | None = None, ) -> None: """PUT an existing row; refresh its hashes in the parent's row list. @@ -409,6 +516,7 @@ def push_update_row( component_id, configuration, allow_plaintext_fallback=allow_plaintext_fallback, + known=known_secrets, ) result = client.update_config_row( @@ -479,6 +587,7 @@ def push_create( *, allow_plaintext_fallback: bool = False, warnings: list[dict[str, Any]] | None = None, + known_secrets: SecretCache | None = None, ds_client: Any = None, ds_branch_id: int | None = None, ) -> dict[str, Any] | None: @@ -523,6 +632,7 @@ def push_create( component_id, configuration, allow_plaintext_fallback=allow_plaintext_fallback, + known=known_secrets, ) if component_id == DATA_APP_COMPONENT_ID and ds_client is not None: @@ -591,6 +701,7 @@ def push_update( *, allow_plaintext_fallback: bool = False, warnings: list[dict[str, Any]] | None = None, + known_secrets: SecretCache | None = None, ) -> dict[str, Any]: """Update an existing config from a local _config.yml file. @@ -628,6 +739,7 @@ def push_update( component_id, configuration, allow_plaintext_fallback=allow_plaintext_fallback, + known=known_secrets, ) if component_id == SANDBOXES_COMPONENT_ID and warnings is not None: diff --git a/src/keboola_agent_cli/services/_sync_writeback.py b/src/keboola_agent_cli/services/_sync_writeback.py index d3e737a34..ff521e79d 100644 --- a/src/keboola_agent_cli/services/_sync_writeback.py +++ b/src/keboola_agent_cli/services/_sync_writeback.py @@ -13,6 +13,7 @@ from typing import TYPE_CHECKING, Any from ..errors import KeboolaApiError +from ..sync.config_format import BASE_CONFIG_HASH_KEY from ..sync.manifest import ManifestConfigRow, ManifestConfiguration from ._encryption import apply_encrypted_to_local from ._sync_baseline import apply_stamp, config_baseline @@ -243,9 +244,12 @@ def writeback_after_push( """Update local ``_config.yml`` after a successful push. Writes back the API-assigned ``_keboola.config_id`` (on first create) and - the encrypted secret values (so local matches remote state). + the encrypted secret values (so local matches remote state). A scaffold's + ``_keboola.base_config_hash`` is dropped: after the push the manifest + holds the baseline (issue #792 E). """ keboola_meta = local_data.setdefault("_keboola", {}) + keboola_meta.pop(BASE_CONFIG_HASH_KEY, None) if config_id: keboola_meta["config_id"] = config_id diff --git a/src/keboola_agent_cli/services/component_service.py b/src/keboola_agent_cli/services/component_service.py index ca424975f..1f8bd08a0 100644 --- a/src/keboola_agent_cli/services/component_service.py +++ b/src/keboola_agent_cli/services/component_service.py @@ -19,7 +19,8 @@ from ..errors import ConfigError, ErrorCode, KeboolaApiError from ..models import ComponentDetail, ComponentSuggestion, ProjectConfig from ..sync.code_extraction import DESCRIPTION_FILENAME, extract_code_files -from ..sync.config_format import api_config_to_local, dump_config_yaml +from ..sync.config_format import BASE_CONFIG_HASH_KEY, api_config_to_local, dump_config_yaml +from ..sync.diff_engine import config_hash from .base import BaseService, ClientFactory, make_session_aware_client_factory from .org_service import slugify @@ -308,7 +309,21 @@ def _build_pyproject_toml(component_id: str, name: str, packages: list[str] | No ) -def stamp_scaffold_config_id(scaffold: dict[str, Any], config_id: str) -> dict[str, Any]: +def pushed_config_base_hash(component_id: str, config_id: str, pushed: dict[str, Any]) -> str: + """Return the sync baseline of a config ``config new --push`` has just created. + + It is the ``config_hash`` of the API response in the local format, the + value ``sync pull`` stores as ``pull_config_hash``. The scaffold records it + as ``_keboola.base_config_hash``, because the file has no manifest entry: + without it, ``sync diff`` cannot tell an edit of the file from an edit made + in the UI (issue #792 E). + """ + return config_hash(api_config_to_local(component_id, pushed, str(config_id))) + + +def stamp_scaffold_config_id( + scaffold: dict[str, Any], config_id: str, base_config_hash: str = "" +) -> dict[str, Any]: """Return a copy of *scaffold* whose ``_config.yml`` records *config_id*. On the ``config new --push --output-dir`` path the configuration already @@ -321,9 +336,13 @@ def stamp_scaffold_config_id(scaffold: dict[str, Any], config_id: str) -> dict[s The ID is emitted double-quoted so legacy numeric IDs stay YAML strings (an unquoted ``12345`` would parse as ``int`` and never match the string-keyed remote lookup). A ``_config.yml`` without a ``_keboola`` - block (flow scaffolds, issue #650) gets one appended wholesale. Pure - function: the input scaffold is not mutated. + block (flow scaffolds, issue #650) gets one appended wholesale. A given + *base_config_hash* (:func:`pushed_config_base_hash`) is recorded next to + the ID. Pure function: the input scaffold is not mutated. """ + stamped_lines = [f' config_id: "{config_id}"'] + if base_config_hash: + stamped_lines.append(f' {BASE_CONFIG_HASH_KEY}: "{base_config_hash}"') files: list[dict[str, Any]] = [] for entry in scaffold["files"]: if entry["path"] != CONFIG_FILENAME: @@ -343,7 +362,7 @@ def stamp_scaffold_config_id(scaffold: dict[str, Any], config_id: str) -> dict[s if line == "_keboola:": in_keboola = True elif in_keboola and line.startswith(" component_id:"): - out.append(f' config_id: "{config_id}"') + out.extend(stamped_lines) stamped = True in_keboola = False if not stamped: @@ -353,7 +372,7 @@ def stamp_scaffold_config_id(scaffold: dict[str, Any], config_id: str) -> dict[s [ "_keboola:", f" component_id: {scaffold['component_id']}", - f' config_id: "{config_id}"', + *stamped_lines, ] ) new_content = "\n".join(out) @@ -399,6 +418,8 @@ def materialize_pushed_config( {"name": name, "description": description, "configuration": configuration}, str(config_id), ) + # The file mirrors the remote, so the remote is its baseline (#792 E). + local["_keboola"][BASE_CONFIG_HASH_KEY] = config_hash(local) config_dir.mkdir(parents=True, exist_ok=True) # The slugified directory can pre-exist (same-name re-run, stray files): # report only what THIS call wrote, and clear a stale _description.md a diff --git a/src/keboola_agent_cli/services/sync_service.py b/src/keboola_agent_cli/services/sync_service.py index dc15de5f8..1f1733a8a 100644 --- a/src/keboola_agent_cli/services/sync_service.py +++ b/src/keboola_agent_cli/services/sync_service.py @@ -66,13 +66,14 @@ load_manifest, save_manifest, ) -from ..sync.naming import config_path, config_row_path, sanitize_name +from ..sync.naming import config_path, config_row_path from . import _sync_workspace as sql_workspaces from ._encryption import ( encrypt_secrets_in_config, find_plaintext_secret_keys, ) from ._sync_baseline import ( + adopted_skip_detail, config_locally_modified, detect_force_pull_conflicts, effective_stored_hash, @@ -106,7 +107,7 @@ type_needs_rewrite, ) from ._sync_models import CreatedConfig, LocalConfigHashes -from ._sync_push_ops import plan_push, push_create, push_row_change, push_update +from ._sync_push_ops import detect_name_drift, plan_push, push_create, push_row_change, push_update from ._sync_stale import ( apply_stale_sweep, find_stale_entries, @@ -767,6 +768,14 @@ def pull( api_cfg_hash = config_hash(local_data) pull_cfg_hash = api_cfg_hash + # An untracked file that carries this config's id keeps its + # local changes; only --theirs overwrites it (issue #792 E). + if is_new and not theirs: + skipped = adopted_skip_detail(self, config_dir, component_id, cfg, rel_path) + if skipped: + pull_details.append(skipped) + continue + # Detect local modifications: if the file hash differs from the # pull_hash stored in manifest, the user edited the file -- so # preserve it instead of overwriting. This now runs even under @@ -1412,6 +1421,8 @@ def diff( continue if verdict == VERDICT_CREATE: untracked_id = "" # new config, push creates it + # An adopted file has no manifest baseline: compute_changeset reads + # the one a `config new --push` scaffold records (issue #792 E). local_configs.append( { "component_id": component_id, @@ -1419,6 +1430,7 @@ def diff( "config_name": local_data.get("name", ""), "path": added_cfg["path"], "data": local_data, + "adopted": bool(untracked_id), } ) @@ -1658,7 +1670,7 @@ def push( ) # Detect name drift: local dir name doesn't match config name - name_drift_warnings = self._detect_name_drift(manifest, project_root) + name_drift_warnings = detect_name_drift(self, manifest, project_root) client = self._client_factory(project.stack_url, project.token) created = 0 @@ -1688,6 +1700,10 @@ def push( # first manifest branch (#808), and only when a data app is created. ds_branch_id = resolve_ds_branch_id(client, ds_client, branch_id, warnings) branch_path = self._resolve_source_branch_path(manifest, project_root, branch_id) + # An encryption failure stops push before its first write (#792 F). + known_secrets = plan.encrypt_secrets( + self, client, project_root / branch_path, manifest, allow_plaintext_fallback + ) # Process configs before rows, and rebind variable links last. # A freshly-created parent config must carry its API-assigned ULID @@ -1724,6 +1740,7 @@ def push( warnings=warnings, ds_client=ds_client, ds_branch_id=ds_branch_id, + known_secrets=known_secrets, ) if result: new_id = str(result.get("id", "")) @@ -1793,6 +1810,7 @@ def push( branch_id, allow_plaintext_fallback=allow_plaintext_fallback, warnings=warnings, + known_secrets=known_secrets, ) # Update hashes so pull knows local == remote if (config_dir / CONFIG_FILENAME).exists(): @@ -1870,6 +1888,7 @@ def push( branch_id=branch_id, allow_plaintext_fallback=allow_plaintext_fallback, warnings=warnings, + known_secrets=known_secrets, ) manifest_dirty = True if change_type == "added": @@ -2236,44 +2255,6 @@ def _file_hash(self, file_path: Path) -> str: content = file_path.read_bytes() return hashlib.sha256(content).hexdigest() - def _detect_name_drift(self, manifest: Manifest, project_root: Path) -> list[dict[str, str]]: - """Detect configs where local dir name doesn't match the config name. - - Reads each tracked config's _config.yml to get the current name, - then compares sanitize_name(name) against the directory basename. - - Returns a list of warning dicts with component_id, config_id, - local_dirname, and expected_dirname. - """ - warnings: list[dict[str, str]] = [] - for cfg in manifest.configurations: - path = cfg.path - dirname = path.rsplit("/", 1)[-1] if "/" in path else path - - # Find branch dir and read _config.yml - branch_path = self._find_branch_path(manifest, cfg.branch_id) - config_dir = project_root / branch_path / path - local_data = self._read_config_file(config_dir) - if local_data is None: - continue - - config_name = local_data.get("name", "") - if not config_name: - continue - - expected_dirname = sanitize_name(config_name) - if dirname != expected_dirname: - warnings.append( - { - "component_id": cfg.component_id, - "config_id": cfg.id, - "local_dirname": dirname, - "expected_dirname": expected_dirname, - "config_name": config_name, - } - ) - return warnings - def _rename_directory(self, source: Path, target: Path) -> str: """Rename a directory, using git mv if in a git repo, else shutil.move. diff --git a/src/keboola_agent_cli/sync/config_format.py b/src/keboola_agent_cli/sync/config_format.py index 6f88555ef..2ca86f7bf 100644 --- a/src/keboola_agent_cli/sync/config_format.py +++ b/src/keboola_agent_cli/sync/config_format.py @@ -15,6 +15,13 @@ from ..constants import CONFIG_YML_VERSION from .code_extraction import canonical_sql_script, is_sql_transformation_component +# ``_keboola`` footer key of a file that ``config new --push --output-dir`` +# writes for a config it has just created: the ``config_hash`` of that config +# as the API returned it. The file has no manifest entry, so this is the only +# baseline ``sync diff`` and ``sync pull`` have for it (issue #792 E). The +# footer is stripped before hashing, so the key never shows as a change. +BASE_CONFIG_HASH_KEY = "base_config_hash" + def _iter_codes(parameters: dict[str, Any]) -> Any: """Yield every ``blocks[].codes[]`` dict in a transformation's parameters.""" diff --git a/src/keboola_agent_cli/sync/diff_engine.py b/src/keboola_agent_cli/sync/diff_engine.py index 8f462e451..86498efab 100644 --- a/src/keboola_agent_cli/sync/diff_engine.py +++ b/src/keboola_agent_cli/sync/diff_engine.py @@ -9,9 +9,11 @@ import copy import hashlib import json +from collections import Counter from typing import Any from ..constants import DIFF_MAX_DEPTH, DIFF_MAX_LINES, ENCRYPTED_PLACEHOLDER +from .config_format import BASE_CONFIG_HASH_KEY from .secrets import is_encrypted_value # Keys that are internal bookkeeping and should be excluded from comparison. @@ -320,6 +322,13 @@ def compute_changeset( Args: local_configs: List of dicts with keys: ``component_id``, ``config_id``, ``config_name``, ``path``, ``data``. + An untracked file adopted by its ``_keboola.config_id`` also has + ``adopted: True``. It has no manifest base: its base is the + ``_keboola.base_config_hash`` a ``config new --push`` scaffold + records. Without one a local edit and a remote edit look the + same, so a difference is a ``"conflict"``, never ``"modified"``; + the same applies to two files that adopt one id (issue #792 E: + push overwrote an edit made in the UI). remote_configs: Dict keyed by ``"{component_id}/{config_id}"`` with API config data (already converted to local format). tracked_keys: Optional set of ``"{component_id}/{config_id}"`` keys @@ -349,6 +358,9 @@ def compute_changeset( changes: list[ConfigChange] = [] seen_remote_keys: set[str] = set() on_target = target_tracked_keys or set() + adoptions = Counter( + f"{e['component_id']}/{e.get('config_id', '')}" for e in local_configs if e.get("adopted") + ) for entry in local_configs: component_id: str = entry["component_id"] @@ -390,7 +402,12 @@ def compute_changeset( # Determine change direction using base hash (pull-time snapshot) base_h = (base_hashes or {}).get(remote_key) - if base_h is not None: + if entry.get("adopted"): + base_h = (local_data.get("_keboola") or {}).get(BASE_CONFIG_HASH_KEY) + if entry.get("adopted") and (not base_h or adoptions[remote_key] > 1): + # No base, or two files claim the id: the direction is unknown. + local_changed = remote_changed = True + elif base_h is not None: local_changed = local_h != base_h remote_changed = remote_h != base_h else: diff --git a/tests/test_component_service.py b/tests/test_component_service.py index 60ea30600..89b25d003 100644 --- a/tests/test_component_service.py +++ b/tests/test_component_service.py @@ -1210,6 +1210,30 @@ def test_stamps_real_generator_output_with_single_keboola_block( assert parsed["_keboola"]["config_id"] == "01m0njbrqwpyqbx0yqfqq9pyen" assert "assigned by Keboola on first push" not in content + def test_records_base_config_hash_next_to_the_id(self) -> None: + """A given baseline lands next to the id, in both stamping paths, + and nothing is added without one (issue #792 E).""" + from keboola_agent_cli.services.component_service import stamp_scaffold_config_id + + stamped = stamp_scaffold_config_id(self._scaffold(), "12345", "abc123") + assert yaml.safe_load(stamped["files"][0]["content"])["_keboola"] == { + "component_id": "keboola.ex-http", + "config_id": "12345", + "base_config_hash": "abc123", + } + + flow = self._scaffold() + flow["files"][0]["content"] = 'name: "my flow"\nphases: []\n' + appended = stamp_scaffold_config_id(flow, "999", "abc123") + assert yaml.safe_load(appended["files"][0]["content"])["_keboola"] == { + "component_id": "keboola.ex-http", + "config_id": "999", + "base_config_hash": "abc123", + } + + plain = stamp_scaffold_config_id(self._scaffold(), "12345") + assert "base_config_hash" not in plain["files"][0]["content"] + def test_companion_files_untouched_and_input_not_mutated(self) -> None: from keboola_agent_cli.services.component_service import stamp_scaffold_config_id @@ -1254,9 +1278,25 @@ def test_mirrors_pushed_body(self, tmp_path: Path) -> None: # Encrypted value travels verbatim -- never decrypted, never a TODO. assert parsed["parameters"]["#token"] == "KBC::ProjectSecure::abc" assert parsed["input"] == {"tables": [{"source": "in.c-b.t"}]} + # The remote is the file's sync baseline, the value `sync pull` would + # store as pull_config_hash (issue #792 E). + from keboola_agent_cli.services.component_service import pushed_config_base_hash + + pushed = { + "name": "test-config", + "description": "desc", + "configuration": { + "parameters": { + "baseUrl": "https://example.com", + "#token": "KBC::ProjectSecure::abc", + }, + "storage": {"input": {"tables": [{"source": "in.c-b.t"}]}}, + }, + } assert parsed["_keboola"] == { "component_id": "keboola.ex-http", "config_id": "12345", + "base_config_hash": pushed_config_base_hash("keboola.ex-http", "12345", pushed), } def test_transformation_body_extracts_real_code(self, tmp_path: Path) -> None: diff --git a/tests/test_config_create_cli.py b/tests/test_config_create_cli.py index 6ccea167b..065b3bf76 100644 --- a/tests/test_config_create_cli.py +++ b/tests/test_config_create_cli.py @@ -786,6 +786,30 @@ def test_written_scaffold_carries_created_config_id(self, tmp_path: Path) -> Non assert isinstance(parsed["_keboola"]["config_id"], str) assert "assigned by Keboola on first push" not in written.read_text(encoding="utf-8") + def test_written_scaffold_records_the_created_config_as_its_base(self, tmp_path: Path) -> None: + """The scaffold has no manifest entry, so it records its sync baseline: + the created config as the API returned it (issue #792 E).""" + from keboola_agent_cli.services.component_service import pushed_config_base_hash + + out = tmp_path / "ws" + out.mkdir() + svc_config = MagicMock() + svc_config.create_config.return_value = _push_result() + + result = self._run( + tmp_path, + out, + config_service_mock=svc_config, + component_service_mock=self._component_mock(), + ) + + assert result.exit_code == 0, result.output + written = out / "extractor/keboola.ex-http/test-config/_config.yml" + parsed = yaml.safe_load(written.read_text(encoding="utf-8")) + assert parsed["_keboola"]["base_config_hash"] == pushed_config_base_hash( + "keboola.ex-http", "12345", _push_result() + ) + def test_envelope_reports_local_scaffold(self, tmp_path: Path) -> None: out = tmp_path / "ws" out.mkdir() diff --git a/tests/test_sync_adopted_files.py b/tests/test_sync_adopted_files.py new file mode 100644 index 000000000..6eea396df --- /dev/null +++ b/tests/test_sync_adopted_files.py @@ -0,0 +1,275 @@ +"""Untracked files that carry a config id: diff, push and pull (issue #792 E). + +An untracked ``_config.yml`` whose ``_keboola.config_id`` exists on the target +branch is *adopted*: diff compares it with that remote config instead of +creating a duplicate (#482). Such a file has no manifest entry, so it has no +baseline. The 2-way fallback read every difference as a local edit, and push +overwrote an edit made in the UI. Now: + +- without a baseline, a difference is a ``conflict`` (push does not apply it); +- a ``config new --push --output-dir`` scaffold records its own baseline + (``_keboola.base_config_hash``), so its diff is 3-way; +- two files that adopt one id are both a ``conflict``; +- pull keeps an adopted file with local changes (``skipped``); only + ``--theirs`` overwrites it. + +The replay of the original finding is +``test_sync_formal_counterexamples.py::test_e_adopted_scaffold_push_does_not_overwrite_remote_edit``. +""" + +from __future__ import annotations + +import shutil +from pathlib import Path + +import pytest +import yaml + +from keboola_agent_cli.constants import CONFIG_FILENAME +from keboola_agent_cli.services._sync_baseline import ADOPTED_SKIP_REASON +from keboola_agent_cli.services.component_service import ( + materialize_pushed_config, + pushed_config_base_hash, + stamp_scaffold_config_id, +) +from keboola_agent_cli.sync.config_format import BASE_CONFIG_HASH_KEY +from keboola_agent_cli.sync.manifest import load_manifest, save_manifest +from test_sync_formal_counterexamples import COMP, PROD, World, changes + +ORDERS_DIR = "main/extractor/keboola.ex-http/orders" + + +def _pulled_then_untracked(tmp_path: Path) -> World: + """Pull one config, then drop its manifest entry: the file only carries the id.""" + w = World(tmp_path) + w.api.put(PROD, "cfg-1", "Orders", "a") + w.init() + w.pull() + manifest = load_manifest(w.root) + manifest.configurations = [] + save_manifest(w.root, manifest) + return w + + +def _scaffolded(tmp_path: Path) -> World: + """A config created remotely and written the way `config new --push --output-dir` does it.""" + w = World(tmp_path) + w.init() + w.pull() + w.api.put(PROD, "cfg-1", "Orders", "a") + remote = w.api.remote[PROD]["cfg-1"] + materialize_pushed_config( + component_id=COMP, + config_id="cfg-1", + name=remote["name"], + description=remote["description"], + configuration=remote["configuration"], + config_dir=w.root / ORDERS_DIR, + ) + assert w.manifest() == [] + return w + + +def _set_value(config_dir: Path, value: str) -> None: + config_file = config_dir / CONFIG_FILENAME + data = yaml.safe_load(config_file.read_text(encoding="utf-8")) + data["parameters"]["value"] = value + config_file.write_text(yaml.safe_dump(data), encoding="utf-8") + + +def _value(config_dir: Path) -> str: + return yaml.safe_load((config_dir / CONFIG_FILENAME).read_text(encoding="utf-8"))["parameters"][ + "value" + ] + + +def _updates(w: World) -> list[str]: + return [line for line in w.api.log if line.startswith("UPDATE")] + + +# --------------------------------------------------------------------------- +# diff / push without a baseline +# --------------------------------------------------------------------------- + + +def test_adopted_file_without_base_is_a_conflict_when_it_differs(tmp_path: Path) -> None: + """A local edit and a remote edit look the same without a baseline.""" + w = _pulled_then_untracked(tmp_path) + _set_value(w.root / ORDERS_DIR, "LOCAL") + + assert changes(w.diff()) == [("conflict", "cfg-1")] + result = w.push() + + assert (result["status"], result["skipped"]) == ("no_changes", 1) + assert _updates(w) == [] + assert w.api.remote[PROD]["cfg-1"]["configuration"]["parameters"]["value"] == "a" + + +def test_adopted_file_equal_to_the_remote_is_unchanged(tmp_path: Path) -> None: + w = _pulled_then_untracked(tmp_path) + + diff = w.diff() + + assert diff["changes"] == [] + assert diff["summary"]["unchanged"] == 1 + assert w.push()["status"] == "no_changes" + + +# --------------------------------------------------------------------------- +# diff / push of a `config new --push --output-dir` scaffold (3-way) +# --------------------------------------------------------------------------- + + +def test_scaffold_records_the_remote_as_its_base(tmp_path: Path) -> None: + w = _scaffolded(tmp_path) + meta = yaml.safe_load((w.root / ORDERS_DIR / CONFIG_FILENAME).read_text())["_keboola"] + + assert meta[BASE_CONFIG_HASH_KEY] == pushed_config_base_hash( + COMP, "cfg-1", w.api.remote[PROD]["cfg-1"] + ) + assert w.diff()["changes"] == [] + + +def test_scaffold_edit_is_pushed_and_the_config_becomes_tracked(tmp_path: Path) -> None: + w = _scaffolded(tmp_path) + _set_value(w.root / ORDERS_DIR, "LOCAL") + + assert changes(w.diff()) == [("modified", "cfg-1")] + result = w.push() + + assert result["updated"] == 1 + assert w.api.remote[PROD]["cfg-1"]["configuration"]["parameters"]["value"] == "LOCAL" + assert [cid for _, cid, _ in w.manifest()] == ["cfg-1"] + # The manifest holds the baseline now; the file drops its own copy. + assert BASE_CONFIG_HASH_KEY not in (w.root / ORDERS_DIR / CONFIG_FILENAME).read_text() + assert w.diff()["changes"] == [] + + +def test_scaffold_with_a_remote_edit_is_remote_modified_and_not_pushed(tmp_path: Path) -> None: + w = _scaffolded(tmp_path) + w.api.put(PROD, "cfg-1", "Orders", "EDITED-IN-UI") + + assert changes(w.diff()) == [("remote_modified", "cfg-1")] + w.push() + + assert _updates(w) == [] + assert w.api.remote[PROD]["cfg-1"]["configuration"]["parameters"]["value"] == "EDITED-IN-UI" + + +def test_scaffold_edited_on_both_sides_is_a_conflict(tmp_path: Path) -> None: + w = _scaffolded(tmp_path) + _set_value(w.root / ORDERS_DIR, "LOCAL") + w.api.put(PROD, "cfg-1", "Orders", "EDITED-IN-UI") + + assert changes(w.diff()) == [("conflict", "cfg-1")] + w.push() + + assert _updates(w) == [] + + +def test_placeholder_scaffold_is_modified_and_push_applies_it(tmp_path: Path) -> None: + """`config new --push --output-dir` without --configuration pushes an empty + body and writes templates: the documented "edit, then sync push" flow.""" + w = World(tmp_path) + w.init() + w.pull() + w.api.remote[PROD]["cfg-1"] = { + "id": "cfg-1", + "name": "Orders", + "description": "", + "configuration": {}, + "rows": [], + "isDisabled": False, + } + scaffold = { + "component_id": COMP, + "files": [ + { + "path": CONFIG_FILENAME, + "content": ( + 'version: 2\nname: "Orders"\ndescription: ""\n' + "parameters:\n value: TODO\n\n_keboola:\n component_id: keboola.ex-http\n" + ), + } + ], + } + stamped = stamp_scaffold_config_id( + scaffold, "cfg-1", pushed_config_base_hash(COMP, "cfg-1", w.api.remote[PROD]["cfg-1"]) + ) + config_dir = w.root / ORDERS_DIR + config_dir.mkdir(parents=True) + (config_dir / CONFIG_FILENAME).write_text(stamped["files"][0]["content"]) + + assert changes(w.diff()) == [("modified", "cfg-1")] + assert w.push()["updated"] == 1 + assert w.api.remote[PROD]["cfg-1"]["configuration"]["parameters"]["value"] == "TODO" + + +def test_two_files_adopting_one_id_are_both_conflicts(tmp_path: Path) -> None: + """Both copies carry the same base; neither may overwrite the remote.""" + w = _scaffolded(tmp_path) + copy_dir = w.root / "main/extractor/keboola.ex-http/orders-copy" + shutil.copytree(w.root / ORDERS_DIR, copy_dir) + _set_value(w.root / ORDERS_DIR, "FIRST") + _set_value(copy_dir, "SECOND") + + assert changes(w.diff()) == [("conflict", "cfg-1"), ("conflict", "cfg-1")] + w.push() + + assert _updates(w) == [] + assert w.api.remote[PROD]["cfg-1"]["configuration"]["parameters"]["value"] == "a" + + +# --------------------------------------------------------------------------- +# pull +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("force", [False, True]) +def test_pull_keeps_an_adopted_file_with_local_changes(tmp_path: Path, force: bool) -> None: + w = _pulled_then_untracked(tmp_path) + _set_value(w.root / ORDERS_DIR, "LOCAL") + w.api.put(PROD, "cfg-1", "Orders", "EDITED-IN-UI") + + result = w.pull(force=force) + + assert [(d["action"], d.get("reason")) for d in result["details"]] == [ + ("skipped", ADOPTED_SKIP_REASON) + ] + assert _value(w.root / ORDERS_DIR) == "LOCAL" + assert w.manifest() == [] + + +def test_pull_theirs_overwrites_an_adopted_file(tmp_path: Path) -> None: + w = _pulled_then_untracked(tmp_path) + _set_value(w.root / ORDERS_DIR, "LOCAL") + w.api.put(PROD, "cfg-1", "Orders", "EDITED-IN-UI") + + w.pull(theirs=True) + + assert _value(w.root / ORDERS_DIR) == "EDITED-IN-UI" + assert [cid for _, cid, _ in w.manifest()] == ["cfg-1"] + assert w.diff()["changes"] == [] + + +def test_pull_takes_the_remote_edit_of_an_unedited_scaffold(tmp_path: Path) -> None: + w = _scaffolded(tmp_path) + w.api.put(PROD, "cfg-1", "Orders", "EDITED-IN-UI") + + w.pull() + + assert _value(w.root / ORDERS_DIR) == "EDITED-IN-UI" + assert [cid for _, cid, _ in w.manifest()] == ["cfg-1"] + assert w.diff()["changes"] == [] + + +def test_pull_keeps_an_edited_scaffold_and_push_applies_the_edit(tmp_path: Path) -> None: + w = _scaffolded(tmp_path) + _set_value(w.root / ORDERS_DIR, "LOCAL") + + result = w.pull() + + assert [d["action"] for d in result["details"]] == ["skipped"] + assert _value(w.root / ORDERS_DIR) == "LOCAL" + assert w.push()["updated"] == 1 + assert w.api.remote[PROD]["cfg-1"]["configuration"]["parameters"]["value"] == "LOCAL" diff --git a/tests/test_sync_diff_engine.py b/tests/test_sync_diff_engine.py index 5291e9679..ab21f24a2 100644 --- a/tests/test_sync_diff_engine.py +++ b/tests/test_sync_diff_engine.py @@ -507,6 +507,57 @@ def row(parent: str, row_id: str, name: str) -> dict[str, Any]: ] +def _adopted(value: str, base: str | None = None, path: str = "orders") -> dict[str, Any]: + """An untracked local file adopted by its config id (issue #792 E).""" + keboola: dict[str, Any] = {"component_id": "keboola.ex-http", "config_id": "cfg-001"} + if base is not None: + keboola["base_config_hash"] = base + return { + "component_id": "keboola.ex-http", + "config_id": "cfg-001", + "config_name": "Orders", + "path": path, + "data": {"name": "Orders", "parameters": {"value": value}, "_keboola": keboola}, + "adopted": True, + } + + +def _remote(value: str) -> dict[str, dict[str, Any]]: + return {"keboola.ex-http/cfg-001": {"name": "Orders", "parameters": {"value": value}}} + + +def _base(value: str) -> str: + return config_hash({"name": "Orders", "parameters": {"value": value}}) + + +class TestAdoptedFiles: + """An adopted file is diffed 3-way against the base it records, else it + is a conflict on any difference (issue #792 E).""" + + def _types(self, locals_: list[dict[str, Any]], remote_value: str) -> list[str]: + changes = compute_changeset(locals_, _remote(remote_value), tracked_keys=set()) + return [c.change_type for c in changes] + + def test_without_base_a_difference_is_a_conflict(self) -> None: + assert self._types([_adopted("local")], "remote") == ["conflict"] + + def test_without_base_an_equal_file_is_unchanged(self) -> None: + assert self._types([_adopted("same")], "same") == [] + + def test_with_base_the_diff_is_three_way(self) -> None: + assert self._types([_adopted("local", _base("v1"))], "v1") == ["modified"] + assert self._types([_adopted("v1", _base("v1"))], "remote") == ["remote_modified"] + assert self._types([_adopted("local", _base("v1"))], "remote") == ["conflict"] + + def test_two_files_adopting_one_id_are_conflicts(self) -> None: + both = [_adopted("one", _base("v1")), _adopted("two", _base("v1"), path="orders-copy")] + assert self._types(both, "v1") == ["conflict", "conflict"] + + def test_a_tracked_entry_without_base_keeps_the_two_way_fallback(self) -> None: + tracked = {**_adopted("local"), "adopted": False} + assert self._types([tracked], "remote") == ["modified"] + + class TestConfigChange: """Tests for ConfigChange.to_dict() serialization.""" diff --git a/tests/test_sync_formal_counterexamples.py b/tests/test_sync_formal_counterexamples.py index 816d1841a..83fbc5a2e 100644 --- a/tests/test_sync_formal_counterexamples.py +++ b/tests/test_sync_formal_counterexamples.py @@ -566,26 +566,14 @@ def test_d_promote_push_diff_is_clean_and_edits_update_the_dev_copy(tmp_path: Pa # =========================================================================== # E -- an untracked file carrying `_keboola.config_id` (a `config new --push -# --output-dir` scaffold, or an adopted orphan) is diffed 2-way, so -# push silently overwrites a remote edit made after the scaffold was -# written -- and two copies of it both "adopt" the same remote id. +# --output-dir` scaffold, or an adopted orphan) was diffed 2-way, so +# push silently overwrote a remote edit made after the scaffold was +# written -- and two copies of it both "adopted" the same remote id. +# FIXED: without a baseline a difference is a conflict, which push does +# not apply (more cases in tests/test_sync_adopted_files.py). # =========================================================================== -@pytest.mark.xfail( - strict=True, - reason=( - "#792 E: classify_untracked ADOPTs a file carrying " - "`_keboola.config_id` (branch_scope.py:275-276), but base_hashes is " - "built from in-tree manifest entries only (sync_service.py:" - "1441-1452). With no base, compute_changeset's 2-way fallback turns " - "ANY difference into 'modified' (diff_engine.py:388-391) -- so a " - "remote edit made after the scaffold was written (e.g. in the web " - "UI) is silently reverted by the next `sync push`, no conflict " - "shown. Found via TLA I11 and replayed " - "(scratchpad/replay/r_adopt_lost_update.py)." - ), -) def test_e_adopted_scaffold_push_does_not_overwrite_remote_edit(tmp_path: Path) -> None: """Invariant: a `sync push` must never silently revert a remote edit made to a config in between that config's on-disk scaffold being written and @@ -608,41 +596,32 @@ def test_e_adopted_scaffold_push_does_not_overwrite_remote_edit(tmp_path: Path) # was written, before the next push. w.api.put(PROD, "cfg-1", "Orders", "EDITED-IN-UI") - w.push() + result = w.push() assert w.api.remote[PROD]["cfg-1"]["configuration"]["parameters"]["value"] == "EDITED-IN-UI", ( "push silently reverted a remote edit made to an adopted-by-id scaffold config" ) + assert (result.get("updated", 0), result["skipped"]) == (0, 1) + assert changes(w.diff()) == [("conflict", "cfg-1")] # =========================================================================== -# F -- a push aborted by ENCRYPTION_FAILED leaves the manifest unsaved, so a -# change it already applied (a promote CREATE) is re-applied by the -# retry, duplicating it. +# F -- a push aborted by ENCRYPTION_FAILED left the manifest unsaved, so a +# change it already applied (a promote CREATE) was re-applied by the +# retry, duplicating it. FIXED: push encrypts every secret before its +# first write, so the abort happens before anything reaches the remote. # =========================================================================== -@pytest.mark.xfail( - strict=True, - reason=( - "#792 F: push() re-raises ENCRYPTION_FAILED instead of continuing " - "(sync_service.py:1842-1852), but save_manifest only runs after the " - "whole Phase A/B/C/D loop completes (:1946) -- so a CREATE applied " - "to the remote before the failing change is never recorded in the " - "manifest. Re-running push after the encryption problem is fixed " - "creates that same config again, duplicating it. This is a model " - "trace only in the original TLA pilot (I1(b), not previously " - "replayed against real SyncService); replayed here directly by " - "making the second config's secret fail Encryption API mock." - ), -) def test_f_aborted_push_does_not_duplicate_already_created_config(tmp_path: Path) -> None: """Invariant: retrying a push after an ENCRYPTION_FAILED abort must not re-apply a change (here: a promote CREATE of ``main/`` into a dev branch) that the aborted push had already sent to the remote before it failed. - The promote path keeps both creates in manifest order, so "Orders" is - created before "Contacts" fails. (The first version of this test used a - remote-deleted config, which push no longer re-creates since the H fix.) + The promote path keeps both creates in manifest order, so "Orders" was + created before "Contacts" failed. Push now encrypts both secrets before + the first write, so the abort creates nothing and the retry creates each + config once. (The first version of this test used a remote-deleted + config, which push no longer re-creates since the H fix.) Issue #792 finding F. """ @@ -684,13 +663,13 @@ def flaky_encrypt(project_id: int, component_id: str, data: dict[str, str]) -> d w.push(branch=DEV) creates_after_retry = [line for line in w.api.log if line.startswith("CREATE")] - # Safe expectation: the promote CREATE for "Orders" happened exactly - # once across both push attempts, not once per attempt. - assert len(creates_before_retry) == 1, "expected exactly one CREATE before the encryption abort" - assert len(creates_after_retry) == len(creates_before_retry), ( - "retrying the push after the encryption fix duplicated the already-applied promote CREATE: " - f"log={w.api.log}" + # Safe expectation: the aborted push wrote nothing, and each config was + # created exactly once across both push attempts, not once per attempt. + assert creates_before_retry == [], "the aborted push created a config before it failed" + assert len(creates_after_retry) == 2, ( + f"retrying the push after the encryption fix duplicated a promote CREATE: log={w.api.log}" ) + assert sorted(c["name"] for c in w.api.remote[DEV].values()) == ["Contacts", "Orders"] # =========================================================================== diff --git a/tests/test_sync_push_encrypt_first.py b/tests/test_sync_push_encrypt_first.py new file mode 100644 index 000000000..7467f7415 --- /dev/null +++ b/tests/test_sync_push_encrypt_first.py @@ -0,0 +1,261 @@ +"""`sync push` encrypts every secret before its first write (issue #792 F). + +Push used to encrypt each config's ``#`` secrets just before its own write. An +``ENCRYPTION_FAILED`` on a later config then stopped the push after earlier +configs were created, and the manifest, saved only at the end, did not record +them: the retry created them again. Push now encrypts all secrets first, so an +encryption failure stops it before anything reaches the remote. + +The replay of the original finding is +``test_sync_formal_counterexamples.py::test_f_aborted_push_does_not_duplicate_already_created_config``. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock + +import pytest +import yaml + +from keboola_agent_cli.constants import CONFIG_FILENAME +from keboola_agent_cli.errors import ErrorCode, KeboolaApiError +from keboola_agent_cli.services._encryption import encrypt_secrets_in_config +from keboola_agent_cli.services._sync_push_ops import PushPlan +from keboola_agent_cli.sync.manifest import ( + Manifest, + ManifestConfigRow, + ManifestConfiguration, + ManifestNaming, + ManifestProject, + load_manifest, +) +from test_sync_formal_counterexamples import COMP, PROD, World + + +def _new_config(w: World, name: str, secret: str) -> Path: + """A new local config (no id yet), which push creates.""" + config_dir = w.root / "main" / "extractor" / COMP / name.lower() + config_dir.mkdir(parents=True) + (config_dir / CONFIG_FILENAME).write_text( + yaml.safe_dump( + { + "version": 2, + "name": name, + "parameters": {"#token": secret}, + "_keboola": {"component_id": COMP}, + } + ), + encoding="utf-8", + ) + return config_dir + + +def _encrypt(fail_on: str | None = None) -> Any: + def encrypt_values(project_id: int, component_id: str, data: dict[str, str]) -> dict[str, str]: + if fail_on is not None and fail_on in data.values(): + raise RuntimeError("Encryption API unavailable") + return {key: f"KBC::ProjectSecure::{value}" for key, value in data.items()} + + return encrypt_values + + +def _world_with_two_new_configs(tmp_path: Path, fail_on: str | None) -> World: + w = World(tmp_path) + w.init() + w.pull() + _new_config(w, "Orders", "ok-secret") + _new_config(w, "Contacts", "bad-secret") + w.api.encrypt_values = MagicMock(side_effect=_encrypt(fail_on)) + return w + + +def _world_with_an_edit_and_a_new_config(tmp_path: Path) -> World: + """A tracked config with a new secret, then a new config whose secret fails. + + Push writes tracked configs before new ones, so the update used to reach + the remote before the create failed. + """ + w = World(tmp_path) + w.api.put(PROD, "cfg-1", "Orders", "a", extra={"#token": "KBC::ProjectSecure::old"}) + w.init() + w.pull() + config_file = w.config_dir("orders") / CONFIG_FILENAME + data = yaml.safe_load(config_file.read_text(encoding="utf-8")) + data["parameters"]["#token"] = "ok-secret" + config_file.write_text(yaml.safe_dump(data), encoding="utf-8") + _new_config(w, "Contacts", "bad-secret") + w.api.encrypt_values = MagicMock(side_effect=_encrypt("bad-secret")) + return w + + +def test_encryption_failure_stops_the_push_before_any_write(tmp_path: Path) -> None: + w = _world_with_an_edit_and_a_new_config(tmp_path) + manifest_before = load_manifest(w.root).model_dump() + files_before = {p: p.read_text() for p in w.root.rglob(CONFIG_FILENAME)} + + with pytest.raises(KeboolaApiError) as exc_info: + w.push() + + assert exc_info.value.error_code == ErrorCode.ENCRYPTION_FAILED + assert "Nothing was pushed." in exc_info.value.message + assert w.api.log == [] + assert load_manifest(w.root).model_dump() == manifest_before + assert {p: p.read_text() for p in w.root.rglob(CONFIG_FILENAME)} == files_before + + +def test_retry_after_the_failure_applies_each_change_once(tmp_path: Path) -> None: + w = _world_with_an_edit_and_a_new_config(tmp_path) + with pytest.raises(KeboolaApiError): + w.push() + + w.api.encrypt_values = MagicMock(side_effect=_encrypt()) + result = w.push() + + assert (result["updated"], result["created"]) == (1, 1) + assert sorted(line.split()[0] for line in w.api.log) == ["CREATE", "UPDATE"] + assert w.diff()["changes"] == [] + + +def test_secrets_are_encrypted_once_and_the_writes_send_the_ciphertext(tmp_path: Path) -> None: + w = _world_with_two_new_configs(tmp_path, fail_on=None) + + w.push() + + # One call for the component, before the writes; none from the writes. + assert w.api.encrypt_values.call_count == 1 + sent = sorted( + cfg["configuration"]["parameters"]["#token"] for cfg in w.api.remote[PROD].values() + ) + assert sent == ["KBC::ProjectSecure::bad-secret", "KBC::ProjectSecure::ok-secret"] + + +# --------------------------------------------------------------------------- +# PushPlan.encrypt_secrets and the known-value cache +# --------------------------------------------------------------------------- + + +def _write_yaml(path: Path, data: dict[str, Any]) -> None: + path.mkdir(parents=True, exist_ok=True) + (path / CONFIG_FILENAME).write_text(yaml.safe_dump(data), encoding="utf-8") + + +def _service() -> MagicMock: + service = MagicMock() + service._read_config_file.side_effect = lambda config_dir: ( + yaml.safe_load((config_dir / CONFIG_FILENAME).read_text(encoding="utf-8")) + if (config_dir / CONFIG_FILENAME).exists() + else None + ) + return service + + +def _manifest() -> Manifest: + return Manifest( + project=ManifestProject(id=258, apiHost="connection.example.com"), + naming=ManifestNaming(), + configurations=[ + ManifestConfiguration( + branchId=0, + componentId=COMP, + id="cfg-1", + path="extractor/keboola.ex-http/orders", + rows=[ManifestConfigRow(id="row-1", path="rows/first")], + ) + ], + ) + + +def test_encrypt_secrets_covers_rows_and_skips_what_push_does_not_write(tmp_path: Path) -> None: + config_dir = tmp_path / "extractor/keboola.ex-http/orders" + _write_yaml(config_dir, {"name": "Orders", "parameters": {"#token": "config-secret"}}) + _write_yaml(config_dir / "rows/first", {"name": "First", "parameters": {"#pw": "row-secret"}}) + _write_yaml(tmp_path / "gone", {"name": "Gone", "parameters": {"#pw": "deleted-secret"}}) + client = MagicMock() + client.encrypt_values.side_effect = _encrypt() + plan = PushPlan( + changes=[ + { + "change_type": "modified", + "component_id": COMP, + "path": "extractor/keboola.ex-http/orders", + }, + { + "change_type": "modified", + "component_id": COMP, + "path": "rows/first", + "is_row": True, + "parent_config_id": "cfg-1", + }, + { + "change_type": "added", + "component_id": COMP, + "path": "rows/orphan", + "is_row": True, + "parent_config_id": "not-tracked", + }, + {"change_type": "deleted", "component_id": COMP, "path": "gone"}, + ], + skipped=[], + skipped_deletions=[], + ) + + cache = plan.encrypt_secrets(_service(), client, tmp_path, _manifest(), False) + + assert cache == { + (COMP, "config-secret"): "KBC::ProjectSecure::config-secret", + (COMP, "row-secret"): "KBC::ProjectSecure::row-secret", + } + assert client.encrypt_values.call_count == 1 + + +def test_encrypt_secrets_never_caches_a_value_the_api_left_in_plaintext(tmp_path: Path) -> None: + """A partial API answer must not turn a plaintext value into a cache hit.""" + config_dir = tmp_path / "orders" + _write_yaml(config_dir, {"name": "Orders", "parameters": {"#a": "first", "#b": "second"}}) + client = MagicMock() + client.encrypt_values.return_value = {"#secret0": "KBC::ProjectSecure::first"} + plan = PushPlan( + changes=[{"change_type": "added", "component_id": COMP, "path": "orders"}], + skipped=[], + skipped_deletions=[], + ) + + cache = plan.encrypt_secrets(_service(), client, tmp_path, _manifest(), False) + + assert cache == {(COMP, "first"): "KBC::ProjectSecure::first"} + + +def test_encrypt_secrets_does_nothing_with_the_plaintext_fallback(tmp_path: Path) -> None: + config_dir = tmp_path / "orders" + _write_yaml(config_dir, {"name": "Orders", "parameters": {"#token": "config-secret"}}) + client = MagicMock() + plan = PushPlan( + changes=[{"change_type": "added", "component_id": COMP, "path": "orders"}], + skipped=[], + skipped_deletions=[], + ) + + assert plan.encrypt_secrets(_service(), client, tmp_path, _manifest(), True) == {} + client.encrypt_values.assert_not_called() + + +def test_known_values_are_applied_without_an_api_call() -> None: + client = MagicMock() + client.encrypt_values.side_effect = _encrypt() + configuration = {"parameters": {"#known": "a", "nested": {"#new": "b"}}} + + encrypt_secrets_in_config( + client, 258, COMP, configuration, known={(COMP, "a"): "KBC::ProjectSecure::cached"} + ) + + assert configuration == { + "parameters": { + "#known": "KBC::ProjectSecure::cached", + "nested": {"#new": "KBC::ProjectSecure::b"}, + } + } + client.encrypt_values.assert_called_once_with( + project_id=258, component_id=COMP, data={"#parameters.nested.#new": "b"} + ) diff --git a/tests/test_sync_reconcile.py b/tests/test_sync_reconcile.py index dd4ed284e..51783e6e6 100644 --- a/tests/test_sync_reconcile.py +++ b/tests/test_sync_reconcile.py @@ -27,6 +27,7 @@ from keboola_agent_cli.config_store import ConfigStore from keboola_agent_cli.constants import CONFIG_FILENAME from keboola_agent_cli.models import TokenVerifyResponse +from keboola_agent_cli.services.component_service import pushed_config_base_hash from keboola_agent_cli.services.sync_service import SyncService from keboola_agent_cli.sync.manifest import load_manifest, save_manifest @@ -474,12 +475,25 @@ def test_adopted_by_id_push_registers_manifest_entry(tmp_config_dir: Path, tmp_p Pre-#497 the update succeeded but no entry was written, so every subsequent diff re-adopted the file and a later local delete was - invisible (never classified ``deleted``). + invisible (never classified ``deleted``). The file carries the baseline a + ``config new --push --output-dir`` scaffold records, so its local edit is + ``modified`` rather than a conflict (issue #792 E). """ project_root = tmp_path / "project" project_root.mkdir() store = _init_and_pull(tmp_config_dir, project_root, REMOTE_V1) + remote = _http_extractor("https://api.example.com") + remote[0]["configurations"].append( + { + "id": "cfg-777", + "name": "Adopted Extractor", + "description": "", + "configuration": {"parameters": {"baseUrl": "https://original.example.com"}}, + "rows": [], + } + ) + adopted_path = "extractor/keboola.ex-http/adopted-extractor" adopted_dir = project_root / "main" / adopted_path adopted_dir.mkdir(parents=True) @@ -493,22 +507,14 @@ def test_adopted_by_id_push_registers_manifest_entry(tmp_config_dir: Path, tmp_p "_keboola": { "component_id": "keboola.ex-http", "config_id": "cfg-777", + "base_config_hash": pushed_config_base_hash( + "keboola.ex-http", "cfg-777", remote[0]["configurations"][-1] + ), }, } ), encoding="utf-8", ) - - remote = _http_extractor("https://api.example.com") - remote[0]["configurations"].append( - { - "id": "cfg-777", - "name": "Adopted Extractor", - "description": "", - "configuration": {"parameters": {"baseUrl": "https://original.example.com"}}, - "rows": [], - } - ) client = _make_mock_client(components_response=remote) svc = _svc_with_client(store, client) diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index 480602e15..a68f02289 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -2977,13 +2977,14 @@ def test_push_on_dev_branch_after_pull_is_noop( assert push_result["created"] == 0 dev_client.create_config.assert_not_called() - def test_untracked_file_with_known_remote_id_updates_instead_of_creating( + def test_untracked_file_with_known_remote_id_is_not_created_again( self, tmp_config_dir: Path, tmp_path: Path ) -> None: """Adopt-by-id guard: an untracked ``_config.yml`` whose ``_keboola.config_id`` exists on the target branch and is not claimed - by any manifest entry is diffed as ``modified`` (an update), never - ``added`` (a duplicate create).""" + by any manifest entry is diffed against that config, never as + ``added`` (a duplicate create). It has no baseline, so a difference + is a ``conflict`` that push does not apply (issue #792 E).""" project_root = tmp_path / "project" project_root.mkdir() store = self._init_and_pull_main(tmp_config_dir, project_root, SAMPLE_COMPONENTS_NO_ROWS) @@ -3031,16 +3032,16 @@ def test_untracked_file_with_known_remote_id_updates_instead_of_creating( diff_result = svc.diff(alias="prod", project_root=project_root) assert diff_result["summary"]["added"] == 0 - assert diff_result["summary"]["modified"] == 1 - modified = [c for c in diff_result["changes"] if c["change_type"] == "modified"] - assert modified[0]["config_id"] == "cfg-777" + assert diff_result["summary"]["conflict"] == 1 + conflicts = [c for c in diff_result["changes"] if c["change_type"] == "conflict"] + assert conflicts[0]["config_id"] == "cfg-777" push_result = svc.push(alias="prod", project_root=project_root) assert push_result["created"] == 0 - assert push_result["updated"] == 1 + assert push_result["updated"] == 0 + assert push_result["skipped"] == 1 client.create_config.assert_not_called() - client.update_config.assert_called_once() - assert client.update_config.call_args.kwargs["config_id"] == "cfg-777" + client.update_config.assert_not_called() def test_untracked_copy_of_tracked_config_still_creates_fresh( self, tmp_config_dir: Path, tmp_path: Path