Recover bracket-quoted CREATE PROCEDURE/FUNCTION names - #2721
Recover bracket-quoted CREATE PROCEDURE/FUNCTION names#2721ayushcodes10 wants to merge 10 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-quoted identifier handling to the SQL extractor in graphify/extractors/sql.py. It introduces a _debracket_tsql preprocessing pass that rewrites [bracket]-quoted identifiers to backtick-quoted ones before parsing (guarded to skip files that already contain backticks), plus _strip_backtick_parts and helper functions (_clean_name, _ident) to strip the synthetic backticks back out when producing display names. It also factors out a shared _SQL_NAME_PART regex fragment used by the ERROR-node fallback paths and routes existing name-reading call sites through the new identifier helpers. The accompanying test file (tests/test_multilang.py) is updated with new and modified cases covering the bracket-debracketing behavior, array-type handling, and various existing multi-language extraction scenarios across SQL, Go, TypeScript, and Rust.
No blocking issues surfaced. 4 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 275 functions depend on the 121 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract_sql()— 15 callers, 10 callees - worse:
walk()— 1 callers, 10 callees
Verification — 275 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 132 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify extract\_sql.
The verifier did not have enough to check extract\_sql, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set
· 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.
077fb12 to
88bed68
Compare
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 form via _debracket_tsql before parsing, so tree-sitter takes its normal AST path instead of dropping the statement into error recovery—fixing mangled labels and foreign keys that silently lost a whole table. Distinguishes true quoted identifiers from array markers like int[] and ARRAY[1,2,3] by the preceding character, and skips empty, numeric, or comment-bearing bracket spans; recovered names and display labels are un-rewritten by _strip_backtick_parts, applied only when the file was actually debracketed. Extends the routine-recovery regex and _scan_sql to accept backtick-delimited names so a bracketed CREATE PROCEDURE in an error-bearing file still recovers.
Worth a look
- Numeric bracket-quoted identifiers are not debracketed —
graphify/extractors/sql.py:372· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- MySQL backtick identifiers now stripped even when file was debracketed for an unrelated bracket span —
graphify/extractors/sql.py:394· 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()— 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 — 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); 27 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.
88bed68 to
f24c1e0
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 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 a pre-parse pass _debracket_tsql that rewrites T-SQL bracket-quoted identifiers ([dbo].[Orders]) into backtick-quoted ones so tree_sitter_sql parses them through the normal AST path instead of dropping the statement into error recovery, fixing mangled labels and tables lost behind broken foreign keys. Distinguishes real quoted identifiers from array markers (int[], ARRAY[1,2,3]) by the preceding character and refuses to rewrite empty, numeric, or comment-bearing bracket spans. Un-rewrites names on the way out via _clean_name/_strip_backtick_parts, but only for nodes whose byte range overlaps a span it actually rewrote, so genuine MySQL backtick names elsewhere in the same file keep their backticks; the routine-recovery scan and _scan_sql now also treat backticks as delimited identifiers so bracket-named routines still recover from error-bearing files.
Worth a look
- Debracket rewrite changes byte offsets, breaking source_location line/byte contracts for callers relying on original coordinates —
graphify/extractors/sql.py:445· 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()— 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 — 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).
| 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); 28 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.
f24c1e0 to
bdacf53
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 3 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 into backtick-quoted ones via _debracket_tsql before parsing, so tree_sitter_sql handles them on the normal AST path instead of dropping tables or mangling labels through error recovery. Un-rewrites those names back for display and recovered nodes only where a node's byte range overlaps a span the rewrite actually inserted (_clean_name/_strip_backtick_parts), leaving genuine MySQL backtick names untouched. Rewriting is deliberately skipped for array markers (int[], ARRAY[...]), dotted content, comment-bearing spans, and text inside strings scanned across newlines — each falling back to the pre-existing plain-bracket behavior — and the recovery scan and mask now also treat backticks as delimited identifiers.
Worth a look
- Recovery scan can match DDL text inside backtick identifiers —
graphify/extractors/sql.py:181· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Whitespace after ARRAY lets array literals be rewritten —
graphify/extractors/sql.py:365· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Bracketed T-SQL object IDs/labels changed in extract_sql output —
graphify/extractors/sql.py:565· 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()— 29 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
· 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); 29 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
bdacf53 to
a54af82
Compare
|
Investigated the latest bot findings on this PR. Confirmed real, fixed (propagated from the base branch, now at a54af82): the dot-inside-brackets grammar limitation, the multi-line quoted-string corruption bug, and the missing dollar-quoted-body handling in _debracket_tsql — same three fixes described on #2723 and #2722, all with regression tests. Investigated and found to be a false positive: "Debracket rewrite changes byte offsets, breaking source_location line/byte contracts" (sql.py:445). source_location in this extractor is line-only ( 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 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 _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. Only bracket spans that read as identifiers are rewritten: array markers (int[], ARRAY[...]), numeric/empty content, comment openers, and dotted names like [My.Table] are left as plain brackets, since the grammar can't round-trip a dot inside a backtick span; single/double-quoted strings and PL/pgSQL dollar-quoted bodies are skipped so bracket-like text inside them isn't corrupted. The recovery scan and _scan_sql now also accept backtick-quoted names, and callers use the returned insertion spans to un-rewrite only the identifiers that came from a bracket, leaving genuine MySQL backtick names untouched.
Worth a look
- T-SQL bracketed identifiers now produce different public node labels/IDs —
graphify/extractors/sql.py:517· 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 — 355 functions depend on the 175 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 29 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).
| 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); 29 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.
a54af82 to
6615eca
Compare
|
Follow up on this round's bot finding (matching 6615eca): Confirmed real, fixed (propagated from the base branch): the same backtick-corruption bug described on #2723 and #2722 — 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, with a regression test. |
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 T-SQL bracket-identifier support to the SQL extractor by rewriting [dbo].[Orders]-style names to backtick-quoted form before parsing in _debracket_tsql, so statements that previously fell into tree_sitter_sql ERROR recovery (mangling labels and silently dropping tables behind broken foreign keys) now take the normal AST path. Rewriting is confined to spans that read as real identifiers — array markers, comment-bearing, dotted, numeric, and dollar/quote/backtick-delimited content are all left untouched, falling back to the prior plain-bracket behavior — and only inserted backtick pairs are later un-rewritten so genuine MySQL backtick names elsewhere in the file are preserved. Extends _scan_sql and _ROUTINE_RECOVERY_RX to treat backtick-delimited names as recoverable routine names so a bracketed CREATE PROCEDURE in an error-bearing file still mints a node.
Worth a look
- ARRAY constructor with whitespace is rewritten as an identifier —
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
- Non-length-preserving debracket shifts returned source locations —
graphify/extractors/sql.py:477· 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()— 29 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).
| 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); 29 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.
6615eca to
f6fcbb0
Compare
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 _debracket_tsql, so tree_sitter_sql's normal AST path handles them instead of dropping tables and mangling labels through error recovery. Skips string, comment, dollar-quoted, and existing backtick spans while rewriting, leaves array markers (int[], ARRAY[...]) and un-round-trippable content (dotted or backtick-bearing names) as plain brackets, and returns the inserted spans so a downstream reader un-rewrites only bracket-derived names and not genuine MySQL backtick identifiers. Teaches the routine-recovery scan and _scan_sql to accept and preserve backtick-quoted names too, so a bracketed CREATE PROCEDURE in an error-bearing file still recovers a node.
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
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 361 functions depend on the 181 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_sql()— 29 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 — 361 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: 193 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); 29 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.
f6fcbb0 to
d97f6c3
Compare
|
Investigated the latest bot finding on this PR. "_debracket_tsql return type changed from bytes to tuple" describes an accurate but non-actionable observation -- confirmed the one production call site (extract_sql itself) and every test call site already correctly unpack the new 2-tuple signature. Not a bug; the function is private with a single, already-updated caller. No code change made. Separately, the byte-offset fix for invalid UTF-8 bytes described on #2724 (src_text decoding with errors="replace" corrupting _clean_regex_name's offset math) is propagated here too, now at d97f6c3. |
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 MySQL-style backtick-quoted ones before parsing via a new _debracket_tsql, so tree_sitter_sql takes the normal AST path instead of dropping the statement into error recovery (fixing mangled labels and tables silently lost behind broken foreign keys); the rewrite skips array markers like int[] and ARRAY[...], single/double/backtick strings, dollar-quoted bodies, and comments, and leaves spans containing ., embedded backticks, or comment openers as plain brackets since the grammar can't round-trip them. Emits the inserted backtick spans so a downstream reader un-rewrites only bracket-derived names and never a genuine MySQL identifier elsewhere in the file. Extends _scan_sql and the routine-recovery regex to preserve and match backtick-delimited names, so a bracketed CREATE PROCEDURE in an ERROR-bearing file still recovers a node rather than vanishing.
Worth a look
- ARRAY subscript detection ignores whitespace between ARRAY and bracket —
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()— 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 — 363 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: 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.
The verifier did not have enough to check extract, 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 `cache_root` is annotated `Path | None` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_normalize\_ts\_import\_types (not a proof).
The verifier ran both versions of \_normalize\_ts\_import\_types on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify \_resolve\_rescued\_specifier.
The verifier did not have enough to check \_resolve\_rescued\_specifier, 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
Could not verify: Could not verify extract\_dart.
The verifier did not have enough to check extract\_dart, 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
Could not verify: Could not verify \_extract\_generic.
The verifier did not have enough to check \_extract\_generic, 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
Could not verify: Could not verify \_resolve\_js\_module\_path.
The verifier did not have enough to check \_resolve\_js\_module\_path, 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 `start_dir` is annotated `Path | None` — outside the synthesizable primitive/collection set
Could not verify: Could not verify extract\_rust.
The verifier did not have enough to check extract\_rust, 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
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
Could not verify: Could not verify \_atomic\_replace.
The verifier did not have enough to check \_atomic\_replace, 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 `'str | Path'` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_query\_graph\_text (not a proof).
The verifier ran both versions of \_query\_graph\_text on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 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.
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.
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.
This grammar has no rule at all for TSQL's AS BEGIN...END routine body, bracketed name or not, so every CREATE PROCEDURE/FUNCTION only ever gets extracted through the ERROR node regex fallback. That fallback matched a bare or double quoted name but not a bracketed one, so a bracketed routine name produced no node at all rather than a mangled one (0 of 845 procedures on a real SSMS scripted dump). Already fixed as a required part of Graphify-Labs#2712's rewrite: rewriting a bracketed name to a backtick quoted one before parsing means the fallback has to recognize that form too, or it would regress a bracketed routine from a recovered (if ugly) node to no node at all. This adds dedicated coverage for the Graphify-Labs#2718 repro, schema qualified and bare. Fixes Graphify-Labs#2718.
d97f6c3 to
caa1c0f
Compare
|
Investigated this round's bot finding, matching what I already fixed and pushed just before this review ran (matching an earlier commit): "ARRAY subscript detection ignores whitespace between ARRAY and bracket" is the same gap as the newline-specific finding on #2723, now fixed and propagated here at caa1c0f -- the lookback now skips any whitespace kind (space, tab, newline, carriage return), not just spaces and tabs. |
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]) into backtick-quoted ones before parsing so tree_sitter_sql takes its normal AST path instead of falling into ERROR recovery; it skips string, dollar-quoted, comment, and genuine backtick spans, and returns the byte spans it inserted so a downstream reader un-rewrites only the identifiers that came from a bracket. Distinguishes a quoted identifier from an array marker (int[], ARRAY[…], including ARRAY followed by whitespace/newline) by the preceding character, and leaves brackets untouched when the content is empty, numeric, holds a comment opener, or holds a . or backtick that the grammar can't round-trip. Extends the routine-recovery regex and _scan_sql to accept backtick-delimited names so a bracket-named CREATE PROCEDURE/FUNCTION in an error-bearing file still recovers a node after debracketing.
Worth a look
- Backtick identifier scan does not stop at newline, unlike _scan_sql —
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
- Invalid UTF-8 bytes in recovered routine names now become '?' instead of U+FFFD —
graphify/extractors/sql.py:920· 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 — 365 functions depend on the 185 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 — 365 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: 197 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.
The verifier did not have enough to check extract, 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 `cache_root` is annotated `Path | None` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_normalize\_ts\_import\_types (not a proof).
The verifier ran both versions of \_normalize\_ts\_import\_types on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify \_resolve\_rescued\_specifier.
The verifier did not have enough to check \_resolve\_rescued\_specifier, 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
Could not verify: Could not verify extract\_dart.
The verifier did not have enough to check extract\_dart, 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
Could not verify: Could not verify \_extract\_generic.
The verifier did not have enough to check \_extract\_generic, 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
Could not verify: Could not verify \_resolve\_js\_module\_path.
The verifier did not have enough to check \_resolve\_js\_module\_path, 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 `start_dir` is annotated `Path | None` — outside the synthesizable primitive/collection set
Could not verify: Could not verify extract\_rust.
The verifier did not have enough to check extract\_rust, 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
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
Could not verify: Could not verify \_atomic\_replace.
The verifier did not have enough to check \_atomic\_replace, 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 `'str | Path'` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_query\_graph\_text (not a proof).
The verifier ran both versions of \_query\_graph\_text on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 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.
1 participant
This grammar has no rule at all for T-SQL's AS BEGIN...END routine
body, bracketed name or not, so every CREATE PROCEDURE/FUNCTION only
ever gets extracted through the ERROR-node regex fallback. That
fallback matched a bare or double-quoted name but not a bracket-quoted
one, so a bracketed routine name produced no node at all rather than a
mangled one (0 of 845 procedures on a real SSMS-scripted dump).
Extends the fallback's name pattern to also match a backtick-quoted
name, which recovers a bracketed T-SQL name once #2712's
debracketing has rewritten it.
Fixes #2718.