[soustruh]

Why

A data app's runtime type (python-js, streamlit, ...) lives only on the Data Science /apps record, never in the Storage config. sync pull read only the Storage config, so it dropped the type. sync push and sync clone recreated the config through the Storage API alone. A cloned python-js app then deployed as the platform default, streamlit. The bug report was about sync clone. The same gap exists on plain sync push.

What changed

Two changes, both limited to keboola.data-apps:

• pull reads the type from the Data Science /apps list and writes it into the config's _keboola metadata block as data_app_type. That block holds kbagent's own bookkeeping in each _config.yml. It never reaches the Storage API, and kbagent excludes it from the config hash, so the new line produces no sync diff change on existing trees.
• push creates a keboola.data-apps config through the Data Science create_app call. That call creates the DS record with the type and its Storage config. push then fills the body with update_config and updates the parameters.id link to the new app. Without a recorded type, push uses the plain create_config path unchanged.

The DS create logic lives in data_app_service.create_synced_data_app. push_create only delegates for that one component. sync clone uses the same push().

The /apps list is not only data apps

list_apps() also returns sandbox and workspace records. Each carries a parent component's id (for example keboola.ex-db-mysql) and a backend type such as snowflake. So kbagent builds the type map from componentId == keboola.data-apps records only. It writes the type into keboola.data-apps configs only. A live pull of project 4214 confirmed this. It set the type on 21 data apps (14 python-js, 7 streamlit) and added it to no other config.

Testing

• New tests/test_sync_data_app_type.py:
  • pull writes the type into _keboola.
  • the map filter and the write guard defend against sandbox records and against a config id shared across components.
  • push sends the type and updates parameters.id.
  • a push with no recorded type uses create_config instead.
• Added create_synced_data_app unit tests, plus api_config_to_local tests for the type and for hash-invisibility.
• Full sync and data-app suites pass. ruff 0.16.5, ruff format, and ty are clean.
• Live verification on project 4214 was read-only (pull only, no writes to any project).

Scope note

A data-apps config's is_disabled flag does not travel through the DS create path, so a cloned app deploys enabled. This is out of scope for CLI-8. I can do a follow-up if you want.

Linear: CLI-8

[linear-code]

CLI-8

[soustruh]

Review of #752 — fix(sync): keep a data app's runtime type through pull and clone (CLI-8)

Generated by kbagent-pr-reviewer subagent. Verdict and findings below are advisory; the human author retains every veto. CI-coverable issues (lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR fixes a real bug: sync pull dropped a keboola.data-apps config's runtime type (python-js/streamlit), and sync push/sync clone then recreated the config under the platform default. The fix stores the type in the local _config.yml's hash-ignored _keboola footer on pull, and routes a data-app CREATE with a recorded type through a new Data-Science-aware helper on push. The core logic is sound and its field-shape assumptions check out against a live project, but the PR cannot merge as submitted: it grows an already-over-budget service file past its grandfathered file-size ceiling, which fails CI's file-size gate directly, and it changes non-obvious sync behavior with no corresponding entry in plugins/kbagent/skills/kbagent/references/gotchas.md.

Verdict

• Verdict: REQUEST CHANGES
• Blocking findings: 2
• Non-blocking findings: 4
• Nits: 1

Blocking findings

[B-1] src/keboola_agent_cli/services/data_app_service.py:144-206 — new create_synced_data_app pushes the file past its grandfathered file-size ceiling

services/data_app_service.py is already over its 1500-line hard ceiling and grandfathered at 1641 code lines in scripts/file_size_baseline.json. The new create_synced_data_app function (lines 144-206) grows the file to 1679 code lines, which fails make loc-check locally (FAIL: services/data_app_service.py grew to 1679 code lines, past its grandfathered 1641) and tests/test_check_file_size.py::TestRepoState::test_repo_passes_its_own_gate. scripts/check_file_size.py runs as its own step in .github/workflows/ci.yml's check job, so this is a certain CI failure, not just a local nuance. Move create_synced_data_app into a sibling module (e.g. _sync_data_app_ops.py, next to the existing _sync_push_ops.py) or otherwise shrink the file below 1641 before merging.

[B-2] src/keboola_agent_cli/services/sync_service.py:588-601 + _sync_push_ops.py:434-446 — no gotchas.md entry for the new data-app-type pull/push behavior

The diff touches no file under plugins/kbagent/. This PR changes the observable behavior of sync pull (writes a new _keboola.data_app_type key) and sync push/sync clone (routes data-app creates through the Data Science API when a type is recorded) for keboola.data-apps configs — exactly the class of change CONTRIBUTING.md's Plugin synchronization map calls out for gotchas.md: "New non-obvious behavior -- always tag with a version," rated BLOCKING when missing because an untagged behavior change means an AI agent following the existing data-app gotchas (which already document several other data-app silent-failure modes, e.g. "Data app Storage access fails SILENTLY") never learns about this one. Add a (since vNEXT)-tagged entry; while there, a matching one-liner in keboola-expert.md §3 and the sync pull/sync push bullets in commands-reference.md would close the loop.

Non-blocking findings

[NB-1] src/keboola_agent_cli/services/data_app_service.py:198-206 — is_disabled is dropped on the new DS-routed create path

create_synced_data_app's update_config call omits is_disabled, so it defaults to None ("leave unchanged"), and the app is created enabled regardless of the source config's flag. The PR description discloses this ("a cloned app deploys enabled ... out of scope"), but the fix is a two-line addition: update_config already accepts is_disabled: bool | None (client/configs.py:497, exercised by this PR's own FakeApi.update_config test double), and the sibling call two lines below (_sync_push_ops.py:454) already computes bool(local_data.get("is_disabled", False)). This is also a regression relative to the pre-PR create_config path, which did honor is_disabled. Worth folding into this PR rather than a follow-up, given how small the fix is.

[NB-2] src/keboola_agent_cli/services/sync_service.py:1822-1836 — ds_client can leak if a push aborts on ENCRYPTION_FAILED

ds_client (a DataScienceClient, which implements __enter__/__exit__ per data_science_client.py:71-76) is built at line 1697 and closed manually at lines 1835-1836. The except Exception as exc: ... raise at lines 1822-1832 re-raises a KeboolaApiError(ENCRYPTION_FAILED) before the Phase-A loop reaches that close call, so ds_client.close() is skipped entirely on that path (the surrounding with client: at line 1671 still closes the storage client correctly — only ds_client is unguarded). CONTRIBUTING.md's "Resource management" convention calls for a context manager on anything with __enter__/__exit__; wrapping the Phase-A loop in with ds_client (or a contextlib.nullcontext() fallback when it is None) would close this. The trigger is narrow (a data-app CREATE with a recorded type, plus another change in the same push failing encryption), but kbagent serve is long-running, so a leaked client accumulates rather than dying with the process.

[NB-3] src/keboola_agent_cli/services/data_app_service.py:144-206 — no cleanup if update_config fails after create_app succeeds

The established two-step DS-create pattern in this same file, DataAppService.create (lines 483-634), wraps create_app + update_config in try / except Exception: ds_client.delete_app(app_id) ... finally: ds_client.close(), specifically so that "a failed data-app create does not leak an empty deployment shell" (module docstring, lines 1-13). create_synced_data_app runs the identical two-call sequence with no such guard: if update_config (line 198) raises after create_app (line 175) already succeeded, the orphan DS app and its bare Storage config are left in the target project, and sync push's per-change error handling (_record_push_error) reports only that the change failed, with nothing to indicate a stray app was created. Reusing the same cleanup pattern would close this gap.

[NB-4] Live push/clone path unverified against a real Data Science API

The PR description states the only live verification was a read-only pull against project 4214; sync push/sync clone creating a data app through create_synced_data_app is covered only by mocked (FakeDs/MagicMock) unit tests. I independently confirmed the /apps list field shape (componentId, configId, type) that the pull side depends on by running a read-only data-app list against the same project 4214 referenced in the PR description — it matches the assumptions in sync_service.py:588-601 exactly. I did not attempt a live sync push/sync clone create myself: that project carries 2126 configs (too large for a quick reproduction), and creating a real data app is outside what a read-only review should do unprompted. The create_app → update_config sequence and the parameters.id repoint therefore remain unverified against the real Data Science API.

Nits

• [NIT-1] src/keboola_agent_cli/services/data_app_service.py:175-181 — create_synced_data_app sends the full (still source-project-pointing) configuration as the initial create_app body, then corrects parameters.id and re-sends the whole body via update_config. The sibling pattern (DataAppService.create, lines 485-491) instead sends a minimal initial_config shell first and builds the correct full body only once, on the update_config call. Not a bug — the DS service treats config as opaque storage on create, and the correction lands in the same call chain — but the file's two DS-create call sites now follow different shapes for the same two-step contract.

Verification log

• git rev-parse --abbrev-ref HEAD → fix/cli-8-data-app-type; git rev-parse HEAD → 78bdc925daeebfc44527e52d8b80967fe28f33c2 — matches the given branch and PR head exactly.
• gh auth status → authenticated.
• gh -R keboola/cli pr view 752 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state → state OPEN, fix(sync): prefix matches the change type, +577/-11 across 7 files (4 source, 3 test), no plugins/ or docs/ files touched.
• Read CONTRIBUTING.md (Plugin synchronization map, File-size budgets, per-command checklist) and CLAUDE.md (## All CLI Commands, convention #17) in full; read plugins/kbagent/agents/keboola-expert.md in full (§1, §3 as required, plus the rest for the data-app gotcha precedents cited above).
• 3-layer greps (typer/click/formatter/console.print in services/; httpx/requests in the touched service files) on the diff and on the touched files directly → empty, no layer violation.
• Convention greps (magic numbers outside constants.py, raw error_code="..." literals, bare except:, print() in production code, token-in-log) on the diff → empty.
• uv run make check → FAILED at loc-check, exit 2: FAIL: services/data_app_service.py grew to 1679 code lines, past its grandfathered 1641. Every earlier gate passed clean: lint, format, ty (one pre-existing, unrelated unresolved-import warning in scripts/hatch_build.py, does not block per the project's own ty rules), SKILL.md freshness, version-sync, version-gate-check, command-sync-check ("OK: all 268 CLI commands are registered (OPERATION_REGISTRY) and documented (CLAUDE.md, context.py, commands-reference.md)" — confirms no new/removed/renamed command, so no OPERATION_REGISTRY/AGENT_CONTEXT gap here), endpoints-check, changelog-check, check-error-codes, check-sentinel-guards.
• uv run make test (the target loc-check blocked) → 1 failed, 6579 passed, 12 skipped in ~104s; the one failure is tests/test_check_file_size.py::TestRepoState::test_repo_passes_its_own_gate, the same file-size gate — confirming the PR's actual sync/data-app logic is otherwise clean and the file-size budget is the only red signal.
• uv run python scripts/check_file_size.py --report | grep data_app_service → 1679 2188 18% services: HARD >1500 against a grandfathered baseline of 1641 in scripts/file_size_baseline.json.
• Read sync_service.py, _sync_push_ops.py, data_app_service.py, sync/config_format.py, sync/diff_engine.py, data_science_client.py, and client/configs.py around every touched region: traced the configuration encrypt-then-branch ordering (secrets are encrypted before the DS-routed branch, so no plaintext leak), confirmed _IGNORED_KEYS = frozenset({"_keboola", "version"}) in diff_engine.py (backs the PR's hash-invisibility claim), confirmed update_config's is_disabled support, and located the established cleanup pattern in DataAppService.create.
• Ran a read-only kbagent --json data-app list --project <alias> against project 4214 (the project referenced in the PR description) using the repo's own dev CLI — live /apps records carry componentId/type/configId exactly as the new pull code assumes (21 apps returned, a mix of python-js/streamlit, matching the PR description's counts).
• Ran a read-only kbagent --json config list --project <alias> against the same project → 2126 configs, which is why a live sync pull/sync push reproduction of the full flow was not attempted (see NB-4).
• grep '"sync\.' permissions.py → sync.push/sync.pull/sync.clone already registered (write/read/write); no new commands are added by this PR, consistent with the clean command-sync-check above.

Open questions for the author

(none)

[soustruh]

Thanks for the review. I addressed all six points in e278263.

• B-1 (file-size gate): moved create_synced_data_app into a new services/_sync_data_app.py. data_app_service.py is back at its 1641 baseline and make loc-check passes.
• B-2 (gotchas.md): added an entry for the data-app-type behavior, with matching notes in keboola-expert.md §3 and the sync clone bullet in commands-reference.md, tagged (since vNEXT).
• NB-1 (is_disabled): the DS create path now passes it to update_config.
• NB-2 (ds_client leak): the client enters on the with line, so it closes on every exit from the push block, a mid-push ENCRYPTION_FAILED raise included.
• NB-3 (orphan cleanup): if update_config fails after create_app, the code deletes the DS record, so a failed create leaves no orphan app.
• NIT-1 (create body shape): the create call drops the stale parameters.id, so create_app never sees the source project's app id.

New unit tests cover NB-1, NB-3, and NIT-1. The FakeDs double closes on __exit__ like the real client, which covers NB-2. make check is green: 6582 tests, loc-check, and version-gate-check.

NB-4 (a live sync push / sync clone against a real Data Science API) is not done. It needs a real create in a target project, so it stays covered by the mocked unit tests plus the read-only live pull the description records. I will run a live create if a throwaway target is available.

[soustruh]

Update on NB-4 (a live sync push / sync clone against a real Data Science API): done, and it found two create-path bugs the mocked tests missed. Fixed in c26867a.

• POST /apps rejected the full Storage body with HTTP 422. It wants the create-shell shape (parameters.size / autoSuspendAfterSeconds / dataApp.slug + authorization), not runtime.backend.size. Fixed: build the shell, then update_config sends the full body.
• POST /apps also wants branchId=null on a production push, but the sync engine passed the numeric default branch id. Fixed: map the default branch to null for the DS create, and keep a dev branch id.

Live check: a sync push of a python-js app to project 4214 created a DS record with type=python-js. I then deleted the test app. The unit tests now assert the shell shape and branchId=null.

One known limit: a session-token target still cannot create a data app, because the DS API rejects session tokens. That is out of scope here, and session support for this operation is a separate ticket.

[keboola-pr-reviewer-bot]

Verdict: auto_approve (risk 2/5) · profile keboola-mcp-server

Well-tested, backward-compatible sync bug fix carrying a data app's runtime type through pull/push/clone.

Concerns:

• src/keboola_agent_cli/services/_sync_data_app.py: Bare except Exception around update_config; intentional orphan-cleanup, logged, non-blocking coaching note

[zajca]

Reviewed c26867a against base main @ 99a000b (true base→head diff, 9 files, +711/−13). Ran pytest tests/ -k "sync or data_app": 902 passed, 29 skipped (test_serve_* excluded — fastapi missing in the venv).

The direction is right and the DS-record/Storage-config split is documented well. Three things I'd like addressed before merge — the first one undoes the fix on the next push.

---

1. parameters.id back-pointer is never written back to the local file, so the next sync push overwrites the correct remote id with the stale source one

services/_sync_data_app.py:96-98 sets parameters.id to the newly created app id in the body that goes to the remote. But writeback_after_push (_sync_push_ops.py:466 → _sync_writeback.py:236-257 → apply_encrypted_to_local) only copies encrypted #-keys back into _config.yml. The local file keeps the source app's parameters.id.

So after sync clone the remote is correct and the local tree is not. Any later edit to that config goes through push_update, which sends local_config_to_api(local_data) including the stale parameters.id — silently reverting the back-pointer that data_app_service.py:465 calls "writeup §5: required". Phases C/D don't help: _sync_bindings.py:86 and :390 filter on keboola.variables / keboola.flow only.

The test suite already shows the asymmetry: tests/test_sync_data_app_type.py:256 asserts the new id in the PUT, while the local fixture keeps "99999" (:167).

Suggested fix: have create_synced_data_app return the new app id to the caller (or have push_create persist it), and write parameters.id into pristine_data before writeback_after_push. A test asserting the on-disk parameters.id after a create would lock this in.

2. A Data Science failure during sync pull silently deletes an already-recorded data_app_type from disk

services/sync_service.py:589-604 degrades to an empty map with only a logger.warning. sync_service.py:762-771 then rewrites _config.yml without data_app_type (the write is unconditional for an unmodified config) and re-stamps pull_hash.

One ordinary pull during a DS outage — or with a token lacking DS scope — therefore strips the type from every data-app config in the tree. Because config_hash ignores _keboola (sync/diff_engine.py:21) and pull's result envelope has no warnings channel (unlike push, sync_service.py:1949), neither sync diff nor --json shows anything. A later clone then deploys under the platform default — exactly the bug this PR fixes, reintroduced invisibly.

This is a different class from the neighbouring except Exception blocks for storage/jobs: those only skip auxiliary metadata files, this one removes a value from a tracked config file. Please either preserve the existing _keboola.data_app_type when the DS lookup did not run/succeed, or surface a warning on the pull envelope (ideally both).

3. Orphan app left when POST /apps returns id without configId — contradicts the docstring

services/_sync_data_app.py:88-94 raises before the try/except that calls delete_app. With an id present but configId missing, the DS record and its bare Storage config stay behind in the target, while the docstring (:56-58) promises "a failed sync create leaves no orphan app in the target". tests/test_sync_data_app_type.py:395-410 covers the raise but does not assert cleanup.

---

Verified as correct

• _keboola is in diff_engine._IGNORED_KEYS (:21), so the claim of zero sync diff noise on existing trees holds.
• The create shell matches DataAppService.create (data_app_service.py:420-427); runtime.backend.size / authorization survive the _configuration_extra round-trip.
• manifest.branches[0] as the default branch is the established convention repo-wide; mapping production to branchId=null matches data-app create.
• The new SyncService.__init__ signature is backwards compatible — all 20 call sites use kwargs.
• The componentId == keboola.data-apps filter does prevent sandbox/workspace records from landing on an unrelated config; the DS client is built only when the changeset creates a data app, and with client, ds_context closes both on a mid-push raise.
• Docs correctly use the vNEXT placeholder, with no version bump and no changelog.py entry.

One thing worth confirming (I couldn't verify without a live stack)

A data app's slug forms its hostname, so it is presumably unique per project. push_create for a fork-by-copy CREATE within the same project now sends POST /apps with the same slug as the source. If the API rejects that, it is a behaviour change from the previous create_config path — worth a live check.

Dismissing prior approval — a new commit was pushed and this review was for an earlier SHA. Run @keboola-pr-reviewer-bot review to get a fresh verdict.

[keboola-pr-reviewer-bot]

New commit on 3ea2e77 — dismissed 1 stale bot approval. Comment @keboola-pr-reviewer-bot review when you want a fresh review.

[soustruh]

Thanks, all three are fixed in 3ea2e77.

1. parameters.id back-pointer: create_synced_data_app sets the new app id on the body. push_create now writes it into the local file (pristine_data) before writeback, so a later push keeps the correct remote id. A new test checks the on-disk parameters.id after the push.

2. DS failure on pull: pull now tracks whether the Data Science lookup succeeded. On failure it keeps the existing local data_app_type instead of removing it. A new test runs a second pull with a failing DS and confirms the type stays on disk. I did not add a pull-envelope warning yet, because keeping the type already stops the data loss. I can add one as a follow-up if you want it.

3. Orphan on id-without-configId: create_synced_data_app now deletes the app before it raises, so no orphan stays. The existing raise test now also asserts delete_app.

Your open question (same-project fork-by-copy slug): I checked it live against project 4214. POST /apps accepts a duplicate slug. I created a second app with slug api-test, then deleted it. The hostname is disambiguated by app id, so there is no behavior change from the create_config path.

make check is green.

[zajca]

Actionable findings from the automated review.

[zajca]

Reviewed by Opus.

config_hash strips _keboola (sync/diff_engine.py:21 _IGNORED_KEYS), which is exactly what makes this key diff-invisible — but it also means api_cfg_hash computed with the new data_app_type equals the pull_config_hash a previous pull stored. The remote_unchanged guard at sync_service.py:865-882 then short-circuits and _write_config_file is never called, so the freshly-fetched type is discarded.

Failure scenario: a tree pulled by kbagent <= 0.93.1 holds keboola.data-apps/my-app/_config.yml with no _keboola.data_app_type. The user upgrades and runs sync pull --project prod as the docs instruct. The DS /apps list returns {configId: cfg-da, componentId: keboola.data-apps, type: python-js}, but the config body is unchanged since the last pull, so old_cfg_hash == api_cfg_hash, remote_unchanged is True and the file is not rewritten. sync clone then takes the plain create_config path and the cloned app deploys as streamlit — the exact CLI-8 bug, still present after the documented fix.

This is the population the PR targets. commands-reference.md and gotchas.md both tell users to "re-pull the source before you clone", and that remediation is a no-op. Only a config whose remote body also changed, or a brand-new config, picks the type up. --force does not help (it only relaxes the conflict guard) and neither does --theirs (it still returns remote_unchanged=True when local files byte-match the pull state).

It also makes test_pull_preserves_type_when_ds_lookup_fails (tests/test_sync_data_app_type.py:231) vacuous: the second pull writes nothing at all, so the test passes even with the ds_types_available fallback removed.

Fix direction: fold the presence/absence of data_app_type into the write decision for keboola.data-apps — e.g. clear remote_unchanged when the on-disk _keboola.data_app_type differs from da_type — rather than relying on the body hash, which by construction cannot see it.

Reproduced on this head: pull a data-app config with the DS list returning nothing (no type on disk), then re-pull with the DS list reporting python-js and the remote body unchanged — _keboola.data_app_type is still absent.

[zajca]

Reviewed by Opus.

The predicate at lines 1686-1691 keys only on change_type == "added" and component_id == keboola.data-apps; it does not check whether the local _config.yml actually carries _keboola.data_app_type. push_create (_sync_push_ops.py:435-436) does check, and falls back to client.create_config when the type is absent — so for a type-less data app the DS client is constructed and then never used.

That construction is neither free nor guarded. DataScienceClient sets SESSION_AUTH_FEATURE (data_science_client.py:56), and BaseHttpClient.__init__ calls require_static_token (http_base.py:131-132), which raises SessionAuthUnsupportedError for a session token. KeboolaClient has no such sentinel, so sync push is otherwise fully supported under session auth. The call sits outside with client and outside any try, so the exception escapes push() and aborts the whole changeset — including the non-data-app creates, updates and deletes that would have succeeded.

Failure scenario: a user authenticated with a session token (kbagent auth login) has a sync tree containing one untracked keboola.data-apps config with no recorded data_app_type — authored by hand, or pulled before this PR — plus several changed transformations. sync push --project prod reaches line 1692, DataScienceClient.__init__ raises SessionAuthUnsupportedError via require_static_token, and the push aborts before with client is entered. Nothing is created or updated, where on the previous release the whole push (including that data-app config, via create_config) succeeded.

The pull side gets this right: its DS client construction at line 593 is inside try/except Exception and degrades to "no type". Push has no equivalent.

Two independent fixes, both cheap: (a) include data_app_type presence in the predicate so the client is only built when it will be used, and (b) construct it defensively so a DS failure degrades the data-app create to create_config with a warnings[] entry instead of failing the entire push.

Reproduced on this head: with a _ds_client_factory that raises SessionAuthUnsupportedError (what the real factory does under session auth) and an untracked data-app config carrying no data_app_type, SyncService.push(...) propagates the error and creates nothing.

[zajca]

Reviewed by Sonnet.

In push(), whenever the changeset contains an added keboola.data-apps config, ds_client = self._ds_client_factory(project.stack_url, project.token) is called unconditionally and outside any try/except, before the with client, ds_context: block even starts. _default_ds_client_factory builds a DataScienceClient, which is static-token-only (SESSION_AUTH_FEATURE guard in http_base.py) and raises SessionAuthUnsupportedError for any kbc-session:// sentinel token. The Storage client used everywhere else in push() is bearer-aware (BaseService's default make_client_factory), so sync push for a session-registered project worked fine before this PR, including creating new data-apps configs (via the plain create_config path). Now the same push blows up before a single change is applied -- not just the data-app portion, the entire push (unrelated config creates/updates/deletes included) is blocked. Contrast with the equivalent lookup added to pull() a few hundred lines earlier (around line 593), which wraps the same _ds_client_factory(...) call in try/except Exception and degrades gracefully (logs a warning, keeps whatever type was already on disk). push() has no analogous fallback: it should either wrap the factory call and fall back to the plain create_config path (as push already does per-config when no data_app_type is recorded), or skip DS-client creation when the token is a session sentinel. Failure scenario: a project registered via kbagent auth login (session/OAuth token, stored as kbc-session://{project_id}) runs sync push on a tree that includes one new keboola.data-apps config (with or without a recorded data_app_type) alongside unrelated changes to other components. Previously this succeeded. After this change, self._ds_client_factory(...) raises SessionAuthUnsupportedError immediately, and the whole push aborts with zero changes applied -- including the unrelated, otherwise-successful changes.

[zajca]

Reviewed by Sonnet.

The fallback comment states: 'If that lookup failed, keep the type already on disk instead of stripping it,' but the code only takes the fallback branch when ds_types_available is False, i.e. when the whole ds_client.list_apps() call raised. If the call succeeds but simply does not include this particular config_id (or returns a falsy type for it -- both are filtered out by the map's comprehension, if ... and app.get('type')), da_type = data_app_types.get(config_id) evaluates to None, and api_config_to_local(...) then omits data_app_type from the _keboola footer entirely, silently erasing whatever was previously recorded on disk (config_hash ignores _keboola, so this produces no diff signal). A per-app gap in an otherwise-successful list (e.g. the DS deployment record was deleted independently of the Storage config, or the app is in a transient state where type is briefly absent) is treated as 'this app has no type' rather than 'the type for this one app is unknown,' even though the whole point of this PR is to never let a data app silently lose its recorded type. Failure scenario: a keboola.data-apps config previously had its data_app_type recorded on disk (e.g. python-js). Its Data Science /apps record is later deleted or temporarily omitted from GET /apps while the Storage config remains. A subsequent sync pull succeeds (ds_types_available=True) but the type map has no entry for that config, so the pulled _config.yml silently drops _keboola.data_app_type. sync diff shows nothing (the key is hash-invisible). A later sync clone of that tree falls back to the plain create_config path and the cloned app deploys under the platform default type -- the exact regression this PR was written to close, reintroduced in a narrower edge case.

[soustruh]

Finding A (re-pull discards the type) is unrelated to session auth. Fixed here in b7272a0. For keboola.data-apps the write decision now compares the resolved type against the on-disk type. It forces a rewrite when they differ, because config_hash ignores _keboola. A new test guards it.

The session-token findings (O002 / S001) are correct, and the regression comes from this PR. It is resolved by #759, which makes the ds_client factory session-aware. That factory resolves a kbc-session:// token to a real token before it builds DataScienceClient. So the eager build in push() no longer raises. We will merge and release #759 in the same release as this PR, so the two never ship separately.

[zajca]

Actionable findings from the automated review.

[zajca]

Reviewed by Sonnet.

da_type = data_app_types.get(config_id) if ds_types_available else on_disk_da_type only falls back to the on-disk value when the whole DS /apps call raised an exception (ds_types_available is False). If the call succeeds but its returned list simply does not contain an entry for this config_id (e.g. an incomplete/paginated response, or a Storage keboola.data-apps config whose DS deployment record is missing), da_type resolves to None while on_disk_da_type may still hold a previously recorded value such as python-js.

The new forced-rewrite guard added by this commit (lines ~891-896: if remote_unchanged and component_id == DATA_APP_COMPONENT_ID and da_type != on_disk_da_type: remote_unchanged = False) then treats that mismatch as a real change and rewrites _config.yml with the type stripped -- with no warning logged, unlike the explicit-failure path. Before this commit the hash-skip left the file untouched in this exact case, so the previously recorded type survived by accident; after this commit it is actively erased. A later sync push/sync clone of that config would then fall back to the platform-default runtime type, reproducing the very bug (CLI-8) this PR set out to fix, just via a narrower trigger this commit newly introduces.

Failure scenario: a config was pulled once and recorded data_app_type: python-js on disk. On a later sync pull, the DS /apps call succeeds (ds_types_available=True) but its response omits this config's id (transient DS-side omission, or its deployment record was independently removed while the Storage config remains). The Storage body is otherwise unchanged, so remote_unchanged would normally skip the write -- but the guard sees da_type=None != on_disk_da_type='python-js', forces a rewrite, and the local _config.yml loses data_app_type with no log message. A subsequent sync push/sync clone then deploys under the platform default instead of python-js.

Suggested fix: treat "call succeeded but this configId is absent from the list" the same as "call failed" -- fall back to on_disk_da_type unless the entry is explicitly present with a different type, or at minimum log a warning when a previously-recorded type is about to be dropped.

[zajca]

Actionable findings from the automated review.

[zajca]

Reviewed by Opus.

self._ds_client_factory(...) is called outside any try/except and before the with client, ds_context: block. _default_ds_client_factory builds a DataScienceClient, whose base class runs require_static_token(token, feature=SESSION_AUTH_FEATURE) in __init__ (http_base.py:131). For a project registered via kbagent auth login --register-projects, ProjectConfig.token is the kbc-session://{project_id} sentinel, so construction raises SessionAuthUnsupportedError — verified locally:

>>> _default_ds_client_factory('https://connection.keboola.com', 'kbc-session://1234')
SessionAuthUnsupportedError: The Data Science Service (data apps) does not support browser-login (session) projects yet...

sync push otherwise fully supports session projects — make_client_factory (services/base.py:173) detects the sentinel and builds the Storage client with BearerAuth. So this is a regression: before this PR the same push succeeded through create_config.

Two aggravating details:

1. The exception escapes the per-change except Exception handler in the push loop, so no change in the changeset is applied — including configs of unrelated components.
2. The trigger condition only checks change_type == "added" and component_id == DATA_APP_COMPONENT_ID; it does not check whether any of those configs actually carries _keboola.data_app_type. A tree pulled before this fix (no type recorded) would take the unchanged create_config fallback in push_create, yet still pays the hard failure here.

The pull side already handles this correctly — the equivalent factory call at sync_service.py:592 sits inside a try/except that degrades to no type.

Suggested fix: tighten the guard to require a recorded type, and degrade instead of aborting, e.g.

needs_ds = any(
    c.get("change_type") == "added" and c.get("component_id") == DATA_APP_COMPONENT_ID
    for c in changes
    if not bool(c.get("is_row"))
)
if needs_ds:
    try:
        ds_client = self._ds_client_factory(project.stack_url, project.token)
    except Exception:
        logger.warning("Data Science client unavailable; data-app type will not travel", exc_info=True)

and surface the degradation in warnings so the operator learns the type was dropped rather than getting a silently wrong clone.

[zajca]

Reviewed by Opus.

When the /apps lookup fails (outage, session-auth project per C001, a token lacking Data Science scope), data_app_types stays empty and the only signal is a logger.warning. The pull result dict (sync_service.py:1132-1143) carries no warnings key, so sync pull --json and the CLI output are indistinguishable from a fully successful pull.

The consequence is specific to this feature rather than generic: config_hash ignores _keboola, so a config that never received data_app_type shows no diff and looks perfectly in sync. The operator then runs sync clone and the app deploys under the platform default streamlit — exactly the CLI-8 bug this PR fixes — with nothing anywhere in the tree or the command output indicating why.

This differs from the neighbouring log-only degradations for buckets/jobs (sync_service.py:545, 571): those omit enrichment data whose absence is visible in the tree, whereas this one silently reinstates the defect under repair.

Suggested fix: accumulate a record (e.g. {"action": "data_app_type_unavailable", "reason": str(exc)}) and return it on the envelope — either appended to details or under a new warnings key — so the failure is visible to both a human and a downstream tool before a clone is attempted.

[zajca]

Reviewed by Sonnet.

push() builds ds_client = self._ds_client_factory(project.stack_url, project.token) directly, outside any try/except, whenever the changeset contains any new keboola.data-apps config — even one whose local _config.yml carries no data_app_type and would use the plain, session-safe create_config path in push_create. _default_ds_client_factory builds a plain DataScienceClient(stack_url=..., token=...). DataScienceClient declares SESSION_AUTH_FEATURE = "The Data Science Service (data apps)", so its constructor calls require_static_token, which raises SessionAuthUnsupportedError (a ConfigError) whenever token is a kbc-session://{project_id} sentinel (see auth/sentinel.py). Session-registered projects (kbagent auth login --register-projects) are an intentionally supported, documented flow for Storage/Manage operations — SESSION_UNSUPPORTED_FEATURES in services/_auth_registration.py lists kbagent data-app (Data Science Service) as unsupported but does not list sync push/sync clone, and SyncService's own Storage client (built via make_client_factory, services/base.py:156) is sentinel-aware and transparently resolves the session token into a bearer-auth KeboolaClient that works fine. So before this PR, sync push/sync clone worked for session-registered projects, including creating a brand-new keboola.data-apps config (which only used create_config). After this PR, ANY push (or clone, which reuses push() per the PR description) that creates a new keboola.data-apps config on a session-registered project now aborts entirely with SessionAuthUnsupportedError, regardless of whether the local config even has a recorded data_app_type (the any(...) check at line ~1699 only looks at change_type == "added" + component_id == DATA_APP_COMPONENT_ID, never at whether _keboola.data_app_type is set). This is a functional regression for a previously-working, still-intended-to-work code path, and it is especially damaging for sync clone, which creates every config fresh — a clone of any tree containing a data-apps config now always fails for a session-registered target project. In addition, because the raise happens before with client, ds_context: is entered, the already-constructed Storage client (line ~1673, client = self._client_factory(...)) never has its with/close() run, leaking its underlying httpx.Client. Contrast with the equivalent DS-client construction added to pull() (same file, line 592), which IS wrapped in try/except Exception and degrades gracefully (logs a warning, proceeds with no type) — exactly the pattern this push-side construction should follow, additionally gated on whether any of the new data-apps configs actually carry a _keboola.data_app_type before paying the construction cost at all. Failure scenario: a user runs kbagent auth login --register-projects (session auth, no static Storage token stored), then sync clone --source <tree-with-a-data-app> --target <alias> --target-dir <dir> (or a plain sync push that creates a new keboola.data-apps config, with or without a recorded data_app_type). push() unconditionally calls self._ds_client_factory(project.stack_url, project.token), which raises SessionAuthUnsupportedError before the Storage client is ever entered via with. The whole push/clone aborts with exit code 5, even though every other config in the changeset (and the data-apps config itself, had it used the pre-PR create_config path) would have pushed successfully under session auth. The Storage client's httpx.Client is also never closed.

S001 is fixed in 1a0cf31. The earlier findings were addressed in the prior commits, and the session-token finding is handled by #759 (released together).

[keboola-pr-reviewer-bot]

Verdict: needs_human (risk 3/5) · profile keboola-mcp-server

Well-scoped, well-tested data-app-type fix that adds a new DS-API write to the load-bearing sync push/clone create path — worth a quick human confirm.

Suggested reviewers: keboola/keboola-cli-maintainers

[zajca]

No actionable findings were found by the automated review.