Skip to content

fix(holographic): keep hyphenated identifiers searchable (split on -/_) - #64256

Open
dsitmilis wants to merge 1 commit into
NousResearch:mainfrom
dsitmilis:fix/holographic-hyphenated-search
Open

fix(holographic): keep hyphenated identifiers searchable (split on -/_)#64256
dsitmilis wants to merge 1 commit into
NousResearch:mainfrom
dsitmilis:fix/holographic-hyphenated-search

Conversation

@dsitmilis

Copy link
Copy Markdown

fix(holographic): hyphenated-identifier search returned 0 results

_Sanitize FTS query glued hyphenated tokens (vibe-trading -> vibetrading)
but FTS5's default tokenizer splits stored text on the same separators, so
the glued token never existed in the index and search returned 0 hits for
any hyphenated identifier (MCP server names, package names, domains).

Split each token on '-' and '_' before sanitizing so identifiers expand
into OR-joined sub-tokens (vibe-trading -> "vibe" OR "trading"). Phrase
quoting preserved; non-hyphenated queries unaffected.

Re-submission of closed PR #64209. Upstream main still uses plain query.lower().split() which glues vibe-trading into vibetrading; FTS5 tokenizes on -/_ so the glued form never matches. This splits tokens on -/_ into OR-joined sub-tokens.

_Sanitize FTS query glued hyphenated tokens (vibe-trading -> vibetrading)
but FTS5's default tokenizer splits stored text on the same separators, so
the glued token never existed in the index and search returned 0 hits for
any hyphenated identifier (MCP server names, package names, domains).

Split each token on '-' and '_' before sanitizing so identifiers expand
into OR-joined sub-tokens (vibe-trading -> "vibe" OR "trading"). Phrase
quoting preserved; non-hyphenated queries unaffected.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jul 14, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #14033 (earliest-open canonical for the holographic-memory FTS5 hyphenated-query cluster; same plugins/memory/holographic/retrieval.py sanitize mechanism). This is a re-submission of closed #64209, which was already part of the same cluster.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the hyphen-loss path. The premise remains valid on current main: plugins/memory/holographic/retrieval.py:603-615 removes - before its FTS5 MATCH query at line 503.

Problems

  • The added loops in plugins/memory/holographic/retrieval.py:611-612 turn vibe-trading into "vibe" OR "trading", which broadens an identifier lookup to either component. SessionDB instead quotes identifiers to preserve phrase semantics (hermes_state.py:4821-4827).
  • _ is not removed by the current _FTS_SPECIAL set (retrieval.py:603), so splitting it newly broadens existing underscore queries without a demonstrated failure.
  • The PR changes no tests. Existing coverage at tests/plugins/memory/test_holographic_retrieval.py:22-129 has prose-recall checks but no identifier retrieval regression.

Suggested changes

  • Preserve an identifier as one quoted phrase, or demonstrate and test why OR-expanded matching is intended; avoid splitting underscores without a failing case.
  • Add sanitizer and integration coverage for hyphenated identifiers through both FactRetriever.search() and MemoryStore.search_facts().

This is an automated hermes-sweeper review.

# into OR-joined sub-tokens. FTS5's default tokenizer also splits
# on those separators, so gluing them (the old behavior) produced a
# token that never exists in the index and silently returned 0 hits.
word = raw.strip(".,;:!?\"'()[]{}#@<>")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting here turns vibe-trading into "vibe" OR "trading" and also broadens underscore queries even though current _FTS_SPECIAL does not remove _. Please preserve a single quoted identifier phrase (as SessionDB does at hermes_state.py:4821-4827) or add a regression demonstrating why the broader OR semantics are required.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 16, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Twenty PRs address or reference this issue complex: the session-search branch was implemented by #1776, while the unresolved holographic branch concerns preserving hyphenated identifiers through the shared FTS5 sanitizer rather than deleting or broadly OR-splitting their components; several other PRs are adjacent, superseded, contaminated, or unrelated changes.

Related pull requests

Duplicates

#1777 duplicates the session-search core of merged #1776, with #7820 a narrower follow-up; #14033, #14262, #14794, #23221/#39663, #51124, #64209/#64256, and #75095 overlap on holographic FTS5 sanitization, while #16755, #16765, and #16827 are superseded predecessors of merged #16915.

Suggested consolidation

Keep #14033 open as the canonical salvage path because it is a recorded best fix and covers both holographic search paths, but require it to extend the current shared sanitizer without losing prose recall, add colon handling, and retain both-path integration coverage; keep #6667, #16040, and #32545 separate because their surviving scopes are distinct. Despite the keep_open review on #64256, its visible diff OR-expands identifier components, unnecessarily splits underscores, and adds no tests, so close it as a duplicate of #14033 together with #14262, #14794, and #39663 once their useful test cases are carried over; leave closed #51124 as the recorded best-fix reference rather than reopening it, and treat #75095 as already implemented on main via 638d2e7.

Complex graph

flowchart TD
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I14024(["issue #14024 (open)"])
    I21102(["issue #21102 (closed)"])
    subgraph Dup14033 ["PRs duplicating each other"]
        P14033["PR #14033 (open)"]
        P14262["PR #14262 (open)"]
        P14794["PR #14794 (open)"]
        P23221["PR #23221 (closed)"]
        P39663["PR #39663 (open)"]
        P51124["PR #51124 (closed)"]
        P64209["PR #64209 (closed)"]
        P64256["PR #64256 (open)"]
        P75095["PR #75095 (closed)"]
    end
    P64256 -.->|partial| I14024
    P64256 -.->|partial| I21102
    class I14024 open
    class I21102 closed
    class P14033 open
    class P14262 open
    class P14794 open
    class P23221 closed
    class P39663 open
    class P51124 closed
    class P64209 closed
    class P64256 open
    class P75095 closed
    class P14033 best
    class P51124 best
    class P51124 best
    class P64256 target
    click I14024 "https://github.com/NousResearch/hermes-agent/issues/14024"
    click I21102 "https://github.com/NousResearch/hermes-agent/issues/21102"
    click P14033 "https://github.com/NousResearch/hermes-agent/pull/14033"
    click P14262 "https://github.com/NousResearch/hermes-agent/pull/14262"
    click P14794 "https://github.com/NousResearch/hermes-agent/pull/14794"
    click P23221 "https://github.com/NousResearch/hermes-agent/pull/23221"
    click P39663 "https://github.com/NousResearch/hermes-agent/pull/39663"
    click P51124 "https://github.com/NousResearch/hermes-agent/pull/51124"
    click P64209 "https://github.com/NousResearch/hermes-agent/pull/64209"
    click P64256 "https://github.com/NousResearch/hermes-agent/pull/64256"
    click P75095 "https://github.com/NousResearch/hermes-agent/pull/75095"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 20 pull requests and 4 issues in this complex. Each diff was read against this issue; Assessment working set: 190 kB of PR diffs, 43 kB of issue/PR text, 23 kB of discussion (40 comments), 31 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants