feat(0.22.0): config metadata CRUD + workspace orphan GC (FIIA P1-3, P1-4) - #204
Closed
ottomansky wants to merge 13 commits into
Closed
feat(0.22.0): config metadata CRUD + workspace orphan GC (FIIA P1-3, P1-4)#204ottomansky wants to merge 13 commits into
ottomansky wants to merge 13 commits into
Conversation
…P1-4) Config metadata (P1-3): - config metadata-list: list all metadata entries on a config (key-sorted) - config get-metadata --key K: fetch a single value; exits 1 if absent - config set-metadata --key K --value V: upsert a metadata entry - config delete-metadata --metadata-id ID: delete by numeric ID (with confirmation) - config set-folder --name NAME: sugar over KBC.configuration.folderName All metadata commands auto-resolve the default branch via /dev-branches when no --branch or active_branch_id is set (the Storage API metadata endpoint is branch-aware-only, no non-branch route). Workspace GC (P1-4): - workspace list --orphaned: lists keboola.sandboxes workspaces whose sandbox config no longer exists - workspace gc [--dry-run] [--yes]: deletes all orphaned workspaces; dry-run previews the list without touching anything; --yes bypasses confirmation Tests: 62 new unit tests (test_config_metadata.py, test_workspace_gc.py) + standalone E2E classes TestE2EPR8ConfigMetadata and TestE2EPR8WorkspaceGC validated against a real Keboola stack.
…risk, add ConfigError tests - _resolve_metadata_branch_id: catch bare Exception in addition to KeboolaApiError so network-level errors from list_dev_branches produce structured ConfigError output - list/set/delete_config_metadata: move return dict inside try block to eliminate latent UnboundLocalError if _resolve_metadata_branch_id raises - delete_workspace: initialise component=None before inner try (latent UnboundLocalError) - workspace list command: add KeboolaApiError handler (consistent with all other workspace cmds) - test_config_metadata: add ConfigError (exit 5) test for each of the 5 CLI commands - test_workspace_gc: add dry-run-without-yes coverage test
- test_gc_no_confirmation_aborts: was a duplicate of test_gc_dry_run_json; rewritten to actually test the non-JSON abort path (user inputs 'n', exit 0, service not called) - test_gc_delete_error_accumulated: >= 1 tightened to == 1 (one orphan → one error)
- test_gc_deletes_orphan_and_sandbox_config: add delete_config assertion (the test name claimed to verify sandbox-config cleanup but never asserted it) - test_gc_delete_error_accumulated: replace misleading get_workspace side_effect with a normal return_value; the error being accumulated comes from delete_workspace 500, not from the swallowed get_workspace 404 - test_set_folder_empty_clears: assert response data['folder'] == '' (was missing)
…exception catch - config_metadata_list: remove redundant 'from rich.markup import escape as _esc' inside loop; use module-level escape(); also escape the id field for consistency - gc_workspaces: widen per-orphan except to bare Exception so network errors (ConnectionError, TimeoutError, etc.) are accumulated rather than crashing the loop
padak
previously requested changes
Apr 22, 2026
padak
dismissed
their stale review
April 22, 2026 21:41
Replacing with English version for broader team visibility.
padak
requested changes
Apr 22, 2026
padak
left a comment
Member
There was a problem hiding this comment.
Changes requested — 3 required fixes per CONTRIBUTING.md
Architecturally clean, tests complete (37 unit + 5 E2E), but three explicitly required artifacts are missing:
Required fixes
1. CLAUDE.md — the ## All CLI Commands section does not list the new commands (CONTRIBUTING.md line 202). Add:
kbagent config metadata-list --project NAME --component-id ID --config-id ID [--branch ID]
kbagent config get-metadata --project NAME --component-id ID --config-id ID --key KEY [--branch ID]
kbagent config set-metadata --project NAME --component-id ID --config-id ID --key KEY --value VALUE [--branch ID]
kbagent config delete-metadata --project NAME --component-id ID --config-id ID --metadata-id ID [--yes] [--branch ID]
kbagent config set-folder --project NAME --component-id ID --config-id ID --folder FOLDER [--branch ID]
kbagent workspace list [--project NAME] [--orphaned]
kbagent workspace gc [--project NAME] [--dry-run] [--yes]
(Verify signatures against your actual @typer.Option definitions.)
2. plugins/kbagent/skills/kbagent/references/commands-reference.md — same commands missing (CONTRIBUTING.md line 204: "this is a hand-maintained file, NOT auto-generated").
3. Rich escape in src/keboola_agent_cli/commands/workspace.py:491-502 — commit b4c5523 covered only config metadata; workspace gc was left unescaped:
# Lines ~491-502 — user-controlled fields in Rich markup:
f"([cyan]{ws.get('name', '')}[/cyan]) in '{ws['project_alias']}'"
f"[green]deleted[/green] workspace {ws['id']} in '{ws['project_alias']}'"
f"[red]error[/red] workspace {err.get('workspace_id', '?')}: {err.get('error', '')}"Wrap ws['name'], ws['project_alias'], err['error'] with escape() from rich.markup. Workspace name comes from keboola.sandboxes config name (user-controlled), error from API response — potential Rich markup injection.
Nice-to-have (follow-up)
logger.exception()inside thegc_workspacescatch block (src/keboola_agent_cli/services/workspace_service.py:398) — todayexcept Exception as exc:silently swallows programmer errors too (AttributeError etc.). Observability +1.- Consider a
config-metadata-workflow.mdreference — folder organization is a new agent-facing workflow pattern that deserves its own doc. - Unit test for the metadata-extraction enhancement in
get_bucket_detail/get_table_detail(only E2E-covered today).
Positives
_resolve_metadata_branch_id(explicit → active → default) with actionable error messages is elegant.- Both test files (
test_config_metadata.py662 lines +test_workspace_gc.py382 lines) are thorough. - FIIA P1-3 / P1-4 in the changelog entry, commit history shows a healthy review cycle (6 fix commits after the initial implementation).
Add the five config metadata CRUD commands (metadata-list, get-metadata, set-metadata, delete-metadata, set-folder) and the new workspace list --orphaned / workspace gc entries to the All CLI Commands section so agents see the full inventory (CONTRIBUTING.md line 202). Signatures verified against the actual @typer.Option definitions: set-folder uses --name (not --folder) and workspace list/gc use a repeatable --project; these differ from the review template and have been mirrored to the real flags.
…ference commands-reference.md is a hand-maintained file (CONTRIBUTING.md line 204) that agents read via the kbagent skill. Add matching entries for: - config metadata-list / get-metadata / set-metadata / delete-metadata - config set-folder (with a pointer to config-metadata-workflow.md) - workspace list --orphaned (updated the existing entry) - workspace gc --dry-run / --yes (new entry) Flag spelling follows the actual @typer.Option definitions; notably set-folder takes --name (not --folder) and workspace list/gc accept a repeatable --project.
…ields Commit b4c5523 escaped user-controlled fields in config metadata output but missed the workspace gc rendering block. Workspace name comes from the keboola.sandboxes config name (user-controlled) and the error string comes from the Keboola API response -- both reach Rich markup rendering unescaped, allowing injection like "[red]bad[/red]" in a workspace name to hijack formatting. Wrap the three user-controlled interpolations with rich.markup.escape(): - ws.get("name", "") in the --dry-run branch - ws["project_alias"] in both the --dry-run and delete branches - err.get("error", "") in the error reporting branch Static strings, numeric IDs, and literal markup tags are left alone.
…rvability The per-orphan catch in gc_workspaces is a broad "except Exception" to guarantee we never abort the sweep on an unexpected error. That's correct for the user-facing flow (the error gets recorded and the next orphan is processed), but it also silently swallows programmer errors like AttributeError -- so bugs in the delete path would show up only as a vague "error: <str>" line with no traceback. Add logger.exception() inside the catch so the full traceback is logged while leaving the user-facing flow unchanged (the error is still accumulated into delete_errors). The module already has a logger at the top.
Config metadata CRUD is a new agent-facing surface (introduced by PR keboola#204) and folder organization via KBC.configuration.folderName is a pattern that deserves its own doc rather than buried in the commands reference. The workflow covers: - overview and when to use metadata vs config update - CLI cheatsheet for all five commands - folder organization pattern using set-folder (with bulk example) - full lifecycle example (list -> set -> get -> set-folder -> delete) - --json response shapes for each command - provider semantics (user vs system) - branch awareness
Contributor
Author
|
All three required fixes + two of three nice-to-haves:
Two small signature deviations in the docs vs. your template — both reflect actual
Re: unit tests for Targeted tests: 96/96 pass. |
padak
added a commit
that referenced
this pull request
Apr 23, 2026
New: config metadata-list/get/set/delete-metadata -- CRUD for arbitrary key/value pairs on any configuration via branch-aware Storage API metadata endpoint (FIIA P1-3). New: config set-folder -- sugar over set-metadata for KBC.configuration. folderName; organises configs into named folder groups. New: workspace list --orphaned and workspace gc -- detect and clean up sandbox-backed workspaces whose sandbox config no longer exists (FIIA P1-4). Max's review fixes: CLAUDE.md + commands-reference docs, Rich markup escape in GC output, logger.exception on gc catch, config-metadata-workflow.md.
Member
|
Integrated into Full integration PR coming as release/0.22.0 -> main. |
padak
added a commit
that referenced
this pull request
Apr 23, 2026
- sync-workflow.md: new "Adopting an existing kbc Go CLI checkout" section for sync init --adopt-existing (#201). - gotchas.md: new entries for exit code 7 (JOB_TIMEOUT_TERMINATED), --poll-strategy fixed, --log-tail-lines N, logTail response semantics (#202); new section for --deny-writes / --deny-destructive (#203); new section for sync init --adopt-existing (#201). - workspace-workflow.md: new "Orphan detection + garbage collection" section for workspace list --orphaned + workspace gc (#204). - commands-reference.md: sync init --adopt-existing flag documented. - README.md: "What it does" table + "All commands" block updated with the 15 new commands shipped in 0.22.0 (flow CRUD, config metadata, workspace GC, storage describe, project pin + firewall, queue polling parity).
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
config metadata-list/get-metadata/set-metadata/delete-metadata): full CRUD for arbitrary metadata key/value pairs on any configuration, using the branch-aware Storage API metadata endpoint. Branch auto-resolves from default when--branchis omitted.config set-folder --name NAME): sugar overset-metadataforKBC.configuration.folderName; organises configs into named folder groups visible in the Keboola UI.workspace list --orphaned): listskeboola.sandboxes-backed workspaces whose sandbox config no longer exists.workspace gc [--dry-run] [--yes]): deletes all orphaned workspaces;--dry-runpreviews without touching anything;--yesskips interactive confirmation.Changes
client.py—list_config_metadata,set_config_metadata,delete_config_metadataconfig_service.py— 5 metadata methods +_resolve_metadata_branch_idhelperworkspace_service.py—_is_orphaned_workspace,list_workspaces(orphaned_only),gc_workspacescommands/config.py— 5 new commands;commands/workspace.py—--orphanedflag +gcpermissions.py— 6 new entries;hints/definitions/config.py+workspace.pycommands/context.py— updated agent reference docsSKILL.mdregenerated (+6 rows);plugin.jsonbumped to 0.22.0Test plan
tests/test_config_metadata.pyandtests/test_workspace_gc.py— all passingTestE2EPR8ConfigMetadata(3 tests) andTestE2EPR8WorkspaceGC(1 test) validated against real Keboola stackruff check+ruff format --checkclean0.21.1→0.22.0;make version-syncapplied;SKILL.mdregenerated