From ceb42e6265724ee3f108daf6573ceae77e787151 Mon Sep 17 00:00:00 2001 From: Petr Date: Mon, 17 Aug 2026 17:32:50 -0400 Subject: [PATCH 1/3] feat(config): add state-get/state-set, guard --set against API siblings Closes #593. kbagent could read a configuration's runtime state (`config detail --with-state`) but had no way to write it, so seeding or resetting state was only reachable by clicking in the KBC UI. Worse, the closest-looking CLI path silently wrote to the wrong place. Part A -- `config update --set` / `config row-update --set` now reject (exit 2) any path whose first segment is a top-level sibling of `configuration` (`state`, `rows`, `name`, ...). Previously `--set 'state.x=y'` exited 0, bumped the config version and produced a plausible `--dry-run` diff while writing to `configuration.state.x` and leaving the runtime state untouched -- a write that looked successful at every observable layer while doing nothing. The guard runs before any network call, so `--dry-run` is rejected too, and the error routes to the right tool per prefix. `set_nested_value` also stops silently turning `files[0]` into a literal key and points at the supported `files.0` form. Part B -- `config state-get` / `config state-set` call the dedicated Storage API state endpoints (branch-scoped, with `--row-id` routing to the row endpoint). The payload is validated as a JSON object under 4 MB before the round-trip, `--dry-run` previews a current-vs-new diff, an unchanged state short-circuits without an API call, and a missing row id fails loudly instead of returning an empty dict. Note the two state endpoints answer with different shapes: the root PUT returns the full configuration detail, the row PUT returns the bare row object. Reading the row back out of `rows[]` regardless made every row write report a false NOT_FOUND despite a 200 and a landed write. That was caught by running the E2E suite against a live project -- mock-based tests could not, since the mocks returned the shape the author assumed rather than the shape the API sends. Both shapes are now pinned by regression tests. Motivation: Keboola is retiring `processed_tags`/`query` in file input mapping in favour of `changed_since: adaptive`, and `adaptive` with an empty state reloads the entire file history. A dev branch always starts with `state: {}`, so validating that migration safely requires seeding a known `lastImportId` first -- previously the only manual step in an otherwise scriptable, deadline-driven migration. Adds REST parity (`GET`/`PUT /configs/{project}/{component}/{config}/state`), permission entries, a `config-state-workflow` reference, two gotchas, and E2E coverage. `commands/config.py` is CI-ratcheted to shrink only, so the new commands live in `commands/config_state.py` and 13 duplicated try/except blocks were folded into a shared error-mapping helper. --- CLAUDE.md | 10 + plugins/kbagent/skills/kbagent/SKILL.md | 3 + .../kbagent/references/commands-reference.md | 2 + .../references/config-state-workflow.md | 208 +++ .../skills/kbagent/references/gotchas.md | 74 ++ src/keboola_agent_cli/changelog.py | 46 + src/keboola_agent_cli/client/configs.py | 90 ++ src/keboola_agent_cli/commands/config.py | 174 +-- .../commands/config_state.py | 199 +++ src/keboola_agent_cli/commands/context.py | 17 + src/keboola_agent_cli/constants.py | 45 + src/keboola_agent_cli/json_utils.py | 20 +- src/keboola_agent_cli/permissions.py | 2 + .../server/routers/configs.py | 55 + .../services/config_service.py | 280 ++++ tests/test_config_set_guard.py | 504 ++++++++ tests/test_config_state.py | 1134 +++++++++++++++++ tests/test_e2e.py | 369 ++++++ tests/test_server_router_calls.py | 142 +++ 19 files changed, 3263 insertions(+), 111 deletions(-) create mode 100644 plugins/kbagent/skills/kbagent/references/config-state-workflow.md create mode 100644 src/keboola_agent_cli/commands/config_state.py create mode 100644 tests/test_config_set_guard.py create mode 100644 tests/test_config_state.py diff --git a/CLAUDE.md b/CLAUDE.md index 91b00e1e..d252de59 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -389,6 +389,16 @@ kbagent config row-create --project NAME --component-id ID --config-id ID --name kbagent config row-update --project NAME --component-id ID --config-id ID --row-id ID [--name N] [--description D] [--configuration JSON|@file|-] [--change-description TEXT] [--is-disabled | --is-enabled] [--branch ID] [--allow-plaintext-on-encrypt-failure] kbagent config row-delete --project NAME --component-id ID --config-id ID --row-id ID [--branch ID] [--yes] kbagent config oauth-url --project NAME --component-id ID --config-id ID [--redirect-url URL] +kbagent config state-get --project NAME --component-id ID --config-id ID [--row-id ID] [--branch ID] +kbagent config state-set --project NAME --component-id ID --config-id ID [--row-id ID] --state JSON|@file|- [--branch ID] [--dry-run] [--yes] +# state-get/state-set (0.84.2+, #593): read/write a config's runtime state via the dedicated +# PUT .../state endpoint -- closes the gap where `config update --set 'state...'` looked +# successful but silently wrote to configuration.state.* instead (runtime state untouched). +# Since 0.84.2, `config update --set` / `config row-update --set` REJECT (exit 2) any path +# whose first segment is a top-level API field (state, rows, name, description, id, version, +# currentVersion, changeDescription, created, creatorToken, isDeleted, isDisabled) -- use +# state-set / --name / --description / row-update --is-disabled instead, or --configuration +# for a genuine configuration. key. kbagent search QUERY [--project NAME] [--type table|bucket|config|flow|data-app|transformation] [--search-type textual|config-based] [--regex] [--limit N] # --regex (0.67.0+): opt-in regex mode (mode=regex). Case-insensitive whole-term match on ENTITY NAMES diff --git a/plugins/kbagent/skills/kbagent/SKILL.md b/plugins/kbagent/skills/kbagent/SKILL.md index a61a2c90..288f4dee 100644 --- a/plugins/kbagent/skills/kbagent/SKILL.md +++ b/plugins/kbagent/skills/kbagent/SKILL.md @@ -126,6 +126,8 @@ When working inside a git repository or project directory, run `kbagent init` (o | Update an existing configuration row | `kbagent config row-update --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID --row-id ROW-ID` | | Delete a configuration row | `kbagent config row-delete --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID --row-id ROW-ID` | | Requires master token. | `kbagent config oauth-url --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID` | +| Read the runtime ``state`` dict of a configuration or one of its rows | `kbagent config state-get --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID` | +| Overwrite the runtime ``state`` dict of a configuration or one of its rows | `kbagent config state-set --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID --state STATE` | | List data apps across one or more registered projects | `kbagent data-app list` | | Show merged Data Science + Storage detail for one data app | `kbagent data-app detail --project PROJECT --app-id APP-ID` | | Create a Keboola data app end-to-end (POST + encrypt + PUT + deploy) | `kbagent data-app create --project PROJECT --name NAME --slug SLUG` | @@ -409,6 +411,7 @@ For detailed response parsing rules and common pitfalls, see [gotchas](reference | **Semantic layer (metastore)** -- models, metrics, datasets, constraints, glossary; validate / export / diff / promote / build / token | [semantic-layer-workflow](references/semantic-layer-workflow.md) | | **Developer Portal** (identity CRUD, list/get apps, create/patch/upload-icon/publish/deprecate; TTY-confirm on writes) | [dev-portal-workflow](references/dev-portal-workflow.md) | | **Config metadata** (list/get/set/delete arbitrary key-value metadata on a configuration) | [config-metadata-workflow](references/config-metadata-workflow.md) | +| **Config runtime state** (`state-get`/`state-set`; root vs row; seeding a dev branch before testing `changed_since: adaptive`) | [config-state-workflow](references/config-state-workflow.md) | | **Storage descriptions** (describe bucket / table / column, batch from YAML) | [storage-describe-workflow](references/storage-describe-workflow.md) | | **Deep column-level lineage** (`lineage build --ai`, column graph, ER + HTML output) | [lineage-deep-workflow](references/lineage-deep-workflow.md) | | **Session permissions firewall** (`--deny-writes` / `--deny-destructive`, persisted policies, `permissions check`) | [permissions-workflow](references/permissions-workflow.md) | diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index 0b94abbe..bff8368c 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -142,6 +142,8 @@ Requires a **super-admin** Manage API token (same kind as `org setup`). Same def - `config row-update --project NAME --component-id ID --config-id ID --row-id ID [--name N] [--description D] [--configuration JSON|@file|-] [--change-description TEXT] [--is-disabled | --is-enabled] [--branch ID] [--allow-plaintext-on-encrypt-failure]` -- update an existing configuration row. Pass only the fields you want to change; omitted fields are preserved. `--is-disabled` / `--is-enabled` toggle the row's enabled state. `--change-description` sets the new row version's `changeDescription` audit line (default: auto-generated). `#`-prefixed secrets auto-encrypt before write (fail-closed; since 0.54.0, #378). - `config row-delete --project NAME --component-id ID --config-id ID --row-id ID [--branch ID] [--yes]` -- delete a configuration row. Destructive (gated behind `--allow-destructive`). Branch-aware. Without `--yes` and outside `--json` mode, prompts for interactive confirmation; `--json` mode auto-skips the prompt. - `config oauth-url --project NAME --component-id ID --config-id ID [--redirect-url URL]` -- return the OAuth authorization URL for a component that uses OAuth authentication. **Requires a master Storage API token** (canManageTokens privilege) -- non-master tokens fail with `MISSING_MASTER_TOKEN` exit 3 on a fail-fast pre-flight check before any HTTP write happens. Open the URL in a browser to complete the OAuth flow. +- `config state-get --project NAME --component-id ID --config-id ID [--row-id ID] [--branch ID]` -- (since 0.84.2, #593) read a configuration's runtime `state`. Without `--row-id` returns the root config's state; with `--row-id` returns that row's state (a missing row id fails loudly, it does not silently return `{}`). For row-based components the root state node is unused -- read the row state instead. See [config-state-workflow](references/config-state-workflow.md). +- `config state-set --project NAME --component-id ID --config-id ID [--row-id ID] --state JSON|@file|- [--branch ID] [--dry-run] [--yes]` -- (since 0.84.2, #593) write a configuration's runtime `state` via the dedicated branch-scoped `PUT .../state` endpoint. `--state` must be a JSON object (not an array/scalar) under 4 MB. `--row-id` targets a row's state instead of the root. `--dry-run` previews the current-vs-new diff (same shape as `config update --dry-run`) without writing; a no-op (new state equals current) short-circuits with `changed: false` and no write. Guarded write: prompts for confirmation unless `--yes` or `--json`. This is the fix for `config update --set 'state...'`, which never reached runtime state (see [gotchas](references/gotchas.md)). See [config-state-workflow](references/config-state-workflow.md) for the seed-before-migrate playbook. ## Cross-Project Search - `search QUERY [--project NAME] [--type table|bucket|config|flow|data-app|transformation] [--search-type textual|config-based] [--regex] [--limit N]` -- search for items across one or more projects. **Textual** mode (default, fast) matches item names via the Storage API `global-search` endpoint. **Config-based** mode scans full configuration JSON bodies (slow, complete). `--type` is repeatable; `--limit` applies per project in textual mode (1-100, default 50). `--project` is repeatable for multi-project scope. `--regex` (0.67.0+) is opt-in regex mode: a case-insensitive whole-term match on ENTITY NAMES only (`report` != `monthly_report`; use `.*report.*`), textual-only (error with `--search-type config-based`); regex does NOT match columns. In textual mode, a table matched via a column name carries `matched_columns` (JSON) / a "Matched columns" column. **Both modes match case-insensitively** -- reach for `config search --query` when you need a case-sensitive body scan. diff --git a/plugins/kbagent/skills/kbagent/references/config-state-workflow.md b/plugins/kbagent/skills/kbagent/references/config-state-workflow.md new file mode 100644 index 00000000..4586a420 --- /dev/null +++ b/plugins/kbagent/skills/kbagent/references/config-state-workflow.md @@ -0,0 +1,208 @@ +# Config State Workflow -- Reading and Seeding Runtime State + +`config state-get` / `config state-set` (since v0.84.2, #593) read and write a +configuration's runtime `state` -- the checkpoint dict incremental components +persist between jobs (last sync cursors, `lastImportId`, OAuth intermediate +data). This closes the gap where the only write path was the Keboola UI +(`/raw` -> *Update State* tab), and where `config update --set 'state...'` +looked successful but silently no-oped (see +[gotchas](gotchas.md#config-update---set-state-is-now-a-hard-error-not-a-silent-no-op-since-v0842)). + +## When to use this + +- **Backfill / replay**: reprocess from a chosen checkpoint after a + downstream bug, without re-importing everything. +- **Reset after a bad run**: a component wrote a corrupt checkpoint and keeps + skipping data until state is corrected. +- **Seeding a dev branch**: `branch create` always starts with `state: {}` + (a fresh runtime state, not a copy of production's). Testing incremental + behaviour on a branch at all requires seeding. +- **Migrating `processed_tags`/`query` -> `changed_since: adaptive`** in file + input mapping (Keboola is retiring the former; see the [changelog + announcement](https://changelog.keboola.com/deprecating-processed-tags-and-query-in-file-input-mapping/)). + This is the case that forced #593 -- see the dedicated section below. + +## Quick reference + +| Command | Purpose | Permission | +|---------|---------|------------| +| `config state-get` | Read root or row state | read | +| `config state-set` | Write root or row state (guarded) | write | + +```bash +kbagent --json config state-get --project ALIAS --component-id keboola.python-transformation-v2 --config-id 25344315 +kbagent --json config state-get --project ALIAS --component-id keboola.python-transformation-v2 --config-id 25344315 --row-id row1 + +kbagent --json config state-set --project ALIAS --component-id keboola.python-transformation-v2 --config-id 25344315 \ + --state '{"storage": {"input": {"files": [{"tags": ["import"], "lastImportId": "176200172"}]}}}' \ + --branch 1234 --dry-run # preview first, then re-run without --dry-run +``` + +## Root vs row state + +- **Root state** (`state-get`/`state-set` without `--row-id`) is the + configuration-level checkpoint. This is what `config detail --with-state` + shows as the top-level `state` key. +- **Row state** (`--row-id ROW_ID`) is that row's own checkpoint, embedded in + the row object under `config detail`'s `rows[]` array. +- **For row-based components the root state node is unused** -- if your + configuration has rows (per-table extractors, per-endpoint writers, ...), + read/write the *row* state, not the root. Writing to the root on a + row-based config is a no-op for the component's actual behaviour, even + though the write itself succeeds. +- A missing/typo'd `--row-id` fails loudly (named in the error), it does not + silently return or write an empty `{}` -- that would repeat the exact class + of bug #593 is about. + +## Migration playbook: `processed_tags`/`query` -> `changed_since: adaptive` + +The API-verified trap: `changed_since: adaptive` with an **empty** state does +not mean "start watching from now" -- it triggers a full reload of the +component's entire file history (see +[gotchas](gotchas.md#changed_since-adaptive-with-empty-state-reloads-the-entire-file-history-since-v0842)). +Since a dev branch always starts with `state: {}`, validating this migration +on a branch -- the safe place to test it -- reproduces the full reload every +time unless you seed state first. This is the one manual step (issue #593) +that used to require the Keboola UI; it is now fully scriptable. + +1. **Create an isolated branch** (see + [branch-workflow](branch-workflow.md)): + + ```bash + kbagent --json branch create --project ALIAS --name "migrate-adaptive-25344315" + ``` + + This auto-activates the branch; subsequent commands default to it. Note + its `branch_id` for the explicit `--branch` flags below (state and job + commands accept it explicitly too, which is clearer in scripts). + +2. **Edit the file input mapping** to drop `processed_tags`/`query` and add + `"changedSince": "adaptive"`. Fetch fresh, change only the mapping, push + back (`config update`) or edit locally and `sync push` if the project is + under GitOps sync -- see [sync-workflow](sync-workflow.md) and the + [safe-write-workflow](safe-write-workflow.md) (fetch -> dry-run -> confirm + -> push). + +3. **Find the checkpoint to seed** (see below), then seed the branch's state + *before* the first run: + + ```bash + kbagent --json config state-set --project ALIAS --component-id keboola.python-transformation-v2 \ + --config-id 25344315 --branch \ + --state '{"storage": {"input": {"files": [{"tags": ["import"], "lastImportId": ""}]}}}' \ + --dry-run # verify the diff, then drop --dry-run to apply + ``` + + Seed root or row state depending on whether the config uses rows (see + above). This is the safer direction: an empty state is what triggers the + full reload, a seeded checkpoint is what avoids it. + +4. **Run the job on the branch** and let it complete: + + ```bash + kbagent --json job run --project ALIAS --component-id keboola.python-transformation-v2 \ + --config-id 25344315 --branch --wait + ``` + + Watch the run duration and file count -- a seeded checkpoint should + produce a run comparable to a normal incremental run (seconds, only new + files), not a full-history reload (minutes, thousands of files). + +5. **Verify the state advanced**: + + ```bash + kbagent --json config state-get --project ALIAS --component-id keboola.python-transformation-v2 \ + --config-id 25344315 --branch + ``` + + `lastImportId` (or the equivalent cursor) should have moved past your + seeded checkpoint to the newest file processed. + +6. **Merge when satisfied.** `branch merge` returns a Keboola UI URL for + manual review -- it does not merge automatically: + + ```bash + kbagent --json branch merge --project ALIAS + ``` + + Branch merge propagates **configs only, not state** -- production's state + is untouched by the merge, so repeat the seed step (3) against production + with production's own checkpoint before the first production run under + the new mapping, if production also starts from a cleared/uncertain + state. If production already has a healthy `state` under the old mapping + shape, verify with `state-get` whether it needs reshaping for `adaptive` + before relying on it as-is. + +## Finding the right checkpoint + +`lastImportId` (or your component's equivalent cursor field) should point at +a file the component has already durably processed, so the next run picks up +only what comes after it. Two common sources: + +- **An already-healthy production state**: `config state-get` on the + production config (if one exists under the old mapping) may already carry + a comparable checkpoint you can carry over. +- **The Storage Files listing**: `kbagent --json storage files --project ALIAS --tag TAG --limit N` + lists files with their ids and creation times sorted for inspection -- + pick the id of the most recent file you know is already fully imported + downstream, and seed with that. + +Never guess a future or non-existent id "to be safe" -- an id past the true +checkpoint skips real files; use `--dry-run` and `state-get` to confirm +before committing to a value on a config that matters. + +## State document shape for file input mapping + +The exact shape mirrors the file input mapping's own `tags`/`query` +selection criteria; the state you write should describe the same tag set the +mapping filters on. Confirmed shape (from the API-verified `--set` example in +#593, and matching an `adaptive`-based file input mapping's checkpoint +field): + +```json +{ + "storage": { + "input": { + "files": [ + { + "tags": ["import"], + "lastImportId": "176200172" + } + ] + } + } +} +``` + +If your input mapping selects files by more than one tag set (multiple +entries under `storage.input.files` in the configuration itself), the state +document needs one matching entry per selection. When unsure, `state-get` a +comparable already-migrated config in the same project as ground truth +before writing. + +## When to use `--dry-run` + +Always, the first time, on any config that matters. `--dry-run` computes the +current-vs-new diff without writing (same shape as `config update --dry-run`) +and never prompts. A no-op write (new state identical to current) is +detected and skipped even without `--dry-run` (`changed: false`, no API +call) -- so re-running the seed step is safe and idempotent. + +`state-set` without `--dry-run` is a guarded write: it prompts for +interactive confirmation unless `--yes` is passed or the command runs under +`--json` (consistent with the rest of the `config` group -- `--json` skips +the prompt without requiring `--yes`, since this is a `write`, not +`destructive`, operation). + +## Anti-patterns + +- **`config update --set 'state.x=y'`** -- rejected since v0.84.2 (exit 2); + use `state-set` instead. See [gotchas](gotchas.md) for the full history. +- **Seeding with an empty `{}`** to "reset and be safe" on an `adaptive` + file input mapping -- this is the *expensive* direction for this specific + mapping type (full reload), the inverse of the usual "clear state = safe + reset" assumption for other incremental strategies. +- **Merging a branch and assuming production's state came along** -- branch + merge propagates configs, not runtime state; seed production separately. +- **Writing root state on a row-based component** -- succeeds but has no + effect on the component's actual incremental behaviour; use `--row-id`. diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 867e720b..45747352 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -3600,3 +3600,77 @@ Four things a coding agent will otherwise get wrong: does not accept a Storage API token -- it is the still-open primary ask of issue #594. Do not imply this command covers billing/invoice history; tell the user it is out of reach from the CLI today. +## `config update --set 'state...'` is now a hard error, not a silent no-op (since v0.84.2) + +Before v0.84.2, `--set` on `config update` / `config row-update` applied every +path to `configuration` unconditionally. `state` is a **sibling** of +`configuration` in the Storage API response (see the `config detail +--with-state` entry above), not a child of it -- so +`--set 'state.storage.input.files[0].lastImportId=176200172'` silently wrote +to `configuration.state.storage...` instead. The command exited 0, bumped the +config version, and even showed a plausible-looking `--dry-run` diff, while +the actual runtime `state` was completely untouched (#593). A second, +independent bug compounded it: `set_nested_value` (`json_utils.py`) splits +paths on `.` only, so the `files[0]` segment became a literal dict key +`"files[0]"` rather than list index `0` -- the resulting structure wasn't +even shaped like a real state document. + +- **Since v0.84.2**, any `--set PATH=VALUE` whose first dot-separated segment + matches a known top-level field of the config detail response (`state`, + `rows`, `name`, `description`, `id`, `version`, `currentVersion`, + `changeDescription`, `created`, `creatorToken`, `isDeleted`, `isDisabled`) + is **rejected before any network call**, exit code 2 (usage error). This + fires under `--dry-run` too -- there is no way to preview your way past it. +- The error names the offending path and its first segment, and routes you to + the right tool: `state.*` -> `kbagent config state-set`; `name` -> `--name`; + `description` -> `--description`; `isDisabled` on a row -> `config + row-update --is-disabled/--is-enabled` (the config-level flag is simply not + settable via `--set`); anything else -> "not settable via --set". If a + component genuinely has a `configuration.` key with one of these + names, pass the full body via `--configuration JSON|@file|-` instead of + `--set`. +- **A plain `--set 'parameters.x=y'` is unaffected** -- only paths whose + *first* segment collides with a sibling field are rejected; the guard does + not touch anything under `configuration`. +- **`files[0]` bracket syntax is now also rejected** with a message pointing + at the `files.0` form (which already worked, and still does, over an + existing list). It no longer silently creates a literal `"files[0]"` key. +- If you hit the new exit-2 error on a script written before v0.84.2 and the + intent really was to edit runtime state, switch to `kbagent config + state-set --state ...` (see [config-state-workflow](config-state-workflow.md)) + -- do not work around the guard by nesting the path differently. + +## `changed_since: adaptive` with empty state reloads the ENTIRE file history (since v0.84.2) + +This is a platform behaviour, not a kbagent bug, but it is the trap that +motivated `config state-set` (#593) and it is easy to hit by accident on a +dev branch. Keboola is retiring `processed_tags` / `query` in file input +mapping in favor of `changed_since: adaptive`, which tracks a +`lastImportId`-style checkpoint in the configuration's runtime `state` +(see the `config detail --with-state` entry above for what `state` is). + +- **`adaptive` with an empty `state` does not mean "start watching from + now"** -- it means "there is no checkpoint", so the component downloads + the component's entire file history from the very beginning. On a real + project this can mean years of files; one reported case was killed after + 118 seconds with thousands of files already downloaded and no end in + sight. +- **A dev branch always starts with `state: {}`** (`branch create` gives you + a fresh runtime state, not a copy of production's). So testing the + `processed_tags`/`query` -> `changed_since: adaptive` migration on a branch + -- exactly the safe place you'd want to validate it before touching + production -- reproduces the full-reload behaviour every time, unless the + branch's state is seeded first. +- **The fix is to seed, not to skip validation**: `kbagent config state-set + --branch --state '{"storage": {"input": {"files": [{"tag": + "...", "lastImportId": }]}}}'` before the first `job run` + on the branch. See [config-state-workflow](config-state-workflow.md) for + the full seed -> run -> verify -> merge sequence and the exact state + document shape for file input mappings. +- **An empty state is more dangerous than a seeded one for this input + mapping type.** For most incremental components an empty/cleared state is + the deliberate "reprocess everything" reset and is the well-understood, + documented behaviour. `changed_since: adaptive` inverts the usual safety + assumption: here the empty state is the expensive, surprising path, and a + seeded checkpoint is the conservative one. Do not assume "no state = safe + default" when adaptive is involved. diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index 36ffc154..76b53584 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -51,6 +51,52 @@ 'projects on one billing identity still cannot answer "which project does this ' 'Stripe invoice belong to" without matching on (date, amount). Issue #594 tracks the ' "ask; `billing credits` is the half that was already implementable.", + "New: `kbagent config state-get`/`state-set` read and write a configuration's " + "runtime `state` (closes #593). kbagent now calls the dedicated Storage API " + "state endpoint it never used before. `state-get --project P --component-id C --config-id ID " + "[--row-id R] [--branch ID]` returns the root state, or a row's state with " + "`--row-id` (a missing row id fails loudly, it does not return an empty dict). " + "`state-set --state JSON|@file|- [--row-id R] [--branch ID] [--dry-run] [--yes]` " + "writes it: the payload must be a JSON object under 4 MB (validated before the " + "round-trip), `--dry-run` previews a current-vs-new diff without writing (same " + "shape as `config update --dry-run`), and a state identical to the current one " + "short-circuits with `changed: false` and no API call. `state-set` is a guarded " + "write -- it prompts for confirmation unless `--yes` or `--json` is passed, " + "matching every other `config` write. The motivation: state controls what an " + "incremental component considers already processed, so backfills, resets after a " + "bad run, and seeding a dev branch (which always starts with `state: {}`) were all " + "reachable only by hand-editing in the Keboola UI. The case that forced this: " + "Keboola is retiring `processed_tags`/`query` in file input mapping in favor of " + "`changed_since: adaptive`, and `adaptive` with an empty state reloads the ENTIRE " + "file history instead of just new files -- on a dev branch that always starts " + "empty, testing the migration safely requires seeding a known `lastImportId` " + "first, which is now scriptable end-to-end (`branch create` -> edit mapping -> " + "`sync push` -> `state-set` seed -> `job run` -> `state-get` verify -> `branch " + "merge`). See the config-state-workflow reference for the full playbook. Row " + "support matters here: for row-based components the root `state` node is unused, " + "so `--row-id` is the primary path, not a garnish. Permissions: " + "`config.state-get` (read), `config.state-set` (write).", + "Fix (#593): `config update --set` now exits 2 when a path targets a top-level API " + "field instead of `configuration`. Previously such a write silently landed in " + "`configuration.state.*` and did nothing. This covers `config row-update --set` " + "too. The rejected first segments are (`state`, `rows`, `name`, " + "`description`, `id`, `version`, `currentVersion`, `changeDescription`, `created`, " + "`creatorToken`, `isDeleted`, `isDisabled`). Before this fix, `--set " + "'state.x=y'` silently wrote to `configuration.state.x` instead of the real " + "`state` field: the command exited 0, bumped the config version, and even showed " + "a plausible-looking `--dry-run` diff, while the runtime state stayed completely " + "untouched -- a write that looked successful at every observable layer while " + "doing nothing. The guard fires before any network call, so `--dry-run` is " + "rejected too. The error names the offending path and routes to the right tool: " + "`state.*` -> `config state-set`, `name` -> `--name`, `description` -> " + "`--description`, a row's `isDisabled` -> `row-update --is-disabled/--is-enabled` " + "(the config-level flag isn't settable via `--set` at all), anything else -> " + '"not settable via --set"; a genuine `configuration.` key can still be ' + "written via `--configuration JSON|@file|-`. A plain `--set 'parameters.x=y'` is " + "unaffected -- only a first segment colliding with a sibling field is rejected. " + "Also fixed: `set_nested_value` no longer silently accepts bracket syntax like " + "`files[0]` as a literal dict key -- it now raises a clear error pointing at the " + "working `files.0` form (unchanged, still the way to index an existing list).", ], "0.84.1": [ "Fix: `kbagent config new --push` schema validation now validates the body's " diff --git a/src/keboola_agent_cli/client/configs.py b/src/keboola_agent_cli/client/configs.py index b5e7283f..002297ef 100644 --- a/src/keboola_agent_cli/client/configs.py +++ b/src/keboola_agent_cli/client/configs.py @@ -216,6 +216,96 @@ def get_config_state( state = body.get("state") return state if isinstance(state, dict) else {} + def update_config_state( + self, + component_id: str, + config_id: str, + state: dict[str, Any], + branch_id: int | None = None, + ) -> dict[str, Any]: + """Overwrite the runtime state dict of a specific configuration. + + Unlike reads (see ``get_config_state``), writes to ``state`` DO have a + standalone Storage API resource: ``PUT .../state``. The asymmetry is + intentional on the API side -- state is embedded read-only in the + config detail response (there is no ``GET .../state``), but mutating + it in place would require callers to round-trip the entire + configuration body (including ``configuration`` and ``rows``) just to + change one field, and would race with concurrent configuration edits. + A dedicated write endpoint lets state be updated atomically and + independently of ``configuration``. + + The request body is genuine JSON (``+ Request (application/json)`` in + the apiary spec), e.g. ``{"state": {"lastId": 123}}`` -- NOT the + form-encoded ``data={"state": json.dumps(state)}`` shape that + ``update_config`` uses for ``configuration``. ``state`` must be a + JSON object and the serialized body is capped at + ``CONFIG_STATE_MAX_BYTES`` by the API; size/type validation is the + service layer's responsibility, not this client method's. + + The branch-scoped URL form is preferred for new code; passing + ``branch_id=None`` falls back to the non-branch (production) prefix, + mirroring every other config method in this mixin. + + Args: + component_id: The component ID (e.g. keboola.ex-db-snowflake). + config_id: The configuration ID. + state: The new state dict to store (replaces the existing state). + branch_id: If set, write state on a specific dev branch. + + Returns: + The full updated configuration detail dict (id, name, version, + changeDescription, configuration, rows, state, currentVersion) -- + NOT a bare state dict. + """ + prefix = f"/v2/storage/branch/{branch_id}" if branch_id else "/v2/storage" + safe_component_id = quote(component_id, safe="") + safe_config_id = quote(config_id, safe="") + response = self._request( + "PUT", + f"{prefix}/components/{safe_component_id}/configs/{safe_config_id}/state", + json={"state": state}, + ) + return response.json() + + def update_config_row_state( + self, + component_id: str, + config_id: str, + row_id: str, + state: dict[str, Any], + branch_id: int | None = None, + ) -> dict[str, Any]: + """Overwrite the runtime state dict of a specific configuration row. + + Row-level sibling of ``update_config_state`` -- see that docstring + for why writes have a dedicated resource while reads are served + inline from the config/row detail. Same JSON body shape + (``{"state": state}``, not form-encoded), same 4 MB cap enforced by + the service layer, same branch-scoped-preferred URL convention. + + Args: + component_id: The component ID (e.g. keboola.ex-db-snowflake). + config_id: The configuration ID. + row_id: The configuration row ID. + state: The new state dict to store (replaces the existing state). + branch_id: If set, write state on a specific dev branch. + + Returns: + The full updated configuration detail dict, same shape as + ``update_config_state`` returns. + """ + prefix = f"/v2/storage/branch/{branch_id}" if branch_id else "/v2/storage" + safe_component_id = quote(component_id, safe="") + safe_config_id = quote(config_id, safe="") + safe_row_id = quote(row_id, safe="") + response = self._request( + "PUT", + f"{prefix}/components/{safe_component_id}/configs/{safe_config_id}/rows/{safe_row_id}/state", + json={"state": state}, + ) + return response.json() + def list_config_folder_metadata(self, branch_id: int) -> dict[str, str]: """Fetch folder names for all configurations via metadata search. diff --git a/src/keboola_agent_cli/commands/config.py b/src/keboola_agent_cli/commands/config.py index 18651ea2..0a9ca477 100644 --- a/src/keboola_agent_cli/commands/config.py +++ b/src/keboola_agent_cli/commands/config.py @@ -19,6 +19,7 @@ from ..constants import KEBOOLA_DIR_NAME, MANIFEST_FILENAME, VALID_COMPONENT_TYPES from ..errors import ConfigError, ErrorCode, KeboolaApiError from ..output import format_config_detail, format_configs_table, format_search_results +from ..services.config_service import validate_set_paths from ._helpers import ( check_cli_permission, emit_project_warnings, @@ -597,6 +598,33 @@ def _parse_set_value(raw: str) -> object: return raw +def _handle_config_service_error(formatter: Any, exc: ConfigError | KeboolaApiError) -> None: + """Shared ``ConfigError``/``KeboolaApiError`` -> exit-code mapping. + + Nearly every command in this group ends its try/except with this exact + dispatch; factored out so each call site is 2 lines instead of ~10 (also + used by ``commands/config_state.py``, issue #593). + """ + if isinstance(exc, ConfigError): + formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) + raise typer.Exit(code=5) from None + formatter.error(message=exc.message, error_code=exc.error_code, retryable=exc.retryable) + raise typer.Exit(code=map_error_to_exit_code(exc)) from None + + +def _guard_set_paths(formatter: Any, parsed_sets: list[tuple[str, object]] | None) -> None: + """Enforce the ``--set`` sibling-path guard, exiting 2 on violation. + + ``validate_set_paths`` is a no-op for ``None``/empty, so callers can + invoke this unconditionally (issue #593 Part A). + """ + try: + validate_set_paths(parsed_sets) + except KeboolaApiError as exc: + formatter.error(message=exc.message, error_code=exc.error_code) + raise typer.Exit(code=2) from None + + @config_app.command("update", rich_help_panel="Lifecycle") def config_update( ctx: typer.Context, @@ -748,6 +776,8 @@ def config_update( path, _, raw_value = item.partition("=") parsed_sets.append((path.strip(), _parse_set_value(raw_value.strip()))) + _guard_set_paths(formatter, parsed_sets) + # --set implies merge effective_merge = merge or bool(parsed_sets) @@ -766,16 +796,8 @@ def config_update( branch_id=branch, allow_plaintext_fallback=allow_plaintext, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error( - message=exc.message, - error_code=exc.error_code, - retryable=exc.retryable, - ) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) # --- Output --------------------------------------------------------------- normalizations = result.get("normalizations") or [] @@ -942,16 +964,8 @@ def config_set_default_bucket( dry_run=dry_run, branch_id=branch, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error( - message=exc.message, - error_code=exc.error_code, - retryable=exc.retryable, - ) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) if result.get("dry_run"): changes = result.get("changes", []) @@ -1057,16 +1071,8 @@ def config_rename( branch_id=branch, directory=effective_directory, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error( - message=exc.message, - error_code=exc.error_code, - retryable=exc.retryable, - ) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) if formatter.json_mode: formatter.output(result) @@ -1126,16 +1132,8 @@ def config_delete( config_id=config_id, branch_id=branch, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error( - message=exc.message, - error_code=exc.error_code, - retryable=exc.retryable, - ) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) if formatter.json_mode: formatter.output(result) @@ -1566,12 +1564,8 @@ def config_metadata_list( config_id=config_id, branch_id=effective_branch, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error(message=exc.message, error_code=exc.error_code, retryable=exc.retryable) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) if formatter.json_mode: formatter.output(result) @@ -1613,12 +1607,8 @@ def config_get_metadata( key=key, branch_id=effective_branch, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error(message=exc.message, error_code=exc.error_code, retryable=exc.retryable) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) formatter.output(result, lambda c, d: c.print(d["value"])) @@ -1648,12 +1638,8 @@ def config_set_metadata( value=value, branch_id=effective_branch, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error(message=exc.message, error_code=exc.error_code, retryable=exc.retryable) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) formatter.output( result, lambda c, d: c.print(f"[bold green]Success:[/bold green] {d['message']}") ) @@ -1693,12 +1679,8 @@ def config_delete_metadata( metadata_id=metadata_id, branch_id=effective_branch, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error(message=exc.message, error_code=exc.error_code, retryable=exc.retryable) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) formatter.output( result, lambda c, d: c.print(f"[bold green]Success:[/bold green] {d['message']}") ) @@ -1732,12 +1714,8 @@ def config_set_folder( folder_name=name, branch_id=effective_branch, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error(message=exc.message, error_code=exc.error_code, retryable=exc.retryable) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) formatter.output( result, lambda c, d: c.print(f"[bold green]Success:[/bold green] {d['message']}") ) @@ -2195,16 +2173,8 @@ def config_row_create( branch_id=branch, allow_plaintext_fallback=allow_plaintext, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error( - message=exc.message, - error_code=exc.error_code, - retryable=exc.retryable, - ) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) if formatter.json_mode: formatter.output(result) @@ -2377,6 +2347,8 @@ def config_row_update( path, _, raw_value = item.partition("=") parsed_sets.append((path.strip(), _parse_set_value(raw_value.strip()))) + _guard_set_paths(formatter, parsed_sets) + effective_merge = merge or bool(parsed_sets) try: @@ -2396,16 +2368,8 @@ def config_row_update( branch_id=branch, allow_plaintext_fallback=allow_plaintext, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error( - message=exc.message, - error_code=exc.error_code, - retryable=exc.retryable, - ) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) if result.get("dry_run"): changes = result.get("changes", []) @@ -2480,16 +2444,8 @@ def config_row_delete( row_id=row_id, branch_id=branch, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error( - message=exc.message, - error_code=exc.error_code, - retryable=exc.retryable, - ) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) if formatter.json_mode: formatter.output(result) @@ -2558,16 +2514,8 @@ def config_oauth_url( config_id=config_id, redirect_url=redirect_url, ) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) - raise typer.Exit(code=5) from None - except KeboolaApiError as exc: - formatter.error( - message=exc.message, - error_code=exc.error_code, - retryable=exc.retryable, - ) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) if formatter.json_mode: formatter.output(result) @@ -2578,3 +2526,9 @@ def config_oauth_url( ) formatter.console.print(f" [link]{result['url']}[/link]") formatter.console.print("\n[dim]Open this URL in a browser and grant access.[/dim]") + + +# `config state-get` / `config state-set` live in config_state.py (file-size-budget +# split, see that module's docstring) but register on THIS module's `config_app`. +# Import for the side effect only -- nothing here references the module by name. +from . import config_state as _config_state # noqa: E402,F401 diff --git a/src/keboola_agent_cli/commands/config_state.py b/src/keboola_agent_cli/commands/config_state.py new file mode 100644 index 00000000..5193a5ae --- /dev/null +++ b/src/keboola_agent_cli/commands/config_state.py @@ -0,0 +1,199 @@ +"""``config state-get`` / ``config state-set`` -- runtime state read/write (issue #593). + +Split out of ``commands/config.py`` (rather than appended there) purely for +file-size-budget reasons: ``commands/config.py`` is grandfathered at its +CONTRIBUTING.md hard ceiling (``scripts/file_size_baseline.json``) and may +only shrink, not grow (see ``scripts/check_file_size.py``). These two +commands are registered on the SAME ``config_app`` Typer instance imported +from ``.config``, so they behave identically to a command defined there -- +this file is imported once, at the bottom of ``commands/config.py``, purely +for its module-level ``@config_app.command(...)`` side effect. + +Thin CLI layer only: parses arguments, calls ``ConfigService``, formats +output. No business logic belongs here (see ``services/config_service.py`` +for ``get_config_state`` / ``set_config_state``). +""" + +import json +from typing import Any + +import typer +from rich.markup import escape +from rich.syntax import Syntax + +from ..config_store import ConfigStore +from ..errors import ConfigError, ErrorCode, KeboolaApiError +from ._helpers import get_formatter, get_service, resolve_branch +from .config import _handle_config_service_error, _parse_json_input, config_app + + +@config_app.command("state-get", rich_help_panel="State") +def config_state_get( + ctx: typer.Context, + project: str = typer.Option(..., "--project", help="Project alias"), + component_id: str = typer.Option(..., "--component-id", help="Component ID"), + config_id: str = typer.Option(..., "--config-id", help="Configuration ID"), + row_id: str | None = typer.Option( + None, "--row-id", help="Read this row's state instead of the config's root state" + ), + branch: int | None = typer.Option( + None, "--branch", help="Dev branch ID (defaults to active branch)" + ), +) -> None: + """Read the runtime ``state`` dict of a configuration or one of its rows. + + Storage API serves ``state`` inline in the config detail response only -- + there is no standalone ``GET .../state`` endpoint. When a config uses + rows, the root state is typically unused; pass --row-id to read a + row's own state. + + \b + Examples: + kbagent config state-get --project P --component-id C --config-id ID + kbagent config state-get --project P --component-id C --config-id ID --row-id ROW + """ + formatter = get_formatter(ctx) + config_store: ConfigStore = ctx.obj["config_store"] + _, effective_branch = resolve_branch(config_store, formatter, project, branch) + service = get_service(ctx, "config_service") + + try: + result = service.get_config_state( + alias=project, + component_id=component_id, + config_id=config_id, + row_id=row_id, + branch_id=effective_branch, + ) + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) + + if formatter.json_mode: + formatter.output(result) + return + _format_state_get(formatter, result) + + +@config_app.command("state-set", rich_help_panel="State") +def config_state_set( + ctx: typer.Context, + project: str = typer.Option(..., "--project", help="Project alias"), + component_id: str = typer.Option(..., "--component-id", help="Component ID"), + config_id: str = typer.Option(..., "--config-id", help="Configuration ID"), + state: str = typer.Option( + ..., "--state", help="New state as inline JSON object, @file, or - for stdin" + ), + row_id: str | None = typer.Option( + None, "--row-id", help="Write this row's state instead of the config's root state" + ), + branch: int | None = typer.Option( + None, "--branch", help="Dev branch ID (defaults to active branch)" + ), + dry_run: bool = typer.Option( + False, "--dry-run", help="Show what would change without applying" + ), + yes: bool = typer.Option(False, "--yes", "-y", help="Skip confirmation prompt"), +) -> None: + """Overwrite the runtime ``state`` dict of a configuration or one of its rows. + + This replaces the ENTIRE state object -- it is not a merge. ``--state`` + must be a JSON object under the API's 4 MB cap. + + \b + Examples: + kbagent config state-set --project P --component-id C --config-id ID \\ + --state '{"lastId": 123}' + kbagent config state-set --project P --component-id C --config-id ID \\ + --row-id ROW --state @state.json + kbagent config state-set --project P --component-id C --config-id ID \\ + --state '{"lastId": 123}' --dry-run + """ + formatter = get_formatter(ctx) + config_store: ConfigStore = ctx.obj["config_store"] + + try: + state_value = _parse_json_input(state) + except (json.JSONDecodeError, FileNotFoundError) as exc: + formatter.error( + message=f"Invalid --state input: {exc}", error_code=ErrorCode.VALIDATION_ERROR + ) + raise typer.Exit(code=2) from None + + _, effective_branch = resolve_branch(config_store, formatter, project, branch) + + if not dry_run and not yes and not formatter.json_mode: + target = f"{component_id}/{config_id}" + (f" row [{row_id}]" if row_id else "") + if not typer.confirm(f"Overwrite state for {target}?"): + formatter.console.print("Aborted.") + raise typer.Exit(code=0) + + service = get_service(ctx, "config_service") + + try: + result = service.set_config_state( + alias=project, + component_id=component_id, + config_id=config_id, + state=state_value, + row_id=row_id, + branch_id=effective_branch, + dry_run=dry_run, + ) + except (ConfigError, KeboolaApiError) as exc: + _handle_config_service_error(formatter, exc) + + if formatter.json_mode: + formatter.output(result) + return + if result.get("dry_run"): + _format_state_dry_run(formatter, result) + else: + _format_state_set(formatter, result) + + +def _format_state_target(result: dict) -> str: + """Build a ``component_id/config_id[ row [row_id]]`` label for human output.""" + target = ( + f"[cyan]{escape(result['component_id'])}[/cyan]/[cyan]{escape(result['config_id'])}[/cyan]" + ) + if result.get("row_id"): + target += f" row [[yellow]{escape(str(result['row_id']))}[/yellow]]" + return target + + +def _format_state_get(formatter: Any, result: dict) -> None: + formatter.console.print(f"[bold]State for[/bold] {_format_state_target(result)}:\n") + state = result.get("state") or {} + if not state: + formatter.console.print(" [dim](empty state)[/dim]") + return + formatter.console.print( + Syntax(json.dumps(state, indent=2, ensure_ascii=False), "json", theme="monokai") + ) + + +def _format_state_set(formatter: Any, result: dict) -> None: + branch_info = f" (branch {result['branch_id']})" if result.get("branch_id") else "" + if not result.get("changed", True): + formatter.console.print( + f"[yellow]No change:[/yellow] state for {_format_state_target(result)} " + f"already matches{branch_info}." + ) + return + formatter.success(f"Updated state for {_format_state_target(result)}{branch_info}") + state = result.get("state") or {} + if state: + formatter.console.print( + Syntax(json.dumps(state, indent=2, ensure_ascii=False), "json", theme="monokai") + ) + + +def _format_state_dry_run(formatter: Any, result: dict) -> None: + changes = result.get("changes", []) + if not changes: + formatter.success("No changes detected.") + return + formatter.console.print(f"\n[bold]Dry-run: {len(changes)} change(s)[/bold]\n") + for change in changes: + formatter.console.print(f" {change}") + formatter.console.print() diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index 6125cded..ee5e2646 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -471,6 +471,23 @@ Return the OAuth authorization URL for a component that uses OAuth authentication. Open the URL in a browser to complete the OAuth flow. + kbagent config state-get --project NAME --component-id ID --config-id ID [--row-id ID] [--branch ID] + Read a configuration's runtime state (the same dict --with-state attaches + to config detail). Without --row-id returns the root config's state; with + --row-id returns that row's state. For row-based components the root + state is unused -- read/write the row state instead. + + kbagent config state-set --project NAME --component-id ID --config-id ID [--row-id ID] --state JSON|@file|- [--branch ID] [--dry-run] [--yes] + Write a configuration's runtime state via the dedicated PUT .../state + endpoint (branch-scoped). --state must be a JSON object under 4 MB. + --row-id targets a row's state instead of the root. --dry-run previews + the current-vs-new diff without writing; no-op (changed=false) when the + new state equals the current one. Guarded write: prompts for + confirmation unless --yes or --json. Use this to seed/reset/backfill + state (e.g. seeding a dev branch's lastImportId before testing + changed_since: adaptive) -- `config update --set 'state...'` does NOT + reach this endpoint (see gotchas). + ### Cross-Project Search kbagent search QUERY [--project NAME] [--type table|bucket|config|flow|data-app|transformation] [--search-type textual|config-based] [--regex] [--limit N] diff --git a/src/keboola_agent_cli/constants.py b/src/keboola_agent_cli/constants.py index f91704f0..be5e8c2b 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -645,6 +645,51 @@ def _resolve_app_name() -> str: DIFF_MAX_LINES: int = 20 # max number of diff detail lines per config change ENCRYPTED_PLACEHOLDER: str = "" # placeholder for encrypted values during comparison +# --- config update / config row-update --set sibling-path guard (issue #593 Part A) --- +# `--set PATH=VALUE` is documented as editing `configuration.*` only. These +# are the OTHER top-level keys of the Storage API config-detail response +# (`config update`) and config-row response (`config row-update`) -- writing +# through `--set ...=` where is one of these used to +# silently land inside `configuration.` instead of touching the real +# field (e.g. `--set 'state.x=y'` created `configuration.state.x`, leaving +# the actual runtime `state` untouched). See `ConfigService.validate_set_paths`. +CONFIG_SET_GUARDED_PREFIXES: frozenset[str] = frozenset( + { + "state", + "rows", + "name", + "description", + "id", + "version", + "currentVersion", + "changeDescription", + "created", + "creatorToken", + "isDeleted", + "isDisabled", + } +) + +# Per-prefix guidance for the sibling-path guard error message: which real +# tool/flag to use instead of `--set ...`. A prefix absent from this +# map falls back to a generic "not settable via --set" message. +CONFIG_SET_GUARD_HINTS: dict[str, str] = { + "state": "`kbagent config state-set`", + "name": "the `--name` option", + "description": "the `--description` option", + "isDisabled": ( + "`config row-update --is-disabled/--is-enabled` (for a row) -- the " + "config-level isDisabled flag is not settable at all via --set" + ), +} + +# --- Config State (PUT .../state, issue #593) --- +# Storage API caps the serialized `state` request body at 4 MB. Enforced by +# the service layer (ConfigService.set_config_state) before the round-trip -- +# the client layer (update_config_state / update_config_row_state) sends the +# body as-is and relies on the caller to have already validated it. +CONFIG_STATE_MAX_BYTES: int = 4 * 1024 * 1024 + # --- Programmatic Auth (browser login: PKCE + device authorization) --- # Keboola Connection issues a "programmatic session": a short-lived access token # (kbc_at_*) plus a rotating refresh token (kbc_rt_*), used as diff --git a/src/keboola_agent_cli/json_utils.py b/src/keboola_agent_cli/json_utils.py index 3924c22a..1b4b39cf 100644 --- a/src/keboola_agent_cli/json_utils.py +++ b/src/keboola_agent_cli/json_utils.py @@ -52,8 +52,26 @@ def set_nested_value(obj: dict[str, Any], path: str, value: Any) -> dict[str, An Returns a deep-copied dict with the value set — *obj* is not mutated. Supports integer segments for list indexing on **existing** lists - (new intermediate containers are always dicts). + (new intermediate containers are always dicts) using dot-separated + integers, e.g. ``"files.0.name"`` -- NOT bracket syntax like + ``"files[0].name"``. Bracket syntax raises ``ValueError`` instead of + being silently accepted: without this check, a path like ``"files[0]"`` + (no ``.`` in it) would pass straight through as a single segment and + create a literal ``"files[0]"`` dict key instead of indexing into the + list -- the exact silent-corruption bug this guard closes (issue #593). + + Raises: + ValueError: If *path* contains ``[`` or ``]`` (bracket syntax). + KeyError: If a segment cannot be traversed/set on the current + container (e.g. an int segment against a dict, or a dict + segment against a list). """ + if "[" in path or "]" in path: + raise ValueError( + f"Invalid path {path!r}: bracket syntax like 'files[0]' is not " + "supported. Use dot-separated integer segments instead, e.g. " + "'files.0'." + ) result = copy.deepcopy(obj) segments = path.split(".") current: Any = result diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index a74c3fc8..57881635 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -86,6 +86,8 @@ "config.row-update": "write", "config.row-delete": "destructive", "config.oauth-url": "read", + "config.state-get": "read", + "config.state-set": "write", # Job history "job.list": "read", "job.detail": "read", diff --git a/src/keboola_agent_cli/server/routers/configs.py b/src/keboola_agent_cli/server/routers/configs.py index 157a0d9f..ab458f13 100644 --- a/src/keboola_agent_cli/server/routers/configs.py +++ b/src/keboola_agent_cli/server/routers/configs.py @@ -65,6 +65,13 @@ class MetadataSet(BaseModel): value: str +class ConfigStateUpdate(BaseModel): + state: dict[str, Any] + row_id: str | None = None + branch_id: int | None = None + dry_run: bool = False + + @router.get("", summary="List component configurations") def list_configs( project: str | None = Query(None, description="Project alias (None = all)"), @@ -452,6 +459,54 @@ def oauth_url( ) +@router.get( + "/{project}/{component_id}/{config_id}/state", + summary="Get configuration (or row) state", +) +def state_get( + project: str, + component_id: str, + config_id: str, + row_id: str | None = None, + branch_id: int | None = None, + registry: ServiceRegistry = Depends(get_registry), +) -> dict[str, Any]: + """Read a configuration's (or row's) runtime state. Mirrors `kbagent config state-get`.""" + return registry.config.get_config_state( + alias=project, + component_id=component_id, + config_id=config_id, + row_id=row_id, + branch_id=branch_id, + ) + + +@router.put( + "/{project}/{component_id}/{config_id}/state", + summary="Set configuration (or row) state", +) +def state_set( + project: str, + component_id: str, + config_id: str, + body: ConfigStateUpdate, + registry: ServiceRegistry = Depends(get_registry), +) -> dict[str, Any]: + """Set a configuration's (or row's) runtime state. Mirrors `kbagent config state-set`. + + Confirmation is a CLI-only concern -- this route never prompts. + """ + return registry.config.set_config_state( + alias=project, + component_id=component_id, + config_id=config_id, + state=body.state, + row_id=body.row_id, + branch_id=body.branch_id, + dry_run=body.dry_run, + ) + + # ---- Variables (delegated to VariablesService) ---- diff --git a/src/keboola_agent_cli/services/config_service.py b/src/keboola_agent_cli/services/config_service.py index 62304076..2beb88eb 100644 --- a/src/keboola_agent_cli/services/config_service.py +++ b/src/keboola_agent_cli/services/config_service.py @@ -16,6 +16,11 @@ from ..ai_client import AiServiceClient from ..config_store import ConfigStore +from ..constants import ( + CONFIG_SET_GUARD_HINTS, + CONFIG_SET_GUARDED_PREFIXES, + CONFIG_STATE_MAX_BYTES, +) from ..errors import ConfigError, ErrorCode, KeboolaApiError from ..json_utils import compute_diff, deep_merge, set_nested_value from ..models import ComponentDetail, ProjectConfig @@ -87,6 +92,64 @@ def _default_change_description(command: str, *, has_metadata: bool, has_content return f"Updated {' + '.join(parts)} via kbagent {command}" +def validate_set_paths(set_paths: list[tuple[str, Any]] | None) -> None: + """Reject ``--set`` paths whose first segment is an API-level sibling of + ``configuration`` (issue #593 Part A). + + ``config update --set`` and ``config row-update --set`` are documented + as editing ``configuration.*`` only. ``_resolve_configuration`` / + ``_resolve_row_configuration`` used to apply every ``--set`` path + unconditionally onto ``current_detail.get("configuration", {})`` -- + but keys like ``state`` or ``name`` are SIBLINGS of ``configuration`` + in the Storage API config-detail response, not children of it. So + e.g. ``--set 'state.x=y'`` silently created ``configuration.state.x`` + instead of touching the real ``state`` field: exit 0, a bumped + version, a plausible-looking ``--dry-run`` diff, and the runtime + state left untouched. + + Call this BEFORE any network call so the rejection also covers + ``--dry-run`` -- a usage mistake should never be allowed to look like + a successful preview. ``_resolve_configuration`` and + ``_resolve_row_configuration`` both call this as their first line. + + This is a plain, side-effect-free validation function (no CLI/typer + dependency, per the service layer's framework-free contract) so the + command layer can call it directly too, ahead of ``service.update_config`` + / ``service.update_config_row``, and map the raised error to a usage + exit code before anything (including a client) is constructed. + + Args: + set_paths: The parsed ``(path, value)`` pairs from one or more + ``--set PATH=VALUE`` flags, or ``None``/empty if none given. + + Raises: + KeboolaApiError: ``error_code=ErrorCode.INVALID_ARGUMENT`` naming + the offending path, its first segment, and the real tool/flag + to use instead -- callers should map this to a usage-error + exit code (see ``commands/config.py``'s existing ``--set + PATH=VALUE`` format check for the pattern to mirror). + """ + if not set_paths: + return + for path, _value in set_paths: + first_segment = path.split(".", 1)[0] + if first_segment not in CONFIG_SET_GUARDED_PREFIXES: + continue + hint = CONFIG_SET_GUARD_HINTS.get(first_segment, "not settable via --set") + raise KeboolaApiError( + status_code=400, + error_code=ErrorCode.INVALID_ARGUMENT, + message=( + f"--set '{path}' targets '{first_segment}', which is a " + f"top-level API field, not part of 'configuration'. --set " + f"only edits configuration.* -- use {hint} instead. If the " + f"component genuinely has a 'configuration.{first_segment}' " + f"key, pass the full body via --configuration JSON|@file|- " + f"instead." + ), + ) + + def _find_matches_in_json( obj: Any, match_fn: Any, @@ -112,6 +175,16 @@ def _find_matches_in_json( return paths +def _not_found(message: str) -> KeboolaApiError: + """Build a 404 NOT_FOUND error (issue #593 -- shared by ConfigService._extract_state).""" + return KeboolaApiError(status_code=404, error_code=ErrorCode.NOT_FOUND, message=message) + + +def _bad_state(message: str) -> KeboolaApiError: + """Build a 400 VALIDATION_ERROR error (issue #593 -- shared by ConfigService.set_config_state).""" + return KeboolaApiError(status_code=400, error_code=ErrorCode.VALIDATION_ERROR, message=message) + + class ConfigService(BaseService): """Business logic for listing and inspecting Keboola configurations. @@ -836,7 +909,14 @@ def _resolve_configuration( When *merge* is True or *set_paths* are given, the current configuration is fetched from the API and changes are applied on top of it (deep-merge for dicts, replace for scalars/lists). + + Raises: + KeboolaApiError: If *set_paths* targets a top-level API sibling + of ``configuration`` (see :func:`validate_set_paths`). + Raised before ``client.get_config_detail`` so the rejection + also covers ``--dry-run`` -- no network call happens first. """ + validate_set_paths(set_paths) needs_current = merge or bool(set_paths) if needs_current: @@ -998,6 +1078,199 @@ def set_default_bucket( result["default_bucket"] = target return result + @staticmethod + def _extract_state( + detail: dict[str, Any], row_id: str | None, component_id: str, config_id: str + ) -> dict[str, Any]: + """Pull the ``state`` dict for the root config or a specific row out of + a config-detail response (see "Reading row state" in issue #593). + + Storage API has no standalone ``GET .../state`` for either the root + config or a row -- both are served inline in the config detail + response. Root state lives at ``detail["state"]``; row state lives at + ``row["state"]`` for the matching entry of ``detail["rows"]``. + + Args: + detail: A config detail response (from ``get_config_detail``). + row_id: If given, return the named row's state instead of root. + component_id: Used only to build a clear error message. + config_id: Used only to build a clear error message. + + Raises: + KeboolaApiError: ``error_code=ErrorCode.NOT_FOUND`` if *row_id* + is given but no row with that ID exists. A missing row must + fail loudly and name the row -- silently returning ``{}`` for + a typo'd row ID would repeat the exact class of bug issue + #593 fixes for ``--set``. + """ + if row_id is None: + state = detail.get("state") + return state if isinstance(state, dict) else {} + for row in detail.get("rows") or []: + if isinstance(row, dict) and row.get("id") == row_id: + state = row.get("state") + return state if isinstance(state, dict) else {} + raise _not_found(f"Row '{row_id}' not found in configuration {component_id}/{config_id}.") + + def get_config_state( + self, + alias: str, + component_id: str, + config_id: str, + row_id: str | None = None, + branch_id: int | None = None, + ) -> dict[str, Any]: + """Read the runtime ``state`` dict of a config or one of its rows. + + Storage API serves ``state`` inline in the config detail response + only (no standalone ``GET .../state`` for either root or row state -- + see ``client.get_config_state``'s docstring for the production-404 / + branch-scoped-501 behaviour this mirrors). This method always fetches + the full detail and extracts the requested state locally. + + Args: + alias: Project alias. + component_id: Component ID. + config_id: Configuration ID. + row_id: If given, return this row's state instead of the config's + root state. When a config uses rows, the root ``state`` node + is typically unused -- row state is the primary path for + row-based components. + branch_id: Dev branch override; falls back to the project's + active branch when None. + + Returns: + ``{"project_alias", "component_id", "config_id", "row_id", + "branch_id", "state"}``. + + Raises: + KeboolaApiError: ``NOT_FOUND`` if *row_id* is given but does not + exist on the config. + ConfigError: When the alias is unknown. + """ + project = self.resolve_projects([alias])[alias] + effective_branch_id = branch_id or project.active_branch_id + client = self._client_factory(project.stack_url, project.token) + base = { + "project_alias": alias, + "component_id": component_id, + "config_id": config_id, + "row_id": row_id, + "branch_id": effective_branch_id, + } + try: + detail = client.get_config_detail( + component_id, config_id, branch_id=effective_branch_id + ) + state = self._extract_state(detail, row_id, component_id, config_id) + finally: + client.close() + return base | {"state": state} + + def set_config_state( + self, + alias: str, + component_id: str, + config_id: str, + state: dict[str, Any], + row_id: str | None = None, + branch_id: int | None = None, + dry_run: bool = False, + ) -> dict[str, Any]: + """Overwrite the runtime ``state`` dict of a config or one of its rows. + + Read-modify-write for the diff/no-op semantics: fetches the current + config detail, extracts the current state (root or row, see + ``_extract_state``), and PUTs the new state via the dedicated + ``update_config_state`` / ``update_config_row_state`` write endpoint + (see those docstrings for why writes have a standalone resource while + reads are served inline only). + + Args: + alias: Project alias. + component_id: Component ID. + config_id: Configuration ID. + state: The new state dict. Must be a JSON object under + ``CONFIG_STATE_MAX_BYTES`` when serialized -- validated + locally before any network call so a bad payload never looks + like a successful ``--dry-run`` preview. + row_id: If given, write this row's state instead of the config's + root state. + branch_id: Dev branch override; falls back to the project's + active branch when None. + dry_run: If True, return a diff without writing. + + Returns: + On a real write: ``{"project_alias", "component_id", "config_id", + "row_id", "branch_id", "state", "changed": True}``. + On a no-op (new state == current state): same shape with + ``"changed": False`` -- no API write. + On dry_run: ``{"dry_run": True, "project_alias", "component_id", + "config_id", "row_id", "branch_id", "changes", "old_state", + "new_state"}`` mirroring ``set_default_bucket``'s dry-run shape. + + Raises: + KeboolaApiError: ``VALIDATION_ERROR`` if *state* is not a JSON + object or exceeds the size cap; ``NOT_FOUND`` if *row_id* is + given but does not exist; underlying API errors otherwise. + ConfigError: When the alias is unknown. + """ + if not isinstance(state, dict): + raise _bad_state(f"--state must be a JSON object, got {type(state).__name__}.") + size = len(json.dumps(state).encode("utf-8")) + if size >= CONFIG_STATE_MAX_BYTES: + raise _bad_state(f"--state is {size} bytes; cap is {CONFIG_STATE_MAX_BYTES}.") + + project = self.resolve_projects([alias])[alias] + effective_branch_id = branch_id or project.active_branch_id + client = self._client_factory(project.stack_url, project.token) + base = { + "project_alias": alias, + "component_id": component_id, + "config_id": config_id, + "row_id": row_id, + "branch_id": effective_branch_id, + } + try: + detail = client.get_config_detail( + component_id, config_id, branch_id=effective_branch_id + ) + current_state = self._extract_state(detail, row_id, component_id, config_id) + + # Semantic no-op: short-circuit before any write. + if current_state == state: + return base | {"state": current_state, "changed": False} + + if dry_run: + return base | { + "dry_run": True, + "changes": compute_diff(current_state, state), + "old_state": current_state, + "new_state": state, + } + + if row_id is not None: + result = client.update_config_row_state( + component_id, config_id, row_id, state, branch_id=effective_branch_id + ) + else: + result = client.update_config_state( + component_id, config_id, state, branch_id=effective_branch_id + ) + finally: + client.close() + + # Both PUTs carry the updated state at the response's TOP level, but in + # different envelopes: the root endpoint answers with the full config + # detail, the row endpoint with the bare row object (no "rows" key at + # all). So read it with row_id=None in both cases -- passing row_id here + # would search a rows[] array the row response never has, which reported + # a false NOT_FOUND on every row write even though it had landed. Caught + # by the live E2E run against project 5946; mock-based tests could not + # see it, since the mocks returned the assumed shape, not the real one. + new_state = self._extract_state(result, None, component_id, config_id) + return base | {"state": new_state, "changed": True} + def delete_config( self, alias: str, @@ -2030,7 +2303,14 @@ def _resolve_row_configuration( """Build the final row configuration dict by merging/setting paths. Mirrors ``_resolve_configuration`` but operates on a row's config. + + Raises: + KeboolaApiError: If *set_paths* targets a top-level API sibling + of ``configuration`` (see :func:`validate_set_paths`). + Raised before ``client.get_config_row`` so the rejection + also covers ``--dry-run`` -- no network call happens first. """ + validate_set_paths(set_paths) needs_current = merge or bool(set_paths) if needs_current: diff --git a/tests/test_config_set_guard.py b/tests/test_config_set_guard.py new file mode 100644 index 00000000..80ecd193 --- /dev/null +++ b/tests/test_config_set_guard.py @@ -0,0 +1,504 @@ +"""Tests for the `config update --set` / `config row-update --set` sibling-path +guard (issue #593 Part A) and for `set_nested_value`'s bracket-syntax rejection. + +Covers: +* `validate_set_paths` (pure function) rejects every guarded prefix with the + right routing hint. +* `ConfigService.update_config` / `update_config_row` invoke the guard + BEFORE any client/network call -- including with `dry_run=True`. +* A normal `--set 'parameters.x=y'` (and a nested `parameters.state.x` path, + where `state` is not the first segment) still works. +* `set_nested_value` raises a clear error for bracket syntax (`files[0]`) + instead of silently creating a literal `"files[0]"` key, while the existing + `files.0` dotted-integer form keeps working. +""" + +from pathlib import Path +from unittest.mock import MagicMock + +import pytest +from typer.testing import CliRunner, Result + +from helpers import setup_single_project +from keboola_agent_cli.cli import app +from keboola_agent_cli.constants import CONFIG_SET_GUARDED_PREFIXES +from keboola_agent_cli.errors import ErrorCode, KeboolaApiError +from keboola_agent_cli.json_utils import set_nested_value +from keboola_agent_cli.services.config_service import ConfigService, validate_set_paths + +runner = CliRunner() + +# --------------------------------------------------------------------------- +# Fixtures / helpers +# --------------------------------------------------------------------------- + + +def _config_detail(configuration: dict | None = None) -> dict: + return { + "id": "cfg-001", + "name": "My Config", + "description": "desc", + "version": 3, + "configuration": configuration if configuration is not None else {"parameters": {"a": 1}}, + } + + +def _row_detail(configuration: dict | None = None) -> dict: + return { + "id": "row-001", + "name": "My Row", + "description": "desc", + "isDisabled": False, + "configuration": configuration if configuration is not None else {"parameters": {"a": 1}}, + } + + +def _make_service(tmp_config_dir: Path) -> tuple[ConfigService, MagicMock]: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _config_detail() + mock_client.get_config_row.return_value = _row_detail() + mock_client.update_config.return_value = {"id": "cfg-001", "name": "My Config"} + mock_client.update_config_row.return_value = {"id": "row-001", "name": "My Row"} + service = ConfigService( + config_store=store, + client_factory=lambda url, token: mock_client, + ) + return service, mock_client + + +# --------------------------------------------------------------------------- +# validate_set_paths (pure function) -- one case per guarded prefix +# --------------------------------------------------------------------------- + + +class TestValidateSetPathsGuardedPrefixes: + """Every guarded prefix must be rejected, with the right routing hint.""" + + @pytest.mark.parametrize("prefix", sorted(CONFIG_SET_GUARDED_PREFIXES)) + def test_every_guarded_prefix_is_rejected(self, prefix: str) -> None: + with pytest.raises(KeboolaApiError) as exc_info: + validate_set_paths([(f"{prefix}.sub", "value")]) + assert exc_info.value.error_code == ErrorCode.INVALID_ARGUMENT + # Names the offending path and its first segment. + assert f"{prefix}.sub" in exc_info.value.message + assert f"'{prefix}'" in exc_info.value.message + # Explains --set only edits configuration.*. + assert "configuration.*" in exc_info.value.message + # Escape hatch mentioned. + assert "--configuration" in exc_info.value.message + + @pytest.mark.parametrize("prefix", sorted(CONFIG_SET_GUARDED_PREFIXES)) + def test_bare_prefix_without_dot_is_also_rejected(self, prefix: str) -> None: + """A path that IS the guarded key (no further segments) is rejected too.""" + with pytest.raises(KeboolaApiError): + validate_set_paths([(prefix, "value")]) + + def test_state_hints_at_state_set(self) -> None: + with pytest.raises(KeboolaApiError, match="config state-set"): + validate_set_paths([("state.lastId", 123)]) + + def test_name_hints_at_name_flag(self) -> None: + with pytest.raises(KeboolaApiError, match="--name"): + validate_set_paths([("name", "New Name")]) + + def test_description_hints_at_description_flag(self) -> None: + with pytest.raises(KeboolaApiError, match="--description"): + validate_set_paths([("description", "New description")]) + + def test_is_disabled_hints_at_row_update_flags(self) -> None: + with pytest.raises(KeboolaApiError) as exc_info: + validate_set_paths([("isDisabled", True)]) + message = exc_info.value.message + assert "--is-disabled" in message + assert "--is-enabled" in message + + def test_unmapped_prefix_falls_back_to_generic_message(self) -> None: + with pytest.raises(KeboolaApiError, match="not settable via --set"): + validate_set_paths([("creatorToken", "x")]) + + +class TestValidateSetPathsRegression: + """Non-guarded paths must pass through untouched.""" + + def test_none_is_a_noop(self) -> None: + validate_set_paths(None) # no raise + + def test_empty_list_is_a_noop(self) -> None: + validate_set_paths([]) # no raise + + def test_normal_parameters_path_passes(self) -> None: + validate_set_paths([("parameters.db.host", "new-host")]) # no raise + + def test_nested_state_segment_is_not_guarded(self) -> None: + """The guard only inspects the FIRST segment -- `state` deeper in the + path (e.g. a component that legitimately has configuration.parameters.state) + must not be blocked.""" + validate_set_paths([("parameters.state.x", "y")]) # no raise + + def test_multiple_paths_one_bad_one_good(self) -> None: + with pytest.raises(KeboolaApiError): + validate_set_paths( + [ + ("parameters.x", "y"), + ("state.lastId", 1), + ] + ) + + +# --------------------------------------------------------------------------- +# ConfigService.update_config -- guard fires before any client call +# --------------------------------------------------------------------------- + + +class TestUpdateConfigGuard: + def test_guarded_set_blocks_before_get_config_detail(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir) + + with pytest.raises(KeboolaApiError) as exc_info: + service.update_config( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + set_paths=[("state.lastId", 123)], + ) + + assert exc_info.value.error_code == ErrorCode.INVALID_ARGUMENT + client.get_config_detail.assert_not_called() + client.update_config.assert_not_called() + + def test_guarded_set_blocks_with_dry_run(self, tmp_config_dir: Path) -> None: + """--dry-run must fail too -- no network call, no plausible diff.""" + service, client = _make_service(tmp_config_dir) + + with pytest.raises(KeboolaApiError): + service.update_config( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + set_paths=[("name", "New Name")], + dry_run=True, + ) + + client.get_config_detail.assert_not_called() + client.update_config.assert_not_called() + + def test_normal_set_still_works(self, tmp_config_dir: Path) -> None: + """Regression: a plain configuration.* --set is unaffected.""" + service, client = _make_service(tmp_config_dir) + + service.update_config( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + set_paths=[("parameters.x", "y")], + ) + + client.get_config_detail.assert_called_once() + client.update_config.assert_called_once() + written = client.update_config.call_args.kwargs["configuration"] + assert written["parameters"]["x"] == "y" + # Original sibling keys under parameters survive (read-modify-write). + assert written["parameters"]["a"] == 1 + + def test_nested_state_segment_path_still_works(self, tmp_config_dir: Path) -> None: + """`parameters.state.x` -- `state` is NOT the first segment, so it must + pass the guard and land inside configuration.parameters.state.x.""" + service, client = _make_service(tmp_config_dir) + + service.update_config( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + set_paths=[("parameters.state.x", "y")], + ) + + written = client.update_config.call_args.kwargs["configuration"] + assert written["parameters"]["state"]["x"] == "y" + + +# --------------------------------------------------------------------------- +# ConfigService.update_config_row -- same guard, same consistency +# --------------------------------------------------------------------------- + + +class TestUpdateConfigRowGuard: + def test_guarded_set_blocks_before_get_config_row(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir) + + with pytest.raises(KeboolaApiError) as exc_info: + service.update_config_row( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + row_id="row-001", + set_paths=[("isDisabled", True)], + ) + + assert exc_info.value.error_code == ErrorCode.INVALID_ARGUMENT + client.get_config_row.assert_not_called() + client.update_config_row.assert_not_called() + + def test_guarded_set_blocks_with_dry_run(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir) + + with pytest.raises(KeboolaApiError): + service.update_config_row( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + row_id="row-001", + set_paths=[("state.x", 1)], + dry_run=True, + ) + + client.get_config_row.assert_not_called() + client.update_config_row.assert_not_called() + + def test_normal_set_still_works(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir) + + service.update_config_row( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + row_id="row-001", + set_paths=[("parameters.table", "orders")], + ) + + client.get_config_row.assert_called_once() + client.update_config_row.assert_called_once() + written = client.update_config_row.call_args.kwargs["configuration"] + assert written["parameters"]["table"] == "orders" + + +# --------------------------------------------------------------------------- +# set_nested_value -- bracket-syntax guard + files.0 regression +# --------------------------------------------------------------------------- + + +class TestSetNestedValueBracketGuard: + def test_bracket_syntax_raises_clear_error(self) -> None: + with pytest.raises(ValueError, match=r"files\.0"): + set_nested_value({"files": [1, 2, 3]}, "files[0]", 99) + + def test_bracket_syntax_deep_in_path_also_raises(self) -> None: + with pytest.raises(ValueError, match=r"files\.0"): + set_nested_value( + {"parameters": {"files": [{"name": "a"}]}}, + "parameters.files[0].name", + "b", + ) + + def test_dotted_integer_index_still_works(self) -> None: + """Regression: the existing supported form must keep working.""" + result = set_nested_value({"files": [1, 2, 3]}, "files.0", 99) + assert result["files"] == [99, 2, 3] + + def test_dotted_integer_index_on_nested_list_still_works(self) -> None: + result = set_nested_value( + {"parameters": {"files": [{"name": "a"}, {"name": "b"}]}}, + "parameters.files.1.name", + "changed", + ) + assert result["parameters"]["files"][1]["name"] == "changed" + + def test_bracket_syntax_does_not_create_literal_key(self) -> None: + """Defense-in-depth: confirm the rejected call never returns a dict at + all -- i.e. there is no code path where a literal "files[0]" key could + slip through, since the function raises before constructing a result.""" + obj = {"files": [1, 2, 3]} + try: + set_nested_value(obj, "files[0]", 99) + except ValueError: + pass + else: + pytest.fail("expected ValueError for bracket syntax") + assert "files[0]" not in obj + + +# --------------------------------------------------------------------------- +# CLI layer -- exit code 2 for the guard (issue #593 Part A follow-up) +# +# `validate_set_paths` raises KeboolaApiError(INVALID_ARGUMENT), which +# `map_error_to_exit_code` maps to the generic exit 1 (INVALID_ARGUMENT has +# no special-cased mapping). The spec requires a USAGE-error exit code (2) +# for this guard specifically, so `config update` / `config row-update` call +# `validate_set_paths` directly right after building `parsed_sets` and force +# `typer.Exit(code=2)` on failure -- see commands/config.py. +# --------------------------------------------------------------------------- + + +class TestConfigSetGuardCliExitCode: + """CLI-level exit-code coverage for the --set sibling-path guard.""" + + @staticmethod + def _invoke(tmp_config_dir: Path, args: list[str]) -> Result: + return runner.invoke(app, ["--json", "--config-dir", str(tmp_config_dir), *args]) + + @staticmethod + def _patch_service(mp: pytest.MonkeyPatch, service: ConfigService) -> None: + mp.setattr( + "keboola_agent_cli.commands.config.get_service", + lambda ctx, name: service, + ) + + def test_config_update_set_state_exits_2(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, service) + result = self._invoke( + tmp_config_dir, + [ + "config", + "update", + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--set", + "state.foo=1", + ], + ) + + assert result.exit_code == 2, result.output + client.get_config_detail.assert_not_called() + client.update_config.assert_not_called() + + def test_config_update_set_state_dry_run_also_exits_2(self, tmp_config_dir: Path) -> None: + """--dry-run must not bypass the guard -- no plausible preview for a + usage mistake.""" + service, client = _make_service(tmp_config_dir) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, service) + result = self._invoke( + tmp_config_dir, + [ + "config", + "update", + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--set", + "state.foo=1", + "--dry-run", + ], + ) + + assert result.exit_code == 2, result.output + client.get_config_detail.assert_not_called() + + def test_config_update_set_parameters_regression_not_exit_2(self, tmp_config_dir: Path) -> None: + """Regression: a normal configuration.* --set passes the guard and + reaches the (mocked) client -- exit code is NOT 2.""" + service, client = _make_service(tmp_config_dir) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, service) + result = self._invoke( + tmp_config_dir, + [ + "config", + "update", + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--set", + "parameters.x=y", + ], + ) + + assert result.exit_code == 0, result.output + client.update_config.assert_called_once() + + def test_config_row_update_set_state_exits_2(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, service) + result = self._invoke( + tmp_config_dir, + [ + "config", + "row-update", + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--row-id", + "row-001", + "--set", + "isDisabled=true", + ], + ) + + assert result.exit_code == 2, result.output + client.get_config_row.assert_not_called() + client.update_config_row.assert_not_called() + + def test_config_row_update_set_state_dry_run_also_exits_2(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, service) + result = self._invoke( + tmp_config_dir, + [ + "config", + "row-update", + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--row-id", + "row-001", + "--set", + "state.x=1", + "--dry-run", + ], + ) + + assert result.exit_code == 2, result.output + client.get_config_row.assert_not_called() + + def test_config_row_update_set_parameters_regression_not_exit_2( + self, tmp_config_dir: Path + ) -> None: + service, client = _make_service(tmp_config_dir) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, service) + result = self._invoke( + tmp_config_dir, + [ + "config", + "row-update", + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--row-id", + "row-001", + "--set", + "parameters.table=orders", + ], + ) + + assert result.exit_code == 0, result.output + client.update_config_row.assert_called_once() diff --git a/tests/test_config_state.py b/tests/test_config_state.py new file mode 100644 index 00000000..6f8df8d0 --- /dev/null +++ b/tests/test_config_state.py @@ -0,0 +1,1134 @@ +"""Tests for config state read/write (issue #593). + +This file is shared across layers per the issue #593 implementation split: +- Client layer tests live in classes prefixed ``TestConfigStateClient*`` + (this agent's scope: ``client/configs.py::update_config_state`` and + ``update_config_row_state``). +- Service and CLI layer tests are added by a follow-up agent in separate + classes in this same file. +""" + +import json +from pathlib import Path +from unittest.mock import MagicMock + +import pytest +from typer.testing import CliRunner, Result + +from helpers import setup_single_project +from keboola_agent_cli.cli import app +from keboola_agent_cli.client import KeboolaClient +from keboola_agent_cli.constants import CONFIG_STATE_MAX_BYTES +from keboola_agent_cli.errors import ErrorCode, KeboolaApiError +from keboola_agent_cli.services.config_service import ConfigService + +FAKE_TOKEN = "901-55555-fakeTestTokenDoNotUseXXXXXXXX" + + +class TestConfigStateClientUpdateConfigState: + """Tests for KeboolaClient.update_config_state().""" + + def test_update_config_state_production_url(self, httpx_mock) -> None: + """No branch_id -> non-branch-scoped production URL.""" + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/components/keboola.ex-db-snowflake/configs/42/state", + json={ + "id": "42", + "name": "cfg", + "configuration": {}, + "state": {"lastId": 123}, + "version": 5, + }, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + result = client.update_config_state("keboola.ex-db-snowflake", "42", {"lastId": 123}) + + assert result["state"] == {"lastId": 123} + assert result["version"] == 5 + + def test_update_config_state_uses_put_method(self, httpx_mock) -> None: + """The request is a PUT, not POST/PATCH.""" + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/components/keboola.ex-db-snowflake/configs/42/state", + json={"id": "42", "state": {}}, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + client.update_config_state("keboola.ex-db-snowflake", "42", {}) + + request = httpx_mock.get_requests()[0] + assert request.method == "PUT" + + def test_update_config_state_branch_scoped_url(self, httpx_mock) -> None: + """branch_id set -> branch-scoped URL prefix.""" + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/branch/123/components/keboola.ex-db-snowflake/configs/42/state", + json={"id": "42", "state": {"cursor": "abc"}}, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + result = client.update_config_state( + "keboola.ex-db-snowflake", "42", {"cursor": "abc"}, branch_id=123 + ) + + assert result["state"] == {"cursor": "abc"} + + def test_update_config_state_body_is_json_not_form_encoded(self, httpx_mock) -> None: + """CRITICAL regression guard: body must be genuine JSON {"state": ...}, + NEVER the form-encoded data={"state": json.dumps(state)} shape that + update_config() uses for the `configuration` field. Sending the wrong + shape here silently breaks the write against the real API. + """ + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/components/keboola.ex-db-snowflake/configs/42/state", + json={"id": "42", "state": {"lastId": 123, "nested": {"a": 1}}}, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + client.update_config_state( + "keboola.ex-db-snowflake", "42", {"lastId": 123, "nested": {"a": 1}} + ) + + request = httpx_mock.get_requests()[0] + # A form-encoded body would not be valid JSON at all (or would parse + # to a flat string-keyed dict without nested structures preserved). + parsed = json.loads(request.content) + assert parsed == {"state": {"lastId": 123, "nested": {"a": 1}}} + # The Content-Type must be application/json, not + # application/x-www-form-urlencoded. + assert "application/json" in request.headers.get("content-type", "") + + def test_update_config_state_component_and_config_id_escaped(self, httpx_mock) -> None: + """Special characters in component_id/config_id are URL-escaped.""" + httpx_mock.add_response( + url=( + "https://connection.keboola.com/v2/storage/components/" + "keboola.ex-http%2Fspecial/configs/cfg%20id/state" + ), + json={"id": "cfg id", "state": {}}, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + result = client.update_config_state("keboola.ex-http/special", "cfg id", {}) + + assert result == {"id": "cfg id", "state": {}} + + def test_update_config_state_returns_full_detail_not_bare_state(self, httpx_mock) -> None: + """Response is the full config detail object, not just the state dict.""" + full_detail = { + "id": "42", + "name": "cfg", + "version": 7, + "changeDescription": "state updated", + "configuration": {"parameters": {"x": 1}}, + "rows": [], + "state": {"lastId": 999}, + "currentVersion": {"created": "2026-08-17T00:00:00+0000"}, + } + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/components/keboola.ex-db-snowflake/configs/42/state", + json=full_detail, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + result = client.update_config_state("keboola.ex-db-snowflake", "42", {"lastId": 999}) + + assert result == full_detail + + +class TestConfigStateClientUpdateConfigRowState: + """Tests for KeboolaClient.update_config_row_state().""" + + def test_update_config_row_state_production_url(self, httpx_mock) -> None: + """No branch_id -> non-branch-scoped production URL for the row endpoint.""" + httpx_mock.add_response( + url=( + "https://connection.keboola.com/v2/storage/components/" + "keboola.ex-db-snowflake/configs/42/rows/row-1/state" + ), + json={"id": "row-1", "state": {"lastId": 5}}, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + result = client.update_config_row_state( + "keboola.ex-db-snowflake", "42", "row-1", {"lastId": 5} + ) + + assert result["state"] == {"lastId": 5} + + def test_update_config_row_state_uses_put_method(self, httpx_mock) -> None: + """The request is a PUT.""" + httpx_mock.add_response( + url=( + "https://connection.keboola.com/v2/storage/components/" + "keboola.ex-db-snowflake/configs/42/rows/row-1/state" + ), + json={"id": "row-1", "state": {}}, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + client.update_config_row_state("keboola.ex-db-snowflake", "42", "row-1", {}) + + request = httpx_mock.get_requests()[0] + assert request.method == "PUT" + + def test_update_config_row_state_branch_scoped_url(self, httpx_mock) -> None: + """branch_id set -> branch-scoped URL prefix for the row endpoint.""" + httpx_mock.add_response( + url=( + "https://connection.keboola.com/v2/storage/branch/123/components/" + "keboola.ex-db-snowflake/configs/42/rows/row-1/state" + ), + json={"id": "row-1", "state": {"cursor": "xyz"}}, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + result = client.update_config_row_state( + "keboola.ex-db-snowflake", "42", "row-1", {"cursor": "xyz"}, branch_id=123 + ) + + assert result["state"] == {"cursor": "xyz"} + + def test_update_config_row_state_body_is_json_not_form_encoded(self, httpx_mock) -> None: + """Same CRITICAL regression guard as the config-level endpoint: body + must be {"state": ...} as real JSON, not form-encoded json.dumps. + """ + httpx_mock.add_response( + url=( + "https://connection.keboola.com/v2/storage/components/" + "keboola.ex-db-snowflake/configs/42/rows/row-1/state" + ), + json={"id": "row-1", "state": {"nested": {"list": [1, 2, 3]}}}, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + client.update_config_row_state( + "keboola.ex-db-snowflake", + "42", + "row-1", + {"nested": {"list": [1, 2, 3]}}, + ) + + request = httpx_mock.get_requests()[0] + parsed = json.loads(request.content) + assert parsed == {"state": {"nested": {"list": [1, 2, 3]}}} + assert "application/json" in request.headers.get("content-type", "") + + def test_update_config_row_state_ids_escaped(self, httpx_mock) -> None: + """Special characters in component_id/config_id/row_id are URL-escaped.""" + httpx_mock.add_response( + url=( + "https://connection.keboola.com/v2/storage/components/" + "keboola.ex-http%2Fspecial/configs/cfg%20id/rows/row%2Fid/state" + ), + json={"id": "row/id", "state": {}}, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + result = client.update_config_row_state( + "keboola.ex-http/special", "cfg id", "row/id", {} + ) + + assert result == {"id": "row/id", "state": {}} + + def test_update_config_row_state_returns_full_detail(self, httpx_mock) -> None: + """Response is the full config detail object, matching the config-level variant.""" + full_detail = { + "id": "42", + "name": "cfg", + "version": 3, + "configuration": {}, + "rows": [{"id": "row-1", "state": {"lastId": 1}}], + "state": {}, + "currentVersion": {"created": "2026-08-17T00:00:00+0000"}, + } + httpx_mock.add_response( + url=( + "https://connection.keboola.com/v2/storage/components/" + "keboola.ex-db-snowflake/configs/42/rows/row-1/state" + ), + json=full_detail, + status_code=200, + ) + + with KeboolaClient( + stack_url="https://connection.keboola.com", + token=FAKE_TOKEN, + ) as client: + result = client.update_config_row_state( + "keboola.ex-db-snowflake", "42", "row-1", {"lastId": 1} + ) + + assert result == full_detail + + +# --------------------------------------------------------------------------- +# Service layer tests (issue #593 Part B) +# +# Mocking follows the rest of the config_service test suite: a MagicMock +# client injected via client_factory, no pytest-httpx. See +# tests/test_config_set_default_bucket.py / tests/test_variables_cli.py for +# the reference shape this mirrors. +# --------------------------------------------------------------------------- + + +runner = CliRunner() + + +def _state_detail(state: dict | None = None, rows: list[dict] | None = None) -> dict: + """Build a sample config detail response carrying root + row state.""" + return { + "id": "cfg-001", + "name": "My Config", + "version": 3, + "configuration": {"parameters": {"a": 1}}, + "state": state if state is not None else {}, + "rows": rows if rows is not None else [], + } + + +def _make_state_service( + tmp_config_dir: Path, detail: dict | None = None +) -> tuple[ConfigService, MagicMock]: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = detail if detail is not None else _state_detail() + service = ConfigService( + config_store=store, + client_factory=lambda url, token: mock_client, + ) + return service, mock_client + + +class TestConfigServiceGetConfigState: + """Tests for ConfigService.get_config_state.""" + + def test_get_root_state(self, tmp_config_dir: Path) -> None: + service, client = _make_state_service(tmp_config_dir, _state_detail(state={"lastId": 42})) + + result = service.get_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + ) + + assert result == { + "project_alias": "prod", + "component_id": "keboola.ex-db-snowflake", + "config_id": "cfg-001", + "row_id": None, + "branch_id": None, + "state": {"lastId": 42}, + } + client.get_config_detail.assert_called_once_with( + "keboola.ex-db-snowflake", "cfg-001", branch_id=None + ) + client.close.assert_called_once() + + def test_get_row_state(self, tmp_config_dir: Path) -> None: + rows = [ + {"id": "row-1", "state": {"cursor": "abc"}}, + {"id": "row-2", "state": {}}, + ] + service, _client = _make_state_service(tmp_config_dir, _state_detail(rows=rows)) + + result = service.get_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + row_id="row-1", + ) + + assert result["state"] == {"cursor": "abc"} + assert result["row_id"] == "row-1" + + def test_get_missing_row_raises_named_not_found(self, tmp_config_dir: Path) -> None: + """A typo'd row id must fail loudly and name the row -- never a + silent empty dict (this is the exact class of bug issue #593 fixes + for --set).""" + rows = [{"id": "row-1", "state": {}}] + service, _client = _make_state_service(tmp_config_dir, _state_detail(rows=rows)) + + with pytest.raises(KeboolaApiError) as exc_info: + service.get_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + row_id="row-typo", + ) + + assert exc_info.value.error_code == ErrorCode.NOT_FOUND + assert "row-typo" in exc_info.value.message + + def test_root_state_missing_key_defaults_to_empty_dict(self, tmp_config_dir: Path) -> None: + detail = _state_detail() + del detail["state"] + service, _client = _make_state_service(tmp_config_dir, detail) + + result = service.get_config_state( + alias="prod", component_id="keboola.ex-db-snowflake", config_id="cfg-001" + ) + + assert result["state"] == {} + + def test_branch_id_propagated_to_client(self, tmp_config_dir: Path) -> None: + service, client = _make_state_service(tmp_config_dir) + + service.get_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + branch_id=999, + ) + + client.get_config_detail.assert_called_once_with( + "keboola.ex-db-snowflake", "cfg-001", branch_id=999 + ) + + +class TestConfigServiceSetConfigState: + """Tests for ConfigService.set_config_state.""" + + def test_reject_non_object_list(self, tmp_config_dir: Path) -> None: + service, client = _make_state_service(tmp_config_dir) + + with pytest.raises(KeboolaApiError) as exc_info: + service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state=[1, 2, 3], # ty: ignore[invalid-argument-type] # deliberately wrong type: exercises the validation + ) + + assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR + client.get_config_detail.assert_not_called() + + def test_reject_non_object_scalar(self, tmp_config_dir: Path) -> None: + service, client = _make_state_service(tmp_config_dir) + + with pytest.raises(KeboolaApiError) as exc_info: + service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state="not-an-object", # ty: ignore[invalid-argument-type] # deliberately wrong type: exercises the validation + ) + + assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR + client.get_config_detail.assert_not_called() + + def test_reject_oversized_state(self, tmp_config_dir: Path) -> None: + """Serialized body >= CONFIG_STATE_MAX_BYTES is rejected before any + network call -- the 4 MB cap is enforced locally, not just by the API.""" + service, client = _make_state_service(tmp_config_dir) + huge_state = {"blob": "x" * (CONFIG_STATE_MAX_BYTES + 10)} + + with pytest.raises(KeboolaApiError) as exc_info: + service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state=huge_state, + ) + + assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR + client.get_config_detail.assert_not_called() + + def test_dry_run_returns_diff_without_writing(self, tmp_config_dir: Path) -> None: + service, client = _make_state_service(tmp_config_dir, _state_detail(state={"lastId": 1})) + + result = service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state={"lastId": 2}, + dry_run=True, + ) + + assert result["dry_run"] is True + assert result["old_state"] == {"lastId": 1} + assert result["new_state"] == {"lastId": 2} + assert any("lastId" in c for c in result["changes"]) + assert result["project_alias"] == "prod" + assert result["component_id"] == "keboola.ex-db-snowflake" + assert result["config_id"] == "cfg-001" + assert result["row_id"] is None + client.update_config_state.assert_not_called() + client.update_config_row_state.assert_not_called() + + def test_no_op_short_circuit_skips_write(self, tmp_config_dir: Path) -> None: + service, client = _make_state_service(tmp_config_dir, _state_detail(state={"lastId": 1})) + + result = service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state={"lastId": 1}, + ) + + assert result["changed"] is False + assert result["state"] == {"lastId": 1} + client.update_config_state.assert_not_called() + client.update_config_row_state.assert_not_called() + + def test_real_write_root_state(self, tmp_config_dir: Path) -> None: + service, client = _make_state_service(tmp_config_dir, _state_detail(state={"lastId": 1})) + client.update_config_state.return_value = _state_detail(state={"lastId": 2}) + + result = service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state={"lastId": 2}, + ) + + assert result["changed"] is True + assert result["state"] == {"lastId": 2} + assert result["row_id"] is None + client.update_config_state.assert_called_once_with( + "keboola.ex-db-snowflake", "cfg-001", {"lastId": 2}, branch_id=None + ) + client.close.assert_called_once() + + def test_real_write_row_state(self, tmp_config_dir: Path) -> None: + rows = [{"id": "row-1", "state": {"cursor": "old"}}] + service, client = _make_state_service(tmp_config_dir, _state_detail(rows=rows)) + # Real shape of PUT .../rows/{row}/state: the bare row, not a detail. + client.update_config_row_state.return_value = { + "id": "row-1", + "state": {"cursor": "new"}, + "version": 2, + } + + result = service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state={"cursor": "new"}, + row_id="row-1", + ) + + assert result["changed"] is True + assert result["state"] == {"cursor": "new"} + assert result["row_id"] == "row-1" + client.update_config_row_state.assert_called_once_with( + "keboola.ex-db-snowflake", "cfg-001", "row-1", {"cursor": "new"}, branch_id=None + ) + client.update_config_state.assert_not_called() + + def test_set_missing_row_raises_named_not_found(self, tmp_config_dir: Path) -> None: + rows = [{"id": "row-1", "state": {}}] + service, client = _make_state_service(tmp_config_dir, _state_detail(rows=rows)) + + with pytest.raises(KeboolaApiError) as exc_info: + service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state={"x": 1}, + row_id="row-typo", + ) + + assert exc_info.value.error_code == ErrorCode.NOT_FOUND + assert "row-typo" in exc_info.value.message + client.update_config_state.assert_not_called() + client.update_config_row_state.assert_not_called() + + def test_branch_id_propagated_to_client_write(self, tmp_config_dir: Path) -> None: + service, client = _make_state_service(tmp_config_dir, _state_detail(state={"lastId": 1})) + client.update_config_state.return_value = _state_detail(state={"lastId": 2}) + + service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state={"lastId": 2}, + branch_id=456, + ) + + client.get_config_detail.assert_called_once_with( + "keboola.ex-db-snowflake", "cfg-001", branch_id=456 + ) + client.update_config_state.assert_called_once_with( + "keboola.ex-db-snowflake", "cfg-001", {"lastId": 2}, branch_id=456 + ) + + +# --------------------------------------------------------------------------- +# CLI layer tests (issue #593 Part B) +# --------------------------------------------------------------------------- + + +class TestConfigStateCli: + """CLI-level tests for `config state-get` / `config state-set`.""" + + @staticmethod + def _invoke( + tmp_config_dir: Path, + command: str, + args: list[str], + json_mode: bool = True, + input_text: str | None = None, + ) -> Result: + base = ["--config-dir", str(tmp_config_dir)] + if json_mode: + base = ["--json", *base] + return runner.invoke(app, [*base, "config", command, *args], input=input_text) + + @staticmethod + def _patch_service(mp: pytest.MonkeyPatch, store, mock_client: MagicMock) -> None: + # `state-get` / `state-set` live in commands/config_state.py (split out + # of config.py for the file-size-budget ratchet, see that module's + # docstring) and import `get_service` into their own module + # namespace, so the patch target differs from the sibling config + # commands defined directly in commands/config.py. + mp.setattr( + "keboola_agent_cli.commands.config_state.get_service", + lambda ctx, name: ConfigService( + config_store=store, + client_factory=lambda url, token: mock_client, + ), + ) + + # -- state-get ---------------------------------------------------------- + + def test_state_get_json_output(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={"lastId": 7}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-get", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + ], + ) + + assert result.exit_code == 0, result.output + payload = json.loads(result.output) + assert payload["data"]["state"] == {"lastId": 7} + assert payload["data"]["row_id"] is None + + def test_state_get_human_output(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={"lastId": 7}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-get", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + ], + json_mode=False, + ) + + assert result.exit_code == 0, result.output + assert "lastId" in result.output + + def test_state_get_row(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail( + rows=[{"id": "row-1", "state": {"cursor": "abc"}}] + ) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-get", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--row-id", + "row-1", + ], + ) + + assert result.exit_code == 0, result.output + payload = json.loads(result.output) + assert payload["data"]["state"] == {"cursor": "abc"} + assert payload["data"]["row_id"] == "row-1" + + def test_state_get_missing_row_exit_code(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail( + rows=[{"id": "row-1", "state": {}}] + ) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-get", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--row-id", + "row-typo", + ], + ) + + assert result.exit_code == 1, result.output + assert "row-typo" in result.output + + def test_state_get_branch_propagation(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-get", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--branch", + "321", + ], + ) + + assert result.exit_code == 0, result.output + mock_client.get_config_detail.assert_called_once_with( + "keboola.ex-db-snowflake", "cfg-001", branch_id=321 + ) + + # -- state-set ------------------------------------------------------ + + def test_state_set_rejects_non_object(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail() + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-set", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--state", + "[1, 2, 3]", + "--yes", + ], + ) + + assert result.exit_code != 0, result.output + assert "VALIDATION_ERROR" in result.output + mock_client.update_config_state.assert_not_called() + + def test_state_set_json_mode_skips_prompt_no_yes_needed(self, tmp_config_dir: Path) -> None: + """`--json` skips the confirmation prompt WITHOUT requiring --yes + (repo convention, see `config row-delete`).""" + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={"lastId": 1}) + mock_client.update_config_state.return_value = _state_detail(state={"lastId": 2}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-set", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--state", + '{"lastId": 2}', + ], + ) + + assert result.exit_code == 0, result.output + payload = json.loads(result.output) + assert payload["data"]["changed"] is True + mock_client.update_config_state.assert_called_once() + + def test_state_set_human_mode_without_yes_prompts_and_aborts_on_no( + self, tmp_config_dir: Path + ) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={"lastId": 1}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-set", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--state", + '{"lastId": 2}', + ], + json_mode=False, + input_text="n\n", + ) + + assert result.exit_code == 0, result.output + assert "Aborted" in result.output + mock_client.update_config_state.assert_not_called() + + def test_state_set_human_mode_without_yes_prompts_and_writes_on_yes( + self, tmp_config_dir: Path + ) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={"lastId": 1}) + mock_client.update_config_state.return_value = _state_detail(state={"lastId": 2}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-set", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--state", + '{"lastId": 2}', + ], + json_mode=False, + input_text="y\n", + ) + + assert result.exit_code == 0, result.output + mock_client.update_config_state.assert_called_once() + + def test_state_set_yes_flag_skips_prompt(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={"lastId": 1}) + mock_client.update_config_state.return_value = _state_detail(state={"lastId": 2}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-set", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--state", + '{"lastId": 2}', + "--yes", + ], + json_mode=False, + input_text="", + ) + + assert result.exit_code == 0, result.output + mock_client.update_config_state.assert_called_once() + + def test_state_set_dry_run_never_prompts_even_without_yes(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={"lastId": 1}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-set", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--state", + '{"lastId": 2}', + "--dry-run", + ], + json_mode=False, + input_text="", + ) + + assert result.exit_code == 0, result.output + assert "Dry-run" in result.output + mock_client.update_config_state.assert_not_called() + + def test_state_set_dry_run_json_shape(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={"lastId": 1}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-set", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--state", + '{"lastId": 2}', + "--dry-run", + ], + ) + + assert result.exit_code == 0, result.output + payload = json.loads(result.output) + assert payload["data"]["dry_run"] is True + assert payload["data"]["old_state"] == {"lastId": 1} + assert payload["data"]["new_state"] == {"lastId": 2} + mock_client.update_config_state.assert_not_called() + + def test_state_set_row(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail( + rows=[{"id": "row-1", "state": {"cursor": "old"}}] + ) + # Real shape of PUT .../rows/{row}/state: the bare row, not a detail. + mock_client.update_config_row_state.return_value = { + "id": "row-1", + "state": {"cursor": "new"}, + "version": 2, + } + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-set", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--row-id", + "row-1", + "--state", + '{"cursor": "new"}', + "--yes", + ], + ) + + assert result.exit_code == 0, result.output + payload = json.loads(result.output) + assert payload["data"]["row_id"] == "row-1" + assert payload["data"]["state"] == {"cursor": "new"} + mock_client.update_config_row_state.assert_called_once() + mock_client.update_config_state.assert_not_called() + + def test_state_set_branch_propagation(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={"lastId": 1}) + mock_client.update_config_state.return_value = _state_detail(state={"lastId": 2}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-set", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--state", + '{"lastId": 2}', + "--branch", + "654", + "--yes", + ], + ) + + assert result.exit_code == 0, result.output + mock_client.get_config_detail.assert_called_once_with( + "keboola.ex-db-snowflake", "cfg-001", branch_id=654 + ) + mock_client.update_config_state.assert_called_once_with( + "keboola.ex-db-snowflake", "cfg-001", {"lastId": 2}, branch_id=654 + ) + + def test_state_set_no_op_json_output(self, tmp_config_dir: Path) -> None: + store = setup_single_project(tmp_config_dir) + mock_client = MagicMock() + mock_client.get_config_detail.return_value = _state_detail(state={"lastId": 1}) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, store, mock_client) + result = self._invoke( + tmp_config_dir, + "state-set", + [ + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--state", + '{"lastId": 1}', + ], + ) + + assert result.exit_code == 0, result.output + payload = json.loads(result.output) + assert payload["data"]["changed"] is False + mock_client.update_config_state.assert_not_called() + + +class TestConfigStateRowWriteResponseShape: + """Regression: the row PUT answers with a different shape than the root PUT. + + Caught by the live E2E run against project 5946 (issue #593). The root + endpoint returns the full configuration detail -- which carries a ``rows`` + array -- while the row endpoint returns the *bare row object* with no + ``rows`` key at all. The service used to look the row back up inside + ``result["rows"]`` regardless, so every row write raised NOT_FOUND even + though the PUT had returned 200 and the state had landed on the server. + + These tests deliberately mock the SHAPES THE REAL API RETURNS. A mock that + echoes a full config detail for the row endpoint would pass while the real + call fails -- which is exactly how the bug survived the first test pass. + """ + + def test_row_write_returns_state_from_bare_row_response(self, tmp_config_dir: Path) -> None: + service, client = _make_state_service( + tmp_config_dir, _state_detail(rows=[{"id": "row-1", "state": {}}]) + ) + # Real shape of PUT .../rows/{row}/state: the bare row, no "rows" key. + client.update_config_row_state.return_value = { + "id": "row-1", + "configuration": {"foo": "bar"}, + "state": {"rowCursor": "abc"}, + "version": 3, + } + + result = service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state={"rowCursor": "abc"}, + row_id="row-1", + ) + + assert result["changed"] is True + assert result["state"] == {"rowCursor": "abc"} + assert result["row_id"] == "row-1" + client.update_config_row_state.assert_called_once() + + def test_root_write_still_reads_state_from_full_detail(self, tmp_config_dir: Path) -> None: + service, client = _make_state_service(tmp_config_dir) + # Real shape of PUT .../state: the full configuration detail. + client.update_config_state.return_value = _state_detail(state={"lastImportId": "999"}) + + result = service.set_config_state( + alias="prod", + component_id="keboola.ex-db-snowflake", + config_id="cfg-001", + state={"lastImportId": "999"}, + ) + + assert result["changed"] is True + assert result["state"] == {"lastImportId": "999"} diff --git a/tests/test_e2e.py b/tests/test_e2e.py index fb44139e..42d5b0e2 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -12905,3 +12905,372 @@ def test_no_selector_in_json_mode_fails_fast_instead_of_prompting(self) -> None: assert "--all" in message or "--project-id" in message, ( f"error should point at the non-interactive flags: {body['error']['message']}" ) + + +# --------------------------------------------------------------------------- +# config state-get / config state-set (issue #593) +# --------------------------------------------------------------------------- + + +@skip_without_credentials +@pytest.mark.e2e +class TestE2EConfigState: + """End-to-end tests for `config state-get` / `config state-set` (issue #593). + + Creates a throwaway ``ex-generic-v2`` config (needs no external + credentials, creates/deletes cleanly -- same choice as + ``TestE2EMcpParityCommands``), exercises the full state round-trip + (fresh-empty -> dry-run -> real write -> read-back -> no-op -> row + variant -> missing-row error), and the Part A ``--set`` sibling-path + guard, then deletes the config. + + NOTE on the row-state-set assertions below: the two state endpoints + answer with DIFFERENT shapes. The root ``PUT .../state`` returns the + full configuration detail (which carries a ``rows[]`` array), while + ``PUT .../rows/{row}/state`` returns the bare updated row object with + no ``rows`` key at all. An early build of #593 ran the same + ``_extract_state(result, row_id, ...)`` lookup over both, so every row + write reported a false ``NOT_FOUND`` even though the PUT returned 200 + and the state had landed. That was found by running this suite against + a live project -- mock-based tests could not catch it, because the + mocks returned the shape the author assumed rather than the shape the + API sends. If ``test_row_state_roundtrip`` ever fails on + ``changed``/``state`` for the write step specifically, suspect that + post-write extraction again before assuming the test is wrong. + """ + + @pytest.fixture(autouse=True) + def setup(self, tmp_path: Path) -> None: + self.token = os.environ[ENV_TOKEN] + raw_url = os.environ.get(ENV_URL, "connection.keboola.com") + self.url = raw_url if raw_url.startswith("https://") else f"https://{raw_url}" + self.alias = f"{RUN_ID}-state"[:60] + self.component_id = "ex-generic-v2" + + self.config_dir = tmp_path / "config" + self.config_dir.mkdir() + + result = _invoke( + self.config_dir, + [ + "--json", + "project", + "add", + "--project", + self.alias, + "--url", + self.url, + "--token", + self.token, + ], + ) + assert result.exit_code == 0, f"project add failed: {result.output}" + + self.client = KeboolaClient(stack_url=self.url, token=self.token) + self._created_config_ids: list[str] = [] + + @pytest.fixture(autouse=True) + def cleanup(self) -> Any: + yield + for cfg_id in self._created_config_ids: + with contextlib.suppress(Exception): + self.client.delete_config(component_id=self.component_id, config_id=cfg_id) + self.client.close() + + def _run(self, *args: str) -> Any: + return _invoke(self.config_dir, ["--json", *args]) + + def _run_ok(self, *args: str) -> dict[str, Any]: + return _json_ok(self._run(*args)) + + def _create_config(self, name_suffix: str) -> str: + cfg = self.client.create_config( + component_id=self.component_id, + name=f"{RUN_ID}-state-{name_suffix}", + configuration={}, + description="E2E throwaway -- issue #593 config state-get/state-set", + ) + config_id = str(cfg["id"]) + self._created_config_ids.append(config_id) + return config_id + + def test_root_state_roundtrip(self) -> None: + """Fresh empty -> dry-run (no write) -> real write -> read-back -> no-op.""" + config_id = self._create_config("root") + + _step(1, "state-get on a fresh config -- expect {}") + data = self._run_ok( + "config", + "state-get", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + )["data"] + assert data["state"] == {} + + _step(2, "state-set --dry-run -- diff only, no write") + data = self._run_ok( + "config", + "state-set", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--state", + '{"lastImportId": "12345"}', + "--dry-run", + )["data"] + assert data["dry_run"] is True + assert "changes" in data + + data = self._run_ok( + "config", + "state-get", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + )["data"] + assert data["state"] == {}, "a --dry-run state-set must not write anything" + + _step(3, 'state-set --state \'{"lastImportId": "12345"}\' --yes -- real write') + data = self._run_ok( + "config", + "state-set", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--state", + '{"lastImportId": "12345"}', + "--yes", + )["data"] + assert data["changed"] is True + assert data["state"] == {"lastImportId": "12345"} + + _step(4, "state-get -- matches what was written") + data = self._run_ok( + "config", + "state-get", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + )["data"] + assert data["state"] == {"lastImportId": "12345"} + + _step(5, "state-set with the SAME state again -- no-op (changed: False)") + data = self._run_ok( + "config", + "state-set", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--state", + '{"lastImportId": "12345"}', + "--yes", + )["data"] + assert data["changed"] is False + + def test_row_state_roundtrip(self) -> None: + """Row state is independent of root state; write/read-back a row's state. + + See the class docstring on why the row PUT's response shape differs + from the root one -- that asymmetry caused a false NOT_FOUND on every + row write until it was fixed in #593. + """ + config_id = self._create_config("row") + + row = self.client.create_config_row( + component_id=self.component_id, + config_id=config_id, + name=f"{RUN_ID}-state-row", + configuration={}, + ) + row_id = str(row["id"]) + + _step(1, "seed root state so independence is checkable") + self._run_ok( + "config", + "state-set", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--state", + '{"lastImportId": "12345"}', + "--yes", + ) + + _step(2, "state-set --row-id -- real write on the row") + data = self._run_ok( + "config", + "state-set", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--row-id", + row_id, + "--state", + '{"rowCursor": "abc"}', + "--yes", + )["data"] + assert data["changed"] is True + assert data["state"] == {"rowCursor": "abc"} + + _step(3, "state-get --row-id -- matches what was written") + data = self._run_ok( + "config", + "state-get", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--row-id", + row_id, + )["data"] + assert data["state"] == {"rowCursor": "abc"} + + _step(4, "root state is unaffected by the row write") + data = self._run_ok( + "config", + "state-get", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + )["data"] + assert data["state"] == {"lastImportId": "12345"} + + _step(5, "row state is unaffected by a subsequent root write") + self._run_ok( + "config", + "state-set", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--state", + '{"lastImportId": "99999"}', + "--yes", + ) + data = self._run_ok( + "config", + "state-get", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--row-id", + row_id, + )["data"] + assert data["state"] == {"rowCursor": "abc"} + + def test_state_get_missing_row_fails_clearly(self) -> None: + """state-get --row-id must fail with a clear error, not return {}.""" + config_id = self._create_config("missing-row") + + _step(1, "state-get --row-id -- clear error, not an empty dict") + result = self._run( + "config", + "state-get", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--row-id", + "does-not-exist-593", + ) + assert result.exit_code != 0 + body = json.loads(result.output) + assert body["status"] == "error" + assert "does-not-exist-593" in body["error"]["message"] + + def test_set_guard_rejects_state_prefix_exit_2(self) -> None: + """`config update --set 'state.foo=1'` must be a hard usage error (exit 2). + + Part A of issue #593: `--set` only edits `configuration.*`; a path + whose first segment is a top-level API sibling like `state` must be + rejected before any network call, and the error must point the + caller at `config state-set`. A plain `--set 'parameters.foo=1'` + must keep working (regression check). + """ + config_id = self._create_config("guard") + + _step(1, "guarded --set 'state.foo=1' -- exit 2, message names config state-set") + result = self._run( + "config", + "update", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--set", + "state.foo=1", + ) + assert result.exit_code == 2, f"expected exit 2, got {result.exit_code}: {result.output}" + body = json.loads(result.output) + assert body["status"] == "error" + assert "config state-set" in body["error"]["message"] + + _step(2, "guard also fires under --dry-run (usage error must never look like a preview)") + result = self._run( + "config", + "update", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--set", + "state.foo=1", + "--dry-run", + ) + assert result.exit_code == 2, f"--dry-run must not bypass the guard: {result.output}" + + _step(3, "regression -- a normal --set 'parameters.foo=1' still works") + data = self._run_ok( + "config", + "update", + "--project", + self.alias, + "--component-id", + self.component_id, + "--config-id", + config_id, + "--set", + "parameters.foo=1", + )["data"] + assert data["configuration"]["parameters"]["foo"] == 1 diff --git a/tests/test_server_router_calls.py b/tests/test_server_router_calls.py index 56ca3bbd..a0db874f 100644 --- a/tests/test_server_router_calls.py +++ b/tests/test_server_router_calls.py @@ -1807,3 +1807,145 @@ def test_table_from_snapshot_passes_kwargs(tmp_path: Path) -> None: branch_id=None, dry_run=False, ) + + +# --------------------------------------------------------------------------- +# configs.py GET/PUT /{p}/{c}/{cfg}/state +# Service: config.get_config_state / config.set_config_state (issue #593) +# --------------------------------------------------------------------------- + + +def test_config_state_get_passes_kwargs(tmp_path: Path) -> None: + """GET /configs/{p}/{c}/{cfg}/state -> config.get_config_state (no row_id/branch_id).""" + config_svc = MagicMock() + config_svc.get_config_state.return_value = { + "project_alias": PROJECT, + "component_id": COMPONENT, + "config_id": CONFIG_ID, + "row_id": None, + "branch_id": None, + "state": {"lastId": 123}, + } + registry = _mock_registry(config=config_svc) + app = _make_app_with_registry(tmp_path, registry) + + with TestClient(app) as client: + res = client.get( + f"/configs/{PROJECT}/{COMPONENT}/{CONFIG_ID}/state", + headers=AUTH, + ) + + assert res.status_code == 200, res.text + config_svc.get_config_state.assert_called_once_with( + alias=PROJECT, + component_id=COMPONENT, + config_id=CONFIG_ID, + row_id=None, + branch_id=None, + ) + + +def test_config_state_get_forwards_row_id_and_branch_id(tmp_path: Path) -> None: + """GET .../state?row_id=&branch_id= forwards both query params to the service.""" + config_svc = MagicMock() + config_svc.get_config_state.return_value = { + "project_alias": PROJECT, + "component_id": COMPONENT, + "config_id": CONFIG_ID, + "row_id": ROW_ID, + "branch_id": 456, + "state": {}, + } + registry = _mock_registry(config=config_svc) + app = _make_app_with_registry(tmp_path, registry) + + with TestClient(app) as client: + res = client.get( + f"/configs/{PROJECT}/{COMPONENT}/{CONFIG_ID}/state", + headers=AUTH, + params={"row_id": ROW_ID, "branch_id": 456}, + ) + + assert res.status_code == 200, res.text + config_svc.get_config_state.assert_called_once_with( + alias=PROJECT, + component_id=COMPONENT, + config_id=CONFIG_ID, + row_id=ROW_ID, + branch_id=456, + ) + + +def test_config_state_set_passes_kwargs(tmp_path: Path) -> None: + """PUT /configs/{p}/{c}/{cfg}/state -> config.set_config_state with body fields.""" + config_svc = MagicMock() + config_svc.set_config_state.return_value = { + "project_alias": PROJECT, + "component_id": COMPONENT, + "config_id": CONFIG_ID, + "row_id": None, + "branch_id": None, + "state": {"lastId": 999}, + "changed": True, + } + registry = _mock_registry(config=config_svc) + app = _make_app_with_registry(tmp_path, registry) + + with TestClient(app) as client: + res = client.put( + f"/configs/{PROJECT}/{COMPONENT}/{CONFIG_ID}/state", + headers=AUTH, + json={"state": {"lastId": 999}}, + ) + + assert res.status_code == 200, res.text + config_svc.set_config_state.assert_called_once_with( + alias=PROJECT, + component_id=COMPONENT, + config_id=CONFIG_ID, + state={"lastId": 999}, + row_id=None, + branch_id=None, + dry_run=False, + ) + + +def test_config_state_set_forwards_row_id_branch_id_and_dry_run(tmp_path: Path) -> None: + """PUT .../state with row_id/branch_id/dry_run in the body forwards all of them.""" + config_svc = MagicMock() + config_svc.set_config_state.return_value = { + "project_alias": PROJECT, + "component_id": COMPONENT, + "config_id": CONFIG_ID, + "row_id": ROW_ID, + "branch_id": 456, + "dry_run": True, + "changes": {}, + "old_state": {}, + "new_state": {"lastId": 1}, + } + registry = _mock_registry(config=config_svc) + app = _make_app_with_registry(tmp_path, registry) + + with TestClient(app) as client: + res = client.put( + f"/configs/{PROJECT}/{COMPONENT}/{CONFIG_ID}/state", + headers=AUTH, + json={ + "state": {"lastId": 1}, + "row_id": ROW_ID, + "branch_id": 456, + "dry_run": True, + }, + ) + + assert res.status_code == 200, res.text + config_svc.set_config_state.assert_called_once_with( + alias=PROJECT, + component_id=COMPONENT, + config_id=CONFIG_ID, + state={"lastId": 1}, + row_id=ROW_ID, + branch_id=456, + dry_run=True, + ) From c964ba813f2c656b6f892585f5c28fda655d1332 Mon Sep 17 00:00:00 2001 From: Petr Date: Mon, 17 Aug 2026 17:51:24 -0400 Subject: [PATCH 2/3] fix(config): reject --set bracket syntax in the guard, not mid-request Review finding on #598: `set_nested_value` rejects `files[0]` with a bare `ValueError` that no command catches. That path is only reached after the configuration is fetched, so the failure surfaced mid-request as an unhandled traceback -- exit 1 with empty stdout under `--json`. A change whose whole point is turning a silent failure into a loud one must not introduce a new unstructured one. Bracket syntax is the same class of usage mistake as a sibling path, so it is now rejected in the same place (`validate_set_paths`), with the same exit code (2), before the same boundary (any network call), and with the same structured JSON error. The `ValueError` stays in `set_nested_value` as defence in depth for other callers, e.g. the SDK. Moves `validate_set_paths` into `services/_config_set_guard.py`: `config_service.py` hit its hard file-size ceiling again, and CONTRIBUTING asks the next PR adding material there to split it first rather than shave lines. The guard is a self-contained, framework-free unit with no dependency on `ConfigService` state, so it is a clean seam. `config_service.py` re-exports the symbol, so existing importers are unaffected. --- .../skills/kbagent/references/gotchas.md | 10 +- .../services/_config_set_guard.py | 88 ++++++++++++++ .../services/config_service.py | 61 +--------- tests/test_config_set_guard.py | 111 ++++++++++++++++++ 4 files changed, 207 insertions(+), 63 deletions(-) create mode 100644 src/keboola_agent_cli/services/_config_set_guard.py diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 45747352..27344bd7 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -3632,9 +3632,13 @@ even shaped like a real state document. - **A plain `--set 'parameters.x=y'` is unaffected** -- only paths whose *first* segment collides with a sibling field are rejected; the guard does not touch anything under `configuration`. -- **`files[0]` bracket syntax is now also rejected** with a message pointing - at the `files.0` form (which already worked, and still does, over an - existing list). It no longer silently creates a literal `"files[0]"` key. +- **`files[0]` bracket syntax is now also rejected**, by the same guard, with + the same exit 2, before any network call -- with a message pointing at the + `files.0` form (which already worked, and still does, over an existing + list). It no longer silently creates a literal `"files[0]"` key. Note the + example path at the top of this entry contains BOTH mistakes at once: a + `state` first segment and a `files[0]` segment. Either one alone is now + enough to be rejected. - If you hit the new exit-2 error on a script written before v0.84.2 and the intent really was to edit runtime state, switch to `kbagent config state-set --state ...` (see [config-state-workflow](config-state-workflow.md)) diff --git a/src/keboola_agent_cli/services/_config_set_guard.py b/src/keboola_agent_cli/services/_config_set_guard.py new file mode 100644 index 00000000..1d4a824a --- /dev/null +++ b/src/keboola_agent_cli/services/_config_set_guard.py @@ -0,0 +1,88 @@ +"""`--set` path guard for `config update` / `config row-update` (issue #593). + +Split out of ``config_service.py``: that module sits at its hard file-size +ceiling, and this guard is a self-contained, framework-free validation unit +with no dependency on ``ConfigService`` state. +""" + +from __future__ import annotations + +from typing import Any + +from ..constants import CONFIG_SET_GUARD_HINTS, CONFIG_SET_GUARDED_PREFIXES +from ..errors import ErrorCode, KeboolaApiError + + +def validate_set_paths(set_paths: list[tuple[str, Any]] | None) -> None: + """Reject ``--set`` paths whose first segment is an API-level sibling of + ``configuration`` (issue #593 Part A). + + ``config update --set`` and ``config row-update --set`` are documented + as editing ``configuration.*`` only. ``_resolve_configuration`` / + ``_resolve_row_configuration`` used to apply every ``--set`` path + unconditionally onto ``current_detail.get("configuration", {})`` -- + but keys like ``state`` or ``name`` are SIBLINGS of ``configuration`` + in the Storage API config-detail response, not children of it. So + e.g. ``--set 'state.x=y'`` silently created ``configuration.state.x`` + instead of touching the real ``state`` field: exit 0, a bumped + version, a plausible-looking ``--dry-run`` diff, and the runtime + state left untouched. + + Call this BEFORE any network call so the rejection also covers + ``--dry-run`` -- a usage mistake should never be allowed to look like + a successful preview. ``_resolve_configuration`` and + ``_resolve_row_configuration`` both call this as their first line. + + This is a plain, side-effect-free validation function (no CLI/typer + dependency, per the service layer's framework-free contract) so the + command layer can call it directly too, ahead of ``service.update_config`` + / ``service.update_config_row``, and map the raised error to a usage + exit code before anything (including a client) is constructed. + + Args: + set_paths: The parsed ``(path, value)`` pairs from one or more + ``--set PATH=VALUE`` flags, or ``None``/empty if none given. + + Raises: + KeboolaApiError: ``error_code=ErrorCode.INVALID_ARGUMENT`` naming + the offending path, its first segment, and the real tool/flag + to use instead -- callers should map this to a usage-error + exit code (see ``commands/config.py``'s existing ``--set + PATH=VALUE`` format check for the pattern to mirror). + """ + if not set_paths: + return + for path, _value in set_paths: + # Bracket syntax is the same class of usage mistake as a sibling path, + # so it is rejected in the same place, with the same exit code, before + # the same boundary (any network call). set_nested_value raises on it + # too, but that ValueError surfaces mid-request as an unhandled crash + # with no structured output -- exactly the silent-ish failure this + # guard exists to prevent. + if "[" in path or "]" in path: + raise KeboolaApiError( + status_code=400, + error_code=ErrorCode.INVALID_ARGUMENT, + message=( + f"--set '{path}' uses bracket syntax, which is not " + f"supported. Use dot-separated integer segments over an " + f"existing list instead, e.g. 'files.0' rather than " + f"'files[0]'." + ), + ) + first_segment = path.split(".", 1)[0] + if first_segment not in CONFIG_SET_GUARDED_PREFIXES: + continue + hint = CONFIG_SET_GUARD_HINTS.get(first_segment, "not settable via --set") + raise KeboolaApiError( + status_code=400, + error_code=ErrorCode.INVALID_ARGUMENT, + message=( + f"--set '{path}' targets '{first_segment}', which is a " + f"top-level API field, not part of 'configuration'. --set " + f"only edits configuration.* -- use {hint} instead. If the " + f"component genuinely has a 'configuration.{first_segment}' " + f"key, pass the full body via --configuration JSON|@file|- " + f"instead." + ), + ) diff --git a/src/keboola_agent_cli/services/config_service.py b/src/keboola_agent_cli/services/config_service.py index 2beb88eb..33d64e9d 100644 --- a/src/keboola_agent_cli/services/config_service.py +++ b/src/keboola_agent_cli/services/config_service.py @@ -17,8 +17,6 @@ from ..ai_client import AiServiceClient from ..config_store import ConfigStore from ..constants import ( - CONFIG_SET_GUARD_HINTS, - CONFIG_SET_GUARDED_PREFIXES, CONFIG_STATE_MAX_BYTES, ) from ..errors import ConfigError, ErrorCode, KeboolaApiError @@ -27,6 +25,7 @@ from ..sync.code_extraction import normalize_blocks_codes_script from ..sync.manifest import Manifest, load_manifest, save_manifest from ..sync.naming import sanitize_name +from ._config_set_guard import validate_set_paths from ._encryption import collect_secrets, encrypt_secrets_in_config, find_plaintext_secret_keys from .base import BaseService, ClientFactory, sanitize_unexpected_error from .workspace_service import find_storage_workspace_for_sandbox_config @@ -92,64 +91,6 @@ def _default_change_description(command: str, *, has_metadata: bool, has_content return f"Updated {' + '.join(parts)} via kbagent {command}" -def validate_set_paths(set_paths: list[tuple[str, Any]] | None) -> None: - """Reject ``--set`` paths whose first segment is an API-level sibling of - ``configuration`` (issue #593 Part A). - - ``config update --set`` and ``config row-update --set`` are documented - as editing ``configuration.*`` only. ``_resolve_configuration`` / - ``_resolve_row_configuration`` used to apply every ``--set`` path - unconditionally onto ``current_detail.get("configuration", {})`` -- - but keys like ``state`` or ``name`` are SIBLINGS of ``configuration`` - in the Storage API config-detail response, not children of it. So - e.g. ``--set 'state.x=y'`` silently created ``configuration.state.x`` - instead of touching the real ``state`` field: exit 0, a bumped - version, a plausible-looking ``--dry-run`` diff, and the runtime - state left untouched. - - Call this BEFORE any network call so the rejection also covers - ``--dry-run`` -- a usage mistake should never be allowed to look like - a successful preview. ``_resolve_configuration`` and - ``_resolve_row_configuration`` both call this as their first line. - - This is a plain, side-effect-free validation function (no CLI/typer - dependency, per the service layer's framework-free contract) so the - command layer can call it directly too, ahead of ``service.update_config`` - / ``service.update_config_row``, and map the raised error to a usage - exit code before anything (including a client) is constructed. - - Args: - set_paths: The parsed ``(path, value)`` pairs from one or more - ``--set PATH=VALUE`` flags, or ``None``/empty if none given. - - Raises: - KeboolaApiError: ``error_code=ErrorCode.INVALID_ARGUMENT`` naming - the offending path, its first segment, and the real tool/flag - to use instead -- callers should map this to a usage-error - exit code (see ``commands/config.py``'s existing ``--set - PATH=VALUE`` format check for the pattern to mirror). - """ - if not set_paths: - return - for path, _value in set_paths: - first_segment = path.split(".", 1)[0] - if first_segment not in CONFIG_SET_GUARDED_PREFIXES: - continue - hint = CONFIG_SET_GUARD_HINTS.get(first_segment, "not settable via --set") - raise KeboolaApiError( - status_code=400, - error_code=ErrorCode.INVALID_ARGUMENT, - message=( - f"--set '{path}' targets '{first_segment}', which is a " - f"top-level API field, not part of 'configuration'. --set " - f"only edits configuration.* -- use {hint} instead. If the " - f"component genuinely has a 'configuration.{first_segment}' " - f"key, pass the full body via --configuration JSON|@file|- " - f"instead." - ), - ) - - def _find_matches_in_json( obj: Any, match_fn: Any, diff --git a/tests/test_config_set_guard.py b/tests/test_config_set_guard.py index 80ecd193..d7321dc9 100644 --- a/tests/test_config_set_guard.py +++ b/tests/test_config_set_guard.py @@ -13,6 +13,7 @@ `files.0` dotted-integer form keeps working. """ +import json from pathlib import Path from unittest.mock import MagicMock @@ -502,3 +503,113 @@ def test_config_row_update_set_parameters_regression_not_exit_2( assert result.exit_code == 0, result.output client.update_config_row.assert_called_once() + + +class TestConfigSetBracketSyntaxCli: + """Bracket syntax must fail like any other usage error, not crash. + + Regression for the PR #598 review finding: ``set_nested_value`` rejects + ``files[0]`` with a bare ``ValueError``, which no command catches. Because + that path is only reached AFTER the configuration is fetched, the failure + surfaced mid-request as an unhandled traceback -- exit 1 with EMPTY stdout + under ``--json``. A PR whose whole point is turning a silent failure into a + loud one must not introduce a new unstructured one, so the bracket check + now lives in ``validate_set_paths`` alongside the sibling-prefix guard: + same place, same exit code, same pre-network boundary. + """ + + @staticmethod + def _invoke(tmp_config_dir: Path, args: list[str]) -> Result: + return runner.invoke(app, ["--json", "--config-dir", str(tmp_config_dir), *args]) + + @staticmethod + def _patch_service(mp: pytest.MonkeyPatch, service: ConfigService) -> None: + mp.setattr( + "keboola_agent_cli.commands.config.get_service", + lambda ctx, name: service, + ) + + def test_bracket_path_exits_2_with_structured_json(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, service) + result = self._invoke( + tmp_config_dir, + [ + "config", + "update", + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--set", + "files[0]=z", + ], + ) + + assert result.exit_code == 2, result.output + payload = json.loads(result.output) + assert payload["status"] == "error" + assert payload["error"]["code"] == ErrorCode.INVALID_ARGUMENT.value + assert "files.0" in payload["error"]["message"] + # Rejected before the network, exactly like the sibling-prefix guard. + client.get_config_detail.assert_not_called() + client.update_config.assert_not_called() + + def test_bracket_path_dry_run_also_exits_2(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, service) + result = self._invoke( + tmp_config_dir, + [ + "config", + "update", + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--set", + "files[0]=z", + "--dry-run", + ], + ) + + assert result.exit_code == 2, result.output + client.get_config_detail.assert_not_called() + + def test_bracket_path_rejected_on_row_update_too(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir) + + with pytest.MonkeyPatch.context() as mp: + self._patch_service(mp, service) + result = self._invoke( + tmp_config_dir, + [ + "config", + "row-update", + "--project", + "prod", + "--component-id", + "keboola.ex-db-snowflake", + "--config-id", + "cfg-001", + "--row-id", + "row-1", + "--set", + "files[0]=z", + ], + ) + + assert result.exit_code == 2, result.output + client.get_config_row.assert_not_called() + + def test_dotted_integer_form_still_works(self, tmp_config_dir: Path) -> None: + """The supported `files.0` form must stay usable over an existing list.""" + validate_set_paths([("parameters.files.0", "z")]) # no raise From ac8f5f0ed7e65aebd82bda1ae355807b358fedbd Mon Sep 17 00:00:00 2001 From: Petr Date: Mon, 17 Aug 2026 21:29:38 -0400 Subject: [PATCH 3/3] fix(config): NoReturn on the shared error handler, align state doc examples Two Devin review findings on #598: `_handle_config_service_error` unconditionally raises `typer.Exit` but was annotated `-> None`, so at all 14 call sites a static analyser had to treat the following `result` as possibly-unbound. `NoReturn` states the contract. The two doc surfaces disagreed on the state document shape: gotchas.md used a singular `"tag": "..."` while config-state-workflow.md used `"tags": [...]`. Keboola's own docs use `tags` as an array, so gotchas.md was simply wrong -- and since the whole point of these examples is that they get copied verbatim before a costly full-history reload, a wrong one is worse than none. Also softened an overreach while fixing it: the workflow doc called its example a "Confirmed shape (from the API-verified --set example in #593)", but #593 only ever verified the `lastImportId` field, not the surrounding document or its types. A component's state is component-defined, so both surfaces now say plainly that the example shows structure, not authoritative types, and that the reliable move is to `state-get` an already-migrated config and mirror what it returns. Seeding a wrong shape does not error -- it behaves like an empty state, which for `adaptive` is exactly the full reload the workflow exists to avoid. The third finding (bracket syntax escaping as a raw ValueError) was already fixed in the previous commit; verified again live -- exit 2 with structured JSON, no traceback. --- .../kbagent/references/config-state-workflow.md | 17 ++++++++++++----- .../skills/kbagent/references/gotchas.md | 13 ++++++++----- src/keboola_agent_cli/commands/config.py | 4 ++-- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/plugins/kbagent/skills/kbagent/references/config-state-workflow.md b/plugins/kbagent/skills/kbagent/references/config-state-workflow.md index 4586a420..21ee5712 100644 --- a/plugins/kbagent/skills/kbagent/references/config-state-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/config-state-workflow.md @@ -153,11 +153,18 @@ before committing to a value on a config that matters. ## State document shape for file input mapping -The exact shape mirrors the file input mapping's own `tags`/`query` -selection criteria; the state you write should describe the same tag set the -mapping filters on. Confirmed shape (from the API-verified `--set` example in -#593, and matching an `adaptive`-based file input mapping's checkpoint -field): +The shape mirrors the file input mapping's own `tags` selection criteria: +the state you write should describe the same tag set the mapping filters on, +and `tags` is an array, not a single string. + +**Treat the example below as structure, not as authoritative types.** Issue +#593 verified the `lastImportId` checkpoint field itself, but a component's +state document is component-defined -- whether an id serializes as a string +or a number is not something to infer from a doc example. The reliable move +is always the same: `state-get` an already-migrated config in the same +project, and mirror exactly what comes back. Seeding a wrong shape does not +error; it silently behaves like an empty state, which for `adaptive` means +the full-history reload this whole workflow exists to avoid. ```json { diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 27344bd7..abbbbd09 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -3666,11 +3666,14 @@ mapping in favor of `changed_since: adaptive`, which tracks a production -- reproduces the full-reload behaviour every time, unless the branch's state is seeded first. - **The fix is to seed, not to skip validation**: `kbagent config state-set - --branch --state '{"storage": {"input": {"files": [{"tag": - "...", "lastImportId": }]}}}'` before the first `job run` - on the branch. See [config-state-workflow](config-state-workflow.md) for - the full seed -> run -> verify -> merge sequence and the exact state - document shape for file input mappings. + --branch --state '{"storage": {"input": {"files": [{"tags": + [""], "lastImportId": ""}]}}}'` before the first `job + run` on the branch. Note `tags` is an ARRAY, matching the file input + mapping's own selection criteria. Do not copy the field types out of this + example -- run `state-get` against an already-migrated production config + and mirror what it actually returns. See + [config-state-workflow](config-state-workflow.md) for the full seed -> run + -> verify -> merge sequence. - **An empty state is more dangerous than a seeded one for this input mapping type.** For most incremental components an empty/cleared state is the deliberate "reprocess everything" reset and is the well-understood, diff --git a/src/keboola_agent_cli/commands/config.py b/src/keboola_agent_cli/commands/config.py index 0a9ca477..0965ad55 100644 --- a/src/keboola_agent_cli/commands/config.py +++ b/src/keboola_agent_cli/commands/config.py @@ -8,7 +8,7 @@ import logging import re from pathlib import Path -from typing import Any +from typing import Any, NoReturn import typer from rich.console import Console @@ -598,7 +598,7 @@ def _parse_set_value(raw: str) -> object: return raw -def _handle_config_service_error(formatter: Any, exc: ConfigError | KeboolaApiError) -> None: +def _handle_config_service_error(formatter: Any, exc: ConfigError | KeboolaApiError) -> NoReturn: """Shared ``ConfigError``/``KeboolaApiError`` -> exit-code mapping. Nearly every command in this group ends its try/except with this exact