Skip to content

fix(gateway): scope pairing platform discovery to the profile dir - #60564

Closed
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/gateway-pairing-profile-discovery
Closed

fix(gateway): scope pairing platform discovery to the profile dir#60564
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/gateway-pairing-profile-discovery

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

What does this PR do?

The per-profile pairing isolation introduced self._dir and scoped every per-file path helper to it (_pending_pathself._dir / f"{platform}-pending.json", _approved_pathself._dir / f"{platform}-approved.json"). But _all_platforms(suffix) — the helper that enumerates which platforms have data files — still iterates the module-global PAIRING_DIR:

def _all_platforms(self, suffix: str) -> list:
    platforms = []
    for f in PAIRING_DIR.iterdir():   # <-- global dir, not self._dir
        ...

For a PairingStore(profile="<name>"), self._dir is <HERMES_HOME>/profiles/<name>/pairing/, which is not PAIRING_DIR. So every caller that passes platform=Nonelist_approved, list_pending, clear_pending — enumerates the platform set from the global dir but then loads each platform's file from the profile dir. The result is a silent divergence between the authz surface and the list/inspect/clear surface: is_approved("telegram", uid) reads self._dir and returns True, while list_approved() scans the (empty, for that profile) global dir and returns [] for that same user. Operators inspecting or clearing a profile's whitelist see the wrong platform set.

The one-line fix routes discovery through self._dir. This is byte-identical for the global store, where self._dir == PAIRING_DIR (set in __init__ when no profile is given), so the existing hermes pairing CLI / dashboard path is unchanged; only the profile-scoped case is corrected. self._dir is guaranteed to exist because __init__ does self._dir.mkdir(parents=True, exist_ok=True).

Related Issue

Type of Change

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

Changes Made

  • gateway/pairing.py: _all_platforms now iterates self._dir instead of the module-global PAIRING_DIR, so platform discovery is scoped to the same directory the per-file path helpers already use.
  • tests/gateway/test_pairing.py: add TestProfileScopedDiscovery — a profile-scoped store approves a user, then asserts is_approved() and list_approved() agree.

How to Test

  1. Build a PairingStore(profile="alice") with PAIRING_DIR patched to a distinct empty directory (so the global dir provably isn't the profile dir).
  2. Approve a user, then check both surfaces: before the fix, is_approved("telegram", "tg-456") is True but list_approved() returns [] (it scanned the empty global dir). After the fix both agree.
  3. uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest tests/gateway/test_pairing.py -v → 53 passed. The new test fails before the one-line change (assert [] == ['tg-456']) and passes after.

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
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

The per-profile pairing isolation added self._dir and scoped every
per-file path helper (_pending_path, _approved_path) to it, but
_all_platforms still enumerated the module-global PAIRING_DIR. For a
profile-scoped PairingStore, list_approved/list_pending/clear_pending
therefore operated on the GLOBAL platform set while loading each
platform's file from the PROFILE dir — so list_approved() returned []
for a user that is_approved() confirmed as approved, a silent divergence
between the authz surface and the list/inspect/clear surface.

Route discovery through self._dir. Byte-identical for the global store
(self._dir == PAIRING_DIR when no profile is set); only the buggy
profile-scoped case changes. self._dir is guaranteed to exist (__init__
mkdirs it).
Copilot AI review requested due to automatic review settings July 7, 2026 23:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a regression in the gateway pairing system where profile-scoped PairingStore instances discovered platforms from the global pairing directory, causing list_approved()/list_pending()/clear_pending() to disagree with is_approved() for the same profile.

Changes:

  • Update _all_platforms() to iterate self._dir (profile-scoped directory) instead of the module-global PAIRING_DIR.
  • Add a regression test asserting is_approved() and list_approved() agree for a profile-scoped store when the global pairing dir is distinct/empty.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
gateway/pairing.py Scope platform discovery to the store’s resolved directory (self._dir) to keep listing/clearing consistent with approval checks under profiles.
tests/gateway/test_pairing.py Adds a profile-scoped regression test for platform discovery behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +84 to +86
with patch("gateway.pairing.PAIRING_DIR", global_dir), patch(
"gateway.pairing.get_hermes_home", return_value=home
):
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state P2 Medium — degraded but workaround exists labels Jul 8, 2026
@briandevans

Copy link
Copy Markdown
Contributor Author

@copilot Addressed in 2708fa8 — you're exactly right. PairingStore.__init__ resolves the profile dir via a function-local from hermes_constants import get_hermes_home inside the if profile: branch, so the module-global patch on gateway.pairing.get_hermes_home never reached it. I confirmed the old target left self._dir rooted at the real ~/.hermes/profiles/alice/pairing (test passed only because is_approved/list_approved both read the same real dir — a profile-safety leak, not the intended path).

Fix: patch hermes_constants.get_hermes_home (the source module the local re-import resolves against), and I added assert store._dir == home / 'profiles' / 'alice' / 'pairing' so the test now provably fails if the mock target ever regresses. Verified fail-before/pass-after against both patch targets.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still has the mismatch: PairingStore(profile=...) writes and reads individual pairing files through self._dir (gateway/pairing.py:251-282), while unfiltered list_approved, list_pending, and clear_pending all use _all_platforms() (gateway/pairing.py:354-362, 539-581), which currently iterates PAIRING_DIR at gateway/pairing.py:656.

The one-line change makes enumeration use the same resolved directory as the per-file helpers. The added regression test also correctly patches hermes_constants.get_hermes_home, matching the function-local import in PairingStore.__init__ (gateway/pairing.py:255-256), and proves the profile directory differs from the global directory.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/profiles Multi-profile isolation, HERMES_HOME scoping labels Jul 15, 2026
@OutThisLife

Copy link
Copy Markdown
Collaborator

Good catch — _all_platforms scanning the module-global PAIRING_DIR while every path helper used self._dir meant a profile store could confirm a user via is_approved() and still return [] from list_approved(). Cherry-picked into #74446 with authorship preserved, alongside #70932.

One update to your test: it pinned profiles/<name>/pairing, and #70932 moves profile stores to the consolidated platforms/pairing layout. Also repointed the patch at get_default_hermes_root rather than get_hermes_home — a profile anchors to the hermes root, and that distinction is precisely the bug, so the test now fails if the resolver regresses to the current home.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/profiles Multi-profile isolation, HERMES_HOME scoping comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants