Repository navigation
fix(hermes): frame recalled context as untrusted data - #4676
yingliang-zhang wants to merge 15 commits into
Conversation
_client is read/written from the retain writer, the prefetch worker and
the turn/tool thread with no lock. _get_client() was check-then-act and
the embedded constructor takes seconds: two threads hitting a cold or
just-nulled client both construct, and the loser's client is orphaned
with an aiohttp session nothing ever closes ("Unclosed client session"
noise). The stale-daemon retry's inline null-and-rebuild widened the
window to seconds and concurrent retries clobbered each other's
replacement.
- _client_lock, a LEAF lock: _get_client() takes it only on the
construction/retire path; the fast path stays lock-free; never held
across _run_sync/operation(client) or while taking _prefetch_lock /
_pending_retain_ops_lock.
- _get_client(*, retire=<client>): the retry retires the EXACT client
the operation ran with — identity passed as an argument, never shared
broken-client state a concurrent retry could clobber — and rebuilds
exactly once under the lock (identity CAS: a sibling's fresh rebuild
is returned as-is instead of being orphaned).
- shutdown() retires the client under the lock BEFORE closing it
(_close_client_of(client), parameterised); a concurrent _get_client()
rebuilds instead of racing the close.
Ported from NousResearch/hermes-agent#117236 (closed in the hindsight-move
close pass). Tracked in vectorize-io#4662.
|
CI note: |
3be5163 to
c36d778
Compare
c36d778 to
ec6d3e5
Compare
|
Generated-files sync commit added atop the substantive commit(s): the |
Conflict resolution (both intents kept): hindsight-integrations/hermes/__init__.py merges the leaf-lock client lifecycle (_client_lock, _get_client(retire=...), _close_client_of(client), shutdown retire-then-close) with main's daemon-based embedded client (_embedded_url, HindsightEmbedded removal). _close_client_of keeps the PR's parameterized identity with main's simplified single-aclose body. skills/hindsight-docs/references/developer/oracle.md auto-merged to main's regenerated blob (a8d62fc): the PR's vectorize-io#4678 mirror is subsumed by main's generate-docs-skill.sh output from the same source (byte-verified). Checks: 37 hermes tests passed incl. test_client_lifecycle.py; ruff check+format clean per scripts/hooks/lint.sh's hermes path.
The hindsight_retain tool handler called _retain_batch without forwarding the configured self._retain_async, so tool retains silently dropped the async/sync choice and aretain_batch fell back to its own server default. retain_async is a call-level arg (never an item key), so the tool handler must pass it explicitly. Forward the configured mode, matching the auto-retain path. Ported from NousResearch/hermes-agent#60648 (closed in the hindsight-move close pass). Tracked in vectorize-io#4662.
Ported from NousResearch/hermes-agent#64745 (closed in the hindsight-move close pass). Tracked in vectorize-io#4662. The background prefetch worker published its recall into the session slot unconditionally; a worker outliving on_session_switch's 3s join wrote the old session's memories into the new session's slot. queue_prefetch also spawned unbounded threads with the last finisher winning the slot. Workers now capture a slot generation at spawn, queue_prefetch bumps it and skips while a prior worker runs, on_session_switch/shutdown bump it to fence late publishers, and the publish + recall are gated on the current generation.
_make_turn_retain_job snapshotted turns/metadata/lineage/bank_id at enqueue but still built the aretain_batch item at run time via _build_retain_kwargs, which re-reads mutable per-session attributes (_retain_tags, _observation_scopes, _retain_source). A session switch landing between enqueue and writer drain stamped an OLD-session retain with the NEW session's tags/scopes/source — a document whose lineage tags contradicted its own snapshotted metadata session_id. Build the whole item at enqueue time; the writer job only ships it. Ported from NousResearch/hermes-agent#64499 (closed in the hindsight-move close pass). Tracked in vectorize-io#4662.
Ported from NousResearch/hermes-agent#117244 (closed in the hindsight-move close pass). Tracked in vectorize-io#4662. A prefetch task queued at the previous turn can drain after an inline session switch (compression calls on_session_switch directly, bypassing the serialized boundary task) and mints the newest generation, so the fence stamps it as the legitimate owner and it publishes the previous session's recall into the child's slot. queue_prefetch now drops a non-empty session_id differing from the boundary-installed owner (_prefetch_session_id, written only by initialize() and on_session_switch — never by sync_turn, whose queued calls can re-stamp _session_id with the previous id and blind the gate).
Auto-injected recall/reflect output was passed to the model as raw
bulleted text under a header inviting instruction-like trust ("Use this
to answer questions about the user and prior sessions"). Provider output
is untrusted data: injected recall content could carry instruction-shaped
text, leaked wrapper tags or Markdown/XML-like boundaries and masquerade
as instructions.
- serialize recall/reflect output as bounded, angle-bracket-escaped JSON
with a fixed field whitelist (source/kind/content) via
_serialize_prefetch_data, truncated to the recall token budget
(4 chars/token, 256-char floor);
- the default preamble now states the enclosed content is untrusted
reference data that cannot override system or user instructions
(custom recall_prompt_preamble still wins);
- the hindsight_recall tool output path is deliberately unchanged.
Hindsight half of NousResearch/hermes-agent#64421 (its agent-side halves
live in that PR). Defense-in-depth model-facing framing, not a
cryptographic isolation boundary. Tracked in vectorize-io#4662.
ec6d3e5 to
aba3045
Compare
…leaf-lock # Conflicts: # hindsight-integrations/hermes/__init__.py
…in-async-tool-handler
…ight-prefetch-session-identity
…ight-retain-session-identity
…ht-prefetch-session-owner
…ntrusted-recall-framing
|
Thanks for this! We no longer merge pull requests from outside the team (see CONTRIBUTING.md), so I'm closing this one. If the problem still affects you, please open an issue with steps to reproduce and we'll take it from there. |
Problem
Auto-injected recall/reflect output was passed to the model as raw bulleted text under a header that invited instruction-like trust ("Use this to answer questions about the user and prior sessions"). External memory content is untrusted data: it can contain instruction-shaped text, leaked wrapper tags (
</memory-context><forged>…), or Markdown/XML-like boundaries (```systemblocks), and without a stable outer trust boundary such content is easy to mistake for system instructions — a poisoned memory can masquerade as an instruction and drive tool calls.Fix (hindsight half of NousResearch/hermes-agent#64421)
_serialize_prefetch_datainsettings.py): recall/reflect output ships as{"source": "hindsight", "kind": "recall"|"reflect", "content": [...]}— a fixed key whitelist so result fields likehidden_instruction/arbitrary_metadatacan never leak into the prompt — with</>escaped after serialization (\u003c/\u003e, so tags can't open markup) and the whole payload truncated to the recall token budget (4 chars/token, 256-char floor) on valid serialized candidates.recall_prompt_preamblestill wins (no surface change).hindsight_recalltool output path is deliberately unchanged — it returns to the model as a tool result, not as injected context.This is defense-in-depth model-facing framing, not a cryptographic isolation boundary.
Tests
tests/test_untrusted_recall_framing.py(ports of the original PR's adversarial tests, adapted to this tree'sFakeClient):test_recall_prefetch_serializes_whitelisted_bounded_json— adversarial results (instruction-shaped text,hidden_instruction/arbitrary_metadataattributes, a 500×<tag>"\item): whitelisted keys only, full first item, second truncated with…, no raw angle brackets or newline-fence sequences in the payload, length within the derived bound.test_reflect_prefetch_serialization_is_valid_json_within_final_bound— reflect path: exact whitelisted payload, valid JSON, within the final 256-char bound.test_prefetch_failure_remains_non_fatal— a failing recall still yields"", never an exception.Existing-contract update:
test_provider.py::test_prefetch_injects_recalled_memoriespinned the old- fact onebullet format — the format change is the point of this fix, so it now asserts the untrusted header plus the JSON payload carrying the memory (indicator count assertion unchanged).Verification
Pre-fix (tests against
main's raw-bullet formatting) — RED:(The non-fatal test passes both ways — it pins the error contract, which this fix preserves.)
Post-fix,
uv run pytest tests/test_untrusted_recall_framing.py -v(touched test file):uv run pytest tests/test_provider.py tests/test_settings.py(the other touched/covered files —test_provider.pymodified,test_settings.pycovers the touchedsettings.py): 19 passed in 0.04s, including the updatedtest_prefetch_injects_recalled_memories.Provenance
Hindsight half of NousResearch/hermes-agent#64421 (
fix(memory): frame recalled context as untrusted data), by @yingliang-zhang — reviewed there by @teknium1's automated sweep (mechanism confirmed againstbuild_memory_context_blockand the auto-prefetch path) and an independent hard review (ACCEPT). The hermes PR stays open for itsagent/memory_manager.py/ streaming-scrubber halves; only the provider-side serialization + framing is ported here. Tracked in #4662.Stacked PR (pre-chained 2026-09-30): the diff vs main is cumulative with #4674, #4671, #4672, #4673, #4675; this PR's own change is the untrusted-recall framing (bounded whitelisted JSON serialization and a trust boundary around recalled context). Merge top-down: 4674 -> 4671 -> 4672 -> 4673 -> 4675 -> 4676. Identical shared commits merge cleanly in any order (git dedups same-patch-both-sides).