diff --git a/docs/design/sol_initiated_chat_lode1.md b/docs/design/sol_initiated_chat_lode1.md index 6940e38df..53c4b6fb9 100644 --- a/docs/design/sol_initiated_chat_lode1.md +++ b/docs/design/sol_initiated_chat_lode1.md @@ -152,11 +152,12 @@ the critical section where possible. Keep these as distinct named functions in `dedup.py`. `_is_live_for_dedup(dedupe_key, dedupe_window)` walks today's stream looking for -a `sol_chat_request` with matching `dedupe` whose `dedupe_window` has not expired -and whose `request_id` has not been released by `owner_chat_dismissed`. +a `sol_chat_request` whose `dedupe` matches `dedupe_key`, whose window has not +expired, and whose `request_id` is still pending owner engagement. -Dismissal releases dedupe because the owner saw the request and chose to defer. -The reader must map dismissals back to the original request category/request id. +Engagement releases dedupe: `owner_chat_open` releases the matching +`request_id`, and `owner_message` releases pending requests older than the owner +message timestamp (strict `<`). Dismissal is not a dedupe release event. `_is_unresolved_for_supersede() -> request_id | None` walks today's stream and returns the most recent `sol_chat_request` not yet followed by `sol_message` and diff --git a/docs/design/sol_initiated_chat_lode2.md b/docs/design/sol_initiated_chat_lode2.md index c4e1611be..5ee023bce 100644 --- a/docs/design/sol_initiated_chat_lode2.md +++ b/docs/design/sol_initiated_chat_lode2.md @@ -139,11 +139,11 @@ HTML. `solstone/apps/chat/workspace.html:207-215` drops all four sol-initiated kinds in its live allowlist today, and the server partial `solstone/apps/chat/_chat_event.html:1-45` has no branch for them. -Idempotency trade-off: lode-1 dedupe ignores `owner_chat_open` and releases only -on dismiss at `solstone/convey/sol_initiated/dedup.py:56-60`. Repeated reloads -therefore append repeated `owner_chat_open` events. That is acceptable for this -lode because each page load is an engagement signal. Do not add suppression -logic in Lode 2. +Idempotency trade-off: lode-1 dedupe releases on `owner_chat_open` by request id, +and repeated opens are idempotent because dropping an already-absent pending +request is a no-op. Repeated reloads therefore append repeated +`owner_chat_open` events, which is acceptable — each page load is an engagement +signal. No suppression logic in Lode 2. ### D4. Backend context surfaced to chat-bar @@ -502,7 +502,8 @@ Add or extend: - The lode scope wording says `_TRIGGER_KINDS` lists all four kinds. Prep verified `solstone/convey/chat_stream.py:61-66` includes only `sol_chat_request` among the new kinds. This design does not modify `_TRIGGER_KINDS`. -- `owner_chat_open` does not release dedupe. That remains lode-1 behavior. +- `owner_chat_open` releases dedupe by request id; repeated opens remain + idempotent no-ops after the first release. - Repeated `/app/chat/` reloads append repeated `owner_chat_open` events. This is accepted as an engagement signal. - The server-side initial transcript creates anchors for all events, but diff --git a/solstone/convey/sol_initiated/copy.py b/solstone/convey/sol_initiated/copy.py index fc03905e0..55be28ad6 100644 --- a/solstone/convey/sol_initiated/copy.py +++ b/solstone/convey/sol_initiated/copy.py @@ -3,10 +3,11 @@ """Locked literals for sol-initiated chat.""" +KIND_OWNER_CHAT_DISMISSED = "owner_chat_dismissed" +KIND_OWNER_CHAT_OPEN = "owner_chat_open" +KIND_OWNER_MESSAGE = "owner_message" KIND_SOL_CHAT_REQUEST = "sol_chat_request" KIND_SOL_CHAT_REQUEST_SUPERSEDED = "sol_chat_request_superseded" -KIND_OWNER_CHAT_OPEN = "owner_chat_open" -KIND_OWNER_CHAT_DISMISSED = "owner_chat_dismissed" SURFACE_CONVEY = "convey" SOL_PINGED_OFFLINE_TOOLTIP = "sol-pinged but offline — refresh" diff --git a/solstone/convey/sol_initiated/dedup.py b/solstone/convey/sol_initiated/dedup.py index d49449e08..7c1a1809d 100644 --- a/solstone/convey/sol_initiated/dedup.py +++ b/solstone/convey/sol_initiated/dedup.py @@ -8,7 +8,8 @@ from __future__ import annotations import re from solstone.convey.sol_initiated.copy import ( - KIND_OWNER_CHAT_DISMISSED, + KIND_OWNER_CHAT_OPEN, + KIND_OWNER_MESSAGE, KIND_SOL_CHAT_REQUEST, KIND_SOL_CHAT_REQUEST_SUPERSEDED, ) @@ -39,33 +40,41 @@ def _is_live_for_dedup( window_ms: int, now_ms: int, ) -> bool: - """Return whether a matching request is still live for deduplication.""" - requests: dict[str, tuple[str, int]] = {} - released: set[str] = set() + """Live if a still-pending request with matching dedupe is within window. + + Engagement releases pending requests: chat open by request_id, owner message + by timestamp (strict <). Dismissal is not a release event. + """ + pending: list[tuple[str, str, int]] = [] for event in events: kind = event.get("kind") if kind == KIND_SOL_CHAT_REQUEST: request_id = str(event.get("request_id") or "") - if request_id: - requests[request_id] = ( + if not request_id: + continue + pending.append( + ( + request_id, str(event.get("dedupe") or ""), int(event.get("ts", 0) or 0), ) + ) continue - if kind == KIND_OWNER_CHAT_DISMISSED: + if kind == KIND_OWNER_CHAT_OPEN: request_id = str(event.get("request_id") or "") - if request_id in requests: - released.add(request_id) - - for request_id, (request_dedupe, request_ts) in requests.items(): - if request_id in released: + if not request_id: + continue + pending = [item for item in pending if item[0] != request_id] continue - if request_dedupe != dedupe_key: - continue - if request_ts + window_ms > now_ms: - return True - return False + if kind == KIND_OWNER_MESSAGE: + event_ts = int(event.get("ts", 0) or 0) + pending = [item for item in pending if item[2] >= event_ts] + + return any( + request_dedupe == dedupe_key and request_ts + window_ms > now_ms + for _, request_dedupe, request_ts in pending + ) def _is_unresolved_for_supersede(events: list[dict]) -> str | None: diff --git a/tests/test_sol_initiated_dedup.py b/tests/test_sol_initiated_dedup.py index b98ec6fd6..58f738a27 100644 --- a/tests/test_sol_initiated_dedup.py +++ b/tests/test_sol_initiated_dedup.py @@ -5,6 +5,8 @@ import pytest from solstone.convey.sol_initiated.copy import ( KIND_OWNER_CHAT_DISMISSED, + KIND_OWNER_CHAT_OPEN, + KIND_OWNER_MESSAGE, KIND_SOL_CHAT_REQUEST, KIND_SOL_CHAT_REQUEST_SUPERSEDED, ) @@ -45,17 +47,65 @@ def test_live_for_dedup_expires_and_matches_key() -> None: assert _is_live_for_dedup(events, "b", 1_000, 1_500) is False -def test_dismissal_releases_dedup() -> None: +def test_dismissal_does_not_release_dedup() -> None: events = [ _request("r1", ts=1_000, dedupe="a"), { "kind": KIND_OWNER_CHAT_DISMISSED, "ts": 1_500, "request_id": "r1", + "surface": "convey", + "reason": "later", }, ] - assert _is_live_for_dedup(events, "a", 10_000, 2_000) is False + assert _is_live_for_dedup(events, "a", 10_000, 2_000) is True + + +def test_open_releases_dedup() -> None: + events = [ + _request("r1", ts=1_000), + { + "kind": KIND_OWNER_CHAT_OPEN, + "ts": 1_500, + "request_id": "r1", + "surface": "convey", + }, + ] + + assert _is_live_for_dedup(events, "k", 2_000, 2_000) is False + + +def test_owner_message_releases_earlier_pending() -> None: + events = [ + _request("r1", ts=1_000), + { + "kind": KIND_OWNER_MESSAGE, + "ts": 1_500, + "text": "hi", + "app": "convey", + "path": "/", + "facet": "personal", + }, + ] + + assert _is_live_for_dedup(events, "k", 2_000, 2_000) is False + + +def test_owner_message_does_not_release_later_request() -> None: + events = [ + { + "kind": KIND_OWNER_MESSAGE, + "ts": 1_000, + "text": "hi", + "app": "convey", + "path": "/", + "facet": "personal", + }, + _request("r1", ts=1_500), + ] + + assert _is_live_for_dedup(events, "k", 2_000, 2_000) is True def test_unresolved_for_supersede_walks_back() -> None: @@ -72,3 +122,19 @@ def test_unresolved_for_supersede_walks_back() -> None: assert _is_unresolved_for_supersede(events) == "new" assert _is_unresolved_for_supersede([*events, {"kind": "sol_message"}]) is None + + +def test_supersede_walk_unchanged_by_dismissal_and_dedup_remains_live() -> None: + events = [ + _request("r1", ts=1_000), + { + "kind": KIND_OWNER_CHAT_DISMISSED, + "ts": 1_500, + "request_id": "r1", + "surface": "convey", + "reason": "later", + }, + ] + + assert _is_unresolved_for_supersede(events) == "r1" + assert _is_live_for_dedup(events, "k", 2_000, 2_000) is True