refactor(matrix): remove delayed reaction redaction, run in background tasks (#22090) - #5
Closed
Phoenix1819 wants to merge 1 commit into
Closed
Conversation
🚨 CRITICAL Supply Chain Risk DetectedThis PR contains a pattern that has been used in real supply chain attacks. A maintainer must review the flagged code carefully before merging. 🚨 CRITICAL: Install-hook file added or modifiedThese files can execute code during package installation or interpreter startup. Files: Scanner only fires on high-signal indicators: .pth files, base64+exec/eval combos, subprocess with encoded commands, or install-hook files. Low-signal warnings were removed intentionally — if you're seeing this comment, the finding is worth inspecting. |
🔎 Lint report:
|
| Rule | Count |
|---|---|
invalid-argument-type |
50 |
unresolved-import |
34 |
invalid-method-override |
26 |
unresolved-attribute |
23 |
invalid-assignment |
7 |
unsupported-operator |
2 |
unused-type-ignore-comment |
1 |
invalid-type-form |
1 |
not-subscriptable |
1 |
unresolved-reference |
1 |
invalid-return-type |
1 |
First entries
gateway/platforms/whatsapp.py:963: [invalid-method-override] invalid-method-override: Invalid override of method `send_image_file`: Definition is incompatible with `BasePlatformAdapter.send_image_file`
tools/session_search_tool.py:375: [invalid-argument-type] invalid-argument-type: Argument to bound method `SessionDB.search_messages` is incorrect: Expected `list[str]`, found `None | list[str]`
hermes_cli/uninstall.py:328: [unresolved-attribute] unresolved-attribute: Module `winreg` has no member `QueryValueEx`
acp_adapter/session.py:624: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `list[str]`, found `str | list[str] | bool | Unknown`
tests/gateway/test_tts_media_routing.py:106: [unresolved-attribute] unresolved-attribute: Object of type `bound method _MediaRoutingAdapter.send_document(chat_id: str, file_path: str, caption: str | None = None, file_name: str | None = None, reply_to: str | None = None, metadata: dict[str, Any] | None = None, **kwargs) -> CoroutineType[Any, Any, SendResult]` has no attribute `assert_not_awaited`
plugins/platforms/teams/adapter.py:1055: [invalid-method-override] invalid-method-override: Invalid override of method `send_image_file`: Definition is incompatible with `BasePlatformAdapter.send_image_file`
tests/tools/test_file_tools.py:387: [invalid-argument-type] invalid-argument-type: Method `__getitem__` of type `Overload[(key: SupportsIndex | slice[SupportsIndex | None, SupportsIndex | None, SupportsIndex | None], /) -> LiteralString, (key: SupportsIndex | slice[SupportsIndex | None, SupportsIndex | None, SupportsIndex | None], /) -> str]` cannot be called with key of type `Literal["required"]` on object of type `str`
scripts/keystroke_diagnostic.py:36: [unresolved-import] unresolved-import: Cannot resolve imported module `prompt_toolkit`
tests/hermes_cli/test_apply_profile_override.py:18: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/tools/test_file_tools.py:379: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["REQUIRED when mode='replace'"]` and `Unknown | str | list[str] | bool`
acp_adapter/session.py:624: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `int | float | None`, found `str | list[str] | bool | Unknown`
tests/tools/test_session_search.py:327: [unresolved-attribute] unresolved-attribute: Unresolved attribute `SessionDB` on type `ModuleType`
tests/tools/test_delegate.py:245: [invalid-argument-type] invalid-argument-type: Argument to function `delegate_task` is incorrect: Expected `list[dict[str, Any]] | None`, found `str`
gateway/platforms/telegram.py:2284: [unresolved-attribute] unresolved-attribute: Attribute `PRIVATE` is not defined on `None` in union `Unknown | None`
tests/cli/test_cli_shift_enter_newline.py:12: [unresolved-import] unresolved-import: Cannot resolve imported module `prompt_toolkit.input.vt100_parser`
tests/gateway/test_telegram_thread_fallback.py:77: [unresolved-attribute] unresolved-attribute: Unresolved attribute `InputMediaPhoto` on type `ModuleType`
run_agent.py:12504: [invalid-argument-type] invalid-argument-type: Argument to bound method `SessionDB.update_token_counts` is incorrect: Expected `str`, found `str | Unknown | dict[Unknown | str, Unknown | str | dict[str, str]] | int | dict[Unknown, Unknown]`
tests/tools/test_file_tools.py:379: [invalid-argument-type] invalid-argument-type: Method `__getitem__` of type `Overload[(i: SupportsIndex, /) -> str, (s: slice[SupportsIndex | None, SupportsIndex | None, SupportsIndex | None], /) -> list[str]]` cannot be called with key of type `Literal["new_string"]` on object of type `list[str]`
acp_adapter/session.py:624: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `dict[str, Any]`, found `str | list[str] | bool | Unknown`
hermes_cli/uninstall.py:324: [unresolved-attribute] unresolved-attribute: Module `winreg` has no member `OpenKey`
tests/tools/test_delegate.py:269: [invalid-argument-type] invalid-argument-type: Argument to function `delegate_task` is incorrect: Expected `list[dict[str, Any]] | None`, found `Literal["[{\"goal\": \"bad}"]`
scripts/keystroke_diagnostic.py:37: [unresolved-import] unresolved-import: Cannot resolve imported module `prompt_toolkit.key_binding`
tests/hermes_cli/test_destructive_slash_confirm_gate.py:26: [invalid-argument-type] invalid-argument-type: Method `__getitem__` of type `Overload[(i: SupportsIndex, /) -> Unknown, (s: slice[SupportsIndex | None, SupportsIndex | None, SupportsIndex | None], /) -> list[Unknown]]` cannot be called with key of type `Literal["destructive_slash_confirm"]` on object of type `list[Unknown]`
acp_adapter/session.py:624: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `list[dict[str, Any]]`, found `str | list[str] | bool | Unknown`
plugins/platforms/google_chat/adapter.py:3236: [unresolved-import] unresolved-import: Cannot resolve imported module `aiohttp`
... and 122 more
✅ Fixed issues (39):
| Rule | Count |
|---|---|
invalid-argument-type |
12 |
unresolved-attribute |
12 |
unresolved-import |
6 |
invalid-assignment |
3 |
no-matching-overload |
2 |
unresolved-reference |
2 |
call-non-callable |
1 |
unsupported-operator |
1 |
First entries
acp_adapter/session.py:623: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `bool`, found `str | list[str] | bool`
plugins/platforms/google_chat/adapter.py:2946: [unresolved-import] unresolved-import: Module `hermes_cli.config` has no member `print_warning`
gateway/platforms/telegram.py:1258: [invalid-argument-type] invalid-argument-type: Argument to constructor `int.__new__` is incorrect: Expected `str | Buffer | SupportsInt | SupportsIndex | SupportsTrunc`, found `str | None`
gateway/platforms/base.py:2793: [invalid-assignment] invalid-assignment: Invalid subscript assignment with key of type `Literal["stop_event"]` and value of type `Event` on object of type `dict[str, dict[str, str] | None]`
plugins/platforms/google_chat/adapter.py:526: [unresolved-attribute] unresolved-attribute: Attribute `Credentials` is not defined on `None` in union `Unknown | None`
plugins/platforms/google_chat/adapter.py:782: [unresolved-attribute] unresolved-attribute: Attribute `SubscriberClient` is not defined on `None` in union `Unknown | None`
acp_adapter/session.py:609: [no-matching-overload] no-matching-overload: No overload of bound method `MutableMapping.update` matches arguments
plugins/platforms/google_chat/adapter.py:2684: [call-non-callable] call-non-callable: Object of type `None` is not callable
plugins/platforms/google_chat/adapter.py:2943: [unresolved-import] unresolved-import: Module `hermes_cli.config` has no member `prompt_yes_no`
plugins/platforms/google_chat/adapter.py:2942: [unresolved-import] unresolved-import: Module `hermes_cli.config` has no member `prompt`
agent/transports/types.py:65: [unresolved-reference] unresolved-reference: Name `Dict` used when not defined: Did you mean `dict`?
tests/gateway/test_tts_media_routing.py:86: [unresolved-attribute] unresolved-attribute: Object of type `bound method _MediaRoutingAdapter.send_voice(chat_id: str, audio_path: str, caption: str | None = None, reply_to: str | None = None, **kwargs) -> CoroutineType[Any, Any, SendResult]` has no attribute `assert_not_awaited`
gateway/platforms/base.py:2797: [invalid-argument-type] invalid-argument-type: Argument to bound method `BasePlatformAdapter._keep_typing` is incorrect: Expected `Event | None`, found `dict[str, str] | None`
acp_adapter/session.py:623: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `IterationBudget`, found `str | list[str] | bool`
plugins/platforms/google_chat/adapter.py:2944: [unresolved-import] unresolved-import: Module `hermes_cli.config` has no member `print_info`
acp_adapter/session.py:623: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `list[dict[str, Any]]`, found `str | list[str] | bool`
plugins/platforms/google_chat/adapter.py:789: [unresolved-attribute] unresolved-attribute: Attribute `NotFound` is not defined on `None` in union `Unknown | None`
tests/run_agent/test_concurrent_interrupt.py:192: [unsupported-operator] unsupported-operator: Operator `+=` is not supported between objects of type `None` and `Literal[1]`
plugins/platforms/google_chat/adapter.py:903: [unresolved-attribute] unresolved-attribute: Attribute `PermissionDenied` is not defined on `None` in union `Unknown | None`
tests/gateway/test_tts_media_routing.py:97: [invalid-assignment] invalid-assignment: Object of type `AsyncMock` is not assignable to attribute `send_document` of type `def send_document(self, chat_id: str, file_path: str, caption: str | None = None, file_name: str | None = None, reply_to: str | None = None, **kwargs) -> CoroutineType[Any, Any, SendResult]`
tui_gateway/entry.py:84: [no-matching-overload] no-matching-overload: No overload of bound method `dict.get` matches arguments
tools/environments/base.py:107: [unresolved-attribute] unresolved-attribute: Attribute `close` is not defined on `None` in union `IO[Unknown] | None`
acp_adapter/session.py:623: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `int | float`, found `str | list[str] | bool`
acp_adapter/session.py:623: [invalid-argument-type] invalid-argument-type: Argument to `AIAgent.__init__` is incorrect: Expected `int`, found `str | list[str] | bool`
gateway/platforms/base.py:2797: [invalid-argument-type] invalid-argument-type: Argument to bound method `BasePlatformAdapter._keep_typing` is incorrect: Expected `int | float`, found `dict[str, str] | None`
... and 14 more
Unchanged: 4056 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
Owner
Author
|
Design note on redaction timing: The previous 5-second delayed redaction task caused lifecycle bugs on disconnect — races between pending asyncio tasks and connection teardown. Moving redaction inline simplifies shutdown, and Matrix homeservers handle redactions idempotently. If you would prefer to keep a delay, I can switch to a fire-and-forget wrapper instead of the old task-set pattern. |
🚨 CRITICAL Supply Chain Risk DetectedThis PR contains a pattern that has been used in real supply chain attacks. A maintainer must review the flagged code carefully before merging. 🚨 CRITICAL: Install-hook file added or modifiedThese files can execute code during package installation or interpreter startup. Files: Scanner only fires on high-signal indicators: .pth files, base64+exec/eval combos, subprocess with encoded commands, or install-hook files. Low-signal warnings were removed intentionally — if you're seeing this comment, the finding is worth inspecting. |
…d tasks Reaction redactions previously slept for 5 seconds before executing. This blocked the processing loop unnecessarily. Now they fire immediately as background tasks, with proper cleanup on disconnect. Closes NousResearch#22090
Phoenix1819
force-pushed
the
phoenix/pr-22087-22090-matrix-presence-reactions
branch
from
May 10, 2026 04:52
1c8594f to
3d7c4e5
Compare
🚨 CRITICAL Supply Chain Risk DetectedThis PR contains a pattern that has been used in real supply chain attacks. A maintainer must review the flagged code carefully before merging. 🚨 CRITICAL: Install-hook file added or modifiedThese files can execute code during package installation or interpreter startup. Files: Scanner only fires on high-signal indicators: .pth files, base64+exec/eval combos, subprocess with encoded commands, or install-hook files. Low-signal warnings were removed intentionally — if you're seeing this comment, the finding is worth inspecting. |
Phoenix1819
changed the title
Phoenix1819
pushed a commit
that referenced
this pull request
May 14, 2026
…rch#25071) * tui: make URLs clickable + hover-highlight in any terminal Problem ------- URLs printed by `hermes --tui` were not clickable in basic macOS Terminal.app. Cmd+click did nothing, the cursor didn't change shape — like nothing was detected — even though arrow buttons and other Box onClick handlers worked fine. Root cause ---------- Two layers of dead plumbing: 1. `<Link>` only emitted the underlying `<ink-link>` (which carries the hyperlink metadata into the screen buffer) when `supportsHyperlinks()` said yes. On Apple_Terminal that's false, so the per-cell hyperlink field stayed empty, so `Ink.getHyperlinkAt()` had nothing to return on click. The visible underline was just decorative. 2. `Ink.openHyperlink()` calls `this.onHyperlinkClick?.(url)`, but `onHyperlinkClick` was never assigned anywhere in the codebase. The click pipeline (`App.tsx → onOpenHyperlink → Ink.openHyperlink`) ran but bailed silently on the optional chain. Bonus discovery: even when wired up, there was no hover affordance — terminal apps can't change the system mouse cursor, so users had no visual signal that a cell was clickable. Arrow buttons in the chrome worked because they had explicit `<Box onClick>` styling; inline link URLs didn't. Fix --- - `Link.tsx`: always emit `<ink-link>` regardless of terminal capability. The renderer's `wrapWithOsc8Link` already gates the actual OSC 8 escape on `supportsHyperlinks()` further down — so terminals that don't understand OSC 8 still don't see the escape, but the screen-buffer metadata (which the click dispatcher reads) is now populated everywhere. - `ink.tsx + root.ts`: add `onHyperlinkClick?: (url: string) => void` to `Options` / `RenderOptions`, wire it to the existing `Ink.onHyperlinkClick` field in the constructor. - `src/lib/openExternalUrl.ts`: small platform-aware opener using `child_process.spawn` with arg-array (no shell) — http(s) only, rejects `file:`, `javascript:`, `data:`, etc., so a hostile model can't trigger arbitrary local handlers via `<Link url="file:///...">`. Detached + stdio ignore so closing the TUI doesn't kill the browser and Chrome stderr doesn't leak into the alt screen. - `entry.tsx`: pass `onHyperlinkClick: openExternalUrl` to `ink.render`. - `hyperlinkHover.ts` + Ink hover wiring: track the URL under the pointer in `Ink.hoveredHyperlink`, update it from `dispatchHover`, and inverse- highlight every cell of the matching link in the render-pass overlay (same pattern as `applySearchHighlight`). This is the cursor-hover affordance for clickable links — terminals don't expose cursor shape, so we light up the link itself. - `types/hermes-ink.d.ts`: add `onHyperlinkClick` to the `RenderOptions` shim so consumers (`entry.tsx`) type-check against the new option. Tests ----- - `src/lib/openExternalUrl.test.ts` (15 cases): http(s) accepted; file/js/ data/mailto/ftp/ssh rejected; macOS open(1), Windows cmd.exe start with empty title slot, Linux xdg-open dispatch; shell-metacharacter URLs pass through unmolested as a single argv element; synchronous spawn failure returns false. Verified empirically in Apple Terminal 455.1 (macOS 15.7.3): clicking a URL opens in default browser, hovering inverts the link cells, and moving away clears the highlight. Full TUI suite: 713 passing, 0 type errors. Reverts ------- The earlier attempt that version-gated Apple_Terminal in `supports-hyperlinks.ts` was based on a wrong assumption — Terminal.app silently strips OSC 8 sequences but does not render them as clickable hyperlinks. Reverted to the original allowlist. * tui: address Copilot review — explorer.exe on win32 + comment fixes - openExternalUrl: switch win32 from `cmd.exe /c start` to `explorer.exe`. cmd.exe's `start` builtin reparses the URL through cmd's tokenizer, so `&`, `|`, `^`, `<`, `>` either split the command or get reinterpreted — breaking both the protocol-allowlist safety story AND plain http(s) URLs with `&` in query strings. `explorer.exe <url>` invokes the registered protocol handler directly with no shell. - openExternalUrl.test.ts: rename the win32 test to reflect the new contract and add two regression tests — one with `&|^<>` metachars, one with the common analytics-URL `&` query-param pattern — both pinned to single-argv-element delivery via explorer.exe. - Link.tsx: fix misleading comment. OSC 8 escapes are emitted unconditionally by the renderer (`wrapWithOsc8Link` in render-node-to-output.ts, `oscLink` in log-update.ts). Non-supporting terminals silently strip the sequence, which is why hover/click affordance has to come from the in-process overlay rather than the terminal's own link rendering. Verified: 715/715 tests pass, type-check + build clean. * tui: address Copilot review #2 — async spawn errors + hover scope + docs 1. openExternalUrl: attach a no-op `'error'` listener on the spawned child BEFORE unref(). spawn() returns a ChildProcess synchronously even when the binary is missing (ENOENT on xdg-open / explorer.exe), unreachable, or otherwise unusable; the failure surfaces later as an 'error' event. An unhandled 'error' on an EventEmitter crashes Node, which would tear down the whole TUI. The listener is a deliberate no-op — we already returned `true` synchronously and the user just doesn't see the browser pop. 2. openExternalUrl.test.ts: add a regression test using a real EventEmitter to simulate the async-error path. Pins both the listener-attached contract and the "doesn't throw on emit" behavior. Was 17/17, now 18/18. 3. ink.tsx dispatchHover: bypass `getHyperlinkAt()` and read `cellAt(...).hyperlink` directly. `getHyperlinkAt` falls back to `findPlainTextUrlAt` for cells without an OSC 8 hyperlink, but the render-pass overlay (`applyHyperlinkHoverHighlight`) only matches on `cell.hyperlink === hoveredUrl` — so plain-text URLs would burn re-renders without ever producing the highlight. Hover is now a strictly 1:1 fit for what the overlay can paint. Plain-text URLs still get the click action via the existing dispatch path. 4. root.ts + ink.tsx doc comments: replace the misleading "typically `open` / `xdg-open` / `start` shell" wording with the actual safe recipe — argv-array spawn into `open` / `xdg-open` / `explorer.exe`, with an explicit warning that `cmd.exe /c start` reparses the URL through cmd's tokenizer and is unsafe + breaks `&`-query URLs. Verified: 716/716 tests pass, type-check + build clean. * tui: address Copilot review #3 — hover damage, alt-screen cleanup, opener allowlist 1. ink.tsx onRender: stop folding steady-state hover into hlActive. hlActive forces a full-screen damage diff so previous-frame inverted cells get re-emitted when the highlight set changes. The transition IS the trigger — enter / leave / change-to-other-link. While the pointer just sits on a link the painted cells don't change and the per-cell diff handles the no-op. Folding the steady state in would burn a full-screen diff on every frame. Added a lastRenderedHoveredHyperlink tracker and gate the hlActive bump on `hovered !== lastRendered`. 2. ink.tsx setAltScreenActive: clear hoveredHyperlink (and the tracker) when toggling alt-screen state. Hover dispatch is alt-screen-gated, so once we leave there's no path to clear it. Without this, remounting <AlternateScreen> would paint a phantom hover from the previous session until the next mouse-move arrived. 3. openExternalUrl.ts openCommand: allowlist linux + the BSD family for xdg-open and return null for everything else (aix, sunos, cygwin, haiku, etc.). Previously the default-fallback always returned xdg-open, which made the caller's `if (!command) return false` dead and yielded a misleading `true` on platforms that probably don't have xdg-open. New tests cover the null path AND the openExternalUrl-returns-false-without-spawning behavior. Verified: 718/718 tests pass, type-check + build clean. * tui: address Copilot review #4 — doc comment accuracy 1. openExternalUrl return-value doc: now lists all three false paths (URL rejected / no opener for platform / synchronous spawn throw) plus a note that async 'error' events still return true because the spawn was attempted. 2. ink.tsx onHyperlinkClick field doc: clarifies the callback receives either an OSC 8 hyperlink OR a plain-text URL detected by findPlainTextUrlAt — App.tsx routes both into the same callback. 3. hyperlinkHover applyHyperlinkHoverHighlight doc: drops the misleading 'caller forces full-frame damage' promise. Caller decides; for hover the current caller only forces full damage on transitions. No behavior change. 718/718 tests pass. * tui: address Copilot review #5 — lint fixes 1. ink.tsx: reorder `./hyperlinkHover.js` import before `./screen.js` to satisfy perfectionist/sort-imports. 2. Link.tsx: drop unused `fallback` parameter destructuring + the trailing `void (null as ...)` dead-statement (would trip no-unused-expressions). Kept `fallback?: ReactNode` on the Props interface as a documented compat shim so existing call sites still compile, with a comment explaining why it's no longer wired up. 3. openExternalUrl.test.ts: replace `typeof import('node:child_process').spawn` inline annotations (forbidden by @typescript-eslint/consistent-type-imports) with a `SpawnLike` type alias backed by a real `import type { spawn as SpawnFn }`. No behavior change. 718/718 tests pass, type-check clean, lint clean on all modified files.
Owner
Author
|
Cleaning up fork |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR removes the 5-second sleep delay from Matrix reaction redactions and runs them as non-blocking background tasks instead.
Changes
_reaction_redaction_delay_secondsconstant and the sleep inside reaction redaction tasksasyncio.Tasksdisconnect()remains for graceful shutdownBackwards Compatibility
Note on Presence
The presence state tracking feature originally planned for NousResearch#22087 has been rolled back due to server-side incompatibilities with Synapse rate-limiting behavior.