Skip to content

fix(sql): walk block nodes so DDL before a DO block is extracted - #3900

Closed
bercedev wants to merge 1 commit into
Graphify-Labs:v8from
bercedev:fix/sql-block-transaction
Closed

bercedev wants to merge 1 commit into
Graphify-Labs:v8from
bercedev:fix/sql-block-transaction

Conversation

@bercedev

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #3886.

If a migration opens with a bare BEGIN; and later has a DO $$ ... END $$; block, tree-sitter-sql treats everything from that BEGIN up to the END of the DO body as a single block node. The top-level loop in extract_sql handled statement and transaction but not block, so every CREATE TABLE inside it got skipped without any error. That's why the reporter's schema only showed the tables that came after the DO block.

Here's the tree for the reduced repro:

block ['keyword_begin', ';', 'statement', ';', 'ERROR', 'keyword_end']
ERROR []
;
ERROR ['dollar_quote', ';', 'keyword_commit', ';']

The fix adds a block branch that walks the node, the same way transaction is already handled. walk() already recurses and _add_node dedupes by id, so nothing new is needed past the dispatch.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Tests or CI
  • Refactor
  • Security fix

Verification & Invariants

Invariant: a CREATE TABLE that parses cleanly should always end up as a table node, whatever wrapper node the grammar happens to put around it.

  • Read the CONTRIBUTING.md guide.
  • Reproduced the issue and identified the invariant.
  • Made the smallest fix necessary.
  • Added a regression test (if bug fix) or isolated boundary test.
  • Kept the PR description synchronized with the final implementation.
  • Documented any limitations / unsupported cases explicitly.

Limitation: the CREATE TRIGGER inside the DO body still isn't extracted, since it lives in a dollar-quoted string the grammar doesn't parse. This PR only brings back the tables that were being dropped around it.

How was this tested?

uv sync --all-extras --frozen

# regression test, extractor reverted to v8 -> fails
uv run pytest tests/test_multilang.py::test_sql_create_table_before_do_block_in_transaction
1 failed

# with the fix -> passes
1 passed

uv run pytest tests -q -k sql
43 passed

uv run pytest tests -q
6088 passed, 11 failed

All 11 failures are in tests/test_skillgen.py and they fail the same way on plain v8. My local clone is shallow, so the audit can't read an older commit (could not read 47042beb…:graphify/skill.md). They're unrelated to this change.

Graphify-specific checklist

  • I updated generated skill artifacts (uv run python -m tools.skillgen --bless) when changing their source fragments.
  • I confirmed that AST/structural extraction remains deterministic (no ambient state dependencies like ENV variables).
  • I reviewed changes for security implications (no unsafe interpolation into shell/Python).
  • I confirmed no API keys or local-only graph data are included.
  • (If applicable) I disclosed AI authorship in my commit messages.

Note: I used Claude Code while working on this fix. I reviewed and ran all of it myself.

With a bare `BEGIN;` transaction, a later `DO $$ ... END $$;` makes
tree-sitter-sql read everything from BEGIN up to the DO body's END as a
single block node. The top-level dispatch had no branch for `block`, so
every CREATE TABLE inside it was silently dropped. Walk block nodes the
same way transaction nodes are walked.

Fixes Graphify-Labs#3886
@github-actions

Copy link
Copy Markdown

Thanks for the pull request, @bercedev. A maintainer will review it soon.

Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions.

A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic.

@graphify-labs graphify-labs Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).

Formal verification. 1 change(s) tested, no difference found (not proven).


Graphify review — findings

Recovers CREATE TABLE definitions that get parsed under a block node when a bare BEGIN; transaction contains a later DO $$ ... END $$;, whose body's END causes the parser to close the transaction as a block. extract_sql now descends into top-level block nodes instead of skipping them, so tables preceding a DO block are no longer dropped.

No blocking issues surfaced.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 347 functions depend on the 157 functions this change touches.

Health — this change adds coupling hotspots:

  • new: main() — 98 callers, 3 callees
  • new: dispatch_command() — 2 callers, 125 callees
  • new: extract_sql() — 24 callers, 9 callees
  • new: walk() — 1 callers, 8 callees
  • new: test_poisoned_manifest_is_healed() — 0 callers, 6 callees

Verification — 347 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 169 function(s) in the blast radius were not formally verified this run

Test selection

Test selection

3 of 304 test file(s) selected (1%) via static blast radius.

  • tests/test_extract.py — impact
  • tests/test_multilang.py — impact, changed-test
  • tests/test_pg_introspect.py — impact

Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.

Formal verification

No difference found (not proven): No behavior difference found in extract\_sql (not a proof).

The verifier ran both versions of extract\_sql on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.

Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.

Note: An input the sampler did not try could still differ.

· 5 more finding(s) on lines outside this diff (see the check run).

safishamsi added a commit that referenced this pull request Sep 29, 2026
…ction

HTML node-info field fix (#3918), Kotlin annotated-inferred-property
crash (#3915/#3899), SQL DDL-before-DO (#3900), Razor @functions (#3908),
Blade @extends (#3907), and imported-module resolution (#3898).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@safishamsi

Copy link
Copy Markdown
Member

Shipped in v0.9.72 (now on PyPI) via an authorship-preserving cherry-pick, so your commit keeps contributor-graph credit. Thanks @bercedev! SQL DDL before a DO block is now extracted (block node is walked).

Release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.72

@safishamsi safishamsi closed this Sep 29, 2026
@bercedev
bercedev deleted the fix/sql-block-transaction branch September 29, 2026 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants