feat: deterministic source project identity via stdio proxy (#40)#140
Conversation
ResolveProjectSlug computes project slug from git remote URL, identical to the plugin's ProjectIDWithName algorithm: SHA-256(remoteURL + "/" + relativePath)[:8]. Worktrees produce the same hash suffix as the main repo. Fallback to path-based slug when git is unavailable. 4 unit tests covering git repo, non-git dir, consistency, and worktree.
- context.go: extractProjectFromHeader, contextWithProject, projectFromContext for X-Engram-Project header → context.Context pipeline - config.go: EnforceSourceProject bool (default true), env ENGRAM_ENFORCE_SOURCE_PROJECT
…3-T006] - T003: ENGRAM_URL/ENGRAM_API_TOKEN env vars with CLI flag override - T004: ResolveProjectSlug at startup, log to stderr - T005: X-Engram-Project header on all HTTP requests - T006: Streamable HTTP (POST /mcp) with SSE fallback
…8, T009] - streamable.go: extract header, store in context before handleRequest - sse.go: extract header in handleMessage, store in context - tools_issues.go: all write handlers use projectFromContext when EnforceSourceProject=true - tools_memory.go: store uses projectFromContext for observation project - tools_store_consolidated.go: extract uses projectFromContext - Read handlers (recall, issues list) unchanged — cross-project reads allowed
- .mcp.json: switch from HTTP to command-based transport (engram binary) - plugin.json: version bump 3.7.5 → 3.7.6 - context_test.go: 4 unit tests for header extraction and context roundtrip
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughПереорганизация MCP-прокси с введением контекстной передачи проекта: удалён старый Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review focus:security |
![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 an engram stdio proxy for MCP endpoints, supporting SSE and Streamable HTTP, and implements project identity resolution using git metadata. The changes include new configuration options to enforce project scoping across tool handlers via HTTP headers and context. Feedback highlights the need for increased bufio.Scanner buffer sizes to handle large messages, the addition of timeouts to HTTP clients for production readiness, and improved error handling in the proxy's forwarding logic. Refactoring redundant project resolution code and enhancing git command error reporting were also recommended.
🧩 Analysis chain🏁 Script executed: Length of output: 107016
🔒 Security Review — PR
|
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | 🔴 HIGH | streamable.go, sse.go |
CORS wildcard on authenticated endpoints |
| 2 | 🔴 HIGH | context.go |
No validation of X-Engram-Project header value |
| 3 | 🟡 MEDIUM | all tool handlers | EnforceSourceProject fallback allows agent spoofing without proxy |
| 4 | 🟡 MEDIUM | main.go |
Git remote URL with credentials logged to stderr |
| 5 | 🟡 MEDIUM | main.go |
No body/scanner size limits → OOM + silent JSON truncation |
| 6 | 🟡 MEDIUM | main.go |
SSE endpoint SSRF via server-controlled redirect path |
| 7 | 🔵 LOW | main.go |
Bearer token exposed in process listing via --token flag |
| 8 | 🔵 LOW | identity.go |
32-bit hash collision space for project slug |
| 9 | 🔵 LOW | streamable.go, sse.go |
X-Engram-Project missing from CORS Allow-Headers |
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
internal/proxy/identity.go (1)
51-74: Рассмотрите использование единого контекста для обеих git-операций.Функция
getGitInfoсоздаёт два отдельных контекста с таймаутами (строки 52-53 и 64-65). Если первая команда git выполняется близко к лимиту в 3 секунды, общее время ожидания может достигнуть 6 секунд. Возможно, стоит использовать единый контекст с общим таймаутом для обеих операций.Текущая реализация функциональна, так как это происходит только один раз при старте прокси и не блокирует критический путь.
♻️ Опциональный рефакторинг с единым контекстом
func getGitInfo(cwd string) (remoteURL, relativePath string, err error) { - ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() rawRemote, err := runGit(ctx, cwd, "remote", "get-url", "origin") if err != nil { return "", "", err } remoteURL = strings.TrimSpace(rawRemote) if remoteURL == "" { return "", "", fmt.Errorf("empty remote URL") } - ctx2, cancel2 := context.WithTimeout(context.Background(), 3*time.Second) - defer cancel2() - - rawPrefix, err := runGit(ctx2, cwd, "rev-parse", "--show-prefix") + rawPrefix, err := runGit(ctx, cwd, "rev-parse", "--show-prefix") if err != nil { return "", "", err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/proxy/identity.go` around lines 51 - 74, getGitInfo creates two separate 3s contexts so the two runGit calls can cumulatively wait up to 6s; change it to create a single context with timeout (e.g. context.WithTimeout(..., 3*time.Second)) at the start of getGitInfo and reuse that ctx for both runGit calls (remote get-url and rev-parse) and call cancel once via defer; update references to ctx2/cancel2 to use the single ctx and cancel to ensure both operations share the same overall timeout while keeping function behavior otherwise unchanged.internal/proxy/identity_test.go (1)
114-121: Windows-специфичная нормализация путей.Строки 116-117 используют жёстко заданный разделитель
\для Windows. На Unix-системах это приведёт к некорректному сравнению путей. Рассмотрите использованиеfilepath.Cleanиstrings.EqualFoldбез ручной замены разделителей, либо используйтеfilepath.ToSlashдля унификации.♻️ Кроссплатформенная нормализация путей
- // Normalise separators for comparison. - normPath := strings.ReplaceAll(path, "/", `\`) - normMain := strings.ReplaceAll(mainRepo, "/", `\`) - if !strings.EqualFold(normPath, normMain) { + // Normalise using filepath for cross-platform compatibility. + normPath := filepath.Clean(path) + normMain := filepath.Clean(mainRepo) + if !strings.EqualFold(normPath, normMain) { worktreePaths = append(worktreePaths, path) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/proxy/identity_test.go` around lines 114 - 121, The Windows-only replacement of separators (`strings.ReplaceAll(..., "/", "\\")`) causes incorrect comparisons on non-Windows platforms; in the block handling `path`, replace the manual ReplaceAll logic with cross-platform normalization using `filepath.Clean` and a consistent slash form (e.g., `filepath.ToSlash`) for both `path` and `mainRepo` before comparing with `strings.EqualFold`, and add the `path/filepath` import; keep the same logic that appends to `worktreePaths` when the cleaned, normalized values (e.g., `normPath := filepath.ToSlash(filepath.Clean(path))` and `normMain := filepath.ToSlash(filepath.Clean(mainRepo))`) are not equal by `strings.EqualFold`.internal/mcp/tools_issues.go (1)
330-338: Рассмотрите возможность выделения в хелпер-функцию.Один и тот же 8-строчный блок логики повторяется 5 раз в этом файле (create, update, comment, reopen, close). Это можно было бы вынести в хелпер:
♻️ Опциональный рефакторинг
// resolveSourceProject returns project from context when enforcement is enabled, // falling back to the explicit parameter. func resolveSourceProject(ctx context.Context, m map[string]any) string { if config.Get().EnforceSourceProject { if p := projectFromContext(ctx); p != "" { return p } } return coerceString(m["project"], "") }Использование:
sourceProject := resolveSourceProject(ctx, m)Also applies to: 368-376, 398-406, 422-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/tools_issues.go` around lines 330 - 338, Extract the repeated 8-line logic that determines sourceProject into a helper function (e.g., resolveSourceProject(ctx context.Context, m map[string]any) string) that uses config.Get().EnforceSourceProject, projectFromContext(ctx) and falls back to coerceString(m["project"], "") when needed; then replace the duplicated blocks in the create/update/comment/reopen/close handlers with a single call sourceProject := resolveSourceProject(ctx, m) to reduce duplication and improve clarity.cmd/engram/main.go (2)
103-104: Ограничение размера буфера Scanner.
bufio.Scannerпо умолчанию имеет буфер 64KB. Если JSON-RPC сообщение превысит этот размер, оно будет обрезано. Рекомендуется увеличить буфер для надёжности.🛡️ Предлагаемое исправление
scanner := bufio.NewScanner(os.Stdin) +scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) // 1MB max for scanner.Scan() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/engram/main.go` around lines 103 - 104, The bufio.Scanner created with bufio.NewScanner(os.Stdin) uses a 64KB token buffer by default, so increase its capacity by calling scanner.Buffer(...) to provide a larger initial slice and a larger max token size (e.g., several megabytes) to avoid truncating large JSON-RPC messages; modify the code around the scanner variable and the loop where scanner.Scan() is used (referencing scanner and bufio.NewScanner) to call scanner.Buffer and then after the loop check scanner.Err() and handle/report any scanning error.
162-167: SSE-соединение также требует таймаута на подключение.Для SSE используется долгоживущее соединение, но начальное подключение должно иметь таймаут. Рассмотрите использование контекста с таймаутом для начального запроса.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/engram/main.go` around lines 162 - 167, The SSE connection currently uses http.DefaultClient.Do(req) without a connection timeout; wrap the initial request in a context with timeout (e.g., ctx, cancel := context.WithTimeout(req.Context(), connectTimeout) and defer cancel()) and attach it to the request via req = req.WithContext(ctx) before calling http.DefaultClient.Do(req) so the initial connect will fail after the timeout while still allowing the long-lived resp.Body stream to continue once established.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/engram/main.go`:
- Around line 227-229: В функции forwardStdin замените тихий выход с os.Exit(0)
на корректный обработчик ошибки: логируйте ошиб (через log.Fatalf /
processLogger или fmt.Fprintf(os.Stderr,...)) и завершайте процесс с ненулевым
кодом (например os.Exit(1)); если поведение с кодом 0 было намеренным, оставьте
os.Exit(0) и добавьте явный комментарий с объяснением причины. Проверьте и
аналогично исправьте второе место (строки около 241-243) чтобы все ошибки
HTTP/запросов не маскировались нулевым кодом выхода, ссылаясь на функцию
forwardStdin и соответствующие os.Exit вызовы.
- Around line 77-80: Исправьте использование http.DefaultClient без таймаута:
добавьте импорт "time", создайте в том же пакете глобальный или локальный
httpClient := &http.Client{Timeout: 10 * time.Second} (или другой подходящий
таймаут) и замените все вызовы http.DefaultClient.Do(req) (включая места, где
используются переменные resp/req) на httpClient.Do(req); убедитесь, что
обработка ошибок и закрытие тела ответа остаются прежними.
In `@internal/proxy/identity_test.go`:
- Around line 69-92: The test uses a hardcoded Windows path (const repoDir =
`D:\Dev\engram`) which breaks portability; change
TestResolveProjectSlug_ConsistentAcrossCalls to create and use a
platform-independent temporary directory (e.g., call t.TempDir() or os.MkdirTemp
and assign to repoDir) and use that variable for both ResolveProjectSlug calls
so the test runs consistently across OSes; update the reference to repoDir in
the test and ensure any necessary minimal setup for a repo path is performed
before calling proxy.ResolveProjectSlug.
- Around line 94-125: The test TestResolveProjectSlug_WorktreeMatchesMain (and
the other two tests that share the same hard-coded mainRepo constant) use a
fixed path `D:\Dev\engram`, which breaks on other machines; replace that
constant with a dynamic setup: either discover the repository root at runtime
(e.g., use os.Getwd or read GIT_DIR to locate the current repo) or create an
isolated test repo using t.TempDir() and run git init + any required git
worktree commands to produce the same conditions; update the constant mainRepo
and any test helpers in TestResolveProjectSlug_WorktreeMatchesMain (and the two
sibling tests) to use the dynamic path so the tests run in any environment.
- Around line 16-42: The test TestResolveProjectSlug_GitRepo uses a hard-coded
Windows path (repoDir = `D:\Dev\engram`) which breaks portability; replace that
with a dynamic repo root lookup (e.g., add a helper like findRepoRoot that runs
`git rev-parse --show-toplevel` and trims whitespace) and pass its result to
proxy.ResolveProjectSlug instead of the fixed path; ensure the helper fails the
test via t.Fatalf on error so ResolveProjectSlug is always called with a valid
repo root.
---
Nitpick comments:
In `@cmd/engram/main.go`:
- Around line 103-104: The bufio.Scanner created with bufio.NewScanner(os.Stdin)
uses a 64KB token buffer by default, so increase its capacity by calling
scanner.Buffer(...) to provide a larger initial slice and a larger max token
size (e.g., several megabytes) to avoid truncating large JSON-RPC messages;
modify the code around the scanner variable and the loop where scanner.Scan() is
used (referencing scanner and bufio.NewScanner) to call scanner.Buffer and then
after the loop check scanner.Err() and handle/report any scanning error.
- Around line 162-167: The SSE connection currently uses
http.DefaultClient.Do(req) without a connection timeout; wrap the initial
request in a context with timeout (e.g., ctx, cancel :=
context.WithTimeout(req.Context(), connectTimeout) and defer cancel()) and
attach it to the request via req = req.WithContext(ctx) before calling
http.DefaultClient.Do(req) so the initial connect will fail after the timeout
while still allowing the long-lived resp.Body stream to continue once
established.
In `@internal/mcp/tools_issues.go`:
- Around line 330-338: Extract the repeated 8-line logic that determines
sourceProject into a helper function (e.g., resolveSourceProject(ctx
context.Context, m map[string]any) string) that uses
config.Get().EnforceSourceProject, projectFromContext(ctx) and falls back to
coerceString(m["project"], "") when needed; then replace the duplicated blocks
in the create/update/comment/reopen/close handlers with a single call
sourceProject := resolveSourceProject(ctx, m) to reduce duplication and improve
clarity.
In `@internal/proxy/identity_test.go`:
- Around line 114-121: The Windows-only replacement of separators
(`strings.ReplaceAll(..., "/", "\\")`) causes incorrect comparisons on
non-Windows platforms; in the block handling `path`, replace the manual
ReplaceAll logic with cross-platform normalization using `filepath.Clean` and a
consistent slash form (e.g., `filepath.ToSlash`) for both `path` and `mainRepo`
before comparing with `strings.EqualFold`, and add the `path/filepath` import;
keep the same logic that appends to `worktreePaths` when the cleaned, normalized
values (e.g., `normPath := filepath.ToSlash(filepath.Clean(path))` and `normMain
:= filepath.ToSlash(filepath.Clean(mainRepo))`) are not equal by
`strings.EqualFold`.
In `@internal/proxy/identity.go`:
- Around line 51-74: getGitInfo creates two separate 3s contexts so the two
runGit calls can cumulatively wait up to 6s; change it to create a single
context with timeout (e.g. context.WithTimeout(..., 3*time.Second)) at the start
of getGitInfo and reuse that ctx for both runGit calls (remote get-url and
rev-parse) and call cancel once via defer; update references to ctx2/cancel2 to
use the single ctx and cancel to ensure both operations share the same overall
timeout while keeping function behavior otherwise unchanged.
🪄 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: 0b491792-41e9-4fd8-bd68-1057394031ae
📒 Files selected for processing (14)
cmd/engram/main.gocmd/mcp-stdio-proxy/main.gointernal/config/config.gointernal/mcp/context.gointernal/mcp/context_test.gointernal/mcp/sse.gointernal/mcp/streamable.gointernal/mcp/tools_issues.gointernal/mcp/tools_memory.gointernal/mcp/tools_store_consolidated.gointernal/proxy/identity.gointernal/proxy/identity_test.goplugin/engram/.claude-plugin/plugin.jsonplugin/engram/.mcp.json
💤 Files with no reviewable changes (1)
- cmd/mcp-stdio-proxy/main.go
- Replace http.DefaultClient with httpClient (30s timeout) in cmd/engram/main.go
to prevent indefinite hangs on unresponsive servers (MAJOR)
- Add stderr logging to forwardStdin error paths; explain intentional exit 0
rationale in comments (MINOR)
- Replace hardcoded D:\Dev\engram paths in identity_test.go with findRepoRoot()
helper that runs git rev-parse --show-toplevel (MAJOR x3)
- Use filepath.Clean for cross-platform worktree path comparison (MINOR)
- Fix test assertion to check slug prefix against actual dirName, not hardcoded
"engram_" — allows tests to pass from any worktree
Reviewers: CodeRabbit
Comment ids: PRRC_kwDORY-ek862--Xc, PRRC_kwDORY-ek862--Xk, PRRC_kwDORY-ek862--Xm,
PRRC_kwDORY-ek862--Xo, PRRC_kwDORY-ek862--Xh
- internal/proxy/identity.go: use single shared context for both git calls
in getGitInfo so total wall time is bounded to 5s (not 6s)
- internal/proxy/identity.go: capture stderr from failed git commands via
exec.Cmd.Stderr so error messages include diagnostic text (e.g. "not a git
repository") rather than bare exit-status strings
- cmd/engram/main.go: increase bufio.Scanner buffer to 4 MB on all three
scanners (runStreamableHTTP stdin, runSSE body, forwardStdin stdin) to
handle large JSON-RPC messages without truncation
Comment ids: coderabbit-nitpick-c5b27a4b-51, coderabbit-nitpick-354bdef6-103,
PRRC_kwDORY-ek862-6uq, PRRC_kwDORY-ek862-6ur, PRRC_kwDORY-ek862-6u6
🤖 PR Review MCP State (auto-managed, do not edit){
"version": 2,
"parentChildren": {},
"resolvedNitpicks": {
"coderabbit-nitpick-c5b27a4b-51": {
"resolvedAt": "2026-04-12T18:37:54.620Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-dcf30349-114": {
"resolvedAt": "2026-04-12T18:37:58.293Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-354bdef6-103": {
"resolvedAt": "2026-04-12T18:38:01.315Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-c5c109b8-330": {
"resolvedAt": "2026-04-12T18:41:45.809Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-c2356718-162": {
"resolvedAt": "2026-04-12T18:43:59.996Z",
"resolvedBy": "agent"
},
"coderabbit-nitpick-2b5e5370-240": {
"resolvedAt": "2026-04-12T18:48:36.361Z",
"resolvedBy": "agent"
}
},
"updatedAt": "2026-04-12T18:48:36.720Z"
} |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/engram/main.go`:
- Around line 52-53: Вместо прямого вывода переменной remote в stderr (в коде
рядом с вызовом ResolveProjectSlug и переменной remote) уберите возможный
userinfo: распарсьте remote через net/url.Parse, обнулите поле User (или
извлеките только Host+Path) и логируйте безопасную часть (host/path) либо строку
URL без userinfo; используйте полученное значение в fmt.Fprintf вместо исходного
remote, чтобы предотвратить утечку токенов в логах/CI.
- Around line 138-142: Удалите лишний вызов fmt.Fprintln(os.Stdout) в
cmd/engram/main.go: оставьте io.Copy(os.Stdout, resp.Body) и resp.Body.Close(),
но уберите строку, которая добавляет дополнительный перевод строки; серверная
сторона (json.NewEncoder(w).Encode) уже добавляет нужную newline, поэтому
удаление fmt.Fprintln устранит пустую строку между JSON-документами.
- Line 110: The bufio.Scanner instances used to read JSON-RPC from stdin and SSE
event streams in runStreamableHTTP and forwardStdin hit the default 64KiB token
limit; update each scanner (the scanner created with bufio.NewScanner in
runStreamableHTTP, the SSE response scanner in the event stream handling, and
the scanner in forwardStdin) to call scanner.Buffer(make([]byte,
bufio.MaxScanTokenSize), maxFrameSize) (or similar) to raise the max token size
to the provided maxFrameSize constant; apply this change to all three scanner
variables so long JSON-RPC messages and SSE events don’t produce "token too
long" errors.
- Around line 229-263: forwardStdin currently calls os.Exit(0) on EOF and on
POST errors which forcibly kills the whole process from a goroutine; change
forwardStdin to stop calling os.Exit and instead signal its completion/error to
the SSE controller (runSSE/main) via a channel or an errgroup.Context so the
main reader can finish processing buffered SSE messages before exiting.
Specifically, remove os.Exit calls in forwardStdin, have forwardStdin return (or
send an error/nil on a provided doneCh/errorCh) when scanner.Scan() ends or
httpClient.Do fails, and update the caller of forwardStdin/runSSE to wait on
that signal and perform graceful shutdown/finalization and then exit if
appropriate.
- Around line 94-101: The probe in tryStreamableHTTP currently uses
io.ReadAll(resp.Body) which can exhaust memory for large responses; change it to
read with a size limit (e.g., wrap resp.Body with io.LimitReader or use
json.Decoder with an enforced limit) and validate JSON only within that limit.
Add a clear constant like maxProbeBodySize and replace the io.ReadAll call with
a limited read (or stream-decoding) so json.Unmarshal/json.Decoder only sees at
most maxProbeBodySize bytes and the probe returns an error if the response
exceeds that limit.
🪄 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: b79cfa1e-8272-4a6a-95b0-c234056c7f92
📒 Files selected for processing (2)
cmd/engram/main.gointernal/proxy/identity_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/proxy/identity_test.go
- internal/proxy/identity.go: use a single shared context for both runGit
calls in getGitInfo so the total timeout is bounded to 5s (not 6s)
- internal/proxy/identity.go: capture git stderr in runGit error messages
so "git identity failed" log includes diagnostic text (e.g. "not a git
repository") instead of a bare exit-status
- internal/mcp/tools_issues.go: extract repeated 8-line sourceProject
resolution into resolveSourceProject helper; replace all 5 duplicate
blocks across create/update-comment/comment/reopen/close handlers
Note: resolveSourceProject preserves the existing fallback-to-parameter
semantics when the header is absent under EnforceSourceProject — changing
that behavior to an error would be a breaking design change requiring
explicit user approval.
Comment ids: coderabbit-nitpick-c5b27a4b-51, coderabbit-nitpick-c5c109b8-330,
PRRC_kwDORY-ek862-6u3, PRRC_kwDORY-ek862-6u6
…tions The shared httpClient has a 30s Timeout which would kill any SSE stream after 30 seconds. Introduce a separate sseClient backed by a custom Transport with a 15s dial/TLS-handshake timeout but no response timeout, so the initial connection fails fast while the long-lived stream stays open. httpClient (30s timeout) — probe ping, Streamable HTTP POSTs sseClient (dial-timeout only) — SSE GET + SSE message POSTs Comment id: coderabbit-nitpick-c2356718-162
…ndings
- Redact userinfo from git remote URL before logging to stderr to prevent
token leaks in CI output and bug reports (MAJOR)
Uses net/url.Parse → u.User = nil to strip embedded credentials
- Limit probe response body to 64 KB using io.LimitReader to prevent an
untrusted server from exhausting proxy memory (MAJOR)
- Remove extra fmt.Fprintln(os.Stdout) after io.Copy in runStreamableHTTP;
json.NewEncoder on the server already appends a newline — the extra call
was emitting a blank line between JSON documents, breaking JSON Lines
consumers (MINOR)
- Refactor forwardStdin to accept a done chan<-struct{} and close it on
return instead of calling os.Exit from a goroutine; runSSE now waits on
that channel before exiting so in-flight POSTs can complete gracefully
without racing against os.Exit (MAJOR)
Comment ids: PRRC_kwDORY-ek862_EKm, PRRC_kwDORY-ek862_EKo,
PRRC_kwDORY-ek862_EKv, PRRC_kwDORY-ek862_EKx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cmd/engram/main.go (3)
143-144:⚠️ Potential issue | 🟡 MinorУдалите лишний вызов
fmt.Fprintln.Серверная сторона использует
json.NewEncoder(w).Encode(...), который добавляет символ новой строки после JSON.io.Copyкопирует полное тело ответа включая эту новую строку. Дополнительныйfmt.Fprintlnсоздаёт пустую строку между JSON-документами, что может нарушить парсинг JSON Lines у клиентов.,
🔧 Предлагаемое исправление
_, _ = io.Copy(os.Stdout, resp.Body) _ = resp.Body.Close() - - // Ensure newline separator between responses - fmt.Fprintln(os.Stdout) }Если сервер не гарантирует trailing newline, лучше проверять последний байт ответа и добавлять newline условно.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/engram/main.go` around lines 143 - 144, Remove the extraneous fmt.Fprintln call that writes an extra newline after copying the response; instead either delete the fmt.Fprintln(os.Stdout) invocation in main (or the function containing it) or replace it with conditional logic that inspects the last byte of the copied response and only writes a '\n' if the response does not already end with one (so that json.NewEncoder(w).Encode(...) which already appends '\n' is not duplicated).
52-54:⚠️ Potential issue | 🟠 MajorСанируйте git remote перед логированием.
Переменная
remoteможет содержать встроенные учётные данные в форматеhttps://token@host/.... Прямое логирование в stderr приводит к утечке секретов в логи и CI.,
🛡️ Предлагаемое исправление
+import "net/url" + +// sanitizeRemote removes userinfo (credentials) from a URL for safe logging. +func sanitizeRemote(remote string) string { + u, err := url.Parse(remote) + if err != nil { + return "<invalid-url>" + } + u.User = nil + return u.String() +}Затем замените:
if remote != "" { - fmt.Fprintf(os.Stderr, "[engram] git remote: %s\n", remote) + fmt.Fprintf(os.Stderr, "[engram] git remote: %s\n", sanitizeRemote(remote)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/engram/main.go` around lines 52 - 54, The code logs the raw git remote in the variable remote which may include embedded credentials (e.g. https://token@host/...), so parse remote using net/url (url.Parse), clear the User info (e.g. set u.User = nil) and then log the sanitized URL (u.String()); update the fmt.Fprintf call that prints remote to use the sanitized string and fall back to a redacted placeholder if parsing fails or the URL contains unexpected sensitive parts.
94-101:⚠️ Potential issue | 🟠 MajorОграничьте размер тела ответа в probe-запросе.
io.ReadAll(resp.Body)читает весь ответ без лимита. Недоверенный сервер может исчерпать память прокси, отправив произвольно большой ответ. Для probe, которому нужна только валидация JSON, достаточно небольшого лимита.,
🛡️ Предлагаемое исправление
+const maxProbeBodySize = 64 * 1024 // 64 KB достаточно для JSON-RPC ответа + func tryStreamableHTTP(serverURL, authToken, projectSlug string) bool { ... - body, err := io.ReadAll(resp.Body) + body, err := io.ReadAll(io.LimitReader(resp.Body, maxProbeBodySize)) if err != nil { return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/engram/main.go` around lines 94 - 101, The probe currently uses io.ReadAll(resp.Body) which can load an unbounded response into memory; replace this with a size-limited read using io.LimitReader(resp.Body, <maxBytes>) (or http.MaxBytesReader) when reading into body in the probe function, then pass the limited body to json.Unmarshal (the check json.RawMessage and json.Unmarshal call) and handle truncated responses (e.g., treat reads that hit the limit as invalid and return false or an appropriate error) so an untrusted server cannot exhaust memory.
🧹 Nitpick comments (1)
cmd/engram/main.go (1)
240-246: Рассмотрите заменуos.Exitна channel-based сигнализацию.Понимаю обоснование в комментариях — exit code 0 чтобы не маскировать здоровую SSE сессию. Однако
os.Exit()из горутины обходит все deferred функции и делает невозможным graceful shutdown (например, финализация буферизованных SSE сообщений перед выходом).Альтернатива: использовать канал или
context.CancelFuncдля сигнализации завершенияforwardStdin, позволяя основному reader'у контролировать жизненный цикл процесса.Also applies to: 257-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/engram/main.go` around lines 240 - 246, The goroutine in main.go currently calls os.Exit(0) on forward error which bypasses deferred cleanup; change this to signal the main SSE reader instead (e.g., add a shared done channel or pass a context with CancelFunc into forwardStdin) so forwardStdin sends a non-fatal completion/error signal instead of exiting; update the caller and the SSE reader goroutine to listen for that channel/context cancellation and perform graceful shutdown/flush of buffered SSE messages; apply the same change to the other os.Exit usage around the forwardStdin/error handling (the second block at 257-263) so all early exits use the same channel/context-based signaling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/engram/main.go`:
- Around line 212-216: The SSE-supplied messageEndpoint is used directly for
POSTs (forwardStdin) which creates an SSRF risk; update
resolveMessageEndpoint/runSSE flow so the resolved URL is parsed and validated
before use: parse messageEndpoint and serverURL, ensure the origins (scheme +
host + port) match the serverURL origin, reject or ignore endpoints that fail
validation (log an error) and only call forwardStdin(messageEndpoint, authToken,
projectSlug) when validation succeeds; reference resolveMessageEndpoint, runSSE,
messageEndpoint, and forwardStdin when making this change.
---
Duplicate comments:
In `@cmd/engram/main.go`:
- Around line 143-144: Remove the extraneous fmt.Fprintln call that writes an
extra newline after copying the response; instead either delete the
fmt.Fprintln(os.Stdout) invocation in main (or the function containing it) or
replace it with conditional logic that inspects the last byte of the copied
response and only writes a '\n' if the response does not already end with one
(so that json.NewEncoder(w).Encode(...) which already appends '\n' is not
duplicated).
- Around line 52-54: The code logs the raw git remote in the variable remote
which may include embedded credentials (e.g. https://token@host/...), so parse
remote using net/url (url.Parse), clear the User info (e.g. set u.User = nil)
and then log the sanitized URL (u.String()); update the fmt.Fprintf call that
prints remote to use the sanitized string and fall back to a redacted
placeholder if parsing fails or the URL contains unexpected sensitive parts.
- Around line 94-101: The probe currently uses io.ReadAll(resp.Body) which can
load an unbounded response into memory; replace this with a size-limited read
using io.LimitReader(resp.Body, <maxBytes>) (or http.MaxBytesReader) when
reading into body in the probe function, then pass the limited body to
json.Unmarshal (the check json.RawMessage and json.Unmarshal call) and handle
truncated responses (e.g., treat reads that hit the limit as invalid and return
false or an appropriate error) so an untrusted server cannot exhaust memory.
---
Nitpick comments:
In `@cmd/engram/main.go`:
- Around line 240-246: The goroutine in main.go currently calls os.Exit(0) on
forward error which bypasses deferred cleanup; change this to signal the main
SSE reader instead (e.g., add a shared done channel or pass a context with
CancelFunc into forwardStdin) so forwardStdin sends a non-fatal completion/error
signal instead of exiting; update the caller and the SSE reader goroutine to
listen for that channel/context cancellation and perform graceful shutdown/flush
of buffered SSE messages; apply the same change to the other os.Exit usage
around the forwardStdin/error handling (the second block at 257-263) so all
early exits use the same channel/context-based signaling.
🪄 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: 017e2b41-221c-4714-a6d9-bd50598a20b7
📒 Files selected for processing (3)
cmd/engram/main.gointernal/mcp/tools_issues.gointernal/proxy/identity.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/mcp/tools_issues.go
- internal/proxy/identity.go
The SSE server delivers a message endpoint URL which the proxy uses as the target for stdin POST forwarding. A malicious server could supply an arbitrary URL and redirect proxy traffic to internal or third-party services. Add validateEndpoint(baseURL, endpoint) that checks the resolved endpoint's host against the configured server host and rejects any mismatch. Relative paths (empty host) are allowed as they inherit the base origin. Comment id: PRRC_kwDORY-ek862_Hbq
…arseSync - Dockerfile: update build path after cmd/mcp-stdio-proxy was renamed to cmd/engram in PR #140 - useMarkdown.ts: marked.parseSync → marked.parse (parseSync removed in marked v13+)
* fix(ui): replace removed parseSync with parse in marked v18 marked v13+ removed parseSync(). The parse() method is synchronous by default (unless async: true is passed). Fixes CI build failure. * fix: Dockerfile reference cmd/mcp-stdio-proxy → cmd/engram + marked parseSync - Dockerfile: update build path after cmd/mcp-stdio-proxy was renamed to cmd/engram in PR #140 - useMarkdown.ts: marked.parseSync → marked.parse (parseSync removed in marked v13+) --------- Co-authored-by: Kirill Turanskiy <thebtf@users.noreply.github.com>
1 participant
Summary
cmd/engram/binary: stdio MCP proxy with git-derived project identityArchitecture
Changes
internal/proxy/identity.go— ResolveProjectSlug from git remote (SHA-256, 8 hex chars)cmd/engram/main.go— stdio-to-HTTP proxy with git identity + Streamable HTTP + SSE fallbackinternal/mcp/context.go— X-Engram-Project header extraction + context pipelineinternal/mcp/streamable.go+sse.go— wire header extraction before tool dispatchinternal/mcp/tools_issues.go+tools_memory.go+tools_store_consolidated.go— use header-derived project for write opsinternal/config/config.go— ENGRAM_ENFORCE_SOURCE_PROJECT feature flag (default true)plugin/engram/.mcp.json— switch from HTTP to command-based transportVerified
go build ./...cleango test ./... -count=1all pass (8 new tests)Test plan
Closes #40
Summary by CodeRabbit
New Features
Chores