Skip to content

fix(gateway): skip background memory/skill review in group chats - #26850

Open
irandoku wants to merge 1 commit into
NousResearch:mainfrom
irandoku:fix/gateway-group-chat-memory-review
Open

fix(gateway): skip background memory/skill review in group chats#26850
irandoku wants to merge 1 commit into
NousResearch:mainfrom
irandoku:fix/gateway-group-chat-memory-review

Conversation

@irandoku

Copy link
Copy Markdown

Problem

Built-in memory (MEMORY.md + USER.md + fact_store) is designed for 1-on-1 conversations. In group chats, any member's message can trigger background review, causing identity pollution (e.g., a group member's self-introduction incorrectly written to USER.md as if it were the owner's identity).

External memory plugins (Honcho, Supermemory) handle per-user scoping, but built-in memory lacks this isolation. Since many users rely on built-in memory without external plugins, group chats pose a real privacy/identity risk.

Solution

Add chat_type guard to both memory and skill background review triggers. When chat_type is "group" or "channel", skip automatic background review entirely.

DMs ("dm") and CLI sessions continue to review normally.

Scope

  • run_agent.py: _should_review_memory (~line 12279)
  • run_agent.py: _should_review_skills (~line 15932)

No changes to:

  • Manual memory/skill tool calls (user-initiated writes still work)
  • External memory plugins
  • Cron jobs or CLI mode

Testing

  • Verified chat_type values across platforms: Telegram ("dm"/"group"/"channel"/"forum"), LINE ("dm"/"group"/"channel"), Discord, Matrix, Signal all use consistent taxonomy.
  • Local test: group messages no longer trigger background review threads.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers tool/skills Skills system (list, view, manage) labels May 16, 2026
@irandoku
irandoku force-pushed the fix/gateway-group-chat-memory-review branch from bc47a80 to d696282 Compare May 16, 2026 11:03

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying a real group-chat safety issue. Current main still schedules automatic review without a chat-type check: memory review is triggered in agent/turn_context.py:306-314, and skill review in both agent/turn_finalizer.py:485-490 and agent/codex_runtime.py:489-496.

Problems

  • The run_agent.py locations in this PR were moved by 54870847c; the patch must cover all three current trigger paths to establish the stated guarantee.
  • group/channel is not the full multi-user taxonomy: LINE emits room at plugins/platforms/line/adapter.py:410-411, and Discord emits thread at plugins/platforms/discord/adapter.py:4716.
  • The LINE display-name and message-type changes are unrelated. The display-name helper hardcodes the default Hermes home, conflicting with profile isolation; the current LINE mapping test requires audio to remain MessageType.VOICE (tests/gateway/test_line_plugin.py:662-672).

Suggested changes

  • Re-scope to a shared multi-user predicate at the current review-trigger sites and add regression coverage for group-like and DM paths.

Automated hermes-sweeper review.

Comment thread run_agent.py Outdated
@@ -12279,7 +12279,8 @@ def run_conversation(
_should_review_memory = False
if (self._memory_nudge_interval > 0
and "memory" in self.valid_tool_names
and self._memory_store):
and self._memory_store

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guard is now stale: current main decides memory review in agent/turn_context.py:306-314, and skill review separately in agent/turn_finalizer.py:485-490 plus agent/codex_runtime.py:489-496. Please rework the fix around all active trigger paths rather than this removed inline location.

Comment thread plugins/platforms/line/adapter.py Outdated
@@ -424,6 +424,24 @@ def _allowed_for_source(
return False


def _resolve_display_names(chat_id: str, user_id: str) -> Tuple[str, str]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unrelated helper hardcodes the default ~/.hermes path. That bypasses profile isolation; persistent Hermes state must use get_hermes_home() as required by AGENTS.md:1169-1177. Please remove this unrelated change from the review-trigger fix or make it profile-safe in a separately tested PR.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@irandoku

Copy link
Copy Markdown
Author

Thanks for the careful review. I agree that the issue is still valid, and your comments are correct.

I am rechecking the change against current main, since the active review paths have moved out of run_agent.py into agent/turn_context.py, agent/turn_finalizer.py, and the Codex runtime path. I also confirmed that the current conversation surface includes additional non-DM types such as room, thread, and forum, so the old group/channel check is incomplete.

I also found unrelated LINE display-name and message-type changes in the current diff. Those do not belong to this fix and will be removed.

I’m rebuilding the change from current main with focused coverage, while preserving explicit/manual memory and skill operations and leaving external memory synchronization unchanged. I’ll follow up with an updated implementation direction after completing that comparison.

Gate automatic memory and skill review at the shared scheduler boundary while preserving CLI/direct contexts and external memory synchronization.
@irandoku
irandoku force-pushed the fix/gateway-group-chat-memory-review branch from d696282 to 84ff3a5 Compare July 13, 2026 16:46
@irandoku

Copy link
Copy Markdown
Author

Implementation update

This PR has been reworked on current main to match the background-review
architecture introduced since the PR was originally opened. The implementation
branch was rebuilt from af250d849.

Analysis and design rationale

On current main, automatic memory and skill review can be triggered from
multiple standard and Codex runtime paths. These paths maintain their own
review counters and trigger conditions, but they all converge on the same
scheduler:

standard / Codex trigger paths
    -> AIAgent._spawn_background_review()
        -> create the background-review thread

Applying chat-type checks independently at every trigger site would duplicate
the policy across several runtime paths and make future divergence likely. It
would also unnecessarily couple the access policy to counter bookkeeping.

The implementation therefore places one guard at
AIAgent._spawn_background_review(), immediately before thread creation. This
is the narrowest shared boundary that prevents the automatic side effect while
leaving the existing trigger and counter semantics unchanged.

This boundary was also selected because:

  • Both the standard and Codex automatic-review paths converge there.
  • Explicit memory and skill tool calls do not use this scheduler and remain
    unaffected.
  • External memory synchronization occurs before automatic-review scheduling,
    so suppressing the scheduler does not suppress synchronization.
  • Cache-parity setup and the review fork's runtime tool restrictions remain
    downstream of the guard and unchanged for allowed contexts.
  • No gateway, session, or platform-adapter dependency needs to be introduced
    into the agent layer.

The policy uses an allowlist for contexts where owner-scoped automatic review is
appropriate:

  • Explicit None or an empty value preserves existing CLI/non-gateway
    behavior.
  • dm, direct, and private allow direct-chat callers.
  • Group-like and unknown non-empty values are denied.
  • Malformed falsy values and a missing _chat_type attribute fail closed.

Gateway-created agents pass their normalized SessionSource.chat_type
explicitly, and chat type is included in the gateway session key. The
missing-attribute case is therefore treated separately from the intentional
CLI None value instead of allowing both through the same fallback.

What changed

  • Added a small background_review_allowed(chat_type) policy helper in
    agent/background_review.py.
  • Applied the policy at AIAgent._spawn_background_review(), before
    background-thread creation.
  • Added allow/deny taxonomy coverage, including malformed values and a missing
    _chat_type attribute.
  • Added Codex integration coverage proving that group contexts do not create a
    review thread while external memory synchronization still runs.
  • Updated existing synthetic review-agent fixtures to declare their intended
    CLI context explicitly with _chat_type=None.

Preserved behavior

  • Explicit memory and skill tool calls are unchanged.
  • External memory synchronization is unchanged.
  • Existing review counters and trigger thresholds are unchanged.
  • Cache-parity behavior and runtime tool restrictions are unchanged.
  • No gateway session or platform-adapter changes are included.

Verification

  • 56 targeted background-review, missing-context, cache-parity,
    tool-restriction, and turn-context tests passed.
  • 5 directly related Codex integration contracts passed, covering:
    • external memory synchronization;
    • review-threshold triggering;
    • scheduler call signature;
    • group-context suppression;
    • review-runtime downgrade behavior.
  • py_compile, targeted ruff, and git diff --check passed.

The complete Codex integration module exceeded the local 600-second runner
limit, so the result above intentionally reports only the five directly
affected contracts that completed successfully.

@teknium1 teknium1 added the area/memory Memory subsystem: store, providers, sync, background reviews label Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/memory Memory subsystem: store, providers, sync, background reviews comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/memory Memory tool and memory providers tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants