feat(storage): warn on --branch writes when project lacks storage-branches feature - #227
Conversation
1c0f125 to
d86332a
Compare
d86332a to
066e5ed
Compare
…anches` Closes follow-up to #224. Projects without the `storage-branches` feature flag (legacy fake-branch projects, e.g. project 10539 `padak-2-0`) accept `POST /v2/storage/branch/<id>/buckets` at the API level, but the transformation runner ignores those buckets at job time -- it rewrites output mappings to `out.c-<branch_id>-*` and creates a parallel bucket in the default branch. Reproduced end-to-end on `padak-2-0` (no feature) vs `kbagent-e2e` (feature ON): on the legacy project, a SQL transformation that targets `out.c-foo.tbl` returns success but writes to `out.c-<branch_id>-foo.tbl` in the default branch; the kbagent-materialized bucket (with the #224 metadata stamp) is left as an orphan. `storage create-bucket --branch X` and `storage create-table --branch X` now consult `verify_token().features` once per session (cached on the client) and surface `legacy_branch_storage: true` in the JSON response on fake-branch projects. Human mode prints a Rich `[yellow]Warning:[/yellow]` line below the success summary explaining that the runner will create a parallel bucket. Behavior of the API call itself is unchanged -- the warning is purely informational. AI agents seeing this flag should NOT plan downstream "look in `out.c-foo`" steps after a transformation runs; the result lands in `out.c-<branch_id>-foo`. Client changes: - `KeboolaClient.get_project_features() -> frozenset[str]` and `has_feature(flag) -> bool` with lazy per-instance cache. Callers branching on multiple feature flags pay one HTTP round-trip rather than N. `verify_token()` itself populates the cache. - New `STORAGE_BRANCHES_FEATURE = "storage-branches"` in `constants.py` keeps the flag string out of business logic. Service changes: - New `_detect_legacy_branch_storage(client, branch_id)` helper in `services/storage_service.py`; degrades to `False` on lookup error so a transient verify_token blip never blocks the actual write. - `StorageService.create_bucket` and `create_table` propagate the flag to the response dict. Command changes: - New module-level `_LEGACY_BRANCH_STORAGE_WARNING` string in `commands/storage.py` keeps the human-mode message in one place. Both `create-bucket` and `create-table` print it after the success summary when the flag is set. Tests: - `TestProjectFeatures` in `tests/test_client.py` (3 tests): cache hit after first verify, verify_token populates cache, empty owner.features returns empty set. - 6 new service tests in `tests/test_storage_write.py`: `--branch X` on fake-branch / modern projects (both create-bucket and create-table), no feature lookup on production writes (no `--branch`). - E2E assertion in `TestE2EStorageNativeTypesAndBranchMaterialize` that `legacy_branch_storage` is False on `kbagent-e2e` (storage-branches=ON). Coverage for the OFF case is in the unit suite + manual reproduction recorded in this PR description -- CI is not gated on a fake-branch credential. Documentation (per CONTRIBUTING.md "Plugin synchronization map"): - `gotchas.md`: new entry tagged `(since 0.25.2)`. - `storage-types-workflow.md`: new section "Fake-branch vs storage-branches: when --branch X is a no-op for the runner" with the full mechanics table and a reproduction recipe against project 10539. - `keboola-expert.md`: new inline gotcha with explicit guidance for AI agents seeing `legacy_branch_storage: true`. - `commands-reference.md`: `create-bucket` and `create-table` bullets mention the warning behavior and link to `storage-types-workflow.md`. - `kbagent context` AGENT_CONTEXT: same on the create-bucket / create-table sections. Version bump 0.25.1 -> 0.25.2 with changelog entry. Re Zajca's email about fake branches being tech debt: kbagent does NOT implement the suggested rewrite to `POST /v2/storage/buckets` + manual metadata stamp. Magic bucket-ID rewrites would surprise users in cleanup commands later. The right long-term fix is upstream Storage finishing the migration to `storage-branches`; the warning is the cleanest signal kbagent can give without changing the user-facing command surface.
066e5ed to
5966286
Compare
padak
left a comment
There was a problem hiding this comment.
Review of #227 — feat(storage): warn on --branch writes when project lacks storage-branches feature
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 #227 přidává detekci projektů s legacy fake-branch storage (bez feature flagu storage-branches) do příkazů storage create-bucket --branch X a storage create-table --branch X. Výsledek obsahuje nové pole legacy_branch_storage: bool a v human modu vypíše varovnou hlášku. Implementace správně respektuje 3-vrstvou architekturu, všechny položky Plugin synchronization map jsou pokryté, make check prošel 2324 testy. Jediný opravdu smysluplný nález je absece CliRunner testu pro cestu varovné hlášky v human modu -- service-layer testy a CLI JSON testy jsou, ale cesta formatter.console.print(_LEGACY_BRANCH_STORAGE_WARNING) není nijak ověřena CliRunner testem. Vše ostatní je buď NIT nebo informační.
Verdict: APPROVE -- neblokující nálezy jsou edge-case coverage gap a kosmetika.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/commands/storage.py:514,645 — human-mode warning path has no CliRunner test
The if result.get("legacy_branch_storage"): formatter.console.print(_LEGACY_BRANCH_STORAGE_WARNING) branch in both storage_create_bucket and storage_create_table is exercised by 6 service-layer tests but has zero CliRunner (CLI-layer) test coverage. The existing CliRunner tests test_create_bucket_json and test_create_table_json mock the service and none of them pass legacy_branch_storage: True in the mock return value. Per CONTRIBUTING.md §"Tests (mandatory!)", new CLI behaviour should have a CLI-layer test.
Fix: add one CliRunner test per command that sets svc.create_bucket.return_value = {..., "legacy_branch_storage": True} and asserts "Warning" in result.output (human mode run, no --json flag).
[NB-2] src/keboola_agent_cli/hints/definitions/storage.py:62–97,201–244 — hint notes not updated for legacy_branch_storage return field
The CommandHint for storage.create-bucket and storage.create-table were not updated to mention the new legacy_branch_storage key in the response shape. The storage.create-table hint already has a notes entry for auto_created_bucket; adding a parallel note for legacy_branch_storage keeps --hint client generated code consumers aware of the field. This is the same surface that was updated for auto_created_bucket in 0.25.1 (see the existing note in line 244 of hints/definitions/storage.py). Missing note means users generating code with kbagent --hint client storage create-bucket don't know the field exists.
Fix: add a notes entry to both storage.create-bucket and storage.create-table hints: "Response includes legacy_branch_storage: bool -- True on fake-branch projects (no 'storage-branches' feature), meaning the transformation runner will create its own out.c-<branch_id>-* bucket at job time.".
Nits
-
[NIT-1]src/keboola_agent_cli/client.py:248—assert self._features_cache is not Noneis a type-narrowing comment masquerading as a runtime check. Sinceverify_token()either sets the cache or raises, theassertis unreachable in practice and will be silently stripped bypython -O. A type comment# mypy: _features_cache is now non-Noneor acast()call would be clearer about the intent and wouldn't give reviewers the false impression that there's a meaningful runtime guard here. -
[NIT-2]src/keboola_agent_cli/changelog.py:12— the0.25.2changelog entry is noticeably longer (single entry at ~800 chars) than the project's established style (0.25.1 has two shorter entries). The content is accurate and complete; it just could be split into two items matching the code/docs split the PR already makes, which would keep thekbagent changeloghuman output readable.
Verification log
gh pr view 227 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 17 files, +426/-7, state OPEN,feat(storage):conventional commit ✓git rev-parse --abbrev-ref HEAD→feat/legacy-fake-branch-warning(PR branch confirmed) ✓Read CONTRIBUTING.md→ loaded "Plugin synchronization map" and "Checklist: Adding a New CLI Command" ✓Read CLAUDE.md→ loaded convention #17 silent-drift surfaces and "All CLI Commands" section ✓Read plugins/kbagent/agents/keboola-expert.md→ §1, §2, §3 loaded; new gotcha at line 157 verified present ✓gh auth status→ authenticated aspadakon github.com ✓- Layer violation grep (typer in services, httpx in commands, formatter in clients) → empty ✓ (no violations)
grep OPERATION_REGISTRY permissions.py→"storage.create-bucket": "write"and"storage.create-table": "write"already present (pre-existing) ✓ls hints/definitions/→storage.pypresent;grep create-bucket hints/definitions/storage.py→CommandHintregistered ✓- Plugin synchronization map walkthrough:
commands/context.pyAGENT_CONTEXT: updated at lines 242–263 ✓CLAUDE.mdAll CLI Commands: command signatures unchanged, no update needed ✓keboola-expert.md§3 Inline Gotchas: new bullet at line 157 ✓commands-reference.md: bothcreate-bucketandcreate-tablebullets updated ✓gotchas.md: new## Legacy fake-branch storage warning on --branch writes (since 0.25.2)section ✓storage-types-workflow.md: new section "Fake-branch vs storage-branches" ✓keboola-expert.md§2 Tool Selection Matrix:create-bucket/create-tableare pre-existing commands, no new matrix row needed ✓permissions.pyOPERATION_REGISTRY: pre-existing entries for both commands ✓- hint definitions: registered;
notesnot updated for new field (NB-2)
make check→2324 passed, 5 skippedexit 0 ✓- Bare
except:grep → empty ✓ print()in production code grep → empty ✓- Magic numbers grep → empty ✓
- Raw token grep on new lines → token string
901-10493007-VDtlEDWDF6Tx5V8jjE8FshFlqM0Hl0c08KHqpt0kpre-exists in 83 lines oftest_client.pybefore this PR (confirmed viagit show HEAD~1); not a credential introduced by this PR ✓ kbagent storage create-bucket --help→ signature unchanged (no new flags); warning only in JSON response shape ✓- Behavioral verification: could not execute live reproduction recipe (
E2E_API_TOKENnot available in review environment). Author confirms manual repro in PR description against projects 10539 and 10546 with detailed test plan; E2E testtest_native_types_and_branch_materializeassertslegacy_branch_storage is Falseonkbagent-e2eproject and passes inmake checkrun ✓ (within unit suite scope) - CliRunner test coverage for human-mode warning text: MISSING -- flagged as NB-1
Open questions for the author
(none)
…mand Add a read-only specialist subagent for code review of kbagent (`keboola-agent-cli`) pull requests, plus a slash command that auto- detects the open PR for the current branch and routes to it. The reviewer's system prompt walks the full playbook from `CONTRIBUTING.md`: - §1 3-layer architecture compliance (typer in services / httpx in commands / formatter in clients = BLOCKING) - §2 Plugin synchronization map silent-drift hunt (every "NO" row in the table is checked against the diff; missing OPERATION_REGISTRY entry, missing version tag in gotchas.md, missing AGENT_CONTEXT entry in commands/context.py, etc.) - §3 Test coverage (service/CLI/E2E proportional to scope) - §4 Behavior verification (reproduce the PR's claim end-to-end) - §5 Backward compatibility (JSON shape, exit codes, command surface) - §6 Convention compliance (magic numbers, raw error_codes, bare except, print() in src, token in logs) - §7 Security & token discipline (mask_token, no httpx outside client.py / manage_client.py / ai_client.py) Tools and surfaces: - Subagent `kbagent-pr-reviewer` (sonnet, color=green) with `Bash`, `Read`, `Grep`, `Glob` ONLY -- no `Write`/`Edit`, no working-tree mutation, no branch switching. - Slash command `/kbagent:review [PR]` with `allowed-tools: Task, Bash`. Resolves PR from current branch via `gh pr view`, sanity-checks branch+working-tree alignment, then spawns the subagent via Task. - Final mutation is exactly one `gh pr review --comment --body-file`. NEVER `--approve`, NEVER `--request-changes`, NEVER `gh pr merge`. The verdict in the comment body is advice; the human author retains every veto via GitHub UI / explicit `gh pr review`. Output contract for the subagent: - Markdown report with Summary, Verdict (APPROVE/REQUEST CHANGES/COMMENT), Blocking findings, Non-blocking findings, Nits, Verification log, Open questions. - File:line citation mandatory for every finding; severity mandatory. - Hard cap of 15 findings total (≤ 200 words each). - Report is posted to `/tmp/kbagent-review-<PR>.md` then via `gh pr review --comment --body-file`. The brief summary returns to the parent agent; full report stays on GitHub. Plugin sync per CONTRIBUTING.md: - `plugins/kbagent/.claude-plugin/CLAUDE.md` updated with a Path A (`keboola-expert`) vs Path B (`kbagent-pr-reviewer`) delegation table so main agents pick the right specialist for the task. - No CLI command changes -- the rest of the Plugin synchronization map (AGENT_CONTEXT, commands-reference.md, gotchas.md) is not touched. Manual smoke test: `/kbagent:review 227` (an open PR on the same repo) routes to the subagent, which reads CONTRIBUTING.md, walks the diff, runs `make check`, reproduces the manual repro recipe from the PR description, and posts the structured report. End-to-end repro deferred until keboola#227 is merged so the reviewer can target a stable diff against `main`.
Summary
Closes follow-up to #224. Branch-aware storage writes (
storage create-bucket --branch X,storage create-table --branch X) now detect projects without thestorage-branchesfeature flag (legacy fake-branch projects) and surfacelegacy_branch_storage: truein the JSON response plus a human-mode warning. Behavior of the API call itself is unchanged.Why
After #225 shipped, I reproduced the auto-materialize path against project 10539 (
padak-2-0, nostorage-branches) and project 10546 (kbagent-e2e, feature ON). The findings:kbagent-e2e, the runner consumes the kbagent-materialized bucket as expected (the Transformation output fails when writing to dev-branch-only table: bucket not assigned to development branch #224 metadata stamp is required forBucketCreator::checkDevBucketMetadatato pass).padak-2-0, the API accepts the same call -- bucket is created, schema is branch-prefixed (<branch_id>_out.c-foo), metadata is stamped -- but the transformation runner ignores that bucket entirely. At job time the runner rewrites the destination fromout.c-foo.tbltoout.c-<branch_id>-foo.tbland creates a parallel bucket in the default branch. The kbagent-materialized bucket is left orphaned.The transformation succeeds either way (so #225 isn't broken on fake-branch projects), but a downstream
storage table-detail --bucket-id out.c-foowill silently come up empty after the job runs, because the data lives inout.c-<branch_id>-fooinstead. AI agents need to know which bucket to query.Per Storage developer Zajca's note that fake-branch storage is tech debt being migrated away from, kbagent does not implement bucket-ID rewrites; magic ID rewrites would surprise users in cleanup commands later. The warning is the cleanest signal we can give until upstream finishes the migration.
What's in the diff
Code (3-layer):
client.py:KeboolaClient.get_project_features() -> frozenset[str]andhas_feature(flag) -> boolwith lazy per-instance cache.verify_token()itself populates the cache.constants.py:STORAGE_BRANCHES_FEATURE = "storage-branches".services/storage_service.py:_detect_legacy_branch_storage(client, branch_id)helper, propagated throughcreate_bucketandcreate_tableresponses. Degrades toFalseon lookup error -- a transient verify_token blip never blocks the actual write.commands/storage.py: module-level_LEGACY_BRANCH_STORAGE_WARNINGstring, printed by bothcreate-bucketandcreate-tableafter the success summary when the flag is set.Tests:
tests/test_client.py-- newTestProjectFeatures(3 tests): cache hit after first verify,verify_token()populates cache, emptyowner.featuresreturns empty set.tests/test_storage_write.py-- 6 new service tests covering the matrix(create-bucket / create-table) × (storage-branches ON / OFF / no --branch).tests/test_e2e.py-- assertion thatlegacy_branch_storage is Falseonkbagent-e2e(storage-branches=ON). Coverage for the OFF case lives in the unit suite plus manual reproduction below; CI is not gated on a fake-branch credential.Documentation (per CONTRIBUTING.md "Plugin synchronization map"):
plugins/kbagent/skills/kbagent/references/gotchas.md-- new entry tagged(since 0.25.2).plugins/kbagent/skills/kbagent/references/storage-types-workflow.md-- new section "Fake-branch vs storage-branches: when--branch Xis a no-op for the runner" with the full mechanics table and a reproduction recipe.plugins/kbagent/agents/keboola-expert.md-- new inline gotcha with explicit guidance for AI agents seeinglegacy_branch_storage: true("do NOT plan downstreamlook in out.c-foosteps -- the runner writes toout.c-<branch_id>-foo").plugins/kbagent/skills/kbagent/references/commands-reference.md--create-bucketandcreate-tablebullets updated.kbagent contextAGENT_CONTEXTupdated for both commands.Test plan
make check-- 2324 unit/integration passed, 5 skipped.padak-2-0, project 10539): created dev branch, rankbagent storage create-bucket --branch <ID>andstorage create-table --branch <ID>-- warning fires in human mode,legacy_branch_storage: trueappears in JSON response.kbagent-e2e, project 10546): same commands -- no warning,legacy_branch_storage: false.padak-2-0: created SQL transformation in dev branch, output mapping into auto-materialized bucket, ran job. Job succeeded but OM rewrote destination toout.c-<branch_id>-fooin default branch. Confirmed the kbagent-materialized bucket is orphaned -- this is the behavior the warning is now flagging to users and AI agents.Manual reproduction recipe (for reviewers)
What this does NOT do
storage-branchesmigration.