fix(gateway): refresh cached prompts when SOUL.md changes - #28078
fix(gateway): refresh cached prompts when SOUL.md changes#28078qindongliang wants to merge 3 commits into
Conversation
magnus919
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment — solves a real problem (stale SOUL.md in cached prompts) with a sensible two-layer approach. One correctness concern with the staleness check worth discussing.
What it does
Two-layer cache invalidation for SOUL.md changes:
- Gateway agent cache (
gateway/run.py): fingerprints SOUL.md by mtime_ns + size in the config signature, so cached AIAgent instances are rebuilt after edits - Persisted prompt staleness (
conversation_loop.py): when restoring a system prompt from the session DB, checks if the current SOUL.md content is embedded in it; if not, rebuilds the prompt
🔴 Correctness concern: substring staleness check
_stored_system_prompt_stale_for_soul uses current_soul not in stored_prompt — a substring match. This produces false negatives when the edited SOUL.md is a subset of the stored prompt:
# Old SOUL.md: "You are Jasper. Be helpful."
# Stored prompt: "You are Jasper. Be helpful. System config..."
# New SOUL.md: "Be helpful." (shortened edit)
# → current_soul "Be helpful." IS in stored_prompt → FALSE (not stale)!
This is a corner case but means a user who shortens their SOUL.md won't see the change take effect. Consider comparing the full text rather than using substring membership.
⚠️ Warnings
- mtime_ns filesystem portability:
_soul_md_cache_key()falls back tost_mtime * 1_000_000_000on systems without nanosecond precision, which loses sub-second resolution. Two edits within the same clock tick could produce the same fingerprint. This is unlikely in practice but worth documenting. on_session_startgating: wrapping the plugin hook inif not stored_prompt_stalechanges behavior — plugins that rely onon_session_startfiring for prompt rebuilds won't fire. The logic is correct (don't re-init session state on a stale-prompt rebuild), but this behavioral change isn't called out in the PR description.
✅ Looks Good
- Two-layer approach is correct: the config signature catches cache-busting at the agent-pool level, while the staleness check catches it at the per-session prompt level. Belt and suspenders.
- Error handling: broad
except Exceptionin the staleness check correctly degrades to "not stale" (conservative). - Test coverage: 2 new tests in
test_agent_cache.pyvalidate the fingerprint extraction and signature busting. The prompt stability tests intest_run_agent.pycover the staleness detection path. - Clean diff: well-structured, no unrelated changes.
Summary
Well-structured fix for a real gap. The substring staleness check is the only correctness concern — fixing it to compare the full SOUL.md text rather than using in would eliminate the false-negative window for shortened edits. Otherwise ready to go.
Thanks for the careful review. Good catch on the substring false negative for shortened SOUL.md content. I’ll update the staleness check to compare against the full SOUL.md content more precisely instead of relying on substring membership, and I’ll also call out the on_session_start behavior change in the PR description. |
|
@magnus919 Hi,Updated. The staleness check now compares the stored identity block against the current SOUL/default identity instead of using substring membership, and I added a regression test for the shortened-SOUL case. I also documented the on_session_start behavior change in the PR description. Could you please take another look when you have a chance? |
|
Nice work on the fixes @qindongliang — the exact comparison with A couple of things that will come up before this can merge: 1. "18210507492@126.com": "qindongliang",2. PR template — the PR body doesn't follow the project's .github/PULL_REQUEST_TEMPLATE.md — it uses different section headers ( Just a friendly heads-up from a fellow contributor — I'm not a maintainer, so a project maintainer will need to give final approval and merge. The substance of the change looks solid! |
|
Thanks for the heads-up! I added my commit email to AUTHOR_MAP and updated the PR body to follow the project template. |
|
Thanks for the careful fix. I verified the premise against current main, and this still addresses a real gateway cache bug. Current main loads SOUL.md into the stable identity slot at I did not find a blocking correctness issue in the PR's approach. The exact identity-block comparison also addresses the shortened-SOUL false negative raised earlier in the discussion, and the added tests cover changed, removed, and shortened SOUL.md cases. Salvage note for maintainers: GitHub reports Automated hermes-sweeper review. |
c092ce4 to
b1d6380
Compare
|
@teknium1 Thanks for the sweeper review and the salvage note. I rebased this PR onto current GitHub now reports the PR as
The full If this looks good to you, could you please help trigger the maintainer review/merge path when you have a chance? |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the careful two-layer implementation. The current branch still demonstrates why a persisted gateway prompt can retain an earlier SOUL.md, but the requested behavior now conflicts with an explicit cache contract.
Problems
agent/system_prompt.py:157-161says Hermes never re-renders the system prompt mid-session to preserve upstream cache warmth;website/docs/user-guide/profiles.md:148likewise says SOUL.md changes take effect cleanly in a new session. The proposed rebuild inagent/conversation_loop.pychanges that active-session contract.- Current main's restore path has since gained
_stored_prompt_matches_runtime()atagent/conversation_loop.py:331-344. Any approved rework must retain and compose with that runtime-identity validation rather than apply the older unconditional-restore hunk.
Suggested changes
- Please obtain maintainer direction on whether SOUL.md edits are an intentional exception to the prompt-cache/session-boundary invariant. If approved, rework against current main and cover the result in
tests/agent/test_system_prompt_restore.py.
Automated hermes-sweeper review.
| @@ -301,10 +350,18 @@ def _restore_or_build_system_prompt(agent, system_message, conversation_history) | |||
| ) | |||
There was a problem hiding this comment.
Rebuilding here changes the system prompt inside an existing conversation. Current main treats that prompt as byte-stable for the session (agent/system_prompt.py:157-161) and documents SOUL.md changes as cleanly taking effect in new sessions (website/docs/user-guide/profiles.md:148); this needs maintainer approval as an explicit cache-invalidation exception.
|
@teknium1 Thanks for flagging the session-boundary invariant and the newer runtime-identity validation. This PR intentionally proposes treating SOUL.md changes as an exception to the byte-stable system-prompt contract for an existing session. Could a maintainer confirm whether that behavior is desired? If it is approved, I'll rebase onto current main, compose the SOUL.md validation with |
What does this PR do?
Refresh cached gateway prompts when the active profile's
SOUL.mdchanges.Gateway sessions intentionally reuse cached
AIAgentinstances and persistedsystem_promptsnapshots to preserve prompt-cache hits. That is useful for normal turns, but it also means edits toSOUL.mdcan keep using stale identity/rules in existing sessions.This PR adds two cache invalidation layers:
SOUL.mdfilesystem fingerprint in the gateway agent config signature so cached agents rebuild afterSOUL.mdis created, edited, or removed.system_promptfrom the session DB, detect if it no longer matches the currentSOUL.mdor default identity and rebuild/persist a fresh prompt instead of reusing the stale snapshot.Behavior change:
on_session_startnow only fires when a brand-new session is created; rebuilding after a stored prompt is stale no longer runs that hook.Implementation note: Gateway agent cache invalidation uses
SOUL.mdfile metadata (mtime_nsplus size) as a cheap fingerprint; on very low-resolution filesystems, multiple same-size edits inside one timestamp tick may require another edit or restart to bust the cache.Related Issue
Complements #26789, which wires gateway sessions to honor
SOUL.mdinitially. This PR handles the follow-up case whereSOUL.mdchanges after a session already has a cached/persisted prompt.Type of Change
Changes Made
gateway/run.py: add the active profile'sSOUL.mdmetadata fingerprint to gateway agent cache-busting config.agent/conversation_loop.py: detect persisted system prompts whose stored identity block no longer matches the currentSOUL.mdor default identity before reusing them.scripts/release.py: add18210507492@126.comtoAUTHOR_MAPfor contributor attribution.tests/gateway/test_agent_cache.py: coverSOUL.mdfingerprint extraction and signature busting.tests/run_agent/test_run_agent.py: cover stale prompt rebuilds, SOUL removal, shortened-SOUL regression, and hook behavior.How to Test
python -m ruff check agent/conversation_loop.py gateway/run.py scripts/release.py tests/run_agent/test_run_agent.py tests/gateway/test_agent_cache.pypython -m pytest tests/gateway/test_agent_cache.py tests/run_agent/test_run_agent.py::TestSystemPromptStability -qpython -m py_compile agent/conversation_loop.py gateway/run.py scripts/release.pygit diff --checkChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
N/A