Add regression test for T-SQL bracketed FK dropping child table - #2722
Add regression test for T-SQL bracketed FK dropping child table#2722ayushcodes10 wants to merge 7 commits into
Conversation
![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.
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
This pull request adds T-SQL bracket-quoted identifier support to the SQL extractor. It introduces two helper functions (_debracket_tsql, which rewrites [bracket]-quoted identifiers to backtick-quoted ones before parsing to work around a tree-sitter-sql grammar gap, and _strip_backtick_parts, which undoes that substitution for display labels), and wires them into extract_sql via new _clean_name and _ident helpers applied wherever identifier/object_reference nodes are read. The change is gated so debracketing is skipped when source already contains a backtick, and includes handling for string literals, comments, and array-type syntax. The test file adds new cases covering bracket identifier labeling, cross-reference resolution, array-type non-corruption, and other scenarios; the accompanying symbol churn in the test module reflects these additions.
No blocking issues surfaced. 5 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 271 functions depend on the 117 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract_sql()— 13 callers, 9 callees - worse:
walk()— 1 callers, 9 callees
Verification — 271 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
· 1 grounded finding(s) anchored inline below; 1 more finding(s) on lines outside this diff (see the check run).
ayushcodes10
changed the title
|
@safishamsi this PR's CI run is waiting on approval (first-time contributor gate) — could you approve the workflow run when you get a chance? Happy to address any review feedback in the meantime. |
tree_sitter_sql has no grammar rule for a bracket delimited identifier, so [dbo].[Orders] lands in ERROR nodes and comes out with a mangled label like dbo].[Orders instead of dbo.Orders. Backtick quoting already parses cleanly, so the source is rewritten before parsing and the synthetic backticks are stripped again for display. Upstream has since landed its own ERROR recovery for routine names (CREATE FUNCTION/PROCEDURE), matched by a shared regex plus a masked whole file scan. Since this rewrite runs before that scan, a bracketed routine name never reaches it as a bracket anymore, only as a backtick, so the shared regex and its masking scan are extended to also recognize backtick delimited names. Without that a bracketed routine would go from a recovered (if ugly) node to no node at all, a regression this change must not introduce. Guards against misfiring on array type and array literal syntax that also uses square brackets (int[], numeric(10)[3], ARRAY[1,2,3]), and against a bracket span that swallows a comment marker. Fixes Graphify-Labs#2712.
12e9a0e to
26f278c
Compare
There was a problem hiding this comment.
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) alter behavior, breaking input(s) attached.
Behavior changes: \_scan\_sql changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_scan\_sql behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"text":"''"\}, the old code produced \('', \[\]\) but the new code produces \(' ', \[\]\). Paste that input straight into a regression test.
Graphify review — findings
Adds a pre-parse _debracket_tsql pass that rewrites T-SQL bracket-quoted identifiers ([dbo].[Orders]) into backtick-quoted ones so tree-sitter's normal AST path handles them instead of dropping tables and mangling labels through error recovery; it distinguishes true quoted identifiers from array markers like int[]/ARRAY[...] by the preceding character and skips empty, numeric, or comment-bearing spans. When a file was debracketed, _clean_name/_strip_backtick_parts strip the backticks back off recovered and display names so labels and FK targets read normally. Extends _scan_sql and _ROUTINE_RECOVERY_RX to treat backtick-delimited identifiers as preserved routine-name spans, keeping bracketed procedure/function names recoverable after rewriting.
No blocking issues surfaced. 11 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 341 functions depend on the 161 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 25 callers, 11 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
walk()— 1 callers, 9 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 341 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: 173 function(s) in the blast radius were not formally verified this run
Formal verification
Behavior changes: \_scan\_sql changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_scan\_sql behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"text":"''"\}, the old code produced \('', \[\]\) but the new code produces \(' ', \[\]\). Paste that input straight into a regression test.
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
· 1 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
| parts.append(p) | ||
| return ".".join(parts) | ||
|
|
||
|
|
||
| def extract_sql(path: Path, content: str | bytes | None = None) -> dict: |
There was a problem hiding this comment.
extract_sql()
fans out to 11 callees (efferent coupling); 25 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
|
@safishamsi rebased onto the latest v8 and all checks are green now — ready for review whenever you get a chance. |
_clean_name gated on a single file wide debracketed flag, so once ANY bracket span in a file got rewritten to backtick form, every backtick-quoted name in that same file had its backticks stripped for display, including a genuinely backtick-quoted MySQL name completely unrelated to the T-SQL bracket span that triggered the rewrite. _debracket_tsql now returns the byte range spans it actually rewrote, in the rewritten source's own coordinates, and _clean_name only un-rewrites a name whose own byte range overlaps one of those spans. A name read off a tree-sitter node carries its own start/end byte naturally; the whole file regex fallback has no node, so its character offsets into the decoded source are converted to byte offsets first, since the decoded string and the byte-level spans can diverge once the file contains any multi byte character. Fixes Graphify-Labs#2721.
26f278c to
f2d7572
Compare
|
Same fix as the note on #2723: _clean_name now tracks the actual spans _debracket_tsql rewrote instead of a file wide flag, so a genuinely backtick-quoted MySQL name in the same file as an unrelated T-SQL bracket span keeps its own backticks. Full details and the other findings I checked (and why I left them as documented trade-offs) are on #2723. |
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. 1 change(s) alter behavior, breaking input(s) attached.
Behavior changes: \_scan\_sql changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_scan\_sql behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"text":"''"\}, the old code produced \('', \[\]\) but the new code produces \(' ', \[\]\). Paste that input straight into a regression test.
Graphify review — findings
Adds a T-SQL bracket-identifier rewrite so extract_sql runs _debracket_tsql over the source before parsing, converting [dbo].[Orders]-style names into backtick-quoted ones the grammar recognizes — fixing mangled labels and whole tables lost behind broken foreign keys that previously fell into error recovery. Distinguishes real quoted identifiers from array markers (int[], ARRAY[...]) by preceding context and refuses spans that are empty, numeric, or contain a comment opener. Un-rewrites names for display via _clean_name/_strip_backtick_parts only where a node overlaps a span _debracket_tsql actually touched, leaving genuine MySQL backtick names elsewhere untouched, and teaches _scan_sql plus the routine-recovery regex to treat backtick-delimited identifiers as recoverable routine names.
Worth a look
- _strip_backtick_parts splits on '.' which corrupts identifiers containing a literal dot inside brackets —
graphify/extractors/sql.py:415· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Debracketing rewrites bracket text inside multiline single-quoted strings —
graphify/extractors/sql.py:323· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- T-SQL bracketed reference names now change returned graph IDs/edges —
graphify/extractors/sql.py:562· 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 — 345 functions depend on the 165 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 26 callers, 11 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
walk()— 1 callers, 9 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 345 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: 177 function(s) in the blast radius were not formally verified this run
Formal verification
Behavior changes: \_scan\_sql changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_scan\_sql behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"text":"''"\}, the old code produced \('', \[\]\) but the new code produces \(' ', \[\]\). Paste that input straight into a regression test.
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
· 1 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
| parts.append(p) | ||
| return ".".join(parts) | ||
|
|
||
|
|
||
| def extract_sql(path: Path, content: str | bytes | None = None) -> dict: |
There was a problem hiding this comment.
extract_sql()
fans out to 11 callees (efferent coupling); 26 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
Exclude two cases the grammar or the scanner cannot handle safely: a bracket span containing a literal dot, which tree_sitter_sql still splits on even inside a backtick pair, and single/double quoted string content past its first line, which the scanner used to treat as ordinary source once it hit a newline before finding the real closing quote.
f2d7572 to
2a136d5
Compare
A dollar quoted span ($$ ... $$ or $tag$ ... $tag$) is opaque body text, not SQL, so bracket like content inside it was being mistaken for a real bracket identifier and rewritten. Skip the whole span instead, the same way a string or comment already is.
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.
Graphify review — findings
Adds T-SQL bracket-quoted identifier support to the SQL extractor by rewriting [dbo].[Orders]-style names to backtick-quoted ones via _debracket_tsql before parsing, so statements the grammar would otherwise dump into ERROR nodes now take the normal AST path instead of mangling labels or dropping tables behind broken foreign keys. Distinguishes real quoted identifiers from array-type markers (int[], ARRAY[...]), comment-bearing or dotted content, and strings scanned across newlines, leaving anything ambiguous as an un-rewritten bracket — the pre-existing fallback. Un-rewrites names back for display and recovery via _clean_name/_strip_backtick_parts only where a node overlaps a span _debracket_tsql actually inserted, so genuine MySQL backtick names elsewhere in the file are untouched; _scan_sql and _ROUTINE_RECOVERY_RX also now accept backtick delimiters so bracket-named routines stay recoverable in error-bearing files.
Worth a look
- subscript_like precedes-check treats a name after ')' as array marker, dropping legitimate identifier after function/paren —
graphify/extractors/sql.py:401· 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 — 349 functions depend on the 169 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 27 callers, 11 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
walk()— 1 callers, 9 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 349 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: 181 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
| parts.append(p) | ||
| return ".".join(parts) | ||
|
|
||
|
|
||
| def extract_sql(path: Path, content: str | bytes | None = None) -> dict: |
There was a problem hiding this comment.
extract_sql()
fans out to 11 callees (efferent coupling); 27 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
2a136d5 to
4f40ee6
Compare
|
Investigated the latest bot findings on this PR. Confirmed real, fixed (propagated from the base branch, now at 4f40ee6):
All three have regression tests. Investigated and concluded not a bug: "T-SQL bracketed reference names now change returned graph IDs/edges" (sql.py:562). This is the intended effect of the fix, not a regression — the node id is derived from the cleaned name ( Already covered on the base PR (#2723): the reproduced |
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. 1 change(s) alter behavior, breaking input(s) attached.
Behavior changes: \_scan\_sql changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_scan\_sql behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"text":"''"\}, the old code produced \('', \[\]\) but the new code produces \(' ', \[\]\). Paste that input straight into a regression test.
Graphify review — findings
Rewrites T-SQL bracket-quoted identifiers ([dbo].[Orders]) to backtick-quoted ones before parsing via _debracket_tsql, so the grammar's normal AST path handles them instead of dropping into error recovery that mangled labels and lost tables behind broken foreign keys. Only bracket spans that read as identifiers are rewritten — array markers (int[], ARRAY[...]), dotted names, numeric/empty content, and comment-bearing spans are left as plain brackets, and single/double-quoted strings, dollar-quoted bodies, and comments are skipped so their contents aren't corrupted. Recorded rewrite spans let downstream _strip_backtick_parts un-rewrite only the identifiers that came from brackets, sparing genuine MySQL backtick names; _scan_sql and the routine-recovery regex now also accept backtick-delimited names so bracketed routines still recover from error-bearing files.
Worth a look
- Debracketing rewrites bracket text inside native backtick identifiers —
graphify/extractors/sql.py:282· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Existing backtick identifiers are debracketed internally —
graphify/extractors/sql.py:344· 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 — 351 functions depend on the 171 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 27 callers, 11 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
walk()— 1 callers, 9 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 351 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: 183 function(s) in the blast radius were not formally verified this run
Formal verification
Behavior changes: \_scan\_sql changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_scan\_sql behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"text":"''"\}, the old code produced \('', \[\]\) but the new code produces \(' ', \[\]\). Paste that input straight into a regression test.
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
· 1 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
| parts.append(p) | ||
| return ".".join(parts) | ||
|
|
||
|
|
||
| def extract_sql(path: Path, content: str | bytes | None = None) -> dict: |
There was a problem hiding this comment.
extract_sql()
fans out to 11 callees (efferent coupling); 27 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
A MySQL backtick quoted identifier had no dedicated scan branch, so its content was walked like ordinary source. A bracket like substring inside one read as a real TSQL bracket identifier and got rewritten in place, producing doubled and orphaned backticks. Skip a backtick span whole, honoring a doubled backtick as an escaped literal one the same way this function writes it.
4f40ee6 to
9d3e957
Compare
|
Follow up on this round's bot findings (matching 9d3e957): Confirmed real, fixed (propagated from the base branch): the same backtick-corruption bug described on #2723 — a genuine backtick-quoted identifier containing bracket-like text got corrupted because _debracket_tsql had no dedicated scan branch for a backtick span. Fixed the same way. Also investigated and confirmed as expected behavior (not a bug): "T-SQL bracketed identifiers now produce different public node labels/IDs" (sql.py:517). Same conclusion as the earlier round's finding on this PR — the node id is derived from the cleaned name, so a correct, clean id replacing whatever an ERROR-recovery path previously fabricated (or nothing, since the FK self-loop bug this PR fixes often dropped the table entirely) is the intended effect of #2712/#2713, not a regression. |
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. 1 change(s) alter behavior, breaking input(s) attached.
Behavior changes: \_scan\_sql changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_scan\_sql behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"text":"''"\}, the old code produced \('', \[\]\) but the new code produces \(' ', \[\]\). Paste that input straight into a regression test.
Graphify review — findings
Rewrites T-SQL bracket-quoted identifiers ([dbo].[Orders]) into backtick-quoted ones before parsing via the new _debracket_tsql, so tree_sitter_sql's normal AST path handles them instead of dropping the statement into error recovery and losing tables behind broken foreign keys (#2712, #2713). Distinguishes real identifiers from array markers (int[], ARRAY[...]) by preceding context and skips string, dollar-quoted, backtick, and comment spans, falling back to leaving dotted names like [My.Table] un-rewritten since the grammar still splits them on the dot; inserted backtick spans are tracked so downstream un-rewriting only touches bracket-derived names and never genuine MySQL backtick identifiers (#2721). Teaches _scan_sql and the routine-recovery regex to preserve and match backtick-quoted names so bracket-named CREATE PROCEDURE/FUNCTION DDL still recovers a node after debracketing.
Worth a look
- Backtick escape not un-doubled when un-rewriting bracket identifiers —
graphify/extractors/sql.py:447· 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 — 353 functions depend on the 173 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 27 callers, 11 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
walk()— 1 callers, 9 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 353 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: 185 function(s) in the blast radius were not formally verified this run
Formal verification
Behavior changes: \_scan\_sql changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_scan\_sql behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"text":"''"\}, the old code produced \('', \[\]\) but the new code produces \(' ', \[\]\). Paste that input straight into a regression test.
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
· 1 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
| parts.append(p) | ||
| return ".".join(parts) | ||
|
|
||
|
|
||
| def extract_sql(path: Path, content: str | bytes | None = None) -> dict: |
There was a problem hiding this comment.
extract_sql()
fans out to 11 callees (efferent coupling); 27 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
A bracket span containing a literal backtick cannot be represented by rewriting to backtick form: the grammar treats the first backtick of the doubled escape this function would write as the closing delimiter rather than an escape, truncating the identifier and leaving the rest as a stray ERROR node. Also stop rewriting a PostgreSQL ARRAY constructor when whitespace separates the keyword from its bracket (ARRAY [1, 2, 3]). The preceding character check only looked one character back, which is a space in that shape rather than the keyword, so the array literal read as an identifier and got rewritten.
A bracket quoted FOREIGN KEY ... REFERENCES clause could confuse the parser badly enough that the whole child table (with its FK constraint) landed as bogus nested content inside the parent table's own subtree, dropping the child table from the graph and fabricating a self referencing EXTRACTED edge on the parent. Already fixed as a side effect of the Graphify-Labs#2712 debracketing change; this adds coverage for the specific Graphify-Labs#2713 repro. Fixes Graphify-Labs#2713.
9d3e957 to
fe3b0b8
Compare
|
Follow up on this round's bot finding (matching fe3b0b8): Confirmed real, fixed: "Backtick escape not un-doubled when un-rewriting bracket identifiers". Traced this to the grammar, not the un-rewriting side: a T-SQL bracket identifier containing a literal backtick ([Foo`Bar], a legal name since only ] needs escaping inside a bracket) gets escaped by doubling when rewritten to backtick form (`Foo``Bar`, matching MySQL's own convention), but tree_sitter_sql's grammar treats the first backtick of that doubled pair as the closing delimiter rather than an escape -- confirmed via a direct AST dump, which showed the identifier truncated to `Foo` with `Bar` left behind as a separate ERROR node. _strip_backtick_parts's own un-doubling is actually correct; it just never receives correct input, since the grammar already mangled the parse before un-rewriting gets a chance to run. Same root cause and same fix as the earlier dot-in-brackets and native-backtick-identifier findings on this stack: _debracket_tsql now also refuses to rewrite a bracket span whose content contains a literal backtick, leaving it as plain bracket text. Regression test added. |
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. 1 change(s) alter behavior, breaking input(s) attached.
Behavior changes: \_scan\_sql changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_scan\_sql behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"text":"''"\}, the old code produced \('', \[\]\) but the new code produces \(' ', \[\]\). Paste that input straight into a regression test.
Graphify review — findings
Rewrites T-SQL bracket-quoted identifiers ([dbo].[Orders]) into backtick-quoted ones before parsing via _debracket_tsql, so tree_sitter_sql handles them on its normal AST path instead of dropping them into ERROR nodes that mangled labels and lost whole tables. Confines the rewrite to spans that actually read as identifiers — leaving array markers (int[], ARRAY[...]), comment-bearing, dotted, or backtick-containing brackets un-rewritten as before — and skips single/double/backtick strings, dollar-quoted PL/pgSQL bodies, and comments so their bracket-like contents are untouched. Extends _scan_sql and the routine-recovery regex to also accept backtick-quoted names, and returns the inserted-span coordinates so a downstream reader un-rewrites only the brackets it introduced and never a genuine MySQL backtick name.
Worth a look
- _clean_regex_name byte offset recomputed via full-prefix encode each call is O(n) but not a correctness bug; overlap uses byte offsets against char-derived source which may mismatch if src_text decode replaced bytes —
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
- Bracket-quoted identifier labels changed from
[dbo].[X]todbo.X— undocumented output contract shift —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
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 357 functions depend on the 177 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 27 callers, 11 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
walk()— 1 callers, 9 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 357 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: 189 function(s) in the blast radius were not formally verified this run
Formal verification
Behavior changes: \_scan\_sql changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_scan\_sql behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"text":"''"\}, the old code produced \('', \[\]\) but the new code produces \(' ', \[\]\). Paste that input straight into a regression test.
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
· 1 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
| return ".".join(parts) | ||
|
|
||
|
|
||
| def extract_sql(path: Path, content: str | bytes | None = None) -> dict: |
There was a problem hiding this comment.
extract_sql()
fans out to 11 callees (efferent coupling); 27 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
1 participant
A bracket-quoted FOREIGN KEY ... REFERENCES clause could confuse the
parser badly enough that the whole child table (with its FK
constraint) landed as bogus nested content inside the parent table's
own subtree, dropping the child table from the graph and fabricating
a self-referencing EXTRACTED edge on the parent. Already fixed as a
side effect of the #2712 debracketing change; this adds coverage for
the specific #2713 repro.
Fixes #2713.