Skip to content

fix(hooks): quote plugin script paths#487

Merged
rohitg00 merged 1 commit into
rohitg00:mainfrom
honor2030:fix/hooks-quote-plugin-paths
May 19, 2026
Merged

fix(hooks): quote plugin script paths#487
rohitg00 merged 1 commit into
rohitg00:mainfrom
honor2030:fix/hooks-quote-plugin-paths

Conversation

@honor2030
Copy link
Copy Markdown
Contributor

@honor2030 honor2030 commented May 18, 2026

Summary

  • Quote AgentMemory plugin script paths in Claude and Codex hook command templates.
  • Add a regression check that hook commands still reference the expected bridge script while remaining safe for plugin roots containing spaces.

Test Plan

  • RED: npm test -- test/codex-plugin.test.ts failed before the implementation when the new quoted-path assertion expected shell-safe script paths.
  • GREEN: npm test -- test/codex-plugin.test.ts test/context-injection.test.ts → 12 tests passed.
  • GREEN: npm run build
  • GREEN: git diff --check

Closes #477

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Fixed plugin hooks to correctly handle installation paths containing spaces. Hook commands now properly wrap path variables in quotes to ensure reliable execution regardless of installation directory name.

Review Change Stack

@vercel
Copy link
Copy Markdown

vercel Bot commented May 18, 2026

@honor2030 is attempting to deploy a commit to the rohitg00's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 18, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5171b787-2540-4be4-960e-c474dffd1698

📥 Commits

Reviewing files that changed from the base of the PR and between 9061da5 and ee3f166.

📒 Files selected for processing (3)
  • plugin/hooks/hooks.codex.json
  • plugin/hooks/hooks.json
  • test/codex-plugin.test.ts

📝 Walkthrough

Walkthrough

This PR fixes hook command failures on Windows when plugin root paths contain spaces by adding double quotes around the ${CLAUDE_PLUGIN_ROOT} variable in all hook command strings across both hook manifests, along with test validation to enforce and maintain the quoted format.

Changes

Hook Command Path Quoting

Layer / File(s) Summary
Hook manifest command path quoting
plugin/hooks/hooks.codex.json, plugin/hooks/hooks.json
All hook command strings now wrap ${CLAUDE_PLUGIN_ROOT}/scripts/*.mjs in double quotes when passed to node, preventing shell path splitting on Windows when usernames contain spaces. Changes applied to SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, PostToolUseFailure, PreCompact, SubagentStart, SubagentStop, Notification, TaskCompleted, Stop, and SessionEnd hooks.
Hook command validation tests
test/codex-plugin.test.ts
Added HookHandler and HookEntry types with a hookCommands() helper that loads and validates all hook command strings match the quoted node "${CLAUDE_PLUGIN_ROOT}/scripts/<name>.mjs" format. Existing path extraction test regex refined to correctly parse the quoted format without whitespace leakage.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • rohitg00/agentmemory#311: Both PRs touch the Codex hook manifest and its command-string parsing/validation—main PR updates quoted ${CLAUDE_PLUGIN_ROOT}/scripts/*.mjs commands in plugin/hooks/hooks.codex.json (and adjusts test/codex-plugin.test.ts to match), which directly builds on the Codex plugin/marketplace + hook definitions shipped in PR #311.

Poem

🐰 A path with spaces once broke with a creak,
On Windows machines where quotes we did seek,
Now "${CLAUDE_PLUGIN_ROOT}" keeps the path whole,
No more split-on-spaces—we've fixed this bug's role! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(hooks): quote plugin script paths' accurately summarizes the main change—adding quotes around plugin script paths in hook configurations to fix path handling with spaces.
Linked Issues check ✅ Passed The pull request fulfills all coding requirements from issue #477: quotes are added to all 11 affected node commands in hooks.json, hooks.codex.json is updated consistently, and regression tests verify the fix without breaking existing functionality.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #477: quoting script paths in hooks.json and hooks.codex.json, plus adding tests to prevent regression. No unrelated modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rohitg00 rohitg00 merged commit 48bf700 into rohitg00:main May 19, 2026
3 of 4 checks passed
@rohitg00
Copy link
Copy Markdown
Owner

Merged + shipping in next release.

Thanks @honor2030 — closes #477 (hooks crashing on Windows usernames with spaces). The path-quoting fix + the regression test in test/codex-plugin.test.ts both look clean.

jonathanzhan1975 pushed a commit to jonathanzhan1975/agentmemory that referenced this pull request May 19, 2026
Co-authored-by: honor2030 <19909783+honor2030@users.noreply.github.com>
This was referenced May 19, 2026
rohitg00 added a commit that referenced this pull request May 19, 2026
Quality + integration wave. Bundles 11 PRs since v0.9.20:

Contributor feature:
- #237 OpenCode plugin with 22 auto-capture hooks (@cl0ckt0wer)

Bug fixes (9):
- #516 memory_recall endpoint + format/token_budget (@serhiizghama, closes #507/#440)
- #461 env-file AGENTMEMORY_DROP_STALE_INDEX flag honored (@honor2030, closes #456)
- #487 Windows hook path quoting (@honor2030, closes #477)
- #517 viewer IME composition guard (@jonathanzhan1975)
- #472 chunk large sessions for LLM context window (@efenex)
- #473 surface lessons in smart-search + diagnose tally (@efenex)
- #486 declare all Hermes plugin hooks (@honor2030)
- #500 rebuildIndex non-blocking on boot (@efenex)
- #504 batched embed in rebuildIndex (25h -> 3h) (@efenex)
- #491 cli skip onboarding without tty (@honor2030)

Upstream-installer revert:
- #546 drop --next workaround now that iii-hq/iii#1660 shipped

1067/1067 tests pass across 95 files.
rohitg00 added a commit that referenced this pull request May 20, 2026
* ci: cross-platform matrix + paths-ignore + concurrency

1. **OS matrix** — Linux + Windows + macOS, both Node 20 + 22. 6 cells,
   ~3min each, ~18min wall time. Direct test against the class of
   bug #487 caught: hooks crashing on Windows usernames with spaces.
   Pre-merge Linux-only CI meant that bug landed in main + a release.
   fail-fast: false so a flake on one cell doesn't mask whether the
   same failure reproduces elsewhere.

2. **paths-ignore** — skip CI runs on README / CHANGELOG / docs /
   website / assets / .md / .mdx pushes. ~half the runner minutes
   back on doc-only churn. Source / config / workflow changes
   always run.

3. **concurrency + cancel-in-progress** — PR force-pushes cancel
   in-flight runs instead of piling them up. Push to main protected
   (concurrency group still scoped to ref, no cancel for main pushes).

Plus minor hardening: persist-credentials: false on the checkout
step so the GITHUB_TOKEN doesn't land in .git/config.

What was NOT lifted (rationale per plan):
- Per-package reusable workflows (Rust/Python/Homebrew — non-TS).
- License-header check (no per-file Apache banners in agentmemory).
- CLA bot (defer until external PR volume justifies friction).
- tsc --noEmit lint job (codebase has ~10 pre-existing type errors
  tsdown skips; gating CI on those would block every PR until
  fixed; tracked as separate cleanup).
- Smoke test (`agentmemory demo + livez`) — defer to its own PR
  with its own validation cycle.
- Codecov badge — defer until baseline is set.

* ci(windows): force bash shell so build script's POSIX idioms work

Windows runners default to cmd.exe for npm run scripts; the build
script uses POSIX patterns the build script's exit codes
(`cp ... 2>/dev/null || true`, `mkdir -p`) that cmd doesn't
parse. ubuntu + macos already use bash by default so this is
Windows-only behaviour change.

Alternative: rewrite the build script in Node. Bigger lift, not
minimal.

* ci(windows): point npm script-shell at git-bash before build

`shell: bash` on the step only sets the shell for the step's own
runner; `npm run` still spawns its inner script via npm's
`script-shell` config, which defaults to cmd.exe on Windows.

Configure npm to use Git-Bash (preinstalled on GitHub-hosted
Windows runners) so `npm run build` and `npm run test` execute
the build script the same way ubuntu + macos do.

Step is gated on `runner.os == 'Windows'` so it's a no-op on the
other matrix cells.

* ci: drop windows-latest from matrix (obsidian-export hardcoded POSIX paths)

Windows runners fail on test/obsidian-export.test.ts because the
test + src hardcode `/tmp/...` POSIX paths that don't resolve on
the D:\ drive Windows uses. Fixing it cleanly requires reworking
src/functions/obsidian-export.ts to use os.tmpdir() + path.join,
which is a separate scope.

Drop windows from the matrix for now. Ship ubuntu + macos coverage
(real darwin/linux divergence catch) and file a follow-up to make
obsidian-export cross-platform so Windows can be added back.

* test(fs-watcher): bump waits to 1500ms + describe retry for macos fsevents flake
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.

Hooks commands break on Windows when username contains spaces

2 participants