feat(lib): typed SDK return models + py.typed marker (#428) - #429
Conversation
Ship a PEP 561 `py.typed` marker and a set of typed pydantic return models for the in-process SDK facade, so downstream consumers (e.g. the FIIA Scaffold Kit) type against a versioned contract instead of undocumented `dict[str, Any]` shapes -- a contract change then surfaces at type-check time, not at runtime against a customer build. - result_models.py: JobResult, QueryResult, UploadTableResult, SyncPushResult, ConfigDetailResult. All set `extra="allow"` so backend field drift never raises, and `populate_by_name=True` + AliasChoices so `Model.model_validate(service_dict)` works on raw API or snake_case keys. - lib.py: typed facade wrappers returning the models -- `Client.run_job` (-> JobResult), `query_result` (-> QueryResult), `config_detail` (-> ConfigDetailResult), `upload_table` (-> UploadTableResult). `query()` refactored onto a shared `_run_query` (behavior unchanged, still returns list[dict]). - py.typed forced into the wheel via hatch force-include. - Export the models from the package root; document in library-workflow.md. Tests: contract tests for every model (real camelCase keys, extra-field survival, convenience props) + facade-method tests. make check green.
There was a problem hiding this comment.
🚩 New pydantic models placed in result_models.py rather than models.py
CONTRIBUTING.md states: "Pydantic 2.x (BaseModel) for all data models -- defined in models.py". The new SDK return models (JobResult, QueryResult, etc.) are pydantic BaseModel subclasses but live in a new result_models.py. This could be seen as a convention violation, but there's a reasonable justification: the existing models.py contains internal CLI data models (like TokenVerifyResponse) shared across the 3-layer architecture, while these new models are SDK-specific return contracts for the library facade. The existing codebase also has InlineQueryResult (dataclass in client.py) and FileEntry (dataclass in lib.py) outside models.py, establishing precedent for domain-specific models living near their consumers. Consider documenting this split or updating the convention.
Was this helpful? React with 👍 or 👎 to provide feedback.
| effective_branch = branch_id if branch_id is not None else self._resolved_branch_id | ||
| detail = dict( | ||
| self._client.get_config_detail(component_id, config_id, branch_id=effective_branch) | ||
| ) | ||
| detail.setdefault("component_id", component_id) | ||
| if effective_branch is not None: | ||
| detail.setdefault("branch_id", effective_branch) |
There was a problem hiding this comment.
🟡 Lazy default-branch cache in _resolved_branch_id leaks into config_detail, run_job, and upload_table
The field self._resolved_branch_id serves two incompatible purposes: (1) storing the user's explicit branch_id from the constructor, and (2) caching the lazily-resolved default branch inside _effective_branch_id() (called by query()/query_result()). The new methods at lines 382, 413, 448 all read self._resolved_branch_id directly via effective_branch = branch_id if branch_id is not None else self._resolved_branch_id. If a user creates Client(url=..., token=...) (no branch), calls query() first (which resolves and caches the default branch, e.g. 123, at lib.py:240), then calls config_detail("comp", "cfg"), effective_branch becomes 123 instead of None. This causes config_detail to inject branch_id=123 into the result (line 418-419), making ConfigDetailResult.branch_id non-None even though the user is targeting production. The docstring at result_models.py:227 states "None = production", so this is a contract violation: the same config_detail() call returns different branch_id metadata depending on whether an unrelated query() was called earlier.
Prompt for agents
The root cause is that _resolved_branch_id is a single field serving two roles: the user-supplied explicit branch (from the constructor) and a cached default-branch ID (set as a side effect of _effective_branch_id). The new methods (run_job, config_detail, upload_table) read _resolved_branch_id directly and thus pick up the cached default when it was never the user's intent.
Fix approach: split the field into two:
- self._user_branch_id = branch_id (the value passed at construction, never mutated)
- self._cached_default_branch_id: int | None = None (set only by _effective_branch_id)
Then _effective_branch_id returns self._user_branch_id if not None, else resolves and caches in _cached_default_branch_id. The new methods (run_job, config_detail, upload_table) should use self._user_branch_id (the explicit choice), not the cache. This way calling query() has no side effects on the other methods.
Affected locations:
- lib.py __init__: split _resolved_branch_id into two fields
- lib.py _effective_branch_id: read _user_branch_id, cache in _cached_default_branch_id
- lib.py run_job (line 382), config_detail (line 413), upload_table (line 448): read _user_branch_id instead of _resolved_branch_id
Was this helpful? React with 👍 or 👎 to provide feedback.
| variable_values_id: Optional explicit values row for linked variables. | ||
| branch_id: Dev branch to run on. Defaults to the client's branch | ||
| (``None`` = production). | ||
| mode: Queue job mode (``run`` | ``debug`` | ``forceRun``). |
There was a problem hiding this comment.
🚩 run_job docstring lists forceRun as a valid mode but VALID_JOB_MODES constant doesn't include it
The docstring at lib.py:377 says mode accepts run | debug | forceRun, but constants.py:116 defines VALID_JOB_MODES = frozenset({"run", "debug"}). No validation is performed against VALID_JOB_MODES in either run_job or create_job (client.py:2496), so forceRun would be accepted by the API if the Queue API supports it. This is a documentation inconsistency rather than a runtime bug — either the constant should include forceRun or the docstring should be narrowed. The existing JobService.run_job in the CLI validates against VALID_JOB_MODES (which excludes forceRun), so there's a discrepancy in what the facade vs. CLI allows.
Was this helpful? React with 👍 or 👎 to provide feedback.
#426/#427/#428) (#432) * feat(lib): typed SDK return models + py.typed marker (#428) (#429) Typed pydantic return models (JobResult/QueryResult/UploadTableResult/SyncPushResult/ConfigDetailResult) + py.typed marker + typed facade wrappers. Part 1/3 of the SDK-hardening collector. * feat(job): client-side idempotency key for run_job (#427) (#430) Optional --idempotency-key on job run + Client.run_job: client-side dedup store (Queue API has no server token, verified vs live spec). Part 2/3 of the SDK-hardening collector. * feat(sync): clone composite + flow-task configId remap (#426) (#431) kbagent sync clone + SyncService.clone_project: copy a reference tree, apply bucket/variable/instance overrides, push fresh with flow-task + variable-link remap (new push Phase D). Part 3/3 of the SDK-hardening collector. * fix(review): address #432 review findings (Devin + kbagent-pr-reviewer) - context.py AGENT_CONTEXT (Devin BUG): the `sync clone` block was inserted between the `sync push` one-liner and its multi-paragraph continuation, so push semantics (encryption fail-closed, fresh-CREATE, --branch) mis-read as clone properties. Move the clone block AFTER the full push description. - keboola-expert.md (NB-1): add a `sync clone` row to the §2 tool matrix so the agent recommends the composite for "provision a new project from a reference", not the manual pull+edit+push flow. - library-workflow.md (NB-2): clarify CloneResult DOCUMENTS the dict shape that SyncService.clone_project returns (the service returns a plain dict; wrap via model_validate) -- it is not returned as a typed instance. - test_result_models.py (NB-2): add CloneResult contract tests (embedded SyncPushResult, ok property, dry_run-without-push) + cover it in the base loop. - test_e2e.py (NB-3): document why the clone E2E step is --dry-run only (no fresh second project in the single-project E2E harness; push path is unit-covered). - job_idempotency_store.py (NIT-2): comment that force_rerun intentionally bypasses the collision guard (don't hoist the check out of the branch). - sync-workflow.md: note that `sync clone --dry-run` still writes --target-dir. NIT-1 (formatter: Any) intentionally NOT applied: all sibling _format_* helpers in sync.py use `Any` -- matching the established pattern. NB-4 (sync_service.py LOC split) tracked as a follow-up tech-debt task. make check green (4092).
Part 1 of 3 of the SDK-hardening collector branch (#426/#427/#428).
What
Closes #428. Makes the importable in-process SDK (
keboola_agent_cli.Client+ result contracts) statically typed.py.typedmarker (PEP 561), forced into the wheel via hatchforce-include. Downstreammypy/ty/IDEs now treat the SDK as typed.result_models.py:JobResult,QueryResult,UploadTableResult,SyncPushResult,ConfigDetailResult. Exported from the package root.extra="allow"so backend field drift never raises (extras preserved, reachable via attribute +model_dump()).populate_by_name=True+AliasChoicessoModel.model_validate(service_dict)works on raw API keys (incl. the asymmetric QueuecomponentvsconfigId) or snake_case.Client:run_job→JobResult,query_result→QueryResult,config_detail→ConfigDetailResult,upload_table→UploadTableResult.query()refactored onto a shared_run_query(behavior unchanged, stilllist[dict]).Why
The service layer returns
dict[str, Any]; in-process consumers (FIIA Scaffold Kit) couple to undocumented shapes and only discover a breaking change at runtime. Typed contracts make integrations robust against version drift, self-documenting, and statically checkable. Strategy: typed at the facade — services keep returning dicts, so the CLI layer is untouched.Notes
command-syncsurfaces touched. Doc surface islibrary-workflow.md(updated).main.Tests
tests/test_result_models.py(per-model contract tests: real camelCase keys, extra-field survival, convenience props) + new facade-method tests intests/test_lib.py.make checkgreen (4037 passed).