feat(v3.7.0): learning-memory-v4 MVP — silence gate, unified inject, realtime feedback#134
Merged
Conversation
When the LLM explicitly returns an empty JSON array (intentional silence),
return []int64{} instead of falling back to composite scoring top-5.
The error/timeout fail-open path (returns allIDs) is preserved for resilience.
Adds 3 unit tests: empty-set silence, parse-error fallback, timeout fallback.
When FilterByRelevance returns an empty slice (LLM silence gate), set clusteredObservations to empty instead of silently keeping the pre-filter set. Adds Info-level log "LLM filter silenced injection" with project and total_considered fields for operator observability (NFR-5).
Disables the injection floor anti-pattern by defaulting InjectionFloor to 0. With floor=0 the silence path is active: no fill observations are injected when composite scoring eliminates all candidates (FR-1, learning-memory-v4). Operators can restore legacy behavior via ENGRAM_INJECTION_FLOOR=3.
Removes the hardcoded fallback that forced injectionFloor=3 even when the config set it to 0. Both floor-fill sites in handlers_context.go are now guarded by `if injectionFloor > 0` so the silence path (FR-1) is honoured. Extracts fillToFloor helper (floor_fill.go) used by the search path site; the inject path site keeps inline logic to correctly account for the total across all four injection sections. Unit tests in handlers_context_floor_test.go cover floor=0 no-fetch, floor=3 fill, already-met no-fetch, and deduplication. Anti-stub check confirmed: stubbing fillToFloor body fails the fill test.
Wire injectionStore into MCP Server and add PropagateOutcome goroutine to handleSetSessionOutcomeMCP, matching the pattern in handlers_learning.go. This closes the bypass where MCP tool callers would update session outcome without triggering utility score propagation.
WalkthroughКрупный релиз Learning Memory v4 MVP: изменение инжекций (InjectionFloor 3→0, сохранение пустых LLM-фильтров), введён унифицированный RetrieveRelevant pipeline, добавлена пропагация исходов через SessionEnd и endpoint /api/sessions/{id}/propagate-outcome, миграция utility_propagated_at и многочисленные тесты и скрипты. Changes
Sequence DiagramsequenceDiagram
participant Client as Клиент
participant Plugin as Plugin\n(session-end.js)
participant API as Backend API
participant Store as Stores\n(injection, observation, session)
participant Learn as Learning\nService
Client->>Plugin: SessionEnd событие
Plugin->>API: POST /api/sessions/{id}/propagate-outcome
Note over API: Валидация sessionId\nПопытка claim via UpdateUtilityPropagatedAtIfStale
API->>Store: Load session и outcome
API->>Store: Если claim OK -> Set utility_propagated_at (NOW)
API->>Learn: Запустить PropagateOutcome (async, timeout 30s)
Learn->>Store: Получить injection/observation данные
Learn->>Store: Обновить/пропагировать outcome в наблюдениях
Learn-->>API: Завершено (фон)
API-->>Plugin: 202 Accepted (propagation queued)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 минут Possibly related PRs
Поэма
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![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 Learning Memory v4, which rebuilds the retrieval path to improve relevance and reduce noise. Key changes include the removal of the 'injection floor' anti-pattern (defaulting to 0), the introduction of a unified hybrid search path for context injection, and real-time outcome propagation via a new SessionEnd hook. Feedback highlights encoding issues (mojibake) in several comments, a non-standard 'future work' section in the CHANGELOG, and potential semantic confusion regarding how floor-filled observations are categorized in the injection response.
Comment on lines
+128
to
+136
| ### Deferred to Phase 2 of v4 | ||
|
|
||
| - **FR-6** Project briefing (Task 23 `generateProjectBriefing` with `NO_CHANGE` sentinel) | ||
| - **FR-7** File-scope prefiltering (`files_being_edited` parameter in inject/search) | ||
| - **FR-8** Per-type search lanes (`SearchLaneConfig` map) | ||
| - **FR-9** Alarm model expansion (`POST /api/memory/triggers` for Bash and repeated Read) | ||
| - **FR-10** Write-time observation merge (`DecideMerge` LLM call before insert) | ||
|
|
||
| Prompt drafts for FR-6 and FR-10 are ready in `.agent/specs/learning-memory-v4/prompt-drafts-f3-f7.md`. |
There was a problem hiding this comment.
This section describes future work, which is not standard for a changelog. A changelog should document what is included in the release to avoid confusion for users trying to understand the contents of version 3.7.0. This information is better suited for the pull request description or project roadmap documentation.
| } | ||
|
|
||
| // Track search misses for self-tuning analytics (inline — avoids unbounded goroutine spawn) | ||
| // Track search misses for self-tuning analytics (inline — avoids unbounded goroutine spawn) |
There was a problem hiding this comment.
This comment, and several others in this file, contains mojibake (—) which appears to be an encoding issue for an em-dash. This harms readability.
This issue is present on lines 290, 428, 692, 710, 762, 801, 1021, 1221, and 1370. Please replace these with a standard em-dash (—) or a hyphen (-) for clarity.
Suggested change
| // Track search misses for self-tuning analytics (inline — avoids unbounded goroutine spawn) | |
| // Track search misses for self-tuning analytics (inline — avoids unbounded goroutine spawn) |
| vectorResults, vecErr := s.vectorClient.Query(ctx, query, 20, where) | ||
| if vecErr != nil { | ||
| log.Debug().Err(vecErr).Str("project", project).Msg("Vector query failed for context inject relevant section") | ||
| if s.config == nil || s.config.InjectUnified { |
There was a problem hiding this comment.
The s.config == nil check is a defensive guard that can mask service initialization problems. A better practice is to ensure that s.config is always non-nil after the service starts, causing a failure early on if it's not configured correctly. Removing this check would make the code cleaner and more robust against configuration errors.
Suggested change
| if s.config == nil || s.config.InjectUnified { | |
| if s.config.InjectUnified { |
| if fillErr == nil { | ||
| for _, obs := range fillObs { | ||
| if _, already := recentIDs[obs.ID]; !already { | ||
| recentFresh = append(recentFresh, obs) |
There was a problem hiding this comment.
The injection floor logic appends top-importance observations to the recentFresh slice. This is semantically confusing, as these "floor-filled" observations are selected for importance, not recency. This could mislead consumers of the API and future maintainers.
To improve clarity, consider appending these observations to a more general-purpose slice like relevantObservations, or creating a new dedicated slice in the response (e.g., floor_fill_observations).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
internal/db/gorm/migrations.go (1)
2166-2172: Добавить индекс для столбцаutility_propagated_atв миграцию 072.Функция
GetSessionsWithPendingOutcome(session_store.go) выполняет запрос с фильтрамиWHERE outcome IS NULL AND (utility_propagated_at IS NULL OR utility_propagated_at < NOW() - INTERVAL '2 hours'). Без индекса это приведёт к полному сканированию таблицыsdk_sessionsпри каждом вызове.Рекомендуется добавить частичный индекс:
Предлагаемое улучшение
{ ID: "072_sessions_utility_propagated_at", Migrate: func(tx *gorm.DB) error { - return tx.Exec(`ALTER TABLE sdk_sessions ADD COLUMN IF NOT EXISTS utility_propagated_at TIMESTAMPTZ`).Error + sqls := []string{ + `ALTER TABLE sdk_sessions ADD COLUMN IF NOT EXISTS utility_propagated_at TIMESTAMPTZ`, + `CREATE INDEX IF NOT EXISTS idx_sdk_sessions_utility_propagated_outcome + ON sdk_sessions (utility_propagated_at) + WHERE outcome IS NULL`, + } + for _, s := range sqls { + if err := tx.Exec(s).Error; err != nil { + return err + } + } + return nil }, Rollback: func(tx *gorm.DB) error { - return tx.Exec(`ALTER TABLE sdk_sessions DROP COLUMN IF EXISTS utility_propagated_at`).Error + if err := tx.Exec(`DROP INDEX IF EXISTS idx_sdk_sessions_utility_propagated_outcome`).Error; err != nil { + return err + } + return tx.Exec(`ALTER TABLE sdk_sessions DROP COLUMN IF EXISTS utility_propagated_at`).Error }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/gorm/migrations.go` around lines 2166 - 2172, Migration "072_sessions_utility_propagated_at" currently only adds the column; modify its Migrate to also create a partial index (e.g. idx_sdk_sessions_utility_propagated_at_pending_outcome) on sdk_sessions(utility_propagated_at) WHERE outcome IS NULL AND (utility_propagated_at IS NULL OR utility_propagated_at < NOW() - INTERVAL '2 hours') to prevent full table scans used by GetSessionsWithPendingOutcome, and update its Rollback to DROP that index; reference the migration ID "072_sessions_utility_propagated_at" and the query/filter in GetSessionsWithPendingOutcome when implementing the change.internal/worker/retrieval.go (3)
318-321: Рассмотрите упрощение сложного условия.Условие на строке 319 содержит несколько вложенных проверок на
nil. Хотя логика корректна, можно рассмотреть выделение в отдельный методhasReranker()для читаемости.💡 Предложение
+func (s *Service) hasReranker() bool { + if s.reranker != nil { + return true + } + if s.retrievalHooks == nil { + return false + } + return s.retrievalHooks.rerank != nil || s.retrievalHooks.rerankByScore != nil +} + func (s *Service) applyReranking(query string, observations []*models.Observation, similarityScores map[int64]float64) []*models.Observation { - if len(observations) == 0 || (s.reranker == nil && (s.retrievalHooks == nil || s.retrievalHooks.rerank == nil && s.retrievalHooks.rerankByScore == nil)) { + if len(observations) == 0 || !s.hasReranker() { return observations }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/retrieval.go` around lines 318 - 321, The boolean check in applyReranking is hard to read due to nested nil checks; extract that logic into a helper like hasReranker() on Service that returns true if s.reranker != nil || s.retrievalHooks != nil && (s.retrievalHooks.rerank != nil || s.retrievalHooks.rerankByScore != nil), then replace the compound condition in applyReranking(query, observations, similarityScores) with a simple len(observations) == 0 || !s.hasReranker() check to improve readability and keep behavior identical.
188-193: Документируйте порог0.05для подсчёта результатов.Значение
0.05определяет минимальный score для учёта результата вtotalResults, но не задокументировано. Это влияет на метрики и должно быть объяснено.💡 Предложение
+ // minScoreForCounting filters out noise-level results from totalResults metric + const minScoreForCounting = 0.05 totalResults := 0 for _, observation := range clusteredObservations { - if score, exists := similarityScores[observation.ID]; exists && score > 0.05 { + if score, exists := similarityScores[observation.ID]; exists && score > minScoreForCounting { totalResults++ } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/retrieval.go` around lines 188 - 193, Порог 0.05, используемый при подсчёте totalResults, не документирован; вынесите магическое число в именованную константу (например minScoreThreshold или minSimilarityScore) с кратким комментарием о её назначении и обосновании, замените литерал 0.05 в цикле по clusteredObservations и similarityScores на эту константу, и обновите комментарий/докстринг функции (или место вычисления метрик) чтобы зафиксировать влияние порога на метрики и возможность настройки.
116-116: Документируйте магическое число0.9.Множитель
threshold*0.9снижает порог на 10%, но причина этого не объяснена. Рассмотрите извлечение в именованную константу или добавление комментария.💡 Предложение
+ // thresholdRelaxFactor allows slightly lower-scored results through to reranking + const thresholdRelaxFactor = 0.9 if len(allVectorResults) > 0 { - filteredResults := vector.FilterByThreshold(allVectorResults, threshold*0.9, 0) + filteredResults := vector.FilterByThreshold(allVectorResults, threshold*thresholdRelaxFactor, 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/retrieval.go` at line 116, В коде вызова vector.FilterByThreshold(allVectorResults, threshold*0.9, 0) магическое число 0.9 не объяснено; вынесите множитель в именованную константу (например RetrievalThresholdRelaxation или THRESHOLD_RELAX_FACTOR) или добавьте краткий комментарий рядом с вызовом, чтобы описать почему порог снижается на 10%, и замените threshold*0.9 на threshold * THRESHOLD_RELAX_FACTOR, сохранив читаемость и документацию через имя константы.internal/worker/handlers_context_inject_unified_test.go (1)
43-56: Рассмотрите рефакторинг для тестирования через реальный обработчик.Все тесты в этом файле реплицируют логику обработчика напрямую (как указано в комментарии на строке 43-44). Это делает тесты хрупкими — если логика в
handleContextInjectизменится, тесты не обнаружат регрессию.Рассмотрите возможность извлечения логики определения
injectQueryв отдельную тестируемую функцию или тестирования через реальный обработчик с mock-зависимостями.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_context_inject_unified_test.go` around lines 43 - 56, The test duplicates the injectQuery derivation instead of exercising the real handler, making it brittle; refactor by extracting the injectQuery logic into a single function (e.g., DetermineInjectQuery(project string, sessionID string, svc *Service) or similar) or test via the real handler handleContextInject with mocked dependencies; move the block that calls svc.loadRecentUserPromptsByProject and its fallback into that new function or call handleContextInject in the test using a mock for loadRecentUserPromptsByProject so changes to handleContextInject are covered and the test no longer replicates implementation details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 10: Добавлена секция релиза "## [3.7.0] - 2026-04-11", но reference-блок
внизу файла не содержит ссылку для [3.7.0] и не обновляет запись для
[Unreleased]; открой CHANGELOG.md, внизу добавь строку с ссылкой для [3.7.0] в
том же формате, что и другие релизы (например "[3.7.0]:
<compare-or-release-URL>") и обнови/синхронизируй строку для [Unreleased] при
необходимости, чтобы ссылки [Unreleased] и [3.7.0] корректно указывали на
соответствующие URL/сравнения.
In `@internal/config/config_test.go`:
- Around line 62-84: Tests use os.Setenv/Unsetenv which doesn't restore prior
state and makes tests order-dependent; replace those calls with s.T().Setenv to
ensure automatic cleanup. Update TestInjectionFloorEnvOverride,
TestInjectUnifiedDefaultTrue, and TestInjectUnifiedEnvOverride to call
s.T().Setenv("ENGRAM_INJECTION_FLOOR","3"),
s.T().Setenv("ENGRAM_INJECT_UNIFIED","") (to force default) and
s.T().Setenv("ENGRAM_INJECT_UNIFIED","false") respectively, then call Load() and
assert the same expectations so environment is isolated and restored via the
test's cleanup.
In `@internal/mcp/tools_learning.go`:
- Around line 48-50: В горутине не используйте context.Background() без дедлайна
— замените создание bgCtx в анонимной функции на bounded context с
context.WithTimeout и defer cancel(), чтобы ограничить время выполнения
PropagateOutcome; вызов должен передавать контекст с таймаутом в
learning.PropagateOutcome (где сейчас передаёте bgCtx вместе с s.injectionStore,
s.observationStore, capturedSessionID, capturedOutcome), чтобы fire-and-forget
не оставлял висящие горутины при зависании хранилища.
- Around line 45-53: After a successful call to learning.PropagateOutcome in the
goroutine (the block using capturedSessionID/capturedOutcome with
s.injectionStore and s.observationStore), update the session record's
utility_propagated_at timestamp to mark the session as propagated so it won't be
re-processed; perform this update inside the same goroutine after
PropagateOutcome returns nil, using the session identifier (capturedSessionID)
and the session store/updater you have available, and log any error if the
update fails (do not update on PropagateOutcome error).
In `@internal/worker/handlers_context_inject_unified_test.go`:
- Around line 159-185: The test TestInjectRelevant_LegacyPath_WhenFlagFalse
incorrectly simulates the branch by directly inspecting svc.config.InjectUnified
instead of invoking the real handler logic; update the test to call the actual
handler (e.g., handleContextInject or the exported method that decides between
unified vs legacy paths) with a request context and payload that exercises
InjectUnified=false on the newInjectTestService(false) instance, then assert
that svc.retrievalHooks.retrieveRelevant was NOT called; ensure you remove the
manual branch replication (the if svc.config.InjectUnified block) and use the
handler invocation to drive the branch decision so the test fails if handler
branching is wrong.
In `@internal/worker/handlers_context.go`:
- Around line 903-909: The current injectQuery lookup uses
loadRecentUserPromptsByProject(...) with only project scope, so if sessionID !=
"" you can pick another session's prompt; change the lookup to be session-scoped
(e.g., call a session-aware function like loadRecentUserPromptsBySession(ctx,
project, sessionID, 1) or filter the returned prompts by Prompt.SessionID ==
sessionID) and assign prompts[0].PromptText to injectQuery only from that
session, or alternatively update the comment/contract to explicitly state this
is a project-level prompt if session scoping is not desired.
In `@internal/worker/handlers_learning.go`:
- Around line 215-220: The goroutine calling learning.PropagateOutcome(bgCtx,
injStore, obsStore, capturedSessionID, capturedOutcome) uses
context.Background() with no deadline and can hang; replace bgCtx with a context
created via context.WithTimeout inside the anonymous goroutine (e.g., ctx,
cancel := context.WithTimeout(context.Background(), <reasonableDuration>)) and
defer cancel() before calling learning.PropagateOutcome so the call is bounded
by the timeout and resources are released if the storage call blocks.
- Around line 180-211: The current rate-limit check using
sess.UtilityPropagatedAt followed later by
sessionStore.UpdateUtilityPropagatedAt is vulnerable to TOCTOU; change the flow
so the check-and-set is atomic in the DB. Implement an atomic setter on the
session store (e.g. UpdateUtilityPropagatedAtAtomic or
TrySetUtilityPropagatedAt) that runs a single SQL UPDATE ... WHERE session_id =
$1 AND (utility_propagated_at IS NULL OR utility_propagated_at < now() -
interval '1 minute') and returns whether a row was updated (and optionally the
existing timestamp when not updated). In the handler replace the initial
sess.UtilityPropagatedAt check with a call to this new atomic method: if it
returns false, compute retry_after from the timestamp returned (or re-read
utility_propagated_at) and respond with 409 as before; if it returns true,
continue and call injStore.CountInjectionsBySession and proceed. Update
references to UpdateUtilityPropagatedAt to use the new atomic semantics and
remove the separate pre-check to eliminate the TOCTOU window.
In `@pkg/models/session.go`:
- Around line 27-35: The UtilityPropagatedAt field currently uses sql.NullTime
which serializes to a nested object and breaks the API contract; change
UtilityPropagatedAt in SDKSession to the same JSON-friendly form as the other
timestamps (e.g., sql.NullString) or replace it with a pointer/time wrapper that
implements the same string-or-null JSON marshaling used by the other timestamp
fields, ensuring the JSON output for utility_propagated_at is a string or null
and keeping the field tagged with `json:"utility_propagated_at,omitempty"`;
update any DB scanning or construction logic that populates UtilityPropagatedAt
to convert from time.Time to the chosen string/null representation.
---
Nitpick comments:
In `@internal/db/gorm/migrations.go`:
- Around line 2166-2172: Migration "072_sessions_utility_propagated_at"
currently only adds the column; modify its Migrate to also create a partial
index (e.g. idx_sdk_sessions_utility_propagated_at_pending_outcome) on
sdk_sessions(utility_propagated_at) WHERE outcome IS NULL AND
(utility_propagated_at IS NULL OR utility_propagated_at < NOW() - INTERVAL '2
hours') to prevent full table scans used by GetSessionsWithPendingOutcome, and
update its Rollback to DROP that index; reference the migration ID
"072_sessions_utility_propagated_at" and the query/filter in
GetSessionsWithPendingOutcome when implementing the change.
In `@internal/worker/handlers_context_inject_unified_test.go`:
- Around line 43-56: The test duplicates the injectQuery derivation instead of
exercising the real handler, making it brittle; refactor by extracting the
injectQuery logic into a single function (e.g., DetermineInjectQuery(project
string, sessionID string, svc *Service) or similar) or test via the real handler
handleContextInject with mocked dependencies; move the block that calls
svc.loadRecentUserPromptsByProject and its fallback into that new function or
call handleContextInject in the test using a mock for
loadRecentUserPromptsByProject so changes to handleContextInject are covered and
the test no longer replicates implementation details.
In `@internal/worker/retrieval.go`:
- Around line 318-321: The boolean check in applyReranking is hard to read due
to nested nil checks; extract that logic into a helper like hasReranker() on
Service that returns true if s.reranker != nil || s.retrievalHooks != nil &&
(s.retrievalHooks.rerank != nil || s.retrievalHooks.rerankByScore != nil), then
replace the compound condition in applyReranking(query, observations,
similarityScores) with a simple len(observations) == 0 || !s.hasReranker() check
to improve readability and keep behavior identical.
- Around line 188-193: Порог 0.05, используемый при подсчёте totalResults, не
документирован; вынесите магическое число в именованную константу (например
minScoreThreshold или minSimilarityScore) с кратким комментарием о её назначении
и обосновании, замените литерал 0.05 в цикле по clusteredObservations и
similarityScores на эту константу, и обновите комментарий/докстринг функции (или
место вычисления метрик) чтобы зафиксировать влияние порога на метрики и
возможность настройки.
- Line 116: В коде вызова vector.FilterByThreshold(allVectorResults,
threshold*0.9, 0) магическое число 0.9 не объяснено; вынесите множитель в
именованную константу (например RetrievalThresholdRelaxation или
THRESHOLD_RELAX_FACTOR) или добавьте краткий комментарий рядом с вызовом, чтобы
описать почему порог снижается на 10%, и замените threshold*0.9 на threshold *
THRESHOLD_RELAX_FACTOR, сохранив читаемость и документацию через имя константы.
🪄 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: 8fae2897-9b5e-4952-8580-f6c65d70dcd1
📒 Files selected for processing (26)
CHANGELOG.mdinternal/config/config.gointernal/config/config_test.gointernal/db/gorm/migrations.gointernal/db/gorm/models.gointernal/db/gorm/session_store.gointernal/mcp/server.gointernal/mcp/tools_learning.gointernal/search/llm_filter.gointernal/search/llm_filter_test.gointernal/worker/floor_fill.gointernal/worker/handlers_context.gointernal/worker/handlers_context_floor_test.gointernal/worker/handlers_context_inject_unified_test.gointernal/worker/handlers_learning.gointernal/worker/retrieval.gointernal/worker/retrieval_helpers.gointernal/worker/retrieval_test.gointernal/worker/service.gopkg/models/session.goplugin/engram/.claude-plugin/plugin.jsonplugin/engram/hooks/hooks.jsonplugin/engram/hooks/session-end.jsplugin/openclaw-engram/openclaw.plugin.jsonplugin/openclaw-engram/package.jsonscripts/bench-inject.sh
Comment on lines
+62
to
+84
| // TestInjectionFloorEnvOverride verifies that ENGRAM_INJECTION_FLOOR=3 restores legacy fill behavior. | ||
| func (s *ConfigSuite) TestInjectionFloorEnvOverride() { | ||
| os.Setenv("ENGRAM_INJECTION_FLOOR", "3") | ||
| defer os.Unsetenv("ENGRAM_INJECTION_FLOOR") | ||
| cfg, err := Load() | ||
| s.Require().NoError(err) | ||
| s.Equal(3, cfg.InjectionFloor) | ||
| } | ||
|
|
||
| // TestInjectUnifiedDefaultTrue verifies that ENGRAM_INJECT_UNIFIED defaults to true (FR-3). | ||
| func (s *ConfigSuite) TestInjectUnifiedDefaultTrue() { | ||
| cfg, err := Load() | ||
| s.Require().NoError(err) | ||
| s.True(cfg.InjectUnified, "InjectUnified must default to true so the unified inject path is active") | ||
| } | ||
|
|
||
| // TestInjectUnifiedEnvOverride verifies that ENGRAM_INJECT_UNIFIED=false enables the emergency rollback path. | ||
| func (s *ConfigSuite) TestInjectUnifiedEnvOverride() { | ||
| os.Setenv("ENGRAM_INJECT_UNIFIED", "false") | ||
| defer os.Unsetenv("ENGRAM_INJECT_UNIFIED") | ||
| cfg, err := Load() | ||
| s.Require().NoError(err) | ||
| s.False(cfg.InjectUnified, "ENGRAM_INJECT_UNIFIED=false must activate the legacy inject path") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go tests, what does testing.T.Setenvdo compared toos.Setenv, and does it automatically restore environment variables after the test?
💡 Result:
In Go tests, testing.T.Setenv sets an environment variable for the duration of the current test (and its subtests), automatically restoring it by calling os.Unsetenv(key) via t.Cleanup when the test exits. In contrast, os.Setenv modifies the process-global environment persistently, which can leak to other tests (especially parallel ones) unless manually restored with defer os.Unsetenv or t.Cleanup(os.Unsetenv). t.Setenv also panics if called on a parallel test and disables parallelism for that test to prevent races.
Citations:
- 1: https://pkg.go.dev/testing
- 2: https://go.dev/issue/41260
- 3: https://stackoverflow.com/questions/66061175/mocking-os-getenvenv-var
- 4: all: replace os.Setenv with T.Setenv in tests golang/go#45448
Используйте s.T().Setenv() для изоляции тестов вместо os.Setenv/defer os.Unsetenv.
TestInjectUnifiedDefaultTrue не очищает переменную окружения ENGRAM_INJECT_UNIFIED и будет неправильно работать, если эта переменная уже установлена в окружении CI или предыдущим тестом. Кроме того, os.Setenv с defer os.Unsetenv не восстанавливает исходное значение переменной — она просто удаляется, что нарушает изоляцию тестов и делает их order-dependent.
testing.T.Setenv (доступный через s.T().Setenv() в контексте suite) автоматически восстанавливает исходное значение переменной при завершении теста через t.Cleanup, что предотвращает утечку состояния на другие тесты. Переиспишите:
Пример исправления
func (s *ConfigSuite) TestInjectUnifiedDefaultTrue() {
s.T().Setenv("ENGRAM_INJECT_UNIFIED", "") // явно установить пустое значение для default-path
cfg, err := Load()
s.Require().NoError(err)
s.True(cfg.InjectUnified, "InjectUnified must default to true")
}
func (s *ConfigSuite) TestInjectUnifiedEnvOverride() {
s.T().Setenv("ENGRAM_INJECT_UNIFIED", "false")
cfg, err := Load()
s.Require().NoError(err)
s.False(cfg.InjectUnified)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config_test.go` around lines 62 - 84, Tests use
os.Setenv/Unsetenv which doesn't restore prior state and makes tests
order-dependent; replace those calls with s.T().Setenv to ensure automatic
cleanup. Update TestInjectionFloorEnvOverride, TestInjectUnifiedDefaultTrue, and
TestInjectUnifiedEnvOverride to call s.T().Setenv("ENGRAM_INJECTION_FLOOR","3"),
s.T().Setenv("ENGRAM_INJECT_UNIFIED","") (to force default) and
s.T().Setenv("ENGRAM_INJECT_UNIFIED","false") respectively, then call Load() and
assert the same expectations so environment is isolated and restored via the
test's cleanup.
…134-H1+H2] Replace the read-then-check-then-write pattern in handlePropagateOutcome with an atomic SQL UPDATE ... WHERE utility_propagated_at IS NULL OR < NOW()-1min. Zero rows affected means rate-limited (HTTP 409). The goroutine now runs with a 30s context timeout and reverts the timestamp on failure so callers can retry. Response changed to HTTP 202 Accepted since propagation is dispatched async. Also resolves M3 (misnamed "updated" field removed) and pr134-major4 (timeout). New session_store methods: UpdateUtilityPropagatedAtIfStale, ClearUtilityPropagatedAt. Also fixes L2: UpdateUtilityPropagatedAt now uses time.Now().UTC().
…sion_outcome [pr134-major5+major6] The MCP set_session_outcome goroutine had two bugs: 1. context.Background() with no deadline -> goroutine leak on DB hang [pr134-major5] 2. Never updated utility_propagated_at after propagation -> maintenance guard does not see the propagation and double-runs [pr134-major6] Fix: wrap with context.WithTimeout(30s), and on success call UpdateUtilityPropagatedAt.
…4-major1]
sql.NullTime serializes as {"Time":"...","Valid":true} which leaks the DB
implementation detail into the public API JSON.
Add SDKSession.MarshalJSON() using a shadow struct that emits
utility_propagated_at as an RFC3339 string when set, or omits it when null.
…r-changelog-ascii]
…-gemini-changelog]
…e [pr134-major2] Add loadLastUserPromptBySession which scopes the inject-query derivation to the current claude_session_id. Previously loadRecentUserPromptsByProject returned the latest prompt across ANY session in the project, so session A could be seeded by session B's last prompt — breaking the feature promise. New fallback chain: session_id != "" → last prompt for that session (via getLastPromptBySession hook) session_id == "" → last prompt for the project (cold-start) both empty → project name as literal query (ultimate fallback) Add getLastPromptBySession hook to retrievalHooks for test injection. Update handlers_context.go doc comment to reflect session-scoped semantics.
…mpts [pr134-major7] Rewrite inject unified tests to use the new session-scoped lookup hook (getLastPromptBySession) instead of the old project-wide hook. Add: - TestInjectRelevant_UnifiedPath_UsesLastUserPrompt: verifies that only the correct session's prompt is used as the retrieval query. - TestInjectRelevant_TwoSessionsDifferentPrompts: anti-stub test — two sessions with different prompts must produce different queries. Replacing the session-scoped hook with project-wide fallback makes both sessions return the project name as query (q1 == q2), failing this test. - TestInjectRelevant_SessionScoped_IgnoresOtherSessionPrompts: verifies session-auth and session-db produce completely different inject queries. - TestInjectRelevant_LegacyPath_WhenFlagFalse: unchanged, verifies that InjectUnified=false skips the unified path entirely.
The applyReranking early-return condition combined three nil-checks with mixed && / || precedence into a single hard-to-parse line. Extract to a named boolean noRerankAvailable and make the && grouping explicit, matching the original semantics exactly but readable at a glance.
…r134-minor2] Extract the 0.05 literal to a named const NoiseFloorScore with a doc comment explaining why the threshold exists: in high-dimensional embedding spaces nearly all observations pass the raw vector threshold, so only composite scores above 0.05 represent genuine semantic matches.
Two separate 0.9 constants in retrieval.go now have names and rationale: - vectorPreFilterFactor (0.9): widens the vector pre-filter by 10% below the project threshold, giving borderline-relevant observations a chance to reach the reranker before being discarded. - clusteringThreshold default (0.9): cosine similarity at which two observations are considered near-duplicates and merged into one cluster. Documented with an inline comment; overridable via config.
…emini1] Eight comment strings contained CP1251-encoded text (—) that was stored as if it were UTF-8, producing unreadable mojibake. Replace all occurrences with the correct UTF-8 em dash (—). Lines affected: 290, 692, 710, 762, 801, 1021, 1221, 1370.
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/worker/handlers_learning.go (1)
110-143:⚠️ Potential issue | 🟠 MajorПометьте propagation как выполненную и в
handleSetSessionOutcome.Этот путь по-прежнему вызывает
learning.PropagateOutcome(...)напрямую, не обновляяutility_propagated_at. Из-за этогоSessionEnd/ручнойPOST /api/sessions/{sessionId}/propagate-outcomeможет повторно принять тот же сеанс и второй раз увеличитьsuccessesиutility_score. Лучше вынести claim+rollback в общий helper и использовать его в обоих хендлерах.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_learning.go` around lines 110 - 143, The propagation path currently calls learning.PropagateOutcome(...) without marking the session's utility_propagated_at, so duplicate propagation can double-count; extract a shared helper (e.g., claimAndPropagateOutcome or PropagateOutcomeWithClaim) that performs the claim (set utility_propagated_at/timestamp), runs learning.PropagateOutcome with a rollback on failure, and use that helper from both this goroutine and handleSetSessionOutcome/SessionEnd/POST /api/sessions/{sessionId}/propagate-outcome so propagation is idempotent and agent-specific propagation (learning.PropagateAgentStats) is invoked only after a successful claim. Ensure the helper accepts the stores (injStore, obsStore, agentStatsStore), capturedSessionID, capturedOutcome and returns an error so callers can log failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 103: Update the CHANGELOG entry for migration
`072_sessions_utility_propagated_at` to reference the correct table name
`sdk_sessions` (not `sessions`); change the human-readable description and the
example SQL from `ALTER TABLE sessions ADD COLUMN IF NOT EXISTS
utility_propagated_at TIMESTAMPTZ` to `ALTER TABLE sdk_sessions ADD COLUMN IF
NOT EXISTS utility_propagated_at TIMESTAMPTZ` so the manual verification command
matches the actual migration implementation.
In `@internal/db/gorm/session_store.go`:
- Around line 264-268: The writer in UpdateUtilityPropagatedAt is setting
utility_propagated_at with time.Now().UTC(), causing drift against queries that
compare to PostgreSQL NOW(); change the update to use the database time instead
(e.g. use a gorm expression/DB NOW() rather than Go time) so
SDKSession.utility_propagated_at is set via the DB clock and remains consistent
with UpdateUtilityPropagatedAtIfStale and the pending sessions query.
In `@internal/worker/handlers_learning.go`:
- Around line 214-215: The call to capturedSessionStore.ClearUtilityPropagatedAt
uses context.Background(), which can leave the background goroutine hanging if
the DB blocks; replace it with a context that has a deadline/timeout (e.g.,
context.WithTimeout) and pass that context to ClearUtilityPropagatedAt, then
defer the cancel; locate the call to ClearUtilityPropagatedAt in
handlers_learning.go (the capturedSessionStore.ClearUtilityPropagatedAt(...)
call) and use a short configurable timeout or reuse the surrounding
request/propagation context instead of context.Background().
In `@internal/worker/retrieval.go`:
- Around line 138-146: Вы сейчас обрезаете векторные кандидаты по "date_desc"
внутри вызова s.fetchObservationsByID(…, "date_desc", limit) до того, как
выполняется composite scoring/rerank, из‑за чего релевантные старые наблюдения
могут быть потеряны; измените логику так, чтобы сначала получить все ID из
vector.ExtractObservationIDs (или получить без привязки к "date_desc" и без
применения limit в s.fetchObservationsByID), затем применить composite
scoring/rerank к полному набору fetched и только после этого применять
limit/сортировку для окончательного среза; отладьте места вокруг
vector.ExtractObservationIDs, s.fetchObservationsByID и логики rerank/composite
scoring, и убедитесь, что флаг usedVector выставляется после успешного
rerank/фильтрации, а не до.
---
Outside diff comments:
In `@internal/worker/handlers_learning.go`:
- Around line 110-143: The propagation path currently calls
learning.PropagateOutcome(...) without marking the session's
utility_propagated_at, so duplicate propagation can double-count; extract a
shared helper (e.g., claimAndPropagateOutcome or PropagateOutcomeWithClaim) that
performs the claim (set utility_propagated_at/timestamp), runs
learning.PropagateOutcome with a rollback on failure, and use that helper from
both this goroutine and handleSetSessionOutcome/SessionEnd/POST
/api/sessions/{sessionId}/propagate-outcome so propagation is idempotent and
agent-specific propagation (learning.PropagateAgentStats) is invoked only after
a successful claim. Ensure the helper accepts the stores (injStore, obsStore,
agentStatsStore), capturedSessionID, capturedOutcome and returns an error so
callers can log failures.
🪄 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: 13d4dec8-d2bc-4b2d-8642-5630fa02944c
📒 Files selected for processing (13)
CHANGELOG.mdinternal/config/config_test.gointernal/db/gorm/migrations.gointernal/db/gorm/session_store.gointernal/mcp/tools_learning.gointernal/worker/handlers_context.gointernal/worker/handlers_context_inject_unified_test.gointernal/worker/handlers_learning.gointernal/worker/retrieval.gointernal/worker/retrieval_helpers.gopkg/models/session.goplugin/engram/hooks/session-end.jsscripts/bench-inject.sh
✅ Files skipped from review due to trivial changes (2)
- internal/db/gorm/migrations.go
- scripts/bench-inject.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/config/config_test.go
- pkg/models/session.go
- plugin/engram/hooks/session-end.js
- internal/mcp/tools_learning.go
- internal/worker/handlers_context.go
- internal/worker/retrieval_helpers.go
- internal/worker/handlers_context_inject_unified_test.go
|
|
||
| ### Schema | ||
|
|
||
| - Migration `072_sessions_utility_propagated_at`: `ALTER TABLE sessions ADD COLUMN IF NOT EXISTS utility_propagated_at TIMESTAMPTZ`. Idempotent. |
There was a problem hiding this comment.
Исправьте имя таблицы в описании миграции.
В коде новое поле живёт в sdk_sessions, а не в sessions. В текущем виде changelog даёт неверную ручную команду для проверки или отладки.
🛠️ Предлагаемое исправление
-- Migration `072_sessions_utility_propagated_at`: `ALTER TABLE sessions ADD COLUMN IF NOT EXISTS utility_propagated_at TIMESTAMPTZ`. Idempotent.
+- Migration `072_sessions_utility_propagated_at`: `ALTER TABLE sdk_sessions ADD COLUMN IF NOT EXISTS utility_propagated_at TIMESTAMPTZ`. Idempotent.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Migration `072_sessions_utility_propagated_at`: `ALTER TABLE sessions ADD COLUMN IF NOT EXISTS utility_propagated_at TIMESTAMPTZ`. Idempotent. | |
| - Migration `072_sessions_utility_propagated_at`: `ALTER TABLE sdk_sessions ADD COLUMN IF NOT EXISTS utility_propagated_at TIMESTAMPTZ`. Idempotent. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 103, Update the CHANGELOG entry for migration
`072_sessions_utility_propagated_at` to reference the correct table name
`sdk_sessions` (not `sessions`); change the human-readable description and the
example SQL from `ALTER TABLE sessions ADD COLUMN IF NOT EXISTS
utility_propagated_at TIMESTAMPTZ` to `ALTER TABLE sdk_sessions ADD COLUMN IF
NOT EXISTS utility_propagated_at TIMESTAMPTZ` so the manual verification command
matches the actual migration implementation.
Comment on lines
+264
to
+268
| func (s *SessionStore) UpdateUtilityPropagatedAt(ctx context.Context, claudeSessionID string) error { | ||
| result := s.db.WithContext(ctx). | ||
| Model(&SDKSession{}). | ||
| Where("claude_session_id = ?", claudeSessionID). | ||
| Update("utility_propagated_at", time.Now().UTC()) |
There was a problem hiding this comment.
Здесь лучше использовать время БД, а не воркера.
UpdateUtilityPropagatedAtIfStale() и выборка pending sessions сравнивают колонку с NOW() на стороне PostgreSQL, а этот writer пишет time.Now().UTC(). При рассинхроне часов 1-минутное и 2-часовое окно начнут вести себя непредсказуемо.
🛠️ Предлагаемое исправление
result := s.db.WithContext(ctx).
Model(&SDKSession{}).
Where("claude_session_id = ?", claudeSessionID).
- Update("utility_propagated_at", time.Now().UTC())
+ Update("utility_propagated_at", gorm.Expr("NOW()"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *SessionStore) UpdateUtilityPropagatedAt(ctx context.Context, claudeSessionID string) error { | |
| result := s.db.WithContext(ctx). | |
| Model(&SDKSession{}). | |
| Where("claude_session_id = ?", claudeSessionID). | |
| Update("utility_propagated_at", time.Now().UTC()) | |
| func (s *SessionStore) UpdateUtilityPropagatedAt(ctx context.Context, claudeSessionID string) error { | |
| result := s.db.WithContext(ctx). | |
| Model(&SDKSession{}). | |
| Where("claude_session_id = ?", claudeSessionID). | |
| Update("utility_propagated_at", gorm.Expr("NOW()")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/db/gorm/session_store.go` around lines 264 - 268, The writer in
UpdateUtilityPropagatedAt is setting utility_propagated_at with
time.Now().UTC(), causing drift against queries that compare to PostgreSQL
NOW(); change the update to use the database time instead (e.g. use a gorm
expression/DB NOW() rather than Go time) so SDKSession.utility_propagated_at is
set via the DB clock and remains consistent with
UpdateUtilityPropagatedAtIfStale and the pending sessions query.
Comment on lines
+214
to
+215
| if clearErr := capturedSessionStore.ClearUtilityPropagatedAt(context.Background(), capturedSessionID); clearErr != nil { | ||
| log.Warn().Err(clearErr).Str("session", capturedSessionID).Msg("failed to clear utility_propagated_at after propagation failure") |
There was a problem hiding this comment.
Сброс utility_propagated_at тоже нужен с дедлайном.
На ошибочном пути ClearUtilityPropagatedAt вызывается с context.Background(). Если БД подвиснет именно здесь, фоновая goroutine останется жить без ограничений.
🛠️ Предлагаемое исправление
- if clearErr := capturedSessionStore.ClearUtilityPropagatedAt(context.Background(), capturedSessionID); clearErr != nil {
+ clearCtx, clearCancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer clearCancel()
+ if clearErr := capturedSessionStore.ClearUtilityPropagatedAt(clearCtx, capturedSessionID); clearErr != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if clearErr := capturedSessionStore.ClearUtilityPropagatedAt(context.Background(), capturedSessionID); clearErr != nil { | |
| log.Warn().Err(clearErr).Str("session", capturedSessionID).Msg("failed to clear utility_propagated_at after propagation failure") | |
| clearCtx, clearCancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer clearCancel() | |
| if clearErr := capturedSessionStore.ClearUtilityPropagatedAt(clearCtx, capturedSessionID); clearErr != nil { | |
| log.Warn().Err(clearErr).Str("session", capturedSessionID).Msg("failed to clear utility_propagated_at after propagation failure") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/worker/handlers_learning.go` around lines 214 - 215, The call to
capturedSessionStore.ClearUtilityPropagatedAt uses context.Background(), which
can leave the background goroutine hanging if the DB blocks; replace it with a
context that has a deadline/timeout (e.g., context.WithTimeout) and pass that
context to ClearUtilityPropagatedAt, then defer the cancel; locate the call to
ClearUtilityPropagatedAt in handlers_learning.go (the
capturedSessionStore.ClearUtilityPropagatedAt(...) call) and use a short
configurable timeout or reuse the surrounding request/propagation context
instead of context.Background().
Comment on lines
+138
to
+146
| observationIDs := vector.ExtractObservationIDs(filteredResults, project) | ||
| if len(observationIDs) > 0 { | ||
| fetched, fetchErr := s.fetchObservationsByID(ctx, observationIDs, "date_desc", limit) | ||
| if fetchErr != nil { | ||
| return nil, nil, fetchErr | ||
| } | ||
| if len(fetched) > 0 { | ||
| observations = fetched | ||
| usedVector = true |
There was a problem hiding this comment.
Не режьте vector-кандидатов по date_desc до пересчёта score.
Сейчас fetchObservationsByID(..., "date_desc", limit) выбрасывает все ID за пределами newest-N ещё до composite scoring и rerank. В результате старое, но более релевантное наблюдение может исчезнуть только потому, что оно не попало в последние limit записей.
🛠️ Предлагаемое исправление
- fetched, fetchErr := s.fetchObservationsByID(ctx, observationIDs, "date_desc", limit)
+ fetched, fetchErr := s.fetchObservationsByID(ctx, observationIDs, "date_desc", len(observationIDs))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| observationIDs := vector.ExtractObservationIDs(filteredResults, project) | |
| if len(observationIDs) > 0 { | |
| fetched, fetchErr := s.fetchObservationsByID(ctx, observationIDs, "date_desc", limit) | |
| if fetchErr != nil { | |
| return nil, nil, fetchErr | |
| } | |
| if len(fetched) > 0 { | |
| observations = fetched | |
| usedVector = true | |
| observationIDs := vector.ExtractObservationIDs(filteredResults, project) | |
| if len(observationIDs) > 0 { | |
| fetched, fetchErr := s.fetchObservationsByID(ctx, observationIDs, "date_desc", len(observationIDs)) | |
| if fetchErr != nil { | |
| return nil, nil, fetchErr | |
| } | |
| if len(fetched) > 0 { | |
| observations = fetched | |
| usedVector = true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/worker/retrieval.go` around lines 138 - 146, Вы сейчас обрезаете
векторные кандидаты по "date_desc" внутри вызова s.fetchObservationsByID(…,
"date_desc", limit) до того, как выполняется composite scoring/rerank, из‑за
чего релевантные старые наблюдения могут быть потеряны; измените логику так,
чтобы сначала получить все ID из vector.ExtractObservationIDs (или получить без
привязки к "date_desc" и без применения limit в s.fetchObservationsByID), затем
применить composite scoring/rerank к полному набору fetched и только после этого
применять limit/сортировку для окончательного среза; отладьте места вокруг
vector.ExtractObservationIDs, s.fetchObservationsByID и логики rerank/composite
scoring, и убедитесь, что флаг usedVector выставляется после успешного
rerank/фильтрации, а не до.
This was referenced Apr 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Learning Memory v4 MVP (Phases F0–F2). Repairs the injection path foundation before adding features. Baseline metrics showed 2164 feedback records with 0 positive/negative citations and 20 noise candidates with 0 high-value observations — the relevant-memory injection was broadcasting guidance rules agents never cited. This PR fixes that.
Breaking change:
InjectionFloordefault changed 3 → 0. Silence is now a legitimate result. Operators who relied on the old behavior must setENGRAM_INJECTION_FLOOR=3.Full spec set in
.agent/specs/learning-memory-v4/(gitignored). CHANGELOG entry in this PR.What ships
Phase F0 (audit, anti-pattern removal):
InjectionFloordefault 3 → 0, skip floor-fill when zeroPhase F1 (unified inject path):
RetrieveRelevantshared function from search pathproject + " code development"query with last-user-prompt derivationENGRAM_INJECT_UNIFIEDrollback flag (default true)retrievalHooksextension point for future F5/F8 phasesscripts/bench-inject.shlatency benchmark harnessPhase F2 (realtime outcome propagation via SessionEnd):
SessionEndhook inhooks.json— engram previously never registered it, which was the actual cause of the "stop hook unreliable" memory entryplugin/engram/hooks/session-end.js→ firesPOST /api/sessions/{id}/propagate-outcome(1200ms timeout within Claude's 1500ms budget)072_sessions_utility_propagated_atfor rate-limit guardrecordPendingOutcomesskips sessions propagated within last 2h (crash-proof fallback preserved)set_session_outcomewhich previously bypassedPropagateOutcomePlugin v3.7.0 — bumped across all 3 manifests.
Commits (17 T-tasks + 1 docs = 18 commits)
F0: T006 T007 T008 T009
F1: T010 T011 T012 T013
F2: T014 T015 T016 T017 T018 T019
Docs: CHANGELOG [3.7.0]
Test plan
Post-merge validation (run on Unraid deployment)
Risks
Deferred (follow-up PRs)
Prompt drafts for FR-6 and FR-10 are ready in the spec directory.
Related issues
Summary by CodeRabbit
Версия 3.7.0
Новые возможности
Исправления ошибок
Тесты