Repository navigation
Conversation
A managed-git-repo data app's first deploy omitted `configVersion` to avoid the pre-0.65.0 "dataApp.git.repository required" crash, but that alone never granted the app a workspace -- provisioning of WORKSPACE_ID/KBC_WORKSPACE_MANIFEST_PATH is gated on parameters.dataApp.git being present in Storage config, independent of hasManagedGitRepo/managedGitRepoId. Backfill the git block from the managed repo's URL before deploying and pin the resulting version, matching the path an external-git app already takes.
Jiri-Soukup
left a comment
There was a problem hiding this comment.
Review of #765 — managed-repo data apps never get a workspace on first deploy
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 fixes a real bug: deploy_data_app skipped pinning configVersion for a pure managed-repo app on its first deploy (to avoid an older crash), but workspace-grant provisioning is actually gated on parameters.dataApp.git being present in the Storage config, independent of configVersion/managedGitRepoId — so the app deployed running but with no WORKSPACE_ID. The fix backfills the git block from the managed repo's clone URL (_build_managed_git_block / backfill_managed_git) before pinning the version that write produces, extracted cleanly into resolve_effective_version. The code itself is sound (read-modify-write pattern matches existing conventions, error handling is consistent, dataclass used correctly for the grouped context argument, new tests cover the https/ssh-url preference and the no-URL failure case). Verdict: REQUEST CHANGES — the fix changes long-standing, actively-documented CLI behavior (data-app deploy "omits configVersion for a pure managed repo") in three hand-maintained, non-CI-checked docs that will now actively mislead an AI agent instead of just being silent about the change.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 1
- Non-blocking findings: 3
- Nits: 2
Blocking findings
[B-1] CLAUDE.md:863-864, plugins/kbagent/skills/kbagent/references/commands-reference.md:287, plugins/kbagent/skills/kbagent/references/gotchas.md:3711-3745 — three hand-maintained docs still state the pre-fix behavior as current guidance
All three say, in effect, "data-app deploy omits configVersion for a PURE managed repo (deploys from managedGitRepoId)" — that was exactly the buggy shortcut this PR removes. After this PR, a pure managed repo with no git block gets the block backfilled and a new pinned version, never an omitted one. gotchas.md's dedicated entry (## data-app managed-repo deploy: omit configVersion ..., since v0.65.0) goes further and explains the previous incident ("could not read Username" symptom) using reasoning that this PR now supersedes — a future reader/agent following that entry will actively recommend/expect the old, broken behavior. This is exactly the class of "CI passes, tests pass, an AI agent quietly recommends wrong behavior three weeks later" gap CONTRIBUTING.md's Plugin synchronization map exists to catch (these three files are all "NO"/"PARTLY" in the CI-catches-drift column). Fix: update all three to describe the backfill-then-pin behavior, tag the correction with (since vNEXT) (per CONTRIBUTING.md's feature-PR convention), and note in gotchas.md that the omit-configVersion workaround is retired.
Non-blocking findings
[NB-1] src/keboola_agent_cli/services/data_app_service.py:600-628 — deploy result carries no signal that it silently rewrote the Storage config
resolve_effective_version → backfill_managed_git performs a real write (a new Storage config version, with its own change_description) as a side effect of what looks like a read-then-deploy path. Neither _format_lifecycle_result's JSON fields nor its message string mention that a git block was backfilled or that the config version changed for that reason — config_version just looks like "the version deploy pinned," same as any other deploy. An agent following the "fresh fetch before write" rule (keboola-expert.md §1 Rule 1) has no way to learn from the deploy response that the config was mutated. Consider adding a git_block_backfilled: bool field (or a message clause) when this branch fires.
[NB-2] tests/test_e2e.py:13488-13572 (TestE2EManagedRepoDataApp) — no E2E test exercises the actual fixed code path
The existing managed-repo E2E test (test_managed_repo_lifecycle) creates the app, reads git-repo, lists runs, and deletes — it never calls data-app deploy, so it never exercises resolve_effective_version's backfill branch this PR adds. The author's PR description documents a manual live verification, which is good, but there's no automated regression guard against this exact bug recurring. Per CONTRIBUTING.md this can reasonably be deferred a cycle (pushing real code to a managed repo in CI is nontrivial), but it should be tracked.
[NB-3] plugins/kbagent/skills/kbagent/references/data-app-workflow.md:284-318 — the managed-repo walkthrough doesn't mention the new automatic backfill
This is the primary "Create an app on a Keboola-MANAGED git repo" recipe (steps 1-4). Step 4's comment ("The platform injects the clone credentials at deploy time, so no credential is wired into the config") remains true for credentials, but is now incomplete: as of this fix, deploy also writes parameters.dataApp.git (URL only, no credentials) into the Storage config on first deploy so the workspace grant provisions correctly. Worth a one-line addition here since this is the natural place a future reader looks for the full deploy contract.
Nits
[NIT-1]src/keboola_agent_cli/services/data_app_service.py:611— the inline comment# None only for pure managed reposon thepatch_app(..., config_version=effective_version, ...)call is stale:resolve_effective_version(see_data_app_bodies.py:264) never actually returnsNoneany more (a pure managed repo now gets a real backfilled version). Update or remove the comment.[NIT-2]src/keboola_agent_cli/services/_data_app_bodies.py:264—resolve_effective_version's return type is annotatedstr | None, but every code path either raises or returns astr; tightening to-> strwould make the (now misleading)Nonecase in callers' comments impossible to reintroduce by accident.
Verification log
gh auth status→ authenticated as Jiri-Soukup ✓gh pr view 765 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN, basemain, headjirisoukup-cli-15-managed-git-repo-workspace-fix, 3 files, +188/-43 ✓git rev-parse --abbrev-ref HEAD→ matches<branch>✓- Read
CONTRIBUTING.md(full),CLAUDE.md,plugins/kbagent/agents/keboola-expert.md(§1, §2, §3) ✓ gh pr diff 765/gh pr view --json files→ 299-line diff, 3 files, no CLI command surface change (no new@*_app.command, confirmed by reading the full diff) ✓- Layer check:
_data_app_bodies.py/data_app_service.pystay inservices/; notyper/click/console.printadded, nohttpx/requestsadded outsideclient//manage_client.py/data_science_client.py— no layer violation ✓ permissions.py—data-app.deployalready registered as"write"(unchanged; no new command, no registry gap) ✓make loc-check→ OK;services/data_app_service.pyat 1622 code lines, under its grandfathered baseline (1641) and shrinking — consistent with the diff's net -33 lines ✓uv run ruff check src/ tests/→ All checks passed ✓uv run ruff format --check src/ tests/→ 467 files already formatted ✓uv run python scripts/check_error_codes.py→ OK, no raw error_code literals ✓uv run pytest tests/test_data_app_service.py -q→ 94 passed ✓ (includes the 3 new/rewritten tests: backfill+pin, ssh-url fallback, no-URL-raises)make check/make skill-gen/ fulluv run pytest tests/ -m "not e2e"→ could not complete:scripts/generate_skill.pyand any import ofkeboola_agent_cli.clifail withImportError: cannot import name '_default_ds_client_factory' from 'keboola_agent_cli.services.data_app_service'(raised fromservices/sync_service.py:117). Verified via agit worktree addcheckout oforigin/main(10a8ac2) with the same venv — the same import error reproduces onorigin/mainitself, so this is a pre-existing break unrelated to this PR's diff, not something introduced here. It does mean 88 test modules fail to collect and the fullmake checkcannot currently pass on either branch. Not counted as a finding against this PR (out of diff scope) — see Open Questions.- Manually traced
_build_managed_git_block/backfill_managed_git/resolve_effective_versionagainstclient/configs.py::update_config's real signature and against_build_git_block's sibling shape for external repos (data_app_service.py:1715-1770) — call shape, hardcodedbranch: "main"andprivate: Trueall match established managed-repo conventions (mutually exclusive with--git-public/PAT flags) ✓ - Confirmed via
data_science_client.py:296-308,services/data_app_git_service.pymodule docstring,commands-reference.md:299,data-app-workflow.md:254-257, andgotchas.md:3711-3745step 4 thatget_git_repois exempt from the "409 until deployed once" precondition specifically for managed repos (is_managed_git_repo: trueresolves immediately viamanagedGitRepoId, no prior deploy needed) — so calling it insidebackfill_managed_gitduring the very firstdeploy_data_appcall is correct, not a latent 409 risk ✓ - Could not reproduce the live "workspace now provisions" behavior claim myself (no test project/credentials in this environment) — the author states they patched a local install and deployed a real managed-repo app end-to-end; taken as sufficient manual verification, but see NB-2 for the missing automated regression.
Open questions for the author
- Unrelated to this diff, but found while trying to run
make check:origin/maincurrently fails to import (services/sync_service.py→data_app_service._default_ds_client_factory, which no longer exists — reproduced on a cleanorigin/mainworktree). This looks like fallout from a recent merge (thedata_app_service.pyhistory on this branch showsfeat(auth): accept browser-login sessions across the service clientsright before this PR's own commits). Is this already tracked? It currently blocksmake check,make skill-gen, and the entire non-e2e test suite onmain.
CLAUDE.md, commands-reference.md, and gotchas.md still documented the pre-fix "omit configVersion for a pure managed repo" behavior as current; update them to describe the new auto-backfill instead, and note the same in data-app-workflow.md. Add a log line where backfill_managed_git rewrites Storage config, since that write was previously silent. Extend the managed-repo E2E test to cover data-app deploy, which it never exercised before. Drop a stale inline comment and tighten resolve_effective_version's return type now that it can no longer return None.
Jiri-Soukup
left a comment
There was a problem hiding this comment.
Review of #765 — managed-repo data apps never get a workspace on first deploy
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 is a targeted bug fix: deploy_data_app now backfills parameters.dataApp.git
(from the managed repo's clone URL) before pinning configVersion on a pure
managed-repo app's first deploy, because workspace-grant provisioning is gated on
that block's presence, independent of configVersion/managedGitRepoId. The
refactor extracts resolve_effective_version / backfill_managed_git /
_build_managed_git_block into _data_app_bodies.py, updates the affected test,
adds two new unit tests (ssh-url fallback, no-URL failure), and extends the E2E
managed-repo lifecycle test. All four hand-maintained doc surfaces this change
touches (CLAUDE.md, commands-reference.md, data-app-workflow.md,
gotchas.md) were updated with a vNEXT/CLI-15 tag, which is exemplary
diligence. The code change itself is small, well-tested, and consistent with the
3-layer architecture. However, make check cannot pass on this PR — not
because of anything in this diff, but because main itself (this PR's merge
base, commit 10a8ac2) is currently broken: sync_service.py imports a name,
_default_ds_client_factory, that no longer exists in data_app_service.py,
so kbagent cannot even be imported. This is inherited breakage from a prior
merged PR (c98eddf, PR #759), confirmed still red on main's own CI. Given the
strict rule that a non-zero make check is blocking, and that this PR's own CI
checks (check, test (3.12)) are failing as a direct consequence, verdict is
REQUEST CHANGES — but the actionable item is "rebase onto/wait for a
main hotfix that restores _default_ds_client_factory", not anything this PR's
author needs to redo. Everything specific to this PR's own diff looks correct.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 1
- Non-blocking findings: 2
- Nits: 2
Blocking findings
[B-1] src/keboola_agent_cli/services/sync_service.py:120 — make check / CI is red, inherited from main, not from this diff
sync_service.py does from .data_app_service import (..., _default_ds_client_factory, ...),
but data_app_service.py has not defined that name since commit c98eddf
("feat(auth): accept browser-login sessions across the service clients", merged
via PR #759). This breaks import keboola_agent_cli.cli entirely (confirmed via
a clean git worktree at main@10a8ac2, the exact commit this PR branched
from) — make check's ty check... continues past it as a warning, but
skill-check, command-sync-check, and pytest collection of ~88 test files
(everything that imports cli.py/sync_service.py transitively) all fail hard.
gh pr checks 765 confirms both check and test (3.12) are currently failing
on this exact PR, and main's own tip (10a8ac2) is also red on GitHub Actions.
This is not caused by this PR's diff — git diff origin/main...HEAD touches
none of sync_service.py's imports — but per the review playbook a non-zero
make check is blocking regardless of origin, and it means the reviewer cannot
mechanically verify anything beyond the one test file the author ran directly
(tests/test_data_app_service.py, which imports data_app_service directly and
therefore never triggers the broken chain — this is why the author's "94 passed"
claim is true but does not prove make check is green). Recommended fix: a
separate, urgent hotfix PR against main that restores
_default_ds_client_factory in data_app_service.py (or updates
sync_service.py's import) must land and be picked up here before this PR can
merge with a green CI. This PR does not need new code changes for this issue.
Non-blocking findings
[NB-1] plugins/kbagent/agents/keboola-expert.md — no update for this behavior change
Per CONTRIBUTING.md > "Plugin synchronization map", keboola-expert.md should
be updated for "behavior change (gotchas)" even when no command group is added.
The agent's §2 matrix row "Bring a new data app online" and §3 "Data apps"
inline gotchas still don't mention that pre-fix deploy silently produced a
workspace-less managed-repo app (or that this is now auto-fixed). Since the CLI
now self-heals this on every deploy, the omission is low-impact for future
users on a patched kbagent, but it is a miss for anyone diagnosing an older
install exhibiting the bug via data-app detail showing Git: {}. Per the
severity table this is explicitly NON-BLOCKING (the authoritative catalogue,
AGENT_CONTEXT, isn't affected the same way — see NB-2).
[NB-2] src/keboola_agent_cli/commands/context.py:1387-1392 — AGENT_CONTEXT's data-app deploy entry never documented the old special-case, and still doesn't document the new one
The kbagent context text for data-app deploy only ever said "Default reads
the latest Storage config version and pins to it" — it never mentioned the
pre-existing managed-repo special case (omit configVersion) nor now mentions
the new backfill-then-pin behavior. Since this text was already incomplete
before this PR (not a regression it introduced), and the change is a bug fix
to existing behavior rather than a new/renamed command, this isn't the BLOCKING
"new command missing from AGENT_CONTEXT" case in the playbook — but it's a good
opportunity, cheap to take: a one-sentence addition here would close a gap AI
agents relying on kbagent context currently have no way to learn about.
Nits
[NIT-1]src/keboola_agent_cli/commands/data_app.py:499-512(data_app_deploy, unmodified by this PR) — the--config-versionhelp text and command docstring don't mention the new auto-backfill-for-managed-repos behavior; worth a short addition given how load-bearing this fix is.[NIT-2]src/keboola_agent_cli/services/_data_app_bodies.py:191-192(backfill_managed_git) — mutates the caller'sconfigurationdict in place viasetdefault(...)["git"] = git_blockbefore passing it tostorage_client.update_config; harmless here since the dict is freshly fetched and not reused afterward, but a future caller reusingconfigurationpost-call would be surprised by the mutation. A short comment or adict(configuration)shallow copy would make the "we mutate, not replace" contract (already asserted by the tests) explicit at the definition site, not just in the test comment.
Verification log
- Read
CONTRIBUTING.md(Checklist for new commands, Plugin synchronization map, Releasing) andCLAUDE.mdconvention #17 ✓ - Read
plugins/kbagent/agents/keboola-expert.md§1/§2/§3 ✓ gh auth status→ authenticated ✓gh pr view 765 --json title,body,files,...→ state OPEN, 8 files, +262/-58 (final: +282/-58 after the second commit), title uses no conventional-commit prefix in the PR title itself but the underlying commits usefix:— matches a bug-fix PR ✓- Working tree was initially on this branch but one commit BEHIND the true PR head (
gh pr view --json headRefOid=d643509, local HEAD was7fa89a1); fast-forwarded locally (git merge --ff-only origin/<branch>) before continuing — no destructive operation, no work lost (clean tree) ✓ git diff origin/main...HEAD(8 files, +282/-58) reviewed in full ✓- Layer-compliance greps (typer/click/formatter in services; httpx/requests in commands; magic numbers; raw error_code strings; bare except; print(); token leakage; new tuple returns) → all empty, no violations ✓
ErrorCode.API_ERRORexists inerrors.py✓;KeboolaClient.update_config/DataScienceClient.get_git_reposignatures confirmed to match the new call sites ✓make loc-check→ OK, no file crosses a hard ceiling because of this PR ✓make check-error-codes→ OK, no raw error-code literals ✓make command-sync-check→ fails with the_default_ds_client_factoryImportError (see B-1) — confirmed via an isolatedgit worktreeatmain@10a8ac2that the identical failure pre-exists onmain, unrelated to this diff ✓gh pr checks 765→checkfail,test (3.12)fail, Windows build fail;gh api repos/keboola/cli/commits/10a8ac2/check-runs→ same failures already present onmaintip, confirming inheritance ✓uv run pytest tests/test_data_app_service.py -q→ 94 passed (matches PR description's claim) ✓ — this file importsdata_app_servicedirectly and does not go through the brokencli.py/sync_service.pychain, which is why it passes despitemainbeing brokenuv run pytest tests/ -q -m "not e2e"→ 88 collection errors, allImportError: cannot import name '_default_ds_client_factory'(see B-1) ✓- Could not reproduce the live managed-repo deploy behavior end-to-end (no
/tmp/kbagent-e2ecredentials / live managed-repo app available in this environment); the author's PR description states they manually verified against a real project ("patched a local kbagent install ... deployed a fresh managed-repo app end to end") — taken at face value, not independently re-verified here - Read the new
_build_managed_git_block/backfill_managed_git/resolve_effective_versioncode directly; confirmed no new bare tuple returns, no layer violations,ManagedGitBackfillTargetcorrectly uses a frozen dataclass per CONTRIBUTING.md's "name multi-value returns" rule rather than a positional tuple ✓ - Confirmed
data-app.deployalready registered inpermissions.pyOPERATION_REGISTRYas"write"(no new command, so no new registration needed) ✓
Open questions for the author
- Is a hotfix for the
_default_ds_client_factorymainbreakage already in flight? If so, worth linking it in this PR's description so reviewers know the red CI here is expected and unrelated.
…aged-git-repo-workspace-fix
A vNEXT placeholder inside a markdown heading is fatal on every PR, not just release PRs: resolving it later rewrites the heading's anchor slug and breaks inbound links (test_check_version_gates.py:: TestHeadingPlaceholders). Move the CLI-15 pointer into a body sentence instead of the heading itself.
Jiri-Soukup
left a comment
There was a problem hiding this comment.
Review of #765 — managed-repo data apps never get a workspace on first deploy
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 fixes a real bug: deploy_data_app never wrote parameters.dataApp.git
for a pure managed-repo data app, so workspace grant provisioning (which is
gated on that block's presence, independent of managedGitRepoId) silently
never happened, even though state: running and runtime.workspace.enabled: true looked healthy. The fix extracts resolve_effective_version +
backfill_managed_git (+ a small ManagedGitBackfillTarget dataclass) into
_data_app_bodies.py, backfills the git block from get_git_repo before
pinning configVersion, and updates docs (CLAUDE.md, commands-reference.md,
data-app-workflow.md, gotchas.md) accordingly. Code quality, layer
separation, error handling, and test coverage (positive path, ssh-url
fallback, empty-lookup failure) are all solid, and make check is fully
green. Verdict: COMMENT — no blocking issues, but this is exactly the kind
of "fails silently" behavior class the plugin doc set warns agents about, and
keboola-expert.md §3 (the highest silent-drift-risk file per
CONTRIBUTING.md) was not touched even though it already carries a sibling
bullet for the closely-related --workspace silent-failure gotcha.
Verdict
- Verdict: COMMENT
- Blocking findings: 0
- Non-blocking findings: 4
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] plugins/kbagent/agents/keboola-expert.md:346-364 — new silent-failure gotcha not added to §3 "Data apps"
This section already has a sibling bullet ("Data app Storage access fails
SILENTLY") for the closely related --workspace gap. The bug this PR fixes is
the same failure class (workspace never provisioned, no platform-side
diagnostic, state: running looks healthy) for managed-repo apps specifically.
CONTRIBUTING.md > "Documentation changes (mandatory!)" calls
keboola-expert.md the highest silent-drift risk file and explicitly asks
for a §3 bullet "when behavior changed in a way the agent will get wrong by
default." An agent relying only on this system prompt (not gotchas.md, which
was updated) will not know a managed-repo data-app deploy used to silently
skip workspace provisioning, or that it is now fixed. Per the review rubric
this is NON-BLOCKING (the authoritative catalogue is AGENT_CONTEXT), but the
value here is unusually high given the existing sibling bullet right above it.
Suggested addition: one line under "Data apps" pointing at
gotchas.md's new "Correction (vNEXT/CLI-15)" entry.
[NB-2] src/keboola_agent_cli/commands/context.py:1387-1392 — AGENT_CONTEXT doesn't mention the new git-block backfill side effect
The data-app deploy entry in AGENT_CONTEXT ("Default reads the latest
Storage config version and pins to it...") happens to still read as accurate
after this fix, but it omits that a pure managed-repo app's first deploy now
also writes to Storage (a PUT on the config with change description
"Auto-backfill managed-repo git block...") before pinning. AGENT_CONTEXT is
the CI-checked-for-presence, hand-maintained "primary reference loaded by AI
agents at session start" (convention #17) — an agent following it has no way
to know deploy can mutate the config it is about to redeploy from. Consider
a one-line addition mirroring the --workspace retrofit note just above it.
[NB-3] src/keboola_agent_cli/services/data_app_service.py:600-633 — the git-block backfill is invisible in the command's own output
backfill_managed_git (_data_app_bodies.py:179-211) logs the backfill via
logger.info only. Neither _format_lifecycle_result's JSON payload nor the
human-mode rendering of data-app deploy surfaces that a Storage write just
happened (e.g. a git_block_backfilled: true / git_backfilled_from field).
CONTRIBUTING.md's UX checklist for new/changed commands says "Write
operations log what they did (created X, uploaded Y rows)" — right now that
only happens in the server-side log stream, not in the response a caller
(human or agent) actually sees. Since this write also lands in the config's
version/change-description history, a caller diffing configs later will see
an unexplained version bump they didn't request.
[NB-4] Behavior claim not independently reproduced
The PR description states the author "patched a local kbagent install...
and deployed a fresh managed-repo app end to end." I do not have a live
Keboola stack + Data Science credentials in this environment to reproduce that
claim, so I could not independently verify the workspace now provisions on a
real deploy. The unit test coverage (positive path, ssh-url fallback, and the
git-repo-lookup-empty failure path in tests/test_data_app_service.py) and the
extended tests/test_e2e.py::test_managed_repo_lifecycle (which now asserts
detail["git"]["repository"] == repo["https_url"] after deploy) are strong
circumstantial evidence, but the actual container-workspace-grant outcome is
only asserted by the author's manual test, not by CI (the E2E suite is
nightly/on-demand, not run here).
Nits
[NIT-1]src/keboola_agent_cli/services/_data_app_bodies.py:232—_build_managed_git_blockhardcodes"branch": "main".get_git_repo()'s response shape (sshUrl/httpsUrl/isManagedGitRepo) carries no branch field, so this matches the documentedgit push ... HEAD:mainworkflow, but if a user ever pushes to a differently-named default branch the backfilled block will silently point at the wrong ref. Worth a one-line comment noting the assumption is tied to the documented push instructions.[NIT-2]tests/test_data_app_service.py:351-434— the three newdeploy_data_apptests (backfill success, ssh-url fallback, empty-lookup failure) don't assertstorage_mock.close.assert_called_once()even thoughstorage_clientis a newly-exercised resource in this code path (the service's ownfinallyblock does close it correctly, so this is a test-completeness nit, not a leak).
Verification log
git rev-parse --abbrev-ref HEAD→jirisoukup-cli-15-managed-git-repo-workspace-fix(matches<branch>) ✓gh auth status→ authenticated asJiri-Soukup✓- Read
CONTRIBUTING.md(Checklist: Adding a New CLI Command, Plugin synchronization map, Releasing a new version) ✓ - Read
CLAUDE.mdconvention #17 and## All CLI Commands(data-app section) ✓ - Read
plugins/kbagent/agents/keboola-expert.md§1 and §3 ✓ gh pr view 765 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN,main<-jirisoukup-cli-15-managed-git-repo-workspace-fix, +265/-57, 8 files ✓gh pr diff 765→ 481 lines, read in full ✓- Conventional commit check:
git logon the branch showsfix:commits (fix: backfill managed-repo git block...,fix: address self-review findings...,fix: move the vNEXT tag...) — matches a bug-fix PR ✓ - Layer-compliance greps (typer/click/formatter in services diff, httpx/requests in commands diff, formatter/typer in client diff) → all empty, no violations ✓
- Convention greps (magic numbers, raw
error_code="..."strings, bareexcept:,print(), token leaks, new baretuple[...]returns) → all empty ✓ make check→ 6790 passed, 12 skipped, exit 0 (lint + format + typecheck + skill freshness + version-sync + command-sync + endpoints + changelog-audit-skipped-locally + error-codes + sentinel-guards + loc-check + full test suite) ✓uv run python scripts/check_version_gates.py→ "All 355 version gates... resolve to a release. 7 unresolved 'vNEXT' gate(s) awaiting the next release PR" — the new(since vNEXT/CLI-15)tags ingotchas.md/CLAUDE.md/commands-reference.md/data-app-workflow.mdare correctly placed on body lines, not ATX headings ✓grep -n "data-app.deploy" src/keboola_agent_cli/permissions.py→"data-app.deploy": "write"already registered (no new command added by this PR, so no registry change is needed) ✓- Traced
create_data_app'suse_managed_git_repo -> deploy=False(unconditional, regardless of--wait) indata_app_service.py:379-385— confirms the PR description's root-cause claim thatdeploy_data_appis the only path needing the fix ✓ - Checked
KeboolaClient.update_configsignature (client/configs.py:488) againstbackfill_managed_git's call — parameter names/order match ✓ - Compared
_build_managed_git_block's hardcoded"private": Trueagainst the external-repo git-block builders (_build_git_block,_build_storage_config_body) — consistent with "managed repos are always private, Keboola-hosted" ✓ uv run python scripts/check_file_size.py→ no_data_app_bodies.py/data_app_service.pyfailures (both stay under budget) ✓- Could NOT run
tests/test_e2e.py::test_managed_repo_lifecycleagainst a real Keboola stack (noE2E_API_TOKEN/E2E_URL/Data Science sandbox in this environment) — see NB-4.
Open questions for the author
(none)
soustruh
left a comment
There was a problem hiding this comment.
Review of #765 — managed-repo data apps never get a workspace on first deploy
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
The PR fixes deploy_data_app: a data app on a pure managed git repo (created via --use-managed-git-repo, no parameters.dataApp.git block yet) previously deployed with configVersion omitted and never got a workspace grant, even with runtime.workspace.enabled: true — the app reported state: running with no WORKSPACE_ID/KBC_WORKSPACE_MANIFEST_PATH and no platform-side diagnostic. The fix resolves the managed repo's clone URL, backfills parameters.dataApp.git into the Storage config before pinning configVersion, and updates CLAUDE.md, commands-reference.md, data-app-workflow.md, and gotchas.md accordingly (all tagged vNEXT/CLI-15). The command surface, permissions, and hint registries are correctly untouched since no command/flag was added. Scope matches the description; commit prefixes (fix:) are correct; make check (lint, format, typecheck, skill/version/changelog/error-code/sentinel/loc gates, full test suite — 6790 passed) is green. One real bug found in the new backfill path: a silent fallback to the stale pre-backfill Storage version if the API response ever omits version, which would defeat the fix it ships. Verdict: REQUEST CHANGES on that one finding; everything else is solid.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 1
- Non-blocking findings: 2
- Nits: 0
Blocking findings
[B-1] src/keboola_agent_cli/services/_data_app_bodies.py:269 — silent fallback to the stale pre-backfill version defeats the fix on an edge case
backfill_managed_git does new_version = str(updated.get("version", "") or ctx.latest_version). If the Storage PUT response ever omits version (empty/falsy), this silently pins ctx.latest_version — the version before the git block was written, i.e. exactly the configuration this PR exists to stop deploying from. That resurrects the original bug with zero error and zero log signal.
The sibling code path for the identical situation, create_data_app (data_app_service.py:475-483, unchanged by this PR), does the opposite: it raises KeboolaApiError loudly when storage_response.get("version") is empty rather than falling back to anything. backfill_managed_git's own docstring for _build_managed_git_block states the design intent explicitly ("fail loudly rather than deploy with no source pointer") — this fallback contradicts that intent for the immediately next step of the same flow.
Fix: raise KeboolaApiError (mirroring create_data_app's handling) when updated.get("version") is empty, instead of falling back to ctx.latest_version.
Non-blocking findings
[NB-1] plugins/kbagent/agents/keboola-expert.md — no §3 Inline Gotchas entry for the vNEXT/CLI-15 correction
gotchas.md, commands-reference.md, CLAUDE.md, and data-app-workflow.md all got the (vNEXT/CLI-15) correction; keboola-expert.md §3 (the file CONTRIBUTING.md calls "highest silent-drift risk") did not. An agent operating a pre-fix kbagent install following the existing §2 matrix row 141 recipe (--use-managed-git-repo → git-credentials-create → deploy) has no inline trigger warning it that the app can come up running with no workspace. Per this reviewer's own severity table a missing §3 line is non-blocking, but it is a one-line, low-cost addition worth doing before merge.
[NB-2] src/keboola_agent_cli/services/_data_app_bodies.py:232 — hardcoded "branch": "main" in the backfilled git block
_build_managed_git_block always writes "branch": "main", because get_git_repo's response ({"sshUrl", "httpsUrl", "isManagedGitRepo"}) carries no branch field to read instead — so there is no better source available, and the documented workflow (data-app-workflow.md) tells users to git push HEAD:main. Still, a user who pushes their managed repo's code to any other branch gets a silently wrong git block (points deploy at a branch that was never pushed to), with no validation or warning anywhere in this path. Worth a one-line callout in data-app-workflow.md or validate-repo that the managed-repo flow requires main specifically.
Verification log
Read CONTRIBUTING.md§"Checklist: Adding a New CLI Command", §"Plugin synchronization map", §"Releasing a new version" ✓Read CLAUDE.mdconvention #17,## All CLI Commands(data-app section) ✓Read plugins/kbagent/agents/keboola-expert.md§1, §2 (data-app rows), §3 ✓gh auth status→ logged in assoustruh✓gh pr view 765 --json title,body,files,...→ state OPEN, 8 files, +265/-57, all commits usefix:prefix ✓git rev-parse --abbrev-ref HEAD→jirisoukup-cli-15-managed-git-repo-workspace-fix, matches<branch>✓; HEADc8b6894matches<pr_url>head SHA ✓gh pr diff 765(481 lines) +git diff origin/main...HEAD --stat→ confirms 8 files, and zero changes tocommands/,cli.py,server/routers/*,permissions.py,hints/definitions/*— consistent with "no command/flag added", so those silent-drift surfaces correctly needed no update ✓- Layer-violation greps (typer/console in
services/, httpx incommands/, formatter/typer in clients) → all empty ✓ - Convention greps (magic numbers, raw error-code strings, bare
except:,print(), token/secret leaks) → all empty ✓ uv run python scripts/check_file_size.py→ nodata_app_service.py/_data_app_bodies.pyviolations (file shrank net -33 code lines; helper grew but stays under budget) ✓make check(background, ~187s) → 6790 passed, 12 skipped, exit code 0 (lint + format + typecheck + skill-check + version-check + version-gate-check + command-sync-check + endpoints-check + changelog-check + check-error-codes + check-sentinel-guards + loc-check + full suite) ✓- Read
src/keboola_agent_cli/client/configs.py::update_configsignature → matches the kwargsbackfill_managed_gitcalls it with ✓ - Read
create_data_app(data_app_service.py:475-483) → confirms the sibling "fail loudly on missing version" precedent that B-1 is measured against - Read
permissions.pyOPERATION_REGISTRY→data-app.deployalready"write"; unaffected by the extra internal Storage write this PR adds ✓ - Did not perform a live end-to-end reproduction (create a real managed-repo data app, deploy, inspect
WORKSPACE_ID) — that requires provisioning and tearing down a real cloud data app, which is out of scope for a read-only review pass. Relying instead on: the three new/rewritten unit tests intests/test_data_app_service.py(backfill + ssh-url fallback + empty-lookup failure, all passing undermake check), the updatedtests/test_e2e.py::test_managed_repo_lifecycle(asserts the backfilled git block viadata-app detailafterdeploy), and the author's own stated manual verification in the PR description (patched local install, live managed-repo deploy).
Open questions for the author
(none)
…anaged-git-repo-workspace-fix
backfill_managed_git fell back to ctx.latest_version when the Storage update response carried no version -- that pins the pre-backfill config, i.e. the exact no-git-block state CLI-15 exists to fix, with no error or log signal. Raise KeboolaApiError instead, mirroring create_data_app's handling of the same case. Addresses review finding B-1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e main-branch contract - keboola-expert.md §3: inline trigger for a managed-repo app running with no WORKSPACE_ID (review NB-1). - data-app-workflow.md: the backfilled git block always targets `main`, since get_git_repo returns no branch -- push the managed repo's code there (review NB-2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Michal Ševčík asked to move the agent and CLI install steps out of the Apps UI and into the docs. The task now has one page in Apps (data-apps/build-with-an-agent). Per-client install stays on cli/for-agents with a new dataapp-developer section, and the pages that repeated or contradicted it now link there. The Claude Code flow was run live in project 264 on 2026-09-23. Until keboola/cli#765 ships, the page works around kbagent CLI-15 (a managed-repo app gets no workspace). The Claude Desktop tab stays VERIFY until its own live run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…aged-git-repo-workspace-fix
soustruh
left a comment
There was a problem hiding this comment.
Review of #765 – managed-repo data apps never get a workspace on first deploy
Summary
data-app deploy on a pure managed-repo app (hasManagedGitRepo, no parameters.dataApp.git) now reads the repo URL from get_git_repo, writes parameters.dataApp.git (repository, branch: main, private: true) into the Storage config, and pins the version that write produced, instead of omitting configVersion. The code is clean on layering, error codes and token handling, make check passes on the PR head, and the service tests cover the new paths. Verdict: COMMENT. There is no blocking finding. The open points are a hard-coded branch: main with no user-visible report of the Storage write, one documentation file that was skipped (AGENT_CONTEXT), stale text left in gotchas.md, and a real-stack claim I could not reproduce.
Verdict
- Verdict: COMMENT
- Blocking findings: 0
- Non-blocking findings: 7
- Nits: 5
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/services/_data_app_bodies.py:234 – backfill hard-codes branch: main
Before this PR a pure managed repo deployed from app.managedGitRepoId with no branch in the Storage config. Now the config carries branch: main. A repo whose code sits on another branch (or whose default branch is not main) deploys the wrong source or an empty one. data-app-workflow.md accepts this and says "nothing warns you". That is a new silent failure mode, introduced by a command that used to work for such repos. Fix: report the branch that was written (see NB-2) so the user sees it, or read the branch from the repo or app record if the Data Science API returns one. See also the open question on what the platform cloned before.
[NB-2] src/keboola_agent_cli/services/_data_app_bodies.py:285 – deploy now writes to Storage config but reports nothing
The write is only sent to the logger (logger.info), not to the command output. The deploy result (--json and human mode) does not say that a git block was written, which URL or branch it holds, or that the version changed because of it. CONTRIBUTING "UX considerations" asks that write operations log what they did. data-app.deploy is a write operation (permissions.py:233), so the permission class is fine, but the user cannot see the side effect. Fix: add a field such as backfilled_git_block plus a warnings[] entry in the result, and print one line in human mode.
[NB-3] src/keboola_agent_cli/commands/context.py:1411 – AGENT_CONTEXT not updated for the new deploy behavior
The PR updates CLAUDE.md, commands-reference.md, data-app-workflow.md, gotchas.md and keboola-expert.md §3, but not AGENT_CONTEXT, which CONTRIBUTING calls the primary reference agents load at session start. The --use-managed-git-repo text (line 1411) still says it "writes no git block" with no mention that deploy later writes one, and the data-app deploy entry (line 1441) says only "Default reads the latest Storage config version and pins to it". Fix: add two or three lines to the deploy entry: the backfill, and that the managed repo must be pushed to main. This is a behavior change of an existing command, not a new command, so I rate it NON-BLOCKING.
[NB-4] plugins/kbagent/skills/kbagent/references/gotchas.md:4147 – old rule left in place next to the correction
The section heading still reads "omit configVersion (the platform injects clone creds)", and the bullet at lines 4179-4186 still says data-app deploy "omits configVersion for a pure managed repo". The new "Correction" bullet at line 4196 contradicts both. An agent reading the section top-down takes the old rule first. Fix: reword the heading and the old bullet in place, and keep the history in the correction.
[NB-5] src/keboola_agent_cli/services/_data_app_bodies.py:263 – write path is less tolerant than the read path
The read at line 325 uses (configuration.get("parameters") or {}).get("dataApp") or {} and so accepts null or empty-array values. The write at line 263, configuration.setdefault("parameters", {}).setdefault("dataApp", {})["git"] = ..., does not. I checked this with a mocked probe: parameters: [] raises AttributeError, dataApp: null raises TypeError, dataApp: [] raises TypeError. The user then gets a traceback instead of a structured KeboolaApiError. The shapes are rare, but the read path already tolerates them, so the write path should too. Fix: normalise both levels to dicts before the assignment.
[NB-6] tests/test_data_app_service.py:1104 – test gaps on the new path
The new tests do not cover: branch_id reaching update_config for deploy --branch; an explicit --config-version on a pure managed repo (it skips the backfill, so the workspace gap stays; a test or one doc line would make this explicit); and ds_mock.close / storage_mock.close being called once, in particular on the two raising paths (CONTRIBUTING "Testing Guidelines"). Fix: add these three assertions or cases.
[NB-7] tests/test_e2e.py:13753 – the core claim is not proven by the E2E test, and I could not reproduce it
The E2E step deploys an empty repo and asserts only that detail["git"] holds the clone URL. Its own comment says it does not prove a container start, so nothing in the suite proves that WORKSPACE_ID is injected, which is the bug being fixed. I did not run E2E or any kbagent command against a real project (no credentials, and I was told not to), so the description's claim "the app loaded real Storage data on the first deploy" is unverified by me. Fix: the author should confirm it on the stack used, and ideally add the config detail / app evidence to the PR.
Nits
[NIT-1]PR title – "managed-repo data apps never get a workspace on first deploy" has no conventional prefix; this is afix:(for examplefix(data-app): ..., like #825 and #836). A squash merge would take the title as the commit subject.[NIT-2]scripts/file_size_baseline.json:7–services/data_app_service.pyis recorded at 1633 code lines but is now 1615 after the extraction. Runmake loc-baselineso the ratchet records the shrink and the 18 lines are not available for regrowth.[NIT-3]src/keboola_agent_cli/data_science_client.py:215– thepatch_appdocstring still documents the "EXCEPTION (python-js managed repos)"config_version=Nonepath and points todeploy_data_appfor it. That branch no longer exists;deploy_data_appnow always sends a version.[NIT-4]src/keboola_agent_cli/services/_data_app_bodies.py:265and:319–"keboola.data-apps"is repeated with "mirrors DATA_APP_COMPONENT_ID" comments instead of a shared constant (DATA_APP_COMPONENT_IDis indata_app_service.py:75andsearch_service.py:41). Alsods_clientandstorage_clientare typedAny(the service types the Data Science client), andManagedGitBackfillTarget(line 238) exists only to shorten one call site.[NIT-5]CLAUDE.md:928,gotchas.md:4196,data-app-workflow.md:358– the tag is writtenvNEXT/CLI-15. CONTRIBUTING asks for(since vNEXT).make vnext-resolveturns this intoX.Y.Z/CLI-15, which the gate regex inscripts/check_version_gates.pydoes not parse, soversion-gate-checkcannot verify it later.
Verification log
gh auth status→ authenticated (accountsoustruh).gh pr view 765 --json ...→ OPEN, basemain, head33e7f3e6, 9 files, +330/-56. CI at review time:checkandauditSUCCESS;test (3.12)and the Windows job still running.- Local checkout HEAD is
33e7f3e6, equal to the PR head SHA, working tree clean. The local branch is namedfix/765-merge-main, not the PR branch name; I took the SHA match as the check. I ran all checks in a copy of the checkout with a freshuv sync --frozen --all-extras(all uv state in a scratch directory); nothing in the checkout was changed. - Read
CONTRIBUTING.md(lines 1-796, up to the beta-release section),CLAUDE.md, andkeboola-expert.md§1, §2 and §3 (lines 1-260 and 370-410). make checkon the PR head → exit 0. ruff check and format clean;ty0 errors (one warning: unresolved importhatchlinginscripts/hatch_build.py, caused by my scratch venv, not by the PR);skill-check,version-check,version-gate-check,command-sync-check,endpoints-check,changelog-check,check-error-codes,check-sentinel-guards,loc-checkall OK; the full unit suite (e2e excluded) had 0 failures.version-gate-checkreports 40 pendingvNEXTplaceholders, which is expected in a feature PR.- Layer greps over the diff (typer/click/formatter in services, httpx/requests in commands, formatter/typer in clients) → empty. Magic-number, raw
error_codeliteral, bareexcept:,print(), token-pattern, real-token-pattern and newtuple[...]return greps → empty. The onlytoken/passwordhits are existing doc lines and a test assertion. - Plugin synchronization map, row by row:
AGENT_CONTEXTnot touched (NB-3);CLAUDE.mdupdated;keboola-expert.md§3 updated (no new command group, so no §2 row; no new version floor, so §1 Rule 6 unchanged);commands-reference.mdupdated;gotchas.mdupdated with avNEXTtag (NIT-5, NB-4);data-app-workflow.mdupdated. No new command, so no newOPERATION_REGISTRYentry (data-app.deployis alreadywrite), no new server route, no new hint definition. wc -c plugins/kbagent/agents/keboola-expert.md→ 69,334 B against the 70,000 B cap (666 B left); the budget test passes. The next addition to that file needs trimming first.- Mocked probe of
resolve_effective_versionwithparameters: [],dataApp: null,dataApp: []→AttributeError/TypeError/TypeError; withparametersmissing → works (NB-5). Run withpython -Ifrom a scratch directory. loc-report→services/data_app_service.py1615 code lines vs baseline 1633 (NIT-2);services/_data_app_bodies.py199 code lines, within budget.- Not run: E2E tests and any
kbagentcommand against a real Keboola project (no credentials, not allowed). The real-stack behavior (workspace injected on first deploy, clone credentials still injected when aprivate: truegit block without credentials is present) is unverified by me. The PR adds no new warning or output line, so there was no human-mode versus--jsoncomparison to make.
Open questions for the author
- What source confirms that workspace grant provisioning is gated on
parameters.dataApp.git(sandboxes-service code or an API contract)?gotchas.mdnow states it as fact, and I could not check the server side from this repository. - Which branch did the platform clone for a pure managed repo before this change (the repo default, or
main)? If it was the repo default, forcingmainchanges behavior for repos whose default is notmain(NB-1). - On which stack was the live end-to-end test done? The 0.65.0 notes in
gotchas.mdrecord stack-specific managed-repo behavior.
zajca
left a comment
There was a problem hiding this comment.
Actionable findings from the automated review.
soustruh
left a comment
There was a problem hiding this comment.
Follow-up review of 08487f9. One non-blocking finding. The rest of the commit checks out.
Checked:
--git-branch(defaultmain) is passed the same way in the CLI, the service and the REST route, and only reaches the git block backfill. An explicit--config-versionskips the backfill.git_backfilled,git_branchandwarnings[]are set only when the backfill ran. The password warnings still append to the same list. The docs (CLAUDE.md, commands-reference.md, gotchas.md, data-app-workflow.md, context.py, keboola-expert.md) match this behaviour._check_text_fieldis a straight move of the old loop body fromcreate, socreatevalidation is unchanged.tests/test_data_app_service.py,tests/test_data_app_cli.pyandtests/test_server_router_calls.pypass (315 tests). The new tests assert the written branch, the result fields and the no-backfill cases.
zajca
left a comment
There was a problem hiding this comment.
Actionable findings from the automated review.
…pp that already ran
zajca
left a comment
There was a problem hiding this comment.
No actionable findings were found by the automated review.
In short: Code fix. A managed-repo app gets no workspace until
parameters.dataApp.gitis in its Storage config.deploy_data_appnever wrote it. This PR makesdeploy_data_appwrite it automatically, before the first deploy.Symptom: the app status is
running, and the config hasruntime.workspace.enabled: true. But the app has noWORKSPACE_IDand noKBC_WORKSPACE_MANIFEST_PATH.Root cause:
•
deploy_data_appis the method behindkbagent data-app deploy. It has a branch for a managed-repo app with no git block in its Storage config. The branch correctly detects that case. It then skips pinningconfigVersion. A comment there says this skip avoids an older crash.• The code can take that branch correctly. The app still gets no workspace. Workspace grant provisioning requires
parameters.dataApp.gitin the Storage config. This does not depend onmanagedGitRepoId, onhasManagedGitRepo, or on whether the code pinsconfigVersion.•
create_data_app's own deploy call does not skip this check either. The code never reaches that call for a managed-repo app:create_data_appalways setsdeploy=Falsewhen the command uses--use-managed-git-repo, regardless of--wait. The bug is only indeploy_data_app.The fix: before pinning a version,
deploy_data_appnow resolves the managed repo's clone URL and writes it intoparameters.dataApp.git, then pins the version that write produced. No manual step, no doc workaround -- the CLI does it.Changes in
src/keboola_agent_cli/services/:_data_app_bodies.py-- three new functions:_build_managed_git_block(builds the git block from aget_git_repolookup, httpsUrl preferred, sshUrl fallback),backfill_managed_git(writes it to Storage config, returns the version to pin),resolve_effective_version(the full configVersion decision, extracted out ofdeploy_data_appso it stays a call site).data_app_service.py--deploy_data_appcallsresolve_effective_versioninstead of inlining the old (wrong) skip-the-pin logic.tests/test_data_app_service.py-- rewrote the test that asserted the old, incorrect "omit configVersion" behavior. Added coverage for the ssh-url fallback and for the case where the git-repo lookup returns no URL at all.Tested: patched a local kbagent install with this exact change and deployed a fresh managed-repo app end to end -- create, push code, deploy, no manual config edit. The git block was populated automatically and the app loaded real Storage data on the first deploy.
ruff format,ruff check,ty check, andpytest tests/test_data_app_service.py(94 passed) are clean.make loc-checkpasses.self-review skipped: no gh auth in this environment
Fixes CLI-15.