fix(sql): recover T-SQL bracket-named, CREATE OR ALTER, and PROC routines - #2960
fix(sql): recover T-SQL bracket-named, CREATE OR ALTER, and PROC routines#2960egarcia74 wants to merge 12 commits into
Conversation
T-SQL's AS BEGIN...END body idiom never parses structurally (the grammar has no create_procedure form for it), so recovery from ERROR nodes is the only path such routines have into the graph — and the recovery pattern matched only bare or double-quoted names and only OR REPLACE. In a T-SQL codebase that brackets every identifier ([dbo].[usp_X], a common house standard) and uses CREATE OR ALTER, every stored procedure silently vanished: 0/26 recovered in the reporting corpus. - accept bracket-delimited name parts and OR ALTER, mirroring what fb_proc_or_trigger already does for Firebird - hoist the pattern into a shared _ROUTINE_RECOVERY_RX used by both recovery sites (walk-time ERROR scan and whole-file has_error fallback): when the two drifted, a mixed-delimiter name (dbo.[usp_Mixed]) was captured differently by each, minting a second phantom node named after the schema that id-dedupe could not catch Recovered routines stay name-only nodes (no body reads_from edges), matching the existing PL/pgSQL recovery. Each new guard was mutation-tested: dropping the bracket alternative, dropping OR ALTER, and re-introducing the pattern drift each fail exactly their test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ames [a]]b] names the identifier a]b; stopping at the first ] truncated the recovered routine to [dbo].[a] — a phantom that could collide with a genuinely named [dbo].[a]. The bracketed-part alternative now consumes ]] before treating a lone ] as the closing delimiter, in both name-part positions of the shared _ROUTINE_RECOVERY_RX. Mutation-tested: reverting the escape handling fails the new regression test. Addresses the CodeRabbit P2 review finding on PR #2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Upstream batches changelog entries in maintainer release commits; the entry's content moves to the PR description.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Extends SQL routine recovery to handle T-SQL dialect: adds bracket-delimited identifiers ([dbo].[usp_Load], with ]] escaping), CREATE OR ALTER, and the PROC shorthand, and consolidates the previously-duplicated recovery regex into a shared _ROUTINE_RECOVERY_RX so the walk-time ERROR scan and whole-file fallback can't drift and double-emit. Adds _mask_sql_comments (offset-preserving) around the whole-file scan so commented-out DDL can't fabricate routine nodes when an unrelated parse error arms recovery. Covers all of this with new test_sql_* cases.
Worth a look
- Comment masking treats -- inside string literals as a real comment —
graphify/extractors/sql.py:37· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 279 functions depend on the 117 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 119 callees - new:
extract_sql()— 15 callers, 9 callees - new:
walk()— 1 callers, 8 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 279 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: 128 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).
…ask the ERROR-node scan The recovery mask read '-- note' inside a string literal as a line comment (blanking to end-of-line) and a /* inside a string as a block comment opener (blanking through the next real */), so real DDL sharing the span could be hidden. The mask now preserves single-quoted strings (with '' escapes), double-quoted identifiers, and bracket-delimited identifiers (with ]] escapes) before blanking comments. Literal patterns are deliberately single-line: the mask only runs on files that already failed to parse, where an unclosed quote is likely, and a multi-line match would let one unclosed delimiter swallow real DDL below it. The walk-time ERROR-node scan now masks too: an ERROR blob whose byte span covers commented-out DDL fabricated a routine node from it exactly as the whole-file scan once did (reproduced: a -- CREATE PROC line sandwiched between broken segments). Both guards are mutation-tested: a literal-blind mask fails the new unit pin; an unmasked ERROR scan fails the extended fabrication test.
|
Addressed the Graphify-review finding (
Both directions are mutation-tested: a literal-blind mask fails the new unit pin ( |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Extends SQL routine recovery in extract_sql to handle T-SQL bracket-delimited identifiers ([dbo].[usp_Load], with ]] escapes), CREATE OR ALTER, and the PROC shorthand, and consolidates the walk-time ERROR scan and whole-file fallback onto a single shared _ROUTINE_RECOVERY_RX so the two sites can no longer capture divergent names and double-emit nodes. Adds _mask_sql_comments to blank comment spans (offset-preserving, literals kept verbatim) at both recovery sites so commented-out DDL can't fabricate routine nodes. Adds tests covering the bracketed/OR ALTER/PROC forms, escaped-bracket names, cross-site name agreement, and comment masking.
Worth a look
- Routine recovery still matches DDL text inside string literals —
graphify/extractors/sql.py:510· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Optional tree_sitter_sql dependency is imported instead of skipped —
tests/test_multilang.py:536· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 283 functions depend on the 121 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 119 callees - new:
extract_sql()— 16 callers, 9 callees - new:
walk()— 1 callers, 9 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 283 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: 132 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).
… name escapes Three review findings on the recovery scans, each mutation-tested: - Single-quoted strings are now BLANKED by the mask instead of preserved: routine names never live in single quotes, and preserved dynamic SQL (EXEC(N'CREATE PROC [dbo].[Fake] ...')) fabricated a routine node whenever an unrelated parse error armed the whole-file scan. Double-quoted and bracket-delimited identifiers stay verbatim — they are exactly the delimited names the recovery regex must see. - An UNCLOSED block comment now masks to end-of-file (matching SQL semantics). Requiring the closing */ left everything after an unterminated /* unmasked, and an unterminated comment is exactly the kind of error that arms recovery, so DDL inside it fabricated nodes. - The double-quoted name atom in _ROUTINE_RECOVERY_RX and the mask now consume "" escapes, so CREATE PROCEDURE "dbo"."a""b" recovers its full name instead of truncating at the first quote — the "" twin of the ]] bracket escape. (For a statement the grammar CAN parse, tree-sitter-sql itself truncates object_reference at the escape; that upstream grammar defect is out of scope here.) Also converts the ]] regression test's hard tree_sitter_sql import to pytest.importorskip — on this branch the grammar is still an optional extra, so a default install's suite must skip, not error.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds T-SQL routine recovery to extract_sql in graphify/extractors/sql.py by sharing a single _ROUTINE_RECOVERY_RX between the walk-time ERROR scan and the whole-file has_error fallback, extending it to accept bracket-delimited names (with ]] escapes), PROC/PROC EDURE, and CREATE OR ALTER. Introduces _mask_sql_comments and applies it at both recovery sites so commented-out or single-quoted (dynamic SQL) DDL can't fabricate routine nodes. Adds tests covering bracketed procedures, CREATE OR ALTER, escaped brackets, cross-site name agreement, and comment/literal masking.
Worth a look
- Routine recovery still scans inside multi-line single-quoted strings —
graphify/extractors/sql.py:59· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 287 functions depend on the 125 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 119 callees - new:
extract_sql()— 18 callers, 9 callees - new:
walk()— 1 callers, 9 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 287 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: 136 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).
… masked file Independent review of 3e44cbe found the regex mask had hit its complexity ceiling, with one regression and one surviving hole: - REGRESSION: a /* inside a MULTI-LINE single-quoted string (ordinary T-SQL dynamic SQL) was not recognized as a literal by the single-line string atom, so the new unclosed-comment-to-EOF rule blanked the rest of the file — hiding every routine below. The mask is now a linear character scanner: the in-string /* is consumed as blanked, line-scoped string content and never opens a comment. - Nested /* */ (SQL Server and PostgreSQL both nest) ended at the first */, so DDL commented out inside a nested comment fabricated a node. The scanner tracks nesting depth. The scanner emits exactly one output character per input character, so the offset/line invariant holds by construction. The per-ERROR-node recovery scan is removed as unsound, not just redundant: tree-sitter's lexer does not nest /* */ either, so an ERROR fragment can begin MID-comment with no opener in sight, and no fragment-level mask can know that. Any ERROR node makes root.has_error true, so the whole-file masked scan already recovers everything the per-node scan could, from the same shared regex, with id dedupe. Also narrows the fabrication claim to what the mask models (MySQL double-quoted strings and dollar-quoted bodies still rely on the has_error gate alone) and updates the comments that overclaimed.
…est rationale Verification review of 5c285bb surfaced one regression and two overclaiming comments: - A bracket/double-quote span that reaches its closer ACROSS a comment opener ('SELECT [Col FROM t -- CREATE PROC [dbo]' closes on [dbo]'s bracket) was preserved verbatim, shielding the commented-out DDL inside it from the mask — as was a truly unterminated opener, whose span ran to end-of-line. Both shapes are now abandoned: the delimiter is emitted alone and the line rescanned, so the comment fires. The trade — a genuine identifier containing '--' or '/*' ([a--b]) is now conservatively blanked past the opener, losing that routine's recovery, never fabricating one — is stated at the branch. - The line-scoping rationale claimed single-quoted dynamic SQL was handled; only the same-line form is. Multi-line dynamic SQL (SET @SQL = N'<newline>CREATE PROC ...') fabricates from its continuation lines and is now documented as a known hole beside the unmodelled quoting dialects, with the reason it is not closed (a string heuristic risks swallowing real DDL in a recovery-only path). - The nesting comment now names the dialects that do NOT nest /* */ (MySQL, Oracle), where depth tracking over-blanks — losing, never fabricating, which is the accepted direction.
Fuzz verification of 1d56f17 found the abandon-and-rescan rule re-paired single quotes: a quote inside an abandoned span became a live opener, shifted pairing could swallow a genuine dynamic-SQL string's closing quote, and EXEC(N'CREATE PROC ...') was exposed again — the exact fabrication 3e44cbe closed, reachable through a one-line prefix (SELECT [Col's -- x] ; EXEC(N'...')). A distrusted span (unterminated, or closing across a -- or /*) is irreducibly ambiguous — identifier data vs stray delimiter — and any rule that picks one reading exposes text another reading blanks. So blank the union of every reading: the rest of the span's line is blanked outright, and a raw /* on it with no later */ on the same line carries forward as nesting-aware comment state (some reading may have left it open; a real */ below still closes it). Under-blanking is what fabricates and is now structurally impossible for these spans; over-blanking loses at most routines on a line that was already broken. Adds a deterministic 20k-case fuzz pinning the structural invariants (one char per char, newlines preserved, blank-or-verbatim, idempotence) so the next masking defect class trips a property, not a hand-written shape.
…ial fuzz Differential fuzzing of c786d61 (1M cases) found the union stopped one step short: where a carried block comment closed MID-line, the rest of that line was emitted verbatim — but under the reading where the carry never opened, that whole line can be a comment or a string ('-- note */ CREATE PROC ...' fabricated from a line that BEGINS with --). The distrusted-span blank is now a loop (_blank_tail_and_carry): blank to end-of-line, carry any raw unclosed /* to its nesting-aware close, and where that close lands mid-line apply the same rule to the remainder of that line, until a line ends carry-free. Content on a carry-close line is conservatively lost; the next line resumes normally. Lands the defect class as a test, not just shapes: a frozen copy of this revision's mask is embedded in the test file and a 20k-case differential fuzz asserts the live mask never exposes a character the frozen baseline blanks — the monotonicity property that found every masking defect in this series (it fails on c786d61; the structural fuzz alone did not).
Review sign-off notes: the subset claim is now scoped to exclude the irreducible */* token-split divergence (a fifth absolute claim was not going to fare better than the first four), and the differential-fuzz test says how to refresh its frozen baseline on a deliberate exposure change instead of being deleted.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds _mask_sql_comments (a linear offset-preserving scanner that blanks single-quoted strings and nesting-aware comments while preserving delimited identifiers) and _ROUTINE_RECOVERY_RX, and routes both SQL routine-recovery sites (walk-time ERROR scan and whole-file fallback) through them so quoted/bracket-delimited CREATE FUNCTION/PROC[EDURE] names recover consistently and DDL hidden in comments or strings no longer fabricates nodes. Extends recovery to handle CREATE OR ALTER, PROC shorthand, and doubled-bracket (]]) escapes. Adds the corresponding SQL multilang tests plus rationale entries.
Worth a look
- Dollar-quoted SQL text can fabricate routines when recovery is armed —
graphify/extractors/sql.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Routine recovery matches DDL keywords inside delimited identifiers —
graphify/extractors/sql.py:31· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 298 functions depend on the 136 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 119 callees - new:
extract_sql()— 18 callers, 9 callees - new:
walk()— 1 callers, 8 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 298 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: 147 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).
The Graphify review bot flagged (and a repro confirmed) that DDL keywords occurring INSIDE a preserved delimited identifier fabricate a routine: 'SELECT 1 AS [CREATE PROCEDURE dbo.usp_Phantom pending]' in a file with an unrelated parse error minted dbo.usp_Phantom(). The mask must preserve delimited identifiers verbatim — they carry the recoverable names — but the recovery regex was not span-aware, so it matched keywords inside them. The scanner (_scan_sql) now also returns the [start, end) of every preserved identifier span, and the whole-file recovery scan skips any match whose CREATE keyword starts inside one: a genuine statement's NAME may be a delimited identifier; its CREATE never is. _mask_sql_comments stays as the masked-text-only wrapper, so the frozen-baseline and structural fuzz tests apply unchanged (blanking behavior is untouched). Mutation-tested: removing the skip fails the new regression test.
|
Both advisory findings from the latest Graphify review triaged:
Validation: full suite |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Reworks SQL routine recovery in graphify/extractors/sql.py around a shared _ROUTINE_RECOVERY_RX and a new _scan_sql character scanner that masks comments and single-quoted strings while preserving delimited identifiers, so the walk-time ERROR scan and whole-file fallback agree on captured names and stop fabricating routine nodes from commented-out or dynamic SQL. Extends the regex to handle quoted/bracket-delimited names, CREATE OR ALTER, PROC shorthand, and doubled-delimiter escapes. Adds extensive tests/test_multilang.py coverage for quoted PL/pgSQL routines, bracket-named T-SQL procedures, escaped delimiters, comment/literal masking, and recovery-site name agreement.
Worth a look
- Routine recovery matches CREATE inside bare identifiers —
graphify/extractors/sql.py:33· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Routine recovery scans PostgreSQL dollar-quoted strings —
graphify/extractors/sql.py:665· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 302 functions depend on the 140 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 119 callees - new:
extract_sql()— 19 callers, 9 callees - new:
walk()— 1 callers, 8 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 302 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: 151 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).
The Graphify review bot flagged (and a repro confirmed) that the recovery regex matched CREATE inside a bare identifier: 'SELECT AUTOCREATE PROCEDURE x FROM t;' in an error-bearing file minted a phantom routine x(). Delimited identifiers are span-skipped at the scan site, but a bare word has no span, so the regex itself must refuse — \bCREATE closes it. Mutation-tested: removing the boundary fails the new regression test.
|
Latest Graphify-review findings triaged:
Validation: full suite |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 3 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds a comment/string masking layer to the SQL routine-recovery path: introduces _scan_sql (with helpers _blank, _blank_tail_and_carry) to blank single-quoted strings and nesting-aware block/line comments while preserving delimited identifiers, plus a shared _ROUTINE_RECOVERY_RX used by both recovery sites. Extends the regex to recover quoted PL/pgSQL, CREATE OR ALTER, PROC, and bracket-delimited T-SQL routines, and makes the scan skip matches inside preserved identifier spans. Adds multilang tests covering dynamic-SQL/comment non-fabrication, delimited-name escapes, PL/pgSQL survival, and mask fuzz/invariants.
Worth a look
- Recovery scan reads names from masked_src, blanking delimited identifier content —
graphify/extractors/sql.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Dollar-quoted SQL strings are scanned as executable DDL —
graphify/extractors/sql.py:79· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Routine recovery scans PostgreSQL dollar-quoted strings as DDL —
graphify/extractors/sql.py:672· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 304 functions depend on the 142 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 119 callees - new:
extract_sql()— 20 callers, 9 callees - new:
walk()— 1 callers, 8 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 304 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: 153 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).
|
Triage of the latest Graphify-review run (on f3d0248), for whoever reviews this next:
The review bot re-derives findings on every push, so the dollar-quoting advisory recurs each run. To keep this thread readable I'll stop replying per-run here: every distinct finding to date is either fixed with a named, mutation-tested commit (four were real — thanks, genuinely useful) or answered with rationale above. Awaiting human review. |
|
Superseded by #3166 — the same change squashed to a single commit and rebased onto current v8 (0.9.51) so it cherry-picks cleanly, with the defect now tracked in #3164. The review history stays here: each Graphify-review finding on this PR was reproduced and either fixed or refuted in the thread above, and the final scanner is what #3166 carries. Closing this one. |
1 participant
Problem
T-SQL stored procedures and functions are silently dropped from the graph. On a real 391-file T-SQL data-migration corpus (SQL Server DACPAC project: staging/model/transform layers,
[Schema].[Object]naming throughout), 0 of 26 stored procedures produced a node — while tables and views extracted fine, so the gap is invisible unless you go counting.Two independent defects stack:
CREATE PROCEDURE ... AS BEGIN ... ENDnever parses structurally in tree-sitter-sql (there is nocreate_procedureproduction for theAS BEGINbody idiom), so ERROR-node regex recovery is these objects' only path into the graph.CREATE PROCEDURE [dbo].[usp_Load]), noCREATE OR ALTER, noPROCshorthand. On this corpus, every routine uses brackets, so recovery recovered nothing.What this PR does
All changes are in
graphify/extractors/sql.py:_ROUTINE_RECOVERY_RX, hoisted to module level and used by both recovery sites (the walk-time ERROR-node scan and the whole-filehas_errorfallback). The two sites had drifted; the drifted second site is what minted a phantomdbo()node for a mixed-delimiter name (CREATE PROCEDURE dbo.[usp_Mixed]) — the schema captured as if it were the routine. Sharing the pattern fixes that and prevents re-drift.]]escape for a literal]inside a name ([dbo].[a]]b]recovers as one routine, fully consumed, not truncated at the escape).CREATE OR ALTERalongside the existingOR REPLACE, and thePROCshorthand alongsidePROCEDURE._mask_sql_comments(src)(offset-preserving blanking of--and/* */), so commented-out DDL in a file with an unrelated parse error can no longer fabricate routine nodes.Recovered routines are name-only nodes (no body
reads_fromedges), matching the existing PL/pgSQL recovery behavior — a named node with acontainsedge beats an invisible object.Result on the real corpus
391
.sqlfiles, before → after: 1 → 28 routine nodes, including every bracket-named[Audit]/[Utils]/[Monitoring]procedure; table and view counts unchanged; no phantom schema nodes.Tests
Added in
tests/test_multilang.py, each verified to fail with the fix reverted:[dbo].[usp_X]name;CREATE OR ALTER PROCEDURErecovers;PROCshorthand recovers;dbo.[usp_Mixed]) yields exactly one node — both recovery sites agree on the captured name (the phantom-dbo()regression);]]escape inside a bracket-delimited name is consumed, capturing[dbo].[a]]b]()as one routine;Validation
6 failed, 4836 passed, 28 skipped— the 6 (test_ollama.py,test_ollama_retry_cap.py) fail identically on unmodifiedv8in this environment (local Ollama config), none touch SQL.ruff check graphify/ tests/: all checks passed.Note
Independent of my companion PR #2959 making
tree-sitter-sqla core dependency; both touchtests/test_multilang.py, and a local merge of the two branches resolves trivially (the combined branch produced the 1 → 28 measurement above and runs in production).