From 3e15159d83cc78e8ed795c1ab737e970ed23537a Mon Sep 17 00:00:00 2001 From: Cameron Date: Fri, 19 Dec 2025 16:21:18 -0800 Subject: [PATCH] fix: prevent human block overwrite when skills block missing (#7656) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: prevent human block overwrite when skills block missing **Bug**: When connecting to agents created before skills blocks were standard, the human block gets overwritten with skills directory content. **Root cause**: agent_manager.py:1893-1898 had `block = block` (no-op). When skills block doesn't exist, loop variable ends as last block in core_memory (often "human"), then updates that wrong block. **Fix**: Use `matched_block` variable to properly track found block. Now correctly raises NoResultFound when block label doesn't exist. **Impact**: Affects pre-December 2025 agents missing skills blocks. Written by Cameron ◯ Letta Code "The best error message is the one that never shows up." - Thomas Fuchs Co-Authored-By: Letta * fix: use correct method name in block update test Change get_block_by_label_async to get_block_with_label_async in test. Written by Cameron ◯ Letta Code Co-Authored-By: Letta --------- Co-authored-by: Letta --- letta/services/agent_manager.py | 14 ++-- .../test_agent_manager_block_update.py | 73 +++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) create mode 100644 tests/managers/test_agent_manager_block_update.py diff --git a/letta/services/agent_manager.py b/letta/services/agent_manager.py index 16648779..58da6915 100644 --- a/letta/services/agent_manager.py +++ b/letta/services/agent_manager.py @@ -1890,25 +1890,25 @@ class AgentManager: ) -> PydanticBlock: """Gets a block attached to an agent by its label.""" async with db_registry.async_session() as session: - block = None + matched_block = None agent = await AgentModel.read_async(db_session=session, identifier=agent_id, actor=actor) for block in agent.core_memory: if block.label == block_label: - block = block + matched_block = block break - if not block: + if not matched_block: raise NoResultFound(f"No block with label '{block_label}' found for agent '{agent_id}'") update_data = block_update.model_dump(to_orm=True, exclude_unset=True, exclude_none=True) # Validate limit constraints before updating - validate_block_limit_constraint(update_data, block) + validate_block_limit_constraint(update_data, matched_block) for key, value in update_data.items(): - setattr(block, key, value) + setattr(matched_block, key, value) - await block.update_async(session, actor=actor) - return block.to_pydantic() + await matched_block.update_async(session, actor=actor) + return matched_block.to_pydantic() @enforce_types @raise_on_invalid_id(param_name="agent_id", expected_prefix=PrimitiveType.AGENT) diff --git a/tests/managers/test_agent_manager_block_update.py b/tests/managers/test_agent_manager_block_update.py new file mode 100644 index 00000000..9e5fde22 --- /dev/null +++ b/tests/managers/test_agent_manager_block_update.py @@ -0,0 +1,73 @@ +import pytest + +# Import shared fixtures and constants from conftest +from conftest import DEFAULT_EMBEDDING_CONFIG + +from letta.orm.errors import NoResultFound +from letta.schemas.agent import CreateAgent +from letta.schemas.block import Block as PydanticBlock, BlockUpdate +from letta.schemas.llm_config import LLMConfig +from letta.server.server import SyncServer +from letta.services.block_manager import BlockManager + + +@pytest.mark.asyncio +async def test_modify_nonexistent_block_raises_error(server: SyncServer, default_user): + """ + Test that modifying a non-existent block raises NoResultFound instead of + silently updating the wrong block. + + Regression test for bug where `block = block` was a no-op, causing the loop + variable to end as the last block in core_memory, which then got incorrectly updated. + """ + # Upsert base tools + await server.tool_manager.upsert_base_tools_async(actor=default_user) + + # Create human and persona blocks + block_manager = BlockManager() + human_block = await block_manager.create_or_update_block_async( + PydanticBlock(label="human", value="Test user context", limit=2000), actor=default_user + ) + persona_block = await block_manager.create_or_update_block_async( + PydanticBlock(label="persona", value="Test persona context", limit=2000), actor=default_user + ) + + # Create agent with human and persona blocks (but no "skills" block) + create_agent_request = CreateAgent( + name="test_block_update_agent", + agent_type="memgpt_v2_agent", + system="test system", + llm_config=LLMConfig.default_config("gpt-4o-mini"), + embedding_config=DEFAULT_EMBEDDING_CONFIG, + block_ids=[human_block.id, persona_block.id], + ) + agent = await server.agent_manager.create_agent_async(create_agent_request, actor=default_user) + + # Try to update a non-existent block (e.g., "skills") + # This should raise NoResultFound, not silently update human block + with pytest.raises(NoResultFound, match="No block with label 'skills' found"): + await server.agent_manager.modify_block_by_label_async( + agent_id=agent.id, + block_label="skills", + block_update=BlockUpdate(value="Skills directory content that should not overwrite human block"), + actor=default_user, + ) + + # Verify human block wasn't modified + retrieved_human_block = await server.agent_manager.get_block_with_label_async( + agent_id=agent.id, + block_label="human", + actor=default_user, + ) + assert retrieved_human_block.value == "Test user context", "Human block should not be modified" + + # Verify persona block wasn't modified + retrieved_persona_block = await server.agent_manager.get_block_with_label_async( + agent_id=agent.id, + block_label="persona", + actor=default_user, + ) + assert retrieved_persona_block.value == "Test persona context", "Persona block should not be modified" + + # Clean up + await server.agent_manager.delete_agent_async(agent.id, default_user)