diff --git a/openspace/agents/grounding/__init__.py b/openspace/agents/grounding/__init__.py new file mode 100644 index 00000000..d94d589b --- /dev/null +++ b/openspace/agents/grounding/__init__.py @@ -0,0 +1 @@ +# openspace.agents.grounding — GroundingAgent subsystem package diff --git a/openspace/agents/grounding/context.py b/openspace/agents/grounding/context.py new file mode 100644 index 00000000..2aced4b4 --- /dev/null +++ b/openspace/agents/grounding/context.py @@ -0,0 +1,65 @@ +"""Skill-context and skill-registry helpers for GroundingAgent. + +Pure side-effect functions that operate on agent instance state. +Extracted from grounding_agent.py (Epic 5.7). +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, List, Optional + +from openspace.utils.logging import Logger + +if TYPE_CHECKING: + from openspace.skill_engine import SkillRegistry + +logger = Logger.get_logger("openspace.agents.grounding_agent") + + +def set_skill_context( + agent, + context: str, + skill_ids: Optional[List[str]] = None, +) -> None: + """Inject skill guidance into the agent's system prompt. + + Called by ``OpenSpace.execute()`` before ``process()`` when skills + are matched. The context is a formatted string built by + ``SkillRegistry.build_context_injection()``. + + Args: + agent: GroundingAgent instance. + context: Formatted skill content for system prompt injection. + skill_ids: skill_id values of injected skills. + """ + agent._skill_context = context if context else None + agent._active_skill_ids = skill_ids or [] + if agent._skill_context: + logger.info( + f"Skill context set: {', '.join(agent._active_skill_ids) or '(unnamed)'}" + ) + + +def clear_skill_context(agent) -> None: + """Remove skill guidance (used before fallback execution).""" + if agent._skill_context: + logger.info( + f"Skill context cleared (was: {', '.join(agent._active_skill_ids)})" + ) + agent._skill_context = None + agent._active_skill_ids = [] + + +def has_skill_context(agent) -> bool: + """Return True if skill context is currently set.""" + return agent._skill_context is not None + + +def set_skill_registry(agent, registry: Optional["SkillRegistry"]) -> None: + """Attach a SkillRegistry so the agent can offer ``retrieve_skill`` as a tool.""" + agent._skill_registry = registry + if registry: + count = len(registry.list_skills()) + logger.info( + f"Skill registry attached ({count} skill(s) available for mid-iteration retrieval)" + ) diff --git a/openspace/agents/grounding/messages.py b/openspace/agents/grounding/messages.py new file mode 100644 index 00000000..af0f69fb --- /dev/null +++ b/openspace/agents/grounding/messages.py @@ -0,0 +1,123 @@ +"""Message safety helpers for GroundingAgent. + +Functions to cap oversized message content and truncate long +conversation histories before LLM calls. +Extracted from grounding_agent.py (Epic 5.7). +""" + +from __future__ import annotations + +import json +from typing import Any, Dict, List + +from openspace.utils.logging import Logger + +logger = Logger.get_logger("openspace.agents.grounding_agent") + +# Maximum characters allowed in a single message content field. +_MAX_SINGLE_CONTENT_CHARS = 30_000 + + +def cap_message_content( + messages: List[Dict[str, Any]], + cap: int = _MAX_SINGLE_CONTENT_CHARS, +) -> List[Dict[str, Any]]: + """Truncate oversized individual message contents in-place. + + Targets tool-result messages and assistant messages that can + carry enormous file contents (read_file on large CSVs/scripts). + System messages and the first user instruction are never touched. + + Args: + messages: The message list (mutated in-place). + cap: Maximum character count per message. + + Returns: + The same *messages* list (for chaining). + """ + trimmed = 0 + for msg in messages: + content = msg.get("content") + if not isinstance(content, str) or len(content) <= cap: + continue + if msg.get("role") == "system": + continue + original_len = len(content) + msg["content"] = ( + content[: cap // 2] + + f"\n\n... [truncated {original_len - cap:,} chars] ...\n\n" + + content[-(cap // 2) :] + ) + trimmed += 1 + if trimmed: + logger.info(f"Capped {trimmed} oversized message(s) to {cap:,} chars each") + return messages + + +def truncate_messages( + messages: List[Dict[str, Any]], + keep_recent: int = 8, + max_tokens_estimate: int = 120_000, + cap: int = _MAX_SINGLE_CONTENT_CHARS, +) -> List[Dict[str, Any]]: + """Trim conversation history to fit within token budget. + + Steps: + 1. Cap any single oversized message (via :func:`cap_message_content`). + 2. If total estimated tokens exceed *max_tokens_estimate*, keep only + the system messages, the first user instruction, and the most + recent *keep_recent* conversation rounds. + + Args: + messages: Full message list. + keep_recent: Number of recent conversation rounds to preserve. + max_tokens_estimate: Approximate token budget. + cap: Per-message character cap (forwarded to cap_message_content). + + Returns: + Possibly shortened message list. + """ + messages = cap_message_content(messages, cap) + + if len(messages) <= keep_recent + 2: # +2 for system and initial user + return messages + + total_text = json.dumps(messages, ensure_ascii=False) + estimated_tokens = len(total_text) // 4 + + if estimated_tokens < max_tokens_estimate: + return messages + + logger.info( + f"Truncating message history: {len(messages)} messages, " + f"~{estimated_tokens:,} tokens -> keeping recent {keep_recent} rounds" + ) + + system_messages: List[Dict[str, Any]] = [] + user_instruction = None + conversation_messages: List[Dict[str, Any]] = [] + + for msg in messages: + role = msg.get("role") + if role == "system": + system_messages.append(msg) + elif role == "user" and user_instruction is None: + user_instruction = msg + else: + conversation_messages.append(msg) + + recent_messages = ( + conversation_messages[-(keep_recent * 2) :] if conversation_messages else [] + ) + + truncated = system_messages.copy() + if user_instruction: + truncated.append(user_instruction) + truncated.extend(recent_messages) + + logger.info( + f"After truncation: {len(truncated)} messages, " + f"~{len(json.dumps(truncated, ensure_ascii=False)) // 4:,} tokens (estimated)" + ) + + return truncated diff --git a/openspace/agents/grounding_agent.py b/openspace/agents/grounding_agent.py index 6d6938c9..61d903d9 100644 --- a/openspace/agents/grounding_agent.py +++ b/openspace/agents/grounding_agent.py @@ -5,6 +5,17 @@ from typing import TYPE_CHECKING, Any, Dict, List, Optional from openspace.agents.base import BaseAgent +from openspace.agents.grounding.context import ( + clear_skill_context as _clear_skill_context, + has_skill_context as _has_skill_context, + set_skill_context as _set_skill_context, + set_skill_registry as _set_skill_registry, +) +from openspace.agents.grounding.messages import ( + _MAX_SINGLE_CONTENT_CHARS, + cap_message_content as _cap_message_content, + truncate_messages as _truncate_messages_impl, +) from openspace.grounding.core.types import BackendType, ToolResult from openspace.platforms.screenshot import ScreenshotClient from openspace.prompts import GroundingAgentPrompts @@ -86,116 +97,35 @@ def set_skill_context( context: str, skill_ids: Optional[List[str]] = None, ) -> None: - """Inject skill guidance into the agent's system prompt. - - Called by ``OpenSpace.execute()`` before ``process()`` when skills - are matched. The context is a formatted string built by - ``SkillRegistry.build_context_injection()``. - - Args: - context: Formatted skill content for system prompt injection. - skill_ids: skill_id values of injected skills. - """ - self._skill_context = context if context else None - self._active_skill_ids = skill_ids or [] - if self._skill_context: - logger.info(f"Skill context set: {', '.join(self._active_skill_ids) or '(unnamed)'}") + """Inject skill guidance into the agent's system prompt.""" + return _set_skill_context(self, context, skill_ids) def clear_skill_context(self) -> None: """Remove skill guidance (used before fallback execution).""" - if self._skill_context: - logger.info(f"Skill context cleared (was: {', '.join(self._active_skill_ids)})") - self._skill_context = None - self._active_skill_ids = [] + return _clear_skill_context(self) @property def has_skill_context(self) -> bool: - return self._skill_context is not None + return _has_skill_context(self) def set_skill_registry(self, registry: Optional["SkillRegistry"]) -> None: """Attach a SkillRegistry so the agent can offer ``retrieve_skill`` as a tool.""" - self._skill_registry = registry - if registry: - count = len(registry.list_skills()) - logger.info(f"Skill registry attached ({count} skill(s) available for mid-iteration retrieval)") + return _set_skill_registry(self, registry) - _MAX_SINGLE_CONTENT_CHARS = 30_000 + _MAX_SINGLE_CONTENT_CHARS = _MAX_SINGLE_CONTENT_CHARS @classmethod def _cap_message_content(cls, messages: List[Dict[str, Any]]) -> List[Dict[str, Any]]: - """Truncate oversized individual message contents in-place. - - Targets tool-result messages and assistant messages that can - carry enormous file contents (read_file on large CSVs/scripts). - System messages and the first user instruction are never touched. - """ - cap = cls._MAX_SINGLE_CONTENT_CHARS - trimmed = 0 - for msg in messages: - content = msg.get("content") - if not isinstance(content, str) or len(content) <= cap: - continue - if msg.get("role") == "system": - continue - original_len = len(content) - msg["content"] = ( - content[: cap // 2] - + f"\n\n... [truncated {original_len - cap:,} chars] ...\n\n" - + content[-(cap // 2) :] - ) - trimmed += 1 - if trimmed: - logger.info(f"Capped {trimmed} oversized message(s) to {cap:,} chars each") - return messages + """Truncate oversized individual message contents in-place.""" + return _cap_message_content(messages, cls._MAX_SINGLE_CONTENT_CHARS) def _truncate_messages( self, messages: List[Dict[str, Any]], keep_recent: int = 8, max_tokens_estimate: int = 120000 ) -> List[Dict[str, Any]]: - # First: cap any single oversized message to prevent one huge - # tool-result from dominating the context window. - messages = self._cap_message_content(messages) - - if len(messages) <= keep_recent + 2: # +2 for system and initial user - return messages - - total_text = json.dumps(messages, ensure_ascii=False) - estimated_tokens = len(total_text) // 4 - - if estimated_tokens < max_tokens_estimate: - return messages - - logger.info( - f"Truncating message history: {len(messages)} messages, " - f"~{estimated_tokens:,} tokens -> keeping recent {keep_recent} rounds" - ) - - system_messages = [] - user_instruction = None - conversation_messages = [] - - for msg in messages: - role = msg.get("role") - if role == "system": - system_messages.append(msg) - elif role == "user" and user_instruction is None: - user_instruction = msg - else: - conversation_messages.append(msg) - - recent_messages = conversation_messages[-(keep_recent * 2) :] if conversation_messages else [] - - truncated = system_messages.copy() - if user_instruction: - truncated.append(user_instruction) - truncated.extend(recent_messages) - - logger.info( - f"After truncation: {len(truncated)} messages, " - f"~{len(json.dumps(truncated, ensure_ascii=False)) // 4:,} tokens (estimated)" + return _truncate_messages_impl( + messages, keep_recent, max_tokens_estimate, cap=self._MAX_SINGLE_CONTENT_CHARS ) - return truncated - async def process(self, context: Dict[str, Any]) -> Dict[str, Any]: """ Process a task execution request with multi-round iteration control. diff --git a/tests/test_grounding_context.py b/tests/test_grounding_context.py new file mode 100644 index 00000000..f6ad9468 --- /dev/null +++ b/tests/test_grounding_context.py @@ -0,0 +1,141 @@ +"""Tests for openspace.agents.grounding.context — skill context helpers. + +Epic 5.7 extraction: set_skill_context, clear_skill_context, +has_skill_context, set_skill_registry. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest + +from openspace.agents.grounding.context import ( + clear_skill_context, + has_skill_context, + set_skill_context, + set_skill_registry, +) + + +class _FakeAgent: + """Minimal stand-in for GroundingAgent instance state.""" + + def __init__(self): + self._skill_context = None + self._active_skill_ids = [] + self._skill_registry = None + + +# ── set_skill_context ────────────────────────────────────────────── + + +class TestSetSkillContext: + def test_sets_context_and_ids(self): + agent = _FakeAgent() + set_skill_context(agent, "skill guidance", ["s1", "s2"]) + assert agent._skill_context == "skill guidance" + assert agent._active_skill_ids == ["s1", "s2"] + + def test_empty_string_clears_context(self): + agent = _FakeAgent() + agent._skill_context = "old" + set_skill_context(agent, "") + assert agent._skill_context is None + + def test_none_ids_defaults_to_empty_list(self): + agent = _FakeAgent() + set_skill_context(agent, "ctx", None) + assert agent._active_skill_ids == [] + + def test_overwrites_previous(self): + agent = _FakeAgent() + set_skill_context(agent, "first", ["a"]) + set_skill_context(agent, "second", ["b"]) + assert agent._skill_context == "second" + assert agent._active_skill_ids == ["b"] + + +# ── clear_skill_context ─────────────────────────────────────────── + + +class TestClearSkillContext: + def test_clears_existing(self): + agent = _FakeAgent() + agent._skill_context = "guidance" + agent._active_skill_ids = ["s1"] + clear_skill_context(agent) + assert agent._skill_context is None + assert agent._active_skill_ids == [] + + def test_noop_when_already_clear(self): + agent = _FakeAgent() + clear_skill_context(agent) + assert agent._skill_context is None + assert agent._active_skill_ids == [] + + +# ── has_skill_context ────────────────────────────────────────────── + + +class TestHasSkillContext: + def test_false_when_none(self): + agent = _FakeAgent() + assert has_skill_context(agent) is False + + def test_true_when_set(self): + agent = _FakeAgent() + agent._skill_context = "present" + assert has_skill_context(agent) is True + + +# ── set_skill_registry ──────────────────────────────────────────── + + +class TestSetSkillRegistry: + def test_attaches_registry(self): + agent = _FakeAgent() + mock_registry = MagicMock() + mock_registry.list_skills.return_value = ["a", "b", "c"] + set_skill_registry(agent, mock_registry) + assert agent._skill_registry is mock_registry + + def test_none_clears_registry(self): + agent = _FakeAgent() + agent._skill_registry = MagicMock() + set_skill_registry(agent, None) + assert agent._skill_registry is None + + def test_logs_skill_count(self): + agent = _FakeAgent() + mock_registry = MagicMock() + mock_registry.list_skills.return_value = ["x"] + set_skill_registry(agent, mock_registry) + mock_registry.list_skills.assert_called_once() + + +# ── Delegation seam tests ────────────────────────────────────────── + + +class TestDelegationSeams: + """Verify grounding_agent.py properly delegates to context module.""" + + def test_set_skill_context_delegates(self): + from openspace.agents.grounding_agent import GroundingAgent + + assert "set_skill_context" in dir(GroundingAgent) + # Method should exist on the class + method = getattr(GroundingAgent, "set_skill_context") + assert callable(method) + + def test_has_skill_context_is_property(self): + from openspace.agents.grounding_agent import GroundingAgent + + assert isinstance( + GroundingAgent.__dict__.get("has_skill_context"), property + ), "has_skill_context must be a property" + + def test_set_skill_registry_delegates(self): + from openspace.agents.grounding_agent import GroundingAgent + + assert "set_skill_registry" in dir(GroundingAgent) diff --git a/tests/test_grounding_messages.py b/tests/test_grounding_messages.py new file mode 100644 index 00000000..6252cc0e --- /dev/null +++ b/tests/test_grounding_messages.py @@ -0,0 +1,179 @@ +"""Tests for openspace.agents.grounding.messages — message safety helpers. + +Epic 5.7 extraction: cap_message_content, truncate_messages. +""" + +from __future__ import annotations + +import pytest + +from openspace.agents.grounding.messages import ( + _MAX_SINGLE_CONTENT_CHARS, + cap_message_content, + truncate_messages, +) + + +# ── cap_message_content ────────────────────────────────────────── + + +class TestCapMessageContent: + def test_short_messages_untouched(self): + msgs = [ + {"role": "system", "content": "system prompt"}, + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "world"}, + ] + result = cap_message_content(msgs) + assert result[1]["content"] == "hello" + assert result[2]["content"] == "world" + + def test_system_message_never_truncated(self): + long = "x" * (_MAX_SINGLE_CONTENT_CHARS + 1000) + msgs = [{"role": "system", "content": long}] + cap_message_content(msgs) + assert len(msgs[0]["content"]) == len(long) + + def test_oversized_user_message_truncated(self): + long = "A" * 50_000 + msgs = [{"role": "user", "content": long}] + cap_message_content(msgs) + assert len(msgs[0]["content"]) < 50_000 + assert "truncated" in msgs[0]["content"] + + def test_oversized_assistant_message_truncated(self): + long = "B" * 50_000 + msgs = [{"role": "assistant", "content": long}] + cap_message_content(msgs) + assert "truncated" in msgs[0]["content"] + + def test_preserves_start_and_end(self): + half = _MAX_SINGLE_CONTENT_CHARS // 2 + content = "START" + "x" * 50_000 + "END" + msgs = [{"role": "tool", "content": content}] + cap_message_content(msgs) + result = msgs[0]["content"] + assert result.startswith("START") + assert result.endswith("END") + + def test_non_string_content_ignored(self): + msgs = [{"role": "assistant", "content": None}] + cap_message_content(msgs) + assert msgs[0]["content"] is None + + def test_missing_content_key_ignored(self): + msgs = [{"role": "assistant"}] + cap_message_content(msgs) + assert "content" not in msgs[0] + + def test_custom_cap(self): + msgs = [{"role": "user", "content": "x" * 200}] + cap_message_content(msgs, cap=100) + assert len(msgs[0]["content"]) < 200 + assert "truncated" in msgs[0]["content"] + + def test_returns_same_list(self): + msgs = [{"role": "user", "content": "short"}] + assert cap_message_content(msgs) is msgs + + def test_multiple_oversized_messages(self): + msgs = [ + {"role": "user", "content": "x" * 50_000}, + {"role": "assistant", "content": "y" * 50_000}, + {"role": "tool", "content": "z" * 50_000}, + ] + cap_message_content(msgs) + for m in msgs: + assert "truncated" in m["content"] + + +# ── truncate_messages ──────────────────────────────────────────── + + +class TestTruncateMessages: + def _make_messages(self, count: int) -> list: + """Build a message list: system + user + N conversation messages.""" + msgs = [ + {"role": "system", "content": "sys"}, + {"role": "user", "content": "initial instruction"}, + ] + for i in range(count): + role = "assistant" if i % 2 == 0 else "user" + msgs.append({"role": role, "content": f"msg-{i}"}) + return msgs + + def test_short_history_untouched(self): + msgs = self._make_messages(5) + result = truncate_messages(msgs) + assert len(result) == len(msgs) + + def test_keeps_system_and_initial_user(self): + msgs = self._make_messages(100) + # Force low token budget + result = truncate_messages(msgs, keep_recent=4, max_tokens_estimate=1) + roles = [m["role"] for m in result] + assert roles[0] == "system" + assert result[1]["content"] == "initial instruction" + + def test_keeps_recent_messages(self): + msgs = self._make_messages(100) + result = truncate_messages(msgs, keep_recent=4, max_tokens_estimate=1) + # Should have: system(1) + user(1) + recent(8) = 10 + assert len(result) == 10 + + def test_under_budget_no_truncation(self): + msgs = self._make_messages(20) + result = truncate_messages(msgs, max_tokens_estimate=1_000_000) + assert len(result) == len(msgs) + + def test_preserves_message_content(self): + msgs = self._make_messages(50) + result = truncate_messages(msgs, keep_recent=2, max_tokens_estimate=1) + # Last messages should be from the original list + last_content = result[-1]["content"] + assert last_content.startswith("msg-") + + def test_default_keep_recent_is_8(self): + msgs = self._make_messages(200) + result = truncate_messages(msgs, max_tokens_estimate=1) + # system(1) + user(1) + recent(16) = 18 + assert len(result) == 18 + + def test_custom_cap_forwarded(self): + """Verify the cap parameter flows through to cap_message_content.""" + msgs = [ + {"role": "system", "content": "sys"}, + {"role": "user", "content": "go"}, + ] + # Add enough oversized messages to trigger truncation + for i in range(20): + msgs.append({"role": "assistant", "content": "x" * 500}) + result = truncate_messages(msgs, keep_recent=2, max_tokens_estimate=1, cap=200) + # Messages should have been capped at 200 chars + for m in result: + if m["role"] != "system" and m["role"] != "user": + assert len(m["content"]) <= 250 # some overhead from truncation marker + + +# ── Delegation seam tests ────────────────────────────────────────── + + +class TestDelegationSeams: + """Verify grounding_agent.py properly delegates to messages module.""" + + def test_cap_message_content_is_classmethod(self): + from openspace.agents.grounding_agent import GroundingAgent + + assert isinstance( + GroundingAgent.__dict__.get("_cap_message_content"), classmethod + ) + + def test_max_single_content_chars_matches(self): + from openspace.agents.grounding_agent import GroundingAgent + + assert GroundingAgent._MAX_SINGLE_CONTENT_CHARS == _MAX_SINGLE_CONTENT_CHARS + + def test_truncate_messages_exists(self): + from openspace.agents.grounding_agent import GroundingAgent + + assert callable(getattr(GroundingAgent, "_truncate_messages", None))