Repository navigation
Conversation
|
Review changes addressed in commits
Verification: 13/13 installed profiles passed Discord + canonical Bot Chat while A2A (ordinary and exact-title) stayed denied; targeted 12-file suite: 347 passed, 0 failed, 1 Windows-only skip; compileall, |
|
@andrexibiza Review requested for the completed Bot Mode trust-boundary changes. Reviewer verification: focused suite 199 passed, installed 13/13 profile matrix passed, A2A exact-title routing denied, diff check clean, and remote HEAD is a748148. The GitHub API would not allow the PR author account to assign a requested reviewer, so this explicit maintainer mention is the available review request. |
|
Re-review found one blocking source-cache authorization bypass.
Please bind both module- and agent-local frozen state to normalized source/trust identity (or validate that identity before reuse), then add same-home/same-session-ID regressions in both directions and cover injection/dispatch, not only the helper. Passing checks: remote/local HEAD |
bot_mode_session_state froze authorization by (profile home, persisted session id) only. A trusted CLI canonical Bot Chat entry was reused by a fresh api_server/a2a agent resuming the same persisted session, bypassing the explicit API/A2A deny boundary; the reverse order could poison a legitimate Bot Chat as denied. - process cache key becomes (resolved home, session id, normalized source) - agent-local fast path validates the normalized source before reuse - same-source recreation still hits the fast path, keeping the system prompt and tool schema byte-stable across a conversation Regressions cover both cross-source directions for the state helper, prompt injection (ensure_message_agent_tool), and gateway sessions on one persisted session id.
|
Re-review fix pushed (commit
Verification: new cross-source regressions (state helper + |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 95f383c3a9edf55574340bfeefb85e582f940b16. Three blockers remain.
1. The trust decision ignores Hermes's authoritative session source
tools/bot_mode_probe.py::_session_source() classifies only agent.platform. Hermes already defines the authoritative source in run_agent._session_source_for_agent(): task-local HERMES_SESSION_SOURCE overrides platform, and the existing source tests enforce that distinction.
That leaves a direct bypass the new matrix does not exercise. On a managed profile, set the task-local/environment source to tool (or another machine source), keep agent.platform == "cli", and resume the canonical Bot Chat. This gate sees cli, returns session_kind="bot_chat", injects message_agent, and the dispatch gate accepts it even though persistence and the rest of Hermes identify the turn as tool. The current cross-source regressions only change agent.platform, so they cannot catch this.
Use the canonical source resolver for classification and both cache identities. Add a regression using set_session_vars(source="tool") with platform="cli" and exact title Bot Chat, covering the state helper, schema injection, and dispatch/no-spawn result.
2. Schema stability and execution authority are frozen into the same non-revocable grant
message_agent_tool() claims a defense-in-depth execution check, but it calls the same process-frozen bot_mode_session_state() used to stabilize the prompt/schema. The new tests explicitly prove that removing the Bot Mode metadata and recreating the agent with the same session ID preserves the trusted result. Therefore removing the final hermes-bots marker, disabling agent.bot_mode_protocol, or tombstoning/deleting the sender profile does not revoke cross-profile delivery until the gateway process restarts.
Keep the tool schema/prompt shape stable if required, but do a live, fail-closed authorization check before any _start_delivery/relay/peer dispatch. At minimum that check must confirm the protocol is currently enabled, the install is currently Bot-Mode-managed, the sender is still a live roster profile, and the current authoritative source is allowed. Add a regression that primes the cached grant, revokes Bot Mode or tombstones the sender, patches the spawn path, calls message_agent_tool, and proves no delivery was attempted.
3. The new process cache has unbounded attacker-/traffic-controlled cardinality
_session_state_cache is a plain process-global dict with no TTL, size limit, or session-lifecycle eviction. bot_mode_session_state() stores rejected states too, and the global turn hook invokes it for API/machine sessions. An authenticated API client or high-cardinality gateway can therefore retain one entry per unique (home, session_id, source) for the lifetime of the process. This also outlives the bounded agent cache the new map is meant to bridge.
Bind this state to the existing session/agent-cache lifecycle, persist the frozen presentation decision with the session, or use a bounded cache with explicit eviction semantics. Add a cardinality/eviction regression; _reset_cache_for_tests() is not a production lifecycle.
Merge gate
Exact-head CI run 33613637846 for 95f383c3 is completed / action_required, not green: https://github.com/NousResearch/hermes-agent/actions/runs/33613637846
The roster containment, symlink/tombstone checks, multiplex home resolution, and the (home, session, source) repair are otherwise coherent. This head is not merge-ready until the three trust/cache blockers are fixed and exact-head CI completes green.
|
Re-review blockers addressed in verified commit
Verification on exact head: targeted 12-file suite |
|
Final re-review fixes are pushed at The source boundary now uses authoritative task/session source before the agent platform hint, so Verification on exact pushed HEAD:
PR remains open and unmerged. GitHub reports no CI checks for this branch. The real Yuki Discord handoff remains a post-deployment verification gate and is not claimed here. |
|
Re-review execution result for exact head This remains unapproved as a full application acceptance verdict. Exact-head CI, Docker, and Nix runs are all |
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 121708af6ddac18a00aa090d0418b12b515dc276 against live main@562ee8ab76a703b7f524172cae0b1d52f9f94bd3, the prior exact-head findings, the new commit, all five surviving commit objects/workflow states, and the live Bot Mode collision graph.
The three implementation blockers from my prior review are cleared on this head:
- authoritative source classification now goes through shared
gateway.session_context.resolve_session_source(), so task-localHERMES_SESSION_SOURCEcannot borrow trust fromagent.platform; the newsource=toolregressions cover helper/schema/dispatch and persisted-prompt restoration; - presentation/schema stability is no longer execution authority:
message_agent_tool()keeps the frozen session classification but separately calls livebot_mode_dispatch_authorized()immediately before delivery, re-reading protocol enablement, Bot Mode participation, live roster membership, title, and authoritative source; the revocation regression proves the spawn path is not reached after the sender profile is removed; - the cross-object session bridge is now a lock-protected, source-qualified LRU capped at 1024 entries, with an explicit eviction regression. Agent-local state remains source-bound and stable.
I do not see a replacement runtime blocker in that repair commit. The code findings from the previous review are therefore resolved.
This is still not merge-clear for two repository-level reasons.
1. Live collision / landing order: #100758
#101057 and #100758 are complementary, not independent: they overlap on 10 of this PR's 13 files — agent/conversation_loop.py, agent/system_prompt.py, agent/turn_context.py, gateway/session_context.py, run_agent.py, tests/agent/test_system_prompt_restore.py, tests/tools/test_bot_mode_dm.py, tests/tools/test_bot_mode_probe.py, tools/bot_mode_dm.py, and tools/bot_mode_probe.py.
The scopes are different: this PR is the narrower managed-gateway trust-boundary repair; #100758 is the broader CLI/Telegram orchestration layer and introduces the separate bot.enabled callable-profile policy. The safe landing order is #101057 first, then #100758 rebased/composed on top, preserving this PR's authoritative-source + live-dispatch-revalidation invariants while #100758 adds its stricter callable-profile gate. Landing #100758 first would require this PR to recompose those same authority owners before it could be trusted.
There is also substantial base drift: this PR's merge base is 6879a621b1daa816a27b7c01b75b500b992bbcd6; live main is 528 commits ahead and has changed overlapping owners including agent/conversation_loop.py and agent/turn_context.py. GitHub currently reports the PR mergeable, but that is not a composed-behavior proof. The eventual landing head needs a current-main rebase/composition pass and exact-object rerun.
2. Every surviving commit still lacks hosted green proof
The five surviving commits are:
eba6712679453e9c7ed5d599e12496cdaf91eb36— CI/Docker/Nix areaction_required;8a86d724c7221e8aa5243826073e3c99c2d62ee1— primary CI33608172609isfailurewith zero dispatched jobs; Docker/Nix areaction_required;a74814854fde0934bea70c34f6fb4bb14244ec18— CI/Docker/Nix areaction_required;95f383c3a9edf55574340bfeefb85e582f940b16— CI/Docker/Nix areaction_required;- current
121708af6ddac18a00aa090d0418b12b515dc276— CI33738426359, Docker33738425398, and Nix33738425473are allaction_required.
I attempted the two available proof-repair routes from the maintainer surface: rerunning current-head CI 33738426359, and creating an upstream exact-SHA witness ref for the surviving history. GitHub rejected both with 403 Resource not accessible by integration. The source fork also exposes push=false to this identity, so I cannot rebase or repair the contributor branch directly from this execution surface.
Result: previous code blockers cleared; merge still gated on the #100758 composition/current-main rebase and green hosted proof for every surviving commit.
|
New live topology edge: #102195 is now an active complementary owner on this exact Bot Mode authority surface. Verified FILE-LIST overlap is three files: The landing contract is therefore: preserve this PR as the source/session authorization owner; compose #102195's install/config enablement and roster constraint into that same decision; then let downstream direct-orchestration work such as #100758 consume the composed law together with its live per-profile The combined acceptance matrix must include config-only enable, roster denial, |
Rematerialize NousResearch#100758 on main f58fcc8, retaining the tested source-first composition of NousResearch#101057 and the declarative roster from NousResearch#102195, together with durable leases, publication fencing, chain-qualified compression recovery, repeated-step identities and strict configuration policy. Original development history: 924fce6, retained in backup/pr100758-before-landing-20260905. Source-authority contribution: NousResearch#101057 at 8355ab1. Declarative roster contribution: NousResearch#102195 at 2215b6e. The NousResearch#99333 client salvage and StodsEcho5 credit remain inherited from main via NousResearch#101214. Validated with scripts/run_tests.sh: 906 passed, 0 failed, 9 skipped across 43 files; retries disabled. Ruff 0.16.4 passed for all changed Python files. Hosted verification must reference this new commit, not earlier tips. Co-authored-by: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com> Co-authored-by: Ahmett101 <ahmet.tunc@gmail.com>
Summary
message_agentcapability and Bot Mode teammate protocol in managed messaging-gateway sessions, including Yuki Discord groupsVerification
scripts/run_tests.sh tests/tools/test_bot_mode_gateway_sessions.py tests/tools/test_bot_mode_dm.py tests/tools/test_bot_mode_probe.py tests/tools/test_bot_relay.py tests/tools/test_bot_turn_lock.py tests/agent/test_bot_profile_prompt_isolation.py tests/agent/test_prompt_builder.py tests/agent/test_prompt_cache_boundary.py tests/agent/test_system_prompt.py tests/agent/test_system_prompt_restore.py tests/agent/test_turn_context.py tests/gateway/test_profile_routing.py -qCoverage
message_agentremains absent from global registry/toolsets