Review: APPROVE (still valid — no rebase needed)

Reviewed core logic only (gateway/platforms/matrix.py, session.py, config.py, slash_commands.py); skipped the 16 locales/*.yaml + 2 website/docs files as i18n/docs noise.

Currency / mergeability

Despite the May-01 open date, this is current: head 3a9439c is rebased onto a72bb037, which is the present origin/main tip (0 commits between merge-base and main). GitHub reports MERGEABLE. No rebase required.

Core claim verified — room-isolation bug is still present on main

The 'isolate room context' fix is not superseded. On current main:

• DM misclassification still live: get_chat_info uses chat_type = "dm" if await self._is_dm_room(chat_id) else "group" — purely _dm_rooms/m.direct cache, with no named-room override. A named two-member project room can still be misclassified as a DM.
• No MATRIX_SESSION_SCOPE (auto|room|thread) on main.
• No MatrixRoomIdentity / is_direct_account_data authority logic.
• No cross-room /resume guard in slash_commands.py (no same-room/--cross-room/--all handling).

This PR adds exactly these: has_explicit_name so named rooms win over stale m.direct; scope routing at matrix.py ~L1529-1541; room-boundary system-prompt injection in session.py (build_session_context_prompt); fail-closed cross-room resume. The PR body's own framing is accurate — it does not re-claim handle_sync() inbound dispatch (already on main via #7914); merge value is the room/session-isolation class of bugs.

Notes / non-blocking

1. matrix.py is +1499/-290 in this single file — large but cohesive (new class + scope plumbing). Recommend the stack (#18506/#18507) stays draft until this lands to avoid re-rebasing churn.
2. session.py change correctly honors redact_pii (_hash_chat_id) for room/thread IDs in the prompt — good.

Verdict: APPROVE. Bug confirmed still live on main, change is well-scoped, branch is current and mergeable.

Code Review Summary

PR #18505 — fix(matrix): isolate room context and restore reliable inbound dispatch
Priority: P1
Verdict: Request Changes

Critical

** exposes full — potential PII / info-leak in shared rooms**
In gateway/slash_commands.py, the new Matrix scope block appends:

t("gateway.status.matrix_scope_key", session_key=session_key)

Session keys typically encode the room ID and/or user ID. Exposing the raw key in a shared/non-DM Matrix room leaks routing metadata to all room participants. Either truncate (e.g. first 8 chars + …) or gate this line on the DM/admin context check that guards other sensitive /status fields.

Warnings

1. Type annotation mismatch on _MatrixModelPickerPrompt.bot_reaction_events

@dataclass
class _MatrixModelPickerPrompt:
    bot_reaction_events: dict[str, str] = None   # ← annotation says dict, default is None

The __post_init__ guard correctly initialises the field at runtime, but type checkers (mypy/pyright) will flag this as dict[str, str] | None conflict on the annotation. Fix: either use field(default_factory=dict) (cleanest) or annotate as dict[str, str] | None = None.

2. PR scope significantly exceeds the stated P1 bug fix — increases review risk
The 2159-line diff bundles code clearly belonging to PR 2/3 and PR 3/3:

• _MatrixHtmlSanitizer (~70 lines) — Matrix rendering safety; PR 3/3's own scope per the stack note
• _MatrixModelPickerPrompt + _MATRIX_MODEL_PICKER_REACTIONS — reaction/model-picker UI; PR 2/3 scope
• _MATRIX_CAPABILITIES dict + get_matrix_capabilities() — capability registry; informational feature
• Env var stubs for PR 2/3 tools: MATRIX_TOOLS_ALLOW_REDACTION, MATRIX_TOOLS_ALLOW_INVITES, MATRIX_TOOLS_ALLOW_ROOM_CREATE, MATRIX_ALLOW_PUBLIC_ROOMS

These are additive, non-breaking additions, but they enlarge the blast radius of a P1 merge and make it harder to bisect if a regression surfaces. Recommend extracting them — or at minimum tag the bundled items explicitly in the PR description so merge reviewers know what can be reverted independently.

3. Non-English locale files contain untranslated English strings
All 15 non-English locales (af, de, es, fr, ga, hu, it, ja, ko, pt, ru, tr, uk, zh-hant, zh) appear to carry the new Matrix-specific keys verbatim in English (including the full message strings, not just placeholders). This is acceptable for a P1 merge but should be tracked as a follow-up localization task.

4. _gateway_session_origin_for_id accesses _entries (private attribute)

entries = getattr(self.session_store, "_entries", {}) or {}

Fragile coupling to session store internals. If _entries is renamed or restructured the cross-room guard silently returns None and the room boundary check degrades to allow_cross_room=False behavior (which is fail-closed, so no safety regression — but it would silently break UX). Consider exposing a public lookup_by_session_id() method on the session store.

Looks Good

Core room isolation: MatrixRoomIdentity + m.direct DM classification ✅
The named-room-wins-over-stale-account-data logic in _resolve_room_identity() is correct. conflict=True when is_direct and has_explicit_name is a good diagnostic flag. Cache invalidation on _refresh_dm_cache() (clearing both dicts) is properly placed.

_dispatch_sync handles both sync and async handle_sync() ✅

tasks = client.handle_sync(sync_data)
if inspect.isawaitable(tasks):
    tasks = await tasks
if tasks:
    await asyncio.gather(*tasks)

Correct. The inspect.isawaitable guard future-proofs against upstream API changes and the gather correctly fans out concurrent task execution.

/resume same-room guard is fail-closed ✅
Cross-room resume requires explicit --cross-room or --all flags. Default behavior blocks. shlex.split for argument parsing is the right call here (handles quoted session names with spaces). The --all and --cross-room escape hatches are well-scoped.

config.py cleans up duplicate allowed_rooms registration ✅
Old code had free_response_rooms AND a duplicate ar block both reaching MATRIX_ALLOWED_ROOMS. The refactor deduplicates correctly and adds the missing allowed_users YAML-to-env mapping.

i18n catalog parity (all 16 locales updated) ✅
All locale files carry the new Matrix-specific slash-command strings in lockstep. No locale is left missing keys that would cause t() fallback failures at runtime.

Room identity LRU-style eviction is correct for the scale ✅
O(n) eviction at 256-entry capacity is negligible. The oldest-timestamp eviction is correct and deterministic.

Reviewed by Hermes Agent

Code Review Summary

PR #18505 — fix(matrix): isolate room context and restore reliable inbound dispatch
Priority: P1
Verdict: Request Changes

Critical

/status exposes full session_key — potential PII / info-leak in shared rooms
In gateway/slash_commands.py, the new Matrix scope block appends:

t("gateway.status.matrix_scope_key", session_key=session_key)

Session keys typically encode the room ID and/or user ID. Exposing the raw key in a shared/non-DM Matrix room leaks routing metadata to all room participants. Either truncate (e.g. first 8 chars + …) or gate this line on the DM/admin context check that guards other sensitive /status fields.

Warnings

1. Type annotation mismatch on _MatrixModelPickerPrompt.bot_reaction_events

@dataclass
class _MatrixModelPickerPrompt:
    bot_reaction_events: dict[str, str] = None   # annotation says dict, default is None

The __post_init__ guard correctly initialises the field at runtime, but mypy/pyright will flag this as a dict[str, str] | None conflict. Fix: use field(default_factory=dict) (cleanest) or annotate as dict[str, str] | None = None.

2. PR scope significantly exceeds the stated P1 bug fix — increases review risk
The 2159-line diff bundles code clearly belonging to PR 2/3 and PR 3/3:

• _MatrixHtmlSanitizer (~70 lines) — Matrix rendering safety; PR 3/3 scope per the stack note
• _MatrixModelPickerPrompt + _MATRIX_MODEL_PICKER_REACTIONS — reaction/model-picker UI; PR 2/3 scope
• _MATRIX_CAPABILITIES dict + get_matrix_capabilities() — capability registry
• Env var stubs for PR 2/3 tools: MATRIX_TOOLS_ALLOW_REDACTION, MATRIX_TOOLS_ALLOW_INVITES, MATRIX_TOOLS_ALLOW_ROOM_CREATE, MATRIX_ALLOW_PUBLIC_ROOMS

These are additive/non-breaking, but they enlarge the blast radius of a P1 merge and make it harder to bisect if a regression surfaces. Recommend extracting them or explicitly tagging them in the PR description so merge reviewers know what can be reverted independently.

3. Non-English locale files contain untranslated English strings
All 15 non-English locales (af, de, es, fr, ga, hu, it, ja, ko, pt, ru, tr, uk, zh-hant, zh) carry the new Matrix-specific keys verbatim in English. Acceptable for a P1 merge but should be tracked as a follow-up i18n task.

4. _gateway_session_origin_for_id accesses _entries (private attribute)

entries = getattr(self.session_store, "_entries", {}) or {}

Fragile coupling to session store internals. If _entries is renamed, the same-room guard silently degrades to fail-closed behavior (safe but silently broken UX). Consider exposing a public lookup_by_session_id() on the session store.

Looks Good

Core room isolation: MatrixRoomIdentity + m.direct DM classification ✅
Named-room-wins-over-stale-account-data logic in _resolve_room_identity() is correct. conflict=True when is_direct and has_explicit_name is a good diagnostic flag. Cache invalidation on _refresh_dm_cache() is correctly placed.

_dispatch_sync handles both sync and async handle_sync() ✅

tasks = client.handle_sync(sync_data)
if inspect.isawaitable(tasks):
    tasks = await tasks
if tasks:
    await asyncio.gather(*tasks)

Correct. The inspect.isawaitable guard future-proofs against upstream API changes; gather correctly fans out concurrent task execution.

/resume same-room guard is fail-closed ✅
Cross-room resume requires explicit --cross-room or --all flags. shlex.split for argument parsing correctly handles quoted session names with spaces. The escape hatches are well-scoped.

config.py cleans up duplicate allowed_rooms registration ✅
Old code had a duplicate ar block both reaching MATRIX_ALLOWED_ROOMS. The refactor deduplicates correctly and adds the missing allowed_users YAML-to-env mapping.

i18n catalog parity (all 16 locales updated) ✅
All locale files carry the new Matrix-specific keys in lockstep — no locale is left missing keys that would cause t() fallback failures at runtime.

Room identity LRU-style eviction is correct for the scale ✅
O(n) eviction at 256-entry capacity is negligible. Oldest-timestamp eviction is correct and deterministic.

Reviewed by Hermes Agent

All three blockers from my last review are resolved:

• /status session_key leak — closed. The _session_key_fingerprint() helper now emits sha256:<12> and the test explicitly asserts session_key not in result and the truncated prefix is absent too. ✓
• bot_reaction_events annotation/default mismatch — fixed with field(default_factory=dict). ✓
• /resume reaching into _entries — replaced with the public SessionStore.lookup_by_session_id() helper, with a fallback for test doubles. ✓

354 passed, 1 skipped on the local Matrix suite; CI green. Approving the foundation layer — stacked PRs 2/3 and 3/3 can proceed.