fix(plugin): wire Codex env and release binaries - #222
Conversation
Add a native Codex plugin manifest, make the shared MCP wrapper accept Codex env and Claude userConfig compatibility values, and fail fast on missing workstation credentials. Fix release binary version injection for cmd/engram and make marketplace sync run after successful Release Binary so plugin installs only advance after downloadable assets exist. Verification: validate_plugin.py plugin/engram; node --check plugin wrappers; go build ./...; go vet ./...; go test ./...; go test -tags=critical ./tests/critical/...; local release-binary matrix cross-build.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPR вводит версию 6.4.4 с нативной поддержкой Codex-плагина наряду с Claude-плагином, переводит Daemon на var и инжектит версию в сборку, добавляет проверку собранного бинарника, перерабатывает runtime-скрипты плагина и обновляет документацию и release-автоматику. ChangesRelease v6.4.4 with Codex Plugin Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 native support for Desktop Codex alongside Claude Code by adding a Codex plugin manifest, updating build and packaging configurations, and refactoring the Node.js wrapper scripts to handle Codex-specific environment variables and paths. Feedback suggests failing fast in the SessionStart hook if ENGRAM_TOKEN is missing to prevent unnecessary network requests that would result in authentication failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (!process.env.ENGRAM_URL) { | ||
| return '<engram-setup>\nEngram plugin is installed but not configured.\nSet environment variables to connect to your Engram server:\n export ENGRAM_URL=http://your-server:37777/mcp\n export ENGRAM_AUTH_ADMIN_TOKEN=your-token\nThen restart Claude Code.\n</engram-setup>'; | ||
| return '<engram-setup>\nEngram plugin is installed but not configured.\nSet ENGRAM_URL and ENGRAM_TOKEN to connect to your Engram server.\nClaude Code: run /engram:setup or edit ~/.claude/settings.json env.\nCodex: edit ~/.codex/config.toml [shell_environment_policy.set].\nNever put ENGRAM_AUTH_ADMIN_TOKEN on a workstation.\n</engram-setup>'; | ||
| } |
There was a problem hiding this comment.
Since the v6 token model strictly requires ENGRAM_TOKEN when ENGRAM_URL is configured, we should also fail fast in the SessionStart hook if ENGRAM_TOKEN is missing. Otherwise, the hook will attempt a live network request to the server, fail with a 401/403 authentication error, and print a warning to stderr before falling back to a stale or empty cache. Adding a check for process.env.ENGRAM_TOKEN here prevents unnecessary network requests and provides immediate, actionable feedback.
| if (!process.env.ENGRAM_URL) { | |
| return '<engram-setup>\nEngram plugin is installed but not configured.\nSet environment variables to connect to your Engram server:\n export ENGRAM_URL=http://your-server:37777/mcp\n export ENGRAM_AUTH_ADMIN_TOKEN=your-token\nThen restart Claude Code.\n</engram-setup>'; | |
| return '<engram-setup>\nEngram plugin is installed but not configured.\nSet ENGRAM_URL and ENGRAM_TOKEN to connect to your Engram server.\nClaude Code: run /engram:setup or edit ~/.claude/settings.json env.\nCodex: edit ~/.codex/config.toml [shell_environment_policy.set].\nNever put ENGRAM_AUTH_ADMIN_TOKEN on a workstation.\n</engram-setup>'; | |
| } | |
| if (!process.env.ENGRAM_URL || !process.env.ENGRAM_TOKEN) { | |
| return '<engram-setup>\nEngram plugin is installed but not configured.\nSet ENGRAM_URL and ENGRAM_TOKEN to connect to your Engram server.\nClaude Code: run /engram:setup or edit ~/.claude/settings.json env.\nCodex: edit ~/.codex/config.toml [shell_environment_policy.set].\nNever put ENGRAM_AUTH_ADMIN_TOKEN on a workstation.\n</engram-setup>'; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/sync-marketplace.yml:
- Around line 24-27: The workflow uses floating tags for the checkout action
(uses: actions/checkout@v4) in two places (e.g., the step named "Checkout
engram"); replace both occurrences with the exact commit SHA for the
actions/checkout repository (uses: actions/checkout@<full_commit_sha>) so the
action is pinned to a specific commit, preserving the existing with: ref: ...
input and keeping both checkout steps updated to the same pinned SHA.
In `@CHANGELOG.md`:
- Around line 10-32: Add a new reference link for the released section by adding
a "[6.4.4]:" footnote entry at the bottom of CHANGELOG.md that points to the
tag/range for v6.4.4, and update the existing "[Unreleased]:" reference to use
the new range "v6.4.4...HEAD" so the two link labels [6.4.4] and [Unreleased]
resolve correctly; edit the reference block at the end of the file where link
labels are defined to insert the [6.4.4] entry and adjust [Unreleased]
accordingly.
In `@Makefile`:
- Around line 4-5: Add a Makefile target that builds the MCP client binary at
./cmd/engram (e.g., target name "engram" or "client") using the existing LDFLAGS
so VERSION and internal/version.Daemon are embedded, and make the top-level
build target depend on that new target; specifically, create a rule that runs go
build -o bin/engram $(LDFLAGS) ./cmd/engram and add that target as a dependency
of the existing build target so make build also compiles and validates the
cmd/engram entry point.
In `@plugin/engram/hooks/session-start.js`:
- Around line 137-139: The guard in handleSessionStart currently only checks
process.env.ENGRAM_URL and shows the setup banner, but the logic should also
check process.env.ENGRAM_TOKEN so you don't proceed to network calls without
Authorization; update the guard in async function handleSessionStart to verify
both ENGRAM_URL and ENGRAM_TOKEN (e.g., if either is missing) and return the
same engram-setup banner instructing to set ENGRAM_URL and ENGRAM_TOKEN,
ensuring subsequent code that performs the network request will only run when
both env vars are present.
In `@plugin/engram/scripts/run-engram.js`:
- Around line 23-38: The wrapper only checks fs.existsSync(binaryPath) before
skipping ensure-binary.js; change run-engram.js to also verify the binary
version and invoke ensure-binary.js when the installed binary version is older
than the manifest-required version: obtain the required version from the plugin
manifest (or equivalent variable in this module), run the existing binary with a
safe version probe (e.g., spawnSync(binaryPath, ['--version'] or read sidecar
metadata) and parse the output, compare semver (or simple string) to the
required version, and if missing or lower, run the ensureBinary path exactly as
currently implemented (same env/stdio) so the binary is replaced/updated; keep
the existing fallback to only run ensure-binary.js when either missing or
version mismatched, and preserve the same variables: binaryPath, ensureBinary,
pluginRoot, pluginData.
In `@scripts/generate-plugin-config.sh`:
- Around line 19-25: Before copying, read and compare the "version" field from
plugin/engram/.claude-plugin/plugin.json and
plugin/engram/.codex-plugin/plugin.json and fail fast if they differ: extract
plugin.json.version from both source files (e.g., via jq or another JSON
parser), if values are unequal print a clear error including both versions and
exit non-zero, otherwise continue to run the existing cp commands that write to
$CLAUDE_OUTPUT_DIR/plugin.json and $CODEX_OUTPUT_DIR/plugin.json; ensure the
check runs before any cp and references the source files and the variables
CLAUDE_OUTPUT_DIR and CODEX_OUTPUT_DIR so the script refuses to proceed on
mismatch.
🪄 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: 8fe24c59-6a69-42fa-84c2-8bd842c80e59
📒 Files selected for processing (19)
.github/workflows/release-binary.yml.github/workflows/sync-marketplace.yml.goreleaser.yamlCHANGELOG.mdDockerfileMakefilecmd/engram/main.gointernal/handlers/engramcore/tools.gointernal/version/version.goplugin/engram/.claude-plugin/plugin.jsonplugin/engram/.codex-plugin/plugin.jsonplugin/engram/.mcp.jsonplugin/engram/commands/doctor.mdplugin/engram/commands/setup.mdplugin/engram/hooks/lib.jsplugin/engram/hooks/session-start.jsplugin/engram/scripts/ensure-binary.jsplugin/engram/scripts/run-engram.jsscripts/generate-plugin-config.sh
![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: 352f93ae41
ℹ️ 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".
| if (!isConfiguredEnvValue(process.env.ENGRAM_URL) || !isConfiguredEnvValue(process.env.ENGRAM_TOKEN)) { | ||
| return '<engram-setup>\nEngram plugin is installed but not configured.\nSet ENGRAM_URL and ENGRAM_TOKEN to connect to your Engram server.\nClaude Code: run /engram:setup or edit ~/.claude/settings.json env.\nCodex: edit ~/.codex/config.toml [shell_environment_policy.set].\nNever put ENGRAM_AUTH_ADMIN_TOKEN on a workstation.\n</engram-setup>'; |
There was a problem hiding this comment.
When Claude users configure the plugin through the /config UI/userConfig path, this commit now maps those values to ENGRAM_CLAUDE_USERCONFIG_URL and ENGRAM_CLAUDE_USERCONFIG_TOKEN for the MCP wrapper (.mcp.json and run-engram.js both consume those fallbacks), but the SessionStart hook still gates on only ENGRAM_URL and ENGRAM_TOKEN. In that configuration the MCP daemon can authenticate successfully while the hook always returns <engram-setup> and skips fetching/injecting static memories, rules, and issues; normalize the same fallback envs before this check or otherwise keep the documented userConfig path working for hooks too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugin/engram/hooks/session-start.js (1)
222-232: ⚡ Quick winДублирование
isConfiguredEnvValueв двух файлах.Функция
isConfiguredEnvValueидентична по логике сisConfiguredValueвrun-engram.js. Рассмотрите вынос вhooks/lib.jsдля единого источника истины и упрощения сопровождения.♻️ Предлагаемый рефакторинг
В
hooks/lib.jsдобавьте:function isConfiguredEnvValue(value) { if (typeof value !== 'string') { return false; } const trimmed = value.trim(); if (trimmed === '') { return false; } return !/^\$\{[^}]+\}$/.test(trimmed); } module.exports = { // ... existing exports isConfiguredEnvValue, };Затем используйте
lib.isConfiguredEnvValueв обоих скриптах.🤖 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/hooks/session-start.js` around lines 222 - 232, The function isConfiguredEnvValue is duplicated; extract it into hooks/lib.js as a single exported helper (export named isConfiguredEnvValue) and then remove the local copies in session-start.js and run-engram.js, replacing their usages with calls to lib.isConfiguredEnvValue (or the import alias you use when requiring hooks/lib.js); ensure the exported name matches the callers and update require/import statements in both files to import the helper from hooks/lib.js.plugin/engram/scripts/run-engram.js (1)
98-117: 💤 Low valueРассмотрите расширение фильтрации плейсхолдеров.
Регулярное выражение
/^\$\{[^}]+\}$/отклоняет только строки, состоящие целиком из одного плейсхолдера${...}. Значения вродеhttps://${HOST}или${A}${B}пройдут проверку и вызовут сбой позже при сетевом вызове.Если такие частичные плейсхолдеры возможны в конфигурации (например, пользователь скопировал шаблон из документации), стоит проверять наличие
${в любом месте строки.♻️ Возможное исправление
function isConfiguredValue(value) { if (typeof value !== "string") { return false; } const trimmed = value.trim(); if (!trimmed) { return false; } - return !/^\$\{[^}]+\}$/.test(trimmed); + // Reject any value containing unsubstituted ${...} placeholders + return !/\$\{[^}]+\}/.test(trimmed); }🤖 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 98 - 117, The isConfiguredValue function currently only rejects strings that are exactly a single placeholder via /^\$\{[^}]+\}$/, allowing partial templates like "https://${HOST}" or "${A}${B}" to slip through; update isConfiguredValue (used by configuredEnvValue) to also return false if the trimmed value contains the sequence "${" anywhere (e.g., if (trimmed.includes("${")) return false;) so any partial/embedded placeholders are treated as unconfigured and filtered out before being returned by configuredEnvValue.scripts/generate-plugin-config.sh (1)
12-25: 💤 Low valueПредыдущая проблема устранена; опционально — добавить валидацию непустой версии.
Добавление функции
read_manifest_versionи fail-fast проверки на строках 22-25 полностью решает проблему, поднятую в предыдущем review: теперь скрипт отказывается продолжать работу, если версии в.claude-plugin/plugin.jsonи.codex-plugin/plugin.jsonрасходятся, что предотвращает ситуацию, когда Claude и Codex тянут разные релизы.Опциональное улучшение: сейчас
read_manifest_versionвозвращает пустую строку, если полеversionотсутствует в JSON (строка 16). Если обе manifest-файла будут иметь пропущенное поле версии, проверка"$CLAUDE_VERSION" != "$CODEX_VERSION"пропустит это (обе пустые строки равны). Хотя это маловероятный edge case (manifest'ы закоммичены и проверены), добавление валидации[ -z "$CLAUDE_VERSION" ]перед сравнением сделало бы скрипт более защищённым.♻️ Опциональное улучшение: проверка непустой версии
CLAUDE_VERSION="$(read_manifest_version "$CLAUDE_MANIFEST")" CODEX_VERSION="$(read_manifest_version "$CODEX_MANIFEST")" +if [ -z "$CLAUDE_VERSION" ] || [ -z "$CODEX_VERSION" ]; then + echo "Plugin manifest version is empty: CLAUDE=$CLAUDE_VERSION, CODEX=$CODEX_VERSION" >&2 + exit 1 +fi + if [ "$CLAUDE_VERSION" != "$CODEX_VERSION" ]; then echo "Plugin manifest version mismatch: $CLAUDE_MANIFEST=$CLAUDE_VERSION, $CODEX_MANIFEST=$CODEX_VERSION" >&2 exit 1 fi🤖 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 `@scripts/generate-plugin-config.sh` around lines 12 - 25, Add a pre-check for empty manifest versions before the existing comparison: after obtaining CLAUDE_VERSION and CODEX_VERSION (from read_manifest_version), test if either is empty (e.g., [ -z "$CLAUDE_VERSION" ] || [ -z "$CODEX_VERSION" ]) and exit non‑zero with a clear error via stderr that names the manifest(s) missing a version; then proceed to the existing inequality check between CLAUDE_VERSION and CODEX_VERSION. This uses the existing symbols read_manifest_version, CLAUDE_VERSION and CODEX_VERSION to locate where to insert the validation.
🤖 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 `@Makefile`:
- Line 65: The global export CGO_ENABLED=1 causes the engram target to produce a
CGO-dependent binary locally; change the engram make target to explicitly build
with CGO_DISABLED by setting CGO_ENABLED=0 for that target's build command
(e.g., prefix the go build invocation for cmd/engram with CGO_ENABLED=0 and
preserve existing LDFLAGS), or alternatively remove the global export and set
CGO_ENABLED only for tests—ensure the make target named "engram" (and the
dependency chain from "build") forces CGO_ENABLED=0 so local artifacts match the
CI release workflow.
---
Nitpick comments:
In `@plugin/engram/hooks/session-start.js`:
- Around line 222-232: The function isConfiguredEnvValue is duplicated; extract
it into hooks/lib.js as a single exported helper (export named
isConfiguredEnvValue) and then remove the local copies in session-start.js and
run-engram.js, replacing their usages with calls to lib.isConfiguredEnvValue (or
the import alias you use when requiring hooks/lib.js); ensure the exported name
matches the callers and update require/import statements in both files to import
the helper from hooks/lib.js.
In `@plugin/engram/scripts/run-engram.js`:
- Around line 98-117: The isConfiguredValue function currently only rejects
strings that are exactly a single placeholder via /^\$\{[^}]+\}$/, allowing
partial templates like "https://${HOST}" or "${A}${B}" to slip through; update
isConfiguredValue (used by configuredEnvValue) to also return false if the
trimmed value contains the sequence "${" anywhere (e.g., if
(trimmed.includes("${")) return false;) so any partial/embedded placeholders are
treated as unconfigured and filtered out before being returned by
configuredEnvValue.
In `@scripts/generate-plugin-config.sh`:
- Around line 12-25: Add a pre-check for empty manifest versions before the
existing comparison: after obtaining CLAUDE_VERSION and CODEX_VERSION (from
read_manifest_version), test if either is empty (e.g., [ -z "$CLAUDE_VERSION" ]
|| [ -z "$CODEX_VERSION" ]) and exit non‑zero with a clear error via stderr that
names the manifest(s) missing a version; then proceed to the existing inequality
check between CLAUDE_VERSION and CODEX_VERSION. This uses the existing symbols
read_manifest_version, CLAUDE_VERSION and CODEX_VERSION to locate where to
insert the validation.
🪄 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: 1842409f-7595-4eb0-8e98-3208b75c3118
📒 Files selected for processing (6)
.github/workflows/sync-marketplace.ymlCHANGELOG.mdMakefileplugin/engram/hooks/session-start.jsplugin/engram/scripts/run-engram.jsscripts/generate-plugin-config.sh
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/sync-marketplace.yml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40eacf8916
ℹ️ 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".
| function buildRequestHeaders(includeJsonBody = false) { | ||
| const headers = {}; | ||
| const token = process.env.ENGRAM_AUTH_ADMIN_TOKEN; | ||
| const token = process.env.ENGRAM_TOKEN; |
There was a problem hiding this comment.
When Claude installs use the /config userConfig path, this commit places the credentials in ENGRAM_CLAUDE_USERCONFIG_* and only session-start/run-engram normalize them back to ENGRAM_TOKEN; other hooks such as user-prompt.js and stop.js call lib.requestPost directly. In that configuration this header builder sees no token, so those hook POSTs go out unauthenticated and the server rejects segment/correction/session-end events even though the MCP daemon and session-start fetch work. Centralize the same fallback normalization in lib (or call it from every hook) before building the Authorization header.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
Release notes
Patch release target: v6.4.4. Do not tag until CI is green and the release branch is merged to main.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores