Skip to content

fix(sql): walk into BEGIN;...COMMIT; transaction blocks (#2953) - #2955

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

fix(sql): walk into BEGIN;...COMMIT; transaction blocks (#2953)#2955
amtulifra wants to merge 1 commit into
Graphify-Labs:v8from
amtulifra:fix/sql-transaction-block-2953

Conversation

@amtulifra

Copy link
Copy Markdown

Fixes #2953 — SQL statements wrapped in BEGIN; ... COMMIT; transaction blocks were silently dropped, with zero nodes extracted and no error surfaced.

Root cause: tree-sitter-sql wraps a transaction block in its own top-level transaction node instead of statement. The extractor's root dispatch loop only walked children of type statement (plus a few special cases like ERROR), so transaction never matched and everything inside — tables, views, functions, triggers — was skipped. Any REFERENCES pointing at one of those tables also fell back to a sourceless stub instead of resolving to the real node, matching the "empty source_file/source_location" symptom described in the issue.

Fix: route transaction through the same generic walk() used everywhere else — it already recurses into whatever children it finds, so no new logic is needed, just letting the node through.

Test plan:

  • Reproduced the issue's minimal repro — confirmed 0 table nodes before the fix, 3 nodes (file + gamma + delta) with correct source_location after
  • Added a regression test (test_sql_transaction_block_is_not_dropped) following the existing conventions in tests/test_multilang.py
  • ROLLBACK instead of COMMIT
  • Views, functions, and triggers inside a transaction, not just tables
  • Multiple separate transaction blocks in one file, with an FK reference resolving across them
  • PL/pgSQL function bodies (the ERROR-node regex recovery path) nested inside a transaction — no interference
  • The issue's exact combination — schema-qualified names, IF NOT EXISTS, CHECK, ALTER TABLE, all transaction-wrapped
  • Checked for double-emission between the AST walk and the existing raw-text regex fallbacks — none found
  • Ran a synthetic 8-file, 52-table, cross-file-FK migration corpus (mirroring the issue's "59 CREATE TABLEs across 8 migrations") through the real extract() pipeline — all tables and FK edges present, zero dangling edges
  • Full test suite: 4585 passed; 12 pre-existing failures (missing optional deps in a fresh venv) confirmed identical on unmodified v8, unrelated to this change

…s#2953)

tree-sitter-sql wraps a transaction in its own top-level transaction node
instead of statement, so the extractor's root dispatch loop never reached
it. Every table, view, function, and trigger defined inside a transaction
was silently dropped with no error, and any REFERENCES pointing at one of
those tables fell back to a sourceless stub instead of the real node.
Route the transaction node through the existing generic walk, which
already recurses into whatever children it finds.

@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. No changes could be formally verified in this run.


Graphify review — findings

Adds handling for top-level transaction nodes in extract_sql so statements inside BEGIN; ... COMMIT; blocks are walked instead of dropped (#2953). Covers it with a new test_sql_transaction_block_is_not_dropped verifying both tables, their source locations, and the FK edge are emitted.

No blocking issues surfaced.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 267 functions depend on the 105 functions this change touches.

Health — this change adds coupling hotspots:

  • new: dispatch_command() — 2 callers, 119 callees
  • new: extract_sql() — 10 callers, 8 callees
  • new: walk() — 1 callers, 8 callees
  • new: test_poisoned_manifest_is_healed() — 0 callers, 6 callees

Verification — 267 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: 116 function(s) in the blast radius were not formally verified this run

Formal verification

Could not verify: Could not verify extract\_sql.

The verifier did not have enough to check extract\_sql, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set

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

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.

SQL: statements wrapped in BEGIN; … COMMIT; extract 0 nodes — 59 CREATE TABLEs silently absent across 8 migrations

1 participant