refactor(plugin/claude-code): extract installer wrapper to checked-in file#2024
Closed
t0saki wants to merge 2 commits into
Closed
refactor(plugin/claude-code): extract installer wrapper to checked-in file#2024t0saki wants to merge 2 commits into
t0saki wants to merge 2 commits into
Conversation
The installer-emitted claude() wrapper had been inlined as a marker-delimited block in the user's ~/.zshrc / ~/.bashrc. Every upgrade required the awk-strip-and-append dance, the inline noise was hostile to anyone reading their own rc, and there was a known footgun: if the END marker got hand-deleted from the rc, the next install's awk-strip would drop everything from the BEGIN marker to EOF. Switch to the standard pyenv / nvm / fnm pattern, mirroring what the codex-memory-plugin installer now does (see volcengine#2023): - Wrapper body lives in its own file at ~/.openviking/claude-plugin.rc.sh (path overridable via OPENVIKING_CLAUDE_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 marker-wrapped for clean uninstall: # >>> openviking claude-code memory plugin >>> [ -f "$HOME/.openviking/claude-plugin.rc.sh" ] && . "..." # <<< openviking claude-code memory plugin <<< Content is constant across installs, so the marker-replacement logic only triggers the legacy-cleanup path once (when upgrading from a pre-rc-split install that inlined the full claude() function body). User-visible improvements: - ~/.zshrc OV-plugin block: ~16 lines of wrapper body → 3 lines. - Wrapper body is a real file you can `cat` / `diff` / restore from source control; no need to re-run the installer to inspect it. - Uninstall: `rm ~/.openviking/claude-plugin.rc.sh` + delete the 3-line marker block. - The END-marker corruption footgun is gone, since the marker block content is bytestring-stable and the awk-strip only runs when both markers are present anyway. No behavior change to the wrapper itself (still pulls url/api_key from ovcli.conf via jq).
…per/wrapper.sh
Squash-style follow-up to the previous rc-split commit on this branch:
now that the wrapper lives in its own file conceptually, just check it
in at examples/claude-code-memory-plugin/setup-helper/wrapper.sh and
have the user's shell rc source it directly from the cloned plugin
checkout. No copy step, no heredoc dance in install.sh.
Why this is better than the previous approach (wrapper body embedded as
a heredoc in install.sh, written to a copy in ~/.openviking):
- Wrapper is a real reviewable file. Diffs show `+ claude() { ... }`,
not "+ heredoc lines that produce the wrapper".
- Updates ride on the installer's existing `git fetch + reset --hard`
step — no separate "re-run installer to refresh the copy" path.
- One less source of truth (no $HOME copy that can drift from the
installer's intent).
- Uninstall: `rm ~/.openviking/openviking-repo`; the source hook in the
rc silently no-ops via `[ -f ... ] && .`.
The previous commit on this branch already shrunk the rc block from
~16 inline lines to 3 (marker + source hook + marker). This commit just
moves the wrapper body from "embedded in installer" to "checked into the
repo at a stable path", with no behavior change to the wrapper itself.
5 tasks
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
Contributor
Author
|
Superseded by #2026 (combined codex + claude-code wrapper refactor). |
There was a problem hiding this comment.
Pull request overview
This PR refactors the claude-code-memory-plugin installer to stop inlining a large claude() shell wrapper into the user’s shell rc file, and instead source a checked-in wrapper file from the cloned plugin checkout.
Changes:
- Added a standalone
claude()wrapper atexamples/claude-code-memory-plugin/setup-helper/wrapper.sh. - Updated the installer to write a marker-delimited rc snippet that sources the wrapper file from
$REPO_DIRinstead of embedding the full function body.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| examples/claude-code-memory-plugin/setup-helper/wrapper.sh | New checked-in claude() wrapper meant to be sourced from the user’s shell rc. |
| examples/claude-code-memory-plugin/setup-helper/install.sh | Installer now writes a small source-hook block into the user’s shell rc instead of inlining the wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+192
to
+195
| SOURCE_HOOK="[ -f \"$WRAPPER_SRC\" ] && . \"$WRAPPER_SRC\"" | ||
| SOURCE_BLOCK="$MARKER_BEGIN | ||
| $SOURCE_HOOK | ||
| $MARKER_END" |
Comment on lines
+204
to
211
| info "Replacing openviking source hook in $RC" | ||
| # Strip existing block (whether it's the new one-liner or an old | ||
| # inline-wrapper block from a previous version). | ||
| awk -v b="$MARKER_BEGIN" -v e="$MARKER_END" ' | ||
| $0 == b {skip=1; next} | ||
| $0 == e {skip=0; next} | ||
| !skip | ||
| ' "$RC" > "$RC.tmp" && mv "$RC.tmp" "$RC" |
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 #2023 (in flight), applied to the
claude-code-memory-plugininstaller.Before
The installer-emitted
claude()wrapper was inlined as a marker-delimited block inside the user's~/.zshrc/~/.bashrc. Upgrade flow usedawkto strip the existing block and append a new one. Drawbacks:awkstrip would drop everything from the BEGIN marker to EOF.cat >> "$RC" <<EOF-with-escaped-$heredoc inside install.sh — annoying to review, annoying to syntax-check.After
examples/claude-code-memory-plugin/setup-helper/wrapper.shas a checked-in source file. Real syntax highlighting, real diffable history.$REPO_DIR/examples/claude-code-memory-plugin/setup-helper/wrapper.sh).git fetch + reset --hardstep. No "re-run installer to refresh the wrapper" path.Same pattern as #2023
This is the same shape applied to the codex installer in #2023 (open). I'm proposing it as a separate PR so the claude-code change can be reviewed / merged independently — there's no functional dependency between the two.
Behavior preserved
The wrapper itself is byte-for-byte the same
claude()function as before (jq to read ovcli.conf → injectOPENVIKING_URL/OPENVIKING_API_KEY→ exec claude). This PR only changes where the wrapper code lives.Test plan
bash -n examples/claude-code-memory-plugin/setup-helper/install.shpassesbash -n examples/claude-code-memory-plugin/setup-helper/wrapper.shpassessource ~/.zshrc && type claudeshows the wrapper loaded from the standalone fileclaudeinvocation still injectsOPENVIKING_URL/OPENVIKING_API_KEYfrom ovcli.conf