feat(us9): drop internal/search + expansion + HyDE (PR-v5-9)#193
Merged
Conversation
…hunk 1/2)
- DELETE internal/search/ (12 files): manager, lanes, llm_filter, rrf, expansion/{expander,hyde}
- REWRITE tools_recall.go: handleRecallSearch uses memoryStore.List + in-memory substring filter; drop preset/by_concept/by_type/similar/timeline/explain actions
- EDIT server.go: remove search.Manager field, searchMgr NewServer param, handlers (handleSearch, handleDecisions, handleChanges, handleHowItWorks, handleExplainSearchRanking) + tool registrations
- EDIT service.go: remove search/expansion imports, searchMgr/llmFilter/queryExpander field wiring
- TRIM 4 docs: ENGRAM_HYDE_ENABLED / ENGRAM_LLM_FILTER_ENABLED env var entries
Cascade tracked in problems.md (chunk 2 scope): internal/mcp/{coerce,tools_memory}.go, internal/worker/{retrieval,handlers_context,handlers_tags}.go
Refs: engram#119
CR: C1-C9 (CodeRabbit-only review per C7)
Closes engram#119 CR: C1-C9 Files (7 consumer edits): - internal/mcp/coerce.go — drop buildSearchParams + buildTimelineParams helpers, drop search import - internal/mcp/tools_memory.go — handleRecallMemory rewritten to use memoryStore.List + in-memory substring filter - internal/mcp/server.go — fix obs.CreatedAt string parsing (time.Parse RFC3339 before Unix compare) - internal/mcp/server_test.go — drop TestHandleExplainSearchRanking_Validation + TestCallTool_SearchTools (tests for deleted handlers); update TestHandleToolsList legacyTools list; redirect TestCallTool_InvalidArgs to find_by_file - internal/worker/retrieval.go — expandQueries now returns []string with original query only; drop DefaultSearchLanes lookups (fixed threshold via config default) - internal/worker/retrieval_helpers.go — drop llmFilter branch in applyLLMFilter (field was removed from Service in chunk 1) - internal/worker/handlers_context.go — handleContextDecisions returns 501 stub with deprecation message - internal/worker/handlers_tags.go — tag-based observation search returns 501 stub (observations table doomed in US3-PR-B anyway) Verification: - go build ./... PASS - go vet ./... PASS - go test ./... -count=1 -short PASS (35 packages green) - rg 'github.com/thebtf/engram/internal/search' --type go → 0 hits - rg 'searchMgr|llmFilter|queryExpander' --type go → 0 code hits (only comments) - rg 'search.(Manager|SearchParams|DefaultSearchLanes|Hit|FuseResults|LLMFilter|ScoredID)' --type go → 0 hits
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Caution Review failedPull request was closed or merged during review WalkthroughPR удаляет внутреннюю подсистему поиска и расширения запросов (HyDE), упрощает инструменты MCP (удаление многих search/timeline/decision инструментов), переводит обработчики на прямое использование observationStore/memoryStore и убирает LLM-фильтрацию и связанные флаги окружения. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MCP as MCP Server
participant Memory as memoryStore
rect rgba(200,200,255,0.5)
Client->>MCP: POST /mcp/tools recall(action="search", project, query, limit)
MCP->>MCP: handleRecall -> validate args
MCP->>Memory: List(ctx, project, fetchLimit)
Memory-->>MCP: []Memory objects
MCP->>MCP: in-memory filter by substring, clamp to limit
MCP-->>Client: JSON {memories, count, query?}
end
Расчётное усилие при рецензировании кода🎯 4 (Complex) | ⏱️ ~60 минут Возможно связанные MR
ОбзорPR удаляет подсистему поиска ( Изменения
Возможно связанные MR
Предлагаемые метки
Стихотворение
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
internal/mcp/server_test.go (1)
456-466: Добавьте сюда явный guard для удалённых v5-инструментов.Сейчас тест проверяет только maintenance-инструменты. Если
searchилиdecisionsслучайно снова попадут вinclude_all, этот регресс не будет пойман.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/server_test.go` around lines 456 - 466, The test currently only checks maintenance tools via removedTools and misses removed v5 tools like "search" and "decisions"; update the check that iterates over removedTools (or add a new removedV5Tools slice) to include "search" and "decisions" and assert they are absent from both toolNames and allToolNames (the same assertions used for maintenance tools) so any accidental re-inclusion into include_all is caught; reference the removedTools variable and the assertions against toolNames and allToolNames in this test.internal/mcp/server.go (2)
2910-2940: Подтвердите производительность in-memory фильтрации для экспорта.Код загружает до
params.Limitнаблюдений, затем фильтрует. Если большинство отфильтровывается, результат может быть значительно меньше ожидаемого. Рассмотрите загрузку с запасом или документирование этого поведения.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/server.go` around lines 2910 - 2940, The in-memory filtering after calling s.observationStore.GetAllRecentObservations(ctx, params.Limit) can return far fewer than params.Limit results if many items are filtered out; update the logic in the handler that builds observations (use GetAllRecentObservations, params.Limit and the observations slice) to either (a) fetch with a safety multiplier or an iterative loop (e.g., request larger page sizes or repeatedly call GetAllRecentObservations with increasing offsets/limits until you accumulate params.Limit matching observations or hit a hard cap) so the exported set aims to contain params.Limit post-filter results, or (b) add explicit documentation and/or return metadata indicating that params.Limit is an input to the pre-filter fetch and final results may be smaller; implement one approach and add a configurable max fetch cap to avoid unbounded reads.
2473-2491: Подтвердите поведение FTS + in-memory фильтрации для точного совпадения тегов.Логика корректна: FTS возвращает кандидатов, затем фильтруем по точному совпадению тега в
Concepts. Однако FTS может не вернуть все наблюдения с нужным тегом если лимит исчерпан раньше. Рассмотрите увеличение лимита FTS-запроса или добавление примечания в документацию.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/server.go` around lines 2473 - 2491, FTS may return fewer candidates than exist when params.Limit is small, so update the search in server.go where you call s.observationStore.SearchObservationsFTS(ctx, params.Tag, params.Project, params.Limit) to request a larger candidate set (e.g. use a higher cap than params.Limit or page/fetch until you collect at least params.Limit exact matches) before applying the in-memory exact-match filter on obs.Concepts (the filtered []*models.Observation logic); alternatively add a clear comment/ docs note near SearchObservationsFTS and the filtering block describing that FTS is a candidate provider and may need a larger limit to guarantee exhaustive tag matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/mcp/server.go`:
- Around line 2926-2937: The date filter compares params.DateStart/DateEnd
(which are epoch milliseconds per the export_observations schema) against
ts.Unix() (seconds), causing wrong filtering; update the comparisons in the
export logic that reads obs.CreatedAt—specifically replace uses of ts.Unix()
with ts.UnixMilli() (or alternatively convert params.DateStart/DateEnd to
seconds by dividing by 1000) so that the comparisons between ts and
params.DateStart/DateEnd use the same millisecond unit; apply this change to all
checks using ts.Unix() for params.DateStart and params.DateEnd.
- Around line 1688-1694: Handler currently requires project even though the
find_by_file tool schema only lists "files" as required; to fix, make the code
consistent with the schema by removing (or guarding) the strict check that
returns an error when project == "" in the handler (the `if project == "" {
return "", fmt.Errorf("project parameter is required") }` block) and instead
handle an empty project as a valid case (e.g., treat it as global search or pass
a nil/empty value downstream), or alternatively update the find_by_file schema
to include "project" in required if project should truly be mandatory—pick one
approach and apply it consistently so `find_by_file` and the handler agree.
In `@internal/mcp/tools_recall.go`:
- Around line 101-104: Переменная limit, полученная через coerceInt в
функции/файле tools_recall.go, сейчас при limit>100 сбрасывается на 20, что даёт
неожиданно мало результатов; нужно вместо этого применять верхнюю границу 100.
Измените логику проверки: если limit <= 0 — установить 20, иначе если limit >
100 — установить 100 (т.е. clamp верхним пределом), сохранив
coerceInt(m["limit"], 20) как исходное значение.
- Around line 118-134: The in-memory substring filter runs after calling
s.memoryStore.List(ctx, project, limit), so you only search the last limit
memories and may miss older matches; change the flow in the recall routine so
filtering by query happens over the full candidate set before truncation: either
call s.memoryStore.List without the limit (or with a higher sentinel like 0
meaning "all") and then apply the case-insensitive substring filter on memories
(the queryLower / strings.Contains logic) and finally enforce the limit when
returning results, or push the query into memoryStore.List if the store
implementation supports server-side filtering; update the code paths around
s.memoryStore.List, the memories slice filtering, and the final limiting logic
to ensure the limit is applied after filtering.
In `@internal/worker/handlers_context.go`:
- Around line 988-998: The handler currently returns a 200 OK with an empty
payload but should return 501 Not Implemented; before calling writeJSON (the
helper used to serialize the map containing "project", "query", etc.) set the
response status to 501 (e.g. call w.WriteHeader(http.StatusNotImplemented)) or
switch to a writeJSON variant that accepts a status code so clients receive the
proper Not Implemented response for the deprecated decisions preset search
endpoint.
In `@internal/worker/handlers_tags.go`:
- Around line 338-347: The handler currently serializes a JSON body with
writeJSON (including "deprecated") which results in a 200 OK; change it to
return HTTP 501 Not Implemented for this deprecated endpoint by calling
w.WriteHeader(http.StatusNotImplemented) and avoid returning the normal payload
(do not call writeJSON with the tag/observations body) so the response is a 501
with an empty body; update the code around the writeJSON call (and the
surrounding variables like tag / _ = limit) in handlers_tags.go to implement
this.
---
Nitpick comments:
In `@internal/mcp/server_test.go`:
- Around line 456-466: The test currently only checks maintenance tools via
removedTools and misses removed v5 tools like "search" and "decisions"; update
the check that iterates over removedTools (or add a new removedV5Tools slice) to
include "search" and "decisions" and assert they are absent from both toolNames
and allToolNames (the same assertions used for maintenance tools) so any
accidental re-inclusion into include_all is caught; reference the removedTools
variable and the assertions against toolNames and allToolNames in this test.
In `@internal/mcp/server.go`:
- Around line 2910-2940: The in-memory filtering after calling
s.observationStore.GetAllRecentObservations(ctx, params.Limit) can return far
fewer than params.Limit results if many items are filtered out; update the logic
in the handler that builds observations (use GetAllRecentObservations,
params.Limit and the observations slice) to either (a) fetch with a safety
multiplier or an iterative loop (e.g., request larger page sizes or repeatedly
call GetAllRecentObservations with increasing offsets/limits until you
accumulate params.Limit matching observations or hit a hard cap) so the exported
set aims to contain params.Limit post-filter results, or (b) add explicit
documentation and/or return metadata indicating that params.Limit is an input to
the pre-filter fetch and final results may be smaller; implement one approach
and add a configurable max fetch cap to avoid unbounded reads.
- Around line 2473-2491: FTS may return fewer candidates than exist when
params.Limit is small, so update the search in server.go where you call
s.observationStore.SearchObservationsFTS(ctx, params.Tag, params.Project,
params.Limit) to request a larger candidate set (e.g. use a higher cap than
params.Limit or page/fetch until you collect at least params.Limit exact
matches) before applying the in-memory exact-match filter on obs.Concepts (the
filtered []*models.Observation logic); alternatively add a clear comment/ docs
note near SearchObservationsFTS and the filtering block describing that FTS is a
candidate provider and may need a larger limit to guarantee exhaustive tag
matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 324fc408-bb28-4cb4-af03-60d12f149289
📒 Files selected for processing (26)
CHANGELOG.mdREADME.mdREADME.ru.mdREADME.zh.mdinternal/mcp/coerce.gointernal/mcp/server.gointernal/mcp/server_test.gointernal/mcp/tools_memory.gointernal/mcp/tools_recall.gointernal/search/expansion/expander.gointernal/search/expansion/expander_test.gointernal/search/expansion/hyde.gointernal/search/expansion/hyde_test.gointernal/search/lanes.gointernal/search/lanes_test.gointernal/search/llm_filter.gointernal/search/llm_filter_test.gointernal/search/manager.gointernal/search/manager_test.gointernal/search/rrf.gointernal/search/rrf_test.gointernal/worker/handlers_context.gointernal/worker/handlers_tags.gointernal/worker/retrieval.gointernal/worker/retrieval_helpers.gointernal/worker/service.go
💤 Files with no reviewable changes (16)
- README.zh.md
- README.ru.md
- internal/search/lanes.go
- internal/search/rrf_test.go
- internal/worker/service.go
- README.md
- internal/search/manager_test.go
- internal/search/llm_filter_test.go
- internal/search/rrf.go
- internal/search/lanes_test.go
- internal/search/expansion/hyde_test.go
- internal/search/expansion/expander_test.go
- internal/search/expansion/expander.go
- internal/search/expansion/hyde.go
- internal/search/llm_filter.go
- internal/search/manager.go
- CRIT: server.go handleListObservations — DateStart/DateEnd treated as ms (Unix → UnixMilli) to match convention elsewhere - MAJOR: server.go handleFindByFileObservations — drop "project is required" guard (schema allows empty project, store supports global scan) - MAJOR: handlers_context.go handleContextDecisions — write 501 status before deprecated JSON payload - MAJOR: handlers_tags.go tag search — write 501 status before deprecated JSON payload - MINOR: tools_recall.go handleRecallSearch — limit clamp > 100 returns 100; when query != "" fetch 10x candidates (min 1000) before in-memory filter - MINOR: server_test.go — add "search" + "decisions" to removedTools regression guard Nitpicks canResolve=false deferred: server.go export filter + FTS candidate ceilings belong to US3-PR-B observations drop. Refs: engram#119
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
1 participant
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.
Summary
Drops
internal/search/*package entirely (12 files, ~90KB) +internal/search/expansion/*(HyDE, query expansion, 4 files). ReducesrecallMCP tool to trivial memoryStore-backed filter. Closes engram#119.This unblocks US3-PR-B (engram#120) per plan.md's inverted phase order — the
ObservationStore/IndexedSession/PromptStoretypes referenced by search package are no longer blocking the observations table drop.Changes
Chunk 1 — deletion + primary rewrite
git rm -r internal/search/(12 files)internal/mcp/tools_recall.go—handleRecallSearchusesmemoryStore.List+ in-memory substring filter; droppreset/by_concept/by_type/similar/timeline/explainactionsinternal/mcp/server.go— dropsearchMgrfield +NewServerparam; removehandleSearch,handleDecisions,handleChanges,handleHowItWorks,handleExplainSearchRanking+ their tool registrationsinternal/worker/service.go— drop search/expansion imports +searchMgr/llmFilter/queryExpanderfield wiringENGRAM_HYDE_ENABLED/ENGRAM_LLM_FILTER_ENABLEDrefs, add US9 Removed entry in CHANGELOGChunk 2 — cascade cleanup (7 consumer files)
internal/mcp/coerce.go— dropbuildSearchParams+buildTimelineParams, drop search importinternal/mcp/tools_memory.go—handleRecallMemoryrewritten:memoryStore.List+ in-memory substring matchinternal/mcp/server.go— fixobs.CreatedAtstring parsing (time.Parse RFC3339 before Unix compare)internal/mcp/server_test.go— drop tests for deleted handlers; updatelegacyToolslist; redirectTestCallTool_InvalidArgstofind_by_fileinternal/worker/retrieval.go—expandQueriesreturns[]string{query}; dropDefaultSearchLaneslookups (fixed threshold via config default)internal/worker/retrieval_helpers.go— dropllmFilterbranch (field gone in chunk 1)internal/worker/handlers_context.go—handleContextDecisionsreturns 501 stub with deprecation messageinternal/worker/handlers_tags.go— tag search returns 501 stub (observations doomed in US3-PR-B anyway)Verification
go build ./...PASSgo vet ./...PASSgo test ./... -count=1 -shortPASS (35 packages)rg 'github.com/thebtf/engram/internal/search' --type go→ 0 hitsrg 'searchMgr|llmFilter|queryExpander' --type go→ 0 code refs (only comments)rg 'search.(Manager|SearchParams|DefaultSearchLanes|Hit|FuseResults|LLMFilter|ScoredID)' --type go→ 0 hitsPlan refs
Breaking changes
recallMCP actions dropped:preset(decisions/changes/how_it_works),by_concept,by_type,similar,timeline,explain. Clients getaction not supported in v5error pointing torecall(action="search")./api/context/decisionsand tag-based observation search return 501 with deprecation payload (contract preserved).Deferred
Summary by CodeRabbit
Примечания к выпуску
Удаленные функции
Документация
Тесты