fix(storage): correct stale "dev branch only" swap-tables wording#373
Conversation
Follow-up to #368. The swap-tables semantics correction (0.52.0) left four co-located surfaces still claiming the swap is dev-branch-only / rejected on production -- now false after that fix: - services/storage_service.py: the ConfigError raised on a missing --branch still said "The Storage API rejects this on production" (user-facing at exit code 5, directly contradicting the corrected docstring) - references/commands-reference.md, commands/storage.py docstring (-> SKILL.md via make skill-gen), commands/context.py (AGENT_CONTEXT): "in a dev branch" / "in a development branch" A swap on the default/production branch is supported -- it is how a typed rebuild is applied to prod. branch_id stays mandatory; this is wording only, no behavior change. The dependent test match ("dev branch" -> "requires a branch") and a misleading test docstring were updated accordingly. Found by the kbagent-pr-reviewer third pass on #368 (NB-1/2/3 + NIT-1).
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
padak
left a comment
padak
left a comment
There was a problem hiding this comment.
Review of #373 — fix(storage): correct stale "dev branch only" swap-tables wording
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
This PR is a wording-only follow-up to #368 that corrects five stale "dev branch only / rejects on production" swap-tables surfaces flagged by the prior review. All six changed files target the right surfaces, and the primary fix (the user-facing ConfigError message in storage_service.py) is correct and consistent. One issue was missed: the CLI-layer test test_swap_missing_branch_fails_clearly (TestSwapTablesCLI) still injects the old error text as its mock side-effect and asserts against "dev branch" in the output — it passes only because the mock bypasses the real service, meaning it no longer exercises what the service actually emits. There is also a secondary minor stale entry in the same service's Args docstring. All other surfaces (context.py, commands/storage.py, commands-reference.md, SKILL.md, keboola-expert.md) are correct. make check passes (3844 tests). Verdict is REQUEST CHANGES on the basis of the stale CLI test.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 1
- Non-blocking findings: 1
- Nits: 0
Blocking findings
[B-1] tests/test_storage_swap.py:436,456 — CLI-layer test still asserts stale "dev branch" error text
TestSwapTablesCLI.test_swap_missing_branch_fails_clearly (lines 425-456) injects ConfigError("swap-tables requires a dev branch.") as its mock side-effect (line 436) and then asserts "dev branch" in payload["error"]["message"] (line 456). The test passes today only because the mock short-circuits the real service — the assertion validates a fictional error string that diverges from the actual service output ("swap-tables requires a branch. Set one with ...", updated in storage_service.py:1378). This is exactly the class of hidden inconsistency this PR was created to eliminate. A future refactor that wires the CLI layer more directly to the service would silently expose that the test was checking phantom text. It also leaves the old "dev branch" wording present in the test file, which partially undoes the exhaustive grep this PR is meant to clean up.
Fix: update line 436 to ConfigError("swap-tables requires a branch.") and line 456 to assert "requires a branch" in payload["error"]["message"]. The service-layer test (TestSwapTablesService.test_no_branch_raises_config_error) was already correctly updated.
Non-blocking findings
[NB-1] src/keboola_agent_cli/services/storage_service.py:1365 — Args docstring still says "Dev branch ID"
The Args block for swap_tables (line 1365) reads branch_id: Dev branch ID (must not be None). The surrounding docstring body (lines 1355-1359) was correctly updated to say "Any branch is accepted, INCLUDING the default/production branch". The Args one-liner is the only place in the function that still says "Dev branch", and it is technically wrong now that production branches are explicitly supported.
Fix: change to branch_id: Branch ID (must not be None; any branch accepted, including the default/production branch).
Nits
(none)
Verification log
gh pr view 373 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 6 files, +9/-9, state OPEN, conventionalfix(storage):✓git rev-parse --abbrev-ref HEAD→fix/swap-tables-stale-production-text✓ (working tree on PR branch)gh pr diff 373→ 89 lines diff; read in full ✓make check→ 3844 passed, 0 failed ✓make skill-check→SKILL.md is up-to-date✓uv run pytest tests/test_storage_swap.py -v→ 14 passed; all pass because CLI test uses mock ✓ (confirmed mock bypasses real service — see B-1)grep -n "dev branch only|rejects on production|rejects this on production" [all live doc surfaces]→ empty ✓ (no stale primary wording remains in live surfaces)grep -n "swap.*dev branch|dev branch.*swap" [all live doc surfaces]→ no hits incontext.py,storage.py,storage_service.py(ConfigError),commands-reference.md,SKILL.md✓grep -n "dev branch" tests/test_storage_swap.py→ lines 436, 456 still contain stale text ✗ (B-1)grep -n "Dev branch ID" src/keboola_agent_cli/services/storage_service.py→ line 1365 still says "Dev branch ID" ✗ (NB-1)keboola-expert.md§2 Tool Selection Matrix swap-tables row (line 152): already says "any branch incl. prod" — updated in prior #368, not in scope here ✓keboola-expert.md§3 Inline Gotchas:storage clone-tablegotcha at line 301 correctly retains "dev branch" wording (clone legitimately targets a dev branch) ✓CLAUDE.md## All CLI Commandsswap-tables signature (line 357): no descriptive prose, only the signature — no stale wording to update ✓- Layer violation check (typer/httpx in wrong layer): no new additions ✓
- Convention compliance (magic numbers, raw error codes, bare except, print, tokens): no violations in added lines ✓
Open questions for the author
(none)
…p text Two surfaces the first pass of this PR missed, flagged by kbagent-pr-reviewer: - B-1 (blocking): CLI test test_swap_missing_branch_fails_clearly mocked the OLD "swap-tables requires a dev branch" string as its side-effect and asserted on it. The mock short-circuits the real service, so the test was validating phantom text that no longer matches service output. Updated the mock + assertion to "requires a branch". - NB-1: the swap_tables Args docstring still read "branch_id: Dev branch ID"; corrected to "any branch accepted, including the default/production branch". clone-table wording left intact (clone legitimately targets a dev branch; its service message and tests correctly keep "requires a dev branch").
1 participant
What
Follow-up to #368. The 0.52.0
swap-tablessemantics correction missed fourco-located surfaces that still claimed the swap is dev-branch-only or "rejected
on production" -- now false after that fix. Flagged by the
kbagent-pr-reviewerthird pass on #368 (NB-1/2/3 + NIT-1).
Fixes
services/storage_service.pyConfigErroron missing--branchno longer says "The Storage API rejects this on production" (this text is user-facing at exit code 5 and contradicted the corrected docstring)references/commands-reference.mdcommands/storage.pydocstring (->SKILL.md)make skill-genre-runtests/test_storage_swap.pycommands/context.pyAGENT_CONTEXTThe dependent test match was also updated (
"dev branch"->"requires a branch") so the corrected error message stays covered.Scope notes
branch_idstays mandatory.clone-table's own "dev branch" wording is intentionally untouched --clone targets a dev branch, so its
match="dev branch"test is correct.Verification
make check: ruff + format clean; regeneratedSKILL.mdcommittedclone-table+ correct typify/swap dev-branch semantics (0.52.0) #368 third-pass review.