fix(hermes): normalize Telegram send_message pseudo-call leaks#4175
Conversation
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDetects and extracts intended message bodies from raw ChangesMessaging Platform Response Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
This is an automated advisory review. A human maintainer must make the final merge decision. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/hermes/plugin/__init__.py`:
- Around line 1108-1111: The broad except Exception around the dynamic import of
"run_agent" (the __import__("run_agent", fromlist=["AIAgent"]) call) should be
narrowed to only catch import-related errors so other failures surface; change
the handler to catch ImportError and ModuleNotFoundError (and return False
there) instead of catching Exception so AttributeError or other runtime errors
during import are not swallowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d6dbc18b-0cb7-482b-83ca-c4d54be0aece
📒 Files selected for processing (2)
agents/hermes/plugin/__init__.pytest/hermes-plugin-handlers.test.ts
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 26434471925
|
Selective E2E Results — ✅ All requested jobs passedRun: 26459374275
|
cv
left a comment
There was a problem hiding this comment.
Requesting changes based on the PR Review Advisor rubric.
The global raw send_message normalizer can misroute cross-platform pseudo-calls. _RAW_MESSAGING_TARGET_RE accepts many platforms and _install_messaging_response_patch() installs the normalization globally on AIAgent._strip_think_blocks() without knowing the current chat platform, first-turn state, or whether the user explicitly asked to send a separate cross-platform message. A malformed response like send_message: "to slack: ..." from a Telegram-origin chat could be stripped to plain text and delivered back to Telegram instead of remaining a tool-dispatch/error case.
Please restrict the fallback to the current platform/current-chat readiness case (ideally with platform/turn context), and add a negative test proving a non-current-platform target is not silently normalized into the current chat. Also document the source-of-truth rationale for the workaround and add/cite runtime validation through the Hermes gateway first-message path.
…4175) Addresses the CHANGES_REQUESTED review by @cv on PR #4175. The original patch installed `_normalize_raw_messaging_tool_response` globally on `AIAgent._strip_think_blocks` with no current-platform context, so a stray response like `send_message: "to slack: ..."` emitted in a Telegram chat would be silently normalized to plain text and delivered back to Telegram — misrouting the cross-platform send_message intent into the wrong chat. This commit: - Adds `_current_messaging_platform` module state + `_set_/_get_` helpers, validated against the `_MESSAGING_PLATFORMS` allowlist. - Extends `_normalize_raw_messaging_tool_response` to take an optional `current_platform` argument and leave the raw `send_message:` text intact whenever the target platform doesn't match the current chat or the current platform is unknown — so dispatch / error paths still surface upstream rather than getting silently delivered into the wrong chat. - Wires the patched `_strip_think_blocks` closure to read `_get_current_messaging_platform()`. - Updates `_pre_llm_call` to call `_set_current_messaging_platform` on every turn (not gated on context injection), so the normalizer has a current-platform anchor even on non-first / non-grounding turns. - Expands the docstring to call out (a) `agents/hermes/run_agent.py` as source of truth for send_message routing, (b) this normalizer as defense-in-depth output filtering, (c) `hermes-e2e`, `hermes-discord-e2e`, and `hermes-slack-e2e` as runtime validation through the gateway first-message path. - Adds negative-test coverage in `test/hermes-plugin-handlers.test.ts` for the cross-platform-blocked and unknown-platform-blocked cases plus positive coverage of the same-platform normalization through the class patch. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Chengjie Wang <chengjiew@nvidia.com>
…m anchor Addresses @cv's review ask #4 on PR #4175 — runtime validation through the Hermes gateway first-message path — by adding an integration-style test that drives the actual `_pre_llm_call` → `AIAgent._strip_think_blocks` chain Hermes uses, rather than only exercising the normalizer in isolation. The new test: - Stubs `run_agent.AIAgent._strip_think_blocks` as a real method. - Calls `_pre_llm_call(platform="telegram", is_first_turn=True)` to simulate the first Telegram turn arriving via the gateway hook — this both sets the platform anchor and installs the patch via the same code path Hermes uses at runtime. - Calls `AIAgent._strip_think_blocks(...)` twice to assert (a) same-platform body is extracted, (b) cross-platform `to slack: ...` is preserved. - Calls `_pre_llm_call(platform="discord", is_first_turn=True)` to simulate a turn on a different platform and asserts the anchor refreshes per-turn (Discord body extracted, telegram target preserved). Combined with the existing isolated-unit cases and the cited `hermes-e2e` / `hermes-discord-e2e` / `hermes-slack-e2e` scenarios, this gives both "added" and "cited" runtime validation for the gateway first-message path. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Chengjie Wang <chengjiew@nvidia.com>
|
@cv The normalizer is now anchored to the current chat: Your The #3893 readiness-gate suggestion is a different architectural approach to the same symptom — left out of scope here. Happy to file a follow-up if you'd rather pursue it. Re-requesting review. |
Summary
Fixes the NemoHermes first Telegram reply path so a raw
send_message: "to telegram: ..."pseudo-tool response is not delivered to the user as plain text.The PR adds both a first-turn prompt guard for messaging platforms and a narrow final-response normalizer for targeted raw messaging pseudo-calls.
Related Issue
Fixes #3893
Changes
send_message: "to telegram: Hello"into the actual message body before gateway delivery.send_messagefor explicit cross-platform delivery requests.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Additional verification run locally:
npm test -- test/hermes-plugin-handlers.test.ts test/generate-hermes-config.test.tspython3 -m py_compile agents/hermes/plugin/__init__.pynpm run build:cligit diff --check HEAD~1..HEADNote: local
pre-commitwas not used for the final commit because its CLI coverage hook recursively triggered hooks through a temporary Git fixture. The targeted checks above were run explicitly after rebasing onto latestorigin/main.Signed-off-by: Chengjie Wang chengjiew@nvidia.com
Summary by CodeRabbit
Bug Fixes
Improvements
Tests