diff --git a/README.md b/README.md index 5fd2a4a..f892ac4 100644 --- a/README.md +++ b/README.md @@ -220,8 +220,9 @@ Unsupported syntax. Most of these raise; the first is silent and therefore more Two different engines run in sequence, and neither is zoekt's RE2: -1. Postgres POSIX ARE (`~` / `~*`) selects which files match. Invalid patterns surface as - a query-time database error, not a parse error. +1. Postgres POSIX ARE (`~` / `~*`) selects which files match. An invalid pattern (any + polarity) surfaces as the recoverable `regex_invalid` payload field carrying the + Postgres message, not a parse error and not a failed tool call. 2. Python `re` rescans those files to produce the highlighted line matches. The practical consequences: `^` and `$` are line anchors and `.` never crosses lines; @@ -265,9 +266,12 @@ never the literal string `"HEAD"` unless that is genuinely the resolved branch ( repo with no `default_branch` recorded). Recoverable conditions come back as payload fields — -`query_parse_error`, `query_too_broad`, `truncated`, `regex_incompatible`, +`query_parse_error`, `query_too_broad`, `truncated`, `regex_incompatible`, `regex_invalid`, `no_content_atom`, `zero_width_only_atoms`, `commit_not_indexed` — rather than errors, so -an agent can react without a failed tool call. Pagination rides the same envelope as +an agent can react without a failed tool call. `regex_invalid` is distinct from +`regex_incompatible`: the latter means Python `re` (not Postgres) rejected an otherwise-valid +pattern and only degrades highlighting; `regex_invalid` means Postgres rejected the pattern +outright and the query did not run. Pagination rides the same envelope as `next_cursor`, and the semantic tool adds its own status fields (`semantic_enabled`, `semantic_schema_missing`). diff --git a/app/AGENTS.md b/app/AGENTS.md index 882aadb..ab5ca56 100644 --- a/app/AGENTS.md +++ b/app/AGENTS.md @@ -30,7 +30,7 @@ The MCP server Databricks App: a FastMCP streamable-HTTP service exposing the in ### Working In This Directory - **The engine is a process-scoped module singleton, not lifespan-owned.** A FastMCP `lifespan` re-enters once per MCP session; building the engine there would re-pay Lakebase cold start and open N×5 pools. `get_engine()` in `main.py` is the only builder; the lifespan only references it and never disposes (that is `atexit`'s job). - **Blocking work runs off the event loop.** Every tool body goes through `_dispatch` → `anyio.to_thread.run_sync` under `_DB_LIMITER` (sized to the 5-conn pool). Never run SQL or the Python `re` rescan inline in an async handler. -- **Recoverable conditions are payload fields, never exceptions**: `truncated`, `query_too_broad`, `query_parse_error`, `regex_incompatible`, `no_content_atom`, `zero_width_only_atoms`, `semantic_schema_missing`. Only genuinely unexpected faults reach `_dispatch`, which logs the traceback and re-raises. Envelope keys are additive and permanent — agents depend on them; never remove or reshape one. +- **Recoverable conditions are payload fields, never exceptions**: `truncated`, `query_too_broad`, `query_parse_error`, `regex_incompatible`, `regex_invalid`, `no_content_atom`, `zero_width_only_atoms`, `semantic_schema_missing`. Only genuinely unexpected faults reach `_dispatch`, which logs the traceback and re-raises. Envelope keys are additive and permanent — agents depend on them; never remove or reshape one. - **`clamp_limit` gates every caller-supplied limit** (`<=0` → `row_limit`, `> max` → `max_row_limit`) before it reaches a builder. - `main.py` aliases `service.*` builders (`_search_code_payload = service.search_code_payload`); tests monkeypatching collaborators must patch `service.*`, since function globals resolve in the defining module. - Tools/routes are registered on a fresh `FastMCP` inside `create_app()` (a `streamable_http_app`'s session manager is single-use); do not decorate onto a module-global instance. diff --git a/app/main.py b/app/main.py index 3396229..548d79d 100644 --- a/app/main.py +++ b/app/main.py @@ -23,10 +23,10 @@ shutdown via ``atexit``; the per-session lifespan only *references* it and never disposes. Recoverable conditions (``truncated``, ``query_too_broad``, ``query_parse_error``, -``regex_incompatible``, ``no_content_atom``, ``zero_width_only_atoms``) are structured payload -fields, never exceptions; only genuinely unexpected faults reach the ``_dispatch`` -choke-point, which logs a full traceback and re-raises (never swallows). Output shapes are -pinned to the zoekt parity assertions in ``tests/unit/test_main.py``. +``regex_incompatible``, ``regex_invalid``, ``no_content_atom``, ``zero_width_only_atoms``) are +structured payload fields, never exceptions; only genuinely unexpected faults reach the +``_dispatch`` choke-point, which logs a full traceback and re-raises (never swallows). Output +shapes are pinned to the zoekt parity assertions in ``tests/unit/test_main.py``. """ from __future__ import annotations @@ -105,6 +105,9 @@ def _signals(payload: dict[str, Any]) -> dict[str, Any]: "truncated": payload.get("truncated"), "query_too_broad": payload.get("query_too_broad"), "query_parse_error": payload.get("query_parse_error"), + # Without this, a Postgres-rejected regex is log-indistinguishable from a genuine + # zero-result query. + "regex_invalid": payload.get("regex_invalid"), # Query-shape signals: a filter-only or all-zero-width query returns zero files # legitimately, so without these a shape problem is indistinguishable in the logs # from a genuine no-match. @@ -243,15 +246,19 @@ async def search_code( ``branch``/``commit`` are convenience params equivalent to appending ``branch:""`` / ``commit:`` to ``query``. ``limit`` caps the number of files scanned (clamped to a server maximum). Recoverable conditions surface as fields (``query_parse_error``, - ``query_too_broad``, ``truncated``, ``regex_incompatible``, ``no_content_atom``, - ``zero_width_only_atoms``). The last two explain an empty result that is NOT a true negative: - ``no_content_atom`` means the query carried no affirmative content atom to highlight -- - either a filter-only query (e.g. ``lang:go`` alone) or one that is entirely negated (e.g. - ``-foo`` alone: an exclusion is never a highlight) -- and ``zero_width_only_atoms`` means - every atom it carried matches zero-width (e.g. ``/^/``). A query mixing content with an - exclusion (e.g. ``foo -bar``) highlights only the affirmative term; excluded terms never - appear as matches. ``no_content_atom`` does not distinguish "no atom at all" from "fully - negated" -- recover which one it was from the echoed ``query`` field, if it matters. + ``query_too_broad``, ``truncated``, ``regex_incompatible``, ``regex_invalid``, + ``no_content_atom``, ``zero_width_only_atoms``). The middle two explain an empty result + that is NOT a true negative: ``no_content_atom`` means the query carried no affirmative + content atom to highlight -- either a filter-only query (e.g. ``lang:go`` alone) or one + that is entirely negated (e.g. ``-foo`` alone: an exclusion is never a highlight) -- and + ``zero_width_only_atoms`` means every atom it carried matches zero-width (e.g. ``/^/``). A + query mixing content with an exclusion (e.g. ``foo -bar``) highlights only the affirmative + term; excluded terms never appear as matches. ``no_content_atom`` does not distinguish "no + atom at all" from "fully negated" -- recover which one it was from the echoed ``query`` + field, if it matters. ``regex_invalid`` carries the Postgres error message when a + ``/regex/``, ``repo:``, ``file:``, or ``sym:`` pattern is not a valid Postgres POSIX ARE + (e.g. ``/[/``) -- distinct from ``regex_incompatible``, which means Python ``re`` (not + Postgres) rejected an otherwise-valid pattern and only degrades highlighting. """ lc = ctx.request_context.lifespan_context engine, cfg = lc["engine"], lc["config"] @@ -290,6 +297,11 @@ async def semantic_search( stripped or reinterpreted). A query that is only filters, or empty/whitespace-only, leaves nothing to embed and returns ``nothing_to_embed: true`` with no embedding call made. + ``repo:``/``file:`` filter values ARE matched as Postgres regular expressions (unlike a + bare token, which is only rejected if written ``/like/this/``): a Postgres-invalid pattern + there (e.g. ``repo:[``) returns ``regex_invalid`` set to the Postgres error message, with a + remedy in ``reason`` -- fix the pattern rather than resubmitting the query verbatim. + ``branch`` is sugar for a ``branch:`` atom -- conjunctive with any ``branch:`` atom already in ``query`` (mirrors ``search_code``'s ``branch`` param) -- restricting to files whose indexed branches include the given name (exact match); with no branch given anywhere, diff --git a/app/query/AGENTS.md b/app/query/AGENTS.md index 2fad98d..8829c56 100644 --- a/app/query/AGENTS.md +++ b/app/query/AGENTS.md @@ -18,7 +18,7 @@ The pure halves of the query pipeline. `parser.py` turns a zoekt-style query str ### Working In This Directory - **Parser purity is a hard invariant.** `parser.py` (and this package `__init__`) must import nothing beyond stdlib; `test_parser_import_is_pure` runs `import app.query.parser` in a subprocess and fails on any sqlalchemy/SDK/tree-sitter leakage. -- **Regex bodies are stored RAW, never `re.compile`d or escaped.** Postgres POSIX ARE ≠ Python `re`; validity is the DB's problem at execution time (`/[/` parses fine). The compiler binds patterns as parameters — never interpolate. +- **Regex bodies are stored RAW, never `re.compile`d or escaped.** Postgres POSIX ARE ≠ Python `re`; validity is the DB's problem at execution time (`/[/` parses fine). The compiler binds patterns as parameters — never interpolate. `app/search/errors.py`'s `reraise_or_recoverable` maps a Postgres-invalid pattern to a typed `RegexInvalidError` -> the `regex_invalid` payload field (issue #75), never an uncaught fault. - **Case is query-global** (last `case:` wins) but stamped only on `Substring`/`Regex` leaves; the compiler derives it from any such leaf, and callers holding the raw query pass `resolve_case(query)` so a filter-only `case:yes file:x` still resolves exactly. `repo:` is ALWAYS case-insensitive (`~*`). - **The `coalesce(default_branch, 'HEAD')` expression must stay byte-identical** across its four sites: this compiler, the 0003 backfill, the semantic default leg, and `get_file_payload`. - `lang:` normalizes with `.strip().lower()` and unknown values match nothing (empty result, no error, no `indexer` import). Substring literals escape `\`, `%`, `_` (backslash first) with `escape="\\"`. diff --git a/app/query/compiler.py b/app/query/compiler.py index 9dec68a..414e136 100644 --- a/app/query/compiler.py +++ b/app/query/compiler.py @@ -23,7 +23,9 @@ case-flip set. * Regex is opaque. Regex/filter patterns bind RAW as parameters -- never escaped, never ``re.compile``-d. An invalid POSIX ARE surfaces as a DB execution error at query time, - not at compile time. + not at compile time -- which the search layer (:mod:`app.search.errors`) maps to a typed + ``RegexInvalidError`` -> the ``regex_invalid`` payload field, never an uncaught fault + (issue #75). * ``lang:`` normalization. ``File.lang == lang.strip().lower()``; unknown values match nothing (empty result) rather than raising. No ``indexer`` import. * Substring escaping. ``LIKE``/``ILIKE`` literals escape ``\\``, ``%``, ``_`` (backslash diff --git a/app/search/AGENTS.md b/app/search/AGENTS.md index 10d2427..d10effe 100644 --- a/app/search/AGENTS.md +++ b/app/search/AGENTS.md @@ -4,15 +4,15 @@ # app/search ## Purpose -The impure serve-side execution layer sitting between the pure `app/query/` seams and the payload builders in `app/service.py`. `grep.py` runs the compiler's candidate query, then streams content one row at a time and rescans it with Python `re` for per-line zoekt-shaped highlights; `symbols.py` answers `sym:` queries with actual definitions (name/kind/line) the highlight-driven grep path cannot return; `semantic.py` is the natural-language companion — vector-ANN + BM25 legs over `chunks` fused with reciprocal-rank fusion (RRF, k=60) via the Lakebase `lakebase_ann`/`lakebase_bm25` operators; `errors.py` is the shared timeout → `QueryTooBroadError` mapper. +The impure serve-side execution layer sitting between the pure `app/query/` seams and the payload builders in `app/service.py`. `grep.py` runs the compiler's candidate query, then streams content one row at a time and rescans it with Python `re` for per-line zoekt-shaped highlights; `symbols.py` answers `sym:` queries with actual definitions (name/kind/line) the highlight-driven grep path cannot return; `semantic.py` is the natural-language companion — vector-ANN + BM25 legs over `chunks` fused with reciprocal-rank fusion (RRF, k=60) via the Lakebase `lakebase_ann`/`lakebase_bm25` operators; `errors.py` is the shared mapper from a raw Postgres `DBAPIError` to a typed recoverable error — `QueryTooBroadError` (statement_timeout) or `RegexInvalidError` (Postgres-invalid regex, issue #75). ## Key Files | File | Description | |------|-------------| -| `grep.py` | `grep_search(conn, query, ...) -> GrepResult`: compile → candidate ids → content fetch with `yield_per=1` (one-row server-side cursor; bare `stream_results` does NOT bound memory) → `extract_line_matches` per line. Bounds: transaction-local `statement_timeout` (cancel → `QueryTooBroadError`), aggregate `max_content_bytes` cap (trip → `truncated`/`"byte_cap"`). Emits UTF-8 line-local half-open `byte_ranges`, `regex_incompatible` (NOT-RE2 degradation), the shape flags `no_content_atom` / `zero_width_only_atoms` (mutually exclusive by construction; zero-width proof via guarded `re._parser.getwidth()`), and `FileCursor`-based pagination (`_Unset` sentinel: cursor kwarg supplied at all = pagination mode) | +| `grep.py` | `grep_search(conn, query, ...) -> GrepResult`: compile → candidate ids → content fetch with `yield_per=1` (one-row server-side cursor; bare `stream_results` does NOT bound memory) → `extract_line_matches` per line. Bounds: transaction-local `statement_timeout` (cancel → `QueryTooBroadError`), aggregate `max_content_bytes` cap (trip → `truncated`/`"byte_cap"`); a Postgres-invalid regex (any polarity) raises `RegexInvalidError`. Emits UTF-8 line-local half-open `byte_ranges`, `regex_incompatible` (NOT-RE2 degradation), the shape flags `no_content_atom` / `zero_width_only_atoms` (mutually exclusive by construction; zero-width proof via guarded `re._parser.getwidth()`), and `FileCursor`-based pagination (`_Unset` sentinel: cursor kwarg supplied at all = pagination mode) | | `symbols.py` | `symbol_search(conn, query, ...) -> SymbolResult`: deliberately TWO queries — the full-AST `compile_query` picks eligible file ids, then a pure projection returns matching `symbols` rows. One joined query would let SQLAlchemy auto-correlate the compiler's `sym:`/`repo:` subqueries against outer rows (silent mis-scope). No `sym:` atom → short-circuit `no_symbol_atom=True` with zero DB hits. Pure SQL: none of grep's NOT-RE2/CPU caveats apply | | `semantic.py` | `_semantic_search_payload`: flag-off short-circuit FIRST (no engine/SDK), then `app.query.semantic_filters.split_semantic_query` (pure, no DB) splits `repo:`/`file:`/`lang:`/`branch:` filters from natural-language prose — `query_parse_error` / `unsupported_filter` (`sym:`/`case:`/`commit:`/regex, remedy in `reason`) / `nothing_to_embed` (branched wording) all return BEFORE the schema probe or the embedder — then a `to_regclass('chunks')` schema probe (→ `semantic_schema_missing` payload, not a 500), embed the RESIDUAL text OUTSIDE any transaction, then the RRF query. `build_hybrid_rrf_sql(filters, branch)` / `filter_params(filters, branch)`: both derive from one shared `_normalized_filter_state` so predicates and binds can never drift; filters compile to numbered `sem_*` binds inside BOTH leg CTEs' `extra_where`; `branch` is unified with `branch:` atoms into ONE normalized, sorted branch list (no separate `:branch` bind). Two rank CTEs whose inner `ORDER BY LIMIT :topk` must stay a SINGLE expression (a second sort key silently collapses the lakebase index path to a seq scan); the query vector is bound as `(:qvec)::vector` via `format_vector_literal` (repr-based), never interpolated. The outer SELECT recomputes `c.embedding <=> (:qvec)::vector AS cosine_distance` for every fused row (Decision B1) — surfaced as `similarity = 1 - cosine_distance`, `None` for NULL embeddings. Process-scoped lazy `get_embedder` singleton mirrors `app.main.get_engine` | -| `errors.py` | `QueryTooBroadError` + `reraise_or_query_too_broad`: maps `psycopg.errors.QueryCanceled` (statement_timeout) to the shared error so grep and symbols raise identically without importing each other; anything else re-raises unchanged | +| `errors.py` | `QueryTooBroadError` / `RegexInvalidError` + `reraise_or_recoverable(error: DBAPIError)`: maps `psycopg.errors.QueryCanceled` (statement_timeout) to the former and `psycopg.errors.InvalidRegularExpression` (Postgres-invalid regex, issue #75) to the latter, so grep/symbols/references/semantic raise identically without importing each other; anything else re-raises unchanged | | `__init__.py` | Empty package marker | ## For AI Agents @@ -21,7 +21,7 @@ The impure serve-side execution layer sitting between the pure `app/query/` seam - **The compiler is the single source of truth for which files match.** grep and symbols never re-derive predicate/case logic; they compose `parse`/`resolve_case`/`compile_query`. grep owns only *which lines* match; symbols only *which definitions*. - **`yield_per=1` on the content fetch is load-bearing** (bounds memory to ~one file), as is checking the byte cap BEFORE `.encode()` (char count is a valid lower bound on UTF-8 bytes). - **Total failure raises, partial success flags.** A statement_timeout on the candidate query raises `QueryTooBroadError` (an empty return would be a lie); a tripped byte/row cap returns `truncated=True` + `truncation_reason`. Grep's shape flags are per-leg facts — the envelope in `app/service.py` ANDs in "the symbol leg did not answer"; do not special-case `SymbolFilter` inside grep. -- **`Not` (lexical negation) contributes nothing to highlighting or projection.** `grep._collect_matchers` skips `Not` subtrees (a `-foo` exclusion is SQL-only, never a green highlight) and does NOT recurse — so a purely-negated broken regex (`-/[/`) builds no pattern and never flags `regex_incompatible` (nothing was meant to highlight). `symbols._collect_symbol_patterns` skips `Not` too, so a `-sym:foo`-only query is `no_symbol_atom=True`. The compiler still lowers the exclusion in SQL regardless; only Python-side highlight/definition collection ignores it. A Postgres-invalid regex still reaches the DB either way (negated or not) and raises `sqlalchemy.exc.DataError`, uncaught — see `errors.py`'s `reraise_or_query_too_broad` docstring. +- **`Not` (lexical negation) contributes nothing to highlighting or projection.** `grep._collect_matchers` skips `Not` subtrees (a `-foo` exclusion is SQL-only, never a green highlight) and does NOT recurse — so a purely-negated broken regex (`-/[/`) builds no pattern and never flags `regex_incompatible` (nothing was meant to highlight). `symbols._collect_symbol_patterns` skips `Not` too, so a `-sym:foo`-only query is `no_symbol_atom=True`. The compiler still lowers the exclusion in SQL regardless; only Python-side highlight/definition collection ignores it. A Postgres-invalid regex still reaches the DB either way (negated or not); it now raises a typed `RegexInvalidError` (issue #75 — `errors.py`'s `reraise_or_recoverable`), which `app/service.py` maps to the recoverable `regex_invalid` payload field rather than an uncaught fault. - **`no_content_atom`/`no_symbol_atom` mean "no affirmative atom", not "no atom".** A filter-only query (`lang:go`) and a fully-negated one (`-foo` alone) both collect zero patterns and are reported identically by these flags — deliberately, so one flag covers both shapes rather than adding a second envelope key. A caller recovers which shape it actually was from the echoed `query` field, not from a distinguishing signal. Same for `no_symbol_atom` and `-sym:foo` alone. - **Documented V1 caveats (do not "fix" silently):** Python `re` is not Postgres POSIX ARE (uncompilable atom → skipped + `regex_incompatible`); non-ASCII case-folding divergence is unsignalled; Python regex CPU is uncapped (catastrophic backtracking holds the GIL — real fix is an RE2 binding). - **Semantic determinism rules:** `row_number()` ties break on `id` and the fused `ORDER BY rrf DESC, id LIMIT` tiebreak is load-bearing, but the inner leg `ORDER BY` must NEVER gain an `, id` — see `_leg_cte`'s docstring. The ANN leg's `c.embedding IS NOT NULL` guard keeps NULL embeddings from earning RRF credit. Filter predicates (`repo:`/`file:`/`lang:`/`branch:`, from `app.query.semantic_filters`) live ONLY in each leg's inner `extra_where` — never the `ORDER BY` — so a filtered query keeps the same single-expression index path as an unfiltered one; `similarity` is recomputed in the OUTER select (never threaded through a leg CTE), so it reaches BM25-only rows the ANN leg's `:topk` window never touched. **Cross-module invariant:** `app.query.semantic_filters` derives field names by inverting the parser's `_FIELD_KINDS` and recomputes value spans with `_read_field_value` — any change to the parser's field scanning must run `tests/unit/test_semantic_filters.py` (see `app/query/AGENTS.md`). @@ -30,14 +30,14 @@ The impure serve-side execution layer sitting between the pure `app/query/` seam - Timeouts are set injection-safe and transaction-local: `set_config('statement_timeout', :ms, true)` (grep/symbols) or int-coerced `SET LOCAL` (semantic) — never a session-level `SET` that leaks onto the pooled connection. ### Testing Requirements -- `make test`: `tests/unit/test_grep.py` (pure helpers, span merge, zero-width, cursor), `test_symbols_search.py`, `test_semantic.py` (SQL shape incl. filters/similarity, vector literal, flag-off no-op, filter-grammar payload states). +- `make test`: `tests/unit/test_grep.py` (pure helpers, span merge, zero-width, cursor), `test_symbols_search.py`, `test_semantic.py` (SQL shape incl. filters/similarity, vector literal, flag-off no-op, filter-grammar payload states), `test_search_errors.py` (`reraise_or_recoverable`'s mapping + breadth-guard). - `make test-integration`: `tests/integration/test_grep.py`, `test_symbols_search.py`, `test_semantic_rrf.py` (real RRF over Postgres; lakebase operator plans are only observable on a real Lakebase branch), `test_webui_semantic.py`. ### Common Patterns - Frozen dataclass result contracts (`GrepResult`, `FileMatches`, `LineMatch`, `SymbolResult`, `SymbolMatch`) with invariants stated in docstrings; keyword-only construction where mis-binding is a risk. - `_Unset` sentinel for optional-kwarg mode switches (pagination), mirrored one level up in `app/service.py`. - AST tree walks with `match` + `assert_never` tails; pure helpers kept DB-import-free and unit-testable. -- `except OperationalError as error: reraise_or_query_too_broad(error)` at every raw execution site. +- `except DBAPIError as error: reraise_or_recoverable(error)` at every raw execution site (grep ×2, symbols ×2, references ×3, semantic's RRF execute). ## Dependencies @@ -45,6 +45,6 @@ The impure serve-side execution layer sitting between the pure `app/query/` seam - `app/query/parser.py` + `app/query/compiler.py` (the pure seams), `app/query/semantic_filters.py` (semantic's filter-split seam), `app/db/models.py` (`File`, `Symbol`), `app/config.py` (`Settings`), `app/embed.py` (lazy, semantic enabled-path only). ### External -`sqlalchemy` (Core, `text`, `tuple_`, `OperationalError`), `psycopg` (`errors.QueryCanceled`), stdlib `re`/`threading`; `databricks-sdk` transitively via `app.embed` (lazy). +`sqlalchemy` (Core, `text`, `tuple_`, `DBAPIError`), `psycopg` (`errors.QueryCanceled`, `errors.InvalidRegularExpression`), stdlib `re`/`threading`; `databricks-sdk` transitively via `app.embed` (lazy). diff --git a/app/search/errors.py b/app/search/errors.py index 4d201cd..8b82d4e 100644 --- a/app/search/errors.py +++ b/app/search/errors.py @@ -1,9 +1,11 @@ """Shared error types for the serve-side search layer. -Kept in its own module so :mod:`app.search.symbols` and :mod:`app.search.grep` both raise -the SAME :class:`QueryTooBroadError` without one importing the other (a timeout in either -serve path must be indistinguishable to the MCP layer). :func:`reraise_or_query_too_broad` -is the single mapper from a Postgres ``statement_timeout`` cancellation to that error. +Kept in its own module so :mod:`app.search.symbols`, :mod:`app.search.grep`, and +:mod:`app.search.references` all raise the SAME error types without one importing another +(a timeout or an invalid regex in any of them must be indistinguishable to the MCP layer). +:func:`reraise_or_recoverable` is the single mapper from a Postgres fault -- surfaced by +SQLAlchemy as a :class:`~sqlalchemy.exc.DBAPIError` -- to one of the typed errors below, or a +re-raise of the original error when it maps to neither. """ from __future__ import annotations @@ -11,26 +13,42 @@ from typing import NoReturn import psycopg -from sqlalchemy.exc import OperationalError +from sqlalchemy.exc import DBAPIError class QueryTooBroadError(Exception): """The per-request statement_timeout cancelled a query (candidate/content/symbol).""" -def reraise_or_query_too_broad(error: OperationalError) -> NoReturn: - """Map a Postgres statement_timeout cancellation to :class:`QueryTooBroadError`. +class RegexInvalidError(Exception): + """A user-supplied pattern is not a valid Postgres POSIX ARE (regardless of polarity). - Any other :class:`~sqlalchemy.exc.OperationalError` is re-raised unchanged. Note an - invalid POSIX regex (e.g. ``/[/``) does NOT reach here at all: Postgres raises - ``InvalidRegularExpression``, a Data Exception that SQLAlchemy surfaces as - ``sqlalchemy.exc.DataError`` -- a sibling class this function is never even called - for, since the ``except OperationalError`` at each call site does not match it (see - ``app/search/grep.py``'s NOT-RE2 caveat). + Raised for a ``Regex`` atom, a ``repo:``/``file:`` filter, or a ``sym:`` filter whose + pattern Postgres rejects with ``InvalidRegularExpression`` (SQLSTATE class 22, a Data + Exception) -- e.g. ``/[/``. The message is ``str(error.orig)``, which describes only the + caller's own pattern (no host/schema/relation leakage), so it is safe to surface verbatim + to the caller, mirroring how ``query_parse_error`` echoes a parser message. + """ + + +def reraise_or_recoverable(error: DBAPIError) -> NoReturn: + """Map a Postgres fault to a typed recoverable error, or re-raise it unchanged. + + ``psycopg.errors.QueryCanceled`` (a statement_timeout cancellation) maps to + :class:`QueryTooBroadError`; ``psycopg.errors.InvalidRegularExpression`` (a Postgres-invalid + POSIX ARE, e.g. ``/[/``) maps to :class:`RegexInvalidError`. Both classes are reachable + through :class:`~sqlalchemy.exc.DBAPIError`'s two sibling subclasses -- + :class:`~sqlalchemy.exc.OperationalError` for the cancellation, + :class:`~sqlalchemy.exc.DataError` for the invalid regex -- so every raw-execution call + site catches the common ``DBAPIError`` ancestor and routes through this single mapper. + Anything else (a NUL-byte ``DataError``, an ``IntegrityError``, ...) is re-raised + unchanged: this widening is behavior-neutral for every error class it does not name. """ if isinstance(error.orig, psycopg.errors.QueryCanceled): raise QueryTooBroadError( "the per-request statement_timeout cancelled a query (candidate, content, or " "symbol fetch) -- the query is too broad for the time budget" ) from error + if isinstance(error.orig, psycopg.errors.InvalidRegularExpression): + raise RegexInvalidError(str(error.orig)) from error raise error diff --git a/app/search/grep.py b/app/search/grep.py index dc3ac01..4ccfed4 100644 --- a/app/search/grep.py +++ b/app/search/grep.py @@ -69,13 +69,13 @@ wraps the predicate in ``not_(...)``, it does not validate the pattern), so a Postgres-invalid POSIX ARE such as ``/[/`` reaches the database whether the atom is written as ``/[/`` or ``-/[/``. Postgres raises ``InvalidRegularExpression`` (SQLSTATE class 22, a - Data Exception), which SQLAlchemy surfaces as ``sqlalchemy.exc.DataError`` -- a different - class from the ``OperationalError`` :func:`app.search.errors.reraise_or_query_too_broad` - catches, so it never reaches that mapper at all and propagates straight out of - ``grep_search`` uncaught, exactly like any other unexpected fault (``app/main.py``'s - ``_dispatch`` logs the traceback and re-raises). This is polarity-independent and - pre-existing (true for ``/[/`` before negation shipped, and unchanged by it): grep only - ever skips a negated broken regex's Python-side highlight compilation (see + Data Exception), which SQLAlchemy surfaces as ``sqlalchemy.exc.DataError``. This module's + raw-execution sites catch the common :class:`~sqlalchemy.exc.DBAPIError` ancestor and route + it through :func:`app.search.errors.reraise_or_recoverable`, which maps it to a typed + :class:`~app.search.errors.RegexInvalidError` -- the service layer (``app/service.py``) + turns that into the ``regex_invalid`` payload field, never an uncaught fault. This is + polarity-independent (true for ``/[/`` whether the atom is written as ``/[/`` or ``-/[/``): + grep only ever skips a negated broken regex's Python-side highlight compilation (see :func:`_collect_matchers`); the SQL predicate is compiled server-side regardless. Byte offsets are UTF-8, line-local, half-open ``[start, end)``: for a :class:`LineMatch`, @@ -91,7 +91,7 @@ class from the ``OperationalError`` :func:`app.search.errors.reraise_or_query_to from typing import NamedTuple, assert_never from sqlalchemy import Connection, select, text, tuple_ -from sqlalchemy.exc import OperationalError +from sqlalchemy.exc import DBAPIError from app.db.models import File from app.query.compiler import DEFAULT_ROW_LIMIT, compile_query @@ -111,7 +111,7 @@ class from the ``OperationalError`` :func:`app.search.errors.reraise_or_query_to parse, resolve_case, ) -from app.search.errors import reraise_or_query_too_broad +from app.search.errors import reraise_or_recoverable # 8 MiB of content pulled/scanned per request (aggregate across files). DEFAULT_MAX_CONTENT_BYTES = 8 * 1024 * 1024 @@ -178,7 +178,8 @@ class FileMatches: @dataclass(frozen=True) class GrepResult: """A grep result. ``truncated`` (with ``truncation_reason``) flags a partial result; - a total failure raises :class:`QueryTooBroadError` instead of returning. + a total failure raises :class:`QueryTooBroadError` or + :class:`~app.search.errors.RegexInvalidError` instead of returning. ``no_content_atom`` and ``zero_width_only_atoms`` are raw structural facts about this leg only. grep reports; it does not know whether a second leg answered the query -- a @@ -427,9 +428,10 @@ def grep_search( A per-request ``statement_timeout`` bounds DB time (a cancellation raises :class:`QueryTooBroadError`); ``max_content_bytes`` bounds app memory / aggregate bytes - scanned (a cap sets ``truncated`` + ``truncation_reason``). See the module docstring for - the NOT-RE2 and uncapped-Python-CPU caveats. Raises ``QueryParseError`` on a malformed - query (propagated from :func:`parse`). + scanned (a cap sets ``truncated`` + ``truncation_reason``). A Postgres-invalid regex + pattern (any polarity) raises :class:`~app.search.errors.RegexInvalidError`. See the + module docstring for the NOT-RE2 and uncapped-Python-CPU caveats. Raises + ``QueryParseError`` on a malformed query (propagated from :func:`parse`). ``cursor`` activates pagination mode when supplied at all, including ``None`` (page 1): a plain row-cap fill then reports ``truncated=False`` plus a non-null @@ -469,8 +471,8 @@ def grep_search( try: rows = conn.execute(stmt).all() - except OperationalError as error: - reraise_or_query_too_broad(error) + except DBAPIError as error: + reraise_or_recoverable(error) # `>=` deliberately over-warns on an exact fit (row_limit files that are exactly all # of them still report truncated -- an accepted, conservative false-positive). @@ -542,8 +544,8 @@ def grep_search( tuple(line_matches), ) ) - except OperationalError as error: - reraise_or_query_too_broad(error) + except DBAPIError as error: + reraise_or_recoverable(error) finally: result.close() diff --git a/app/search/references.py b/app/search/references.py index e1cab6d..6c3c3b5 100644 --- a/app/search/references.py +++ b/app/search/references.py @@ -33,13 +33,13 @@ from sqlalchemy import Connection, Row, Select, Text, any_, func, literal, select, text from sqlalchemy.dialects.postgresql import ARRAY -from sqlalchemy.exc import OperationalError +from sqlalchemy.exc import DBAPIError from sqlalchemy.orm import aliased from sqlalchemy.sql.elements import ColumnElement from app.db.models import File, ReferenceEdge, Repo, Symbol from app.query.compiler import DEFAULT_ROW_LIMIT -from app.search.errors import reraise_or_query_too_broad +from app.search.errors import reraise_or_recoverable # Per-request DB-time bound; a cancellation surfaces as QueryTooBroadError (mirrors symbols.py). DEFAULT_STATEMENT_TIMEOUT_MS = 5000 @@ -382,8 +382,8 @@ def resolve_references( repo_id = conn.execute( select(Repo.id).where(Repo.name == repo) ).scalar_one_or_none() - except OperationalError as error: - reraise_or_query_too_broad(error) + except DBAPIError as error: + reraise_or_recoverable(error) if repo_id is None: return ReferenceResult( (), truncated=False, truncation_reason=None, repo_known=False @@ -398,8 +398,8 @@ def resolve_references( ) try: site_rows = conn.execute(sites_stmt).all() - except OperationalError as error: - reraise_or_query_too_broad(error) + except DBAPIError as error: + reraise_or_recoverable(error) truncated = len(site_rows) >= row_limit @@ -411,8 +411,8 @@ def resolve_references( ) try: candidate_rows = conn.execute(candidates_stmt).all() - except OperationalError as error: - reraise_or_query_too_broad(error) + except DBAPIError as error: + reraise_or_recoverable(error) for row in candidate_rows: candidates_by_name.setdefault(row.name, []).append(row) diff --git a/app/search/semantic.py b/app/search/semantic.py index ea73276..ff94106 100644 --- a/app/search/semantic.py +++ b/app/search/semantic.py @@ -42,6 +42,7 @@ from sqlalchemy import text from sqlalchemy.engine import Engine +from sqlalchemy.exc import DBAPIError from sqlalchemy.sql.elements import TextClause from app.config import Settings @@ -51,6 +52,7 @@ UnsupportedSemanticAtomError, split_semantic_query, ) +from app.search.errors import RegexInvalidError, reraise_or_recoverable if TYPE_CHECKING: from app.embed import EmbedFn @@ -379,6 +381,23 @@ def _semantic_unsupported_filter_payload( } +def _semantic_regex_invalid_payload(query: str, error: RegexInvalidError) -> dict[str, Any]: + """A ``repo:``/``file:`` filter value Postgres rejects as an invalid POSIX ARE (e.g. + ``repo:[``). Filter atoms are matched as regexes even though the surrounding query is + natural-language prose, so this is malformed input, mirroring + :func:`_semantic_unsupported_filter_payload`'s conditional-key + remedy-bearing-``reason`` + shape.""" + return { + "query": query, + "semantic_enabled": True, + "results": [], + "count": 0, + "regex_invalid": str(error), + "reason": "repo:/file: filter values are matched as POSIX regexes by Postgres; fix the " + "pattern", + } + + def _semantic_nothing_to_embed_payload(query: str, *, has_filters: bool) -> dict[str, Any]: """Empty residual: no embedding call is made. Wording branches on WHY it is empty -- filters consumed everything vs. there was never any text -- so the caller sees why.""" @@ -417,7 +436,15 @@ def _semantic_search_payload( query). Only past all three does it probe the schema, lazily build the embedder, embed the RESIDUAL text (not the raw query) OUTSIDE the DB transaction (no network call inside ``conn.begin()``), then run the RRF query under a transaction-local ``statement_timeout`` - and join the fused ids back to ``chunks -> files -> repos``. + and join the fused ids back to ``chunks -> files -> repos``. The RRF execute is wrapped in + ``except DBAPIError: reraise_or_recoverable(error)`` (issue #75): a Postgres-invalid + ``repo:``/``file:`` filter pattern (e.g. ``repo:[``) maps to :class:`RegexInvalidError` -> + the ``regex_invalid`` payload field (see :func:`_semantic_regex_invalid_payload`). + Acknowledged side effect: this site previously had no ``except`` clause at all, so a + ``statement_timeout`` cancellation here now surfaces as :class:`QueryTooBroadError` instead + of a raw ``OperationalError`` -- still uncaught by this function, so the outward behavior + (an unhandled MCP fault / webui 502) is unchanged; mapping semantic timeouts to a + recoverable field is out of #75's scope. ``branch`` (unified with in-query ``branch:`` atoms): sugar for a ``branch:`` atom, conjunctive with any already in ``query``. No value anywhere scopes each leg to its chunk's @@ -472,10 +499,18 @@ def _semantic_search_payload( } params.update(filter_params(filters, branch)) - with engine.connect() as conn: - with conn.begin(): - conn.exec_driver_sql(f"SET LOCAL statement_timeout = {int(cfg.statement_timeout_ms)}") - rows = conn.execute(build_hybrid_rrf_sql(filters, branch), params).all() + try: + with engine.connect() as conn: + with conn.begin(): + conn.exec_driver_sql( + f"SET LOCAL statement_timeout = {int(cfg.statement_timeout_ms)}" + ) + try: + rows = conn.execute(build_hybrid_rrf_sql(filters, branch), params).all() + except DBAPIError as error: + reraise_or_recoverable(error) + except RegexInvalidError as error: + return _semantic_regex_invalid_payload(query, error) results = [ { diff --git a/app/search/symbols.py b/app/search/symbols.py index b701306..c97836b 100644 --- a/app/search/symbols.py +++ b/app/search/symbols.py @@ -52,7 +52,7 @@ from typing import assert_never from sqlalchemy import Connection, Select, or_, select, text -from sqlalchemy.exc import OperationalError +from sqlalchemy.exc import DBAPIError from app.db.models import File, Symbol from app.query.compiler import DEFAULT_ROW_LIMIT, compile_query @@ -72,7 +72,7 @@ parse, resolve_case, ) -from app.search.errors import reraise_or_query_too_broad +from app.search.errors import reraise_or_recoverable # Per-request DB-time bound; a cancellation surfaces as QueryTooBroadError. DEFAULT_STATEMENT_TIMEOUT_MS = 5000 @@ -195,6 +195,7 @@ def symbol_search( Two-step: ``compile_query`` selects the eligible file ids, then a pure projection returns the symbols in those files whose name matches the query's ``sym:`` atoms. Runs in one transaction with a per-request ``statement_timeout`` (a cancellation raises :class:`QueryTooBroadError`). + A Postgres-invalid ``sym:`` pattern raises :class:`~app.search.errors.RegexInvalidError`. Raises ``QueryParseError`` on a malformed query (propagated from :func:`parse`). A query with no ``sym:`` atom short-circuits to an empty result (``no_symbol_atom=True``) without a DB hit. """ @@ -218,8 +219,8 @@ def symbol_search( candidate = compile_query(node, limit=row_limit, case_sensitive=case_sensitive) try: file_ids = [row.id for row in conn.execute(candidate).all()] - except OperationalError as error: - reraise_or_query_too_broad(error) + except DBAPIError as error: + reraise_or_recoverable(error) # `>=` conservatively over-warns on an exact fit, matching grep's row-cap semantics. file_capped = len(file_ids) >= row_limit @@ -232,8 +233,8 @@ def symbol_search( ) try: rows = conn.execute(stmt).all() - except OperationalError as error: - reraise_or_query_too_broad(error) + except DBAPIError as error: + reraise_or_recoverable(error) symbol_capped = len(rows) >= row_limit truncated = file_capped or symbol_capped diff --git a/app/service.py b/app/service.py index f7e2389..3c1bbed 100644 --- a/app/service.py +++ b/app/service.py @@ -43,7 +43,7 @@ SymbolFilter, parse, ) -from app.search.errors import QueryTooBroadError +from app.search.errors import QueryTooBroadError, RegexInvalidError from app.search.grep import FileCursor, grep_search from app.search.references import EdgeSite, ReferenceResult, resolve_references from app.search.semantic import _semantic_search_payload as semantic_search_payload # noqa: F401 @@ -369,6 +369,7 @@ def _search_envelope( truncated: bool, truncation_reason: str | None, regex_incompatible: bool, + regex_invalid: str | None, query_too_broad: bool, query_parse_error: str | None, no_content_atom: bool, @@ -389,6 +390,14 @@ def _search_envelope( ``zero_width_only_atoms`` -- content atoms were present but every one provably matches zero-width (e.g. ``/^/``), so every span was dropped. Mutually exclusive by construction. + ``regex_invalid`` -- ``None`` normally; the Postgres-rejected pattern's message (e.g. + ``"invalid regular expression: brackets [] not balanced"``) when a ``Regex``/``repo:``/ + ``file:``/``sym:`` pattern is not a valid Postgres POSIX ARE (see + :class:`app.search.errors.RegexInvalidError`). Distinct from ``regex_incompatible``: + that flag means Python ``re`` rejected a Postgres-valid pattern (results still returned, + highlighting degraded); ``regex_invalid`` means Postgres rejected the pattern outright + (the query could not run at all). + Both are grep's per-leg fact AND-ed with "the symbol leg did not answer this query", so neither ever fires beside results the caller can see. That suppression is what carries grep's own invariants (see :class:`app.search.grep.GrepResult`) up to this layer: @@ -421,6 +430,7 @@ def _search_envelope( "truncated": truncated, "truncation_reason": truncation_reason, "regex_incompatible": regex_incompatible, + "regex_invalid": regex_invalid, "query_too_broad": query_too_broad, "query_parse_error": query_parse_error, "no_content_atom": no_content_atom, @@ -453,7 +463,11 @@ def search_code_payload( kind}]`` and ``line`` = the definition's first line (no highlight ``text``, so ``grep_search("sym:X")`` -- which returns nothing highlight-driven -- is answered here). ``QueryParseError`` -> ``query_parse_error`` + empty files; ``QueryTooBroadError`` (either - leg) -> ``query_too_broad`` + ``truncated``. ``repo_id`` is resolved to ``Repo.name``. + leg) -> ``query_too_broad`` + ``truncated``; ``RegexInvalidError`` (either leg) -> + ``regex_invalid`` + the Postgres message, with ``truncated`` staying False (nothing was + attempted-and-cut, the query never ran) -- a grep-leg trip returns empty ``files``, a + symbol-leg-only trip keeps grep's already-fetched ``files`` (D4 partial-result contract, + mirroring ``QueryTooBroadError``). ``repo_id`` is resolved to ``Repo.name``. ``byte_ranges`` are UTF-8 line-local half-open offsets (a documented divergence from zoekt's char ``start_col``/``end_col``). @@ -525,6 +539,7 @@ def search_code_payload( truncated=False, truncation_reason=None, regex_incompatible=False, + regex_invalid=None, query_too_broad=False, query_parse_error=str(error), no_content_atom=False, @@ -558,6 +573,7 @@ def search_code_payload( truncated=False, truncation_reason=None, regex_incompatible=False, + regex_invalid=None, query_too_broad=False, query_parse_error=None, no_content_atom=not has_content, @@ -587,6 +603,7 @@ def search_code_payload( truncated=True, truncation_reason=None, regex_incompatible=False, + regex_invalid=None, query_too_broad=True, query_parse_error=None, no_content_atom=False, @@ -594,6 +611,27 @@ def search_code_payload( next_cursor=(None if pagination_mode else _UNSET), **commit_kwargs, ) + except RegexInvalidError as error: + # Postgres rejected the pattern outright -- unlike query_too_broad, nothing was + # attempted-and-cut, so `truncated` stays False (D4). The symbol leg would hit the + # same compiled predicates and fail identically, so it does not run either. + return _search_envelope( + query, + files=[], + file_count=0, + match_count=0, + duration_ns=int((time.monotonic() - t0) * 1e9), + truncated=False, + truncation_reason=None, + regex_incompatible=False, + regex_invalid=str(error), + query_too_broad=False, + query_parse_error=None, + no_content_atom=False, + zero_width_only_atoms=False, + next_cursor=(None if pagination_mode else _UNSET), + **commit_kwargs, + ) # Symbol leg: sym: definitions the highlight-driven grep path cannot return. A timeout # here flags query_too_broad but still returns whatever grep found (partial, not a lie). @@ -604,6 +642,7 @@ def search_code_payload( # the symbol leg so folded symbols never repeat across pages. run_symbol_leg = not (pagination_mode and decoded_cursor is not None) query_too_broad = False + regex_invalid: str | None = None sym_result: SymbolResult | None if run_symbol_leg: try: @@ -616,6 +655,13 @@ def search_code_payload( except QueryTooBroadError: sym_result = None query_too_broad = True + except RegexInvalidError as error: + # Symbol-leg-only trip (theoretically possible, practically shadowed by the + # grep leg compiling the same sym: predicate first): keep grep's files rather + # than discarding them, mirroring the query_too_broad partial-result contract + # (D4). + sym_result = None + regex_invalid = str(error) else: sym_result = None @@ -782,6 +828,7 @@ def _entry( truncated=truncated, truncation_reason=truncation_reason, regex_incompatible=result.regex_incompatible, + regex_invalid=regex_invalid, query_too_broad=query_too_broad, query_parse_error=None, no_content_atom=no_content_atom, diff --git a/tests/integration/test_grep.py b/tests/integration/test_grep.py index 89461b4..038bce5 100644 --- a/tests/integration/test_grep.py +++ b/tests/integration/test_grep.py @@ -20,13 +20,12 @@ import pytest from sqlalchemy import Connection, insert, text -from sqlalchemy.exc import DataError from app.db.client import create_db_engine from app.db.models import Base, File, Repo from app.query.compiler import compile_query from app.query.parser import parse, resolve_case -from app.search.errors import QueryTooBroadError +from app.search.errors import QueryTooBroadError, RegexInvalidError from app.search.grep import FileCursor, GrepResult, grep_search from indexer.hashing import content_sha @@ -257,21 +256,32 @@ def test_healthy_query_does_not_raise_query_too_broad(seeded: Seeded) -> None: @pytest.mark.integration -@pytest.mark.xfail( - raises=DataError, - strict=False, - reason="Follow-up issue #75 (filed while scoping #70's polarity-awareness work). A " - "Postgres-invalid POSIX regex (e.g. `[`) reaches the DB raw regardless of polarity -- " - "the compiler never validates a Regex atom's pattern, negated or not -- and Postgres's " - "InvalidRegularExpression is a Data Exception, which SQLAlchemy surfaces as " - "sqlalchemy.exc.DataError: a sibling class reraise_or_query_too_broad's " - "`except OperationalError` does not catch, so it propagates uncaught as an unhandled " - "fault instead of a recoverable payload field (see app/search/grep.py's NOT-RE2 " - "caveat). Asserts the DESIRED future behavior (no uncaught exception) so a fix makes " - "this XPASS (non-gating here) rather than silently rotting as a stale pin.", -) -def test_negated_broken_regex_reaching_postgres_is_an_unhandled_fault(seeded: Seeded) -> None: - grep_search(seeded.conn, "-/[/ foo") +def test_negated_broken_regex_reaching_postgres_raises_regex_invalid_error( + seeded: Seeded, +) -> None: + # Polarity-independent (issue #75): a Postgres-invalid POSIX regex reaches the DB raw + # regardless of polarity -- the compiler never validates a Regex atom's pattern, negated + # or not -- and Postgres's InvalidRegularExpression is mapped by + # app.search.errors.reraise_or_recoverable to a typed RegexInvalidError, never an + # uncaught DataError (see app/search/grep.py's caveat bullet). + with pytest.raises(RegexInvalidError, match="invalid regular expression"): + grep_search(seeded.conn, "-/[/ foo") + + +@pytest.mark.integration +def test_broken_regex_reaching_postgres_raises_regex_invalid_error(seeded: Seeded) -> None: + with pytest.raises(RegexInvalidError, match="invalid regular expression"): + grep_search(seeded.conn, "/[/ foo") + + +@pytest.mark.integration +def test_broken_repo_filter_regex_reaching_postgres_raises_regex_invalid_error( + seeded: Seeded, +) -> None: + # The repo: filter site (compiler.py's `repo:` lowering) compiles to a `~*` predicate too, + # so an invalid pattern there is the same fault class as a bare Regex atom. + with pytest.raises(RegexInvalidError, match="invalid regular expression"): + grep_search(seeded.conn, "repo:[ foo") # ----------------------------------------------------------------- 7. byte cap -> truncated diff --git a/tests/integration/test_service.py b/tests/integration/test_service.py index ba3a20e..5bff74e 100644 --- a/tests/integration/test_service.py +++ b/tests/integration/test_service.py @@ -163,6 +163,21 @@ def test_bare_call_has_no_next_cursor_key(seeded: Seeded) -> None: assert payload["file_count"] == 5 # handler.go, a.go, b.go, c.go, note.py +@pytest.mark.integration +def test_invalid_regex_returns_recoverable_regex_invalid_envelope(seeded: Seeded) -> None: + # The exact scenario issue #75 reports as a fault: a Postgres-invalid regex through the + # full search_code_payload stack, end-to-end against real Postgres, must come back as a + # recoverable envelope field -- never an uncaught DataError. + payload = service.search_code_payload(seeded.engine, seeded.cfg, "/[/", 200) + + assert payload["regex_invalid"] is not None + assert "invalid regular expression" in payload["regex_invalid"] + assert payload["files"] == [] + assert payload["file_count"] == 0 + assert payload["truncated"] is False + assert payload["query_too_broad"] is False + + @pytest.mark.integration def test_page_one_folds_symbol_and_content_matches(seeded: Seeded) -> None: payload = service.search_code_payload( diff --git a/tests/integration/test_symbols_search.py b/tests/integration/test_symbols_search.py index 95e78ab..895ecb3 100644 --- a/tests/integration/test_symbols_search.py +++ b/tests/integration/test_symbols_search.py @@ -22,7 +22,7 @@ from app.db.client import create_db_engine from app.db.models import Base, File, Repo, Symbol -from app.search.errors import QueryTooBroadError +from app.search.errors import QueryTooBroadError, RegexInvalidError from app.search.symbols import SymbolResult, symbol_search from indexer.hashing import content_sha @@ -296,6 +296,15 @@ def test_tiny_statement_timeout_raises_query_too_broad(seeded: Seeded) -> None: _search(seeded.conn, "sym:blobsym /zq/", statement_timeout_ms=1) +@pytest.mark.integration +def test_invalid_sym_pattern_raises_regex_invalid_error(seeded: Seeded) -> None: + # The compiler's sym: lowering compiles to the SAME `~`/`~*` predicate as a bare Regex + # atom (issue #75), so a Postgres-invalid pattern raises RegexInvalidError from the + # candidate-selection step, never an uncaught DataError. + with pytest.raises(RegexInvalidError, match="invalid regular expression"): + _search(seeded.conn, "sym:[") + + # ------------------------------------------------------------------- branch scoping (0003) diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index fa0c1c9..dbe3b45 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -18,7 +18,7 @@ from app import main, service from app.config import Settings -from app.search.errors import QueryTooBroadError +from app.search.errors import QueryTooBroadError, RegexInvalidError from app.search.grep import FileCursor, FileMatches, GrepResult, LineMatch from app.search.symbols import SymbolMatch, SymbolResult @@ -154,6 +154,7 @@ def test_search_code_payload_matches_golden_shape(monkeypatch: pytest.MonkeyPatc assert payload["truncated"] is False assert payload["truncation_reason"] is None assert payload["regex_incompatible"] is False + assert payload["regex_invalid"] is None assert payload["query_too_broad"] is False assert payload["query_parse_error"] is None # An ordinary content query proves nothing about the query shape: both flags stay False. @@ -213,6 +214,24 @@ def _raise(*_a: object, **_k: object) -> GrepResult: assert payload["zero_width_only_atoms"] is False +@pytest.mark.unit +def test_search_code_regex_invalid_maps_to_signal(monkeypatch: pytest.MonkeyPatch) -> None: + def _raise(*_a: object, **_k: object) -> GrepResult: + raise RegexInvalidError("invalid regular expression: brackets [] not balanced") + + monkeypatch.setattr(service, "grep_search", _raise) + payload = main._search_code_payload(_FakeEngine([]), _cfg(), "/[/", 50) + + assert payload["regex_invalid"] == "invalid regular expression: brackets [] not balanced" + # Unlike query_too_broad, nothing was attempted-and-cut -- the query never ran (D4). + assert payload["truncated"] is False + assert payload["query_too_broad"] is False + assert payload["files"] == [] + assert payload["query_parse_error"] is None + assert payload["no_content_atom"] is False + assert payload["zero_width_only_atoms"] is False + + @pytest.mark.unit def test_search_code_truncation_and_regex_incompatible_passthrough( monkeypatch: pytest.MonkeyPatch, @@ -345,6 +364,26 @@ def _raise(*_a: object, **_k: object) -> SymbolResult: assert payload["match_count"] == 2 +@pytest.mark.unit +def test_sym_leg_regex_invalid_keeps_grep_files(monkeypatch: pytest.MonkeyPatch) -> None: + # Symbol-leg-only trip (D4): theoretically possible, practically shadowed by the grep leg + # compiling the same sym: predicate first. Keep grep's files, set regex_invalid. + monkeypatch.setattr(service, "grep_search", lambda *a, **k: _grep_result()) + + def _raise(*_a: object, **_k: object) -> SymbolResult: + raise RegexInvalidError("invalid regular expression: brackets [] not balanced") + + monkeypatch.setattr(service, "symbol_search", _raise) + engine = _FakeEngine([_FakeResult([_Row(id=7, name="acme/widgets")])]) + + payload = main._search_code_payload(engine, _cfg(), "sym:[ foo", 50) + + assert payload["regex_invalid"] == "invalid regular expression: brackets [] not balanced" + assert payload["query_too_broad"] is False + assert payload["file_count"] == 1 # grep's content match is still returned + assert payload["match_count"] == 2 + + @pytest.mark.unit def test_sym_truncation_sets_row_cap(monkeypatch: pytest.MonkeyPatch) -> None: # A sym:-only query IS filter-only at the grep layer, so grep reports no_content_atom=True; @@ -595,6 +634,7 @@ def test_envelope_keys_are_pinned_shape_plus_exactly_two(monkeypatch: pytest.Mon "truncated", "truncation_reason", "regex_incompatible", + "regex_invalid", "query_too_broad", "query_parse_error", } @@ -627,6 +667,7 @@ def test_envelope_carries_commit_keys_only_for_commit_query( "truncated", "truncation_reason", "regex_incompatible", + "regex_invalid", "query_too_broad", "query_parse_error", } @@ -1237,6 +1278,16 @@ def test_signals_log_includes_both_flags() -> None: assert signals["zero_width_only_atoms"] is False +@pytest.mark.observability +def test_signals_log_includes_regex_invalid() -> None: + # Without this, a Postgres-rejected regex is log-indistinguishable from a genuine + # zero-result query. + signals = main._signals( + {"regex_invalid": "invalid regular expression: brackets [] not balanced"} + ) + assert signals["regex_invalid"] == "invalid regular expression: brackets [] not balanced" + + @pytest.mark.observability def test_signals_log_includes_reference_tool_keys() -> None: # A list_imports validation miss / repo typo must be diagnosable from the log line alone -- diff --git a/tests/unit/test_search_errors.py b/tests/unit/test_search_errors.py new file mode 100644 index 0000000..0c42fdd --- /dev/null +++ b/tests/unit/test_search_errors.py @@ -0,0 +1,73 @@ +"""Unit tests for ``app.search.errors``: the shared Postgres-fault -> typed-error mapper. + +No DB: ``reraise_or_recoverable`` is exercised with real ``psycopg.errors`` instances wrapped +in the SQLAlchemy ``DBAPIError`` subclass a raw execution site would actually catch them as +(``OperationalError`` for a cancellation, ``DataError`` for an invalid regex or a NUL byte). +""" + +from __future__ import annotations + +import psycopg +import pytest +from sqlalchemy.exc import DataError, IntegrityError, OperationalError + +from app.search.errors import QueryTooBroadError, RegexInvalidError, reraise_or_recoverable + + +def _wrap(cls: type[Exception], orig: Exception) -> Exception: + return cls("SELECT 1", {}, orig) + + +@pytest.mark.unit +def test_query_canceled_maps_to_query_too_broad_error() -> None: + orig = psycopg.errors.QueryCanceled("canceling statement due to statement timeout") + error = _wrap(OperationalError, orig) + + with pytest.raises(QueryTooBroadError): + reraise_or_recoverable(error) # type: ignore[arg-type] + + +@pytest.mark.unit +def test_invalid_regular_expression_maps_to_regex_invalid_error_carrying_message() -> None: + orig = psycopg.errors.InvalidRegularExpression( + "invalid regular expression: brackets [] not balanced" + ) + error = _wrap(DataError, orig) + + with pytest.raises(RegexInvalidError, match=r"brackets \[\] not balanced") as excinfo: + reraise_or_recoverable(error) # type: ignore[arg-type] + assert str(excinfo.value) == "invalid regular expression: brackets [] not balanced" + + +@pytest.mark.unit +def test_generic_data_error_is_reraised_unchanged() -> None: + # NUL-byte shape (see tests/unit/test_webui_main.py): a DataError this mapper does not + # name must propagate unchanged, never silently swallowed or reclassified. + orig = ValueError("PostgreSQL text fields cannot contain NUL (0x00) bytes") + error = _wrap(DataError, orig) + + with pytest.raises(DataError) as excinfo: + reraise_or_recoverable(error) # type: ignore[arg-type] + assert excinfo.value is error + + +@pytest.mark.unit +def test_generic_operational_error_is_reraised_unchanged() -> None: + orig = psycopg.errors.OperationalError("connection reset") + error = _wrap(OperationalError, orig) + + with pytest.raises(OperationalError) as excinfo: + reraise_or_recoverable(error) # type: ignore[arg-type] + assert excinfo.value is error + + +@pytest.mark.unit +def test_integrity_error_is_reraised_unchanged() -> None: + # Breadth guard on the DBAPIError widening (OperationalError + DataError -> DBAPIError): + # a sibling DBAPIError subclass this mapper does not name must also pass through untouched. + orig = psycopg.errors.UniqueViolation("duplicate key value violates unique constraint") + error = _wrap(IntegrityError, orig) + + with pytest.raises(IntegrityError) as excinfo: + reraise_or_recoverable(error) # type: ignore[arg-type] + assert excinfo.value is error diff --git a/tests/unit/test_semantic.py b/tests/unit/test_semantic.py index e69264b..2bd00e2 100644 --- a/tests/unit/test_semantic.py +++ b/tests/unit/test_semantic.py @@ -14,8 +14,10 @@ import sys from typing import Any +import psycopg import pytest from sqlalchemy.dialects import postgresql +from sqlalchemy.exc import DataError from app.config import Settings from app.query.compiler import compile_query @@ -357,6 +359,17 @@ def execute(self, *_args: object, **_kwargs: object) -> _FakeResult: return self._results.pop(0) +class _RaisingResult: + """A canned ``execute()`` result whose ``.all()`` raises -- simulates a raw DBAPIError + surfacing from the RRF execute, e.g. an invalid regex or a statement_timeout cancellation.""" + + def __init__(self, error: Exception) -> None: + self._error = error + + def all(self) -> list[Any]: + raise self._error + + class _FakeEngine: def __init__(self, results: list[Any]) -> None: self._conn = _FakeConn(results) @@ -482,6 +495,34 @@ def test_similarity_null_for_null_cosine_distance(monkeypatch: pytest.MonkeyPatc assert payload["results"][0]["rrf_score"] == 0.3 +@pytest.mark.unit +def test_regex_invalid_repo_filter_maps_to_recoverable_payload( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # A Postgres-invalid repo:/file: filter pattern (e.g. repo:[) reaches the RRF execute as a + # DataError wrapping InvalidRegularExpression; reraise_or_recoverable maps it to + # RegexInvalidError, and _semantic_search_payload maps THAT to the regex_invalid payload + # field (D6) -- never an uncaught exception. + monkeypatch.setattr(semantic, "get_embedder", lambda cfg: lambda texts: [[0.1, 0.2]]) + orig = psycopg.errors.InvalidRegularExpression( + "invalid regular expression: brackets [] not balanced" + ) + engine = _FakeEngine( + [ + _FakeResult(["chunks"]), + _RaisingResult(DataError("SELECT 1", {}, orig)), + ] + ) + + payload = semantic._semantic_search_payload(engine, _cfg(enabled=True), "repo:[ auth flow", 50) + + assert payload["semantic_enabled"] is True + assert payload["results"] == [] + assert payload["count"] == 0 + assert payload["regex_invalid"] == "invalid regular expression: brackets [] not balanced" + assert "reason" in payload + + # ------------------------------------------------------------- filter-semantics: payload shaping diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py index 7f6b27d..f8a929f 100644 --- a/tests/unit/test_service.py +++ b/tests/unit/test_service.py @@ -16,7 +16,7 @@ from app import service from app.config import Settings from app.query.parser import parse -from app.search.errors import QueryTooBroadError +from app.search.errors import QueryTooBroadError, RegexInvalidError from app.search.grep import FileCursor, FileMatches, GrepResult, LineMatch from app.search.references import CandidateSymbol, EdgeSite, ReferenceResult from app.search.symbols import SymbolMatch, SymbolResult @@ -383,6 +383,86 @@ def _raise(*_a: object, **_k: object) -> GrepResult: assert "next_cursor" not in payload +# ------------------------------------------------- regex_invalid (issue #75) + + +@pytest.mark.unit +def test_regex_invalid_on_grep_leg_returns_empty_envelope(monkeypatch: pytest.MonkeyPatch) -> None: + def _raise(*_a: object, **_k: object) -> GrepResult: + raise RegexInvalidError("invalid regular expression: brackets [] not balanced") + + monkeypatch.setattr(service, "grep_search", _raise) + payload = service.search_code_payload(_FakeEngine([]), _cfg(), "/[/", 50) + + assert payload["regex_invalid"] == "invalid regular expression: brackets [] not balanced" + assert payload["files"] == [] + # Unlike QueryTooBroadError, nothing was attempted-and-cut -- the query never ran (D4). + assert payload["truncated"] is False + assert payload["truncation_reason"] is None + assert payload["query_too_broad"] is False + + +@pytest.mark.unit +def test_regex_invalid_on_symbol_leg_keeps_grep_files(monkeypatch: pytest.MonkeyPatch) -> None: + grep_result = GrepResult( + files=( + FileMatches( + repo_id=7, + path="src/handler.go", + lang="go", + content_sha="deadbeef", + branches=("main",), + line_matches=(LineMatch(1, "foo", ((0, 3),)),), + ), + ), + truncated=False, + truncation_reason=None, + regex_incompatible=False, + no_content_atom=False, + zero_width_only_atoms=False, + next_cursor=None, + ) + monkeypatch.setattr(service, "grep_search", lambda *a, **k: grep_result) + + def _raise(*_a: object, **_k: object) -> SymbolResult: + raise RegexInvalidError("invalid regular expression: brackets [] not balanced") + + monkeypatch.setattr(service, "symbol_search", _raise) + engine = _FakeEngine([_FakeResult([_Row(id=7, name="acme/widgets")])]) + + payload = service.search_code_payload(engine, _cfg(), "sym:[ foo", 50) + + assert payload["regex_invalid"] == "invalid regular expression: brackets [] not balanced" + assert payload["file_count"] == 1 # grep's content match is kept, not discarded (D4) + + +@pytest.mark.unit +def test_regex_invalid_on_grep_sets_next_cursor_null_in_pagination_mode( + monkeypatch: pytest.MonkeyPatch, +) -> None: + def _raise(*_a: object, **_k: object) -> GrepResult: + raise RegexInvalidError("invalid regular expression: brackets [] not balanced") + + monkeypatch.setattr(service, "grep_search", _raise) + payload = service.search_code_payload(_FakeEngine([]), _cfg(), "/[/", 50, cursor=None) + + assert payload["regex_invalid"] is not None + assert payload["next_cursor"] is None + + +@pytest.mark.unit +def test_regex_invalid_on_grep_omits_next_cursor_when_bare( + monkeypatch: pytest.MonkeyPatch, +) -> None: + def _raise(*_a: object, **_k: object) -> GrepResult: + raise RegexInvalidError("invalid regular expression: brackets [] not balanced") + + monkeypatch.setattr(service, "grep_search", _raise) + payload = service.search_code_payload(_FakeEngine([]), _cfg(), "/[/", 50) + + assert "next_cursor" not in payload + + # ------------------------------------------------- permalink_branch selection diff --git a/tests/unit/test_webui_main.py b/tests/unit/test_webui_main.py index a7d32e2..c88a6e5 100644 --- a/tests/unit/test_webui_main.py +++ b/tests/unit/test_webui_main.py @@ -21,6 +21,7 @@ from app import service from app.config import Settings +from app.search.errors import RegexInvalidError from app.search.grep import FileCursor, FileMatches, GrepResult, LineMatch from webui.main import api_imports, api_references, app, get_engine, get_settings @@ -273,6 +274,25 @@ def _raise(*_a: object, **_k: object) -> GrepResult: assert resp.json()["detail"]["error"] == "invalid parameter" +@pytest.mark.unit +def test_api_search_invalid_regex_is_400_with_postgres_message( + client: TestClient, monkeypatch: pytest.MonkeyPatch +) -> None: + # Unlike the NUL-byte case above, an invalid regex never reaches this route as a raw + # exception -- service.search_code_payload maps it internally to the regex_invalid field + # (see app/search/errors.py's RegexInvalidError), so the route inspects the payload rather + # than catching an exception (D5). + def _raise(*_a: object, **_k: object) -> GrepResult: + raise RegexInvalidError("invalid regular expression: brackets [] not balanced") + + monkeypatch.setattr(service, "grep_search", _raise) + + resp = client.get("/api/search", params={"q": "/[/"}) + + assert resp.status_code == 400 + assert resp.json()["detail"]["error"] == "invalid regular expression: brackets [] not balanced" + + # ----------------------------------------------------------------------------------- /api/file @@ -598,6 +618,30 @@ def _raise(*_a: object, **_k: object) -> dict[str, Any]: assert resp.json()["detail"]["error"] == "invalid parameter" +@pytest.mark.unit +def test_api_semantic_regex_invalid_is_400( + client: TestClient, monkeypatch: pytest.MonkeyPatch +) -> None: + # Unlike DataError above, regex_invalid never escapes semantic_search_payload as a raw + # exception -- it is a payload field (D6), so the route must inspect the payload (assign- + # then-inspect, per Critic note 3) rather than catching an exception. + payload = { + "query": "repo:[", + "semantic_enabled": True, + "results": [], + "count": 0, + "regex_invalid": "invalid regular expression: brackets [] not balanced", + "reason": "repo:/file: filter values are matched as POSIX regexes by Postgres; fix " + "the pattern", + } + monkeypatch.setattr(service, "semantic_search_payload", lambda *a, **k: payload) + + resp = client.get("/api/semantic", params={"q": "repo:["}) + + assert resp.status_code == 400 + assert resp.json()["detail"]["error"] == "invalid regular expression: brackets [] not balanced" + + @pytest.mark.unit def test_api_semantic_backend_failure_is_502_and_does_not_leak_error_detail( client: TestClient, monkeypatch: pytest.MonkeyPatch diff --git a/webui/main.py b/webui/main.py index e6bfd97..76e90ca 100644 --- a/webui/main.py +++ b/webui/main.py @@ -169,6 +169,12 @@ async def api_search( content. That NUL then reaches a bound SQL parameter in the resumed candidate scan, where Postgres itself rejects it (``sqlalchemy.exc.DataError``: "PostgreSQL text fields cannot contain NUL (0x00) bytes"), 500ing an attacker-controlled input instead of 400ing it. + + A Postgres-invalid regex pattern (``/regex/``, ``repo:``, ``file:``, or ``sym:`` -- e.g. + ``/[/``) is a DIFFERENT case: it never reaches this route as a raw exception at all -- + :func:`service.search_code_payload` maps it internally to the ``regex_invalid`` field (see + ``app/search/errors.py``'s ``RegexInvalidError``), so the check below inspects the payload + exactly like the ``query_parse_error`` check, rather than an ``except`` clause. """ clamped = service.clamp_limit(limit, cfg) try: @@ -179,6 +185,8 @@ async def api_search( raise HTTPException(status_code=400, detail={"error": str(error)}) from error except DataError as error: raise HTTPException(status_code=400, detail={"error": "invalid parameter"}) from error + if payload["regex_invalid"] is not None: + raise HTTPException(status_code=400, detail={"error": payload["regex_invalid"]}) if payload["query_parse_error"] is not None: raise HTTPException(status_code=400, detail={"error": payload["query_parse_error"]}) return payload @@ -257,18 +265,25 @@ async def api_semantic( ``reason`` naming the remedy), or a query with nothing left to embed after filters are excised (``nothing_to_embed`` + ``reason``) -- all pass through unchanged as 200 bodies with ``results: []``/``count: 0``: recoverable conditions are payload fields, never HTTP errors - (mirrors ``app/main.py``'s dispatch contract). Only malformed input and backend faults - become HTTP errors: a NUL byte in ``q``/``branch`` reaching a bound SQL parameter raises - ``DataError`` -> 400 (same rationale as the existing routes); anything else (e.g. the - embedding endpoint's SDK/auth/network failures, which are arbitrary exception types) is - logged with a full traceback server-side and mapped to a generic 502 so a raw error body - never echoes endpoint/host detail (mirrors ``ready()``'s no-leak policy). + (mirrors ``app/main.py``'s dispatch contract) -- EXCEPT ``regex_invalid``: a Postgres-invalid + ``repo:``/``file:`` filter pattern (e.g. ``repo:[``) is mapped to a 400 with the Postgres + message, mirroring ``/api/search``'s ``regex_invalid`` check (D5) -- the filter values are + matched as Postgres regexes even though the surrounding query is natural-language prose, + so a rejected pattern is malformed input, not a ranking outcome. Every other malformed + input and backend fault becomes an HTTP error too: a NUL byte in ``q``/``branch`` reaching a + bound SQL parameter raises ``DataError`` -> 400 (same rationale as the existing routes); + anything else (e.g. the embedding endpoint's SDK/auth/network failures, which are arbitrary + exception types) is logged with a full traceback server-side and mapped to a generic 502 so + a raw error body never echoes endpoint/host detail (mirrors ``ready()``'s no-leak policy). """ clamped = service.clamp_limit(limit, cfg) try: - return await _run_blocking( + payload = await _run_blocking( lambda: service.semantic_search_payload(engine, cfg, q, clamped, branch) ) + if payload.get("regex_invalid") is not None: + raise HTTPException(status_code=400, detail={"error": payload["regex_invalid"]}) + return payload except DataError as error: raise HTTPException(status_code=400, detail={"error": "invalid parameter"}) from error except Exception as error: