mirror of
https://github.com/HKUDS/nanobot.git
synced 2026-08-06 17:38:35 +00:00
review(dream): harden line-age annotation per review feedback
Follow-up to #3212, fully backward compatible: - Extract the 14-day staleness threshold as `_STALE_THRESHOLD_DAYS` module constant and pass it into the Phase 1 prompt template as `{{ stale_threshold_days }}`. The number lived in three places before (code threshold, prompt instruction, docstring); now there is one. - Add `DreamConfig.annotate_line_ages` (default True = current behavior) and propagate it through `Dream.__init__` and the gateway wiring in cli/commands.py. Gives users a knob to disable the feature without a code patch if an LLM reacts poorly to the `← Nd` suffix. - Harden `_annotate_with_ages` against dirty working trees: when HEAD blob line count disagrees with the working-tree content length, skip annotation entirely instead of assigning ages to the wrong lines. The previous `i >= len(ages)` guard only handled one direction of the mismatch. - Inline-comment the `max_iterations` 10→15 bump with a pointer to exp002 so future blame has context. - Add 4 regression tests: end-to-end `← 30d` reaches prompt, 14/15 threshold boundary, `annotate_line_ages=False` bypasses git entirely (verified via `assert_not_called`), length-mismatch defense, and template-var rendering. Made-with: Cursor
This commit is contained in:
parent
35f3084c03
commit
cc5a666d5d
@ -552,6 +552,13 @@ class Consolidator:
|
|||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
# Single source of truth for the staleness threshold used in _annotate_with_ages
|
||||||
|
# *and* in the Phase 1 prompt template (passed as `stale_threshold_days`).
|
||||||
|
# Keep code and prompt aligned — if you bump this, the LLM's instruction string
|
||||||
|
# updates automatically.
|
||||||
|
_STALE_THRESHOLD_DAYS = 14
|
||||||
|
|
||||||
|
|
||||||
class Dream:
|
class Dream:
|
||||||
"""Two-phase memory processor: analyze history.jsonl, then edit files via AgentRunner.
|
"""Two-phase memory processor: analyze history.jsonl, then edit files via AgentRunner.
|
||||||
|
|
||||||
@ -568,6 +575,7 @@ class Dream:
|
|||||||
max_batch_size: int = 20,
|
max_batch_size: int = 20,
|
||||||
max_iterations: int = 10,
|
max_iterations: int = 10,
|
||||||
max_tool_result_chars: int = 16_000,
|
max_tool_result_chars: int = 16_000,
|
||||||
|
annotate_line_ages: bool = True,
|
||||||
):
|
):
|
||||||
self.store = store
|
self.store = store
|
||||||
self.provider = provider
|
self.provider = provider
|
||||||
@ -575,6 +583,10 @@ class Dream:
|
|||||||
self.max_batch_size = max_batch_size
|
self.max_batch_size = max_batch_size
|
||||||
self.max_iterations = max_iterations
|
self.max_iterations = max_iterations
|
||||||
self.max_tool_result_chars = max_tool_result_chars
|
self.max_tool_result_chars = max_tool_result_chars
|
||||||
|
# Kill switch for the git-blame-based per-line age annotation in Phase 1.
|
||||||
|
# Default True keeps the #3212 behavior; set False to feed MEMORY.md raw
|
||||||
|
# (e.g. if a specific LLM reacts poorly to the `← Nd` suffix).
|
||||||
|
self.annotate_line_ages = annotate_line_ages
|
||||||
self._runner = AgentRunner(provider)
|
self._runner = AgentRunner(provider)
|
||||||
self._tools = self._build_tools()
|
self._tools = self._build_tools()
|
||||||
|
|
||||||
@ -635,9 +647,12 @@ class Dream:
|
|||||||
def _annotate_with_ages(self, content: str) -> str:
|
def _annotate_with_ages(self, content: str) -> str:
|
||||||
"""Append per-line age suffixes to MEMORY.md content.
|
"""Append per-line age suffixes to MEMORY.md content.
|
||||||
|
|
||||||
Each non-blank line gets a suffix like ``← 30d`` indicating how
|
Each non-blank line whose age exceeds ``_STALE_THRESHOLD_DAYS`` gets a
|
||||||
many days since it was last modified. Lines ≤14 days old get no
|
suffix like ``← 30d`` indicating days since last modification.
|
||||||
suffix. Returns the original content unchanged if git is unavailable.
|
Returns the original content unchanged if git is unavailable,
|
||||||
|
annotate fails, or the line count doesn't match the age count
|
||||||
|
(which can happen with an uncommitted working-tree edit — better to
|
||||||
|
skip annotation than to tag the wrong line).
|
||||||
SOUL.md and USER.md are never annotated.
|
SOUL.md and USER.md are never annotated.
|
||||||
"""
|
"""
|
||||||
file_path = "memory/MEMORY.md"
|
file_path = "memory/MEMORY.md"
|
||||||
@ -651,14 +666,23 @@ class Dream:
|
|||||||
|
|
||||||
had_trailing = content.endswith("\n")
|
had_trailing = content.endswith("\n")
|
||||||
lines = content.splitlines()
|
lines = content.splitlines()
|
||||||
|
# If HEAD-blob line count disagrees with the working-tree content we
|
||||||
|
# received, ages would be assigned to the wrong lines — skip entirely
|
||||||
|
# and feed the LLM un-annotated content rather than misleading data.
|
||||||
|
if len(lines) != len(ages):
|
||||||
|
logger.debug(
|
||||||
|
"line_ages length mismatch for {} (lines={}, ages={}); skipping annotation",
|
||||||
|
file_path, len(lines), len(ages),
|
||||||
|
)
|
||||||
|
return content
|
||||||
|
|
||||||
annotated: list[str] = []
|
annotated: list[str] = []
|
||||||
for i, line in enumerate(lines):
|
for line, age in zip(lines, ages):
|
||||||
if not line.strip() or i >= len(ages):
|
if not line.strip():
|
||||||
annotated.append(line)
|
annotated.append(line)
|
||||||
continue
|
continue
|
||||||
d = ages[i].age_days
|
if age.age_days > _STALE_THRESHOLD_DAYS:
|
||||||
if d > 14:
|
annotated.append(f"{line} \u2190 {age.age_days}d")
|
||||||
annotated.append(f"{line} \u2190 {d}d")
|
|
||||||
else:
|
else:
|
||||||
annotated.append(line)
|
annotated.append(line)
|
||||||
result = "\n".join(annotated)
|
result = "\n".join(annotated)
|
||||||
@ -686,10 +710,14 @@ class Dream:
|
|||||||
f"[{e['timestamp']}] {e['content']}" for e in batch
|
f"[{e['timestamp']}] {e['content']}" for e in batch
|
||||||
)
|
)
|
||||||
|
|
||||||
# Current file contents + per-line age annotations
|
# Current file contents + per-line age annotations (MEMORY.md only)
|
||||||
current_date = datetime.now().strftime("%Y-%m-%d")
|
current_date = datetime.now().strftime("%Y-%m-%d")
|
||||||
raw_memory = self.store.read_memory() or "(empty)"
|
raw_memory = self.store.read_memory() or "(empty)"
|
||||||
current_memory = self._annotate_with_ages(raw_memory)
|
current_memory = (
|
||||||
|
self._annotate_with_ages(raw_memory)
|
||||||
|
if self.annotate_line_ages
|
||||||
|
else raw_memory
|
||||||
|
)
|
||||||
current_soul = self.store.read_soul() or "(empty)"
|
current_soul = self.store.read_soul() or "(empty)"
|
||||||
current_user = self.store.read_user() or "(empty)"
|
current_user = self.store.read_user() or "(empty)"
|
||||||
|
|
||||||
@ -711,7 +739,11 @@ class Dream:
|
|||||||
messages=[
|
messages=[
|
||||||
{
|
{
|
||||||
"role": "system",
|
"role": "system",
|
||||||
"content": render_template("agent/dream_phase1.md", strip=True),
|
"content": render_template(
|
||||||
|
"agent/dream_phase1.md",
|
||||||
|
strip=True,
|
||||||
|
stale_threshold_days=_STALE_THRESHOLD_DAYS,
|
||||||
|
),
|
||||||
},
|
},
|
||||||
{"role": "user", "content": phase1_prompt},
|
{"role": "user", "content": phase1_prompt},
|
||||||
],
|
],
|
||||||
|
|||||||
@ -871,6 +871,7 @@ def gateway(
|
|||||||
agent.dream.model = dream_cfg.model_override
|
agent.dream.model = dream_cfg.model_override
|
||||||
agent.dream.max_batch_size = dream_cfg.max_batch_size
|
agent.dream.max_batch_size = dream_cfg.max_batch_size
|
||||||
agent.dream.max_iterations = dream_cfg.max_iterations
|
agent.dream.max_iterations = dream_cfg.max_iterations
|
||||||
|
agent.dream.annotate_line_ages = dream_cfg.annotate_line_ages
|
||||||
from nanobot.cron.types import CronJob, CronPayload
|
from nanobot.cron.types import CronJob, CronPayload
|
||||||
cron.register_system_job(CronJob(
|
cron.register_system_job(CronJob(
|
||||||
id="dream",
|
id="dream",
|
||||||
|
|||||||
@ -43,7 +43,12 @@ class DreamConfig(Base):
|
|||||||
validation_alias=AliasChoices("modelOverride", "model", "model_override"),
|
validation_alias=AliasChoices("modelOverride", "model", "model_override"),
|
||||||
) # Optional Dream-specific model override
|
) # Optional Dream-specific model override
|
||||||
max_batch_size: int = Field(default=20, ge=1) # Max history entries per run
|
max_batch_size: int = Field(default=20, ge=1) # Max history entries per run
|
||||||
|
# Bumped from 10 to 15 in #3212 (exp002: +30% dedup, no accuracy loss; >15 plateaus).
|
||||||
max_iterations: int = Field(default=15, ge=1) # Max tool calls per Phase 2
|
max_iterations: int = Field(default=15, ge=1) # Max tool calls per Phase 2
|
||||||
|
# Per-line git-blame age annotation in Phase 1 prompt (see #3212). Default
|
||||||
|
# on — set to False to feed MEMORY.md raw if a specific LLM reacts poorly
|
||||||
|
# to the `← Nd` suffix or you want deterministic, git-independent prompts.
|
||||||
|
annotate_line_ages: bool = True
|
||||||
|
|
||||||
def build_schedule(self, timezone: str) -> CronSchedule:
|
def build_schedule(self, timezone: str) -> CronSchedule:
|
||||||
"""Build the runtime schedule, preferring the legacy cron override if present."""
|
"""Build the runtime schedule, preferring the legacy cron override if present."""
|
||||||
|
|||||||
@ -26,7 +26,7 @@ Staleness — MEMORY.md lines may have a ``← Nd`` suffix showing days since la
|
|||||||
- Age only indicates when content was last touched, not whether it should be removed
|
- Age only indicates when content was last touched, not whether it should be removed
|
||||||
- Use content judgment: user habits/preferences/personality traits are permanent regardless of age
|
- Use content judgment: user habits/preferences/personality traits are permanent regardless of age
|
||||||
- Only prune content that is objectively outdated: passed events, resolved tracking, superseded approaches
|
- Only prune content that is objectively outdated: passed events, resolved tracking, superseded approaches
|
||||||
- Lines with ``← Nd`` (N>14) deserve closer review but are NOT automatically removable
|
- Lines with ``← Nd`` (N>{{ stale_threshold_days }}) deserve closer review but are NOT automatically removable
|
||||||
- When removing: prefer deleting individual items over entire sections
|
- When removing: prefer deleting individual items over entire sections
|
||||||
|
|
||||||
Skill discovery — flag [SKILL] when ALL of these are true:
|
Skill discovery — flag [SKILL] when ALL of these are true:
|
||||||
|
|||||||
@ -2,11 +2,12 @@
|
|||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
from unittest.mock import AsyncMock, MagicMock
|
from unittest.mock import AsyncMock, MagicMock, patch
|
||||||
|
|
||||||
from nanobot.agent.memory import Dream, MemoryStore
|
from nanobot.agent.memory import Dream, MemoryStore
|
||||||
from nanobot.agent.runner import AgentRunResult
|
from nanobot.agent.runner import AgentRunResult
|
||||||
from nanobot.agent.skills import BUILTIN_SKILLS_DIR
|
from nanobot.agent.skills import BUILTIN_SKILLS_DIR
|
||||||
|
from nanobot.utils.gitstore import LineAge
|
||||||
|
|
||||||
|
|
||||||
@pytest.fixture
|
@pytest.fixture
|
||||||
@ -175,3 +176,83 @@ class TestDreamRun:
|
|||||||
user_msg = call_args.kwargs.get("messages", call_args[1].get("messages"))[1]["content"]
|
user_msg = call_args.kwargs.get("messages", call_args[1].get("messages"))[1]["content"]
|
||||||
assert "## Current MEMORY.md" in user_msg
|
assert "## Current MEMORY.md" in user_msg
|
||||||
|
|
||||||
|
async def test_phase1_prompt_carries_age_suffix_for_stale_lines(
|
||||||
|
self, dream, mock_provider, mock_runner, store,
|
||||||
|
):
|
||||||
|
"""End-to-end: ages >14d must appear verbatim in the LLM prompt, ages ≤14d must not."""
|
||||||
|
# MEMORY.md fixture has 2 non-blank lines ("# Memory" and "- Project X active").
|
||||||
|
# Inject four ages to cover threshold boundaries: >14 suffix, ==14 no suffix, <14 no suffix.
|
||||||
|
store.write_memory("# Memory\n- Project X active\n- fresh item\n- edge case line")
|
||||||
|
store.append_history("some event")
|
||||||
|
mock_provider.chat_with_retry.return_value = MagicMock(content="[SKIP]")
|
||||||
|
mock_runner.run = AsyncMock(return_value=_make_run_result())
|
||||||
|
|
||||||
|
fake_ages = [
|
||||||
|
LineAge(age_days=30), # "# Memory" → should get ← 30d
|
||||||
|
LineAge(age_days=20), # "- Project X..." → should get ← 20d
|
||||||
|
LineAge(age_days=14), # "- fresh item" → ==14, threshold is strictly >14, no suffix
|
||||||
|
LineAge(age_days=5), # "- edge case..." → no suffix
|
||||||
|
]
|
||||||
|
with patch.object(store.git, "line_ages", return_value=fake_ages):
|
||||||
|
await dream.run()
|
||||||
|
|
||||||
|
call_args = mock_provider.chat_with_retry.call_args
|
||||||
|
user_msg = call_args.kwargs.get("messages", call_args[1].get("messages"))[1]["content"]
|
||||||
|
memory_section = user_msg.split("## Current MEMORY.md")[1].split("## Current SOUL.md")[0]
|
||||||
|
assert "\u2190 30d" in memory_section
|
||||||
|
assert "\u2190 20d" in memory_section
|
||||||
|
assert "\u2190 14d" not in memory_section
|
||||||
|
assert "\u2190 5d" not in memory_section
|
||||||
|
|
||||||
|
async def test_phase1_skips_annotation_when_disabled(
|
||||||
|
self, dream, mock_provider, mock_runner, store,
|
||||||
|
):
|
||||||
|
"""`annotate_line_ages=False` must bypass the git lookup entirely and keep MEMORY.md raw."""
|
||||||
|
store.append_history("some event")
|
||||||
|
mock_provider.chat_with_retry.return_value = MagicMock(content="[SKIP]")
|
||||||
|
mock_runner.run = AsyncMock(return_value=_make_run_result())
|
||||||
|
|
||||||
|
dream.annotate_line_ages = False
|
||||||
|
# line_ages must be bypassed entirely — verify with a spy rather than a
|
||||||
|
# raising side_effect, because _annotate_with_ages catches Exception
|
||||||
|
# (which swallows AssertionError) and would hide an accidental call.
|
||||||
|
with patch.object(store.git, "line_ages") as mock_line_ages:
|
||||||
|
await dream.run()
|
||||||
|
mock_line_ages.assert_not_called()
|
||||||
|
|
||||||
|
call_args = mock_provider.chat_with_retry.call_args
|
||||||
|
user_msg = call_args.kwargs.get("messages", call_args[1].get("messages"))[1]["content"]
|
||||||
|
assert "\u2190" not in user_msg
|
||||||
|
|
||||||
|
async def test_phase1_skips_annotation_on_line_ages_length_mismatch(
|
||||||
|
self, dream, mock_provider, mock_runner, store,
|
||||||
|
):
|
||||||
|
"""If ages length != lines length (dirty working tree), skip annotation instead of mis-tagging."""
|
||||||
|
# MEMORY.md has 2 non-blank lines but we hand back only 1 age → mismatch.
|
||||||
|
store.append_history("some event")
|
||||||
|
mock_provider.chat_with_retry.return_value = MagicMock(content="[SKIP]")
|
||||||
|
mock_runner.run = AsyncMock(return_value=_make_run_result())
|
||||||
|
|
||||||
|
with patch.object(store.git, "line_ages", return_value=[LineAge(age_days=999)]):
|
||||||
|
await dream.run()
|
||||||
|
|
||||||
|
call_args = mock_provider.chat_with_retry.call_args
|
||||||
|
user_msg = call_args.kwargs.get("messages", call_args[1].get("messages"))[1]["content"]
|
||||||
|
memory_section = user_msg.split("## Current MEMORY.md")[1].split("## Current SOUL.md")[0]
|
||||||
|
# No age arrow at all — we refused to annotate rather than tag the wrong line.
|
||||||
|
assert "\u2190" not in memory_section
|
||||||
|
|
||||||
|
async def test_phase1_prompt_uses_threshold_from_template_var(
|
||||||
|
self, dream, mock_provider, mock_runner, store,
|
||||||
|
):
|
||||||
|
"""System prompt should reference the stale-threshold constant, not a hardcoded 14."""
|
||||||
|
store.append_history("some event")
|
||||||
|
mock_provider.chat_with_retry.return_value = MagicMock(content="[SKIP]")
|
||||||
|
mock_runner.run = AsyncMock(return_value=_make_run_result())
|
||||||
|
|
||||||
|
await dream.run()
|
||||||
|
|
||||||
|
system_msg = mock_provider.chat_with_retry.call_args.kwargs["messages"][0]["content"]
|
||||||
|
# The template renders with stale_threshold_days=14 → LLM must see "N>14"
|
||||||
|
assert "N>14" in system_msg
|
||||||
|
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user