Skip to content

refactor(feishu-comment): use SessionStore, sentinel doc-read, redacted logs - #13045

Open
liujinkun2025 wants to merge 1 commit into
NousResearch:mainfrom
liujinkun2025:feat/feishu-document-comment-v2
Open

refactor(feishu-comment): use SessionStore, sentinel doc-read, redacted logs#13045
liujinkun2025 wants to merge 1 commit into
NousResearch:mainfrom
liujinkun2025:feat/feishu-document-comment-v2

Conversation

@liujinkun2025

Copy link
Copy Markdown
Contributor

Summary

Reworks the feishu document-comment integration (#11898) to align with
hermes's core session architecture and close review-flagged concerns
around session bloat, tool scoping, and log leakage.

Session management

  • Replace the in-memory _session_cache dict (1h hard TTL, no
    persistence) with hermes's generic SessionStore pipeline
    (SessionSource + SessionDB). Sessions now persist through gateway
    restart and auto-reset on daily/idle policy, matching IM.
  • Local comments key on comment_id (per-thread isolation).
  • Whole-doc comments collapse onto a __whole_doc__ sentinel thread_id
    so all whole-document comments on the same doc share one document-level
    session.
  • Persist only the user's actual comment text + optional quote anchor,
    not the rendered prompt (~50x smaller per user row).

Tool scoping

  • Delete tools/feishu_doc_tool.py and tools/feishu_drive_tool.py.
    These are feishu-specific document/comment operations, not cross-cutting
    agent capabilities — yet they lived in the global tool registry
    alongside core tools like memory, session_search, and
    send_message. The global registry should host tools any agent on any
    platform legitimately uses; feishu-scoped surfaces belong inside the
    feishu handler, not next to memory/session_search. Pulling these out
    closes that scope mismatch.
  • Document content now flows through a two-pass <NEED_DOC_READ> sentinel
    protocol owned by the comment handler: the agent lists the tokens it
    needs, business code fetches them against a whitelist (source doc +
    comment-referenced docs, docx only), and the agent responds on the
    second turn.
  • Drop the thread-local client injection; the lark client is built from
    config at call time.

Log hardening

  • Strip user comment text, quote text, agent response text, and full
    prompts from all log statements. ~/.hermes/logs/agent.log now stores
    only identifiers, lengths, and status codes — closes the multi-operator
    leak risk where operators sharing a hermes instance could read each
    other's document content through log files.

Adapter wiring

  • handle_drive_comment_event now takes the FeishuAdapter instance
    instead of the bare client so it can reach the gateway-injected
    SessionStore. The change stays fully off the IM message path.

Test plan

  • New unit tests for session-source construction, sentinel parsing,
    whitelist enforcement, doc-content truncation, history persistence,
    compact user-turn rendering, and error-path degradation
  • Full tests/gateway/ regression passes
  • Live smoke test:
    • Local comment thread creates a session keyed by comment_id
    • Whole-doc comments share one __whole_doc__ session across users
    • History loads (history=N grows turn-by-turn)
    • <NEED_DOC_READ> sentinel triggers parallel fetch of source +
      wiki-resolved referenced docs
    • Long replies auto-chunk to fit feishu's per-comment length limit
    • SessionDB rows show compact [Quoted] … payloads, not full prompts

@liujinkun2025
liujinkun2025 force-pushed the feat/feishu-document-comment-v2 branch 7 times, most recently from 7c26fd0 to 1e19289 Compare April 21, 2026 02:29
@liujinkun2025
liujinkun2025 marked this pull request as draft April 21, 2026 03:28
@liujinkun2025
liujinkun2025 marked this pull request as ready for review April 21, 2026 04:05
@liujinkun2025
liujinkun2025 force-pushed the feat/feishu-document-comment-v2 branch from 1e19289 to 5f838ac Compare April 21, 2026 04:20
…ed logs

Reworks the feishu document-comment integration (introduced in NousResearch#11898) to
align with hermes's core session architecture and to close several session
bloat, tool scoping, and log leakage concerns flagged in review.

Session management
  - Replace the in-memory _session_cache dict (1h hard TTL, pure memory)
    with hermes's generic SessionStore pipeline (SessionSource + SessionDB).
    Sessions now persist through gateway restart and auto-reset on daily /
    idle policy, matching IM.
  - Local comments key on comment_id (per-thread isolation).
  - Whole-doc comments collapse onto a __whole_doc__ sentinel thread_id so
    all whole-document comments on the same doc share one document-level
    session.
  - Persist only the user's actual comment text + optional quote anchor,
    not the rendered prompt (~50x smaller per user row).

Tool scoping
  - Delete tools/feishu_doc_tool.py and tools/feishu_drive_tool.py. These
    are feishu-specific document / comment operations, not cross-cutting
    agent capabilities -- yet they lived in the global tool registry
    alongside core tools like memory, session_search, and send_message.
    The global registry should host tools any agent on any platform
    legitimately uses; feishu-scoped surfaces belong inside the feishu
    handler, not next to memory / session_search.
  - Document content now flows through a two-pass <NEED_DOC_READ> sentinel
    protocol owned by the comment handler: the agent lists tokens it
    needs, business code fetches them against a whitelist (source doc +
    comment-referenced docs, docx only), and the agent responds on the
    second turn.
  - Drop the thread-local client injection; the lark client is built from
    config at call time.

Log hardening
  - Strip user comment text, quote text, agent response text, and full
    prompts from all log statements. ~/.hermes/logs/agent.log now stores
    only identifiers, lengths, and status codes -- closes the multi-
    operator leak risk where operators sharing a hermes instance could
    read each other's document content through log files.

Adapter wiring
  - handle_drive_comment_event now takes the FeishuAdapter instance
    instead of the bare client so it can reach the gateway-injected
    SessionStore. The change stays fully off the IM message path.

Tests cover session source construction, sentinel parsing, whitelist
enforcement, doc-content truncation, history persistence, compact user-
turn rendering, and error-path degradation. Full tests/gateway/ regression
passes; live smoke verified local + whole-doc + multi-doc sentinel fetch
and cross-restart history replay.

Change-Id: Icc69e499f5db982973ccd9cc0b7b691f0e68ee74
@liujinkun2025
liujinkun2025 force-pushed the feat/feishu-document-comment-v2 branch from 5f838ac to d7d54bb Compare April 22, 2026 10:25
@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists type/refactor Code restructuring, no behavior change platform/feishu Feishu / Lark adapter comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets labels Apr 22, 2026
@liujinkun2025

Copy link
Copy Markdown
Contributor Author

@teknium1 — review when you get a chance.

test is red with the same 8 failures as
main@c6b1ef4e
(none in this
PR's diff). Other checks green.

V2 redesign of the Feishu document-comment handler (V1 was #11898). Replaces the two registry tools
(feishu_doc_tool.py + feishu_drive_tool.py, ~560 LoC) with a <NEED_DOC_READ> sentinel +
SessionStore-backed state, plus log redaction. 13 files, +1890 / -820.

@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 the thorough Feishu comment-handler redesign. The underlying problems are still present on current main: the active bundled handler retains its one-hour _session_cache (plugins/platforms/feishu/feishu_comment.py:1003-1044) and logs request bodies, raw failures, full prompts, and replies (plugins/platforms/feishu/feishu_comment.py:62-94, 1346-1360).

Problems

  • gateway/platforms/feishu_comment.py:1635 passes persist_session=False, but current AIAgent.__init__ no longer accepts that argument (run_agent.py:416-488). Commit 454d883e6977419854cf26138b93b118871d36d7 removed it. Transplanting this unchanged would make _build_comment_agent() fail; the surrounding broad exception path returns an empty response (gateway/platforms/feishu_comment.py:1767-1769).
  • The PR targets the pre-plugin Feishu paths. Current runtime wiring is plugins/platforms/feishu/adapter.py:2578-2594; commit 560010547 migrated the adapter family there.

Suggested changes

  • Port the handler, adapter wiring, and tests to plugins/platforms/feishu/.
  • Rework the no-full-prompt persistence guarantee using current AIAgent/session APIs, and cover the current adapter-to-SessionStore integration path.

Automated hermes-sweeper review.

# doc-content injection), so a tiny iteration budget is enough.
max_iterations=2,
enabled_toolsets=[],
persist_session=False,

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.

persist_session was removed from AIAgent.__init__ by current-main commit 454d883e6977419854cf26138b93b118871d36d7. After porting this stale-path PR to the bundled Feishu plugin, this argument would raise TypeError; _run_comment_agent catches that broadly and returns an empty response, so document comments would receive no reply. Please replace this dependency with a persistence strategy supported on current main.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 area/sessions Session lifecycle, resume, persistence, history labels Jul 12, 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/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists platform/feishu Feishu / Lark adapter 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-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants