fix(diagnostics): address round-6 review findings on PR #445

- AdminDiagnostics list: fix regression where rows dereferenced the
  now-omitted manifest for app_build. Project app_build server-side out
  of manifest JSONB into both list and detail responses (cheap
  COALESCE(manifest->'report'->>'app_build','')), split the TS type into
  DiagnosticReportSummary (list, no manifest) and DiagnosticReport
  (detail, with manifest), and read report.app_build in the row/detail.
- embeddedManifestMatches: decode with json.Decoder + UseNumber so large
  integers above 2^53 (e.g. log_summary.lines) can't collapse to the same
  float and falsely match; re-assert no-trailing-data strictness.
- Quota reservation (SKIP): reserving the client-claimed archive.bytes is
  sound because archiveMatches requires claimed==actual before MarkReady,
  so no stored report exceeds its reservation; documented in a code comment.
- Multipart parts: reject a wrong-name/wrong-content-type part without
  calling part.Close(), which would drain up to the bundle limit while
  holding the in-flight slot; abandon it so malformed uploads fail promptly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012e3QjbPo96ed9Mn2qRiUkh

internal/api/handlers/diagnostics.go

@@ -220,12 +220,16 @@ func nextDiagnosticsPart(mr *multipart.Reader, expectedName, expectedContentType
 	if err != nil {
 		return nil, err
 	}
+	// Reject a mismatched part without calling part.Close(): Close drains the
+	// unread part body first, so a max-size wrongly-named/typed first part would
+	// stream up to the bundle limit — holding the per-user/global in-flight slot —
+	// before we return 400. Abandoning the part returns immediately; net/http then
+	// discards only a small bounded prefix of the request before closing the
+	// connection, so malformed uploads fail promptly under load.
 	if part.FormName() != expectedName {
-		_ = part.Close()
 		return nil, errDiagnosticsUnexpectedPart
 	}
 	if !diagnosticsContentTypeMatches(part.Header.Get("Content-Type"), expectedContentType) {
-		_ = part.Close()
 		return nil, errDiagnosticsUnexpectedPart
 	}
 	return part, nil

internal/api/handlers/diagnostics_test.go

@@ -85,6 +85,60 @@ func TestDiagnosticsUploadWrongPartOrder(t *testing.T) {
 	assertDiagnosticsRejectionLog(t, logs, "invalid_bundle", 42)
 }
 
+func TestDiagnosticsUploadWrongFirstPartNotDrained(t *testing.T) {
+	service := newFakeDiagnosticsService()
+	handler := NewDiagnosticsHandler(service)
+
+	// A large, wrongly-named first part must be rejected without draining its
+	// body: an invalid upload must not be allowed to stream its whole payload
+	// (holding the in-flight slot) before it gets a 400.
+	var body bytes.Buffer
+	writer := multipart.NewWriter(&body)
+	header := make(textproto.MIMEHeader)
+	header.Set("Content-Disposition", `form-data; name="bundle"`)
+	header.Set("Content-Type", "application/gzip")
+	w, err := writer.CreatePart(header)
+	if err != nil {
+		t.Fatalf("create multipart part: %v", err)
+	}
+	if _, err := w.Write(bytes.Repeat([]byte("x"), 1<<20)); err != nil {
+		t.Fatalf("write multipart part: %v", err)
+	}
+	if err := writer.Close(); err != nil {
+		t.Fatalf("close multipart writer: %v", err)
+	}
+
+	counter := &countingReader{r: &body}
+	req := httptest.NewRequest(http.MethodPost, "/api/v1/diagnostics/reports", counter)
+	req.Header.Set("Content-Type", writer.FormDataContentType())
+	req.Header.Set("Authorization", "Bearer token")
+	req = req.WithContext(apimw.SetClaims(req.Context(), accessClaims()))
+
+	rec := httptest.NewRecorder()
+	handler.HandleUpload(rec, req)
+
+	assertDiagnosticsError(t, rec, http.StatusBadRequest, "invalid_bundle")
+	if service.ingestCalls != 0 {
+		t.Fatalf("ingest calls = %d, want 0", service.ingestCalls)
+	}
+	// Only the small multipart framing/headers should have been read; the 1 MiB
+	// wrong-first-part body must not have been drained.
+	if counter.n > 128*1024 {
+		t.Fatalf("read %d bytes from request body, want the wrong first part rejected without draining (< 128 KiB)", counter.n)
+	}
+}
+
+type countingReader struct {
+	r io.Reader
+	n int64
+}
+
+func (c *countingReader) Read(p []byte) (int, error) {
+	n, err := c.r.Read(p)
+	c.n += int64(n)
+	return n, err
+}
+
 func TestDiagnosticsUploadBodyTooLargeLogsRejection(t *testing.T) {
 	service := newFakeDiagnosticsService()
 	service.status.MaxBundleBytes = 1

internal/diagnostics/repo.go

@@ -297,7 +297,8 @@ func (r *PostgresRepository) DeleteByID(ctx context.Context, id string) (*Report
 		WHERE id = $1::uuid
 		RETURNING id::text, short_id, user_id, profile_id, state, captured_at, received_at,
 		          report_type, platform, app_version, crash_summary, manifest, playback_session_ids,
-		          blob_bucket, blob_key, blob_bytes, uncompressed_bytes, blob_sha256
+		          blob_bucket, blob_key, blob_bytes, uncompressed_bytes, blob_sha256,
+		          COALESCE(manifest->'report'->>'app_build', '')
 	`, id)
 	report, err := scanReport(row)
 	if err != nil {
@@ -472,17 +473,21 @@ func validateInsertReceivingInput(input InsertReceivingInput) error {
 func reportSelectSQL() string {
 	return `SELECT id::text, short_id, user_id, profile_id, state, captured_at, received_at,
 		       report_type, platform, app_version, crash_summary, manifest, playback_session_ids,
-		       blob_bucket, blob_key, blob_bytes, uncompressed_bytes, blob_sha256
+		       blob_bucket, blob_key, blob_bytes, uncompressed_bytes, blob_sha256,
+		       COALESCE(manifest->'report'->>'app_build', '')
 		FROM client_diagnostic_reports`
 }
 
 // reportListSelectSQL mirrors reportSelectSQL but omits the manifest column.
 // Reports scanned with scanReportSummary therefore have a nil Manifest; the
-// list and cleanup callers only need summary and blob fields.
+// list and cleanup callers only need summary and blob fields. It still projects
+// app_build out of the manifest JSONB (a cheap text extraction) so list rows can
+// show the build number without shipping the whole manifest.
 func reportListSelectSQL() string {
 	return `SELECT id::text, short_id, user_id, profile_id, state, captured_at, received_at,
 		       report_type, platform, app_version, crash_summary, playback_session_ids,
-		       blob_bucket, blob_key, blob_bytes, uncompressed_bytes, blob_sha256
+		       blob_bucket, blob_key, blob_bytes, uncompressed_bytes, blob_sha256,
+		       COALESCE(manifest->'report'->>'app_build', '')
 		FROM client_diagnostic_reports`
 }
 
@@ -553,6 +558,7 @@ func scanReport(row reportScanner) (*Report, error) {
 		&nulls.blobBytes,
 		&nulls.uncompressedBytes,
 		&nulls.blobSHA256,
+		&report.AppBuild,
 	); err != nil {
 		return nil, fmt.Errorf("scan diagnostic report: %w", err)
 	}
@@ -586,6 +592,7 @@ func scanReportSummary(row reportScanner) (*Report, error) {
 		&nulls.blobBytes,
 		&nulls.uncompressedBytes,
 		&nulls.blobSHA256,
+		&report.AppBuild,
 	); err != nil {
 		return nil, fmt.Errorf("scan diagnostic report: %w", err)
 	}

internal/diagnostics/service.go

@@ -1,6 +1,7 @@
 package diagnostics
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -181,6 +182,17 @@ func (s *Service) Ingest(ctx context.Context, userID int, profileID *string, man
 		return IngestResult{}, err
 	}
 	crashSummary := crashSummaryFromManifest(manifest)
+	// ExpectedBlobBytes reserves the client-claimed archive size against the
+	// per-user byte quota before the bundle streams in. Trusting the claim here is
+	// safe: a report only reaches MarkReady (which rewrites blob_bytes to the
+	// measured size) after archiveMatches confirms the claimed bytes/sha/entries
+	// exactly equal the streamed bundle, so no stored report can ever occupy more
+	// bytes than it reserved. A client that under-claims to under-reserve then
+	// uploads a larger bundle fails archiveMatches and is compensated (blob
+	// deleted, row marked failed) before it counts, gaining no storage. The
+	// residual cost of such rejected churn is bandwidth, not storage — bounded by
+	// the per-account single in-flight upload cap, not by max_bytes_per_user,
+	// which is a storage cap enforced oldest-first by retention.
 	reserved, err := s.reports.InsertReceiving(ctx, InsertReceivingInput{
 		UserID:               userID,
 		ProfileID:            reportProfileID,
@@ -536,9 +548,21 @@ func embeddedManifestMatches(received, embedded []byte) bool {
 }
 
 func decodeJSONObject(data []byte) (map[string]any, error) {
+	dec := json.NewDecoder(bytes.NewReader(data))
+	// UseNumber keeps every JSON number as its exact decimal text (json.Number)
+	// instead of float64. Without it, two manifests that differ only in a large
+	// integer above 2^53 (e.g. log_summary.lines) both round to the same float
+	// and DeepEqual reports a false match, letting a tampered bundle through.
+	dec.UseNumber()
 	var obj map[string]any
-	if err := json.Unmarshal(data, &obj); err != nil {
+	if err := dec.Decode(&obj); err != nil {
 		return nil, err
 	}
+	// json.Unmarshal rejects trailing bytes after the value; Decoder.Decode does
+	// not, so re-assert that strictness — a manifest with trailing junk is not a
+	// clean single object and must not compare equal.
+	if dec.More() {
+		return nil, errors.New("unexpected trailing data after JSON object")
+	}
 	return obj, nil
 }

internal/diagnostics/service_test.go

@@ -160,6 +160,26 @@ func TestServiceIngestRejectsEmbeddedManifestMismatch(t *testing.T) {
 	}
 }
 
+func TestEmbeddedManifestMatchesPreservesLargeIntegers(t *testing.T) {
+	// The received (part-1) and embedded manifests differ only in
+	// log_summary.lines by 1, at a magnitude above 2^53 where float64 cannot
+	// represent adjacent integers. Decoding with UseNumber must keep them distinct
+	// so a bundle whose embedded manifest was tampered is rejected, not accepted.
+	received := []byte(`{"log_summary":{"lines":9007199254740993},"archive":{"bytes":1}}`)
+	embedded := []byte(`{"log_summary":{"lines":9007199254740992}}`)
+	if embeddedManifestMatches(received, embedded) {
+		t.Fatal("embeddedManifestMatches = true for manifests differing by a large integer, want false")
+	}
+
+	// Sanity: with the archive object stripped from the received manifest, an
+	// otherwise identical large integer still compares equal.
+	sameReceived := []byte(`{"log_summary":{"lines":9007199254740993},"archive":{"bytes":1}}`)
+	sameEmbedded := []byte(`{"log_summary":{"lines":9007199254740993}}`)
+	if !embeddedManifestMatches(sameReceived, sameEmbedded) {
+		t.Fatal("embeddedManifestMatches = false for identical manifests, want true")
+	}
+}
+
 func TestServiceIngestReturnsStorageErrorForMidStreamPutFailure(t *testing.T) {
 	bundle, info := testDiagnosticsBundle(t)
 	putErr := errors.New("s3: connection reset by peer")

internal/diagnostics/types.go

@@ -71,16 +71,20 @@ type BlobInfo struct {
 }
 
 type Report struct {
-	ID                 string          `json:"id"`
-	ShortID            string          `json:"short_id"`
-	UserID             int             `json:"user_id"`
-	ProfileID          *string         `json:"profile_id,omitempty"`
-	State              ReportState     `json:"state"`
-	CapturedAt         time.Time       `json:"captured_at"`
-	ReceivedAt         time.Time       `json:"received_at"`
-	ReportType         string          `json:"report_type"`
-	Platform           string          `json:"platform"`
-	AppVersion         string          `json:"app_version"`
+	ID         string      `json:"id"`
+	ShortID    string      `json:"short_id"`
+	UserID     int         `json:"user_id"`
+	ProfileID  *string     `json:"profile_id,omitempty"`
+	State      ReportState `json:"state"`
+	CapturedAt time.Time   `json:"captured_at"`
+	ReceivedAt time.Time   `json:"received_at"`
+	ReportType string      `json:"report_type"`
+	Platform   string      `json:"platform"`
+	AppVersion string      `json:"app_version"`
+	// AppBuild is projected from manifest->'report'->>'app_build' so list rows
+	// (which omit the full manifest) can still show the build number without
+	// dragging every report's manifest JSONB through the DB. Empty when absent.
+	AppBuild           string          `json:"app_build"`
 	CrashSummary       *string         `json:"crash_summary,omitempty"`
 	Manifest           json.RawMessage `json:"manifest,omitempty"`
 	PlaybackSessionIDs []string        `json:"playback_session_ids"`

web/src/api/types.ts

@@ -2554,7 +2554,10 @@ export interface ClientDiagnosticManifest {
   [key: string]: unknown;
 }
 
-export interface DiagnosticReport {
+// DiagnosticReportSummary is the shape returned by the admin list endpoint,
+// which omits the full manifest JSONB for cheap paging. `app_build` is projected
+// out of the manifest server-side so rows can still show the build number.
+export interface DiagnosticReportSummary {
   id: string;
   short_id: string;
   user_id: number;
@@ -2565,8 +2568,8 @@ export interface DiagnosticReport {
   report_type: DiagnosticReportType;
   platform: DiagnosticPlatform;
   app_version: string;
+  app_build: string;
   crash_summary?: string;
-  manifest: ClientDiagnosticManifest;
   playback_session_ids: string[];
   blob_bucket?: string;
   blob_key?: string;
@@ -2575,8 +2578,14 @@ export interface DiagnosticReport {
   blob_sha256?: string;
 }
 
+// DiagnosticReport is the full detail shape (GetByID), which additionally
+// includes the parsed manifest for the detail pane.
+export interface DiagnosticReport extends DiagnosticReportSummary {
+  manifest: ClientDiagnosticManifest;
+}
+
 export interface DiagnosticReportListResponse {
-  reports: DiagnosticReport[];
+  reports: DiagnosticReportSummary[];
   next_cursor?: string;
 }
 

web/src/hooks/queries/admin/diagnostics.ts

@@ -5,6 +5,7 @@ import { api, apiResponse } from "@/api/client";
 import type {
   DiagnosticReport,
   DiagnosticReportListResponse,
+  DiagnosticReportSummary,
   DiagnosticStatus,
 } from "@/api/types";
 import { adminKeys } from "@/hooks/queries/keys";
@@ -75,7 +76,7 @@ export function useDeleteDiagnosticReport() {
   });
 }
 
-export async function downloadDiagnosticReport(report: DiagnosticReport) {
+export async function downloadDiagnosticReport(report: DiagnosticReportSummary) {
   // Always stream the bundle through the server (proxy mode) instead of
   // following a presigned URL. When S3Private points at an endpoint only the
   // server can reach (an internal MinIO/R2 gateway), a presigned URL sends the

web/src/pages/AdminDiagnostics.tsx

@@ -4,7 +4,7 @@ import { Link, useSearchParams } from "react-router";
 import { Bug, Download, ExternalLink, FilterX, Trash2, TriangleAlert } from "lucide-react";
 import { toast } from "sonner";
 
-import type { DiagnosticReport, DiagnosticReportState } from "@/api/types";
+import type { DiagnosticReport, DiagnosticReportState, DiagnosticReportSummary } from "@/api/types";
 import { ConfirmDialog } from "@/components/ConfirmDialog";
 import { Badge } from "@/components/ui/badge";
 import { Button } from "@/components/ui/button";
@@ -417,7 +417,7 @@ function DiagnosticReportRow({
   report,
   onSelect,
 }: {
-  report: DiagnosticReport;
+  report: DiagnosticReportSummary;
   onSelect: (id: string) => void;
 }) {
   function handleKeyDown(event: KeyboardEvent<HTMLTableRowElement>) {
@@ -449,7 +449,7 @@ function DiagnosticReportRow({
         <div>{formatPlatform(report.platform)}</div>
         <div className="text-muted-foreground text-xs">
           v{report.app_version}
-          {report.manifest.report.app_build ? ` (${report.manifest.report.app_build})` : ""}
+          {report.app_build ? ` (${report.app_build})` : ""}
         </div>
       </TableCell>
       <TableCell>
@@ -484,7 +484,7 @@ function DiagnosticReportDetail({
   onDelete: () => void;
 }) {
   const device = report.manifest.device_summary;
-  const appBuild = report.manifest.report.app_build;
+  const appBuild = report.app_build;
 
   return (
     <div className="space-y-6">