diff --git a/docs/memory/feature-flows/workspace-sidebar-ia.md b/docs/memory/feature-flows/workspace-sidebar-ia.md index 13572c3da..178a943b0 100644 --- a/docs/memory/feature-flows/workspace-sidebar-ia.md +++ b/docs/memory/feature-flows/workspace-sidebar-ia.md @@ -124,10 +124,16 @@ destination with its own content, not a shortcut to a conversation). - **No existence check on `chat_id`, on purpose.** A 404 for an unknown chat would answer "does chat X exist?" for every id in the install (invariant #8). The write lands in the caller's own namespace, so an unknown id gains them - nothing — a per-viewer row cap (`MAX_CHAT_STATE_ROWS`) bounds the write - instead of validation. The cap applies only to writes that **create** a row: - capping updates would freeze a user at the ceiling out of unstarring, which is - the only action that gets them back under it. + nothing — per-viewer caps bound the write instead of validation. + + **Two caps, because one number cannot do both jobs.** `MAX_CHAT_STATE_ROWS` + (1000) bounds abuse: any row, star or read cursor. `MAX_STARRED_CHATS` (200) + bounds the starred set and is the only one a real user reaches. They have to + be separate — a read cursor is written for every chat ever opened, so a single + total cap gets consumed by ordinary use, and then every star returns + *"unstar some first"* while unstarring frees nothing, forever. Counting only + starred rows makes that advice true, and unstarring a chat with no read cursor + deletes its row outright so the table cannot only ever grow. ## Tests diff --git a/docs/memory/requirements/core-agent.md b/docs/memory/requirements/core-agent.md index a7ab62a5c..09fcd1abe 100644 --- a/docs/memory/requirements/core-agent.md +++ b/docs/memory/requirements/core-agent.md @@ -396,7 +396,11 @@ `PUT|DELETE .../chat-state/{kind}/{id}/star`, `POST .../chat-state/{kind}/{id}/read`. No roster gate (every row is keyed by the caller's own email) and no existence check on the id — a 404 for an unknown chat would be an enumeration oracle - (invariant #8); a per-viewer row cap bounds the write instead. + (invariant #8); two per-viewer caps bound the write instead. A total-row cap + (abuse) and a separate **starred**-row cap: read cursors accrue from ordinary + use, so a single cap would be spent by activity the user cannot undo, making + the 409's "unstar some first" advice false. Unstarring a chat that carries no + read cursor deletes its row. - **Known gap**: rooms report `unread: 0`. A room keeps its own seq cursor, and reconciling the two cursor models is follow-up work; stars work for both kinds. - **Not this issue**: opening an agent's own **page** (a destination with its own diff --git a/src/backend/client_portal/db.py b/src/backend/client_portal/db.py index b45dfffd3..26f35e906 100644 --- a/src/backend/client_portal/db.py +++ b/src/backend/client_portal/db.py @@ -511,11 +511,21 @@ def list_agent_share_emails(agent_name: str) -> list[str]: CHAT_KINDS = ("thread", "room") -# A ceiling on rows one user can create. `star` and `read` both write a row, and -# neither validates that the chat exists (see the router: a 404 for an unknown id -# would be an enumeration oracle). Without a cap, that is an unbounded write -# primitive for anyone holding a portal session. +# Two ceilings, because one number cannot do both jobs. +# +# MAX_CHAT_STATE_ROWS bounds ABUSE: `star` and `read` both write a row and +# neither validates that the chat exists (a 404 for an unknown id would be an +# enumeration oracle), so without it a portal session is an unbounded write +# primitive. +# +# MAX_STARRED_CHATS bounds the STARRED set, and it is the one a real user can +# reach. It has to be separate: read cursors accumulate from ordinary use (one +# per chat ever opened, rooms included), so a single total-row cap would be +# consumed by activity the user cannot undo — every star click 409ing forever +# with "unstar some first" while unstarring frees nothing. Counting only starred +# rows makes that advice true. MAX_CHAT_STATE_ROWS = 1000 +MAX_STARRED_CHATS = 200 def get_chat_state(client_email: str) -> list[dict]: @@ -586,12 +596,55 @@ def _upsert_chat_state(client_email: str, chat_kind: str, chat_id: str, def set_chat_star(client_email: str, chat_kind: str, chat_id: str, starred: bool, now: str) -> None: - """Star or unstar one chat for one user. Unstar keeps the row — it still - carries the read cursor, and dropping it would mark the chat unread again.""" + """Star or unstar one chat for one user. + + Unstar keeps the row when it still carries a read cursor — dropping that + would mark the chat unread again — but DELETES it when there is nothing + left to remember. Without that, a row could only ever be created: the star + cap below counts starred rows, but the table itself would grow forever from + ids that were starred once and unstarred. + """ + if not starred and not _has_read_cursor(client_email, chat_kind, chat_id): + delete_chat_state(client_email, chat_kind, chat_id) + return _upsert_chat_state(client_email, chat_kind, chat_id, now, starred_at=now if starred else None) +def _has_read_cursor(client_email: str, chat_kind: str, chat_id: str) -> bool: + stmt = text( + "SELECT 1 FROM enterprise_portal_chat_state " + "WHERE client_email = :email AND chat_kind = :kind AND chat_id = :id " + " AND last_read_at IS NOT NULL" + ) + with get_engine().connect() as conn: + return conn.execute(stmt, { + "email": (client_email or "").lower(), "kind": chat_kind, "id": chat_id, + }).first() is not None + + +def delete_chat_state(client_email: str, chat_kind: str, chat_id: str) -> None: + stmt = text( + "DELETE FROM enterprise_portal_chat_state " + "WHERE client_email = :email AND chat_kind = :kind AND chat_id = :id" + ) + with get_engine().begin() as conn: + conn.execute(stmt, { + "email": (client_email or "").lower(), "kind": chat_kind, "id": chat_id, + }) + + +def count_starred_rows(client_email: str) -> int: + """Starred rows only — what the star cap counts, so unstarring is genuinely + the way back under it.""" + stmt = text( + "SELECT COUNT(*) FROM enterprise_portal_chat_state " + "WHERE client_email = :email AND starred_at IS NOT NULL" + ) + with get_engine().connect() as conn: + return int(conn.execute(stmt, {"email": (client_email or "").lower()}).scalar() or 0) + + def mark_chat_read(client_email: str, chat_kind: str, chat_id: str, now: str) -> None: """Advance one chat's read cursor for one user.""" _upsert_chat_state(client_email, chat_kind, chat_id, now, last_read_at=now) diff --git a/src/backend/client_portal/service.py b/src/backend/client_portal/service.py index 87f5ebd1f..22c0ff585 100644 --- a/src/backend/client_portal/service.py +++ b/src/backend/client_portal/service.py @@ -2127,24 +2127,22 @@ def get_chat_state(email: str) -> dict: """ rows = db.get_chat_state(email) unread = db.count_unread_by_session(email) - seen = set() chats = [] for r in rows: kind, cid = r.get("chat_kind"), r.get("chat_id") if not kind or not cid: continue - seen.add((kind, cid)) chats.append({ "kind": kind, "id": cid, "starred": bool(r.get("starred_at")), "unread": unread.get(cid, 0) if kind == "thread" else 0, }) - # A thread can have unread messages without a state row only if the cursor - # was cleared out from under us; carry it anyway rather than losing a count. - for sid, n in unread.items(): - if ("thread", sid) not in seen: - chats.append({"kind": "thread", "id": sid, "starred": False, "unread": n}) + # No fallback for "unread without a state row": `count_unread_by_session` + # INNER JOINs the state table and requires `last_read_at IS NOT NULL`, so + # every session it can return already has a row `get_chat_state` yielded. + # The loop that used to be here could never append, and a safety net that + # cannot fire is worse than none — it reads as protection that exists. return {"chats": chats} @@ -2158,8 +2156,14 @@ def set_chat_star(email: str, chat_kind: str, chat_id: str, starred: bool) -> No bounds the write instead. """ kind, cid = _validate_chat_ref(chat_kind, chat_id) - if _would_create_row_past_cap(email, kind, cid): - raise ClientPortalError(409, "Too many saved chats — unstar some first") + if starred: + # Counts STARRED rows, so unstarring is genuinely the way back under it. + # A total-row cap here would be unreachable-by-recovery: read cursors + # accumulate from ordinary use and unstar cannot remove them. + if db.count_starred_rows(email) >= db.MAX_STARRED_CHATS: + raise ClientPortalError(409, "Too many saved chats — unstar some first") + if _would_create_row_past_cap(email, kind, cid): + raise ClientPortalError(409, "Too much saved chat state — open fewer new chats") db.set_chat_star(email, kind, cid, starred, utc_now_iso()) diff --git a/src/frontend/src/components/portal/PortalConversation.vue b/src/frontend/src/components/portal/PortalConversation.vue index ea3c2f4d5..f6943b224 100644 --- a/src/frontend/src/components/portal/PortalConversation.vue +++ b/src/frontend/src/components/portal/PortalConversation.vue @@ -269,7 +269,14 @@ async function reattach(executionId) { try { await store.streamPortalExecution(props.agent.name, executionId, onStreamEvent) const data = await awaitPersistedReply(currentSessionId.value, baseline) - if (data?.response) messages.value.push({ role: 'assistant', content: data.response }) + if (data?.response) { + messages.value.push({ role: 'assistant', content: data.response }) + // A reattached reply is still a reply the user just watched land, so it + // has to announce itself like `deliver()` does. Without this the thread + // keeps its server-side unread count and the sidebar badges the + // conversation on screen. + emit('sessions-changed', currentSessionId.value) + } } catch { /* the reply lands in history on the next load */ } finally { sending.value = false diff --git a/src/frontend/src/components/portal/PortalStarButton.vue b/src/frontend/src/components/portal/PortalStarButton.vue index 5276a737c..cda803b85 100644 --- a/src/frontend/src/components/portal/PortalStarButton.vue +++ b/src/frontend/src/components/portal/PortalStarButton.vue @@ -14,6 +14,7 @@ :aria-label="starred ? 'Unstar this chat' : 'Star this chat'" :aria-pressed="starred" @click.stop="$emit('toggle')" + @keydown.stop > { prefill.value = text }) } @@ -586,8 +585,10 @@ function onConversationTurnDone(sessionId) { // arrives unseen. Marking read unconditionally cleared exactly the badge the // feature exists to show, and made it near-unreachable in normal use. const open = activeSessionId.value || pendingSession.value - if (shouldMarkTurnRead(sessionId, open)) markRead('thread', sessionId) - return refreshThreads() + return (shouldMarkTurnRead(sessionId, open) + ? markRead('thread', sessionId) + : Promise.resolve() + ).then(refreshThreads) } // Optimistic: a star is a personal bookmark, and waiting on a round trip to @@ -615,14 +616,18 @@ async function toggleStar(t) { // Opening a chat is what "reading" it means here. Clear the badge locally first // so the count does not linger for a round trip, then persist. +// Returns the write promise. Callers that refresh afterwards MUST await it: +// `GET /chat-state` racing the cursor UPSERT overwrites the optimistic zero +// with a stale count, and the badge comes back on the conversation the user is +// reading — possibly for minutes, until the next refresh. function markRead(kind, id) { - if (!id) return + if (!id) return Promise.resolve() const key = `${kind}:${id}` if (chatState.value[key]?.unread) { chatState.value = { ...chatState.value, [key]: { ...chatState.value[key], unread: 0 } } threads.value = decorate(threads.value) } - store.markChatRead(kind, id) + return store.markChatRead(kind, id) } // ---- Cross-chat search (sidebar) ---------------------------------------------- @@ -649,6 +654,10 @@ watch([() => route.params.sessionId, () => threads.value.length], () => { if (pendingSession.value === sid && activeAgentName.value) return const known = threads.value.find((t) => (t.id || t.session_id) === sid) if (known) { activeAgentName.value = known.agent_name; pendingSession.value = sid; convGen.value++ } + // Opening by ROUTE is an open. Back/forward, a bookmark and a reload all land + // here rather than in `openThread`, and it is the commonest way in — without + // this the sidebar badges the conversation on screen, through every reload. + markRead('thread', sid) }) // ent#358: `/workspace?agent=` opens that agent's conversation directly — @@ -696,6 +705,7 @@ async function bootstrap() { if (known) { activeAgentName.value = known.agent_name; pendingSession.value = sid } else pendingSession.value = sid // let the conversation resolve/load it convGen.value++ + markRead('thread', sid) // a deep-linked open is still an open return } resolveAgentQuery() diff --git a/tests/unit/test_ent359_portal_chat_state.py b/tests/unit/test_ent359_portal_chat_state.py index 3bb2d6eff..a19f95652 100644 --- a/tests/unit/test_ent359_portal_chat_state.py +++ b/tests/unit/test_ent359_portal_chat_state.py @@ -222,11 +222,11 @@ def test_starring_an_unknown_chat_succeeds_rather_than_leaking_existence(chat_db assert [c["chat_id"] for c in pdb.get_chat_state(ALICE)] == ["no-such-chat"] -def test_the_row_cap_stops_new_rows(chat_db, monkeypatch): +def test_the_star_cap_stops_new_stars(chat_db, monkeypatch): from client_portal import db as pdb from client_portal import service as svc - monkeypatch.setattr(pdb, "MAX_CHAT_STATE_ROWS", 2) + monkeypatch.setattr(pdb, "MAX_STARRED_CHATS", 2) svc.set_chat_star(ALICE, "thread", "t1", True) svc.set_chat_star(ALICE, "thread", "t2", True) @@ -235,24 +235,86 @@ def test_the_row_cap_stops_new_rows(chat_db, monkeypatch): assert e.value.status_code == 409 +def test_unstarring_actually_gets_you_back_under_the_cap(chat_db, monkeypatch): + """The claim the 409 makes ("unstar some first") has to be TRUE. + + It was not. The cap counted every row, `mark_chat_read` creates one per chat + ever opened, and unstar kept the row — so a viewer who had simply used the + Workspace long enough hit a permanent 409 with no action that could clear + it. The old test asserted only that `starred_at` went NULL and never that a + subsequent star succeeded, which is exactly why the false claim survived. + """ + from client_portal import db as pdb + from client_portal import service as svc + + monkeypatch.setattr(pdb, "MAX_STARRED_CHATS", 2) + svc.set_chat_star(ALICE, "thread", "t1", True) + svc.set_chat_star(ALICE, "thread", "t2", True) + with pytest.raises(svc.ClientPortalError): + svc.set_chat_star(ALICE, "thread", "t3", True) + + svc.set_chat_star(ALICE, "thread", "t1", False) # the advertised recovery + + svc.set_chat_star(ALICE, "thread", "t3", True) # ...must now work + starred = {r["chat_id"] for r in pdb.get_chat_state(ALICE) if r["starred_at"]} + assert starred == {"t2", "t3"} + + +def test_read_cursors_do_not_consume_the_star_cap(chat_db, monkeypatch): + """Opening chats is ordinary use and must not spend a budget the user can + only free by unstarring.""" + from client_portal import db as pdb + from client_portal import service as svc + + monkeypatch.setattr(pdb, "MAX_STARRED_CHATS", 2) + for i in range(10): + svc.mark_chat_read(ALICE, "thread", f"opened-{i}") + + svc.set_chat_star(ALICE, "thread", "keep", True) # must not raise + assert any(r["chat_id"] == "keep" and r["starred_at"] for r in pdb.get_chat_state(ALICE)) + + +def test_unstarring_a_never_opened_chat_removes_its_row(chat_db): + """Nothing left to remember ⇒ no row. Otherwise a star-then-unstar leaves + permanent residue and the table only ever grows.""" + from client_portal import db as pdb + from client_portal import service as svc + + svc.set_chat_star(ALICE, "thread", "t1", True) + svc.set_chat_star(ALICE, "thread", "t1", False) + + assert pdb.get_chat_state(ALICE) == [] + + +def test_unstarring_a_read_chat_keeps_its_cursor(chat_db): + """The row survives when it still carries a read cursor — dropping it would + mark the whole thread unread again, so unstarring would light up a badge.""" + from client_portal import db as pdb + from client_portal import service as svc + + svc.mark_chat_read(ALICE, "thread", "t1") + svc.set_chat_star(ALICE, "thread", "t1", True) + svc.set_chat_star(ALICE, "thread", "t1", False) + + row = pdb.get_chat_state(ALICE)[0] + assert row["starred_at"] is None and row["last_read_at"] is not None + + def test_the_cap_never_freezes_a_row_the_user_already_owns(chat_db, monkeypatch): """A cap that applied to updates would leave a user at the ceiling unable to - unstar (the only action that gets them back under it) or to advance a read - cursor they already have — punishing them for state they legitimately - accumulated.""" + advance a read cursor they already have — punishing them for state they + legitimately accumulated.""" from client_portal import db as pdb from client_portal import service as svc monkeypatch.setattr(pdb, "MAX_CHAT_STATE_ROWS", 2) - svc.set_chat_star(ALICE, "thread", "t1", True) - svc.set_chat_star(ALICE, "thread", "t2", True) + svc.mark_chat_read(ALICE, "thread", "t1") + svc.mark_chat_read(ALICE, "thread", "t2") svc.mark_chat_read(ALICE, "thread", "t1") # update, not insert - svc.set_chat_star(ALICE, "thread", "t1", False) # the way back under the cap rows = {r["chat_id"]: r for r in pdb.get_chat_state(ALICE)} assert rows["t1"]["last_read_at"] is not None - assert rows["t1"]["starred_at"] is None def test_mark_read_at_the_cap_is_a_no_op_not_an_error(chat_db, monkeypatch):