From 7a0e1dba4c64e68cb357760d6821ab49215b80b9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 17:08:16 +0000 Subject: [PATCH 1/2] fix(errors): stop masking query-level errors as 'database_not_connected' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _classify_error matched the substrings "operationalerror", "role", "does not exist", and a bare "timeout" when routing exceptions to the PostgreSQL setup guide. Those match ordinary query failures, not connection failures: - sqlite3.OperationalError's class name is literally "OperationalError", so type(exc).__name__.lower() == "operationalerror" — EVERY SQLite query error (FTS5 syntax error, "no such table", "database is locked") was classified as database_not_connected and handed a `brew install postgresql@17` guide. On the SQLite backend that guide is doubly wrong, and it buried the real error (a FTS5 syntax error in get_causal_chain surfaced exactly this way during benchmarking). - bare "role" matched "control", "payroll", "role-based"; bare "does not exist" matched `column "x" does not exist` (a query/schema bug); bare "timeout" matched statement/lock timeouts. Tightened the bucket to unambiguous connection/auth phrases only (connection refused, could not connect, could not translate host name, server closed the connection, the database system is starting up, password authentication failed, connection timed out, timeout expired). Genuine create-db/create-role setup conditions remain fully actionable from their own honest `OperationalError: ... FATAL: database "cortex" does not exist` text, which the fall-through returns verbatim. Regression tests pin that FTS5 syntax errors, no-such-table, column-does-not- exist, a "role" substring in unrelated text, and a locked-db error all fall through to their honest ': ', while the seven genuine connection failures still get the setup guide. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_017fnomaXS71hgxMw2zr7HGR --- mcp_server/tool_error_handler.py | 27 +++++++-- .../test_tool_error_handler_classification.py | 60 +++++++++++++++++++ 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/mcp_server/tool_error_handler.py b/mcp_server/tool_error_handler.py index e7206956..c28ce967 100644 --- a/mcp_server/tool_error_handler.py +++ b/mcp_server/tool_error_handler.py @@ -106,18 +106,35 @@ def _classify_error(exc: Exception) -> tuple[str, str]: ): return "missing_extension", _EXTENSION_GUIDE + # Connection/auth failures ONLY — each phrase below is unambiguous about a + # server that is unreachable, still starting, or refusing credentials. + # + # Deliberately NOT here (issue: SQLite query errors masked as + # "PostgreSQL not connected"): the exception CLASS name "operationalerror", + # a bare "does not exist", a bare "role", and a bare "timeout". Those match + # ordinary query-level failures — a FTS5 "syntax error", "no such table", + # a column that "does not exist", a statement/lock "timeout" — none of + # which are connection problems. On the SQLite backend they are never + # connection problems, yet the base class ``sqlite3.OperationalError`` set + # ``type(exc).__name__.lower() == "operationalerror"`` and every one of + # them got the PostgreSQL ``brew install`` guide, burying the real error + # (a FTS5 syntax error in get_causal_chain surfaced exactly this way). + # An error that is genuinely a create-db/create-role setup step is still + # fully actionable from its own honest ``OperationalError: ... FATAL: + # database "cortex" does not exist`` text, which the fall-through returns. if any( kw in exc_lower for kw in [ "connection refused", "could not connect", + "could not translate host name", "no such host", "connection reset", - "does not exist", - "operationalerror", - "role", - "password authentication", - "timeout", + "server closed the connection", + "the database system is starting up", + "password authentication failed", + "connection timed out", + "timeout expired", # psycopg connect-timeout wording ] ): return "database_not_connected", _DB_SETUP_GUIDE diff --git a/tests_py/server/test_tool_error_handler_classification.py b/tests_py/server/test_tool_error_handler_classification.py index 8c2a796c..da5a490e 100644 --- a/tests_py/server/test_tool_error_handler_classification.py +++ b/tests_py/server/test_tool_error_handler_classification.py @@ -69,6 +69,66 @@ def test_unrecognized_error_falls_through_unclassified(self): assert message == "some unrelated application error" +class TestQueryErrorsAreNotMaskedAsDbNotConnected: + """Regression: SQLite query-level OperationalErrors were classified as + 'database_not_connected' (PostgreSQL setup guide) because the exception + CLASS name is 'OperationalError' and the old keyword list matched + 'operationalerror'. A FTS5 syntax error in get_causal_chain surfaced this + way — the real error was buried under a 'brew install postgresql' guide, + doubly wrong on the SQLite backend. Query errors must fall through to + their honest ': '.""" + + def test_fts5_syntax_error_not_masked(self): + import sqlite3 + + exc = sqlite3.OperationalError('fts5: syntax error near ""__main__""') + error_type, message = _classify_error(exc) + assert error_type == "OperationalError" + assert "fts5: syntax error" in message + + def test_no_such_table_not_masked(self): + import sqlite3 + + exc = sqlite3.OperationalError("no such table: memories") + error_type, message = _classify_error(exc) + assert error_type == "OperationalError" + assert "no such table" in message + + def test_column_does_not_exist_not_masked(self): + # bare "does not exist" used to route a query bug to the DB guide. + exc = Exception('column "heat" does not exist') + error_type, _ = _classify_error(exc) + assert error_type == "Exception" + + def test_word_containing_role_not_masked(self): + # bare "role" matched substrings like "control"/"payroll" — gone. + exc = ValueError("invalid role assignment in access control policy") + error_type, _ = _classify_error(exc) + assert error_type == "ValueError" + + def test_statement_timeout_not_masked(self): + # a lock/statement timeout is not a connection failure. + import sqlite3 + + exc = sqlite3.OperationalError("database is locked") + error_type, _ = _classify_error(exc) + assert error_type == "OperationalError" + + def test_genuine_connection_failures_still_classified(self): + for msg in [ + "connection refused", + "could not connect to server", + "could not translate host name", + "server closed the connection unexpectedly", + "the database system is starting up", + "password authentication failed for user", + "connection timed out", + ]: + error_type, message = _classify_error(RuntimeError(msg)) + assert error_type == "database_not_connected", msg + assert "PostgreSQL" in message + + class TestSafeHandlerErrorPath: """safe_handler must raise ToolError, not return an error dict, and must log the underlying exception (with traceback) before From e74ee3a85b98031ae4ff1f295c239b3384fe5b01 Mon Sep 17 00:00:00 2001 From: cdeust Date: Mon, 24 Aug 2026 19:56:30 +0200 Subject: [PATCH 2/2] =?UTF-8?q?refactor(errors):=20extract=20classifier=20?= =?UTF-8?q?phrase=20tables=20to=20satisfy=20=C2=A74.2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _classify_error grew to a 58-line AST span (cap: 40 local / 50 hard, High stakes — tool_error_handler.py is imported by 7 production modules registering ~50 tools), flagged blocking in the PR #448 review. The violation was pre-baselined debt the diff-scoped craftsmanship gate could not see growing. Hoist the missing_extension and database_not_connected keyword lists into module-level named constants (_MISSING_EXTENSION_PHRASES, _CONNECTION_FAILURE_PHRASES), moving the connection-failure rationale comment onto its constant. Pure Extract Variable — classification logic and phrase lists are unchanged; span now 21 lines. Prune the now-stale baseline entry for _classify_error (safe_handler's separate, untouched entry stays). Also close the non-blocking truth-table gap: add the three untested database_not_connected phrases (no such host, connection reset, timeout expired) to the genuine-connection-failures test loop. Co-Authored-By: Claude --- .craftsmanship-baseline.json | 5 -- mcp_server/tool_error_handler.py | 76 +++++++++---------- .../test_tool_error_handler_classification.py | 3 + 3 files changed, 40 insertions(+), 44 deletions(-) diff --git a/.craftsmanship-baseline.json b/.craftsmanship-baseline.json index b3b63055..a0ebc17f 100644 --- a/.craftsmanship-baseline.json +++ b/.craftsmanship-baseline.json @@ -5517,11 +5517,6 @@ "kind": "method-size", "detail": "classify_write_class" }, - { - "file": "mcp_server/tool_error_handler.py", - "kind": "method-size", - "detail": "_classify_error" - }, { "file": "mcp_server/tool_error_handler.py", "kind": "method-size", diff --git a/mcp_server/tool_error_handler.py b/mcp_server/tool_error_handler.py index c28ce967..e33c344d 100644 --- a/mcp_server/tool_error_handler.py +++ b/mcp_server/tool_error_handler.py @@ -80,6 +80,41 @@ async def tool_remember(...) -> dict: "Then restart Claude Code." ) +_MISSING_EXTENSION_PHRASES = [ + 'type "vector" does not exist', + "extension", + "pg_trgm", +] + +# Connection/auth failures ONLY — each phrase below is unambiguous about a +# server that is unreachable, still starting, or refusing credentials. +# +# Deliberately NOT here (issue: SQLite query errors masked as +# "PostgreSQL not connected"): the exception CLASS name "operationalerror", +# a bare "does not exist", a bare "role", and a bare "timeout". Those match +# ordinary query-level failures — a FTS5 "syntax error", "no such table", +# a column that "does not exist", a statement/lock "timeout" — none of +# which are connection problems. On the SQLite backend they are never +# connection problems, yet the base class ``sqlite3.OperationalError`` set +# ``type(exc).__name__.lower() == "operationalerror"`` and every one of +# them got the PostgreSQL ``brew install`` guide, burying the real error +# (a FTS5 syntax error in get_causal_chain surfaced exactly this way). +# An error that is genuinely a create-db/create-role setup step is still +# fully actionable from its own honest ``OperationalError: ... FATAL: +# database "cortex" does not exist`` text, which the fall-through returns. +_CONNECTION_FAILURE_PHRASES = [ + "connection refused", + "could not connect", + "could not translate host name", + "no such host", + "connection reset", + "server closed the connection", + "the database system is starting up", + "password authentication failed", + "connection timed out", + "timeout expired", # psycopg connect-timeout wording +] + def _classify_error(exc: Exception) -> tuple[str, str]: """Classify an exception into a user-friendly category and message.""" @@ -96,47 +131,10 @@ def _classify_error(exc: Exception) -> tuple[str, str]: if "explicit database_url unreachable" in exc_lower: return "explicit_database_url_unreachable", str(exc) - if any( - kw in exc_lower - for kw in [ - 'type "vector" does not exist', - "extension", - "pg_trgm", - ] - ): + if any(kw in exc_lower for kw in _MISSING_EXTENSION_PHRASES): return "missing_extension", _EXTENSION_GUIDE - # Connection/auth failures ONLY — each phrase below is unambiguous about a - # server that is unreachable, still starting, or refusing credentials. - # - # Deliberately NOT here (issue: SQLite query errors masked as - # "PostgreSQL not connected"): the exception CLASS name "operationalerror", - # a bare "does not exist", a bare "role", and a bare "timeout". Those match - # ordinary query-level failures — a FTS5 "syntax error", "no such table", - # a column that "does not exist", a statement/lock "timeout" — none of - # which are connection problems. On the SQLite backend they are never - # connection problems, yet the base class ``sqlite3.OperationalError`` set - # ``type(exc).__name__.lower() == "operationalerror"`` and every one of - # them got the PostgreSQL ``brew install`` guide, burying the real error - # (a FTS5 syntax error in get_causal_chain surfaced exactly this way). - # An error that is genuinely a create-db/create-role setup step is still - # fully actionable from its own honest ``OperationalError: ... FATAL: - # database "cortex" does not exist`` text, which the fall-through returns. - if any( - kw in exc_lower - for kw in [ - "connection refused", - "could not connect", - "could not translate host name", - "no such host", - "connection reset", - "server closed the connection", - "the database system is starting up", - "password authentication failed", - "connection timed out", - "timeout expired", # psycopg connect-timeout wording - ] - ): + if any(kw in exc_lower for kw in _CONNECTION_FAILURE_PHRASES): return "database_not_connected", _DB_SETUP_GUIDE return type(exc).__name__, str(exc) diff --git a/tests_py/server/test_tool_error_handler_classification.py b/tests_py/server/test_tool_error_handler_classification.py index da5a490e..a0e5e3c6 100644 --- a/tests_py/server/test_tool_error_handler_classification.py +++ b/tests_py/server/test_tool_error_handler_classification.py @@ -119,10 +119,13 @@ def test_genuine_connection_failures_still_classified(self): "connection refused", "could not connect to server", "could not translate host name", + "no such host", + "connection reset", "server closed the connection unexpectedly", "the database system is starting up", "password authentication failed for user", "connection timed out", + "timeout expired", ]: error_type, message = _classify_error(RuntimeError(msg)) assert error_type == "database_not_connected", msg