Skip to content

fix(skills): key the bundle cache on its directory, not mtime alone - #328

Merged
OmarB97 merged 1 commit into
mainfrom
fix/skill-bundles-cache-key-fork-20260802
Aug 2, 2026
Merged

fix(skills): key the bundle cache on its directory, not mtime alone#328
OmarB97 merged 1 commit into
mainfrom
fix/skill-bundles-cache-key-fork-20260802

Conversation

@OmarB97

@OmarB97 OmarB97 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

get_skill_bundles() decided freshness by comparing the highest mtime across the
bundles directory and its files against the cached snapshot's. The directory
itself was not part of the key.

_bundles_dir() resolves <HERMES_HOME>/skill-bundles at call time, so in a
backend serving several profiles it points at a different home per request. mtime
does not identify a profile: when two homes' newest bundle files happen to share an
mtime, the freshness check saw no change and returned the snapshot built from the
other profile's directory. /my-bundle from one profile then resolved — or
failed to resolve — against another profile's bundles.

Equal mtimes are not an exotic condition. Bundles are commonly seeded by the same
install, copy, or checkout, which is precisely when several profiles' directories
carry the same timestamp.

The fix adds the resolved directory to the key. A profile switch now always
rescans. That is the correct trade: a rescan is a glob plus a few small YAML
parses, and get_skill_bundles() already stat()s the directory and every bundle
file on each call, so the added cost on a switch is small and the cost on the
common path is zero.

This completes the command.dispatch profile-scoping audit alongside #323
(/undo), #324 (_finalize_session), and #326 (skill discovery).

Related Issue

No filed issue — found while auditing command.dispatch for profile scoping.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/skill_bundles.py — new module-level _bundles_cache_dir, set by
    scan_bundles() and compared in get_skill_bundles() alongside the existing
    mtime check. Comment records why mtime alone is not a sufficient key.
  • agent/skill_bundles.pyget_skill_bundles() docstring now states that a
    directory change forces a rescan, so the behaviour is discoverable at the call
    site rather than only in the diff.
  • tests/agent/test_skill_bundles.py — new TestCacheIsKeyedOnTheBundlesDir
    (3 tests).

Deliberately not reshaped: _bundles_cache and _bundles_cache_mtime keep
their existing names and types. Six test files across tests/agent,
tests/hermes_cli, tests/gateway, tests/cron, and tests/openviking_plugin
reset that pair between tests; adding a third global leaves every one of those
reset idioms working. A per-directory cache dict would avoid the rescan-on-switch
entirely, but it would change that shared shape for a saving this call site does
not need.

How to Test

  1. Confirm the new tests fail without the fix. Revert only
    agent/skill_bundles.py and run
    pytest tests/agent/test_skill_bundles.py -k TestCacheIsKeyedOnTheBundlesDir.
    Two of the three fail, showing one profile's bundles served to another:
    E  AssertionError: assert '/launch-bundle' in {'/profile-bundle': {...}}
    
    The third (..._same_dir_unchanged_still_hits_the_cache) passes either way by
    design — it guards against over-invalidating rather than reproducing the bug.
  2. Confirm they pass with it.
    pytest tests/agent/test_skill_bundles.py -q → 38 passed.
  3. Check for regressions across every bundle consumer — all six files that
    reset the cache globals, plus skill-command discovery:
    pytest tests/agent/test_skill_bundles.py tests/hermes_cli/test_bundles.py \
      tests/openviking_plugin/test_openviking.py \
      tests/cron/test_cron_prompt_injection_skill.py \
      tests/gateway/test_bundles_command.py tests/tui_gateway/test_protocol.py \
      tests/agent/test_skill_commands.py -q
    
    255 passed, 0 failed. Baseline on an unpatched tree: 252 passed, 0
    failed.
    The delta is exactly the three new tests; both runs are fully green,
    so there is no pre-existing noise to account for here.

The test forces byte-identical mtimes across both homes with os.utime — on dirs
and files alike, since _max_mtime() watches both. Without that the two homes
differ by ordinary filesystem timestamps and the bug hides.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — not claimed. The full suite is not green on this machine independent of this change. I ran the 7 bundle-consumer suites listed above, matched before and after: both fully green, 252 → 255.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.6.0), arm64

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A (get_skill_bundles() docstring updated; rationale comment added at the cache global)
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A (N/A — no config keys)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A (N/A — internal cache key)
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A (the key is str(Path), compared for equality only — no path semantics involved)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A (N/A — no tool surface change)

Screenshots / Logs

Reproduction with two homes forced to identical mtimes — the process scans under
launch, then a request scoped to profile asks again:

launch  first call        -> LAUNCH
profile under override    -> LAUNCH   ❌ POISONED (served launch data to a profile request)

With this change the second call returns PROFILE.

`get_skill_bundles()` decided freshness by comparing the highest mtime across
the bundles directory and its files against the cached snapshot's. The
directory itself was not part of the key.

`_bundles_dir()` resolves `<HERMES_HOME>/skill-bundles` at call time, so in a
backend serving several profiles it points at a different home per request.
mtime does not identify a profile: when two homes' newest bundle files happen
to share an mtime, the freshness check saw no change and returned the snapshot
built from the OTHER profile's directory. `/my-bundle` from one profile then
resolved — or failed to resolve — against another profile's bundles.

Equal mtimes are not exotic. Bundles are commonly seeded by the same install,
copy, or checkout, which is exactly when several profiles' directories carry
the same timestamp.

Add the resolved directory to the key. A profile switch now always rescans,
which is the correct trade: a rescan is a glob plus a few small YAML parses,
and `get_skill_bundles()` already stat()s on every call. The ordinary
single-home case still hits the cache — covered by a test that counts scans.

`_bundles_cache` and `_bundles_cache_mtime` keep their names and shapes; six
test files reset that pair between tests and are unaffected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@OmarB97
OmarB97 merged commit b95146b into main Aug 2, 2026
32 checks passed
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