fix(0.43.1): sandbox annotation in service layer for HTTP / REST parity (#312) - #314
Conversation
padak
left a comment
There was a problem hiding this comment.
Review of #314 — fix(0.43.1): sandbox annotation in service layer for HTTP / REST parity (#312)
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
PR #314 is a clean, well-scoped follow-up to PR #311. It moves the keboola.sandboxes parameters.id → storage_workspace_id resolution from commands/config.py into ConfigService.get_config_detail() behind an opt-in parameter, closing the HTTP/REST parity gap where web UI and scheduled-agent callers could not access the sandbox annotation. The pure-function extraction (find_storage_workspace_for_sandbox_config) is correct and avoids the circular dependency. The opt-in contract (include_sandbox_annotation: bool = False) is properly defended by tests. The v0.43.0 changelog backfill and v0.43.1 version bump are handled cleanly.
There are zero blocking findings. Two non-blocking findings and two nits follow.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] plugins/kbagent/agents/keboola-expert.md:112 — VERSION GATE not updated for the ?include_sandbox_annotation=true HTTP parameter
keboola-expert.md §1 Rule 6 currently reads: "sandbox config annotation needs 0.42.0+ (#304; gotchas.md)". That covers the CLI behavior, which existed since v0.42.0. But the ?include_sandbox_annotation=true query parameter on GET /configs/{project}/{component_id}/{config_id} is new in v0.43.1. An AI agent running on kbagent 0.42.x that tries to instruct a web UI caller about the HTTP endpoint will refer to non-existent behavior. The CONTRIBUTING.md release checklist (step 5, §2) requires reviewing the VERSION GATE for new minimum-version requirements.
Fix: add a sub-note next to the existing line, e.g.: "?include_sandbox_annotation=true on GET /configs/... (REST/serve HTTP parity for the same annotation) needs 0.43.1+."
[NB-2] tests/test_services.py:2045 (class TestConfigServiceSandboxAnnotation) — none of the 5 new service tests assert mock_client.close()
Per CONTRIBUTING.md > Testing Guidelines: "Verify client.close() is called (via mock_client.close.assert_called_once())". The new tests exercise the ConfigService.get_config_detail happy-path and error paths but none of them verify that the underlying HTTP client is closed in the finally block after the list_workspaces fan-out. If the finally: client.close() line were accidentally removed, these tests would not catch it.
Fix: add mock_client.close.assert_called_once() to at least the two happy-path tests (test_annotation_off_by_default and test_annotation_resolves_storage_workspace_id). The error-degradation test (test_annotation_swallows_workspace_listing_error) should also include this assertion to pin the "detail succeeds, client still closed" contract.
Nits
-
[NIT-1]tests/test_services.py:2218—KeboolaApiError(error_code="RATE_LIMITED", ...)uses a raw string that has no matchingErrorCodeenum member.check-error-codesdoes not scantests/so this passes CI, but it is an inconsistency with the rest of the test file (which usesErrorCode.API_ERROR,ErrorCode.NOT_FOUND, etc. where enum members exist).ErrorCodedoes not currently have aRATE_LIMITorRATE_LIMITEDmember. Either add the member or useErrorCode.API_ERROR(the generic fallback) for the fixture. -
[NIT-2]src/keboola_agent_cli/services/config_service.py:427— The conditionif include_sandbox_annotation and component_id == "keboola.sandboxes":embeds the component ID as a magic string literal. The same string already appears incommands/config.py(removed by this PR),output.py,commands/context.py, and several test fixtures. This is grandfathered in the pre-existing codebase and does not need to be fixed in this PR, but addingSANDBOX_COMPONENT_ID = "keboola.sandboxes"toconstants.pywould make future cross-layer consistency easier to grep.
Verification log
gh pr view 314 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 13 files, +451/-72, state=OPEN, headRefName=fix/issue-312-sandbox-annotation-service, conventionalfix(0.43.1):prefix ✓git rev-parse --abbrev-ref HEAD(in worktree) →fix/issue-312-sandbox-annotation-service✓ (matches PR branch)wc -l /tmp/kbagent-pr-314.diff→ 796 lines ✓- Layer violation checks (typer in services, httpx in commands, formatter in clients) → empty ✓ (no violations)
- New
@*_app.commanddecorators → none (PR does not add or remove CLI commands; no OPERATION_REGISTRY, hints/definitions, or CLAUDE.md## All CLI Commandsupdate required) ✓ - Convention checks: magic numbers, bare
except:, raw error_code strings insrc/,print()in production code → all empty ✓ make check-error-codes(in worktree) →OK: no raw error_code string literals in source.✓ (note: only scanssrc/, nottests/)make check(main repo, main branch) → 3373 passed, 7 skipped ✓make check(worktree, PR branch) → 3381 passed, 7 skipped ✓ (8 new tests confirmed passing)- 3-layer compliance:
ConfigService.get_config_detailinservices/config_service.pycallsfind_storage_workspace_for_sandbox_config(module-level pure function inworkspace_service.py) — no circularConfigService → WorkspaceServiceclass dependency, no typer imports in services ✓ WorkspaceService.resolve_sandbox_workspace_idpreserved as a one-line wrapper atworkspace_service.py:367✓ (no backward compat break for direct callers)sandbox_annotationvariable initialized toNoneat line 426 inside the outertryblock; the outerfinally: client.close()at line 452 runs on all paths; code afterfinally(lines 455–459) only executes if the outertrydid not raise — so thesandbox_annotationreference at line 457 is always valid when reached ✓- Plugin synchronization map — "NO" rows checked:
commands/context.py(AGENT_CONTEXT) → mentionssandbox_annotation(line 160–163), no new command added so no update needed ✓CLAUDE.md## All CLI Commands→kbagent config detailsignature unchanged; no update needed ✓plugins/kbagent/agents/keboola-expert.md§1 Rule 6 VERSION GATE → mentions0.42.0+for annotation, but?include_sandbox_annotation=trueHTTP param is 0.43.1+ (finding NB-1)plugins/kbagent/skills/kbagent/references/gotchas.md→ updated with(updated v0.43.1 -- closes #312)paragraph inside existing(since v0.42.0)section, matching the file's own documented convention (lines 7–11) ✓plugins/kbagent/skills/kbagent/references/commands-reference.md→ no new command; existingconfig detailnotes unaffected ✓src/keboola_agent_cli/permissions.pyOPERATION_REGISTRY→config.detailalready registered asread; no new command ✓hints/definitions/→ no new command; existingconfighint definition unaffected ✓
- Security: no new endpoint exposes tokens without
mask_token();?include_sandbox_annotation=truecarries no auth data in response; no new HTTP calls outsideclient.py✓ - Backward compat:
include_sandbox_annotation: bool = Falsedefault confirmed bytest_annotation_off_by_default(mock_client.list_workspaces.assert_not_called) ✓; existingsandbox_annotationconsumers (output.py:230,context.py:160,test_e2e.py:6077) unaffected ✓ - v0.43.0 changelog backfill: retroactively adding a
changelog.pydict entry for a version already released is cosmetic (the dict is displayed bykbagent changelog); no runtime impact ✓ - Behavior reproduction:
kbagent config detailbehavior per PR description verified by author (3381 passing tests including 5 service + 3 HTTP + 3 CLI-updated tests); could not independently reproduce against real API (noE2E_API_TOKENavailable in this session). Functional claim is adequately covered by the unit + HTTP-layer tests.
Open questions for the author
(none)
padak
left a comment
There was a problem hiding this comment.
Review of #314 — fix(0.43.1): sandbox annotation in service layer for HTTP / REST parity (#312)
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
PR #314 is a clean, well-scoped follow-up to PR #311. It moves the keboola.sandboxes parameters.id → storage_workspace_id resolution from commands/config.py into ConfigService.get_config_detail() behind an opt-in parameter, closing the HTTP/REST parity gap where web UI and scheduled-agent callers could not access the sandbox annotation. The pure-function extraction (find_storage_workspace_for_sandbox_config) is correct and avoids the circular dependency. The opt-in contract (include_sandbox_annotation: bool = False) is properly defended by tests. The v0.43.0 changelog backfill and v0.43.1 version bump are handled cleanly.
There are zero blocking findings. Two non-blocking findings and two nits follow.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] plugins/kbagent/agents/keboola-expert.md:112 — VERSION GATE not updated for the ?include_sandbox_annotation=true HTTP parameter
keboola-expert.md §1 Rule 6 currently reads: "sandbox config annotation needs 0.42.0+ (#304; gotchas.md)". That covers the CLI behavior, which existed since v0.42.0. But the ?include_sandbox_annotation=true query parameter on GET /configs/{project}/{component_id}/{config_id} is new in v0.43.1. An AI agent running on kbagent 0.42.x that tries to instruct a web UI caller about the HTTP endpoint will refer to non-existent behavior. The CONTRIBUTING.md release checklist (step 5, §2) requires reviewing the VERSION GATE for new minimum-version requirements.
Fix: add a sub-note next to the existing line, e.g.: "?include_sandbox_annotation=true on GET /configs/... (REST/serve HTTP parity for the same annotation) needs 0.43.1+."
[NB-2] tests/test_services.py:2045 (class TestConfigServiceSandboxAnnotation) — none of the 5 new service tests assert mock_client.close()
Per CONTRIBUTING.md > Testing Guidelines: "Verify client.close() is called (via mock_client.close.assert_called_once())". The new tests exercise the ConfigService.get_config_detail happy-path and error paths but none of them verify that the underlying HTTP client is closed in the finally block after the list_workspaces fan-out. If the finally: client.close() line were accidentally removed, these tests would not catch it.
Fix: add mock_client.close.assert_called_once() to at least the two happy-path tests (test_annotation_off_by_default and test_annotation_resolves_storage_workspace_id). The error-degradation test (test_annotation_swallows_workspace_listing_error) should also include this assertion to pin the "detail succeeds, client still closed" contract.
Nits
-
[NIT-1]tests/test_services.py:2218—KeboolaApiError(error_code="RATE_LIMITED", ...)uses a raw string that has no matchingErrorCodeenum member.check-error-codesdoes not scantests/so this passes CI, but it is an inconsistency with the rest of the test file (which usesErrorCode.API_ERROR,ErrorCode.NOT_FOUND, etc. where enum members exist).ErrorCodedoes not currently have aRATE_LIMITorRATE_LIMITEDmember. Either add the member or useErrorCode.API_ERROR(the generic fallback) for the fixture. -
[NIT-2]src/keboola_agent_cli/services/config_service.py:427— The conditionif include_sandbox_annotation and component_id == "keboola.sandboxes":embeds the component ID as a magic string literal. The same string already appears incommands/config.py(removed by this PR),output.py,commands/context.py, and several test fixtures. This is grandfathered in the pre-existing codebase and does not need to be fixed in this PR, but addingSANDBOX_COMPONENT_ID = "keboola.sandboxes"toconstants.pywould make future cross-layer consistency easier to grep.
Verification log
gh pr view 314 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 13 files, +451/-72, state=OPEN, headRefName=fix/issue-312-sandbox-annotation-service, conventionalfix(0.43.1):prefix ✓git rev-parse --abbrev-ref HEAD(in worktree) →fix/issue-312-sandbox-annotation-service✓ (matches PR branch)wc -l /tmp/kbagent-pr-314.diff→ 796 lines ✓- Layer violation checks (typer in services, httpx in commands, formatter in clients) → empty ✓ (no violations)
- New
@*_app.commanddecorators → none (PR does not add or remove CLI commands; no OPERATION_REGISTRY, hints/definitions, or CLAUDE.md## All CLI Commandsupdate required) ✓ - Convention checks: magic numbers, bare
except:, raw error_code strings insrc/,print()in production code → all empty ✓ make check-error-codes(in worktree) →OK: no raw error_code string literals in source.✓ (note: only scanssrc/, nottests/)make check(main repo, main branch) → 3373 passed, 7 skipped ✓make check(worktree, PR branch) → 3381 passed, 7 skipped ✓ (8 new tests confirmed passing)- 3-layer compliance:
ConfigService.get_config_detailinservices/config_service.pycallsfind_storage_workspace_for_sandbox_config(module-level pure function inworkspace_service.py) — no circularConfigService → WorkspaceServiceclass dependency, no typer imports in services ✓ WorkspaceService.resolve_sandbox_workspace_idpreserved as a one-line wrapper atworkspace_service.py:367✓ (no backward compat break for direct callers)sandbox_annotationvariable initialized toNoneat line 426 inside the outertryblock; the outerfinally: client.close()at line 452 runs on all paths; code afterfinally(lines 455–459) only executes if the outertrydid not raise — so thesandbox_annotationreference at line 457 is always valid when reached ✓- Plugin synchronization map — "NO" rows checked:
commands/context.py(AGENT_CONTEXT) → mentionssandbox_annotation(line 160–163), no new command added so no update needed ✓CLAUDE.md## All CLI Commands→kbagent config detailsignature unchanged; no update needed ✓plugins/kbagent/agents/keboola-expert.md§1 Rule 6 VERSION GATE → mentions0.42.0+for annotation, but?include_sandbox_annotation=trueHTTP param is 0.43.1+ (finding NB-1)plugins/kbagent/skills/kbagent/references/gotchas.md→ updated with(updated v0.43.1 -- closes #312)paragraph inside existing(since v0.42.0)section, matching the file's own documented convention (lines 7–11) ✓plugins/kbagent/skills/kbagent/references/commands-reference.md→ no new command; existingconfig detailnotes unaffected ✓src/keboola_agent_cli/permissions.pyOPERATION_REGISTRY→config.detailalready registered asread; no new command ✓hints/definitions/→ no new command; existingconfighint definition unaffected ✓
- Security: no new endpoint exposes tokens without
mask_token();?include_sandbox_annotation=truecarries no auth data in response; no new HTTP calls outsideclient.py✓ - Backward compat:
include_sandbox_annotation: bool = Falsedefault confirmed bytest_annotation_off_by_default(mock_client.list_workspaces.assert_not_called) ✓; existingsandbox_annotationconsumers (output.py:230,context.py:160,test_e2e.py:6077) unaffected ✓ - v0.43.0 changelog backfill: retroactively adding a
changelog.pydict entry for a version already released is cosmetic (the dict is displayed bykbagent changelog); no runtime impact ✓ - Behavior reproduction:
kbagent config detailbehavior per PR description verified by author (3381 passing tests including 5 service + 3 HTTP + 3 CLI-updated tests); could not independently reproduce against real API (noE2E_API_TOKENavailable in this session). Functional claim is adequately covered by the unit + HTTP-layer tests.
Open questions for the author
(none)
…ty (#312) PR #311 (v0.42.0, closes #304) placed the keboola.sandboxes parameters.id → storage_workspace_id resolution in `commands/config.py`, which meant the annotation only fired on `kbagent config detail` CLI invocations. HTTP / REST callers (`kbagent serve` web UI, scheduled agents, third-party clients hitting `GET /configs/...`) hit the same parameters.id trap David Ešner originally reported in #304. This PR moves the annotation into `ConfigService.get_config_detail()` behind a new opt-in parameter so all callers can get it. Architecture ------------ 1. **Pure-function helper extraction.** The workspace[].configurationId → workspace.id filter logic moves from `WorkspaceService.resolve_sandbox_workspace_id` to a module-level `find_storage_workspace_for_sandbox_config(workspaces, config_id)` in `services/workspace_service.py`. This lets ConfigService call it without taking a circular `ConfigService → WorkspaceService` dependency in the DI graph. `WorkspaceService.resolve_sandbox_workspace_id` becomes a thin wrapper around the helper (still useful for direct programmatic callers). 2. **Opt-in service parameter.** `ConfigService.get_config_detail()` gains `include_sandbox_annotation: bool = False`. Default off so existing programmatic consumers see the unchanged shape -- zero-regression contract. When the flag is on AND `component_id == "keboola.sandboxes"` AND single-config mode, the service fetches `list_workspaces` once and stamps the annotation onto the response. 3. **CLI: switch to service-layer annotation.** `commands/config.py` drops its ad-hoc post-fetch enrichment block (which previously called `WorkspaceService` directly from the command layer -- a layering violation) and instead passes `include_sandbox_annotation=True` to `get_config_detail`. Bulk mode stays off because it would N+1 the workspace listing endpoint. 4. **HTTP / REST parity.** `GET /configs/{project}/{component_id}/{config_id}` on `kbagent serve` accepts a new query parameter `?include_sandbox_annotation=true` (default false), forwarded verbatim to the service. The FastAPI `description=` on the Query annotation renders the rationale inline in /docs. 5. **Graceful degradation.** If `list_workspaces` fails (rate limit, transient 5xx), the detail call still succeeds and `storage_workspace_id` is set to `None`. The annotation is UX, not a contract -- the caller still gets the raw detail with the same shape they would see if `include_sandbox_annotation=False`. Tests ----- - **5 new in `test_services.py::TestConfigServiceSandboxAnnotation`**: default-off zero-regression, opt-in resolution to real workspace ID, orphan (no matching workspace -> storage_workspace_id=None), non-sandbox component is no-op (no list_workspaces fan-out), graceful degradation on list_workspaces KeboolaApiError. - **3 new in `test_serve_ui.py::TestConfigDetailSandboxAnnotation`**: HTTP router parameter binding -- default-off, opt-in, non-sandbox no-op. Stubs `app.state.registry.config.get_config_detail` to avoid real Keboola HTTP and assert the router forwards the flag verbatim. - **3 existing CLI tests in `test_cli.py::TestConfigDetail`** updated to mock the new service-layer call path (`client.list_workspaces` instead of `WorkspaceService.resolve_sandbox_workspace_id`). Test suite: 3381 passed, 104 skipped. Versioning ---------- v0.43.0 was released yesterday for the Semantic Layer UI (PR #308) but its changelog entry was missing from `changelog.py` -- the `changelog-check` make target was failing. This PR backfills the 0.43.0 entry (reconstructed from the GitHub release notes) AND adds 0.43.1 for the #312 fix. Plugin.json + marketplace.json synced via `make version-sync`.
Two follow-ups to the issue #312 fix in response to /kbagent:review: 1. [NON-BLOCKING] plugins/kbagent/agents/keboola-expert.md VERSION GATE now distinguishes the 0.42.0+ CLI sandbox annotation from the new 0.43.1+ HTTP opt-in (`?include_sandbox_annotation=true` on `GET /configs/...`). Kept the VERSION GATE entry tight so the agent prompt stays under the 60_000-byte budget (`test_agent_prompt_under_token_budget`). 2. [NON-BLOCKING] Each of the 5 `TestConfigServiceSandboxAnnotation` tests now asserts `mock_client.close.assert_called_once()`. This pins the contract that the finally block runs on every path -- fast-path / opt-in / orphan / non-sandbox-no-op / exception-mid-try -- so an accidental early return or a regression in the bare try/except KeboolaApiError swallow would be caught instead of leaking an httpx client per call. Nits (2 in the original report) were already addressed by the follow-up. Test suite: 3381 passed, 104 skipped.
5141f04 to
271d889
Compare
Summary
Closes #312. Follow-up to PR #311 (v0.42.0, closes #304). The original fix placed the
keboola.sandboxesparameters.id→storage_workspace_idresolution incommands/config.py, so it only fired on the CLI. HTTP / REST callers (kbagent serve, web UI, scheduled agents, third-party clients hittingGET /configs/...) hit the same trap David Ešner reported in #304.This PR moves the annotation into
ConfigService.get_config_detail()behind an opt-in parameter so every caller can request it.Architecture
find_storage_workspace_for_sandbox_config(workspaces, config_id) -> int | Nonemoves fromWorkspaceService.resolve_sandbox_workspace_idto a module-level function inservices/workspace_service.py. This letsConfigServicecall it without taking a circular dependency onWorkspaceService. The existing public method is preserved as a one-line wrapper.ConfigService.get_config_detail()gainsinclude_sandbox_annotation: bool = False. Default off → zero-regression contract for existing programmatic consumers. Single-config mode only (bulk mode would N+1 the workspace listing endpoint).commands/config.pydrops its ad-hoc post-fetch enrichment (a layering violation) and passesinclude_sandbox_annotation=Trueto the service. End-user behavior unchanged.GET /configs/{project}/{component_id}/{config_id}accepts?include_sandbox_annotation=true(default false), forwarded verbatim. Rendered in /docs via FastAPIQuery(..., description=...).list_workspacesfails (rate limit / 5xx), the detail call still succeeds andstorage_workspace_idis set toNone. Annotation is UX, not a contract.Test plan
make lintcleanmake format-checkcleanmake changelog-checkclean[server]for HTTP-endpoint testing)sandbox_annotationkey appears in response without explicit opt-in (zero regression for existing web UI / agents)?include_sandbox_annotation=truereturns the block via HTTPVersioning note
v0.43.0 was released yesterday for the Semantic Layer UI (PR #308) but its changelog.py entry was missing --
changelog-checkwas failing. This PR backfills the 0.43.0 entry (reconstructed from the GitHub release notes) AND adds 0.43.1 for the #312 fix.make version-syncbroughtplugin.json+marketplace.jsonin line.Out of scope