review: post-v4.0.0 hotfixes (cmux, constraints, logging, plugin auto-binary)#150
Closed
thebtf wants to merge 4 commits into
Closed
review: post-v4.0.0 hotfixes (cmux, constraints, logging, plugin auto-binary)#150thebtf wants to merge 4 commits into
thebtf wants to merge 4 commits into
Conversation
…ation to detection_source File-relation detector creates modifies/reads relations but constraints only allowed causes/fixes/supersedes/depends_on/relates_to/evolves_from. Consolidation uses creative_association but migration 076 dropped it. Migration 077: updates both CHECK constraints. detector.go: sets DetectionSource=file_overlap on modifies/reads relations.
- ensure-binary.sh: SessionStart hook downloads correct platform binary from GitHub Releases to CLAUDE_PLUGIN_DATA/bin/, version-pinned to plugin.json version. Atomic replace, cached until version changes. - userConfig: prompts for server_url and api_token on plugin install (no manual env var setup needed for MCP server) - .mcp.json: uses CLAUDE_PLUGIN_DATA binary path + userConfig env vars - Plugin version 4.0.1 Co-authored-by: Kirill Turanskiy <thebtf@users.noreply.github.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
ПрохождениеДанные изменения добавляют новую миграцию БД для обновления ограничений PostgreSQL, внедряют источник обнаружения отношений в детектор, заменяют HTTP middleware логирования на отладочную версию с метриками, переопределяют CMux маршрутизацию, обновляют манифест плагина с конфигурацией пользователя и добавляют скрипты Node.js для управления двоичным файлом engram. Изменения
Оценка сложности проверки кода🎯 3 (Умеренная) | ⏱️ ~25 минут Возможно связанные PR
Предложенные метки
Стихотворение
🚥 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 |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@gemini-code-assist review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (6)
plugin/engram/scripts/ensure-binary.js (3)
13-13: Неиспользуемый импортexecSync.
execSyncимпортируется, но нигде не используется в коде.🧹 Предлагаемое исправление
const https = require("https"); const http = require("http"); -const { execSync } = require("child_process");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/engram/scripts/ensure-binary.js` at line 13, Remove the unused import by deleting the destructured require of execSync (the line containing "const { execSync } = require('child_process');") from ensure-binary.js; if execSync is actually needed elsewhere, instead reference the symbol where used (e.g., call execSync) or replace the destructure with a single require usage, otherwise simply remove the import to eliminate the unused variable.
94-101: Рассмотрите использованиеrenameбез предварительногоunlinkна POSIX.Текущая последовательность
unlink→renameсоздаёт окно, когда бинарник отсутствует. На POSIX-системахrenameатомарно перезаписывает существующий файл. Однако на Windows требуется предварительныйunlink.♻️ Кроссплатформенное улучшение
// Atomic replace try { - if (fs.existsSync(binaryPath)) fs.unlinkSync(binaryPath); - fs.renameSync(tmpPath, binaryPath); + if (process.platform === "win32") { + // Windows: unlink first (rename fails if target exists) + try { fs.unlinkSync(binaryPath); } catch {} + } + fs.renameSync(tmpPath, binaryPath); } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/engram/scripts/ensure-binary.js` around lines 94 - 101, Replace the unconditional fs.unlinkSync before fs.renameSync with a platform-aware flow: if running on Windows (process.platform === 'win32') then check and unlink binaryPath before renaming, otherwise call fs.renameSync(tmpPath, binaryPath) directly to leverage POSIX atomic overwrite; keep the try/catch around the operations and preserve the existing stderr message using err.message. Use the identifiers binaryPath and tmpPath to locate and update the logic.
46-48: Windows ARM64 явно не поддерживается.Код жёстко задаёт
windows-amd64.exeнезависимо от архитектуры (строка 47), в то время как Darwin и Linux используют проверкуarch === "arm64"для выбора правильного бинарника. Устройства Windows на ARM (Surface Pro X и др.) получат x64 бинарник, который будет работать через эмуляцию с потерей производительности.На сегодня релизы включают только
engram-windows-amd64.exe, поэтому добавление проверки архитектуры возможно только при условии, что будет доступна сборкаwindows-arm64.exe.♻️ Предлагаемое исправление при наличии arm64 сборки
if (platform === "win32") { - suffix = "windows-amd64.exe"; + suffix = arch === "arm64" ? "windows-arm64.exe" : "windows-amd64.exe"; binaryName = "engram.exe";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/engram/scripts/ensure-binary.js` around lines 46 - 48, Код жёстко устанавливает suffix = "windows-amd64.exe" и binaryName = "engram.exe" при platform === "win32", игнорируя переменную arch; исправьте блок, чтобы при platform === "win32" также проверялось arch === "arm64" и в этом случае устанавливался suffix = "windows-arm64.exe" (иначе оставлять "windows-amd64.exe"), при этом сохраняется установка binaryName = "engram.exe"; добавьте короткий комментарий рядом с этим условием, что ветка arm64 должна использоваться только если доступна соответствующая сборка.plugin/engram/.mcp.json (1)
10-10: Рассмотрите возможность вынестиENGRAM_INSECUREвuserConfig.Значение
"1"жёстко задано, что отключает проверку TLS для всех пользователей. Для локальной разработки это приемлемо, но для production-развёртываний пользователи могут захотеть включить проверку сертификатов.♻️ Предлагаемое изменение
В
plugin.jsonдобавить:"insecure_mode": { "description": "Disable TLS certificate verification (for self-signed certs)", "required": false, "sensitive": false, "default": "1" }В
.mcp.json:- "ENGRAM_INSECURE": "1" + "ENGRAM_INSECURE": "${user_config.insecure_mode}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/engram/.mcp.json` at line 10, The ENGRAM_INSECURE flag is hardcoded in .mcp.json; move it into the plugin/user-config schema so users can opt-in per-deployment: add a new userConfig field (e.g., "insecure_mode" or "insecure" in plugin.json) with description, required:false and default:"1", then update the code that reads ENGRAM_INSECURE (reference the ENGRAM_INSECURE environment usage) to instead read from the userConfig/insecure_mode value and set the environment or runtime TLS-check behavior accordingly; ensure the .mcp.json no longer hardcodes "ENGRAM_INSECURE":"1" and that defaults are preserved when userConfig is absent.plugin/engram/scripts/run-engram.js (1)
5-5: Неиспользуемый импортexecFileSync.
execFileSyncимпортируется, но не используется — код используетspawnSyncиз строки 27.🧹 Предлагаемое исправление
-const { execFileSync } = require("child_process"); +const { spawnSync } = require("child_process"); const path = require("path"); const fs = require("fs");И убрать повторный импорт в строке 27:
try { - const { spawnSync } = require("child_process"); const result = spawnSync(binaryPath, process.argv.slice(2), {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/engram/scripts/run-engram.js` at line 5, Удалите неиспользуемый импорт execFileSync: уберите execFileSync из require("child_process") (строка где объявлено const { execFileSync } = require("child_process")) и оставьте единственный импорт spawnSync (т.е. используйте только spawnSync из require("child_process") в коде, удалив дублирующий/лишний require на строке с spawnSync) чтобы не держать неиспользуемые символы execFileSync в run-engram.js.internal/worker/middleware.go (1)
457-464: Добавьтеrequest_idв запись лога запроса.Сейчас
RequestIDуже лежит в контексте, но этот middleware его не пишет, поэтому DEBUG-лог запроса сложно связать с логами хендлеров и ошибок.♻️ Вариант правки
log.Debug(). + Str("request_id", GetRequestID(r.Context())). Str("method", r.Method). Str("path", r.URL.Path). Int("status", ww.Status()). Int("bytes", ww.BytesWritten()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/middleware.go` around lines 457 - 464, В лог-запись в middleware.go (там где вызывается log.Debug().Str("method", r.Method)...Msg("HTTP request")) нужно извлечь request ID из контекста запроса (например requestID := r.Context().Value(RequestID) или через существующий helper если есть) и добавить его в запись, например .Str("request_id", requestID.(string)), чтобы связать DEBUG-лог запроса с логами хендлеров и ошибок; убедитесь, что вы безопасно проверяете тип/наличие requestID перед приведением и не паниковать при его отсутствии.
🤖 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/db/gorm/migrations.go`:
- Around line 2271-2273: The loop currently discards SQL execution errors (for
_, s := range sqls { _ = tx.Exec(s).Error }), which can mark the migration
successful despite failures; change the loop in migrations.go that iterates sqls
to capture the result (err := tx.Exec(s).Error) and return that error
immediately (or wrap it with context) so the migration fails on the first Exec
error (mirror the error handling used in the earlier migration that returns on
tx.Exec errors).
- Around line 2265-2266: The new CHECK constraint
chk_observation_relations_relation_type on observation_relations dropped many
valid relation types; update the ALTER TABLE statement that adds
chk_observation_relations_relation_type so its CHECK includes the full superset
of relation strings originally defined in migration 019 plus the ones currently
used in code (e.g. types referenced from detector.go like follows, prompted_by,
references, referenced_by and classifyRelation entries, and types used in
consolidation/associations.go such as shares_theme and parallel_context),
ensuring the final list contains all 17 original entries plus any additional
ones so inserts from functions like classifyRelation and code paths in
detector.go and consolidation/associations.go no longer violate the constraint.
- Around line 2260-2276: The migration entry with ID
"077_relations_constraints_update" defines only a Migrate func and lacks a
Rollback; add a Rollback function that reverses the SQL changes applied in
Migrate: for each constraint added in Migrate
(chk_observation_relations_relation_type and
chk_observation_relations_detection_source) execute statements to DROP those
constraints IF EXISTS and re-ADD the previous CHECK definitions (i.e., remove
'modifies' and 'reads' from relation_type and remove 'creative_association' from
detection_source) using the same tx.Exec(...).Error pattern used in other
migrations so the migration can be cleanly reverted on failure.
In `@internal/worker/service.go`:
- Around line 2195-2209: В текущем коде однократная проверка s.grpcServer == nil
при настройке cmux может навсегда пропустить регистрацию gRPC matcher —
исправьте это, убрав зависимость от раннего one-shot nil: либо
инициализируйте/присвойте s.grpcServer до настройки cmux (перенесите/вызовите
создание gRPC сервера из initializeAsync в Start или перед m.Match), либо
отложите запуск/настройку cmux до тех пор, пока initializeAsync не установит
s.grpcServer (например, дождитесь готовности через сигнал/канал или метод
IsReady), и затем вызовите
m.Match(cmux.HTTP2HeaderFieldPrefix("content-type","application/grpc")) и запуск
s.grpcServer.Serve(grpcL); не полагайтесь на однократную nil-проверку в месте с
m.Match.
In `@plugin/engram/scripts/ensure-binary.js`:
- Around line 138-141: The file error handler currently calls
fs.unlinkSync(destPath) which can itself throw and hide the original error; wrap
the unlink in a try/catch (or use fs.unlink with a callback) inside the
file.on("error", ...) handler so any unlink failure is swallowed or logged but
does not override the original error, and always call reject(err) with the
original error; locate the handler that uses file.on("error", (err) => {
fs.unlinkSync(destPath); reject(err); }) and modify it to protect the unlink
operation and ensure reject(err) is invoked unconditionally.
---
Nitpick comments:
In `@internal/worker/middleware.go`:
- Around line 457-464: В лог-запись в middleware.go (там где вызывается
log.Debug().Str("method", r.Method)...Msg("HTTP request")) нужно извлечь request
ID из контекста запроса (например requestID := r.Context().Value(RequestID) или
через существующий helper если есть) и добавить его в запись, например
.Str("request_id", requestID.(string)), чтобы связать DEBUG-лог запроса с логами
хендлеров и ошибок; убедитесь, что вы безопасно проверяете тип/наличие requestID
перед приведением и не паниковать при его отсутствии.
In `@plugin/engram/.mcp.json`:
- Line 10: The ENGRAM_INSECURE flag is hardcoded in .mcp.json; move it into the
plugin/user-config schema so users can opt-in per-deployment: add a new
userConfig field (e.g., "insecure_mode" or "insecure" in plugin.json) with
description, required:false and default:"1", then update the code that reads
ENGRAM_INSECURE (reference the ENGRAM_INSECURE environment usage) to instead
read from the userConfig/insecure_mode value and set the environment or runtime
TLS-check behavior accordingly; ensure the .mcp.json no longer hardcodes
"ENGRAM_INSECURE":"1" and that defaults are preserved when userConfig is absent.
In `@plugin/engram/scripts/ensure-binary.js`:
- Line 13: Remove the unused import by deleting the destructured require of
execSync (the line containing "const { execSync } = require('child_process');")
from ensure-binary.js; if execSync is actually needed elsewhere, instead
reference the symbol where used (e.g., call execSync) or replace the destructure
with a single require usage, otherwise simply remove the import to eliminate the
unused variable.
- Around line 94-101: Replace the unconditional fs.unlinkSync before
fs.renameSync with a platform-aware flow: if running on Windows
(process.platform === 'win32') then check and unlink binaryPath before renaming,
otherwise call fs.renameSync(tmpPath, binaryPath) directly to leverage POSIX
atomic overwrite; keep the try/catch around the operations and preserve the
existing stderr message using err.message. Use the identifiers binaryPath and
tmpPath to locate and update the logic.
- Around line 46-48: Код жёстко устанавливает suffix = "windows-amd64.exe" и
binaryName = "engram.exe" при platform === "win32", игнорируя переменную arch;
исправьте блок, чтобы при platform === "win32" также проверялось arch ===
"arm64" и в этом случае устанавливался suffix = "windows-arm64.exe" (иначе
оставлять "windows-amd64.exe"), при этом сохраняется установка binaryName =
"engram.exe"; добавьте короткий комментарий рядом с этим условием, что ветка
arm64 должна использоваться только если доступна соответствующая сборка.
In `@plugin/engram/scripts/run-engram.js`:
- Line 5: Удалите неиспользуемый импорт execFileSync: уберите execFileSync из
require("child_process") (строка где объявлено const { execFileSync } =
require("child_process")) и оставьте единственный импорт spawnSync (т.е.
используйте только spawnSync из require("child_process") в коде, удалив
дублирующий/лишний require на строке с spawnSync) чтобы не держать
неиспользуемые символы execFileSync в run-engram.js.
🪄 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: 15484914-2ae7-4ae4-aa8f-96f46537d5c4
📒 Files selected for processing (9)
internal/db/gorm/migrations.gointernal/relation/detector.gointernal/worker/middleware.gointernal/worker/service.goplugin/engram/.claude-plugin/plugin.jsonplugin/engram/.mcp.jsonplugin/engram/hooks/hooks.jsonplugin/engram/scripts/ensure-binary.jsplugin/engram/scripts/run-engram.js
Comment on lines
+2195
to
+2209
| // Only add gRPC matcher if the gRPC server is available. | ||
| // Closing a cmux sub-listener cascades to the parent — so we must NOT | ||
| // create a matcher and then close it, as that kills the entire mux. | ||
| if s.grpcServer != nil { | ||
| grpcL := m.Match(cmux.HTTP2HeaderFieldPrefix("content-type", "application/grpc")) | ||
| go func() { | ||
| if err := s.grpcServer.Serve(grpcL); err != nil { | ||
| log.Error().Err(err).Msg("gRPC server error") | ||
| } | ||
| }() | ||
| } else { | ||
| // No gRPC server yet — drain the grpcL so cmux does not stall. | ||
| go func() { _ = grpcL.Close() }() | ||
| log.Info().Msg("gRPC server registered on cmux") | ||
| } | ||
|
|
||
| // All other connections (HTTP/1.1, HTTP/2 non-gRPC) go to the HTTP server. | ||
| httpL := m.Match(cmux.Any()) |
There was a problem hiding this comment.
Этот one-shot nil check может навсегда отключить gRPC после обычного старта.
Start() поднимает listener сразу, а initializeAsync() присваивает s.grpcServer только позже. Поэтому при типичном запуске эта ветка выполнится с nil, matcher для gRPC не зарегистрируется вообще, и весь HTTP/2/gRPC трафик уйдёт в httpL до перезапуска процесса. Нужно либо создавать/подключать gRPC server до настройки cmux, либо откладывать запуск cmux до момента, когда готовность gRPC уже известна.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/worker/service.go` around lines 2195 - 2209, В текущем коде
однократная проверка s.grpcServer == nil при настройке cmux может навсегда
пропустить регистрацию gRPC matcher — исправьте это, убрав зависимость от
раннего one-shot nil: либо инициализируйте/присвойте s.grpcServer до настройки
cmux (перенесите/вызовите создание gRPC сервера из initializeAsync в Start или
перед m.Match), либо отложите запуск/настройку cmux до тех пор, пока
initializeAsync не установит s.grpcServer (например, дождитесь готовности через
сигнал/канал или метод IsReady), и затем вызовите
m.Match(cmux.HTTP2HeaderFieldPrefix("content-type","application/grpc")) и запуск
s.grpcServer.Serve(grpcL); не полагайтесь на однократную nil-проверку в месте с
m.Match.
Comment on lines
+138
to
+141
| file.on("error", (err) => { | ||
| fs.unlinkSync(destPath); | ||
| reject(err); | ||
| }); |
There was a problem hiding this comment.
Обработчик ошибки файла может выбросить исключение.
fs.unlinkSync в обработчике ошибки может сам выбросить исключение (например, если файл уже удалён), что замаскирует оригинальную ошибку.
🐛 Предлагаемое исправление
file.on("error", (err) => {
- fs.unlinkSync(destPath);
+ try { fs.unlinkSync(destPath); } catch {}
reject(err);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/engram/scripts/ensure-binary.js` around lines 138 - 141, The file
error handler currently calls fs.unlinkSync(destPath) which can itself throw and
hide the original error; wrap the unlink in a try/catch (or use fs.unlink with a
callback) inside the file.on("error", ...) handler so any unlink failure is
swallowed or logged but does not override the original error, and always call
reject(err) with the original error; locate the handler that uses
file.on("error", (err) => { fs.unlinkSync(destPath); reject(err); }) and modify
it to protect the unlink operation and ensure reject(err) is invoked
unconditionally.
Owner
Author
|
Post-merge review complete. All findings (2 CRIT, 2 MAJOR, 1 MINOR) fixed on main. Cleanup branches. |
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.
Post-merge review of 4 commits after v4.0.0. Code already on main — review only, fix findings with separate commits.
Summary by CodeRabbit
Примечания к выпуску
Новые функции
Изменения