fix(sync): keep the config folder on pull and clone (CLI-9)

[soustruh]

What changed

sync pull now reads each config's UI folder (KBC.configuration.folderName) and stores it in the manifest, so sync clone recreates the folder in the target project.

The bug

A config's folder is config metadata under KBC.configuration.folderName. That key is not part of the list_components_with_configs response. It comes from a separate branch-only search endpoint that sync pull never queried. Pull dropped the folder, and every config landed in the root after a pull or clone. The old Go CLI (keboola-as-code) kept the folder, so this was a regression.

The fix

• New helper _fetch_config_folders(client, branch_id) fetches the folder map. It reuses the same branch-only idiom as ConfigService: production resolves the default branch first, and a lookup failure degrades to an empty map so it never aborts the pull.
• pull() writes KBC.configuration.folderName into each manifest entry.
• No push-side change was needed. The create path already sends the manifest's KBC.* metadata through propagate_kbc_metadata, and writeback_create_config_in_manifest keeps that metadata on the cloned placeholder entry.

Scope

This covers pull capture and clone (the create path). Changing the folder on an existing config through a plain sync push still does not propagate, because propagate_kbc_metadata runs only on create. That is a separate, pre-existing limitation.

Testing

• A new test reproduces the loss and the fix: after pull, a foldered config carries KBC.configuration.folderName in the manifest, and a config with no folder gets no key.
• Helper unit tests cover the resolved branch, the default-branch fallback for production, an empty map when no branch resolves, degradation on an API error, and a non-dict response.
• The full non-e2e test suite passes. ruff check, ruff format --check, and ty check are clean.

Linear: CLI-9

[linear-code]

CLI-9

[soustruh]

Review of #753 — fix(sync): keep the config folder on pull and clone (CLI-9)

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 closes a real regression against the reference Go CLI: sync pull never fetched a config's UI folder (KBC.configuration.folderName), so every pulled or cloned config landed in the tree root. The fix adds one static helper (_fetch_config_folders) that reuses the exact branch-resolution idiom already established in config_service.py's list_config, injects the folder into cfg_metadata for both the locally-modified and normal code paths, and degrades to an empty map on any lookup failure so a pull never aborts over it. Source-level tracing confirms every claim in the PR description holds up: the branch fallback matches config_service.py, the push-side propagation via propagate_kbc_metadata / writeback_create_config_in_manifest already preserves the key (and is already tested), and a folder removed remotely is correctly dropped on re-pull because cfg_metadata is rebuilt fresh every time rather than merged with old metadata. Seven new focused unit tests cover the helper and the pull integration; the full suite and make check are clean. Verdict is REQUEST CHANGES for one reason only: this repo already documents the mirror-image push-side fix for this exact metadata key in exhaustive, version-tagged detail in gotchas.md / sync-workflow.md — the pull-side half of the same story has no equivalent entry.

Verdict

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

Blocking findings

[B-1] plugins/kbagent/skills/kbagent/references/gotchas.md — no entry for the pull-side folder-loss fix

sync pull/sync clone silently dropped every config's UI folder before this fix (every config landed in the tree root). This repo already documents the mirror-image push-side fix for the same metadata key in exhaustive, version-tagged detail (gotchas.md:697-704, sync-workflow.md:194-198, tagged v0.47.0) — the diff adds no matching bullet for the pull-side gap it closes. On a pre-fix kbagent install, sync clone (keboola-expert.md matrix row: "Provision a new project from a golden reference") still silently produces a folder-less clone, and nothing warns an agent to check the version or verify folders came across. Per CONTRIBUTING.md's Plugin synchronization map, a missing gotchas.md tag for a behavior change is BLOCKING. Add a (since vNEXT)-tagged bullet next to the existing "KBC.configuration.* metadata propagates on CREATE" note.

Non-blocking findings

[NB-1] src/keboola_agent_cli/services/sync_service.py:2054 / services/config_service.py:164 — duplicated folder-fetch-with-branch-fallback logic

The new _fetch_config_folders static method reimplements the exact try/except block already in config_service.py's list_config — same fallback chain (branch_id or find_default_branch_id(...)), same isinstance(result, dict) guard, and the identical "config-folder metadata lookup failed" debug message. base.py's own docstring already states it is "the one home for the isDefault scan shared by the config, sync, workspace and merge-request services" — this is a good candidate to extract there next to find_default_branch_id, so a future change to the fallback logic doesn't need to land twice.

[NB-2] src/keboola_agent_cli/client/configs.py:309 — unverified pagination on the folder-search endpoint at pull scale

list_config_folder_metadata calls GET .../search/component-configurations with no limit/offset handling. The method pre-dates this PR (used by config list), but sync pull is a new, larger-scale caller — a whole-project fan-out rather than a single interactive listing. If the endpoint caps results server-side, a project with more folder-tagged configs than one page would have folder_map.get(lookup_key) return None for the overflow configs — indistinguishable from "no folder assigned" (sync_service.py:988), so pull would silently drop folders for some configs rather than none. Could not verify against a live project (no E2E_API_TOKEN/E2E_URL or /tmp/kbagent-e2e in this environment). Recommend confirming against a project with 100+ foldered configs, or checking the Storage API docs for this route's default page size.

Nits

• [NIT-1] tests/test_sync_storage_jobs.py:1469 — the TestPullConfigFolder docstring points to "TestPropagateKbcMetadata in test_sync_service.py" for the push-side tests; that class doesn't exist. The actual coverage is TestFreshCreateWriteback (test_sync_service.py:3013, test_propagate_kbc_metadata_* methods at lines 3139-3211).

Verification log

• Working tree verified: /home/soustruh/cc-dev/cli-9, branch fix/cli-9-config-folder, HEAD=64293c87e234099aac408bf9c9f94f447262067c, git status clean — matches the PR head.
• gh auth status → authenticated as soustruh ✓
• Read CONTRIBUTING.md (Checklist for new commands, Plugin synchronization map, Releasing a new version), CLAUDE.md (convention #17, ## All CLI Commands), plugins/kbagent/agents/keboola-expert.md §1/§2/§3 before touching the diff.
• gh pr view 753 -R keboola/cli --json ... → OPEN, fix(sync): prefix matches a genuine bug fix, +155/-1, 2 files (services/sync_service.py, tests/test_sync_storage_jobs.py) ✓
• gh pr diff 753 -R keboola/cli → 196 lines; git diff origin/main...HEAD --stat matches the PR's reported file list exactly (no drift from a stale local main) ✓
• git diff origin/main...HEAD -- src/keboola_agent_cli/cli.py 'src/keboola_agent_cli/commands/**/*.py' → empty: no command/flag added or changed, so commands/context.py, CLAUDE.md "All CLI Commands", permissions.py OPERATION_REGISTRY, commands-reference.md, and the keboola-expert.md Tool Selection Matrix are correctly untouched (their triggers require a command/flag change) ✓
• Layer-violation greps (typer/click/formatter in services, httpx/requests in commands) → empty ✓
• Convention greps (magic numbers, raw error_code= strings, bare except:, print(), token literals) → empty; the only "token" hits are the pre-existing lambda url, token: pull_client factory pattern, not a leak ✓
• make check → exit 0, 6576 passed, 12 skipped, 5 warnings in 83.12s (re-run with full log captured, EXIT_CODE=0 confirmed; stage markers for ruff check, ruff format --check, ty check, check_version_gates.py, check_command_sync.py, generate_changelog.py --check, check_error_codes.py, check_sentinel_guards.py, check_file_size.py, and pytest all present in the log) ✓
• make loc-check → sync_service.py not listed among WARN/FAIL files; scripts/check_file_size.py reports it at 1603 code lines vs. the grandfathered baseline of 1655 — the file shrank overall despite this PR's +30 lines, so the hard-ceiling ratchet is respected ✓
• uv run pytest tests/test_sync_storage_jobs.py::TestPullConfigFolder tests/test_sync_storage_jobs.py::TestFetchConfigFolders -v → all 7 new tests pass in isolation ✓
• uv run pytest tests/test_sync_storage_jobs.py tests/test_sync_service.py -q → 172 passed, confirming no regression among the broader pull/push test modules from the new list_config_folder_metadata/list_dev_branches calls ✓
• Read sync_service.py pull() end-to-end (lines 462-1010, 2054-2140): confirmed branch_id passed to _fetch_config_folders is the fully-resolved value from _resolve_branch_id (production commonly resolves to the manifest's registered branch id, not None, once sync init has run); confirmed the folder injection at lines 988-990 sits after the locally_modified/else branch, so it applies uniformly to both; confirmed cfg_metadata is a fresh dict per pull (not merged from existing_metadata), so a folder removed on the remote is correctly absent after re-pull ✓
• Compared sync_service.py:2054-2072 (_fetch_config_folders) against config_service.py:164-176 (list_config's inline folder-fetch) line by line — same find_default_branch_id fallback, same endpoint, same degrade-on-error design ✓
• Read _sync_writeback.py (propagate_kbc_metadata, writeback_create_config_in_manifest) and _sync_clone.py (clone_project) to verify the PR description's push/clone-side claims independently rather than trust them — confirmed accurate; writeback_create_config_in_manifest's own docstring already names KBC.configuration.folderName as an example of preserved metadata ✓
• Checked sync.manifest.ManifestConfiguration.metadata → dict[str, Any] with model_config = ConfigDict(extra="allow") — adding a new dict key has no backward/forward-compat schema risk for older/newer kbagent reading the same manifest ✓
• Checked permissions.py OPERATION_REGISTRY → sync.pull already registered as "read", pre-existing and untouched (no new command) ✓
• Checked plugins/kbagent/skills/kbagent/references/gotchas.md, sync-workflow.md, commands-reference.md, commands/context.py for any existing mention of pull-side folder handling → none found, confirming [B-1] is a genuine gap and not already covered elsewhere ✓
• grep -rn "KBC.configuration.folderName" across src/ → consistent string everywhere (client, service, command, changelog, context.py) — no typo risk ✓
• Could not reproduce live against a real Keboola project: no E2E_API_TOKEN/E2E_URL set, and /tmp/kbagent-e2e does not exist in this environment. Relied on source-level tracing plus the green full test suite instead.

Open questions for the author

(none)

[soustruh]

All four findings handled.

Addressed in 3c5801b:

• B-1 (blocking): added a (since vNEXT) gotcha for the pull side in gotchas.md, next to the mirror-image push-side note. The tag sits on the first body line, not the heading, so check_version_gates.py stays green.
• NIT-1: fixed the TestPullConfigFolder docstring. It now points at the test_propagate_kbc_metadata_* tests in test_sync_service.py.

Deferred to keep this PR surgical, tracked as follow-ups:

• NB-1 (duplicated folder-fetch logic, extract one helper into services/base.py): CLI-10.
• NB-2 (folder-search paging in list_config_folder_metadata): CLI-11. I could not confirm the endpoint's page size from the docs (pages moved, apiary returned HTTP 502), so the ticket verifies it against a real project before any paging change.

make check stays green.

[keboola-pr-reviewer-bot]

Verdict: auto_approve (risk 2/5) · profile _default

Backward-compatible, well-tested fix that captures config UI folders on sync pull/clone; safe to auto-approve.

[zajca]

Reviewed at 3c5801b against base main @ 99a000b (merge-base confirmed identical, so this is the true base→head diff).

The pull-side capture is correct and well tested. Two findings below; the first one means the headline behaviour in the PR title/description does not actually hold.

---

1. (blocking) sync clone does not recreate the folder — the create path never sees the KBC.* metadata

services/_sync_writeback.py:146-156 matches the placeholder entry on (branch_id, component_id, path). sync/clone.py:83-86 (repoint_manifest_project) re-points only manifest.branches[0].id onto the target's default branch (CLI-5 / #744); it leaves branch_id on every copied ManifestConfiguration pointing at the source project's production branch id.

On push into the target, _resolve_branch_id returns the target's default branch id (sync_service.py:2137-2140), so the match fails, a second entry is appended carrying only pull_hash/pull_config_hash, and propagate_kbc_metadata finds no KBC.* keys to POST.

Reproduced by driving the repo's own functions (no mocks of the logic under test):

source entry branch_id: 12345
after repoint -> branches[0].id: 555 | entry.branch_id: 12345
matched existing entry: False
metadata handed to propagate_kbc_metadata: {'pull_hash': 'fh', 'pull_config_hash': 'ch'}
set_config_metadata called: False
manifest entries: 2        <- plus a stale duplicate entry

This is production-realistic: sync init writes the real default branch id (sync_service.py:332-338) and pull stores branchId=branch_id or 0 with that real id — as this PR's own test asserts (assert_called_once_with(branch_id=12345)). Two different projects never share a production branch id, and passing --branch mismatches as well.

Why the suite stays green: the clone fixtures in tests/test_sync_clone.py:37-43 build manifests with branch id = 0, which hides the mismatch. There is no test covering pull → clone → propagated folder end to end.

Extra concern: plugins/kbagent/skills/kbagent/references/gotchas.md now states "a clone recreates the folder in the target with no push-side change". A documented guarantee the code does not honour is worse than silence — it tells users (and the agent reading gotchas.md) not to bother verifying.

Suggested direction: either re-point cfg.branch_id alongside manifest.branches[0].id in repoint_manifest_project (which also removes the duplicate-entry symptom), or resolve the writeback entry without the branch-id predicate on a fresh clone. Whichever way, please add a test that goes pull → clone → assert set_config_metadata receives KBC.configuration.folderName, and keep the gotchas.md wording in step with whatever ships.

2. A degraded folder lookup silently deletes folder names already in the manifest

services/sync_service.py:2054-2072 (helper), :988-990 (write), :1059 (manifest.configurations = new_configurations)

_fetch_config_folders degrades to {} on any failure — API error, unresolvable branch, non-dict body — and only logs at debug. Pull rebuilds cfg_metadata from scratch and then replaces manifest.configurations wholesale, and nothing carries the previous KBC.configuration.folderName over. existing_metadata (:579-581) is right there and is already consulted for config_hash_version (:958), just not for the folder.

Net effect: one transient failure of search/component-configurations (or a stack/token where that endpoint is unavailable) during sync pull strips the folder from every manifest entry, silently. The pull output shows nothing, so a user cannot tell "no folders configured" from "the lookup failed" — which quietly reintroduces exactly the regression this PR fixes.

pull() has no warnings[] channel like push, but pull_details is available; even a single detail entry noting the degraded lookup would make it diagnosable. Preserving the prior value when the map came back empty because of a failure (as opposed to a successful empty result) would be better still.

---

Verified as fine

• lookup_key = f"{component_id}/{config_id}" (sync_service.py:656, config_id always str) matches the key format built in client/configs.py:309-341.
• The helper faithfully mirrors the established idiom in config_service.py:164-177, default-branch fallback included.
• if folder_name: correctly omits the key for a folder-less config, and the test asserts it.
• No version bump and no changelog.py entry — correct for a feature PR; (since vNEXT) is the right placeholder and is not in an ATX heading.
• Locally green: 7 new tests, test_sync_storage_jobs.py + test_sync_service.py (172 passed), ruff check, ruff format --check, make loc-check.