fix(workspace): the six #2139 review findings, which missed the merge (ent#359)

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
 

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

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)

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())
 
 

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

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
   >
     <svg
       :class="dense ? 'w-4 h-4' : 'w-5 h-5'"
@@ -35,6 +36,12 @@
  * the setup where an inline copy per site drifts: three icons, three hover
  * rules, three `aria-pressed` states to keep honest.
  *
+ * `@keydown.stop` matters as much as `@click.stop`: the sidebar row is a
+ * `role="button"` div with `@keydown.enter.prevent`/`@keydown.space.prevent`,
+ * and a keydown from this button bubbles into it — the row would preventDefault
+ * the button's own activation and open the chat instead, leaving the star
+ * reachable by mouse only. The focus styling below assumes keyboard use works.
+ *
  * `revealOnHover` is for the dense sidebar row, where a permanently visible
  * outline star on every row is noise. It applies only when NOT starred — a star
  * that is set is state, and state that appears only on hover is state the user

src/frontend/src/views/Portal.vue

@@ -544,8 +544,7 @@ function onSessionAdopted(id) {
   // A thread you are actively talking in is by definition read. This is also
   // what gives a brand-new thread its read cursor, so the very next reply the
   // user does NOT see is the first thing that badges.
-  markRead('thread', id)
-  refreshThreads()
+  markRead('thread', id).then(refreshThreads)
 }
 function usePlaybook(text) { prefill.value = ''; nextTick(() => { 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=<name>` 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()

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):