fix(sql/mcp): robust bracketed identifier parsing and mcp error flagging

[shard-c6]

Fixes #2712, #2713, and #2714

What the Issues Were:

1. MCP Tool Failures Not Flagged (Issue graphify-mcp: tool failures return isError unset, so a missing project_path graph is indistinguishable from success to any programmatic caller #2714): When an MCP tool failed (e.g., due to an invalid graph path), the error was returned as a standard successful text payload. This caused programmatic clients to silently fail, thinking the request succeeded.
2. Missing Nodes on Bracketed T-SQL (Issue T-SQL: a bracketed FOREIGN KEY can drop the child table entirely and emit a self-referencing edge tagged EXTRACTED #2713): A bracketed FOREIGN KEY (like [dbo].[Customer]) and T-SQL GO statements caused tree-sitter-sql to produce ERROR nodes. This catastrophic parser failure swallowed subsequent CREATE TABLE definitions, dropping nodes from the graph and generating incorrect self-referencing edges on previous tables.
3. Mangled Identifiers (Issue T-SQL: bracket-quoted identifiers keep their brackets — [dbo].[Customer] becomes the label dbo].[Customer, so no lookup by real name matches #2712): Bracket-quoted identifiers were incorrectly parsed, dropping the opening bracket but retaining the closing bracket (e.g., dbo].[Customer]), breaking reference lookups.

How They Have Been Handled:

• MCP Error Handling: Updated _on_call_tool and call_tool in graphify/serve.py to directly return a CallToolResult object with isError=True whenever an exception is caught or an unknown tool is called.
• Robust SQL Fallback Extraction: Updated graphify/extractors/sql.py to use a global regex fallback (_IDENT_PATTERN) that natively supports bracketed [identifiers], quoted "identifiers", and backticks.
• Graceful AST Recovery: The SQL extractor now identifies un-parsed CREATE TABLE and CREATE VIEW statements and correctly links references even when tree-sitter-sql's AST completely fails to parse the file due to unsupported T-SQL syntax.
• Identifier Normalization: Modified _norm_ident to proactively strip all unbalanced leading and trailing delimiters caused by tree-sitter AST drops, ensuring mangled names properly normalize to their exact underlying identifiers.

MCP Version Compatibility

This fix maintains compatibility with both mcp 1.x and 2.x by:

• Using a runtime check (hasattr(Server, "list_tools")) to detect the installed version
• mcp 1.x: Uses the @server.list_tools() decorator API. The SDK automatically wraps
  handler returns (raw list[TextContent] → ListToolsResult)
• mcp 2.x: Uses the on_list_tools=... callback API with explicit (ctx, params) -> Result
  contracts. Handlers must return result objects directly.

The call_tool function now returns CallToolResult objects (which exist in both
versions) with isError=True set on failures. This works correctly in both:

• 1.x: The SDK wraps the result in a CallToolResult wrapper (no-op)
• 2.x: The result is returned directly to the client

No version-specific branching is needed for CallToolResult construction.

[graphify-labs]

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: \_norm\_ident changes behavior, here is the input that shows it.

The verifier found a concrete input on which \_norm\_ident 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 \{"name":"'\\"'"\}, the old code produced '"' but the new code produces ''. Paste that input straight into a regression test.

---

Graphify review — findings

This PR touches two areas: 1. graphify/extractors/sql.py: Refactors SQL identifier handling by introducing a shared _IDENT_PATTERN regex and replacing several inline identifier-matching patterns (in function/procedure, table reference, and REFERENCES extraction) with it. It also simplifies quote/bracket stripping in _norm_ident, adds a regex-based name extraction path in _obj_name, and adds fallback blocks that scan for CREATE TABLE/VIEW statements when the parse tree has errors. 2. graphify/serve.py: Changes the MCP call_tool handler to return a types.CallToolResult directly instead of a list of TextContent, setting isError=True on the unknown-tool and exception paths, and updates the _on_call_tool wrapper to pass that result through. The surface area is SQL entity/relationship extraction logic and the MCP tool-call response shaping.

Worth a look

• _norm_ident strips quote/bracket chars from anywhere in start/end, corrupting identifiers with legitimate leading/trailing brackets — graphify/extractors/sql.py:24 · 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 — 477 functions depend on the 128 functions this change touches.

Health — grade A; 10 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):

• dispatch_command() — 2 callers, 117 callees (high)
• _query_graph_text() — 18 callers, 8 callees (high)
• _score_query() — 15 callers, 5 callees (high)
• extract_sql() — 9 callers, 8 callees (high)
• _query_terms() — 17 callers, 3 callees (high)
• run_benchmark() — 16 callers, 3 callees (high)
• _build_server() — 2 callers, 16 callees (high)
• _load_graph() — 9 callers, 3 callees (medium)
• …and 2 more

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

Formal verification

Behavior changes: \_norm\_ident changes behavior, here is the input that shows it.

The verifier found a concrete input on which \_norm\_ident 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 \{"name":"'\\"'"\}, 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

Could not verify: Could not verify \_build\_server.

The verifier did not have enough to check \_build\_server, 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: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly ImportError — names the real obstacle, not a sampling gap)

[shard-c6]

adding to the previous summary ..

MCP Version Compatibility

This fix maintains compatibility with both mcp 1.x and 2.x by:

• Using a runtime check (hasattr(Server, "list_tools")) to detect the installed version
• mcp 1.x: Uses the @server.list_tools() decorator API. The SDK automatically wraps
  handler returns (raw list[TextContent] → ListToolsResult)
• mcp 2.x: Uses the on_list_tools=... callback API with explicit (ctx, params) -> Result
  contracts. Handlers must return result objects directly.

The call_tool function now returns CallToolResult objects (which exist in both
versions) with isError=True set on failures. This works correctly in both:

• 1.x: The SDK wraps the result in a CallToolResult wrapper (no-op)
• 2.x: The result is returned directly to the client

No version-specific branching is needed for CallToolResult construction.

Enhanced Diff:

--- a/graphify/serve.py
+++ b/graphify/serve.py
@@ -1513,8 +1513,15 @@ def _build_server(graph_path: str):
 
     # NOTE: no decorators here — the handlers below are plain coroutines,
     # bound to the Server at the END of this function in a version-aware way:
-    # mcp 1.x exposes the @server.list_tools()/... decorator API, mcp 2.x
-    # replaced it with on_list_tools=/... constructor callbacks.
+    #   - mcp 1.x exposes @server.list_tools()/call_tool()/... decorators.
+    #     The SDK automatically wraps handler returns (str -> TextContent,
+    #     list[TextContent] -> CallToolResult, etc).
+    #   - mcp 2.x replaced decorators with on_list_tools=/on_call_tool=/... 
+    #     constructor callbacks using the (ctx, params) -> Result contract.
+    #     Handlers must construct and return result objects directly.
+    # The version check uses hasattr(Server, "list_tools") to detect which
+    # API is available at runtime, allowing a single codebase to support both.
+    # See lines 1995 (1.x) and 2003 (2.x) for the registration branches.
     async def list_tools() -> list[types.Tool]:
         _tools = [
             types.Tool(
@@ -1973,6 +1980,9 @@ def _build_server(graph_path: str):
         raise ValueError(f"Unknown resource: {uri_str}")
 
     async def call_tool(name: str, arguments: dict) -> types.CallToolResult:
+        # Both mcp 1.x and 2.x define CallToolResult, so isError=True
+        # can be set regardless of version. This unified approach avoids
+        # per-version branching in the implementation.
         arguments = dict(arguments or {})
         project_path = arguments.pop("project_path", None)
         handler = _handlers.get(name)

[graphify-labs]

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: \_norm\_ident changes behavior, here is the input that shows it.

The verifier found a concrete input on which \_norm\_ident 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 \{"name":"'\\"'"\}, the old code produced '"' but the new code produces ''. Paste that input straight into a regression test.

---

Graphify review — findings

This PR touches two areas: SQL extractor (graphify/extractors/sql.py): Introduces a shared _IDENT_PATTERN regex for SQL identifiers (handling quoted/bracketed/backtick names with schema qualification) and uses it across the various fallback regexes that were previously narrower ([\w$]+). It also broadens _obj_name to derive object names from a CREATE/ALTER regex and adds regex-based fallback collection of tables/views when the tree-sitter parse contains errors. MCP server (graphify/serve.py): Changes call_tool to return a types.CallToolResult directly (setting isError=True on unknown-tool and exception paths) instead of returning a bare list of TextContent, and updates the mcp 2.x callback branch to pass that result through unchanged. Also expands the explanatory comment about the 1.x vs 2.x registration APIs. Surface area is the SQL identifier-matching logic and error-node fallbacks, plus the tool-call result shape and error signaling in the server.

No blocking issues surfaced. 7 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 477 functions depend on the 128 functions this change touches.

Health — grade A; 10 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):

• dispatch_command() — 2 callers, 117 callees (high)
• _query_graph_text() — 18 callers, 8 callees (high)
• _score_query() — 15 callers, 5 callees (high)
• extract_sql() — 9 callers, 8 callees (high)
• _query_terms() — 17 callers, 3 callees (high)
• run_benchmark() — 16 callers, 3 callees (high)
• _build_server() — 2 callers, 16 callees (high)
• _load_graph() — 9 callers, 3 callees (medium)
• …and 2 more

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

Formal verification

Behavior changes: \_norm\_ident changes behavior, here is the input that shows it.

The verifier found a concrete input on which \_norm\_ident 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 \{"name":"'\\"'"\}, 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

Could not verify: Could not verify \_build\_server.

The verifier did not have enough to check \_build\_server, 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: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly ImportError — names the real obstacle, not a sampling gap)

[shard-c6]

Graphify Review Fix: _norm_ident delimiter stripping

The Graphify bot correctly identified an edge case where the updated _norm_ident function corrupted identifiers consisting entirely of delimiters (e.g., stripping multiple delimiters from ends because of the + quantifier in the regex).

The issue has been fixed by replacing the greedy + regex substitution with two precise re.sub calls that strip exactly one unbalanced leading/trailing delimiter, successfully retaining valid nested quotes while continuing to correctly handle the missing opening bracket issue (dbo].[Customer]).

Enhanced Diff:

--- a/graphify/extractors/sql.py
+++ b/graphify/extractors/sql.py
@@ -17,10 +17,16 @@ def _norm_ident(name: str) -> str:
     `table_nids` keys and lookups — node ids and display labels keep the
     original text.
     """
+    if not name:
+        return name
+    # 2712: tree-sitter AST drops the opening bracket on syntax errors but retains the
+    # closing bracket (e.g. `dbo].[Customer]`). We must strip all leading/trailing
+    # delimiter characters regardless of whether they are balanced, otherwise references break.
     parts = []
     for part in name.split("."):
         p = part.strip()
-        p = re.sub(r'^["`\[]+|["`\]]+$', '', p)
+        p = re.sub(r'^["`\[]', '', p)
+        p = re.sub(r'["`\]]$', '', p)
         parts.append(p.lower())
     return ".".join(parts)

[graphify-labs]

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: \_norm\_ident changes behavior, here is the input that shows it.

The verifier found a concrete input on which \_norm\_ident 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 \{"name":"'\\"'"\}, the old code produced '"' but the new code produces ''. Paste that input straight into a regression test.

---

Graphify review — findings

This PR modifies SQL extraction and MCP server tool handling. In graphify/extractors/sql.py, it introduces a shared _IDENT_PATTERN regex used across identifier matching, rewrites _norm_ident to strip delimiter characters via regex rather than only balanced pairs, and adds fallback CREATE TABLE/VIEW regex passes that run when the tree-sitter parse contains errors. In graphify/serve.py, it changes call_tool to return types.CallToolResult objects directly (setting isError on failure) and updates the mcp 2.x registration branch accordingly, plus expands explanatory comments. The uv.lock file also has adjustments to dependency version markers.

No blocking issues surfaced. 3 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 477 functions depend on the 128 functions this change touches.

Health — grade A; 10 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):

• dispatch_command() — 2 callers, 117 callees (high)
• _query_graph_text() — 18 callers, 8 callees (high)
• _score_query() — 15 callers, 5 callees (high)
• extract_sql() — 9 callers, 8 callees (high)
• _query_terms() — 17 callers, 3 callees (high)
• run_benchmark() — 16 callers, 3 callees (high)
• _build_server() — 2 callers, 16 callees (high)
• _load_graph() — 9 callers, 3 callees (medium)
• …and 2 more

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

Formal verification

Behavior changes: \_norm\_ident changes behavior, here is the input that shows it.

The verifier found a concrete input on which \_norm\_ident 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 \{"name":"'\\"'"\}, 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

Could not verify: Could not verify \_build\_server.

The verifier did not have enough to check \_build\_server, 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: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly ImportError — names the real obstacle, not a sampling gap)

[shard-c6]

Hi Maintainers! Please review the PR.