fix(fts): AND-join expand_fts_query groups so mixed group+phrase parses - #447
Merged
Conversation
expand_fts_query joined its per-word groups with whitespace. FTS5's grammar accepts implicit-AND between two bare phrases (a b) but rejects it when either operand is a parenthesized group: (a OR b) "c" raises "fts5: syntax error". This fired whenever a multi-token identifier (which becomes an OR group) sat next to a single-token word (a bare phrase) in the same query. A knowledge-graph entity named cortex_viz/__main__.py hit it exactly -- get_causal_chain's entity-mention lookup crashed on the SQLite backend with 'fts5: syntax error near ""__main__""'. Fix: join groups with an explicit AND. FTS5 treats "a b" and "a AND b" as identical, but the explicit form is grammatically valid for every group shape, so the crash is gone with no change in match semantics. Regression tests run expand_fts_query output against a real in-memory FTS5 table (parametrized over the group-then-phrase, phrase-then-group, and interleaved cases) asserting no OperationalError, plus one test proving the join is a conjunction (group AND phrase both required). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fnomaXS71hgxMw2zr7HGR
This was referenced Aug 24, 2026
Owner
Author
|
ZETETIC-REVIEW: APPROVE Independent review (fresh-context reviewer, read-only via gh, head 62ef415):
Non-blocking notes:
|
cdeust
added a commit
that referenced
this pull request
Aug 24, 2026
… drift) origin/main advanced past this branch's fork point (#447 FTS5 fix, #448 error-classification fix) and #448 fixed mcp_server/tool_error_handler.py _classify_error's method-size violation (was in the branch-point baseline; now 21 lines on main, verified by AST), pruning it from main's .craftsmanship-baseline.json. This branch's copy of the file still carried the stale entry, which the gate's ratchet rule flags as "added without a base-ref match" once compared against a freshly fetched origin/main (the ratchet may only shrink within a PR). Not something this PR's own changes caused — reproduced by stashing every change in this branch back to dddc001 and re-running the gate against fresh origin/main; the same failure appears. Synced this branch's baseline to match origin/main's exactly for this one entry (diffed byte-for-byte confirmed no other divergence in either direction) rather than grandfathering it or regenerating blind. Co-Authored-By: Claude <noreply@anthropic.com>
cdeust
added a commit
that referenced
this pull request
Aug 24, 2026
…s instead of hanging (#449) * fix(ap): bound interactive AP read calls so a wedged pipeline degrades The AP MCP client runs with callTimeoutMs=0 — deliberately unbounded, because a fresh index_codebase of a large tree legitimately exceeds any fixed wall clock. The only backstop is the 600s wedge-silence window. That is correct for ingestion but wrong for the interactive read path: search_codebase (behind unified_search) and the get_symbol / get_context / get_impact / get_processes / health_check lookups inherited the same 600s, so an AP that connects but then wedges made unified_search / get_causal_chain hang for up to 10 minutes instead of degrading to Cortex-only results — the "unreachable => status=partial" promise in unified_search's docstring was not actually kept. Fix: APBridge.call gains an optional timeout_s that wraps the client call in asyncio.wait_for and degrades a timeout to None (recorded reason + stderr note), exactly like every other AP failure. The interactive read wrappers pass interactive_call_timeout_s() (30s default, env-overridable via CORTEX_AP_INTERACTIVE_TIMEOUT_S, 20x below the 600s indexing window). The indexing/write wrappers (index_codebase, analyze_codebase, resolve_graph, cluster_graph, detect_changes) and the build-loop query_graph stay unbounded. A None from a timed-out search_codebase flows through as_list(None) -> [] -> unified_search returns status=partial with Cortex-only hits. Tests: a hanging fake client returns None within a 50ms ceiling; the interactive wrapper forwards a positive timeout while index_codebase forwards none; the default ceiling is positive and strictly below the 600s wedge window and honors/validates the env override. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fnomaXS71hgxMw2zr7HGR * fix(ap): extract APBridge._degrade to clear the 40-line method-size gate APBridge.call spanned 42 lines (repo cap: 40, scripts/craftsmanship_rules.py METHOD_LINE_LIMIT). Both except-branches duplicated the same "record _unavailable_reason + stderr note + return None" degrade logic; pulled into one _degrade(reason, note) helper, called from both branches. Fixes the CI craftsmanship-gate FAILURE on 3debfda (PR #449 review finding 1). Co-Authored-By: Claude <noreply@anthropic.com> * fix(ap): surface AP call failure in unified_search status, not just the config flag unified_search.handler derived status solely from the static is_enabled() config check. An AP that is enabled but times out or errors on the actual search_codebase call returned [] indistinguishably from AP genuinely finding nothing: status="ok", sources=["cortex","ap"], counts.ap=0 either way. This silently hid the exact failure mode #3debfda's timeout fix was meant to expose. WorkflowGraphASTSource gains last_search_degraded_reason, reading the outcome APBridge._degrade already records on its own call path (no widened return type, no second round-trip) — None when the call succeeded or was never attempted (AP disabled / no graph configured), set when it genuinely failed. unified_search now sets status="partial" and a degraded: {source, reason} field whenever AP was attempted but failed, keeping status="ok", degraded=null for a genuine empty AP result. PR #449 review finding 2. Co-Authored-By: Claude <noreply@anthropic.com> * docs(ap): remove false get_causal_chain AP-hang claim from source comments get_causal_chain (mcp_server/handlers/get_causal_chain.py) imports only MemoryStore and core.knowledge_graph — no AP dependency, so it cannot hang on a wedged AP call. mcp_call_timeout.py's interactive-ceiling comments and the interactive-timeout test's module docstring both claimed otherwise; corrected to name unified_search only, with an explicit note that get_causal_chain is unaffected. PR #449 review finding 3 (PR body corrected separately via gh pr edit). Co-Authored-By: Claude <noreply@anthropic.com> * fix(ap): extract search_codebase hit normalization to clear the 300-line/40-line gates The degraded-tracking addition (bd74351) pushed workflow_graph_source_ast.py to 316 lines (cap 300) and search_codebase's body to 45 lines (cap 40) — a NEW craftsmanship violation not in the base-ref baseline, caught by CI on 30b2d20. Local `python3 scripts/check_craftsmanship.py` (no args) at the time compared against a stale local origin/main; re-running after `git fetch origin main` reproduces CI's FAILURE exactly. Root fix: search_codebase's row-normalization loop (id/qualified_name/ file_path/score/snippet construction from the raw AP response) is lifted into workflow_graph_ast_response.normalize_search_hits — the same module that already owns "normalize an AP response shape" for the symbol- and edge-loading concerns (issue #275's split), so this is the same seam, not a new one. search_codebase now just resolves the graph path, calls the bridge, and delegates. Docstrings on search_codebase/last_search_degraded_reason trimmed to essentials. workflow_graph_source_ast.py: 316 -> 288 lines. search_codebase: 45 -> delegates (no longer a > 40-line method). Behavior-preserving; targeted tests (ap_bridge/mcp_client/workflow_graph + the two new degraded-status test files + response_budget_wiring) all still green. Co-Authored-By: Claude <noreply@anthropic.com> * fix(ap): reset APBridge._unavailable_reason at the top of every call() _unavailable_reason was only cleared in connect()'s slow (reconnect) path. connect()'s fast path (client already connected) skipped the clear, and call()'s success branch never reset it either. On a reused instance — the codebase already does this, wiki_verify.py reuses one WorkflowGraphASTSource across its candidate loop — a stale reason from an earlier failed call leaked into a later successful one, so last_search_degraded_reason could report a call as failed when it had actually just succeeded. Contradicted its own docstring ("None if it succeeded"). Fix: call() resets self._unavailable_reason = None right after the tool-allowlist check, before touching connect()/the RPC — each call's own outcome is now authoritative regardless of prior calls on the same bridge instance. call() stays at 39 lines (cap 40). New regression test: two calls on one APBridge instance, first via a hanging client (fails), second via a client that succeeds, asserting unavailable_reason is set after the first and None after the second. Fails on pre-fix code (verified via git stash), passes after. PR #449 review round 3 (Medium finding, house "no deferred coverage on new code" rule). Co-Authored-By: Claude <noreply@anthropic.com> * chore(craftsmanship): sync baseline ratchet to origin/main (unrelated drift) origin/main advanced past this branch's fork point (#447 FTS5 fix, #448 error-classification fix) and #448 fixed mcp_server/tool_error_handler.py _classify_error's method-size violation (was in the branch-point baseline; now 21 lines on main, verified by AST), pruning it from main's .craftsmanship-baseline.json. This branch's copy of the file still carried the stale entry, which the gate's ratchet rule flags as "added without a base-ref match" once compared against a freshly fetched origin/main (the ratchet may only shrink within a PR). Not something this PR's own changes caused — reproduced by stashing every change in this branch back to dddc001 and re-running the gate against fresh origin/main; the same failure appears. Synced this branch's baseline to match origin/main's exactly for this one entry (diffed byte-for-byte confirmed no other divergence in either direction) rather than grandfathering it or regenerating blind. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
expand_fts_queryjoined its per-word groups with whitespace. FTS5's grammar accepts implicit-AND between two bare phrases (a b) but rejects it when either operand is a parenthesized group:(a OR b) "c"raisesfts5: syntax error.This fired whenever a multi-token identifier (which becomes an
(x OR y OR z)OR-group of camelCase/snake_case sub-tokens) sat next to a single-token word (a bare phrase) in the same query.How it surfaced
A knowledge-graph entity named
cortex_viz/__main__.pyhit it exactly.get_causal_chain's entity-mention lookup (get_memories_mentioning_entity→expand_fts_query→ FTS5MATCH) crashed on the SQLite backend with:because the query expanded to
("cortex_viz" OR "cortex" OR "viz") "__main__" "py"— a group immediately followed by a bare phrase. This was surfaced by running the harness-comparison benchmark against a real ingested store (cortex-viz), where it degraded the P2 (highest fan-in) probe to a partial result.Fix
Join the groups with an explicit
AND. FTS5 treatsa banda AND bas identical, so match semantics are unchanged, but the explicit form is grammatically valid for every group shape — the crash is gone regardless of how groups and bare phrases interleave.Verified against the live benchmark DB: the previously-crashing entity now returns 11 hits instead of raising.
Tests
test_expand_query_preserves_and_across_wordsto expect the explicitANDjoin.expand_fts_queryoutput against a real in-memory FTS5 table across the group-then-phrase (original crash), phrase-then-group, and interleaved cases — asserting noOperationalError(skipped gracefully if sqlite3 was built without FTS5).Gates:
ruff check✓ ·ruff format --check✓ · craftsmanship gate ✓ ·pytest tests_py/shared/test_code_tokenize.py→ 19 passed.🤖 Generated with Claude Code
Generated by Claude Code