feat(mcp): add common DDL and SQL formatting utilities - #965
Conversation
📝 WalkthroughWalkthroughThe MCP server now validates canonical workspace paths, resolves inline and workspace DDL sources, formats explicit generated SQL artifacts, exposes typed input errors, and type-checks tests before running Vitest. The core package also exposes strict SQL formatter-option parsing. ChangesMCP input processing and formatter validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant resolveSqlFormatting
participant resolveWorkspacePath
participant parseSqlFormatterOptions
participant formatGeneratedSqlArtifact
Caller->>resolveSqlFormatting: formatter input
resolveSqlFormatting->>resolveWorkspacePath: workspace-relative JSON config path
resolveSqlFormatting->>parseSqlFormatterOptions: merged formatter options
resolveSqlFormatting-->>Caller: resolved formatter options
Caller->>formatGeneratedSqlArtifact: generated SQL artifact
formatGeneratedSqlArtifact-->>Caller: formatted artifact or McpInputError
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mcp-server/src/sqlFormatting.ts`:
- Around line 161-170: Validate formatter option keys as own properties before
retrieving validators in the formatter-options validation loop of
sqlFormatting.ts, so toString, constructor, and __proto__ produce
FORMAT_OPTION_UNKNOWN without errors. Add tests covering all three unknown keys
in packages/mcp-server/tests/commonIo.test.ts at lines 244-256.
In `@packages/mcp-server/src/workspacePaths.ts`:
- Around line 53-74: Update resolveWorkspacePath in
packages/mcp-server/src/workspacePaths.ts lines 53-74 to return a stable
validated file descriptor or equivalent identifier instead of only a pathname.
Update packages/mcp-server/src/ddlSources.ts lines 79-85 and
packages/mcp-server/src/sqlFormatting.ts lines 143-149 to perform size checks
and reads through that same identifier, avoiding subsequent pathname-based
statSync, readFileSync, or JSON reads.
In `@packages/mcp-server/tsconfig.tests.json`:
- Line 9: Update the include configuration in tsconfig.tests.json to cover all
Vitest test files by specifying tests/**/*.test.ts, ensuring files such as
tests/server.test.ts are included in type checking while preserving the existing
source inclusion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79bcccbe-e1f6-4fc2-b825-03709a14e22f
📒 Files selected for processing (8)
packages/mcp-server/package.jsonpackages/mcp-server/src/ddlSources.tspackages/mcp-server/src/inputError.tspackages/mcp-server/src/server.tspackages/mcp-server/src/sqlFormatting.tspackages/mcp-server/src/workspacePaths.tspackages/mcp-server/tests/commonIo.test.tspackages/mcp-server/tsconfig.tests.json
Summary
Verification
Merge Readiness
Tracking issue: not needed; no baseline exception requested.
Scoped checks run: not needed; no baseline exception requested.
Why full baseline is not required: full baseline exception path not used for this PR.
Self Review
Self-review workflow: Two-cycle self-review skill after scoped and full-workspace verification.
Self-review result: No scoped merge blockers remain; the acknowledged pathname TOCTOU hardening is intentionally deferred per the review-fix scope.
Concept-review workflow: Reviewed root and core AGENTS.md/DESIGN.md, core public API ownership, Changeset policy, and MCP package boundary.
Concept-review result: Formatter runtime validation now has one core authority; no package-boundary or existing MCP I/O violation remains.
CLI Surface Migration
No-migration rationale: not selected for this PR.
Upgrade note: not selected for this PR.
Deprecation/removal plan or issue: not selected for this PR.
Docs/help/examples updated: not selected for this PR.
Release/changeset wording: not selected for this PR.
Scaffold Contract Proof
No-proof rationale: not selected for this PR.
Non-edit assertion: not selected for this PR.
Fail-fast input-contract proof: not selected for this PR.
Generated-output viability proof: not selected for this PR.
Summary by CodeRabbit
.sqldiscovery.