feat: self-learning utility signals#1
Conversation
…earnings) - session-start: render <engram-guidance> block, fire-and-forget mark-injected - user-prompt: fire-and-forget mark-injected after context search - stop: collect messages ring buffer (50 msgs, 5000 char cap), call extract-learnings
Replace hardcoded Unix paths (/some/absolute/path, /etc/passwd) with filepath.Join-based paths that are valid on both Unix and Windows.
- Rewrite self-learning.md to reflect actual implementation status (JS hooks in plugin/hooks/, not Go binaries in cmd/hooks/) - Mark Phases 1-3 as DONE, add Phase 2.5 (per-session utility signals) - Remove time estimates from global-roadmap.md (violates no-time-estimates rule)
Server-side:
- New session_observation_injections table (migration 028)
- RecordSessionInjections/GetSessionInjectedObservations store methods
- POST /api/sessions/{sessionId}/mark-injected (dual-write: per-session + global)
- GET /api/sessions/{sessionId}/injected-observations (returns obs with title/facts)
Client-side:
- user-prompt.js: use per-session mark-injected after session init
- stop.js: detectUtilitySignal() matches injected observations against
assistant messages via verbatim citation + correction proximity detection
- Signals "used"/"corrected" sent to POST /api/observations/{id}/utility
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly advances the self-learning capabilities of the system by integrating new mechanisms for tracking and evaluating the utility of injected observations. It establishes a feedback loop where the system can learn from user interactions, identify effective guidance, and extract new knowledge using LLMs, thereby transforming it into a more adaptive and intelligent agent. The changes span both frontend JavaScript hooks and backend Go services, alongside crucial database schema updates. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚥 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 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.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a self-learning mechanism by tracking observation utility within sessions, adding JS hooks to inject guidance, track injections, and extract learnings at session end, with backend updates including new database tables, store methods, and API endpoints to support per-session tracking. A high-severity prompt injection vulnerability was identified where guidance observations are rendered into the LLM prompt due to a lack of escaping for observation content, potentially allowing XML tag breakout and manipulation of the LLM's behavior. Additionally, the review highlighted a potential N+1 query issue, a raw SQL construction that could be improved, and a logic inconsistency in one of the hooks that may impact the feature's effectiveness.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
internal/db/gorm/migrations.go (1)
1007-1014: Добавьте FK и каскадное удаление для таблицы-связки.
session_observation_injectionsздесь ведёт себя как обычный join table, но схема не запрещает битыеsession_id/observation_id, а удаление observation потом оставит сиротские строки. Для этой таблицы лучше сразу зафиксировать ссылочную целостность (REFERENCES ... ON DELETE CASCADEкак минимум дляobservation_id, а лучше и дляsession_id), иначе session-scoped выборка со временем начнёт работать по неполным данным.🤖 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 1007 - 1014, The join table session_observation_injections currently lacks foreign keys and can accumulate orphans; modify the CREATE TABLE for session_observation_injections to add REFERENCES constraints on session_id and observation_id (e.g. session_id REFERENCES sessions(id) ON DELETE CASCADE, observation_id REFERENCES observations(id) ON DELETE CASCADE) so deletes on sessions or observations cascade and maintain referential integrity; also ensure any existing CREATE INDEX (idx_soi_session_id) still applies.internal/worker/handlers_scoring.go (1)
531-550: Здесь лучше уйти от N+1 запросов.
GetSessionInjectedObservationsуже возвращает весь список ID, но дальше код делает отдельныйGetObservationByIDна каждый элемент. Для endpoint-а, который будет вызываться на stop hook, это линейно раздувает latency и количество обращений к БД по мере роста сессии. Предпочтительнее добавить batch-метод в store и собирать response из одной выборки.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_scoring.go` around lines 531 - 550, The current loop uses observationStore.GetObservationByID for each ID causing N+1 DB queries; add a batch fetch method (e.g., observationStore.GetObservationsByIDs(ctx, ids) or similar) that returns a map or slice of observations, call it from the handler after GetSessionInjectedObservations, then build the []InjectedObservationResponse by mapping/ordering the returned observations (skip missing/nil entries) and extracting Title, Type, Facts exactly as before; ensure the new store method is implemented in the store interface and concrete store so the handler can replace the per-ID calls with a single batch query.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agent/plans/self-learning.md:
- Around line 23-28: Update the plan to match the current implementation:
replace the statement that guidance is injected via `<relevant-memory>` in
user-prompt.js with the correct location that guidance is rendered inside
`<engram-guidance>` in the session-start hook (the current implementation uses
plugin/hooks/session-start.js), and ensure references to MemTypeGuidance and
ObsTypeGuidance remain accurate; adjust the Phase 1 bullet list to reflect
guidance is returned via the session-start engram flow rather than the
user-prompt relevant-memory flow so debugging and rollout notes align with the
code.
In `@internal/worker/handlers_scoring.go`:
- Around line 485-488: В текущем handler-е вызов
observationStore.RecordSessionInjections(sessionID, req.IDs) игнорирует ошибку и
всё равно увеличивает глобальный счётчик injection_count, поэтому нужно либо
вернуть ошибку клиенту, либо объединить оба действия в атомарный метод;
исправьте обработчик так, чтобы при ошибке RecordSessionInjections он не
продолжал и возвращал 5xx/4xx (т.е. прекратил обработку) OR реализуйте новый
метод observationStore.RecordInjectionsAtomic(sessionID, ids) (или аналогичный,
напр. UpsertSessionAndIncrementGlobal) который в транзакции/атомарно выполняет
запись в сессионную таблицу и инкремент глобального счётчика, и замените текущие
отдельные вызовы на этот атомарный метод в том же handler-е.
- Around line 533-536: В блоке где вызывается
observationStore.GetObservationByID (всегда проверяйте и err и obs отдельно):
измените проверку так, чтобы ошибка чтения (err != nil) не просто скрывалась в
continue — логируйте ошибку и завершайте обработку запроса (или верните
500/соответствующий код) при возникновении err, а уже отдельно проверяйте obs ==
nil и только в этом случае делайте continue (это соответствует "not found").
Ссылайтесь на вызов observationStore.GetObservationByID и переменные obs/err для
внесения правок.
In `@plugin/hooks/session-start.js`:
- Around line 115-127: The current SessionStart hook only posts IDs to
/api/observations/mark-injected via lib.requestPost, so injected observations
from SessionStart never appear in per-session utility scoring (which reads
/api/sessions/{sessionId}/injected-observations in plugin/hooks/stop.js); update
the SessionStart logic (the block that collects observations, guidance, ids and
calls lib.requestPost) to also send the same ids to the per-session endpoint for
the current session (use the sessionId available in this hook) — e.g., make a
second fire-and-forget request to
/api/sessions/{sessionId}/injected-observations (or combine into a single call
to both endpoints), ensuring you reuse the ids array and handle errors similarly
to the existing lib.requestPost(...).catch(...) handling.
In `@plugin/hooks/stop.js`:
- Around line 169-199: Паттерны в correctionPatterns слишком общие и текущая
логика использует indexOf по всему assistantTextLower, из‑за чего общие слова
типа "however," дают ложные срабатывания и возвращают 'corrected'; исправьте
это, сузив набор маркеров до явных коррекций (оставить такие паттерны как
"actually", "correction:", "that's not", "incorrect", "was wrong" и
убрать/пересмотреть общие "however,", "rather,", "instead,"), и измените
проверку: для каждого term из searchTerms найдите его индекс(ы) в
assistantTextLower и только в локальном окне вокруг каждого совпадения (например
±100–200 символов) ищите паттерн с помощью indexOf(pattern, windowStart) или
явных регулярных выражений с границами слова; обновите блок, где используются
correctionPatterns, assistantTextLower, searchTerms и возврат 'corrected' чтобы
опираться на локальный контекст и более строгие паттерны.
---
Nitpick comments:
In `@internal/db/gorm/migrations.go`:
- Around line 1007-1014: The join table session_observation_injections currently
lacks foreign keys and can accumulate orphans; modify the CREATE TABLE for
session_observation_injections to add REFERENCES constraints on session_id and
observation_id (e.g. session_id REFERENCES sessions(id) ON DELETE CASCADE,
observation_id REFERENCES observations(id) ON DELETE CASCADE) so deletes on
sessions or observations cascade and maintain referential integrity; also ensure
any existing CREATE INDEX (idx_soi_session_id) still applies.
In `@internal/worker/handlers_scoring.go`:
- Around line 531-550: The current loop uses observationStore.GetObservationByID
for each ID causing N+1 DB queries; add a batch fetch method (e.g.,
observationStore.GetObservationsByIDs(ctx, ids) or similar) that returns a map
or slice of observations, call it from the handler after
GetSessionInjectedObservations, then build the []InjectedObservationResponse by
mapping/ordering the returned observations (skip missing/nil entries) and
extracting Title, Type, Facts exactly as before; ensure the new store method is
implemented in the store interface and concrete store so the handler can replace
the per-ID calls with a single batch query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c1c9b1a-bea2-46a0-88b6-3bc583e5854b
📒 Files selected for processing (10)
.agent/plans/global-roadmap.md.agent/plans/self-learning.mdinternal/db/gorm/migrations.gointernal/db/gorm/scoring_store.gointernal/worker/handlers_scoring.gointernal/worker/sdk/processor_test.gointernal/worker/service.goplugin/hooks/session-start.jsplugin/hooks/stop.jsplugin/hooks/user-prompt.js
💤 Files with no reviewable changes (1)
- .agent/plans/global-roadmap.md
|
@codex review |
🤖 PR Review MCP State (auto-managed, do not edit){
"version": 2,
"parentChildren": {},
"resolvedNitpicks": {
"coderabbit-nitpick-d06dc68a-531": {
"resolvedAt": "2026-03-07T10:40:39.185Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-47f7901b-1007": {
"resolvedAt": "2026-03-07T10:44:54.669Z",
"resolvedBy": "agent"
}
},
"updatedAt": "2026-03-07T10:44:55.072Z"
} |
- Fix N+1 query in handleGetSessionInjectedObservations (batch fetch) - Add XML injection prevention in session-start.js (escapeXmlTags) - Add FK constraints to session_observation_injections migration - Use GORM idiom in scoring_store.go (replace raw SQL with Clauses) - Improve correction detection precision in stop.js - Fix error handling in RecordSessionInjections (return 500 on failure) - Update self-learning plan doc accuracy
User got 400 trying to close issue #1 from dashboard — it was in 'reopened' state, not 'resolved'. CloseIssue was too strict. Now accepts: - resolved → closed: source confirms fix (normal flow) - reopened → closed: source decided issue no longer needed - Any state → closed: dashboard operator override Operator detection: source_project == 'dashboard'. Dashboard bypasses both state validation and source_project match checks. Bump all plugins to 3.5.3 (server parity).
User got 400 trying to close issue #1 from dashboard — it was in 'reopened' state, not 'resolved'. CloseIssue was too strict. Now accepts: - resolved → closed: source confirms fix (normal flow) - reopened → closed: source decided issue no longer needed - Any state → closed: dashboard operator override Operator detection: source_project == 'dashboard'. Dashboard bypasses both state validation and source_project match checks. Bump all plugins to 3.5.3 (server parity).
…REAKING) Consolidates the BREAKING CHANGE from PR #176 (legacy MCP/CLI removal) with two user-visible plugin tactical fixes into a single coherent v4.5.0 release. ## BREAKING CHANGES (carried over from PR #176) The 'engram-mcp' and 'engram-mcp-stdio-proxy' release artifacts no longer ship. Bare-metal users who installed via tarball or scripts/install.sh and relied on the engram-mcp binary must migrate to the plugin marketplace install path: /plugin marketplace add thebtf/engram-marketplace /plugin install engram Plugin marketplace users are unaffected — ensure-binary.js downloads only the 'engram' stdio-proxy binary, not engram-mcp. cmd/mcp/, cmd/engram-cli/, the Dockerfile client image stage, the engram-mcp Makefile + goreleaser + install.sh entries, and the bin/mcp-server / bin/mcp-stdio-proxy COMPONENTS.md sections are all gone. ## Fix #1 — MCP startup reliability after CC plugin update Smoking gun (.agent/reports/plugin-tactical-fix-triage.md): when CC updates the plugin from v4.3.0 to v4.4.x it does NOT migrate plugin-level user_config (server_url + api_token). .mcp.json then expands ${user_config.server_url} to an empty string, the binary spawns silently, and every MCP tool call fails with an opaque gRPC dial error against an empty target. Fix: - plugin/engram/.mcp.json: env block now also passes "ENGRAM_URL_LEGACY": "${ENGRAM_URL}" so users who had ENGRAM_URL in their shell from the v4.3.0 setup still get a working MCP server. - plugin/engram/scripts/run-engram.js: before spawning, fall back to ENGRAM_URL_LEGACY if ENGRAM_URL is empty, AND emit a visible WARN to stderr if both are empty so the failure mode is no longer silent. ## Fix #2 — disable noisy context injection Per .agent/reports/phase-2-synthesis.md fix #16 + the entity audits, the session-start hook currently injects 100 raw observations unioned with 10 semantic hits, plus project briefing, plus learned guidance. Reported as noise rather than relevant context. plugin/engram/hooks/session-start.js now keeps the inject GET (still needed for result.always_inject) but renders ONLY: - result.always_inject -> <user-behavior-rules> - GET /api/issues -> <engram-issues> - GET /api/issues resolved -> <engram-resolved-issues> Skipped: result.observations / result.full_count / result.project_briefing / result.guidance. mark-injected scoped to always_inject IDs only so citation tracking does not log false positives. Misleading observation- count log line removed. Save / recall MCP tools (recall_memory, store_memory, find_by_file, observations, store, feedback, vault, issues) are completely independent of this hook — separate gRPC path, unaffected. This is tactical, NOT a redesign. Phase 2 strategic work continues: citation session_id="" smoking gun, 100-obs cap policy, hook curated-render redesign — all in .agent/reports/phase-2-synthesis.md. ## Version bump Per Constitution §15 the daemon version and plugin version move together: - cmd/engram/main.go daemonVersion "v4.4.0" -> "v4.5.0" - plugin/engram/.claude-plugin/plugin.json "4.4.0" -> "4.5.0" ## Verification - JSON parse on .mcp.json + plugin.json: clean - node --check on run-engram.js + session-start.js: clean - go build ./... clean - 3-OS matrix CI green on the prior 4.4.1 commit (re-runs on amend) ## Files 4 plugin/daemon files + 1 daemon version constant. Net: -76 LOC (mostly removed render blocks in session-start.js).
…REAKING) (#177) Consolidates BREAKING from PR #176 + 2 plugin tactical fixes into v4.5.0. BREAKING: 'engram-mcp' and 'engram-mcp-stdio-proxy' release artifacts no longer ship. Bare-metal users migrate to plugin marketplace path. Plugin marketplace users unaffected (ensure-binary.js downloads only the 'engram' stdio-proxy binary). Fix #1 — MCP startup reliability after CC plugin update: - .mcp.json adds ENGRAM_URL_LEGACY = ${ENGRAM_URL} fallback - run-engram.js falls back if user_config-derived ENGRAM_URL is empty, + visible WARN to stderr if both empty (no more silent failure) Fix #2 — Disable noisy context injection: - session-start.js keeps inject GET (still needed for always_inject) but renders ONLY result.always_inject + 2 issues fetches - Skipped: result.observations / result.full_count / result.project_briefing / result.guidance - mark-injected scoped to always_inject IDs only - All MCP save/recall tools unchanged (separate gRPC path) Version bump per Constitution §15: - cmd/engram/main.go daemonVersion v4.4.0 -> v4.5.0 - plugin/engram/.claude-plugin/plugin.json 4.4.0 -> 4.5.0 Phase 2 strategic redesign continues separately — see .agent/reports/phase-2-synthesis.md for the 21-item fix priority matrix.
Summary
Changes
Self-Learning JS Hooks (b370db5)
session-start.js: guidance observation injection via<engram-guidance>blockuser-prompt.js: observation ID extraction + mark-injected fire-and-forgetstop.js: transcript parsing (ring buffer, last 50 messages) + extract-learnings callPer-Session Utility Signal Detection (a6b9583)
Server-side:
session_observation_injectionstable (session_id, observation_id, UNIQUE constraint)RecordSessionInjections/GetSessionInjectedObservationsstore methodsPOST /api/sessions/{sessionId}/mark-injected- dual-write (per-session + global counter)GET /api/sessions/{sessionId}/injected-observations- returns obs with title/type/factsClient-side:
user-prompt.js: switched from global to per-session mark-injected endpointstop.js:detectUtilitySignal()- matches injected observations against assistant messagesPOST /api/observations/{id}/utilityBug Fix
TestSafeResolvePath: replace hardcoded Unix paths withfilepath.Joinfor Windows compatibilityDocs
.agent/plans/self-learning.md- reflects actual implementation status.agent/plans/global-roadmap.mdTest plan
go build ./cmd/worker/compiles cleannode -c plugin/hooks/stop.jsandnode -c plugin/hooks/user-prompt.jspass syntax checkSummary by CodeRabbit
Примечания к выпуску
Новые функции
Улучшения