hanahmily commented on code in PR #1201: URL: https://github.com/apache/skywalking-banyandb/pull/1201#discussion_r3519497498
########## mcp/src/query/parse-validator.ts: ########## @@ -0,0 +1,148 @@ +/** + * Licensed to Apache Software Foundation (ASF) under one or more contributor + * license agreements. See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. Apache Software + * Foundation (ASF) licenses this file to you under the Apache License, + * Version 2.0 (the "License"); you may not use this file except in + * compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { spawn } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const validatorTimeoutMs = 10000; +const validatorBinaryName = process.platform === 'win32' ? 'bydbql-parse.exe' : 'bydbql-parse'; + +export type BydbQLParseValidationResult = { + valid: boolean; + message: string; + queryType?: string; + syntaxOnly: boolean; + warnings?: string[]; +}; + +type ValidatorCommand = { + command: string; + args: string[]; + cwd: string; +}; + +function mcpRootDir(): string { + const modulePath = fileURLToPath(import.meta.url); + return path.resolve(path.dirname(modulePath), '..', '..'); +} + +function validatorCommands(rootDir: string): ValidatorCommand[] { Review Comment: **[major] `validate_bydbql` shells out to `go run` at query time — the Go toolchain becomes a runtime dependency.** If `tools/bin/bydbql-parse` isn't prebuilt, every validation call runs `go run ./tools/bydbql-parse`, which (a) requires a Go toolchain in the plugin's *runtime* environment and (b) does a full compile — easily exceeding the 10s timeout on a cold cache, producing a spurious "validation timed out". Could we build the binary at install time (nothing in `.claude-plugin/plugin.json` / `.codex-plugin/plugin.json` currently runs `build:validator`) and have the fallback fail with an actionable message ("run `npm run build:validator`") instead of silently invoking `go run`? The same class of issue affects `.mcp.json` pointing at `./mcp/dist/index.js` while `dist/` is git-ignored — the server won't start unless `tsc` build ran first. ########## skills/bydbql/SKILL.md: ########## @@ -0,0 +1,37 @@ +--- +name: bydbql +description: Generate, validate, and optionally execute read-only BanyanDB BydbQL for STREAM, MEASURE, TRACE, and PROPERTY resources. Use when the user asks to query BanyanDB, translate natural language to BydbQL, inspect BanyanDB schema or data, validate BydbQL, or fetch raw BanyanDB records. +--- + +# BanyanDB BydbQL + +Use this skill when the user asks to query BanyanDB, generate BydbQL, translate natural language to BydbQL, inspect BanyanDB data, validate BydbQL, or run read-only BanyanDB queries. + +## Workflow + +1. Read `references/safety.md` before generating, validating, or executing BydbQL. +2. Read `references/syntax.md` when query syntax is needed. +3. Read `references/examples.md` when mapping natural language to BydbQL. +4. Identify the BanyanDB resource type: `STREAM`, `MEASURE`, `TRACE`, or `PROPERTY`. +5. If the group, resource name, or resource type is missing or ambiguous, use `list_groups_schemas`. +6. Generate exactly one read-only BydbQL statement. +7. Call `validate_bydbql` before execution. +8. Execute with `list_resources_bydbql` only when the user asks to run, query, fetch, show, or inspect raw BanyanDB data. +9. If `validate_bydbql` returns `valid=false`, fix the query and validate again before any execution. +10. If execution fails due to a missing group, resource, or schema, use `list_groups_schemas` to discover the correct names and retry only when the correction is clear. + +## Tool Use + +Use the existing BanyanDB MCP tools: + +- `list_groups_schemas`: discover groups and resource names for streams, measures, traces, and properties. +- `validate_bydbql`: run read-only safety checks and parse-only BydbQL syntax validation. +- `list_resources_bydbql`: execute a validated BydbQL statement when data retrieval is requested. + +The Codex host model should perform natural language to BydbQL translation using these references and live schema discovery. Use `get_generate_bydbql_prompt` only when its live schema prompt text would materially help. Review Comment: **[major] Demoting `get_generate_bydbql_prompt` drops the only index-rule `ORDER BY` safety.** That prompt (`mcp/src/query/llm-prompt.ts`) is the only component that injects the indexed-field list and enforces field-similarity substitution/omission for `ORDER BY`. `validate_bydbql` is parse-only, so a host model following this skill can emit `ORDER BY <non-indexed-field>` that validates `true` and then fails at execution. Recommend promoting `get_generate_bydbql_prompt` to a normal step whenever ranking/sorting is involved (or whenever schema was already fetched), or otherwise carrying the index-rule check into the skill flow. ########## .mcp.json: ########## @@ -0,0 +1,15 @@ +{ + "mcpServers": { + "banyandb": { Review Comment: **[minor] Assumes install location.** `cwd: "."` + `args: ["./mcp/dist/index.js"]` assume the plugin is loaded from the repo root. Can we confirm the plugin loader resolves this relative to the plugin dir in a real install (not just the repo checkout)? ########## skills/bydbql/references/syntax.md: ########## @@ -0,0 +1,374 @@ +# BydbQL Syntax Reference + +Use this reference to generate one read-only BanyanDB Query Language statement from natural language. Keep dynamic names such as groups, resource names, tags, fields, and sortable fields aligned with live BanyanDB schema discovered through MCP. + +## Core Rules + +- Statement forms: `SELECT ... FROM ...` or `SHOW TOP ... FROM MEASURE ...`. +- Resource types: `STREAM`, `MEASURE`, `TRACE`, `PROPERTY`. +- `FROM <RESOURCE_TYPE> <resource_name> IN <group>[, <group>...]` is required. +- Parentheses around group lists are optional: `IN default, staging` and `IN (default, staging)` are both valid. +- `TIME` is required for `STREAM`, `MEASURE`, `TRACE`, and `SHOW TOP` queries. `PROPERTY` queries do not use `TIME`. Review Comment: **[minor] Doc ↔ grammar inconsistency: "TIME is required".** The grammar makes `TIME` optional (`GrammarSelectStatement.Time` is `@@?`), and `references/examples.md` example #2 omits it — and it parses clean, so `validate_bydbql` returns `valid=true` and execution may then fail. Either add `TIME` to that example, or soften this to "required at execution / strongly recommended" and note the parser doesn't enforce it. ########## .claude-plugin/plugin.json: ########## @@ -0,0 +1,37 @@ +{ Review Comment: **[minor] Duplicated manifest — drift risk.** `.claude-plugin/plugin.json` and `.codex-plugin/plugin.json` are ~95% identical (only `version` differs). A short note on how they're kept in sync — or generating one from the other — would avoid divergence over time. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
