From fca78ef9d06ee58567f09a0ee49c29730bb1d630 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Thu, 13 Aug 2026 10:57:25 +0300 Subject: [PATCH] fix(workspace): address the six review findings on #2139 (ent#359) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The row cap was a one-way door, and three documents plus a test said it was not.** `mark_chat_read` creates a row for every chat ever opened — rooms included, which always report `unread: 0` — while unstar only nulled `starred_at` and no DELETE against the table existed anywhere. So a viewer who simply used the Workspace long enough reached the cap through activity they could not undo, and from then on every star returned "unstar some first" forever, with unstarring freeing nothing. The code comment, the flow doc, the requirement and the test all asserted unstar was the recovery path; the test only checked that `starred_at` went NULL and never that a subsequent star succeeded, which is precisely why the false claim survived review twice. Split into two caps, because one number cannot do both jobs: a total-row cap still bounds abuse (neither writer validates that the chat exists — a 404 would be an enumeration oracle), and a separate STARRED-row cap is the one a real user reaches, so unstarring genuinely frees capacity. Unstarring a chat with no read cursor now deletes its row, so the table cannot only ever grow. Docs corrected; the test now asserts the recovery actually works. **The badge could come back on the chat you were reading.** `markRead` fired without being awaited and `refreshThreads` raced it, so `GET /chat-state` could be served before the cursor UPSERT committed and overwrite the optimistic zero. `markRead` now returns its promise and every refresh awaits it. **Route-driven opens never wrote a cursor at all** — only sidebar clicks did. Back/forward, a bookmark and a reload all land in `bootstrap`/the route watcher, which is the commonest way into a chat, so the sidebar kept badging the conversation on screen through every reload. **A reattached reply never announced itself.** `deliver()` emits `sessions-changed`; `reattach()` pushed the reply and returned silently, so a user who reloaded into a running turn watched the reply land and kept the badge. **The star was mouse-only.** Keydown from the focused star bubbled to the row's `role="button"` handler, which `preventDefault`ed the button's own activation and opened the chat instead — so Tab-then-Enter navigated rather than starred. **Removed an unreachable fallback** in `get_chat_state`: the unread query INNER JOINs the state table, so the "cursor cleared out from under us" branch could never fire. A safety net that cannot fire is worse than none. Related to abilityai/trinity-enterprise#359 Co-Authored-By: Claude Opus 5 (1M context) --- .../feature-flows/workspace-sidebar-ia.md | 14 +++- docs/memory/requirements/core-agent.md | 6 +- src/backend/client_portal/db.py | 65 +++++++++++++-- src/backend/client_portal/service.py | 22 ++--- .../components/portal/PortalConversation.vue | 9 ++- .../components/portal/PortalStarButton.vue | 7 ++ src/frontend/src/views/Portal.vue | 22 +++-- tests/unit/test_ent359_portal_chat_state.py | 80 ++++++++++++++++--- 8 files changed, 189 insertions(+), 36 deletions(-) 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 0f6d7ba9f..deb9100db 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 5ed798955..759be3c23 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 14ae6f38b..8b3e52901 100644 --- a/src/backend/client_portal/service.py +++ b/src/backend/client_portal/service.py @@ -2093,24 +2093,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} @@ -2124,8 +2122,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 6bffe8fc2..769e273e1 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 }) } @@ -512,8 +511,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 @@ -541,14 +542,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) ---------------------------------------------- @@ -575,6 +580,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 — @@ -622,6 +631,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):