feat(us7): drop consolidation + maintenance + reranking + smart_gc (PR-v5-7)#185
Merged
Conversation
…R-v5-7)
Implements plan.md §Phase 7 — deletes 4 internal packages and all caller
code. ~5000 LOC removed across 26 files.
Deleted packages (19 files total):
internal/consolidation/ — 6 files (scheduler, associations, similarity)
internal/maintenance/ — 9 files (service, smart_gc, entity_extraction,
file_staleness, hit_rate, near_dedup,
wiki_generation) + 2 test files
internal/reranking/ — 4 files (api, interface, types + test)
Plus internal/worker/handlers_maintenance.go (HTTP handlers that owned
/api/maintenance/* routes).
relation package: plan-v1 listed this, but it was already gone from main
before US7 started — not in this diff.
Caller cleanup (7 files edited):
internal/worker/service.go
- Remove imports: consolidation, maintenance, reranking
- Remove struct fields: reranker, consolidationScheduler, maintenanceService
- Remove createReranker() method
- Remove reranker init + consolidation scheduler init + smartGC/maintenance
init blocks
- Remove 11 /api/maintenance/* route registrations
- Remove shutdown cleanup for reranker.Close() and consolidationScheduler.Stop()
- Reduce mcp.NewServer call from 14 to 12 args
internal/worker/retrieval.go
- Remove reranking import + strconv import
- Remove rerank/rerankByScore function fields from retrievalHooks
- Remove rerankResults method
- applyReranking becomes no-op (returns candidates unchanged)
internal/worker/handlers_patterns.go
- Remove maintSvc := s.maintenanceService local + orphan-detection block
from handlePatternCleanupAdvanced (the orphan cleanup was owned by
maintenance service; without maintenance, this step is a no-op)
internal/worker/handlers_update.go
- Remove "Cross-Encoder Reranker" health-check status block from the
/health endpoint (~28 lines)
internal/mcp/server.go
- Remove consolidation/maintenance imports + 2 Server struct fields
- Remove 2 NewServer params + their assignments
- Remove "maintenance"/"consolidation" mentions from admin tool description
- Remove suggest_consolidations / trigger_maintenance / get_maintenance_stats
tool registrations + dispatch cases + handler methods
internal/mcp/tools_admin.go
- Remove "consolidations" from handleAdmin switch + error message
internal/mcp/server_test.go
- NewServer calls reduced from 14 to 12 args (replace_all, 50+ call sites)
- Remove 6 test functions that referenced dropped handlers:
TestHandleTriggerMaintenance_Validation, _NilService
TestHandleGetMaintenanceStats_Validation, _NilService
TestHandleSuggestConsolidations_Validation, _ValidationExtended
- legacyTools slice in TestHandleToolsList no longer lists trigger_maintenance
internal/mcp/tools_learning_test.go
- NewServer call reduced from 14 to 12 args
Verified:
go build ./... clean
go vet ./... clean
go test -short -count=1 ./... all suites green
Conventions compliance (plan.md §Cross-Phase Conventions):
C3: no new migrations — these packages own no tables
C7: CodeRabbit-only review
C8: single coherent PR (~5000 LOC delete)
Unblocks US5 (internal/pattern/ had a consumer in maintenance/service.go;
that consumer is now gone).
Plan.md §Phase 7.
WalkthroughЭтот PR полностью удаляет подсистемы консолидирования и обслуживания: движок ассоциаций, планировщик, векторное сходство, переранжирование, извлечение сущностей, генерацию вики, Smart GC, обработчики и связанные тесты и маршруты. Changes
Estimated code review effort🎯 4 (Сложный) | ⏱️ ~60 минут Possibly related PRs
Poem
🚥 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.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the consolidation, maintenance, and reranking features, including their respective packages and associated MCP tools. The feedback identifies inconsistencies in the hardcoded lists of valid actions for the admin tool within error messages and descriptions, recommending synchronization. Furthermore, the applyReranking function in the retrieval logic is now a no-op and should be removed to eliminate dead code.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/worker/retrieval.go (1)
459-460: Уберите no-op этапapplyReranking, чтобы пайплайн отражал фактическую логику.На Line 459 функция стала технической заглушкой (возврат входа без изменений). Лучше удалить её и вызов в
RetrieveRelevant, чтобы не держать «пустой» этап.♻️ Предлагаемое упрощение
@@ - if len(freshObservations) > 0 { - freshObservations = s.applyReranking(query, freshObservations, similarityScores) - }-func (s *Service) applyReranking(query string, observations []*models.Observation, similarityScores map[int64]float64) []*models.Observation { - // Reranking removed in v5. - return observations -}As per coding guidelines "Complete, working implementations only - no stub code or placeholders".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/retrieval.go` around lines 459 - 460, Remove the no-op stage by deleting the applyReranking function and removing its invocation inside RetrieveRelevant so the retrieval pipeline no longer contains a stub step; update RetrieveRelevant to pass observations directly to the next real stage (or return them) and ensure any imports, tests, or callers that referenced applyReranking are cleaned up or adapted to the new flow.internal/mcp/server_test.go (1)
425-428: Добавьте регрессионную проверку на отсутствие удалённых tool-ов.Сейчас тест фиксирует только legacy alias-ы, которые должны появляться при
include_all=true, но не проверяет главное изменение этой PR:trigger_maintenance,get_maintenance_stats,suggest_consolidationsиrun_consolidationбольше не должны рекламироваться ни в default list, ни вinclude_all. Параassert.Falseздесь лучше закроет риск случайного возврата удалённого surface.🧪 Пример
// Legacy tools should now be present for _, name := range legacyTools { assert.True(t, allToolNames[name], "legacy tool %s should be present with include_all=true", name) } + + removedTools := []string{ + "trigger_maintenance", + "get_maintenance_stats", + "suggest_consolidations", + "run_consolidation", + } + for _, name := range removedTools { + assert.False(t, toolNames[name], "removed tool %s should not be in default listing", name) + assert.False(t, allToolNames[name], "removed tool %s should not be present with include_all=true", name) + }🤖 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 425 - 428, The test currently only asserts presence of legacy alias tools via the legacyTools slice but doesn't assert that removed tools ("trigger_maintenance", "get_maintenance_stats", "suggest_consolidations", "run_consolidation") are absent; update the test (around the legacyTools usage in server_test.go) to add assertions (e.g., using assert.False) that these four tool names are not present in both the default advertised list and the list when include_all=true so the regression is caught if they reappear.
🤖 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/tools_admin.go`:
- Line 16: The allowed admin.action values are duplicated and inconsistent:
update code to centralize the valid actions into a single authoritative variable
(e.g., a package-level slice or map like validAdminActions) and use that
variable both when validating actions (the switch handling the actions) and when
constructing the error message (the fmt.Errorf return currently listing
actions); modify the switch that handles compress_aaak, set_aaak_code,
taxonomy_stats to reference the centralized list so those actions are included,
and update docs/arch/API_CONTRACTS.md to reference the same canonical list or
generate the docs from it so the client and docs share one source of truth.
Ensure the variable is used in both the error text and the validation logic so
they remain consistent.
---
Nitpick comments:
In `@internal/mcp/server_test.go`:
- Around line 425-428: The test currently only asserts presence of legacy alias
tools via the legacyTools slice but doesn't assert that removed tools
("trigger_maintenance", "get_maintenance_stats", "suggest_consolidations",
"run_consolidation") are absent; update the test (around the legacyTools usage
in server_test.go) to add assertions (e.g., using assert.False) that these four
tool names are not present in both the default advertised list and the list when
include_all=true so the regression is caught if they reappear.
In `@internal/worker/retrieval.go`:
- Around line 459-460: Remove the no-op stage by deleting the applyReranking
function and removing its invocation inside RetrieveRelevant so the retrieval
pipeline no longer contains a stub step; update RetrieveRelevant to pass
observations directly to the next real stage (or return them) and ensure any
imports, tests, or callers that referenced applyReranking are cleaned up or
adapted to the new flow.
🪄 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: 19e4cacf-77b1-46df-b911-8a2c9d358e8b
📒 Files selected for processing (28)
internal/consolidation/associations.gointernal/consolidation/associations_test.gointernal/consolidation/scheduler.gointernal/consolidation/scheduler_test.gointernal/consolidation/similarity.gointernal/consolidation/similarity_test.gointernal/maintenance/entity_extraction.gointernal/maintenance/file_staleness.gointernal/maintenance/hit_rate.gointernal/maintenance/near_dedup.gointernal/maintenance/service.gointernal/maintenance/service_test.gointernal/maintenance/smart_gc.gointernal/maintenance/smart_gc_test.gointernal/maintenance/wiki_generation.gointernal/mcp/server.gointernal/mcp/server_test.gointernal/mcp/tools_admin.gointernal/mcp/tools_learning_test.gointernal/reranking/api.gointernal/reranking/api_test.gointernal/reranking/interface.gointernal/reranking/types.gointernal/worker/handlers_maintenance.gointernal/worker/handlers_patterns.gointernal/worker/handlers_update.gointernal/worker/retrieval.gointernal/worker/service.go
💤 Files with no reviewable changes (23)
- internal/maintenance/hit_rate.go
- internal/worker/handlers_update.go
- internal/consolidation/similarity.go
- internal/worker/handlers_patterns.go
- internal/consolidation/similarity_test.go
- internal/reranking/interface.go
- internal/consolidation/scheduler_test.go
- internal/maintenance/service_test.go
- internal/worker/service.go
- internal/reranking/types.go
- internal/maintenance/file_staleness.go
- internal/consolidation/associations_test.go
- internal/maintenance/smart_gc_test.go
- internal/reranking/api_test.go
- internal/maintenance/wiki_generation.go
- internal/maintenance/near_dedup.go
- internal/maintenance/smart_gc.go
- internal/maintenance/entity_extraction.go
- internal/consolidation/associations.go
- internal/reranking/api.go
- internal/consolidation/scheduler.go
- internal/worker/handlers_maintenance.go
- internal/maintenance/service.go
…c admin action lists, add regression assertions, clean API_CONTRACTS doc CodeRabbit review on PR #185 raised 6 comments; 4 were resolvable threads (all resolved) and 2 were nitpicks (both fixed below). Changes: - internal/worker/retrieval.go: remove no-op applyReranking function and its call site. freshObservations now flow directly to clusterObservations. (per-G1 — the stub was a fake backend left behind after reranker removal) - internal/mcp/tools_admin.go: error message at line 16 was missing compress_aaak, set_aaak_code, taxonomy_stats valid actions. Sync with handler switch. - internal/mcp/server.go: admin tool Description at line ~580 was out of sync with the handler switch — added missing compress_aaak, set_aaak_code, taxonomy_stats actions. - internal/mcp/server_test.go: added removedTools regression loop after the legacyTools assertions, asserting trigger_maintenance, get_maintenance_stats, suggest_consolidations, run_consolidation are absent from BOTH the default listing AND include_all=true listing. - docs/arch/API_CONTRACTS.md: remove stale Memory Consolidation section listing run_consolidation / trigger_maintenance / get_maintenance_stats; remove suggest_consolidations row; adjust dashboard note to not mention 'consolidation status'. Verified: go build ./... clean after each edit.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/mcp/server_test.go (1)
32-32: Снизьте дублирование и хрупкость тестов через helper дляNewServer.На Line 32 (и далее по файлу) повторяется длинный вызов
NewServer(...); это уже привело к массовому механическому редактированию при смене сигнатуры. Лучше вынести создание сервера в helper.♻️ Вариант рефакторинга
+func newTestServer(version string) *Server { + return NewServer(nil, version, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) +} + func (s *ServerSuite) TestNewServer() { - server := NewServer(nil, "1.0.0", nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + server := newTestServer("1.0.0") s.NotNil(server) s.Nil(server.searchMgr) s.Equal("1.0.0", server.version) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/server_test.go` at line 32, Вынеси повторяющийся вызов NewServer(...) в тестах в небольшой helper-функцию (например newTestServer или makeTestServer) внутри internal/mcp/server_test.go: реализуй helper, который принимает только необходимые параметры (или none) и внутри вызывает NewServer(nil, "1.0.0", nil, ..., nil) с набором дефолтов, затем заменяй все прямые вызовы NewServer в файле на этот helper; это уменьшит дублирование и сделает изменения сигнатуры NewServer менее хрупкими при рефакторинге.internal/mcp/server.go (1)
588-595: Сведите списокadminactions к единому источнику истины.На Line 588 список действий обновлён вручную в
Description, ноactionв schema по-прежнему не ограничен enum. Это повышает риск нового рассинхрона между описанием, schema и фактическим dispatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/server.go` around lines 588 - 595, The Description string lists admin actions but the InputSchema's "action" property is not an enum, causing a drift; create a single canonical list (e.g., an exported const slice/array named adminActions or adminActionEnum) and use it to (1) build the Description text, (2) populate the InputSchema["properties"]["action"]["enum"], and (3) drive the dispatch logic so the symbol InputSchema, the "action" property, and the dispatch all reference that one constant (update any code that currently hardcodes the actions, e.g., the Description assignment and the dispatch switch/map that handles admin actions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/mcp/server_test.go`:
- Line 32: Вынеси повторяющийся вызов NewServer(...) в тестах в небольшой
helper-функцию (например newTestServer или makeTestServer) внутри
internal/mcp/server_test.go: реализуй helper, который принимает только
необходимые параметры (или none) и внутри вызывает NewServer(nil, "1.0.0", nil,
..., nil) с набором дефолтов, затем заменяй все прямые вызовы NewServer в файле
на этот helper; это уменьшит дублирование и сделает изменения сигнатуры
NewServer менее хрупкими при рефакторинге.
In `@internal/mcp/server.go`:
- Around line 588-595: The Description string lists admin actions but the
InputSchema's "action" property is not an enum, causing a drift; create a single
canonical list (e.g., an exported const slice/array named adminActions or
adminActionEnum) and use it to (1) build the Description text, (2) populate the
InputSchema["properties"]["action"]["enum"], and (3) drive the dispatch logic so
the symbol InputSchema, the "action" property, and the dispatch all reference
that one constant (update any code that currently hardcodes the actions, e.g.,
the Description assignment and the dispatch switch/map that handles admin
actions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22f4feb7-5200-467e-865b-08b8d31b91a3
📒 Files selected for processing (5)
docs/arch/API_CONTRACTS.mdinternal/mcp/server.gointernal/mcp/server_test.gointernal/mcp/tools_admin.gointernal/worker/retrieval.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/mcp/tools_admin.go
This was referenced Apr 18, 2026
This was referenced Apr 19, 2026
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.
Scope: US7 per plan.md §Phase 7 — delete 4 internal packages and all caller code.
Packages deleted (19 files)
internal/consolidation/(6 files)internal/maintenance/(9 files, includes smart_gc.go)internal/reranking/(4 files)internal/worker/handlers_maintenance.go(dropped with /api/maintenance/* routes)Callers cleaned (7 files edited)
internal/worker/service.go— 3 imports, 3 fields, createReranker, 3 init blocks, 11 routes, 2 shutdown blocks, NewServer call reduced 14→12 argsinternal/worker/retrieval.go— reranker paths removed, applyReranking becomes no-opinternal/worker/handlers_patterns.go— maintSvc orphan-detection block removed from handlePatternCleanupAdvancedinternal/worker/handlers_update.go— reranker health-check block removedinternal/mcp/server.go— 2 imports, 2 fields, 2 params, 3 handler methods, 3 tool registrations + dispatch casesinternal/mcp/tools_admin.go— "consolidations" removed from admin switchinternal/mcp/server_test.go— NewServer calls reduced 14→12 args (50+ sites); 6 test functions removedinternal/mcp/tools_learning_test.go— NewServer call reducedStats
28 files, +86/-5914 (pure delete + caller cleanup)
Verified
go build ./...cleango vet ./...cleango test -short -count=1 ./...all suites greenConventions compliance (plan.md §Cross-Phase Conventions)
Unblocks
US5 (
internal/pattern/) — its consumer inmaintenance/service.gois gone.Summary by CodeRabbit
Удаленные функции
Removed Features
Documentation
Tests