feat(worker): complete learning-memory-v4 follow-up phases#135
Merged
thebtf merged 13 commits intoApr 11, 2026
Conversation
Ship the post-MVP retrieval, briefing, trigger, merge, graph, and dashboard wave so GitHub can build and publish the new release for live validation.
WalkthroughДобавлены типизированные поисковые линии и их применение в retrieval, LLM-управляемое решение о слиянии наблюдений при записи (DecideMerge), проектные брифинги с генерирующей задачей обслуживания, новый серверный endpoint /api/memory/triggers, file-scoped retrieval (files_being_edited), опциональная BFS-инжекция графа, изменение супрессии по флагу и множественные тесты и документация. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Клиент/IDE
participant Handler as handleMemoryTriggers
participant Vector as VectorClient
participant Store as ObservationStore
Client->>Handler: POST /api/memory/triggers (tool, project, params)
Handler->>Vector: Query (BuildWhereFilter с project/fileScope)
Vector-->>Handler: QueryResults (observation_ids)
Handler->>Store: GetObservationsByIDs
Store-->>Handler: Observations
Handler->>Client: JSON matches (Kind, ObservationID, Blurb)
sequenceDiagram
participant SDK as SDK Processor
participant Vector as VectorClient
participant Store as ObservationStore
participant LLM as LLM Client
SDK->>Vector: Query similar observations
Vector-->>SDK: candidate IDs
SDK->>Store: FetchObservationsByIDs
Store-->>SDK: candidate Observations
rect rgba(135, 206, 250, 0.5)
SDK->>LLM: DecideMerge (newObs + candidates)
LLM-->>SDK: {action, target_id}
end
alt SKIP
SDK->>SDK: Пропустить сохранение
else CREATE_NEW
SDK->>Store: StoreObservation
else UPDATE
SDK->>Store: UpdateObservation (merge fields)
else SUPERSEDE
SDK->>Store: StoreObservation
SDK->>Store: MarkAsSuperseded(target_id)
end
sequenceDiagram
participant Worker as Retrieval
participant Vector as VectorClient
participant Graph as GraphStore
participant Search as Search Manager
Worker->>Vector: BuildWhereFilter(..., filePaths?)
Worker->>Vector: Query
Vector-->>Worker: observations + scores
alt InjectGraphBFSEnabled
Worker->>Worker: ExtractSessionEntitySeeds
Worker->>Graph: GetNeighbors(seed)
Graph-->>Worker: neighbor IDs
Worker->>Search: RRF fuse vector scores + neighbor seeds
Search-->>Worker: fused scores
end
alt TypeLanesEnabled
Worker->>Worker: Group by ObservationType -> filter by MinScore, TopK
Worker->>Search: ApplyLaneWeights
Search-->>Worker: reweighted results
end
Worker-->>Client: selected observations
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
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 implements the Learning Memory v4 feature wave, introducing file-scope prefiltering, per-type search lanes, project briefings, and write-time merge logic. Key improvements include a unified retrieval pipeline for both inject and search paths, and a new alarm model for semantic trigger matching. However, several critical issues were identified: a logic error in the write-merge path prevents new observations from being stored during supersession, RRF scores are incorrectly compared against cosine similarity thresholds in typed lanes when graph expansion is enabled, and session signal tracking relies on local temporary files which breaks distributed workstation setups. Additionally, the typed lane selection logic fails to respect global result limits and lacks a final re-sorting step after score adjustments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugin/engram/hooks/lib.js (1)
348-400:⚠️ Potential issue | 🟠 MajorГонка при межпроцессном обновлении session signals.
Функции
incrementSessionSignals(л. 348) иappendSessionFile(л. 376) используют паттерн read-modify-write на одном файле (_signalPath(sessionID)) без блокировки. Так как хуки работают как независимые процессы (см. комментарий на л. 334), параллельные вызовы приводят к потере обновлений: один процесс перезаписывает изменения другого, теряя либо счётчики, либо записи в массивеfiles.Требуется добавить синхронизацию (например, через lock-файл или атомарную операцию) в общий helper для обновления состояния.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/engram/hooks/lib.js` around lines 348 - 400, Both incrementSessionSignals and appendSessionFile perform unsafe read-modify-write on the same file returned by _signalPath(sessionID); you must serialize access to avoid lost updates—implement a small helper (e.g. with a lock-file or atomic file-rename + retry loop) like withSessionSignalUpdate(sessionID, updater) that acquires a per-session lock, reads JSON, calls updater(current) to get the new state, writes atomically (write to temp + rename), and releases the lock; then refactor incrementSessionSignals and appendSessionFile to call withSessionSignalUpdate(sessionID, current => { ...modify current as before... return next; }) so all updates are synchronized across processes.internal/worker/handlers_data.go (1)
998-1008:⚠️ Potential issue | 🟡 MinorНесоответствие комментария Godoc и функции.
Комментарий Godoc на строках 998-1007 описывает
handleGetSimilarityTelemetry, но следует функцияhandleMCPHitRateAnalytics. Вероятно, новая функция была добавлена перед существующим обработчиком без собственной документации.📝 Предлагаемое исправление
-// handleGetSimilarityTelemetry godoc -// `@Summary` Get similarity telemetry -// `@Description` Returns the latest similarity telemetry data. Optionally filter by project to get a single snapshot. -// `@Tags` Analytics -// `@Produce` json -// `@Security` ApiKeyAuth -// `@Param` project query string false "Filter by project" -// `@Success` 200 {object} map[string]interface{} -// `@Failure` 500 {string} string "internal error" -// `@Router` /api/telemetry/similarity [get] +// handleMCPHitRateAnalytics generates hit rate analytics in Markdown format for MCP tools. func (s *Service) handleMCPHitRateAnalytics(ctx context.Context, project string, limit int) (string, error) {И перенесите исходный комментарий Godoc к функции
handleGetSimilarityTelemetryна строке 1164.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_data.go` around lines 998 - 1008, Godoc над функцией сейчас описывает handleGetSimilarityTelemetry, тогда как под ним объявлена handleMCPHitRateAnalytics; переместите/восстановите правильный комментарий: вытащите существующий блок Godoc (котрый описывает handleGetSimilarityTelemetry) и перенесите его над реальной функции handleGetSimilarityTelemetry, а над текущей функции handleMCPHitRateAnalytics добавьте корректную документацию (или новый краткий Godoc), чтобы комментарии соответствовали именам функций handleGetSimilarityTelemetry и handleMCPHitRateAnalytics.internal/worker/retrieval.go (1)
227-323:⚠️ Potential issue | 🟠 MajorФайл-скоуп теряется при fallback и BFS fusion.
opts.FilePathsиспользуется только вvector.BuildWhereFilter(...). Если векторный путь недоступен/пустой, дальше возвращаются project-wide результаты изsearchFallbackObservations(...), а затем BFS может добавить ещё соседей без повторной фильтрации по файлам. В результате новыйFilePathsне является инвариантом финальной выдачи.Что лучше сделать
if !usedVector || len(observations) == 0 { if vectorSearchFailed { log.Info().Str("project", project).Msg("Using FTS fallback due to vector search failure") } scopeFilter := gorm.ScopeFilter{Project: project, AgentID: state.agentID} fallbackObservations, fallbackErr := s.searchFallbackObservations(ctx, query, scopeFilter, limit) if fallbackErr != nil { return nil, nil, fallbackErr } observations = fallbackObservations } + observations = filterObservationsByFilePaths(observations, opts.FilePaths) if s.config != nil && s.config.InjectGraphBFSEnabled { ... } + observations = filterObservationsByFilePaths(observations, opts.FilePaths)Здесь нужен общий post-filter по
FilesRead/FilesModified, чтобы любое ответвление retrieval pipeline соблюдало один и тот же file scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/retrieval.go` around lines 227 - 323, The retrieval pipeline loses file-scoping because opts.FilePaths is only applied to vector.BuildWhereFilter and not enforced after the FTS fallback or BFS fusion; add a post-filter that enforces FilesRead/FilesModified against opts.FilePaths (e.g., a helper like filterObservationsByFilePaths(ctx, observations, opts.FilePaths) that checks observation.FilesRead/FilesModified), and invoke it whenever observations is assigned from fetchObservationsByID or searchFallbackObservations (references: vector.BuildWhereFilter, searchFallbackObservations, ExtractSessionEntitySeeds, lookupGraphSeedNeighbors, fetchObservationsByID) so the final observations always respect the original FilePaths scope.
🧹 Nitpick comments (6)
internal/vector/types_test.go (1)
39-43: Снизьте хрупкость теста к внутренней форме фильтра.Проверка
require.Len(t, clause.OrGroup, 0)для каждого clause излишне привязывает тест к текущему внутреннему представлению. Лучше оставить проверку только семантики (отсутствиеfiles_modified/files_read), чтобы безопасно рефакторить builder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vector/types_test.go` around lines 39 - 43, The test is too coupled to the filter's internal form by asserting require.Len(t, clause.OrGroup, 0); remove that assertion and only assert the semantic expectations (that none of the clauses has clause.Column equal to "files_modified" or "files_read"), keeping the loop over filter.Clauses but dropping checks against clause.OrGroup so the builder implementation can be refactored safely; specifically update the test that iterates filter.Clauses to no longer reference clause.OrGroup and only perform the assert.NotEqual checks.plugin/openclaw-engram/src/client.ts (1)
370-388: Рассмотрите более надёжный парсинг MCP-ответа.Текущая проверка через
includes()на строкуRated observation ${id} as ${rating}хрупка — если формат сообщения на сервере изменится, метод вернётfalseбез индикации ошибки. Рассмотрите проверку на структуру успешного MCP-ответа вместо текстового содержимого.♻️ Возможный рефакторинг
async rateObservation(id: number, rating: 'useful' | 'not_useful'): Promise<boolean> { - const resp = await this.post<{ result?: { content?: Array<{ text?: string }> } }>( + const resp = await this.post<{ result?: { content?: Array<{ text?: string }> }; error?: { message?: string } }>( '/mcp', { jsonrpc: '2.0', id: 1, method: 'tools/call', params: { name: 'feedback', arguments: { action: 'rate', id, rating, }, }, }, ); - return resp?.result?.content?.[0]?.text?.includes(`Rated observation ${id} as ${rating}`) ?? false; + if (resp?.error) { + console.error(`[engram] rateObservation failed: ${resp.error.message}`); + return false; + } + return resp?.result?.content != null && resp.result.content.length > 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/openclaw-engram/src/client.ts` around lines 370 - 388, The current rateObservation implementation relies on string matching the message text which is brittle; update rateObservation to validate the MCP response structure instead: inspect the returned object from this.post (the resp variable) and check for an explicit success indicator (e.g., a boolean or status field under resp.result or in resp.result.content[0]) or a structured payload (e.g., content[0].type === 'status' and content[0].status === 'ok' or content[0].id === id and content[0].rating === rating) rather than using includes on text; if the expected structure is absent, treat it as an error (throw or return false) so failures are detectable. Use the existing rateObservation and post symbols to locate and modify the parsing logic and add clear guards for resp, resp.result, and resp.result.content array shape before evaluating success.internal/worker/retrieval_helpers.go (1)
230-240: Рассмотрите логирование ошибок при получении entity observations.Ошибки при получении entity observations игнорируются молча. Хотя это может быть намеренным для обеспечения отказоустойчивости, логирование на уровне Debug помогло бы при диагностике проблем.
♻️ Предлагаемое изменение
var entityObservations []*models.Observation if s.retrievalHooks != nil && s.retrievalHooks.getEntityObservationsBySession != nil { - entityObservations, _ = s.retrievalHooks.getEntityObservationsBySession(ctx, sessionID) + var err error + entityObservations, err = s.retrievalHooks.getEntityObservationsBySession(ctx, sessionID) + if err != nil { + log.Debug().Err(err).Str("session_id", sessionID).Msg("failed to get entity observations via hook") + } } else if s.observationStore != nil { - allSessionObservations, _ := s.observationStore.GetObservationsBySession(ctx, sessionID) + allSessionObservations, err := s.observationStore.GetObservationsBySession(ctx, sessionID) + if err != nil { + log.Debug().Err(err).Str("session_id", sessionID).Msg("failed to get session observations") + } for _, obs := range allSessionObservations {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/retrieval_helpers.go` around lines 230 - 240, The code silently ignores errors when fetching entity observations; update the branches that call s.retrievalHooks.getEntityObservationsBySession and s.observationStore.GetObservationsBySession to capture their returned errors and log them (at Debug level) via the existing logger (include context like sessionID and the error), then proceed with fallback behavior; target the variables/functions entityObservations, s.retrievalHooks.getEntityObservationsBySession, s.observationStore.GetObservationsBySession and models.Observation so the error-logging is added immediately after each call without changing the current success/fallback flow.internal/worker/sdk/processor.go (2)
485-488: Ошибки DecideMerge обрабатываются молча.При ошибке
DecideMergeфункция возвращаетnil, false, nil, что приводит к fallback на create path. Это обеспечивает отказоустойчивость, но может скрывать постоянные проблемы с LLM.🔍 Рекомендация
Добавьте логирование ошибки на уровне Warning для облегчения диагностики:
decision, err := learning.DecideMerge(ctx, p.llmClient, newObs, candidates) if err != nil { + log.Warn().Err(err).Str("project", project).Msg("write-merge: DecideMerge failed, falling back to create path") return nil, false, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/sdk/processor.go` around lines 485 - 488, В блоке где вызывается learning.DecideMerge (decision, err := learning.DecideMerge(ctx, p.llmClient, newObs, candidates)) не логируется ошибка — при err вы просто возвращаете nil, false, nil; добавьте логирование уровня Warning/Warningf перед возвратом, включив текст ошибки и полезный контекст (например идентификаторы запроса/обсервации или длину candidates) используя текущий логгер в p (или processLogger), чтобы не терять информацию о повторяющихся проблемах с LLM.
691-696: Логика определения UPDATE-пути хрупкая.Проверка
mergedObs.Narrative.String != obs.Narrative || mergedObs.Title.String != obs.Title || len(mergedObs.Facts) != len(obs.Facts)для определения, был ли выполнен UPDATE, может дать ложноположительные результаты если merge просто объединил идентичные значения.Рекомендуется использовать явный флаг или возвращать тип действия из
applyWriteMergeDecision.♻️ Предлагаемый подход
Измените возвращаемое значение
applyWriteMergeDecisionчтобы явно указывать тип выполненного действия:type writeMergeResult struct { observation *models.Observation action string // "update", "supersede", "skip", "" } func (p *Processor) applyWriteMergeDecision(...) (writeMergeResult, error) { // ... case learning.MergeActionUpdate: // ... return writeMergeResult{observation: updated, action: "update"}, nil // ... }Это позволит точно определить, какое действие было выполнено, без сравнения полей.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/sdk/processor.go` around lines 691 - 696, The current UPDATE-path detection is fragile because it compares mergedObs fields to obs; modify applyWriteMergeDecision to return an explicit result type (e.g., writeMergeResult with fields observation *models.Observation and action string/const like "update"/"supersede"/"skip") and update all callers to check result.action instead of comparing mergedObs.Narrative/Title/Facts; specifically replace the mergedObs vs obs comparison around storedCount with a check like result.action == "update" (or the chosen enum) and use result.observation for the updated row; update any tests and call sites that expect the old return signature accordingly.internal/worker/sdk/processor_test.go (1)
1889-1920: Не собирайте test XML конкатенацией без escaping.Этот helper теперь сериализует и
commands_run, а shell-команды очень быстро приносят&,<и>. Как только в фикстуре появится что-то вродеgo test ./... && go vet ./..., тест начнёт падать на невалидном XML вместо проверки merge-логики.Лучше собирать payload через
encoding/xmlили хотя бы прогонять значения черезxml.EscapeTextперед вставкой.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/sdk/processor_test.go` around lines 1889 - 1920, testObservationXML builds XML by concatenating raw strings which breaks when values contain XML-special chars (e.g. commands_run with "&" or "<"); update testObservationXML to escape all inserted text (title, narrative) and each element in facts, concepts, filesRead, filesModified and commands (commands_run) using encoding/xml (e.g. xml.EscapeText) or by constructing the payload with encoding/xml marshalling instead of string concat so the resulting XML is always valid; ensure the function still returns an <observation> with the same element names and that commands are serialized into <commands_run>/<command> elements.
🤖 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 753-777: Add handling for the ENGRAM_TYPE_SEARCH_LANES environment
override: read strings.TrimSpace(os.Getenv("ENGRAM_TYPE_SEARCH_LANES")), and if
non-empty attempt to json.Unmarshal it into the existing lane-overrides map on
the config (e.g., cfg.TypeSearchLanes or the TypeSearch lane overrides field),
and only assign the parsed map when unmarshalling succeeds (log or ignore errors
consistent with nearby blocks). Place this new block alongside the other env
flag parses (near cfg.TypeLanesEnabled, cfg.ProjectBriefingEnabled, etc.) so the
runtime can override the settings.json rollout map via environment variable.
In `@internal/db/gorm/observation_store.go`:
- Around line 524-531: The GetProjectBriefingObservation function can run the
query with project=="" and match global/empty-project rows; add a guard at the
start of GetProjectBriefingObservation to reject empty project values (e.g.,
return an error such as gorm.ErrRecordNotFound or a validation error) before
calling s.db.Scopes(activeObservationFilter(), importanceOrdering()) so the DB
WHERE("project = ?", project) is never executed with an empty string; update
callers/tests if they rely on empty-project semantics. Ensure references:
GetProjectBriefingObservation, dbObservation, activeObservationFilter,
importanceOrdering.
- Around line 565-579: The query in GetObservationsByCommandPrefix builds
pattern from user input and passes it to LIKE, so unescaped '%' and '_' in
command will act as wildcards; fix by escaping '%' and '_' in the command before
appending '%' (e.g. use strings.ReplaceAll to replace "%" -> "\\%" and "_" ->
"\\_"), set pattern := escaped + "%" and update the SQL predicate to use LIKE ?
ESCAPE '\' (i.e. WHERE "EXISTS (SELECT 1 FROM
jsonb_array_elements_text(commands_run) AS cmd WHERE cmd LIKE ? ESCAPE '\\')")
so commands_run is matched as a literal prefix.
In `@internal/learning/merge.go`:
- Around line 83-93: parseMergeDecision currently accepts MergeActionUpdate and
MergeActionSupersede even when the parsed.MergeDecision has no valid target
(TargetID == 0); update/supersede must require a concrete target. Modify
parseMergeDecision (and use the MergeDecision struct's TargetID field) so that
after unmarshalling you additionally check that for actions MergeActionUpdate
and MergeActionSupersede parsed.TargetID > 0; if not, return MergeDecision{}
false (so caller can fallback to CREATE_NEW) — keep existing returns for
MergeActionCreateNew and MergeActionSkip unchanged.
In `@internal/mcp/tools_memory.go`:
- Around line 198-210: The code enters the dedupResult.Action ==
dedup.ActionUpdate branch but when storePathSupersessionEnabled is false it logs
that nothing was superseded while leaving contradictionAction as UPDATE; change
the branch so that when storePathSupersessionEnabled is false you explicitly set
the contradictionAction (or dedupResult.Action used downstream) to a
non-update/no-op value (e.g., dedup.ActionIgnore/Noop) to reflect that no update
occurred; make the same fix in the similar spots around the later handling (the
logic around contradictionAction at lines ~260 and the block at ~317-327) so the
API and subsequent code behave consistently.
In `@internal/vector/pgvector/client.go`:
- Around line 341-359: The code builds SQL fragments using oc.Operator (e.g., in
the orParts assembly in client.go) without validating it, allowing malformed
operators into the query; add an explicit allowlist of permitted operators
(e.g., "=", "!=","<>", "<", ">", "<=", ">=", "ILIKE", "LIKE", "?|", "@>", "<@")
and validate/normalize operator := oc.Operator against that list before using it
in fmt.Sprintf; if the operator is empty default to "=" and if it is not in the
allowlist return an error (or sanitize to a safe default) so invalid or
malicious operators cannot reach the constructed SQL; apply the same validation
logic to the other similar block referenced around lines 366-371 that also
injects oc.Operator into orParts/andParts or WHERE building (e.g., WhereFilter
handling).
- Around line 319-326: The allowlist in buildWhereClauses includes
files_modified and files_read which do not exist on the vectors table and will
cause "column does not exist" SQL errors when Query() builds WHERE clauses;
remove "files_modified" and "files_read" from the allowedColumns map in
buildWhereClauses (and any other allowlist definitions) so Query() will not emit
filters for those fields until storage for metadata is implemented, or
alternatively implement a proper storage change: add a JSONB metadata column to
the vectors table via a migration, add a corresponding field to the vectorRecord
GORM model, and update sync.go to persist Document.Metadata into that column
before re-enabling those keys in the allowlist.
In `@internal/worker/handlers_ingest.go`:
- Around line 219-221: The current branch that calls
s.observationStore.MarkAsSuperseded (guarded by
config.Get().StorePathSupersessionEnabled) swallows the returned error; change
it to capture the error from MarkAsSuperseded, and on non-nil error either
return it from the handler or propagate/report it (e.g., wrap with context about
ingestDedupResult.ExistingID and the operation) instead of ignoring it so the
handler does not continue a success flow when supersession fails.
In `@internal/worker/retrieval.go`:
- Around line 82-128: The feature flag check is too strict: typeLanesEnabled()
currently requires s.config.TypeSearchLanes to be non-empty so a config that
only wants default lanes won’t enable type lanes; remove the len(...) > 0
requirement so the flag respects s.config.TypeLanesEnabled when a config exists
(update typeLanesEnabled to return s.config != nil &&
s.config.TypeLanesEnabled). Also ensure laneWeightMap() returns weights for
defaults when the config has no TypeSearchLanes by merging
search.DefaultSearchLanes with s.config.TypeSearchLanes (use
models.ObservationType(name) keys, preferring explicit config values but falling
back to search.DefaultSearchLanes), and verify related consumers like
laneConfigForType / typedLaneMinScore still get appropriate fallbacks.
In `@internal/worker/sdk/parser.go`:
- Around line 97-98: The CommandsRun values extracted in parser.go via
extractArrayElements must be sanitized to mask secrets before persistence;
update the parsing path (in parser.go around the extractArrayElements call and
the extractArrayElements implementation) to run each command string through a
sanitizer function (e.g., MaskSecrets or SanitizeCommand) that detects and
redacts tokens, passwords, API keys and SSH credentials, or alternatively call
that sanitizer from the persistence layer in observation_store.go before saving
to JSONB; reuse the same masking conventions used for EncryptedSecret handling
so the redaction format is consistent across credential observations and ensure
both occurrences (lines ~97–98 and ~158–159) use this sanitizer.
In `@internal/worker/trigger_matcher_bash_test.go`:
- Around line 46-58: The tests seed observations into a shared DB using fixed
project/sdkSessionID which breaks isolation across runs; modify the setup in
testObservationStoreForWorker / the calls to sessionStore.CreateSDKSession and
seedBashTriggerObservation to use a unique namespace (e.g., include t.Name() or
a UUID in the project or sdkSessionID) or add explicit teardown that deletes
inserted rows from observationStore after the test completes (ensure cleanup
removes rows created by seedBashTriggerObservation and any session rows created
by CreateSDKSession) so repeated or parallel runs don't affect assertions like
Len == 3 and Len == 0.
- Line 23: The test uses a literal "Bearer sk-..." token in secretCommand which
will trigger secret scanners; change secretCommand so the token is not a
contiguous realistic-looking secret (e.g., build it from concatenated parts or
replace with a safe sentinel string that still triggers
privacy.ContainsSecrets). Update the declaration of secretCommand in the test
and any assertions that rely on its value (keep the header and URL semantics) so
the test behavior is preserved but the raw "sk-..." pattern is not present as a
single literal.
In `@internal/worker/trigger_matcher.go`:
- Around line 106-114: The fallback in filePathObservations ignores the project
parameter and calls observationStore.GetObservationsByFile only with filePath,
breaking project isolation; modify filePathObservations to respect project by
either calling a project-aware store method (e.g.,
observationStore.GetObservationsByProjectAndFile(ctx, project, filePath, limit))
or by filtering the results of GetObservationsByFile to only include
observations whose Project equals the given project; ensure this change is
applied in the branch where retrievalHooks.filePathObservations is nil and when
observationStore is non-nil so project-scoped observations are returned.
- Around line 244-245: The buildEditWriteTriggerQuery function currently embeds
the entire params payload (via toJSONString(params)) which can leak secrets and
large user text; update buildEditWriteTriggerQuery to serialize only a safe
whitelist of allowed keys from params (e.g., small metadata fields like action,
filePath, userId, timestamp or whatever project-safe keys you define) and omit
file content/patch fields and any keys that may contain secrets; locate the
function buildEditWriteTriggerQuery and replace the full toJSONString(params)
usage with a routine that constructs a new map containing only the permitted
keys and then serializes that map for the query.
In `@pkg/models/observation.go`:
- Around line 239-240: The CommandsRun field (type JSONStringArray) is currently
persisted and returned raw, which exposes secrets in shell/Bash commands; update
the handling around CommandsRun (and any serialization paths that emit it) to
either persist a redacted/normalized version or omit secret-containing entries
before saving/serializing—implement a sanitizer function (e.g.,
redactShellSecrets) and call it from the model's marshaling/save flow where
CommandsRun is prepared (and likewise for any code paths that read/write
CommandsRun, such as JSON marshaling helpers or DB write helpers) so stored and
API-output values do not include raw tokens/secrets.
In `@plugin/engram/hooks/pre-tool-use.js`:
- Around line 131-146: In fetchTriggerContext, the normalized command/filename
values (derived from aliases like cmd/command and file/filename via getString()
and extractFilePath()) are computed but you still pass the original toolInput
into lib.requestPost; update the payload to send the canonicalized object (e.g.,
build a normalizedInput with normalized command and file_path fields or use the
URLSearchParams values) so lib.requestPost('/api/memory/triggers', ...) receives
the normalized keys; modify the lib.requestPost call inside fetchTriggerContext
to send that normalizedInput instead of toolInput and ensure the session/project
fields remain included.
In `@README.md`:
- Line 20: Unify the wording about the inject behavior: choose one phrasing
(either "returns nothing" or "returns an empty relevant-memory block") and make
it consistent in the intro, the paragraph that currently reads "Since
learning-memory-v4, context injection treats **silence as valid**..." and the
referenced lines around 138-139; update the sentence that mentions "returns
nothing instead of force-filling..." or the sentence that mentions "returns an
empty block" to use the same term and ensure the ENGRAM_INJECT_UNIFIED note
remains intact so readers understand inject/search share the unified retrieval
pipeline semantics.
In `@ui/src/views/LearningView.vue`:
- Around line 37-42: The hit-rate widget is being fetched without the selected
period, so it doesn't update with Last 7/30/90; update the calls to
fetchHitRateAnalytics to include the selectedDays period (e.g., pass
selectedDays.value or map to the API param) where it's invoked (the Promise.all
call with fetchHitRateAnalytics and the other occurrences around lines
referenced), and if the API signature lacks a period parameter, add an optional
period argument to fetchHitRateAnalytics and thread it through the API client so
the widget receives period-filtered data; alternatively, if you intentionally
want a snapshot, rename the UI section to "Snapshot" instead of changing the
API.
---
Outside diff comments:
In `@internal/worker/handlers_data.go`:
- Around line 998-1008: Godoc над функцией сейчас описывает
handleGetSimilarityTelemetry, тогда как под ним объявлена
handleMCPHitRateAnalytics; переместите/восстановите правильный комментарий:
вытащите существующий блок Godoc (котрый описывает handleGetSimilarityTelemetry)
и перенесите его над реальной функции handleGetSimilarityTelemetry, а над
текущей функции handleMCPHitRateAnalytics добавьте корректную документацию (или
новый краткий Godoc), чтобы комментарии соответствовали именам функций
handleGetSimilarityTelemetry и handleMCPHitRateAnalytics.
In `@internal/worker/retrieval.go`:
- Around line 227-323: The retrieval pipeline loses file-scoping because
opts.FilePaths is only applied to vector.BuildWhereFilter and not enforced after
the FTS fallback or BFS fusion; add a post-filter that enforces
FilesRead/FilesModified against opts.FilePaths (e.g., a helper like
filterObservationsByFilePaths(ctx, observations, opts.FilePaths) that checks
observation.FilesRead/FilesModified), and invoke it whenever observations is
assigned from fetchObservationsByID or searchFallbackObservations (references:
vector.BuildWhereFilter, searchFallbackObservations, ExtractSessionEntitySeeds,
lookupGraphSeedNeighbors, fetchObservationsByID) so the final observations
always respect the original FilePaths scope.
In `@plugin/engram/hooks/lib.js`:
- Around line 348-400: Both incrementSessionSignals and appendSessionFile
perform unsafe read-modify-write on the same file returned by
_signalPath(sessionID); you must serialize access to avoid lost
updates—implement a small helper (e.g. with a lock-file or atomic file-rename +
retry loop) like withSessionSignalUpdate(sessionID, updater) that acquires a
per-session lock, reads JSON, calls updater(current) to get the new state,
writes atomically (write to temp + rename), and releases the lock; then refactor
incrementSessionSignals and appendSessionFile to call
withSessionSignalUpdate(sessionID, current => { ...modify current as before...
return next; }) so all updates are synchronized across processes.
---
Nitpick comments:
In `@internal/vector/types_test.go`:
- Around line 39-43: The test is too coupled to the filter's internal form by
asserting require.Len(t, clause.OrGroup, 0); remove that assertion and only
assert the semantic expectations (that none of the clauses has clause.Column
equal to "files_modified" or "files_read"), keeping the loop over filter.Clauses
but dropping checks against clause.OrGroup so the builder implementation can be
refactored safely; specifically update the test that iterates filter.Clauses to
no longer reference clause.OrGroup and only perform the assert.NotEqual checks.
In `@internal/worker/retrieval_helpers.go`:
- Around line 230-240: The code silently ignores errors when fetching entity
observations; update the branches that call
s.retrievalHooks.getEntityObservationsBySession and
s.observationStore.GetObservationsBySession to capture their returned errors and
log them (at Debug level) via the existing logger (include context like
sessionID and the error), then proceed with fallback behavior; target the
variables/functions entityObservations,
s.retrievalHooks.getEntityObservationsBySession,
s.observationStore.GetObservationsBySession and models.Observation so the
error-logging is added immediately after each call without changing the current
success/fallback flow.
In `@internal/worker/sdk/processor_test.go`:
- Around line 1889-1920: testObservationXML builds XML by concatenating raw
strings which breaks when values contain XML-special chars (e.g. commands_run
with "&" or "<"); update testObservationXML to escape all inserted text (title,
narrative) and each element in facts, concepts, filesRead, filesModified and
commands (commands_run) using encoding/xml (e.g. xml.EscapeText) or by
constructing the payload with encoding/xml marshalling instead of string concat
so the resulting XML is always valid; ensure the function still returns an
<observation> with the same element names and that commands are serialized into
<commands_run>/<command> elements.
In `@internal/worker/sdk/processor.go`:
- Around line 485-488: В блоке где вызывается learning.DecideMerge (decision,
err := learning.DecideMerge(ctx, p.llmClient, newObs, candidates)) не логируется
ошибка — при err вы просто возвращаете nil, false, nil; добавьте логирование
уровня Warning/Warningf перед возвратом, включив текст ошибки и полезный
контекст (например идентификаторы запроса/обсервации или длину candidates)
используя текущий логгер в p (или processLogger), чтобы не терять информацию о
повторяющихся проблемах с LLM.
- Around line 691-696: The current UPDATE-path detection is fragile because it
compares mergedObs fields to obs; modify applyWriteMergeDecision to return an
explicit result type (e.g., writeMergeResult with fields observation
*models.Observation and action string/const like "update"/"supersede"/"skip")
and update all callers to check result.action instead of comparing
mergedObs.Narrative/Title/Facts; specifically replace the mergedObs vs obs
comparison around storedCount with a check like result.action == "update" (or
the chosen enum) and use result.observation for the updated row; update any
tests and call sites that expect the old return signature accordingly.
In `@plugin/openclaw-engram/src/client.ts`:
- Around line 370-388: The current rateObservation implementation relies on
string matching the message text which is brittle; update rateObservation to
validate the MCP response structure instead: inspect the returned object from
this.post (the resp variable) and check for an explicit success indicator (e.g.,
a boolean or status field under resp.result or in resp.result.content[0]) or a
structured payload (e.g., content[0].type === 'status' and content[0].status ===
'ok' or content[0].id === id and content[0].rating === rating) rather than using
includes on text; if the expected structure is absent, treat it as an error
(throw or return false) so failures are detectable. Use the existing
rateObservation and post symbols to locate and modify the parsing logic and add
clear guards for resp, resp.result, and resp.result.content array shape before
evaluating success.
🪄 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: 1fc6bfd6-2b19-48ae-800c-b615243fbd25
📒 Files selected for processing (75)
CHANGELOG.mdREADME.mdREADME.ru.mdREADME.zh.mddocs/arch/GOTCHAS.mdinternal/config/config.gointernal/config/config_test.gointernal/db/gorm/models.gointernal/db/gorm/observation_store.gointernal/db/gorm/observation_store_test.gointernal/dedup/checker.gointernal/instincts/dedup.gointernal/learning/briefing.gointernal/learning/briefing_test.gointernal/learning/merge.gointernal/learning/merge_test.gointernal/maintenance/near_dedup.gointernal/maintenance/service.gointernal/maintenance/service_test.gointernal/mcp/server.gointernal/mcp/server_test.gointernal/mcp/store_supersession_test.gointernal/mcp/tools_memory.gointernal/mcp/tools_store_consolidated.gointernal/relation/detector.gointernal/search/lanes.gointernal/search/lanes_test.gointernal/search/manager.gointernal/search/manager_test.gointernal/telemetry/similarity.gointernal/vector/pgvector/client.gointernal/vector/pgvector/client_sql_test.gointernal/vector/types.gointernal/vector/types_test.gointernal/worker/handlers_context.gointernal/worker/handlers_context_briefing_test.gointernal/worker/handlers_context_inject_unified_test.gointernal/worker/handlers_data.gointernal/worker/handlers_data_test.gointernal/worker/handlers_ingest.gointernal/worker/handlers_ingest_test.gointernal/worker/handlers_triggers.gointernal/worker/handlers_triggers_test.gointernal/worker/retrieval.gointernal/worker/retrieval_helpers.gointernal/worker/retrieval_test.gointernal/worker/sdk/parser.gointernal/worker/sdk/processor.gointernal/worker/sdk/processor_test.gointernal/worker/service.gointernal/worker/service_routes_test.gointernal/worker/trigger_matcher.gointernal/worker/trigger_matcher_bash_test.gointernal/worker/trigger_matcher_read_test.gointernal/worker/trigger_matcher_test.gopkg/models/observation.goplugin/engram/.claude-plugin/plugin.jsonplugin/engram/commands/issue.mdplugin/engram/commands/retro.mdplugin/engram/hooks/lib.jsplugin/engram/hooks/lib.test.jsplugin/engram/hooks/pre-tool-use.jsplugin/engram/hooks/pre-tool-use.test.jsplugin/engram/hooks/session-start.jsplugin/engram/hooks/session-start.test.jsplugin/engram/hooks/user-prompt.jsplugin/engram/hooks/user-prompt.test.jsplugin/engram/skills/memory/SKILL.mdplugin/openclaw-engram/openclaw.plugin.jsonplugin/openclaw-engram/package.jsonplugin/openclaw-engram/src/client.tsplugin/openclaw-engram/src/tools/engram-issues.tsplugin/openclaw-engram/src/tools/engram-rate.tsui/src/utils/api.tsui/src/views/LearningView.vue
…iggers Fix write-merge action handling for SUPERSEDE/UPDATE, preserve typed-lane MinScore semantics under graph fusion, and make repeated-read triggers work in remote worker setups by passing read-count signals in trigger requests.
Scope read-trigger file lookups by project, strip sensitive payloads from Edit/Write trigger queries, normalize trigger request params in pre-tool-use hook, and reject UPDATE/SUPERSEDE merge decisions without target IDs.
Owner
Author
🤖 PR Review MCP State (auto-managed, do not edit){
"version": 2,
"parentChildren": {},
"resolvedNitpicks": {
"coderabbit-nitpick-fcc92962-691": {
"resolvedAt": "2026-04-11T21:06:57.434Z",
"resolvedBy": "agent"
},
"coderabbit-outside-diff-fcc92962-691": {
"resolvedAt": "2026-04-11T21:07:00.460Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-83d257e9-485": {
"resolvedAt": "2026-04-11T21:07:03.065Z",
"resolvedBy": "agent"
},
"coderabbit-outside-diff-83d257e9-485": {
"resolvedAt": "2026-04-11T21:07:05.771Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-c839c8da-39": {
"resolvedAt": "2026-04-11T21:38:50.052Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-55d37369-370": {
"resolvedAt": "2026-04-11T21:38:52.679Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-d5dafde0-230": {
"resolvedAt": "2026-04-11T21:38:55.637Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-6c36cc5c-1889": {
"resolvedAt": "2026-04-11T21:38:58.760Z",
"resolvedBy": "agent"
},
"coderabbit-outside-diff-68337db7-39": {
"resolvedAt": "2026-04-11T21:39:01.387Z",
"resolvedBy": "agent"
},
"coderabbit-outside-diff-55d37369-370": {
"resolvedAt": "2026-04-11T21:39:04.025Z",
"resolvedBy": "agent"
},
"coderabbit-outside-diff-d5dafde0-230": {
"resolvedAt": "2026-04-11T21:39:06.727Z",
"resolvedBy": "agent"
},
"coderabbit-outside-diff-6c36cc5c-1889": {
"resolvedAt": "2026-04-11T21:39:09.303Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-d52599af-1828": {
"resolvedAt": "2026-04-11T22:49:23.707Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-4d0e6e77-2117": {
"resolvedAt": "2026-04-11T22:49:27.325Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-c054b22e-402": {
"resolvedAt": "2026-04-11T23:04:43.399Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-bf4d34b3-280": {
"resolvedAt": "2026-04-11T23:04:48.411Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-ff82e760-63": {
"resolvedAt": "2026-04-11T23:07:07.137Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-d6431c9b-500": {
"resolvedAt": "2026-04-11T23:07:10.086Z",
"resolvedBy": "agent"
}
},
"updatedAt": "2026-04-11T23:07:10.523Z"
} |
…nitization Addresses unresolved review threads for lane config env override, command-prefix escaping, commands_run redaction, trigger query hardening, and test isolation improvements.
Resolve remaining CodeRabbit findings by reducing test coupling, hardening MCP response parsing, adding debug logging for entity observation fetch failures, escaping XML in test payload construction, and aligning wording/UI labels with snapshot behavior. Reviewer comment ids: coderabbit-nitpick-c839c8da-39, coderabbit-nitpick-55d37369-370, coderabbit-nitpick-d5dafde0-230, coderabbit-nitpick-6c36cc5c-1889, PRRC_kwDORY-ek8625tfQ, PRRC_kwDORY-ek8625tfS Thread ids: coderabbit-nitpick-c839c8da-39, coderabbit-nitpick-55d37369-370, coderabbit-nitpick-d5dafde0-230, coderabbit-nitpick-6c36cc5c-1889, PRRT_kwDORY-ek856Uw-D, PRRT_kwDORY-ek856Uw-F
There was a problem hiding this comment.
Actionable comments posted: 4
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)
1009-1043:⚠️ Potential issue | 🟠 MajorСанитизируйте
commands_runи в raw-scan путях.
StoreObservationиtoModelObservationредактируют команды, но оба raw-query path'а (SearchObservationsFTSScoredиscanObservation) читаютcommands_runкак есть. Для старых строк или данных, записанных в обход store, это снова открывает утечку secret-ов через search/recall ответы.Also applies to: 1508-1539
🤖 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 1009 - 1043, The raw-scan code paths (SearchObservationsFTSScored and scanObservation) currently unmarshal commands_run into obs.CommandsRun without sanitizing, which can leak secrets; update both raw-scan locations to: unmarshal commandsRunJSON into obs.CommandsRun and then apply the same command-sanitization used by StoreObservation and toModelObservation (i.e., call the existing sanitization routine those functions use) so stored/legacy rows are normalized before being returned; make the identical change in the other raw-scan block referenced (the 1508-1539 region).
♻️ Duplicate comments (3)
internal/worker/handlers_ingest.go (1)
219-226:⚠️ Potential issue | 🟠 MajorНе продолжайте ingest как успешный, если supersede не сработал.
После
StoreObservationновый observation уже записан. ЕслиMarkAsSupersededпадает, старый contradictory row остаётся active, новый тоже остаётся active, а handler всё равно отвечает202 Accepted. Здесь нужен error/compensation path, а не тихий success flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_ingest.go` around lines 219 - 226, The handler currently continues and returns 202 even if s.observationStore.MarkAsSuperseded fails, leaving both old and new observations active; change the flow so that when config.Get().StorePathSupersessionEnabled is true and s.observationStore.MarkAsSuperseded(r.Context(), ingestDedupResult.ExistingID) returns an error you abort the ingest and surface an error response instead of treating it as success, and perform a compensation/rollback for the already-created observation from StoreObservation (e.g., call the store's delete/undo method for the new observation ID or otherwise revert the write) before returning the error; update the handler to return a non-202 error and include the original error in the log/response so callers know the supersede failed.internal/worker/trigger_matcher.go (1)
117-133:⚠️ Potential issue | 🟠 MajorФильтрация после unscoped
Limitвсё ещё теряет project-local matches.
GetObservationsByFileрежет выборку доlimitдо всякой project-фильтрации. Для общих путей вродеREADME.mdtop-N легко забьётся чужими проектами, а локальные/глобальные observations отфильтруются уже после этого и вернётся ложный пустой результат. Лучше делать project/global scope прямо на стороне store-запроса.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/trigger_matcher.go` around lines 117 - 133, The current code calls s.observationStore.GetObservationsByFile(...) unscoped and then filters by project in-memory, which lets the store's limit drop project-local matches; change this so scoping happens in the store query itself: add or use a store method that accepts a project (e.g., GetObservationsByFileWithProject or extend GetObservationsByFile to accept a project/global flag), call that from the TriggerMatcher logic instead of post-filtering, and ensure the store applies the limit after scoping so the returned slice already contains only the requested project's observations (remove the in-function scoped post-filtering loop).internal/db/gorm/observation_store.go (1)
578-584:⚠️ Potential issue | 🟠 MajorLiteral-prefix экранирование для
LIKEвсё ещё сломано.В parameter value сейчас попадают двойные
\(\\%,\\_), поэтому%и_не становятся literal-символами, а одиночный\вообще не экранируется. ДляLIKE ... ESCAPE '\'здесь нужны\%,\_и\\в самом значении pattern.Как быстро проверить текущую форму pattern
#!/bin/bash python - <<'PY' samples = ["printf '%s\n'", "echo foo_bar", r"type C:\Temp\file.txt"] for s in samples: out = s.replace(r"\\", r"\\\\").replace("%", r"\\%").replace("_", r"\\_") print(f"{s!r} -> {out!r}") PYОжидаемый сигнал: перед
%и_в результате окажутся два\, а не один.🤖 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 578 - 584, The current replacer produces the wrong number of backslashes for SQL LIKE ESCAPE '\' — fix by first replacing every backslash in command with two backslashes, then prefixing a single backslash before any % and _ characters, and finally append "%" for the prefix search; update the code around escaper and pattern (replace the strings.NewReplacer usage and pattern construction) so you perform these two sequential ReplaceAll steps on strings.TrimSpace(command) before using pattern in the s.db query (keep the SAME query filter uses: projectScopeFilter, activeObservationFilter, importanceOrdering and the EXISTS ... LIKE ... ESCAPE '\\' clause).
🤖 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/worker/sdk/processor.go`:
- Around line 536-552: The syncObservationFunc call in processor.go is executed
synchronously after observationStore.UpdateObservation (in function/method using
p.observationStore.UpdateObservation and variable updated), which can block the
hot path; change this to dispatch the sync to the same bounded
worker/worker-pool path used by create-path (or at minimum run
p.syncObservationFunc(updated) in a non-blocking goroutine/submit-to-worker
call) so the UpdateObservation return and main processing are not delayed by
vector-sync; ensure you only dispatch when updated != nil and
p.syncObservationFunc != nil, and preserve error/context handling semantics.
- Around line 697-707: StoreObservation is inserting the replacement row before
MarkAsSuperseded and on failure you just continue, leaving the DB in an
inconsistent half-updated state; change the flow to ensure atomicity by either
performing both operations inside a single DB transaction (use a transaction API
on p.observationStore or a BeginTx/WithTx helper to call StoreObservation and
MarkAsSuperseded in one TX and commit/rollback as a unit) or, if transactions
aren't available, delete the newly inserted row (use the returned id from
StoreObservation) and return/propagate an error instead of continue so the
caller sees the failure; update the code paths around
p.observationStore.StoreObservation, mergedObs handling and
p.observationStore.MarkAsSuperseded to implement one of these fixes and ensure
you log and propagate the error rather than skipping broadcast/vector sync.
In `@plugin/engram/hooks/pre-tool-use.js`:
- Around line 29-37: shouldSkipPath fails for Windows because windowsTempMarker
is built as 'T\e\m\p' and the check is case-sensitive; update shouldSkipPath to
normalize the incoming filePath (e.g., toLowerCase()) and check for a proper
Windows temp boundary (e.g., include '\\temp\\' or use a case-insensitive regex
like /[\\/]temp[\\/]/i) instead of the current join-built token; keep the
unixTempMarker and dependencyDir checks but perform them against the normalized
path (or use path.sep-aware checks) so C:\...\Temp\... and c:\...\temp\... are
correctly filtered.
- Around line 177-183: В блоке, где вы формируете triggerInput для Read
(toolName === 'Read'), вы всегда принудительно ставите read_counts[filePath] =
3, из‑за чего threshold repeated-read отключается; исправьте так, чтобы не
перезаписывать существующие счётчики из toolInput/сессии — только установить 3
если для этого filePath нет уже значения (т.е. merge/assign только тогда, когда
triggerInput.read_counts[filePath] === undefined), оставляя extractFilePath,
triggerInput и вызов fetchTriggerContext без других изменений.
---
Outside diff comments:
In `@internal/db/gorm/observation_store.go`:
- Around line 1009-1043: The raw-scan code paths (SearchObservationsFTSScored
and scanObservation) currently unmarshal commands_run into obs.CommandsRun
without sanitizing, which can leak secrets; update both raw-scan locations to:
unmarshal commandsRunJSON into obs.CommandsRun and then apply the same
command-sanitization used by StoreObservation and toModelObservation (i.e., call
the existing sanitization routine those functions use) so stored/legacy rows are
normalized before being returned; make the identical change in the other
raw-scan block referenced (the 1508-1539 region).
---
Duplicate comments:
In `@internal/db/gorm/observation_store.go`:
- Around line 578-584: The current replacer produces the wrong number of
backslashes for SQL LIKE ESCAPE '\' — fix by first replacing every backslash in
command with two backslashes, then prefixing a single backslash before any % and
_ characters, and finally append "%" for the prefix search; update the code
around escaper and pattern (replace the strings.NewReplacer usage and pattern
construction) so you perform these two sequential ReplaceAll steps on
strings.TrimSpace(command) before using pattern in the s.db query (keep the SAME
query filter uses: projectScopeFilter, activeObservationFilter,
importanceOrdering and the EXISTS ... LIKE ... ESCAPE '\\' clause).
In `@internal/worker/handlers_ingest.go`:
- Around line 219-226: The handler currently continues and returns 202 even if
s.observationStore.MarkAsSuperseded fails, leaving both old and new observations
active; change the flow so that when config.Get().StorePathSupersessionEnabled
is true and s.observationStore.MarkAsSuperseded(r.Context(),
ingestDedupResult.ExistingID) returns an error you abort the ingest and surface
an error response instead of treating it as success, and perform a
compensation/rollback for the already-created observation from StoreObservation
(e.g., call the store's delete/undo method for the new observation ID or
otherwise revert the write) before returning the error; update the handler to
return a non-202 error and include the original error in the log/response so
callers know the supersede failed.
In `@internal/worker/trigger_matcher.go`:
- Around line 117-133: The current code calls
s.observationStore.GetObservationsByFile(...) unscoped and then filters by
project in-memory, which lets the store's limit drop project-local matches;
change this so scoping happens in the store query itself: add or use a store
method that accepts a project (e.g., GetObservationsByFileWithProject or extend
GetObservationsByFile to accept a project/global flag), call that from the
TriggerMatcher logic instead of post-filtering, and ensure the store applies the
limit after scoping so the returned slice already contains only the requested
project's observations (remove the in-function scoped post-filtering loop).
🪄 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: e08a3f7c-6395-48a2-a0ba-5c9144da7922
📒 Files selected for processing (16)
internal/config/config.gointernal/db/gorm/observation_store.gointernal/learning/merge.gointernal/learning/merge_test.gointernal/mcp/tools_memory.gointernal/vector/pgvector/client.gointernal/vector/pgvector/client_sql_test.gointernal/worker/handlers_ingest.gointernal/worker/retrieval.gointernal/worker/retrieval_test.gointernal/worker/sdk/processor.gointernal/worker/trigger_matcher.gointernal/worker/trigger_matcher_bash_test.gointernal/worker/trigger_matcher_test.goplugin/engram/hooks/pre-tool-use.jsplugin/engram/hooks/pre-tool-use.test.js
✅ Files skipped from review due to trivial changes (3)
- plugin/engram/hooks/pre-tool-use.test.js
- internal/config/config.go
- internal/worker/retrieval_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/vector/pgvector/client.go
- internal/vector/pgvector/client_sql_test.go
- internal/learning/merge_test.go
- internal/worker/trigger_matcher_test.go
- internal/worker/retrieval.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/worker/sdk/processor_test.go (2)
1828-1840: Хрупкое сравнение строки системного промпта.Сравнение
systemPrompt == "You decide whether a new observation should be CREATE_NEW, UPDATE, SUPERSEDE, or SKIP. Return JSON only."на строке 1836 точно соответствует промпту изinternal/learning/merge.go:37, но при изменении промпта тест молча начнёт возвращать неверный ответ вместо того, чтобы упасть.Рекомендуется вынести промпт в экспортируемую константу в пакете
learning, чтобы тест использовал её напрямую.♻️ Предлагаемый рефакторинг
В
internal/learning/merge.go:+// MergeSystemPrompt is the system prompt used for merge decisions. +const MergeSystemPrompt = "You decide whether a new observation should be CREATE_NEW, UPDATE, SUPERSEDE, or SKIP. Return JSON only." + func DecideMerge(ctx context.Context, llm LLMClient, newObs *models.Observation, candidates []*models.Observation) (MergeDecision, error) { // ... - systemPrompt := "You decide whether a new observation should be CREATE_NEW, UPDATE, SUPERSEDE, or SKIP. Return JSON only." + systemPrompt := MergeSystemPromptВ тесте:
func (m *testMergeLLMClient) Complete(_ context.Context, systemPrompt, _ string) (string, error) { m.calls++ - if systemPrompt == "You decide whether a new observation should be CREATE_NEW, UPDATE, SUPERSEDE, or SKIP. Return JSON only." { + if systemPrompt == learning.MergeSystemPrompt { return m.mergeResponse, nil } return m.observationXML, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/sdk/processor_test.go` around lines 1828 - 1840, The test uses a fragile literal string comparison in testMergeLLMClient.Complete (comparing systemPrompt to "You decide whether a new observation should be CREATE_NEW, UPDATE, SUPERSEDE, or SKIP. Return JSON only."), so export that prompt as a constant in the learning package (e.g., LearningMergeSystemPrompt or MergeSystemPrompt) in internal/learning/merge.go and update the test to import learning and compare against that constant instead of the hardcoded string; change the test's Complete method to use learning.MergeSystemPrompt to decide which response to return.
2117-2170: Дублирование логики управления переменными окружения и использованиеrequireв defer.Тест корректно проверяет поведение при отключённом
ContradictionDetectionEnabled: действиеSUPERSEDEоткатывается к созданию новой записи без пометки старой как superseded (соответствуетprocessor.go:512-519).Однако:
- Управление
ENGRAM_CONTRADICTION_DETECTION_ENABLED(строки 2119-2131) дублирует паттерн изsetWriteMergeEnabledForTest. Рассмотрите обобщение хелпера.- Использование
require.NoErrorвdefer(строки 2125-2130) имеет ту же проблему — лучше использоватьt.Errorf.♻️ Предлагаемый рефакторинг: обобщённый хелпер для env vars
func setEnvForTest(t *testing.T, key string, value string) { t.Helper() original, hadOriginal := os.LookupEnv(key) if err := os.Setenv(key, value); err != nil { t.Fatalf("failed to set %s: %v", key, err) } _, _, err := config.Reload() if err != nil { t.Fatalf("failed to reload config: %v", err) } t.Cleanup(func() { var restoreErr error if hadOriginal { restoreErr = os.Setenv(key, original) } else { restoreErr = os.Unsetenv(key) } if restoreErr != nil { t.Errorf("failed to restore %s: %v", key, restoreErr) } if _, _, reloadErr := config.Reload(); reloadErr != nil { t.Errorf("failed to reload config: %v", reloadErr) } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/sdk/processor_test.go` around lines 2117 - 2170, The test TestProcessObservation_ContradictionDetectionDisabledKeepsCreateNewPath duplicates env-var setup/teardown logic and uses require.NoError inside a defer; extract a reusable helper (e.g., setEnvForTest) similar to setWriteMergeEnabledForTest to set ENGRAM_CONTRADICTION_DETECTION_ENABLED, call config.Reload(), and register restoration with t.Cleanup instead of a defer, and in cleanup use t.Errorf (or t.Fatalf when failing earlier) rather than require to avoid fatal assertions from deferred code; update the test to call the new helper and remove the manual os.LookupEnv/os.Setenv/config.Reload block and its defer.
🤖 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/worker/retrieval_helpers.go`:
- Around line 85-107: The hook path currently returns raw neighbor IDs while the
graphStore path filters out non-positive IDs and deduplicates; update the branch
that calls s.retrievalHooks.getGraphNeighbors to apply the same normalization:
call s.retrievalHooks.getGraphNeighbors(ctx, seedID, 2, 10), then iterate the
returned slice, skip IDs <= 0, dedupe using a seen map (like the graphStore
branch uses on neighbor.ObsID), and return the normalized []int64; ensure you
reference the hook call (s.retrievalHooks.getGraphNeighbors) and the same
filtering/dedupe logic applied to neighbor.ObsID from s.graphStore.GetNeighbors.
- Around line 215-273: The seed selection is too permissive because entity
titles are added into the same nameSet used to match seeds and the hook branch
can return non-entity observations; update getEntityObservationsBySession
handling to filter results to only models.ObsTypeEntity (apply same filter
currently used for observationStore), and change the nameSet population so it
comes only from prompt tokens and file base names (FilesRead/FilesModified) but
do NOT add obs.Title.String into nameSet when iterating entityObservations; then
keep the existing seedIDs logic that matches obs.Title against nameSet (so an
entity only becomes a seed when its title matches prompt/files, not because its
own title was pre-inserted). Use identifiers: loadLastUserPromptBySession,
s.retrievalHooks.getEntityObservationsBySession,
observationStore.GetObservationsBySession, entityObservations, nameSet,
appendName, seedIDs, and obs.Title.String.
In `@internal/worker/sdk/processor_test.go`:
- Around line 1872-1889: The cleanup function in setWriteMergeEnabledForTest
uses require.NoError inside t.Cleanup which can call t.FailNow() after test
completion; change the cleanup to avoid require and instead use assert.NoError
(from testify) or record errors with t.Errorf/t.Logf inside the anonymous
function used in t.Cleanup to restore the environment and call config.Reload(),
ensuring restoreErr and reloadErr are reported without invoking FailNow. Locate
setWriteMergeEnabledForTest and replace the require.NoError calls inside the
t.Cleanup closure with assert.NoError or conditional t.Errorf so cleanup errors
are reported safely.
---
Nitpick comments:
In `@internal/worker/sdk/processor_test.go`:
- Around line 1828-1840: The test uses a fragile literal string comparison in
testMergeLLMClient.Complete (comparing systemPrompt to "You decide whether a new
observation should be CREATE_NEW, UPDATE, SUPERSEDE, or SKIP. Return JSON
only."), so export that prompt as a constant in the learning package (e.g.,
LearningMergeSystemPrompt or MergeSystemPrompt) in internal/learning/merge.go
and update the test to import learning and compare against that constant instead
of the hardcoded string; change the test's Complete method to use
learning.MergeSystemPrompt to decide which response to return.
- Around line 2117-2170: The test
TestProcessObservation_ContradictionDetectionDisabledKeepsCreateNewPath
duplicates env-var setup/teardown logic and uses require.NoError inside a defer;
extract a reusable helper (e.g., setEnvForTest) similar to
setWriteMergeEnabledForTest to set ENGRAM_CONTRADICTION_DETECTION_ENABLED, call
config.Reload(), and register restoration with t.Cleanup instead of a defer, and
in cleanup use t.Errorf (or t.Fatalf when failing earlier) rather than require
to avoid fatal assertions from deferred code; update the test to call the new
helper and remove the manual os.LookupEnv/os.Setenv/config.Reload block and its
defer.
🪄 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: 727a5b6c-d0b7-4c6e-afc7-3aff3864cc5c
📒 Files selected for processing (6)
README.mdinternal/vector/types_test.gointernal/worker/retrieval_helpers.gointernal/worker/sdk/processor_test.goplugin/openclaw-engram/src/client.tsui/src/views/LearningView.vue
✅ Files skipped from review due to trivial changes (1)
- internal/vector/types_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/src/views/LearningView.vue
- README.md
Address all currently unresolved CodeRabbit threads by hardening write-merge consistency, normalizing retrieval seed/neighbor behavior, and fixing hook trigger/path edge cases with targeted tests. Reviewer comment ids: coderabbit-nitpick-d52599af-1828, coderabbit-nitpick-4d0e6e77-2117, PRRC_kwDORY-ek86256dL, PRRC_kwDORY-ek86256dP, PRRC_kwDORY-ek86256dS, PRRC_kwDORY-ek86256dT, PRRC_kwDORY-ek86257x9, PRRC_kwDORY-ek86257x_, PRRC_kwDORY-ek86257yB Thread ids: coderabbit-nitpick-d52599af-1828, coderabbit-nitpick-4d0e6e77-2117, PRRT_kwDORY-ek856U7gL, PRRT_kwDORY-ek856U7gP, PRRT_kwDORY-ek856U7gS, PRRT_kwDORY-ek856U7gT, PRRT_kwDORY-ek856U8jS, PRRT_kwDORY-ek856U8jU, PRRT_kwDORY-ek856U8jW
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/worker/retrieval_test.go (1)
280-329: Усилите BFS-тест проверкой отбрасывания невалидных neighbor IDСейчас тест проверяет только то, что
scores[42]выставлен. Поскольку вход содержит0и-1, полезно явно зафиксировать, что такие ID не попадают в скоринг/результаты.Предложение (минимальное усиление проверки)
observations, scores, err := service.RetrieveRelevant(context.Background(), "engram", "auth query", RetrievalOptions{MaxResults: 10, SessionID: "session-1"}) require.NoError(t, err) require.Len(t, observations, 1) require.Equal(t, int64(1), observations[0].ID) require.NotZero(t, scores[42]) + require.Zero(t, scores[0], "invalid neighbor id should not be scored") + require.Zero(t, scores[-1], "invalid neighbor id should not be scored")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/retrieval_test.go` around lines 280 - 329, Test TestRetrieveRelevant_InjectGraphBFSEnabled_FusesGraphNeighbors should explicitly assert that invalid neighbor IDs from getGraphNeighbors (0 and -1) are ignored: update the test to check that scores for ID 0 and ID -1 are absent or zero in the returned scores map and that observations slice does not include observations with those IDs (use the existing observations, scores variables from the call to RetrieveRelevant and the mocked getGraphNeighbors behavior to locate where to add assertions).internal/learning/merge.go (1)
63-66: Уточнить prompt для LLM: индекс vs ID.Кандидаты нумеруются индексами
[0],[1], но LLM должен вернуть реальныйtarget_id(например,id=12345). Это может привести к тому, что LLM вернёт индекс вместо ID.📝 Предлагаемое улучшение prompt
for i := 0; i < maxCandidates; i++ { - sb.WriteString(fmt.Sprintf("[%d] %s\n", i, formatMergeObservation(candidates[i]))) + sb.WriteString(fmt.Sprintf("- %s\n", formatMergeObservation(candidates[i]))) } - sb.WriteString("Respond as JSON: {\"action\":\"CREATE_NEW|UPDATE|SUPERSEDE|SKIP\",\"target_id\":number}\n") + sb.WriteString("Respond as JSON: {\"action\":\"CREATE_NEW|UPDATE|SUPERSEDE|SKIP\",\"target_id\":<observation id from candidates>}\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/learning/merge.go` around lines 63 - 66, The prompt currently lists candidates by index using formatMergeObservation(candidates[i]) and then asks "Respond as JSON: {\"action\":\"CREATE_NEW|UPDATE|SUPERSEDE|SKIP\",\"target_id\":number}", which risks the LLM returning the displayed index instead of the real record ID; update the prompt string (the line that writes "Respond as JSON: ...") to explicitly state that "target_id" must be the actual record ID (e.g., id=12345) and not the displayed candidate index, and add a short clarifier after the candidate lines (referencing the candidates slice and formatMergeObservation) like "Note: candidate labels [0],[1],... are indexes for reference only — return the database record's actual id in target_id." Ensure the change is applied where sb.WriteString builds the prompt in merge.go.internal/worker/sdk/processor.go (2)
402-418: Рекомендация: добавить capacity hint при создании слайса.
unionStringsсоздаёт слайсmergedбез указания ёмкости, хотя можно оценить примерный размер на основе входных данных.♻️ Предлагаемое улучшение
func unionStrings(parts ...[]string) []string { seen := make(map[string]struct{}) - merged := make([]string, 0) + totalLen := 0 + for _, part := range parts { + totalLen += len(part) + } + merged := make([]string, 0, totalLen) for _, part := range parts { for _, item := range part {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/sdk/processor.go` around lines 402 - 418, The unionStrings function builds merged without a capacity hint causing potential reallocations; compute an estimated capacity by summing len(...) over the input parts, then create merged with make([]string, 0, totalCap) (and optionally pre-size the seen map with make(map[string]struct{}, totalCap)) before the loops so appends and map inserts avoid repeated growth and reduce allocations.
500-503: Ошибка LLM молча игнорируется.При ошибке вызова
learning.DecideMergeфункция возвращаетnil, false, "", nil, не логируя причину. Это затрудняет диагностику проблем с LLM в production.🔧 Предлагаемое улучшение
decision, err := learning.DecideMerge(ctx, p.llmClient, newObs, candidates) if err != nil { + log.Debug().Err(err).Str("project", project).Msg("write-merge: DecideMerge failed, falling back to create") return nil, false, "", nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/sdk/processor.go` around lines 500 - 503, The call to learning.DecideMerge silences errors; instead log the error with context and return it: at the decision,err := learning.DecideMerge(ctx, p.llmClient, newObs, candidates) site, replace the silent return with a log call using the processor's logger (e.g. p.logger or p.processLogger).Include the error and key context (newObs, len(candidates), maybe p.llmClient id) in the log message, then propagate the error by returning it (or wrapping it) instead of returning nil as the error value so the caller can handle it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/learning/merge.go`:
- Around line 63-66: The prompt currently lists candidates by index using
formatMergeObservation(candidates[i]) and then asks "Respond as JSON:
{\"action\":\"CREATE_NEW|UPDATE|SUPERSEDE|SKIP\",\"target_id\":number}", which
risks the LLM returning the displayed index instead of the real record ID;
update the prompt string (the line that writes "Respond as JSON: ...") to
explicitly state that "target_id" must be the actual record ID (e.g., id=12345)
and not the displayed candidate index, and add a short clarifier after the
candidate lines (referencing the candidates slice and formatMergeObservation)
like "Note: candidate labels [0],[1],... are indexes for reference only — return
the database record's actual id in target_id." Ensure the change is applied
where sb.WriteString builds the prompt in merge.go.
In `@internal/worker/retrieval_test.go`:
- Around line 280-329: Test
TestRetrieveRelevant_InjectGraphBFSEnabled_FusesGraphNeighbors should explicitly
assert that invalid neighbor IDs from getGraphNeighbors (0 and -1) are ignored:
update the test to check that scores for ID 0 and ID -1 are absent or zero in
the returned scores map and that observations slice does not include
observations with those IDs (use the existing observations, scores variables
from the call to RetrieveRelevant and the mocked getGraphNeighbors behavior to
locate where to add assertions).
In `@internal/worker/sdk/processor.go`:
- Around line 402-418: The unionStrings function builds merged without a
capacity hint causing potential reallocations; compute an estimated capacity by
summing len(...) over the input parts, then create merged with make([]string, 0,
totalCap) (and optionally pre-size the seen map with make(map[string]struct{},
totalCap)) before the loops so appends and map inserts avoid repeated growth and
reduce allocations.
- Around line 500-503: The call to learning.DecideMerge silences errors; instead
log the error with context and return it: at the decision,err :=
learning.DecideMerge(ctx, p.llmClient, newObs, candidates) site, replace the
silent return with a log call using the processor's logger (e.g. p.logger or
p.processLogger).Include the error and key context (newObs, len(candidates),
maybe p.llmClient id) in the log message, then propagate the error by returning
it (or wrapping it) instead of returning nil as the error value so the caller
can handle it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3af52f18-e5ad-41ce-b8a8-eb22efe9ce85
📒 Files selected for processing (7)
internal/learning/merge.gointernal/worker/retrieval_helpers.gointernal/worker/retrieval_test.gointernal/worker/sdk/processor.gointernal/worker/sdk/processor_test.goplugin/engram/hooks/pre-tool-use.jsplugin/engram/hooks/pre-tool-use.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/worker/sdk/processor_test.go
- internal/worker/retrieval_helpers.go
thebtf
added a commit
that referenced
this pull request
Apr 18, 2026
Integration test TestCredentialDecryptRoundTripAfterMigration in
internal/grpcserver/credential_migration_test.go verifies the core US3
invariant: credentials migrated from `observations` into the dedicated
`credentials` table via migration 090 preserve AES-256-GCM ciphertext
byte-for-byte, and the vault can still decrypt every migrated secret
back to its original plaintext with 100% fidelity.
Why this gate exists:
Migration 090 copies encrypted_secret from observations into
credentials via a SQL SELECT into a BYTEA column. If either end
silently mutated the bytes (encoding, trimming, pad handling),
AES-GCM decrypt would fail authentication-tag check and the
production vault would become unusable. That would lose all 13
prod credentials irrecoverably.
What the test does:
1. Opens a real Postgres + pgvector DB, applies migrations through 090.
2. Creates a real crypto.Vault with a deterministic test key
(not the prod key — the test is scoped to a unique test project
slug to avoid colliding with other data).
3. Encrypts 5 diverse plaintexts via the real vault:
- short ASCII
- empty plaintext (GCM edge case: 0-byte plaintext produces
nonce+tag = 28-byte ciphertext)
- unicode mixed (Cyrillic + CJK + emoji, multi-byte UTF-8)
- long plaintext (~1KB of repeated bytes)
- binary-like (embedded NUL bytes + high-bit bytes, guards
against any future code that reads the secret as a text string
instead of raw bytes)
4. Seeds each plaintext as an observations row with the ciphertext
+ fingerprint + type='credential' + the required NOT NULL columns.
5. Seeds 3 ADVERSARIAL EXCLUDED observations (is_suppressed=true,
is_archived=1, is_superseded=1 — one each). Migration 090 filters
these out via its WHERE clause; if the filters ever get loosened,
one of the assertions in sub-test adversarial_excluded_rows_not_
migrated will fire.
6. Seeds 1 ORPHAN observation encrypted with a different (wrong)
fingerprint to exercise CountWithDifferentFingerprint.
7. Re-runs the exact migration 090 credentials INSERT SQL scoped to
the test project slug (the SQL is copy-pasted verbatim from
internal/db/gorm/migrations.go — drift will fail the test).
8. Reads each credential back via CredentialStore.Get and asserts:
- encrypted_secret bytes == original ciphertext (byte-for-byte)
- encryption_key_fingerprint == vault fingerprint (exact)
- vault.Decrypt(migrated_ciphertext) == original plaintext
9. Three adversarial sub-tests:
- orphan_credential_visible_via_fingerprint_check: verifies the
badFP orphan is visible in List and counted globally by
CountWithDifferentFingerprint(goodFP).
- adversarial_excluded_rows_not_migrated: verifies each of the
three excluded observations (suppressed / archived / superseded)
is NOT present in the credentials table. This is the load-
bearing test for the WHERE clause filters.
- tampered_ciphertext_fails_decrypt: flips one byte in the
migrated ciphertext and verifies AES-GCM authentication
catches the mutation. This guards against "decrypt accepts
anything" — which would make the gate a stub.
Anti-stub proof:
Swap vault.Decrypt body -> return "", nil ⇒ short_ascii / unicode /
long / binary sub-tests all fail (decrypted != plaintext) AND
tampered_ciphertext_fails_decrypt fails (require.Error vs nil).
Test cannot pass with a stubbed decrypt path.
Resource hygiene:
NewStore's *Store is captured and closed via defer gormStore.Close().
A separate direct gorm.Open handle is used for raw INSERT seeds and
the project-scoped migration re-run, explicitly closed via its
sqlDB.Close() defer.
Scope guard:
ONE new file: internal/grpcserver/credential_migration_test.go.
No production code changed. Uses only exported APIs:
crypto.NewVault, crypto.Vault.Encrypt/Decrypt/Fingerprint,
gorm.NewStore, gorm.Config, gorm.Store.Close, gorm.NewCredentialStore,
gorm.CredentialStore.Get/List/CountWithDifferentFingerprint.
Verified:
DATABASE_DSN=postgres://.../engram_f1_test — TestCredentialDecrypt-
RoundTripAfterMigration PASS (1 top-level + 8 sub-tests, ~1.8s wall).
Sub-tests: short_ascii, empty_plaintext, unicode_mixed,
long_plaintext_1kb, binary_like_with_nul_and_high_bytes,
orphan_credential_visible_via_fingerprint_check,
adversarial_excluded_rows_not_migrated, tampered_ciphertext_fails_decrypt.
Does NOT verify (intentional — out of scope for CI F-1 gate):
- The production invariants credential_count=13, fingerprint=
aa78e55cf896508c, mismatch_count=0. Those belong to the
staging dry-run against the real 2026-04-17 pg_dump snapshot,
run out of band by the operator before Commit G lands. F-1 CI
gate proves the PRIMITIVE works (round-trip byte-preserving);
staging dry-run proves the MIGRATION ran correctly on prod data.
Pre-existing test failures (NOT regressions from this commit):
- TestSyncProjectState_* (internal/grpcserver) — introduced in
d324f01 (PR #170, Phase 4) with a minimal schema that conflicts
with the full projects-table migration. Fails on any DB with
migrations 045+ applied.
- TestHandleMemoryTriggers_BashCommand* (internal/worker) — from
fd8ce26 (PR #135, learning-memory-v4) — returns 500 not 200
on fresh pgvector DB. Unrelated to US3.
Ref: .agent/specs/engram-v5-baseline/changes/CR-001-initial-scope/
tasks.md T022 AC 1-5. Addresses two review findings:
(1) NewStore return value leaked pool — fixed via capture+Close.
(2) Exclusion filter coverage gap — fixed via adversarial sub-test.
thebtf
added a commit
that referenced
this pull request
Apr 18, 2026
Two interconnected regressions in GetObservationsByCommandPrefix and its test harness, shipped in PR #135 (fd8ce26, learning-memory-v4). Never passed on a fresh pgvector DB. 1. SQL: ESCAPE '\\\\' in Go double-quoted source produces '\\\\' in the SQL text, which under standard_conforming_strings=on (PostgreSQL 9.1+ default) is a 2-character escape string. PostgreSQL rejects multi-character ESCAPE with SQLSTATE 22025 ('invalid escape string'). Any call to this function against a real Postgres returned 500 from handleMemoryTriggers. 2. Semantics: the function name says 'ByCommandPrefix' — intent is to find observations whose stored commands_run entry is a PREFIX of the incoming query (e.g. stored 'git push --force' matches query 'git push --force origin main'). The old SQL implemented the reverse direction: cmd LIKE 'query%' finds stored values that START WITH the query, which no real case exercises. Fix: use starts_with(?, cmd) — PostgreSQL 11+ native function with clean semantics (no escape hazard, no wildcard ambiguity, both sides treated as literals). The escaper + % suffix are removed as no longer needed. Additionally, internal/worker/trigger_matcher_bash_test.go seeded commands with the narrative prefix 'Executed: ' (e.g. 'Executed: git push --force'), which does not reflect how the Bash tool records commands in production (raw command, no prefix). The test could never pass with a correct prefix matcher because the seed shape mismatched the matcher shape. Fix: strip 'Executed: ' from all 5 seed call sites. Verified (fresh pgvector/pgvector:pg17 with truncated observations): go test -count=1 -run '^TestHandleMemoryTriggers_' ./internal/worker/ — 6 subtests all PASS. TestHandleMemoryTriggers_BashCommandPrefixMatchesTop3Warnings now correctly returns the 3 bugfix+pitfall prefix matches (excluding the discovery-typed row and the non-prefix force-with-lease row).
thebtf
added a commit
that referenced
this pull request
Apr 18, 2026
…ules (PR-A, US3) (#181) * feat(us3): migration 087_credentials + CredentialStore skeleton (Commit A, PR-A) Create dedicated credentials table as additive schema for US3 observations split. Migration ID note: spec tasks.md says 077_credentials, but US1+US2 consumed 083-086 (083 drop orphan joins, 084 Gemini-fix Rollback, 085 drop content_chunks, 086 drop used_vector). US3 migrations therefore shift to 087 onwards (next-free). This commit creates 087; subsequent commits in PR-A will create 088 (memories), 089 (behavioral_rules), 090 (data migration 3-way split). Changes: - internal/db/gorm/migrations.go: add migration 087_credentials CREATE TABLE credentials per spec.md §Data Model (id BIGSERIAL PK, project, key, encrypted_secret BYTEA, encryption_key_fingerprint, scope, version, edited_by, created_at, updated_at, deleted_at, UNIQUE(project, key)). Partial indexes on project and encryption_key_fingerprint WHERE deleted_at IS NULL. Rollback drops indexes + table. - pkg/models/credential.go (new): domain Credential struct, no GORM tags (mirrors pkg/models/observation.go pattern — infra concerns stay in internal/db/gorm). - internal/db/gorm/models.go: add GORM Credential row type with tags matching all columns + TableName() returning "credentials". - internal/db/gorm/credential_store.go (new): CredentialStore with Create, Get, List, Delete (soft-delete via UPDATE deleted_at), CountCredentials, CountWithDifferentFingerprint, DeleteOrphanedByFingerprint. Signatures mirror ObservationStore vault helpers so callers in handlers_vault.go can swap receiver types without touching call sites. Conversion via credentialRowToModel() keeps pkg/models isolated from gorm internals. - internal/db/gorm/credential_store_test.go (new): integration smoke tests skip unless DATABASE_DSN is set (matches migrations_integration_test.go pattern). Exercises Create→Get→List→Count→Delete round-trip against a real pgvector/pgvector:pg17 instance. Anti-stub: replacing any method body with `return nil` causes the round-trip test to fail. Protected invariants (NONE violated by this commit — additive-only): - observations table UNTOUCHED (still has 2775 live rows / 13 credentials) - content table UNTOUCHED - vault ciphertext bytes unchanged - fingerprint aa78e55cf896508c / credential_count 13 / mismatch_count 0 (verified pre-flight 2026-04-17, nothing in this commit touches prod data) Verification: - go build ./... — green - go vet ./internal/db/gorm/... — green - go test ./internal/db/gorm/... -run Credential — 3/3 PASS against pgvector/pgvector:pg17 with full migration chain 001→087 applied fresh - TestMigrationsIntegration failure is pre-existing on main 83bbc62 (stale vector-dim assertion, legacy US2 leftover) — not introduced here Task: T017 Commit A Spec: .agent/specs/engram-v5-baseline/spec.md §Data Model §credentials Plan: .agent/specs/engram-v5-baseline/plan.md §Phase 5 Phase 5 gate: F-1 decrypt round-trip (T022) — not yet * feat(us3): migrations 088_memories + 089_behavioral_rules (Commit B, PR-A) CREATE TABLE only — no Go store code, no data migration, no handlers. Commit C will add memory_store + behavioral_rules_store + gRPC dispatch. Commit D will migrate data from observations. Migration ID shift: spec.md says 078/079; Commit A took 087; next-free IDs are 088 (memories) + 089 (behavioral_rules). Schemas match spec.md §Data Model verbatim (column-for-column verified): MEMORIES (internal/db/gorm/migrations.go migration 088): - id BIGSERIAL PK, project TEXT NOT NULL, content TEXT NOT NULL, tags JSONB DEFAULT '[]', source_agent TEXT, version INT DEFAULT 1, edited_by TEXT, created_at/updated_at TIMESTAMPTZ DEFAULT NOW(), deleted_at TIMESTAMPTZ, search_vector tsvector GENERATED ALWAYS AS STORED (dual-dict english + simple per migration 076 pattern). - Indexes: idx_memories_project_created partial WHERE deleted_at IS NULL, idx_memories_fts GIN, idx_memories_tags GIN. - Rollback drops indexes (reverse order) then table. BEHAVIORAL_RULES (migration 089): - id BIGSERIAL PK, project TEXT (NULLable — NULL = global rule), content TEXT NOT NULL, priority INT DEFAULT 0, version INT DEFAULT 1, edited_by TEXT, created_at/updated_at TIMESTAMPTZ DEFAULT NOW(), deleted_at TIMESTAMPTZ. - Indexes: idx_behavioral_rules_project_priority partial WHERE deleted_at IS NULL, idx_behavioral_rules_global partial WHERE project IS NULL AND deleted_at IS NULL. - Rollback drops indexes (reverse order) then table. NOT carried over from observations: importance_score, relevance_score, effectiveness_*, cited, inject_count — per spec S1 (scoring dropped). No FTS on behavioral_rules — rules are injected unconditionally at session-start, not searched. Protected invariants (NONE violated — additive only): - observations table UNTOUCHED (still 2775 live rows / 13 credentials) - credentials table from Commit A UNTOUCHED - content table UNTOUCHED - vault ciphertext bytes unchanged - fingerprint aa78e55cf896508c / credential_count 13 / mismatch_count 0 Verification: - go build ./... — green - Migration chain 001→089 "all migrations passed" against pgvector/pgvector:pg17 fresh DB - TestCredentialStore_* 3/3 PASS + rotation sub-test + 5 validation sub-tests (regression for Commit A — unchanged) - psql SELECT tablename FROM pg_tables WHERE tablename IN ('credentials','memories','behavioral_rules') — 3 rows - 11 indexes created (pkey + idx_* across 3 tables) - TestMigrationsIntegration vector-dim failure is pre-existing on main 83bbc62 (US2 leftover, not introduced here) Task: T018 Commit B Spec: .agent/specs/engram-v5-baseline/spec.md §Data Model §memories + §behavioral_rules * feat(us3): memory_store + behavioral_rules_store + store_rule/list_rules MCP tools (Commit C, PR-A) Implement CRUD stores for the new static-entity tables (migrations 088_memories, 089_behavioral_rules from Commit B) and wire two new MCP tools (store_rule, list_rules) that read/write through them. Scope boundary respected (per tasks.md T019): - handleStoreMemory / handleRecall STILL route to observations (Commit E switches them) - handlers_vault.go + credential_store.go UNTOUCHED (Commit E scope) - No data migrations touched (Commit D = T020) - No observations dropped (Commit G = T023) FILES CREATED (6 new): - pkg/models/memory.go: domain Memory struct (no GORM tags, Tags []string real slice, search_vector intentionally absent from domain — it's a GENERATED column). - pkg/models/behavioral_rule.go: domain BehavioralRule struct (Project *string for NULLable global-rule semantics; *time.Time pointers for all timestamps). - internal/db/gorm/memory_store.go: MemoryStore with Create/Get/List/Update/Delete. Create returns a new *models.Memory (does not mutate input — per Commit A code-review HIGH-1 immutability rule). Delete is HARD (Unscoped) per Commit A code-review HIGH-2 rotation analysis — deleted_at column remains in schema for future use but is not populated. Update uses gorm.Expr("version + 1") + re-fetch. Empty-arg guards on Get/List. - internal/db/gorm/memory_store_test.go: 3 integration tests skip without DATABASE_DSN. Covers full round-trip (Create→Get→Update→List→Delete), validation errors (nil/empty/empty), and per-project filtering (2 projects, 3 memories, asserts isolation). - internal/db/gorm/behavioral_rules_store.go: BehavioralRulesStore with same CRUD shape + global-rule semantics: List(nil, limit) returns only rows where project IS NULL; List(&proj, limit) returns rows WHERE project = proj OR project IS NULL (global rules always apply). - internal/db/gorm/behavioral_rules_store_test.go: 3 integration tests. TestBehavioralRulesStore_List_GlobalRulesAlwaysIncluded validates the union semantics: 1 global + 1 project-scoped row; List(&"p1") returns both, List(nil) returns only the global. - internal/mcp/tools_rules.go: handleStoreRule + handleListRules. Both validate inputs, wrap errors with %w, return JSON responses with RFC3339 timestamps. handleStoreRule requires content, defaults priority to 0, project optional (nil = global). handleListRules bounds limit 1-500 (default 50), project optional. FILES MODIFIED (2): - internal/db/gorm/models.go: +38 lines appending GORM row structs for Memory (Tags via models.JSONStringArray matching existing JSONB convention; deleted_at *time.Time; TableName returns "memories") and BehavioralRule (Project *string NULLable; TableName returns "behavioral_rules"). - internal/mcp/server.go: +47 lines. * Server struct: 2 new fields memoryStore + behavioralRulesStore after issueStore. * 2 new setters SetMemoryStore + SetBehavioralRulesStore (mirrors SetIssueStore pattern — called by worker bootstrap after async init). * handleToolsList: 2 new Tool entries (store_rule, list_rules) at tierUseful. * callTool switch: 2 new cases routing to tools_rules.go handlers (after merge_observations, before get_observation). - internal/worker/service.go: +9 lines. * Construct memoryStore and behavioralRulesStore via gorm.New*Store(store) alongside reasoningStore. * Wire both into mcpServer via the new setters, alongside SetReasoningStore + SetIssueStore (consistent bootstrap order). Protected invariants (NONE violated — additive only, no observation writes): - observations table UNTOUCHED (still 2775 live rows / 13 credentials) - credentials table UNTOUCHED - memories + behavioral_rules tables gain functional CRUD (no production data yet) - content table UNTOUCHED - vault ciphertext bytes unchanged - fingerprint aa78e55cf896508c / credential_count 13 / mismatch_count 0 Verification: - go build ./... — green - go vet ./internal/db/gorm/... ./internal/mcp/... — clean - go test ./internal/db/gorm/... -run "Memory|BehavioralRules|Credential" -v against pgvector/pgvector:pg17 fresh DB: PASS TestBehavioralRulesStore_CreateGetUpdateListDelete (65s — includes migration chain) PASS TestBehavioralRulesStore_Create_ValidationErrors (2 sub-tests) PASS TestBehavioralRulesStore_List_GlobalRulesAlwaysIncluded PASS TestCredentialStore_CreateGetCountDelete (regression preserved) PASS TestCredentialStore_DeleteOrphanedByFingerprint (regression preserved) PASS TestCredentialStore_Create_ValidationErrors (5 sub-tests — regression) PASS TestMemoryStore_CreateGetUpdateListDelete PASS TestMemoryStore_Create_ValidationErrors (3 sub-tests) PASS TestMemoryStore_List_FiltersByProject TOTAL: 9 top-level + 10 sub-tests, all green, ~78s incl. migration chain. Task: T019 Commit C Spec: .agent/specs/engram-v5-baseline/spec.md §Data Model + §FR-4 * feat(us3): migration 090_observations_to_static_entities (Commit D, PR-A) 3-way data split: observations → credentials + behavioral_rules + memories. Ciphertext preserved byte-for-byte; DO-block enforces 2 invariants. Migration ID shift: plan.md originally drafted this as 080_...; actual next-free is 090 (US1+US2 consumed 083-086; Commit A=087, Commit B=088+089). Plan.md column mapping amended 2026-04-18 (see plan.md §Amendment 2026-04-18) — the original draft assumed columns that do not exist on observations: - content (creds) → title (per ObservationStore.GetCredential) - content (mem/rul) → COALESCE(NULLIF(TRIM(narrative),''), title, '') - always_inject col → concepts @> ["always-inject"]::jsonb - creation_path fil → dropped (migrate ALL live non-credential non-always-inject) - tags → concepts (JSONStringArray stored jsonb) - source_agent → agent_source - updated_at → reuse created_at (observations has no updated_at) - timestamp cast → TO_TIMESTAMP(created_at_epoch) — unambiguous from int64 - priority (rules) → derived from importance_score (≥0.8→10, ≥0.5→5, else 0) - live filter → is_suppressed=false AND is_archived=0 AND is_superseded=0 Three INSERTs in order: 1. credentials — type='credential' rows with encrypted_secret IS NOT NULL AND encryption_key_fingerprint IS NOT NULL AND title IS NOT NULL AND live. Ciphertext + fingerprint bytes preserved verbatim (no re-encryption). 2. behavioral_rules — observations where concepts @> '["always-inject"]'::jsonb AND type != 'credential' AND live AND has content. priority from importance. 3. memories — remaining live non-credential non-always-inject observations with non-empty content. tags = concepts verbatim. project NULL → '' fallback. Sanity check DO block (BOTH invariants active tripwires): - (a) dst_count < src_count/2 → RAISE EXCEPTION - (b) credentials.count != observations WHERE type='credential' live count → RAISE EXCEPTION — every vault credential MUST migrate byte-for-byte Rollback: TRUNCATE the 3 static tables in reverse order (memories → behavioral_rules → credentials). Forward migration only READS observations, so rollback == undo the data copy. Schema remains (migrations 087-089 Rollback handles schema). Protected invariants (NONE violated by this commit — observations UNTOUCHED): - observations table unchanged (2775 live / 13 credentials in production) - credentials/memories/behavioral_rules tables GAIN data; no lossy transform - vault ciphertext bytes preserved verbatim - production fingerprint aa78e55cf896508c / count 13 / mismatch 0 Verification: - go build ./... — green - go vet ./internal/db/gorm/... — clean - Migration chain 001→090 "all migrations passed" on empty pgvector/pgvector:pg17 - Credential/Memory/BehavioralRules tests 9 top-level + 10 sub-tests — all green - Synthetic-data test: seeded 6 observations (5 live + 1 archived), ran migration logic via psql; got credentials=2 memories=2 behavioral_rules=1 (archived row correctly excluded); DO block NOTICE fired with correct counts - Invariant (b) tripwire confirmed: DELETE 1 credential row → RAISE EXCEPTION "credential invariant FAIL cred=1 cred_live=2" - Invariant (a) tripwire confirmed: TRUNCATE all static tables → RAISE EXCEPTION "sanity FAIL dst=0 src=5" (fires first since order is a → b) - TestMigrationsIntegration vector-dim failure remains pre-existing (US2 leftover, not introduced here) Next: Commit E (T021) switches handlers (handleStoreMemory/handleRecall) to use the new stores; current commit leaves handlers routed to observations still. After Commit E comes Commit F-1 (HARD GATE decrypt round-trip test) — BLOCKER for PR-B drop migrations (commits G+H) per spec §FR-9 + §NFR-7. Task: T020 Commit D Spec: .agent/specs/engram-v5-baseline/spec.md §FR-4 + plan.md §Phase 5 (Amendment 2026-04-18) * feat(us3): switch vault handlers to credentialStore + add /api/memories routes (Commit E, PR-A) Route vault HTTP handlers through the new credentialStore, expose a new set of /api/memories HTTP endpoints backed by memoryStore, and wire all three static- entity stores (credentialStore + memoryStore + behavioralRulesStore) as fields on the worker Service struct. Dual-read preserved: this commit DOES NOT touch handleStoreMemory / handleRecall inside internal/mcp - those still route to observationStore. The actual MCP handler switch happens in Commit G (post Commit F-1 decrypt round-trip gate). Files changed: - internal/worker/service.go: 3 new fields on Service struct (credentialStore, memoryStore, behavioralRulesStore); initializeAsync constructs credentialStore alongside existing memoryStore + behavioralRulesStore (now promoted from locals to fields); initMu block wires all 3 pointers; setupRoutes registers POST/GET/DELETE /api/memories routes inside the authenticated router group. - internal/worker/handlers_vault.go: handleVaultStatus switched to credentialStore.CountCredentials + .CountWithDifferentFingerprint; handleDeleteOrphanedCredentials switched to credentialStore.DeleteOrphanedByFingerprint. Signatures adapted to new method names on CredentialStore. - internal/worker/handlers_memories.go (NEW): thin HTTP wrappers over memoryStore. handleStoreMemoryExplicit validates project+content non-empty, returns 201 on success, 503 if store not initialised. handleListMemories requires project query param, defaults limit 50, returns [] not null. handleDeleteMemoryByID parses chi URL param, maps gorm.ErrRecordNotFound to 404. - internal/worker/handlers_memories_test.go (NEW): 4 integration tests (skip without DATABASE_DSN) - RoundTrip (POST+GET), ValidationErrors (2 sub-tests for empty project + empty content), DeleteByID RoundTrip (POST+DELETE+GET returns empty), DeleteByID NotFound (404 on missing id). - .gitignore: add .tmp_gocache/ and .tmp_gomodcache/ underscore variants (GoCache and GoModCache directories use either hyphen or underscore depending on tool version). Scope boundary respected: - handleStoreMemory / handleRecall in internal/mcp UNTOUCHED (dual-read preserved) - observationStore.CountCredentials / DeleteOrphanedCredentials methods still exist - only CALLERS migrated. Commit G removes the methods when observations drops. - No migration changes (087-090 frozen) - No observations data writes from this commit Protected invariants (NONE violated - additive + caller redirection only): - observations table UNTOUCHED (dual-read still works via MCP store/recall) - credentials / memories / behavioral_rules schemas UNTOUCHED - Vault ciphertext bytes unchanged - Production fingerprint aa78e55cf896508c / count 13 / mismatch 0 preserved - vault handlers now read the same 13 credentials from the credentials table populated by migration 090 instead of observations Verification: - go build ./... - green - go vet ./internal/worker/... ./internal/mcp/... ./internal/db/gorm/... - clean - go test ./internal/worker/ -run "HandleStoreMemoryExplicit|HandleDeleteMemoryByID" - all 4 PASS + 2 sub-tests - go test ./internal/db/gorm/ -run "Credential|Memory|BehavioralRules" - 9 top-level + 10 sub-tests PASS (regression) Task: T021 Commit E Spec: .agent/specs/engram-v5-baseline/spec.md FR-4 + plan.md Phase 5 * chore: ignore .cache/ directory (Go test cache artifacts) * feat(us3): Commit F-1 — DECRYPT ROUND-TRIP HARD GATE (T022) Integration test TestCredentialDecryptRoundTripAfterMigration in internal/grpcserver/credential_migration_test.go verifies the core US3 invariant: credentials migrated from `observations` into the dedicated `credentials` table via migration 090 preserve AES-256-GCM ciphertext byte-for-byte, and the vault can still decrypt every migrated secret back to its original plaintext with 100% fidelity. Why this gate exists: Migration 090 copies encrypted_secret from observations into credentials via a SQL SELECT into a BYTEA column. If either end silently mutated the bytes (encoding, trimming, pad handling), AES-GCM decrypt would fail authentication-tag check and the production vault would become unusable. That would lose all 13 prod credentials irrecoverably. What the test does: 1. Opens a real Postgres + pgvector DB, applies migrations through 090. 2. Creates a real crypto.Vault with a deterministic test key (not the prod key — the test is scoped to a unique test project slug to avoid colliding with other data). 3. Encrypts 5 diverse plaintexts via the real vault: - short ASCII - empty plaintext (GCM edge case: 0-byte plaintext produces nonce+tag = 28-byte ciphertext) - unicode mixed (Cyrillic + CJK + emoji, multi-byte UTF-8) - long plaintext (~1KB of repeated bytes) - binary-like (embedded NUL bytes + high-bit bytes, guards against any future code that reads the secret as a text string instead of raw bytes) 4. Seeds each plaintext as an observations row with the ciphertext + fingerprint + type='credential' + the required NOT NULL columns. 5. Seeds 3 ADVERSARIAL EXCLUDED observations (is_suppressed=true, is_archived=1, is_superseded=1 — one each). Migration 090 filters these out via its WHERE clause; if the filters ever get loosened, one of the assertions in sub-test adversarial_excluded_rows_not_ migrated will fire. 6. Seeds 1 ORPHAN observation encrypted with a different (wrong) fingerprint to exercise CountWithDifferentFingerprint. 7. Re-runs the exact migration 090 credentials INSERT SQL scoped to the test project slug (the SQL is copy-pasted verbatim from internal/db/gorm/migrations.go — drift will fail the test). 8. Reads each credential back via CredentialStore.Get and asserts: - encrypted_secret bytes == original ciphertext (byte-for-byte) - encryption_key_fingerprint == vault fingerprint (exact) - vault.Decrypt(migrated_ciphertext) == original plaintext 9. Three adversarial sub-tests: - orphan_credential_visible_via_fingerprint_check: verifies the badFP orphan is visible in List and counted globally by CountWithDifferentFingerprint(goodFP). - adversarial_excluded_rows_not_migrated: verifies each of the three excluded observations (suppressed / archived / superseded) is NOT present in the credentials table. This is the load- bearing test for the WHERE clause filters. - tampered_ciphertext_fails_decrypt: flips one byte in the migrated ciphertext and verifies AES-GCM authentication catches the mutation. This guards against "decrypt accepts anything" — which would make the gate a stub. Anti-stub proof: Swap vault.Decrypt body -> return "", nil ⇒ short_ascii / unicode / long / binary sub-tests all fail (decrypted != plaintext) AND tampered_ciphertext_fails_decrypt fails (require.Error vs nil). Test cannot pass with a stubbed decrypt path. Resource hygiene: NewStore's *Store is captured and closed via defer gormStore.Close(). A separate direct gorm.Open handle is used for raw INSERT seeds and the project-scoped migration re-run, explicitly closed via its sqlDB.Close() defer. Scope guard: ONE new file: internal/grpcserver/credential_migration_test.go. No production code changed. Uses only exported APIs: crypto.NewVault, crypto.Vault.Encrypt/Decrypt/Fingerprint, gorm.NewStore, gorm.Config, gorm.Store.Close, gorm.NewCredentialStore, gorm.CredentialStore.Get/List/CountWithDifferentFingerprint. Verified: DATABASE_DSN=postgres://.../engram_f1_test — TestCredentialDecrypt- RoundTripAfterMigration PASS (1 top-level + 8 sub-tests, ~1.8s wall). Sub-tests: short_ascii, empty_plaintext, unicode_mixed, long_plaintext_1kb, binary_like_with_nul_and_high_bytes, orphan_credential_visible_via_fingerprint_check, adversarial_excluded_rows_not_migrated, tampered_ciphertext_fails_decrypt. Does NOT verify (intentional — out of scope for CI F-1 gate): - The production invariants credential_count=13, fingerprint= aa78e55cf896508c, mismatch_count=0. Those belong to the staging dry-run against the real 2026-04-17 pg_dump snapshot, run out of band by the operator before Commit G lands. F-1 CI gate proves the PRIMITIVE works (round-trip byte-preserving); staging dry-run proves the MIGRATION ran correctly on prod data. Pre-existing test failures (NOT regressions from this commit): - TestSyncProjectState_* (internal/grpcserver) — introduced in d324f01 (PR #170, Phase 4) with a minimal schema that conflicts with the full projects-table migration. Fails on any DB with migrations 045+ applied. - TestHandleMemoryTriggers_BashCommand* (internal/worker) — from fd8ce26 (PR #135, learning-memory-v4) — returns 500 not 200 on fresh pgvector DB. Unrelated to US3. Ref: .agent/specs/engram-v5-baseline/changes/CR-001-initial-scope/ tasks.md T022 AC 1-5. Addresses two review findings: (1) NewStore return value leaked pool — fixed via capture+Close. (2) Exclusion filter coverage gap — fixed via adversarial sub-test. * fix(grpcserver): SyncProjectState pq.Array + heartbeat precision Two pre-existing regressions in internal/grpcserver/sync_project_state.go and its test, shipped since PR #170 (d324f01, 2026-04-18 gRPC Phase 4): 1. Raw(...ANY(?)..., localIDs) and Exec(...ANY(?)..., now, localIDs) passed []string directly. The gorm postgres driver does NOT encode []string as a PostgreSQL array for positional params, so the query failed with ERROR: malformed array literal (SQLSTATE 22P02). Fix: wrap localIDs with pq.Array() at both call sites. Mirrors the working pattern in internal/sessions/store.go:189. 2. TestSyncProjectState_HeartbeatUpdated flaked on Windows: Go's time.Now().UTC() has 100-nanosecond resolution from the monotonic clock; PostgreSQL timestamptz stores microsecond precision. When the captured 'before' fraction was in the same microsecond bucket as the server's UPDATE timestamp, the round-tripped newHB was strictly less-than 'before' and the assertion fired. Fix: Truncate(time.Microsecond) on 'before' so the comparison operates at the precision the DB stores. Verified: go test -count=3 -run ^TestSyncProjectState ./internal/grpcserver/ — 5 tests x 3 iterations = 15 passes, none flaked. * fix(gorm): command prefix matcher + correct test seed format Two interconnected regressions in GetObservationsByCommandPrefix and its test harness, shipped in PR #135 (fd8ce26, learning-memory-v4). Never passed on a fresh pgvector DB. 1. SQL: ESCAPE '\\\\' in Go double-quoted source produces '\\\\' in the SQL text, which under standard_conforming_strings=on (PostgreSQL 9.1+ default) is a 2-character escape string. PostgreSQL rejects multi-character ESCAPE with SQLSTATE 22025 ('invalid escape string'). Any call to this function against a real Postgres returned 500 from handleMemoryTriggers. 2. Semantics: the function name says 'ByCommandPrefix' — intent is to find observations whose stored commands_run entry is a PREFIX of the incoming query (e.g. stored 'git push --force' matches query 'git push --force origin main'). The old SQL implemented the reverse direction: cmd LIKE 'query%' finds stored values that START WITH the query, which no real case exercises. Fix: use starts_with(?, cmd) — PostgreSQL 11+ native function with clean semantics (no escape hazard, no wildcard ambiguity, both sides treated as literals). The escaper + % suffix are removed as no longer needed. Additionally, internal/worker/trigger_matcher_bash_test.go seeded commands with the narrative prefix 'Executed: ' (e.g. 'Executed: git push --force'), which does not reflect how the Bash tool records commands in production (raw command, no prefix). The test could never pass with a correct prefix matcher because the seed shape mismatched the matcher shape. Fix: strip 'Executed: ' from all 5 seed call sites. Verified (fresh pgvector/pgvector:pg17 with truncated observations): go test -count=1 -run '^TestHandleMemoryTriggers_' ./internal/worker/ — 6 subtests all PASS. TestHandleMemoryTriggers_BashCommandPrefixMatchesTop3Warnings now correctly returns the 3 bugfix+pitfall prefix matches (excluding the discovery-typed row and the non-prefix force-with-lease row). * fix(gorm): migration 090 epoch ms + scope column + safe rollback + store hardening Addresses 6 review findings across 3 files: migrations.go (3 CRITICAL, 1 MAJOR, 1 MAJOR): - TO_TIMESTAMP(created_at_epoch) was interpreting milliseconds as seconds, producing year-56247 dates. Divided by 1000.0 in all three INSERT sites (credentials, behavioral_rules, memories). - credentials INSERT now copies 'scope' column via COALESCE(NULLIF(scope,''),'project'); schema has the column, legacy observations carry it, previous SELECT dropped it silently. - Migration 090 rollback previously TRUNCATEd the 3 tables, which would wipe data created AFTER the forward migration. Replaced with an explicit error instructing pg_restore from the 2026-04-17 snapshot per rollback-procedure.md §3b. observation_store.go (LOW): - Documentation note: starts_with() is PostgreSQL 9.1+ (gemini reviewer claimed 17+ — verified against release notes). engram targets PG 17 in all live deployments; no portability concern for current scope. credential_store.go (MEDIUM, LOW, MEDIUM): - Create: capture a single 'now' up-front so CreatedAt==UpdatedAt deterministically; copy the secret bytes into a defensive slice so subsequent zeroize/reuse by the caller cannot alias the row. - Delete: added 'deleted_at IS NULL' WHERE predicate so soft-deleted rows remain soft-deleted (was possible to hard-delete a tombstone and return success, instead of gorm.ErrRecordNotFound). - Preserve hard-delete for the rotation case (UNIQUE(project,key) constraint + documented design note). Verified: go build + go vet clean; TestCredentialDecryptRoundTripAfterMigration + TestCredentialStore_* all green on pgvector/pgvector:pg17. * fix(stores): break pointer aliasing in BehavioralRules.Create Copy rule.Project into row.Project by value so the returned model does not share a backing string with the caller's variable. If the caller mutates its original input after Create returns, the stored model should not change — matches the API contract implied by 'Create returns the persisted row'. * fix(handlers): memory limit cap, error disclosure, global-scope credential guards Addresses 3 MAJOR + 2 MEDIUM review findings in HTTP handlers: handlers_memories.go: - handleListMemories now caps 'limit' at 500 and rejects non-positive values with 400. Prevents an adversarial or misconfigured client from triggering an unbounded SELECT and multi-MB JSON response. - 500 responses no longer include err.Error() verbatim — both store + request failures now emit stable messages; full error stays in the server log (information-leak mitigation). handlers_vault.go: - handleStoreCredential rejects scope='global' with 400 until a schema migration admits project-less credentials (credentials.project is NOT NULL); previously the write would either fail the NOT NULL constraint or create a row that no READ path could retrieve (LIST, GET, DELETE all require project). - handleGetCredential and handleDeleteCredential now require project to be non-empty; returning 400 instead of silently returning [] / ErrRecordNotFound for empty-project requests. - 500 responses no longer leak err.Error() — stable client message, full error in log. * fix(mcp,test,ci): guard store_rule/list_rules on nil, sync F-1 to migration 090 schema, gitignore cleanup Addresses 2 MEDIUM + 1 MAJOR review findings + infra hygiene: internal/mcp/server.go (MEDIUM): - store_rule and list_rules MCP tools are now only registered when s.behavioralRulesStore != nil. Previously tools/list always advertised them even though the handlers returned 503 at dispatch time — clients would see the tool, invoke it, and get an error. Same conditional pattern as versionedDocumentStore elsewhere in this file. internal/grpcserver/credential_migration_test.go (MAJOR — F-1 sync): - Test SQL updated to match migration 090's corrected form: scope column added to the INSERT list, TO_TIMESTAMP divided by 1000.0. Without this sync the HARD GATE would pass against stale SQL while production would use the fixed SQL — defeating the purpose of the gate. The test still copy-maintains the SQL verbatim; a drift detection mechanism is out of scope for this PR and noted for future work. - Seed timestamps now use UnixMilli() to match observations schema convention (created_at_epoch is milliseconds per models.go). .gitignore: - Deduplicated Go cache patterns (collapsed hyphen/underscore variants into one), moved .cache/ into the dedicated Go cache section, removed stray duplicates. Verified: all target tests green on pgvector/pgvector:pg17 after these commits — TestCredentialDecryptRoundTripAfterMigration (1 top + 8 sub), TestSyncProjectState_* (5), TestHandleMemoryTriggers_Bash* (3), TestCredentialStore_* (9 incl sub-tests), behavioral_rules + memory store suites. * fix(handlers-vault): remove err.Error() from all 500 responses Addresses CodeRabbit MINOR finding PRRT_kwDORY-ek8577YWO on re-review of PR #181: 'Don't disclose error details in 500-response' (handlers_vault.go:62). Initial fix only covered some handlers; this commit extends the same mitigation to the remaining five 500 sites in handlers_vault.go: - handleListCredentials line 62: 'list credentials: ' + err.Error() → 'failed to list credentials' - handleGetCredential line 111: 'vault not available: ' + err.Error() → 'vault not available' - handleGetCredential line 137: 'decrypt credential: ' + err.Error() → 'failed to decrypt credential' - handleStoreCredential line 207: 'vault not available: ' + err.Error() → 'vault not available' - handleStoreCredential line 214: 'encrypt credential: ' + err.Error() → 'failed to encrypt credential' - handleStoreCredential line 229: 'store credential: ' + err.Error() → 'failed to store credential' All sites now call log.Error().Err(err).Msg(...) first so server-side debugging still has full context. The 400 (invalid JSON body) site keeps err.Error() in the body — 400 is a client-parse error and the detail helps the client diagnose their own malformed request; this is intentional and not flagged by the reviewer. Verified: go build + go vet clean. --------- Co-authored-by: Kirill Turanskiy <thebtf@users.noreply.github.com>
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
Test plan
Summary by CodeRabbit
New Features
Improvements
Documentation