Skip to content

fix(feishu): preserve quoted reply ancestry - #66207

Open
hydraxman wants to merge 2 commits into
NousResearch:mainfrom
hydraxman:fix/feishu-reply-ancestry
Open

fix(feishu): preserve quoted reply ancestry#66207
hydraxman wants to merge 2 commits into
NousResearch:mainfrom
hydraxman:fix/feishu-reply-ancestry

Conversation

@hydraxman

@hydraxman hydraxman commented Jul 17, 2026

Copy link
Copy Markdown

What does this PR do?

Feishu quote replies currently fetch only the direct parent text. When that parent is itself a reply, short follow-ups such as “this one” or “agree” lose the original question that gives the parent its meaning.

This change follows the explicit parent_id / upper_message_id ancestry for quoted messages and supplies a bounded text chain through the existing reply_to_text field.

Related Issue

Fixes #67184.

This is intentionally separate from quote/topic routing in #36233 and replied-attachment handling in #54570.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • plugins/platforms/feishu/adapter.py
    • Follow bounded quoted-message ancestry with the direct parent first.
    • Reuse a shared LRU message-item fetch path so text and future media context do not duplicate API calls.
    • Follow parent_id, upper_message_id, and root-only ancestry while retaining cycle protection.
    • Keep the complete reply context within the existing 500-character gateway budget.
    • Stop after six messages, on cycles, API errors, cross-chat ancestors, or explicit thread mismatches.
    • Use the adapter-owned blocking-call executor for Feishu SDK requests.
  • tests/gateway/test_feishu.py
    • Cover nested quotes, direct-parent priority, total budget, cycles, API degradation, chat boundaries, thread boundaries, and inbound propagation.

How to Test

  1. On unmodified main, run the new focused tests and observe that the ancestry method is missing and nested quotes expose only the direct parent.
  2. Run .venv/bin/python -m pytest tests/gateway/test_feishu.py -o 'addopts=' -q — 221 passed.
  3. Run .venv/bin/python -m pytest tests/gateway/test_reply_to_injection.py -o 'addopts=' -q — 6 passed.
  4. Run .venv/bin/ruff check . — passed.
  5. Run .venv/bin/python -m py_compile plugins/platforms/feishu/adapter.py tests/gateway/test_feishu.py and git diff --check — passed.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the complete pytest tests/ -q suite
  • I've added tests for my changes
  • I've tested on macOS with Python 3.12 and lark-oapi==1.6.8

Documentation & Housekeeping

  • Documentation update — N/A; no user-facing configuration or API changes
  • cli-config.yaml.example update — N/A
  • CONTRIBUTING.md / AGENTS.md update — N/A
  • Cross-platform impact considered; the change is platform-independent Python and Feishu API handling
  • Tool descriptions/schemas update — N/A

Screenshots / Logs

Not applicable. The behavior is covered by deterministic adapter and gateway regression tests.

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/feishu Feishu / Lark adapter P3 Low — cosmetic, nice to have sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #36233, #22935, and #37262 cover adjacent Feishu quote/thread and quoted-content handling. This PR's bounded multi-level ancestry retrieval is a distinct mechanism; consider consolidating the overlapping adapter changes.

@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

Verdict: Approved

Fix preserves multi-level quoted reply ancestry for Feishu by following parent_id/upper_message_id chains up to 6 levels deep, staying within the 500-character budget. The shared LRU message-item cache avoids duplicate API calls. Cycle protection and chat/thread boundary guards are appropriate. Comprehensive test coverage (265 lines). No security issues.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Feishu reply-context fix. Current main resolves one reply target and fetches only that message (plugins/platforms/feishu/adapter.py:3257-3263, 4173-4204); the bounded ancestry traversal therefore addresses a present gap. The PR keeps the existing reply_to_text transport, whose gateway injection is already capped at 500 characters (gateway/run.py:11026-11040), and uses the adapter-owned blocking executor through _fetch_message_item.

The related discussion was considered: #36233 concerns quote-versus-topic routing, while this change is limited to reply-context ancestry. The PR base versions of both changed files match current main, so the change should salvage mechanically without adapting to later main changes.

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 18, 2026
@hydraxman
hydraxman force-pushed the fix/feishu-reply-ancestry branch from bc9506c to 9fcb9f0 Compare July 30, 2026 23:12
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Two open PRs address #67184 by adding bounded Feishu quoted-reply ancestry traversal through the existing reply_to_text path. Both follow parent/root links with depth, size, cycle, lookup, chat, and thread guards, but #67222 retains an Optional-text slicing defect that #66207 avoids.

Related pull requests

  • fix(feishu): preserve quoted reply ancestry #66207 best fix — (+390/-11) — n/a: Adds direct-parent-first ancestry traversal capped at six messages and 500 characters, shared message-item caching, boundary and failure guards, and focused regression coverage; rendering is guarded by if text, avoiding failure on non-textual ancestors. The maintainer-bot keep_open verdict identifies this as a present gap with high salvageability, and the contributor discussion treats the mechanism as distinct while requesting consolidation of overlapping adapter work.
  • fix(feishu): preserve quoted reply ancestry (#67184) #67222 duplicate — (+374/-11) — n/a: Implements substantially the same bounded ancestry and caching mechanism, but slices the Optional extraction result without a null guard, so an empty or unknown quoted item can raise TypeError rather than continue to a textual ancestor. Despite its contributor keep_open review, that same review documents this defect and explicitly recommends consolidation with fix(feishu): preserve quoted reply ancestry #66207; another contributor also identifies fix(feishu): preserve quoted reply ancestry (#67184) #67222 as its duplicate.

Duplicates

#67222 is substantially a duplicate of #66207: both modify the same adapter and test paths for the same bounded Feishu ancestry, cache, and reply-context mechanism, while #66207 has the safer rendering path and cache-reuse regression.

Suggested consolidation

Keep #66207 open with a salvage path: retain its null-safe bounded traversal, shared cache, and regression coverage, consistent with the maintainer-bot keep_open verdict and the recorded best-fix selection. Close #67222 as a duplicate of #66207 because its overlapping implementation retains the contributor-reviewed Optional-text slicing defect.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I67184(["issue #67184 (open)"])
    subgraph Dup66207 ["PRs duplicating each other"]
        P66207["PR #66207 (open)"]
        P67222["PR #67222 (open)"]
    end
    P66207 -->|best fix| I67184
    class I67184 open
    class P66207 open
    class P67222 open
    class P66207 best
    class P66207 target
    click I67184 "https://github.com/NousResearch/hermes-agent/issues/67184"
    click P66207 "https://github.com/NousResearch/hermes-agent/pull/66207"
    click P67222 "https://github.com/NousResearch/hermes-agent/pull/67222"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 38 kB of PR diffs, 8 kB of issue/PR text, 2 kB of discussion (5 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have 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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feishu quoted replies lose nested message ancestry

6 participants