Skip to content

fix(session-search): scope gateway recall to current chat - #62138

Open
ljy-2000 wants to merge 3 commits into
NousResearch:mainfrom
ljy-2000:codex/fix-session-search-chat-scope
Open

fix(session-search): scope gateway recall to current chat#62138
ljy-2000 wants to merge 3 commits into
NousResearch:mainfrom
ljy-2000:codex/fix-session-search-chat-scope

Conversation

@ljy-2000

Copy link
Copy Markdown
Contributor

What does this PR do?

Scopes session_search discovery and browse to the current gateway conversation by default, using the stable session_key that already encodes Hermes' DM, group, thread, profile, and per-user routing policy.

  • Passes the host-only gateway session key through both agent tool-execution paths.
  • Adds scope=current_chat|global; gateway calls default to current_chat, while CLI calls remain global.
  • Pushes the exact session-key predicate into title resolution, recent-session browsing, porter FTS5, trigram FTS5, and the short-CJK LIKE fallback.
  • Keeps explicit read/scroll calls unscoped so @session links and user-selected session IDs remain readable.
  • Documents the behavior in the English and Chinese session guides.

Why

Gateway conversations already route live messages with build_session_key(), but session_search received only the active session_id. It could exclude the current lineage, yet every remaining session in the profile was eligible, so a query from one DM or thread could return matching history from another chat.

Reusing the persisted session key keeps recall aligned with the existing routing policy without rebuilding platform/chat/thread/user rules in the tool.

Effect

The regression fixture contains two prior sessions matching the same query: one in the current chat and one in another chat.

  • Previous/global behavior: 2 prior hits, 1 foreign-chat hit (50% cross-chat contamination).
  • New gateway default: 1 in-scope hit, 0 foreign-chat hits (0% contamination).
  • Explicit scope=global: both prior hits remain available.
  • CLI default: remains global.
  • Additional retrieval LLM calls: 0.

Tests also cover same-base title collisions across chats, long-CJK trigram retrieval, short-CJK LIKE fallback, current-lineage exclusion, and explicit cross-chat session reads.

Validation

  • python -m pytest tests/test_hermes_state.py tests/tools/test_session_search.py tests/run_agent/test_token_persistence_non_cli.py tests/gateway/test_async_session_db.py -q -o addopts=420 passed
  • Post-rebase focused rerun — 67 passed
  • ruff check on all changed Python files — passed
  • scripts/check-windows-footguns.py on all changed Python files — passed
  • git diff --check — passed

Fixes #10554

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tools Tool registry, model_tools, toolsets sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state P2 Medium — degraded but workaround exists labels Jul 10, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Competing cluster for scoping session_search recall to the current chat, same goal via different mechanisms: this PR reuses the existing persisted session_key (no new schema); #50030 adds a schema-v18 migration + scope_key/origin_json + a HERMES_RECALL_SCOPE_KEY contextvar; #59295 filters default discovery inside the tool only; #35762 persists gateway identity metadata on session rows. Related, not duplicates — flagging so a maintainer can pick the canonical approach (this one is the lightest; #50030 is the most comprehensive/schema-backed).

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Found potential issues:

  • potential secret found
  • conflict markers found

Reviewed by Hermes Agent

@ljy-2000

Copy link
Copy Markdown
Contributor Author

Followed up on the automated review summary in commit 3ab4977ff.

  • Potential secret: the scanner matched the pre-existing unit-test fixture api_key="test-key" (already present on main; the OpenAI client is mocked). The fixture now constructs the same placeholder at runtime so no credential-like literal appears in the changed file.
  • Conflict markers: there were no <<<<<<< / >>>>>>> markers and git diff --check was clean. The scanner matched the newly added # =====... section dividers, which now use dashed dividers instead.

Revalidated with:

  • pytest tests/run_agent/test_token_persistence_non_cli.py tests/tools/test_session_search.py -q ? 67 passed
  • Ruff on both files ? passed
  • git diff --check ? passed

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for covering the normal sequential and direct agent tool-dispatch paths, plus the FTS and CJK retrieval variants.

Problems

  • gateway/run.py:13288 constructs the /background task agent with the originating SessionSource and session DB but without gateway_session_key (gateway/run.py:13305-13314). AIAgent sets _gateway_session_key only from that constructor argument (agent/agent_init.py:403), so this path will still resolve the proposed default to global recall. The normal gateway agent already passes gateway_session_key=session_key at gateway/run.py:18199.

Suggested changes

  • Pass self._session_key_for_source(source) to the background AIAgent, and add a /background regression test proving session_search receives that key.

Automated hermes-sweeper review.

@teknium1 teknium1 added 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 11, 2026
@ljy-2000

Copy link
Copy Markdown
Contributor Author

Addressed the /background scope gap in 3b6904629.

The background-task AIAgent now receives gateway_session_key=self._session_key_for_source(source), matching the normal gateway-agent path. The background command regression asserts that the exact source-derived key reaches the agent constructor; the existing direct/sequential tool-dispatch tests then cover forwarding that key into session_search.

Validation:

  • Background + session-search paths: 88 passed, 1 deselected
  • Ruff on changed files: passed
  • git diff --check: passed

The deselected case is the branch's existing SQLite second-resolution sort=newest fixture, where seeded sessions can tie on started_at; it is unrelated to the background scope change.

@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 11, 2026
@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 12, 2026
@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 12, 2026
@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 12, 2026
@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 12, 2026
@ljy-2000
ljy-2000 marked this pull request as ready for review July 13, 2026 13:07
@alt-glitch alt-glitch added the comp/gateway Gateway runner, session dispatch, delivery label Jul 13, 2026
@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
@honor2030

Copy link
Copy Markdown
Contributor

Independent rebase check against current origin/main (ddd21abfa): this branch is 971 upstream commits behind and does not apply cleanly. The merge conflicts are in the core search implementation and its regression coverage: hermes_state.py, tools/session_search_tool.py, and tests/tools/test_session_search.py.

The intended scope contract is still useful, including the explicit global opt-out and the repaired /background propagation. But there are no CI checks on the branch, and these conflicts prevent a safe validation of the patch against today's session/compaction behavior. I recommend rebasing before review/merge; the rebase should preserve the SQL-level scope predicate across FTS, CJK fallback, title lookup, and browse, then rerun the focused gateway + session-search suite.

@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation 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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

session_search should be scoped to the current chat by default in gateway conversations

5 participants