From d8afd0fe82dce51a99260a36e687ced91490950c Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Wed, 16 Sep 2026 16:37:16 -0600 Subject: [PATCH 1/4] feat(feedback): content-free thumbs on assistant messages joined to cost rows (offload PR-7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A thumbs up/down on assistant messages, persisted as an F# row on the sessions-metadata table keyed (sessionId, messageId) beside the C# cost row, so the admin session profile joins the outcome signal to the call's turn class in one key lookup. Value ±1, timestamp and an optional reason code from a closed enum — never free text, by the request model and the storage layer both. - apis.shared.sessions.feedback: F#{sessionId}#{messageId} rows (documented with the C#/D# schemas in metadata.py); a second click replaces; session rollups thumbsUp/thumbsDown ADDed while COST_DIAGNOSTICS_ENABLED. - GET /sessions/{id}/messages merges the thumb onto metadata.feedback so a reload restores the pressed state. - PUT/DELETE /sessions/{id}/messages/{messageId}/feedback via get_current_user_from_session; MESSAGE_FEEDBACK_ENABLED kill switch (404). - Admin profile: feedback {up, down, byTurnClass, unjoined} + dataCoverage. feedback; byTurnClass is null ("not tracked") until the C# rows carry hasDocuments / documentDigests / documentReads (#1137). - content_policy: FEEDBACK_ROW_PROJECTION walked by the existing test; session projection gains thumbsUp/thumbsDown; call projection gains the three turn-class fields. - SPA: thumbs pair with reason chips in the message actions, optimistic with rollback; Feedback tile with down-thumb rate by turn class on the anatomy page. Co-Authored-By: Claude Fable 5.1 --- .../src/apis/app_api/admin/costs/models.py | 42 ++++ .../src/apis/app_api/admin/costs/service.py | 102 ++++++++ backend/src/apis/app_api/messages/models.py | 34 ++- backend/src/apis/app_api/sessions/routes.py | 87 ++++++- backend/src/apis/shared/feature_flags.py | 20 ++ .../shared/observability/content_policy.py | 23 ++ backend/src/apis/shared/sessions/feedback.py | 225 ++++++++++++++++++ backend/src/apis/shared/sessions/metadata.py | 30 +++ backend/src/apis/shared/sessions/models.py | 34 ++- .../apis/shared/storage/dynamodb_storage.py | 43 ++++ .../costs/test_session_profile_service.py | 85 +++++++ .../app_api/test_message_feedback_routes.py | 92 +++++++ .../costs/test_content_free_projections.py | 24 ++ backend/tests/shared/test_message_feedback.py | 188 +++++++++++++++ .../admin/costs/models/admin-cost.models.ts | 25 ++ .../costs/pages/session-cost-anatomy.page.ts | 42 ++++ .../costs/pages/session-profile.util.spec.ts | 37 +++ .../admin/costs/pages/session-profile.util.ts | 37 +++ .../message-actions.component.spec.ts | 101 +++++++- .../components/message-actions.component.ts | 130 +++++++++- .../session/services/models/message.model.ts | 15 ++ .../session/message-feedback.service.spec.ts | 114 +++++++++ .../session/message-feedback.service.ts | 161 +++++++++++++ 23 files changed, 1680 insertions(+), 11 deletions(-) create mode 100644 backend/src/apis/shared/sessions/feedback.py create mode 100644 backend/tests/apis/app_api/test_message_feedback_routes.py create mode 100644 backend/tests/shared/test_message_feedback.py create mode 100644 frontend/ai.client/src/app/session/services/session/message-feedback.service.spec.ts create mode 100644 frontend/ai.client/src/app/session/services/session/message-feedback.service.ts diff --git a/backend/src/apis/app_api/admin/costs/models.py b/backend/src/apis/app_api/admin/costs/models.py index 1754da8ad..a0bfe98af 100644 --- a/backend/src/apis/app_api/admin/costs/models.py +++ b/backend/src/apis/app_api/admin/costs/models.py @@ -421,6 +421,44 @@ class ToolCensusEntry(BaseModel): errors: int = 0 +class FeedbackCounts(BaseModel): + """Thumbs on one bucket of calls: ``up`` / ``down`` are counts of live + feedback rows, never the text of anything.""" + model_config = ConfigDict(populate_by_name=True) + + up: int = 0 + down: int = 0 + + +class FeedbackByTurnClass(BaseModel): + """Feedback split by the call's document turn class (document-context + offload spec §6.1): *full* (``hasDocuments``), *retrieved* + (``documentReads.pages > 0``), *digestOnly* (``documentDigests > 0``), + else *none* — in that precedence, since a retrieving call still holds + the digest. ``n`` per class is ``up + down``; the down-thumb rate is + ``down / n``.""" + model_config = ConfigDict(populate_by_name=True) + + full: FeedbackCounts = Field(default_factory=FeedbackCounts) + digest_only: FeedbackCounts = Field(default_factory=FeedbackCounts, alias="digestOnly") + retrieved: FeedbackCounts = Field(default_factory=FeedbackCounts) + none: FeedbackCounts = Field(default_factory=FeedbackCounts) + + +class FeedbackProfile(BaseModel): + """The outcome signal joined to the session's cost rows. ``byTurnClass`` + is ``None`` when no cost row carries the turn-class fields (they arrive + with #1137; rows written before it have none) — "not tracked", not zero. + ``unjoined`` counts thumbs whose message has no cost row at all (the row + expired, or the call was never recorded).""" + model_config = ConfigDict(populate_by_name=True) + + up: int = 0 + down: int = 0 + by_turn_class: Optional[FeedbackByTurnClass] = Field(None, alias="byTurnClass") + unjoined: int = 0 + + class DataCoverage(BaseModel): """Which optional signals this session actually has, so the UI can say "not tracked" instead of rendering an honest-looking zero.""" @@ -433,6 +471,8 @@ class DataCoverage(BaseModel): prefix_tokens: bool = Field(False, alias="prefixTokens") window_trim: bool = Field(False, alias="windowTrim") compaction_events: bool = Field(False, alias="compactionEvents") + # Any F# row, or a session rollup written while diagnostics were on. + feedback: bool = False class SessionProfile(BaseModel): @@ -475,3 +515,5 @@ class SessionProfile(BaseModel): ) # The summary's token size at the most recent compaction decision. last_summary_tokens: Optional[int] = Field(None, alias="lastSummaryTokens") + # Thumbs up/down joined to the cost rows by (sessionId, messageId). + feedback: FeedbackProfile = Field(default_factory=FeedbackProfile) diff --git a/backend/src/apis/app_api/admin/costs/service.py b/backend/src/apis/app_api/admin/costs/service.py index d355306c8..a5aa76e16 100644 --- a/backend/src/apis/app_api/admin/costs/service.py +++ b/backend/src/apis/app_api/admin/costs/service.py @@ -25,6 +25,9 @@ AttachmentProfile, ContextTrajectoryPoint, DataCoverage, + FeedbackByTurnClass, + FeedbackCounts, + FeedbackProfile, FingerprintChanges, SessionDiagnosis, SessionProfile, @@ -77,6 +80,79 @@ def _record_cost(record: Dict[str, Any]) -> Optional[float]: return _as_float(raw) +def _turn_class(record: Dict[str, Any]) -> Optional[str]: + """Turn class of one ``C#`` row from its document-context fields (spec + §6.1): ``full`` (``hasDocuments``), ``retrieved`` (``documentReads.pages + > 0``), ``digestOnly`` (``documentDigests > 0``), else ``none``. + + Precedence is full > retrieved > digestOnly: a call that pulled pages back + still holds the digest, so testing the digest first would leave the + retrieved arm — the one the quality gate is about — permanently empty. + ``None`` when the row carries none of the fields (written before #1137 + or with diagnostics off), so the caller says "not tracked", not "none". + """ + has_documents = record.get("hasDocuments") + digests = record.get("documentDigests") + reads = record.get("documentReads") + if has_documents is None and digests is None and reads is None: + return None + if has_documents: + return "full" + pages = _as_int(reads.get("pages")) if isinstance(reads, dict) else _as_int(reads) + if (pages or 0) > 0: + return "retrieved" + if (_as_int(digests) or 0) > 0: + return "digestOnly" + return "none" + + +def _join_feedback( + records: List[Dict[str, Any]], + feedback_rows: List[Dict[str, Any]], +) -> FeedbackProfile: + """Join ``F#`` rows to ``C#`` rows on ``messageId`` and bucket by turn + class. Pure; the profile's numbers, never any content.""" + by_message: Dict[int, Dict[str, Any]] = {} + for record in records: + message_id = _as_int(record.get("messageId")) + if message_id is not None: + # The last call of a multi-call turn is the one the user thumbed; + # rows share a messageId only across the turn's tool round trips + # and later rows have the fuller context, so last write wins. + by_message[message_id] = record + + any_turn_class = any(_turn_class(r) is not None for r in records) + buckets = FeedbackByTurnClass() if any_turn_class else None + profile = FeedbackProfile() + for row in feedback_rows: + value = _as_int(row.get("value")) + if value not in (1, -1): + continue + if value == 1: + profile.up += 1 + else: + profile.down += 1 + message_id = _as_int(row.get("messageId")) + record = by_message.get(message_id) if message_id is not None else None + if record is None: + profile.unjoined += 1 + continue + if buckets is None: + continue + klass = _turn_class(record) or "none" + bucket: FeedbackCounts = { + "full": buckets.full, + "digestOnly": buckets.digest_only, + "retrieved": buckets.retrieved, + }.get(klass, buckets.none) + if value == 1: + bucket.up += 1 + else: + bucket.down += 1 + profile.by_turn_class = buckets + return profile + + def _context_tokens(record: Dict[str, Any]) -> int: """True context occupancy of one call: uncached input + cached prefix + newly cached tokens (Bedrock reports the three disjointly).""" @@ -966,6 +1042,20 @@ async def _attachment_profile(self, session_id: str) -> AttachmentProfile: by_mime=dict(by_mime), ) + async def _feedback_rows(self, session_id: str) -> List[Dict[str, Any]]: + """The session's ``F#`` rows. Best-effort: a storage fork without the + reader, or a transient error, yields none — the profile then falls + back to the session rollups and reports coverage honestly.""" + reader = getattr(self.storage, "get_session_feedback_rows", None) + if reader is None: + return [] + try: + rows = await reader(session_id) + except Exception as e: # noqa: BLE001 - feedback is one signal of several + logger.debug("Feedback rows unavailable for session: %s", e) + return [] + return list(rows or []) + async def get_session_profile(self, session_id: str) -> Optional[SessionProfile]: """The content-free diagnostic profile of one conversation, or ``None`` when the session has no metadata row. @@ -982,6 +1072,7 @@ async def get_session_profile(self, session_id: str) -> Optional[SessionProfile] records = await self.storage.get_session_cost_records(session_id) attachments = await self._attachment_profile(session_id) + feedback_rows = await self._feedback_rows(session_id) user_period_cost = ( await self._user_period_cost(user_id, self._get_current_period()) if user_id else None @@ -1105,6 +1196,15 @@ async def get_session_profile(self, session_id: str) -> Optional[SessionProfile] facts.tool_call_count = sum(e.calls for e in census.values()) facts.tool_error_count = sum(e.errors for e in census.values()) + # Outcome signal: thumbs joined to the calls they rate. The rows are + # authoritative when present; the session rollups cover thumbs whose + # rows expired (they share the C# TTL, so this is rare). + feedback = _join_feedback(records, feedback_rows) + if not feedback_rows: + feedback.up = _as_int(row.get("thumbsUp")) or 0 + feedback.down = _as_int(row.get("thumbsDown")) or 0 + feedback_tracked = bool(feedback_rows) or row.get("thumbsUp") is not None + findings = run_diagnoses(facts) summary = self._session_summary(row, findings, share) if any_census and summary.tool_call_count is None: @@ -1142,7 +1242,9 @@ async def get_session_profile(self, session_id: str) -> Optional[SessionProfile] compaction_events=( any_compaction_events or row.get("compactionAppliedCount") is not None ), + feedback=feedback_tracked, ), + feedback=feedback, prefix_tokens=prefix_tokens, window_trim_calls=window_trim_calls, window_removed_messages=last_removed, diff --git a/backend/src/apis/app_api/messages/models.py b/backend/src/apis/app_api/messages/models.py index 44c0f4723..99d83ccbc 100644 --- a/backend/src/apis/app_api/messages/models.py +++ b/backend/src/apis/app_api/messages/models.py @@ -117,6 +117,34 @@ class Citation(BaseModel): text: str = Field(..., description="Relevant text excerpt from the document") +#: Reason codes a down-thumb may carry. A closed enum, never free text — the +#: row is content-free by construction so it can sit beside the ``C#`` cost +#: row and be read by the admin profile without reading the conversation. +FEEDBACK_REASONS = ("wrong", "incomplete", "slow", "other") +FeedbackReason = Literal["wrong", "incomplete", "slow", "other"] + + +class MessageFeedback(BaseModel): + """One user's thumb on one assistant message (``F#`` row, see + ``apis.shared.sessions.metadata``). ``value`` is +1 (up) or -1 (down); + ``reason`` is an optional code from ``FEEDBACK_REASONS``.""" + + model_config = ConfigDict(populate_by_name=True) + + value: Literal[1, -1] = Field(..., description="+1 for thumbs up, -1 for thumbs down") + reason: Optional[FeedbackReason] = Field(None, description="Optional reason code (never free text)") + updated_at: str = Field(..., alias="updatedAt", description="ISO timestamp of the latest thumb") + + +class MessageFeedbackRequest(BaseModel): + """Body of ``PUT /sessions/{id}/messages/{messageId}/feedback``.""" + + model_config = ConfigDict(populate_by_name=True) + + value: Literal[1, -1] = Field(..., description="+1 for thumbs up, -1 for thumbs down") + reason: Optional[FeedbackReason] = Field(None, description="Optional reason code (never free text)") + + class MessageMetadata(BaseModel): """Metadata associated with a single message""" @@ -129,8 +157,10 @@ class MessageMetadata(BaseModel): cost: Optional[Union[float, Dict[str, float]]] = Field(None, description="Cost for this message — either a total float (legacy) or a breakdown dict with total, inputCost, outputCost, cacheReadCost, cacheWriteCost") citations: Optional[List[Dict[str, str]]] = Field(None, description="RAG citations for this message (stored as dicts for flexible JSON storage)") display_text: Optional[str] = Field(None, alias="displayText", description="Original user message text before RAG augmentation (for clean UI display)") - # Note: Feedback will be added in future implementation - # feedback: Optional[Feedback] = None + # One user's thumb on this message, merged from the ``F#`` row on read + # (see ``apis.shared.sessions.metadata``). Content-free: a ±1, a timestamp + # and an optional reason code. + feedback: Optional[MessageFeedback] = Field(None, description="User thumbs up/down on this assistant message") class Message(BaseModel): diff --git a/backend/src/apis/app_api/sessions/routes.py b/backend/src/apis/app_api/sessions/routes.py index 47c01fee4..6b40bed14 100644 --- a/backend/src/apis/app_api/sessions/routes.py +++ b/backend/src/apis/app_api/sessions/routes.py @@ -3,7 +3,7 @@ Provides endpoints for managing session metadata. """ -from fastapi import APIRouter, HTTPException, Depends, Query, Response, BackgroundTasks, status +from fastapi import APIRouter, HTTPException, Depends, Path, Query, Response, BackgroundTasks, status from typing import Optional import logging from apis.shared.sessions.models import ( @@ -18,7 +18,14 @@ BulkDeleteSessionsRequest, BulkDeleteSessionsResponse, BulkDeleteSessionResult, - MessagesListResponse + MessagesListResponse, + MessageFeedback, + MessageFeedbackRequest, +) +from apis.shared.sessions.feedback import ( + SessionNotOwned, + delete_message_feedback, + put_message_feedback, ) from apis.shared.sessions.messages import get_messages from apis.shared.sessions.metadata import ( @@ -35,7 +42,7 @@ from apis.app_api.shares.service import get_share_service from apis.app_api.artifacts.service import get_artifact_share_service from apis.shared.auth.dependencies import get_current_user_from_session -from apis.shared.feature_flags import mid_turn_steering_enabled +from apis.shared.feature_flags import message_feedback_enabled, mid_turn_steering_enabled from apis.shared.auth.models import User from apis.shared.system_prompts.service import get_system_prompts_service @@ -662,6 +669,80 @@ async def get_session_messages_endpoint( ) +def _require_message_feedback() -> None: + """404 while ``MESSAGE_FEEDBACK_ENABLED=false`` — the surface does not exist.""" + if not message_feedback_enabled(): + raise HTTPException(status_code=404, detail="Not found") + + +@router.put( + "/{session_id}/messages/{message_id}/feedback", + response_model=MessageFeedback, + response_model_by_alias=True, + response_model_exclude_none=True, +) +async def put_message_feedback_endpoint( + session_id: str, + message_id: int = Path(..., ge=0, description="0-based message index"), + body: MessageFeedbackRequest = ..., + current_user: User = Depends(get_current_user_from_session), +): + """Thumb an assistant message up (+1) or down (-1), optionally with a + reason code. Idempotent per (user, message): a second click replaces the + first. Content-free by construction — the body is a closed enum, so no + text can be stored (``apis.shared.sessions.feedback``). + + ``message_id`` is the message's 0-based index in the conversation — the + trailing number of the SPA's ``msg-{sessionId}-{index}`` id, and the + ``messageId`` the message's cost row carries. + """ + _require_message_feedback() + try: + return await put_message_feedback( + session_id=session_id, + user_id=current_user.user_id, + message_id=message_id, + value=body.value, + reason=body.reason, + ) + except SessionNotOwned: + raise HTTPException(status_code=404, detail=f"Session not found: {session_id}") + except ValueError as e: + raise HTTPException(status_code=422, detail=str(e)) + except RuntimeError as e: + # No metadata table configured (local dev without DynamoDB). + logger.warning("Message feedback unavailable: %s", scrub_log(str(e))) + raise HTTPException(status_code=503, detail="Message feedback is not available") + except Exception: + logger.error("Error storing message feedback", exc_info=True) + raise HTTPException(status_code=500, detail="Failed to store message feedback") + + +@router.delete("/{session_id}/messages/{message_id}/feedback", status_code=204) +async def delete_message_feedback_endpoint( + session_id: str, + message_id: int = Path(..., ge=0, description="0-based message index"), + current_user: User = Depends(get_current_user_from_session), +): + """Withdraw this user's thumb on a message. 204 whether or not one existed.""" + _require_message_feedback() + try: + await delete_message_feedback( + session_id=session_id, + user_id=current_user.user_id, + message_id=message_id, + ) + except SessionNotOwned: + raise HTTPException(status_code=404, detail=f"Session not found: {session_id}") + except RuntimeError as e: + logger.warning("Message feedback unavailable: %s", scrub_log(str(e))) + raise HTTPException(status_code=503, detail="Message feedback is not available") + except Exception: + logger.error("Error deleting message feedback", exc_info=True) + raise HTTPException(status_code=500, detail="Failed to delete message feedback") + return Response(status_code=204) + + @router.post("/{session_id}/interrupt", status_code=204) async def signal_turn_interrupted_endpoint( session_id: str, diff --git a/backend/src/apis/shared/feature_flags.py b/backend/src/apis/shared/feature_flags.py index 31555cb95..6ce00b90c 100644 --- a/backend/src/apis/shared/feature_flags.py +++ b/backend/src/apis/shared/feature_flags.py @@ -352,3 +352,23 @@ def ask_user_question_enabled() -> bool: every time it flipped. """ return os.environ.get("ASK_USER_QUESTION_ENABLED", "").strip().lower() != "false" + + +def message_feedback_enabled() -> bool: + """Whether users can thumb an assistant message up or down. + + Covers the ``PUT`` / ``DELETE /sessions/{id}/messages/{messageId}/feedback`` + routes, the ``feedback`` field merged into ``GET /sessions/{id}/messages``, + and the ``thumbsUp`` / ``thumbsDown`` session rollups. **Default ON with a + kill switch** (house style, mirroring ``cost_diagnostics_enabled``): unset + or empty resolves to enabled; only the literal ``"false"`` (case- + insensitive) disables. While off the write routes 404 and the read merge + is skipped; rows already written stay in the table. + + The signal is content-free by construction (a ±1, a timestamp and an + optional reason *code* from a fixed enum — never free text), which is + what lets it join the ``C#`` cost row's turn class on the admin session + profile without the profile ever reading the conversation. See + ``docs/specs/document-context-offload.md`` §5 row 7 / §6.1. + """ + return os.environ.get("MESSAGE_FEEDBACK_ENABLED", "").strip().lower() != "false" diff --git a/backend/src/apis/shared/observability/content_policy.py b/backend/src/apis/shared/observability/content_policy.py index 5867f1218..5b7b85f6f 100644 --- a/backend/src/apis/shared/observability/content_policy.py +++ b/backend/src/apis/shared/observability/content_policy.py @@ -132,6 +132,10 @@ def is_content_bearing(path: str) -> bool: "compactionAppliedCount", "compactionForcedCount", "compactionFloorUnreachableCount", + # Message-feedback rollups (live F# rows written while diagnostics were + # on; see `apis.shared.sessions.feedback`) + "thumbsUp", + "thumbsDown", ) #: C# rows for the cost anatomy and the session profile's trajectory. @@ -158,6 +162,13 @@ def is_content_bearing(path: str) -> bool: "prefixTokens", "windowRemovedMessages", "compactionEvents", + # Turn class for the feedback join (document-context offload §6.1). The + # rows carry these from #1137 (offload PR-1), which also widens this + # projection to the full document-context field set; a row written before + # that lacks them and the profile reports the class as not tracked. + "hasDocuments", + "documentDigests", + "documentReads", ) #: FILE# rows for the session profile's attachment summary. @@ -171,10 +182,22 @@ def is_content_bearing(path: str) -> bool: "createdAt", ) +#: F# rows for the session profile's feedback join. A thumb is a ±1, an +#: optional reason *code* and a timestamp — never text, by the request +#: model's closed enum (`apis.shared.sessions.models.FEEDBACK_REASONS`). +FEEDBACK_ROW_PROJECTION: Tuple[str, ...] = ( + "sessionId", + "messageId", + "value", + "reason", + "updatedAt", +) + ALL_PROJECTIONS: Dict[str, Tuple[str, ...]] = { "SESSION_ROW_PROJECTION": SESSION_ROW_PROJECTION, "CALL_ROW_PROJECTION": CALL_ROW_PROJECTION, "FILE_ROW_PROJECTION": FILE_ROW_PROJECTION, + "FEEDBACK_ROW_PROJECTION": FEEDBACK_ROW_PROJECTION, } diff --git a/backend/src/apis/shared/sessions/feedback.py b/backend/src/apis/shared/sessions/feedback.py new file mode 100644 index 000000000..a1d935318 --- /dev/null +++ b/backend/src/apis/shared/sessions/feedback.py @@ -0,0 +1,225 @@ +"""Message feedback (thumbs up / down) storage — the ``F#`` row family. + +The outcome signal the cost work has been missing (document-context-offload +spec §5 row 7, §6.1 "the outcome signal"; compaction thresholds spec §7.2). +One row per ``(user, session, message)``, content-free by construction: a +``value`` of +1 / -1, a timestamp and an optional reason *code* from +:data:`FEEDBACK_REASONS`. No free text can be stored here — the request +model's ``reason`` is a closed ``Literal`` and this module never accepts a +string outside the tuple. + +Schema (``sessions-metadata`` table, beside the ``C#`` / ``D#`` rows — see the +row-family summary in ``apis.shared.sessions.metadata``):: + + PK: USER#{user_id} + SK: F#{session_id}#{message_id} + GSI_PK: SESSION#{session_id} (SessionLookupIndex) + GSI_SK: F#{message_id} + sessionId, messageId, userId, value, reason?, updatedAt, ttl + +``messageId`` is the same 0-based index the ``C#`` cost row carries for the +assistant message, so an admin read joins feedback to the call's turn class +(``hasDocuments`` / ``documentDigests`` / ``documentReads``, #1137) on +``(sessionId, messageId)`` with no second lookup. The SK is deterministic, so +a second thumb on the same message *replaces* the first (``put_item``), and a +``DELETE`` removes it — one thumb per (user, message) by key design. + +Session-row rollups: ``thumbsUp`` / ``thumbsDown`` are ``ADD``ed on the ``S#`` +row like ``toolCallCount``, only while ``COST_DIAGNOSTICS_ENABLED`` (an absent +attribute reads "not tracked", never 0). A replace adjusts both counters so +the rollup always equals the count of live ``F#`` rows written while the +diagnostics were on. + +Everything is gated by :func:`apis.shared.feature_flags.message_feedback_enabled` +at the route; this module is deliberately flag-free so a backfill or a test +can drive it directly. +""" + +from __future__ import annotations + +import logging +import os +from datetime import datetime, timedelta, timezone +from typing import Any, Dict, Optional + +from .models import FEEDBACK_REASONS, MessageFeedback +from .preview import is_preview_session + +logger = logging.getLogger(__name__) + +#: Retention, matching the ``C#`` cost rows the feedback joins to. +FEEDBACK_TTL_DAYS = 365 + + +class SessionNotOwned(Exception): + """The session has no metadata row for this user (missing or another user's).""" + + +def _table_name() -> str: + name = os.environ.get("DYNAMODB_SESSIONS_METADATA_TABLE_NAME") + if not name: + raise RuntimeError("DYNAMODB_SESSIONS_METADATA_TABLE_NAME environment variable is required") + return name + + +def _table(): + import boto3 + + return boto3.resource("dynamodb").Table(_table_name()) + + +def feedback_sk(session_id: str, message_id: int) -> str: + return f"F#{session_id}#{message_id}" + + +def _keys(user_id: str, session_id: str, message_id: int) -> Dict[str, str]: + return {"PK": f"USER#{user_id}", "SK": feedback_sk(session_id, message_id)} + + +def _to_model(item: Dict[str, Any]) -> MessageFeedback: + return MessageFeedback( + value=int(item.get("value", 0)), + reason=item.get("reason") if item.get("reason") in FEEDBACK_REASONS else None, + updated_at=str(item.get("updatedAt", "")), + ) + + +async def _owned_session_sk(session_id: str, user_id: str, table) -> str: + """The session row's SK, or raise :class:`SessionNotOwned`.""" + from .metadata import _get_session_by_gsi + + existing = await _get_session_by_gsi(session_id, user_id, table) + if not existing or not existing.get("SK"): + raise SessionNotOwned(session_id) + return existing["SK"] + + +def _bump_rollups(table, user_id: str, session_sk: str, *, up: int, down: int) -> None: + """``ADD thumbsUp :up, thumbsDown :down`` on the session row, only while + the content-free diagnostics are on. Best-effort: a failed bump never + fails the user's click — the rows stay authoritative and the profile + prefers them when present.""" + from apis.shared.feature_flags import cost_diagnostics_enabled + + if not cost_diagnostics_enabled(): + return + try: + table.update_item( + Key={"PK": f"USER#{user_id}", "SK": session_sk}, + UpdateExpression="ADD thumbsUp :up, thumbsDown :down", + ExpressionAttributeValues={":up": int(up), ":down": int(down)}, + ) + except Exception as e: # noqa: BLE001 - rollup drift is tolerable, a lost click is not + logger.debug("feedback rollup bump failed for %s: %s", session_sk, e) + + +async def put_message_feedback( + session_id: str, + user_id: str, + message_id: int, + value: int, + reason: Optional[str] = None, +) -> MessageFeedback: + """Write (or replace) this user's thumb on one message. + + Raises :class:`SessionNotOwned` when the session is not this user's, and + ``ValueError`` on a value outside ``{1, -1}`` or a reason outside + :data:`FEEDBACK_REASONS` — the route's request model already rejects + both, this is the storage layer refusing to become a text field. + """ + if value not in (1, -1): + raise ValueError("feedback value must be 1 or -1") + if reason is not None and reason not in FEEDBACK_REASONS: + raise ValueError("feedback reason must be one of the fixed codes") + if is_preview_session(session_id): + # Preview sessions persist nothing; echo the thumb so the UI is consistent. + return MessageFeedback(value=value, reason=reason, updated_at=_now()) + + table = _table() + session_sk = await _owned_session_sk(session_id, user_id, table) + + now = _now() + ttl = int((datetime.now(timezone.utc) + timedelta(days=FEEDBACK_TTL_DAYS)).timestamp()) + item: Dict[str, Any] = { + **_keys(user_id, session_id, message_id), + "GSI_PK": f"SESSION#{session_id}", + "GSI_SK": f"F#{message_id}", + "sessionId": session_id, + "messageId": int(message_id), + "userId": user_id, + "value": int(value), + "updatedAt": now, + "ttl": ttl, + } + if reason: + item["reason"] = reason + + # ReturnValues=ALL_OLD tells us what a replace is replacing, so the + # session rollups move by the delta rather than double-counting. + response = table.put_item(Item=item, ReturnValues="ALL_OLD") + previous = response.get("Attributes") or {} + previous_value = int(previous.get("value", 0)) if previous else 0 + + up = (1 if value == 1 else 0) - (1 if previous_value == 1 else 0) + down = (1 if value == -1 else 0) - (1 if previous_value == -1 else 0) + if up or down: + _bump_rollups(table, user_id, session_sk, up=up, down=down) + + logger.info("👍 feedback stored for message %s in session %s", message_id, session_id) + return _to_model(item) + + +async def delete_message_feedback(session_id: str, user_id: str, message_id: int) -> bool: + """Remove this user's thumb on one message. Returns whether a row existed.""" + if is_preview_session(session_id): + return False + table = _table() + session_sk = await _owned_session_sk(session_id, user_id, table) + + response = table.delete_item(Key=_keys(user_id, session_id, message_id), ReturnValues="ALL_OLD") + previous = response.get("Attributes") or {} + if not previous: + return False + previous_value = int(previous.get("value", 0)) + _bump_rollups( + table, user_id, session_sk, + up=-1 if previous_value == 1 else 0, + down=-1 if previous_value == -1 else 0, + ) + return True + + +def query_session_feedback(table, session_id: str, user_id: Optional[str] = None) -> Dict[str, MessageFeedback]: + """All ``F#`` rows for a session via ``SessionLookupIndex``, keyed by + message id (as ``str``, matching the metadata index). With ``user_id`` + set, rows belonging to another user are dropped (the SPA read); the + admin reader passes ``None`` and gets every user's thumb.""" + from boto3.dynamodb.conditions import Key + + out: Dict[str, MessageFeedback] = {} + last_key = None + while True: + kwargs: Dict[str, Any] = { + "IndexName": "SessionLookupIndex", + "KeyConditionExpression": Key("GSI_PK").eq(f"SESSION#{session_id}") & Key("GSI_SK").begins_with("F#"), + } + if last_key: + kwargs["ExclusiveStartKey"] = last_key + response = table.query(**kwargs) + for item in response.get("Items", []): + if user_id is not None and item.get("userId") != user_id: + continue + raw_id = item.get("messageId") + try: + message_id = str(int(raw_id)) + except (TypeError, ValueError): + continue + out[message_id] = _to_model(item) + last_key = response.get("LastEvaluatedKey") + if not last_key: + break + return out + + +def _now() -> str: + return datetime.now(timezone.utc).isoformat() diff --git a/backend/src/apis/shared/sessions/metadata.py b/backend/src/apis/shared/sessions/metadata.py index f58a0486d..c96705d87 100644 --- a/backend/src/apis/shared/sessions/metadata.py +++ b/backend/src/apis/shared/sessions/metadata.py @@ -5,6 +5,22 @@ Architecture: - Cloud: Stores metadata in DynamoDB table specified by DYNAMODB_SESSIONS_METADATA_TABLE_NAME + +Row families on the ``sessions-metadata`` table (PK = ``USER#{user_id}``; all +carry ``GSI_PK = SESSION#{session_id}`` so ``SessionLookupIndex`` lists one +session's rows by prefix): + + S#{session_id} session row (rollups, preferences, compaction state) + C#{timestamp}#{uuid} one model call's cost/usage record; ``messageId`` = the + assistant message's 0-based index (``_store_message_metadata_cloud``) + D#{session_id}#{message_id} the user's original prompt text for display (``store_user_display_text``) + F#{session_id}#{message_id} the user's thumb on an assistant message — value ±1, optional + reason code, timestamp; content-free (``apis.shared.sessions.feedback``). + Same ``messageId`` as the ``C#`` row, so feedback joins the call's + turn class on ``(sessionId, messageId)`` in one lookup. + +``GSI_SK`` is ``META`` / ``C#{timestamp}`` / ``D#{message_id}`` / ``F#{message_id}`` +respectively. Only ``C#`` / ``D#`` / ``F#`` rows carry a ``ttl``. """ import logging @@ -2120,6 +2136,20 @@ async def _get_all_message_metadata_cloud(session_id: str, user_id: str, table_n metadata_index[message_id] = {"displayText": display_text} logger.debug(f"🔗 Merged displayText for user message {message_id}") + # Merge this user's thumbs (F# rows) so a reload restores the SPA's + # pressed state. Skipped while the feature is off — the rows stay. + from apis.shared.feature_flags import message_feedback_enabled + + if message_feedback_enabled(): + from .feedback import query_session_feedback + + try: + for message_id, feedback in query_session_feedback(table, session_id, user_id).items(): + entry = metadata_index.setdefault(message_id, {}) + entry["feedback"] = feedback.model_dump(by_alias=True, exclude_none=True) + except Exception as e: # noqa: BLE001 - feedback is a UI enhancement, never block history + logger.warning(f"Failed to merge message feedback: {e}") + logger.info(f"📋 Metadata keys: {sorted(metadata_index.keys())}") return metadata_index diff --git a/backend/src/apis/shared/sessions/models.py b/backend/src/apis/shared/sessions/models.py index 82dc611b0..f19d5b3f8 100644 --- a/backend/src/apis/shared/sessions/models.py +++ b/backend/src/apis/shared/sessions/models.py @@ -642,6 +642,34 @@ class Citation(BaseModel): text: str = Field(..., description="Relevant text excerpt from the document") +#: Reason codes a down-thumb may carry. A closed enum, never free text — the +#: row is content-free by construction so it can sit beside the ``C#`` cost +#: row and be read by the admin profile without reading the conversation. +FEEDBACK_REASONS = ("wrong", "incomplete", "slow", "other") +FeedbackReason = Literal["wrong", "incomplete", "slow", "other"] + + +class MessageFeedback(BaseModel): + """One user's thumb on one assistant message (``F#`` row, see + ``apis.shared.sessions.metadata``). ``value`` is +1 (up) or -1 (down); + ``reason`` is an optional code from ``FEEDBACK_REASONS``.""" + + model_config = ConfigDict(populate_by_name=True) + + value: Literal[1, -1] = Field(..., description="+1 for thumbs up, -1 for thumbs down") + reason: Optional[FeedbackReason] = Field(None, description="Optional reason code (never free text)") + updated_at: str = Field(..., alias="updatedAt", description="ISO timestamp of the latest thumb") + + +class MessageFeedbackRequest(BaseModel): + """Body of ``PUT /sessions/{id}/messages/{messageId}/feedback``.""" + + model_config = ConfigDict(populate_by_name=True) + + value: Literal[1, -1] = Field(..., description="+1 for thumbs up, -1 for thumbs down") + reason: Optional[FeedbackReason] = Field(None, description="Optional reason code (never free text)") + + class MessageMetadata(BaseModel): """Metadata associated with a single message""" @@ -654,8 +682,10 @@ class MessageMetadata(BaseModel): cost: Optional[Union[float, Dict[str, float]]] = Field(None, description="Cost for this message — either a total float (legacy) or a breakdown dict with total, inputCost, outputCost, cacheReadCost, cacheWriteCost") citations: Optional[List[Dict[str, str]]] = Field(None, description="RAG citations for this message (stored as dicts for flexible JSON storage)") display_text: Optional[str] = Field(None, alias="displayText", description="Original user message text before RAG augmentation (for clean UI display)") - # Note: Feedback will be added in future implementation - # feedback: Optional[Feedback] = None + # One user's thumb on this message, merged from the ``F#`` row on read + # (see ``apis.shared.sessions.metadata``). Content-free: a ±1, a timestamp + # and an optional reason code. + feedback: Optional[MessageFeedback] = Field(None, description="User thumbs up/down on this assistant message") class Message(BaseModel): diff --git a/backend/src/apis/shared/storage/dynamodb_storage.py b/backend/src/apis/shared/storage/dynamodb_storage.py index b010640ad..6b03ba1ff 100644 --- a/backend/src/apis/shared/storage/dynamodb_storage.py +++ b/backend/src/apis/shared/storage/dynamodb_storage.py @@ -337,6 +337,49 @@ async def get_session_cost_records( except ClientError as e: raise Exception(f"Failed to get session cost records: {e}") + async def get_session_feedback_rows( + self, + session_id: str, + ) -> List[Dict[str, Any]]: + """All ``F#`` message-feedback rows for a session, any user — admin + scope, content-free by projection (``FEEDBACK_ROW_PROJECTION``). + Each row: ``messageId`` (the assistant message's index, the same key + the ``C#`` row carries), ``value`` ±1, optional ``reason`` code, + ``updatedAt``. Empty when the session has no thumbs. + """ + from boto3.dynamodb.conditions import Key + from apis.shared.observability.content_policy import ( + FEEDBACK_ROW_PROJECTION, + build_projection, + strip_content, + ) + + projection, names = build_projection(FEEDBACK_ROW_PROJECTION) + try: + items: List[Dict[str, Any]] = [] + last_evaluated_key = None + while True: + query_kwargs = { + "IndexName": "SessionLookupIndex", + "KeyConditionExpression": ( + Key("GSI_PK").eq(f"SESSION#{session_id}") + & Key("GSI_SK").begins_with("F#") + ), + "ProjectionExpression": projection, + "ExpressionAttributeNames": names, + } + if last_evaluated_key: + query_kwargs["ExclusiveStartKey"] = last_evaluated_key + response = self.sessions_metadata_table.query(**query_kwargs) + items.extend(response.get("Items", [])) + last_evaluated_key = response.get("LastEvaluatedKey") + if not last_evaluated_key: + break + except ClientError as e: + raise Exception(f"Failed to get session feedback rows: {e}") + + return [strip_content(self._convert_decimal_to_float(item)) for item in items] + async def get_session_diagnostic_row( self, session_id: str, diff --git a/backend/tests/apis/app_api/admin/costs/test_session_profile_service.py b/backend/tests/apis/app_api/admin/costs/test_session_profile_service.py index ea7f6ebf7..295c6b735 100644 --- a/backend/tests/apis/app_api/admin/costs/test_session_profile_service.py +++ b/backend/tests/apis/app_api/admin/costs/test_session_profile_service.py @@ -254,3 +254,88 @@ async def test_session_row_counters_alone_mark_compaction_events_as_tracked(): p = await _service(_row(compactionAppliedCount=0), [_call(0)]).get_session_profile("s1") assert p.data_coverage.compaction_events assert p.session.compaction_applied_count == 0 + + +# ── feedback join (document-context offload PR-7) ─────────────────────────── + + +def _feedback(message_id, value, reason=None): + row = {"sessionId": "s1", "messageId": message_id, "value": value, "updatedAt": "2026-09-16T00:00:00Z"} + if reason: + row["reason"] = reason + return row + + +def _service_with_feedback(row, records, feedback): + service = _service(row, records) + service.storage.get_session_feedback_rows = AsyncMock(return_value=feedback) + return service + + +@pytest.mark.asyncio +async def test_feedback_joins_the_turn_class_when_the_rows_carry_it(): + records = [ + _call(0), # attach turn: full document inline + _call(1), # follow-up: digest only + _call(2), # follow-up that read pages back + _call(3), # no documents at all + ] + records[0]["hasDocuments"] = True + records[1]["hasDocuments"] = False + records[1]["documentDigests"] = 1 + records[2]["hasDocuments"] = False + records[2]["documentDigests"] = 1 + records[2]["documentReads"] = {"calls": 1, "pages": 4, "bytes": 1000} + records[3]["hasDocuments"] = False + records[3]["documentDigests"] = 0 + feedback = [ + _feedback(0, 1), + _feedback(1, -1, "wrong"), + _feedback(2, 1), + _feedback(3, -1, "slow"), + _feedback(9, -1), # no cost row for this message + ] + p = await _service_with_feedback(_row(), records, feedback).get_session_profile("s1") + + assert (p.feedback.up, p.feedback.down, p.feedback.unjoined) == (2, 3, 1) + by = p.feedback.by_turn_class + assert by is not None + assert (by.full.up, by.full.down) == (1, 0) + assert (by.digest_only.up, by.digest_only.down) == (0, 1) + assert (by.retrieved.up, by.retrieved.down) == (1, 0) + assert (by.none.up, by.none.down) == (0, 1) + assert p.data_coverage.feedback is True + # Wire shape the SPA reads. + wire = p.model_dump(by_alias=True)["feedback"] + assert wire["byTurnClass"]["digestOnly"] == {"up": 0, "down": 1} + + +@pytest.mark.asyncio +async def test_feedback_counts_without_turn_class_when_rows_predate_1137(): + records = [_call(0), _call(1)] # no hasDocuments / documentDigests / documentReads + feedback = [_feedback(0, 1), _feedback(1, -1, "incomplete")] + p = await _service_with_feedback(_row(), records, feedback).get_session_profile("s1") + assert (p.feedback.up, p.feedback.down) == (1, 1) + assert p.feedback.by_turn_class is None, "turn class is 'not tracked', not 'none'" + assert p.feedback.unjoined == 0 + assert p.data_coverage.feedback is True + + +@pytest.mark.asyncio +async def test_no_feedback_rows_falls_back_to_rollups_and_coverage_is_honest(): + p = await _service_with_feedback(_row(), [_call(0)], []).get_session_profile("s1") + assert (p.feedback.up, p.feedback.down) == (0, 0) + assert p.data_coverage.feedback is False + + p = await _service_with_feedback(_row(thumbsUp=2, thumbsDown=1), [_call(0)], []).get_session_profile("s1") + assert (p.feedback.up, p.feedback.down) == (2, 1) + assert p.data_coverage.feedback is True + assert p.feedback.by_turn_class is None + + +@pytest.mark.asyncio +async def test_feedback_reader_failure_never_breaks_the_profile(): + service = _service(_row(), [_call(0)]) + service.storage.get_session_feedback_rows = AsyncMock(side_effect=RuntimeError("boom")) + p = await service.get_session_profile("s1") + assert p is not None and p.feedback.up == 0 and p.data_coverage.feedback is False diff --git a/backend/tests/apis/app_api/test_message_feedback_routes.py b/backend/tests/apis/app_api/test_message_feedback_routes.py new file mode 100644 index 000000000..574bb6a38 --- /dev/null +++ b/backend/tests/apis/app_api/test_message_feedback_routes.py @@ -0,0 +1,92 @@ +"""Routes for the thumbs signal: + +- PUT /sessions/{id}/messages/{messageId}/feedback → 200 with the stored thumb +- DELETE /sessions/{id}/messages/{messageId}/feedback → 204 +- 404 for another user's session, 404 while MESSAGE_FEEDBACK_ENABLED=false, + 422 for anything but ±1 / a fixed reason code, 503 with no table. + +Auth is the cookie-aware `get_current_user_from_session` (SPA-facing route); +storage is swapped through the module's own function names. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from apis.app_api.sessions import routes as session_routes +from apis.shared.auth.dependencies import get_current_user_from_session +from apis.shared.auth.models import User +from apis.shared.sessions.feedback import SessionNotOwned +from apis.shared.sessions.models import MessageFeedback + + +def _user() -> User: + return User(user_id="user-1", email="u@example.com", name="U", roles=["default"], raw_token="tok") + + +def _client(monkeypatch, put=None, delete=None) -> TestClient: + monkeypatch.delenv("MESSAGE_FEEDBACK_ENABLED", raising=False) + monkeypatch.setattr(session_routes, "put_message_feedback", put or AsyncMock( + return_value=MessageFeedback(value=1, updated_at="2026-09-16T00:00:00Z") + )) + monkeypatch.setattr(session_routes, "delete_message_feedback", delete or AsyncMock(return_value=True)) + app = FastAPI() + app.include_router(session_routes.router) + app.dependency_overrides[get_current_user_from_session] = _user + return TestClient(app) + + +def test_put_stores_and_echoes_the_thumb(monkeypatch): + put = AsyncMock(return_value=MessageFeedback(value=-1, reason="incomplete", updated_at="2026-09-16T00:00:00Z")) + client = _client(monkeypatch, put=put) + + resp = client.put("/sessions/s1/messages/3/feedback", json={"value": -1, "reason": "incomplete"}) + + assert resp.status_code == 200 + assert resp.json() == {"value": -1, "reason": "incomplete", "updatedAt": "2026-09-16T00:00:00Z"} + put.assert_awaited_once_with(session_id="s1", user_id="user-1", message_id=3, value=-1, reason="incomplete") + + +def test_put_rejects_free_text_and_out_of_range_values(monkeypatch): + client = _client(monkeypatch) + assert client.put("/sessions/s1/messages/3/feedback", json={"value": 1, "reason": "it lied"}).status_code == 422 + assert client.put("/sessions/s1/messages/3/feedback", json={"value": 0}).status_code == 422 + assert client.put("/sessions/s1/messages/3/feedback", json={"value": 5}).status_code == 422 + assert client.put("/sessions/s1/messages/three/feedback", json={"value": 1}).status_code == 422 + assert client.put("/sessions/s1/messages/-1/feedback", json={"value": 1}).status_code == 422 + + +def test_delete_returns_204(monkeypatch): + delete = AsyncMock(return_value=True) + client = _client(monkeypatch, delete=delete) + resp = client.delete("/sessions/s1/messages/3/feedback") + assert resp.status_code == 204 + delete.assert_awaited_once_with(session_id="s1", user_id="user-1", message_id=3) + + +def test_another_users_session_is_404(monkeypatch): + client = _client( + monkeypatch, + put=AsyncMock(side_effect=SessionNotOwned("s1")), + delete=AsyncMock(side_effect=SessionNotOwned("s1")), + ) + assert client.put("/sessions/s1/messages/3/feedback", json={"value": 1}).status_code == 404 + assert client.delete("/sessions/s1/messages/3/feedback").status_code == 404 + + +def test_kill_switch_hides_the_surface(monkeypatch): + put = AsyncMock() + client = _client(monkeypatch, put=put) + monkeypatch.setenv("MESSAGE_FEEDBACK_ENABLED", "false") + assert client.put("/sessions/s1/messages/3/feedback", json={"value": 1}).status_code == 404 + assert client.delete("/sessions/s1/messages/3/feedback").status_code == 404 + put.assert_not_awaited() + + +def test_missing_table_is_503_not_500(monkeypatch): + client = _client(monkeypatch, put=AsyncMock(side_effect=RuntimeError("DYNAMODB_SESSIONS_METADATA_TABLE_NAME"))) + assert client.put("/sessions/s1/messages/3/feedback", json={"value": 1}).status_code == 503 diff --git a/backend/tests/costs/test_content_free_projections.py b/backend/tests/costs/test_content_free_projections.py index 454a45835..8eb1ce682 100644 --- a/backend/tests/costs/test_content_free_projections.py +++ b/backend/tests/costs/test_content_free_projections.py @@ -177,3 +177,27 @@ async def test_deleted_sessions_are_listed_on_request_and_stay_content_free(stor assert by_id["gone"]["status"] == "deleted" assert by_id["gone"]["totalCost"] == 9 assert "title" not in by_id["gone"] + + +@pytest.mark.asyncio +async def test_feedback_rows_come_back_content_free_and_keyed_to_the_call(storage): + _seed(storage) + storage.sessions_metadata_table.put_item(Item={ + "PK": f"USER#{USER_ID}", "SK": f"F#{SESSION_ID}#3", + "GSI_PK": f"SESSION#{SESSION_ID}", "GSI_SK": "F#3", + "sessionId": SESSION_ID, "messageId": Decimal(3), "userId": USER_ID, + "value": Decimal(-1), "reason": "wrong", "updatedAt": "2026-09-16T00:00:00Z", + "ttl": Decimal(1_800_000_000), + # A stray content-bearing attribute must never leave the reader even + # if something wrote one (the writer cannot, but the reader is the guard). + "displayText": "SECRET", + }) + rows = await storage.get_session_feedback_rows(SESSION_ID) + assert len(rows) == 1 + row = rows[0] + assert content_bearing_paths(row) == [] + assert row == {"sessionId": SESSION_ID, "messageId": 3, "value": -1, "reason": "wrong", "updatedAt": "2026-09-16T00:00:00Z"} + # Joins the C# row on messageId. + records = await storage.get_session_cost_records(SESSION_ID) + assert records[0]["messageId"] == row["messageId"] + assert await storage.get_session_feedback_rows("no-such-session") == [] diff --git a/backend/tests/shared/test_message_feedback.py b/backend/tests/shared/test_message_feedback.py new file mode 100644 index 000000000..d1926204e --- /dev/null +++ b/backend/tests/shared/test_message_feedback.py @@ -0,0 +1,188 @@ +"""The ``F#`` message-feedback row family (``apis.shared.sessions.feedback``) +against a real (moto) sessions-metadata table. + +Pins the key shape that makes the admin join a one-key lookup, replace-not- +append semantics, the session rollups' deltas, ownership, the content-free +guard, and the read-side merge into the messages-list metadata index. +""" + +from __future__ import annotations + +import boto3 +import pytest +from boto3.dynamodb.conditions import Key + +from apis.shared.sessions import feedback as fb +from apis.shared.sessions import metadata as md + +SESSION = "sess-fb-1" +OWNER = "user-owner" +OTHER = "user-other" + + +def _table(): + return boto3.resource("dynamodb", region_name="us-east-1").Table("test-sessions-metadata") + + +def _seed_session(user_id=OWNER, session_id=SESSION): + _table().put_item(Item={ + "PK": f"USER#{user_id}", + "SK": f"S#{session_id}", + "GSI_PK": f"SESSION#{session_id}", + "GSI_SK": "META", + "sessionId": session_id, + "userId": user_id, + "status": "active", + "messageCount": 4, + }) + + +def _session_row(user_id=OWNER, session_id=SESSION): + return _table().get_item(Key={"PK": f"USER#{user_id}", "SK": f"S#{session_id}"}).get("Item") or {} + + +def _feedback_items(session_id=SESSION): + resp = _table().query( + IndexName="SessionLookupIndex", + KeyConditionExpression=Key("GSI_PK").eq(f"SESSION#{session_id}") & Key("GSI_SK").begins_with("F#"), + ) + return resp.get("Items", []) + + +@pytest.fixture() +def table(sessions_metadata_table, monkeypatch): + monkeypatch.delenv("COST_DIAGNOSTICS_ENABLED", raising=False) + _seed_session() + return _table() + + +@pytest.mark.asyncio +async def test_put_writes_one_row_keyed_beside_the_cost_row(table): + result = await fb.put_message_feedback(SESSION, OWNER, 3, 1) + assert result.value == 1 and result.reason is None and result.updated_at + + items = _feedback_items() + assert len(items) == 1 + row = items[0] + assert row["PK"] == f"USER#{OWNER}" + assert row["SK"] == f"F#{SESSION}#3" + assert row["GSI_PK"] == f"SESSION#{SESSION}" and row["GSI_SK"] == "F#3" + assert int(row["messageId"]) == 3 and row["sessionId"] == SESSION + assert int(row["value"]) == 1 + assert "reason" not in row + assert "ttl" in row + # Content-free: nothing on the row but ids, a number, a timestamp and keys. + assert set(row) <= {"PK", "SK", "GSI_PK", "GSI_SK", "sessionId", "messageId", "userId", "value", "updatedAt", "ttl"} + + +@pytest.mark.asyncio +async def test_second_thumb_replaces_the_first_and_rollups_follow(table): + await fb.put_message_feedback(SESSION, OWNER, 3, 1) + assert _session_row()["thumbsUp"] == 1 and _session_row()["thumbsDown"] == 0 + + result = await fb.put_message_feedback(SESSION, OWNER, 3, -1, reason="incomplete") + assert result.value == -1 and result.reason == "incomplete" + + items = _feedback_items() + assert len(items) == 1, "a second click replaces, never appends" + assert int(items[0]["value"]) == -1 and items[0]["reason"] == "incomplete" + row = _session_row() + assert row["thumbsUp"] == 0 and row["thumbsDown"] == 1 + + # Same value again: no rollup movement, reason updates in place. + await fb.put_message_feedback(SESSION, OWNER, 3, -1, reason="wrong") + row = _session_row() + assert row["thumbsUp"] == 0 and row["thumbsDown"] == 1 + assert _feedback_items()[0]["reason"] == "wrong" + + +@pytest.mark.asyncio +async def test_delete_removes_the_row_and_decrements(table): + await fb.put_message_feedback(SESSION, OWNER, 1, -1, reason="slow") + await fb.put_message_feedback(SESSION, OWNER, 3, 1) + assert await fb.delete_message_feedback(SESSION, OWNER, 1) is True + assert [int(i["messageId"]) for i in _feedback_items()] == [3] + row = _session_row() + assert row["thumbsUp"] == 1 and row["thumbsDown"] == 0 + # Deleting what isn't there is a no-op, not an error. + assert await fb.delete_message_feedback(SESSION, OWNER, 1) is False + assert _session_row()["thumbsDown"] == 0 + + +@pytest.mark.asyncio +async def test_rollups_are_not_written_while_diagnostics_are_off(table, monkeypatch): + monkeypatch.setenv("COST_DIAGNOSTICS_ENABLED", "false") + await fb.put_message_feedback(SESSION, OWNER, 3, 1) + row = _session_row() + assert "thumbsUp" not in row and "thumbsDown" not in row, "absent reads 'not tracked', never 0" + assert len(_feedback_items()) == 1, "the row itself is still written" + + +@pytest.mark.asyncio +async def test_another_users_session_is_not_found(table): + with pytest.raises(fb.SessionNotOwned): + await fb.put_message_feedback(SESSION, OTHER, 3, 1) + with pytest.raises(fb.SessionNotOwned): + await fb.delete_message_feedback(SESSION, OTHER, 3) + assert _feedback_items() == [] + + +@pytest.mark.asyncio +async def test_storage_refuses_free_text_and_bad_values(table): + with pytest.raises(ValueError): + await fb.put_message_feedback(SESSION, OWNER, 3, 1, reason="it was rude to me") + with pytest.raises(ValueError): + await fb.put_message_feedback(SESSION, OWNER, 3, 2) + assert _feedback_items() == [] + + +@pytest.mark.asyncio +async def test_preview_sessions_echo_without_persisting(table): + result = await fb.put_message_feedback("preview-abc", OWNER, 0, 1) + assert result.value == 1 + assert _feedback_items("preview-abc") == [] + + +@pytest.mark.asyncio +async def test_query_filters_by_user_unless_admin(table): + _seed_session(OTHER, "sess-shared") + _seed_session(OWNER, "sess-shared") + await fb.put_message_feedback("sess-shared", OWNER, 1, 1) + await fb.put_message_feedback("sess-shared", OTHER, 1, -1) + + mine = fb.query_session_feedback(table, "sess-shared", OWNER) + assert {k: v.value for k, v in mine.items()} == {"1": 1} + everyone = fb.query_session_feedback(table, "sess-shared", None) + assert len(everyone) == 1 # keyed by message id; the admin reader returns rows, this map is per message + + +@pytest.mark.asyncio +async def test_messages_list_metadata_index_carries_feedback(table, monkeypatch): + """The read path the SPA uses on reload: F# rows merge into the metadata + index by message id, beside (or in place of) the cost record.""" + table.put_item(Item={ + "PK": f"USER#{OWNER}", "SK": "C#2026-09-16T00:00:00Z#u1", + "GSI_PK": f"SESSION#{SESSION}", "GSI_SK": "C#2026-09-16T00:00:00Z", + "sessionId": SESSION, "messageId": 3, "userId": OWNER, + "timestamp": "2026-09-16T00:00:00Z", "cost": {"total": 1}, + }) + await fb.put_message_feedback(SESSION, OWNER, 3, -1, reason="wrong") + await fb.put_message_feedback(SESSION, OWNER, 5, 1) + + index = await md.get_all_message_metadata(SESSION, OWNER) + assert index["3"]["cost"] == {"total": 1} + assert index["3"]["feedback"] == {"value": -1, "reason": "wrong", "updatedAt": index["3"]["feedback"]["updatedAt"]} + assert index["5"] == {"feedback": {"value": 1, "updatedAt": index["5"]["feedback"]["updatedAt"]}} + + monkeypatch.setenv("MESSAGE_FEEDBACK_ENABLED", "false") + index = await md.get_all_message_metadata(SESSION, OWNER) + assert "feedback" not in index["3"] and "5" not in index + + +@pytest.mark.asyncio +async def test_message_metadata_model_round_trips_feedback(table): + from apis.shared.sessions.models import MessageMetadata + + meta = MessageMetadata(**{"cost": 0.1, "feedback": {"value": 1, "updatedAt": "2026-09-16T00:00:00Z"}}) + assert meta.feedback is not None and meta.feedback.value == 1 + assert meta.model_dump(by_alias=True, exclude_none=True)["feedback"] == {"value": 1, "updatedAt": "2026-09-16T00:00:00Z"} diff --git a/frontend/ai.client/src/app/admin/costs/models/admin-cost.models.ts b/frontend/ai.client/src/app/admin/costs/models/admin-cost.models.ts index 0f500c9ef..364f0f5b3 100644 --- a/frontend/ai.client/src/app/admin/costs/models/admin-cost.models.ts +++ b/frontend/ai.client/src/app/admin/costs/models/admin-cost.models.ts @@ -377,6 +377,29 @@ export interface DataCoverage { prefixTokens?: boolean; windowTrim?: boolean; compactionEvents?: boolean; + /** Any thumbs row, or a session rollup written while diagnostics were on. */ + feedback?: boolean; +} + +/** Thumbs on one bucket of calls — counts, never content. */ +export interface FeedbackCounts { + up: number; + down: number; +} + +/** Turn classes from the document-context offload spec §6.1. */ +export type TurnClass = 'full' | 'digestOnly' | 'retrieved' | 'none'; + +/** + * The outcome signal joined to the cost rows. `byTurnClass` is null when no + * cost row carries the turn-class fields (they arrive with offload PR-1); + * that is "not tracked", not zero. `unjoined` thumbs have no cost row. + */ +export interface FeedbackProfile { + up: number; + down: number; + byTurnClass?: Record | null; + unjoined?: number; } /** The content-free diagnostic profile of one conversation. */ @@ -405,6 +428,8 @@ export interface SessionProfile { compactionEventCounts?: Record; /** The summary's token size at the most recent compaction decision. */ lastSummaryTokens?: number | null; + /** Thumbs up/down joined to the cost rows by (sessionId, messageId). */ + feedback?: FeedbackProfile; } // ========== API Request Options ========== diff --git a/frontend/ai.client/src/app/admin/costs/pages/session-cost-anatomy.page.ts b/frontend/ai.client/src/app/admin/costs/pages/session-cost-anatomy.page.ts index 63e37c001..450a9d2f3 100644 --- a/frontend/ai.client/src/app/admin/costs/pages/session-cost-anatomy.page.ts +++ b/frontend/ai.client/src/app/admin/costs/pages/session-cost-anatomy.page.ts @@ -35,6 +35,8 @@ import { import { SEVERITY_LABELS, buildDiagnosticJson, + downRate, + feedbackByTurnClassLine, formatBytes, formatEvidenceValue, humanizeKey, @@ -218,6 +220,36 @@ import {

healthy ≈ 0.1

+
+

Feedback

+ @if (profile.dataCoverage.feedback && profile.feedback; as feedback) { + +

+ {{ feedbackDownRate() != null ? feedbackDownRate() + '% down' : '—' }} +

+

+ {{ feedback.up }} up · {{ feedback.down }} down + @if (feedbackTurnClassLine(); as byClass) { + · {{ byClass }} + } @else { + · turn class not tracked + } +

+ } @else { +

—

+

not tracked

+ } +
@@ -775,6 +807,16 @@ export class SessionCostAnatomyPage { * deducting it, so the page does the subtraction where a reader can see both halves. */ /** "3 applied · 1 forced · summary 2.3K" — the compaction decisions by kind. */ + readonly feedbackDownRate = computed(() => { + if (!this.profileResource.hasValue()) return null; + const feedback = this.profileResource.value().feedback; + return feedback ? downRate(feedback) : null; + }); + + readonly feedbackTurnClassLine = computed(() => + this.profileResource.hasValue() ? feedbackByTurnClassLine(this.profileResource.value().feedback) : null, + ); + readonly compactionEventsLine = computed(() => { if (!this.profileResource.hasValue()) return ''; const p = this.profileResource.value(); diff --git a/frontend/ai.client/src/app/admin/costs/pages/session-profile.util.spec.ts b/frontend/ai.client/src/app/admin/costs/pages/session-profile.util.spec.ts index 7e7114cc6..9763d7ef9 100644 --- a/frontend/ai.client/src/app/admin/costs/pages/session-profile.util.spec.ts +++ b/frontend/ai.client/src/app/admin/costs/pages/session-profile.util.spec.ts @@ -2,6 +2,8 @@ import { describe, it, expect } from 'vitest'; import { buildDiagnosticJson, cleanCeiling, + downRate, + feedbackByTurnClassLine, formatBytes, formatEvidenceValue, formatTokensShort, @@ -20,6 +22,41 @@ function point(callIndex: number, contextTokens: number, cacheStatus: ContextTra } describe('session-profile.util', () => { + describe('feedback', () => { + it('downRate is a whole percentage, null with nothing to rate', () => { + expect(downRate({ up: 0, down: 0 })).toBeNull(); + expect(downRate({ up: 3, down: 1 })).toBe(25); + expect(downRate({ up: 0, down: 2 })).toBe(100); + }); + + it('feedbackByTurnClassLine reports rate and n per class, skipping empty classes', () => { + const line = feedbackByTurnClassLine({ + up: 3, + down: 2, + byTurnClass: { + full: { up: 1, down: 1 }, + digestOnly: { up: 2, down: 0 }, + retrieved: { up: 0, down: 0 }, + none: { up: 0, down: 1 }, + }, + }); + expect(line).toBe('full 50% of 2 · digest 0% of 2 · no docs 100% of 1'); + }); + + it('feedbackByTurnClassLine is null when the turn class is not tracked', () => { + expect(feedbackByTurnClassLine({ up: 1, down: 1, byTurnClass: null })).toBeNull(); + expect(feedbackByTurnClassLine({ up: 1, down: 1 })).toBeNull(); + expect(feedbackByTurnClassLine(undefined)).toBeNull(); + expect( + feedbackByTurnClassLine({ + up: 0, + down: 0, + byTurnClass: { full: { up: 0, down: 0 }, digestOnly: { up: 0, down: 0 }, retrieved: { up: 0, down: 0 }, none: { up: 0, down: 0 } }, + }), + ).toBeNull(); + }); + }); + describe('cleanCeiling', () => { it('rounds up to a 1/2/2.5/5/10 step of the magnitude', () => { expect(cleanCeiling(0)).toBe(1); diff --git a/frontend/ai.client/src/app/admin/costs/pages/session-profile.util.ts b/frontend/ai.client/src/app/admin/costs/pages/session-profile.util.ts index 3b909ef90..7463f493c 100644 --- a/frontend/ai.client/src/app/admin/costs/pages/session-profile.util.ts +++ b/frontend/ai.client/src/app/admin/costs/pages/session-profile.util.ts @@ -2,8 +2,10 @@ import { CacheStatus, ContextTrajectoryPoint, DiagnosisSeverity, + FeedbackProfile, SessionCostAnatomy, SessionProfile, + TurnClass, } from '../models'; /** @@ -207,3 +209,38 @@ export function buildDiagnosticJson( 2, ); } + +// ── feedback ──────────────────────────────────────────────────────────────── + +export const TURN_CLASS_LABELS: Record = { + full: 'full', + digestOnly: 'digest', + retrieved: 'retrieved', + none: 'no docs', +}; + +const TURN_CLASS_ORDER: TurnClass[] = ['full', 'digestOnly', 'retrieved', 'none']; + +/** Down-thumb rate as a percentage, or null with nothing to rate. */ +export function downRate(counts: { up: number; down: number }): number | null { + const n = counts.up + counts.down; + return n > 0 ? Math.round((counts.down / n) * 100) : null; +} + +/** + * One line of down-thumb rate per turn class, with n per class + * (`full 50% of 4 · digest 0% of 2`). Classes with no thumbs are skipped; + * null when the turn class is not tracked or nothing was thumbed. + */ +export function feedbackByTurnClassLine(feedback: FeedbackProfile | null | undefined): string | null { + const by = feedback?.byTurnClass; + if (!by) return null; + const parts = TURN_CLASS_ORDER.flatMap((klass) => { + const counts = by[klass]; + if (!counts) return []; + const n = counts.up + counts.down; + if (n === 0) return []; + return [`${TURN_CLASS_LABELS[klass]} ${downRate(counts)}% of ${n}`]; + }); + return parts.length > 0 ? parts.join(' · ') : null; +} diff --git a/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.spec.ts b/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.spec.ts index 9fcd62c51..21bda944b 100644 --- a/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.spec.ts +++ b/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.spec.ts @@ -2,7 +2,9 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; import { describe, it, expect, beforeEach } from 'vitest'; import { provideMarkdown, MarkdownService } from 'ngx-markdown'; import { MessageActionsComponent } from './message-actions.component'; -import { Message } from '../../../services/models/message.model'; +import { Message, MessageFeedback } from '../../../services/models/message.model'; +import { MessageFeedbackService } from '../../../services/session/message-feedback.service'; +import { signal } from '@angular/core'; function makeMessage(text: string): Message { return { @@ -120,3 +122,100 @@ describe('MessageActionsComponent — interrupted-turn chip', () => { expect(fixture.nativeElement.textContent).not.toContain('Response interrupted'); }); }); + +/** Duck-typed MessageFeedbackService: records calls, serves a fixed value. */ +class FakeFeedbackService { + unavailable = signal(false); + isPending = signal(() => false); + current: MessageFeedback | null = null; + set: Array<{ id: string; value: 1 | -1; reason?: string }> = []; + cleared: string[] = []; + feedbackFor(): MessageFeedback | null { + return this.current; + } + async setFeedback(message: Message, value: 1 | -1, reason?: string): Promise { + this.set.push({ id: message.id, value, reason }); + } + async clearFeedback(message: Message): Promise { + this.cleared.push(message.id); + } +} + +function serverMessage(text: string, id = 'msg-sess-1-3'): Message { + return { id, role: 'assistant', content: [{ type: 'text', text }] }; +} + +describe('MessageActionsComponent — thumbs feedback', () => { + let fixture: ComponentFixture; + let feedback: FakeFeedbackService; + + const up = () => fixture.nativeElement.querySelector('button[aria-label="Good response"], button[aria-label="Remove thumbs up"]') as HTMLButtonElement | null; + const down = () => fixture.nativeElement.querySelector('button[aria-label="Bad response"], button[aria-label="Remove thumbs down"]') as HTMLButtonElement | null; + + beforeEach(async () => { + feedback = new FakeFeedbackService(); + await TestBed.configureTestingModule({ + imports: [MessageActionsComponent], + providers: [provideMarkdown(), { provide: MessageFeedbackService, useValue: feedback }], + }).compileComponents(); + TestBed.inject(MarkdownService).parse = () => ''; + fixture = TestBed.createComponent(MessageActionsComponent); + fixture.componentRef.setInput('messages', [serverMessage('answer')]); + }); + + it('renders an unpressed thumbs pair with tooltips for a server-shaped message', () => { + fixture.detectChanges(); + expect(up()).not.toBeNull(); + expect(down()).not.toBeNull(); + expect(up()!.getAttribute('aria-pressed')).toBe('false'); + expect(up()!.getAttribute('aria-label')).toBe('Good response'); + // No free-text input anywhere in the affordance. + expect(fixture.nativeElement.querySelector('input, textarea')).toBeNull(); + }); + + it('hides the pair for a message whose id carries no server index', () => { + fixture.componentRef.setInput('messages', [serverMessage('x', 'placeholder')]); + fixture.detectChanges(); + expect(up()).toBeNull(); + }); + + it('hides the pair once the service reports the surface unavailable', () => { + feedback.unavailable.set(true); + fixture.detectChanges(); + expect(up()).toBeNull(); + }); + + it('thumbs up sets +1 on the run\'s last message', () => { + fixture.componentRef.setInput('messages', [serverMessage('a', 'msg-sess-1-2'), serverMessage('b', 'msg-sess-1-3')]); + fixture.detectChanges(); + up()!.click(); + expect(feedback.set).toEqual([{ id: 'msg-sess-1-3', value: 1, reason: undefined }]); + }); + + it('clicking the pressed thumb withdraws it', () => { + feedback.current = { value: 1, updatedAt: 't' }; + fixture.detectChanges(); + expect(up()!.getAttribute('aria-pressed')).toBe('true'); + expect(up()!.getAttribute('aria-label')).toBe('Remove thumbs up'); + up()!.click(); + expect(feedback.cleared).toEqual(['msg-sess-1-3']); + expect(feedback.set).toEqual([]); + }); + + it('a thumbs down reveals the reason codes and a pick re-sends with the code', () => { + feedback.current = { value: -1, updatedAt: 't' }; + fixture.detectChanges(); + const group = fixture.nativeElement.querySelector('[role="group"]'); + expect(group).not.toBeNull(); + const chips = Array.from(group.querySelectorAll('button')) as HTMLButtonElement[]; + expect(chips.map((c) => c.textContent!.trim())).toEqual(['Wrong', 'Incomplete', 'Slow', 'Other']); + chips[1].click(); + expect(feedback.set).toEqual([{ id: 'msg-sess-1-3', value: -1, reason: 'incomplete' }]); + }); + + it('reason codes stay hidden on a thumbs up', () => { + feedback.current = { value: 1, updatedAt: 't' }; + fixture.detectChanges(); + expect(fixture.nativeElement.querySelector('[role="group"]')).toBeNull(); + }); +}); diff --git a/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.ts b/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.ts index 4222fe52e..09e2fb712 100644 --- a/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.ts +++ b/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.ts @@ -10,16 +10,28 @@ import { } from '@angular/core'; import { isPlatformBrowser } from '@angular/common'; import { NgIcon, provideIcons } from '@ng-icons/core'; -import { heroSquare2Stack, heroCheck, heroArrowPath } from '@ng-icons/heroicons/outline'; +import { heroSquare2Stack, heroCheck, heroArrowPath, heroHandThumbUp, heroHandThumbDown } from '@ng-icons/heroicons/outline'; +import { heroHandThumbUpSolid, heroHandThumbDownSolid } from '@ng-icons/heroicons/solid'; import { MarkdownService } from 'ngx-markdown'; -import { Message, isTextContentBlock } from '../../../services/models/message.model'; +import { FeedbackReason, Message, isTextContentBlock } from '../../../services/models/message.model'; +import { FEEDBACK_REASONS, MessageFeedbackService, parseMessageRef } from '../../../services/session/message-feedback.service'; import { TooltipDirective } from '../../../../components/tooltip'; @Component({ selector: 'app-message-actions', changeDetection: ChangeDetectionStrategy.OnPush, imports: [NgIcon, TooltipDirective], - providers: [provideIcons({ heroSquare2Stack, heroCheck, heroArrowPath })], + providers: [ + provideIcons({ + heroSquare2Stack, + heroCheck, + heroArrowPath, + heroHandThumbUp, + heroHandThumbDown, + heroHandThumbUpSolid, + heroHandThumbDownSolid, + }), + ], template: `
+ + + @if (feedbackValue() === -1) { + +
+ @for (reason of reasons; track reason) { + + } +
+ } + } + @if (canContinue()) { Response length limit reached @@ -92,6 +153,7 @@ export class MessageActionsComponent { private platformId = inject(PLATFORM_ID); private isBrowser = isPlatformBrowser(this.platformId); private markdown = inject(MarkdownService); + private feedbackService = inject(MessageFeedbackService); /** * The assistant messages of one run (see AssistantMessageComponent). @@ -129,6 +191,68 @@ export class MessageActionsComponent { protected hasCopyableText = computed(() => this.copyableText().length > 0); + // ── feedback ── + // The thumb keys on the run's LAST message (the one whose cost row + // describes the finished answer — see message-list.component.html). + protected readonly reasons = FEEDBACK_REASONS; + protected readonly reasonLabels: Record = { + wrong: 'Wrong', + incomplete: 'Incomplete', + slow: 'Slow', + other: 'Other', + }; + + private lastMessage = computed(() => { + const messages = this.messages(); + return messages.length > 0 ? messages[messages.length - 1] : null; + }); + + /** Only messages with a server-shaped id (`msg-{session}-{index}`) can be + * thumbed: that index is the key the cost row shares. */ + protected showFeedback = computed(() => { + const last = this.lastMessage(); + return !!last && !this.feedbackService.unavailable() && parseMessageRef(last.id) !== null; + }); + + protected feedback = computed(() => { + const last = this.lastMessage(); + return last ? this.feedbackService.feedbackFor(last) : null; + }); + protected feedbackValue = computed(() => this.feedback()?.value ?? null); + protected feedbackReason = computed(() => this.feedback()?.reason ?? null); + protected feedbackPending = computed(() => { + const last = this.lastMessage(); + return !!last && this.feedbackService.isPending()(last.id); + }); + + thumb(value: 1 | -1): void { + const last = this.lastMessage(); + if (!last) return; + if (this.feedbackValue() === value) { + void this.feedbackService.clearFeedback(last); + } else { + void this.feedbackService.setFeedback(last, value); + } + } + + pickReason(reason: FeedbackReason): void { + const last = this.lastMessage(); + if (!last || this.feedbackValue() !== -1) return; + void this.feedbackService.setFeedback(last, -1, reason); + } + + protected thumbClass(value: 1 | -1): string { + return this.feedbackValue() === value + ? 'text-primary-accessible dark:text-primary-accessible-dark' + : 'text-gray-500 hover:text-gray-700 dark:text-gray-400 dark:hover:text-gray-200'; + } + + protected reasonClass(reason: FeedbackReason): string { + return this.feedbackReason() === reason + ? 'border-primary-accessible bg-primary-accessible text-white dark:border-primary-accessible-dark dark:bg-primary-accessible-dark dark:text-gray-900' + : 'border-gray-300 bg-white text-gray-600 hover:bg-gray-100 dark:border-gray-600 dark:bg-gray-800 dark:text-gray-300 dark:hover:bg-gray-700'; + } + async copy(): Promise { if (!this.isBrowser || !this.hasCopyableText()) return; diff --git a/frontend/ai.client/src/app/session/services/models/message.model.ts b/frontend/ai.client/src/app/session/services/models/message.model.ts index 1ec55d898..474c0a643 100644 --- a/frontend/ai.client/src/app/session/services/models/message.model.ts +++ b/frontend/ai.client/src/app/session/services/models/message.model.ts @@ -103,6 +103,21 @@ export interface ContentBlock { fileAttachment?: FileAttachmentData | null; } +/** Reason codes a thumbs-down may carry — a closed enum, never free text. */ +export type FeedbackReason = 'wrong' | 'incomplete' | 'slow' | 'other'; + +/** + * A user's thumb on an assistant message. Persisted content-free on the + * sessions-metadata table beside the message's cost row and merged onto + * `metadata.feedback` by `GET /sessions/{id}/messages`. + */ +export interface MessageFeedback { + /** +1 thumbs up, -1 thumbs down */ + value: 1 | -1; + reason?: FeedbackReason; + updatedAt: string; +} + /** * Message model matching the backend API MessageResponse. * This is the canonical Message type used throughout the application. diff --git a/frontend/ai.client/src/app/session/services/session/message-feedback.service.spec.ts b/frontend/ai.client/src/app/session/services/session/message-feedback.service.spec.ts new file mode 100644 index 000000000..c1c802167 --- /dev/null +++ b/frontend/ai.client/src/app/session/services/session/message-feedback.service.spec.ts @@ -0,0 +1,114 @@ +import { TestBed } from '@angular/core/testing'; +import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing'; +import { provideHttpClient } from '@angular/common/http'; +import { signal } from '@angular/core'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ConfigService } from '../../../services/config.service'; +import { Message } from '../models/message.model'; +import { + MessageFeedbackService, + parseMessageRef, + readPersistedFeedback, +} from './message-feedback.service'; + +function message(id: string, metadata: Record | null = null): Message { + return { id, role: 'assistant', content: [{ type: 'text', text: 'hi' }], metadata }; +} + +describe('message-feedback helpers', () => { + it('parseMessageRef splits on the LAST dash so dashed session ids survive', () => { + expect(parseMessageRef('msg-abc-def-7')).toEqual({ sessionId: 'abc-def', index: 7 }); + expect(parseMessageRef('msg-s-0')).toEqual({ sessionId: 's', index: 0 }); + expect(parseMessageRef('placeholder')).toBeNull(); + expect(parseMessageRef('msg-s-x')).toBeNull(); + expect(parseMessageRef('msg--1')).toBeNull(); + }); + + it('readPersistedFeedback accepts only ±1 and known reason codes', () => { + expect(readPersistedFeedback(message('m'))).toBeNull(); + expect(readPersistedFeedback(message('m', { feedback: { value: 2 } }))).toBeNull(); + expect(readPersistedFeedback(message('m', { feedback: { value: -1, reason: 'wrong', updatedAt: 't' } }))).toEqual({ + value: -1, + reason: 'wrong', + updatedAt: 't', + }); + // An unknown reason is dropped rather than rendered — it cannot be a chip. + expect(readPersistedFeedback(message('m', { feedback: { value: 1, reason: 'free text' } }))?.reason).toBeUndefined(); + }); +}); + +describe('MessageFeedbackService', () => { + let service: MessageFeedbackService; + let http: HttpTestingController; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [ + provideHttpClient(), + provideHttpClientTesting(), + { provide: ConfigService, useValue: { appApiUrl: signal('http://api.test/') } }, + ], + }); + service = TestBed.inject(MessageFeedbackService); + http = TestBed.inject(HttpTestingController); + }); + + afterEach(() => http.verify()); + + it('prefers what the server sent with the message until a click overrides it', () => { + const m = message('msg-s-3', { feedback: { value: 1, updatedAt: 't' } }); + expect(service.feedbackFor(m)?.value).toBe(1); + }); + + it('PUTs a content-free body to the message index and keeps the optimistic value', async () => { + const m = message('msg-s-3'); + const done = service.setFeedback(m, -1, 'slow'); + expect(service.feedbackFor(m)?.value).toBe(-1); + + const req = http.expectOne('http://api.test/sessions/s/messages/3/feedback'); + expect(req.request.method).toBe('PUT'); + expect(req.request.body).toEqual({ value: -1, reason: 'slow' }); + req.flush({ value: -1, reason: 'slow', updatedAt: '2026-09-16T00:00:00Z' }); + await done; + expect(service.feedbackFor(m)).toEqual({ value: -1, reason: 'slow', updatedAt: '2026-09-16T00:00:00Z' }); + }); + + it('rolls back to the last confirmed value when the write fails', async () => { + const m = message('msg-s-3', { feedback: { value: 1, updatedAt: 't' } }); + const done = service.setFeedback(m, -1); + expect(service.feedbackFor(m)?.value).toBe(-1); + http.expectOne('http://api.test/sessions/s/messages/3/feedback').flush('nope', { status: 500, statusText: 'err' }); + await done; + expect(service.feedbackFor(m)?.value).toBe(1); + expect(service.unavailable()).toBe(false); + }); + + it('DELETE withdraws the thumb', async () => { + const m = message('msg-s-3', { feedback: { value: 1, updatedAt: 't' } }); + const done = service.clearFeedback(m); + expect(service.feedbackFor(m)).toBeNull(); + const req = http.expectOne('http://api.test/sessions/s/messages/3/feedback'); + expect(req.request.method).toBe('DELETE'); + req.flush(null, { status: 204, statusText: 'No Content' }); + await done; + expect(service.feedbackFor(m)).toBeNull(); + }); + + it('marks the surface unavailable only on the kill-switch 404', async () => { + const m = message('msg-s-3'); + let done = service.setFeedback(m, 1); + http.expectOne('http://api.test/sessions/s/messages/3/feedback').flush({ detail: 'Session not found: s' }, { status: 404, statusText: 'nf' }); + await done; + expect(service.unavailable()).toBe(false); + + done = service.setFeedback(m, 1); + http.expectOne('http://api.test/sessions/s/messages/3/feedback').flush({ detail: 'Not found' }, { status: 404, statusText: 'nf' }); + await done; + expect(service.unavailable()).toBe(true); + }); + + it('ignores messages without a server index', async () => { + await service.setFeedback(message('placeholder'), 1); + http.expectNone(() => true); + }); +}); diff --git a/frontend/ai.client/src/app/session/services/session/message-feedback.service.ts b/frontend/ai.client/src/app/session/services/session/message-feedback.service.ts new file mode 100644 index 000000000..02f448838 --- /dev/null +++ b/frontend/ai.client/src/app/session/services/session/message-feedback.service.ts @@ -0,0 +1,161 @@ +import { computed, inject, Injectable, signal } from '@angular/core'; +import { HttpClient } from '@angular/common/http'; +import { firstValueFrom } from 'rxjs'; +import { ConfigService } from '../../../services/config.service'; +import { Message, MessageFeedback, FeedbackReason } from '../models/message.model'; + +/** + * Thumbs up / down on assistant messages — the outcome signal joined to the + * cost rows (docs/specs/document-context-offload.md §5 row 7). + * + * Content-free by construction: the wire carries `value` (+1 / -1) and an + * optional `reason` code from a closed enum. There is no text field anywhere + * in this service, so nothing a user types can reach the metadata table. + * + * State is optimistic: a click flips the local value immediately, the PUT / + * DELETE runs after, and a failure rolls the local value back to what the + * server last confirmed. On reload the value comes back on the message's + * metadata (`metadata.feedback`, merged from the `F#` row by `GET /messages`), + * which `feedbackFor` prefers unless a local click has overridden it. + */ +@Injectable({ providedIn: 'root' }) +export class MessageFeedbackService { + private readonly http = inject(HttpClient); + private readonly config = inject(ConfigService); + + /** Local overrides keyed by message id; `null` = withdrawn. */ + private readonly overrides = signal>(new Map()); + + /** Message ids with a request in flight (disables the pair while pending). */ + private readonly pending = signal>(new Set()); + + /** + * Set once the backend answers 404 with the kill switch off: the surface + * does not exist in this environment, so the buttons hide. + */ + readonly unavailable = signal(false); + + readonly isPending = computed(() => (id: string) => this.pending().has(id)); + + /** The effective thumb for a message: a local override wins, else what + * the server sent with the message. */ + feedbackFor(message: Message): MessageFeedback | null { + const overrides = this.overrides(); + if (overrides.has(message.id)) return overrides.get(message.id) ?? null; + return readPersistedFeedback(message); + } + + /** Thumb a message, replacing any earlier thumb (one per user+message). */ + async setFeedback(message: Message, value: 1 | -1, reason?: FeedbackReason): Promise { + const target = parseMessageRef(message.id); + if (!target) return; + const previous = this.feedbackFor(message); + const optimistic: MessageFeedback = { + value, + reason: reason ?? (previous?.value === value ? previous?.reason : undefined), + updatedAt: new Date().toISOString(), + }; + this.setOverride(message.id, optimistic); + this.markPending(message.id, true); + try { + const body: { value: 1 | -1; reason?: FeedbackReason } = { value }; + if (optimistic.reason) body.reason = optimistic.reason; + const stored = await firstValueFrom( + this.http.put(this.url(target.sessionId, target.index), body), + ); + this.setOverride(message.id, stored); + } catch (error) { + this.rollback(message.id, previous, error); + } finally { + this.markPending(message.id, false); + } + } + + /** Withdraw the thumb on a message. */ + async clearFeedback(message: Message): Promise { + const target = parseMessageRef(message.id); + if (!target) return; + const previous = this.feedbackFor(message); + this.setOverride(message.id, null); + this.markPending(message.id, true); + try { + await firstValueFrom(this.http.delete(this.url(target.sessionId, target.index))); + } catch (error) { + this.rollback(message.id, previous, error); + } finally { + this.markPending(message.id, false); + } + } + + private url(sessionId: string, index: number): string { + const base = this.config.appApiUrl().replace(/\/$/, ''); + return `${base}/sessions/${encodeURIComponent(sessionId)}/messages/${index}/feedback`; + } + + private rollback(messageId: string, previous: MessageFeedback | null, error: unknown): void { + if (isKillSwitch404(error)) { + // MESSAGE_FEEDBACK_ENABLED=false: the surface does not exist here, so + // hide the pair rather than keep offering a click that cannot land. + // (A missing session is also a 404, but with a different detail, and + // must not hide feedback everywhere else.) + this.unavailable.set(true); + } + this.setOverride(messageId, previous); + console.warn('Message feedback not saved:', error); + } + + private setOverride(messageId: string, value: MessageFeedback | null): void { + this.overrides.update((current) => { + const next = new Map(current); + next.set(messageId, value); + return next; + }); + } + + private markPending(messageId: string, on: boolean): void { + this.pending.update((current) => { + const next = new Set(current); + if (on) next.add(messageId); + else next.delete(messageId); + return next; + }); + } +} + +/** `msg-{sessionId}-{index}` → the session and the 0-based message index the + * cost row keys on. Splits on the LAST dash so a session id with dashes is + * irrelevant. `null` for any id not in that shape (a client-only placeholder). */ +export function parseMessageRef(messageId: string): { sessionId: string; index: number } | null { + if (!messageId.startsWith('msg-')) return null; + const cut = messageId.lastIndexOf('-'); + if (cut <= 4) return null; + const index = Number(messageId.slice(cut + 1)); + if (!Number.isInteger(index) || index < 0) return null; + return { sessionId: messageId.slice(4, cut), index }; +} + +/** The thumb `GET /messages` merged onto the message's metadata, if any. */ +export function readPersistedFeedback(message: Message): MessageFeedback | null { + const raw = message.metadata?.['feedback']; + if (!raw || typeof raw !== 'object') return null; + const value = (raw as { value?: unknown }).value; + if (value !== 1 && value !== -1) return null; + const reason = (raw as { reason?: unknown }).reason; + return { + value, + reason: isFeedbackReason(reason) ? reason : undefined, + updatedAt: String((raw as { updatedAt?: unknown }).updatedAt ?? ''), + }; +} + +export const FEEDBACK_REASONS: readonly FeedbackReason[] = ['wrong', 'incomplete', 'slow', 'other']; + +export function isFeedbackReason(value: unknown): value is FeedbackReason { + return typeof value === 'string' && (FEEDBACK_REASONS as readonly string[]).includes(value); +} + +function isKillSwitch404(error: unknown): boolean { + if (typeof error !== 'object' || error === null) return false; + const { status, error: body } = error as { status?: number; error?: { detail?: unknown } }; + return status === 404 && body?.detail === 'Not found'; +} From 8d5739c226b7d4c7f63f68fd5db6b617cfcfebf6 Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Wed, 16 Sep 2026 16:39:56 -0600 Subject: [PATCH 2/4] test(admin): profile dataCoverage now carries feedback Co-Authored-By: Claude Fable 5.1 --- backend/tests/apis/app_api/admin/costs/test_drilldown_routes.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/tests/apis/app_api/admin/costs/test_drilldown_routes.py b/backend/tests/apis/app_api/admin/costs/test_drilldown_routes.py index 0d5d9f569..6311b1f1e 100644 --- a/backend/tests/apis/app_api/admin/costs/test_drilldown_routes.py +++ b/backend/tests/apis/app_api/admin/costs/test_drilldown_routes.py @@ -105,7 +105,9 @@ def test_session_profile_returns_200(): assert body["dataCoverage"] == { "toolCensus": False, "compactionCount": False, "fingerprints": False, "cost": False, "prefixTokens": False, "windowTrim": False, "compactionEvents": False, + "feedback": False, } + assert body["feedback"] == {"up": 0, "down": 0, "byTurnClass": None, "unjoined": 0} service.get_session_profile.assert_awaited_once_with("s1") From 5992539726614a3fafe4e9cb29d6ef2d6c34139c Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Wed, 16 Sep 2026 17:01:20 -0600 Subject: [PATCH 3/4] =?UTF-8?q?feat(feedback):=20align=20with=20response-f?= =?UTF-8?q?eedback=20spec=20=E2=80=94=20flag=20name,=20six=20reason=20buck?= =?UTF-8?q?ets,=20explicit/implicit=20signal=20discriminator?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RESPONSE_FEEDBACK_ENABLED per spec §5; reason codes are the spec's §6 buckets (still a closed enum — free text stays out by decision, recorded in the spec's new §13); rows carry signal="explicit" and every thumb reader filters on it so the §10 implicit phase needs no backfill and never sums in. Co-Authored-By: Claude Fable 5.1 --- .../src/apis/app_api/admin/costs/service.py | 7 +++- backend/src/apis/app_api/messages/models.py | 14 ++++--- backend/src/apis/app_api/sessions/routes.py | 6 +-- backend/src/apis/shared/feature_flags.py | 7 ++-- .../shared/observability/content_policy.py | 4 +- backend/src/apis/shared/sessions/feedback.py | 18 +++++++- backend/src/apis/shared/sessions/metadata.py | 4 +- backend/src/apis/shared/sessions/models.py | 14 ++++--- .../costs/test_session_profile_service.py | 14 ++++++- .../app_api/test_message_feedback_routes.py | 16 +++---- .../costs/test_content_free_projections.py | 4 +- backend/tests/shared/test_message_feedback.py | 30 ++++++++++--- docs/specs/response-feedback.md | 42 ++++++++++++++++++- .../message-actions.component.spec.ts | 11 ++++- .../components/message-actions.component.ts | 12 +++--- .../session/services/models/message.model.ts | 5 ++- .../session/message-feedback.service.spec.ts | 8 ++-- .../session/message-feedback.service.ts | 2 +- 18 files changed, 162 insertions(+), 56 deletions(-) diff --git a/backend/src/apis/app_api/admin/costs/service.py b/backend/src/apis/app_api/admin/costs/service.py index a5aa76e16..83a88c714 100644 --- a/backend/src/apis/app_api/admin/costs/service.py +++ b/backend/src/apis/app_api/admin/costs/service.py @@ -111,7 +111,8 @@ def _join_feedback( feedback_rows: List[Dict[str, Any]], ) -> FeedbackProfile: """Join ``F#`` rows to ``C#`` rows on ``messageId`` and bucket by turn - class. Pure; the profile's numbers, never any content.""" + class. Pure; the profile's numbers, never any content. Explicit thumbs + only (``signal`` absent or ``"explicit"``).""" by_message: Dict[int, Dict[str, Any]] = {} for record in records: message_id = _as_int(record.get("messageId")) @@ -125,6 +126,10 @@ def _join_feedback( buckets = FeedbackByTurnClass() if any_turn_class else None profile = FeedbackProfile() for row in feedback_rows: + # Explicit thumbs only — implicit signals (spec §10) share the row + # family but answer a different question and must never be summed in. + if row.get("signal") not in (None, "explicit"): + continue value = _as_int(row.get("value")) if value not in (1, -1): continue diff --git a/backend/src/apis/app_api/messages/models.py b/backend/src/apis/app_api/messages/models.py index 99d83ccbc..95d4fc006 100644 --- a/backend/src/apis/app_api/messages/models.py +++ b/backend/src/apis/app_api/messages/models.py @@ -117,11 +117,15 @@ class Citation(BaseModel): text: str = Field(..., description="Relevant text excerpt from the document") -#: Reason codes a down-thumb may carry. A closed enum, never free text — the -#: row is content-free by construction so it can sit beside the ``C#`` cost -#: row and be read by the admin profile without reading the conversation. -FEEDBACK_REASONS = ("wrong", "incomplete", "slow", "other") -FeedbackReason = Literal["wrong", "incomplete", "slow", "other"] +#: Reason codes a down-thumb may carry — the six buckets of +#: ``docs/specs/response-feedback.md`` §6, each of which routes to an +#: evaluator or an ops signal. A closed enum, never free text: the row is +#: content-free by construction so it can sit beside the ``C#`` cost row and +#: be read by the admin profile without reading the conversation. The spec's +#: "something else → free text" is deliberately not here; that hand-off is +#: the existing Agent report dialog (spec §3), which already has moderation. +FEEDBACK_REASONS = ("wrong", "instructions", "length", "tool_failed", "outdated", "other") +FeedbackReason = Literal["wrong", "instructions", "length", "tool_failed", "outdated", "other"] class MessageFeedback(BaseModel): diff --git a/backend/src/apis/app_api/sessions/routes.py b/backend/src/apis/app_api/sessions/routes.py index 6b40bed14..5d069c9bd 100644 --- a/backend/src/apis/app_api/sessions/routes.py +++ b/backend/src/apis/app_api/sessions/routes.py @@ -42,7 +42,7 @@ from apis.app_api.shares.service import get_share_service from apis.app_api.artifacts.service import get_artifact_share_service from apis.shared.auth.dependencies import get_current_user_from_session -from apis.shared.feature_flags import message_feedback_enabled, mid_turn_steering_enabled +from apis.shared.feature_flags import response_feedback_enabled, mid_turn_steering_enabled from apis.shared.auth.models import User from apis.shared.system_prompts.service import get_system_prompts_service @@ -670,8 +670,8 @@ async def get_session_messages_endpoint( def _require_message_feedback() -> None: - """404 while ``MESSAGE_FEEDBACK_ENABLED=false`` — the surface does not exist.""" - if not message_feedback_enabled(): + """404 while ``RESPONSE_FEEDBACK_ENABLED=false`` — the surface does not exist.""" + if not response_feedback_enabled(): raise HTTPException(status_code=404, detail="Not found") diff --git a/backend/src/apis/shared/feature_flags.py b/backend/src/apis/shared/feature_flags.py index 6ce00b90c..2e08a93b9 100644 --- a/backend/src/apis/shared/feature_flags.py +++ b/backend/src/apis/shared/feature_flags.py @@ -354,7 +354,7 @@ def ask_user_question_enabled() -> bool: return os.environ.get("ASK_USER_QUESTION_ENABLED", "").strip().lower() != "false" -def message_feedback_enabled() -> bool: +def response_feedback_enabled() -> bool: """Whether users can thumb an assistant message up or down. Covers the ``PUT`` / ``DELETE /sessions/{id}/messages/{messageId}/feedback`` @@ -363,7 +363,8 @@ def message_feedback_enabled() -> bool: kill switch** (house style, mirroring ``cost_diagnostics_enabled``): unset or empty resolves to enabled; only the literal ``"false"`` (case- insensitive) disables. While off the write routes 404 and the read merge - is skipped; rows already written stay in the table. + is skipped; rows already written stay in the table. Name and default per + ``docs/specs/response-feedback.md`` §5. The signal is content-free by construction (a ±1, a timestamp and an optional reason *code* from a fixed enum — never free text), which is @@ -371,4 +372,4 @@ def message_feedback_enabled() -> bool: profile without the profile ever reading the conversation. See ``docs/specs/document-context-offload.md`` §5 row 7 / §6.1. """ - return os.environ.get("MESSAGE_FEEDBACK_ENABLED", "").strip().lower() != "false" + return os.environ.get("RESPONSE_FEEDBACK_ENABLED", "").strip().lower() != "false" diff --git a/backend/src/apis/shared/observability/content_policy.py b/backend/src/apis/shared/observability/content_policy.py index 5b7b85f6f..e2586ccf5 100644 --- a/backend/src/apis/shared/observability/content_policy.py +++ b/backend/src/apis/shared/observability/content_policy.py @@ -183,13 +183,15 @@ def is_content_bearing(path: str) -> bool: ) #: F# rows for the session profile's feedback join. A thumb is a ±1, an -#: optional reason *code* and a timestamp — never text, by the request +#: optional reason *code*, a `signal` discriminator (explicit / implicit, +#: response-feedback spec §10) and a timestamp — never text, by the request #: model's closed enum (`apis.shared.sessions.models.FEEDBACK_REASONS`). FEEDBACK_ROW_PROJECTION: Tuple[str, ...] = ( "sessionId", "messageId", "value", "reason", + "signal", "updatedAt", ) diff --git a/backend/src/apis/shared/sessions/feedback.py b/backend/src/apis/shared/sessions/feedback.py index a1d935318..1400b1639 100644 --- a/backend/src/apis/shared/sessions/feedback.py +++ b/backend/src/apis/shared/sessions/feedback.py @@ -15,7 +15,12 @@ SK: F#{session_id}#{message_id} GSI_PK: SESSION#{session_id} (SessionLookupIndex) GSI_SK: F#{message_id} - sessionId, messageId, userId, value, reason?, updatedAt, ttl + sessionId, messageId, userId, value, reason?, signal, updatedAt, ttl + +``signal`` is ``"explicit"`` for a thumb. ``docs/specs/response-feedback.md`` +§10 adds implicit signals (copy, continue, edit-and-resend, abandonment) to +this same row family under ``signal: "implicit"``; every reader here filters +to explicit rows so that phase needs no backfill and the two are never summed. ``messageId`` is the same 0-based index the ``C#`` cost row carries for the assistant message, so an admin read joins feedback to the call's turn class @@ -30,7 +35,7 @@ the rollup always equals the count of live ``F#`` rows written while the diagnostics were on. -Everything is gated by :func:`apis.shared.feature_flags.message_feedback_enabled` +Everything is gated by :func:`apis.shared.feature_flags.response_feedback_enabled` at the route; this module is deliberately flag-free so a backfill or a test can drive it directly. """ @@ -148,6 +153,7 @@ async def put_message_feedback( "messageId": int(message_id), "userId": user_id, "value": int(value), + "signal": "explicit", "updatedAt": now, "ttl": ttl, } @@ -209,6 +215,8 @@ def query_session_feedback(table, session_id: str, user_id: Optional[str] = None for item in response.get("Items", []): if user_id is not None and item.get("userId") != user_id: continue + if not is_explicit(item): + continue raw_id = item.get("messageId") try: message_id = str(int(raw_id)) @@ -221,5 +229,11 @@ def query_session_feedback(table, session_id: str, user_id: Optional[str] = None return out +def is_explicit(row: Dict[str, Any]) -> bool: + """A thumb, as opposed to a §10 implicit signal. Rows written before the + discriminator existed carry none and are explicit by construction.""" + return row.get("signal") in (None, "explicit") + + def _now() -> str: return datetime.now(timezone.utc).isoformat() diff --git a/backend/src/apis/shared/sessions/metadata.py b/backend/src/apis/shared/sessions/metadata.py index c96705d87..4f018cd5b 100644 --- a/backend/src/apis/shared/sessions/metadata.py +++ b/backend/src/apis/shared/sessions/metadata.py @@ -2138,9 +2138,9 @@ async def _get_all_message_metadata_cloud(session_id: str, user_id: str, table_n # Merge this user's thumbs (F# rows) so a reload restores the SPA's # pressed state. Skipped while the feature is off — the rows stay. - from apis.shared.feature_flags import message_feedback_enabled + from apis.shared.feature_flags import response_feedback_enabled - if message_feedback_enabled(): + if response_feedback_enabled(): from .feedback import query_session_feedback try: diff --git a/backend/src/apis/shared/sessions/models.py b/backend/src/apis/shared/sessions/models.py index f19d5b3f8..ae3c3d9d5 100644 --- a/backend/src/apis/shared/sessions/models.py +++ b/backend/src/apis/shared/sessions/models.py @@ -642,11 +642,15 @@ class Citation(BaseModel): text: str = Field(..., description="Relevant text excerpt from the document") -#: Reason codes a down-thumb may carry. A closed enum, never free text — the -#: row is content-free by construction so it can sit beside the ``C#`` cost -#: row and be read by the admin profile without reading the conversation. -FEEDBACK_REASONS = ("wrong", "incomplete", "slow", "other") -FeedbackReason = Literal["wrong", "incomplete", "slow", "other"] +#: Reason codes a down-thumb may carry — the six buckets of +#: ``docs/specs/response-feedback.md`` §6, each of which routes to an +#: evaluator or an ops signal. A closed enum, never free text: the row is +#: content-free by construction so it can sit beside the ``C#`` cost row and +#: be read by the admin profile without reading the conversation. The spec's +#: "something else → free text" is deliberately not here; that hand-off is +#: the existing Agent report dialog (spec §3), which already has moderation. +FEEDBACK_REASONS = ("wrong", "instructions", "length", "tool_failed", "outdated", "other") +FeedbackReason = Literal["wrong", "instructions", "length", "tool_failed", "outdated", "other"] class MessageFeedback(BaseModel): diff --git a/backend/tests/apis/app_api/admin/costs/test_session_profile_service.py b/backend/tests/apis/app_api/admin/costs/test_session_profile_service.py index 295c6b735..530b81316 100644 --- a/backend/tests/apis/app_api/admin/costs/test_session_profile_service.py +++ b/backend/tests/apis/app_api/admin/costs/test_session_profile_service.py @@ -292,7 +292,7 @@ async def test_feedback_joins_the_turn_class_when_the_rows_carry_it(): _feedback(0, 1), _feedback(1, -1, "wrong"), _feedback(2, 1), - _feedback(3, -1, "slow"), + _feedback(3, -1, "tool_failed"), _feedback(9, -1), # no cost row for this message ] p = await _service_with_feedback(_row(), records, feedback).get_session_profile("s1") @@ -313,7 +313,7 @@ async def test_feedback_joins_the_turn_class_when_the_rows_carry_it(): @pytest.mark.asyncio async def test_feedback_counts_without_turn_class_when_rows_predate_1137(): records = [_call(0), _call(1)] # no hasDocuments / documentDigests / documentReads - feedback = [_feedback(0, 1), _feedback(1, -1, "incomplete")] + feedback = [_feedback(0, 1), _feedback(1, -1, "instructions")] p = await _service_with_feedback(_row(), records, feedback).get_session_profile("s1") assert (p.feedback.up, p.feedback.down) == (1, 1) assert p.feedback.by_turn_class is None, "turn class is 'not tracked', not 'none'" @@ -333,6 +333,16 @@ async def test_no_feedback_rows_falls_back_to_rollups_and_coverage_is_honest(): assert p.feedback.by_turn_class is None +@pytest.mark.asyncio +async def test_implicit_signal_rows_are_never_summed_into_the_thumb_counts(): + records = [_call(0)] + records[0]["hasDocuments"] = True + feedback = [_feedback(0, 1), {**_feedback(0, -1), "signal": "implicit"}, {**_feedback(0, -1), "signal": "explicit"}] + p = await _service_with_feedback(_row(), records, feedback).get_session_profile("s1") + assert (p.feedback.up, p.feedback.down) == (1, 1) + assert (p.feedback.by_turn_class.full.up, p.feedback.by_turn_class.full.down) == (1, 1) + + @pytest.mark.asyncio async def test_feedback_reader_failure_never_breaks_the_profile(): service = _service(_row(), [_call(0)]) diff --git a/backend/tests/apis/app_api/test_message_feedback_routes.py b/backend/tests/apis/app_api/test_message_feedback_routes.py index 574bb6a38..f2b7ed395 100644 --- a/backend/tests/apis/app_api/test_message_feedback_routes.py +++ b/backend/tests/apis/app_api/test_message_feedback_routes.py @@ -2,7 +2,7 @@ - PUT /sessions/{id}/messages/{messageId}/feedback → 200 with the stored thumb - DELETE /sessions/{id}/messages/{messageId}/feedback → 204 -- 404 for another user's session, 404 while MESSAGE_FEEDBACK_ENABLED=false, +- 404 for another user's session, 404 while RESPONSE_FEEDBACK_ENABLED=false, 422 for anything but ±1 / a fixed reason code, 503 with no table. Auth is the cookie-aware `get_current_user_from_session` (SPA-facing route); @@ -29,7 +29,7 @@ def _user() -> User: def _client(monkeypatch, put=None, delete=None) -> TestClient: - monkeypatch.delenv("MESSAGE_FEEDBACK_ENABLED", raising=False) + monkeypatch.delenv("RESPONSE_FEEDBACK_ENABLED", raising=False) monkeypatch.setattr(session_routes, "put_message_feedback", put or AsyncMock( return_value=MessageFeedback(value=1, updated_at="2026-09-16T00:00:00Z") )) @@ -41,19 +41,19 @@ def _client(monkeypatch, put=None, delete=None) -> TestClient: def test_put_stores_and_echoes_the_thumb(monkeypatch): - put = AsyncMock(return_value=MessageFeedback(value=-1, reason="incomplete", updated_at="2026-09-16T00:00:00Z")) + put = AsyncMock(return_value=MessageFeedback(value=-1, reason="instructions", updated_at="2026-09-16T00:00:00Z")) client = _client(monkeypatch, put=put) - resp = client.put("/sessions/s1/messages/3/feedback", json={"value": -1, "reason": "incomplete"}) + resp = client.put("/sessions/s1/messages/3/feedback", json={"value": -1, "reason": "instructions"}) assert resp.status_code == 200 - assert resp.json() == {"value": -1, "reason": "incomplete", "updatedAt": "2026-09-16T00:00:00Z"} - put.assert_awaited_once_with(session_id="s1", user_id="user-1", message_id=3, value=-1, reason="incomplete") + assert resp.json() == {"value": -1, "reason": "instructions", "updatedAt": "2026-09-16T00:00:00Z"} + put.assert_awaited_once_with(session_id="s1", user_id="user-1", message_id=3, value=-1, reason="instructions") def test_put_rejects_free_text_and_out_of_range_values(monkeypatch): client = _client(monkeypatch) - assert client.put("/sessions/s1/messages/3/feedback", json={"value": 1, "reason": "it lied"}).status_code == 422 + assert client.put("/sessions/s1/messages/3/feedback", json={"value": 1, "reason": "it lied to me"}).status_code == 422 assert client.put("/sessions/s1/messages/3/feedback", json={"value": 0}).status_code == 422 assert client.put("/sessions/s1/messages/3/feedback", json={"value": 5}).status_code == 422 assert client.put("/sessions/s1/messages/three/feedback", json={"value": 1}).status_code == 422 @@ -81,7 +81,7 @@ def test_another_users_session_is_404(monkeypatch): def test_kill_switch_hides_the_surface(monkeypatch): put = AsyncMock() client = _client(monkeypatch, put=put) - monkeypatch.setenv("MESSAGE_FEEDBACK_ENABLED", "false") + monkeypatch.setenv("RESPONSE_FEEDBACK_ENABLED", "false") assert client.put("/sessions/s1/messages/3/feedback", json={"value": 1}).status_code == 404 assert client.delete("/sessions/s1/messages/3/feedback").status_code == 404 put.assert_not_awaited() diff --git a/backend/tests/costs/test_content_free_projections.py b/backend/tests/costs/test_content_free_projections.py index 8eb1ce682..ae86d1389 100644 --- a/backend/tests/costs/test_content_free_projections.py +++ b/backend/tests/costs/test_content_free_projections.py @@ -186,7 +186,7 @@ async def test_feedback_rows_come_back_content_free_and_keyed_to_the_call(storag "PK": f"USER#{USER_ID}", "SK": f"F#{SESSION_ID}#3", "GSI_PK": f"SESSION#{SESSION_ID}", "GSI_SK": "F#3", "sessionId": SESSION_ID, "messageId": Decimal(3), "userId": USER_ID, - "value": Decimal(-1), "reason": "wrong", "updatedAt": "2026-09-16T00:00:00Z", + "value": Decimal(-1), "reason": "wrong", "signal": "explicit", "updatedAt": "2026-09-16T00:00:00Z", "ttl": Decimal(1_800_000_000), # A stray content-bearing attribute must never leave the reader even # if something wrote one (the writer cannot, but the reader is the guard). @@ -196,7 +196,7 @@ async def test_feedback_rows_come_back_content_free_and_keyed_to_the_call(storag assert len(rows) == 1 row = rows[0] assert content_bearing_paths(row) == [] - assert row == {"sessionId": SESSION_ID, "messageId": 3, "value": -1, "reason": "wrong", "updatedAt": "2026-09-16T00:00:00Z"} + assert row == {"sessionId": SESSION_ID, "messageId": 3, "value": -1, "reason": "wrong", "signal": "explicit", "updatedAt": "2026-09-16T00:00:00Z"} # Joins the C# row on messageId. records = await storage.get_session_cost_records(SESSION_ID) assert records[0]["messageId"] == row["messageId"] diff --git a/backend/tests/shared/test_message_feedback.py b/backend/tests/shared/test_message_feedback.py index d1926204e..c4ea26c51 100644 --- a/backend/tests/shared/test_message_feedback.py +++ b/backend/tests/shared/test_message_feedback.py @@ -72,7 +72,8 @@ async def test_put_writes_one_row_keyed_beside_the_cost_row(table): assert "reason" not in row assert "ttl" in row # Content-free: nothing on the row but ids, a number, a timestamp and keys. - assert set(row) <= {"PK", "SK", "GSI_PK", "GSI_SK", "sessionId", "messageId", "userId", "value", "updatedAt", "ttl"} + assert row["signal"] == "explicit" + assert set(row) <= {"PK", "SK", "GSI_PK", "GSI_SK", "sessionId", "messageId", "userId", "value", "signal", "updatedAt", "ttl"} @pytest.mark.asyncio @@ -80,12 +81,12 @@ async def test_second_thumb_replaces_the_first_and_rollups_follow(table): await fb.put_message_feedback(SESSION, OWNER, 3, 1) assert _session_row()["thumbsUp"] == 1 and _session_row()["thumbsDown"] == 0 - result = await fb.put_message_feedback(SESSION, OWNER, 3, -1, reason="incomplete") - assert result.value == -1 and result.reason == "incomplete" + result = await fb.put_message_feedback(SESSION, OWNER, 3, -1, reason="instructions") + assert result.value == -1 and result.reason == "instructions" items = _feedback_items() assert len(items) == 1, "a second click replaces, never appends" - assert int(items[0]["value"]) == -1 and items[0]["reason"] == "incomplete" + assert int(items[0]["value"]) == -1 and items[0]["reason"] == "instructions" row = _session_row() assert row["thumbsUp"] == 0 and row["thumbsDown"] == 1 @@ -98,7 +99,7 @@ async def test_second_thumb_replaces_the_first_and_rollups_follow(table): @pytest.mark.asyncio async def test_delete_removes_the_row_and_decrements(table): - await fb.put_message_feedback(SESSION, OWNER, 1, -1, reason="slow") + await fb.put_message_feedback(SESSION, OWNER, 1, -1, reason="tool_failed") await fb.put_message_feedback(SESSION, OWNER, 3, 1) assert await fb.delete_message_feedback(SESSION, OWNER, 1) is True assert [int(i["messageId"]) for i in _feedback_items()] == [3] @@ -156,6 +157,23 @@ async def test_query_filters_by_user_unless_admin(table): assert len(everyone) == 1 # keyed by message id; the admin reader returns rows, this map is per message +@pytest.mark.asyncio +async def test_implicit_signal_rows_share_the_family_but_never_read_as_thumbs(table): + """Spec §10: implicit signals land in the same F# family under + signal="implicit"; every thumb reader skips them and never sums the two.""" + await fb.put_message_feedback(SESSION, OWNER, 3, 1) + table.put_item(Item={ + "PK": f"USER#{OWNER}", "SK": f"F#{SESSION}#5", "GSI_PK": f"SESSION#{SESSION}", "GSI_SK": "F#5", + "sessionId": SESSION, "messageId": 5, "userId": OWNER, "value": 1, "signal": "implicit", + "updatedAt": "2026-09-16T00:00:00Z", + }) + assert set(fb.query_session_feedback(table, SESSION, OWNER)) == {"3"} + index = await md.get_all_message_metadata(SESSION, OWNER) + assert "5" not in index + # A row written before the discriminator existed is explicit. + assert fb.is_explicit({"value": 1}) and not fb.is_explicit({"value": 1, "signal": "implicit"}) + + @pytest.mark.asyncio async def test_messages_list_metadata_index_carries_feedback(table, monkeypatch): """The read path the SPA uses on reload: F# rows merge into the metadata @@ -174,7 +192,7 @@ async def test_messages_list_metadata_index_carries_feedback(table, monkeypatch) assert index["3"]["feedback"] == {"value": -1, "reason": "wrong", "updatedAt": index["3"]["feedback"]["updatedAt"]} assert index["5"] == {"feedback": {"value": 1, "updatedAt": index["5"]["feedback"]["updatedAt"]}} - monkeypatch.setenv("MESSAGE_FEEDBACK_ENABLED", "false") + monkeypatch.setenv("RESPONSE_FEEDBACK_ENABLED", "false") index = await md.get_all_message_metadata(SESSION, OWNER) assert "feedback" not in index["3"] and "5" not in index diff --git a/docs/specs/response-feedback.md b/docs/specs/response-feedback.md index 8768952fa..b8aa02693 100644 --- a/docs/specs/response-feedback.md +++ b/docs/specs/response-feedback.md @@ -1,7 +1,9 @@ # Response feedback -**Status:** PROPOSED — no code. Written 2026-09-04 from the "how would we -benefit?" conversation. +**Status:** PARTIALLY BUILT — capture and the read model shipped in PR #1142 +(2026-09-16, as document-context-offload PR-7); the consequence (§11 PR-1's +retry-with-correction), implicit signals and eval sampling are not built. +See §13. Written 2026-09-04 from the "how would we benefit?" conversation. **Refs:** `docs/specs/agentcore-evaluations-spike-findings.md` (the eval harness this feeds), `docs/specs/mid-turn-steering.md` (the injection path Phase 1 reuses), `docs/specs/agent-marketplace.md` D15 (the *other* feedback @@ -320,3 +322,39 @@ if PR-3 proves the signal is real. than copying whichever neighbour is read first. Agent Designer previews are exactly where an author would want to thumb their own work, but that data must never reach fleet aggregates. + +## 13. Built — PR #1142 (2026-09-16) + +Shipped as the outcome signal of `document-context-offload.md` §5 row 7, so +it was built against that spec's content-free rule first and reconciled with +this one after. What landed, and the decisions it took on this spec's open +points: + +- **Storage exactly per §5**: `F#{session_id}#{message_id}` on + `sessions-metadata`, `GSI_SK F#{message_id}`, idempotent upsert, overwrite + in place, never on the message object (`apis/shared/sessions/feedback.py`). + Rows carry `signal: "explicit"` from day one so §10's implicit rows can + join the family without a backfill; every thumb reader filters on it, so + the two are never summed (§10's rule, enforced). +- **Routes** `PUT`/`DELETE /sessions/{id}/messages/{message_id}/feedback` on + app-api (PUT rather than POST: the write is an upsert). Flag + `RESPONSE_FEEDBACK_ENABLED`, default on with a kill switch. +- **Reason set = §6's six buckets as codes** (`wrong`, `instructions`, + `length`, `tool_failed`, `outdated`, `other`). **Free text is not stored, + and this is a decision, not an omission**: the row sits beside the `C#` + cost row and is read by the content-free admin profile, whose test walks + the projection against the denylist. "Something else → free text" is + served by the §3 hand-off to the Agent report dialog, which already has + moderation and a scope. Revisit only together with §8 rule 1. +- **Read model**: the session profile (`GET /admin/costs/sessions/{id}/profile`) + joins thumbs to the call's document turn class with `n` per bucket + (§9's "every response carries its n"); `dataCoverage.feedback` says when + nothing is tracked. The other §7 attribution axes (model, compaction, + `agentSwitched`, skills) are further buckets in the same join loop. +- **Open question 4 settled**: preview sessions echo the thumb and persist + nothing, matching the `D#` write. Preview data never reaches aggregates. +- **Not built**: retry-with-correction (§11 PR-1's consequence — open + question 1 still stands), implicit signals (PR-2), eval sampling (PR-4), + author/marketplace surfaces (PR-5), the report-dialog escape hatch in the + reason row. + diff --git a/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.spec.ts b/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.spec.ts index 21bda944b..7c26e8938 100644 --- a/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.spec.ts +++ b/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.spec.ts @@ -208,9 +208,16 @@ describe('MessageActionsComponent — thumbs feedback', () => { const group = fixture.nativeElement.querySelector('[role="group"]'); expect(group).not.toBeNull(); const chips = Array.from(group.querySelectorAll('button')) as HTMLButtonElement[]; - expect(chips.map((c) => c.textContent!.trim())).toEqual(['Wrong', 'Incomplete', 'Slow', 'Other']); + expect(chips.map((c) => c.textContent!.trim())).toEqual([ + 'Wrong or made up', + 'Ignored instructions', + 'Too long / short', + 'A tool failed', + 'Out of date', + 'Something else', + ]); chips[1].click(); - expect(feedback.set).toEqual([{ id: 'msg-sess-1-3', value: -1, reason: 'incomplete' }]); + expect(feedback.set).toEqual([{ id: 'msg-sess-1-3', value: -1, reason: 'instructions' }]); }); it('reason codes stay hidden on a thumbs up', () => { diff --git a/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.ts b/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.ts index 09e2fb712..5199430ad 100644 --- a/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.ts +++ b/frontend/ai.client/src/app/session/components/message-list/components/message-actions.component.ts @@ -82,7 +82,7 @@ import { TooltipDirective } from '../../../../components/tooltip'; @if (feedbackValue() === -1) { -
+
@for (reason of reasons; track reason) {