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

- Extend the upload write deadline alongside the read deadline so a slow
  upload finishing after the integrated server's 120s WriteTimeout can still
  return its success response instead of timing out a report that succeeded.
- Reject child-profile attribution for diagnostics: wire the attribution
  validator through a shared profile lookup that reports IsChild and drop
  attribution for child profiles, which must not perform diagnostics actions.
- Assert the download test captures the clicked anchor and checks its blob:
  href and silo-diagnostics-<short_id>.tar.gz filename, not just cleanup.

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

internal/api/handlers/diagnostics.go

@@ -82,6 +82,17 @@ func (h *DiagnosticsHandler) HandleUpload(w http.ResponseWriter, r *http.Request
 			"error", err,
 		)
 	}
+	// Extend the write deadline the same way. cmd/silo's integrated server sets
+	// WriteTimeout 120s, but a slow upload can take longer than that within the
+	// read window above; Ingest may store and mark the report ready and then the
+	// final writeJSON would miss the 120s write deadline, so the client sees a
+	// timeout and retries a report that already succeeded.
+	if err := rc.SetWriteDeadline(time.Now().Add(diagnosticsUploadReadTimeout)); err != nil {
+		h.diagnosticsLogger().WarnContext(r.Context(), "diagnostics upload write deadline not extended",
+			"component", "diagnostics",
+			"error", err,
+		)
+	}
 
 	userID, ok := diagnosticsUserID(w, r)
 	if !ok {

internal/api/router.go

@@ -255,14 +255,22 @@ func NewRouter(deps Dependencies) chi.Router {
 	// acting-admin profile policy, degrading admin routes to the plain
 	// role check.
 	var checkPrimaryProfile apimw.PrimaryProfileChecker
+	// lookupProfile resolves a profile for the given user, returning nil when it
+	// does not exist. Shared by the acting-admin primary check and the
+	// diagnostics profile-attribution validator so both read profile state
+	// (IsPrimary, IsChild) the same way.
+	var lookupProfile func(ctx context.Context, userID int, profileID string) (*userstore.Profile, error)
 	if deps.UserStoreProvider != nil {
 		userStores := deps.UserStoreProvider
-		checkPrimaryProfile = func(ctx context.Context, userID int, profileID string) (bool, bool, error) {
+		lookupProfile = func(ctx context.Context, userID int, profileID string) (*userstore.Profile, error) {
 			store, err := userStores.ForUser(ctx, userID)
 			if err != nil {
-				return false, false, err
+				return nil, err
 			}
-			profile, err := store.GetProfile(ctx, profileID)
+			return store.GetProfile(ctx, profileID)
+		}
+		checkPrimaryProfile = func(ctx context.Context, userID int, profileID string) (bool, bool, error) {
+			profile, err := lookupProfile(ctx, userID, profileID)
 			if err != nil {
 				return false, false, err
 			}
@@ -321,11 +329,20 @@ func NewRouter(deps Dependencies) chi.Router {
 			diagnosticsStore,
 			slog.Default(),
 		)
-		if checkPrimaryProfile != nil {
-			diagnosticsService.SetProfileAttributionValidator(diagnostics.ProfileAttributionValidatorFunc(
-				func(ctx context.Context, userID int, profileID string) (bool, error) {
-					_, found, err := checkPrimaryProfile(ctx, userID, profileID)
-					return found, err
+		if lookupProfile != nil {
+			// Attribute reports only to a profile that belongs to the account and
+			// is not a child profile; child profiles must not perform diagnostics
+			// actions per the design, so reject their attribution here.
+			diagnosticsService.SetProfileAttributionValidator(diagnostics.NewProfileAttributionValidator(
+				func(ctx context.Context, userID int, profileID string) (bool, bool, error) {
+					profile, err := lookupProfile(ctx, userID, profileID)
+					if err != nil {
+						return false, false, err
+					}
+					if profile == nil {
+						return false, false, nil
+					}
+					return true, profile.IsChild, nil
 				},
 			))
 		}

internal/diagnostics/service.go

@@ -90,6 +90,26 @@ func (f ProfileAttributionValidatorFunc) ProfileBelongsToUser(ctx context.Contex
 	return f(ctx, userID, profileID)
 }
 
+// ProfileLookup resolves whether profileID exists for userID and whether it is
+// a child profile. It backs NewProfileAttributionValidator.
+type ProfileLookup func(ctx context.Context, userID int, profileID string) (found bool, isChild bool, err error)
+
+// NewProfileAttributionValidator builds a ProfileAttributionValidator that
+// attributes a report to a profile only when it belongs to the user and is not
+// a child profile. The client diagnostics design forbids child profiles from
+// performing diagnostics actions, so a nonconforming client that sends a child
+// profile's ID (via X-Profile-Id or a manifest profile_id) has that attribution
+// rejected rather than recorded against the child.
+func NewProfileAttributionValidator(lookup ProfileLookup) ProfileAttributionValidator {
+	return ProfileAttributionValidatorFunc(func(ctx context.Context, userID int, profileID string) (bool, error) {
+		found, isChild, err := lookup(ctx, userID, profileID)
+		if err != nil {
+			return false, err
+		}
+		return found && !isChild, nil
+	})
+}
+
 func (s *Service) SetProfileAttributionValidator(validator ProfileAttributionValidator) {
 	s.profileValidator = validator
 }

internal/diagnostics/service_test.go

@@ -248,6 +248,45 @@ func TestServiceIngestStoresOnlyValidatedProfileAttribution(t *testing.T) {
 	}
 }
 
+func TestNewProfileAttributionValidatorRejectsChildProfiles(t *testing.T) {
+	tests := []struct {
+		name    string
+		found   bool
+		isChild bool
+		lookErr error
+		want    bool
+		wantErr bool
+	}{
+		{name: "non-child profile attributed", found: true, isChild: false, want: true},
+		{name: "child profile rejected", found: true, isChild: true, want: false},
+		{name: "missing profile rejected", found: false, isChild: false, want: false},
+		{name: "lookup error propagated", lookErr: errors.New("boom"), wantErr: true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			validator := NewProfileAttributionValidator(func(_ context.Context, userID int, profileID string) (bool, bool, error) {
+				if userID != 42 || profileID != "prof_1" {
+					t.Fatalf("lookup called with userID=%d profileID=%q", userID, profileID)
+				}
+				return tc.found, tc.isChild, tc.lookErr
+			})
+			got, err := validator.ProfileBelongsToUser(context.Background(), 42, "prof_1")
+			if tc.wantErr {
+				if err == nil {
+					t.Fatal("ProfileBelongsToUser error = nil, want error")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("ProfileBelongsToUser: %v", err)
+			}
+			if got != tc.want {
+				t.Fatalf("ProfileBelongsToUser = %v, want %v", got, tc.want)
+			}
+		})
+	}
+}
+
 func TestServiceIngestAcceptsManifestWithoutProfileID(t *testing.T) {
 	bundle, manifest, _ := testDiagnosticsUpload(t, "server-1", DefaultConsentNoticeVer, "")
 	repo := &fakeDiagnosticReportStore{}

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

@@ -40,6 +40,11 @@ describe("downloadDiagnosticReport", () => {
     );
     expect(click).toHaveBeenCalledOnce();
     expect(objectURL).toHaveBeenCalledOnce();
+    // The click spy records `this` as the anchor the download helper clicked;
+    // assert it carried the blob URL and expected filename before cleanup.
+    const clickedAnchor = click.mock.contexts[0] as HTMLAnchorElement;
+    expect(clickedAnchor.href).toBe("blob:diagnostic");
+    expect(clickedAnchor.download).toBe("silo-diagnostics-ABCDEF123456.tar.gz");
     expect(document.querySelector('a[download="silo-diagnostics-ABCDEF123456.tar.gz"]')).toBeNull();
 
     await new Promise((resolve) => window.setTimeout(resolve, 0));