From f395e9dbcf8fa663f83e4f4774bde5b116eec501 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 10 Oct 2026 06:13:34 +0000 Subject: [PATCH 1/5] feat(errors): add AI agent guidance fields to error details Enrich XError details with hint, retryable, category, and native DB codes for MySQL/PostgreSQL. Document the contract, add RFC 0015, and cover mapping with unit and integration tests. Co-authored-by: x_zhuo --- .agents/skills/xsql/SKILL.md | 12 +- docs/ai.md | 1 + docs/error-contract.md | 58 +++- docs/rfcs/0015-error-guidance-for-agents.md | 39 +++ internal/db/mysql/driver.go | 4 +- internal/db/pg/driver.go | 6 +- internal/db/query.go | 17 +- internal/db/query_readonly_test.go | 4 +- internal/errors/dbclassify.go | 277 ++++++++++++++++++++ internal/errors/dbclassify_test.go | 113 ++++++++ internal/errors/error.go | 8 +- internal/errors/guidance.go | 195 ++++++++++++++ internal/errors/guidance_test.go | 123 +++++++++ tests/integration/error_guidance_test.go | 159 +++++++++++ 14 files changed, 991 insertions(+), 25 deletions(-) create mode 100644 docs/rfcs/0015-error-guidance-for-agents.md create mode 100644 internal/errors/dbclassify.go create mode 100644 internal/errors/dbclassify_test.go create mode 100644 internal/errors/guidance.go create mode 100644 internal/errors/guidance_test.go create mode 100644 tests/integration/error_guidance_test.go diff --git a/.agents/skills/xsql/SKILL.md b/.agents/skills/xsql/SKILL.md index be664d4d..896e2dd5 100644 --- a/.agents/skills/xsql/SKILL.md +++ b/.agents/skills/xsql/SKILL.md @@ -22,6 +22,7 @@ description: > - Use additional attributes to make usage searchable: `agent` (for example `codex`), `env` (for example `prod`), `team`, and `task` are recommended when known. - `stdout` is data; `stderr` is logs. Never parse stderr as result data. - Validate responses by checking `ok`, `schema_version`, and `error.code` — not by string matching. +- On `ok: false`, read `error.details.hint`, `error.details.category`, and `error.details.retryable` before retrying (see `docs/error-contract.md`). Use `hint` as the next action; respect `retryable: false` for SQL/schema/permission fixes instead of blind retries. - Assume read-only mode. Writes require both `unsafe_allow_write: true` in the selected profile and a runtime write opt-in: `--unsafe-allow-write` on `xsql query` / `xsql ai`, or `--unsafe-allow-write` when starting `xsql mcp server` (or `XSQL_MCP_UNSAFE_ALLOW_WRITE=true`). Only enable both factors when the user has explicitly requested writes. - Never leak secrets, full DSNs, passwords, or private keys in output or summaries. - `--ssh-skip-known-hosts-check` is a risky last resort — call out the security tradeoff if it's needed. @@ -137,7 +138,16 @@ JSON responses follow this envelope: {"ok": false, "schema_version": 1, "error": {"code": "...", "message": "...", "details": {...}}} ``` -Handle failures by `error.code` and exit status: +Structured guidance in `error.details` (when present): + +| Field | Use | +|-------|-----| +| `hint` | Short next step (e.g. run `schema dump`, fix syntax, check profile/ssh_proxy) | +| `category` | Stable class: `unknown_table`, `syntax_error`, `read_only_blocked`, `connection`, … | +| `retryable` | If `false`, change inputs or config before retrying | +| `native_code` | MySQL errno or PostgreSQL SQLSTATE when available | + +Handle failures by `error.code`, `details.category`, and exit status: | Exit Code | Meaning | |-----------|---------| diff --git a/docs/ai.md b/docs/ai.md index 9098c767..35c33be8 100644 --- a/docs/ai.md +++ b/docs/ai.md @@ -10,6 +10,7 @@ ## 规范建议 - 非 TTY 默认输出 JSON；TTY 默认 table。 - 错误对象：`code/message/details`；并保证退出码与 code 对应。 +- 失败时优先读取 `error.details.hint`、`error.details.category` 与 `error.details.retryable`（见 `docs/error-contract.md` §2.1），再决定是否改 SQL、拉 schema、修连接或开启写入；不要仅根据 `message` 字符串盲重试。 - 提供 `xsql spec --format json` 导出： - commands/flags/env mapping - output schema diff --git a/docs/error-contract.md b/docs/error-contract.md index 31696cab..4ec8c899 100644 --- a/docs/error-contract.md +++ b/docs/error-contract.md @@ -20,8 +20,44 @@ 字段含义： - `code`：稳定错误码（字符串，供程序判断） - `message`：面向人的简短描述（可本地化，但 `code` 不变） -- `details`：结构化细节（可选），用于调试/自动化处理 - - **注意**：`cause` 字段不在 JSON/YAML 中暴露，仅在错误字符串输出（`Error()`）和错误解包（`Unwrap()`）中可用 +- `details`：结构化细节（可选），用于调试/自动化处理 + - **注意**：`cause` 字段不在 JSON/YAML 中暴露，仅在错误字符串输出（`Error()`）和错误解包（`Unwrap()`）中可用 + +### 2.1 AI/agent 指引字段（`details` 内，只增不改） + +以下字段由 `internal/errors` 在 `New`/`Wrap` 时自动合并（不覆盖调用方已设置的同名键）： + +| 字段 | 类型 | 说明 | +|------|------|------| +| `hint` | string | 面向 agent 的简短下一步建议（英文） | +| `retryable` | bool | 是否建议在相同输入下立即重试 | +| `category` | string | 稳定语义分类，如 `unknown_table`、`syntax_error`、`read_only_blocked`、`connection` | +| `dialect` | string | 可选，`mysql` 或 `pg` | +| `native_code` | string | 可选，MySQL errno 或 PostgreSQL SQLSTATE | +| `native_code_kind` | string | 可选，`mysql_errno` 或 `pg_sqlstate` | + +只读拦截等既有字段（如 `reason`）保持不变。`schema_version` 仍为 `1`（仅追加 `details` 字段，不 bump 版本号）。 + +示例（未知表）： + +```json +{ + "ok": false, + "schema_version": 1, + "error": { + "code": "XSQL_DB_EXEC_FAILED", + "message": "query failed", + "details": { + "dialect": "mysql", + "category": "unknown_table", + "native_code": "1146", + "native_code_kind": "mysql_errno", + "retryable": false, + "hint": "Table or database not found; run `xsql schema dump` ..." + } + } +} +``` 安全约束： - `details` 中不得包含明文密码、私钥、passphrase、完整 DSN/URL（可脱敏）。 @@ -55,15 +91,15 @@ DB 类： 只读策略： - `XSQL_RO_BLOCKED` - 写操作被只读策略拦截 -端口： -- `XSQL_PORT_IN_USE` - 代理端口被占用 - -鉴权： -- `XSQL_AUTH_REQUIRED` - 请求缺少鉴权 token -- `XSQL_AUTH_INVALID` - 请求提供的鉴权 token 无效 - -内部： -- `XSQL_INTERNAL` - 内部错误 +端口： +- `XSQL_PORT_IN_USE` - 代理端口被占用 + +鉴权： +- `XSQL_AUTH_REQUIRED` - 请求缺少鉴权 token +- `XSQL_AUTH_INVALID` - 请求提供的鉴权 token 无效 + +内部： +- `XSQL_INTERNAL` - 内部错误 ## 5. 查询结果格式 diff --git a/docs/rfcs/0015-error-guidance-for-agents.md b/docs/rfcs/0015-error-guidance-for-agents.md new file mode 100644 index 00000000..1190ef9e --- /dev/null +++ b/docs/rfcs/0015-error-guidance-for-agents.md @@ -0,0 +1,39 @@ +# RFC 0015: AI Agent 错误指引（Error Guidance） + +Status: Accepted + +## 摘要 +为 xsql 的 JSON/YAML 错误 `details` 增加面向 AI/agent 的稳定指引字段（`hint`、`retryable`、`category`、方言与原生 DB 错误码），在不改变顶层 `error.code`、退出码与 `schema_version` 的前提下，减少 agent 对 DB/连接/只读类错误的无效重试。 + +## 背景 / 动机 +- 当前痛点：`XSQL_DB_EXEC_FAILED` 等错误仅携带驱动原始 message，agent 难以判断应改 SQL、查 schema、修连接还是开启写入。 +- 目标：在 `error.details` 中追加可机读、可行动的指引；MySQL errno 与 PostgreSQL SQLSTATE 映射到少量稳定 `category`。 +- 非目标：不改变错误码含义、不提升 `schema_version`、不在 details 中暴露密码/完整 DSN。 + +## 方案（Proposed） +### 用户视角（CLI/配置/输出） +- 失败响应仍为 `{"ok":false,"schema_version":1,"error":{"code","message","details"}}`。 +- `details` **只增**字段（已有键如 `reason` 保留）： + - `hint` (string)：简短下一步建议（英文，便于模型直接遵循）。 + - `retryable` (bool)：是否建议在相同输入下立即重试。 + - `category` (string)：稳定语义分类（如 `unknown_table`、`syntax_error`、`read_only_blocked`）。 + - `dialect` (string, 可选)：`mysql` | `pg`。 + - `native_code` (string, 可选)：MySQL errno 或 PostgreSQL SQLSTATE。 + - `native_code_kind` (string, 可选)：`mysql_errno` | `pg_sqlstate`。 + +### 技术设计（Architecture） +- `internal/errors`：`New`/`Wrap` 返回前调用 `enrichDetails`，根据 `code`、`details` 与 `cause` 合并指引（不覆盖调用方已设置的 details 键）。 +- `internal/errors/dbclassify.go`：解析 `go-sql-driver/mysql.MySQLError` 与 `pgconn.PgError`。 +- `internal/db/query.go`：执行失败时在 details 传入 `dialect`。 +- CLI / MCP / Web 共用 `internal/errors` 与 `internal/output`，无需各自 duplicate。 + +## 兼容性与迁移 +- 不破坏兼容：仅追加 `details` 字段；`schema_version` 保持 `1`。 +- 无迁移步骤。 + +## 测试计划 +- 单元测试：MySQL/PG 典型 errno/SQLSTATE → category/hint/retryable。 +- 集成测试（Docker MySQL 8.0 / PG 16）：未知表、语法错误、只读拦截产生预期 `hint`。 + +## 安全与隐私 +- `hint`/`category` 不含 secrets；`native_code` 为公开错误码，非连接串。 diff --git a/internal/db/mysql/driver.go b/internal/db/mysql/driver.go index 52c8d284..04f46d04 100644 --- a/internal/db/mysql/driver.go +++ b/internal/db/mysql/driver.go @@ -95,14 +95,14 @@ func (d *Driver) Open(ctx context.Context, opts db.ConnOptions) (*sql.DB, *error dsn := cfg.FormatDSN() conn, err := sql.Open("mysql", dsn) if err != nil { - return nil, errors.Wrap(errors.CodeDBConnectFailed, "failed to open mysql connection", nil, err) + return nil, errors.Wrap(errors.CodeDBConnectFailed, "failed to open mysql connection", map[string]any{"dialect": "mysql"}, err) } if err := conn.PingContext(ctx); err != nil { if closeErr := conn.Close(); closeErr != nil { log.Printf("failed to close mysql connection: %v", closeErr) } cleanupDialContext(dialName) - return nil, errors.Wrap(errors.CodeDBConnectFailed, "failed to ping mysql", nil, err) + return nil, errors.Wrap(errors.CodeDBConnectFailed, "failed to ping mysql", map[string]any{"dialect": "mysql"}, err) } return conn, nil } diff --git a/internal/db/pg/driver.go b/internal/db/pg/driver.go index 763a7a4b..3f224a9e 100644 --- a/internal/db/pg/driver.go +++ b/internal/db/pg/driver.go @@ -42,20 +42,20 @@ func (d *Driver) Open(ctx context.Context, opts db.ConnOptions) (*sql.DB, *error if closeErr := conn.Close(); closeErr != nil { log.Printf("failed to close pg connection: %v", closeErr) } - return nil, errors.Wrap(errors.CodeDBConnectFailed, "failed to ping pg", nil, err) + return nil, errors.Wrap(errors.CodeDBConnectFailed, "failed to ping pg", map[string]any{"dialect": "pg"}, err) } return conn, nil } conn, err := sql.Open("pgx", dsn) if err != nil { - return nil, errors.Wrap(errors.CodeDBConnectFailed, "failed to open pg connection", nil, err) + return nil, errors.Wrap(errors.CodeDBConnectFailed, "failed to open pg connection", map[string]any{"dialect": "pg"}, err) } if err := conn.PingContext(ctx); err != nil { if closeErr := conn.Close(); closeErr != nil { log.Printf("failed to close pg connection: %v", closeErr) } - return nil, errors.Wrap(errors.CodeDBConnectFailed, "failed to ping pg", nil, err) + return nil, errors.Wrap(errors.CodeDBConnectFailed, "failed to ping pg", map[string]any{"dialect": "pg"}, err) } return conn, nil } diff --git a/internal/db/query.go b/internal/db/query.go index 85b1f951..bba4c009 100644 --- a/internal/db/query.go +++ b/internal/db/query.go @@ -38,7 +38,7 @@ type QueryOptions struct { func Query(ctx context.Context, db *sql.DB, query string, opts QueryOptions) (*QueryResult, *errors.XError) { // UnsafeAllowWrite bypasses all read-only protections if opts.UnsafeAllowWrite { - return executeQuery(ctx, db, query) + return executeQuery(ctx, db, query, opts.DBType) } // Enable dual read-only protection by default @@ -100,7 +100,7 @@ func queryWithReadOnlyTx(ctx context.Context, db *sql.DB, query string, dbType s rows, err := tx.QueryContext(ctx, query) if err != nil { - return nil, errors.Wrap(errors.CodeDBExecFailed, "query failed", nil, err) + return nil, wrapQueryFailed(dbType, err) } defer rows.Close() @@ -108,10 +108,10 @@ func queryWithReadOnlyTx(ctx context.Context, db *sql.DB, query string, dbType s } // executeQuery executes a query directly (without a transaction). -func executeQuery(ctx context.Context, db *sql.DB, query string) (*QueryResult, *errors.XError) { +func executeQuery(ctx context.Context, db *sql.DB, query string, dbType string) (*QueryResult, *errors.XError) { rows, err := db.QueryContext(ctx, query) if err != nil { - return nil, errors.Wrap(errors.CodeDBExecFailed, "query failed", nil, err) + return nil, wrapQueryFailed(dbType, err) } defer rows.Close() @@ -147,6 +147,15 @@ func scanRows(rows *sql.Rows) (*QueryResult, *errors.XError) { return result, nil } +func wrapQueryFailed(dbType string, err error) *errors.XError { + dialect := normalizeDBType(dbType) + details := map[string]any{} + if dialect != "" { + details["dialect"] = dialect + } + return errors.Wrap(errors.CodeDBExecFailed, "query failed", details, err) +} + func convertValue(v any) any { switch val := v.(type) { case []byte: diff --git a/internal/db/query_readonly_test.go b/internal/db/query_readonly_test.go index dfc8f117..60b2fd5d 100644 --- a/internal/db/query_readonly_test.go +++ b/internal/db/query_readonly_test.go @@ -225,7 +225,7 @@ func TestExecuteQuery_Success(t *testing.T) { rows := sqlmock.NewRows([]string{"msg"}).AddRow([]byte("hi")) mock.ExpectQuery("SELECT 'hi'").WillReturnRows(rows) - result, xe := executeQuery(context.Background(), db, "SELECT 'hi'") + result, xe := executeQuery(context.Background(), db, "SELECT 'hi'", "mysql") if xe != nil { t.Fatalf("executeQuery failed: %v", xe) } @@ -243,7 +243,7 @@ func TestExecuteQuery_Failure(t *testing.T) { mock.ExpectQuery("SELECT 1").WillReturnError(errors.New("boom")) - _, xe := executeQuery(context.Background(), db, "SELECT 1") + _, xe := executeQuery(context.Background(), db, "SELECT 1", "mysql") if xe == nil || xe.Code != xsqlerrors.CodeDBExecFailed { t.Fatalf("expected failure, got %v", xe) } diff --git a/internal/errors/dbclassify.go b/internal/errors/dbclassify.go new file mode 100644 index 00000000..220afb5f --- /dev/null +++ b/internal/errors/dbclassify.go @@ -0,0 +1,277 @@ +package errors + +import ( + stderrors "errors" + "net" + "strconv" + "strings" + "syscall" + + "github.com/go-sql-driver/mysql" + "github.com/jackc/pgx/v5/pgconn" +) + +// DBCategory is a stable, dialect-agnostic classification for database errors. +type DBCategory string + +const ( + CategoryUnknownTable DBCategory = "unknown_table" + CategoryUnknownColumn DBCategory = "unknown_column" + CategorySyntaxError DBCategory = "syntax_error" + CategoryPermissionDenied DBCategory = "permission_denied" + CategoryTimeout DBCategory = "timeout" + CategoryConnection DBCategory = "connection" + CategoryReadOnlyViolation DBCategory = "read_only_violation" + CategoryDuplicateKey DBCategory = "duplicate_key" + CategoryOther DBCategory = "other" +) + +// dbClassification holds parsed native DB error metadata and agent guidance. +type dbClassification struct { + Category DBCategory + NativeCode string + NativeCodeKind string + Hint string + Retryable bool +} + +func classifyDriverError(dialect string, err error) (dbClassification, bool) { + if err == nil { + return dbClassification{}, false + } + dialect = normalizeDialect(dialect) + + var myErr *mysql.MySQLError + if stderrors.As(err, &myErr) { + return classifyMySQLError(myErr), true + } + var pgErr *pgconn.PgError + if stderrors.As(err, &pgErr) { + return classifyPgError(pgErr), true + } + + // Walk the chain for wrapped driver errors. + for unwrapped := err; unwrapped != nil; unwrapped = stderrors.Unwrap(unwrapped) { + if stderrors.As(unwrapped, &myErr) { + return classifyMySQLError(myErr), true + } + if stderrors.As(unwrapped, &pgErr) { + return classifyPgError(pgErr), true + } + } + + if dialect == "pg" { + if c, ok := classifyPgMessage(err.Error()); ok { + return c, true + } + } + if dialect == "mysql" { + if c, ok := classifyMySQLMessage(err.Error()); ok { + return c, true + } + } + return dbClassification{}, false +} + +func classifyMySQLError(e *mysql.MySQLError) dbClassification { + c := dbClassification{ + NativeCode: intString(e.Number), + NativeCodeKind: "mysql_errno", + } + switch e.Number { + case 1146, 1049: + c.Category = CategoryUnknownTable + c.Hint = "Table or database not found; run `xsql schema dump` (or query information_schema) to list valid tables, then fix the SQL." + c.Retryable = false + case 1054, 1109: + c.Category = CategoryUnknownColumn + c.Hint = "Column not found; run `xsql schema dump --table ` to inspect columns, then fix the SQL." + c.Retryable = false + case 1064, 1149, 1060: + c.Category = CategorySyntaxError + c.Hint = "Fix SQL syntax; compare with the profile dialect (mysql) and re-run." + c.Retryable = false + case 1142, 1227, 1044, 1045: + c.Category = CategoryPermissionDenied + c.Hint = "Insufficient database privileges; use a read-only SELECT or ask for a profile with required grants." + c.Retryable = false + case 1205, 1213, 3024: + c.Category = CategoryTimeout + c.Hint = "Query timed out or lock wait exceeded; narrow the query, add LIMIT, or retry later." + c.Retryable = true + case 1792, 1836: + c.Category = CategoryReadOnlyViolation + c.Hint = "Write blocked in read-only mode; set profile `unsafe_allow_write: true` AND pass `--unsafe-allow-write` on query/ai/mcp server." + c.Retryable = false + case 1062: + c.Category = CategoryDuplicateKey + c.Hint = "Duplicate key conflict; adjust INSERT/UPDATE values or use ON DUPLICATE handling if writes are explicitly allowed." + c.Retryable = false + case 2002, 2003, 2006, 2013: + c.Category = CategoryConnection + c.Hint = "Cannot reach MySQL; verify profile host/port, network, and ssh_proxy settings." + c.Retryable = true + default: + c.Category = CategoryOther + c.Hint = "Database returned an error; inspect native_code and message, adjust SQL or connection settings." + c.Retryable = false + } + return c +} + +func classifyPgError(e *pgconn.PgError) dbClassification { + c := dbClassification{ + NativeCode: e.Code, + NativeCodeKind: "pg_sqlstate", + } + switch e.Code { + case "42P01", "3F000": + c.Category = CategoryUnknownTable + c.Hint = "Table or schema not found; run `xsql schema dump` to list tables, then fix the SQL." + c.Retryable = false + case "42703": + c.Category = CategoryUnknownColumn + c.Hint = "Column not found; run `xsql schema dump --table ` to inspect columns, then fix the SQL." + c.Retryable = false + case "42601", "42803", "42809": + c.Category = CategorySyntaxError + c.Hint = "Fix SQL syntax; compare with the profile dialect (pg) and re-run." + c.Retryable = false + case "42501": + c.Category = CategoryPermissionDenied + c.Hint = "Insufficient database privileges; use a read-only SELECT or ask for a profile with required grants." + c.Retryable = false + case "57014", "55P03": + c.Category = CategoryTimeout + c.Hint = "Query canceled or lock timeout; narrow the query, add LIMIT, or retry later." + c.Retryable = true + case "25006", "25007": + c.Category = CategoryReadOnlyViolation + c.Hint = "Write blocked in read-only transaction; set profile `unsafe_allow_write: true` AND pass `--unsafe-allow-write` on query/ai/mcp server." + c.Retryable = false + case "23505": + c.Category = CategoryDuplicateKey + c.Hint = "Duplicate key conflict; adjust values or constraints if writes are explicitly allowed." + c.Retryable = false + case "08000", "08001", "08003", "08006", "57P01", "53300": + c.Category = CategoryConnection + c.Hint = "Cannot reach PostgreSQL; verify profile host/port, sslmode, and ssh_proxy settings." + c.Retryable = true + case "40001", "40P01": + c.Category = CategoryTimeout + c.Hint = "Serialization or deadlock failure; retry the read query or simplify concurrent access." + c.Retryable = true + default: + c.Category = CategoryOther + c.Hint = "Database returned an error; inspect native_code and message, adjust SQL or connection settings." + c.Retryable = false + } + return c +} + +func classifyMySQLMessage(msg string) (dbClassification, bool) { + lower := strings.ToLower(msg) + switch { + case strings.Contains(lower, "doesn't exist"): + return dbClassification{ + Category: CategoryUnknownTable, + Hint: "Table or database not found; run `xsql schema dump` to list tables, then fix the SQL.", + Retryable: false, + NativeCodeKind: "mysql_errno", + }, true + case strings.Contains(lower, "unknown column"): + return dbClassification{ + Category: CategoryUnknownColumn, + Hint: "Column not found; run `xsql schema dump --table ` to inspect columns, then fix the SQL.", + Retryable: false, + NativeCodeKind: "mysql_errno", + }, true + case strings.Contains(lower, "syntax"): + return dbClassification{ + Category: CategorySyntaxError, + Hint: "Fix SQL syntax; compare with the profile dialect (mysql) and re-run.", + Retryable: false, + NativeCodeKind: "mysql_errno", + }, true + default: + return dbClassification{}, false + } +} + +func classifyPgMessage(msg string) (dbClassification, bool) { + lower := strings.ToLower(msg) + switch { + case strings.Contains(lower, "does not exist") && strings.Contains(lower, "relation"): + return dbClassification{ + Category: CategoryUnknownTable, + Hint: "Table or schema not found; run `xsql schema dump` to list tables, then fix the SQL.", + Retryable: false, + NativeCodeKind: "pg_sqlstate", + }, true + case strings.Contains(lower, "column") && strings.Contains(lower, "does not exist"): + return dbClassification{ + Category: CategoryUnknownColumn, + Hint: "Column not found; run `xsql schema dump --table ` to inspect columns, then fix the SQL.", + Retryable: false, + NativeCodeKind: "pg_sqlstate", + }, true + case strings.Contains(lower, "syntax error"): + return dbClassification{ + Category: CategorySyntaxError, + Hint: "Fix SQL syntax; compare with the profile dialect (pg) and re-run.", + Retryable: false, + NativeCodeKind: "pg_sqlstate", + }, true + default: + return dbClassification{}, false + } +} + +func classifyNetworkError(err error) (dbClassification, bool) { + var netErr net.Error + if stderrors.As(err, &netErr) { + if netErr.Timeout() { + return dbClassification{ + Category: CategoryTimeout, + Hint: "Network timeout; verify host reachability, ssh_proxy, and firewall rules, then retry.", + Retryable: true, + }, true + } + return dbClassification{ + Category: CategoryConnection, + Hint: "Network connection failed; verify profile host/port and ssh_proxy settings.", + Retryable: true, + }, true + } + var opErr *net.OpError + if stderrors.As(err, &opErr) { + return dbClassification{ + Category: CategoryConnection, + Hint: "Network connection failed; verify profile host/port and ssh_proxy settings.", + Retryable: true, + }, true + } + if stderrors.Is(err, syscall.ECONNREFUSED) || stderrors.Is(err, syscall.ECONNRESET) { + return dbClassification{ + Category: CategoryConnection, + Hint: "Connection refused or reset; verify the database is listening and profile port is correct.", + Retryable: true, + }, true + } + return dbClassification{}, false +} + +func normalizeDialect(d string) string { + switch strings.ToLower(strings.TrimSpace(d)) { + case "mysql", "mariadb": + return "mysql" + case "pg", "postgres", "postgresql": + return "pg" + default: + return strings.ToLower(strings.TrimSpace(d)) + } +} + +func intString(n uint16) string { + return strconv.FormatUint(uint64(n), 10) +} diff --git a/internal/errors/dbclassify_test.go b/internal/errors/dbclassify_test.go new file mode 100644 index 00000000..164a092c --- /dev/null +++ b/internal/errors/dbclassify_test.go @@ -0,0 +1,113 @@ +package errors + +import ( + "strings" + "testing" + + "github.com/go-sql-driver/mysql" + "github.com/jackc/pgx/v5/pgconn" +) + +func TestClassifyMySQLError(t *testing.T) { + tests := []struct { + name string + number uint16 + wantCat DBCategory + wantCode string + wantRetry bool + wantHintSub string + }{ + {"unknown_table", 1146, CategoryUnknownTable, "1146", false, "schema dump"}, + {"unknown_column", 1054, CategoryUnknownColumn, "1054", false, "schema dump"}, + {"syntax", 1064, CategorySyntaxError, "1064", false, "syntax"}, + {"read_only_tx", 1792, CategoryReadOnlyViolation, "1792", false, "unsafe_allow_write"}, + {"connection", 2003, CategoryConnection, "2003", true, "host/port"}, + {"timeout", 1205, CategoryTimeout, "1205", true, "LIMIT"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := classifyMySQLError(&mysql.MySQLError{Number: tt.number, Message: "driver message"}) + if c.Category != tt.wantCat { + t.Errorf("category = %q, want %q", c.Category, tt.wantCat) + } + if c.NativeCode != tt.wantCode { + t.Errorf("native_code = %q, want %q", c.NativeCode, tt.wantCode) + } + if c.NativeCodeKind != "mysql_errno" { + t.Errorf("native_code_kind = %q", c.NativeCodeKind) + } + if c.Retryable != tt.wantRetry { + t.Errorf("retryable = %v, want %v", c.Retryable, tt.wantRetry) + } + if tt.wantHintSub != "" && !containsFold(c.Hint, tt.wantHintSub) { + t.Errorf("hint %q should contain %q", c.Hint, tt.wantHintSub) + } + }) + } +} + +func TestClassifyPgError(t *testing.T) { + tests := []struct { + name string + code string + wantCat DBCategory + wantRetry bool + wantHintSub string + }{ + {"unknown_table", "42P01", CategoryUnknownTable, false, "schema dump"}, + {"unknown_column", "42703", CategoryUnknownColumn, false, "schema dump"}, + {"syntax", "42601", CategorySyntaxError, false, "syntax"}, + {"read_only", "25006", CategoryReadOnlyViolation, false, "unsafe_allow_write"}, + {"connection", "08006", CategoryConnection, true, "PostgreSQL"}, + {"timeout", "57014", CategoryTimeout, true, "LIMIT"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := classifyPgError(&pgconn.PgError{Code: tt.code, Message: "driver message"}) + if c.Category != tt.wantCat { + t.Errorf("category = %q, want %q", c.Category, tt.wantCat) + } + if c.NativeCode != tt.code { + t.Errorf("native_code = %q, want %q", c.NativeCode, tt.code) + } + if c.NativeCodeKind != "pg_sqlstate" { + t.Errorf("native_code_kind = %q", c.NativeCodeKind) + } + if c.Retryable != tt.wantRetry { + t.Errorf("retryable = %v, want %v", c.Retryable, tt.wantRetry) + } + if tt.wantHintSub != "" && !containsFold(c.Hint, tt.wantHintSub) { + t.Errorf("hint %q should contain %q", c.Hint, tt.wantHintSub) + } + }) + } +} + +func TestClassifyDriverError_Chain(t *testing.T) { + inner := &mysql.MySQLError{Number: 1146, Message: "Table 'db.t' doesn't exist"} + xe := Wrap(CodeDBExecFailed, "query failed", map[string]any{"dialect": "mysql"}, inner) + if xe.Details["category"] != string(CategoryUnknownTable) { + t.Errorf("expected unknown_table category, got %v", xe.Details["category"]) + } + if xe.Details["native_code"] != "1146" { + t.Errorf("expected native_code 1146, got %v", xe.Details["native_code"]) + } +} + +func TestReadOnlyBlockedGuidance(t *testing.T) { + xe := New(CodeROBlocked, "write blocked", map[string]any{"reason": "forbidden_keyword: DELETE"}) + if xe.Details["category"] != "read_only_blocked" { + t.Errorf("category = %v", xe.Details["category"]) + } + if xe.Details["retryable"] != false { + t.Errorf("retryable should be false") + } + hint, _ := xe.Details["hint"].(string) + if hint == "" || !containsFold(hint, "DELETE") && !containsFold(hint, "write keyword") { + t.Errorf("unexpected hint: %q", hint) + } +} + +func containsFold(s, sub string) bool { + return strings.Contains(strings.ToLower(s), strings.ToLower(sub)) +} diff --git a/internal/errors/error.go b/internal/errors/error.go index 90681535..1c7557af 100644 --- a/internal/errors/error.go +++ b/internal/errors/error.go @@ -27,11 +27,15 @@ func (e *XError) Error() string { func (e *XError) Unwrap() error { return e.cause } func New(code Code, message string, details map[string]any) *XError { - return &XError{Code: code, Message: message, Details: details} + xe := &XError{Code: code, Message: message, Details: details} + enrichDetails(xe) + return xe } func Wrap(code Code, message string, details map[string]any, cause error) *XError { - return &XError{Code: code, Message: message, Details: details, cause: cause} + xe := &XError{Code: code, Message: message, Details: details, cause: cause} + enrichDetails(xe) + return xe } func As(err error) (*XError, bool) { diff --git a/internal/errors/guidance.go b/internal/errors/guidance.go new file mode 100644 index 00000000..d0c600e6 --- /dev/null +++ b/internal/errors/guidance.go @@ -0,0 +1,195 @@ +package errors + +import ( + "strings" +) + +const ( + detailHint = "hint" + detailRetryable = "retryable" + detailCategory = "category" + detailDialect = "dialect" + detailNativeCode = "native_code" + detailNativeCodeKind = "native_code_kind" +) + +func enrichDetails(xe *XError) { + if xe == nil { + return + } + additions := guidanceFor(xe) + if len(additions) == 0 { + return + } + xe.Details = mergeDetails(xe.Details, additions) +} + +func mergeDetails(base, additions map[string]any) map[string]any { + if len(additions) == 0 { + if base == nil { + return nil + } + return base + } + out := make(map[string]any, len(base)+len(additions)) + for k, v := range base { + out[k] = v + } + for k, v := range additions { + if _, exists := out[k]; exists { + continue + } + out[k] = v + } + return out +} + +func guidanceFor(xe *XError) map[string]any { + switch xe.Code { + case CodeROBlocked: + return readOnlyGuidance(xe.Details) + case CodeDBExecFailed: + return dbExecGuidance(xe) + case CodeDBConnectFailed, CodeDBAuthFailed: + return connectGuidance(xe) + case CodeSSHDialFailed, CodeSSHAuthFailed, CodeSSHHostKeyMismatch: + return sshGuidance(xe) + case CodeCfgNotFound, CodeCfgInvalid, CodeSecretNotFound: + return configGuidance(xe) + default: + return nil + } +} + +func readOnlyGuidance(details map[string]any) map[string]any { + out := map[string]any{ + detailCategory: "read_only_blocked", + detailRetryable: false, + detailHint: "Writes require profile `unsafe_allow_write: true` AND `--unsafe-allow-write` on `xsql query` / `xsql ai` or MCP server startup.", + } + if reason, _ := details["reason"].(string); reason != "" { + if strings.HasPrefix(reason, "forbidden_keyword:") { + out[detailHint] = "Statement contains a write keyword blocked by read-only policy; use SELECT-only SQL, or enable profile `unsafe_allow_write` and `--unsafe-allow-write` if the user explicitly requested writes." + } + } + return out +} + +func dbExecGuidance(xe *XError) map[string]any { + dialect := dialectFromDetails(xe.Details) + if xe.cause != nil { + if c, ok := classifyDriverError(dialect, xe.cause); ok { + return classificationToDetails(dialect, c) + } + if c, ok := classifyNetworkError(xe.cause); ok { + return classificationToDetails(dialect, c) + } + } + return map[string]any{ + detailCategory: string(CategoryOther), + detailRetryable: false, + detailHint: "Query execution failed; verify SQL against schema (try `xsql schema dump`) and dialect-specific syntax.", + } +} + +func connectGuidance(xe *XError) map[string]any { + dialect := dialectFromDetails(xe.Details) + out := map[string]any{ + detailCategory: string(CategoryConnection), + detailRetryable: true, + detailHint: "Database connection failed; check profile host, port, database name, credentials, and ssh_proxy settings.", + } + if dialect != "" { + out[detailDialect] = dialect + } + if xe.cause != nil { + if c, ok := classifyDriverError(dialect, xe.cause); ok { + return classificationToDetails(dialect, c) + } + if c, ok := classifyNetworkError(xe.cause); ok { + return classificationToDetails(dialect, c) + } + msg := strings.ToLower(xe.cause.Error()) + if xe.Code == CodeDBAuthFailed || strings.Contains(msg, "access denied") || strings.Contains(msg, "password authentication failed") { + out[detailCategory] = string(CategoryPermissionDenied) + out[detailRetryable] = false + out[detailHint] = "Authentication failed; verify username/password or keyring secret for this profile (avoid plaintext in logs)." + } + } + return out +} + +func sshGuidance(xe *XError) map[string]any { + out := map[string]any{ + detailCategory: string(CategoryConnection), + detailRetryable: true, + } + switch xe.Code { + case CodeSSHAuthFailed: + out[detailHint] = "SSH authentication failed; verify ssh user, private key, and keyring-stored passphrase for the profile or ssh_proxy." + out[detailRetryable] = false + case CodeSSHHostKeyMismatch: + out[detailHint] = "SSH host key mismatch; update known_hosts or fix ssh host. Avoid `--ssh-skip-known-hosts-check` unless explicitly accepted." + out[detailRetryable] = false + default: + out[detailHint] = "SSH dial failed; verify ssh_proxy host, port, and network path to the bastion." + } + if xe.cause != nil { + if c, ok := classifyNetworkError(xe.cause); ok { + if c.Hint != "" { + out[detailHint] = c.Hint + } + out[detailRetryable] = c.Retryable + } + } + return out +} + +func configGuidance(xe *XError) map[string]any { + out := map[string]any{ + detailRetryable: false, + detailCategory: "config", + } + switch xe.Code { + case CodeCfgNotFound: + out[detailHint] = "Config file missing; run `xsql config init` or pass `--config` with a valid path." + case CodeSecretNotFound: + out[detailHint] = "Secret not found in OS keyring; set the secret or use `--allow-plaintext` only when explicitly permitted." + default: + out[detailHint] = "Invalid configuration; run `xsql profile show ` and fix the reported field." + } + return out +} + +func classificationToDetails(dialect string, c dbClassification) map[string]any { + out := map[string]any{ + detailCategory: string(c.Category), + detailRetryable: c.Retryable, + } + if c.Hint != "" { + out[detailHint] = c.Hint + } + if dialect != "" { + out[detailDialect] = dialect + } + if c.NativeCode != "" { + out[detailNativeCode] = c.NativeCode + } + if c.NativeCodeKind != "" { + out[detailNativeCodeKind] = c.NativeCodeKind + } + return out +} + +func dialectFromDetails(details map[string]any) string { + if details == nil { + return "" + } + if d, ok := details["dialect"].(string); ok { + return normalizeDialect(d) + } + if d, ok := details["db"].(string); ok { + return normalizeDialect(d) + } + return "" +} diff --git a/internal/errors/guidance_test.go b/internal/errors/guidance_test.go new file mode 100644 index 00000000..c1cf8b2e --- /dev/null +++ b/internal/errors/guidance_test.go @@ -0,0 +1,123 @@ +package errors + +import ( + "errors" + "net" + "syscall" + "testing" + + "github.com/jackc/pgx/v5/pgconn" +) + +func TestMergeDetails_PreservesExisting(t *testing.T) { + base := map[string]any{"reason": "forbidden_keyword: INSERT", "hint": "custom"} + add := map[string]any{"hint": "ignored", "retryable": false} + merged := mergeDetails(base, add) + if merged["hint"] != "custom" { + t.Errorf("hint = %v, want custom", merged["hint"]) + } + if merged["retryable"] != false { + t.Errorf("retryable not merged") + } +} + +func TestConnectGuidance_Auth(t *testing.T) { + xe := Wrap(CodeDBAuthFailed, "auth failed", map[string]any{"dialect": "mysql"}, errors.New("access denied for user")) + if xe.Details["category"] != string(CategoryPermissionDenied) { + t.Errorf("category = %v", xe.Details["category"]) + } + if xe.Details["retryable"] != false { + t.Errorf("retryable should be false") + } +} + +func TestConnectGuidance_Network(t *testing.T) { + xe := Wrap(CodeDBConnectFailed, "ping failed", map[string]any{"dialect": "pg"}, &net.OpError{Op: "dial", Err: syscall.ECONNREFUSED}) + if xe.Details["category"] != string(CategoryConnection) { + t.Errorf("category = %v", xe.Details["category"]) + } + if xe.Details["retryable"] != true { + t.Errorf("retryable should be true") + } +} + +func TestSSHGuidance_Codes(t *testing.T) { + auth := Wrap(CodeSSHAuthFailed, "ssh auth", nil, errors.New("unable to authenticate")) + if auth.Details["retryable"] != false { + t.Error("ssh auth should not be retryable") + } + hostkey := New(CodeSSHHostKeyMismatch, "host key", nil) + if !containsFold(hostkey.Details["hint"].(string), "host key") { + t.Errorf("hint = %v", hostkey.Details["hint"]) + } +} + +func TestConfigGuidance(t *testing.T) { + xe := New(CodeCfgNotFound, "missing", nil) + if !containsFold(xe.Details["hint"].(string), "config init") { + t.Errorf("hint = %v", xe.Details["hint"]) + } +} + +func TestClassifyNetworkTimeout(t *testing.T) { + err := &net.DNSError{IsTimeout: true, Err: "timeout"} + c, ok := classifyNetworkError(err) + if !ok || c.Category != CategoryTimeout || !c.Retryable { + t.Fatalf("unexpected classification: %+v ok=%v", c, ok) + } +} + +func TestClassifyPgMessageFallback(t *testing.T) { + c, ok := classifyPgMessage(`relation "missing" does not exist`) + if !ok || c.Category != CategoryUnknownTable { + t.Fatalf("got %+v ok=%v", c, ok) + } +} + +func TestDBExecGuidance_Fallback(t *testing.T) { + xe := Wrap(CodeDBExecFailed, "query failed", map[string]any{"dialect": "mysql"}, errors.New("generic failure")) + if xe.Details["category"] != string(CategoryOther) { + t.Errorf("category = %v", xe.Details["category"]) + } +} + +func TestReadOnlyGuidance_DefaultHint(t *testing.T) { + xe := New(CodeROBlocked, "blocked", map[string]any{"reason": "not_select"}) + if xe.Details["hint"] == "" { + t.Fatal("expected hint") + } +} + +func TestDialectFromDetails(t *testing.T) { + if dialectFromDetails(map[string]any{"db": "postgresql"}) != "pg" { + t.Error("expected pg from db field") + } +} + +func TestClassifyMySQLMessagePaths(t *testing.T) { + c, ok := classifyMySQLMessage("Unknown column 'x' in 'field list'") + if !ok || c.Category != CategoryUnknownColumn { + t.Fatalf("got %+v ok=%v", c, ok) + } +} + +func TestClassifyPgError_Default(t *testing.T) { + c := classifyPgError(&pgconn.PgError{Code: "99999", Message: "weird"}) + if c.Category != CategoryOther { + t.Errorf("category = %q", c.Category) + } +} + +func TestSSHDialGuidance(t *testing.T) { + xe := Wrap(CodeSSHDialFailed, "dial", map[string]any{"host": "bastion"}, errors.New("connection reset")) + if xe.Details["retryable"] != true { + t.Errorf("retryable = %v", xe.Details["retryable"]) + } +} + +func TestMergeDetails_NilBase(t *testing.T) { + merged := mergeDetails(nil, map[string]any{"hint": "x"}) + if merged["hint"] != "x" { + t.Errorf("merged = %v", merged) + } +} diff --git a/tests/integration/error_guidance_test.go b/tests/integration/error_guidance_test.go new file mode 100644 index 00000000..7ecff760 --- /dev/null +++ b/tests/integration/error_guidance_test.go @@ -0,0 +1,159 @@ +//go:build integration + +package integration + +import ( + "context" + "os" + "testing" + "time" + + "github.com/zx06/xsql/internal/db" + "github.com/zx06/xsql/internal/errors" + _ "github.com/zx06/xsql/internal/db/mysql" + _ "github.com/zx06/xsql/internal/db/pg" +) + +func assertGuidance(t *testing.T, xe *errors.XError, wantCategory string, wantHintSub string) { + t.Helper() + if xe == nil { + t.Fatal("expected error") + } + if xe.Details == nil { + t.Fatal("expected error details") + } + cat, _ := xe.Details["category"].(string) + if cat != wantCategory { + t.Errorf("category = %q, want %q", cat, wantCategory) + } + hint, _ := xe.Details["hint"].(string) + if hint == "" { + t.Error("expected non-empty hint") + } else if wantHintSub != "" && !containsInsensitive(hint, wantHintSub) { + t.Errorf("hint %q should mention %q", hint, wantHintSub) + } + if _, ok := xe.Details["retryable"].(bool); !ok { + t.Error("expected retryable bool in details") + } +} + +func containsInsensitive(s, sub string) bool { + return len(sub) == 0 || (len(s) >= len(sub) && stringIndexInsensitive(s, sub) >= 0) +} + +func stringIndexInsensitive(s, sub string) int { + ls := len(s) + lsub := len(sub) + for i := 0; i+lsub <= ls; i++ { + if equalFoldASCII(s[i:i+lsub], sub) { + return i + } + } + return -1 +} + +func equalFoldASCII(a, b string) bool { + if len(a) != len(b) { + return false + } + for i := 0; i < len(a); i++ { + ca, cb := a[i], b[i] + if ca >= 'A' && ca <= 'Z' { + ca += 'a' - 'A' + } + if cb >= 'A' && cb <= 'Z' { + cb += 'a' - 'A' + } + if ca != cb { + return false + } + } + return true +} + +func TestErrorGuidance_MySQL(t *testing.T) { + dsn := os.Getenv("XSQL_TEST_MYSQL_DSN") + if dsn == "" { + t.Skip("XSQL_TEST_MYSQL_DSN not set") + } + runMySQLGuidanceCases(t, dsn) +} + +func runMySQLGuidanceCases(t *testing.T, dsn string) { + drv, _ := db.Get("mysql") + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + conn, xe := drv.Open(ctx, db.ConnOptions{DSN: dsn}) + if xe != nil { + t.Fatalf("open: %v", xe) + } + defer conn.Close() + + t.Run("unknown_table", func(t *testing.T) { + _, xe := db.Query(ctx, conn, "SELECT * FROM definitely_nonexistent_table_12345", db.QueryOptions{DBType: "mysql"}) + if xe == nil || xe.Code != errors.CodeDBExecFailed { + t.Fatalf("expected exec failed, got %v", xe) + } + assertGuidance(t, xe, "unknown_table", "schema dump") + if xe.Details["dialect"] != "mysql" { + t.Errorf("dialect = %v", xe.Details["dialect"]) + } + }) + + t.Run("syntax_error", func(t *testing.T) { + _, xe := db.Query(ctx, conn, "SELECT 1 +", db.QueryOptions{DBType: "mysql"}) + if xe == nil || xe.Code != errors.CodeDBExecFailed { + t.Fatalf("expected exec failed, got %v", xe) + } + assertGuidance(t, xe, "syntax_error", "syntax") + }) + + t.Run("read_only_blocked", func(t *testing.T) { + _, xe := db.Query(ctx, conn, "DELETE FROM nonexistent_table_xsql_ro", db.QueryOptions{DBType: "mysql"}) + if xe == nil || xe.Code != errors.CodeROBlocked { + t.Fatalf("expected RO blocked, got %v", xe) + } + assertGuidance(t, xe, "read_only_blocked", "unsafe_allow_write") + }) +} + +func TestErrorGuidance_PostgreSQL(t *testing.T) { + dsn := os.Getenv("XSQL_TEST_PG_DSN") + if dsn == "" { + t.Skip("XSQL_TEST_PG_DSN not set") + } + drv, _ := db.Get("pg") + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + conn, xe := drv.Open(ctx, db.ConnOptions{DSN: dsn}) + if xe != nil { + t.Fatalf("open: %v", xe) + } + defer conn.Close() + + t.Run("unknown_table", func(t *testing.T) { + _, xe := db.Query(ctx, conn, "SELECT * FROM definitely_nonexistent_table_12345", db.QueryOptions{DBType: "pg"}) + if xe == nil || xe.Code != errors.CodeDBExecFailed { + t.Fatalf("expected exec failed, got %v", xe) + } + assertGuidance(t, xe, "unknown_table", "schema dump") + }) + + t.Run("syntax_error", func(t *testing.T) { + _, xe := db.Query(ctx, conn, "SELECT 1 +", db.QueryOptions{DBType: "pg"}) + if xe == nil || xe.Code != errors.CodeDBExecFailed { + t.Fatalf("expected exec failed, got %v", xe) + } + assertGuidance(t, xe, "syntax_error", "syntax") + }) + + t.Run("read_only_blocked", func(t *testing.T) { + _, xe := db.Query(ctx, conn, "DELETE FROM nonexistent_table_xsql_ro", db.QueryOptions{DBType: "pg"}) + if xe == nil || xe.Code != errors.CodeROBlocked { + t.Fatalf("expected RO blocked, got %v", xe) + } + assertGuidance(t, xe, "read_only_blocked", "unsafe_allow_write") + }) +} From b1fa3a7bd99e6925e4a0041e29c82e0a39165611 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 10 Oct 2026 06:14:17 +0000 Subject: [PATCH 2/5] docs(rfc): renumber error guidance RFC to 0016 Co-authored-by: x_zhuo --- ...guidance-for-agents.md => 0016-error-guidance-for-agents.md} | 2 +- docs/rfcs/README.md | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) rename docs/rfcs/{0015-error-guidance-for-agents.md => 0016-error-guidance-for-agents.md} (97%) diff --git a/docs/rfcs/0015-error-guidance-for-agents.md b/docs/rfcs/0016-error-guidance-for-agents.md similarity index 97% rename from docs/rfcs/0015-error-guidance-for-agents.md rename to docs/rfcs/0016-error-guidance-for-agents.md index 1190ef9e..cd82591e 100644 --- a/docs/rfcs/0015-error-guidance-for-agents.md +++ b/docs/rfcs/0016-error-guidance-for-agents.md @@ -1,4 +1,4 @@ -# RFC 0015: AI Agent 错误指引（Error Guidance） +# RFC 0016: AI Agent 错误指引（Error Guidance） Status: Accepted diff --git a/docs/rfcs/README.md b/docs/rfcs/README.md index 3104f0c6..b2b56f7f 100644 --- a/docs/rfcs/README.md +++ b/docs/rfcs/README.md @@ -20,6 +20,7 @@ | [0012](0012-goja-js-data-analysis.md) | goja Data Analysis & ReAct Loop | Implemented | | [0013](0013-ai-multi-tool-calls-sequential-execution.md) | AI Multi Tool Call Sequential Execution & Dialect Guidance | Implemented | | [0014](0014-mcp-write-dual-authorization.md) | MCP 写入双重授权 | Implemented | +| [0016](0016-error-guidance-for-agents.md) | AI Agent 错误指引（Error Guidance） | Accepted | ## 何时需要 RFC 当变更会影响以下任一项时，必须先提交/更新 RFC（并同步更新对应 `docs/*`）： From ea6d10a789a401858b3b720184b5cab715b6b92b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 10 Oct 2026 06:27:34 +0000 Subject: [PATCH 3/5] test(mcp): assert schema_list_tables hint on table_not_found Co-authored-by: x_zhuo --- internal/mcp/schema_tools_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/mcp/schema_tools_test.go b/internal/mcp/schema_tools_test.go index e8322afc..5cbd5ebd 100644 --- a/internal/mcp/schema_tools_test.go +++ b/internal/mcp/schema_tools_test.go @@ -273,6 +273,10 @@ func TestSchemaDescribeTable_TableNotFound(t *testing.T) { t.Fatalf("unexpected error: %v", err) } assertToolError(t, result, "CFG_INVALID", "table not found") + text := result.Content[0].(*mcp.TextContent).Text + if !strings.Contains(text, "schema_list_tables") { + t.Fatalf("expected schema_list_tables hint in %s", text) + } } func TestSchemaListTablesHandler_Success(t *testing.T) { From 3de0edf68732324b4029d5cd33e12663b1316f46 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 10 Oct 2026 06:35:32 +0000 Subject: [PATCH 4/5] fix(errors): auth_failed category, RO reason hints, test dedup - Split MySQL 1045 and PG 28P01/28000 into auth_failed with keyring hint - Map no_keyword and similar static RO reasons to unrecognized_statement - Centralize hint strings; expand dbclassify unit tests and table-driven integration Co-authored-by: x_zhuo --- .agents/skills/xsql/SKILL.md | 2 +- docs/error-contract.md | 2 +- internal/errors/dbclassify.go | 76 ++++--- internal/errors/dbclassify_test.go | 191 ++++++++++------- internal/errors/guidance.go | 44 +++- internal/errors/guidance_test.go | 57 +++-- internal/errors/hints.go | 22 ++ tests/integration/error_guidance_test.go | 255 ++++++++++++----------- 8 files changed, 410 insertions(+), 239 deletions(-) create mode 100644 internal/errors/hints.go diff --git a/.agents/skills/xsql/SKILL.md b/.agents/skills/xsql/SKILL.md index 0744dd6c..3501fc6e 100644 --- a/.agents/skills/xsql/SKILL.md +++ b/.agents/skills/xsql/SKILL.md @@ -143,7 +143,7 @@ Structured guidance in `error.details` (when present): | Field | Use | |-------|-----| | `hint` | Short next step (e.g. run `schema dump`, fix syntax, check profile/ssh_proxy) | -| `category` | Stable class: `unknown_table`, `syntax_error`, `read_only_blocked`, `connection`, … | +| `category` | Stable class: `unknown_table`, `syntax_error`, `read_only_blocked`, `unrecognized_statement`, `auth_failed`, `permission_denied`, `connection`, … | | `retryable` | If `false`, change inputs or config before retrying | | `native_code` | MySQL errno or PostgreSQL SQLSTATE when available | diff --git a/docs/error-contract.md b/docs/error-contract.md index 4ec8c899..be724ab1 100644 --- a/docs/error-contract.md +++ b/docs/error-contract.md @@ -31,7 +31,7 @@ |------|------|------| | `hint` | string | 面向 agent 的简短下一步建议（英文） | | `retryable` | bool | 是否建议在相同输入下立即重试 | -| `category` | string | 稳定语义分类，如 `unknown_table`、`syntax_error`、`read_only_blocked`、`connection` | +| `category` | string | 稳定语义分类，如 `unknown_table`、`syntax_error`、`read_only_blocked`、`unrecognized_statement`、`auth_failed`、`permission_denied`、`connection` | | `dialect` | string | 可选，`mysql` 或 `pg` | | `native_code` | string | 可选，MySQL errno 或 PostgreSQL SQLSTATE | | `native_code_kind` | string | 可选，`mysql_errno` 或 `pg_sqlstate` | diff --git a/internal/errors/dbclassify.go b/internal/errors/dbclassify.go index 1ed549fa..42c280bf 100644 --- a/internal/errors/dbclassify.go +++ b/internal/errors/dbclassify.go @@ -19,6 +19,7 @@ const ( CategoryUnknownColumn DBCategory = "unknown_column" CategorySyntaxError DBCategory = "syntax_error" CategoryPermissionDenied DBCategory = "permission_denied" + CategoryAuthFailed DBCategory = "auth_failed" CategoryTimeout DBCategory = "timeout" CategoryConnection DBCategory = "connection" CategoryReadOnlyViolation DBCategory = "read_only_violation" @@ -50,7 +51,6 @@ func classifyDriverError(dialect string, err error) (dbClassification, bool) { return classifyPgError(pgErr), true } - // Walk the chain for wrapped driver errors. for unwrapped := err; unwrapped != nil; unwrapped = stderrors.Unwrap(unwrapped) { if stderrors.As(unwrapped, &myErr) { return classifyMySQLError(myErr), true @@ -81,39 +81,43 @@ func classifyMySQLError(e *mysql.MySQLError) dbClassification { switch e.Number { case 1146, 1049: c.Category = CategoryUnknownTable - c.Hint = "Table or database not found; use MCP `schema_list_tables` or `xsql schema dump` to list valid tables, then fix the SQL." + c.Hint = hintUnknownTableMySQL c.Retryable = false case 1054, 1109: c.Category = CategoryUnknownColumn - c.Hint = "Column not found; run `xsql schema dump --table ` to inspect columns, then fix the SQL." + c.Hint = hintUnknownColumn c.Retryable = false case 1064, 1149, 1060: c.Category = CategorySyntaxError - c.Hint = "Fix SQL syntax; compare with the profile dialect (mysql) and re-run." + c.Hint = hintSyntaxMySQL c.Retryable = false - case 1142, 1227, 1044, 1045: + case 1045: + c.Category = CategoryAuthFailed + c.Hint = hintAuthFailed + c.Retryable = false + case 1142, 1227, 1044: c.Category = CategoryPermissionDenied - c.Hint = "Insufficient database privileges; use a read-only SELECT or ask for a profile with required grants." + c.Hint = hintPermissionDenied c.Retryable = false case 1205, 1213, 3024: c.Category = CategoryTimeout - c.Hint = "Query timed out or lock wait exceeded; narrow the query, add LIMIT, or retry later." + c.Hint = hintQueryTimeoutMySQL c.Retryable = true case 1792, 1836: c.Category = CategoryReadOnlyViolation - c.Hint = "Write blocked in read-only mode; set profile `unsafe_allow_write: true` AND pass `--unsafe-allow-write` on query/ai/mcp server." + c.Hint = hintReadOnlyWrite c.Retryable = false case 1062: c.Category = CategoryDuplicateKey - c.Hint = "Duplicate key conflict; adjust INSERT/UPDATE values or use ON DUPLICATE handling if writes are explicitly allowed." + c.Hint = hintDuplicateKey c.Retryable = false case 2002, 2003, 2006, 2013: c.Category = CategoryConnection - c.Hint = "Cannot reach MySQL; verify profile host/port, network, and ssh_proxy settings." + c.Hint = hintMySQLConnection c.Retryable = true default: c.Category = CategoryOther - c.Hint = "Database returned an error; inspect native_code and message, adjust SQL or connection settings." + c.Hint = hintDBOther c.Retryable = false } return c @@ -127,43 +131,47 @@ func classifyPgError(e *pgconn.PgError) dbClassification { switch e.Code { case "42P01", "3F000": c.Category = CategoryUnknownTable - c.Hint = "Table or schema not found; use MCP `schema_list_tables` or `xsql schema dump` to list tables, then fix the SQL." + c.Hint = hintUnknownTablePG c.Retryable = false case "42703": c.Category = CategoryUnknownColumn - c.Hint = "Column not found; run `xsql schema dump --table ` to inspect columns, then fix the SQL." + c.Hint = hintUnknownColumn c.Retryable = false case "42601", "42803", "42809": c.Category = CategorySyntaxError - c.Hint = "Fix SQL syntax; compare with the profile dialect (pg) and re-run." + c.Hint = hintSyntaxPG + c.Retryable = false + case "28P01", "28000": + c.Category = CategoryAuthFailed + c.Hint = hintAuthFailed c.Retryable = false case "42501": c.Category = CategoryPermissionDenied - c.Hint = "Insufficient database privileges; use a read-only SELECT or ask for a profile with required grants." + c.Hint = hintPermissionDenied c.Retryable = false case "57014", "55P03": c.Category = CategoryTimeout - c.Hint = "Query canceled or lock timeout; narrow the query, add LIMIT, or retry later." + c.Hint = hintQueryTimeoutPG c.Retryable = true case "25006", "25007": c.Category = CategoryReadOnlyViolation - c.Hint = "Write blocked in read-only transaction; set profile `unsafe_allow_write: true` AND pass `--unsafe-allow-write` on query/ai/mcp server." + c.Hint = hintReadOnlyWritePG c.Retryable = false case "23505": c.Category = CategoryDuplicateKey - c.Hint = "Duplicate key conflict; adjust values or constraints if writes are explicitly allowed." + c.Hint = hintDuplicateKeyPG c.Retryable = false case "08000", "08001", "08003", "08006", "57P01", "53300": c.Category = CategoryConnection - c.Hint = "Cannot reach PostgreSQL; verify profile host/port, sslmode, and ssh_proxy settings." + c.Hint = hintPGConnection c.Retryable = true case "40001", "40P01": c.Category = CategoryTimeout - c.Hint = "Serialization or deadlock failure; retry the read query or simplify concurrent access." + c.Hint = hintSerializationPG c.Retryable = true default: c.Category = CategoryOther - c.Hint = "Database returned an error; inspect native_code and message, adjust SQL or connection settings." + c.Hint = hintDBOther c.Retryable = false } return c @@ -172,24 +180,31 @@ func classifyPgError(e *pgconn.PgError) dbClassification { func classifyMySQLMessage(msg string) (dbClassification, bool) { lower := strings.ToLower(msg) switch { + case strings.Contains(lower, "access denied"): + return dbClassification{ + Category: CategoryAuthFailed, + Hint: hintAuthFailed, + Retryable: false, + NativeCodeKind: "mysql_errno", + }, true case strings.Contains(lower, "doesn't exist"): return dbClassification{ Category: CategoryUnknownTable, - Hint: "Table or database not found; run `xsql schema dump` to list tables, then fix the SQL.", + Hint: hintUnknownTableMySQL, Retryable: false, NativeCodeKind: "mysql_errno", }, true case strings.Contains(lower, "unknown column"): return dbClassification{ Category: CategoryUnknownColumn, - Hint: "Column not found; run `xsql schema dump --table ` to inspect columns, then fix the SQL.", + Hint: hintUnknownColumn, Retryable: false, NativeCodeKind: "mysql_errno", }, true case strings.Contains(lower, "syntax"): return dbClassification{ Category: CategorySyntaxError, - Hint: "Fix SQL syntax; compare with the profile dialect (mysql) and re-run.", + Hint: hintSyntaxMySQL, Retryable: false, NativeCodeKind: "mysql_errno", }, true @@ -201,24 +216,31 @@ func classifyMySQLMessage(msg string) (dbClassification, bool) { func classifyPgMessage(msg string) (dbClassification, bool) { lower := strings.ToLower(msg) switch { + case strings.Contains(lower, "password authentication failed"), strings.Contains(lower, "authentication failed"): + return dbClassification{ + Category: CategoryAuthFailed, + Hint: hintAuthFailed, + Retryable: false, + NativeCodeKind: "pg_sqlstate", + }, true case strings.Contains(lower, "does not exist") && strings.Contains(lower, "relation"): return dbClassification{ Category: CategoryUnknownTable, - Hint: "Table or schema not found; run `xsql schema dump` to list tables, then fix the SQL.", + Hint: hintUnknownTablePG, Retryable: false, NativeCodeKind: "pg_sqlstate", }, true case strings.Contains(lower, "column") && strings.Contains(lower, "does not exist"): return dbClassification{ Category: CategoryUnknownColumn, - Hint: "Column not found; run `xsql schema dump --table ` to inspect columns, then fix the SQL.", + Hint: hintUnknownColumn, Retryable: false, NativeCodeKind: "pg_sqlstate", }, true case strings.Contains(lower, "syntax error"): return dbClassification{ Category: CategorySyntaxError, - Hint: "Fix SQL syntax; compare with the profile dialect (pg) and re-run.", + Hint: hintSyntaxPG, Retryable: false, NativeCodeKind: "pg_sqlstate", }, true diff --git a/internal/errors/dbclassify_test.go b/internal/errors/dbclassify_test.go index 164a092c..46ef0bce 100644 --- a/internal/errors/dbclassify_test.go +++ b/internal/errors/dbclassify_test.go @@ -1,84 +1,101 @@ package errors import ( + "net" "strings" + "syscall" "testing" "github.com/go-sql-driver/mysql" "github.com/jackc/pgx/v5/pgconn" ) +type classifyWant struct { + category DBCategory + code string + kind string + retry bool + hintSub string +} + +func assertMySQLClassify(t *testing.T, number uint16, want classifyWant) { + t.Helper() + c := classifyMySQLError(&mysql.MySQLError{Number: number, Message: "driver message"}) + assertClassification(t, c, want) +} + +func assertPgClassify(t *testing.T, code string, want classifyWant) { + t.Helper() + c := classifyPgError(&pgconn.PgError{Code: code, Message: "driver message"}) + want.code = code + want.kind = "pg_sqlstate" + assertClassification(t, c, want) +} + +func assertClassification(t *testing.T, c dbClassification, want classifyWant) { + t.Helper() + if c.Category != want.category { + t.Errorf("category = %q, want %q", c.Category, want.category) + } + if want.code != "" && c.NativeCode != want.code { + t.Errorf("native_code = %q, want %q", c.NativeCode, want.code) + } + if want.kind != "" && c.NativeCodeKind != want.kind { + t.Errorf("native_code_kind = %q, want %q", c.NativeCodeKind, want.kind) + } + if c.Retryable != want.retry { + t.Errorf("retryable = %v, want %v", c.Retryable, want.retry) + } + if want.hintSub != "" && !strings.Contains(strings.ToLower(c.Hint), strings.ToLower(want.hintSub)) { + t.Errorf("hint %q should contain %q", c.Hint, want.hintSub) + } +} + func TestClassifyMySQLError(t *testing.T) { - tests := []struct { - name string - number uint16 - wantCat DBCategory - wantCode string - wantRetry bool - wantHintSub string + cases := map[string]struct { + number uint16 + want classifyWant }{ - {"unknown_table", 1146, CategoryUnknownTable, "1146", false, "schema dump"}, - {"unknown_column", 1054, CategoryUnknownColumn, "1054", false, "schema dump"}, - {"syntax", 1064, CategorySyntaxError, "1064", false, "syntax"}, - {"read_only_tx", 1792, CategoryReadOnlyViolation, "1792", false, "unsafe_allow_write"}, - {"connection", 2003, CategoryConnection, "2003", true, "host/port"}, - {"timeout", 1205, CategoryTimeout, "1205", true, "LIMIT"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - c := classifyMySQLError(&mysql.MySQLError{Number: tt.number, Message: "driver message"}) - if c.Category != tt.wantCat { - t.Errorf("category = %q, want %q", c.Category, tt.wantCat) - } - if c.NativeCode != tt.wantCode { - t.Errorf("native_code = %q, want %q", c.NativeCode, tt.wantCode) - } - if c.NativeCodeKind != "mysql_errno" { - t.Errorf("native_code_kind = %q", c.NativeCodeKind) - } - if c.Retryable != tt.wantRetry { - t.Errorf("retryable = %v, want %v", c.Retryable, tt.wantRetry) - } - if tt.wantHintSub != "" && !containsFold(c.Hint, tt.wantHintSub) { - t.Errorf("hint %q should contain %q", c.Hint, tt.wantHintSub) - } + "unknown_table": {1146, classifyWant{CategoryUnknownTable, "1146", "mysql_errno", false, "schema_list_tables"}}, + "unknown_db": {1049, classifyWant{CategoryUnknownTable, "1049", "mysql_errno", false, "schema dump"}}, + "unknown_column": {1054, classifyWant{CategoryUnknownColumn, "1054", "mysql_errno", false, "schema dump"}}, + "syntax": {1064, classifyWant{CategorySyntaxError, "1064", "mysql_errno", false, "syntax"}}, + "auth_failed": {1045, classifyWant{CategoryAuthFailed, "1045", "mysql_errno", false, "keyring"}}, + "permission": {1142, classifyWant{CategoryPermissionDenied, "1142", "mysql_errno", false, "privileges"}}, + "read_only_tx": {1792, classifyWant{CategoryReadOnlyViolation, "1792", "mysql_errno", false, "unsafe_allow_write"}}, + "duplicate_key": {1062, classifyWant{CategoryDuplicateKey, "1062", "mysql_errno", false, "Duplicate"}}, + "connection": {2003, classifyWant{CategoryConnection, "2003", "mysql_errno", true, "MySQL"}}, + "timeout": {1205, classifyWant{CategoryTimeout, "1205", "mysql_errno", true, "LIMIT"}}, + "other": {9999, classifyWant{CategoryOther, "9999", "mysql_errno", false, "native_code"}}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + assertMySQLClassify(t, tc.number, tc.want) }) } } func TestClassifyPgError(t *testing.T) { - tests := []struct { - name string - code string - wantCat DBCategory - wantRetry bool - wantHintSub string + cases := map[string]struct { + code string + want classifyWant }{ - {"unknown_table", "42P01", CategoryUnknownTable, false, "schema dump"}, - {"unknown_column", "42703", CategoryUnknownColumn, false, "schema dump"}, - {"syntax", "42601", CategorySyntaxError, false, "syntax"}, - {"read_only", "25006", CategoryReadOnlyViolation, false, "unsafe_allow_write"}, - {"connection", "08006", CategoryConnection, true, "PostgreSQL"}, - {"timeout", "57014", CategoryTimeout, true, "LIMIT"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - c := classifyPgError(&pgconn.PgError{Code: tt.code, Message: "driver message"}) - if c.Category != tt.wantCat { - t.Errorf("category = %q, want %q", c.Category, tt.wantCat) - } - if c.NativeCode != tt.code { - t.Errorf("native_code = %q, want %q", c.NativeCode, tt.code) - } - if c.NativeCodeKind != "pg_sqlstate" { - t.Errorf("native_code_kind = %q", c.NativeCodeKind) - } - if c.Retryable != tt.wantRetry { - t.Errorf("retryable = %v, want %v", c.Retryable, tt.wantRetry) - } - if tt.wantHintSub != "" && !containsFold(c.Hint, tt.wantHintSub) { - t.Errorf("hint %q should contain %q", c.Hint, tt.wantHintSub) - } + "unknown_table": {"42P01", classifyWant{CategoryUnknownTable, "", "", false, "schema dump"}}, + "unknown_column": {"42703", classifyWant{CategoryUnknownColumn, "", "", false, "schema dump"}}, + "syntax": {"42601", classifyWant{CategorySyntaxError, "", "", false, "syntax"}}, + "auth_failed": {"28P01", classifyWant{CategoryAuthFailed, "", "", false, "keyring"}}, + "auth_invalid": {"28000", classifyWant{CategoryAuthFailed, "", "", false, "Authentication"}}, + "permission": {"42501", classifyWant{CategoryPermissionDenied, "", "", false, "privileges"}}, + "read_only": {"25006", classifyWant{CategoryReadOnlyViolation, "", "", false, "unsafe_allow_write"}}, + "duplicate_key": {"23505", classifyWant{CategoryDuplicateKey, "", "", false, "Duplicate"}}, + "connection": {"08006", classifyWant{CategoryConnection, "", "", true, "PostgreSQL"}}, + "timeout": {"57014", classifyWant{CategoryTimeout, "", "", true, "LIMIT"}}, + "serialization": {"40001", classifyWant{CategoryTimeout, "", "", true, "Serialization"}}, + "other": {"99999", classifyWant{CategoryOther, "", "", false, "native_code"}}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + assertPgClassify(t, tc.code, tc.want) }) } } @@ -94,20 +111,50 @@ func TestClassifyDriverError_Chain(t *testing.T) { } } -func TestReadOnlyBlockedGuidance(t *testing.T) { - xe := New(CodeROBlocked, "write blocked", map[string]any{"reason": "forbidden_keyword: DELETE"}) - if xe.Details["category"] != "read_only_blocked" { +func TestClassifyDriverError_AuthOnConnect(t *testing.T) { + inner := &mysql.MySQLError{Number: 1045, Message: "Access denied for user 'root'@'localhost'"} + xe := Wrap(CodeDBConnectFailed, "failed to ping mysql", map[string]any{"dialect": "mysql"}, inner) + if xe.Details["category"] != string(CategoryAuthFailed) { t.Errorf("category = %v", xe.Details["category"]) } if xe.Details["retryable"] != false { - t.Errorf("retryable should be false") + t.Error("auth should not be retryable") + } +} + +func TestClassifyMySQLMessagePaths(t *testing.T) { + c, ok := classifyMySQLMessage("Access denied for user") + if !ok || c.Category != CategoryAuthFailed { + t.Fatalf("auth message: %+v ok=%v", c, ok) + } + c, ok = classifyMySQLMessage("Unknown column 'x' in 'field list'") + if !ok || c.Category != CategoryUnknownColumn { + t.Fatalf("column: %+v ok=%v", c, ok) } - hint, _ := xe.Details["hint"].(string) - if hint == "" || !containsFold(hint, "DELETE") && !containsFold(hint, "write keyword") { - t.Errorf("unexpected hint: %q", hint) +} + +func TestClassifyPgMessagePaths(t *testing.T) { + c, ok := classifyPgMessage(`password authentication failed for user "postgres"`) + if !ok || c.Category != CategoryAuthFailed { + t.Fatalf("auth: %+v ok=%v", c, ok) + } + c, ok = classifyPgMessage(`relation "missing" does not exist`) + if !ok || c.Category != CategoryUnknownTable { + t.Fatalf("table: %+v ok=%v", c, ok) } } -func containsFold(s, sub string) bool { - return strings.Contains(strings.ToLower(s), strings.ToLower(sub)) +func TestClassifyNetworkErrorBranches(t *testing.T) { + c, ok := classifyNetworkError(&net.DNSError{IsTimeout: true}) + if !ok || c.Category != CategoryTimeout { + t.Fatalf("timeout: %+v", c) + } + c, ok = classifyNetworkError(&net.OpError{Op: "dial", Err: syscall.ECONNREFUSED}) + if !ok || c.Category != CategoryConnection { + t.Fatalf("conn: %+v", c) + } + c, ok = classifyNetworkError(syscall.ECONNRESET) + if !ok || c.Category != CategoryConnection { + t.Fatalf("reset: %+v", c) + } } diff --git a/internal/errors/guidance.go b/internal/errors/guidance.go index eac9d734..e3c93c12 100644 --- a/internal/errors/guidance.go +++ b/internal/errors/guidance.go @@ -61,20 +61,48 @@ func guidanceFor(xe *XError) map[string]any { } } +const ( + categoryReadOnlyBlocked = "read_only_blocked" + categoryUnrecognizedStmt = "unrecognized_statement" + hintROWriteAuthorization = "Writes require profile `unsafe_allow_write: true` AND `--unsafe-allow-write` on `xsql query` / `xsql ai` or MCP server startup." + hintROForbiddenKeyword = "Statement contains a write keyword blocked by read-only policy; use SELECT-only SQL, or enable profile `unsafe_allow_write` and `--unsafe-allow-write` if the user explicitly requested writes." + hintROForbiddenStart = "Statement starts with a non-read-only command; use SELECT-only SQL, or enable profile `unsafe_allow_write` and `--unsafe-allow-write` if the user explicitly requested writes." + hintROUnrecognizedStatement = "Statement not recognized as read-only SQL; check spelling and syntax (must start with an allowed read-only keyword such as SELECT)." +) + func readOnlyGuidance(details map[string]any) map[string]any { + reason, _ := details["reason"].(string) + if isReadOnlyUnrecognizedReason(reason) { + return map[string]any{ + detailCategory: categoryUnrecognizedStmt, + detailRetryable: false, + detailHint: hintROUnrecognizedStatement, + } + } out := map[string]any{ - detailCategory: "read_only_blocked", + detailCategory: categoryReadOnlyBlocked, detailRetryable: false, - detailHint: "Writes require profile `unsafe_allow_write: true` AND `--unsafe-allow-write` on `xsql query` / `xsql ai` or MCP server startup.", + detailHint: hintROWriteAuthorization, } - if reason, _ := details["reason"].(string); reason != "" { - if strings.HasPrefix(reason, "forbidden_keyword:") { - out[detailHint] = "Statement contains a write keyword blocked by read-only policy; use SELECT-only SQL, or enable profile `unsafe_allow_write` and `--unsafe-allow-write` if the user explicitly requested writes." - } + switch { + case strings.HasPrefix(reason, "forbidden_keyword:"): + out[detailHint] = hintROForbiddenKeyword + case strings.HasPrefix(reason, "forbidden_start:"): + out[detailHint] = hintROForbiddenStart + case strings.HasPrefix(reason, "cte_write_operation"): + out[detailHint] = hintROForbiddenKeyword } return out } +func isReadOnlyUnrecognizedReason(reason string) bool { + switch reason { + case "no_keyword", "non_letter_start", "empty", "empty_after_comment", "invalid_outer_paren", "multiple_statements": + return true + } + return strings.HasPrefix(reason, "tokenize_error:") +} + func dbExecGuidance(xe *XError) map[string]any { dialect := dialectFromDetails(xe.Details) if xe.cause != nil { @@ -111,9 +139,9 @@ func connectGuidance(xe *XError) map[string]any { } msg := strings.ToLower(xe.cause.Error()) if xe.Code == CodeDBAuthFailed || strings.Contains(msg, "access denied") || strings.Contains(msg, "password authentication failed") { - out[detailCategory] = string(CategoryPermissionDenied) + out[detailCategory] = string(CategoryAuthFailed) out[detailRetryable] = false - out[detailHint] = "Authentication failed; verify username/password or keyring secret for this profile (avoid plaintext in logs)." + out[detailHint] = hintAuthFailed } } return out diff --git a/internal/errors/guidance_test.go b/internal/errors/guidance_test.go index 1228e7e1..52203b04 100644 --- a/internal/errors/guidance_test.go +++ b/internal/errors/guidance_test.go @@ -3,6 +3,7 @@ package errors import ( "errors" "net" + "strings" "syscall" "testing" @@ -23,7 +24,7 @@ func TestMergeDetails_PreservesExisting(t *testing.T) { func TestConnectGuidance_Auth(t *testing.T) { xe := Wrap(CodeDBAuthFailed, "auth failed", map[string]any{"dialect": "mysql"}, errors.New("access denied for user")) - if xe.Details["category"] != string(CategoryPermissionDenied) { + if xe.Details["category"] != string(CategoryAuthFailed) { t.Errorf("category = %v", xe.Details["category"]) } if xe.Details["retryable"] != false { @@ -97,29 +98,55 @@ func TestDBExecGuidance_Fallback(t *testing.T) { } func TestReadOnlyGuidance_DefaultHint(t *testing.T) { - xe := New(CodeROBlocked, "blocked", map[string]any{"reason": "not_select"}) + xe := New(CodeROBlocked, "blocked", map[string]any{"reason": "forbidden_share_lock"}) + if xe.Details["category"] != categoryReadOnlyBlocked { + t.Errorf("category = %v", xe.Details["category"]) + } if xe.Details["hint"] == "" { t.Fatal("expected hint") } } -func TestDialectFromDetails(t *testing.T) { - if dialectFromDetails(map[string]any{"db": "postgresql"}) != "pg" { - t.Error("expected pg from db field") +func TestReadOnlyGuidance_NoKeyword(t *testing.T) { + xe := New(CodeROBlocked, "blocked", map[string]any{"reason": "no_keyword"}) + if xe.Details["category"] != categoryUnrecognizedStmt { + t.Errorf("category = %v", xe.Details["category"]) + } + hint, _ := xe.Details["hint"].(string) + if !containsFold(hint, "spelling") { + t.Errorf("hint = %q", hint) } } -func TestClassifyMySQLMessagePaths(t *testing.T) { - c, ok := classifyMySQLMessage("Unknown column 'x' in 'field list'") - if !ok || c.Category != CategoryUnknownColumn { - t.Fatalf("got %+v ok=%v", c, ok) +func TestReadOnlyGuidance_TypoSELEC(t *testing.T) { + xe := New(CodeROBlocked, "blocked", map[string]any{"reason": "no_keyword"}) + if containsFold(xe.Details["hint"].(string), "unsafe_allow_write") { + t.Error("typo should not suggest enabling writes") } } -func TestClassifyPgError_Default(t *testing.T) { - c := classifyPgError(&pgconn.PgError{Code: "99999", Message: "weird"}) - if c.Category != CategoryOther { - t.Errorf("category = %q", c.Category) +func TestReadOnlyBlockedGuidance_ForbiddenKeyword(t *testing.T) { + xe := New(CodeROBlocked, "write blocked", map[string]any{"reason": "forbidden_keyword: DELETE"}) + if xe.Details["category"] != categoryReadOnlyBlocked { + t.Errorf("category = %v", xe.Details["category"]) + } + hint, _ := xe.Details["hint"].(string) + if !containsFold(hint, "write keyword") { + t.Errorf("unexpected hint: %q", hint) + } +} + +func TestConnectGuidance_PGAuthSQLState(t *testing.T) { + inner := &pgconn.PgError{Code: "28P01", Message: "password authentication failed"} + xe := Wrap(CodeDBConnectFailed, "failed to ping pg", map[string]any{"dialect": "pg"}, inner) + if xe.Details["category"] != string(CategoryAuthFailed) { + t.Errorf("category = %v", xe.Details["category"]) + } +} + +func TestDialectFromDetails(t *testing.T) { + if dialectFromDetails(map[string]any{"db": "postgresql"}) != "pg" { + t.Error("expected pg from db field") } } @@ -136,3 +163,7 @@ func TestMergeDetails_NilBase(t *testing.T) { t.Errorf("merged = %v", merged) } } + +func containsFold(s, sub string) bool { + return strings.Contains(strings.ToLower(s), strings.ToLower(sub)) +} diff --git a/internal/errors/hints.go b/internal/errors/hints.go new file mode 100644 index 00000000..35159cce --- /dev/null +++ b/internal/errors/hints.go @@ -0,0 +1,22 @@ +package errors + +// Shared agent hint strings (single source for Sonar duplicate-literal rules). +const ( + hintUnknownTableMySQL = "Table or database not found; use MCP `schema_list_tables` or `xsql schema dump` to list valid tables, then fix the SQL." + hintUnknownTablePG = "Table or schema not found; use MCP `schema_list_tables` or `xsql schema dump` to list tables, then fix the SQL." + hintUnknownColumn = "Column not found; run `xsql schema dump --table ` to inspect columns, then fix the SQL." + hintSyntaxMySQL = "Fix SQL syntax; compare with the profile dialect (mysql) and re-run." + hintSyntaxPG = "Fix SQL syntax; compare with the profile dialect (pg) and re-run." + hintPermissionDenied = "Insufficient database privileges; use a read-only SELECT or ask for a profile with required grants." + hintAuthFailed = "Authentication failed; verify profile username/password or keyring secret reference for this profile." + hintReadOnlyWrite = "Write blocked in read-only mode; set profile `unsafe_allow_write: true` AND pass `--unsafe-allow-write` on query/ai/mcp server." + hintReadOnlyWritePG = "Write blocked in read-only transaction; set profile `unsafe_allow_write: true` AND pass `--unsafe-allow-write` on query/ai/mcp server." + hintDuplicateKey = "Duplicate key conflict; adjust INSERT/UPDATE values or use ON DUPLICATE handling if writes are explicitly allowed." + hintDuplicateKeyPG = "Duplicate key conflict; adjust values or constraints if writes are explicitly allowed." + hintMySQLConnection = "Cannot reach MySQL; verify profile host/port, network, and ssh_proxy settings." + hintPGConnection = "Cannot reach PostgreSQL; verify profile host/port, sslmode, and ssh_proxy settings." + hintQueryTimeoutMySQL = "Query timed out or lock wait exceeded; narrow the query, add LIMIT, or retry later." + hintQueryTimeoutPG = "Query canceled or lock timeout; narrow the query, add LIMIT, or retry later." + hintSerializationPG = "Serialization or deadlock failure; retry the read query or simplify concurrent access." + hintDBOther = "Database returned an error; inspect native_code and message, adjust SQL or connection settings." +) diff --git a/tests/integration/error_guidance_test.go b/tests/integration/error_guidance_test.go index 7ecff760..cefaa051 100644 --- a/tests/integration/error_guidance_test.go +++ b/tests/integration/error_guidance_test.go @@ -5,155 +5,176 @@ package integration import ( "context" "os" + "strings" "testing" "time" "github.com/zx06/xsql/internal/db" "github.com/zx06/xsql/internal/errors" - _ "github.com/zx06/xsql/internal/db/mysql" - _ "github.com/zx06/xsql/internal/db/pg" -) -func assertGuidance(t *testing.T, xe *errors.XError, wantCategory string, wantHintSub string) { - t.Helper() - if xe == nil { - t.Fatal("expected error") - } - if xe.Details == nil { - t.Fatal("expected error details") - } - cat, _ := xe.Details["category"].(string) - if cat != wantCategory { - t.Errorf("category = %q, want %q", cat, wantCategory) - } - hint, _ := xe.Details["hint"].(string) - if hint == "" { - t.Error("expected non-empty hint") - } else if wantHintSub != "" && !containsInsensitive(hint, wantHintSub) { - t.Errorf("hint %q should mention %q", hint, wantHintSub) - } - if _, ok := xe.Details["retryable"].(bool); !ok { - t.Error("expected retryable bool in details") - } -} + _ "github.com/zx06/xsql/internal/db/mysql" // register MySQL driver + _ "github.com/zx06/xsql/internal/db/pg" // register PostgreSQL driver +) -func containsInsensitive(s, sub string) bool { - return len(sub) == 0 || (len(s) >= len(sub) && stringIndexInsensitive(s, sub) >= 0) +type errorGuidanceCase struct { + name string + driver string + dsnEnv string + dbType string + dsnOverride string + openOnly bool + sql string + wantCode errors.Code + wantCategory string + wantHintSub string + checkDialect string } -func stringIndexInsensitive(s, sub string) int { - ls := len(s) - lsub := len(sub) - for i := 0; i+lsub <= ls; i++ { - if equalFoldASCII(s[i:i+lsub], sub) { - return i - } +func TestErrorGuidance(t *testing.T) { + mysqlDSN := os.Getenv("XSQL_TEST_MYSQL_DSN") + pgDSN := os.Getenv("XSQL_TEST_PG_DSN") + if mysqlDSN == "" && pgDSN == "" { + t.Skip("XSQL_TEST_MYSQL_DSN and XSQL_TEST_PG_DSN not set") } - return -1 -} -func equalFoldASCII(a, b string) bool { - if len(a) != len(b) { - return false + cases := []errorGuidanceCase{ + { + name: "mysql_unknown_table", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", + sql: "SELECT * FROM definitely_nonexistent_table_12345", + wantCode: errors.CodeDBExecFailed, wantCategory: "unknown_table", wantHintSub: "schema", + checkDialect: "mysql", + }, + { + name: "mysql_syntax_error", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", + sql: "SELECT 1 +", + wantCode: errors.CodeDBExecFailed, wantCategory: "syntax_error", wantHintSub: "syntax", + }, + { + name: "mysql_read_only_write", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", + sql: "DELETE FROM nonexistent_table_xsql_ro", + wantCode: errors.CodeROBlocked, wantCategory: "read_only_blocked", wantHintSub: "unsafe_allow_write", + }, + { + name: "mysql_unrecognized_typo", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", + sql: "SELEC 1", + wantCode: errors.CodeROBlocked, wantCategory: "unrecognized_statement", wantHintSub: "spelling", + }, + { + name: "mysql_auth_failed", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", + openOnly: true, dsnOverride: badMySQLDSN(mysqlDSN), + wantCode: errors.CodeDBConnectFailed, wantCategory: "auth_failed", wantHintSub: "keyring", + }, + { + name: "pg_unknown_table", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", + sql: "SELECT * FROM definitely_nonexistent_table_12345", + wantCode: errors.CodeDBExecFailed, wantCategory: "unknown_table", wantHintSub: "schema", + }, + { + name: "pg_syntax_error", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", + sql: "SELECT 1 +", + wantCode: errors.CodeDBExecFailed, wantCategory: "syntax_error", wantHintSub: "syntax", + }, + { + name: "pg_read_only_write", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", + sql: "DELETE FROM nonexistent_table_xsql_ro", + wantCode: errors.CodeROBlocked, wantCategory: "read_only_blocked", wantHintSub: "unsafe_allow_write", + }, + { + name: "pg_unrecognized_typo", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", + sql: "SELEC 1", + wantCode: errors.CodeROBlocked, wantCategory: "unrecognized_statement", wantHintSub: "spelling", + }, + { + name: "pg_auth_failed", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", + openOnly: true, dsnOverride: badPgDSN(pgDSN), + wantCode: errors.CodeDBConnectFailed, wantCategory: "auth_failed", wantHintSub: "Authentication", + }, } - for i := 0; i < len(a); i++ { - ca, cb := a[i], b[i] - if ca >= 'A' && ca <= 'Z' { - ca += 'a' - 'A' - } - if cb >= 'A' && cb <= 'Z' { - cb += 'a' - 'A' - } - if ca != cb { - return false - } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + runErrorGuidanceCase(t, tc) + }) } - return true } -func TestErrorGuidance_MySQL(t *testing.T) { - dsn := os.Getenv("XSQL_TEST_MYSQL_DSN") +func runErrorGuidanceCase(t *testing.T, tc errorGuidanceCase) { + t.Helper() + + dsn := tc.dsnOverride if dsn == "" { - t.Skip("XSQL_TEST_MYSQL_DSN not set") + dsn = os.Getenv(tc.dsnEnv) + } + if dsn == "" { + t.Skipf("%s not set", tc.dsnEnv) + } + + drv, ok := db.Get(tc.driver) + if !ok { + t.Fatalf("driver %q not registered", tc.driver) } - runMySQLGuidanceCases(t, dsn) -} -func runMySQLGuidanceCases(t *testing.T, dsn string) { - drv, _ := db.Get("mysql") ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() conn, xe := drv.Open(ctx, db.ConnOptions{DSN: dsn}) + if tc.openOnly { + assertErrorGuidance(t, xe, tc.wantCode, tc.wantCategory, tc.wantHintSub) + return + } if xe != nil { t.Fatalf("open: %v", xe) } defer conn.Close() - t.Run("unknown_table", func(t *testing.T) { - _, xe := db.Query(ctx, conn, "SELECT * FROM definitely_nonexistent_table_12345", db.QueryOptions{DBType: "mysql"}) - if xe == nil || xe.Code != errors.CodeDBExecFailed { - t.Fatalf("expected exec failed, got %v", xe) - } - assertGuidance(t, xe, "unknown_table", "schema dump") - if xe.Details["dialect"] != "mysql" { - t.Errorf("dialect = %v", xe.Details["dialect"]) - } - }) - - t.Run("syntax_error", func(t *testing.T) { - _, xe := db.Query(ctx, conn, "SELECT 1 +", db.QueryOptions{DBType: "mysql"}) - if xe == nil || xe.Code != errors.CodeDBExecFailed { - t.Fatalf("expected exec failed, got %v", xe) - } - assertGuidance(t, xe, "syntax_error", "syntax") - }) - - t.Run("read_only_blocked", func(t *testing.T) { - _, xe := db.Query(ctx, conn, "DELETE FROM nonexistent_table_xsql_ro", db.QueryOptions{DBType: "mysql"}) - if xe == nil || xe.Code != errors.CodeROBlocked { - t.Fatalf("expected RO blocked, got %v", xe) - } - assertGuidance(t, xe, "read_only_blocked", "unsafe_allow_write") - }) + _, xe = db.Query(ctx, conn, tc.sql, db.QueryOptions{DBType: tc.dbType}) + assertErrorGuidance(t, xe, tc.wantCode, tc.wantCategory, tc.wantHintSub) + if tc.checkDialect != "" && xe != nil && xe.Details["dialect"] != tc.checkDialect { + t.Errorf("dialect = %v, want %q", xe.Details["dialect"], tc.checkDialect) + } } -func TestErrorGuidance_PostgreSQL(t *testing.T) { - dsn := os.Getenv("XSQL_TEST_PG_DSN") - if dsn == "" { - t.Skip("XSQL_TEST_PG_DSN not set") +func assertErrorGuidance(t *testing.T, xe *errors.XError, wantCode errors.Code, wantCategory, wantHintSub string) { + t.Helper() + if xe == nil { + t.Fatal("expected error") } - drv, _ := db.Get("pg") - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) - defer cancel() - - conn, xe := drv.Open(ctx, db.ConnOptions{DSN: dsn}) - if xe != nil { - t.Fatalf("open: %v", xe) + if xe.Code != wantCode { + t.Fatalf("code = %s, want %s", xe.Code, wantCode) } - defer conn.Close() - - t.Run("unknown_table", func(t *testing.T) { - _, xe := db.Query(ctx, conn, "SELECT * FROM definitely_nonexistent_table_12345", db.QueryOptions{DBType: "pg"}) - if xe == nil || xe.Code != errors.CodeDBExecFailed { - t.Fatalf("expected exec failed, got %v", xe) - } - assertGuidance(t, xe, "unknown_table", "schema dump") - }) + if xe.Details == nil { + t.Fatal("expected error details") + } + cat, _ := xe.Details["category"].(string) + if cat != wantCategory { + t.Errorf("category = %q, want %q", cat, wantCategory) + } + hint, _ := xe.Details["hint"].(string) + if hint == "" { + t.Error("expected non-empty hint") + } else if wantHintSub != "" && !strings.Contains(strings.ToLower(hint), strings.ToLower(wantHintSub)) { + t.Errorf("hint %q should mention %q", hint, wantHintSub) + } + if _, ok := xe.Details["retryable"].(bool); !ok { + t.Error("expected retryable bool in details") + } +} - t.Run("syntax_error", func(t *testing.T) { - _, xe := db.Query(ctx, conn, "SELECT 1 +", db.QueryOptions{DBType: "pg"}) - if xe == nil || xe.Code != errors.CodeDBExecFailed { - t.Fatalf("expected exec failed, got %v", xe) +func badMySQLDSN(good string) string { + if good == "" { + return "" + } + if i := strings.Index(good, "@"); i > 0 { + if j := strings.Index(good, ":"); j > 0 && j < i { + return good[:j+1] + "xsql_wrong_password@" + good[i+1:] } - assertGuidance(t, xe, "syntax_error", "syntax") - }) + } + return good + "_invalid" +} - t.Run("read_only_blocked", func(t *testing.T) { - _, xe := db.Query(ctx, conn, "DELETE FROM nonexistent_table_xsql_ro", db.QueryOptions{DBType: "pg"}) - if xe == nil || xe.Code != errors.CodeROBlocked { - t.Fatalf("expected RO blocked, got %v", xe) - } - assertGuidance(t, xe, "read_only_blocked", "unsafe_allow_write") - }) +func badPgDSN(good string) string { + if good == "" { + return "" + } + return strings.Replace(good, "postgres:postgres@", "postgres:xsql_wrong_password@", 1) } From b316d0ec6ed497a225580615c0e6a3a00a2a5826 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 10 Oct 2026 06:44:13 +0000 Subject: [PATCH 5/5] test: dedupe integration error guidance cases for Sonar Co-authored-by: x_zhuo --- internal/errors/dbclassify_test.go | 23 ++++ internal/errors/guidance_test.go | 19 +++ tests/integration/error_guidance_test.go | 160 ++++++++++++++--------- 3 files changed, 139 insertions(+), 63 deletions(-) diff --git a/internal/errors/dbclassify_test.go b/internal/errors/dbclassify_test.go index 46ef0bce..40fbb0e1 100644 --- a/internal/errors/dbclassify_test.go +++ b/internal/errors/dbclassify_test.go @@ -1,6 +1,7 @@ package errors import ( + stderrors "errors" "net" "strings" "syscall" @@ -144,6 +145,28 @@ func TestClassifyPgMessagePaths(t *testing.T) { } } +func TestClassifyDriverError_MessageFallback(t *testing.T) { + c, ok := classifyDriverError("mysql", stderrors.New("Error 1064: syntax error")) + if !ok || c.Category != CategorySyntaxError { + t.Fatalf("mysql message fallback: %+v ok=%v", c, ok) + } + c, ok = classifyDriverError("pg", stderrors.New(`ERROR: syntax error at or near "FROM"`)) + if !ok || c.Category != CategorySyntaxError { + t.Fatalf("pg message fallback: %+v ok=%v", c, ok) + } + _, ok = classifyDriverError("mysql", stderrors.New("something else entirely")) + if ok { + t.Fatal("expected no classification") + } +} + +func TestClassifyMySQLMessage_noMatch(t *testing.T) { + _, ok := classifyMySQLMessage("generic database failure") + if ok { + t.Fatal("expected no match") + } +} + func TestClassifyNetworkErrorBranches(t *testing.T) { c, ok := classifyNetworkError(&net.DNSError{IsTimeout: true}) if !ok || c.Category != CategoryTimeout { diff --git a/internal/errors/guidance_test.go b/internal/errors/guidance_test.go index 52203b04..d84d2764 100644 --- a/internal/errors/guidance_test.go +++ b/internal/errors/guidance_test.go @@ -125,6 +125,25 @@ func TestReadOnlyGuidance_TypoSELEC(t *testing.T) { } } +func TestReadOnlyGuidance_ForbiddenStart(t *testing.T) { + xe := New(CodeROBlocked, "blocked", map[string]any{"reason": "forbidden_start: INSERT"}) + if xe.Details["category"] != categoryReadOnlyBlocked { + t.Errorf("category = %v", xe.Details["category"]) + } + if !containsFold(xe.Details["hint"].(string), "non-read-only") { + t.Errorf("hint = %v", xe.Details["hint"]) + } +} + +func TestIsReadOnlyUnrecognizedReason(t *testing.T) { + if !isReadOnlyUnrecognizedReason("tokenize_error: bad") { + t.Error("expected tokenize_error prefix") + } + if isReadOnlyUnrecognizedReason("forbidden_keyword: DELETE") { + t.Error("write keyword should not be unrecognized") + } +} + func TestReadOnlyBlockedGuidance_ForbiddenKeyword(t *testing.T) { xe := New(CodeROBlocked, "write blocked", map[string]any{"reason": "forbidden_keyword: DELETE"}) if xe.Details["category"] != categoryReadOnlyBlocked { diff --git a/tests/integration/error_guidance_test.go b/tests/integration/error_guidance_test.go index cefaa051..b97c671f 100644 --- a/tests/integration/error_guidance_test.go +++ b/tests/integration/error_guidance_test.go @@ -4,6 +4,7 @@ package integration import ( "context" + "fmt" "os" "strings" "testing" @@ -16,86 +17,119 @@ import ( _ "github.com/zx06/xsql/internal/db/pg" // register PostgreSQL driver ) -type errorGuidanceCase struct { - name string +type errorGuidanceEngine struct { + label string driver string dsnEnv string dbType string - dsnOverride string - openOnly bool + dsn string + badDSN func(string) string + authHintSub string +} + +type errorGuidanceScenario struct { + suffix string sql string + openOnly bool + useBadDSN bool wantCode errors.Code wantCategory string wantHintSub string - checkDialect string + checkDialect bool } -func TestErrorGuidance(t *testing.T) { - mysqlDSN := os.Getenv("XSQL_TEST_MYSQL_DSN") - pgDSN := os.Getenv("XSQL_TEST_PG_DSN") - if mysqlDSN == "" && pgDSN == "" { - t.Skip("XSQL_TEST_MYSQL_DSN and XSQL_TEST_PG_DSN not set") - } +var errorGuidanceScenarios = []errorGuidanceScenario{ + { + suffix: "unknown_table", sql: "SELECT * FROM definitely_nonexistent_table_12345", + wantCode: errors.CodeDBExecFailed, wantCategory: "unknown_table", wantHintSub: "schema", checkDialect: true, + }, + { + suffix: "syntax_error", sql: "SELECT 1 +", + wantCode: errors.CodeDBExecFailed, wantCategory: "syntax_error", wantHintSub: "syntax", + }, + { + suffix: "read_only_write", sql: "DELETE FROM nonexistent_table_xsql_ro", + wantCode: errors.CodeROBlocked, wantCategory: "read_only_blocked", wantHintSub: "unsafe_allow_write", + }, + { + suffix: "unrecognized_typo", sql: "SELEC 1", + wantCode: errors.CodeROBlocked, wantCategory: "unrecognized_statement", wantHintSub: "spelling", + }, + { + suffix: "auth_failed", openOnly: true, useBadDSN: true, + wantCode: errors.CodeDBConnectFailed, wantCategory: "auth_failed", + }, +} - cases := []errorGuidanceCase{ - { - name: "mysql_unknown_table", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", - sql: "SELECT * FROM definitely_nonexistent_table_12345", - wantCode: errors.CodeDBExecFailed, wantCategory: "unknown_table", wantHintSub: "schema", - checkDialect: "mysql", - }, - { - name: "mysql_syntax_error", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", - sql: "SELECT 1 +", - wantCode: errors.CodeDBExecFailed, wantCategory: "syntax_error", wantHintSub: "syntax", - }, - { - name: "mysql_read_only_write", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", - sql: "DELETE FROM nonexistent_table_xsql_ro", - wantCode: errors.CodeROBlocked, wantCategory: "read_only_blocked", wantHintSub: "unsafe_allow_write", - }, - { - name: "mysql_unrecognized_typo", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", - sql: "SELEC 1", - wantCode: errors.CodeROBlocked, wantCategory: "unrecognized_statement", wantHintSub: "spelling", - }, - { - name: "mysql_auth_failed", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", - openOnly: true, dsnOverride: badMySQLDSN(mysqlDSN), - wantCode: errors.CodeDBConnectFailed, wantCategory: "auth_failed", wantHintSub: "keyring", - }, - { - name: "pg_unknown_table", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", - sql: "SELECT * FROM definitely_nonexistent_table_12345", - wantCode: errors.CodeDBExecFailed, wantCategory: "unknown_table", wantHintSub: "schema", - }, - { - name: "pg_syntax_error", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", - sql: "SELECT 1 +", - wantCode: errors.CodeDBExecFailed, wantCategory: "syntax_error", wantHintSub: "syntax", - }, - { - name: "pg_read_only_write", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", - sql: "DELETE FROM nonexistent_table_xsql_ro", - wantCode: errors.CodeROBlocked, wantCategory: "read_only_blocked", wantHintSub: "unsafe_allow_write", - }, +func TestErrorGuidance(t *testing.T) { + engines := []errorGuidanceEngine{ { - name: "pg_unrecognized_typo", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", - sql: "SELEC 1", - wantCode: errors.CodeROBlocked, wantCategory: "unrecognized_statement", wantHintSub: "spelling", + label: "mysql", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", + dsn: os.Getenv("XSQL_TEST_MYSQL_DSN"), badDSN: badMySQLDSN, authHintSub: "keyring", }, { - name: "pg_auth_failed", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", - openOnly: true, dsnOverride: badPgDSN(pgDSN), - wantCode: errors.CodeDBConnectFailed, wantCategory: "auth_failed", wantHintSub: "Authentication", + label: "pg", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", + dsn: os.Getenv("XSQL_TEST_PG_DSN"), badDSN: badPgDSN, authHintSub: "Authentication", }, } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - runErrorGuidanceCase(t, tc) - }) + var runnable int + for _, eng := range engines { + if eng.dsn != "" { + runnable++ + } + } + if runnable == 0 { + t.Skip("XSQL_TEST_MYSQL_DSN and XSQL_TEST_PG_DSN not set") + } + + for _, eng := range engines { + if eng.dsn == "" { + continue + } + for _, sc := range errorGuidanceScenarios { + t.Run(fmt.Sprintf("%s_%s", eng.label, sc.suffix), func(t *testing.T) { + runErrorGuidanceCase(t, guidanceCaseFrom(eng, sc)) + }) + } + } +} + +func guidanceCaseFrom(eng errorGuidanceEngine, sc errorGuidanceScenario) errorGuidanceCase { + hintSub := sc.wantHintSub + if sc.useBadDSN { + hintSub = eng.authHintSub + } + tc := errorGuidanceCase{ + driver: eng.driver, + dsnEnv: eng.dsnEnv, + dbType: eng.dbType, + openOnly: sc.openOnly, + sql: sc.sql, + wantCode: sc.wantCode, + wantCategory: sc.wantCategory, + wantHintSub: hintSub, + } + if sc.useBadDSN { + tc.dsnOverride = eng.badDSN(eng.dsn) + } + if sc.checkDialect { + tc.checkDialect = eng.dbType } + return tc +} + +type errorGuidanceCase struct { + driver string + dsnEnv string + dbType string + dsnOverride string + openOnly bool + sql string + wantCode errors.Code + wantCategory string + wantHintSub string + checkDialect string } func runErrorGuidanceCase(t *testing.T, tc errorGuidanceCase) {