hanahmily commented on code in PR #1201: URL: https://github.com/apache/skywalking-banyandb/pull/1201#discussion_r3522413740
########## 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[] { + const binaryPath = path.join(rootDir, 'tools', 'bin', validatorBinaryName); + if (existsSync(binaryPath)) { + return [ + { command: binaryPath, args: [], cwd: rootDir }, + { command: 'go', args: ['run', './tools/bydbql-parse'], cwd: rootDir }, + ]; + } + + return [{ command: 'go', args: ['run', './tools/bydbql-parse'], cwd: rootDir }]; +} + +function validationEnvironment(): NodeJS.ProcessEnv { + return { + ...process.env, + GOCACHE: process.env.GOCACHE || path.join(tmpdir(), 'banyandb-mcp-go-build-cache'), + GOMODCACHE: process.env.GOMODCACHE || path.join(tmpdir(), 'banyandb-mcp-go-mod-cache'), + GOPATH: process.env.GOPATH || path.join(tmpdir(), 'banyandb-mcp-go-path'), + GOFLAGS: process.env.GOFLAGS || '-mod=readonly', + }; +} + +export async function validateBydbQLSyntax(query: string): Promise<BydbQLParseValidationResult> { Review Comment: **[recommended][Layer 4] Integration test for the spawn path.** Once `vitest` is in place, add a test that runs `validateBydbQLSyntax()` against the built binary (`npm run build:validator` first) to cover the JSON round-trip and binary-vs-`go run` selection, e.g. a valid query returns `{valid:true, queryType:'STREAM', syntaxOnly:true}` (use a ~20s vitest timeout; skip gracefully if the binary is absent). This is also the natural place to assert the timeout/error-to-`{valid:false}` behavior — which ties directly to the cold-`go run` timeout concern in my other comment on this file. ########## mcp/package.json: ########## @@ -5,6 +5,7 @@ "type": "module", "main": "dist/index.js", "scripts": { Review Comment: **[blocking][Layer 3] No test tooling in `mcp/`.** Please add a test runner so the TypeScript safety layer can be covered: ```jsonc "scripts": { "test": "vitest run", ... }, "devDependencies": { "vitest": "^2.x", ... } ``` Then add `src/query/validation.test.ts` asserting the first-gate `validateBydbQL` behavior that `validate_bydbql` relies on: accepts `SELECT`/`SHOW TOP`; rejects non-read-only (`DELETE`), `;`/`--` comments, and over-length (>4096) input. These are pure functions — no spawn needed — so the test is fast and hermetic. Please also wire `npm test` into whatever CI runs the `mcp/` lint step. ########## mcp/tools/bydbql-parse/main.go: ########## @@ -0,0 +1,90 @@ +// 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. + +package main + +import ( + "encoding/json" + "fmt" + "os" + + "github.com/apache/skywalking-banyandb/pkg/bydbql" +) + +type validateRequest struct { + Query string `json:"query"` +} + +type validateResponse struct { + Valid bool `json:"valid"` + Message string `json:"message"` + QueryType string `json:"queryType,omitempty"` + SyntaxOnly bool `json:"syntaxOnly"` + Warnings []string `json:"warnings,omitempty"` +} + +func main() { Review Comment: **[blocking][Layer 1+2] Add a unit test for the validator tool.** The validation logic lives inside `main()`, so it isn't callable from a test. Please extract it: ```go func validate(query string) validateResponse { grammar, parseErr := bydbql.ParseQuery(query) if parseErr != nil { return validateResponse{Valid: false, SyntaxOnly: true, Message: fmt.Sprintf("failed to parse BydbQL: %v", parseErr)} } return validateResponse{Valid: true, SyntaxOnly: true, Message: "valid BydbQL syntax", QueryType: queryType(grammar), Warnings: []string{"parse-only validation does not verify group, resource, tag, field, or index-rule existence"}} } ``` Then add a table-driven `main_test.go` covering: valid SELECT per resource type (STREAM/MEASURE/TRACE/PROPERTY) → `Valid=true` + correct `QueryType`; `SHOW TOP` → `QueryType=TOPN`; and rejection of `DELETE`, trailing `;`, `--` comments, two statements, and empty input. This tool is in the root module, so `go test ./mcp/tools/...` runs under the existing `make test` with no CI changes. **(Layer 2)** Also add a curated `testdata/valid.txt` + `testdata/invalid.txt` corpus read by the test — do not scrape the markdown docs, since they contain placeholders (`<stream_name>`) and clause fragments that intentionally don't parse. The corpus becomes the regression net for future bugs. ########## 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 Review Comment: **[note, non-blocking] Document that generation quality is validated out-of-band.** Since the host model performs NL→BydbQL, its output can't be unit-tested deterministically. Consider a `make eval-bydbql` target (or scheduled job, not PR CI) that runs a curated `{nl_prompt → [acceptable BydbQL]}` corpus through a real model and scores each output with two LLM-free checks: (a) does it parse (reuse the Layer-1 `validate()`), and (b) does it normalize-match an accepted form. Keep it non-blocking so CI stays model-independent. -- 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]
