feat(config): add --change-description to config update and row-update#505
feat(config): add --change-description to config update and row-update#505jordanrburger wants to merge 1 commit into
Conversation
Callers can now set the Keboola config version's `changeDescription` (the
version-history audit line) instead of always getting the generic default
("Updated ... via kbagent config update"). This matters for auditability on
shared production configs, where the version history is the paper trail.
- `config update` and `config row-update` gain `--change-description TEXT`;
when omitted, the previous auto-generated default is preserved unchanged.
- `--dry-run` echoes the change description that would be sent (JSON + human).
- The `kbagent serve` router mirrors the flag on both PATCH routes (1:1).
- The client layer already supported `change_description`; the service simply
threads a caller value through, falling back to a shared default helper.
Creates (`config new`, `row-create`) and `flow new/update` are left as a
follow-up: their client methods do not yet accept a change description, so
wiring them is more than the trivial passthrough done here.
padak
left a comment
There was a problem hiding this comment.
Review of #505 — feat(config): add --change-description to config update and row-update
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR threads an optional --change-description TEXT flag through config update and config row-update (Typer command → ConfigService → existing KeboolaClient parameter, which already supported it) so callers can set a meaningful Keboola config-version changeDescription instead of always getting the generic auto-generated default. The refactor also deduplicates the previously copy-pasted default-generation logic into one shared _default_change_description() helper. Coverage is thorough (service, CLI, serve router, and E2E layers), the 3-layer architecture is respected, all BLOCKING plugin-sync surfaces (context.py, CLAUDE.md, commands-reference.md, server/routers/configs.py 1:1 mirror) are updated correctly, and make check passes on every gate that actually gates CI. Verdict: COMMENT — no blocking issues; two non-blocking documentation/debt notes and one coverage nit.
Verdict
- Verdict: COMMENT
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 1
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/commands/config.py:565,2190 — new --change-description vs. --description naming collision has no gotcha entry
The PR's own description explicitly calls out that --change-description (sets the version's audit-trail changeDescription) is easy to confuse with the pre-existing --description (sets the config's display description) — the two flags differ by one word and touch completely different API fields. This is exactly the class of trap gotchas.md and keboola-expert.md §3 already document elsewhere (e.g. the existing source vs destination output-mapping gotcha), but neither file was updated in this diff. It's not a default-behavior change (omitting the flag preserves the old default exactly, verified by test_default_change_description_when_omitted), so I'm not treating it as BLOCKING — but an AI agent driving kbagent from keboola-expert.md alone (no --help read) could plausibly pass the wrong flag when asked to "set the description."
Fix: add a one-liner to plugins/kbagent/skills/kbagent/references/gotchas.md (tag (since vX.Y.Z) once the target release is known) and a §3 inline gotcha bullet in keboola-expert.md, e.g. "--change-description sets the version audit line, NOT the config's display description (--description)."
[NB-2] src/keboola_agent_cli/commands/config.py (2493 LOC) / src/keboola_agent_cli/services/config_service.py (2141 LOC) — both files are already far past the CONTRIBUTING.md hard ceiling, and this PR adds more material to both
CONTRIBUTING.md's file-size budget table caps commands/*.py at a 1200-line hard ceiling and services/*.py at 1500, stating "When a file crosses the hard ceiling, splitting is required before merging more functionality into it." Both files are already roughly 2x over (2493 and 2141 LOC respectively) before this PR, which adds ~26 lines to commands/config.py and a net +21 to config_service.py. This is chronic, pre-existing debt (not introduced by this PR, and the added code itself is a small, well-isolated, DRY-improving change), so I'm not blocking on it — but it's worth flagging since it keeps growing one small PR at a time and nobody currently has line-of-sight on it (no CI check for this budget).
Suggestion: no action required for this PR; consider filing/bumping a tracking issue for splitting commands/config.py (e.g. into _crud.py / _rows.py / _metadata.py) and config_service.py along the lines CONTRIBUTING.md already sketches for client.py.
Nits
[NIT-1]tests/test_config_row_cli.py:312—config row-updateonly gets a CLI-level "flag forwarded" test for--change-description, not a CLI-level--dry-run --jsontest (the siblingconfig updategets both, seetests/test_config_update.py'sTestConfigUpdateChangeDescriptionCli::test_dry_run_echoes_change_description). The dry-run-echo behavior for row-update is covered at the service layer (test_dry_run_includes_change_descriptionintests/test_config_row_service.py), so this is a symmetry nit, not a real gap.
Verification log
- Required reading:
Read CONTRIBUTING.md(full file — Code Quality Patterns, Checklist for new CLI commands, Plugin synchronization map, Releasing a new version) ✓;plugins/kbagent/agents/keboola-expert.md§1 + §2 + §3 (full file) ✓;CLAUDE.mdconvention #17 and## All CLI Commands(via project context + cross-checked against the diff) ✓. gh auth status→ authenticated aspadak, scopes includerepo✓.gh pr view 505 --json title,body,files,additions,deletions,...→ stateOPEN,feat/config-change-description→main, +400/-20 across 11 files, titlefeat(config): add --change-description to config update and row-update— conventional-commit prefix matches change type (new opt-in flag =feat) ✓.- Working-tree branch mismatch:
git rev-parse --abbrev-ref HEADin the review worktree returnedclaude/pr-505-review-e2efe3(still atmaintip), notfeat/config-change-description— the parent invocation did not check out the PR branch. Per my read-only mandate (nogit checkout/switchon the working tree), I compensated bygit fetch origin feat/config-change-description:refs/kbagent-review/pr-505-head(read-only ref fetch) andgit worktree add --detach <scratch>/pr-505-check refs/kbagent-review/pr-505-head(a fully separate, isolated worktree) to read files at PR HEAD and run checks — the original review worktree's branch/state never changed (re-verified after cleanup: stillclaude/pr-505-review-e2efe3, clean). Both the temp ref and temp worktree were removed at the end of the run. git merge-base main refs/kbagent-review/pr-505-head==git rev-parse main(dbcd32d8) → PR is fully rebased on currentmain, no drift.gh pr diff 505→ 693-line diff fetched and read in full;gh pr view --json files --jq→ matches the 11 files in the PR description exactly, nothing undisclosed in the diff.- 3-layer violation greps (typer/formatter in services, httpx/requests in commands, formatter/typer in clients) → all empty ✓ clean.
- Convention greps (magic numbers, raw
error_code="..."strings, bareexcept:,print(), token leakage, newtuple[...]returns) → all empty or manually-vetted false positives (testlambda url, token: mock_clientfactory boilerplate;passwordinside a pre-existing docs sentence about#-secret key naming) ✓ clean. Only one new production function (_default_change_description) — returnsstr, categorical arg first, no tuple. permissions.pyOPERATION_REGISTRY→"config.update": "write"(line 58) and"config.row-update": "write"(line 72) already present; PR adds no new command name so no new entry required.grep change_description src/keboola_agent_cli/client.py→ confirmsupdate_config/update_config_rowalready acceptchange_descriptionand map it todata["changeDescription"](lines ~1046-1067, ~1126-1148), matching the PR description's "Client: no change" claim.make checkin the isolated PR worktree (afteruv sync --extra server):ruff check→ "All checks passed!";ruff format --check→ 333 files already formatted;ty check→ 1 pre-existingunresolved-importwarning inscripts/hatch_build.py(not touched by this PR, warnings don't block per CONTRIBUTING.md);SKILL.md is up-to-date;version is in sync;check_command_sync.py→ "OK: all 238 CLI commands are registered (OPERATION_REGISTRY) and documented (CLAUDE.md, context.py, commands-reference.md)"; full test suite → 4379 passed, 8 skipped;changelog-check→ FAILED ("Missing changelog entries for: 0.72.0 (7 pre-release(s) skipped)"). This failure is not attributable to this PR: the PR touches nochangelog.py/pyproject.toml/version files (confirmed viagit diff --stat),changelog-checkis explicitly scoped by CONTRIBUTING.md as "local-only... a release-time concern, not a per-PR gate" and is excluded from.github/workflows/ci.yml, and the underlying gap (an already-tagged0.72.0with no backfilled changelog entry) predates and is independent of this diff.uv run pytest tests/test_config_update.py tests/test_config_row_cli.py tests/test_config_row_service.py tests/test_server_router_calls.py -k change_description -v→ 11/11 passed individually, exercising: CLI flag forwarding (both commands), default-preservation when omitted, dry-run JSON echo (both commands), andserverouter kwarg forwarding (both PATCH routes).- Code-path read (
commands/config.py:696-712forconfig update,:2320-2338forrow-update) → confirmed thechangeDescription:dim-line print fires only inside the--dry-run+ human-mode branch, gated byif change_desc:which is always truthy (default is never empty) — deterministic, matches the PR description's claim exactly. - Live/E2E reproduction against a real Keboola project was not performed —
--dry-runfor this command still makes a realget_config_detailAPI call, and credential-bearing runs are left to the PR author per this project's convention; the addedtests/test_e2e.pyblock (drives--change-descriptiondry-run + apply + verifies viaconfig detail) was read and is structurally sound but unexecuted by me.
Open questions for the author
(none)
padak
left a comment
There was a problem hiding this comment.
Approving — clean passthrough across the CLI → service → serve layers, the omitted-flag path preserves the previous auto-generated changeDescription default exactly (shared _default_change_description helper), and the full suite is green (make check, 4379 passed + the 11 new tests). No blocking issues.
One non-blocking follow-up before/around the release that ships this: please add a plugins/kbagent/skills/kbagent/references/gotchas.md entry distinguishing the two similarly-named flags — it's an easy trap for both humans and AI agents:
--change-description→ sets the new config version'schangeDescription(the version-history audit line)--description→ sets the config's display description
commands-reference.md and AGENT_CONTEXT (context.py) already cover the distinction; gotchas.md is the one silent-drift surface (CLAUDE.md convention #17) still missing it. Per the gotchas convention, tag the entry (since vX.Y.Z) with the release version this lands in.
Thanks!
What
Adds a
--change-description TEXToption tokbagent config updateandkbagent config row-updateso callers can set the Keboola config version'schangeDescription— the version-history audit line — instead of always getting the generic default (Updated ... via kbagent config update).Why
While applying a change to a production Python transformation via
kbagent config update ... --configuration @payload.json --merge, the new config version'schangeDescriptioncame out as the generic default. There was no flag to set a meaningful one. On shared production configs the version history is the paper trail, so a generic message is poor for auditability. Note--change-descriptionis distinct from--description, which sets the config's display description, not the version changeDescription.What changed
commands/config.py):--change-descriptiononconfig updateandconfig row-update, threaded to the service; help text + examples updated.services/config_service.py):update_config/update_config_rowtakechange_description: str | None; when omitted, the previous auto-generated default is preserved exactly (extracted into a shared_default_change_descriptionhelper). The effective value is included in the--dry-runresult.--dry-run: echoes the change description that would be sent (JSON keychange_description; human mode prints achangeDescription:line).kbagent serve(server/routers/configs.py): both PATCH routes mirror the flag (1:1 CLI<->REST convention).update_config/update_config_rowalready acceptedchange_description.kbagent context(AGENT_CONTEXT),CLAUDE.mdcommand list, andcommands-reference.mdupdated.Tests
update_configandupdate_config_row).CliRunner): flag forwarded to client;--dry-run --jsonechoes it (both commands).change_description=to the service.test_e2e.py): drivesconfig update --change-description(dry-run echo + real apply) and verifies the resulting version viaconfig detail->changeDescription.make checkis green (lint, format,ty, skill/version/command-sync/changelog/error-code checks, full non-e2e suite: 4379 passed).Scope / follow-up
Scoped to
config update+config row-update— the two write paths whose client methods already acceptchange_description, so this is a trivial passthrough. Extending to creates (config new,config row-create) andflow new/flow updateneeds new client parameters and is left as a follow-up.No version bump / changelog entry — releases are cut separately via dedicated release commits, and
changelog-checkvalidates existing releases only (stays green).