Skip to content

fix(0.30.8): close issue #274 -- per-element sql_resplit closes ODBC statement-count crash - #275

Merged
padak merged 1 commit into
mainfrom
fix/issue-274-sql-resplit
May 11, 2026
Merged

fix(0.30.8): close issue #274 -- per-element sql_resplit closes ODBC statement-count crash#275
padak merged 1 commit into
mainfrom
fix/issue-274-sql-resplit

Conversation

@padak

@padak padak commented May 11, 2026

Copy link
Copy Markdown
Member

Summary

Closes #274.

Closes the remaining gap on the v0.28.0 (#245) script[] normalization. v0.28.0 covered str -> array for the lax-Storage-API / strict-runtime shape validator mismatch. v0.30.8 covers the case where script is already a list but an element packs multiple ;-separated statements — the Snowflake / BigQuery / etc. runtime crashes at odbc_prepare with Actual statement count 2 did not match the desired statement count 1 (SQL state 0A000), the same write-side trap the v0.28.0 fix closed for string inputs.

Repro before fix (verified)

Live-reproduced against project 901 (padak) config 01km0sd189fdrcnjwk89cd1fkc:

  1. kbagent config update --configuration-file repro.json with script: ['CREATE TABLE ...; alter session unset week_start;'] (list of 1, packs 2 stmts)
  2. Storage API: 200 OK, version 58 -> 59, response envelope normalizations: [] (no warning)
  3. kbagent job run --wait: job 1307622107 crashed with:
    odbc_prepare(): SQL error: Actual statement count 2 did not match the
    desired statement count 1, SQL state 0A000 in SQLPrepare
    
  4. Rolled config back to 3-block baseline (v60), cleaned up.

Fix

Extend normalize_blocks_codes_script in src/keboola_agent_cli/sync/code_extraction.py. After the existing isinstance(script, str) branch, add an elif is_sql and isinstance(script, list) branch that runs each element through the same split_statements() state machine #245 already wired up, and replaces multi-statement elements inline. Path: smallest possible delta — same is_sql_transformation_component() gate, same splitter helper.

Each replacement emits a sql_resplit entry in the normalizations envelope:

{
  "path": "parameters.blocks[0].codes[0].script[2]",
  "action": "sql_resplit",
  "before_type": "str",
  "after_type": "list",
  "before_length": 1,
  "after_length": 2
}

path points at the original element index on input (not the post-split position) so users can map the warning back to their source payload even when later elements shift due to upstream splits in the same list.

Non-SQL components (Python ; is a valid intra-statement separator: print('a'); print('b')) skip this pass — same is_sql gate as #245.

Tests

Plugin / agent surfaces updated (per CLAUDE.md convention #17)

Test plan

…statement-count crash

Closes the remaining gap on the v0.28.0 (#245) `script[]` normalization.
v0.28.0 covered `str -> array` for the lax-Storage-API / strict-runtime
shape validator mismatch. v0.30.8 covers the case where `script` is
already a list but an element packs multiple `;`-separated statements --
the Snowflake/BigQuery/etc. runtime crashes at `odbc_prepare` with
`Actual statement count 2 did not match the desired statement count 1`
(SQL state 0A000), the same write-side trap the v0.28.0 fix closed for
string inputs.

Live-reproduced against project 901 (`padak`) config
`01km0sd189fdrcnjwk89cd1fkc`: push CREATE+ALTER as single list element
-> Storage API 200 OK, version 58->59, normalizations empty (pre-fix);
job 1307622107 crashed with the exact ODBC error.

Fix runs each SQL-transformation list element back through
`split_statements()` (same state machine #245 wired up) and replaces
multi-statement entries inline. Each replacement emits a `sql_resplit`
entry in the `normalizations` envelope with `path` pointing at the
**original** element index on input. Non-SQL components (Python `;` is
a valid intra-statement separator) skip this pass.

11 new unit tests + 1 new integration test cover: CREATE+ALTER,
CREATE+SELECT, well-formed list no-op, mixed bad+good (original-index
reporting), multiple bad elements, `;` inside `/* ... */` not a
separator, `;` inside `'...'` not a separator, Python list passthrough,
BigQuery list resplit, defensive None-element passthrough, and the
end-to-end `ConfigService.update_config` write path. Suite now passes
2884 (+ 12 deselected as `not e2e`) tests, 0 regressions.

@padak padak left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of #275 — fix(0.30.8): per-element sql_resplit closes ODBC statement-count crash

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 closes issue #274 by extending normalize_blocks_codes_script in src/keboola_agent_cli/sync/code_extraction.py with a per-element re-split branch for SQL transformations: when script is already a list but an element packs multiple ;-separated statements, each element is re-run through split_statements() and replaced inline. The fix is well-scoped (smallest possible delta on top of the existing #245 machinery), the live repro is documented, test coverage is thorough (11 unit tests + 1 service-layer integration test), and all plugin/agent surfaces listed in CONTRIBUTING.md's Plugin synchronization map have been updated. One non-blocking gap exists in the CLI-layer (CliRunner) test suite, and the existing human-mode warning message text is mildly misleading for the new action — both noted below.

Verdict: APPROVE

Verdict

  • Verdict: APPROVE
  • Blocking findings: 0
  • Non-blocking findings: 2
  • Nits: 1

Blocking findings

(none)

Non-blocking findings

[NB-1] tests/test_normalize_script.py:776 — no CliRunner test exercises the sql_resplit path end-to-end

TestConfigUpdateCliNormalization has two existing tests that cover sql_split (string input) via CliRunner — one for JSON-mode envelope, one for human-mode warning text. Neither was extended for the new sql_resplit path (list-with-multi-statement input). The service-layer integration test at line 733 (test_sql_list_element_resplit_before_push) confirms the post-resplit payload reaches the HTTP client, but it bypasses the Typer command layer entirely. A CliRunner test that feeds --configuration '{"parameters":{"blocks":[...script: ["SELECT 1; SELECT 2;"]]...}}' and asserts envelope["data"]["normalizations"][0]["action"] == "sql_resplit" would pin the full path from CLI argument through service through output formatter, consistent with the existing sql_split coverage. Per CONTRIBUTING.md, every CLI command change requires a CLI-layer test.

Fix: add a test_json_envelope_carries_sql_resplit method to TestConfigUpdateCliNormalization that supplies a list-with-multi-statement element and asserts action == "sql_resplit" in the JSON envelope.

[NB-2] src/keboola_agent_cli/commands/config.py:751 — human-mode warning text says "string -> list" when sql_resplit fires

The _emit_normalizations_warning function (not modified in this PR) emits the hardcoded message "Auto-normalized N script field(s) to array (string -> list)." at line 751. When sql_resplit fires the transformation is list -> list (element expansion), not string -> list. This PR intentionally reuses the existing action-agnostic warning, which is a reasonable trade-off, but the literal message text is now factually wrong for the new case. The PR's own changelog entry acknowledges this ("the line is action-agnostic so it fires for sql_split, wrap_array, AND sql_resplit") but the message was not updated to match. Users who see the warning in human mode and parse it literally will misunderstand what was normalized.

Fix: update the message to "Auto-normalized N script field(s). See --json for details." (dropping to array (string -> list) which is now inaccurate for one of the three action types). This is a one-line change to commands/config.py:751-752. Alternatively, emit per-action counts with accurate text.

Nits

  • [NIT-1] tests/test_normalize_script.py:489-503test_well_formed_list_is_noop passes cfg (the original dict) to normalize_blocks_codes_script without copy.deepcopy, then asserts out == before. Since the function mutates config in place, cfg and out are the same object and the equality assertion is trivially true even if mutation occurred. The assertion is sound only because no mutation happens for this input, but the test pattern is fragile: a future regression that mutates a well-formed list would not be caught. All other tests in the class correctly use copy.deepcopy(cfg).

Verification log

  • gh auth status → authenticated to github.com as padak (active account) ✓
  • gh pr view 275 --json state,...state: OPEN, +489/-68, 10 files, conventional fix(0.30.8): prefix ✓
  • git rev-parse --abbrev-ref HEAD in worktree → fix/issue-274-sql-resplit ✓ (matches <branch> input)
  • 3-layer compliance: grep typer/click/httpx in new + lines across layers → (none) ✓
  • New @*.command() decorators in diff → (none); no new CLI command, so permissions.py, context.py, hints/definitions/, CLAUDE.md All CLI Commands updates are not required ✓
  • Plugin synchronization map (all "NO" rows): keboola-expert.md §1 Rule 6 VERSION GATE updated with 0.30.8+ list-element re-split entry ✓; §2 Tool Selection Matrix row for "Update SQL transformation body" updated with 0.30.8+ note ✓; §3 Inline Gotchas new entry added ✓; gotchas.md extended with (since v0.30.8; #274) tag ✓; commands-reference.md config update note updated ✓
  • grep 'commands/config.py' /tmp/kbagent-pr-275.files → NOT in PR diff (warning text not updated — leads to NB-2) ✓
  • make check (worktree) → ruff clean, 211 files formatted, SKILL.md up-to-date, plugin.json in sync, changelog complete, error-code check OK, 2884 passed, 7 skipped exit 0 ✓
  • CliRunner test for sql_resplit path → absent in TestConfigUpdateCliNormalization (leads to NB-1)
  • test_well_formed_list_is_noop mutation-safety → uses cfg without deepcopy, trivially true equality (leads to NIT-1)
  • Behavior verification: live repro documented in PR description (project 901, job 1307622107, exact ODBC error); cannot re-run without E2E credentials. Unit test test_create_plus_alter_session_resplit_in_place reproduces the canonical input shape from the bug report and confirms correct 2-element split output ✓
  • Security: no token exposure, no httpx outside client files, no bare except:, no magic numbers, no print() in production code ✓
  • Backward compat: before_length is a new field in sql_resplit records only; existing consumers of normalizations (e.g. commands/config.py:706, test_e2e.py:6772) use .get() / or [] patterns that tolerate extra fields ✓

Open questions for the author

(none)

@padak
padak merged commit 56a4915 into main May 11, 2026
1 check passed
@padak
padak deleted the fix/issue-274-sql-resplit branch May 11, 2026 21:33
padak added a commit that referenced this pull request May 11, 2026
…276)

Both PR #266 (feat: project edit --new-alias + --dry-run) and PR #275
(fix: per-element sql_resplit closes ODBC statement-count crash on
#274) shipped as patch bumps (0.30.7 / 0.30.8) but never carried an
external release. Consolidate them into a single minor release 0.31.0
-- a feature warrants the minor bump and the bug-fix rides along.

No code changes; this is purely the version-label rename across the
silent-drift sync surfaces:

- pyproject.toml + plugin.json + marketplace.json + uv.lock -> 0.31.0
  (via make version-sync)
- changelog.py: 0.30.8 + 0.30.7 keys merged into one 0.31.0 block,
  features first (highlight ordering), then the SQL fix, then tests
- gotchas.md: 5 references to (since v0.30.7) / (since v0.30.8) ->
  (since v0.31.0)
- commands-reference.md: project edit gotcha pointer + config update
  auto-normalize version-gate annotation -> 0.31.0
- keboola-expert.md: 6 references in Rule 6 VERSION GATE, Tool
  Selection Matrix rows, and inline gotchas -> 0.31.0

make check (lint + format + skill freshness + version sync + changelog
completeness + 2895 tests) clean. No regressions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

config update: re-split script[] elements that contain multiple statements (gap beyond #245)

1 participant