Skip to content

fix(sync): reject non-scalar values in sync clone override files - #646

Merged
padak merged 4 commits into
mainfrom
fix/sync-clone-override-shape-validation
Aug 22, 2026
Merged

fix(sync): reject non-scalar values in sync clone override files#646
padak merged 4 commits into
mainfrom
fix/sync-clone-override-shape-validation

Conversation

@padak

@padak padak commented Aug 22, 2026

Copy link
Copy Markdown
Member

Problem

sync clone's override-file loader (_load_override_file in commands/sync.py, backing --bucket-map / --variable-values / --instance-rename) coerced every YAML value with bare str(value). A nested mapping — one fat-fingered colon away from valid input:

in.c-old:
  new: in.c-new

was silently accepted as the literal string "{'new': 'in.c-new'}" and pushed into the target project as a "bucket ID". No error, no warning — just silently wrong rewrites.

Fix

Extract the load-and-validate logic into a small shared module src/keboola_agent_cli/yaml_input.py:

  • load_flat_scalar_mapping(path, *, label) — loads a JSON/YAML file as a flat {str: str} mapping. Scalar values (string/number/boolean) keep the existing str coercion; a mapping, list, or null value is rejected with a ConfigError naming the offending key and its actual type. The sync clone command already maps ConfigError to CONFIG_ERROR + exit 5, so no new wiring.
  • yaml_type_name(value) — type names in YAML vocabulary (mapping/list/null/...), not Python's — the author is reading their own YAML file, not a traceback.

This mirrors the shape-validation approach #645 introduced for storage describe-batch --from-file.

Reuse notes (the three near-identical implementations)

  • commands/sync.py _load_override_file — now delegates to the shared helper.
  • services/_describe_batch_input.py (fix(storage): reject malformed describe-batch --from-file shapes with a structured error (#640) #645, merged while this PR was open) — now imports the shared yaml_type_name and drops its private _TYPE_NAMES/_type_name copy; its per-section subjects/examples and the nested columns section stay its own.
  • commands/flow.py _load_flow_yaml — deliberately left alone: flow definitions are legitimately nested, so the flat-scalar contract does not apply; its existing top-level-mapping check already rejects correctly and has no silent-stringification bug.

Versioning

0.89.0 (config delete trash guard, #643) is merged on main but not yet published, so this PR's changelog entry is folded into that in-flight 0.89.0 release (changelog-check allows exactly one unreleased version) and the docs tags read (since v0.89.0) — a concrete tag, per the Devin review finding, instead of a (since vX.Y.Z) placeholder.

Tests

  • New tests/test_yaml_input.py (18 tests): valid JSON/YAML, scalar coercion, rejection of nested mapping / list / null values (message names the key and the YAML type), top-level non-mapping, empty file, missing file, invalid YAML, label wording.
  • New CLI test in tests/test_sync_clone.py: a nested --bucket-map value exits 5, names the offending key, and never reaches SyncService.clone_project.
  • Written test-first: both failed on unpatched code (the CLI test exited 0 — the bug live).

Docs: gotchas.md (sync clone section) + sync-workflow.md override-file notes + CLAUDE.md sync clone line, all tagged (since v0.89.0).

make check green after the rebase onto 0.89.0 main: 5953 passed, 12 skipped.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Comment thread plugins/kbagent/skills/kbagent/references/gotchas.md Outdated
padak added a commit that referenced this pull request Aug 22, 2026
…rsion

Devin review on #646 flagged the '(since vX.Y.Z)' placeholder in the new
sync clone override gotcha -- the repo rule requires a concrete version tag,
and an identical placeholder from the 0.88.0 cycle (gotchas.md, describe-batch
section) shows the 'tag at release' step rots. Bump the version in this PR
instead: pyproject 0.88.1 + changelog entry (#646) + version-sync
(plugin.json, marketplace.json, uv.lock), and tag the gotcha, sync-workflow
note and CLAUDE.md line with 0.88.1.
padak added 4 commits August 22, 2026 23:24
sync clone's --bucket-map / --variable-values / --instance-rename loader
coerced every YAML value with bare str(value), so a nested mapping (one
fat-fingered colon away from valid input, e.g. 'in.c-old:' followed by an
indented 'new: in.c-new') was silently used as the literal string
"{'new': 'in.c-new'}" -- a bogus bucket ID pushed into the target project.

Extract the load-and-validate logic into a shared yaml_input module
(load_flat_scalar_mapping + yaml_type_name) that rejects non-scalar values
and None with a ConfigError naming the offending key and its actual type in
YAML vocabulary (mapping/list/null), mirroring the describe-batch shape
validation approach from PR #645. Scalars keep the existing str coercion.

commands/flow.py's _load_flow_yaml is deliberately left alone: flow
definitions are legitimately nested, so only the top-level-mapping check is
shared there and it already rejects correctly.
…rsion

Devin review on #646 flagged the '(since vX.Y.Z)' placeholder in the new
sync clone override gotcha -- the repo rule requires a concrete version tag,
and an identical placeholder from the 0.88.0 cycle (gotchas.md, describe-batch
section) shows the 'tag at release' step rots. Bump the version in this PR
instead: pyproject 0.89.1 (renumbered from 0.88.1 after 0.89.0 landed on main)
+ changelog entry (#646) + version-sync (plugin.json, marketplace.json,
uv.lock), and tag the gotcha, sync-workflow note and CLAUDE.md line with
0.89.1.
…dation

Now that #645 is on main, drop _describe_batch_input.py's private
_TYPE_NAMES/_type_name copy in favor of the shared yaml_input.yaml_type_name
introduced for the sync clone override validation -- the reuse follow-up both
PRs promised. Behavior unchanged; the describe-batch error messages keep
their per-section subjects and examples.
0.89.0 (config delete trash guard, #643) is merged on main but not yet
published, and changelog-check allows exactly one in-flight version -- so the
#646 entry joins the 0.89.0 release being prepared instead of stacking a
second unreleased bump on top. pyproject back to 0.89.0 (version-sync'd),
docs tags now read (since v0.89.0).
@padak
padak force-pushed the fix/sync-clone-override-shape-validation branch from 1c854db to 9c61b4d Compare August 22, 2026 21:29
@padak
padak merged commit 12fad00 into main Aug 22, 2026
4 checks passed
@padak
padak deleted the fix/sync-clone-override-shape-validation branch August 22, 2026 21:54
padak added a commit that referenced this pull request Aug 23, 2026
…es (#651)

Release prep for 0.89.0: adds the changelog entries for everything merged since v0.88.0 (#620, #642, #643, #644, #645, #646, #647, #648, #649, #650, #517, #586, #641), resolves every vNEXT placeholder left by feature PRs to v0.89.0 per the new #648 release process, closes the 10 gaps a full doc-surface audit found across the kbagent plugin (SKILL.md triggers, commands-reference, gotchas, workflow files, keboola-expert.md, AGENT_CONTEXT, CLAUDE.md), and records the live e2e verification evidence. Version files were already at 0.89.0 (bumped by #643); make version-sync is a no-op.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant