diff --git a/.agents/skills/xsql/SKILL.md b/.agents/skills/xsql/SKILL.md index a1051d98..3501fc6e 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`, `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 | + +Handle failures by `error.code`, `details.category`, and exit status: | Exit Code | Meaning | |-----------|---------| diff --git a/docs/ai.md b/docs/ai.md index 0a475c83..19d2d805 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..be724ab1 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`、`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` | + +只读拦截等既有字段（如 `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/0016-error-guidance-for-agents.md b/docs/rfcs/0016-error-guidance-for-agents.md new file mode 100644 index 00000000..cd82591e --- /dev/null +++ b/docs/rfcs/0016-error-guidance-for-agents.md @@ -0,0 +1,39 @@ +# RFC 0016: 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/docs/rfcs/README.md b/docs/rfcs/README.md index f3700cab..60c210cf 100644 --- a/docs/rfcs/README.md +++ b/docs/rfcs/README.md @@ -21,6 +21,7 @@ | [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 | | [0015](0015-mcp-app-layer-schema-tools.md) | MCP 复用 app 层与 Schema Tools | Implemented | +| [0016](0016-error-guidance-for-agents.md) | AI Agent 错误指引（Error Guidance） | Accepted | ## 何时需要 RFC 当变更会影响以下任一项时，必须先提交/更新 RFC（并同步更新对应 `docs/*`）： 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..42c280bf --- /dev/null +++ b/internal/errors/dbclassify.go @@ -0,0 +1,299 @@ +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" + CategoryAuthFailed DBCategory = "auth_failed" + 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 + } + + 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 = hintUnknownTableMySQL + c.Retryable = false + case 1054, 1109: + c.Category = CategoryUnknownColumn + c.Hint = hintUnknownColumn + c.Retryable = false + case 1064, 1149, 1060: + c.Category = CategorySyntaxError + c.Hint = hintSyntaxMySQL + c.Retryable = false + case 1045: + c.Category = CategoryAuthFailed + c.Hint = hintAuthFailed + c.Retryable = false + case 1142, 1227, 1044: + c.Category = CategoryPermissionDenied + c.Hint = hintPermissionDenied + c.Retryable = false + case 1205, 1213, 3024: + c.Category = CategoryTimeout + c.Hint = hintQueryTimeoutMySQL + c.Retryable = true + case 1792, 1836: + c.Category = CategoryReadOnlyViolation + c.Hint = hintReadOnlyWrite + c.Retryable = false + case 1062: + c.Category = CategoryDuplicateKey + c.Hint = hintDuplicateKey + c.Retryable = false + case 2002, 2003, 2006, 2013: + c.Category = CategoryConnection + c.Hint = hintMySQLConnection + c.Retryable = true + default: + c.Category = CategoryOther + c.Hint = hintDBOther + 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 = hintUnknownTablePG + c.Retryable = false + case "42703": + c.Category = CategoryUnknownColumn + c.Hint = hintUnknownColumn + c.Retryable = false + case "42601", "42803", "42809": + c.Category = CategorySyntaxError + 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 = hintPermissionDenied + c.Retryable = false + case "57014", "55P03": + c.Category = CategoryTimeout + c.Hint = hintQueryTimeoutPG + c.Retryable = true + case "25006", "25007": + c.Category = CategoryReadOnlyViolation + c.Hint = hintReadOnlyWritePG + c.Retryable = false + case "23505": + c.Category = CategoryDuplicateKey + c.Hint = hintDuplicateKeyPG + c.Retryable = false + case "08000", "08001", "08003", "08006", "57P01", "53300": + c.Category = CategoryConnection + c.Hint = hintPGConnection + c.Retryable = true + case "40001", "40P01": + c.Category = CategoryTimeout + c.Hint = hintSerializationPG + c.Retryable = true + default: + c.Category = CategoryOther + c.Hint = hintDBOther + c.Retryable = false + } + return c +} + +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: hintUnknownTableMySQL, + Retryable: false, + NativeCodeKind: "mysql_errno", + }, true + case strings.Contains(lower, "unknown column"): + return dbClassification{ + Category: CategoryUnknownColumn, + Hint: hintUnknownColumn, + Retryable: false, + NativeCodeKind: "mysql_errno", + }, true + case strings.Contains(lower, "syntax"): + return dbClassification{ + Category: CategorySyntaxError, + Hint: hintSyntaxMySQL, + 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, "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: hintUnknownTablePG, + Retryable: false, + NativeCodeKind: "pg_sqlstate", + }, true + case strings.Contains(lower, "column") && strings.Contains(lower, "does not exist"): + return dbClassification{ + Category: CategoryUnknownColumn, + Hint: hintUnknownColumn, + Retryable: false, + NativeCodeKind: "pg_sqlstate", + }, true + case strings.Contains(lower, "syntax error"): + return dbClassification{ + Category: CategorySyntaxError, + Hint: hintSyntaxPG, + 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..40fbb0e1 --- /dev/null +++ b/internal/errors/dbclassify_test.go @@ -0,0 +1,183 @@ +package errors + +import ( + stderrors "errors" + "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) { + cases := map[string]struct { + number uint16 + want classifyWant + }{ + "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) { + cases := map[string]struct { + code string + want classifyWant + }{ + "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) + }) + } +} + +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 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.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) + } +} + +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 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 { + 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/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..e3c93c12 --- /dev/null +++ b/internal/errors/guidance.go @@ -0,0 +1,231 @@ +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 + } +} + +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: categoryReadOnlyBlocked, + detailRetryable: false, + detailHint: hintROWriteAuthorization, + } + 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 { + 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(CategoryAuthFailed) + out[detailRetryable] = false + out[detailHint] = hintAuthFailed + } + } + 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 { + if reason, _ := xe.Details["reason"].(string); reason == "table_not_found" { + return map[string]any{ + detailCategory: string(CategoryUnknownTable), + detailRetryable: false, + detailHint: "Table not found; call MCP tool `schema_list_tables` (or `xsql schema dump` / `schema describe`) to list valid tables, then retry with the correct name or `table_pattern`.", + } + } + + 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..d84d2764 --- /dev/null +++ b/internal/errors/guidance_test.go @@ -0,0 +1,188 @@ +package errors + +import ( + "errors" + "net" + "strings" + "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(CategoryAuthFailed) { + 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 TestConfigGuidance_TableNotFound(t *testing.T) { + xe := New(CodeCfgInvalid, "table not found", map[string]any{ + "reason": "table_not_found", + "schema": "public", + "name": "missing_tbl", + }) + if xe.Details["category"] != string(CategoryUnknownTable) { + t.Errorf("category = %v", xe.Details["category"]) + } + hint, _ := xe.Details["hint"].(string) + if !containsFold(hint, "schema_list_tables") { + t.Errorf("hint = %q", 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": "forbidden_share_lock"}) + if xe.Details["category"] != categoryReadOnlyBlocked { + t.Errorf("category = %v", xe.Details["category"]) + } + if xe.Details["hint"] == "" { + t.Fatal("expected hint") + } +} + +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 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 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 { + 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") + } +} + +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) + } +} + +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/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) { diff --git a/tests/integration/error_guidance_test.go b/tests/integration/error_guidance_test.go new file mode 100644 index 00000000..b97c671f --- /dev/null +++ b/tests/integration/error_guidance_test.go @@ -0,0 +1,214 @@ +//go:build integration + +package integration + +import ( + "context" + "fmt" + "os" + "strings" + "testing" + "time" + + "github.com/zx06/xsql/internal/db" + "github.com/zx06/xsql/internal/errors" + + _ "github.com/zx06/xsql/internal/db/mysql" // register MySQL driver + _ "github.com/zx06/xsql/internal/db/pg" // register PostgreSQL driver +) + +type errorGuidanceEngine struct { + label string + driver string + dsnEnv string + dbType string + 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 bool +} + +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", + }, +} + +func TestErrorGuidance(t *testing.T) { + engines := []errorGuidanceEngine{ + { + label: "mysql", driver: "mysql", dsnEnv: "XSQL_TEST_MYSQL_DSN", dbType: "mysql", + dsn: os.Getenv("XSQL_TEST_MYSQL_DSN"), badDSN: badMySQLDSN, authHintSub: "keyring", + }, + { + label: "pg", driver: "pg", dsnEnv: "XSQL_TEST_PG_DSN", dbType: "pg", + dsn: os.Getenv("XSQL_TEST_PG_DSN"), badDSN: badPgDSN, authHintSub: "Authentication", + }, + } + + 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) { + t.Helper() + + dsn := tc.dsnOverride + if dsn == "" { + 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) + } + + 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() + + _, 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 assertErrorGuidance(t *testing.T, xe *errors.XError, wantCode errors.Code, wantCategory, wantHintSub string) { + t.Helper() + if xe == nil { + t.Fatal("expected error") + } + if xe.Code != wantCode { + t.Fatalf("code = %s, want %s", xe.Code, wantCode) + } + 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") + } +} + +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:] + } + } + return good + "_invalid" +} + +func badPgDSN(good string) string { + if good == "" { + return "" + } + return strings.Replace(good, "postgres:postgres@", "postgres:xsql_wrong_password@", 1) +}