Skip to content

feat(config): add --change-description to config update and row-update#505

Open
jordanrburger wants to merge 1 commit into
mainfrom
feat/config-change-description
Open

feat(config): add --change-description to config update and row-update#505
jordanrburger wants to merge 1 commit into
mainfrom
feat/config-change-description

Conversation

@jordanrburger

Copy link
Copy Markdown
Collaborator

What

Adds a --change-description TEXT option to kbagent config update and kbagent config row-update so callers can set the Keboola config version's changeDescription — 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's changeDescription came 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-description is distinct from --description, which sets the config's display description, not the version changeDescription.

What changed

  • Command (commands/config.py): --change-description on config update and config row-update, threaded to the service; help text + examples updated.
  • Service (services/config_service.py): update_config / update_config_row take change_description: str | None; when omitted, the previous auto-generated default is preserved exactly (extracted into a shared _default_change_description helper). The effective value is included in the --dry-run result.
  • --dry-run: echoes the change description that would be sent (JSON key change_description; human mode prints a changeDescription: line).
  • kbagent serve (server/routers/configs.py): both PATCH routes mirror the flag (1:1 CLI<->REST convention).
  • Client: no change — update_config / update_config_row already accepted change_description.
  • Docs: kbagent context (AGENT_CONTEXT), CLAUDE.md command list, and commands-reference.md updated.

Tests

  • Service: explicit value forwarded; omitted -> default preserved; dry-run includes it (both update_config and update_config_row).
  • CLI (CliRunner): flag forwarded to client; --dry-run --json echoes it (both commands).
  • Router: PATCH routes forward change_description= to the service.
  • E2E (test_e2e.py): drives config update --change-description (dry-run echo + real apply) and verifies the resulting version via config detail -> changeDescription.

make check is 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 accept change_description, so this is a trivial passthrough. Extending to creates (config new, config row-create) and flow new/flow update needs 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-check validates existing releases only (stays green).

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.
@jordanrburger
jordanrburger requested a review from padak July 20, 2026 17:47

@padak padak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review of #505 — feat(config): add --change-description to config update and row-update

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make 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:312config row-update only gets a CLI-level "flag forwarded" test for --change-description, not a CLI-level --dry-run --json test (the sibling config update gets both, see tests/test_config_update.py's TestConfigUpdateChangeDescriptionCli::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_description in tests/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.md convention #17 and ## All CLI Commands (via project context + cross-checked against the diff) ✓.
  • gh auth status → authenticated as padak, scopes include repo ✓.
  • gh pr view 505 --json title,body,files,additions,deletions,... → state OPEN, feat/config-change-descriptionmain, +400/-20 across 11 files, title feat(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 HEAD in the review worktree returned claude/pr-505-review-e2efe3 (still at main tip), not feat/config-change-description — the parent invocation did not check out the PR branch. Per my read-only mandate (no git checkout/switch on the working tree), I compensated by git fetch origin feat/config-change-description:refs/kbagent-review/pr-505-head (read-only ref fetch) and git 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: still claude/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 current main, 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, bare except:, print(), token leakage, new tuple[...] returns) → all empty or manually-vetted false positives (test lambda url, token: mock_client factory boilerplate; password inside a pre-existing docs sentence about #-secret key naming) ✓ clean. Only one new production function (_default_change_description) — returns str, categorical arg first, no tuple.
  • permissions.py OPERATION_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 → confirms update_config/update_config_row already accept change_description and map it to data["changeDescription"] (lines ~1046-1067, ~1126-1148), matching the PR description's "Client: no change" claim.
  • make check in the isolated PR worktree (after uv sync --extra server): ruff check → "All checks passed!"; ruff format --check → 333 files already formatted; ty check → 1 pre-existing unresolved-import warning in scripts/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-checkFAILED ("Missing changelog entries for: 0.72.0 (7 pre-release(s) skipped)"). This failure is not attributable to this PR: the PR touches no changelog.py/pyproject.toml/version files (confirmed via git diff --stat), changelog-check is 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-tagged 0.72.0 with 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 -v11/11 passed individually, exercising: CLI flag forwarding (both commands), default-preservation when omitted, dry-run JSON echo (both commands), and serve router kwarg forwarding (both PATCH routes).
  • Code-path read (commands/config.py:696-712 for config update, :2320-2338 for row-update) → confirmed the changeDescription: dim-line print fires only inside the --dry-run + human-mode branch, gated by if 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-run for this command still makes a real get_config_detail API call, and credential-bearing runs are left to the PR author per this project's convention; the added tests/test_e2e.py block (drives --change-description dry-run + apply + verifies via config detail) was read and is structurally sound but unexecuted by me.

Open questions for the author

(none)

@padak padak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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's changeDescription (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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants