From b89f63e511a0d8446aec25a02de7c4d7d2704bc4 Mon Sep 17 00:00:00 2001 From: roshan <38771624+rohoswagger@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:30:39 +0000 Subject: [PATCH 01/19] feat(usage): add PDF review pack to exports (#13872) --- .../reporting/usage_export_generation.py | 49 +- .../server/reporting/usage_report_branding.py | 79 +++ .../server/reporting/usage_report_data.py | 171 +++++ .../onyx/server/reporting/usage_report_pdf.py | 628 ++++++++++++++++++ backend/requirements/default.txt | 8 +- .../tests/reporting/test_usage_export_api.py | 7 + .../reporting/test_usage_report_branding.py | 88 +++ .../reporting/test_usage_report_data.py | 281 ++++++++ docs/usage/usage-reports.md | 273 ++++++++ pyproject.toml | 2 + uv.lock | 15 + 11 files changed, 1599 insertions(+), 2 deletions(-) create mode 100644 backend/ee/onyx/server/reporting/usage_report_branding.py create mode 100644 backend/ee/onyx/server/reporting/usage_report_data.py create mode 100644 backend/ee/onyx/server/reporting/usage_report_pdf.py create mode 100644 backend/tests/unit/ee/onyx/server/reporting/test_usage_report_branding.py create mode 100644 backend/tests/unit/ee/onyx/server/reporting/test_usage_report_data.py create mode 100644 docs/usage/usage-reports.md diff --git a/backend/ee/onyx/server/reporting/usage_export_generation.py b/backend/ee/onyx/server/reporting/usage_export_generation.py index c94c3ac6915..a874d9e9859 100644 --- a/backend/ee/onyx/server/reporting/usage_export_generation.py +++ b/backend/ee/onyx/server/reporting/usage_export_generation.py @@ -4,6 +4,7 @@ import zipfile from collections.abc import Iterable from datetime import datetime, timedelta, timezone +from io import BytesIO from fastapi_users_db_sqlalchemy import UUID_ID from sqlalchemy import cast @@ -19,6 +20,9 @@ UsageReportMetadata, UserSkeleton, ) +from ee.onyx.server.reporting.usage_report_branding import load_report_branding +from ee.onyx.server.reporting.usage_report_data import build_usage_report_data +from ee.onyx.server.reporting.usage_report_pdf import render_usage_report_pdf from onyx.configs.constants import FileOrigin from onyx.db.models import User from onyx.db.user_usage import UsageExportRow, iter_usage_export @@ -184,6 +188,31 @@ def generate_usage_breakdown_report( return file_id +def generate_usage_report_pdf( + db_session: Session, + file_store: FileStore, + report_id: str, + period: tuple[datetime, datetime] | None, + rows: list[UsageExportRow], +) -> str: + """Render the review pack PDF and store it. Returns the file id.""" + file_name = f"{report_id}_review_pack" + + # The queried bounds are half-open; only a given period gets the extra day. + display_start, display_end = period if period else _normalize_period(None) + + data = build_usage_report_data(db_session, rows, display_start, display_end) + branding = load_report_branding(file_store) + pdf_bytes = render_usage_report_pdf(data, branding) + + return file_store.save_file( + content=BytesIO(pdf_bytes), + display_name=file_name, + file_origin=FileOrigin.GENERATED_REPORT, + file_type="application/pdf", + ) + + def create_new_usage_report( db_session: Session, user_id: UUID_ID | None, # None = auto-generated @@ -204,12 +233,24 @@ def create_new_usage_report( intermediate_file_ids.append(users_file_id) query_start, query_end = normalized_period - usage_rows = iter_usage_export(db_session, query_start, query_end) + # The CSV and PDF must use the same rows so their totals reconcile. + usage_rows = list(iter_usage_export(db_session, query_start, query_end)) usage_breakdown_file_id = generate_usage_breakdown_report( file_store, report_id, usage_rows ) intermediate_file_ids.append(usage_breakdown_file_id) + # A render failure must not cost the admin their CSV export. + pdf_file_id: str | None = None + try: + pdf_file_id = generate_usage_report_pdf( + db_session, file_store, report_id, period, usage_rows + ) + except Exception: + logger.exception("Failed to render usage report PDF; continuing without it") + else: + intermediate_file_ids.append(pdf_file_id) + # Re-check just before writing the final report: the API-level check # happens before this (async) task runs, so a second request with the # same client-supplied report_id can slip past it while this task is @@ -239,6 +280,12 @@ def create_new_usage_report( ) zip_file.writestr("usage_by_user.csv", usage_breakdown_tmpfile.read()) + if pdf_file_id is not None: + pdf_tmpfile = file_store.read_file( + pdf_file_id, mode="b", use_tempfile=True + ) + zip_file.writestr("usage_report.pdf", pdf_tmpfile.read()) + zip_buffer.seek(0) # store zip blob to file_store diff --git a/backend/ee/onyx/server/reporting/usage_report_branding.py b/backend/ee/onyx/server/reporting/usage_report_branding.py new file mode 100644 index 00000000000..7d97157fa5d --- /dev/null +++ b/backend/ee/onyx/server/reporting/usage_report_branding.py @@ -0,0 +1,79 @@ +"""Branding for the usage report review pack, from enterprise settings.""" + +from pathlib import Path + +from pydantic import BaseModel + +from ee.onyx.server.enterprise_settings.store import ( + get_logo_filename, + get_logotype_filename, + load_runtime_settings, +) +from onyx.configs.constants import ONYX_DEFAULT_APPLICATION_NAME +from onyx.file_store.file_store import FileStore +from onyx.utils.logger import setup_logger + +logger = setup_logger() + +_FALLBACK_LOGO = Path(__file__).parents[4] / "static" / "images" / "logotype.png" + +_SUPPORTED_LOGO_TYPES = ("image/png", "image/jpeg", "image/jpg", "image/gif") + + +class ReportBranding(BaseModel): + application_name: str + # Raster bytes ReportLab can draw. None means the pack sets the name as a + # wordmark instead, which is right when a deployment has a custom logo we + # cannot draw: its own name beats another company's mark. + logo: bytes | None = None + + +def _read_logo(file_store: FileStore, file_id: str) -> bytes | None: + try: + stored = file_store.get_file_with_mime_type(file_id) + except Exception: + logger.exception("Failed to read logo %s for the usage report", file_id) + return None + + if stored is None: + return None + + # ReportLab cannot rasterize SVG, the common upload. + if stored.mime_type not in _SUPPORTED_LOGO_TYPES: + logger.info( + "Usage report cannot draw logo %s of type %s", file_id, stored.mime_type + ) + return None + + return stored.data + + +def load_report_branding(file_store: FileStore) -> ReportBranding: + settings = load_runtime_settings() + name = settings.application_name or ONYX_DEFAULT_APPLICATION_NAME + + has_custom_logo = settings.use_custom_logotype or settings.use_custom_logo + logo: bytes | None = None + if settings.use_custom_logotype: + logo = _read_logo(file_store, get_logotype_filename()) + if logo is None and settings.use_custom_logo: + logo = _read_logo(file_store, get_logo_filename()) + + if logo is None and has_custom_logo: + # Their logo exists but cannot be drawn, so fall through to the + # wordmark. Stamping the bundled mark here would ship our brand on + # their report. + logger.warning( + "Usage report rendering %s as a wordmark: the configured logo is " + "not a raster image ReportLab can draw", + name, + ) + return ReportBranding(application_name=name, logo=None) + + if logo is None: + try: + logo = _FALLBACK_LOGO.read_bytes() + except OSError: + logger.exception("Usage report could not read the fallback logo") + + return ReportBranding(application_name=name, logo=logo) diff --git a/backend/ee/onyx/server/reporting/usage_report_data.py b/backend/ee/onyx/server/reporting/usage_report_data.py new file mode 100644 index 00000000000..d7b9215c9b5 --- /dev/null +++ b/backend/ee/onyx/server/reporting/usage_report_data.py @@ -0,0 +1,171 @@ +"""Aggregates behind the usage report review pack.""" + +from collections import defaultdict +from datetime import datetime + +from pydantic import BaseModel +from sqlalchemy.orm import Session + +from ee.onyx.db.license import user_counts_toward_seats +from onyx.db.api_key import is_api_key_email_address +from onyx.db.user_usage import DELETED_USER_EXPORT_EMAIL, UsageExportRow +from onyx.db.users import get_all_users + +TOP_USER_LIMIT = 10 +TOP_ENTRY_LIMIT = 8 +DORMANT_USER_LIMIT = 25 +UNLABELED_FLOW = "other" + + +class NamedSpend(BaseModel): + name: str + cost_cents: float + input_tokens: int + output_tokens: int + + @property + def total_tokens(self) -> int: + return self.input_tokens + self.output_tokens + + +class DailySpend(BaseModel): + day: str # YYYY-MM-DD + cost_cents: float + active_users: int + + +class UsageReportData(BaseModel): + period_start: datetime + period_end: datetime + + total_cost_cents: float + total_input_tokens: int + total_output_tokens: int + total_cache_read_tokens: int + + licensed_users: int + # Everyone who used it, including people since deactivated, so this can + # exceed licensed_users. `seated_active_users` is the subset holding a seat. + active_users: int + seated_active_users: int + dormant_users: list[str] + + top_users: list[NamedSpend] + by_model: list[NamedSpend] + by_flow: list[NamedSpend] + daily: list[DailySpend] + + @property + def dormant_user_count(self) -> int: + return len(self.dormant_users) + + @property + def cost_per_active_user_cents(self) -> float: + if not self.active_users: + return 0.0 + return self.total_cost_cents / self.active_users + + @property + def has_usage(self) -> bool: + return bool(self.daily) + + +def _top_n(spend_by_name: dict[str, NamedSpend], limit: int) -> list[NamedSpend]: + ordered = sorted(spend_by_name.values(), key=lambda s: s.cost_cents, reverse=True) + if len(ordered) <= limit: + return ordered + + head, tail = ordered[:limit], ordered[limit:] + # Folding a single entry hides a name and saves no space. + if len(tail) == 1: + return ordered + + remainder = NamedSpend( + name=f"Other ({len(tail)})", + cost_cents=sum(s.cost_cents for s in tail), + input_tokens=sum(s.input_tokens for s in tail), + output_tokens=sum(s.output_tokens for s in tail), + ) + return head + [remainder] + + +def build_usage_report_data( + db_session: Session, + rows: list[UsageExportRow], + period_start: datetime, + period_end: datetime, +) -> UsageReportData: + """`rows` is the list written to usage_by_user.csv, so the two cannot + diverge. The period is the admin's requested bounds, for display.""" + by_user: dict[str, NamedSpend] = {} + by_model: dict[str, NamedSpend] = {} + by_flow: dict[str, NamedSpend] = {} + daily_cost: dict[str, float] = defaultdict(float) + daily_users: dict[str, set[str]] = defaultdict(set) + + total_cost = 0.0 + total_input = 0 + total_output = 0 + total_cache_read = 0 + active_emails: set[str] = set() + + for row in rows: + for bucket, key in ( + (by_user, row.email), + (by_model, row.model), + (by_flow, row.flow or UNLABELED_FLOW), + ): + entry = bucket.get(key) + if entry is None: + entry = NamedSpend( + name=key, cost_cents=0.0, input_tokens=0, output_tokens=0 + ) + bucket[key] = entry + entry.cost_cents += row.cost_cents + entry.input_tokens += row.input_tokens + entry.output_tokens += row.output_tokens + + total_cost += row.cost_cents + total_input += row.input_tokens + total_output += row.output_tokens + total_cache_read += row.cache_read_tokens + + daily_cost[row.day] += row.cost_cents + # Their spend still counts toward totals so the pack reconciles with the + # CSV, but neither is a person. + if row.email != DELETED_USER_EXPORT_EMAIL and not is_api_key_email_address( + row.email + ): + active_emails.add(row.email) + daily_users[row.day].add(row.email) + + # Must match license enforcement, or this disagrees with what is billed. + users = get_all_users(db_session, include_api_key_users=False) + seat_emails = {user.email for user in users if user_counts_toward_seats(user)} + dormant = sorted(seat_emails - active_emails) + + daily = [ + DailySpend( + day=day, + cost_cents=daily_cost[day], + active_users=len(daily_users[day]), + ) + for day in sorted(daily_cost) + ] + + return UsageReportData( + period_start=period_start, + period_end=period_end, + total_cost_cents=total_cost, + total_input_tokens=total_input, + total_output_tokens=total_output, + total_cache_read_tokens=total_cache_read, + licensed_users=len(seat_emails), + active_users=len(active_emails), + seated_active_users=len(active_emails & seat_emails), + dormant_users=dormant, + top_users=_top_n(by_user, TOP_USER_LIMIT), + by_model=_top_n(by_model, TOP_ENTRY_LIMIT), + by_flow=_top_n(by_flow, TOP_ENTRY_LIMIT), + daily=daily, + ) diff --git a/backend/ee/onyx/server/reporting/usage_report_pdf.py b/backend/ee/onyx/server/reporting/usage_report_pdf.py new file mode 100644 index 00000000000..414cf40d3d5 --- /dev/null +++ b/backend/ee/onyx/server/reporting/usage_report_pdf.py @@ -0,0 +1,628 @@ +"""Renders the usage report review pack as a PDF. + +The `ty: ignore`s below are load-bearing: ReportLab types `chart.data` from a +sample literal and populates `valueAxis.labels` dynamically, so neither is +resolvable by a static checker. +""" + +from io import BytesIO +from xml.sax.saxutils import escape + +from reportlab.graphics.charts.barcharts import VerticalBarChart +from reportlab.graphics.charts.linecharts import HorizontalLineChart +from reportlab.graphics.shapes import Drawing +from reportlab.lib import colors +from reportlab.lib.enums import TA_LEFT +from reportlab.lib.pagesizes import LETTER +from reportlab.lib.styles import ParagraphStyle, getSampleStyleSheet +from reportlab.lib.units import inch +from reportlab.lib.utils import ImageReader +from reportlab.pdfgen import canvas +from reportlab.platypus import ( + Flowable, + Image, + KeepTogether, + PageBreak, + Paragraph, + SimpleDocTemplate, + Spacer, + Table, + TableStyle, +) + +from ee.onyx.server.reporting.usage_report_branding import ReportBranding +from ee.onyx.server.reporting.usage_report_data import ( + DORMANT_USER_LIMIT, + NamedSpend, + UsageReportData, +) +from onyx.configs.constants import DANSWER_API_KEY_PREFIX, UNNAMED_KEY_PLACEHOLDER +from onyx.db.api_key import is_api_key_email_address +from onyx.utils.logger import setup_logger + +logger = setup_logger() + +_INK = colors.HexColor("#1c1c1c") # onyx-ink-95 +_ACCENT = colors.HexColor("#286df8") # action-selection-05 / blue-50 +_BODY = colors.HexColor("#54545d") # stone-60, 7.5:1 on white +_HAIRLINE = colors.HexColor("#e6e6e9") # stone-10 +_SURFACE = colors.HexColor("#f0f0f1") # stone-05 + +_CONTENT_WIDTH = LETTER[0] - 2 * inch +_MAX_AXIS_LABELS = 12 +_LOGO_MAX_W, _LOGO_MAX_H = 2.0 * inch, 0.5 * inch + + +def _dollars(cents: float) -> str: + return f"${cents / 100:,.2f}" + + +def _thousands(value: int) -> str: + return f"{value:,}" + + +def _display_name(name: str) -> str: + """Render an API key by its name instead of its synthetic address. + + Each API key owns a `User` row whose email is + `API_KEY__@onyxapikey.ai`, so per-key spend already + aggregates correctly. Only the label needs help. A no-op for every other + name, since none of them carry the API-key domain. + """ + if not is_api_key_email_address(name): + return name + + # The key's name can itself contain "@", so split on the last one. + local_part = name.rsplit("@", 1)[0] + # Stored emails are lowercased by a DB check constraint, so the prefix + # cannot be matched case-sensitively against the constant. + if local_part.lower().startswith(DANSWER_API_KEY_PREFIX.lower()): + local_part = local_part[len(DANSWER_API_KEY_PREFIX) :] + return f"{local_part or UNNAMED_KEY_PLACEHOLDER} (API key)" + + +def _styles() -> dict[str, ParagraphStyle]: + base = getSampleStyleSheet() + return { + "cover_title": ParagraphStyle( + "CoverTitle", + parent=base["Title"], + fontName="Helvetica-Bold", + fontSize=32, + leading=36, + textColor=_INK, + alignment=TA_LEFT, + spaceAfter=6, + ), + "wordmark": ParagraphStyle( + "Wordmark", + parent=base["Normal"], + fontName="Helvetica-Bold", + fontSize=19, + leading=23, + textColor=_INK, + ), + "cover_period": ParagraphStyle( + "CoverPeriod", + parent=base["Normal"], + fontName="Helvetica", + fontSize=13, + leading=18, + textColor=_BODY, + ), + "lede": ParagraphStyle( + "Lede", + parent=base["Normal"], + fontName="Helvetica", + fontSize=11.5, + leading=17, + textColor=_INK, + ), + "heading": ParagraphStyle( + "Heading", + parent=base["Heading2"], + fontName="Helvetica-Bold", + fontSize=14, + leading=18, + textColor=_INK, + spaceBefore=22, + spaceAfter=2, + keepWithNext=1, + ), + "subheading": ParagraphStyle( + "Subheading", + parent=base["Normal"], + fontName="Helvetica", + fontSize=9.5, + leading=13, + textColor=_BODY, + spaceAfter=10, + keepWithNext=1, + ), + "note": ParagraphStyle( + "Note", + parent=base["Normal"], + fontName="Helvetica", + fontSize=8.5, + leading=12, + textColor=_BODY, + spaceBefore=4, + ), + } + + +def _logo_flowable(branding: ReportBranding) -> Flowable | None: + if not branding.logo: + return None + try: + reader = ImageReader(BytesIO(branding.logo)) + src_w, src_h = reader.getSize() + except Exception: + logger.exception( + "Usage report could not render the configured logo for %s", + branding.application_name, + ) + return None + if not src_w or not src_h: + return None + + scale = min(_LOGO_MAX_W / src_w, _LOGO_MAX_H / src_h) + return Image( + BytesIO(branding.logo), width=src_w * scale, height=src_h * scale, mask="auto" + ) + + +class _NumberedCanvas(canvas.Canvas): + """Stamps "page N of M" once the total is known. + + The total only exists after the last page is laid out, so pages are held + back until save. The cover is deliberately left unnumbered. + """ + + def __init__(self, *args: object, **kwargs: object) -> None: + super().__init__(*args, **kwargs) + self._pages: list[dict[str, object]] = [] + + def showPage(self) -> None: + self._pages.append(dict(self.__dict__)) + self._startPage() + + def save(self) -> None: + total = len(self._pages) + for number, state in enumerate(self._pages, start=1): + self.__dict__.update(state) + if number > 1: + self._draw_folio(number, total) + super().showPage() + super().save() + + def _draw_folio(self, number: int, total: int) -> None: + width = self._pagesize[0] + self.setFont("Helvetica", 8.5) + self.setFillColor(_BODY) + self.drawRightString(width - inch, 0.6 * inch, f"{number} of {total}") + + +class _Rule(Flowable): + def __init__(self, width: float, color: colors.Color = _HAIRLINE) -> None: + super().__init__() + self.width, self.height, self.color = width, 1, color + + def draw(self) -> None: + self.canv.setStrokeColor(self.color) + self.canv.setLineWidth(1) + self.canv.line(0, 0, self.width, 0) + + +class _SeatMeter(Flowable): + """Seats in use against seats bought.""" + + def __init__(self, active: int, licensed: int, unseated: int, width: float) -> None: + super().__init__() + self.active, self.licensed, self.unseated = active, licensed, unseated + self.width, self.height = width, 54 + + def draw(self) -> None: + c = self.canv + bar_h, bar_y = 14, 20 + ratio = min(1.0, self.active / self.licensed) if self.licensed else 0.0 + + c.setFillColor(_SURFACE) + c.roundRect(0, bar_y, self.width, bar_h, 3, stroke=0, fill=1) + if ratio > 0: + c.setFillColor(_ACCENT) + c.roundRect( + 0, bar_y, max(3.0, self.width * ratio), bar_h, 3, stroke=0, fill=1 + ) + + c.setFillColor(_INK) + c.setFont("Helvetica-Bold", 11) + c.drawString( + 0, bar_y + bar_h + 8, f"{self.active} of {self.licensed} seats active" + ) + + c.setFillColor(_BODY) + c.setFont("Helvetica", 9) + idle = self.licensed - self.active + caption = ( + f"{ratio:.0%} in use · {idle} seats idle" + if idle + else f"{ratio:.0%} of licensed seats in use" + ) + if self.unseated: + caption += f" · {self.unseated} used it without a seat" + c.drawString(0, bar_y - 13, caption) + + +def _headline(data: UsageReportData) -> Table: + figures = [ + (_thousands(data.active_users), "People using it"), + (_dollars(data.total_cost_cents), "Total spend"), + (_dollars(data.cost_per_active_user_cents), "Cost per active person"), + ] + value_style = ParagraphStyle( + "Figure", + fontName="Helvetica-Bold", + fontSize=23, + leading=26, + textColor=_INK, + ) + label_style = ParagraphStyle( + "FigureLabel", + fontName="Helvetica", + fontSize=9, + leading=12, + textColor=_BODY, + ) + row = [ + [Paragraph(v, value_style) for v, _ in figures], + [Paragraph(label, label_style) for _, label in figures], + ] + col = _CONTENT_WIDTH / 3 + table = Table(row, colWidths=[col] * 3, hAlign="LEFT") + table.setStyle( + TableStyle( + [ + ("VALIGN", (0, 0), (-1, -1), "BOTTOM"), + ("TOPPADDING", (0, 0), (-1, 0), 0), + ("BOTTOMPADDING", (0, 0), (-1, 0), 2), + ("TOPPADDING", (0, 1), (-1, 1), 0), + ("LEFTPADDING", (0, 0), (-1, -1), 0), + ("RIGHTPADDING", (0, 0), (-1, -1), 12), + ] + ) + ) + return table + + +def _table(rows: list[list[str]], col_widths: list[float]) -> Table: + table = Table(rows, colWidths=col_widths, repeatRows=1, hAlign="LEFT") + table.setStyle( + TableStyle( + [ + # Header and body typography. + ("FONTNAME", (0, 0), (-1, 0), "Helvetica-Bold"), + ("FONTNAME", (0, 1), (-1, -1), "Helvetica"), + ("FONTSIZE", (0, 0), (-1, -1), 9), + ("TEXTCOLOR", (0, 0), (-1, 0), _INK), + ("TEXTCOLOR", (0, 1), (-1, -1), _BODY), + ("TEXTCOLOR", (0, 1), (0, -1), _INK), + # Text columns left-align; numeric columns right-align. + ("ALIGN", (1, 0), (-1, -1), "RIGHT"), + ("ALIGN", (0, 0), (0, -1), "LEFT"), + # Separate the header and each body row. + ("LINEBELOW", (0, 0), (-1, 0), 1, _INK), + ("LINEBELOW", (0, 1), (-1, -2), 0.5, _HAIRLINE), + # Keep rows readable without changing column width. + ("TOPPADDING", (0, 0), (-1, -1), 7), + ("BOTTOMPADDING", (0, 0), (-1, -1), 7), + ("LEFTPADDING", (0, 0), (-1, -1), 0), + ("VALIGN", (0, 0), (-1, -1), "MIDDLE"), + ] + ) + ) + return table + + +def _axis_labels(days: list[str]) -> list[str]: + """Keep at most `_MAX_AXIS_LABELS` ticks, blanking the rest.""" + step = max(1, (len(days) + _MAX_AXIS_LABELS - 1) // _MAX_AXIS_LABELS) + # Drop the year: the period is already stated on the cover. + return [day[5:] if index % step == 0 else "" for index, day in enumerate(days)] + + +def _style_axes(chart: HorizontalLineChart | VerticalBarChart) -> None: + chart.categoryAxis.labels.fontName = "Helvetica" + chart.categoryAxis.labels.fontSize = 7 + chart.categoryAxis.labels.fillColor = _BODY + chart.categoryAxis.strokeColor = _HAIRLINE + chart.valueAxis.labels.fontName = "Helvetica" # ty: ignore[unresolved-attribute] + chart.valueAxis.labels.fontSize = 7 # ty: ignore[unresolved-attribute] + chart.valueAxis.labels.fillColor = _BODY # ty: ignore[unresolved-attribute] + chart.valueAxis.strokeColor = _HAIRLINE + chart.valueAxis.valueMin = 0 + chart.valueAxis.gridStrokeColor = _HAIRLINE + chart.valueAxis.gridStrokeWidth = 0.5 + chart.valueAxis.visibleGrid = True + + +def _spend_over_time(data: UsageReportData) -> Drawing: + drawing = Drawing(_CONTENT_WIDTH, 168) + chart = HorizontalLineChart() + chart.x, chart.y = 42, 28 + chart.width, chart.height = int(_CONTENT_WIDTH - 56), 122 + chart.data = [[point.cost_cents / 100 for point in data.daily]] # ty: ignore[invalid-assignment] + chart.categoryAxis.categoryNames = _axis_labels([p.day for p in data.daily]) + _style_axes(chart) + chart.lines[0].strokeColor = _ACCENT + chart.lines[0].strokeWidth = 1.6 + drawing.add(chart) + return drawing + + +def _active_users_over_time(data: UsageReportData) -> Drawing: + drawing = Drawing(_CONTENT_WIDTH, 168) + chart = HorizontalLineChart() + chart.x, chart.y = 42, 28 + chart.width, chart.height = int(_CONTENT_WIDTH - 56), 122 + chart.data = [[float(point.active_users) for point in data.daily]] # ty: ignore[invalid-assignment] + chart.categoryAxis.categoryNames = _axis_labels([p.day for p in data.daily]) + _style_axes(chart) + chart.lines[0].strokeColor = _INK + chart.lines[0].strokeWidth = 1.6 + drawing.add(chart) + return drawing + + +def _spend_by_model(data: UsageReportData) -> Drawing: + drawing = Drawing(_CONTENT_WIDTH, 170) + chart = VerticalBarChart() + chart.x, chart.y = 42, 38 + chart.width, chart.height = int(_CONTENT_WIDTH - 56), 112 + chart.data = [[entry.cost_cents / 100 for entry in data.by_model]] # ty: ignore[invalid-assignment] + chart.categoryAxis.categoryNames = [ + _display_name(entry.name) for entry in data.by_model + ] + _style_axes(chart) + chart.categoryAxis.labels.angle = 20 + chart.categoryAxis.labels.dy = -8 + chart.bars[0].fillColor = _ACCENT + chart.bars[0].strokeColor = None + chart.barSpacing = 2 + drawing.add(chart) + return drawing + + +def _cover( + data: UsageReportData, + branding: ReportBranding, + styles: dict[str, ParagraphStyle], +) -> list[Flowable]: + period = ( + f"{data.period_start.date().isoformat()} to " + f"{data.period_end.date().isoformat()} (UTC)" + ) + story: list[Flowable] = [] + + logo = _logo_flowable(branding) + if logo is not None: + logo.hAlign = "LEFT" + story += [logo, Spacer(1, 30)] + else: + story += [ + Paragraph(escape(branding.application_name), styles["wordmark"]), + Spacer(1, 26), + ] + + story += [ + Paragraph("Usage report", styles["cover_title"]), + Paragraph(period, styles["cover_period"]), + Spacer(1, 26), + _Rule(_CONTENT_WIDTH, _INK), + Spacer(1, 22), + _headline(data), + Spacer(1, 30), + ] + + if data.licensed_users: + story += [ + _SeatMeter( + data.seated_active_users, + data.licensed_users, + data.active_users - data.seated_active_users, + _CONTENT_WIDTH, + ) + ] + + story += [ + Spacer(1, 30), + Paragraph(_summary_sentence(data, branding.application_name), styles["lede"]), + ] + + if data.by_model: + story += [ + Spacer(1, 26), + Paragraph("Top models by spend", styles["subheading"]), + _spend_table("Model", data.by_model[:3]), + ] + + return story + + +def _summary_sentence(data: UsageReportData, application_name: str) -> str: + """Returns Paragraph markup, so every interpolated name is escaped.""" + people = "1 person" if data.active_users == 1 else f"{data.active_users} people" + parts = [ + f"{people} used {escape(application_name)} in this period, at a total cost " + f"of {_dollars(data.total_cost_cents)}." + ] + if data.by_flow: + flow = escape(_display_name(data.by_flow[0].name)) + parts.append(f"Most of that ran through {flow}.") + if data.dormant_user_count: + share = ( + data.dormant_user_count / data.licensed_users + if data.licensed_users + else 0.0 + ) + parts.append( + f"{data.dormant_user_count} of {data.licensed_users} licensed seats " + f"({share:.0%}) went unused and are candidates to reassign." + ) + return " ".join(parts) + + +def _spend_table(label: str, entries: list[NamedSpend]) -> Table: + rows: list[list[str]] = [[label, "Spend (USD)", "Tokens"]] + rows += [ + [ + _display_name(entry.name), + _dollars(entry.cost_cents), + _thousands(entry.total_tokens), + ] + for entry in entries + ] + widths = [_CONTENT_WIDTH * 0.5, _CONTENT_WIDTH * 0.25, _CONTENT_WIDTH * 0.25] + return _table(rows, widths) + + +def _section( + heading: str, subheading: str, styles: dict[str, ParagraphStyle] +) -> list[Flowable]: + return [ + Paragraph(heading, styles["heading"]), + Paragraph(subheading, styles["subheading"]), + ] + + +def _charted_section( + heading: str, + subheading: str, + chart: Drawing, + styles: dict[str, ParagraphStyle], +) -> Flowable: + """`keepWithNext` does not reach into a KeepTogether, so the heading must + travel inside the group or it strands at the page foot.""" + return KeepTogether( + [ + Paragraph(heading, styles["heading"]), + Paragraph(subheading, styles["subheading"]), + chart, + ] + ) + + +def render_usage_report_pdf(data: UsageReportData, branding: ReportBranding) -> bytes: + styles = _styles() + buffer = BytesIO() + doc = SimpleDocTemplate( + buffer, + pagesize=LETTER, + leftMargin=inch, + rightMargin=inch, + topMargin=0.9 * inch, + bottomMargin=0.9 * inch, + title=f"{branding.application_name} usage report", + author=branding.application_name, + # Byte-identical output for identical input. + invariant=1, + ) + + story: list[Flowable] = [] + + if not data.has_usage: + logo = _logo_flowable(branding) + if logo is not None: + logo.hAlign = "LEFT" + story += [logo, Spacer(1, 30)] + else: + story += [ + Paragraph(escape(branding.application_name), styles["wordmark"]), + Spacer(1, 26), + ] + story += [ + Paragraph("Usage report", styles["cover_title"]), + Paragraph( + f"{data.period_start.date().isoformat()} to " + f"{data.period_end.date().isoformat()} (UTC)", + styles["cover_period"], + ), + Spacer(1, 24), + Paragraph( + "No recorded usage in this period. If the deployment was active, " + "the usage rollup may have started after the period began.", + styles["lede"], + ), + ] + doc.build(story, canvasmaker=_NumberedCanvas) + return buffer.getvalue() + + story += _cover(data, branding, styles) + story += [PageBreak()] + + story += [ + _charted_section( + "Adoption", + "Distinct people who sent at least one message each day.", + _active_users_over_time(data), + styles, + ), + _charted_section( + "Spend over time", + "Daily cost across every model and surface.", + _spend_over_time(data), + styles, + ), + _charted_section( + "Where the spend goes", + "Cost by model for the period.", + _spend_by_model(data), + styles, + ), + Spacer(1, 6), + _spend_table("Model", data.by_model), + ] + + story += _section("Heaviest users", "The people driving most of the cost.", styles) + story += [_spend_table("User", data.top_users)] + + story += _section("Spend by surface", "Where the work happens.", styles) + story += [_spend_table("Flow", data.by_flow)] + + story += _section( + "Seats not in use", + "Licensed people who sent nothing this period. Reclaim, retrain, or " + "drop them at renewal.", + styles, + ) + if not data.dormant_users: + story.append( + Paragraph("Every licensed seat was used this period.", styles["lede"]) + ) + else: + shown = data.dormant_users[:DORMANT_USER_LIMIT] + rows: list[list[str]] = [["User"]] + [[email] for email in shown] + story.append(_table(rows, [_CONTENT_WIDTH])) + remaining = data.dormant_user_count - len(shown) + if remaining > 0: + story.append( + Paragraph( + f"{remaining} more idle seats. See users.csv for the full list.", + styles["note"], + ) + ) + + story += [ + Spacer(1, 20), + _Rule(_CONTENT_WIDTH), + Spacer(1, 8), + Paragraph( + "Spend from deleted users and API keys is included in every total " + "and attributed separately. Neither counts as a person or a seat. " + "Days are UTC.", + styles["note"], + ), + ] + + doc.build(story, canvasmaker=_NumberedCanvas) + return buffer.getvalue() diff --git a/backend/requirements/default.txt b/backend/requirements/default.txt index 4c0bd13d0d4..b71eb75ba64 100644 --- a/backend/requirements/default.txt +++ b/backend/requirements/default.txt @@ -531,6 +531,7 @@ charset-normalizer==3.4.4 \ # htmldate # markitdown # pdfminer-six + # reportlab # requests # trafilatura # unstructured @@ -2021,7 +2022,9 @@ pillow==12.3.0 \ --hash=sha256:fa4ecea169a355be7a3ade2c783e2ed12f0e40d2c5621cda8b3297faf7fbb9f5 \ --hash=sha256:fdafc9cce40277e0f7a0feabce0ee50dd2fa1800f3b38015e51296b5e814048d \ --hash=sha256:fe3cca2e4e8a592be0f269a1ca4835c25199d9f3ce815c8491048f785b0a0198 - # via python-pptx + # via + # python-pptx + # reportlab platformdirs==4.5.0 \ --hash=sha256:70ddccdd7c99fc5942e9fc25636a8b34d04c24b335100223152c2803e4063312 \ --hash=sha256:e578a81bb873cbb89a41fcc904c7ef523cc18284b7e3b3ccf06aca1403b7ebd3 @@ -2672,6 +2675,9 @@ regex==2025.11.3 \ # dateparser # nltk # tiktoken +reportlab==5.0.0 \ + --hash=sha256:9d5a3affa84919e1111ede580031266a570e93b1ce388219621347965ff1d93c \ + --hash=sha256:e4494a0c6623ae213bb856fba523171b2b54a7bf629fda02d5e525a7b899a784 requests==2.33.0 \ --hash=sha256:3324635456fa185245e24865e810cecec7b4caf933d7eb133dcde67d48cee69b \ --hash=sha256:c7ebc5e8b0f21837386ad0e1c8fe8b829fa5f544d8df3b2253bff14ef29d7652 diff --git a/backend/tests/integration/tests/reporting/test_usage_export_api.py b/backend/tests/integration/tests/reporting/test_usage_export_api.py index e3420181cdf..87f36c38891 100644 --- a/backend/tests/integration/tests/reporting/test_usage_export_api.py +++ b/backend/tests/integration/tests/reporting/test_usage_export_api.py @@ -290,6 +290,13 @@ def test_read_usage_report( assert "chat_messages.csv" in file_names assert "users.csv" in file_names assert "usage_by_user.csv" in file_names + assert "usage_report.pdf" in file_names + + with zip_file.open("usage_report.pdf") as pdf_file: + pdf_bytes = pdf_file.read() + assert pdf_bytes.startswith(b"%PDF-") + assert len(pdf_bytes) > 1000 + # Verify usage_by_user.csv has the expected columns. The seeded # chat history doesn't record UserUsage rows, so there's no data # to assert on, just the header shape. diff --git a/backend/tests/unit/ee/onyx/server/reporting/test_usage_report_branding.py b/backend/tests/unit/ee/onyx/server/reporting/test_usage_report_branding.py new file mode 100644 index 00000000000..9010c38d61d --- /dev/null +++ b/backend/tests/unit/ee/onyx/server/reporting/test_usage_report_branding.py @@ -0,0 +1,88 @@ +"""Which logo the review pack renders under.""" + +from unittest.mock import MagicMock, patch + +from ee.onyx.server.enterprise_settings.models import EnterpriseSettings +from ee.onyx.server.reporting.usage_report_branding import ( + ReportBranding, + load_report_branding, +) +from onyx.utils.file import FileWithMimeType + +_LOGOTYPE = b"logotype-bytes" +_LOGO = b"logo-bytes" + + +def _file_store(stored: dict[str, FileWithMimeType]) -> MagicMock: + store = MagicMock() + store.get_file_with_mime_type.side_effect = lambda file_id: stored.get(file_id) + return store + + +def _load( + settings: EnterpriseSettings, stored: dict[str, FileWithMimeType] +) -> ReportBranding: + with patch( + "ee.onyx.server.reporting.usage_report_branding.load_runtime_settings", + return_value=settings, + ): + return load_report_branding(_file_store(stored)) + + +def test_logotype_wins_over_the_square_mark() -> None: + branding = _load( + EnterpriseSettings( + application_name="Acme", use_custom_logo=True, use_custom_logotype=True + ), + { + "__logotype__": FileWithMimeType(data=_LOGOTYPE, mime_type="image/png"), + "__logo__": FileWithMimeType(data=_LOGO, mime_type="image/png"), + }, + ) + + assert branding.logo == _LOGOTYPE + assert branding.application_name == "Acme" + + +def test_falls_back_to_the_square_mark_when_no_logotype() -> None: + branding = _load( + EnterpriseSettings(use_custom_logo=True, use_custom_logotype=True), + {"__logo__": FileWithMimeType(data=_LOGO, mime_type="image/png")}, + ) + + assert branding.logo == _LOGO + + +def test_an_undrawable_logo_becomes_a_wordmark_not_our_mark() -> None: + """ReportLab cannot draw SVG. Their own name beats stamping the Onyx mark + on a report they forward to their leadership.""" + branding = _load( + EnterpriseSettings(application_name="Acme", use_custom_logotype=True), + {"__logotype__": FileWithMimeType(data=b"", mime_type="image/svg+xml")}, + ) + + assert branding.logo is None + assert branding.application_name == "Acme" + + +def test_bundled_logo_is_used_when_nothing_is_uploaded() -> None: + branding = _load(EnterpriseSettings(), {}) + + assert branding.logo is not None + assert branding.logo.startswith(b"\x89PNG") + + +def test_a_file_store_failure_falls_back_to_the_wordmark() -> None: + """A configured custom logo we cannot read still must not become our mark.""" + store = MagicMock() + store.get_file_with_mime_type.side_effect = RuntimeError("object storage down") + + with patch( + "ee.onyx.server.reporting.usage_report_branding.load_runtime_settings", + return_value=EnterpriseSettings( + application_name="Acme", use_custom_logotype=True + ), + ): + branding = load_report_branding(store) + + assert branding.logo is None diff --git a/backend/tests/unit/ee/onyx/server/reporting/test_usage_report_data.py b/backend/tests/unit/ee/onyx/server/reporting/test_usage_report_data.py new file mode 100644 index 00000000000..331b497b1f2 --- /dev/null +++ b/backend/tests/unit/ee/onyx/server/reporting/test_usage_report_data.py @@ -0,0 +1,281 @@ +"""Aggregation behind the usage report review pack.""" + +from datetime import datetime, timezone +from io import BytesIO +from unittest.mock import MagicMock, patch + +import pytest +from PIL import Image as PILImage +from pypdf import PdfReader + +from ee.onyx.server.reporting.usage_report_branding import ReportBranding +from ee.onyx.server.reporting.usage_report_data import ( + TOP_USER_LIMIT, + UsageReportData, + build_usage_report_data, +) +from ee.onyx.server.reporting.usage_report_pdf import ( + _axis_labels, + _display_name, + render_usage_report_pdf, +) +from onyx.db.enums import AccountType +from onyx.db.models import User +from onyx.db.user_usage import DELETED_USER_EXPORT_EMAIL, UsageExportRow + +_BRANDING = ReportBranding(application_name="Acme Intelligence", logo=None) + +PERIOD_START = datetime(2026, 7, 1, tzinfo=timezone.utc) +PERIOD_END = datetime(2026, 7, 31, tzinfo=timezone.utc) + + +def _row( + email: str, + cost: float = 10.0, + day: str = "2026-07-01", + flow: str = "chat", +) -> UsageExportRow: + return UsageExportRow( + email=email, + model="gpt-5", + flow=flow, + provider="openai", + day=day, + input_tokens=100, + output_tokens=50, + cache_read_tokens=10, + cost_cents=cost, + ) + + +def _user( + email: str, + is_active: bool = True, + account_type: AccountType = AccountType.STANDARD, +) -> User: + user = User() + user.email = email + user.is_active = is_active + user.account_type = account_type + return user + + +def _build(rows: list[UsageExportRow], users: list[User]) -> UsageReportData: + with patch( + "ee.onyx.server.reporting.usage_report_data.get_all_users", return_value=users + ): + return build_usage_report_data( + db_session=MagicMock(), + rows=rows, + period_start=PERIOD_START, + period_end=PERIOD_END, + ) + + +def test_totals_reconcile_with_the_rows() -> None: + """The pack's totals must equal the CSV's, including deleted-user spend.""" + rows = [ + _row("a@x.com", 10.0), + _row("b@x.com", 5.5), + _row(DELETED_USER_EXPORT_EMAIL, 4.5), + ] + + data = _build(rows, [_user("a@x.com"), _user("b@x.com")]) + + assert data.total_cost_cents == pytest.approx(20.0) + assert sum(e.cost_cents for e in data.by_model) == pytest.approx(20.0) + assert sum(e.cost_cents for e in data.by_flow) == pytest.approx(20.0) + assert sum(e.cost_cents for e in data.top_users) == pytest.approx(20.0) + assert data.total_input_tokens == 300 + + +def test_deleted_user_counts_toward_spend_but_is_not_a_person() -> None: + rows = [_row("a@x.com"), _row(DELETED_USER_EXPORT_EMAIL)] + + data = _build(rows, [_user("a@x.com")]) + + assert data.active_users == 1 + assert data.daily[0].active_users == 1 + assert data.total_cost_cents == pytest.approx(20.0) + + +def test_api_key_usage_is_not_an_active_user() -> None: + """Seats exclude API-key users, so activity must exclude them too, or + active_users can exceed licensed_users.""" + rows = [_row("a@x.com"), _row("somekey@onyxapikey.ai")] + + data = _build(rows, [_user("a@x.com")]) + + assert data.active_users == 1 + assert data.active_users <= data.licensed_users + + +def test_unlabeled_flow_is_grouped_as_other() -> None: + data = _build([_row("a@x.com", flow="")], [_user("a@x.com")]) + + assert [(entry.name, entry.cost_cents) for entry in data.by_flow] == [ + ("other", 10.0) + ] + + +def test_service_accounts_do_not_hold_a_seat() -> None: + users = [ + _user("human@x.com"), + _user("bot@x.com", account_type=AccountType.SERVICE_ACCOUNT), + _user("gone@x.com", is_active=False), + ] + + data = _build([_row("human@x.com")], users) + + assert data.licensed_users == 1 + assert data.seated_active_users == 1 + assert data.dormant_users == [] + + +def test_a_deactivated_user_is_active_but_holds_no_seat() -> None: + """Someone who used it mid-period and was deactivated before the report ran + counts as a person, not as an occupied seat. The meter must not read + "2 of 1 seats active".""" + rows = [_row("stayed@x.com"), _row("departed@x.com")] + users = [_user("stayed@x.com"), _user("departed@x.com", is_active=False)] + + data = _build(rows, users) + + assert data.active_users == 2 + assert data.licensed_users == 1 + assert data.seated_active_users == 1 + assert data.seated_active_users <= data.licensed_users + + +def test_dormant_seats_are_named() -> None: + users = [_user("active@x.com"), _user("idle@x.com")] + + data = _build([_row("active@x.com")], users) + + assert data.licensed_users == 2 + assert data.active_users == 1 + assert data.dormant_users == ["idle@x.com"] + + +def test_top_users_folds_the_tail_and_preserves_the_total() -> None: + rows = [_row(f"u{i}@x.com", cost=float(i + 1)) for i in range(TOP_USER_LIMIT + 3)] + + data = _build(rows, []) + + assert len(data.top_users) == TOP_USER_LIMIT + 1 + assert data.top_users[-1].name == "Other (3)" + assert sum(e.cost_cents for e in data.top_users) == pytest.approx( + data.total_cost_cents + ) + + +def test_a_single_extra_user_is_named_rather_than_folded() -> None: + """Folding one entry hides a name and saves no space.""" + rows = [_row(f"u{i}@x.com", cost=float(i + 1)) for i in range(TOP_USER_LIMIT + 1)] + + data = _build(rows, []) + + assert not any(e.name.startswith("Other") for e in data.top_users) + + +def test_api_key_spend_is_labeled_by_key_name() -> None: + """Per-key spend stays in the breakdown; only the label is cleaned up.""" + # A DB check constraint lowercases stored emails, so this is the real shape. + assert ( + _display_name("api_key__nightly-sync@2f3c8a10-uuid.onyxapikey.ai") + == "nightly-sync (API key)" + ) + assert ( + _display_name("API_KEY__nightly-sync@2f3c8a10-uuid.onyxapikey.ai") + == "nightly-sync (API key)" + ) + assert ( + _display_name("api_key__sync@corp@2f3c8a10-uuid.onyxapikey.ai") + == "sync@corp (API key)" + ) + assert _display_name("human@corp.com") == "human@corp.com" + assert _display_name("gpt-5") == "gpt-5" + assert _display_name(DELETED_USER_EXPORT_EMAIL) == DELETED_USER_EXPORT_EMAIL + + +def test_api_key_spend_still_reconciles_after_relabeling() -> None: + rows = [_row("a@x.com", 10.0), _row("api_key__bot@uuid.onyxapikey.ai", 90.0)] + + data = _build(rows, [_user("a@x.com")]) + + assert data.total_cost_cents == pytest.approx(100.0) + assert sum(e.cost_cents for e in data.top_users) == pytest.approx(100.0) + assert data.active_users == 1 + + +def test_zero_usage_still_renders_a_pdf() -> None: + data = _build([], [_user("idle@x.com")]) + + assert not data.has_usage + assert data.cost_per_active_user_cents == 0.0 + assert render_usage_report_pdf(data, _BRANDING).startswith(b"%PDF-") + + +def test_application_name_is_not_parsed_as_markup() -> None: + """ReportLab Paragraph parses a markup subset: an unescaped name with a tag + silently drops text, injects formatting, or raises and loses the PDF.""" + data = _build([_row("a@x.com")], [_user("a@x.com")]) + + for name in ["Tools Us", "Acme Corp", 'X Y']: + branding = ReportBranding(application_name=name, logo=None) + pdf = render_usage_report_pdf(data, branding) + assert pdf.startswith(b"%PDF-") + + +def _png(width: int = 40, height: int = 10) -> bytes: + buffer = BytesIO() + PILImage.new("RGBA", (width, height), (0, 0, 0, 255)).save(buffer, format="PNG") + return buffer.getvalue() + + +def test_the_cover_is_unnumbered_and_later_pages_are_not() -> None: + """A folio on the cover, or an off-by-one, corrupts every generated pack.""" + rows = [_row(f"u{i}@x.com", cost=float(i + 1)) for i in range(12)] + data = _build(rows, [_user(f"u{i}@x.com") for i in range(12)]) + + reader = PdfReader(BytesIO(render_usage_report_pdf(data, _BRANDING))) + total = len(reader.pages) + + assert total > 1 + assert f"1 of {total}" not in reader.pages[0].extract_text() + for number in range(2, total + 1): + assert f"{number} of {total}" in reader.pages[number - 1].extract_text() + + +def test_render_is_deterministic() -> None: + data = _build([_row("a@x.com")], [_user("a@x.com")]) + + assert render_usage_report_pdf(data, _BRANDING) == render_usage_report_pdf( + data, _BRANDING + ) + + +def test_render_is_deterministic_with_an_embedded_logo() -> None: + """The default path embeds a logo, so determinism must hold with one.""" + data = _build([_row("a@x.com")], [_user("a@x.com")]) + branding = ReportBranding(application_name="Acme", logo=_png()) + + assert render_usage_report_pdf(data, branding) == render_usage_report_pdf( + data, branding + ) + + +def test_unreadable_logo_still_produces_a_pdf() -> None: + """A corrupt upload must cost the branding, not the report.""" + data = _build([_row("a@x.com")], [_user("a@x.com")]) + + for logo in (b"not-an-image", b""): + branding = ReportBranding(application_name="Acme", logo=logo) + assert render_usage_report_pdf(data, branding).startswith(b"%PDF-") + + +def test_axis_labels_never_exceed_the_display_limit() -> None: + for day_count in (0, 1, 12, 13, 23, 24, 25, 30, 365): + days = [f"2026-07-{day + 1:02d}" for day in range(day_count)] + + assert sum(bool(label) for label in _axis_labels(days)) <= 12 diff --git a/docs/usage/usage-reports.md b/docs/usage/usage-reports.md new file mode 100644 index 00000000000..553ecfbe475 --- /dev/null +++ b/docs/usage/usage-reports.md @@ -0,0 +1,273 @@ +# Usage reports: what they are for + +This document defines what an Onyx usage report must tell an organization admin, +and why. It starts from the admin's job, not from the data we happen to store. + +The rule that governs every decision here: **a number belongs in the report only +if the admin can finish the sentence "so I will...".** If no action follows, cut +the number. + +## Where we are today + +`create_new_usage_report` builds a zip with three CSVs and a PDF review pack: + +| File | Contents | +| -------------------- | ------------------------------------------------------------------------------------- | +| `chat_messages.csv` | One row per message: session, user, flow, time, agent, email, tokens, model | +| `users.csv` | `user_id`, `is_active` | +| `usage_by_user.csv` | Per user, per day, per model/flow/provider: tokens, cache reads, cost | +| `usage_report.pdf` | Summary of spend, adoption, seats, and usage attribution | + +The raw CSV files are still a data dump. They have three problems: + +1. **It answers no question.** The admin must build every pivot. +2. **It has no dimensions the admin budgets by.** No team, no agent, no source. +3. **It joins badly.** `users.csv` has no email, so it cannot join to the other files. + +The raw export is still valuable. It is just the wrong artifact for every job. + +## The admin's job + +An org admin is accountable for three things: + +1. **Money.** They signed the contract. They own the spend. +2. **Adoption.** They championed the rollout. Someone will ask if it worked. +3. **Risk.** If the tool leaks or misbehaves, it lands on them. + +Every useful metric comes from one of these. The sections below derive the +metrics from the decisions. + +### Decision: how many seats do I buy at renewal? + +The admin needs a seat ledger, not a message count. + +- Licensed seats, provisioned seats, and active seats. +- A named list of users who hold a seat and did not use it in 30 days. +- The inverse list: users who hit rate limits. + +The named lists are the deliverable. The admin acts on them directly. They +reclaim a seat, retrain the person, or drop the seat at renewal. + +### Decision: am I overspending, and what can I cut? + +Total cost drives no action. Cost **concentration** does. + +- Share of spend from the top 5 users, the top 3 agents, and the top model. +- Cost per active user per month. This is the one number finance accepts. +- Spend sent to an expensive model for work a cheap model handles. +- Money already saved by prompt caching. We store `cache_read_tokens` today. + +### Decision: who pays for it? + +Cost split by team or user group. Most orgs must charge the cost back, or at +least explain the invoice internally. An admin who cannot attribute cost to a +cost center cannot grow the deployment. This blocks expansion. + +`User__UserGroup` already gives us the join. + +### Decision: did the rollout work? + +Message volume is a trap. It rises when three people go heavy. + +Measure breadth first, then habit, then depth: + +- Distinct humans who used Onyx in the period. +- How many use it every week, and whether that count rises. +- Multi-turn sessions versus one-question-and-leave. +- The funnel: invited, first message, five messages, weekly habit. + +The drop-off point in the funnel tells the admin what to fix. A drop before the +first message means onboarding. A drop after it means answer quality. + +### Decision: is the quality good? + +The admin cannot read conversations. Give them proxies, and give them trends. +Nobody knows what a good absolute thumbs-down rate is. + +- Negative feedback rate. +- Regeneration rate. +- Sessions abandoned after one answer. +- Answers where retrieval returned nothing. + +### Decision: what do I do next to improve it? + +This section is the most actionable, and it does not exist today. + +- Connectors that are indexed but never cited. +- Topics that people ask often and Onyx answers badly. +- Agents that nobody uses. + +Each item maps to one concrete action: add a source, write a document, delete +an agent. + +### Decision: am I exposed? + +Admins do not want a security console here. They want a short "look at this" +list, with names on it. + +- A user whose usage jumped 10x. +- Access to sensitive sources. +- Traffic from a service account or API key, not a human. +- Activity from an employee who left and kept an account. + +## The flagship: the knowledge gap report + +Every vendor can report spend and logins. Only Onyx knows **what the +organization tries to learn and fails to find.** + +A monthly artifact that lists the top unanswered questions, clustered by topic, +with the missing sources named, is worth more than the full cost breakdown. It +tells the admin something about their own company that they cannot get anywhere +else. It also makes the strongest renewal argument that exists. + +Treat this as the headline of the report, not an extra tab. + +## Three artifacts, not one zip + +The current report tries to be one thing. The admin needs three, at three +cadences. This is the main structural change. + +| Artifact | Cadence | Form | Purpose | +| ----------------------- | --------- | --------------------------------- | -------------------------------- | +| **Pulse** | Monthly | Pushed to email or Slack, no download | Tell the admin if anything changed | +| **Review pack** | Quarterly | A PDF the admin forwards to their boss | Defend the spend and the rollout | +| **Investigation export**| On demand | The raw CSVs we build today | Answer a specific question, feed BI | + +The pulse must be pushed. An artifact that needs a download and a spreadsheet +gets read once. + +## The one screen + +If the pulse were a single screen, it holds eight items: + +1. Active users, and the change. +2. Spend, and the change. +3. Cost per active user. +4. Percent of seats dormant, with the list. +5. Top 3 cost concentrations. +6. Quality trend, one line. +7. Top 5 unanswered topics. +8. One anomaly callout. + +Everything else lives one click deeper. + +## The PDF review pack + +The review pack must be a PDF. An admin forwards it to a VP or a CFO. Those +people do not open a zip of CSVs, and they do not log in to an admin panel. The +PDF is the artifact that travels. + +### Build it in pure Python with ReportLab + +Use **ReportLab** (BSD licensed). Write the document in Python. Do not template +markdown, do not render HTML, and do not drive a browser. + +ReportLab supplies everything the pack needs: + +- `platypus` flows content into pages. It splits long tables across pages and + repeats the header row. +- `graphics.charts` draws bar, line, and pie charts as native PDF vectors. +- The PDF base-14 fonts (Helvetica, Times) need no embedding. A brand font + works too, but vendor the TTF in the repo. Never fetch a font at render time. + +Measured on a representative pack (40-row table across 2 pages, one line chart, +one bar chart): **13 ms, 4.4 KB**. No subprocess, no browser. + +### The shared layer is the data model, not the text + +Build one typed aggregate object, and give each output its own renderer: + +| Output | Renderer | +| ------------ | --------------------------------- | +| PDF | ReportLab document builder | +| Email digest | HTML with inlined styles | +| Slack digest | Slack blocks | +| CSV rollups | The existing writers | + +All four read the same aggregate object, so the numbers always agree. Sharing a +data model is stronger than sharing a markdown string. Markdown cannot express a +chart, a page break, or a repeated table header, so it would have leaked layout +concerns into the shared layer anyway. + +### Pipeline + +1. Query the aggregates into the typed object. Same queries as the CSV rollups. +2. Build the ReportLab document from that object. +3. Save the PDF to the file store next to the zip, under the same `report_id`. + +This runs in the existing Celery report task. + +### Alternatives considered + +- **Chromium through Playwright.** It works, and it needs no new dependency, + because the image already installs Chromium (`backend/Dockerfile:124`) and + already launches it for the web connector. It also renders correctly with no + network (verified below). It still loses: a browser launch costs about 470 ms + and a few hundred MB of RSS, versus 13 ms for ReportLab, and it drags a + browser process, a template layer, and hand-written SVG into the report path. +- **WeasyPrint.** Adds a Python dependency plus pango system libraries, and + still makes us write CSS to control pagination. +- **Markdown plus Jinja2.** A lossy intermediate format. It cannot express + charts or pagination, so every hard part would still need solving elsewhere. + +### Air-gap status + +Verified inside the shipped image with `docker run --network none`: + +- Chromium launches and prints a PDF with tables, inline SVG, and real fonts. + Text extracts correctly. So the browser path is air-gap safe if we ever want + it. +- ReportLab needs no network by construction, and the base-14 fonts live inside + the PDF spec. + +Note that the air-gap CI job only starts `api_server`, `inference_model_server`, +and `minio`. It does not exercise the background worker or a browser. Any +air-gap claim for a new render path needs its own check. + +### Constraints to respect + +- Determinism. The same period must produce the same PDF. Do not stamp a + render timestamp inside the content body. +- Size. Cap the page count. The pack is a summary, not the export. +- Anonymized mode. The PDF must honor it, the same as the CSVs. +- Failure isolation. A PDF failure must not fail the zip. Generate them + independently. + +## Anti-metrics + +Do not put these in a report. They look informative and drive no decision: + +- Total messages, total tokens, total sessions. +- Average response time. +- Any cumulative all-time count. + +## What the raw export still needs + +The investigation export stays. Fix its defects: + +- Add `summary.csv`, `manifest.json` (schema version, period, timezone, row + counts, generator version), and a `README.md` that defines every column. +- Add pre-built rollups: by team, by agent, by model, by day. +- Fix `users.csv`: email, role, groups, created date, last active, seat state. +- State the units. Name the currency. Snapshot the price table, so the numbers + stay reproducible after prices change. +- Warn when the period is incomplete. If the usage rollup started after the + period start, say so on the report. Otherwise a partial month reads as a + full one. +- Support an anonymized mode. Some EU customers cannot legally receive per + person usage. Hash the email with a per-report salt. + +## Build order + +Ordered by value per unit of work. The first two are independent of each other. + +1. **Seat ledger.** Fix `users.csv` and derive the dormant-seat list. Small + change, and it makes every other file joinable. +2. **Knowledge gap report.** The differentiated artifact. +3. **Team and agent dimensions** on the cost breakdown. Unblocks chargeback. +4. **Summary, manifest, and README** on the export. +5. **Extend the PDF review pack.** The shipped pack summarizes spend and + adoption by person, model, and flow. Add the missing dimensions from steps + 1 to 3 as they become available. +6. **Scheduled pulse** to email or Slack. The Celery beat already exists, and + the existing typed aggregate object supplies the content. diff --git a/pyproject.toml b/pyproject.toml index 40d8265304a..ee65af339ef 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -99,6 +99,8 @@ backend = [ "pywikibot==11.4.2", "readerwriterlock==1.0.9", "redis==5.0.8", + # PDF generation for the usage report review pack. + "reportlab==5.0.0", "requests==2.33.0", "requests-oauthlib==2.0.0", "simple-salesforce==1.12.6", diff --git a/uv.lock b/uv.lock index d27dfd39bd0..fda8c3d5acd 100644 --- a/uv.lock +++ b/uv.lock @@ -4350,6 +4350,7 @@ backend = [ { name = "rapidfuzz" }, { name = "readerwriterlock" }, { name = "redis" }, + { name = "reportlab" }, { name = "requests" }, { name = "requests-oauthlib" }, { name = "sendgrid" }, @@ -4524,6 +4525,7 @@ backend = [ { name = "rapidfuzz", specifier = "==3.14.5" }, { name = "readerwriterlock", specifier = "==1.0.9" }, { name = "redis", specifier = "==5.0.8" }, + { name = "reportlab", specifier = "==5.0.0" }, { name = "requests", specifier = "==2.33.0" }, { name = "requests-oauthlib", specifier = "==2.0.0" }, { name = "sendgrid", specifier = "==6.12.5" }, @@ -6271,6 +6273,19 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/dd/6f/832c2023a8bd8414c93452bd8b43bf61cedfa5b9575f70c06fb911e51a29/release_tag-0.5.2-py3-none-win_arm64.whl", hash = "sha256:5f26b008e0be0c7a122acd8fcb1bb5c822f38e77fed0c0bf6c550cc226c6bf14", size = 1203191, upload-time = "2026-03-11T00:27:29.789Z" }, ] +[[package]] +name = "reportlab" +version = "5.0.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "charset-normalizer" }, + { name = "pillow" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/41/d6/4b7b0cf56880eb96533e607967be6a939e344675601e033d113a0bfa1f4e/reportlab-5.0.0.tar.gz", hash = "sha256:e4494a0c6623ae213bb856fba523171b2b54a7bf629fda02d5e525a7b899a784", size = 3701928, upload-time = "2026-06-18T11:34:31.145Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/a3/07/70085c17a369605f15e301d10ab902115019b1126c7253d964afc230c7d6/reportlab-5.0.0-py3-none-any.whl", hash = "sha256:9d5a3affa84919e1111ede580031266a570e93b1ce388219621347965ff1d93c", size = 1956710, upload-time = "2026-06-18T11:34:29.07Z" }, +] + [[package]] name = "requests" version = "2.33.0" From 72b2ec0184d4fa60f52c3ffb042bcba878877503 Mon Sep 17 00:00:00 2001 From: Nikolas Garza <90273783+nmgarza5@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:36:26 +0000 Subject: [PATCH 02/19] feat(chat): incognito chat sessions with content-free records (#13866) --- .../background/celery/tasks/beat_schedule.py | 12 + .../tasks/user_file_processing/tasks.py | 50 ++++ backend/onyx/chat/chat_state.py | 27 +- backend/onyx/chat/chat_utils.py | 19 +- backend/onyx/chat/incognito.py | 75 +++++ backend/onyx/chat/incognito_context.py | 201 +++++++++++++ backend/onyx/chat/llm_loop.py | 78 +++-- backend/onyx/chat/llm_step.py | 5 +- backend/onyx/chat/models.py | 6 + backend/onyx/chat/process_message.py | 181 +++++++++--- backend/onyx/chat/save_chat.py | 19 +- backend/onyx/chat/stream_buffer.py | 38 +++ backend/onyx/configs/constants.py | 6 + backend/onyx/db/chat.py | 25 ++ backend/onyx/db/chat_search.py | 8 +- backend/onyx/db/file_record.py | 42 ++- backend/onyx/error_handling/error_codes.py | 2 + backend/onyx/file_store/constants.py | 4 + .../onyx/server/features/projects/models.py | 4 +- .../server/query_and_chat/chat_backend.py | 87 +++++- backend/onyx/server/query_and_chat/models.py | 6 + backend/onyx/tools/models.py | 2 + .../memory/memory_tool.py | 4 +- .../tracing/braintrust_tracing_processor.py | 16 ++ backend/onyx/tracing/incognito.py | 9 + .../tracing/langfuse_tracing_processor.py | 14 + backend/shared_configs/contextvars.py | 18 ++ .../chat/test_incognito_persistence.py | 266 ++++++++++++++++++ .../db/test_incognito_history_exclusion.py | 178 ++++++++++++ .../redis/test_incognito_context.py | 230 +++++++++++++++ .../onyx/chat/test_incognito_record_mode.py | 47 ++++ .../onyx/chat/test_multi_model_streaming.py | 5 +- .../onyx/connectors/braintrust/__init__.py | 0 .../test_braintrust_incognito_suppression.py | 80 ++++++ .../test_langfuse_incognito_suppression.py | 60 ++++ 35 files changed, 1738 insertions(+), 86 deletions(-) create mode 100644 backend/onyx/chat/incognito.py create mode 100644 backend/onyx/chat/incognito_context.py create mode 100644 backend/onyx/tracing/incognito.py create mode 100644 backend/tests/external_dependency_unit/chat/test_incognito_persistence.py create mode 100644 backend/tests/external_dependency_unit/db/test_incognito_history_exclusion.py create mode 100644 backend/tests/external_dependency_unit/redis/test_incognito_context.py create mode 100644 backend/tests/unit/onyx/chat/test_incognito_record_mode.py delete mode 100644 backend/tests/unit/onyx/connectors/braintrust/__init__.py create mode 100644 backend/tests/unit/onyx/tracing/test_braintrust_incognito_suppression.py create mode 100644 backend/tests/unit/onyx/tracing/test_langfuse_incognito_suppression.py diff --git a/backend/onyx/background/celery/tasks/beat_schedule.py b/backend/onyx/background/celery/tasks/beat_schedule.py index e31e9f960fd..b54980bf65a 100644 --- a/backend/onyx/background/celery/tasks/beat_schedule.py +++ b/backend/onyx/background/celery/tasks/beat_schedule.py @@ -49,6 +49,18 @@ "expires": BEAT_EXPIRES_DEFAULT, }, }, + { + "name": "check-for-incognito-file-cleanup", + "task": OnyxCeleryTask.CHECK_FOR_INCOGNITO_FILE_CLEANUP, + "schedule": timedelta(minutes=10), + "options": { + "priority": OnyxCeleryPriority.LOW, + "expires": BEAT_EXPIRES_DEFAULT, + # Run on gated tenants too, their registries hold blob handles. + "skip_gated": False, + "work_gated": True, + }, + }, { "name": "check-for-user-file-project-sync", "task": OnyxCeleryTask.CHECK_FOR_USER_FILE_PROJECT_SYNC, diff --git a/backend/onyx/background/celery/tasks/user_file_processing/tasks.py b/backend/onyx/background/celery/tasks/user_file_processing/tasks.py index 71c7746cc6b..7402f413f55 100644 --- a/backend/onyx/background/celery/tasks/user_file_processing/tasks.py +++ b/backend/onyx/background/celery/tasks/user_file_processing/tasks.py @@ -16,6 +16,10 @@ ) from onyx.background.celery.celery_utils import httpx_init_vespa_pool from onyx.background.celery.tasks.shared.RetryDocumentIndex import RetryDocumentIndex +from onyx.cache.factory import get_cache_backend +from onyx.chat.chat_processing_checker import is_chat_session_processing +from onyx.chat.incognito import delete_incognito_generated_files +from onyx.chat.incognito_context import incognito_session_ended from onyx.configs.app_configs import ( DISABLE_VECTOR_DB, MANAGED_VESPA, @@ -29,6 +33,7 @@ CELERY_USER_FILE_PROCESSING_TASK_EXPIRES, CELERY_USER_FILE_PROJECT_SYNC_LOCK_TIMEOUT, CELERY_USER_FILE_PROJECT_SYNC_TASK_EXPIRES, + INCOGNITO_FILE_CLEANUP_BATCH, USER_FILE_DELETE_MAX_QUEUE_DEPTH, USER_FILE_PROCESSING_MAX_QUEUE_DEPTH, USER_FILE_PROJECT_SYNC_MAX_QUEUE_DEPTH, @@ -42,6 +47,7 @@ from onyx.connectors.models import Document, HierarchyNode from onyx.db.engine.sql_engine import get_session_with_current_tenant from onyx.db.enums import UserFileStatus +from onyx.db.file_record import get_session_ids_with_incognito_files from onyx.db.models import SearchSettings, UserFile from onyx.db.port_attempt import port_backfill_has_pending_work from onyx.db.port_orphan_candidate import record_port_orphan_candidates_for_user_file @@ -1196,3 +1202,47 @@ def process_single_user_file_project_sync( project_sync_user_file_impl( user_file_id=user_file_id, tenant_id=tenant_id, redis_locking=True ) + + +@shared_task( # ty: ignore[invalid-argument-type] + name=OnyxCeleryTask.CHECK_FOR_INCOGNITO_FILE_CLEANUP, + soft_time_limit=300, + bind=True, + ignore_result=True, +) +def check_for_incognito_file_cleanup(self: Task, *, tenant_id: str) -> None: # noqa: ARG001 + """Retry deletion of tool-generated blobs whose teardown pass failed. + + A blob's own record carries the session that produced it, and deleting the + blob deletes the record, so anything still stamped is what a store failure + left behind.""" + redis_client = get_redis_client(tenant_id=tenant_id) + lock: RedisLock = redis_client.lock( + OnyxRedisLocks.INCOGNITO_FILE_CLEANUP_BEAT_LOCK, + timeout=CELERY_GENERIC_BEAT_LOCK_TIMEOUT, + ) + if not lock.acquire(blocking=False): + return + try: + with get_session_with_current_tenant() as db_session: + cache = get_cache_backend() + for raw_id in get_session_ids_with_incognito_files( + db_session, limit=INCOGNITO_FILE_CLEANUP_BATCH + ): + session_id = UUID(raw_id) + # A turn in flight owns its files, and an evicted context reads + # as ended, so the processing fence decides, not the context. + if is_chat_session_processing(session_id, cache): + continue + if not incognito_session_ended(session_id): + continue + try: + delete_incognito_generated_files(session_id, db_session) + except Exception: + # One unreachable blob must not strand every session behind it. + task_logger.exception( + "Incognito file cleanup failed for session %s", session_id + ) + finally: + if lock.owned(): + lock.release() diff --git a/backend/onyx/chat/chat_state.py b/backend/onyx/chat/chat_state.py index 771a1c265c8..61f963b9f3f 100644 --- a/backend/onyx/chat/chat_state.py +++ b/backend/onyx/chat/chat_state.py @@ -15,8 +15,9 @@ SearchParams, ) from onyx.context.search.models import SearchDoc +from onyx.db.enums import IncognitoRecordMode from onyx.db.memory import UserMemoryContext -from onyx.db.models import ChatMessage, ChatSession, Persona +from onyx.db.models import ChatMessage, Persona from onyx.llm.interfaces import LLM, LLMUserIdentity from onyx.llm.models import ReasoningEffort from onyx.onyxbot.slack.models import SlackContext @@ -183,18 +184,24 @@ class ChatTurnSetup: """Immutable context produced by ``build_chat_turn`` and consumed by ``_run_models``. **Detached-safety contract:** instances of this class travel outside the DB - session that built them. Every ORM object reachable from this dataclass - (``chat_session``, ``persona``, ``user_message``, ``reserved_messages``, - ``llms``) is detached after ``build_chat_turn`` returns. Downstream code - must only read column attributes that were eager-loaded during setup — - do NOT access lazy-loaded relationships (e.g. ``setup.chat_session.messages``, - ``setup.persona.tools[i].some_lazy_field``) or SQLAlchemy will raise - ``DetachedInstanceError`` at runtime.""" + session that built them. The ORM objects still reachable from this dataclass + (``persona``, ``reserved_messages``) are detached after ``build_chat_turn`` + returns. Downstream code must only read column attributes that were + eager-loaded during setup. Do NOT access lazy-loaded relationships + (e.g. ``setup.persona.tools[i].some_lazy_field``) or SQLAlchemy will raise + ``DetachedInstanceError`` at runtime. Closures stored here count: bind the + ids they need, never the rows. + + Session and user-message identity are carried as plain scalars: the turn + needs only their ids and the session's project id.""" new_msg_req: SendMessageRequest - chat_session: ChatSession + chat_session_id: UUID + chat_session_project_id: int | None + # The session's pinned recording policy. None is an ordinary chat. + incognito_record_mode: IncognitoRecordMode | None persona: Persona - user_message: ChatMessage + user_message_id: int user_identity: LLMUserIdentity llms: list[LLM] # length 1 for single-model, N for multi-model model_display_names: list[str] # parallel to llms diff --git a/backend/onyx/chat/chat_utils.py b/backend/onyx/chat/chat_utils.py index e5e086c3c79..a11fb383aae 100644 --- a/backend/onyx/chat/chat_utils.py +++ b/backend/onyx/chat/chat_utils.py @@ -8,6 +8,8 @@ from pydantic import BaseModel from sqlalchemy.orm import Session +from onyx.chat.incognito import resolve_incognito_record_mode +from onyx.chat.incognito_context import incognito_context_available from onyx.chat.models import ( ChatHistoryResult, ChatLoadedFile, @@ -28,7 +30,7 @@ get_chat_messages_by_session, get_or_create_root_message, ) -from onyx.db.enums import UserFileStatus +from onyx.db.enums import IncognitoRecordMode, UserFileStatus from onyx.db.file_record import FileRecordNotFoundError from onyx.db.kg_config import ( get_kg_config_settings, @@ -39,6 +41,8 @@ from onyx.db.persona import user_can_access_persona from onyx.db.projects import check_project_ownership from onyx.db.user_file import get_user_file_by_id +from onyx.error_handling.error_codes import OnyxErrorCode +from onyx.error_handling.exceptions import OnyxError from onyx.file_processing.extract_file_text import extract_file_text from onyx.file_store.file_store import get_default_file_store from onyx.file_store.models import ChatFileType, FileDescriptor @@ -185,12 +189,25 @@ def create_chat_session_from_request( ): raise ValueError("User does not have access to persona") + # Pinned at creation. A deployment without the Redis cache backend cannot + # hold incognito context, so the request is refused rather than downgraded + # into a chat the user believes is incognito. + incognito_mode: IncognitoRecordMode | None = None + if chat_session_request.incognito: + if not incognito_context_available(): + raise OnyxError( + OnyxErrorCode.DEPLOYMENT_UNSUPPORTED, + "Incognito chat is not supported on this deployment.", + ) + incognito_mode = resolve_incognito_record_mode() + return create_chat_session( db_session=db_session, description=chat_session_request.description or "", user_id=user.id, persona_id=chat_session_request.persona_id, project_id=chat_session_request.project_id, + incognito_record_mode=incognito_mode, ) diff --git a/backend/onyx/chat/incognito.py b/backend/onyx/chat/incognito.py new file mode 100644 index 00000000000..551b43f0c41 --- /dev/null +++ b/backend/onyx/chat/incognito.py @@ -0,0 +1,75 @@ +"""Recording policy for incognito chat turns. + +An incognito chat is an ordinary chat carrying a mode. ``IncognitoRecordMode`` +is the only policy object: behavior is exposed as derived properties on it, so +the legal states are the only representable ones and no caller can assemble an +illegal combination out of loose booleans. + +The contract that enforcement points must honor: an incognito session must pin +its mode on a metadata-only ``chat_session`` row at creation, and downstream +code must read the pinned value, never the live admin setting, so a setting +change cannot alter a session under way. Only FULL_HISTORY may write +conversation content into ``chat_message`` rows. USAGE_ONLY writes +content-free rows and must carry the live conversation outside Postgres +for the length of the session. +""" + +from uuid import UUID + +from sqlalchemy.orm import Session + +from onyx.db.enums import IncognitoRecordMode, record_mode_persists_content +from onyx.db.file_record import get_incognito_file_ids +from onyx.file_store.file_store import get_default_file_store +from onyx.file_store.models import FileDescriptor +from onyx.utils.logger import setup_logger +from shared_configs.contextvars import get_current_incognito_record_mode + +logger = setup_logger() + + +def current_turn_persists_content() -> bool: + mode = IncognitoRecordMode.from_context_value(get_current_incognito_record_mode()) + return record_mode_persists_content(mode) + + +def resolve_incognito_record_mode() -> IncognitoRecordMode: + """The mode a new incognito session must pin. + + The seam a workspace's record-mode setting must resolve through. Today it + returns the default unconditionally. + """ + return IncognitoRecordMode.USAGE_ONLY + + +def content_free_file_descriptors( + file_descriptors: list[FileDescriptor], +) -> list[FileDescriptor]: + """Descriptors safe to persist for a content-free turn. Linkage ids and + type survive for the file-reader tool and teardown. The content-derived + filename does not.""" + return [ + FileDescriptor( + id=fd["id"], type=fd["type"], user_file_id=fd.get("user_file_id") + ) + for fd in file_descriptors + ] + + +def delete_incognito_generated_files( + chat_session_id: UUID, db_session: Session +) -> bool: + """Delete the blobs the session's tools saved. True when none remain. + + The file record carries the session stamp, so deleting the blob deletes the + handle with it. A blob the store refuses keeps both, which is what the + cleanup sweep retries from.""" + file_store = get_default_file_store() + outstanding = False + for file_id in get_incognito_file_ids(str(chat_session_id), db_session): + try: + file_store.delete_file(file_id, error_on_missing=False) + except Exception: + logger.warning("Failed to delete incognito generated file %s", file_id) + outstanding = True + return not outstanding diff --git a/backend/onyx/chat/incognito_context.py b/backend/onyx/chat/incognito_context.py new file mode 100644 index 00000000000..8c40d3cb738 --- /dev/null +++ b/backend/onyx/chat/incognito_context.py @@ -0,0 +1,201 @@ +"""Ephemeral conversation context for incognito chat turns. + +USAGE_ONLY must carry the live conversation outside Postgres, so it lives in +Redis: one value per session, a sliding TTL that starts over on every save, +and explicit teardown when the chat closes. Keys are tenant-prefixed by the +Redis client. Redis may evict or expire the value mid-session: an expired +value loads as empty and the turn continues without earlier context. + +Concurrent turns on one session are possible (the chat processing fence is a +status marker, not admission control), so save is a compare-and-set on a +version. A lost save means a concurrent writer won or the session ended, and +the caller must not retry with the history it loaded. +""" + +from uuid import UUID + +from pydantic import BaseModel, TypeAdapter, ValidationError + +from onyx.cache.interface import CacheBackendType +from onyx.chat.models import ChatMessageSimple +from onyx.chat.stream_buffer import stream_buffer_key_pattern +from onyx.configs import app_configs +from onyx.redis.redis_pool import get_redis_client +from onyx.utils.logger import setup_logger + +logger = setup_logger() + +# Sliding: restarted on every save, so context survives while the page stays +# active and dies within the hour once it goes idle or closes uncleanly. +INCOGNITO_CONTEXT_TTL_SECONDS = 3600 +# Long enough that an in-flight turn cannot resurrect a torn-down context. +_TOMBSTONE_TTL_SECONDS = INCOGNITO_CONTEXT_TTL_SECONDS +# Raw-storage caps. Token budgeting trims context further at prompt build. +# These only bound what one session may hold in Redis. +_MAX_CONTEXT_MESSAGES = 200 +_MAX_CONTEXT_BYTES = 1_000_000 +# 15 digits stay exact in a Lua double, and turn counts never approach it. +_MAX_VERSION_DIGITS = 15 + +_KEY_PREFIX = "incognito_ctx" + +_MESSAGES_ADAPTER: TypeAdapter[list[ChatMessageSimple]] = TypeAdapter( + list[ChatMessageSimple] +) + +# Stored value grammar: ``:``. Lua and Python agree +# only on the digits-before-colon prefix, mirrored by _parse_version_prefix. +# Non-matching values read as version 0. Applies when stored version == ARGV[1]. +_TOMBSTONE = b"tombstone" +_CAS_SCRIPT = """ +local cur = redis.call('GET', KEYS[1]) +if cur == 'tombstone' then + return 0 +end +local cur_version = 0 +if cur then + local v = string.match(cur, '^(%d+):') + if v ~= nil and #v <= 15 then + cur_version = tonumber(v) + end +end +if cur_version ~= tonumber(ARGV[1]) then + return 0 +end +redis.call('SET', KEYS[1], ARGV[2], 'EX', tonumber(ARGV[3])) +return 1 +""" + + +class IncognitoContext(BaseModel): + """A session's history plus the version that makes save a compare-and-set.""" + + version: int + messages: list[ChatMessageSimple] + + +def incognito_context_available() -> bool: + """Whether this deployment can hold incognito context at all. + + USAGE_ONLY content must never reach Postgres, so the Postgres cache + backend (Lite) means the feature is absent rather than degraded. + """ + return app_configs.CACHE_BACKEND == CacheBackendType.REDIS + + +def _context_key(chat_session_id: UUID) -> str: + return f"{_KEY_PREFIX}:{chat_session_id}" + + +def _parse_version_prefix(raw: bytes) -> tuple[int, bytes | None]: + """The value's version and JSON body, or (0, None) for a tombstone or a + value this store did not write. + + Byte-for-byte the same rule as the CAS script: ASCII digits, at most + ``_MAX_VERSION_DIGITS`` of them, immediately followed by a colon. + """ + prefix, sep, body = raw.partition(b":") + if sep and prefix.isdigit() and len(prefix) <= _MAX_VERSION_DIGITS: + return int(prefix), body + return 0, None + + +def load_incognito_context(chat_session_id: UUID) -> IncognitoContext: + """The session's context, messages oldest first. + + Empty messages mean nothing was written, the session was torn down, the + value expired, or its body failed to parse. The turn proceeds with whatever + loads: missing context is degraded recall, never an error. + """ + raw = get_redis_client().get(_context_key(chat_session_id)) + if raw is None: + return IncognitoContext(version=0, messages=[]) + + version, body = _parse_version_prefix(raw) + if body is None: + logger.warning( + "Dropping unreadable incognito context for session %s", chat_session_id + ) + return IncognitoContext(version=0, messages=[]) + try: + messages = _MESSAGES_ADAPTER.validate_json(body) + except ValidationError: + # Corrupt context must end the session cleanly, not fail the turn. + # Keeping the prefix version lets the next save overwrite the value. + logger.warning( + "Dropping unparseable incognito context for session %s", chat_session_id + ) + return IncognitoContext(version=version, messages=[]) + return IncognitoContext(version=version, messages=messages) + + +def save_incognito_context(chat_session_id: UUID, context: IncognitoContext) -> bool: + """Write the full history, bump the version, restart the idle clock. + + Applies only while the stored version still equals ``context.version`` + and the session has not been torn down. False means the write was + discarded. + + Images are stripped: file bytes do not round-trip JSON, and incognito + attachments only live within their own turn. Oldest messages fall off + past the count and byte caps. + """ + trimmed = [ + message.model_copy(update={"image_files": None, "image_token_count": 0}) + for message in context.messages[-_MAX_CONTEXT_MESSAGES:] + ] + body = _MESSAGES_ADAPTER.dump_json(trimmed) + while len(body) > _MAX_CONTEXT_BYTES and len(trimmed) > 1: + trimmed = trimmed[1:] + body = _MESSAGES_ADAPTER.dump_json(trimmed) + payload = f"{context.version + 1}:".encode() + body + + client = get_redis_client() + result = client.eval( + _CAS_SCRIPT, + keys=[_context_key(chat_session_id)], + args=[ + str(context.version).encode(), + payload, + str(INCOGNITO_CONTEXT_TTL_SECONDS).encode(), + ], + ) + return bool(result) + + +def append_incognito_message(chat_session_id: UUID, message: ChatMessageSimple) -> None: + """Append one message to the session's live context, tolerating failure. + + A lost compare-and-set (a concurrent writer or an ended session) or a Redis + blip must degrade the stored context, never fail the turn. + Worst case the next turn is missing this message, which the load contract + treats as ordinary missing context rather than an error. + """ + try: + context = load_incognito_context(chat_session_id) + context.messages.append(message) + if not save_incognito_context(chat_session_id, context): + logger.warning( + "Incognito context save lost the CAS for session %s", chat_session_id + ) + except Exception: + logger.exception( + "Failed to persist incognito context for session %s", chat_session_id + ) + + +def incognito_session_ended(chat_session_id: UUID) -> bool: + """Whether the live context is gone, by teardown or by expiry.""" + raw = get_redis_client().get(_context_key(chat_session_id)) + return raw is None or raw == _TOMBSTONE + + +def teardown_incognito_session(chat_session_id: UUID) -> None: + """End the session now: tombstone the context so an in-flight turn cannot + recreate it (a missing key reads as version zero), and delete the buffered + stream chunks holding the streamed answer NDJSON.""" + client = get_redis_client() + client.set(_context_key(chat_session_id), _TOMBSTONE, ex=_TOMBSTONE_TTL_SECONDS) + buffered = list(client.scan_iter(match=stream_buffer_key_pattern(chat_session_id))) + if buffered: + client.delete(*buffered) diff --git a/backend/onyx/chat/llm_loop.py b/backend/onyx/chat/llm_loop.py index c9006f99ade..d1435e0e19d 100644 --- a/backend/onyx/chat/llm_loop.py +++ b/backend/onyx/chat/llm_loop.py @@ -67,6 +67,7 @@ from onyx.tools.models import ( ChatFile, CustomToolCallSummary, + CustomToolUserFileSnapshot, MemoryToolResponseSnapshot, PythonToolRichResponse, ToolCallInfo, @@ -84,6 +85,7 @@ from onyx.tools.utils import compute_all_tool_tokens from onyx.tracing.framework.create import ChatTraceMetadata, trace from onyx.utils.logger import setup_logger +from shared_configs.contextvars import get_current_incognito_record_mode logger = setup_logger() @@ -1220,35 +1222,60 @@ def run_llm_loop( tool_response.rich_response.generated_files or None ) + # Custom tools save image/CSV blobs and return their ids. + generated_file_ids = None + if isinstance( + tool_response.rich_response, CustomToolCallSummary + ) and isinstance( + tool_response.rich_response.tool_result, CustomToolUserFileSnapshot + ): + generated_file_ids = ( + tool_response.rich_response.tool_result.file_ids or None + ) + # Persist memory if this is a memory tool response memory_snapshot: MemoryToolResponseSnapshot | None = None + incognito_memory_refusal: str | None = None if isinstance(tool_response.rich_response, MemoryToolResponse): - persisted_memory_id: int | None = None - if user_memory_context and user_memory_context.user_id: - if tool_response.rich_response.index_to_replace is not None: - persisted_memory_id = update_memory_at_index( - user_id=user_memory_context.user_id, - index=tool_response.rich_response.index_to_replace, - new_text=tool_response.rich_response.memory_text, - ) - else: - persisted_memory_id = add_memory( - user_id=user_memory_context.user_id, - memory_text=tool_response.rich_response.memory_text, - ) - operation: Literal["add", "update"] = ( - "update" - if tool_response.rich_response.index_to_replace is not None - else "add" - ) - memory_snapshot = MemoryToolResponseSnapshot( - memory_text=tool_response.rich_response.memory_text, - operation=operation, - memory_id=persisted_memory_id, - index=tool_response.rich_response.index_to_replace, - ) + # Any incognito mode refuses memory writes with an explicit + # error, so neither the model nor the user sees a saved + # memory that does not exist. + if get_current_incognito_record_mode() is not None: + incognito_memory_refusal = ( + "Error: memories cannot be saved from an incognito " + "chat. Tell the user their request was not saved." + ) + else: + persisted_memory_id: int | None = None + if user_memory_context and user_memory_context.user_id: + if tool_response.rich_response.index_to_replace is not None: + persisted_memory_id = update_memory_at_index( + user_id=user_memory_context.user_id, + index=tool_response.rich_response.index_to_replace, + new_text=tool_response.rich_response.memory_text, + ) + else: + persisted_memory_id = add_memory( + user_id=user_memory_context.user_id, + memory_text=tool_response.rich_response.memory_text, + ) + operation: Literal["add", "update"] = ( + "update" + if tool_response.rich_response.index_to_replace is not None + else "add" + ) + memory_snapshot = MemoryToolResponseSnapshot( + memory_text=tool_response.rich_response.memory_text, + operation=operation, + memory_id=persisted_memory_id, + index=tool_response.rich_response.index_to_replace, + ) - if memory_snapshot: + if incognito_memory_refusal: + saved_response = incognito_memory_refusal + # The next LLM cycle must see the refusal too. + tool_response.llm_facing_response = incognito_memory_refusal + elif memory_snapshot: saved_response = json.dumps(memory_snapshot.model_dump()) elif isinstance(tool_response.rich_response, CustomToolCallSummary): saved_response = json.dumps( @@ -1272,6 +1299,7 @@ def run_llm_loop( search_docs=displayed_docs or search_docs, generated_images=generated_images, generated_files=generated_files, + generated_file_ids=generated_file_ids, ) # Add to state container for partial save support state_container.add_tool_call(tool_call_info) diff --git a/backend/onyx/chat/llm_step.py b/backend/onyx/chat/llm_step.py index d414b530c0d..26cf6713762 100644 --- a/backend/onyx/chat/llm_step.py +++ b/backend/onyx/chat/llm_step.py @@ -9,6 +9,7 @@ from onyx.chat.chat_state import ChatStateContainer from onyx.chat.citation_processor import DynamicCitationProcessor from onyx.chat.emitter import Emitter +from onyx.chat.incognito import current_turn_persists_content from onyx.chat.models import ChatMessageSimple, LlmStepResult from onyx.chat.tool_call_args_streaming import maybe_emit_argument_delta from onyx.configs.app_configs import ( @@ -1155,7 +1156,7 @@ def _current_placement() -> Placement: llm_msg_history = translate_history_to_llm_format(history, llm.config) has_reasoned = False - if LOG_ONYX_MODEL_INTERACTIONS: + if LOG_ONYX_MODEL_INTERACTIONS and current_turn_persists_content(): logger.debug( "Message history:\n%s", _format_message_history_for_logging(llm_msg_history), @@ -1521,7 +1522,7 @@ def _emit_content_chunk(content_chunk: str) -> Generator[Packet, None, None]: # Note: Content (AgentResponseDelta) doesn't need an explicit end packet - OverallStop handles it # Tool calls are handled by tool execution code and emit their own packets (e.g., SectionEnd) - if LOG_ONYX_MODEL_INTERACTIONS: + if LOG_ONYX_MODEL_INTERACTIONS and current_turn_persists_content(): logger.debug("Accumulated reasoning: %s", accumulated_reasoning) logger.debug("Accumulated answer: %s", accumulated_answer) diff --git a/backend/onyx/chat/models.py b/backend/onyx/chat/models.py index 43ee8dc7bd7..57a34265488 100644 --- a/backend/onyx/chat/models.py +++ b/backend/onyx/chat/models.py @@ -37,6 +37,9 @@ class CustomToolResponse(BaseModel): class CreateChatSessionID(BaseModel): chat_session_id: UUID + # Echoes the pinned mode so the client can verify the server honored an + # incognito request. A server that omits it did not. + incognito: bool = False AnswerStreamPart = ( @@ -93,6 +96,9 @@ class ChatFullResponse(BaseModel): # Metadata message_id: int chat_session_id: UUID | None = None + # Echoes the pinned mode for newly-created sessions, like the streaming + # packet does. A server that omits it did not honor an incognito request. + incognito: bool = False error_msg: str | None = None diff --git a/backend/onyx/chat/process_message.py b/backend/onyx/chat/process_message.py index 07d781153fa..47479668223 100644 --- a/backend/onyx/chat/process_message.py +++ b/backend/onyx/chat/process_message.py @@ -39,6 +39,14 @@ get_compression_params, ) from onyx.chat.emitter import Emitter +from onyx.chat.incognito import ( + content_free_file_descriptors, +) +from onyx.chat.incognito_context import ( + append_incognito_message, + incognito_session_ended, + load_incognito_context, +) from onyx.chat.llm_loop import EmptyLLMResponseError, run_llm_loop from onyx.chat.models import ( AnswerStream, @@ -78,9 +86,9 @@ ) from onyx.db.document_set import filter_document_set_names_by_user_access from onyx.db.engine.sql_engine import get_session_with_current_tenant -from onyx.db.enums import HookPoint +from onyx.db.enums import HookPoint, record_mode_persists_content from onyx.db.memory import get_memories -from onyx.db.models import ChatMessage, Persona, User, UserFile +from onyx.db.models import ChatMessage, ChatSession, Persona, User, UserFile from onyx.db.projects import get_user_files_from_project from onyx.db.tools import get_tools from onyx.deep_research.dr_loop import run_deep_research_llm_loop @@ -142,7 +150,11 @@ from onyx.utils.logger import setup_logger from onyx.utils.telemetry import mt_cloud_telemetry from onyx.utils.timing import log_function_time -from shared_configs.contextvars import get_current_tenant_id +from shared_configs.contextvars import ( + CURRENT_CONTENT_FREE_SESSION_ID_CONTEXTVAR, + CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR, + get_current_tenant_id, +) logger = setup_logger() ERROR_TYPE_CANCELLED = "cancelled" @@ -626,7 +638,10 @@ def build_chat_turn( user=user, db_session=db_session, ) - yield CreateChatSessionID(chat_session_id=chat_session.id) + yield CreateChatSessionID( + chat_session_id=chat_session.id, + incognito=chat_session.incognito_record_mode is not None, + ) chat_session = get_chat_session_by_id( chat_session_id=chat_session.id, user_id=user_id, @@ -741,11 +756,11 @@ def build_chat_turn( if parent_message.message_type == MessageType.USER: user_message = parent_message else: - # New message — run the Query Processing hook before saving to DB. - # Skipped on regeneration: the message already exists and was accepted previously. - # Skip for empty/whitespace-only messages — no meaningful query to process, - # and SendMessageRequest.message has no min_length guard. - if message_text.strip(): + # Runs only for new, non-blank messages: regeneration already processed + # this text, and SendMessageRequest.message has no min_length guard. The + # hook ships the query and user email out, so egress-suppressing modes skip it. + mode = chat_session.incognito_record_mode + if message_text.strip() and (mode is None or mode.fires_hooks): hook_result = execute_hook( db_session=db_session, hook_point=HookPoint.QUERY_PROCESSING, @@ -767,13 +782,22 @@ def build_chat_turn( # assistant/summary rows in save_chat.py) so budget math sums a single # unit even after mid-session model switches. default_tokenizer = get_tokenizer(None, None) + user_token_count = len(default_tokenizer.encode(message_text)) + # Incognito keeps the row for tracking (id, tokens, structure) but its + # text lives in the ephemeral store, never in Postgres. Token count is + # from the real text so usage and budgeting are unaffected. + keeps_content = record_mode_persists_content(mode) user_message = create_new_chat_message( chat_session_id=chat_session.id, parent_message=parent_message, - message=message_text, - token_count=len(default_tokenizer.encode(message_text)), + message=message_text if keeps_content else "", + token_count=user_token_count, message_type=MessageType.USER, - files=new_msg_req.file_descriptors, + files=( + new_msg_req.file_descriptors + if keeps_content + else content_free_file_descriptors(new_msg_req.file_descriptors) + ), db_session=db_session, commit=True, ) @@ -957,6 +981,26 @@ def build_chat_turn( ) simple_chat_history = chat_history_result.simple_messages + # Incognito rows are content-free, so earlier turns come from the store and + # the current message's text is restored onto convert_chat_history()'s + # blank-row shape. Regeneration uses the store as-is, it already holds the turn. + incognito_mode = chat_session.incognito_record_mode + if not record_mode_persists_content(incognito_mode): + stored_messages = load_incognito_context(chat_session.id).messages + is_new_user_message = parent_message.message_type != MessageType.USER + if ( + is_new_user_message + and simple_chat_history + and simple_chat_history[-1].message_type == MessageType.USER + ): + current_user = simple_chat_history[-1].model_copy( + update={"message": new_msg_req.message} + ) + simple_chat_history = stored_messages + [current_user] + append_incognito_message(chat_session.id, current_user) + else: + simple_chat_history = stored_messages + # Metadata for every text file injected into the history. After context-window # truncation drops older messages, the LLM loop compares surviving file_id tags # against this map to discover "forgotten" files and provide their metadata to @@ -991,8 +1035,12 @@ def build_chat_turn( cache = get_cache_backend() reset_cancel_status(chat_session.id, cache) + # Bind the id, not the row: this closure is stored on ChatTurnSetup and + # would otherwise keep a detached ChatSession reachable for the whole turn. + chat_session_id = chat_session.id + def check_is_connected() -> bool: - return check_stop_signal(chat_session.id, cache) + return check_stop_signal(chat_session_id, cache) set_processing_status( chat_session_id=chat_session.id, @@ -1012,9 +1060,11 @@ def check_is_connected() -> bool: return ChatTurnSetup( new_msg_req=new_msg_req, - chat_session=chat_session, + chat_session_id=chat_session.id, + chat_session_project_id=chat_session.project_id, + incognito_record_mode=chat_session.incognito_record_mode, persona=persona, - user_message=user_message, + user_message_id=user_message.id, user_identity=user_identity, llms=llms, model_display_names=model_display_names, @@ -1247,7 +1297,7 @@ def _run_post_steps() -> None: # "processing", whatever happened to the request generator. try: set_processing_status( - chat_session_id=setup.chat_session.id, + chat_session_id=setup.chat_session_id, cache=setup.cache, value=False, ) @@ -1287,8 +1337,8 @@ def _run_model(model_idx: int) -> None: auto_detect_filters=auto_detect_search_filters, ), custom_tool_config=CustomToolConfig( - chat_session_id=setup.chat_session.id, - message_id=setup.user_message.id, + chat_session_id=setup.chat_session_id, + message_id=setup.user_message_id, additional_headers=setup.custom_tool_additional_headers, mcp_headers=setup.mcp_headers, ), @@ -1312,7 +1362,7 @@ def _run_model(model_idx: int) -> None: # Per-thread copy: run_llm_loop mutates simple_chat_history in-place. if n_models == 1 and setup.new_msg_req.deep_research: - if setup.chat_session.project_id: + if setup.chat_session_project_id: raise RuntimeError("Deep research is not supported for projects") run_deep_research_llm_loop( emitter=model_emitter, @@ -1325,7 +1375,7 @@ def _run_model(model_idx: int) -> None: reasoning_effort=setup.reasoning_effort, skip_clarification=setup.skip_clarification, user_identity=setup.user_identity, - chat_session_id=str(setup.chat_session.id), + chat_session_id=str(setup.chat_session_id), all_injected_file_metadata=setup.all_injected_file_metadata, ) else: @@ -1342,7 +1392,7 @@ def _run_model(model_idx: int) -> None: token_counter=get_llm_token_counter(model_llm), forced_tool_id=setup.forced_tool_id, user_identity=setup.user_identity, - chat_session_id=str(setup.chat_session.id), + chat_session_id=str(setup.chat_session_id), chat_files=setup.chat_files_for_tools, reasoning_effort=setup.reasoning_effort, include_citations=setup.new_msg_req.include_citations, @@ -1371,18 +1421,33 @@ def _save_errored_message(model_idx: int, context: _PersistContext) -> None: ChatMessage, setup.reserved_messages[model_idx].id ) if msg is not None: - info = model_error_info[model_idx] - detail = ( - info.message - if info is not None - else "model encountered an error during generation." - ) - error_text = "Error from %s: %s" % ( - setup.model_display_names[model_idx], - detail, - ) + mode = setup.incognito_record_mode + if not record_mode_persists_content(mode): + # Provider errors can echo prompt fragments, so the + # durable row gets a generic marker. The live stream + # still carries the real error to the user. + error_text = "The model encountered an error." + else: + info = model_error_info[model_idx] + detail = ( + info.message + if info is not None + else "model encountered an error during generation." + ) + error_text = "Error from %s: %s" % ( + setup.model_display_names[model_idx], + detail, + ) msg.message = error_text msg.error = error_text + # The reservation's placeholder count must not survive: + # rows carry the real output count, zero when none emitted. + partial_answer = state_containers[model_idx].get_answer_tokens() + msg.token_count = ( + len(get_tokenizer(None, None).encode(partial_answer)) + if partial_answer + else 0 + ) save_db_session.commit() except Exception: logger.exception( @@ -1416,7 +1481,7 @@ def _drain_to_completion() -> None: last_fence_refresh = now try: set_processing_status( - chat_session_id=setup.chat_session.id, + chat_session_id=setup.chat_session_id, cache=setup.cache, value=True, run_id=setup.processing_run_id, @@ -1554,7 +1619,7 @@ def _read_stream() -> AnswerStream: # the writer thread keeps draining to completion in the background. logger.info( "chat stream reader detached; writer continues for session %s", - setup.chat_session.id, + setup.chat_session_id, ) return _read_stream() @@ -1610,6 +1675,7 @@ def _stream_chat_turn( ) mock_response_token: Token[str | None] | None = None + incognito_mode_flag_set = False setup: ChatTurnSetup | None = None pre_run_packets: list[AnswerStreamPart] = [] run_started = False @@ -1677,10 +1743,29 @@ def _stream_chat_turn( assert setup is not None, ( "build_chat_turn must complete before _run_models is called" ) + # Read at trace start, by the memory gate, and by interaction logging. + # Cleared with a plain set: a Token reset raises when this generator's + # frames resume under a different context. + if setup.incognito_record_mode is not None: + CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR.set( + setup.incognito_record_mode.value + ) + incognito_mode_flag_set = True + content_free = not record_mode_persists_content(setup.incognito_record_mode) + if content_free: + # Set for the whole turn so a blob any tool saves carries the + # session on its record, which is what teardown deletes by. + CURRENT_CONTENT_FREE_SESSION_ID_CONTEXTVAR.set(str(setup.chat_session_id)) stream_buffer = StreamBufferWriter( cache=setup.cache, - chat_session_id=setup.chat_session.id, + chat_session_id=setup.chat_session_id, run_id=setup.processing_run_id, + delete_on_done=content_free, + session_ended=( + (lambda: incognito_session_ended(setup.chat_session_id)) + if content_free + else None + ), ) for pre_run_packet in pre_run_packets: stream_buffer.append_line(get_json_line(pre_run_packet.model_dump())) @@ -1769,12 +1854,15 @@ def _stream_chat_turn( finally: if mock_response_token is not None: reset_llm_mock_response(mock_response_token) + if incognito_mode_flag_set: + CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR.set(None) + CURRENT_CONTENT_FREE_SESSION_ID_CONTEXTVAR.set(None) try: # Once _run_models started, its writer thread owns the fence — the # run may still be in flight after this generator is closed. if setup is not None and not run_started: set_processing_status( - chat_session_id=setup.chat_session.id, + chat_session_id=setup.chat_session_id, cache=setup.cache, value=False, ) @@ -1923,6 +2011,12 @@ def llm_loop_completion_handle( "ChatMessage %d not found during completion" % assistant_message_id ) + incognito_session = db_session.get(ChatSession, chat_session_id) + incognito_mode = ( + incognito_session.incognito_record_mode if incognito_session else None + ) + keeps_content = record_mode_persists_content(incognito_mode) + save_chat_turn( message_text=final_answer, reasoning_tokens=reasoning_tokens, @@ -1934,8 +2028,22 @@ def llm_loop_completion_handle( is_clarification=is_clarification, emitted_citations=emitted_citations, pre_answer_processing_time=pre_answer_processing_time, + persist_content=keeps_content, ) + # Incognito: the answer lives only in the ephemeral store, and + # compression is skipped since a summary is a durable content row. + if not keeps_content: + append_incognito_message( + chat_session_id, + ChatMessageSimple( + message=final_answer, + token_count=attached_message.token_count, + message_type=MessageType.ASSISTANT, + ), + ) + return + updated_chat_history = create_chat_history_chain( chat_session_id=chat_session_id, db_session=db_session, @@ -2090,6 +2198,7 @@ def gather_stream_full( message_id: int | None = None top_documents: list[SearchDoc] = [] chat_session_id: UUID | None = None + incognito = False for packet in packets: if isinstance(packet, Packet): @@ -2109,6 +2218,7 @@ def gather_stream_full( message_id = packet.reserved_assistant_message_id elif isinstance(packet, CreateChatSessionID): chat_session_id = packet.chat_session_id + incognito = packet.incognito if message_id is None: raise ValueError("Message ID is required") @@ -2141,5 +2251,6 @@ def gather_stream_full( citation_info=citations, message_id=message_id, chat_session_id=chat_session_id, + incognito=incognito, error_msg=error_msg, ) diff --git a/backend/onyx/chat/save_chat.py b/backend/onyx/chat/save_chat.py index fedeb36bed9..c85b9a14447 100644 --- a/backend/onyx/chat/save_chat.py +++ b/backend/onyx/chat/save_chat.py @@ -176,6 +176,7 @@ def save_chat_turn( is_clarification: bool = False, emitted_citations: set[int] | None = None, pre_answer_processing_time: float | None = None, + persist_content: bool = True, ) -> None: """ Save a chat turn by populating the assistant_message and creating related entities. @@ -206,10 +207,20 @@ def save_chat_turn( sanitized_message_text = ( sanitize_string(message_text) if message_text else message_text ) - assistant_message.message = sanitized_message_text - assistant_message.reasoning_tokens = ( - sanitize_string(reasoning_tokens) if reasoning_tokens else reasoning_tokens - ) + # A content-free turn keeps the row and its token count, which comes from + # the real answer, but none of the conversation-derived parts. + if persist_content: + assistant_message.message = sanitized_message_text + assistant_message.reasoning_tokens = ( + sanitize_string(reasoning_tokens) if reasoning_tokens else reasoning_tokens + ) + else: + assistant_message.message = "" + assistant_message.reasoning_tokens = None + tool_calls = [] + citation_to_doc = {} + all_search_docs = {} + emitted_citations = set() assistant_message.is_clarification = is_clarification # Use pre-answer processing time (captured when MESSAGE_START was emitted) diff --git a/backend/onyx/chat/stream_buffer.py b/backend/onyx/chat/stream_buffer.py index 9016a80bc8c..5a1402a572c 100644 --- a/backend/onyx/chat/stream_buffer.py +++ b/backend/onyx/chat/stream_buffer.py @@ -11,6 +11,7 @@ """ import zlib +from collections.abc import Callable from uuid import UUID from pydantic import BaseModel, ValidationError @@ -55,6 +56,11 @@ def _meta_key(chat_session_id: UUID, run_id: int) -> str: return f"{_PREFIX}_{chat_session_id}_{run_id}:meta" +def stream_buffer_key_pattern(chat_session_id: UUID) -> str: + """Glob matching every buffered chunk and meta key of the session's runs.""" + return f"{_PREFIX}_{chat_session_id}_*" + + class StreamBufferWriter: """Append-only writer for one run. Errors never propagate into the stream path — a broken cache downgrades the run to non-resumable (truncated).""" @@ -64,10 +70,20 @@ def __init__( cache: CacheBackend, chat_session_id: UUID, run_id: int, + delete_on_done: bool = False, + session_ended: Callable[[], bool] | None = None, ) -> None: self._cache = cache self._chat_session_id = chat_session_id self._run_id = run_id + # Content-free incognito runs: completion deletes the run's keys, so a + # flush racing the session teardown still cleans itself up. Costs + # post-completion resume. + self._delete_on_done = delete_on_done + # Teardown scans the session's keys once. Without this the run keeps + # writing answer chunks behind it, which then live out the buffer TTL + # if the run never reaches completion. + self._session_ended = session_ended self._meta = StreamBufferMeta() self._pending: list[str] = [] self._pending_bytes = 0 @@ -88,6 +104,11 @@ def append_line(self, line: str) -> None: def flush(self) -> None: if not self._pending or self._meta.truncated or self._meta.done: return + if self._session_ended is not None and self._session_ended(): + self._pending = [] + self._pending_bytes = 0 + self.mark_done() + return payload = zlib.compress("".join(self._pending).encode("utf-8")) self._pending = [] self._pending_bytes = 0 @@ -129,6 +150,23 @@ def flush(self) -> None: ) def mark_done(self) -> None: + if self._delete_on_done: + if self._meta.done: + return + self._meta.done = True + try: + self._cache.delete(_meta_key(self._chat_session_id, self._run_id)) + for chunk_n in range(self._meta.chunk_count): + self._cache.delete( + _chunk_key(self._chat_session_id, self._run_id, chunk_n) + ) + except Exception: + logger.exception( + "stream buffer deletion failed for session %s run %d", + self._chat_session_id, + self._run_id, + ) + return self.flush() if self._meta.done: return diff --git a/backend/onyx/configs/constants.py b/backend/onyx/configs/constants.py index 45ce6dd3c5a..b58d48c8e28 100644 --- a/backend/onyx/configs/constants.py +++ b/backend/onyx/configs/constants.py @@ -133,6 +133,10 @@ # NOTE: we use this timeout / 4 in various places to refresh a lock # might be worth separating this timeout into separate timeouts for each situation +# One pass of the incognito cleanup sweep. Leftovers wait for the next pass +# rather than holding the beat lock past its timeout. +INCOGNITO_FILE_CLEANUP_BATCH = 200 + CELERY_GENERIC_BEAT_LOCK_TIMEOUT = 120 CELERY_VESPA_SYNC_BEAT_LOCK_TIMEOUT = 120 @@ -560,6 +564,7 @@ class OnyxRedisLocks: USER_FILE_PROJECT_SYNC_LOCK_PREFIX = "da_lock:user_file_project_sync" USER_FILE_PROJECT_SYNC_QUEUED_PREFIX = "da_lock:user_file_project_sync_queued" USER_FILE_DELETE_BEAT_LOCK = "da_lock:check_user_file_delete_beat" + INCOGNITO_FILE_CLEANUP_BEAT_LOCK = "da_lock:check_incognito_file_cleanup_beat" USER_FILE_DELETE_LOCK_PREFIX = "da_lock:user_file_delete" # Short-lived key set when a delete task is enqueued; cleared when the worker picks it up. # Prevents the beat from re-enqueuing the same file while a delete task is already queued. @@ -643,6 +648,7 @@ class OnyxCeleryTask: PROCESS_SINGLE_USER_FILE_PROJECT_SYNC = "process_single_user_file_project_sync" CHECK_FOR_USER_FILE_DELETE = "check_for_user_file_delete" DELETE_SINGLE_USER_FILE = "delete_single_user_file" + CHECK_FOR_INCOGNITO_FILE_CLEANUP = "check_for_incognito_file_cleanup" # Targeted reindex TARGETED_REINDEX_TASK = "targeted_reindex_task" diff --git a/backend/onyx/db/chat.py b/backend/onyx/db/chat.py index 285be197452..72ce24b506a 100644 --- a/backend/onyx/db/chat.py +++ b/backend/onyx/db/chat.py @@ -12,6 +12,7 @@ from onyx.configs.constants import MessageType from onyx.context.search.models import InferenceSection, SavedSearchDoc from onyx.context.search.models import SearchDoc as ServerSearchDoc +from onyx.db.enums import IncognitoRecordMode from onyx.db.models import ( ChatMessage, ChatMessage__SearchDoc, @@ -93,6 +94,19 @@ def get_chat_sessions_by_slack_thread_id( return db_session.scalars(stmt).all() +def get_incognito_session_ids_for_user( + user_id: UUID, db_session: Session +) -> list[UUID]: + return list( + db_session.scalars( + select(ChatSession.id).where( + ChatSession.user_id == user_id, + ChatSession.incognito_record_mode.is_not(None), + ) + ) + ) + + # Retrieves chat sessions by user # Chat sessions do not include onyxbot flows def get_chat_sessions_by_user( @@ -104,6 +118,7 @@ def get_chat_sessions_by_user( project_id: int | None = None, only_non_project_chats: bool = False, include_failed_chats: bool = False, + exclude_incognito: bool = False, ) -> list[ChatSession]: stmt = ( select(ChatSession) @@ -112,6 +127,9 @@ def get_chat_sessions_by_user( .order_by(desc(ChatSession.time_updated)) ) + if exclude_incognito: + stmt = stmt.where(ChatSession.incognito_record_mode.is_(None)) + if deleted is not None: stmt = stmt.where(ChatSession.deleted == deleted) @@ -215,6 +233,7 @@ def create_chat_session( onyxbot_flow: bool = False, slack_thread_id: str | None = None, project_id: int | None = None, + incognito_record_mode: IncognitoRecordMode | None = None, ) -> ChatSession: chat_session = ChatSession( user_id=user_id, @@ -225,6 +244,7 @@ def create_chat_session( onyxbot_flow=onyxbot_flow, slack_thread_id=slack_thread_id, project_id=project_id, + incognito_record_mode=incognito_record_mode, ) db_session.add(chat_session) @@ -250,6 +270,8 @@ def duplicate_chat_session_for_user_from_slack( user_id=None, # Ignore user permissions for this db_session=db_session, ) + if chat_session.incognito_record_mode is not None: + raise ValueError("Incognito chat sessions cannot be duplicated") if not chat_session: raise HTTPException(status_code=400, detail="Invalid Chat Session ID provided") @@ -474,6 +496,9 @@ def add_chats_to_session_from_slack_thread( slack_chat_session_id: UUID, new_chat_session_id: UUID, ) -> None: + source_session = db_session.get(ChatSession, slack_chat_session_id) + if source_session and source_session.incognito_record_mode is not None: + raise ValueError("Incognito chat sessions cannot be duplicated") new_root_message = get_or_create_root_message( chat_session_id=new_chat_session_id, db_session=db_session, diff --git a/backend/onyx/db/chat_search.py b/backend/onyx/db/chat_search.py index f2c132c7361..1e926d5794f 100644 --- a/backend/onyx/db/chat_search.py +++ b/backend/onyx/db/chat_search.py @@ -32,6 +32,7 @@ def search_chat_sessions( stmt = ( select(ChatSession) .where(ChatSession.onyxbot_flow.is_(False)) + .where(ChatSession.incognito_record_mode.is_(None)) .order_by(desc(ChatSession.time_created)) .offset(offset_val) .limit(page_size + 1) @@ -53,7 +54,12 @@ def search_chat_sessions( # Otherwise, proceed with full-text search query = query.strip() - base_conditions: list[ColumnElement[bool]] = [ChatSession.onyxbot_flow.is_(False)] + # Applied to both arms of the union, so an incognito session cannot surface + # through a message body when its description does not match. + base_conditions: list[ColumnElement[bool]] = [ + ChatSession.onyxbot_flow.is_(False), + ChatSession.incognito_record_mode.is_(None), + ] if user_id is not None: base_conditions.append(ChatSession.user_id == user_id) if not include_deleted: diff --git a/backend/onyx/db/file_record.py b/backend/onyx/db/file_record.py index a9f65af2e1e..a09fa2bf313 100644 --- a/backend/onyx/db/file_record.py +++ b/backend/onyx/db/file_record.py @@ -6,6 +6,8 @@ from onyx.configs.constants import FileOrigin, FileType from onyx.db.enums import IndexingStatus from onyx.db.models import FileRecord, IndexAttempt +from onyx.file_store.constants import INCOGNITO_SESSION_METADATA_KEY +from shared_configs.contextvars import CURRENT_CONTENT_FREE_SESSION_ID_CONTEXTVAR def get_query_history_export_files( @@ -190,7 +192,18 @@ def upsert_filerecord( file_size: int | None = None, ) -> FileRecord: """Atomic upsert using INSERT ... ON CONFLICT DO UPDATE to avoid - race conditions when concurrent calls target the same file_id.""" + race conditions when concurrent calls target the same file_id. + + Every backend writes its record here, so this is also where a blob saved + during a content-free chat turn gets stamped with its session. The stamp + is the only handle cleanup has, and it lands with the record itself. + """ + session_id = CURRENT_CONTENT_FREE_SESSION_ID_CONTEXTVAR.get() + if session_id is not None: + file_metadata = { + **(file_metadata or {}), + INCOGNITO_SESSION_METADATA_KEY: session_id, + } stmt = insert(FileRecord).values( file_id=file_id, display_name=display_name, @@ -216,3 +229,30 @@ def upsert_filerecord( db_session.execute(stmt) return db_session.get(FileRecord, file_id) # ty: ignore[invalid-return-type] + + +def get_incognito_file_ids(session_id: str, db_session: Session) -> list[str]: + """Ids of blobs a content-free session produced and has not deleted yet.""" + return list( + db_session.scalars( + select(FileRecord.file_id).where( + FileRecord.file_metadata[INCOGNITO_SESSION_METADATA_KEY].astext + == session_id + ) + ) + ) + + +def get_session_ids_with_incognito_files( + db_session: Session, limit: int | None = None +) -> list[str]: + """Sessions still holding blobs. Empty in steady state, since teardown + deletes the records, so this only sees what a store failure left.""" + return list( + db_session.scalars( + select(FileRecord.file_metadata[INCOGNITO_SESSION_METADATA_KEY].astext) + .distinct() + .where(FileRecord.file_metadata.has_key(INCOGNITO_SESSION_METADATA_KEY)) + .limit(limit) + ) + ) diff --git a/backend/onyx/error_handling/error_codes.py b/backend/onyx/error_handling/error_codes.py index bf57a783b62..129e49bfd16 100644 --- a/backend/onyx/error_handling/error_codes.py +++ b/backend/onyx/error_handling/error_codes.py @@ -43,6 +43,8 @@ class OnyxErrorCode(Enum): EE_REQUIRED = ("EE_REQUIRED", 403) SINGLE_TENANT_ONLY = ("SINGLE_TENANT_ONLY", 403) ENV_VAR_GATED = ("ENV_VAR_GATED", 403) + # The deployment cannot support the feature at all, so no grant helps. + DEPLOYMENT_UNSUPPORTED = ("DEPLOYMENT_UNSUPPORTED", 403) # -------------------------------------------------------------------------- # Validation / Bad Request (400) diff --git a/backend/onyx/file_store/constants.py b/backend/onyx/file_store/constants.py index a0845d35ef7..b6836dcd48e 100644 --- a/backend/onyx/file_store/constants.py +++ b/backend/onyx/file_store/constants.py @@ -1,2 +1,6 @@ MAX_IN_MEMORY_SIZE = 30 * 1024 * 1024 # 30MB STANDARD_CHUNK_SIZE = 10 * 1024 * 1024 # 10MB chunks + +# Marks a blob a content-free chat turn produced, so cleanup can find it by the +# record itself rather than by anything that can expire. +INCOGNITO_SESSION_METADATA_KEY = "incognito_session_id" diff --git a/backend/onyx/server/features/projects/models.py b/backend/onyx/server/features/projects/models.py index 4c3dba40728..f22eb1a7c94 100644 --- a/backend/onyx/server/features/projects/models.py +++ b/backend/onyx/server/features/projects/models.py @@ -95,10 +95,12 @@ def from_model(cls, model: UserProject) -> "UserProjectSnapshot": created_at=model.created_at, user_id=model.user_id, instructions=model.instructions, + # A project lists its sessions by title, so an incognito chat would + # surface here the same way it would in the sidebar. chat_sessions=[ ChatSessionDetails.from_model(chat) for chat in model.chat_sessions - if not chat.deleted + if not chat.deleted and chat.incognito_record_mode is None ], ) diff --git a/backend/onyx/server/query_and_chat/chat_backend.py b/backend/onyx/server/query_and_chat/chat_backend.py index 8f2afca4ea1..ae49ffdecb3 100644 --- a/backend/onyx/server/query_and_chat/chat_backend.py +++ b/backend/onyx/server/query_and_chat/chat_backend.py @@ -27,6 +27,8 @@ create_chat_session_from_request, extract_headers, ) +from onyx.chat.incognito import delete_incognito_generated_files +from onyx.chat.incognito_context import teardown_incognito_session from onyx.chat.models import ChatFullResponse, CreateChatSessionID from onyx.chat.process_message import ( gather_stream_full, @@ -53,6 +55,7 @@ get_chat_messages_by_session, get_chat_session_by_id, get_chat_sessions_by_user, + get_incognito_session_ids_for_user, set_as_latest_chat_message, set_preferred_response, translate_db_message_to_chat_message_detail, @@ -60,7 +63,7 @@ ) from onyx.db.chat_search import search_chat_sessions from onyx.db.engine.sql_engine import get_session, get_session_with_current_tenant -from onyx.db.enums import Permission +from onyx.db.enums import Permission, record_mode_persists_content from onyx.db.feedback import create_chat_message_feedback, remove_chat_message_feedback from onyx.db.llm import fetch_default_chat_naming_model from onyx.db.models import ChatMessage, ChatSessionSharedStatus, Persona, User @@ -79,6 +82,7 @@ ) from onyx.llm.override_models import LLMOverride from onyx.secondary_llm_flows.chat_session_naming import ( + DEFAULT_CHAT_SESSION_NAME, generate_chat_session_name, get_fallback_chat_session_name, ) @@ -199,6 +203,8 @@ def get_user_chat_sessions( project_id=project_id, only_non_project_chats=only_non_project_chats, include_failed_chats=include_failed_chats, + # The owner's own history is the one surface incognito hides from. + exclude_incognito=True, limit=page_size + 1, before=before_dt, ) @@ -433,6 +439,7 @@ def get_chat_session( # Packets are now directly serialized as Packet Pydantic models packets=replay_packet_lists, current_run=current_run, + incognito=chat_session.incognito_record_mode is not None, ) @@ -450,6 +457,9 @@ def create_new_chat_session( user=user, db_session=db_session, ) + except OnyxError: + # Carries its own status and detail (e.g. incognito refused). + raise except ValueError as e: # Project or persona access denied raise HTTPException(status_code=403, detail=str(e)) @@ -457,7 +467,10 @@ def create_new_chat_session( logger.exception(e) raise HTTPException(status_code=400, detail="Invalid Persona provided.") - return CreateChatSessionID(chat_session_id=new_chat_session.id) + return CreateChatSessionID( + chat_session_id=new_chat_session.id, + incognito=new_chat_session.incognito_record_mode is not None, + ) def _generate_or_fallback_chat_session_name( @@ -541,6 +554,11 @@ def rename_chat_session( db_session=db_session, eager_load_persona=True, ) + # Auto-naming derives a title from the conversation and writes it to the + # session row. A non-persisting incognito mode keeps no content in + # Postgres, so it keeps the fallback name and skips the LLM call. + if not record_mode_persists_content(chat_session.incognito_record_mode): + return RenameChatSessionResponse(new_name=DEFAULT_CHAT_SESSION_NAME) full_history = create_chat_history_chain( chat_session_id=chat_session_id, db_session=db_session, @@ -594,15 +612,37 @@ def patch_chat_session( return None +def _teardown_incognito_after_delete(chat_session_id: UUID) -> None: + """The rows are already gone, so a failed teardown is logged rather than + failing a delete the caller cannot retry. The context TTL is the backstop.""" + try: + teardown_incognito_session(chat_session_id) + except Exception: + logger.exception("Incognito teardown failed for session %s", chat_session_id) + + @router.delete("/delete-all-chat-sessions", tags=PUBLIC_API_TAGS) def delete_all_chat_sessions( user: User = Depends(require_permission(Permission.BASIC_ACCESS)), db_session: Session = Depends(get_session), ) -> None: + incognito_session_ids = get_incognito_session_ids_for_user(user.id, db_session) + # Blobs first, and nothing is deleted while any remain: their ids live on + # the rows this is about to drop, so the other order strands them. + if not all( + delete_incognito_generated_files(incognito_id, db_session) + for incognito_id in incognito_session_ids + ): + raise OnyxError( + OnyxErrorCode.SERVICE_UNAVAILABLE, + "Some generated files could not be deleted yet. Try again shortly.", + ) try: delete_all_chat_sessions_for_user(user=user, db_session=db_session) except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) + for incognito_id in incognito_session_ids: + _teardown_incognito_after_delete(incognito_id) @router.delete("/delete-chat-session/{session_id}", tags=PUBLIC_API_TAGS) @@ -614,6 +654,17 @@ def delete_chat_session_by_id( ) -> None: user_id = user.id try: + session = get_chat_session_by_id( + chat_session_id=session_id, user_id=user_id, db_session=db_session + ) + is_incognito = session.incognito_record_mode is not None + if is_incognito and not delete_incognito_generated_files( + session_id, db_session + ): + raise OnyxError( + OnyxErrorCode.SERVICE_UNAVAILABLE, + "Some generated files could not be deleted yet. Try again shortly.", + ) # Use the provided hard_delete parameter if specified, otherwise use the default config actual_hard_delete = ( hard_delete if hard_delete is not None else HARD_DELETE_CHATS @@ -623,6 +674,31 @@ def delete_chat_session_by_id( ) except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) + if is_incognito: + _teardown_incognito_after_delete(session_id) + + +@router.post("/end-incognito-session/{session_id}", tags=PUBLIC_API_TAGS) +def end_incognito_session( + session_id: UUID, + user: User = Depends(require_permission(Permission.BASIC_ACCESS)), + db_session: Session = Depends(get_session), +) -> None: + """Drop an incognito session's live context the moment the chat closes. + + The context TTL is only the backstop for when this never arrives, such as + a hard tab close. + """ + chat_session = get_chat_session_by_id( + chat_session_id=session_id, user_id=user.id, db_session=db_session + ) + if chat_session.incognito_record_mode is not None: + teardown_incognito_session(session_id) + if not delete_incognito_generated_files(session_id, db_session): + raise OnyxError( + OnyxErrorCode.SERVICE_UNAVAILABLE, + "Some generated files could not be deleted yet and will be retried.", + ) # NOTE: This endpoint is extremely central to the application, any changes to it should be reviewed and approved by an experienced @@ -674,7 +750,12 @@ def handle_send_chat_message( Returns: StreamingResponse | ChatFullResponse: Either streams or returns complete response. """ - logger.debug("Received new chat message: %s", chat_message_req.message) + # Session id only: the session's incognito mode isn't loaded yet, and a + # verbatim prompt in the debug log would be exactly the durable message + # log incognito must never leave behind. + logger.debug( + "Received new chat message for session %s", chat_message_req.chat_session_id + ) tenant_id = get_current_tenant_id() mt_cloud_telemetry( diff --git a/backend/onyx/server/query_and_chat/models.py b/backend/onyx/server/query_and_chat/models.py index e8831083cd4..703f46784c0 100644 --- a/backend/onyx/server/query_and_chat/models.py +++ b/backend/onyx/server/query_and_chat/models.py @@ -79,6 +79,9 @@ class ChatSessionCreationRequest(BaseModel): persona_id: int = 0 description: str | None = None project_id: int | None = None + # Start the session incognito. Refused with an error when incognito is + # unavailable, never silently downgraded to an ordinary chat. + incognito: bool = False class ChatFeedbackRequest(BaseModel): @@ -274,6 +277,9 @@ class ChatSessionDetailResponse(BaseModel): # Set while a run is in flight and resumable: cursor-0 replay+tail is # available at /chat-session/{id}/resume-stream. current_run: CurrentRunInfo | None = None + # True for sessions pinned to an incognito record mode, so a reload can + # restore the incognito UI state. + incognito: bool = False class AdminSearchRequest(BaseModel): diff --git a/backend/onyx/tools/models.py b/backend/onyx/tools/models.py index 999defbd901..3bbef6ec084 100644 --- a/backend/onyx/tools/models.py +++ b/backend/onyx/tools/models.py @@ -274,6 +274,8 @@ class ToolCallInfo(BaseModel): search_docs: list[SearchDoc] | None = None generated_images: list[GeneratedImage] | None = None generated_files: list[PythonExecutionFile] | None = None + # File-store ids of blobs custom tools saved during the call. + generated_file_ids: list[str] | None = None CHAT_SESSION_ID_PLACEHOLDER = "CHAT_SESSION_ID" diff --git a/backend/onyx/tools/tool_implementations/memory/memory_tool.py b/backend/onyx/tools/tool_implementations/memory/memory_tool.py index 67917d1bd8b..3bafad4ba5b 100644 --- a/backend/onyx/tools/tool_implementations/memory/memory_tool.py +++ b/backend/onyx/tools/tool_implementations/memory/memory_tool.py @@ -12,6 +12,7 @@ from typing_extensions import override from onyx.chat.emitter import Emitter +from onyx.chat.incognito import current_turn_persists_content from onyx.llm.interfaces import LLM from onyx.secondary_llm_flows.memory_update import process_memory_update from onyx.server.query_and_chat.placement import Placement @@ -134,7 +135,8 @@ def run( user_role=override_kwargs.user_role, ) - logger.info("New memory to be added: %s", memory_text) + if current_turn_persists_content(): + logger.info("New memory to be added: %s", memory_text) operation: Literal["add", "update"] = ( "update" if index_to_replace is not None else "add" diff --git a/backend/onyx/tracing/braintrust_tracing_processor.py b/backend/onyx/tracing/braintrust_tracing_processor.py index 43917a8e818..ff791c00371 100644 --- a/backend/onyx/tracing/braintrust_tracing_processor.py +++ b/backend/onyx/tracing/braintrust_tracing_processor.py @@ -6,6 +6,7 @@ from onyx.llm.cost import compute_cost_cents from onyx.tracing.flows import IMAGE_FLOWS +from onyx.tracing.incognito import suppresses_external_traces from .framework.processor_interface import TracingProcessor from .framework.span_data import ( @@ -72,8 +73,16 @@ def __init__(self, logger: Optional[braintrust.Logger] = None): self._last_output: Dict[str, Any] = {} self._trace_metadata: Dict[str, Dict[str, Any]] = {} self._span_names: Dict[str, str] = {} + # Traces suppressed at start. Membership decides every later callback, + # so an incognito flag change mid-trace cannot mismatch start/end state. + self._suppressed_traces: set[str] = set() def on_trace_start(self, trace: Trace) -> None: + # Incognito turns must leave no content in external tracing, so the + # whole trace is dropped, spans included. + if suppresses_external_traces(): + self._suppressed_traces.add(trace.trace_id) + return trace_meta = trace.export() or {} metadata = trace_meta.get("metadata") or {} if metadata: @@ -102,6 +111,9 @@ def on_trace_start(self, trace: Trace) -> None: self._span_names[trace.trace_id] = trace.name def on_trace_end(self, trace: Trace) -> None: + if trace.trace_id in self._suppressed_traces: + self._suppressed_traces.discard(trace.trace_id) + return span: Any = self._spans.pop(trace.trace_id) self._trace_metadata.pop(trace.trace_id, None) self._span_names.pop(trace.trace_id, None) @@ -227,6 +239,8 @@ def _log_data(self, span: Span[Any]) -> Dict[str, Any]: return {} def on_span_start(self, span: Span[SpanData]) -> None: + if span.trace_id in self._suppressed_traces: + return parent: Any = ( self._spans[span.parent_id] if span.parent_id is not None @@ -253,6 +267,8 @@ def on_span_start(self, span: Span[SpanData]) -> None: created_span.set_current() def on_span_end(self, span: Span[SpanData]) -> None: + if span.trace_id in self._suppressed_traces: + return s: Any = self._spans.pop(span.span_id) self._span_names.pop(span.span_id, None) event = dict(error=span.error, **self._log_data(span)) diff --git a/backend/onyx/tracing/incognito.py b/backend/onyx/tracing/incognito.py new file mode 100644 index 00000000000..c500223cb0f --- /dev/null +++ b/backend/onyx/tracing/incognito.py @@ -0,0 +1,9 @@ +from onyx.db.enums import IncognitoRecordMode +from shared_configs.contextvars import get_current_incognito_record_mode + + +def suppresses_external_traces() -> bool: + """External tracing processors must check this at trace start and drop the + whole trace, spans included, when True.""" + mode = IncognitoRecordMode.from_context_value(get_current_incognito_record_mode()) + return mode is not None and not mode.emits_external_traces diff --git a/backend/onyx/tracing/langfuse_tracing_processor.py b/backend/onyx/tracing/langfuse_tracing_processor.py index c4db920d4dd..9799405b7c9 100644 --- a/backend/onyx/tracing/langfuse_tracing_processor.py +++ b/backend/onyx/tracing/langfuse_tracing_processor.py @@ -20,6 +20,7 @@ ) from onyx.tracing.framework.spans import Span from onyx.tracing.framework.traces import Trace +from onyx.tracing.incognito import suppresses_external_traces logger = logging.getLogger(__name__) @@ -64,6 +65,8 @@ def __init__( self._langfuse_span_ids: dict[ str, str ] = {} # framework_span_id -> langfuse_span.id + # Membership decides every later callback, immune to mid-trace changes. + self._suppressed_traces: set[str] = set() def _get_client(self) -> Langfuse: """Get or create Langfuse client.""" @@ -125,6 +128,10 @@ def _calculate_cost(self, data: GenerationSpanData) -> Optional[float]: def on_trace_start(self, trace: Trace) -> None: """Called when a trace is started.""" + if suppresses_external_traces(): + with self._lock: + self._suppressed_traces.add(trace.trace_id) + return try: client = self._get_client() trace_meta = trace.export() or {} @@ -162,6 +169,10 @@ def on_trace_start(self, trace: Trace) -> None: def on_trace_end(self, trace: Trace) -> None: """Called when a trace is finished.""" + with self._lock: + if trace.trace_id in self._suppressed_traces: + self._suppressed_traces.discard(trace.trace_id) + return try: with self._lock: langfuse_span = self._trace_spans.pop(trace.trace_id, None) @@ -191,6 +202,9 @@ def on_span_start(self, span: Span[SpanData]) -> None: agents run in parallel threads, and calling methods on span objects created in other threads can cause OpenTelemetry context issues. """ + with self._lock: + if span.trace_id in self._suppressed_traces: + return try: data = span.span_data # Declare as Any since different code paths return different observation types diff --git a/backend/shared_configs/contextvars.py b/backend/shared_configs/contextvars.py index 8b900d71605..67783b96f27 100644 --- a/backend/shared_configs/contextvars.py +++ b/backend/shared_configs/contextvars.py @@ -36,6 +36,19 @@ "current_user_id", default=None ) +# IncognitoRecordMode value of the streaming turn's session, None outside +# incognito. A plain string keeps this layer free of onyx imports. +CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR: contextvars.ContextVar[str | None] = ( + contextvars.ContextVar("current_incognito_record_mode", default=None) +) + +# Session id of a content-free turn, and only of a content-free turn: a blob +# saved while this is set is conversation-derived and must die with the +# session, so the file store stamps it on the record at creation. +CURRENT_CONTENT_FREE_SESSION_ID_CONTEXTVAR: contextvars.ContextVar[str | None] = ( + contextvars.ContextVar("current_content_free_session_id", default=None) +) + class UsageCredentialIdentity(NamedTuple): credential_type: UsageCredentialType @@ -71,5 +84,10 @@ def get_current_user_id() -> str | None: return CURRENT_USER_ID_CONTEXTVAR.get() +def get_current_incognito_record_mode() -> str | None: + """The incognito record-mode value of the current turn, None outside one.""" + return CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR.get() + + def get_current_usage_credential() -> UsageCredentialIdentity | None: return CURRENT_USAGE_CREDENTIAL_CONTEXTVAR.get() diff --git a/backend/tests/external_dependency_unit/chat/test_incognito_persistence.py b/backend/tests/external_dependency_unit/chat/test_incognito_persistence.py new file mode 100644 index 00000000000..571bcd497d4 --- /dev/null +++ b/backend/tests/external_dependency_unit/chat/test_incognito_persistence.py @@ -0,0 +1,266 @@ +"""Guards the incognito persistence seams against real Postgres and Redis. + +Two behaviors the feature rests on: save_chat_turn keeps the assistant row for +tracking but writes no text when content is not persisted, and the ephemeral +store round-trips a turn's messages so the next turn has its context. Run here +rather than as unit tests because both only mean something against the real +stores. +""" + +from collections.abc import Generator +from io import BytesIO +from unittest.mock import patch +from uuid import UUID, uuid4 + +import pytest +from sqlalchemy.orm import Session + +from onyx.chat.incognito import delete_incognito_generated_files +from onyx.chat.incognito_context import ( + append_incognito_message, + load_incognito_context, + teardown_incognito_session, +) +from onyx.chat.models import ChatMessageSimple +from onyx.chat.save_chat import save_chat_turn +from onyx.configs.constants import DocumentSource, FileOrigin, MessageType +from onyx.context.search.models import SearchDoc +from onyx.db.chat import ( + create_chat_session, + get_or_create_root_message, + reserve_message_id, +) +from onyx.db.file_record import ( + get_incognito_file_ids, + get_session_ids_with_incognito_files, +) +from onyx.db.models import ChatMessage, ChatSession, User +from onyx.file_store.file_store import get_default_file_store +from onyx.redis.redis_pool import get_redis_client +from onyx.tools.models import ToolCallInfo +from shared_configs.contextvars import CURRENT_CONTENT_FREE_SESSION_ID_CONTEXTVAR +from tests.external_dependency_unit.conftest import create_test_user + + +@pytest.fixture +def owner(db_session: Session) -> Generator[User, None, None]: + user = create_test_user(db_session, "incognito-persist") + yield user + db_session.rollback() + db_session.query(ChatSession).filter(ChatSession.user_id == user.id).delete() + db_session.delete(user) + db_session.commit() + + +def _new_session(db_session: Session, user_id: UUID) -> ChatSession: + return create_chat_session( + db_session=db_session, + description="incognito", + user_id=user_id, + persona_id=None, + ) + + +def _reserve_assistant(db_session: Session, session_id: UUID) -> ChatMessage: + root = get_or_create_root_message(chat_session_id=session_id, db_session=db_session) + return reserve_message_id( + db_session=db_session, + chat_session_id=session_id, + parent_message=root.id, + ) + + +def _search_doc(document_id: str) -> SearchDoc: + return SearchDoc( + document_id=document_id, + chunk_ind=0, + semantic_identifier="secret doc", + blurb="confidential excerpt", + source_type=DocumentSource.WEB, + boost=0, + hidden=False, + metadata={}, + match_highlights=["confidential"], + ) + + +def test_save_chat_turn_keeps_the_row_but_writes_no_text( + db_session: Session, owner: User +) -> None: + session = _new_session(db_session, owner.id) + assistant = _reserve_assistant(db_session, session.id) + + doc = _search_doc("secret-doc-1") + save_chat_turn( + message_text="the acquisition target is confidential", + reasoning_tokens="secret reasoning", + tool_calls=[ + ToolCallInfo( + parent_tool_call_id=None, + turn_index=0, + tab_index=0, + tool_name="run_search", + tool_call_id="call-1", + tool_id=1, + reasoning_tokens=None, + tool_call_arguments={"query": "the confidential query"}, + tool_call_response="retrieved excerpt text", + search_docs=[doc], + ) + ], + citation_to_doc={1: doc}, + all_search_docs={doc.document_id: doc}, + db_session=db_session, + assistant_message=assistant, + emitted_citations={1}, + persist_content=False, + ) + db_session.commit() + + stored = db_session.get(ChatMessage, assistant.id) + assert stored is not None + # Row survives for tracking, with a real token count, but no text. + assert stored.message == "" + assert stored.reasoning_tokens is None + assert stored.token_count > 0 + # Conversation-derived artifacts stay out too: no tool calls, no search + # docs, no citations for a content-free turn. + assert not stored.tool_calls + assert not stored.search_docs + assert not stored.citations + + +def test_save_chat_turn_persists_text_by_default( + db_session: Session, owner: User +) -> None: + session = _new_session(db_session, owner.id) + assistant = _reserve_assistant(db_session, session.id) + + save_chat_turn( + message_text="an ordinary answer", + reasoning_tokens=None, + tool_calls=[], + citation_to_doc={}, + all_search_docs={}, + db_session=db_session, + assistant_message=assistant, + ) + db_session.commit() + + stored = db_session.get(ChatMessage, assistant.id) + assert stored is not None + assert stored.message == "an ordinary answer" + + +def test_turn_round_trips_through_the_store() -> None: + """A turn appends the user message then the answer. The next turn loads both + in order so the model sees its own context.""" + session_id = uuid4() + try: + append_incognito_message( + session_id, + ChatMessageSimple( + message="what is our runway", + token_count=4, + message_type=MessageType.USER, + ), + ) + append_incognito_message( + session_id, + ChatMessageSimple( + message="eighteen months", + token_count=2, + message_type=MessageType.ASSISTANT, + ), + ) + + history = load_incognito_context(session_id).messages + assert [(m.message_type, m.message) for m in history] == [ + (MessageType.USER, "what is our runway"), + (MessageType.ASSISTANT, "eighteen months"), + ] + finally: + teardown_incognito_session(session_id) + + +def test_teardown_ends_the_session_immediately() -> None: + session_id = uuid4() + append_incognito_message( + session_id, + ChatMessageSimple( + message="secret", token_count=1, message_type=MessageType.USER + ), + ) + assert load_incognito_context(session_id).messages + + teardown_incognito_session(session_id) + + assert load_incognito_context(session_id).messages == [] + + +def test_teardown_clears_buffered_stream_chunks() -> None: + """The stream buffer holds the streamed answer NDJSON, so teardown must + delete it with the context instead of leaving it to the TTL.""" + session_id = uuid4() + client = get_redis_client() + chunk_key = f"chatstream_{session_id}_1:0" + client.set(chunk_key, b"buffered answer text", ex=600) + assert client.get(chunk_key) is not None + + teardown_incognito_session(session_id) + + assert client.get(chunk_key) is None + + +def _content_free_blob(session_id: UUID) -> str: + """Save a blob the way a tool does inside a content-free turn.""" + token = CURRENT_CONTENT_FREE_SESSION_ID_CONTEXTVAR.set(str(session_id)) + try: + return get_default_file_store().save_file( + content=BytesIO(b"generated chart bytes"), + display_name="chart.png", + file_origin=FileOrigin.CHAT_IMAGE_GEN, + file_type="image/png", + ) + finally: + CURRENT_CONTENT_FREE_SESSION_ID_CONTEXTVAR.reset(token) + + +def test_a_blob_is_stamped_when_it_is_saved(db_session: Session) -> None: + """The stamp lands with the record, so no window exists where a blob is + durable but unfindable.""" + session_id = uuid4() + file_id = _content_free_blob(session_id) + + assert get_incognito_file_ids(str(session_id), db_session) == [file_id] + + +def test_teardown_deletes_the_stamped_blobs(db_session: Session) -> None: + session_id = uuid4() + file_id = _content_free_blob(session_id) + file_store = get_default_file_store() + + assert delete_incognito_generated_files(session_id, db_session) + + with pytest.raises(Exception): + file_store.read_file(file_id) + assert get_incognito_file_ids(str(session_id), db_session) == [] + + +def test_a_refused_deletion_keeps_the_stamp(db_session: Session) -> None: + """A store outage must leave the blob findable for the sweep.""" + session_id = uuid4() + file_id = _content_free_blob(session_id) + file_store = get_default_file_store() + + with patch.object( + type(file_store), "delete_file", side_effect=RuntimeError("store blip") + ): + assert not delete_incognito_generated_files(session_id, db_session) + + assert get_incognito_file_ids(str(session_id), db_session) == [file_id] + assert str(session_id) in get_session_ids_with_incognito_files(db_session) + + assert delete_incognito_generated_files(session_id, db_session) + with pytest.raises(Exception): + file_store.read_file(file_id) diff --git a/backend/tests/external_dependency_unit/db/test_incognito_history_exclusion.py b/backend/tests/external_dependency_unit/db/test_incognito_history_exclusion.py new file mode 100644 index 00000000000..f33c3d66c3d --- /dev/null +++ b/backend/tests/external_dependency_unit/db/test_incognito_history_exclusion.py @@ -0,0 +1,178 @@ +"""Guards that an incognito session stays out of every surface its owner sees. + +Covers the three that list a user's own sessions: history, search, and the +chat list a project carries. Exercises the real WHERE clauses against Postgres, +since a mocked session would happily return rows the SQL would have filtered. +""" + +from collections.abc import Generator +from uuid import UUID + +import pytest +from sqlalchemy.orm import Session + +from onyx.configs.constants import MessageType +from onyx.db.chat import ( + create_chat_session, + create_new_chat_message, + get_chat_sessions_by_user, + get_or_create_root_message, +) +from onyx.db.chat_search import search_chat_sessions +from onyx.db.enums import IncognitoRecordMode +from onyx.db.models import ChatSession, User, UserProject +from onyx.server.features.projects.models import UserProjectSnapshot +from tests.external_dependency_unit.conftest import create_test_user + + +@pytest.fixture +def owner(db_session: Session) -> Generator[User, None, None]: + """A user whose rows are deleted afterwards rather than rolled back. + + ``create_chat_session`` commits, so a rollback cannot reach these rows, and + the filter under test hides them from the UI that would otherwise clean them + up. Left alone they accumulate as sessions nobody can see or remove. + """ + user = create_test_user(db_session, "incognito-history") + yield user + + db_session.rollback() + # Sessions before projects: chat_session.project_id references user_project. + db_session.query(ChatSession).filter(ChatSession.user_id == user.id).delete() + db_session.query(UserProject).filter(UserProject.user_id == user.id).delete() + db_session.delete(user) + db_session.commit() + + +def _make_session( + db_session: Session, + user_id: UUID, + description: str, + mode: IncognitoRecordMode | None, + project_id: int | None = None, +) -> ChatSession: + chat_session = create_chat_session( + db_session=db_session, + description=description, + user_id=user_id, + persona_id=None, + project_id=project_id, + ) + chat_session.incognito_record_mode = mode + # Commit rather than flush: the next create_chat_session would otherwise be + # what commits this assignment, leaving the last one written only in memory. + db_session.commit() + return chat_session + + +def _history_ids(db_session: Session, user_id: UUID) -> set[UUID]: + """The owner's own history, which is the call site that opts out.""" + return { + session.id + for session in get_chat_sessions_by_user( + user_id=user_id, + deleted=None, + db_session=db_session, + include_failed_chats=True, + exclude_incognito=True, + ) + } + + +def test_every_mode_is_excluded_from_history(db_session: Session, owner: User) -> None: + """Every mode must stay out of the owner's history. + + Iterates the enum so a newly added mode is covered without editing this + test. + """ + sessions = { + mode: _make_session(db_session, owner.id, f"chat {mode.value}", mode) + for mode in IncognitoRecordMode + } + + returned_ids = _history_ids(db_session, owner.id) + for mode, chat_session in sessions.items(): + assert chat_session.id not in returned_ids, f"{mode.value} leaked" + + +def test_search_excludes_incognito_matching_the_query( + db_session: Session, owner: User +) -> None: + """The description arm of the union must not surface an incognito session.""" + ordinary = _make_session(db_session, owner.id, "penguin migration notes", None) + incognito = _make_session( + db_session, + owner.id, + "penguin migration secrets", + IncognitoRecordMode.FULL_HISTORY, + ) + + sessions, _ = search_chat_sessions( + user_id=owner.id, db_session=db_session, query="penguin" + ) + returned_ids = {session.id for session in sessions} + assert ordinary.id in returned_ids + assert incognito.id not in returned_ids + + +def test_project_does_not_list_its_incognito_sessions( + db_session: Session, owner: User +) -> None: + """A project lists sessions by title, which is the thing incognito hides.""" + project = UserProject(name="incognito-project", user_id=owner.id) + db_session.add(project) + db_session.commit() + + ordinary = _make_session( + db_session, owner.id, "ordinary chat", None, project_id=project.id + ) + incognito = _make_session( + db_session, + owner.id, + "incognito chat", + IncognitoRecordMode.FULL_HISTORY, + project_id=project.id, + ) + + db_session.expire(project) + listed = UserProjectSnapshot.from_model(project).chat_sessions + listed_ids = {session.id for session in listed} + assert ordinary.id in listed_ids + assert incognito.id not in listed_ids + assert all(session.name != "incognito chat" for session in listed) + + +def test_search_excludes_incognito_matching_only_in_a_message( + db_session: Session, owner: User +) -> None: + """The message-body arm of the union is filtered too. + + Both arms carry base_conditions, so a hit on message text must not surface + a session whose description never matched. + """ + incognito = _make_session( + db_session, owner.id, "untitled", IncognitoRecordMode.FULL_HISTORY + ) + ordinary = _make_session(db_session, owner.id, "untitled", None) + + for chat_session in (incognito, ordinary): + create_new_chat_message( + chat_session_id=chat_session.id, + parent_message=get_or_create_root_message( + chat_session_id=chat_session.id, db_session=db_session + ), + message="the aardvark budget is confidential", + token_count=7, + message_type=MessageType.USER, + db_session=db_session, + ) + db_session.commit() + + sessions, _ = search_chat_sessions( + user_id=owner.id, db_session=db_session, query="aardvark" + ) + returned_ids = {session.id for session in sessions} + # The ordinary control proves the query actually matches message bodies, + # so the incognito assertion is not passing for want of any hit at all. + assert ordinary.id in returned_ids + assert incognito.id not in returned_ids diff --git a/backend/tests/external_dependency_unit/redis/test_incognito_context.py b/backend/tests/external_dependency_unit/redis/test_incognito_context.py new file mode 100644 index 00000000000..42bffebf51e --- /dev/null +++ b/backend/tests/external_dependency_unit/redis/test_incognito_context.py @@ -0,0 +1,230 @@ +"""Guards the incognito context store's Redis contract. + +Round trip, the compare-and-set that guards against concurrent turns, the +sliding TTL, teardown, corruption degrading to an ended session, image +stripping, and the storage caps, all against a real Redis. Each test runs +under a unique tenant so runs cannot collide, mirroring test_tenant_redis.py. +""" + +import time +from collections.abc import Generator +from unittest.mock import patch +from uuid import UUID, uuid4 + +import pytest + +from onyx.cache.interface import CacheBackendType +from onyx.chat.incognito_context import ( + INCOGNITO_CONTEXT_TTL_SECONDS, + IncognitoContext, + _context_key, + incognito_context_available, + load_incognito_context, + save_incognito_context, + teardown_incognito_session, +) +from onyx.chat.models import ChatLoadedFile, ChatMessageSimple, ToolCallSimple +from onyx.configs.constants import MessageType +from onyx.file_store.models import ChatFileType +from onyx.redis.redis_pool import get_raw_redis_client, get_redis_client +from shared_configs.contextvars import CURRENT_TENANT_ID_CONTEXTVAR + + +@pytest.fixture(autouse=True) +def isolated_tenant() -> Generator[str, None, None]: + tenant = f"tenant_test_{uuid4().hex[:12]}" + token = CURRENT_TENANT_ID_CONTEXTVAR.set(tenant) + yield tenant + CURRENT_TENANT_ID_CONTEXTVAR.reset(token) + raw = get_raw_redis_client() + keys = list(raw.scan_iter(match=f"{tenant}:*")) + if keys: + raw.delete(*keys) + + +def _message( + text: str, message_type: MessageType = MessageType.USER +) -> ChatMessageSimple: + return ChatMessageSimple( + message=text, token_count=len(text), message_type=message_type + ) + + +def _save( + chat_session_id: UUID, messages: list[ChatMessageSimple], version: int = 0 +) -> bool: + return save_incognito_context( + chat_session_id, IncognitoContext(version=version, messages=messages) + ) + + +def test_missing_key_loads_empty_version_zero() -> None: + context = load_incognito_context(uuid4()) + assert context.messages == [] + assert context.version == 0 + + +def test_stale_version_save_is_discarded() -> None: + """A concurrent turn that loaded the same version must not roll the + winner's write back.""" + session_id = uuid4() + assert _save(session_id, [_message("turn one")], version=0) + + # A racing writer that also loaded version 0 loses. + assert not _save(session_id, [_message("stale rollback")], version=0) + + loaded = load_incognito_context(session_id) + assert loaded.version == 1 + assert loaded.messages[0].message == "turn one" + + +def test_sequential_turns_chain_versions() -> None: + session_id = uuid4() + assert _save(session_id, [_message("one")], version=0) + + first = load_incognito_context(session_id) + assert _save(session_id, first.messages + [_message("two")], first.version) + + second = load_incognito_context(session_id) + assert second.version == 2 + assert [m.message for m in second.messages] == ["one", "two"] + + +def test_corrupt_value_degrades_and_is_overwritable() -> None: + session_id = uuid4() + get_redis_client().set(_context_key(session_id), b"not json at all") + + context = load_incognito_context(session_id) + assert context.messages == [] + assert context.version == 0 + + # The load/save pair recovers: expecting version 0 overwrites the garbage. + assert _save(session_id, [_message("fresh start")], version=0) + assert load_incognito_context(session_id).messages[0].message == "fresh start" + + +def test_ttl_is_set_and_slides_on_save() -> None: + session_id = uuid4() + client = get_redis_client() + + assert _save(session_id, [_message("first")]) + ttl_after_first = client.ttl(_context_key(session_id)) + assert 0 < ttl_after_first <= INCOGNITO_CONTEXT_TTL_SECONDS + + time.sleep(2) + first = load_incognito_context(session_id) + assert _save(session_id, first.messages + [_message("second")], first.version) + ttl_after_second = client.ttl(_context_key(session_id)) + # A non-sliding TTL would have decayed by the sleep. A fresh save restarts it. + assert ttl_after_second > INCOGNITO_CONTEXT_TTL_SECONDS - 2 + + +def test_teardown_ends_the_context_and_fences_writers() -> None: + session_id = uuid4() + assert _save(session_id, [_message("secret plans")]) + context = load_incognito_context(session_id) + assert context.messages + + teardown_incognito_session(session_id) + + # Loads empty, and the tombstone refuses any save from an in-flight turn. + assert load_incognito_context(session_id).messages == [] + assert not _save(session_id, [_message("resurrected")]) + assert load_incognito_context(session_id).messages == [] + + +def test_images_are_stripped_before_storage() -> None: + """File bytes do not round-trip JSON, so save must drop them rather than + fail the turn or store binary content.""" + session_id = uuid4() + image = ChatLoadedFile( + file_id="f1", + content=b"\x89PNG\r\n", + file_type=ChatFileType.IMAGE, + filename="chart.png", + content_text=None, + token_count=0, + ) + message = ChatMessageSimple( + message="see attached", + token_count=100, + message_type=MessageType.USER, + image_files=[image], + image_token_count=85, + ) + + assert _save(session_id, [message]) + (loaded,) = load_incognito_context(session_id).messages + + assert loaded.image_files is None + assert loaded.image_token_count == 0 + assert loaded.message == "see attached" + + +def test_tool_calls_round_trip() -> None: + """Assistant tool calls and tool responses are part of history and must + survive storage intact.""" + session_id = uuid4() + call = ChatMessageSimple( + message="", + token_count=12, + message_type=MessageType.ASSISTANT, + tool_calls=[ + ToolCallSimple( + tool_call_id="call_1", + tool_name="run_search", + tool_arguments={"query": "churn", "limit": 5, "nested": {"a": [1]}}, + token_count=12, + ) + ], + ) + response = ChatMessageSimple( + message="3 documents found", + token_count=4, + message_type=MessageType.TOOL_CALL_RESPONSE, + tool_call_id="call_1", + ) + + assert _save(session_id, [call, response]) + loaded = load_incognito_context(session_id).messages + + assert loaded == [call, response] + + +def test_message_count_cap_keeps_the_newest() -> None: + session_id = uuid4() + history = [_message(f"m{i}") for i in range(205)] + + assert _save(session_id, history) + loaded = load_incognito_context(session_id).messages + + assert len(loaded) == 200 + assert loaded[0].message == "m5" + assert loaded[-1].message == "m204" + + +def test_byte_cap_drops_oldest_but_keeps_an_oversized_singleton() -> None: + session_id = uuid4() + big = "x" * 600_000 + oversized = "y" * 1_200_000 + + assert _save(session_id, [_message(big), _message(big + "newer")]) + loaded = load_incognito_context(session_id).messages + assert len(loaded) == 1 + assert loaded[0].message.endswith("newer") + + # One message alone over the cap is stored anyway: an empty save would + # read as session-ended on the next turn. + singleton_session = uuid4() + assert _save(singleton_session, [_message(oversized)]) + assert len(load_incognito_context(singleton_session).messages) == 1 + + +def test_availability_follows_the_cache_backend() -> None: + """USAGE_ONLY content must never reach Postgres, so the Postgres cache + backend (Lite) means the feature is absent.""" + with patch("onyx.chat.incognito_context.app_configs") as mock_configs: + mock_configs.CACHE_BACKEND = CacheBackendType.REDIS + assert incognito_context_available() + mock_configs.CACHE_BACKEND = CacheBackendType.POSTGRES + assert not incognito_context_available() diff --git a/backend/tests/unit/onyx/chat/test_incognito_record_mode.py b/backend/tests/unit/onyx/chat/test_incognito_record_mode.py new file mode 100644 index 00000000000..5372d984b1e --- /dev/null +++ b/backend/tests/unit/onyx/chat/test_incognito_record_mode.py @@ -0,0 +1,47 @@ +"""Guards the incognito recording policy: no mode other than FULL_HISTORY may +write conversation content, a corrupt mode value reads as the safe one, and a +content-free turn's persisted file descriptors keep linkage but not names. +""" + +from onyx.chat.incognito import ( + content_free_file_descriptors, + resolve_incognito_record_mode, +) +from onyx.db.enums import IncognitoRecordMode +from onyx.file_store.models import ChatFileType, FileDescriptor + + +def test_only_full_history_persists_content() -> None: + persisting = [m for m in IncognitoRecordMode if m.persists_content] + assert persisting == [IncognitoRecordMode.FULL_HISTORY] + + +def test_default_never_persists_content() -> None: + """A dropped admin setting must not silently start recording chats.""" + assert resolve_incognito_record_mode() is IncognitoRecordMode.USAGE_ONLY + assert resolve_incognito_record_mode().persists_content is False + + +def test_unknown_context_value_fails_closed() -> None: + """A corrupt contextvar must never read as content-persisting.""" + assert IncognitoRecordMode.from_context_value("garbage") is ( + IncognitoRecordMode.USAGE_ONLY + ) + assert IncognitoRecordMode.from_context_value(None) is None + + +def test_descriptors_strip_the_name_and_keep_linkage() -> None: + scrubbed = content_free_file_descriptors( + [ + FileDescriptor( + id="file-1", + type=ChatFileType.DOC, + name="acquisition_target.pdf", + user_file_id="uf-1", + ) + ] + ) + assert scrubbed == [ + FileDescriptor(id="file-1", type=ChatFileType.DOC, user_file_id="uf-1") + ] + assert "name" not in scrubbed[0] diff --git a/backend/tests/unit/onyx/chat/test_multi_model_streaming.py b/backend/tests/unit/onyx/chat/test_multi_model_streaming.py index 88d556b546f..72b4a4e942d 100644 --- a/backend/tests/unit/onyx/chat/test_multi_model_streaming.py +++ b/backend/tests/unit/onyx/chat/test_multi_model_streaming.py @@ -276,8 +276,9 @@ def _make_setup(n_models: int = 1) -> MagicMock: setup.available_files.chat_file_ids = [] setup.forced_tool_id = None setup.simple_chat_history = [] - setup.chat_session.id = uuid4() - setup.user_message.id = None + setup.chat_session_id = uuid4() + setup.chat_session_project_id = None + setup.user_message_id = None setup.custom_tool_additional_headers = None setup.mcp_headers = None return setup diff --git a/backend/tests/unit/onyx/connectors/braintrust/__init__.py b/backend/tests/unit/onyx/connectors/braintrust/__init__.py deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/backend/tests/unit/onyx/tracing/test_braintrust_incognito_suppression.py b/backend/tests/unit/onyx/tracing/test_braintrust_incognito_suppression.py new file mode 100644 index 00000000000..9284e0a6c4a --- /dev/null +++ b/backend/tests/unit/onyx/tracing/test_braintrust_incognito_suppression.py @@ -0,0 +1,80 @@ +"""Incognito turns must leave no content in Braintrust: the processor drops +the whole trace, spans included, keyed on membership recorded at trace start.""" + +from collections.abc import Generator +from unittest.mock import MagicMock + +import pytest + +from onyx.tracing.braintrust_tracing_processor import BraintrustTracingProcessor +from shared_configs.contextvars import CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR + + +@pytest.fixture +def incognito_context() -> Generator[None, None, None]: + token = CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR.set("usage_only") + yield + CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR.reset(token) + + +def _fake_trace(trace_id: str) -> MagicMock: + trace = MagicMock() + trace.trace_id = trace_id + trace.name = "run_llm_loop" + trace.export.return_value = {} + return trace + + +def _fake_span(trace_id: str, span_id: str) -> MagicMock: + span = MagicMock() + span.trace_id = trace_id + span.span_id = span_id + span.parent_id = None + return span + + +def test_incognito_trace_is_fully_suppressed( + incognito_context: None, # noqa: ARG001 (requested for the flag side-effect) +) -> None: + logger = MagicMock() + processor = BraintrustTracingProcessor(logger=logger) + trace = _fake_trace("t1") + span = _fake_span("t1", "s1") + + processor.on_trace_start(trace) + processor.on_span_start(span) + processor.on_span_end(span) + processor.on_trace_end(trace) + + logger.start_span.assert_not_called() + assert processor._spans == {} + assert processor._suppressed_traces == set() + + +def test_suppression_holds_even_if_flag_clears_mid_trace() -> None: + """Membership at trace start decides, so a reset contextvar cannot leak + the tail of an incognito trace.""" + logger = MagicMock() + processor = BraintrustTracingProcessor(logger=logger) + trace = _fake_trace("t1") + span = _fake_span("t1", "s1") + + token = CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR.set("usage_only") + processor.on_trace_start(trace) + CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR.reset(token) + + processor.on_span_start(span) + processor.on_span_end(span) + processor.on_trace_end(trace) + + logger.start_span.assert_not_called() + assert processor._suppressed_traces == set() + + +def test_regular_trace_still_logs() -> None: + logger = MagicMock() + processor = BraintrustTracingProcessor(logger=logger) + + processor.on_trace_start(_fake_trace("t2")) + + logger.start_span.assert_called_once() diff --git a/backend/tests/unit/onyx/tracing/test_langfuse_incognito_suppression.py b/backend/tests/unit/onyx/tracing/test_langfuse_incognito_suppression.py new file mode 100644 index 00000000000..e47221c9066 --- /dev/null +++ b/backend/tests/unit/onyx/tracing/test_langfuse_incognito_suppression.py @@ -0,0 +1,60 @@ +"""Incognito turns must leave no content in Langfuse: the processor drops the +whole trace, spans included, keyed on membership recorded at trace start.""" + +from collections.abc import Generator +from unittest.mock import MagicMock + +import pytest + +from onyx.tracing.langfuse_tracing_processor import LangfuseTracingProcessor +from shared_configs.contextvars import CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR + + +@pytest.fixture +def incognito_context() -> Generator[None, None, None]: + token = CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR.set("usage_only") + yield + CURRENT_INCOGNITO_RECORD_MODE_CONTEXTVAR.reset(token) + + +def _fake_trace(trace_id: str) -> MagicMock: + trace = MagicMock() + trace.trace_id = trace_id + trace.name = "run_llm_loop" + trace.export.return_value = {} + return trace + + +def _fake_span(trace_id: str, span_id: str) -> MagicMock: + span = MagicMock() + span.trace_id = trace_id + span.span_id = span_id + span.parent_id = None + return span + + +def test_incognito_trace_is_fully_suppressed( + incognito_context: None, # noqa: ARG001 (requested for the flag side-effect) +) -> None: + client = MagicMock() + processor = LangfuseTracingProcessor(client=client) + trace = _fake_trace("t1") + span = _fake_span("t1", "s1") + + processor.on_trace_start(trace) + processor.on_span_start(span) + processor.on_span_end(span) + processor.on_trace_end(trace) + + client.start_observation.assert_not_called() + assert processor._suppressed_traces == set() + + +def test_ordinary_trace_still_exports() -> None: + client = MagicMock() + processor = LangfuseTracingProcessor(client=client) + trace = _fake_trace("t2") + + processor.on_trace_start(trace) + + client.start_observation.assert_called_once() From 4185d413ff486678d56bac0c539898a0625a0646 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:41:35 +0000 Subject: [PATCH 03/19] chore(deps): bump github.com/go-git/go-git/v5 from 5.19.1 to 5.19.2 in /tools/ods (#13818) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- tools/ods/go.mod | 20 ++++++++++---------- tools/ods/go.sum | 40 ++++++++++++++++++++-------------------- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/tools/ods/go.mod b/tools/ods/go.mod index a0cd05321de..ccfa2c66d62 100644 --- a/tools/ods/go.mod +++ b/tools/ods/go.mod @@ -70,7 +70,7 @@ require ( github.com/go-errors/errors v1.5.1 // indirect github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect github.com/go-git/go-billy/v5 v5.9.0 // indirect - github.com/go-git/go-git/v5 v5.19.1 // indirect + github.com/go-git/go-git/v5 v5.19.2 // indirect github.com/go-logr/logr v1.4.3 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/go-ole/go-ole v1.3.0 // indirect @@ -146,16 +146,16 @@ require ( go.uber.org/zap v1.28.0 // indirect go.yaml.in/yaml/v2 v2.4.4 // indirect go.yaml.in/yaml/v4 v4.0.0-rc.4 // indirect - golang.org/x/crypto v0.52.0 // indirect - golang.org/x/mod v0.36.0 // indirect - golang.org/x/net v0.55.0 // indirect + golang.org/x/crypto v0.53.0 // indirect + golang.org/x/mod v0.37.0 // indirect + golang.org/x/net v0.56.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect - golang.org/x/sync v0.20.0 // indirect - golang.org/x/sys v0.45.0 // indirect - golang.org/x/telemetry v0.0.0-20260508192327-42602be52be6 // indirect - golang.org/x/term v0.43.0 // indirect - golang.org/x/text v0.37.0 // indirect - golang.org/x/tools v0.45.0 // indirect + golang.org/x/sync v0.21.0 // indirect + golang.org/x/sys v0.46.0 // indirect + golang.org/x/telemetry v0.0.0-20260625142307-59b4966ccb57 // indirect + golang.org/x/term v0.44.0 // indirect + golang.org/x/text v0.39.0 // indirect + golang.org/x/tools v0.47.0 // indirect golang.org/x/vuln v1.3.0 // indirect golang.org/x/xerrors v0.0.0-20240903120638-7835f813f4da // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260511170946-3700d4141b60 // indirect diff --git a/tools/ods/go.sum b/tools/ods/go.sum index 65fb68ce051..ae2ca7ba3b3 100644 --- a/tools/ods/go.sum +++ b/tools/ods/go.sum @@ -167,8 +167,8 @@ github.com/go-git/go-billy/v5 v5.9.0 h1:jItGXszUDRtR/AlferWPTMN4j38BQ88XnXKbilmm github.com/go-git/go-billy/v5 v5.9.0/go.mod h1:jCnQMLj9eUgGU7+ludSTYoZL/GGmii14RxKFj7ROgHw= github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399 h1:eMje31YglSBqCdIqdhKBW8lokaMrL3uTkpGYlE2OOT4= github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399/go.mod h1:1OCfN199q1Jm3HZlxleg+Dw/mwps2Wbk9frAWm+4FII= -github.com/go-git/go-git/v5 v5.19.1 h1:nX27AnaU43/K5bKktKwgBmR9lawoYVe1Ckg0rgzzN00= -github.com/go-git/go-git/v5 v5.19.1/go.mod h1:Pb1v0c7/g8aGQJwx9Us09W85yGoyvSwuhEGMH7zjDKQ= +github.com/go-git/go-git/v5 v5.19.2 h1:wkfn7vOlUBu8ivAWKBWisTiwJK4jYHzTF8Ndv1LyGqY= +github.com/go-git/go-git/v5 v5.19.2/go.mod h1:QqCBE1EFN5ddFmrliLQ3/ntRCUjZU3EJuwuB/jWEHjk= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= @@ -450,8 +450,8 @@ golang.org/x/crypto v0.13.0/go.mod h1:y6Z2r+Rw4iayiXXAIxJIDAJ1zMW4yaTpebo8fPOliY golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/crypto v0.23.0/go.mod h1:CKFgDieR+mRhux2Lsu27y0fO304Db0wZe70UKqHu0v8= golang.org/x/crypto v0.33.0/go.mod h1:bVdXmD7IV/4GdElGPozy6U7lWdRXA4qyRVGJV57uQ5M= -golang.org/x/crypto v0.52.0 h1:RMs7fP2rXdep0CftQlK8Uf+kibLm7qkCcradZWYz988= -golang.org/x/crypto v0.52.0/go.mod h1:1QgfPxDqh0T2M/elOJtp9RvuR95kVjir0e6/BvEmGbc= +golang.org/x/crypto v0.53.0 h1:QZ4Muo8THX6CizN2vPPd5fBGHyogrdK9fG4wLPFUsto= +golang.org/x/crypto v0.53.0/go.mod h1:DNLU434OwVakk9PzuwV8w62mAJpRJL3vsgcfp4Qnsio= golang.org/x/exp v0.0.0-20260508232706-74f9aab9d74a h1:+3jdDGGB8NGb1Zktc737jlt3/A5f6UlwSzmvqUuufxw= golang.org/x/exp v0.0.0-20260508232706-74f9aab9d74a/go.mod h1:d2fgXJLVs4dYDHUk5lwMIfzRzSrWCfGZb0ZqeLa/Vcw= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= @@ -461,8 +461,8 @@ golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.12.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.15.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/mod v0.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/mod v0.36.0 h1:JJjpVx6myfUsUdAzZuOSTTmRE0PfZeNWzzvKrP7amb4= -golang.org/x/mod v0.36.0/go.mod h1:moc6ELqsWcOw5Ef3xVprK5ul/MvtVvkIXLziUOICjUQ= +golang.org/x/mod v0.37.0 h1:vF1DjpVEshcIqoEaauuHebaLk1O1forxjxBaVn884JQ= +golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20190827160401-ba9fcec4b297/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= @@ -477,8 +477,8 @@ golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= golang.org/x/net v0.15.0/go.mod h1:idbUs1IY1+zTqbi8yxTbhexhEEk5ur9LInksu6HrEpk= golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44= golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= -golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= -golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= +golang.org/x/net v0.56.0 h1:Rw8j/hFzGvJUZwNBXnAtf5sVDVt+65SK2C7IxCxZt5o= +golang.org/x/net v0.56.0/go.mod h1:D3Ku6r+V6JROoZK144D2XfMHFcMq/0zSfLelVTCFKec= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= @@ -490,8 +490,8 @@ golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sync v0.11.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= -golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= -golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= +golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20190916202348-b4ddaad3f8a3/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -515,11 +515,11 @@ golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.30.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= -golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= +golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/telemetry v0.0.0-20240228155512-f48c80bd79b2/go.mod h1:TeRTkGYfJXctD9OcfyVLyj2J3IxLnKwHJR8f4D8a3YE= -golang.org/x/telemetry v0.0.0-20260508192327-42602be52be6 h1:HjU6IWBiAgRIdAJ9/y1rwCn+UELEmwV+VsTLzj/W4sE= -golang.org/x/telemetry v0.0.0-20260508192327-42602be52be6/go.mod h1:Eqhaxk/wZsWEH8CRxLwj6xzEJbz7k1EFGqx7nyCoabE= +golang.org/x/telemetry v0.0.0-20260625142307-59b4966ccb57 h1:nwGZBCt+FnXUrGsj5vjzAsEmkcaFvd82BbOjECiFYZc= +golang.org/x/telemetry v0.0.0-20260625142307-59b4966ccb57/go.mod h1:3AWMyWHS+caVoiEXpiq6+tzKA40J4vQT3MYr80ZtQpc= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k= @@ -528,8 +528,8 @@ golang.org/x/term v0.12.0/go.mod h1:owVbMEjm3cBLCHdkQu9b1opXd4ETQWc3BhuQGKgXgvU= golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= golang.org/x/term v0.20.0/go.mod h1:8UkIAJTvZgivsXaD6/pH6U9ecQzZ45awqEOzuCvwpFY= golang.org/x/term v0.29.0/go.mod h1:6bl4lRlvVuDgSf3179VpIxBF0o10JUpXWOnI7nErv7s= -golang.org/x/term v0.43.0 h1:S4RLU2sB31O/NCl+zFN9Aru9A/Cq2aqKpTZJ6B+DwT4= -golang.org/x/term v0.43.0/go.mod h1:lrhlHNdQJHO+1qVYiHfFKVuVioJIheAc3fBSMFYEIsk= +golang.org/x/term v0.44.0 h1:0rLvDRCtNj0gZkyIXhCyOb2OAzEhLVqc4B+hrsBhrmc= +golang.org/x/term v0.44.0/go.mod h1:7ze4MdzUzLXpSAoFP1H0bOI9aXDqveSvatT5vKcFh2Y= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= @@ -540,8 +540,8 @@ golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.22.0/go.mod h1:YRoo4H8PVmsu+E3Ou7cqLVH8oXWIHVoX0jqUWALQhfY= -golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc= -golang.org/x/text v0.37.0/go.mod h1:a5sjxXGs9hsn/AJVwuElvCAo9v8QYLzvavO5z2PiM38= +golang.org/x/text v0.39.0 h1:UbZz4pLOvn600D6Oh6GGEI6VAmndrEBLv8/6BEXzyus= +golang.org/x/text v0.39.0/go.mod h1:3UwRclnC2g0TU9x8PZiyfOajCd1zaUNHF9cvqcQZ+ZM= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= @@ -552,8 +552,8 @@ golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= golang.org/x/tools v0.13.0/go.mod h1:HvlwmtVNQAhOuCjW7xxvovg8wbNq7LwfXh/k7wXUl58= golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d/go.mod h1:aiJjzUbINMkxbQROHiO6hDPo2LHcIPhhQsa9DLh0yGk= -golang.org/x/tools v0.45.0 h1:18qN3FAooORvApf5XjCXgsuayZOEtXf6JK18I3+ONa8= -golang.org/x/tools v0.45.0/go.mod h1:LuUGqqaXcXMEFEruIVJVm5mgDD8vww/z/SR1gQ4uE/0= +golang.org/x/tools v0.47.0 h1:7Kn5x/d1svx/PzryTsqeoZN4TZwqeH5pGWjefhLi/1Q= +golang.org/x/tools v0.47.0/go.mod h1:dFHnyTvFWY212G+h7ZY4Vsp/K3U4/7W9TyVaAul8uCA= golang.org/x/tools/go/expect v0.1.1-deprecated h1:jpBZDwmgPhXsKZC6WhL20P4b/wmnpsEAGHaNy0n/rJM= golang.org/x/tools/go/expect v0.1.1-deprecated/go.mod h1:eihoPOH+FgIqa3FpoTwguz/bVUSGBlGQU67vpBeOrBY= golang.org/x/tools/go/packages/packagestest v0.1.1-deprecated h1:1h2MnaIAIXISqTFKdENegdpAgUXz6NrPEsbIeWaBRvM= From eb12c2a3b5348c1d2bba44e5771ee8119799e701 Mon Sep 17 00:00:00 2001 From: acaprau <48705707+acaprau@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:53:55 +0000 Subject: [PATCH 04/19] fix(ods): detect cherry-pick target from release branches, not tags (#13880) --- tools/ods/cmd/cherry-pick.go | 141 ++++++++++++++++++--- tools/ods/cmd/cherry-pick_test.go | 189 +++++++++++++++++++++++++++++ tools/ods/internal/git/git.go | 34 ++++++ tools/ods/internal/git/git_test.go | 22 ++++ 4 files changed, 368 insertions(+), 18 deletions(-) create mode 100644 tools/ods/cmd/cherry-pick_test.go diff --git a/tools/ods/cmd/cherry-pick.go b/tools/ods/cmd/cherry-pick.go index d38b430a5e9..40d4d2378cf 100644 --- a/tools/ods/cmd/cherry-pick.go +++ b/tools/ods/cmd/cherry-pick.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "regexp" + "sort" "strconv" "strings" @@ -45,7 +46,8 @@ with fewer than 6 digits is treated as a PR number and resolved to its merge commit automatically. This command will: - 1. Find the nearest stable version tag + 1. Detect the newest release branch that does not already contain the commit + (unless --release is given) 2. Fetch the corresponding release branch(es) 3. Create a hotfix branch with the cherry-picked commit(s) 4. Push and create a PR using the GitHub CLI @@ -172,11 +174,11 @@ func runCherryPick(cmd *cobra.Command, args []string, opts *CherryPickOptions) { } log.Debugf("Using specified release versions: %v", releases) } else { - // Find the nearest stable tag using the first commit - version, err := findNearestStableTag(commitSHAs[0]) + // Find the newest release branch missing the first commit. + version, err := findTargetReleaseVersion(commitSHAs[0]) if err != nil { git.RestoreStash(stashResult) - log.Fatalf("Failed to find nearest stable tag: %v", err) + log.Fatalf("Failed to auto-detect the target release: %v", err) } // Prompt user for confirmation @@ -388,7 +390,7 @@ func cherryPickToRelease(commitSHAs, commitMessages []string, branchSuffix, vers // Fetch the release branch log.Infof("Fetching release branch: %s", releaseBranch) - if err := git.RunCommand("fetch", "--prune", "--quiet", "origin", releaseBranch); err != nil { + if err := git.RunCommand("fetch", "--prune", "--quiet", "origin", releaseBranchRefspec(releaseBranch)); err != nil { return "", fmt.Errorf("failed to fetch release branch %s: %w", releaseBranch, err) } @@ -570,26 +572,129 @@ func extractPRNumbers(commitMsg string) []string { return matches } -// findNearestStableTag finds the nearest tag matching v*.*.* pattern and returns major.minor -func findNearestStableTag(commitSHA string) (string, error) { - // Get tags that are ancestors of the commit, sorted by version - cmd := exec.Command("git", "describe", "--tags", "--abbrev=0", "--match", "v*.*.*", commitSHA) +// releaseBranchPattern matches maintained release branch names, e.g. +// "release/v4.5". Ad-hoc branches such as "release/v3.0-qa-f1df36e" are +// deliberately excluded. +var releaseBranchPattern = regexp.MustCompile(`^release/v(\d+)\.(\d+)$`) + +// releaseBranchRefspec returns a forced fetch refspec that creates or updates +// the origin/ tracking ref even in clones whose configured fetch +// refspec does not cover release branches (e.g. single-branch clones), where a +// plain "git fetch origin " only writes FETCH_HEAD. +func releaseBranchRefspec(releaseBranch string) string { + return fmt.Sprintf("+refs/heads/%s:refs/remotes/origin/%s", releaseBranch, releaseBranch) +} + +// releaseVersion is the parsed version of a "release/vX.Y" branch. +type releaseVersion struct { + major int + minor int +} + +// String returns the version with its 'v' prefix, e.g. "v4.5". +func (v releaseVersion) String() string { + return fmt.Sprintf("v%d.%d", v.major, v.minor) +} + +// parseReleaseVersions extracts "release/vX.Y" versions from branch names and +// returns them sorted newest first. Names that do not match the pattern are +// ignored. +func parseReleaseVersions(branchNames []string) []releaseVersion { + versions := []releaseVersion{} + for _, name := range branchNames { + matches := releaseBranchPattern.FindStringSubmatch(name) + if matches == nil { + continue + } + major, err := strconv.Atoi(matches[1]) + if err != nil { + continue + } + minor, err := strconv.Atoi(matches[2]) + if err != nil { + continue + } + versions = append(versions, releaseVersion{major: major, minor: minor}) + } + sort.Slice(versions, func(i, j int) bool { + if versions[i].major != versions[j].major { + return versions[i].major > versions[j].major + } + return versions[i].minor > versions[j].minor + }) + return versions +} + +// listRemoteReleaseBranches returns the names (e.g. "release/v4.5") of all +// release branches on origin. +func listRemoteReleaseBranches() ([]string, error) { + cmd := exec.Command("git", "ls-remote", "--heads", "origin", "release/*") output, err := cmd.Output() if err != nil { - return "", fmt.Errorf("git describe failed: %w", err) + if exitErr, ok := err.(*exec.ExitError); ok { + return nil, fmt.Errorf("git ls-remote failed: %w: %s", err, string(exitErr.Stderr)) + } + return nil, fmt.Errorf("git ls-remote failed: %w", err) } - tag := strings.TrimSpace(string(output)) - log.Debugf("Found tag: %s", tag) + branches := []string{} + for _, line := range strings.Split(string(output), "\n") { + // Each line is "\trefs/heads/". + _, ref, found := strings.Cut(line, "\t") + if !found { + continue + } + branches = append(branches, strings.TrimPrefix(strings.TrimSpace(ref), "refs/heads/")) + } + return branches, nil +} + +// findTargetReleaseVersion returns the version (e.g. "v4.5") of the newest +// "release/vX.Y" branch on origin that does not already contain commitSHA. A +// commit merged to main after the latest branch cut targets the newest branch; +// a commit that predates the cut (and is therefore already part of the newer +// branches) falls back to the newest branch actually missing it. Tags are +// deliberately not consulted: release tag names on main (e.g. "vX.Y.0-cloud.N") +// roll over to a new version asynchronously from the branch cut, so the nearest +// tag can disagree with the newest branch. +func findTargetReleaseVersion(commitSHA string) (string, error) { + // A shallow clone cannot answer ancestry truthfully: history beyond the + // shallow boundary makes contained commits look uncontained, silently + // routing them to the wrong branch. Fail loudly instead. + shallow, err := git.IsShallowRepository() + if err != nil { + return "", err + } + if shallow { + return "", fmt.Errorf("this is a shallow clone, so release auto-detection cannot check branch ancestry; pass --release explicitly") + } - // Extract major.minor with v prefix from tag (e.g., v1.2.3 -> v1.2) - re := regexp.MustCompile(`^(v\d+\.\d+)\.\d+`) - matches := re.FindStringSubmatch(tag) - if len(matches) < 2 { - return "", fmt.Errorf("tag %s does not match expected format v*.*.* ", tag) + branchNames, err := listRemoteReleaseBranches() + if err != nil { + return "", err + } + versions := parseReleaseVersions(branchNames) + if len(versions) == 0 { + return "", fmt.Errorf("no release/vX.Y branches found on origin") + } + + for _, version := range versions { + releaseBranch := fmt.Sprintf("release/%s", version) + // Fetch so the ancestry check runs against the branch's current tip. + if err := git.RunCommand("fetch", "--quiet", "origin", releaseBranchRefspec(releaseBranch)); err != nil { + return "", fmt.Errorf("failed to fetch %s: %w", releaseBranch, err) + } + contained, err := git.IsAncestor(commitSHA, fmt.Sprintf("origin/%s", releaseBranch)) + if err != nil { + return "", err + } + if !contained { + return version.String(), nil + } + log.Infof("Commit %s is already contained in %s, checking the next older release branch", commitSHA, releaseBranch) } - return matches[1], nil + return "", fmt.Errorf("commit %s is already contained in every release branch; pass --release explicitly", commitSHA) } // createCherryPickPR creates a pull request for cherry-picks using the GitHub CLI diff --git a/tools/ods/cmd/cherry-pick_test.go b/tools/ods/cmd/cherry-pick_test.go new file mode 100644 index 00000000000..e1239390da8 --- /dev/null +++ b/tools/ods/cmd/cherry-pick_test.go @@ -0,0 +1,189 @@ +package cmd + +import ( + "os" + "os/exec" + "path/filepath" + "slices" + "strings" + "testing" +) + +func TestParseReleaseVersions_sortsNewestFirstIgnoringNonMatching(t *testing.T) { + // Precondition. + branchNames := []string{ + "release/v4.4", + "release/v3.0-qa-f1df36e", + "release/v4.10", + "main", + "release/v4.5", + "release/v10.0", + } + + // Under test. + versions := parseReleaseVersions(branchNames) + + // Postcondition. + got := make([]string, len(versions)) + for i, version := range versions { + got[i] = version.String() + } + want := []string{"v10.0", "v4.10", "v4.5", "v4.4"} + if !slices.Equal(got, want) { + t.Errorf("expected %v, got %v", want, got) + } +} + +func TestParseReleaseVersions_emptyWhenNothingMatches(t *testing.T) { + // Under test and postcondition. + if versions := parseReleaseVersions([]string{"main", "hotfix/abc-v4.4"}); len(versions) != 0 { + t.Errorf("expected no versions, got %v", versions) + } +} + +// gitIn runs a git command in dir, failing the test on error. +func gitIn(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s failed: %v\n%s", strings.Join(args, " "), err, out) + } + return strings.TrimSpace(string(out)) +} + +// commitIn creates a file and commits it in dir, returning the commit SHA. +func commitIn(t *testing.T, dir, filename string) string { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, filename), []byte(filename), 0644); err != nil { + t.Fatal(err) + } + gitIn(t, dir, "add", filename) + gitIn(t, dir, "commit", "-m", "add "+filename) + return gitIn(t, dir, "rev-parse", "HEAD") +} + +// setupReleaseBranchRepo creates a bare origin holding main, release/v4.4, and +// release/v4.5, with a local work repo as the current directory. It returns +// three main-line commit SHAs: the v4.4 cut point (ancestor of both release +// branches), the v4.5 cut point (only on release/v4.5), and a post-cut commit +// (on neither release branch). +func setupReleaseBranchRepo(t *testing.T) (preCutSHA, cutSHA, postCutSHA string) { + t.Helper() + + origin := t.TempDir() + gitIn(t, origin, "init", "--bare", "-b", "main") + + work := t.TempDir() + gitIn(t, work, "init", "-b", "main") + gitIn(t, work, "config", "user.email", "test@test.com") + gitIn(t, work, "config", "user.name", "Test") + gitIn(t, work, "config", "commit.gpgsign", "false") + gitIn(t, work, "remote", "add", "origin", origin) + // Narrow the fetch refspec to main only, like a single-branch clone, so the + // tests also pin that detection fetches release branches with an explicit + // refspec (a plain "git fetch origin " would only write FETCH_HEAD + // here and never create origin/release/vX.Y). + gitIn(t, work, "config", "remote.origin.fetch", "+refs/heads/main:refs/remotes/origin/main") + + preCutSHA = commitIn(t, work, "a.txt") + gitIn(t, work, "branch", "release/v4.4", preCutSHA) + cutSHA = commitIn(t, work, "b.txt") + gitIn(t, work, "branch", "release/v4.5", cutSHA) + postCutSHA = commitIn(t, work, "c.txt") + + gitIn(t, work, "push", "--quiet", "origin", "main", "release/v4.4", "release/v4.5") + + // Tags named after the previous release must not influence detection + // (tag-anchored detection was the original misrouting bug). Mirror the + // incident topology: a stable v4.4 tag at the v4.4 cut point, plus a v4.4 + // pre-release tag minted on main after the v4.5 cut, which is the exact + // shape that misrouted real cherry-picks to release/v4.4. + gitIn(t, work, "tag", "v4.4.2", preCutSHA) + gitIn(t, work, "tag", "v4.4.0-cloud.9", postCutSHA) + + // Drop the local release branches and the remote-tracking refs the push + // created, so the fixture looks like a clone that has never fetched the + // release branches; detection must create origin/* itself via fetch. + gitIn(t, work, "branch", "-D", "release/v4.4", "release/v4.5") + gitIn(t, work, "update-ref", "-d", "refs/remotes/origin/release/v4.4") + gitIn(t, work, "update-ref", "-d", "refs/remotes/origin/release/v4.5") + + // The functions under test run git in the process working directory. + t.Chdir(work) + + return preCutSHA, cutSHA, postCutSHA +} + +func TestFindTargetReleaseVersion_postCutCommitTargetsNewestBranch(t *testing.T) { + // Precondition. + _, _, postCutSHA := setupReleaseBranchRepo(t) + + // Under test. + version, err := findTargetReleaseVersion(postCutSHA) + + // Postcondition. + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if version != "v4.5" { + t.Errorf("expected v4.5, got %s", version) + } +} + +func TestFindTargetReleaseVersion_preCutCommitFallsBackToOlderBranch(t *testing.T) { + // Precondition. + _, cutSHA, _ := setupReleaseBranchRepo(t) + + // Under test. + version, err := findTargetReleaseVersion(cutSHA) + + // Postcondition. + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if version != "v4.4" { + t.Errorf("expected v4.4, got %s", version) + } +} + +func TestFindTargetReleaseVersion_commitOnAllBranchesErrors(t *testing.T) { + // Precondition. + preCutSHA, _, _ := setupReleaseBranchRepo(t) + + // Under test. + _, err := findTargetReleaseVersion(preCutSHA) + + // Postcondition. + if err == nil || !strings.Contains(err.Error(), "already contained in every release branch") { + t.Errorf("expected already-contained error, got %v", err) + } +} + +func TestFindTargetReleaseVersion_shallowCloneErrors(t *testing.T) { + // Precondition: a shallow clone, where ancestry cannot be answered. + origin := t.TempDir() + gitIn(t, origin, "init", "--bare", "-b", "main") + seed := t.TempDir() + gitIn(t, seed, "init", "-b", "main") + gitIn(t, seed, "config", "user.email", "test@test.com") + gitIn(t, seed, "config", "user.name", "Test") + gitIn(t, seed, "config", "commit.gpgsign", "false") + gitIn(t, seed, "remote", "add", "origin", origin) + sha := commitIn(t, seed, "a.txt") + gitIn(t, seed, "branch", "release/v4.5") + gitIn(t, seed, "push", "--quiet", "origin", "main", "release/v4.5") + shallow := filepath.Join(t.TempDir(), "shallow") + // Depth flags are ignored for plain local-path clones, hence file://. + gitIn(t, t.TempDir(), "clone", "--quiet", "--depth", "1", "--no-single-branch", "file://"+origin, shallow) + t.Chdir(shallow) + + // Under test. + _, err := findTargetReleaseVersion(sha) + + // Postcondition. + if err == nil || !strings.Contains(err.Error(), "shallow clone") { + t.Errorf("expected shallow-clone error, got %v", err) + } +} diff --git a/tools/ods/internal/git/git.go b/tools/ods/internal/git/git.go index d20a4bae707..96a4735f7ff 100644 --- a/tools/ods/internal/git/git.go +++ b/tools/ods/internal/git/git.go @@ -2,6 +2,7 @@ package git import ( "encoding/json" + "errors" "fmt" "os" "os/exec" @@ -118,6 +119,39 @@ func RestoreStash(result *StashResult) { } } +// IsAncestor reports whether ancestor is an ancestor of (or equal to) +// descendant. Both arguments may be any commit-ish. +func IsAncestor(ancestor, descendant string) (bool, error) { + cmd := exec.Command("git", "merge-base", "--is-ancestor", ancestor, descendant) + var stderr strings.Builder + cmd.Stderr = &stderr + err := cmd.Run() + if err == nil { + return true, nil + } + // Exit code 1 is the documented "not an ancestor" result; anything else + // (e.g. an unknown revision) is a real error. + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + return false, nil + } + if diagnostic := strings.TrimSpace(stderr.String()); diagnostic != "" { + return false, fmt.Errorf("git merge-base --is-ancestor %s %s failed: %w: %s", ancestor, descendant, err, diagnostic) + } + return false, fmt.Errorf("git merge-base --is-ancestor %s %s failed: %w", ancestor, descendant, err) +} + +// IsShallowRepository reports whether the current repository is a shallow +// clone. +func IsShallowRepository() (bool, error) { + cmd := exec.Command("git", "rev-parse", "--is-shallow-repository") + output, err := cmd.Output() + if err != nil { + return false, fmt.Errorf("git rev-parse --is-shallow-repository failed: %w", err) + } + return strings.TrimSpace(string(output)) == "true", nil +} + // CommitExistsOnBranch checks if a commit exists on a branch func CommitExistsOnBranch(commitSHA, branchName string) bool { cmd := exec.Command("git", "branch", "--contains", commitSHA, "--list", branchName) diff --git a/tools/ods/internal/git/git_test.go b/tools/ods/internal/git/git_test.go index 156039a1902..0ef696c51a1 100644 --- a/tools/ods/internal/git/git_test.go +++ b/tools/ods/internal/git/git_test.go @@ -182,3 +182,25 @@ func TestIsCommitAppliedOnBranch_NoFalsePositiveFromBody(t *testing.T) { t.Error("should NOT match when subject only appears in body of another commit") } } + +// --- IsAncestor tests --- + +func TestIsAncestor_distinguishesFalseFromError(t *testing.T) { + // Precondition. + r := newTestRepo(t) + first := r.HEAD() + second := r.Commit("second commit", "second.txt", "content") + + // Under test and postcondition: ancestor, non-ancestor, and error cases. + contained, err := IsAncestor(first, second) + if err != nil || !contained { + t.Errorf("expected (true, nil) for ancestor, got (%v, %v)", contained, err) + } + contained, err = IsAncestor(second, first) + if err != nil || contained { + t.Errorf("expected (false, nil) for non-ancestor, got (%v, %v)", contained, err) + } + if _, err = IsAncestor("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", first); err == nil { + t.Error("expected an error for an unknown revision, got nil") + } +} From f624117dce69432adb4fcbe545ee667b02c91e45 Mon Sep 17 00:00:00 2001 From: Raunak Bhagat Date: Wed, 12 Aug 2026 18:29:06 +0000 Subject: [PATCH 05/19] refactor(opal): give spacing a numeric scale (#13909) --- .../opal/src/components/modal/components.tsx | 12 ++-- web/lib/opal/src/components/tabs/README.md | 4 +- .../opal/src/components/tabs/Tabs.stories.tsx | 2 +- .../opal/src/components/tabs/components.tsx | 13 +++-- web/lib/opal/src/layouts/general/README.md | 11 ++-- .../opal/src/layouts/general/components.tsx | 15 +++-- .../opal/src/layouts/inputs/components.tsx | 4 +- web/lib/opal/src/shared.ts | 13 +++++ web/lib/opal/src/types.ts | 18 ++++++ .../app/admin/billing/BillingDetailsView.tsx | 27 ++++----- web/src/app/admin/billing/CheckoutView.tsx | 16 +++--- .../admin/billing/LicenseActivationCard.tsx | 16 +++--- web/src/app/admin/billing/PlansView.tsx | 19 +++---- web/src/app/admin/billing/page.tsx | 2 +- .../connector/[ccPairId]/ConfigDisplay.tsx | 8 +-- .../DocPermissionSyncAttemptsTable.tsx | 2 +- .../ExternalGroupSyncAttemptsTable.tsx | 2 +- .../[ccPairId]/IndexAttemptsTable.tsx | 9 +-- .../stage-metrics/AttemptOverhead.tsx | 6 +- .../[ccPairId]/stage-metrics/AvgTimeCell.tsx | 2 +- .../stage-metrics/PerBatchSection.tsx | 2 +- .../[ccPairId]/stage-metrics/SortToggle.tsx | 2 +- .../stage-metrics/StageLabelCell.tsx | 2 +- .../stage-metrics/StageMetricsPanel.tsx | 2 +- .../[connector]/AddConnectorPage.tsx | 2 +- .../ConnectorInput/StringPairListInput.tsx | 8 +-- .../[connector]/pages/FieldRendering.tsx | 6 +- .../[connector]/pages/gdrive/Credential.tsx | 8 +-- .../[connector]/pages/gmail/Credential.tsx | 8 +-- .../app/admin/discord-bot/BotConfigCard.tsx | 8 +-- .../[guild-id]/DiscordChannelsTable.tsx | 2 +- .../app/admin/discord-bot/[guild-id]/page.tsx | 2 +- web/src/app/admin/scim/ScimModal.tsx | 2 +- web/src/app/admin/scim/ScimSyncCard.tsx | 4 +- .../CreateRateLimitModal.tsx | 2 +- .../TokenRateLimitTables.tsx | 2 +- .../TokenRateLimitsPanel.tsx | 2 +- web/src/app/app/components/WelcomeMessage.tsx | 4 +- .../timeline/headers/CompletedHeader.tsx | 4 +- .../filereader/FileReaderToolRenderer.tsx | 6 +- web/src/app/app/settings/layout.tsx | 2 +- .../app/app/settings/usage/UsageSettings.tsx | 24 ++++---- .../app/shared/[chatId]/SharedChatDisplay.tsx | 4 +- .../app/craft/components/BuildLLMPopover.tsx | 2 +- web/src/app/craft/components/ShareButton.tsx | 10 ++-- .../app/craft/components/UserLibraryModal.tsx | 2 +- .../components/output-panel/ArtifactsTab.tsx | 2 +- .../output-panel/FilePreviewContent.tsx | 8 +-- .../components/output-panel/FilesTab.tsx | 8 +-- .../components/output-panel/ImagePreview.tsx | 2 +- .../components/output-panel/PdfPreview.tsx | 4 +- .../components/output-panel/PptxPreview.tsx | 6 +- .../components/output-panel/PreviewTab.tsx | 4 +- .../onboarding/components/LivingMapModal.tsx | 2 +- web/src/app/craft/v1/apps/page.tsx | 2 +- .../v1/tasks/components/RunHistoryTable.tsx | 4 +- .../v1/tasks/components/ScheduleEditor.tsx | 6 +- web/src/app/craft/v1/tasks/page.tsx | 2 +- .../query-history/QueryHistoryTable.tsx | 2 +- web/src/ee/sections/SearchCard.tsx | 10 ++-- web/src/ee/sections/SearchUI.tsx | 2 +- .../views/admin/HooksPage/HookFormModal.tsx | 2 +- .../views/admin/HooksPage/HookLogsModal.tsx | 8 +-- .../admin/HooksPage/HookStatusPopover.tsx | 16 +++--- web/src/layouts/chromes/AdminChrome.tsx | 2 +- web/src/layouts/general-layouts.tsx | 9 +-- .../refresh-components/buttons/LineItem.tsx | 4 +- web/src/refresh-components/cards/Card.tsx | 2 +- .../commandmenu/CommandMenu.tsx | 14 +---- .../form/InputTypeInElementField.tsx | 2 +- .../inputs/InputDatePicker.tsx | 4 +- .../modals/MemoriesModal.tsx | 12 ++-- .../ActionsPopover/ActionLineItem.tsx | 2 +- .../popovers/ActionsPopover/MCPLineItem.tsx | 2 +- .../actions/modals/AddMCPServerModal.tsx | 8 +-- .../actions/modals/AddOpenAPIActionModal.tsx | 10 ++-- .../sections/admin/LiteModeIndexingNotice.tsx | 2 +- web/src/sections/admin/ProviderCard.tsx | 6 +- web/src/sections/banners/BannerQueue.tsx | 12 ++-- .../sections/knowledge/AgentKnowledgePane.tsx | 6 +- .../knowledge/SourceHierarchyBrowser.tsx | 16 +++--- .../agent-knowledge/KnowledgeAddView.tsx | 4 +- .../agent-knowledge/KnowledgeSearch.tsx | 12 ++-- .../agent-knowledge/KnowledgeTable.tsx | 6 +- .../agent-knowledge/KnowledgeTableContent.tsx | 4 +- .../KnowledgeTwoColumnView.tsx | 2 +- web/src/sections/modals/AgentViewerModal.tsx | 10 ++-- .../modals/PreviewModal/PreviewModal.tsx | 2 +- .../PreviewModal/variants/csvVariant.tsx | 2 +- .../PreviewModal/variants/docxVariant.tsx | 4 +- .../PreviewModal/variants/xlsxVariant.tsx | 2 +- .../sections/modals/ShareChatSessionModal.tsx | 2 +- web/src/sections/modals/SkillPreviewModal.tsx | 4 +- web/src/sections/modals/UserFilesModal.tsx | 8 +-- .../modals/languageModels/BedrockModal.tsx | 6 +- .../modals/languageModels/CustomModal.tsx | 4 +- .../modals/languageModels/VertexAIModal.tsx | 2 +- .../sections/modals/languageModels/shared.tsx | 10 ++-- .../model-selector/ModelSelectorContent.tsx | 16 ++---- .../components/OnboardingHeader.tsx | 2 +- .../sections/onboarding/steps/FinalStep.tsx | 4 +- web/src/sections/sidebar/AccountPopover.tsx | 2 +- .../sections/sidebar/NotificationsPopover.tsx | 8 +-- web/src/sections/usage/SpendByUserTable.tsx | 2 +- .../sections/usage/UserUsageDetailModal.tsx | 12 ++-- web/src/views/AgentEditorPage.tsx | 29 +++++----- web/src/views/AppPage.tsx | 2 +- web/src/views/SettingsPage.tsx | 56 +++++++++---------- web/src/views/SkillEditorPage.tsx | 8 +-- .../views/admin/AgentsPage/AgentsTable.tsx | 4 +- web/src/views/admin/ChatPreferencesPage.tsx | 24 ++++---- .../views/admin/CodeInterpreterPage/index.tsx | 10 ++-- web/src/views/admin/CostOverridesPanel.tsx | 4 +- .../admin/CraftInstructionsPage/index.tsx | 2 +- web/src/views/admin/CraftPage/index.tsx | 4 +- .../admin/GroupsPage/CreateGroupPage.tsx | 6 +- .../views/admin/GroupsPage/EditGroupPage.tsx | 8 +-- web/src/views/admin/GroupsPage/GroupCard.tsx | 2 +- web/src/views/admin/GroupsPage/GroupsList.tsx | 2 +- .../SharedGroupResources/ResourcePopover.tsx | 4 +- .../GroupsPage/SharedGroupResources/index.tsx | 14 ++--- .../admin/GroupsPage/TokenLimitSection.tsx | 2 +- .../ImageGenerationContent.tsx | 2 +- .../views/admin/IndexSettingsPage/index.tsx | 34 +++++------ .../views/admin/IndexSettingsPage/modals.tsx | 2 +- web/src/views/admin/LanguageModelsPage.tsx | 6 +- web/src/views/admin/PerUserUsagePanel.tsx | 12 ++-- .../EditServiceAccountModal.tsx | 4 +- .../views/admin/ServiceAccountsPage/index.tsx | 2 +- .../TracingPage/TracingDisconnectModal.tsx | 2 +- .../views/admin/UsersPage/EditUserModal.tsx | 4 +- .../views/admin/UsersPage/InviteOnlyCard.tsx | 2 +- .../views/admin/UsersPage/UserRowActions.tsx | 2 +- .../views/admin/UsersPage/UsersSummary.tsx | 6 +- web/src/views/admin/VoicePage/index.tsx | 10 ++-- web/src/views/admin/VoicePage/shared.tsx | 4 +- .../WebSearchDisconnectModal.tsx | 2 +- 137 files changed, 464 insertions(+), 467 deletions(-) diff --git a/web/lib/opal/src/components/modal/components.tsx b/web/lib/opal/src/components/modal/components.tsx index 6095da86e9b..eedfab1c8bb 100644 --- a/web/lib/opal/src/components/modal/components.tsx +++ b/web/lib/opal/src/components/modal/components.tsx @@ -359,13 +359,13 @@ function ModalHeader({ ); return ( -
+
{closeButton}
@@ -416,7 +416,7 @@ function ModalBody({ className="opal-modal-body" {...(twoTone && { "data-two-tone": "" })} > -
+
{children}
@@ -433,8 +433,8 @@ function ModalFooter({ ref, ...props }: ModalFooterProps) { ref={ref} flexDirection="row" justifyContent="end" - gap={0.5} - padding={1} + gap={2} + padding={4} height="fit" {...props} /> @@ -469,7 +469,7 @@ function BasicModalFooter({ left, cancel, submit }: BasicModalFooterProps) { <> {left &&
{left}
} {(cancel || submit) && ( -
+
{cancel} {submit}
diff --git a/web/lib/opal/src/components/tabs/README.md b/web/lib/opal/src/components/tabs/README.md index 3f49b556187..5023f5c8cf8 100644 --- a/web/lib/opal/src/components/tabs/README.md +++ b/web/lib/opal/src/components/tabs/README.md @@ -109,7 +109,7 @@ When tabs overflow the available width, show navigation arrows: ### Content padding ```tsx - + Padded content ``` @@ -150,4 +150,4 @@ Forwards all [Radix Tabs.Root](https://www.radix-ui.com/docs/primitives/componen | Prop | Type | Default | Description | |---|---|---|---| | `value` | `string` | **required** | Must match a `Tabs.Trigger` value | -| `padding` | `number` | `0` | Additional inner padding in rem units | +| `padding` | `Spacing` | `0` | Additional inner padding, as a spacing step (`N / 4` rem) | diff --git a/web/lib/opal/src/components/tabs/Tabs.stories.tsx b/web/lib/opal/src/components/tabs/Tabs.stories.tsx index efc41c70d5b..c0b0388e349 100644 --- a/web/lib/opal/src/components/tabs/Tabs.stories.tsx +++ b/web/lib/opal/src/components/tabs/Tabs.stories.tsx @@ -190,7 +190,7 @@ export const ContentPadding: Story = { Padded Flush - +
Inner content with 1rem padding
diff --git a/web/lib/opal/src/components/tabs/components.tsx b/web/lib/opal/src/components/tabs/components.tsx index 41cf5935256..a48b5871c8b 100644 --- a/web/lib/opal/src/components/tabs/components.tsx +++ b/web/lib/opal/src/components/tabs/components.tsx @@ -4,7 +4,12 @@ import "@opal/components/tabs/styles.css"; import React, { useRef, useState, useEffect, useMemo } from "react"; import * as TabsPrimitive from "@radix-ui/react-tabs"; import { mergeRefs } from "@opal/utils"; -import { IconFunctionComponent, type WithoutStyles } from "@opal/types"; +import { + IconFunctionComponent, + type Spacing, + type WithoutStyles, +} from "@opal/types"; +import { spacingToRem } from "@opal/shared"; import { SvgChevronLeft, SvgChevronRight } from "@opal/icons"; import { Tooltip, Text, Button } from "@opal/components"; import { @@ -283,15 +288,15 @@ function TabsTrigger({ interface TabsContentProps extends WithoutStyles< React.ComponentProps > { - /** Additional inner padding in rem. @default 0 */ - padding?: number; + /** Additional inner padding, as a {@link Spacing} step (`N / 4` rem). @default 0 */ + padding?: Spacing; } function TabsContent({ padding, children, ...props }: TabsContentProps) { return ( {padding ? ( -
{children}
+
{children}
) : ( children )} diff --git a/web/lib/opal/src/layouts/general/README.md b/web/lib/opal/src/layouts/general/README.md index 60e6d755978..ca3b761f616 100644 --- a/web/lib/opal/src/layouts/general/README.md +++ b/web/lib/opal/src/layouts/general/README.md @@ -4,7 +4,10 @@ A flexbox container primitive for grouping related content. Configurable direction, alignment, spacing, and dimensions. Defaults to a full-width / full-height column with centered children -and a 1rem gap. +and a gap of `4` (1rem). + +`gap` and `padding` are spacing steps, not raw lengths: `N` is `N / 4` rem, the same scale +Tailwind uses. So `gap={2}` is the same distance as `gap-2`. ## Props @@ -15,8 +18,8 @@ and a 1rem gap. | `alignItems` | `"start" \| "center" \| "end" \| "stretch"` | `"center"` | Cross-axis alignment | | `width` | `"auto" \| "fit" \| "full" \| number` | `"full"` | Width. `number` = rem. | | `height` | `"auto" \| "fit" \| "full" \| number` | `"full"` | Height. `number` = rem. | -| `gap` | `number` | `1` | Gap between children, in rem | -| `padding` | `number` | `0` | Padding, in rem | +| `gap` | `Spacing` | `4` | Gap between children, as a spacing step (`N / 4` rem) | +| `padding` | `Spacing` | `0` | Padding, as a spacing step (`N / 4` rem) | | `wrap` | `boolean` | `false` | Enables `flex-wrap` | | `dbg` | `boolean` | `false` | Adds a red debug border | | `className` | `string` | — | Additional classes | @@ -40,7 +43,7 @@ import { Section } from "@opal/layouts";
// Tighter gap, custom width -
+
One Two
diff --git a/web/lib/opal/src/layouts/general/components.tsx b/web/lib/opal/src/layouts/general/components.tsx index 30ca086885a..84ff26961d0 100644 --- a/web/lib/opal/src/layouts/general/components.tsx +++ b/web/lib/opal/src/layouts/general/components.tsx @@ -2,7 +2,8 @@ import React from "react"; import { cn } from "@opal/utils"; -import type { WithoutStyles } from "@opal/types"; +import type { Spacing, WithoutStyles } from "@opal/types"; +import { spacingToRem } from "@opal/shared"; type FlexDirection = "row" | "column"; type JustifyContent = "start" | "center" | "end" | "between"; @@ -46,8 +47,10 @@ interface SectionProps extends WithoutStyles< width?: Length; height?: Length; - gap?: number; - padding?: number; + /** Spacing between children, as a {@link Spacing} step (`N / 4` rem). @default 4 */ + gap?: Spacing; + /** Inner padding, as a {@link Spacing} step (`N / 4` rem). @default 0 */ + padding?: Spacing; wrap?: boolean; ref?: React.Ref; @@ -60,7 +63,7 @@ function Section({ alignItems = "center", width = "full", height = "full", - gap = 1, + gap = 4, padding = 0, wrap, ref, @@ -83,8 +86,8 @@ function Section({ className )} style={{ - gap: `${gap}rem`, - padding: `${padding}rem`, + gap: spacingToRem(gap), + padding: spacingToRem(padding), ...(typeof width === "number" && { width: `${width}rem` }), ...(typeof height === "number" && { height: `${height}rem` }), }} diff --git a/web/lib/opal/src/layouts/inputs/components.tsx b/web/lib/opal/src/layouts/inputs/components.tsx index 854397f6185..de888cc033f 100644 --- a/web/lib/opal/src/layouts/inputs/components.tsx +++ b/web/lib/opal/src/layouts/inputs/components.tsx @@ -120,7 +120,7 @@ function Vertical({ ); const content = ( -
+
{titleRow} {children} {fieldName && } @@ -182,7 +182,7 @@ function Horizontal({ typeof withLabelProp === "string" ? withLabelProp : undefined; const content = ( -
+
= { fit: "py-0", }; +/** + * Converts a spacing step to a CSS length: `N` is `N / 4` rem. + * + * Kept as a function rather than a class lookup so the scale stays open — + * Tailwind cannot build a class name from a runtime value, but arithmetic can. + */ +function spacingToRem(spacing: Spacing): string { + return `${spacing / 4}rem`; +} + const cardRoundingVariants: Record = { xl: "rounded-20", lg: "rounded-16", @@ -176,7 +187,9 @@ export { type ContainerSizeVariants, type OverridableExtremaSizeVariants, type SizeVariants, + type Spacing, containerSizeVariants, + spacingToRem, paddingVariants, paddingXVariants, paddingYVariants, diff --git a/web/lib/opal/src/types.ts b/web/lib/opal/src/types.ts index 9ce49ad3793..8f90c1354b6 100644 --- a/web/lib/opal/src/types.ts +++ b/web/lib/opal/src/types.ts @@ -85,6 +85,24 @@ export type RoundingVariants = Extract< */ export type ExtremaSizeVariants = Extract; +// --------------------------------------------------------------------------- +// Spacing Scale +// --------------------------------------------------------------------------- + +/** + * A spacing step. `N` is `N / 4` rem, so `4` is `1rem` and `2` is `0.5rem`. + * + * This borrows Tailwind's scale as an interface, not as an implementation — a + * step reads the same here as in a class name, so a `padding` of `2` is the same + * distance as `p-2`. The value is converted with {@link spacingToRem} rather + * than looked up as a class, which keeps the scale open: any step works, + * including ones Tailwind does not ship. + * + * Replaces the named scales. `PaddingVariants` meant one distance on a card and + * a different one on a container; a number cannot be ambiguous that way. + */ +export type Spacing = number; + /** * Shadow depth variants. * diff --git a/web/src/app/admin/billing/BillingDetailsView.tsx b/web/src/app/admin/billing/BillingDetailsView.tsx index 5b4b1cd88f9..bb119abfb2e 100644 --- a/web/src/app/admin/billing/BillingDetailsView.tsx +++ b/web/src/app/admin/billing/BillingDetailsView.tsx @@ -299,7 +299,7 @@ function SubscriptionCard({ alignItems="start" height="auto" > -
+
{planName} @@ -310,7 +310,7 @@ function SubscriptionCard({
@@ -547,7 +547,7 @@ function SeatsCard({ flexDirection="row" alignItems="center" justifyContent="between" - padding={1} + padding={4} height="auto" > {isAdding ? ( @@ -595,7 +595,7 @@ function SeatsCard({ alignItems="center" height="auto" > -
+
{totalSeats} Seats @@ -606,7 +606,7 @@ function SeatsCard({
Payment -
+
+
{/* Renewal fetched on arrival while expired. The page renders regardless: billing is the one route a lapsed instance must always reach. */} {isGraceSyncing && ( diff --git a/web/src/app/admin/billing/CheckoutView.tsx b/web/src/app/admin/billing/CheckoutView.tsx index b896c6c0897..bd58c2ec557 100644 --- a/web/src/app/admin/billing/CheckoutView.tsx +++ b/web/src/app/admin/billing/CheckoutView.tsx @@ -42,7 +42,7 @@ function BillingOption({ >
@@ -185,8 +185,8 @@ export default function CheckoutView({ onAdjustPlan }: CheckoutViewProps) {
{/* Billing Cycle */} @@ -197,7 +197,7 @@ export default function CheckoutView({ onAdjustPlan }: CheckoutViewProps) { >
{error ? ( diff --git a/web/src/app/admin/billing/LicenseActivationCard.tsx b/web/src/app/admin/billing/LicenseActivationCard.tsx index 65619575587..9579f3f223c 100644 --- a/web/src/app/admin/billing/LicenseActivationCard.tsx +++ b/web/src/app/admin/billing/LicenseActivationCard.tsx @@ -86,7 +86,7 @@ export default function LicenseActivationCard({ // License status view (when license exists and not editing) if (hasLicense && !showInput) { return ( - +
@@ -118,7 +118,7 @@ export default function LicenseActivationCard({ )}
-
+
@@ -137,7 +137,7 @@ export default function LicenseActivationCard({ return ( {/* Header */} -
+
{success && (
@@ -197,7 +197,7 @@ export default function LicenseActivationCard({ flexDirection="row" alignItems="center" justifyContent="start" - gap={0.25} + gap={1} height="auto" >
@@ -221,7 +221,7 @@ export default function LicenseActivationCard({
{/* Footer */} -
+