refactor(gateway): extract session-resume authorization into GatewayResumeAuthorizationMixin - #75770
Conversation
|
Thanks for the focused decomposition. The six authorization methods currently reside together at Automated hermes-sweeper review. |
SummaryOne PR addresses #75769. #75770 extracts the six session-resume authorization methods from the slash-command module into a dedicated mixin and preserves their consumers through the existing MRO, directly addressing the reported responsibility boundary without changing gateway/run.py. Related pull requests
Suggested consolidationKeep #75770 open with a salvage path: retain the extraction of the six authorization methods into GatewayResumeAuthorizationMixin and its attachment through GatewaySlashCommandsMixin, which directly implements the requested decomposition while leaving gateway/run.py untouched. There are no competing or duplicate PRs to close. Complex graphflowchart 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
I75769(["issue #75769 (open)"])
P75770["PR #75770 (open)"]
P75770 -->|best fix| I75769
class I75769 open
class P75770 open
class P75770 best
class P75770 target
click I75769 "https://github.com/NousResearch/hermes-agent/issues/75769"
click P75770 "https://github.com/NousResearch/hermes-agent/pull/75770"
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 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 37 kB of PR diffs, 6 kB of issue/PR text, 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
…esumeAuthorizationMixin (god-file Phase 3) GatewaySlashCommandsMixin was itself lifted out of gateway/run.py by the god-file decomposition campaign. It has since grown to 5,383 lines and 65 methods, which makes it a decomposition target in its own right. This takes the cluster in it with the clearest boundary: deciding whether a caller may resume, or even see, somebody else's session. That is a trust decision rather than command plumbing. _resume_target_allowed is what stops a user in one chat from resuming a session belonging to a user in another, and tests/gateway/test_resume_command.py exercises it with explicit cross-user and IDOR cases. Moved verbatim (279 lines, 6 methods, contiguous at 912-1195): _gateway_session_origin_for_id _resume_caller_is_admin _same_matrix_room _resume_target_allowed _same_origin_chat _resume_row_visible Behavior-neutral. Every body is byte-identical to what it replaced; the only edits are the class declaration, one import, and the new module header. The new mixin becomes a base of GatewaySlashCommandsMixin rather than of GatewayRunner, so gateway/run.py is untouched and the change stays inside one file plus the new module. GatewayRunner picks the methods up through the existing MRO: GatewayRunner -> GatewayAuthorizationMixin -> GatewayKanbanWatchersMixin -> GatewaySlashCommandsMixin -> GatewayResumeAuthorizationMixin. Verified at runtime that every method name defined on the old class still resolves off GatewayRunner. The cluster calls no method that stays behind, and needs only Optional, Platform, SessionSource and is_shared_multi_user_session, taken from their original modules. _redact_matrix_session_key stays put: its one caller is _handle_context_command, which is not part of this cluster. slash_commands.py 5,483 -> 5,199 lines; the class 5,383 -> 5,098 lines and 65 -> 59 methods.
710f0e1 to
dfeacc8
Compare
|
Rebased onto current main at dfeacc8. Small conflict: main added the All six extracted methods still match main byte for byte, so the extraction stays behavior-neutral, and comparing every definition in main's 205 tests pass across the gateway resume, slash and sethome selection. One failure, |
Closes #75769.
GatewaySlashCommandsMixinis 5,383 lines and 65 methods. It is itself a product of the god-file decomposition campaign, lifted out ofgateway/run.pyto become one of the three mixinsGatewayRunneris built from, and it has since grown enough to be a decomposition target of its own.slash_commands.pygoes 5,483 -> 5,199 lines. The class goes 5,383 -> 5,098 lines and 65 -> 59 methods.What moved
279 contiguous lines (912-1195), six methods, and they are the six in the file that are not about dispatching a command:
_gateway_session_origin_for_id_same_matrix_room/_same_origin_chat_resume_caller_is_admin_resume_target_allowed_resume_row_visibleThis is a trust boundary.
_resume_target_allowedis what stops a user in one chat from resuming a session belonging to a user in another, andtests/gateway/test_resume_command.pycovers it with explicit cross-user and IDOR cases.Where it is attached, and why
The new mixin becomes a base of
GatewaySlashCommandsMixin, not ofGatewayRunner:GatewayRunnerpicks the methods up through the MRO it already has, sogateway/run.pyis not touched at all. On a file taking roughly eleven commits a day that is worth the indirection: this PR cannot go conflict-dirty against work happening there.Resulting order:
GatewayRunner -> GatewayAuthorizationMixin -> GatewayKanbanWatchersMixin -> GatewaySlashCommandsMixin -> GatewayResumeAuthorizationMixin. The six sit one step further back than before, which is immaterial: they were already behind the first two mixins, and neither defines any of these names. Verified at runtime that every method name defined on the old class still resolves offGatewayRunner, and that all six resolve to the new mixin.Why it is safe
The bodies are byte-identical. A script parses the pre-refactor
slash_commands.pyout of git and the two post-refactor files, extracts each method's exact source span, and compares strings. All 6 match. The same script confirms exactly those 6 left the class, that nothing was added, and that no other method changed by a character.Nothing follows it that shouldn't. The cluster calls no method that stays behind, and needs only
Optional,Platform,SessionSourceandis_shared_multi_user_session, taken from their original modules. No test patches a module-level name ingateway/slash_commands.py, by dotted string or through the module object. The new module imports neithergateway.slash_commandsnorgateway.run, so there is no cycle._redact_matrix_session_keystays put: its one caller is_handle_context_command, which is not part of this cluster.Verification
Differential run against clean
upstream/mainat the same base commit, in two isolated worktrees, overtests/gatewayandtests/hermes_cli:Identical failure sets, all pre-existing on
main; set difference empty in both directions. The resume-specific suites (test_resume_command.py,test_dm_topics.py,test_base_topic_sessions.py) are 35 passed on both sides.No test file is modified by this PR.