Skip to content

feat(0.23.0): config detail --component-id bulk + --with-state + config list --include-rows (#197) - #218

Merged
padak merged 4 commits into
mainfrom
feat/197-bulk-config-detail
Apr 23, 2026
Merged

feat(0.23.0): config detail --component-id bulk + --with-state + config list --include-rows (#197)#218
padak merged 4 commits into
mainfrom
feat/197-bulk-config-detail

Conversation

@padak

@padak padak commented Apr 23, 2026

Copy link
Copy Markdown
Member

Summary

Closes #197. Adds three first-class features to kbagent config that expose the bulk listing path (already consumed internally by sync pull / config search) as direct CLI/JSON surface. All additive; single-config JSON shape preserved exactly for backward compatibility.

  1. config detail --component-id ID (omit --config-id) -> BULK MODE. Returns {"configs": [...], "errors": [...]} with every configuration of the given component across one or many projects. --project is repeatable. One HTTP request per project (via list_components_with_configs(include=configuration,rows)), not one per config -- 100+ Snowflake writers come back in a single round-trip per project.
  2. config list --include-rows. Extends each row with the full configuration + rows body. Opt-in because the payload is noticeably larger.
  3. config detail --with-state. Attaches the runtime state dict (incremental sync cursors, auth refresh tokens, OAuth intermediate state) under a state key. Single mode -> state is read directly from the same get_config_detail response (no extra HTTP call -- Storage API embeds state inline and has no separate state resource; the earlier get_config_state double-fetch was redundant and was removed in review-feedback commit 2102697); bulk mode -> rides inline via include=state on the listing request (still one call per project, no N+1). Security note: --with-state output may carry OAuth tokens / refresh tokens from credential-bearing components -- the CLI help emits a WARNING, and UNEXPECTED_ERROR envelopes truncate exception messages to 256 chars to prevent token fragments from leaking through (CWE-209 mitigation).

Motivation (from the issue)

@ondrej-lorenc audited 14 projects and needed config details (rows + state) for 102 Snowflake writers. The underlying Storage API endpoint (GET /components?include=configuration,rows) exposes bulk data but was only consumed internally by sync pull / config search. Before this PR, audits forked 102 parallel subprocess calls to config detail --config-id .... Now a single config detail --project P --component-id keboola.wr-db-snowflake returns all 102 in one request.

Design decisions

Backward compatibility is load-bearing

Single-config JSON shape is untouched. Passing --config-id keeps the original flat dict (.id, .name, .configuration, .rows, ...). Existing callers (scripts, agents parsing data.configuration etc.) are unaffected. A regression test locks this in: TestConfigDetail::test_config_detail_single_shape_preserved_for_backward_compat asserts configs is NOT present in the single-mode response.

--config-id with multiple --project is rejected (exit 2)

A single configuration lives in exactly one project. The CLI refuses to silently pick the first alias or fan out. Instead it exits with INVALID_ARGUMENT, message: "--config-id is only valid with exactly one --project. Omit --config-id to fan out across multiple projects." This matches the multi-project discipline established by storage tables (v0.21.2).

--branch requires exactly one --project (both modes)

Branch IDs are per-project; mixing bulk-with-branch across projects would conflate meanings. The service layer also enforces this (ConfigError raised in get_config_detail when branch_id is set with multiple aliases).

State endpoint URL rationale

The Keboola Storage API does not expose a standalone GET .../configs/{id}/state resource:

  • Production route (/v2/storage/components/{c}/configs/{id}/state): returns 404 resource not found.
  • Branch-scoped route (/v2/storage/branch/{b}/components/{c}/configs/{id}/state): returns 501 Not Implemented.

State is only served as a field inside the configuration detail response AND as an opt-in via include=state on the listing endpoint (same mechanism keboola-mcp-server/clients/storage.py uses). The new KeboolaClient.get_config_state is retained as a convenience wrapper over get_config_detail(...).get("state", {}) for discoverability, but callers that already have a detail response (the service layer's single-mode --with-state path does) read state directly from it -- issuing a second GET against the same URL would be pure waste. Documented in the method docstring and in gotchas.md so the next contributor doesn't wonder why .../state is missing.

Parallelism bounds

Bulk state fetching uses include=state on the listing call (one request per project, not per config). Cross-project fan-out uses BaseService._run_parallel (existing infra) with the standard ThreadPoolExecutor. Worker count = min(len(projects), KBAGENT_MAX_PARALLEL_WORKERS), default 10, overridable via env var or config.json's max_parallel_workers. Per-project failures are captured in errors[] without aborting other projects (same pattern as storage tables, lineage build).

Why config list --include-rows and config detail --component-id are semantically distinct

Both hit list_components_with_configs(include=configuration,rows) under the hood, but:

  • config list --include-rows returns the full set of configs across all components (filtered optionally by --component-type or --component-id). Use case: bulk audit across components.
  • config detail --component-id (bulk) returns the configs of one component with a richer shape (name, description, configuration, rows, rowsSortOrder, version, isDisabled, isDeleted, changeDescription, created, currentVersion, creatorToken). Use case: deep dive on one component type.

Documented in gotchas.md so the choice is explicit.

Architecture / CONTRIBUTING compliance

  • Client layer (client.py): new get_config_state(component_id, config_id, branch_id); existing list_components_with_configs extended with include_state: bool.
  • Service layer (config_service.py): get_config_detail accepts optional config_id, with_state, aliases. New private helpers _get_config_detail_bulk + _fetch_project_component_configs follow the _run_parallel 3-tuple success / 2-tuple error convention. list_configs gained include_rows.
  • Command layer (commands/config.py): thin wiring only; all business logic stays in services. Human-mode bulk render grouped by project via _format_config_detail_bulk.
  • --hint client + --hint service: hints updated in hints/definitions/config.py to branch on --config-id presence.
  • SKILL.md: regenerated via make skill-gen.
  • Docs: CLAUDE.md, commands-reference.md, AGENT_CONTEXT updated. gotchas.md gained three entries documenting (a) the bulk-mode array shape, (b) the --include-rows payload-size tradeoff, and (c) the --with-state fetch mechanism.
  • Tests: 15 new service-layer tests + 15 new CLI tests + 6 new client tests + E2E coverage for all five scenarios (bulk detail, single-shape preservation, --with-state single mode, --with-state bulk mode, --include-rows, --config-id+multi---project rejection). make check passes (2165 tests total).

Live verification (against /tmp/kosik, padak project, 13 ex-generic-v2 configs)

# Bulk detail (no --config-id): 1 API call, 13 configs in ~1.4s
kbagent --json config detail --project padak --component-id ex-generic-v2
  -> {"status": "ok", "data": {"configs": [...13 rows...], "errors": []}}

# Single detail (--config-id): backward-compat shape preserved
kbagent --json config detail --project padak --component-id ex-generic-v2 --config-id 249225269
  -> {"status": "ok", "data": {"id": "249225269", "name": "...", "configuration": {...}, ...}}
  # No `configs` wrapper; flat dict.

# --with-state (bulk): all 13 configs get state inline, no N+1
kbagent --json config detail --project padak --component-id ex-generic-v2 --with-state
  -> 13 configs, each with "state": {...}; real runtime state visible on configs that have run

# --include-rows: full configuration body attached per row
kbagent --json config list --project padak --component-id ex-generic-v2 --include-rows
  -> configs[].configuration = {...}, configs[].rows = [...]

# Multi-project: fans out, per-row project_alias tagged
kbagent --json config detail --project padak --project kbagent-e2e --component-id ex-generic-v2
  -> configs from both projects in one envelope, errors[] empty

Test plan

  • make check green
  • Backward-compat regression test (test_config_detail_single_shape_preserved_for_backward_compat) passes
  • Service-layer tests: bulk single-project, bulk multi-project, partial failure, with_state, empty result, branch-rejected-with-multi-project
  • CLI tests: bulk envelope shape, multi-project forwarding, --config-id+multi-project rejection, --with-state flag forwarding, --include-rows flag forwarding, --project required
  • Client tests: get_config_state (populated / empty / branched), list_components_with_configs(include_state=...)
  • E2E tests against real API: bulk detail, single-shape preservation, --with-state single/bulk, --include-rows
  • Live /tmp/kosik sanity-check: all five variants return the expected shapes, bulk mode issues a single HTTP call, state data visible on configs that have run
  • --hint client + --hint service render correctly for both modes
  • Human-mode bulk output renders per-project grouped Rich tables

Review-pass fixes (review replied inline — see this comment)

  • B1 (blocking): single-mode --with-state no longer issues the redundant second GET against /components/{c}/configs/{id}. Regression test locks mock_client.get_config_state.call_count == 0.
  • B2 (HIGH CWE-209): UNEXPECTED_ERROR envelopes truncate exception messages to 256 chars + trailing .... Applied at 3 sites (_fetch_project_configs, _fetch_project_component_configs, BaseService._run_parallel). Full exception still reaches debug log.
  • M1: --with-state help text carries a WARNING about OAuth / refresh tokens.
  • M3: empty / whitespace-only --component-id rejected up front with exit 2.
  • L2: component_id escaped with rich.markup.escape() in human-mode bulk render.
  • A1: bumped to 0.23.0; changelog entry added.
  • A2: bulk mode derives component_type from keboola.{ex,wr,app}-* / keboola.*-transformation prefixes and forwards it to the listing endpoint so the API returns a narrower payload.
  • A3: list_components_with_configs docstring mentions the bulk-detail --with-state caller.

…rows (#197)

Closes #197. Audit-use-case: ondrej-lorenc audited 14 projects looking
at 102 Snowflake writers. The existing `config detail` was single-config,
forcing 102 parallel subprocess calls. This PR exposes the bulk listing
path (already consumed internally by `sync pull` and `config search`) as
a first-class `config detail` / `config list` surface.

## Three enhancements (all additive, zero breaking changes)

1. `config detail --component-id ID` (omit --config-id) -> BULK MODE.
   Returns `{"configs": [...], "errors": [...]}` with every configuration
   of the given component across one or many projects. `--project` is
   repeatable. One HTTP request per project (via
   `list_components_with_configs(include=configuration,rows)`), not one
   per config -- 100 Snowflake writers return in a single round-trip.

2. `config list --include-rows`. Extends each row with the full
   `configuration` + `rows` body by switching the service from
   `list_components` (summary) to `list_components_with_configs` (full).
   Opt-in because the payload is noticeably larger.

3. `config detail --with-state`. Attaches the runtime state dict
   (incremental sync cursors, auth refresh tokens, OAuth intermediate
   state) under a `state` key. Single mode: dedicated call via
   `get_config_state`. Bulk mode: rides along via `include=state`
   on the listing request -- still ONE call per project, no N+1.

## Design decisions

- **Single-config JSON shape unchanged.** Passing `--config-id` keeps the
  original flat dict (`.id`, `.name`, `.configuration`, `.rows`, ...).
  Existing callers are unaffected. A regression test covers this
  (`test_config_detail_single_shape_preserved_for_backward_compat`).

- **`--config-id` with multiple `--project` is rejected (exit 2 /
  `INVALID_ARGUMENT`).** A single config lives in exactly one project;
  the CLI refuses to guess. Drop `--config-id` for multi-project fan-out.

- **`--branch` requires exactly one `--project`** in both modes (branch
  IDs are per-project; mixing bulk-with-branch across projects would
  conflate meanings).

- **State endpoint URL rationale.** The Keboola Storage API does not
  expose a standalone `GET .../configs/{id}/state` resource: it returns
  404 on production and 501 Not Implemented under `/branch/{id}/...`.
  State only rides as a field inside the configuration detail (and as
  an opt-in via `include=state` on the listing endpoint). The new
  `client.get_config_state` therefore reads the detail endpoint and
  returns `response.get("state", {})` -- semantically "fetch the state",
  implemented against the only URL that actually serves it.

- **Bulk state via `include=state`, not N+1.** `list_components_with_configs`
  now accepts `include_state=True`, which toggles `include=state` on the
  query. One request serves every config's state per project, even with
  hundreds of configs. Parallelism across projects uses the existing
  `BaseService._run_parallel` thread pool (default 10 workers, override
  via `KBAGENT_MAX_PARALLEL_WORKERS` env var or `config.json`).

## Architecture / CONTRIBUTING compliance

- Client layer: new `get_config_state`, existing
  `list_components_with_configs` extended with `include_state`.
- Service layer: `get_config_detail` now accepts optional `config_id`,
  `with_state`, `aliases`. New private helpers `_get_config_detail_bulk`
  and `_fetch_project_component_configs` follow the `_run_parallel` +
  3-tuple success / 2-tuple error convention established by
  `storage_service.list_tables`. `list_configs` gained `include_rows`.
- Command layer: thin wiring only; human-mode bulk render grouped by
  project via `_format_config_detail_bulk`.
- `--hint client` and `--hint service` updated; both modes render.
- `make skill-gen` regenerated SKILL.md.
- Docs updated: CLAUDE.md, commands-reference.md, gotchas.md (three
  entries: bulk mode, --include-rows payload warning, --with-state
  parallelism bounds), AGENT_CONTEXT.
- Tests: 15 new service-layer + 15 new CLI tests + 6 new client tests +
  E2E coverage for bulk detail / single-shape preservation /
  --with-state (single + bulk) / --include-rows / --config-id+multi
  --project rejection. `make check` green (2165 tests pass).

## Live verification (/tmp/kosik against padak project, 13 ex-generic-v2 configs)

- Bulk detail (no --config-id): 13 configs returned in 1 API call,
  `configs[]`/`errors[]` envelope, per-row `project_alias` tagged.
- Single detail (unchanged): top-level `id`, `name`, `configuration`,
  `rows`, `state` present; no `configs` wrapper.
- `--with-state` bulk: 13 configs with state key attached; real state
  payload visible on configs that have run (e.g. `state.component`).
- `--include-rows`: full `configuration`/`rows` bodies attached per row.
- Multi-project: fans out across projects, per-project `project_alias`
  tagging preserved.

@padak padak left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Review PR #218 -- overall

Nicely contained feature PR. Three-layer split (client / service / commands) is respected end-to-end, backward compatibility is locked in by a dedicated regression test, and the bulk-state-via-include=state path is the right optimization. Gotchas doc and hints are updated everywhere they should be. CI is green. Below are a few observations, one of them worth fixing before merge.

Main finding: redundant HTTP call in single-mode --with-state

src/keboola_agent_cli/client.py:334-347 vs :359-404 -- get_config_detail and get_config_state both issue the exact same GET /v2/storage[/branch/{b}]/components/{c}/configs/{id}. The only difference is that get_config_state extracts the state key from the body afterwards.

src/keboola_agent_cli/services/config_service.py:339-350 -- single-mode then calls both in sequence:

detail = client.get_config_detail(component_id, config_id, branch_id=effective_branch_id)
if with_state:
    # Refresh state via the dedicated client method so the
    # returned dict always carries the latest runtime state,
    # not just whatever snapshot the detail endpoint returned.
    detail["state"] = client.get_config_state(
        component_id, config_id, branch_id=effective_branch_id
    )

That comment is slightly misleading -- the second call hits the very same URL as the first, so it doesn't return a "fresher" state in any meaningful sense, it just pays for a second round-trip. Two easy fixes:

  1. Trust the first response (minimal change):

    if with_state:
        detail.setdefault("state", {})
        if not isinstance(detail["state"], dict):
            detail["state"] = {}

    and update the docstring at config_service.py:259-263 so it no longer claims get_config_state is "a dedicated refresh".

  2. Drop get_config_state altogether and document on list_components_with_configs(include_state=True) + get_config_detail that state rides inline -- you already have the include=state path for bulk, and the detail endpoint already returns it for single. The client method is effectively an alias for get_config_detail(...).get("state", {}); keeping it mostly exists to match the "conceptual" API we wish existed. That's a reasonable rationale, but then single-mode should use the detail response directly and not re-fetch.

If you prefer keeping the dedicated method for semantic clarity (the gotchas.md entry makes this argument well), then the service layer should still just read state from the already-fetched detail instead of re-calling the client -- basically inline option 1.

Minor observations

  1. component_type isn't propagated in bulk mode -- services/config_service.py:412-414 calls list_components_with_configs(branch_id=..., include_state=...) without component_type, then filters to component_id in memory. For a project with many component types (and where you only want one specific component), passing component_type=<derived> to the API would shrink the response. You can derive the type from the known keboola.ex-* / keboola.wr-* / keboola.app-* / keboola.*-transformation conventions, or accept it as an optional CLI flag. Not a blocker, just a potential win for large multi-tenant audits (which is the whole point of this PR).

  2. Double validation in CLI + service -- commands/config.py:255-267 rejects --config-id + multi --project and --branch + multi --project, and services/config_service.py:325-337, 355-360 does the same. Defensive, but both layers should be kept in sync if the rules ever change. Consider centralizing in the service and having CLI surface the resulting ConfigError with exit code 2 -- or explicitly comment that the CLI copy is the "fail-fast before resolving anything" layer. (Matter of taste; not blocking.)

  3. alias argument is silently ignored when aliases is given -- services/config_service.py:243-246, 336 -- the docstring flags this, and the CLI always passes both (alias=project[0], aliases=project). Works, but the API is slightly confusing for other service-layer callers. A cleaner split would be a separate get_config_detail_bulk(aliases=..., component_id=..., ...) public method so both callers pass exactly what they need (_get_config_detail_bulk exists as private already -- consider promoting it). Would also reduce the existing method's cyclomatic complexity.

  4. Test assertion robustness -- tests/test_cli.py:1237 asserts list(call.kwargs.get("aliases") or []) == ["prod", "stage"]. The or [] guard hides the case where the kwarg is accidentally not forwarded at all; a direct == ["prod", "stage"] would fail more loudly. Trivial.

  5. list_components_with_configs docstring at client.py:257-259 still says it fetches "everything needed for sync pull and for deep search". With the new include_state parameter this is now a superset of those use cases -- worth a one-line nod to the new bulk-detail caller so future readers don't trip over why state is being fetched during a "sync" call.

What's done well

  • Backward compatibility is load-bearing and has a dedicated regression test (test_config_detail_single_shape_preserved_for_backward_compat). Good guard.
  • gotchas.md entries are the right shape -- they answer "why" (404 / 501 on the missing /state endpoint), not just "what". That's exactly what saves the next contributor from re-discovering dead routes.
  • Bulk state via include=state avoids the N+1 trap neatly and the test test_get_config_detail_bulk_with_state verifies the single call.
  • Error accumulation follows the existing storage tables / lineage build pattern -- one bad project doesn't take the whole batch down.
  • E2E coverage for all five new scenarios (bulk detail / single-shape preservation / --with-state single+bulk / --include-rows) is the right bar.

Recommendation

Ship-ready once the redundant get_config_state call in single mode is either removed or justified with a more accurate docstring. The other items are polish.

@padak padak left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Follow-up: security review + CONTRIBUTING.md compliance

Ran a dedicated security review (bandit + pip-audit + manual walk of the diff) and cross-checked the PR against the "Adding a New CLI Command" checklist in CONTRIBUTING.md. Details below.


Security review

Verdict: safe to merge, with one H-severity item worth fixing before merge and a couple of polish items that can land in a follow-up.

No CRITICAL findings. No new dependencies were introduced (pip-audit clean). URL construction in the two new/modified client methods (get_config_state, list_components_with_configs) correctly uses quote(x, safe=""), branch ID is typed int | None, and each parallel worker uses its own project-scoped client, so cross-project token leakage through the worker path is not possible. E2E_API_TOKEN/E2E_URL are env-loaded, not hardcoded. Permission engine classification (config.list/config.detail = read) remains appropriate -- --with-state returns more sensitive data, but the Storage API token itself already governs what state data the caller can access.

HIGH

H1 -- UNEXPECTED_ERROR handler surfaces raw str(exc) into JSON output (CWE-209 Information Exposure Through an Error Message)

  • services/config_service.py:458-465 (new _fetch_project_component_configs)
  • Same pattern at config_service.py:163-170 (pre-existing _fetch_project_configs, modified in this PR) and services/base.py:139-141 (outer _run_parallel handler)
except Exception as exc:
    return (
        alias,
        {
            "project_alias": alias,
            "error_code": "UNEXPECTED_ERROR",
            "message": str(exc),  # untruncated, unsanitized
        },
    )

Unlike the KeboolaApiError branch above (where exc.message is pre-sanitized by _raise_api_error() in http_base.py), a bare Exception here can be anything: httpx transport errors that embed URLs with query params, standard-library formatting of internal state, or in rare edge cases fragments of a response buffer. With the new --with-state path, that response buffer can contain OAuth refresh tokens. Truncating + optionally logging the full detail at DEBUG would close the gap:

except Exception as exc:
    raw = str(exc)
    safe_msg = (raw[:256] + "...") if len(raw) > 256 else raw
    logger.debug("Unexpected error for %s: %s", alias, exc)
    return (alias, {"project_alias": alias, "error_code": "UNEXPECTED_ERROR", "message": safe_msg})

Worth applying consistently to all three call sites while you're in there.

MEDIUM

M1 -- --with-state help text should warn the caller about credential-bearing payloads (CWE-312)

commands/config.py:196-202. The help text mentions "auth refresh tokens, OAuth intermediate state" but not that the resulting JSON output should be handled as sensitive data (i.e. don't pipe it into logs / scratch files / shared workspaces). No code change, just a one-line WARNING: in the help string.

M2 -- Pre-existing quote(component_id) inconsistency across client.py (CWE-22, out-of-scope flag)

client.py:330, 517, 557, 581, 617, 636 use quote(component_id) without safe="", which retains /. The two methods this PR adds/extends already use safe="" correctly. Flagging here so the inconsistency is on record; not required for this PR.

M3 -- Empty --component-id passes validation (CWE-20)

commands/config.py:180. Typer ... enforces presence but not non-emptiness, so --component-id "" reaches the service and constructs a malformed URL. API will reject with 4xx, but a CLI-side fail-fast would produce a cleaner error:

if not component_id.strip():
    formatter.error(message="--component-id must not be empty.", error_code=ErrorCode.INVALID_ARGUMENT)
    raise typer.Exit(code=2)

LOW

  • L1 -- creatorToken dict forwarded verbatim in bulk rows (config_service.py:443). Description/id only; no raw token value. Consistent with pre-existing single-config detail output. Informational.
  • L2 -- _format_config_detail_bulk interpolates component_id into Rich markup strings (commands/config.py:342, 358). component_id is user-supplied and Rich markup interprets [tag] syntax. Cosmetic terminal-rendering risk only (not RCE). Use rich.markup.escape() when interpolating.
  • L3 -- Pre-existing bandit B110/B607 findings on git mv subprocess; unrelated to this PR.

CONTRIBUTING.md compliance

Ran through the "Adding a New CLI Command" checklist. PR satisfies the checklist with one caveat worth noting:

Checklist item Status Notes
Client method (client.py) OK get_config_state added; list_components_with_configs extended with include_state
Service method (services/) OK get_config_detail dual-mode; _fetch_project_component_configs follows the _run_parallel 3-tuple/2-tuple convention
Command function (commands/) OK Thin wiring, _format_config_detail_bulk is the only human-mode renderer added
--hint client + --hint service OK hints/definitions/config.py now has both single and bulk steps; multi-step hint correctly branches on config_id is None
Permission registration OK config.list and config.detail were already registered as "read"; no new operation keys needed. --with-state does not change the read/write category
Service wiring in cli.py N/A No new service class
kbagent context (commands/context.py) OK ### Configuration Browsing section rewritten to describe both modes + --include-rows + --with-state
SKILL.md freshness OK Single-row diff; matches the "auto-generated by make skill-gen" contract
CLAUDE.md OK Signatures of config list + config detail updated in the ## All CLI Commands block
commands-reference.md OK Both commands get verbose descriptions including mode switch, envelope shape, and per-project fan-out semantics
New reference workflow file Not added -- defensible This PR extends an existing command group (config) with additional flags / a second mode. It is not a new topic area (like workspace-workflow.md or branch-workflow.md were). The three gotchas.md entries (bulk envelope, --include-rows payload warning, --with-state mechanism) are the right shape for this kind of non-obvious but bounded behavior. If this is the intended interpretation, no change needed; if the maintainer wants a dedicated config-bulk-workflow.md, it can be a follow-up
gotchas.md OK Three new entries, each explains why (including the 404/501 on the missing /state endpoint)
--help text OK Detailed docstrings on both commands with explicit mode + examples
Service-layer tests OK 15 new tests in test_services.py covering bulk single-project, multi-project, partial failure, with_state, empty result, branch-rejected-multi
CLI-layer tests OK 15 new tests in test_cli.py, including the back-compat regression test
E2E tests OK 5 scenarios in test_e2e.py, invoked by make test-e2e
make check OK PR description reports 2165 tests green; CI check is passing

Summary of asks

Before merge (one item):

  • H1 -- sanitize/truncate str(exc) in the three UNEXPECTED_ERROR handlers (config_service.py:163-170, config_service.py:458-465, base.py:139-141).

Nice-to-have in a follow-up (not blocking):

  • M1 -- add a sensitivity WARNING: to the --with-state help text.
  • M3 -- reject empty --component-id at the CLI layer.
  • L2 -- rich.markup.escape() the component_id interpolation in _format_config_detail_bulk.
  • Earlier main-finding about the redundant get_config_state HTTP call in single-mode still stands.

PR is otherwise well-structured, backward-compatible, and tested. Ship once H1 is addressed.

padak added 3 commits April 23, 2026 15:59
… truncation, type hint

Three service-layer fixes from PR #218 review:

B1 (blocking): Avoid redundant HTTP call in single-mode --with-state.

Previously the service called get_config_detail() and then immediately
called get_config_state(), but both hit the same
GET /v2/storage[/branch/{b}]/components/{c}/configs/{id} URL -- the second
call extracted the state field the first response already contained. Pure
waste. The service now trusts the state key inline in the detail response
and normalises it to a dict. Regression test
test_with_state_single_uses_one_api_call asserts
mock_client.get_config_state.call_count == 0. The client helper is
retained as a convenience wrapper and the docstring now clarifies that
callers already holding a detail response should read state directly
rather than double-fetching.

B2 (HIGH-severity, CWE-209): Truncate UNEXPECTED_ERROR messages.

Any bare Exception message forwarded via str(exc) into the JSON error
envelope can embed URLs with query params, response-buffer fragments, or
-- with --with-state -- OAuth refresh tokens from the runtime state dict.
Added services.base.sanitize_unexpected_error(), applied at all three
sites (_fetch_project_configs, _fetch_project_component_configs,
BaseService._run_parallel). Limit: 256 chars with trailing "...".
Full exception still reaches the debug log so operators retain the
detail when they explicitly opt in.

A2 (optional): Pre-filter the listing endpoint by component_type.

When --component-id follows the standard keboola.{ex,wr,app}-* /
keboola.*-transformation conventions, the service derives the
component_type and forwards it via
list_components_with_configs(component_type=...). On projects with many
component families this shrinks the response body considerably. Custom
prefixes (e.g. kds-team.*) omit the hint and keep the historical
unfiltered behaviour.
…escape

Three UX / hygiene fixes from PR #218 review:

M1: Add a WARNING paragraph to --with-state help text so the caller
knows the output may carry OAuth tokens, refresh tokens, and other
credential-bearing runtime data. "Do not pipe into logs, scratch files,
or shared workspaces without redaction."

M3: Reject empty / whitespace-only --component-id up front with
INVALID_ARGUMENT (exit 2) instead of passing it to the service where it
would build a malformed /components//configs URL. New CLI test
test_config_detail_rejects_empty_component_id covers both
--component-id "" and --component-id "   ".

L2: Pass component_id through rich.markup.escape() before embedding it
in Rich-rendered strings in _format_config_detail_bulk(). Terminal-only
hygiene -- prevents an ID like keboola.[red]evil[/red] from painting
the output.
Fifth review item (A1): the PR title advertised 0.23.0 but pyproject was
still 0.22.0. Bump the source-of-truth version and document the user-
visible surface changes shipped under this release:

* config detail --component-id ID (without --config-id) -- bulk mode
* config detail --with-state -- inline state, no N+1
* config list --include-rows -- opt-in rows body
* client: get_config_state convenience wrapper; list_components_with_configs
  include_state parameter
* security: UNEXPECTED_ERROR message truncation to 256 chars with ...
  sentinel (CWE-209 mitigation)

plugin.json synced via make version-sync; uv.lock refreshed by the
editable reinstall that sync_version.py triggers.
@padak

padak commented Apr 23, 2026

Copy link
Copy Markdown
Member Author

Review fixes applied

Addressed the blocking and most of the optional review items. Pushed 3 focused commits on top of 3bd7631:

ID What Commit
B1 (blocking) Single-mode --with-state no longer issues the redundant second GET. Service now trusts the state field already embedded in get_config_detail's response (the API serves it inline — get_config_state just read it back from the same URL). Normalises missing/non-dict state to {}. Docstrings updated in client.py, services/config_service.py; get_config_state kept as a convenience wrapper with a note pointing callers to reuse existing detail responses. Regression test test_with_state_single_uses_one_api_call asserts mock_client.get_config_state.call_count == 0. 2102697
B2 (HIGH CWE-209) Added services.base.sanitize_unexpected_error(). Applied at all three sites: _fetch_project_configs, _fetch_project_component_configs, BaseService._run_parallel. Truncation limit = 256 chars + trailing "..." (new UNEXPECTED_ERROR_MAX_MESSAGE_LEN in constants.py). Full exception still reaches the debug log via logger.debug(...). Regression tests cover long-message truncation and short-message passthrough. 2102697
M1 --with-state help text now carries a WARNING paragraph about OAuth / refresh tokens and credential-bearing runtime data. 21dd7a8
M3 Fail-fast guard in config detail rejects --component-id "" and whitespace-only values with INVALID_ARGUMENT / exit 2 before touching the service. Test test_config_detail_rejects_empty_component_id covers both cases. 21dd7a8
L2 component_id passed through rich.markup.escape() in _format_config_detail_bulk before it hits the Rich console. 21dd7a8
A1 pyproject.toml bumped to 0.23.0; changelog entry added covering bulk mode, --with-state, --include-rows, client helpers, and the CWE-209 mitigation. make version-sync ran; plugin.json is in sync. make changelog-check passes. db8c3c6
A2 (optional) New _infer_component_type() in config_service.py derives extractor / writer / transformation / application from the standard keboola.* ID prefixes. Bulk mode now forwards component_type to list_components_with_configs so the API returns a narrower payload. Custom prefixes (e.g. kds-team.*) omit the hint. Verified live: URL for keboola.ex-db-snowflake now carries &componentType=extractor. 2102697
A3 list_components_with_configs docstring extended to mention the bulk-detail --with-state caller. 2102697

make check final pass

lint + format-check + skill-check + version-check + changelog-check + check-error-codes + test — all green. 2171 passed, 4 skipped.

Live verification against /tmp/kosik (padak project)

  • Single mode with --with-state now issues exactly 1 HTTP request (was 2 before):
    GET /v2/storage/components/ex-generic-v2/configs/1159268298 "HTTP/1.1 200 OK"
    
    JSON shape unchanged — id, name, configuration, rows, state all present.
  • Bulk mode still issues 1 HTTP request per project (13 configs returned).
  • keboola.ex-db-snowflake bulk now includes the componentType=extractor hint in the URL (A2).
  • --component-id "" and --component-id " " exit 2 with clean INVALID_ARGUMENT.
  • kbagent config detail --help displays the new WARNING paragraph about --with-state output.

No behavioural regression on the single-config JSON shape (locked in by test_config_detail_single_shape_preserved_for_backward_compat).

@padak padak left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Follow-up: fixes verified

Pulled the three new commits (2102697, 21dd7a8, db8c3c6) and walked each originally-flagged item against the diff. All blocking and medium items are resolved; the bonus component_type pre-filter (A2) is a nice addition; the approach to B2 (CWE-209) is exactly what I'd have hoped for -- centralised helper, both worker sites and the outer _run_parallel handler rewired, full exception still reaches logger.debug. CI is green.

Items addressed

ID Finding Commit Verified
B1 Redundant get_config_state in single-mode --with-state 2102697 config_service.py:372-378 Yes -- service reads inline state from get_config_detail() response and normalises; get_config_state client method retained as convenience wrapper with an honest docstring (client.py:366-377). Regression test test_with_state_single_uses_one_api_call asserts get_config_state.call_count == 0.
B2 UNEXPECTED_ERROR envelope leaks raw str(exc) (CWE-209) 2102697 base.py:24-40 + 3 call sites Yes -- new sanitize_unexpected_error() in services/base.py, UNEXPECTED_ERROR_MAX_MESSAGE_LEN = 256 constant in constants.py (respects the project's no-magic-numbers rule), consistently applied to _fetch_project_configs:193, _fetch_project_component_configs:497, and BaseService._run_parallel:160. logger.debug captures the full exception for verbose operators. Two unit tests cover the truncation and short-message paths.
M1 --with-state help text missing sensitivity warning 21dd7a8 config.py:200-206 Yes -- explicit "WARNING: --with-state output may contain OAuth tokens, refresh tokens, and other credential-bearing runtime data. Do not pipe into logs, scratch files, or shared workspaces without redaction."
M3 Empty --component-id "" not guarded 21dd7a8 config.py:257-265 Yes -- component_id.strip() check before service call, INVALID_ARGUMENT + exit 2. Test test_config_detail_rejects_empty_component_id covers both empty and whitespace-only inputs.
L2 component_id rendered unescaped in Rich markup 21dd7a8 config.py:353-377 Yes -- safe_component_id = escape(component_id) once, reused in both string interpolations.
A1 Version bump to 0.23.0 + changelog db8c3c6 Yes -- pyproject.toml bumped, plugin.json synced via make version-sync, changelog.py has five entries including an explicit security note for the CWE-209 mitigation, uv.lock refreshed.
A2 (bonus) Pre-filter bulk listing by inferred component_type 2102697 config_service.py:28-47,447 Yes -- _infer_component_type() covers keboola.ex-*, keboola.wr-*, keboola.app-*, keboola.*-transformation prefixes and falls back to None (unfiltered listing) for unknown prefixes like kds-team.*. Test test_bulk_passes_inferred_component_type_to_api verifies the API is called with the derived type.

Quality notes on the fixes

  • Layering respected. sanitize_unexpected_error lives in services/base.py, not in the client -- the right call since it's about service-layer error hygiene, not HTTP response parsing. The magic number (256) is pulled into constants.py with a comment linking back to CWE-209; matches the project's "no hardcoded values" rule.
  • Docstrings updated to match behaviour. get_config_state is now openly described as a convenience wrapper ("callers that already have a detail response should read state from it directly"), and the class-level docstring on get_config_detail explains why there is no extra HTTP call in single mode. No more misleading "fresh read" language.
  • A2 is defensively implemented. Unknown prefixes (kds-team.*, custom components) return None and fall through to the unfiltered listing -- no risk of silently dropping matches on custom layouts.
  • Tests are the right shape. The B1 regression test asserts the call count (not just the return value), which is the right contract to lock in.

Residual items (not blocking)

These were non-blocking observations in the original review and remain open. All are polish / refactor, not correctness issues:

  1. Double validation in CLI + service (both reject --config-id + multi---project / --branch + multi---project). The redundancy is intentional defense-in-depth. Fine as-is.
  2. alias argument ignored when aliases is given in ConfigService.get_config_detail. Current shape works; a future clean-up could promote _get_config_detail_bulk to a separate public method so callers pick the right entry point by shape.
  3. test_cli.py:1237 or [] guard in list(call.kwargs.get("aliases") or []) == ["prod", "stage"]. Trivial; a direct == would fail more loudly if the kwarg ever got dropped.

Verdict

LGTM. Ship it. Thorough response to review feedback -- not just the letter of each finding, but the right abstractions (shared sanitizer, constant for the limit, honest docstrings). Good final commit separation too (security/correctness in one commit, UX/hygiene in another, release metadata in a third).

@padak
padak merged commit 0403eed into main Apr 23, 2026
1 check passed
@padak
padak deleted the feat/197-bulk-config-detail branch April 23, 2026 14:14
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.

No bulk path to fetch config details (rows + state) for many configs

1 participant