Repository navigation
feat(whatsapp): observe-unmentioned group context (Telegram observe-mode parity) - #133418
samuelcardillo wants to merge 2 commits into
Conversation
…ode parity) Port Telegram's `observe_unmentioned_group_messages` to the WhatsApp bridge so a require_mention bot can FOLLOW a group conversation without answering every message. Behavior (all opt-in, default unchanged): - `whatsapp.observe_unmentioned_group_messages: true` + `whatsapp.observe_allowed_chats:` (explicit group-JID allowlist, mirroring Telegram's observe_allowed_chats gate). - Group messages the require_mention gate would skip are appended to the group's SHARED session transcript as `observed=True` rows (sender-tagged `[Name|id]`), never dispatched. - Observed rows land in a chat-scoped session (user identity stripped, same mechanism Telegram uses), so a later @mention / wake-word / reply-to-bot turn in the same chat resolves to the same session and receives the stored chatter as a context-only block (`_build_gateway_agent_history` already splits `observed` rows out of replayable history when the turn's channel prompt carries the observed-context marker). - Slash-command turns keep the original sender identity; free-response chats are never observed; the bot's own messages and DMs are never observed. Auth fix required by the shared-session design: - `_GROUP_CHAT_ENV` gains `WHATSAPP_GROUP_ALLOWED_CHATS`. Attributed trigger turns carry `user_id=None` (chat-scoped source); central authz previously only knew chat-scoped group allowlists for Telegram/QQBOT, so every such WhatsApp turn was dropped by the no-user-id guard in `_hm_admit_event` while observed rows kept accumulating. This gives WhatsApp the same escape hatch Telegram observe-mode ships with. Config: - YAML bridge: `observe_unmentioned_group_messages` → `WHATSAPP_OBSERVE_UNMENTIONED_GROUP_MESSAGES`, `observe_allowed_chats` → `WHATSAPP_OBSERVE_ALLOWED_CHATS` (env wins, as with all WHATSAPP_* keys). Tests: tests/gateway/test_whatsapp_observe_unmentioned_groups.py (17 cases: observe gating matrix, shared-session storage, attribution, user-less-turn authorization, history-builder marker recognition).
alt-glitch
added
type/feature
JoaoMarcos44
left a comment
JoaoMarcos44
left a comment
There was a problem hiding this comment.
@samuelcardillo — I reviewed the five changed files and the producer → attribution → admission → session/store → replay boundaries at 042a4fe7d6be89082d733e138689507f271c568f. There is one P1 availability/configuration blocker and three P2 correctness findings below. This is a COMMENT review, not an approval or a REQUEST_CHANGES event. No repair implementation or competing PR is being proposed here.
Findings
F-001 — P1: Preserve admission for already authorized trigger senders
Observation attribution removes the caller before the gateway can apply its existing grant. Already permitted addressed turns can be silently dropped after opting in. Preserve principal-based admission independently of shared routing, or make the separate chat execution grant an explicit, validated configuration requirement.
With a synthetic allowed participant, real _build_message_event -> attribution -> _hm_admit_event admitted the original event but refused the attributed event at this head (user_id became None). The same fixture passed at the merge base; feature-off and a separately configured chat grant passed at the head. No live bridge, model call or forbidden principal was used.
Controlling source; the inline comment supplies the complete conditions and regression contract.
F-002 — P2: Constrain both observation and attribution to effective mention-gated scope
Collection and attribution do not share the effective mention-gated scope. Normal traffic is double-classified, and free-response traffic is unnecessarily re-sourced. Guard both paths with the same effective observe scope or a single exclusive process/observe/drop decision.
A single legitimate text message through the real polling loop produced one normal queue event plus one real observed SQLite row at the head; merge base produced one queue event and zero observed rows. A separate free-response event-build/attribution probe changed user_id to None and prefixed text at the head, while baseline preserved both. The bridge response and queue sink were inert substitutes, not live delivery.
Controlling source; the inline comment supplies the complete conditions and regression contract.
F-003 — P2: Route slash commands to the observed group session without dropping their sender
Command sender preservation also preserves a per-user routing suffix. Session controls such as /new select a different lane from the shared observed conversation. Separate principal identity from shared-session routing and keep that choice through command and restore consumers.
Actual event construction, attribution and real SessionStore produced text key agent:main:whatsapp:group:120363000000000001@g.us versus command key agent:main:whatsapp:group:120363000000000001@g.us:700000000001, with distinct session IDs and the command sender still present. Baseline text/command keys matched. The probe did not execute a reset or interrupt an agent. /stop has a separate same-chat fallback, so this is not a claim that /stop invariably fails.
Controlling source; the inline comment supplies the complete conditions and regression contract.
F-004 — P2: Emit a timestamp accepted by the durable transcript writer
The ISO producer value is rejected by the existing timestamp coercion boundary. Observed writes produce false corruption warnings and substitute the stamped time. Write a supported epoch/datetime value and assert exact durable readback without warnings.
With the observer clock fixed to 2024-01-02T03:04:05+00:00, a real SQLite readback returned a fallback timestamp instead of expected epoch 1704164645.0 and logged the corrupt-timestamp warning. A numeric timestamp control was preserved by the same writer. The composed observation happy-path probe also emitted these warnings. Baseline has no WhatsApp observer, so this is head-only producer/store proof, not branch RED/GREEN.
Controlling source; the inline comment supplies the complete conditions and regression contract.
Independent execution and test adequacy
Reviewed head: 042a4fe7d6be89082d733e138689507f271c568f.
PR base / merge base: 38d19b92e81e39f2025add3ff75da425c1f31648.
Runtime: native Windows, CPython 3.14.7, pytest 9.1.1, aiohttp 3.14.3; fresh PM-built dev/test + messaging environment, clean environment and isolated homes.
The canonical command was scripts/run_tests.sh with the following nine files, -j 3 --file-timeout 120 --file-retries 0, and HERMES_PYTHON pointing at that isolated interpreter:
tests/gateway/test_whatsapp_observe_unmentioned_groups.pytests/gateway/test_whatsapp_group_gating.pytests/gateway/test_whatsapp_group_allowlist_env.pytests/gateway/test_whatsapp_text_batching.pytests/gateway/test_whatsapp_from_owner.pytests/gateway/test_whatsapp_multiplex_secret_scope.pytests/gateway/test_replay_entry_fields.pytests/gateway/test_telegram_group_gating.pytests/gateway/test_whatsapp_cloud.py
Head: 134 passed, 0 failed, including the 17 new cases; command wall time 53.70 s. The eight already-existing sibling files ran at the merge base with the same interpreter/options: 117 passed, 0 failed, command wall time 50.96 s. These are separate focused execution receipts, not a combined count of distinct tests or evidence of full CI.
The new storage test substitutes event construction and FakeStore; its authorization tests exercise _chat_scoped_grant with the added grant already supplied, and its history test checks the marker predicate. Consequently, those green cases do not cover existing caller-grant compatibility, a real persisted observation, process/observe exclusivity, or command/text session equality. The independent probes above deliberately exercise those missing boundaries.
The final fresh-process probe set had five contract failures covering the four causal findings (normal and free-response classification are one scope defect), plus six passing baseline/positive controls. It used benign synthetic participants and an owned temporary SQLite store. Polling used an inert HTTP response and a recording queue sink; admission used the real gateway gate without full startup. No live WhatsApp/Baileys service, external model, message send, credentials, unauthorized principal or adversarial payload was exercised. An initial Windows loop-construction fixture error was corrected and excluded from these behavioral results.
A composed working control with an explicitly configured group_allowed_chats grant retained two unmentioned rows, enqueued/admitted one addressed turn, and loaded both stored rows into context-only history rather than ordinary replay. This supports the WhatsApp marker change and bounds F-001: the feature is not universally inoperative. It still emitted the F-004 timestamp warnings.
Related work and repair design
I inspected the relevant effective hunks, not just titles. #104245 and #85490 overlap the WhatsApp observation/session/replay feature; both observer predicates include effective mention gating, and #104245 also excludes free-response from attribution. #102980 chooses normal processing versus passive observation once at poll entry, with a narrower text-only contract. #84926 deliberately changes invocation to native-mention-only; that is not equivalent to preserving this PR's wake-word/reply/command parity.
#131336 handles the same principal versus shared-session problem for Telegram by making sharing independent of the sender identity; it does not implement the WhatsApp side. #118106 repairs the timestamp producer class at Telegram/Yuanbao, not this new WhatsApp site. #111214 is closed and unmerged, so its generic replay-marker proposal is not inherited main behavior.
The smallest coherent direction for F-001/F-003 is to separate caller identity from routing identity and preserve that contract through keys, commands, restore and shared prompt/context consumers. A separate, explicitly required chat grant is a viable configuration contract for F-001, but cannot repair F-003 on its own; observation permission must not silently confer execution permission. F-002 can be addressed by a shared effective-scope guard or an exclusive intake decision. F-004 belongs at the producer rather than relaxing every reader's corrupt-timestamp handling.
A platform-neutral observation service is another credible consolidation design, but the broader candidates introduce distinct persistence/media/compaction contracts. Their complete branches were not independently executed, and I am not claiming that one is superior, fully correct, or ready to merge. Existing ownership should be preserved instead of publishing another duplicate implementation.
Merge and evidence boundaries
The inspected current-main comparison was 3569fba6a686e17f9208da7500de8bd20b4c82f9: this branch was 1,499 commits behind / 1 ahead at that snapshot. The relevant current-main admission/key predicates still matched the inspected baseline, while the WhatsApp observer was absent. That is source comparison, not a full suite or automatic merge/rebase proof.
Before merging, reconcile the above behavior/configuration contracts, rebase or otherwise verify the effective patch against current main, rerun the focused and new composed regression assertions at the final head, and satisfy the repository's actual checks/review requirements. The captured GitHub reads reported no checks; mergeability changed from MERGEABLE/BLOCKED to UNKNOWN during preparation, so the final merge/protection state must be rechecked before merging. None of those states proves that CI passed or never ran.
Live bridge/media delivery, native text-debounce/busy-session composition, full multiplex-profile provenance/restore, and full-repository CI remain unverified by these probes. I did not turn the source-level loss of transient provenance into a confirmed cross-profile exploit: adapter ingress can re-canonicalize the source, and a harmful end-to-end consequence was not established. Likewise, /stop's same-chat fallback prevents inferring an unconditional stop failure from the command key mismatch.
These are bounded functional findings, not an official vulnerability/advisory classification or a claim that no other defects exist.
| return dataclasses.replace(event, channel_prompt=channel_prompt) | ||
| return dataclasses.replace( | ||
| event, text=self._whatsapp_group_observe_attributed_text(event), | ||
| source=self._whatsapp_group_observe_shared_source(event.source), channel_prompt=channel_prompt) |
There was a problem hiding this comment.
[P1] Preserve admission for already authorized trigger senders (F-001)
The attribution path clears user_id before central admission. _principal_authorized returns False for a user-less source before evaluating existing caller grants unless a separate chat-scoped grant is configured.
Repro conditions: Enable observation and observe_allowed_chats for a group that already passes group_policy: allowlist/group_allow_from, and permit the triggering participant via WHATSAPP_ALLOWED_USERS. Do not add a second WHATSAPP_GROUP_ALLOWED_CHATS or extra.group_allowed_chats grant.
An already permitted mention/wake-word/reply trigger becomes silent after observation is enabled, even while passive chatter may continue accumulating. Observation permission and the normal group intake allowlist are not substitutes for the new execution grant.
Observed evidence: With a synthetic allowed participant, real _build_message_event -> attribution -> _hm_admit_event admitted the original event but refused the attributed event at this head (user_id became None). The same fixture passed at the merge base; feature-off and a separately configured chat grant passed at the head. No live bridge, model call or forbidden principal was used.
Required repair: Decouple session sharing from caller authorization so the real principal survives admission, or explicitly make the distinct chat execution grant a required configuration contract and wire/document a complete working configuration. Do not automatically turn observe_allowed_chats into permission to invoke the agent. Validate both normal user grants and the deliberate chat-grant path.
Regression contract: Exercise actual event construction/attribution through _hm_admit_event with an already allowed participant and existing group intake config; require continued admission or an explicit setup error. Keep a feature-off control and an explicit chat-grant control. Preserve sender checks on commands and the distinction between observation and invocation permission.
Pinned source:
| or self._message_matches_mention_patterns(data) | ||
| ): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
[P2] Constrain both observation and attribution to effective mention-gated scope (F-002)
The observer predicate omits not _whatsapp_require_mention(), while polling still runs the normal build/queue arm after the observer. Attribution likewise ignores require_mention and free_response_chats.
Repro conditions: Keep the opt-in observation flag/list configured, but set require_mention: false. Independently, use an explicitly configured free_response_chats entry with require_mention: true.
Normally handled chatter is additionally stored as observed context, and normal/free-response traffic loses its original principal and text/session shape. This creates duplicate context and can also reach the admission failure in F-001; it is not passive-only behavior.
Observed evidence: A single legitimate text message through the real polling loop produced one normal queue event plus one real observed SQLite row at the head; merge base produced one queue event and zero observed rows. A separate free-response event-build/attribution probe changed user_id to None and prefixed text at the head, while baseline preserved both. The bridge response and queue sink were inert substitutes, not live delivery.
Required repair: Use one effective observe-chat predicate in collection and attribution: feature enabled, explicit observe list, supported group intake, mention gating enabled, and not free-response. Alternatively decide process/observe/drop once at poll entry and keep the dispositions exclusive. Preserve the intentional passive group-context utility rather than disabling it.
Regression contract: For require_mention=False, require one normal queue event and zero observed writes through actual polling plus a real store. For free-response chats, require the original principal/text and zero observe attribution. Keep a mention-gated unmentioned positive case with an observed write and no normal dispatch, and an addressed trigger control.
Pinned source:
| channel_prompt = f"{event.channel_prompt}\n\n{observe_prompt}" if event.channel_prompt else observe_prompt | ||
| if str(getattr(event, "text", "") or "").strip().startswith("/"): | ||
| # Commands keep the original source (user_id) so sender checks can identify the sender. | ||
| return dataclasses.replace(event, channel_prompt=channel_prompt) |
There was a problem hiding this comment.
[P2] Route slash commands to the observed group session without dropping their sender (F-003)
The command branch preserves the original source, but non-command text uses a sender-less source. With default group_sessions_per_user=True, the actual SessionStore selects a sender-suffixed command lane and a different chat-only conversation lane.
Repro conditions: In an opted-in observed group with the default per-user session setting, the same participant sends an addressed text turn and then /new (or another session-scoped command).
Session-scoped commands operate on a different conversation from the shared observed-group text. For example, the reset handler computes the command source key and resets that exact key, leaving the shared text lane untouched.
Observed evidence: Actual event construction, attribution and real SessionStore produced text key agent:main:whatsapp:group:120363000000000001@g.us versus command key agent:main:whatsapp:group:120363000000000001@g.us:700000000001, with distinct session IDs and the command sender still present. Baseline text/command keys matched. The probe did not execute a reset or interrupt an agent. /stop has a separate same-chat fallback, so this is not a claim that /stop invariably fails.
Required repair: Represent shared-session routing independently of user_id, and use it consistently for observed rows, addressed turns, commands, restored origins and shared-session guards/prompt context. Preserve the principal for authorization; do not fix this by nulling the command sender.
Regression contract: At default group_sessions_per_user=True, require the actual store/key path to give observed chatter, addressed text, commands and restored command origins the same session, while the command still retains its user identity. Verify /new resets that shared lane and unrelated groups remain per-user where intended.
Pinned source:
| session_entry = store.get_or_create_session(self._whatsapp_group_observe_shared_source(event.source)) | ||
| entry = { | ||
| "role": "user", "content": self._whatsapp_group_observe_attributed_text(event), | ||
| "timestamp": datetime.now(tz=timezone.utc).isoformat(), "observed": True} |
There was a problem hiding this comment.
[P2] Emit a timestamp accepted by the durable transcript writer (F-004)
The new observer emits datetime.now(...).isoformat(), but SessionStore forwards timestamp unchanged and SessionDB coercion accepts epochs/numeric strings/datetime objects, not ISO strings.
Repro conditions: Persist any observed WhatsApp group row through the real SessionStore/SessionDB path; the new producer always uses an ISO timestamp string.
Every such write can generate an Ignoring corrupt message timestamp warning and replaces the producer timestamp with the writer fallback. The message content and observed flag are retained; this is not database corruption or wholesale transcript loss.
Observed evidence: With the observer clock fixed to 2024-01-02T03:04:05+00:00, a real SQLite readback returned a fallback timestamp instead of expected epoch 1704164645.0 and logged the corrupt-timestamp warning. A numeric timestamp control was preserved by the same writer. The composed observation happy-path probe also emitted these warnings. Baseline has no WhatsApp observer, so this is head-only producer/store proof, not branch RED/GREEN.
Required repair: Emit a supported numeric epoch or datetime object at the observation producer, consistently with the existing transcript contract. Do not broaden the shared corrupt-row reader merely to accommodate this new local writer.
Regression contract: Freeze the observer clock, persist through real SessionStore/SessionDB, read back the exact expected epoch and assert no corrupt-timestamp warning. Keep the numeric/datetime positive control and verify observed/content/message-ID metadata still survives.
Pinned source:
…ode parity) Rebase + rework of NousResearch#133418 addressing the review findings (F-001..F-004): NousResearch#133418 (review) Core design change (F-001, F-003): shared-session routing is now declared on the SessionSource (``shared_session: bool``) instead of erasing the caller identity. - Attributed text turns, commands and observed chatter KEEP ``user_id``, so ``_hm_admit_event`` authorizes an already-allowed participant exactly as before — no second chat-grant configuration is required for the feature to work, and observation permission never silently becomes invocation permission. - ``build_session_key`` / ``is_shared_multi_user_session`` honor ``shared_session``, so observed chatter, addressed turns, commands and origins restored through ``to_dict``/``from_dict`` all resolve to ONE shared group lane. ``/new`` and other session-scoped commands now act on the conversation the bot is actually in, while slash-access sender checks keep working. The field round-trips only when set, so existing stored origins stay byte-identical. (Same field name/contract as NousResearch#131336 so the two PRs compose.) F-002: a single effective observe scope (``_whatsapp_observe_scope_active``) governs BOTH collection and attribution: feature flag + explicit ``observe_allowed_chats`` + mention-gated intake (require_mention on, chat not free-response). With ``require_mention: false`` a message is processed once, normally, and never observed; free-response chats keep their original principal and text. F-004: the observer emits a ``datetime`` object, which ``coerce_epoch`` accepts — verified by a real SessionStore/SessionDB round-trip with exact epoch readback and no corrupt-timestamp warning. The WHATSAPP_GROUP_ALLOWED_CHATS authz registration is retained as a general capability (mirrors Telegram's TELEGram_GROUP_ALLOWED_CHATS for user-less group principals) but is no longer required by this feature. Tests (18): observe gating matrix, effective-scope exclusivity (require_mention off / free-response), principal-preserving shared routing with the text/command/observed/restored one-key invariant, real-database timestamp round-trip, history-builder marker recognition. Sibling suites: 118 passed. Driver: live WhatsApp bot validated (unmentioned chatter observed and injected on the next mention; mention-only reply behavior preserved).
|
Thank you @JoaoMarcos44 for the rigorous review — this is exactly the kind of boundary analysis the feature needed. All four findings are addressed in da5141f: F-001 (P1) — principal survives admission. Reworked per your suggested direction: shared-session routing is now declared, not identity-erasing. F-002 — one effective scope. F-003 — commands act on the shared session. With F-004 — timestamp contract. The observer now emits a The tests were rebuilt around your regression contracts: the gating matrix now exercises the effective-scope exclusivity ( One note: the branch was rebased onto current main (38d19b9 base was 1,499 commits stale per your snapshot; the push carries the current-main tree). Happy to rebase again if main has moved further. |
JoaoMarcos44
left a comment
JoaoMarcos44
left a comment
There was a problem hiding this comment.
Correction re-review at da5141fcdf27720941dfa0349b947d4d302e4458
@samuelcardillo, I independently compared the correction with review 5444491279, reran the previous contracts on both immutable snapshots, and checked the correction's unrelated startup changes. The reviewed base is 38d19b92e81e39f2025add3ff75da425c1f31648; the previous failing head is 042a4fe7d6be89082d733e138689507f271c568f.
Scoped verdict: the four original functional findings are corrected in the tested boundaries, but a separate startup-recovery regression remains a blocker. This is a COMMENT review, not approval or an unconditional merge recommendation.
Original findings: individually reconciled
- F-001, original P1 — corrected with behavioral proof. An already allowlisted participant now remains admitted through actual event construction, observe attribution and
_hm_admit_event, without adding a chat-level execution grant. Feature-off and separate-grant controls still work. Preserving the sender while declaring shared routing repairs the original admission loss rather than broadly granting observation participants execution authority. - F-002, original P2 — corrected with behavioral proof. The real polling path with
require_mention=falsequeues one ordinary event and writes no observed row; free-response attribution preserves the original sender/text. Collection and attribution now use the same effective mention-gated scope. - F-003, original P2 — corrected with behavioral proof. Actual
SessionStoretext and command lookups select the same lane while the command retains its sender. A serialized/restored command origin also retains that lane. This does not establish live/newor/stopexecution. - F-004, original P2 — corrected with behavioral proof. The observer's frozen datetime reaches real SQLite as the exact expected epoch, without the old corrupt-timestamp warning or fallback substitution. A numeric timestamp control passes through the same writer.
The same seven-case external contract gave 2 controls passed / 5 intended failures on the old head, then 7 passed on this head. The two scope failures are one F-002 mechanism, not two findings. Both executions used native Windows, CPython 3.11.16 and pytest 9.1.1. The original review's earlier Python 3.14.7 result is historical; these paired runs use the same runtime and assertions. The HTTP response and queue sink are inert substitutes; event construction, admission, polling, real stores and history separation execute production code. These are composed local boundary checks, not live WhatsApp/Baileys/model E2E.
N1 — P1: the correction removes boot replay without bringing its replacement owner
The correction also removes _recover_pending_flushes and its call after a successful runner.start() in gateway/run.py. Unlike a complete recovery-boundary move, this snapshot has neither recover_gateway_pending in gateway/shutdown_flush.py nor its invocation in the real finish-wiring phase.
Reachable condition: an earlier DB outage/cap eviction or shutdown left a valid pending recovery payload, and the gateway starts successfully. The existing live drain is not a replacement boot scan: session_transcript.py returns unless the process-local spooled-session set was populated. A cold process cannot infer that set merely from the surviving file.
I exercised the same benign startup contract at both heads:
- Create an owned real session/SQLite database and spool one ordinary transcript message with
spool_dropped_transcript_message. - Call the actual
start_gatewayorchestration with successful, inert adapter/host/MCP/supervisor infrastructure and the real session store. - Read the database and original spool file at successful return.
- As an independent control, call the unchanged public recovery reader on any remaining payload.
Previous head: 1 passed. Current head: 1 intended failure. Current startup returned True, but the row was absent and the spool remained. The manual reader then recovered exactly that message into the same database and consumed the file. This excludes a corrupt payload or unavailable SQLite writer as the explanation. The full real GatewayRunner.start/live service was not executed in this harness; the actual finish-wiring and live-drain source were separately checked for an alternative consumer.
There is also a directly attributable canonical collection regression: tests/gateway/test_profile_spool_recovery.py still imports the removed helper. The same two-file selection, including test_shutdown_flush.py, gives 12 passed on the previous head, versus 11 passed and one collection-error file on this head, exit 1. This is not a passing suite merely because its summary prints zero failed assertions.
Consequence and bound: valid messages can remain outside the transcript after restart, making prior conversation/recovery data unavailable to the normal reader. The demonstrated file is preserved and manual recovery works; I am not claiming permanent byte destruction, SQLite corruption, an exploit or an official security vulnerability. The default-home effect is exercised; the routed-profile contract currently cannot collect and still needs final-head validation.
Controlling repair: keep the recovery lifecycle coherent. Merged #133932 removes this same outer helper only together with the new recover_gateway_pending owner, its pre-resume finish-wiring call, shared transcript replay mapping and updated tests. This candidate includes the removal without those replacement siblings. Bring the canonical transition in coherently or otherwise preserve the existing boot replay; do not fix this by deleting the failing test or simply reviving an obsolete API after rebasing. A proposed new observer-specific recovery loop would duplicate the shared spool owner and still miss non-WhatsApp sessions.
Regression contract: successful cold startup must consume a healthy owned spool into the correct store before resumed/live writes; default and routed profiles must remain separate, chronology and full row metadata must survive, and failure must retain the recoverable file. Verify the selected canonical lifecycle after integration, not just the deleted symbol's importability.
N1 is explained in the body because its causal change is deletion-only. There is no meaningful newly added RIGHT line for that removed call; attaching it to an unrelated observation/prompt line would misrepresent the finding.
Execution receipts and remaining conditions
All named tests ran through scripts/run_tests.sh, with -j 1 --file-retries 0 --file-timeout 120 -v -s --tb=short, an existing explicit HERMES_PYTHON, credential-free environment and separate temporary homes. The external contracts remain supplemental despite using that runner.
- Original-finding paired contract: old 2 passed / 5 failed, 54.68 s; current 7 passed, 19.80 s.
- New startup paired contract: old 1 passed, 4.40 s; current 1 failed, 4.20 s.
- Shipped observation/admission/replay siblings: 135 passed across nine files, 54.06 s. This includes
test_whatsapp_observe_unmentioned_groups, group gating/allowlist, batching, from-owner, multiplex secret scope, replay fields, Telegram gating and WhatsApp Cloud. - Shipped spool selection: old 12 passed, 15.11 s; current 11 passed plus one collection-error file, 15.65 s.
Times are command wall times, not session duration. Source/test hashes were unchanged. Archive snapshots have no Git metadata, so the runner's best-effort precompile step warns; the actual result protocol completed. The wrapper reuses a requested JUnit pathname per file, so multi-file totals above come from its completed per-file/summary output, not the last overwritten XML alone.
Before merge, reconcile N1 against the complete current-main recovery design, rerun the original four contracts plus meaningful boot/routed-spool and relevant sibling assertions at the final head, and establish the required hosted checks and reviews. GitHub currently reports no checks and unknown mergeability/merge state; that proves neither passing CI nor that workflows never executed. Physical/live bridge behavior, all multiplex restore/debounce combinations, package-install/reconnect failure paths, crash consistency and full-repository CI remain unverified. No code implementation, competing PR or change to contributor authorship is proposed here.
…ted main-drift hunks The previous rework commit accidentally carried new-main state into gateway/run.py, removing _recover_pending_flushes and its startup call while the old-base ecosystem (shutdown_flush.py, test_profile_spool_recovery.py) still expects them. Restore the old-base run.py (which already contains the WhatsApp marker change) and rebuild the other five files as feature-only diffs — no unrelated main drift. Verified: test_profile_spool_recovery + test_shutdown_flush 12 passed (was 11 + collection error); observe tests 18 passed; sibling suites 117 passed.
da5141f to
210fb9e
Compare
|
N1 fixed in 210fb9e — good catch, and it was entirely my fault in how I assembled the rework commit: I checked the five feature files out of a new-main-based branch onto the old base, which silently replaced Repair: Verified per your contract: Thanks again — the review's pairing of behavioral probes with the import-error signal is what surfaced this. |
JoaoMarcos44
left a comment
JoaoMarcos44
left a comment
There was a problem hiding this comment.
Correction re-review at exact head 210fb9ef1f19a9a608d54e137cea74fa814d6ef9
@samuelcardillo, I revalidated the four findings from review 5444491279 and the startup-recovery finding from review 5447282016 against this exact head (210fb9ef1f19a9a608d54e137cea74fa814d6ef9, base 08165d58931841cee713468ae89032af7c57060a).
Scoped status: the original four behavior findings and the later startup-recovery regression are corrected on this pinned head. This is not an approval or a merge-readiness recommendation.
Original findings F1-F4
The same seven-case reviewer contract passed 7/7 at this head. It exercises the real WhatsApp event construction, polling/attribution, gateway admission, session-key selection, and SessionStore/SQLite timestamp write. The cases cover already-authorized sender admission, exclusive observe/dispatch scope, shared command routing, and durable timestamp preservation, with their feature-off/free-response/lifecycle controls. The final transport and host services are inert; this is not live WhatsApp, model, or provider E2E. These reviewer-only tests are untracked and are not part of the PR.
N1: boot spool recovery
The same one-case successful-startup spool contract that failed on the previous reviewed head (da5141fcdf27720941dfa0349b947d4d302e4458) now passes on 210fb9ef1f19a9a608d54e137cea74fa814d6ef9. On the previous head, startup returned successfully with the row absent and spool still present; the unchanged manual recovery reader then recovered the same payload. On the current head, startup consumed the spool and the row was present by return.
Canonical command on the current head: source scripts/run_tests.sh tests/gateway/test_manual_review_startup_spool.py tests/gateway/test_manual_review_whatsapp_correction.py tests/gateway/test_profile_spool_recovery.py tests/gateway/test_shutdown_flush.py -j 1 --file-retries 0 --file-timeout 180 -v --tb=short, with HERMES_PYTHON set to an external CPython 3.11.15 environment. Native Windows, pytest 9.1.1; the local Hermes test site-packages were reused, so exact uv.lock parity was not established. Result: 20 passed, 0 failed (1 startup case, 7 correction cases, and 12 shipped assertions). The first two files are reviewer-only, untracked contracts, not additions to the PR. On the previous head, the same startup contract failed; the selected spool files also had 11 passes plus a collection error because _recover_pending_flushes was missing. This is a bounded local composition probe, not a full live gateway/service restart.
Current-base integration remains unverified
The PR is still DIRTY against current base 08165d58931841cee713468ae89032af7c57060a; its merge base is 38d19b92e81e39f2025add3ff75da425c1f31648 (1,769 commits on the base side and 2 on the head side). The current base contains the newer shared recovery owner from #133932: recover_gateway_pending runs before resume/restore, marks held-back sessions, and handles served profiles. The verified head still reflects the pre-#133932 startup tree, so the results above do not validate the eventual conflict resolution. The current-base recovery suites passed 18 tests locally, but that is not evidence for the unre-based PR head.
Before merge, rebase/resolve against current main while preserving the shared pre-resume recovery lifecycle, then rerun these correction contracts and the current-base recovery suites on the final head. GitHub reported no check runs or status contexts; that does not prove CI never ran. No additional inline finding is submitted in this bounded correction re-review.
This branch has not been deployed
3 participants
Summary
Ports Telegram's
observe_unmentioned_group_messagesto the WhatsApp bridge, so arequire_mentionbot can follow a group conversation without answering every message.The problem this solves: with
require_mention: true(the recommended WhatsApp bot posture), any group message that does not @mention the bot (or match a wake-word pattern) is dropped at the adapter with no trace. The bot then literally has no context of the conversation it is supposedly part of — when finally mentioned, it has to ask "what are we talking about?".The mechanism (all opt-in, default behavior unchanged):
whatsapp.observe_unmentioned_group_messages: true+whatsapp.observe_allowed_chats:(explicit group-JID allowlist, mirroring Telegram'sobserve_allowed_chatsgate).require_mentiongate would skip are appended to the group's shared session transcript asobserved=Truerows (sender-tagged[Name|id]), never dispatched._build_gateway_agent_historyalready splitsobservedrows out of replayable history when the turn's channel prompt carries the observed-context marker — this PR teaches that function to recognize the WhatsApp marker alongside the Telegram one.Auth fix required by the shared-session design
Attributed trigger turns carry
user_id=None(chat-scoped source). Central authorization only knew chat-scoped group allowlists for Telegram (TELEGRAM_GROUP_ALLOWED_CHATS) and QQBOT, so every such WhatsApp turn was silently dropped by the no-user-id guard in_hm_admit_eventwhile observed rows kept accumulating. This PR addsWHATSAPP_GROUP_ALLOWED_CHATSto_GROUP_CHAT_ENV— the same escape hatch Telegram observe-mode ships with.Config
YAML bridge extended (env wins, as with all
WHATSAPP_*keys):observe_unmentioned_group_messages→WHATSAPP_OBSERVE_UNMENTIONED_GROUP_MESSAGESobserve_allowed_chats→WHATSAPP_OBSERVE_ALLOWED_CHATSExample:
Test plan
tests/gateway/test_whatsapp_observe_unmentioned_groups.py(17 cases): observe gating matrix (mention / wake-word / reply-to-bot / command / non-allowlisted chat / own message / DM / empty allowlist / feature off), shared-session storage of observed rows, trigger-turn attribution + command sender preservation,WHATSAPP_GROUP_ALLOWED_CHATSadmission of user-less group turns, and history-builder marker recognition.tests/gateway/test_whatsapp_observe_unmentioned_groups.py+ neighboring WhatsApp gating/allowlist suites: 31 passed.pytest tests/gateway -k observe(Telegram observe-mode suites): 30 passed, 2 skipped — no parity regression.