Skip to content

fix(memory): install profile secret scope in hindsight background threads - #93028

Closed
Parker-Fawcett wants to merge 2 commits into
NousResearch:mainfrom
Parker-Fawcett:fix/92608-hindsight-bg-secret-scope
Closed

Parker-Fawcett wants to merge 2 commits into
NousResearch:mainfrom
Parker-Fawcett:fix/92608-hindsight-bg-secret-scope

Conversation

@Parker-Fawcett

Copy link
Copy Markdown

Fixes #92608

What & why

Under gateway multiplexing, get_secret fails closed on unscoped reads rather
than risk returning another profile's credential (agent/secret_scope.py).
Hindsight's background threads — the retain writer loop, the embedded
daemon-start thread, and the prefetch thread — are spawned raw with no
contextvars propagation, so _get_client()'s
get_secret("HINDSIGHT_LLM_API_KEY") raised UnscopedSecretError every time:
the local_embedded daemon could never boot and every retain failed, exactly
as reported. Same root-cause family as #76574 / #86402.

The fix mirrors the established contract in gateway/run.py (and the
web-server flow runner): capture the profile HERMES_HOME once at provider
construction — which always runs inside the adapter's per-profile context —
and have each background body re-install both overrides through a
_profile_scope() context manager:

home_token = set_hermes_home_override(str(self._profile_home))
secret_token = set_secret_scope(build_profile_secret_scope(self._profile_home))

Design notes:

  • The writer loop wraps per job, not the whole loop — a retain that saves
    an updated .env mid-session is visible to later jobs, and the sentinel
    exit path is untouched.
  • The module-level _build_embedded_profile_env read (a third unreported
    unscooped site) runs inside the daemon-start body, so it is covered.
  • Non-multiplex deployments are unaffected: the scope is installed either way,
    but with multiplex off get_secret treats it as a .env overlay over
    os.environ, preserving legacy behavior.

How to test

New tests in tests/plugins/memory/test_hindsight_provider.py::TestBackgroundSecretScope, all daemon-free:

  • writer job reads HINDSIGHT_LLM_API_KEY from the captured profile .env
    and the scope is reset after the thread body
  • a writer job observes get_hermes_home() == the captured home even when the
    process env points elsewhere
  • under _MULTIPLEX_ACTIVE=True, an unscoped read still raises
    UnscopedSecretError (guard intact)
  • the reported crash shape: multiplex on + key only in profile .env + raw
    writer thread → job succeeds inside the scope

Suite: full tests/plugins/memory/ via scripts/run_tests.sh → 338 passed;
the 6 test_hindsight_provider.py and 1 test_openviking_provider.py
failures are pre-existing on clean main (verified by stashing this change —
identical failure sets). macOS 26, Python 3.11.

…eads

Under gateway multiplexing, get_secret fails closed on unscoped reads
(rather than risk returning another profile's credential). hindsight's
writer, daemon-start and prefetch threads are spawned raw — no contextvars
propagation — so local_embedded could never boot its daemon: _get_client's
get_secret('HINDSIGHT_LLM_API_KEY') raised UnscopedSecretError on every
start and retain.

Capture the profile HERMES_HOME at construction (always scoped) and wrap
each background body in a _profile_scope context manager that re-installs
set_secret_scope(build_profile_secret_scope(home)) plus the home override —
the same contract gateway/run.py applies to its own worker threads.
Per-job wrapping in the writer loop keeps .env edits visible to later
retains; sentinel exit is unaffected.

Closes NousResearch#92608.
@Parker-Fawcett
Parker-Fawcett force-pushed the fix/92608-hindsight-bg-secret-scope branch from a0f2e30 to 20becd4 Compare August 23, 2026 15:58
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P3 Low — cosmetic, nice to have labels Aug 23, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Related to #81816: both repair Hindsight background profile-secret scoping, but this PR scopes writer jobs, daemon startup, and prefetch bodies directly while #81816 guards client creation.

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

Correct fix shape for #92608: capturing profile identity at construction and having every raw thread re-enter it mirrors the gateway's thread-wrapping contract, and the token-based install/reset pair handles nesting safely.

Findings:

  1. plugins/memory/hindsight/__init__.py:1497-1498 (_profile_scope) — build_profile_secret_scope(self._profile_home) executes on every writer job and every prefetch recall. If scope construction re-reads/parses the profile .env each time, busy retain queues now pay repeated filesystem IO per job. Since _profile_home is already frozen at construction (plugins/memory/hindsight/__init__.py:759), consider building the scope object once alongside it and installing the cached instance — unless live .env reload is intentional, in which case say so in the docstring.

  2. plugins/memory/hindsight/__init__.py:759 — the capture silently assumes the constructor always runs inside the adapter's per-profile context. A provider constructed outside that context (direct tool usage, some test setups) bakes in the process-default home with no signal. A debug log when multiplexing is active and the captured home equals the process default would make such misconstructions diagnosable.

  3. Coverage imbalance: the writer loop gets four solid tests, but the daemon-start wrapper (plugins/memory/hindsight/__init__.py:1815-1818) and the prefetch body (:2006) are untested. Add at least one regression test per path — e.g. observe current_secret_scope()/get_hermes_home() from inside the daemon's first iteration — since those are equally load-bearing under multiplex.

  4. tests/plugins/memory/test_hindsight_provider.py:1577-1584 — the _bare_provider helper is used exactly once; the tests at :1614, :1640, and :1650 rebuild the same four-line bare provider inline. Reuse the helper everywhere. Also, test_unscoped_read_fails_closed_under_multiplex (:1630-1635) monkeypatches the private _MULTIPLEX_ACTIVE; if agent.secret_scope exposes any public activation hook, prefer it so an internal rename doesn't turn the test into a false negative.

Minor: reset order in the finally correctly mirrors installation; no scope-leak path spotted across the three wrapped sites.

Review findings on NousResearch#93028:

1. Cache the built profile scope at construction instead of re-parsing the
   profile .env on every writer job / prefetch recall; docstring documents
   the snapshot lifecycle trade-off.
2. Debug-log a misconstruction signal: multiplex active with no
   HERMES_HOME override at construction means the captured home is likely
   the process default.
3. Extract _spawn_embedded_daemon/_daemon_start_body and
   _prefetch_background from their closures so both wrapper paths are
   directly testable; add per-path regressions observing
   current_secret_scope()/get_hermes_home() inside each body.
4. Reuse _bare_provider across all scope tests; switch the multiplex
   activation to the public set_multiplex_active hook.
@Parker-Fawcett

Copy link
Copy Markdown
Author

All four findings addressed in 388686e:

  1. Per-job scope construction — _profile_scope now installs a mapping built once at construction (self.\_profile\_secret\_scope), so busy retain queues pay zero per-job .env IO. The docstring documents the snapshot trade-off explicitly: the scope tracks the provider instance's lifecycle; mid-session .env edits are picked up on provider recreation, and the daemon-restart path reads config changes separately via \_build/\_materialize\_embedded\_profile\_env.

  2. Misconstruction diagnosability — the constructor now emits logger.debug(...) when multiplexing is active but no HERMES_HOME override was active at capture time (i.e. \_profile\_home is probably the process default).

  3. Per-path regressions — the two closures weren't reachable from tests, so I extracted them into methods (behavior-identical: \_spawn\_embedded\_daemon/\_daemon\_start\_body and \_prefetch\_background; spawn sites unchanged). New tests drive each body directly with hindsight_embed/rich stubbed into sys.modules and observe current\_secret\_scope() + get\_hermes\_home() from inside — daemon path also proves the profile key resolves under multiplex (multiplex\_on fixture) and that scope resets despite log/env artifacts left behind.

  4. Test hygiene — \_bare\_provider is now the single construction path for all six tests in the class; the multiplex activation switched to the public set\_multplex\_active() hook via an auto-restoring fixture.

Suite: tests/plugins/memory/test\_hindsight\_provider.py → 77 passed, 6 failed — the 6 are the pre-existing baseline set, verified identical with this change stashed.

@teknium1

teknium1 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Thanks @Parker-Fawcett for the work here. Merged via #101255 (bd81bf0) on current main.

Your mechanism — installing the profile secret scope in hindsight background threads — is what landed; it was carried in the combined #101255 diff rather than this PR. You are credited via Co-authored-by on the merged commit and in the PR body.

Closing this PR as superseded.

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

Labels

area/memory Memory subsystem: store, providers, sync, background reviews area/profiles Multi-profile isolation, HERMES_HOME scoping comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

4 participants