Skip to content

Commit 72adfdc

Browse files
Isla-LiuIslaLiu
andauthored
fix: surface source description in MCP tool description (#267) (#309)
* fix: surface source description in MCP tool description The `description` field on TOML [[sources]] was parsed into SourceConfig and exposed via the REST /api/sources endpoint, but never surfaced through MCP tool metadata. AI clients using DBHub via MCP only saw the hardcoded template "Execute SQL queries on the '<id>' <dbType> database" regardless of how the user described the source, making it impossible for models to route requests between multiple sources based on the user's descriptions. Prepend sourceConfig.description (when set) to both execute_sql and search_objects generated tool descriptions so AI clients see the source's purpose first while retaining all existing technical context (readonly note, max_rows note, dbType, source id). Custom tool descriptions (CustomToolConfig.description) are untouched because custom tools already carry their own required description. Fixes #267 * refactor: extract buildSourceDescriptionPrefix helper and normalize input Addresses Copilot review feedback on PR #309: - Extract prefix-building logic into a pure helper `buildSourceDescriptionPrefix` used by both `getExecuteSqlMetadata` and `getSearchObjectsMetadata` instead of inlining the same expression twice. - Trim surrounding whitespace from the user-provided description so leading or trailing whitespace does not leak into the MCP tool description. - Skip appending '.' when the description already ends with a sentence-ending character (one of '.', '!', '?') to avoid double punctuation such as "Prod DB.. Execute SQL...". Adds 10 pure unit tests in src/utils/__tests__/tool-metadata.test.ts covering undefined/empty/whitespace-only inputs, each sentence-ending punctuation case, whitespace trimming, preservation of internal formatting, and documenting that mid-sentence punctuation (':', ';', ')') is still terminated with '. '. * refactor: treat trailing ':' as terminator to avoid ':.' artifact Addresses second-pass Copilot review on PR #309: descriptions ending with a colon (e.g. "Details below:") produced the awkward artifact "Details below:. Execute SQL..." under the previous regex. A trailing colon naturally introduces what follows (the generic tool template), so appending "." after it is both ugly and semantically redundant. Extend the terminator regex to /[.!?:]$/ and update the corresponding test case. ';' and ')' remain non-terminators because they signal mid-sentence continuation rather than introduction. --------- Co-authored-by: IslaLiu <C19087@csccss.com.tw>
1 parent f35144b commit 72adfdc

3 files changed

Lines changed: 172 additions & 5 deletions

File tree

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
import { describe, it, expect, beforeAll, afterAll } from "vitest";
2+
import type { ConnectorManager } from "../../connectors/manager.js";
3+
import { setupManagerWithFixture, FIXTURES, loadFixtureConfig } from "../../__fixtures__/helpers.js";
4+
import { getExecuteSqlMetadata, getSearchObjectsMetadata } from "../tool-metadata.js";
5+
import { initializeToolRegistry } from "../../tools/registry.js";
6+
7+
// Import SQLite connector to ensure it's registered
8+
import "../../connectors/sqlite/index.js";
9+
10+
describe("tool-metadata description propagation", () => {
11+
let manager: ConnectorManager;
12+
13+
beforeAll(async () => {
14+
// readonly-maxrows fixture has three sources:
15+
// readonly_limited: description = "Read-only database for safe queries", readonly + max_rows
16+
// writable_limited: no description, writable + max_rows
17+
// writable_unlimited: no description, writable + no limits
18+
manager = await setupManagerWithFixture(FIXTURES.READONLY_MAXROWS);
19+
const { sources, tools } = loadFixtureConfig(FIXTURES.READONLY_MAXROWS);
20+
initializeToolRegistry({ sources, tools: tools || [] });
21+
}, 30000);
22+
23+
afterAll(async () => {
24+
if (manager) {
25+
await manager.disconnect();
26+
}
27+
});
28+
29+
describe("getExecuteSqlMetadata", () => {
30+
it("prepends user description from source config when present", () => {
31+
const metadata = getExecuteSqlMetadata("readonly_limited");
32+
33+
expect(metadata.description).toContain("Read-only database for safe queries");
34+
// Original technical context must still be preserved
35+
expect(metadata.description).toContain("readonly_limited");
36+
expect(metadata.description).toContain("sqlite");
37+
expect(metadata.description).toContain("[READ-ONLY MODE]");
38+
// User description comes first, technical template follows
39+
expect(metadata.description.indexOf("Read-only database for safe queries")).toBeLessThan(
40+
metadata.description.indexOf("Execute SQL queries")
41+
);
42+
});
43+
44+
it("omits the prefix entirely when source has no description", () => {
45+
const metadata = getExecuteSqlMetadata("writable_limited");
46+
47+
// No extra prefix — description starts with the template
48+
expect(metadata.description.startsWith("Execute SQL queries")).toBe(true);
49+
expect(metadata.description).toContain("writable_limited");
50+
expect(metadata.description).toContain("sqlite");
51+
});
52+
});
53+
54+
describe("getSearchObjectsMetadata", () => {
55+
it("prepends user description from source config when present", () => {
56+
const metadata = getSearchObjectsMetadata("readonly_limited");
57+
58+
expect(metadata.description).toContain("Read-only database for safe queries");
59+
expect(metadata.description).toContain("readonly_limited");
60+
expect(metadata.description).toContain("sqlite");
61+
expect(metadata.description.indexOf("Read-only database for safe queries")).toBeLessThan(
62+
metadata.description.indexOf("Search and list database objects")
63+
);
64+
});
65+
66+
it("omits the prefix entirely when source has no description", () => {
67+
const metadata = getSearchObjectsMetadata("writable_unlimited");
68+
69+
expect(metadata.description.startsWith("Search and list database objects")).toBe(true);
70+
expect(metadata.description).toContain("writable_unlimited");
71+
expect(metadata.description).toContain("sqlite");
72+
});
73+
});
74+
});
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import { describe, it, expect } from "vitest";
2+
import { buildSourceDescriptionPrefix } from "../tool-metadata.js";
3+
4+
describe("buildSourceDescriptionPrefix", () => {
5+
it("returns empty string when description is undefined", () => {
6+
expect(buildSourceDescriptionPrefix(undefined)).toBe("");
7+
});
8+
9+
it("returns empty string when description is empty", () => {
10+
expect(buildSourceDescriptionPrefix("")).toBe("");
11+
});
12+
13+
it("returns empty string when description is whitespace-only", () => {
14+
expect(buildSourceDescriptionPrefix(" ")).toBe("");
15+
expect(buildSourceDescriptionPrefix("\t\n")).toBe("");
16+
});
17+
18+
it("appends '. ' when description has no sentence-ending punctuation", () => {
19+
expect(buildSourceDescriptionPrefix("Prod DB")).toBe("Prod DB. ");
20+
});
21+
22+
it("appends only a space when description already ends with a period", () => {
23+
// Guards against the classic "Prod DB.. Execute SQL..." double-period bug.
24+
expect(buildSourceDescriptionPrefix("Prod DB.")).toBe("Prod DB. ");
25+
});
26+
27+
it("appends only a space when description ends with '!'", () => {
28+
expect(buildSourceDescriptionPrefix("Production DB!")).toBe("Production DB! ");
29+
});
30+
31+
it("appends only a space when description ends with '?'", () => {
32+
expect(buildSourceDescriptionPrefix("Query me?")).toBe("Query me? ");
33+
});
34+
35+
it("appends only a space when description ends with ':'", () => {
36+
// A trailing colon naturally introduces what follows (the tool template),
37+
// so adding '.' here would produce the artifact "Details below:. Execute..."
38+
expect(buildSourceDescriptionPrefix("Details below:")).toBe("Details below: ");
39+
});
40+
41+
it("trims surrounding whitespace before assessing punctuation", () => {
42+
expect(buildSourceDescriptionPrefix(" Prod DB ")).toBe("Prod DB. ");
43+
expect(buildSourceDescriptionPrefix(" Prod DB. ")).toBe("Prod DB. ");
44+
});
45+
46+
it("preserves internal whitespace and punctuation", () => {
47+
// Internal formatting (newlines, multiple words, mid-sentence punctuation)
48+
// is the user's intent and must not be altered.
49+
expect(buildSourceDescriptionPrefix("Line 1\nLine 2")).toBe("Line 1\nLine 2. ");
50+
expect(buildSourceDescriptionPrefix("Line A, Line B")).toBe("Line A, Line B. ");
51+
});
52+
53+
it("does not treat non-sentence-ending punctuation as terminators", () => {
54+
// ')' and ';' are mid-sentence / structural punctuation; the helper
55+
// should still append ". " to produce a complete sentence boundary.
56+
// (':' is handled separately — see the colon-specific test above.)
57+
expect(buildSourceDescriptionPrefix("(read-only)")).toBe("(read-only). ");
58+
expect(buildSourceDescriptionPrefix("Clause 1; clause 2")).toBe("Clause 1; clause 2. ");
59+
});
60+
});

src/utils/tool-metadata.ts

Lines changed: 38 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,33 @@ export interface ToolMetadata {
3939
annotations: ToolAnnotations;
4040
}
4141

42+
/**
43+
* Build a prefix string for prepending a source's user-provided `description`
44+
* onto a generated tool description. Returns "" when no description is set
45+
* (undefined, empty, or whitespace-only). Normalizes surrounding whitespace
46+
* with `trim()` and skips adding a period when the description already ends
47+
* with one of "." / "!" / "?" / ":" — the colon is included because a
48+
* trailing colon naturally introduces what follows (the generic tool
49+
* template) and appending "." after it would produce artifacts like
50+
* "Details below:. Execute SQL...".
51+
*
52+
* Examples:
53+
* undefined -> ""
54+
* " " -> ""
55+
* "Prod DB" -> "Prod DB. "
56+
* "Prod DB." -> "Prod DB. " (no double period)
57+
* " Prod DB! " -> "Prod DB! " (trimmed, no added period)
58+
* "Query me?" -> "Query me? "
59+
* "Details below:" -> "Details below: " (colon introduces what follows)
60+
* "Clause 1; clause 2" -> "Clause 1; clause 2. " (semicolon is mid-sentence)
61+
* "(read-only)" -> "(read-only). " (closing paren is mid-sentence)
62+
*/
63+
export function buildSourceDescriptionPrefix(description: string | undefined): string {
64+
const trimmed = description?.trim() ?? "";
65+
if (!trimmed) return "";
66+
return /[.!?:]$/.test(trimmed) ? `${trimmed} ` : `${trimmed}. `;
67+
}
68+
4269
/**
4370
* Convert a Zod schema object to simplified parameter list
4471
* @param schema - Zod schema object (e.g., { sql: z.string().describe("...") })
@@ -106,12 +133,15 @@ export function getExecuteSqlMetadata(sourceId: string): ToolMetadata {
106133
? `Execute SQL (${dbType})`
107134
: `Execute SQL on ${sourceId} (${dbType})`;
108135

109-
// Determine description with more context
136+
// Determine description with more context.
137+
// Prepend the user-provided `description` from the source config (if set)
138+
// so AI clients reading the MCP tool list see the source's purpose first.
139+
const userDescPrefix = buildSourceDescriptionPrefix(sourceConfig.description);
110140
const readonlyNote = executeOptions.readonly ? " [READ-ONLY MODE]" : "";
111141
const maxRowsNote = executeOptions.maxRows ? ` (limited to ${executeOptions.maxRows} rows)` : "";
112142
const description = isSingleSource
113-
? `Execute SQL queries on the ${dbType} database${readonlyNote}${maxRowsNote}`
114-
: `Execute SQL queries on the '${sourceId}' ${dbType} database${readonlyNote}${maxRowsNote}`;
143+
? `${userDescPrefix}Execute SQL queries on the ${dbType} database${readonlyNote}${maxRowsNote}`
144+
: `${userDescPrefix}Execute SQL queries on the '${sourceId}' ${dbType} database${readonlyNote}${maxRowsNote}`;
115145

116146
// Build annotations object with all standard MCP hints
117147
const isReadonly = executeOptions.readonly === true;
@@ -149,9 +179,12 @@ export function getSearchObjectsMetadata(sourceId: string): { name: string; desc
149179
const title = isSingleSource
150180
? `Search Database Objects (${dbType})`
151181
: `Search Database Objects on ${sourceId} (${dbType})`;
182+
// Prepend the user-provided `description` from the source config (if set)
183+
// so AI clients reading the MCP tool list see the source's purpose first.
184+
const userDescPrefix = buildSourceDescriptionPrefix(sourceConfig.description);
152185
const description = isSingleSource
153-
? `Search and list database objects (schemas, tables, columns, procedures, functions, indexes) on the ${dbType} database`
154-
: `Search and list database objects (schemas, tables, columns, procedures, functions, indexes) on the '${sourceId}' ${dbType} database`;
186+
? `${userDescPrefix}Search and list database objects (schemas, tables, columns, procedures, functions, indexes) on the ${dbType} database`
187+
: `${userDescPrefix}Search and list database objects (schemas, tables, columns, procedures, functions, indexes) on the '${sourceId}' ${dbType} database`;
155188

156189
return {
157190
name: toolName,

0 commit comments

Comments
 (0)