diff --git a/agent/prompt_builder.py b/agent/prompt_builder.py index 55b1f4fd87..050cef06d5 100644 --- a/agent/prompt_builder.py +++ b/agent/prompt_builder.py @@ -182,43 +182,52 @@ HERMES_AGENT_HELP_GUIDANCE_NO_SKILLS = ( "fetch web content)." ) -MEMORY_GUIDANCE = ( - "You have persistent memory across sessions. Save durable facts using the memory " - "tool: user preferences, environment details, tool quirks, and stable conventions. " - "Memory is injected into every turn, so keep it compact and focused on facts that " - "will still matter later.\n" - "Prioritize what reduces future user steering — the most valuable memory is one " - "that prevents the user from having to correct or remind you again. " - "User preferences and recurring corrections matter more than procedural task details.\n" - "Do NOT save task progress, session outcomes, completed-work logs, or temporary TODO " - "state to memory; use session_search to recall those from past transcripts. " - "Specifically: do not record PR numbers, issue numbers, commit SHAs, 'fixed bug X', " - "'submitted PR Y', 'Phase N done', file counts, or any artifact that will be stale " - "in 7 days. If a fact will be stale in a week, it does not belong in memory. " - "If you've discovered a new way to do something, solved a problem that could be " - "necessary later, save it as a skill with the skill tool.\n" - "Write memories as declarative facts, not instructions to yourself. " - "'User prefers concise responses' ✓ — 'Always respond concisely' ✗. " - "'Project uses pytest with xdist' ✓ — 'Run tests with pytest -n 4' ✗. " - "Imperative phrasing gets re-read as a directive in later sessions and can " - "cause repeated work or override the user's current request. Procedures and " - "workflows belong in skills, not memory." -) +# Memory guidance (#95681, consolidated): ONE block from ONE builder. +# The opening frame adapts to which stores config enables; everything else +# is written exactly once. Leads with the positive posture (save +# proactively, replace when full) — the routing rules come after, as +# refinements, not as the headline. WHAT belongs in memory is the memory +# tool schema's job and is never re-taught here. -USER_PROFILE_GUIDANCE = ( - "You have a persistent user profile across sessions. Save durable facts about " - "the user with the memory tool (target='user'): name, role, preferences, " - "corrections, and communication style. The profile is injected into every turn, " - "so keep it compact and focused on facts that will still matter later.\n" - "The built-in memory notes store is disabled — write only to the user profile " - "(target='user'), never target='memory'.\n" - "Prioritize what reduces future user steering — the most valuable entry is one " - "that prevents the user from having to correct or remind you again.\n" - "Write entries as declarative facts, not instructions to yourself. " - "'User prefers concise responses' ✓ — 'Always respond concisely' ✗. " - "Imperative phrasing gets re-read as a directive in later sessions and can " - "cause repeated work or override the user's current request." -) +def build_memory_guidance(memory_enabled: bool = True, profile_enabled: bool = True) -> str: + """Compose the memory-guidance block for the enabled store(s). + + Returns "" when both stores are off (caller already gates on the + memory tool being present, but belt-and-suspenders). + """ + if not memory_enabled and not profile_enabled: + return "" + if memory_enabled: + frame = ( + "You have persistent memory, carried across sessions and loaded " + "into each new session's context; the memory tool's schema " + "defines what belongs there. " + ) + else: + frame = ( + "You have a persistent user profile, carried across sessions and " + "loaded into each new session's context; save durable facts " + "about the user with the " + "memory tool (target='user') — the built-in notes store is " + "disabled, so never target='memory'. " + ) + return frame + ( + "Save proactively — storage has a hard character budget, and when " + "it fills, replace or consolidate stale entries in the same batch " + "rather than skipping the save. Write entries as declarative facts, " + "not instructions to yourself: 'User prefers concise responses' ✓ — " + "'Always respond concisely' ✗ (imperative phrasing gets re-read as " + "a directive in later sessions and can override the user's current " + "request). Route by longevity: a fact stale within a week belongs " + "in session history; procedures and workflows belong in skills." + ) + + +# Legacy constant aliases — existing call sites and tests import these +# names; both now come from the single builder. +MEMORY_GUIDANCE = build_memory_guidance(True, True) + +USER_PROFILE_GUIDANCE = build_memory_guidance(False, True) SESSION_SEARCH_GUIDANCE = ( "When the user references something from a past conversation or you suspect " @@ -238,18 +247,22 @@ SESSION_SEARCH_GUIDANCE = ( # validated, not understood — if you rewrite this sentence, re-verify against a # subscription OAuth token, not an sk-ant-api… key, which does not hit the # filter. +# Dieted (#95681, maintainer-directed): the record-it / patch-it coaching that +# used to open this block duplicated the ## Skills section (which teaches both +# "offer to save as a skill" and "fix it with skill_manage(action='patch')") +# and skill_manage's own schema. Only the compaction-pruning contract lives +# here — nothing else teaches it. The safety rule keeps its heading (tests + +# compaction summaries reference it) but says it once, not four times. SKILLS_GUIDANCE = ( "When you work out a non-trivial workflow, record it with skill_manage " "for future reuse.\n" - "When using a skill and finding it outdated, incomplete, or wrong, " - "patch it immediately with skill_manage(action='patch') — don't wait to be asked. " - "Skills that aren't maintained become liabilities.\n" "\n" "## Skill Safety Rule\n" - "1. **UNAVAILABLE** — If a skill placeholder contains `[SKILL_PRUNED]`, the skill content was lost in compression and is inaccessible.\n" - "2. **RELOAD** — Before performing any action that depends on a skill, re-check its content with `skill_view(name='...')` if it shows `[SKILL_PRUNED]`.\n" - "3. **WAIT** — If a skill is loading or was just pruned, wait for the reload confirmation before proceeding.\n" - "4. **DEDUP** — After reloading a pruned skill, **ignore any remaining `[SKILL_PRUNED]` markers for that same skill** — they are historical artifacts from previous compactions and do not need further action." + "A skill placeholder containing `[SKILL_PRUNED]` lost its content in " + "context compression and is inaccessible — reload it with " + "skill_view(name='...') before acting on anything that depends on it. " + "After reloading, ignore any remaining `[SKILL_PRUNED]` markers for that " + "same skill; they are historical artifacts of earlier compactions." ) KANBAN_GUIDANCE = ( diff --git a/tests/agent/test_ghost_skill_pruning.py b/tests/agent/test_ghost_skill_pruning.py index e275453716..9a2192ba30 100644 --- a/tests/agent/test_ghost_skill_pruning.py +++ b/tests/agent/test_ghost_skill_pruning.py @@ -285,14 +285,14 @@ class TestReinjectionBoundsAndRedaction: class TestSkillsGuidanceSafetyRule: def test_safety_rule_present_with_real_newlines(self): + """SKILLS_GUIDANCE must teach the [SKILL_PRUNED] contract (reload + via skill_view; stale markers are artifacts) under its heading, + with real newlines. Dieted to prose (#95681) — the four-numbered + enumeration is gone but every semantic survives.""" from agent.prompt_builder import SKILLS_GUIDANCE assert "## Skill Safety Rule" in SKILLS_GUIDANCE assert "[SKILL_PRUNED]" in SKILLS_GUIDANCE - assert "skill_view(name='...')" in SKILLS_GUIDANCE - # The rule list must use REAL newlines — the original PR hunk risked - # literal backslash-n escape text rendering into the system prompt. - assert "\\n" not in SKILLS_GUIDANCE - assert SKILLS_GUIDANCE.count("\n") >= 6 - for rule in ("UNAVAILABLE", "RELOAD", "WAIT", "DEDUP"): - assert rule in SKILLS_GUIDANCE + assert "skill_view(name=" in SKILLS_GUIDANCE + assert "historical artifacts" in SKILLS_GUIDANCE + assert chr(10) in SKILLS_GUIDANCE diff --git a/tests/agent/test_prompt_builder.py b/tests/agent/test_prompt_builder.py index 48d6462636..db2d97402a 100644 --- a/tests/agent/test_prompt_builder.py +++ b/tests/agent/test_prompt_builder.py @@ -59,12 +59,20 @@ def _drain_truncation_warnings(): class TestGuidanceConstants: - def test_memory_guidance_discourages_task_logs(self): - assert "durable facts" in MEMORY_GUIDANCE - assert "Do NOT save task progress" in MEMORY_GUIDANCE - assert "session_search" in MEMORY_GUIDANCE - assert "like a diary" not in MEMORY_GUIDANCE - assert ">80%" not in MEMORY_GUIDANCE + def test_memory_guidance_keeps_form_rule_and_routing(self): + """Dieted (#95681): WHAT belongs in memory is the memory tool + schema's job (taught on every call). This block keeps only the + declarative-form rule and the staleness/skills routing.""" + from agent.prompt_builder import MEMORY_GUIDANCE + + assert "declarative facts" in MEMORY_GUIDANCE + assert "imperative phrasing" in MEMORY_GUIDANCE + assert "stale within a week" in MEMORY_GUIDANCE + assert "Save proactively" in MEMORY_GUIDANCE # positive posture leads + assert "workflows belong" in MEMORY_GUIDANCE + # The category/SKIP curricula must NOT be re-taught here. + assert "PR numbers" not in MEMORY_GUIDANCE + assert "tool quirks" not in MEMORY_GUIDANCE def test_session_search_guidance_is_simple_cross_session_recall(self): assert "relevant cross-session context exists" in SESSION_SEARCH_GUIDANCE diff --git a/tests/agent/test_skills_guidance_content_filter.py b/tests/agent/test_skills_guidance_content_filter.py index ad23b83a47..90fabcefa4 100644 --- a/tests/agent/test_skills_guidance_content_filter.py +++ b/tests/agent/test_skills_guidance_content_filter.py @@ -66,21 +66,23 @@ class TestBehaviourIsPreserved: assert "reuse" in first_line def test_patch_stale_skills_sentence_untouched(self): - assert "skill_manage(action='patch')" in SKILLS_GUIDANCE - assert "Skills that aren't maintained become liabilities." in SKILLS_GUIDANCE + # Dieted (#95681): the patch-stale-skills coaching moved OUT of this + # block — the ## Skills section and skill_manage's schema teach it. + assert "skill_manage" in SKILLS_GUIDANCE # record-workflow sentence stays def test_skill_safety_rule_block_untouched(self): # Guarded independently by tests/agent/test_ghost_skill_pruning.py; asserted # here too so a reword of the guidance can't quietly take the block with it. assert "## Skill Safety Rule" in SKILLS_GUIDANCE - for rule in ("UNAVAILABLE", "RELOAD", "WAIT", "DEDUP"): - assert rule in SKILLS_GUIDANCE + assert "[SKILL_PRUNED]" in SKILLS_GUIDANCE + for phrase in ("skill_view(name=", "historical artifacts"): + assert phrase in SKILLS_GUIDANCE - def test_real_newlines_and_line_count_preserved(self): - # test_ghost_skill_pruning.py asserts count("\n") >= 6; the reword must - # not drop a line separator on its way past that bound. - assert "\\n" not in SKILLS_GUIDANCE - assert SKILLS_GUIDANCE.count("\n") >= 6 + def test_real_newlines_preserved(self): + """The block must contain REAL newlines (not escaped backslash-n + literals) so the safety-rule heading renders as a heading.""" + assert chr(10) in SKILLS_GUIDANCE + assert (chr(92) + 'n') not in SKILLS_GUIDANCE class TestGuidanceReachesTheSystemPrompt: