Fix T-SQL bracket-quoted identifiers producing mangled labels - #2723
Fix T-SQL bracket-quoted identifiers producing mangled labels#2723ayushcodes10 wants to merge 13 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 PR adds T-SQL bracket-identifier handling to the SQL extractor in graphify/extractors/sql.py. It introduces two helpers — _debracket_tsql, which rewrites [bracket]-quoted identifiers into backtick-quoted ones before parsing (skipping strings/comments and array-type syntax), and _strip_backtick_parts, which strips that synthetic quoting back out for display labels — plus wiring in extract_sql (a debracketed flag, _clean_name/_ident wrappers) so object references and names are read through the cleaning path. The extractor is only debracketed when the source contains no existing backtick. The test file tests/test_multilang.py adds several new SQL tests covering clean bracket labels, foreign-key reference resolution by clean name, and array-type non-corruption. The changed-symbols list also references many other multilang tests (Go, Rust, TS) and rationale entries, though the shown diff centers on SQL extraction and its new tests.
No blocking issues surfaced. 5 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 269 functions depend on the 115 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract_sql()— 12 callers, 9 callees - worse:
walk()— 1 callers, 9 callees
Verification — 269 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: 126 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).
|
@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.
0c8b40e to
f821303
Compare
|
Rebased onto current v8. Upstream landed its own TSQL routine recovery (#3164) in the meantime, which shares the same ERROR node masked scan approach this PR relies on for the bracket rewrite to stay compatible. Reworked the debracketing so it composes with that instead of duplicating it: the shared routine recovery regex and its masking scan now also recognize the backtick form a bracketed name becomes once rewritten, so a bracketed routine keeps recovering (this PR's job is the clean label, not the recovery itself). Verified against the original #2712 repro plus the full existing TSQL test suite, all green. |
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]) to backtick-quoted form via _debracket_tsql before parsing, so extract_sql gets a clean AST through the grammar's existing MySQL-dialect path instead of dropping the statement into error recovery — fixing mangled labels and tables lost behind broken foreign keys. _clean_name/_strip_backtick_parts undo the rewrite for display labels and recovered names, but only when the file was actually debracketed, leaving genuine MySQL backtick names alone. Bracket spans that read as array markers (int[], ARRAY[1,2,3]), are empty, purely numeric, or contain a comment opener are left untouched, and both _scan_sql and the _ROUTINE_RECOVERY_RX recovery scan now accept backtick-delimited identifiers so a bracketed routine name still recovers to a node in an error-bearing file.
Worth a look
- Bracket-quoted T-SQL routine labels and IDs changed in extract_sql output —
graphify/extractors/sql.py:829· 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 — 339 functions depend on the 159 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 24 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 — 339 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: 171 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); 24 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.
|
Investigated the bot's findings and fixed a real one: _clean_name (used to un-rewrite a debracketed name for display) was gated on a single file wide flag, so once ANY bracket span in a file got rewritten, every backtick-quoted name in that same file had its backticks stripped, including a genuinely backtick-quoted MySQL name completely unrelated to the T-SQL bracket span that triggered the rewrite. Reworked it to track the actual byte range spans _debracket_tsql rewrote and only un-rewrite a name that overlaps one of them, verified against a mixed T-SQL bracket plus genuine MySQL backtick file (now keeps both correctly). Also checked the other two lower confidence findings on this PR:
|
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
Adds a source-rewriting pass to the SQL extractor that converts T-SQL bracket-quoted identifiers ([dbo].[Orders]) into backtick-quoted ones before parsing, so tree_sitter_sql takes its normal AST path instead of dropping whole statements into ERROR recovery — fixing mangled labels and tables lost behind broken foreign keys. _debracket_tsql rewrites only bracket spans that read as identifiers (leaving array markers like int[], empty/numeric content, and comment-bearing spans alone) and records the inserted spans, and _clean_name un-rewrites a name only when its byte range overlaps one of those spans, so a genuine MySQL backtick identifier elsewhere in the same file keeps its backticks. Also extends routine-recovery scanning and _scan_sql to recognize backtick-delimited identifiers, so a bracket-named CREATE PROCEDURE in an error-bearing file still recovers a node rather than vanishing.
Worth a look
- Debracketing rewrites bracket identifiers inside dollar-quoted PL/pgSQL bodies —
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
- Regex table recovery skips debracketed T-SQL identifiers —
graphify/extractors/sql.py:820· 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 — 343 functions depend on the 163 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 — 343 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: 175 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); 25 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.
|
Investigated the latest bot findings on this PR (matching 38259c8). Confirmed real, fixed in d8e6939:
Both fixes have regression tests and are propagated through the whole stack (#2723/#2722/#2721/#2724). Already covered by an earlier comment on this PR: the "numeric bracket identifiers not debracketed" finding (accepted, documented trade off) and the reproduced _scan_sql behavior claim (verified as a tooling artifact against unmodified upstream). |
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 2 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] into backtick-quoted names via _debracket_tsql before parsing, so statements that previously fell into tree_sitter_sql's error recovery (mangling labels and dropping whole tables behind broken foreign keys) now take the normal AST path. Records the byte spans it rewrote and un-rewrites names only where they overlap those spans (_clean_name/_strip_backtick_parts), so a genuine MySQL backtick name elsewhere in the same file keeps its backticks; bracket spans that read as array markers, contain a dot, embed a comment opener, or are empty/numeric are left un-rewritten as before. Extends _scan_sql and the routine-recovery regex to accept backtick-delimited identifiers so a bracket-named routine in an error-bearing file still recovers to a node instead of vanishing.
Worth a look
- Existing backtick-quoted identifiers are rewritten when they contain brackets —
graphify/extractors/sql.py:390· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- PostgreSQL ARRAY literals with whitespace are rewritten as identifiers —
graphify/extractors/sql.py:390· 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 — 347 functions depend on the 167 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 — 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: 179 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).
| 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.
|
Follow up on the two remaining findings from the last bot review (matching 38259c8): Confirmed real, fixed in ab1f115: a PostgreSQL dollar quoted PL/pgSQL body ($$ ... $$ or Investigated and found to be pre existing, out of scope for this PR: the regex table recovery finding (sql.py around line 820, the Firebird COMPUTED BY fallback at |
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
Adds T-SQL bracket-identifier support to the SQL extractor: _debracket_tsql rewrites [dbo].[Orders]-style quoted names to backtick-quoted ones before parsing so the grammar takes its normal AST path instead of dropping statements into ERROR recovery (fixing mangled labels and tables lost behind broken foreign keys), while carefully leaving array markers (int[], ARRAY[...]), dotted names it can't round-trip, and content inside strings, comments, and dollar-quoted bodies untouched. Tracks the byte spans it inserted so _strip_backtick_parts un-rewrites only those identifiers for display labels and recovered names, never a genuine MySQL backtick name elsewhere in the file. Extends _scan_sql and the routine-recovery regex to treat backtick-delimited identifiers the same as double-quoted and bracketed ones, so a bracket-named routine in an error-bearing file still recovers a node rather than vanishing.
Worth a look
- extract_sql changes public node labels/ids for T-SQL bracket identifiers —
graphify/extractors/sql.py:506· 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()— 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 — 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
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); 26 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.
|
Follow up on this round's bot finding (matching aadd5f6): Confirmed real, fixed: a genuine MySQL backtick-quoted identifier that happens to contain bracket-like text (like `[weird]name`) had no dedicated scan branch in _debracket_tsql at all, so its content was walked character by character like ordinary source. The embedded [weird] read as a real T-SQL bracket identifier and got rewritten right there inside the existing backtick span, producing doubled and orphaned backticks (`[weird]name` -> |
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
Adds T-SQL bracket-quoted identifier support to the SQL extractor by rewriting [dbo].[Orders] to backtick-quoted names before parsing (_debracket_tsql), so statements that previously fell into tree_sitter_sql ERROR recovery — mangling labels and dropping whole tables behind broken foreign keys — now take the normal AST path. The rewrite is span-tracked so a downstream reader un-rewrites only the identifiers that came from a bracket, leaving genuine MySQL backtick names untouched, and skips string/dollar-quoted/comment/backtick regions to avoid corrupting their contents; bracket spans that read as array markers, or that hold a dot, comment opener, or purely numeric content, are left as plain brackets. Extends _scan_sql and the routine-recovery regex to also honor backtick-delimited names so a bracketed CREATE PROCEDURE in an error-bearing file still recovers a node instead of vanishing.
Worth a look
- Debracketing rewrites valid ARRAY constructors when whitespace precedes '[' —
graphify/extractors/sql.py:419· 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()— 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 — 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).
| 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.
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.
|
Follow up on this round's bot finding (matching d638043): Confirmed real, fixed: "Debracketing rewrites valid ARRAY constructors when whitespace precedes '['". PostgreSQL allows a space between ARRAY and its bracket constructor (ARRAY [1, 2, 3]); the preceding-character check that tells an array marker apart from a bracket-quoted identifier only looked at the single character right before '[', which in that shape is the space, not the ARRAY keyword. Confirmed empirically before the fix: SELECT ARRAY [1, 2, 3]; was rewritten to SELECT ARRAY `1, 2, 3`;. Fixed by looking back past a run of spaces or tabs for a bare ARRAY word before falling through to the identifier check. Regression test added, including a check that an ordinary whitespace-preceded bracket identifier (the common case) still gets rewritten. |
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
Rewrites T-SQL bracket-quoted identifiers ([dbo].[Orders]) into backtick-quoted ones before parsing via the new _debracket_tsql, so the normal tree_sitter_sql AST path handles them instead of dropping into error recovery that mangled labels and lost tables behind broken foreign keys. Skips rewriting for spans that aren't real identifiers — array markers like int[]/ARRAY[...], empty/numeric content, comment openers, dots or literal backticks the grammar can't round-trip — and steps over string, dollar-quoted, comment, and existing backtick spans so their contents aren't corrupted, falling back to the un-rewritten bracket in every excluded case. Extends _scan_sql and _ROUTINE_RECOVERY_RX to recognize backtick-delimited names too, so a bracket-named routine in an error-bearing file still recovers a node after debracketing.
No blocking issues surfaced. 12 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 355 functions depend on the 175 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 — 355 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: 187 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); 26 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
src_text decoded with errors=replace, which collapses every invalid byte to one U+FFFD that re encodes to 3 bytes, permanently inflating every offset _clean_regex_name computes past an invalid byte anywhere earlier in the file. A routine recovered only through the regex fallback then had its overlap check miss every real debracket span, so a name debracketing did touch kept its backticks in the final label. Decode with surrogateescape instead, which round trips losslessly, and sanitize a name on the way out in case its own content carried one of those escaped bytes, since a label is not meant to hold a lone surrogate.
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
Rewrites T-SQL bracket-quoted identifiers ([dbo].[Orders]) into backtick-quoted form before parsing via new _debracket_tsql, so tree_sitter_sql handles them on its normal AST path instead of dropping the surrounding statement into ERROR recovery and mangling labels or losing tables. Only genuine identifier brackets are rewritten — array markers (int[], ARRAY[…]), comment-bearing or dotted/backtick-bearing content, and text inside strings, dollar-quoted bodies, and existing backtick spans are left alone, falling back to plain un-rewritten brackets when a shape can't round-trip. Extends the routine-recovery scan and _scan_sql masking to accept backtick-quoted names too, since a bracketed routine name only survives as a backtick by the time recovery runs.
No blocking issues surfaced. 15 lower-confidence candidates did not survive cross-model 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).
Health delta baseline: last indexed commit 67f99bd (diverged from this PR's base — delta is approximate).
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.
The ARRAY keyword lookback only skipped spaces and tabs, so ARRAY followed by a newline then its bracket (SQL treats all whitespace between tokens the same way) still read as a bracket quoted identifier and got rewritten. Skip any whitespace kind, not just spaces and tabs.
|
Follow up on this round's bot finding (matching b73259a): Confirmed real, fixed: the ARRAY-keyword lookback added earlier this round only skipped spaces and tabs, so ARRAY followed by a newline then its bracket (SQL treats all whitespace between tokens the same way, so ARRAY\n[1, 2, 3] is just as valid as ARRAY [1, 2, 3]) still read as a bracket-quoted identifier and got rewritten. Reproduced empirically before the fix. Now skips any whitespace kind (space, tab, newline, carriage return), not just spaces and tabs. Regression test added, including a mixed-whitespace case. |
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 T-SQL bracket-quoted identifier support to the SQL extractor via a new _debracket_tsql pre-parse pass that rewrites [dbo].[Orders] into backtick quoting the grammar already understands, so these statements take the normal AST path instead of falling into error recovery that mangled labels and dropped tables. Distinguishes quoted identifiers from array markers (int[], ARRAY[1,2,3]), and leaves brackets un-rewritten when the content is empty, numeric, or holds a ., a backtick, or a comment opener — cases the grammar can't round-trip — while skipping over string, dollar-quoted, backtick, and comment spans so their contents aren't corrupted. Extends _scan_sql and _ROUTINE_RECOVERY_RX to treat backtick-delimited names as recoverable routine names, since a bracketed routine reaches recovery already rewritten to backticks and would otherwise yield no node at all.
No blocking issues surfaced. 12 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 359 functions depend on the 179 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 — 359 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 67f99bd (diverged from this PR's base — delta is approximate).
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: 191 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.
Unlike a string or dollar quoted span, a backtick span must not cross a newline: a name does not span lines in practice, so an unclosed backtick is far more likely a stray character than a genuine multi line identifier. Scanning past a newline for one let one accidental, unmatched backtick anywhere in the file silently swallow everything after it, disabling debracketing for the rest of the file. A backtick that does not close on its own line is now left as an ordinary character, matching how _scan_sql treats an unterminated delimited identifier.
A regex recovered name whose own content carried one of src_text's surrogate escaped invalid bytes was sanitized via encode(errors=replace).decode(), but that handler turns a lone surrogate into a bare question mark, not the U+FFFD every other invalid byte path in this module produces. Replace it explicitly instead so the placeholder character stays consistent.
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
Adds _debracket_tsql, which rewrites T-SQL bracket-quoted identifiers ([dbo].[Orders]) into backtick-quoted ones before parsing so the grammar takes its normal AST path instead of falling into error recovery, fixing mangled labels and tables silently lost behind broken foreign keys. Restricts rewriting to bracket spans that actually read as identifiers — leaving array markers (int[], ARRAY[...]), empty/numeric content, comment-openers, and dotted or backtick-bearing names as plain un-rewritten brackets (the prior fallback) since those can't round-trip — while skipping over single/double-quoted strings, dollar-quoted bodies, and genuine backtick names so their contents aren't corrupted. Extends the routine-recovery scan and _scan_sql to accept and preserve backtick-delimited identifiers, since a recovered bracketed routine name now arrives already rewritten to a backtick name.
Worth a look
- surrogateescape decode may crash when text is re-encoded for offset math —
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 — 363 functions depend on the 183 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 28 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 — 363 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 67f99bd (diverged from this PR's base — delta is approximate).
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: 195 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); 28 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
This loop scanned raw src_text directly, unlike its sibling routine/view recovery loops which scan the masked output _scan_sql already produces, and ran unconditionally rather than only on files that already failed to parse. A CREATE TABLE ... REFERENCES ... shape sitting inside a single quoted string literal, dynamic SQL text never executed as DDL, fabricated a real edge between two unrelated tables that merely share names with the string content, in any file including a completely clean one. Compute the mask unconditionally, before this loop, and reuse it for the routine/view recovery loops below instead of computing it twice.
|
Investigated this round's bot finding on this PR ("surrogateescape decode may crash when text is re-encoded for offset math") -- verified with a battery of adversarial invalid-byte inputs (500 consecutive invalid bytes, overlong encodings, raw continuation bytes, invalid bytes inside a debracketed identifier's own content) through the full extract_sql pipeline: no crash, no corruption. surrogateescape's whole design is a lossless, crash-free round trip for exactly this scenario. No code change made. Also fixed here (propagated from the base branch, now at ababa65): the Firebird CREATE TABLE/REFERENCES fallback fabricating edges from string literal content, described in detail on #2724. |
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
Adds a T-SQL bracket-quoting preprocessor _debracket_tsql that rewrites [dbo].[Orders]-style identifiers into backtick-quoted names before parsing, so tree_sitter_sql handles them on the normal AST path instead of dropping whole statements into error recovery; it skips array markers (int[], ARRAY[...]), string/dollar-quoted/backtick spans, and leaves un-rewritable shapes (embedded ., literal backticks, comment openers) as plain brackets, returning the inserted backtick spans so a downstream reader only un-rewrites bracket-derived names. Extends _scan_sql and _ROUTINE_RECOVERY_RX to accept backtick-quoted routine names during recovery, since a bracketed CREATE PROCEDURE name now arrives already rewritten to backticks. Also normalizes lone surrogates in identifier names to U+FFFD via _LONE_SURROGATE_RX to match the rest of the extractor's invalid-byte placeholder.
Worth a look
- _debracket_tsql return type changed from bytes to tuple —
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
- Firebird REFERENCES fallback matches against masked source but reads name from masked text —
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 — 367 functions depend on the 187 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 30 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 — 367 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 67f99bd (diverged from this PR's base — delta is approximate).
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: 199 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
3 of 255 test file(s) selected (1%) via static blast radius.
tests/test_extract.py— impacttests/test_multilang.py— impact, changed-testtests/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
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); 30 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
A name's own byte range could overlap a debracketed span even when only one of its dotted parts was actually touched, for example a schema debracketed alongside a genuinely backtick quoted table name in the same reference. The un rewrite step stripped every part once the whole name overlapped anywhere, so the untouched part's backticks got stripped too. Both callers now check and strip per part.
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
Adds _debracket_tsql, which rewrites T-SQL bracket-quoted identifiers ([dbo].[Orders]) to backtick-quoted form before parsing so tree_sitter_sql's normal AST path handles them instead of dropping the statement into error recovery (fixing mangled labels and tables lost behind broken foreign keys); it skips single/double-quoted strings, dollar-quoted bodies, and genuine backtick spans, and leaves brackets untouched when the content isn't a legal identifier (empty, numeric, contains a comment opener, a dot, or a literal backtick). Extends _scan_sql and _ROUTINE_RECOVERY_RX to treat backtick-delimited names the same as double-quoted and bracketed ones, since a recovered bracketed routine name now arrives as a backtick after debracketing. Returns the inserted-span coordinates so a downstream reader can un-rewrite only bracket-derived identifiers and leave real MySQL backtick names alone.
Worth a look
- Dollar-quoted span skip not implemented in _debracket_tsql despite documented behavior —
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
- _strip_backtick_parts mis-tracks byte offset for multibyte identifier parts —
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 — 369 functions depend on the 189 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 31 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 — 369 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 67f99bd (diverged from this PR's base — delta is approximate).
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: 201 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
3 of 255 test file(s) selected (1%) via static blast radius.
tests/test_extract.py— impacttests/test_multilang.py— impact, changed-testtests/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
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 b".".join(out_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); 31 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
A bracket quoted identifier containing a literal dot is left as plain bracket text since the grammar cannot represent it via backtick quoting either way. Confirmed this is a tree_sitter_sql grammar limitation independent of any rewriting: feeding a raw, untouched multi part bracket reference straight to the parser reproduces the same leak. Its own error recovery for such a span sometimes groups the opening bracket into the object_reference node while the matching one lands in a separate sibling ERROR node this reader never visits, leaking a stray character into the name. Strip an excess opener or closer unconditionally, since two dot containing parts joined by a dot can leak one even when neither part was ever debracketed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
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
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 371 functions depend on the 191 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 32 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 — 371 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 67f99bd (diverged from this PR's base — delta is approximate).
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: 203 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
3 of 255 test file(s) selected (1%) via static blast radius.
tests/test_extract.py— impacttests/test_multilang.py— impact, changed-testtests/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
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 b".".join(out_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); 32 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
|
Investigated this round's bot findings on this PR. "Dollar-quoted span skip not implemented in _debracket_tsql despite documented behavior" -- checked directly: the skip is implemented (the "_strip_backtick_parts mis-tracks byte offset for multibyte identifier parts" -- checked directly: the function works entirely in byte-space (splits raw bytes on b".", tracks byte lengths of each part), never mixing character and byte offsets, which is exactly why this class of bug can't occur in it. Verified with CJK characters, a 4-byte emoji character, multi-byte content both before and after a debracketed part in the same dotted reference, and multi-byte content combined with invalid-UTF-8-byte padding that shifts surrounding offsets -- all produced correct, uncorrupted labels through both the tree-walk (_clean_name) and regex-fallback (_clean_regex_name) code paths. No code change made. The stray-bracket-leak fix described in detail on #2721 is also fixed here, now at 54a70d4 (this is the base branch where _clean_name lives). |
1 participant
tree-sitter-sql has no grammar token for T-SQL [bracket] quoting: each bracket lands as its own ERROR node, one byte short of the real pair, so [dbo].[Alpha] reads back as the label
dbo].[Alphainstead of dbo.Alpha. Rewrite bracket-quoted identifiers to backtick-quoted ones before parsing (a form the grammar already handles cleanly), then strip the synthetic backticks back out for display. Guards against misfiring on Postgres/MySQL array-type syntax (text[], numeric(10)[3]).Fixes #2712.