fix(hermesagent): honor HERMES_HOME globally - #2394
Conversation
dyoshikawa
left a comment
dyoshikawa
left a comment
There was a problem hiding this comment.
Thanks for the thorough work here. I verified the premise of #2389 independently: on main, every global Hermes adapter hardcodes the .hermes prefix while hermes-project-plugin-activation.ts already resolves HERMES_HOME, so the inconsistency is real. Dropping your new e2e spec onto main makes it fail, and it passes on this branch, so it is a genuine regression test. No existing tests were modified or weakened (the test diff is two new files, +304 lines only), docs and skills/rulesync/ stay byte-identical, and the supported-tools tables are untouched as they should be. The path-resolution design is sound and the negative assertions in the e2e spec (~/.hermes/config.yaml must not exist, wrong-profile must not be imported) are exactly the right way to pin this down.
One blocker before this can go in. buildCommandsStrategy in src/lib/convert.ts still passes the raw outputRoot while all seven other strategies in that file wrap it in resolveToolOutputRoot. You fixed the same omission in generate.ts, but convert.ts was missed, and now that the .hermes prefix gets stripped when HERMES_HOME is set, that gap starts writing into $HOME directly instead of the profile root. Details inline. It needs the one-line fix plus a convert case in the e2e spec, since the current coverage is generate/import only, which is why this slipped through.
I would also drop the NODE_ENV: "e2e" change in vitest.e2e.config.ts — it disables a safety net for the whole suite and the new spec does not need it. Rest of the comments are minor and can be follow-ups.
CI is green across all five jobs including Windows, and I found nothing concerning from a security standpoint: no network calls, no new process spawning, no CI/workflow/dependency changes, and the only env vars read are HERMES_HOME and the test-only HOME_DIR.
Vitest sets NODE_ENV=test, which suppresses ConsoleLogger output inherited by spawned CLI processes. Define NODE_ENV=e2e for the E2E runner so command output matches real CLI behavior.
c63ce70 to
de0f70f
Compare
|
@dyoshikawa review again, please? |
|
@rudironsoni Thank you! |
…ize the shared-file key Four follow-ups from the dyoshikawa#2394 review (issue dyoshikawa#2439, findings 1, 2, 9, 10): - Upstream Hermes defaults its profile root to %LOCALAPPDATA%\hermes on win32 and ~/.hermes elsewhere, but rulesync wrote ~/.hermes on every platform when HERMES_HOME was unset, so a global generate on Windows landed where Hermes never reads. getHermesagentRelativeDirPath now re-anchors the de-prefixed path on the platform default, and the false docs sentence is corrected. - The shared-file key derived from getSettablePaths depended on HERMES_HOME (and now the platform), so the gateway drift guards went blind in exactly the configuration this feature targets. Every Hermes config.yaml writer now declares all three spellings through getExtraSharedWritePaths, and the ownership table declares them alongside .hermes/config.yaml. - resolveToolOutputRoot validates a substituted home override with validateOutputRoot, closing the same hole for KIMI_CODE_HOME. - The de-prefix guard uses checkPathTraversal semantics on both the input and the de-prefixed result, rejecting any .. segment rather than a bare prefix. The unit suite is now hermetic with respect to HERMES_HOME and KIMI_CODE_HOME (finding 5): both are cleared in vitest.config.ts before any module loads, since they change paths resolved at module load. E2E specs use the platform-aware directory instead of a hardcoded .hermes for global scope. Part of dyoshikawa#2439 (findings 1, 2, 5, 9, 10). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
HERMES_HOMEas the global Hermes profile root across MCP, hooks, permissions, skills, commands, subagents, and conversion.hermespaths unchanged and canonical RuleSync sources under$HOME/.rulesyncNODE_ENV=test, usingNODE_ENV=e2eonly for subprocesses that assert user-visible output.hermesand resolve the home-directory fallback lazilyTest plan
mise exec -- pnpm cicheckgit diff --checkCloses #2389