refactor(plugin/codex): extract installer wrapper to checked-in file#2025
Closed
t0saki wants to merge 2 commits into
Closed
refactor(plugin/codex): extract installer wrapper to checked-in file#2025t0saki wants to merge 2 commits into
t0saki wants to merge 2 commits into
Conversation
The installer-emitted codex() wrapper had grown to ~60 lines of shell
function body inlined as a marker-delimited block inside the user's
~/.zshrc / ~/.bashrc. Every upgrade required the awk-strip-and-append
dance, which had a known edge case (rc with begin-marker but no
end-marker) we'd already had to harden against, and the inline noise
was hostile to anyone reading their own rc.
This commit switches to the standard pyenv / nvm / fnm pattern:
- Wrapper body lives in its own file at ~/.openviking/codex-plugin.rc.sh
(path overridable via OPENVIKING_CODEX_WRAPPER_RC). Full overwrite on
every install — no marker logic inside the wrapper file itself.
- The user's shell rc gets a single one-line source hook, still wrapped
in marker comments for cleanup-on-uninstall:
# >>> openviking-codex-plugin >>>
[ -f "$HOME/.openviking/codex-plugin.rc.sh" ] && . "..."
# <<< openviking-codex-plugin <<<
Since the content of this block never changes across installs, the
marker-replacement logic only triggers the legacy-cleanup path once
when upgrading from a pre-rc-split install that inlined the full
wrapper.
User-visible improvements:
- ~/.zshrc OV-plugin block: ~70 lines → 3 lines.
- `cat ~/.openviking/codex-plugin.rc.sh` shows the wrapper directly.
- Uninstall is just `rm ~/.openviking/codex-plugin.rc.sh` + delete the
3-line block — no awk required.
- Upgrades touch the rc file at most once (to install the source hook);
subsequent installs only rewrite the wrapper file.
Verified end-to-end: stale install with the old inline wrapper got the
3-line source hook substituted in place; `source ~/.zshrc && type codex`
showed the new wrapper loaded from the standalone file.
…ance
Follow-up to the rc-split commit. Instead of embedding the wrapper body
as a heredoc inside install.sh and writing it to a copy under
~/.openviking/, the wrapper now lives as its own checked-in file at
examples/codex-memory-plugin/setup-helper/wrapper.sh. The user's shell
rc sources that file directly from the cloned plugin checkout (the path
the installer already manages via git fetch + reset --hard).
What this buys:
- Wrapper diffs are real diffs — code review sees `+ codex() { ... }`
rather than `+ heredoc lines inside install.sh that produce
~/.openviking/codex-plugin.rc.sh`.
- No copy step in the installer means no installer code path for "did
the user accidentally edit ~/.openviking/codex-plugin.rc.sh?" or "is
the copied file in sync with what the installer would produce now?"
- Updates ride for free on `git pull` / the installer's existing
fetch+reset. No "re-run installer to refresh the wrapper" step.
- Uninstall is just `rm ~/.openviking/openviking-repo` (or just leave it
— the source hook will silently no-op when the file is gone, since
it's gated with `[ -f ... ] && .`).
Installer shrinks from ~420 lines (with the inline heredoc) to ~340.
The wrapper is unchanged content-wise; this commit only moves where it
lives.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
5 tasks
Collaborator
Author
|
Superseded by #2026 (combined codex + claude-code wrapper refactor). |
There was a problem hiding this comment.
Pull request overview
Refactors the Codex memory plugin installer to stop inlining the codex() shell wrapper into the user’s shell rc, and instead source a checked-in wrapper script from the cloned plugin checkout so wrapper updates flow automatically with the installer’s repo refresh.
Changes:
- Extracts the
codex()wrapper implementation intoexamples/codex-memory-plugin/setup-helper/wrapper.sh. - Updates
install.shto write a minimal marker-delimited “source hook” into the user’s rc instead of emitting the full function body. - Removes the heredoc-based embedded wrapper body from
install.sh.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| examples/codex-memory-plugin/setup-helper/install.sh | Replaces inline wrapper emission with a marker-delimited rc source hook to the checked-in wrapper. |
| examples/codex-memory-plugin/setup-helper/wrapper.sh | Adds the standalone codex() wrapper script that resolves config/env and keeps cached .mcp.json in sync. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+278
to
281
| SOURCE_HOOK="[ -f \"$WRAPPER_SRC\" ] && . \"$WRAPPER_SRC\"" | ||
| SOURCE_BLOCK="$WRAPPER_MARKER_BEGIN | ||
| $SOURCE_HOOK | ||
| $WRAPPER_MARKER_END" |
Comment on lines
+269
to
+277
| # The user's shell rc gets a single one-line source hook pointing directly | ||
| # at the wrapper source in the cloned plugin checkout. No copy step: | ||
| # updates to wrapper.sh propagate via the `git fetch + reset --hard` the | ||
| # installer runs at the top, with no extra installer step required. | ||
| # | ||
| # It also re-renders the cached .mcp.json's bearer_token_env_var field on | ||
| # every codex launch, based on whichever ovcli.conf the user pointed | ||
| # OPENVIKING_CLI_CONFIG_FILE at this time. That lets a single install | ||
| # handle both authenticated and unauthenticated configs without forcing | ||
| # a re-install when the user swaps configs (e.g. to isolate a benchmark | ||
| # run from production memory). | ||
| # | ||
| # Codex 0.130 hard-fails MCP startup when bearer_token_env_var points at | ||
| # an EMPTY env var ("Environment variable ... is empty"), and also when | ||
| # bearer_token_env_var is present but the env var is unset. So when the | ||
| # resolved api_key is empty, we both drop the field from .mcp.json AND | ||
| # omit OPENVIKING_API_KEY from the env passed to codex. | ||
| read -r -d '' WRAPPER_BODY <<'WRAPPER' || true | ||
| codex() { | ||
| local _ov_conf="${OPENVIKING_CLI_CONFIG_FILE:-$HOME/.openviking/ovcli.conf}" | ||
| if ! command -v node >/dev/null 2>&1; then | ||
| command codex "$@" | ||
| return | ||
| fi | ||
|
|
||
| # Resolve OV connection settings: existing env > ovcli.conf > nothing. | ||
| local _ov_url _ov_key _ov_account _ov_user | ||
| if [ -f "$_ov_conf" ]; then | ||
| local _ov_env | ||
| _ov_env=$(node -e ' | ||
| try { | ||
| const c = JSON.parse(require("node:fs").readFileSync(process.argv[1], "utf8")); | ||
| const out = (k, v) => v ? `${k}=${JSON.stringify(String(v))}\n` : ""; | ||
| process.stdout.write( | ||
| out("OV_URL", c.url) + | ||
| out("OV_KEY", c.api_key) + | ||
| out("OV_ACCOUNT", c.account) + | ||
| out("OV_USER", c.user) | ||
| ); | ||
| } catch {} | ||
| ' "$_ov_conf" 2>/dev/null) | ||
| eval "$_ov_env" | ||
| fi | ||
| _ov_url="${OPENVIKING_URL:-${OV_URL:-}}" | ||
| _ov_key="${OPENVIKING_API_KEY:-${OV_KEY:-}}" | ||
| _ov_account="${OPENVIKING_ACCOUNT:-${OV_ACCOUNT:-}}" | ||
| _ov_user="${OPENVIKING_USER:-${OV_USER:-}}" | ||
| unset OV_URL OV_KEY OV_ACCOUNT OV_USER | ||
|
|
||
| # Sync cache .mcp.json to current OV connection state: rewrite both the | ||
| # URL (so OPENVIKING_CLI_CONFIG_FILE swaps actually change the target) | ||
| # and the bearer_token_env_var field (Codex 0.130 hard-fails on empty | ||
| # bearer env vars, so the field must be absent in no-auth mode). The | ||
| # node script writes only when something actually changes — idempotent | ||
| # fast-path so we don't bump file mtime on every codex launch. | ||
| local _has_key _mcp_url_from_conf | ||
| if [ -n "$_ov_key" ]; then _has_key=1; else _has_key=0; fi | ||
| if [ -n "$_ov_url" ]; then | ||
| # Strip trailing slashes then append /mcp (matching the install-time | ||
| # resolve_mcp_url logic). OPENVIKING_MCP_URL env can fully override. | ||
| if [ -n "${OPENVIKING_MCP_URL:-}" ]; then | ||
| _mcp_url_from_conf="$OPENVIKING_MCP_URL" | ||
| else | ||
| _mcp_url_from_conf="${_ov_url%/}/mcp" | ||
| fi | ||
| else | ||
| _mcp_url_from_conf="" | ||
| fi | ||
| local _cache_mcp | ||
| for _cache_mcp in "$HOME"/.codex/plugins/cache/openviking-plugins-local/openviking-memory/*/.mcp.json; do | ||
| [ -f "$_cache_mcp" ] || continue | ||
| node -e ' | ||
| const fs = require("node:fs"); | ||
| // node -e: argv is [node, file, hasKey, url] — no [eval] placeholder. | ||
| const file = process.argv[1]; | ||
| const hasKey = process.argv[2]; | ||
| const url = process.argv[3] || ""; | ||
| const j = JSON.parse(fs.readFileSync(file, "utf8")); | ||
| const s = j.mcpServers && j.mcpServers["openviking-memory"]; | ||
| if (s) { | ||
| let changed = false; | ||
| if (url && s.url !== url) { | ||
| s.url = url; | ||
| changed = true; | ||
| } | ||
| const cur = s.bearer_token_env_var || ""; | ||
| if (hasKey === "1" && cur !== "OPENVIKING_API_KEY") { | ||
| s.bearer_token_env_var = "OPENVIKING_API_KEY"; | ||
| changed = true; | ||
| } else if (hasKey !== "1" && cur) { | ||
| delete s.bearer_token_env_var; | ||
| changed = true; | ||
| } | ||
| if (changed) { | ||
| fs.writeFileSync(file, JSON.stringify(j, null, 2) + "\n"); | ||
| } | ||
| } | ||
| ' "$_cache_mcp" "$_has_key" "$_mcp_url_from_conf" 2>/dev/null || true | ||
| done | ||
|
|
||
| # Build env-prefix dynamically so empty values are NOT exported as empty | ||
| # strings — Codex hard-fails on empty bearer_token_env_var targets. | ||
| local -a _env_args=() | ||
| [ -n "$_ov_url" ] && _env_args+=("OPENVIKING_URL=$_ov_url") | ||
| [ -n "$_ov_key" ] && _env_args+=("OPENVIKING_API_KEY=$_ov_key") | ||
| [ -n "$_ov_account" ] && _env_args+=("OPENVIKING_ACCOUNT=$_ov_account") | ||
| [ -n "$_ov_user" ] && _env_args+=("OPENVIKING_USER=$_ov_user") | ||
| _env_args+=("OPENVIKING_AGENT_ID=${OPENVIKING_AGENT_ID:-codex}") | ||
|
|
||
| env "${_env_args[@]}" codex "$@" | ||
| } | ||
| WRAPPER | ||
|
|
||
| # Wrap the function body with marker lines. | ||
| WRAPPER_BLOCK="$WRAPPER_MARKER_BEGIN | ||
| $WRAPPER_BODY | ||
| # The hook content stays stable across installs (only the absolute path | ||
| # matters), so the marker-replacement logic only triggers the legacy | ||
| # cleanup path once when upgrading from a pre-rc-split install that | ||
| # inlined the full wrapper into the rc. |
Comment on lines
+3
to
+5
| # Installed by examples/codex-memory-plugin/setup-helper/install.sh to | ||
| # ${OPENVIKING_CODEX_WRAPPER_RC:-~/.openviking/codex-plugin.rc.sh}. The | ||
| # installer copies this file verbatim — re-run the installer to update. |
Comment on lines
+75
to
+77
| local _cache_mcp | ||
| for _cache_mcp in "$HOME"/.codex/plugins/cache/openviking-plugins-local/openviking-memory/*/.mcp.json; do | ||
| [ -f "$_cache_mcp" ] || continue |
Comment on lines
+37
to
+51
| local _ov_env | ||
| _ov_env=$(node -e ' | ||
| try { | ||
| const c = JSON.parse(require("node:fs").readFileSync(process.argv[1], "utf8")); | ||
| const out = (k, v) => v ? `${k}=${JSON.stringify(String(v))}\n` : ""; | ||
| process.stdout.write( | ||
| out("OV_URL", c.url) + | ||
| out("OV_KEY", c.api_key) + | ||
| out("OV_ACCOUNT", c.account) + | ||
| out("OV_USER", c.user) | ||
| ); | ||
| } catch {} | ||
| ' "$_ov_conf" 2>/dev/null) | ||
| eval "$_ov_env" | ||
| fi |
Comment on lines
+66
to
+71
| if [ -n "$_ov_url" ]; then | ||
| if [ -n "${OPENVIKING_MCP_URL:-}" ]; then | ||
| _mcp_url_from_conf="$OPENVIKING_MCP_URL" | ||
| else | ||
| _mcp_url_from_conf="${_ov_url%/}/mcp" | ||
| fi |
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Same refactor as #2024 (in flight for the claude-code installer), applied to the codex installer. PR #2023 carried the bug fixes (empty api_key, runtime config swap, URL re-render) but I pushed these refactor commits after the squash-merge fired, so they got left behind.
What changes
Wrapper body extracted to
examples/codex-memory-plugin/setup-helper/wrapper.shas a checked-in source file. Real syntax highlighting, real diffable history, no moreread -r -d '' WRAPPER_BODY <<'WRAPPER'heredoc inside install.sh.No copy step. The installer doesn't write a copy of the wrapper to
~/.openviking/. The user's shell rc sources the wrapper directly from the cloned plugin checkout path:Updates ride for free on the installer's existing
git fetch + reset --hardstep. No "re-run installer to refresh the wrapper" path.Shell rc footprint drops from ~60 lines of inline function body to 3 lines (marker + source hook + marker).
Marker-replacement on re-install only triggers the legacy-cleanup pass once — when upgrading from the pre-split installer that inlined the full wrapper into the rc.
Behavior preserved
Wrapper body is byte-for-byte the same as what shipped in #2023 — same ovcli.conf resolution, same .mcp.json cache rewriting (URL + bearer_token_env_var sync), same env-args build. Only the wrapper's storage location changes.
Installer size
install.sh: ~420 lines (with embedded heredoc) → ~340
wrapper.sh: 117 lines (new, was inline)
Test plan
bash -n+zsh -npass on both filessource ~/.zshrc && type codexshows the wrapper loaded from the standalone filecodexinvocation (verified bycat ~/.codex/plugins/cache/.../0.5.0/.mcp.jsonbefore/after runningcodexwith differentOPENVIKING_CLI_CONFIG_FILEvalues)rm ~/.openviking/openviking-repono-ops the source hook (no error, function just isn't defined)