fix(memory): neutralize provider prompt delimiters - #64288
Conversation
|
CI follow-up: the only failing slice is I reproduced the same 2 failures on an untouched checkout at this PR's exact base, |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
This PR neutralizes provider prompt delimiters in the memory system.
Looks Good
- Well-scoped: 2 files, 99 additions.
No Issues Found
Reviewed by Hermes Agent
b6f9e33 to
3dada22
Compare
|
Refreshed onto current The branch rebased without conflicts. Current-head local verification passes 811 tests across the memory-provider, streaming scrubber, run-agent, vision-memory, Honcho, Supermemory, and model-validation suites; Ruff, diff check, public identity, overlap review, metadata scan, and gitleaks also pass. Replacement GitHub CI is running on the refreshed head. |
3dada22 to
acaa0e6
Compare
|
The first refreshed CI attempt stalled inside GitHub's The second rebase was conflict-free and the full seven-file validation still passes 811 tests. The existing-PR publish gate, Ruff, identity, metadata, diff, overlap, and gitleaks checks pass on the new head. Replacement CI is running now. |
|
Thanks for the focused hardening change. Current GitHub main still passes provider recall through The diff preserves the existing fence cleanup and limits escaping to the role/control delimiter allowlist, while its tests cover case, whitespace, attributes, provider fan-out, payload preservation, unrelated XML, and unchanged assistant-output sanitation. GitHub reports the branch mergeable with required checks passing. Automated hermes-sweeper review. |
acaa0e6 to
cd45b35
Compare
|
suggesting changes The PR correctly escapes the XML-like role tags covered by its new regular expression and preserves unrelated XML, but it does not establish its claimed prompt-delimiter invariant across Hermes's supported model backends. Common non-XML chat-template control tokens such as ChatML
Security evidence:
Uncertainty: No live inference endpoint was authorized or available, so model obedience to a surviving delimiter was not measured against a specific server/model pair.; The isolated inputs did not include the PR or issue discussion, so any narrower author intent not represented in the diff and commit metadata could not be evaluated.; The checkout lacks a pytest-capable environment, so the added pytest cases were reviewed statically rather than executed. Signed: GPT-5.6-sol-xhigh in Codex |
|
Rebased onto current Local validation on the rebased head passes 385 tests across the memory provider, streaming scrubber, run-agent, vision-memory, Honcho, and Supermemory suites. The focused fencing suite passes 18 tests; Ruff, compilation, diff check, the existing-PR publish gate, metadata scan, and gitleaks also pass. Replacement CI completed all substantive jobs except the known current-main baseline in |
cd45b35 to
48cee8d
Compare
|
Refreshed onto current The rebase was conflict-free and preserves the original two-file memory-boundary diff. Isolated local verification passes 181 full-file tests across the memory provider, streaming scrubber, vision-memory, Honcho, Supermemory, and Vercel sandbox suites, plus the isolated run-agent memory-context regression (182 total). Ruff, diff check, public identity, overlap review, metadata scan, gitleaks, and the existing-PR publish gate also pass. Replacement CI is running at https://github.com/NousResearch/hermes-agent/actions/runs/30514392610. OSV lockfile scanning has passed; affected-area detection is currently delayed in GitHub checkout with no new failure reported. |
48cee8d to
7ff7925
Compare
|
Addressed the review at the new exact-current-main head The model-facing memory boundary now neutralizes ChatML The focused 92-test memory suite is green after rebase. The expanded run had 308 passes; its one failure was the absent optional Anthropic SDK, and that exact test passed after installing the locked |
|
suggesting changes The change correctly neutralizes the tested role tags and common model-template control tokens at the external-memory boundary, but its tag vocabulary omits prompt delimiters that Hermes itself uses. In particular, recalled provider text containing or <tool_response> remains byte-for-byte unchanged when appended to the API-bound user message. The current implementation therefore does not establish the claimed invariant that provider-controlled prompt-structuring delimiters are rendered only as data.
Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
7d40570 to
4aa13d4
Compare
|
Addressed the delimiter-surface review in Rebased onto current |
|
suggesting changes The patch closes the current-main gap for standard XML-like, role, and model-template delimiters in external memory prefetch. A residual source-level bypass remains: the regex refuses any candidate containing a nested
Security evidence:
Not checked:
Signed: GPT-5.6-luna-max in Codex |
fix(memory): neutralize provider prompt delimiters
|
4aa13d4 to
d705948
Compare
|
@egilewski Addressed the malformed-opener finding in commit The regression runs through |
|
suggesting changes The PR hardens the live memory-prefetch flow by escaping XML, role, and common model-template delimiters before they enter the user API message. A legacy-session bypass remains: api_content sidecars written before this fix are replayed verbatim into historical messages after upgrade, so a provider-supplied role/control block can still reach the model on resumed turns.
Security evidence:
Not checked:
Signed: GPT-5.6-luna-max in Codex |
d705948 to
ac06e6a
Compare
|
Addressed in ac06e6a. Thanks for catching the persisted-sidecar path, @egilewski. Legacy Validation on current |
Summary
[INST], and<<SYS>>template syntaxsanitize_context()unchanged because it also scrubs assistant output, where literal markup examples are validRoot cause
MemoryManager.prefetch_all()merges backend recall as untrusted text. The singlebuild_memory_context_block()model boundary previously removed nested<memory-context>fences and a bounded XML vocabulary, but model-template control tokens could still cross into the prompt verbatim.The fix stays at that shared provider boundary, so it covers every memory backend without altering persisted data or legitimate assistant output.
Validation
anthropicextragit diff --check, public commit identity, metadata scan, gitleaks, and the Hermes publish gate passedFixes #10694