feat: Engram v2 — Memory Evolution#3
Merged
Merged
Conversation
Replace path-based project ID (SHA-256(absolutePath)[0:6]) with git-remote-based composite ID (SHA-256(remote_url/relative_path)[0:8]). - hooks/lib.js: getGitRemoteID() + LegacyProjectID() with fallback - sessions/parser.go: GitRemoteProjectID() aligns indexer to same scheme - migrations.go: 030_projects_table with GIN index on legacy_ids - project_store.go: UpsertProject with dual-ID migration, ResolveProjectID - models.go: Project GORM model Zero-downtime: hooks send both project + legacy_project, server re-associates observations in background via idempotent UPDATE. Non-git directories keep path-based fallback.
…ory) Add store_memory and recall_memory MCP tools for agents to explicitly create and retrieve memories, beyond passive hook capture. - tools_memory.go: handleStoreMemory with content governance (hard/soft limits), keyword-based type classification, hierarchical tag expansion, cosine dedup (0.92 threshold), and scope auto-detection - tools_memory.go: handleRecallMemory wrapping hybrid search with text/items/detailed output formats - observation.go: add SourceManual source type constant - config.go: add StoreMemory* configuration fields - server.go: register tool definitions + dispatch cases
Fix BuildWhereFilter vector search bug to include global-scoped observations via OR clause. Wire SearchParams.IncludeGlobal into hybrid search. Surface [GLOBAL] tag in context injection hooks. - types.go: WhereFilter/WhereClause types replacing map[string]any, BuildWhereFilter now accepts includeGlobal bool for OR project/scope - pgvector/client.go: Query accepts WhereFilter, builds OR SQL groups - interface.go: updated Client.Query signature - manager.go: pass IncludeGlobal to BuildWhereFilter in hybridSearch - Updated all 13 BuildWhereFilter callers across 9 files - user-prompt.js + session-start.js: [GLOBAL] prefix on global obs
Update engramInstructions with Memory Management tier-1 section, store_memory/recall_memory in Quick Start, and revised Common Mistakes. Update SKILL.md with top 12 tools (was 10), explicit memory workflow, and store_memory/recall_memory usage examples.
ONNX inference was never used in production — docker-compose defaults to EMBEDDING_PROVIDER=openai and RERANKING_PROVIDER=api. Remove ~49 MB of dead code: ONNX models, runtime libraries, BGE service, cross-encoder reranker, platform-specific lib loaders, and related tests. - Delete internal/embedding/service_onnx.go, lib_*.go, assets/ - Delete internal/reranking/service.go, factory*.go, assets/ - Simplify embedding.Service to always use OpenAI provider - Remove builtin embedding dimension override in store.go - Update config defaults: EmbeddingProvider "builtin" -> "openai" - Clean ONNX references from worker/service.go and config comments - Remove onnxruntime_go and sugarme/tokenizer dependencies
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughУдалены локальные ONNX‑модули embedding и reranking; добавлены инструменты памяти и управления секретами (store_memory, recall_memory, credential tools) с Vault‑шифрованием; введён структурированный WhereFilter для векторных запросов; добавлена таблица projects и git‑remote‑based project ID; обновлены вызовы векторов и миграции. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP as "MCP Server"
participant Vault as "Vault"
participant Vector as "Vector Store"
participant Obs as "Observation Store"
participant Search as "Search Manager"
Client->>MCP: store_memory(args)
MCP->>Vault: ensure vault (init / fingerprint)
MCP->>Vector: dedup query (WhereFilter)
Vector-->>MCP: dedup result
alt duplicate found
MCP->>Client: return existing observation summary
else not duplicate
MCP->>Obs: persist ParsedObservation (may include encrypted secret)
Obs-->>MCP: id
MCP->>Search: index item
MCP->>Client: return stored id/title/type/scope
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Engram's memory system through a multi-phase evolution. It introduces a more robust and cross-platform project identification mechanism, empowers agents with explicit memory management tools, and improves the discoverability of global knowledge. Concurrently, it cleans up substantial legacy ONNX-related code, simplifying the system and focusing on API-based external providers for embedding and reranking services. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a significant evolution of the memory system, encompassing stable project identity, explicit memory management, global cross-project memory, and enhanced tool discoverability. A major cleanup was performed by removing ONNX-based embedding and reranking code, streamlining dependencies and shifting towards API-based providers. The new store_memory and recall_memory tools, alongside the projects table and a more flexible WhereFilter for vector queries, represent robust new functionalities. The changes are well-aligned with the provided summary and test plan.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
internal/config/config.go (1)
246-249: Рассмотрите добавление поддержки переменных окружения для новых полей конфигурации.Для новых полей
StoreMemory*не предусмотрены переопределения через переменные окружения в функцииLoad(). Это может затруднить настройку в контейнерных средах, где env-переменные предпочтительнее файлов конфигурации.💡 Пример добавления env-переопределений
// В функции Load(), после существующих env-переопределений: if v := strings.TrimSpace(os.Getenv("ENGRAM_STORE_MEMORY_HARD_LIMIT")); v != "" { if n, err := strconv.Atoi(v); err == nil && n > 0 { cfg.StoreMemoryHardLimit = n } } if v := strings.TrimSpace(os.Getenv("ENGRAM_STORE_MEMORY_SOFT_LIMIT")); v != "" { if n, err := strconv.Atoi(v); err == nil && n > 0 { cfg.StoreMemorySoftLimit = n } } if v := strings.TrimSpace(os.Getenv("ENGRAM_STORE_MEMORY_DEDUP_THRESHOLD")); v != "" { if f, err := strconv.ParseFloat(v, 64); err == nil && f > 0 && f <= 1.0 { cfg.StoreMemoryDedupThreshold = f } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 246 - 249, Добавьте поддержку переопределения новых полей конфигурации через переменные окружения в функции Load(): для полей StoreMemoryHardLimit и StoreMemorySoftLimit читайте соответствующие ENGRAM_STORE_MEMORY_HARD_LIMIT / ENGRAM_STORE_MEMORY_SOFT_LIMIT через os.Getenv, очищайте строку с strings.TrimSpace, парсьте в int через strconv.Atoi и применяйте только при успешном парсинге и значении >0; для StoreMemoryDedupThreshold читайте ENGRAM_STORE_MEMORY_DEDUP_THRESHOLD и парсьте в float64 через strconv.ParseFloat, проверяя диапазон >0 и <=1.0; не забывайте оставить StoreMemorySummarize доступным через переменную окружения (например ENGRAM_STORE_MEMORY_SUMMARIZE) и парсить её как булево значение. Это позволит контейнерным/CI средам переопределять эти настройки.internal/pipeline/deterministic.go (1)
241-271: Устаревший комментарий с упоминанием BGE.Комментарий на строке 265 (
~400 tokens for BGE) ссылается на модель BGE, которая удалена в этом PR. Учитывая переход на OpenAI-провайдер по умолчанию (text-embedding-3-small), рекомендуется обновить комментарий для отражения актуального контекста.💡 Предлагаемое исправление
- // Rough truncation at ~1500 chars (~400 tokens for BGE) + // Rough truncation at ~1500 chars (~375 tokens)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pipeline/deterministic.go` around lines 241 - 271, Комментарий в функции FormatEmbeddingText упоминает устаревшую модель BGE ("~400 tokens for BGE"); обновите этот комментарий, чтобы он отражал текущий контекст (например, заменить на ссылку на используемую модель text-embedding-3-small или сделать его нейтральным, например: "Rough truncation at ~1500 chars (approx token budget for embedding model)"). Найдите функцию FormatEmbeddingText и отредактируйте строку комментария над трюнкейтом (текущая "Rough truncation at ~1500 chars (~400 tokens for BGE)") на актуальную формулировку или уберите модель-специфику.internal/worker/handlers_context.go (1)
99-99: Рассмотрите возможность оптимизации: фильтрация проекта на уровне вектора.Пустой проект в
BuildWhereFilterозначает, что векторный поиск вернёт результаты из всех проектов, а фильтрация по проекту происходит позже вExtractObservationIDs(строка 147). Это может быть неэффективно при большом количестве данных.Если глобальные наблюдения должны быть включены, рассмотрите:
where := vector.BuildWhereFilter(vector.DocTypeObservation, project, true)Это позволит фильтровать на уровне базы данных, возвращая только
project=X OR scope=global.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_context.go` at line 99, Здесь фильтр векторного поиска строится с пустым project (vector.BuildWhereFilter(vector.DocTypeObservation, "", false)), из‑за чего поиск вернёт все проекты и фильтрация по проекту выполняется позже в ExtractObservationIDs; замените вызов на использование project и флага включения глобальных наблюдений (т.е. вызвать vector.BuildWhereFilter(vector.DocTypeObservation, project, true) или эквивалент), чтобы переносить фильтрацию проекта/глобальных записей в запрос к векторной БД и уменьшить объём возвращаемых результатов; поправьте параметры вызова в том же контексте и убедитесь, что логика ExtractObservationIDs остаётся совместимой с новым предфильтром.
🤖 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/models.go`:
- Around line 347-351: The Project.LegacyIDs field uses models.JSONStringArray
but the DB stores native PostgreSQL TEXT[] (used with array_append() and @>),
causing JSONStringArray.Scan to fail on PostgreSQL array format; replace the
field type with pq.StringArray (from github.com/lib/pq), update the struct tag
to match a PostgreSQL array (e.g. `gorm:"column:legacy_ids;type:text[]"`), add
the pq import, and remove/stop relying on JSONStringArray.Scan—this makes
Project.LegacyIDs, any reads in project_store.go, and usages of
array_append()/@> handle PostgreSQL TEXT[] correctly.
In `@internal/db/gorm/project_store.go`:
- Around line 50-56: The background goroutine launches reassociateObservations
without a context or timeout, which can leak work after parent cancellation;
modify this to derive a context with timeout/cancel (e.g., ctx, cancel :=
context.WithTimeout(parentCtx, timeout)) and run the goroutine with that ctx,
defer cancel, and pass a context-aware DB to reassociateObservations (either add
a ctx parameter to reassociateObservations or call db.WithContext(ctx) inside
the goroutine) so the operation respects cancellation and timeouts while still
logging failures tied to legacyID/newID.
In `@internal/mcp/server.go`:
- Around line 1562-1563: The similarity search is excluding global memories
because the where filter calls
vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, false);
update both places (the call at the shown diff and the other occurrence around
lines 2307-2308) to pass includeGlobal=true so project-scoped searches also
include scope='global'; i.e., change the third argument to true so functions
like find_similar_observations and suggest_consolidations return results that
include global duplicates when querying via s.vectorClient.Query.
In `@internal/mcp/tools_memory.go`:
- Around line 22-30: The params struct currently parses Importance but never
applies it to the saved observation; update the save path to propagate
params.Importance into the stored observation (e.g., set observation.Importance
= params.Importance or use the same scaling logic used elsewhere) before calling
the store/save function so the persisted observation reflects the provided
priority; apply the same fix for the other identical block referenced (lines
~139-149) to ensure both code paths use the Importance field from params.
- Around line 109-123: The dedup logic uses params.Project directly (possibly
empty) and calls vector.BuildWhereFilter with includeGlobal=false, causing
memories to be stored under "" or to miss global matches; fix by normalizing the
project before building the filter and querying: compute project :=
params.Project; if project == "" use the service's current/default project (e.g.
s.defaultProject or s.CurrentProject) and then call
vector.BuildWhereFilter(vector.DocTypeObservation, project, true) so global
observations are included in the dedup scope; apply the same normalized project
when calling StoreObservation and anywhere else you pass params.Project
(references: BuildWhereFilter, Query usage that sets where, and
StoreObservation).
In `@internal/search/manager.go`:
- Line 881: The call to vector.BuildWhereFilter currently mis-handles
params.Scope=="global"; change it to handle three cases explicitly: if
params.Scope == "global" call vector.BuildWhereFilter(docType, "" , true) to
return only global results; if params.Scope == "project" call
vector.BuildWhereFilter(docType, params.Project, false) to return only project
results; otherwise (params.Scope == "") call vector.BuildWhereFilter(docType,
params.Project, params.IncludeGlobal) to preserve existing behavior. Use
params.Scope, params.Project and params.IncludeGlobal to implement this
branching before invoking BuildWhereFilter.
In `@internal/sessions/parser.go`:
- Around line 210-218: The runGitCommand function currently uses exec.Command
without a timeout which can hang; update runGitCommand to create a context with
a timeout (matching the JS implementation, e.g. 3s), use
exec.CommandContext(ctx, "git", fullArgs...) and defer cancel(), then run
cmd.Output() (or CombinedOutput if you want stderr included) so the call is
killed on timeout and you still return the trimmed stdout and the error;
reference the runGitCommand function and replace the exec.Command invocation
with exec.CommandContext using a context.WithTimeout.
- Around line 179-188: ProjectID currently returns only an 8-char hash, causing
mismatch with JS which prefixes the directory name; change ProjectID to build
IDs as dirName + "_" + hash: obtain dirName with filepath.Base(cwdPath), call
GitRemoteProjectID(cwdPath) and if it returns a non-empty id return dirName +
"_" + id, otherwise compute sha256 of cwdPath, take the first 6 hex chars and
return dirName + "_" + hex[:6]; also update GitRemoteProjectID to return the
same hash form expected by JS for git-based repos (i.e., the hash portion only,
not prefixed) so ProjectID composes the final dirName_prefix consistently.
In `@internal/vector/pgvector/client.go`:
- Around line 153-167: The loop that assembles WHERE SQL uses unvalidated
WhereClause.Column directly in fmt.Sprintf, allowing injection via column
identifiers; update the Query/BuildWhereFilter path to validate each
clause.Column against an allowlist (e.g., a set/slice of permitted column names)
before appending to whereClauses, and return an error if a column is not
allowed; ensure both the OrGroup branch and the single-clause branch check the
column name (referencing WhereClause.Column, where.Clauses loop, and
BuildWhereFilter/Query) so only vetted identifiers are used when formatting the
"%s = $%d" pieces while continuing to parameterize values into args.
In `@plugin/engram/skills/memory/SKILL.md`:
- Around line 42-49: The SKILL.md references a non-existent tool name "merge"
which will cause a tool-not-found error; update the documentation to reference
the actual exported tool name merge_observations (or alternatively export an
alias called merge from internal/mcp/server.go), e.g., replace "merge" with
"merge_observations" in SKILL.md or add an exported alias function named merge
that forwards to merge_observations so the agent can find the tool.
---
Nitpick comments:
In `@internal/config/config.go`:
- Around line 246-249: Добавьте поддержку переопределения новых полей
конфигурации через переменные окружения в функции Load(): для полей
StoreMemoryHardLimit и StoreMemorySoftLimit читайте соответствующие
ENGRAM_STORE_MEMORY_HARD_LIMIT / ENGRAM_STORE_MEMORY_SOFT_LIMIT через os.Getenv,
очищайте строку с strings.TrimSpace, парсьте в int через strconv.Atoi и
применяйте только при успешном парсинге и значении >0; для
StoreMemoryDedupThreshold читайте ENGRAM_STORE_MEMORY_DEDUP_THRESHOLD и парсьте
в float64 через strconv.ParseFloat, проверяя диапазон >0 и <=1.0; не забывайте
оставить StoreMemorySummarize доступным через переменную окружения (например
ENGRAM_STORE_MEMORY_SUMMARIZE) и парсить её как булево значение. Это позволит
контейнерным/CI средам переопределять эти настройки.
In `@internal/pipeline/deterministic.go`:
- Around line 241-271: Комментарий в функции FormatEmbeddingText упоминает
устаревшую модель BGE ("~400 tokens for BGE"); обновите этот комментарий, чтобы
он отражал текущий контекст (например, заменить на ссылку на используемую модель
text-embedding-3-small или сделать его нейтральным, например: "Rough truncation
at ~1500 chars (approx token budget for embedding model)"). Найдите функцию
FormatEmbeddingText и отредактируйте строку комментария над трюнкейтом (текущая
"Rough truncation at ~1500 chars (~400 tokens for BGE)") на актуальную
формулировку или уберите модель-специфику.
In `@internal/worker/handlers_context.go`:
- Line 99: Здесь фильтр векторного поиска строится с пустым project
(vector.BuildWhereFilter(vector.DocTypeObservation, "", false)), из‑за чего
поиск вернёт все проекты и фильтрация по проекту выполняется позже в
ExtractObservationIDs; замените вызов на использование project и флага включения
глобальных наблюдений (т.е. вызвать
vector.BuildWhereFilter(vector.DocTypeObservation, project, true) или
эквивалент), чтобы переносить фильтрацию проекта/глобальных записей в запрос к
векторной БД и уменьшить объём возвращаемых результатов; поправьте параметры
вызова в том же контексте и убедитесь, что логика ExtractObservationIDs остаётся
совместимой с новым предфильтром.
🪄 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: ca4261b9-f2a3-4dbb-9315-15c45e739c14
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (52)
cmd/mcp/main.gogo.modinternal/config/config.gointernal/db/gorm/migrations.gointernal/db/gorm/models.gointernal/db/gorm/project_store.gointernal/db/gorm/store.gointernal/embedding/assets.gointernal/embedding/assets/.model_versioninternal/embedding/assets/lib/darwin-arm64/.versioninternal/embedding/assets/lib/linux-arm64/.versioninternal/embedding/assets/lib/windows-amd64/.versioninternal/embedding/assets/model.onnxinternal/embedding/assets/tokenizer.jsoninternal/embedding/lib_darwin.gointernal/embedding/lib_darwin_arm64.gointernal/embedding/lib_linux_amd64.gointernal/embedding/lib_linux_arm64.gointernal/embedding/lib_windows.gointernal/embedding/lib_windows_amd64.gointernal/embedding/model.gointernal/embedding/service.gointernal/embedding/service_onnx.gointernal/embedding/service_test.gointernal/instincts/dedup.gointernal/mcp/server.gointernal/mcp/tools_memory.gointernal/pipeline/deterministic.gointernal/reranking/assets.gointernal/reranking/assets/model.onnxinternal/reranking/assets/tokenizer.jsoninternal/reranking/assets/tokenizer_config.jsoninternal/reranking/factory.gointernal/reranking/factory_onnx.gointernal/reranking/interface.gointernal/reranking/service.gointernal/reranking/service_test.gointernal/search/manager.gointernal/sessions/parser.gointernal/telemetry/similarity.gointernal/vector/interface.gointernal/vector/pgvector/client.gointernal/vector/types.gointernal/worker/handlers_backfill.gointernal/worker/handlers_context.gointernal/worker/handlers_data.gointernal/worker/service.gopkg/models/observation.goplugin/engram/hooks/lib.jsplugin/engram/hooks/session-start.jsplugin/engram/hooks/user-prompt.jsplugin/engram/skills/memory/SKILL.md
💤 Files with no reviewable changes (23)
- internal/embedding/assets/model.onnx
- internal/embedding/assets/lib/windows-amd64/.version
- internal/embedding/lib_darwin_arm64.go
- internal/embedding/lib_linux_arm64.go
- internal/reranking/service_test.go
- internal/embedding/model.go
- internal/embedding/lib_windows.go
- internal/reranking/assets.go
- internal/reranking/factory_onnx.go
- internal/reranking/factory.go
- internal/embedding/assets/lib/linux-arm64/.version
- internal/embedding/lib_darwin.go
- internal/embedding/assets/.model_version
- internal/embedding/service_onnx.go
- internal/db/gorm/store.go
- internal/embedding/service_test.go
- internal/embedding/assets.go
- internal/reranking/assets/tokenizer_config.json
- internal/embedding/lib_linux_amd64.go
- internal/reranking/assets/model.onnx
- internal/reranking/service.go
- internal/embedding/assets/lib/darwin-arm64/.version
- internal/embedding/lib_windows_amd64.go
- Update SKILL.md frontmatter description to mention store_memory/recall_memory explicitly so skill matching triggers on explicit storage intent - Add "Engram vs Claude Code File Memory" comparison table to SKILL.md - Add "Engram vs File-Based Memory" guidance to engramInstructions - Rule of thumb: searchable knowledge → engram, static instructions → file memory
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/engram/skills/memory/SKILL.md (1)
205-205:⚠️ Potential issue | 🟡 MinorНесоответствие: "top 10" vs "Top 12".
Заголовок раздела на строке 53 теперь
Top 12 Tools, но здесь указано "beyond the top 10". Следует обновить для согласованности.-These tools cover specialized use cases beyond the top 10: +These tools cover specialized use cases beyond the top 12:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/engram/skills/memory/SKILL.md` at line 205, The sentence "These tools cover specialized use cases beyond the top 10" is inconsistent with the section header "Top 12 Tools"; update that sentence to reference "top 12" instead of "top 10" so the text matches the heading (locate the sentence in SKILL.md that follows the "Top 12 Tools" heading and change "top 10" → "top 12").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugin/engram/skills/memory/SKILL.md`:
- Line 205: The sentence "These tools cover specialized use cases beyond the top
10" is inconsistent with the section header "Top 12 Tools"; update that sentence
to reference "top 12" instead of "top 10" so the text matches the heading
(locate the sentence in SKILL.md that follows the "Top 12 Tools" heading and
change "top 10" → "top 12").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3fe57f6-8daf-49cc-9171-b8bdd6eff5fa
📒 Files selected for processing (2)
internal/mcp/server.goplugin/engram/skills/memory/SKILL.md
Add encrypted credential storage infrastructure: - internal/crypto/vault.go: AES-256-GCM encrypt/decrypt, key loading (env var → file → auto-generate), SHA-256 fingerprint - internal/crypto/vault_test.go: 10 tests (round-trip, tamper, nonce randomness, key formats, auto-generation, edge cases) - internal/db/gorm/migrations.go: migration 031 adds encrypted_secret (BYTEA) and encryption_key_fingerprint (TEXT) columns, updates CHECK constraint to include 'credential' type - internal/db/gorm/models.go: add credential fields to GORM model - internal/db/gorm/observation_store.go: GetCredential (with project shadowing), ListCredentials, DeleteCredential, CountCredentials - internal/config/config.go: ENGRAM_ENCRYPTION_KEY_FILE and ENGRAM_ENCRYPTION_KEY env var support - pkg/models/observation.go: ObsTypeCredential constant, encrypted fields in ParsedObservation Security: parameterized ORDER BY via gorm.Expr (no SQL injection), COALESCE(is_archived, 0) for NULL-safety, key fingerprint mismatch detection (refuse-to-corrupt, never serve corrupted plaintext).
5 MCP tools for credential management: - store_credential: encrypt and store (AES-256-GCM, lazy vault init) - get_credential: decrypt with fingerprint verification - list_credentials: names + metadata only (no values) - delete_credential: remove by name - vault_status: key status, fingerprint, credential count 4 leak prevention barriers: - internal/search/manager.go: filterCredentials() strips credential type from UnifiedSearch results before caching - plugin/engram/hooks/user-prompt.js: safeObservations filter excludes type='credential' from context injection - internal/mcp/tools_memory.go: privacy.ContainsSecrets() guard rejects plaintext secrets in store_memory (use store_credential instead) - Credentials only accessible via dedicated MCP tools, never via search/recall_memory/find_by_file
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/db/gorm/observation_store.go (1)
153-173:⚠️ Potential issue | 🔴 CriticalCredential записи теперь попадают под обычный retention-cleanup.
Секреты сохраняются через тот же
StoreObservation(), а ниже эта же ветка безусловно ставит проект в очередьCleanupOldObservations()(Lines 1013-1046), который удаляет старые записи без фильтра по типу. Как только проект перевалит за лимит, старые credential-наблюдения начнут тихо исчезать.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/gorm/observation_store.go` around lines 153 - 173, The credential observations are being stored via StoreObservation() but not marked, so CleanupOldObservations() removes them indiscriminately; detect credential observations when building dbObs (obs.EncryptedSecret != "" or obs.Type == CredentialType) and set a new marker field on Observation (e.g., ProtectedFromRetentionCleanup bool) when creating dbObs in observation_store.go, and update CleanupOldObservations() to skip/defer deletion of records with ProtectedFromRetentionCleanup == true; ensure the Observation model, the db insert path in StoreObservation(), and the CleanupOldObservations() deletion query/logic are all updated to use that marker.
♻️ Duplicate comments (5)
internal/db/gorm/models.go (1)
349-353:⚠️ Potential issue | 🔴 Critical
LegacyIDsвсё ещё несовместим сTEXT[].Миграция
030_projects_tableсоздаётlegacy_ids TEXT[], аmodels.JSONStringArrayизpkg/models/observation.go(Lines 112-143) ожидает JSON вида["a","b"], не PostgreSQL array literal вида{a,b}. На чтении/записи через эту модель вы снова упрётесь в ошибку сериализации; здесь нужен тип, который реально умеетTEXT[], напримерpq.StringArray.💡 Предлагаемое исправление
+import "github.com/lib/pq" + type Project struct { GitRemote sql.NullString `gorm:"column:git_remote;index"` RelativePath sql.NullString `gorm:"column:relative_path"` DisplayName sql.NullString `gorm:"column:display_name"` - LegacyIDs models.JSONStringArray `gorm:"column:legacy_ids;type:text"` + LegacyIDs pq.StringArray `gorm:"column:legacy_ids;type:text[]"` ID string `gorm:"primaryKey"` CreatedAt time.Time `gorm:"autoCreateTime"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/gorm/models.go` around lines 349 - 353, The Project model's LegacyIDs field uses models.JSONStringArray (from pkg/models/observation.go) which serializes JSON (["a","b"]) but the DB migration created legacy_ids as a PostgreSQL TEXT[] and requires a Postgres array type; replace LegacyIDs' type with a Postgres-aware array type (e.g., pq.StringArray) and update the GORM tag to use the proper Postgres array column type (e.g., `type:text[]`) so reads/writes use the DB array literal format; ensure imports are adjusted and that the Project struct's LegacyIDs and any code that marshals/unmarshals it are updated to the new type.internal/search/manager.go (1)
900-900:⚠️ Potential issue | 🟠 Major
Scope="global"всё ещё строит неверный where-фильтр.На Line 900 при
params.Scope == "global"сюда по-прежнему попадаетincludeGlobal=false, поэтомуBuildWhereFilter()строит project-only фильтр. Дополнительноinternal/vector/types.go(Lines 103-121) вообще не умеет выразить режим global-only отдельным clause, так что для явного global-поиска здесь всё ещё нужен отдельный путь.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/search/manager.go` at line 900, When params.Scope == "global" the call to vector.BuildWhereFilter(docType, params.Project, params.IncludeGlobal || params.Scope == "") still passes includeGlobal=false and produces a project-only filter; fix by adding an explicit branch for global-only: if params.Scope == "global" call a dedicated global-only builder (or extend BuildWhereFilter to accept a scope argument) so it constructs a clause that matches only global entries, otherwise keep the existing logic for project or mixed searches; update any call sites to use vector.BuildWhereFilter(docType, params.Project, true/false) or a new vector.BuildGlobalWhereFilter to ensure global-only queries are expressible (see symbols: params.Scope, params.IncludeGlobal, vector.BuildWhereFilter and internal/vector/types.go types that represent global vs project clauses).internal/mcp/server.go (1)
1636-1637:⚠️ Potential issue | 🟠 MajorSimilarity-пути всё ещё скрывают global memories.
Оба вызова по-прежнему передают
includeGlobal=false, поэтомуfind_similar_observationsиsuggest_consolidationsбудут пропускать глобальные дубликаты и кандидатов на merge, хотя Phase 3 добавлял для этого OR-фильтр.🛠️ Прямая правка для обоих мест
- where := vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, false) + where := vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, true)Also applies to: 2381-2382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/server.go` around lines 1636 - 1637, Both Query calls build a where filter with includeGlobal=false which hides global memories; update the BuildWhereFilter calls to pass includeGlobal=true so global observations are included. Specifically, change the vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, false) used before s.vectorClient.Query (and the same pattern at the other occurrence) to vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, true) so find_similar_observations / suggest_consolidations will consider global duplicates and merge candidates.internal/mcp/tools_memory.go (2)
23-31:⚠️ Potential issue | 🟠 Major
importanceвсё ещё ни на что не влияет.Поле парсится и рекламируется в schema, но после
StoreObservationникак не применяется. Для клиента это выглядит как успешное сохранение приоритета, хотя у записи остаётся дефолтный score.Also applies to: 145-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_memory.go` around lines 23 - 31, The parsed Importance from the request (params.Importance in the params struct) is never applied after parsing—pass it into the storage flow by setting the observation score or priority before calling StoreObservation: populate the created observation's Score (or Priority) field from params.Importance (or convert to the expected score type) and ensure StoreObservation receives that observation; update any helper/constructor used to build the observation (the code path that creates the observation object prior to calling StoreObservation) so the importance is not ignored.
115-129:⚠️ Potential issue | 🟠 MajorДедуп всё ещё пропускает global и пишет память под пустым project.
BuildWhereFilter(..., params.Project, false)по-прежнему исключаетscope='global', хотя именно это исправлялось в Phase 3. Плюс при пустомprojectзапись всё ещё уходит вStoreObservation(..., "", ...), несмотря на обещание "defaults to current".🛠️ Минимальная безопасная правка
+ project := strings.TrimSpace(params.Project) + if project == "" { + return "", fmt.Errorf("project is required until current-project resolution is implemented") + } + // Dedup check: skip if very similar observation already exists. if s.vectorClient != nil && s.vectorClient.IsConnected() { - where := vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, false) + where := vector.BuildWhereFilter(vector.DocTypeObservation, project, true) similar, err := s.vectorClient.Query(ctx, params.Content, 1, where) if err == nil && len(similar) > 0 && similar[0].Similarity >= dedupThreshold { @@ - id, _, err := s.observationStore.StoreObservation(ctx, "", params.Project, obs, 0, 0) + id, _, err := s.observationStore.StoreObservation(ctx, "", project, obs, 0, 0)Also applies to: 145-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_memory.go` around lines 115 - 129, The dedup logic is still excluding global-scope and allows empty project writes: update the call to vector.BuildWhereFilter in the dedup block to include global scope (i.e., pass the flag that keeps scope='global' rather than false) so global observations are considered when querying via s.vectorClient.Query; additionally ensure when params.Project is empty you substitute the effective current project before calling BuildWhereFilter and before calling StoreObservation (i.e., normalize params.Project to the default/current project value used elsewhere in this component), so queries and StoreObservation(...) never run with an empty project.
🤖 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/config/config.go`:
- Around line 119-120: Add json:"-" struct tags to the sensitive Config fields
so they are excluded from JSON serialization: update the Config struct to
annotate EncryptionKeyFile and EncryptionKey with `json:"-"` (same style used
for DatabaseDSN) to prevent these env-only keys from leaking when calling
json.Marshal on Config.
In `@internal/crypto/vault.go`:
- Around line 54-75: The current os.Stat(keyFile) handling treats any stat error
as "file missing" and proceeds to generate a new key; change the logic in the
vault key-loading block that uses os.Stat and keyFile so that if statErr != nil
you first check if os.IsNotExist(statErr) and only generate a new key in that
case, otherwise return the statErr (wrapped with context) immediately instead of
overwriting the existing key; update the conditional around loadKeyFromFile/key
generation to explicitly handle these two paths and ensure the error returned
includes context about failing to stat the key file.
In `@internal/db/gorm/migrations.go`:
- Around line 1090-1100: The Rollback in the migration currently re-adds the old
constraint chk_observations_type which forbids type = 'credential', so if any
observations have type = 'credential' the ADD CONSTRAINT will fail; update the
Rollback (the anonymous Rollback func that runs tx.Exec(...)) to either 1) clean
or rewrite offending rows before re-adding the constraint (e.g. run an UPDATE
observations SET type = 'discovery' WHERE type = 'credential' OR DELETE FROM
observations WHERE type = 'credential' as appropriate, then re-add the
constraint), or 2) mark the rollback as irreversible by returning a non-nil
error (or removing the ADD CONSTRAINT statements) so the migration framework
won't attempt a dangerous restore; reference the Rollback function, the tx.Exec
calls, and the chk_observations_type constraint when making the change.
In `@internal/db/gorm/observation_store.go`:
- Around line 1249-1254: The current DeleteCredential method can delete both
project-scoped and global credentials (and archived matches) when names collide;
update DeleteCredential to first resolve exactly one Observation using the same
selection logic as GetCredential (matching type "credential", title or narrative
== name, prefer project-scoped over global per GetCredential rules, and exclude
archived/soft-deleted rows if GetCredential does), then perform the DELETE by
the resolved Observation.ID (or return an error if zero or multiple matches) so
only the intended record is removed; refer to the
ObservationStore.DeleteCredential and ObservationStore.GetCredential selection
logic and the Observation.ID field to implement this two-step lookup-then-delete
flow.
In `@internal/mcp/tools_credential.go`:
- Around line 40-58: handleStoreCredential currently sets a default scope
"project" but doesn't resolve the provided project to the canonical/current
project namespace, causing cross-project name collisions; fix by calling the
shared resolveProject() helper to normalize/resolve params.Project before
persisting when params.Scope == "project" (and use that same normalized value in
GetCredential, ListCredentials and DeleteCredential), i.e., update
handleStoreCredential to resolve and replace params.Project via
resolveProject(), and ensure GetCredential, ListCredentials and DeleteCredential
use the same resolveProject() logic for lookups/deletions so project-scoped
credentials are isolated consistently.
- Around line 250-257: handleVaultStatus is making a non-read-only call by
invoking getVault() which can create and persist a vault key; instead, avoid
initializing the vault and do a passive existence check (e.g., check the env var
or the vault key file path with os.Stat or add a helper like crypto.VaultExists)
to determine keyConfigured and fingerprint should remain empty unless a real
vault is already present; update handleVaultStatus to remove getVault() usage,
perform a file/env existence check for the vault key, set keyConfigured based on
that check, and only call v.Fingerprint() when you actually load a pre-existing
vault (or provide a safe crypto function to read only the fingerprint without
creating files).
In `@internal/mcp/tools_memory.go`:
- Around line 183-220: The recall_memory tool is missing a project scope: add a
"project" field to the params struct and the recall_memory InputSchema in
internal/mcp/server.go, then populate search.SearchParams.Project (or resolve
and set the current project string) before calling s.searchMgr.UnifiedSearch;
ensure you reference the recall_memory handler that builds params, the
search.SearchParams struct (add Project), and avoid leaving Project empty so
downstream BuildWhereFilter will apply project filtering.
- Around line 53-60: The code currently uses byte-based len(...) and slicing
which can split UTF-8 runes; change the checks and truncation in the
store-memory logic to operate on runes: use
utf8.RuneCountInString(params.Content) (or convert to []rune) to compare against
hardLimit and softLimit, and when truncating do params.Content =
string([]rune(params.Content)[:softLimit]) so multibyte characters aren’t cut in
half; keep the same error message and log but ensure limits are treated as
rune/character counts.
In `@internal/search/manager.go`:
- Around line 727-731: filterCredentials is being called too late so raw
executeSearch() results can be cached by putInCache (and by warmFrequentQueries)
before sanitation; move credential filtering into the common execution path so
no code path calls putInCache or returns an executeSearch() result without first
calling filterCredentials. Concretely, ensure executeSearch (and any callers
like warmFrequentQueries and the code that computes cacheKey) sanitize the
searchResult via filterCredentials(searchResult) immediately after
receive/create and before any m.putInCache(cacheKey, searchResult) or return, so
all cache writes and returns use the filtered result.
- Around line 741-753: В функции filterCredentials замените текущую логику
проверки типа записи: помимо r.Type == "credential" также учитывайте случаи,
когда r.Type == "observation" и реальный тип хранится в r.Metadata["obs_type"];
то есть пропускайте (не добавляйте в filtered) записи, если r.Type ==
"credential" или (r.Type == "observation" && r.Metadata["obs_type"] ==
"credential"). Обновите присвоение result.Results и result.TotalCount как
сейчас; используйте имена функций/полей filterCredentials, SearchResult.Type и
SearchResult.Metadata["obs_type"] чтобы найти место правки.
In `@plugin/engram/hooks/user-prompt.js`:
- Around line 38-40: Сейчас searchIds собирается прямо из safeObservations,
из‑за чего /mark-injected получает ID для записей, которые потом могут быть
удалены при dedup/token trimming и не попасть в contextToInject; перенесите
формирование searchIds после шагов dedup и token trimming (то есть после
функции/blocks, которые производят dedup и обрезку токенов) и используйте ту же
результирующую коллекцию contextToInject (или её переменную) для заполнения
searchIds, чтобы /mark-injected и метрики получали точный набор ID; обновите код
вокруг safeObservations, searchIds и вызова /mark-injected соответственно.
- Around line 165-167: Вставка полей памяти (title, facts, narrative, возможно
obsType) в XML-подобный блок через contextBuilder происходит без экранирования и
подвержена prompt-injection; перед объединением в строку в том месте, где
формируется contextBuilder (`contextBuilder += \`## ${idx}. [${obsType}]
${title}${scopeTag}${scoreTag}\n\`;`), пропустите title, facts и narrative через
тот же защитный хелпер escapeXmlTags (используемый в session-start.js) — т.е.
заменить сырой title/facts/narrative на escapeXmlTags(title) и т.д., чтобы любые
символы `<`, `>`, `</relevant-memory>` и т.п. были экранированы до вставки.
---
Outside diff comments:
In `@internal/db/gorm/observation_store.go`:
- Around line 153-173: The credential observations are being stored via
StoreObservation() but not marked, so CleanupOldObservations() removes them
indiscriminately; detect credential observations when building dbObs
(obs.EncryptedSecret != "" or obs.Type == CredentialType) and set a new marker
field on Observation (e.g., ProtectedFromRetentionCleanup bool) when creating
dbObs in observation_store.go, and update CleanupOldObservations() to skip/defer
deletion of records with ProtectedFromRetentionCleanup == true; ensure the
Observation model, the db insert path in StoreObservation(), and the
CleanupOldObservations() deletion query/logic are all updated to use that
marker.
---
Duplicate comments:
In `@internal/db/gorm/models.go`:
- Around line 349-353: The Project model's LegacyIDs field uses
models.JSONStringArray (from pkg/models/observation.go) which serializes JSON
(["a","b"]) but the DB migration created legacy_ids as a PostgreSQL TEXT[] and
requires a Postgres array type; replace LegacyIDs' type with a Postgres-aware
array type (e.g., pq.StringArray) and update the GORM tag to use the proper
Postgres array column type (e.g., `type:text[]`) so reads/writes use the DB
array literal format; ensure imports are adjusted and that the Project struct's
LegacyIDs and any code that marshals/unmarshals it are updated to the new type.
In `@internal/mcp/server.go`:
- Around line 1636-1637: Both Query calls build a where filter with
includeGlobal=false which hides global memories; update the BuildWhereFilter
calls to pass includeGlobal=true so global observations are included.
Specifically, change the vector.BuildWhereFilter(vector.DocTypeObservation,
params.Project, false) used before s.vectorClient.Query (and the same pattern at
the other occurrence) to vector.BuildWhereFilter(vector.DocTypeObservation,
params.Project, true) so find_similar_observations / suggest_consolidations will
consider global duplicates and merge candidates.
In `@internal/mcp/tools_memory.go`:
- Around line 23-31: The parsed Importance from the request (params.Importance
in the params struct) is never applied after parsing—pass it into the storage
flow by setting the observation score or priority before calling
StoreObservation: populate the created observation's Score (or Priority) field
from params.Importance (or convert to the expected score type) and ensure
StoreObservation receives that observation; update any helper/constructor used
to build the observation (the code path that creates the observation object
prior to calling StoreObservation) so the importance is not ignored.
- Around line 115-129: The dedup logic is still excluding global-scope and
allows empty project writes: update the call to vector.BuildWhereFilter in the
dedup block to include global scope (i.e., pass the flag that keeps
scope='global' rather than false) so global observations are considered when
querying via s.vectorClient.Query; additionally ensure when params.Project is
empty you substitute the effective current project before calling
BuildWhereFilter and before calling StoreObservation (i.e., normalize
params.Project to the default/current project value used elsewhere in this
component), so queries and StoreObservation(...) never run with an empty
project.
In `@internal/search/manager.go`:
- Line 900: When params.Scope == "global" the call to
vector.BuildWhereFilter(docType, params.Project, params.IncludeGlobal ||
params.Scope == "") still passes includeGlobal=false and produces a project-only
filter; fix by adding an explicit branch for global-only: if params.Scope ==
"global" call a dedicated global-only builder (or extend BuildWhereFilter to
accept a scope argument) so it constructs a clause that matches only global
entries, otherwise keep the existing logic for project or mixed searches; update
any call sites to use vector.BuildWhereFilter(docType, params.Project,
true/false) or a new vector.BuildGlobalWhereFilter to ensure global-only queries
are expressible (see symbols: params.Scope, params.IncludeGlobal,
vector.BuildWhereFilter and internal/vector/types.go types that represent global
vs project clauses).
🪄 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: 168fbf2c-eb8c-4c10-bdab-b7b9c2e4a18b
📒 Files selected for processing (12)
internal/config/config.gointernal/crypto/vault.gointernal/crypto/vault_test.gointernal/db/gorm/migrations.gointernal/db/gorm/models.gointernal/db/gorm/observation_store.gointernal/mcp/server.gointernal/mcp/tools_credential.gointernal/mcp/tools_memory.gointernal/search/manager.gopkg/models/observation.goplugin/engram/hooks/user-prompt.js
7 fixes from multi-model code review (3 HIGH, 2 MEDIUM, 2 LOW): - C1: Validate scope parameter server-side (reject invalid values) - C3: filterCredentials returns new struct (fix singleflight data race) - C4: Scope-aware DeleteCredential (prevent cross-project global deletion) - S1: Use synthetic "credential:vault" session ID (fix unique constraint) - S4: Trim raw-bytes fallback in loadKeyFromFile (handle trailing newline) - S6: Use subtle.ConstantTimeCompare for fingerprint matching - C2: Document sync.Once permanent-failure semantics 5 items tracked in TECHNICAL_DEBT.md for follow-up.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
internal/mcp/tools_credential.go (2)
272-280:⚠️ Potential issue | 🟠 Major
handleVaultStatusимеет побочный эффект — создание ключа.Вызов
getVault()в status endpoint может создать новыйvault.keyфайл черезcrypto.NewVault()(см.internal/crypto/vault.go:56-77). Для read-only status endpoint это неожиданное поведение.Рекомендуется добавить passive check для существования ключа без его создания.
🔍 Предлагаемое решение
Добавить в
cryptoпакет функцию пассивной проверки:// VaultKeyExists checks if a vault key is configured without creating one. func VaultKeyExists(cfg *config.Config) bool { if cfg.EncryptionKey != "" { return true } if cfg.EncryptionKeyFile != "" { if _, err := os.Stat(cfg.EncryptionKeyFile); err == nil { return true } } keyFile := filepath.Join(config.DataDir(), "vault.key") _, err := os.Stat(keyFile) return err == nil }Затем использовать в
handleVaultStatus:func (s *Server) handleVaultStatus(ctx context.Context, _ json.RawMessage) (string, error) { - v, vErr := getVault() - - keyConfigured := vErr == nil && v != nil - fingerprint := "" - if keyConfigured { - fingerprint = v.Fingerprint() - } + cfg := config.Get() + keyConfigured := crypto.VaultKeyExists(cfg) + fingerprint := "" + if keyConfigured { + if v, err := getVault(); err == nil { + fingerprint = v.Fingerprint() + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_credential.go` around lines 272 - 280, handleVaultStatus currently calls getVault() which can create a new vault key as a side effect; replace that with a passive existence check to avoid creating files during a read-only status request. Add a new crypto.VaultKeyExists(cfg *config.Config) bool (or similar) that checks cfg.EncryptionKey, cfg.EncryptionKeyFile, and the presence of the data-dir vault.key without creating anything, then change handleVaultStatus to call VaultKeyExists instead of getVault() and only call v.Fingerprint() when an existing vault instance is explicitly loaded elsewhere; ensure you reference the getVault and handleVaultStatus symbols when making the change.
247-255:⚠️ Potential issue | 🟠 MajorТа же проблема валидации project при scope="project".
Аналогично
handleStoreCredential, еслиscope="project"иparams.Projectпустой, удаление может не найти ожидаемый credential или удалить не тот.Добавьте такую же валидацию project.
🔒 Предлагаемое исправление
if params.Scope == "" { params.Scope = "project" } switch params.Scope { case "project", "global": // valid default: return "", fmt.Errorf("invalid scope %q: must be \"project\" or \"global\"", params.Scope) } + if params.Scope == "project" && params.Project == "" { + return "", fmt.Errorf("project is required for project-scoped credentials") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_credential.go` around lines 247 - 255, When scope is "project" the code must validate that params.Project is non-empty (same as handleStoreCredential); add a check (e.g. if params.Scope == "project" && params.Project == "") that returns an error like fmt.Errorf("missing project for scope \"project\"") before proceeding to delete so the deletion cannot run with an empty project and accidentally target the wrong credential; mirror the validation logic used in handleStoreCredential and reference params.Scope and params.Project in your change.internal/db/gorm/observation_store.go (1)
1248-1274:⚠️ Potential issue | 🟡 MinorУлучшение по сравнению с предыдущей версией, но остаётся edge case.
Добавление параметра
scopeрешает проблему из предыдущего ревью — теперь project и global credentials удаляются раздельно. Однако если существует несколько credentials с одинаковым именем в одном scope (например, две project-scoped credentials с name="api-key"), будут удалены все совпадения.Для большей безопасности можно использовать двухэтапный подход: сначала найти одну запись, затем удалить по ID.
🛡️ Предлагаемое уточнение
func (s *ObservationStore) DeleteCredential(ctx context.Context, name, project, scope string) error { if scope == "" { scope = "project" } + + // Find exactly one credential to delete + var obs Observation query := s.db.WithContext(ctx). Where("type = ?", "credential"). Where("(title = ? OR narrative = ?)", name, name). Where("COALESCE(is_archived, 0) = 0") if scope == "global" { query = query.Where("scope = 'global'") } else { query = query.Where("project = ? AND scope = 'project'", project) } - result := query.Delete(&Observation{}) + + if err := query.First(&obs).Error; err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return fmt.Errorf("credential %q not found", name) + } + return fmt.Errorf("find credential: %w", err) + } + + result := s.db.WithContext(ctx).Delete(&Observation{}, obs.ID) if result.Error != nil { return fmt.Errorf("delete credential: %w", result.Error) } - if result.RowsAffected == 0 { - return fmt.Errorf("credential %q not found", name) - } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/gorm/observation_store.go` around lines 1248 - 1274, DeleteCredential currently deletes all matching observations by name+scope; instead first query for a single Observation (respecting the same scope logic) using s.db.WithContext(ctx).Where(...).First(&obs) and if found call s.db.WithContext(ctx).Delete(&Observation{}, obs.ID) so only that specific record is removed; keep the existing scope defaulting logic and error handling (return not-found if First returns gorm.ErrRecordNotFound, wrap other DB errors similarly) and reference the DeleteCredential method, ObservationStore receiver, and Observation model/ID when making the change.internal/search/manager.go (2)
749-753:⚠️ Potential issue | 🔴 CriticalНеверное поле для фильтрации — credential-наблюдения не будут отфильтрованы.
observationToResult()устанавливаетType = "observation"для всех наблюдений, а реальный тип (ObsTypeCredential) хранится вMetadata["obs_type"]. Текущая проверкаr.Type != "credential"пропустит все credential-записи, так как ихTypeравен"observation".🐛 Предлагаемое исправление
func filterCredentials(result *UnifiedSearchResult) *UnifiedSearchResult { if result == nil { return nil } filtered := make([]SearchResult, 0, len(result.Results)) for _, r := range result.Results { - if r.Type != "credential" { - filtered = append(filtered, r) + // Skip credential observations (stored with Type="observation", obs_type="credential") + if r.Type == "observation" { + if obsType, _ := r.Metadata["obs_type"].(string); obsType == "credential" { + continue + } } + filtered = append(filtered, r) } out := *result out.Results = filtered out.TotalCount = len(filtered) return &out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/search/manager.go` around lines 749 - 753, В цикле, где фильтруются результаты (перебор result.Results), проверка должна опираться не на r.Type, а на реальный тип наблюдения, который observationToResult() кладёт в Metadata["obs_type"]; замените условие r.Type != "credential" на проверку значения в r.Metadata — например пропускайте/фильтруйте записи, у которых r.Metadata["obs_type"] == ObsTypeCredential — при этом защититесь от отсутствующего ключа или нестрокового значения (проверить наличие и привести к string) перед сравнением; используйте существующие символы observationToResult(), ObsTypeCredential и result.Results/filtered для локализации правки.
905-905:⚠️ Potential issue | 🔴 CriticalПараметр
Scope="global"возвращает результаты проекта вместо только глобальных результатов.Текущая логика
params.IncludeGlobal || params.Scope == ""приScope="global"вычисляется какfalse || false = false, что приводит к фильтрации только по проекту, игнорируя глобальную область видимости.Требуется обработка трёх случаев:
Scope="global": только глобальные результаты (scope='global')Scope="project": только результаты проектаScope="": оба типа результатов (проект и глобальные)Предлагаемое исправление
- where := vector.BuildWhereFilter(docType, params.Project, params.IncludeGlobal || params.Scope == "") + var where vector.WhereFilter + switch params.Scope { + case "global": + // Only global results + where = vector.BuildWhereFilter(docType, "", false) + where.Clauses = append(where.Clauses, vector.WhereClause{Column: "scope", Value: "global"}) + case "project": + // Only project results + where = vector.BuildWhereFilter(docType, params.Project, false) + default: + // Both project and global results + where = vector.BuildWhereFilter(docType, params.Project, true) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/search/manager.go` at line 905, The current single boolean expression mishandles params.Scope; replace the inline call to vector.BuildWhereFilter with explicit handling of the three scope cases: if params.Scope == "global" call vector.BuildWhereFilter(docType, "", true) to return only global results; else if params.Scope == "project" call vector.BuildWhereFilter(docType, params.Project, false) to return only project results; else (params.Scope == "") call vector.BuildWhereFilter(docType, params.Project, true) to return both project and global results. Use the params.Scope, params.Project, and vector.BuildWhereFilter symbols to locate and update the code.
🧹 Nitpick comments (1)
internal/mcp/tools_credential.go (1)
15-36: Документация sync.Once семантики — хорошо, но тесты могут быть проблемой.Комментарий корректно описывает permanent-failure семантику
sync.Once. Однако package-level состояние усложняет изоляцию тестов.Для улучшения тестируемости можно рассмотреть передачу vault как зависимости в
Serverили добавить функцию сброса для тестов.♻️ Опциональное улучшение для тестируемости
+// resetVaultForTesting resets the vault singleton. For tests only. +func resetVaultForTesting() { + vaultOnce = sync.Once{} + sharedVault = nil + vaultInitErr = nil +}Или переместить vault в поле
Serverи инициализировать при создании сервера.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_credential.go` around lines 15 - 36, The package-level vault (sharedVault, vaultInitErr, vaultOnce) makes tests brittle; add a test-only reset function (e.g., ResetVaultForTests) in the same package that reinitializes vaultOnce (assign sync.Once{}), and clears sharedVault and vaultInitErr so getVault can be rerun in tests, and mark it with a test-only build tag (or a //go:build test) so it is not included in production; alternatively, refactor to inject the vault into Server (add a Vault field on Server and initialize it during Server creation instead of using getVault) and update call-sites that reference getVault to use server.Vault.
🤖 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_credential.go`:
- Around line 60-68: When params.Scope is "project" we must enforce that
params.Project is non-empty; update the validation around the existing switch on
params.Scope to return an error (e.g., fmt.Errorf("project must be provided when
scope==\"project\"")) if params.Scope == "project" && params.Project == "";
apply the same check at the other similar validation site referenced (the other
scope check) so credentials cannot be stored without a project association.
In `@internal/search/manager.go`:
- Around line 727-738: warmFrequentQueries currently caches raw results from
executeSearch (via m.putInCache) and can leak credentials; update
warmFrequentQueries to mirror UnifiedSearch by filtering the executeSearch
return before caching: call result, err := m.executeSearch(...), then create
filtered := filterCredentials(result) (ensuring you return a new struct to avoid
mutating shared singleflight results) and call m.putInCache(cacheKey, filtered)
instead of caching the unfiltered result.
---
Duplicate comments:
In `@internal/db/gorm/observation_store.go`:
- Around line 1248-1274: DeleteCredential currently deletes all matching
observations by name+scope; instead first query for a single Observation
(respecting the same scope logic) using
s.db.WithContext(ctx).Where(...).First(&obs) and if found call
s.db.WithContext(ctx).Delete(&Observation{}, obs.ID) so only that specific
record is removed; keep the existing scope defaulting logic and error handling
(return not-found if First returns gorm.ErrRecordNotFound, wrap other DB errors
similarly) and reference the DeleteCredential method, ObservationStore receiver,
and Observation model/ID when making the change.
In `@internal/mcp/tools_credential.go`:
- Around line 272-280: handleVaultStatus currently calls getVault() which can
create a new vault key as a side effect; replace that with a passive existence
check to avoid creating files during a read-only status request. Add a new
crypto.VaultKeyExists(cfg *config.Config) bool (or similar) that checks
cfg.EncryptionKey, cfg.EncryptionKeyFile, and the presence of the data-dir
vault.key without creating anything, then change handleVaultStatus to call
VaultKeyExists instead of getVault() and only call v.Fingerprint() when an
existing vault instance is explicitly loaded elsewhere; ensure you reference the
getVault and handleVaultStatus symbols when making the change.
- Around line 247-255: When scope is "project" the code must validate that
params.Project is non-empty (same as handleStoreCredential); add a check (e.g.
if params.Scope == "project" && params.Project == "") that returns an error like
fmt.Errorf("missing project for scope \"project\"") before proceeding to delete
so the deletion cannot run with an empty project and accidentally target the
wrong credential; mirror the validation logic used in handleStoreCredential and
reference params.Scope and params.Project in your change.
In `@internal/search/manager.go`:
- Around line 749-753: В цикле, где фильтруются результаты (перебор
result.Results), проверка должна опираться не на r.Type, а на реальный тип
наблюдения, который observationToResult() кладёт в Metadata["obs_type"];
замените условие r.Type != "credential" на проверку значения в r.Metadata —
например пропускайте/фильтруйте записи, у которых r.Metadata["obs_type"] ==
ObsTypeCredential — при этом защититесь от отсутствующего ключа или нестрокового
значения (проверить наличие и привести к string) перед сравнением; используйте
существующие символы observationToResult(), ObsTypeCredential и
result.Results/filtered для локализации правки.
- Line 905: The current single boolean expression mishandles params.Scope;
replace the inline call to vector.BuildWhereFilter with explicit handling of the
three scope cases: if params.Scope == "global" call
vector.BuildWhereFilter(docType, "", true) to return only global results; else
if params.Scope == "project" call vector.BuildWhereFilter(docType,
params.Project, false) to return only project results; else (params.Scope == "")
call vector.BuildWhereFilter(docType, params.Project, true) to return both
project and global results. Use the params.Scope, params.Project, and
vector.BuildWhereFilter symbols to locate and update the code.
---
Nitpick comments:
In `@internal/mcp/tools_credential.go`:
- Around line 15-36: The package-level vault (sharedVault, vaultInitErr,
vaultOnce) makes tests brittle; add a test-only reset function (e.g.,
ResetVaultForTests) in the same package that reinitializes vaultOnce (assign
sync.Once{}), and clears sharedVault and vaultInitErr so getVault can be rerun
in tests, and mark it with a test-only build tag (or a //go:build test) so it is
not included in production; alternatively, refactor to inject the vault into
Server (add a Vault field on Server and initialize it during Server creation
instead of using getVault) and update call-sites that reference getVault to use
server.Vault.
🪄 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: edfa3355-7960-442f-b583-2561fae39074
📒 Files selected for processing (5)
TECHNICAL_DEBT.mdinternal/crypto/vault.gointernal/db/gorm/observation_store.gointernal/mcp/tools_credential.gointernal/search/manager.go
✅ Files skipped from review due to trivial changes (1)
- TECHNICAL_DEBT.md
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/crypto/vault.go
- S2: add key_source field to vault_status (env/file/auto_generated) - S3: simplify GetCredential/DeleteCredential WHERE to title-only - S7: log migration 031 rollback errors instead of swallowing - S8: deduplicate expandTagHierarchy — tools_memory.go calls shared func - Add credential management section to engramInstructions - Add credential tools documentation to SKILL.md - Mark resolved items in TECHNICAL_DEBT.md
There was a problem hiding this comment.
♻️ Duplicate comments (8)
internal/crypto/vault.go (1)
58-82:⚠️ Potential issue | 🔴 CriticalОшибка
os.Stat, отличная от отсутствия файла, приводит к генерации нового ключа.Блок
elseна строке 67 обрабатывает ВСЕ ошибкиos.Statкак "файл отсутствует". Еслиvault.keyсуществует, ноStatпадает из-за недостатка прав или I/O ошибки, код сгенерирует новый ключ и перезапишет существующий, делая все сохранённые credentials недешифруемыми.Необходимо явно проверять
os.IsNotExist(statErr)перед генерацией нового ключа.
,🐛 Предлагаемое исправление
default: keyFile := filepath.Join(config.DataDir(), "vault.key") - if _, statErr := os.Stat(keyFile); statErr == nil { + _, statErr := os.Stat(keyFile) + switch { + case statErr == nil: key, err = loadKeyFromFile(keyFile) if err != nil { return nil, fmt.Errorf("load auto-generated key from %q: %w", keyFile, err) } log.Info().Str("file", keyFile).Msg("vault: loaded auto-generated key") return &Vault{key: key, fingerprint: computeFingerprint(key), source: "auto_generated"}, nil - } else { + case os.IsNotExist(statErr): key = make([]byte, 32) if _, err = io.ReadFull(rand.Reader, key); err != nil { return nil, fmt.Errorf("generate encryption key: %w", err) } if err = os.MkdirAll(filepath.Dir(keyFile), 0700); err != nil { return nil, fmt.Errorf("create key directory %q: %w", filepath.Dir(keyFile), err) } if err = saveKeyToFile(keyFile, key); err != nil { return nil, fmt.Errorf("save generated key to %q: %w", keyFile, err) } log.Warn(). Str("file", keyFile). Msg("vault: auto-generated new encryption key — BACK UP THIS FILE to avoid losing access to stored credentials") return &Vault{key: key, fingerprint: computeFingerprint(key), source: "auto_generated"}, nil + default: + return nil, fmt.Errorf("stat vault key %q: %w", keyFile, statErr) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/crypto/vault.go` around lines 58 - 82, The os.Stat error handling in the default branch incorrectly treats any statErr as "file not found" and proceeds to generate and overwrite a key; update the logic around the os.Stat(keyFile) result so you only generate a new key when os.IsNotExist(statErr) is true, and for any other non-nil statErr return an error (propagate a wrapped error) instead of creating a new key; locate the check around keyFile/os.Stat, and adjust the control flow that currently calls loadKeyFromFile, make([]byte, 32), saveKeyToFile, and returns a Vault to handle the non-existence case separately and error out on other statErr values.internal/mcp/tools_memory.go (4)
23-31:⚠️ Potential issue | 🟠 MajorПараметр
importanceпо-прежнему декоративный.Поле парсится и рекламируется в schema, но на сохранённую observation не влияет:
StoreObservationвызывается как раньше, а значение приоритета нигде не применяется.💡 Минимальная правка
var params struct { Tags []string `json:"tags"` Content string `json:"content"` Title string `json:"title"` Type string `json:"type"` Scope string `json:"scope"` Project string `json:"project"` - Importance float64 `json:"importance"` + Importance *float64 `json:"importance"` } @@ id, _, err := s.observationStore.StoreObservation(ctx, "", params.Project, obs, 0, 0) if err != nil { return "", fmt.Errorf("store observation: %w", err) } + if params.Importance != nil { + if *params.Importance < 0 || *params.Importance > 1 { + return "", fmt.Errorf("importance must be between 0 and 1") + } + if err := s.observationStore.UpdateImportanceScore(ctx, id, *params.Importance); err != nil { + return "", fmt.Errorf("set importance: %w", err) + } + }Also applies to: 143-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_memory.go` around lines 23 - 31, The incoming params struct includes an Importance field but it's never applied when saving; update the code path that calls StoreObservation so the parsed params.Importance is propagated to the saved observation (e.g., set observation.Priority = params.Importance or pass it as an argument into StoreObservation) — locate where params is used (the params struct definition and the call site to StoreObservation) and ensure the importance value is applied to the observation before persisting; also mirror this change for the other occurrence noted around lines 143-145 where StoreObservation is invoked.
53-57:⚠️ Potential issue | 🟠 MajorЛимиты и усечение здесь считаются в байтах, а не в символах.
len(string)иs[:n]в Go работают по байтам, поэтому многобайтный UTF-8 текст может обрезаться раньше обещанного лимита и даже посередине руны. Переведите эти проверки и усечения наutf8.RuneCountInString/[]runeдляcontent,truncateTitleи текстового preview.#!/bin/bash rg -n 'len\(params\.Content\)|params\.Content = params\.Content\[:softLimit\]|if len\(content\) <= maxLen|truncated := content\[:maxLen\]|if len\(content\) > 300|content = content\[:300\]' internal/mcp/tools_memory.goAlso applies to: 165-169, 281-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_memory.go` around lines 53 - 57, The byte-based length checks and slicing for params.Content (using len(params.Content) and params.Content[:softLimit]) must be converted to rune-aware operations: replace byte-counting with utf8.RuneCountInString for comparisons and convert strings to []rune for truncation, e.g., in the blocks around the hardLimit/softLimit checks and the other occurrences you flagged (including truncateTitle and any preview/truncated variables at the later sites), so you compare rune counts and slice rune slices to the desired max runes rather than slicing bytes.
181-210:⚠️ Potential issue | 🟠 Major
recall_memoryпо-прежнему не умеет искать в project-скоупе.В handler нет поля
project, аsearch.SearchParamsсобирается безProject, поэтому поиск уходит с пустым project и снимает ожидаемое ограничениеcurrent + global. Добавьтеprojectв параметры handler и синхронизируйтеInputSchemaвinternal/mcp/server.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_memory.go` around lines 181 - 210, Handler for recall_memory lacks a project scope so searches run with empty project and include global results; update the handler and schemas to accept and forward project: add a Project string `json:"project"` field to the local params struct used in the recall_memory handler, populate search.SearchParams.Project = params.Project when building searchParams (in the same function that constructs search.SearchParams), and update the handler's InputSchema in internal/mcp/server.go to include the project field so the API/validation and server-side handler stay in sync.
29-30:⚠️ Potential issue | 🟠 Major
store_memoryвсё ещё сохраняет в""и дедупит по неправильному скоупу.
projectиз контракта здесь не нормализуется: пустая строка уходит и вStoreObservation, и вBuildWhereFilter. В итоге память может записываться под"", dedup при пустом project теряет project-фильтр целиком, а при непустомincludeGlobal=falseвсё равно пропускает global memories, которые потом будут видны через recall/search.Also applies to: 113-116, 143-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_memory.go` around lines 29 - 30, The bug is that the incoming contract's project string is not normalized before being used, so code paths in store_memory, StoreObservation, and BuildWhereFilter receive an empty "" project which causes incorrect storage under "" and wrong dedup/filter scopes (and includeGlobal logic bypasses project checks). Fix by normalizing the contract project once at the start of the store_memory flow (e.g., compute a normalizedProject or nil-equivalent), then pass that normalized value into StoreObservation, BuildWhereFilter, and any dedup/includeGlobal checks (instead of the raw contract.Project) so dedup and recall/search honor the correct project scope. Ensure all usages referenced (store_memory, StoreObservation, BuildWhereFilter and includeGlobal handling) are changed to use the normalized value.plugin/engram/skills/memory/SKILL.md (1)
48-48:⚠️ Potential issue | 🟡 MinorЗдесь всё ещё указан несуществующий tool
merge.В MCP экспортируется
merge_observations, поэтому эта инструкция снова приведёт кtool not found. Заменитеmergeнаmerge_observations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/engram/skills/memory/SKILL.md` at line 48, The documentation references a non-existent tool name "merge" (seen in the line "Found duplicate memories? → suggest_consolidations / merge"); update that reference to the exported tool name "merge_observations" so the instruction matches the actual MCP export; search for and replace any occurrences of the symbol "merge" in SKILL.md (or this specific line) with "merge_observations" to prevent "tool not found" errors.internal/mcp/server.go (1)
1646-1647:⚠️ Potential issue | 🟠 MajorSimilarity-поиск всё ещё скрывает global memories.
Оба вызова
BuildWhereFilter(..., params.Project, false)противоречат Phase 3:find_similar_observationsиsuggest_consolidationsпо-прежнему не видятscope='global', из-за чего будут пропускаться глобальные дубликаты.💡 Минимальная правка
- where := vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, false) + where := vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, true) ... - where := vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, false) + where := vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, true)Also applies to: 2391-2392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/server.go` around lines 1646 - 1647, The similarity search is excluding global memories because BuildWhereFilter is called with the local-only flag set to false; update the calls to vector.BuildWhereFilter(vector.DocTypeObservation, params.Project, true) so the filter includes scope='global' (fix the occurrences used by find_similar_observations and suggest_consolidations that call s.vectorClient.Query with the resulting where) to ensure global observations are visible to the similarity queries.internal/db/gorm/migrations.go (1)
1090-1102:⚠️ Potential issue | 🟠 MajorОткат
031_credential_storageвсё ещё может “успешно” завершиться в полуоткаченном состоянии.Если в
observationsуже есть строки сtype = 'credential', восстановление старогоchk_observations_typeснова упадёт. Сейчас ошибка только логируется, а rollback возвращаетnil, поэтому мигратор посчитает откат успешным при несовместимой схеме. Либо очистите/перепишите такие строки перед восстановлением constraint, либо сделайте rollback явно необратимым и возвращайте ошибку.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/gorm/migrations.go` around lines 1090 - 1102, The Rollback function in migration 031_credential_storage currently logs errors but returns nil, allowing a "half-rolled-back" state if rows with type = 'credential' exist and the ALTER ... ADD CONSTRAINT chk_observations_type fails; fix it by making the rollback deterministic: either (A) clean or rewrite incompatible rows before re-adding the constraint (e.g., run an UPDATE/DELETE on observations where type='credential' prior to the ALTER in the Rollback function identified by Rollback and sqls), or (B) make the rollback fail fast by returning the error when tx.Exec(s).Error is non-nil so the migrator treats the rollback as failed (replace the log-only handling in Rollback with an immediate return err). Ensure you modify the Rollback logic that constructs sqls and the loop over tx.Exec to implement one of these approaches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/crypto/vault.go`:
- Around line 58-82: The os.Stat error handling in the default branch
incorrectly treats any statErr as "file not found" and proceeds to generate and
overwrite a key; update the logic around the os.Stat(keyFile) result so you only
generate a new key when os.IsNotExist(statErr) is true, and for any other
non-nil statErr return an error (propagate a wrapped error) instead of creating
a new key; locate the check around keyFile/os.Stat, and adjust the control flow
that currently calls loadKeyFromFile, make([]byte, 32), saveKeyToFile, and
returns a Vault to handle the non-existence case separately and error out on
other statErr values.
In `@internal/db/gorm/migrations.go`:
- Around line 1090-1102: The Rollback function in migration
031_credential_storage currently logs errors but returns nil, allowing a
"half-rolled-back" state if rows with type = 'credential' exist and the ALTER
... ADD CONSTRAINT chk_observations_type fails; fix it by making the rollback
deterministic: either (A) clean or rewrite incompatible rows before re-adding
the constraint (e.g., run an UPDATE/DELETE on observations where
type='credential' prior to the ALTER in the Rollback function identified by
Rollback and sqls), or (B) make the rollback fail fast by returning the error
when tx.Exec(s).Error is non-nil so the migrator treats the rollback as failed
(replace the log-only handling in Rollback with an immediate return err). Ensure
you modify the Rollback logic that constructs sqls and the loop over tx.Exec to
implement one of these approaches.
In `@internal/mcp/server.go`:
- Around line 1646-1647: The similarity search is excluding global memories
because BuildWhereFilter is called with the local-only flag set to false; update
the calls to vector.BuildWhereFilter(vector.DocTypeObservation, params.Project,
true) so the filter includes scope='global' (fix the occurrences used by
find_similar_observations and suggest_consolidations that call
s.vectorClient.Query with the resulting where) to ensure global observations are
visible to the similarity queries.
In `@internal/mcp/tools_memory.go`:
- Around line 23-31: The incoming params struct includes an Importance field but
it's never applied when saving; update the code path that calls StoreObservation
so the parsed params.Importance is propagated to the saved observation (e.g.,
set observation.Priority = params.Importance or pass it as an argument into
StoreObservation) — locate where params is used (the params struct definition
and the call site to StoreObservation) and ensure the importance value is
applied to the observation before persisting; also mirror this change for the
other occurrence noted around lines 143-145 where StoreObservation is invoked.
- Around line 53-57: The byte-based length checks and slicing for params.Content
(using len(params.Content) and params.Content[:softLimit]) must be converted to
rune-aware operations: replace byte-counting with utf8.RuneCountInString for
comparisons and convert strings to []rune for truncation, e.g., in the blocks
around the hardLimit/softLimit checks and the other occurrences you flagged
(including truncateTitle and any preview/truncated variables at the later
sites), so you compare rune counts and slice rune slices to the desired max
runes rather than slicing bytes.
- Around line 181-210: Handler for recall_memory lacks a project scope so
searches run with empty project and include global results; update the handler
and schemas to accept and forward project: add a Project string `json:"project"`
field to the local params struct used in the recall_memory handler, populate
search.SearchParams.Project = params.Project when building searchParams (in the
same function that constructs search.SearchParams), and update the handler's
InputSchema in internal/mcp/server.go to include the project field so the
API/validation and server-side handler stay in sync.
- Around line 29-30: The bug is that the incoming contract's project string is
not normalized before being used, so code paths in store_memory,
StoreObservation, and BuildWhereFilter receive an empty "" project which causes
incorrect storage under "" and wrong dedup/filter scopes (and includeGlobal
logic bypasses project checks). Fix by normalizing the contract project once at
the start of the store_memory flow (e.g., compute a normalizedProject or
nil-equivalent), then pass that normalized value into StoreObservation,
BuildWhereFilter, and any dedup/includeGlobal checks (instead of the raw
contract.Project) so dedup and recall/search honor the correct project scope.
Ensure all usages referenced (store_memory, StoreObservation, BuildWhereFilter
and includeGlobal handling) are changed to use the normalized value.
In `@plugin/engram/skills/memory/SKILL.md`:
- Line 48: The documentation references a non-existent tool name "merge" (seen
in the line "Found duplicate memories? → suggest_consolidations / merge");
update that reference to the exported tool name "merge_observations" so the
instruction matches the actual MCP export; search for and replace any
occurrences of the symbol "merge" in SKILL.md (or this specific line) with
"merge_observations" to prevent "tool not found" errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5721199-c26e-424b-b66f-d2b30ef410b2
📒 Files selected for processing (8)
TECHNICAL_DEBT.mdinternal/crypto/vault.gointernal/db/gorm/migrations.gointernal/db/gorm/observation_store.gointernal/mcp/server.gointernal/mcp/tools_credential.gointernal/mcp/tools_memory.goplugin/engram/skills/memory/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- TECHNICAL_DEBT.md
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/mcp/tools_credential.go
Go hooks now compute GitRemoteProjectID (8-char, cross-platform) with LegacyProjectID fallback. Both JS and Go session-start hooks send legacy_project, git_remote, relative_path to the server. handleContextInject reads these params and calls UpsertProject async, which re-associates existing observations from old path-based IDs to new git-remote-based IDs (idempotent, non-blocking). Session parser ProjectID now includes dirName_ prefix to match hook format.
Owner
Author
|
@codex review |
Security: - SQL injection prevention: allowlist for WhereClause.Column in pgvector - Credential leak prevention: json:"-" on encryption config fields - Prompt injection prevention: XML-escape memory content in user-prompt hook - Cache leak fix: filter credentials before caching in warmFrequentQueries Correctness: - Fix pq.StringArray for PostgreSQL TEXT[] column (Project.LegacyIDs) - Fix BuildWhereFilter scope handling (global/project/both) - Fix credential filtering: check obs_type metadata, not just Type field - Wire importance parameter to UpdateImportanceScore in store_memory - Fix recall_memory project scope propagation - Fix dedup to include global observations Robustness: - Add context timeout to UpsertProject background goroutine - Add 3s timeout to git commands in session parser - Fix os.Stat error handling in vault (don't overwrite on permission errors) - Add VaultExists helper for passive vault checks - UTF-8 rune-aware content limits in store_memory - Safe migration 031 rollback with credential type conversion - Collect searchIds after dedup/trimming in user-prompt hook
Owner
Author
🤖 PR Review MCP State (auto-managed, do not edit){
"version": 2,
"parentChildren": {},
"resolvedNitpicks": {
"coderabbit-nitpick-863e1856-15": {
"resolvedAt": "2026-03-12T00:10:01.950Z",
"resolvedBy": "agent"
}
},
"updatedAt": "2026-03-12T00:10:02.589Z"
} |
5 tasks
thebtf
added a commit
that referenced
this pull request
Apr 5, 2026
User prompt context:
- Server-side session prompt cache (sync.Map with 2h eviction)
- handleSessionInit stores last user prompt per session
- ProcessObservation forwards user prompt to BuildObservationPrompt
- <user_intent> tag added to extraction prompt — LLM now sees WHY
the agent performed an action, not just WHAT it did
Mid-session extract-learnings:
- PreCompact hook calls /api/sessions/{id}/extract-learnings with
last 20 messages (4000 token budget)
- Idempotency: skips if guidance observations already extracted for session
- Fire-and-forget (Constitution #3: non-blocking hooks)
Data model:
- UserPrompt field added to ObservationData (session manager queue)
- CountBySessionAndType for idempotency checks
4 tasks
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
Major evolution of engram's memory system across 4 phases + dead code cleanup:
SHA-256(absolutePath)[0:6]) with git-remote-based ID (SHA-256(remote_url + "/" + relative_path)[0:8]). Same repo on Windows and Linux now produces identical project ID. Dual-ID migration re-associates existing observations.store_memoryandrecall_memoryMCP tools. Agents can now create memories explicitly (not just passive capture via hooks). Content governance: hard limit 10K chars, soft limit 1K, dedup threshold 0.92, hierarchical tag expansion.BuildWhereFiltervector search bug that excludedscope='global'observations. NewWhereFilter/WhereClausetypes enable OR group SQL generation. Global observations now properly surface cross-project.engramInstructionsand skill file to documentstore_memory/recall_memoryas Tier 1 tools.53 files changed, +696 / -63,305 lines.
Test plan
store_memoryvia MCP — verify observation created with correct type/tags/scoperecall_memory— verify hybrid search returns relevant resultsduplicate: truescope=globalin project A, search from project B — must appeargo build ./...succeeds (except pre-existing go-tree-sitter CGo issue on Windows)go vetclean on all engram packagesEMBEDDING_PROVIDER=openaiconfigSummary by CodeRabbit
Новые функции
Улучшения
Безопасность
База данных