diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 42ff29b0..0e12e06a 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "plugins": [ { "name": "kbagent", - "version": "0.85.0", + "version": "0.85.1", "source": "./plugins/kbagent", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, sync configs as files, manage dev branches, and debug SQL in workspaces", "category": "development" diff --git a/plugins/kbagent/.claude-plugin/plugin.json b/plugins/kbagent/.claude-plugin/plugin.json index 5b86e68c..2bf3f42a 100644 --- a/plugins/kbagent/.claude-plugin/plugin.json +++ b/plugins/kbagent/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "kbagent", - "version": "0.85.0", + "version": "0.85.1", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, sync configs as files, manage dev branches, and debug SQL in workspaces", "author": { "name": "Keboola", diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 598a3f97..377c5964 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -1160,11 +1160,17 @@ events and emits a final `done` SSE frame mirroring the same record. `:` error on a body you know is correct, the config is fine and the validator is wrong -- upgrade rather than reaching for `--no-validate`, which switches off the checking that still works. -- **A body with no `parameters` key is validated whole** -- that is the - keboola.flow shape (`phases` / `tasks` sit at the configuration root). - Consequence: a parameters-level body posted by mistake as the whole - configuration still validates `ok`, because it is indistinguishable from a - flow-style config. Always POST the full object. +- **A body with no `parameters` key is validated as an EMPTY `parameters` + section (since v0.85.1, issue #605).** So a body that forgot the wrapper -- + the component's own fields sitting at the configuration root -- now fails + with the schema's required-field errors plus a `hint:` line naming the + missing wrapper. **Before v0.85.1 it validated `ok` and was POSTed + verbatim**, producing a live configuration with no `parameters` key, which + the UI and the component runtime both read as empty (blank boilerplate) even + though `--push` reported success. The whole-body exemption is now keyed on + the component (`keboola.flow` / `keboola.orchestrator`, whose `phases` / + `tasks` genuinely ARE the configuration root), not on the body's shape. + Always POST the full object: `{"storage": ..., "parameters": {...}}`. ## `config clone` duplicates a config whole; cross-project cannot carry secrets (since v0.84.2) diff --git a/pyproject.toml b/pyproject.toml index c1ff43f9..033ec2d1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "keboola-cli" -version = "0.85.0" +version = "0.85.1" description = "AI-friendly CLI for managing Keboola projects" readme = "README.md" requires-python = ">=3.12" diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index d6f0bac0..0ebcaa62 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -24,6 +24,25 @@ # Ordered newest-first. Each value is a list of brief one-line descriptions. CHANGELOG: dict[str, list[str]] = { + "0.85.1": [ + "Fix: `kbagent config new --push` no longer creates a broken configuration from a " + "body that forgot the `parameters` wrapper (#605). A component's " + "`configurationSchema` describes the CONTENTS of `configuration.parameters`, so a " + "flattened body -- the component's own fields sitting at the configuration root -- " + "matched that schema, validated `ok`, and was POSTed verbatim. The result was a " + "live configuration with no `parameters` key at all, which the Keboola UI and the " + "component runtime both read as empty (blank boilerplate) while `--push` reported " + "success and returned a config id. A body with no `parameters` key is now validated " + "as an EMPTY parameters section, so the schema's required fields fail it, and the " + "error list carries a `hint:` line naming the missing wrapper -- otherwise the " + "report reads \"'db' is a required property\" to a caller who did supply `db`, one " + "level too high. The whole-body exemption that this used to ride on is now keyed on " + "the component (`keboola.flow` / `keboola.orchestrator`, whose `phases` / `tasks` " + "genuinely are the configuration root) instead of on the body's shape -- deciding it " + "from the body meant the one thing under test was also the thing granting the " + "exemption. Pass `--no-validate` if a component legitimately takes a root-level " + "body. Completes the half of #587 that #589 left standing.", + ], "0.85.0": [ "BREAKING: the MCP passthrough is removed (epic #390 phase 3, deprecated since " "0.74.0). `kbagent tool list` and `kbagent tool call` are gone, `kbagent agent " diff --git a/src/keboola_agent_cli/constants.py b/src/keboola_agent_cli/constants.py index e65d562d..5ac44ed5 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -811,3 +811,18 @@ def _resolve_app_name() -> str: # Token value prefixes, used only for masking/validation -- never for auth logic. AUTH_ACCESS_TOKEN_PREFIX: str = "kbc_at_" AUTH_REFRESH_TOKEN_PREFIX: str = "kbc_rt_" + +# --- Components whose configurationSchema describes the configuration ROOT --- +# For almost every component the AI Service `configurationSchema` describes the +# CONTENTS of `configuration.parameters` (issue #587). Conditional flows are the +# exception: `phases` / `tasks` ARE the configuration root and the schema +# describes that root, so their body must be validated whole. Keying that +# exemption on the component -- rather than on "the body happens to carry no +# `parameters` key" -- is what stops a body that merely FORGOT the wrapper from +# validating clean and being created broken (issue #605). +ROOT_LEVEL_CONFIG_COMPONENTS: frozenset[str] = frozenset( + { + "keboola.flow", + "keboola.orchestrator", # legacy flows; kbagent cannot write them, but reads share this path + } +) diff --git a/src/keboola_agent_cli/services/config_service.py b/src/keboola_agent_cli/services/config_service.py index 2c4f557d..adaf2775 100644 --- a/src/keboola_agent_cli/services/config_service.py +++ b/src/keboola_agent_cli/services/config_service.py @@ -18,6 +18,7 @@ from ..config_store import ConfigStore from ..constants import ( CONFIG_STATE_MAX_BYTES, + ROOT_LEVEL_CONFIG_COMPONENTS, ) from ..errors import ConfigError, ErrorCode, KeboolaApiError from ..json_utils import compute_diff, deep_merge, set_nested_value @@ -2000,10 +2001,14 @@ def _validate_config_body( The schema describes the contents of the body's ``parameters`` key, so that section is what gets validated; sibling keys (``storage``, ``runtime``, ``authorization``) are not covered by it and are left - alone. A body with no ``parameters`` key is validated as a whole - (keboola.flow-style configurations). Reported error paths are prefixed - with ``parameters.`` so they point at the section the caller must fix. - Unwrapping affects validation only -- the POSTed body is never altered. + alone. A body with no ``parameters`` key is validated as an EMPTY + parameters section, which is what makes a forgotten wrapper fail loudly + instead of creating a broken configuration. Components whose schema + describes the configuration root (``ROOT_LEVEL_CONFIG_COMPONENTS``, + i.e. flows) are validated whole instead. Reported error paths are + prefixed with ``parameters.`` so they point at the section the caller + must fix. Unwrapping affects validation only -- the POSTed body is + never altered. Returns: ``("ok", [])`` when the body matches the schema. @@ -2043,12 +2048,13 @@ def _validate_config_body( # (": 'db' is a required property") while a body missing the # ``parameters`` wrapper validated clean. # - # Two shapes are validated WHOLE instead, and both are decided before - # unwrapping: + # Two shapes are validated WHOLE instead, and both are decided on the + # COMPONENT and the SCHEMA -- never on the body, which is the very thing + # under test (issue #605): # - # 1. The body carries no ``parameters`` key. For keboola.flow, - # ``phases`` / ``tasks`` ARE the configuration root and the schema - # describes that root, so there is nothing to unwrap. + # 1. The component's configuration root is what the schema describes. + # For keboola.flow, ``phases`` / ``tasks`` ARE that root, so there is + # nothing to unwrap. # 2. The schema itself declares a top-level ``parameters`` property, # which means it describes the whole configuration object. Deciding # on the body alone would regress such a component: every body used @@ -2057,9 +2063,17 @@ def _validate_config_body( # include one named ``parameters`` -- is misvalidated today too, so # preferring the schema signal here never trades a working case for # a broken one. - schema_is_whole_body = "parameters" in (schema.get("properties") or {}) - unwrapped = "parameters" in body and not schema_is_whole_body - target = body["parameters"] if unwrapped else body + schema_is_whole_body = component_id in ROOT_LEVEL_CONFIG_COMPONENTS or "parameters" in ( + schema.get("properties") or {} + ) + unwrapped = not schema_is_whole_body + # A body with no ``parameters`` key has an EMPTY parameters section -- + # it is not a licence to validate the body whole. Reading it the other + # way let a flattened body (the component's own fields sitting at the + # configuration root) match the parameters-level schema, pass, and be + # POSTed verbatim into a configuration the UI and the runtime both read + # as empty, with ``--push`` reporting success (issue #605). + target = body.get("parameters", {}) if unwrapped else body try: validator = jsonschema.Draft7Validator(schema) @@ -2085,6 +2099,14 @@ def _validate_config_body( return ("skipped", []) if errors: + if unwrapped and "parameters" not in body: + # Otherwise the report reads "'db' is a required property" to a + # caller who DID supply ``db`` -- one level too high. + errors.append( + "hint: the body has no 'parameters' key -- this component's schema " + "describes the CONTENTS of configuration.parameters, so wrap it as " + '{"parameters": {...}} (or pass --no-validate to skip this check)' + ) return ("failed", errors) return ("ok", []) diff --git a/tests/test_config_create_service.py b/tests/test_config_create_service.py index 3cd0c0c4..3de32953 100644 --- a/tests/test_config_create_service.py +++ b/tests/test_config_create_service.py @@ -561,3 +561,102 @@ def test_config_without_parameters_key_still_reports_its_errors( configuration={"phases": []}, # 'tasks' missing ) storage.create_config.assert_not_called() + + +# --------------------------------------------------------------------------- +# Missing `parameters` wrapper (issue #605) +# --------------------------------------------------------------------------- + +# A parameters-level schema with NO required fields: an empty ``parameters`` +# section is legitimate for such a component, so a body that carries only +# sibling keys must not be rejected. +OPTIONAL_TABLE_SCHEMA = { + "type": "object", + "properties": {"table": {"type": "string"}}, +} + + +class TestMissingParametersWrapper: + """A body missing the ``parameters`` wrapper must fail, not create silently. + + Issue #587 fixed one half of the mismatch (a correctly nested body was + rejected). The other half survived: a FLATTENED body -- the component's + parameters sitting at the configuration root -- was validated whole against + the parameters-level schema, matched it, and was POSTed verbatim. The + result was a live configuration with no ``parameters`` key at all, which + the UI and the component runtime both read as empty, while ``--push`` + reported success (issue #605). + """ + + def test_flattened_body_fails_instead_of_creating_a_broken_config( + self, tmp_config_dir: Path + ) -> None: + """The parameters section of a flattened body is empty, so a schema with + required fields must reject it -- nothing reaches the Storage API. + """ + service, storage, _ = _make_service(tmp_config_dir, schema=TABLE_SCHEMA) + + with pytest.raises(ConfigError, match="failed schema validation"): + service.create_config( + alias="prod", + component_id="keboola.ex-db-snowflake", + name="My Config", + configuration={"table": "orders"}, # missing the parameters wrapper + ) + storage.create_config.assert_not_called() + + def test_flattened_body_error_names_the_missing_wrapper(self, tmp_config_dir: Path) -> None: + """ "'table' is a required property" alone is baffling when the caller DID + supply ``table`` -- at the wrong level. The errors must say so. + """ + service, _, _ = _make_service(tmp_config_dir, schema=TABLE_SCHEMA) + + result = service.create_config( + alias="prod", + component_id="keboola.ex-db-snowflake", + name="My Config", + configuration={"table": "orders"}, + dry_run=True, + ) + + assert result["validation_status"] == "failed" + assert result["validation_errors"][0] == "parameters: 'table' is a required property" + assert any("no 'parameters' key" in err for err in result["validation_errors"]), result[ + "validation_errors" + ] + + def test_body_without_parameters_passes_when_the_schema_requires_nothing( + self, tmp_config_dir: Path + ) -> None: + """Treating a missing wrapper as an empty ``parameters`` section must not + invent a failure: a config that is legitimately storage-only still creates. + """ + service, storage, _ = _make_service(tmp_config_dir, schema=OPTIONAL_TABLE_SCHEMA) + + result = service.create_config( + alias="prod", + component_id="keboola.ex-db-snowflake", + name="My Config", + configuration={"storage": {"input": {"tables": []}}}, + ) + + storage.create_config.assert_called_once() + assert result["validation_status"] == "ok", result + + def test_whole_body_carve_out_is_keyed_on_the_component_not_the_body_shape( + self, tmp_config_dir: Path + ) -> None: + """``keboola.flow`` keeps whole-body validation because ITS configuration + root is what the schema describes -- an ordinary component with a + flow-shaped body does not inherit that exemption. + """ + service, storage, _ = _make_service(tmp_config_dir, schema=FLOW_SCHEMA) + + with pytest.raises(ConfigError, match="failed schema validation"): + service.create_config( + alias="prod", + component_id="keboola.ex-db-snowflake", + name="My Config", + configuration={"phases": [], "tasks": []}, + ) + storage.create_config.assert_not_called() diff --git a/tests/test_e2e.py b/tests/test_e2e.py index c136d1cf..2d18ecaf 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -2259,6 +2259,31 @@ def _test_config_new_push_schema_validation(self) -> None: # which pointed the reader at the wrong level of their own config. assert all(e.startswith("parameters") for e in invalid["validation_errors"]), invalid + # A body that FORGOT the `parameters` wrapper must fail too (issue + # #605). It used to validate clean -- the flattened body matched the + # parameters-level schema, so `--push` created a configuration with no + # `parameters` key at all, which the UI and the runtime read as empty + # while reporting success. Same `db` payload as `valid_body`, one level + # too high. + flattened = self._run_ok( + "config", + "new", + "--component-id", + "keboola.ex-db-mysql", + "--project", + self.alias, + "--name", + f"{RUN_ID} validation-flat (dry)", + "--push", + "--no-files", + "--configuration", + json.dumps({"db": {"host": "mysql.example.com", "database": "e2e"}}), + "--dry-run", + )["data"] + + assert flattened["validation_status"] == "failed", flattened + assert any("no 'parameters' key" in e for e in flattened["validation_errors"]), flattened + def _test_config_clone(self) -> None: """Test ``config clone`` -- whole-configuration duplicate (0.84.2+, #587). diff --git a/uv.lock b/uv.lock index c7c05bdf..5526c55f 100644 --- a/uv.lock +++ b/uv.lock @@ -581,7 +581,7 @@ wheels = [ [[package]] name = "keboola-cli" -version = "0.85.0" +version = "0.85.1" source = { editable = "." } dependencies = [ { name = "croniter" },