Skip to content

fix: ignore tool-call items when collecting user senders - #284

Closed
rendigua2025-gif wants to merge 1 commit into
EverMind-AI:mainfrom
rendigua2025-gif:codex/fix-toolcall-senders
Closed

rendigua2025-gif wants to merge 1 commit into
EverMind-AI:mainfrom
rendigua2025-gif:codex/fix-toolcall-senders

Conversation

@rendigua2025-gif

Copy link
Copy Markdown

Summary

Fixes #276.

User-side extraction strategies assumed every item in memcell.items exposes a role attribute. Agent/tool-call trajectories can include ToolCallRequest items, which do not expose role, so extract_atomic_facts and extract_foresight could crash before processing otherwise valid user messages.

This PR adds a small shared helper for collecting user sender ids defensively, then uses it in both strategies.

Scope

  • Collect sender ids only from items where role == "user".
  • Skip non-chat/tool-call items instead of treating them as malformed messages.
  • Preserve the existing extraction behavior for normal chat messages.

This does not add tool-call semantics to user-side extraction; it only prevents unrelated tool-call items from crashing the user memory pipeline.

Tests

  • tests/unit/test_memory/test_strategies/test_strategy_to_handler_contract.py
  • tests/unit/test_memory/test_strategies/test_extract_atomic_facts.py
  • tests/unit/test_memory/test_strategies/test_extract_foresight.py

Note: I verified locally on Windows with a test-process-only fcntl stub because the current repository still imports POSIX fcntl during test collection on Windows.

@cyfyifanchen

Copy link
Copy Markdown
Collaborator

Thanks for catching this and putting together the defensive sender helper. The affected paths have since changed: atomic-fact extraction now receives a single-owner episode event, and foresight filters for chat messages before reading role.

The foresight fix and mixed tool-call regression coverage landed with #393 in v1.2.3: #393

That covers the crash this PR addresses, so I'm closing the older patch as superseded, along with #276. We appreciate the work you put into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

User-side extraction strategies crash on ToolCallRequest because they assume every memcell item has role

2 participants