Follow-ups from the PR #2394 review: HERMES_HOME shared-file key drift, Windows default, and hardening

[dyoshikawa]

Background

PR #2394 resolved #2389 by treating HERMES_HOME as the global Hermes profile root across MCP, hooks, permissions, skills, commands, subagents, and conversion. It also lifted resolveToolOutputRoot out of kimi-code.ts into a neutral src/utils/tool-output-root.ts, so the two tools with a home override now share one resolver.

The PR was reviewed and merged: all four of the maintainer's earlier inline points were resolved and verified, CI was green, the full e2e suite passed (794 tests, up from 789), and no tests were weakened or disabled. The findings below were all mid or low severity, so they did not block the merge — this issue is where they go instead.

Details

Correctness / behavior

1. (mid) Windows default profile root disagrees with upstream Hermes.
src/utils/hermesagent.ts:6-9, docs/reference/supported-tools.md:67-68

getHermesagentHome() reads only HERMES_HOME; the unset fallback is $HOME + .hermes on every platform. Upstream _get_platform_default_hermes_home() in hermes_constants.py returns %LOCALAPPDATA%\hermes on win32 and ~/.hermes only elsewhere. So on native Windows with HERMES_HOME unset, rulesync writes global output to %USERPROFILE%\.hermes\, which Hermes never reads.

The behavior predates the PR, but the PR is what added the sentence "When it is unset, the default remains ~/.hermes." — which is false on Windows. Mitigated in practice because the Windows installer sets HERMES_HOME.

2. (mid) Registry-derived shared-file key becomes env-dependent and diverges from SHARED_CONFIG_OWNERSHIP.
src/utils/hermesagent.ts:21-36, consumed via getSettablePaths in hermesagent-mcp.ts:333, hermesagent-hooks.ts:260, hermesagent-permissions.ts:181

With HERMES_HOME set at module-load time, getSettablePaths({ global: true }) returns relativeDirPath: "", so sharedFileKey() (shared-file-derive.ts:63-67) produces the bare config.yaml. Writes still use the hardcoded HERMES_CONFIG_SHARED_FILE_KEY = ".hermes/config.yaml" (shared-config-gateway.ts:254), so the documented invariant at shared-config-gateway.ts:288 ("keys are dir/file tokens matching deriveSharedFileWriters()") no longer holds.

No production impact today — the write path passes the constant, so applySharedConfigPatch never throws, and the e2e proves generation works. The cost is that the drift-detection guards go false-positive in exactly the configuration this feature is about. Reproduced with HERMES_HOME set:

• shared-config-gateway.test.ts — "accounts for every registry-derived shared file with an ownership declaration" (reports unaccounted config.yaml)
• shared-config-gateway.test.ts — "declares exactly the writer features..." (subagents drops off .hermes/config.yaml)
• shared-file-contract.test.ts — "no generation step deletes another step's key paths (global scope)"

7. (low) convert and generate now disagree on checks.
src/lib/convert.ts:430 vs src/lib/generate.ts:1169

buildChecksStrategy was updated for symmetry during review, but generateChecksCore still passes outputRoot verbatim. Harmless today — hermesagent checks is supportsGlobal: false (checks-processor.ts:78) and amp is the only global checks tool, with no home override — but the two entry points have drifted. Leave generateIgnoreCore (generate.ts:770) alone; it has an explicit comment justifying verbatim pass-through.

8. (low) HERMES_HOME silently overrides an explicit --output-roots.
src/utils/tool-output-root.ts:11-21

Under --global with HERMES_HOME set, resolveToolOutputRoot discards a user-specified --output-roots hermesagent=.... Identical to existing kimi-code behavior, so not a regression, but the precedence is documented nowhere.

Hardening

9. (low) Env-var-derived outputRoot bypasses validateOutputRoot.
src/utils/tool-output-root.ts:11-21, src/utils/hermesagent.ts:6-9

CLI/config outputRoot is validated at src/config/config-resolver.ts:419,429 (rejects empty, unnormalized absolute, filesystem root), but resolveToolOutputRoot substitutes HERMES_HOME afterwards using only resolve(), skipping that validation — HERMES_HOME=/ or ../.. becomes the output root verbatim. The same hole already exists for KIMI_CODE_HOME on main. Calling validateOutputRoot() on the resolved value inside resolveToolOutputRoot closes both at once.

10. (low) De-prefix guard is weaker than checkPathTraversal, and fails open at the call site.
src/utils/hermesagent.ts:30-35

The guard rejects only an exact .., a .. + separator prefix, and absolute paths; the codebase standard checkPathTraversal (src/utils/file.ts:207-225) rejects any .. segment. Inputs are compile-time constants today, so there is no exploitable path. But src/lib/shared-file-derive.ts:93-118 swallows exceptions from getSettablePaths / getExtraSharedWritePaths at module load (SHARED_WRITE_STEPS, src/lib/generate.ts:339), so if a Hermes global path ever moves outside .hermes, config.yaml silently drops out of the shared-write ordering graph and features would overwrite each other. Fragile coupling rather than a current vulnerability.

Note the structural divergence this leaves behind: hermesagent strips the .hermes prefix while kimi-code prefixes instead. De-prefixing the hermesagent constants to match kimi-code would make the class of bug impossible rather than merely asserted against.

Code quality

3. (mid) The same path-comparison expression is copy-pasted five times.
src/features/commands/hermesagent-command.ts:178-240

In HermesagentCommandAuxiliaryFile, this six-line block repeats five times (three of them character-for-character identical):

this.getRelativePathFromCwd() ===
  toPosixPath(getHermesagentRelativeFilePath({ global: this.global, relativeFilePath: <CONST> }))

across shouldMergeExistingFileContent, setFileContent, and three branches of getFileContent. On main each was a single line. A private matchesPath(relativeFilePath: string): boolean helper collapses 40+ lines to a handful and structurally prevents passing the wrong constant.

4. (low) Duplication against the kimi-code utility module.
src/utils/hermesagent.ts:55-60 vs src/utils/kimi-code.ts:34-41

getHermesagentRulesyncOutputRoot and getKimiCodeRulesyncOutputRoot are identical apart from which home getter they call. Now that src/utils/tool-output-root.ts exists, a shared getToolRulesyncOutputRoot({ nativeOutputRoot, global, toolHome }) belongs there. There is local duplication too: getHermesagentRelativeDirPath({ global, relativeDirPath: HERMESAGENT_RULESYNC_COMMANDS_PLUGIN_DIR_PATH }) is spelled out three times in hermesagent-command.ts:336-364 and twice in hermesagent-subagent.ts:225-241, where one local const would do.

Tests

5. (low) Unit tests are not hermetic with respect to HERMES_HOME.

Running the unit suite with HERMES_HOME set produces 12 failures: hermesagent-mcp.test.ts (6), hermesagent-skill.test.ts (1), hermesagent-subagent.test.ts (1), plus the 4 from finding 2. CI is unaffected, but CLAUDE.md tells developers to run pnpm cicheck locally, and the developers most likely to have HERMES_HOME exported are exactly the Hermes users this feature targets. Precedent exists — main has the same problem for KIMI_CODE_HOME — but at 2 failures, not 12.

6. (low) No unit-level coverage of the new global + HERMES_HOME behavior on the per-tool classes.

New tests are src/utils/hermesagent.test.ts (helper-level only) plus e2e. None of HermesagentMcp / HermesagentSkill / HermesagentSubagent / HermesagentHooks / HermesagentPermissions has a getSettablePaths({ global: true }) case with HERMES_HOME set.

Release notes

11. (informational) Users with HERMES_HOME set will have stale files left at ~/.hermes/* after upgrading, since --delete no longer reaches them. Worth documenting alongside the fact that --global --delete orphan removal now operates under $HERMES_HOME.

Solution / Next Steps

Roughly in the order I would take them:

1. Fix the docs sentence for finding 1 (one line), and decide separately whether to implement the platform default for Windows — that part is a real behavior change and could be its own issue.
2. Settle finding 2 deliberately: either make HERMES_CONFIG_SHARED_FILE_KEY derive from the same registry path the guards read, or teach sharedFileKey() to normalize an empty relativeDirPath. The point is that the drift guards should stay meaningful when HERMES_HOME is set.
3. Apply the matchesPath helper refactor (finding 3) and the shared getToolRulesyncOutputRoot extraction (finding 4).
4. Add validateOutputRoot() inside resolveToolOutputRoot (finding 9) — this also closes the pre-existing kimi-code hole. Consider tightening the de-prefix guard to checkPathTraversal semantics at the same time (finding 10).
5. Clear HERMES_HOME in the affected specs' beforeEach (finding 5) and add the per-class getSettablePaths({ global: true }) cases (finding 6). Doing 6 first makes 5 unavoidable.
6. Align generateChecksCore with buildChecksStrategy (finding 7), and document the HERMES_HOME vs --output-roots precedence (finding 8).
7. Add the migration note from finding 11 to the next release.

[dyoshikawa]

Processed in the 2026-07-28 scrap-issue batch: this is by design the parking list for the non-blocking #2394 review findings, so it stays open as the tracking record. Two notes from this pass:

• The two mids are both structural rather than spot fixes. Finding 1 (Windows default profile root) needs the same treatment Zed got in fix(zed): translate MCP servers, map the global default and write_file, use the Windows config dir #2450: a platform-branched relative dir plus BOTH spellings declared to the shared-write derivation and the gateway ownership table — but across every Hermes writer of config.yaml, so it is bigger than the Zed case. Finding 2's clean fix is the structural one the issue itself names: de-prefix the hermesagent constants to match kimi-code so getSettablePaths stops being HERMES_HOME-dependent at module load; anything short of that keeps the drift-guard false positives.
• Given both mids want the same restructure (env-independent settable paths + resolver-level home handling), doing findings 1, 2, 9 and 10 as one refactor PR — with the hermeticity fix (5) falling out of it — looks like the efficient cut; 3/4/7/8 are separable cleanups.

[dyoshikawa]

Findings 1-10 are resolved across two merged PRs, split as you asked: 1+2+9+10 as one refactor with 5 falling out of it, and 3/4/7/8 separately (6 rode along with the second, since it is test-only).

#2483 — findings 1, 2, 5, 9, 10

• 1 (Windows default). getHermesagentRelativeDirPath now strips the canonical .hermes prefix and re-anchors it: nothing is prepended when HERMES_HOME is set, otherwise the platform default (%LOCALAPPDATA%\hermes on win32, ~/.hermes elsewhere) takes its place. Keeping the path constants canonical meant one funnel rather than de-prefixing every constant. The false docs sentence is corrected, and the win32 branch is exercised by the windows-latest E2E leg because the specs now derive the directory instead of hardcoding .hermes.
• 2 (shared-file key drift). Rather than make the writers' key env-dependent, all five Hermes writers of config.yaml declare every spelling the profile root can take through getExtraSharedWritePaths, and the ownership table declares all three from one hoisted declaration. Writers pass the key of the file they are actually writing (getHermesagentConfigSharedFileKey), matching the sharedConfigFileKey convention the Zed and Codex adapters already use. Verified: 0 mismatches and 0 unaccounted keys with HERMES_HOME set, unset, and with a simulated win32 platform.
• 9. resolveToolOutputRoot validates a substituted home override and re-throws naming the variable, since the user never passed --output-roots.
• 10. The de-prefix guard uses checkPathTraversal semantics on both the input and the de-prefixed result — checking both matters because relative() normalizes .. away, so a path that walks out of .hermes and back in would pass a check on the result alone.
• 5. HERMES_HOME/KIMI_CODE_HOME are cleared in vitest.config.ts (and the e2e config) before any module loads; a beforeEach is too late because the shared-write derivation runs at module load. HERMES_HOME=/tmp/x KIMI_CODE_HOME=/tmp/y npx vitest run is green where it produced 12 failures before.

While fixing 2 it turned out KIMI_CODE_HOME broke both drift guards the same way, so kimi-code got the identical two-spelling treatment in the same PR — otherwise the env clearing would have masked a live bug rather than a fixed one.

#2484 — findings 3, 4, 6, 7, 8

• 3. A private matchesPath helper replaces the five repeats in HermesagentCommandAuxiliaryFile.
• 4. Both get*RulesyncOutputRoot functions delegate to a shared getToolRulesyncOutputRoot parameterized by the home getter. It lives in a new leaf src/utils/tool-home.ts rather than tool-output-root.ts, which would have formed a hermesagent → tool-output-root → hermesagent cycle. The repeated getHermesagentRelativeDirPath(...) expressions in the command and subagent files are one local const each now.
• 6. All five global Hermes classes have getSettablePaths({ global: true }) cases with HERMES_HOME both set and unset, pinning the platform literals directly.
• 7. generateChecksCore resolves the tool output root like buildChecksStrategy does. It is a no-op today — the global checks targets are amp and takt, neither of which has a home override — so this is pure symmetry. generateIgnoreCore is left alone, with a note recording that the override tools are not global ignore targets.
• 8. docs/reference/cli-commands.md documents that HERMES_HOME/KIMI_CODE_HOME override outputRoots and --output-roots in global scope for both generate and convert, and that an unusable value is rejected naming the variable.

Remaining

Finding 11 (informational) is a release-notes item, not a code change: users with HERMES_HOME set will have stale files left at ~/.hermes/* after upgrading, since --delete no longer reaches them, and --global --delete orphan removal now operates under $HERMES_HOME. Leaving this issue open for that.