fix(plugin): refresh marketplace and add OMP support - #443
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
WalkthroughРелиз 6.46.2 синхронизирует версии плагина и marketplace, добавляет OMP marketplace manifest, расширяет запуск и диагностику Engram, обновляет команды и инструкции, а также добавляет проверки версий и OMP layout. ChangesРелиз и интеграция плагина
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Host as Claude Code или Oh My Pi
participant Plugin as Engram plugin
participant Config as env или config.json
participant Binary as Engram binary
Host->>Plugin: запуск MCP и команд
Plugin->>Config: чтение server_url и api_token
Config-->>Plugin: конфигурационные значения
Plugin->>Binary: запуск с аргументами и process.env
Binary-->>Host: MCP-соединение или ошибка запуска
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43d7c76bb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with: | ||
| node-version: "20" | ||
| - name: Test plugin scripts | ||
| run: node --test --test-concurrency=1 plugin/engram/hooks/*.test.js plugin/engram/scripts/*.test.js |
There was a problem hiding this comment.
In the credential-free GitHub runner, this exact command fails six session-start.test.js cases: those tests set only ENGRAM_URL, while handleSessionStart requires both a URL and token and therefore returns the setup banner instead of exercising their assertions. I reproduced 115/121 passing with no credentials and 121/121 after setting a dummy ENGRAM_TOKEN; consequently this job fails before build, and the identical step in release.yaml also prevents GoReleaser from running. Update the affected fixtures to set and restore a test token, or supply a non-secret dummy token to the test steps.
AGENTS.md reference: AGENTS.md:L45-L47
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugin/engram/commands/retro.md (3)
40-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winНе выдавайте системную статистику за статистику текущей сессии.
Endpoint назван
effectiveness-distributionи в самой инструкции описан как system-wide, но вывод озаглавленSession Effectiveness. Используйте session-scoped данные либо переименуйте раздел вSystem-wide Effectiveness, иначе отчёт будет вводить пользователя в заблуждение.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/engram/commands/retro.md` around lines 40 - 56, Rename the report section generated from the system-wide effectiveness-distribution endpoint from “Session Effectiveness” to “System-wide Effectiveness”; do not present these system-wide values as current-session statistics.
21-24: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftУберите
ENGRAM_AUTH_ADMIN_TOKENиз plugin-команд.
Сейчасrestart.mdиretro.mdвызывают admin API из plugin/workstation контекста, хотя этот токен должен оставаться только на серверном хосте. Если это операторские действия — вынесите их из plugin и пометьте как operator-only; иначе переведите наENGRAM_TOKEN/~/.engram/config.jsonи API с минимальными правами.
plugin/engram/commands/restart.md#L15-L20plugin/engram/commands/retro.md#L21-L24🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/engram/commands/retro.md` around lines 21 - 24, Remove ENGRAM_AUTH_ADMIN_TOKEN usage from plugin commands in plugin/engram/commands/retro.md lines 21-24 and plugin/engram/commands/restart.md lines 15-20. Either move these operator-only admin actions out of the plugin and mark them operator-only, or switch both commands to ENGRAM_TOKEN or ~/.engram/config.json with the least-privileged API; apply the same policy consistently at both sites.
11-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winЯвно задайте
SESSION_IDи выровняйте отчет с поддерживаемым auth.
- После
check_system_health()здесь не указан источник DB ID, поэтому${SESSION_ID}остается неинициализированным; добавьте явное извлечение или сначала берите нужную сессию через/api/sessions/listпо четкому правилу выбора.- Блок
Session Effectivenessсейчас выводит системную метрику, а не данные одной сессии — переименуйте его или стройте из сессионного среза.- Этот шаг должен использовать
ENGRAM_TOKEN/ENGRAM_URL(или~/.engram/config.jsonв Codex/OMP), а неENGRAM_AUTH_ADMIN_TOKEN.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/engram/commands/retro.md` around lines 11 - 26, Update the session injection step after check_system_health() to explicitly obtain and assign SESSION_ID, using a deterministic most-recent-session selection from /api/sessions/list when necessary. Change the Session Effectiveness section to either use metrics from that session’s data or rename it to reflect that it reports a system-wide metric. Replace ENGRAM_AUTH_ADMIN_TOKEN usage with the supported ENGRAM_TOKEN/ENGRAM_URL configuration, falling back to ~/.engram/config.json where applicable.
🧹 Nitpick comments (2)
.github/workflows/sync-marketplace.yml (1)
51-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winСовет: сгенерируйте манифест из канонического файла, а не из литерала в workflow.
Метаданные (
description,owner,category) продублированы здесь и в.claude-plugin/marketplace.json; при правке одного места второе тихо разойдётся. Альтернатива — взять репозиторный манифест и переписать толькоversionиsource:♻️ Вариант
- jq -n --arg version "$VERSION" ' - { - "$schema": "https://anthropic.com/claude-code/marketplace.schema.json", - name: "engram", - ... - } - ' > marketplace/.claude-plugin/marketplace.json + jq --arg version "$VERSION" ' + .version = $version + | .plugins |= map(.version = $version | .source = "./engram") + ' .claude-plugin/marketplace.json > marketplace/.claude-plugin/marketplace.json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/sync-marketplace.yml around lines 51 - 70, Update the sync workflow to generate the marketplace manifest from the canonical repository manifest instead of duplicating metadata in the inline jq literal. Preserve the canonical fields, then override only the release-specific version and source values before writing both marketplace output files.plugin/engram/scripts/run-engram.js (1)
124-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winСогласуйте резолвинг
pluginRootсconfiguredEnvValue.
resolvePluginDataфильтрует placeholder-значения вида${...}черезconfiguredEnvValue, аresolvePluginRootпринимает любую непустую строку. Если хост подставит неразвёрнутый${...}вPLUGIN_ROOT/CLAUDE_PLUGIN_ROOT, все производные пути (ensure-binary,.data) окажутся мусорными.♻️ Предлагаемое выравнивание
function resolvePluginRoot() { - return ( - process.env.PLUGIN_ROOT || - process.env.CLAUDE_PLUGIN_ROOT || - path.resolve(__dirname, "..") - ); + return ( + configuredEnvValue("PLUGIN_ROOT", "CLAUDE_PLUGIN_ROOT") || + path.resolve(__dirname, "..") + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/engram/scripts/run-engram.js` around lines 124 - 130, Update resolvePluginRoot to validate PLUGIN_ROOT and CLAUDE_PLUGIN_ROOT with the existing configuredEnvValue logic, ignoring unresolved ${...} placeholders and falling back to path.resolve(__dirname, ".."). Preserve the current precedence between the two environment variables and the existing fallback path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release-binary.yml:
- Line 15: Update the actions/checkout@v4 step in the release workflow to set
persist-credentials to false, ensuring credentials are not retained before the
subsequent Node.js tests run.
In @.github/workflows/sync-marketplace.yml:
- Around line 39-50: Harden the “Sync plugin files” shell block by enabling
fail-fast shell options with `set -euo pipefail` and validating VERSION
immediately after extracting it from plugin.json. Reject empty or "null"
versions before generating or committing the marketplace manifest, while
preserving the existing sync commands.
In `@README.md`:
- Around line 255-256: Update the installation instructions in the README to
install the marketplace plugin before running /engram:setup, then instruct users
to restart the host after setup. Replace the current setup-first ordering while
preserving the universal ~/.engram/config.json configuration guidance.
---
Outside diff comments:
In `@plugin/engram/commands/retro.md`:
- Around line 40-56: Rename the report section generated from the system-wide
effectiveness-distribution endpoint from “Session Effectiveness” to “System-wide
Effectiveness”; do not present these system-wide values as current-session
statistics.
- Around line 21-24: Remove ENGRAM_AUTH_ADMIN_TOKEN usage from plugin commands
in plugin/engram/commands/retro.md lines 21-24 and
plugin/engram/commands/restart.md lines 15-20. Either move these operator-only
admin actions out of the plugin and mark them operator-only, or switch both
commands to ENGRAM_TOKEN or ~/.engram/config.json with the least-privileged API;
apply the same policy consistently at both sites.
- Around line 11-26: Update the session injection step after
check_system_health() to explicitly obtain and assign SESSION_ID, using a
deterministic most-recent-session selection from /api/sessions/list when
necessary. Change the Session Effectiveness section to either use metrics from
that session’s data or rename it to reflect that it reports a system-wide
metric. Replace ENGRAM_AUTH_ADMIN_TOKEN usage with the supported
ENGRAM_TOKEN/ENGRAM_URL configuration, falling back to ~/.engram/config.json
where applicable.
---
Nitpick comments:
In @.github/workflows/sync-marketplace.yml:
- Around line 51-70: Update the sync workflow to generate the marketplace
manifest from the canonical repository manifest instead of duplicating metadata
in the inline jq literal. Preserve the canonical fields, then override only the
release-specific version and source values before writing both marketplace
output files.
In `@plugin/engram/scripts/run-engram.js`:
- Around line 124-130: Update resolvePluginRoot to validate PLUGIN_ROOT and
CLAUDE_PLUGIN_ROOT with the existing configuredEnvValue logic, ignoring
unresolved ${...} placeholders and falling back to path.resolve(__dirname,
".."). Preserve the current precedence between the two environment variables and
the existing fallback path.
🪄 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: 16c80e31-4a51-48f0-95a0-bafedbc6e410
📒 Files selected for processing (19)
.claude-plugin/marketplace.json.claude-plugin/plugin.json.github/workflows/release-binary.yml.github/workflows/release.yaml.github/workflows/sync-marketplace.yml.omp-plugin/marketplace.jsonCHANGELOG.mdREADME.mdplugin/engram/.claude-plugin/plugin.jsonplugin/engram/.codex-plugin/plugin.jsonplugin/engram/commands/cleanup.mdplugin/engram/commands/doctor.mdplugin/engram/commands/export.mdplugin/engram/commands/restart.mdplugin/engram/commands/retro.mdplugin/engram/commands/setup.mdplugin/engram/commands/stats.mdplugin/engram/scripts/run-engram.jsplugin/engram/scripts/run-engram.test.js
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ded34c630
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,3 +1,7 @@ | |||
| --- | |||
| description: Review and curate low-quality Engram memories | |||
There was a problem hiding this comment.
When an OMP user invokes the newly advertised /engram:cleanup, its first step calls admin(action="quality"), followed by consolidations, but the current server's admin schema and dispatcher only implement stats (plus flag-gated purge_project), so this command deterministically fails before producing a report. Either implement cleanup using a live API or stop exposing this stale command metadata.
AGENTS.md reference: AGENTS.md:L30-L39
Useful? React with 👍 / 👎.
| @@ -1,3 +1,7 @@ | |||
| --- | |||
| description: Export Engram observations in human-readable or machine-readable formats | |||
There was a problem hiding this comment.
For OMP users selecting the newly exposed /engram:export, the workflow always calls admin(action="export", ...), but the live admin dispatcher has no export action and rejects it as unknown. The command therefore cannot export in any requested format; it should target a current retrieval/export surface or remain unadvertised until one exists.
AGENTS.md reference: AGENTS.md:L30-L39
Useful? React with 👍 / 👎.
| @@ -1,3 +1,7 @@ | |||
| --- | |||
| description: Review this session's Engram injections and capture improvements | |||
There was a problem hiding this comment.
Invoking the newly exposed /engram:retro directs OMP to GET /api/sessions/${SESSION_ID}/injections, but a repo-wide route search shows that the current session router exposes list/init/summarize/index/check endpoints and no injections endpoint; the legacy observation-injection table was also dropped in favor of injection_log. Consequently every retrospective reaches a 404 rather than injection data, so this command needs a live injection-log read path or should not be advertised.
AGENTS.md reference: AGENTS.md:L30-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
plugin/engram/scripts/run-engram.js (4)
116-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueКомментарий не соответствует поведению.
spawnSyncне заменяет текущий процесс (в отличие отexec), а порождает дочерний и ждёт его завершения.♻️ Уточнение комментария
- // Replace this process with the engram binary + // Run the engram binary as a child process and propagate its exit code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/engram/scripts/run-engram.js` around lines 116 - 121, Update the comment above the checkedSpawnSync invocation to accurately state that it starts the engram binary as a child process and waits for it to finish, rather than replacing the current process. Keep the existing status propagation through process.exit(status) unchanged.
124-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueНесогласованная валидация env между
resolvePluginRootиresolvePluginData. Здесь используется голый||, поэтому значение-плейсхолдер вида${...}(например, из непроинтерполированного.mcp.json) будет принято как корень плагина, тогда какresolvePluginDataтакие значения отфильтровывает черезconfiguredEnvValue.♻️ Унификация
function resolvePluginRoot() { - return ( - process.env.PLUGIN_ROOT || - process.env.CLAUDE_PLUGIN_ROOT || - path.resolve(__dirname, "..") - ); + return ( + configuredEnvValue("PLUGIN_ROOT", "CLAUDE_PLUGIN_ROOT") || + path.resolve(__dirname, "..") + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/engram/scripts/run-engram.js` around lines 124 - 130, Update resolvePluginRoot to validate PLUGIN_ROOT and CLAUDE_PLUGIN_ROOT through the same configuredEnvValue logic used by resolvePluginData, rejecting unresolved placeholder values such as ${...} before selecting a root. Preserve the existing fallback to path.resolve(__dirname, "..") when neither environment value is valid.
198-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueНиточка:
require("os")вызывается внутри функции при каждом обращении; логичнее вынести к остальным импортам в начало файла.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/engram/scripts/run-engram.js` around lines 198 - 208, Вынесите загрузку модуля os из функции, определяющей путь конфигурации, к остальным импортам в начале файла. В этой функции замените inline-вызов require("os") использованием вынесенной переменной, сохранив текущий fallback-путь через homedir().
255-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winДиагностика не покрывает часть ключей, участвующих в резолвинге.
main()читает такжеCLAUDE_PLUGIN_OPTION_SERVER_URL,CLAUDE_PLUGIN_OPTION_API_TOKEN(варианты в верхнем регистре) иENGRAM_CONFIG_FILE, но в стартовом отчёте они не отображаются — при разборе проблем конфигурации это как раз те значения, которые нужны.♻️ Дополнение списка ключей
["CLAUDE_PLUGIN_OPTION_server_url", false], + ["CLAUDE_PLUGIN_OPTION_SERVER_URL", false], ["CLAUDE_PLUGIN_OPTION_api_token", true], + ["CLAUDE_PLUGIN_OPTION_API_TOKEN", true], ["ENGRAM_CLAUDE_USERCONFIG_URL", false], ["ENGRAM_CLAUDE_USERCONFIG_TOKEN", true], + ["ENGRAM_CONFIG_FILE", false],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/engram/scripts/run-engram.js` around lines 255 - 267, Расширьте массив keys в run-engram.js, добавив CLAUDE_PLUGIN_OPTION_SERVER_URL, CLAUDE_PLUGIN_OPTION_API_TOKEN и ENGRAM_CONFIG_FILE с корректными признаками чувствительности. Убедитесь, что стартовый диагностический отчёт, формируемый main(), отображает эти ключи наряду с уже существующими вариантами.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugin/engram/scripts/run-engram.js`:
- Around line 116-121: Update the comment above the checkedSpawnSync invocation
to accurately state that it starts the engram binary as a child process and
waits for it to finish, rather than replacing the current process. Keep the
existing status propagation through process.exit(status) unchanged.
- Around line 124-130: Update resolvePluginRoot to validate PLUGIN_ROOT and
CLAUDE_PLUGIN_ROOT through the same configuredEnvValue logic used by
resolvePluginData, rejecting unresolved placeholder values such as ${...} before
selecting a root. Preserve the existing fallback to path.resolve(__dirname,
"..") when neither environment value is valid.
- Around line 198-208: Вынесите загрузку модуля os из функции, определяющей путь
конфигурации, к остальным импортам в начале файла. В этой функции замените
inline-вызов require("os") использованием вынесенной переменной, сохранив
текущий fallback-путь через homedir().
- Around line 255-267: Расширьте массив keys в run-engram.js, добавив
CLAUDE_PLUGIN_OPTION_SERVER_URL, CLAUDE_PLUGIN_OPTION_API_TOKEN и
ENGRAM_CONFIG_FILE с корректными признаками чувствительности. Убедитесь, что
стартовый диагностический отчёт, формируемый main(), отображает эти ключи наряду
с уже существующими вариантами.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d701041-d8d7-4ad2-9ef9-8fc514399d71
📒 Files selected for processing (16)
.claude-plugin/marketplace.json.claude-plugin/plugin.json.omp-plugin/marketplace.jsonCHANGELOG.mdREADME.mdplugin/engram/.claude-plugin/plugin.jsonplugin/engram/.codex-plugin/plugin.jsonplugin/engram/commands/cleanup.mdplugin/engram/commands/doctor.mdplugin/engram/commands/export.mdplugin/engram/commands/restart.mdplugin/engram/commands/retro.mdplugin/engram/commands/setup.mdplugin/engram/commands/stats.mdplugin/engram/scripts/run-engram.jsplugin/engram/scripts/run-engram.test.js
🚧 Files skipped from review as they are similar to previous changes (13)
- .claude-plugin/plugin.json
- plugin/engram/.codex-plugin/plugin.json
- plugin/engram/commands/export.md
- plugin/engram/commands/cleanup.md
- plugin/engram/.claude-plugin/plugin.json
- .claude-plugin/marketplace.json
- plugin/engram/commands/stats.md
- .omp-plugin/marketplace.json
- plugin/engram/commands/doctor.md
- README.md
- plugin/engram/commands/setup.md
- plugin/engram/commands/retro.md
- CHANGELOG.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e27bbce6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| omp plugin marketplace add thebtf/engram-marketplace | ||
| omp plugin install engram@engram |
There was a problem hiding this comment.
When an OMP user runs this newly documented command, it clones thebtf/engram-marketplace, but the release sync in .github/workflows/sync-marketplace.yml copies only plugin/engram and generates .agents/plugins/marketplace.json; it never copies the new root .omp-plugin/marketplace.json into that repository. Thus the native OMP catalog added by this commit is not available at the installation URL, and future version updates cannot reach OMP users through this command. Copy the OMP catalog during marketplace sync or direct the command to the repository that actually contains it.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugin/engram/scripts/run-engram.js (1)
109-121: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winПредупреждение об
ENGRAM_AUTH_ADMIN_TOKENне мешает его пробросу в дочерний процесс.Токен остаётся в
process.envи передаётся бинарнику на строке 120. Если политика v6 действительно запрещает операторский ключ на рабочей станции, надёжнее его вычистить, а не только предупредить.🛡️ Возможная правка
if (process.env.ENGRAM_AUTH_ADMIN_TOKEN) { process.stderr.write( "[engram] WARN: ENGRAM_AUTH_ADMIN_TOKEN is set on this workstation. v6 forbids " + "this — the operator key belongs ONLY on the server host. Remove it from " + "your local agent config and use ENGRAM_TOKEN with a dashboard-issued keycard.\n" ); + delete process.env.ENGRAM_AUTH_ADMIN_TOKEN; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/engram/scripts/run-engram.js` around lines 109 - 121, Sanitize the child-process environment in the ENGRAM_AUTH_ADMIN_TOKEN warning path: remove ENGRAM_AUTH_ADMIN_TOKEN from the environment passed to checkedSpawnSync while preserving all other process.env values and the existing warning behavior. Update the environment construction in the “Run the engram binary as a child process” block so the forbidden token cannot reach the engram binary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/engram/scripts/run-engram.js`:
- Around line 159-161: Update the ompCacheSlot/ompMatch logic in the cache-path
parsing block to ensure ompCacheSlot is always a string when the path ends at
“cache/plugins” and no following segment exists. Preserve the existing matching
and length checks for valid plugin cache paths.
In `@plugin/engram/scripts/run-engram.test.js`:
- Around line 80-85: Update the version assertions in the manifest test around
claudePlugin, codexPlugin, rootPlugin, claudeMarketplace, and ompMarketplace to
verify the release target is 6.46.2, preferably through one authoritative
version assertion while retaining consistency checks between manifests.
---
Nitpick comments:
In `@plugin/engram/scripts/run-engram.js`:
- Around line 109-121: Sanitize the child-process environment in the
ENGRAM_AUTH_ADMIN_TOKEN warning path: remove ENGRAM_AUTH_ADMIN_TOKEN from the
environment passed to checkedSpawnSync while preserving all other process.env
values and the existing warning behavior. Update the environment construction in
the “Run the engram binary as a child process” block so the forbidden token
cannot reach the engram binary.
🪄 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: d53dc878-2b41-413c-9484-273994f9fc82
📒 Files selected for processing (16)
.claude-plugin/marketplace.json.claude-plugin/plugin.json.omp-plugin/marketplace.jsonCHANGELOG.mdREADME.mdplugin/engram/.claude-plugin/plugin.jsonplugin/engram/.codex-plugin/plugin.jsonplugin/engram/commands/cleanup.mdplugin/engram/commands/doctor.mdplugin/engram/commands/export.mdplugin/engram/commands/restart.mdplugin/engram/commands/retro.mdplugin/engram/commands/setup.mdplugin/engram/commands/stats.mdplugin/engram/scripts/run-engram.jsplugin/engram/scripts/run-engram.test.js
💤 Files with no reviewable changes (4)
- plugin/engram/commands/export.md
- plugin/engram/commands/restart.md
- plugin/engram/commands/cleanup.md
- plugin/engram/commands/retro.md
🚧 Files skipped from review as they are similar to previous changes (10)
- .claude-plugin/marketplace.json
- plugin/engram/commands/stats.md
- plugin/engram/.codex-plugin/plugin.json
- .omp-plugin/marketplace.json
- .claude-plugin/plugin.json
- plugin/engram/.claude-plugin/plugin.json
- README.md
- CHANGELOG.md
- plugin/engram/commands/setup.md
- plugin/engram/commands/doctor.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0714b3e2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Install the marketplace plugin first, then run `/engram:setup` to create the | ||
| universal `~/.engram/config.json` configuration and restart the host. |
There was a problem hiding this comment.
For a fresh Claude Code or OMP installation following this new recommended flow, /engram:setup requires the user to open /tokens and click “Generate token” before it can create the config, but this same README explicitly states that /tokens is absent and that no accepted browser keycard-issuance workflow exists. The documented installation therefore dead-ends unless the user already obtained a keycard out of band; direct users to the operator's verified issuance procedure or implement the advertised browser route.
AGENTS.md reference: AGENTS.md:L30-L39
Useful? React with 👍 / 👎.
| --- | ||
| description: Display Engram memory health and usage statistics | ||
| --- |
There was a problem hiding this comment.
When OMP discovers this command through the newly added frontmatter, the workflow cannot complete: it calls /api/learning/effectiveness-distribution and /api/learning/curve, neither of which has a route or handler in the current code, and finishes with admin(action="quality") even though internal/mcp/tools_admin.go advertises and dispatches only stats (plus flag-gated purge_project). Remove this command from the supported OMP surface or rewrite it exclusively against live actions and endpoints.
AGENTS.md reference: AGENTS.md:L32-L39
Useful? React with 👍 / 👎.
|
@coderabbitai review incremental |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef8e0c6eed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,3 +1,7 @@ | |||
| --- | |||
| description: Diagnose Engram MCP connectivity and subsystem health | |||
There was a problem hiding this comment.
When an OMP user discovers the newly advertised /engram:doctor and has the documented bare-origin URL, step 2(e) tells them to append /mcp. The current client instead connects over gRPC using the origin, and README.md explicitly says /mcp is a removed server transport and must not be appended, so following this diagnosis can turn a correct configuration into a broken one. Rewrite the command to preserve the bare origin before making it discoverable.
AGENTS.md reference: AGENTS.md:L30-L39
Useful? React with 👍 / 👎.
Actionable findings addressed in ef8e0c6; 124 plugin tests and manifest/version checks pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4953ec5462
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| The marketplace plugin registers the MCP server, skills, and slash commands in | ||
| Claude Code and Oh My Pi. Claude Code also activates the bundled lifecycle hooks; |
There was a problem hiding this comment.
When an OMP user invokes the newly exposed /engram:issue, its mandatory tracked-project check runs curl with $ENGRAM_AUTH_ADMIN_TOKEN against http://$ENGRAM_HOST/api/issues/tracked-projects (plugin/engram/commands/issue.md), but the OMP setup added here only writes server_url/api_token to ~/.engram/config.json, ENGRAM_HOST has no live definition elsewhere in the repository, and workstation operator tokens are explicitly forbidden. A correctly configured OMP installation therefore fails before triage; rewrite this check against the live worker-auth/config path or do not advertise this slash command to OMP.
AGENTS.md reference: AGENTS.md:L32-L39
Useful? React with 👍 / 👎.
|
Post-merge gate classification: the non-required Docker verify-images check was red because Trivy reported a HIGH/CRITICAL vulnerability while scanning the unchanged server image. The same failure reproduced on the authority-only PR #444 and ordinary plugin PR #443, while authority-guard and the full OS test matrix passed. This failure is pre-existing/external to the plugin marketplace diff and was not bypassed by changing image policy or product code. |
1 participant
Summary
Verification
After merge, cut tag v6.46.2 and verify marketplace sync.
Summary by CodeRabbit
Новые возможности
Исправления
Документация