Mcp improvements

src/django_smartbase_admin/actions/admin_action_list.py

@@ -832,11 +832,16 @@ def get_aggregates(self, aggregate, field_map=None, group_by=None):
         degenerate case of the same query.
 
         Without ``group_by`` returns a scalar ``{alias: value}`` dict; with
-        ``group_by`` returns a list of ``{**group_fields, **aliases}`` rows,
-        ordered by the group columns. Each metric runs in its own query and
-        is merged by group key, so a metric that traverses a to-many
-        relation (a multi-valued ``count``, or an annotated field that
-        aggregates a related set) can never fan-out-inflate a sibling.
+        ``group_by`` returns a list of
+        ``{"group": {name: value, ...}, "aggregates": {alias: value, ...}}``
+        rows, ordered by the group columns. The two namespaces are nested
+        rather than merged into one flat dict so a group column whose public
+        name equals a derived aggregate alias (e.g. a ``count`` field grouped
+        on while also aggregating ``count``) can't clobber the other — they
+        live under separate keys. Each metric runs in its own query and is
+        merged by group key, so a metric that traverses a to-many relation (a
+        multi-valued ``count``, or an annotated field that aggregates a related
+        set) can never fan-out-inflate a sibling.
         """
         specs = self.validate_aggregate(aggregate, field_map=field_map)
         group_fields = self.validate_group_by(group_by, field_map=field_map)
@@ -864,11 +869,20 @@ def get_aggregates(self, aggregate, field_map=None, group_by=None):
 
         # Resolve group targets the same way; a computed (annotated) group
         # column needs its expression present before ``.values()``.
+        # Keep the caller-facing group key (the declared name they grouped by)
+        # separate from the ORM target. They diverge when a declared field's
+        # public name differs from its DB column — e.g. an ``id_alias`` method
+        # field backed by ``@admin.display(ordering="id")`` resolves to ``id``.
+        # ``group_targets`` drives ``.values()`` / ``order_by`` / dedup; the
+        # response ``group`` dict is keyed by ``group_keys`` so
+        # ``group_by=["id_alias"]`` comes back under ``id_alias``, not ``id``.
+        group_keys: list[str] = []
         group_targets: list[str] = []
         for field in group_fields:
             target = (
                 field.model_field.name if field.model_field is not None else field.field
             )
+            group_keys.append(field.name)
             group_targets.append(target)
             if (
                 field.model_field is None
@@ -878,6 +892,7 @@ def get_aggregates(self, aggregate, field_map=None, group_by=None):
                 if field.supporting_annotates:
                     extra.update(field.supporting_annotates)
                 extra[target] = field.annotate
+
         if extra:
             queryset = queryset.annotate(**extra)
 
@@ -909,13 +924,20 @@ def get_aggregates(self, aggregate, field_map=None, group_by=None):
                     key = tuple(row[t] for t in group_targets)
                     slot = merged.get(key)
                     if slot is None:
-                        slot = {t: row[t] for t in group_targets}
+                        slot = {
+                            "group": {
+                                key_name: row[target]
+                                for key_name, target in zip(group_keys, group_targets)
+                            },
+                            "aggregates": {},
+                        }
                         merged[key] = slot
                         order.append(key)
-                    slot[alias] = row[alias]
+                    slot["aggregates"][alias] = row[alias]
         except FieldError as exc:
             raise ValueError(f"Cannot aggregate the requested field(s): {exc}") from exc
 
+        # No group dimension → unwrap to the bare scalar aggregate dict.
         if not group_targets:
-            return merged[order[0]] if order else {}
+            return merged[order[0]]["aggregates"] if order else {}
         return [merged[key] for key in order]

src/django_smartbase_admin/mcp/mcp.py

@@ -565,18 +565,22 @@ def list_rows(
             ``fn`` is one of ``sum / avg / min / max / count``; ``field``
             must be a declared column from ``list_admins`` —
             ``sum/avg/min/max`` need a numeric one, ``count`` may omit
-            ``field`` for a row count. Without ``group_by`` the results
-            land under ``aggregates``, keyed ``f"{fn}_{field}"`` (or
-            ``"count"``).
+            ``field`` for a row count. Each result is keyed by a derived
+            alias — ``f"{fn}_{field}"`` (or ``"count"`` for a bare count);
+            aliases can't be overridden. Without ``group_by`` the results
+            land under ``aggregates`` as a flat ``{alias: value}`` dict.
           group_by: optional list of declared column names to break the
             ``aggregate`` totals down by (SQL ``GROUP BY``) — e.g.
             ``group_by=["queue"]`` with ``aggregate=[{"fn": "count"}]`` for
             tickets-per-queue in one call instead of one call per queue.
             Multi-valued (relation) columns are rejected. Requires
             ``aggregate``. Results land under ``groups`` as a list of
-            ``{**group_columns, **aggregate_aliases}`` rows ordered by the
-            group columns; a column grouped on a foreign key comes back as
-            the bare pk (resolve names with ``autocomplete``).
+            ``{"group": {column: value, ...}, "aggregates": {alias: value,
+            ...}}`` rows ordered by the group columns — group columns and
+            aggregate aliases are nested under separate keys so a column whose
+            name equals an aggregate alias can't collide. A column grouped on a
+            foreign key comes back as the bare pk (resolve names with
+            ``autocomplete``).
 
         Returns ``{"data": [...], "last_page": int, "last_row": int}``
         plus any pagination metadata the list view emits, ``aggregates``

src/django_smartbase_admin/mcp/oauth/validators.py

@@ -31,20 +31,27 @@ def is_non_loopback_http(redirect_uri: str) -> bool:
 
 
 def loopback_redirect_allowed(redirect_uri: str, allowed_uris) -> bool:
-    """RFC 8252 §7.3 — match a loopback redirect URI ignoring its port.
+    """RFC 8252 §7.3 — match a loopback ``http`` redirect URI ignoring its port.
 
     Loopback hosts are treated as interchangeable (a client registered with
-    ``127.0.0.1`` may come back as ``localhost``); scheme, path and query must
-    still match a registered loopback URI exactly.
+    ``127.0.0.1`` may come back as ``localhost``); path and query must still
+    match a registered loopback URI exactly.
+
+    Port relaxation is confined to ``http`` — the only RFC 8252 §7.3 case and
+    the only scheme this validator deliberately permits for loopback hosts.
+    Custom/native schemes (``cursor://``, ``claude://``) stay on DOT's exact
+    match, which enforces ``ALLOWED_REDIRECT_URI_SCHEMES``; without this gate a
+    dynamically registered URI like ``javascript://localhost:1/callback`` could
+    slip past the scheme allowlist on a mere port mismatch.
     """
     requested = urlsplit(redirect_uri)
-    if requested.hostname not in LOOPBACK_HOSTS:
+    if requested.scheme != "http" or requested.hostname not in LOOPBACK_HOSTS:
         return False
     for allowed in allowed_uris:
         candidate = urlsplit(allowed)
         if (
-            candidate.hostname in LOOPBACK_HOSTS
-            and candidate.scheme == requested.scheme
+            candidate.scheme == "http"
+            and candidate.hostname in LOOPBACK_HOSTS
             and candidate.path == requested.path
             and candidate.query == requested.query
         ):

src/django_smartbase_admin/mcp/tests/test_aggregate.py

@@ -25,7 +25,7 @@ class _Admin(SBAdmin):
     search_fields = ("name",)
     # ``children`` is the reverse of the self-FK ``parent`` → a one-to-many
     # (multi-valued) field, used to exercise the join fan-out path.
-    sbadmin_list_display = ("id", "name", "id_alias", "parent", "children")
+    sbadmin_list_display = ("id", "name", "id_alias", "count", "parent", "children")
 
     # A method field with ``admin_order_field`` → backed by the ``id``
     # column via an annotation, so its ORM identifier is suffixed
@@ -34,6 +34,13 @@ class _Admin(SBAdmin):
     def id_alias(self, obj):
         return obj.id
 
+    # A method field whose public name equals the derived ``count`` aggregate
+    # alias — used to exercise that nesting keeps a group column and an
+    # equally-named aggregate from colliding.
+    @admin.display(ordering="id")
+    def count(self, obj):
+        return obj.id
+
 
 @override_settings(
     ROOT_URLCONF=__name__,
@@ -124,16 +131,67 @@ def test_group_by_breaks_totals_down_per_group(self):
 
         # No top-level scalar aggregates; a "groups" list instead, one row
         # per parent, ordered by the group column (NULLs first in sqlite).
+        # Group columns and aggregates are nested under separate keys.
         self.assertNotIn("aggregates", result)
         self.assertEqual(
             result["groups"],
             [
-                {"parent": None, "count": 2, "sum_id": a.pk + b.pk},
-                {"parent": a.pk, "count": 2, "sum_id": sum(a_ids)},
-                {"parent": b.pk, "count": 1, "sum_id": sum(b_ids)},
+                {
+                    "group": {"parent": None},
+                    "aggregates": {"count": 2, "sum_id": a.pk + b.pk},
+                },
+                {
+                    "group": {"parent": a.pk},
+                    "aggregates": {"count": 2, "sum_id": sum(a_ids)},
+                },
+                {
+                    "group": {"parent": b.pk},
+                    "aggregates": {"count": 1, "sum_id": sum(b_ids)},
+                },
             ],
         )
 
+    def test_group_by_keys_rows_by_requested_name_not_orm_target(self):
+        # ``id_alias`` is a method field whose ORM identifier is the annotation
+        # alias ``id_alias_annt``. Grouping by it must still surface rows under
+        # the agent-facing name ``id_alias`` (what the caller asked for), not
+        # the internal target — otherwise the group key the caller reads back
+        # doesn't match the field they grouped on.
+        ids = sorted(Folder.objects.create(name=n).pk for n in ("a", "b", "c"))
+
+        result = self._tools().list_rows(
+            "filer_folder",
+            fields=["id", "name"],
+            group_by=["id_alias"],
+            aggregate=[{"fn": "count"}],
+        )
+
+        self.assertNotIn("aggregates", result)
+        self.assertEqual(
+            result["groups"],
+            [{"group": {"id_alias": pk}, "aggregates": {"count": 1}} for pk in ids],
+        )
+
+    def test_group_key_equal_to_aggregate_alias_does_not_collide(self):
+        # ``count`` is a method field whose public name equals the derived
+        # ``count`` aggregate alias. Nesting the group column and the aggregate
+        # under separate keys means they no longer share a dict, so both
+        # survive — the group value under ``group.count`` and the metric under
+        # ``aggregates.count``.
+        ids = sorted(Folder.objects.create(name=n).pk for n in ("a", "b", "c"))
+
+        result = self._tools().list_rows(
+            "filer_folder",
+            fields=["id", "name"],
+            group_by=["count"],
+            aggregate=[{"fn": "count"}],
+        )
+
+        self.assertEqual(
+            result["groups"],
+            [{"group": {"count": pk}, "aggregates": {"count": 1}} for pk in ids],
+        )
+
     def test_aggregate_without_group_by_still_returns_scalar_dict(self):
         # Back-compat: the degenerate single-constant-group path keeps the
         # original scalar shape under "aggregates".

src/django_smartbase_admin/mcp/tests/test_oauth_redirect_loopback.py

@@ -81,6 +81,23 @@ def test_validator_ignores_port_for_loopback_redirects(self):
             loopback_redirect_allowed("http://evil.example:52756/callback", registered)
         )
 
+    def test_loopback_fallback_is_http_only(self):
+        """Port relaxation must not smuggle a non-http scheme past the allowlist.
+
+        DOT rejects a scheme outside ``ALLOWED_REDIRECT_URI_SCHEMES``; the
+        loopback fallback must not re-admit it just because a registered URI
+        shares the (non-http) scheme, loopback host, path and query but a
+        different port.
+        """
+        registered = ["javascript://localhost:1/callback"]
+        self.assertFalse(
+            loopback_redirect_allowed("javascript://localhost:9/callback", registered)
+        )
+        # Even an exact non-http loopback match goes through DOT, never here.
+        self.assertFalse(
+            loopback_redirect_allowed("javascript://localhost:1/callback", registered)
+        )
+
         # End-to-end through the validator: DOT's exact match fails on the
         # port, the loopback fallback recovers it; no client -> no fallback.
         validator = SBAdminMCPOAuth2Validator()