Repository navigation
fix(sync): keep UI edits of adopted configs and encrypt secrets before the first push write - #839
Merged
Merged
Conversation
…e the first push write (#792 E, F)
keboola-pr-reviewer-bot
left a comment
There was a problem hiding this comment.
Verdict: needs_human (risk 3/5) · profile keboola-mcp-server
Well-tested sync bug-fix, but it reshapes the push/pull write and encryption-ordering paths, which warrants a human owner.
Impact flags: possible rollback re-introduction — see Check Run summary.
Concerns:
src/keboola_agent_cli/services/_sync_push_ops.py: Encrypt-first changes push failure semantics on the config write pathsrc/keboola_agent_cli/sync/diff_engine.py: Adopted files without baseline now classified conflict; author questions exit-0 behaviorsrc/keboola_agent_cli/services/sync_service.py: pull now skips untracked local-edited files; verify no legitimate overwrite regressed
soustruh
commented
Oct 10, 2026
soustruh
left a comment
Contributor
Author
There was a problem hiding this comment.
No findings. Checked:
- E fix: an adopted file without
_keboola.base_config_hashis aconflictin diff. A scaffold fromconfig new --pushrecords the hash and gets a 3-way diff. Two files with one id are both conflicts. Pull keeps an adopted file with local changes, and--theirsoverwrites it. Push removes the key after the first write. Targeted tests intests/test_sync_adopted_files.pyandtests/test_sync_diff_engine.pypass. - F fix:
encrypt_secretsruns before the first write and covers configs and rows. The deletes inpush --force(includingsyncWorkspacesdeletes) run after it, so a failure writes nothing.--allow-plaintext-on-encrypt-failureskips the pre-encryption, as the docs say.ENCRYPTION_FAILEDstill maps to exit code 1, so the #747 exit codes do not change.tests/test_sync_push_encrypt_first.pyasserts an empty API log and an unchanged manifest and files on failure. - Docs (
CLAUDE.md,context.py,gotchas.md, sync and scaffold workflows, commands reference, expert agent) match the code and carryvNEXTtags. - Ran the changed test files locally with a temporary config dir: 188 passed, 2 xfailed. I did not run the full
make check.
zajca
approved these changes
Oct 10, 2026
This was referenced Oct 11, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #792
What was wrong
sync pushoverwrote an edit made in the UI. This happened for a config file that has a_keboola.config_idbut no manifest entry, for example aconfig new --push --output-dirscaffold. Sync adopted such a file by its id. It had no base state, so the diff could not see which side changed. It reportedmodifiedand push wrote the local file over the remote.sync pullalso overwrote such a file. A local edit was lost without a message.sync pushencrypted secrets one config at a time, just before each write. AnENCRYPTION_FAILEDstopped the push after some configs were already created. The manifest did not record them. The nextsync pushcreated them again.What changed
config new --push --output-dirwrites_keboola.base_config_hashinto the_config.ymlfooter. The value is the hash of the created config as the API returned it.conflictwhen it differs from the remote. It is nevermodified. Two files that adopt the same id are bothconflict.base_config_hashfrom the file.sync pullkeeps an adopted file with local changes and reports it asskipped.sync pull --theirsoverwrites it.sync pushencrypts all secrets of the changes it will write before the first write. It makes one Encryption API call per component. The writes use the encrypted values from that call. If encryption fails, push writes nothing, and the error message ends withNothing was pushed.--allow-plaintext-on-encrypt-failurekeeps its old behavior. Push does not encrypt in advance with this flag.keboola.sandboxesworkspace configs in asyncWorkspacestree.sync_service.pyto_sync_push_ops.pyto stay in the file-size budget. Its behavior did not change.commands-reference.md,sync-workflow.md,gotchas.md,scaffold-workflow.md, thekbagent contexttext andformal/sync/README.mddescribe the new behavior withvNEXTtags.Open questions
conflict, the user must runsync pull, which keeps the local file, and then resolve the conflict by hand, or usesync pull --theirsto take the remote. Do we need a flag that pushes the local file anyway?_keboola.base_config_hash), not in the manifest.config new --push --output-dircan write into a directory that has no manifest, or into a tree that does not track the config yet. The file is the only place that always exists. After the first push the base moves to the manifest. Shouldconfig newadd a manifest entry instead when a manifest exists?conflictfrom E goes toskipped. It is not inerrors[], so the push exits 0, like every other skipped conflict since fix(cli): partial failures set the exit code and the headline (CLI-6) #747. Is this correct for E?Tests
tests/test_sync_formal_counterexamples.py: the replay tests for E and F werexfail. They now pass without the marker.tests/test_sync_adopted_files.py: diff, push and pull of adopted files with and without a base, a remote edit, edits on both sides, a placeholder scaffold, and two files with one id.tests/test_sync_push_encrypt_first.py: an encryption failure stops the push before any write. A retry applies each change once. Each secret is encrypted once, and the writes send the ciphertext. Rows are covered. A value the API leaves in plaintext is not cached. The plaintext fallback flag skips the step.test_sync_diff_engine.py,test_sync_service.py,test_sync_reconcile.py,test_component_service.pyandtest_config_create_cli.py.make checkpasses.