Skip to content

fix: hook-wrapper variable expansion + embed instructions in binary#4

Merged
quangdang46 merged 1 commit into
mainfrom
devin/1778420000-fix-install-and-instructions
May 9, 2026
Merged

fix: hook-wrapper variable expansion + embed instructions in binary#4
quangdang46 merged 1 commit into
mainfrom
devin/1778420000-fix-install-and-instructions

Conversation

@quangdang46
Copy link
Copy Markdown
Owner

Summary

Two real-installation bugs surfaced while running curl | bash from main — both pre-existing, both prevent the published binary from working correctly out of the box.

Bug 1: install.sh leaked $HARNESS / $1 expansion at generation time

The two cat > "$hooks_dir/...sh" <<EOF blocks for the Claude/Codex hook wrappers used unquoted EOF, so the outer install.sh (running with set -u) tried to expand $HARNESS, $1, and MEMPALACE_HOOK_HARNESS while writing the wrapper. This produced:

main: line 544: HARNESS: unbound variable
main: line 558: HARNESS: unbound variable

…on every install. Worse, the resulting wrapper had $HARNESS and $1 stripped to empty strings, so:

  • ./mempal_save_hook.sh codex — argument ignored (always ran claude-code).
  • MEMPALACE_HOOK_HARNESS=codex ./mempal_save_hook.sh — env var ignored.
  • case "$HARNESS" collapsed to case "" and hit the unsupported-harness branch.

Fix: backslash-escape the variable refs we want preserved verbatim (\${1:-...}, \${MEMPALACE_HOOK_HARNESS:-...}, \$HARNESS) while leaving ${bin_path} to be substituted at install time. Added an inline comment explaining the escaping.

Bug 2: mpr instructions <topic> failed on every packaged binary

The released v0.1.5 binary fails:

$ mpr instructions init
Error: Instructions file not found: /home/runner/work/mempalace_rust/mempalace_rust/crates/core/../../instructions/init.md

…because cli.rs resolved the instructions directory via:

static INSTRUCTIONS_DIR: LazyLock<PathBuf> =
    LazyLock::new(|| PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../instructions"));

env!("CARGO_MANIFEST_DIR") is a compile-time macro — it bakes the build machine's path into the binary. On the GitHub Actions runner that's /home/runner/work/mempalace_rust/mempalace_rust/crates/core, which obviously doesn't exist on a user's machine. So mpr instructions init|search|mine|help|status was 100% broken in every release tarball ever shipped.

Fix: switch to include_str!. The five instruction .md files are embedded into the binary at compile time, so the topic text travels with the binary regardless of where it gets installed. Removed the unused INSTRUCTIONS_DIR LazyLock.

Files changed

  • install.sh — escaped variable refs in two hook-wrapper heredocs.
  • crates/core/src/cli.rs — replaced INSTRUCTIONS_DIR + runtime file read with five include_str! constants, simplified run_instructions() to a match. Added regression test test_instruction_content_is_embedded_not_runtime_path.

Review & Testing Checklist for Human

  • Spot-check the install diff: confirm that only \$HARNESS, \$1, and \$MEMPALACE_HOOK_HARNESS are escaped and that ${bin_path} remains unescaped.
  • After merging and tagging a new release, run curl -fsSL https://raw.githubusercontent.com/quangdang46/mempalace_rust/main/install.sh | bash on a fresh box and verify (a) no HARNESS: unbound variable warning appears in the install output, and (b) mpr instructions init prints the topic content instead of a "file not found" error.
  • Optional: run mempal_save_hook.sh codex to confirm the wrapper now respects the harness argument (previously it was silently ignored).
  • CI must pass on all four jobs.

Notes

  • Both bugs are pre-existing — not regressions from PR fix: bug fixes to Rust core #2 / PR fix: port three upstream hardening follow-ups to Rust core #3.
  • 387/387 tests passing locally (was 386 before; +1 new regression test).
  • Verified end-to-end on this VM:
    • Re-ran install.sh with HOME=/tmp/mpr_install_e2e/home — no warnings, wrapper script reads exactly as intended, executes with default claude-code, rejects unsupported harness.
    • Built local binary, ran mpr instructions init|search|mine|help|status from /tmp (away from source) — all print embedded content.
  • The new regression test asserts every embedded instruction body is non-empty AND that run_instructions(name) succeeds for every supported topic. The pre-fix implementation would have failed this test in any sandbox without access to the build-time manifest dir.
  • instructions/*.md source files are unchanged — only the load mechanism switches from runtime read to compile-time embed.

Two real-installation bugs surfaced while running curl|bash from main.

1. install.sh: heredoc expanded $HARNESS/$1/MEMPALACE_HOOK_HARNESS at
   generation time
   - The unquoted heredoc terminator `<<EOF` causes the outer
     install.sh shell (running with `set -u`) to expand variable refs
     in the wrapper body. $HARNESS and $1 are unbound in the
     installer's own scope, so they emit:
       main: line 544: HARNESS: unbound variable
       main: line 558: HARNESS: unbound variable
     to stderr (and would have failed under `set -e` if the heredoc
     hadn't already started writing).
   - Worse: the resulting wrapper had $HARNESS/$1 stripped to empty
     strings, which would silently break `mempal_save_hook.sh codex`
     and `MEMPALACE_HOOK_HARNESS=codex mempal_save_hook.sh`.
   - Fix: backslash-escape the variables we want preserved
     verbatim while keeping ${bin_path} substituted at install time.

2. cli.rs: `mpr instructions <topic>` failed on packaged binaries
   - The previous implementation computed
     `PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../instructions")`
     at compile time, baking the build machine's path into the
     released binary. A user running `mpr instructions init` after
     curl|bash hit:
       Error: Instructions file not found:
         /home/runner/work/mempalace_rust/mempalace_rust/crates/core/../../instructions/init.md
     because that path doesn't exist on their machine.
   - Fix: switch to `include_str!` for all five instruction files so
     the bodies travel with the binary regardless of install location.
   - Removed the now-unused `INSTRUCTIONS_DIR` LazyLock.
   - Added regression test
     `test_instruction_content_is_embedded_not_runtime_path` that
     calls `run_instructions` for every supported topic; the
     pre-fix code would have failed it inside any sandbox without
     access to the build-time manifest dir.

Tests: 387/387 passing locally (was 386; +1 new regression test).

Verified end-to-end on local machine:
- Re-ran install.sh with `HOME=/tmp/...` — no HARNESS warnings, wrapper
  generated correctly with literal $HARNESS, executes with default
  claude-code harness, rejects unsupported harness.
- Built local binary, ran `mpr instructions init|search|mine|help|status`
  from /tmp — all print embedded content, no "file not found" error.
@quangdang46 quangdang46 merged commit b119e6a into main May 9, 2026
4 checks passed
@quangdang46 quangdang46 deleted the devin/1778420000-fix-install-and-instructions branch May 9, 2026 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant