feat: add static session-start gRPC flow#199
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughДобавлены два новых gRPC-эндпоинта ( Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Plugin (frontend)
participant HTTP as HTTP Server
participant GRPC as Internal gRPC Server
participant DB as Database
Plugin->>HTTP: GET /api/context/session-start?project=...
HTTP->>GRPC: GetSessionStartContext(project, limits)
GRPC->>DB: SELECT issues WHERE project=? AND status IN (open,ack,reopened) LIMIT issuesLimit
DB-->>GRPC: rows(issues)
GRPC->>DB: SELECT memories WHERE project=? LIMIT memoriesLimit
DB-->>GRPC: rows(memories)
GRPC->>DB: SELECT rules WHERE project=? OR project IS NULL ORDER BY priority DESC, created_at DESC LIMIT rulesLimit
DB-->>GRPC: rows(rules)
GRPC-->>HTTP: {issues, memories, rules, generated_at}
HTTP-->>Plugin: JSON payload (RFC3339 timestamps)
Plugin->>Plugin: write cache (on success) / read cache (on failure)
Plugin->>HTTP: POST /api/issues/acknowledge (ack open issues)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new static context mechanism for session starts, including gRPC and HTTP endpoints for fetching issues, behavioral rules, and memories. It also implements a major-version negotiation endpoint and refactors the plugin hook to utilize these new services with a local file-based caching layer for improved resilience. Feedback focuses on refining the API design to allow for empty result sets, enforcing server-side limits on behavioral rule queries to prevent resource exhaustion, and securing the local cache files with restricted file permissions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
internal/worker/handlers_context.go (1)
803-814: Рассмотрите добавление маппинга дляcodes.PermissionDeniedиcodes.UnauthenticatedФункция
grpcCodeToHTTPобрабатывает толькоInvalidArgument,NotFoundиUnavailable. Для полноты API рассмотрите добавление:
codes.PermissionDenied→403 Forbiddencodes.Unauthenticated→401 Unauthorized♻️ Предложенное расширение
func grpcCodeToHTTP(code codes.Code) int { switch code { case codes.InvalidArgument: return http.StatusBadRequest case codes.NotFound: return http.StatusNotFound case codes.Unavailable: return http.StatusServiceUnavailable + case codes.PermissionDenied: + return http.StatusForbidden + case codes.Unauthenticated: + return http.StatusUnauthorized default: return http.StatusInternalServerError } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_context.go` around lines 803 - 814, Update the grpcCodeToHTTP function to handle authorization-related gRPC codes: add mappings for codes.PermissionDenied -> http.StatusForbidden (403) and codes.Unauthenticated -> http.StatusUnauthorized (401); modify the switch in grpcCodeToHTTP to include these two cases alongside the existing InvalidArgument, NotFound, and Unavailable branches so permission/auth failures return the correct HTTP status codes.plugin/engram/hooks/session-start.js (1)
158-166: ХелперbuildCachedSessionStartPayloadсодержит захардкоженную датуФункция
buildCachedSessionStartPayloadимеет дефолтное значениеgenerated_at: '2026-04-22T12:00:00Z'. Это выглядит как тестовые данные, "утёкшие" в production-код. Для тестового хелпера лучше использовать более очевидное значение или документировать назначение.♻️ Предложенное улучшение
function buildCachedSessionStartPayload(overrides = {}) { return { issues: [], rules: [], memories: [], - generated_at: '2026-04-22T12:00:00Z', + generated_at: new Date().toISOString(), // or document this is a test helper ...overrides, }; }Если это намеренно для тестов, рассмотрите перенос в тестовый файл или добавление JSDoc-комментария.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/engram/hooks/session-start.js` around lines 158 - 166, The helper buildCachedSessionStartPayload contains a hardcoded generated_at timestamp that looks like leaked test data; change it to a dynamic default (e.g. generated_at: new Date().toISOString()) or set generated_at to null/omit it and let callers/tests inject a stable value, and if the fixed timestamp is intentional for tests either move this helper into the test suite or add a JSDoc explaining its purpose and why the static date is required; update references to buildCachedSessionStartPayload accordingly.plugin/engram/hooks/lib.js (1)
149-156:writeJSONFileможет выбросить исключение при ошибке записиВ отличие от
readJSONFile, которая безопасно возвращаетnullпри ошибках,writeJSONFileне обрабатывает исключения отmkdirSyncиwriteFileSync. Это может привести к неперехваченным исключениям при проблемах с файловой системой (нет прав, диск заполнен и т.д.).Учитывая, что кеширование — некритичная операция, рассмотрите добавление try-catch для согласованности с паттерном "silent fail" в
readJSONFile.♻️ Предложенное исправление
function writeJSONFile(filePath, value) { if (!filePath) { return; } - const parentDir = path.dirname(filePath); - fs.mkdirSync(parentDir, { recursive: true }); - fs.writeFileSync(filePath, JSON.stringify(value, null, 2), 'utf8'); + try { + const parentDir = path.dirname(filePath); + fs.mkdirSync(parentDir, { recursive: true }); + fs.writeFileSync(filePath, JSON.stringify(value, null, 2), 'utf8'); + } catch { + // Cache write is best-effort; failures are non-critical + } }🤖 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 149 - 156, The writeJSONFile function currently may throw from fs.mkdirSync or fs.writeFileSync; wrap the body of writeJSONFile in a try-catch so filesystem errors are swallowed similarly to readJSONFile (i.e., do not throw for cache write failures), optionally returning a boolean (true on success, false on failure) or undefined on error; reference the writeJSONFile function and use the same silent-fail pattern as readJSONFile to keep behavior consistent across the module.internal/worker/service.go (1)
856-867: Некорректный отступ в группе маршрутовБлок маршрутов для "Context injection" имеет лишний уровень отступа по сравнению с остальными маршрутами в группе
s.requireReady. Это нарушает согласованность форматирования кода.♻️ Предложенное исправление форматирования
- // Context injection - r.Get("/api/context/count", s.handleContextCount) - r.Post("/api/context/inject", s.handleContextInject) - r.Get("/api/context/inject", s.handleContextInject) // deprecated — use POST - r.Post("/api/context/session-start", s.handleSessionStartContextStatic) - r.Get("/api/context/session-start", s.handleSessionStartContextStatic) - r.Get("/api/context/search", s.handleSearchByPrompt) - r.Post("/api/context/search", s.handleSearchByPrompt) - r.Get("/api/context/files", s.handleFileContext) - r.Get("/api/context/by-file", s.handleContextByFile) - r.Post("/api/memory/triggers", s.handleMemoryTriggers) - r.Post("/api/decisions/search", s.handleSearchDecisions) + // Context injection + r.Get("/api/context/count", s.handleContextCount) + r.Post("/api/context/inject", s.handleContextInject) + r.Get("/api/context/inject", s.handleContextInject) // deprecated — use POST + r.Post("/api/context/session-start", s.handleSessionStartContextStatic) + r.Get("/api/context/session-start", s.handleSessionStartContextStatic) + r.Get("/api/context/search", s.handleSearchByPrompt) + r.Post("/api/context/search", s.handleSearchByPrompt) + r.Get("/api/context/files", s.handleFileContext) + r.Get("/api/context/by-file", s.handleContextByFile) + r.Post("/api/memory/triggers", s.handleMemoryTriggers) + r.Post("/api/decisions/search", s.handleSearchDecisions)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/service.go` around lines 856 - 867, The "Context injection" routes are indented one level deeper than the other routes in the s.requireReady group; align their indentation with the rest of the s.requireReady route registrations by removing the extra indentation so the entries like r.Get("/api/context/count", s.handleContextCount), r.Post("/api/context/inject", s.handleContextInject), r.Get("/api/context/inject", s.handleContextInject), r.Post("/api/context/session-start", s.handleSessionStartContextStatic), r.Get("/api/context/session-start", s.handleSessionStartContextStatic), r.Get("/api/context/search", s.handleSearchByPrompt), r.Post("/api/context/search", s.handleSearchByPrompt), r.Get("/api/context/files", s.handleFileContext), r.Get("/api/context/by-file", s.handleContextByFile), r.Post("/api/memory/triggers", s.handleMemoryTriggers), and r.Post("/api/decisions/search", s.handleSearchDecisions) match the same indentation level as the other route registrations in the s.requireReady block.internal/grpcserver/session_start_test.go (1)
113-114: Уберитеtime.Sleepиз тестов, чтобы снизить флаки.На Line 113-114 и 143-144 порядок данных делается через паузы. Надёжнее выставлять
created_atдетерминированно (через update) или сортировать по явному полю в seed.Also applies to: 143-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/grpcserver/session_start_test.go` around lines 113 - 114, Remove the brittle time.Sleep calls used to order created memories in session_start_test.go (around memoryStore.Create calls) and instead create deterministic ordering by explicitly setting the created_at timestamps on the Memory objects (or updating them after Create) or by asserting against a sorted order using an explicit field; update the tests that call memoryStore.Create with models.Memory to assign deterministic created_at values (or call an UpdateCreatedAt helper) and/or sort the returned seed list by created_at when asserting, replacing the Sleep at both the locations referenced.
🤖 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/grpcserver/session_start_test.go`:
- Around line 25-28: The current test only checks DATABASE_DSN presence but
later runs destructive DELETEs; update the test setup (where dsn is read into
the dsn variable in session_start_test.go, e.g. inside TestSessionStart) to
verify the DSN points to a test database before proceeding: if dsn does not
match an allowed test pattern (for example contains "test", "localhost", a
specific test DB name, or matches a configured TEST_DATABASE_DSN regex) then
call t.Skip with a clear message; additionally explicitly reject known
production/staging hosts/containers (e.g., hostnames like "prod", "production",
or your staging domain) to ensure the destructive cleanup never runs against
non-test DBs.
In `@internal/grpcserver/session_start.go`:
- Around line 54-55: The gRPC responses currently include raw DB/driver errors
via status.Errorf(codes.Internal, "…%v", err) (seen in the three occurrences of
status.Errorf(codes.Internal, ...) around the session-start listing code);
instead remove the %v and return a neutral error string (e.g. "failed to list
session-start issues") and log the detailed err server-side; update each
occurrence (the status.Errorf calls at the three locations) to return a generic
message and call your server logger (e.g. serverLogger.Errorf or log.Printf) to
record the full err and any context before returning the neutral gRPC error.
- Around line 28-45: Добавьте проверку верхнего предела для
req.GetMemoriesLimit() и req.GetIssuesLimit(): объявите константы
maxSessionStartMemoriesLimit и maxSessionStartIssuesLimit, затем после
существующей проверки >=0 (и перед присвоением переменным memoriesLimit и
issuesLimit) либо вернуть status.Error(codes.InvalidArgument, "... must be <=
<max>") при превышении, либо безопасно капировать значение до соответствующего
max (например memoriesLimit = min(int(req.GetMemoriesLimit()),
maxSessionStartMemoriesLimit)); обновите логику использования
defaultSessionStartMemoriesLimit/defaultSessionStartIssuesLimit чтобы применять
их только для нулевого значения. Укажите эти константы и используйте
req.GetMemoriesLimit(), req.GetIssuesLimit(), memoriesLimit, issuesLimit и s.db
в правках.
---
Nitpick comments:
In `@internal/grpcserver/session_start_test.go`:
- Around line 113-114: Remove the brittle time.Sleep calls used to order created
memories in session_start_test.go (around memoryStore.Create calls) and instead
create deterministic ordering by explicitly setting the created_at timestamps on
the Memory objects (or updating them after Create) or by asserting against a
sorted order using an explicit field; update the tests that call
memoryStore.Create with models.Memory to assign deterministic created_at values
(or call an UpdateCreatedAt helper) and/or sort the returned seed list by
created_at when asserting, replacing the Sleep at both the locations referenced.
In `@internal/worker/handlers_context.go`:
- Around line 803-814: Update the grpcCodeToHTTP function to handle
authorization-related gRPC codes: add mappings for codes.PermissionDenied ->
http.StatusForbidden (403) and codes.Unauthenticated -> http.StatusUnauthorized
(401); modify the switch in grpcCodeToHTTP to include these two cases alongside
the existing InvalidArgument, NotFound, and Unavailable branches so
permission/auth failures return the correct HTTP status codes.
In `@internal/worker/service.go`:
- Around line 856-867: The "Context injection" routes are indented one level
deeper than the other routes in the s.requireReady group; align their
indentation with the rest of the s.requireReady route registrations by removing
the extra indentation so the entries like r.Get("/api/context/count",
s.handleContextCount), r.Post("/api/context/inject", s.handleContextInject),
r.Get("/api/context/inject", s.handleContextInject),
r.Post("/api/context/session-start", s.handleSessionStartContextStatic),
r.Get("/api/context/session-start", s.handleSessionStartContextStatic),
r.Get("/api/context/search", s.handleSearchByPrompt),
r.Post("/api/context/search", s.handleSearchByPrompt),
r.Get("/api/context/files", s.handleFileContext), r.Get("/api/context/by-file",
s.handleContextByFile), r.Post("/api/memory/triggers", s.handleMemoryTriggers),
and r.Post("/api/decisions/search", s.handleSearchDecisions) match the same
indentation level as the other route registrations in the s.requireReady block.
In `@plugin/engram/hooks/lib.js`:
- Around line 149-156: The writeJSONFile function currently may throw from
fs.mkdirSync or fs.writeFileSync; wrap the body of writeJSONFile in a try-catch
so filesystem errors are swallowed similarly to readJSONFile (i.e., do not throw
for cache write failures), optionally returning a boolean (true on success,
false on failure) or undefined on error; reference the writeJSONFile function
and use the same silent-fail pattern as readJSONFile to keep behavior consistent
across the module.
In `@plugin/engram/hooks/session-start.js`:
- Around line 158-166: The helper buildCachedSessionStartPayload contains a
hardcoded generated_at timestamp that looks like leaked test data; change it to
a dynamic default (e.g. generated_at: new Date().toISOString()) or set
generated_at to null/omit it and let callers/tests inject a stable value, and if
the fixed timestamp is intentional for tests either move this helper into the
test suite or add a JSDoc explaining its purpose and why the static date is
required; update references to buildCachedSessionStartPayload accordingly.
🪄 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: e54fec1b-32d5-4c6b-9659-e9ed35598425
⛔ Files ignored due to path filters (2)
proto/engram/v1/engram.pb.gois excluded by!**/*.pb.goproto/engram/v1/engram_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
internal/grpcserver/session_start.gointernal/grpcserver/session_start_test.gointernal/grpcserver/version_negotiate.gointernal/grpcserver/version_negotiate_test.gointernal/worker/handlers_context.gointernal/worker/handlers_context_session_start_test.gointernal/worker/service.goplugin/engram/hooks/lib.jsplugin/engram/hooks/session-start.jsplugin/engram/hooks/session-start.test.jsproto/engram/v1/engram.proto
Owner
Author
🤖 PR Review MCP State (auto-managed, do not edit){
"version": 2,
"parentChildren": {},
"resolvedNitpicks": {
"coderabbit-nitpick-87633c09-803": {
"resolvedAt": "2026-04-22T22:25:00.617Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-cdaeb395-158": {
"resolvedAt": "2026-04-22T22:25:03.237Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-936107ef-149": {
"resolvedAt": "2026-04-22T22:25:06.106Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-5093434e-856": {
"resolvedAt": "2026-04-22T22:25:08.915Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-bd29bda0-113": {
"resolvedAt": "2026-04-22T22:25:11.380Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-abc045c8-126": {
"resolvedAt": "2026-04-22T22:26:27.520Z",
"resolvedBy": "agent"
}
},
"updatedAt": "2026-04-22T22:26:28.358Z"
} |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/grpcserver/session_start_test.go (1)
126-126: Использованиеtime.Sleepдля упорядочивания записей.Паузы в 5мс для создания различных
created_atвременных меток работают, но могут быть нестабильными под нагрузкой или при низкой точности временных меток БД. Рекомендуется рассмотреть явную установку временных меток через фикстуры, если тесты станут нестабильными.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/grpcserver/session_start_test.go` at line 126, Вместо ненадёжного вызова time.Sleep(5 * time.Millisecond) в тесте (строка с time.Sleep) задайте детерминированные created_at через фикстуры или мок часов: обновите код теста в session_start_test.go чтобы явно присваивать временные метки нужным записям (например через helper/fixture setCreatedAt или через внедряемый Clock/Now mock) или используйте фиктивный clock, который выдвигает время вперёд на ожидаемую разницу; уберите sleep и убедитесь, что сравнения/сортировка используют эти явные created_at значения.
🤖 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/grpcserver/session_start_test.go`:
- Line 126: Вместо ненадёжного вызова time.Sleep(5 * time.Millisecond) в тесте
(строка с time.Sleep) задайте детерминированные created_at через фикстуры или
мок часов: обновите код теста в session_start_test.go чтобы явно присваивать
временные метки нужным записям (например через helper/fixture setCreatedAt или
через внедряемый Clock/Now mock) или используйте фиктивный clock, который
выдвигает время вперёд на ожидаемую разницу; уберите sleep и убедитесь, что
сравнения/сортировка используют эти явные created_at значения.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09179efd-24a9-4213-89b4-bbfb0745355b
📒 Files selected for processing (6)
internal/grpcserver/session_start.gointernal/grpcserver/session_start_test.gointernal/worker/handlers_context.gointernal/worker/service.goplugin/engram/hooks/lib.jsplugin/engram/hooks/session-start.js
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/grpcserver/session_start.go
- plugin/engram/hooks/lib.js
- plugin/engram/hooks/session-start.js
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
GetSessionStartContextandNegotiateVersionto the Engram gRPC APIScope
Implements US13 / engram#121:
Verification
node --test plugin/engram/hooks/session-start.test.jsgo test ./internal/grpcserver ./internal/worker -count=1go build ./...Notes
/api/context/injectendpoint in place for compatibility, but it is no longer the primary session-start pathSummary by CodeRabbit
Новые возможности
Tests