docs(lib): correct query() value-typing contract -- Snowflake returns strings (0.61.1) - #418
Conversation
… strings (0.61.1) A live E2E round-trip against a Snowflake workspace showed the Query Service /results endpoint returns scalars as JSON strings (1 -> "1", true -> "true"; NULL -> None), not native types. The 0.61.0 Client.query() docstring and release notes wrongly claimed "native JSON types". The facade is transparent and does not coerce -- callers must cast. - lib.py: query() docstring documents the real, stable contract. - gotchas.md: add the type-serialization gotcha to the facade entry. - v0.61.0 GitHub release notes corrected (gh release edit). - Bump 0.61.1, changelog. No behavior change.
padak
left a comment
There was a problem hiding this comment.
Review of #418 — docs(lib): correct query() value-typing contract -- Snowflake returns strings (0.61.1)
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 corrects a contract lie introduced in 0.61.0: the Client.query() docstring and release notes claimed values come back as "native JSON types (int/float/bool/None)" but a live E2E round-trip showed the Query Service serializes every Snowflake scalar as a JSON string. The fix is documentation-only -- lib.py docstring updated, gotchas.md extended, version bumped to 0.61.1. No behavior change. The approach is sound and the core fix is correct.
However, there are two issues worth addressing. First, the existing unit test test_maps_columns_and_rows_to_dicts uses integer mock data ([1, "alice"]) and asserts {"id": 1, ...}, which now contradicts the documented contract -- callers reading the test will believe integers come through, not strings. This is the most high-signal gap. Second, the 0.61.0 entry in changelog.py still says "native JSON types" (the wrong claim), and the new gotchas.md bullet does not carry an (updated v0.61.1) sub-tag as the file's own versioning convention requires.
Verdict: REQUEST CHANGES — one blocking finding (misleading test fixture), two non-blocking findings.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 1
- Non-blocking findings: 2
- Nits: 1
Blocking findings
[B-1] tests/test_lib.py:77 and tests/test_lib.py:64–72 — unit test mock data contradicts the newly documented contract
The _wire_single_select fixture (line 63–72) populates "data": [[1, "alice"], [2, "bob"]] with an integer 1 in position 0, and test_maps_columns_and_rows_to_dicts (line 76–78) asserts rows == [{"id": 1, ...}] -- an integer. But the PR's whole point is that the Query Service returns every Snowflake scalar as a JSON string (1 → "1"), so the real contract is {"id": "1", ...}. A developer reading this test comes away with exactly the wrong belief the PR is trying to fix: "the library gives me ints." The test passes only because both the fixture and the assertion use integers, which is internally consistent but wrong against reality.
Fix: Update the fixture to return string values where the contract dictates ("data": [["1", "alice"], ["2", "bob"]]) and update the assertion to match ({"id": "1", "name": "alice"}). Add a short comment noting the strings are intentional. The test_multi_statement_returns_last_result_set mock at line 127 ("data": [[7]], assert {"n": 7}) has the same issue and should be updated in the same commit.
Non-blocking findings
[NB-1] src/keboola_agent_cli/changelog.py:44 — the 0.61.0 changelog entry still says "native JSON types"
The 0.61.0 entry (line 44) reads "returns \list[dict]` rows over the fast inline `/results` path (native JSON types; ...". This is the text the PR describes as wrong. The 0.61.1 note says "the 0.61.0 docstring and release notes wrongly claimed 'native JSON types'", but the changelog entry itself is not corrected. Future readers of kbagent changelog --full` will still see the wrong claim under 0.61.0 and the correction under 0.61.1 only, leaving the history in a contradicted state.
Fix: In the 0.61.0 entry replace "native JSON types" with "warehouse-serialized strings (see 0.61.1 correction)" -- or append a parenthetical (corrected in 0.61.1: values are strings, not native types). The correction does not need to be long; it just needs to remove the actively misleading claim from the displayed history.
[NB-2] plugins/kbagent/skills/kbagent/references/gotchas.md:2679 — new bullet missing (updated v0.61.1) per the file's own versioning convention
The file's opening comment (lines 3–11) documents the convention explicitly: for a refinement added to an existing section, use (updated vX.Y.Z -- closes #N) as an inline sub-tag. The new third bullet ("query() values are warehouse-serialized strings, NOT native types.") was added to the existing ### ... (since v0.61.0) section without any version tag. An AI agent scanning for "when did this behavior appear?" will attribute it to v0.61.0, not v0.61.1, which matters because 0.61.0 got the contract wrong and 0.61.1 is the first release where this correction is documented.
Fix: Add (updated v0.61.1 -- closes #416) to the end of the new bullet's opening sentence, e.g. - **\query()` values are warehouse-serialized strings, NOT native types. (updated v0.61.1 -- closes #416)**`.
Nits
[NIT-1]plugins/kbagent/skills/kbagent/references/gotchas.md:2684— the parenthetical "(Verified live against a Snowflake workspace; BigQuery may differ.)" is a useful hedge but "may differ" is under-specified. The PR description says only Snowflake was tested; it would be more honest to say "BigQuery behavior not yet verified." Not a merge blocker.
Verification log
gh pr view 418 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 7 files, +28/-7, state OPEN, baseRef main, headRef fix/facade-query-types-docs ✓git rev-parse --abbrev-ref HEAD→fix/facade-query-types-docs✓ (working tree on PR branch)CONTRIBUTING.mdandCLAUDE.mdread, Plugin synchronization map loaded ✓plugins/kbagent/agents/keboola-expert.mdread ✓- Layer violation checks (typer in services, httpx in commands, formatter in clients): all empty ✓
- Command surface changes (
@*_app.command(...)additions/removals): zero — this PR adds no CLI commands, so the Plugin synchronization map's per-command checklist does not apply ✓ - Convention checks (magic numbers, raw error_code strings, bare except, print() in src, token in diff): all clean ✓
- Security checks: no tokens, no credential patterns in diff ✓
make check→ 4001 passed, 8 skipped, 16 warnings — exit 0 ✓grep -n "native JSON types" src/keboola_agent_cli/changelog.py→ two hits: line 33 (0.61.1 entry, citing the wrong claim as historical context) and line 44 (0.61.0 entry, still actively asserting it) — the 0.61.0 entry is not corrected in this PRtests/test_lib.py:64–72mock data[[1, "alice"], [2, "bob"]]uses integer1, assertion{"id": 1}uses integer — contradicts the documented string contractgotchas.mdnew bullet has no(updated v0.61.1)sub-tag; file versioning convention requires it for refinements to existing sectionsplugins/kbagent/agents/keboola-expert.md§2 Tool Selection Matrix "Client" row (line 104): does not claim native types (omits them entirely, just shows-> list[dict]); no correction needed ✓- Behavior reproduction: could not run live against Snowflake workspace (no credentials in this session); the author reports a live E2E round-trip in the PR description. The code change is documentation-only (no
lib.pylogic changed), so the reproduction claim is plausible.
Open questions for the author
(none)
…otchas Follow-ups from the kbagent-pr-reviewer pass on PR #418: - B-1: tests/test_lib.py mock data + assertions now use the real string contract ("1" not 1), so the tests no longer teach "the library gives ints". - NB-1: the 0.61.0 changelog entry no longer actively claims "native JSON types" (corrected in-place; 0.61.1 cites it as the prior wrong claim). - NB-2: the new gotchas bullet carries the (updated v0.61.1 -- closes #416) tag. - NIT-1: the gotchas hedge now says "BigQuery behavior not yet verified".
|
Addressed the review in e0b7899:
|
Why
The 0.61.0
Client.query()docstring and release notes claimed values come back as "native JSON types (int/float/bool/None)". A live E2E round-trip against a Snowflake workspace proved otherwise:The Query Service
/resultsendpoint serializes every Snowflake scalar as a JSON string (1→"1",true→"true"), with SQLNULL→None. The facade is transparent and does not coerce — so the documented contract was wrong, not the code.What (documentation-only, no behavior change)
lib.py—query()docstring now states values are warehouse-serialized strings (Snowflake), cast caller-side.gotchas.md— adds the type-serialization gotcha to the existing facade entry.gh release edit.Impact
This matters for the jasnost use case (#415): they wanted to drop their
_to_int/_coerce_boolshims. With Snowflake returning strings, those shims stay —query()gives a structuredlist[dict](no more CSV text parsing), but typed values still need caller-side casting. The contract is now documented and stable rather than wrong.Tests
ty+ruffclean,test_lib.py23 passed,changelog-checkgreen. Caught and re-verified by a live E2E facade round-trip against a Snowflake workspace (query + Storage Files).