From 40dd8497d60991c436b35594ed8e275688356710 Mon Sep 17 00:00:00 2001 From: soustruh Date: Tue, 29 Sep 2026 15:38:42 +0200 Subject: [PATCH 1/2] fix(sync): push deletes only with --force and skips remote-deleted configs (#792 G, H) --- CLAUDE.md | 4 + formal/sync/README.md | 17 +-- plugins/kbagent/agents/keboola-expert.md | 5 +- .../skills/kbagent-cicd-migration/SKILL.md | 2 + .../kbagent-promotion-pipeline/SKILL.md | 9 +- .../scripts/generate_promotion_pipeline.py | 17 ++- .../kbagent/references/commands-reference.md | 4 +- .../skills/kbagent/references/gotchas.md | 48 +++++++- .../kbagent/references/sync-rows-workflow.md | 6 +- .../kbagent/references/sync-workflow.md | 18 ++- .../commands/_sync_push_render.py | 52 ++++++++ src/keboola_agent_cli/commands/context.py | 5 +- src/keboola_agent_cli/commands/sync.py | 40 +++--- src/keboola_agent_cli/services/_sync_clone.py | 2 + .../services/_sync_push_ops.py | 63 +++++++++- .../services/sync_service.py | 37 +++--- src/keboola_agent_cli/sync/branch_scope.py | 37 +++++- src/keboola_agent_cli/sync/clone.py | 16 +++ src/keboola_agent_cli/sync/diff_engine.py | 32 ++++- tests/test_generate_promotion_pipeline.py | 17 +++ tests/test_sync_branch_scope.py | 61 ++++++++++ tests/test_sync_cli.py | 69 ++++++++++- tests/test_sync_diff_engine.py | 55 +++++++++ tests/test_sync_formal_counterexamples.py | 115 +++++++++++------- tests/test_sync_service.py | 77 +++++++++++- 25 files changed, 690 insertions(+), 118 deletions(-) create mode 100644 src/keboola_agent_cli/commands/_sync_push_render.py create mode 100644 tests/test_sync_branch_scope.py diff --git a/CLAUDE.md b/CLAUDE.md index a1ffd3cf6..e16b9a1b1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -962,6 +962,10 @@ kbagent sync pull --project ALIAS [--all-projects] [--force] [--theirs] [--dry-r kbagent sync status [--directory DIR] kbagent sync diff --project ALIAS [--all-projects] [--directory DIR] [--branch ID] kbagent sync push --project ALIAS [--all-projects] [--dry-run] [--force] [--allow-plaintext-on-encrypt-failure] [--branch ID] [--no-name-drift-warnings] +# sync push --force (#792): push deletes remote configs and rows ONLY with --force; a plain push lists them +# 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 push (since 0.91.0, #686): the manifest baseline `pull_config_hash` is stamped from the API # response (or a read-back), never from disk -- push-deployed multi-statement SQL transformations # (and anything disabled in the UI whose local YAML lacks `is_disabled`) no longer show permanent diff --git a/formal/sync/README.md b/formal/sync/README.md index 419f9b007..6ab19bbcd 100644 --- a/formal/sync/README.md +++ b/formal/sync/README.md @@ -77,13 +77,13 @@ explores traces of up to 5 actions. BFS returns the shortest counterexample. | I7 never-fetched entry never deleted | **holds** (104,809 states, never-fetched initial entry) | | I1 no double create | violated: a promote push re-creates configs on every run; a resurrect followed by an `ENCRYPTION_FAILED` abort also creates a copy | | I2 push deletes only user-removed dirs | violated: **pull's stale-entry sweep deletes a directory the same pull just wrote, and the next push deletes the live remote config** | -| I2b delete requires `--force` | violated: `push()` never reads `force` (S1) | +| I2b delete requires `--force` | violated: `push()` never reads `force` (S1). Fixed in the code (G); the model is unchanged | | 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 | -| I11b no silent resurrect | violated: a remote delete followed by any push re-creates the config, even with no local edit (S2) | +| 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) | Each violation was checked against the code. The main ones were replayed @@ -105,8 +105,8 @@ the regression test for each in `tests/test_sync_formal_counterexamples.py`. | 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` | -| 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 (product decision) | `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 | `test_h_push_does_not_silently_resurrect_deleted_config` | +| 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` | | J | `sync pull --branch dev` reports untouched production configs as "removed" | Spec S4 | LOW | `test_j_branch_scoped_pull_does_not_report_other_branch_configs_removed` | | K | A cosmetic local edit (raw vs normalized hash) blocks pull from ever applying a real remote change; `--force` raises a conflict | Spec S3, TLA I12 | LOW (documented, conservative behavior) | `test_k_cosmetic_edit_is_conservative_not_unsafe` (unmarked regression guard, not xfail) | @@ -120,10 +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, H and I reproduce on current code and are `xfail(strict=True)` (D is fixed) -- -flipping to a hard failure the moment a fix lands is the point: delete the -`xfail` marker to adopt the fix. G is kept `xfail` too even though the fix -direction is a product decision (see the test's docstring). J reproduces and +E, F and I reproduce on current code and are `xfail(strict=True)` (D, 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 +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 (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 2e08d1e6c..0c3f9ce58 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -306,7 +306,10 @@ its absence is NOT a promise the entry is version-independent (see ยง1 Rule 6). `is_disabled: true` in `_config.yml` = config disabled (absent = enabled); a `never_fetched` warning on diff/push = run `sync pull` first; a non-zero `summary.orphaned` (0.89.0+, #649) = the manifest is targeted at another - branch's tree -- `sync pull` to re-target, never push. `sync status` + branch's tree -- `sync pull` to re-target, never push. Since vNEXT (#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` 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-cicd-migration/SKILL.md b/plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md index e412fdf1b..f2d4aa7cb 100644 --- a/plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md +++ b/plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md @@ -259,6 +259,8 @@ for the full mapping from the old `secrets.KBC_SAPI_TOKEN_*` / `vars.KBC_*` sche push is fail-closed by design. - `sync push --force` deletes remote configs removed locally. It is wired to the `allow_delete` workflow input (default off). Treat it like the old `--force`. + Without it push deletes nothing and lists the deletions (since vNEXT; older + kbagent deleted without `--force`, so `allow_delete` off did not stop them). - Tokens live **only** in GitHub secrets and are injected as env vars per step; the generated workflows never write a `config.json` to disk. - **Never run `--all-projects` in a directory that also holds a flat single-project diff --git a/plugins/kbagent/skills/kbagent-promotion-pipeline/SKILL.md b/plugins/kbagent/skills/kbagent-promotion-pipeline/SKILL.md index 431fe4879..8d43324b3 100644 --- a/plugins/kbagent/skills/kbagent-promotion-pipeline/SKILL.md +++ b/plugins/kbagent/skills/kbagent-promotion-pipeline/SKILL.md @@ -40,19 +40,24 @@ two Storage API tokens (source, destination): (using a PAT, not the default token -- see [references/secrets-setup.md](references/secrets-setup.md)). 2. **Validate** (`kbagent-promote-validate.yml`, on the PR) runs - `sync push --dry-run --project __env__ --directory ` against the + `sync push --dry-run --force --project __env__ --directory ` against the **destination** project's token, once per configured pipeline (the `paths:` trigger only gates whether the workflow runs at all, not which pipeline steps execute inside it -- every pipeline's dry-run always runs) -- this is the cross-project diff: *if this PR merges, here is exactly what changes in the destination project.* Read this before approving. 3. **Push** (`kbagent-promote-push.yml`, on push to `main`) runs, in a - **separate job per pipeline**, `sync push --project __env__ --directory + **separate job per pipeline**, `sync push --force --project __env__ --directory ` against the **destination** project's token, each job gated by the `prod` GitHub Environment (add required reviewers there -- every job run gets its own separate approval, so approving one pipeline never approves another). +Both steps pass `--force`, so a config deleted in the source is deleted in +the destination too. Without it `sync push` deletes nothing *(since vNEXT, +#792)*; a pipeline generated before that relied on push deleting without +`--force`, so regenerate it or add `--force` to both steps by hand. + `main` therefore always represents "the last thing approved and pushed to every destination project" -- the reviewable source of truth the whole repo is built around. A promotion is: pull opens a PR -> validate shows the diff --git a/plugins/kbagent/skills/kbagent-promotion-pipeline/scripts/generate_promotion_pipeline.py b/plugins/kbagent/skills/kbagent-promotion-pipeline/scripts/generate_promotion_pipeline.py index 32d88f403..6a83eb7d0 100644 --- a/plugins/kbagent/skills/kbagent-promotion-pipeline/scripts/generate_promotion_pipeline.py +++ b/plugins/kbagent/skills/kbagent-promotion-pipeline/scripts/generate_promotion_pipeline.py @@ -10,12 +10,15 @@ Pulls every pipeline's directory from its SOURCE project and opens/updates one PR against the main branch with the combined diff. 2. kbagent-promote-validate.yml (pull_request against main) - For every pipeline, runs `sync push --dry-run` against the DESTINATION - project -- this is the cross-project diff: "if this PR merges, here is - exactly what changes in the destination project." + For every pipeline, runs `sync push --dry-run --force` against the + DESTINATION project -- this is the cross-project diff: "if this PR merges, + here is exactly what changes in the destination project." 3. kbagent-promote-push.yml (push to main, environment-gated) Pushes every pipeline's directory to its DESTINATION project once the PR - has merged. + has merged, with `--force`: a config deleted in the SOURCE is deleted in + the DESTINATION too. Without `--force`, `sync push` deletes nothing + (since vNEXT, #792), and the validate dry-run passes it for the same + reason, so it shows what the push does. Each pipeline needs two Storage API token secrets (`KBC_TOKEN__SOURCE` / `KBC_TOKEN__DEST`) and uses kbagent's `KBAGENT_PROJECT_FROM_ENV=1` / @@ -364,7 +367,7 @@ def gen_validate(pipelines: list[Pipeline]) -> str: _pipeline_step( p, "Destination dry-run", - "push --dry-run", + "push --dry-run --force", p.dest_token_secret, p.dest_stack_url, json_output=True, @@ -414,7 +417,9 @@ def _push_job(p: Pipeline) -> str: f"{_INSTALL_TOKEN}" " # `sync push` encrypts #-secrets fail-closed by default. Do NOT add\n" " # --allow-plaintext-on-encrypt-failure in CI.\n" - f"{_pipeline_step(p, 'Push', 'push', p.dest_token_secret, p.dest_stack_url)}" + " # --force: a config deleted in the source is deleted here too;\n" + " # without it, `sync push` deletes nothing.\n" + f"{_pipeline_step(p, 'Push', 'push --force', p.dest_token_secret, p.dest_stack_url)}" ) diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index 594af3090..33864977a 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -360,9 +360,9 @@ Requires the project to be added with its **master ('owner') Storage API token** ## Sync (GitOps) - `sync init --project ALIAS [--directory DIR] [--git-branching] [--adopt-existing]` -- initialize sync working directory; `--adopt-existing` adopts a `.keboola/manifest.json` already written by the kbc Go CLI without overwriting (idempotent; validates `project_id` against the alias token) - `sync pull --project ALIAS [--all-projects] [--force] [--theirs] [--dry-run] [--with-samples] [--no-storage] [--no-jobs] [--job-limit N] [--branch ID]` -- download configs to local files. **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` round-trips (since v0.72.0) as sparse `is_disabled: true` in `_config.yml` -- absent key = enabled. `--branch` (0.47.0+) per-invocation dev-branch override, beats every other branch source. Ignored components (since 0.91.0): `keboola.sandboxes` + `keboola.mcp-server-tool` are always excluded, 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. -- `sync push --project ALIAS [--all-projects] [--dry-run] [--force] [--allow-plaintext-on-encrypt-failure] [--branch ID] [--no-name-drift-warnings]` -- push local changes (auto-encrypts secrets, fails if encryption fails). Fresh-CREATE writeback updates placeholder manifest entries in place (since 0.47.0) and propagates any `KBC.configuration.*` metadata via `set_config_metadata`. Fresh-CREATE variable binding (since 0.47.2): when a `keboola.variables` config + its values row are created alongside a transformation in the same push, the transformation's `variables_id` / `variables_values_id` placeholders are rebound to the assigned ULIDs and the row's `values` are hoisted even without a `_keboola` block, so `job run` succeeds with no post-push `config variables-set` step (unresolvable/ambiguous links surface a `variable_link` entry in `errors[]`, never a broken link). Never-fetched guard (since v0.72.0): a manifest entry with an empty `pull_hash` and no local files (pre-0.72 name-collision phantom) is **never** planned as a remote DELETE -- diff/push exclude it and report it under `never_fetched` with a warning (run `sync pull` to materialize); local deletion of a properly-pulled config still deletes on push. Adopted-by-id writeback (since v0.72.0): pushing an untracked file whose `_keboola.config_id` resolves on the branch also writes the manifest entry, so follow-up diffs are stable. `--branch` (0.47.0+) per-invocation override; when no `/` subtree exists on disk (since 0.47.2) the local default tree (`main/`) is promoted to the target branch (API writes still target the branch id); `--no-name-drift-warnings` (0.47.0+) drops the cosmetic warnings array. Branch-scoped since v0.89.0 (issue #649): push consumes the diff's changeset, so configs tracked on another branch's tree are never planned as creates -- they ride along on the result envelope under `orphaned` instead (see `sync diff`). **Since 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. +- `sync push --project ALIAS [--all-projects] [--dry-run] [--force] [--allow-plaintext-on-encrypt-failure] [--branch ID] [--no-name-drift-warnings]` -- push local changes (auto-encrypts secrets, fails if encryption fails). Fresh-CREATE writeback updates placeholder manifest entries in place (since 0.47.0) and propagates any `KBC.configuration.*` metadata via `set_config_metadata`. Fresh-CREATE variable binding (since 0.47.2): when a `keboola.variables` config + its values row are created alongside a transformation in the same push, the transformation's `variables_id` / `variables_values_id` placeholders are rebound to the assigned ULIDs and the row's `values` are hoisted even without a `_keboola` block, so `job run` succeeds with no post-push `config variables-set` step (unresolvable/ambiguous links surface a `variable_link` entry in `errors[]`, never a broken link). Never-fetched guard (since v0.72.0): a manifest entry with an empty `pull_hash` and no local files (pre-0.72 name-collision phantom) is **never** planned as a remote DELETE -- diff/push exclude it and report it under `never_fetched` with a warning (run `sync pull` to materialize); local deletion of a properly-pulled config deletes on `push --force`; since vNEXT (#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. - `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`). **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 vNEXT)*. Re-pull a tree pulled by an older version before cloning a Streamlit app, or it is created as `python-js`. **Storage buckets (since vNEXT)**: 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. -- `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`. +- `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 vNEXT (#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`. - `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 b187268a3..5c8b865a8 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -4206,8 +4206,9 @@ Four related sync-engine behaviors landed together (issues #466 / #467 / #472 / whose directory/`_config.yml` was deleted locally is re-fetched on the next pull even when the remote is unchanged (manifest<->disk invariant). The old behavior silently reported "Already up to date". NOTE the interplay with the - GitOps delete flow: delete-dir-then-PUSH still deletes the remote config; - delete-dir-then-PULL now restores it instead of doing nothing. + GitOps delete flow: delete-dir-then-PUSH still deletes the remote config + (since vNEXT only with `push --force`, #792); delete-dir-then-PULL now + restores it instead of doing nothing. - **Config-level `isDisabled` round-trips.** Pull writes a sparse `is_disabled: true` line into `_config.yml` (absent key = enabled -- old trees do not mass-diff), `sync diff` surfaces enabled/disabled drift (a config @@ -4220,7 +4221,8 @@ Four related sync-engine behaviors landed together (issues #466 / #467 / #472 / remote config. diff/push report it under `never_fetched` (JSON key + human warning); the next `sync pull` materializes it. A properly-pulled config (non-empty `pull_hash`) that you delete locally is still planned as a remote - DELETE on push -- the guard only protects entries that were never on disk. + DELETE on push (applied since vNEXT only with `--force`, #792) -- the guard + only protects entries that were never on disk. - **Adopted-by-id push writes the manifest.** Pushing an untracked local file whose `_keboola.config_id` resolves on the target branch (the #482 adopt-update path) now also creates the manifest entry (fresh @@ -5464,5 +5466,41 @@ drops manifest entries whose config is gone from the remote) are closed: `reason: "deleted on remote"`. Only `pull --theirs` still deletes it (remote wins). "Edited" covers `_config.yml`, companion files (`transform.sql`, `code.py`, `_description.md`, ...) and row files. A kept directory is still - tracked, so the next `sync push` re-creates the config remotely -- delete the - directory if the remote delete was intended. + tracked, but `sync push` does not re-create it (it is `remote_deleted`, see + the next entry) -- delete the directory if the remote delete was intended. + +## `sync push` deletes only with `--force` and never re-creates a config deleted on the remote (#792) + +*(since vNEXT)* Two changes to what `sync push` sends: + +- **Deletions need `--force`.** Before, push deleted a remote config or row as + soon as its local files were gone, with or without `--force`, although the + `--force` help said it allows the deletion. Now a plain push deletes nothing. + It lists each held-back deletion under `skipped_deletions`, with + `skipped_deletions_reason`, also in `--dry-run`. The `--dry-run` + `summary.deleted` counts only the deletions push would apply. `push --force` + deletes as before. A script that deletes by removing a directory must add + `--force`. +- **No silent re-create.** Before, a config or row deleted on the remote by + someone else was created again under a new id by the next push, with no + notice. Now `sync diff` reports it as `remote_deleted` (human: `- REMOTE + DELETED`, `summary.remote_deleted`), push skips it and reports `skipped` with + `skipped_reason`, and `sync pull` removes the local copy (or keeps a locally + edited one, see the previous entry). Configs never fetched from the target + are still created: a `sync clone` copy, a promote push from `main/`, a + hand-written placeholder entry. +- `skipped` / `skipped_reason` now appear in every push result (`pushed`, + `dry_run`), not only in `no_changes`. +- **To keep a config someone deleted on the remote**, restore it with + `kbagent config restore` (a config deleted once is in the trash, and the + restored config keeps its id, so the next diff matches it again). This also covers a dev + copy that a promote push created and someone then deleted in the branch. +- **Pipelines.** The `kbagent-promotion-pipeline` workflows now pass `--force` + in the validate dry-run and in the push, so a config deleted in the source is + deleted in the destination. A pipeline generated earlier needs `--force` in + both steps. The `kbagent-cicd-migration` push passes `--force` only when its + `allow_delete` input is set. +- **`sync clone` re-runs.** A target directory written by an older kbagent + whose first clone run failed part-way can report the configs it never + created as `remote_deleted`. Delete that target directory and run the clone + again. diff --git a/plugins/kbagent/skills/kbagent/references/sync-rows-workflow.md b/plugins/kbagent/skills/kbagent/references/sync-rows-workflow.md index 772798f71..948f0f1eb 100644 --- a/plugins/kbagent/skills/kbagent/references/sync-rows-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/sync-rows-workflow.md @@ -219,8 +219,10 @@ Row diff is the same 3-way engine as parent configs, just keyed by row id: | no | no | `conflict` (manual resolve) | Added rows (filesystem-only) show as `added`; removed rows (manifest-only -after file deletion) show as `deleted` -- push DELETEs them via -`_push_delete_row`. +after file deletion) show as `deleted` -- `push --force` DELETEs them via +`_push_delete_row`, a plain push lists them under `skipped_deletions` +*(since vNEXT, #792)*. A row deleted on the remote since the last pull shows +as `remote_deleted`; push never re-creates it. Human-mode diff output prints row-level changes with the same `+`/`~`/`-`/`=` prefixes as parent configs, e.g.: diff --git a/plugins/kbagent/skills/kbagent/references/sync-workflow.md b/plugins/kbagent/skills/kbagent/references/sync-workflow.md index 20b3fa673..4c606b1b1 100644 --- a/plugins/kbagent/skills/kbagent/references/sync-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/sync-workflow.md @@ -88,8 +88,10 @@ kbagent sync diff --project prod Related semantics (all since v0.72.0): - Plain `sync pull` re-materializes a tracked config whose local dir was - deleted (delete-dir-then-pull refetches; delete-dir-then-PUSH still deletes - the remote config -- the direction of the command picks the winner). + deleted (delete-dir-then-pull refetches; delete-dir-then-`push --force` + deletes the remote config -- the direction of the command picks the winner). + *(since vNEXT, #792)* A plain `sync push` deletes nothing: it lists the + deletion under `skipped_deletions` and says to add `--force`. - Config-level enabled/disabled state round-trips: `_config.yml` carries `is_disabled: true` for disabled configs (absent = enabled), `sync diff` shows the drift, push updates the remote state when the key is present. @@ -414,7 +416,8 @@ Stored in `.keboola/branch-mapping.json`: | REMOTE MODIFIED | Remote changed, local unchanged | Run pull to fetch | | CONFLICT | Both sides changed | Resolve manually, then push | | ADDED | New local config | Push creates it | -| DELETED | Local file removed | Push deletes from remote | +| DELETED | Local file removed | `push --force` deletes from remote; a plain push lists it under `skipped_deletions` *(since vNEXT)* | +| REMOTE DELETED | Deleted on the remote since the last pull | Run pull; push never re-creates it *(since vNEXT)* | ## Key behaviors @@ -425,7 +428,9 @@ Stored in `.keboola/branch-mapping.json`: ...) protects the config the same way *(since vNEXT, #792)*. Before, such an edit was silently overwritten whenever the remote changed, plain or `--force` - **`--force` is conflict-aware**: see below -- it no longer blindly overwrites -- **Push only sends local changes**: remote_modified and conflict changes are skipped +- **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 vNEXT, #792)* - **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 @@ -500,8 +505,9 @@ was overwritten: with `SYNC_CONFLICT` (conflict `reason: "deleted on remote"`). Plain pull keeps the edited directory and its manifest entry and reports it as `skipped` (`locally modified, deleted on remote`). Only `--theirs` deletes it. - A kept directory stays tracked, so the next `sync push` re-creates the config; - delete the directory if the remote delete was intended. + A kept directory stays tracked, but `sync push` does not re-create the config + (it diffs as `remote_deleted`); delete the directory if the remote delete was + intended, or restore the config with `kbagent config restore` to keep it. > A config deleted and re-created remotely under the same name *(since vNEXT, > #792)* is written to a suffixed directory while the old one is removed; the diff --git a/src/keboola_agent_cli/commands/_sync_push_render.py b/src/keboola_agent_cli/commands/_sync_push_render.py new file mode 100644 index 000000000..89d27d4ce --- /dev/null +++ b/src/keboola_agent_cli/commands/_sync_push_render.py @@ -0,0 +1,52 @@ +"""Human output for what ``sync push`` holds back (issue #792 G, H). + +Split out of ``commands/sync.py``, which is at its size ceiling. Push lists +what it did not apply in every result: remote-side changes that need a +``sync pull`` (``skipped``) and the deletions it held back because +``--force`` was not given (``skipped_deletions``). +""" + +from __future__ import annotations + +from typing import Any + +# Remote-side change types in the diff, labelled for human output. +REMOTE_CHANGE_LABELS = { + "remote_modified": "~ REMOTE MODIFIED", + "remote_deleted": "- REMOTE DELETED", +} + + +def print_push_skips(formatter: Any, result: dict[str, Any]) -> None: + """Print the changes a push result did not apply, with the next step. + + A notice, not a result, so it goes to stderr like every other hint. + """ + skipped_reason = result.get("skipped_reason") + if skipped_reason: + formatter.err_console.print(f" [yellow]{skipped_reason}[/yellow]") + deletions = result.get("skipped_deletions", []) + if not deletions: + return + formatter.err_console.print( + f" [yellow]{len(deletions)} deletion(s) not applied:[/yellow] " + f"{result.get('skipped_deletions_reason', '')}" + ) + for change in deletions: + kind = "row" if change.get("is_row") else "config" + formatter.err_console.print( + f" - {change.get('component_id')}/{change.get('config_id')} " + f"[dim]({kind})[/dim] {change.get('config_name', '')}" + ) + + +def push_skips_one_liner(result: dict[str, Any]) -> str: + """Short suffix for the ``--all-projects`` push line; empty when nothing was held back.""" + parts = [] + if result.get("skipped"): + parts.append(f"[cyan]{result['skipped']} to pull first[/cyan]") + if result.get("skipped_deletions"): + parts.append( + f"[yellow]{len(result['skipped_deletions'])} deletion(s) need --force[/yellow]" + ) + return "".join(f", {part}" for part in parts) diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index a2368e45b..867724db3 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -1625,7 +1625,10 @@ Never-fetched guard: 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 it. Local deletion of a properly-pulled config still deletes on push. + to materialize it. Local deletion of a properly-pulled config deletes on push --force + only (#792): a plain push deletes nothing and lists the deletion under skipped_deletions, + also in --dry-run. A config or row deleted on the remote since the last pull is + remote_deleted: push never re-creates it, sync pull removes the local copy. Adopted-by-id writeback: pushing an untracked local file whose _keboola.config_id resolves on the branch (adopt-update, #482) now also writes the manifest entry, so follow-up diffs are stable and a later local delete is detected. diff --git a/src/keboola_agent_cli/commands/sync.py b/src/keboola_agent_cli/commands/sync.py index 16543df08..9f4e965b5 100644 --- a/src/keboola_agent_cli/commands/sync.py +++ b/src/keboola_agent_cli/commands/sync.py @@ -12,6 +12,7 @@ from ..constants import SYNC_ORPHAN_PREVIEW_LIMIT from ..errors import ConfigError, ErrorCode, KeboolaApiError, SyncConflictError from ._helpers import check_cli_permission, get_formatter, get_service, map_error_to_exit_code +from ._sync_push_render import REMOTE_CHANGE_LABELS, print_push_skips, push_skips_one_liner sync_app = typer.Typer(help="Sync project configurations with local filesystem") @@ -307,7 +308,7 @@ def _format_diff_result(formatter: Any, result: dict) -> None: return local_changes = [c for c in changes if c["change_type"] in ("added", "modified", "deleted")] - remote_changes = [c for c in changes if c["change_type"] == "remote_modified"] + remote_changes = [c for c in changes if c["change_type"] in REMOTE_CHANGE_LABELS] conflict_changes = [c for c in changes if c["change_type"] == "conflict"] if local_changes: @@ -318,11 +319,12 @@ def _format_diff_result(formatter: Any, result: dict) -> None: formatter.console.print(f" {prefix} {ct.upper()} {label}") formatter.console.print( f" {summary['added']} to create, {summary['modified']} to update, " - f"{summary['deleted']} to delete" + f"{summary['deleted']} to delete (push --force)" ) if remote_changes: for change in remote_changes: - formatter.console.print(f" ~ REMOTE MODIFIED {_change_label(change)}") + label = REMOTE_CHANGE_LABELS[change["change_type"]] + formatter.console.print(f" {label} {_change_label(change)}") if conflict_changes: for change in conflict_changes: formatter.console.print(f" ! CONFLICT {_change_label(change)}") @@ -354,6 +356,7 @@ def _format_push_result(formatter: Any, result: dict) -> None: status = result.get("status", "") if status == "no_changes": formatter.console.print(" No changes to push.") + print_push_skips(formatter, result) _format_never_fetched(formatter, result.get("never_fetched", [])) _format_orphaned(formatter, result.get("orphaned", [])) return @@ -364,6 +367,7 @@ def _format_push_result(formatter: Any, result: dict) -> None: f"update {summary.get('modified', 0)}, " f"delete {summary.get('deleted', 0)}" ) + print_push_skips(formatter, result) _format_never_fetched(formatter, result.get("never_fetched", [])) _format_orphaned(formatter, result.get("orphaned", [])) return @@ -372,6 +376,7 @@ def _format_push_result(formatter: Any, result: dict) -> None: f"{result.get('updated', 0)} updated, " f"{result.get('deleted', 0)} deleted" ) + print_push_skips(formatter, result) _format_never_fetched(formatter, result.get("never_fetched", [])) _format_orphaned(formatter, result.get("orphaned", [])) # Show name drift warnings @@ -426,7 +431,7 @@ def _diff_one_liner(result: dict) -> str: mod = s.get("modified", 0) add = s.get("added", 0) dlt = s.get("deleted", 0) - rmod = s.get("remote_modified", 0) + rmod = s.get("remote_modified", 0) + s.get("remote_deleted", 0) conf = s.get("conflict", 0) ro = s.get("remote_only", 0) if not any([mod, add, dlt, rmod, conf, ro]): @@ -437,7 +442,7 @@ def _diff_one_liner(result: dict) -> str: if mod: parts.append(f"[yellow]{mod} to push[/yellow]") if dlt: - parts.append(f"[red]{dlt} to delete[/red]") + parts.append(f"[red]{dlt} to delete (--force)[/red]") if rmod: parts.append(f"[cyan]{rmod} to pull[/cyan]") if conf: @@ -450,15 +455,16 @@ def _diff_one_liner(result: dict) -> str: def _push_one_liner(result: dict) -> str: """One-line summary of a single push result.""" status = result.get("status", "") + held = push_skips_one_liner(result) if status == "no_changes": - return "[green]nothing to push[/green]" + return f"[green]nothing to push[/green]{held}" if status == "dry_run": s = result.get("summary", {}) - return f"would: +{s.get('added', 0)} ~{s.get('modified', 0)} -{s.get('deleted', 0)}" + return f"would: +{s.get('added', 0)} ~{s.get('modified', 0)} -{s.get('deleted', 0)}{held}" c = result.get("created", 0) u = result.get("updated", 0) d = result.get("deleted", 0) - return f"+{c} created, ~{u} updated, -{d} deleted" + return f"+{c} created, ~{u} updated, -{d} deleted{held}" def _format_all_results( @@ -897,7 +903,7 @@ def sync_diff( } local_changes = [c for c in changes if c["change_type"] in ("added", "modified", "deleted")] - remote_changes = [c for c in changes if c["change_type"] == "remote_modified"] + remote_changes = [c for c in changes if c["change_type"] in REMOTE_CHANGE_LABELS] conflict_changes = [c for c in changes if c["change_type"] == "conflict"] # Local changes (what push would do) @@ -913,7 +919,7 @@ def sync_diff( formatter.console.print(f" {detail}") formatter.console.print( f"\n{summary['added']} to create, {summary['modified']} to update, " - f"{summary['deleted']} to delete" + f"{summary['deleted']} to delete (push --force)" ) # Remote changes (need pull) @@ -923,7 +929,8 @@ def sync_diff( formatter.console.print("[bold]Remote changes (run 'sync pull' to fetch):[/bold]") for change in remote_changes: label = _change_label(change) - formatter.console.print(f" [cyan]~ REMOTE MODIFIED {label}[/cyan]") + kind = REMOTE_CHANGE_LABELS[change["change_type"]] + formatter.console.print(f" [cyan]{kind} {label}[/cyan]") for detail in change.get("details", []): formatter.console.print(f" {detail}") @@ -982,7 +989,10 @@ def sync_push( force: bool = typer.Option( False, "--force", - help="Allow deletion of remote configs that were removed locally", + help=( + "Delete remote configs and rows whose local files were removed. " + "Without it push skips those deletions and lists them." + ), ), allow_plaintext: bool = typer.Option( False, @@ -1083,9 +1093,7 @@ def sync_push( if status == "no_changes": formatter.console.print("[green]No changes to push.[/green]") - skipped_reason = result.get("skipped_reason") - if skipped_reason: - formatter.console.print(f" [yellow]{skipped_reason}[/yellow]") + print_push_skips(formatter, result) _format_never_fetched(formatter, result.get("never_fetched", [])) _format_orphaned(formatter, result.get("orphaned", [])) return @@ -1100,6 +1108,7 @@ def sync_push( f"\nWould create {summary['added']}, update {summary['modified']}, " f"delete {summary['deleted']}" ) + print_push_skips(formatter, result) _format_never_fetched(formatter, result.get("never_fetched", [])) _format_orphaned(formatter, result.get("orphaned", [])) return @@ -1109,6 +1118,7 @@ def sync_push( f"{result['updated']} updated, " f"{result['deleted']} deleted" ) + print_push_skips(formatter, result) _format_never_fetched(formatter, result.get("never_fetched", [])) _format_orphaned(formatter, result.get("orphaned", [])) for change in result.get("pushed_details", []): diff --git a/src/keboola_agent_cli/services/_sync_clone.py b/src/keboola_agent_cli/services/_sync_clone.py index eab707ef5..cfb7184ec 100644 --- a/src/keboola_agent_cli/services/_sync_clone.py +++ b/src/keboola_agent_cli/services/_sync_clone.py @@ -20,6 +20,7 @@ apply_instance_rename, apply_variable_values, copy_reference_tree, + drop_source_pull_marks, repoint_default_branch_configs, repoint_manifest_project, ) @@ -163,6 +164,7 @@ def clone_project( source_default_branch_id=source_default_branch_id, new_branch_id=push_branch_id or 0, ) + drop_source_pull_marks(manifest) bucket_rewrites = apply_bucket_map(target_path, manifest, bucket_map) variable_overrides = apply_variable_values(target_path, manifest, variable_values) renamed_instances = apply_instance_rename(target_path, manifest, instance_rename) diff --git a/src/keboola_agent_cli/services/_sync_push_ops.py b/src/keboola_agent_cli/services/_sync_push_ops.py index 03c7fc706..90632b2c0 100644 --- a/src/keboola_agent_cli/services/_sync_push_ops.py +++ b/src/keboola_agent_cli/services/_sync_push_ops.py @@ -1,6 +1,7 @@ """Per-change push CRUD operations (create/update/delete config + rows). -Extracted from sync_service.py. ``push()`` calls :func:`push_create`, +Extracted from sync_service.py. ``push()`` first splits the diff with +:func:`plan_push`, then calls :func:`push_create`, :func:`push_update`, and :func:`push_row_change`; the row dispatcher fans out to the create/update/delete row helpers. Each reads a local ``_config.yml``, encrypts ``#``-prefixed secrets (fail-closed), POSTs/PUTs/DELETEs, then writes @@ -13,6 +14,7 @@ import copy import logging +from dataclasses import dataclass from pathlib import Path from typing import TYPE_CHECKING, Any @@ -32,6 +34,65 @@ logger = logging.getLogger(__name__) +# The local-side change types push applies. ``remote_modified``, ``conflict`` +# and ``remote_deleted`` need a ``sync pull`` first. +PUSHABLE_CHANGE_TYPES = frozenset({"added", "modified", "deleted"}) +SKIPPED_REASON = "Remote changes detected. Run 'sync pull' first." +SKIPPED_DELETIONS_REASON = ( + "Push deletes remote configs and rows only with --force. " + "Run 'kbagent sync push --force' to delete these." +) + + +@dataclass +class PushPlan: + """The changes one ``sync push`` applies, and the ones it holds back. + + Attributes: + changes: Changes push applies. + skipped: Remote-side changes; they need ``sync pull`` first. + skipped_deletions: ``deleted`` changes held back because ``--force`` + was not given (issue #792 G). + """ + + changes: list[dict[str, Any]] + skipped: list[dict[str, Any]] + skipped_deletions: list[dict[str, Any]] + + @property + def deletions(self) -> int: + """Number of ``deleted`` changes push applies.""" + return sum(1 for change in self.changes if change["change_type"] == "deleted") + + def report(self) -> dict[str, Any]: + """Result keys that list what push does not apply.""" + report: dict[str, Any] = {} + if self.skipped: + report["skipped"] = len(self.skipped) + report["skipped_reason"] = SKIPPED_REASON + if self.skipped_deletions: + report["skipped_deletions"] = self.skipped_deletions + report["skipped_deletions_reason"] = SKIPPED_DELETIONS_REASON + return report + + +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. + + Without *force* push deletes nothing on the remote, configs and rows + alike, as the ``--force`` help says (issue #792 G). The held-back + deletions are listed in the result, also for ``--dry-run``. + """ + plan = PushPlan(changes=[], skipped=[], skipped_deletions=[]) + for change in all_changes: + if change["change_type"] not in PUSHABLE_CHANGE_TYPES: + plan.skipped.append(change) + elif change["change_type"] == "deleted" and not force: + plan.skipped_deletions.append(change) + else: + plan.changes.append(change) + return plan + def guard_script_shape( component_id: str, diff --git a/src/keboola_agent_cli/services/sync_service.py b/src/keboola_agent_cli/services/sync_service.py index eda386769..d08e5923c 100644 --- a/src/keboola_agent_cli/services/sync_service.py +++ b/src/keboola_agent_cli/services/sync_service.py @@ -106,7 +106,7 @@ type_needs_rewrite, ) from ._sync_models import CreatedConfig, LocalConfigHashes -from ._sync_push_ops import push_create, push_row_change, push_update +from ._sync_push_ops import plan_push, push_create, push_row_change, push_update from ._sync_stale import ( apply_stale_sweep, find_stale_entries, @@ -1437,6 +1437,7 @@ def diff( tracked_keys, base_hashes or None, local_override_hashes or None, + scope.target_tracked_keys, ) # Row-level diff: walk manifest rows, load local YAML, feed into @@ -1499,6 +1500,12 @@ def diff( remote_rows, tracked_row_keys, row_base_hashes or None, + scope.target_tracked_row_keys, + { + f"{c.component_id}/{c.config_id}" + for c in changeset + if c.change_type == "remote_deleted" + }, ) changeset.extend(row_changeset) @@ -1507,6 +1514,7 @@ def diff( remote_modified = [c for c in changeset if c.change_type == "remote_modified"] conflicts = [c for c in changeset if c.change_type == "conflict"] deleted = [c for c in changeset if c.change_type == "deleted"] + remote_deleted = [c for c in changeset if c.change_type == "remote_deleted"] # Detect remote-only configs (new on server, not yet pulled). local_keys = { @@ -1535,11 +1543,13 @@ def diff( "remote_modified": len(remote_modified), "conflict": len(conflicts), "deleted": len(deleted), + "remote_deleted": len(remote_deleted), "unchanged": len(local_configs) - len(added) - len(modified) - len(remote_modified) - - len(conflicts), + - len(conflicts) + - sum(1 for c in remote_deleted if not c.is_row), "remote_only": len(remote_only), "never_fetched": len(never_fetched), "orphaned": len(orphaned), @@ -1569,7 +1579,9 @@ def push( alias: Project alias from config store. project_root: Root directory of the sync working tree. dry_run: If True, compute changes but don't execute them. - force: If True, allow deletions without extra confirmation. + force: If True, delete remote configs and rows whose local files + were removed. Without it push deletes nothing and lists the + deletions under ``skipped_deletions`` (issue #792 G). allow_plaintext_fallback: If True, allow push when secret encryption fails (DANGEROUS). branch_override: If set, target this dev-branch ID for the push. @@ -1594,13 +1606,10 @@ def push( # branch's tree (issue #649) -- reported, never pushed. orphaned = diff_result.get("orphaned", []) - # Only push local-side changes (added, modified, deleted). - # Skip remote_modified (need pull) and conflict (need resolution). - pushable_types = {"added", "modified", "deleted"} - changes = [c for c in all_changes if c["change_type"] in pushable_types] - - # Warn about skipped changes - skipped = [c for c in all_changes if c["change_type"] not in pushable_types] + # Push applies local-side changes only, and deletes only with --force. + # What it holds back is listed in every result (issue #792 G, H). + plan = plan_push(all_changes, force=force) + changes = plan.changes if not changes: result: dict[str, Any] = { @@ -1609,10 +1618,8 @@ def push( "updated": 0, "deleted": 0, "errors": [], + **plan.report(), } - if skipped: - result["skipped"] = len(skipped) - result["skipped_reason"] = "Remote changes detected. Run 'sync pull' first." if never_fetched: result["never_fetched"] = never_fetched if orphaned: @@ -1623,7 +1630,8 @@ def push( dry_result: dict[str, Any] = { "status": "dry_run", "changes": changes, - "summary": diff_result["summary"], + "summary": {**diff_result["summary"], "deleted": plan.deletions}, + **plan.report(), } if never_fetched: dry_result["never_fetched"] = never_fetched @@ -1922,6 +1930,7 @@ def push( "deleted": deleted, "errors": errors, "pushed_details": pushed_details, + **plan.report(), } if warnings: result_data["warnings"] = warnings diff --git a/src/keboola_agent_cli/sync/branch_scope.py b/src/keboola_agent_cli/sync/branch_scope.py index 20291f455..6bc5e9bd4 100644 --- a/src/keboola_agent_cli/sync/branch_scope.py +++ b/src/keboola_agent_cli/sync/branch_scope.py @@ -112,18 +112,42 @@ class TreeScope: excluded before any branch reasoning happens. orphaned: Report records for entries that belong to another tree. claims: ``config_key`` -> the claims held on it by *any* tree. + on_target: The ``in_tree`` entries fetched from the push target branch + itself (they carry a ``pull_hash``). Not the production entries a + promote push reads from ``main/``, a hand-authored placeholder, or + a ``sync clone`` copy: none of them was ever on the target. """ in_tree: list[ManifestConfiguration] never_fetched: list[dict[str, str]] orphaned: list[dict[str, Any]] claims: dict[str, list[Claim]] + on_target: list[ManifestConfiguration] @property def tracked_keys(self) -> set[str]: """Keys diff/push treat as tracked -- source tree only.""" return {config_key(cfg.component_id, cfg.id) for cfg in self.in_tree} + @property + def target_tracked_keys(self) -> set[str]: + """Config keys tracked on the target branch (issue #792 H).""" + return {config_key(cfg.component_id, cfg.id) for cfg in self.on_target} + + @property + def target_tracked_row_keys(self) -> set[str]: + """Keys of the rows fetched from the target branch (issue #792 H). + + A row carries its own ``pull_hash``: a row whose create failed under a + freshly created parent has none and stays ``added``. + """ + return { + f"{config_key(cfg.component_id, cfg.id)}/rows/{row.id}" + for cfg in self.on_target + for row in cfg.rows + if row.metadata and row.metadata.get("pull_hash") + } + @property def never_fetched_keys(self) -> set[str]: """Keys of entries that were never materialized on disk.""" @@ -152,9 +176,9 @@ def scope_manifest( (``ALWAYS_IGNORED_COMPONENTS`` plus the manifest's ``ignoredComponents``). Entries for these components are dropped from EVERY partition -- see below. - target_branch_id: Branch the API writes go to. Only consulted on the - promote path (the target's own tree is not *source_branch_path*), - see :func:`_promoted_paths`. + target_branch_id: Branch the API writes go to. Selects + ``on_target``, and on the promote path (the target's own tree is + not *source_branch_path*) :func:`_promoted_paths`. Returns: A :class:`TreeScope`. @@ -163,6 +187,8 @@ def scope_manifest( never_fetched: list[dict[str, str]] = [] orphaned: list[dict[str, Any]] = [] claims: dict[str, list[Claim]] = {} + on_target: list[ManifestConfiguration] = [] + target_tree = branch_tree_path(manifest, target_branch_id) promoted = _promoted_paths(manifest, project_root, source_branch_path, target_branch_id) for cfg in manifest.configurations: @@ -210,10 +236,14 @@ def scope_manifest( if promote_key in promoted: if cfg.branch_id == target_branch_id: in_tree.append(cfg) + if cfg.metadata.get("pull_hash"): + on_target.append(cfg) continue if tree_path == source_branch_path: in_tree.append(cfg) + if tree_path == target_tree and cfg.metadata.get("pull_hash"): + on_target.append(cfg) continue orphaned.append( @@ -225,6 +255,7 @@ def scope_manifest( never_fetched=never_fetched, orphaned=orphaned, claims=claims, + on_target=on_target, ) diff --git a/src/keboola_agent_cli/sync/clone.py b/src/keboola_agent_cli/sync/clone.py index aebce6168..57fed0bc4 100644 --- a/src/keboola_agent_cli/sync/clone.py +++ b/src/keboola_agent_cli/sync/clone.py @@ -106,6 +106,22 @@ def repoint_default_branch_configs( cfg.branch_id = new_branch_id +def drop_source_pull_marks(manifest: Any) -> None: + """Drop the ``pull_hash`` the copied entries carry from the SOURCE project. + + ``pull_hash`` marks an entry as fetched from the remote it now points at. + After the re-point it would claim that for the target, where none of these + configs exists, and the diff would report each one as deleted on the + target instead of new (``remote_deleted``, issue #792 H). The rows carry + their own. Push stamps a fresh ``pull_hash`` when it creates each one. + """ + for cfg in manifest.configurations: + cfg.metadata.pop("pull_hash", None) + for row in cfg.rows: + if row.metadata: + row.metadata.pop("pull_hash", None) + + def _default_branch_dir(manifest: Any) -> str: """On-disk tree for an unregistered branch id -- the default branch's dir. diff --git a/src/keboola_agent_cli/sync/diff_engine.py b/src/keboola_agent_cli/sync/diff_engine.py index 6ddf051f5..8f462e451 100644 --- a/src/keboola_agent_cli/sync/diff_engine.py +++ b/src/keboola_agent_cli/sync/diff_engine.py @@ -31,6 +31,8 @@ class ConfigChange: - ``"remote_modified"`` -- remote changed, local unchanged (run pull) - ``"conflict"`` -- both sides changed since last pull - ``"deleted"`` -- local file removed, wants to delete from remote + - ``"remote_deleted"`` -- tracked on the target branch, deleted on the + remote since the last pull (run pull; push never re-creates it) Row changes set ``is_row=True`` and carry ``parent_config_id`` so ``sync push`` can dispatch them to the row-specific client methods @@ -311,6 +313,7 @@ def compute_changeset( tracked_keys: set[str] | None = None, base_hashes: dict[str, str] | None = None, local_override_hashes: dict[str, str] | None = None, + target_tracked_keys: set[str] | None = None, ) -> list[ConfigChange]: """3-way diff: compare local vs base (pull_hash) vs remote. @@ -335,12 +338,17 @@ def compute_changeset( -> hash to use for local side instead of computing from data. Used when local files haven't changed since pull to avoid lossy code-merge roundtrip. + target_tracked_keys: Optional set of keys the manifest tracks on the + target branch itself. Such a key missing from the remote was + deleted there since the last pull: it is ``"remote_deleted"``, + never ``"added"`` (issue #792 H -- push re-created it silently). Returns: List of :class:`ConfigChange` objects. """ changes: list[ConfigChange] = [] seen_remote_keys: set[str] = set() + on_target = target_tracked_keys or set() for entry in local_configs: component_id: str = entry["component_id"] @@ -351,11 +359,11 @@ def compute_changeset( remote_key = f"{component_id}/{config_id}" if config_id else "" - # New config (no id yet, or not in remote) + # New config (no id yet, or not in remote), or one deleted remotely if not config_id or remote_key not in remote_configs: changes.append( ConfigChange( - change_type="added", + change_type="remote_deleted" if remote_key in on_target else "added", component_id=component_id, config_id=config_id, config_name=config_name, @@ -448,6 +456,8 @@ def compute_row_changeset( remote_rows: dict[str, dict[str, Any]], tracked_row_keys: set[str] | None = None, base_hashes: dict[str, str] | None = None, + target_tracked_keys: set[str] | None = None, + remote_deleted_parents: set[str] | None = None, ) -> list[ConfigChange]: """3-way diff for configuration rows, parallel to :func:`compute_changeset`. @@ -467,12 +477,21 @@ def compute_row_changeset( Only manifest-tracked remote rows can be flagged as ``"deleted"``. base_hashes: Optional dict of row_key -> normalized config hash at last pull time. Enables 3-way diff (same semantics as parent configs). + target_tracked_keys: Optional set of row keys the manifest tracks on + the target branch. Same ``"remote_deleted"`` rule as + :func:`compute_changeset`. + remote_deleted_parents: Optional set of parent config keys + (``"{component_id}/{config_id}"``) classified ``"remote_deleted"``. + Every local row under one is ``"remote_deleted"`` too, a new row + included: push cannot create a row under a config that is gone. Returns: List of :class:`ConfigChange` objects, each with ``is_row=True``. """ changes: list[ConfigChange] = [] seen_remote_keys: set[str] = set() + on_target = target_tracked_keys or set() + gone_parents = remote_deleted_parents or set() for entry in local_rows: component_id: str = entry["component_id"] @@ -488,11 +507,14 @@ def compute_row_changeset( else "" ) - # New row (no id yet, or not in remote) - if not row_id or remote_key not in remote_rows: + # New row (no id yet, or not in remote), or one deleted remotely -- + # on its own or with its parent config + parent_gone = f"{component_id}/{parent_config_id}" in gone_parents + if parent_gone or not row_id or remote_key not in remote_rows: + deleted_remotely = parent_gone or remote_key in on_target changes.append( ConfigChange( - change_type="added", + change_type="remote_deleted" if deleted_remotely else "added", component_id=component_id, config_id=row_id, config_name=row_name, diff --git a/tests/test_generate_promotion_pipeline.py b/tests/test_generate_promotion_pipeline.py index bef0838bd..4d1dad5ac 100644 --- a/tests/test_generate_promotion_pipeline.py +++ b/tests/test_generate_promotion_pipeline.py @@ -81,6 +81,23 @@ def test_every_job_has_its_own_prod_environment(self) -> None: parsed = _parse_yaml(gen_push(pipelines, main_branch="main")) assert all(job["environment"] == "prod" for job in parsed["jobs"].values()) + def test_push_and_validate_pass_force_so_source_deletions_propagate(self) -> None: + """Without --force `sync push` deletes nothing (#792 G): both steps pass it.""" + pipelines = [_pipeline()] + push_runs = [ + step["run"] + for job in _parse_yaml(gen_push(pipelines, main_branch="main"))["jobs"].values() + for step in job["steps"] + if "sync push" in step.get("run", "") + ] + validate_runs = [ + step["run"] + for step in _parse_yaml(gen_validate(pipelines))["jobs"]["validate"]["steps"] + if "sync push" in step.get("run", "") + ] + assert push_runs and all("sync push --force " in run for run in push_runs) + assert validate_runs and all("--dry-run --force " in run for run in validate_runs) + def test_jobs_have_no_needs_dependency_so_one_failure_does_not_block_others(self) -> None: pipelines = [_pipeline("SALESFORCE", "salesforce"), _pipeline("GA4", "ga4")] parsed = _parse_yaml(gen_push(pipelines, main_branch="main")) diff --git a/tests/test_sync_branch_scope.py b/tests/test_sync_branch_scope.py new file mode 100644 index 000000000..5103c81af --- /dev/null +++ b/tests/test_sync_branch_scope.py @@ -0,0 +1,61 @@ +"""Tests for sync/branch_scope.py: which entries count as fetched from the target (#792 H).""" + +from __future__ import annotations + +from pathlib import Path + +from keboola_agent_cli.constants import CONFIG_FILENAME +from keboola_agent_cli.sync.branch_scope import scope_manifest +from keboola_agent_cli.sync.manifest import ( + Manifest, + ManifestBranch, + ManifestConfigRow, + ManifestConfiguration, + ManifestNaming, + ManifestProject, +) + +PROD, DEV = 100, 200 + + +def _manifest(root: Path) -> Manifest: + entries = [ + ManifestConfiguration( + branchId=PROD, + componentId="c", + id="pulled", + path="c/pulled", + metadata={"pull_hash": "h"}, + rows=[ + ManifestConfigRow(id="row-pulled", path="rows/a", metadata={"pull_hash": "h"}), + ManifestConfigRow(id="row-failed", path="rows/b", metadata={}), + ], + ), + ManifestConfiguration(branchId=PROD, componentId="c", id="placeholder", path="c/new"), + ] + for entry in entries: + config_dir = root / "main" / entry.path + config_dir.mkdir(parents=True) + (config_dir / CONFIG_FILENAME).write_text("name: x\n") + return Manifest( + project=ManifestProject(id=1, apiHost="connection.keboola.com"), + naming=ManifestNaming(), + branches=[ManifestBranch(id=PROD, path="main"), ManifestBranch(id=DEV, path="dev")], + configurations=entries, + ) + + +def test_on_target_needs_a_pull_hash(tmp_path: Path) -> None: + """A placeholder entry and a row whose create failed were never on the target.""" + scope = scope_manifest(_manifest(tmp_path), tmp_path, "main", set(), target_branch_id=PROD) + + assert scope.target_tracked_keys == {"c/pulled"} + assert scope.target_tracked_row_keys == {"c/pulled/rows/row-pulled"} + + +def test_promote_push_source_entries_are_not_on_the_target(tmp_path: Path) -> None: + """``main/`` read for a dev branch with no tree of its own: nothing is on the target yet.""" + scope = scope_manifest(_manifest(tmp_path), tmp_path, "main", set(), target_branch_id=DEV) + + assert {cfg.id for cfg in scope.in_tree} == {"pulled", "placeholder"} + assert scope.target_tracked_keys == set() diff --git a/tests/test_sync_cli.py b/tests/test_sync_cli.py index 2bfbaa773..2cd6ccb35 100644 --- a/tests/test_sync_cli.py +++ b/tests/test_sync_cli.py @@ -13,7 +13,12 @@ from typer.testing import CliRunner from keboola_agent_cli.cli import app -from keboola_agent_cli.commands.sync import _format_pull_result, _pull_one_liner +from keboola_agent_cli.commands.sync import ( + _diff_one_liner, + _format_pull_result, + _pull_one_liner, + _push_one_liner, +) from keboola_agent_cli.config_store import ConfigStore from keboola_agent_cli.constants import SYNC_ORPHAN_PREVIEW_LIMIT from keboola_agent_cli.errors import ConfigError, KeboolaApiError, SyncConflictError @@ -447,6 +452,23 @@ def test_pull_one_liner_counts_ignored(self) -> None: assert "2 ignored" in line assert "up to date" not in line + def test_push_one_liner_names_what_push_held_back(self) -> None: + """--all-projects push line: remote changes to pull and deletions that need --force (#792).""" + line = _push_one_liner( + {"status": "no_changes", "skipped": 2, "skipped_deletions": [{"config_id": "c"}]} + ) + + assert "nothing to push" in line + assert "2 to pull first" in line + assert "1 deletion(s) need --force" in line + + def test_diff_one_liner_counts_remote_deletions(self) -> None: + """A config deleted on the remote is a change to pull, not "in sync" (#792 H).""" + line = _diff_one_liner({"summary": {"remote_deleted": 1}}) + + assert "1 to pull" in line + assert "in sync" not in line + def test_pull_one_liner_flags_folder_lookup_failed(self) -> None: """The --all-projects one-liner signals a degraded folder lookup, even when nothing else changed (the default path never calls the fuller @@ -1245,6 +1267,51 @@ def test_sync_push_no_changes_human(self, tmp_path: Path) -> None: assert result.exit_code == 0, f"Exit code {result.exit_code}: {result.output}" assert "No changes to push" in result.output + def test_sync_push_lists_deletions_held_back_without_force_human(self, tmp_path: Path) -> None: + """Human mode names each deletion push skipped without --force, and the fix (#792 G).""" + config_dir = tmp_path / "config" + config_dir.mkdir() + store = _setup_config(config_dir, {"prod": {"token": TEST_TOKEN}}) + + mock_sync = _make_sync_service_mock() + mock_sync.push.return_value = { + "status": "no_changes", + "created": 0, + "updated": 0, + "deleted": 0, + "errors": [], + "skipped_deletions": [ + { + "change_type": "deleted", + "component_id": "keboola.ex-http", + "config_id": "cfg-001", + "config_name": "My HTTP Extractor", + "path": "", + "details": [], + } + ], + "skipped_deletions_reason": "Push deletes remote configs and rows only with --force.", + } + + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockProjService, + patch("keboola_agent_cli.cli.SyncService") as MockSyncService, + ): + MockStore.return_value = store + MockProjService.return_value = ProjectService(config_store=store) + MockSyncService.return_value = mock_sync + result = runner.invoke( + app, ["sync", "push", "--project", "prod", "--directory", str(tmp_path)] + ) + + assert result.exit_code == 0, f"Exit code {result.exit_code}: {result.output}" + # A notice, not a result: stderr only (#791 proposal, rule O6). + assert "1 deletion(s) not applied" in result.stderr + assert "only with --force" in result.stderr + assert "keboola.ex-http/cfg-001" in result.stderr + assert "deletion(s) not applied" not in result.stdout + def test_sync_push_with_row_changes_json(self, tmp_path: Path) -> None: """JSON output reflects row-level push results (P0-1). diff --git a/tests/test_sync_diff_engine.py b/tests/test_sync_diff_engine.py index 3bd85ddc1..5291e9679 100644 --- a/tests/test_sync_diff_engine.py +++ b/tests/test_sync_diff_engine.py @@ -13,6 +13,7 @@ from keboola_agent_cli.sync.diff_engine import ( ConfigChange, compute_changeset, + compute_row_changeset, config_hash, deep_diff, normalize_for_comparison, @@ -452,6 +453,60 @@ def test_mixed_changeset(self) -> None: # =================================================================== +def _local_config() -> dict[str, Any]: + return { + "component_id": "keboola.ex-http", + "config_id": "cfg-001", + "config_name": "My Extractor", + "path": "extractor/keboola.ex-http/my-extractor", + "data": {"name": "My Extractor", "parameters": {}}, + } + + +class TestRemoteDeleted: + """``remote_deleted``: tracked on the target branch, gone on the remote (#792 H).""" + + def test_config_tracked_on_target_and_missing_remotely(self) -> None: + changes = compute_changeset( + [_local_config()], {}, target_tracked_keys={"keboola.ex-http/cfg-001"} + ) + assert [c.change_type for c in changes] == ["remote_deleted"] + + def test_config_not_tracked_on_target_is_added(self) -> None: + changes = compute_changeset([_local_config()], {}, target_tracked_keys=set()) + assert [c.change_type for c in changes] == ["added"] + + def test_rows(self) -> None: + def row(parent: str, row_id: str, name: str) -> dict[str, Any]: + return { + "component_id": "keboola.ex-http", + "parent_config_id": parent, + "row_id": row_id, + "row_name": name, + "path": f"rows/{name}", + "data": {"name": name}, + } + + changes = compute_row_changeset( + [ + row("cfg-001", "row-1", "under-gone-parent"), + row("cfg-001", "", "new-under-gone-parent"), + row("cfg-002", "row-2", "fetched-then-gone"), + row("cfg-002", "row-3", "never-fetched"), + ], + {}, + target_tracked_keys={"keboola.ex-http/cfg-002/rows/row-2"}, + remote_deleted_parents={"keboola.ex-http/cfg-001"}, + ) + + assert [(c.config_name, c.change_type) for c in changes] == [ + ("under-gone-parent", "remote_deleted"), + ("new-under-gone-parent", "remote_deleted"), + ("fetched-then-gone", "remote_deleted"), + ("never-fetched", "added"), + ] + + 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 9149af3eb..816d1841a 100644 --- a/tests/test_sync_formal_counterexamples.py +++ b/tests/test_sync_formal_counterexamples.py @@ -617,7 +617,7 @@ def test_e_adopted_scaffold_push_does_not_overwrite_remote_edit(tmp_path: Path) # =========================================================================== # F -- a push aborted by ENCRYPTION_FAILED leaves the manifest unsaved, so a -# change it already applied (a resurrect CREATE) is re-applied by the +# change it already applied (a promote CREATE) is re-applied by the # retry, duplicating it. # =========================================================================== @@ -638,8 +638,11 @@ def test_e_adopted_scaffold_push_does_not_overwrite_remote_edit(tmp_path: Path) ) 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 resurrect CREATE) that the aborted push had - already sent to the remote before it failed. + 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.) Issue #792 finding F. """ @@ -649,8 +652,6 @@ def test_f_aborted_push_does_not_duplicate_already_created_config(tmp_path: Path w.init() w.pull() - # Remote deletes cfg-1 (resurrect precondition): local file untouched. - w.api.remote[PROD].pop("cfg-1") # Local edit to cfg-2's secret triggers the encryption failure below. contacts_file = w.config_dir("contacts") / CONFIG_FILENAME data = yaml.safe_load(contacts_file.read_text()) @@ -665,7 +666,7 @@ def flaky_encrypt(project_id: int, component_id: str, data: dict[str, str]) -> d w.api.encrypt_values = flaky_encrypt with pytest.raises(KeboolaApiError) as exc_info: - w.push() + w.push(branch=DEV) assert exc_info.value.error_code == ErrorCode.ENCRYPTION_FAILED creates_before_retry = [line for line in w.api.log if line.startswith("CREATE")] @@ -680,41 +681,25 @@ def flaky_encrypt(project_id: int, component_id: str, data: dict[str, str]) -> d w.api.encrypt_values = lambda project_id, component_id, data: { k: f"KBC::Encrypted=={v}" for k, v in data.items() } - w.push() + w.push(branch=DEV) creates_after_retry = [line for line in w.api.log if line.startswith("CREATE")] - # Safe expectation: the resurrect CREATE for "Orders" happened exactly + # 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 resurrect CREATE: " + "retrying the push after the encryption fix duplicated the already-applied promote CREATE: " f"log={w.api.log}" ) # =========================================================================== -# G -- `sync push` deletes a remote config with no `--force`, contradicting -# the CLI's own help text ("--force: allow deletion of remote configs -# removed locally"). Product decision (soft-delete to trash since -# 0.89.0, so it is recoverable) -- xfail per the task's own note. +# G -- `sync push` deleted a remote config with no `--force`, contradicting +# the CLI's own help text. Fixed: push deletes nothing remote without +# `--force` and lists the held-back deletions (`skipped_deletions`). # =========================================================================== -@pytest.mark.xfail( - strict=True, - reason=( - "#792 G: SyncService.push() accepts `force` but never reads it in " - "its body (grep sync_service.py:1574-1966) -- only pull()'s conflict " - "guard does. The CLI help text for `sync push --force` " - "(commands/sync.py:982-986) promises 'Allow deletion of remote " - "configs that were removed locally', implying a plain push should " - "NOT delete, but push deletes a remote config the instant its local " - "directory is missing, force=True or not. This is a PRODUCT " - "DECISION (the delete is soft, into the Storage trash, since " - "0.89.0, so it is recoverable via `sync restore`) -- kept xfail per " - "issue #792 rather than resolved either way here." - ), -) def test_g_push_without_force_does_not_delete_remote_config( tmp_config_dir: Path, tmp_path: Path ) -> None: @@ -738,28 +723,18 @@ def test_g_push_without_force_does_not_delete_remote_config( client.delete_config.assert_not_called() assert push_result.get("deleted", 0) == 0 + assert [(c["component_id"], c["config_id"]) for c in push_result["skipped_deletions"]] == [ + (cfg.component_id, cfg.id) + ] # =========================================================================== -# H -- a config deleted remotely by another actor is silently re-created -# (no warning) by the next push, even with no local change. +# H -- a config deleted remotely by another actor was silently re-created by +# the next push, even with no local change. Fixed: the diff classifies it +# `remote_deleted`, which push never applies and reports as skipped. # =========================================================================== -@pytest.mark.xfail( - strict=True, - reason=( - "#792 H: compute_changeset routes ANY local entry whose remote_key " - "is absent into 'added' (diff_engine.py:355), regardless of whether " - "the id was previously tracked. A tracked config deleted/trashed " - "remotely by another actor -- with NO local change at all -- is " - "silently recreated under a brand-new id on the next push, with no " - "warning distinguishing it from a genuinely new config. Confirmed " - "via spec S2, Lean F2 and TLA I11b (three independent hits) and " - "replayed (scratchpad/repro/test_lean_refutations.py::test_R6, " - "scratchpad/replay/r_misc.py)." - ), -) def test_h_push_does_not_silently_resurrect_deleted_config(tmp_path: Path) -> None: """Invariant: push must not silently POST a fresh create for a config another actor deleted remotely when the local user made no change to it @@ -781,6 +756,58 @@ def test_h_push_does_not_silently_resurrect_deleted_config(tmp_path: Path) -> No "push silently recreated a remotely-deleted, locally-untouched config" ) assert "cfg-1" not in w.api.remote[PROD] + assert result["skipped"] == 1 + assert changes(w.diff()) == [("remote_deleted", "cfg-1")] + + +def test_h_locally_edited_remote_deleted_config_is_not_recreated(tmp_path: Path) -> None: + """A local edit does not turn a config deleted on the remote into a new one. + + Issue #792 finding H (the local-edit half; C keeps the edited dir on pull). + """ + w = World(tmp_path) + w.api.put(PROD, "cfg-1", "Orders", "a") + w.init() + w.pull() + cfg_file = w.config_dir("orders") / CONFIG_FILENAME + cfg_file.write_text(cfg_file.read_text().replace("value: a", "value: b")) + w.api.remote[PROD].pop("cfg-1") + + result = w.push() + + assert result.get("created", 0) == 0 + assert "cfg-1" not in w.api.remote[PROD] + assert changes(w.diff()) == [("remote_deleted", "cfg-1")] + + +def test_g_dry_run_counts_only_the_deletions_push_applies(tmp_path: Path) -> None: + """``--dry-run`` shows what push does: without --force a deletion is listed, + not counted, and push leaves the remote config alone until --force. + + Issue #792 finding G. + """ + w = World(tmp_path) + w.api.put(PROD, "cfg-1", "Orders", "a") + w.api.put(PROD, "cfg-2", "Contacts", "b") + w.init() + w.pull() + cfg_file = w.config_dir("orders") / CONFIG_FILENAME + cfg_file.write_text(cfg_file.read_text().replace("value: a", "value: b")) + shutil.rmtree(w.config_dir("contacts")) + + dry = w.push(dry_run=True) + assert (dry["summary"]["modified"], dry["summary"]["deleted"]) == (1, 0) + assert [c["config_id"] for c in dry["skipped_deletions"]] == ["cfg-2"] + assert w.push(dry_run=True, force=True)["summary"]["deleted"] == 1 + + pushed = w.push() + assert (pushed["updated"], pushed["deleted"]) == (1, 0) + assert [c["config_id"] for c in pushed["skipped_deletions"]] == ["cfg-2"] + assert "cfg-2" in w.api.remote[PROD] + + forced = w.push(force=True) + assert forced["deleted"] == 1 + assert "cfg-2" not in w.api.remote[PROD] # =========================================================================== diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index 625ce59e2..bb009b7b3 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -1185,7 +1185,7 @@ def test_push_row_update_calls_update_config_row( def test_push_row_delete_calls_delete_config_row( self, tmp_config_dir: Path, tmp_path: Path ) -> None: - """Removing a row YAML file triggers client.delete_config_row + manifest pruning.""" + """With --force, removing a row YAML file triggers client.delete_config_row + manifest pruning.""" project_root = tmp_path / "project" project_root.mkdir() store, _ = self._init_and_pull(tmp_config_dir, project_root) @@ -1205,7 +1205,7 @@ def test_push_row_delete_calls_delete_config_row( client_factory=lambda url, token: push_client, ) - result = push_svc.push(alias="prod", project_root=project_root) + result = push_svc.push(alias="prod", project_root=project_root, force=True) assert result["status"] == "pushed" assert result["deleted"] == 1 @@ -1220,6 +1220,79 @@ def test_push_row_delete_calls_delete_config_row( parent = next(c for c in manifest.configurations if c.id == "cfg-001") assert all(r.id != "row-001" for r in parent.rows) + def test_push_row_delete_without_force_is_skipped_and_listed( + self, tmp_config_dir: Path, tmp_path: Path + ) -> None: + """Without --force a removed row YAML deletes nothing; push lists it (#792 G, D5).""" + project_root = tmp_path / "project" + project_root.mkdir() + store, _ = self._init_and_pull(tmp_config_dir, project_root) + (row_file,) = ( + f + for f in project_root.rglob(CONFIG_FILENAME) + if "rows" in f.relative_to(project_root).parts + ) + row_file.unlink() + + push_client = _make_sync_mock_client(components_response=SAMPLE_COMPONENTS) + push_svc = SyncService(config_store=store, client_factory=lambda url, token: push_client) + + dry = push_svc.push(alias="prod", project_root=project_root, dry_run=True) + result = push_svc.push(alias="prod", project_root=project_root) + + push_client.delete_config_row.assert_not_called() + for res in (dry, result): + assert res["status"] == "no_changes" + (held,) = res["skipped_deletions"] + assert (held["config_id"], held["parent_config_id"], held["is_row"]) == ( + "row-001", + "cfg-001", + True, + ) + assert "--force" in res["skipped_deletions_reason"] + manifest = load_manifest(project_root) + parent = next(c for c in manifest.configurations if c.id == "cfg-001") + assert any(r.id == "row-001" for r in parent.rows) + + def test_push_does_not_recreate_config_or_rows_deleted_remotely( + self, tmp_config_dir: Path, tmp_path: Path + ) -> None: + """A config deleted on the remote since pull is not re-created, nor are its rows, + not even a row added locally under it -- also with --force (#792 H).""" + project_root = tmp_path / "project" + project_root.mkdir() + store, _ = self._init_and_pull(tmp_config_dir, project_root) + (row_file,) = ( + f + for f in project_root.rglob(CONFIG_FILENAME) + if "rows" in f.relative_to(project_root).parts + ) + new_row = row_file.parent.parent / "new-row" + new_row.mkdir() + (new_row / CONFIG_FILENAME).write_text("name: New Row\nparameters: {}\n") + + remaining = [c for c in SAMPLE_COMPONENTS if c["id"] != "keboola.ex-http"] + push_client = _make_sync_mock_client(components_response=remaining) + push_svc = SyncService(config_store=store, client_factory=lambda url, token: push_client) + + diff = push_svc.diff(alias="prod", project_root=project_root) + result = push_svc.push(alias="prod", project_root=project_root) + forced = push_svc.push(alias="prod", project_root=project_root, force=True) + + assert sorted((c["change_type"], c["config_name"]) for c in diff["changes"]) == [ + ("remote_deleted", "My HTTP Extractor"), + ("remote_deleted", "New Row"), + ("remote_deleted", "Users Endpoint"), + ] + assert diff["summary"]["remote_deleted"] == 3 + assert diff["summary"]["unchanged"] == 1 # cfg-002; rows are not counted + push_client.create_config.assert_not_called() + push_client.create_config_row.assert_not_called() + push_client.delete_config.assert_not_called() + push_client.delete_config_row.assert_not_called() + for res in (result, forced): + assert (res["status"], res["skipped"]) == ("no_changes", 3) + def test_push_row_encrypts_hash_secrets_before_api_call( self, tmp_config_dir: Path, tmp_path: Path ) -> None: From 95c785e82e71c2a8493c0503ab7a5bfdb190e08c Mon Sep 17 00:00:00 2001 From: soustruh Date: Tue, 29 Sep 2026 19:03:40 +0200 Subject: [PATCH 2/2] fix(sync): pull removes a tracked row deleted on the remote, so push does not re-create it (#792 H) --- .../kbagent/references/sync-rows-workflow.md | 3 +- src/keboola_agent_cli/services/_sync_stale.py | 55 +++++++++++++- .../services/sync_service.py | 13 ++++ tests/test_sync_service.py | 74 ++++++++++++++++++- 4 files changed, 140 insertions(+), 5 deletions(-) diff --git a/plugins/kbagent/skills/kbagent/references/sync-rows-workflow.md b/plugins/kbagent/skills/kbagent/references/sync-rows-workflow.md index 948f0f1eb..210775290 100644 --- a/plugins/kbagent/skills/kbagent/references/sync-rows-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/sync-rows-workflow.md @@ -222,7 +222,8 @@ Added rows (filesystem-only) show as `added`; removed rows (manifest-only after file deletion) show as `deleted` -- `push --force` DELETEs them via `_push_delete_row`, a plain push lists them under `skipped_deletions` *(since vNEXT, #792)*. A row deleted on the remote since the last pull shows -as `remote_deleted`; push never re-creates it. +as `remote_deleted`; push never re-creates it. `sync pull` deletes its +directory, or keeps an edited one and reports it as `skipped`. Human-mode diff output prints row-level changes with the same `+`/`~`/`-`/`=` prefixes as parent configs, e.g.: diff --git a/src/keboola_agent_cli/services/_sync_stale.py b/src/keboola_agent_cli/services/_sync_stale.py index 1ec7d2560..32dfa0320 100644 --- a/src/keboola_agent_cli/services/_sync_stale.py +++ b/src/keboola_agent_cli/services/_sync_stale.py @@ -26,7 +26,7 @@ from typing import TYPE_CHECKING, Any from ..constants import CONFIG_FILENAME -from ..sync.manifest import ManifestConfiguration +from ..sync.manifest import ManifestConfigRow, ManifestConfiguration from ._sync_baseline import extras_modified if TYPE_CHECKING: @@ -172,3 +172,56 @@ def apply_stale_sweep( ) if not dry_run and s.entry.path and s.entry.path not in live_paths: _remove_dir(branch_dir / s.entry.path, branch_dir) + + +def sweep_stale_rows( + service: SyncService, + config_dir: Path, + existing_rows: dict[str, dict[str, str]], + row_key_prefix: str, + remote_row_ids: set[str], + *, + theirs: bool, + dry_run: bool, + rel_path: str, + pull_details: list[dict[str, str]], +) -> list[ManifestConfigRow]: + """Handle the tracked rows of one config that are gone from the remote. + + Pull rebuilds the row entries from the remote listing, so a row deleted on + the remote used to lose its manifest entry while its directory stayed on + disk. The next diff read that directory as a new row, and push created it + again under a new id (issue #792 H). Now an unedited row directory is + deleted (``removed``). An edited one is kept with its entry (``skipped``), + so diff keeps reporting it as ``remote_deleted``; ``--theirs`` deletes it. + + Returns the entries to keep in the manifest. + """ + kept: list[ManifestConfigRow] = [] + for key, tracked in existing_rows.items(): + row_id = key.removeprefix(row_key_prefix) + if row_id == key or row_id in remote_row_ids: + continue + row_dir = config_dir / tracked["path"] + row_file = row_dir / CONFIG_FILENAME + pull_hash = tracked["pull_hash"] + edited = bool(pull_hash) and row_file.exists() and service._file_hash(row_file) != pull_hash + path = f"{rel_path}/{tracked['path']}" + detail = {"component_id": row_key_prefix.split("/", 1)[0], "config_name": "", "path": path} + if edited and not theirs: + kept.append( + ManifestConfigRow( + id=row_id, + path=tracked["path"], + metadata={ + "pull_hash": pull_hash, + "pull_config_hash": tracked["pull_config_hash"], + }, + ) + ) + pull_details.append({**detail, "action": "skipped", "reason": REMOTE_DELETED_REASON}) + continue + pull_details.append({**detail, "action": "removed"}) + if not dry_run and row_dir.is_dir(): + shutil.rmtree(row_dir) + return kept diff --git a/src/keboola_agent_cli/services/sync_service.py b/src/keboola_agent_cli/services/sync_service.py index d08e5923c..110110929 100644 --- a/src/keboola_agent_cli/services/sync_service.py +++ b/src/keboola_agent_cli/services/sync_service.py @@ -112,6 +112,7 @@ find_stale_entries, remote_deleted_conflicts, reserved_paths, + sweep_stale_rows, ) from ._sync_storage import ( fetch_jobs_per_config, @@ -988,6 +989,18 @@ def pull( }, ) ) + # A tracked row that is gone from the remote (issue #792 H). + row_manifests += sweep_stale_rows( + self, + config_dir, + existing_rows, + f"{component_id}/{config_id}/", + {str(row.get("id", "")) for row in cfg.get("rows", [])}, + theirs=theirs, + dry_run=dry_run, + rel_path=rel_path, + pull_details=pull_details, + ) # Record in manifest (store file hash for change detection). # For skipped configs: keep existing pull_hash (file untouched) diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index bb009b7b3..480602e15 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -3,6 +3,7 @@ Tests use tmp_path for filesystem operations and MagicMock for API client. """ +import copy import json from pathlib import Path from typing import Any @@ -537,10 +538,14 @@ def test_pull_removes_orphaned_directories(self, tmp_config_dir: Path, tmp_path: ) result2 = svc2.pull(alias="prod", project_root=project_root, force=True) - # Verify transformation was detected as removed + # Verify transformation was detected as removed. The extractor's row is + # gone from the remote too (SAMPLE_COMPONENTS_NO_ROWS), so pull removes + # its directory as well (#792 H) -- reported with a rows/ path. removed = [d for d in result2["details"] if d["action"] == "removed"] - assert len(removed) == 1 - assert removed[0]["component_id"] == "keboola.snowflake-transformation" + removed_configs = [d for d in removed if "/rows/" not in d["path"]] + assert len(removed_configs) == 1 + assert removed_configs[0]["component_id"] == "keboola.snowflake-transformation" + assert [d["component_id"] for d in removed if "/rows/" in d["path"]] == ["keboola.ex-http"] # Verify the orphan directory no longer exists on disk assert not orphan_dir.exists(), "Orphaned config directory should be deleted" @@ -1293,6 +1298,69 @@ def test_push_does_not_recreate_config_or_rows_deleted_remotely( for res in (result, forced): assert (res["status"], res["skipped"]) == ("no_changes", 3) + def _without_row_001(self) -> list[dict[str, Any]]: + """SAMPLE_COMPONENTS with row-001 deleted on the remote; cfg-001 stays.""" + components: list[dict[str, Any]] = copy.deepcopy(SAMPLE_COMPONENTS) + components[0]["configurations"][0]["rows"] = [] + return components + + def test_row_deleted_remotely_is_removed_by_pull_and_not_recreated( + self, tmp_config_dir: Path, tmp_path: Path + ) -> None: + """Remote row delete -> diff, pull, diff, push: the row is never created again (#792 H).""" + project_root = tmp_path / "project" + project_root.mkdir() + store, _ = self._init_and_pull(tmp_config_dir, project_root) + (row_file,) = ( + f + for f in project_root.rglob(CONFIG_FILENAME) + if "rows" in f.relative_to(project_root).parts + ) + client = _make_sync_mock_client(components_response=self._without_row_001()) + svc = SyncService(config_store=store, client_factory=lambda url, token: client) + + before = svc.diff(alias="prod", project_root=project_root) + pulled = svc.pull(alias="prod", project_root=project_root) + after = svc.diff(alias="prod", project_root=project_root) + pushed = svc.push(alias="prod", project_root=project_root) + + assert [(c["change_type"], c["config_id"]) for c in before["changes"]] == [ + ("remote_deleted", "row-001") + ] + assert not row_file.exists() + assert any(d["action"] == "removed" and "rows" in d["path"] for d in pulled["details"]) + assert after["changes"] == [] + assert pushed["status"] == "no_changes" + client.create_config_row.assert_not_called() + + def test_edited_row_deleted_remotely_is_kept_by_pull_and_not_recreated( + self, tmp_config_dir: Path, tmp_path: Path + ) -> None: + """An edited row whose remote is gone stays tracked as remote_deleted (#792 H).""" + project_root = tmp_path / "project" + project_root.mkdir() + store, _ = self._init_and_pull(tmp_config_dir, project_root) + (row_file,) = ( + f + for f in project_root.rglob(CONFIG_FILENAME) + if "rows" in f.relative_to(project_root).parts + ) + row_file.write_text(row_file.read_text().replace("/users", "/members")) + client = _make_sync_mock_client(components_response=self._without_row_001()) + svc = SyncService(config_store=store, client_factory=lambda url, token: client) + + pulled = svc.pull(alias="prod", project_root=project_root) + after = svc.diff(alias="prod", project_root=project_root) + pushed = svc.push(alias="prod", project_root=project_root) + + assert row_file.exists() + assert any(d["action"] == "skipped" and "rows" in d["path"] for d in pulled["details"]) + assert [(c["change_type"], c["config_id"]) for c in after["changes"]] == [ + ("remote_deleted", "row-001") + ] + assert pushed["status"] == "no_changes" + client.create_config_row.assert_not_called() + def test_push_row_encrypts_hash_secrets_before_api_call( self, tmp_config_dir: Path, tmp_path: Path ) -> None: