refactor(sql): share transaction-wrapping and error-formatting across transports

[tianzhou]

Why

This is the first increment of the single core, dual transport idea: the ConnectRPC ExecuteSQL handler and the MCP execution tools were each carrying their own copy of the execute pipeline, and the copies had already drifted.

• Transaction wrapping was copy-pasted — query-service.ts and mcp.ts both had the same BEGIN;\n…\n;\nCOMMIT; logic and the same comment. A change to transaction-safety rules had to be made in both or they'd silently diverge.
• Error enrichment lived only on the RPC path — line context + DETAIL + HINT. MCP agents got bare err.message, i.e. strictly worse errors than the UI for the same failure.

What

Extract the two genuinely-identical pieces into server/lib/execute-sql.ts and route both transports through them:

buildExecutableSql(rawSql, analysis)  // BEGIN/COMMIT wrapping for safe multi-statement batches
formatExecutionError(err, sql)        // line/DETAIL/HINT enrichment

• query-service.ts::executeSQL — uses both helpers (behavior unchanged; it already enriched).
• mcp.ts::execute — uses buildExecutableSql; runAndAudit now throws formatExecutionError(...), so agents get the same rich errors the UI shows. Audit still records the bare message on both paths (unchanged).

Deliberately not unified: the intentional differences stay in each handler — the user vs. agent permission ladders, result/column-metadata shaping, and the RPC streaming/PID/cancellation lifecycle. Forcing those together would abstract things that aren't actually the same.

Verification

• New tests/execute-sql.test.ts — 8 tests (wrapping matrix + error formatting incl. line context, DETAIL/HINT, non-Error fallback)
• Existing tests/mcp.test.ts — 32 pass
• tsc --noEmit clean; pnpm build:server succeeds

Note

This is the safe, surgical slice. A fuller runGovernedSql core (also folding in execution + audit) is constrained by the RPC streaming generator's client lifecycle and is left for a follow-up if desired.

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
pgconsole	Ready	Preview, Comment	Jun 25, 2026 4:51pm

[Copilot]

Pull request overview

This PR extracts shared SQL execution helpers so both the ConnectRPC ExecuteSQL handler and the MCP execution tools use the same transaction-wrapping behavior and the same enriched Postgres error formatting, reducing drift between transports.

Changes:

• Added server/lib/execute-sql.ts with buildExecutableSql (BEGIN/COMMIT wrapping) and formatExecutionError (line/DETAIL/HINT enrichment).
• Updated server/services/query-service.ts and server/mcp.ts to route through the shared helpers.
• Added tests/execute-sql.test.ts covering wrapping behavior and error formatting.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
tests/execute-sql.test.ts	Adds unit coverage for shared SQL wrapping + error formatting helpers.
server/services/query-service.ts	Replaces inlined wrapping/error-enrichment with shared helper calls in the RPC handler.
server/mcp.ts	Reuses shared wrapping and adds enriched error formatting to MCP execution errors.
server/lib/execute-sql.ts	Introduces transport-agnostic helpers for executable SQL construction and error formatting.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[tianzhou]

Fixed in d20735e — both call sites now format against the executed SQL (RPC: executableSql hoisted above the try; MCP: execSql), so the Postgres position maps to the right line. Added a regression test for the transaction-wrapped case. Good catch — and you're right this also closes the pre-existing RPC mismatch, not just the one MCP newly introduced.

[tianzhou]

Fixed in d20735e — both call sites now format against the executed SQL (RPC: executableSql hoisted above the try; MCP: execSql), so the Postgres position maps to the right line. Added a regression test for the transaction-wrapped case. Good catch — and you're right this also closes the pre-existing RPC mismatch, not just the one MCP newly introduced.

[greptile-apps]

Greptile Summary

This PR extracts two duplicated pieces of the SQL execution pipeline — transaction wrapping and error enrichment — into a new shared module (server/lib/execute-sql.ts) and routes both the ConnectRPC and MCP transports through it. The MCP path gains the same rich line/DETAIL/HINT error context that the UI already provided, and future changes to wrapping logic only need to be made in one place.

• buildExecutableSql: Encapsulates the BEGIN;\ …\ ;\ COMMIT; wrapping for safe multi-statement batches; previously copy-pasted with identical comments in both transports.
• formatExecutionError: Centralizes position-to-line mapping and DETAIL/HINT appending; previously existed only in the RPC path, leaving MCP agents with bare error messages.
• Tests: 8 unit tests in tests/execute-sql.test.ts cover the wrapping matrix and all error-formatting branches.

Confidence Score: 4/5

Safe to merge — the refactor preserves all existing execution behavior and meaningfully improves MCP error quality with no risk to correctness.

Both transports now share helpers that are well-tested and faithful extractions of the original inline logic. The one noteworthy issue is that runAndAudit passes auditSql (raw, unwrapped SQL) to formatExecutionError while the Postgres error position is relative to execSql (which may be BEGIN; ${rawSql} ; COMMIT;). For wrapped multi-statement batches this produces a 7-character offset that can report the wrong line in the enriched error. The same mismatch already exists in query-service.ts, so both paths are now consistent — but MCP newly gains the potentially-imprecise line context where it previously had none.

server/mcp.ts — specifically the formatExecutionError(err, auditSql) call in runAndAudit; consider whether execSql should be passed instead for accurate position mapping.

Important Files Changed

Filename	Overview
server/lib/execute-sql.ts	New shared helper module with two well-documented, pure functions extracted from both transports; logic is faithful to the originals and correctly handles edge cases (position '0', non-Error throwables, optional chaining on null).
server/mcp.ts	Delegates SQL wrapping to buildExecutableSql and now enriches MCP errors via formatExecutionError in runAndAudit; passes auditSql (rawSql) instead of execSql to formatExecutionError, causing the position-to-line mapping to be slightly off for wrapped multi-statement batches.
server/services/query-service.ts	Replaces ~30 lines of duplicated inline logic with calls to the two shared helpers; behavior is exactly preserved, including the pre-existing use of req.sql (not the wrapped sql) for position calculations.
tests/execute-sql.test.ts	Eight focused unit tests covering the wrapping matrix and all formatExecutionError branches; the position-to-line arithmetic is correctly verified, though the combined line-context + DETAIL/HINT case is untested (minor gap, code path is trivially sequential).

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant RPC as ConnectRPC (query-service.ts)
    participant MCP as MCP Agent (mcp.ts)
    participant LIB as execute-sql.ts
    participant PG as PostgreSQL

    RPC->>LIB: buildExecutableSql(req.sql, analysis)
    MCP->>LIB: buildExecutableSql(rawSql, analysis)
    LIB-->>RPC: wrapped or plain SQL
    LIB-->>MCP: wrapped or plain SQL

    RPC->>PG: client.unsafe(wrappedSql)
    MCP->>PG: runStatement(wrappedSql)

    alt success
        PG-->>RPC: result rows
        PG-->>MCP: result rows
    else error
        PG-->>RPC: pg error (with position)
        RPC->>LIB: formatExecutionError(err, req.sql)
        LIB-->>RPC: enriched error string

        PG-->>MCP: pg error (with position)
        MCP->>LIB: formatExecutionError(err, auditSql)
        LIB-->>MCP: enriched error string
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant RPC as ConnectRPC (query-service.ts)
    participant MCP as MCP Agent (mcp.ts)
    participant LIB as execute-sql.ts
    participant PG as PostgreSQL

    RPC->>LIB: buildExecutableSql(req.sql, analysis)
    MCP->>LIB: buildExecutableSql(rawSql, analysis)
    LIB-->>RPC: wrapped or plain SQL
    LIB-->>MCP: wrapped or plain SQL

    RPC->>PG: client.unsafe(wrappedSql)
    MCP->>PG: runStatement(wrappedSql)

    alt success
        PG-->>RPC: result rows
        PG-->>MCP: result rows
    else error
        PG-->>RPC: pg error (with position)
        RPC->>LIB: formatExecutionError(err, req.sql)
        LIB-->>RPC: enriched error string

        PG-->>MCP: pg error (with position)
        MCP->>LIB: formatExecutionError(err, auditSql)
        LIB-->>MCP: enriched error string
    end

Loading

_{Reviews (1): Last reviewed commit: "refactor(sql): share transaction-wrappin..." | Re-trigger Greptile}

[greptile-apps]

Postgres position offset against wrong SQL string

formatExecutionError is documented as "sql is the statement the position refers to," but auditSql (= rawSql) is passed while execSql (= finalSql) is the string actually sent to Postgres. For a wrapped multi-statement batch, execSql is BEGIN;\n${rawSql}\n;\nCOMMIT;, so every error position reported by Postgres is shifted by 7 characters ("BEGIN;\n"). The line-extraction logic will then map that position onto rawSql, quietly landing on the wrong line whenever the error falls in the first several lines of a multi-statement transaction. query-service.ts carries the same pre-existing mismatch; this PR introduces it to the MCP path for the first time.

[tianzhou]

Fixed in d20735e — both call sites now format against the executed SQL (RPC: executableSql hoisted above the try; MCP: execSql), so the Postgres position maps to the right line. Added a regression test for the transaction-wrapped case. Good catch — and you're right this also closes the pre-existing RPC mismatch, not just the one MCP newly introduced.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[tianzhou]

Looked into this and I'm going to leave it as-is. Two reasons:

1. The suggested remedy isn't available. postgres.js builds its error solely from PG's ErrorResponse fields (parseError in connection.js → Errors.postgres); it does not attach the failing SQL text to the error object. There's no err.query to prefer over executableSql.

2. The edge is effectively unreachable and not a regression. The only other statements in this try are SELECT pg_backend_pid() (a constant — cannot carry a position) and SET search_path TO <escaped idents> (doesn't validate schema existence, won't produce a position-bearing error with quoted identifiers). The pre-existing code formatted against req.sql, which was equally unrelated to a setup-statement error — so this isn't introduced here.

Isolating the main query in its own try/catch to scope the formatting would be a real restructure of a streaming generator for a scenario that can't occur in practice, so per the repo's 'no error handling for impossible scenarios' guideline I'll skip it.