feat(config): add config clone to duplicate a configuration whole - #596
Conversation
zajca
left a comment
There was a problem hiding this comment.
Changes requested based on the completed review findings.
Devin review on #596; zajca requested changes on the strength of it. BLOCKING -- a re-supplied row secret landed in the parent body. Row ciphertext is reported under a `rows[N].` prefix and the refusal check accepted a `--secret` at that exact path, but the substitution applied every override to the parent. So a cross-project clone of a config with per-row credentials reported success while the copied rows still held the source project's undecryptable ciphertext -- precisely the outcome this command exists to prevent, discoverable only at runtime, in the other project. Proven before fixing: set_nested_value put the value under a literal "rows[0]" key in the parent. List-element secret paths were unusable. `parameters.values.[0].#token` raises ValueError in set_nested_value, so the CLI refused the clone with an instruction that then crashed when followed. Paths now use a plain index segment and point at the `value` field that actually gets replaced: `parameters.values.0.value`. Ciphertext under a plain (non-`#`) key is now refused. collect_secrets only picks up `#` keys, so a `--secret` supplied for such a path would have been written to the target project in plaintext. Refusing and pointing at `kbagent encrypt values` + `--set` is the only option that neither breaks the clone nor leaks the credential. `--set` is encrypted on both paths. The same-project path wrote overrides through a direct update_config, bypassing the Encryption API -- and `--set 'parameters.db.#password=...'` is the documented way to repoint a copy at another database, so this was a live plaintext write that version history would keep. `--allow-plaintext-on-encrypt-failure` was a no-op on that path; now it means what it says. Also: cross-project inherits the source description instead of blanking it; `--target-branch` is rejected on a same-project clone when it differs from `--branch`, because the server-side copy writes into the source's branch and silently using the wrong one is not acceptable; a row failing mid-copy reports the created config id and how many rows landed, so the partial clone can be found and removed. Added the matching POST .../{config_id}/clone route -- CONTRIBUTING.md mandates 1:1 CLI/HTTP coverage and I had skipped it without saying so. The loc baseline is back to main's values. Raising a grandfathered ceiling to fit two new lines is exactly what CONTRIBUTING.md forbids; `config oauth-url` moved into its own private module instead, so commands/config.py SHRANK to 1953 code lines from 2007. 7 new tests covering each fix.
f8683f0 to
edcb35e
Compare
|
@zajca — review findings addressed in edcb35e, all nine threads replied to and resolved. Summary of what actually changed, since three of them were real defects rather than style: The blocking one was real and I had missed it. A re-supplied row secret ( Two more that would have leaked or crashed:
Also fixed: list-element secret paths were emitted as Two process points you were right to flag via the review: I added the missing 7 new tests, |
Closes #587. There was no way to copy a configuration, so people rebuilt the body from `config detail` output -- typically taking `configuration["parameters"]` and nothing else. A configuration's root also carries `storage`, `runtime` and `authorization`, and dropping one is silent. The reporter lost `runtime.parallelism`; Keboola falls back to `parallelism: 1`, so a 65-row Snowflake writer ran strictly sequentially -- 140 minutes instead of the expected 60-90. Nothing errored. It surfaced only by hand-comparing per-row job timestamps afterwards. Two paths, and the split is not arbitrary -- it is encryption. Within a project the Storage API copies server-side (POST .../configs/{id}/versions/{v}/create), so nothing is rebuilt at all. Verified live against a real project: the clone came back with `runtime.parallelism` AND both rows of a 2-row source, so no client-side row copying is needed. `--set PATH=VALUE` edits are applied afterwards as a normal update on the new config, which means an override can never be the reason a key went missing. Across projects that endpoint is unavailable, and would be wrong anyway: a Keboola ciphertext is scoped to the project it was encrypted in. Copying it verbatim produces a configuration that looks complete and fails at runtime, in a project nobody is watching. So the cross-project path assembles the body itself, recreates rows one by one, and refuses (exit 5) while any `KBC::` value is unaccounted for -- listing every path, from the parent body and from rows (`rows[N].path`). Each is re-supplied with `--secret PATH=VALUE` and encrypted in the TARGET project on write. `--dry-run` reports those paths in `missing_secrets` rather than refusing, which is how a caller discovers what to gather. Storage mappings are copied verbatim: bucket and table IDs are NOT remapped, and cross-project output says so out loud. `sync clone` is the command that remaps; duplicating that here would be scope creep. The flow lives in a new services/_config_clone.py rather than in config_service.py, which `make loc-check` already reports as over its per-layer budget with an explicit "split it first" instruction. Tests: 15 service tests (both paths, encrypted-value refusal, row copying, client selection) and 9 CLI tests. The cross-project tests were written after the module, so I mutation-tested them -- disabling encrypted detection and disabling row copying each fail exactly the tests that claim to cover them. The CLI suite exists because a first live run crashed on a human_formatter arity bug that no service test could catch; re-introducing that bug fails 6 of the 9. E2E step 19d duplicates a config carrying a runtime sibling and two rows, and asserts both survive.
commands/config.py was at its grandfathered loc-check ceiling (2007 code
lines); the clone command pushed it 2 lines over. Moved the command body
into a new commands/_config_clone_cmd.py, mounted via register(), mirroring
the existing commands/_storage_snapshots.py pattern. Net effect: config.py
drops the ~230-line clone command down to a 2-line import+register hook.
Ran `make loc-baseline` to record the new (lower) code-line counts for
every grandfathered file, per its own documented purpose ("after a
split"). This is that split.
WIP checkpoint -- pausing for the weekend. Remaining before this is
ready for review:
- Verify Devin/kbagent-pr-reviewer haven't already run on stale state
- Open the PR against main (branch: feat/config-clone-587)
- Request review from zajca per the user's explicit instruction
Devin review on #596; zajca requested changes on the strength of it. BLOCKING -- a re-supplied row secret landed in the parent body. Row ciphertext is reported under a `rows[N].` prefix and the refusal check accepted a `--secret` at that exact path, but the substitution applied every override to the parent. So a cross-project clone of a config with per-row credentials reported success while the copied rows still held the source project's undecryptable ciphertext -- precisely the outcome this command exists to prevent, discoverable only at runtime, in the other project. Proven before fixing: set_nested_value put the value under a literal "rows[0]" key in the parent. List-element secret paths were unusable. `parameters.values.[0].#token` raises ValueError in set_nested_value, so the CLI refused the clone with an instruction that then crashed when followed. Paths now use a plain index segment and point at the `value` field that actually gets replaced: `parameters.values.0.value`. Ciphertext under a plain (non-`#`) key is now refused. collect_secrets only picks up `#` keys, so a `--secret` supplied for such a path would have been written to the target project in plaintext. Refusing and pointing at `kbagent encrypt values` + `--set` is the only option that neither breaks the clone nor leaks the credential. `--set` is encrypted on both paths. The same-project path wrote overrides through a direct update_config, bypassing the Encryption API -- and `--set 'parameters.db.#password=...'` is the documented way to repoint a copy at another database, so this was a live plaintext write that version history would keep. `--allow-plaintext-on-encrypt-failure` was a no-op on that path; now it means what it says. Also: cross-project inherits the source description instead of blanking it; `--target-branch` is rejected on a same-project clone when it differs from `--branch`, because the server-side copy writes into the source's branch and silently using the wrong one is not acceptable; a row failing mid-copy reports the created config id and how many rows landed, so the partial clone can be found and removed. Added the matching POST .../{config_id}/clone route -- CONTRIBUTING.md mandates 1:1 CLI/HTTP coverage and I had skipped it without saying so. The loc baseline is back to main's values. Raising a grandfathered ceiling to fit two new lines is exactly what CONTRIBUTING.md forbids; `config oauth-url` moved into its own private module instead, so commands/config.py SHRANK to 1953 code lines from 2007. 7 new tests covering each fix.
Main already carried an unreleased 0.84.2 (billing credits #597, config state-get/state-set #598, kbc->kbagent CI/CD skill #402), and v0.84.1 is the newest published release. Folding clone into that same unreleased version ships one release instead of two, and leaves 0.85.0 free for the `tool` group removal it is already promised to (epic #390 phase 3) -- the doc references to that removal deliberately still say 0.85.0. Version files, the changelog key (clone notes merged above the existing 0.84.2 entries) and the since-tags in CLAUDE.md, context.py, gotchas.md, commands-reference.md, keboola-expert.md and the E2E docstring all move to 0.84.2.
edcb35e to
a5c8bbc
Compare
Dismissing as stale, with the PR author's authorisation. This review was submitted against f8683f0; HEAD is now a5c8bbc. All nine findings it referenced (via the Devin pass) have been fixed, each replied to on its thread and resolved — including the blocking one where a re-supplied row secret was written to the parent body instead of the row. Full summary: #596 (comment). Re-review welcome; not blocking the 0.84.2 release on it.
Closes #587 (remainder — the schema-validation bug was #589, already merged).
The problem
There was no way to copy a configuration, so people rebuilt the body from
config detailoutput — typically copyingconfiguration["parameters"]and nothing else. A configuration's root also carriesstorage,runtimeandauthorization, and dropping one is silent: the reporter lostruntime.parallelism, Keboola fell back toparallelism: 1, and a 65-row Snowflake writer ran strictly sequentially — 140 minutes instead of the expected 60-90, caught only by hand-comparing per-row job timestamps afterwards.The fix
kbagent config clone --project P --component-id C --config-id ID --name N [--target-project P2] [--set PATH=VALUE ...] [--secret PATH=VALUE ...] [--dry-run]Two paths, split by encryption — not an arbitrary design choice:
Same project (default): server-side copy via
POST .../configs/{id}/versions/{v}/create. Nothing is rebuilt. Verified live against a real project: a 2-row source with aruntimesibling produced a 2-row clone carryingruntime.parallelismintact.--set PATH=VALUEis applied afterwards as a normal update on the copy, so an override can never be why a key went missing.Cross project (
--target-project): reassembled client-side, rows recreated one by one — because a Keboola ciphertext is scoped to the project it was encrypted in; copying it verbatim would produce a configuration that looks complete and fails at runtime, in a project nobody is watching. AnyKBC::value — in the parent body or in a row (rows[N].path) — makes the clone fail fast (exit 5), listing every affected path, until re-supplied via--secret PATH=VALUE(encrypted in the target project on write).--dry-runreports the same paths inmissing_secretsinstead of refusing, which is how you discover what to gather.Storage bucket/table IDs are copied verbatim, never remapped —
sync cloneis the command that does that; duplicating it here would be scope creep.Structure
New
services/_config_clone.py(notconfig_service.py, whichmake loc-checkalready flags as over its per-layer budget). Newcommands/_config_clone_cmd.pyfor the same reason on the commands side — mirrors the existingcommands/_storage_snapshots.pypattern.KeboolaClient.create_config_copyis the one new client method.Testing
human_formatterarity bug (lambdataking 1 arg instead of 2) that no service test could catch; re-introducing that bug fails 6 of the 9 CLI tests.runtimesibling and two rows, asserts both survive.make check: 5591 passed, 12 skipped.Docs
CLAUDE.md,
context.py(AGENT_CONTEXT),commands-reference.md,gotchas.md,keboola-expert.md(had to trim ~90 bytes elsewhere to stay under its 62000-byte cap). Version bumped to 0.85.0 with a full changelog entry.Review
@zajca — could you take a look at this one specifically? The cross-project path (encrypted-value detection + refusal + row-by-row recreation) is the part I'd most want a second pair of eyes on.