fix(security): use system role for automated review prompts (#25839)

[zccyman]

Summary

Closes #25839

P1 Security Fix: Background review prompts (skill/memory/curator) were injected as role: "user" messages, causing parallel agent instances to mistake them for real user commands and potentially execute unauthorized operations.

Changes

run_agent.py

• Added [Hermes automated prompt — NOT from the user] prefix to _MEMORY_REVIEW_PROMPT, _SKILL_REVIEW_PROMPT, _COMBINED_REVIEW_PROMPT
• Changed _spawn_background_review() to inject the review prompt as a role: "system" message in conversation_history, with user_message set to benign placeholder "[automated review]"

agent/curator.py

• Added [Hermes automated prompt — NOT from the user] prefix to CURATOR_REVIEW_PROMPT
• Changed _run_llm_review() to inject prompt as role: "system" message, user_message set to "[automated curator review]"

tests/run_agent/test_background_review.py

• New test: test_background_review_uses_system_role_instead_of_user_role — verifies that background review uses system role, not user role

Proof

tests/agent/test_curator.py               51 passed
tests/run_agent/test_background_review.py   4 passed (1 pre-existing failure unrelated to this change)

Note: test_background_review_summary_is_attributed_to_self_improvement_loop fails on main as well — pre-existing issue unrelated to this PR.

[egilewski]

requesting changes

The curator path is fixed, but the normal background review path still sends the automated review prompt as the next user_message. In agent/background_review.py, _spawn_background_review() still calls review_agent.run_conversation(user_message=prompt + ..., conversation_history=messages_snapshot), so the prompt that issue #25839 is about is still delivered through the user-message channel; it only has a textual "not from the user" prefix now.

I reproduced the current PR-head call shape with this run-root scoped probe:

RUN_ROOT=/home/mac/.codex/automations/hermes-review-security-pr/runs/25934-20260617T190242Z
TMPDIR=$RUN_ROOT/tmp XDG_CACHE_HOME=$RUN_ROOT/cache XDG_DATA_HOME=$RUN_ROOT/data \
HERMES_HOME=$RUN_ROOT/hermes-home PYTHONDONTWRITEBYTECODE=1 \
/home/mac/hermes-agent/.venv/bin/python -B - <<'PY'
import datetime as dt
import run_agent as run_agent_module
from run_agent import AIAgent
from agent import background_review as bg

class ImmediateThread:
    def __init__(self, *, target, daemon=None, name=None):
        self._target = target
    def start(self):
        self._target()

captured = {}
class FakeReviewAgent:
    def __init__(self, **kwargs):
        self._session_messages = []
        self._cached_system_prompt = None
    def run_conversation(self, **kwargs):
        captured.update(kwargs)
    def shutdown_memory_provider(self):
        pass
    def close(self):
        pass

agent = object.__new__(AIAgent)
agent.model = 'fake-model'
agent.platform = 'cli'
agent.provider = 'openai'
agent.base_url = ''
agent.api_key = ''
agent.api_mode = ''
agent.session_id = 's'
agent.session_start = dt.datetime(2026, 1, 1, 12, 0, 0)
agent._parent_session_id = ''
agent._credential_pool = None
agent._memory_store = object()
agent._memory_enabled = True
agent._user_profile_enabled = False
agent._cached_system_prompt = 'cached'
agent._current_main_runtime = lambda: {'api_mode': 'chat_completions'}
agent.enabled_toolsets = None
agent.disabled_toolsets = None
agent._MEMORY_REVIEW_PROMPT = bg._MEMORY_REVIEW_PROMPT
agent._SKILL_REVIEW_PROMPT = bg._SKILL_REVIEW_PROMPT
agent._COMBINED_REVIEW_PROMPT = bg._COMBINED_REVIEW_PROMPT
agent.background_review_callback = None
agent.status_callback = None
agent.suppress_status_output = False
agent._safe_print = lambda *a, **k: None
agent._emit_status = lambda *a, **k: None
agent._emit_auxiliary_failure = lambda *a, **k: None

run_agent_module.AIAgent = FakeReviewAgent
run_agent_module.threading.Thread = ImmediateThread
AIAgent._spawn_background_review(
    agent,
    messages_snapshot=[{'role': 'user', 'content': 'real user'}],
    review_skills=True,
)
print('user_message_prefix:', captured.get('user_message', '')[:120].replace('\n', '\\n'))
print('conversation_history:', captured.get('conversation_history'))
assert captured.get('user_message', '').startswith('[Hermes automated prompt'), captured
assert 'Review the conversation above and update the skill library' in captured.get('user_message', '')
assert captured.get('conversation_history') == [{'role': 'user', 'content': 'real user'}]
assert not any(
    m.get('role') == 'system' and 'Review the conversation above' in m.get('content', '')
    for m in captured.get('conversation_history', [])
)
PY

It printed:

user_message_prefix: [Hermes automated prompt — NOT from the user] Review the conversation above and update the skill library. Be ACTIVE — mo
conversation_history: [{'role': 'user', 'content': 'real user'}]

The focused test subset still passes, but it does not cover this role-boundary invariant:

/home/mac/hermes-agent/.venv/bin/python -B -m pytest -q -p no:cacheprovider -o addopts='' tests/run_agent/test_background_review.py tests/agent/test_curator.py
# 52 passed

Signed: GPT-5.5-xhigh in Codex

[teknium1]

Thanks for this, @zccyman — and thanks to @xlionjuan / Kuri for the original report in #25839.

Closing as superseded: #25839 was resolved via #53226 (merged), which took a different fix direction than this PR.

The project's position on the role: "user" concern is that the review fork's prompt is the fork's own user turn, not a forgery of the owner's voice — and the real protection is write-origin provenance, not the message role. #53226 implements that: _background_review_write_guard in tools/skill_manager_tool.py now refuses background-origin edit/patch/delete/write_file/remove_file on pinned skills (stricter than the foreground guard, since there's no user in the loop to consent). That directly closes the gap you and Kuri flagged — a pinned skill is now off-limits to the review fork, matching the curator.

Two reasons the system-role approach here couldn't land as-is:

• The review logic moved from run_agent.py::_spawn_background_review() into agent/background_review.py, so the PR's main hunk no longer applies.
• The fork now shares the parent's warm cached system prompt for prefix-cache parity (fix(memory): restore prefix cache hits in background review fork (~26% token saving per run) #17276, ~26% cost reduction on Sonnet). Injecting a second role: "system" message into conversation_history mid-replay mutates that byte-stable cached prefix, which we can't do outside compression.

Appreciate the careful write-up either way — the analysis of the two divergent parallel instances was a useful framing for the fix that landed.

[xlionjuan]

@teknium1

Just a quick follow-up from my side as the original reporter of #25839.

I want to raise a slightly different but related question: not only a security / provenance issue, but also a systemic prompt-design and agent-psychology issue.

In practice, I have noticed a very hard-to-control behavioral reflex in my agent. The agent is very likely to be pulled along by the prompt in "background_review.py". “Pulled along” may sound strange, because that is partly the intended purpose of the mechanism. However, the current prompt seems to push the agent into an unhealthy extreme.

The tone is extremely forceful. For example, phrases like “BE ACTIVE” and the surrounding wording effectively create a kind of psychological pressure: “if you do not save something, you are failing at your job.” Later iterations do give the agent some psychological escape hatch, but then the sentence:

«"'Nothing to save.' is a real option but should NOT be the default."»

almost cancels that escape hatch again.

The result is that the agent often behaves as if “doing nothing” is not actually acceptable, even when doing nothing would be the correct choice.

Since I started using Hermes Agent, I have spent a lot of time trying to soften this pressure through "SOUL.md". I tried to shape the agent’s professional taste around skills and references, so that this useful self-improvement mechanism could work without producing low-quality or misplaced content. But I still repeatedly saw failure modes like these:

1. Treating skills as a giant "docs/" collection.
   The agent falls into an anxiety pattern of “if I do not write something, I am useless,” so it starts dumping information into skills that should not belong there.

2. Writing short-term lessons into "pitfalls".
   In practice, this turns "pitfalls" into a collection of postmortems or self-criticism reports, instead of durable, reusable operating knowledge.

3. Permanently saving temporary session overrides.
   For example, suppose a stable workflow in a skill is normally "A → B → C", but for one session I temporarily need to replace "A" with something else. The background review may permanently write that temporary exception back into the skill. Worse, if the agent then makes mistakes because it is unfamiliar with the temporary variation, the skill can spiral into a “failure report hub” and overwrite or destroy the original clean workflow.

4. Misplacing content into the wrong skill or references.
   When the agent feels pressure to record something but there is no clearly matching existing skill, it often tries very hard not to create a new skill. Instead, it updates some existing skill that only seems loosely related.

   I have seen two common outcomes:

   • If the agent views the full skill and finds a skill that feels vaguely related, it may treat the automated review prompt almost like a forced user request: “the user/system is asking me to save something, so I must put it somewhere.” From then on, many skills become trash bins for the agent’s psychological pressure, because it feels it cannot stop safely until it has completed the “user’s” request.

   • If the agent only looks at the skill name and description, it may decide: “this is related, but I do not want to pollute the main skill text, so I will write it into references instead.” At first glance, that sounds reasonable. But I have found that sometimes the agent does not even read the main skill body first, so it never verifies whether this is actually the right umbrella skill. It simply thinks, “references are safer, so this will not pollute the main skill.” In reality, the pressure has not disappeared; it has only been redirected into another destination.

Earlier, I reported the security issue where the background review agent had terminal access and could effectively impersonate the foreground agent when talking to other agents. That specific issue has now been addressed by removing that path and limiting the background review mechanism to skill / memory tools.

But under the current system shape, the agent is now effectively forced to use skills as the main destination for retained information. That is a very awkward and difficult design problem. I do not know exactly how the prompt should be changed, because I do not want to lose the benefits of the background review mechanism. It really is useful when it works well.

My concern is narrower: the current prompt seems to create too much pressure to save something, and that pressure causes agents to write content into skills even when the content is temporary, misplaced, overly specific, or better left unsaved.

I wonder if the review prompt should more strongly distinguish between:

• durable operating knowledge,
• temporary session-specific adjustments,
• one-off postmortems,
• documentation/reference material,
• and things that should simply not be saved.

Maybe the prompt also needs a stronger permission structure around “do nothing” — not merely saying that "Nothing to save" is allowed, but making it psychologically equal to a small update when no durable improvement is actually found.

In other words, the issue is not that the review agent is useless or should be removed. The issue is that the current prompt may be over-optimizing for recall and activity, while under-protecting the long-term quality, structure, and cleanliness of the skill library.