chore(v5): drop orphan join tables + citation-tracking code (PR-v5-1, US1)#179
Merged
Conversation
… US1) Migrations: - 083_drop_session_observation_injections (was 081 in spec; IDs 081/082 taken) - 084_drop_injection_log (was 082 in spec) Both: DROP TABLE IF EXISTS with Rollback re-CREATE to original schema. Go code removal (10 files, -625/+83 = net -542 LOC): - internal/db/gorm/injection_log_store.go: all 7 methods + struct gone - internal/db/gorm/scoring_store.go: sessionObservationInjection + 2 methods gone - internal/worker/handlers_context.go: async LogInjections goroutine gone - internal/worker/handlers_maintenance.go: cleanupInjectionLog + "injection_log" from derivedTables gone - internal/worker/handlers_scoring.go: RecordSessionInjections call + handleGetSessionInjectedObservations + handleMarkCited gone - internal/worker/service.go: /mark-cited and /injected-observations routes gone - internal/worker/retrieval_helpers.go: lookupDiversityScores production path returns nil; test hook preserved - internal/maintenance/hit_rate.go: analyzeHitRate stubbed to (0, nil) — full maintenance drop in US7 - internal/maintenance/service.go: adjustAdaptiveThresholds stubbed to (0, nil) — full maintenance drop in US7 Known stubs (remove in US7 when maintenance subsystem drops): - Service.analyzeHitRate(ctx) → (0, nil) - Service.adjustAdaptiveThresholds(ctx) → (0, nil) Both have explanatory comments; callers in maintenance scheduler are in US7 scope. Cleaner than dangling references for the window between US1 and US7. Verification (run locally in worktree off e9c275d): - rg 'session_observation_injections|injection_log' --type go: only historical migration entries + CHANGELOG references; zero live-code hits - go build ./...: clean - go vet ./...: clean - go test -short ./...: 40+ packages, all ok, 0 failures Invariants preserved (not touched by this PR): - Vault: internal/crypto/*, handlers_vault.go, tools_credential.go, VaultView.vue - content table, credentials observation rows, all 6 static entities - scripts/safety-gate.* (just landed in #178) - spec/plan/tasks artifacts under .agent/specs/engram-v5-baseline/
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughУдалена подсистема учёта инъекций наблюдений: таблицы и модели БД, методы хранилища, HTTP-эндпоинты и связанная логика — всё поведение, читающее/пишущее в Changes
Sequence Diagram(s)(Пропущено — изменения в основном удаляют/отключают подсистему и не вводят новый многокомпонентный поток.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 минут Possibly related PRs
Поэма
🚥 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 performs a cleanup of the injection tracking system by dropping the injection_log and session_observation_injections tables and removing their associated logic across the codebase. Key changes include new database migrations to drop these tables, the removal of the injection_log_store.go file, and converting maintenance tasks like hit-rate analysis and adaptive threshold adjustments into no-ops. A significant issue was identified in the rollback logic for migration 084, which fails to restore the cited column and its corresponding index, potentially causing schema mismatches if the migration is reverted.
![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/maintenance/service.go (1)
629-631: Устаревший комментарий: ссылка на injection_log.Комментарий для Task 20 всё ещё упоминает "Reads citation rates from injection_log", хотя функция теперь возвращает no-op. Рекомендуется обновить комментарий для согласованности с текущей реализацией.
📝 Предлагаемое исправление комментария
- // Task 20: Adaptive threshold adjustment per project (Learning Memory v3 FR-6) - // Reads citation rates from injection_log, adjusts per-project thresholds. - // Bounds: [0.15, 0.60]. Window: last 50 sessions per project. + // Task 20: Adaptive threshold adjustment per project (Learning Memory v3 FR-6) + // NOTE: Currently a no-op — injection_log was dropped in v5 (US1). + // A follow-up task should replace this with an effectiveness_score-based approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/maintenance/service.go` around lines 629 - 631, Update the outdated Task 20 block comment to reflect the current no-op implementation: remove the phrase "Reads citation rates from injection_log" and explicitly state that the adaptive per-project threshold adjustment is currently disabled/no-op while keeping the descriptive title "Adaptive threshold adjustment per project (Learning Memory v3 FR-6)" and any notes about bounds [0.15, 0.60] and window semantics for future reference; locate the comment block labeled "Task 20" in internal/maintenance/service.go and edit that comment only so it accurately documents the function's present behavior.internal/db/gorm/injection_log_store.go (1)
1-3: Рассмотрите полное удаление файла вместо оставления пустого stub.Файл содержит только объявление пакета и комментарий. Хотя комментарий полезен для контекста, git history уже предоставляет эту информацию.
Два варианта:
- Удалить файл полностью — более чистый подход, уменьшает количество файлов в репозитории
- Оставить как есть — комментарий даёт быстрый контекст без необходимости смотреть git blame
Оба варианта приемлемы. Это вопрос предпочтений команды.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/gorm/injection_log_store.go` around lines 1 - 3, The file injection_log_store.go is an empty stub containing only the package declaration and a historical comment; remove this file entirely from the repository to keep the codebase clean (or, if team prefers to retain context, explicitly state that decision in a README—but default to deletion). Locate injection_log_store.go (package symbol: package gorm) and delete it; ensure no imports or references to injection_log_store.go remain elsewhere and run tests/build to confirm nothing breaks.
🤖 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/db/gorm/migrations.go`:
- Around line 2743-2763: The rollback for migration 084 recreates injection_log
without the cited column added in migration 066; update the Rollback func for
migration 084 (the sqls slice inside Rollback) to include the cited BOOLEAN
DEFAULT false column in the CREATE TABLE IF NOT EXISTS injection_log statement
and also add the corresponding index creation for
idx_injection_log_session_cited so the restored table matches the schema
introduced by migration 066.
---
Nitpick comments:
In `@internal/db/gorm/injection_log_store.go`:
- Around line 1-3: The file injection_log_store.go is an empty stub containing
only the package declaration and a historical comment; remove this file entirely
from the repository to keep the codebase clean (or, if team prefers to retain
context, explicitly state that decision in a README—but default to deletion).
Locate injection_log_store.go (package symbol: package gorm) and delete it;
ensure no imports or references to injection_log_store.go remain elsewhere and
run tests/build to confirm nothing breaks.
In `@internal/maintenance/service.go`:
- Around line 629-631: Update the outdated Task 20 block comment to reflect the
current no-op implementation: remove the phrase "Reads citation rates from
injection_log" and explicitly state that the adaptive per-project threshold
adjustment is currently disabled/no-op while keeping the descriptive title
"Adaptive threshold adjustment per project (Learning Memory v3 FR-6)" and any
notes about bounds [0.15, 0.60] and window semantics for future reference;
locate the comment block labeled "Task 20" in internal/maintenance/service.go
and edit that comment only so it accurately documents the function's present
behavior.
🪄 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: 46063481-3319-49f2-90a7-f06464530514
📒 Files selected for processing (10)
internal/db/gorm/injection_log_store.gointernal/db/gorm/migrations.gointernal/db/gorm/scoring_store.gointernal/maintenance/hit_rate.gointernal/maintenance/service.gointernal/worker/handlers_context.gointernal/worker/handlers_maintenance.gointernal/worker/handlers_scoring.gointernal/worker/retrieval_helpers.gointernal/worker/service.go
💤 Files with no reviewable changes (4)
- internal/worker/service.go
- internal/worker/handlers_context.go
- internal/db/gorm/scoring_store.go
- internal/worker/handlers_scoring.go
Rollback was missing schema added by migration 066 (learning memory v3): - cited BOOLEAN DEFAULT false column - idx_injection_log_session_cited index on (session_id, cited) Without these, a post-rollback database would lack schema that existed immediately before DROP, breaking downstream migration replay.
CodeRabbit-flagged: - Delete injection_log_store.go tombstone (was 3-line package stub referencing dropped table; G1 no-stubs compliance) - internal/maintenance/service.go: Task 20 + Task 24 comments updated to reflect no-op state + US7 handoff pointer Additional stale references found during review sweep: - internal/mcp/tools_recall.go: user-facing 'no hit rate analytics data' message replaced (injection_log no longer exists) - internal/worker/handlers_data.go: same constant message replaced - internal/worker/reaper/reaper.go: FK audit comment updated to note v5 US1 drop
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.
PR-v5-1: drop orphan join tables (US1, first destructive PR of v5.0.0)
First destructive PR of the engram v5.0.0 BREAKING cleanup sequence.
SAFEST starting point: 2 orphan tables, no static-entity impact, no vault impact.
Scope
Drop
session_observation_injections+injection_log(both derived, orphan per audit) and all Go code referencing them.What landed
083_drop_session_observation_injections+084_drop_injection_log(IDs in spec said 081/082 but those were already taken)Verification (local, run in worktree off e9c275d)
go build ./...→ cleango vet ./...→ cleango test -short ./...→ 40+ packages, all ok, 0 failuresscripts/safety-gate.ps1against live vault →OK (fingerprint=aa78e55cf896508c, credentials=13, mismatches=0)(unchanged baseline)Known stubs (intentional, tracked for US7)
Two maintenance functions reduced to no-op
return (0, nil):Service.analyzeHitRate(ctx)ininternal/maintenance/hit_rate.goService.adjustAdaptiveThresholds(ctx)ininternal/maintenance/service.goRationale: maintenance subsystem drops in its entirety in US7 (PR-v5-7). Callers (scheduler) still run in the window between US1 and US7. Stubbing is cleaner than dangling references. Both have explanatory comments.
Invariants preserved (NFR-2)
Vault + content table + all static entities UNCHANGED. This PR modifies only derived/orphan storage + related code. No migrations touch any static entity table. Manual safety-gate run post-commit confirms
aa78e55cf896508c / 13 / 0baseline.Known deviation from spec
Migration IDs 081/082 (per spec) were already taken by
081_project_identity_pure_hashand082_projects_lifecycle. Landed as 083/084. Downstream planned IDs (084_drop_observations, 085_drop_user_prompts, etc.) need +2 renumbering — will be handled inline in those PRs' task briefs.Next
After merge: PR-v5-2 (drop
content_chunks+ vector/embedding packages — US2).Spec:
.agent/specs/engram-v5-baseline/spec.mdUS1 · Phase 3 · Commit0a858e0Summary by CodeRabbit
Удалённые функции
Изменения в обработчиках
Поведение и аналитика
Изменения базы данных