Repository navigation
Conversation
PR #759 (session-token support) replaced data_app_service.py's module-level _default_ds_client_factory with make_session_aware_client_factory(config_store, DataScienceClient), but left sync_service.py importing the removed name. Every command reachable through SyncService (pull/push/diff/status, and any command that imports keboola_agent_cli.cli or the server app at all) failed at import time with ImportError: cannot import name '_default_ds_client_factory' -- this broke CI on main across all three jobs (check, test, Windows wheel build) with 124-127 collection errors plus several failed tests wherever the import succeeded via a different path. Wire SyncService's DS client factory default through the same session-aware factory DataAppService already uses, so a browser-login session project's Storage/DS calls during pull/push work the same way CLI-13 intended everywhere else. The factory import is local to __init__ (not hoisted to the module-level .base import) to keep the file within its grandfathered file-size ceiling.
There was a problem hiding this comment.
🟡 Changes recommended
A moderate unresolved issue remains: the default session-aware factory lacks a SyncService-level regression test.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Restores SyncService’s session-aware Data Science client factory after CLI-13, fixing import-time failures.
Changes:
- Replaces the removed factory dependency.
- Uses the shared session-aware helper via a local import.
File summaries
| File | Summary |
|---|---|
src/keboola_agent_cli/services/sync_service.py |
Restores DS factory wiring with browser-session support; add a SyncService-level regression test for the default factory. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+269
to
+271
| self._ds_client_factory = ds_client_factory or make_session_aware_client_factory( | ||
| config_store, DataScienceClient | ||
| ) |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
main's own tip being CI-red (check,test (3.12), and the Windows wheel-build job all failing) since PR feat: accept browser-login (session) tokens for the commands whose backend supports them (CLI-13) #759 (CLI-13, session-token support) merged.data_app_service.py's module-level_default_ds_client_factorywithmake_session_aware_client_factory(config_store, DataScienceClient), but leftsync_service.pyimporting the now-removed name. Any code path that importskeboola_agent_cli.cli(almost the entire CLI) or the FastAPI server app failed at import time withImportError: cannot import name '_default_ds_client_factory'.SyncServicenow builds its DS client factory default through the samemake_session_aware_client_factoryhelperDataAppServicealready uses, so a browser-login session project's Storage/DS calls duringsync pull/pushwork the same way CLI-13 intended everywhere else. The helper is imported locally inside__init__(not hoisted into the top-level.baseimport) specifically to keepservices/sync_service.pywithin its grandfathered file-size ceiling —scripts/file_size_baseline.jsoncaps it at 1655 code lines,mainwas already at 1654, and a straightforward top-level-import fix would have pushed it to 1661 and failed the file's owntests/test_check_file_size.pygate. This version lands at 1653.wingetrelease-job fix) show red CI after rebasing ontomain— that PR's diff only touches a GitHub Actions workflow file and was never the cause.Test plan
uv run ruff check src/ tests/ scripts/,uv run ruff format . --check,uv run ty check— all clean.uv run python scripts/check_file_size.py—OK: 240 modules within budget,services/sync_service.pyat 1653/1655.uv run python scripts/sync_version.py,scripts/check_command_sync.py,scripts/check_version_gates.py,scripts/gen_endpoint_reference.py— no drift.uv run pytest tests/ -q— full local suite: 6787 passed, 184 skipped, 0 failed (previously 62 failed + 124-127 collection errors onmain).Related
fix(release): re-enable the winget release job), whose CI was red only because of this bug onmain.